Quote contextual search values containing a comma - #97083
Conversation
Clicking a contextual search suggestion seeds the query with the workspace or room name and registers the entity ID under a substitution key built from that same full name. The parser ends an unquoted value at a comma, so a name like `Acme,Inc` was read back as the two values `Acme` and `Inc`, the key never matched, the substitution was skipped and the literal name was sent to the server as a policyID or reportID. Quote the seeded value when it contains a comma so it parses as a single value and resolves to the ID. Fixes Expensify#96767
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@parasharrajat 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: 478b36a8d1
ℹ️ 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 sanitizeContextualSearchValue(value: string) { | ||
| return value.includes(',') ? `"${value}"` : sanitizeSearchValue(value); |
There was a problem hiding this comment.
When a workspace or group chat name contains both a comma and a quotation mark, such as Acme "US",Inc, this produces workspace:"Acme "US",Inc". The autocomplete parser splits that into multiple values, so none matches the substitution key built from the full name and the policy/report ID is not substituted. These names are valid because the workspace and group-chat name forms only enforce length, so the contextual search remains broken for this input; the quoting logic needs to escape or otherwise preserve embedded quote characters.
Useful? React with 👍 / 👎.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| function sanitizeContextualSearchValue(value: string) { |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
sanitizeContextualSearchValue re-implements the "${value}" quote-escaping that already lives inside sanitizeSearchValue (which wraps a value in quotes when it contains a space or non-breaking space). This duplicates the escaping mechanism and creates two places that must stay in sync about how a value is quoted for the search query syntax. The comma is simply another delimiter that requires the same quoting, so it belongs in the shared helper rather than in a bypass wrapper.
Extend the shared sanitizeSearchValue to also quote on comma, then drop the wrapper and keep the original call sites:
// in SearchQueryUtils.ts
function sanitizeSearchValue(str: string) {
if (str.includes(' ') || str.includes('\xA0') || str.includes(',')) {
return `"${str}"`;
}
return str;
}This removes the duplicated "${value}" logic and lets every existing sanitizeSearchValue caller benefit from comma handling for free.
Reviewed at: 478b36a | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
A workspace or room name may contain a quotation mark, and the grammar has no way to represent one inside a value: the quoted-string rule excludes quotes outright, and the nested variant treats a quote followed by a comma as the closing quote. So `Acme "US",Inc` was seeded as `workspace:"Acme "US",Inc"`, split into two values, matched no substitution key, and the name reached the server in place of the policy ID. Drop quote characters from the value, and build the substitution key from that same value so the two always agree. Quoting on a comma also moves into sanitizeSearchValue behind an opt-in parameter rather than being re-implemented at the call site. It stays off by default, so existing callers are unaffected. The two `.map(sanitizeSearchValue)` call sites now pass an explicit lambda, since map would otherwise hand the array index to the new parameter.
Screenshots🔲 iOS / native🔲 iOS / Safari🔲 MacOS / Chrome🔲 Android / Chrome🔲 Android / native |
2 participants
Explanation of Change
Clicking a contextual search suggestion seeds the query with the workspace or room name and registers the entity ID under a substitution key built from that same full name. The parser ends an unquoted value at a comma, so a name like
Acme,Incwas read back as the two valuesAcmeandInc, the key never matched, the substitution was skipped and the literal name was sent to the server as a policyID or reportID.Quote the seeded value when it contains a comma so it parses as a single value and resolves to the ID.
Fixed Issues
$ #96767
PROPOSAL: #96448 (comment)
Tests
type:expenseworkspace:Acme,Incquoted.Verify you see results from Acme, Inc. workspace
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_web.mp4
Android: mWeb Chrome
android_web.mp4
iOS: Native
ios_app.mp4
iOS: mWeb Safari
ios_web.mp4
MacOS: Chrome / Safari
web.mp4