fix: hide feedback button when it overlaps with navbar#735
fix: hide feedback button when it overlaps with navbar#735an1ndra wants to merge 4 commits intoTanStack:mainfrom
Conversation
✅ Deploy Preview for tanstack ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
📝 WalkthroughWalkthroughThe BlockButton in 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
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/DocFeedbackProvider.tsx (1)
386-435:⚠️ Potential issue | 🟠 MajorPer-block scroll listeners create unnecessary performance overhead—consider lifting to provider or evaluating the z-index alternative from
#730.Two actionable issues:
Performance concern (major). Each
BlockButtoninstance attaches its ownscrollandresizelisteners (lines 398–403), which on a typical docs page with dozens of blocks means dozens of listeners firing on every scroll tick. Each firessetScrollY, triggering a re-render that callsgetComputedStyle(...)andblock.getBoundingClientRect()synchronously—an O(N) cost per scroll frame.Fix: Lift the scroll/resize subscription to
DocFeedbackProvideras a single listener broadcasting via context, or userequestAnimationFramethrottling so updates fire at most once per frame.Code smell:
scrollYstate as a re-render trigger. Lines 380–438 declarescrollYstate purely to force re-renders when scroll changes, then suppress the unused variable warning withvoid scrollY. A cleaner pattern: use a counter (const [tick, setTick] = useState(0)) and callsetTick(t => t+1)on scroll to explicitly signal intent for re-renders.Minor: Line 429 hardcodes
buttonHeight = 24based on the Tailwindw-6 h-6class, but the actual renderedDocFeedbackFloatingButtonheight is not enforced anywhere. Consider measuring via a ref or exporting a constant.Note on alternatives: Both navbar and button carry
z-[100]. A simpler CSS-only fix—lowering the button's z-index below the navbar (e.g.,z-[60])—would let the navbar paint over it with zero runtime cost. Verify against issue#730's actual requirements to determine if this approach is sufficient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DocFeedbackProvider.tsx` around lines 386 - 435, This per-block effect in DocFeedbackProvider.tsx (the useEffect that adds 'scroll' and 'resize' listeners and uses setScrollY/scrollY) creates many listeners and costly synchronous layout checks; move the scroll/resize subscription out of the per-block scope into the provider level (DocFeedbackProvider) and publish a single scroll tick via context, or throttle updates with requestAnimationFrame so updates happen at most once per frame; replace the scrollY state used purely to force renders with a simple counter state (e.g., tick and setTick(t => t+1)) so intent is explicit, and when deciding button overlap use that single tick-driven re-render to run getComputedStyle and block.getBoundingClientRect(); finally, stop hardcoding buttonHeight (currently buttonHeight = 24) by measuring the floating button via a ref or exposing a constant used by DocFeedbackFloatingButton so overlap calculation is accurate.
🧹 Nitpick comments (2)
src/components/DocFeedbackProvider.tsx (2)
427-435: HardcodedbuttonHeight = 24can desync from the actual button.The check assumes
DocFeedbackFloatingButtonrenders at exactly 24px tall (w-6 h-6). If that component's size ever changes (padding, icon swap, hover scale, etc.), the overlap math silently goes wrong and the button will either flash through the navbar or hide too eagerly.Prefer measuring the rendered portal container with a ref /
getBoundingClientRect(), or at least extract a named constant near theDocFeedbackFloatingButtondefinition so the two stay coupled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DocFeedbackProvider.tsx` around lines 427 - 435, The hardcoded buttonHeight (24) used to compute buttonTop in DocFeedbackProvider can drift from the actual DocFeedbackFloatingButton size; replace the magic number by measuring the rendered button container (e.g., use a ref on the portal/root element for DocFeedbackFloatingButton and call getBoundingClientRect() to get its height) and compute buttonTop from that measured height, or alternatively export a shared constant from the DocFeedbackFloatingButton module and import it into DocFeedbackProvider; ensure the navbar overlap check uses the measuredHeight (or shared constant) instead of buttonHeight and keep the existing navbarHeight and isMenuOpen logic intact.
380-380: ReplacescrollY+void scrollYwith a render-tick state to drop the lint workaround.
scrollY's value is never actually consumed (getBoundingClientRect()already reflects current scroll), it exists solely to trigger a re-render, hence thevoid scrollYescape hatch. A nameless tick state expresses that intent without needing avoidreference:♻️ Proposed change
- const [scrollY, setScrollY] = React.useState(0) + const [, forceTick] = React.useState(0)- const handleScroll = () => { - setScrollY(window.scrollY) - } - - const handleResize = () => { - setScrollY(window.scrollY) - } + const tick = () => forceTick((n) => n + 1) + window.addEventListener('scroll', tick, { passive: true }) + window.addEventListener('resize', tick) + + return () => { + window.removeEventListener('scroll', tick) + window.removeEventListener('resize', tick) + }- // Suppress unused state warning - scrollY triggers re-renders - void scrollY -Also applies to: 437-438
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DocFeedbackProvider.tsx` at line 380, The component DocFeedbackProvider currently introduces state named scrollY (const [scrollY, setScrollY]) solely to force re-renders and uses a `void scrollY` lint workaround; replace that with a dedicated render-tick state (e.g., renderTick and setRenderTick) and remove any `void scrollY` usages. Change every place that calls setScrollY(...) (including the code around lines 437-438) to instead increment the tick via setRenderTick(t => t + 1) so the component re-renders without storing a faux scroll value, and remove any remaining references to the scrollY symbol and the lint escape hatch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/components/DocFeedbackProvider.tsx`:
- Around line 386-435: This per-block effect in DocFeedbackProvider.tsx (the
useEffect that adds 'scroll' and 'resize' listeners and uses setScrollY/scrollY)
creates many listeners and costly synchronous layout checks; move the
scroll/resize subscription out of the per-block scope into the provider level
(DocFeedbackProvider) and publish a single scroll tick via context, or throttle
updates with requestAnimationFrame so updates happen at most once per frame;
replace the scrollY state used purely to force renders with a simple counter
state (e.g., tick and setTick(t => t+1)) so intent is explicit, and when
deciding button overlap use that single tick-driven re-render to run
getComputedStyle and block.getBoundingClientRect(); finally, stop hardcoding
buttonHeight (currently buttonHeight = 24) by measuring the floating button via
a ref or exposing a constant used by DocFeedbackFloatingButton so overlap
calculation is accurate.
---
Nitpick comments:
In `@src/components/DocFeedbackProvider.tsx`:
- Around line 427-435: The hardcoded buttonHeight (24) used to compute buttonTop
in DocFeedbackProvider can drift from the actual DocFeedbackFloatingButton size;
replace the magic number by measuring the rendered button container (e.g., use a
ref on the portal/root element for DocFeedbackFloatingButton and call
getBoundingClientRect() to get its height) and compute buttonTop from that
measured height, or alternatively export a shared constant from the
DocFeedbackFloatingButton module and import it into DocFeedbackProvider; ensure
the navbar overlap check uses the measuredHeight (or shared constant) instead of
buttonHeight and keep the existing navbarHeight and isMenuOpen logic intact.
- Line 380: The component DocFeedbackProvider currently introduces state named
scrollY (const [scrollY, setScrollY]) solely to force re-renders and uses a
`void scrollY` lint workaround; replace that with a dedicated render-tick state
(e.g., renderTick and setRenderTick) and remove any `void scrollY` usages.
Change every place that calls setScrollY(...) (including the code around lines
437-438) to instead increment the tick via setRenderTick(t => t + 1) so the
component re-renders without storing a faux scroll value, and remove any
remaining references to the scrollY symbol and the lint escape hatch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4d62612c-3b6c-48ac-b49e-904edda01523
📒 Files selected for processing (1)
src/components/DocFeedbackProvider.tsx
2 participants
Fix: #730
Before:
After:
Summary by CodeRabbit