⚠️ No Changeset found

Latest commit: 12f508a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

PR Review: Fix mouse hover on timeline not updating after resize

Thanks for this contribution! This PR addresses a real UX issue where the vertical hover line on the timeline doesn't update its position when the container resizes (e.g., when opening a side panel). Here's my feedback:

Code Quality & Design

Overall approach is reasonable - Storing the last client position and recalculating on resize/scroll is the right pattern for this problem.

Good practices observed:

• Proper cleanup in useEffect return functions
• Using useCallback appropriately for memoized handlers
• Using capture phase for scroll events to catch ancestor scrolling

Potential Issues

1. Animation detection logic may be unreliable (lines 80-84)

   const isAnimating = () => {
    const styles = window.getComputedStyle(ref.current);
    return styles.transition !== "none" || styles.animation !== "none";
   };

   This checks if a transition/animation property is set, not if an animation is currently running. Elements with transition: all 0.3s would report as "animating" even when idle, causing unnecessary RAF loops. Consider:

   • Using the transitionend / animationend events instead
   • Or checking for specific animation states

2. Potential infinite RAF loop (lines 77-103)

   The second useEffect depends on position in its dependency array and calls computeFromClient which can update position. If isAnimating() always returns true (due to the issue above), this creates an infinite loop that runs every frame and never stops. This could cause significant performance issues.

3. Memory of stale mouse position (line 22)

   lastClient.current is set on mouseMove but never cleared on mouseLeave. When the user moves mouse out and then the container resizes, you'll recalculate position from a stale coordinate. This might cause the hover line to briefly appear then disappear (since the bounds check will fail), which could cause visual flickering.

   Consider clearing lastClient.current when onMouseLeave fires:

   onMouseLeave={() => {
    lastClient.current = null;
    setPosition(undefined);
   }}

Performance Considerations

• Scroll listener is very broad - Adding a capture-phase scroll listener to window will fire on every scroll event anywhere in the document. For complex pages, this could add overhead. Consider debouncing/throttling the scroll handler.

• Multiple getComputedStyle calls - Each tick() call in the RAF loop calls getComputedStyle(), which can force layout. In the worst case (if animation detection is stuck), this runs every frame.

Suggestions

1. Simplest fix for the animation issue - Remove the animation-tracking useEffect entirely and just rely on ResizeObserver. ResizeObserver will naturally fire during CSS animations that change dimensions. If animations don't change the size, the position shouldn't change either.

2. If animation tracking is needed - Use event listeners:

   useEffect(() => {
    const el = ref.current;
    if (!el) return;
    
    const onAnimationFrame = () => {
    const lc = lastClient.current;
    if (lc) computeFromClient(lc.clientX, lc.clientY);
    };
    
    el.addEventListener('transitionrun', onAnimationFrame);
    el.addEventListener('animationstart', onAnimationFrame);
    // ... cleanup
   }, [computeFromClient]);

3. Consider debouncing scroll handler:

   const onRecalc = useMemo(() => 
    debounce(() => {
    const lc = lastClient.current;
    if (lc) computeFromClient(lc.clientX, lc.clientY);
    }, 16), // ~1 frame
   [computeFromClient]);

Test Coverage

No tests exist for this component (Timeline.tsx). While the component is primarily visual, the new logic (especially the animation detection and RAF handling) would benefit from unit tests to prevent regressions.

Summary

The core idea is sound, but there are potential edge cases that could cause performance issues or visual glitches. The animation detection logic in the second useEffect is the main concern - it could lead to unnecessary continuous RAF loops on elements with always-present transition/animation CSS properties.

Recommendation: Consider simplifying by removing the animation-tracking useEffect and relying solely on ResizeObserver + scroll/resize listeners. If the animation tracking is essential for the use case, please use event-based detection (transitionrun/animationstart) instead of polling getComputedStyle.