Escape search query values and resolve typed workspace names - #98522
Escape search query values and resolve typed workspace names#98522mukhrr wants to merge 5 commits into
Conversation
Two mechanisms handled query values and each dropped something. The grammar had no way to write a quotation mark, so a workspace or room called `Acme "US",Inc` could not be expressed and the previous fix silently deleted the quotes. And a name that was typed rather than picked was never mapped to an ID, so the literal text went to the server and the search came back empty. Give the grammar an escape character. A backslash only escapes what the parser would otherwise consume, so a value already stored in a saved search or a shared URL keeps the meaning it has today, including one that contains a lone backslash. Both parsers are regenerated from the shared rules. Ranges now carry the offset and width of the text each part actually matched. Walking the parsed values was only correct while a value and its source were the same length, which escaping breaks, and those ranges position both the substitutions and the live markdown highlighting. Write the escapes in sanitizeSearchValue, and resolve a typed workspace name to its ID when exactly one workspace matches. Names are not unique, so an ambiguous one is left alone rather than guessing. With any value now expressible, the quote stripping workarounds are gone. A comma inside quotes needs no escape, so a value that held one still serializes byte for byte as before and keeps its query hash. Only a value containing a quote or a backslash serializes differently, and those are the values that do not survive a round trip today.
|
@gijoe0295 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] |
|
This PR adds a new |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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.
|
![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: cf92845360
ℹ️ 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".
Submitting a query rebuilds it through buildFilterValuesString, which asked sanitizeSearchValue not to quote commas. A value that holds one was written back bare, so `merchant:Acme\,Inc` and `merchant:"Acme,Inc"` both came out as `merchant:Acme,Inc` and the next read took them as two merchants. The quoted form was already lost this way before the escape character existed. Callers pass one value at a time and join them with commas themselves, so a comma inside a value is always part of it. Quote on it unconditionally and drop the opt in, which only invited a caller to forget it. A list of values is unaffected, since each value is sanitized on its own and only one that contains a comma gains quotes. Take the display name lookup out of the loop that compares it, justify the policy subscription, and read ranges back through the typed autocomplete wrapper so the tests add no assertions.
Resolving a typed workspace name read a module level cache filled by its own Onyx subscription, a second copy of data the callers already hold and one whose timing nothing guarantees. Take the collection as a parameter instead. Every caller is a component or a hook that already reads it, so the value now comes from a subscription that has settled before the render that uses it. Drop the workspace named with a bare comma from the tests. The remaining cases still cover a comma inside a value, which is what the quoting is for, under a name that reads as one workspace rather than two.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c7af07f26
ℹ️ 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".
1 participant
Explanation of Change
Adds an escape character to the search grammar so a value containing a quote or a comma can be written at all, writes those escapes in
sanitizeSearchValue, and resolves a typed workspace name to its ID when exactly one workspace matches. The quote-stripping workarounds from #97083 are removed.A backslash only escapes what the parser would otherwise consume, so a value already stored in a saved search or a shared URL keeps its current meaning, including one containing a lone backslash.
Submitting a query rebuilds it, and that rebuild used to write a value holding a comma back out bare, so
merchant:"Globex,Ltd"returned as two merchants. Every value containing a comma is now quoted. A comma separated list is unaffected, since each value is sanitized on its own.Values containing a comma, a quote or a backslash therefore serialize differently than before and their query hash changes. Those are exactly the values that do not survive a round trip today.
Fixed Issues
$ #98374
PROPOSAL:
Tests
Setup: create two workspaces named exactly
Acme, Inc.andAcme "US",Inc.type:expense workspace:"Acme, Inc."and press Enter without clicking a suggestion.policyID:<the workspace's ID>, not the literal name, and the Workspace filter shows that workspace as selected.type:expense workspace:Acmeand click theAcme "US",Incsuggestion.workspace:"Acme \"US\",Inc"— the full name including quotes, notAcme US,Inc.type:expense workspace:"Acme \"US\",Inc"by hand and press Enter.Acme, Inc., reload, and verify the saved search still loads and still shows as saved.from:and pick them.merchant:"Globex,Ltd"and press Enter.category:Travel,Mealsis still treated as two categories.Offline tests
Same as above. Name→ID resolution reads workspaces from Onyx, so it works offline; the search request itself queues as usual.
QA Steps
Same as the Tests section.
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.app.mp4
iOS: Native
ios_app.mp4
iOS: mWeb Safari
IOS-web.mp4
MacOS: Chrome / Safari
web.mp4