⚡ Bolt: optimize scroll event listeners for smoother scrolling#106
⚡ Bolt: optimize scroll event listeners for smoother scrolling#106
Conversation
- Add `{ passive: true }` to global `scroll` event listeners to prevent scroll blocking.
- Throttle state updates inside scroll handlers using `requestAnimationFrame` to run at most once per frame.
- Tested and verified no visual regression on scrolling.
Co-authored-by: anyulled <100741+anyulled@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughScroll-event handling is optimized across multiple layout components by implementing throttling via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Review Summary by QodoOptimize scroll event listeners for smoother scrolling
WalkthroughsDescription• Optimize scroll event listeners with { passive: true } flag
• Throttle scroll state updates using requestAnimationFrame
• Prevent scroll blocking and reduce main thread jank
• Update tests to mock requestAnimationFrame behavior
Diagramflowchart LR
A["Scroll Event Fired"] -->|"passive: true"| B["Non-blocking Listener"]
B -->|"requestAnimationFrame"| C["Throttle to Frame Rate"]
C -->|"State Update"| D["Smooth Scrolling"]
File Changes1. components/elements/BackToTop.tsx
|
Code Review by Qodo
1. rAF not cancelled
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience by optimizing scroll event handling across key components. By introducing passive event listeners and Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively optimizes scroll event listeners by using requestAnimationFrame throttling and the passive: true option, which is a great improvement for scrolling performance. I've identified a recurring issue across the new useEffect hooks: they don't clean up pending requestAnimationFrame calls on unmount. This could lead to React warnings and memory leaks. I've provided suggestions to cancel the animation frame in the cleanup function for BackToTop.tsx, DynamicHeaderWrapper.tsx, and Layout.tsx. Additionally, I found a new test case in __tests__/snapshots/elements/BackToTop.test.tsx that lacks assertions, making it ineffective. I've suggested adding assertions to properly verify the component's behavior.
| it("updates scroll state on scroll event", () => { | ||
| render(<BackToTop target="#top" />); | ||
|
|
||
| Object.defineProperty(window, "scrollY", { value: 150, writable: true, configurable: true }); | ||
| window.requestAnimationFrame = (cb) => { | ||
| cb(performance.now()); | ||
| return 1; | ||
| }; | ||
| fireEvent.scroll(window); | ||
| }); |
There was a problem hiding this comment.
This test case is missing assertions. A test without assertions doesn't verify any behavior and will pass even if the component is broken. You should add assertions to check that the component's state is updated correctly after the scroll event and that the 'Back to Top' button is displayed.
it("updates scroll state on scroll event", () => {
const { container } = render(<BackToTop target="#top" />);
expect(container.querySelector(".paginacontainer")).toBeNull();
Object.defineProperty(window, "scrollY", { value: 150, writable: true, configurable: true });
window.requestAnimationFrame = (cb) => {
cb(performance.now());
return 1;
};
fireEvent.scroll(window);
expect(container.querySelector(".paginacontainer")).not.toBeNull();
});
| useEffect(() => { | ||
| const state = { isTicking: false }; | ||
| const onScroll = () => { | ||
| setHasScrolled(window.scrollY > 100); | ||
| if (!state.isTicking) { | ||
| window.requestAnimationFrame(() => { | ||
| setHasScrolled(window.scrollY > 100); | ||
| state.isTicking = false; | ||
| }); | ||
| state.isTicking = true; | ||
| } | ||
| }; | ||
|
|
||
| window.addEventListener("scroll", onScroll); | ||
| window.addEventListener("scroll", onScroll, { passive: true }); | ||
| return () => window.removeEventListener("scroll", onScroll); | ||
| }, []); |
There was a problem hiding this comment.
The current implementation of requestAnimationFrame could lead to a state update on an unmounted component if the component unmounts after the animation frame is requested but before it executes. This can cause memory leaks and warnings in development mode.
To prevent this, you should cancel the animation frame in the useEffect cleanup function.
useEffect(() => {
const state = { isTicking: false };
let animationFrameId: number;
const onScroll = () => {
if (!state.isTicking) {
animationFrameId = window.requestAnimationFrame(() => {
setHasScrolled(window.scrollY > 100);
state.isTicking = false;
});
state.isTicking = true;
}
};
window.addEventListener("scroll", onScroll, { passive: true });
return () => {
window.removeEventListener("scroll", onScroll);
window.cancelAnimationFrame(animationFrameId);
};
}, []);
| React.useEffect(() => { | ||
| const state = { isTicking: false }; | ||
| const handleScroll = (): void => { | ||
| const scrollCheck: boolean = window.scrollY > 100; | ||
| if (scrollCheck !== scroll) { | ||
| setScroll(scrollCheck); | ||
| if (!state.isTicking) { | ||
| window.requestAnimationFrame(() => { | ||
| const scrollCheck: boolean = window.scrollY > 100; | ||
| setScroll((prev) => (prev !== scrollCheck ? scrollCheck : prev)); | ||
| state.isTicking = false; | ||
| }); | ||
| state.isTicking = true; | ||
| } | ||
| }; | ||
| document.addEventListener("scroll", handleScroll); | ||
| document.addEventListener("scroll", handleScroll, { passive: true }); | ||
| return () => { | ||
| document.removeEventListener("scroll", handleScroll); | ||
| }; | ||
| }, [scroll]); | ||
| }, []); |
There was a problem hiding this comment.
The current implementation of requestAnimationFrame could lead to a state update on an unmounted component if the component unmounts after the animation frame is requested but before it executes. This can cause memory leaks and warnings in development mode.
To prevent this, you should cancel the animation frame in the useEffect cleanup function.
React.useEffect(() => {
const state = { isTicking: false };
let animationFrameId: number;
const handleScroll = (): void => {
if (!state.isTicking) {
animationFrameId = window.requestAnimationFrame(() => {
const scrollCheck: boolean = window.scrollY > 100;
setScroll((prev) => (prev !== scrollCheck ? scrollCheck : prev));
state.isTicking = false;
});
state.isTicking = true;
}
};
document.addEventListener("scroll", handleScroll, { passive: true });
return () => {
document.removeEventListener("scroll", handleScroll);
window.cancelAnimationFrame(animationFrameId);
};
}, []);
| useEffect(() => { | ||
| AOS.init(); | ||
|
|
||
| const state = { isTicking: false }; | ||
| const handleScroll = (): void => { | ||
| const scrollCheck: boolean = window.scrollY > 100; | ||
| if (scrollCheck !== scroll) { | ||
| setScroll(scrollCheck); | ||
| if (!state.isTicking) { | ||
| window.requestAnimationFrame(() => { | ||
| const scrollCheck: boolean = window.scrollY > 100; | ||
| setScroll((prev) => (prev !== scrollCheck ? scrollCheck : prev)); | ||
| state.isTicking = false; | ||
| }); | ||
| state.isTicking = true; | ||
| } | ||
| }; | ||
|
|
||
| document.addEventListener("scroll", handleScroll); | ||
| document.addEventListener("scroll", handleScroll, { passive: true }); | ||
|
|
||
| return () => { | ||
| document.removeEventListener("scroll", handleScroll); | ||
| }; | ||
| }, [scroll]); | ||
| }, []); |
There was a problem hiding this comment.
The current implementation of requestAnimationFrame could lead to a state update on an unmounted component if the component unmounts after the animation frame is requested but before it executes. This can cause memory leaks and warnings in development mode.
To prevent this, you should cancel the animation frame in the useEffect cleanup function.
useEffect(() => {
AOS.init();
const state = { isTicking: false };
let animationFrameId: number;
const handleScroll = (): void => {
if (!state.isTicking) {
animationFrameId = window.requestAnimationFrame(() => {
const scrollCheck: boolean = window.scrollY > 100;
setScroll((prev) => (prev !== scrollCheck ? scrollCheck : prev));
state.isTicking = false;
});
state.isTicking = true;
}
};
document.addEventListener("scroll", handleScroll, { passive: true });
return () => {
document.removeEventListener("scroll", handleScroll);
window.cancelAnimationFrame(animationFrameId);
};
}, []);
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| const state = { isTicking: false }; | ||
| const handleScroll = (): void => { | ||
| const scrollCheck: boolean = window.scrollY > 100; | ||
| if (scrollCheck !== scroll) { | ||
| setScroll(scrollCheck); | ||
| if (!state.isTicking) { | ||
| window.requestAnimationFrame(() => { | ||
| const scrollCheck: boolean = window.scrollY > 100; | ||
| setScroll((prev) => (prev !== scrollCheck ? scrollCheck : prev)); | ||
| state.isTicking = false; | ||
| }); | ||
| state.isTicking = true; | ||
| } |
There was a problem hiding this comment.
1. Raf not cancelled 🐞 Bug ⛯ Reliability
Layout/DynamicHeaderWrapper/BackToTop schedule setState inside requestAnimationFrame but never cancel a pending frame on unmount, so the callback can still run after cleanup and call setState on an unmounted component. This can trigger React warnings and leak work during route changes and effect teardown/replay.
Agent Prompt
## Issue description
Scroll handlers in `Layout`, `DynamicHeaderWrapper`, and `BackToTop` throttle via `requestAnimationFrame`, but the effect cleanup does not cancel pending frames. If unmount happens after a scroll event scheduled a frame (route change, StrictMode effect teardown/replay), the queued callback can still run and call `setState` on an unmounted component.
## Issue Context
Each component uses an `isTicking` flag to limit rAF scheduling, but does not store the returned rAF id, so there’s nothing to cancel during cleanup.
## Fix Focus Areas
- components/layout/Layout.tsx[80-100]
- components/layout/DynamicHeaderWrapper.tsx[18-34]
- components/elements/BackToTop.tsx[7-21]
## Suggested fix
- Store the rAF id (e.g., `let rafId: number | null = null` or `useRef<number | null>(null)`).
- In the rAF callback, optionally guard with a `mounted` flag (`let mounted = true;` set to `false` in cleanup) before calling `setState`.
- In the effect cleanup: `if (rafId !== null) cancelAnimationFrame(rafId);` and reset ticking/mounted state as needed.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
- Add `{ passive: true }` to global `scroll` event listeners to prevent scroll blocking.
- Throttle state updates inside scroll handlers using `requestAnimationFrame` to run at most once per frame.
- Tested and verified no visual regression on scrolling.
- Fixed unused variable `screen` in `__tests__/snapshots/elements/BackToTop.test.tsx` causing CI lint error.
Co-authored-by: anyulled <100741+anyulled@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
components/layout/Layout.tsx (1)
83-93: Consider extracting a shareduseScrolledhook to reduce duplication.The RAF-throttled scroll pattern is now duplicated across three files (
Layout.tsx,DynamicHeaderWrapper.tsx,BackToTop.tsx). A custom hook would centralize this logic and ensure consistency.♻️ Example shared hook
// hooks/useScrolled.ts import { useEffect, useState } from "react"; export function useScrolled(threshold = 100): boolean { const [scrolled, setScrolled] = useState(false); useEffect(() => { const state = { isTicking: false }; const handleScroll = () => { if (!state.isTicking) { window.requestAnimationFrame(() => { setScrolled((prev) => { const check = window.scrollY > threshold; return prev !== check ? check : prev; }); state.isTicking = false; }); state.isTicking = true; } }; window.addEventListener("scroll", handleScroll, { passive: true }); return () => window.removeEventListener("scroll", handleScroll); }, [threshold]); return scrolled; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/layout/Layout.tsx` around lines 83 - 93, Extract the RAF-throttled scroll logic from Layout.tsx into a reusable hook (e.g., export function useScrolled(threshold = 100)) and replace the inline state/handleScroll in Layout.tsx (the state object, handleScroll function, and setScroll usage) with this hook; the hook should encapsulate the requestAnimationFrame pattern, add/remove the "scroll" listener with passive: true, accept a threshold parameter, and return a boolean scrolled value so Layout.tsx (and DynamicHeaderWrapper.tsx and BackToTop.tsx) can simply call useScrolled() and use the returned boolean instead of duplicating the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/components/layout/DynamicHeaderWrapper.test.tsx`:
- Around line 55-57: Update the requestAnimationFrame stub in
DynamicHeaderWrapper.test.tsx so its signature matches real RAF: accept the
callback (cb) and call it with a numeric timestamp, then return a numeric
request ID; specifically replace the current stub window.requestAnimationFrame =
(cb) => { cb(); } with one that invokes cb(performance.now() or Date.now()) and
returns a non-zero integer, mirroring the pattern used in BackToTop.test.tsx and
Layout.test.tsx to ensure the FrameRequestCallback and return value are correct.
In `@__tests__/components/layout/Layout.test.tsx`:
- Around line 99-101: The requestAnimationFrame stub in the test currently calls
cb() without the DOMHighResTimeStamp and doesn't return an ID; update the stub
for window.requestAnimationFrame to match the real signature by accepting a
FrameRequestCallback, invoking it with performance.now() (or Date.now()) and
returning a numeric request id (e.g., 1) so it matches the stub in
BackToTop.test.tsx and prevents signature inconsistencies when tests run.
In `@__tests__/snapshots/elements/BackToTop.test.tsx`:
- Line 2: The import statement in BackToTop.test.tsx currently imports render,
fireEvent, and screen but screen is unused; update the import at the top (the
line that imports render, fireEvent, screen from "@testing-library/react") to
remove screen so only render and fireEvent are imported, which will resolve the
unused-import lint error.
- Around line 11-20: The test in BackToTop.test.tsx exercises a scroll but has
no assertions; update the it("updates scroll state on scroll event") test to
assert the component's response (e.g., that the internal hasScrolled state
becomes true and the back-to-top control is rendered/visible). After setting
window.scrollY and dispatching the scroll (or using window.requestAnimationFrame
stub), wrap the event in act() or use waitFor() to let effects run, then query
the BackToTop UI (by role, text, or test id used in the BackToTop component) and
assert it exists/is visible; also restore any window stubs
(scrollY/requestAnimationFrame) after the test. Ensure you reference the
BackToTop component and its hasScrolled-driven output when adding the
assertions.
---
Nitpick comments:
In `@components/layout/Layout.tsx`:
- Around line 83-93: Extract the RAF-throttled scroll logic from Layout.tsx into
a reusable hook (e.g., export function useScrolled(threshold = 100)) and replace
the inline state/handleScroll in Layout.tsx (the state object, handleScroll
function, and setScroll usage) with this hook; the hook should encapsulate the
requestAnimationFrame pattern, add/remove the "scroll" listener with passive:
true, accept a threshold parameter, and return a boolean scrolled value so
Layout.tsx (and DynamicHeaderWrapper.tsx and BackToTop.tsx) can simply call
useScrolled() and use the returned boolean instead of duplicating the logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fdfc1716-1bc1-4295-a54a-0836da25eb5e
📒 Files selected for processing (6)
__tests__/components/layout/DynamicHeaderWrapper.test.tsx__tests__/components/layout/Layout.test.tsx__tests__/snapshots/elements/BackToTop.test.tsxcomponents/elements/BackToTop.tsxcomponents/layout/DynamicHeaderWrapper.tsxcomponents/layout/Layout.tsx
| it("updates scroll state on scroll event", () => { | ||
| render(<BackToTop target="#top" />); | ||
|
|
||
| Object.defineProperty(window, "scrollY", { value: 150, writable: true, configurable: true }); | ||
| window.requestAnimationFrame = (cb) => { | ||
| cb(performance.now()); | ||
| return 1; | ||
| }; | ||
| fireEvent.scroll(window); | ||
| }); |
There was a problem hiding this comment.
Test lacks assertions — it exercises code but verifies nothing.
The test fires a scroll event but never asserts that hasScrolled updated or that the "back to top" element is rendered. Without assertions, this test provides no regression protection.
🧪 Proposed fix — add meaningful assertions
it("updates scroll state on scroll event", () => {
- render(<BackToTop target="#top" />);
+ const { container } = render(<BackToTop target="#top" />);
+
+ // Initially not scrolled - element should not be visible
+ expect(container.querySelector(".paginacontainer")).toBeNull();
Object.defineProperty(window, "scrollY", { value: 150, writable: true, configurable: true });
window.requestAnimationFrame = (cb) => {
cb(performance.now());
return 1;
};
fireEvent.scroll(window);
+
+ // After scrolling past 100px, the back-to-top element should appear
+ expect(container.querySelector(".paginacontainer")).toBeInTheDocument();
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@__tests__/snapshots/elements/BackToTop.test.tsx` around lines 11 - 20, The
test in BackToTop.test.tsx exercises a scroll but has no assertions; update the
it("updates scroll state on scroll event") test to assert the component's
response (e.g., that the internal hasScrolled state becomes true and the
back-to-top control is rendered/visible). After setting window.scrollY and
dispatching the scroll (or using window.requestAnimationFrame stub), wrap the
event in act() or use waitFor() to let effects run, then query the BackToTop UI
(by role, text, or test id used in the BackToTop component) and assert it
exists/is visible; also restore any window stubs (scrollY/requestAnimationFrame)
after the test. Ensure you reference the BackToTop component and its
hasScrolled-driven output when adding the assertions.
- Add `{ passive: true }` to global `scroll` event listeners to prevent scroll blocking.
- Throttle state updates inside scroll handlers using `requestAnimationFrame` to run at most once per frame.
- Tested and verified no visual regression on scrolling.
- Fixed unused variable `screen` in `__tests__/snapshots/elements/BackToTop.test.tsx` causing CI lint error.
Co-authored-by: anyulled <100741+anyulled@users.noreply.github.com>
1 participant
💡 What: Added
{ passive: true }andrequestAnimationFramethrottling toscrollevent listeners inLayout.tsx,DynamicHeaderWrapper.tsx, andBackToTop.tsx.🎯 Why: Unthrottled scroll handlers that aren't marked
passiverun synchronously on the main thread for every minute scroll tick, causing high CPU usage, unnecessary consecutive renders, and browser scroll jank (because the browser waits for the JS execution to check forpreventDefault).📊 Impact: Reduces main thread blocking during scrolling and eliminates excess consecutive state updates by batching updates to the display refresh rate (e.g., 60fps). Greatly improves scrolling frame rate and smoothness.
🔬 Measurement: Verify by scrolling heavily on the page; observe smoother scrolling and fewer
setScrollre-renders in DevTools Performance Profiler.PR created automatically by Jules for task 18273862933436870828 started by @anyulled
Summary by CodeRabbit
Tests
Performance