fix(types,react,plugin-form): narrow RecordContextValue.recordId and DataSource.update's id to the protocol's string

.changeset/9333-record-id-narrows-to-string.md

@@ -0,0 +1,93 @@
+---
+'@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
+---
+
+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 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
+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`.
+- `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)`
+  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.
+
+## 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
+`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:
+
+- `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`
+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.
+
+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.

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<string, any>;
   /** Previous state (for rollback) */

packages/data-objectstack/src/userState.ts

@@ -74,7 +74,13 @@ export interface UserDataAdapter<T> {
 }
 
 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<T = unknown>(
 
   // 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

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 extends true> = T;
+
+type ParentIdParam = Parameters<typeof buildMasterDetailEditBatch>[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<RecordContextValue['recordId']> 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<unknown extends ParentIdParam ? true : false>;
+
+/* ------------------------------------------------------------------ *
+ * 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);
+  });
+});

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<Record<string, any>[]>([]);
   const [original, setOriginal] = useState<Record<string, any>[]>([]);

packages/plugin-grid/src/ObjectGrid.tsx

@@ -4114,7 +4114,11 @@ export const ObjectGrid: React.FC<ObjectGridComponentProps> = ({
   // 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 (

packages/react/src/context/RecordContext.tsx

@@ -113,8 +113,26 @@ const EMPTY_SET: ReadonlySet<string> = new Set<string>();
 export interface RecordContextValue<TData = any, TObjectSchema = any> {
   /** 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.
+   *
+   * `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;
   /**
    * 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 +189,34 @@ export interface RecordContextValue<TData = any, TObjectSchema = any> {
 
 const RecordContext = React.createContext<RecordContextValue | null>(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<RecordContextValue, 'recordId'> {
+  /** 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<RecordContextProviderProps> = ({
   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<RecordContextValue>(() => value, [