diff --git a/.changeset/10035-objectview-refresh-in-place.md b/.changeset/10035-objectview-refresh-in-place.md index 654fa171d5..7e68fa6c98 100644 --- a/.changeset/10035-objectview-refresh-in-place.md +++ b/.changeset/10035-objectview-refresh-in-place.md @@ -3,11 +3,11 @@ '@object-ui/app-shell': patch --- -A save, delete or other write no longer rebuilds the object view it happened in for most view types — the view refetches its rows in place and keeps its component state (objectui#10035). +A save, delete or other write no longer rebuilds the object view it happened in — the view refetches its rows in place and keeps its component state (objectui#10035). Both `ObjectView` layers carried their refresh counter in a React `key`, so every write remounted the whole view and reset everything held in it: a calendar jumped back to the current month and its default mode, the object page's list lost its scroll position and the visualization picked in its own switcher. The counter now reaches the rendered view as a data signal instead: - `@object-ui/app-shell`: the object page hands its refresh counter to `ListView` as `refreshTrigger`, the input `ListView`'s fetch already follows, and keys the list on the object and view alone. - `@object-ui/plugin-view`: kanban, calendar, gallery, timeline, map and tree views keep their instance across a write and receive the refetched rows. -Views whose renderer fetches for itself and reads no refresh input are still remounted after a write, because that remount is the only way they show it: gantt and chart views in both layers, any object-page list whose visualization whitelist offers gantt or chart, and the standalone grid `plugin-view` renders when no host list view is supplied. +Views whose renderer fetches for itself — gantt and chart views in both layers, any object-page list whose visualization whitelist offers gantt or chart, and the standalone grid `plugin-view` renders when no host list view is supplied — refresh in place too, through the data-invalidation bus; see the `plugin-grid` / `plugin-gantt` / `plugin-charts` note for objectui#10035 in this release. diff --git a/.changeset/10035-renderers-read-invalidation.md b/.changeset/10035-renderers-read-invalidation.md new file mode 100644 index 0000000000..6267af6fd2 --- /dev/null +++ b/.changeset/10035-renderers-read-invalidation.md @@ -0,0 +1,19 @@ +--- +'@object-ui/plugin-grid': patch +'@object-ui/plugin-gantt': patch +'@object-ui/plugin-charts': patch +'@object-ui/plugin-view': patch +'@object-ui/app-shell': patch +--- + +Gantt, chart and grid views refresh in place after a write instead of being rebuilt (objectui#10035). + +`ObjectGrid`, `ObjectGantt` and `ObjectChart` query for themselves and read no refresh input, so both object-view layers could show them a write only by remounting them — which lost a gantt's scroll, collapsed groups and zoom, a chart's open drill-down, and a grid's selection, column state and in-progress edit. Each now refetches when the data-invalidation bus (`notifyDataChanged` / `useDataInvalidation` from `@object-ui/react`) reports a change to the object it reads: + +- `@object-ui/plugin-grid`: `ObjectGrid` re-reads its rows into the same table. +- `@object-ui/plugin-gantt`: `ObjectGantt` re-reads silently and keeps the chart mounted, as its toolbar refresh does. +- `@object-ui/plugin-charts`: `ObjectChart` re-runs its query on both binding shapes — an object-bound aggregate follows its `objectName`, a dataset-bound chart follows the dataset's base object as the `queryDataset` answer names it. +- `@object-ui/plugin-view`: the object view keys every view on its identity alone, and declares on the bus its own saves and deletes and each `onMutation` write it hears. +- `@object-ui/app-shell`: the object page keys its list and its chart view on identity alone, and declares on the bus the changes it learns of without a data-source write — server actions, flows, imports, realtime events and an explicit refresh. + +Rows a host hands these components (`data`, `bind`, inline values) are still the host's to refresh; only the components' own queries follow the bus. diff --git a/packages/app-shell/src/views/ObjectView.refreshInPlace-10035.test.tsx b/packages/app-shell/src/views/ObjectView.refreshInPlace-10035.test.tsx index 526a15f939..e67e59df92 100644 --- a/packages/app-shell/src/views/ObjectView.refreshInPlace-10035.test.tsx +++ b/packages/app-shell/src/views/ObjectView.refreshInPlace-10035.test.tsx @@ -35,16 +35,27 @@ * key" fix would ship: the page's own counter used to reach the list through * the key alone. * - * ## The remount that stays, pinned as the refresh it still is + * ## The lists that draw a self-fetching visualization * * A list that can draw `gantt` or `chart` — as its own type, or through the - * author's visualization whitelist — keeps the counter in its key, because - * those renderers fetch for themselves and read nothing that moves on a write - * (`REMOUNT_TO_REFRESH_VISUALIZATIONS`). Those cases assert the fresh tree. + * author's visualization whitelist — used to keep the counter in its key, + * because those renderers query for themselves and read no refresh input + * (`REMOUNT_TO_REFRESH_VISUALIZATIONS`), and these cases asserted the fresh + * tree. The renderers now refetch in place on the data-invalidation bus, so the + * list keeps its tree and the gantt re-issues ITS OWN query, exactly once, when + * the write is declared on the bus — by the console for its own writes (the + * bridge, undo / redo), and by this page for the changes it learns of another + * way (realtime, import, server actions: `refreshData`). * * Everything here renders for real — the page, `plugin-view`'s `ObjectView` * and `plugin-list`'s `ListView` — on the harness of - * `ObjectView.hostRerenderRefetch-10046.test.tsx`. + * `ObjectView.hostRerenderRefetch-10046.test.tsx`. The gantt is a stand-in + * registered as `object-gantt` (the `ObjectView.stableFilterIdentity-7237` + * precedent: `app-shell` does not depend on `plugin-gantt`). It reads the REAL + * bus hook and queries on it exactly as `ObjectGantt` now does — + * `ObjectGantt.invalidationRefetch-10035.test.tsx` proves the real component + * does that — and carries an instance id from a `useState` initializer, so a + * remount of the visualization itself is visible too. */ import * as React from 'react'; @@ -79,10 +90,21 @@ vi.mock('@object-ui/auth', async (importOriginal) => ({ createAuthenticatedFetch: () => vi.fn(), })); +/** + * The page's realtime channel; a case sets it and re-renders to deliver one. + * `resolveAllConflicts` is ONE function for the file's life: the page's realtime + * effect names it, and the real hook hands back a `useCallback` result, so a + * fresh function per render would re-fire that effect on every render. + */ +const realtime = vi.hoisted(() => ({ + lastMessage: null as unknown, + conflicts: { hasConflicts: false, resolveAllConflicts: () => {} }, +})); + vi.mock('@object-ui/collaboration', async (importOriginal) => ({ ...(await importOriginal>()), - useRealtimeSubscription: () => ({ lastMessage: null }), - useConflictResolution: () => ({ hasConflicts: false, resolveAllConflicts: () => {} }), + useRealtimeSubscription: () => ({ lastMessage: realtime.lastMessage }), + useConflictResolution: () => realtime.conflicts, })); vi.mock('sonner', () => ({ @@ -98,6 +120,8 @@ vi.mock('./MetadataInspector', () => ({ })); vi.mock('./RecordDetailView', () => ({ RecordDetailView: () => null })); +import { ComponentRegistry } from '@object-ui/core'; +import { notifyDataChanged, useDataInvalidation } from '@object-ui/react'; import { ObjectView } from './ObjectView'; import { ExpressionProvider } from '../providers/ExpressionProvider'; @@ -129,11 +153,20 @@ function objectsWith(allView: Record) { /** The list queries `ListView` issued; the page's `$top: 0` count probe is excluded. */ let listQueries = 0; +/** The stand-in gantt's own queries, and the instance ids it mounted with. */ +let ganttQueries = 0; +let ganttInstanceSeq = 0; +/** + * One row, never none: `ListView` draws its loading skeleton IN PLACE of the + * visualization while it refetches an EMPTY list, which would unmount the + * gantt for reasons that have nothing to do with the key under test. + */ +const ROW = { id: 't1', name: 'Task 1', stage: 'a', starts_on: '2026-01-01', ends_on: '2026-01-05' }; function makeDataSource() { return { find: vi.fn(async (_object: string, params: any) => { if (params?.$top !== 0) listQueries++; - return { data: [], total: 0 }; + return { data: [{ ...ROW }], total: 1 }; }), findOne: vi.fn(async () => null), create: vi.fn(async () => ({})), @@ -146,16 +179,29 @@ function makeDataSource() { const settle = () => act(() => new Promise((resolve) => setTimeout(resolve, 400))); /** - * Mounts the page and returns the console's write signal: `externalRefreshKey` - * is what `AppContent` bumps after a record-form save, an undo and a redo. + * Mounts the page and returns two ways a write reaches it: + * - `write` — the console's own write, as `AppContent` delivers an undo: it + * bumps `externalRefreshKey` AND declares the change on the bus (a form + * save's declaration comes from the dataSource bridge instead; the two + * signals the page receives are the same); + * - `realtime` — another user's write, which the page learns of on its + * realtime channel and must declare on the bus itself. */ -async function mountPage(allView: Record): Promise<{ write: () => Promise }> { +async function mountPage( + allView: Record, +): Promise<{ write: () => Promise; realtime: () => Promise }> { const dataSource = makeDataSource(); const objects = objectsWith(allView); let bump: () => void = () => {}; + let rerender: () => void = () => {}; function Harness() { const [externalRefreshKey, setExternalRefreshKey] = React.useState(0); - bump = () => setExternalRefreshKey((n) => n + 1); + const [, setTick] = React.useState(0); + // Handed out from an effect, not assigned during render (react-hooks/globals). + React.useEffect(() => { + bump = () => setExternalRefreshKey((n) => n + 1); + rerender = () => setTick((n) => n + 1); + }, []); return ( @@ -180,18 +226,43 @@ async function mountPage(allView: Record): Promise<{ write: () await settle(); return { write: async () => { - await act(async () => bump()); + await act(async () => { + bump(); + notifyDataChanged({ objectName: OBJECT_NAME }); + }); + await settle(); + }, + realtime: async () => { + await act(async () => { + realtime.lastMessage = { type: 'update', objectName: OBJECT_NAME, at: Date.now() }; + rerender(); + }); await settle(); }, }; } +/** A gantt that queries for itself and refetches on the bus, like `ObjectGantt`. */ +function GanttStandIn({ schema }: any) { + const [id] = React.useState(() => ++ganttInstanceSeq); + const nonce = useDataInvalidation(schema?.objectName); + React.useEffect(() => { + ganttQueries++; + }, [nonce]); + return
; +} +const ganttInstance = () => screen.getByTestId('gantt-stand-in').dataset.instance; + /** A node rendered inside `ListView` — replaced wholesale by a key remount. */ const listNode = () => screen.getByTestId('view-description'); beforeEach(() => { cleanup(); listQueries = 0; + ganttQueries = 0; + ganttInstanceSeq = 0; + realtime.lastMessage = null; + ComponentRegistry.register('object-gantt', GanttStandIn as any); vi.stubGlobal( 'fetch', vi.fn(async () => @@ -212,6 +283,18 @@ describe('the object page refreshes its list in place after a write (objectui#10 it.each([ ['grid', { type: 'grid' }], ['kanban', { type: 'kanban', kanban: { groupByField: 'stage' } }], + // Used to keep the counter in its key: this host cannot see which + // visualization the in-list switcher shows, so a whitelist offering a + // self-fetching one remounted the list for EVERY visualization. + [ + 'grid whose whitelist offers gantt and chart', + { + type: 'grid', + gantt: { startDateField: 'starts_on', endDateField: 'ends_on' }, + chart: { xAxisField: 'stage', yAxisFields: ['name'] }, + appearance: { allowedVisualizations: ['grid', 'gantt', 'chart'] }, + }, + ], ])('%s: the same list re-issues its query once', async (_label, allView) => { const page = await mountPage(allView); const node = listNode(); @@ -230,32 +313,52 @@ describe('the object page refreshes its list in place after a write (objectui#10 '(a) The write REMOUNTED the list: the node read inside `ListView` before the\n' + 'write is gone. The refresh counter is back in the `` key, so every\n' + 'save throws the list\'s UI state away (AGENTS.md #8: refresh data, don\'t\n' - + 'rebuild UI). Only lists that can draw a `REMOUNT_TO_REFRESH_VISUALIZATIONS`\n' - + 'member may keep it.', + + 'rebuild UI).', ).toBe(node); }); }); -describe('a list that can draw a view with no in-place refetch path still shows a write — by remount (objectui#10035)', () => { - it.each([ - ['its own type is gantt', { type: 'gantt', gantt: { startDateField: 'starts_on', endDateField: 'ends_on' } }], - [ - 'gantt is in its visualization whitelist', - { type: 'grid', gantt: { startDateField: 'starts_on', endDateField: 'ends_on' }, appearance: { allowedVisualizations: ['grid', 'gantt'] } }, - ], - ])('%s: a write mounts a fresh list', async (_label, allView) => { - const page = await mountPage(allView); +const GANTT_VIEW = { type: 'gantt', gantt: { startDateField: 'starts_on', endDateField: 'ends_on' } }; + +describe('a list drawing a self-fetching visualization keeps its tree and the visualization refetches in place (objectui#10035)', () => { + it('its own type is gantt: a console write re-issues the gantt\'s own query once, in the same list', async () => { + const page = await mountPage(GANTT_VIEW); const node = listNode(); + expect(ganttQueries, 'the gantt must be drawing this list').toBeGreaterThan(0); + const instance = ganttInstance(); + const before = ganttQueries; await page.write(); + expect( + ganttQueries - before, + '(b) The gantt did not re-query after the write, or re-queried more than once. It\n' + + 'queries for itself and must refetch on the data-invalidation bus now that the\n' + + 'list is no longer remounted to show it the write.', + ).toBe(1); expect( listNode(), - 'The list was NOT remounted after a write although it can draw a gantt or chart.\n' - + 'Those renderers fetch for themselves and read no refresh input, so without the\n' - + 'remount they silently keep showing the pre-write rows. Remove a member from\n' - + '`REMOUNT_TO_REFRESH_VISUALIZATIONS` only together with a change that makes its\n' - + 'renderer refetch in place.', - ).not.toBe(node); + '(a) The write REMOUNTED the list: the refresh counter is back in the ``\n' + + 'key (AGENTS.md #8: refresh data, don\'t rebuild UI).', + ).toBe(node); + expect(ganttInstance(), '(a) the write REMOUNTED the gantt itself').toBe(instance); + }); + + it('its own type is gantt: a realtime change is declared on the bus by the page itself', async () => { + const page = await mountPage(GANTT_VIEW); + const node = listNode(); + const instance = ganttInstance(); + const before = ganttQueries; + + await page.realtime(); + + expect( + ganttQueries - before, + '(b) The gantt did not re-query after a realtime change. The page learns of that\n' + + 'change from no data-source write, so it must declare it on the bus itself\n' + + '(`refreshData`).', + ).toBe(1); + expect(listNode(), '(a) the realtime change REMOUNTED the list').toBe(node); + expect(ganttInstance(), '(a) the realtime change REMOUNTED the gantt itself').toBe(instance); }); }); diff --git a/packages/app-shell/src/views/ObjectView.tsx b/packages/app-shell/src/views/ObjectView.tsx index c2c41bebc8..b13b2a26ca 100644 --- a/packages/app-shell/src/views/ObjectView.tsx +++ b/packages/app-shell/src/views/ObjectView.tsx @@ -74,7 +74,7 @@ import { useObjectTranslation, useObjectLabel } from '@object-ui/i18n'; import { usePermissions } from '@object-ui/permissions'; import { useAuth, useWorkspaceAdminStatus } from '@object-ui/auth'; import { useRealtimeSubscription, useConflictResolution } from '@object-ui/collaboration'; -import { ActionProvider, useNavigationOverlay, SchemaRenderer, useActionTextLocalizer, useRowPredicate, RelatedRecordActionsProvider } from '@object-ui/react'; +import { ActionProvider, useNavigationOverlay, SchemaRenderer, useActionTextLocalizer, useRowPredicate, RelatedRecordActionsProvider, notifyDataChanged } from '@object-ui/react'; import type { RelatedRecordActionsValue, RelatedRecordHandlers } from '@object-ui/react'; import { toast } from 'sonner'; import { useConsoleActionRuntime } from '../hooks/useConsoleActionRuntime.js'; @@ -504,29 +504,6 @@ function isSameOptionsValue(a: unknown, b: unknown, depth = 0): boolean { return false; } -/** - * objectui#10035 — the visualizations `ListView` draws WITHOUT an in-place - * refetch path, so a list that can show one of them is still remounted to show - * a write (`remountKey` in `renderListView`). - * - * Every other visualization draws the rows `ListView` fetched, and `ListView` - * refetches in place on `refreshTrigger` — `tree` re-issues its own query when - * those rows change. These two query for themselves and read nothing that - * moves on a write: - * - `gantt` — the registered `object-gantt` renderer hands `ObjectGantt` only - * `schema` and `dataSource`; its own query names no refresh counter, no - * `onMutation` and no invalidation bus. - * - `chart` — `ObjectChart` runs its own aggregate query off the node and - * reads no refresh input. (A view whose own type is `chart` never reaches - * `ListView` on this page — it takes the chart branch — so this member is - * about the in-list switcher.) - * - * ⛔ Do not drop a member to "finish" objectui#10035 — that turns a remount into - * a list that silently stops showing writes. A member leaves when its renderer - * refetches in place. - */ -const REMOUNT_TO_REFRESH_VISUALIZATIONS: ReadonlySet = new Set(['gantt', 'chart']); - /** * THE record-detail URL this list surface builds — one route shape, one place. * @@ -1521,6 +1498,27 @@ function ObjectViewInner({ dataSource, objects, onEdit, externalRefreshKey }: an // Refresh trigger — bumped after view CRUD or external data mutations. const [refreshKey, setRefreshKey] = useState(0); + /** + * objectui#10035 — this page learned that the object's DATA changed by a + * path that is not a data-source write the console's + * `useMutationInvalidationBridge` announces: a server action, flow or API + * action (raw HTTP), an import (`importRecords` emits no `onMutation`), a + * realtime event (another user's write), an explicit refresh action, or a + * delete this page ran. The counter reaches `ListView`'s own rows as + * `refreshTrigger`; the renderers that query for themselves (`ObjectGrid`, + * `ObjectGantt`, `ObjectChart`) read the data-invalidation bus instead, so + * the change is declared there too — AGENTS.md #8's corollary. It used to + * reach them only by remounting the whole list. + * + * View CRUD (rename, pin, reorder, config save) keeps bumping the counter + * alone: it changes the view's metadata, not the object's data, and the new + * view config reaches every renderer as props. + */ + const refreshData = useCallback(() => { + setRefreshKey((k) => k + 1); + if (objectDef.name) notifyDataChanged({ objectName: objectDef.name }); + }, [objectDef.name]); + // Shared console action runtime: confirm/param/result dialogs, the // authenticated api/flow/server-action handlers, SPA navigation, and the // paused screen-flow runner. The same runtime PageView mounts (#1605). @@ -1531,7 +1529,7 @@ function ObjectViewInner({ dataSource, objects, onEdit, externalRefreshKey }: an dataSource, objects, objectName: objectDef.name, - onRefresh: () => setRefreshKey((k) => k + 1), + onRefresh: refreshData, }); const { confirmHandler, toastHandler } = actionRuntime; @@ -2162,7 +2160,7 @@ function ObjectViewInner({ dataSource, objects, onEdit, externalRefreshKey }: an objectLabel: objectDef.label, dataSource, onEdit, - onRefresh: () => setRefreshKey(k => k + 1), + onRefresh: refreshData, onConfirm: confirmHandler, // ToastHandler's `type` is a string-literal union — a subset of the // looser `{ type?: string }` useObjectActions declares; cast is sound. @@ -2184,9 +2182,9 @@ function ObjectViewInner({ dataSource, objects, onEdit, externalRefreshKey }: an if (hasConflicts) { resolveAllConflicts('remote'); } - setRefreshKey(k => k + 1); + refreshData(); } - }, [realtimeMessage, hasConflicts, resolveAllConflicts]); + }, [realtimeMessage, hasConflicts, resolveAllConflicts, refreshData]); // Fetch record count for footer display. // @@ -2451,14 +2449,12 @@ function ObjectViewInner({ dataSource, objects, onEdit, externalRefreshKey }: an // realtime event threw the list away — its search box, open popovers, // scroll, selection and in-list visualization choice with it. // `identityKey` is what a remount is for (another object, another - // view); `ListView` now receives the counter as `refreshTrigger`, which - // its fetch effect already names, and refetches in place. - // - // `remountKey` is kept ONLY where the renderer has no in-place refetch - // path, so dropping the counter would make it silently stop showing - // writes (see `REMOUNT_TO_REFRESH_VISUALIZATIONS`). + // view), and it is the whole key: `ListView` receives the counter as + // `refreshTrigger`, which its fetch effect names, and the + // visualizations that query for themselves (`gantt`, `chart`) refetch + // in place on the data-invalidation bus (see `refreshData`). ⛔ Do not + // put the counter back in the key. const identityKey = `${objectName}-${activeView.id}`; - const remountKey = `${identityKey}-${combinedRefreshKey}`; const viewDef = activeView; // Per-user, per-view runtime-filter cache (advanced filter + search). @@ -2515,9 +2511,9 @@ function ObjectViewInner({ dataSource, objects, onEdit, externalRefreshKey }: an */ if (viewDef.type === 'chart') { const chartConfig = viewDef.chart || {}; - // Both chart branches below keep `remountKey` (objectui#10035): - // `ObjectChart` runs its own query and reads no refresh input, so a - // remount is still the only way it shows a write. + // Both chart branches below are keyed on `identityKey` alone + // (objectui#10035): `ObjectChart` runs its own query and refetches + // in place on the data-invalidation bus (see `refreshData`). // // ADR-0021 (#1890): dataset-bound chart — the single author-facing // shape. Selects dimensions/measures BY NAME and runs through the @@ -2526,7 +2522,7 @@ function ObjectViewInner({ dataSource, objects, onEdit, externalRefreshKey }: an const dims: string[] = Array.isArray(chartConfig.dimensions) ? chartConfig.dimensions : []; const vals: string[] = Array.isArray(chartConfig.values) ? chartConfig.values : []; return ( - Loading chart…
}> + Loading chart…}> Loading chart…}> + Loading chart…}> v != null && REMOUNT_TO_REFRESH_VISUALIZATIONS.has(v), - ) - ? remountKey - : identityKey; - return ( { - setRefreshKey(k => k + 1); + refreshData(); const ok = result.importedRows; const skip = result.skippedRows; if (skip > 0) { diff --git a/packages/plugin-charts/src/ObjectChart.invalidationRefetch-10035.test.tsx b/packages/plugin-charts/src/ObjectChart.invalidationRefetch-10035.test.tsx new file mode 100644 index 0000000000..26a9ae1048 --- /dev/null +++ b/packages/plugin-charts/src/ObjectChart.invalidationRefetch-10035.test.tsx @@ -0,0 +1,210 @@ +/** + * 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#10035 — `ObjectChart` refetches IN PLACE when the data-invalidation + * bus reports a write to the object it queries (AGENTS.md #8's corollary: + * refresh data, don't rebuild UI). + * + * The chart used to read no refresh input: its fetch effect named none, so a + * host could show it a write only by bumping its `key` — a remount that also + * closes an open drill drawer and resets the chart's own state. + * + * It has TWO fetch paths, and each is pinned: + * - object-bound — `aggregate` over `schema.objectName`; keys on that object; + * - dataset-bound (ADR-0021) — `queryDataset`; the node carries no + * `objectName`, so it keys on the dataset's base object as the query's + * answer names it (`object`, the field the dashboard's DatasetWidget reads + * for drill-through). + * + * `ChartRenderer` is a stand-in carrying an instance id from a `useState` + * initializer (once per mount) and the row count it was handed. Each in-place + * case asserts: + * (a) the SAME `ChartRenderer` instance afterwards — neither a remount nor the + * loading skeleton swapped it out; + * (b) the rows written after the first read reach it; + * (c) one invalidation costs exactly one query (no refetch storm). + */ +import React from 'react'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { render, cleanup, waitFor, act, screen } from '@testing-library/react'; +import { notifyDataChanged } from '@object-ui/react'; + +let instanceSeq = 0; + +vi.mock('./ChartRenderer', () => ({ + ChartRenderer: ({ schema }: any) => { + const [id] = React.useState(() => ++instanceSeq); + return ( +
+ ); + }, +})); + +import { ObjectChart } from './ObjectChart'; + +beforeEach(() => { + instanceSeq = 0; + // The metadata probes (option colours, dataset definition) are best-effort + // and not what these cases are about; answer them with nothing. + vi.stubGlobal('fetch', vi.fn(async () => ({ ok: true, json: async () => ({}) }))); +}); +afterEach(() => { + vi.unstubAllGlobals(); + cleanup(); +}); + +const settle = () => act(() => new Promise((resolve) => setTimeout(resolve, 100))); +const chart = () => screen.getByTestId('chart-renderer'); + +/** An aggregate server whose answer grows by one bucket per call. */ +function makeAggregateSource() { + let answered = 0; + const aggregate = vi.fn(async () => { + answered += 1; + return Array.from({ length: answered }, (_, i) => ({ stage: `s${i + 1}`, amount: i + 1 })); + }); + return { aggregate }; +} + +const OBJECT_SCHEMA: any = { + type: 'object-chart', + chartType: 'bar', + objectName: 'crm_opportunity', + aggregate: { field: 'amount', function: 'sum', groupBy: 'stage' }, + xAxisKey: 'stage', + series: [{ dataKey: 'amount', label: 'Amount' }], +}; + +/** A dataset server that names its base object, and grows by one row per call. */ +function makeDatasetSource(object: string | undefined = 'task') { + let answered = 0; + const queryDataset = vi.fn(async () => { + answered += 1; + return { + rows: Array.from({ length: answered }, (_, i) => ({ status: `st${i + 1}`, task_count: i + 1 })), + fields: [ + { name: 'status', label: 'Status' }, + { name: 'task_count', label: 'Tasks' }, + ], + ...(object ? { object } : {}), + }; + }); + return { queryDataset }; +} + +const DATASET_SCHEMA: any = { + type: 'object-chart', + chartType: 'bar', + dataset: 'task_metrics', + dimensions: ['status'], + values: ['task_count'], + xAxisKey: 'status', + series: [{ dataKey: 'task_count', label: 'task_count' }], +}; + +describe('ObjectChart refetches in place on a data-invalidation (objectui#10035)', () => { + it('object-bound: a write to its object re-runs the aggregate once, into the same chart', async () => { + const ds = makeAggregateSource(); + render(); + await waitFor(() => expect(chart().dataset.rows).toBe('1')); + await settle(); + const before = chart().dataset.instance; + const queriesBefore = ds.aggregate.mock.calls.length; + + await act(async () => { + notifyDataChanged({ objectName: 'crm_opportunity' }); + }); + + await waitFor(() => + expect( + chart().dataset.rows, + '(b) The chart never re-ran its aggregate after the bus reported a write to its object.', + ).toBe('2'), + ); + await settle(); + expect(ds.aggregate.mock.calls.length - queriesBefore, '(c) one invalidation, one query').toBe(1); + expect(chart().dataset.instance, '(a) the chart was rebuilt instead of refreshed').toBe(before); + }); + + it('object-bound: a write to another object does not query', async () => { + const ds = makeAggregateSource(); + render(); + await waitFor(() => expect(chart().dataset.rows).toBe('1')); + await settle(); + const queriesBefore = ds.aggregate.mock.calls.length; + + await act(async () => { + notifyDataChanged({ objectName: 'some_other_object' }); + }); + await settle(); + + expect(ds.aggregate.mock.calls.length - queriesBefore).toBe(0); + }); + + it('dataset-bound: a write to the dataset\'s base object re-runs queryDataset once, into the same chart', async () => { + const ds = makeDatasetSource('task'); + render(); + await waitFor(() => expect(chart().dataset.rows).toBe('1')); + await settle(); + const before = chart().dataset.instance; + const queriesBefore = ds.queryDataset.mock.calls.length; + + await act(async () => { + notifyDataChanged({ objectName: 'task' }); + }); + + await waitFor(() => + expect( + chart().dataset.rows, + '(b) The dataset chart never re-ran queryDataset after the bus reported a write to\n' + + 'the base object its query answer names.', + ).toBe('2'), + ); + await settle(); + expect(ds.queryDataset.mock.calls.length - queriesBefore, '(c) one invalidation, one query').toBe(1); + expect(chart().dataset.instance, '(a) the chart was rebuilt instead of refreshed').toBe(before); + }); + + it('dataset-bound: a write to another object does not query', async () => { + const ds = makeDatasetSource('task'); + render(); + await waitFor(() => expect(chart().dataset.rows).toBe('1')); + await settle(); + const queriesBefore = ds.queryDataset.mock.calls.length; + + await act(async () => { + notifyDataChanged({ objectName: 'some_other_object' }); + }); + await settle(); + + expect(ds.queryDataset.mock.calls.length - queriesBefore).toBe(0); + }); + + it('a chart drawing inline rows queries nothing on an invalidation', async () => { + const ds = makeAggregateSource(); + render( + , + ); + await waitFor(() => expect(chart().dataset.rows).toBe('1')); + + await act(async () => { + notifyDataChanged({ objectName: 'crm_opportunity' }); + }); + await settle(); + + expect(ds.aggregate).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/plugin-charts/src/ObjectChart.tsx b/packages/plugin-charts/src/ObjectChart.tsx index 6287f4ecbb..1f26145df6 100644 --- a/packages/plugin-charts/src/ObjectChart.tsx +++ b/packages/plugin-charts/src/ObjectChart.tsx @@ -1,6 +1,6 @@ import React, { useState, useEffect, useContext, useCallback, useMemo } from 'react'; -import { useDataScope, SchemaRendererContext, SchemaRenderer, useDrillNavigation, useFilterScope, ElementDataSourceGate, type ElementDataSourceMapping } from '@object-ui/react'; +import { useDataScope, SchemaRendererContext, SchemaRenderer, useDrillNavigation, useFilterScope, ElementDataSourceGate, useDataInvalidation, type ElementDataSourceMapping } from '@object-ui/react'; import { ChartRenderer } from './ChartRenderer'; import { normalizeChartSchema } from './normalizeChartSchema'; import { ComponentRegistry, chartMeasureKey, isStructuredGroupBy, objectAggregateSpecQuery, humanizeLabel, extractRecords, computeDrillFilter, composeDrillFilter, isDrillEnabled, resolveDrillTitle, resolveFilterPlaceholders, resolveContextTokens, shiftFilterByCompareTo, compareToTrendLabelKey, chartTypeIgnoresCompareTo, buildChartSeries, buildOptionColorMap, deriveDimensionLabelMaps, dimensionOptionTranslator, loadDimensionFieldMeta, relabelDimensions, localizeFieldOptions, elementDataSourceBlock, type DimensionFieldMeta, type CompareToConfig, type DrillEvent, type ChartResultField, type ChartSegmentClickEvent } from '@object-ui/core'; @@ -503,6 +503,11 @@ export const ObjectChart = (props: ObjectChartProps) => { // buildChartSeries() below can resolve a human series label instead of // falling back to the raw field name. const [datasetFields, setDatasetFields] = useState(null); + // The dataset's BASE object, as the `queryDataset` answer names it (the same + // `object` the dashboard's DatasetWidget reads for drill-through). A dataset + // node carries no `objectName`, so this is the only object a dataset-bound + // chart can key its data-invalidation subscription on (objectui#10035). + const [datasetObject, setDatasetObject] = useState(undefined); // Start in loading state when we will fetch, so the no-data / empty branch // doesn't flash before the fetch effect runs and flips loading to true. const [loading, setLoading] = useState(() => { @@ -780,6 +785,7 @@ export const ObjectChart = (props: ObjectChartProps) => { if (mounted.current) { setFetchedData(Array.isArray(res?.rows) ? res.rows : []); setDatasetFields(Array.isArray(res?.fields) ? res.fields : null); + setDatasetObject(typeof res?.object === 'string' && res.object ? res.object : undefined); } return; } @@ -938,10 +944,29 @@ export const ObjectChart = (props: ObjectChartProps) => { // eslint-disable-next-line react-hooks/exhaustive-deps }, [schema.objectName, datasetKey, aggregateKey, filterKey, compareToKey, schema.xAxisKey, schema.chartType, runAggregate, filterScope, fieldOptionLabel]); + // objectui#10035 — the refresh input this chart had none of, so a host could + // show it a write only by remounting it (AGENTS.md #8's corollary: refresh + // data, don't rebuild UI). The nonce moves when the data-invalidation bus + // reports a change to the object this chart QUERIES, and the fetch effect + // below names it, so both binding shapes re-run their own query in place: + // - object-bound (`aggregate` / `find` over `schema.objectName`) keys on + // that object; + // - dataset-bound (`queryDataset`) keys on the dataset's base object, which + // only the query's answer names (`datasetObject` above) — a dataset node + // carries no `objectName`. + // A chart drawing bound or inline rows fetches nothing and is not subscribed. + // The re-read keeps the chart mounted (`RefreshIndicator` over the current + // rows, not the skeleton), so an open drill drawer and the chart's own state + // survive a save. + const fetchesForItself = !!(schema.objectName || schema.dataset) && !boundData && !schema.data; + const invalidationNonce = useDataInvalidation( + fetchesForItself ? (schema.dataset ? datasetObject : schema.objectName) : undefined, + ); + useEffect(() => { const mounted = { current: true }; - if ((schema.objectName || schema.dataset) && !boundData && !schema.data) { + if (fetchesForItself) { fetchData(dataSource, mounted); } else if (mounted.current) { // Have inline / bound data — won't fetch; clear loading. @@ -949,7 +974,7 @@ export const ObjectChart = (props: ObjectChartProps) => { } return () => { mounted.current = false; }; // eslint-disable-next-line react-hooks/exhaustive-deps - }, [schema.objectName, datasetKey, dataSource, boundData, schema.data, filterKey, aggregateKey, compareToKey, fetchData]); + }, [schema.objectName, datasetKey, dataSource, boundData, schema.data, filterKey, aggregateKey, compareToKey, fetchData, invalidationNonce]); const rawData = boundData || schema.data || fetchedData; const finalData = Array.isArray(rawData) ? rawData : []; diff --git a/packages/plugin-gantt/src/ObjectGantt.invalidationRefetch-10035.test.tsx b/packages/plugin-gantt/src/ObjectGantt.invalidationRefetch-10035.test.tsx new file mode 100644 index 0000000000..35449bf4c3 --- /dev/null +++ b/packages/plugin-gantt/src/ObjectGantt.invalidationRefetch-10035.test.tsx @@ -0,0 +1,157 @@ +/** + * 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#10035 — `ObjectGantt` refetches IN PLACE when the data-invalidation + * bus reports a write to the object it reads (AGENTS.md #8's corollary: + * refresh data, don't rebuild UI). + * + * The gantt used to read no refresh input: the registered `object-gantt` + * renderer hands it only `schema` and `dataSource`, and its own query named no + * counter, no `onMutation` and no bus. So a host could show it a write only by + * bumping its `key` — a remount that loses the chart's scroll, collapsed groups + * and zoom. + * + * Every case drives the REAL bus (`notifyDataChanged` from `@object-ui/react`). + * `GanttView` is a stand-in carrying an instance id from a `useState` + * initializer, which runs once per mount: a changed id means `GanttView` was + * unmounted — by a remount of the gantt OR by the loading placeholder a + * non-silent reload swaps in. Either is the damage this card is about, so the + * in-place case asserts: + * (a) the SAME `GanttView` instance afterwards, and the placeholder never + * painted; + * (b) the rows written after the first read reach it; + * (c) one invalidation costs exactly one query (no refetch storm). + */ + +import React from 'react'; +import { render, screen, waitFor, act } from '@testing-library/react'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { notifyDataChanged } from '@object-ui/react'; +import { ObjectGantt } from './ObjectGantt'; + +vi.mock('sonner', () => ({ toast: { error: vi.fn(), success: vi.fn() } })); + +let instanceSeq = 0; + +vi.mock('./GanttView', () => ({ + GanttView: ({ tasks }: any) => { + const [id] = React.useState(() => ++instanceSeq); + return ( +
+ ); + }, +})); + +vi.mock('@object-ui/plugin-detail', async (importOriginal) => ({ + ...(await importOriginal()), + RecordDetailDrawer: () => null, + deriveRecordPageHref: () => null, +})); + +const OBJECT = 'task'; + +const OBJECT_DEF = { + name: OBJECT, + fields: { + id: { name: 'id', type: 'text' }, + subject: { name: 'subject', type: 'text' }, + visible_from: { name: 'visible_from', type: 'date' }, + due_date: { name: 'due_date', type: 'date' }, + }, +}; + +/** A server whose store the test writes to, so a re-read returns the new row. */ +function makeAdapter() { + const store = [{ id: '1', subject: 'Renewal', visible_from: '2026-01-01', due_date: '2026-01-05' }]; + return { + store, + find: vi.fn(async () => ({ data: store.map((r) => ({ ...r })), total: store.length })), + findOne: vi.fn(), + create: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + getObjectSchema: vi.fn(async () => OBJECT_DEF), + } as any; +} + +const schema: any = { + type: 'object-gantt', + objectName: OBJECT, + gantt: { titleField: 'subject', startDateField: 'visible_from', endDateField: 'due_date' }, +}; + +const settle = () => act(() => new Promise((resolve) => setTimeout(resolve, 100))); +const view = () => screen.getByTestId('gantt-view'); + +beforeEach(() => { + instanceSeq = 0; +}); + +describe('ObjectGantt refetches in place on a data-invalidation for its object (objectui#10035)', () => { + it('re-reads the rows once, silently, into the same GanttView', async () => { + const adapter = makeAdapter(); + render(); + await waitFor(() => expect(view().dataset.taskCount).toBe('1')); + await settle(); + const before = view().dataset.instance; + const queriesBefore = adapter.find.mock.calls.length; + + adapter.store.push({ id: '2', subject: 'Kickoff', visible_from: '2026-01-02', due_date: '2026-01-03' }); + await act(async () => { + notifyDataChanged({ objectName: OBJECT }); + // The placeholder must not paint at any point of the reload. + expect(screen.queryByText('Loading Gantt chart...')).toBeNull(); + }); + + await waitFor(() => + expect( + view().dataset.taskCount, + '(b) The gantt never re-read its rows after the bus reported a write to its object.', + ).toBe('2'), + ); + await settle(); + expect(adapter.find.mock.calls.length - queriesBefore, '(c) one invalidation, one query').toBe(1); + expect( + view().dataset.instance, + '(a) GanttView was unmounted: the refresh went through the loading placeholder (a\n' + + 'non-silent reload) or a remount, and took the chart\'s scroll and collapse state with it.', + ).toBe(before); + expect(screen.queryByText('Loading Gantt chart...')).toBeNull(); + }); + + it('a change to another object does not query', async () => { + const adapter = makeAdapter(); + render(); + await waitFor(() => expect(view().dataset.taskCount).toBe('1')); + await settle(); + const queriesBefore = adapter.find.mock.calls.length; + + await act(async () => { + notifyDataChanged({ objectName: 'some_other_object' }); + }); + await settle(); + + expect(adapter.find.mock.calls.length - queriesBefore).toBe(0); + }); + + it('rows a host handed down are not re-queried — the host owns that refresh', async () => { + const adapter = makeAdapter(); + const hostRows = [{ id: 'h1', subject: 'Host row', visible_from: '2026-01-01', due_date: '2026-01-02' }]; + render(); + await waitFor(() => expect(view().dataset.taskCount).toBe('1')); + await settle(); + + await act(async () => { + notifyDataChanged({ objectName: OBJECT }); + }); + await settle(); + + expect(adapter.find).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/plugin-gantt/src/ObjectGantt.tsx b/packages/plugin-gantt/src/ObjectGantt.tsx index 416c8f84ed..85bd313b6a 100644 --- a/packages/plugin-gantt/src/ObjectGantt.tsx +++ b/packages/plugin-gantt/src/ObjectGantt.tsx @@ -36,6 +36,7 @@ import { useSettledSchema, SchemaRendererContext, NonGridRowCeilingNote, + useDataInvalidation, } from '@object-ui/react'; import { useLocalization, useDisplayLocale, resolveFieldCurrency } from '@object-ui/i18n'; import { @@ -973,6 +974,37 @@ export const ObjectGantt: React.FC = ({ reloadRef.current(); }, [adapterInputsKey, dataSource, apiFetch, resource, hasInlineData, dataProvider, schema.filter, schema.sort, searchTerm, searchFieldsKey, objectSchema, perms, recordQueryDerivesExpand, objectSchemaReady]); + /** + * objectui#10035 — the refresh input this gantt had none of, so a host could + * show it a write only by remounting it (AGENTS.md #8's corollary: refresh + * data, don't rebuild UI). The nonce moves when the data-invalidation bus + * reports a change to the object this gantt reads. + * + * ⭐ A SILENT reload, deliberately not the fetch effect above. That effect's + * reload flips `loading`, which swaps `GanttView` for the placeholder — the + * same loss of scroll, collapsed groups and zoom a remount causes, one level + * down. `reload({ silent: true })` is the path the toolbar refresh and every + * write-readback here already take for exactly that reason, and it keeps + * `reload`'s sequencing, so an invalidation that lands mid-load cannot let a + * stale answer win. + * + * Subscribed only when the rows come from an adapter this gantt queries + * (`recordQueryDerivesExpand`): a host `data` array and an inline `value` + * set are not ours to refresh. Each nonce is answered at most once + * (`handledInvalidationRef`): one that lands while the object-schema gate + * above is still closed is marked handled and dropped, because the gated + * first load has not run yet and reads rows written before it — so the gate + * opening later can never add a second query beside that load. + */ + const invalidationNonce = useDataInvalidation(recordQueryDerivesExpand && resource ? resource : undefined); + const handledInvalidationRef = useRef(0); + useEffect(() => { + if (invalidationNonce === handledInvalidationRef.current) return; + handledInvalidationRef.current = invalidationNonce; + if (recordQueryDerivesExpand && !objectSchemaReady) return; + void reloadRef.current({ silent: true }); + }, [invalidationNonce, recordQueryDerivesExpand, objectSchemaReady]); + // Transform data to gantt tasks const tasks = useMemo(() => { if (!ganttConfig || !data.length) { diff --git a/packages/plugin-grid/src/ObjectGrid.tsx b/packages/plugin-grid/src/ObjectGrid.tsx index fc0fa1982f..7596743159 100644 --- a/packages/plugin-grid/src/ObjectGrid.tsx +++ b/packages/plugin-grid/src/ObjectGrid.tsx @@ -25,7 +25,7 @@ import React, { useEffect, useState, useCallback, useMemo } from 'react'; import type { ObjectGridSchema, DataSource, ListColumn, TableColumn, ViewData, TableSortItem, DataTableSchema, ListViewExportFormat } from '@object-ui/types'; import { isSystemManagedField, normalizeTableColumnType } from '@object-ui/types'; import type { I18nLabel } from '@objectstack/spec/ui'; -import { SchemaRenderer, useDataScope, useNavigationOverlay, useAction, useSafeFieldLabel, usePredicateScope, useRelatedRecordActions } from '@object-ui/react'; +import { SchemaRenderer, useDataScope, useNavigationOverlay, useAction, useSafeFieldLabel, usePredicateScope, useRelatedRecordActions, useDataInvalidation } from '@object-ui/react'; import { createSafeTranslation } from '@object-ui/i18n'; // objectui#8920 — the grid reaches a cell renderer through THIS module and // nowhere else. `getCellRenderer` / `resolveCellRendererType` are deliberately @@ -1839,6 +1839,18 @@ export const ObjectGrid: React.FC = ({ return () => { cancelled = true; }; }, [hasInlineData, objectName, dataSource]); + // objectui#10035 — the refresh input this grid had none of, so a host could + // show it a write only by remounting it (AGENTS.md #8's corollary: refresh + // data, don't rebuild UI). The nonce moves when the data-invalidation bus + // reports a change to the object this grid FETCHES, and the load effect below + // names it, so the rows are re-read in place — the table stays mounted + // (`RefreshIndicator`, not the skeleton), and with it selection, scroll, + // column state and any open inline edit. Subscribed only when the grid + // fetches for itself: rows a host handed down (`data`, `bind`, an inline + // `value` set) are the host's to refresh, and a nonce there would only + // re-render for nothing. + const invalidationNonce = useDataInvalidation(hasInlineData ? undefined : objectName); + // --- Unified async data loading effect --- // Combines schema fetch + data fetch into a single async flow with AbortController. // This avoids the fragile "chained effects" pattern where Effect 1 sets objectSchema, @@ -2349,7 +2361,9 @@ export const ObjectGrid: React.FC = ({ // the query asking for the OLD one and the new grouping would read // `undefined` on every row — the very `(empty)` bucket this card fixes, // reachable a second way. - }, [objectName, schemaFields, schemaColumns, schemaFilter, schemaFilterRefusal, schemaSort, headerSort, searchTerm, schemaPagination, schemaPageSize, serverPage, serverPageSize, dataSource, hasInlineData, dataConfig, refreshKey, perms.isLoaded, groupingProjectionKey]); + // `invalidationNonce` (objectui#10035): a write to this object, reported on + // the data-invalidation bus — see its declaration above. + }, [objectName, schemaFields, schemaColumns, schemaFilter, schemaFilterRefusal, schemaSort, headerSort, searchTerm, schemaPagination, schemaPageSize, serverPage, serverPageSize, dataSource, hasInlineData, dataConfig, refreshKey, perms.isLoaded, groupingProjectionKey, invalidationNonce]); // The same reset, for the path the loader above never runs on (objectui#4501 // clause 2). "All N matching are selected" is a claim about ONE query, so it diff --git a/packages/plugin-grid/src/__tests__/ObjectGrid.invalidationRefetch-10035.test.tsx b/packages/plugin-grid/src/__tests__/ObjectGrid.invalidationRefetch-10035.test.tsx new file mode 100644 index 0000000000..e428222ec6 --- /dev/null +++ b/packages/plugin-grid/src/__tests__/ObjectGrid.invalidationRefetch-10035.test.tsx @@ -0,0 +1,149 @@ +/** + * 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#10035 — `ObjectGrid` refetches IN PLACE when the data-invalidation + * bus reports a write to the object it fetches (AGENTS.md #8's corollary: + * refresh data, don't rebuild UI). + * + * The grid used to read no refresh input at all: its load effect moved only on + * its own internal counter, so a host (`plugin-view`'s `ObjectView`) could show + * it a write only by bumping its `key` — a remount that threw away selection, + * scroll, column state and any open inline edit. The grid now names the bus's + * nonce in that effect. + * + * Every case drives the REAL bus (`notifyDataChanged` from `@object-ui/react`) + * and asserts, where it applies: + * (a) the table is the SAME DOM node afterwards — a remount would replace it; + * (b) the rows written after the first read reach the screen; + * (c) one invalidation costs exactly one refetch (no refetch storm). + */ +import { describe, it, expect, vi, beforeAll } from 'vitest'; +import { render, screen, waitFor, act } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import React from 'react'; + +import { ObjectGrid } from '../ObjectGrid'; +import { registerAllFields } from '@object-ui/fields'; +import { notifyDataChanged } from '@object-ui/react'; + +registerAllFields(); + +beforeAll(() => { + if (!Element.prototype.scrollIntoView) { + Element.prototype.scrollIntoView = vi.fn() as any; + } +}); + +const OBJECT = 'duly_task'; + +/** A server whose store the test writes to, so a refetch returns the new row. */ +function makeDataSource() { + const store: Record[] = [{ id: 'r1', name: 'First row' }]; + const find = vi.fn(async () => { + const data = store.map((r) => ({ ...r })); + return { data, total: data.length, hasMore: false, pageSize: 50 }; + }); + return { + store, + find, + getObjectSchema: async (name: string) => ({ + name, + fields: { id: { type: 'text' }, name: { type: 'text' } }, + }), + } as any; +} + +function renderGrid(dataSource: any, extra: Record = {}) { + const schema: any = { + type: 'object-grid', + objectName: OBJECT, + columns: [{ field: 'name', label: 'Name' }], + pagination: { pageSize: 50 }, + }; + return render(); +} + +/** Long enough for any effect a step schedules to have run and settled. */ +const settle = () => act(() => new Promise((resolve) => setTimeout(resolve, 150))); + +describe('ObjectGrid refetches in place on a data-invalidation for its object (objectui#10035)', () => { + it('an object-scoped change re-reads the rows once, into the same table', async () => { + const ds = makeDataSource(); + renderGrid(ds); + await waitFor(() => expect(screen.getByText('First row')).toBeInTheDocument()); + await settle(); + const table = screen.getByRole('table'); + const before = ds.find.mock.calls.length; + + ds.store.push({ id: 'r2', name: 'Written later' }); + await act(async () => { + notifyDataChanged({ objectName: OBJECT }); + }); + + await waitFor(() => + expect( + screen.queryByText('Written later'), + '(b) The grid never re-read its rows after the bus reported a write to its object.\n' + + 'Its load effect must name `useDataInvalidation(objectName)`\'s nonce.', + ).toBeInTheDocument(), + ); + await settle(); + expect(ds.find.mock.calls.length - before, '(c) one invalidation must cost exactly one refetch').toBe(1); + expect( + screen.getByRole('table'), + '(a) The table was replaced: the refresh rebuilt the grid instead of refreshing its data.', + ).toBe(table); + }); + + it('a record-scoped change to its object re-reads the list too (membership may have moved)', async () => { + const ds = makeDataSource(); + renderGrid(ds); + await waitFor(() => expect(screen.getByText('First row')).toBeInTheDocument()); + await settle(); + const before = ds.find.mock.calls.length; + + ds.store[0].name = 'Renamed'; + await act(async () => { + notifyDataChanged({ objectName: OBJECT, recordId: 'r1' }); + }); + + await waitFor(() => expect(screen.getByText('Renamed')).toBeInTheDocument()); + await settle(); + expect(ds.find.mock.calls.length - before).toBe(1); + }); + + it('a change to another object does not refetch', async () => { + const ds = makeDataSource(); + renderGrid(ds); + await waitFor(() => expect(screen.getByText('First row')).toBeInTheDocument()); + await settle(); + const before = ds.find.mock.calls.length; + + await act(async () => { + notifyDataChanged({ objectName: 'some_other_object' }); + }); + await settle(); + + expect(ds.find.mock.calls.length - before).toBe(0); + }); + + it('a grid handed its rows by a host does not fetch on an invalidation — the host owns that refresh', async () => { + const ds = makeDataSource(); + renderGrid(ds, { data: [{ id: 'h1', name: 'Host row' }] }); + await waitFor(() => expect(screen.getByText('Host row')).toBeInTheDocument()); + await settle(); + + await act(async () => { + notifyDataChanged({ objectName: OBJECT }); + }); + await settle(); + + expect(ds.find).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/plugin-view/src/ObjectView.tsx b/packages/plugin-view/src/ObjectView.tsx index a8eb765bf7..79ea85e947 100644 --- a/packages/plugin-view/src/ObjectView.tsx +++ b/packages/plugin-view/src/ObjectView.tsx @@ -75,7 +75,7 @@ import { convertSortToQueryParams, recordDelete, } from '@object-ui/core'; -import { SchemaRenderer as ImportedSchemaRenderer, useSettledSchema } from '@object-ui/react'; +import { SchemaRenderer as ImportedSchemaRenderer, useSettledSchema, notifyDataChanged } from '@object-ui/react'; import type { HandleClickModifiers } from '@object-ui/react'; import { usePermissions } from '@object-ui/permissions'; import { ViewSwitcher } from './ViewSwitcher'; @@ -739,34 +739,6 @@ export const OBJECT_VIEW_HOST_COMPOSITION_VIEW_TYPES = [ 'tree', ] as const; -/** - * objectui#10035 — the non-grid view types whose renderer still has to be - * REMOUNTED to show a write, because it has no in-place refetch path. - * - * AGENTS.md #8's corollary: refresh data, don't rebuild UI. `refreshKey` is - * this component's refresh signal, and it used to ride in the `key` of every - * view it renders, so each save, delete or `onMutation` event threw the whole - * view away. It no longer rides there for a view that refetches in place: - * `kanban`, `calendar`, `gallery`, `timeline` and `map` draw `data={data}`, - * the rows the fetch effect above re-reads when `refreshKey` moves, and - * `tree` re-issues its own query when that `data` array changes. - * - * The two members below read nothing that moves on a write, so for them the - * counter stays in the key until the renderer gains a refresh input: - * - `gantt` — the registered `object-gantt` renderer hands `ObjectGantt` - * only `schema` and `dataSource`, so `data` never reaches it, and its own - * query names no refresh counter, no `onMutation` and no invalidation bus. - * - `chart` — `ObjectChart` runs its own aggregate query off the node and - * reads neither the host's `data` nor any refresh input. - * The grid branch keeps the counter for the same reason: `ObjectGrid` fetches - * for itself and its query moves only on its own internal counter. - * - * ⛔ Do not drop a member to "finish" objectui#10035 — that turns a remount - * into a view that silently stops showing writes. A member leaves when its - * renderer refetches in place. - */ -const REMOUNT_TO_REFRESH_VIEW_TYPES: ReadonlySet = new Set(['gantt', 'chart']); - /** * ObjectView Component * @@ -882,12 +854,26 @@ export const ObjectView: React.FC = ({ // ListView-driven configurations already manage refreshKey via // form success / delete handlers. To avoid double refreshes and // duplicate find() calls, skip auto-subscription when renderListView is provided. + // + // objectui#10035 — the same write is also published on the data-invalidation + // bus, because `ObjectGrid`, `ObjectGantt` and `ObjectChart` query for + // themselves and read that bus, not `refreshKey` (see `renderContent`). The + // console's `useMutationInvalidationBridge` announces every dataSource write + // already, so there this is a duplicate raised in the SAME synchronous + // `onMutation` dispatch — React batches the two into one render, one + // refetch. Where no bridge is mounted (an `object-view` embedded outside the + // console) it is the only announcement, and it is what keeps a write reaching + // those renderers now that they are no longer remounted to show it. useEffect(() => { if (!dataSource?.onMutation || !schema.objectName) return; if (renderListView) return; const unsub = dataSource.onMutation((event: any) => { if (event.resource === schema.objectName) { setRefreshKey(prev => prev + 1); + notifyDataChanged({ + objectName: event.resource, + recordId: event.id != null ? String(event.id) : undefined, + }); } }); return unsub; @@ -1401,12 +1387,29 @@ export const ObjectView: React.FC = ({ setDeleteRequest({ open: true, records: valid, bulk: true }); }, []); + /** + * A write THIS host made — a form save or a delete — reported to both + * readers of it (objectui#10035): `refreshKey` for the rows this component + * fetches for its non-grid views, and the data-invalidation bus for the + * renderers that fetch for themselves (`ObjectGrid`, `ObjectGantt`, + * `ObjectChart`). The writer declares the change, as AGENTS.md #8's + * corollary asks, so a data source without `onMutation` still refreshes + * them. With one, the `onMutation` subscription above has announced the + * same write: the two notifications land in one render when React batches + * them, and never cost more refetches than the key bumps they replace (each + * of those remounted, and every remount fetched). + */ + const announceOwnWrite = useCallback(() => { + setRefreshKey(prev => prev + 1); + if (schema.objectName) notifyDataChanged({ objectName: schema.objectName }); + }, [schema.objectName]); + // Handle form submission const handleFormSuccess = useCallback(() => { setIsFormOpen(false); setSelectedRecord(null); - setRefreshKey(prev => prev + 1); - }, []); + announceOwnWrite(); + }, [announceOwnWrite]); // Handle form cancellation const handleFormCancel = useCallback(() => { @@ -2259,11 +2262,19 @@ export const ObjectView: React.FC = ({ // --- Content renderer --- const renderContent = () => { - // The view's IDENTITY — switching object, view or type is a real remount. - // The refresh counter is appended only where the renderer cannot refetch - // in place (objectui#10035, see `REMOUNT_TO_REFRESH_VIEW_TYPES`). + // The view's IDENTITY — switching object, view or type is a real remount, + // and it is the ONLY thing in the key (objectui#10035; AGENTS.md #8's + // corollary: refresh data, don't rebuild UI). A write no longer remounts + // any view: `kanban`, `calendar`, `gallery`, `timeline` and `map` draw + // `data={data}`, the rows the non-grid fetch effect re-reads when + // `refreshKey` moves, and `tree` re-queries when that array changes; + // `ObjectGrid`, `ObjectGantt` and `ObjectChart` query for themselves and + // refetch in place on the data-invalidation bus, which every site that + // moves `refreshKey` also notifies (`announceOwnWrite`, the `onMutation` + // subscription). ⛔ Do not put `refreshKey` back in a key: that is the + // remount the corollary forbids, and it throws away the view's scroll, + // selection, open drawers and in-progress edits on every save. const identityKey = `${schema.objectName}-${activeNamedView || activeView?.id || 'default'}-${currentViewType}`; - const remountKey = `${identityKey}-${refreshKey}`; // If a custom renderListView is provided, use it // #region object-view HOST-COMPOSITION SURFACE (objectui#5097) @@ -2431,7 +2442,7 @@ export const ObjectView: React.FC = ({ if (viewSchema && SchemaRendererComponent) { return ( = ({ } } - // Default: use ObjectGrid — still remounted to show a write, because - // `ObjectGrid` has no refresh input (see `REMOUNT_TO_REFRESH_VIEW_TYPES`). + // Default: use ObjectGrid — keyed on identity alone; it refetches in place + // on the data-invalidation bus (see `identityKey` above). return ( = ({ dataSource, t: tView, toast, - onRefresh: () => setRefreshKey(prev => prev + 1), + onRefresh: announceOwnWrite, }, bulk ? { params: { records } } diff --git a/packages/plugin-view/src/__tests__/ObjectView.refreshInPlace-10035.test.tsx b/packages/plugin-view/src/__tests__/ObjectView.refreshInPlace-10035.test.tsx index e039ac6313..26ad345f1d 100644 --- a/packages/plugin-view/src/__tests__/ObjectView.refreshInPlace-10035.test.tsx +++ b/packages/plugin-view/src/__tests__/ObjectView.refreshInPlace-10035.test.tsx @@ -16,38 +16,40 @@ * `objectName-view-type-refreshKey`, and `refreshKey` moves on every write * this component hears about (`onMutation`, a form save, a delete). So each * write REMOUNTED the view: a fresh instance, every piece of component state in - * it gone. The rows were refetched too — the non-grid fetch effect names - * `refreshKey` — which is why nothing looked broken: the damage was the - * remount, not a missing refetch. + * it gone. The rows were refetched too — through the remount — which is why + * nothing looked broken: the damage was the remount, not a missing refetch. * - * ## The two halves, and which world each one can fail in + * ## Two kinds of view, one rule * - * Each in-place case asserts BOTH: - * (a) the rendered view is the SAME instance after the write, and - * (b) it received the refetched rows. - * Against the pre-fix source (a) is the red half; (b) stays green there, - * because that world refetched as well — through the remount. (b) is the half - * that goes red if the refresh SIGNAL is lost (the counter dropped from the - * key AND from the fetch effect), which is the regression a naive "remove the - * key" fix would ship. - * - * ## The remount that stays, pinned as the refresh it still is + * - DATA-FED views (`kanban`, `calendar`, `gallery`, `timeline`, `map`, + * `tree`) draw the `data` this component fetches; its non-grid fetch effect + * re-reads on `refreshKey`. + * - SELF-FETCHING views (`gantt`, `chart`, the grid branch) render a component + * that queries for itself. They read the data-invalidation bus + * (`useDataInvalidation`), which every site that moves `refreshKey` also + * notifies. That half used to be missing — they kept `refreshKey` in their + * key instead (`REMOUNT_TO_REFRESH_VIEW_TYPES`), and these cases asserted a + * fresh instance until the renderers gained the bus input. * - * `gantt`, `chart` and the grid branch render a component that fetches for - * itself and reads nothing that moves on a write, so for them a remount is - * still the only way a write shows up (`REMOUNT_TO_REFRESH_VIEW_TYPES`). Those - * cases assert the fresh instance. When one of those renderers gains an - * in-place refresh input, its case moves to the in-place table — it is red - * until someone does that deliberately, never silently green. + * Every case asserts BOTH: + * (a) the rendered view is the SAME instance after the write, and + * (b) it received rows read after the write. + * The self-fetching cases also assert (c): the write costs exactly ONE refetch + * of that view (no refetch storm). * - * The stand-ins below render the props a real data-fed renderer reads (`data`) - * and carry an instance id from a `useState` initializer, which runs once per - * mount: a changed id IS a remount. + * The stand-ins carry an instance id from a `useState` initializer, which runs + * once per mount: a changed id IS a remount. The self-fetching stand-in reads + * the REAL bus hook and queries on it exactly as `ObjectGrid` / `ObjectGantt` / + * `ObjectChart` now do — each renderer's own pin + * (`*.invalidationRefetch-10035.test.tsx` in its package) proves the real + * component does that; this file proves the HOST keeps them mounted and tells + * the bus. */ import * as React from 'react'; import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { render, screen, waitFor, act, cleanup } from '@testing-library/react'; +import { render, screen, waitFor, act, cleanup, fireEvent, within } from '@testing-library/react'; +import { useDataInvalidation } from '@object-ui/react'; import { ObjectView } from '../ObjectView'; import type { ObjectViewSchema, DataSourceMutationEvent } from '@object-ui/types'; @@ -58,10 +60,34 @@ function useInstanceId(): number { return id; } +/** + * A view that queries for itself and refetches on the bus — the idiom the + * three real self-fetching renderers follow. Its own reads go through + * `selfQuery`, so they are counted apart from the host's non-grid fetch. + */ +function SelfFetchingStandIn({ renderedType, objectName, dataSource }: any) { + const id = useInstanceId(); + const nonce = useDataInvalidation(objectName); + const [rows, setRows] = React.useState(-1); + React.useEffect(() => { + let live = true; + void dataSource.selfQuery().then((n: number) => { + if (live) setRows(n); + }); + return () => { + live = false; + }; + }, [dataSource, objectName, nonce]); + return
; +} + vi.mock('@object-ui/react', async (importOriginal) => { const ReactMod = await import('react'); - function ViewStandIn({ schema, data }: any) { + function ViewStandIn({ schema, data, dataSource }: any) { const id = useInstanceId(); + if (schema?.type === 'object-gantt' || schema?.type === 'object-chart') { + return ; + } return (
{ ...(await importOriginal>()), SchemaRenderer: ViewStandIn, SchemaRendererContext: ReactMod.createContext(null), - subscribeDataChanges: () => () => {}, - notifyDataChanged: () => {}, }; }); vi.mock('@object-ui/plugin-grid', async (importOriginal) => { - function GridStandIn() { - const id = useInstanceId(); - return
; + function GridStandIn({ schema, dataSource, onDelete }: any) { + return ( + <> + +