From cc36f79d0bb44343952867179a6e7acc11c6c9db Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 13:05:43 +0000 Subject: [PATCH 1/2] refactor(plugin-grid): import the batch-explain cap from the spec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `useRecordCrudVerdicts` declared `EXPLAIN_BATCH_MAX_RECORD_IDS = 200` locally, a hand copy of a server contract constant, under a doc comment that named its own expiry condition. `@objectstack/spec/security` exports the constant, so the copy is replaced by an import and only the "why it was declared locally" half of the comment is dropped. No value and no behaviour change: the spec exports 200, verified statically and at runtime against the resolved package. What changes is reference identity — the client can no longer drift from the server's cap. Covered by a reference-identity test that stands the spec module in at a cap no hand copy could produce; an assertion on 200 passes on both sides and proves nothing. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011SfZeFWrhGLHmfq61xbz4q --- .../6286-explain-batch-cap-from-spec.md | 40 +++++ .../__tests__/rowRecordCrudVerdict.test.tsx | 21 ++- .../useRecordCrudVerdicts.batchCap.test.tsx | 151 ++++++++++++++++++ .../src/hooks/useRecordCrudVerdicts.ts | 19 ++- 4 files changed, 215 insertions(+), 16 deletions(-) create mode 100644 .changeset/6286-explain-batch-cap-from-spec.md create mode 100644 packages/plugin-grid/src/hooks/useRecordCrudVerdicts.batchCap.test.tsx diff --git a/.changeset/6286-explain-batch-cap-from-spec.md b/.changeset/6286-explain-batch-cap-from-spec.md new file mode 100644 index 0000000000..67e18ef880 --- /dev/null +++ b/.changeset/6286-explain-batch-cap-from-spec.md @@ -0,0 +1,40 @@ +--- +'@object-ui/plugin-grid': patch +--- + +The batch-explain cap the row-verdict hook paginates under is now imported from +`@objectstack/spec/security` instead of re-declared locally (objectui#6286). +`useRecordCrudVerdicts` carried `const EXPLAIN_BATCH_MAX_RECORD_IDS = 200`, a hand copy of +a SERVER contract constant, under a doc comment that named its own expiry condition: the +pinned `@objectstack/spec@17.0.0-rc.6` predated the batch form, and the pin bump would +supersede the declaration. It has. + +**No value changes and no behaviour changes.** The spec exports `200`, which is what the +local copy said, verified by resolving the installed package and reading the export — both +statically (`dist/security/index.d.mts`) and at runtime through the same specifier the +source now uses. What changes is reference identity: if the server relaxes or tightens the +cap and the spec follows, the client follows too, instead of paginating at the old boundary +with no signal anywhere. The cap's whole point is that an over-cap request is refused with +`400 VALIDATION_FAILED` rather than truncated, so a client that silently disagrees with it +is exactly the drift `scripts/check-spec-symbol-derivation.mjs` argues about — and could +not catch here, because both of its scanners skip non-exported declarations and this const +was module-local (objectui#5899). + +The declared floor already carries the symbol, so no range moves: `@objectstack/spec@17.0.0` +— the minimum `^17.0.0` admits — exports `EXPLAIN_BATCH_MAX_RECORD_IDS = 200` from +`./security`. Measured against the published tarballs of `17.0.0-rc.6`, `17.0.0`, `17.1.0` +and `17.2.0`: only the rc lacks it. The declaration was therefore expired one release +earlier than the card that found it assumed. + +The half of the comment that explains *why* the cap exists and what the server does with an +over-cap request is kept and now sits on the import; only the half explaining why it was +declared LOCALLY is gone, since that is the part that stopped being true. + +Covered by a new reference-identity test rather than a value assertion. Every assertion on +`200` passes on both sides of this change — a ghost — so +`useRecordCrudVerdicts.batchCap.test.tsx` stands the spec module in at a cap no hand copy +could produce and asserts the request chunking follows it, with a control case proving the +stand-in installed and differs from the shipped value. The pre-existing cap assertion in +`rowRecordCrudVerdict.test.tsx` now derives its fixture and its bound from the same export +instead of re-typing `200`, which removes the last hand copy on this surface without +pretending to be a two-world test. diff --git a/packages/plugin-grid/src/__tests__/rowRecordCrudVerdict.test.tsx b/packages/plugin-grid/src/__tests__/rowRecordCrudVerdict.test.tsx index 67c4b36302..69a492a3b3 100644 --- a/packages/plugin-grid/src/__tests__/rowRecordCrudVerdict.test.tsx +++ b/packages/plugin-grid/src/__tests__/rowRecordCrudVerdict.test.tsx @@ -37,6 +37,7 @@ */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { EXPLAIN_BATCH_MAX_RECORD_IDS } from '@objectstack/spec/security'; import { render, screen, waitFor, cleanup, act } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import '@testing-library/jest-dom'; @@ -444,20 +445,28 @@ describe('[#4296] one batched call per (object, operation) per page', () => { } }); - it('paginates under the server\'s 200-id cap instead of sending a request it would refuse', async () => { - // The cap is the server's (`EXPLAIN_BATCH_MAX_RECORD_IDS`): over-cap + it('paginates under the server\'s cap instead of sending a request it would refuse', async () => { + // The cap is the server's (`EXPLAIN_BATCH_MAX_RECORD_IDS`, imported from + // the package that owns it rather than re-typed — objectui#6286): over-cap // requests are refused with 400 VALIDATION_FAILED, never truncated, and the // spec directs consumers to paginate under it. Still amortized — 2 calls - // per operation for 250 rows, not 250. - const rows = bigPage(250); + // per operation, never one per row. + // + // The fixture is sized FROM the cap: `cap + m` rows with `0 < m <= cap` is + // exactly two chunks per operation for ANY cap, so this stays a cap test if + // the contract moves. That does NOT make it a two-world test for the + // import itself — the value is the same on both sides of that change, which + // is what `../hooks/useRecordCrudVerdicts.batchCap.test.tsx` exists to pin. + const cap = EXPLAIN_BATCH_MAX_RECORD_IDS; + const rows = bigPage(cap + Math.min(50, cap)); for (const r of rows) server.verdicts.set(r.id, { update: true, delete: true }); renderGrid({ rows }); - await waitFor(() => expect(screen.getByText('Row 249')).toBeInTheDocument()); + await waitFor(() => expect(screen.getByText(`Row ${rows.length - 1}`)).toBeInTheDocument()); await settle(4); expect(server.calls.length).toBe(4); for (const call of server.calls) { - expect(call.recordIds!.length).toBeLessThanOrEqual(200); + expect(call.recordIds!.length).toBeLessThanOrEqual(cap); expect(call.recordIds!.length).toBeGreaterThan(0); } for (const operation of ['update', 'delete'] as const) { diff --git a/packages/plugin-grid/src/hooks/useRecordCrudVerdicts.batchCap.test.tsx b/packages/plugin-grid/src/hooks/useRecordCrudVerdicts.batchCap.test.tsx new file mode 100644 index 0000000000..81c242f7c0 --- /dev/null +++ b/packages/plugin-grid/src/hooks/useRecordCrudVerdicts.batchCap.test.tsx @@ -0,0 +1,151 @@ +/** + * 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. + */ + +/** + * [objectui#6286] The cap this hook paginates under IS + * `@objectstack/spec/security`'s `EXPLAIN_BATCH_MAX_RECORD_IDS` — not a local + * number that happens to equal it. + * + * ## Why this file exists, and why asserting the VALUE would not do + * + * Until this card the hook declared `const EXPLAIN_BATCH_MAX_RECORD_IDS = 200` + * locally, and the spec exports `200`. So every assertion on the value passes + * in BOTH worlds — with the hand copy and with the import. That includes the + * cap assertion in `../__tests__/rowRecordCrudVerdict.test.tsx`, which reads + * like a cap test and cannot fail for a drifted cap. A ghost assertion. + * + * What the fix actually changes is REFERENCE IDENTITY: the client can no + * longer drift from the server contract. The only honest way to pin that is to + * MOVE the spec's export and watch the hook follow — so this file stands the + * spec module in at a cap no hand copy could produce and asserts the request + * chunking tracks it. Against the pre-fix hook the stand-in is inert by + * construction (that module is not in its import graph at all) and every case + * below fails: it would send one request of seven ids where three are due. + * + * The stand-in is a bare package specifier, which + * `scripts/check-vi-mock-specifiers.mjs` does not judge (it resolves RELATIVE + * specifiers only), so "the mock silently did not install" is guarded here + * instead — see the control case at the bottom. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { renderHook, waitFor } from '@testing-library/react'; + +const { STUB_CAP, probe } = vi.hoisted(() => ({ + /** + * Deliberately nothing like the real cap, and small enough that the fixture + * stays readable. A hook reading its own copy of the contract cannot produce + * this chunking under any value the spec has ever shipped. + */ + STUB_CAP: 3, + /** + * What the stand-in observed on its way past: whether the factory ran at all + * (i.e. something in the hook's graph really imports this module), and what + * the REAL contract says (so the stub cannot be accidentally equal to it). + */ + probe: { factoryRan: false, realCap: undefined as unknown }, +})); + +vi.mock('@objectstack/spec/security', async (importOriginal) => { + const actual = await importOriginal(); + probe.factoryRan = true; + probe.realCap = actual.EXPLAIN_BATCH_MAX_RECORD_IDS; + return { ...actual, EXPLAIN_BATCH_MAX_RECORD_IDS: STUB_CAP }; +}); + +import { useRecordCrudVerdicts, __clearRecordCrudVerdictCache } from './useRecordCrudVerdicts'; + +const OBJECT = 'showcase_project'; + +interface ExplainCall { + object?: string; + operation?: string; + recordIds?: string[]; +} + +let calls: ExplainCall[] = []; + +/** Records every explain request and answers it `visible: true`. */ +function stubExplain() { + vi.stubGlobal( + 'fetch', + vi.fn(async (_input: unknown, init?: { body?: unknown }) => { + const body = JSON.parse(String(init?.body ?? '{}')) as ExplainCall; + calls.push(body); + return { + ok: true, + status: 200, + json: async () => ({ + allowed: true, + object: body.object, + operation: body.operation, + records: (body.recordIds ?? []).map((recordId) => ({ recordId, visible: true })), + }), + }; + }), + ); +} + +const idsFor = (n: number) => Array.from({ length: n }, (_, i) => `r_${i}`); + +describe('[#6286] the batch cap is the spec\'s export, not a local copy', () => { + beforeEach(() => { + // Module-level memo — without this, ids answered by an earlier case are + // "not missing" in the next one and the chunk arithmetic silently changes. + __clearRecordCrudVerdictCache(); + calls = []; + stubExplain(); + }); + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it('splits a page at the SPEC\'s cap — chunk sizes follow the export when it moves', async () => { + const ids = idsFor(STUB_CAP * 2 + 1); + renderHook(() => useRecordCrudVerdicts({ objectName: OBJECT, recordIds: ids, update: true })); + + await waitFor(() => expect(calls.length).toBe(3)); + // The pre-fix hook produces exactly one call of seven ids here. + expect(calls.map((c) => c.recordIds?.length)).toEqual([STUB_CAP, STUB_CAP, 1]); + // Splitting must not drop or reorder a row: every id asked exactly once. + expect(calls.flatMap((c) => c.recordIds ?? [])).toEqual(ids); + expect(calls.every((c) => c.object === OBJECT && c.operation === 'update')).toBe(true); + }); + + it('does not split AT the cap — a page of exactly cap ids is one request', async () => { + const ids = idsFor(STUB_CAP); + renderHook(() => useRecordCrudVerdicts({ objectName: OBJECT, recordIds: ids, update: true })); + + await waitFor(() => expect(calls.length).toBe(1)); + expect(calls[0].recordIds).toEqual(ids); + }); + + it('splits per operation, so two verbs over cap+1 ids cost four requests', async () => { + const ids = idsFor(STUB_CAP + 1); + renderHook(() => + useRecordCrudVerdicts({ objectName: OBJECT, recordIds: ids, update: true, delete: true }), + ); + + await waitFor(() => expect(calls.length).toBe(4)); + for (const operation of ['update', 'delete'] as const) { + const forOp = calls.filter((c) => c.operation === operation); + expect(forOp.map((c) => c.recordIds?.length)).toEqual([STUB_CAP, 1]); + } + }); + + it('control: the stand-in really installed, and it differs from the real contract', () => { + // If this is false the three cases above proved nothing about provenance — + // they would be measuring the real cap under a different name. It is true + // only because the hook's own import graph pulled this module in. + expect(probe.factoryRan).toBe(true); + // A stub that happened to equal the shipped cap would make every assertion + // above pass for the pre-fix hook too — the ghost this file exists to avoid. + expect(typeof probe.realCap).toBe('number'); + expect(probe.realCap).not.toBe(STUB_CAP); + }); +}); diff --git a/packages/plugin-grid/src/hooks/useRecordCrudVerdicts.ts b/packages/plugin-grid/src/hooks/useRecordCrudVerdicts.ts index b8af8141c3..5065876bd5 100644 --- a/packages/plugin-grid/src/hooks/useRecordCrudVerdicts.ts +++ b/packages/plugin-grid/src/hooks/useRecordCrudVerdicts.ts @@ -54,24 +54,23 @@ * cookie-session hosts. */ import * as React from 'react'; -import { SchemaRendererContext } from '@object-ui/react'; - -/** The two write verbs a list row's kebab can offer. */ -export type RecordCrudOperation = 'update' | 'delete'; - /** * Hard cap on `recordIds` per batch explain request — the SERVER's contract * (`EXPLAIN_BATCH_MAX_RECORD_IDS`, objectstack#8326): over-cap requests are * refused with `400 VALIDATION_FAILED`, never truncated, and the spec's own * TSDoc directs a consumer with more records to paginate under it. A page - * larger than the cap is therefore split into ceil(N / 200) requests per + * larger than the cap is therefore split into ceil(N / cap) requests per * operation — still amortized, never one per row. * - * Declared locally because the pinned `@objectstack/spec@17.0.0-rc.6` predates - * the batch form and exports neither the constant nor the request/response - * types; the pin bump (objectui#4636) supersedes this declaration. + * Imported from the package that OWNS the contract, never re-declared: a hand + * copy passes every value comparison on the day it is written and drifts + * silently on the day the server moves the cap. */ -const EXPLAIN_BATCH_MAX_RECORD_IDS = 200; +import { EXPLAIN_BATCH_MAX_RECORD_IDS } from '@objectstack/spec/security'; +import { SchemaRendererContext } from '@object-ui/react'; + +/** The two write verbs a list row's kebab can offer. */ +export type RecordCrudOperation = 'update' | 'delete'; /** * One entry of the batch response's `records` array, narrowed to what this hook From db31e5fe917ba44363c61af932555bee434fc6e3 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 13:08:16 +0000 Subject: [PATCH 2/2] test(plugin-grid): make the at-cap boundary case a two-world assertion The ablation leg found "exactly cap ids is one request" surviving the revert: three ids fit under a cap of 200 as readily as under a cap of 3, so that half is another ghost. The case now asserts the cap + 1 side in the same breath, which only passes when the boundary being applied is the spec's. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011SfZeFWrhGLHmfq61xbz4q --- .../useRecordCrudVerdicts.batchCap.test.tsx | 25 +++++++++++++++---- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/packages/plugin-grid/src/hooks/useRecordCrudVerdicts.batchCap.test.tsx b/packages/plugin-grid/src/hooks/useRecordCrudVerdicts.batchCap.test.tsx index 81c242f7c0..c71a41c09e 100644 --- a/packages/plugin-grid/src/hooks/useRecordCrudVerdicts.batchCap.test.tsx +++ b/packages/plugin-grid/src/hooks/useRecordCrudVerdicts.batchCap.test.tsx @@ -117,12 +117,27 @@ describe('[#6286] the batch cap is the spec\'s export, not a local copy', () => expect(calls.every((c) => c.object === OBJECT && c.operation === 'update')).toBe(true); }); - it('does not split AT the cap — a page of exactly cap ids is one request', async () => { - const ids = idsFor(STUB_CAP); - renderHook(() => useRecordCrudVerdicts({ objectName: OBJECT, recordIds: ids, update: true })); - + it('splits only ABOVE the cap — exactly cap ids is one request, cap + 1 is two', async () => { + // BOTH sides are asserted deliberately. "Exactly cap ids is one request" is + // true of the pre-fix hook too (3 ids fit under a cap of 200), so on its own + // it is another ghost — it is the `cap + 1` half that can only pass when the + // boundary being applied is the spec's. The ablation leg for this file found + // that out: written with the first half alone, this case survived the revert. + const atCap = idsFor(STUB_CAP); + const { unmount } = renderHook(() => + useRecordCrudVerdicts({ objectName: OBJECT, recordIds: atCap, update: true }), + ); await waitFor(() => expect(calls.length).toBe(1)); - expect(calls[0].recordIds).toEqual(ids); + expect(calls[0].recordIds).toEqual(atCap); + + unmount(); + __clearRecordCrudVerdictCache(); + calls = []; + + const overCap = idsFor(STUB_CAP + 1); + renderHook(() => useRecordCrudVerdicts({ objectName: OBJECT, recordIds: overCap, update: true })); + await waitFor(() => expect(calls.length).toBe(2)); + expect(calls.map((c) => c.recordIds?.length)).toEqual([STUB_CAP, 1]); }); it('splits per operation, so two verbs over cap+1 ids cost four requests', async () => {