From ee86dc33995144c25f990955e28004942d39ae07 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 28 Aug 2026 16:51:20 +0000 Subject: [PATCH] fix(plugin-tree): re-key ObjectTree's record-fetch effect onto dataConfig primitives (#6700) The record-fetch effect closed over `dataConfig`, a `useMemo` over the `schema` prop, whose identity `useMemo` carries no semantic guarantee for -- React may discard the cache and recompute even when `schema` is unchanged, and `getDataConfig(schema)` builds a fresh `{ provider, object }` / `{ provider, items }` wrapper object on every call. A discard alone was therefore enough to re-run the effect and issue an extra `dataSource.find` call, with nothing about the bound object actually different. Re-keys the effect onto the three primitive fields it actually reads off `dataConfig` (`provider`, and conditionally `object` / `items`) instead of the container object, mirroring the pattern already shipped for `ObjectMap`/`ObjectCalendar`/`ObjectGantt` (objectui#6592). This is the last `dataConfig`-identity dependence in the component: the schema-resolution effect was already re-keyed onto `useSettledSchema`'s primitive `schemaKey` by #6696. New pin (`ObjectTree.discardedConfigMemo.test.tsx`) simulates a discarded memo via a schema reference swap with identical primitive content (verified against this component's own `dataConfig = useMemo(..., [schema])`, which reliably recomputes on that swap) and asserts no extra `dataSource.find` call, alongside a counter-probe that a genuinely different `objectName` still refetches. Reverse-verified RED against the pre-fix source (2 calls observed vs. 1 expected) and GREEN after restoring the fix. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_8ca04858-ea8e-5b85-9182-de59aa49e00c --- ...700-objecttree-record-effect-primitives.md | 24 ++++ .../ObjectTree.discardedConfigMemo.test.tsx | 106 ++++++++++++++++++ packages/plugin-tree/src/ObjectTree.tsx | 37 +++++- 3 files changed, 162 insertions(+), 5 deletions(-) create mode 100644 .changeset/6700-objecttree-record-effect-primitives.md create mode 100644 packages/plugin-tree/src/ObjectTree.discardedConfigMemo.test.tsx diff --git a/.changeset/6700-objecttree-record-effect-primitives.md b/.changeset/6700-objecttree-record-effect-primitives.md new file mode 100644 index 0000000000..b4d2775677 --- /dev/null +++ b/.changeset/6700-objecttree-record-effect-primitives.md @@ -0,0 +1,24 @@ +--- +'@object-ui/plugin-tree': patch +--- + +Re-key `ObjectTree`'s record-fetch effect onto the primitive fields it +actually reads off `dataConfig` (`provider` / `object` / `items`) instead of +the whole memoised `dataConfig` object (objectui#6700, closing out the +census #6592 opened — the schema-resolution half of this component was +already re-keyed onto `useSettledSchema`'s primitive `schemaKey` by #6696). + +`useMemo` carries no semantic guarantee — React is permitted to discard a +memo cache and recompute even when its dependency array compares equal to +the previous render, and the local `getDataConfig(schema)` helper builds a +fresh `{ provider, object }` / `{ provider, items }` wrapper object on every +call. So the record-fetch effect, keyed on `dataConfig` itself, was correct +only for as long as that identity happened to survive a discard: a +recompute alone (no author or caller action) was enough to re-run the +effect and issue an extra `dataSource.find` call. Keying the effect on the +primitives instead makes a cache discard a no-op, restoring `useMemo` to a +pure optimisation — mirroring the fix already shipped for `ObjectMap` / +`ObjectCalendar` / `ObjectGantt`. + +No behaviour change for a schema whose `useMemo` caches survive normally; +the effect is unaffected by React discarding one. diff --git a/packages/plugin-tree/src/ObjectTree.discardedConfigMemo.test.tsx b/packages/plugin-tree/src/ObjectTree.discardedConfigMemo.test.tsx new file mode 100644 index 0000000000..bb073511df --- /dev/null +++ b/packages/plugin-tree/src/ObjectTree.discardedConfigMemo.test.tsx @@ -0,0 +1,106 @@ +/** + * 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#6700 — the record-fetch effect is the LAST `dataConfig`-identity + * dependence in `ObjectTree` (the schema-resolution effect #6592 also named + * was already retired by #6696 in favor of `useSettledSchema`'s primitive + * `schemaKey`). See `ObjectMap.discardedConfigMemo.test.tsx` for the full + * rationale this mirrors: `useMemo` carries no semantic guarantee — React + * may discard its cache and recompute even when `schema` itself hasn't + * changed — and `getDataConfig(schema)` builds a FRESH `{ provider, object }` + * wrapper object on every call. A fetch effect keyed on that container + * object's identity therefore re-runs (and refetches) on a bare discard, + * with nothing about the bound object actually different. + * + * The discard proxy: two schema object literals with identical primitive + * content but different references. `dataConfig = useMemo(() => + * getDataConfig(schema), [schema])` is keyed on `[schema]` (the whole prop + * object, confirmed at `ObjectTree.tsx` — `const dataConfig = useMemo(() => + * getDataConfig(schema), [schema]);`), so a schema reference swap reliably + * forces the recompute — the same "different identity, same content" shape + * a genuine memo-cache discard would produce with `schema` held constant. + * `useSettledSchema`'s OWN internal effect is keyed on the derived primitive + * `schemaKey` string, not on `schema`/`dataConfig`, so this swap leaves + * `objectSchema`/`schemaSettled` referentially stable and cannot confound + * the result through that channel. + */ + +import React from 'react'; +import { render, screen, waitFor, cleanup } from '@testing-library/react'; +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { ObjectTree } from './ObjectTree'; + +afterEach(cleanup); + +function makeDataSource() { + return { + find: vi.fn().mockResolvedValue([{ id: '1', name: 'Acme', parent_id: null }]), + getObjectSchema: vi.fn().mockResolvedValue({ name: 'business_unit', fields: {} }), + } as any; +} + +describe('ObjectTree — record-fetch effect survives a discarded `dataConfig` memo (objectui#6700)', () => { + it('does not re-fire dataSource.find when `schema` gets a new reference with the SAME primitive fields', async () => { + const dataSource = makeDataSource(); + const schemaA: any = { + type: 'object-tree', + objectName: 'business_unit', + parentField: 'parent_id', + labelField: 'name', + }; + const schemaB: any = { + type: 'object-tree', + objectName: 'business_unit', + parentField: 'parent_id', + labelField: 'name', + }; + expect(schemaA).not.toBe(schemaB); + expect(schemaA).toEqual(schemaB); + + const { rerender } = render(); + await waitFor(() => expect(screen.getByTestId('object-tree')).toBeTruthy()); + await waitFor(() => expect(dataSource.find).toHaveBeenCalledTimes(1)); + + const findCallsAtRest = dataSource.find.mock.calls.length; + + rerender(); + // Give any (incorrectly) re-triggered effect a turn of the microtask + // queue to actually issue its fetch before asserting it did not. + await new Promise((r) => setTimeout(r, 0)); + + expect(dataSource.find.mock.calls.length).toBe(findCallsAtRest); + }); + + it('still DOES re-fire when the recomputed `dataConfig` carries a genuinely different `object`', async () => { + const dataSource = makeDataSource(); + const schemaA: any = { + type: 'object-tree', + objectName: 'business_unit', + parentField: 'parent_id', + labelField: 'name', + }; + const schemaB: any = { + type: 'object-tree', + objectName: 'department', + parentField: 'parent_id', + labelField: 'name', + }; + + const { rerender } = render(); + await waitFor(() => + expect(dataSource.find).toHaveBeenCalledWith('business_unit', expect.any(Object)), + ); + const callsBefore = dataSource.find.mock.calls.length; + + rerender(); + + await waitFor(() => expect(dataSource.find.mock.calls.length).toBeGreaterThan(callsBefore)); + expect(dataSource.find).toHaveBeenCalledWith('department', expect.any(Object)); + }); +}); diff --git a/packages/plugin-tree/src/ObjectTree.tsx b/packages/plugin-tree/src/ObjectTree.tsx index 941e7ae63a..94ba6dce7f 100644 --- a/packages/plugin-tree/src/ObjectTree.tsx +++ b/packages/plugin-tree/src/ObjectTree.tsx @@ -398,6 +398,30 @@ export const ObjectTree: React.FC = ({ dataSource, ); + /** + * The record-fetch effect below used to key on `dataConfig` itself — the + * whole memoised object identity. `useMemo` carries no semantic guarantee: + * React is permitted to discard its cache and recompute, and + * `getDataConfig(schema)` builds a fresh `{ provider, object }` / + * `{ provider, items }` wrapper object on every call even when `schema` + * hasn't changed. So a discard (not just a `schema` change) was enough to + * re-run the effect and refetch, with nothing about the bound object + * actually different. These are every primitive field the effect actually + * reads off `dataConfig`; keying on them instead of the container object + * makes a cache discard a no-op for it, and returns the `useMemo` above to + * being a pure optimisation rather than a correctness dependency — + * mirroring `ObjectMap`/`ObjectCalendar`/`ObjectGantt` (objectui#6592). + * + * This is the record effect #6592's branch left untouched (objectui#6700): + * the OTHER dataConfig-identity dependence in this component — the schema + * resolution effect — was already retired by #6696 in favor of + * `useSettledSchema`'s own primitive `schemaKey` above, so this closes out + * the component rather than one effect of two. + */ + const dataProvider = dataConfig?.provider; + const dataObjectName = dataConfig?.provider === 'object' ? dataConfig.object : undefined; + const dataItems = dataConfig?.provider === 'value' ? dataConfig.items : undefined; + // Fetch records. useEffect(() => { let cancelled = false; @@ -412,7 +436,7 @@ export const ObjectTree: React.FC = ({ // columns — which usually omit the parent field and would flatten the // tree. Fetching our own records (no column projection) guarantees the // parent field is present so the hierarchy resolves. - if (dataConfig?.provider === 'object' && dataSource && typeof dataSource.find === 'function') { + if (dataProvider === 'object' && dataSource && typeof dataSource.find === 'function') { // Wait for the schema before querying. `$expand` is DERIVED from it, // so firing early guaranteed one query whose lookup columns came back // as bare ids — the user saw those raw ids painted, then replaced a @@ -421,7 +445,10 @@ export const ObjectTree: React.FC = ({ // this effect re-runs the moment the latch flips. if (!schemaSettled) return; const expand = buildExpandFields(objectSchema?.fields); - const result = await dataSource.find(dataConfig.object, { + // `dataObjectName` is required on the 'object' variant of the + // discriminated union — same narrowing the pre-refactor + // `dataConfig.object` read carried. + const result = await dataSource.find(dataObjectName as string, { $filter: schema.filter, ...(expand.length > 0 ? { $expand: expand } : {}), }); @@ -442,9 +469,9 @@ export const ObjectTree: React.FC = ({ return; } - if (dataConfig?.provider === 'value') { + if (dataProvider === 'value') { if (!cancelled) { - setRecords((dataConfig.items as any[]) ?? []); + setRecords((dataItems as any[]) ?? []); setLoading(false); } return; @@ -465,7 +492,7 @@ export const ObjectTree: React.FC = ({ return () => { cancelled = true; }; - }, [dataConfig, dataSource, schema.filter, objectSchema, schemaSettled, (rest as any).data]); + }, [dataProvider, dataObjectName, dataItems, dataSource, schema.filter, objectSchema, schemaSettled, (rest as any).data]); const config = useMemo(() => getTreeConfig(schema), [schema]); const parentField = useMemo(