diff --git a/.changeset/6194-line-items-row-fetch-decline.md b/.changeset/6194-line-items-row-fetch-decline.md new file mode 100644 index 0000000000..312e431c29 --- /dev/null +++ b/.changeset/6194-line-items-row-fetch-decline.md @@ -0,0 +1,28 @@ +--- +"@object-ui/plugin-form": patch +--- + +`record:line_items` declines to LOAD OR WRITE the rows of a panel whose child object it never resolved, instead of calling `find(undefined, …)` — the sibling site of the child-schema decline, in the same component. + +`LineItemsPanel` read `schema.childObject` at two sites. The first now declines; the row load still +asked the data layer to `find` an object literally named `undefined`, scoped by +`{ [relationshipField]: parentId }`. `load` guarded the *data source* and the *parent id* — the two +things `RelatedList` calls "can I scope this query" — but not the *object being queried*. + +Declining that fetch is not enough on its own, and this is the part worth reading: `load` owns +`loading`, and the panel branched `loading ? "Loading…" : !parentId ? "Save the record first…" : +`. So the moment the fetch declined, an unresolvable panel with a parent id bound fell to the +third branch and showed an **empty editable grid with an Add button, over an object that does not +exist** — a worse outcome than the fetch it replaced. Measured on the pre-fix component: one +keystroke in the grid's always-present ghost row materialised a row, which enabled Save, which +reached `batchTransaction([{ object: undefined, action: 'create', data: { qty: 3, invoice: 'inv-1' } }])`. +The bad *read* was one keystroke away from a bad *write*. + +An unresolvable panel therefore gets its own render branch — a config hint naming `childObject` and +what to set it to, following the precedent `object-master-detail-form` set for this exact key and +`AdvancedChartImpl`'s refusal placeholders. It is checked ahead of `loading`, because nothing is +pending: the schema itself already says the panel can never resolve, so there is no honest moment at +which "Loading…" is true. `save` takes the same one-line guard, for the one route the render branch +cannot close — a schema edited to drop `childObject` while rows are already dirty. + +A panel that names its child object loads, renders and saves exactly as before. diff --git a/packages/plugin-form/src/LineItemsPanel.childObjectDecline.test.tsx b/packages/plugin-form/src/LineItemsPanel.childObjectDecline.test.tsx index 45fd4ae629..a1d2f88dd8 100644 --- a/packages/plugin-form/src/LineItemsPanel.childObjectDecline.test.tsx +++ b/packages/plugin-form/src/LineItemsPanel.childObjectDecline.test.tsx @@ -5,9 +5,16 @@ * This source code is licensed under the MIT license found in the * LICENSE file in the root directory of this source tree. * - * `record:line_items` must DECLINE TO FETCH the child schema of a panel whose - * child object it never resolved — not call `getObjectSchema(undefined)` - * (objectui#6188). + * `record:line_items` must DECLINE TO TOUCH THE DATA LAYER AT ALL for a panel + * whose child object it never resolved — not call `getObjectSchema(undefined)` + * (objectui#6188), not `find(undefined, …)` (objectui#6194), and not write rows + * into it either. + * + * The component read `schema.childObject` at TWO sites and objectui#6188 guarded + * only the first; the row load was pinned here as a KNOWN HOLE until + * objectui#6194 closed it. Both are guarded now, so the call list below is + * empty — see the flipped expectation and the render tests at the foot of this + * file, which pin the thing that made closing the second site safe. * * Same defect, same key name and same package as objectui#5940, which fixed the * sibling site in `MasterDetailForm`. `childObject` is declared @@ -39,7 +46,7 @@ */ import { describe, it, expect, vi } from 'vitest'; -import { render, act } from '@testing-library/react'; +import { render, act, fireEvent } from '@testing-library/react'; import React from 'react'; import { SchemaRenderer, SchemaRendererProvider } from '@object-ui/react'; // Registers `record:line_items`. @@ -57,15 +64,38 @@ const COLUMNS = [{ name: 'qty', label: 'Qty', type: 'number' as const }]; * child fetch also carries `$filter` / `$top`, and pinning those would make this * file fail for changes that have nothing to do with the object name. */ -function recordingDataSource(calls: string[]) { +function recordingDataSource(calls: string[], writes: string[] = []) { const record = (key: string) => (...args: unknown[]) => { calls.push(`${key}(${args.length === 0 ? '' : (JSON.stringify(args[0]) ?? 'undefined')})`); + // The WRITE side, recorded separately and spelled by OBJECT NAME + // (objectui#6194). It needs its own channel because the save arrives as + // ONE `batchTransaction(ops)` call whose object names live inside the + // array — and because `JSON.stringify` DROPS an undefined value, so the + // op for an unresolvable panel would read as a create carrying no object + // key at all rather than one naming `undefined`. `String(op.object)` says + // it out loud. + if (key === 'batchTransaction') { + for (const op of ((args[0] as any[]) ?? [])) { + writes.push(`${op?.action}(${String(op?.object)})`); + } + } else if (key === 'create' || key === 'update' || key === 'delete') { + writes.push(`${key}(${String(args[0])})`); + } return /^on[A-Z]/.test(key) || key === 'subscribe' ? () => {} : Promise.resolve({ data: [] }); }; const seeded: Record = {}; - for (const m of ['find', 'findOne', 'create', 'update', 'delete', 'aggregate', 'getObjectSchema']) { + for (const m of [ + 'find', + 'findOne', + 'create', + 'update', + 'delete', + 'aggregate', + 'getObjectSchema', + 'batchTransaction', + ]) { seeded[m] = record(m); } return new Proxy(seeded, { @@ -73,9 +103,8 @@ function recordingDataSource(calls: string[]) { }) as any; } -async function callsFor(schemaExtra: Record): Promise { - const calls: string[] = []; - const schema: any = { +function panelSchema(schemaExtra: Record): any { + return { type: 'record:line_items', relationshipField: 'invoice', // Authored directly, as `LineItemsPanel.elementDataSource.test.tsx` does: @@ -85,18 +114,28 @@ async function callsFor(schemaExtra: Record): Promise columns: COLUMNS, ...schemaExtra, }; - const view = render( - - - , - ); - // Settle: both reads run in effects, and the row load runs a second pass once - // the first response lands. +} + +/** + * Settle: both reads run in effects, and the row load runs a second pass once + * the first response lands. + */ +async function settle() { for (let i = 0; i < 10; i++) { await act(async () => { await new Promise((resolve) => setTimeout(resolve, 50)); }); } +} + +async function callsFor(schemaExtra: Record): Promise { + const calls: string[] = []; + const view = render( + + + , + ); + await settle(); try { view.unmount(); } catch { @@ -112,18 +151,15 @@ describe('record:line_items — a panel with no childObject (objectui#6188)', () // The FULL LIST, not `.not.toContain(...)`: an absence-only assertion would // also pass for a panel that stopped fetching altogether, and the list is - // what makes the remaining entry visible instead of implied. + // what makes any remaining entry visible instead of implied. // - // ⚠️ `find(undefined)` IS STILL HERE AND IS STILL WRONG. It is the SIBLING - // site in this same component — `load()` guards `dataSource` and `parentId` - // but not `schema.childObject`, so the row fetch still queries an object - // literally named `undefined`. It is deliberately NOT fixed by objectui#6188, - // whose dispatch order scoped this card to the `getObjectSchema` call and - // said to FILE any further unguarded sub-key site rather than fix it; filed - // as objectui#6194. Pinned here rather than hidden behind a narrower fixture - // so the hole is recorded where the next reader will see it. When #6194 - // lands, this expectation becomes `toEqual([])` and the line below it goes. - expect(calls).toEqual(['find(undefined)']); + // EMPTY as of objectui#6194, which closed the second site. This assertion + // read `['find(undefined)']` while that hole was open — the row fetch + // queried an object literally named `undefined` because `load()` guarded + // `dataSource` and `parentId` but not `schema.childObject`. Keep it an + // exact-list assertion: it is the only shape that catches a THIRD read of + // this key being added unguarded, which is how the second one arrived. + expect(calls).toEqual([]); // Stated separately so a failure names which half broke. expect(calls).not.toContain('getObjectSchema(undefined)'); @@ -134,6 +170,12 @@ describe('record:line_items — a panel with no childObject (objectui#6188)', () // rows"), and the warning names the key and what to set it to rather than // reporting that something was undefined. expect(warn).toHaveBeenCalledWith(expect.stringContaining('childObject')); + + // BOTH declines say so, and each names what IT refused rather than leaving + // the author to guess which read stopped (objectui#6194). + const warned = warn.mock.calls.map((c) => String(c[0])); + expect(warned.some((m) => m.includes('refusing to fetch its child schema'))).toBe(true); + expect(warned.some((m) => m.includes('refusing to fetch its rows'))).toBe(true); warn.mockRestore(); }); @@ -146,3 +188,140 @@ describe('record:line_items — a panel with no childObject (objectui#6188)', () expect(calls).not.toContain('getObjectSchema(undefined)'); }); }); + +/** + * ⭐ WHY THIS SECOND SECTION EXISTS — the decline above is not safe on its own. + * + * `load` owns `loading`, and the panel used to branch + * `loading ? "Loading…" : !parentId ? "Save the record first…" : `. So the + * moment the row fetch declines, an unresolvable panel with a `parentId` bound + * lands on the THIRD branch: an empty EDITABLE grid with an Add button, over an + * object that does not exist. That is a worse outcome than the unguarded fetch + * objectui#6194 removed, and it is reachable *because* of the fix — which is why + * the card treated the render branch as its real question rather than a polish + * item, and why it is pinned here next to the decline that creates it. + * + * It is also what makes the SAVE path unreachable. Measured on the component as + * it stood before this fix, with the fixture below and `childObject` unset: + * + * grid rendered = true + * Add button = true + * Save disabled at t0 = true + * after ONE keystroke in the always-present ghost row: + * Save disabled = false + * reached the adapter = batchTransaction([ + * { object: undefined, action: 'create', + * data: { qty: 3, invoice: 'inv-1' } } ]) + * + * So the write was reachable, in one keystroke, through the very affordance the + * empty grid advertises ("No items yet — click Add to begin"). Removing the grid + * removes the only producer of `dirty`, which is the only thing that enables + * Save. The guard in `save` itself covers the case this render branch cannot — + * a schema edited to drop `childObject` while rows are already dirty — and the + * last test here is the only thing that exercises it. + */ +describe('record:line_items — an unresolvable panel offers no editable grid (objectui#6194)', () => { + function saveButton(container: HTMLElement): HTMLButtonElement | undefined { + return Array.from(container.querySelectorAll('button')).find((b) => + /^Sav(e|ing)/.test((b.textContent || '').trim()), + ) as HTMLButtonElement | undefined; + } + + it('shows a config hint instead of an empty editable grid, and writes nothing', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const calls: string[] = []; + const writes: string[] = []; + const view = render( + + + , + ); + await settle(); + + // The hint, and it NAMES the key — an author who cannot see the console is + // the one who has to act on this. + const hint = view.container.querySelector('[data-testid="line-items-no-child-object"]'); + expect(hint).not.toBeNull(); + expect(hint?.textContent).toContain('childObject'); + + // ⭐ The half that matters: NOT the grid. Each of these is a separate + // assertion so a failure names which affordance came back. + expect(view.container.querySelector('[data-testid="line-items"]')).toBeNull(); + expect(view.container.querySelector('[data-testid="line-items-add"]')).toBeNull(); + // No cell to type in — the ghost row is what made the save path reachable in + // ONE keystroke, without the Add button being touched at all. + expect(view.container.querySelector('input[aria-label="Qty"]')).toBeNull(); + + // ⛔ And NOT a spinner that never ends: hiding a permanent authoring error + // behind `loading` would satisfy every assertion above and is the one + // alternative this card ruled out by name. + expect(view.container.textContent).not.toContain('Loading…'); + + // With no producer of `dirty`, Save cannot leave its disabled state. + expect(saveButton(view.container)?.disabled).toBe(true); + expect(writes).toEqual([]); + warn.mockRestore(); + }); + + it('still renders the editable grid for a panel that names its child object', async () => { + // ⭐ The other direction, the same discipline as the schema tests above: a + // "fix" that showed the config hint unconditionally would pass every + // assertion in the test before this one. + const calls: string[] = []; + const view = render( + + + , + ); + await settle(); + + expect(view.container.querySelector('[data-testid="line-items-no-child-object"]')).toBeNull(); + expect(view.container.querySelector('[data-testid="line-items"]')).not.toBeNull(); + expect(view.container.querySelector('input[aria-label="Qty"]')).not.toBeNull(); + }); + + it('refuses to save when the child object is dropped from a panel with dirty rows', async () => { + // The one route the render branch cannot close, and the reason `save` takes + // the guard too: the grid was offered legitimately, the user dirtied it, and + // only THEN did the schema lose `childObject` — a live edit in the designer. + // `dirty` survives that re-render, so Save is enabled over a panel whose + // object no longer resolves. + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const calls: string[] = []; + const writes: string[] = []; + const view = render( + + + , + ); + await settle(); + + const cell = view.container.querySelector('input[aria-label="Qty"]') as HTMLInputElement; + expect(cell).not.toBeNull(); + await act(async () => { + fireEvent.change(cell, { target: { value: '3' } }); + }); + + // Precondition, asserted rather than assumed: without it the test would pass + // for a Save that was disabled the whole time and prove nothing. + expect(saveButton(view.container)?.disabled).toBe(false); + + view.rerender( + + + , + ); + await settle(); + + const save = saveButton(view.container); + expect(save?.disabled).toBe(false); // still dirty — the guard is what stops it + await act(async () => { + fireEvent.click(save as HTMLButtonElement); + }); + await settle(); + + // Nothing named `undefined` reached the adapter. + expect(writes).toEqual([]); + warn.mockRestore(); + }); +}); diff --git a/packages/plugin-form/src/LineItemsPanel.tsx b/packages/plugin-form/src/LineItemsPanel.tsx index 234898c054..64ef8c364d 100644 --- a/packages/plugin-form/src/LineItemsPanel.tsx +++ b/packages/plugin-form/src/LineItemsPanel.tsx @@ -162,6 +162,31 @@ export const LineItemsPanel: React.FC<{ schema: LineItemsPanelSchema }> = ({ sch setLoading(false); return; } + // Decline to fetch when the child object never resolved (objectui#6194) — + // the SIBLING site of the child-schema decline above (objectui#6188), and + // the second of this component's two reads of `schema.childObject`. Same + // reason it is a defect and not a shrug: nothing enforces the key (see the + // effect above), so an authored node reaches this renderer with it + // `undefined` and the fetch below then asked the data layer to `find` an + // object literally named `undefined`, scoped by + // `{ [relationshipField]: parentId }`. + // + // Ordered AFTER the dataSource/parentId guard on purpose, exactly as the + // child-schema effect orders its own: a designer palette renders this block + // with no dataSource at all while the author is still configuring it, and + // warning there would be noise about a panel nobody has finished authoring. + // + // `setLoading(false)` because this panel is NOT loading. It can never + // resolve, and holding `loading` true would only hide a permanent authoring + // error behind a spinner that never ends — the render below therefore reads + // `schema.childObject` ahead of `loading` and says what is wrong. + if (!schema.childObject) { + setLoading(false); + console.warn( + `[LineItemsPanel] a line-items panel has no childObject — refusing to fetch its rows. Set childObject to the child object the panel lists.`, + ); + return; + } setLoading(true); try { // Parent relationship AND the panel's own criteria (objectstack#7137). @@ -209,7 +234,18 @@ export const LineItemsPanel: React.FC<{ schema: LineItemsPanelSchema }> = ({ sch }, []); const save = useCallback(async () => { - if (!dataSource || !parentId) return; + // `childObject` joins this guard for the same reason both reads decline + // (objectui#6194): every child op below carries `object: schema.childObject`, + // so an unresolvable panel would WRITE rows into an object literally named + // `undefined` — strictly worse than the read this card was filed for. + // Measured on the pre-fix component, which is why this is not a guess: one + // keystroke in the grid's always-present ghost row materialised a non-blank + // row, that enabled Save, and Save reached + // `batchTransaction([{ object: undefined, action: 'create', … }])`. + // The render branch below closes that route by not offering the grid, but a + // write contract is not the render tree's to keep: this is the same one-line + // guard `load` takes, on the component's other data-layer entry point. + if (!dataSource || !parentId || !schema.childObject) return; setSaving(true); setError(null); try { @@ -275,7 +311,28 @@ export const LineItemsPanel: React.FC<{ schema: LineItemsPanelSchema }> = ({ sch {error &&

{error}

} - {loading ? ( + {/* An unresolvable panel gets its OWN branch, ahead of `loading` + (objectui#6194) — following objectui#5940's config-hint precedent for + this exact key, and `AdvancedChartImpl`'s refusal placeholders + ("This chart cannot plot its category axis: no row has a `x` field"). + Two things it must not do. It must not fall through to the grid: an + empty EDITABLE grid with an Add button, over an object that does not + exist, is a worse outcome than the unguarded fetch this card removes, + and it is exactly what made the save path reachable. And it must not + sit on `loading`, which would hide a permanent authoring error behind + a spinner that can never end. Checked BEFORE `loading` because + nothing here is pending — the schema itself already says this panel + can never resolve, so there is no first paint where "Loading…" is + true. */} + {!schema.childObject ? ( +

+ This panel has no child object configured: set{' '} + childObject to the object whose rows it lists. +

+ ) : loading ? (

Loading…

) : !parentId ? (