From cee2b98f8d5ffe3d1b9dadf2f2b4b09fd50d362e Mon Sep 17 00:00:00 2001 From: "claude[bot]" Date: Sun, 30 Aug 2026 06:32:29 +0000 Subject: [PATCH] perf(permissions): build both providers' context value where React cannot discard it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both permission providers built their context value in a `useMemo` over `useCallback`s. Neither carries a semantic guarantee: React may discard the cache and recompute even when the dependency list compares equal, and every factory builds a fresh object, so a discard handed `PermCtx.Provider` a new value with every permission it carries unchanged. That moves the key `usePermissions()` caches on and re-runs the consumer chain that names it. Hardening, not a repair: on the pinned React 19.2.8 the cache is not discarded spontaneously and this repo has no `Activity`/Offscreen subtree. Each member and each value is now keyed on the identities of the inputs it is derived from, in a module-level `WeakMap` — the technique `usePermissions()` already uses. Dependency sets are unchanged; no export or context shape changes. Part of #6813 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013hfmP9hoMd3dJwTh85J4yB --- .../tidy-permissions-provider-ctx-identity.md | 35 ++ .../permissions/src/MePermissionsProvider.tsx | 120 +++-- .../permissions/src/PermissionProvider.tsx | 108 ++-- .../providerCtxIdentity.discarded.test.tsx | 476 ++++++++++++++++++ packages/permissions/src/discardProofCache.ts | 105 ++++ 5 files changed, 768 insertions(+), 76 deletions(-) create mode 100644 .changeset/tidy-permissions-provider-ctx-identity.md create mode 100644 packages/permissions/src/__tests__/providerCtxIdentity.discarded.test.tsx create mode 100644 packages/permissions/src/discardProofCache.ts diff --git a/.changeset/tidy-permissions-provider-ctx-identity.md b/.changeset/tidy-permissions-provider-ctx-identity.md new file mode 100644 index 0000000000..217495d986 --- /dev/null +++ b/.changeset/tidy-permissions-provider-ctx-identity.md @@ -0,0 +1,35 @@ +--- +'@object-ui/permissions': patch +--- + +Both permission providers now build their context value where React cannot discard it + +`PermissionProvider` built its context value in a `useMemo` over four +`useCallback`s, and `MePermissionsProvider` in a `useMemo` over six. Neither +carries a semantic guarantee: React is permitted to discard the cache and +recompute even when the dependency list compares equal, and every one of those +factories builds a fresh object. A discard would therefore hand +`PermCtx.Provider` a NEW context value with every permission it carries +unchanged — which moves the key `usePermissions()` caches on, and re-runs the +consumer chain that names it: `ListView`'s data-fetch effect (an extra +`dataSource.find`), `DetailView`'s gatedSchema, `ObjectForm`, `ModalForm`, +`ObjectGrid` and `RelatedList`. + +⚠️ This is **hardening, not a repair**. Nothing misbehaves today: on this +repo's pinned React 19.2.8 the cache is not discarded spontaneously — 51 +re-renders with no provider, 51 with one and 42 under `StrictMode` each +returned one identity — and there is no `Activity`/Offscreen subtree here, +which is the documented case where React does throw memo caches away. What is +removed is the dependency on React continuing not to exercise a licence it +holds. + +Each cached member and each context value is now keyed on the identities of the +inputs it is derived from, in a module-level `WeakMap` React has no say over — +the same technique that made `usePermissions()`'s own return discard-proof one +link down the chain. The dependency sets are unchanged, so nothing churns more +often than it did, and a genuine permission change still publishes a new +context value to every consumer. Two providers given the same inputs now share +one context value, which is stricter than the per-instance memo it replaces. + +No published export changes, and the context carries exactly what it carried +before. diff --git a/packages/permissions/src/MePermissionsProvider.tsx b/packages/permissions/src/MePermissionsProvider.tsx index 70f69df2e9..5e3d607049 100644 --- a/packages/permissions/src/MePermissionsProvider.tsx +++ b/packages/permissions/src/MePermissionsProvider.tsx @@ -6,7 +6,7 @@ * LICENSE file in the root directory of this source tree. */ -import React, { useEffect, useMemo, useState, useCallback } from 'react'; +import React, { useEffect, useState, useCallback } from 'react'; import { HttpFetchError, backoffMs, @@ -20,6 +20,7 @@ import type { FieldLevelPermission, } from '@object-ui/types'; import { PermCtx, type PermissionContextValue } from './PermissionContext.js'; +import { createDiscardProofCache } from './discardProofCache.js'; /** * Shape of the upstream `/api/v1/auth/me/permissions` response. @@ -99,6 +100,48 @@ export interface MePermissionsProviderProps { const DEFAULT_ENDPOINT = '/api/v1/auth/me/permissions'; +/** + * [objectui#6813] One cache per cached thing, each keyed on exactly the inputs + * that thing is derived from — the same sets the `useCallback`/`useMemo` + * dependency arrays named before, so nothing churns more often than it did. + * What changes is that React can no longer discard them: a discard used to + * hand `PermCtx.Provider` a NEW value with every permission it carries + * unchanged, which moves the key `usePermissions()` caches on (objectui#6724) + * and re-runs every consumer effect downstream. See `discardProofCache.ts` for + * why this is a module-level `WeakMap` and not a `useMemo` or a `useRef`. + */ +const CHECK = createDiscardProofCache(); +const CHECK_FIELD = createDiscardProofCache(); +const GET_FIELD_PERMISSIONS = createDiscardProofCache(); +const GET_OBJECT_API_OPERATIONS = createDiscardProofCache(); +const VALUE = createDiscardProofCache(); + +/** + * Stands in for `data === null`, which cannot key a `WeakMap`. With no data + * every member answers its fail-closed constant and `isLoaded` is false, so + * this sentinel names exactly one reachable value rather than a family of them. + */ +const NO_DATA: object = { data: 'unloaded' }; + +/** + * `isLoaded` is a boolean and cannot key a `WeakMap` either. Its domain has two + * members, so two module-level sentinels cover it totally — no coercion, no + * collision. It is the ONLY thing `loading` and `error` contribute to the + * context value, so keying on it directly (rather than on `[data, loading, + * error]`) also stops a new `Error` identity from churning a value that reads + * the same to every consumer. + */ +const LOADED: object = { isLoaded: true }; +const NOT_LOADED: object = { isLoaded: false }; + +/** + * The row filter this provider can offer for any object: none. `/me/permissions` + * carries no row-level filter, so this is a constant for every object and every + * provider instance — module-level, which is strictly stabler than the + * `useCallback(..., [])` it replaces, since React may discard that one. + */ +const NO_ROW_FILTER: PermissionContextValue['getRowFilter'] = () => undefined; + /** * MePermissionsProvider * @@ -188,7 +231,9 @@ export function MePermissionsProvider({ return () => { token.cancelled = true; }; }, [fetchPermissions, initialPermissions]); - const checkField = useCallback( + const dataKey: object = data ?? NO_DATA; + + const checkField = CHECK_FIELD([dataKey], () => (object: string, field: string, action: 'read' | 'write'): boolean => { if (!data) return false; // fail-closed // Normalize casing — backend stores keys lowercase but callers may @@ -219,10 +264,9 @@ export function MePermissionsProvider({ ? objPerm.allowRead !== false : objPerm.allowEdit !== false; }, - [data], ); - const check = useCallback( + const check = CHECK([dataKey], () => (object: string, action: PermissionAction): PermissionCheckResult => { if (!data) return { allowed: false, reason: 'permissions-loading' }; const objPerm = data.objects?.[object] ?? data.objects?.['*']; @@ -245,10 +289,9 @@ export function MePermissionsProvider({ const allowed = objPerm ? (objPerm as any)[k] !== false : data.authenticated !== true; return { allowed, reason: allowed ? undefined : 'denied-by-permission-set' }; }, - [data], ); - const getFieldPermissions = useCallback( + const getFieldPermissions = GET_FIELD_PERMISSIONS([dataKey], () => (object: string): FieldLevelPermission[] => { if (!data) return []; const prefix = `${object}.`; @@ -264,12 +307,9 @@ export function MePermissionsProvider({ } return out; }, - [data], ); - const getRowFilter = useCallback(() => undefined, []); - - const getObjectApiOperations = useCallback( + const getObjectApiOperations = GET_OBJECT_API_OPERATIONS([dataKey], () => (object: string): string[] | undefined => { if (!data) return undefined; const objKey = (object ?? '').toLowerCase(); @@ -279,38 +319,38 @@ export function MePermissionsProvider({ const ops = objPerm?.apiOperations; return Array.isArray(ops) ? ops : undefined; }, - [data], ); - const value = useMemo( - () => ({ - check, - checkField, - getFieldPermissions, - getRowFilter, - getObjectApiOperations, - roles: data?.roles ?? [], - // [objectui#5683] `null` while unloaded/anonymous — never ''. Consumers - // treat null as "unknown" and defer to the server. - userId: data?.userId ?? null, - // [objectui#4656] Forward the raw signal — do NOT `?? []` this. A - // backend predating ADR-0066 omits `systemPermissions` from the - // response entirely, and defaulting that to `[]` here made it - // indistinguishable from a genuinely empty grant to every consumer - // downstream (this provider's own `hasCapabilities` included). - systemPermissions: data?.systemPermissions, - hasCapabilities: (required: string[]) => { - const perms = data?.systemPermissions; - // Unknown (backend never reported systemPermissions) fails OPEN — see - // the doctrine on `PermissionContextValue.hasCapabilities`. - if (!Array.isArray(perms)) return true; - const held = new Set(perms); - return required.every((p) => held.has(p)); - }, - isLoaded: !loading && !error && data !== null, - }), - [check, checkField, getFieldPermissions, getRowFilter, getObjectApiOperations, data, loading, error], - ); + const isLoaded = !loading && !error && data !== null; + + // Keyed on the union of what the members above are keyed on, so this value is + // rebuilt exactly when one of them is and never captures a stale member. + const value = VALUE([dataKey, isLoaded ? LOADED : NOT_LOADED], () => ({ + check, + checkField, + getFieldPermissions, + getRowFilter: NO_ROW_FILTER, + getObjectApiOperations, + roles: data?.roles ?? [], + // [objectui#5683] `null` while unloaded/anonymous — never ''. Consumers + // treat null as "unknown" and defer to the server. + userId: data?.userId ?? null, + // [objectui#4656] Forward the raw signal — do NOT `?? []` this. A + // backend predating ADR-0066 omits `systemPermissions` from the + // response entirely, and defaulting that to `[]` here made it + // indistinguishable from a genuinely empty grant to every consumer + // downstream (this provider's own `hasCapabilities` included). + systemPermissions: data?.systemPermissions, + hasCapabilities: (required: string[]) => { + const perms = data?.systemPermissions; + // Unknown (backend never reported systemPermissions) fails OPEN — see + // the doctrine on `PermissionContextValue.hasCapabilities`. + if (!Array.isArray(perms)) return true; + const held = new Set(perms); + return required.every((p) => held.has(p)); + }, + isLoaded, + })); if (loading && !data) return <>{loadingFallback}; if (error && !data) { diff --git a/packages/permissions/src/PermissionProvider.tsx b/packages/permissions/src/PermissionProvider.tsx index b1c76c2a0e..899df91fea 100644 --- a/packages/permissions/src/PermissionProvider.tsx +++ b/packages/permissions/src/PermissionProvider.tsx @@ -6,7 +6,7 @@ * LICENSE file in the root directory of this source tree. */ -import React, { useMemo, useCallback } from 'react'; +import React from 'react'; import type { RoleDefinition, ObjectPermissionConfig, @@ -16,6 +16,7 @@ import type { } from '@object-ui/types'; import { PermCtx, type PermissionContextValue } from './PermissionContext.js'; import { evaluatePermission } from './evaluator.js'; +import { createDiscardProofCache } from './discardProofCache.js'; export interface PermissionProviderProps { /** Role definitions */ @@ -30,6 +31,51 @@ export interface PermissionProviderProps { children: React.ReactNode; } +/** + * [objectui#6813] One cache per cached thing, each keyed on exactly the inputs + * that thing is derived from — the same sets the `useCallback`/`useMemo` + * dependency arrays named before, so nothing churns more often than it did. + * What changes is that React can no longer discard them: a discard used to + * hand `PermCtx.Provider` a NEW value with every permission it carries + * unchanged, which moves the key `usePermissions()` caches on (objectui#6724) + * and re-runs every consumer effect downstream. See `discardProofCache.ts` for + * why this is a module-level `WeakMap` and not a `useMemo` or a `useRef`. + */ +const CHECK = createDiscardProofCache(); +const CHECK_FIELD = createDiscardProofCache(); +const GET_FIELD_PERMISSIONS = createDiscardProofCache(); +const GET_ROW_FILTER = createDiscardProofCache(); +const VALUE = createDiscardProofCache(); + +/** + * Stands in for an absent `user` prop, which is optional and therefore cannot + * key a `WeakMap` on its own. One module-level object, so "no user" is a + * stable identity rather than a hole in the key tuple. + */ +const NO_USER: object = { user: 'absent' }; + +/** + * [#3391] Role-based provider does not model the server's effective API + * operation set — return undefined so consumers keep current behavior. + * + * [objectui#6813] Module-level rather than a literal rebuilt inside the value + * factory: three consumers name this function in a dependency array + * (`RecordDetailView`, `ObjectDataPage`, `ObjectView`), and a constant that + * answers `undefined` for every object has nothing per-provider to close over. + */ +const NO_API_OPERATIONS: PermissionContextValue['getObjectApiOperations'] = () => undefined; + +/** + * This role-based provider has no backend answer to give — it never fetches + * /me/permissions — so ADR-0066 system capabilities are simply unreported here + * (`systemPermissions: undefined` below, not `[]`; objectui#4656). A literal + * `[]` would claim "reported, holds nothing", which this provider cannot back + * up. `hasCapabilities` stays fail-open to match. The console uses + * MePermissionsProvider, which wires the real systemPermissions from + * /me/permissions. + */ +const ALL_CAPABILITIES: PermissionContextValue['hasCapabilities'] = () => true; + export function PermissionProvider({ roles, permissions, @@ -37,7 +83,9 @@ export function PermissionProvider({ user, children, }: PermissionProviderProps) { - const check = useCallback( + const userKey = user ?? NO_USER; + + const check = CHECK([roles, permissions, userRoles, userKey], () => (object: string, action: PermissionAction, record?: Record): PermissionCheckResult => { return evaluatePermission({ roles, @@ -49,10 +97,9 @@ export function PermissionProvider({ record, }); }, - [roles, permissions, userRoles, user], ); - const checkField = useCallback( + const checkField = CHECK_FIELD([permissions, userRoles], () => (object: string, field: string, action: 'read' | 'write'): boolean => { const objectConfig = permissions.find((p) => p.object === object); if (!objectConfig) return true; // No config means no restrictions @@ -81,10 +128,9 @@ export function PermissionProvider({ return true; // Default allow }, - [permissions, userRoles], ); - const getFieldPermissions = useCallback( + const getFieldPermissions = GET_FIELD_PERMISSIONS([permissions, userRoles], () => (object: string): FieldLevelPermission[] => { const objectConfig = permissions.find((p) => p.object === object); if (!objectConfig) return []; @@ -100,10 +146,9 @@ export function PermissionProvider({ } return fieldPerms; }, - [permissions, userRoles], ); - const getRowFilter = useCallback( + const getRowFilter = GET_ROW_FILTER([permissions, userRoles], () => (object: string): string | undefined => { const objectConfig = permissions.find((p) => p.object === object); if (!objectConfig) return undefined; @@ -118,36 +163,27 @@ export function PermissionProvider({ } return undefined; }, - [permissions, userRoles], ); - const value = useMemo( - () => ({ - check, - checkField, - getFieldPermissions, - getRowFilter, - // [#3391] Role-based provider does not model the server's effective API - // operation set — return undefined so consumers keep current behavior. - getObjectApiOperations: () => undefined, - roles: userRoles, - // [objectui#5683] Role-based provider never learns who the user IS — - // unreported (`null`), so create-form current_user seeding stays - // server-side under this provider. - userId: null, - // This role-based provider has no backend answer to give — it never - // fetches /me/permissions — so ADR-0066 system capabilities are simply - // unreported here: `undefined`, not `[]` (objectui#4656; a literal `[]` - // would claim "reported, holds nothing", which this provider cannot - // back up). `hasCapabilities` stays fail-open to match. The console - // uses MePermissionsProvider, which wires the real systemPermissions - // from /me/permissions. - systemPermissions: undefined, - hasCapabilities: () => true, - isLoaded: true, - }), - [check, checkField, getFieldPermissions, getRowFilter, userRoles], - ); + // Keyed on the union of what the members above are keyed on, so this value + // is rebuilt exactly when one of them is and never captures a stale member. + const value = VALUE([roles, permissions, userRoles, userKey], () => ({ + check, + checkField, + getFieldPermissions, + getRowFilter, + getObjectApiOperations: NO_API_OPERATIONS, + roles: userRoles, + // [objectui#5683] Role-based provider never learns who the user IS — + // unreported (`null`), so create-form current_user seeding stays + // server-side under this provider. + userId: null, + // [objectui#4656] Unreported, not a reported-empty grant — see + // `ALL_CAPABILITIES` above for the full reasoning. + systemPermissions: undefined, + hasCapabilities: ALL_CAPABILITIES, + isLoaded: true, + })); return {children}; } diff --git a/packages/permissions/src/__tests__/providerCtxIdentity.discarded.test.tsx b/packages/permissions/src/__tests__/providerCtxIdentity.discarded.test.tsx new file mode 100644 index 0000000000..73e5d090dd --- /dev/null +++ b/packages/permissions/src/__tests__/providerCtxIdentity.discarded.test.tsx @@ -0,0 +1,476 @@ +/** + * 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#6813 — neither permission provider may move the context value's + * identity while the inputs it is derived from are unchanged. + * + * `PermissionProvider` built its value in a `useMemo` over four `useCallback`s + * and `MePermissionsProvider` in a `useMemo` over six. Neither carries a + * semantic guarantee: React may discard the cache and recompute even when the + * dependency list compares equal, and every factory here builds a fresh + * object. A discard therefore handed `PermCtx.Provider` a NEW context value + * with every permission it carries unchanged — which moves the key + * `usePermissions()` caches on (objectui#6724) and re-runs the whole consumer + * chain: `ListView`'s data-fetch effect (an extra `dataSource.find`), + * `DetailView`'s gatedSchema, `ObjectForm`/`ModalForm`/`ObjectGrid`/ + * `RelatedList` — 9 dependency arrays across 6 files naming the whole object, + * measured on `1e14d70ae`. + * + * ⚠️ WHAT IS AND IS NOT OBSERVABLE TODAY. This file pins a LATENT hazard, not + * a reproduction, and must not be read as a bug being fixed. On React 19.2.8 + * (this repo's pinned version) the cache is NOT discarded spontaneously — + * measured while objectui#6724 landed: 51 re-renders with no provider, 51 with + * one and 42 under `StrictMode` each returned ONE identity — and there is no + * `Activity`/Offscreen subtree in this repo, which is the documented case + * where React does throw memo caches away. So the discard below is FORCED by a + * proxy, because React will not do it on its own and a pin that does not force + * one would prove nothing here. + * + * The proxy patches `useMemo` AND `useCallback` at the MODULE level: the + * providers reach them through their own `import { … } from 'react'` bindings, + * and `vi.spyOn`/assignment/`defineProperty` on the frozen `[object Module]` + * namespace all fail to patch those — silently leaving any pin built on them + * unfalsifiable. Same technique and same reason as + * `usePermissions.discardedIdentity.test.tsx` (objectui#6724) and + * `plugin-list/src/__tests__/ListView.discardedExpandFieldsMemo.test.tsx` + * (objectui#6697). It differs from those in one way that matters here: it + * discards EVERY armed memo and callback rather than ones matched by a marker + * dependency, because the fix under test removes the dependency arrays + * altogether — a marker-matched proxy would have nothing left to match and + * would go green for the trivial reason. + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { cleanup, render, waitFor } from '@testing-library/react'; +import * as ReactNS from 'react'; +import type { ObjectPermissionConfig, RoleDefinition } from '@object-ui/types'; +import { PermCtx, type PermissionContextValue } from '../PermissionContext'; +import { usePermissions } from '../usePermissions'; +import { PermissionProvider } from '../PermissionProvider'; +import { MePermissionsProvider, type MePermissionsResponse } from '../MePermissionsProvider'; + +const memoProxy = vi.hoisted(() => ({ armed: false, epoch: 0 })); + +vi.mock('react', async (importOriginal) => { + // `` matches the sibling pins (objectui#6697 / #6724) and is + // load-bearing: a precise module type makes the real hooks' deps parameter + // `DependencyList`, which the patched signatures below cannot satisfy. + const actual = await importOriginal(); + const realUseMemo = actual.useMemo; + const realUseCallback = actual.useCallback; + const patchedUseMemo = (factory: () => unknown, deps?: unknown[]) => + memoProxy.armed && Array.isArray(deps) + ? realUseMemo(factory, [...deps, memoProxy.epoch]) + : realUseMemo(factory, deps); + const patchedUseCallback = (fn: unknown, deps?: unknown[]) => + memoProxy.armed && Array.isArray(deps) + ? realUseCallback(fn, [...deps, memoProxy.epoch]) + : realUseCallback(fn, deps); + return { + ...actual, + useMemo: patchedUseMemo, + useCallback: patchedUseCallback, + default: { + ...(actual.default ?? actual), + useMemo: patchedUseMemo, + useCallback: patchedUseCallback, + }, + }; +}); + +/** Put EVERY memo and callback in the tree under this file's control. */ +function armDiscardProxy(): () => void { + memoProxy.armed = true; + return () => { + memoProxy.armed = false; + }; +} +/** Throw away every armed cache — one discard event, on demand. */ +function discardNow(): void { + memoProxy.epoch += 1; +} + +afterEach(() => { + cleanup(); + memoProxy.armed = false; +}); + +/** Records the ctx the provider published, what a consumer saw, and effect runs. */ +function makeProbe() { + const ctxSeen: (PermissionContextValue | null)[] = []; + const permsSeen: ReturnType[] = []; + const effectRuns: unknown[] = []; + const Probe: React.FC = () => { + // The provider's own output, and the consumer-visible object one link + // downstream. objectui#6724 made the second stable while the FIRST is + // unchanged, so both are asserted here: that is the end-to-end claim. + ctxSeen.push(ReactNS.useContext(PermCtx)); + const perms = usePermissions(); + permsSeen.push(perms); + // Exactly the consumer shape this card is about: `ListView`'s data-fetch + // effect names the whole object in its dependency array. + ReactNS.useEffect(() => { + effectRuns.push(perms); + }, [perms]); + return null; + }; + return { ctxSeen, permsSeen, effectRuns, Probe }; +} + +const ROLES: RoleDefinition[] = [{ name: 'restricted', label: 'Restricted' } as RoleDefinition]; +const PERMISSIONS: ObjectPermissionConfig[] = [ + { + object: 'accounts', + roles: { + restricted: { + fieldPermissions: [{ field: 'secret', read: false, write: false }], + rowPermissions: [{ filter: 'owner_id = me' }], + }, + }, + } as unknown as ObjectPermissionConfig, +]; +const USER_ROLES = ['restricted']; + +const ME: MePermissionsResponse = { + authenticated: true, + userId: 'u-1', + tenantId: 't-1', + roles: ['restricted'], + permissionSets: ['ps-1'], + systemPermissions: ['manage_app'], + objects: { accounts: { allowRead: true, allowEdit: false, apiOperations: ['find'] } }, + fields: { 'accounts.secret': { readable: false, editable: false } }, +}; + +describe('the discard proxy really reaches the binding the providers use (objectui#6813)', () => { + it('provesTheProxyDiscriminates: an armed memo AND an armed callback are both discarded', () => { + const memos: unknown[] = []; + const callbacks: unknown[] = []; + const Probe: React.FC = () => { + memos.push(ReactNS.useMemo(() => ({}), [])); + callbacks.push(ReactNS.useCallback(() => {}, [])); + return null; + }; + + const restore = armDiscardProxy(); + try { + const { rerender } = render(); + // Armed but not fired: normal caching still holds, so a green below + // cannot be green because the proxy breaks caching outright. + rerender(); + expect(memos[1]).toBe(memos[0]); + expect(callbacks[1]).toBe(callbacks[0]); + + discardNow(); + rerender(); + } finally { + restore(); + } + expect(memos[2]).not.toBe(memos[1]); + expect(callbacks[2]).not.toBe(callbacks[1]); + }); +}); + +describe('PermissionProvider — ctx identity survives a discarded cache (objectui#6813)', () => { + it('keeps ONE ctx identity across a discard while the props are unchanged', () => { + const { ctxSeen, permsSeen, effectRuns, Probe } = makeProbe(); + const tree = () => ( + + + + ); + + const restore = armDiscardProxy(); + try { + const { rerender } = render(tree()); + rerender(tree()); + expect(new Set(ctxSeen).size).toBe(1); + + // Two discards, each followed by a re-render with the SAME props. + // Nothing an author or a caller controls has changed. + discardNow(); + rerender(tree()); + discardNow(); + rerender(tree()); + } finally { + restore(); + } + + expect(new Set(ctxSeen).size).toBe(1); + expect(new Set(permsSeen).size).toBe(1); + // The observable the card names, modelled at its source: the consuming + // effect must not re-run, so there is no redundant `dataSource.find`. + expect(effectRuns).toHaveLength(1); + }); + + it('keeps the member identities three consumers name in dependency arrays', () => { + const { ctxSeen, Probe } = makeProbe(); + const tree = () => ( + + + + ); + + const restore = armDiscardProxy(); + try { + const { rerender } = render(tree()); + discardNow(); + rerender(tree()); + } finally { + restore(); + } + + const first = ctxSeen[0]!; + const last = ctxSeen[ctxSeen.length - 1]!; + // `RecordDetailView`, `ObjectDataPage` and `ObjectView` each name + // `getObjectApiOperations` in a `useMemo` dependency array. + expect(last.getObjectApiOperations).toBe(first.getObjectApiOperations); + // `useFieldPermissions` names both of these in its own dependency arrays. + expect(last.checkField).toBe(first.checkField); + expect(last.getFieldPermissions).toBe(first.getFieldPermissions); + expect(last.check).toBe(first.check); + expect(last.getRowFilter).toBe(first.getRowFilter); + expect(last.hasCapabilities).toBe(first.hasCapabilities); + }); + + it('still publishes a NEW ctx when the permissions genuinely change, carrying the new answers', () => { + const { ctxSeen, effectRuns, Probe } = makeProbe(); + const OPEN: ObjectPermissionConfig[] = [ + { object: 'accounts', roles: { restricted: { fieldPermissions: [] } } } as unknown as ObjectPermissionConfig, + ]; + const tree = (permissions: ObjectPermissionConfig[]) => ( + + + + ); + + const { rerender } = render(tree(PERMISSIONS)); + rerender(tree(OPEN)); + + const first = ctxSeen[0]!; + const last = ctxSeen[ctxSeen.length - 1]!; + expect(last).not.toBe(first); + expect(effectRuns).toHaveLength(2); + // …and the new identity answers with the NEW permissions, not a stale + // snapshot: `secret` was denied under PERMISSIONS and is open under OPEN. + expect(first.checkField('accounts', 'secret', 'read')).toBe(false); + expect(last.checkField('accounts', 'secret', 'read')).toBe(true); + expect(first.getRowFilter('accounts')).toBe('owner_id = me'); + expect(last.getRowFilter('accounts')).toBeUndefined(); + }); + + it('answers exactly what it answered before the discard', () => { + const { ctxSeen, Probe } = makeProbe(); + const tree = () => ( + + + + ); + + const restore = armDiscardProxy(); + try { + const { rerender } = render(tree()); + discardNow(); + rerender(tree()); + } finally { + restore(); + } + + const readAll = (c: PermissionContextValue) => ({ + checkFieldOpen: c.checkField('accounts', 'name', 'read'), + checkFieldDenied: c.checkField('accounts', 'secret', 'read'), + fieldPerms: c.getFieldPermissions('accounts'), + rowFilter: c.getRowFilter('accounts'), + apiOps: c.getObjectApiOperations('accounts'), + roles: c.roles, + userId: c.userId, + systemPermissions: c.systemPermissions, + capabilities: c.hasCapabilities(['anything']), + isLoaded: c.isLoaded, + }); + const expected = { + checkFieldOpen: true, + checkFieldDenied: false, + fieldPerms: [{ field: 'secret', read: false, write: false }], + rowFilter: 'owner_id = me', + // [#3391] role-based provider models no effective API operation set. + apiOps: undefined, + roles: USER_ROLES, + // [objectui#5683] never learns who the user IS. + userId: null, + // [objectui#4656] unreported, NOT a reported-empty grant — and + // `hasCapabilities` stays fail-open on it. + systemPermissions: undefined, + capabilities: true, + isLoaded: true, + }; + expect(readAll(ctxSeen[0]!)).toEqual(expected); + expect(readAll(ctxSeen[ctxSeen.length - 1]!)).toEqual(expected); + }); + + it('two providers given the same inputs cannot evict each other', () => { + // This is what makes the cache immune rather than merely lucky: a single + // slot comparing a stored dependency list would be SHARED by both trees + // below, so each render would evict the other's entry and churn the very + // identity this card is about. Keying on the input tuple has no such slot. + const a = makeProbe(); + const b = makeProbe(); + const tree = () => ( + <> + + + + + + + + ); + + const restore = armDiscardProxy(); + try { + const { rerender } = render(tree()); + rerender(tree()); + discardNow(); + rerender(tree()); + } finally { + restore(); + } + + expect(new Set(a.ctxSeen).size).toBe(1); + expect(new Set(b.ctxSeen).size).toBe(1); + expect(b.ctxSeen[0]).toBe(a.ctxSeen[0]); + expect(a.effectRuns).toHaveLength(1); + expect(b.effectRuns).toHaveLength(1); + }); +}); + +describe('MePermissionsProvider — ctx identity survives a discarded cache (objectui#6813)', () => { + it('keeps ONE ctx identity across a discard while the fetched data is unchanged', () => { + const { ctxSeen, permsSeen, effectRuns, Probe } = makeProbe(); + const tree = () => ( + + + + ); + + const restore = armDiscardProxy(); + try { + const { rerender } = render(tree()); + rerender(tree()); + expect(new Set(ctxSeen).size).toBe(1); + + discardNow(); + rerender(tree()); + discardNow(); + rerender(tree()); + } finally { + restore(); + } + + expect(new Set(ctxSeen).size).toBe(1); + expect(new Set(permsSeen).size).toBe(1); + expect(effectRuns).toHaveLength(1); + }); + + it('keeps the member identities and the answers across a discard', () => { + const { ctxSeen, Probe } = makeProbe(); + const tree = () => ( + + + + ); + + const restore = armDiscardProxy(); + try { + const { rerender } = render(tree()); + discardNow(); + rerender(tree()); + } finally { + restore(); + } + + const first = ctxSeen[0]!; + const last = ctxSeen[ctxSeen.length - 1]!; + expect(last.getObjectApiOperations).toBe(first.getObjectApiOperations); + expect(last.checkField).toBe(first.checkField); + expect(last.getFieldPermissions).toBe(first.getFieldPermissions); + expect(last.check).toBe(first.check); + expect(last.getRowFilter).toBe(first.getRowFilter); + + const readAll = (c: PermissionContextValue) => ({ + checkAllowed: c.check('accounts', 'read').allowed, + checkDenied: c.check('accounts', 'update').allowed, + checkFieldDenied: c.checkField('accounts', 'secret', 'read'), + apiOps: c.getObjectApiOperations('accounts'), + rowFilter: c.getRowFilter('accounts'), + roles: c.roles, + userId: c.userId, + systemPermissions: c.systemPermissions, + capHeld: c.hasCapabilities(['manage_app']), + capMissing: c.hasCapabilities(['manage_billing']), + isLoaded: c.isLoaded, + }); + const expected = { + checkAllowed: true, + checkDenied: false, + checkFieldDenied: false, + apiOps: ['find'], + rowFilter: undefined, + roles: ['restricted'], + userId: 'u-1', + systemPermissions: ['manage_app'], + capHeld: true, + capMissing: false, + isLoaded: true, + }; + expect(readAll(first)).toEqual(expected); + expect(readAll(last)).toEqual(expected); + }); + + it('still publishes a NEW ctx when the fetched permissions genuinely change', async () => { + // Driven through the FETCH, which is the only way this provider's data + // actually changes: `initialPermissions` seeds `useState` and is ignored on + // every later render, so re-rendering with a different one would assert + // nothing (it measured exactly that until this comment was written). + const { ctxSeen, Probe } = makeProbe(); + const OPENED: MePermissionsResponse = { + ...ME, + objects: { accounts: { allowRead: true, allowEdit: true } }, + fields: {}, + }; + const fetcher = vi.fn(async (input: RequestInfo | URL) => + new Response(JSON.stringify(String(input).includes('/v2') ? OPENED : ME), { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }), + ) as unknown as typeof fetch; + const tree = (endpoint: string) => ( + + + + ); + + const { rerender } = render(tree('/v1/me/permissions')); + await waitFor(() => expect(ctxSeen.length).toBeGreaterThan(0)); + const first = ctxSeen[ctxSeen.length - 1]!; + expect(first.checkField('accounts', 'secret', 'write')).toBe(false); + expect(first.getObjectApiOperations('accounts')).toEqual(['find']); + + rerender(tree('/v2/me/permissions')); + await waitFor(() => expect(ctxSeen[ctxSeen.length - 1]).not.toBe(first)); + + const last = ctxSeen[ctxSeen.length - 1]!; + // A genuine permission change must reach every consumer — the cache keys on + // the fetched payload's identity, so a DIFFERENT payload cannot collide + // onto the entry the previous one made. + expect(last.checkField('accounts', 'secret', 'write')).toBe(true); + expect(last.getObjectApiOperations('accounts')).toBeUndefined(); + expect(last.check('accounts', 'update').allowed).toBe(true); + }); +}); diff --git a/packages/permissions/src/discardProofCache.ts b/packages/permissions/src/discardProofCache.ts new file mode 100644 index 0000000000..3c34ec0ca2 --- /dev/null +++ b/packages/permissions/src/discardProofCache.ts @@ -0,0 +1,105 @@ +/** + * 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#6813] A cache React cannot discard, keyed on the IDENTITIES of the + * inputs the cached value is derived from. + * + * ## Why this exists rather than `useMemo` / `useCallback` + * + * `useMemo` and `useCallback` carry no semantic guarantee: React is permitted + * to throw the cache away and recompute even when the dependency list compares + * equal. That is fine for an optimisation and wrong for a value another + * module's CORRECTNESS rests on — and a context value is exactly that, because + * consumers name it in dependency arrays and a moved identity re-runs their + * effects (`ListView`'s data fetch, `DetailView`'s gatedSchema, `ObjectForm`, + * `ModalForm`, `ObjectGrid`, `RelatedList`). + * + * ⚠️ This is a LATENT hazard, not a reproduction, and this file must not be + * read as fixing a bug. Measured on this repo's pinned React 19.2.8 while + * objectui#6724 was implemented: 51 re-renders with no provider, 51 with a + * provider and 42 under `StrictMode` each returned ONE identity, and this repo + * has no `Activity`/Offscreen subtree — the documented case where React does + * throw memo caches away. So React has not exercised the licence here. What + * this file removes is the dependency on it not doing so. + * + * ## Why a module-level `WeakMap` and not a `useRef` + * + * A ref would also survive a discard, but reading or writing `ref.current` + * during render is the shape `react-hooks/refs` flags and that objectui#6745 / + * #6797 were opened and closed to remove from published hooks. This costs no + * hook at all, so there is no render-phase ref write and no render-phase state + * adjustment to reason about — the same property objectui#6724 landed on at + * the hook end of this chain, where a module-level `WeakMap` keyed on `ctx` + * replaced a `useMemo` keyed on `[ctx]`. + * + * ## Why the whole tuple is the key, rather than a stored dependency list + * + * A single slot holding `{ deps, value }` and comparing deps would be shared + * by every component instance reaching it, so two providers with different + * inputs would evict each other and churn the identity on every render — the + * defect this file exists to remove. Keying a nested `WeakMap` on the full + * input tuple has no such slot to fight over: one value per distinct tuple, + * for as long as that tuple's members are alive. Two providers given the same + * inputs then share ONE value, which is strictly stronger than the + * per-instance memo it replaces and is the same guarantee objectui#6724 gives + * at the hook. + * + * ## Lifetime + * + * Every level is a `WeakMap`, so an entry is reachable only while the input + * objects that key it are. A caller that builds a fresh array each render + * allocates a fresh entry each render and drops the previous one — exactly + * what a memo miss costs today, with nothing retained. + * + * ⚠️ Keys must be objects: `WeakMap` cannot hold a primitive. Inputs that are + * legitimately absent (an optional prop) or primitive (a boolean) are mapped + * to a stable module-level sentinel by the CALLER, where the mapping is + * obvious and total — see `NO_USER` in `PermissionProvider` and `NO_DATA` / + * `LOADED` / `NOT_LOADED` in `MePermissionsProvider`. A generic coercion here + * could not do it safely: two distinct primitives would have to collide on one + * sentinel and silently answer with each other's cached value. + * + * Internal to `@object-ui/permissions` — deliberately not exported from + * `index.ts`. Whether this idiom should be shared repo-wide is a public-surface + * decision, not one this file makes. + */ + +/** One entry, boxed so that a legitimately falsy cached value still hits. */ +interface Entry { + value: T; +} + +/** + * Create one independent cache. Each cached thing needs its OWN cache — the + * key tuple identifies the inputs, not which value was derived from them, so + * two different values sharing a tuple would otherwise collide. + */ +export function createDiscardProofCache(): (keys: readonly object[], build: () => T) => T { + const root = new WeakMap(); + + return function lookup(keys: readonly object[], build: () => T): T { + let node = root; + for (let i = 0; i < keys.length - 1; i++) { + let next = node.get(keys[i]) as WeakMap | undefined; + if (next === undefined) { + next = new WeakMap(); + node.set(keys[i], next); + } + node = next; + } + + const last = keys[keys.length - 1]; + let entry = node.get(last) as Entry | undefined; + if (entry === undefined) { + entry = { value: build() }; + node.set(last, entry); + } + return entry.value; + }; +}