Perf: Defer autocomplete computations and remove debug logs#83312
Perf: Defer autocomplete computations and remove debug logs#83312
Conversation
Extract useAutocompleteSuggestions hook from SearchAutocompleteList to guard expensive autocomplete computations behind the autocomplete key. On initial open (empty query), none of the heavy data processing runs, reducing JS thread blocking during the ManualOpenSearchRouter span. Also removes temporary CMD_K_DEBUG/CMD_K_FREEZE diagnostic logging from SearchRouter, SearchPageHeaderInput, and SearchAutocompleteList. Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
…tests Use proper types from the codebase (OptionList, DismissedProductTraining, Beta[], FeedKeysWithAssignedCards) instead of loosely typed alternatives. Fix test params to match corrected types and use optional chaining for .at() array access. Co-authored-by: Cursor <cursoragent@cursor.com>
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
aimane-chnaif
left a comment
aimane-chnaif
left a comment
There was a problem hiding this comment.
Confirmed all codes are moved correctly into new hook
| // We don't want to show the "Expensify Card" feeds in the autocomplete suggestion list as they don't have real "Statements" | ||
| // Thus passing an empty object to the `allCards` parameter. |
There was a problem hiding this comment.
Keep this comment in new hook
There was a problem hiding this comment.
Done — restored in 8051efe. The comment about Expensify Card feeds not having real "Statements" is now above the getCardFeedsForDisplay call in the FEED case of useAutocompleteSuggestions.ts.
| // If autocompleteValue is empty or just whitespace and we have already completed keys, | ||
| // return empty array to hide suggestion list (consistent with group-by behavior) |
There was a problem hiding this comment.
Keep this comment in new hook
There was a problem hiding this comment.
Done — restored in 8051efe. The comment explaining the early return for empty autocompleteValue with already-completed keys is now above the guard in the IN case of useAutocompleteSuggestions.ts.
|
What's remaining to mark this as ready for review? |
Preserve two explanatory comments from the original SearchAutocompleteList that were dropped during the hook extraction, as flagged by reviewer. Co-authored-by: Cursor <cursoragent@cursor.com>
melvin-bot
bot
requested review from
aimane-chnaif and
heyjennahay
and removed request for
a team and
heyjennahay
|
@aimane-chnaif 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] |
|
@aimane-chnaif nothing, can you please test and review? |
|
@JakubKorytko @staszekscp for a review too please |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| .slice(0, 10); | ||
|
|
||
| return filteredCurrencies.map((currencyName) => ({ | ||
| filterKey: getUserFriendlyKey(autocompleteKey), |
There was a problem hiding this comment.
❌ PERF-13 (docs)
getUserFriendlyKey(autocompleteKey) does not depend on the iterator variable currencyName and produces the same result on every iteration. It should be hoisted outside the .map() call.
const friendlyKey = getUserFriendlyKey(autocompleteKey);
return filteredCurrencies.map((currencyName) => ({
filterKey: friendlyKey,
text: currencyName,
}));Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Fixed in cb5d090. Hoisted getUserFriendlyKey(autocompleteKey) outside the .map() into a friendlyKey variable.
| getHasOptions: jest.fn(() => [{value: 'attachment'}, {value: 'note'}]), | ||
| })); | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/naming-convention |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
This eslint-disable lacks a comment explaining why the naming convention rule needs to be disabled.
Add a justification comment, for example:
// eslint-disable-next-line @typescript-eslint/naming-convention -- jest.requireMock returns a module-shaped object; destructured name must match the original export
const {parseForAutocomplete} = jest.requireMock<{parseForAutocomplete: jest.Mock}>('@libs/SearchAutocompleteUtils');Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Fixed in cb5d090. Added justification: jest.requireMock returns a module-shaped object whose destructured name must match the original export.
Address AI reviewer feedback: - PERF-13: Hoist iterator-independent getUserFriendlyKey() call outside the .map() in the currency case - CONSISTENCY-5: Add justification comment for eslint-disable in tests Co-authored-by: Cursor <cursoragent@cursor.com>
| return filteredStatuses.map((status) => ({filterKey: CONST.SEARCH.SEARCH_USER_FRIENDLY_KEYS.STATUS, text: status})); | ||
| } | ||
| case CONST.SEARCH.SYNTAX_FILTER_KEYS.EXPENSE_TYPE: { | ||
| const expenseTypes = Object.values(CONST.SEARCH.TRANSACTION_TYPE).map((value) => getUserFriendlyValue(value)); |
There was a problem hiding this comment.
Is it worth moving this kind of defines (not depending on component lifecycle) to the top?
There was a problem hiding this comment.
Good catch! Hoisted all static Object.values(CONST.*) and .map(getUserFriendlyValue) derivations to module-level constants so they're computed once at module load rather than on every hook invocation. This covers DATA_TYPE_VALUES, GROUP_BY_FRIENDLY_VALUES, VIEW_FRIENDLY_VALUES, EXPENSE_TYPE_FRIENDLY_VALUES, WITHDRAWAL_TYPE_VALUES, BOOLEAN_VALUES, ACTION_FILTER_VALUES, IS_VALUES_LIST, and all the status arrays.
See commit 3044327.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-02-25.at.11.16.47.am.mov |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ 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". |
|
aw but conflicts |
Address reviewer feedback to move Object.values(CONST.*) and .map(getUserFriendlyValue) derivations outside the hook body so they are computed once at module load rather than on every invocation. Made-with: Cursor
…rAutocomplete Made-with: Cursor # Conflicts: # src/components/Search/SearchPageHeader/SearchPageHeaderInput.tsx # src/components/Search/SearchRouter/SearchRouter.tsx
Made-with: Cursor
Made-with: Cursor
|
Resolved though there has been bunch of changes, @aimane-chnaif do you think you could retest please once more? |
|
Conflicts again |
Resolve merge conflict from main adding visibleReportActionsData to getSearchOptions calls and the new EXPORTED_TO autocomplete case. Both are incorporated into the useAutocompleteSuggestions hook. Reset Mobile-Expensify submodule pointer to match main. Made-with: Cursor
Made-with: Cursor
|
That is failing on main, looking into that |
Being fixed in #83619 |
|
Not the TS, that is fixed here #83628 |
|
Ah yes, 2 different checks are broken at the same time on main 😅 |
|
What a moment to be alive |
|
@dangrous this PR is ready for a final review |
|
🚧 @dangrous 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/dangrous in version: 9.3.27-0 🚀
|
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.3.27-8 🚀
|
4 participants
Explanation of Change
Extracts a
useAutocompleteSuggestionshook fromSearchAutocompleteList.tsxto defer expensive autocomplete computations until the user actually types an autocomplete prefix (e.g.,tag:,category:,from:). On initial open with an empty query,autocompleteKeyisundefinedand the hook returns[]immediately — skipping all heavy data processing that previously ran on every render.Key changes:
New
useAutocompleteSuggestionshook — encapsulates all autocomplete suggestion logic (parsing, continuation detection, filtering, and the entire switch-case). Guards all expensive computations behindif (!autocompleteKey) return [](PERF-2: early return before expensive work).Simplified
SearchAutocompleteList— reduced from ~930 lines to ~450 lines by extracting the autocomplete logic. The component now calls the hook and focuses on rendering.Removed temporary
CMD_K_DEBUG/CMD_K_FREEZEdiagnostic logging fromSearchRouter.tsx,SearchPageHeaderInput.tsx, andSearchAutocompleteList.tsx. This was temporary instrumentation that added overhead (Date.now(), Math.random(), try/catch, Log.info/alert calls) on every interaction.What gets deferred on initial open (empty query):
getAutocompleteCategories()/getAutocompleteRecentCategories()getAutocompleteTags()/getAutocompleteRecentTags()getAutocompleteTaxList()workspaceListiteration over all policiescardAutocompleteList/feedAutoCompleteListcurrencyAutocompleteListfilteringgetSearchOptions()calls within autocomplete casesFixed Issues
$ #79353
Tests
tag:and verify tag suggestions appearcategory:and verify category suggestions appearfrom:and verify participant suggestions appearcurrency:uand verify currency suggestions appear (USD, AUD, EUR)type:and verify type suggestions appearfrom:(e.g.,from:John Doe) and verify continuation detection worksnpx jest tests/unit/hooks/useAutocompleteSuggestions.test.tsOffline tests
N/A — autocomplete is computed from locally cached Onyx data, no network dependency.
QA Steps
tag:,category:,from:,currency:,type:,status:) and verify suggestions appearPR 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