diff --git a/.changeset/8666-tree-expanded-derived.md b/.changeset/8666-tree-expanded-derived.md new file mode 100644 index 0000000000..c508ec2ad0 --- /dev/null +++ b/.changeset/8666-tree-expanded-derived.md @@ -0,0 +1,33 @@ +--- +'@object-ui/plugin-tree': patch +--- + +`ObjectTree` derives expansion during render instead of mirroring it into state +(objectui#8666). + +**The rendering artifact this removes.** Expansion lived in a +`useState>(new Set())` that a passive `useEffect` keyed on +`[roots, defaultExpandedDepth]` re-seeded from the forest, with rows computed as +`flattenVisible(roots, expanded)`. So the commit that first painted the table +still carried the previous, empty mirror: the root drew, its children did not, +and a second commit drew the seeded-open forest. Probed in the DOM the sequence +was `loading` then a one-row table then a two-row table; it is now `loading` then +the two-row table. Every mount with a non-zero `defaultExpandedDepth` showed a +collapsed forest for one frame, and that frame is also why a test could observe a +half-drawn tree at all. + +**The behaviour change that comes with it, and it is the load-bearing half.** +Component state now holds only the answers the *user* gave by clicking a chevron +— a sparse map of node id to open/closed — and the seed is computed from the +forest during render. They compose by one rule: a new forest may re-seed, but a +node the user deliberately opened or closed, and which is still in the forest, +keeps the user's answer; every other node, a genuinely new one included, takes +the seed. + +That is a fix in the same direction as the frame, not a side effect of it. +Before this change a re-seed *overwrote* the user's expansion, so any change to +the identity of the record set — a refetch, a filter, a host re-render that +reallocated the rows — silently reopened every subtree the user had collapsed +and reclosed every one they had opened below `defaultExpandedDepth`. Authored +metadata is unaffected: `defaultExpandedDepth` means exactly what it meant, and +no schema key changes. diff --git a/packages/plugin-tree/README.md b/packages/plugin-tree/README.md index d71be52b38..880e15c5e6 100644 --- a/packages/plugin-tree/README.md +++ b/packages/plugin-tree/README.md @@ -43,6 +43,20 @@ const schema: ObjectQLComponentSchema = { Records whose parent is missing (or points outside the result set) are kept as roots, so nothing is silently dropped. +### Expansion: the seed and the user's answer + +`defaultExpandedDepth` **seeds** expansion; it does not own it. The seeded set is +derived from the forest during render rather than mirrored into component state, +so a tree that expands by default is painted expanded in the first commit that +has rows — there is no frame in which the forest is drawn collapsed +(objectui#8666). + +When the record set changes — a refetch, a filter, a host that reallocates the +rows — the seed is recomputed for the new forest. A node the user opened or +closed by clicking its chevron, **and which is still in the forest**, keeps the +user's answer; every other node, a genuinely new one included, takes the seed. +Expansion is per-mount session state: it is not addressable and is not persisted. + ### The `tree` view type is host composition, not authoring `tree` is **not** an authorable view type. Neither `ObjectViewSchema.defaultViewType` diff --git a/packages/plugin-tree/src/ObjectTree.contractEnvelope-6839.test.tsx b/packages/plugin-tree/src/ObjectTree.contractEnvelope-6839.test.tsx index c896254303..91b329ab93 100644 --- a/packages/plugin-tree/src/ObjectTree.contractEnvelope-6839.test.tsx +++ b/packages/plugin-tree/src/ObjectTree.contractEnvelope-6839.test.tsx @@ -82,6 +82,25 @@ * ⛔ Do not fold these back into one wait, and ⛔ do not "fix" a future red here * with a longer timeout: the failure was never slowness, it was reading a * signal that does not carry the answer. + * + * ## ⚠️ The race described above was FIXED in the component (objectui#8666) + * + * Everything above stands as the record of why these waits are shaped the way + * they are, but one of its statements is no longer true of `ObjectTree`: + * expansion is no longer a `useState` mirror re-seeded from a `useEffect`, so + * the `loading → table:1rows → table:2rows` sequence it measures is now + * `loading → table:2rows` and the intermediate one-row commit does not happen. + * The new shape and both halves of its contract are pinned in + * `ObjectTree.expandedDerived-8666.test.tsx`. + * + * ⭐ NOTHING IN THIS FILE CHANGED FOR THAT, and the note exists to say why the + * absence of a change is deliberate. No assertion here was standing on the + * two-commit sequence: the positive arms wait FOR the descendant row, which is + * a condition on the settled forest and not on how many commits produced it, so + * they were green before objectui#8666 and are green after it. The reason to + * keep them as they are is the one the section above gives — the table's + * `data-testid` is a MOUNT signal and the rows are what this file counts, which + * stays true no matter how many commits the component takes to get there. */ import React from 'react'; diff --git a/packages/plugin-tree/src/ObjectTree.expandedDerived-8666.test.tsx b/packages/plugin-tree/src/ObjectTree.expandedDerived-8666.test.tsx new file mode 100644 index 0000000000..f500e679dd --- /dev/null +++ b/packages/plugin-tree/src/ObjectTree.expandedDerived-8666.test.tsx @@ -0,0 +1,383 @@ +/** + * 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` DERIVES expansion during render, and a user's answer survives a + * re-seed (objectui#8666). + * + * Two things are pinned here, and they are pinned separately because ONE of + * them is satisfiable by an implementation strictly worse than the bug. + * + * ## 1 — the frames (`describe` "the commit sequence") + * + * Expansion used to be a `useState>(new Set())` MIRROR that a + * passive `useEffect` keyed on `[roots, defaultExpandedDepth]` re-seeded from + * the forest, with rows computed as `flattenVisible(roots, expanded)`. So the + * commit that FIRST painted the table still carried the previous, empty mirror: + * the root drew, its children did not, and a SECOND commit drew the seeded-open + * forest. Probed in the DOM on this file's own fixture, the sequence was + * + * loading → table:1rows → table:2rows + * + * and it is now + * + * loading → table:2rows + * + * ⭐ MEASURED, not inferred: the recorder is a `React.Profiler` whose + * `onRender` reads the container on EVERY commit of the tree's subtree, and + * only CONSECUTIVE IDENTICAL snapshots collapse. A one-row table and a two-row + * table are different snapshots, so an intermediate commit cannot be collapsed + * away — which is why this recorder was able to show the defect before it was + * fixed, and is the reason to trust the absence it now reports. + * + * ⚠️ The DEFERRED arm exists because a same-task pair of commits could + * otherwise be argued to have batched rather than merged. `@testing-library`'s + * `asyncWrapper` drains one macrotask before returning, so a `setTimeout(…, 0)` + * deferral sits INSIDE that drain window and proves nothing about ordering; + * the arm uses **50ms**, which lands the rows in their own task, well outside + * it. Both arms record the same sequence. + * + * ## 2 — the feature (`describe` "the user's answer vs. a re-seed") + * + * ⛔ The frames pin ALONE is passed by "seed during render and ignore the + * user's overrides" — an implementation that removes the intermediate commit + * and answers the same thing for every input, destroying expand/collapse + * entirely. That mutation is the reason this second group exists, and each of + * its cases was OBSERVED RED under it. + * + * The composition rule those cases read (stated on `resolveExpanded`): + * + * > A new forest may re-seed, but a node the user deliberately opened or closed + * > — and which is still in the forest — keeps the user's answer. Every other + * > node, including a genuinely NEW one, takes the seed. + * + * Both halves are load-bearing, and they fail in OPPOSITE directions, so each + * needs its own case: + * + * - "re-seed and drop the overrides" is caught by the two SURVIVES cases — + * the collapse case and the expand case, which also prove the override map + * carries `false` and `true` and not merely "the user touched this". + * - "never re-seed" (freeze the seed on the first forest) is caught ONLY by + * the NEW-NODE case: the two survives cases pass under it, because a frozen + * seed leaves the user's answer alone. + * + * ⚠️ Rows are read by COUNTING nodes (`querySelectorAll` over + * `tbody tr[data-testid="object-tree-row"]`) rather than by navigating to the + * first match: `queryByText` THROWS on multiple matches, so it cannot express + * "how many" without the throw becoming the result. Nothing here reads pixels — + * happy-dom reports `clientWidth: 0`, so the indent is asserted through + * `data-depth`, which is a real attribute, and never through the inline + * `paddingLeft` it also carries. + */ + +import React from 'react'; +import { render, waitFor, cleanup, fireEvent } from '@testing-library/react'; +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { ObjectTree } from './ObjectTree'; + +// Same reason as `ObjectTree.contractEnvelope-6839.test.tsx` (objectui#6892 +// slice 9): inherit the real surface through ``, because `plugin-tree` +// does not declare `@object-ui/plugin-detail` and a type-position `import()` +// of it would be a real specifier to `check-phantom-dependencies`. +vi.mock('@object-ui/plugin-detail', async (importOriginal) => ({ + ...((await importOriginal()) as Record), + RecordDetailDrawer: () => null, + deriveRecordPageHref: () => null, +})); + +/* ────────────────────────── shared DOM readers ────────────────────────── */ + +/** One row the tree-grid painted: its label and its indent level. */ +interface DrawnRow { + readonly label: string; + readonly depth: number; +} + +function drawnRows(container: HTMLElement): DrawnRow[] { + return Array.from( + container.querySelectorAll('tbody tr[data-testid="object-tree-row"]'), + ).map((tr) => ({ + label: (tr.querySelector('td')?.textContent ?? '').trim(), + depth: Number(tr.getAttribute('data-depth')), + })); +} + +/** + * The accessible name of one row's chevron — `Expand` or `Collapse`, i.e. the + * resolved expansion answer for that node, NAMED rather than inferred from + * whether a child happens to be on screen. + */ +function toggleLabel(container: HTMLElement, label: string): string | null { + const row = Array.from( + container.querySelectorAll('tbody tr[data-testid="object-tree-row"]'), + ).find((tr) => (tr.querySelector('td')?.textContent ?? '').trim() === label); + return row?.querySelector('button')?.getAttribute('aria-label') ?? null; +} + +function clickToggle(container: HTMLElement, label: string): void { + const row = Array.from( + container.querySelectorAll('tbody tr[data-testid="object-tree-row"]'), + ).find((tr) => (tr.querySelector('td')?.textContent ?? '').trim() === label); + const button = row?.querySelector('button'); + if (!button) throw new Error(`no chevron on the row labelled "${label}"`); + fireEvent.click(button); +} + +afterEach(() => { + cleanup(); + vi.clearAllMocks(); +}); + +/* ───────────────────── 1. the commit sequence (frames) ───────────────────── */ + +const ROWS = [ + { id: '1', name: 'Root', parent_id: null }, + { id: '2', name: 'Child', parent_id: '1' }, +]; + +const fetchedSchema: any = { + type: 'object-tree', + objectName: 'node', + tree: { parentField: 'parent_id', labelField: 'name' }, + data: { provider: 'object', object: 'node' }, +}; + +/** + * One commit's worth of DOM, as a short face. `loading` / `empty` / `nothing` + * are the three non-table states `ObjectTree` can return early with, kept + * distinct so a sequence cannot read as "the table" when no table was drawn. + */ +function snapshot(container: HTMLElement): string { + if (!container.querySelector('[data-testid="object-tree"]')) { + const text = container.textContent ?? ''; + if (text.includes('Loading')) return 'loading'; + if (text.includes('No records')) return 'empty'; + return 'nothing'; + } + return `table:${drawnRows(container).length}rows`; +} + +/** + * Mount the tree over a `find()` that answers after `delayMs`, recording the + * DOM at every commit of its subtree. + * + * ⛔ `render` is called ONCE, outside every `waitFor` (objectui#7802): a + * predicate that renders feeds itself on each DOM mutation and leaks a + * container per run. The reads below ARE inside `waitFor` and stay sound under + * the same rule, because they mount nothing. + */ +async function recordCommits(delayMs: number): Promise { + const find = vi.fn(async () => { + if (delayMs > 0) await new Promise((r) => setTimeout(r, delayMs)); + return { data: ROWS, total: ROWS.length }; + }); + const ds: any = { + find, + findOne: vi.fn(), + create: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + getObjectSchema: vi.fn(async () => ({ + name: 'node', + fields: { + id: { name: 'id', type: 'text' }, + name: { name: 'name', type: 'text' }, + parent_id: { name: 'parent_id', type: 'text' }, + }, + })), + }; + + // The container is created HERE rather than taken from `render`'s return, + // because the recorder fires during the very first commit — before that + // return value exists. + const container = document.createElement('div'); + document.body.appendChild(container); + + const sequence: string[] = []; + const onCommit = () => { + const face = snapshot(container); + if (sequence[sequence.length - 1] !== face) sequence.push(face); + }; + + render( + + + , + { container }, + ); + + // Wait FOR the descendant row — the row the expansion answer gates. The + // table wrapper is a MOUNT signal and appeared a commit earlier on the + // pre-fix component, which is the whole thing being measured. + await waitFor(() => + expect(drawnRows(container).length, 'the seeded-open forest must arrive').toBe(2), + ); + expect( + toggleLabel(container, 'Root'), + 'the root must have settled OPEN — 2 rows is also what a tree that ignores expansion draws', + ).toBe('Collapse'); + return sequence; +} + +describe('ObjectTree — the commit sequence (objectui#8666)', () => { + it('paints the seeded-open forest in the FIRST commit that has rows', async () => { + const sequence = await recordCommits(0); + expect( + sequence.join(' -> '), + 'a commit drawing the table with FEWER rows than the seed asks for is the defect: ' + + 'the forest must never be painted collapsed and then re-painted open', + ).toBe('loading -> table:2rows'); + }); + + it('still paints it in one commit when the rows arrive in their own task', async () => { + // 50ms, NOT 0: RTL's `asyncWrapper` drains one macrotask before returning, + // so a 0ms deferral resolves inside that drain window and would leave the + // ordering unforced. See this file's header. + const sequence = await recordCommits(50); + expect( + sequence.join(' -> '), + 'rows landing outside the drain window must not reintroduce the collapsed frame', + ).toBe('loading -> table:2rows'); + }); +}); + +/* ────────── 2. the user's answer vs. a re-seed (the feature) ────────── */ + +// Acme +// ├─ Engineering +// │ └─ Platform +// └─ Sales +const FOREST_V1 = [ + { id: '1', name: 'Acme', parent_id: null }, + { id: '2', name: 'Engineering', parent_id: '1' }, + { id: '3', name: 'Platform', parent_id: '2' }, + { id: '4', name: 'Sales', parent_id: '1' }, +]; + +/** The same four records, plus a subtree that was NOT there before. */ +const FOREST_V2_WITH_NEW_SUBTREE = [ + ...FOREST_V1.map((r) => ({ ...r })), + { id: '5', name: 'Ops', parent_id: null }, + { id: '6', name: 'Ops Platform', parent_id: '5' }, +]; + +/** The same four records, freshly allocated — a new forest of the same shape. */ +const FOREST_V2_SAME_SHAPE = FOREST_V1.map((r) => ({ ...r })); + +function mountInline(rows: any[], extra: any = {}) { + const schema: any = { + type: 'object-tree', + objectName: 'business_unit', + parentField: 'parent_id', + labelField: 'name', + fields: ['name'], + data: rows, + ...extra, + }; + return render(); +} + +function reseed( + rerender: (ui: React.ReactElement) => void, + rows: any[], + extra: any = {}, +): void { + const schema: any = { + type: 'object-tree', + objectName: 'business_unit', + parentField: 'parent_id', + labelField: 'name', + fields: ['name'], + data: rows, + ...extra, + }; + rerender(); +} + +describe("ObjectTree — the user's answer vs. a re-seed (objectui#8666)", () => { + it('keeps a node the user COLLAPSED when the forest identity changes', async () => { + const { container, rerender } = mountInline(FOREST_V1); + await waitFor(() => expect(drawnRows(container).length).toBe(4)); + + clickToggle(container, 'Engineering'); + await waitFor(() => + expect(drawnRows(container).map((r) => r.label)).toEqual(['Acme', 'Engineering', 'Sales']), + ); + + // A NEW forest of the same shape: different record objects, so `roots` is + // a different value and the seed is recomputed from scratch. + reseed(rerender, FOREST_V2_SAME_SHAPE); + await waitFor(() => expect(container.querySelector('[data-testid="object-tree"]')).toBeTruthy()); + + expect( + drawnRows(container).map((r) => r.label), + "the re-seed must not re-open a node the user closed — that is the mutation 'seed during render and drop the overrides'", + ).toEqual(['Acme', 'Engineering', 'Sales']); + expect( + toggleLabel(container, 'Engineering'), + "and the chevron must still NAME the user's answer", + ).toBe('Expand'); + }); + + it('keeps a node the user EXPANDED below `defaultExpandedDepth` across a re-seed', async () => { + // depth 0 seeds NOTHING open, so every open node here is one the user + // opened — the `true` direction of the override map, which the collapse + // case above cannot reach. + const depth0 = { defaultExpandedDepth: 0 }; + const { container, rerender } = mountInline(FOREST_V1, depth0); + await waitFor(() => expect(drawnRows(container).map((r) => r.label)).toEqual(['Acme'])); + + clickToggle(container, 'Acme'); + await waitFor(() => + expect(drawnRows(container).map((r) => r.label)).toEqual(['Acme', 'Engineering', 'Sales']), + ); + + reseed(rerender, FOREST_V2_SAME_SHAPE, depth0); + await waitFor(() => expect(container.querySelector('[data-testid="object-tree"]')).toBeTruthy()); + + expect( + drawnRows(container).map((r) => r.label), + 'a re-seed at depth 0 must not close a node the user opened', + ).toEqual(['Acme', 'Engineering', 'Sales']); + expect(toggleLabel(container, 'Acme'), 'the root must still read as open').toBe('Collapse'); + expect( + toggleLabel(container, 'Engineering'), + 'and a node the user never touched must still take the seed, which at depth 0 is closed', + ).toBe('Expand'); + }); + + it('seeds a genuinely NEW subtree open while the override still holds', async () => { + const { container, rerender } = mountInline(FOREST_V1); + await waitFor(() => expect(drawnRows(container).length).toBe(4)); + + clickToggle(container, 'Engineering'); + await waitFor(() => + expect(drawnRows(container).map((r) => r.label)).toEqual(['Acme', 'Engineering', 'Sales']), + ); + + // `Ops` and its child did not exist when the user clicked, so nothing the + // user said covers them: they take the seed. + reseed(rerender, FOREST_V2_WITH_NEW_SUBTREE); + await waitFor(() => expect(drawnRows(container).length).toBe(5)); + + expect( + drawnRows(container), + "the new subtree must arrive OPEN (a frozen seed leaves it closed) while the user's collapse still holds (a dropped override re-opens Platform)", + ).toEqual([ + { label: 'Acme', depth: 0 }, + { label: 'Engineering', depth: 1 }, + { label: 'Sales', depth: 1 }, + { label: 'Ops', depth: 0 }, + { label: 'Ops Platform', depth: 1 }, + ]); + expect(toggleLabel(container, 'Ops'), 'the new root took the seed').toBe('Collapse'); + expect(toggleLabel(container, 'Engineering'), "the old one kept the user's answer").toBe( + 'Expand', + ); + }); +}); diff --git a/packages/plugin-tree/src/ObjectTree.tsx b/packages/plugin-tree/src/ObjectTree.tsx index 5a1fc6580b..7867221cd8 100644 --- a/packages/plugin-tree/src/ObjectTree.tsx +++ b/packages/plugin-tree/src/ObjectTree.tsx @@ -255,7 +255,7 @@ function flattenVisible(roots: TreeNode[], expanded: Set): TreeNode[] { } /** Collect ids that should start expanded, honoring an optional depth cap. */ -function initialExpanded(roots: TreeNode[], depth?: number): Set { +function seedExpanded(roots: TreeNode[], depth?: number): Set { const set = new Set(); const walk = (nodes: TreeNode[]) => { for (const n of nodes) { @@ -270,6 +270,62 @@ function initialExpanded(roots: TreeNode[], depth?: number): Set { return set; } +/** + * What the USER said about one node's expansion, keyed by record id. Sparse on + * purpose: an id is present only if the user clicked that node's chevron, and + * the value is the answer they gave (`true` = open, `false` = closed). It is + * NOT an expansion set — a missing id means "the user never said", which is a + * third state that a `Set` of open ids cannot represent. + */ +type ExpansionOverrides = ReadonlyMap; + +/** + * The expanded-id set for ONE render: the seed the forest asks for, with the + * user's answers laid over it. + * + * ## The composition rule (objectui#8666) + * + * > A new forest may re-seed, but a node the user deliberately opened or closed + * > — and which is still in the forest — keeps the user's answer. Every other + * > node, including a genuinely NEW one, takes the seed. + * + * The override walk descends the WHOLE forest, not only the seeded-open part: + * the user can open a node that sits below `defaultExpandedDepth`, and its own + * children are then reachable and overridable in turn. + * + * ⭐ WHY THE USER'S EDITS ARE STORED, AND NOT THE EXPANSION SET ITSELF. The + * shape this replaces kept the resolved set in state and re-seeded it from an + * effect. That set cannot tell "the user closed this node" apart from "the seed + * never opened it", so re-seeding a new forest has only two outcomes and both + * are wrong: overwrite, and the user's collapse is lost on every identity + * change of `roots`; or union/skip, and a genuinely new subtree never opens. + * Recording the user's EDITS separately is what makes both halves derivable at + * once — it is the reason for the map, not an implementation detail of it. + * + * An override for an id that is no longer in the forest is never read, because + * this walk only visits nodes that ARE in it. It is also not discarded: if that + * record comes back (a filter widened, a refetch), it is the same record and it + * keeps the same answer. + */ +function resolveExpanded( + roots: TreeNode[], + depth: number | undefined, + overrides: ExpansionOverrides, +): Set { + const expanded = seedExpanded(roots, depth); + if (overrides.size === 0) return expanded; + const walk = (nodes: TreeNode[]) => { + for (const n of nodes) { + const answer = overrides.get(n.id); + if (answer === true) expanded.add(n.id); + else if (answer === false) expanded.delete(n.id); + walk(n.children); + } + }; + walk(roots); + return expanded; +} + /** * One entry of a field's `options`. The index signature is not incidental — it * is what `useSafeFieldLabel().translateOptions` declares, and this alias exists @@ -615,17 +671,39 @@ export const ObjectTree: React.FC = ({ [records, parentField], ); - const [expanded, setExpanded] = useState>(new Set()); - // Re-seed expansion whenever the tree shape changes. - useEffect(() => { - setExpanded(initialExpanded(roots, config.defaultExpandedDepth)); - }, [roots, config.defaultExpandedDepth]); - - const toggle = (id: string) => - setExpanded((prev) => { - const next = new Set(prev); - if (next.has(id)) next.delete(id); - else next.add(id); + /** + * Expansion is DERIVED, not mirrored (objectui#8666). + * + * State holds only the user's own answers; the seed is computed from the + * forest during render. What this fixes: the seed used to live in a + * `useState` set that a passive `useEffect` re-seeded, so the commit that + * FIRST painted the table still carried the previous (empty) mirror — the + * root drew, its children did not, and a second commit drew the seeded-open + * forest. Probed in the DOM, the sequence was + * `loading → table with 1 row → table with 2 rows`; it is now + * `loading → table with 2 rows`. See {@link resolveExpanded} for the rule + * that lets a re-seed and a user's override coexist, which is the half a + * naive "just seed during render" conversion destroys. + * + * Pinned by `ObjectTree.expandedDerived-8666.test.tsx` — both halves. + */ + const [overrides, setOverrides] = useState(() => new Map()); + + const expanded = useMemo( + () => resolveExpanded(roots, config.defaultExpandedDepth, overrides), + [roots, config.defaultExpandedDepth, overrides], + ); + + /** + * Record the user's answer for one node. `isOpen` is the state the row was + * PAINTED with, read from the resolved set at the call site: the updater + * below sees only the override map, which does not know the seed, so the + * node's current answer has to come from the render that the user clicked. + */ + const toggle = (id: string, isOpen: boolean) => + setOverrides((prev) => { + const next = new Map(prev); + next.set(id, !isOpen); return next; }); @@ -748,7 +826,7 @@ export const ObjectTree: React.FC = ({ className="flex h-5 w-5 items-center justify-center rounded-sm text-muted-foreground hover:bg-muted" onClick={(e) => { e.stopPropagation(); - toggle(node.id); + toggle(node.id, isOpen); }} > {isOpen ? (