diff --git a/.changeset/7895-timeline-settled-schema-gate.md b/.changeset/7895-timeline-settled-schema-gate.md new file mode 100644 index 0000000000..9250f5c4b5 --- /dev/null +++ b/.changeset/7895-timeline-settled-schema-gate.md @@ -0,0 +1,47 @@ +--- +'@object-ui/plugin-timeline': patch +--- + +`ObjectTimeline` waits for the object definition instead of querying twice +(objectui#7895). + +It was the last member of the set objectui#6482 converged on the shared +settled-schema gate — `ObjectKanban`, `ObjectView`, `ObjectCalendar` and +`ObjectTree` were named there, `ObjectGantt` was ask 2 of objectui#7225 — and +nothing marked it a deliberate exclusion. It still held the object definition in +a local `useState` fed by its own metadata effect, and listed that definition in +the record-fetch effect's dependency array. + +**User-visible.** Every object-bound timeline load issued **two** `find` calls +instead of one: the first before the definition landed, with `buildExpandFields` +seeing no fields and therefore carrying no `$expand` at all, and a second one +after. Whenever the metadata read is the slower of the two — the common case on +a cold metadata cache — the second call is not merely a wasted round trip but a +**three-step paint**: raw foreign-key ids, back to the loading skeleton (the +effect's re-run calls `setLoading(true)` and `loading` is an early return), then +the expanded rows. After this change the timeline paints once, from a query that +already carries its expansion. + +Measured on the component with an instrumented renderer, one mount per hold, +`getObjectSchema` held 0/1/2/3/4/5/6/7/8/9/10/15/25/50/100 ms, with +`ObjectCalendar` and `ObjectGantt` as positive controls in the same run: before, +2 `find` calls with expand sets `[null, ['owner']]`, 1 paint at the readiness +predicate and 3 late writes after it at every hold from +3 ms up; after, 1 `find` +carrying `['owner']`, 1 paint, 0 late writes, and a first-paint time that tracks +the hold (8 ms at +3, 30 ms at +25, 105 ms at +100) where before it was a flat +3-7 ms at every hold. Both controls read 1 paint / 0 late writes before and +after. + +The resolution half is now `useSettledSchema` from `@object-ui/react`, which +settles on **every** exit — no source, no `getObjectSchema`, no object name, and +a read that threw alike. That is what makes the gate safe: the replaced effect +returned without settling on all four, which cost nothing while nothing waited on +it and would have held a gated query open forever. Pinned by +`ObjectTimeline.fetchGate-7895.test.tsx`, including a timeline whose adapter +exposes no `getObjectSchema` and one whose definition read rejects — both still +query, unexpanded. + +Unlike the two sibling conversions, the metadata read is **not** disabled for a +timeline whose items were authored inline: this component also reads the +definition's fields for option colours and field labels on that path, where no +record query is issued at all. diff --git a/packages/plugin-timeline/src/ObjectTimeline.fetchGate-7895.test.tsx b/packages/plugin-timeline/src/ObjectTimeline.fetchGate-7895.test.tsx new file mode 100644 index 0000000000..e6a0c97eb3 --- /dev/null +++ b/packages/plugin-timeline/src/ObjectTimeline.fetchGate-7895.test.tsx @@ -0,0 +1,232 @@ +/** + * 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#7895 — the timeline's DUPLICATE query is gated. It was the last + * member of the set objectui#6482 converged (`ObjectKanban`, `ObjectView`, + * `ObjectCalendar`, `ObjectTree`; `ObjectGantt` at objectui#7225 ask 2) still + * carrying the pre-gate shape, and nothing marked it a deliberate exclusion. + * + * Before this, `ObjectTimeline` held the object definition in a local + * `useState` and listed it in the record-fetch effect's dependency array, so a + * mount issued TWO `find` calls: one before the definition settled, with + * `buildExpandFields` seeing no fields and therefore NO `$expand` at all, and + * one after. + * + * ⛔ A GREEN RUN WITH NO HOLD IS ZERO EVIDENCE HERE. This container settles the + * metadata read and the record read together by default, so the defect is + * invisible unperturbed — that is the reading objectui#7466 paid for. The lever + * is the METADATA fetch, so `getObjectSchema` is HELD and the writes into an + * instrumented renderer are counted per SINGLE mount. Measured on this + * component with that instrument, `ObjectCalendar` and `ObjectGantt` as + * positive controls in the same vitest run, holds 0/1/2/3/4/5/6/7/8/9/10/15/ + * 25/50/100 ms: + * + * component hold paints late writes first paint find calls + * ObjectTimeline before, +3ms and up 4 3 3-7ms 2 + * ObjectTimeline before, +0 / +1ms 1 0 5ms 2 + * ObjectTimeline after, every hold 1 0 tracks the hold 1 + * ObjectCalendar before and after 1 0 tracks the hold 1 + * ObjectGantt before and after 1 0 tracks the hold 1 + * + * ⭐ "First paint tracks the hold" is the signature that the gate is LIVE, and + * it is what the last case below pins — not as a wall-clock threshold, which + * would be a timing test, but as an ORDER: the definition resolves, then the + * query goes out, then the renderer is written to, exactly once. Before the + * gate that order was find, paint, definition, find, paint. Measured after: + * 8ms at a +3ms hold, 19ms at +10, 30ms at +25, 105ms at +100, against a flat + * 3-7ms at every hold before it. + * + * ⚠️ The gate is only safe because the resolution now SETTLES ON EVERY EXIT + * (objectui#7232, via the shared `useSettledSchema`): the replaced effect + * returned without settling on `!dataSource`, on a missing `getObjectSchema`, + * on an absent object name and in its `catch`. That cost nothing while nothing + * waited on it and would hold a gated query open FOREVER. The two middle cases + * below are that trap, pinned: a timeline that never loads is the failure this + * file exists to make impossible. + * + * ⛔ GATING IS NOT EXPANDING. objectui#7429 is the separate, non-overlapping + * concern of whether the `$expand` set this query carries is FLS-gated; this + * file asserts only HOW MANY queries go out and whether the first one carries + * its expansion. + */ + +import React from 'react'; +import { render, screen, waitFor, act } from '@testing-library/react'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { ObjectTimeline } from './ObjectTimeline'; + +/** Every write into the renderer, in order — the paint counter. */ +let paints: number[] = []; +/** Definition resolve / query / paint, interleaved in the order they happen. */ +let sequence: string[] = []; + +vi.mock('./renderer', () => ({ + TimelineRenderer: ({ schema }: any) => { + const items = schema.items ?? []; + paints.push(items.length); + sequence.push('paint'); + return
; + }, +})); + +/** A definition with a lookup, so a gated query has a real `$expand` to carry. */ +const OBJECT_SCHEMA = { + name: 'duly_task', + fields: { + id: { name: 'id', type: 'text' }, + subject: { name: 'subject', type: 'text' }, + starts_at: { name: 'starts_at', type: 'datetime' }, + owner: { name: 'owner', type: 'lookup', reference_to: 'user' }, + }, +}; + +const ROWS = [{ id: '1', subject: 'Ship it', owner: 'u1', starts_at: '2026-01-01T09:00:00Z' }]; + +const schema: any = { + type: 'timeline', + objectName: 'duly_task', + titleField: 'subject', + startDateField: 'starts_at', +}; + +function makeAdapter(getObjectSchema?: any) { + return { + find: vi.fn(async () => { + sequence.push('find'); + return { data: ROWS, total: ROWS.length }; + }), + findOne: vi.fn(), + create: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + ...(getObjectSchema === undefined ? {} : { getObjectSchema }), + } as any; +} + +/** The expand sets of every issued query, in order. */ +function expandSets(adapter: any): Array { + return adapter.find.mock.calls.map(([, params]: [string, any]) => params?.$expand ?? null); +} + +beforeEach(() => { + paints = []; + sequence = []; +}); + +describe('objectui#7895 — the timeline waits for the object definition instead of querying twice', () => { + it('issues ONE query per load, and it already carries the expansion', async () => { + const adapter = makeAdapter(vi.fn(async () => OBJECT_SCHEMA)); + + render(); + + await waitFor(() => expect(screen.getByTestId('timeline-renderer')).toBeTruthy()); + await waitFor(() => expect(adapter.find).toHaveBeenCalledTimes(1)); + + // The old regime's signature was `[null, ['owner']]`. One expanded call. + expect(expandSets(adapter)).toEqual([['owner']]); + expect(adapter.getObjectSchema).toHaveBeenCalledTimes(1); + }); + + it('never issues an UNEXPANDED query for an object that declares a lookup', async () => { + const adapter = makeAdapter(vi.fn(async () => OBJECT_SCHEMA)); + + render(); + + await waitFor(() => expect(adapter.find).toHaveBeenCalled()); + // The discarded round trip — and the raw-id frame it painted — is the thing + // gating removes. Not "fewer" unexpanded calls: none. + for (const params of adapter.find.mock.calls.map(([, p]: [string, any]) => p)) { + expect(params.$expand).toEqual(['owner']); + } + }); + + it('still queries when the adapter exposes NO `getObjectSchema` — the gate is on SETTLED, not on truthy', async () => { + // objectui#7232's trap: an exit that returns without settling would hold + // this query open forever, and the timeline would never load. + const adapter = makeAdapter(undefined); + + render(); + + await waitFor(() => expect(screen.getByTestId('timeline-renderer')).toBeTruthy()); + expect(adapter.find).toHaveBeenCalledTimes(1); + // Nothing to derive an expand set from, so the query is unexpanded — the + // same query this case produced before the gate. + expect(expandSets(adapter)).toEqual([null]); + }); + + it('still queries when the definition read REJECTS', async () => { + const adapter = makeAdapter( + vi.fn(async () => { + throw new Error('metadata endpoint down'); + }), + ); + const error = vi.spyOn(console, 'error').mockImplementation(() => {}); + try { + render(); + + await waitFor(() => expect(screen.getByTestId('timeline-renderer')).toBeTruthy()); + expect(adapter.find).toHaveBeenCalledTimes(1); + expect(expandSets(adapter)).toEqual([null]); + expect(String(error.mock.calls[0]?.[0] ?? '')).toContain('[useSettledSchema]'); + } finally { + error.mockRestore(); + } + }); + + it('an AUTHORED `items` timeline paints without issuing any record query', async () => { + const authored: any = { + ...schema, + items: [{ title: 'Ship it', time: '2026-01-01T09:00:00Z' }], + }; + const adapter = makeAdapter(vi.fn(async () => OBJECT_SCHEMA)); + + render(); + + await waitFor(() => + expect(screen.getByTestId('timeline-renderer').getAttribute('data-item-count')).toBe('1'), + ); + expect(adapter.find).not.toHaveBeenCalled(); + // ⛔ The metadata read is NOT disabled on this path, unlike the sibling + // components' `hasInlineData ? undefined : dataSource`. `effectiveItems` + // reads `objectDef.fields` for option colours and field labels even when + // the items were authored, so taking the read away would be a second, + // unasked-for change riding on a fetch-sequencing fix. + expect(adapter.getObjectSchema).toHaveBeenCalledTimes(1); + }); + + it('under a HELD definition read: ONE paint, and it arrives AFTER the definition', async () => { + // ⛔ The hold is the whole instrument. Without it this container settles + // both reads together and the case passes on the unfixed component too. + const HOLD_MS = 25; + const adapter = makeAdapter( + vi.fn(async () => { + await new Promise((r) => setTimeout(r, HOLD_MS)); + sequence.push('definition'); + return OBJECT_SCHEMA; + }), + ); + + render(); + + await waitFor(() => expect(screen.getByTestId('timeline-renderer')).toBeTruthy()); + // Leave the render MOUNTED and let anything still in flight land — the late + // writes are only observable on a render that is still there to receive + // them. Measured before the gate at this hold: 3 of them. + await act(async () => { + await new Promise((r) => setTimeout(r, HOLD_MS + 150)); + }); + + expect(paints).toEqual([ROWS.length]); + // The ORDER is the signature, not a wall-clock threshold. Before the gate: + // find, paint, definition, find, paint (and two more writes after). + expect(sequence).toEqual(['definition', 'find', 'paint']); + expect(adapter.find).toHaveBeenCalledTimes(1); + expect(expandSets(adapter)).toEqual([['owner']]); + }); +}); diff --git a/packages/plugin-timeline/src/ObjectTimeline.tsx b/packages/plugin-timeline/src/ObjectTimeline.tsx index 1b4575deff..6eae2ef843 100644 --- a/packages/plugin-timeline/src/ObjectTimeline.tsx +++ b/packages/plugin-timeline/src/ObjectTimeline.tsx @@ -8,7 +8,7 @@ import React, { useEffect, useState, useCallback, useMemo } from 'react'; import type { DataSource, TimelineSchema, ListViewTimelineConfig } from '@object-ui/types'; -import { useDataScope, useNavigationOverlay, useSafeFieldLabel } from '@object-ui/react'; +import { useDataScope, useNavigationOverlay, useSafeFieldLabel, useSettledSchema } from '@object-ui/react'; import { NavigationOverlay } from '@object-ui/components'; import { extractRecords, buildExpandFields, convertSortToQueryParams, createFieldColorResolver } from '@object-ui/core'; import { usePullToRefresh } from '@object-ui/mobile'; @@ -173,7 +173,6 @@ export const ObjectTimeline: React.FC = ({ }); const [error, setError] = useState(null); const [refreshKey, setRefreshKey] = useState(0); - const [objectDef, setObjectDef] = useState(null); // Resolve nested TimelineConfig (spec-compliant) const timelineConfig = schema.timeline; @@ -187,21 +186,50 @@ export const ObjectTimeline: React.FC = ({ const boundData = useDataScope(schema.bind); - // Fetch object definition for metadata - useEffect(() => { - let isMounted = true; - const fetchMeta = async () => { - if (!dataSource || typeof dataSource.getObjectSchema !== 'function' || !schema.objectName) return; - try { - const def = await dataSource.getObjectSchema(schema.objectName); - if (isMounted) setObjectDef(def); - } catch (e) { - console.warn('Failed to fetch object def for ObjectTimeline', e); - } - }; - fetchMeta(); - return () => { isMounted = false; }; - }, [schema.objectName, dataSource]); + /** + * The object definition, and whether the read for THIS object has SETTLED — + * one piece of state, through the shared hook (objectui#7895). + * + * `ObjectTimeline` was the last member of the converged set still carrying + * the pre-gate shape: a local `useState` fed by its own metadata effect, + * with `objectDef` listed in the record-fetch effect's dependency array + * below. That shape issues the record query TWICE per mount — once before + * the definition lands, with `buildExpandFields` seeing no fields and so no + * `$expand` at all, and once after — and whenever the metadata read is the + * slower of the two the user sees the three-step paint `ObjectGantt`'s own + * conversion names: raw foreign-key ids, back to the loading skeleton (the + * re-run calls `setLoading(true)` and `loading` is an early return below), + * then the expanded rows. Measured on this component before the change, + * instrumented renderer, one mount per hold: 2 `find` calls with expand + * sets `[null, ['owner']]`, 1 paint at the readiness predicate and 3 late + * writes after it, at every hold from +3ms up. + * + * ⚠️ The gate below is only safe because this resolution SETTLES ON EVERY + * EXIT (objectui#7232) — no source, no `getObjectSchema`, no object name, + * and a read that threw alike. The hand-written effect it replaces returned + * WITHOUT settling on all four, which cost nothing while nothing waited on + * it and would hold a gated query open forever. + * + * ⛔ `dataSource` is passed unconditionally, NOT `hasInlineData ? undefined + * : dataSource` the way `ObjectCalendar` and `ObjectGantt` pass it. Those + * two read metadata only to expand a record query, so an inline data set has + * nothing to wait for. This component also reads `objectDef.fields` in + * `effectiveItems` below — option colours and field labels — on the AUTHORED + * items path, where no query is issued at all. Their recipe would stop that + * read happening; the conversion is a fetch-sequencing change and must not + * take a metadata read away from a path that still uses it. + * + * The key is `schema.objectName`, which is the object the record query + * itself names (`dataSource.find(schema.objectName, …)` below) and the one + * the replaced effect read. ⛔ Not `resolveRecordSourceObjectName`: this + * component has no resolved `data` block to read a second name from, and + * gating on a key the query does not use is exactly the stale-key mismatch + * `useSettledSchema`'s render-time comparison exists to make unrepresentable. + */ + const { ready: objectDefReady, def: objectDef } = useSettledSchema( + schema.objectName ?? '', + dataSource, + ); // Content keys, not identities. `filter` / `sort` are fetch inputs from here on // (objectstack#7137), and an inline array on a schema node is a NEW object every @@ -245,13 +273,28 @@ export const ObjectTimeline: React.FC = ({ }; if (schema.objectName && !boundData && !schema.items && !(props as any).data) { + // ⭐ objectui#7895 — the object definition GATES this query; it does not + // refine it afterwards. `objectDef` stays in the dependency list below + // and the two are ONE mechanism, not two: the dependency is what makes + // this effect re-run when the definition lands, and this line is what + // stops the first run from spending a query before it has. Removing + // either half alone restores the double fetch — the reverse + // verification `ObjectGantt.fetchGate-7225.test.tsx` recorded on the + // sibling, re-measured here. + // + // Scoped to the branch that actually issues the query. The `else` below + // has authored or bound items and never queries, so gating it would + // hold nothing useful — and this component still reads the definition + // on that path (option colours in `effectiveItems`), which is why the + // resolution above is not disabled for it. + if (!objectDefReady) return; fetchData(); } else { // Have inline / bound items — won't fetch; clear loading. setLoading(false); } // eslint-disable-next-line react-hooks/exhaustive-deps -- `schema.filter`/`schema.sort` are tracked by CONTENT (filterKey/sortKey) on purpose; see above - }, [schema.objectName, dataSource, boundData, schema.items, (props as any).data, refreshKey, objectDef, filterKey, sortKey, schema.limit]); + }, [schema.objectName, dataSource, boundData, schema.items, (props as any).data, refreshKey, objectDefReady, objectDef, filterKey, sortKey, schema.limit]); const rawData = (props as any).data || boundData || fetchedData; const { t } = useTimelineTranslation();