diff --git a/.changeset/6724-usepermissions-stable-identity.md b/.changeset/6724-usepermissions-stable-identity.md new file mode 100644 index 0000000000..0eb9da7766 --- /dev/null +++ b/.changeset/6724-usepermissions-stable-identity.md @@ -0,0 +1,47 @@ +--- +'@object-ui/permissions': patch +--- + +`usePermissions()` now returns an identity React cannot discard (objectui#6724). + +The hook cached its return in a `useMemo` keyed on `[ctx]`, and both branches build a +fresh object — an object literal when no provider is mounted, a spread of `ctx` when one +is. `useMemo` carries no semantic guarantee: React may throw the cache away and recompute +even when `[ctx]` compares equal, and that hands the caller a new identity while every +permission it carries is unchanged. + +That matters because consumers name this value in dependency arrays — 13 arrays across 6 +files: `ListView`'s data-fetch effect (`perms`), `DetailView`'s `gatedSchema`, +`ObjectForm`, `ModalForm`, `ObjectGrid`, `RelatedList`. A discard alone re-ran the fetch +effect and re-issued `dataSource.find` with nothing an author or a caller controls having +changed. Same family as objectui#6018 / #5976 / #6591 / #6592 / #6697. + +The by-identity dependency at the consumers is the correct shape and stays: what they read +off this object is the verdict FUNCTIONS (`checkField(object, field, 'read')`, +`can(object, 'update')`) over an open set of field names, which flatten to no fixed list of +primitives the way objectui#6592's `dataConfig` members did. So the fix is at the hook, +where the identity can be made trustworthy: + +- the decoration becomes a plain function of `ctx` — the same context value always yields + the same object, because the mapping lives in a module-level `WeakMap` React has no say + over, keyed weakly so it dies with the provider's value. That is strictly stronger than + the memo it replaces: the identity is now stable across every component reading the same + provider, not just across one component's re-renders. It also costs no hook, so there is + no render-phase ref write and no state adjustment to reason about. +- the no-provider answer becomes one shared frozen module constant. Every member is a pure + constant function, so there was never anything per-instance to keep, and a single frozen + object cannot churn in any component for any reason. + +A new context value still produces a new identity, on purpose: that is a real permission +change and every consumer must see it. + +No permission value moves: the returned object still spreads `ctx` by identity and derives +`can`/`cannot` from `ctx.check`, and the documented no-provider fallbacks (`isLoaded: +false`, `userId: null`, `systemPermissions: undefined` with `hasCapabilities` fail-open — +objectui#5683 / #4656) answer exactly as before. + +Measured while fixing, and worth recording: on React 19.2.8 this repo has no reproduction — +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. This closes a latent hazard, +not an observed re-fetch. The providers' own context-value memos are the remaining link in +the same chain (objectui#6813). diff --git a/packages/permissions/src/__tests__/usePermissions.discardedIdentity.test.tsx b/packages/permissions/src/__tests__/usePermissions.discardedIdentity.test.tsx new file mode 100644 index 0000000000..a15bd7b656 --- /dev/null +++ b/packages/permissions/src/__tests__/usePermissions.discardedIdentity.test.tsx @@ -0,0 +1,358 @@ +/** + * 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#6724 — `usePermissions()`'s returned identity must not churn while + * the permission context is unchanged. + * + * Consumers put this hook's return value straight into dependency arrays: + * `ListView`'s data-fetch effect names it as `perms`, and `DetailView`, + * `ObjectForm`, `ModalForm`, `ObjectGrid` and `RelatedList` name it in memo + * deps (13 dependency arrays across 6 files, measured on `26896c689`). The + * hook used to cache its return in a `useMemo` keyed on `[ctx]`, and BOTH of + * its branches build a fresh object — an object literal with no provider, a + * spread of `ctx` with one. `useMemo` carries no semantic guarantee: React + * may discard the cache and recompute even when `[ctx]` compares equal, so a + * discard alone moved the identity while every permission it carries stayed + * the same, and the consuming effect re-ran. + * + * ⚠️ WHAT IS AND IS NOT OBSERVABLE TODAY — measured here rather than assumed, + * because the card reasons from React's documented licence and not from a + * reproduction. On React 19.2.8 (this repo's pinned version) the cache is NOT + * discarded spontaneously: 51 re-renders with no provider, 51 with one, and + * 42 under `StrictMode` all returned ONE identity. There is no `` / + * Offscreen subtree in this repo either, which is the documented case where + * React does throw memo caches away. So this is a LATENT hazard — a + * correctness dependency resting on a licence React has not yet exercised + * here — not a bug reproducible from user actions today. The discard is + * forced below by a proxy, which is the only way to exercise it; the first + * case proves the proxy really reaches the binding the hook uses, so the + * greens below cannot be green for the trivial reason. + * + * The discard has to be forced at the MODULE level: the hook reached + * `useMemo` through its own `import { useMemo } from 'react'` binding, and + * `vi.spyOn`/assignment/`defineProperty` on the frozen `[object Module]` + * namespace all fail to patch it — silently leaving any pin built on them + * unfalsifiable. Same technique and same reason as + * `plugin-list/src/__tests__/ListView.discardedExpandFieldsMemo.test.tsx` + * (objectui#6697). + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { cleanup, render } from '@testing-library/react'; +import * as ReactNS from 'react'; +import type { FieldLevelPermission } from '@object-ui/types'; +import { PermCtx, type PermissionContextValue } from '../PermissionContext'; +import { usePermissions } from '../usePermissions'; + +const memoProxy = vi.hoisted(() => ({ markers: [] as unknown[], epoch: 0 })); + +vi.mock('react', async (importOriginal) => { + // `` matches the sibling pins (objectui#6697) and is load-bearing: a + // precise module type makes `realUseMemo`'s deps parameter `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 } }; +}); + +/** Put memos whose deps name one of `markers` under this file's control. */ +function armDiscardProxy(markers: unknown[]): () => void { + memoProxy.markers = markers; + return () => { + memoProxy.markers = []; + }; +} +/** Throw away the armed memos' caches — one discard event, on demand. */ +function discardNow(): void { + memoProxy.epoch += 1; +} + +/** + * The marker for the provider case: the context VALUE itself, which is the + * hook's only memo dependency. Held as a module constant so its IDENTITY is + * what the proxy matches, and so a real `PermissionProvider` (whose own + * `value` memo is a separate link in this chain — see the header) is not in + * the way of what this file measures. + */ +const SECRET_FIELD_PERMISSION: FieldLevelPermission = { + field: 'secret', + read: false, + write: false, +}; + +const CTX: PermissionContextValue = { + check: (object, action) => ({ allowed: !(object === 'locked' && action === 'update') }), + checkField: (_object, field) => field !== 'secret', + getFieldPermissions: () => [SECRET_FIELD_PERMISSION], + getRowFilter: (object) => (object === 'accounts' ? 'owner_id = me' : undefined), + getObjectApiOperations: () => ['find', 'update'], + roles: ['admin'], + userId: 'u-1', + systemPermissions: ['manage_app'], + hasCapabilities: (required) => required.every((c) => c === 'manage_app'), + isLoaded: true, +}; + +/** A second, DIFFERENT context — a genuine change the hook must still see. */ +const CTX_B: PermissionContextValue = { + ...CTX, + checkField: () => false, + roles: ['viewer'], + userId: 'u-2', +}; + +type Perms = ReturnType; + +/** Records every value the hook returns, plus each run of an effect keyed on it. */ +function makeProbe() { + const seen: Perms[] = []; + const effectRuns: Perms[] = []; + const Probe: React.FC = () => { + const perms = usePermissions(); + seen.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 { seen, effectRuns, Probe }; +} + +afterEach(() => { + cleanup(); + memoProxy.markers = []; +}); + +describe('usePermissions — the returned identity survives a discarded memo cache (objectui#6724)', () => { + it('provesTheProxyDiscriminates: the proxy reaches the same React binding the hook uses', () => { + const MARKER = 'canary-marker'; + const seen: unknown[] = []; + const Probe: React.FC = () => { + seen.push(ReactNS.useMemo(() => ({}), [MARKER])); + return null; + }; + + const restore = armDiscardProxy([MARKER]); + try { + const { rerender } = render(); + // Armed but not fired: normal caching still holds. + rerender(); + expect(seen[1]).toBe(seen[0]); + + discardNow(); + rerender(); + } finally { + restore(); + } + expect(seen[2]).not.toBe(seen[1]); + }); + + it('keeps ONE identity across a discard while `ctx` is unchanged (provider mounted)', () => { + const { seen, effectRuns, Probe } = makeProbe(); + const tree = () => ( + + + + ); + + const restore = armDiscardProxy([CTX]); + try { + const { rerender } = render(tree()); + rerender(tree()); + expect(new Set(seen).size).toBe(1); + + // One discard, then a re-render with the SAME context value. Nothing an + // author or a caller controls has changed. + discardNow(); + rerender(tree()); + } finally { + restore(); + } + + expect(new Set(seen).size).toBe(1); + // The observable the card names, modelled at its source: the consuming + // effect must not re-run. + expect(effectRuns).toHaveLength(1); + }); + + it('keeps ONE identity across a discard with NO provider mounted', () => { + const { seen, effectRuns, Probe } = makeProbe(); + + // With no provider the hook's only memo dependency WAS `ctx === null`, so + // `null` is the marker that reaches it. Nothing else in this tree memoises + // on `null` — the probe is the whole tree. + const restore = armDiscardProxy([null]); + try { + const { rerender } = render(); + rerender(); + expect(new Set(seen).size).toBe(1); + + discardNow(); + rerender(); + } finally { + restore(); + } + + expect(new Set(seen).size).toBe(1); + expect(effectRuns).toHaveLength(1); + }); + + it('still hands back a NEW identity when the context value genuinely changes', () => { + const { seen, effectRuns, Probe } = makeProbe(); + const tree = (ctx: PermissionContextValue) => ( + + + + ); + + const { rerender } = render(tree(CTX)); + rerender(tree(CTX_B)); + + expect(seen[seen.length - 1]).not.toBe(seen[0]); + expect(effectRuns).toHaveLength(2); + // …and the new identity carries the NEW answers, not a stale snapshot. + expect(seen[0].checkField('accounts', 'name', 'read')).toBe(true); + expect(seen[seen.length - 1].checkField('accounts', 'name', 'read')).toBe(false); + expect(seen[seen.length - 1].userId).toBe('u-2'); + }); +}); + +describe('usePermissions — the permission VALUES are untouched by the identity fix (objectui#6724)', () => { + /** Every answer, read off one render, asserted against `CTX` itself. */ + const readAll = (p: Perms) => ({ + checkAllowed: p.check('accounts', 'update').allowed, + checkDenied: p.check('locked', 'update').allowed, + checkFieldOpen: p.checkField('accounts', 'name', 'read'), + checkFieldDenied: p.checkField('accounts', 'secret', 'read'), + fieldPerms: p.getFieldPermissions('accounts'), + rowFilter: p.getRowFilter('accounts'), + rowFilterNone: p.getRowFilter('contacts'), + apiOps: p.getObjectApiOperations('accounts'), + roles: p.roles, + userId: p.userId, + systemPermissions: p.systemPermissions, + capHeld: p.hasCapabilities(['manage_app']), + capMissing: p.hasCapabilities(['manage_billing']), + isLoaded: p.isLoaded, + can: p.can('accounts', 'update'), + canDenied: p.can('locked', 'update'), + cannot: p.cannot('locked', 'update'), + cannotAllowed: p.cannot('accounts', 'update'), + }); + + it('answers exactly what the context answers — before AND after a discard', () => { + const { seen, Probe } = makeProbe(); + const tree = () => ( + + + + ); + + const restore = armDiscardProxy([CTX]); + let after: Perms; + try { + const { rerender } = render(tree()); + discardNow(); + rerender(tree()); + after = seen[seen.length - 1]; + } finally { + restore(); + } + + const expected = { + checkAllowed: true, + checkDenied: false, + checkFieldOpen: true, + checkFieldDenied: false, + fieldPerms: [SECRET_FIELD_PERMISSION], + rowFilter: 'owner_id = me', + rowFilterNone: undefined, + apiOps: ['find', 'update'], + roles: ['admin'], + userId: 'u-1', + systemPermissions: ['manage_app'], + capHeld: true, + capMissing: false, + isLoaded: true, + // `can`/`cannot` are derived from `check`, and must stay derived. + can: true, + canDenied: false, + cannot: true, + cannotAllowed: false, + }; + expect(readAll(seen[0])).toEqual(expected); + expect(readAll(after)).toEqual(expected); + // Every member the context itself defines is passed through by identity — + // the spread is intact, not re-implemented. + expect(after.check).toBe(CTX.check); + expect(after.checkField).toBe(CTX.checkField); + expect(after.getFieldPermissions).toBe(CTX.getFieldPermissions); + expect(after.getRowFilter).toBe(CTX.getRowFilter); + expect(after.getObjectApiOperations).toBe(CTX.getObjectApiOperations); + expect(after.hasCapabilities).toBe(CTX.hasCapabilities); + expect(after.roles).toBe(CTX.roles); + }); + + it('keeps the documented no-provider fallback answers, and shares ONE frozen object', () => { + const { seen: seenA, Probe: ProbeA } = makeProbe(); + const { seen: seenB, Probe: ProbeB } = makeProbe(); + render( + <> + + + , + ); + const p = seenA[0]; + + expect(p.isLoaded).toBe(false); + expect(p.check('accounts', 'update')).toEqual({ allowed: true }); + expect(p.checkField('accounts', 'secret', 'read')).toBe(true); + expect(p.getFieldPermissions('accounts')).toEqual([]); + expect(p.getRowFilter('accounts')).toBeUndefined(); + expect(p.getObjectApiOperations('accounts')).toBeUndefined(); + expect(p.roles).toEqual([]); + // [objectui#5683] identity unknown, not "anonymous". + expect(p.userId).toBeNull(); + // [objectui#4656] unreported, NOT a reported-empty grant — and + // `hasCapabilities` stays fail-open on it. + expect(p.systemPermissions).toBeUndefined(); + expect(p.hasCapabilities(['manage_app'])).toBe(true); + expect(p.can('accounts', 'update')).toBe(true); + expect(p.cannot('accounts', 'update')).toBe(false); + + // One shared answer, and frozen: it is no longer a per-call literal, so a + // consumer must not be able to mutate everyone else's copy. + expect(seenB[0]).toBe(p); + expect(Object.isFrozen(p)).toBe(true); + expect(Object.isFrozen(p.roles)).toBe(true); + }); + + it('hands the SAME identity to two components reading the same context value', () => { + const { seen: seenA, Probe: ProbeA } = makeProbe(); + const { seen: seenB, Probe: ProbeB } = makeProbe(); + render( + + + + , + ); + // One decorated object per context value, not per component instance — + // stronger than the per-instance memo it replaces, and the property that + // makes the identity a function of the permissions themselves. + expect(seenB[0]).toBe(seenA[0]); + expect(seenA[0].can('accounts', 'update')).toBe(true); + expect(seenB[0].can('locked', 'update')).toBe(false); + }); +}); diff --git a/packages/permissions/src/usePermissions.ts b/packages/permissions/src/usePermissions.ts index 9c74b565f8..247907b355 100644 --- a/packages/permissions/src/usePermissions.ts +++ b/packages/permissions/src/usePermissions.ts @@ -6,52 +6,113 @@ * LICENSE file in the root directory of this source tree. */ -import { useContext, useMemo } from 'react'; +import { useContext } from 'react'; import type { PermissionAction, PermissionCheckResult } from '@object-ui/types'; import { PermCtx, type PermissionContextValue } from './PermissionContext.js'; -/** - * Hook to access the permission system. - * Must be used within a PermissionProvider. - */ -export function usePermissions(): PermissionContextValue & { +/** What `usePermissions()` hands back: the context plus the two conveniences. */ +type PermissionsWithHelpers = PermissionContextValue & { /** Convenience: check if action is allowed */ can: (object: string, action: PermissionAction) => boolean; /** Convenience: check if action is denied */ cannot: (object: string, action: PermissionAction) => boolean; -} { - const ctx = useContext(PermCtx); +}; + +/** Shared, frozen: the no-provider answer holds no roles and nobody may add one. */ +const NO_ROLES = Object.freeze([]) as unknown as string[]; + +/** + * [objectui#6724] The no-provider answer, as ONE module-level object rather + * than a fresh literal per call. Every member is a pure constant function, so + * there is nothing per-instance to keep — and a single frozen object is the + * strongest identity guarantee available: it cannot churn for any reason, in + * any component, ever. Frozen because it is now shared: a consumer that + * mutated its own copy used to affect only itself. + */ +const NO_PROVIDER_PERMISSIONS: PermissionsWithHelpers = Object.freeze({ + check: (): PermissionCheckResult => ({ allowed: true }), + checkField: () => true, + getFieldPermissions: () => [], + getRowFilter: () => undefined, + getObjectApiOperations: () => undefined, + roles: NO_ROLES, + // [objectui#5683] No provider → identity unknown, defer to the server. + userId: null, + // [objectui#4656] No provider mounted at all → no answer, not "holds + // nothing". `undefined` matches MePermissionsProvider's own signal + // for an unreported backend and keeps `hasCapabilities` fail-open. + systemPermissions: undefined, + hasCapabilities: () => true, + isLoaded: false, + can: () => true, + cannot: () => false, +}); + +/** + * [objectui#6724] One decorated object per context value, for the life of that + * context value. A `WeakMap` keyed on `ctx` holds the entry only as long as + * the provider's own value is reachable, so nothing here outlives the render + * tree that produced it. + */ +const DECORATED = new WeakMap(); - // Memoize the returned object so consumers that include `usePermissions()` - // in dependency arrays don't re-run on every render. Without this, - // downstream `useMemo`/`useEffect` deps see a fresh object each render and - // can enter infinite update loops (see DetailView gatedSchema → data - // fetch effect, which would re-fire on every render otherwise). - return useMemo(() => { - if (!ctx) { - return { - check: (): PermissionCheckResult => ({ allowed: true }), - checkField: () => true, - getFieldPermissions: () => [], - getRowFilter: () => undefined, - getObjectApiOperations: () => undefined, - roles: [], - // [objectui#5683] No provider → identity unknown, defer to the server. - userId: null, - // [objectui#4656] No provider mounted at all → no answer, not "holds - // nothing". `undefined` matches MePermissionsProvider's own signal - // for an unreported backend and keeps `hasCapabilities` fail-open. - systemPermissions: undefined, - hasCapabilities: () => true, - isLoaded: false, - can: () => true, - cannot: () => false, - }; - } - return { - ...ctx, - can: (object: string, action: PermissionAction) => ctx.check(object, action).allowed, - cannot: (object: string, action: PermissionAction) => !ctx.check(object, action).allowed, - }; - }, [ctx]); +function withHelpers(ctx: PermissionContextValue): PermissionsWithHelpers { + const cached = DECORATED.get(ctx); + if (cached) return cached; + const decorated: PermissionsWithHelpers = { + ...ctx, + can: (object: string, action: PermissionAction) => ctx.check(object, action).allowed, + cannot: (object: string, action: PermissionAction) => !ctx.check(object, action).allowed, + }; + DECORATED.set(ctx, decorated); + return decorated; +} + +/** + * Hook to access the permission system. + * Must be used within a PermissionProvider. + * + * ## Why the identity is cached outside React, not in a `useMemo` + * + * Consumers put this hook's return value straight into dependency arrays — + * `ListView`'s data-fetch effect (`perms`), `DetailView`'s `gatedSchema` memo, + * `ObjectForm`, `ModalForm`, `ObjectGrid`, `RelatedList` (13 arrays across 6 + * files). Without a cache those deps see a fresh object every render and + * re-fire on every render; that is the infinite-update loop this cache has + * always existed to stop. + * + * It used to be a `useMemo` keyed on `[ctx]`, and that is the wrong tool for a + * dependency other code's CORRECTNESS rests on (objectui#6724, the family of + * #6018 / #5976 / #6591 / #6592 / #6697). `useMemo` is a pure optimisation + * carrying no semantic guarantee: React is permitted to discard the cache and + * recompute even when `[ctx]` compares equal, and BOTH branches build a fresh + * object — an object literal with no provider, a spread of `ctx` with one. A + * discard therefore moved the identity while every permission it carries + * stayed the same, and the consuming fetch effect re-ran: an extra + * `dataSource.find` with nothing an author or a caller controls having + * changed. + * + * What replaces it is not another React cache but a plain function of `ctx`: + * the same context value always yields the same decorated object, because the + * mapping lives in a module-level `WeakMap` React has no say over. That is + * strictly stronger than the memo it replaces — the identity is now stable + * across every component reading the same provider, not just across one + * component's re-renders — and it costs no hook, so there is no render-phase + * ref write and no state adjustment to reason about (objectui#6745 / #6797 + * are open on exactly that smell in published hooks). + * + * ⚠️ The guarantee is "one identity per context value" — which is what the + * consumers need, since what they read off this object is the VERDICT + * FUNCTIONS (`checkField(object, field, 'read')`, `can(object, 'update')`) + * over an open set of field names. There is no fixed list of primitives those + * flatten to, so the by-identity dependency at the consumers is the correct + * shape and stays; this hook is where the identity is made trustworthy. A new + * context value still produces a new identity, on purpose: that is a real + * permission change and every consumer must see it. The providers' own + * context-value memos are the remaining link in that chain and are not + * addressed here (objectui#6813). + */ +export function usePermissions(): PermissionsWithHelpers { + const ctx = useContext(PermCtx); + return ctx ? withHelpers(ctx) : NO_PROVIDER_PERMISSIONS; }