Fix/search saved tab native highlight#90138
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: febd1229e0
ℹ️ 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 isSavedSearchActive = hash !== undefined && !!savedSearches && Object.keys(savedSearches).some((key) => Number(key) === hash); | ||
|
|
||
| if (isSavedSearchActive) { | ||
| return -1; |
There was a problem hiding this comment.
Ignore pending-delete saved searches when determining active tab
getActiveItemIndex() now returns -1 whenever the current hash exists in savedSearches, but this does not exclude entries with pendingAction: DELETE while online. In narrow search, those pending-delete saved searches are filtered out of the rendered tabs, so this branch leaves activeItemKey empty and no tab is selected; previously the built-in tab matching similarSearchHash stayed highlighted. This regression appears when opening a query whose saved search is pending deletion while online.
Useful? React with 👍 / 👎.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-07-10.at.4.30.26.PM.movAndroid: mWeb ChromeScreen.Recording.2026-07-10.at.4.34.13.PM.moviOS: HybridAppScreen.Recording.2026-07-10.at.4.39.25.PM.moviOS: mWeb SafariScreen.Recording.2026-07-10.at.4.37.19.PM.movMacOS: Chrome / SafariScreen.Recording.2026-07-10.at.4.24.51.PM.mov |
|
@eh2077 friendly bump. thanks |
|
Bug: can't go back to saved search if back from inbox
Screen.Recording.2026-05-15.at.10.18.04.PM.movScreen.Recording.2026-05-15.at.10.19.10.PM.mov |
eh2077
left a comment
There was a problem hiding this comment.
Please refer original steps for QA Steps.
| const flattenedTypeMenuItems = useMemo(() => typeMenuSections.flatMap((section) => section.menuItems), [typeMenuSections]); | ||
| const activeItemKey = useMemo(() => flattenedTypeMenuItems.at(activeItemIndex)?.key, [flattenedTypeMenuItems, activeItemIndex]); |
There was a problem hiding this comment.
I don't think we need useMemo anymore as we already have React Compiler CI.
|
@neerajbachani friendly bump! |
|
@eh2077 Thanks for the bump, sorry for the delay. Removed the extra |
|
@eh2077 friendly bump, thanks |
|
thanks for the update! I'll recheck shortly |
|
@eh2077 friendly bump |
|
looking into it |
|
confirmed #90138 (comment) was fixed Screen.Recording.2026-06-04.at.9.42.44.PM.mov |
|
@AndrewGable @flaviadefaria friendly bump, thx |
|
@neerajbachani please take care of the conflicts, thx! |
|
@neerajbachani friendly bump! |
|
Since this PR has been open for a while, I re-tested the original flow on production and the saved search tab now highlights correctly on native (and narrow web) without our changes merged. From what I can tell, the interactive narrow path was likely fixed on main (SearchTypeMenuNarrow now uses useSearchTypeMenuSections / activeTypeMenuKey, same SSOT approach as wide layout). Could you please take a look and let me know if you think anything is still worth covering here? A few gaps I noticed on current main that this PR still addresses:
Happy to close as superseded, or rebase/trim the PR to only the remaining items if you think those are still needed. Thanks! |
@eh2077 friendly bump, could you please guide me to proceed from here? |
|
Sorry for late response. Yes, let's proceed with polishing these flaws and update QA test accordingly, thx! |
|
@neerajbachani friendly bump |
e7db793 to
1124418
Compare
|
@eh2077 Rebased onto latest main, trimmed to pending-delete and done with static menu and tests. Ready for review!! |
|
checking |
|
@eh2077 friendly bump |
|
@neerajbachani Can you update test steps? Btw, I don't think it's necessary to mention unit test in QA steps. |
|
@eh2077 Updated Tests / Offline / QA steps in the PR description. QA steps are manual only (no unit test mention). Thanks! |
eh2077
left a comment
There was a problem hiding this comment.
works well, checklist #90138 (comment)
|
@AndrewGable @flaviadefaria gentle bump, thanks |
Explanation of Change
The main interactive narrow saved-search highlight path was fixed on main separately (#89112 via
useSearchTypeMenuSections/activeTypeMenuKey). This PR polishes the remaining gaps:hashas an active saved search, so no tab was highlighted. We now ignore pending-delete entries (while online) inuseSearchTypeMenuSectionsso the matching built-in tab (e.g. Expenses) highlights instead.StaticSearchTypeMenuonly knew about built-in tabs andsimilarSearchHash, so during the static loading phase a built-in tab could flash as active for a saved search. It now includes the active saved search (when not pending-delete online) and highlights it.SearchTypeMenuSavedSearchHighlightTestfor static + interactive narrow menus, including the pending-delete case.Fixed Issues
$ #87769
PROPOSAL: #87769 (comment)
Tests
similarSearchHash(e.g. expense search with a custom sort). Confirm the saved search tab is highlighted (not Expenses/Reports).npx jest --runTestsByPath tests/ui/components/SearchTypeMenuSavedSearchHighlightTest.tsx --watchman=false --runInBandand confirm all tests pass.Offline tests
QA Steps
PR 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
lv_0_20260511213849.mp4
Android: mWeb Chrome
iOS: Native
lv_0_20260511220701.mp4
iOS: mWeb Safari
MacOS: Chrome / Safari