refactor(app-shell): paramToField's reference-bearing rule derives from core, not the last private copy

.changeset/paramtofield-reference-rule-derives-from-core-5312.md

@@ -0,0 +1,33 @@
+---
+'@object-ui/app-shell': patch
+---
+
+`ActionParamDialog`'s "this param carries a reference target" rule now derives from the shared reference-field family instead of the last private copy of it.
+
+`packages/app-shell/src/utils/paramToField.ts` restated the rule inline as
+`LOOKUP_WIDGET_TYPES.has(type) || type === 'user'` — the fourth and last
+hand-maintained answer to one question ("does this widget resolve a foreign key,
+so hand it `reference_to` / `display_field` / the rest of the picker config").
+The other three converged on `@object-ui/core`'s `EXPANDABLE_FIELD_TYPES` in
+objectui#4770 / #4790 / #4815; this face is now the fourth.
+
+No reachable behaviour change. The shared set is one member wider (`tree`), and
+that member can never be a widget key on this surface: it is absent from
+`fields`' widget map and `mapFieldTypeToFormType` sends it to `field:lookup`, so
+every key the rule tests arrives as `lookup`. Both halves are pinned, so
+registering a real `tree` widget surfaces the change instead of shipping it
+silently.
+
+The module's second rule — which widget keys degrade to a text input for want of
+a declared `referenceTo` — is a different set over overlapping types (`user`
+defaults its target to `sys_user` and must never degrade) and was deliberately
+left un-merged, matching the same split the plugin-grid twin keeps.
+
+Also retires a comment that claimed the disjunction "moves in lockstep with
+plugin-grid's `bulkParamToField` twin — the two param faces are never split".
+Measured on the tip before this change, that was false in both senses: the twin
+had read core's Set since objectui#4815 while this line read a private literal,
+so the two shared nothing and no gate could report a split; and the two member
+sets already differed, by `tree`. Lockstep now holds mechanically — the pin is
+on object identity (a spy on core's `has`), so a member-identical private copy
+fails where a value check would pass.

packages/app-shell/src/utils/paramToField.test.ts

@@ -15,9 +15,9 @@
  * `FORM_FIELD_TYPES` + this drift test makes that class of bug impossible to
  * reintroduce silently.
  */
-import { describe, it, expect, vi } from 'vitest';
+import { describe, it, expect, vi, afterEach } from 'vitest';
 import { FORM_FIELD_TYPES } from '@object-ui/fields';
-import type { ActionParamDef } from '@object-ui/core';
+import { EXPANDABLE_FIELD_TYPES, type ActionParamDef } from '@object-ui/core';
 import { paramToField, resolveParamWidgetType } from './paramToField';
 
 const p = (over: Partial<ActionParamDef>): ActionParamDef => ({
@@ -27,6 +27,13 @@ const p = (over: Partial<ActionParamDef>): ActionParamDef => ({
   ...over,
 });
 
+afterEach(() => {
+  // The identity pins below install a spy on the Set object EXPORTED by core —
+  // a shared, module-level object. A leaked spy would follow every later file
+  // in the worker, so restoring is not optional here.
+  vi.restoreAllMocks();
+});
+
 describe('param widget support ⊇ form widget support (drift guard)', () => {
   it('every form field type resolves to its own widget — never the text fallback', () => {
     // If this fails: a type was added to `fieldWidgetMap` that the param
@@ -153,3 +160,103 @@ describe('paramToField', () => {
     expect(paramToField(p({ type: 'user' }))).toMatchObject({ type: 'user' });
   });
 });
+/**
+ * The reference-bearing rule is core's object, not a copy (objectui#5312).
+ *
+ * This module held the FOURTH and last hand-maintained answer to one question —
+ * "does this widget resolve a foreign key, so hand it the reference target?" —
+ * as the inline disjunction `LOOKUP_WIDGET_TYPES.has(type) || type === 'user'`.
+ * The other three converged on `@object-ui/core`'s `EXPANDABLE_FIELD_TYPES` in
+ * objectui#4770 / #4790 / #4815.
+ *
+ * Every membership assertion in this file is satisfied by a private
+ * `new Set(['lookup', 'master_detail', 'user', 'tree'])` holding the same
+ * strings — i.e. by the exact state this change removed. So the load-bearing
+ * pin below is on OBJECT IDENTITY (a spy on core's `has`), not on members.
+ */
+describe("the reference-bearing rule is core's object, not a copy (objectui#5312)", () => {
+  it('asks `@object-ui/core` EXPANDABLE_FIELD_TYPES which params carry a reference target', () => {
+    // The spy is installed on the Set exported by core and records a call only
+    // if THIS module consulted THAT object. A member-identical private copy
+    // leaves it empty, so this fails where a value check would pass.
+    const spy = vi.spyOn(EXPANDABLE_FIELD_TYPES, 'has');
+    try {
+      expect(paramToField(p({ type: 'user' }))).toMatchObject({ type: 'user' });
+      expect(spy.mock.calls.map(([k]) => k)).toContain('user');
+    } finally {
+      spy.mockRestore();
+    }
+  });
+
+  it('reaches that object on the lookup path too, not just the person path', () => {
+    // `user` and `lookup` enter the branch by different routes (the degrade
+    // rule above runs first and only for `lookup` / `master_detail`), so a
+    // convergence that reconnected one route would leave the other forked.
+    const spy = vi.spyOn(EXPANDABLE_FIELD_TYPES, 'has');
+    try {
+      paramToField(p({ type: 'lookup', referenceTo: 'accounts' }));
+      expect(spy.mock.calls.map(([k]) => k)).toContain('lookup');
+    } finally {
+      spy.mockRestore();
+    }
+  });
+
+  // Behaviour equivalence with the literal this replaced, stated as the literal
+  // itself rather than as prose: the retired inline rule matched exactly these
+  // three widget keys.
+  const RETIRED_INLINE_MEMBERS = ['lookup', 'master_detail', 'user'];
+
+  it('still hands the reference config to every member of the literal it replaced', () => {
+    for (const type of RETIRED_INLINE_MEMBERS) {
+      expect(
+        paramToField(p({ type, referenceTo: 'accounts', displayField: 'name' })),
+        `'${type}' lost its reference config in the convergence`,
+      ).toMatchObject({ type, reference_to: 'accounts', display_field: 'name' });
+    }
+    expect(RETIRED_INLINE_MEMBERS.filter((t) => !EXPANDABLE_FIELD_TYPES.has(t))).toEqual([]);
+  });
+
+  it('adds exactly one member, and that member is unreachable on this surface', () => {
+    // Why converging cost no behaviour change: the shared set is one member
+    // wider, and that member can never BE a widget key here — `tree` is absent
+    // from `fields`' widget map and `mapFieldTypeToFormType` sends it to
+    // `field:lookup`, so every key tested by the rule (always
+    // `resolveParamWidgetType` output) arrives as `lookup`. If someone
+    // registers a real `tree` widget this flips and `tree` starts matching the
+    // rule here — a behaviour change that would otherwise arrive silently.
+    const added = [...EXPANDABLE_FIELD_TYPES].filter((t) => !RETIRED_INLINE_MEMBERS.includes(t));
+    expect(added).toEqual(['tree']);
+    expect(resolveParamWidgetType('tree')).toBe('lookup');
+  });
+
+  it('holds every core member under the normalization this surface applies', () => {
+    // The shared set is defined over SCHEMA types; this surface tests WIDGET
+    // keys (`resolveParamWidgetType` output). Reading one with the other is
+    // only safe while the family is closed under that resolution — every member
+    // resolves to a member. A future core member resolving to `text` here would
+    // be a schema-declared reference field silently rendered as a plain input
+    // with no target, so it fails loudly instead.
+    for (const type of EXPANDABLE_FIELD_TYPES) {
+      const widgetKey = resolveParamWidgetType(type);
+      expect(
+        EXPANDABLE_FIELD_TYPES.has(widgetKey),
+        `core member '${type}' resolves to '${widgetKey}', which is outside the family`,
+      ).toBe(true);
+    }
+  });
+
+  it('keeps the target-required rule SEPARATE from the reference-bearing rule', () => {
+    // The two rules are different sets over overlapping types and must not be
+    // folded together: `user` is reference-bearing but defaults its target to
+    // `sys_user`, so it must never degrade for want of `referenceTo`; `lookup`
+    // and `master_detail` must. Merging them would either strip the person
+    // picker or stop degrading targetless record pickers.
+    expect(paramToField(p({ type: 'user' }))).toMatchObject({ type: 'user' });
+    const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
+    try {
+      expect(paramToField(p({ type: 'master_detail' }))).toMatchObject({ type: 'text' });
+    } finally {
+      warn.mockRestore();
+    }
+  });
+});

packages/app-shell/src/utils/paramToField.ts

@@ -12,7 +12,7 @@
  * render tree (mirrors `filterVisibleParams`' style), with a drift test
  * asserting param support ⊇ form support.
  */
-import type { ActionParamDef } from '@object-ui/core';
+import { EXPANDABLE_FIELD_TYPES, type ActionParamDef } from '@object-ui/core';
 import { resolveFormWidgetType } from '@object-ui/fields';
 
 /**
@@ -40,7 +40,17 @@ export function resolveParamWidgetType(paramType: string): string {
   return resolveFormWidgetType(PARAM_TYPE_ALIASES[paramType] ?? paramType);
 }
 
-/** Widget keys that render the record-picker family and need a reference target. */
+/**
+ * Widget keys whose picker cannot query without an explicitly DECLARED target,
+ * so a param of that type with no `referenceTo` degrades to a plain text input.
+ *
+ * Deliberately NOT the reference-bearing family (`EXPANDABLE_FIELD_TYPES`, read
+ * below): `user` belongs to that family but defaults its target to `sys_user`,
+ * so it must never degrade. Two rules over overlapping types, kept separate —
+ * the same split the twin keeps between its own `LOOKUP_WIDGET_TYPES` and
+ * `widgetNeedsDataSource` (plugin-grid's `bulkParamToField`). objectui#5312
+ * converged the second rule only, on purpose.
+ */
 const LOOKUP_WIDGET_TYPES = new Set(['lookup', 'master_detail']);
 
 /**
@@ -91,10 +101,35 @@ export function paramToField(param: ActionParamDef): Record<string, any> {
     field.widget = 'checkbox';
   }
 
-  // `|| type === 'owner'` stood here until objectui#4814 retired that spelling
-  // (ruling A′). It moves in lockstep with plugin-grid's `bulkParamToField`
-  // twin — the two param faces are never split.
-  if (LOOKUP_WIDGET_TYPES.has(type) || type === 'user') {
+  // Which widgets carry a reference target is NOT restated here: it is
+  // `EXPANDABLE_FIELD_TYPES` from `@object-ui/core`, the one relational-field
+  // family that `buildExpandFields`, the predicate-record projection, the object
+  // form's `needsDataSourceWiring` and the grid's `bulkParamToField` already
+  // read (objectui#4770 / #4790 / #4815). This face held the fourth and last
+  // private copy of it — `LOOKUP_WIDGET_TYPES.has(type) || type === 'user'`,
+  // once with a fifth spelling `owner` that objectui#4814 retired (ruling A′).
+  //
+  // The comment that stood here claimed the disjunction "moves in lockstep with
+  // plugin-grid's `bulkParamToField` twin — the two param faces are never
+  // split". Measured on the tip before this change, that was false in BOTH
+  // senses: mechanically, the twin had read core's Set since objectui#4815 while
+  // this line read a private literal, so the two faces shared nothing and no
+  // gate could report a split; and by membership they already differed, by
+  // `tree`. Lockstep is true again — and for the first time it is mechanical,
+  // not hand-kept: the identity pin in `paramToField.test.ts` fails on a
+  // member-identical private copy, which is what a value check would not.
+  //
+  // `tree` is the member this face gains, and it is unreachable here: it is
+  // absent from `fields`' widget map and `mapFieldTypeToFormType` sends it to
+  // `field:lookup`, so every key tested here — always `resolveParamWidgetType`
+  // output — arrives as `lookup`. Pinned, so registering a real `tree` widget
+  // surfaces that behaviour change instead of shipping it silently.
+  //
+  // Extending this surface later: OR in a second, surface-local set, the way
+  // `needsDataSourceWiring` does. Never `new Set([...EXPANDABLE_FIELD_TYPES, …])`
+  // — a copy re-forks the table, which is the defect this change removed, and
+  // the identity pin fails on it by design.
+  if (EXPANDABLE_FIELD_TYPES.has(type)) {
     Object.assign(field, {
       reference_to: param.referenceTo,
       display_field: param.displayField,