Gate deep-link navigation on TabNavigator mount via lifecycle signal#92482
Gate deep-link navigation on TabNavigator mount via lifecycle signal#92482MelvinBot wants to merge 5 commits into
Conversation
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>
|
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: Then you can close this PR and open a new one from your fork. |
|
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 (
What's left (human action, not a code fix): before marking the PR ready for review, the co-author needs to fill out the |
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.
|
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 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".
| function setTabNavigatorUnmounted() { | ||
| if (!isMounted) { | ||
| return; | ||
| } | ||
| isMounted = false; | ||
| resetReadyPromise(); |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot this makes sense, please update as the comment above
There was a problem hiding this comment.
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.ts—setTabNavigatorMounted()increments and only resolves the promise on the0 → 1transition;setTabNavigatorUnmounted()decrements and only re-arms the promise on the1 → 0transition (with a guard against going negative on an unbalanced call).isTabNavigatorMounted()/whenTabNavigatorReady()now report ready whilemountedCount > 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 passeslinton both changed files — cleanprettier— cleantypecheck-tsgo— clean
|
@MelvinBot merge main |
…rDeepLinkRaceV2 Co-authored-by: daledah <daledah@users.noreply.github.com>
|
🤖 Merged latest Merge commit: |
|
🤖 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:
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 (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. |
|
@MelvinBot please add these issues to the and @MelvinBot update your checklist:
|
|
🤖 Updated the PR description as requested:
|
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>
|
@MelvinBot mark this PR as ready for review |
|
@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] |
|
🤖 Done — this PR is now marked Ready for review. Heads up: the |
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required.
|
PR still in progress |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeN/A iOS: HybridAppios-resize.mp4iOS: mWeb SafariN/A MacOS: Chrome / Safariweb-resize.mp4Screen.Recording.2026-06-10.at.13.29.34.movweb-resize.mp4web-resize.mp4 |
|
@MelvinBot please merge main |
…rDeepLinkRaceV2 Co-authored-by: daledah <daledah@users.noreply.github.com>
melvin-bot
Bot
requested review from
carlosmiceli,
daledah,
deetergp,
situchan and
yuwenmemon
and removed request for
a team
|
@situchan @daledah @deetergp @yuwenmemon @carlosmiceli One of you needs to 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] |
|
🤖 Merged latest |
|
⏱️ Ran out of time before I could finish. This usually means the task was too complex or I got stuck exploring. Please try again with a more specific request or post in #melvinbot-ai. |
|
@MelvinBot I completed my checklist and tested well. Please complete your checklist to fix the failing check |
|
Completed the PR Author Checklist — the The failure wasn't a code problem: the PR body contained an abbreviated 18-item author checklist, but |
|
Please let me know if this requires secondary C+ review and test. |
|
@MelvinBot please resolve conflict |
…rDeepLinkRaceV2 Co-authored-by: daledah <daledah@users.noreply.github.com> # Conflicts: # src/libs/actions/Link.ts
|
🤖 Resolved the merge conflict with Conflict:
Resolution: kept all of |
|
@situchan yes, it does. |
|
@situchan, any update on this one, by the way? |
|
Doing regression test... |
|
@situchan how did that go? |
|
@situchan bump! |
situchan
left a comment
situchan
left a comment
There was a problem hiding this comment.
Regression Analysis:
🔴 1. whenTabNavigatorReady() can hang permanently for sibling full-screen deep-link targets (Concierge / Track / Submit Expense)
The PR's safety premise — "the user is over the auth wall, so TAB_NAVIGATOR is guaranteed to mount" — is false for a class of routes.
waitForProtectedRoutes()resolves onrouteNames.includes(PROTECTED_SCREENS.CONCIERGE)— a static list of declared screen names on RootStack.- But
CONCIERGE,TRACK_EXPENSE,SUBMIT_EXPENSE,REPORT_ATTACHMENTS, etc. are siblings ofTAB_NAVIGATORon the RootStack (AuthScreens.tsx), not children of it. getAdaptedStateFromPathdoes not always prepend TAB_NAVIGATOR. A deep link to/concierge(a full-screen sibling) yields an authenticated state whose base route isCONCIERGE, with no TAB_NAVIGATOR instate.routes.persistentScreensonly marksdontDetachScreenon routes that are already instate.routes. It does not force a persistent screen to mount when it's absent. So TAB_NAVIGATOR never renders → the newuseEffectnever callssetTabNavigatorMounted()→whenTabNavigatorReady()never resolves.
Consequences (all new vs. main):
- The entire post-auth callback in
openReportFromDeepLinknever runs. Before this PR it always ran oncewaitForProtectedRoutes()resolved. Onyx.disconnect(connection)is never called — theNVP_ONBOARDINGconnectWithoutViewsubscription leaks and its callback re-fires on every onboarding-data change, each time spawning a fresh never-resolving promise chain.- For an anonymous user deep-linking to a non-accessible sibling route,
signOutAndRedirectToSignIn(true)never fires (the access-gate is now behind the dead promise). setOnboardingErrorMessage(...)for the onboarding-flow case never fires.
The visible navigation to Concierge itself still works for authenticated users (react-navigation handles it via the linking config) — which is exactly why this will be easy to miss in QA. The damage is the leaked subscription and the skipped anonymous/onboarding branches.
Fix: add a timeout fallback so the gate degrades to the old behavior instead of hanging, e.g. Promise.race([whenTabNavigatorReady(), timeout(ms)]), or only gate when the deep-link target is actually inside TAB_NAVIGATOR.
Bottom line: The fix is correct for its tested flow (report deep links, which live inside TAB_NAVIGATOR), but it generalizes an invariant — "authenticated ⇒ TAB_NAVIGATOR mounted" — that doesn't hold for sibling full-screen routes (Concierge, Track/Submit Expense, attachments).
Please verify by cold-starting a deep link to /concierge and confirming the NVP_ONBOARDING connection is never disconnected.
🟠 2. subscribe.ts defers (and can drop) deep links that don't target TAB_NAVIGATOR
The deferral gates on isTabNavigatorDeclared && !isTabNavigatorMounted() regardless of where the URL points:
if (isTabNavigatorDeclared && !isTabNavigatorMounted()) {
whenTabNavigatorReady().then(() => listener(url));
return;
}- Any warm deep link arriving in the declared-but-not-mounted window — including ones targeting sibling full-screens (Concierge,
TRANSITION_BETWEEN_APPS, search router) — is delayed until TAB_NAVIGATOR mounts, even though those targets don't need it. - Same root cause as #1: if the destination state never contains TAB_NAVIGATOR, the deferred
listener(url)never fires and the link is silently dropped — the very failure mode this PR set out to eliminate, reintroduced for a different route class. Today (onmain) these forward immediately.
🟡 3. Stale-URL / cross-session deferral in subscribe.ts
whenTabNavigatorReady().then(() => listener(url)) captures url by closure. If the session changes between deferral and the next TAB_NAVIGATOR mount (e.g. quick logout→login, or a pushed-then-popped navigator triggering re-arm), a stale URL fires against a fresh mount. Low probability, but it's a new ordering hazard the old immediate-forward didn't have.
🟡 4. No timeout anywhere ties #1–#3 together
Both consumers chain on an unbounded promise. The design has no escape hatch, so any unmet "TAB_NAVIGATOR will mount" assumption fails as a permanent hang / dropped link / leaked subscription rather than a graceful degrade. A single shared timeout helper in tabNavigatorReadiness.ts would bound all three.
@MelvinBot suggest solutions if above concerns are valid.
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
I traced the navigation internals to check the load-bearing premise of this analysis — "authenticated ⇒ TAB_NAVIGATOR mounted is false for sibling full-screen routes (Concierge, Track/Submit Expense, attachments, search)." That premise does not hold in the current code, so the 🔴/🟠 hang-and-leak scenarios (#1, #2) do not actually occur for the named routes. #3 is real but extremely narrow, and #4 is worth doing as cheap hardening. Details and a concrete suggestion below. Why #1 and #2 don't trigger for the named routesEvery authenticated deep-link target resolves to a state that contains Concierge / Track Expense / Submit Expense / Attachments — these are root-stack siblings but are not in Search / Workspace — these are in The only state with no So the leak mechanism in #1 is correctly described — it just isn't reachable todayThe reviewer is right that This is the real takeaway from the review: correctness is now coupled to an implicit invariant — every authenticated deep-link state contains TAB_NAVIGATOR. It holds today only because Search/Workspace happen to be nested under TAB_NAVIGATOR and the other siblings get it prepended. If someone later hoists Search/Workspace to top-level root-stack siblings, or adds a new top-level authenticated full-screen navigator, this gate would silently start hanging and leaking — exactly as #1 describes. On #3 (stale URL) and #4 (no timeout)#3 is real but very narrow. The deferred #4 is the right fix to ship anyway. A bounded wait neutralizes #1–#4 in one move and removes the dependency on that implicit invariant, degrading to today's behavior instead of hanging if it's ever violated. Suggested solution (defense-in-depth, low risk)Bound the gate with a timeout in // tabNavigatorReadiness.ts
const TAB_NAVIGATOR_READY_TIMEOUT_MS = 5000;
/** Resolves when TAB_NAVIGATOR mounts, or after a timeout so callers never hang. */
function whenTabNavigatorReady(timeoutMs = TAB_NAVIGATOR_READY_TIMEOUT_MS): Promise<void> {
if (mountedCount > 0) {
return Promise.resolve();
}
return Promise.race([readyPromise, new Promise<void>((resolve) => setTimeout(resolve, timeoutMs))]);
}This keeps the happy path identical (mount resolves immediately) and guarantees Bottom line: The PR is safe as-is for the routes called out — I couldn't find an authenticated deep-link target that fails to mount TAB_NAVIGATOR. The review's lasting value is spotting the implicit invariant; a small timeout fallback is cheap insurance that makes the gate robust if that invariant is ever broken by a future navigation change. 🤖 Generated with Claude Code · This is an automated analysis — please verify the navigation-state claims against your own reading before acting. |
Re: regression analysisI traced the navigation architecture on this branch. The central 🔴 concern (#1) is invalid — but for a subtle reason the analysis missed, so it's worth spelling out. #2's "silently dropped" claim follows the same flawed premise and is also invalid. #3/#4 are real-but-minor defensive nits. 🔴 #1 —
|
5 participants
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_NAVIGATORis declared on the root navigator as soon as the authenticatedRootStackrenders, but it is loaded lazily (getComponent), so there is a window where the screen is declared while its child router has not runuseNavigationBuilderyet. A deep link or push notification that dispatches a nestedNAVIGATEduring that window is "not handled by any navigator" and is silently dropped.Why #91796 was reverted: it gated the shared
waitForProtectedRoutes()primitive inNavigation.tson 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 rootstateevent fired, so the promise never resolved and navigation hung. BecausewaitForProtectedRoutes()is shared by many flows, that hang surfaced as multiple regressions.This PR's approach:
tabNavigatorReadiness.ts— a lifecycle-driven readiness signal.TabNavigatorflips 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.TabNavigator.tsxandTabNavigator.native.tsx.openReportFromDeepLink(Link.ts) onwhenTabNavigatorReady()afterwaitForProtectedRoutes(), instead of gating inside the shared primitive. The user is over the auth wall here, soTAB_NAVIGATORis guaranteed to mount.subscribe.ts), defer (don't drop) the URL until the tab router mounts. On public screensTAB_NAVIGATORisn't declared, so the URL is forwarded immediately.Navigation.tsis 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.
Offline tests
Same as QA Tests.
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari