diff --git a/.changeset/8672-lookup-action-param-depends-on.md b/.changeset/8672-lookup-action-param-depends-on.md new file mode 100644 index 0000000000..0c7ad0cf1d --- /dev/null +++ b/.changeset/8672-lookup-action-param-depends-on.md @@ -0,0 +1,46 @@ +--- +'@object-ui/app-shell': patch +'@object-ui/core': patch +--- + +A `dependsOn` declared on a field-backed **lookup action param** now gates, **ungates** +and filters the picker (objectui#8672, maintainer ruling — director decision batch #115, +2026-09-11, arm A "wire it"). + +**What was broken.** `ActionParamDialog` threaded its live record (`dependentValues`) to +the option widgets only — `select` / `multiselect` / `radio` / `checkboxes`. A `lookup` +param is in none of those, so `LookupField` fell through to the `SchemaRendererContext` +tail that is unconditionally `{}` (nothing can populate a member the type does not +declare), `dependenciesMissing` could never clear, and the trigger rendered **disabled +forever** — prompting for the very field the user had just filled. There was no error at +author time, at type-check or at runtime: the failure looked like a broken picker rather +than a key that did nothing here. + +Independently, the one route the repo's own `RESOLVED_ONLY_PARAM_KEYS.dependsOn` message +points authors to ("make the param field-backed to pick it up") read the **snake** +spelling `field.depends_on`, which `@objectstack/spec`'s `FieldSchema` refuses by name, +while the camel `dependsOn` it declares was never read. The two spellings were disjoint, +so no spec-valid document could reach the feature at all. + +**What changed.** + +- `ActionParamDialog` now supplies its live `values` to the reference-bearing pickers as + well as to the option widgets. That is the dialog's whole record: unlike the grid + (`ctx.pendingRow ?? ctx.row`) it is not scoped to a row — its params *are* the record, + which is the same record its option widgets have resolved against since objectui#3765. +- `resolveActionParams` reads the declared `field.dependsOn` and no longer reads + `field.depends_on`. Unlike its five sibling lookup keys the snake leg is removed rather + than demoted, because there is no producer to protect: no document that parses can + carry a spelling `FieldSchema` rejects by name. + +Nothing about the cascade itself is new. `LookupField` has always turned `dependsOn` into +a hard `$filter` shared by the quick-select popover, the Level-2 table picker and +PeoplePicker; this supplies the one input no host could otherwise deliver. + +**Unchanged on purpose.** `ActionParamSchema` still refuses `dependsOn` written *inline* +on a param — the honoured route is the field-backed one. `CASCADE_OPTION_WIDGET_TYPES` +gains no member: it is shared verbatim with the object form's cascade-clear loop and with +`plugin-grid`'s `BulkActionDialog`, and it means "this widget's offered *option set* is +re-resolved", which a lookup has none of. The dialog ORs a second family beside it +instead — the shape the object form has shipped all along. The bulk action dialog is +untouched and still carries the original gap. diff --git a/packages/app-shell/src/utils/expandableFamily.identity-5874.test.ts b/packages/app-shell/src/utils/expandableFamily.identity-5874.test.ts index d7d7604714..7560b6e58c 100644 --- a/packages/app-shell/src/utils/expandableFamily.identity-5874.test.ts +++ b/packages/app-shell/src/utils/expandableFamily.identity-5874.test.ts @@ -100,7 +100,22 @@ const PICKER_KEYS = [ 'dependsOn', ] as const; -/** One object field per relevant type, each carrying full picker config. */ +/** + * One object field per relevant type, each carrying full picker config. + * + * ⚠️ The spellings here are MIXED, and deliberately so — this fixture is about + * WHICH members inherit picker config, never about how a key is spelled. Most + * rows stay snake because `resolveActionParams` really does read that spelling + * (some as the only spelling it reads, some behind the declared one). + * + * ⭐ `dependsOn` is the exception and must stay camel (objectui#8672, ruling A). + * It was `depends_on: ['region']` and passed only because the resolver read the + * snake spelling — a spelling `@objectstack/spec`'s `FieldSchema` refuses BY + * NAME. Ruling A moved that read onto the declared `dependsOn` and removed the + * snake leg, so the old fixture row silently produced nothing and the + * `PICKER_KEYS` loop below caught it. ⛔ Do not "fix" a future failure here by + * restoring a dual read: the key is camelCase-only on both sides of the seam. + */ const field = (type: string) => ({ type, label: type, @@ -112,7 +127,7 @@ const field = (type: string) => ({ lookup_columns: ['name'], lookup_filters: [['active', '=', true]], lookup_page_size: 25, - depends_on: ['region'], + dependsOn: ['region'], }); const ctx = (): ResolveActionParamsContext => ({ diff --git a/packages/app-shell/src/utils/paramToField.test.ts b/packages/app-shell/src/utils/paramToField.test.ts index b15e892c98..20c1ec1b44 100644 --- a/packages/app-shell/src/utils/paramToField.test.ts +++ b/packages/app-shell/src/utils/paramToField.test.ts @@ -110,18 +110,17 @@ describe('paramToField', () => { // in-repo framework producer of that spelling, so the emit had to move with // the reader. // - // ⚠️ CORRECTED (objectui#8672). This sentence used to end "…or the dialog's - // dependent lookups would have gone dead silently", which describes a WORKING - // cascade in this dialog. Measured, there has never been one: `dependsOn` is - // emitted onto the field bag below, but `ActionParamDialog` supplies - // `dependentValues` only to `CASCADE_OPTION_WIDGET_TYPES` (`select` / - // `multiselect` / `radio` / `checkboxes` — `lookup` is not a member), so - // `LookupField` resolves `{}` and the gate NEVER lifts. What moving the emit - // preserved is therefore the GATE, not a cascade: leaving it behind would have - // flipped a permanently gated picker into an ungated, UNFILTERED one — the - // silent behaviour change `paramToField.ts`'s own comment records. Pinned in - // `views/ActionParamDialog.lookupDependsOnReach-8672.test.tsx`; which - // disposition that gap gets is open on objectui#8672. + // ⚠️ CORRECTED ONCE, then OVERTAKEN (objectui#8672). The sentence above used + // to end "…or the dialog's dependent lookups would have gone dead silently", + // which described a WORKING cascade in this dialog; at the time there had + // never been one, so the correction said what moving the emit preserved was + // the GATE, not a cascade. ⭐ Ruling A then wired the missing half: the dialog + // feeds the reference-bearing pickers their live record too + // (`paramNeedsDependentValues()` in `views/ActionParamDialog.tsx`), so this + // emit now reaches a cascade that really does ungate and really does filter. + // Both readings are kept because the order matters — the emit had to move + // BEFORE the supply existed, and it is what made the wiring a one-line + // change. Pinned in `views/ActionParamDialog.lookupDependsOnReach-8672.test.tsx`. // The remaining snake members below (`reference_to`, `title_format`, // `lookup_columns`, `lookup_page_size`) were outside both rulings and are // unchanged — this mixed shape is deliberate, and asserting it keeps the two diff --git a/packages/app-shell/src/utils/paramToField.ts b/packages/app-shell/src/utils/paramToField.ts index 9cb4764ee4..7464d02d03 100644 --- a/packages/app-shell/src/utils/paramToField.ts +++ b/packages/app-shell/src/utils/paramToField.ts @@ -189,18 +189,20 @@ export function paramToField(param: ActionParamDef): Record { // leaving it would have handed `LookupField` a key it no longer reads. // // ⚠️ What that would have changed, stated as MEASURED and not larger: - // this dialog supplies `dependentValues` only to - // `CASCADE_OPTION_WIDGET_TYPES` (`select` / `multiselect` / `radio` / - // `checkboxes`, the supply site being `ActionParamDialog.tsx`'s - // `cascadeProps`), and `LookupField`'s context fallback resolves to `{}` - // here, so a lookup param's cascade gate in this dialog is PERMANENT: - // it never lifts, whatever the user types into the parent. Dropping the - // read arm without moving this emit would therefore have flipped that - // permanent gate into an UNGATED, UNFILTERED picker — a silent behaviour - // change, and the reason the emit moves with the reader; ⛔ not a - // working cascade that would have died. That the gate never lifts here - // is a PRE-EXISTING residue of its own, not something this card - // introduced or fixes. + // dropping the read arm without moving this emit would have flipped the + // gate this key raises into an UNGATED, UNFILTERED picker — a silent + // behaviour change, and the reason the emit moves with the reader. + // + // ⭐ objectui#8672, ruling A — the residue that note recorded is gone. + // When it was written, this dialog supplied `dependentValues` only to + // `CASCADE_OPTION_WIDGET_TYPES` (no `lookup` member), so `LookupField` + // resolved `{}` and the gate this emit raises NEVER lifted, whatever the + // user typed into the parent. `ActionParamDialog` now feeds the + // reference-bearing family its live `values` as well — see + // `paramNeedsDependentValues()` there — so the key this line emits does + // what its author meant: the trigger ungates once the named parent + // carries a value, and the picker is narrowed by it on every surface. + // ⛔ Do not restate the supply rule here; it is asked of that predicate. // // The remaining snake members above are a different question // (objectui#7155's ruling covered four keys and this was not one of them). diff --git a/packages/app-shell/src/utils/resolveActionParams.ts b/packages/app-shell/src/utils/resolveActionParams.ts index 46866712a3..9d7687b77d 100644 --- a/packages/app-shell/src/utils/resolveActionParams.ts +++ b/packages/app-shell/src/utils/resolveActionParams.ts @@ -325,9 +325,16 @@ interface RuntimeField { // camel spelling here would fossilise a spelling no contract declares — // the exact defect this family exists to stop. Routed to objectui#7650. // - // ⛔ `depends_on` gains no camel twin either, and its reason is the opposite - // shape: `FieldSchema` DOES declare `dependsOn`, so the omission is not - // contractual but MEASURED. See the read site below. + // ⭐ objectui#8672, ruling A — `dependsOn` is now declared in the SPEC + // spelling and ONLY in it. Unlike the five keys above it gains no snake twin + // to rank behind, because there was never a contractual reason for one: + // `FieldSchema` DECLARES `dependsOn` and refuses `depends_on` by name. The + // snake member this interface used to carry was the only spelling the read + // site below looked at, so a spec-valid field def declaring the cascade + // resolved to `undefined` while the one spelling that DID arrive was one no + // author could legally write. That is not a producer leg worth keeping behind + // the declared one — it is the inverse of one, and keeping it would be the + // consumer-side tolerance AGENTS.md #0.1 bans. reference_to?: string; reference?: string; displayField?: string; @@ -343,7 +350,7 @@ interface RuntimeField { lookup_filters?: unknown[]; lookupPageSize?: number; lookup_page_size?: number; - depends_on?: unknown[]; + dependsOn?: unknown[]; } interface RuntimeObject { @@ -599,23 +606,30 @@ export function resolveActionParam( // rank first, so there is nothing to add that would not fossilise an // undeclared key. // - // ⛔ `dependsOn` is the third declared key of that slice and it keeps - // its snake-only read for a MEASURED reason, not a contractual one. - // Adding `field.dependsOn ?? field.depends_on` was built and rendered - // before being refused: a field-backed lookup param whose def declares - // the spec spelling then renders `lookup-trigger-gated` and DISABLED, - // where the same def renders an enabled trigger today (control: an - // otherwise identical lookup without the key, enabled in both runs). - // The gate never lifts — this dialog supplies `dependentValues` only to - // `CASCADE_OPTION_WIDGET_TYPES`, which excludes `lookup`, so - // `LookupField`'s `dependenciesMissing` can never clear. That is pinned, - // with a keystroke witness, by leg A of - // `views/ActionParamDialog.lookupDependsOnReach-8672.test.tsx`, and leg - // C of the same file pins the `undefined` this read produces today. - // ⇒ ranking the declared spelling first here would trade a config-loss - // bug for an unusable picker on every spec-valid def that declares the - // cascade. Which of objectui#8672's three dispositions to take — wire - // it, refuse it, declare the limit — is that card's ruling to make. + // ⭐ objectui#8672, ruling A — `dependsOn` is the third declared key of + // that slice and it lands here now, in the DECLARED spelling and with + // no snake leg behind it. It was held back deliberately: honouring the + // spec spelling while `ActionParamDialog` fed `dependentValues` to + // option widgets only would have turned every spec-valid def that + // declares a cascade into a `lookup-trigger-gated`, permanently + // DISABLED picker — a measurement objectui#9130 built, rendered and + // then refused to ship, routing the disposition to objectui#8672. + // + // The dialog now supplies that record (see `paramNeedsDependentValues` + // in `views/ActionParamDialog.tsx`), so the gate lifts as soon as the + // named parent carries a value and `LookupField` narrows the picker by + // it. The two halves ship together on purpose: either alone is a + // regression, and this comment is the reason they may not be split + // again. + // + // ⛔ The snake read is GONE rather than demoted, which is the one place + // this key departs from its five siblings above. Their snake legs are + // kept because a pre-tightening document or an out-of-repo host adapter + // could still emit them. `depends_on` has no such producer to protect: + // `FieldSchema` refuses it BY NAME (suggesting `dependsOn`), so no + // document that parses can carry it, and objectui#7357 already retired + // the renderer-side twin in `LookupField`. Keeping it would leave this + // resolver the last reader of a spelling the protocol rejects. referenceTo: param.reference ?? field.reference, displayField: field.displayField ?? field.display_field ?? field.reference_field, @@ -625,7 +639,7 @@ export function resolveActionParam( lookupColumns: field.lookupColumns ?? field.lookup_columns, lookupFilters: field.lookupFilters ?? field.lookup_filters, lookupPageSize: field.lookupPageSize ?? field.lookup_page_size, - dependsOn: field.depends_on, + dependsOn: field.dependsOn, } : {}; diff --git a/packages/app-shell/src/views/ActionParamDialog.lookupDependsOnReach-8672.test.tsx b/packages/app-shell/src/views/ActionParamDialog.lookupDependsOnReach-8672.test.tsx index 5835d024f5..74e93c26bc 100644 --- a/packages/app-shell/src/views/ActionParamDialog.lookupDependsOnReach-8672.test.tsx +++ b/packages/app-shell/src/views/ActionParamDialog.lookupDependsOnReach-8672.test.tsx @@ -7,60 +7,65 @@ */ /** - * objectui#8672 — MEASUREMENT PINS for a `dependsOn` lookup ACTION PARAM. + * objectui#8672 — a `dependsOn` lookup ACTION PARAM gates, UNGATES, and filters. * - * ⭐⭐ **CURRENT SHAPE, NOT CONTRACT.** ⭐⭐ Read this before changing anything - * below. The card names three dispositions — wire it · refuse it · declare the - * limit — and **none of them is chosen here**. This file exists so that the - * ruling is made against a measured, stable behaviour instead of a guess; every - * `expect` in legs A and C records what the code does TODAY and endorses none of - * it. Whoever implements a disposition should expect these to go red and should - * **rewrite them**, not trust them: a red here is the pin doing its job, not a - * regression. Leg B is the exception and says so on its own describe block. + * ⭐ These were MEASUREMENT pins and are now CONTRACT pins. The file that stood + * here recorded today's behaviour and endorsed none of it: legs A and C were + * labelled "CURRENT SHAPE, NOT CONTRACT" and told whoever implemented one of the + * card's three dispositions to expect them red and rewrite them. The maintainer + * ruled **A — wire it** (director seat, decision batch #115, 2026-09-11), so + * that is what happened: every `expect` below now states what the code MUST do. * - * ## What the three legs measure + * ## What ruling A settled, and what each leg holds to it * - * - **Leg A (current shape)** — the behaviour the card reports, made permanent: - * a lookup param declaring `dependsOn` renders GATED and a keystroke in the - * named parent does not lift the gate. `CASCADE_OPTION_WIDGET_TYPES` (the - * dialog's `dependentValues` supply set in `ActionParamDialog.tsx`) does not - * contain `lookup`, so `LookupField` resolves its record through the context - * tail that is unconditionally `{}` (objectui#7206) and `dependenciesMissing` - * can never go false. - * - **Leg B (contract, version-qualified)** — where the authoring surface is. - * `@objectstack/spec`'s `ActionParamSchema` is `.strict()` and REFUSES - * `dependsOn` on an action param outright. `@object-ui/types`' `ActionParam` - * derives its key set from `z.input`, so it declares - * no `dependsOn` either — which is why leg A has to synthesise the resolved - * `ActionParamDef` directly rather than author a param. - * - **Leg C (current shape)** — the ONE route the repo itself points authors to - * (`RESOLVED_ONLY_PARAM_KEYS.dependsOn`: "make the param field-backed … to - * pick it up") reads `field.depends_on`, the SNAKE spelling, while the spelling - * `FieldSchema` accepts is camel `dependsOn`. So a spec-valid object field - * declaring the cascade delivers `undefined` here. + * - **Leg A — the dialog.** `ActionParamDialog` supplies its live record to the + * reference-bearing pickers, not only to the option widgets, so a lookup param + * declaring `dependsOn` is gated while the named parent is EMPTY and ungates + * the moment it carries a value. The record is the dialog's own in-progress + * `values`: unlike the grid (`ctx.pendingRow ?? ctx.row`, objectui#7165/#7188) + * this surface holds no row at all — its params ARE the record, which is the + * measurement the ruling left to the implementer. + * - **Leg B — the authoring surface. UNCHANGED by the ruling, which says so in + * as many words:** `@objectstack/spec`'s `ActionParamSchema` still refuses + * `dependsOn` written INLINE on a param. The honoured route is the + * field-backed one, so leg A still has to synthesise a resolved + * `ActionParamDef` rather than author a param. + * - **Leg C — the resolver.** `resolveActionParams` reads the DECLARED spelling + * `field.dependsOn` and no longer reads snake `field.depends_on`, which + * `FieldSchema` refuses by name. Both halves of that swap are pinned: the + * declared key must arrive, and the refused one must not. + * - **Leg D — the two halves meeting.** The end-to-end route the repo's own + * `RESOLVED_ONLY_PARAM_KEYS.dependsOn` message points authors to ("make the + * param field-backed … to pick it up"), driven from an object field def + * through the resolver into the rendered dialog and out to the picker's + * query. ⭐ This is the leg that would have been impossible before the ruling + * and the one that fails if either half is reverted alone. * - * ## Every reading has a lit control beside it + * ## Every reading still has a lit control beside it * - * A gated trigger, a refused parse and an `undefined` key are all shapes a - * broken fixture produces for free, so none of them is asserted alone: + * An ungated trigger and a narrowed candidate list are both shapes a broken + * fixture produces for free, so neither is asserted alone: * - * - leg A pairs the gated lookup with a CONTROL lookup identical but for - * `dependsOn` (asserted enabled), and drives the keystroke through a `radio` - * param whose `visibleWhen` DOES react to it — so "the gate did not lift" is - * read against a keystroke proven to have reached the dialog and re-rendered - * it, not against a dialog that ignored the event; - * - leg B pairs each refusal with a positive control document that must PARSE, - * and with an unknown-key document that must be REFUSED — so an "accept" is a - * reading of a live strict schema rather than of a permissive one; - * - leg C pairs the `undefined` with a sibling key resolved off the SAME field - * def in the same call, so an empty reading cannot come from a fixture the - * resolver never saw. + * - leg A keeps the CONTROL lookup (identical but for `dependsOn`, asserted + * enabled throughout) and the keystroke witness — a `radio` whose `visibleWhen` + * reacts to the same keystroke — so "the gate lifted" is read against a + * keystroke proven to have reached the dialog, and it keeps the NEGATIVE + * control that an empty parent still gates, so the fix cannot be read as + * "the gate was deleted"; + * - leg B pairs each refusal with a positive control document that must PARSE + * and an unknown-key document that must be REFUSED; + * - leg C pairs each reading with a sibling key resolved off the SAME field def + * in the same call; + * - leg D offers two records under one reference and asserts the declared param + * sees one while the control param sees both — so a narrowed list cannot be a + * picker that simply failed to load. */ -import { describe, it, expect } from 'vitest'; -import { render, screen, fireEvent, waitFor } from '@testing-library/react'; +import { describe, it, expect, vi, beforeAll } from 'vitest'; +import { render, screen, fireEvent, waitFor, within } from '@testing-library/react'; import '@testing-library/jest-dom'; import type { ActionParamDef } from '@object-ui/core'; -import { CASCADE_OPTION_WIDGET_TYPES } from '@object-ui/core'; +import { CASCADE_OPTION_WIDGET_TYPES, EXPANDABLE_FIELD_TYPES } from '@object-ui/core'; +import { SchemaRendererProvider } from '@object-ui/react'; import { ActionParamSchema } from '@objectstack/spec/ui'; import { FieldSchema } from '@objectstack/spec/data'; // Module scope, per AGENTS.md 测试纪律: the dialog reaches `LookupField` through @@ -76,8 +81,12 @@ import { type RawActionParam, } from '../utils/resolveActionParams'; +beforeAll(() => { + if (!Element.prototype.scrollIntoView) Element.prototype.scrollIntoView = vi.fn() as never; +}); + /* ────────────────────────────────────────────────────────────────────────── */ -/* Leg A — the dialog's behaviour today CURRENT SHAPE */ +/* Leg A — the dialog supplies the record CONTRACT */ /* ────────────────────────────────────────────────────────────────────────── */ /** The parent the child lookup names. A plain text param, so it is typable. */ @@ -86,7 +95,8 @@ const ACCOUNT: ActionParamDef = { name: 'account', label: 'Account', type: 'text /** * The card's shape verbatim: `{ type: 'lookup', referenceTo: 'contacts', * dependsOn: ['account'] }`. Synthesised as a RESOLVED `ActionParamDef` on - * purpose — leg B measures that this cannot be authored as a raw param at all. + * purpose — leg B measures that this cannot be authored as a raw param at all, + * and leg D drives the route that CAN produce it. */ const GATED_LOOKUP: ActionParamDef = { name: 'contact', @@ -107,8 +117,8 @@ const CONTROL_LOOKUP: ActionParamDef = { /** * CONTROL for the KEYSTROKE. A `radio` IS in `CASCADE_OPTION_WIDGET_TYPES`, so * the dialog threads its in-progress values to it and this option's predicate - * re-resolves on every change to `account`. Without it, "the gate did not lift" - * would also be the reading for a dialog that never saw the keystroke. + * re-resolves on every change to `account`. Without it, a gate that failed to + * lift would be indistinguishable from a dialog that never saw the keystroke. */ const KEYSTROKE_WITNESS: ActionParamDef = { name: 'tier', @@ -120,20 +130,25 @@ const KEYSTROKE_WITNESS: ActionParamDef = { ], }; -function openDialog(params: ActionParamDef[]) { - render( +function openDialog(params: ActionParamDef[], dataSource?: unknown) { + const dialog = ( {} }} onOpenChange={() => {}} - />, + /> + ); + return render( + dataSource + ? {dialog} + : dialog, ); } -const typeAccount = (value: string) => - fireEvent.change(screen.getByLabelText('Account'), { target: { value } }); +const typeAccount = (value: string, label = 'Account') => + fireEvent.change(screen.getByLabelText(label), { target: { value } }); -describe('objectui#8672 leg A — a `dependsOn` lookup param is gated, permanently (CURRENT SHAPE, NOT CONTRACT)', () => { - it('renders the declared lookup GATED while the control lookup beside it is usable', async () => { +describe('objectui#8672 leg A — a `dependsOn` lookup param gates on an EMPTY parent and ungates when it is filled', () => { + it('NEGATIVE CONTROL — with the parent still empty the declared lookup is gated, and the control beside it is not', async () => { openDialog([ACCOUNT, GATED_LOOKUP, CONTROL_LOOKUP]); // CONTROL first: an identical lookup differing only in `dependsOn` renders a @@ -142,14 +157,15 @@ describe('objectui#8672 leg A — a `dependsOn` lookup param is gated, permanent const control = await screen.findByTestId('lookup-trigger-control_contact'); expect(control).toBeEnabled(); - // SUBJECT: the declared lookup renders the gate — disabled, and prompting - // for the very field the user is about to fill. + // ⭐ The gate is NOT deleted by ruling A — it is given a way out. An empty + // parent must still gate, or the picker would issue the unfiltered query + // that the cascade exists to prevent. const gated = screen.getByTestId('lookup-trigger-gated'); expect(gated).toBeDisabled(); expect(gated).toHaveTextContent('Select account first'); }); - it('does NOT lift the gate when the named parent is filled — while the same keystroke moves a witness', async () => { + it('⭐ LIFTS the gate when the named parent is filled — the assertion ruling A turned around', async () => { openDialog([ACCOUNT, GATED_LOOKUP, KEYSTROKE_WITNESS]); // Both witness options are offered before anything is typed: `record.account` @@ -160,29 +176,52 @@ describe('objectui#8672 leg A — a `dependsOn` lookup param is gated, permanent typeAccount('acme'); - // ⭐ The keystroke DID reach the dialog and DID re-render it: the witness's - // offered set narrowed. Load-bearing — it converts the assertion after it - // from "nothing happened" into "this specific thing did not happen". + // The keystroke DID reach the dialog and DID re-render it: the witness's + // offered set narrowed. Kept from the measurement pins — it is what makes + // the assertion after it a reading of the supply rule and not of the event. await waitFor(() => expect(screen.queryByTestId('radio-option-smb')).not.toBeInTheDocument()); expect(screen.getByTestId('radio-option-ent')).toBeInTheDocument(); - // …and the lookup is still gated on the field that now carries `acme`. This - // assertion is the card. ⛔ It is NOT a statement that it should stay this - // way — see the file docblock. - const stillGated = screen.getByTestId('lookup-trigger-gated'); - expect(stillGated).toBeDisabled(); - expect(stillGated).toHaveTextContent('Select account first'); + // ⭐ THE CARD, INVERTED. Before ruling A this stayed `lookup-trigger-gated` + // and `disabled` forever, prompting for the field the user had just filled. + // The gated test id is `LookupField`'s own signal, so its disappearance and + // the named trigger's arrival are one fact read two ways. + const ungated = await screen.findByTestId('lookup-trigger-contact'); + expect(ungated).toBeEnabled(); + expect(screen.queryByTestId('lookup-trigger-gated')).not.toBeInTheDocument(); + expect(ungated).not.toHaveTextContent('Select account first'); }); - it('records WHY: the dialog\'s `dependentValues` supply set excludes `lookup`', () => { - // The mechanism behind the two renders above, stated where it can be found - // from them. `ActionParamDialog.tsx` supplies `dependentValues` only to - // members of this set, so a lookup is reached without one and falls through - // to the context tail that objectui#7206 measured as unconditionally `{}`. + it('re-gates when the parent is CLEARED, so the record is read live and not once', async () => { + // Guards the shape where a host seeds the record at mount: `values` is read + // on every render, so emptying the parent must put the gate back. + openDialog([ACCOUNT, GATED_LOOKUP]); + expect(await screen.findByTestId('lookup-trigger-gated')).toBeDisabled(); + + typeAccount('acme'); + expect(await screen.findByTestId('lookup-trigger-contact')).toBeEnabled(); + + typeAccount(''); + await waitFor(() => expect(screen.getByTestId('lookup-trigger-gated')).toBeDisabled()); + }); + + it('records WHY: TWO supply rules, and the shared option set was deliberately NOT widened', () => { + // The mechanism behind the renders above, stated where it can be found from + // them. `ActionParamDialog`'s `paramNeedsDependentValues()` ORs two families + // that mean different things, exactly as the object form's two lines do. + // + // ⛔ This assertion is not a leftover from the measurement pins: it is the + // guard against "simplifying" the fix into `CASCADE_OPTION_WIDGET_TYPES.add + // ('lookup')`. That set is shared verbatim with the object form's + // cascade-CLEAR loop and with `plugin-grid`'s `BulkActionDialog`, so a + // member added there changes two surfaces this card never measured and + // decides objectui#4771's open boundary for them. expect(CASCADE_OPTION_WIDGET_TYPES.has('lookup')).toBe(false); - // Lit control on the same read: the members that DO get the record. + expect(EXPANDABLE_FIELD_TYPES.has('lookup')).toBe(true); + // Lit controls on the same two reads, in both directions: an option widget + // is in the first set and not the second. expect(CASCADE_OPTION_WIDGET_TYPES.has('radio')).toBe(true); - expect(CASCADE_OPTION_WIDGET_TYPES.has('select')).toBe(true); + expect(EXPANDABLE_FIELD_TYPES.has('radio')).toBe(false); }); }); @@ -191,13 +230,13 @@ describe('objectui#8672 leg A — a `dependsOn` lookup param is gated, permanent /* ────────────────────────────────────────────────────────────────────────── */ /** - * ⚠️ Unlike legs A and C this leg pins a CONTRACT, not a current shape — but a - * VERSION-QUALIFIED one: it is a reading of the `@objectstack/spec` this repo - * has installed, and the answer moves if that schema does. It is here because it - * is the fact the card's disposition turns on: a refusal for `dependsOn` on an - * action param already exists, and it lives UPSTREAM. + * ⚠️ A VERSION-QUALIFIED contract: it is a reading of the `@objectstack/spec` + * this repo has installed, and the answer moves if that schema does. Ruling A + * left it standing explicitly — "`ActionParamSchema` still refuses `dependsOn` + * written inline on a param — that stays; the honoured route is the field-backed + * one" — so this leg is the fence that keeps the fix on that route. */ -describe('objectui#8672 leg B — `@objectstack/spec` already refuses `dependsOn` on an action param', () => { +describe('objectui#8672 leg B — `@objectstack/spec` still refuses `dependsOn` INLINE on an action param', () => { const param = (over: Record) => ({ name: 'contact', label: 'Contact', @@ -226,8 +265,8 @@ describe('objectui#8672 leg B — `@objectstack/spec` already refuses `dependsOn }); it('the refusal is not lookup-specific — a `select` param is refused the same way', () => { - // Recorded so a disposition author does not read the refusal as a narrow, - // type-scoped rule it is not: no action param of any type admits `dependsOn`. + // Recorded so a reader does not take the refusal for a narrow, type-scoped + // rule it is not: no action param of any type admits `dependsOn` inline. const r = ActionParamSchema.safeParse({ name: 'city', label: 'City', type: 'select', dependsOn: ['country'], }); @@ -236,50 +275,175 @@ describe('objectui#8672 leg B — `@objectstack/spec` already refuses `dependsOn }); /* ────────────────────────────────────────────────────────────────────────── */ -/* Leg C — the field-backed route reads the spelling spec refuses */ -/* CURRENT SHAPE */ +/* Leg C — the field-backed read follows the declared spelling CONTRACT */ /* ────────────────────────────────────────────────────────────────────────── */ -describe('objectui#8672 leg C — the field-backed route reads `depends_on`, which `FieldSchema` refuses (CURRENT SHAPE, NOT CONTRACT)', () => { +const ctx = (field: Record): ResolveActionParamsContext => ({ + objectName: 'crm_case', + objects: [{ name: 'crm_case', fields: { contact_id: field } }] as never, + fieldLabel: (_o, _f, fallback) => fallback, +}); + +const FIELD_BACKED: RawActionParam[] = [{ field: 'contact_id' }]; + +describe('objectui#8672 leg C — the field-backed route reads the spelling `FieldSchema` declares', () => { it('CONTROL — `FieldSchema` accepts camel `dependsOn` on a lookup field and refuses the snake twin by name', () => { const base = { name: 'contact_id', label: 'Contact', type: 'lookup', reference: 'contacts' }; // POSITIVE CONTROL: the field def parses at all. expect(FieldSchema.safeParse(base).success).toBe(true); // The spelling the spec declares (objectui#7357 retired objectui's twin). expect(FieldSchema.safeParse({ ...base, dependsOn: ['account_id'] }).success).toBe(true); - // …and the spelling `resolveActionParams` actually reads is refused here. + // …and the spelling this resolver used to read is refused here. That + // asymmetry is the whole reason the read moved rather than gaining a + // second arm behind the declared one. expect(FieldSchema.safeParse({ ...base, depends_on: ['account_id'] }).success).toBe(false); }); - const ctx = (field: Record): ResolveActionParamsContext => ({ - objectName: 'crm_case', - objects: [{ name: 'crm_case', fields: { contact_id: field } }] as never, - fieldLabel: (_o, _f, fallback) => fallback, - }); - - const FIELD_BACKED: RawActionParam[] = [{ field: 'contact_id' }]; - - it('a spec-valid field declaring camel `dependsOn` resolves to `dependsOn: undefined`', () => { + it('⭐ a spec-valid field declaring camel `dependsOn` now RESOLVES it', () => { const [resolved] = resolveActionParams( FIELD_BACKED, ctx({ type: 'lookup', label: 'Contact', reference: 'contacts', dependsOn: ['account_id'] }), ); - // ⭐ LIT CONTROL on the same call: a sibling key off the SAME field def DOES - // arrive, so the `undefined` below cannot be a fixture the resolver never - // read. Without this the assertion would pass against an empty object. + // LIT CONTROL on the same call: a sibling key off the SAME field def, so a + // reading below cannot come from a fixture the resolver never saw. expect(resolved.referenceTo).toBe('contacts'); expect(resolved.type).toBe('lookup'); - // SUBJECT — the cascade key does not survive the route the repo's own - // `RESOLVED_ONLY_PARAM_KEYS.dependsOn` message points authors to. - expect(resolved.dependsOn).toBeUndefined(); + // SUBJECT — this was `undefined` until ruling A, which is why the route the + // repo's own `RESOLVED_ONLY_PARAM_KEYS.dependsOn` message points authors to + // reached nothing. + expect(resolved.dependsOn).toEqual(['account_id']); }); - it('only the snake spelling — the one the spec refuses — reaches the resolved param', () => { + it('the snake spelling the spec refuses NO LONGER reaches the resolved param', () => { const [resolved] = resolveActionParams( FIELD_BACKED, ctx({ type: 'lookup', label: 'Contact', reference: 'contacts', depends_on: ['account_id'] }), ); + // Same lit control: the def WAS read, so the `undefined` below is a refusal + // and not an unread fixture. expect(resolved.referenceTo).toBe('contacts'); + // ⛔ The retirement half. Keeping this leg behind the declared one would + // leave this resolver the last reader of a spelling no parseable document + // can carry (AGENTS.md #0.1 — no consumer-side tolerance for metadata the + // producer refuses). + expect(resolved.dependsOn).toBeUndefined(); + }); + + it('a def declaring BOTH spellings resolves the declared one — no snake fallback survives', () => { + const [resolved] = resolveActionParams( + FIELD_BACKED, + ctx({ + type: 'lookup', label: 'Contact', reference: 'contacts', + dependsOn: ['account_id'], depends_on: ['legacy_id'], + }), + ); expect(resolved.dependsOn).toEqual(['account_id']); }); }); + +/* ────────────────────────────────────────────────────────────────────────── */ +/* Leg D — end to end, the route the repo points authors to CONTRACT */ +/* ────────────────────────────────────────────────────────────────────────── */ + +/** Only the two members these assertions read; the picker passes more. */ +type QueryParams = { $filter?: Record }; + +const CONTACTS = [ + { id: 'k1', name: 'Ada (acme)', account_id: 'acme' }, + { id: 'k2', name: 'Bo (other)', account_id: 'other' }, +]; + +/** Honours the `$filter` record, so the cascade is observable as RENDERED ROWS + * and not only as call arguments. */ +function makeDataSource() { + const queries: Array<{ objectName: string; params: QueryParams }> = []; + return { + queries, + find: vi.fn(async (objectName: string, params: QueryParams) => { + queries.push({ objectName, params }); + let recs = CONTACTS; + const filter = params?.$filter; + if (filter && typeof filter === 'object' && filter.account_id) { + recs = recs.filter((c) => c.account_id === filter.account_id); + } + return { data: recs, total: recs.length, hasMore: false, pageSize: 50 }; + }), + findOne: vi.fn(async (_o: string, id: string) => CONTACTS.find((c) => c.id === id) ?? null), + getObjectSchema: async (name: string) => ({ + name, + fields: { id: { type: 'text' }, name: { type: 'text' }, account_id: { type: 'text' } }, + }), + }; +} + +/** The object def an author really writes — three fields, one declaring the + * cascade and one deliberately not. */ +const CASE_FIELDS: Record> = { + account_id: { type: 'text', label: 'Account' }, + contact_id: { + type: 'lookup', label: 'Contact', reference: 'contacts', dependsOn: ['account_id'], + }, + control_contact_id: { type: 'lookup', label: 'Control contact', reference: 'contacts' }, +}; + +const FIELD_BACKED_PARAMS: RawActionParam[] = [ + { field: 'account_id' }, + { field: 'contact_id' }, + { field: 'control_contact_id' }, +]; + +describe('objectui#8672 leg D — a field-backed lookup param cascades end to end', () => { + const resolveAll = () => + resolveActionParams(FIELD_BACKED_PARAMS, { + objectName: 'crm_case', + objects: [{ name: 'crm_case', fields: CASE_FIELDS }] as never, + fieldLabel: (_o, _f, fallback) => fallback, + }); + + it('the declared cascade survives the resolver and reaches the rendered dialog', async () => { + const params = resolveAll(); + // The resolver's half, asserted where the render can be read against it. + expect(params.map((p) => p.name)).toEqual(['account_id', 'contact_id', 'control_contact_id']); + expect(params[1].dependsOn).toEqual(['account_id']); + expect(params[2].dependsOn).toBeUndefined(); + + openDialog(params, makeDataSource()); + + // CONTROL — the sibling param with no cascade is open from the start. + expect(await screen.findByTestId('lookup-trigger-control_contact_id')).toBeEnabled(); + // SUBJECT — gated on the empty parent, then released by it. + expect(screen.getByTestId('lookup-trigger-gated')).toBeDisabled(); + + typeAccount('acme'); + + expect(await screen.findByTestId('lookup-trigger-contact_id')).toBeEnabled(); + }); + + it('⭐ and the parent NARROWS the picker — the control offers the record the subject must not', async () => { + const ds = makeDataSource(); + openDialog(resolveAll(), ds); + + await screen.findByTestId('lookup-trigger-control_contact_id'); + typeAccount('acme'); + + // SUBJECT — `account_id: 'acme'` reaches the query as a hard `$filter`, so + // only Ada is a candidate. This is what proves the dialog supplied a + // CORRECT record and not merely a non-empty one: an unscoped picker lists Bo. + fireEvent.click(await screen.findByTestId('lookup-trigger-contact_id')); + await waitFor(() => expect(screen.getByText('Ada (acme)')).toBeInTheDocument()); + expect(screen.queryByText('Bo (other)')).not.toBeInTheDocument(); + expect( + ds.queries.some((q) => q.objectName === 'contacts' && q.params?.$filter?.account_id === 'acme'), + ).toBe(true); + + fireEvent.keyDown(document.body, { key: 'Escape' }); + await waitFor(() => expect(screen.queryByText('Ada (acme)')).not.toBeInTheDocument()); + + // CONTROL — the sibling param declares no `dependsOn`, so the SAME reference + // over the SAME records is unfiltered and Bo IS offered. Without this, + // "Bo is absent" could just mean the picker never loaded. + fireEvent.click(screen.getByTestId('lookup-trigger-control_contact_id')); + await waitFor(() => expect(screen.getByText('Bo (other)')).toBeInTheDocument()); + expect(within(document.body).getByText('Ada (acme)')).toBeInTheDocument(); + }); +}); diff --git a/packages/app-shell/src/views/ActionParamDialog.tsx b/packages/app-shell/src/views/ActionParamDialog.tsx index fc36b84e91..84a178d672 100644 --- a/packages/app-shell/src/views/ActionParamDialog.tsx +++ b/packages/app-shell/src/views/ActionParamDialog.tsx @@ -14,11 +14,12 @@ * uploads) and `SchemaRendererContext` (dataSource for lookup/user pickers) * come from the host view, exactly as the previous `LookupField` reuse did. * - * One thing is threaded rather than ambient, and deliberately so: the record an - * option widget resolves its per-option `visibleWhen` against. The dialog is a - * small form, so its own in-progress `values` are that record — for WHICH - * widgets receive it, see `CASCADE_OPTION_WIDGET_TYPES` in `@object-ui/core` - * (objectui#3765; shared with the form and the bulk dialog per objectui#4770). + * One thing is threaded rather than ambient, and deliberately so: the live + * record a dependent widget scopes itself by. The dialog is a small form, so + * its own in-progress `values` are that record — for WHICH widgets receive it + * and on which of the two independent grounds, see + * `paramNeedsDependentValues()` below (objectui#3765 for the option widgets, + * objectui#8672 ruling A for the reference-bearing pickers). * * Returns collected param values or null on cancel. */ @@ -44,6 +45,11 @@ import { // dialog (objectui#4770). Its TSDoc carries the rationale that used to be // repeated in each of the three copies. CASCADE_OPTION_WIDGET_TYPES, + // The shared reference-bearing family — the SECOND reason a widget in this + // dialog needs the live record, and a different one (objectui#8672, ruling A). + // See `paramNeedsDependentValues` below; ⛔ never copied into a local literal + // and never merged into the set above. + EXPANDABLE_FIELD_TYPES, } from '@object-ui/core'; import { usePredicateScope } from '@object-ui/react'; import { getLazyFieldWidget, fileIdOf } from '@object-ui/fields'; @@ -213,6 +219,48 @@ function WidgetFallback() { return