fix: hide feedback button when it overlaps with navbar#735
fix: hide feedback button when it overlaps with navbar#735an1ndra wants to merge 3 commits intoTanStack:mainfrom
Conversation
👷 Deploy request for tanstack pending review.Visit the deploys page to approve it
|
|
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/components/DocFeedbackProvider.tsx (3)
415-416: Removevoid scrollY- the variable is already used in the dependency array implicitly.The
void scrollYstatement is a workaround to suppress an unused variable warning, but ESLint and TypeScript typically don't flag state variables as unused when they trigger re-renders. If a linter is flagging this, consider adding a comment explaining the purpose instead.♻️ Replace with explanatory comment
- // Suppress unused state warning - scrollY triggers re-renders - void scrollY + // Note: scrollY state is intentionally "unused" - its updates trigger re-renders + // so that getBoundingClientRect() returns fresh values during scroll🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DocFeedbackProvider.tsx` around lines 415 - 416, Remove the unnecessary "void scrollY" statement in DocFeedbackProvider (the scrollY state/variable), and instead add a brief explanatory comment near where scrollY is declared/used that explains it is intentionally included only to trigger re-renders (and therefore appears in effect dependency arrays); ensure any useEffect or hook that relies on scrollY includes it in its dependency array so linters understand the variable is intentionally used.
405-408: Hardcoded button height may become stale if styling changes.The
buttonHeight = 24assumesw-6 h-6Tailwind classes (6 × 4px = 24px). IfDocFeedbackFloatingButtonstyling changes, this will silently break the overlap calculation.♻️ Consider extracting as a shared constant or measuring dynamically
A shared constant near
DocFeedbackFloatingButtonwould be more maintainable:// In DocFeedbackFloatingButton.tsx or a shared constants file export const FEEDBACK_BUTTON_HEIGHT = 24 // Then import and use here import { FEEDBACK_BUTTON_HEIGHT } from './DocFeedbackFloatingButton' // ... const buttonTop = blockRect.top - FEEDBACK_BUTTON_HEIGHT🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DocFeedbackProvider.tsx` around lines 405 - 408, The hardcoded buttonHeight (24) used to compute buttonTop can drift if DocFeedbackFloatingButton's styling changes; replace the literal with a shared exported constant (e.g., export FEEDBACK_BUTTON_HEIGHT from DocFeedbackFloatingButton or a shared constants file) and import it here to compute buttonTop (replace usage of buttonHeight with the imported FEEDBACK_BUTTON_HEIGHT), or alternatively measure the actual element height at runtime via getBoundingClientRect on the DocFeedbackFloatingButton node and use that value to compute buttonTop so the overlap stays correct.
358-377: Consider throttling scroll handler to reduce re-render frequency.Scroll events fire at high frequency (potentially 60+ times per second during active scrolling). Each
setScrollYcall triggers a re-render that runsgetBoundingClientRect()andgetComputedStyle()DOM queries. While{ passive: true }prevents blocking the main thread, the render volume could still cause jank on lower-powered devices.♻️ Suggested throttle implementation
// Track scroll position to trigger re-renders for navbar overlap check React.useEffect(() => { if (!mounted) return + let rafId: number | null = null + const handleScroll = () => { - setScrollY(window.scrollY) + if (rafId === null) { + rafId = requestAnimationFrame(() => { + setScrollY(window.scrollY) + rafId = null + }) + } } - const handleResize = () => { - setScrollY(window.scrollY) - } - window.addEventListener('scroll', handleScroll, { passive: true }) - window.addEventListener('resize', handleResize) + window.addEventListener('resize', handleScroll) return () => { window.removeEventListener('scroll', handleScroll) - window.removeEventListener('resize', handleResize) + window.removeEventListener('resize', handleScroll) + if (rafId !== null) { + cancelAnimationFrame(rafId) + } } }, [mounted])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DocFeedbackProvider.tsx` around lines 358 - 377, The scroll handler (handleScroll) calls setScrollY on every event causing frequent re-renders; inside the useEffect that depends on mounted, wrap/debounce the handler with a throttle mechanism (preferably requestAnimationFrame or a small lodash.throttle) so setScrollY is only invoked at animation frame rate or limited frequency, apply the same throttling to handleResize, store any RAF id or throttle cancel token in a ref, and ensure the cleanup function cancels the RAF or calls the throttle cancel before removing the event listeners; keep existing event options (e.g., { passive: true }) and reference the existing symbols mounted, handleScroll, handleResize, setScrollY, and the useEffect cleanup to implement this.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/DocFeedbackProvider.tsx`:
- Around line 415-416: Remove the unnecessary "void scrollY" statement in
DocFeedbackProvider (the scrollY state/variable), and instead add a brief
explanatory comment near where scrollY is declared/used that explains it is
intentionally included only to trigger re-renders (and therefore appears in
effect dependency arrays); ensure any useEffect or hook that relies on scrollY
includes it in its dependency array so linters understand the variable is
intentionally used.
- Around line 405-408: The hardcoded buttonHeight (24) used to compute buttonTop
can drift if DocFeedbackFloatingButton's styling changes; replace the literal
with a shared exported constant (e.g., export FEEDBACK_BUTTON_HEIGHT from
DocFeedbackFloatingButton or a shared constants file) and import it here to
compute buttonTop (replace usage of buttonHeight with the imported
FEEDBACK_BUTTON_HEIGHT), or alternatively measure the actual element height at
runtime via getBoundingClientRect on the DocFeedbackFloatingButton node and use
that value to compute buttonTop so the overlap stays correct.
- Around line 358-377: The scroll handler (handleScroll) calls setScrollY on
every event causing frequent re-renders; inside the useEffect that depends on
mounted, wrap/debounce the handler with a throttle mechanism (preferably
requestAnimationFrame or a small lodash.throttle) so setScrollY is only invoked
at animation frame rate or limited frequency, apply the same throttling to
handleResize, store any RAF id or throttle cancel token in a ref, and ensure the
cleanup function cancels the RAF or calls the throttle cancel before removing
the event listeners; keep existing event options (e.g., { passive: true }) and
reference the existing symbols mounted, handleScroll, handleResize, setScrollY,
and the useEffect cleanup to implement this.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 770047cf-68db-4e2a-a362-2d7049e1a0c2
📒 Files selected for processing (1)
src/components/DocFeedbackProvider.tsx
1 participant
Fix: #730
Before:
After:
Summary by CodeRabbit