Solved the broken progress bar issue#226
Conversation
✅ Deploy Preview for github-spy ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughScrollProgressBar refactors scroll event handling to improve performance by eliminating conditional listener attachment and replacing it with unconditional mount-time registration. A new ChangesScroll Progress Bar Performance and Styling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🎉 Thank you @Shayan-Bhowmik for your contribution. Please make sure your PR follows https://github.com/GitMetricsLab/github_tracker/blob/main/CONTRIBUTING.md#-pull-request-guidelines
There was a problem hiding this comment.
Pull request overview
Fixes the navbar scroll progress indicator so it updates immediately and displays with improved visibility/smoothness.
Changes:
- Register the scroll event listener on mount (instead of waiting for the 2s load animation to finish)
- Initialize progress state by calling
handleScroll()on mount - Update progress bar styling (color, height, transition timing)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const scrollTop = document.documentElement.scrollTop; | ||
| const scrollHeight = | ||
| document.documentElement.scrollHeight - | ||
| document.documentElement.clientHeight; | ||
| const width = (scrollTop / scrollHeight) * 100; |
| const handleScroll = () => { | ||
| const scrollTop = document.documentElement.scrollTop; | ||
| const scrollHeight = | ||
| document.documentElement.scrollHeight - | ||
| document.documentElement.clientHeight; | ||
| const width = (scrollTop / scrollHeight) * 100; | ||
| setScrollWidth(width); | ||
| }; |
| useEffect(() => { | ||
| if (!isAnimating) { | ||
| window.addEventListener("scroll", handleScroll); | ||
| } | ||
| // Always listen for scroll events | ||
| window.addEventListener("scroll", handleScroll); | ||
| // Call handleScroll once on mount to set initial position | ||
| handleScroll(); | ||
| return () => window.removeEventListener("scroll", handleScroll); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/components/ScrollProgressBar.tsx`:
- Around line 7-14: The handleScroll function can produce NaN when
document.documentElement.scrollHeight - document.documentElement.clientHeight is
0 and is recreated each render, breaking cleanup; wrap handleScroll in
useCallback and inside it compute denom = document.documentElement.scrollHeight
- document.documentElement.clientHeight, if denom === 0 setScrollWidth(0) else
setScrollWidth((scrollTop / denom) * 100); then use that same memoized
handleScroll in the effect that adds/removes the 'scroll' listener (include
handleScroll in the effect deps) so addEventListener and removeEventListener
receive the identical function reference.
🪄 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
Run ID: fc05cfaa-23db-4282-96d8-0b4073953ec8
📒 Files selected for processing (1)
src/components/ScrollProgressBar.tsx
| const handleScroll = () => { | ||
| const scrollTop = document.documentElement.scrollTop; | ||
| const scrollHeight = | ||
| document.documentElement.scrollHeight - | ||
| document.documentElement.clientHeight; | ||
| const width = (scrollTop / scrollHeight) * 100; | ||
| setScrollWidth(width); | ||
| }; |
There was a problem hiding this comment.
Guard against division by zero and wrap in useCallback.
Two issues:
-
When page content fits on screen without scrolling,
scrollHeight - clientHeightequals 0, causingwidthto becomeNaNand breaking the progress bar rendering. -
The function is recreated on every render. The cleanup at line 31 attempts to remove a different function reference than was registered at line 28, preventing proper cleanup and causing a memory leak.
🔧 Proposed fix
-import { useEffect, useState } from "react";
+import { useCallback, useEffect, useState } from "react";
...
- const handleScroll = () => {
+ const handleScroll = useCallback(() => {
const scrollTop = document.documentElement.scrollTop;
const scrollHeight =
document.documentElement.scrollHeight -
document.documentElement.clientHeight;
+
+ // Guard against division by zero when content doesn't scroll
+ if (scrollHeight === 0) {
+ setScrollWidth(0);
+ return;
+ }
+
const width = (scrollTop / scrollHeight) * 100;
setScrollWidth(width);
- };
+ }, []);📝 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.
| const handleScroll = () => { | |
| const scrollTop = document.documentElement.scrollTop; | |
| const scrollHeight = | |
| document.documentElement.scrollHeight - | |
| document.documentElement.clientHeight; | |
| const width = (scrollTop / scrollHeight) * 100; | |
| setScrollWidth(width); | |
| }; | |
| const handleScroll = useCallback(() => { | |
| const scrollTop = document.documentElement.scrollTop; | |
| const scrollHeight = | |
| document.documentElement.scrollHeight - | |
| document.documentElement.clientHeight; | |
| // Guard against division by zero when content doesn't scroll | |
| if (scrollHeight === 0) { | |
| setScrollWidth(0); | |
| return; | |
| } | |
| const width = (scrollTop / scrollHeight) * 100; | |
| setScrollWidth(width); | |
| }, []); |
🤖 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 `@src/components/ScrollProgressBar.tsx` around lines 7 - 14, The handleScroll
function can produce NaN when document.documentElement.scrollHeight -
document.documentElement.clientHeight is 0 and is recreated each render,
breaking cleanup; wrap handleScroll in useCallback and inside it compute denom =
document.documentElement.scrollHeight - document.documentElement.clientHeight,
if denom === 0 setScrollWidth(0) else setScrollWidth((scrollTop / denom) * 100);
then use that same memoized handleScroll in the effect that adds/removes the
'scroll' listener (include handleScroll in the effect deps) so addEventListener
and removeEventListener receive the identical function reference.
Related Issue
Description
I've fixed the scroll progress bar functionality on the navbar. The progress bar had several issues that prevented it from working properly:
I've resolved these by:
handleScroll()on mount to capture the initial scroll positionHow Has This Been Tested?
I've manually tested by:
Screenshots (if applicable)
N/A
Type of Change
Summary by CodeRabbit
Bug Fixes
Style