fix(webapp):> Fix for mouse hover on a timeline not being updates after resize #2780
Conversation
Refactor MousePositionProvider to improve reactivity during resize, scroll, and layout shifts.
|
WalkthroughThe Timeline component was refactored to improve position tracking and synchronization across multiple event sources. The component now uses a Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/webapp/app/components/primitives/Timeline.tsx (1)
22-23: Consider clearinglastClienton mouse leave.The refs are correctly used to store state that shouldn't trigger re-renders. However,
lastClient.currentretains the last mouse position even after the mouse exits the container. While not critical, clearing it in theonMouseLeavehandler would be more precise.Apply this diff to clear the ref on mouse leave:
<div ref={ref} onMouseEnter={handleMouseMove} - onMouseLeave={() => setPosition(undefined)} + onMouseLeave={() => { + setPosition(undefined); + lastClient.current = null; + }} onMouseMove={handleMouseMove} style={{ width: "100%", height: "100%" }} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webapp/app/components/primitives/Timeline.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
Files:
apps/webapp/app/components/primitives/Timeline.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/components/primitives/Timeline.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
apps/webapp/app/components/primitives/Timeline.tsx
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access all environment variables through the
envexport ofenv.server.tsinstead of directly accessingprocess.envin the Trigger.dev webapp
Files:
apps/webapp/app/components/primitives/Timeline.tsx
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, use subpath exports from the package.json instead of importing from the root path
Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webapp
Files:
apps/webapp/app/components/primitives/Timeline.tsx
**/*.{js,ts,jsx,tsx,json,md,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier
Files:
apps/webapp/app/components/primitives/Timeline.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). (23)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
apps/webapp/app/components/primitives/Timeline.tsx (4)
8-8: LGTM!The addition of
useEffectis necessary for the new resize/scroll handling and animation frame logic.
25-41: LGTM!The
computeFromClienthelper effectively centralizes position calculation logic. The boundary checks correctly prevent position updates when the cursor is outside the container.
43-49: LGTM!The updated mouse handler correctly stores the last client coordinates and delegates calculation to
computeFromClient.
52-75: LGTM!The resize and scroll handling effectively addresses the core issue. Using
ResizeObserverfor container changes and scroll capture for ancestor scrolling are appropriate choices. The cleanup logic properly disconnects all observers and listeners.
| useEffect(() => { | ||
| if (position === undefined || !lastClient.current) return; | ||
|
|
||
| const tick = () => { | ||
| const lc = lastClient.current; | ||
| if (lc) computeFromClient(lc.clientX, lc.clientY); | ||
| rafId.current = requestAnimationFrame(tick); | ||
| }; | ||
|
|
||
| rafId.current = requestAnimationFrame(tick); | ||
| return () => { | ||
| if (rafId.current !== null) cancelAnimationFrame(rafId.current); | ||
| rafId.current = null; | ||
| }; | ||
| }, [position, computeFromClient]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Optimize the animation frame loop to reduce performance overhead.
The continuous RAF loop runs at ~60fps whenever the mouse is hovering, repeatedly calling getBoundingClientRect() even when the container is static. This forced layout operation on every frame can impact performance, especially with multiple timeline instances or on lower-end devices.
Consider these optimizations:
- Add a transition/animation detector to only run the RAF loop when the container is actually animating
- Use a throttle or debounce mechanism to reduce frequency
- Add a flag to control when continuous updates are needed
Additionally, the effect's dependency on position causes the RAF loop to restart whenever position updates during animations, which is inefficient.
Example: Only run RAF during transitions
useEffect(() => {
- if (position === undefined || !lastClient.current) return;
+ if (position === undefined || !lastClient.current || !ref.current) return;
+
+ // Check if container or ancestors are transitioning/animating
+ const isAnimating = () => {
+ if (!ref.current) return false;
+ const styles = window.getComputedStyle(ref.current);
+ // Check for ongoing transitions or animations
+ return styles.transition !== 'none' || styles.animation !== 'none';
+ };
const tick = () => {
const lc = lastClient.current;
- if (lc) computeFromClient(lc.clientX, lc.clientY);
- rafId.current = requestAnimationFrame(tick);
+ if (lc) {
+ computeFromClient(lc.clientX, lc.clientY);
+ // Only continue RAF if actively animating
+ if (isAnimating()) {
+ rafId.current = requestAnimationFrame(tick);
+ } else {
+ rafId.current = null;
+ }
+ }
};
rafId.current = requestAnimationFrame(tick);
return () => {
if (rafId.current !== null) cancelAnimationFrame(rafId.current);
rafId.current = null;
};
}, [position, computeFromClient]);Or consider removing the RAF loop entirely and relying on the ResizeObserver and scroll/resize listeners, which should handle most layout changes adequately.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/webapp/app/components/primitives/Timeline.tsx around lines 77–91, the
current RAF loop runs continuously while hovering and forces layout each frame;
instead start/stop RAF only when the container is actually animating or needs
continuous updates: add a ref/boolean (e.g., needsContinuousUpdate or
isAnimating) and detect animations/transitions via element.getAnimations() or
getComputedStyle(elt).transitionDuration > 0 (and listen for
animationend/transitionend to clear the flag), run requestAnimationFrame only
when that flag is true, cancel it when the flag clears, and replace the
RAF-for-everything approach with ResizeObserver and scroll/resize listeners (or
a throttled handler) to handle static layout changes; also avoid restarting the
effect on every mouse move by removing position from the dependency array and
using refs for computeFromClient, and add a small throttle/debounce on
computeFromClient to reduce frequency while preserving responsiveness.
Refactor MousePositionProvider to improve reactivity during resize, scroll, and layout shifts.
Closes #
✅ Checklist
Testing
In the runs page, after clicking on a segment that opens the side menu the vertical line updates to the new position without mouse movement.
Changelog
Mose hover vertical line on a timeline updates after timeline resize