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 000000000..bddce8718 --- /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 acfbd629b..7ee41c9d4 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 e2a4c605c..11139cef0 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,119 @@ 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. 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(), + ); + 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(); + }); +});