refactor: derive a group's rows once, by the list that renders them by TaduJR · Pull Request #98716 · Expensify/App

TaduJR · 2026-08-15T14:15:33Z

Explanation of Change

A group's rows were being worked out three times over. TransactionGroupListItem, GroupChildrenContent and GroupHeader each ran getSections() on the group's snapshot, with slightly different inputs. useSearchSnapshot already does that work and puts the result on group.transactions, so all three were redoing what the list had already handed them.

They also disagreed. The header built its rows without the policy or the report action, so a group selected from its header came out with different hold, reject, split and change report flags than the same rows selected any other way.

All three now read group.transactions. A new test writes a snapshot and records every render, showing it reaches the row and the list that renders it at the same time. That is what makes dropping the extra work safe.

Three props existed only to feed those calls: bankAccountList, cardFeeds and conciergeReportID. They are gone, and so are the three Onyx subscriptions in ExpenseGroupedSearchView that supplied them. That view renders the whole list, so it no longer re-renders when a card feed changes.

Whether a row is checked was duplicated in the same way. There were three versions and they disagreed about rows being deleted and about rows taken back out of a wider selection. There is one now, and the row, the group header and the children container all ask it.

Fixed Issues

$ #90539
PROPOSAL: #90539 (comment)

Tests

Setup (do once)

Use a workspace with at least: one category (e.g. Advertising) holding 3+ expenses across 2+ reports, a second category holding exactly 1 expense, and one report with no expenses. At least one expense should be on hold and one should be a card expense.

Test 1 — Grouped expenses still show the right rows (wide screen, web)

Open Search → Expenses, maximize the browser window.
Set Group by: Category.
Verify each group header shows the correct category name, expense count and total.
Click a group header's chevron to expand it.
Verify the expenses listed inside are exactly the expenses in that category, with the correct merchant, date, amount, category, tag and card columns — nothing blank, missing or duplicated.
Repeat steps 2–5 for Group by: Merchant, Tag, From, Card and Month.

Test 2 — Group checkbox reflects its expenses (wide screen, web)

With a group expanded, tick one expense inside it.
Verify the group header checkbox shows the partially-selected (dash) state.
Tick the remaining expenses in that group.
Verify the group header checkbox becomes fully checked.
Untick one expense.
Verify the header returns to the dash state.
Tick the group header checkbox itself.
Verify every expense inside the group becomes ticked, and the count in the top selection bar matches the number of expenses in that group.
Untick the group header checkbox and verify every expense inside is unticked and the selection bar clears.

Test 3 — Same behavior on narrow screens

Shrink the browser window (or use mWeb / iOS / Android).
Repeat Test 2 in the narrow layout.
Verify the group checkbox behaves identically to the wide layout — checked, dash and unchecked appear at the same moments in both.

Test 4 — Bulk actions after selecting from a group header

On wide web, tick a group header checkbox (do not tick individual expenses).
Verify the buttons offered in the selection bar (Hold, Unhold, Reject, Split, Change report, Delete) are the same ones offered when you instead tick every expense in that group individually.
Perform Hold on a header-selected group and verify it succeeds and the expenses show as held.

This is the main fix: previously a header selection produced different available actions than selecting the same expenses one by one.

Test 5 — Select all matching items

Open Search → Expenses, Group by: Category.
Tick any expense, then click Select all matching items in the selection bar.
Verify every group header and every visible expense reads as checked.
Untick one expense inside a group.
Verify that expense is unticked, its group header drops to the dash state, and every other group stays checked.
Untick a whole group header.
Verify only that group unchecks and the rest stay checked.
Re-tick that group header and verify it and its expenses return to checked.

Test 6 — Groups that haven't loaded yet, and empty reports

Group by: Merchant on a search with many groups; scroll so a group appears before you expand it.
Tick that collapsed group's checkbox.
Expand it.
Verify every expense inside appears ticked, and the selection-bar count updates to the number of expenses in that group.
Untick the group header and verify the expenses inside all untick.
Open Search → Reports and tick a report that has no expenses.
Verify it ticks and unticks normally, and the selection bar count is correct.

Test 7 — Reports view row selection

Open Search → Reports.
Tick a report row and verify it highlights and the selection bar shows the correct count and actions.
Click Select all matching items and verify every report row reads as checked.
Untick one report and verify only that row unchecks.

Test 8 — Load more inside a group

Find (or create) a category/merchant group with more than 20 expenses.
Expand it and scroll to the bottom of the group.
Click Show more.
Verify additional expenses load into the group, in the same sort order, with no duplicates and no rows disappearing.
Tick the group header and verify the newly loaded expenses are ticked too.

Verify that no errors appear in the JS console

Offline tests

Same as tests

QA Steps

// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

Mac-Chrome.mp4

melvin-bot · 2026-08-15T14:15:40Z

@situchan Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

codecov · 2026-08-15T14:18:31Z

Codecov Report

❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.

Files with missing lines	Coverage Δ
...h/SearchList/ListItem/TransactionGroupListItem.tsx	`75.73% <100.00%> (-1.29%)`	⬇️
src/components/Search/SearchList/ListItem/types.ts	`100.00% <ø> (ø)`
...nts/Search/SearchList/ListItem/useGroupChildren.ts	`100.00% <100.00%> (ø)`
src/components/Search/SearchSelectionProvider.tsx	`73.00% <100.00%> (-0.27%)`	⬇️
src/components/Search/selectionBuilders.ts	`79.45% <100.00%> (+20.19%)`	⬆️
src/libs/SearchUIUtils.ts	`67.57% <100.00%> (+0.46%)`	⬆️
...arch/SearchList/ListItem/ExpenseReportListItem.tsx	`0.00% <0.00%> (ø)`
...earch/SearchList/ListItem/GroupChildrenContent.tsx	`0.00% <0.00%> (-1.36%)`	⬇️
...ponents/Search/SearchList/ListItem/GroupHeader.tsx	`76.85% <80.00%> (+76.85%)`	⬆️
...rch/SearchList/ListItem/GroupChildrenContainer.tsx	`0.00% <0.00%> (ø)`
... and 1 more
... and 24 files with indirect coverage changes

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1b578b75ff

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

…ecked

TaduJR · 2026-08-15T14:39:50Z

@codex review

chatgpt-codex-connector · 2026-08-15T14:43:11Z

Codex Review: Didn't find any major issues. 🎉

Reviewed commit: fcea90bad9

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

… source

TaduJR · 2026-08-15T15:04:48Z

@codex review

TaduJR · 2026-08-15T15:06:57Z

@situchan

Split the PR.

This is the first one. Will get back to #98130 after this one.

chatgpt-codex-connector · 2026-08-15T15:08:09Z

Codex Review: Didn't find any major issues. Already looking forward to the next diff.

Reviewed commit: 27f67889c1

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

… source

chatgpt-codex-connector · 2026-08-16T03:20:59Z

Codex Review: Didn't find any major issues. Hooray!

Reviewed commit: 3fe6406db8

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

MelvinBot · 2026-08-16T03:28:06Z

Review — LGTM with a couple of things to verify manually

Solid refactor. The core claim holds up: useSearchSnapshot stage 2 already runs getSections over each group's sub-snapshot and writes the result onto group.transactions, using the same inputs (bankAccountList, cardFeeds, conciergeReportID, isActionLoadingSet, convertToDisplayString) that the three components were re-passing. So having TransactionGroupListItem, GroupChildrenContent, and GroupHeader read group.transactions instead of re-deriving is genuinely redundant work removed, not behavior dropped — and it fixes the header, which previously built rows without the policy/report action.

Dropping the three Onyx subscriptions in ExpenseGroupedSearchView is a real win: that view renders the whole list, so it no longer re-renders on every card-feed change. Consolidating the three divergent "is this checked" derivations into isRowChecked/getGroupCheckboxState is the right call, and the new tests cover the tricky cases well (all-deleted rows, group-key fallback for unloaded children, exclusions).

Two things worth a manual pass before merge:

Leaf-row selection semantics changed. isRowChecked now returns checked when selectedTransactions[parentGroupKey]?.isSelected, and useRowSelection routes through it. The old useRowSelection had no such clause, so the two leaf callers that pass a parentGroupKey — TransactionListItemNarrow and TransactionListItemWide — now reflect a group selected as a whole (e.g. selected before its children loaded). This looks intentional and consistent with the dedup goal, but it's the one change not exercised end-to-end by the new tests. Worth eyeballing: select a group before expanding it, then expand — the child rows should render checked.
getEmptyObject default removed from useRowSelection. Fine as written — excludedTransactions is typed non-optional and both the provider and the default context supply {}, so Object.hasOwn inside isRowChecked can't hit undefined. Just noting it since the removed default was the only guard.

Other checks I ran

All removed props (bankAccountList/cardFeeds/conciergeReportID, originalKey) are fully cleaned from the call sites and GroupChildrenContentProps — no dangling references.
GroupChildrenContent still needs transactionsSnapshot/snapshotData for filteredViolations and the fetch helpers, so keeping the useOnyx subscription there is correct (not dead code).
isEmpty simplification in GroupHeader/TransactionGroupListItem (transactions.length === 0 && !transactionsQueryJSON) is equivalent given the list-level enrichment, since an unloaded lazy group keeps its transactionsQueryJSON.
CI is green: all 8 test jobs, typecheck, ESLint, React Compiler Compliance, and Oxfmt pass. The only red check is Check independent approval, which just needs a human reviewer.

I couldn't get runtime confirmation in the app — the assigned web test account has no workspaces/expenses/reports, so Search's grouped view is empty and there was nothing to click. The manual checks above (especially item 1, and the narrow/mobile layout where the checkbox derivation changed most) would benefit from a hands-on pass on an account with grouped data.

MelvinBot · 2026-08-16T03:39:05Z

Regression hunt — no behavior-breaking regressions found

I went through the diff adversarially, focusing on the risky swaps and the timing premise. Every one I checked turns out behavior-preserving or a deliberate consistency fix. CI backs this up: all 8 test jobs, typecheck, and React Compiler Compliance are green.

Two intentional behavioral changes worth knowing about (not bugs, but they change what the checkbox shows in edge cases — worth a manual pass since the test account was empty and I couldn't exercise them live):

Collapsed group selected → then expanded now shows its rows checked. The new isRowChecked adds a selectedTransactions[parentGroupKey]?.isSelected clause that the old useRowSelection lacked, and the two leaf rows (TransactionListItemNarrow / Wide) now route through it. This looks like a fix for a group selected-before-its-children-loaded, but it's the one changed path not exercised end-to-end by the new tests. Highest-value thing to verify by hand: select a group while collapsed, expand it, confirm the child rows read checked and that deselecting clears them.
A report/group checkbox now reads unchecked when one child is excluded under select-all (previously it read fully checked). In ExpenseReportListItem, getGroupCheckboxState now evaluates each child's exclusion, whereas the old liveRowSelected || areAllReportTransactionsSelected only checked exclusion on the report's own key. More correct, but a visible change under "select all matching, then deselect one row in a group."

Risky swaps I checked that are safe

isTransactionPendingDelete() vs the old pendingAction === DELETE — equivalent. getTransactionPendingAction only returns DELETE when pendingAction is literally delete; its pendingFields branch returns UPDATE, never DELETE. So the deleted-row filtering and the group's pendingAction derivation are unchanged.
newTransactionIDByItemKey.get(originalKey) with originalKey = groupKeyForList — buildNewTransactionIDMap keys by the original group key, which is exactly what groupKeyForList holds (same value the old .replace('children_','') produced). Dropping the item.keyForList ? guard is safe since groupKeyForList is a required string.
Removed getEmptyObject default for excludedTransactions in useRowSelection — safe; the field is typed non-optional and both the provider and default context supply {}, so Object.hasOwn can't hit undefined.
isEmpty simplification (dropping hasSnapshotTransactions) in GroupHeader/TransactionGroupListItem — equivalent, because the list now enriches group.transactions from the same snapshot, so transactions.length === 0 already implies "no snapshot rows," and a not-yet-fetched lazy group still keeps its transactionsQueryJSON.
Reading group.transactions instead of the component's own getSections — the list already runs the identical getSections call at useSearchSnapshot stage 2 with the same inputs. Since rows now render only what the list handed them (top-down), they can no longer skew ahead of/behind the list the way the old per-component subscription could — strictly more consistent.

One low-risk thing to keep an eye on

GroupChildrenContent now takes its rows from the list-enriched group.transactions (props) but still derives filteredViolations from its own direct sub-snapshot subscription. If the list's enrichment ever lagged that subscription by a frame, a violation icon could briefly reference a row not yet in props (or vice versa). The new snapshotSubscriptionParity test argues both land in the same commit, so this should stay a non-issue — just noting it as the one place the two sources aren't the exact same object.

I couldn't confirm any of this at runtime — the assigned web test account has no workspaces/expenses/reports, so Search's grouped view is empty. The two manual checks above (especially #1) would benefit from a hands-on pass on an account with grouped data, ideally on both the wide/web split layout and the narrow/mobile layout where the checkbox derivation changed most.

MelvinBot · 2026-08-16T03:43:29Z

Short answer: not strictly required — but it's a sensible enabling prerequisite for grouped Search specifically, and does nothing for the feature elsewhere.

None of the actual range-selection mechanism lives in this PR. Per the approved proposal, Shift+Click is the useShiftRangeSelection hook + widening the press-path callbacks to carry {shiftKey} + wiring BaseSelectionList and the custom lists. This PR has none of that (no shiftKey, no hook) — it only consolidates how a group's rows and checkbox state are derived. So you could ship the hook without merging this.

Where it genuinely matters is the one line in the proposal's Search section: "the items array must match the rendered visual order — getSortedSections(filteredData) flattened to [group, ...children]." Range selection computes toSelect/toDeselect over that flattened list, so the rows it ranges over must be exactly the rows on screen, with one agreed selection state. On main, they aren't:

Three components re-derived a group's children independently from per-group sub-snapshots, so the rendered children could differ from filteredData's children — a range computed over the list wouldn't reliably match what's painted.
There were three separate "is this row checked" derivations that disagreed (header vs rows). Range math on top of an inconsistent truth produces wrong ranges.

This PR removes exactly those two hazards: one enriched group.transactions produced by the list (the parity test even pins that the row and the list see the same rows in the same commit), and one isRowChecked/getGroupCheckboxState source of truth. That's the foundation the proposal's Search wiring leans on.

So my read: it's a de-risking prerequisite for grouped Search, not a hard compile-time dependency. It could technically be deferred or folded into the feature PR, but doing it first makes range selection in grouped Search correct-by-construction rather than bolted onto a divergent base. For flat (non-grouped) lists and the onShiftRangeApply opt-in pages, this refactor isn't needed at all — those don't have the group-derivation problem.

If the goal is to keep the feature PR reviewable, splitting the prep out like this is reasonable; if a reviewer is pushing back on scope, the honest framing is "prerequisite cleanup for grouped Search," not "required to compile the feature."

(This is an architecture judgment from the code + proposal, not a runtime finding.)

situchan · 2026-08-16T03:45:21Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified that the composer does not automatically focus or open the keyboard on mobile unless explicitly intended. This includes checking that returning the app from the background does not unexpectedly open the keyboard.
I verified tests pass on all platforms & I tested again on:
- Android: HybridApp
- Android: mWeb Chrome
- iOS: HybridApp
- iOS: mWeb Safari
- MacOS: Chrome / Safari
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
For any bug fix or new feature in this PR, I verified that sufficient unit tests are included to prevent regressions in this flow.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: HybridApp

Android: mWeb Chrome

iOS: HybridApp

Screen.Recording.2026-08-16.at.5.32.16.AM.mov

iOS: mWeb Safari

MacOS: Chrome / Safari

Screen.Recording.2026-08-16.at.5.33.29.AM.mov

situchan · 2026-08-16T03:53:14Z

@TaduJR Before approving, please address:

Why is this refactor needed to support Shift+Click Range Selection (original issue)? Melvin says it's not. Is this really relevant?
Confirm behavioral changes mentioned below and in refactor: derive a group's rows once, by the list that renders them #98716 (comment) are expected.
Add Test steps and platform recordings.

Suggested QA Steps

Setup (do once)

Use a workspace with at least: one category (e.g. Advertising) holding 3+ expenses across 2+ reports, a second category holding exactly 1 expense, and one report with no expenses. At least one expense should be on hold and one should be a card expense.

Test 1 — Grouped expenses still show the right rows (wide screen, web)

Open Search → Expenses, maximize the browser window.
Set Group by: Category.
Verify each group header shows the correct category name, expense count and total.
Click a group header's chevron to expand it.
Verify the expenses listed inside are exactly the expenses in that category, with the correct merchant, date, amount, category, tag and card columns — nothing blank, missing or duplicated.
Repeat steps 2–5 for Group by: Merchant, Tag, From, Card and Month.

Test 2 — Group checkbox reflects its expenses (wide screen, web)

With a group expanded, tick one expense inside it.
Verify the group header checkbox shows the partially-selected (dash) state.
Tick the remaining expenses in that group.
Verify the group header checkbox becomes fully checked.
Untick one expense.
Verify the header returns to the dash state.
Tick the group header checkbox itself.
Verify every expense inside the group becomes ticked, and the count in the top selection bar matches the number of expenses in that group.
Untick the group header checkbox and verify every expense inside is unticked and the selection bar clears.

Test 3 — Same behavior on narrow screens

Shrink the browser window (or use mWeb / iOS / Android).
Repeat Test 2 in the narrow layout.
Verify the group checkbox behaves identically to the wide layout — checked, dash and unchecked appear at the same moments in both.

Test 4 — Bulk actions after selecting from a group header

On wide web, tick a group header checkbox (do not tick individual expenses).
Verify the buttons offered in the selection bar (Hold, Unhold, Reject, Split, Change report, Delete) are the same ones offered when you instead tick every expense in that group individually.
Perform Hold on a header-selected group and verify it succeeds and the expenses show as held.

This is the main fix: previously a header selection produced different available actions than selecting the same expenses one by one.

Test 5 — Select all matching items

Open Search → Expenses, Group by: Category.
Tick any expense, then click Select all matching items in the selection bar.
Verify every group header and every visible expense reads as checked.
Untick one expense inside a group.
Verify that expense is unticked, its group header drops to the dash state, and every other group stays checked.
Untick a whole group header.
Verify only that group unchecks and the rest stay checked.
Re-tick that group header and verify it and its expenses return to checked.

Test 6 — Groups that haven't loaded yet, and empty reports

Group by: Merchant on a search with many groups; scroll so a group appears before you expand it.
Tick that collapsed group's checkbox.
Expand it.
Verify every expense inside appears ticked, and the selection-bar count updates to the number of expenses in that group.
Untick the group header and verify the expenses inside all untick.
Open Search → Reports and tick a report that has no expenses.
Verify it ticks and unticks normally, and the selection bar count is correct.

Test 7 — Reports view row selection

Open Search → Reports.
Tick a report row and verify it highlights and the selection bar shows the correct count and actions.
Click Select all matching items and verify every report row reads as checked.
Untick one report and verify only that row unchecks.

Test 8 — Load more inside a group

Find (or create) a category/merchant group with more than 20 expenses.
Expand it and scroll to the bottom of the group.
Click Show more.
Verify additional expenses load into the group, in the same sort order, with no duplicates and no rows disappearing.
Tick the group header and verify the newly loaded expenses are ticked too.

Test 9 — New expense highlight

With a grouped search open, create a new expense that belongs to one of the visible groups.
Verify the group refreshes and the new expense appears inside it with the green highlight animation.

Offline tests

Go offline.
Delete an expense that sits inside an expanded group.
Verify the expense greys out (pending deletion) and the group header checkbox ignores it — i.e. if all the remaining expenses in the group are ticked, the header reads fully checked rather than dashed.
Go back online and verify the expense disappears and the header checkbox stays correct.

Two behavior changes to expect (not bugs — confirm they're intended)

Under Select all matching items, a report in the Reports view whose expenses are all pending deletion now reads unchecked (it previously read checked).
ExpenseReportListItem (Reports view), select-all-matching + a report whose expenses are all pending-delete. Old liveRowSelected (useRowSelection(reportID)) painted it checked; getGroupCheckboxState now sees children.length > 0 but selectableCount === 0 and returns unchecked. This is the stated intent of fcea90bad93, applied to a surface that commit didn't mention.
A group ticked while still collapsed now paints its expenses as checked the instant they load, rather than a moment later.
A group selected while empty, whose children then load. Rows now paint checked immediately (via isRowChecked's new selectedTransactions[parentGroupKey] clause) instead of waiting for useReconcileSelection. For the one commit before the reconcile effect fires, the rows read checked while the selection bar still counts 1. Previously the inconsistency ran the other way (rows unchecked, count 1).

Neither is a defect, but both are user-visible and neither is covered by an existing test

…group-rows-derivation

TaduJR · 2026-08-16T09:20:27Z

Why is this refactor needed to support Shift+Click Range Selection (original issue)? #98716 (comment). Is this really relevant?

@situchan

Melvin is answering "would the hook compile without this" yes it would. But this isn't unrelated cleanup.

I pulled it out from #98130 to split. #98130 still uses it: the range hook is given isItemSelected: isRowVisiblyChecked, which calls isRowChecked. A range has to ask the same "is this checked" question the checkbox answers, or Shift+click anchors on a row you've already unchecked.

Melvin is right that flat lists don't need this. It's grouped Search only. Happy to move it back into #98130 if you prefer, it just puts many lines back on the PR you asked to shrink.

Both behaviour changes fix a disagreement that exists on main today:

TransactionGroupListItem stamps isSelected on its rows and the header counts that stamp, but TransactionListItemWide ignores it and calls useRowSelection, which has no parent-group clause. On main: select a group while collapsed, expand it, and the header shows checked above rows that show unchecked. Now both ask one predicate.
ExpenseReportListItem read checked under "select all matching" even with a child excluded, because the report's own key never looked at its children. It now reads unchecked, same as the grouped header.

JmillsExpensify

No product review required.

…group-rows-derivation # Conflicts: # src/components/Search/SearchList/ListItem/GroupChildrenContent.tsx # src/components/Search/SearchList/ListItem/TransactionGroupListItem.tsx

TaduJR · 2026-08-19T10:47:14Z

@codex review

chatgpt-codex-connector · 2026-08-19T10:51:03Z

Codex Review: Didn't find any major issues. Keep it up!

Reviewed commit: 9a6074be26

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

TaduJR · 2026-08-19T10:52:26Z

Bump @stitesExpensify

For review and merge

Thanks!

chatgpt-codex-connector · 2026-08-19T11:06:29Z

Codex Review: Didn't find any major issues. What shall we delve into next?

Reviewed commit: 9a6074be26

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

MelvinBot · 2026-08-19T11:09:57Z

Re-review — LGTM, with the same one manual check still open

Nothing new to flag. The refactor still holds up, and the two commits since my last review (3fe6406 → 9a6074be) actually improve the one behavioral edge I called out.

What changed since the last review, and why it's now better: ExpenseReportListItem now derives both isSelected and isIndeterminate from useGroupCheckboxState and passes the indeterminate flag through (previously hard-coded false). So a report with one child excluded under "select all matching" now reads partially-selected (dash) — which is more correct than either the old-main behavior (fully checked) or the interim behavior (fully unchecked). This traces cleanly through getGroupCheckboxState/isRowChecked: the excluded child fails isRowChecked, so checkedCount < selectableCount yields isIndeterminate: true.

I re-verified the consolidation is behavior-preserving where it should be:

Non-empty expense report — selection is keyed by child transaction ID, never the report key, so selectedTransactions[parentGroupKey] never fires for it and each child is still checked by its own entry — same as the old areAllReportTransactionsSelected.
Empty report (children.length === 0) — falls to the groupKey branch and checks the report's own key, matching the old liveRowSelected.
All-children-pending-delete — selectableCount === 0 → reads unchecked/not-indeterminate, unchanged.

Still the one thing worth a hands-on pass: the leaf-row change where a group selected before its children load now marks those children checked once expanded (the selectedTransactions[parentGroupKey]?.isSelected clause in isRowChecked, routed to via TransactionListItemNarrow/Wide). It's the one path the new tests don't exercise end-to-end.

Verification status

CI: fully green — all 8 test jobs, Bun tests, typecheck, ESLint, Oxfmt, React Compiler Compliance, Storybook, and both perf-test suites pass. The only red check is Check independent approval, which just needs a human reviewer.
Runtime: still couldn't confirm in-app — the assigned web test account has no workspaces/expenses/reports, so Search's grouped view is empty (No expenses yet) and there's nothing to select. The manual checks above (especially the collapsed-group-then-expand path, and the narrow/mobile layout where the checkbox derivation changed most) would benefit from a hands-on pass on an account with grouped data.

For the full diff walk-through (the getSections de-duplication premise, the dropped Onyx subscriptions, isTransactionPendingDelete equivalence, groupKeyForList keying, and the getEmptyObject default removal), see my two earlier review comments on this PR — those all still stand.

MelvinBot · 2026-08-19T11:22:44Z

Regression hunt — no behavior-breaking regressions found

I ran a deeper adversarial pass on the current head (9a6074b), targeting the two premises this refactor actually rests on: that reading group.transactions is safe, and that the consolidated checkbox math preserves prior behavior. Both hold up with code evidence. Every behavioral change I found is either exactly equivalent or an intended fix. CI is fully green (all 8 test jobs, Bun tests, typecheck, ESLint, React Compiler Compliance, Oxfmt, Storybook, both perf suites); the only red check is Check independent approval, which needs a human.

The group.transactions enrichment premise is safe — including timing. useSearchSnapshot stage 2 runs getSections over each group's sub-snapshot with the same inputs the removed component code used, and writes onto group.transactions. Both the old direct useOnyx(SNAPSHOT+hash) subscription and the new list-level read are driven by the same Onyx write, and enrichment is a synchronous useMemo (not an effect), so there's no extra-frame lag that could flash an empty group or a stale checkbox. The one input difference — reportActions is now unscoped rather than scoped to the group's child reports — is a functional superset that yields identical rows (stage 1 already uses the unscoped version), so it's a re-render nuance, not a correctness change.

The isEmpty simplification is equivalent. Dropping hasSnapshotTransactions only widens isEmpty in the state transactions.length === 0 && !transactionsQueryJSON && snapshotHasRows, which is unreachable — the snapshot is keyed by transactionsQueryJSON.hash, so with no transactionsQueryJSON there is no snapshot. A lazily-loaded group keeps its transactionsQueryJSON, so it's never wrongly treated as empty.

The isTransactionPendingDelete swap is exactly equivalent. getTransactionPendingAction returns pendingAction verbatim when set, and its pendingFields fallback resolves to UPDATE — never DELETE. So isTransactionPendingDelete() is true iff pendingAction === DELETE. No previously-selectable row gets skipped in the "all selected" math or in transactionsWithoutPendingDelete.

The three checkbox behavior changes are intended fixes, not regressions (worth a manual pass since the test account is empty — see below):

Select-all-matching now paints group checkboxes correctly. selectAllMatchingItems sets areAllMatchingItemsSelected but never writes into selectedTransactions. The old GroupHeader/TransactionGroupListItem group math read only selectedTransactions keys, so it computed selectedCount = 0 and drew the group empty under select-all. The new getGroupCheckboxState/isRowChecked reads areAllMatchingItemsSelected, so it correctly shows checked (or indeterminate when one row is excluded).
A partly-selected report now reads indeterminate. ExpenseReportListItem derives isIndeterminate from the shared helper instead of hard-coding false — the PR's stated purpose.
A leaf row under a group selected-as-a-whole now reads checked. isRowChecked adds the selectedTransactions[parentGroupKey]?.isSelected clause that the old useRowSelection lacked, so TransactionListItemNarrow/Wide now agree with the header. The group-selected-before-children-load case (stored under the group key) resolves to the same selectedTransactions[groupKey] read as before — equivalent.

Runtime status — still couldn't confirm in-app

For the third time, the assigned web test account has no workspaces/expenses/reports, so Search → Expenses shows "No expenses yet" even after applying Group by: Category/Merchant — there's nothing to expand or select. So all of the above is verified from code, not runtime. The two highest-value manual checks on an account with grouped data are:

Select all matching → deselect one child in a group, and confirm the group header (and the report row) shows the dash/indeterminate state, not empty and not fully-checked.
Select a collapsed group → expand it, and confirm the child rows render checked, and deselecting clears them.

Ideally exercise both on the wide/web split layout and the narrow/mobile layout, since that's where the checkbox derivation changed most.

refactor: derive a group's rows once, by the list that renders them

1b578b7

TaduJR requested review from a team as code owners August 15, 2026 14:15

melvin-bot Bot assigned TaduJR Aug 15, 2026

melvin-bot Bot requested review from a team, heyjennahay and situchan and removed request for a team August 15, 2026 14:15

melvin-bot Bot requested review from JmillsExpensify and removed request for a team August 15, 2026 14:15

chatgpt-codex-connector Bot reviewed Aug 15, 2026

View reviewed changes

Comment thread src/components/Search/selectionBuilders.ts Outdated

github-actions Bot reviewed Aug 15, 2026

View reviewed changes

Comment thread src/components/Search/selectionBuilders.ts Outdated

github-actions Bot reviewed Aug 15, 2026

View reviewed changes

Comment thread src/components/Search/SearchList/ListItem/useGroupChildren.ts

fix: keep a group whose rows are all being deleted from reading as ch…

fcea90b

…ecked

refactor: drop the dead expense-report branch and the second checkbox…

27f6788

… source

refactor: give the expense-report row and the header the one checkbox…

3fe6406

… source

situchan reviewed Aug 16, 2026

View reviewed changes

Comment thread src/components/Search/SearchList/ListItem/ExpenseReportListItem.tsx Outdated

Merge branch 'main' of https://github.com/TaduJR/App into feat/90539-…

caaa671

…group-rows-derivation

docs: say why the report row asks the group helper, not what it does

af4075c

situchan approved these changes Aug 16, 2026

View reviewed changes

melvin-bot Bot requested a review from stitesExpensify August 16, 2026 10:15

TaduJR changed the title ~~[No-QA] refactor: derive a group's rows once, by the list that renders them~~ refactor: derive a group's rows once, by the list that renders them Aug 16, 2026

JmillsExpensify reviewed Aug 16, 2026

View reviewed changes

TaduJR added 2 commits August 19, 2026 13:19

Merge branch 'main' of https://github.com/TaduJR/App into feat/90539-…

8b310c8

…group-rows-derivation # Conflicts: # src/components/Search/SearchList/ListItem/GroupChildrenContent.tsx # src/components/Search/SearchList/ListItem/TransactionGroupListItem.tsx

fix: show a partly selected report as indeterminate instead of empty

9a6074b

Uh oh!

Conversation

TaduJR commented Aug 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation of Change

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

melvin-bot Bot commented Aug 15, 2026

Uh oh!

codecov Bot commented Aug 15, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TaduJR commented Aug 15, 2026

Uh oh!

chatgpt-codex-connector Bot commented Aug 15, 2026

Uh oh!

TaduJR commented Aug 15, 2026

Uh oh!

TaduJR commented Aug 15, 2026

Uh oh!

chatgpt-codex-connector Bot commented Aug 15, 2026

Uh oh!

chatgpt-codex-connector Bot commented Aug 16, 2026

Uh oh!

MelvinBot commented Aug 16, 2026

Review — LGTM with a couple of things to verify manually

Uh oh!

MelvinBot commented Aug 16, 2026

Regression hunt — no behavior-breaking regressions found

Uh oh!

MelvinBot commented Aug 16, 2026

Uh oh!

situchan commented Aug 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer Checklist

Screenshots/Videos

Uh oh!

situchan commented Aug 16, 2026

Suggested QA Steps

Offline tests

Uh oh!

Uh oh!

TaduJR commented Aug 16, 2026

Uh oh!

JmillsExpensify left a comment

Choose a reason for hiding this comment

Uh oh!

TaduJR commented Aug 19, 2026

Uh oh!

chatgpt-codex-connector Bot commented Aug 19, 2026

Uh oh!

TaduJR commented Aug 19, 2026

Uh oh!

chatgpt-codex-connector Bot commented Aug 19, 2026

Uh oh!

MelvinBot commented Aug 19, 2026

Re-review — LGTM, with the same one manual check still open

Uh oh!

MelvinBot commented Aug 19, 2026

Regression hunt — no behavior-breaking regressions found

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

TaduJR commented Aug 15, 2026 •

edited

Loading

codecov Bot commented Aug 15, 2026 •

edited by github-actions Bot

Loading

situchan commented Aug 16, 2026 •

edited

Loading