From 6099e17b382a898e112eb9c2570401a330cf0852 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 21:54:12 +0000 Subject: [PATCH] fix(plugin-grid): the cross-page select-all banner works under external pagination (#4464) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3 --- ...oss-page-select-all-external-pagination.md | 18 ++ packages/plugin-grid/src/ObjectGrid.tsx | 21 +- .../ListView.crossPageSelectAll.test.tsx | 249 ++++++++++++++++++ vitest.config.mts | 7 + 4 files changed, 291 insertions(+), 4 deletions(-) create mode 100644 .changeset/cross-page-select-all-external-pagination.md create mode 100644 packages/plugin-list/src/__tests__/ListView.crossPageSelectAll.test.tsx diff --git a/.changeset/cross-page-select-all-external-pagination.md b/.changeset/cross-page-select-all-external-pagination.md new file mode 100644 index 000000000..f625b88d6 --- /dev/null +++ b/.changeset/cross-page-select-all-external-pagination.md @@ -0,0 +1,18 @@ +--- +'@object-ui/plugin-grid': patch +--- + +fix(plugin-grid): the cross-page "Select all N matching" banner works under external pagination + +`BulkActionBar`'s cross-page affordance was gated on ObjectGrid's `totalMatching` +state, whose only writer is the component's own data loader. Under a host that +fetches the rows itself — ListView passing `manualPagination` + `rowCount`, which +is what the console does — that loader never runs, so the total stayed +`undefined` and the banner was permanently absent for any match-set size, even +though the pager two lines away was already rendering the correct page count from +the host's total. + +The pager's derivation is now hoisted to a single `resolvedTotalMatching` value +that both the pager and `BulkActionBar` consume, so the affordance reports the +real server total on both paths. The `selection.type: 'single'` suppression is +unchanged. diff --git a/packages/plugin-grid/src/ObjectGrid.tsx b/packages/plugin-grid/src/ObjectGrid.tsx index 934ba2010..c59ed5777 100644 --- a/packages/plugin-grid/src/ObjectGrid.tsx +++ b/packages/plugin-grid/src/ObjectGrid.tsx @@ -2285,7 +2285,20 @@ export const ObjectGrid: React.FC = ({ ? schema.searchableFields.length > 0 : (schema.showSearch !== undefined ? schema.showSearch : true); - const manualRowCount = externalManualPagination ? hostRowCount : totalMatching; + // The real match total, from whichever side owns the fetch — ONE derived + // value with ONE answer (objectui#4464). The pager has always read it this + // way; the cross-page "Select all N matching" affordance read the raw + // `totalMatching` STATE instead, whose only writer is this component's own + // data loader. Under a host that fetches the rows itself (ListView passing + // `manualPagination` + `rowCount`, i.e. the console) that loader never runs, + // so the state stayed `undefined` and `BulkActionBar`'s gate was permanently + // false while the pager, two lines away, showed the correct page count off + // the very number the bar needed. + // + // Both consumers now read this const. Do NOT re-spell the conditional at a + // second consumption site: two copies of the fallback is exactly how one of + // them gets missed again (this defect, and #4138 before it). + const resolvedTotalMatching = externalManualPagination ? hostRowCount : totalMatching; const manualPage = externalManualPagination ? hostPage : serverPage; const manualPageSize = externalManualPagination ? (hostPageSize ?? serverPageSize) @@ -2345,7 +2358,7 @@ export const ObjectGrid: React.FC = ({ // In server mode `data` IS the current page; tell DataTable to render it // as-is and drive paging via the callbacks below using the real match total. manualPagination: manualPaginationOn, - rowCount: manualPaginationOn ? manualRowCount : undefined, + rowCount: manualPaginationOn ? resolvedTotalMatching : undefined, page: manualPaginationOn ? manualPage : undefined, onPageChange: manualPaginationOn ? manualOnPageChange : undefined, onPageSizeChange: manualPaginationOn ? manualOnPageSizeChange : undefined, @@ -3088,7 +3101,7 @@ export const ObjectGrid: React.FC = ({ onActionDef={dispatchBulkActionDef} onClearSelection={resetSelection} pageSize={data.length} - totalMatching={singleSelection ? undefined : totalMatching} + totalMatching={singleSelection ? undefined : resolvedTotalMatching} allMatchingSelected={selectAllMatching} onSelectAllMatching={singleSelection ? undefined : () => setSelectAllMatching(true)} /> @@ -3126,7 +3139,7 @@ export const ObjectGrid: React.FC = ({ onActionDef={dispatchBulkActionDef} onClearSelection={resetSelection} pageSize={data.length} - totalMatching={singleSelection ? undefined : totalMatching} + totalMatching={singleSelection ? undefined : resolvedTotalMatching} allMatchingSelected={selectAllMatching} onSelectAllMatching={singleSelection ? undefined : () => setSelectAllMatching(true)} /> diff --git a/packages/plugin-list/src/__tests__/ListView.crossPageSelectAll.test.tsx b/packages/plugin-list/src/__tests__/ListView.crossPageSelectAll.test.tsx new file mode 100644 index 000000000..6c07b8082 --- /dev/null +++ b/packages/plugin-list/src/__tests__/ListView.crossPageSelectAll.test.tsx @@ -0,0 +1,249 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * Cross-page "Select all N matching" under ListView's EXTERNAL pagination + * (objectui#4464). + * + * `BulkActionBar` gates the cross-page banner on `totalMatching`, and the only + * writer of the `totalMatching` STATE is `setTotalMatching(...)` 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 as `data` plus + * `manualPagination` / `rowCount` / `page` / `onPageChange`, so the grid takes + * the inline-data path, `totalMatching` stays `undefined`, and the banner + * condition is permanently false — for any match-set size. Found by a retest of + * `records-forms.bulk-select-all-matching` (objectstack#7439) against a genuinely + * server-paginated view: 26 records, pageSize 10, "Page 1 of 3", select the whole + * page → only `10 selected | Delete | Clear`. + * + * WHY THIS FILE LIVES IN plugin-list, NOT plugin-grid + * ObjectGrid rendered in isolation fetches its own rows, which runs the internal + * loader and populates `totalMatching` — precisely the path the console does not + * take. A unit test there is structurally blind to this defect and stays green + * either side of the fix. The pin has to be the ListView-hosted composition, so + * that what is asserted is the real prop handoff (`ListView.tsx` ~:3079) meeting + * the real consumer (`ObjectGrid.tsx`). + * + * The grid here is the REAL `@object-ui/plugin-grid` renderer, not a stub: this + * file is listed in `heavyDomTests` (vitest.config.mts), whose setup imports + * plugin-grid for its side-effect registration. That keeps the composition + * honest without plugin-list taking a dependency on plugin-grid. + */ +import { describe, it, expect, vi, beforeAll, beforeEach, afterEach } from 'vitest'; +import { cleanup, fireEvent, render, screen, waitFor, within } from '@testing-library/react'; +import React from 'react'; +import { I18nProvider, SchemaRenderer, SchemaRendererProvider } from '@object-ui/react'; +import { ListView } from '../ListView'; +import type { ListViewSchema } from '@object-ui/types'; + +const OBJECT = 'showcase_contact'; +const TOTAL = 26; +const PAGE_SIZE = 10; + +beforeAll(() => { + if (!Element.prototype.scrollIntoView) { + Element.prototype.scrollIntoView = vi.fn() as any; + } +}); + +/** + * A server that pages for real: it answers `$top`/`$skip` with that window and + * reports the full match count as `total` — the shape ListView reads into + * `serverTotal` and forwards as the grid's `rowCount`. + */ +function makeDataSource(total = TOTAL) { + const find = vi.fn(async (_object: string, params: any) => { + const top = params?.$top ?? PAGE_SIZE; + const skip = params?.$skip ?? 0; + const data = Array.from( + { length: Math.max(0, Math.min(top, total - skip)) }, + (_, i) => ({ id: `c-${skip + i}`, name: `Contact ${skip + i}` }), + ); + return { data, total, hasMore: skip + data.length < total }; + }); + return { + find, + findOne: vi.fn(), + create: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + getObjectSchema: async (name: string) => ({ + name, + fields: { id: { type: 'text' }, name: { type: 'text' } }, + }), + } as any; +} + +const listSchema = (over: Record = {}): ListViewSchema => ({ + type: 'list-view', + objectName: OBJECT, + fields: ['name'], + // A view authored with a page size is what turns on ListView's real + // server-side paging — the reproduction's `config.pagination.pageSize`. + pagination: { pageSize: PAGE_SIZE }, + selection: { type: 'multiple' }, + ...over, +} as unknown as ListViewSchema); + +// BulkActionBar surfaces its strings through the i18n layer, so every render +// wraps in an English provider — without one `{{count}}` interpolation never +// runs and the raw templates reach the DOM, which would make "carries the +// server total" unassertable (same reason as `BulkActionBar.test.tsx`). +function renderComposition(ds: any, schema: ListViewSchema) { + return render( + + + + + , + ); +} + +const headerCheckbox = () => + document.querySelector('thead [role="checkbox"]') as HTMLElement | null; +const rowCheckboxes = () => + Array.from(document.querySelectorAll('tbody [role="checkbox"]')) as HTMLElement[]; + +/** Rows are on screen and the grid holds exactly one server window. */ +async function awaitFirstPage() { + await waitFor(() => expect(screen.getByText('Contact 0')).toBeInTheDocument()); + await waitFor(() => expect(rowCheckboxes().length).toBe(PAGE_SIZE)); +} + +async function selectWholePage() { + await awaitFirstPage(); + fireEvent.click(headerCheckbox() as HTMLElement); + await waitFor(() => expect(headerCheckbox()?.getAttribute('data-state')).toBe('checked')); + return screen.getByTestId('bulk-actions-bar'); +} + +beforeEach(() => { vi.clearAllMocks(); }); +afterEach(() => { cleanup(); }); + +describe('ListView-hosted grid — cross-page select-all under external pagination (#4464)', () => { + it('offers the cross-page banner when the whole server page is selected', async () => { + const ds = makeDataSource(); + renderComposition(ds, listSchema()); + + const bar = await selectWholePage(); + + // The whole page — and only the page — is selected. + expect(within(bar).getByText(/10 selected/)).toBeInTheDocument(); + + // Pre-fix BOTH of these were absent, because `totalMatching` was written + // only by a loader that never ran on this path. + expect(screen.getByTestId('bulk-cross-page-banner')).toBeInTheDocument(); + expect(screen.getByTestId('bulk-select-all-matching')).toBeInTheDocument(); + }); + + it('carries the SERVER total in the escalation, not the page window', async () => { + const ds = makeDataSource(); + renderComposition(ds, listSchema()); + + await selectWholePage(); + + // 26 is `result.total` — the number ListView passes down as `rowCount`. + // The window (10) is what a page-local count would report instead. + expect(await screen.findByTestId('bulk-select-all-matching')).toHaveTextContent( + 'Select all 26 matching', + ); + expect(screen.getByTestId('bulk-cross-page-banner')).toHaveTextContent( + 'All 10 on this page are selected.', + ); + }); + + it('escalates the selection past the page when "Select all matching" is clicked', async () => { + const ds = makeDataSource(); + renderComposition(ds, listSchema()); + + const bar = await selectWholePage(); + fireEvent.click(await screen.findByTestId('bulk-select-all-matching')); + + // The selection state is now the whole match set (26), not the window (10): + // the escalation button is gone and the bar reports the match count. + await waitFor(() => + expect(screen.queryByTestId('bulk-select-all-matching')).not.toBeInTheDocument(), + ); + expect(screen.getByTestId('bulk-cross-page-banner')).toHaveTextContent( + 'All 26 matching records are selected.', + ); + expect(within(bar).getByText(/26 selected \(all matches\)/)).toBeInTheDocument(); + + // Scope note, stated rather than faked: the COLLECTED id set is materialized + // only when a bulk action is dispatched (`resolveBulkRows` re-issues the + // query), so it is not observable from the selection UI this test drives. + // Asserting it here would need a bulk action + its dialog, and it would + // additionally pin `lastFindParamsRef` — internal-loader-only state whose + // behaviour on THIS external path is a separate, filed finding (#4501). + }); + + it('shows no banner for a partial page selection', async () => { + const ds = makeDataSource(); + renderComposition(ds, listSchema()); + await awaitFirstPage(); + + fireEvent.click(rowCheckboxes()[0]); + await waitFor(() => expect(screen.getByTestId('bulk-actions-bar')).toBeInTheDocument()); + expect(within(screen.getByTestId('bulk-actions-bar')).getByText(/1 selected/)).toBeInTheDocument(); + + // `totalMatching` IS 26 here after the fix — the banner stays away purely + // because the selection is short of a full page. That is the clause this + // case pins, and it is the one the fix could plausibly have widened. + expect(screen.queryByTestId('bulk-cross-page-banner')).not.toBeInTheDocument(); + expect(screen.queryByTestId('bulk-select-all-matching')).not.toBeInTheDocument(); + }); + + it('suppresses the escalation entirely under selection.type = single', async () => { + const ds = makeDataSource(); + renderComposition(ds, listSchema({ selection: { type: 'single' } })); + await awaitFirstPage(); + + // Honest limit of this case: `selection.type: 'single'` caps the selection + // at one row (the data-table enforces replace-on-select, #2941), so the + // count clause alone would keep the banner away even if the + // `singleSelection ? undefined : …` prop gate were dropped. It is kept + // because it is the console-visible must-not-change; the prop gate itself + // is only pinned by inspection. + fireEvent.click(rowCheckboxes()[0]); + await waitFor(() => expect(screen.getByTestId('bulk-actions-bar')).toBeInTheDocument()); + expect(screen.queryByTestId('bulk-cross-page-banner')).not.toBeInTheDocument(); + expect(screen.queryByTestId('bulk-select-all-matching')).not.toBeInTheDocument(); + }); +}); + +describe('standalone ObjectGrid — the internal-loader path is unchanged (#4464)', () => { + // The must-not-change side: a grid that fetches its OWN rows still resolves + // the total from `result.total` and offers the banner at the same thresholds. + // Rendered through the registry (`type: 'object-grid'`) rather than by + // importing the component, so plugin-list keeps no dependency on plugin-grid. + const gridSchema = { + type: 'object-grid', + objectName: OBJECT, + columns: ['name'], + selection: { type: 'multiple' }, + pagination: { pageSize: PAGE_SIZE }, + }; + + it('still offers "Select all 26 matching" when it loads its own rows', async () => { + const ds = makeDataSource(); + render( + + + + + , + ); + + await selectWholePage(); + + expect(screen.getByTestId('bulk-cross-page-banner')).toBeInTheDocument(); + expect(await screen.findByTestId('bulk-select-all-matching')).toHaveTextContent( + 'Select all 26 matching', + ); + }); +}); diff --git a/vitest.config.mts b/vitest.config.mts index 1c767f300..cc0780bde 100644 --- a/vitest.config.mts +++ b/vitest.config.mts @@ -109,6 +109,13 @@ const heavyDomTests = [ 'packages/plugin-grid/src/__tests__/predicate-surface-parity.test.tsx', 'packages/plugin-kanban/src/registration.test.tsx', 'packages/plugin-kanban/src/KanbanRenderer.uncolumned.test.tsx', + // objectui#4464 — a ListView-hosted composition over the REAL object-grid. + // The defect only exists where ListView owns the fetch and the grid takes the + // inline-data path, so a stub grid (what every other plugin-list test + // registers) cannot see it; this file needs the setup's plugin-grid + // side-effect registration, and taking a plugin-list -> plugin-grid + // dependency to import it directly would be the heavier change. + 'packages/plugin-list/src/__tests__/ListView.crossPageSelectAll.test.tsx', ]; export default defineConfig({