diff --git a/.changeset/6419-objectview-expand-gate.md b/.changeset/6419-objectview-expand-gate.md new file mode 100644 index 0000000000..8b53c9e1a0 --- /dev/null +++ b/.changeset/6419-objectview-expand-gate.md @@ -0,0 +1,32 @@ +--- +'@object-ui/plugin-view': patch +--- + +`ObjectView`'s non-grid fetch now carries `$expand` (objectui#6419). The effect built its +expand set from `objectSchemaRef.current` — a ref assigned in the render body, deliberately +kept out of the effect's dependency list so the effect would run exactly once per mount. On +that one run the ref was still `null`, so `buildExpandFields` saw no fields and the query +went out as `{ $top: 100 }` with no `$expand` at all; because the effect never re-ran on the +schema's arrival, it never went out with one either. + +`ObjectView` hands the rows it fetches to the child view as `data={data}`, which suppresses +that child's own fetch. So every lookup / master_detail / user / tree field in the six +non-grid views it hosts — kanban, calendar, gallery, timeline, gantt, map — rendered from +raw foreign-key ids: blank on the kanban (its `resolveDisplay` suppresses opaque ids) and +potentially the raw id on the other five. + +The object schema and the fact that its read has SETTLED are now one piece of state, keyed +by object name, and the record query waits on it — the shape `ObjectKanban` adopted in +objectui#6271. The gate is on the read having settled, **not** on a truthy schema: a view +whose adapter exposes no `getObjectSchema`, or whose read threw, still queries (unexpanded) +rather than waiting forever, and switching objects closes the gate in the same commit rather +than sending the previous object's expand set. + +The trade was measured on this effect rather than inherited, because it has five more +dependencies than the board's. With an instrumented adapter (schema and `find` both 30ms) +across four host regimes: before, one query with no `$expand` and one raw delivery to the +child; with `objectSchema` merely added to the dependency list, two queries and two +deliveries — `raw` then `expanded`, a visible two-step paint, because here the raw rows +settle into state *before* the re-run's cleanup rather than being discarded as they were on +the board; gated, one query carrying `$expand` the first time and a single expanded +delivery, with correct rows landing at the same wall clock as the dependency version. diff --git a/packages/plugin-view/src/ObjectView.tsx b/packages/plugin-view/src/ObjectView.tsx index 23c0163f2b..0ed68e6a51 100644 --- a/packages/plugin-view/src/ObjectView.tsx +++ b/packages/plugin-view/src/ObjectView.tsx @@ -22,7 +22,7 @@ * - ViewSwitcher for toggling between view types */ -import React, { useEffect, useState, useCallback, useMemo, useRef } from 'react'; +import React, { useEffect, useState, useCallback, useMemo } from 'react'; import type { ObjectViewSchema, ObjectGridSchema, @@ -618,12 +618,34 @@ export const ObjectView: React.FC = ({ // Declared with the other top-level hooks so it stays above every conditional // return — rules-of-hooks. const { t: tView } = useObjectViewTranslation(); - const [objectSchema, setObjectSchema] = useState | null>(null); - // Assigned in the render body (not in an effect) so the fetchData effect always - // reads the latest objectSchema without needing it as a dependency. This matches - // the same pattern used in ObjectCalendar's objectSchemaRef. - const objectSchemaRef = useRef | null>(null); - objectSchemaRef.current = objectSchema; + // The object-schema read and the fact that it has SETTLED are ONE piece of + // state, keyed by the object it belongs to (objectui#6419). This replaces a + // `useState` + a render-body `objectSchemaRef.current = objectSchema` write, + // which existed so the non-grid fetch effect below could read the schema + // without listing it as a dependency. That bought the effect one run per + // mount — and paid for it with the expansion, permanently: on that one run + // the ref was still `null`, so `buildExpandFields` saw no fields and the + // query went out with no `$expand` at all, for every non-grid view this + // component hosts. + // + // Two separate states (`def` + `hasSettled`) could disagree for one commit — + // long enough for the record query to fire against the previous object's + // expand set — and a bare `objectSchema` 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: Record | null } | null>(null); + const schemaKey = schema.objectName ?? ''; + /** + * Has the object schema for THIS object finished resolving? Note what this is + * NOT: "`objectSchema` is truthy". A view whose adapter exposes no + * `getObjectSchema`, or whose schema read failed, must still fetch its rows — + * gating on a truthy schema would leave those views empty forever. + */ + const objectSchemaReady = schemaResolution !== null && schemaResolution.key === schemaKey; + const objectSchema = objectSchemaReady ? schemaResolution.def : null; const [isFormOpen, setIsFormOpen] = useState(false); const [formMode, setFormMode] = useState('create'); const [selectedRecord, setSelectedRecord] = useState | null>(null); @@ -707,20 +729,32 @@ export const ObjectView: React.FC = ({ // Navigation config const navigationConfig: ViewNavigationConfig | undefined = schema.navigation; - // Fetch object schema from ObjectQL/ObjectStack + // Fetch object schema from ObjectQL/ObjectStack. + // + // Every exit settles the resolution — success, failure, and "there is nothing + // to read from" alike — because the non-grid record query below WAITS on this + // (objectui#6419). A path that returned without settling would not merely + // skip the expansion, it would hold that query open forever. useEffect(() => { let isMounted = true; + const key = schema.objectName ?? ''; const fetchObjectSchema = async () => { + if (!schema.objectName || !dataSource || typeof dataSource.getObjectSchema !== 'function') { + // No source for a schema: settle with none, so the view 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 schemaData = await dataSource.getObjectSchema(schema.objectName); - if (isMounted) setObjectSchema(schemaData); + if (isMounted) setSchemaResolution({ key, def: schemaData }); } catch (err) { console.error('Failed to fetch object schema:', err); + if (isMounted) setSchemaResolution({ key, def: null }); } }; - if (schema.objectName && dataSource) { - fetchObjectSchema(); - } + fetchObjectSchema(); return () => { isMounted = false; }; }, [schema.objectName, dataSource]); @@ -737,6 +771,43 @@ export const ObjectView: React.FC = ({ if (currentViewType === 'grid') return; if (!dataSource || !schema.objectName) return; + // ⭐ objectui#6419 — the object schema GATES this query; it does not + // refine it afterwards. The shape is `ObjectKanban`'s (objectui#6271), + // but the measurement behind it is this component's own, because this + // effect has five more dependencies and the kanban's numbers do not + // transfer. Instrumented adapter, schema/find both 30ms, rows handed to + // the child as `data={data}`: + // + // before 1 find, `{$top:100}` — no `$expand`, EVER; the child + // receives exactly one delivery, of raw rows. + // `objectSchema` 2 finds, `[{$top:100}, {$top:100,$expand:[...]}]`; + // in the deps the child receives TWO deliveries, `raw` then + // `expanded`. + // gated (here) 1 find, carrying `$expand` the first time; one + // delivery, `expanded`. + // + // That middle row is where this component parts company with the kanban. + // On the board the unexpanded first response was DISCARDED on arrival + // (`isMounted` flipped false before it landed) — a wasted round trip, no + // visible artefact. Here the ordering measured is + // `schema:settled -> find:settled -> find:issued`: the raw rows settle + // into `setData` BEFORE the re-run's cleanup, reach the child, and paint. + // So an extra re-run here costs a visible two-step render — every + // lookup / master_detail / user / tree field blank (kanban's + // `isOpaqueId`) or a raw id for ~40ms, then swapping — which is exactly + // the "duplicate events in child views like the calendar" the ref this + // replaces was introduced to avoid. Gating avoids both. + // + // What the gate costs is one schema resolution ahead of the query, and + // this component ALREADY issues that read unconditionally on mount + // (measured: `getObjectSchema` calls = 1 in every regime, before and + // after). It is one small GET, served from `MetadataCache` (5-min TTL, + // concurrent readers coalesced onto one request) for every reader after + // the first. Correct, expanded rows land at the same wall clock as the + // dependency version reached them — with half the queries and no wrong + // paint in between. + if (!objectSchemaReady) return; + setLoading(true); try { // `mergeFilterNodes` rescues an OBJECT source: `table.defaultFilters` is @@ -790,11 +861,12 @@ export const ObjectView: React.FC = ({ || schema.table?.sort || (schema.table?.defaultSort ? [schema.table.defaultSort] : undefined); - // Auto-inject $expand for lookup/master_detail fields. - // Use a ref instead of the state variable to avoid re-running this effect - // every time the object schema loads — that would cause a double-fetch and - // duplicate events in child views like the calendar. - const expand = buildExpandFields((objectSchemaRef.current as any)?.fields); + // Auto-inject $expand for lookup/master_detail fields. Reached only + // with the schema resolved (the gate above), so a view whose object + // declares lookups queries WITH its expansion the first time — + // `objectSchema` here is `null` only when there was nothing to resolve + // it from. + const expand = buildExpandFields((objectSchema as any)?.fields); const results = await dataSource.find(schema.objectName, { // `mergeFilterNodes` returns a node or `undefined`; the old // `.length > 0` here was the second place an object filter was lost. @@ -833,9 +905,17 @@ export const ObjectView: React.FC = ({ fetchData(); return () => { isMounted = false; }; - // objectSchema intentionally omitted from deps — read via ref to prevent double-fetch - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [schema.objectName, dataSource, currentViewType, refreshKey, currentNamedViewConfig, activeView, renderListView]); + // `objectSchemaReady` and `objectSchema` are BOTH listed and both are load + // bearing: `objectSchema` is `null` in two different situations — before + // the read settles, and after it settles with nothing — and only the first + // of those may hold the query. Listing them is what makes the gate open; + // it is not the dependency-driven refetch this replaced, because the runs + // before the gate opens return above without querying. + }, [ + schema.objectName, dataSource, currentViewType, refreshKey, + currentNamedViewConfig, activeView, renderListView, + objectSchemaReady, objectSchema, + ]); // Determine layout mode. #2578: default the record surface from how heavy the // object is — a field-heavy object opens create/edit/detail as a full page, a diff --git a/packages/plugin-view/src/__tests__/ObjectView.expandGate.test.tsx b/packages/plugin-view/src/__tests__/ObjectView.expandGate.test.tsx new file mode 100644 index 0000000000..dc05768956 --- /dev/null +++ b/packages/plugin-view/src/__tests__/ObjectView.expandGate.test.tsx @@ -0,0 +1,341 @@ +/** + * 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#6419 — the object schema GATES `ObjectView`'s non-grid record query. + * + * ## What this replaces + * + * The non-grid fetch effect built its expand set from a REF + * (`objectSchemaRef.current`), assigned in the render body, and deliberately + * omitted `objectSchema` from its dependency list. That bought exactly one + * effect run per mount — and paid for it with the expansion, permanently: on + * that one run the ref was still `null`, `buildExpandFields` saw no fields, and + * the query went out as + * + * ['task', { $top: 100 }] ← no `$expand`, ever + * + * `ObjectView` hands those rows to the child as `data={data}`, which suppresses + * the child's own fetch, so every lookup / master_detail / user / tree field in + * the six non-grid views it hosts (kanban, calendar, gallery, timeline, gantt, + * map) rendered from raw foreign-key ids — blank on the kanban via + * `isOpaqueId`, potentially the raw id on the other five. + * + * ## Why gating, and why the kanban's numbers did not decide it + * + * objectui#6271 settled the same trade for `ObjectKanban`, but this effect has + * five more dependencies (`currentViewType`, `currentNamedViewConfig`, + * `activeView`, `renderListView`, `refreshKey`), so what an extra re-run costs + * HERE was measured separately. Instrumented adapter, `getObjectSchema` and + * `find` both resolving in 30ms, four host regimes (bare, named `listViews`, + * `views` prop, `views` prop with a re-rendering parent): + * + * before 1 find, `{$top:100}`; `$expand` NEVER sent. Child gets + * one delivery — raw rows. + * `objectSchema` 2 finds, `[{$top:100}, {$top:100,$expand:[…]}]`. Child + * added to the deps gets TWO deliveries: `raw`, then `expanded`. + * gated (this file) 1 find, carrying `$expand` the first time. One + * delivery — `expanded`. + * + * The middle row is where this component parts company with the board. On the + * kanban the unexpanded first response was DISCARDED on arrival (`isMounted` + * flipped false before it landed): a wasted round trip, no visible artefact. + * Here the measured order is `schema:settled -> find:settled -> find:issued` — + * the raw rows settle into `setData` BEFORE the re-run's cleanup, reach the + * child, and paint. An extra re-run on THIS effect therefore costs a visible + * two-step render (~40ms of blank-or-raw relation fields, then a swap), which + * is precisely the "duplicate events in child views like the calendar" the + * removed ref-comment cited. Gating avoids the wasted query AND the wrong + * paint; correct rows land at the same wall clock either way (66.8–69.1ms + * gated vs 68.3–69.3ms via the deps). + * + * ## ⚠️ What "gated" must mean — the trap this file exists to hold shut + * + * The gate is on the schema read having **settled**, NOT on `objectSchema` + * being truthy. Those differ for exactly the views least able to report it: an + * adapter exposing no `getObjectSchema`, and a read that throws. Under a + * truthy-value gate both wait forever and the view renders empty, with no error + * and no request — the third and fourth tests below go red the moment anyone + * writes that. + * + * ## ⚠️ Ghost-assertion guard + * + * A query count, or an `$expand` presence check, would ALSO pass if this view + * stopped fetching altogether. So: every count here is reached only after + * waiting for a real call; the first test's `waitFor` targets the EXPANDED call + * specifically, so zero fetches times out rather than reading as success; one + * test asserts rows actually reach the child; and `$expand` is asserted against + * the expandable fields DERIVED FROM THE FIXTURE SCHEMA, never a bare + * `toBeDefined()`. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render, waitFor } from '@testing-library/react'; +import { EXPANDABLE_FIELD_TYPES } from '@object-ui/core'; +import { ObjectView } from '../ObjectView'; +import type { ObjectViewSchema } from '@object-ui/types'; + +/** Every non-empty row array `ObjectView` hands the child view, in order. */ +const deliveries: any[][] = []; + +vi.mock('@object-ui/react', async () => { + const React = await import('react'); + return { + // Records the `data` prop, which is the seam the defect is visible at: + // ObjectView passes `data={data}` down, suppressing the child's own fetch. + SchemaRenderer: ({ schema, data }: any) => { + if (Array.isArray(data) && data.length > 0) { + const g = (globalThis as any).__objectViewDeliveries as any[][] | undefined; + if (g && g[g.length - 1] !== data) g.push(data); + } + return
{schema?.type}
; + }, + SchemaRendererContext: React.createContext(null), + subscribeDataChanges: () => () => {}, + notifyDataChanged: () => {}, + }; +}); +vi.mock('@object-ui/plugin-grid', () => ({ ObjectGrid: () =>
})); +vi.mock('@object-ui/plugin-form', () => ({ ObjectForm: () =>
})); + +/** + * One field of every expandable type, plus non-expandable neighbours. The + * expectation below is DERIVED from this map rather than written out, so the + * assertion is on `$expand`'s contents against the schema — a field added here + * with an expandable type must show up in the query or the test fails. + */ +const TASK_FIELDS: Record = { + name: { type: 'text', label: 'Name' }, + amount: { type: 'currency', label: 'Amount' }, + due_date: { type: 'date', label: 'Due' }, + owner: { type: 'user', label: 'Owner' }, + account: { type: 'lookup', label: 'Account', reference_to: 'account' }, + parent_task: { type: 'tree', label: 'Parent', reference_to: 'task' }, + line_item: { type: 'master_detail', label: 'Line item', reference_to: 'line_item' }, +}; + +const TASK_SCHEMA = { name: 'task', label: 'Task', fields: TASK_FIELDS }; + +/** The four expandable types, read from core's own set — not a copy of it. */ +const EXPECTED_EXPAND = Object.entries(TASK_FIELDS) + .filter(([, def]) => EXPANDABLE_FIELD_TYPES.has(def.type)) + .map(([fieldName]) => fieldName); + +const NON_EXPANDABLE = Object.entries(TASK_FIELDS) + .filter(([, def]) => !EXPANDABLE_FIELD_TYPES.has(def.type)) + .map(([fieldName]) => fieldName); + +const ROWS = [{ id: 't1', name: 'Ship it', account: 'acc-1' }]; + +/** + * `getObjectSchema` deliberately resolves a tick LATER than a bare + * `mockResolvedValue` would, so a view that queries before the schema settles + * is caught rather than 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'); + // A FRESH array per response, as the wire produces, tagged with the query + // that produced it. Returning one shared array would make `setData` a + // reference-equal no-op and hide every extra delivery. + const tag = Array.isArray(params?.$expand) && params.$expand.length > 0 ? 'expanded' : 'raw'; + return { data: ROWS.map((r) => ({ ...r, _from: tag })), total: ROWS.length }; + }), + findOne: vi.fn(), + create: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + }; + if (getObjectSchema) { + adapter.getObjectSchema = vi.fn(async (objectName: string) => { + order.push('schema:issued'); + try { + return await getObjectSchema(); + } finally { + order.push('schema:settled'); + void objectName; + } + }); + } + return adapter; +} + +const resolvesSchema = () => + makeAdapter(async () => { + await new Promise((r) => setTimeout(r, 10)); + return TASK_SCHEMA; + }); + +/** + * Render the REAL `ObjectView` on a NON-grid path. `defaultViewType` is + * anything but `grid`: the grid path delegates its fetch to `ObjectGrid` and + * never reaches the effect under test. + */ +function renderView(adapter: Record, extra: Partial = {}) { + return render( + , + ); +} + +const paramsOf = (adapter: Record) => + adapter.find.mock.calls.map((c: any[]) => c[1] ?? {}); +const expandedCalls = (adapter: Record) => + paramsOf(adapter).filter((p: any) => Array.isArray(p.$expand) && p.$expand.length > 0); +const unexpandedCalls = (adapter: Record) => + paramsOf(adapter).filter((p: any) => !Array.isArray(p.$expand) || p.$expand.length === 0); + +beforeEach(() => { + vi.clearAllMocks(); + deliveries.length = 0; + (globalThis as any).__objectViewDeliveries = deliveries; +}); + +describe('ObjectView gates its non-grid query on the object schema (objectui#6419)', () => { + it('issues ONE query, and it carries the object’s `$expand`', async () => { + const adapter = resolvesSchema(); + renderView(adapter); + + // Control — this `waitFor` targets the EXPANDED call, not "any call" and + // not "the mock exists". If the gate ever stops opening, no such call is + // recorded, this times out, and the file goes red: "0 queries" can never + // read as success here. + await waitFor(() => expect(expandedCalls(adapter)).toHaveLength(1)); + + // RED before the fix: this read `[{ $top: 100 }]` — the query that never + // carried an expansion at all. + expect(unexpandedCalls(adapter)).toEqual([]); + expect(adapter.find).toHaveBeenCalledTimes(1); + expect(adapter.find.mock.calls[0][0]).toBe('task'); + }); + + it('sends exactly the schema’s expandable fields — asserted against the fixture, not merely present', async () => { + const adapter = resolvesSchema(); + renderView(adapter); + + await waitFor(() => expect(expandedCalls(adapter)).toHaveLength(1)); + const $expand: string[] = expandedCalls(adapter)[0].$expand; + + // Contents, both directions. `EXPECTED_EXPAND` is derived from the fixture + // through core's own `EXPANDABLE_FIELD_TYPES`, so this covers all four + // relation types (`user`, `lookup`, `tree`, `master_detail`) and fails if + // one stops being expanded. + expect(EXPECTED_EXPAND.length).toBe(4); + expect([...$expand].sort()).toEqual([...EXPECTED_EXPAND].sort()); + for (const plain of NON_EXPANDABLE) { + expect($expand).not.toContain(plain); + } + }); + + it('issues that query only AFTER the schema read settles', async () => { + const adapter = resolvesSchema(); + renderView(adapter); + + await waitFor(() => expect(adapter.find).toHaveBeenCalled()); + // Ordering, not just counting: a fix that merely deduplicated a second + // query would satisfy the count above while still querying too early. + expect(adapter.order).toEqual(['schema:issued', 'schema:settled', 'find']); + }); + + it('hands the child view ONE delivery, and it is the expanded one', async () => { + // The measured user-visible cost of an extra re-run on THIS effect: with + // `objectSchema` in the dependency list the child received `raw` then + // `expanded` — a two-step paint in which every relation field is blank or a + // raw id for ~40ms. This pins the single-delivery outcome, and doubles as + // the control that rows really reach the child rather than the query + // vanishing. + const adapter = resolvesSchema(); + renderView(adapter); + + await waitFor(() => expect(deliveries.length).toBeGreaterThan(0)); + expect(deliveries).toHaveLength(1); + expect(deliveries[0][0]._from).toBe('expanded'); + }); + + it('still queries — and paints — when the adapter exposes NO `getObjectSchema`', async () => { + // The gate is on the read having settled, not on a truthy schema. An + // adapter without the method settles with nothing to report, and the view + // must fall through to an unexpanded query rather than wait forever. + const adapter = makeAdapter(); + renderView(adapter); + + await waitFor(() => expect(deliveries.length).toBeGreaterThan(0)); + expect(adapter.find).toHaveBeenCalledTimes(1); + // Nothing declared any field, so there is no expand set to derive. + expect(unexpandedCalls(adapter)).toHaveLength(1); + expect(deliveries[0][0]._from).toBe('raw'); + }); + + it('still queries — and paints — when the schema read REJECTS', async () => { + const adapter = makeAdapter(async () => { + await new Promise((r) => setTimeout(r, 10)); + throw new Error('metadata endpoint down'); + }); + const err = vi.spyOn(console, 'error').mockImplementation(() => {}); + try { + renderView(adapter); + + await waitFor(() => expect(deliveries.length).toBeGreaterThan(0)); + expect(adapter.find).toHaveBeenCalledTimes(1); + expect(unexpandedCalls(adapter)).toHaveLength(1); + expect(adapter.order).toEqual(['schema:issued', 'schema:settled', 'find']); + } finally { + err.mockRestore(); + } + }); + + it('a grid view still delegates its fetch — the gate did not start one', async () => { + // Control in the other direction: the gate must not have turned the grid + // path (which owns its own fetching, via ObjectGrid) into a fetching one. + const adapter = resolvesSchema(); + renderView(adapter, { defaultViewType: 'grid' }); + + await waitFor(() => expect(adapter.getObjectSchema).toHaveBeenCalled()); + expect(adapter.find).not.toHaveBeenCalled(); + }); + + it('re-gates when the object changes, so no query carries the previous object’s expand set', async () => { + // The resolution is KEYED by object name and compared during render, so + // switching objects closes the gate in the same commit that changes it. + const adapter = resolvesSchema(); + const { rerender } = renderView(adapter); + await waitFor(() => expect(expandedCalls(adapter)).toHaveLength(1)); + + adapter.getObjectSchema.mockImplementation(async () => { + adapter.order.push('schema:issued'); + await new Promise((r) => setTimeout(r, 10)); + adapter.order.push('schema:settled'); + return { name: 'note', label: 'Note', fields: { body: { type: 'text', label: 'Body' } } }; + }); + + rerender( + , + ); + + await waitFor(() => expect(adapter.find.mock.calls.length).toBeGreaterThan(1)); + const noteCalls = adapter.find.mock.calls.filter((c: any[]) => c[0] === 'note'); + expect(noteCalls).toHaveLength(1); + // `note` declares no expandable field. A stale resolution would have sent + // `task`'s expand set against `note`. + const noteParams = noteCalls[0][1] ?? {}; + expect(noteParams.$expand === undefined || noteParams.$expand.length === 0).toBe(true); + }); +});