From 4a57d813f98675ed16736d56de556348598c6d13 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 23:05:01 +0000 Subject: [PATCH 1/2] fix(grid): bulk-bar Clear resets the row checkboxes, not just the toolbar (#4140) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3 --- ...d-bulk-bar-clear-resets-checkboxes-4140.md | 9 ++ packages/plugin-grid/src/ObjectGrid.tsx | 21 ++-- .../src/__tests__/bulkActionRefresh.test.tsx | 115 +++++++++++++++++- 3 files changed, 137 insertions(+), 8 deletions(-) create mode 100644 .changeset/grid-bulk-bar-clear-resets-checkboxes-4140.md diff --git a/.changeset/grid-bulk-bar-clear-resets-checkboxes-4140.md b/.changeset/grid-bulk-bar-clear-resets-checkboxes-4140.md new file mode 100644 index 0000000000..bddce87180 --- /dev/null +++ b/.changeset/grid-bulk-bar-clear-resets-checkboxes-4140.md @@ -0,0 +1,9 @@ +--- +'@object-ui/plugin-grid': patch +--- + +ObjectGrid's bulk-bar **Clear** now unticks the row checkboxes, instead of only removing the toolbar + +Selecting rows and pressing Clear emptied the bulk-actions bar but left every row checkbox at `data-state="checked"` (the header checkbox stuck at `indeterminate` on a partial pick). The user was stranded on a page of ticked rows with no toolbar left to act on them, and the only way out was a reload or re-selecting and clearing through some other path. + +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 when `selectionResetKey` moves. `resetSelection()` writes all three, and the delete / dispatch / dialog-close paths have gone through it since the reset-key mechanism was introduced. Both `BulkActionBar` mount sites, however, hand-wrote their `onClearSelection` as `setSelectedRows([]); setSelectAllMatching(false);` — exactly `resetSelection()` minus the key bump — so Clear updated one source and left the other ticked. Both sites now call `resetSelection()`, so there is one reset for every path that clears a selection rather than three hand-copied ones, and the cross-page "all matching" state drops with it. diff --git a/packages/plugin-grid/src/ObjectGrid.tsx b/packages/plugin-grid/src/ObjectGrid.tsx index acfbd629b9..7ee41c9d48 100644 --- a/packages/plugin-grid/src/ObjectGrid.tsx +++ b/packages/plugin-grid/src/ObjectGrid.tsx @@ -1935,11 +1935,18 @@ export const ObjectGrid: React.FC = ({ // the consumer-provided onBulkDelete (which already knows about confirm + // refresh). Other actions fall through to the generic action runner. // - // [#3056] Both branches must clear BOTH selection sources. `selectedRows` is - // ours and drives the toolbar; the row checkboxes live inside the data-table - // and only clear when `selectionResetKey` moves. Bumping one without the - // other strands the user on a page of ticked rows with no toolbar to act on - // them — the exact drift `handleBulkDialogClose` below already guards. + // [#3056] Every path that clears the selection must clear BOTH selection + // sources. `selectedRows` is ours and drives the toolbar; the row checkboxes + // live inside the data-table and only clear when `selectionResetKey` moves. + // Bumping one without the other strands the user on a page of ticked rows + // with no toolbar to act on them — the exact drift `handleBulkDialogClose` + // below already guards. + // + // [#4140] So this is the ONE reset — dispatch, delete, dialog-close AND the + // bulk bar's own `onClearSelection`. Both `BulkActionBar` sites previously + // hand-wrote `setSelectedRows([]); setSelectAllMatching(false);`, which is + // this function minus the key bump: Clear emptied the toolbar and left every + // checkbox ticked. Never re-implement the reset inline — call this. const resetSelection = () => { setSelectedRows([]); setSelectAllMatching(false); @@ -2953,7 +2960,7 @@ export const ObjectGrid: React.FC = ({ objectFields={objectSchema?.fields} onAction={dispatchBulkAction} onActionDef={dispatchBulkActionDef} - onClearSelection={() => { setSelectedRows([]); setSelectAllMatching(false); }} + onClearSelection={resetSelection} pageSize={data.length} totalMatching={singleSelection ? undefined : totalMatching} allMatchingSelected={selectAllMatching} @@ -2991,7 +2998,7 @@ export const ObjectGrid: React.FC = ({ objectFields={objectSchema?.fields} onAction={dispatchBulkAction} onActionDef={dispatchBulkActionDef} - onClearSelection={() => { setSelectedRows([]); setSelectAllMatching(false); }} + onClearSelection={resetSelection} pageSize={data.length} totalMatching={singleSelection ? undefined : totalMatching} allMatchingSelected={selectAllMatching} diff --git a/packages/plugin-grid/src/__tests__/bulkActionRefresh.test.tsx b/packages/plugin-grid/src/__tests__/bulkActionRefresh.test.tsx index e2a4c605c9..497bb25627 100644 --- a/packages/plugin-grid/src/__tests__/bulkActionRefresh.test.tsx +++ b/packages/plugin-grid/src/__tests__/bulkActionRefresh.test.tsx @@ -21,7 +21,7 @@ * the missing refresh. */ import { describe, it, expect, vi, beforeAll } from 'vitest'; -import { render, screen, waitFor, fireEvent } from '@testing-library/react'; +import { render, screen, waitFor, fireEvent, within } from '@testing-library/react'; import '@testing-library/jest-dom'; import React from 'react'; @@ -373,3 +373,116 @@ describe('ObjectGrid — dispatchBulkAction keeps selection in sync (#3056)', () await waitFor(() => expect(headerChecked()).toBe('unchecked')); }); }); + +/** + * The SAME desync, the third selection-clearing site (#4140) — the bulk bar's + * own **Clear** button. `resetSelection()` above upholds the #3056 invariant for + * every dispatch path, but both `onClearSelection` props were hand-written as + * `setSelectedRows([]); setSelectAllMatching(false);` — the two state writes + * without the `selectionResetKey` bump. Clicking Clear therefore 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 + * literal failure the reset-key mechanism exists to prevent. + * + * Found by the records-forms QA run (objectstack#7439, `bulk-select-all-matching` + * FAIL, reproduced 2x). These pin the button on BOTH selection sources and on + * both bar instances' shared handler, so the next hand-copy cannot drift again. + */ +describe('ObjectGrid — bulk bar Clear resets both selection sources (#4140)', () => { + const headerChecked = () => + (document.querySelector('thead [role="checkbox"]') as HTMLElement)?.getAttribute('data-state'); + const rowStates = () => + Array.from(document.querySelectorAll('tbody [role="checkbox"]')).map((el) => + el.getAttribute('data-state'), + ); + const clickClear = () => + fireEvent.click( + within(screen.getByTestId('bulk-actions-bar')).getByRole('button', { name: 'Clear' }), + ); + + it('unticks the row checkboxes (not just the toolbar) on a partial selection', async () => { + const ds = makeDataSource(); + renderGrid(ds, { approve: vi.fn(async () => ({ success: true })) }); + + await waitFor(() => expect(screen.getByText('Plan A')).toBeInTheDocument()); + + // Tick ONE of the two rows — the header goes `indeterminate`, which is the + // exact state the QA repro reported as stuck after Clear. + const firstRow = document.querySelectorAll('tbody [role="checkbox"]')[0] as HTMLElement; + expect(firstRow).toBeTruthy(); + fireEvent.click(firstRow); + await waitFor(() => expect(headerChecked()).toBe('indeterminate')); + expect(rowStates()).toEqual(['checked', 'unchecked']); + expect(screen.getByTestId('bulk-actions-bar')).toBeInTheDocument(); + + clickClear(); + + // Both sources reset in lockstep: toolbar gone AND the boxes untick. + await waitFor(() => + expect(screen.queryByTestId('bulk-actions-bar')).not.toBeInTheDocument(), + ); + await waitFor(() => expect(rowStates()).toEqual(['unchecked', 'unchecked'])); + expect(headerChecked()).toBe('unchecked'); + }); + + it('unticks the row checkboxes when the whole page was selected', async () => { + const ds = makeDataSource(); + renderGrid(ds, { approve: vi.fn(async () => ({ success: true })) }); + + await waitFor(() => expect(screen.getByText('Plan A')).toBeInTheDocument()); + fireEvent.click(document.querySelector('thead [role="checkbox"]') as HTMLElement); + await waitFor(() => expect(headerChecked()).toBe('checked')); + + clickClear(); + + await waitFor(() => + expect(screen.queryByTestId('bulk-actions-bar')).not.toBeInTheDocument(), + ); + await waitFor(() => expect(headerChecked()).toBe('unchecked')); + expect(rowStates()).toEqual(['unchecked', 'unchecked']); + }); + + it('drops the cross-page "all matching" state too, so a fresh pick starts clean', async () => { + // `total` exceeds the returned page, so the bar offers "Select all N matching". + const ds = makeDataSource(); + ds.find = vi.fn(async () => { + const data = Object.values(ds.store).map((r: any) => ({ ...r })); + return { data, total: 5, hasMore: true, pageSize: 50 }; + }); + renderGrid(ds, { approve: vi.fn(async () => ({ success: true })) }); + + await waitFor(() => expect(screen.getByText('Plan A')).toBeInTheDocument()); + fireEvent.click(document.querySelector('thead [role="checkbox"]') as HTMLElement); + await waitFor(() => expect(headerChecked()).toBe('checked')); + + // Escalate to the cross-page selection, then Clear it. + // + // The banner's two branches are told apart *structurally*, by whether the + // escalation button is still offered — not by its copy. The test-env `t()` + // returns `defaultValue` without interpolating, so every count-bearing + // string arrives as the literal "All {{count}} matching records are + // selected."; asserting on the rendered count would pin i18n plumbing, and + // its negation would pass for the wrong reason (the interpolated text is + // never present, before or after the fix). + fireEvent.click(await screen.findByTestId('bulk-select-all-matching')); + await waitFor(() => + expect(screen.queryByTestId('bulk-select-all-matching')).not.toBeInTheDocument(), + ); + expect(screen.getByTestId('bulk-cross-page-banner')).toBeInTheDocument(); + + clickClear(); + + await waitFor(() => + expect(screen.queryByTestId('bulk-actions-bar')).not.toBeInTheDocument(), + ); + await waitFor(() => expect(headerChecked()).toBe('unchecked')); + + // Re-picking the page must start from scratch: the header checkbox is live + // again (it would toggle the *other* way if the table still held the old + // selection) and the bar offers the escalation rather than remembering it — + // the button being back is exactly `selectAllMatching === false`. + fireEvent.click(document.querySelector('thead [role="checkbox"]') as HTMLElement); + await waitFor(() => expect(headerChecked()).toBe('checked')); + expect(await screen.findByTestId('bulk-select-all-matching')).toBeInTheDocument(); + }); +}); From 9429abc7af218bc82be4564b86da63a543adea66 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 23:25:32 +0000 Subject: [PATCH 2/2] test(grid): name the real reason the banner is probed structurally (#4140) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3 --- .../src/__tests__/bulkActionRefresh.test.tsx | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/packages/plugin-grid/src/__tests__/bulkActionRefresh.test.tsx b/packages/plugin-grid/src/__tests__/bulkActionRefresh.test.tsx index 497bb25627..11139cef06 100644 --- a/packages/plugin-grid/src/__tests__/bulkActionRefresh.test.tsx +++ b/packages/plugin-grid/src/__tests__/bulkActionRefresh.test.tsx @@ -458,12 +458,15 @@ describe('ObjectGrid — bulk bar Clear resets both selection sources (#4140)', // Escalate to the cross-page selection, then Clear it. // // The banner's two branches are told apart *structurally*, by whether the - // escalation button is still offered — not by its copy. The test-env `t()` - // returns `defaultValue` without interpolating, so every count-bearing - // string arrives as the literal "All {{count}} matching records are - // selected."; asserting on the rendered count would pin i18n plumbing, and - // its negation would pass for the wrong reason (the interpolated text is - // never present, before or after the fix). + // escalation button is still offered — not by its copy. These grid tests + // wrap in `ActionProvider` only, with no `I18nProvider`, so `{{count}}` + // interpolation never runs and count-bearing strings reach the DOM as the + // raw template ("All {{count}} matching records are selected."). See + // BulkActionBar.test.tsx, which asserts on that copy and therefore *does* + // wrap in an English `I18nProvider`. Asserting the rendered count here + // would pin i18n plumbing rather than the reset, and its negation would + // pass for the wrong reason — the interpolated text is absent either side + // of the fix. Structure is the right probe for a selection-state question. fireEvent.click(await screen.findByTestId('bulk-select-all-matching')); await waitFor(() => expect(screen.queryByTestId('bulk-select-all-matching')).not.toBeInTheDocument(),