Filter hidden rooms from search router after leaving#87796
Filter hidden rooms from search router after leaving#87796
Conversation
Workspace rooms left by the user were still appearing in search because the excludeHiddenThreads filter only excluded threads with HIDDEN notification preference, not rooms. Rename to excludeHidden and remove the isThread guard so all hidden reports are filtered. Also add an isHiddenForCurrentUser check in the SearchRouter contextual fallback. Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
For non-thread reports, check the current user's participant entry directly rather than using the resolved notificationPreference, which defaults to HIDDEN when the user isn't a participant at all. This avoids incorrectly filtering reports the user can see but isn't listed as a participant of. Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
|
@MelvinBot Mark this as ready for review |
|
🤖 I attempted to mark this PR as ready for review but don't have permission to do so. Please mark it as ready manually by clicking Ready for review on the PR page. |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot This uses isHiddenForCurrentUser(report), which defaults missing participant data to hidden. If participant data is missing, this can suppress the contextual search suggestion for a valid room when it is not already in recentReports (for example because it fell outside the limited result set). What do you think about using the same current-user participant check used in OptionsListUtils instead? Also, this removes hidden rooms only after optionsOrderAndGroupBy(..., maxElements, filteringFunction) has already limited the heap. Hidden rooms can still consume the maxResults slots, then get removed here without backfilling with valid matches, so search/autocomplete can return fewer or no visible rooms even when later valid rooms match. What do you think about folding the excludeHidden check into filteringFunction before the heap limit is applied? |
…ap limit 1. SearchRouter.tsx: Replace isHiddenForCurrentUser (which defaults missing participant data to hidden) with explicit participant notificationPreference check, matching the logic in OptionsListUtils. 2. OptionsListUtils: Move excludeHidden check into filteringFunction so hidden rooms are filtered before the heap limit, preventing them from consuming maxResults slots. Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
|
Both points are valid — fixed in b302207:
|
|
@codex review |
|
@MelvinBot Now it is not working for cross-platform updates. For example, if I create and then leave a room on Android, it still shows in the search results on web (though it doesn't show on Android). |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ 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". |
…entUser Use isHiddenForCurrentUser which correctly defaults to HIDDEN for missing or null participant entries, handling the case where the server removes the participant rather than explicitly setting notificationPreference. Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
|
Pushed a fix in 249e98b. Two changes:
Important caveat: These changes improve robustness when the participant entry is removed or nullified by the server push. However, if the server doesn't push any participant update at all to the web client after Could you verify whether the room disappears on web after a hard refresh? That would help narrow down whether it's a real-time sync gap or a deeper data issue. |
|
The only failing check is PR Author Checklist, which is expected — it fails until the checklist items in the PR description are manually completed. All other checks from the previous run passed (TypeScript, Jest Unit Tests, ESLint, Prettier, perf tests, etc.), and the current run is still in progress. No code fix is needed. |
|
@codex review |
|
@MelvinBot Mark this as ready for review |
|
@MelvinBot Mark as ready for review |
|
🤖 Done! This PR has been marked as ready for review. |
melvin-bot
Bot
requested review from
JmillsExpensify and
ShridharGoel
and removed request for
a team and
JmillsExpensify
|
@ShridharGoel 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] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp
Android: mWeb Chrome
iOS: HybridAppScreen.Recording.2026-04-14.at.1.58.08.AM.moviOS: mWeb Safari
MacOS: Chrome / SafariScreen.Recording.2026-04-14.at.1.55.40.AM.mov |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #87661 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
![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: 249e98ba01
ℹ️ 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".
| if (!reportForContextualSearch) { | ||
| const report = reports?.[`${ONYXKEYS.COLLECTION.REPORT}${contextualReportID}`]; | ||
| if (!report) { | ||
| if (!report || isHiddenForCurrentUser(report)) { |
There was a problem hiding this comment.
isHiddenForCurrentUser(report) treats reports with no current-user participant as hidden because getReportNotificationPreference() defaults to HIDDEN when participant data is missing. Room reports can legitimately arrive without participants (see the chat-room comment in ReportUtils), so this guard can hide the contextual “Search in …” suggestion for rooms the user can still access whenever the fallback path reads such a report from Onyx.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot What do you think about the above comment?
There was a problem hiding this comment.
The concern is technically valid but practically low-risk in this specific code path.
isHiddenForCurrentUser does default to hidden when participant data is missing (ReportUtils.ts:1905 — the comment even says "This should ALWAYS default to hidden. Do not change this!"). So in theory, a room report in Onyx without participant data would be treated as hidden.
However, contextualReportID is the report the user is currently viewing. If they're actively looking at a room, participant data should already be loaded. A false positive here would require a race condition where the report exists in Onyx but its participants haven't synced yet — which is unlikely for the focused report.
That said, if you want to be defensive, you could add a simple guard:
if (!report || (report.participants?.[currentUserAccountID] && isHiddenForCurrentUser(report))) {This would only filter when we have an explicit participant entry confirming HIDDEN, avoiding false positives from missing data. Up to you whether the edge case warrants the extra guard.
|
@ShridharGoel please add tests and remove all the AI noise, and let me know when this is ready for review |
|
@MelvinBot Edit the description to follow this template properly: https://github.com/Expensify/App/blob/main/.github/PULL_REQUEST_TEMPLATE.md |
|
Updated the PR description to follow the template. Merged the separate "AI Tests" and "Human Tests" sections into a single "### Tests" section as the template requires. |
|
@MelvinBot Copy the QA tests into the "Tests" section and don't include anything else in that section. Also, check all the boxes in the author checklist. |
|
Done — copied the QA steps into the Tests section (removed the AI/Human test sub-sections) and checked all boxes in the author checklist. |
|
@cead22 can you check now? |
cead22
left a comment
cead22
left a comment
There was a problem hiding this comment.
@ShridharGoel what do you think about this small simplification?
| if (excludeHidden) { | ||
| if (report.isThread) { | ||
| if (report.notificationPreference === CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN) { | ||
| return false; | ||
| } | ||
| } else { | ||
| const participant = report.item?.participants?.[currentUserAccountID]; | ||
| if (participant && isHiddenForCurrentUser(participant.notificationPreference)) { | ||
| return false; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
| if (excludeHidden) { | |
| if (report.isThread) { | |
| if (report.notificationPreference === CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN) { | |
| return false; | |
| } | |
| } else { | |
| const participant = report.item?.participants?.[currentUserAccountID]; | |
| if (participant && isHiddenForCurrentUser(participant.notificationPreference)) { | |
| return false; | |
| } | |
| } | |
| } | |
| if (excludeHidden) { | |
| if (report.isThread && report.notificationPreference === CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN) { | |
| return false; | |
| } | |
| if (!report.isThread) { | |
| const participant = report.item?.participants?.[currentUserAccountID]; | |
| if (participant && isHiddenForCurrentUser(participant.notificationPreference)) { | |
| return false; | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Done in d80f684 — flattened the nested if/else into two separate guard clauses per cead22's suggestion.
|
@ShridharGoel conflicts |
|
@MelvinBot fix the conflicts |
Resolved conflict in SearchRouter.tsx: kept main's contextualReport useOnyx hook while preserving PR's isHiddenForCurrentUser check. Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
|
Resolved merge conflicts with |
melvin-bot
Bot
requested review from
ShridharGoel
and removed request for
a team
|
@ShridharGoel 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] |
|
⏱️ 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. |
|
@ShridharGoel is this ready? Any thoughts on this? |
|
Yes, that looks good. Thanks for pointing it out. |
Flatten the nested if/else into two separate guard clauses as suggested in code review. Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
3 participants
Explanation of Change
After a workspace member leaves a workspace room, the room was still appearing in the search router and search results. This happened because the
excludeHiddenThreadsfilter ingetValidOptionsonly excluded threads withHIDDENnotification preference — workspace rooms haveisThread = false, so the!option.isThreadguard short-circuited and the room always passed through.This PR:
excludeHiddenThreads→excludeHiddento reflect the broader scopenotificationPreference)HIDDEN) without falsely filtering reports where the user simply isn't listed as a participantisHiddenForCurrentUserguard inSearchRouter.tsx's contextual search fallback, which could reconstruct a hidden room option directly from OnyxFixed Issues
$ #87661
PROPOSAL: #87661 (comment)
Tests
Offline tests
Same as 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