Confirm new expenses with a “View” toast instead of scrolling to and highlighting the row - #96496
Confirm new expenses with a “View” toast instead of scrolling to and highlighting the row#96496mhawryluk wants to merge 25 commits into
Conversation
…highlighting the row
|
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 |
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.
|
JakubKorytko
left a comment
There was a problem hiding this comment.
Swapping the imperative per-flow growl calls for a single declarative ExpenseAddedGrowl component that subscribes to Onyx is a solid improvement over the old approach! the comments are mostly nits. The batch-ordering one and the signal-lifecycle one are the two I would actually confirm before merge
| import useResponsiveLayout from '@hooks/useResponsiveLayout'; | ||
| import useSaveSortedReportIDs from '@hooks/useSaveSortedReportIDs'; | ||
| import useSearchHighlightAndScroll from '@hooks/useSearchHighlightAndScroll'; | ||
| import useSearchAutoRefetch from '@hooks/useSearchHighlightAndScroll'; |
There was a problem hiding this comment.
the hook is now useSearchAutoRefetch, but the file (and its test) are still named useSearchHighlightAndScroll. Could we rename the file to useSearchAutoRefetch.ts so it matches the hook and is easier to find?
There was a problem hiding this comment.
yes, definitely, but I'll rename it after the review, so the diff is easier to read. github shows the files as deleted and newly created after rename
|
@situchan can you review please? |
|
🚧 mountiny 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! 🧪🧪
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safariweb1.movweb2.movweb3.movweb3-2.movweb4.movweb6.movweb7.movweb8.mov |
JmillsExpensify
left a comment
There was a problem hiding this comment.
BIg fan of this. LGTM
|
Is it expected not show growl when add new expense to existing report with multiple expenses? And as you see video, highlighting the row still happens. Screen.Recording.2026-07-27.at.9.27.52.PM.mov |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62419b62d7
ℹ️ 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".
| <GrowlNotification ref={growlRef} /> | ||
| <ExpenseAddedGrowl /> |
There was a problem hiding this comment.
Coordinate the expense toast with the existing growl host
This mounts a second, independently managed GrowlNotificationContent alongside the existing singleton growl. On narrow layouts both use the same fixed top anchor, so if an existing Growl.error() is shown during the expense toast—for example from the tab-navigation error path—the two notifications overlap and neither replaces or dismisses the other, obscuring text and touch targets. Route this notification through the shared growl host or otherwise serialize the two sources.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
it's technically true, but in practice this will not happen currently - we very rarely show the imperative growl errors, and not when creating new expenses. and I don't think it's worth adding synchronization of the two growl mechanisms at this moment, I think this can be handled if we do end up using growls in more scenarios, which would cause the overlap. right now it would complicate the code and fix an issue that is not present and might never be
|
🤖 Review — this is a supplementary pass; the PR is already approved and has had thorough review from 1. Independent rapid creations can swallow a growl. In 2. 3. Idle 4. Confirm the removed-highlight cleanup is complete. This PR removes row highlighting in the Spend tab and switches The |
yes, that's intentional, here is the discussion: #91214 (comment) |
…transaction is scheduled
I added a comment.
Renamed ✅
Fixed by separating the component into two:
Confirmed. I found one outdated comment and updated it. |
|
@situchan I addressed the review comments, over to you |
Explanation of Change
Shows an "Expense added" growl (with a "View" action) after an expense is created from FAB outside of its report‚ as well as when duplicating and splitting expenses on Spend. Removes the highlighting of new expenses in the Spend tab tables completely.
It's driven declaratively by a single ExpenseAddedGrowl component, which watches an Onyx signal that the creation flows write to (same as they did for highlighting). In the previous approach each creation flow triggered the growl and its navigation imperatively, passing values that could go stale before the user tapped "View". Centralizing it in one Onyx-driven component removes that duplication and the stale-data problem.
Fixed Issues
$ #91408
PROPOSAL: N/A
Tests
Growl + "View" from global create (Inbox tab)
Growl in the Spend tab
"View" opens the correct destination for the current surface
Single growl/single navigation on a batch (skip-confirm multi-scan)
Invoice
Split an existing expense into multiple
Duplicate an expense
Self-DM expense
Share flow (native only)
Offline 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
Nagranie.z.ekranu.2026-07-23.o.18.05.47.mov
Nagranie.z.ekranu.2026-07-23.o.18.29.15.mov
Android: mWeb Chrome
iOS: Native
Simulator.Screen.Recording.-.iPhone.17.Pro.-.2026-07-21.at.17.04.37.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
Nagranie.z.ekranu.2026-07-20.o.16.46.23.mov
Nagranie.z.ekranu.2026-07-20.o.16.51.38.mov