fix(grid): bulk-bar Clear resets the row checkboxes, not just the toolbar - #4203
Merged
Conversation
…lbar (#4140) Both `BulkActionBar` mount sites hand-wrote `onClearSelection` as `setSelectedRows([]); setSelectAllMatching(false);` — precisely `resetSelection()` minus the `selectionResetKey` bump. The row checkboxes live inside the embedded data-table and only clear when that key moves, so Clear removed the toolbar and left every row `data-state="checked"` (header `indeterminate` on a partial pick): stranded ticks with no toolbar to act on them, which is the exact failure the reset-key mechanism was introduced for in #3056. Route both sites through `resetSelection()` rather than adding a third hand-copied reset. It is a strict superset of what the sites already did — the same two state writes plus the key bump, no refresh and no refetch — so Clear gains the checkbox reset and changes nothing else. Pinned red-first in bulkActionRefresh.test.tsx on both selection sources (partial pick and whole-page) and on the cross-page "all matching" state. 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
|
…4140) The comment said the test-env `t()` never interpolates, which would send the next reader looking for broken i18n plumbing. The actual mechanism is local and fixable: these grid tests wrap in `ActionProvider` only, so with no `I18nProvider` in the tree `{{count}}` interpolation never runs and the raw template reaches the DOM. BulkActionBar.test.tsx asserts on that same copy and wraps in an English `I18nProvider` precisely for this reason — point at it, so the distinction is one hop away. No assertion changes: structure remains the right probe for a selection-state question, independent of any i18n setup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
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.
Fixes #4140
What was wrong
The selection lives in two places:
selectedRows, which is the grid's own state and drives the toolbar, and the row checkboxes, which live inside the embedded data-table and only clear whenselectionResetKeymoves.resetSelection()writes all three, and the delete / dispatch / dialog-close paths have gone through it since the reset-key mechanism landed in #3056.Both
BulkActionBarmount sites, however, hand-wrote their handler:That is exactly
resetSelection()minus the key bump. So Clear emptied the toolbar and left every row atdata-state="checked"— header stuck atindeterminateon a partial pick — stranding the user on a page of ticked rows with no toolbar left to act on them. That is the literal failure the reset-key mechanism was introduced to prevent, so this is drift from #3056 rather than a new mechanism.Change
Both sites now call
resetSelection(). Measured before choosing this shape (card direction 2):resetSelection()is a strict superset of what the sites already did — the same two state writes plus the key bump, with no refresh, no refetch and no other side effect — so Clear gains the checkbox reset and changes nothing else. No third hand-copied reset was added; the comment aboveresetSelection()now states it is the one reset for every clearing path, including the bar's own Clear.Evidence
Red-first, then reverse-verified by taking the fix back out with
git checkout origin/main -- packages/plugin-grid/src/ObjectGrid.tsx(nevergit stash). Predicted RED in the standard direction, and confirmed with the stranded-ticks signature — the 8 pre-existing tests in the file stayed green both times, so the pin is specific to the defect rather than a broad breakage:In each red case the bulk bar had already been removed and the boxes stayed ticked — the reported symptom exactly. With the fix:
pnpm exec vitest run packages/plugin-grid/— 58 files, 551 tests passed, which covers control 3 (the delete / dispatch / dialog-close reset paths stay green).turbo run type-check --filter=@object-ui/plugin-grid— 13/13 tasks successful (turbo built the dependency closure first).eslinton both touched files — 0 errors;check-control-bytesOK.Three tests pin the button on both selection sources and on the shared handler behind both bar instances.
One test-authoring note
The third test originally asserted the banner's rendered copy, which never matched. The reason is local rather than global: these grid tests wrap in
ActionProvideronly, and with noI18nProviderin the tree the{{count}}interpolation never runs, so the raw template reaches the DOM.BulkActionBar.test.tsxasserts on that same copy and wraps in an EnglishI18nProviderfor exactly this reason. Pinning the rendered count here would have tested i18n plumbing instead of the reset — and its negation would have passed for the wrong reason, since the interpolated text is absent either side of the fix. The banner's two branches are now told apart structurally, by whether the escalation button is still offered, which is the right probe for a selection-state question and holds regardless of i18n setup.(The second commit only corrects that comment's stated mechanism — an earlier draft blamed the test environment generally, which would have sent the next reader hunting for broken i18n plumbing. No assertion changed.)
Scope
Per the card, the cross-page-banner clauses of the QA item stay out (fixture-blocked; recorded on objectstack-ai/objectstack#7439), as does the #3056 mechanism redesign. No action-renderer whitelist or
scripts/file is touched. Changeset added (@object-ui/plugin-gridpatch) — this is user-visible, so noskip-changeset.Generated by Claude Code