Conversation
…tical group order
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
| 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); |
There was a problem hiding this comment.
🟠 hasUserSortedTransactions is never reset, so sorting one report can suppress RBR-first ordering on the next report you open. The flag is plain useState with no reset path. The component is reused across reportID changes rather than remounted, which is exactly why :364-368 exists:
useEffect(() => {
clearSelectedTransactions(true);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [reportID]);If selections had to be cleared manually on reportID change, hasUserSortedTransactions does too. There is also no key on the usage at MoneyRequestReportActionsList.tsx:555.
Concrete failure: open report A, press Date twice so you are back on Date ↑, navigate to report B which has violation-flagged expenses. On main, isDefaultSort is true for B and the RBR expenses surface first. With this PR, hasUserSortedTransactions is still true, so B opens in plain chronological order and the RBR expenses are buried.
That directly contradicts the contract agreed in the issue, "RBR-first on first open stays", and it is the exact failure mode #85553 was opened to fix.
useEffect(() => {
clearSelectedTransactions(true);
setHasUserSortedTransactions(false);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [reportID]);The reset is cheap and correct either way.
There was a problem hiding this comment.
Fixed — you're right, and the leak is a bit wider than the flag.
sortConfig was never reset either, so on main sorting report A to Date ↓ already carried into report B and suppressed its RBR-first open. Resetting only hasUserSortedTransactions would also have left B in an incoherent state: rows in A's sort order, group headers back to alphabetical. So both reset together, giving the next report a real first open.
src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx:392
One deviation from your snippet: putting it in the [reportID] effect trips react-hooks/set-state-in-effect, which is an error in npm run lint. It's the render-time reset pattern instead, so the new report never paints with the old report's sort.
Covered by tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx:372 — it fails without the reset.
|
|
||
| // 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(() => { |
There was a problem hiding this comment.
🟠 compareLeadingTransactions re-introduces the full report and policy objects into groupedTransactions, defeating the optimization the comment right below it describes. The existing memo deliberately narrows to report?.reportID and report?.currency:
// 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 new comparator memo depends on report and policy wholesale, and is itself now a dependency of groupedTransactions.
Every time the report object changes identity, which that comment says happens often, compareLeadingTransactions gets a new reference, which invalidates groupedTransactions, which re-runs groupTransactionsByCategory over every transaction plus calculateGroupTotal per group. The narrowing is bypassed.
getTransactionSortValue (ReportUtils.ts:14110-14160) only reads isExpenseReport(report) and report?.currency from the report, so the same narrowing applies. Match the convention already in the file:
}, [hasUserSortedTransactions, sortBy, sortOrder, report?.reportID, report?.currency, policy?.id, policyCategories, policyTagLists, localeCompare]);with the same eslint-disable-next-line react-hooks/exhaustive-deps and a one-line reason, or build the comparator inside the groupedTransactions memo so it inherits the narrowed list.
Without this, a large report re-buckets and re-totals on unrelated report updates such as an incoming comment or a status change.
There was a problem hiding this comment.
Changed — comparator now lives inside the groupedTransactions memo, so it inherits the narrowed dependency list instead of adding a second memo keyed on the whole report.
src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx:525
One correction on the impact, since it changes how the fix should be read: the narrowing on groupedTransactions was already bypassed before this PR. sortedTransactions depends on the full report and policy at MoneyRequestReportTransactionList.tsx:440, resolvedTransactions derives from it, and groupedTransactions depends on resolvedTransactions. So an incoming comment re-buckets and re-totals on main too — the comparator memo wasn't adding that.
That's also why the narrowed list can't go stale here: any report or policy change already invalidates this memo through resolvedTransactions. Worth noting getTransactionSortValue reads more than isExpenseReport(report) and report?.currency — getReportCustomColumnValue(key, report) for the submitter and deal-number columns — so report?.reportID alone wouldn't be safe without that upstream dependency.
| if (compareLeadingTransactions) { | ||
| const leadingA = a.transactions.at(0); | ||
| const leadingB = b.transactions.at(0); | ||
| if (leadingA && leadingB) { |
There was a problem hiding this comment.
🟡 The leadingA && leadingB guard silently falls through to alphabetical for an empty group. Correct in practice since groupTransactionsByCategory only creates a group when it pushes a transaction, so a group is never empty. Worth a short comment saying so, otherwise a future reader cannot tell whether the guard is defensive or load-bearing.
There was a problem hiding this comment.
Added — the guard is now labelled as defensive, with the reason a group is never empty.
src/libs/ReportLayoutUtils.ts:30
| @@ -25,6 +28,15 @@ const createMockReport = (overrides: Partial<Report> = {}): Report => | |||
| ...overrides, | |||
| }) as Report; | |||
|
|
|||
There was a problem hiding this comment.
🟠 The promised RHP arrow regression test is missing. The approved proposal committed to it explicitly:
Covered by a regression assertion in
tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx: sort, then assert the arrow order equals the rendered row order.
That file exists in the repo but is not in this diff. What shipped instead is a pure-function test in ReportLayoutUtilsTest.ts, which never exercises visualOrderTransactionIDs, setActiveTransactionIDs, or the navigation component. The arrow-order guarantee is the main argument for threading the sort rather than flattening, so it is the thing most worth protecting from a future refactor.
There was a problem hiding this comment.
Added — tests/ui/MoneyRequestReportTransactionsNavigationTest.tsx:349, in the file the proposal named.
It renders the real MoneyRequestReportTransactionList (the unified list is stubbed so the controller it hands down is readable), presses the Date header through the real onSortPress, and asserts the seeded carousel equals the rendered rows. visualOrderTransactionIDs, the real setActiveTransactionIDs, and the TRANSACTION_THREAD_NAVIGATION_TRANSACTION_IDS key the arrows read are all in the path — nothing about the order is recomputed in the test.
Fixture: four expenses across Meals and Travel, with the newest expense in the alphabetically-last category, so the three candidate orders are all distinct:
| Rendered order | |
|---|---|
| First open (Date ↑, groups alphabetical) | 1, 3, 2, 4 |
| Date ↓ with group ordering | 4, 2, 3, 1 |
| Date ↓ flattened (the bug being guarded) | 4, 3, 2, 1 |
Mutation-checked both ways — the test fails when compareLeadingTransactions is forced to undefined, and when visualOrderTransactionIDs is made to flatten sortedTransactions instead of the groups.
|
|
||
| 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']); |
There was a problem hiding this comment.
🟡 The test named flattens back into the sorted row order does not assert that, and the name is actively misleading. Input rows are ['4', '3', '2', '1'] (already sorted DESC). The assertion is:
expect(result.flatMap((group) => group.transactions.map((transaction) => transaction.transactionID))).toEqual(['4', '2', '3', '1']);['4','2','3','1'] is not the sorted row order, it is the rendered order once rows are bucketed by category. The assertion is right and the behaviour is correct, but the name claims the opposite. A future contributor reading the name could "fix" the expectation to ['4','3','2','1'] and break grouping. Rename to flattens back into the rendered row order and keep the existing comment explaining why the arrows depend on it.
There was a problem hiding this comment.
Renamed to flattens back into the rendered row order, and added a line at the assertion saying ['4', '3', '2', '1'] would be asserting grouping is bypassed — so the trap you described is called out where someone would fall into it.
tests/unit/ReportLayoutUtilsTest.ts:563
| // 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<typeof CONST.SEARCH.SORT_ORDER>): CompareLeadingTransactions => |
There was a problem hiding this comment.
🟡 ValueOf<typeof CONST.SEARCH.SORT_ORDER> re-derives a type that already exists in production code. SortOrder is exported from @components/Search/types and is what the sibling component uses (MoneyRequestReportTableHeaderRow.tsx:4).
Re-deriving it in a test also pulls type-fest into the file for no reason. Import the real one so the test moves with the production type if it ever changes:
import type {SortOrder} from '@components/Search/types';
const compareByCreated = (sortOrder: SortOrder): CompareLeadingTransactions => ...There was a problem hiding this comment.
Done — imports SortOrder from @components/Search/types, and type-fest is out of the file.
tests/unit/ReportLayoutUtilsTest.ts:33
|
@MelvinBot Address the 6 code-comment that I posted above. |
|
🤖 Web test steps verification results: All six behavioural verifications (1–6) pass on dev NewDot web: the RBR-first hoist is preserved on first open, both Date-header presses produce true chronological/reverse-chronological order with no hoist re-applied, group headers order by date rather than alphabetically, and the RHP prev/next arrows follow the visible row order. Verification 7 (no JS console errors) could not be executed because console capture is not supported by agent-device on web. Fixture: workspace
One wording nuance on step 1: with |
… the grouping memo, add the RHP arrow regression test
|
All six addressed and pushed in
Two things worth your attention: On #2, the performance regression as described was already happening before this PR. On #1, the reset runs during render rather than in the The new regression test and how it was validated
The fixture puts the newest expense in the alphabetically-last category, so the three candidate orders are distinct: first open Mutation-checked: it fails when Checks run locally on this branch
|
Co-authored-by: Kevin Brian Bader <ikevin127@users.noreply.github.com>
|
Fixed and pushed as
No source or test logic changed. The one check still red is Verification
I didn't run browser verification for this one, since a formatting-only change to a test file has no UI behavior to check. |
| /** 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[]; | ||
| }; |
There was a problem hiding this comment.
@MelvinBot 🟡 New finding in the UI test: two locally-declared types duplicate exported production types.
type CapturedListItem = {type: 'section-header'} | {type: 'transaction'; transaction: Transaction};
type CapturedController = {tableColumnHeader: ...; transactionListItems: CapturedListItem[]};MoneyRequestReportTransactionList.tsx:938 already exports both TransactionListItemData and MoneyRequestReportTransactionListController, and MoneyRequestReportUnifiedList.tsx:17 imports exactly those.
The test also narrows transaction to Transaction where production uses TransactionWithOptionalHighlight (also exported), so the local copy will silently drift if the production shape changes.
Should be Pick<MoneyRequestReportTransactionListController, 'tableColumnHeader' | 'transactionListItems'>.
…der test Co-authored-by: Kevin Brian Bader <ikevin127@users.noreply.github.com>
|
Fixed in
One deviation from the literal suggestion: the controller types Checks
|
2 participants
Explanation of Change
An explicit column press now wins over both the RBR ordering and the alphabetical group order, so the Date header produces a real chronological sort while RBR-flagged expenses still surface on first open.
Two mechanisms overrode the date sort. The comparator itself was correct.
isDefaultSortwas exactlyDate+ASC, which is both the initial state and where every second Date press lands, so the RBR hoist re-applied itself and ascending never came out chronological. AhasUserSortedTransactionsflag, set inonSortPress, is folded straight intoisDefaultSortrather than carried as a second condition that could drift from it.sortGroupedTransactionsordered the group headers by name with no reference to the active sort, and the rendered list is built from the groups, so date order only survived inside each category block. It now takes an optionalcompareLeadingTransactionscomparator, whichgroupTransactionsByCategoryandgroupTransactionsByTagforward. The rows reach the grouping already sorted, so each group's first row is its leading row under the active sort and reusing the row comparator gives coherent group ordering: Date ASC orders groups by earliest date, Date DESC by latest, text columns stay effectively alphabetical. The empty-key rule becomes the tiebreak instead of a hard pin, so Uncategorized can leave the bottom when the sort puts it first.Grouping is never bypassed, so the user's server-backed
NVP_REPORT_LAYOUT_GROUP_BYchoice is untouched. That also keeps the RHP prev/next arrows on the visible row order:visualOrderTransactionIDsflat-mapsgroupedTransactionswhenever grouping is active, so changing the group order rather than flattening keeps the arrow order identical to the rendered rows.sortGroupedTransactionsalso no longer sorts its argument in place.The report preview carousel keeps its own RBR-first ordering, and narrow layouts have no sortable column header, so both are unchanged.
Behaviour agreed in the linked issue and confirmed internally: RBR-first on first open stays, the table then follows the selected column, and the arrows follow the table.
AI Tests
Run locally by MelvinBot on this branch:
npm test -- tests/unit/ReportLayoutUtilsTest.ts— passed (42 tests, including 8 new group-ordering cases)npm testfor the related suitesMoneyRequestReportTransactionListActiveTransactionIDsTest,MoneyRequestReportTransactionsNavigationTest,MoneyRequestReportGroupHeaderTest,MoneyRequestReportViewTest,MoneyRequestReportActionsListRejectModalTest,MoneyRequestReportTransactionItemRejectErrorTest— all passednpm run typecheck— passednpm run lint-changed— passednpm run spell-changed— passed (3 files, 0 issues)npm run react-compiler-compliance-check check <MoneyRequestReportTransactionList.tsx>— fails identically onmain(pre-existing "missing/extra memoization dependencies"), so no regression; this file is not compiler-memoized, which is why the newuseMemois manual like the ones around itBrowser verification could not be run: the handed-off web session rendered a blank page for the whole run (0 accessibility nodes, no network activity), so no UI screenshots are attached.
Fixed Issues
$ #101424
PROPOSAL: #101424 (comment)
Tests
// TODO: The human co-author must fill out the tests they ran before marking this PR as "ready for review".
// Please describe what tests you performed that validate your change worked.
Offline tests
// TODO: The human co-author must fill out the offline tests they ran before marking this PR as "ready for review".
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
// TODO: The human co-author must fill out the QA steps before marking this PR as "ready for review". Please describe what QA needs to do to validate these changes and which areas they need to check for regressions.
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari