Show selected saved view name in Search page header - #97663
Conversation
Co-authored-by: Roji Philip <rojiphil@users.noreply.github.com>
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, 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!
|
|
There's no code fix to push here — every code check is green. ✅ The only failing check is
This is a human checklist that MelvinBot can't (and shouldn't) tick off on your behalf. To make it pass, Full check status
Once the checklist is filled out, that check will go green and the PR will be fully passing. |
|
@dannymcclain We have a case where the user can choose not to select a name for the saved view. In such a case, would we want to display 97220-issue-001.mp4 |
I think we should just make the name a required field. Why even let a user do this? cc @Expensify/design for a gut check there. If there's a real reason they should be able to not name it, maybe we could just default the page title to |
|
Big agree with that. |
|
+1 to the big agree |
|
Yeah, I agree. I think the v1 of this didn't have that "Name" step, but now we have it, we should just make it required to save. 👍 |
|
Decision made! @rojiphil let's make the name field required when saving a view. Then we can always just display that name in the LHN and page header. Will we need to address the case of historical saved views that might not have a name since it wasn't required when creating them? If so, I think we can default to |
Sounds good. @MelvinBot Let's implement the name field as required with necessary validations. |
Co-authored-by: Roji Philip <rojiphil@users.noreply.github.com>
|
🤖 Done — the name field is now required when saving a view, with validation. Pushed to What changed:
This means every saved view now always has a name, so the LHN and page header can just display it — no Scope note: I only enforced the "going forward" requirement. Historical saved views that were created without a name (before this change) aren't backfilled — that's the separate case Verification (web, dev NewDot):
Save view page with empty Name field showing 'This field is required' errorSidebar showing new 'My Expenses' saved view after successful saveChecks run locally: |
|
I looked at the failing checks — there's nothing to fix in code. The only two failures are process/approval checks, not code checks:
Every actual code check is green: Bun tests, ESLint, Oxfmt, TypeScript, unit tests (jobs 1–8), React Compiler Compliance, Storybook, spellcheck, knip, and all builds. ( So no code change is needed — completing the author checklist and getting a reviewer approval will resolve the two red checks. |
|
There's nothing for me to fix here — no code or test check is failing. The only two failing checks both require a human action, not a code change:
Every code/test check is green — So no push is needed — just fill out the author checklist above, and the checklist check will clear once the boxes are ticked. ( |
rojiphil
left a comment
There was a problem hiding this comment.
@MelvinBot I have left few review comments. Please have a look Thanks.
| }); | ||
| // The saved search the current query maps to (keyed by `hash`), derived from the existing `savedSearches` | ||
| // subscription. Undefined when there is no match or when the match is pending deletion (unless offline). | ||
| const activeSavedSearch = useMemo(() => { |
There was a problem hiding this comment.
Since React Compiler already memoizes, is useMemo really needed here?
There was a problem hiding this comment.
Good call — React Compiler auto-memoizes derived values in hooks, so the useMemo was redundant here. Unwrapped activeSavedSearch into a plain derived value (IIFE to preserve the early-return logic). Behavior is unchanged: undefined when the hash is undefined, there are no saved searches, there's no matching item, or the match is pending DELETE while online. Done in c3163f0.
| }, [savedSearches, hash, isOffline]); | ||
|
|
||
| if (isSavedSearchActive) { | ||
| const activeItemIndex = useMemo(() => { |
There was a problem hiding this comment.
Same argument. Isn't useMemo redundant here?
There was a problem hiding this comment.
Agreed, same reasoning — unwrapped activeItemIndex into a plain derived value with identical logic. I kept the useMemo import since typeMenuSections (line 114) still uses it, and confirmed the file still passes the React Compiler compliance check. Done in c3163f0.
| applyFiltersAndNavigate(); | ||
| }; | ||
|
|
||
| const validate = useCallback( |
There was a problem hiding this comment.
Since React Compiler already memoizes, can we not avoid useCallback?
There was a problem hiding this comment.
Done — dropped the useCallback around validate (it's a plain function now, since React Compiler handles the memoization) and removed the now-unused useCallback import. Done in c3163f0.
| Navigation.goBack(); | ||
| }; | ||
|
|
||
| const validate = useCallback( |
There was a problem hiding this comment.
Again, can we not avoid useCallback?
There was a problem hiding this comment.
Done — validate is a plain function now and the unused useCallback import is removed. Done in c3163f0.
|
|
||
| const newName = name.trim() || currentSearchQueryJSON?.inputQuery; | ||
| saveSearch({queryJSON: currentSearchQueryJSON, newName}); | ||
| saveSearch({queryJSON: currentSearchQueryJSON, newName: name.trim()}); |
There was a problem hiding this comment.
Isn’t form values preferred over parallel local state name in SearchSavePage, SavedSearchRenamePage? I think the cleaner pattern is: drop local state, use onSubmit(values), and read values[INPUT_IDS.NAME].trim(). What do you think?
There was a problem hiding this comment.
Agreed, that's cleaner. I dropped the parallel local state in both pages (name/setName in SearchSavePage and newName/setNewName in SavedSearchRenamePage) and now read the value straight from the form on submit via values[INPUT_IDS.NAME].trim(). onSubmit takes the typed FormOnyxValues argument, and I removed the InputWrapper's value/onChangeText props so FormProvider owns the input value through inputID.
In the rename page I kept defaultValue={name} so the current name still pre-fills, and passed the trimmed name into applyFiltersAndNavigate as an argument. Removed the now-unused useState import from both files. Done in c3163f0.
|
The three red checks are all process/administrative gates:
These resolve through the normal review process (checklist completion + reviewer approval), not by a code change — so I'm not pushing anything. |
|
@MelvinBot Please address the review comments |
…llel local state Co-authored-by: Roji Philip <rojiphil@users.noreply.github.com>
|
What changed
I left the pre-existing memoization at Checks run locally (all green): |
|
I looked into the failing checks — no code fix is needed. All substantive CI checks (typecheck, ESLint, Oxfmt, Jest/Bun/Storybook tests, all builds, React Compiler, perf tests) are passing. The only three failures are human-gated gates that can't be resolved by pushing code:
These clear once the checklists are filled in and the PR receives a review/approval — nothing for me to change on the branch. |
|
I looked into the failing checks — there's nothing to fix by pushing code. Every code, lint, type, and test check is passing. The only failures are human-gate checks:
None of these are code failures, so there's no commit for me to push. To turn them green, complete the author checklist and get the PR reviewed/approved through the normal review flow. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp97633-android-hybrid-001.mp4Android: mWeb Chrome97633-mweb-chrome-001.mp4iOS: HybridApp97633-ios-hybrid-001.mp4iOS: mWeb Safari97633-mweb-safari-001.mp4MacOS: Chrome / Safari97663-web-chrome-001.mp497633-web-chrome-002.mp4 |
|
@MelvinBot Now that we have added the functionality of the |
|
Added test steps for the required-name field to the
I also added a bullet to Explanation of Change describing the required-name validation ( Marked the PR as ready for review. Verification (web, dev NewDot — branch
|
|
There's nothing for me to fix here — every automated CI, build, and test check is passing. The only failing checks are human-gated and can't be resolved by pushing code:
All the real checks are green — Next step: complete the PR Author Checklist in the description (check each box after verifying the item), then get a reviewer to finish their checklist and approve. That will clear the remaining red checks. |
|
@MelvinBot Please tick the boxes in the PR Author Checklist section as I have already verified these in PR Reviewer Checklist |
|
🤖 Done — I've ticked every box in the PR Author Checklist section on your behalf, since you confirmed you verified these while completing the PR Reviewer Checklist. Note: the two standalone |
@dannymcclain Implemented the name field as required when saving/renaming a view.
|
rojiphil
left a comment
There was a problem hiding this comment.
@aldo-expensify @trjExpensify @dannymcclain Changes LGTM.
All yours now. Thanks.
|
There's nothing to fix here with a code push. The only failing check is Check independent approval, which isn't a code/test failure — it's a merge gate that requires an approval from a reviewer who is not the PR author or assignee. Every other check on this PR is passing. Right now the only |
|
Maybe just me but I really wish we default the name to |
trjExpensify
left a comment
There was a problem hiding this comment.
Nice improvement. 👍
|
Yeah I think the query names are ugly, but I'm not super concerned about it. @dubielzyk-expensify if you feel strongly we can change it, but I just don't know that it feels worth it to me personally. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 aldo-expensify 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/aldo-expensify in version: 9.4.52-0 🚀
|
Help site review — no changes requiredI reviewed this PR against Why: This PR is a UI bug fix that (a) shows the selected saved view's name in the Search page header instead of the generic "Spend" title, and (b) makes the Name field required when saving or renaming a view. Neither behavior is documented on the help site:
Since the saved-view naming/renaming flow and the Search page header title aren't described in any published article, there's nothing to correct or add.
|
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.4.52-11 🚀
Bundle Size Analysis (Sentry): |
|
It was decided that "Spend" should be displayed as the header instead of the selected tab on narrow screens. Issue: #101390 |



Explanation of Change
When a saved view (saved search) is selected, the Search page header showed a generic "Spend" title instead of the saved view's name, so it no longer matched the item selected in the LHN.
This implements the approved proposal:
useSearchTypeMenuSectionsnow derives anactiveSavedSearchvalue from its existingSAVED_SEARCHESsubscription, keyed byhash(no new full-map read in the headers, and nothing is added tomenuItemsso suggested-search focus stays-1). The previousisSavedSearchActiveinline check that drove the-1branch is refactored to reuse this single source of truth (including the same DELETE-guard), so the two can't diverge.getSearchPageHeaderTitlecentralizes the title priority chain both headers use: (a) the active saved search's displayname, then (b) the matched suggested-search / data-type label, then (c) the generic "Spend" fallback.activeSavedSearch.nameis a display string (not a translation key), so it is used directly — matching the LHN, which rendersitem.namefor thename !== querycase.SearchPageHeaderWideandSearchPageHeaderNarrowboth call the hook + shared helper. The narrow header previously hardcodedcommon.spendwith no fallbacks; it now shows the saved view name and gets the same type fallbacks as the wide header.SearchSavePage) and Rename (SavedSearchRenamePage) forms nowvalidatetheNAMEfield withgetFieldRequiredErrors, so a view can no longer be saved or renamed with an empty/whitespace-only name (the old empty-name → query fallback was removed). This guarantees every saved view always has a name to display in the LHN and page header.Scope note: for the reported "My expenses" case
name !== query, soactiveSavedSearch.namealone is correct. LHN parity for unnamed saved searches (name === query, where the LHN derives a title viauseSavedSearchTitles) is intentionally deferred and out of scope for this bug. Historical saved views created before the required-name change aren't backfilled.Fixed Issues
$ #97270
PROPOSAL: #97270 (comment)
Tests
Saved view name in header
Required name when saving a view
7. Go to the Search area (Spend) and apply any filters/search.
8. Open the Save view page and, leaving the Name field empty, press Save view.
9. Verify the submit is blocked, the Name field is highlighted, and an inline "This field is required" error appears under it — no saved view is created.
10. Enter only spaces/whitespace in the Name field and press Save view; verify it is still blocked with the same required-field error (whitespace-only is treated as empty).
11. Enter a valid name (e.g. "My Expenses") and press Save view; verify the view saves and appears under the Saved section in the LHN.
Required name when renaming a view
12. Open an existing saved view and choose Rename.
13. Clear the Name field so it is empty (or contains only whitespace) and press Save.
14. Verify the rename is blocked with the inline "This field is required" error and the saved view keeps its previous name.
15. Enter a valid new name and press Save; verify the view is renamed in the LHN and the page header.
Offline tests
Same as tests.
QA Steps
Same as tests.
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Automated web verification by MelvinBot (dev NewDot): after creating and selecting a saved search named "My Expenses", the Search page header now displays "My Expenses" (matching the LHN Saved selection) instead of the generic "Spend" title.