From 409b565abad31c9f9e963fa4b3d24f23b4ab5e35 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 11:26:41 +0000 Subject: [PATCH 1/4] fix(types,react,plugin-form): narrow the two record-id declarations to the protocol's string `@objectstack/spec` declares a record id as `z.string()` on every record door. Two consumer types were wider: `RecordContextValue.recordId` was `string | number | null | undefined` and `DataSource.update`'s `id` was `string | number`. A host mounting a record with a numeric primary key therefore reached `buildMasterDetailEditBatch(parentId: string)` as a number, kept meeting it only through a type assertion in `LineItemsPanel`, and that assertion was the evidence objectui#9304 left behind. - `RecordContextValue.recordId` is `string | null | undefined`. `RecordContextProviderProps` keeps the wider `string | number | null | undefined`, and `RecordContextProvider` pays the conversion once, typed, at that injection boundary -- so no read site carries a `String(...)` or a cast. The conversion is `typeof`-gated: `null` / `undefined` stay themselves ("no record bound" is not the id "undefined"), and a numeric `0` is a real key rather than an absent one. - `DataSource.update`'s `id` is `string`. Implementors are unaffected (TypeScript compares method parameters bivariantly); callers must hand over a string, which is where a numeric-key backend maps at its own adapter. - `LineItemsPanel` drops the assertion. The declarations now meet on their own. Three pins, each split into a compile-time half that only `tsc -p tsconfig.test.json` executes and a runtime half vitest executes, so a green run of one is never read as a reading on the other. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01L5xpA5q533BgTTNADibEFt --- .../9333-record-id-narrows-to-string.md | 51 +++++++ ...LineItemsPanel.parentIdNoCast-9333.test.ts | 129 +++++++++++++++++ packages/plugin-form/src/LineItemsPanel.tsx | 22 +-- packages/react/src/context/RecordContext.tsx | 40 +++++- ...cordContext.recordIdNarrowed-9333.test.tsx | 135 ++++++++++++++++++ .../data-source-update-id-9333.test.ts | 128 +++++++++++++++++ packages/types/src/data.ts | 7 +- 7 files changed, 491 insertions(+), 21 deletions(-) create mode 100644 .changeset/9333-record-id-narrows-to-string.md create mode 100644 packages/plugin-form/src/LineItemsPanel.parentIdNoCast-9333.test.ts create mode 100644 packages/react/src/context/__tests__/RecordContext.recordIdNarrowed-9333.test.tsx create mode 100644 packages/types/src/__tests__/data-source-update-id-9333.test.ts diff --git a/.changeset/9333-record-id-narrows-to-string.md b/.changeset/9333-record-id-narrows-to-string.md new file mode 100644 index 0000000000..3547b70edf --- /dev/null +++ b/.changeset/9333-record-id-narrows-to-string.md @@ -0,0 +1,51 @@ +--- +'@object-ui/types': minor +'@object-ui/react': minor +'@object-ui/plugin-form': patch +--- + +Narrow the two record-id declarations that were wider than the protocol: +`RecordContextValue.recordId` and `DataSource.update`'s `id` are now `string` +(objectui#9333, director seat decision batch #129 item 5, 2026-09-13). + +⚠️ **BREAKING for any code that hands a numeric primary key to either +declaration.** Ships as `minor` per the launch-window convention: objectui's +`major` is a cross-repo pin to `@objectstack`'s so that "same major means +compatible" holds across the two repos +(`scripts/check-changeset-no-major.mjs`), and objectui's own breaking changes +ship as `minor` with the break named where it lands — this entry is the channel +that carries it. + +## What changed + +- `RecordContextValue.recordId` (`@object-ui/react`) was + `string | number | null | undefined`; it is now `string | null | undefined`. +- `DataSource.update`'s `id` parameter (`@object-ui/types`) was + `string | number`; it is now `string`. +- `LineItemsPanel` (`@object-ui/plugin-form`) drops the type assertion + objectui#9304 left on its parent id. That assertion was the only thing making + the context declaration and `buildMasterDetailEditBatch(parentId: string)` + meet; the declaration now does it, so the evidence is discharged. + +## Why the protocol, and not a wider consumer type + +`@objectstack/spec` declares a record id as `z.string()` on every record door — +get, update, delete and the batch operation. A consumer type may not be wider +than the protocol: a declaration that admits `number` promises callers something +the wire never carries, and the promise is kept only by an assertion at the far +end, which is what this card was filed about. + +## Migration — no `String(...)` at your call sites + +`RecordContextProvider` still **accepts** `string | number | null | undefined` +and narrows it once, itself. A host that mounts a record with a numeric primary +key therefore changes nothing: the conversion is paid at that injection +boundary, typed, in one place. Consumers of `useRecordContext()` read a +`string`. + +`DataSource` implementors are unaffected — TypeScript compares method parameters +bivariantly, so an adapter that still declares `id: string | number` continues +to satisfy the interface. What changes is the **caller** side: a call that passes +a `string | number` to `dataSource.update` is now a type error. A backend whose +primary keys are numeric maps them at its own adapter boundary rather than +pushing the union through every caller. diff --git a/packages/plugin-form/src/LineItemsPanel.parentIdNoCast-9333.test.ts b/packages/plugin-form/src/LineItemsPanel.parentIdNoCast-9333.test.ts new file mode 100644 index 0000000000..db604ab37a --- /dev/null +++ b/packages/plugin-form/src/LineItemsPanel.parentIdNoCast-9333.test.ts @@ -0,0 +1,129 @@ +/** + * 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#9333 - the line-items parent id needs no assertion, because the two + * declarations now agree. + * + * ## What was here before + * + * objectui#9304 removed the whole-context assertions on `useRecordContext()` + * and found that the INNER assertion on the parent id was not redundant: with + * it deleted, `tsc` answered TS2345, `string | number` is not assignable to + * `string`. It kept the assertion and documented it as evidence, because both + * repairs available inside that card moved bytes on the wire. + * + * The ruling on objectui#9333 discharged that evidence by repairing the + * DECLARATION instead: `RecordContextValue.recordId` is the protocol's + * `string`, narrowed once at the `RecordContextProvider` injection boundary. + * The assertion is therefore gone, and this file pins the two halves of "gone" + * that fail for different reasons. + * + * ## Where each row lives + * + * The `type _...` row is erased before vitest loads this file; `tsc -p + * packages/plugin-form/tsconfig.test.json` is the only thing that executes it, + * and it resolves `@object-ui/react` through the built `.d.ts` rather than + * sibling sources (this project drops the root `paths`). The `it(...)` rows + * are a source-text census over `LineItemsPanel.tsx` and run under vitest; a + * green run of one is not a reading on the other. + */ + +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import type { RecordContextValue } from '@object-ui/react'; +import type { buildMasterDetailEditBatch } from './masterDetailTx'; + +/* ------------------------------------------------------------------ * + * Compile-time half. + * ------------------------------------------------------------------ */ + +type Expect = T; + +type ParentIdParam = Parameters[1]; + +/** + * The whole point of the card: a record id read off the context fits the + * helper's parameter with no assertion in between. Widen either declaration + * and this row reds. + */ +type _ContextRecordIdFitsParentId = Expect< + NonNullable extends ParentIdParam ? true : false +>; + +/** + * Control: the row above is not vacuously true through an `any` on either + * side. `unknown` is assignable to neither, so a degraded `ParentIdParam` + * would make this directive UNUSED (TS2578). + */ +// @ts-expect-error `unknown` is not a `buildMasterDetailEditBatch` parent id +type _UnknownIsRefusedAsParentId = Expect; + +/* ------------------------------------------------------------------ * + * Runtime half - the assertion has not come back. + * ------------------------------------------------------------------ */ + +const here = path.dirname(fileURLToPath(import.meta.url)); +const PANEL = path.join(here, 'LineItemsPanel.tsx'); + +/** Strip `//` and block comments so prose about the old defect is not a hit. */ +function maskComments(src: string): string { + return src.replace(/\/\*[\s\S]*?\*\//g, (m) => m.replace(/[^\n]/g, ' ')).replace(/\/\/[^\n]*/g, ''); +} + +/** A type assertion applied to any `...recordId` read. */ +const RECORD_ID_ASSERTION = /\brecord(?:\?)?\.recordId\s*\)?\s*\bas\b/g; + +function assertionCount(src: string): number { + RECORD_ID_ASSERTION.lastIndex = 0; + return (maskComments(src).match(RECORD_ID_ASSERTION) || []).length; +} + +const PANEL_TEXT = readFileSync(PANEL, 'utf8'); + +describe('the census instrument can fire (controls)', () => { + it('anchors on this file, not on the cwd', () => { + expect(PANEL_TEXT).toContain('LineItemsPanel'); + expect(PANEL_TEXT).toContain('useRecordContext'); + }); + + it('flags the assertion this card removed', () => { + // Assembled from fragments so the control is not itself a census hit if + // the scan is ever widened to this file. + const wide = 'const parentId = schema.parentId || (record?.recordId' + ' as ' + 'string);'; + expect(assertionCount(wide)).toBe(1); + }); + + it('does not flag the repaired read', () => { + expect(assertionCount('const parentId = schema.parentId || record?.recordId;')).toBe(0); + }); + + it('masks comments, so prose about the old defect is not a defect', () => { + const commented = '// record?.recordId' + ' as ' + 'string\nconst x = 1;'; + expect(assertionCount(commented)).toBe(0); + const block = '/* record?.recordId' + ' as ' + 'string */\nconst x = 1;'; + expect(assertionCount(block)).toBe(0); + }); +}); + +describe('LineItemsPanel reads the parent id through the declaration (objectui#9333)', () => { + it('carries no type assertion on the record-context id', () => { + expect( + assertionCount(PANEL_TEXT), + [ + 'The parent id is asserted again.', + '`RecordContextValue.recordId` is the protocol`s `string` and', + '`buildMasterDetailEditBatch` takes a `string` parent id, so the two', + 'meet without help. An assertion here means one of those declarations', + 'moved - repair the declaration, not the read site (objectui#9333).', + ].join('\n'), + ).toBe(0); + }); +}); diff --git a/packages/plugin-form/src/LineItemsPanel.tsx b/packages/plugin-form/src/LineItemsPanel.tsx index 927ae6f286..328f1a4c01 100644 --- a/packages/plugin-form/src/LineItemsPanel.tsx +++ b/packages/plugin-form/src/LineItemsPanel.tsx @@ -94,21 +94,13 @@ export const LineItemsPanel: React.FC<{ schema: LineItemsPanelSchema }> = ({ sch const record = useRecordContext(); const parentObject = schema.parentObject || record?.objectName; - // The assertion below is LOAD-BEARING, and only became so when the - // whole-context assertion on `record` was removed (objectui#9304). While the - // binding was `any` it did nothing at all; now `RecordContextValue.recordId` - // is declared `string | number | null | undefined` and - // `buildMasterDetailEditBatch` takes a `string` parent id, so dropping it is - // a real error rather than a tidy-up — measured: TS2345, `string | number` - // is not assignable to `string`. - // - // Kept rather than repaired here because both repairs move bytes on the wire - // for a numeric primary key (coercing with `String()` changes the id this - // panel sends; widening `masterDetailTx`'s parameter is that module's - // contract, not this one's), and this change is type-side with no runtime - // effect. The residue is tracked separately. - const parentId = - schema.parentId || schema.recordId || (record?.recordId as string | undefined); + // No assertion: `RecordContextValue.recordId` is the protocol's `string` + // (narrowed once at the `RecordContextProvider` injection boundary) and + // `buildMasterDetailEditBatch` takes a `string` parent id, so the two + // declarations meet on their own. objectui#9304 left a documented assertion + // here as evidence that they did not; objectui#9333 repaired the declaration + // and discharged the evidence. + const parentId = schema.parentId || schema.recordId || record?.recordId; const [rows, setRows] = useState[]>([]); const [original, setOriginal] = useState[]>([]); diff --git a/packages/react/src/context/RecordContext.tsx b/packages/react/src/context/RecordContext.tsx index ac5d741af6..02dcd5d8dd 100644 --- a/packages/react/src/context/RecordContext.tsx +++ b/packages/react/src/context/RecordContext.tsx @@ -113,8 +113,20 @@ const EMPTY_SET: ReadonlySet = new Set(); export interface RecordContextValue { /** Object machine name, e.g. "crm_opportunity". */ objectName: string; - /** Primary key value of the record being displayed. */ - recordId: string | number | null | undefined; + /** + * Primary key value of the record being displayed, as a `string` -- the one + * spelling `@objectstack/spec` declares on every record door (get, update, + * delete and the batch operation are all `z.string()`), so this consumer + * type is no longer wider than the protocol (objectui#9333). + * + * A host whose primary keys are numeric does NOT stringify at its call + * sites: `RecordContextProviderProps` still accepts `string | number`, and + * the provider pays the conversion once, below. Everything downstream -- + * `record:*` renderers, `LineItemsPanel`'s parent id -- reads a `string` and + * hands it to a `string` parameter with no assertion in between, which is + * what objectui#9304 had to leave behind. + */ + recordId: string | null | undefined; /** * The data adapter the page is bound to, as the host resolved it — the same * object the host hands `SchemaRendererProvider`, forwarded so that @@ -171,14 +183,34 @@ export interface RecordContextValue { const RecordContext = React.createContext(null); -export interface RecordContextProviderProps extends RecordContextValue { +/** + * Props of `RecordContextProvider` -- THE injection boundary (objectui#9333). + * + * Identical to `RecordContextValue` except for `recordId`, the one member a + * host may still hand over in the wider shape its backend actually holds. The + * provider narrows it to the protocol's `string` exactly once, here, so no + * consumer downstream carries a `String(...)` or a cast. + */ +export interface RecordContextProviderProps extends Omit { + /** Primary key as the host holds it; narrowed to `string` on the way in. */ + recordId: string | number | null | undefined; children: React.ReactNode; } export const RecordContextProvider: React.FC = ({ children, - ...value + recordId, + ...rest }) => { + // The whole conversion for this contract, in one typed place. Gated on + // `typeof` rather than written as an unconditional `String(...)`: `null` and + // `undefined` mean "no record bound", and stringifying them would hand + // consumers the literal ids "null" / "undefined". A falsiness gate would be + // wrong in the other direction -- a numeric `0` is a real primary key. + const value: RecordContextValue = { + ...rest, + recordId: typeof recordId === 'number' ? String(recordId) : recordId, + }; // Memoize so consumers that rely on referential equality don't re-render // on unrelated parent renders. const memo = React.useMemo(() => value, [ diff --git a/packages/react/src/context/__tests__/RecordContext.recordIdNarrowed-9333.test.tsx b/packages/react/src/context/__tests__/RecordContext.recordIdNarrowed-9333.test.tsx new file mode 100644 index 0000000000..891b233a9c --- /dev/null +++ b/packages/react/src/context/__tests__/RecordContext.recordIdNarrowed-9333.test.tsx @@ -0,0 +1,135 @@ +/** + * 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#9333 - `RecordContextValue.recordId` is a `string`, and the ONE + * place a wider host value is narrowed is this provider. + * + * ## What is pinned + * + * `@objectstack/spec` declares a record id as `z.string()` on every record + * door (get, update, delete, and the batch operation), so a consumer type may + * not be wider than the protocol (director seat, decision batch #129 item 5). + * `RecordContextValue.recordId` used to be `string | number | null | + * undefined`, which is how a numeric primary key reached + * `buildMasterDetailEditBatch` - declared `parentId: string` - and why + * `LineItemsPanel` carried a type assertion to make the two meet. + * + * The repair has a shape, not just a direction: the conversion is paid ONCE, + * at the injection boundary, and never as `String(...)` at a read site. So + * this file pins BOTH halves, and each half reddens on a different mistake: + * + * - narrow the READ type but drop the conversion -> the runtime half reds + * (a consumer sees the number it was handed). + * - keep the conversion but widen the READ type back -> the compile-time + * half reds (`Equal` fails and the `@ts-expect-error` control goes unused, + * TS2578). + * - narrow the PROVIDER PROP too, pushing `String(...)` out to hosts -> the + * prop-shape assertion below reds. + * + * ## Where each row lives + * + * The `type _...` rows are erased before vitest ever sees this file; `tsc -p + * packages/react/tsconfig.test.json` is the only thing that executes them. The + * `it(...)` rows are the runtime half and run under vitest. A green vitest run + * is therefore NOT a reading on the type rows, and vice versa. + */ + +import * as React from 'react'; +import { describe, it, expect } from 'vitest'; +import { render } from '@testing-library/react'; +import { + RecordContextProvider, + useRecordContext, + type RecordContextValue, + type RecordContextProviderProps, +} from '../RecordContext'; + +/* ------------------------------------------------------------------ * + * Compile-time half. + * ------------------------------------------------------------------ */ + +type Equal = + (() => T extends X ? 1 : 2) extends (() => T extends Y ? 1 : 2) ? true : false; +type Expect = T; + +/** The read surface every `record:*` consumer gets: the protocol's `string`. */ +type _RecordIdIsTheProtocolString = Expect< + Equal +>; + +/** Stated separately so a degradation to `any` cannot pass the row above. */ +type _RecordIdIsNotAny = Expect, false>>; + +/** + * The injection boundary keeps the wider shape. Removing this is the + * `String(...)`-at-call-sites anti-pattern the ruling names in its own title, + * so it is pinned rather than left to review. + */ +type _ProviderPropStaysWide = Expect< + Equal +>; + +/** + * Control: this file's checker really resolves the declaration. Widen + * `recordId` back and the directive becomes UNUSED (TS2578) - i.e. the control + * fires for the same reason the assertions above would stop meaning anything. + */ +// @ts-expect-error a numeric primary key is no longer a `RecordContextValue.recordId` +const _numericRecordIdIsRefused: RecordContextValue['recordId'] = 7; +void _numericRecordIdIsRefused; + +/* ------------------------------------------------------------------ * + * Runtime half - the conversion is really paid, and paid here. + * ------------------------------------------------------------------ */ + +function renderWithProbe(recordId: RecordContextProviderProps['recordId']) { + const seen: Array = []; + const Probe: React.FC = () => { + seen.push(useRecordContext()); + return null; + }; + render( + + + , + ); + return seen; +} + +describe('RecordContextProvider narrows the host record id (objectui#9333)', () => { + it('hands a numeric primary key to consumers as a string', () => { + const seen = renderWithProbe(42); + // Removing the conversion in the provider makes this the number 42: the + // control that proves this row is not vacuous. + expect(seen[0]!.recordId).toBe('42'); + expect(typeof seen[0]!.recordId).toBe('string'); + }); + + it('leaves a string primary key byte-identical', () => { + const seen = renderWithProbe('rec_1'); + expect(seen[0]!.recordId).toBe('rec_1'); + }); + + it('keeps "no record bound" distinguishable from a stringified nothing', () => { + // `String(undefined)` is the string "undefined" and `String(null)` is + // "null" - both would be read downstream as a real id. The conversion is + // deliberately typeof-gated, and these two rows are what reds if someone + // simplifies it to an unconditional `String(...)`. + expect(renderWithProbe(undefined)[0]!.recordId).toBeUndefined(); + expect(renderWithProbe(null)[0]!.recordId).toBeNull(); + }); + + it('narrows zero rather than treating it as absent', () => { + // `0` is falsy; a conversion written as `recordId && String(recordId)` + // would hand back `0` (a number) and pass the first test in this file. + const seen = renderWithProbe(0); + expect(seen[0]!.recordId).toBe('0'); + expect(typeof seen[0]!.recordId).toBe('string'); + }); +}); diff --git a/packages/types/src/__tests__/data-source-update-id-9333.test.ts b/packages/types/src/__tests__/data-source-update-id-9333.test.ts new file mode 100644 index 0000000000..dbb34ba4ed --- /dev/null +++ b/packages/types/src/__tests__/data-source-update-id-9333.test.ts @@ -0,0 +1,128 @@ +/** + * 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#9333 - `DataSource.update` takes the protocol's `string` record id. + * + * ## What is pinned, and why the adapter contract is the second half of one card + * + * `@objectstack/spec` declares a record id as `z.string()` on every record + * door. `RecordContextValue.recordId` was one consumer type wider than that; + * `DataSource.update`'s `id: string | number` is the same defect class on the + * adapter contract, ruled to narrow in the same card (director seat, decision + * batch #129 item 5). A backend whose primary keys are numeric maps at ITS + * adapter boundary rather than making every caller in this monorepo carry a + * union the protocol does not have. + * + * ## Where each row lives - these two halves measure different things + * + * The `type _...` rows below are ERASED before vitest loads this file. `tsc -p + * packages/types/tsconfig.test.json` (chained from this package's `type-check` + * script) is the only thing that executes them, which is the objectui#3009 + * lesson: a green vitest run says nothing about a compile-time assertion. + * + * The `it(...)` rows are a source-text census over the declaration itself, and + * they are NOT redundant with the type rows: they red even in a tree where + * `tsc` is never run, and they name the offending spelling in the failure + * message. Every matcher below is proven to fire on a synthetic wide signature + * before any absence is believed. + */ + +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import type { DataSource } from '../data'; + +/* ------------------------------------------------------------------ * + * Compile-time half. + * ------------------------------------------------------------------ */ + +type Equal = + (() => T extends X ? 1 : 2) extends (() => T extends Y ? 1 : 2) ? true : false; +type Expect = T; + +/** The ruled narrowing. */ +type _UpdateIdIsTheProtocolString = Expect[1], string>>; + +/** Stated separately so a degradation to `any` cannot pass the row above. */ +type _UpdateIdIsNotAny = Expect[1], any>, false>>; + +declare const adapter: DataSource; + +/** + * Two call-site controls, held inside a function that is referenced and never + * invoked. `declare const adapter` is erased, so the body would throw if it + * ever ran - keeping it unreachable is what makes these rows compile-time-only + * in a file vitest also loads. + * + * Control: the checker really resolves this signature. Widen `id` back and the + * directive becomes UNUSED (TS2578), so the control fires for exactly the + * change it exists to catch. The neighbouring string call must stay legal, or + * the refusal above would prove nothing. + */ +function updateIdCallSiteControls(): void { + // @ts-expect-error a numeric primary key is no longer a `DataSource.update` id + void adapter.update('account', 7, {}); + void adapter.update('account', 'rec_1', {}); +} +void updateIdCallSiteControls; + +/* ------------------------------------------------------------------ * + * Runtime half - a census over the declaration text. + * ------------------------------------------------------------------ */ + +const here = path.dirname(fileURLToPath(import.meta.url)); +// packages/types/src/__tests__ -> packages/types/src +const SRC = path.resolve(here, '..'); +const DATA_TS = path.join(SRC, 'data.ts'); + +/** `update(resource: string, id: ,` - whitespace and newlines tolerated. */ +const UPDATE_ID = /\bupdate\s*\(\s*resource\s*:\s*string\s*,\s*id\s*:\s*([^,\n]+?)\s*,/s; + +function updateIdTypeOf(source: string): string | null { + const m = UPDATE_ID.exec(source); + return m ? m[1].replace(/\s+/g, ' ') : null; +} + +const DATA_SOURCE_TEXT = readFileSync(DATA_TS, 'utf8'); + +describe('the census instrument can fire (controls)', () => { + it('anchors on this file, not on the cwd', () => { + expect(DATA_SOURCE_TEXT.length).toBeGreaterThan(0); + expect(DATA_SOURCE_TEXT).toContain('export interface DataSource'); + }); + + it('reads the wide spelling back off a synthetic declaration', () => { + const wide = 'update(\n resource: string,\n id: string | number,\n data: X,\n ): Y;'; + expect(updateIdTypeOf(wide)).toBe('string | number'); + }); + + it('reads the narrow spelling back off a synthetic declaration', () => { + const narrow = 'update(\n resource: string,\n id: string,\n data: X,\n ): Y;'; + expect(updateIdTypeOf(narrow)).toBe('string'); + }); + + it('returns null rather than a false negative when the member is absent', () => { + expect(updateIdTypeOf('export interface Empty {}')).toBeNull(); + }); +}); + +describe('DataSource.update declares the protocol record id (objectui#9333)', () => { + it('takes `id: string`, not a union with `number`', () => { + expect( + updateIdTypeOf(DATA_SOURCE_TEXT), + [ + '`DataSource.update` declares a record id wider than the protocol.', + '@objectstack/spec declares every record door as `z.string()`; a backend', + 'whose keys are numeric maps at its own adapter boundary rather than', + 'widening this contract for every caller in the monorepo.', + ].join('\n'), + ).toBe('string'); + }); +}); diff --git a/packages/types/src/data.ts b/packages/types/src/data.ts index d17f573929..edeee028d7 100644 --- a/packages/types/src/data.ts +++ b/packages/types/src/data.ts @@ -404,7 +404,10 @@ export interface DataSource { * Update an existing record. * * @param resource - Resource name - * @param id - Record identifier + * @param id - Record identifier. A `string`, as `@objectstack/spec` declares + * every record door; an adapter for a backend whose primary keys are + * numeric maps at its own boundary rather than widening this contract for + * every caller (objectui#9333). * @param data - Updated data (partial) * @param opts - Optional write options. Pass `opts.ifMatch` to enable * Optimistic Concurrency Control: the implementation forwards the @@ -417,7 +420,7 @@ export interface DataSource { */ update( resource: string, - id: string | number, + id: string, data: Partial, opts?: { ifMatch?: string }, ): Promise; From f4ce0fe40f3de16282f7e7f83a1797d7b24d7206 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 13:15:56 +0000 Subject: [PATCH 2/4] fix(core,data-objectstack,plugin-grid): narrow the three id declarations the DataSource.update narrowing reached Consequence of narrowing `DataSource.update`'s `id` to the protocol's `string`. Narrowing an interface PARAMETER never reaches implementors (TypeScript compares method parameters bivariantly); it reaches callers. A local type-check of every workspace type-check program found exactly six call sites, in three packages, each red because a further declaration one layer in was itself wider than the protocol. All three narrow here, types only -- no runtime change, no coercion added at any call site, and nothing re-widened: - `TransactionOperation.id` is `string`. Its sibling `BatchTransactionOperation.id` already was; the two now agree. Re-exported through `@object-ui/core`'s barrel, so this is a breaking narrowing on that package too, declared in the changeset. - `UserPreferenceRecord.id` and the `cachedRowId` it feeds are `string`. Module-local. These rows are read back off the protocol, so the union was a claim the wire never makes. - `resolveRecordId`'s return type is `string | undefined`. Module-local. It annotates `any`-typed row data, so the union was an assertion, not a measurement. Reproduced before repairing: `packages/core` build exit 2 with `src/actions/TransactionManager.ts(469,57): error TS2345`. After: exit 0. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01L5xpA5q533BgTTNADibEFt --- .../9333-record-id-narrows-to-string.md | 25 +++++++++++++++++++ .../core/src/actions/TransactionManager.ts | 9 +++++-- packages/data-objectstack/src/userState.ts | 10 ++++++-- packages/plugin-grid/src/ObjectGrid.tsx | 6 ++++- 4 files changed, 45 insertions(+), 5 deletions(-) diff --git a/.changeset/9333-record-id-narrows-to-string.md b/.changeset/9333-record-id-narrows-to-string.md index 3547b70edf..9a9f77f197 100644 --- a/.changeset/9333-record-id-narrows-to-string.md +++ b/.changeset/9333-record-id-narrows-to-string.md @@ -1,7 +1,10 @@ --- '@object-ui/types': minor '@object-ui/react': minor +'@object-ui/core': minor '@object-ui/plugin-form': patch +'@object-ui/data-objectstack': patch +'@object-ui/plugin-grid': patch --- Narrow the two record-id declarations that were wider than the protocol: @@ -35,6 +38,28 @@ than the protocol: a declaration that admits `number` promises callers something the wire never carries, and the promise is kept only by an assertion at the far end, which is what this card was filed about. +## The consumers the narrowing reached, and what they cost + +Narrowing an interface **parameter** never reaches implementors — TypeScript +compares method parameters bivariantly, so an adapter that still declares +`id: string | number` keeps satisfying `DataSource`. It reaches **callers**. A +full local type-check of every workspace type-check program found exactly six, +in three packages, and each was red because a further declaration one layer in +was itself wider than the protocol. All three are narrowed here, types only, with +no runtime change and no coercion added at any call site: + +- `TransactionOperation.id` (`@object-ui/core`) is `string`. ⚠️ **Published**: + it is re-exported through `@object-ui/core`'s barrel, so this is a breaking + narrowing on that package too. Its own sibling `BatchTransactionOperation.id` + was already `string`; the two now agree. +- `UserPreferenceRecord.id` and the `cachedRowId` it feeds + (`@object-ui/data-objectstack`) are `string`. Module-local, not published — + these rows are read back off the protocol, so the union was a claim the wire + never makes. +- `resolveRecordId`'s return type (`@object-ui/plugin-grid`) is + `string | undefined`. Module-local, not published — it annotates `any`-typed + row data, so the union was an assertion rather than a measurement. + ## Migration — no `String(...)` at your call sites `RecordContextProvider` still **accepts** `string | number | null | undefined` diff --git a/packages/core/src/actions/TransactionManager.ts b/packages/core/src/actions/TransactionManager.ts index 2aebfb5f9b..c1d32a950e 100644 --- a/packages/core/src/actions/TransactionManager.ts +++ b/packages/core/src/actions/TransactionManager.ts @@ -33,8 +33,13 @@ export interface TransactionOperation { type: 'create' | 'update' | 'delete'; /** Target resource name */ resource: string; - /** Record ID (for update/delete) */ - id?: string | number; + /** + * Record ID (for update/delete). A `string`, as `@objectstack/spec` declares + * every record door and as this type's own sibling `BatchTransactionOperation.id` + * already did; the rollback path hands it straight to `DataSource.update` + * (objectui#9333). + */ + id?: string; /** Data payload */ data?: Record; /** Previous state (for rollback) */ diff --git a/packages/data-objectstack/src/userState.ts b/packages/data-objectstack/src/userState.ts index a4a6af4a62..6ec15fe388 100644 --- a/packages/data-objectstack/src/userState.ts +++ b/packages/data-objectstack/src/userState.ts @@ -74,7 +74,13 @@ export interface UserDataAdapter { } interface UserPreferenceRecord { - id?: string | number; + /** + * Row id, as `@objectstack/spec` declares every record door: a `string` + * (objectui#9333). These rows are read back off the protocol and handed to + * `DataSource.update`, so the union this used to carry was a claim the wire + * never makes. + */ + id?: string; user_id: string; key: string; value: unknown; @@ -102,7 +108,7 @@ export function createObjectStackUserStateAdapter( // Cache the row id between load() and save() so we can update in place // without re-querying. Reset on every successful load. - let cachedRowId: string | number | null = null; + let cachedRowId: string | null = null; // Serializes overlapping save() calls. A fresh adapter is created whenever // the data source / user changes (see UserStateBridge), so its cachedRowId diff --git a/packages/plugin-grid/src/ObjectGrid.tsx b/packages/plugin-grid/src/ObjectGrid.tsx index 1d7b9e5c1b..b752a55980 100644 --- a/packages/plugin-grid/src/ObjectGrid.tsx +++ b/packages/plugin-grid/src/ObjectGrid.tsx @@ -4114,7 +4114,11 @@ export const ObjectGrid: React.FC = ({ // refresh so the grid reflects persisted values. Throwing on failure is // important: DataTable's saveRow/saveBatch keep pending changes when the save // promise rejects, so a failed write doesn't silently lose the user's edits. - const resolveRecordId = (row: any): string | number | undefined => + // The one place a row's primary key is read for a write. `string`, as + // `@objectstack/spec` declares every record door — the union this used to + // annotate was a claim about `any`-typed row data, not a measurement of it + // (objectui#9333). + const resolveRecordId = (row: any): string | undefined => row?._id ?? row?.id; const defaultRowSave = async ( From b58b36ecc1d9012fcf85cde1a0a4601df006be57 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 13:30:27 +0000 Subject: [PATCH 3/4] docs(changeset): name the @object-ui/core break in a consumer's language The `TransactionOperation.id` narrowing is a breaking change for anyone importing that type from `@object-ui/core`, not only a knock-on of the `DataSource.update` narrowing. It was described under an internal-consequence heading, so a reader of the `@object-ui/core` changelog would have seen a `minor` bump whose headline named two other packages' declarations. All three published breaks now sit in one BREAKING list at the top, each said in terms of what a consumer does -- the value `useRecordContext()` returns, the argument `dataSource.update` takes, the object `recordOperation()` accepts -- and the migration says where to convert a numeric key (in the adapter that knows the backend's key type, not at each call site). The remaining section is relabelled as what it is: internal consumers repaired in the same change, moving no public contract. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01L5xpA5q533BgTTNADibEFt --- .../9333-record-id-narrows-to-string.md | 37 ++++++++++++++----- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/.changeset/9333-record-id-narrows-to-string.md b/.changeset/9333-record-id-narrows-to-string.md index 9a9f77f197..ec01ce5d95 100644 --- a/.changeset/9333-record-id-narrows-to-string.md +++ b/.changeset/9333-record-id-narrows-to-string.md @@ -7,12 +7,24 @@ '@object-ui/plugin-grid': patch --- -Narrow the two record-id declarations that were wider than the protocol: -`RecordContextValue.recordId` and `DataSource.update`'s `id` are now `string` -(objectui#9333, director seat decision batch #129 item 5, 2026-09-13). +A record id is a `string` everywhere in the published types, as +`@objectstack/spec` has always declared it. Three published declarations that +admitted `number` no longer do. -⚠️ **BREAKING for any code that hands a numeric primary key to either -declaration.** Ships as `minor` per the launch-window convention: objectui's +⚠️ **BREAKING if your code hands a numeric primary key to any of these three:** + +- **`RecordContextValue.recordId`** (`@object-ui/react`) — the value + `useRecordContext()` gives you is now a `string`, never a `number`. +- **`DataSource.update`'s `id` parameter** (`@object-ui/types`) — a call that + passes a `string | number` is now a type error. Adapters that *implement* + `DataSource` are unaffected (see Migration). +- **`TransactionOperation.id`** (`@object-ui/core`) — the operation record you + hand to `TransactionManager.recordOperation()` must carry a `string` id. If + you build that object from a numeric key, convert it where you build it. This + type is exported from the package root, so this is a breaking change for + `@object-ui/core` consumers in its own right, not just a knock-on. + +Ships as `minor` per the launch-window convention: objectui's `major` is a cross-repo pin to `@objectstack`'s so that "same major means compatible" holds across the two repos (`scripts/check-changeset-no-major.mjs`), and objectui's own breaking changes @@ -25,6 +37,9 @@ that carries it. `string | number | null | undefined`; it is now `string | null | undefined`. - `DataSource.update`'s `id` parameter (`@object-ui/types`) was `string | number`; it is now `string`. +- `TransactionOperation.id` (`@object-ui/core`) was `string | number`; it is now + `string`. Its sibling `BatchTransactionOperation.id` was already a `string`, + so the two operation records finally agree. - `LineItemsPanel` (`@object-ui/plugin-form`) drops the type assertion objectui#9304 left on its parent id. That assertion was the only thing making the context declaration and `buildMasterDetailEditBatch(parentId: string)` @@ -38,7 +53,7 @@ than the protocol: a declaration that admits `number` promises callers something the wire never carries, and the promise is kept only by an assertion at the far end, which is what this card was filed about. -## The consumers the narrowing reached, and what they cost +## Internal consumers repaired at the same time (no public contract moves) Narrowing an interface **parameter** never reaches implementors — TypeScript compares method parameters bivariantly, so an adapter that still declares @@ -48,10 +63,6 @@ in three packages, and each was red because a further declaration one layer in was itself wider than the protocol. All three are narrowed here, types only, with no runtime change and no coercion added at any call site: -- `TransactionOperation.id` (`@object-ui/core`) is `string`. ⚠️ **Published**: - it is re-exported through `@object-ui/core`'s barrel, so this is a breaking - narrowing on that package too. Its own sibling `BatchTransactionOperation.id` - was already `string`; the two now agree. - `UserPreferenceRecord.id` and the `cachedRowId` it feeds (`@object-ui/data-objectstack`) are `string`. Module-local, not published — these rows are read back off the protocol, so the union was a claim the wire @@ -74,3 +85,9 @@ to satisfy the interface. What changes is the **caller** side: a call that passe a `string | number` to `dataSource.update` is now a type error. A backend whose primary keys are numeric maps them at its own adapter boundary rather than pushing the union through every caller. + +For `TransactionOperation`, the same rule applies one level up: build the +operation record with a `string` id. If the id arrives from a numeric-keyed +backend, convert it in your adapter — the one place that knows the backend's key +type — rather than at each `recordOperation()` call. Nothing about this change +alters what is sent over the wire; only the declarations moved. From 1b67972584c0df682669f290cbbc346038149297 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 14:06:13 +0000 Subject: [PATCH 4/4] docs(react): say what was measured about RecordContextValue.recordId, not "everything" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Contract review round 1, item 3. The TSDoc on this member ships in `@object-ui/react`'s published `.d.ts`, so a consumer reads it in their editor, and it asserted that "everything downstream -- `record:*` renderers, `LineItemsPanel`'s parent id -- reads a `string` ... with no assertion in between". Three `record:*` renderers in `@object-ui/plugin-detail` (`record-details`, `record-quick-actions`, `record-alert`) read exactly this member and still cast it, all three present before this change and untouched by it -- so the sentence was false as published. ⛔ The casts are deliberately NOT removed: `plugin-detail` is outside both the ruling's enumeration and this change's authorised surface, and the residual is recorded rather than repaired. What changes is the claim: the sentence now says what was measured -- `LineItemsPanel` -- and names the three readers that still cast, so the next card inherits a measured population instead of a false universal. A universal quantifier is the one construction whose counter-evidence is guaranteed to sit on lines the diff never touched, which is why a review bounded at added lines cannot catch it and why it is worth not writing. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01L5xpA5q533BgTTNADibEFt --- packages/react/src/context/RecordContext.tsx | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/packages/react/src/context/RecordContext.tsx b/packages/react/src/context/RecordContext.tsx index 02dcd5d8dd..63b797a019 100644 --- a/packages/react/src/context/RecordContext.tsx +++ b/packages/react/src/context/RecordContext.tsx @@ -121,10 +121,16 @@ export interface RecordContextValue { * * A host whose primary keys are numeric does NOT stringify at its call * sites: `RecordContextProviderProps` still accepts `string | number`, and - * the provider pays the conversion once, below. Everything downstream -- - * `record:*` renderers, `LineItemsPanel`'s parent id -- reads a `string` and - * hands it to a `string` parameter with no assertion in between, which is - * what objectui#9304 had to leave behind. + * the provider pays the conversion once, below. + * + * `LineItemsPanel` reads this member as a `string` and hands it to a + * `string` parameter with no assertion in between, which is what + * objectui#9304 had to leave behind. That is a statement about that one + * reader, not about every reader: three `record:*` renderers in + * `@object-ui/plugin-detail` -- `record-details`, `record-quick-actions` and + * `record-alert` -- still read it through an `as any`. Those casts are + * redundant now rather than load-bearing, and removing them was left outside + * objectui#9333 deliberately. */ recordId: string | null | undefined; /**