diff --git a/.changeset/6481-objecttree-keyed-schema-latch.md b/.changeset/6481-objecttree-keyed-schema-latch.md new file mode 100644 index 0000000000..9b0ede668f --- /dev/null +++ b/.changeset/6481-objecttree-keyed-schema-latch.md @@ -0,0 +1,32 @@ +--- +'@object-ui/plugin-tree': patch +--- + +`ObjectTree` no longer queries a switched-to object with the previous object's +`$expand` set (objectui#6481). + +The schema-settled gate was two separate pieces of state — the definition +(`objectSchema`) and a bare `schemaSettled` boolean that was set `true` in a +`finally` and never reset. Two independent values cannot express "settled, but +for a DIFFERENT object", so when the host swapped the bound object both effects +re-ran while the latch still read `true` from the previous object's settle and +the definition still held the previous object's fields. The tree issued + + find(newObject, { $filter: …, $expand: [ …previous object's relation fields… ] }) + +— rejected or silently ignored depending on the adapter, plus the transient it +painted — before a correct second query followed. + +`ObjectTree` now adopts `useSettledSchema` from `@object-ui/react` (the shared +resolution hook ruled in objectui#6482), replacing BOTH pieces of state with the +hook's single `{ key, def } | null`. Readiness is derived during render by +comparing the settled key against the currently bound object, so the gate closes +in the same commit that changes the object rather than one commit later — the +stale-key window is not merely fixed but unrepresentable. + +Behaviour that deliberately does NOT change: the gate stays inside the +object-provider branch of the record effect (the inline/static branches issue no +metadata read and must not wait on one), and every exit still settles — no +`getObjectSchema`, no object name, or a rejected read each settle with no +definition, so a tree whose adapter serves no schema still queries instead of +waiting forever. diff --git a/packages/plugin-tree/src/ObjectTree.settledSchemaKeying-6481.test.tsx b/packages/plugin-tree/src/ObjectTree.settledSchemaKeying-6481.test.tsx new file mode 100644 index 0000000000..19869e620f --- /dev/null +++ b/packages/plugin-tree/src/ObjectTree.settledSchemaKeying-6481.test.tsx @@ -0,0 +1,213 @@ +/** + * 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. + */ + +/** + * ObjectTree — the settled-schema gate must be KEYED to the bound object + * (objectui#6481), and must still settle on EVERY exit (objectui#6014). + * + * These two properties pull in opposite directions and that is the whole + * point of the file: one of them says "do not query until the schema is in", + * the other says "never wait for a schema that is not coming". A latch that + * satisfies only the first hangs forever on an adapter with no + * `getObjectSchema`; a latch that satisfies only the second — the bare + * boolean this card replaces — queries the NEW object with the OLD object's + * `$expand`. + * + * Measured on THIS component before the fix, not transferred from + * objectui#6453 / #6419: `ObjectTree`'s record effect has its own dependency + * set (`dataConfig`, `dataSource`, `schema.filter`, the schema resolution, + * `(rest as any).data`), and its gate sits INSIDE the object-provider branch + * rather than at the top of the effect. + */ + +import React from 'react'; +import { render, waitFor, cleanup, act } from '@testing-library/react'; +import { describe, it, expect, afterEach, beforeEach, vi } from 'vitest'; +import { ObjectTree } from './ObjectTree'; + +afterEach(cleanup); + +interface FindCall { + object: string; + options: any; +} + +/** A promise a test resolves by hand, so "not settled yet" is a real state. */ +function deferred() { + let resolve!: (value: T) => void; + let reject!: (reason?: unknown) => void; + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; + }); + return { promise, resolve, reject }; +} + +/** + * Two self-referencing objects whose EXPANDABLE field sets are disjoint apart + * from the parent pointer. `head` exists only on `business_unit` and `region` + * only on `territory`, so an `$expand` naming the wrong one is unambiguous + * evidence of which object's schema built it. + */ +const SCHEMAS: Record = { + business_unit: { + name: 'business_unit', + fields: { + name: { type: 'text' }, + parent_id: { type: 'tree', reference: 'business_unit' }, + head: { type: 'lookup', reference: 'users' }, + }, + }, + territory: { + name: 'territory', + fields: { + name: { type: 'text' }, + parent_id: { type: 'tree', reference: 'territory' }, + region: { type: 'lookup', reference: 'regions' }, + }, + }, +}; + +function treeSchema(objectName: string) { + return { + type: 'object-tree', + objectName, + parentField: 'parent_id', + labelField: 'name', + fields: ['name'], + } as any; +} + +let errorSpy: ReturnType; +beforeEach(() => { + // The rejected-read path logs; keep the suite output honest without + // swallowing a real unexpected error (asserted on in that test). + errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); +}); +afterEach(() => { + errorSpy.mockRestore(); +}); + +describe('ObjectTree settled-schema gate is keyed to the bound object (objectui#6481)', () => { + it('never queries a switched-to object with the previous object\'s $expand', async () => { + const findCalls: FindCall[] = []; + const gates: Record>> = { + business_unit: deferred(), + territory: deferred(), + }; + const schemaRequests: string[] = []; + + const dataSource: any = { + getObjectSchema: (objectName: string) => { + schemaRequests.push(objectName); + return gates[objectName].promise; + }, + find: async (object: string, options: any) => { + findCalls.push({ object, options }); + return []; + }, + }; + + const { rerender } = render( + , + ); + + // Leg 1 — the first object settles and queries with ITS OWN expand set. + await act(async () => { + gates.business_unit.resolve(SCHEMAS.business_unit); + await gates.business_unit.promise; + }); + await waitFor(() => expect(findCalls.length).toBe(1)); + expect(findCalls[0].object).toBe('business_unit'); + expect([...findCalls[0].options.$expand].sort()).toEqual(['head', 'parent_id']); + + // Leg 2 — the host swaps the bound object. `territory`'s schema has NOT + // settled yet, so nothing is entitled to build an `$expand` for it. + await act(async () => { + rerender(); + }); + + // The defect, stated as the query it emits: with an unkeyed boolean latch + // the gate reads "settled" from `business_unit`'s settle and the previous + // resolution is still in state, so this fires + // find('territory', { $expand: ['parent_id', 'head'] }) + // — `head` is not a field `territory` declares. + const territoryCalls = findCalls.filter((c) => c.object === 'territory'); + const strayExpands = territoryCalls + .map((c) => (c.options?.$expand ?? []) as string[]) + .filter((expand) => expand.some((f) => !(f in SCHEMAS.territory.fields))); + expect(strayExpands).toEqual([]); + + // Stated the second way: no query at all may go out for an object whose + // schema has not settled. (The two assertions fail together on the bare + // boolean; keeping both says WHICH property broke if they ever diverge.) + expect(territoryCalls).toEqual([]); + + // Leg 3 — once the new schema lands, the correct query follows. The gate + // must close, not deadlock. + await act(async () => { + gates.territory.resolve(SCHEMAS.territory); + await gates.territory.promise; + }); + await waitFor(() => { + const calls = findCalls.filter((c) => c.object === 'territory'); + expect(calls.length).toBeGreaterThan(0); + }); + const settled = findCalls.filter((c) => c.object === 'territory'); + expect([...settled[settled.length - 1].options.$expand].sort()).toEqual([ + 'parent_id', + 'region', + ]); + // Exactly one query per object — the switch must not re-query the object + // it left, and must not double-query the one it arrived at. + expect(findCalls.map((c) => c.object)).toEqual(['business_unit', 'territory']); + expect(schemaRequests).toEqual(['business_unit', 'territory']); + }); + + it('still queries when the adapter exposes no getObjectSchema (every exit settles)', async () => { + const findCalls: FindCall[] = []; + // No `getObjectSchema` at all: the resolution has no source to read from. + // It must SETTLE with no definition rather than stay pending, or the gated + // record query waits forever — objectui#6014's `finally`, which this card + // must not trade away. + const dataSource: any = { + find: async (object: string, options: any) => { + findCalls.push({ object, options }); + return []; + }, + }; + + render(); + + await waitFor(() => expect(findCalls.length).toBe(1)); + expect(findCalls[0].object).toBe('business_unit'); + // No schema means no expand set to derive — the key must be absent, not + // an empty array. + expect('$expand' in findCalls[0].options).toBe(false); + }); + + it('still queries when the schema read rejects (every exit settles)', async () => { + const findCalls: FindCall[] = []; + const dataSource: any = { + getObjectSchema: async () => { + throw new Error('metadata endpoint down'); + }, + find: async (object: string, options: any) => { + findCalls.push({ object, options }); + return []; + }, + }; + + render(); + + await waitFor(() => expect(findCalls.length).toBe(1)); + expect(findCalls[0].object).toBe('business_unit'); + expect('$expand' in findCalls[0].options).toBe(false); + expect(errorSpy).toHaveBeenCalled(); + }); +}); diff --git a/packages/plugin-tree/src/ObjectTree.tsx b/packages/plugin-tree/src/ObjectTree.tsx index 9b1d1e3f6d..941e7ae63a 100644 --- a/packages/plugin-tree/src/ObjectTree.tsx +++ b/packages/plugin-tree/src/ObjectTree.tsx @@ -21,7 +21,7 @@ import React, { useEffect, useMemo, useState } from 'react'; import type { DataSource, ViewData } from '@object-ui/types'; -import { useNavigationOverlay, useSafeFieldLabel } from '@object-ui/react'; +import { useNavigationOverlay, useSafeFieldLabel, useSettledSchema } from '@object-ui/react'; import { NavigationOverlay, cn } from '@object-ui/components'; import { createSafeTranslation } from '@object-ui/i18n'; import { @@ -338,65 +338,65 @@ export const ObjectTree: React.FC = ({ const [records, setRecords] = useState([]); const [loading, setLoading] = useState(true); const [error, setError] = useState(null); - const [objectSchema, setObjectSchema] = useState(null); + const dataConfig = useMemo(() => getDataConfig(schema), [schema]); + /** - * Whether the object-schema fetch below has finished — settled, not - * successful: a dataSource that cannot serve a schema, an object with no - * name, and a rejected fetch all count, so the record fetch can never be - * blocked forever by a schema that is never going to arrive. - * - * A one-way latch on purpose. Re-arming it on every run of that effect would - * mean a `setState` in the effect body, and this component's dependency list - * includes `dataConfig` — a `useMemo` over the `schema` PROP object — so a - * host that rebuilds its schema each render would turn a benign re-run into a - * render loop. Not re-arming costs at most one fetch against a stale schema - * when `objectName` changes mid-life, which is exactly what happened on every - * fetch before this. + * The object THIS render is bound to, as a plain string — so the resolution + * below re-keys on the OBJECT rather than on `dataConfig`, a `useMemo` over + * the `schema` PROP object whose identity a host that rebuilds its schema + * each render changes without changing which object is bound. */ - const [schemaSettled, setSchemaSettled] = useState(false); + const schemaKey = + (dataConfig?.provider === 'object' ? dataConfig.object : schema.objectName) ?? ''; - const dataConfig = useMemo(() => getDataConfig(schema), [schema]); - - // Fetch the object schema whenever the dataSource can serve one. - // - // It feeds FOUR things: parent-field auto-detection, column labels, the - // `$expand` list built below, and (objectui#6014) the per-field definitions - // the cell formatter reads to resolve select options and reference values. - // - // This used to be gated on "the host passed no inline data", which read as a - // cheap skip but disagreed with the record-fetch effect below: THAT branch - // prefers a live object dataSource over any inline `data`, so on the one - // mount shape `ListView` actually uses (objectName + dataSource + its own - // pre-fetched `data`) the tree ran its own query with - // `buildExpandFields(undefined)` → `[]` → no `$expand` at all, and had no - // field definitions to format cells with. That is the whole of objectui#6014: - // lookups rendered as bare ids and selects as raw stored values, on the very - // page whose flat-table tab rendered both correctly. The guard inside - // `fetchSchema` already no-ops without a dataSource, so dropping the gate - // costs nothing on the pure inline/static path. - useEffect(() => { - let cancelled = false; - const fetchSchema = async () => { - try { - if (!dataSource || typeof dataSource.getObjectSchema !== 'function') return; - const objectName = - dataConfig?.provider === 'object' ? dataConfig.object : schema.objectName; - if (!objectName) return; - const result = await dataSource.getObjectSchema(objectName); - if (!cancelled) setObjectSchema(result); - } catch (err) { - console.error('[ObjectTree] Failed to fetch object schema:', err); - } finally { - // `finally`, so the two early `return`s and a rejected fetch all settle - // too — see the latch's docstring. - if (!cancelled) setSchemaSettled(true); - } - }; - fetchSchema(); - return () => { - cancelled = true; - }; - }, [schema.objectName, dataSource, dataConfig]); + /** + * The object schema, and whether it has settled FOR `schemaKey` — a single + * piece of state, from the shared hook ruled in objectui#6482. + * + * It feeds FOUR things: parent-field auto-detection, column labels, the + * `$expand` list built below, and (objectui#6014) the per-field definitions + * the cell formatter reads to resolve select options and reference values. + * + * ## Why the hook, and not the two `useState`s that were here + * + * This component used to carry the definition (`objectSchema`) and "has it + * settled" (`schemaSettled`) as two SEPARATE pieces of state, the second a + * one-way latch that nothing ever reset. Two independent values cannot + * express "settled, but for a DIFFERENT object" — so on an object switch the + * gate below read `schemaSettled === true` left over from the PREVIOUS + * object's settle, while `objectSchema` still held the previous object's + * fields, and the query went out as + * `find(newObject, { $expand: [ …previous object's relation fields… ] })`: + * rejected or silently ignored depending on the adapter, plus the transient + * it painted, before a correct second query followed (objectui#6481). + * + * `useSettledSchema` holds ONE value, `{ key, def } | null`, and derives + * readiness during render by comparing the settled key against `schemaKey`. + * The gate therefore closes in the SAME commit that changes the object + * rather than one commit later — and "ready for the wrong object" is not + * merely fixed but unwritable, because there is no second piece of state + * left to disagree with the first. + * + * ## The settle-on-every-exit guarantee, preserved + * + * The `finally` that used to live here existed so the two early `return`s + * (no `dataSource` / no `getObjectSchema`; no object name) and a rejected + * read all settled too — otherwise the gated record query below waits + * forever, and a tree whose adapter serves no schema never renders a row. + * The hook makes that structural rather than incidental: each of those exits + * settles explicitly with `def: null`, which is a DISTINCT outcome from "not + * ready yet". Both halves are pinned in + * `ObjectTree.settledSchemaKeying-6481.test.tsx`. + * + * Gate PLACEMENT stays this component's own, per that same ruling: it sits + * INSIDE the object-provider branch of the record effect, not at the top of + * it, because the inline/static branches issue no metadata read and must not + * be made to wait on one. + */ + const { ready: schemaSettled, def: objectSchema } = useSettledSchema( + schemaKey, + dataSource, + ); // Fetch records. useEffect(() => {