fix: Preserve all matching selection after deselecting an expense - #95902
Conversation
|
@mkhutornyi 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc68c95a59
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
🤖 Reviewed the exclusion-tracking approach. The core design — keeping 1. Count vs. total may disagree for partially-excluded expense reports — worth confirming 2. Pagination offset reset is the highest-risk change — please QA carefully 3. Negative-amount (refund/credit) rows — low priority 4. Confirm the backend honors None of these are blockers on their own, but 1 and 2 are the ones I'd verify with a manual pass before merging. |
|
🤖 Code-quality pass (separate from the correctness notes above — this is about readability/maintainability, not bugs). The PR is generally clean: types are documented, the deps-omission comments were kept in sync, and the new unit tests are well-structured. A few things worth tightening: 1. Duplicated — and subtly divergent — selection-count logic (worth consolidating)
Two counters that are supposed to agree on the same selection can drift apart. Extracting one shared helper (e.g. in 2. Pagination/totals orchestration in 3. 4. Redundant per-consumer default for 5. Minor: naming drift Items 1 and 2 are the ones with real maintenance value; the rest are polish. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0b00c16ff
ℹ️ 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".
mkhutornyi
left a comment
There was a problem hiding this comment.
Please address AI review comments above
|
🤖 Regression hunt on this diff. Most of the change is correctly gated behind the new all-matching/exclusion state, so ordinary selection and pagination are largely untouched. The risks below are the spots where existing behavior changes: R1 — Group / expense-report rows now derive their checked state purely from child Normal per-item selection is fine — for non-empty groups/reports selection is keyed by child transaction ID, so R2 — R3 — Pagination offset can be pushed backward under all-matching R4 — R1 is the one I'd verify first, since it touches the existing select-all display and isn't behind the exclusion flag. |
|
🤖 Regression scan of PR #95902. These are behaviors that worked before and could break — none are confirmed at runtime (logic-derived), so I've ranked by likelihood and flagged what to test. 1. Group/report header checkbox can show unchecked under "Select all matching" (medium)
Previously both fell back to 2. Pagination offset can be pulled backward (medium) 3. Count/total no longer refreshed while paginating under all-matching (low) 4. Bulk-actions bar stays visible when all loaded rows are excluded (low) Item 1 is the one I'd verify first — it touches the most visible everyday flow (Select all in grouped/report views). |
|
@mkhutornyi I found an additional issue while testing this PR:
The backend returns Should we address this in the current PR? It appears the backend may need to return the total report count, possibly as a separate field. staging.mp4 |
|
The original scope is |
Thanks! The selection issue is addressed: expanding an excluded group keeps it unchecked, and rechecking one child no longer reselects the whole group. Grouped CSVs still omit that partial group because the backend exports aggregate rows and does not aggregate standalone transaction IDs. This also occurs on |
|
@mkhutorny, the latest comments have been addressed. Could you please review the updates when you have a chance? |
|
@mkhutornyi kindly bump, thanks. |
|
Please fix conflict |
@mkhutornyi Done, thanks! |
|
I did regression test as many cases as possible. |
Regression check — webNo regressions found in the paths I could exercise, but the core new behavior of this PR ("select all matching" at scale, with exclusions) could not be tested — the provisioned web test account had 0 expenses/reports, and even after seeding a few, the dataset never crosses the pagination threshold that triggers the server-side "All matching items selected" mode. So this is a partial pass, not a clean bill of health. What passed (seeded a workspace + 3 expenses, flat Expenses search, selection mode):
What I could NOT verify (needs a High‑Traffic / 50+ expense account — same precondition as your Tests section):
Given that, I'd treat the manual videos in the PR description as the primary evidence for the at-scale flow. Code areas worth a manual re-check (unverified — flagged from reading the diff, not confirmed bugs)
Evidence3 seeded expensesSelect all → 3 selectedUncheck one → 2 selected, others stay checkedRecheck → back to 3 selectedUncheck all → cleared, stable |
Thanks for the thorough review! I also tested as many different flows and edge cases as possible and found no regressions. Melvin’s review did not identify any confirmed issues either. |
|
@Valforte Done from our side and all yours, thanks! |
Valforte
left a comment
There was a problem hiding this comment.
Code looks good and concise
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 Valforte has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/Valforte in version: 9.4.53-0 🚀
|
|
🤖 Help site review: no changes required. I reviewed the changes in this PR against Expensify's help site articles under Why: This is an internal bug fix, not a change to documented user-facing behavior. It restores the expected behavior on the Spend page: after using Select all (which shows "All matching items selected"), unchecking a single expense now excludes only that expense instead of clearing every non-visible selection — matching what the linked issue #94143 describes as the expected result. The help articles that touch bulk selection only describe it at a high level and remain accurate as written:
None of these document the deselect-one-item exclusion nuance, so nothing becomes inaccurate. The fixed behavior is the intuitive one a user already expects, so it doesn't warrant new documentation. Since no changes are required, I did not create a help site draft PR — there's nothing to review. If you'd like the "select all / exclude individual items" behavior explicitly documented as a feature anyway, let me know and I'll draft that PR. |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.4.53-10 🚀
Bundle Size Analysis (Sentry): |





Explanation of Change
This PR preserves the “all matching items” selection when individual expenses are unchecked.
It:
Fixed Issues
$ #94143
PROPOSAL: #94143 (comment)
Tests
Precondition: Use an account with more than 50 expenses.
Select all matching expenses and Exclude one expense
Preserve selection during pagination
Avoid recalculating totals during pagination
shouldCalculateTotals: false.Recheck an excluded expense
Exclude multiple expenses
Verify grouped and expense-report selection
Verify the selection footer
Offline tests
N/A
QA Steps
Same as Tests
// TODO: These must be filled out, or the issue title must include "[No QA]."
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-native-Select.all.matching.expenses.and.Exclude.one.expense.mov
android-native-Preserve.selection.during.pagination.mp4
Not applicable.
android-native-Recheck.an.excluded.expense.mov
android-native-Exclude.multiple.expenses.mov
android-native-Verify.grouped.and.expense-report.selection.mp4
andoir-native-Verify.the.selection.footer.mov
Android: mWeb Chrome
android-web-Select.all.matching.expenses.and.Exclude.one.expense.mov
android-web-Preserve.selection.during.pagination.mp4
Not applicable.
android-web-Recheck.an.excluded.expense.mov
android-web-Exclude.multiple.expenses.mov
android-web-Verify.grouped.and.expense-report.selection.mp4
andoir-web-Verify.the.selection.footer.mov
iOS: Native
ios-native-Select.all.matching.expenses.and.Exclude.one.expense.mov
ios-native-Preserve.selection.during.pagination.mp4
Not applicable.
ios-native-Recheck.an.excluded.expense.mov
ios-native-Exclude.multiple.expenses.mov
ios-native-Verify.grouped.and.expense-report.selection.mov
ios-native-Verify.the.selection.footer.mov
iOS: mWeb Safari
ios-web-Select.all.matching.expenses.and.Exclude.one.expense.mov
ios-web-Preserve.selection.during.pagination.mp4
Not applicable.
ios-web-Recheck.an.excluded.expense.mov
ios-web-Exclude.multiple.expenses.mov
ios-web-Verify.grouped.and.expense-report.selection.mov
ios-web-Verify.the.selection.footer.mov
MacOS: Chrome / Safari
Select.all.matching.expenses.and.Exclude.one.expense.mov
Preserve.selection.during.pagination.mov
Avoid.recalculating.totals.during.pagination.mp4
Recheck.an.excluded.expense.mov
Exclude.multiple.expenses.mov
Verify.grouped.and.expense-report.selection.webm
Verify.the.selection.footer.mov