fix: fall back to Onyx transactions for grouped view actions and default merchant sort to ASC#82053
Conversation
|
@dukenv0307 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] |
…ts-Release-2-Top-Merchants-Add-group-by-merchant-and-suggested-search # Conflicts: # src/libs/SearchUIUtils.ts
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.
|
|
@TaduJR Should I review this PR? |
Thanks for the help @dukenv0307 🙏 But @ShridharGoel will handle this as part of follow up |
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Related to WN project
|
@ShridharGoel also ready for review. |
| const itemTransaction = searchResults?.data?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionItem.transactionID}`] as OnyxEntry<Transaction>; | ||
| const originalItemTransaction = searchResults?.data?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${itemTransaction?.comment?.originalTransactionID}`]; | ||
| const itemTransaction = (searchResults?.data?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionItem.transactionID}`] ?? | ||
| transactions?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionItem.transactionID}`]) as OnyxEntry<Transaction>; |
There was a problem hiding this comment.
We should also include transactions in the deps.
There was a problem hiding this comment.
transactions is intentionally excluded. It's the entire Onyx transaction collection useOnyx(ONYXKEYS.COLLECTION.TRANSACTION and updates on every transaction change app-wide.
I think adding it would cause expensive useEffect to re-run for unrelated transaction changes.
WDYT?
There was a problem hiding this comment.
But then the effect won’t re-run when only Onyx transactions change which might lead to stale data. Wouldn't it be a problem? Or instead of the full transactions collection, is it feasible to use a narrowed subscription/selector ?
There was a problem hiding this comment.
I verified with debug loggers. On a page load with 3 Open expenses, transactions (the full Onyx collection) fires ~30+ times from property-only changes while hasTransactionsIDsChange=false every time meaning these are all property updates on existing transactions, not new/deleted ones.
Adding transactions to deps wouldn't fix stale data either filteredData (which drives the iteration inside the effect) is built from the search snapshot, not live Onyx. So the effect would re-run ~30+ extra times but still iterate stale snapshot data.
A narrowed selector has a circular dependency problem: we'd need filteredData transaction IDs to build the selector, but the effect depends on filteredData. I tried to search but there's also no existing codebase pattern using a selector on a full collection key.
So I think the ?? transactions fallback is only a lookup for grouped views where searchResults.data lacks individual transaction keys it's read at execution time with whatever value it has, not a reactivity source.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-02-27.at.6.19.15.PM.movAndroid: mWeb ChromeScreen.Recording.2026-02-27.at.6.24.54.PM.moviOS: HybridAppScreen.Recording.2026-02-27.at.6.10.31.PM.moviOS: mWeb SafariScreen.Recording.2026-02-27.at.6.15.37.PM.movMacOS: Chrome / SafariScreen.Recording.2026-02-27.at.6.00.33.PM.movScreen.Recording.2026-02-27.at.6.02.51.PM.mov |
The split option shouldn't show for multiple selected expenses, right? |
|
Kindly add the pending recordings as well |
Yes that is correct |
Can you update the test then? It says to select multiple expenses and then verify that the split option shows. |
Done.
Done. |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #80767 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
🚧 @dangrous has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/dangrous in version: 9.3.28-0 🚀
|
|
🚀 Deployed to staging by https://github.com/dangrous in version: 9.3.30-0 🚀
|
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.3.30-3 🚀
|
Explanation of Change
Fix 1: Grouped view action capabilities
In grouped search views (Top Merchants, Top Categories, etc.),
searchResults.dataonly contains group-level keys (group_123), not individual transaction keys (transactions_456). Two code paths inSearch/index.tsxused
searchResults.datato look up transactions for computing action capabilities (split, hold, reject). These lookups returnedundefinedin grouped views, causingisSplitActionto fail and the Split option todisappear after selection.
Added
?? transactions?.[key]fallback to the Onyx transactions collection in both paths:canSplit: trueFix 2: Merchant sort order defaults
createTopSearchMenuItemhardcoded category-specific defaults (sortBy: GROUP_CATEGORY,sortOrder: ASC), but merchants fell through toGROUP_TOTAL/DESC. This meant the Merchant column started inactive, andSortableHeaderTextdefaulted inactive columns to DESC on first click — giving Z-A instead of A-Z.Replaced the category-only ternary with a generic
groupByToSortColumnmap so merchants also default tosortBy: GROUP_MERCHANT,sortOrder: ASC, matching the parser defaults and category behavior.Fixed Issues
$ #80767
PROPOSAL:
Tests
Test Case 1: Grouped view action capabilities
Test Case 2: Merchant sort order defaults
Offline tests
Same as tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
Screen.Recording.2026-02-27.at.7.24.02.in.the.evening.mov
Android: mWeb Chrome
Android-mWeb.mp4
iOS: Native
iOS-Native.mp4
iOS: mWeb Safari
Screen.Recording.2026-02-27.at.5.30.43.in.the.afternoon.mov
MacOS: Chrome / Safari
Mac-Chrome.mp4