Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about spec authority. useCommittedRef implements the latest-value-ref rule that #10161 proposes for architecture:react-component-runtime (DEC-3 and an INV3 exception), but on main INV3 still requires every new Effect to name the external system it synchronizes, and this hook's insertion Effect names none. Either #10161 lands first, or an engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner. Non-blocking: the file header's @output and @position repeat the hook's JSDoc (useCommittedRef.ts:8-12), with a suggested trim inline.
Claims and files checked
- Claim: the hook writes
currentin a dependency-freeuseInsertionEffect, after each commit and before layout Effects: verified (useCommittedRef.ts:25-27; the layout-order test). - Claim: the quoted "values only" comment: verified (
useCommittedRef.ts:24). - Claim: not in
hooks/index.tsor the package entry, so no new public API: verified (Core's exports reach hooks only throughhooks/index.ts, which does not list it). - Claim: it lives beside the internal
useIsomorphicLayoutEffect: verified (packages/core/src/hooks/). - Claim: no callers move: verified (only its test imports it).
- Claim: no React floor change, and
useInsertionEffectexists in every supported React: verified (peer range>=19.0.0). - Claim: one internal patch Changeset for Core: verified.
- Claim: no workflow or CI script changes: verified.
- Claim: Core has about 176 latest-value refs, about 70 written during render: partly checked (72 render-body ref writes in Core; the total was not counted).
- Claim: six tests pass, with the listed cases: the six cases exist and assert what the description lists; not run.
- Claim: a render-phase variant fails both dropped-render tests: not run.
- Claim: the new tests fail on
mainbecause the module is missing: the module is absent onmain; not run. - Claim: ESLint, Prettier, typecheck, provenance, Changeset, knowledge and pre-review checks pass: not rerun; the pre-review ref-copy rule does not match
useInsertionEffect, so zero findings is expected. - Changeset: an internal hook that keeps a ref at the latest committed prop or callback, used by nothing, with nothing new for apps to import: verified.
- File
.changeset/core-use-committed-ref.md: clear. - File
packages/core/src/hooks/useCommittedRef.ts: waits on the spec decision above; non-blocking header note. - File
packages/core/src/hooks/useCommittedRef.test.tsx: clear.
| * @output Exports useCommittedRef, a read-only ref that holds the value of | ||
| * the last committed render | ||
| * @position Internal hook; not in hooks/index.ts or the package entry. Event | ||
| * and gesture callbacks read the latest committed prop or callback through | ||
| * it. Render and useMemo read the value directly, never the ref. |
There was a problem hiding this comment.
Non-blocking (architecture:knowledge-contracts INV29): @output and @position repeat the JSDoc at lines 18-20, and a file header carries no prose that repeats the main JSDoc. Suggested:
* @output Exports useCommittedRef
* @position Internal hook; not in hooks/index.ts or the package entryThen end the JSDoc with "never during render or in useMemo" so the useMemo rule stays.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about spec authority, and one fix is needed.
useCommittedRef implements the latest-value-ref rule that #10161 proposes for architecture:react-component-runtime (DEC-3 and an INV3 exception), but on main INV3 still requires every new Effect to name the external system it synchronizes, and this hook's insertion Effect names none. Either #10161 lands first, or an engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner.
- The JSDoc's Effect-cleanup example (
packages/core/src/hooks/useCommittedRef.ts:27-29) subscribes with the value read at setup and unsubscribes with the value read at cleanup, which differ once the value changes (the new test atuseCommittedRef.test.tsx:159-178shows the cleanup reading'second'after mounting with'first'), so code that copies it leaks the first subscription; please delete lines 21-30 and keep a value that selects a resource as an Effect dependency.
Non-blocking: the file header still repeats the hook's JSDoc (see the inline note on lines 8-12).
Claims and files checked
- Changeset: Core adds an internal hook that keeps a ref at the latest committed prop or callback for event and gesture callbacks: verified (
useCommittedRef.ts:32-39). - Changeset: nothing uses it yet: verified; only its own test imports it.
- Changeset: apps import nothing new: verified; it is not in
hooks/index.tsor anyexportsentry. - Description: it writes
currentinuseInsertionEffectwith no dependencies: verified (useCommittedRef.ts:35-37). - Description: the write survives a dropped render and lands before every layout Effect in the same commit: verified by reading the tests at
useCommittedRef.test.tsx:63-125; not run. - Description: event and gesture callbacks read it, render and
useMemonever do: contradicted at this head by the JSDoc cleanup paragraph (useCommittedRef.ts:22-30), the finding above. - Description: it sits beside
useIsomorphicLayoutEffectand outside the package entry: verified. - Description: no React floor change: verified; the peer range stays
react >=19.0.0. - Description: no callers move: verified.
- Description: seven tests, including a cleanup read in the lint-clean shape the JSDoc shows: seven cases are present; the test run and the lint statement were not reproduced.
packages/core/src/hooks/useCommittedRef.ts: blocking cleanup-example finding; non-blocking header note.packages/core/src/hooks/useCommittedRef.test.tsx: clear..changeset/core-use-committed-ref.md: clear.
| * In an Effect cleanup, read `current` through a function that the Effect | ||
| * body declares. A direct read in the cleanup trips | ||
| * `@eslint-react/exhaustive-deps`. | ||
| * | ||
| * ``` | ||
| * const getTarget = () => committed.current; | ||
| * subscribe(getTarget()); | ||
| * return () => unsubscribe(getTarget()); | ||
| * ``` |
There was a problem hiding this comment.
Blocking: this example subscribes with the value read in the Effect body and unsubscribes with the value read in the cleanup. The Effect depends only on the stable ref, so a changed value never resubscribes, and the new test at useCommittedRef.test.tsx:159-178 shows the cleanup reading 'second' after mounting with 'first'. Code that copies this leaks the first subscription, and the paragraph also contradicts line 20, which limits reads of current to event and gesture callbacks. Suggest deleting lines 21-30; a value that selects a resource stays an Effect dependency:
useEffect(() => {
subscribe(target);
return () => unsubscribe(target);
}, [target]);
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about spec authority, and two fixes are needed.
useCommittedRef and useCommittedRefInto implement the latest-value-ref rule that #10161 proposes for architecture:react-component-runtime (DEC-3 and an INV3 exception), but on main INV3 still requires every new Effect to name the external system it synchronizes, and #10161's DEC-3 names only the read-only useCommittedRef, not a shape that writes into a caller's useRef. Either #10161 lands first and covers both shapes (or useCommittedRefInto leaves this pull request), or an engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner.
- The JSDoc's Effect-cleanup example (
packages/core/src/hooks/useCommittedRef.ts:23-31) subscribes with the value read at setup and unsubscribes with the value read at cleanup, which differ once the value changes (the test atuseCommittedRef.test.tsx:193-212shows cleanup reading'second'after mounting with'first'), so please delete the paragraph and that test and keep a value that selects a resource as an Effect dependency. - The description says the first four evidence checks run for both call shapes, but the listener-installed-once and Effect-cleanup tests sit in
describe('useCommittedRef')only (useCommittedRef.test.tsx:160-225), so please run the listener test for both shapes throughdescribe.each(SHAPES)or reword that line.
Non-blocking: the file header still repeats the hooks' JSDoc (see the inline note on lines 8-13).
Claims and files checked
- Changeset: Core adds an internal hook that keeps a ref at the latest committed prop or callback for event and gesture callbacks: verified (
useCommittedRef.ts:33-51). - Changeset: nothing uses it yet, and apps import nothing new: verified; only its own test imports the file, and no Core entry point reaches it.
- Changeset: a
[chore] Internal:patch for@astryxdesign/core: verified. - Description: both shapes write
currentin a no-dependencyuseInsertionEffect, never during render: verified (useCommittedRef.ts:33-37,:45-51). - Description: the two call signatures: verified (
useCommittedRef.ts:33,:45). - Description: the write's comment text: verified (
useCommittedRef.ts:46). - Description: the hook sits beside the internal
useIsomorphicLayoutEffect, outsidehooks/index.tsand the package entry: verified. - Description: no callers move: verified.
- Description: no React floor change, since Core's
react >=19.0.0peer range shipsuseInsertionEffect: verified (packages/core/package.json). - Description: no new public API: verified from the exports map and source.
- Description: 12 tests: verified by count.
- Description: each of the first four checks runs for both call shapes: false; the listener and cleanup tests cover
useCommittedRefonly. - Description: the cleanup read is the lint-clean shape the JSDoc shows: accurate as written, but the shape releases a different value than it acquired (blocking finding above).
- Description:
useCommittedRefIntolets a callback declared before the value read the latest committed value: verified (useCommittedRef.test.tsx:227-251). - Description: a render-phase variant fails both dropped-render tests: not checked.
- Description: about 176 latest-value refs in Core, 70 written during render: not checked; a codebase count outside this change.
- File
.changeset/core-use-committed-ref.md: clear. - File
packages/core/src/hooks/useCommittedRef.ts: cleanup example (blocking); header repeats the JSDoc (non-blocking). - File
packages/core/src/hooks/useCommittedRef.test.tsx: the cleanup test pins the example; the listener and cleanup tests cover one shape.
| }); | ||
| }); | ||
|
|
||
| describe('useCommittedRef', () => { |
There was a problem hiding this comment.
Blocking: the description says the listener-installed-once and Effect-cleanup checks run for both call shapes, but this block runs them for useCommittedRef only, so useCommittedRefInto (the shape meant for callbacks) has no listener test. Move the listener test into describe.each(SHAPES) above (the useIntoShape wrapper already serves it), or reword the description to name the checks that run for both shapes.
|
/approve-spec 8fde373 |
|
Decision: D-20261112-150 |
Records: #10161 (merged) and #11368. #11368 should merge first or together with this PR.
Maintainer impact
Core has about 176 latest-value refs: a ref that holds the newest prop or callback, so a listener installed once can call the current one. Each one decides on its own when it writes, and 70 of them write during render. A render that React drops, for example a transition that suspends, leaves its value in such a ref, so a later click can call a handler for a screen the user never saw. This PR adds the shared internal hooks that
architecture:react-component-runtimeINV3 and DEC-3 name, so components can move onto one commit-safe timing.Intended invariant
architecture:react-component-runtimeINV3, INV17 and DEC-3, as #10161 (merged) and #11368 state them. A latest-value ref goes only throughuseCommittedRef, which writescurrentin a commit-phase insertion Effect and never during render. Event and gesture callbacks read it; render anduseMemonever do. The hooks live inpackages/core/src/hooks/useCommittedRef.ts, beside the internaluseIsomorphicLayoutEffect, and are never inhooks/index.tsor the package entry. Lab reaches them through@astryxdesign/core/internal/committed-ref.Change
packages/core/src/hooks/useCommittedRef.tshas two call shapes. Both writecurrentinuseInsertionEffectwith no dependencies, so the write happens after every commit and before any layout Effect in that commit:useCommittedRef<T>(value: T): { readonly current: T }returns a read-only ref.useCommittedRefInto<T>(ref: { current: T }, value: T): voidwrites into a ref that the component created earlier withuseRef. This is for callbacks declared above the value, where the value cannot exist yet when the ref is made.hooks/index.ts, and the package entry exports neither.packages/core/src/internal/committed-ref.ts: the internal entry@astryxdesign/core/internal/committed-ref, which re-exports both hooks by name for Lab. It is added toscripts/sync-exports.jsand the generatedexportsmap, like the other internal entries. Lint allows it only in Core and Lab.useInsertionEffectships in every React version Core supports (>=19.0.0).[chore] Internal:Changeset for@astryxdesign/core.New public API: none.
Evidence
mainbecause the module did not exist. A render-phase variant of the same write fails both dropped-render tests: the ref holds the dropped value.renderToStringgives no warning and keeps the initial value.useCommittedRefInto, a callback declared before the value reads the latest committed value.Scope
Components: none (internal hook)
Testing
vitest run packages/core/src/hooks/useCommittedRef.test.tsx.tsc.no-comment-provenance.baseline.mjs --check.pnpm check:changesetsandpnpm check:knowledge.pnpm review:pre: 0 findings, merges cleanly.