From 91311f0c9bf94c0e08cdc53e210136b89209d27c Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 28 Aug 2026 15:59:43 +0000 Subject: [PATCH] fix(plugin-tree): key ObjectTree's schema-settled gate to the bound object MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adopt the shared `useSettledSchema` hook from `@object-ui/react` in `ObjectTree`, replacing BOTH the unkeyed `schemaSettled` boolean and the separate `objectSchema` state with the hook's single `{ key, def } | null`. Two independent pieces of state cannot express "settled, but for a DIFFERENT object". `schemaSettled` was a one-way latch set `true` in a `finally` and never reset, so on an object switch the gate read `true` from the previous object's settle while `objectSchema` still held the previous object's fields, and the record query went out as find(newObject, { $filter: …, $expand: [ …previous object's fields… ] }) Readiness is now 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. Deliberately unchanged: gate PLACEMENT stays inside the object-provider branch of the record effect, 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. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_8ca04858-ea8e-5b85-9182-de59aa49e00c --- .../6481-objecttree-keyed-schema-latch.md | 32 +++ ...jectTree.settledSchemaKeying-6481.test.tsx | 213 ++++++++++++++++++ packages/plugin-tree/src/ObjectTree.tsx | 114 +++++----- 3 files changed, 302 insertions(+), 57 deletions(-) create mode 100644 .changeset/6481-objecttree-keyed-schema-latch.md create mode 100644 packages/plugin-tree/src/ObjectTree.settledSchemaKeying-6481.test.tsx 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(() => {