feat: Add scroll-to-top button component (#340)#815
feat: Add scroll-to-top button component (#340)#815HNR138 wants to merge 3 commits intoSolFoundry:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds a new client-only React component Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/common/ScrollToTop.tsx`:
- Around line 18-19: The scroll listener in the ScrollToTop component attaches
toggleVisibility without passive options; update the event registration to use a
passive option to improve scroll responsiveness by passing an options object
(e.g., { passive: true }) when calling window.addEventListener('scroll',
toggleVisibility, options) and ensure you remove the listener with the same
options object in the cleanup (window.removeEventListener('scroll',
toggleVisibility, options)); locate this change in the ScrollToTop component
where toggleVisibility is referenced.
- Around line 9-20: The ScrollToTop component's useEffect currently only sets
isVisible in the scroll event handler, so the initial visibility can be wrong
when the page mounts at an offset >300px; modify the effect that defines
toggleVisibility (the useEffect containing toggleVisibility,
window.addEventListener, and cleanup) to call toggleVisibility once immediately
on mount (before adding the event listener) so setIsVisible is initialized
correctly; keep the existing window.addEventListener('scroll', toggleVisibility)
and the cleanup removing the listener.
- Around line 36-37: Remove the redundant tabIndex={0} prop from the native
<button> in the ScrollToTop component
(frontend/src/components/common/ScrollToTop.tsx) — locate the JSX button element
that currently includes tabIndex={0} and delete that attribute so the element
relies on default keyboard-focusable semantics; confirm no tests or custom logic
depend on an explicit tabIndex and run basic accessibility checks to ensure
focus order remains correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: aa9fdd30-f0d2-4d9d-b630-d17a516a7eec
📒 Files selected for processing (1)
frontend/src/components/common/ScrollToTop.tsx
| window.addEventListener('scroll', toggleVisibility) | ||
| return () => window.removeEventListener('scroll', toggleVisibility) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use a passive scroll listener for better scroll responsiveness.
On Line 18-Line 19, the scroll handler is attached without passive listener options. For scroll-heavy pages, this can degrade perceived scroll performance on the main thread.
As per coding guidelines, frontend/**: React/TypeScript frontend. Check: Component structure and state management.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/common/ScrollToTop.tsx` around lines 18 - 19, The
scroll listener in the ScrollToTop component attaches toggleVisibility without
passive options; update the event registration to use a passive option to
improve scroll responsiveness by passing an options object (e.g., { passive:
true }) when calling window.addEventListener('scroll', toggleVisibility,
options) and ensure you remove the listener with the same options object in the
cleanup (window.removeEventListener('scroll', toggleVisibility, options));
locate this change in the ScrollToTop component where toggleVisibility is
referenced.
… add initial check, remove redundant tabIndex
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/common/ScrollToTop.tsx`:
- Around line 9-15: The toggleVisibility function in ScrollToTop currently uses
the deprecated window.pageYOffset; update it to use window.scrollY instead
(inside the useCallback where setIsVisible is called) so the check becomes
window.scrollY > 300, leaving the rest of the logic and dependencies unchanged;
ensure the reference appears in the toggleVisibility function and any related
scroll handler cleanup remains correct.
- Around line 21-22: The scroll listener is added with an options object but
removed without it, which can prevent removal in some environments; update the
ScrollToTop component to use a single options constant (e.g., const
scrollOptions = { passive: true }) and pass that same scrollOptions to both
window.addEventListener('scroll', toggleVisibility, scrollOptions) and
window.removeEventListener('scroll', toggleVisibility, scrollOptions) so the
listener matches correctly when cleaned up.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e2978c3a-a451-433f-9f23-22897d4e0478
📒 Files selected for processing (1)
frontend/src/components/common/ScrollToTop.tsx
| const toggleVisibility = useCallback(() => { | ||
| if (window.pageYOffset > 300) { | ||
| setIsVisible(true) | ||
| } else { | ||
| setIsVisible(false) | ||
| } | ||
| }, []) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider replacing deprecated pageYOffset with scrollY.
window.pageYOffset is a legacy alias that has been deprecated in favor of window.scrollY. Both work identically, but using the modern API is preferred for forward compatibility.
♻️ Suggested refactor
const toggleVisibility = useCallback(() => {
- if (window.pageYOffset > 300) {
+ if (window.scrollY > 300) {
setIsVisible(true)
} else {
setIsVisible(false)
}
}, [])📝 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 toggleVisibility = useCallback(() => { | |
| if (window.pageYOffset > 300) { | |
| setIsVisible(true) | |
| } else { | |
| setIsVisible(false) | |
| } | |
| }, []) | |
| const toggleVisibility = useCallback(() => { | |
| if (window.scrollY > 300) { | |
| setIsVisible(true) | |
| } else { | |
| setIsVisible(false) | |
| } | |
| }, []) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/common/ScrollToTop.tsx` around lines 9 - 15, The
toggleVisibility function in ScrollToTop currently uses the deprecated
window.pageYOffset; update it to use window.scrollY instead (inside the
useCallback where setIsVisible is called) so the check becomes window.scrollY >
300, leaving the rest of the logic and dependencies unchanged; ensure the
reference appears in the toggleVisibility function and any related scroll
handler cleanup remains correct.
| window.addEventListener('scroll', toggleVisibility, { passive: true }) | ||
| return () => window.removeEventListener('scroll', toggleVisibility) |
There was a problem hiding this comment.
Event listener cleanup options mismatch with registration.
On Line 21, the scroll listener is registered with { passive: true } options, but on Line 22, removeEventListener is called without passing the same options object. While modern browsers typically match listeners by function reference alone for removal, this inconsistency can cause the listener to not be properly removed in some older browser implementations and contradicts the pattern established for passive listeners.
🔧 Proposed fix
useEffect(() => {
// Check initial scroll position
toggleVisibility()
- window.addEventListener('scroll', toggleVisibility, { passive: true })
- return () => window.removeEventListener('scroll', toggleVisibility)
+ const options = { passive: true }
+ window.addEventListener('scroll', toggleVisibility, options)
+ return () => window.removeEventListener('scroll', toggleVisibility, options)
}, [toggleVisibility])📝 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.
| window.addEventListener('scroll', toggleVisibility, { passive: true }) | |
| return () => window.removeEventListener('scroll', toggleVisibility) | |
| useEffect(() => { | |
| // Check initial scroll position | |
| toggleVisibility() | |
| const options = { passive: true } | |
| window.addEventListener('scroll', toggleVisibility, options) | |
| return () => window.removeEventListener('scroll', toggleVisibility, options) | |
| }, [toggleVisibility]) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/common/ScrollToTop.tsx` around lines 21 - 22, The
scroll listener is added with an options object but removed without it, which
can prevent removal in some environments; update the ScrollToTop component to
use a single options constant (e.g., const scrollOptions = { passive: true })
and pass that same scrollOptions to both window.addEventListener('scroll',
toggleVisibility, scrollOptions) and window.removeEventListener('scroll',
toggleVisibility, scrollOptions) so the listener matches correctly when cleaned
up.
- Add ScrollToTop component at frontend/src/components/common/ScrollToTop.tsx - Button appears when scrolling down more than 300px - Smooth scroll animation to top on click - Fade-in/fade-out animations - Dark theme styling with indigo-600 - Accessible with aria-label and keyboard focusable - No new dependencies, uses CSS transitions Closes SolFoundry#340
1 participant
Description
Closes #
Solana Wallet for Payout
**Wallet:8dgSUWh7Bm3VWkeac7py5xBoePGjiHG1j7eoYKoVboJr
Type of Change
Checklist
console.logor debugging code left behindTesting
Screenshots (if applicable)
Additional Notes