Conversation
…r-blocks-timing This PR adds two new hooks to `@khanacademy/wonder-blocks-timing` that complete the hook-based API for the timing package. - Adds `useAnimationFrame`: a hook wrapping `requestAnimationFrame`/`cancelAnimationFrame` that follows the same proxy-ref and policy pattern as the existing `useTimeout` and `useInterval` hooks — supports `SchedulePolicy`, `ClearPolicy`, and `ActionPolicy` - Adds `useActionScheduler`: a hook-based alternative to the `withActionScheduler` HOC that returns a stable `IScheduleActions` API and automatically calls `disable()` on unmount, allowing components to schedule timeouts, intervals, and animation frames without needing to wrap in a HOC - Exports both hooks from the package's public index Note: animation frame tests require `cleanup()` to be called explicitly before `jest.restoreAllMocks()` in `afterEach` — RTL's auto-cleanup runs after the describe-level `afterEach`, so restoring the `cancelAnimationFrame` spy first causes sinon's fake timer to throw on component unmount. Issue: https://khanacademy.atlassian.net/browse/WB-2281 Test plan: - [ ] Run `pnpm jest packages/wonder-blocks-timing --no-coverage --no-watchman`
…er hooks to wonder-blocks-timing
🦋 Changeset detectedLatest commit: 694acbe The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Size Change: +158 B (+0.13%) Total Size: 121 kB 📦 View Changed
ℹ️ View Unchanged
|
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (2d76160) and published all packages with changesets to npm. You can install the packages in ./dev/tools/deploy_wonder_blocks.js --tag="PR2991"Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR2991 |
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-cfnffgvhgi.chromatic.com/ Chromatic results:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review.
Tip: disable this comment in your organization's Code Review settings.
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
|
@claude review |
somewhatabstract
left a comment
somewhatabstract
left a comment
There was a problem hiding this comment.
The implementation looks pretty solid, but the docs seem quite wrong. They are describing behaviors that are not present (nor should be, at least for this initial version). Can you update them to be correct - it's likely you can mostly borrow from existing docs to make something consistent.
tony-dinh
force-pushed
the
WB-2281
branch
2 times, most recently
from
e165bf8 to
ba1d3e2
Compare
There was a problem hiding this comment.
All previously raised issues have been addressed — LGTM.
Extended reasoning...
Overview
This PR adds useAnimationFrame and useActionScheduler hooks to wonder-blocks-timing, completing the hook-based API alongside the existing useTimeout and useInterval. Changes span two new hook implementations, comprehensive tests (~840 lines combined), Storybook docs/stories, type additions, and public exports.
Security risks
No security-sensitive code paths — purely client-side React hooks wrapping browser timer APIs (requestAnimationFrame, setTimeout, setInterval). No auth, crypto, or data exposure concerns.
Level of scrutiny
This is new public API surface for an internal component library. The implementation follows well-established patterns from the existing useTimeout and useInterval hooks. I reviewed this PR in two prior passes and raised eight issues (one red, seven yellow). The author addressed all of them in the subsequent "Address feedback" commits: the React 18 Strict Mode bug (ActionScheduler moved into useEffect), JSDoc inaccuracies, missing timestamp parameter in docs, broken anchor link, stale Sinon references in test comments, missing pre-mount no-op documentation, an unmount race in the animation loop story, and a misleading useTimeout/useInterval comparison. The fixes are correct and match the patterns used in the rest of the timing package.
Other factors
Test coverage is thorough — the useAnimationFrame test file has ~40 cases covering all policy combinations, proxy-ref behavior, and lifecycle edges. The useActionScheduler tests cover delegation, stability, and unmount cleanup. Chromatic shows no visual regressions. The human reviewer’s open questions were resolved in the PR thread.
somewhatabstract
left a comment
somewhatabstract
left a comment
There was a problem hiding this comment.
This looks great. The docs just need some tidy-up...mostly just deleting implementation details from the type docs.
| if (action !== actionProxyRef.current) { | ||
| actionProxyRef.current = action; | ||
| if (actionPolicy === ActionPolicy.Reset) { | ||
| animationFrameRef.current?.set(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟣 This is a pre-existing issue being replicated from the sibling useTimeout and useInterval hooks: animationFrameRef.current?.set() is called directly in the render body when actionPolicy === ActionPolicy.Reset (lines 47-52), which is technically a side effect during the React render phase. The same pattern exists verbatim in use-timeout.ts and use-interval.ts, so fixing it here would require fixing all three hooks consistently.
Extended reasoning...
What the bug is
In use-animation-frame.ts lines 47-52, when actionPolicy === ActionPolicy.Reset and action !== actionProxyRef.current, the hook calls animationFrameRef.current?.set() directly in the component render body — not inside a useEffect. React considers side effects during render incorrect, particularly in Concurrent Mode, where the render function can be called multiple times before committing.
The specific code path that triggers it
The render body runs:
if (action !== actionProxyRef.current) {
actionProxyRef.current = action; // ref mutation
if (actionPolicy === ActionPolicy.Reset) {
animationFrameRef.current?.set(); // side effect: cancel + reschedule rAF
}
}In React Concurrent Mode, if a render is interrupted and restarted, this block can fire from a render that is never committed. The set() call invokes cancelAnimationFrame + requestAnimationFrame — real browser APIs — from the interrupted render.
Addressing the refutation
One verifier correctly notes that the ref guard provides practical idempotency: once actionProxyRef.current is updated to action2 in the first (interrupted) render, the condition action !== actionProxyRef.current evaluates false in the restarted render, so set() is not called a second time. This substantially limits the practical impact. The worst case is a single extra cancel+reschedule pair from the interrupted render, not unbounded repetition.
Additionally, because actionProxyRef.current was already updated, the rAF scheduled from the interrupted render will call the correct (new) action when it fires — so correctness is preserved even in the edge case.
Why this is pre-existing
The identical pattern appears in both use-timeout.ts (same lines range) and use-interval.ts (same lines range):
// use-timeout.ts and use-interval.ts — the same pattern
if (action !== actionProxyRef.current) {
actionProxyRef.current = action;
if (actionPolicy === ActionPolicy.Reset) {
timeoutRef.current?.set(); // or intervalRef.current?.set()
}
}This PR introduces useAnimationFrame following the exact same established pattern. All three verifiers confirmed the pre_existing classification — fixing it in this PR alone without fixing the sibling hooks would be inconsistent.
Practical impact and fix
In practice, the impact is low: the idempotency guard prevents double-scheduling, and the worst outcome is one extra cancel+reschedule from an interrupted render. The correct fix — if the team decides to address it — is to move the animationFrameRef.current?.set() call into a useEffect with [action, actionPolicy] dependencies, and apply the same fix symmetrically to useTimeout and useInterval. The fix in this PR should be part of a broader cleanup of all three hooks together.
3 participants
Summary:
This PR adds two new hooks to
@khanacademy/wonder-blocks-timingthat complete the hook-based API for the timing package.useAnimationFrame: a hook wrappingrequestAnimationFrame/cancelAnimationFramethat follows the same proxy-ref and policy pattern as the existinguseTimeoutanduseIntervalhooks — supportsSchedulePolicy,ClearPolicy, andActionPolicyuseActionScheduler: a hook-based alternative to thewithActionSchedulerHOC that returns a stableIScheduleActionsAPI and automatically callsdisable()on unmount, allowing components to schedule timeouts, intervals, and animation frames without needing to wrap in a HOCIssue: https://khanacademy.atlassian.net/browse/WB-2281
Test plan:
pnpm jest packages/wonder-blocks-timing --no-coverage --no-watchman