Skip to content

94143 test#95332

Draft
mkhutornyi wants to merge 1 commit into
Expensify:mainfrom
mkhutornyi:fix-94143
Draft

94143 test#95332
mkhutornyi wants to merge 1 commit into
Expensify:mainfrom
mkhutornyi:fix-94143

Conversation

@mkhutornyi

Copy link
Copy Markdown
Contributor
important changes

@codecov

codecov Bot commented Jul 5, 2026

Copy link
Copy Markdown

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 Δ
src/components/Search/SearchContextDefinitions.ts 51.85% <ø> (ø)
src/hooks/useSearchBulkActions.ts 53.62% <100.00%> (ø)
src/libs/actions/Search.ts 40.03% <100.00%> (ø)
src/components/Search/SearchBulkActionsButton.tsx 1.51% <0.00%> (-0.10%) ⬇️
...c/components/Search/SearchWriteActionsProvider.tsx 0.00% <0.00%> (ø)
src/components/Search/SearchSelectionFooter.tsx 33.33% <12.50%> (-8.34%) ⬇️
src/components/Search/SearchSelectionProvider.tsx 44.44% <40.00%> (-4.94%) ⬇️
... and 8 files with indirect coverage changes

@mkhutornyi

Copy link
Copy Markdown
Contributor Author

@MelvinBot Compare two PRs - #95902 and #95332

@MelvinBot

Copy link
Copy Markdown
Contributor

Both PRs fix the same issue (#94143preserve the "all matching" selection after unchecking an individual expense) and both land on the same core design: add an excludedTransactions map that represents "all matching EXCEPT these", flip a flag on applySelection so a row toggle narrows the select-all set instead of collapsing it, and pass excludedTransactionIDList to the CSV export endpoints. The difference is scope and completeness, not strategy.

TL;DR

  • #95332 (mkhutornyi) — a minimal reference implementation. Draft, 1 commit, no description, currently CONFLICTING with main. Fixes the selection/exclusion logic only.
  • #95902 (emkhalid) — the full contributor PR. Ready for review, MERGEABLE, full test steps + all-platform videos + linked proposal, and ~5× larger because it also handles pagination/totals and adds substantial unit tests.

The two are functionally equivalent for the basic "uncheck one, keep the rest" flow. #95902 additionally covers cases #95332 leaves out (see below).

Detailed comparison

Shared design (both PRs)

  • New excludedTransactions: SelectedTransactions on the selection state/context, defaulted to {} and cleared whenever select-all is toggled or selection is cleared.
  • applySelection gains an option flag that, while areAllMatchingItemsSelected is true, moves unchecked rows into excludedTransactions and removes re-checked rows from it.
  • useRowSelection: a row is selected if areAllMatchingItemsSelected && !excluded.
  • useReconcileSelectionWithData skips excluded rows so a later data reconcile can't re-add them.
  • hasSelectedTransactions now returns true when areAllMatchingItemsSelected (so the bar/footer stay visible after excluding rows).
  • Button/footer counts subtract the excluded count/total; excludedTransactionIDList added to ExportSearchItemsToCSVParams and threaded through Search.ts.

Where they differ

#95332 (mkhutornyi) #95902 (emkhalid)
Size 152 / 25 across 21 files 806 / 86 across 29 files
State Draft, 1 commit, CONFLICTING Ready, 6 commits, MERGEABLE
Metadata Title "94143 test", no description/tests Full description, test steps, all-platform videos, proposal link
applySelection flag shouldUpdateMatchingExclusions (separate early-return branch) shouldPreserveAllMatchingSelection (inline branch)
Bar visibility useSelectionCounts returns Math.max(visibleSelected, 1) SearchActionsBarWide switches from useSelectionCountshasSelectedTransactions
Pagination / totals Not touched — relies on currentSearchResults.search.count already being present Adds getSearchRequestOffsetForMissingAllMatchingCount + effects in Search/index.tsx to fetch the server-wide count/total when it's missing and avoid recalculating totals on every page
Grouped / expense-report counts Uses existing count logic Adds dedicated getTransactionCount (reportID-set for expense-report type) in both button and footer; also tweaks ExpenseReportListItem/TransactionGroupListItem selection display
Tests Adds excludedTransactions to mocks + extends useRowSelectionTest New SearchBulkActionsButtonTest (165), SearchSelectionFooterTest (105), SearchSelectionProviderTest (197), SearchActionsTest (62)

Most significant functional gap

#95332 computes the button/footer count as search.count − excludedCount and assumes search.count is already loaded. #95902 explicitly handles the "count/total not yet computed" case by triggering a server totals request (the searchRequestOffset machinery). So on a fresh ad-hoc query where totals haven't been fetched, #95332 can show allMatchingItemsSelected / an incomplete count where #95902 will resolve the real number. #95902 is the more complete of the two; #95332 reads like a stripped-down reference to validate the core exclusion approach.


⚠️ I could not verify either PR in a browser in this run (web driving was unavailable), so this comparison is from static analysis of the two diffs only. I did not runtime-test the pagination/totals behavior described above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants