diff --git a/.changeset/6271-kanban-fetch-gate.md b/.changeset/6271-kanban-fetch-gate.md new file mode 100644 index 0000000000..dd74c6c0bb --- /dev/null +++ b/.changeset/6271-kanban-fetch-gate.md @@ -0,0 +1,41 @@ +--- +'@object-ui/plugin-kanban': patch +--- + +`ObjectKanban` no longer queries twice on mount (objectui#6271). A standalone board issued +its first `find` before the object definition resolved — so `buildExpandFields` saw no +fields and that query carried no `$expand` at all — then issued a second, expanded one once +the definition landed: + +``` +['deal', { $top: 100 }] +['deal', { $top: 100, $expand: ['owner'] }] +``` + +The definition now GATES the query instead of refining it afterwards: one query per mount, +carrying the expansion the first time. + +Decided on measurement rather than on the two shapes' relative appeal. The first response +never reached the screen in the regimes that matter: with the schema resolving no slower +than the row query (profiles schema/find = 30/30, 30/60, 5/30 ms), the definition lands +first, the effect re-runs, its cleanup flips `isMounted` false, and the unexpanded rows are +discarded on arrival — a DOM probe polling every 2ms for a title only that response carried +never fired once. What the gate costs is one schema resolution ahead of the query, and that +read is cheap and shared: one small GET behind the same discovery call `find` already +awaits, served thereafter from `MetadataCache` (5-minute TTL, concurrent readers coalesced +onto one request). Measured against the real `ObjectStackAdapter` over loopback HTTP, 22 +reads of one object produced exactly one metadata request and every read after the first +returned in 0.01ms. End to end the board is not slower for it — same harness, before → +after, time to the fully populated board: 156.9 → 145.2ms (30/30), 119.8 → 110.6ms (30/60), +54.7 → 52.4ms (5/30). + +The gate is on the definition read having **settled**, not on the definition being truthy: +an adapter that exposes no `getObjectSchema`, and a read that throws, both settle with +nothing to report and the board falls through to an unexpanded query rather than waiting +forever. Boards fed rows by a parent (`data`, `bind`, inline `schema.data`) are untouched — +they never ran this effect, and they still read the definition for lane titles and labels. + +The `isOpaqueId` suppression in the card-description path is unchanged and keeps its +comment beside it: part of what it hid was this fetch ordering, but unexpanded rows still +reach it from parents that pass rows they fetched without `$expand`, from author-supplied +data, and from backends that decline an expansion. diff --git a/packages/plugin-kanban/src/ObjectKanban.tsx b/packages/plugin-kanban/src/ObjectKanban.tsx index 2c99db839a..1e99dfa648 100644 --- a/packages/plugin-kanban/src/ObjectKanban.tsx +++ b/packages/plugin-kanban/src/ObjectKanban.tsx @@ -172,7 +172,24 @@ export const ObjectKanban: React.FC = ({ const hasExternalData = Array.isArray(externalData); const [fetchedData, setFetchedData] = useState([]); - const [objectDef, setObjectDef] = useState(null); + // The object-definition read and the fact that it has SETTLED are one piece + // of state, keyed by the object it belongs to (objectui#6271). Two separate + // states could disagree for one commit — long enough for the record query to + // fire against the previous object's expand set — and a bare `objectDef` + // cannot express "settled with nothing", which is a legitimate outcome (an + // adapter with no `getObjectSchema`, or a read that threw). `key` is compared + // against the CURRENT object name during render, so switching objects closes + // the gate in the same commit that changes it, not one commit later. + const [schemaResolution, setSchemaResolution] = useState<{ key: string; def: any } | null>(null); + const schemaKey = schema.objectName ?? ''; + /** + * Has the object definition for THIS object finished resolving? Note what + * this is NOT: "`objectDef` is truthy". A board whose adapter exposes no + * `getObjectSchema`, or whose schema read failed, must still get its cards — + * gating on a truthy definition would leave those boards empty forever. + */ + const objectDefReady = schemaResolution !== null && schemaResolution.key === schemaKey; + const objectDef = objectDefReady ? schemaResolution.def : null; // loading state const [loading, setLoading] = useState(hasExternalData ? (externalLoading ?? false) : false); const [error, setError] = useState(null); @@ -201,16 +218,29 @@ export const ObjectKanban: React.FC = ({ } }, [externalLoading, hasExternalData]); - // Fetch object definition for metadata (labels, options) + // Fetch object definition for metadata (labels, options). + // + // Every exit settles the resolution — success, failure, and "there is nothing + // to read from" alike — because the record query below WAITS on this + // (objectui#6271). A path that returned without settling would not merely + // skip the expansion, it would hold the query open forever. useEffect(() => { let isMounted = true; + const key = schema.objectName ?? ''; const fetchMeta = async () => { - if (!dataSource || !schema.objectName) return; + if (!dataSource || !schema.objectName || typeof dataSource.getObjectSchema !== 'function') { + // No source for a definition: settle with none, so the board still + // queries (unexpanded — with no schema there is no expand set to + // derive, which is the same query this case produced before). + if (isMounted) setSchemaResolution({ key, def: null }); + return; + } try { const def = await dataSource.getObjectSchema(schema.objectName); - if (isMounted) setObjectDef(def); + if (isMounted) setSchemaResolution({ key, def }); } catch (e) { console.warn("Failed to fetch object def", e); + if (isMounted) setSchemaResolution({ key, def: null }); } }; fetchMeta(); @@ -221,12 +251,36 @@ export const ObjectKanban: React.FC = ({ // Skip internal fetch when data is managed by a parent component if (hasExternalData) return; + // ⭐ objectui#6271 — the object definition GATES this query; it does not + // refine it afterwards. Before this line the effect ran twice on every + // mount: once with `objectDef` still unresolved (so `buildExpandFields` + // saw no fields and the query carried NO `$expand` at all), then again + // once the definition landed. Measured on the standalone board, the first + // response never even reached the screen in the regimes that matter — the + // definition settles first, the effect re-runs, its cleanup flips + // `isMounted` false, and the unexpanded rows are dropped on arrival. So + // that round trip bought no earlier paint; it was a query whose answer was + // thrown away. + // + // What the gate costs is one schema resolution before the first query, and + // that is the measurement this was decided on: a metadata read is one small + // GET behind the same shared discovery call `find` already awaits, and it + // is served from `MetadataCache` (5-min TTL, concurrent readers coalesced + // onto one request) for every reader after the first — 0.01ms, no request. + // Anything hosting this board (ObjectView, ListView) has already read the + // same definition through the same adapter, so the gate is free there. + if (!objectDefReady) return; + let isMounted = true; const fetchData = async () => { if (!dataSource || typeof dataSource.find !== 'function' || !schema.objectName) return; if (isMounted) setLoading(true); try { - // Auto-inject $expand for lookup/master_detail fields + // Auto-inject $expand for lookup/master_detail fields. Reached only + // with the definition resolved (the gate above), so a board whose + // object declares lookups queries WITH its expansion the first + // time — `objectDef` here is `null` only when there was nothing to + // resolve it from. const expand = buildExpandFields(objectDef?.fields); // The row cap is a REAL `$top` (objectui#4025). It used to be // `{ options: { $top: 100 } }` — `$filter` at the top level where the @@ -259,7 +313,11 @@ export const ObjectKanban: React.FC = ({ fetchData(); } return () => { isMounted = false; }; - }, [schema.objectName, dataSource, boundData, schema.data, schema.filter, schema.limit, hasExternalData, objectDef, refreshKey]); + // `objectDefReady` is what re-runs this effect once the definition lands; + // `objectDef` stays listed because the body reads it, and with the gate in + // place the two flip together in one commit — the pre-resolution run now + // returns above without querying instead of issuing an unexpanded one. + }, [schema.objectName, dataSource, boundData, schema.data, schema.filter, schema.limit, hasExternalData, objectDefReady, objectDef, refreshKey]); // Determine which data to use: external -> bound -> inline -> fetched const rawData = (hasExternalData ? externalData : undefined) || boundData || schema.data || fetchedData; @@ -321,6 +379,34 @@ export const ObjectKanban: React.FC = ({ }; // Detect strings that look like opaque foreign-key IDs so we don't dump // gibberish into card descriptions when the server didn't expand the lookup. + // + // ⭐ WHY THIS IS STILL HERE, given objectui#6271 (read this before + // deleting it). Part of what this suppression used to hide was THIS + // component's own fetch ordering: the board issued its first query before + // the object definition resolved, so that query carried no `$expand` and + // the first paint was rendered from raw lookup ids. That half is gone — + // the record query is now gated on the definition (see the fetch effect + // above), so the board's own rows arrive expanded or not at all. + // + // The suppression is NOT thereby redundant, because unexpanded rows still + // reach this function from sources the gate does not sit in front of: + // + // 1. `data` handed down by a parent. Measured, not assumed: ObjectView + // hosts the board this way and its own query goes out as + // `{ $top: 100 }` — it reads the schema through a ref that is still + // empty on the one run it makes, so it never injects `$expand` at + // all. Every card in that path is built from raw ids. + // 2. `bind` / inline `schema.data` — author-supplied rows, expanded by + // nobody. + // 3. An adapter or backend that ignores `$expand`, or a single lookup + // it cannot resolve (a dangling reference), which comes back as the + // bare id inside an otherwise expanded row. + // + // So the coupling the two issues share is real and stays recorded: the + // display heuristic and the fetch ordering move together. What changed is + // the JUSTIFICATION, not the code — this predicate is now a guard against + // genuinely unexpanded DATA, no longer a cover for a query this component + // issued too early. const OPAQUE_ID_RE = /^[A-Za-z0-9_-]{12,32}$/; const isOpaqueId = (v: unknown): boolean => { if (typeof v !== 'string') return false; diff --git a/packages/plugin-kanban/src/__tests__/expandableFamily.identity-5874.test.tsx b/packages/plugin-kanban/src/__tests__/expandableFamily.identity-5874.test.tsx index 6e2d005d70..2a3e76dbf2 100644 --- a/packages/plugin-kanban/src/__tests__/expandableFamily.identity-5874.test.tsx +++ b/packages/plugin-kanban/src/__tests__/expandableFamily.identity-5874.test.tsx @@ -296,15 +296,24 @@ describe('the membership delta is OBSERVABLE on this read — the counter-probe * stand in `resolveDisplay` lacked. So this pair is exactly the delta the * convergence introduced, read off the wire instead of off the DOM. * - * ⚠️ The board fetches TWICE: once before the object schema resolves (no - * field types are known yet, so no `$expand` can be computed) and again once - * `objectDef` arrives. Only the second call can carry the delta, so both - * cases below wait for it — otherwise the negative case would be green - * against a query that had not been built yet, which is the vacuous form of - * this assertion. + * ⚠️ This wait exists to keep the negative case from going green against a + * query that had not been built yet — the vacuous form of this assertion. It + * used to spell that as "wait for a SECOND call", because the board fetched + * twice: once before the object definition resolved (no field types known, + * so no `$expand` computable) and again once it arrived, and only the second + * could carry the delta. + * + * objectui#6271 gated the query on the definition instead, so there is now + * exactly ONE call and it is the schema-informed one. The wait moved with it + * — same condition ("a query built from resolved field types has been + * issued"), spelled against the ordering that actually holds. ⛔ Do not read + * this as a weakened wait: what makes one call sufficient is the gate. If + * that gate is ever removed, the pre-resolution query comes back, this wait + * starts admitting it, and the negative case below goes vacuous again — + * `fetchGate.objectDef-6271.test.tsx` is what fails first if anyone tries. */ const awaitSchemaFetch = (adapter: ReturnType) => - waitFor(() => expect(adapter.find.mock.calls.length).toBeGreaterThanOrEqual(2)); + waitFor(() => expect(adapter.find.mock.calls.length).toBeGreaterThanOrEqual(1)); const everyExpand = (adapter: ReturnType): string[] => adapter.find.mock.calls.flatMap( diff --git a/packages/plugin-kanban/src/__tests__/fetchGate.objectDef-6271.test.tsx b/packages/plugin-kanban/src/__tests__/fetchGate.objectDef-6271.test.tsx new file mode 100644 index 0000000000..d256ae87b1 --- /dev/null +++ b/packages/plugin-kanban/src/__tests__/fetchGate.objectDef-6271.test.tsx @@ -0,0 +1,230 @@ +/** + * 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#6271 — the object definition GATES `ObjectKanban`'s record query. + * + * ## What this replaces + * + * The board ran its fetch effect twice on every standalone mount, because + * `objectDef` was in that effect's dependency list while being resolved by a + * SEPARATE effect. Captured from a real render, the two argument sets in order: + * + * ['deal', { $top: 100 }] + * ['deal', { $top: 100, $expand: ['owner'] }] + * + * The first ran before the definition resolved, so `buildExpandFields` saw no + * fields and the query carried no `$expand` at all. + * + * ## Why gating, and not "make the unexpanded first paint deliberate" + * + * Both were live options and the choice was made on measurement, not taste. + * + * 1. That first response never reached the screen in the regimes that + * matter. With the schema resolving no slower than the row query + * (measured profiles schema/find = 30/30, 30/60, 5/30 ms), the definition + * lands first, the effect re-runs, its cleanup flips `isMounted` false — + * and the unexpanded rows are DISCARDED on arrival. A probe watching the + * DOM every 2ms for a title only the first response carried never fired + * once. So the extra round trip bought no earlier paint; it bought a + * query whose answer was thrown away. + * 2. What the gate costs is one schema resolution ahead of the query, and a + * schema read is cheap and shared: one small GET behind the same + * discovery call `find` already awaits, served thereafter from + * `MetadataCache` (5-minute TTL, concurrent readers coalesced onto ONE + * request). Measured against the real `ObjectStackAdapter` over loopback + * HTTP: 22 reads of the same object produced exactly one metadata + * request, and every read after the first returned in 0.01ms. + * 3. End to end the board is not slower for it. Same harness, before/after: + * the fully-populated board landed at 156.9 → 145.2ms (30/30), + * 119.8 → 110.6ms (30/60), 54.7 → 52.4ms (5/30) — one query instead of + * two, and nothing left to overwrite. + * + * ## ⚠️ What "gated" must mean — the trap this file exists to hold shut + * + * The gate is on the definition read having **settled**, NOT on `objectDef` + * being truthy. Those differ for exactly the boards least able to report it: + * an adapter that exposes no `getObjectSchema`, and a schema read that throws. + * Under a truthy-value gate both wait forever and the board renders empty, with + * no error and no request — the third and fourth tests below are red the moment + * anyone writes that. They are also the control the first test needs: a + * query-count assertion that would pass with ZERO fetches is decoration, so + * every count here is reached only after waiting for a real call. + */ +import { describe, it, expect, vi } from 'vitest'; +import { render, waitFor } from '@testing-library/react'; +import React from 'react'; +import { SchemaRenderer, SchemaRendererProvider } from '@object-ui/react'; +// Registers `object-kanban`. +import '../index'; +// The cards asserted below render INSIDE `KanbanRenderer`'s `React.lazy` +// boundary. Importing the chunk at module scope bills the cold transform to the +// import phase (unbounded) instead of racing a `waitFor` budget under full +// parallelism — the objectui#3010 rule, same specifier as `index.tsx`'s factory +// so ESM's module cache makes that factory resolve immediately. +import '../KanbanImpl'; + +const DEAL_SCHEMA = { + name: 'deal', + label: 'Deal', + fields: { + name: { type: 'text', label: 'Name' }, + status: { type: 'text', label: 'Status' }, + // The only expandable member, so `$expand` has one predictable entry. + owner: { type: 'lookup', reference_to: 'user', label: 'Owner' }, + }, +}; + +const ROWS = [{ id: 'd1', name: 'Q3 renewal', status: 'open', owner: { id: 'u1', name: 'Jane Ops' } }]; + +/** + * `getObjectSchema` deliberately resolves a tick LATER than a bare + * `mockResolvedValue` would, so a board that queries before the definition + * settles is caught rather than accidentally passing on scheduling luck. + */ +function makeAdapter( + getObjectSchema?: () => Promise, +): Record { + const order: string[] = []; + const adapter: Record = { + order, + find: vi.fn(async (_object: string, _params: any) => { + order.push('find'); + return { data: ROWS }; + }), + findOne: vi.fn(), + create: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + }; + if (getObjectSchema) { + adapter.getObjectSchema = vi.fn(async () => { + order.push('schema:issued'); + try { + return await getObjectSchema(); + } finally { + order.push('schema:settled'); + } + }); + } + return adapter; +} + +const resolvesSchema = () => + makeAdapter(async () => { + await new Promise((r) => setTimeout(r, 10)); + return DEAL_SCHEMA; + }); + +function renderBoard(adapter: Record) { + return render( + + + , + ); +} + +const paramsOf = (adapter: Record) => + adapter.find.mock.calls.map((c: any[]) => c[1] ?? {}); +const unexpandedCalls = (adapter: Record) => + paramsOf(adapter).filter((p: any) => !Array.isArray(p.$expand) || p.$expand.length === 0); + +describe('ObjectKanban gates its record query on the object definition (objectui#6271)', () => { + it('issues ONE query, and it carries the object’s `$expand`', async () => { + const adapter = resolvesSchema(); + renderBoard(adapter); + + // Wait for the EXPANDED query specifically. This is the control that keeps + // the two assertions under it from being vacuous: if the gate ever stops + // opening, no call is recorded, this times out, and the file goes red — + // "0 queries" can never read as success here. + await waitFor(() => + expect(paramsOf(adapter).filter((p: any) => p.$expand?.includes('owner'))).toHaveLength(1), + ); + + // RED before the fix: the board also issued `['deal', { $top: 100 }]` + // before the definition resolved. + expect(unexpandedCalls(adapter)).toEqual([]); + expect(adapter.find).toHaveBeenCalledTimes(1); + expect(adapter.find.mock.calls[0][0]).toBe('deal'); + }); + + it('issues that query only AFTER the definition read settles', async () => { + const adapter = resolvesSchema(); + renderBoard(adapter); + + await waitFor(() => expect(adapter.find).toHaveBeenCalled()); + // Ordering, not just counting: a fix that merely deduplicated the second + // query would satisfy the test above while still querying too early. + expect(adapter.order).toEqual(['schema:issued', 'schema:settled', 'find']); + }); + + it('still queries — and paints — when the adapter exposes NO `getObjectSchema`', async () => { + // The gate is on the read having settled, not on a truthy definition. An + // adapter without the method settles with nothing to report, and the board + // must fall through to an unexpanded query rather than wait forever. + const adapter = makeAdapter(); + const { container } = renderBoard(adapter); + + await waitFor(() => expect(container.textContent).toContain('Q3 renewal')); + expect(adapter.find).toHaveBeenCalledTimes(1); + // Nothing declared any field, so there is no expand set to derive. + expect(unexpandedCalls(adapter)).toHaveLength(1); + }); + + it('still queries — and paints — when the definition read REJECTS', async () => { + const adapter = makeAdapter(async () => { + await new Promise((r) => setTimeout(r, 10)); + throw new Error('metadata endpoint down'); + }); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + try { + const { container } = renderBoard(adapter); + await waitFor(() => expect(container.textContent).toContain('Q3 renewal')); + expect(adapter.find).toHaveBeenCalledTimes(1); + expect(unexpandedCalls(adapter)).toHaveLength(1); + expect(adapter.order).toEqual(['schema:issued', 'schema:settled', 'find']); + } finally { + warn.mockRestore(); + } + }); + + it('a parent-fed board still reads the definition and issues no query of its own', async () => { + // `data` from a parent has always suppressed the internal fetch; the gate + // must not have turned that into a suppressed SCHEMA read, which is what + // the lane titles and card labels are built from. + const adapter = resolvesSchema(); + render( + + + , + ); + + await waitFor(() => expect(adapter.getObjectSchema).toHaveBeenCalled()); + expect(adapter.find).not.toHaveBeenCalled(); + }); +});