From 9a723ebbe7003c91648e02061d833f9f22f5dd71 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 12:05:07 +0000 Subject: [PATCH] =?UTF-8?q?fix(app-shell):=20the=20Studio=20grid's=20colum?= =?UTF-8?q?ns=20keep=20a=20stable=20identity=20=E2=80=94=20no=20duplicate?= =?UTF-8?q?=20find()=20per=20render=20(#4567)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Data pillar built its object-view `table.fields` inline, allocating a fresh columns array on every render. That array's IDENTITY is a data-fetch input downstream: plugin-view's ObjectView forwards it to the `renderListView` slot as `columns` by reference, and ListView derives its `$expand` fields from `schema.columns` with the array in that memo's dependency array by identity, which is itself in the fetch effect's dependency array. Each render of the pillar therefore issued another list query — measured 1 to 4 across three re-renders that changed nothing — invisible in the UI while multiplying backend load. The array is now memoized on the draft's `fields`, so it changes identity only when its contents change. Keyed on `objDraft.fields` rather than `objDraft`: `onPatch` replaces the draft object while keeping `fields` identical, so the looser key would refetch on every unrelated draft edit. The fix is at the PRODUCER. ListView's by-identity dependency is correct for a genuine column change and is untouched (plugin-list is read-only here), as is the refresh channel pinned by #4549. Tests drive the real producer (`DataPillar`), not a reconstruction: three no-op re-renders issue no extra find(), and a real column change still refetches through a path that does not remount the grid, so the dependency is proven live rather than defeated. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3 --- .../studio-grid-columns-stable-identity.md | 9 + .../StudioDesignSurface.gridColumns.test.tsx | 172 ++++++++++++++++++ .../studio-design/StudioDesignSurface.tsx | 47 ++++- 3 files changed, 221 insertions(+), 7 deletions(-) create mode 100644 .changeset/studio-grid-columns-stable-identity.md create mode 100644 packages/app-shell/src/views/studio-design/StudioDesignSurface.gridColumns.test.tsx diff --git a/.changeset/studio-grid-columns-stable-identity.md b/.changeset/studio-grid-columns-stable-identity.md new file mode 100644 index 000000000..ee86c8287 --- /dev/null +++ b/.changeset/studio-grid-columns-stable-identity.md @@ -0,0 +1,9 @@ +--- +'@object-ui/app-shell': patch +--- + +The Studio Data pillar's grid no longer issues a duplicate `find()` on every render (#4567). + +The pillar built its object-view `table.fields` inline, allocating a fresh columns array on every render. That array's IDENTITY is a data-fetch input downstream: plugin-view's ObjectView forwards it to the `renderListView` slot as `columns` by reference, and ListView derives its `$expand` fields from `schema.columns` with the array in that memo's dependency array by identity, which is itself in the fetch effect's dependency array. So each render of the pillar issued another list query — measured 1 to 4 across three re-renders that changed nothing. It was invisible in the UI (the rows repaint with the same data) while multiplying backend load on a surface that re-renders at keystroke rate. + +The columns array is now memoized on the draft's `fields`, so it changes identity only when its contents change. The dependency stays live: adding, removing or reordering a field still refetches. The fix is module-local memoization at the producer — `ListView`'s by-identity dependency is correct for a genuine column change and is untouched, as is the refresh channel pinned by #4549. diff --git a/packages/app-shell/src/views/studio-design/StudioDesignSurface.gridColumns.test.tsx b/packages/app-shell/src/views/studio-design/StudioDesignSurface.gridColumns.test.tsx new file mode 100644 index 000000000..48b1a17a2 --- /dev/null +++ b/packages/app-shell/src/views/studio-design/StudioDesignSurface.gridColumns.test.tsx @@ -0,0 +1,172 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The Data pillar's grid must not issue a duplicate find() on every render — + * objectui#4567. + * + * Measured on origin/main: driving the pillar and forcing three re-renders that + * change NOTHING (same object, same fields, same refresh signal) produced one + * extra `find()` per render — 1 -> 4. The trigger is the IDENTITY of the columns + * array, not its contents. Three legs, all on the path this suite drives: + * + * 1. producer — `StudioDesignSurface`'s object-view schema built + * `table.fields` with `readFields(objDraft.fields).entries.map().filter()` + * inline, allocating a fresh array on every render; + * 2. forward — plugin-view's ObjectView hands that array to the + * `renderListView` slot as `columns` BY REFERENCE + * (plugin-view/src/ObjectView.tsx:997 — no map/filter of its own), and in + * design mode there is no saved view, so it falls through to + * `schema.table?.fields`; + * 3. consumer — `ListView` derives `expandFields` from `schema.columns` with + * that array in the memo's dep array BY IDENTITY + * (plugin-list/src/ListView.tsx:1245-1294), and `expandFields` is itself in + * the fetch effect's dep array (:1609). A fresh array therefore refetches. + * + * The fix is identity stabilisation at the PRODUCER (leg 1). ListView's + * by-identity dependency is correct for a real column change and is left + * untouched — plugin-list is read-only here. + * + * This suite drives the REAL producer (`DataPillar`), not a reconstruction of + * it: the defect is a property of how that component builds its schema, so a + * harness that built its own array would measure the harness. That is also why + * it is a separate file from `StudioDesignSurface.gridRefresh.test.tsx` + * (objectui#4549), which drives the slot directly and hoists its own stable + * `columns` to work around this very defect — those tests stay as they are. + */ + +import '@testing-library/jest-dom/vitest'; +import * as React from 'react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { render, screen, fireEvent, cleanup, waitFor } from '@testing-library/react'; +import { MemoryRouter } from 'react-router-dom'; + +const objectDef = { + name: 'showcase_task', + label: 'Task', + fields: [ + { name: 'name', label: 'Name', type: 'text' }, + { name: 'status', label: 'Status', type: 'text' }, + ], +}; + +const mockClient = { + list: vi.fn(async () => [{ name: 'showcase_task', label: 'Task' }]), + listDrafts: vi.fn(async () => []), + layered: vi.fn(async () => ({ effective: objectDef, code: objectDef })), + getDraft: vi.fn(async () => null), + save: vi.fn(async () => ({})), +}; + +/** The counting dataSource — `find` is the measurement. */ +function createDataSource() { + return { + find: vi.fn(async () => []), + findOne: vi.fn(async () => null), + create: vi.fn(async () => ({})), + update: vi.fn(async () => ({})), + delete: vi.fn(async () => ({})), + getObjectSchema: vi.fn(async () => objectDef), + }; +} + +/** + * Module-scope so the `useAdapter` mock factory (hoisted) closes over the + * binding rather than a value, and so the reference handed to the pillar is + * STABLE across renders — an adapter that changed identity per render would + * manufacture the very churn these tests measure. + */ +let dataSource = createDataSource(); + +vi.mock('../metadata-admin/useMetadata', async (importOriginal) => { + const mod = await importOriginal(); + return { + ...mod, + useMetadataClient: () => mockClient, + useMetadataTypes: () => ({ entries: [] }), + }; +}); + +vi.mock('./packages-io', async (importOriginal) => { + const mod = await importOriginal(); + return { ...mod, fetchPackages: vi.fn(async () => []) }; +}); + +vi.mock('@object-ui/react', async (importOriginal) => { + const mod = await importOriginal(); + return { ...mod, useAdapter: () => dataSource }; +}); + +import { DataPillar } from './StudioDesignSurface'; + +beforeEach(() => { + dataSource = createDataSource(); +}); + +afterEach(() => { + cleanup(); + vi.clearAllMocks(); +}); + +/** Long enough for the mount's own fetch to settle before a measurement. */ +const SETTLE_MS = 300; +const settle = () => new Promise((r) => setTimeout(r, SETTLE_MS)); + +/** + * Renders the real pillar under a parent that can force a no-op re-render. + * `DataPillar` is not memoized, so a parent render re-renders it — which is + * exactly the Studio steady state the card describes (its whole schema is a + * fresh object literal each time). + */ +function Harness(): React.ReactElement { + const [, force] = React.useState(0); + return ( + + + + + ); +} + +/** Mount, land on the auto-selected object's grid, and let the first fetch settle. */ +async function mountSettledGrid(): Promise { + render(); + await waitFor(() => expect(dataSource.find).toHaveBeenCalled(), { timeout: 4000 }); + await settle(); + return dataSource.find.mock.calls.length; +} + +describe('Studio Data pillar grid — the columns array keeps a stable identity (#4567)', () => { + it('three no-op re-renders issue NO extra find()', async () => { + const before = await mountSettledGrid(); + + // Nothing changes: same object, same fields, same refresh signal. Before the + // fix each of these rebuilt `table.fields`, and one find() rode out per render. + fireEvent.click(screen.getByTestId('rerender')); + fireEvent.click(screen.getByTestId('rerender')); + fireEvent.click(screen.getByTestId('rerender')); + await settle(); + + expect(dataSource.find.mock.calls.length).toBe(before); + }); + + /** + * The dependency must stay LIVE, not be defeated. This pin is green before and + * after the fix on purpose — it is a must-not-change, not a red-first case. + * Its force comes from the path it uses: "+ Add field" mutates `objDraft.fields` + * WITHOUT remounting the grid (its key is `${current.name}:${gridVer}` and + * `gridVer` is unchanged), so the refetch it produces can only have arrived + * through the column-identity dependency chain. A memo that froze the columns + * (or returned a constant) would turn this red. + */ + it('a REAL column change still refetches — the dependency stays live', async () => { + const before = await mountSettledGrid(); + + fireEvent.click(screen.getAllByRole('button', { name: /Add field/i })[0]); + + await waitFor(() => expect(dataSource.find.mock.calls.length).toBeGreaterThan(before), { + timeout: 4000, + }); + }); +}); diff --git a/packages/app-shell/src/views/studio-design/StudioDesignSurface.tsx b/packages/app-shell/src/views/studio-design/StudioDesignSurface.tsx index 0bc43cc4b..e98c54ee3 100644 --- a/packages/app-shell/src/views/studio-design/StudioDesignSurface.tsx +++ b/packages/app-shell/src/views/studio-design/StudioDesignSurface.tsx @@ -2165,6 +2165,42 @@ export function DataPillar({ const fieldCount = React.useMemo(() => readFields(objDraft.fields).entries.length, [objDraft]); + /** + * The design-mode grid's columns: the object's own fields in metadata order, + * dropping framework-managed/audit fields so the grid opens on the meaningful + * columns first. Also drops a field named `actions` — the grid always pins its + * own row-actions column headed "Actions", so a data column of the same name + * reads as a duplicated column. (The field stays editable in the form designer.) + * + * Memoized because the IDENTITY of this array, not its contents, is a data-fetch + * input downstream (objectui#4567). It reaches `ListView` unchanged — plugin-view's + * ObjectView forwards it to the `renderListView` slot as `columns` by reference + * (plugin-view/src/ObjectView.tsx) — and ListView derives its `$expand` fields from + * `schema.columns` with that array in the memo's dependency array BY IDENTITY, which + * is itself in the fetch effect's dependency array (plugin-list/src/ListView.tsx). + * Built inline, this allocated a fresh array on every render of the pillar, so the + * Studio grid issued a duplicate find() per render — measured 1 -> 4 across three + * re-renders that changed nothing. The pillar re-renders constantly (its whole + * schema is a fresh object literal each time), so that was a steady-state duplicate + * query source against the backend, invisible in the UI because the rows just + * repainted with the same data. + * + * Keyed on `objDraft.fields` rather than on `objDraft`: `onPatch` replaces the draft + * object while keeping `fields` identical, so the looser key would churn the columns + * — and refetch — on every unrelated draft edit (icon, label). The dependency stays + * LIVE: a real field add/remove/reorder produces a new `fields`, hence a new array, + * hence the refetch that change must have. Stabilising identity here is deliberately + * a PRODUCER-side fix; ListView's by-identity dependency is correct for a genuine + * column change and is left alone. + */ + const gridColumns = React.useMemo( + () => + readFields(objDraft.fields) + .entries.map((e) => e.name) + .filter((n) => !STUDIO_SYSTEM_FIELD_NAMES.has(n) && n !== 'actions'), + [objDraft.fields], + ); + const onPatch = React.useCallback((patch: Record) => { setObjDraft((d) => ({ ...d, ...patch })); setDirty(true); @@ -2596,14 +2632,11 @@ export function DataPillar({ // own fields as columns (in metadata order), dropping // framework-managed/audit fields so the grid opens on the // meaningful columns first — the way Airtable does. + // Memoized at the top of the component: this array's IDENTITY + // is a fetch input downstream, so rebuilding it inline here + // issued a duplicate find() on every render (objectui#4567). table: { - fields: readFields(objDraft.fields) - .entries.map((e) => e.name) - // Also drop a field named `actions`: the grid always pins - // its own row-actions column headed "Actions", so a data - // column of the same name reads as a duplicated column. - // The field stays editable in the form designer. - .filter((n) => !STUDIO_SYSTEM_FIELD_NAMES.has(n) && n !== 'actions'), + fields: gridColumns, }, } as never }