diff --git a/.changeset/10664-data-table-filter-bar.md b/.changeset/10664-data-table-filter-bar.md new file mode 100644 index 0000000000..5a2d647767 --- /dev/null +++ b/.changeset/10664-data-table-filter-bar.md @@ -0,0 +1,8 @@ +--- +'@object-ui/plugin-dashboard': patch +--- + +The dashboard's `object-data-table` reads its rows once per mount, already expanded, and a dashboard filter re-reads its options when `optionsFrom.filter` changes (objectui#10664). + +- `object-data-table` loaded the object definition in an effect of its own and listed it among the fetch effect's dependencies, so every object-bound mount issued two `find` calls, the first without `$expand`. It now reads the definition through `useSettledSchema` from `@object-ui/react` and its query waits until that read settles; a failed read, or an adapter with no `getObjectSchema`, still loads the rows, unexpanded. Its expansion also depends on `columns`, which did not re-run the read: adding a lookup column to a mounted table now re-reads with the new `$expand`, and relabelling a column does not. +- The filter bar's option list sends `optionsFrom.filter` on both of its reads, the dataset query and the record fallback, but did not re-read when only that filter changed. It now does, and an equal filter in a new object does not re-read. diff --git a/.changeset/10664-map-reads-once.md b/.changeset/10664-map-reads-once.md new file mode 100644 index 0000000000..3e0960bf7e --- /dev/null +++ b/.changeset/10664-map-reads-once.md @@ -0,0 +1,7 @@ +--- +'@object-ui/plugin-map': patch +--- + +An `object-map` block reads its records once per mount, and that read already carries the lookup expansion (objectui#10664). + +The map loaded the object definition in an effect of its own and listed it among the fetch effect's dependencies. The definition lands after the first query, so every mount issued two `find` calls, the first without `$expand`. Switching the bound object also sent the new object's query with the previous object's expansion. The map now reads the definition through `useSettledSchema` from `@object-ui/react`, and its object query waits until that read settles, the way `object-timeline` (objectui#7895) and `object-gallery` (objectui#7903) already do. A definition read that fails, or an adapter with no `getObjectSchema`, settles with no definition, so the map still loads, unexpanded. Host rows (`data`) and an inline `value` set are not held. diff --git a/.changeset/10664-object-view-table-sort.md b/.changeset/10664-object-view-table-sort.md new file mode 100644 index 0000000000..fccbcc5d7d --- /dev/null +++ b/.changeset/10664-object-view-table-sort.md @@ -0,0 +1,7 @@ +--- +'@object-ui/plugin-view': patch +--- + +The object view's own read for a non-grid view (calendar, kanban and the other views it fetches for) re-reads when `table.sort` changes (objectui#10664). + +That read falls back to `table.sort` for its `$orderby` when neither the named view nor the active view declares a sort, but a change to `table.sort` alone did not re-run it, so the view kept the old order. It now keys the read on that sort's content, so an equal sort in a new array does not re-read. diff --git a/.changeset/10664-repeater-picker-sort-key.md b/.changeset/10664-repeater-picker-sort-key.md new file mode 100644 index 0000000000..a0740fc644 --- /dev/null +++ b/.changeset/10664-repeater-picker-sort-key.md @@ -0,0 +1,7 @@ +--- +'@object-ui/components': patch +--- + +`element:repeater` and `element:record_picker` re-read their rows when the sort they send changes (objectui#10664, folding objectui#10665). + +Both blocks put their sort on `$orderby` (the repeater's `properties.sort`; the picker's `properties.sort` or its `dataSource` binding's `sort`), but neither re-ran its read when only the sort changed, so a bound sort control or a live preview kept showing the old order. Each now keys its read on the sort's content, the way it already keys on the filter's, so an equal sort in a new array does not re-read. diff --git a/packages/components/src/renderers/basic/__tests__/data-list.sortKey-10664.test.tsx b/packages/components/src/renderers/basic/__tests__/data-list.sortKey-10664.test.tsx new file mode 100644 index 0000000000..744ff4ea48 --- /dev/null +++ b/packages/components/src/renderers/basic/__tests__/data-list.sortKey-10664.test.tsx @@ -0,0 +1,93 @@ +/** + * 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#10664 (folded from objectui#10665) — `element:repeater` re-reads + * when its `sort` changes, and only then. + * + * The fetch effect lowers `properties.sort` onto `$orderby`, but its dependency + * list named `filterKey` and no sort at all. A mounted repeater whose sort + * changed (a bound sort control, a live designer preview) kept the old order + * until the object, the filter, the limit or a data-invalidation event re-ran + * the read. The sort is now keyed the way `filterKey` keys the filter: by + * CONTENT, so a new array with the same entries is not a change. + * + * Both halves are pinned. The CONTROL (an equal sort in a fresh array) is green + * before the fix and after it; it goes red for a fix that keys on the array's + * identity instead of its content (AGENTS.md #10: a key is a primitive or a + * structurally stable value). + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import * as React from 'react'; +import { render, screen, waitFor, act, cleanup } from '@testing-library/react'; +import { AdapterCtx, SchemaRenderer } from '@object-ui/react'; +// Registers every `element:*` renderer at module scope, not in a hook +// (object-ui/no-dynamic-import-in-test-hook, objectui#3010). +import '../../../renderers'; + +afterEach(cleanup); + +const settle = () => act(() => new Promise((resolve) => setTimeout(resolve, 50))); + +const BY_NAME_ASC = [{ field: 'name', order: 'asc' }]; +const BY_NAME_DESC = [{ field: 'name', order: 'desc' }]; + +type HostHandle = { setSort: (sort: unknown) => void }; + +/** Holds the repeater's `sort` as state and rebuilds the node on every change. */ +const Host = React.forwardRef(function Host({ adapter }, ref) { + const [sort, setSort] = React.useState(BY_NAME_ASC); + React.useImperativeHandle(ref, () => ({ setSort }), []); + const schema = React.useMemo( + () => ({ type: 'element:repeater', id: 'rep', properties: { object: 'contact', fields: ['name'], sort } }), + [sort], + ); + return ( + + + + ); +}); + +function mount() { + const adapter = { find: vi.fn(async () => ({ data: [{ id: 'r1', name: 'Ada' }], total: 1 })) }; + const host = React.createRef(); + render(); + return { adapter, host }; +} + +/** The `$orderby` of every read the repeater issued, in order. */ +const orderbys = (adapter: { find: { mock: { calls: any[][] } } }) => + adapter.find.mock.calls.map((c) => c[1]?.$orderby); + +describe('element:repeater keys its read on the sort it sends (objectui#10664)', () => { + it('SUBJECT: a changed sort re-reads, with the new `$orderby`', async () => { + const { adapter, host } = mount(); + await waitFor(() => expect(screen.getByTestId('repeater')).toBeInTheDocument()); + await settle(); + expect(orderbys(adapter)).toEqual([BY_NAME_ASC]); + + await act(async () => { host.current!.setSort(BY_NAME_DESC); }); + await settle(); + + expect(orderbys(adapter), 'the changed sort never reached a read').toEqual([BY_NAME_ASC, BY_NAME_DESC]); + }); + + it('CONTROL: an equal sort in a fresh array does not re-read', async () => { + const { adapter, host } = mount(); + await waitFor(() => expect(screen.getByTestId('repeater')).toBeInTheDocument()); + await settle(); + const atRest = adapter.find.mock.calls.length; + + await act(async () => { host.current!.setSort([{ field: 'name', order: 'asc' }]); }); + await settle(); + + expect(adapter.find.mock.calls.length, 'an equal sort re-read the list').toBe(atRest); + }); +}); diff --git a/packages/components/src/renderers/basic/__tests__/record-picker.sortKey-10664.test.tsx b/packages/components/src/renderers/basic/__tests__/record-picker.sortKey-10664.test.tsx new file mode 100644 index 0000000000..c7118b146a --- /dev/null +++ b/packages/components/src/renderers/basic/__tests__/record-picker.sortKey-10664.test.tsx @@ -0,0 +1,101 @@ +/** + * 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#10664 (the census row for `element:record_picker`) — the picker + * re-reads its options when the `sort` it sends changes, and only then. + * + * Same defect as `element:repeater` in the sibling file: the fetch effect puts + * `sort` on `$orderby` (the flat `properties.sort`, or a `dataSource` binding's + * `sort`, which replaces it), but its dependency list named `filterKey` and no + * sort. A changed sort kept the old option order until something else re-ran + * the read. The sort is now keyed by CONTENT, the way `filterKey` keys the + * filter. + * + * The CONTROL (an equal sort in a fresh array) is green before and after; it + * goes red for a fix that keys on the array's identity (AGENTS.md #10). + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import * as React from 'react'; +import { render, act, cleanup, waitFor } from '@testing-library/react'; +import { AdapterCtx, SchemaRenderer } from '@object-ui/react'; +// Registers `element:record_picker` at module scope, not in a hook +// (object-ui/no-dynamic-import-in-test-hook, objectui#3010). +import '../../../renderers'; + +afterEach(cleanup); + +const settle = () => act(() => new Promise((resolve) => setTimeout(resolve, 50))); + +const BY_NAME_ASC = [{ field: 'name', order: 'asc' }]; +const BY_NAME_DESC = [{ field: 'name', order: 'desc' }]; + +type Where = 'properties' | 'binding'; +type HostHandle = { setSort: (sort: unknown) => void }; + +/** + * Holds the picker's `sort` as state and rebuilds the node on every change. + * `where` puts it on the flat `properties.sort` or on a `dataSource` binding + * (no `view`, so nothing waits on a saved-view read). + */ +const Host = React.forwardRef(function Host({ adapter, where }, ref) { + const [sort, setSort] = React.useState(BY_NAME_ASC); + React.useImperativeHandle(ref, () => ({ setSort }), []); + const schema = React.useMemo( + () => + where === 'properties' + ? { type: 'element:record_picker', id: 'picker', properties: { object: 'account', sort } } + : { type: 'element:record_picker', id: 'picker', properties: {}, dataSource: { object: 'account', sort } }, + [sort, where], + ); + return ( + + + + ); +}); + +function mount(where: Where) { + const adapter = { find: vi.fn(async () => ({ data: [{ id: 'a1', name: 'Acme' }], total: 1 })), getObjectSchema: vi.fn() }; + const host = React.createRef(); + render(); + return { adapter, host }; +} + +/** The `$orderby` of every read the picker issued, in order. */ +const orderbys = (adapter: { find: { mock: { calls: any[][] } } }) => + adapter.find.mock.calls.map((c) => c[1]?.$orderby); + +describe('element:record_picker keys its read on the sort it sends (objectui#10664)', () => { + for (const where of ['properties', 'binding'] as const) { + it(`SUBJECT: a changed sort on the ${where} re-reads, with the new \`$orderby\``, async () => { + const { adapter, host } = mount(where); + await waitFor(() => expect(adapter.find).toHaveBeenCalled()); + await settle(); + expect(orderbys(adapter)).toEqual([BY_NAME_ASC]); + + await act(async () => { host.current!.setSort(BY_NAME_DESC); }); + await settle(); + + expect(orderbys(adapter), 'the changed sort never reached a read').toEqual([BY_NAME_ASC, BY_NAME_DESC]); + }); + } + + it('CONTROL: an equal sort in a fresh array does not re-read', async () => { + const { adapter, host } = mount('properties'); + await waitFor(() => expect(adapter.find).toHaveBeenCalled()); + await settle(); + const atRest = adapter.find.mock.calls.length; + + await act(async () => { host.current!.setSort([{ field: 'name', order: 'asc' }]); }); + await settle(); + + expect(adapter.find.mock.calls.length, 'an equal sort re-read the options').toBe(atRest); + }); +}); diff --git a/packages/components/src/renderers/basic/data-list.tsx b/packages/components/src/renderers/basic/data-list.tsx index e37dc5e792..4fa38e3b62 100644 --- a/packages/components/src/renderers/basic/data-list.tsx +++ b/packages/components/src/renderers/basic/data-list.tsx @@ -111,6 +111,10 @@ function RepeaterRenderer({ schema }: { schema: any }) { const [loading, setLoading] = React.useState(true); const [error, setError] = React.useState(null); const filterKey = React.useMemo(() => (props.filter ? JSON.stringify(props.filter) : ''), [props.filter]); + // objectui#10664 — the sort reaches `$orderby` below, so the fetch effect + // keys on it, by CONTENT the way `filterKey` keys the filter: a fresh array + // with the same entries is not a change (AGENTS.md #10). + const sortKey = React.useMemo(() => (props.sort ? JSON.stringify(props.sort) : ''), [props.sort]); const cols: RepeaterColumn[] = React.useMemo( () => (props.fields ?? []).map((f) => (typeof f === 'string' ? { field: f } : f)), @@ -161,7 +165,7 @@ function RepeaterRenderer({ schema }: { schema: any }) { cancelled = true; }; // eslint-disable-next-line react-hooks/exhaustive-deps - }, [adapter, props.object, filterKey, props.limit, invalidationNonce]); + }, [adapter, props.object, filterKey, sortKey, props.limit, invalidationNonce]); if (loading) return

Loading…

; if (error) return

{error}

; diff --git a/packages/components/src/renderers/basic/record-picker.tsx b/packages/components/src/renderers/basic/record-picker.tsx index 331ba9a200..b67a00c205 100644 --- a/packages/components/src/renderers/basic/record-picker.tsx +++ b/packages/components/src/renderers/basic/record-picker.tsx @@ -120,6 +120,10 @@ function ElementRecordPickerRenderer({ schema }: { schema: any }) { const [loading, setLoading] = React.useState(true); const [error, setError] = React.useState(null); const filterKey = React.useMemo(() => (filter ? JSON.stringify(filter) : ''), [filter]); + // objectui#10664 — the sort reaches `$orderby` below, so the fetch effect + // keys on it, by CONTENT the way `filterKey` keys the filter: a fresh array + // with the same entries is not a change (AGENTS.md #10). + const sortKey = React.useMemo(() => (sort ? JSON.stringify(sort) : ''), [sort]); React.useEffect(() => { let cancelled = false; @@ -156,7 +160,7 @@ function ElementRecordPickerRenderer({ schema }: { schema: any }) { cancelled = true; }; // eslint-disable-next-line react-hooks/exhaustive-deps - }, [adapter, object, filterKey, limit]); + }, [adapter, object, filterKey, sortKey, limit]); // Reflect the bound variable's value back into the control. When a variable // targets this picker we stay controlled for its whole lifetime (empty string diff --git a/packages/plugin-dashboard/src/DashboardFilterBar.tsx b/packages/plugin-dashboard/src/DashboardFilterBar.tsx index 8739a020e1..2e8546f22d 100644 --- a/packages/plugin-dashboard/src/DashboardFilterBar.tsx +++ b/packages/plugin-dashboard/src/DashboardFilterBar.tsx @@ -274,6 +274,10 @@ function SelectFilter({ def, value, onChange, dataSource }: { def: DashboardFilt // total failure (same tolerance style as DatasetWidget's option-color // fetch). const from = def.optionsFrom; + // objectui#10664 — both reads below send `from.filter`, so the effect keys on + // it, by CONTENT: an equal filter in a fresh object is not a change + // (AGENTS.md #10). + const optionsFilterKey = JSON.stringify(from?.filter ?? null); useEffect(() => { if (!from || !dataSource) return; let cancelled = false; @@ -350,7 +354,7 @@ function SelectFilter({ def, value, onChange, dataSource }: { def: DashboardFilt } return () => { cancelled = true; }; // eslint-disable-next-line react-hooks/exhaustive-deps - }, [from?.object, from?.valueField, from?.labelField, dataSource]); + }, [from?.object, from?.valueField, from?.labelField, optionsFilterKey, dataSource]); const localizedOptions = useMemo(() => { // `def.options` is already normalized to `{ value, label }` PAIRS by diff --git a/packages/plugin-dashboard/src/ObjectDataTable.tsx b/packages/plugin-dashboard/src/ObjectDataTable.tsx index f3faa92669..c6830a3141 100644 --- a/packages/plugin-dashboard/src/ObjectDataTable.tsx +++ b/packages/plugin-dashboard/src/ObjectDataTable.tsx @@ -7,7 +7,7 @@ */ import React, { useState, useEffect, useContext, useMemo, useCallback } from 'react'; -import { useDataScope, SchemaRendererContext, SchemaRenderer, useFilterScope } from '@object-ui/react'; +import { useDataScope, SchemaRendererContext, SchemaRenderer, useFilterScope, useSettledSchema } from '@object-ui/react'; import { extractRecords, isDrillEnabled, @@ -625,7 +625,6 @@ export const ObjectDataTable: React.FC = ({ schema, dataSo if (b && b !== 'dashboard.noDataSourceFor') noDataSourceLabel = b; const [fetchedData, setFetchedData] = useState([]); - const [objectSchema, setObjectSchema] = useState(null); // Start in loading state when we will fetch from a dataSource, so the // "No data available" empty state doesn't flash on slow networks before // the fetch effect runs and flips loading to true. @@ -657,6 +656,37 @@ export const ObjectDataTable: React.FC = ({ schema, dataSo // exists by the time render reaches the fetch effect below (objectui#7230). const perms = usePermissions(); + /** + * The object definition, and whether the read for THIS object has SETTLED: + * one piece of state, through the shared hook (objectui#10664). + * + * It sat in a local `useState` fed by its own effect and was listed in the + * fetch effect's dependencies below, so every object-bound mount queried + * twice: once before the definition landed (`computeLookupExpand` returns + * nothing without a field map, so no `$expand`), once after. The same shape + * `ObjectGallery` (objectui#7903) left, and the gate below is theirs. + * + * `dataSource` is passed on every path, as `ObjectGallery` passes it: the + * column headers and select-option cells read the definition on bound and + * inline rows too, where no query waits on it. The key is `schema.objectName`, + * the object the query names. + * + * ⚠️ The gate is only safe because the hook SETTLES ON EVERY EXIT + * (objectui#7232): no source, no `getObjectSchema`, no name, and a read that + * threw. The replaced effect returned without settling on the first three. + */ + const { ready: objectSchemaReady, def: objectSchema } = useSettledSchema( + schema.objectName ?? '', + dataSource, + ); + + // The expansion the query below sends, as a CONTENT key (objectui#10664). The + // query reads `schema.columns` (an explicit whitelist decides which relations + // expand) and the dependency list did not name it, so a column added on a + // mounted widget never reached `$expand`. Keyed on the expansion rather than + // on the columns, so a relabelled column is not a change. + const lookupExpandKey = computeLookupExpand(schema, objectSchema).join(','); + useEffect(() => { let isMounted = true; @@ -733,7 +763,20 @@ export const ObjectDataTable: React.FC = ({ schema, dataSo }; if (schema.objectName && !boundData && (!schema.data || schema.data.length === 0)) { - fetchData(); + // ⭐ objectui#10664: the object definition GATES this query; it does not + // refine it afterwards. `objectSchema` stays in the dependency list and + // the two are one mechanism: the dependency re-runs this effect when the + // definition lands, and this branch stops the first run from spending a + // query before it has. Removing either half restores the double read. + // + // Scoped to the branch that queries. The placeholder is held across the + // window: `loading` starts `true` here only on first mount, and a later + // object switch closes the gate with the previous object's rows in state. + if (!objectSchemaReady) { + if (isMounted) setLoading(true); + } else { + fetchData(); + } } else if (isMounted) { // We have inline / bound data and won't fetch — make sure loading is // cleared (matters when we lazily-initialized it to true). @@ -741,19 +784,9 @@ export const ObjectDataTable: React.FC = ({ schema, dataSo } return () => { isMounted = false; }; - }, [schema.objectName, dataSource, boundData, schema.data, schema.filter, objectSchema, filterScope, perms]); - - // Fetch object schema for column-header translation and select-option cell labels. - useEffect(() => { - let isMounted = true; - if (!dataSource || !schema.objectName || typeof dataSource.getObjectSchema !== 'function') { - return; - } - dataSource.getObjectSchema(schema.objectName) - .then((s: any) => { if (isMounted) setObjectSchema(s); }) - .catch(() => { /* schema lookup failure is non-fatal */ }); - return () => { isMounted = false; }; - }, [schema.objectName, dataSource]); + // `schema.columns` is read through `lookupExpandKey`, by content; see above. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [schema.objectName, dataSource, boundData, schema.data, schema.filter, objectSchemaReady, objectSchema, lookupExpandKey, filterScope, perms]); // Resolve data: bound data > static schema data > fetched data const rawData = boundData || schema.data || fetchedData; diff --git a/packages/plugin-dashboard/src/__tests__/DashboardFilterBar.optionsFilterKey-10664.test.tsx b/packages/plugin-dashboard/src/__tests__/DashboardFilterBar.optionsFilterKey-10664.test.tsx new file mode 100644 index 0000000000..552953568d --- /dev/null +++ b/packages/plugin-dashboard/src/__tests__/DashboardFilterBar.optionsFilterKey-10664.test.tsx @@ -0,0 +1,98 @@ +/** + * 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#10664 (the census row for the dashboard filter bar) — a filter's + * option list is re-read when its `optionsFrom.filter` changes, and only then. + * + * The options effect sends `optionsFrom.filter` on both of its reads (the + * dataset query's `runtimeFilter` and the client-side fallback's `$filter`), + * but its dependency list named only `object`, `valueField`, `labelField` and + * the data source. A mounted bar whose option filter changed kept offering the + * previous filter's options. The filter is now keyed by CONTENT, so an equal + * filter in a fresh object is not a change (AGENTS.md #10). + * + * The CONTROL (an equal filter in a fresh object) is green before and after; it + * goes red for a fix that keys on the filter's identity. + */ + +import * as React from 'react'; +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, act, cleanup, waitFor } from '@testing-library/react'; +import { DashboardFilterBar } from '../DashboardFilterBar'; +import type { DashboardFilterDef } from '@object-ui/core'; + +afterEach(cleanup); + +const settle = () => act(() => new Promise((resolve) => setTimeout(resolve, 50))); + +const ACTIVE = { status: 'active' }; +const CHURNED = { status: 'churned' }; + +const defsFor = (filter: unknown): DashboardFilterDef[] => [ + { + name: 'industry', + field: 'industry', + type: 'select', + optionsFrom: { object: 'accounts', valueField: 'industry', labelField: 'industry', filter }, + } as DashboardFilterDef, +]; + +type Path = 'dataset' | 'fallback'; + +function makeDataSource(path: Path) { + const find = vi.fn().mockResolvedValue({ data: [{ industry: 'finance' }] }); + const queryDataset = + path === 'dataset' + ? vi.fn().mockResolvedValue({ rows: [{ industry: 'finance', option_count: 1 }] }) + : vi.fn().mockRejectedValue(new Error('datasets unsupported')); + return { find, queryDataset }; +} + +/** The filter each option read carried, whichever of the two reads served it. */ +const filtersSent = (ds: ReturnType, path: Path) => + path === 'dataset' + ? ds.queryDataset.mock.calls.map((c: any[]) => c[1]?.runtimeFilter) + : ds.find.mock.calls.map((c: any[]) => c[1]?.$filter); + +function mount(path: Path) { + const ds = makeDataSource(path); + const view = render( + , + ); + const rerender = (filter: unknown) => + view.rerender(); + return { ds, rerender }; +} + +describe('the dashboard filter bar keys its option read on the filter it sends (objectui#10664)', () => { + for (const path of ['dataset', 'fallback'] as const) { + it(`SUBJECT: a changed \`optionsFrom.filter\` re-reads the options (${path} read)`, async () => { + const { ds, rerender } = mount(path); + await waitFor(() => expect(filtersSent(ds, path)).toHaveLength(1)); + await settle(); + expect(filtersSent(ds, path)).toEqual([ACTIVE]); + + await act(async () => { rerender(CHURNED); }); + await settle(); + + expect(filtersSent(ds, path), 'the changed filter never reached an option read').toEqual([ACTIVE, CHURNED]); + }); + } + + it('CONTROL: an equal filter in a fresh object does not re-read', async () => { + const { ds, rerender } = mount('dataset'); + await waitFor(() => expect(ds.queryDataset).toHaveBeenCalledTimes(1)); + await settle(); + + await act(async () => { rerender({ status: 'active' }); }); + await settle(); + + expect(ds.queryDataset, 'an equal filter re-read the options').toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/plugin-dashboard/src/__tests__/ObjectDataTable.expandFls-7230.test.tsx b/packages/plugin-dashboard/src/__tests__/ObjectDataTable.expandFls-7230.test.tsx index cd44589cb0..052d6643d5 100644 --- a/packages/plugin-dashboard/src/__tests__/ObjectDataTable.expandFls-7230.test.tsx +++ b/packages/plugin-dashboard/src/__tests__/ObjectDataTable.expandFls-7230.test.tsx @@ -46,7 +46,7 @@ */ import React from 'react'; import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { render, waitFor, cleanup } from '@testing-library/react'; +import { render, waitFor, cleanup, act } from '@testing-library/react'; /** Stable stub identity — `perms` rides the fetch effect's dependency list. */ const { permsStub, state } = vi.hoisted(() => { @@ -116,15 +116,17 @@ async function expandFor(schemaExtra: Record): Promise, ); - // ⚠️ Wait for the SCHEMA-DEPENDENT query, not the first one. `objectSchema` - // is component state and sits in the fetch effect's dependency list, so the - // widget issues one `find` before the schema lands (necessarily carrying no - // `$expand` — `computeLookupExpand` returns `[]` without a field map) and - // re-issues it after. Reading the first call would report "no expansion" for - // every case and turn this whole file green for the wrong reason. + // ⚠️ Read the SCHEMA-DEPENDENT query. Since objectui#10664 the widget's query + // waits on the settled definition (`useSettledSchema`), so a mount issues ONE + // `find` and it is that one. The count is asserted, not assumed: an ungated + // widget issues an unexpanded `find` first (`computeLookupExpand` returns `[]` + // without a field map), and reading it would report "no expansion" for every + // case and turn this whole file green for the wrong reason. await waitFor(() => expect(ds.getObjectSchema).toHaveBeenCalled()); - await waitFor(() => expect(ds.find.mock.calls.length).toBeGreaterThan(1)); - return (ds.find.mock.calls.at(-1)?.[1]?.$expand ?? []) as string[]; + await waitFor(() => expect(ds.find).toHaveBeenCalled()); + await act(() => new Promise((resolve) => setTimeout(resolve, 50))); + expect(ds.find, 'more than one read per mount: the settled-definition gate is gone').toHaveBeenCalledTimes(1); + return (ds.find.mock.calls[0]?.[1]?.$expand ?? []) as string[]; } const AUTHORED_COLUMNS = { diff --git a/packages/plugin-dashboard/src/__tests__/ObjectDataTable.fetchGate-10664.test.tsx b/packages/plugin-dashboard/src/__tests__/ObjectDataTable.fetchGate-10664.test.tsx new file mode 100644 index 0000000000..a1959e718e --- /dev/null +++ b/packages/plugin-dashboard/src/__tests__/ObjectDataTable.fetchGate-10664.test.tsx @@ -0,0 +1,180 @@ +/** + * 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#10664 (the census row for `object-data-table`) — the widget issues + * ONE `find` per mount, that read already carries the lookup `$expand`, and a + * column change that moves the expansion re-reads. + * + * Same shape as `object-map` on the card: the definition sat in a local + * `useState` fed by its own effect and was listed in the fetch effect's + * dependencies, so the query ran once before it landed (`computeLookupExpand` + * returns nothing without a field map) and once after. The widget now reads the + * definition through the shared `useSettledSchema` and its query waits on it, + * the objectui#7895 / objectui#7903 gate. + * + * The second half is the other direction of the same mismatch: the query reads + * `schema.columns` (the explicit whitelist decides which relations expand), and + * the dependency list did not name it. It now keys on the expansion those + * columns produce, so a relabelled column (same fields) is not a change. + * + * Rendered through the real `SchemaRenderer` and this package's registration. + * The two SETTLES cases are the trap a gate can introduce, pinned: a widget + * whose definition read throws, or whose adapter has no `getObjectSchema`, must + * still load its rows. + */ +import React from 'react'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, act, cleanup, screen, waitFor } from '@testing-library/react'; +import { SchemaRenderer, SchemaRendererProvider } from '@object-ui/react'; +// Registers `object-data-table`. +import '../index'; + +beforeEach(() => { + // Best-effort metadata probes are not what these cases are about. + vi.stubGlobal('fetch', vi.fn(async () => ({ ok: true, status: 200, json: async () => ({}), text: async () => '{}' }))); +}); +afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); + cleanup(); +}); + +const settle = () => act(() => new Promise((resolve) => setTimeout(resolve, 150))); + +const DEFINITION = { + name: 'opportunity', + fields: { + name: { type: 'text', label: 'Name' }, + account: { type: 'lookup', reference: 'accounts', label: 'Account' }, + owner_dept: { type: 'master_detail', reference: 'departments', label: 'Dept' }, + }, +}; + +function makeDataSource(opts: { hold?: boolean; schema?: 'reject' | 'absent' } = {}) { + let release!: () => void; + const held = new Promise((resolve) => { release = resolve; }); + const sequence: string[] = []; + const ds: any = { + sequence, + release: () => release(), + find: vi.fn(async () => { + sequence.push('find'); + return { data: [{ id: 'o1', name: 'Acme' }], total: 1 }; + }), + findOne: vi.fn(), + create: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + }; + if (opts.schema !== 'absent') { + ds.getObjectSchema = vi.fn(async () => { + if (opts.hold) await held; + if (opts.schema === 'reject') throw new Error('metadata unavailable'); + sequence.push('schema'); + return DEFINITION; + }); + } + return ds; +} + +const expands = (ds: any) => ds.find.mock.calls.map((c: any[]) => c[1]?.$expand ?? null); + +type HostHandle = { setColumns: (columns: unknown) => void }; + +/** Holds the widget's `columns` as state and rebuilds the node on every change. */ +const Host = React.forwardRef(function Host({ ds, columns: initial }, ref) { + const [columns, setColumns] = React.useState(initial); + React.useImperativeHandle(ref, () => ({ setColumns }), []); + const schema = React.useMemo( + () => ({ type: 'object-data-table', objectName: 'opportunity', ...(columns ? { columns } : {}) }), + [columns], + ); + return ( + + + + ); +}); + +describe('object-data-table reads once per mount, expanded, and keys on its expansion (objectui#10664)', () => { + it('SUBJECT: one `find` per mount, and it carries the lookup `$expand`', async () => { + const ds = makeDataSource(); + render(); + await waitFor(() => expect(screen.getByText('Acme')).toBeInTheDocument()); + await settle(); + + expect(expands(ds), 'a mount read the table more than once, or the first read went out unexpanded').toEqual([ + ['account', 'owner_dept'], + ]); + }); + + it('SUBJECT: the query WAITS for the definition, then goes out once', async () => { + const ds = makeDataSource({ hold: true }); + render(); + await settle(); + expect(ds.find, 'the widget queried before its definition settled').not.toHaveBeenCalled(); + + await act(async () => { ds.release(); }); + await waitFor(() => expect(screen.getByText('Acme')).toBeInTheDocument()); + await settle(); + + expect(ds.sequence).toEqual(['schema', 'find']); + }); + + it('SUBJECT: a column change that moves the expansion re-reads, with the new `$expand`', async () => { + const ds = makeDataSource(); + const host = React.createRef(); + render(); + await waitFor(() => expect(screen.getByText('Acme')).toBeInTheDocument()); + await settle(); + expect(expands(ds)).toEqual([['account']]); + + await act(async () => { + host.current!.setColumns([{ field: 'name' }, { field: 'account' }, { field: 'owner_dept' }]); + }); + await settle(); + + expect(expands(ds), 'the new column never reached a read').toEqual([['account'], ['account', 'owner_dept']]); + }); + + it('CONTROL: a relabelled column (same fields) does not re-read', async () => { + const ds = makeDataSource(); + const host = React.createRef(); + render(); + await waitFor(() => expect(screen.getByText('Acme')).toBeInTheDocument()); + await settle(); + const atRest = ds.find.mock.calls.length; + + await act(async () => { + host.current!.setColumns([{ field: 'name', label: 'Deal' }, { field: 'account', label: 'Customer' }]); + }); + await settle(); + + expect(ds.find.mock.calls.length, 'a relabel re-read the table').toBe(atRest); + }); + + it('SETTLES ON A THROWN READ: a failed definition read still loads the rows, unexpanded', async () => { + vi.spyOn(console, 'error').mockImplementation(() => {}); + const ds = makeDataSource({ schema: 'reject' }); + render(); + await waitFor(() => expect(screen.getByText('Acme')).toBeInTheDocument()); + await settle(); + + expect(expands(ds)).toEqual([null]); + }); + + it('SETTLES WITH NO SOURCE: an adapter without `getObjectSchema` still loads the rows', async () => { + const ds = makeDataSource({ schema: 'absent' }); + render(); + await waitFor(() => expect(screen.getByText('Acme')).toBeInTheDocument()); + await settle(); + + expect(expands(ds)).toEqual([null]); + }); +}); diff --git a/packages/plugin-map/src/ObjectMap.fetchGate-10664.test.tsx b/packages/plugin-map/src/ObjectMap.fetchGate-10664.test.tsx new file mode 100644 index 0000000000..bbf8b51ffc --- /dev/null +++ b/packages/plugin-map/src/ObjectMap.fetchGate-10664.test.tsx @@ -0,0 +1,216 @@ +/** + * 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#10664 — `object-map` issues ONE `find` per mount, and that first + * read already carries the lookup `$expand`. + * + * The fetch effect listed the object definition among its dependencies while a + * separate effect loaded that definition into a local `useState`. The + * definition lands after the first query, so every mount read twice: once with + * `buildExpandFields` seeing no fields (no `$expand` at all), and once after. + * Without a lookup field the two reads were the same query. `object-timeline` + * (objectui#7895) and `object-gallery` (objectui#7903) gate the same query on a + * SETTLED definition through the shared `useSettledSchema`; the map now does + * too, on the branch that issues the object query. + * + * Rendered through the real `SchemaRenderer` and this package's own + * registration, over a fake data source that counts reads. + * + * ⚠️ The gate is only safe because `useSettledSchema` settles on EVERY exit: a + * read that throws, and an adapter with no `getObjectSchema`, both settle with + * no definition. The two cases for them are the trap, pinned: a map that never + * loads is the failure a gate can introduce. + * + * The CONTROL is a definition that changes after mount. It is green before and + * after the gate, and it goes red for a gate that forgot to keep the definition + * in the dependency list: then the re-read would never carry the new expansion. + */ +import React from 'react'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, act, cleanup, screen, waitFor } from '@testing-library/react'; +import { SchemaRenderer, SchemaRendererProvider } from '@object-ui/react'; + +// No WebGL in the test env: the same stub the sibling ObjectMap tests use. +vi.mock('react-map-gl/maplibre', () => ({ + default: ({ children }: any) =>
{children}
, + Map: ({ children }: any) =>
{children}
, + NavigationControl: () =>
, + Marker: ({ children }: any) =>
{children}
, + Popup: ({ children }: any) =>
{children}
, +})); + +// Registers `object-map` through this package's own entry. +import './index'; + +beforeEach(() => { + // Best-effort metadata probes are not what these cases are about. + vi.stubGlobal('fetch', vi.fn(async () => ({ ok: true, status: 200, json: async () => ({}), text: async () => '{}' }))); +}); +afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); + cleanup(); +}); + +const settle = () => act(() => new Promise((resolve) => setTimeout(resolve, 150))); + +const BASE_FIELDS = { name: { type: 'text' }, lat: { type: 'number' }, lng: { type: 'number' } }; +const WITH_OWNER = { name: 'store', fields: { ...BASE_FIELDS, owner: { type: 'lookup', reference: 'user' } } }; +const WITH_OWNER_AND_REGION = { + name: 'store', + fields: { ...BASE_FIELDS, owner: { type: 'lookup', reference: 'user' }, region: { type: 'lookup', reference: 'region' } }, +}; +const NO_LOOKUP = { name: 'store', fields: BASE_FIELDS }; +const DEPOT_FIELDS = { name: 'depot', fields: { ...BASE_FIELDS, manager: { type: 'lookup', reference: 'user' } } }; + +type Def = { name: string; fields: Record }; + +/** + * `definitions` answers `getObjectSchema` per object. `hold` makes that read + * wait until `release()`; `schema: 'reject'` makes it throw; `schema: 'absent'` + * gives the adapter no `getObjectSchema` at all. + */ +function makeDataSource( + definitions: Record, + opts: { hold?: boolean; schema?: 'reject' | 'absent' } = {}, +) { + let release!: () => void; + const held = new Promise((resolve) => { release = resolve; }); + const sequence: string[] = []; + const ds: any = { + sequence, + release: () => release(), + find: vi.fn(async (object: string) => { + sequence.push(`find:${object}`); + return { data: [{ id: '1', name: 'Depot', lat: 10, lng: 20 }], total: 1 }; + }), + findOne: vi.fn(), + create: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + }; + if (opts.schema !== 'absent') { + ds.getObjectSchema = vi.fn(async (object: string) => { + if (opts.hold) await held; + if (opts.schema === 'reject') throw new Error('metadata unavailable'); + sequence.push(`schema:${object}`); + return definitions[object]; + }); + } + return ds; +} + +/** The `find` calls this map issued for `object`, as their query params. */ +const findsFor = (ds: any, object: string): any[] => + ds.find.mock.calls.filter((c: any[]) => c[0] === object).map((c: any[]) => c[1]); + +const MAP = { latitudeField: 'lat', longitudeField: 'lng', titleField: 'name' }; +const blockFor = (objectName: string) => ({ type: 'object-map', objectName, map: MAP }); + +type HostHandle = { setDataSource: (ds: any) => void; setObjectName: (name: string) => void }; + +/** Holds the adapter and the bound object as state, so a test can change either in one commit. */ +const Host = React.forwardRef(function Host(props, ref) { + const [ds, setDataSource] = React.useState(props.ds); + const [objectName, setObjectName] = React.useState(props.objectName ?? 'store'); + React.useImperativeHandle(ref, () => ({ setDataSource, setObjectName }), []); + const schema = React.useMemo(() => blockFor(objectName), [objectName]); + return ( + + + + ); +}); + +describe('object-map reads once per mount, expanded on the first read (objectui#10664)', () => { + it('SUBJECT: one `find` per mount, and it carries the lookup `$expand`', async () => { + const ds = makeDataSource({ store: WITH_OWNER }); + render(); + await waitFor(() => expect(screen.getAllByTestId('map-marker')).toHaveLength(1)); + await settle(); + + expect(findsFor(ds, 'store'), 'a mount read the store more than once').toHaveLength(1); + expect(findsFor(ds, 'store')[0].$expand, 'the first read went out unexpanded').toEqual(['owner']); + }); + + it('SUBJECT: without a lookup field it is still one `find` (the two reads were the same query)', async () => { + const ds = makeDataSource({ store: NO_LOOKUP }); + render(); + await waitFor(() => expect(screen.getAllByTestId('map-marker')).toHaveLength(1)); + await settle(); + + expect(findsFor(ds, 'store')).toHaveLength(1); + expect(findsFor(ds, 'store')[0].$expand).toBeUndefined(); + }); + + it('SUBJECT: the query WAITS for the definition, then goes out once', async () => { + const ds = makeDataSource({ store: WITH_OWNER }, { hold: true }); + render(); + await settle(); + expect(findsFor(ds, 'store'), 'the map queried before its definition settled').toHaveLength(0); + + await act(async () => { ds.release(); }); + await waitFor(() => expect(screen.getAllByTestId('map-marker')).toHaveLength(1)); + await settle(); + + expect(ds.sequence).toEqual(['schema:store', 'find:store']); + expect(findsFor(ds, 'store')[0].$expand).toEqual(['owner']); + }); + + it('SUBJECT: switching the bound object reads the new object once, with ITS expansion', async () => { + const ds = makeDataSource({ store: WITH_OWNER, depot: DEPOT_FIELDS }); + const host = React.createRef(); + render(); + await waitFor(() => expect(screen.getAllByTestId('map-marker')).toHaveLength(1)); + await settle(); + + await act(async () => { host.current!.setObjectName('depot'); }); + await waitFor(() => expect(screen.getAllByTestId('map-marker')).toHaveLength(1)); + await settle(); + + // Never the previous object's expansion on the new object's query. + expect(findsFor(ds, 'depot').map((q) => q.$expand ?? null)).toEqual([['manager']]); + }); + + it('SETTLES ON A THROWN READ: a failed definition read still loads the markers, unexpanded', async () => { + vi.spyOn(console, 'error').mockImplementation(() => {}); + const ds = makeDataSource({ store: WITH_OWNER }, { schema: 'reject' }); + render(); + await waitFor(() => expect(screen.getAllByTestId('map-marker')).toHaveLength(1)); + await settle(); + + expect(findsFor(ds, 'store')).toHaveLength(1); + expect(findsFor(ds, 'store')[0].$expand).toBeUndefined(); + }); + + it('SETTLES WITH NO SOURCE: an adapter without `getObjectSchema` still loads the markers', async () => { + const ds = makeDataSource({ store: WITH_OWNER }, { schema: 'absent' }); + render(); + await waitFor(() => expect(screen.getAllByTestId('map-marker')).toHaveLength(1)); + await settle(); + + expect(findsFor(ds, 'store')).toHaveLength(1); + }); + + it('CONTROL: a definition that changes after mount still re-reads, with the new expansion', async () => { + const first = makeDataSource({ store: WITH_OWNER }); + const host = React.createRef(); + render(); + await waitFor(() => expect(screen.getAllByTestId('map-marker')).toHaveLength(1)); + await settle(); + + const second = makeDataSource({ store: WITH_OWNER_AND_REGION }); + await act(async () => { host.current!.setDataSource(second); }); + await settle(); + + const reads = findsFor(second, 'store'); + expect(reads.length, 'the new definition never reached a read').toBeGreaterThan(0); + expect(reads[reads.length - 1].$expand).toEqual(['owner', 'region']); + }); +}); diff --git a/packages/plugin-map/src/ObjectMap.invalidationRefetch-10623.test.tsx b/packages/plugin-map/src/ObjectMap.invalidationRefetch-10623.test.tsx index 00fc7347a3..ebcd350fd1 100644 --- a/packages/plugin-map/src/ObjectMap.invalidationRefetch-10623.test.tsx +++ b/packages/plugin-map/src/ObjectMap.invalidationRefetch-10623.test.tsx @@ -19,11 +19,10 @@ * `MapGL` for a fetch, and a bus re-read that went through it would reset the * camera the user panned — see the comment on the nonce in `ObjectMap.tsx`. * - * ⚠️ Counted as a DELTA, not an absolute. This map issues two `find` calls on - * mount (the fetch effect keys on the object definition, which lands after the - * first query); that is recorded on objectui#10623 as a separate finding and is - * not this file's subject. What is pinned here is that one bus event adds - * exactly one query. + * ⚠️ Counted as a DELTA, not an absolute. How many `find` calls a mount issues + * is not this file's subject: it is pinned by `ObjectMap.fetchGate-10664.test.tsx` + * (objectui#10664 gated the query on a settled definition, so a mount reads + * once). What is pinned here is that one bus event adds exactly one query. * * Rendered through the real `SchemaRenderer` and this package's own * registration, over a fake data source that counts reads. The bare diff --git a/packages/plugin-map/src/ObjectMap.tsx b/packages/plugin-map/src/ObjectMap.tsx index 9f71dd7149..3c59cd6219 100644 --- a/packages/plugin-map/src/ObjectMap.tsx +++ b/packages/plugin-map/src/ObjectMap.tsx @@ -27,6 +27,7 @@ import { useNavigationOverlay, NonGridRowCeilingNote, useDataInvalidation, + useSettledSchema, } from '@object-ui/react'; import { NavigationOverlay, cn, useIsMobile } from '@object-ui/components'; import { usePermissions } from '@object-ui/permissions'; @@ -627,7 +628,6 @@ export const ObjectMap: React.FC = ({ * and the same footnote. This docblock used to say both paths were exempt. */ const [rowCeiling, setRowCeiling] = useState(null); - const [objectSchema, setObjectSchema] = useState(null); const [selectedMarkerId, setSelectedMarkerId] = useState(null); const [searchQuery, setSearchQuery] = useState(''); // Mobile UX (round 3) @@ -762,13 +762,40 @@ export const ObjectMap: React.FC = ({ /** * The object this map is BOUND to — the resolved record source's object when * it names one, else the schema's own `objectName` (objectui#7627, the - * objectui#6939 ladder). Hoisted to render scope so the metadata effect below - * reads one named value instead of re-deriving the ladder inline; it is a pure - * function of `dataProvider` / `dataObjectName` / `schema.objectName`, all of - * which that effect already depends on, so listing it adds no re-run. + * objectui#6939 ladder). Hoisted to render scope so the definition read below + * keys on one named value instead of re-deriving the ladder inline. */ const recordSourceObjectName = resolveRecordSourceObjectName(schema, dataConfig); + /** + * The object definition, and whether the read for THIS object has SETTLED: + * one piece of state, through the shared hook (objectui#10664). + * + * The map held the definition in a local `useState` fed by its own metadata + * effect, and listed it in the fetch effect's dependencies below. The + * definition lands after the first query, so every mount read twice: once + * with `buildExpandFields` seeing no fields (no `$expand`), once after. It is + * the shape `ObjectTimeline` (objectui#7895) and `ObjectGallery` + * (objectui#7903) left, and the gate below is theirs. + * + * The key is `recordSourceObjectName`, the object the replaced effect read. On + * the branch the gate holds (`dataProvider === 'object'`) it names the same + * object the query does, `dataObjectName`. + * + * An inline `value` set passes no source, as `ObjectCalendar` does: it has no + * definition to read, and the hook settles with none at once. Every other + * path keeps the read, host rows included, because the marker titles resolve + * from it (ADR-0079). + * + * ⚠️ The gate is only safe because the hook SETTLES ON EVERY EXIT + * (objectui#7232): no source, no `getObjectSchema`, no key, and a read that + * threw. The replaced effect returned without settling on all four. + */ + const { ready: objectSchemaReady, def: objectSchema } = useSettledSchema( + recordSourceObjectName ?? '', + hasInlineData ? undefined : dataSource, + ); + // Permissions context, read here rather than inside the fetch effect below: // an effect's DEPENDENCY ARRAY is evaluated during render, so `perms` has to // be a binding that already exists by the time this component's render @@ -818,9 +845,28 @@ export const ObjectMap: React.FC = ({ // Fetch data based on provider useEffect(() => { + // ⭐ objectui#10664: the object definition GATES the object query; it does + // not refine it afterwards. `objectSchema` stays in the dependency list and + // the two are one mechanism: the dependency re-runs this effect when the + // definition lands, and this line stops the first run from spending a query + // before it has. Removing either half restores the double read. + // + // Scoped to the branch that issues that query. Host rows and an inline + // `value` set build no expansion, so there is nothing for them to wait on. + // + // The gate closes only when the key moves, and on this branch the key is + // the queried object, so a closed gate always means a CHANGED query. The + // placeholder is held for the window: the rows in state answer the previous + // object, and the query this run would have issued goes through the loading + // gate anyway. + if (fetchesForItself && !objectSchemaReady) { + setLoading(true); + return; + } // Every input of the query below: this effect's dependency list, less the - // nonce. Compared value for value (`Object.is`), the way React compares - // the list itself. + // nonce, the readiness flag and `fetchesForItself` (a function of two + // members already in it). Compared value for value (`Object.is`), the way + // React compares the list itself. const query = [dataProp, dataProvider, dataObjectName, dataItems, dataSource, hasInlineData, schema.filter, schema.sort, objectSchema, perms] as const; const committed = committedQueryRef.current; const answersShownQuery = committed !== null && committed.length === query.length && query.every((v, i) => Object.is(v, committed[i])); @@ -958,8 +1004,8 @@ export const ObjectMap: React.FC = ({ // policy filters nothing, and `perms` is in this effect's dependency // list, so the expansion is rebuilt the moment the answer arrives. // - // `objectName` (checkField's target) and `objectSchema` (fetched - // keyed by `recordSourceObjectName`, the OTHER effect below) agree + // `objectName` (checkField's target) and `objectSchema` (read keyed + // by `recordSourceObjectName`, through `useSettledSchema` above) agree // only because this line sits inside the `dataProvider === 'object'` // branch, where the two resolvers coincide — not by construction; // hoisting this gate out of that branch would let them diverge silently. @@ -1009,29 +1055,7 @@ export const ObjectMap: React.FC = ({ return () => { fetchSeqRef.current += 1; }; - }, [dataProp, dataProvider, dataObjectName, dataItems, dataSource, hasInlineData, schema.filter, schema.sort, objectSchema, perms, invalidationNonce]); - - // Fetch object schema for field metadata - useEffect(() => { - const fetchObjectSchema = async () => { - try { - if (!dataSource) return; - - const objectName = recordSourceObjectName; - - if (!objectName) return; - - const schemaData = await dataSource.getObjectSchema(objectName); - setObjectSchema(schemaData); - } catch (err) { - console.error('Failed to fetch object schema:', err); - } - }; - - if (!hasInlineData && dataSource) { - fetchObjectSchema(); - } - }, [schema.objectName, dataSource, hasInlineData, dataProvider, dataObjectName, recordSourceObjectName]); + }, [dataProp, dataProvider, dataObjectName, dataItems, dataSource, hasInlineData, schema.filter, schema.sort, objectSchemaReady, objectSchema, perms, invalidationNonce, fetchesForItself]); // Transform data to map markers const { markers, invalidCount } = useMemo(() => { diff --git a/packages/plugin-view/src/ObjectView.tsx b/packages/plugin-view/src/ObjectView.tsx index 2c79127091..a5da47e324 100644 --- a/packages/plugin-view/src/ObjectView.tsx +++ b/packages/plugin-view/src/ObjectView.tsx @@ -1078,6 +1078,12 @@ export const ObjectView: React.FC = ({ // PR #7428 recorded for `ListView`'s memo and `RecordDetailView`'s effect). const perms = usePermissions(); + // objectui#10664 — the fetch below falls back to `schema.table?.sort` for its + // `$orderby`, and its dependency list names the two sources ahead of it but + // not this one, so a changed table sort kept the old order. Keyed by CONTENT: + // an equal sort in a fresh array is not a change (AGENTS.md #10). + const tableSortKey = JSON.stringify(schema.table?.sort ?? null); + // Fetch data for non-grid view types (grid handles its own data via ObjectGrid) useEffect(() => { let isMounted = true; @@ -1279,7 +1285,7 @@ export const ObjectView: React.FC = ({ }, [ schema.objectName, dataSource, currentViewType, refreshKey, currentNamedViewConfig, activeViewQueryInputs, renderListView, - objectSchemaReady, objectSchema, perms, authoredFilters, + objectSchemaReady, objectSchema, perms, authoredFilters, tableSortKey, ]); // Determine layout mode. #2578: default the record surface from how heavy the diff --git a/packages/plugin-view/src/__tests__/ObjectView.tableSortKey-10664.test.tsx b/packages/plugin-view/src/__tests__/ObjectView.tableSortKey-10664.test.tsx new file mode 100644 index 0000000000..2d43138238 --- /dev/null +++ b/packages/plugin-view/src/__tests__/ObjectView.tableSortKey-10664.test.tsx @@ -0,0 +1,101 @@ +/** + * 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#10664 (the census row for `object-view`) — the non-grid fetch + * re-reads when `table.sort` changes, and only then. + * + * The effect lowers `currentNamedViewConfig?.sort || activeViewQueryInputs?.sort + * || schema.table?.sort` onto `$orderby`. The first two sources are in its + * dependency list; the third was not. A mounted calendar / kanban view whose + * `table.sort` changed (and whose named view and active view declare none) kept + * the old order until something else re-ran the read. It is now keyed by + * CONTENT, so an equal sort in a fresh array is not a change (AGENTS.md #10). + * + * The CONTROL (an equal sort in a fresh array) is green before and after; it + * goes red for a fix that keys on the array's identity. + */ +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, act, cleanup, waitFor } from '@testing-library/react'; +import { ObjectView } from '../ObjectView'; +import type { ObjectViewSchema } from '@object-ui/types'; + +// The same seams `ObjectView.sortSink.test.tsx` stubs: the child renderers are +// not what these cases read, only the query this component issues itself. +vi.mock('@object-ui/react', async (importOriginal) => { + const React = await import('react'); + return { + ...(await importOriginal>()), + SchemaRenderer: ({ schema }: any) =>
{schema?.type}
, + SchemaRendererContext: React.createContext(null), + subscribeDataChanges: () => () => {}, + notifyDataChanged: () => {}, + }; +}); +vi.mock('@object-ui/plugin-grid', async (importOriginal) => ({ + ...(await importOriginal>()), + ObjectGrid: () =>
, +})); +vi.mock('@object-ui/plugin-form', async (importOriginal) => ({ + ...(await importOriginal>()), + ObjectForm: () =>
, +})); + +afterEach(cleanup); + +const settle = () => act(() => new Promise((resolve) => setTimeout(resolve, 50))); + +const BY_NAME_ASC = [{ field: 'name', order: 'asc' }]; +const BY_NAME_DESC = [{ field: 'name', order: 'desc' }]; + +const mockDataSource = () => ({ + find: vi.fn().mockResolvedValue({ data: [], total: 0 }), + findOne: vi.fn(), + create: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + getObjectSchema: vi.fn().mockResolvedValue({ name: 'task', fields: {} }), +}); + +const schemaFor = (sort: unknown) => + ({ type: 'object-view', objectName: 'task', defaultViewType: 'calendar', table: { sort } }) as unknown as ObjectViewSchema; + +function mount() { + const ds = mockDataSource(); + const view = render(); + const rerender = (sort: unknown) => view.rerender(); + return { ds, rerender }; +} + +const orderbys = (ds: ReturnType) => ds.find.mock.calls.map((c: any[]) => c[1]?.$orderby); + +describe('object-view keys its non-grid read on the table sort it sends (objectui#10664)', () => { + it('SUBJECT: a changed `table.sort` re-reads, with the new `$orderby`', async () => { + const { ds, rerender } = mount(); + await waitFor(() => expect(ds.find).toHaveBeenCalled()); + await settle(); + expect(orderbys(ds)).toEqual([{ name: 'asc' }]); + + await act(async () => { rerender(BY_NAME_DESC); }); + await settle(); + + expect(orderbys(ds), 'the changed sort never reached a read').toEqual([{ name: 'asc' }, { name: 'desc' }]); + }); + + it('CONTROL: an equal `table.sort` in a fresh array does not re-read', async () => { + const { ds, rerender } = mount(); + await waitFor(() => expect(ds.find).toHaveBeenCalled()); + await settle(); + const atRest = ds.find.mock.calls.length; + + await act(async () => { rerender([{ field: 'name', order: 'asc' }]); }); + await settle(); + + expect(ds.find.mock.calls.length, 'an equal sort re-read the view').toBe(atRest); + }); +});