From b10ac4f5ccb9e931f55c131b73d7ab83520f1202 Mon Sep 17 00:00:00 2001 From: os-sales Date: Sat, 29 Aug 2026 01:26:13 +0000 Subject: [PATCH 1/3] fix(plugin-detail,components,plugin-list): re-key three fetch effects onto primitives (#6697) --- ...rekey-three-fetch-effects-on-primitives.md | 40 ++++ .../page-tabs-discarded-probe-memo.test.tsx | 198 +++++++++++++++++ .../src/renderers/layout/containers.tsx | 16 +- packages/plugin-detail/src/RelatedList.tsx | 14 +- ...RelatedList.discardedMemoIdentity.test.tsx | 204 ++++++++++++++++++ packages/plugin-list/src/ListView.tsx | 13 +- ...istView.discardedExpandFieldsMemo.test.tsx | 188 ++++++++++++++++ 7 files changed, 670 insertions(+), 3 deletions(-) create mode 100644 .changeset/6697-rekey-three-fetch-effects-on-primitives.md create mode 100644 packages/components/src/__tests__/page-tabs-discarded-probe-memo.test.tsx create mode 100644 packages/plugin-detail/src/__tests__/RelatedList.discardedMemoIdentity.test.tsx create mode 100644 packages/plugin-list/src/__tests__/ListView.discardedExpandFieldsMemo.test.tsx diff --git a/.changeset/6697-rekey-three-fetch-effects-on-primitives.md b/.changeset/6697-rekey-three-fetch-effects-on-primitives.md new file mode 100644 index 0000000000..2c7baa7212 --- /dev/null +++ b/.changeset/6697-rekey-three-fetch-effects-on-primitives.md @@ -0,0 +1,40 @@ +--- +'@object-ui/plugin-detail': patch +'@object-ui/components': patch +'@object-ui/plugin-list': patch +--- + +Re-key three more renderer effects onto the primitives they actually read, +instead of the memoised object identity that produced them (objectui#6697 — +the three census members from objectui#6592 that sit outside its +`getDataConfig(schema)` family): + +- `RelatedList`'s collection fetch now depends on `defaultSortKey` / + `filterKey` (the `JSON.stringify`-derived content strings the two memos are + already keyed on) rather than on `defaultSortSpec` / `listFilterNode`. +- `page:tabs`' related-count probe now depends on a serialised `probeKey` + rather than on the `probeTargets` `Map`. +- `ListView`'s data fetch now depends on a serialised `expandKey` rather than + on the `expandFields` array. + +`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 all three factories return a FRESH value on every call +(`normalizeSortSpec`/`toFilterNode` build a new array / a freshly lowered AST, +the probe factory builds a new `Map`, `buildExpandFields` returns a new array +in every branch). So each effect re-ran on a discard alone, with nothing an +author or a caller controls having changed: an extra `dataSource.find` for the +related collection, an extra `dataSource.find` for the list window, and a +redundant re-probe of every tab's count. Keying on the primitives makes a +cache discard a no-op and returns `useMemo` to being a pure optimisation. + +Severity is low and the fix is deliberately narrow: the observable was a +redundant round trip, never incorrect data, so only the re-run condition +moves — each effect body still reads the memoised value, and a genuine change +of content still refetches exactly as before. + +One correction to the census card's account, measured while pinning it: for +`page:tabs` the redundant probe costs nothing on the wire. +`RelatedCountStore.fetch` returns the cached count as its first act and dedupes +concurrent probes, so the extra work is the effect re-running, not an extra +request. diff --git a/packages/components/src/__tests__/page-tabs-discarded-probe-memo.test.tsx b/packages/components/src/__tests__/page-tabs-discarded-probe-memo.test.tsx new file mode 100644 index 0000000000..1d68508221 --- /dev/null +++ b/packages/components/src/__tests__/page-tabs-discarded-probe-memo.test.tsx @@ -0,0 +1,198 @@ +/** + * 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#6697 (item 2) — `page:tabs`' related-count probe must survive a + * DISCARDED `probeTargets` memo. + * + * `probeTargets` is a `Map`, and the count-probing effect right below it names + * that Map in its OWN dependency array. `useMemo` is a pure optimisation, not + * a correctness dependency: React may throw the cache away and recompute even + * when `[items, recordObject]` compare equal, and the factory builds a brand + * new `Map` every time. So a discard alone re-ran the effect and re-probed + * every tab's count with nothing an author or a caller controls having + * changed. + * + * ⚠️ WHAT THE OBSERVABLE ACTUALLY IS — re-measured here, because the census + * card overstates it. The card calls the cost "an extra count-probe round + * trip". `RelatedCountStore.fetch` returns the CACHED number as its first act + * and dedupes concurrent calls through `inflight`, so a re-probe of an + * already-warm key issues NO `dataSource.find` at all. The wire cost of a + * discard is therefore normally ZERO, and the honest observable — the one this + * file pins — is the redundant `RelatedCountStore.fetch` invocation the effect + * makes. Both are asserted below so the distinction survives in the record. + * + * See `plugin-detail/src/__tests__/RelatedList.discardedMemoIdentity.test.tsx` + * for why the discard is forced at the module level rather than by varying a + * prop, and why a pin built on `vi.spyOn(React, 'useMemo')` would be + * unfalsifiable. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, waitFor, cleanup } from '@testing-library/react'; +import * as ReactNS from 'react'; +import { RecordContextProvider, SchemaRenderer } from '@object-ui/react'; +import { RelatedCountStore } from '../hooks/related-count-store'; +// Registers the renderers at module scope, NOT inside a `beforeAll` — there +// the cold transform is billed to `hookTimeout` (objectui#3010/#3021). +import '../renderers'; + +const memoProxy = vi.hoisted(() => ({ markers: [] as unknown[], epoch: 0 })); + +vi.mock('react', async (importOriginal) => { + const actual = await importOriginal(); + const realUseMemo = actual.useMemo; + const patched = (factory: () => unknown, deps?: unknown[]) => + Array.isArray(deps) && deps.some((d) => memoProxy.markers.includes(d)) + ? realUseMemo(factory, [...deps, memoProxy.epoch]) + : realUseMemo(factory, deps); + return { ...actual, useMemo: patched, default: { ...(actual.default ?? actual), useMemo: patched } }; +}); + +/** Put memos whose deps name one of `markers` under this file's control. */ +function armDiscardProxy(markers: unknown[]): () => void { + memoProxy.markers = markers; + return () => { + memoProxy.markers = []; + }; +} +/** Throw away the armed memos' caches — one discard event, on demand. */ +function discardNow(): void { + memoProxy.epoch += 1; +} + +/** + * The marker. `probeTargets` is keyed on `[items, recordObject]`, and + * `recordObject` is the only one of the two this file can name. Marking it + * also discards `RecordContextProvider`'s own memo, which is harmless: every + * value the probe effect reads off the context (`dataSource`, `data.id`) is a + * stable reference or a primitive, so that memo recomputing changes nothing + * the effect compares. The post-fix green is what proves it. + */ +const RECORD_OBJECT = 'zz_probe_account'; +const PARENT_ID = 'ACC-1'; +const CHILD_OBJECT = 'zz_probe_contact'; + +const tabsSchema = (childObject: string) => ({ + type: 'page:tabs', + id: 'tabs', + items: [ + // Two tabs minimum — the strip hides itself at length 1. + { label: 'Details', value: 'details', children: [{ type: 'element:text', properties: { content: 'DETAILS' } }] }, + { + label: 'Contacts', + value: 'contacts', + children: [ + { type: 'record:related_list', properties: { objectName: childObject, relationshipField: 'account' } }, + ], + }, + ], +}); + +const makeDS = () => ({ + find: vi.fn(async () => ({ data: [{ id: 'c-1' }], total: 3 })), + getObjectSchema: vi.fn(async (name: string) => ({ name, fields: {} })), +}); + +const tree = (ds: any, childObject: string) => ( + + + +); + +const settle = () => new Promise((r) => setTimeout(r, 0)); + +let fetchSpy: ReturnType; +beforeEach(() => { + RelatedCountStore._reset(); + fetchSpy = vi.spyOn(RelatedCountStore, 'fetch'); +}); +afterEach(() => { + cleanup(); + fetchSpy.mockRestore(); + RelatedCountStore._reset(); + memoProxy.markers = []; +}); + +describe('page:tabs — the count probe survives a discarded `probeTargets` memo (objectui#6697)', () => { + it('provesTheProxyDiscriminates: the proxy reaches the same React binding the component uses', () => { + const MARKER = 'canary-marker'; + const seen: unknown[] = []; + const Probe: React.FC = () => { + seen.push(ReactNS.useMemo(() => ({}), [MARKER])); + return null; + }; + + const restore = armDiscardProxy([MARKER]); + try { + const { rerender } = render(); + // Armed but not fired: normal caching still holds. + rerender(); + expect(seen[1]).toBe(seen[0]); + + discardNow(); + rerender(); + } finally { + restore(); + } + expect(seen[2]).not.toBe(seen[1]); + }); + + it('does not re-probe when `probeTargets` is discarded under an UNCHANGED tab set', async () => { + const ds = makeDS(); + const restore = armDiscardProxy([RECORD_OBJECT]); + try { + const { rerender } = render(tree(ds, CHILD_OBJECT)); + await waitFor(() => expect(fetchSpy).toHaveBeenCalled()); + // Settle to a RESTING count first. The store bumps its version when the + // first probe resolves, and `countsVersion` is a dependency of the probe + // effect BY DESIGN (objectui#2269: an invalidation has to re-probe), so + // mount legitimately costs more than one `fetch` call before the loop + // quiesces. Anchoring on "called once" instead would measure that + // designed re-probe and blame it on the discard. + await settle(); + await settle(); + const atRest = fetchSpy.mock.calls.length; + const findsAtRest = ds.find.mock.calls.length; + expect(atRest).toBeGreaterThan(0); + + // One discard, then a re-render of the same tree. `probeTargets` + // reconstructs an equal-content `Map` with a new identity; the tabs, + // their related lists, the parent id and the data source are all + // untouched. Nothing here is a reason to probe again. + discardNow(); + rerender(tree(ds, CHILD_OBJECT)); + await settle(); + + expect(fetchSpy.mock.calls.length).toBe(atRest); + // The store would have absorbed a redundant probe anyway (see header): + // this is the "no wire cost either way" half of the record. + expect(ds.find.mock.calls.length).toBe(findsAtRest); + } finally { + restore(); + } + }); + + it('still DOES probe again when a tab genuinely points at a different object', async () => { + const ds = makeDS(); + const { rerender } = render(tree(ds, CHILD_OBJECT)); + await waitFor(() => expect(fetchSpy).toHaveBeenCalledTimes(1)); + expect(fetchSpy.mock.calls[0][1]).toBe(CHILD_OBJECT); + + rerender(tree(ds, 'zz_probe_case')); + + await waitFor(() => expect(fetchSpy.mock.calls.length).toBeGreaterThan(1)); + const last = fetchSpy.mock.calls[fetchSpy.mock.calls.length - 1]; + expect(last[1]).toBe('zz_probe_case'); + }); +}); diff --git a/packages/components/src/renderers/layout/containers.tsx b/packages/components/src/renderers/layout/containers.tsx index 236ed5ccd6..50cc714c06 100644 --- a/packages/components/src/renderers/layout/containers.tsx +++ b/packages/components/src/renderers/layout/containers.tsx @@ -531,6 +531,19 @@ const PageTabsRenderer: React.FC = ({ schema, className, ...props }) => { }); return out; }, [items, recordObject]); + // objectui#6697 — the probe effect below keys on THIS string, not on the + // `Map` above. `useMemo` is a pure optimisation, not a correctness + // dependency: React may discard the cache and recompute even when + // `[items, recordObject]` compare equal, and the factory builds a brand new + // `Map` every time, so naming `probeTargets` in the effect re-probed every + // tab on a discard alone. A string cannot carry that identity churn — it + // compares by VALUE — so the effect now re-runs only when the probe set + // really differs. Derived inside a memo of its own so the walk is paid once + // per genuine recompute rather than once per render. + const probeKey = React.useMemo( + () => JSON.stringify(Array.from(probeTargets.entries())), + [probeTargets], + ); React.useEffect(() => { if (!ds || typeof ds.find !== 'function') return; @@ -563,7 +576,8 @@ const PageTabsRenderer: React.FC = ({ schema, className, ...props }) => { return () => { cancelled = true; }; - }, [ds, probeTargets, parentId, countsVersion]); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [ds, probeKey, parentId, countsVersion]); // Compute the displayed count by reading the store for every probe target. // useRelatedCountVersion above subscribed us to changes, so any store update — diff --git a/packages/plugin-detail/src/RelatedList.tsx b/packages/plugin-detail/src/RelatedList.tsx index 045d0d3e6b..05267d8d44 100644 --- a/packages/plugin-detail/src/RelatedList.tsx +++ b/packages/plugin-detail/src/RelatedList.tsx @@ -597,7 +597,19 @@ export const RelatedList: React.FC = ({ return () => { cancelled = true; }; - }, [api, dataProvided, dataSource, referenceField, parentId, refreshNonce, windowed, effectivePageSize, fetchPage, fetchSortField, fetchSortDirection, defaultSortSpec, listFilterNode]); + // objectui#6697 — keyed on the two CONTENT strings, not on the memoised + // objects they produce. `useMemo` is a pure optimisation, not a + // correctness dependency: React may discard a cache and recompute even + // when the deps compare equal, and `normalizeSortSpec`/`toFilterNode` + // both hand back a FRESH value on every call (a new array; a freshly + // lowered AST). Naming `defaultSortSpec`/`listFilterNode` here therefore + // re-ran this effect — and re-fetched the whole collection — on a discard + // alone, with nothing an author or a caller controls having changed. The + // body still reads the memoised values; only the re-run condition moves, + // onto the very keys the memos are already keyed on, so a content change + // still refetches exactly as before. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [api, dataProvided, dataSource, referenceField, parentId, refreshNonce, windowed, effectivePageSize, fetchPage, fetchSortField, fetchSortDirection, defaultSortKey, filterKey]); // Windowed mode: a page beyond the (shrunken) collection — e.g. the last // row of the last page was just deleted — comes back empty. Step back one diff --git a/packages/plugin-detail/src/__tests__/RelatedList.discardedMemoIdentity.test.tsx b/packages/plugin-detail/src/__tests__/RelatedList.discardedMemoIdentity.test.tsx new file mode 100644 index 0000000000..ad3e617e1e --- /dev/null +++ b/packages/plugin-detail/src/__tests__/RelatedList.discardedMemoIdentity.test.tsx @@ -0,0 +1,204 @@ +/** + * 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#6697 (item 1) — `RelatedList`'s collection fetch must survive a + * DISCARDED `defaultSortSpec` / `listFilterNode` memo. + * + * The family contract (objectui#6018/#5976/#6591/#6592/#6698/#6701): `useMemo` + * is a pure optimisation, not a correctness dependency. React is permitted to + * throw a memo's cache away and recompute even when the dependency array + * compares equal, so an effect whose OWN dependency array names the memoised + * OBJECT re-runs on a discard alone. Here that meant an extra + * `dataSource.find` for the related collection with nothing an author or a + * caller controls having changed. + * + * ⚠️ WHY THE FAMILY'S USUAL DISCARD PROXY DOES NOT WORK ON THIS COMPONENT. + * The pins for `ObjectMap`/`ObjectTree` force the recompute by handing the + * component a NEW prop reference carrying equal content, because those memos + * are keyed on the prop object itself. `RelatedList`'s two memos are already + * keyed on `JSON.stringify`-derived STRINGS (`defaultSortKey` / `filterKey`), + * so a new `defaultSort` / `filter` reference with equal content produces an + * EQUAL key and the memo simply keeps its cache — the trigger would never fire + * and the pin would pass no matter what the source said. That is exactly how + * `ObjectCalendar`'s first pin in #6592 came out green pre-fix: a pin that + * cannot fail is indistinguishable from a working guard. + * + * So this file forces the DISCARD ITSELF instead of varying a prop. `useMemo` + * is replaced at the MODULE level, because it cannot be replaced any other + * way: `RelatedList` reaches it through `import * as React from 'react'`, and + * that namespace is a frozen `[object Module]` — `vi.spyOn`, plain assignment + * and `Object.defineProperty` all fail on it ("Cannot redefine property"), + * while patching the separate `import React from 'react'` interop default + * object succeeds and reaches NOTHING in this component. Measured in this + * session: a first draft that patched only the default binding reported all + * four cases green against the UNFIXED source. `provesTheProxyDiscriminates` + * below is the permanent guard against that recurring. + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, waitFor, cleanup } from '@testing-library/react'; +import * as ReactNS from 'react'; +import { RelatedList } from '../RelatedList'; + +/** + * Armed per-test by `armDiscardProxy`. While `markers` is empty the mock below + * is a pure pass-through, so every other test in this file sees stock React + * semantics. `epoch` is what actually fires a discard: it is appended to the + * deps of every MARKED memo, so bumping it once invalidates exactly those + * caches, exactly once. (Appending unconditionally while armed also keeps the + * deps array a CONSTANT length for the lifetime of a marked memo — arming + * mid-test would otherwise change its size between renders, which React + * reports as a warning and handles on its own terms.) + */ +const memoProxy = vi.hoisted(() => ({ markers: [] as unknown[], epoch: 0 })); + +vi.mock('react', async (importOriginal) => { + const actual = await importOriginal(); + const realUseMemo = actual.useMemo; + const patched = (factory: () => unknown, deps?: unknown[]) => + Array.isArray(deps) && deps.some((d) => memoProxy.markers.includes(d)) + ? // An extra dependency that changes only when `discardNow()` says so == + // a cache thrown away at exactly that moment == the memo recomputing + // precisely as a discard makes it. + realUseMemo(factory, [...deps, memoProxy.epoch]) + : realUseMemo(factory, deps); + return { ...actual, useMemo: patched, default: { ...(actual.default ?? actual), useMemo: patched } }; +}); + +afterEach(() => { + cleanup(); + memoProxy.markers = []; +}); + +/** + * Put every `useMemo` whose dependency array contains one of `markers` under + * this file's control. Scoped by marker rather than applied globally so the + * assertions below isolate the memos under test; arming alone discards + * nothing. + */ +function armDiscardProxy(markers: unknown[]): () => void { + memoProxy.markers = markers; + return () => { + memoProxy.markers = []; + }; +} + +/** Throw away the armed memos' caches — one discard event, on demand. */ +function discardNow(): void { + memoProxy.epoch += 1; +} + +const ROWS = [{ id: 'c1', name: 'Alice' }]; +const SORT = [{ field: 'name', order: 'asc' as const }]; +/** + * A MongoDB-style object, on purpose: `toFilterNode` hands an ARRAY source + * straight back by identity, so an array `filter` would survive a discard by + * accident and blunt the trigger. An object goes through + * `convertFiltersToAST`, which builds a fresh value on every call — the + * identity churn a real discard produces. + */ +const FILTER = { stage: 'won' }; +/** The exact strings `RelatedList` keys the two memos on. */ +const SORT_KEY = JSON.stringify(SORT); +const FILTER_KEY = JSON.stringify(FILTER); + +const makeDS = () => ({ + find: vi.fn(async () => ROWS), + getObjectSchema: vi.fn(async (name: string) => ({ name, fields: {} })), +}); + +const listElement = (ds: any, over: Record = {}) => ( + +); + +const settle = () => new Promise((r) => setTimeout(r, 0)); + +describe('RelatedList — the collection fetch survives a discarded memo (objectui#6697)', () => { + it('provesTheProxyDiscriminates: the proxy reaches the same React binding the component uses', () => { + const MARKER = 'canary-marker'; + const seen: unknown[] = []; + // Deliberately the NAMESPACE binding — the one `RelatedList` itself + // imports. A canary written against the interop default would pass while + // the component under test never saw the proxy at all. + const Probe: React.FC = () => { + seen.push(ReactNS.useMemo(() => ({}), [MARKER])); + return null; + }; + + const restore = armDiscardProxy([MARKER]); + try { + const { rerender } = render(); + // Armed but not fired: normal caching still holds, so a green result + // below cannot come from the proxy simply churning everything. + rerender(); + expect(seen[1]).toBe(seen[0]); + + discardNow(); + rerender(); + } finally { + restore(); + } + // Same deps, new identity — the shape a discarded cache produces. + expect(seen[2]).not.toBe(seen[1]); + }); + + it('does not re-fetch when `defaultSortSpec`/`listFilterNode` are discarded under UNCHANGED keys', async () => { + const ds = makeDS(); + const restore = armDiscardProxy([SORT_KEY, FILTER_KEY]); + try { + const { rerender } = render(listElement(ds)); + await waitFor(() => expect(ds.find).toHaveBeenCalledTimes(1)); + + // One discard, then a re-render with byte-identical props — the same + // `defaultSort` and `filter` references, so both memo KEYS are + // unchanged and only the memoised objects' identities move. Nothing + // here is a reason to talk to the server again. + discardNow(); + rerender(listElement(ds)); + await settle(); + + expect(ds.find).toHaveBeenCalledTimes(1); + } finally { + restore(); + } + }); + + it('still DOES re-fetch when the list filter genuinely changes', async () => { + const ds = makeDS(); + const { rerender } = render(listElement(ds)); + await waitFor(() => expect(ds.find).toHaveBeenCalledTimes(1)); + + rerender(listElement(ds, { filter: { stage: 'lost' } })); + + await waitFor(() => expect(ds.find).toHaveBeenCalledTimes(2)); + }); + + it('still DOES re-fetch when the declared default sort genuinely changes', async () => { + const ds = makeDS(); + const { rerender } = render(listElement(ds, { pageSize: 10 })); + await waitFor(() => expect(ds.find).toHaveBeenCalledTimes(1)); + expect(ds.find.mock.calls[0][1].$orderby).toEqual(SORT); + + rerender(listElement(ds, { pageSize: 10, defaultSort: [{ field: 'created', order: 'desc' }] })); + + await waitFor(() => expect(ds.find).toHaveBeenCalledTimes(2)); + expect(ds.find.mock.calls[1][1].$orderby).toEqual([{ field: 'created', order: 'desc' }]); + }); +}); diff --git a/packages/plugin-list/src/ListView.tsx b/packages/plugin-list/src/ListView.tsx index 9e68c5983c..14df0f6c2a 100644 --- a/packages/plugin-list/src/ListView.tsx +++ b/packages/plugin-list/src/ListView.tsx @@ -1487,6 +1487,16 @@ export const ListView = React.forwardRef(({ (schema as any).gantt, (schema as any).options, ]); + // objectui#6697 — the data-fetch effect keys on THIS string, not on the + // `expandFields` array. `useMemo` is a pure optimisation, not a correctness + // dependency: React may discard the cache and recompute even when the deps + // above compare equal, and `buildExpandFields` returns a FRESH array on + // every call (`[]`, a fresh collection, or a fresh `.filter()` result), so + // naming `expandFields` in the effect re-issued the whole `dataSource.find` + // on a discard alone. A string compares by value, so the effect now re-runs + // only when the set of expanded fields really differs; the body still reads + // `expandFields` itself. + const expandKey = React.useMemo(() => JSON.stringify(expandFields), [expandFields]); // Permissions context — must be read before the data-fetch effect so // the effect can FLS-gate the `$select` projection (preventing the @@ -1843,7 +1853,8 @@ export const ListView = React.forwardRef(({ fetchData(); return () => { isMounted = false; }; - }, [schema.objectName, schema.data, dataSource, schema.filter, effectivePageSize, currentSort, currentFilters, userFilterConditions, refreshKey, searchTerm, schema.searchableFields, expandFields, objectDefLoaded, schema.refreshTrigger, perms, serverPage, currentView, groupingConfig, ganttOwnsData]); // Re-fetch on filter/sort/search/refreshTrigger/perms/page change + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [schema.objectName, schema.data, dataSource, schema.filter, effectivePageSize, currentSort, currentFilters, userFilterConditions, refreshKey, searchTerm, schema.searchableFields, expandKey, objectDefLoaded, schema.refreshTrigger, perms, serverPage, currentView, groupingConfig, ganttOwnsData]); // Re-fetch on filter/sort/search/refreshTrigger/perms/page change // Any change to the result-defining inputs (object, filters, sort, search, // grouping, page size) invalidates the current page number — snap back to diff --git a/packages/plugin-list/src/__tests__/ListView.discardedExpandFieldsMemo.test.tsx b/packages/plugin-list/src/__tests__/ListView.discardedExpandFieldsMemo.test.tsx new file mode 100644 index 0000000000..3aefeace3d --- /dev/null +++ b/packages/plugin-list/src/__tests__/ListView.discardedExpandFieldsMemo.test.tsx @@ -0,0 +1,188 @@ +/** + * 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#6697 (item 3) — TRIAGE, then a pin. `ListView`'s data-fetch effect + * names the memoised `expandFields` ARRAY in its own dependency array, so a + * discarded memo cache re-runs the effect and issues an extra + * `dataSource.find` with nothing an author or a caller controls having + * changed. The census card explicitly did NOT confirm this member ("`ListView` + * is large, central plumbing and a false positive here would be costly to + * chase"), so the first case below is written to answer that question rather + * than to assume it: it is RED against the unfixed source or the member is not + * real. + * + * Two facts make this component triageable in isolation, both re-derived here + * rather than taken from the card: + * + * - `buildExpandFields` returns a FRESH array on every call — `[]`, a fresh + * collection, or a fresh `.filter()` result — so a recompute always moves + * the identity. There is no accidental stability to hide behind. + * - Of the fetch effect's ~19 dependencies, exactly TWO are memo-derived: + * `expandFields` and `perms`. Everything else is a prop, a `useState` + * value, or a plain derived primitive, all stable across a re-render with + * unchanged props. The proxy below is scoped by marker so it discards + * `expandFields` alone and leaves `perms` (keyed on `[ctx]`) cached, which + * is what lets a failure here name `expandFields` and nothing else. + * + * See `plugin-detail/src/__tests__/RelatedList.discardedMemoIdentity.test.tsx` + * for why the discard has to be forced at the module level: `ListView` reaches + * `useMemo` through `import * as React from 'react'`, and that namespace is a + * frozen `[object Module]` that `vi.spyOn`, assignment and `defineProperty` + * all fail to patch — silently leaving any pin built on them unfalsifiable. + */ + +import { describe, it, expect, vi, beforeAll, afterAll, afterEach } from 'vitest'; +import { cleanup, render, waitFor } from '@testing-library/react'; +import * as ReactNS from 'react'; +import { ComponentRegistry } from '@object-ui/core'; +import { SchemaRendererProvider } from '@object-ui/react'; +import { ListView } from '../ListView'; +import type { ListViewSchema } from '@object-ui/types'; + +const memoProxy = vi.hoisted(() => ({ markers: [] as unknown[], epoch: 0 })); + +vi.mock('react', async (importOriginal) => { + const actual = await importOriginal(); + const realUseMemo = actual.useMemo; + const patched = (factory: () => unknown, deps?: unknown[]) => + Array.isArray(deps) && deps.some((d) => memoProxy.markers.includes(d)) + ? realUseMemo(factory, [...deps, memoProxy.epoch]) + : realUseMemo(factory, deps); + return { ...actual, useMemo: patched, default: { ...(actual.default ?? actual), useMemo: patched } }; +}); + +/** Put memos whose deps name one of `markers` under this file's control. */ +function armDiscardProxy(markers: unknown[]): () => void { + memoProxy.markers = markers; + return () => { + memoProxy.markers = []; + }; +} +/** Throw away the armed memos' caches — one discard event, on demand. */ +function discardNow(): void { + memoProxy.epoch += 1; +} + +const OBJECT = 'showcase_contact'; +/** + * The marker: `schema.columns` is a dependency of the `expandFields` memo and + * of nothing else the fetch effect depends on. Held as a module constant so + * its IDENTITY is what the proxy matches. + */ +const COLUMNS = ['name', 'account']; + +function makeDataSource() { + return { + find: vi.fn(async () => ({ data: [{ id: 'c-1', name: 'Ada', account: 'a-1' }], total: 1 })), + findOne: vi.fn(), + create: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + getObjectSchema: vi.fn(async (name: string) => ({ + name, + fields: { + id: { type: 'text' }, + name: { type: 'text' }, + // A reference field, so `expandFields` carries real content rather + // than an empty array — the shape a real list actually fetches with. + account: { type: 'lookup', reference: 'accounts' }, + }, + })), + } as any; +} + +const schemaWith = (columns: string[]): ListViewSchema => + ({ type: 'list-view', objectName: OBJECT, columns } as unknown as ListViewSchema); + +const SCHEMA = schemaWith(COLUMNS); + +const listElement = (ds: any, schema: ListViewSchema) => ( + + + +); + +const settle = () => new Promise((r) => setTimeout(r, 0)); + +let prevObjectGrid: any; +beforeAll(() => { + prevObjectGrid = ComponentRegistry.get('object-grid'); + ComponentRegistry.register('object-grid', () =>
); +}); +afterAll(() => { + if (prevObjectGrid) ComponentRegistry.register('object-grid', prevObjectGrid); + else ComponentRegistry.unregister('object-grid'); +}); +afterEach(() => { + cleanup(); + memoProxy.markers = []; +}); + +describe('ListView — the data fetch survives a discarded `expandFields` memo (objectui#6697)', () => { + it('provesTheProxyDiscriminates: the proxy reaches the same React binding the component uses', () => { + const MARKER = 'canary-marker'; + const seen: unknown[] = []; + const Probe: React.FC = () => { + seen.push(ReactNS.useMemo(() => ({}), [MARKER])); + return null; + }; + + const restore = armDiscardProxy([MARKER]); + try { + const { rerender } = render(); + // Armed but not fired: normal caching still holds. + rerender(); + expect(seen[1]).toBe(seen[0]); + + discardNow(); + rerender(); + } finally { + restore(); + } + expect(seen[2]).not.toBe(seen[1]); + }); + + it('does not re-fetch when `expandFields` is discarded under an UNCHANGED column set', async () => { + const ds = makeDataSource(); + const restore = armDiscardProxy([COLUMNS]); + try { + const { rerender } = render(listElement(ds, SCHEMA)); + await waitFor(() => expect(ds.find).toHaveBeenCalledTimes(1)); + // The value under test really is the one the query carries. + expect(ds.find.mock.calls[0][1].$expand).toEqual(['account']); + + // One discard, then a re-render with the SAME schema object. + // `expandFields` recomputes to a NEW array carrying the SAME field + // names; the schema, the columns and every other dependency are + // untouched. Nothing here is a reason to re-query the server. + discardNow(); + rerender(listElement(ds, SCHEMA)); + await settle(); + + expect(ds.find).toHaveBeenCalledTimes(1); + } finally { + restore(); + } + }); + + it('still DOES re-fetch when the column set genuinely changes what must be expanded', async () => { + const ds = makeDataSource(); + const { rerender } = render(listElement(ds, SCHEMA)); + await waitFor(() => expect(ds.find).toHaveBeenCalledTimes(1)); + expect(ds.find.mock.calls[0][1].$expand).toEqual(['account']); + + // Drop the reference column: `$expand` must follow, which it can only do + // if the effect still re-runs on a genuine change. + rerender(listElement(ds, schemaWith(['name']))); + + await waitFor(() => expect(ds.find.mock.calls.length).toBeGreaterThan(1)); + const last = ds.find.mock.calls[ds.find.mock.calls.length - 1][1]; + expect(last.$expand).toBeUndefined(); + }); +}); From 26b0e47da63306d0b31ac1bc74ca73f01b172f23 Mon Sep 17 00:00:00 2001 From: os-sales Date: Sat, 29 Aug 2026 01:34:50 +0000 Subject: [PATCH 2/3] test(plugin-detail): type the discard pin's filter fixture and find mock --- ...RelatedList.discardedMemoIdentity.test.tsx | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/packages/plugin-detail/src/__tests__/RelatedList.discardedMemoIdentity.test.tsx b/packages/plugin-detail/src/__tests__/RelatedList.discardedMemoIdentity.test.tsx index ad3e617e1e..4d7282d0d7 100644 --- a/packages/plugin-detail/src/__tests__/RelatedList.discardedMemoIdentity.test.tsx +++ b/packages/plugin-detail/src/__tests__/RelatedList.discardedMemoIdentity.test.tsx @@ -97,19 +97,22 @@ function discardNow(): void { const ROWS = [{ id: 'c1', name: 'Alice' }]; const SORT = [{ field: 'name', order: 'asc' as const }]; /** - * A MongoDB-style object, on purpose: `toFilterNode` hands an ARRAY source - * straight back by identity, so an array `filter` would survive a discard by - * accident and blunt the trigger. An object goes through - * `convertFiltersToAST`, which builds a fresh value on every call — the - * identity churn a real discard produces. + * The spec's own `ViewFilterRule[]` vocabulary, chosen for what + * `toFilterNode` does with it: an array holding rule OBJECTS goes through + * `source.map(viewFilterRuleToNode)`, which builds a fresh array on every + * call — the identity churn a real discard produces. An array of bare AST + * nodes would NOT do: `toFilterNode` hands that straight back by identity, so + * the memo would survive a discard by accident and blunt the trigger. */ -const FILTER = { stage: 'won' }; +const FILTER = [{ field: 'stage', operator: 'equals' as const, value: 'won' }]; /** The exact strings `RelatedList` keys the two memos on. */ const SORT_KEY = JSON.stringify(SORT); const FILTER_KEY = JSON.stringify(FILTER); const makeDS = () => ({ - find: vi.fn(async () => ROWS), + // Params are declared so `find.mock.calls[n][1]` is typed as the query + // object rather than as an out-of-range index on an empty tuple. + find: vi.fn(async (_object: string, _params: Record) => ROWS), getObjectSchema: vi.fn(async (name: string) => ({ name, fields: {} })), }); @@ -185,7 +188,7 @@ describe('RelatedList — the collection fetch survives a discarded memo (object const { rerender } = render(listElement(ds)); await waitFor(() => expect(ds.find).toHaveBeenCalledTimes(1)); - rerender(listElement(ds, { filter: { stage: 'lost' } })); + rerender(listElement(ds, { filter: [{ field: 'stage', operator: 'equals' as const, value: 'lost' }] })); await waitFor(() => expect(ds.find).toHaveBeenCalledTimes(2)); }); From c5603dab7112fb7a2f6bf349b28ffbfc12d6e460 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 29 Aug 2026 02:47:19 +0000 Subject: [PATCH 3/3] fix(plugin-list): key the list fetch on the expand memo's inputs, not its output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit objectui#6697's ListView leg keyed the data-fetch effect on `JSON.stringify(expandFields)`. That is discard-immune, but it is not content-equivalent to what the effect reads, and it defeated objectui#4567's live-dependency pin: `buildExpandFields` collapses the collected set down to the relation roots, so a column change that leaves the expand set alone is invisible to the key — while the effect body still builds `$select` from `schema.columns` and the view bindings. Key on the memo's own INPUTS instead. They are props and state, so a discarded memo cache still cannot re-run the effect (what #6697 asked for), and every re-run the effect had before is kept (what #4567 ruled correct, with the identity stabilisation living at the producer). Adds the case that was missing locally: a column change that does NOT move `$expand` must still refetch, asserted on `$select`. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CRJge11jso9TpXRWFt1Z49 --- ...rekey-three-fetch-effects-on-primitives.md | 20 ++++++++-- packages/plugin-list/src/ListView.tsx | 40 ++++++++++++++----- ...istView.discardedExpandFieldsMemo.test.tsx | 32 +++++++++++++++ 3 files changed, 78 insertions(+), 14 deletions(-) diff --git a/.changeset/6697-rekey-three-fetch-effects-on-primitives.md b/.changeset/6697-rekey-three-fetch-effects-on-primitives.md index 2c7baa7212..ca4ef188c0 100644 --- a/.changeset/6697-rekey-three-fetch-effects-on-primitives.md +++ b/.changeset/6697-rekey-three-fetch-effects-on-primitives.md @@ -14,8 +14,10 @@ the three census members from objectui#6592 that sit outside its already keyed on) rather than on `defaultSortSpec` / `listFilterNode`. - `page:tabs`' related-count probe now depends on a serialised `probeKey` rather than on the `probeTargets` `Map`. -- `ListView`'s data fetch now depends on a serialised `expandKey` rather than - on the `expandFields` array. +- `ListView`'s data fetch now depends on the `expandFields` memo's own INPUTS + — `schema.columns`, the alternate views' binding blocks and + `objectDef?.fields`, all props and state a discard cannot move — rather than + on the `expandFields` array the memo returns. `useMemo` carries no semantic guarantee — React is permitted to discard a memo cache and recompute even when its dependency array compares equal to the @@ -31,7 +33,19 @@ cache discard a no-op and returns `useMemo` to being a pure optimisation. Severity is low and the fix is deliberately narrow: the observable was a redundant round trip, never incorrect data, so only the re-run condition moves — each effect body still reads the memoised value, and a genuine change -of content still refetches exactly as before. +still refetches exactly as before. + +The three take two routes on purpose — key on the nearest DISCARD-IMMUNE +thing. `RelatedList`'s memos are keyed on exactly one primitive each, and +`page:tabs`' probe memo is keyed on another MEMO's output (`items`), which is +not discard-immune, so both take a content string. `ListView`'s memo is keyed +on props and state, so it names those directly: a value key over +`expandFields` would NOT have been content-equivalent there — `buildExpandFields` +collapses the collected set down to the relation roots, while the effect body +also builds `$select` from `schema.columns` and the view bindings — and it +would have defeated objectui#4567's live-dependency pin, which ruled that +"ListView's by-identity dependency is correct for a real column change" and +put the identity stabilisation at the PRODUCER. One correction to the census card's account, measured while pinning it: for `page:tabs` the redundant probe costs nothing on the wire. diff --git a/packages/plugin-list/src/ListView.tsx b/packages/plugin-list/src/ListView.tsx index 14df0f6c2a..ec95520098 100644 --- a/packages/plugin-list/src/ListView.tsx +++ b/packages/plugin-list/src/ListView.tsx @@ -1487,16 +1487,6 @@ export const ListView = React.forwardRef(({ (schema as any).gantt, (schema as any).options, ]); - // objectui#6697 — the data-fetch effect keys on THIS string, not on the - // `expandFields` array. `useMemo` is a pure optimisation, not a correctness - // dependency: React may discard the cache and recompute even when the deps - // above compare equal, and `buildExpandFields` returns a FRESH array on - // every call (`[]`, a fresh collection, or a fresh `.filter()` result), so - // naming `expandFields` in the effect re-issued the whole `dataSource.find` - // on a discard alone. A string compares by value, so the effect now re-runs - // only when the set of expanded fields really differs; the body still reads - // `expandFields` itself. - const expandKey = React.useMemo(() => JSON.stringify(expandFields), [expandFields]); // Permissions context — must be read before the data-fetch effect so // the effect can FLS-gate the `$select` projection (preventing the @@ -1853,8 +1843,36 @@ export const ListView = React.forwardRef(({ fetchData(); return () => { isMounted = false; }; + // objectui#6697 — this effect names the `expandFields` memo's INPUTS, not + // the memo's OUTPUT. `useMemo` is a pure optimisation, not a correctness + // dependency: React may discard the cache and recompute even when the + // deps compare equal, and `buildExpandFields` hands back a FRESH array on + // every call, so naming `expandFields` here re-issued the whole + // `dataSource.find` on a discard alone. Its inputs are all props and + // state, which a discard cannot move — so the effect is discard-immune + // WITHOUT losing a single re-run it used to have. + // + // ⚠️ A value key over `expandFields` (`JSON.stringify(...)`) is NOT the + // route here, and this is the measured reason: `buildExpandFields` + // collapses the whole collected set down to the relation roots, so the + // key it produces is not content-equivalent to what this effect reads — + // the body builds `$select` from `schema.columns` and the view bindings + // too. It also DEFEATS objectui#4567's live-dependency pin, which drives + // "+ Add field" on the Studio grid: that appends an unpublished field, so + // the producer's `gridColumns` rebuilds with EQUAL content and only a new + // identity, and a value key cannot see it. objectui#4567 ruled that + // "ListView's by-identity dependency is correct for a real column change" + // and put the stabilisation at the PRODUCER; naming the props keeps that + // ruling intact. + // + // The sibling re-keys in this fix take the other route on purpose: + // `RelatedList`'s memos are keyed on exactly one primitive each, and + // `PageTabsRenderer`'s probe memo is keyed on another MEMO's output + // (`items`), which is not discard-immune. Key on the nearest + // discard-immune thing — props/state where they are the memo's inputs, a + // value key where they are not. // eslint-disable-next-line react-hooks/exhaustive-deps - }, [schema.objectName, schema.data, dataSource, schema.filter, effectivePageSize, currentSort, currentFilters, userFilterConditions, refreshKey, searchTerm, schema.searchableFields, expandKey, objectDefLoaded, schema.refreshTrigger, perms, serverPage, currentView, groupingConfig, ganttOwnsData]); // Re-fetch on filter/sort/search/refreshTrigger/perms/page change + }, [schema.objectName, schema.data, dataSource, schema.filter, effectivePageSize, currentSort, currentFilters, userFilterConditions, refreshKey, searchTerm, schema.searchableFields, schema.columns, (schema as any).kanban, (schema as any).calendar, (schema as any).gallery, (schema as any).timeline, (schema as any).gantt, (schema as any).options, objectDef?.fields, objectDefLoaded, schema.refreshTrigger, perms, serverPage, currentView, groupingConfig, ganttOwnsData]); // Re-fetch on filter/sort/search/refreshTrigger/perms/page change // Any change to the result-defining inputs (object, filters, sort, search, // grouping, page size) invalidates the current page number — snap back to diff --git a/packages/plugin-list/src/__tests__/ListView.discardedExpandFieldsMemo.test.tsx b/packages/plugin-list/src/__tests__/ListView.discardedExpandFieldsMemo.test.tsx index 3aefeace3d..dd0abe05b9 100644 --- a/packages/plugin-list/src/__tests__/ListView.discardedExpandFieldsMemo.test.tsx +++ b/packages/plugin-list/src/__tests__/ListView.discardedExpandFieldsMemo.test.tsx @@ -30,6 +30,14 @@ * `expandFields` alone and leaves `perms` (keyed on `[ctx]`) cached, which * is what lets a failure here name `expandFields` and nothing else. * + * Both statements above describe the UNFIXED source, which is what the first + * case has to be red against. The fix keys the effect on that memo's own + * INPUTS (`schema.columns`, the view-binding blocks, `objectDef?.fields` — + * props and state, which a discard cannot move) instead of on its output, so + * afterwards `perms` is the effect's only memo-derived dependency. The proxy's + * scope is unchanged either way: the marker is `schema.columns`, which only + * the `expandFields` memo names. + * * See `plugin-detail/src/__tests__/RelatedList.discardedMemoIdentity.test.tsx` * for why the discard has to be forced at the module level: `ListView` reaches * `useMemo` through `import * as React from 'react'`, and that namespace is a @@ -171,6 +179,30 @@ describe('ListView — the data fetch survives a discarded `expandFields` memo ( } }); + /** + * The counterpart to the case above, and the one a value key over + * `expandFields` gets WRONG. `buildExpandFields` collapses the whole + * collected set down to the relation roots, so adding a plain `text` column + * leaves `$expand` byte-identical — a key derived from that result cannot + * see the change, and the list goes on serving rows without the new column. + * `$select` is the assertion because `$select` is what actually moves. + */ + it('still DOES re-fetch when the columns change WITHOUT changing what must be expanded', async () => { + const ds = makeDataSource(); + const { rerender } = render(listElement(ds, SCHEMA)); + await waitFor(() => expect(ds.find).toHaveBeenCalledTimes(1)); + expect(ds.find.mock.calls[0][1].$select).not.toContain('status'); + expect(ds.find.mock.calls[0][1].$expand).toEqual(['account']); + + rerender(listElement(ds, schemaWith(['name', 'account', 'status']))); + + await waitFor(() => expect(ds.find.mock.calls.length).toBeGreaterThan(1)); + const last = ds.find.mock.calls[ds.find.mock.calls.length - 1][1]; + // The expand set is unchanged — only the projection moved. + expect(last.$expand).toEqual(['account']); + expect(last.$select).toContain('status'); + }); + it('still DOES re-fetch when the column set genuinely changes what must be expanded', async () => { const ds = makeDataSource(); const { rerender } = render(listElement(ds, SCHEMA));