fix: keep public room in LHN for anonymous users#93054
fix: keep public room in LHN for anonymous users#93054yusufdeveloper2903 wants to merge 6 commits into
Conversation
47d8ddf to
375edfe
Compare
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Good for product
|
I can still reproduce the original bug: Screen.Recording.2026-06-11.at.11.57.48.mov@yusufdeveloper2903, did everything work out in your tests? Thank you. |
375edfe to
0a220fc
Compare
|
Hi @brunovjk, thanks again for catching this and for your patience! You were right that the bug was still reproducible — but only in a specific scenario that the original fix didn't cover. I dug deep into it and pushed additional commits. Here's the full picture: What the original fix covered (and what it missed) The first commit (isPublicRoom && isAnonymousUser in SidebarUtils) is the display half — it keeps a public room visible in an anonymous user's LHN even when it isn't the focused report. This correctly fixes the web and warm app flows (room already loaded → open a thread → room stays). But it didn't fix the cold native flow (fresh app / cleared data → tap the public room link), which is exactly what QA reported. The reason: in that flow the public room never reaches Onyx at all, so there's nothing for the LHN filter to display. Root cause of the cold-native flow (verified with logs + Onyx inspection) Tracing it on a device, the sequence is:
I confirmed the room was genuinely absent from Onyx at that point (not just hidden), which is why no display-only change could fix it. There was also a secondary The action 'NAVIGATE' ... was not handled by any navigator console error in this flow. What the new commits do
So the three pieces are complementary: load the room (refetch) → keep it visible (LHN override) → no console error (nav guard). Verification I tested the exact cold-native repro from the issue (cleared app data → tapped the public room link as an anonymous user). The public room now appears in the Inbox and the console error is gone. Video below: REC-20260612161320.mp4 |
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.
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp93054_android_native.movAndroid: mWeb Chrome93054_android_native.moviOS: HybridApp93054_ios_native.moviOS: mWeb Safari93054_ios_web.movMacOS: Chrome / Safari93054_web_chrome.mov |
|
Thanks for the update @yusufdeveloper2903, everything seems fine now, do you think we managed to increase the cover test here? Another thing, we have a warning related to |
0a220fc to
139874b
Compare
|
Thanks @brunovjk! Coverage: Good call - I've added unit tests to cover the new logic:
Warning: The only console warning I see is The error that was part of this flow - Happy to report the |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
Hi @brunovjk 👋 Besides the main bug, I also fixed a separate issue in this PR: on web, after opening a public room, going back to Home, reloading, and returning to the Inbox — the public room would disappear from the LHN. This PR fixes that too. Commit: bebb427 Should we keep it here or split it into its own PR? Before fix: 605054097-ff9c7219-b877-4979-815f-86f3b8cd6663.mp4After fix: After-fix.Chrome-mac.mp4 |
|
Hi @MonilBhavsar! 👋 Both points are in 9200c00 now. I pulled the duplicated tracking code out into a shared trackPendingPublicRoomFromDeepLink helper, and the ref is reset as soon as the room shows up in Onyx — so the effect no longer keeps firing on every report update. No change to the actual fix flow, and the existing tests are still green. Let me know if you'd like anything else! 🙏 |
|
@brunovjk mind taking a look at this comment #93054 (comment) |
|
Sure, I'll check it ASAP; I'm a bit busy with other PRs. Thanks. |
|
Sorry for the delay. I checked it, and this web fix is closely related to the same LHN/public-room issue, so I think we should keep it in this PR. |
|
There are conflicts to resolve @yusufdeveloper2903 |
An anonymous session can only access public rooms, whose notification preference defaults to `hidden`. Previously a public room only survived the LHN filter while it was the focused report, so opening a thread inside the room (which steals focus) dropped the room from the Inbox, leaving the anon user unable to return to it. Add an override in `shouldDisplayReportInLHN` so a public room stays in the LHN for anonymous users regardless of focus, mirroring how ReportUtils already gates anon-only public-room behaviour (e.g. canLeaveChat). Fixes Expensify#92672 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
9200c00 to
aba48a9
Compare
|
@MonilBhavsar rebased onto latest |
|
Oxfmt check / Oxfmt check (pull_request) seems to be failing |
|
@MonilBhavsar the failing |
Explanation of Change
An anonymous session can only ever access public rooms, and such a room's notification preference defaults to
hidden. In the LHN filter (shouldDisplayReportInLHNinSidebarUtils), ahiddenreport only survives when one of theshouldOverrideHiddenconditions is true — and for an anonymous user the only one that ever applied wasisFocused. So the public room stayed in the Inbox only while it was the focused report. The moment the anon user opened a thread inside the room, the thread became the focused report, the public room was no longer focused,isHidden && !shouldOverrideHiddenbecame true, and the room was dropped from the Inbox — collapsing it to the "Woohoo! All caught up" empty state and leaving the anon user unable to return to the room.This PR adds a new
shouldOverrideHiddencondition so a public room is never hidden from an anonymous user's LHN just because it isn't the focused report:This pairs
isPublicRoomwithisAnonymousUser()exactly the wayReportUtilsalready gates anon-only public-room behaviour elsewhere (e.g.canLeaveChat). It is a display-only predicate — it does not mutate any report/Onyx data or notification preference — so there are no side effects on unread/GBR or leave-room logic. After sign-inisAnonymousUser()becomesfalse, the override no longer applies, and the existing flow (#91931) re-fetches the room with a real notification preference, so there is no regression and no double-handling.Fixed Issues
$ #92672
PROPOSAL: #92672 (comment)
Tests
…/r/<publicRoomID>).Offline tests
This change is a purely client-side display predicate, so it behaves identically offline.
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
Before fix:
Before-fix-android.mp4
After fix:
After-fix-android.mp4
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Before fix:
Before-fix.Chrome-mac.mp4
After fix:
After-fix.Chrome-mac.mp4