Conversation
… shadowing a specific search Co-authored-by: nkdengineer <nkdengineer@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
|
I created this PR from an upstream branch because I don't have push access to your fork. To take ownership of this branch and push updates yourself, run: Then you can close this PR and open a new one from your fork. |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
🤖 Android test steps verification results: The
Worth noting: I had to create a brand new account for this run because the pre-run sign-in replay for melvinbottestuser+34959535449_1_android_0@gmail.com failed on the "Phone or email" field, so nothing here was tested against a pre-existing polluted view run · no recording available |
| // The query can satisfy the current key's default filters and still be a different search entirely, | ||
| // because a generic key's default constrains nothing: every expense-report query "matches" Reports. | ||
| // So when the new query *is* another search's default (or a saved search), switch to that more | ||
| // specific key. Only those exact signals may switch the key here — a last-query match is too coarse |
There was a problem hiding this comment.
@MelvinBot Please remove the em dashes in comment. Split that last sentence in two.
There was a problem hiding this comment.
Done in a603ef3. Dropped the em dash and split the last sentence in two:
specific key. Only those exact signals may switch the key here. A last-query match is too coarse
to distinguish a genuine tab change from the user editing a filter on the current tab.
That was the only em dash in the comments this PR adds.
|
|
||
| if (savedSearchID === searchKeyToSavedSearchID(currentSearchKey)) { | ||
| const query = lastExpensesSearchQuery ?? buildCannedSearchQuery(); | ||
| const query = getValidLastQuery(lastExpensesSearchQuery, buildCannedSearchQuery()); |
There was a problem hiding this comment.
@MelvinBot getValidLastQuery cannot reject a stale same-type query. The expenses default has no filters, so a stored statements query still “matches” and is returned.
There was a problem hiding this comment.
You're right, and I've fixed it in a603ef3.
doesQueryMatchDefaultFilterKeysAndType only asserts that the default's filter keys are a subset of the query's, plus a matching type. buildCannedSearchQuery() has no filters, so [...defaultQueryFilterKeys].every(...) is vacuously true and every type:expense query passes — a stored statements query included.
It also mattered more than "wrong tab for a moment": setCurrentSearchKey sets a pending key, and the provider's pending branch runs before the exact-signal branch, so the self-heal that the rest of this PR adds is skipped on exactly this navigation.
So the hook now rejects a last query that is another suggested search's query, using the same signal the provider uses:
const lastQuery = getValidLastQuery(lastExpensesSearchQuery, defaultQuery);
const lastQuerySimilarSearchHash = buildSearchQueryJSON(lastQuery)?.similarSearchHash;
const isSpecificSuggestedSearchQuery = Object.values(suggestedSearches).some(
(search) => !GENERIC_SEARCH_KEYS.has(search.key) && search.similarSearchHash === lastQuerySimilarSearchHash,
);
const query = isSpecificSuggestedSearchQuery ? defaultQuery : lastQuery;Two things I did not change, so you can push back if you'd rather I did:
- It doesn't check saved searches, only suggested ones. We've just deleted the saved search we were on, and a stored expenses query matching a different saved search's hash felt too thin to justify pulling
savedSearchesinto this hook. - The same weakness exists in the other generic-key callers,
src/components/Navigation/NavigationTabBar/getSearchTabRoute.ts:34andsrc/libs/SearchNavigationUtils.ts:18. I left them alone to keep this PR scoped, but if you want the check hoisted intogetValidLastQueryitself so all three callers get it, say the word.
Checks on the new commit: lint-changed clean, typecheck passed, react-compiler-compliance-check passed on both changed files, spell-changed 0 issues, and the Search suites pass (SearchQueryProvider 32 tests, plus SearchActionsTest/useShareSavedSearchTest/SearchUIUtilsTest/seedMyExpensesSearchTest/useSeedMyExpensesSearch 641 tests).
|
@MelvinBot Please check the review comment. |
|
🤖 Web test steps verification results: The
Two caveats worth your attention: the pre-run onboarding macro had already failed before testing started, so I used a fresh account — nothing here was tested against a pre-existing polluted view run · no recording available |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-09-15.at.16.45.48.movAndroid: mWeb ChromeScreen.Recording.2026-09-15.at.16.45.03.moviOS: HybridAppScreen.Recording.2026-09-15.at.16.47.43.moviOS: mWeb SafariScreen.Recording.2026-09-15.at.16.46.18.movMacOS: Chrome / SafariScreen.Recording.2026-09-15.at.16.43.18.mov |
|
@MelvinBot Please fill the checklist and add test steps. |
…h comment Co-authored-by: nkdengineer <nkdengineer@users.noreply.github.com>
|
Both review comments are addressed in a603ef3, and I replied in each thread. 1. Em dashes in the comment (thread) — removed the em dash and split the last sentence in two. It was the only em dash in the comments this PR adds. 2. Why the objection was correct, and what changed
The consequence was not self-healing either.
Two deliberate limits, both open to push-back:
Checks on the new commit: |
|
Nothing to fix here — all four failures come from a syntax error on
The fix is on How each failing check traces back to itCI builds the merge of this branch into typecheck — fails directly on the file: test (jobs 4, 7, 8) — Babel throws at parse time, so the sidebar never mounts: Every failing suite renders the LHN, and each fails on a missing sidebar element rather than on anything search-related:
This PR's own suite passes.
Once the duplicate is removed from |
|
Still nothing to fix on this branch — the failures come from a build break on
They landed ~7 lines apart, so git auto-merged both with no conflict and nobody saw the combined file. #101195 deletes line 47; both were the same destructure of the same argument-free hook, so the remaining declaration serves every consumer and behavior is unchanged. #101195 CI confirms the diagnosis — every check that fails here passes there:
Once #101195 merges, re-run the checks here and they should go green with no change to this branch. Why each check failed, and why none of it is yoursCI builds the merge of this branch into Each failing check reports that one identifier and nothing else:
Every failing suite renders the LHN and fails on a missing sidebar element, not on anything search-related:
This PR's own suite passes.
One caveat on #101195I could not run the local AI test bundle for #101195 — |
|
Status update — still nothing to fix on this branch, and the fix for #101195 removes the duplicate declaration. Every check that fails here passes there:
That's direct confirmation of the root cause. #101195 only needs a reviewer — its sole red check is One thing changed since my last comment:
Recap of the root cause
CI builds the merge of this branch into Babel throws at parse time, so the LHN never mounts — which is why the three failing test jobs fail on missing sidebar elements rather than on anything search-related:
This PR's own suite is green: |
|
🤖 Done — I filled in I validated the steps on dev NewDot web before writing them, so they are the steps I actually ran rather than steps from reading the diff. Steps 1-9 passed: with Reports pre-selected, the Home "Approve 2 reports" to-do lands on Needs approval with that tab selected; applying a Title filter keeps the tab; reopening a saved search from the Reports tab selects it; deleting it while selected lands on the Expenses default list; and the approve query pasted directly as a URL selects Needs approval. Android steps 1-6 were covered in my earlier run on this PR. Seven boxes I left unchecked for you — each asserts something I can't do or didn't do:
I also unchecked the UI-modification block: this PR touches no UI, but its sub-item asserts you added the One unrelated thing worth a separate issue: at desktop width, the Filters flyout for Needs approval renders a fixed-height, non-scrollable left category list that clips everything below "Exported to", so "Title" is unreachable. I had to shrink the viewport to 500×900 to get the full-page Filters list. Not caused by this PR. view run · no recording available |
|
@MelvinBot Please mark PR as ready |
|
@nkdengineer 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] |
|
Done — this PR is out of draft and marked ready for review. Two things to be aware of before a reviewer picks it up. The red checks are not from this PR. ESLint, typecheck and Jest jobs 4/7/8 are all failing, but none of the failures touch the four files in this diff:
Once that redeclare is fixed on The view run · no recording available |
![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: a603ef3717
ℹ️ 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".
| const exactSearchKey = getExactSearchKeyForQuery(currentSearchQueryJSON); | ||
| if (exactSearchKey && exactSearchKey !== currentSearchKey) { | ||
| setCurrentSearchKey(exactSearchKey); |
There was a problem hiding this comment.
When two suggested searches share a similarSearchHash, this new branch can replace the current key after an otherwise valid filter-value or sort edit. For example, Statements precedes Unapproved Card in suggestedSearches, and their defaults collide when the card feed is empty; changing an ignored value such as the feed while on Unapproved Card changes currentSearchHash but preserves the similar hash, so getExactSearchKeyForQuery() returns Statements and the selected tab flips incorrectly. This branch needs a genuinely exact default-query signal, or it must preserve the current matching key when the similar hash is ambiguous.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, and fixed in 0534401.
You're right that similarSearchHash is not a genuinely exact signal, and that the ambiguity is resolved by declaration order. I confirmed the collision and the flip:
statementsandunapprovedCardcollide onsimilarSearchHash1141398826when the card feed is empty, andstatementsis declared first (src/libs/SearchUIUtils.ts:930vs:978). It is the only colliding pair across all suggested searches under realistic inputs.- Sorting a column or picking a feed while on Unapproved card changes
hashbut notsimilarSearchHash,doesQueryMatchDefaultFilterKeysAndTypepasses, and the newelsebranch sets the key tostatements. Reproduced by rendering the real provider. - It is a regression from this PR: the same two edits against
main's provider keepunapprovedCard, becausemainnever re-resolves once the default filter keys still match.
Your option "preserve the current matching key when the similar hash is ambiguous" is the one I took. Requiring an exact query hash instead would defeat the PR's purpose, because feed is a deliberately ignored filter, so statements on any non-default feed has a different hash but the same similarSearchHash. An exact-hash test would refuse to switch to Card statements whenever a link carries a feed other than the user's default.
getExactSearchKeyForQuery now takes the key we are already on and prefers it when it is among the matches. It can only ever prevent a switch, so every intended switch is preserved. The same tie-break is applied to duplicate saved searches. It has to be a parameter rather than a read of currentSearchKey, because the function also runs in the useState initializer before currentSearchKey is declared.
Two regression tests added. The first fails against the pre-fix provider (returns statements), the second pins the unchanged first-match fallback when the current key is not one of the matches.
Checks on 0534401
SearchQueryProvider.test.tsx34/34, including the 32 pre-existing tests- All Search suites: 96 suites, 2,091 tests, 0 failures
typecheckpassed,lint-changedexit 0,spell-changed0 issuesreact-compiler-compliance-checkincheckmode:babel=compiled, oxc=compiled. Thecheck-changedmode could not run in this environment becauseGITHUB_BASE_REFis empty on aworkflow_dispatchrun andscripts/utils/Git.ts:431uses??, which does not catch''. CI will run the real one.
Reachability note for anyone reading this later: the empty-feed state that creates the collision also normally hides both tabs, so this is hard to hit in the product today. The exception is a feed whose display name starts with vcf/cdf/gl1025, because src/hooks/useCardFeedsForDisplay.ts:58 calls isCustomFeed(feed.name) where it looks like it means feed.feed. I did not change that here.
|
Still nothing to fix on this branch — and the fix is already written and waiting. It just needs a human to take it out of draft. The fix PR: Expensify/App#101195 removes the duplicate line. Its only failing check is Why this PR is red: CI builds the merge of this branch into
How one line produces all five failures
Line 40 is the one to keep — it already provides the binding used at lines 118, 127, and 148. That is exactly what Expensify/App#101195 does, deleting only line 47. Babel throws at parse time: So This PR's own suite passes. Once Expensify/App#101195 merges, these checks should go green here with no change to this branch. I have deliberately not pushed the same one-line deletion onto this branch — it would put an unrelated LHN fix in a Search PR and conflict with |
|
@MelvinBot please check the codex review |
|
@MelvinBot The fix lint PR was merged please update. |
| import Navigation from '@libs/Navigation/Navigation'; | ||
| import {buildCannedSearchQuery} from '@libs/SearchQueryUtils'; | ||
| import {searchKeyToSavedSearchID} from '@libs/SearchUIUtils'; | ||
| import {buildCannedSearchQuery, buildSearchQueryJSON, getValidLastQuery} from '@libs/SearchQueryUtils'; |
There was a problem hiding this comment.
@MelvinBot let revert this change since we're focusing the main problem on suggested search
There was a problem hiding this comment.
Reverted in c6142c4. src/hooks/useDeleteSavedSearch.tsx is now byte-identical to main — git diff origin/main -- src/hooks/useDeleteSavedSearch.tsx is empty — so the import on this line and the whole getValidLastQuery / similarSearchHash block are gone, back to src/hooks/useDeleteSavedSearch.tsx:42:
const query = lastExpensesSearchQuery ?? buildCannedSearchQuery();The PR now touches only SearchQueryProvider.tsx, SearchUIUtils.ts, and the provider test. I kept the GENERIC_SEARCH_KEYS export in SearchUIUtils.ts because the provider uses it at SearchQueryProvider.tsx:117, so nothing is orphaned.
One thing to be aware of, since it re-opens what you flagged in your earlier comment on this file: the provider does not self-heal this path. setCurrentSearchKey(EXPENSES, query) sets a pending key, and the pending branch at SearchQueryProvider.tsx:161-164 commits it and returns before either the doesQueryMatchDefaultFilterKeysAndType re-resolve or the new exact-signal branch runs. So if lastExpensesSearchQuery holds a query belonging to another suggested search, deleting the saved search you are on still selects the Expenses tab while showing that other search. That is the same behavior as main, not a regression from this PR — just not covered by it either. Happy to file it separately if you want it tracked.
Checks on c6142c4
lint-changedclean,typecheckpassed,spell-changed0 issuesreact-compiler-compliance-check -- checkon the reverted hook and the provider: no findings- Search suites: 96 suites, 2,091 tests, 0 failures.
SearchQueryProvider.test.tsx34/34 - Full Jest suite via
npm test -- --silent --shard=N/8for N=1..8: 1,444 suites, ~25,200 tests, 0 failures
npm run lint (full repo) still exceeds the time/heap available here; lint-changed covers the changed files.
I left the PR description alone — its third "This PR" bullet now describes a change that is no longer in the diff, but sed isn't available in this environment and rewriting the whole body by hand would risk clobbering the checklist items you have already ticked. Worth dropping that bullet when you next edit it.
…ne search Two suggested searches can share a similarSearchHash, so the re-resolve on a query change could hand an ambiguous match to whichever search is declared first. Pass the key we are already on and prefer it when it is among the matches, so editing a filter never flips the tab. Co-authored-by: nkdengineer <nkdengineer@users.noreply.github.com>
|
@nkdengineer 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] |
|
Codex's one finding is valid, and it's a regression this PR introduced. Fixed in 0534401, with a reply on the thread. What it got right: Fix: Codex's first option. What it missed: the empty-feed state that creates the collision also normally hides both tabs, so this is hard to hit in the product today. It's still worth fixing since the change is strictly narrowing. Reachability, the latent hole, and checksThe one real way in today is a feed whose display name starts with Also pre-existing and not changed here: Two regression tests added. The first fails against the pre-fix provider, the second pins the unchanged first-match fallback. Checks on 0534401: Codex made no other findings on commit view run · no recording available |
…OnGenericTab Co-authored-by: nkdengineer <nkdengineer@users.noreply.github.com>
|
Fixed — all checks pass now. I merged the latest Why the checks were red:
All 8 Evidence and verificationThe two failure signatures, both pointing at a file outside this PR's diff: Babel threw at parse time, so the LHN never mounted. That is why the failing suites were all sidebar renders ( The merge is a clean, content-free merge. It resolved with no conflicts, and I verified the four files this PR owns are byte-identical before and after it:
While I was working, On local AI tests: this environment requires interactive approval to invoke |
|
@MelvinBot Bump on this #101184 (comment) |
…ested search Co-authored-by: nkdengineer <nkdengineer@users.noreply.github.com>
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
@MelvinBot Let verify your test case on web again. |
|
🤖 Web test steps verification results: On dev NewDot web (branch
Two things worth knowing: the pre-run sign-in for melvinbottestuser+34990563274_1_web_0@gmail.com timed out waiting for Home, so the session was recovered by hand on a fresh fixture — meaning the self-heal path for accounts with already-polluted stored filters is still unverified on both platforms. And reload persistence remains the one case neither platform could prove; a Jest test asserting the resolved key after a re-mount with the same query params would cover it more reliably than device testing. view run · no recording available |
nkdengineer
left a comment
nkdengineer
left a comment
There was a problem hiding this comment.
Looks good to me and test well.
@mountiny All yours.
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #101173 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
JS00001
left a comment
JS00001
left a comment
There was a problem hiding this comment.
Looks like this breaks always staying on the search that the user is currently on 🤔
Screen.Recording.2026-09-15.at.3.25.11.PM.mov
|
@JS00001 Is it expected since we reset the filter? |
|
Closing this in favour of Set the Search key when a Home to-do opens a canned Spend search, per
The two pre-existing issues found while verifying this one are unfixed and still worth filing separately: |
4 participants
Explanation of Change
The Search page keeps its selected tab in
currentSearchKey, which is app-lifetime state inSearchQueryProvider. Two things let a stale tab survive a navigation to a different search:getSearchKeyForQuerymatched a search either by its own default query or by the query it was last used with, in a single pass. The genericexpenses/reportsentries are declared first and every Search run overwrites their stored last query, so a generic key could match — and win — for a query that is exactly another search's default.Together, an approver who had visited Spend > Reports and then opened the Home "Needs approval" to-do landed on the approve query with the Reports tab still selected. The wrong key is then persisted into
nvp_searchFilters, so it stuck across reloads.This PR:
similarSearchHashcompares only the filter keys of most filters, somerchant:Amazonandmerchant:Zuluhash identically. Matching on a last-used query here would flip the tab when the user merely edits a filter on the current tab.useDeleteSavedSearchthroughgetValidLastQuery, like every other generic-key caller, so it cannot carry a stale query forward.Deliberately not in this PR:
navigateToCannedSpendSearch. With the two changes above the provider self-heals, so this is cleanup rather than a fix, and it would also make the to-do open the user's last approve-tab query instead of the full list of reports awaiting approval — a behavior change worth deciding on separately.statementsandunapprovedCardshare asimilarSearchHashwhen the card feed is empty (both1141398826, verified in Jest), andgetQueryHashesseeds aSetwith a string rather than[string], making part of the hash a character bag. Both are worth separate issues.AI Tests
Run locally against this branch, all passing:
npm run lint-changed(exit 0)npm run typechecknpm run react-compiler-compliance-check -- checkon both changed components —babel=compiled, oxc=compilednpm run spell-changed— 0 issuesnpm test -- --silent --shard=N/8for N=1..8 — 1,444 suites, ~25,000 tests, 0 failuresNot run:
npm run lint(full repo) andnpx eslinton the changed files both exceed the available time/heap in this environment;npm run lint-changedcovers the changed files and passed.npm run prettieris not a script in this repo and App tracks no Prettier config, so that matrix entry does not apply.Fixed Issues
$ #101173
PROPOSAL: #101173 (comment)
Tests
Setup: sign in as an approver who has more than one report awaiting their approval. If the account has none, create a workspace, enable Workflows with approvals on and yourself as the approver, then submit two expense reports.
similarSearchHashcompares only filter keys, so editing a filter must not be mistaken for a tab change./search?q=type:expense-report action:approve to:<yourAccountID>) directly into the address bar. Verify Needs approval is selected.Offline tests
QA Steps
Same as Tests steps 1-6 and 10.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.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
Tests steps 1-6 and 10 run on standalone Android NewDot.
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Tests steps 1-9 run on dev NewDot web.