Skip to content

83207: debounce autocomplete query and memoize search options#86925

Open
abbasifaizan70 wants to merge 44 commits into
Expensify:mainfrom
abbasifaizan70:83207
Open

83207: debounce autocomplete query and memoize search options#86925
abbasifaizan70 wants to merge 44 commits into
Expensify:mainfrom
abbasifaizan70:83207

Conversation

@abbasifaizan70

@abbasifaizan70 abbasifaizan70 commented Apr 1, 2026

Copy link
Copy Markdown
Contributor

Explanation of Change

The SearchRouter was passing autocompleteQueryValue (updated via useState on every keystroke) directly to SearchAutocompleteList, triggering synchronous getSearchOptions() + combineOrderingOfReportsAndPersonalDetails() + isSearchStringMatch() on every character typed, freezing the JS thread on mobile.

Structural optimizations (debouncing + memoization prevent redundant work):

  • useDebouncedState for autocompleteQueryValue: Replaces useState so the expensive SearchAutocompleteList filtering only runs after the user pauses typing, not on every keystroke. The immediate value still drives arrow-key navigation and contextual logic.
  • useMemo for searchOptions: Wraps getSearchOptions() so it only re-runs when its 13 dependencies actually change, instead of re-computing on every render.
  • useMemo for recentReportsOptions: Wraps combineOrderingOfReportsAndPersonalDetails() so sorting/filtering is skipped when autocompleteQueryValue and searchOptions haven't changed.

Algorithmic improvement:

  • isSearchStringMatch: new RegExp() per word per item → compile once per call, reuse across all items
  • isSearchStringMatch: matching flag with continue past failures → early return false on first mismatch
  • searchWords: new Set() keeping empty strings → Array.from(new Set(...)).filter(Boolean)

Performance (Reassure, 40K reports):

Metric main (baseline) PR (current) Change
getFilteredOptions with search 2.8 ms 2.5 ms -0.3 ms (-11.7%)
filterAndOrderOptions (40K items, 3-word query) 11.1 ms 10.4 ms -0.7 ms (-6.4%)

Fixed Issues

$ #83207
PROPOSAL: #83207 (comment)

Tests

  1. Open the app on a mobile device (iOS or Android) and tap the search icon in Inbox
  2. Type rapidly into the search input (e.g. "Email Report Five") — verify the input stays responsive with no visible lag or dropped characters
  3. Stop typing and wait briefly — verify search results update and reflect the typed query
  4. Clear the search input — verify recent chats reappear immediately
  5. Type a partial query (e.g. "Exp") — verify autocomplete suggestions appear after a short delay
  6. Select an autocomplete suggestion — verify the query updates correctly and results reflect the selection
  7. Use arrow keys (desktop) or scroll (mobile) to navigate through the results list while typing — verify navigation remains responsive
  8. Type a multi-word query (e.g. "John Expense Report") — verify all matching results are returned correctly
  9. Open search on a device/account with many reports and contacts — verify no JS thread freeze or unresponsiveness during typing
  10. Press the back button or swipe to dismiss search while typing — verify navigation is not blocked
  • Verify that no errors appear in the JS console

Offline tests

Same as tests.

QA Steps

Same as tests.

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I verified there are no new alerts related to the canBeMissing param for useOnyx
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I used JaimeGPT to get English > Spanish translation. I then posted it in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.ts or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native
Screen.Recording.2026-04-02.at.4.04.17.AM.mov
Android: mWeb Chrome
Screen.Recording.2026-04-02.at.4.05.33.AM.mov
iOS: Native
Screen.Recording.2026-04-02.at.3.57.50.AM.mov
iOS: mWeb Safari
Screen.Recording.2026-04-02.at.3.58.48.AM.mov
MacOS: Chrome / Safari
Screen.Recording.2026-04-02.at.3.56.08.AM.mov

@codecov

codecov Bot commented Apr 1, 2026

Copy link
Copy Markdown

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.

Files with missing lines Coverage Δ
src/components/Search/SearchAutocompleteList.tsx 92.34% <78.57%> (-0.33%) ⬇️
...rc/components/Search/SearchRouter/SearchRouter.tsx 50.00% <37.50%> (-1.88%) ⬇️
src/libs/OptionsListUtils/index.ts 85.11% <0.00%> (+0.12%) ⬆️
... and 103 files with indirect coverage changes

@abbasifaizan70 abbasifaizan70 marked this pull request as ready for review April 1, 2026 23:45
@abbasifaizan70 abbasifaizan70 requested review from a team as code owners April 1, 2026 23:46
@melvin-bot melvin-bot Bot requested a review from aimane-chnaif April 1, 2026 23:46
@melvin-bot

melvin-bot Bot commented Apr 1, 2026

Copy link
Copy Markdown

@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]

@melvin-bot melvin-bot Bot requested review from trjExpensify and removed request for a team April 1, 2026 23:46

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4e8ba5a482

ℹ️ 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".

Comment thread src/components/Search/SearchRouter/SearchRouter.tsx Outdated
@trjExpensify

Copy link
Copy Markdown
Contributor

PR doesn’t need product input as a performance PR. Unassigning and unsubscribing myself.

@trjExpensify trjExpensify removed their request for review April 2, 2026 11:58
@abbasifaizan70 abbasifaizan70 changed the title 82307: debounce autocomplete query and memoize search options 83207: debounce autocomplete query and memoize search options Apr 2, 2026
@aimane-chnaif

Copy link
Copy Markdown
Contributor

Please merge main

@abbasifaizan70

Copy link
Copy Markdown
Contributor Author

@aimane-chnaif Update with the latest main. Thanks

@MelvinBot

Copy link
Copy Markdown
Contributor

PR Review

All 30 CI checks pass. No merge conflicts detected (MERGEABLE). However, the last commit was May 6 — aimane-chnaif asked to fix conflicts on May 13, so you may want to merge main again to ensure the branch is fully up to date before merging.

Looks Good

  • Debounce architecture is correct: textInputValue via useState (immediate) for UI responsiveness + debouncedAutocompleteQueryValue via useDebouncedState (300ms) for expensive filtering. Clean separation of concerns.
  • Mid-debounce stale-result guard (lines 375-382 in SearchAutocompleteList.tsx): Returns [] when debounced query is empty but immediate input is non-empty — correctly prevents the "Recent searches" flash regression aimane-chnaif flagged earlier.
  • Submit guard (lines 389-400 in SearchRouter.tsx): During the debounce window, preserves focused search-query items (e.g. Ask Concierge) while falling back to raw text search for stale non-search items. Good edge-case handling.
  • isSearchStringMatch optimization: Regex pre-compilation, .filter(Boolean) for empty strings, and early return false are all correct and functionally equivalent to the old behavior.
  • Test coverage is thorough: 40K-item perf test, render-count test, and multi-space normalization unit test all target the right code paths.

Action needed

abbasifaizan70 — please merge main to address the reviewer's May 13 request and ensure the branch is current.

No blocking code issues.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ad06633a9f

ℹ️ 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".

Comment on lines 398 to 402
if (!handleSearch || !autocompleteQueryWithoutFilters) {
return;
}

handleSearch(autocompleteQueryWithoutFilters);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Restore debounce before calling handleSearch

Calling handleSearch directly in this effect now fires a server search on every autocompleteQueryValue change, but SearchAutocompleteList is also used by SearchPageInputWide and SearchPageInputNarrow, where useSearchPageInput updates autocompleteQueryValue on each keystroke. That means the search page path (SearchPage.tsxsearchInServer) now issues/cancels network requests and toggles loading state per keypress instead of using the prior debounce window, which can cause request churn and UI flicker for fast typers or high-latency connections.

Useful? React with 👍 / 👎.

@MelvinBot

Copy link
Copy Markdown
Contributor

Regression Analysis

isSearchStringMatch refactor: No regressions. The debounce architecture changes introduce two behavioral changes worth validating — one was already flagged and fixed, the other is minor.

1. Flash of empty content during debounce window (known, mitigated)

During the 300ms debounce window after the first keystroke, autocompleteQueryValue is still '' while hasEffectiveInputQuery is true. The recentReportsOptions memo hits the autocompleteQueryValue.trim() === '' → return [] guard, producing an empty list. Simultaneously, "Recent Searches" and "Recent Chats" headers are hidden (because hasEffectiveInputQuery is true).

Result: The user sees an empty list for up to 300ms on the first keystroke before results appear. The old code showed stale recent chats during this window. This was flagged by aimane-chnaif and the return [] guard was the fix — it trades stale results for a brief empty state. This is an intentional UX trade-off, not a bug.

2. Focus resets on every keystroke instead of per-debounce

prevQueryRef now tracks effectiveInputQueryValue (immediate) instead of autocompleteQueryValue (debounced). This means updateAndScrollToFocusedIndex(0, true) fires on every keystroke — typing "hello" resets focus 5 times vs once in the old code. This could cause visible scroll-to-top jumps if the user arrow-keys into the list while typing.

Risk: Low. Users typically don't arrow-navigate mid-word. But if they do, their arrow-key position gets reset on the next character.

3. Fast-Enter during debounce window

The submit guard checks textInputValue !== debouncedAutocompleteQueryValue. During the debounce window, if a non-search-query item is focused (e.g. a report from stale results), the guard bypasses it and falls back to submitSearch(textInputValue). This is correct for stale items but means a user who intentionally arrow-keys to a report and hits Enter within 300ms gets a text search instead. Search-query items (like "Ask Concierge" or the typed-query row) are correctly dispatched.

Risk: Low. The guard is protective, and the edge case requires arrow-keying + Enter within the debounce window.

4. isSearchStringMatch refactor — No regression

.filter(Boolean) removes empty strings that were no-ops (empty regex /(?:)/i matches everything). Early return false is equivalent to the old matching flag + continue. Pre-compiled regexes are behaviorally identical. The unit test validates multi-space normalization equivalence.

5. Double-debounce removal — Correct, no regression

The old code had useDebounce wrapping handleSearch inside SearchAutocompleteList on top of useDebouncedState in SearchRouter — a double debounce. The PR correctly removes the inner debounce since autocompleteQueryValue is already debounced upstream.

Key architectural note

The old code's useDebouncedState for textInputValue was effectively dead: [textInputValue, , setTextInputValue] = useDebouncedState('', 500) destructured position 0 (the immediate value) and discarded position 1 (the debounced value). Combined with autocompleteQueryValue={autocompleteQueryValue || textInputValue}, the autocomplete list received the immediate value on every keystroke — there was no actual debouncing happening. This PR introduces real 300ms debouncing for the first time.


Verdict: No hard regressions. Items #1-#3 are known behavioral changes from introducing real debouncing. Recommend manual verification of arrow-key navigation during rapid typing (item #2).

@aimane-chnaif

Copy link
Copy Markdown
Contributor

@MelvinBot find performance regressions for high traffic accounts

@MelvinBot

Copy link
Copy Markdown
Contributor

Performance Regression Analysis for High-Traffic Accounts

TL;DR: No hard regressions in computation cost. One structural concern: the PR introduces 1 extra re-render per keystroke of SearchAutocompleteList that didn't exist before. The expensive memos are protected, but the component body itself (20+ useOnyx calls, memo cache checks) runs on every keystroke. On high-traffic accounts with 40K+ reports on low-end devices, this could contribute to frame drops.


Finding 1: Extra per-keystroke re-render (main concern)

Before: textInputValue was debounced at 500ms via useDebouncedState. SearchAutocompleteList saw 0 immediate re-renders per keystroke — it only re-rendered once after the debounce.

After: textInputValue is immediate (useState). The new inputQueryValue prop changes on every keystroke. Despite React.memo, shallow comparison detects the prop change → re-render on every keystroke + a second re-render after 300ms debounce.

The expensive useMemos (searchOptions, recentReportsOptions, sections) are protected — they depend on autocompleteQueryValue (debounced) or hasEffectiveInputQuery (boolean that only toggles once). So the re-renders are cheap in theory. But the component function body still executes 20+ useOnyx selectors and multiple memo cache checks on each keystroke.

Suggestion: Consider whether inputQueryValue could be passed via a ref or context instead of a prop, to avoid defeating React.memo. Alternatively, split the UI-state–dependent logic (focus reset, section header toggling) into a lightweight wrapper that re-renders per-keystroke while keeping the heavy list component gated behind the debounced value only.

Finding 2: Server search debounce reduced from 2 layers to 1 (low risk)

The internal useDebounce on handleSearch was removed. Now handleSearch fires directly when autocompleteQueryWithoutFilters changes, relying solely on the upstream 300ms debounce in SearchRouter. Before there were two debounce layers; now there is one. In practice this is mitigated by cancelPendingRequests in searchForReports, so API call frequency should be equivalent.

Finding 3: isSearchStringMatch — net positive

Regex pre-compilation + early return false on first mismatch is faster than the old code which continued iterating after failure (if (!matching) continue). The .filter(Boolean) also fixes a correctness issue with empty-string regex matches. PR's perf test on 40K items confirms the improvement.

Finding 4: sections useMemo — performance win

The sections useMemo switched its dependency from autocompleteQueryValue (string, recomputes on every debounce tick) to hasEffectiveInputQuery (boolean, only toggles on empty↔non-empty). Fewer rebuilds of the sections array for high-traffic accounts.


Bottom line: The algorithmic changes are all positive. The only concern is Finding 1 — the per-keystroke re-render of a heavy component. Worth profiling on a low-end Android device with a high-traffic account to confirm whether this causes visible jank.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ad06633a9f

ℹ️ 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".

Comment thread src/components/Search/SearchAutocompleteList.tsx Outdated
@abbasifaizan70

Copy link
Copy Markdown
Contributor Author

@aimane-chnaif Solved the above regression issue pointed out by AI feedback.

@aimane-chnaif

Copy link
Copy Markdown
Contributor

Please merge main

@abbasifaizan70

Copy link
Copy Markdown
Contributor Author

@aimane-chnaif I updated the branch with the latest main.

@aimane-chnaif

Copy link
Copy Markdown
Contributor

@abbasifaizan70 is this expected? There's delay in showing results (only "Ask Concierge" shows instantly)

this branch:

Screen.Recording.2026-05-21.at.5.53.40.PM.mov

main:

Screen.Recording.2026-05-21.at.5.54.20.PM.mov

@abbasifaizan70

Copy link
Copy Markdown
Contributor Author

@aimane-chnaif Yes, that's expected. The "Ask Concierge"/typed query row uses the immediate input value, so it renders instantly. The recent chats/autocomplete list is gated by the 300ms debounce (SEARCH_OPTION_LIST_DEBOUNCE_TIME) — that's the optimization preventing the heavy filtering from running on every keystroke. After the recent fix, we also avoid showing stale rows from the previous query during that window, so for a brief moment, only the header rows are visible.

@aimane-chnaif

Copy link
Copy Markdown
Contributor

Overview

The PR addresses a JS-thread freeze when typing in mobile search (issue #83207). On main, every keystroke synchronously fed SearchAutocompleteList, re-running getSearchOptions()combineOrderingOfReportsAndPersonalDetails()isSearchStringMatch(). The fix:

  • SearchRouter now keeps an immediate textInputValue (plain useState) and a debounced debouncedAutocompleteQueryValue (useDebouncedState, 300ms). The expensive filtering keys off the debounced value; arrow-key / row UI keys off the immediate value.
  • A new optional inputQueryValue prop lets SearchAutocompleteList distinguish "user is mid-type" (isInputAheadOfDebounce) and avoid showing stale results.
  • isSearchStringMatch is rewritten.

The structural debouncing approach is sound and is the right fix. However, there is one regression and a couple of accuracy/quality issues.

🔴 Correctness — likely regression for the Search page header

SearchAutocompleteList.tsx:391-393 removes the useDebounce wrapper around handleSearch:

// before: debounceHandleSearch = useDebounce(..., SEARCH_OPTION_LIST_DEBOUNCE_TIME)
useEffect(() => {
    if (!handleSearch || !autocompleteQueryWithoutFilters) return;
    handleSearch(autocompleteQueryWithoutFilters);
}, [autocompleteQueryWithoutFilters, handleSearch]);

This is safe for SearchRouter (its autocompleteQueryValue is already debounced upstream), but SearchAutocompleteList has two other callersSearchPageInputWide.tsx:99-100 and SearchPageInputNarrow.tsx:81-82. Those get autocompleteQueryValue from useSearchPageInput.tsx:61, a plain useState updated on every keystroke, and handleSearch resolves to searchInServer (SearchPage.tsx:108-117).

Consequences for the full-page search header:

  • searchInServer now fires on every keystroke instead of once per 300ms pause. searchInServer does HttpUtils.cancelPendingRequests + API.read + Onyx.set(RAM_ONLY_IS_SEARCHING_FOR_REPORTS, true) each call.
  • The action even has a comment assuming a debounced caller: "while the API request is debounced we want to show the loading state right away" (Report/index.ts:5542).
  • handleSearchAction in useSearchPageInput is a plain function declaration (not useCallback), so it's a new reference every render. With it in the effect deps, the effect can fire every render, not just per keystroke.

Recommendation: keep the debounce inside SearchAutocompleteList (double-debouncing the router path is harmless — just a slightly longer total delay), or debounce handleSearchAction in useSearchPageInput and wrap it in useCallback. Don't leave the non-router callers undebounced.

🟠 isSearchStringMatch — eager .map() defeats the early-exit

OptionsListUtils/index.ts:513-525:

const compiledRegexes = searchWords.map((word) => ({word, regex: new RegExp(Str.escapeForRegExp(word), 'i')}));
for (const {word, regex} of compiledRegexes) {
    if (!(regex.test(...) || ...)) return false;
}

The .map() compiles all regexes up front, before the loop runs. The old code created each RegExp lazily inside the loop, after the if (!matching) continue guard — so a multi-word query that fails on word 1 (the common case for most items) only ever compiled one regex. The new code always compiles N. For a 3-word query over 40K mostly-non-matching items that's ~120K RegExp constructions vs. ~40K on main — i.e. this can be slower in the dominant case, and the PR description's claim of "compile once per call" is not what the code does.

Compile lazily inside the loop instead — this keeps both the early exit and the empty-word filtering:

for (const word of searchWords) {
    const regex = new RegExp(Str.escapeForRegExp(word), 'i');
    if (!(regex.test(valueToSearch ?? '') || (!isReportChatRoom && participantNames.has(word)))) {
        return false;
    }
}
return true;

(The genuine "compile once, reuse across items" win would require hoisting compilation into the filterAndOrderOptions caller — that's the real optimization the description alludes to, but it's not in this diff.)

The .filter(Boolean) to drop empty words and the new "extra spaces" unit test are good and correct.

🟡 PR description overstates the change

The description lists "useMemo for searchOptions" and "useMemo for recentReportsOptions" as new optimizations. Both were already useMemo on main (searchOptions:198, recentReportsOptions was memoized too) — the diff only edits the existing memos. The real win is that their dependency autocompleteQueryValue is now debounced, so they recompute less often. Worth correcting the description so reviewers/QA aren't misled about scope.

🟡 UX: empty-list flash during the debounce window

SearchAutocompleteList.tsx:373-379: when isInputAheadOfDebounce, recentReportsOptions returns []. This intentionally avoids stale results, but it means the recent-reports section is empty for up to 300ms after each keystroke pause. On a slow device this reads as a brief "no results" flash before results pop in. It's a defensible trade-off, but please verify on a real low-end Android device that it doesn't look like flicker — the skeleton/isLoadingOptions path should ideally cover this gap.

Test coverage

  • New Reassure tests + the multi-space unit test are welcome additions.
  • Gap: every new test and the manual test plan exercises only the SearchRouter (Inbox search icon). Nothing covers SearchPageInputWide/Narrow — exactly the path affected by the debounce removal above. Please add coverage (or at least manual QA steps) for the full-page search header.

Minor

  • inputQueryValue defaulting to autocompleteQueryValue via effectiveInputQueryValue cleanly preserves backward behavior for callers that don't pass it — good.
  • Removing the unused useDebouncedState('', 500) 2nd-tuple value from textInputValue is a legit cleanup (it was dead).
  • onSubmit isInputAheadOfDebounce branch in SearchRouter.tsx:392-403 looks correct, since suggestions are hidden during the debounce window and only searchQueryItems are focusable.

Verdict

Request changes. The debouncing architecture is the right approach and the SearchRouter path looks solid, but the unconditional removal of the handleSearch debounce regresses the Search page header (searchInServer per keystroke), and isSearchStringMatch's eager .map() can be slower than main in the common case. Both are small fixes. Also recommend correcting the PR description and adding Search-page-header coverage before merge.

@aimane-chnaif

Copy link
Copy Markdown
Contributor

@abbasifaizan70 Original issue was reported in search router only (🔍 icon). Any reason for affecting search autocomplete (Spend page) as well?

@abbasifaizan70

Copy link
Copy Markdown
Contributor Author

@aimane-chnaif The Spend page input goes through the same SearchAutocompleteList + isSearchStringMatch code paths under the hood, so the regex pre-compilation in OptionsListUtils benefits both surfaces — that one is pure perf, no behavior change.

The actual behavior change (debounce + stale-result suppression) is opt-in via the new inputQueryValue prop. SearchRouter is the only caller passing it; useSearchPageInput (Spend page) does not pass it, so the Spend page path stays exactly the same as before — just slightly faster.

@MelvinBot

Copy link
Copy Markdown
Contributor

Review

🤖 One likely regression worth confirming before merge, plus a couple of minor notes. The isSearchStringMatch and memoization changes look correct.

🔴 Removing the internal handleSearch debounce un-debounces the server search on the main Search page

The PR replaces the debounced handleSearch with a plain useEffect in SearchAutocompleteList.tsx:393-399:

useEffect(() => {
    if (!handleSearch || !autocompleteQueryWithoutFilters) {
        return;
    }
    handleSearch(autocompleteQueryWithoutFilters);
}, [autocompleteQueryWithoutFilters, handleSearch]);

This relies on the caller passing a debounced autocompleteQueryValue. That's true for SearchRouter (which now passes debouncedAutocompleteQueryValue), but not for the two other callers — SearchPageInputWide/SearchPageInputNarrow — which get autocompleteQueryValue from a plain useState in useSearchPageInput.tsx:61 that updates on every keystroke and do not pass inputQueryValue.

For those callers handleSearch resolves to handleSearchActionsearchInServer (SearchPage.tsx:111-120), which fires the SEARCH_FOR_REPORTS API request. Previously the 300ms useDebounce (SEARCH_OPTION_LIST_DEBOUNCE_TIME) inside SearchAutocompleteList throttled this; with it gone, the main Search page header now sends a server search request on every keystroke.

In-flight requests are cancelled (canCancel: true + HttpUtils.cancelPendingRequests), so this isn't catastrophic, but it's a behavior change that increases request volume and toggles the loading state per character on the primary search experience.

Suggested fix

Either keep the useDebounce wrapper inside SearchAutocompleteList for handleSearch, or debounce autocompleteQueryValue/onSearch in useSearchPageInput the same way SearchRouter now does. Worth confirming the SearchPage (Wide + Narrow) flow still debounces the server search after this change.

Minor notes

  • The new recentReportsOptions returns [] while isInputAheadOfDebounce (SearchAutocompleteList.tsx). This intentionally blanks results for up to 300ms after each keystroke to avoid stale rows — reasonable, but it's a visible UX trade-off (brief empty state vs. stale state) that's worth a QA pass on slower devices.
  • The isSearchStringMatch change correctly preserves behavior: .filter(Boolean) drops empty words that previously matched vacuously (empty-string RegExp always tests true), and the unit test for multi-space queries covers it. 👍

Next Steps: Reply with @MelvinBot <feedback> to refine, or resolve if the debounce concern is already handled elsewhere.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. What shall we delve into next?

ℹ️ 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".

@aimane-chnaif

Copy link
Copy Markdown
Contributor

Please check review comments above. And conflict

@abbasifaizan70

Copy link
Copy Markdown
Contributor Author

@MelvinBot I re-added the local useDebounce around handleSearch in SearchAutocompleteList. The Spend page (useSearchPageInput) doesn't pass inputQueryValue and isn't debounced upstream, so without it searchInServer was firing per keystroke. SearchRouter still works correctly since cancelPendingRequests coalesces the back-to-back debounced calls.

@abbasifaizan70

Copy link
Copy Markdown
Contributor Author

@aimane-chnaif Updated, Can u check again? Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants