fix(plugin-grid): the cross-page select-all banner works under external pagination (#4464) - #4503
Merged
Merged
Conversation
…al pagination (#4464) `BulkActionBar`'s cross-page affordance was gated on the `totalMatching` STATE, whose only writer is ObjectGrid's own data loader. Under a host that fetches the rows itself (ListView passing `manualPagination` + `rowCount` — the console) that loader never runs, so the total stayed `undefined` and the banner was permanently absent for any match-set size, while the pager two lines away already showed the correct page count from the host's total. Hoist the pager's derivation to a single `resolvedTotalMatching` and have both the pager and both `BulkActionBar` sites consume it — one derived value, one answer, no second copy of the conditional. `singleSelection` gating unchanged. The pin is a ListView-hosted composition test: ObjectGrid in isolation runs the internal loader, which is precisely the path the console does not take, so a unit test there is structurally blind to this defect. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
Collaborator
Author
|
ACCEPT — but HELD IN DRAFT, deliberately not armed — step-7 复核 by PM session The fix itself is accepted in full: one derived value with three consumers and zero second copies (the rename-in-place is exactly the no-alias-pair shape the ruling wanted), red-first reproducing the card's symptom verbatim (bar with the page count, both testids absent), the honest pin-limit notes on single-mode and the collected-set clause, byte-identical Why it does not arm yet — the dev's own headline, and it is right: this PR makes a previously-unreachable DESTRUCTIVE path reachable. On the external-pagination path Sequencing ruling: #4501 clause 1 is being dispatched NOW with priority. This PR flips ready + arms only after #4501's guard is in Generated by Claude Code Generated by Claude Code |
This was referenced Aug 12, 2026
yinlianghui
marked this pull request as ready for review
August 13, 2026 02:13
yinlianghui
enabled auto-merge
August 13, 2026 02:13
yinlianghui
disabled auto-merge
August 13, 2026 02:13
yinlianghui
enabled auto-merge
August 13, 2026 02:13
…l compose PR #4510 (#4501's abstain floor) landed on the same two `BulkActionBar` prop sites this branch rewrites, so both hunks conflicted. Composed rather than picked: the offer requires #4510's FLOOR (`canOfferSelectAllMatching` — no escalation without a query to replay) and carries #4503's RESOLVED total (`resolvedTotalMatching` — the host's `rowCount` on the external path), i.e. totalMatching={canOfferSelectAllMatching ? resolvedTotalMatching : undefined} at both sites. The floor subsumes the `singleSelection ? undefined : …` suppression the incoming side spelled there — `!singleSelection` is its first conjunct — so no gating is lost; a host with a real total but no `findParams` still gets no offer, which is the safety semantics winning the tie. Follow-through, ruled in both PRs: the composition test gains the full-path fan-out assertion #4510 made possible. The real ListView issues a real filtered query, the real grid offers the escalation off the host's `rowCount`, and the dispatched bulk action's fan-out is asserted to replay ListView's own params verbatim — measured against what went on the wire, not a hand-written literal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3
…sspage-totalmatching
Collaborator
Author
Conflict with #4510 resolved — the two changes COMPOSE#4510 (#4501's safety guard) landed on the same two The composed shape
Resolved to, at both sites: The floor decides whether the escalation is offered; the resolved total says how many it covers. Neither substitutes for the other, and the safety semantics win the tie: a host with a real The incoming side spelled Both halves reverse-verified as load-bearing at the merged sitesEach half removed in turn, restored byte-identical afterwards (sha256-verified,
Follow-through — the assertion #4510 made possibleBoth PRs ruled it: with #4510 landed, ListView hands Real ListView issues a real filtered query (26 of 40 records match), the real grid offers the escalation off the host's …and therefore the executor receives the match set only (26 active, The selection-state escalation was already pinned; the fan-out params assertion is the addition. Re-verification on the merged tree
Auto-merge deliberately not re-enabled — leaving that to the PM. PR stays ready, not draft. Generated by Claude Code Generated by Claude Code |
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
Collaborator
Author
|
Post-merge record — PM session
#4464 closes with this merge. The #4501 family is fully landed: guard (#4510), banner (#4503), composition pin. Generated by Claude Code Generated by Claude Code |
This was referenced Aug 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #4464
The defect
BulkActionBargates the cross-page banner ontotalMatching, and the only writer of that state issetTotalMatching(...)inside ObjectGrid's own data loader. In the console the grid never runs that loader: ListView fetches the rows itself and hands the window down asdataplusmanualPagination/rowCount/page/onPageChange, so the grid takes the inline-data path, the state staysundefined, and the banner condition is permanently false — for any match-set size.The tell the issue records is exact: the pager was already correct, because it reads a derived total (
externalManualPagination ? rowCount : totalMatching). The grid had the real number on the external path; it just never reached the bar.The fix — one derived value, three consumers
The pager's derivation is hoisted to a single
resolvedTotalMatching, and the pager plus bothBulkActionBarrender sites now read it. The conditional is written once and only once — two copies of the fallback is how one of them gets missed again (this defect, and #4138 before it). ThesingleSelection ? undefined : ...suppression is unchanged, and the value resolves to the SERVER total (rowCount, the 26 the reproduction measured), never the page window.No public type moved:
dist/**/*.d.tsis byte-identical built both with and without the change, so this is runtime wiring only —patchfor@object-ui/plugin-grid.Red-first
The pin is a ListView-hosted composition test, not an ObjectGrid unit test: ObjectGrid in isolation fetches its own rows, which runs the internal loader and populates
totalMatching— precisely the path the console does not take, so a unit test there stays green either side of the fix. The new file renders the REALobject-gridunder ListView with a server that pages for real (26 records, pageSize 10).Before the fix, with
10 selectedon screen and the whole page ticked:The three that PASSED pre-fix are the must-not-change cases, green on both sides: a partial page selection shows no banner,
selection.type: 'single'suppresses, and a standalone grid on the internal-loader path still offers "Select all 26 matching". After the fix:Test Files 1 passed (1) / Tests 6 passed (6), andpackages/plugin-grid/+packages/plugin-list/are95 passed (95) / 1100 passed (1100).The file is listed in
heavyDomTests(vitest.config.mts) so the setup's plugin-grid side-effect registration provides the real renderer — the alternative was a plugin-list to plugin-grid dependency purely for a test. That is the only file touched outside the fix, the test and the changeset.Reachability note — please read before merging
The census this issue asks for turned up two more members of the same class, filed as #4501. One of them is not merely adjacent:
lastFindParamsRefalso has a single writer inside that same loader, and it is whatresolveBulkRowsreplays when the user accepts "Select all N matching". On the external path it isnull, so the fan-out queries the object with no filter, sort or search, andapps/app-shell'sObjectViewwiresonBulkDeleteto that result.That path was unreachable before this PR precisely because the banner never appeared. This PR is what makes it reachable, which is what the checklist item asks for — but it means #4501 clause 1 should be graded against this merge rather than triaged at leisure. I did not fix it here: it is outside the ruled surface, and the fix needs a decision (the host has no way to hand its params down today, so it likely wants a prop rather than a fallback inside the grid).
Scope
packages/plugin-grid/src/ObjectGrid.tsx, one new composition test inpackages/plugin-list/src/__tests__/, one changeset, and theheavyDomTestsentry.plugin-list/src/ListView.tsxsource is untouched (imported read-only by the test).Generated by Claude Code