Skip to content

Remove dead betas plumbing - #101885

Merged
mountiny merged 16 commits into
Expensify:mainfrom
arekm213:arekm213/chore/remove-dead-betas
Sep 24, 2026
Merged

mountiny merged 16 commits into
Expensify:mainfrom
arekm213:arekm213/chore/remove-dead-betas

Conversation

@arekm213

@arekm213 arekm213 commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Explanation of Change

Follow up to #99444 and #100061. Those two PRs routed every beta check through usePermissions, which made something visible: the betas array is now read in only four places in src (Permissions.ts, usePermissions.ts, the beta overrides page, and the test mock). Everywhere else it is forwarded into parameters that no function body ever reads.

Four dead sinks carried it:

  • OpenReportActionParams.betas in src/libs/actions/Report/index.ts
  • getMoneyRequestOptions and canCreateRequest in src/libs/ReportUtils.ts
  • completePaymentOnboarding in src/libs/actions/IOU/PayMoneyRequest.ts
  • BuildPolicyDataOptions.betas in src/libs/actions/Policy/Policy.ts

They went dead in May, when #89469 removed the onboarding cohort gate and #91084 removed Manager McTest. Nothing flagged it, because an unused parameter sitting in the middle of a signature is invisible to both ESLint (no-unused-vars runs with args: 'after-used', which only reports trailing parameters) and TypeScript (noUnusedParameters is off).

The cost is not only dead code. 97 files still call useOnyx(ONYXKEYS.BETAS) purely to have something to pass along, so each of them re-renders whenever BETAS updates. This PR takes those 97 down to 2, leaving 3 live BETAS subscriptions in the app:

  • the createOnyxContext(ONYXKEYS.BETAS) in OnyxListItemProvider, which serves all 154 isBetaEnabled call sites through usePermissions
  • DeepLinkHandler, which keeps only the loading status (see below)
  • DynamicBetaOverridesPage, which deliberately reads the raw account value

The one thing that is not purely mechanical

betas also sat in the dependency list of 12 effects (10 useEffect, the useLayoutEffect in AmountSubmitDataSync, and the useFocusEffect in ConciergePage), plus one useCallback that is itself an effect dependency — fetchReport in ReportAddAttachmentModalContent. That is 13 effect triggers, of which 11 no longer re-run when BETAS arrives from Onyx.

The worry that deserves an answer is this: BETAS starts out undefined, so an effect could have run early, passed undefined down, and relied on the later betas change to run again with the real value. Losing that trigger would leave something computed from undefined.

That cannot happen here, for two reasons.

Nothing downstream can read the betas value at all. It is not just unused as a parameter, there is no second channel either. Outside useOnyx, ONYXKEYS.BETAS appears in exactly three places in src: the type declaration in ONYXKEYS.ts, the key list in ExportOnyxState, and the createOnyxContext call in OnyxListItemProvider. There is no module level Onyx.connectWithoutView on it and no helper that reads it, and nothing leaked to the server either: each sink builds its API parameters object field by field, with no ...params spread, and no type under src/libs/API/ has a betas field. So openReport(..., undefined) and openReport(..., ['all', ...]) do exactly the same thing, and after this PR the parameter does not exist at all.

The two effects that do resolve a beta keep a reactive dependency on it. AmountSubmitDataSync writes isASAPSubmitBetaEnabled: isBetaEnabled(CONST.BETAS.ASAP_SUBMIT) into a snapshot ref; its dependency array contains isBetaEnabled, which usePermissions returns from a useMemo keyed on [betas, betaConfiguration, betaOverrides], so when betas load that function gets a new identity and the effect re-runs with the correct value. DeepLinkHandler keeps its subscription for the same reason in a different shape: betasMetadata still gates isLoadingOnyxValue, so only the unused value is dropped — const [, betasMetadata] = useOnyx(ONYXKEYS.BETAS). In both cases the trigger is preserved through the channel that actually determines the result, rather than through the dead array.

Separately, 1 useCallback and 2 useMemo dependency lists lose betas as well. Those only change how often a value is recomputed, and I checked that none of them feeds an effect.

Test changes

Four tests go away, all of them duplicates or tautologies once betas is gone:

  • two in PromotedActionsBarTest that asserted betas was forwarded and became exact duplicates of the neighbouring introSelected tests
  • a describe('betas parameter handling') block in ReportUtilsTest that was mostly tautological
  • creates a new workspace when betas are explicitly passed in PolicyTest, which became a byte-for-byte duplicate of the BASIC approval mode test directly above it

An it.each in ReportTest parameterised over with betas and without betas collapsed to a plain it, since both cases became identical.

Seven tests in ReportTest are renamed. Their titles promised betas pass-through coverage that no longer exists, while the bodies still cover the real openReport and createChildReport paths, so only the names change.

Finally, GetMoneyRequestInformationTest carried betas: [] as Beta[] in a baseParams object spread into every call, even though getMoneyRequestInformation has no such parameter. That one is a third blind spot on top of the two above: TypeScript only checks for excess properties on fresh object literals, so spreading a pre-declared object hides the dead field.

Fixed Issues

$ #98409
PROPOSAL:

Tests

There is no new UI. Everything below should behave exactly as it does on main.
Steps 7 and 11 need a brand new account.

  1. Open a report from the LHN, then open a thread from an "X replies" link, and verify both load with their messages.
  2. Open a deep link to a report you have not opened in this session, both while signed in and from signed out, and verify it lands on the right report.
  3. Open an attachment on a report whose actions have not loaded yet and verify the report and the attachment carousel load.
  4. Open the composer + menu in a workspace chat, in a 1:1 DM and in a chat thread, and verify each offers the expense options you expect for that chat, then use Create report in the workspace chat.
  5. Use the FAB quick action and verify it offers the right action for the last thing you did, targeting the right chat.
  6. Reach Concierge from a few entry points (the /concierge route, Settings > About > Report a bug) and verify each one opens the Concierge chat.
  7. Create a new workspace and verify it is created with its #admins room and the submitter's expense chat.
  8. Pay someone from a 1:1 DM on an account that has never paid before, and verify the payment completes and onboarding is not re-triggered.
  9. Leave a workspace room and verify you stay on it with a Join button; leave a group chat and verify you are navigated to Concierge.
  10. Delete a report you are currently viewing and verify you land on its parent report or on Concierge.
  11. On a fresh account that has not viewed the tour, start the test drive from the Concierge checklist and verify the demo loads.
  • Verify that no errors appear in the JS console

Offline tests

Same as Tests. Nothing here depends on network state.

QA Steps

Same as Tests.

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari

The betas array is threaded through ~130 files but read in only four
places in src: Permissions.ts, usePermissions.ts, the test mock and the
beta overrides page. Everywhere else it is forwarded into parameters
that no function body reads.

Remove the four dead sinks that carried it (OpenReportActionParams.betas,
getMoneyRequestOptions and canCreateRequest, completePaymentOnboarding,
BuildPolicyDataOptions.betas) and the useOnyx(ONYXKEYS.BETAS)
subscriptions that existed only to feed them, taking those from 97 files
down to 2.

Also drop a stale {virtual: true} on the RecentlyAddedSection mock in
HomePage.test.tsx, which stopped the mock from ever resolving.
…ve-dead-betas

# Conflicts:
#	src/libs/actions/Task.ts
@arekm213

Copy link
Copy Markdown
Contributor Author

@MelvinBot review

@MelvinBot

Copy link
Copy Markdown
Contributor

Review: LGTM, with two notes

The core claim holds up. betas is genuinely dead in every sink you removed, and nothing downstream can read it.

What I verified locally at 0ba3a65:

  • npm run typecheck passes on all five projects.
  • Lint is clean on the highest-risk changed files (Report/index.ts, ReportUtils.ts, DeepLinkHandler.tsx, ReportFetchHandler.tsx, AmountSubmitDataSync.tsx, ConciergePage.tsx, MoneyRequestReportActionsList.tsx, ReportAddAttachmentModalContent, DynamicReportDetailsPage.tsx, useReportActionsScroll.ts).
  • All 34 changed Jest suites pass — 3458 tests, 0 failures.
  • grep -rn "betas" src/libs/API/ returns nothing, so the value never reached the server. Your "no ...params spread" argument checks out.
  • Only three useOnyx(ONYXKEYS.BETAS) subscriptions remain, plus the createOnyxContext one.
  • Your effect-dependency reasoning is right where it matters. src/hooks/usePermissions.ts:37 memoizes on [betas, betaConfiguration, betaOverrides], so AmountSubmitDataSync still re-runs through isBetaEnabled when betas load. DeepLinkHandler still keeps betasMetadata.status in its dep array at src/DeepLinkHandler.tsx:168-176.

1. The HomePage.test.tsx mock change is not needed (please drop it)

Your description says the virtual mock never resolved, so the real RecentlyAddedSection rendered, and that only started failing once this PR touched the file. That does not reproduce.

I restored {virtual: true} on top of your branch and ran the suite with --no-cache: 15/15 pass. The mock applies either way — Jest's virtual flag permits mocking a non-existent module, it does not prevent a real one from being mocked.

The test itself confirms this. It asserts getByTestId('section-RecentlyAddedSection') at tests/unit/pages/HomePage.test.tsx:312, and only mockSection renders that testID. If the real component had been rendering, the suite would already have been failing on main.

Dropping tests/unit/pages/HomePage.test.tsx:104 keeps the PR purely mechanical and lets you delete the "A test fix that is not about betas" section. If you'd rather keep it as cleanup, that's fine too — just correct the rationale.

2. Description nit: it's 3 subscriptions, not 2

src/DeepLinkHandler.tsx:45 still holds const [, betasMetadata] = useOnyx(ONYXKEYS.BETAS). The code is right — you need that status to keep the load gate — but the count in the description should say 3.

3. Optional: one assertion worth keeping

The deleted describe('betas parameter handling') block was mostly tautological (expect(typeof withEmptyBetas).toBe('boolean')), so removing it is the right call. The one line with independent value was canCreateRequest(report, undefined, SUBMIT, false, ...) === true for a 1:1 DM. You could keep that as a de-betas'd one-liner. Your call — the surrounding temporary_getMoneyRequestOptions cases probably cover it.


Positional-argument safety

The removals that shift positional arguments (completePaymentOnboarding, getMoneyRequestOptions, temporary_getMoneyRequestOptions, canCreateRequest, navigateToAndOpenChildReport, createChildReport, navigateToAndOpenReportWithAccountIDs, leaveRoom, leaveGroupChat, navigateToMostRecentReport, navigateToConciergeChatAndDeleteReport, clearCreateChatError) are the part a reviewer should not take on faith, since a missed call site can silently shift a defaulted trailing parameter rather than erroring.

I checked every src call site of the ReportUtils three and all pass the new arity with rules in the right slot. For the rest, the adjacent parameter types are incompatible with OnyxEntry<Beta[]>, so typecheck would have caught a miss — and it's clean. Dropping the now-unneeded @typescript-eslint/max-params disable on navigateToConciergeChatAndDeleteReport is a nice touch.

Commands run
npm run typecheck                                  # passed, 5 projects
npm run lint -- <10 highest-risk changed files>    # clean
npm test -- <all 34 changed test files>            # 3458 passed, 2 skipped

I reviewed this statically and through the local test suite. I could not verify the 11 manual test steps in a browser this run — the web environment wasn't available — so the runtime behavior of the deep-link, Concierge, and payment-onboarding paths still needs a human pass.

…l sites

Both came in with the main merge and passed [CONST.BETAS.ALL] in the
position that is now rules.
The mock resolves either way, so the change was unnecessary.
…ve-dead-betas

# Conflicts:
#	src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx
@arekm213

Copy link
Copy Markdown
Contributor Author

@MelvinBot review

…ve-dead-betas

# Conflicts:
#	src/pages/inbox/report/ContextMenu/BaseReportActionContextMenu.tsx
@arekm213

Copy link
Copy Markdown
Contributor Author

@MelvinBot review again after merge 😅

@MelvinBot

Copy link
Copy Markdown
Contributor

Re-review at f4a9962: still LGTM

Both notes from my last pass are resolved, and the two merges from main didn't break anything. One new nit, one optional sweep-up.

Resolved:

  • 6238f2f restores {virtual: true}, so HomePage.test.tsx is back to untouched. I re-ran it at this head with RecentlyAddedSection still modified — 15/15 pass, which closes that thread.
  • 4b2678 catches the two new temporary_getMoneyRequestOptions call sites main added. Good catch; that's exactly the failure mode worth watching on every re-merge.
  • Description now says "97 down to 2, leaving 3 live BETAS subscriptions." Accurate.

Re-verified at this head: npm run typecheck passes on all five projects; lint clean on the 13 highest-risk changed files; all 34 changed Jest suites pass (3469 tests). CI agrees — typecheck, ESLint, and all 8 test shards are green. Only baseline-perf-tests / branch-perf-tests are still running, plus Check independent approval, which is an approval-policy gate rather than a code signal. Still 3 useOnyx(ONYXKEYS.BETAS) in src after both merges — main added none.


1. Eight test names still advertise betas coverage that no longer exists

These titles say the test asserts betas pass-through, but betas is gone from every body — tests/actions/ReportTest.ts now contains zero occurrences of betas outside these seven title strings.

The tests themselves are still worth keeping — they cover the real openReport / createChildReport / createWorkspace paths. They just need names that describe what they now assert. You already edit all eight in this PR, so renaming is in scope and is the last place a reader could still believe betas plumbing exists.

2. Optional: one dead betas fixture your sweep couldn't see

tests/actions/IOU/GetMoneyRequestInformationTest.ts:71 carries betas: [] as Beta[] in the baseParams object that every case spreads into getMoneyRequestInformation. That function has no betas parameter — MoneyRequestBuilder.ts contains no occurrence of the word.

This one predates your PR (neither file is in your diff), so it's genuinely optional. It's interesting because it's a third blind spot beyond the two you named: spreading a pre-declared object skips excess-property checking, so TypeScript stays silent where a fresh inline literal would have errored. That's the same "invisible to both ESLint and TypeScript" story as unused middle parameters, and it fits the PR's theme. Removing the property also lets you drop Beta from the type import on line 5.


What I ran at f4a9962
npm run typecheck                          # passed, 5 projects
npm run lint -- <13 changed files>         # clean
npm test -- <all 34 changed test files>    # 3469 passed, 2 skipped
npm test -- tests/unit/pages/HomePage.test.tsx   # 15 passed, with {virtual: true} restored
grep -rn "ONYXKEYS.BETAS" src/             # 3 subscriptions, unchanged by the merges
grep -rn "betas" src/libs/API/             # no matches

I also re-walked every positional call site of the signatures that lost a parameter (temporary_getMoneyRequestOptions, canCreateRequest, completePaymentOnboarding, navigateToAndOpenChildReport, createChildReport, leaveRoom, leaveGroupChat, navigateToMostRecentReport, navigateToConciergeChatAndDeleteReport, clearCreateChatError, navigateToAndOpenReportWithAccountIDs). All match the new arity post-merge.

I reviewed this statically and through the local and CI test suites. I could not drive the app in a browser this run, so the 11 manual test steps still need a human pass.

@codecov

codecov Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.

Files with missing lines Coverage Δ
src/DeepLinkHandler.tsx 91.80% <100.00%> (-3.51%) ⬇️
src/components/AddPaymentMethodMenu.tsx 100.00% <100.00%> (ø)
...neProvider/HTMLRenderers/ConciergeLinkRenderer.tsx 0.00% <ø> (ø)
src/components/KYCWall/BaseKYCWall.tsx 29.89% <ø> (-0.38%) ⬇️
...eaderActions/MoneyReportHeaderSecondaryActions.tsx 0.66% <ø> (+<0.01%) ⬆️
...oneyReportHeaderPrimaryAction/PayPrimaryAction.tsx 0.00% <ø> (ø)
...derPrimaryAction/ReviewDuplicatesPrimaryAction.tsx 0.00% <ø> (ø)
.../components/MoneyRequestHeaderSecondaryActions.tsx 0.44% <ø> (+<0.01%) ⬆️
...tView/MoneyRequestReportTransactionsNavigation.tsx 67.82% <ø> (-0.28%) ⬇️
...eyRequestReportView/useMoneyRequestReportScroll.ts 96.34% <ø> (-0.05%) ⬇️
... and 120 more
... and 135 files with indirect coverage changes

@MelvinBot

Copy link
Copy Markdown
Contributor

Re-review after merge: still LGTM, nothing broke

Both earlier findings are addressed, the three merges re-applied cleanly, and everything is green at 40b0ec1.

Prior findings resolved

  • {virtual: true} restored on the RecentlyAddedSection mock (6238f2f). tests/unit/pages/HomePage.test.tsx is no longer in the diff at all, so the PR is now purely mechanical.
  • Description corrected — it now lists all three live BETAS subscriptions instead of claiming two.

The merges landed correctly. Main split two of the files you'd already edited, and the removal was re-applied to both:

4b26789 handles the two new temporary_getMoneyRequestOptions call sites main added. Both drop the arg correctly: undefined lands in rules, and the trailing defaults are unchanged.

No plumbing leaked back in. grep -rn "ONYXKEYS.BETAS" src/ still returns exactly the same four hits as before the merges — the context provider, the metadata-only read in DeepLinkHandler, the overrides page, and the type declaration.

What I ran at 40b0ec1:

Check Result
npm run typecheck passed, 5 projects
npm run lint on all 165 changed files clean
33 changed Jest suites 3442 passed, 2 skipped, 0 failed
MoneyRequestReportTransactionsNavigationTest + HomePage.test 32 passed

The last row matters most for a merge review — MoneyRequestReportTransactionsNavigationTest.tsx is a new main-side suite (221 lines, arrived in f4a9962) that exercises code this PR edits. It passes.

CI agrees: typecheck, ESLint, all 8 test shards, perf, React Compiler, knip, and codecov are green. The only red check is Check independent approval, which is the approval gate, not a code failure.

Conflict-resolution audit

The risk with three merges into a 167-file mechanical PR is a resolution that silently reverts a main-side change. I scanned the full merged diff for it: of 1648 changed lines, 471 contain no beta substring. Every one of those is accounted for — Prettier reflow where a multi-line call collapsed after losing an argument, positional undefined placeholders being dropped, and the removed duplicate tests. No main-side code was clobbered.

One nit left: the description still says scrollToLatestMessages is in MoneyRequestReportActionsList. Main moved it to useMoneyRequestReportScroll.ts, so that reference (and possibly the "12 effects / 14 triggers" count) is stale post-merge. Cosmetic.

I reviewed this statically and through the test suite. I could not drive the app in a browser this run, so your 11 manual test steps still need a human pass.

The bodies no longer touch betas, so the titles described coverage that
does not exist. The tests still cover the openReport and createChildReport
paths, so only the names change.
getMoneyRequestInformation has no betas parameter. The property survived
because baseParams is spread into the call, and spreading a declared
object skips excess property checking.
Once betas is gone the test is byte-for-byte the same call and the same
assertions as the BASIC approval mode test above it.
@arekm213
arekm213 marked this pull request as ready for review September 22, 2026 16:01
@arekm213
arekm213 requested review from a team as code owners September 22, 2026 16:01
@melvin-bot
melvin-bot Bot requested review from garrettmknight and huult and removed request for a team September 22, 2026 16:01
@melvin-bot

melvin-bot Bot commented Sep 22, 2026

Copy link
Copy Markdown

@huult Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

@arekm213

Copy link
Copy Markdown
Contributor Author

@huult its ready for review

@garrettmknight
garrettmknight removed their request for review September 22, 2026 16:50
@mountiny
mountiny self-requested a review September 22, 2026 17:45
…ve-dead-betas

# Conflicts:
#	src/libs/actions/Link.ts
#	src/libs/actions/Report/index.ts
#	src/pages/DynamicReportDetailsPage.tsx
@huult

huult commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Reviewer Checklist

  • I have verified the author checklist is complete (all boxes are checked off).
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified that the composer does not automatically focus or open the keyboard on mobile unless explicitly intended. This includes checking that returning the app from the background does not unexpectedly open the keyboard.
  • I verified tests pass on all platforms & I tested again on:
    • Android: HybridApp
    • Android: mWeb Chrome
    • iOS: HybridApp
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • For any bug fix or new feature in this PR, I verified that sufficient unit tests are included to prevent regressions in this flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: HybridApp
Screen.Recording.2026-09-24.at.12.29.59.mov
Android: mWeb Chrome
Screen.Recording.2026-09-24.at.12.32.17.mov
iOS: HybridApp
Screen.Recording.2026-09-24.at.12.33.51.mov
iOS: mWeb Safari
Screen.Recording.2026-09-24.at.12.34.30.mov
MacOS: Chrome / Safari
Screen.Recording.2026-09-24.at.12.16.34.mov

@huult

huult commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

I’ll take a look soon.

It fits within the line limit once betas is gone, which is what oxfmt
expects.
@arekm213

Copy link
Copy Markdown
Contributor Author

@huult friendly bump

@huult

huult commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

@arekm213 I think I can start reviewing this PR in about 2 hours

…ve-dead-betas

# Conflicts:
#	src/components/SettlementButton/index.tsx
#	src/pages/home/TimeSensitiveSection/items/UnlockBankAccount.tsx
#	src/pages/settings/Wallet/WalletPage/index.tsx
#	src/pages/workspace/workflows/tabs/WorkflowsPaymentsTab.tsx
Auto-merge kept main's canCreateRequest call, which still passed allBetas
after this branch removed the parameter.
@huult

huult commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

@arekm213 You still need to update this PR, right? If so, go ahead and make the changes. I’ll review it again afterward.

@arekm213

arekm213 commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor Author

@huult i had to resolve conflicts that just appeared hour ago, it has a lot of files changes so i would be happy if we could actively work on reviewing it now and get it ready to merge soon - in few minutes it will be ready for review again

Main added this suite for the extracted hook and asserted betas was
forwarded to openReport, which this branch removes.
@arekm213

Copy link
Copy Markdown
Contributor Author

@huult its ready now

@huult

huult commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

@MelvinBot review this

@MelvinBot

Copy link
Copy Markdown
Contributor

Review at 3abae09: LGTM — two trivial leftovers, one question

The change is sound. betas really is dead in every sink, no live logic came out with it, and every call site that lost a positional argument is correct after both merges from main. Nothing here should block you.

Both findings from my last pass are resolved: tests/actions/ReportTest.ts now contains zero occurrences of betas, including in test titles, and the GetMoneyRequestInformationTest fixture is gone (2a2301e).


1. Two orphaned JSDoc comments now document the wrong field

The betas property was deleted but its doc comment was not, so it now sits above the next field:

Every other site deleted the comment, the field, and the trailing blank line together. These two are the only misses.

2. Question: the DeepLinkHandler gate now waits on a key nothing reads

src/DeepLinkHandler.tsx:47 keeps const [, betasMetadata] = useOnyx(ONYXKEYS.BETAS) purely to feed isLoadingOnyxValue at :74 and the dep array at :195. Before this PR that gate had a reason — the value was passed down. Now deep-link processing still blocks on a key no one in that path consumes.

Keeping it is the conservative call, and the delay is small. But it is the last piece of dead betas plumbing in the PR, so either drop it or add a one-line comment saying why it stays.

3. Nit: a redundant trailing undefined in two tests

tests/actions/ReportTest.ts:9158 and :9185 still pass 8 arguments to navigateToAndOpenReportWithAccountIDs. Behavior is unchanged — the last undefined used to land in conciergeChat and now lands in shouldRevalidateExistingChat, which defaults to false on undefined. It reads confusingly next to the sibling test at :9212, which passes true in that same slot. Dropping the last argument from both would make the three calls consistent.

4. Description accuracy

The PR says the deleted PolicyTest case was "a byte-for-byte duplicate" of the BASIC approval mode test above it. It wasn't quite — it also asserted policy?.id and policy?.name. Deleting it is still right, because both are covered by creates a new workspace at tests/actions/PolicyTest.ts:204. Only the wording overstates.


Positional-argument sweep — the part not to take on faith

17 functions lost a positional betas parameter. I checked every call site in src/ and tests/; all pass the new arity with the right value in each slot.

The risk worth naming: for 15 of the 17, the parameter that followed betas accepts undefined, so a missed call site that still passed undefined in that slot would compile cleanly and silently shift. Typecheck cannot protect those. I hand-checked each one. Two calls were genuinely left unedited — ReportTest.ts:9158/:9185 (note 3 above) and ReportTest.ts:8605 for handleWalletStatementNavigation, whose trailing undefined now lands in type?. Both are benign: the wallet test is literally "should do nothing when type is undefined".

Only two removals are protected by typecheck outright, because the next parameter is a required number: openReportFromDeepLink → callerAccountID and completePaymentOnboarding → currentUserAccountID. That is also why the merge at 30e029b8 surfaced as a hard error in QuickActionMenuItem and got fixed in d656b86 — the rules slot rejected allBetas.

The [8] → [7] index shift in DeepLinkHandlerTest is correct. openReportFromDeepLink now takes 8 parameters, and index 7 is reportNameValuePairs — exactly what the test asserts.

Effect triggers and coverage

I swept every hook that lost betas from a dependency array and checked whether its body still reads a beta through any channel.

Four do, and all four keep isBetaEnabled in their deps, which gets a new identity when betas load because usePermissions memoizes on [betas, betaConfiguration, betaOverrides]: AmountSubmitDataSync, ExpenseReportListItem, useSearchBulkActions, and useAutoCreateTrackWorkspace. The rest read no beta at all. No effect lost its only reactive trigger.

I also read the current code for the 16 files with outsized deletions (Report/index.ts, ContextMenuActions.tsx, App.ts, Search.ts, PayMoneyRequest.ts, replaceOptimisticReportWithActualReport.ts, DynamicReportDetailsPage.tsx, ReportActionItemThread.tsx, QuickActionMenuItem.tsx, DeepLinkHandler.tsx, and others). Every removed line that did not mention betas is Prettier reflow, a dropped OnyxEntry type import, or the now-unneeded max-params eslint-disable on navigateToConciergeChatAndDeleteReport. No conditional, early return, or non-betas dependency was touched.

On the deleted tests: no real coverage loss. The canCreateRequest(1:1 DM, SUBMIT) === true case I flagged last time is covered by return multiple expense options if › it is a 1:1 DM at tests/unit/ReportUtilsTest.ts:4941, which asserts the same thing more strictly. The two PromotedActionsBarTest deletions are line-for-line duplicates of the surviving introSelected tests. The it.each over with betas / without betas collapsed correctly — betas appeared in exactly one place in that body.

What I ran at 3abae09
npx tsc --noEmit -p tsconfig.json   # clean
grep -rn "ONYXKEYS.BETAS" src/      # 3 subscriptions + the createOnyxContext, unchanged by both merges
grep -rn "betas" tests/actions/ReportTest.ts   # 0 matches, titles included

CI is green at this exact head (3abae09f): typecheck, ESLint, Oxfmt, React Compiler Compliance, knip, spellcheck, Storybook, both perf jobs, and all 8 test shards. The only red check is Check independent approval, which is an approval-policy gate rather than a code signal.


I reviewed this statically and against CI. I could not drive the app in a browser this run, so the 11 manual test steps — deep links, Concierge entry points, payment onboarding, and the test drive — still need a human pass.

Comment thread src/libs/SearchUIUtils.ts Outdated
@@ -2431,7 +2431,6 @@ type CreateAndOpenSearchTransactionThreadParams = {
currentUserAccountID: number;

/** Beta features list */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arekm213 Did you miss this comment? Please remove it.

@@ -45,7 +45,6 @@ type WithReportOrNotFoundOnyxProps = {
policy: OnyxEntry<OnyxTypes.Policy>;

/** Beta features list */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arekm213 Did you miss this comment? Please remove it.

@huult

huult commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

@arekm213 Could you remove the two unused pieces of code above, then tag me to approve this PR?

Both described the betas field that this branch already deleted.
@arekm213

Copy link
Copy Markdown
Contributor Author

@huult done!

@huult huult left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@arekm213

Copy link
Copy Markdown
Contributor Author

@mountiny its ready for your review now, thanks @huult

@mountiny
mountiny merged commit 1e3ca2f into Expensify:main Sep 24, 2026
35 of 37 checks passed
@OSBotify

Copy link
Copy Markdown
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@github-actions

Copy link
Copy Markdown
Contributor

🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here.

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.94-0 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 success ✅
🍎 iOS 🍎 success ✅

@MelvinBot

Copy link
Copy Markdown
Contributor

No help site changes are required for this PR, so I did not create a docs PR.

This PR removes an unused betas parameter and drops the ONYXKEYS.BETAS subscriptions that only existed to pass it along. It changes no user-facing behavior:

  • No UI copy changed. The 100 changed files include no src/languages/* files.
  • No features, settings, buttons, or flows changed. The PR's test steps confirm that everything should behave exactly as it does on main.
  • No API parameters changed, so no server-side behavior changed either.

Nothing in App/docs/articles describes the internal code this PR touches, so no article is now out of date.


view run

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by https://github.com/Julesssss in version: 9.4.94-0 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 success ✅
🍎 iOS 🍎 success ✅

Bundle Size Analysis (Sentry):

@MelvinBot MelvinBot mentioned this pull request Oct 6, 2026
40 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants