Conversation
Signed-off-by: ragnep <ragneinfo@gmail.com>
|
Warning Rate limit exceeded@ragnep has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 14 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
WalkthroughThe CustomTooltip component adds layout-aware positioning: ResizeObserver for child and tooltip, window resize/scroll listeners, isBrowser guards, merged child event handlers with safe ref assignment, useLayoutEffect-driven recalculation, and improved cleanup for observers and timers. Changes
Sequence Diagram(s)sequenceDiagram
participant Mount as Component Mount
participant Layout as useLayoutEffect
participant Child as Child Node
participant Tooltip as Tooltip Node
participant ResizeObs as ResizeObserver
participant Window as Window (resize/scroll)
participant Reposition as Recalculate Position
participant Cleanup as Cleanup/Unmount
Mount->>Layout: mount -> setup refs & handlers
Layout->>ResizeObs: create observers for Child & Tooltip
ResizeObs->>Reposition: size change -> trigger recalculation
Child->>Reposition: child resize/measure data
Tooltip->>Reposition: tooltip resize/measure data
Window->>Reposition: resize/scroll events trigger recalculation
Reposition->>Reposition: compute placement (isBrowser guards)
Cleanup->>ResizeObs: disconnect observers
Cleanup->>Window: remove listeners & clear timers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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.
Actionable comments posted: 0
🧹 Nitpick comments (8)
components/utils/tooltip/CustomTooltip.tsx (8)
49-52: Timer typing and guards for DOM/Node compatibilityUse ReturnType and guard clearTimeout to avoid ambient NodeJS typings conflicts across browser builds.
- const showTimer = useRef<NodeJS.Timeout | undefined>(undefined); - const hideTimer = useRef<NodeJS.Timeout | undefined>(undefined); + const showTimer = useRef<ReturnType<typeof setTimeout> | undefined>(undefined); + const hideTimer = useRef<ReturnType<typeof setTimeout> | undefined>(undefined); @@ - clearTimeout(hideTimer.current); + if (hideTimer.current !== undefined) clearTimeout(hideTimer.current); @@ - clearTimeout(showTimer.current); + if (showTimer.current !== undefined) clearTimeout(showTimer.current); @@ - clearTimeout(showTimer.current); - clearTimeout(hideTimer.current); + if (showTimer.current !== undefined) clearTimeout(showTimer.current); + if (hideTimer.current !== undefined) clearTimeout(hideTimer.current);Also applies to: 213-216, 218-221, 296-303
73-86: Observer can go stale if the child DOM node changes while visibleIf the trigger element is replaced (e.g., conditional render), the existing ResizeObserver will keep observing the old node until visibility flips. Rebind on ref changes.
- const assignChildNode = useCallback( - (node: HTMLElement | null) => { + const assignChildNode = useCallback( + (node: HTMLElement | null) => { childRef.current = node; @@ - setRefValue(originalRef, node); + setRefValue(originalRef, node); + // Rebind observer if tooltip is visible and observer exists + if (isVisible && childObserverRef.current && node) { + try { + childObserverRef.current.disconnect(); + childObserverRef.current.observe(node); + } catch {} + } }, - [originalRef, setRefValue] + [originalRef, setRefValue, isVisible] );Also applies to: 256-278
283-294: Throttle scroll/resize reposition with rAFCalling calculatePosition on every scroll/resize can jank. Batch per frame.
- const handleReposition = () => { - calculatePosition(); - }; + let rafId: number | null = null; + const handleReposition = () => { + if (rafId !== null) return; + rafId = requestAnimationFrame(() => { + rafId = null; + calculatePosition(); + }); + }; @@ - return () => { + return () => { window.removeEventListener("resize", handleReposition); window.removeEventListener("scroll", handleReposition, true); + if (rafId !== null) cancelAnimationFrame(rafId); };
77-82: Avoid console noise in productionGate the warning so production builds stay clean.
- if (node && typeof node.getBoundingClientRect !== "function") { - console.warn( - "[CustomTooltip] Child ref is not an HTMLElement; tooltip positioning may fail." - ); - } + if (process.env.NODE_ENV !== "production") { + if (node && typeof (node as any).getBoundingClientRect !== "function") { + console.warn("[CustomTooltip] Child ref is not an HTMLElement; tooltip positioning may fail."); + } + }
223-232: useLayoutEffect deps: remove unused isBrowser or early-returnisBrowser is not used in the effect body. Either remove it from deps or add a guard. Prefer removing to avoid unnecessary re-runs.
- }, [isVisible, calculatePosition, isBrowser]); + }, [isVisible, calculatePosition]);
3-10: Add aria linkage for accessible tooltipsExpose an id on the tooltip and reference it from the trigger via aria-describedby. Helps screen readers and aligns with a11y best practices.
import React, { useState, useEffect, useRef, useCallback, useLayoutEffect, MutableRefObject, + useId, } from "react"; @@ }: CustomTooltipProps) { + const tooltipId = useId(); @@ const clonedChild = React.cloneElement( childElement, { ref: assignChildNode, + "aria-describedby": isVisible ? tooltipId : undefined, @@ <div ref={tooltipRef} + id={tooltipId} role="tooltip"Also applies to: 40-46, 305-334, 340-345
338-340: Styling via CSS Modules in TSX; guideline prefers TailwindThis component uses SCSS module classes. Our .tsx guideline favors TailwindCSS. Consider migrating classes (tooltip container, content, arrow) to Tailwind utilities or exposing a Tailwind-friendly API.
As per coding guidelines
Also applies to: 340-368
193-209: Minor: isBrowser guard already implied by "use client"calculatePosition early-returns on !isBrowser; fine, but including isBrowser in the callback deps forces re-memoization with no behavior change. Safe to drop for stability.
- }, [getOptimalPlacement, calculateInitialPosition, adjustPositionForViewport, calculateArrowPosition, isBrowser]); + }, [getOptimalPlacement, calculateInitialPosition, adjustPositionForViewport, calculateArrowPosition]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/utils/tooltip/CustomTooltip.tsx(7 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: Do not include any comments in the code
Use react-query for data fetching
Always add readonly before propsUse TypeScript across the codebase
Files:
components/utils/tooltip/CustomTooltip.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursorrules)
**/*.tsx: Use FontAwesome for icons
Use TailwindCSS for stylingUse React functional components with hooks for UI components
Files:
components/utils/tooltip/CustomTooltip.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
components/utils/tooltip/CustomTooltip.tsx (1)
305-334: Ref injection requires a DOM-ref-capable childIf children are function components without forwardRef (or not passing ref through under React 19’s “ref as prop”), assignChildNode won’t receive a DOM node; positioning will no-op.
Please confirm that consumers always pass a DOM element or a component that forwards ref to a DOM node. If not, I can propose a wrapper or event-target fallback to capture the element.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
codex/tickets/TKT-0020.md (1)
28-29: Add the primary PR link.Populate the placeholder with this PR for traceability.
- - Primary PR: _(add when available)_ + - Primary PR: [#1575](https://github.com/6529-Collections/6529seize-frontend/pull/1575)codex/STATE.md (1)
26-26: Link the active PR in the PRs column.Keeps the board actionable.
-| TKT-0020 | Harden CustomTooltip positioning robustness | In-Progress | P1 | openai-assistant | — | 2025-10-28 | +| TKT-0020 | Harden CustomTooltip positioning robustness | In-Progress | P1 | openai-assistant | [#1575](https://github.com/6529-Collections/6529seize-frontend/pull/1575) | 2025-10-28 |components/utils/tooltip/CustomTooltip.tsx (7)
53-54: Unify timer typings for browser/Node.Prefer ReturnType to avoid NodeJS.Timeout mismatches in DOM builds.
- const showTimer = useRef<NodeJS.Timeout | undefined>(undefined); - const hideTimer = useRef<NodeJS.Timeout | undefined>(undefined); + const showTimer = useRef<ReturnType<typeof setTimeout> | undefined>(undefined); + const hideTimer = useRef<ReturnType<typeof setTimeout> | undefined>(undefined);
24-29: Support non-HTML children (e.g., SVG) and improve ref typing.Widen event/ref types from HTMLElement to Element; keeps measurement working on any Element with getBoundingClientRect.
-type TooltipChildHandlers = { - onMouseEnter?: React.MouseEventHandler<HTMLElement>; - onMouseLeave?: React.MouseEventHandler<HTMLElement>; - onFocus?: React.FocusEventHandler<HTMLElement>; - onBlur?: React.FocusEventHandler<HTMLElement>; +type TooltipChildHandlers = { + onMouseEnter?: React.MouseEventHandler<Element>; + onMouseLeave?: React.MouseEventHandler<Element>; + onFocus?: React.FocusEventHandler<Element>; + onBlur?: React.FocusEventHandler<Element>; }; -const childRef = useRef<HTMLElement>(null); +const childRef = useRef<Element>(null); -const originalRef = (childElement as React.ReactElement & { - ref?: React.Ref<HTMLElement>; +const originalRef = (childElement as React.ReactElement & { + ref?: React.Ref<Element>; }).ref; -const setRefValue = useCallback((ref: React.Ref<HTMLElement> | undefined, node: HTMLElement | null) => { +const setRefValue = useCallback((ref: React.Ref<Element> | undefined, node: Element | null) => { if (!ref) return; if (typeof ref === "function") { ref(node); return; } try { - (ref as React.MutableRefObject<HTMLElement | null>).current = node; + (ref as React.MutableRefObject<Element | null>).current = node; } catch { console.warn("[CustomTooltip] Failed to assign ref (may be read-only)"); } }, []); -const assignChildNode = useCallback( - (node: HTMLElement | null) => { +const assignChildNode = useCallback( + (node: Element | null) => { childRef.current = node; if (node && typeof node.getBoundingClientRect !== "function") { console.warn( "[CustomTooltip] Child ref is not an HTMLElement; tooltip positioning may fail." ); } setRefValue(originalRef, node); }, [originalRef, setRefValue] ); const clonedChild = React.cloneElement( childElement, { ref: assignChildNode, - onMouseEnter: mergeHandlers<React.MouseEvent<HTMLElement>>( + onMouseEnter: mergeHandlers<React.MouseEvent<Element>>( () => { show(); }, childElement.props.onMouseEnter ), - onMouseLeave: mergeHandlers<React.MouseEvent<HTMLElement>>( + onMouseLeave: mergeHandlers<React.MouseEvent<Element>>( () => { hide(); }, childElement.props.onMouseLeave ), - onFocus: mergeHandlers<React.FocusEvent<HTMLElement>>( + onFocus: mergeHandlers<React.FocusEvent<Element>>( () => { show(); }, childElement.props.onFocus ), - onBlur: mergeHandlers<React.FocusEvent<HTMLElement>>( + onBlur: mergeHandlers<React.FocusEvent<Element>>( () => { hide(); }, childElement.props.onBlur ), - } as React.Attributes & TooltipChildHandlers + } as React.Attributes & TooltipChildHandlers );Also applies to: 51-61, 62-74, 75-89, 315-339
90-97: Call consumer handlers first, then internal ones.Preserves child semantics if they preventDefault/stopPropagation.
-const mergeHandlers = useCallback( - <E extends React.SyntheticEvent>(ourHandler: (event: E) => void, theirHandler?: (event: E) => void) => - (event: E) => { - ourHandler(event); - theirHandler?.(event); - }, - [] -); +const mergeHandlers = useCallback( + <E extends React.SyntheticEvent>(our: (event: E) => void, theirs?: (event: E) => void) => + (event: E) => { + theirs?.(event); + our(event); + }, + [] +);
285-301: Throttle scroll/resize reflows with requestAnimationFrame.Prevents excessive synchronous layout reads during fast scrolling.
- const childObserverRef: MutableRefObject<ResizeObserver | null> = useRef(null); - const tooltipObserverRef: MutableRefObject<ResizeObserver | null> = useRef(null); + const childObserverRef: MutableRefObject<ResizeObserver | null> = useRef(null); + const tooltipObserverRef: MutableRefObject<ResizeObserver | null> = useRef(null); + const repositionRaf = useRef<number | null>(null); useEffect(() => { if (!isVisible) return; if (!isBrowser) return; - const handleReposition = () => { - calculatePosition(); - }; + const handleReposition = () => { + if (repositionRaf.current != null) return; + repositionRaf.current = requestAnimationFrame(() => { + repositionRaf.current = null; + calculatePosition(); + }); + }; win.addEventListener("resize", handleReposition); win.addEventListener("scroll", handleReposition, true); return () => { win.removeEventListener("resize", handleReposition); win.removeEventListener("scroll", handleReposition, true); + if (repositionRaf.current != null) { + cancelAnimationFrame(repositionRaf.current); + repositionRaf.current = null; + } }; }, [isVisible, calculatePosition]); useEffect(() => { return () => { clearTimeout(showTimer.current); clearTimeout(hideTimer.current); childObserverRef.current?.disconnect(); tooltipObserverRef.current?.disconnect(); + if (repositionRaf.current != null) { + cancelAnimationFrame(repositionRaf.current); + repositionRaf.current = null; + } }; }, []);Also applies to: 55-57, 302-310
3-10: Improve accessibility with an explicit aria-describedby link.Screen readers can associate the trigger with tooltip content.
import React, { useState, useEffect, useRef, useCallback, useLayoutEffect, MutableRefObject, + useId, } from "react"; @@ export default function CustomTooltip({ children, content, placement = "top", delayShow = 700, delayHide = 0, disabled = false, offset = 8, }: CustomTooltipProps) { + const tooltipId = useId(); @@ const clonedChild = React.cloneElement( childElement, { ref: assignChildNode, + 'aria-describedby': [childElement.props?.['aria-describedby'], tooltipId].filter(Boolean).join(' ') || undefined, @@ - } as React.Attributes & TooltipChildHandlers + } as React.Attributes & TooltipChildHandlers & React.AriaAttributes ); @@ - <div + <div ref={tooltipRef} + id={tooltipId} role="tooltip"Also applies to: 311-340, 346-356
14-22: Optional: support interactive hover-cards. Verify needed for profile cards.If the profile hover-card contains interactive elements, pointerEvents: 'none' blocks interaction and causes immediate hide on mouseout from trigger. Suggest an opt-in interactive mode.
interface CustomTooltipProps { readonly children: React.ReactElement; readonly content: React.ReactNode; readonly placement?: "top" | "bottom" | "left" | "right" | "auto"; readonly delayShow?: number; readonly delayHide?: number; readonly disabled?: boolean; readonly offset?: number; + readonly interactive?: boolean; } @@ - style={{ + onMouseEnter={interactive ? () => show() : undefined} + onMouseLeave={interactive ? () => hide() : undefined} + style={{ position: 'fixed', left: `${position.x}px`, top: `${position.y}px`, zIndex: 999999, - pointerEvents: 'none', + pointerEvents: interactive ? 'auto' : 'none', }}Also applies to: 346-356
69-73: Gate console warnings to development.Avoid noisy logs in production.
- } catch { - console.warn("[CustomTooltip] Failed to assign ref (may be read-only)"); + } catch { + if (process.env.NODE_ENV !== "production") { + console.warn("[CustomTooltip] Failed to assign ref (may be read-only)"); + } } @@ - if (node && typeof node.getBoundingClientRect !== "function") { - console.warn( - "[CustomTooltip] Child ref is not an HTMLElement; tooltip positioning may fail." - ); + if (node && typeof (node as any).getBoundingClientRect !== "function") { + if (process.env.NODE_ENV !== "production") { + console.warn("[CustomTooltip] Child ref is not an HTMLElement; tooltip positioning may fail."); + } }Also applies to: 79-84
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
codex/STATE.md(1 hunks)codex/tickets/TKT-0020.md(1 hunks)components/utils/tooltip/CustomTooltip.tsx(7 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: Do not include any comments in the code
Use react-query for data fetching
Always add readonly before propsUse TypeScript across the codebase
Files:
components/utils/tooltip/CustomTooltip.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursorrules)
**/*.tsx: Use FontAwesome for icons
Use TailwindCSS for stylingUse React functional components with hooks for UI components
Files:
components/utils/tooltip/CustomTooltip.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
|
2 participants
Summary by CodeRabbit
Bug Fixes
Chores / Documentation