Fix/82013 public room deeplink signed out#94570
Conversation
… + OpenApp) A signed-out user opening a public-room deeplink (/r/<id>) failed to land on the room. Three independent root causes, each fixed: RC#1 (QueuedOnyxUpdates.ts) — flushQueue() dropped the report_* / reportActions_* collection data because currentAccountID is still undefined when the OpenReport WRITE batch flushes (the SESSION update that sets it is inside the same batch), so the preservedKeys stale-data filter (Expensify#52822/Expensify#48427) discarded it and deeplink navigation hung. Fix: bypass the filter when the batch establishes the anonymous session (a SESSION update with an authToken whose authTokenType is anonymous) — that data belongs to the new session and can't be the stale cross-account replay the filter guards against. RC#2 (linkingConfig/subscribe.ts) — React Navigation's built-in linking dispatched NAVIGATE into TabNavigator (absent from the signed-out PublicScreens tree) -> "not handled by any navigator", leaving the user on sign-in. Fix: skip forwarding report deeplinks to React Navigation while signed out; DeepLinkHandler -> openReportFromDeepLink() performs the navigation once the anonymous session establishes the protected routes. RC#3 (AuthScreensInitHandler.tsx) — once AuthScreens mounts it called App.openApp() with the default shouldKeepPublicRooms=false, so OpenApp's response supersedes the anonymous public-room report data; ReportNavigateAwayHandler then sees the report removed and falls back to Concierge (the "room flashes then Concierge" symptom on slower devices). Fix: call App.openApp(isAnonymousUser(session)) so public rooms are preserved through OpenApp, matching SignInModal's openApp(true) after anon->sign-in.
…t report RC#3 follow-up. Even with openApp(true) preserving the room data, once OpenApp and the auth tree settle the ReportsSplitNavigator re-resolves its central pane without the reportID in its route params, so ReportUtils.findLastAccessedReport() picks the default report (Concierge for a fresh anonymous user) and overrides the deeplinked room. Verified on emulator: room shown, then a NAVIGATE to the Concierge report ~85s later, with report_<id> NOT removed (so openApp(true) alone is insufficient). Fix: remember the signed-out public-room deeplink reportID in a RAM-only Onyx key and prefer it over findLastAccessedReport in ReportsSplitNavigator's initial central report. Set in openReportFromDeepLink, read via a module-level connectWithoutView tracker (available synchronously in the useState initializer), cleared once the room is loaded and focused (ReportFetchHandler) or when the deeplink is not a signed-out report deeplink.
… (real RC#3 fix) The Concierge override was ReportNavigateAwayHandler: while the anonymous OpenApp settles, one of its "report removed/left/closed" conditions spuriously fires for the deeplinked public room and navigates away to Concierge (in a deferred isNavigationReady().then(), ~tens of seconds after the room is shown — the "room flashes then Concierge" symptom). Confirmed on emulator: with the guard the handler's two navigate-away paths are blocked (no navigateToConciergeChat, no override); without it navigateToConciergeChat fires and Concierge wins. Fix: guard both navigate-away paths in ReportNavigateAwayHandler with the pending public-room deeplink reportID — don't navigate away from the report being opened from a signed-out public-room deeplink. Also refine the pending-key clear to keep it for the whole anonymous session (cleared on sign-in) instead of on first room-load, so the late navigate-away is still guarded.
…ction Rebased onto current main and addressed eslint-seatbelt: - QueuedOnyxUpdates: narrow the anonymous-session check via `unknown` instead of an `as Session` assertion (avoids a 2nd unsafe assertion). - Add Link.clearPendingPublicRoomDeepLink() so ReportFetchHandler clears the RAM-only pending reportID through the action layer (prefer-actions-set-data) rather than calling Onyx.set directly.
…bypass Covers the RC#1 fix: when flushQueue runs with currentAccountID undefined but the queued batch establishes an anonymous session (SESSION update with an anonymous authTokenType), report/reportActions data is preserved instead of being dropped by the stale-data filter.
… onboarding flow On current main the new OnboardingGuard redirected signed-out and anonymous users (stale hasCompletedGuidedSetupFlow=false left over after sign-out) into onboarding -> navigateAfterOnboarding -> Concierge, overriding the deeplinked public room. - OnboardingGuard: skip onboarding unless there is a real authenticated (non-anonymous) session, so signed-out/anonymous users are never redirected. - Link.ts: keep the RAM-only pending public-room deeplink reportID through the anonymous session (don't clear it once the anonymous account is "authenticated"), so the navigator and navigate-away guards retain their signal.
…oom deeplink On current main, ReportNotFoundGuard declares the report "not found" once loading finishes and the report is momentarily missing. While a signed-out public-room deeplink is being opened, the report data can be transiently absent as the anonymous session and OpenApp settle (a release-build timing race not seen in dev), which made the deeplinked public room flash and then show "Hmm... it's not here". Suppress the NotFound page while the pending public-room deeplink is the focused report.
melvin-bot
Bot
requested review from
eVoloshchak,
situchan and
trjExpensify
and removed request for
a team
|
@eVoloshchak @situchan 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] |
![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: 82b09b493c
ℹ️ 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".
…an't ride the anonymous bypass Addresses review feedback on QueuedOnyxUpdates: flushQueue() bypasses the no-account stale-data filter for the batch that establishes an anonymous session (the signed-out public-room deeplink). cleanupSession() cleared persisted requests but not the QueuedOnyxUpdates buffer, so old-account updates left buffered at sign-out could ride through that bypass into the new anonymous session. Clear the buffer in cleanupSession() too, and cover it with a unit test.
… changes - AuthScreensInitHandler test: mock Session.isAnonymousUser, now called by openApp(isAnonymousUser). - OnboardingGuard tests: set up a real authenticated session so the new "only a real, non-anonymous account can complete onboarding" gate evaluates as before (the guard now skips signed-out/anonymous users). - cspell: add "Onboardable".
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.
|
| // the anonymous SESSION and the report_* collection data in the same batch. Without this bypass the | ||
| // report data is filtered out and deeplink navigation hangs. We gate on the anonymous authTokenType | ||
| // (not just any authToken) so the stale-data protection stays intact for every other flow. | ||
| const establishesAnonymousSession = copyUpdates.some((update) => { |
There was a problem hiding this comment.
The anonymous-session bypass disables the stale-data filter for the entire flush batch, not just SESSION/report_* keys.
establishesAnonymousSession uses .some() over the whole buffer; if it's true, copyUpdates is applied with no preservedKeys filtering at all. queueOnyxUpdates concatenates updates from multiple sources into one buffer, so a stale/cross-account update buffered before the anonymous OpenReport response gets merged into the fresh anonymous session unfiltered.
clear() on cleanupSession() only covers the explicit sign-out path.
Consider keeping the filter but additionally whitelisting the session + report_*/reportActions_* keys, rather than dropping it wholesale.
establishesAnonymousSession only matches when the SESSION update itself carries authTokenType === ANONYMOUS.
If the server ever delivers the anonymous session as a partial merge that includes authToken but omits authTokenType (unchanged/split into a prior batch), the check evaluates false, the filter runs, and the co-batched report_* data is dropped — reproducing the exact hang this PR fixes.
Please confirm the anonymous OpenReport response always co-delivers authTokenType with authToken.
The hand-rolled session-shape narrowing duplicates Session.isAnonymousUser.
The typeof … === 'object' && 'authTokenType' in … === ANONYMOUS check re-implements existing anonymous-session detection (Session/index.ts:306, no circular-dep risk since Session doesn't import QueuedOnyxUpdates).
Only the extra authToken-presence check is new; wrapping the shared helper keeps the definition of "anonymous" in one place.
| import continuePlaidOAuth from '@libs/continuePlaidOAuth'; | ||
| import navigationRef from '@libs/Navigation/navigationRef'; | ||
| import type {RootNavigatorParamList} from '@libs/Navigation/types'; | ||
| import {getReportIDFromLink} from '@libs/ReportUtils'; |
There was a problem hiding this comment.
The new import {getReportIDFromLink} from '@libs/ReportUtils' introduces a circular import.
Confirmed chain: ReportUtils.ts:141 imports linkingConfig, linkingConfig/index.ts:9 imports subscribe, and subscribe now imports ReportUtils.
It's runtime-lazy (called inside the url handler) so it works today, but it directly contradicts this file's own comment justifying the manual SESSION subscription "to detect a signed-out user without importing the Session action module (which would create a circular dependency through the navigation layer)."
A lighter helper (e.g. reuse getRouteFromLink/route parsing without the full ReportUtils barrel) avoids the cycle.
| // instead of defaulting to the last-accessed report (which is Concierge for a fresh anonymous | ||
| // user). Without this, once OpenApp/auth settle and this navigator re-resolves without the | ||
| // reportID in its route params, findLastAccessedReport() picks Concierge and overrides the room. | ||
| if (pendingPublicRoomDeepLinkReportID) { |
There was a problem hiding this comment.
The module-level pendingPublicRoomDeepLinkReportID is read synchronously in a useState initializer but populated by an async Onyx.connectWithoutView callback.
If this navigator ever mounts before the listener has delivered its first value, the initializer falls through to findLastAccessedReport() (Concierge) and — because it's a useState initializer — never re-runs, so the wrong initial report is latched for the navigator's lifetime.
The set happens well before the auth tree mounts in the happy path, so this is a timing race rather than a guaranteed failure, but it cannot self-heal.
| // opened. The anonymous OpenApp settling can briefly make this report look removed/closed and | ||
| // spuriously trigger a navigate-away to Concierge ~tens of seconds after the room is shown | ||
| // ("room flashes then Concierge"). The pending key is cleared once the user signs in. | ||
| if (pendingPublicRoomReportID && reportIDFromRoute === pendingPublicRoomReportID) { |
There was a problem hiding this comment.
The reportIDFromRoute === pendingPublicRoomReportID guard is duplicated across four sites (twice here, plus ReportNotFoundGuard.tsx and ReportFetchHandler.tsx) with no shared selector/hook.
A future change to the matching rule (ID normalization, thread/parent handling) must be applied to every copy; missing one silently re-introduces the bug in just that guard.
A single useIsPendingPublicRoomDeeplink(reportID) hook would consolidate it.
3 participants
Explanation of Change
When a signed-out user opened a public room deeplink while the app was already running (or cold-started from the link), the app failed to land on the room — it showed the login screen, or briefly opened the room and then bounced to Concierge / a "Hmm… it's not here" page. The failure was a chain of independent causes, each fixed below:
Report data was dropped before it could be rendered.
QueuedOnyxUpdates.flushQueueapplies a stale-data filter when there is no current account. For the signed-out deeplink,OpenReportreturns the anonymousSESSIONand thereport_*data in the same batch, so the report data was filtered out and navigation had nothing to land on. Fix: bypass the filter only when the batch itself establishes an anonymous session (gated onauthTokenType === ANONYMOUS), so the stale-data protection stays intact for every other flow.The deeplink was dispatched into a navigator that did not exist yet. While signed out,
linkingConfig/subscribeforwarded the report route into the authenticated navigator before it was mounted. Fix: skip that forward for a report deeplink while there is no auth token.The room lost focus to the default report after auth settled. Once the anonymous session and
OpenAppsettled, the app re-resolved to the last-accessed report (Concierge for a fresh anonymous user) and overrode the room. Fix: preserve public rooms acrossOpenApp, keep the deeplinked room focused inReportsSplitNavigator, and block the navigate-away — coordinated through a RAM-onlypending public-room deeplinkkey whose writes go through an action.The onboarding guard hijacked the signed-out user. On current
main,OnboardingGuardredirected the user into the onboarding flow (ending on Concierge). Fix: an onboarding flow can only be completed by a real, authenticated account, so skip it for signed-out / anonymous users (gated on a real, non-anonymous session — real users are unaffected).The NotFound page flashed for the room during a warm transition. On current
main,ReportNotFoundGuarddeclares a report "not found" once loading finishes and the report is momentarily missing. During the anonymous-session/OpenAppsettle the deeplinked report is transiently absent (a release-build timing race not seen in dev), producing "Hmm… it's not here". Fix: suppress the NotFound page only while the focused report is the pending public-room deeplink (gated on the same RAM-only key — every other report is unaffected).Changes #4 and #5 touch shared guards but are gated so they only alter the signed-out public-room deeplink path; all other flows (real-user onboarding, normal not-found for deleted/inaccessible reports) keep their existing behavior.
A unit test (
tests/actions/QueuedOnyxUpdatesTest.ts) covers the anonymous-session flushQueue bypass.Fixed Issues
$ #82013
PROPOSAL: #82013 (comment)
Tests
https://new.expensify.com/r/<publicRoomID>.Regression checks:
6. Logged in: while signed in, open a public room deeplink → it opens normally.
7. Onboarding (shared guard): sign up a brand-new account → onboarding is still shown.
8. Not-found (shared guard): while signed in, open a deleted / inaccessible report → the "Hmm… it's not here" page is still shown.
Offline tests
QA Steps
https://new.expensify.com/r/<publicRoomID>.Regression checks:
6. Logged in: while signed in, open a public room deeplink → it opens normally.
7. Onboarding (shared guard): sign up a brand-new account → onboarding is still shown.
8. Not-found (shared guard): while signed in, open a deleted / inaccessible report → the "Hmm… it's not here" page is still shown.
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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.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.native.deeplink.mp4
Android: mWeb Chrome
android.mweb.mp4
iOS: Native
ios.deeplink-.mp4
iOS: mWeb Safari
ios.mweb.deeplink.mp4
ios.mweb.signin.-.deeplink-.mp4
MacOS: Chrome / Safari
web.deeplink-.mp4