feat: seed default My expenses saved search for dual-role users and remove obsolete rename tooltip - #93541
Conversation
…emove obsolete rename tooltip
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
…acking plumbing - Replace isGroupPolicy with isPolicyUser for submitter check so admins are not incorrectly counted as submitters in seedMyExpensesSearch eligibility logic - Remove areAllSectionsExpanded prop from SavedSearchList and the entire collapsedSectionCount/onCollapsed chain from SearchTypeMenuWide, which existed solely to gate the now-removed RENAME_SAVED_SEARCH tooltip - Remove unused buildCannedSearchQuery import from Search.ts - Eliminate redundant saveSearchName variable in seedMyExpensesSearch - Fix OnyxUpdate missing type argument on optimistic/failure/success arrays - Remove stray blank line in CONST after RENAME_SAVED_SEARCH removal
- Extract eligibility logic from SavedSearchList useEffect into
isDualRoleUser(policies, email) in PolicyUtils so it is testable
and reusable
- Remove savedSearches from useEffect dep array — it was only used as
a load guard but caused an infinite retry loop when failureData nulled
the hash key and reset the NVP, re-triggering the effect
- Replace the load guard with allPolicies === undefined which is the
only collection needed for eligibility
- Add 11 unit tests in seedMyExpensesSearchTest.ts covering:
- isDualRoleUser: submit+approve, submit-only, approve-only, admin
role, optional approval mode, null/empty inputs
- seedMyExpensesSearch: Onyx write, query format (type/from), NVP gate
- Reformat DismissedProductTraining.ts (Prettier: double→single quotes, 4-space indent) - Replace removed RENAME_SAVED_SEARCH tooltip with GPS_TOOLTIP in ProductTrainingContextProvider tests - Remove the wide-layout-specific test case since RENAME_SAVED_SEARCH was the only wide-layout-only tooltip
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
- Remove unused eslint-disable directive on no-restricted-imports in SavedSearchList.tsx - Fix naming-convention violations in test: snake_case policy IDs → camelCase, email key → PEER_EMAIL constant - Fix no-unsafe-type-assertion in test factory functions with targeted eslint-disable - Fix prefer-at violations: [0] → .at(0)
… when savedSearches is empty SavedSearchList only mounts when savedSearches is non-empty, so the seeding effect never fired after a user deleted their saved searches and the NVP was reset. Moving the effect to SearchTypeMenuWide, which always mounts when the Search left panel is visible, ensures seeding runs regardless of whether any saved searches exist.
|
@parasharrajat 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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![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: 29ba6de554
ℹ️ 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".
| }, | ||
| ]; | ||
|
|
||
| API.write(WRITE_COMMANDS.SAVE_SEARCH, {jsonQuery, newName: searchName}, {optimisticData, failureData, successData}); |
There was a problem hiding this comment.
Because the only request sent here is SaveSearch with jsonQuery/newName, nvp_hasSeededMyExpensesSearch is never written to the server; the SET is only an optimistic Onyx update. In any fresh Onyx state, full reconnect, or other device after the user deletes the seeded search, OpenApp will not return this flag, so the SearchPage effect can recreate the deleted search. Persist the flag with the NVP write path or have the backend return it with the save.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The flag intentionally lives only as an optimistic Onyx update for now . Persisting it server-side would require a backend change
There was a problem hiding this comment.
we should request backend change.
…rch calls The Onyx NVP guard (hasSeededMyExpensesSearch) is set optimistically and asynchronously, so the effect could re-run before it propagates back, producing duplicate SAVE_SEARCH API calls. Add a useRef guard that is checked and set synchronously in the same tick.
If the user already has a saved search with the same query hash (e.g. under a custom name), seedMyExpensesSearch would overwrite their name with "My expenses". Now bail early if the hash is already present in savedSearches, preserving any existing custom name.
Moves the My Expenses seeding concern out of SearchPage into a focused custom hook. Also narrows the policy subscription to a boolean selector so SearchPage only re-renders when the dual-role status changes, not on every policy field update.
Covers: seeds for dual-role user, skips for non-dual-role, skips when NVP already true, skips when accountID not yet loaded, seeds only once on re-render.
| if (!policy) { | ||
| continue; | ||
| } | ||
| isSubmitter = isSubmitter || isPolicyUser(policy, currentUserEmail); |
There was a problem hiding this comment.
Should this be any member of a policy not just user.
There was a problem hiding this comment.
Agreed. isPolicyUser matched only members whose role is literally 'user', which excluded admins or owners who also submit. Switched the submitter check to isGroupPolicy, matching the role-agnostic Submit-suggestion gate.
| } | ||
| isSubmitter = isSubmitter || isPolicyUser(policy, currentUserEmail); | ||
| if (!isApprover) { | ||
| const hasApprovalFlow = isPaidGroupPolicy(policy) && !!policy?.approvalMode && policy.approvalMode !== CONST.POLICY.APPROVAL_MODE.OPTIONAL; |
There was a problem hiding this comment.
why are we checking only for paid group policy?
There was a problem hiding this comment.
Good catch. Switched the check to isGroupPolicy so it lines up with the Approve surface.
| isSubmitter = isSubmitter || isPolicyUser(policy, currentUserEmail); | ||
| if (!isApprover) { | ||
| const hasApprovalFlow = isPaidGroupPolicy(policy) && !!policy?.approvalMode && policy.approvalMode !== CONST.POLICY.APPROVAL_MODE.OPTIONAL; | ||
| const isSubmittedTo = Object.values(policy.employeeList ?? {}).some((employee) => employee.submitsTo === currentUserEmail || employee.forwardsTo === currentUserEmail); |
There was a problem hiding this comment.
Why are we checking isSubmittedTo?
Issue say user who submits and approves.
There was a problem hiding this comment.
isSubmittedTo was the "approves" signal. A user others submit/forward reports to is a de-facto approver. But it's redundant here: isPolicyApprover already covers submitsTo/forwardsTo (plus overLimitForwardsTo) internally, and the inline check even omitted overLimitForwardsTo. Removed it in and now relys on isPolicyApprover alone which is simpler and slightly more correct.
| return policy?.reimbursementChoice !== CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_NO; | ||
| } | ||
|
|
||
| /** Returns true when the user is both a submitter (role "user") in at least one policy and an approver on at least one paid policy with a non-optional approval flow. */ |
There was a problem hiding this comment.
Why specifically have you taken this criteria?
There was a problem hiding this comment.
The criteria mirror the existing suggested-search eligibility in getSuggestedSearchesVisibility (SearchUIUtils.ts), per the approved proposal: submitter ↔ the Submit gate (isGroupPolicy), approver ↔ the Approve gate (isEligibleForApproveSuggestion).
|
I logged in with this account, which has a workspace where he is an approver, but I don't see the search. 22.07.2026_16.25.28_REC.mp4 |
|
@aswin-s Check comments. |
Align isSubmitterAndApprover with getSuggestedSearchesVisibility: - Submitter is any group-workspace member (isGroupPolicy) instead of role 'user', so admins/owners who submit are no longer excluded. - Approver gate uses isGroupPolicy instead of isPaidGroupPolicy, so Submit-type workspaces with an approval flow qualify. - Drop the redundant isSubmittedTo check; isPolicyApprover already covers submitsTo/forwardsTo (and overLimitForwardsTo). Update and extend the unit tests accordingly.
Gate the seed effect on the SAVED_SEARCHES Onyx load status. The value is undefined both while loading and when empty, so seeding on value alone could run before the key hydrates, bypass the duplicate-hash guard in seedMyExpensesSearch, and overwrite a pre-existing saved search of the same query on a fresh reconnect. Add regression tests covering the loading gate.
Fixed the scenario where user is an approver in another workspace. Please retest |
parasharrajat
left a comment
parasharrajat
left a comment
There was a problem hiding this comment.
Reviewer Checklist
- I have verified the author checklist is complete (all boxes are checked off).
- I verified the correct issue is linked in the
### Fixed Issuessection above - I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the
Testssection - I verified the steps for Staging and/or Production testing are in the
QA stepssection - I verified the steps cover any possible 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 verified the steps for local testing are in the
- I checked that screenshots or videos are included for tests on all platforms
- I included screenshots or videos for tests on all platforms
- I verified that the composer does not automatically focus or open the keyboard on mobile unless explicitly intended. This includes checking that returning the app from the background does not unexpectedly open the keyboard.
- I verified tests pass on all platforms & I tested again on:
- Android: HybridApp
- Android: mWeb Chrome
- iOS: HybridApp
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
- I verified proper code patterns were followed (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.
toggleReportand notonIconClick). - 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 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 that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e.
- If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
- I verified that this PR follows the guidelines as stated in the Review Guidelines
- I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like
Avatar, I verified the components usingAvatarhave been tested & I retested again) - If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a
/** comment above it */ - The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to
thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor) - Any internal methods bound to
thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick) - All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
- 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 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
Avataris modified, I verified thatAvataris 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
Designlabel and/or tagged@Expensify/designso the design team can review the changes.
- For any bug fix or new feature in this PR, I verified that sufficient unit tests are included to prevent regressions in this flow.
- If the
mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps. - I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.
Screenshots
🔲 iOS / native
24.07.2026_16.07.47_REC.mp4
🔲 iOS / Safari
24.07.2026_16.10.12_REC.mp4
🔲 MacOS / Chrome
24.07.2026_15.58.03_REC.mp4
🔲 Android / native
24.07.2026_16.15.10_REC.mp4
🎀 👀 🎀 C+ reviewed
|
🚧 Valforte 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/Valforte in version: 9.4.45-0 🚀
|
Help site review — docs update required ✅I reviewed the changes in this PR against the help site articles under Why: This PR now auto-seeds a default My expenses saved search ( I added a short "Why you have a My expenses saved search" section under How to save a search, explaining that it's seeded once and won't reappear if renamed or deleted. No docs change needed for the tooltip removal — the "Rename your saved searches here" product training tooltip is an in-product tooltip that was never documented on the help site. Draft help site PR: #97141 (labeled
@aswin-s, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
This PR failing because of the issue #97213 |
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.4.45-14 🚀
Bundle Size Analysis (Sentry): |
Expensify#93541 removed the only two uses of `index` in `buildSavedSearchMenuItem` (both were `index === 0 && shouldShowSavedSearchTooltip` gates for the deleted rename tooltip). The parameter stayed threaded through to createBaseSavedSearchMenuItem, which never referenced it. Also trim the two eligibility doc comments down to what they need to say.
@jponikarchuk The linked issue doesn't seem like a bug, rather that is the intended behaviour. Could you please check the explanation provided in #97213? |
7 participants
Explanation of Change
Automatically creates a "My expenses" saved search (filtered to
type:expense from:<currentUserAccountID>) for users who both submit and approve expenses. Seeding is gated by thenvp_hasSeededMyExpensesSearchNVP so it only fires once per account. This NVP is set server-side bySaveSearchand returned byOpenApp(backend support deployed via a separate Auth PR), so a deleted seed is not recreated — even after a fresh install, a full reconnect, or on another device. Also removes the now-obsoleteRename your saved searches hereproduct training tooltip that would otherwise show to users with the new seeded search.Fixed Issues
$ #92780
PROPOSAL: #92780 (comment)
Tests
from:<your account ID>.nvp_hasSeededMyExpensesSearchreturned byOpenAppprevents re-seeding).Offline tests
The seed uses the existing
saveSearchAPI write path which has optimistic data + failure rollback. Offline behavior is identical to manually saving a search offline — the entry appears optimistically and syncs when back online.QA Steps
Same as tests
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
android.mp4
Android: mWeb Chrome
android-web.mp4
iOS: Native
ios.mp4
iOS: mWeb Safari
ios-web.mp4
MacOS: Chrome / Safari
Mac.web.mp4