fix(plugin-tree): re-key ObjectTree's record-fetch effect onto dataConfig primitives (#6700)

.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.

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(<ObjectTree schema={schemaA} dataSource={dataSource} />);
+    await waitFor(() => expect(screen.getByTestId('object-tree')).toBeTruthy());
+    await waitFor(() => expect(dataSource.find).toHaveBeenCalledTimes(1));
+
+    const findCallsAtRest = dataSource.find.mock.calls.length;
+
+    rerender(<ObjectTree schema={schemaB} dataSource={dataSource} />);
+    // 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(<ObjectTree schema={schemaA} dataSource={dataSource} />);
+    await waitFor(() =>
+      expect(dataSource.find).toHaveBeenCalledWith('business_unit', expect.any(Object)),
+    );
+    const callsBefore = dataSource.find.mock.calls.length;
+
+    rerender(<ObjectTree schema={schemaB} dataSource={dataSource} />);
+
+    await waitFor(() => expect(dataSource.find.mock.calls.length).toBeGreaterThan(callsBefore));
+    expect(dataSource.find).toHaveBeenCalledWith('department', expect.any(Object));
+  });
+});

packages/plugin-tree/src/ObjectTree.tsx

@@ -398,6 +398,30 @@ export const ObjectTree: React.FC<ObjectTreeProps> = ({
     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<ObjectTreeProps> = ({
         // 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<ObjectTreeProps> = ({
           // 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<ObjectTreeProps> = ({
           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<ObjectTreeProps> = ({
     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(