From f892e41d335518e799a539f80301d37f81d63dbb Mon Sep 17 00:00:00 2001 From: "Kevin Brian Bader (via MelvinBot)" Date: Thu, 17 Sep 2026 23:29:27 +0000 Subject: [PATCH 1/7] Let an explicit column sort win over the RBR ordering and the alphabetical group order --- .../MoneyRequestReportTransactionList.tsx | 30 ++++- src/libs/ReportLayoutUtils.ts | 44 +++++-- tests/unit/ReportLayoutUtilsTest.ts | 119 ++++++++++++++++++ 3 files changed, 182 insertions(+), 11 deletions(-) diff --git a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx index 2ce48fa2bba1..753126be22e0 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx @@ -26,6 +26,7 @@ import {navigationRef} from '@libs/Navigation/Navigation'; import {isPolicyTaxEnabled} from '@libs/PolicyUtils'; import {getOriginalMessage, isMoneyRequestAction} from '@libs/ReportActionsUtils'; import {groupTransactionsByCategory, groupTransactionsByTag} from '@libs/ReportLayoutUtils'; +import type {CompareLeadingTransactions} from '@libs/ReportLayoutUtils'; import { getActionErrorsByTransaction, getMoneyRequestSpendBreakdown, @@ -373,7 +374,10 @@ function MoneyRequestReportTransactionList({ }); const {sortBy, sortOrder} = sortConfig; - const isDefaultSort = sortBy === CONST.SEARCH.TABLE_COLUMNS.DATE && sortOrder === CONST.SEARCH.SORT_ORDER.ASC; + // Date/ASC is both the initial state and where every second Date press lands, so pressing a column has to be + // tracked separately for an explicit sort to win over the RBR ordering below. + const [hasUserSortedTransactions, setHasUserSortedTransactions] = useState(false); + const isDefaultSort = !hasUserSortedTransactions && sortBy === CONST.SEARCH.TABLE_COLUMNS.DATE && sortOrder === CONST.SEARCH.SORT_ORDER.ASC; // In a single pass over reportActions, build: // - reportActionsMap: keyed by reportActionID for transactionHasRBR. @@ -495,18 +499,35 @@ function MoneyRequestReportTransactionList({ const currentGroupBy: OnyxTypes.ReportLayoutGroupBy = currentSelection !== CONST.REPORT_LAYOUT.LAYOUT_OPTION.MATRIX ? currentSelection : getReportLayoutGroupBy(reportLayoutGroupBy); const shouldGroupTransactions = shouldShowGroupedTransactions && !isLayoutMatrixSelected; + // Once the user presses a column the group headers follow that column too, otherwise the groups stay alphabetical + // and only the rows inside each group would be ordered. + const compareLeadingTransactions: CompareLeadingTransactions | undefined = useMemo(() => { + if (!hasUserSortedTransactions) { + return undefined; + } + return (a, b) => + compareValues( + getTransactionSortValue(a, sortBy, report, policy, policyCategories, policyTagLists), + getTransactionSortValue(b, sortBy, report, policy, policyCategories, policyTagLists), + sortOrder, + sortBy, + localeCompare, + true, + ); + }, [hasUserSortedTransactions, sortBy, sortOrder, report, policy, policyCategories, policyTagLists, localeCompare]); + const groupedTransactions = useMemo(() => { if (!shouldGroupTransactions) { return []; } if (currentGroupBy === CONST.REPORT_LAYOUT.GROUP_BY.TAG) { - return groupTransactionsByTag(resolvedTransactions, report, localeCompare); + return groupTransactionsByTag(resolvedTransactions, report, localeCompare, compareLeadingTransactions); } - return groupTransactionsByCategory(resolvedTransactions, report, localeCompare); + return groupTransactionsByCategory(resolvedTransactions, report, localeCompare, compareLeadingTransactions); // groupTransactionsByTag() and groupTransactionsByCategory() use the full report object to perform a null check. // We skip including the report as a dependency to avoid unnecessary re-renders as it changes often and we only need to recalculate when currency changes. // eslint-disable-next-line react-hooks/exhaustive-deps - }, [resolvedTransactions, currentGroupBy, report?.reportID, report?.currency, localeCompare, shouldGroupTransactions]); + }, [resolvedTransactions, currentGroupBy, report?.reportID, report?.currency, localeCompare, shouldGroupTransactions, compareLeadingTransactions]); const visualOrderTransactionIDs = useMemo(() => { if (!shouldGroupTransactions || groupedTransactions.length === 0) { @@ -818,6 +839,7 @@ function MoneyRequestReportTransactionList({ if (!isSortableColumnName(selectedSortBy)) { return; } + setHasUserSortedTransactions(true); setSortConfig((prevState) => ({...prevState, sortBy: selectedSortBy, sortOrder: selectedSortOrder})); }} dateColumnSize={dateColumnSize} diff --git a/src/libs/ReportLayoutUtils.ts b/src/libs/ReportLayoutUtils.ts index 48491784ec68..76bfbc951538 100644 --- a/src/libs/ReportLayoutUtils.ts +++ b/src/libs/ReportLayoutUtils.ts @@ -10,11 +10,30 @@ import {getDecodedCategoryName, isCategoryMissing} from './CategoryUtils'; import {getDecodedTagName, isTagMissing} from './TagUtils'; import {getAmount, getCategory, getCurrency, getTag, isTransactionPendingDelete} from './TransactionUtils'; +/** Compares the leading (first rendered) transaction of two groups under the sort the user selected */ +type CompareLeadingTransactions = (a: Transaction, b: Transaction) => number; + /** - * Sorts groups alphabetically (A→Z) with empty keys at the end + * Sorts groups alphabetically (A→Z) with empty keys at the end. + * When `compareLeadingTransactions` is passed, the groups follow the sorted column instead, so the group headers + * can't hold a group in place while the rows inside it move. Alphabetical order remains the tiebreak. */ -function sortGroupedTransactions(groups: GroupedTransactions[], localeCompare: LocaleContextProps['localeCompare']): GroupedTransactions[] { - return groups.sort((a, b) => { +function sortGroupedTransactions( + groups: GroupedTransactions[], + localeCompare: LocaleContextProps['localeCompare'], + compareLeadingTransactions?: CompareLeadingTransactions, +): GroupedTransactions[] { + return [...groups].sort((a, b) => { + if (compareLeadingTransactions) { + const leadingA = a.transactions.at(0); + const leadingB = b.transactions.at(0); + if (leadingA && leadingB) { + const result = compareLeadingTransactions(leadingA, leadingB); + if (result !== 0) { + return result; + } + } + } if (a.groupKey === '' && b.groupKey !== '') { return 1; } @@ -57,7 +76,12 @@ function calculateGroupTotal(transactionList: Transaction[], reportCurrency: str /** * Groups transactions by category */ -function groupTransactionsByCategory(transactions: Transaction[], report: OnyxEntry, localeCompare: LocaleContextProps['localeCompare']): GroupedTransactions[] { +function groupTransactionsByCategory( + transactions: Transaction[], + report: OnyxEntry, + localeCompare: LocaleContextProps['localeCompare'], + compareLeadingTransactions?: CompareLeadingTransactions, +): GroupedTransactions[] { if (!report) { return []; } @@ -86,13 +110,18 @@ function groupTransactionsByCategory(transactions: Transaction[], report: OnyxEn }); } - return sortGroupedTransactions(result, localeCompare); + return sortGroupedTransactions(result, localeCompare, compareLeadingTransactions); } /** * Groups transactions by tag */ -function groupTransactionsByTag(transactions: Transaction[], report: OnyxEntry, localeCompare: LocaleContextProps['localeCompare']): GroupedTransactions[] { +function groupTransactionsByTag( + transactions: Transaction[], + report: OnyxEntry, + localeCompare: LocaleContextProps['localeCompare'], + compareLeadingTransactions?: CompareLeadingTransactions, +): GroupedTransactions[] { if (!report) { return []; } @@ -121,7 +150,8 @@ function groupTransactionsByTag(transactions: Transaction[], report: OnyxEntry a.localeCompare(b); @@ -25,6 +28,15 @@ const createMockReport = (overrides: Partial = {}): Report => ...overrides, }) as Report; +// Stands in for the date comparator the transaction list builds from the active sort, so the groups can be checked +// against the same ordering the rows use. +const compareByCreated = + (sortOrder: ValueOf): CompareLeadingTransactions => + (a, b) => { + const result = (a.created ?? '').localeCompare(b.created ?? ''); + return sortOrder === CONST.SEARCH.SORT_ORDER.ASC ? result : -result; + }; + describe('groupTransactionsByCategory', () => { it('returns empty array when report is undefined', () => { const transactions = [createMockTransaction({category: 'Travel'})]; @@ -468,3 +480,110 @@ describe('groupTransactionsByTag', () => { expect(result.at(0)?.subTotalAmount).toBe(3000); }); }); + +describe('group ordering under an explicit sort', () => { + it('keeps groups alphabetical with the empty key last when no comparator is passed', () => { + const report = createMockReport(); + const transactions = [ + createMockTransaction({transactionID: '1', category: '', created: '2026-09-01'}), + createMockTransaction({transactionID: '2', category: 'Zebra', created: '2026-09-02'}), + createMockTransaction({transactionID: '3', category: 'Alpha', created: '2026-09-03'}), + ]; + + const result = groupTransactionsByCategory(transactions, report, mockLocaleCompare); + + expect(result.map((group) => group.groupKey)).toEqual(['Alpha', 'Zebra', '']); + }); + + it('orders groups by their leading transaction when sorting by date ascending', () => { + const report = createMockReport(); + const transactions = [ + createMockTransaction({transactionID: '1', category: 'Travel', created: '2026-09-15'}), + createMockTransaction({transactionID: '2', category: 'Meals', created: '2026-09-17'}), + createMockTransaction({transactionID: '3', category: 'Advertising', created: '2026-09-20'}), + ]; + + const result = groupTransactionsByCategory(transactions, report, mockLocaleCompare, compareByCreated(CONST.SEARCH.SORT_ORDER.ASC)); + + expect(result.map((group) => group.groupKey)).toEqual(['Travel', 'Meals', 'Advertising']); + }); + + it('orders groups by their leading transaction when sorting by date descending', () => { + const report = createMockReport(); + const transactions = [ + createMockTransaction({transactionID: '1', category: 'Advertising', created: '2026-09-20'}), + createMockTransaction({transactionID: '2', category: 'Meals', created: '2026-09-17'}), + createMockTransaction({transactionID: '3', category: 'Travel', created: '2026-09-15'}), + ]; + + const result = groupTransactionsByCategory(transactions, report, mockLocaleCompare, compareByCreated(CONST.SEARCH.SORT_ORDER.DESC)); + + expect(result.map((group) => group.groupKey)).toEqual(['Advertising', 'Meals', 'Travel']); + }); + + it('lets the empty key group leave the bottom when the sort puts it first', () => { + const report = createMockReport(); + const transactions = [ + createMockTransaction({transactionID: '1', category: '', created: '2026-09-01'}), + createMockTransaction({transactionID: '2', category: 'Travel', created: '2026-09-10'}), + ]; + + const result = groupTransactionsByCategory(transactions, report, mockLocaleCompare, compareByCreated(CONST.SEARCH.SORT_ORDER.ASC)); + + expect(result.map((group) => group.groupKey)).toEqual(['', 'Travel']); + }); + + it('falls back to alphabetical order when the leading transactions tie', () => { + const report = createMockReport(); + const transactions = [ + createMockTransaction({transactionID: '1', category: 'Zebra', created: '2026-09-10'}), + createMockTransaction({transactionID: '2', category: 'Alpha', created: '2026-09-10'}), + createMockTransaction({transactionID: '3', category: '', created: '2026-09-10'}), + ]; + + const result = groupTransactionsByCategory(transactions, report, mockLocaleCompare, compareByCreated(CONST.SEARCH.SORT_ORDER.ASC)); + + expect(result.map((group) => group.groupKey)).toEqual(['Alpha', 'Zebra', '']); + }); + + it('orders tag groups by their leading transaction too', () => { + const report = createMockReport(); + const transactions = [ + createMockTransaction({transactionID: '1', tag: 'Project Z', created: '2026-09-15'}), + createMockTransaction({transactionID: '2', tag: 'Project A', created: '2026-09-20'}), + ]; + + const result = groupTransactionsByTag(transactions, report, mockLocaleCompare, compareByCreated(CONST.SEARCH.SORT_ORDER.ASC)); + + expect(result.map((group) => group.groupKey)).toEqual(['Project Z', 'Project A']); + }); + + // The RHP prev/next arrows walk the transaction IDs flat-mapped out of these groups, so the flattened groups have + // to come back in the same order as the sorted rows or "next" would jump to a row the user isn't looking at. + it('flattens back into the sorted row order', () => { + const report = createMockReport(); + // The rows reach the grouping already sorted, so they are passed in newest first here as well. + const transactions = [ + createMockTransaction({transactionID: '4', category: 'Meals', created: '2026-09-18'}), + createMockTransaction({transactionID: '3', category: 'Travel', created: '2026-09-17'}), + createMockTransaction({transactionID: '2', category: 'Meals', created: '2026-09-16'}), + createMockTransaction({transactionID: '1', category: 'Travel', created: '2026-09-15'}), + ]; + + const result = groupTransactionsByCategory(transactions, report, mockLocaleCompare, compareByCreated(CONST.SEARCH.SORT_ORDER.DESC)); + + expect(result.flatMap((group) => group.transactions.map((transaction) => transaction.transactionID))).toEqual(['4', '2', '3', '1']); + }); + + it('does not reorder the transactions array it was given', () => { + const report = createMockReport(); + const transactions = [ + createMockTransaction({transactionID: '1', category: 'Zebra', created: '2026-09-20'}), + createMockTransaction({transactionID: '2', category: 'Alpha', created: '2026-09-15'}), + ]; + + groupTransactionsByCategory(transactions, report, mockLocaleCompare, compareByCreated(CONST.SEARCH.SORT_ORDER.ASC)); + + expect(transactions.map((transaction) => transaction.transactionID)).toEqual(['1', '2']); + }); +}); From 8a42888884a36c965fffc073b759975c269097b8 Mon Sep 17 00:00:00 2001 From: "Kevin Brian Bader (via MelvinBot)" Date: Fri, 18 Sep 2026 00:30:37 +0000 Subject: [PATCH 2/7] Address review: reset sort per report, fold the group comparator into the grouping memo, add the RHP arrow regression test --- .../MoneyRequestReportTransactionList.tsx | 72 ++++-- src/libs/ReportLayoutUtils.ts | 2 + ...equestReportTransactionsNavigationTest.tsx | 211 +++++++++++++++++- tests/unit/ReportLayoutUtilsTest.ts | 10 +- 4 files changed, 267 insertions(+), 28 deletions(-) diff --git a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx index 753126be22e0..8cdd851f90e9 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx @@ -229,6 +229,12 @@ type SortedTransactions = { sortOrder: SortOrder; }; +/** Kept at module scope so resetting to it on a report change is a no-op re-render when the sort is already default. */ +const DEFAULT_SORT_CONFIG: SortedTransactions = { + sortBy: CONST.SEARCH.TABLE_COLUMNS.DATE, + sortOrder: CONST.SEARCH.SORT_ORDER.ASC, +}; + function MoneyRequestReportTransactionList({ report, transactions, @@ -368,10 +374,7 @@ function MoneyRequestReportTransactionList({ // eslint-disable-next-line react-hooks/exhaustive-deps }, [reportID]); - const [sortConfig, setSortConfig] = useState({ - sortBy: CONST.SEARCH.TABLE_COLUMNS.DATE, - sortOrder: CONST.SEARCH.SORT_ORDER.ASC, - }); + const [sortConfig, setSortConfig] = useState(DEFAULT_SORT_CONFIG); const {sortBy, sortOrder} = sortConfig; // Date/ASC is both the initial state and where every second Date press lands, so pressing a column has to be @@ -379,6 +382,19 @@ function MoneyRequestReportTransactionList({ const [hasUserSortedTransactions, setHasUserSortedTransactions] = useState(false); const isDefaultSort = !hasUserSortedTransactions && sortBy === CONST.SEARCH.TABLE_COLUMNS.DATE && sortOrder === CONST.SEARCH.SORT_ORDER.ASC; + // This component is reused across reportID changes instead of being remounted (there is no key on the usage in + // MoneyRequestReportActionsList), which is why the selection above has to be cleared by hand. The sort is the + // same: without this reset, sorting one report would carry into the next report opened and suppress the RBR-first + // ordering that report's first open is supposed to get. Adjusted during render rather than in an effect, which is + // the pattern React recommends for resetting state on a prop change: it re-renders before anything is committed, + // so the new report never paints with the previous one's sort. + const [sortedReportID, setSortedReportID] = useState(reportID); + if (sortedReportID !== reportID) { + setSortedReportID(reportID); + setSortConfig(DEFAULT_SORT_CONFIG); + setHasUserSortedTransactions(false); + } + // In a single pass over reportActions, build: // - reportActionsMap: keyed by reportActionID for transactionHasRBR. // - transactionThreadReportIDByTransactionID: transactionID → transaction-thread report ID, so each row can pass it @@ -499,35 +515,47 @@ function MoneyRequestReportTransactionList({ const currentGroupBy: OnyxTypes.ReportLayoutGroupBy = currentSelection !== CONST.REPORT_LAYOUT.LAYOUT_OPTION.MATRIX ? currentSelection : getReportLayoutGroupBy(reportLayoutGroupBy); const shouldGroupTransactions = shouldShowGroupedTransactions && !isLayoutMatrixSelected; - // Once the user presses a column the group headers follow that column too, otherwise the groups stay alphabetical - // and only the rows inside each group would be ordered. - const compareLeadingTransactions: CompareLeadingTransactions | undefined = useMemo(() => { - if (!hasUserSortedTransactions) { - return undefined; - } - return (a, b) => - compareValues( - getTransactionSortValue(a, sortBy, report, policy, policyCategories, policyTagLists), - getTransactionSortValue(b, sortBy, report, policy, policyCategories, policyTagLists), - sortOrder, - sortBy, - localeCompare, - true, - ); - }, [hasUserSortedTransactions, sortBy, sortOrder, report, policy, policyCategories, policyTagLists, localeCompare]); - const groupedTransactions = useMemo(() => { if (!shouldGroupTransactions) { return []; } + // Once the user presses a column the group headers follow that column too, otherwise the groups stay + // alphabetical and only the rows inside each group would be ordered. Built inside this memo so it inherits the + // narrowed report dependency below instead of pulling the whole report object back in as its own memo would. + const compareLeadingTransactions: CompareLeadingTransactions | undefined = hasUserSortedTransactions + ? (a, b) => + compareValues( + getTransactionSortValue(a, sortBy, report, policy, policyCategories, policyTagLists), + getTransactionSortValue(b, sortBy, report, policy, policyCategories, policyTagLists), + sortOrder, + sortBy, + localeCompare, + true, + ) + : undefined; if (currentGroupBy === CONST.REPORT_LAYOUT.GROUP_BY.TAG) { return groupTransactionsByTag(resolvedTransactions, report, localeCompare, compareLeadingTransactions); } return groupTransactionsByCategory(resolvedTransactions, report, localeCompare, compareLeadingTransactions); // groupTransactionsByTag() and groupTransactionsByCategory() use the full report object to perform a null check. // We skip including the report as a dependency to avoid unnecessary re-renders as it changes often and we only need to recalculate when currency changes. + // The comparator reads report and policy fields too, but resolvedTransactions is derived from the row sort, + // which does depend on both in full, so any change to either already invalidates this memo through it. // eslint-disable-next-line react-hooks/exhaustive-deps - }, [resolvedTransactions, currentGroupBy, report?.reportID, report?.currency, localeCompare, shouldGroupTransactions, compareLeadingTransactions]); + }, [ + resolvedTransactions, + currentGroupBy, + report?.reportID, + report?.currency, + localeCompare, + shouldGroupTransactions, + hasUserSortedTransactions, + sortBy, + sortOrder, + policy?.id, + policyCategories, + policyTagLists, + ]); const visualOrderTransactionIDs = useMemo(() => { if (!shouldGroupTransactions || groupedTransactions.length === 0) { diff --git a/src/libs/ReportLayoutUtils.ts b/src/libs/ReportLayoutUtils.ts index 76bfbc951538..054ac4f7fd9c 100644 --- a/src/libs/ReportLayoutUtils.ts +++ b/src/libs/ReportLayoutUtils.ts @@ -27,6 +27,8 @@ function sortGroupedTransactions( if (compareLeadingTransactions) { const leadingA = a.transactions.at(0); const leadingB = b.transactions.at(0); + // Defensive only: the grouping functions below create a group at the moment they push a transaction into + // it, so a group is never empty and this guard never falls through to the alphabetical order in practice. if (leadingA && leadingB) { const result = compareLeadingTransactions(leadingA, leadingB); if (result !== 0) { diff --git a/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx b/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx index 453ecba4ef86..e0040470d3e3 100644 --- a/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx +++ b/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx @@ -1,38 +1,100 @@ -import {fireEvent, render, screen, waitFor} from '@testing-library/react-native'; +import {act, fireEvent, render, screen, waitFor} from '@testing-library/react-native'; +import ComposeProviders from '@components/ComposeProviders'; +import {LocaleContextProvider} from '@components/LocaleContextProvider'; +import MoneyRequestReportTransactionList from '@components/MoneyRequestReportView/MoneyRequestReportTransactionList'; import MoneyRequestReportTransactionsNavigation from '@components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation'; +import OnyxListItemProvider from '@components/OnyxListItemProvider'; +import type {SortOrder} from '@components/Search/types'; import * as ReportActions from '@libs/actions/Report'; +import {clearActiveTransactionIDs} from '@libs/actions/TransactionThreadNavigation'; import Navigation from '@navigation/Navigation'; import CONST from '@src/CONST'; import ONYXKEYS from '@src/ONYXKEYS'; import ROUTES from '@src/ROUTES'; +import SCREENS from '@src/SCREENS'; +import type {StableReport} from '@src/selectors/Report'; import type {ReportActions as OnyxReportActions, Transaction} from '@src/types/onyx'; import React from 'react'; +import {View} from 'react-native'; import Onyx from 'react-native-onyx'; +import type * as SearchContext from '@components/Search/SearchContext'; + import createRandomReportAction from '../utils/collections/reportActions'; import createRandomTransaction from '../utils/collections/transaction'; import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; +/** The rendered list items the transaction list hands the unified list, in render order. */ +type CapturedListItem = {type: 'section-header'} | {type: 'transaction'; transaction: Transaction}; + +/** The slice of the transaction list's controller the tests below read. */ +type CapturedController = { + tableColumnHeader: React.ReactElement<{onSortPress: (sortBy: string, sortOrder: SortOrder) => void}> | null; + transactionListItems: CapturedListItem[]; +}; + +// Drives the RHP-open check both the transaction list and the navigation component make through findFocusedRoute(). +// Undefined reproduces the real module's behaviour while the navigation ref isn't ready, which is what the existing +// tests below run against. +const mockFocusedRoute: {value: {name: string; key: string} | undefined} = {value: undefined}; + +// Populated by the mocked unified list below with the controller the transaction list renders it with. +const mockUnifiedList: {controller: CapturedController | undefined} = {controller: undefined}; + jest.mock('@react-navigation/native', () => { // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment -- jest.requireActual() returns the real module for partial mocking const actualNavigation = jest.requireActual('@react-navigation/native'); - // eslint-disable-next-line @typescript-eslint/no-unsafe-return -- returning the real module plus one overridden hook is the standard Jest partial-mock pattern + // eslint-disable-next-line @typescript-eslint/no-unsafe-return -- returning the real module plus a few overridden hooks is the standard Jest partial-mock pattern return { ...actualNavigation, useIsFocused: () => true, + useFocusEffect: () => {}, + findFocusedRoute: () => mockFocusedRoute.value, }; }); +// The unified list is a FlashList; the tests below only need the controller it is handed, which carries the rendered +// row order and the real column-header element. +jest.mock('@components/MoneyRequestReportView/MoneyRequestReportUnifiedList', () => ({ + __esModule: true, + default: ({controller}: {controller: CapturedController}) => { + mockUnifiedList.controller = controller; + return null; + }, +})); + +jest.mock('@hooks/useResponsiveLayout', () => ({ + __esModule: true, + default: () => ({shouldUseNarrowLayout: false, isSmallScreenWidth: false, isMediumScreenWidth: false, isLargeScreenWidth: true, isExtraLargeScreenWidth: true, onboardingIsMediumOrLargerScreenWidth: true}), +})); + +jest.mock('@hooks/useResponsiveLayoutOnWideRHP', () => ({ + __esModule: true, + default: () => ({shouldUseNarrowLayout: false}), +})); + +// The transaction list only reads the selection slice of the search context, and the real provider needs a +// NavigationContainer, so the two selection hooks are stubbed instead of mounting the whole provider tree. +jest.mock('@components/Search/SearchContext', () => ({ + ...jest.requireActual('@components/Search/SearchContext'), + useSearchSelectionContext: () => ({selectedTransactionIDs: []}), + useSearchSelectionActions: () => ({setSelectedTransactions: () => {}, clearSelectedTransactions: () => {}}), +})); + jest.mock('@components/WideRHPContextProvider', () => ({ useWideRHPActions: () => ({markReportRHPWidth: jest.fn(), unmarkReportRHPWidth: jest.fn()}), })); jest.mock('@components/OnyxListItemProvider', () => ({ + __esModule: true, + // The provider itself only seeds context the components under test read through the mocked hook below, so it is a + // passthrough here. It still has to be a component: the transaction-list tests compose it as a provider. + default: ({children}: {children: React.ReactNode}) => children, usePersonalDetails: () => ({}), })); @@ -177,3 +239,148 @@ describe('MoneyRequestReportTransactionsNavigation', () => { expect(openReportSpy).not.toHaveBeenCalled(); }); }); + +const EXPENSE_REPORT_ID = 'expense1'; +const POLICY_ID = 'policy1'; + +function buildExpenseReportTransaction(transactionID: string, created: string, category: string, index: number): Transaction { + return {...createRandomTransaction(index), transactionID, reportID: EXPENSE_REPORT_ID, created, category, currency: 'USD', amount: -1000}; +} + +function buildTransactionListElement(transactions: Transaction[], reportID: string) { + const report = { + reportID, + policyID: POLICY_ID, + type: CONST.REPORT.TYPE.EXPENSE, + currency: 'USD', + ownerAccountID: 1, + } as StableReport; + + return ( + + } + reportActionsExtraData={undefined} + linkedReportActionID={undefined} + listRef={null} + accessibilityLabel="transactions" + onListLayout={jest.fn()} + onScroll={jest.fn()} + onScrollBeginDrag={jest.fn()} + onContentSizeChange={jest.fn()} + onViewableItemsChanged={jest.fn()} + onEndReached={jest.fn()} + onStartReached={jest.fn()} + contentContainerStyle={undefined} + isLoadingInitialActions={false} + /> + + ); +} + +function pressDateHeader(sortOrder: SortOrder) { + const onSortPress = mockUnifiedList.controller?.tableColumnHeader?.props.onSortPress; + if (!onSortPress) { + throw new Error('the sortable column header did not render'); + } + act(() => { + onSortPress(CONST.SEARCH.TABLE_COLUMNS.DATE, sortOrder); + }); +} + +function getRenderedTransactionIDs(): string[] { + const listItems = mockUnifiedList.controller?.transactionListItems ?? []; + return listItems.filter((item): item is {type: 'transaction'; transaction: Transaction} => item.type === 'transaction').map((item) => item.transaction.transactionID); +} + +async function getCarouselTransactionIDs(): Promise { + return new Promise((resolve) => { + const connection = Onyx.connectWithoutView({ + key: ONYXKEYS.TRANSACTION_THREAD_NAVIGATION_TRANSACTION_IDS, + callback: (ids) => { + Onyx.disconnect(connection); + resolve(ids); + }, + }); + }); +} + +// The RHP prev/next arrows walk the transaction IDs this list seeds, so the seed has to stay equal to the rows the +// user is actually looking at. Sorting is the case that breaks it: the rows move, and if the seed is built from +// anything other than the rendered groups (a flattened list, or groups left in alphabetical order) "next" jumps to a +// row that isn't below the current one on screen. +describe('MoneyRequestReportTransactionList - RHP arrow order', () => { + // The newest expense is in the alphabetically-last category, so date order and alphabetical group order disagree: + // under Date DESC the groups must come back Travel then Meals, which is the opposite of the alphabetical order the + // groups fall back to when no column has been pressed. + const transactions = [ + buildExpenseReportTransaction('1', '2026-09-15', 'Meals', 0), + buildExpenseReportTransaction('2', '2026-09-16', 'Travel', 1), + buildExpenseReportTransaction('3', '2026-09-17', 'Meals', 2), + buildExpenseReportTransaction('4', '2026-09-18', 'Travel', 3), + ]; + + beforeAll(() => { + Onyx.init({keys: ONYXKEYS}); + }); + + beforeEach(async () => { + jest.clearAllMocks(); + mockIsOffline.value = false; + mockUnifiedList.controller = undefined; + // The list only seeds the carousel while a transaction thread is open in the RHP. + mockFocusedRoute.value = {name: SCREENS.RIGHT_MODAL.SEARCH_REPORT, key: 'rhp'}; + await Onyx.clear(); + await clearActiveTransactionIDs(); + await Onyx.set(ONYXKEYS.NVP_PREFERRED_LOCALE, CONST.LOCALES.DEFAULT); + await waitForBatchedUpdates(); + }); + + afterEach(() => { + mockFocusedRoute.value = undefined; + }); + + it('seeds the arrows with the rendered row order, and re-seeds it when a column is sorted', async () => { + render(buildTransactionListElement(transactions, EXPENSE_REPORT_ID)); + await waitForBatchedUpdates(); + + // On first open the rows are grouped and the groups are alphabetical, so the arrows must follow that order + // rather than the flat date order the rows were sorted into. + const initialRenderedOrder = getRenderedTransactionIDs(); + expect(initialRenderedOrder).toEqual(['1', '3', '2', '4']); + expect(await getCarouselTransactionIDs()).toEqual(initialRenderedOrder); + + // When the user sorts by date descending, both the rows and the group headers move. + pressDateHeader(CONST.SEARCH.SORT_ORDER.DESC); + await waitForBatchedUpdates(); + + const sortedRenderedOrder = getRenderedTransactionIDs(); + // Grouping is still applied after the sort — Travel leads because it holds the newest expense — so this is + // neither the alphabetical group order nor a flat date sort. + expect(sortedRenderedOrder).toEqual(['4', '2', '3', '1']); + expect(await getCarouselTransactionIDs()).toEqual(sortedRenderedOrder); + }); + + // The component is reused across reportID changes, so a sort left over from the previous report would otherwise + // still be in effect — and would keep suppressing the RBR-first ordering the next report's first open gets. + it('drops the sort when a different report is opened in the same component', async () => { + const {rerender} = render(buildTransactionListElement(transactions, EXPENSE_REPORT_ID)); + await waitForBatchedUpdates(); + + pressDateHeader(CONST.SEARCH.SORT_ORDER.DESC); + await waitForBatchedUpdates(); + expect(getRenderedTransactionIDs()).toEqual(['4', '2', '3', '1']); + + rerender(buildTransactionListElement(transactions, 'expense2')); + await waitForBatchedUpdates(); + + // Back to the first-open order: date ascending with the groups alphabetical again. + expect(getRenderedTransactionIDs()).toEqual(['1', '3', '2', '4']); + }); +}); diff --git a/tests/unit/ReportLayoutUtilsTest.ts b/tests/unit/ReportLayoutUtilsTest.ts index 6575cfc49e60..af49de7a0b20 100644 --- a/tests/unit/ReportLayoutUtilsTest.ts +++ b/tests/unit/ReportLayoutUtilsTest.ts @@ -1,11 +1,11 @@ +import type {SortOrder} from '@components/Search/types'; + import {groupTransactionsByCategory, groupTransactionsByTag} from '@libs/ReportLayoutUtils'; import type {CompareLeadingTransactions} from '@libs/ReportLayoutUtils'; import CONST from '@src/CONST'; import type {Report, Transaction} from '@src/types/onyx'; -import type {ValueOf} from 'type-fest'; - import createMock from '../utils/createMock'; const mockLocaleCompare = (a: string, b: string) => a.localeCompare(b); @@ -31,7 +31,7 @@ const createMockReport = (overrides: Partial = {}): Report => // Stands in for the date comparator the transaction list builds from the active sort, so the groups can be checked // against the same ordering the rows use. const compareByCreated = - (sortOrder: ValueOf): CompareLeadingTransactions => + (sortOrder: SortOrder): CompareLeadingTransactions => (a, b) => { const result = (a.created ?? '').localeCompare(b.created ?? ''); return sortOrder === CONST.SEARCH.SORT_ORDER.ASC ? result : -result; @@ -560,7 +560,7 @@ describe('group ordering under an explicit sort', () => { // The RHP prev/next arrows walk the transaction IDs flat-mapped out of these groups, so the flattened groups have // to come back in the same order as the sorted rows or "next" would jump to a row the user isn't looking at. - it('flattens back into the sorted row order', () => { + it('flattens back into the rendered row order', () => { const report = createMockReport(); // The rows reach the grouping already sorted, so they are passed in newest first here as well. const transactions = [ @@ -572,6 +572,8 @@ describe('group ordering under an explicit sort', () => { const result = groupTransactionsByCategory(transactions, report, mockLocaleCompare, compareByCreated(CONST.SEARCH.SORT_ORDER.DESC)); + // Rendered order, not the raw sorted order: rows are bucketed by category, so Meals (4, 2) renders before + // Travel (3, 1). Changing this to ['4', '3', '2', '1'] would be asserting that grouping is bypassed. expect(result.flatMap((group) => group.transactions.map((transaction) => transaction.transactionID))).toEqual(['4', '2', '3', '1']); }); From 5b86f6172d93f09983e2c56c46f022978a745bd3 Mon Sep 17 00:00:00 2001 From: "Kevin Brian Bader (via MelvinBot)" Date: Fri, 18 Sep 2026 00:41:17 +0000 Subject: [PATCH 3/7] Run npm run fmt on the new RHP arrow order test file Co-authored-by: Kevin Brian Bader --- .../MoneyRequestReportTransactionsNavigationTest.tsx | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx b/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx index e0040470d3e3..33afa828d4d6 100644 --- a/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx +++ b/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx @@ -5,6 +5,7 @@ import {LocaleContextProvider} from '@components/LocaleContextProvider'; import MoneyRequestReportTransactionList from '@components/MoneyRequestReportView/MoneyRequestReportTransactionList'; import MoneyRequestReportTransactionsNavigation from '@components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation'; import OnyxListItemProvider from '@components/OnyxListItemProvider'; +import type * as SearchContext from '@components/Search/SearchContext'; import type {SortOrder} from '@components/Search/types'; import * as ReportActions from '@libs/actions/Report'; @@ -23,8 +24,6 @@ import React from 'react'; import {View} from 'react-native'; import Onyx from 'react-native-onyx'; -import type * as SearchContext from '@components/Search/SearchContext'; - import createRandomReportAction from '../utils/collections/reportActions'; import createRandomTransaction from '../utils/collections/transaction'; import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; @@ -70,7 +69,14 @@ jest.mock('@components/MoneyRequestReportView/MoneyRequestReportUnifiedList', () jest.mock('@hooks/useResponsiveLayout', () => ({ __esModule: true, - default: () => ({shouldUseNarrowLayout: false, isSmallScreenWidth: false, isMediumScreenWidth: false, isLargeScreenWidth: true, isExtraLargeScreenWidth: true, onboardingIsMediumOrLargerScreenWidth: true}), + default: () => ({ + shouldUseNarrowLayout: false, + isSmallScreenWidth: false, + isMediumScreenWidth: false, + isLargeScreenWidth: true, + isExtraLargeScreenWidth: true, + onboardingIsMediumOrLargerScreenWidth: true, + }), })); jest.mock('@hooks/useResponsiveLayoutOnWideRHP', () => ({ From 8130594df5ec4b5bae32e8ffe4a846af6f759c58 Mon Sep 17 00:00:00 2001 From: "Kevin Brian Bader (via MelvinBot)" Date: Fri, 18 Sep 2026 01:28:40 +0000 Subject: [PATCH 4/7] Reuse the exported controller and list-item types in the RHP arrow order test Co-authored-by: Kevin Brian Bader --- ...equestReportTransactionsNavigationTest.tsx | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx b/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx index 33afa828d4d6..58d198e44702 100644 --- a/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx +++ b/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx @@ -2,6 +2,7 @@ import {act, fireEvent, render, screen, waitFor} from '@testing-library/react-na import ComposeProviders from '@components/ComposeProviders'; import {LocaleContextProvider} from '@components/LocaleContextProvider'; +import type {MoneyRequestReportTransactionListController, TransactionListItemData} from '@components/MoneyRequestReportView/MoneyRequestReportTransactionList'; import MoneyRequestReportTransactionList from '@components/MoneyRequestReportView/MoneyRequestReportTransactionList'; import MoneyRequestReportTransactionsNavigation from '@components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation'; import OnyxListItemProvider from '@components/OnyxListItemProvider'; @@ -28,14 +29,8 @@ import createRandomReportAction from '../utils/collections/reportActions'; import createRandomTransaction from '../utils/collections/transaction'; import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; -/** The rendered list items the transaction list hands the unified list, in render order. */ -type CapturedListItem = {type: 'section-header'} | {type: 'transaction'; transaction: Transaction}; - /** The slice of the transaction list's controller the tests below read. */ -type CapturedController = { - tableColumnHeader: React.ReactElement<{onSortPress: (sortBy: string, sortOrder: SortOrder) => void}> | null; - transactionListItems: CapturedListItem[]; -}; +type CapturedController = Pick; // Drives the RHP-open check both the transaction list and the navigation component make through findFocusedRoute(). // Undefined reproduces the real module's behaviour while the navigation ref isn't ready, which is what the existing @@ -291,10 +286,16 @@ function buildTransactionListElement(transactions: Transaction[], reportID: stri } function pressDateHeader(sortOrder: SortOrder) { - const onSortPress = mockUnifiedList.controller?.tableColumnHeader?.props.onSortPress; - if (!onSortPress) { + // The controller types the header as a plain ReactElement, so the one prop these tests drive is narrowed here + // rather than by duplicating the controller's own type. + const columnHeader = mockUnifiedList.controller?.tableColumnHeader; + if (!React.isValidElement<{onSortPress?: (sortBy: string, sortOrder: SortOrder) => void}>(columnHeader)) { throw new Error('the sortable column header did not render'); } + const onSortPress = columnHeader.props.onSortPress; + if (!onSortPress) { + throw new Error('the sortable column header rendered without onSortPress'); + } act(() => { onSortPress(CONST.SEARCH.TABLE_COLUMNS.DATE, sortOrder); }); @@ -302,7 +303,7 @@ function pressDateHeader(sortOrder: SortOrder) { function getRenderedTransactionIDs(): string[] { const listItems = mockUnifiedList.controller?.transactionListItems ?? []; - return listItems.filter((item): item is {type: 'transaction'; transaction: Transaction} => item.type === 'transaction').map((item) => item.transaction.transactionID); + return listItems.filter((item): item is Extract => item.type === 'transaction').map((item) => item.transaction.transactionID); } async function getCarouselTransactionIDs(): Promise { From 19b60bbdc49f9bf9950cb62ebbe66ffc6323f894 Mon Sep 17 00:00:00 2001 From: "Kevin Brian Bader (via MelvinBot)" Date: Fri, 18 Sep 2026 22:45:27 +0000 Subject: [PATCH 5/7] Narrow to deleting the RBR hoist: keep group headers alphabetical and let the column sort stand alone Product narrowed the expected behaviour: the table just obeys the selected column (Date by default), and category/tag group order stays alphabetical as a separate axis. - Revert src/libs/ReportLayoutUtils.ts and tests/unit/ReportLayoutUtilsTest.ts to their pre-PR state; the group comparator and its 8 tests are no longer needed. - Delete the RBR hoist from the table sort along with isDefaultSort, hasUserSortedTransactions, the per-report sort reset and the rbrTransactionIDs memo. reportActionsMap only fed transactionHasRBR, so the report-actions pass now builds just the transaction-thread map, and getActionErrorsByTransaction over every report action is gone from the default report open. - Update the RHP arrow order test for the new Date DESC order and drop the per-report sort reset test. Co-authored-by: Kevin Brian Bader --- .../MoneyRequestReportTransactionList.tsx | 119 +++-------------- src/libs/ReportLayoutUtils.ts | 46 +------ ...equestReportTransactionsNavigationTest.tsx | 40 ++---- tests/unit/ReportLayoutUtilsTest.ts | 121 ------------------ 4 files changed, 35 insertions(+), 291 deletions(-) diff --git a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx index 8cdd851f90e9..4d83d589e9f0 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx @@ -26,9 +26,7 @@ import {navigationRef} from '@libs/Navigation/Navigation'; import {isPolicyTaxEnabled} from '@libs/PolicyUtils'; import {getOriginalMessage, isMoneyRequestAction} from '@libs/ReportActionsUtils'; import {groupTransactionsByCategory, groupTransactionsByTag} from '@libs/ReportLayoutUtils'; -import type {CompareLeadingTransactions} from '@libs/ReportLayoutUtils'; import { - getActionErrorsByTransaction, getMoneyRequestSpendBreakdown, getReportOfflinePendingActionAndErrors, getTransactionSortValue, @@ -39,7 +37,6 @@ import { import type {SortableColumnName} from '@libs/ReportUtils'; import {compareValues, getColumnsToShow, getTableMinWidth, isTransactionAmountTooLong, isTransactionTaxAmountTooLong} from '@libs/SearchUIUtils'; import {getPendingSubmitFollowUpAction} from '@libs/telemetry/submitFollowUpAction'; -import {transactionHasRBR} from '@libs/TransactionPreviewUtils'; import {getTransactionPendingAction, getVisibleTransactionViolations, hasNonReimbursableTransactions, isTransactionPendingDelete} from '@libs/TransactionUtils'; import shouldShowTransactionPostedYear from '@libs/TransactionUtils/shouldShowTransactionPostedYear'; import shouldShowTransactionYear from '@libs/TransactionUtils/shouldShowTransactionYear'; @@ -229,12 +226,6 @@ type SortedTransactions = { sortOrder: SortOrder; }; -/** Kept at module scope so resetting to it on a report change is a no-op re-render when the sort is already default. */ -const DEFAULT_SORT_CONFIG: SortedTransactions = { - sortBy: CONST.SEARCH.TABLE_COLUMNS.DATE, - sortOrder: CONST.SEARCH.SORT_ORDER.ASC, -}; - function MoneyRequestReportTransactionList({ report, transactions, @@ -374,37 +365,19 @@ function MoneyRequestReportTransactionList({ // eslint-disable-next-line react-hooks/exhaustive-deps }, [reportID]); - const [sortConfig, setSortConfig] = useState(DEFAULT_SORT_CONFIG); + const [sortConfig, setSortConfig] = useState({ + sortBy: CONST.SEARCH.TABLE_COLUMNS.DATE, + sortOrder: CONST.SEARCH.SORT_ORDER.ASC, + }); const {sortBy, sortOrder} = sortConfig; - // Date/ASC is both the initial state and where every second Date press lands, so pressing a column has to be - // tracked separately for an explicit sort to win over the RBR ordering below. - const [hasUserSortedTransactions, setHasUserSortedTransactions] = useState(false); - const isDefaultSort = !hasUserSortedTransactions && sortBy === CONST.SEARCH.TABLE_COLUMNS.DATE && sortOrder === CONST.SEARCH.SORT_ORDER.ASC; - - // This component is reused across reportID changes instead of being remounted (there is no key on the usage in - // MoneyRequestReportActionsList), which is why the selection above has to be cleared by hand. The sort is the - // same: without this reset, sorting one report would carry into the next report opened and suppress the RBR-first - // ordering that report's first open is supposed to get. Adjusted during render rather than in an effect, which is - // the pattern React recommends for resetting state on a prop change: it re-renders before anything is committed, - // so the new report never paints with the previous one's sort. - const [sortedReportID, setSortedReportID] = useState(reportID); - if (sortedReportID !== reportID) { - setSortedReportID(reportID); - setSortConfig(DEFAULT_SORT_CONFIG); - setHasUserSortedTransactions(false); - } - // In a single pass over reportActions, build: - // - reportActionsMap: keyed by reportActionID for transactionHasRBR. - // - transactionThreadReportIDByTransactionID: transactionID → transaction-thread report ID, so each row can pass it - // to the RBR, letting rows without RBR content early-return instead of mounting the heavy RBR inner (6 Onyx - // subscriptions). Without this, the per-row alternative would re-scan every report action (O(transactions × actions)). - const {reportActionsMap, transactionThreadReportIDByTransactionID} = useMemo(() => { - const actionsMap: Record = {}; + // transactionID → transaction-thread report ID, so each row can pass it to the RBR, letting rows without RBR + // content early-return instead of mounting the heavy RBR inner (6 Onyx subscriptions). Without this, the per-row + // alternative would re-scan every report action (O(transactions × actions)). + const transactionThreadReportIDByTransactionID = useMemo(() => { const threadReportIDByTransactionID = new Map(); for (const action of reportActions) { - actionsMap[action.reportActionID] = action; if (!isMoneyRequestAction(action)) { continue; } @@ -414,49 +387,21 @@ function MoneyRequestReportTransactionList({ threadReportIDByTransactionID.set(iouTransactionID, action.childReportID); } } - return {reportActionsMap: actionsMap, transactionThreadReportIDByTransactionID: threadReportIDByTransactionID}; + return threadReportIDByTransactionID; }, [reportActions]); - // Precompute the set of RBR-flagged transaction IDs - const rbrTransactionIDs = useMemo(() => { - if (!isDefaultSort || !allTransactionViolations) { - return null; - } - const login = currentUserDetails?.login ?? ''; - const accountID = currentUserDetails?.accountID ?? CONST.DEFAULT_NUMBER_ID; - // Precompute report-action errors once so each transaction's RBR check is an O(1) lookup instead of - // re-scanning every report action (O(transactions × actions)). - const actionErrors = getActionErrorsByTransaction(report?.reportID, reportActionsMap); - const ids = new Set(); - for (const transaction of transactions) { - const violations = allTransactionViolations[`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transaction.transactionID}`] ?? []; - if (transactionHasRBR(transaction, violations, login, accountID, report, ownerLogin, policy, reportActionsMap, actionErrors)) { - ids.add(transaction.transactionID); - } - } - return ids; - }, [isDefaultSort, allTransactionViolations, currentUserDetails?.login, currentUserDetails?.accountID, transactions, report, ownerLogin, policy, reportActionsMap]); - const sortedTransactions: TransactionWithOptionalHighlight[] = useMemo(() => { - return [...transactions].sort((a, b) => { - // When on default sort (Date/ASC), prioritize RBR-flagged transactions - if (rbrTransactionIDs) { - const aHasRBR = rbrTransactionIDs.has(a.transactionID); - const bHasRBR = rbrTransactionIDs.has(b.transactionID); - if (aHasRBR !== bHasRBR) { - return aHasRBR ? -1 : 1; - } - } - return compareValues( + return [...transactions].sort((a, b) => + compareValues( getTransactionSortValue(a, sortBy, report, policy, policyCategories, policyTagLists), getTransactionSortValue(b, sortBy, report, policy, policyCategories, policyTagLists), sortOrder, sortBy, localeCompare, true, - ); - }); - }, [sortBy, sortOrder, transactions, localeCompare, report, policy, policyCategories, policyTagLists, rbrTransactionIDs]); + ), + ); + }, [sortBy, sortOrder, transactions, localeCompare, report, policy, policyCategories, policyTagLists]); const resolvedTransactions = useMemo(() => resolveTransactionCardFields(sortedTransactions, cardList, translate), [sortedTransactions, cardList, translate]); @@ -519,43 +464,14 @@ function MoneyRequestReportTransactionList({ if (!shouldGroupTransactions) { return []; } - // Once the user presses a column the group headers follow that column too, otherwise the groups stay - // alphabetical and only the rows inside each group would be ordered. Built inside this memo so it inherits the - // narrowed report dependency below instead of pulling the whole report object back in as its own memo would. - const compareLeadingTransactions: CompareLeadingTransactions | undefined = hasUserSortedTransactions - ? (a, b) => - compareValues( - getTransactionSortValue(a, sortBy, report, policy, policyCategories, policyTagLists), - getTransactionSortValue(b, sortBy, report, policy, policyCategories, policyTagLists), - sortOrder, - sortBy, - localeCompare, - true, - ) - : undefined; if (currentGroupBy === CONST.REPORT_LAYOUT.GROUP_BY.TAG) { - return groupTransactionsByTag(resolvedTransactions, report, localeCompare, compareLeadingTransactions); + return groupTransactionsByTag(resolvedTransactions, report, localeCompare); } - return groupTransactionsByCategory(resolvedTransactions, report, localeCompare, compareLeadingTransactions); + return groupTransactionsByCategory(resolvedTransactions, report, localeCompare); // groupTransactionsByTag() and groupTransactionsByCategory() use the full report object to perform a null check. // We skip including the report as a dependency to avoid unnecessary re-renders as it changes often and we only need to recalculate when currency changes. - // The comparator reads report and policy fields too, but resolvedTransactions is derived from the row sort, - // which does depend on both in full, so any change to either already invalidates this memo through it. // eslint-disable-next-line react-hooks/exhaustive-deps - }, [ - resolvedTransactions, - currentGroupBy, - report?.reportID, - report?.currency, - localeCompare, - shouldGroupTransactions, - hasUserSortedTransactions, - sortBy, - sortOrder, - policy?.id, - policyCategories, - policyTagLists, - ]); + }, [resolvedTransactions, currentGroupBy, report?.reportID, report?.currency, localeCompare, shouldGroupTransactions]); const visualOrderTransactionIDs = useMemo(() => { if (!shouldGroupTransactions || groupedTransactions.length === 0) { @@ -867,7 +783,6 @@ function MoneyRequestReportTransactionList({ if (!isSortableColumnName(selectedSortBy)) { return; } - setHasUserSortedTransactions(true); setSortConfig((prevState) => ({...prevState, sortBy: selectedSortBy, sortOrder: selectedSortOrder})); }} dateColumnSize={dateColumnSize} diff --git a/src/libs/ReportLayoutUtils.ts b/src/libs/ReportLayoutUtils.ts index 054ac4f7fd9c..48491784ec68 100644 --- a/src/libs/ReportLayoutUtils.ts +++ b/src/libs/ReportLayoutUtils.ts @@ -10,32 +10,11 @@ import {getDecodedCategoryName, isCategoryMissing} from './CategoryUtils'; import {getDecodedTagName, isTagMissing} from './TagUtils'; import {getAmount, getCategory, getCurrency, getTag, isTransactionPendingDelete} from './TransactionUtils'; -/** Compares the leading (first rendered) transaction of two groups under the sort the user selected */ -type CompareLeadingTransactions = (a: Transaction, b: Transaction) => number; - /** - * Sorts groups alphabetically (A→Z) with empty keys at the end. - * When `compareLeadingTransactions` is passed, the groups follow the sorted column instead, so the group headers - * can't hold a group in place while the rows inside it move. Alphabetical order remains the tiebreak. + * Sorts groups alphabetically (A→Z) with empty keys at the end */ -function sortGroupedTransactions( - groups: GroupedTransactions[], - localeCompare: LocaleContextProps['localeCompare'], - compareLeadingTransactions?: CompareLeadingTransactions, -): GroupedTransactions[] { - return [...groups].sort((a, b) => { - if (compareLeadingTransactions) { - const leadingA = a.transactions.at(0); - const leadingB = b.transactions.at(0); - // Defensive only: the grouping functions below create a group at the moment they push a transaction into - // it, so a group is never empty and this guard never falls through to the alphabetical order in practice. - if (leadingA && leadingB) { - const result = compareLeadingTransactions(leadingA, leadingB); - if (result !== 0) { - return result; - } - } - } +function sortGroupedTransactions(groups: GroupedTransactions[], localeCompare: LocaleContextProps['localeCompare']): GroupedTransactions[] { + return groups.sort((a, b) => { if (a.groupKey === '' && b.groupKey !== '') { return 1; } @@ -78,12 +57,7 @@ function calculateGroupTotal(transactionList: Transaction[], reportCurrency: str /** * Groups transactions by category */ -function groupTransactionsByCategory( - transactions: Transaction[], - report: OnyxEntry, - localeCompare: LocaleContextProps['localeCompare'], - compareLeadingTransactions?: CompareLeadingTransactions, -): GroupedTransactions[] { +function groupTransactionsByCategory(transactions: Transaction[], report: OnyxEntry, localeCompare: LocaleContextProps['localeCompare']): GroupedTransactions[] { if (!report) { return []; } @@ -112,18 +86,13 @@ function groupTransactionsByCategory( }); } - return sortGroupedTransactions(result, localeCompare, compareLeadingTransactions); + return sortGroupedTransactions(result, localeCompare); } /** * Groups transactions by tag */ -function groupTransactionsByTag( - transactions: Transaction[], - report: OnyxEntry, - localeCompare: LocaleContextProps['localeCompare'], - compareLeadingTransactions?: CompareLeadingTransactions, -): GroupedTransactions[] { +function groupTransactionsByTag(transactions: Transaction[], report: OnyxEntry, localeCompare: LocaleContextProps['localeCompare']): GroupedTransactions[] { if (!report) { return []; } @@ -152,8 +121,7 @@ function groupTransactionsByTag( }); } - return sortGroupedTransactions(result, localeCompare, compareLeadingTransactions); + return sortGroupedTransactions(result, localeCompare); } export {groupTransactionsByCategory, groupTransactionsByTag}; -export type {CompareLeadingTransactions}; diff --git a/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx b/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx index 58d198e44702..c6fb65fd4cc1 100644 --- a/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx +++ b/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx @@ -319,13 +319,13 @@ async function getCarouselTransactionIDs(): Promise { } // The RHP prev/next arrows walk the transaction IDs this list seeds, so the seed has to stay equal to the rows the -// user is actually looking at. Sorting is the case that breaks it: the rows move, and if the seed is built from -// anything other than the rendered groups (a flattened list, or groups left in alphabetical order) "next" jumps to a -// row that isn't below the current one on screen. +// user is actually looking at. Sorting is the case that breaks it: the rows move within their groups, and if the seed +// is built from anything other than the rendered groups (a flat date sort, say) "next" jumps to a row that isn't +// below the current one on screen. describe('MoneyRequestReportTransactionList - RHP arrow order', () => { - // The newest expense is in the alphabetically-last category, so date order and alphabetical group order disagree: - // under Date DESC the groups must come back Travel then Meals, which is the opposite of the alphabetical order the - // groups fall back to when no column has been pressed. + // The newest expense is in the alphabetically-last category, so a flat date sort and the rendered order disagree: + // under Date DESC the newest expense (4, Travel) is not the first row, because the alphabetical Meals group still + // renders first. const transactions = [ buildExpenseReportTransaction('1', '2026-09-15', 'Meals', 0), buildExpenseReportTransaction('2', '2026-09-16', 'Travel', 1), @@ -357,37 +357,19 @@ describe('MoneyRequestReportTransactionList - RHP arrow order', () => { render(buildTransactionListElement(transactions, EXPENSE_REPORT_ID)); await waitForBatchedUpdates(); - // On first open the rows are grouped and the groups are alphabetical, so the arrows must follow that order - // rather than the flat date order the rows were sorted into. + // Date ascending, bucketed into alphabetical groups: Meals (1, 3) then Travel (2, 4). The arrows must follow + // that rendered order rather than the flat date order the rows were sorted into. const initialRenderedOrder = getRenderedTransactionIDs(); expect(initialRenderedOrder).toEqual(['1', '3', '2', '4']); expect(await getCarouselTransactionIDs()).toEqual(initialRenderedOrder); - // When the user sorts by date descending, both the rows and the group headers move. pressDateHeader(CONST.SEARCH.SORT_ORDER.DESC); await waitForBatchedUpdates(); const sortedRenderedOrder = getRenderedTransactionIDs(); - // Grouping is still applied after the sort — Travel leads because it holds the newest expense — so this is - // neither the alphabetical group order nor a flat date sort. - expect(sortedRenderedOrder).toEqual(['4', '2', '3', '1']); + // The rows reverse inside each group while the group headers stay alphabetical, so Meals (3, 1) still renders + // before Travel (4, 2). Group order is a separate axis from the column sort and is deliberately unaffected. + expect(sortedRenderedOrder).toEqual(['3', '1', '4', '2']); expect(await getCarouselTransactionIDs()).toEqual(sortedRenderedOrder); }); - - // The component is reused across reportID changes, so a sort left over from the previous report would otherwise - // still be in effect — and would keep suppressing the RBR-first ordering the next report's first open gets. - it('drops the sort when a different report is opened in the same component', async () => { - const {rerender} = render(buildTransactionListElement(transactions, EXPENSE_REPORT_ID)); - await waitForBatchedUpdates(); - - pressDateHeader(CONST.SEARCH.SORT_ORDER.DESC); - await waitForBatchedUpdates(); - expect(getRenderedTransactionIDs()).toEqual(['4', '2', '3', '1']); - - rerender(buildTransactionListElement(transactions, 'expense2')); - await waitForBatchedUpdates(); - - // Back to the first-open order: date ascending with the groups alphabetical again. - expect(getRenderedTransactionIDs()).toEqual(['1', '3', '2', '4']); - }); }); diff --git a/tests/unit/ReportLayoutUtilsTest.ts b/tests/unit/ReportLayoutUtilsTest.ts index af49de7a0b20..87e46eb30f2c 100644 --- a/tests/unit/ReportLayoutUtilsTest.ts +++ b/tests/unit/ReportLayoutUtilsTest.ts @@ -1,7 +1,4 @@ -import type {SortOrder} from '@components/Search/types'; - import {groupTransactionsByCategory, groupTransactionsByTag} from '@libs/ReportLayoutUtils'; -import type {CompareLeadingTransactions} from '@libs/ReportLayoutUtils'; import CONST from '@src/CONST'; import type {Report, Transaction} from '@src/types/onyx'; @@ -28,15 +25,6 @@ const createMockReport = (overrides: Partial = {}): Report => ...overrides, }) as Report; -// Stands in for the date comparator the transaction list builds from the active sort, so the groups can be checked -// against the same ordering the rows use. -const compareByCreated = - (sortOrder: SortOrder): CompareLeadingTransactions => - (a, b) => { - const result = (a.created ?? '').localeCompare(b.created ?? ''); - return sortOrder === CONST.SEARCH.SORT_ORDER.ASC ? result : -result; - }; - describe('groupTransactionsByCategory', () => { it('returns empty array when report is undefined', () => { const transactions = [createMockTransaction({category: 'Travel'})]; @@ -480,112 +468,3 @@ describe('groupTransactionsByTag', () => { expect(result.at(0)?.subTotalAmount).toBe(3000); }); }); - -describe('group ordering under an explicit sort', () => { - it('keeps groups alphabetical with the empty key last when no comparator is passed', () => { - const report = createMockReport(); - const transactions = [ - createMockTransaction({transactionID: '1', category: '', created: '2026-09-01'}), - createMockTransaction({transactionID: '2', category: 'Zebra', created: '2026-09-02'}), - createMockTransaction({transactionID: '3', category: 'Alpha', created: '2026-09-03'}), - ]; - - const result = groupTransactionsByCategory(transactions, report, mockLocaleCompare); - - expect(result.map((group) => group.groupKey)).toEqual(['Alpha', 'Zebra', '']); - }); - - it('orders groups by their leading transaction when sorting by date ascending', () => { - const report = createMockReport(); - const transactions = [ - createMockTransaction({transactionID: '1', category: 'Travel', created: '2026-09-15'}), - createMockTransaction({transactionID: '2', category: 'Meals', created: '2026-09-17'}), - createMockTransaction({transactionID: '3', category: 'Advertising', created: '2026-09-20'}), - ]; - - const result = groupTransactionsByCategory(transactions, report, mockLocaleCompare, compareByCreated(CONST.SEARCH.SORT_ORDER.ASC)); - - expect(result.map((group) => group.groupKey)).toEqual(['Travel', 'Meals', 'Advertising']); - }); - - it('orders groups by their leading transaction when sorting by date descending', () => { - const report = createMockReport(); - const transactions = [ - createMockTransaction({transactionID: '1', category: 'Advertising', created: '2026-09-20'}), - createMockTransaction({transactionID: '2', category: 'Meals', created: '2026-09-17'}), - createMockTransaction({transactionID: '3', category: 'Travel', created: '2026-09-15'}), - ]; - - const result = groupTransactionsByCategory(transactions, report, mockLocaleCompare, compareByCreated(CONST.SEARCH.SORT_ORDER.DESC)); - - expect(result.map((group) => group.groupKey)).toEqual(['Advertising', 'Meals', 'Travel']); - }); - - it('lets the empty key group leave the bottom when the sort puts it first', () => { - const report = createMockReport(); - const transactions = [ - createMockTransaction({transactionID: '1', category: '', created: '2026-09-01'}), - createMockTransaction({transactionID: '2', category: 'Travel', created: '2026-09-10'}), - ]; - - const result = groupTransactionsByCategory(transactions, report, mockLocaleCompare, compareByCreated(CONST.SEARCH.SORT_ORDER.ASC)); - - expect(result.map((group) => group.groupKey)).toEqual(['', 'Travel']); - }); - - it('falls back to alphabetical order when the leading transactions tie', () => { - const report = createMockReport(); - const transactions = [ - createMockTransaction({transactionID: '1', category: 'Zebra', created: '2026-09-10'}), - createMockTransaction({transactionID: '2', category: 'Alpha', created: '2026-09-10'}), - createMockTransaction({transactionID: '3', category: '', created: '2026-09-10'}), - ]; - - const result = groupTransactionsByCategory(transactions, report, mockLocaleCompare, compareByCreated(CONST.SEARCH.SORT_ORDER.ASC)); - - expect(result.map((group) => group.groupKey)).toEqual(['Alpha', 'Zebra', '']); - }); - - it('orders tag groups by their leading transaction too', () => { - const report = createMockReport(); - const transactions = [ - createMockTransaction({transactionID: '1', tag: 'Project Z', created: '2026-09-15'}), - createMockTransaction({transactionID: '2', tag: 'Project A', created: '2026-09-20'}), - ]; - - const result = groupTransactionsByTag(transactions, report, mockLocaleCompare, compareByCreated(CONST.SEARCH.SORT_ORDER.ASC)); - - expect(result.map((group) => group.groupKey)).toEqual(['Project Z', 'Project A']); - }); - - // The RHP prev/next arrows walk the transaction IDs flat-mapped out of these groups, so the flattened groups have - // to come back in the same order as the sorted rows or "next" would jump to a row the user isn't looking at. - it('flattens back into the rendered row order', () => { - const report = createMockReport(); - // The rows reach the grouping already sorted, so they are passed in newest first here as well. - const transactions = [ - createMockTransaction({transactionID: '4', category: 'Meals', created: '2026-09-18'}), - createMockTransaction({transactionID: '3', category: 'Travel', created: '2026-09-17'}), - createMockTransaction({transactionID: '2', category: 'Meals', created: '2026-09-16'}), - createMockTransaction({transactionID: '1', category: 'Travel', created: '2026-09-15'}), - ]; - - const result = groupTransactionsByCategory(transactions, report, mockLocaleCompare, compareByCreated(CONST.SEARCH.SORT_ORDER.DESC)); - - // Rendered order, not the raw sorted order: rows are bucketed by category, so Meals (4, 2) renders before - // Travel (3, 1). Changing this to ['4', '3', '2', '1'] would be asserting that grouping is bypassed. - expect(result.flatMap((group) => group.transactions.map((transaction) => transaction.transactionID))).toEqual(['4', '2', '3', '1']); - }); - - it('does not reorder the transactions array it was given', () => { - const report = createMockReport(); - const transactions = [ - createMockTransaction({transactionID: '1', category: 'Zebra', created: '2026-09-20'}), - createMockTransaction({transactionID: '2', category: 'Alpha', created: '2026-09-15'}), - ]; - - groupTransactionsByCategory(transactions, report, mockLocaleCompare, compareByCreated(CONST.SEARCH.SORT_ORDER.ASC)); - - expect(transactions.map((transaction) => transaction.transactionID)).toEqual(['1', '2']); - }); -}); From d35a8b88d4d63eff4958a233d5b1e9b7540b74d4 Mon Sep 17 00:00:00 2001 From: "Kevin Brian Bader (via MelvinBot)" Date: Sat, 19 Sep 2026 00:13:28 +0000 Subject: [PATCH 6/7] Run npm run fmt: collapse the ReportUtils import now that it fits on one line Removing getActionErrorsByTransaction left the remaining six named imports short enough for oxfmt to fold the multi-line form back onto a single line. Co-authored-by: Kevin Brian Bader --- .../MoneyRequestReportTransactionList.tsx | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx index 4d83d589e9f0..b898e30f019c 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx @@ -26,14 +26,7 @@ import {navigationRef} from '@libs/Navigation/Navigation'; import {isPolicyTaxEnabled} from '@libs/PolicyUtils'; import {getOriginalMessage, isMoneyRequestAction} from '@libs/ReportActionsUtils'; import {groupTransactionsByCategory, groupTransactionsByTag} from '@libs/ReportLayoutUtils'; -import { - getMoneyRequestSpendBreakdown, - getReportOfflinePendingActionAndErrors, - getTransactionSortValue, - isExpenseReport, - isIOUReport, - isSortableColumnName, -} from '@libs/ReportUtils'; +import {getMoneyRequestSpendBreakdown, getReportOfflinePendingActionAndErrors, getTransactionSortValue, isExpenseReport, isIOUReport, isSortableColumnName} from '@libs/ReportUtils'; import type {SortableColumnName} from '@libs/ReportUtils'; import {compareValues, getColumnsToShow, getTableMinWidth, isTransactionAmountTooLong, isTransactionTaxAmountTooLong} from '@libs/SearchUIUtils'; import {getPendingSubmitFollowUpAction} from '@libs/telemetry/submitFollowUpAction'; From bd1ebd6cb25dae34e0226e870b8c3b446611a277 Mon Sep 17 00:00:00 2001 From: "Kevin Brian Bader (via MelvinBot)" Date: Sat, 19 Sep 2026 00:32:52 +0000 Subject: [PATCH 7/7] Address CONSISTENCY-15: write the two flagged comments as plain sentences - Spell out the transactionID to transaction-thread-report-ID mapping in words instead of using an arrow. - Split the FlashList comment's semicolon into two sentences. Co-authored-by: Kevin Brian Bader --- .../MoneyRequestReportTransactionList.tsx | 6 +++--- tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx index b898e30f019c..f649e4aeb6fa 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx @@ -365,9 +365,9 @@ function MoneyRequestReportTransactionList({ const {sortBy, sortOrder} = sortConfig; - // transactionID → transaction-thread report ID, so each row can pass it to the RBR, letting rows without RBR - // content early-return instead of mounting the heavy RBR inner (6 Onyx subscriptions). Without this, the per-row - // alternative would re-scan every report action (O(transactions × actions)). + // Maps each transactionID to its transaction-thread report ID, so each row can pass it to the RBR, letting rows + // without RBR content early-return instead of mounting the heavy RBR inner (6 Onyx subscriptions). Without this, + // the per-row alternative would re-scan every report action (O(transactions x actions)). const transactionThreadReportIDByTransactionID = useMemo(() => { const threadReportIDByTransactionID = new Map(); for (const action of reportActions) { diff --git a/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx b/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx index c6fb65fd4cc1..533a314e0765 100644 --- a/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx +++ b/tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx @@ -52,7 +52,7 @@ jest.mock('@react-navigation/native', () => { }; }); -// The unified list is a FlashList; the tests below only need the controller it is handed, which carries the rendered +// The unified list is a FlashList. The tests below only need the controller it is handed, which carries the rendered // row order and the real column-header element. jest.mock('@components/MoneyRequestReportView/MoneyRequestReportUnifiedList', () => ({ __esModule: true,