Add cache to buildSearchQueryJSON to avoid re-parsing identical queries#82931
Conversation
5fbe839 to
2bc1144
Compare
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
4c8fc7e to
c74ed92
Compare
|
@Krishna2323 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] |
|
Reviewing... |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c74ed925f0
ℹ️ 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".
src/libs/SearchQueryUtils.ts
Outdated
| if (buildSearchQueryJSONCache.has(cacheKey)) { | ||
| return buildSearchQueryJSONCache.get(cacheKey); |
There was a problem hiding this comment.
Avoid returning mutable cached query objects
This now returns the same cached SearchQueryJSON instance to every caller, but some callers mutate the returned object in place (for example SearchChartView sets newQueryJSON.groupBy and newQueryJSON.view in src/components/Search/SearchChartView.tsx:165-166). After one such mutation, later buildSearchQueryJSON() calls for the same query can observe altered fields and stale hashes from the pre-mutation parse, which changes search behavior based on call order instead of input.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a critical bug.
There was a problem hiding this comment.
Fixed, both cache miss and hit paths now return a shallow clone, so the cached object is never exposed directly. Checked all other callers and SearchChartView seems to be the only direct mutation site. Everything else is read-only, uses spread, or cloneDeep.
src/libs/SearchQueryUtils.ts
Outdated
| const BUILD_SEARCH_QUERY_JSON_CACHE_MAX_SIZE = 50; | ||
|
|
||
| function buildSearchQueryJSON(query: SearchQueryString, rawQuery?: SearchQueryString) { | ||
| const cacheKey = rawQuery ? `${query}|||${rawQuery}` : query; |
There was a problem hiding this comment.
Build an unambiguous cache key for query/rawQuery
The cache key rawQuery ? ${query}|||${rawQuery} : query can collide for valid inputs (e.g. query='foo|||bar', rawQuery=undefined vs query='foo', rawQuery='bar'), because the parser allows | in free-text tokens (src/libs/SearchParser/searchParser.peggy, freeTextFilter matches non-whitespace characters). A collision returns the wrong cached parse result and hash for the current inputs.
Useful? React with 👍 / 👎.
|
@OlimpiaZurek Does it make sense to add unit tests for the cache? Since it's a module-level |
heyjennahay
left a comment
There was a problem hiding this comment.
Product review not required
Good idea! Added a few tests, including one that directly verifies the mutation fix. |
|
@OlimpiaZurek I think manual testing would be helpful here since the cache impacts search behavior globally. We can add this as a QA test. Chart view mutation scenario
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid_native.mp4Android: mWeb Chromeandroid_chrome.mp4iOS: HybridAppios_native.mp4iOS: mWeb Safariios_safari.mp4MacOS: Chrome / Safariweb_chrome.mp4 |
Krishna2323
left a comment
There was a problem hiding this comment.
LGTM! 🚀
We can add QA tests: #82931 (comment)
|
🚧 @luacmartins has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
|
||
| // Cache for buildSearchQueryJSON to avoid re-running the PEG parser for identical queries. | ||
| // This is a pure function called from 64+ sites — many fire during the same render cycle | ||
| // with identical query strings, each running the full parser from scratch. |
There was a problem hiding this comment.
| // with identical query strings, each running the full parser from scratch. | |
| // with identical query strings and without this cache we would run the full parser from scratch. |
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.3.25-0 🚀
|
Explanation of Change
This PR adds a simple LRU cache to the
buildSearchQueryJSONfunction to avoid re-parsing identical search queries with the PEG parser. The function is called from 64+ sites across the codebase, many of which fire during the same render cycle with identical query strings, causing the parser to run multiple times unnecessarily.The cache uses a Map with a maximum size limit of 50 entries. When the limit is reached, the oldest entry is removed (simple FIFO eviction). The cache key includes both the main query and the optional rawQuery parameter to ensure correct cache hits.
Fixed Issues
$ #82994
Tests
Offline tests
N/A
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodAvatar, 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