Uh oh!
There was an error while loading. Please reload this page.
perf(permissions): build both providers' context value where React cannot discard it - #6863
Merged
Merged
Conversation
…nnot discard it 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013hfmP9hoMd3dJwTh85J4yB
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
os-sam
marked this pull request as ready for review
August 30, 2026 06:55
os-sam
enabled auto-merge
August 30, 2026 06:55
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes#6813
Both permission providers built their context value in a
useMemo—PermissionProviderover fouruseCallbacks,MePermissionsProviderover six. Neither carries a semantic guarantee: React may discard the cache and recompute even when the dependency list compares equal, and every one of those factories builds a fresh object. A discard therefore handedPermCtx.Providera NEW context value with every permission it carries unchanged, which moves the keyusePermissions()caches on after #6819 and re-runs the whole consumer chain.This is hardening, not a repair
Nothing misbehaves today, and the card proves it rather than asserting it. 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
StrictModeeach returned ONE identity — and this repo has noActivity/Offscreen subtree, the documented case where React does throw memo caches away. What this PR removes is the dependency on React continuing not to exercise a licence it holds. The changeset, the code comments and the pin header all say so; none of them describe a bug being fixed. TypeTask, notBug.The route
Per triage, no value key was derived and none is available: the context carries verdict FUNCTIONS over an open set of object/field names, which flatten to no fixed primitive list. The route taken is the one #6819 landed for
usePermissions()one link down the chain — cache the value where React cannot drop it, keyed on the inputs it is derived from.packages/permissions/src/discardProofCache.ts(new, internal, deliberately NOT exported fromindex.ts) is that cache: a nested module-levelWeakMapkeyed on the identities of the inputs. Both providers use the one idiom.Two design points worth a reviewer's attention, both documented at the call sites:
WeakMap, notuseRef. A ref would also survive a discard, but reading or writingref.currentduring render is the shapereact-hooks/refsflags and that finding(react): useSchemaPersistence writes adapterRef.current during render — three react-hooks/refs warnings on a published hook #6745 / finding(react): three more react-hooks/refs warnings on published hooks — useETagCache, useGlobalUndo, useOffline #6797 were opened and closed to remove from published hooks. This costs no hook at all — the same property fix(permissions): give usePermissions() a return identity React cannot discard #6819's landed comment gives as its reason: no render-phase ref write, no render-phase state adjustment. Measured:pnpm exec eslint packages/permissions --format jsonreports 0 errors and 28 warnings both before and after this change, with an identical rule breakdown and noreact-hooks/refsin either.{ deps, value }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 very defect this PR removes. Keying the trie on the full tuple has no such slot to fight over. There is a test for it.Dependency sets are unchanged. Each cached member keeps exactly the inputs its old
useCallbacknamed, which matters:useFieldPermissionsnamescheckFieldandgetFieldPermissionsin its own dependency arrays, andRecordDetailView/ObjectDataPage/ObjectVieweach namegetObjectApiOperations. Folding the members into one value-level key would have churned those on arolesoruserchange that today leaves them alone. Nothing here churns more often than it did.Three members that were rebuilt per value and can never vary are now module constants —
PermissionProvider'sgetObjectApiOperationsandhasCapabilities, andMePermissionsProvider'sgetRowFilter(previouslyuseCallback(..., []), which React may also discard). That is strictly stabler for the three consumers naminggetObjectApiOperationsin a dependency array.No published export changes and the context carries exactly what it carried before.
Premise check against
origin/mainThe card's line numbers held on my own base (
1e14d70ae):PermissionProvider.tsx:124over fouruseCallbacks,MePermissionsProvider.tsx:285over six. The consumer chain re-measured on the same base: 9 dependency arrays across 6 files name the wholepermsobject —ListView(2, one of them the data-fetch effect),ObjectForm(3),RelatedList,DetailView,ModalForm,ObjectGrid. The 6 files match the census in #6819's pin header exactly; the array count differs from the 13 recorded there because this pattern matches single-line dependency closers only.The pin FORCES a discard
packages/permissions/src/__tests__/providerCtxIdentity.discarded.test.tsx— a pin that does not force one proves nothing here, since React will not discard on its own. It reuses the module-level proxy technique fromusePermissions.discardedIdentity.test.tsx(#6819) andListView.discardedExpandFieldsMemo.test.tsx(#6697), with one difference that matters: it patchesuseCallbackas well asuseMemo, and discards EVERY armed cache rather than ones matched by a marker dependency — because the fix removes the dependency arrays altogether, so a marker-matched proxy would have nothing left to match and would go green for the trivial reason.The first case proves the proxy really reaches the binding the providers use, and proves that arming alone does not break caching, so the greens below cannot be green for either trivial reason.
Reverse verification (ablation). Both provider files reverted to
1e14d70aewith the pin left in place, run from a committed state, mutation confirmed on disk by marker counts and blob hashes before measuring, restore confirmed by blob hash equality against theHEADblobs and an emptygit diff HEAD:The 5 reds are exactly the identity cases (
expected 3 to be 1— three distinct context identities across two discards). The 4 that stay green in both states are the controls: the proxy canary, both genuine-change cases, and the answers-unchanged case.Verification
Run on
cee2b98(the final commit), from the repo root —pnpm --filter <pkg> testis refused here by the objectui#3378 guard:The typecheck was confirmed to actually cover the new files rather than excluding them:
tsc -p tsconfig.test.json --listFilesnames bothdiscardProofCache.tsand the new pin.Downstream consumers of the changed identity semantics — every file in the repo mounting either provider or
PermCtx.Provider:Gates derived from this repo's own
package.jsonand.github/workflows/for the paths this diff actually touches (objectstack'sdispatch-gates.mjsanswers only about its own tree and was not used):Declared narrowing. The repo-wide
pnpm lintwas not run here; lint was measured onpackages/permissionsonly, and the base-vs-head comparison above is what makes that a measurement rather than a gap. CI runs the full farm regardless.check:published-distis RED, and not from this branch: it reports one finding in@object-ui/fieldsthat is present onorigin/mainand unrelated topackages/permissions. Filed as #6861. That gate carries nopull_requesttrigger by design, so it will not redden this PR — but it does block the release path.Out-of-scope findings
@object-ui/fieldsemitsdist/__tests__/numberInputBrowserReadings.d.tsinto its published tarball, socheck:published-distis red on the release path #6861 —@object-ui/fieldsemitsdist/__tests__/numberInputBrowserReadings.d.tsinto its published tarball; its tsconfig excludes tooling by NAME (**/*.test.ts) and not by DIRECTORY.MePermissionsProvider'sfetchPermissionsis auseCallbackthe fetch effect depends on, so a discard costs a redundant/me/permissionsround trip. Same family, the effect end rather than the value end; deliberately not folded in here, because the input-keyed idiom this PR introduces does not transfer unchanged to four dependencies that include an optionalfetcherand three primitives.Neither is touched by this PR.
Generated by Claude Code
Generated by Claude Code