Use the room name for the contextual search suggestion - #96448
Conversation
The "Search in <name>" suggestion falls back to building its own option when the open report is not in the recent reports list, which is capped at 500. That fallback passed showPersonalDetails: true unconditionally, so the label showed a participant's display name instead of the room name for rooms, group chats and policy expense chats. Enable showPersonalDetails only for 1:1 chats, and pass the already subscribed report attributes so the fallback resolves the same derived name the LHN shows instead of the raw reportName, which is empty for unnamed group chats.
|
@mukhrr, when do we expect this pr to be ready for review? thanks. |
|
@marufsharifi hopefully tomorrow |
|
@mukhrr, do you need any help here? |
|
@marufsharifi Almost done, testing my code. thanks! |
|
I found a pre-existing bug here:
ExpectedOpening search from a chat seeds the chat's scope, and submitting resolves that name to the report/policy ID: ActualThe name is submitted verbatim as an ID, and the search silently returns nothing: Root cause
The line-break row is the one case our diff made worse: we normalize the query with REC-20260722124217.mp4@twisterdotcom @marufsharifi do you think we can fix this here or in a separate issue? |
|
@marufsharifi 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] |
![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: c4e2467654
ℹ️ 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".
|
@marufsharifi my only concern is this #96448 (comment) and I think it's not blocker. The rest can be reviewed |
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.
|
|
@twisterdotcom, could you please share your thoughts on this? Do you think it's necessary to address it in this PR, or would it be okay to handle it in a follow-up PR? Thanks! |
| const [[initialQuery, initialSubstitutions]] = useState<[string, SubstitutionMap]>(() => { | ||
| if (!currentSearchQueryJSON || !isFromSearchPageSearchButton || !searchContext?.shouldShowSearchQuery) { | ||
| return [pendingInitialQuery, {}]; | ||
| if (pendingInitialQuery || !contextualReport) { | ||
| return [pendingInitialQuery, {}]; | ||
| } | ||
|
|
||
| // Built once on mount, so a half-loaded name would stay wrong for as long as the router is open. | ||
| // Self DMs carry a fake policy ID that never resolves, so waiting on their policy would never seed. |
There was a problem hiding this comment.
Medium: The seed runs only once, on mount — a slow/cold load silently leaves the input empty with no recovery.
This lazy useState initializer runs a single time. Your own comment on L176 acknowledges it: "Built once on mount, so a half-loaded name would stay wrong…". The failure mode:
- Deep-link straight into a chat (or hard-refresh on a chat), then immediately hit
Cmd+Kbefore Onyx finishes hydratingreportAttributes/policy. - The initializer hits the
return [pendingInitialQuery, {}]fallback (L173 / L180 / L197) and never re-seeds once the data arrives a few ms later.
Result: an empty search box on exactly the flow this PR is meant to improve, recoverable only by closing and reopening the router.
I understand seeding-after-mount would fight the user's typing, so I'm not asking to make it reactive — but please confirm on a fresh reload directly on a chat that the first Cmd+K still seeds, and document the "context data must be loaded at open time" contract in the PR description.
| const contextualQuery = `${StringUtils.lineBreaksToSpaces(getContextualSearchQuery(contextualTarget, contextualPoliciesMap, contextualReportsMap), true)}${AUTOCOMPLETE_TRAILING_SPACE}`; | ||
| const autocompleteKey = getContextualSearchAutocompleteKey(contextualTarget, contextualPoliciesMap, contextualReportsMap); | ||
| return [contextualQuery, autocompleteKey && contextualTarget.autocompleteID ? {[autocompleteKey]: contextualTarget.autocompleteID} : {}]; |
There was a problem hiding this comment.
Low, pre-existing: sanitize mismatch between the query token and the substitution key can break name→ID resolution.
The query sanitizes the name but the substitution key does not:
// L200 → getContextualSearchQuery → `type:chat in:${sanitizeSearchValue(item.searchQuery)}`
// L201 → getContextualSearchAutocompleteKey → `in:${item.searchQuery}` // raw, unsanitizedFor a name containing a space (a 1:1 DM display name like John Doe, or a room with spaces) the query token becomes in:"John Doe" while the substitution map key is in:John Doe. If the key doesn't match the token, submitting searches the literal name instead of resolving to the reportID.
This mismatch already exists in the onSelectRow click path, so the PR doesn't introduce it — but seeding now fires it automatically on every open from a chat, greatly increasing exposure. Please verify with a DM/room whose name has a space that the seeded query still resolves to the correct report on submit; if not, align both helpers in SearchRouterUtils.ts to sanitize identically.
| /** Ends the preceding filter, so what the user types next starts a new term instead of extending that filter's value. */ | ||
| const AUTOCOMPLETE_TRAILING_SPACE = '\u00A0'; | ||
|
|
||
| /** Shared by the contextual suggestion and the query the input is seeded with, so both scope the search identically. */ |
There was a problem hiding this comment.
Nit: Add an explicit return type to this shared helper.
function getContextualSearchTarget(option: OptionData): ContextualSearchTarget {Its shape must stay compatible with ContextualSearchTarget (used by getContextualSearchQuery / getContextualSearchAutocompleteKey). Annotating the return catches drift at the definition site rather than only at call sites, and documents the contract — which matters more now that this is reused by both the suggestion row and the seed. You'd need to export/import the type from SearchRouterUtils.ts.
|
|
||
| if (option.isInvoiceRoom) { | ||
| roomType = CONST.SEARCH.DATA_TYPES.INVOICE; | ||
| const report = option as SearchOption<Report>; |
There was a problem hiding this comment.
NAB Type assertion (as) — flagging per our "no type casting" guideline.
const report = option as SearchOption<Report>;Casts bypass the type checker: if option isn't actually a SearchOption<Report> at runtime, report.item?.invoiceReceiver reads are unsound. This was moved verbatim from the old inline block, so it's pre-existing — but since it now lives in a shared helper, consider a proper narrowing (e.g. a type guard on option.item) instead of the assertion.
| const contextualPoliciesMap = (() => { | ||
| if (!contextualReport?.policyID || !contextualReportPolicy) { | ||
| return {}; | ||
| } | ||
| const policyKey = `${ONYXKEYS.COLLECTION.POLICY}${contextualReport.policyID}`; | ||
| return {[policyKey]: contextualReportPolicy}; | ||
| })(); | ||
|
|
||
| const contextualReportsMap = (() => { | ||
| if (!contextualReportID || !contextualReport) { | ||
| return {}; | ||
| } | ||
| const reportKey = `${ONYXKEYS.COLLECTION.REPORT}${contextualReportID}`; | ||
| return {[reportKey]: contextualReport}; | ||
| })(); |
There was a problem hiding this comment.
NAB contextualPoliciesMap / contextualReportsMap recompute every render via IIFE.
React Compiler will memoize these so there's no runtime concern — flagging only because the IIFE-assigned-to-const pattern reads oddly next to the surrounding hook-based code. useMemo would make the memoization intent explicit. Pre-existing (just relocated), so fine to leave.
| expect(result.reportID).toBe('1'); | ||
| }); | ||
|
|
||
| it('should keep the room name for a chat room unless showPersonalDetails is enabled', () => { |
There was a problem hiding this comment.
Good test for the room-name fix — but the shared scoping helpers have no coverage.
This test correctly pins the createOptionFromReport room-name behavior ✅. However the actual new logic — getContextualSearchTarget, getContextualSearchQuery, and getContextualSearchAutocompleteKey — is untested. These are pure, exportable functions, so a small table-driven test over {chat, room, invoice individual, invoice non-individual, policy expense, self DM} would cheaply lock in the scoping/substitution logic and guard the shared helper against regressions. I'd like to see that added before merge.
| } | ||
|
|
||
| function getContextualSearchAutocompleteKey(item: SearchQueryItem, policies: OnyxCollection<OnyxTypes.Policy>, reports?: OnyxCollection<OnyxTypes.Report>) { | ||
| type ContextualSearchTarget = Pick<SearchQueryItem, 'searchQuery' | 'autocompleteID' | 'roomType' | 'policyID'>; |
There was a problem hiding this comment.
Nice use of Pick here.
Pick<SearchQueryItem, 'searchQuery' | 'autocompleteID' | 'roomType' | 'policyID'> is the right, cast-free way to narrow the shared parameter type so both helpers accept the lightweight target object without dragging in the full SearchQueryItem. Consider exporting it so getContextualSearchTarget in SearchRouter.tsx can annotate its return with the same type (see the return-type comment on that function).
|
Let's fix the workspace name/ID issuse separately, but it does need fixing. |
OK, i'll have a follow-up PR for that, maybe with separate issue? @twisterdotcom |
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
Go to any chat
Click Search icon or CMD+K
Verify search input is pre-populated with type:chat in:currentReport
Woah, @twisterdotcom @dannymcclain. I do not think we want to do that. That'll make chat switching incredibly cumbersome by having to clear the contents of the router first.
it's just confusing why the cmd+k popup doesn't show Search in #admins and does show Search in CFO Expensive Pie which is the display name of this current user.
^^ My understanding is this is the bug to focus on.
Totally agree. cc @Expensify/design
Also agree. I think we should also change the default keyboard focus when opening the router—you can see here how I tried to hit command + k and then just arrow down to the CleanShot.2026-07-23.at.10.24.23.mp4Ideally, I could hit command + k, arrow down with the keyboard one time, hit enter, and type my search for the current chat. I think that would be a pretty good experience for this use case. |
|
well to be more clear, we are regretting the feauture asked in original issue and found a new bug: cc @marufsharifi
|
And here is: RCAFocus skips past the contextual suggestion because 1. The target index is computed by walking sections to the first recent report: App/src/components/Search/SearchAutocompleteList.tsx Lines 647 to 674 in 812a277 2. That index is applied declaratively via 3. And again imperatively, because late-arriving options don't pick up App/src/components/Search/SearchAutocompleteList.tsx Lines 678 to 688 in 812a277 Neither path is aware that a contextual suggestion sits at index 0, so both land on the first recent chat. Why That value feeds the keyboard-focus manager: SolutionDerive Gives exactly the requested flow: cmd+K → ArrowDown → Enter → type. Tradeoff needing sign-offWith focus unset, cmd+K → Enter no longer opens the most recent chat — So are we okay with that? @trjExpensify @twisterdotcom @shawnborton @dannymcclain |
Personally I would be ok with this - but I'd like to hear from more of the team. Also, when opening the router from a chat room, could we focus that first item (Search in {roomName}) so that you could just do command + k, then enter, then type your query? (Eliminating the need to key down to it?) Definitely definitely curious for more thoughts here. |
Hmm yes okay I like this. |
The list focused the first recent chat on open, so reaching "Search in <chat>" meant arrowing up past every row above it. Focus that suggestion instead when it is shown, letting Enter scope the search to the chat the user is already looking at. Move the section walk into a helper so the flat index, which has to count header rows, can be tested on its own.
well that makes sense. Let me implement it |
|
@marufsharifi I think now your previous reviews are obsolete since I had to revert search input pre-population logic. Can you, pls, re-review this? thanks |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeScreen_Recording_20260724_124733_Chrome.mp4iOS: HybridAppScreen.Recording.2026-07-24.at.3.09.55.PM.moviOS: mWeb SafariScreen.Recording.2026-07-24.at.12.50.16.PM.movMacOS: Chrome / SafariScreen.Recording.2026-07-24.at.12.30.39.PM.mov |
|
Let us know when this is ready for an ad hoc build - would love for @twisterdotcom, @trjExpensify, and @Expensify/design to get to test it for real. |
|
@mukhrr, could you please merge the main. thanks. |
@dannymcclain I think this can already be adhoc built. WDYT, @marufsharifi ? |
|
I'll run the adhoc machine 👍 |
|
🚧 dubielzyk-expensify has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
Is it expected that you have to click enter to search in the room? It feels a bit odd, but I guess it's expected to default to global search. Took me a try to understand though. CleanShot.2026-07-27.at.14.16.26.mp4 |
|
@trjExpensify, @twisterdotcom, gentle bump on this #96448 (comment). thanks. |
|
You just want us to take a look at the ad-hoc @marufsharifi? |
Yeah, that's expected for the build. Instead of taking over global search entirely (which would make chat switching cumbersome), the idea was to just focus the My only other thought for an alt is to add a secondary keyboard shortcut that automatically starts with the But if we don't want to do something like that, I think the build is pretty good. Definitely curious for others' thoughts. |
This is a setting they have, so not the default, which I only found out recently too: https://expensify.slack.com/archives/C03U7DCU4/p1784070488097879?thread_ts=1784069966.021469&cid=C03U7DCU4 |
This is generally how I'm feeling too, I guess it feels pretty familiar based on how other apps handle it at least? |
|
Sounds good to me 👍 |
| const personalDetailsOption = createOptionFromReport(report, PERSONAL_DETAILS, undefined, POLICY, sortedActions, undefined, {showPersonalDetails: true}); | ||
|
|
||
| expect(roomOption.text).toBe('#admins'); | ||
| expect(personalDetailsOption.text).not.toBe('#admins'); |
There was a problem hiding this comment.
Tighten this assertion — it only proves "changed", not "changed to the right thing".
expect(personalDetailsOption.text).not.toBe('#admins');This passes for any non-#admins value, including an accidental '' or undefined. Assert the actual expected participant display name so a future regression that produces an empty string is caught:
expect(personalDetailsOption.text).toBe(/* expected participant display name */);
|
@MelvinBot review |
ReviewOverall this looks solid and correctly implements both the room-name fix and the "focus the 1. Weak test assertion (should tighten before merge) — Already flagged in this earlier comment and still unaddressed. In expect(personalDetailsOption.text).not.toBe('#admins');This passes for any non- 2. 3. Verification notes
The earlier review threads about |
|
@mukhrr, could you please check the above comments? thanks. |
8 participants
Explanation of Change
Fixed both bug found here #96152 (comment) and implement the feature original issue mentions
Fixed Issues
$ #96152
PROPOSAL: #96152 (comment)
Tests
Verify first item in search, i.e, current chat is focused and can be easily entered into seach input by Enter on web
Verify that no errors appear in the JS console
Offline tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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_app.mp4
Android: mWeb Chrome
android_web.mp4
iOS: Native
ios_app.mp4
iOS: mWeb Safari
ios_web.mp4
MacOS: Chrome / Safari
web.mp4