feat(raf): add frameloop utils. - #824
Conversation
- Added `useFrameloop` util to use unified request animation frame calls. - Added `createScheduledFrameloop` to handle request animation frame from external sources. - Fixed `createRAF` cleanup for id `0` by using `null` instead.
|
| } | ||
|
|
||
| /** | ||
| * Returns an advanced primitive factory function (that has an API similar to `createRAF`) to handle multiple animation frame callbacks in a single batched `requestAnimationFrame`, avoiding the overhead of scheduling multiple animation frames outside of a batch and making them all sync on the same delta. |
There was a problem hiding this comment.
The explanation is way too complex. "A version of createRAF that batches multiple frames within the same render cycle instead of skipping them ."
|
Sorry for the delay on this, the end of the year tends to be really busy. I was starting to re-visit the approach itself and maybe landed into a nicer primitive set, want to hear your opinions on this @atk (I can leave it as an update on this PR, but I don't think this is the right approach for this):
|
atk
left a comment
atk
left a comment
There was a problem hiding this comment.
I don't see my last points addressed, also, the tests seem to fail.
|
Hi @royeden any interest in completing out this PR? We're preparing for Solid Primitives 2.0 and would love to include these improvements. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds ChangesRAF Package New Primitives
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Component
participant useGlobalRAF
participant SingletonRoot
participant BrowserRAF
Component->>useGlobalRAF: callback, startWhenAdded
useGlobalRAF->>SingletonRoot: add(callback)
SingletonRoot->>BrowserRAF: requestAnimationFrame(loop)
BrowserRAF-->>SingletonRoot: timestamp
SingletonRoot->>Component: invoke callback(timestamp)
Component->>useGlobalRAF: remove(callback)
SingletonRoot->>BrowserRAF: cancelAnimationFrame
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/raf/test/index.test.ts (1)
274-303: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSame un-awaited
vi.waitUntilissue as above.Line 298's
vi.waitUntil(() => { expect(caf).toHaveBeenCalledTimes(1); })is not awaited and theitcallback (line 280 test, or 274 wrapping) is notasync, so the cancellation-after-dispose assertion never actually gates the test outcome.🐛 Proposed fix
- createRoot(dispose => { + createRoot(async dispose => { ... dispose(); - vi.waitUntil(() => { - expect(caf).toHaveBeenCalledTimes(1); - }); + await vi.waitUntil(() => caf.mock.calls.length >= 1); + expect(caf).toHaveBeenCalledTimes(1); });Note:
createRoot's callback is synchronous by design, so this may require restructuring the test to run outsidecreateRoot(e.g., capturedisposeand await afterwards) for theawaitto work correctly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/raf/test/index.test.ts` around lines 274 - 303, The frameloop dispose test is using vi.waitUntil without awaiting it, so the cancelAnimationFrame assertion never participates in the test result. Update the test around useGlobalRAF/createRoot to make the outer it callback async, capture dispose from createRoot, and await the vi.waitUntil assertion after dispose is called so the caf expectation is properly verified.
🧹 Nitpick comments (2)
packages/raf/src/index.ts (1)
161-172: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
requestIDis never reset tonullafter cancellation.In
stop(), oncecancel(requestID)runs,requestIDkeeps its stale value. Ifstop()is invoked twice (e.g. manualstop()followed byonCleanup(stop)on dispose),cancel()is called again with the same stale id. This is harmless forcancelAnimationFrame(a browser API tolerant of stale/invalid ids), but sincecancelhere is an arbitrary user-supplied function (e.g. Motion'scancelFrame), redundant calls with a stale/reused reference may not be safe.🛡️ Proposed fix
const stop = () => { setRunning(false); - if (requestID !== null) cancel(requestID); + if (requestID !== null) { + cancel(requestID); + requestID = null; + } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/raf/src/index.ts` around lines 161 - 172, In the RAF controller’s start/stop logic, the local requestID is left stale after canceling, so a later stop() can cancel the same id again. Update the stop() path in the create RAF helper to clear requestID back to null immediately after calling cancel(requestID), and keep the existing guard in start() so a new schedule(callback) only happens when not already running.packages/raf/test/index.test.ts (1)
42-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAddress the acknowledged test gap.
// TODO add better test — wiring
createScheduledLoopdirectly to rawwindow.requestAnimationFrameonly verifies a single scheduling round-trip, not repeated-loop behavior, which is the primitive's stated purpose. Want me to draft a test using a self-reschedulingschedulefunction (similar tocreateRAF's internalloop) to actually validate repeated invocation and cancellation?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/raf/test/index.test.ts` around lines 42 - 63, The current createScheduledLoop test only covers one requestAnimationFrame round-trip, so it does not validate the repeated-loop behavior the primitive is meant to provide. Update the test in createScheduledLoop to use a self-rescheduling scheduler (similar to the loop used by createRAF) and assert that the callback is invoked repeatedly after start() until stop() cancels it. Keep the existing createScheduledLoop API usage and verify both running() state changes and cancelAnimationFrame behavior through the named start, stop, and running helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/raf/src/index.ts`:
- Around line 150-177: The Motion adapter in createScheduledLoop is using the
wrong cancellation token, so stop() cannot cancel work scheduled by
frame.render(). Update the schedule/cancel contract to return and store a stable
handle such as the original callback reference from the Motion adapter, then
pass that same handle into cancel from start/stop. Make sure the fix is applied
in createScheduledLoop and the Motion-specific scheduler implementation so the
value returned by schedule is the one later consumed by cancel.
In `@packages/raf/test/index.test.ts`:
- Around line 64-78: The test in createScheduledLoop is using the wrong
primitive and is currently duplicating the createRAF case. Update the test body
to instantiate and exercise createScheduledLoop instead of createRAF, while
keeping the same dispose/cancel assertions so the scheduled loop’s cleanup
behavior is actually covered. Use the createScheduledLoop and createRoot symbols
to locate and correct the copy-paste error.
- Around line 193-253: The automatic-start RAF singleton test is not actually
asserting the async disposal state because vi.waitUntil in the
useGlobalRAF/createRoot block is not awaited. Make the it callback async and
await the vi.waitUntil call so the running() and cancelAnimationFrame
expectations are part of the test result, then keep dispose() after the awaited
check.
---
Duplicate comments:
In `@packages/raf/test/index.test.ts`:
- Around line 274-303: The frameloop dispose test is using vi.waitUntil without
awaiting it, so the cancelAnimationFrame assertion never participates in the
test result. Update the test around useGlobalRAF/createRoot to make the outer it
callback async, capture dispose from createRoot, and await the vi.waitUntil
assertion after dispose is called so the caf expectation is properly verified.
---
Nitpick comments:
In `@packages/raf/src/index.ts`:
- Around line 161-172: In the RAF controller’s start/stop logic, the local
requestID is left stale after canceling, so a later stop() can cancel the same
id again. Update the stop() path in the create RAF helper to clear requestID
back to null immediately after calling cancel(requestID), and keep the existing
guard in start() so a new schedule(callback) only happens when not already
running.
In `@packages/raf/test/index.test.ts`:
- Around line 42-63: The current createScheduledLoop test only covers one
requestAnimationFrame round-trip, so it does not validate the repeated-loop
behavior the primitive is meant to provide. Update the test in
createScheduledLoop to use a self-rescheduling scheduler (similar to the loop
used by createRAF) and assert that the callback is invoked repeatedly after
start() until stop() cancels it. Keep the existing createScheduledLoop API usage
and verify both running() state changes and cancelAnimationFrame behavior
through the named start, stop, and running helpers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 922a2a45-0592-4713-9037-e7ecdf9780f0
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
packages/raf/README.mdpackages/raf/package.jsonpackages/raf/src/index.tspackages/raf/test/index.test.tspackages/raf/tsconfig.json
| it("(Automatic start) global RAF singleton calls rafs with the same timestamp", () => { | ||
| // Note on this test: For some reason, the raf is being called twice, once when started (but id doesn't invoke the callback for some strange reason) and once after the timers advance (and the callback is properly invoked). | ||
| const raf = vi.spyOn(window, "requestAnimationFrame"); | ||
| const caf = vi.spyOn(window, "cancelAnimationFrame"); | ||
| createRoot(dispose => { | ||
| const timestamps = new Set<number>(); | ||
| const createGlobalRAFCallback = useGlobalRAF(); | ||
| const callback1: Mock<FrameRequestCallback> = vi.fn(ts => timestamps.add(ts)); | ||
| const [added1, add1, remove1, running1, start1, stop1] = createGlobalRAFCallback( | ||
| callback1, | ||
| true, | ||
| ); | ||
| const callback2: Mock<FrameRequestCallback> = vi.fn(ts => timestamps.add(ts)); | ||
| const [added2, add2, remove2, running2, start2, stop2] = createGlobalRAFCallback( | ||
| callback2, | ||
| true, | ||
| ); | ||
|
|
||
| // Queue functions should not be equal | ||
| expect(added1).not.toEqual(added2); | ||
| expect(add1).not.toEqual(add2); | ||
| expect(remove1).not.toEqual(remove2); | ||
| expect(callback1).not.toHaveBeenCalled(); | ||
| expect(callback2).not.toHaveBeenCalled(); | ||
|
|
||
| // Frameloop functions should be equal because of the singleton | ||
| expect(running1).toEqual(running2); | ||
| expect(start1).toEqual(start2); | ||
| expect(stop1).toEqual(stop2); | ||
|
|
||
| // Aliases | ||
| const running = running1; | ||
| const stop = stop1; | ||
|
|
||
| expect(added1()).toBe(false); | ||
| add1(); | ||
| expect(added1()).toBe(true); | ||
| expect(added2()).toBe(false); | ||
| expect(running()).toBe(true); | ||
| vi.advanceTimersToNextFrame(); | ||
| expect(raf).toHaveBeenCalledTimes(2); | ||
| expect(callback1).toHaveBeenCalledTimes(1); | ||
| expect(timestamps.size).toEqual(1); | ||
| stop(); | ||
| expect(running()).toBe(false); | ||
| expect(caf).toHaveBeenCalledTimes(1); | ||
| add2(); | ||
| expect(added2()).toBe(true); | ||
| expect(running()).toBe(true); | ||
| vi.advanceTimersToNextFrame(); | ||
| expect(raf).toHaveBeenCalledTimes(4); | ||
| expect(timestamps.size).toEqual(2); | ||
| remove1(); | ||
| remove2(); | ||
| vi.waitUntil(() => { | ||
| expect(running()).toBe(false); | ||
| expect(caf).toHaveBeenCalledTimes(2); | ||
| }); | ||
| dispose(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
vi.waitUntil assertions are never verified — not awaited.
vi.waitUntil returns a Promise and this call is not awaited nor is the enclosing it callback async. The assertions inside its callback (expect(running()).toBe(false), expect(caf).toHaveBeenCalledTimes(2)) will never actually gate the test result — dispose() runs immediately after, and any failures inside the un-awaited promise surface at best as an unhandled rejection, not a test failure. This test currently provides false confidence about disposal/cancellation behavior in the automatic-start scenario.
🐛 Proposed fix
- it("(Automatic start) global RAF singleton calls rafs with the same timestamp", () => {
+ it("(Automatic start) global RAF singleton calls rafs with the same timestamp", async () => {
...
remove1();
remove2();
- vi.waitUntil(() => {
- expect(running()).toBe(false);
- expect(caf).toHaveBeenCalledTimes(2);
- });
+ await vi.waitUntil(() => !running());
+ expect(caf).toHaveBeenCalledTimes(2);
dispose();
- });
+ });
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("(Automatic start) global RAF singleton calls rafs with the same timestamp", () => { | |
| // Note on this test: For some reason, the raf is being called twice, once when started (but id doesn't invoke the callback for some strange reason) and once after the timers advance (and the callback is properly invoked). | |
| const raf = vi.spyOn(window, "requestAnimationFrame"); | |
| const caf = vi.spyOn(window, "cancelAnimationFrame"); | |
| createRoot(dispose => { | |
| const timestamps = new Set<number>(); | |
| const createGlobalRAFCallback = useGlobalRAF(); | |
| const callback1: Mock<FrameRequestCallback> = vi.fn(ts => timestamps.add(ts)); | |
| const [added1, add1, remove1, running1, start1, stop1] = createGlobalRAFCallback( | |
| callback1, | |
| true, | |
| ); | |
| const callback2: Mock<FrameRequestCallback> = vi.fn(ts => timestamps.add(ts)); | |
| const [added2, add2, remove2, running2, start2, stop2] = createGlobalRAFCallback( | |
| callback2, | |
| true, | |
| ); | |
| // Queue functions should not be equal | |
| expect(added1).not.toEqual(added2); | |
| expect(add1).not.toEqual(add2); | |
| expect(remove1).not.toEqual(remove2); | |
| expect(callback1).not.toHaveBeenCalled(); | |
| expect(callback2).not.toHaveBeenCalled(); | |
| // Frameloop functions should be equal because of the singleton | |
| expect(running1).toEqual(running2); | |
| expect(start1).toEqual(start2); | |
| expect(stop1).toEqual(stop2); | |
| // Aliases | |
| const running = running1; | |
| const stop = stop1; | |
| expect(added1()).toBe(false); | |
| add1(); | |
| expect(added1()).toBe(true); | |
| expect(added2()).toBe(false); | |
| expect(running()).toBe(true); | |
| vi.advanceTimersToNextFrame(); | |
| expect(raf).toHaveBeenCalledTimes(2); | |
| expect(callback1).toHaveBeenCalledTimes(1); | |
| expect(timestamps.size).toEqual(1); | |
| stop(); | |
| expect(running()).toBe(false); | |
| expect(caf).toHaveBeenCalledTimes(1); | |
| add2(); | |
| expect(added2()).toBe(true); | |
| expect(running()).toBe(true); | |
| vi.advanceTimersToNextFrame(); | |
| expect(raf).toHaveBeenCalledTimes(4); | |
| expect(timestamps.size).toEqual(2); | |
| remove1(); | |
| remove2(); | |
| vi.waitUntil(() => { | |
| expect(running()).toBe(false); | |
| expect(caf).toHaveBeenCalledTimes(2); | |
| }); | |
| dispose(); | |
| }); | |
| }); | |
| it("(Automatic start) global RAF singleton calls rafs with the same timestamp", async () => { | |
| // Note on this test: For some reason, the raf is being called twice, once when started (but id doesn't invoke the callback for some strange reason) and once after the timers advance (and the callback is properly invoked). | |
| const raf = vi.spyOn(window, "requestAnimationFrame"); | |
| const caf = vi.spyOn(window, "cancelAnimationFrame"); | |
| createRoot(dispose => { | |
| const timestamps = new Set<number>(); | |
| const createGlobalRAFCallback = useGlobalRAF(); | |
| const callback1: Mock<FrameRequestCallback> = vi.fn(ts => timestamps.add(ts)); | |
| const [added1, add1, remove1, running1, start1, stop1] = createGlobalRAFCallback( | |
| callback1, | |
| true, | |
| ); | |
| const callback2: Mock<FrameRequestCallback> = vi.fn(ts => timestamps.add(ts)); | |
| const [added2, add2, remove2, running2, start2, stop2] = createGlobalRAFCallback( | |
| callback2, | |
| true, | |
| ); | |
| // Queue functions should not be equal | |
| expect(added1).not.toEqual(added2); | |
| expect(add1).not.toEqual(add2); | |
| expect(remove1).not.toEqual(remove2); | |
| expect(callback1).not.toHaveBeenCalled(); | |
| expect(callback2).not.toHaveBeenCalled(); | |
| // Frameloop functions should be equal because of the singleton | |
| expect(running1).toEqual(running2); | |
| expect(start1).toEqual(start2); | |
| expect(stop1).toEqual(stop2); | |
| // Aliases | |
| const running = running1; | |
| const stop = stop1; | |
| expect(added1()).toBe(false); | |
| add1(); | |
| expect(added1()).toBe(true); | |
| expect(added2()).toBe(false); | |
| expect(running()).toBe(true); | |
| vi.advanceTimersToNextFrame(); | |
| expect(raf).toHaveBeenCalledTimes(2); | |
| expect(callback1).toHaveBeenCalledTimes(1); | |
| expect(timestamps.size).toEqual(1); | |
| stop(); | |
| expect(running()).toBe(false); | |
| expect(caf).toHaveBeenCalledTimes(1); | |
| add2(); | |
| expect(added2()).toBe(true); | |
| expect(running()).toBe(true); | |
| vi.advanceTimersToNextFrame(); | |
| expect(raf).toHaveBeenCalledTimes(4); | |
| expect(timestamps.size).toEqual(2); | |
| remove1(); | |
| remove2(); | |
| await vi.waitUntil(() => !running()); | |
| expect(caf).toHaveBeenCalledTimes(2); | |
| dispose(); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/raf/test/index.test.ts` around lines 193 - 253, The automatic-start
RAF singleton test is not actually asserting the async disposal state because
vi.waitUntil in the useGlobalRAF/createRoot block is not awaited. Make the it
callback async and await the vi.waitUntil call so the running() and
cancelAnimationFrame expectations are part of the test result, then keep
dispose() after the awaited check.
I'll make the current suggested changes and I'd have no problem on porting this to V2, my only issue is knowing when the dependencies I consume on the project are ported. I'll send a follow up message on discord so I can communicate better on this 😄 |
|
@davedbase comments should have been addressed 😄 |
3 participants
Added
createCallbacksSetutil to unify calling multiple void callbacks stored in aReactiveSet.Added
createScheduledLoopto handle external animation loops, such asmotion's frame util.Added
useGlobalRAFutil to handle unified request animation frame calls.Fixed
createRAFcleanup for id0by usingnullinstead.Addresses this proposal #822.
Summary by CodeRabbit