diff --git a/.changeset/11699-record-load-once.md b/.changeset/11699-record-load-once.md new file mode 100644 index 0000000000..1982ed0a08 --- /dev/null +++ b/.changeset/11699-record-load-once.md @@ -0,0 +1,12 @@ +--- +'@object-ui/app-shell': patch +--- + +A record page open reads its record once: the page's own `$expand` read is no longer sent a second time when the object definition reaches the page again as a new object (objectui#11699). + +Measured on full reloads of a showcase task record page (a real ObjectStack backend and a production console build, 4 reloads per build): the page's `$expand` read of the record (`GET /api/v1/data/OBJ?populate=…`) went from 2 per open, one after the other, to 1. API requests per reload went from 40 to 39. The second read came from a host re-render that handed the page an identical object definition as a new object. The definition was equal to the previous one, so nothing the read sends had changed. + +- **The record page keys its record read on what the read sends.** The load effect depended on the object definition, the page and the permission answer as objects. All it reads from them is whether a page exists and the list of relations to expand after field-level security. It now depends on those two values. The record is no longer read again for an equal definition handed down as a new object, for an assigned page that replaces the synthesized default page, or for a permission answer that leaves the list unchanged. A permission answer that denies a relation still reads the record again without that relation, and so does a definition whose relations change. +- **`useMetadata().objects` keeps its identity.** The list and its entries stay the same objects while the stored object and view metadata stay the same. A re-render, or another metadata type landing, no longer rebuilds them. A landed object or view fetch, or an invalidation, still produces a new list that carries the change. + +Nothing is added to the package entry: no export, prop, type member or language-pack key. `mergeViewsIntoObjects` and `attachInlineSubforms` keep their signatures. diff --git a/packages/app-shell/src/providers/MetadataProvider.tsx b/packages/app-shell/src/providers/MetadataProvider.tsx index 6a7b90772f..40143ceda4 100644 --- a/packages/app-shell/src/providers/MetadataProvider.tsx +++ b/packages/app-shell/src/providers/MetadataProvider.tsx @@ -561,6 +561,43 @@ export function attachInlineSubforms(objects: any[]): any[] { }); } +/** + * The composed object list, keyed on the two STORED arrays it is built from + * (objectui#11699). + * + * The context value's `objects` getter used to run `mergeViewsIntoObjects` + + * `attachInlineSubforms` on every READ, and both hand back a new wrapper for + * every object they touch. So each re-render of a host that reads `objects` + * handed its children a new object for an unchanged definition — measured on a + * showcase record page, where the record-load effect read the record a second + * time for a definition that was JSON-equal, from the same context version. + * AGENTS.md #10: a provider may not republish an equal payload as a new + * object. + * + * The key is the payload, not a React memo: `entry.items` for `object` and for + * `view`. Every write to a cache entry REPLACES that array (a landed fetch, a + * by-name or whole-type invalidation, an org or preview-mode clear), and none + * mutates it in place, so a new array is exactly "this input changed" and the + * same array is exactly "it did not". ⚠️ Nothing re-checks that (AGENTS.md + * #9): it was read off this file and the list's readers once, when this cache + * was written. A write that pushes, splices, sorts or index-assigns into a + * stored `object` / `view` array would make this hand out the list as it was + * — replace the array instead. A discarded `useMemo` around the context + * value cannot move either array, so it cannot move the composed list. + * Module-level and weak, so an entry lives only as long as the stored list it + * was built from. + */ +const COMPOSED_OBJECTS = new WeakMap(); + +function composeObjects(objs: unknown[], views: unknown[]): unknown[] { + const cached = COMPOSED_OBJECTS.get(objs); + if (cached && cached.views === views) return cached.composed; + const merged = views.length ? mergeViewsIntoObjects(objs, views) : objs; + const composed = attachInlineSubforms(merged); + COMPOSED_OBJECTS.set(objs, { views, composed }); + return composed; +} + function emptyEntry(): TypeCacheEntry { return { status: 'idle', @@ -1069,11 +1106,9 @@ export function MetadataProvider({ children, adapter, ttlMs = DEFAULT_TTL_MS }: const base: MetadataContextValue = { apps: getEntry('app').items, + // The same list for the same stored inputs, read after read (objectui#11699). get objects() { - const objs = readType(TYPE_BY_STATE_KEY.objects); - const views = readType('view'); - const merged = views.length ? mergeViewsIntoObjects(objs, views) : objs; - return attachInlineSubforms(merged); + return composeObjects(readType(TYPE_BY_STATE_KEY.objects), readType('view')); }, get dashboards() { return readType(TYPE_BY_STATE_KEY.dashboards); diff --git a/packages/app-shell/src/providers/__tests__/MetadataProvider.composedObjectsIdentity-11699.test.tsx b/packages/app-shell/src/providers/__tests__/MetadataProvider.composedObjectsIdentity-11699.test.tsx new file mode 100644 index 0000000000..1cf708f58f --- /dev/null +++ b/packages/app-shell/src/providers/__tests__/MetadataProvider.composedObjectsIdentity-11699.test.tsx @@ -0,0 +1,236 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * objectui#11699 — the context value's `objects` list is the same list, entry + * for entry, for as long as the stored metadata it is built from is the same. + * + * ## What was measured + * + * On full reloads of a showcase record page (a real ObjectStack backend and a + * production console build), the page read its record a second time, in + * sequence. Instrumented, the record-load effect's second run had changed one + * dependency, `objectDef`, and the new definition was JSON-equal to the old + * one. Both definitions came out of this provider's `objects` getter at the + * SAME context version, and neither was the stored item: the getter ran + * `mergeViewsIntoObjects` + `attachInlineSubforms` on every read, and both + * wrap every object they touch in a new object. So any re-render of a host + * that reads `objects` handed its children new objects for unchanged + * metadata. The record page now keys its read on data (its own pin is + * `RecordDetailView.recordOpenRequests-11699.test.tsx`); this file pins the + * provider half (AGENTS.md #10: "a provider ... may not republish an equal + * payload as a new object"). + * + * ## What these pins hold, in both directions + * + * - Re-reading at the same version, and another metadata type landing (a new + * context value), hand back the same list and the same entries. + * - Replacing either stored array — the objects or the views — hands back a + * new list that carries the change. A cache that never let go would pass + * the first half and fail this one. + * - A DISCARDED `useMemo` cache — the context value's own — leaves the list + * where it was. React does not discard on its own in this tree (AGENTS.md + * #10), so the discard is forced by a module-level proxy, the technique and + * the reason of `packages/permissions`' `providerCtxIdentity.discarded.test.tsx`: + * every armed `useMemo` is thrown away, not ones picked by a marker. The + * case first shows the discard reached the provider (the context value IS a + * new object), then that the list did not move. A cache kept in the value's + * own `useMemo` would pass the first two bullets and fail this one. + * + * ⚠️ Unlike that file, `useCallback` is NOT discarded here, and that is a + * measured choice, not an omission: discarding `bump` re-runs the + * provider's preview-mode effect, which lists it as a dependency and, after + * mount, clears the WHOLE metadata cache — so the list empties for a reason + * outside the getter this file pins. That effect's dependency on a + * memoised callback is its own latent AGENTS.md #10 hazard, recorded on + * objectui#11699 and not changed here. + * + * The fixture makes both helpers wrap: `task` has a list view (merged into its + * `listViews`), and its `project` field declares `inlineEdit`, so `project` + * gets a derived form `subforms` entry. + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { act, cleanup, render, waitFor } from '@testing-library/react'; +import { MetadataProvider, useMetadata } from '../MetadataProvider'; + +const memoProxy = vi.hoisted(() => ({ armed: false, epoch: 0 })); + +type UseMemo = (factory: () => unknown, deps?: unknown[]) => unknown; +type ReactModule = Record & { useMemo: UseMemo; default?: Record }; + +vi.mock('react', async (importOriginal) => { + // Typed loosely on purpose: React's own `useMemo` type takes a + // `DependencyList`, which the patched signature below cannot satisfy. + const actual = await importOriginal(); + const realUseMemo = actual.useMemo; + const patchedUseMemo = (factory: () => unknown, deps?: unknown[]) => + memoProxy.armed && Array.isArray(deps) + ? realUseMemo(factory, [...deps, memoProxy.epoch]) + : realUseMemo(factory, deps); + return { + ...actual, + useMemo: patchedUseMemo, + default: { ...(actual.default ?? actual), useMemo: patchedUseMemo }, + }; +}); + +type Ctx = ReturnType; + +const OBJECTS = [ + { + name: 'task', + label: 'Task', + fields: { + title: { type: 'text' }, + project: { type: 'master_detail', reference: 'project', inlineEdit: true }, + }, + }, + { name: 'project', label: 'Project', fields: { name: { type: 'text' } } }, +]; +const VIEWS = [ + { name: 'task.all', object: 'task', viewKind: 'list', label: 'All', isDefault: true, config: { type: 'grid', columns: ['title'] } }, +]; +const PAGES = [{ name: 'task_detail', type: 'record', object: 'task' }]; + +/** Serves each type from `served`, as the network does: a new array per fetch. */ +function makeAdapter(served: Record) { + return { + clearCache: vi.fn(), + getClient: () => ({ + meta: { + getItems: (type: string) => Promise.resolve({ type, items: structuredClone(served[type] ?? []) }), + getItem: () => Promise.resolve({ item: null }), + }, + }), + } as unknown as Parameters[0]['adapter']; +} + +function mount(served: Record) { + let latest: Ctx | null = null; + function Probe() { + latest = useMetadata(); + return null; + } + const adapter = makeAdapter(served); + const tree = () => ( + + + + ); + const utils = render(tree()); + return { + ctx: () => latest as unknown as Ctx, + rerender: () => utils.rerender(tree()), + }; +} + +type Def = { name: string; fields?: Record; listViews?: Record; form?: { subforms?: unknown[] } }; +const byName = (list: unknown[], name: string) => (list as Def[]).find((o) => o.name === name); + +/** Wait until both helpers have had something to wrap. */ +async function untilComposed(ctx: () => Ctx) { + await waitFor(() => { + const task = byName(ctx().objects, 'task'); + expect(task?.listViews?.['task.all']).toBeTruthy(); + expect(byName(ctx().objects, 'project')?.form?.subforms).toHaveLength(1); + }); +} + +afterEach(() => { + cleanup(); + memoProxy.armed = false; +}); + +describe('MetadataProvider: the composed `objects` list keeps its identity (objectui#11699)', () => { + it('re-reading at the same version hands back the same list and the same entries', async () => { + const m = mount({ object: OBJECTS, view: VIEWS }); + await untilComposed(m.ctx); + + const ctx = m.ctx(); + const first = ctx.objects; + // Two reads in one render — the measured case: one context version, two getter reads. + expect(ctx.objects).toBe(first); + // A host re-render with nothing changed. + m.rerender(); + m.rerender(); + expect(m.ctx().objects).toBe(first); + expect(byName(m.ctx().objects, 'task')).toBe(byName(first, 'task')); + expect(byName(m.ctx().objects, 'project')).toBe(byName(first, 'project')); + }); + + it('another metadata type landing (a new context value) hands back the same list', async () => { + const m = mount({ object: OBJECTS, view: VIEWS, page: PAGES }); + await untilComposed(m.ctx); + const before = m.ctx(); + const first = before.objects; + + await act(async () => { + await before.ensureType('page'); + }); + await waitFor(() => expect(m.ctx().pages).toHaveLength(1)); + + expect(m.ctx()).not.toBe(before); + expect(m.ctx().objects).toBe(first); + expect(byName(m.ctx().objects, 'task')).toBe(byName(first, 'task')); + }); + + it('LIVE CONTROL: replacing the stored objects hands back a new list carrying the change', async () => { + const served: Record = { object: OBJECTS, view: VIEWS }; + const m = mount(served); + await untilComposed(m.ctx); + const first = m.ctx().objects; + + served.object = [ + { ...OBJECTS[0], fields: { ...OBJECTS[0].fields, due: { type: 'date' } } }, + OBJECTS[1], + ]; + await act(async () => { + await m.ctx().refresh('object'); + }); + await waitFor(() => expect(byName(m.ctx().objects, 'task')?.fields?.due).toBeTruthy()); + + expect(m.ctx().objects).not.toBe(first); + // Still composed: the merge and the subform pass ran over the new list. + expect(byName(m.ctx().objects, 'task')?.listViews?.['task.all']).toBeTruthy(); + expect(byName(m.ctx().objects, 'project')?.form?.subforms).toHaveLength(1); + }); + + it('LIVE CONTROL: replacing the stored views hands back a new list carrying the change', async () => { + const served: Record = { object: OBJECTS, view: VIEWS }; + const m = mount(served); + await untilComposed(m.ctx); + const first = m.ctx().objects; + + served.view = [ + ...VIEWS, + { name: 'task.mine', object: 'task', viewKind: 'list', label: 'Mine', config: { type: 'grid', columns: ['title'] } }, + ]; + await act(async () => { + await m.ctx().refresh('view'); + }); + await waitFor(() => expect(byName(m.ctx().objects, 'task')?.listViews?.['task.mine']).toBeTruthy()); + + expect(m.ctx().objects).not.toBe(first); + }); + + it('a discarded context-value memo leaves the list where it was (AGENTS.md #10)', async () => { + memoProxy.armed = true; + const m = mount({ object: OBJECTS, view: VIEWS }); + await untilComposed(m.ctx); + const before = m.ctx(); + const first = before.objects; + + // Two discards, each followed by a re-render with nothing changed. + memoProxy.epoch += 1; + m.rerender(); + memoProxy.epoch += 1; + m.rerender(); + + // The discard reached the provider: its context value was rebuilt. + expect(m.ctx()).not.toBe(before); + // And the list it hands out did not move. + expect(m.ctx().objects).toBe(first); + expect(byName(m.ctx().objects, 'task')).toBe(byName(first, 'task')); + expect(byName(m.ctx().objects, 'project')).toBe(byName(first, 'project')); + }); +}); diff --git a/packages/app-shell/src/views/RecordDetailView.recordOpenRequests-11699.test.tsx b/packages/app-shell/src/views/RecordDetailView.recordOpenRequests-11699.test.tsx index ac620190d1..2175150b82 100644 --- a/packages/app-shell/src/views/RecordDetailView.recordOpenRequests-11699.test.tsx +++ b/packages/app-shell/src/views/RecordDetailView.recordOpenRequests-11699.test.tsx @@ -22,11 +22,11 @@ * - each question is asked by the page header (`RecordDetailView`) and by * that `DetailView`, which mounts while the header's answer is pending. * - * Neither host is changed. The two sources now share an in-flight request — - * `ObjectStackAdapter.findOne` the way `find` already did, and the explain - * probe in `useRecordEditable` beside its finished-answer memo — so this file - * mounts the REAL page over the REAL adapter and the REAL hook and counts - * requests on the wire. + * Neither host is changed for those. The two sources share an in-flight + * request — `ObjectStackAdapter.findOne` the way `find` already did, and the + * explain probe in `useRecordEditable` beside its finished-answer memo — so + * this file mounts the REAL page over the REAL adapter and the REAL hook and + * counts requests on the wire. * * The transport HOLDS every request until the test releases it, in the order * the measurement saw: the page's side reads answer first, re-rendering the @@ -35,16 +35,73 @@ * answered at once would let every read settle before the next caller asked, * and pass on the defect. * - * ⚠️ Out of this file's count, by the card's ruling: the header's own - * `$expand` read (`GET /api/v1/data/OBJ?populate=…`), which the measurement - * also saw twice — sequentially, the second after the first settled, so no - * in-flight sharing can absorb it. + * ## The page's own `$expand` read (`GET /api/v1/data/OBJ?populate=…`) + * + * Measured the same way, it went out twice IN SEQUENCE, the second after the + * first had settled, so no in-flight sharing can absorb it. Instrumented, the + * record-load effect's second run had changed exactly one dependency: + * `objectDef`, handed down again as a new object by a host re-render — JSON- + * equal, with the very same `fields` object. Nothing the read sends had + * changed. The effect now keys on what the read sends (the gated `$expand` + * list and whether there is a page), so the cases below each deliver one of + * the identity changes the old dependency list re-read on, and count that read: + * + * - an equal definition as a new object (the measured trigger), and as a new + * object all the way down (what a byte-identical refetch hands over); + * - an assigned page landing after the record was read against a synthesized + * one (`effectivePage` changes, the read does not); + * - a permission answer arriving after the read that leaves the expansion as + * it was; + * - a DISCARDED `useMemo` cache (AGENTS.md #10). The synthesized page is a + * memo over `objectDef`, and React does not discard on its own in this + * tree, so the discard is forced — the same marker-scoped proxy as + * `plugin-list`'s `ListView.discardedExpandFieldsMemo.test.tsx`. + * + * Two live controls keep the key from being merely sticky: a definition whose + * relations change, and a permission answer that denies one (objectui#7230), + * each read the record again with the new expansion. */ import * as React from 'react'; import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { render, screen, waitFor, cleanup, act } from '@testing-library/react'; import { MemoryRouter } from 'react-router-dom'; +/** + * The discard proxy: a `useMemo` whose dependency list names one of `markers` + * recomputes when `discardNow()` bumps the epoch. Inert while `markers` is + * empty, which it is for every case but the discard one. + */ +const memoProxy = vi.hoisted(() => ({ markers: [] as unknown[], epoch: 0 })); + +type UseMemo = (factory: () => unknown, deps?: unknown[]) => unknown; +type ReactModule = Record & { useMemo: UseMemo; default?: Record }; + +vi.mock('react', async (importOriginal) => { + // Typed loosely on purpose: React's own `useMemo` type takes a + // `DependencyList`, which the patched signature below cannot satisfy. + const actual = await importOriginal(); + const realUseMemo = actual.useMemo; + const patched = (factory: () => unknown, deps?: unknown[]) => + Array.isArray(deps) && deps.some((d) => memoProxy.markers.includes(d)) + ? realUseMemo(factory, [...deps, memoProxy.epoch]) + : realUseMemo(factory, deps); + return { ...actual, useMemo: patched, default: { ...(actual.default ?? actual), useMemo: patched } }; +}); + +/** The permission answer the page reads; `null` leaves the real hook's answer. */ +const permsState = vi.hoisted(() => ({ current: null as unknown })); + +vi.mock('@object-ui/permissions', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + usePermissions: () => { + const real = actual.usePermissions(); + return (permsState.current ?? real) as typeof real; + }, + }; +}); + vi.mock('@object-ui/auth', async (importOriginal) => ({ ...(await importOriginal>()), useAuth: () => ({ user: { id: 'u1', name: 'Ada', image: null }, activeOrganization: null }), @@ -128,6 +185,14 @@ function json(body: unknown): Response { const RECORD_GET = `/api/v1/data/${OBJECT}/${REC}`; const isRecordRead = (h: Held) => h.method === 'GET' && h.path.startsWith(`/api/v1/data/${OBJECT}`); +/** The page's own read: `findOne` with `$expand` goes out as a one-row `populate` list read. */ +const isPageRecordRead = (h: Held) => + h.method === 'GET' && h.path.startsWith(`/api/v1/data/${OBJECT}?`) && h.path.includes('populate='); +/** The relations a page read asked the server to expand, sorted. */ +const expansionOf = (h: Held) => + (new URLSearchParams(h.path.slice(h.path.indexOf('?') + 1)).get('populate') ?? '') + .split(',').filter(Boolean).sort(); +const pageReads = (held: Held[]) => held.filter(isPageRecordRead); /** Answer a held request the way the platform would. */ function answerFor(h: Held): unknown { @@ -178,46 +243,116 @@ async function idle() { }); } -beforeEach(() => { - cleanup(); - __clearRecordEditableCache(); -}); -afterEach(() => { - cleanup(); - vi.unstubAllGlobals(); -}); +/** Answer everything, and anything that sets off, until the page asks for nothing more. */ +async function drain(held: Held[]) { + for (let round = 0; round < 10 && held.some((h) => !h.released); round++) { + await releaseWhere(held, () => true); + await idle(); + } + expect(held.filter((h) => !h.released)).toEqual([]); +} -describe('record page open: one record GET and one explain per distinct question (objectui#11699)', () => { - it('counts the requests a record open puts on the wire', async () => { - const { fetch, held } = heldTransport(); - // The explain probe rides the global fetch when no host apiFetch is - // provided; the adapter rides the same transport. - vi.stubGlobal('fetch', fetch); - const ds = new ObjectStackAdapter({ baseUrl: BASE, fetch, autoReconnect: false }); - await ds.connect(); - // Object metadata is the metadata service's, not this count's. - (ds as unknown as { getObjectSchema: () => Promise }).getObjectSchema = async () => objectDef; +/** A permission answer, in the shape `usePermissions()` hands back. */ +function policy(isLoaded: boolean, deniedReads: string[] = []) { + return { + isLoaded, + checkField: (_object: string, field: string, action: 'read' | 'write') => + !(action === 'read' && deniedReads.includes(field)), + check: () => ({ allowed: true }), + getFieldPermissions: () => [], + getRowFilter: () => undefined, + getObjectApiOperations: () => undefined, + roles: [] as string[], + userId: null, + systemPermissions: undefined, + hasCapabilities: () => true, + can: () => true, + cannot: () => false, + }; +} + +/** + * Mount the real record page over the real adapter, the way the console host + * does: the object list arrives as a prop AND through the metadata context. + * `show` re-renders it the way a host re-render does — a NEW metadata context + * value every time, with whatever object list and pages it is handed. + */ +async function openRecordPage(initial: { objects?: unknown[]; pages?: unknown[] } = {}) { + const { fetch, held } = heldTransport(); + // The explain probe rides the global fetch when no host apiFetch is + // provided; the adapter rides the same transport. + vi.stubGlobal('fetch', fetch); + const ds = new ObjectStackAdapter({ baseUrl: BASE, fetch, autoReconnect: false }); + await ds.connect(); + // Object metadata is the metadata service's, not this count's. + (ds as unknown as { getObjectSchema: () => Promise }).getObjectSchema = async () => objectDef; + let objects = initial.objects ?? [objectDef]; + let pages = initial.pages ?? [RECORD_PAGE]; + const tree = () => { const metadata = { - objects: [objectDef], pages: [RECORD_PAGE], loading: false, error: null, + objects, pages, loading: false, error: null, refresh: async () => {}, invalidate: () => {}, - ensureType: async () => [RECORD_PAGE], getItem: async () => null, - getItemsByType: () => [RECORD_PAGE], + ensureType: async () => pages, getItem: async () => null, + getItemsByType: () => pages, } as never; - - render( + return ( {}} objectNameOverride={OBJECT} recordIdOverride={REC} /> - , + ); + }; + const { rerender } = render(tree()); + const show = async (next: { objects?: unknown[]; pages?: unknown[] } = {}) => { + if (next.objects) objects = next.objects; + if (next.pages) pages = next.pages; + await act(async () => { + rerender(tree()); + }); + await drain(held); + }; + return { held, show }; +} + +/** Open, and answer the page's side reads before its record reads, as measured. */ +async function openAndSettle(initial: { objects?: unknown[]; pages?: unknown[] } = {}) { + const opened = await openRecordPage(initial); + const { held } = opened; + await waitFor(() => expect(pageReads(held).length).toBeGreaterThan(0)); + await idle(); + await releaseWhere(held, (h) => !isRecordRead(h)); + await idle(); + await releaseWhere(held, (h) => !isRecordRead(h)); + await idle(); + await drain(held); + expect(pageReads(held), 'the record open itself reads the page record once').toHaveLength(1); + return opened; +} + +beforeEach(() => { + cleanup(); + __clearRecordEditableCache(); + permsState.current = null; + memoProxy.markers = []; +}); +afterEach(() => { + cleanup(); + vi.unstubAllGlobals(); + permsState.current = null; + memoProxy.markers = []; +}); + +describe('record page open: one record GET and one explain per distinct question (objectui#11699)', () => { + it('counts the requests a record open puts on the wire', async () => { + const { held } = await openRecordPage(); // The body's own read is on the wire while the page's read is pending. await waitFor(() => expect(held.some((h) => h.path === RECORD_GET)).toBe(true)); @@ -232,17 +367,17 @@ describe('record page open: one record GET and one explain per distinct question // Then the record reads, and anything they set off, until the page asks // for nothing more. - for (let round = 0; round < 10 && held.some((h) => !h.released); round++) { - await releaseWhere(held, () => true); - await idle(); - } - expect(held.filter((h) => !h.released)).toEqual([]); + await drain(held); // The page really opened the record: the strip shows its values. await waitFor(() => expect(screen.getAllByText(RECORD.priority).length).toBeGreaterThan(0)); const recordGets = held.filter((h) => h.method === 'GET' && h.path === RECORD_GET); expect(recordGets).toHaveLength(1); + // The page's own `$expand` read, once, with both relations. + expect(pageReads(held)).toHaveLength(1); + expect(expansionOf(pageReads(held)[0])).toEqual(['assignee', 'project']); + const explains = held.filter((h) => h.method === 'POST' && h.path.endsWith('/security/explain')); const perQuestion = new Map(); for (const h of explains) { @@ -256,3 +391,78 @@ describe('record page open: one record GET and one explain per distinct question }); }); }); + +describe('the page reads its record once, whatever identity its inputs arrive with (objectui#11699)', () => { + it('a host re-render handing the same definition down as a new object reads nothing again', async () => { + const { held, show } = await openAndSettle(); + + // The measured trigger: the stored definition re-wrapped, `fields` shared. + await show({ objects: [{ ...objectDef, listViews: {} }] }); + // What a byte-identical refetch hands over: new objects all the way down. + await show({ objects: [structuredClone(objectDef)] }); + + expect(pageReads(held)).toHaveLength(1); + }); + + it('LIVE CONTROL: a definition whose relations change reads the record again, with the new expansion', async () => { + const { held, show } = await openAndSettle(); + + await show({ + objects: [{ + ...objectDef, + fields: { ...objectDef.fields, reviewer: { label: 'Reviewer', type: 'lookup', reference: 'sys_user' } }, + }], + }); + + expect(pageReads(held)).toHaveLength(2); + expect(expansionOf(pageReads(held)[1])).toEqual(['assignee', 'project', 'reviewer']); + }); + + it('an assigned page landing after the record was read against a synthesized one reads nothing again', async () => { + const { held, show } = await openAndSettle({ pages: [] }); + + await show({ pages: [RECORD_PAGE] }); + + expect(pageReads(held)).toHaveLength(1); + }); + + it('a discarded memo cache (AGENTS.md #10) reads nothing again', async () => { + // Armed before mount, so no memo's dependency count changes mid-life. A + // synthesized page: that is the memo over `objectDef` the old dependency + // list reached through `effectivePage`. + memoProxy.markers = [objectDef]; + const { held, show } = await openAndSettle({ pages: [] }); + + memoProxy.epoch += 1; + await show(); + memoProxy.epoch += 1; + await show(); + + expect(pageReads(held)).toHaveLength(1); + }); +}); + +describe('a permission answer that arrives after the read (objectui#11699, objectui#7230)', () => { + it('leaving the expansion as it was, it reads nothing again', async () => { + permsState.current = policy(false); + const { held, show } = await openAndSettle(); + expect(expansionOf(pageReads(held)[0])).toEqual(['assignee', 'project']); + + permsState.current = policy(true); + await show(); + + expect(pageReads(held)).toHaveLength(1); + }); + + it('LIVE CONTROL: denying a relation, it reads the record again without it', async () => { + permsState.current = policy(false); + const { held, show } = await openAndSettle(); + expect(expansionOf(pageReads(held)[0])).toEqual(['assignee', 'project']); + + permsState.current = policy(true, ['assignee']); + await show(); + + expect(pageReads(held)).toHaveLength(2); + expect(expansionOf(pageReads(held)[1])).toEqual(['project']); + }); +}); diff --git a/packages/app-shell/src/views/RecordDetailView.tsx b/packages/app-shell/src/views/RecordDetailView.tsx index f0bd4e123f..921ada8cff 100644 --- a/packages/app-shell/src/views/RecordDetailView.tsx +++ b/packages/app-shell/src/views/RecordDetailView.tsx @@ -524,59 +524,85 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri // // ⚠️ [objectui#7230] THE POSITION IS LOAD-BEARING, not cosmetic. This used to // be a `usePermissions()` call ~670 lines below, next to the header's - // Edit/Delete gates. The record-load effect immediately after this line now - // FLS-gates its `$expand`, and an effect's DEPENDENCY ARRAY is evaluated - // DURING render — so listing `perms` there while the binding was still - // declared below would hit the temporal dead zone and throw + // Edit/Delete gates. The record-load effect just below FLS-gates its + // `$expand`, and that gated list is computed DURING render (objectui#11699) + // — so reading `perms` there while the binding was still declared below + // would hit the temporal dead zone and throw // `Cannot access 'perms' before initialization`: a crash, not a stale value. // The hook moved up; the site below destructures THIS value instead of // calling the hook a second time, so the hook order is unchanged in shape. // Same structural note PR #7229 recorded for `ListView`'s memo. const perms = usePermissions(); + // What the page-record read below SENDS, held as primitives (objectui#11699). + // + // The record-load effect used to depend on three object identities — + // `effectivePage`, `objectDef` and `perms` — while all it reads from them + // is a yes/no ("is there a page?") and one list (the relations to expand). + // Measured on full reloads of a showcase record page, the record's `$expand` + // read went out twice in sequence, and the second run had changed nothing + // but `objectDef`'s identity: a JSON-equal definition, with the very same + // `fields` object, handed down again as a new object by a host re-render. + // `effectivePage` was the same trap one step removed: the synthesized page + // is a `useMemo` over `objectDef`, and an assigned page landing replaces it + // without changing anything the read sends. + // + // So the effect keys on the DATA it reads (AGENTS.md #10), never on the + // objects it reads it from: a new object carrying the same relations, an + // assigned page taking over from a synthesized one, a discarded memo, or a + // permission answer that leaves the list as it was does not read the record + // again; a list that CHANGES does. + const hasPage = !!effectivePage; + // Expand lookup/master_detail fields so the page receives display + // names (e.g. account.name) rather than raw foreign-key IDs. The + // page subtitle interpolation and record:* renderers depend on this. + // + // [objectui#7230] FIELD-LEVEL SECURITY ON `$expand`, the gate + // objectui#7215 / PR #7229 put on the two projection sites in its scope. + // `$select` on a denied lookup asks the server for a bare foreign key; + // `$expand` asks it to RESOLVE the relation and hand back the related + // record — the larger of the two requests. + // + // ⚠️ NO COLUMN LIST IS PASSED HERE, which makes this the sharpest of the + // family: `buildExpandFields` reads an absent column list as "no column + // restriction" and falls back to EVERY declared relation on the object, + // denied ones included. Every record page in the console therefore asked + // for the object's full relation set by default, not by configuration. + // + // Graded as objectui#7215 graded it, by measurement rather than assumption: + // against ObjectStack this is defence-in-depth, because `plugin-security`'s + // `FieldMasker.maskRecord` does `delete result[field]` on every unreadable + // key and objectql's expand path writes the resolved record back under THAT + // SAME KEY, so one statement removes the expanded object and the bare id + // alike; the expansion sub-read itself takes the referenced object's full + // CRUD + RLS + FLS treatment (objectstack#7626). It is load-bearing for a + // backend that does not strip. + // + // ⭐ THE GATE IS ON THE HELPER'S OUTPUT. There is no input to gate on this + // site, and the output holds only DECLARED reference-bearing fields, so the + // "`checkField` answers false for an undeclared key" trap is structurally + // unreachable and a derived / host-joined key is never judged. An + // unanswered policy filters nothing. The gated list is the effect's + // dependency (as a string), so an answer that NARROWS it re-reads the record + // without the denied relation the moment it arrives, and an answer that + // leaves it as it was reads nothing again (objectui#11699). Pinned in + // `RecordDetailView.expandFls-7230.test.tsx` and + // `RecordDetailView.recordOpenRequests-11699.test.tsx`. + const expandable = buildExpandFields(objectDef?.fields); + const pageRecordExpand = JSON.stringify( + !objectName || !perms?.isLoaded + ? expandable + : expandable.filter((f) => perms.checkField(objectName, f, 'read')), + ); + useEffect(() => { let cancelled = false; - if (!effectivePage || !pureRecordId || !objectName || !dataSource?.findOne) { + if (!hasPage || !pureRecordId || !objectName || !dataSource?.findOne) { setPageRecord(null); setPageRecordStatus('idle'); return; } - // Expand lookup/master_detail fields so the page receives display - // names (e.g. account.name) rather than raw foreign-key IDs. The - // page subtitle interpolation and record:* renderers depend on this. - // - // [objectui#7230] FIELD-LEVEL SECURITY ON `$expand`, the gate - // objectui#7215 / PR #7229 put on the two projection sites in its scope. - // `$select` on a denied lookup asks the server for a bare foreign key; - // `$expand` asks it to RESOLVE the relation and hand back the related - // record — the larger of the two requests. - // - // ⚠️ NO COLUMN LIST IS PASSED HERE, which makes this the sharpest of the - // family: `buildExpandFields` reads an absent column list as "no column - // restriction" and falls back to EVERY declared relation on the object, - // denied ones included. Every record page in the console therefore asked - // for the object's full relation set by default, not by configuration. - // - // Graded as objectui#7215 graded it, by measurement rather than assumption: - // against ObjectStack this is defence-in-depth, because `plugin-security`'s - // `FieldMasker.maskRecord` does `delete result[field]` on every unreadable - // key and objectql's expand path writes the resolved record back under THAT - // SAME KEY, so one statement removes the expanded object and the bare id - // alike; the expansion sub-read itself takes the referenced object's full - // CRUD + RLS + FLS treatment (objectstack#7626). It is load-bearing for a - // backend that does not strip. - // - // ⭐ THE GATE IS ON THE HELPER'S OUTPUT. There is no input to gate on this - // site, and the output holds only DECLARED reference-bearing fields, so the - // "`checkField` answers false for an undeclared key" trap is structurally - // unreachable and a derived / host-joined key is never judged. An - // unanswered policy filters nothing; `perms` is in this effect's dependency - // list, so the record is re-read the moment the answer arrives. Pinned in - // `RecordDetailView.expandFls-7230.test.tsx`. - const expandable = buildExpandFields(objectDef?.fields); - const expandFields = !perms?.isLoaded - ? expandable - : expandable.filter((f) => perms.checkField(objectName, f, 'read')); + const expandFields: string[] = JSON.parse(pageRecordExpand); const params = expandFields.length > 0 ? { $expand: expandFields } : undefined; const loadRecord = () => { setPageRecordStatus('loading'); @@ -618,7 +644,12 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri }; // #2269: recordInvalidationNonce re-runs this fetch in place whenever the // record (or its object) is invalidated on the bus. - }, [effectivePage, objectName, pureRecordId, dataSource, objectDef, recordInvalidationNonce, perms]); + // + // objectui#11699: every dependency here is either a primitive or the + // adapter the read goes through. ⛔ Do not list `objectDef`, + // `effectivePage` or `perms` here — derive what the read sends from them + // above instead (AGENTS.md #10). + }, [hasPage, objectName, pureRecordId, dataSource, pageRecordExpand, recordInvalidationNonce]); // The loaded record AS THE VIEWER MAY READ IT (objectui#10434, // objectui#10499): `withoutDeniedFields` removes the fields the loaded