Skip to content

fix: fall back to Onyx transactions for grouped view actions and default merchant sort to ASC #82053

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dangrous merged 8 commits into from
Feb 27, 2026
Merged

fix: fall back to Onyx transactions for grouped view actions and default merchant sort to ASC #82053

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
10687df
fix: fall back to Onyx transactions collection for grouped view actio…
TaduJR Feb 10, 2026
f0e1354
fix: default merchant sort to groupMerchant ASC matching category beh…
TaduJR Feb 10, 2026
f701771
Merge branch 'main' of https://github.com/TaduJR/App into feat-Insigh…
TaduJR Feb 10, 2026
0aaaf88
refactor: move groupByToSortColumn to module-level constant
TaduJR Feb 10, 2026
abe37dd
chore: run prettier
TaduJR Feb 10, 2026
9569cd1
fix: inline template literals to preserve PrefixedRecord key types
TaduJR Feb 10, 2026
7a91484
chore: run prettier
TaduJR Feb 10, 2026
2e33df1
test: add sort default tests for Top Categories and Top Merchants
TaduJR Feb 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions src/components/Search/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -644,8 +644,11 @@ function Search({
);
const canRejectRequest = email && transactionItem.report ? canRejectReportAction(email, transactionItem.report, transactionItem.policy) : false;

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>;
Copy link
Contributor

@ShridharGoel ShridharGoel Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also include transactions in the deps.

Copy link
Contributor Author

@TaduJR TaduJR Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@ShridharGoel ShridharGoel Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor Author

@TaduJR TaduJR Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const originalItemTransaction =
searchResults?.data?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${itemTransaction?.comment?.originalTransactionID}`] ??
transactions?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${itemTransaction?.comment?.originalTransactionID}`];

newTransactionList[transactionItem.transactionID] = {
transaction: transactionItem,
Expand Down Expand Up @@ -896,8 +899,11 @@ function Search({
currentTransactions
.filter((t) => !isTransactionPendingDelete(t))
.map((transactionItem) => {
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>;
const originalItemTransaction =
searchResults?.data?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${itemTransaction?.comment?.originalTransactionID}`] ??
transactions?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${itemTransaction?.comment?.originalTransactionID}`];
return mapTransactionItemToSelectedEntry(transactionItem, itemTransaction, originalItemTransaction, email ?? '', accountID, outstandingReportsByPolicyID);
}),
),
Expand Down
12 changes: 10 additions & 2 deletions src/libs/SearchUIUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,11 @@ type GetSectionsParams = {
allReportMetadata: OnyxCollection<OnyxTypes.ReportMetadata>;
};

const GROUP_BY_TO_SORT_COLUMN: Partial<Record<ValueOf<typeof CONST.SEARCH.GROUP_BY>, string>> = {
[CONST.SEARCH.GROUP_BY.CATEGORY]: CONST.SEARCH.TABLE_COLUMNS.GROUP_CATEGORY,
[CONST.SEARCH.GROUP_BY.MERCHANT]: CONST.SEARCH.TABLE_COLUMNS.GROUP_MERCHANT,
} as const;

/**
* Creates a top search menu item with common structure for TOP_SPENDERS, TOP_CATEGORIES, and TOP_MERCHANTS
*/
Expand All @@ -492,6 +497,9 @@ function createTopSearchMenuItem(
limit?: number,
view?: ValueOf<typeof CONST.SEARCH.VIEW>,
): SearchTypeMenuItem {
const defaultSortBy = GROUP_BY_TO_SORT_COLUMN[groupBy] ?? CONST.SEARCH.TABLE_COLUMNS.GROUP_TOTAL;
const defaultSortOrder = GROUP_BY_TO_SORT_COLUMN[groupBy] ? CONST.SEARCH.SORT_ORDER.ASC : CONST.SEARCH.SORT_ORDER.DESC;

const searchQuery = buildQueryStringFromFilterFormValues(
{
type: CONST.SEARCH.DATA_TYPES.EXPENSE,
Expand All @@ -500,8 +508,8 @@ function createTopSearchMenuItem(
...(view && {view}),
},
{
sortBy: CONST.SEARCH.TABLE_COLUMNS.GROUP_TOTAL,
sortOrder: CONST.SEARCH.SORT_ORDER.DESC,
sortBy: defaultSortBy,
sortOrder: defaultSortOrder,
...(limit && {limit}),
},
);
Expand Down
16 changes: 16 additions & 0 deletions tests/unit/Search/SearchUIUtilsTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5239,6 +5239,22 @@ describe('SearchUIUtils', () => {
});
});

describe('Test getSuggestedSearches sort defaults', () => {
test('Should default Top Categories to sortBy groupCategory and sortOrder asc', () => {
const suggestedSearches = SearchUIUtils.getSuggestedSearches(adminAccountID);
const topCategories = suggestedSearches[CONST.SEARCH.SEARCH_KEYS.TOP_CATEGORIES];
expect(topCategories.searchQueryJSON?.sortBy).toBe(CONST.SEARCH.TABLE_COLUMNS.GROUP_CATEGORY);
expect(topCategories.searchQueryJSON?.sortOrder).toBe(CONST.SEARCH.SORT_ORDER.ASC);
});

test('Should default Top Merchants to sortBy groupMerchant and sortOrder asc', () => {
const suggestedSearches = SearchUIUtils.getSuggestedSearches(adminAccountID);
const topMerchants = suggestedSearches[CONST.SEARCH.SEARCH_KEYS.TOP_MERCHANTS];
expect(topMerchants.searchQueryJSON?.sortBy).toBe(CONST.SEARCH.TABLE_COLUMNS.GROUP_MERCHANT);
expect(topMerchants.searchQueryJSON?.sortOrder).toBe(CONST.SEARCH.SORT_ORDER.ASC);
});
});

describe('Test getColumnsToShow', () => {
test('Should show all default columns when no custom columns are saved & viewing expense reports', () => {
expect(SearchUIUtils.getColumnsToShow(1, [], [], false, CONST.SEARCH.DATA_TYPES.EXPENSE_REPORT)).toEqual([
Expand Down
Loading