Skip to content

Gate deep-link navigation on TabNavigator mount via lifecycle signal - #92482

Merged
yuwenmemon merged 6 commits into
mainfrom
claude-fixTabNavigatorDeepLinkRaceV2
Jul 9, 2026
Merged

yuwenmemon merged 6 commits into
mainfrom
claude-fixTabNavigatorDeepLinkRaceV2

Conversation

@MelvinBot

@MelvinBot MelvinBot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Explanation of Change

This re-does the fix from #91796 (which was reverted for causing regressions) without touching the shared navigation primitive that caused the blast radius.

Background: TAB_NAVIGATOR is declared on the root navigator as soon as the authenticated RootStack renders, but it is loaded lazily (getComponent), so there is a window where the screen is declared while its child router has not run useNavigationBuilder yet. A deep link or push notification that dispatches a nested NAVIGATE during that window is "not handled by any navigator" and is silently dropped.

Why #91796 was reverted: it gated the shared waitForProtectedRoutes() primitive in Navigation.ts on a state-shape inference (isTabNavigatorReady). On a quiet cold start the tab's non-stale nested state was never pushed up to the root navigator's own state object and no further root state event fired, so the promise never resolved and navigation hung. Because waitForProtectedRoutes() is shared by many flows, that hang surfaced as multiple regressions.

This PR's approach:

  • Add tabNavigatorReadiness.ts — a lifecycle-driven readiness signal. TabNavigator flips it on mount and resets it on unmount, so it reliably reflects the current mount and re-arms across a logout → login cycle. It can't get stuck waiting on a state event that never arrives.
  • Wire the mount/unmount signal into TabNavigator.tsx and TabNavigator.native.tsx.
  • Gate the deep-link navigate in openReportFromDeepLink (Link.ts) on whenTabNavigatorReady() after waitForProtectedRoutes(), instead of gating inside the shared primitive. The user is over the auth wall here, so TAB_NAVIGATOR is guaranteed to mount.
  • In the linking subscription (subscribe.ts), defer (don't drop) the URL until the tab router mounts. On public screens TAB_NAVIGATOR isn't declared, so the URL is forwarded immediately.
  • Navigation.ts is intentionally left untouched.

Added a unit test for the readiness helper (mount resolves a pending promise, immediate resolve once mounted, re-arm after unmount, idempotent mount, no-op unmount).

Fixed Issues

$ #91777
$ #92023
$ #92039
$ #92030
$ #92041
PROPOSAL: #91777 (comment)

Tests

Same as QA Tests.

  • Verify that no errors appear in the JS console

Offline tests

Same as QA Tests.

QA Steps

  1. Log in to the app
  2. Copy a conversation/chat link
  3. Log out from the app
  4. Open the conversation/chat link from step 2
  5. Verify that: There is no TabNavigator console error
  • 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 any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • 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 shown in the product is localized by adding it to src/languages/* files and using the translation method
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • 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)
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • 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)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.ts or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • 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.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • 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

TAB_NAVIGATOR is declared on the root navigator before its lazily-loaded
child router mounts. A deep link or notification that dispatches a nested
NAVIGATE during that window is unhandled and silently dropped.

Replace the prior state-shape inference (which could hang on a quiet cold
start) with a lifecycle-driven readiness signal that TabNavigator flips on
mount and resets on unmount, so it re-arms across a logout/login cycle.
Gate the deep-link navigate in openReportFromDeepLink and defer (don't drop)
URLs in the linking subscription until the tab router mounts, leaving the
shared waitForProtectedRoutes primitive untouched.

Co-authored-by: daledah <daledah@users.noreply.github.com>
@MelvinBot
MelvinBot requested a review from a team June 3, 2026 08:34
@MelvinBot

Copy link
Copy Markdown
Contributor Author

I created this PR from an upstream branch since I don't have push access to your fork.

To take ownership of this branch and be able to push updates, run:

git remote add upstream https://github.com/Expensify/App.git
git fetch upstream claude-fixTabNavigatorDeepLinkRaceV2
git checkout -b claude-fixTabNavigatorDeepLinkRaceV2 upstream/claude-fixTabNavigatorDeepLinkRaceV2
git push -u origin claude-fixTabNavigatorDeepLinkRaceV2

Then you can close this PR and open a new one from your fork.

@MelvinBot

Copy link
Copy Markdown
Contributor Author

I checked the CI — there's no code fix needed. Every automated check on this PR passes; the only red check is the PR Author Checklist, which is the human-only checklist (it fails simply because the boxes aren't ticked yet).

I verified the functional checks both in CI and locally on this branch (2323a6d):

  • Jest Unit Tests (all 8 shards) + Storybook tests — passed in CI
  • TypeScript, ESLint, Prettier, React Compiler, knip, unused styles, spellcheck, all builds — passed in CI
  • Locally I ran the suites most relevant to the changed navigation files — all green:
    • tests/unit/tabNavigatorReadinessTest.ts — 6/6
    • tests/unit/DeeplinkRouteNormalizationTest.ts + tests/unit/Navigation/** — 300/300
    • branch-perf-tests passed in CI (baseline-perf-tests reference run on main was still finishing — not a failure of this PR)

What's left (human action, not a code fix): before marking the PR ready for review, the co-author needs to fill out the Tests/QA sections and tick the boxes in the PR Author Checklist. That will clear the only remaining red check.

@codecov

codecov Bot commented Jun 3, 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 Δ
...on/AppNavigator/Navigators/TabNavigator.native.tsx 97.05% <100.00%> (+0.18%) ⬆️
...c/libs/Navigation/helpers/tabNavigatorReadiness.ts 100.00% <100.00%> (ø)
...avigation/AppNavigator/Navigators/TabNavigator.tsx 0.00% <0.00%> (ø)
src/libs/Navigation/linkingConfig/subscribe.ts 16.66% <0.00%> (-6.42%) ⬇️
src/libs/actions/Link.ts 28.35% <2.38%> (-0.29%) ⬇️
... and 12 files with indirect coverage changes

@daledah

daledah commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2323a6dc02

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +37 to +42
function setTabNavigatorUnmounted() {
if (!isMounted) {
return;
}
isMounted = false;
resetReadyPromise();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Track mounted TabNavigators instead of a single boolean

This reset runs for any TabNavigator unmount, but the root stack can contain more than one TAB_NAVIGATOR at once (e.g. Navigation.ts documents that cross-tab PUSH stacks a new TAB_NAVIGATOR, and the workspace/domain split handler pushes one). If a pushed tab navigator is later popped while an underlying tab navigator is still mounted, this cleanup sets isMounted to false and creates a fresh unresolved promise; future deep links/notifications that call whenTabNavigatorReady() will wait indefinitely because the remaining mounted navigator will not mount again to resolve it. This needs to be ref-counted or tied to the currently mounted instances rather than a single boolean.

Useful? React with 👍 / 👎.

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.

@MelvinBot this makes sense, please update as the comment above

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 26b8471. Replaced the single isMounted boolean with a ref count so the signal tracks every mounted TAB_NAVIGATOR instead of just the latest one:

  • tabNavigatorReadiness.tssetTabNavigatorMounted() increments and only resolves the promise on the 0 → 1 transition; setTabNavigatorUnmounted() decrements and only re-arms the promise on the 1 → 0 transition (with a guard against going negative on an unbalanced call). isTabNavigatorMounted() / whenTabNavigatorReady() now report ready while mountedCount > 0.

So popping a pushed TAB_NAVIGATOR while another is still mounted keeps the signal ready, and a deep link / notification that calls whenTabNavigatorReady() resolves immediately instead of waiting forever.

Added unit coverage for both: a stacked mount → pop-one stays ready → pop-last re-arms, and an unbalanced unmount that must not push the count below zero.

Verification
  • npm test -- tests/unit/tabNavigatorReadinessTest.ts — 6/6 pass
  • eslint on both changed files — clean
  • prettier — clean
  • typecheck-tsgo — clean

@daledah

daledah commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

@MelvinBot merge main

…rDeepLinkRaceV2

Co-authored-by: daledah <daledah@users.noreply.github.com>
@MelvinBot

Copy link
Copy Markdown
Contributor Author

🤖 Merged latest main into this branch (no conflicts). The branch is now up to date.

Merge commit: 165a49d0413

@MelvinBot

Copy link
Copy Markdown
Contributor Author

🤖 I analyzed all the CI checks on this PR — there are no code-related failures to fix.

Every code check passed: TypeScript, Jest unit tests (all 8 jobs), ESLint, Prettier, Knip, spell check, React Compiler Compliance, and the Android/iOS builds.

The only two failing checks are:

  • PR Author Checklist
  • PR Reviewer Checklist

These fail only because their checkboxes aren't ticked yet — they're completed by a human, not by code. The PR body itself notes that the human co-author must fill out the Tests / Offline tests / QA Steps sections and check the boxes before marking the PR "Ready for review". MelvinBot can't (and shouldn't) auto-complete those, so there's nothing for me to push.

(The Reassure performance tests were still running when I checked — that's an informational benchmark comparison, not a code-fixable check.)

✅ No code fix is needed. Once the human co-author fills out the checklist and ticks the boxes, those checks will pass.

@daledah

daledah commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

@MelvinBot please add these issues to the Fixed issues section

#92023
#92039
#92030
#92041

and @MelvinBot update your checklist:

  • Check all checkboxes.
  • Update the "Tests" and "Offline Tests" sections to: Same as QA Tests.
  • Update the "QA Tests" section:
  1. Log in to the app
  2. Copy a conversation/chat link
  3. Log out from the app
  4. Open the conversation/chat link from step 2
  5. Verify that: There is no TabNavigator console error

@MelvinBot

Copy link
Copy Markdown
Contributor Author

The readiness signal used a single boolean, but multiple TAB_NAVIGATORs can
be mounted on the root stack at once (cross-tab PUSH / workspace split). Popping
a pushed navigator reset the signal while another stayed mounted, so future deep
links waited on a promise that never resolved. Track mounted instances with a ref
count so the signal stays ready until the last instance unmounts.

Co-authored-by: daledah <daledah@users.noreply.github.com>
@daledah

daledah commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

@MelvinBot mark this PR as ready for review

@MelvinBot
MelvinBot marked this pull request as ready for review June 5, 2026 07:11
@MelvinBot
MelvinBot requested a review from a team as a code owner June 5, 2026 07:11
@melvin-bot

melvin-bot Bot commented Jun 5, 2026

Copy link
Copy Markdown

@sobitneupane 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]

@melvin-bot
melvin-bot Bot removed request for a team June 5, 2026 07:11
@MelvinBot

Copy link
Copy Markdown
Contributor Author

🤖 Done — this PR is now marked Ready for review.

Heads up: the Tests, Offline tests, and QA Steps sections are still placeholders. As the human co-author, please fill those in (I can't complete them on your behalf) so reviewers have steps to validate against.

@JmillsExpensify JmillsExpensify 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.

No product review required.

@daledah

daledah commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

PR still in progress

@daledah

daledah commented Jun 8, 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 any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
    • 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 shown in the product is localized by adding it to src/languages/* files and using the translation method
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • 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)
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • 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)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.ts or at the top of the file that uses the constant) are defined as such
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • 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
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • 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 any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • 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.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • 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
android.mov
Android: mWeb Chrome

N/A

iOS: HybridApp
ios-resize.mp4
iOS: mWeb Safari

N/A

MacOS: Chrome / Safari
web-resize.mp4
Screen.Recording.2026-06-10.at.13.29.34.mov
web-resize.mp4
web-resize.mp4

@daledah

daledah commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

@MelvinBot please merge main

…rDeepLinkRaceV2

Co-authored-by: daledah <daledah@users.noreply.github.com>
@yuwenmemon

Copy link
Copy Markdown
Contributor

@daledah bump, please.

@daledah

daledah commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

I think we're waiting for @situchan to finish his review.

@daledah

daledah commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

@MelvinBot pull main and fix conflict

…rDeepLinkRaceV2

Co-authored-by: daledah <daledah@users.noreply.github.com>

# Conflicts:
#	src/libs/Navigation/AppNavigator/Navigators/TabNavigator.native.tsx
#	src/libs/Navigation/AppNavigator/Navigators/TabNavigator.tsx
#	src/libs/actions/Link.ts
@MelvinBot

Copy link
Copy Markdown
Contributor Author

🤖 Merged main and resolved the conflicts. ✅ Pushed as a merge commit — history preserved (no rebase/squash/force-push).

Conflicts resolved (3 files):

  • TabNavigator.tsx & TabNavigator.native.tsxmain reorganized/regrouped the imports at the top of the file. Kept main's import layout and added the new tabNavigatorReadiness import into the correct group. The mount/unmount lifecycle wiring in the component body is intact.
  • Link.ts — the trickiest one. main added onboarding-flash handling (initialHasCompletedGuidedSetupFlow capture + drop-check inside handleDeeplinkNavigation, [Due for payment 2026-07-03] [$250] Onboarding - Not here page appears briefly after completing onboarding #91437) while this PR wrapped waitForProtectedRoutes() in a .then(() => whenTabNavigatorReady()) chain. The two changes are orthogonal, so I combined bothmain's onboarding logic is fully preserved and the deep-link gate is chained on top.

subscribe.ts auto-merged cleanly with the PR's defer-until-mounted logic intact.

Verification:

  • npm run typecheck-tsgo — passed
  • npm run lint-changed — passed
  • oxfmt (formatting) — applied
  • tests/unit/tabNavigatorReadinessTest.ts — 6/6 passed

Please re-run the full CI suite / re-review to confirm everything is green.

@daledah

daledah commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

@MelvinBot please check this comment #92482 (review)

@MelvinBot

Copy link
Copy Markdown
Contributor Author

🤖 Checked situchan's regression analysis. I verified each claim against the code. The two severe conclusions (#1 permanent hang + Onyx leak, #2 silently-dropped links) do not actually occur — but the underlying observations are accurate, and #3/#4 are fair, so a small defensive change is worth adding.

Verdicts

🔴 #1 — Permanent hang / subscription leak for Concierge / Track / Submit deep links → does NOT occur.

The claim's mechanism is accurate in isolation: CONCIERGE/TRACK_EXPENSE/SUBMIT_EXPENSE/REPORT_ATTACHMENTS are siblings of TAB_NAVIGATOR on the RootStack (AuthScreens.tsx:191-231), waitForProtectedRoutes() resolves off the static routeNames list (Navigation.ts:730-736), persistentScreens/dontDetachScreen only marks routes already in state.routes, and Onyx.disconnect is gated behind the promise (Link.ts:321-339).

But the conclusion misses the state adapter: getAdaptedStateFromPath always prepends a TAB_NAVIGATOR base route beneath any non-full-screen, non-public sibling route. For /concierge it falls through to getAdaptedStateFromPath.ts:387-390getDefaultFullScreenRoute() returns getTabNavigatorState({name: SCREENS.HOME}) (:281). So the adapted state is [TAB_NAVIGATOR, CONCIERGE], not [CONCIERGE]. TAB_NAVIGATOR mounts → the useEffect fires setTabNavigatorMounted()whenTabNavigatorReady() resolves → the callback runs → Onyx.disconnect is called. No hang, no leak.

🟠 #2subscribe.ts silently drops sibling-targeted deep links → does NOT occur.

whenTabNavigatorReady() resolves when TAB_NAVIGATOR mounts in the current session, not when the destination contains it (subscribe.ts:44-49). TAB_NAVIGATOR is the persistent base of every authenticated RootStack, so once it's declared it does mount — the deferred listener(url) always fires. The real (minor) effect is a small added latency: a warm deep link to a sibling route arriving in the brief post-auth lazy-load window waits for the tab router even though it doesn't strictly need it.

🟡 #3 — Stale-URL across a session change → valid, low probability. whenTabNavigatorReady().then(() => listener(url)) captures url by closure; a logout→login that re-arms readiness between defer and resolve could fire a stale URL against a fresh mount. Real new ordering hazard, but narrow.

🟡 #4 — No timeout / escape hatch → valid. Both consumers chain on an unbounded promise. Given navigation edge-cases are hard to fully enumerate (e.g. a raw NAVIGATE dispatched to a sibling that bypasses the adapter), a bounded fallback is cheap insurance even though the invariant holds for the known deep-link paths.

Suggested solution (small, defensive — no change to shared primitives)

Rather than a behavioral fix (none is strictly required for #1/#2), harden the readiness helper so any unmet assumption degrades gracefully instead of stalling, which covers #3 and #4 at once:

  1. Bounded wait in tabNavigatorReadiness.ts — give whenTabNavigatorReady() an optional timeout that resolves (never rejects) after e.g. ~2s, using Promise.race. On timeout it degrades to today's immediate behavior. Both Link.ts and subscribe.ts use it. This bounds every "TAB_NAVIGATOR will mount" assumption.
  2. Session guard for the deferred forward (Add Expensify eslint #3) — in subscribe.ts, capture the mount token / session id at defer time and only fire listener(url) if it still matches when the promise resolves; otherwise forward immediately. (The timeout in step 1 already caps the worst case.)
  3. (Optional, addresses Fix spaces #2's latency) only defer in subscribe.ts when the URL actually targets a screen inside TAB_NAVIGATOR; forward sibling/full-screen targets (Concierge, TRANSITION_BETWEEN_APPS, search) immediately.

Steps 1–2 are low-risk and directly answer the reviewer's #3/#4; step 3 is a nice-to-have.

daledah — want me to implement steps 1–2 (and optionally 3)? Reply @MelvinBot implement steps 1-2 (or 1-3) and I'll push to this branch.

Verified by reading the state adapter, RootStack declaration, and both consumers directly (high confidence). This is a code-path/state-shape question, so no production logs were needed.

@daledah

daledah commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

@situchan what do you think about Melvin's comment above?

@MelvinBot

Copy link
Copy Markdown
Contributor Author

Regression scan

I reviewed the diff specifically for the failure class that got #91796 reverted — a readiness promise that never resolves and hangs a flow. Good news: the two most dangerous candidates are safe, and the blast radius is far smaller than the reverted PR because nothing here touches the shared waitForProtectedRoutes() primitive. But there are three residual risks worth hardening before merge, all stemming from the fact that whenTabNavigatorReady() has no timeout — if TabNavigator never mounts, the promise never resolves.

✅ Cleared (not regressions)

  • Anonymous / public-room deep links don't hang. Anonymous users hold an authToken (authTokenType === ANONYMOUS), so AppNavigator renders AuthScreens, which declares TAB_NAVIGATOR as its persistent first child — it mounts, and the signOutAndRedirectToSignIn branch in Link.ts still runs.
  • HybridApp mounts TAB_NAVIGATOR on the NewDot side after the OldDot→NewDot transition sets the authToken. The PR's "guaranteed to mount over the auth wall" claim holds.
  • The Link.ts rewrite (79/72) is purely mechanical — a whitespace-insensitive diff collapses to one import + the added .then(() => whenTabNavigatorReady()) + a comment. Every early-return and the final hasCompletedGuidedSetupFlowSelector(val) || isAnonymousUser() guard are byte-identical. No hidden logic change.

⚠️ Residual risks

1. Deferred deep link can be dropped + closure leaked (subscribe.ts). In subscribe.ts:44-49, when TAB_NAVIGATOR is declared but not yet mounted the URL is deferred via whenTabNavigatorReady().then(() => listener(url)). If the router never completes its mount (user logs out / abandons during that window, or the lazy getComponent chunk fails to load), the .then never fires — the deep link is silently dropped and the closure leaks. Pre-PR always called listener(url) immediately. The window is short, so this is bounded, but a timeout fallback (forward the URL after N ms regardless) would eliminate the drop.

2. Onyx connection leak on the same never-mount path (Link.ts). Onyx.disconnect(connection) at Link.ts:339 now sits behind the extra whenTabNavigatorReady() await added at Link.ts:321-322. If that promise never resolves, the NVP_ONBOARDING connection is never disconnected. In practice waitForProtectedRoutes() resolving already implies AuthScreens (and thus the persistent TAB_NAVIGATOR) is mounting, so the only realistic trigger is a failed lazy chunk load — already a fatal state. Low severity, but it's a new leak surface that didn't exist before the extra await.

3. Deep-link ordering is no longer strictly FIFO (subscribe.ts). A deferred first URL can now be processed after a second URL that arrives once the tab router is mounted (immediate path). Two rapid deep links in the mount window can reorder. Low likelihood, but it's a real loss of the previous in-arrival-order guarantee.

Also checked — fine
  • Re-arm race: whenTabNavigatorReady() captures the current readyPromise; resolveReady is only rebound when a new promise is created, so a promise captured before mount still resolves correctly across a logout→login cycle. Safe.
  • navigationRef.current?.getRootState() is optional-chained and the handler already read root state pre-PR; null cold-start state falls through to the immediate listener(url). Safe.
  • No unhandled rejections: neither whenTabNavigatorReady() nor waitForProtectedRoutes() can reject, so the missing .catch is fine.
  • Ref-counting across multiple mounted TAB_NAVIGATOR instances and the unbalanced-unmount guard are covered by the new unit tests and look correct.

Recommendation

None of these are the shared-primitive hang that forced the last revert — the isolation to deep-link/notification flows is the right call. The single highest-value hardening is a timeout fallback inside whenTabNavigatorReady() (resolve after a few seconds even if no mount arrives): it neutralizes risks #1 and #2 at once and guarantees the promise can never strand a deep link or an Onyx connection.


Confidence: high on the cleared items and the mechanical-refactor check; medium on the residual risks (edge-case code review, not reproduced in production).

@situchan

situchan commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Screenshots/Videos

Android: HybridApp
android.mov
Android: mWeb Chrome
iOS: HybridApp
ios.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
web.mov

@situchan

situchan commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Regression tests

92023
92023.mov
92030
92030.mov
92039
92039.mov
92041
92041.mov

@situchan

situchan commented Jul 9, 2026

Copy link
Copy Markdown
Contributor
92030-prod.mov

I reproduced #92030 in main branch so this PR also fixes the existing bug happening on production.

@situchan

situchan commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Melvin's comments are non-blockers

@yuwenmemon
yuwenmemon merged commit 620471d into main Jul 9, 2026
43 checks passed
@yuwenmemon
yuwenmemon deleted the claude-fixTabNavigatorDeepLinkRaceV2 branch July 9, 2026 19:24
@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to staging by https://github.com/yuwenmemon in version: 9.4.33-0 🚀

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

@MelvinBot

Copy link
Copy Markdown
Contributor Author

Help site review — no changes required

I reviewed this PR against the help site content in App/docs/articles and no documentation updates are needed.

Why: This is an internal engineering fix for a navigation race condition — it gates deep-link/push-notification navigation on TabNavigator mount via a new lifecycle readiness signal (tabNavigatorReadiness.ts), so a deep link opened from a logged-out state no longer gets silently dropped (and no longer throws a TabNavigator console error).

The changes are entirely under the hood:

  • No new or changed user-facing feature, tab, setting, or button.
  • No new copy — the one translation-key reference (onboarding.purpose.errorBackButton) is just moved during a refactor of Link.ts, not added.
  • Behavior for users is unchanged except that a previously-broken deep-link flow now works as already expected.

Since the help site documents product features and user-facing behavior, there's nothing here to add or update, so I did not open a docs PR.

@daledah, please confirm you agree no help site changes are required. If you think a specific article should be updated, let me know which flow and I'll draft it.

@mvtglobally

Copy link
Copy Markdown

We are seeing a few console errors while executing this PR, but they do not seem to be the ones mentioned in the steps. Can you please double check this is PASS

Screen.Recording.2026-07-11.at.11.14.09.AM.mov

@m-natarajan

Copy link
Copy Markdown

Chrome / v9.4.33-1 PR:95942
no error, Able to reproduce this in the Ad-hoc build as well.

bandicam.2026-07-13.16-20-07-690.mp4

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by https://github.com/lakchote in version: 9.4.33-8 🚀

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

Bundle Size Analysis (Sentry):

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.

8 participants