fix(webapp): synchronize run page highlight when hovering spans#2998
fix(webapp): synchronize run page highlight when hovering spans#2998
Conversation
|
WalkthroughAdds hover-aware interactions and scroll-aware hover deferral to the runs route. Introduces Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
nicktrn
changed the title
nicktrn
left a comment
nicktrn
left a comment
There was a problem hiding this comment.
PR Review: Synchronize run page highlight on hover
Overall: The feature goal is good — syncing hover highlights between the tree panel and timeline panel is a clear UX improvement. However, the current implementation has a high-severity bug, significant performance concerns, and several code quality issues that should be addressed before merging.
HIGH — Stale hover after mousewheel scroll
When scrolling via mousewheel while the cursor hovers over a row, the rows move under the mouse but onMouseEnter/onMouseLeave events do not fire for the new rows appearing beneath the cursor. The pendingHoverIdRef retains the ID of the row that was under the mouse when scrolling started. When the 150ms debounce expires, that stale ID is applied — highlighting a row the user isn't pointing at.
Similarly, when virtualized rows are recycled during scroll (TanStack Virtual unmounts/mounts row components), no onMouseLeave fires for the unmounted row, leaving pendingHoverIdRef stale.
Suggestion: When scroll ends, set hoveredNodeId to null instead of applying the ref value. Let the next natural mouse event re-establish hover:
scrollTimeoutRef.current = setTimeout(() => {
setIsScrolling(false);
setHoveredNodeId(null);
pendingHoverIdRef.current = null;
}, 150);HIGH — Performance: every hover re-renders the entire tree + timeline
hoveredNodeId is React state in TasksTreeView. Every setHoveredNodeId call re-renders the parent, which re-renders both TreeView components (tree + timeline). Key factors:
TreeViewis not wrapped inReact.memorenderNodecallbacks are inline arrow functions, recreated every render- Virtualizer uses
overscan: 50, so ~70-80 rows re-render per panel per hover event (~140-160 total) - Timeline rows are expensive (multiple
Timeline.Point,motion.div, Framer Motion animations)
Suggestion: Consider a CSS/DOM-based approach that avoids React re-renders entirely. Add data-node-id attributes to rows and toggle a data-hovered attribute via refs + direct DOM manipulation. The hover highlight is purely visual — it doesn't need to participate in React's render cycle. If that feels too imperative, an intermediate option is to add pointer-events: none to the scroll container during scroll (one state toggle on the container, not per-node), and keep CSS :hover for the actual highlight.
MEDIUM — CSS hover replaced with JS hover adds latency
The PR replaces native CSS :hover pseudo-classes (hover:bg-grid-dimmed) with React state-driven styling. CSS :hover is handled on the compositor thread and is effectively instant. The JS approach adds at minimum one frame of latency (~16ms) through the React event → setState → reconciliation → paint pipeline, and more under load. Users will notice the highlight trailing the mouse when scanning quickly across rows.
LOW — setHoveredNodeId prop passed but unused in TimelineView
TimelineViewProps includes setHoveredNodeId, but TimelineView only uses handleHoverChange (which wraps setHoveredNodeId with scroll-aware logic). The raw setter shouldn't be exposed — it bypasses scroll suppression and is dead code.
LOW — handleHoverChange dependency on isScrolling state destabilizes callback identity
const handleHoverChange = useCallback((nodeId) => {
pendingHoverIdRef.current = nodeId;
if (!isScrolling) {
setHoveredNodeId(nodeId);
}
}, [isScrolling]); // changes every time isScrolling togglesWhen isScrolling toggles, handleHoverChange gets a new identity. This would bust any React.memo on child components receiving it. If isScrolling were tracked as a ref instead of state, the callback would be stable ([] deps).
LOW — Duplicated hover styling logic
The hover class logic is copy-pasted in both the tree renderNode and timeline renderNode:
state.selected
? isHovered ? "bg-grid-bright" : "bg-grid-dimmed"
: isHovered ? "bg-grid-dimmed" : "bg-transparent"Could be extracted into a helper like getRowBgClass(selected, hovered).
LOW — Indentation bug in handleScroll
scrollTimeoutRef.current = setTimeout(() => {
setIsScrolling(false);
setHoveredNodeId(pendingHoverIdRef.current); // 8 spaces, should be 6
}, 150);LOW — Unused parameter
handleScroll takes scrollTop: number but never reads it. Should be (_scrollTop: number) => void or () => void.
LOW — NodeJS.Timeout type in browser context
useRef<NodeJS.Timeout | null> — in a browser environment this should be ReturnType<typeof setTimeout> for correctness.
| const [hoveredNodeId, setHoveredNodeId] = useState<string | null>(null); | ||
| const [isScrolling, setIsScrolling] = useState(false); | ||
| const scrollTimeoutRef = useRef<NodeJS.Timeout | null>(null); | ||
| const pendingHoverIdRef = useRef<string | null>(null); | ||
|
|
||
| useEffect(() => { | ||
| return () => { | ||
| if (scrollTimeoutRef.current) { |
There was a problem hiding this comment.
I am pretty sure this is going to cause a re-render of the entire tree every time a hover changes. Which will crush the rendering performance in the browser for complex trees.
We have to very careful to avoid the inputs to the tree view changing rapidly.
There was a problem hiding this comment.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx:
- Around line 1034-1060: TimelineView declares and destructures setHoveredNodeId
in TimelineViewProps and the TimelineView parameter list but never uses it;
remove this unused prop to simplify the API by deleting setHoveredNodeId from
TimelineViewProps, removing it from the TimelineView function parameter
destructuring, and removing the argument passed from the parent component that
supplies TimelineView (where setHoveredNodeId is currently forwarded), ensuring
only handleHoverChange remains for hover handling.
- Around line 729-738: The handleScroll function currently takes an unused
parameter scrollTop; update the signature to either remove the parameter
entirely or rename it to _scrollTop to indicate it's intentionally unused
(change in the handleScroll definition where scrollTop is declared). No other
logic changes needed — keep the setIsScrolling, clearTimeout/scrollTimeoutRef,
pendingHoverIdRef, and setHoveredNodeId behavior intact.
🧹 Nitpick comments (2)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx: - Around line 1034-1060: TimelineView declares and destructures setHoveredNodeId in TimelineViewProps and the TimelineView parameter list but never uses it; remove this unused prop to simplify the API by deleting setHoveredNodeId from TimelineViewProps, removing it from the TimelineView function parameter destructuring, and removing the argument passed from the parent component that supplies TimelineView (where setHoveredNodeId is currently forwarded), ensuring only handleHoverChange remains for hover handling. - Around line 729-738: The handleScroll function currently takes an unused parameter scrollTop; update the signature to either remove the parameter entirely or rename it to _scrollTop to indicate it's intentionally unused (change in the handleScroll definition where scrollTop is declared). No other logic changes needed — keep the setIsScrolling, clearTimeout/scrollTimeoutRef, pendingHoverIdRef, and setHoveredNodeId behavior intact.apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx (2)
1034-1060:setHoveredNodeIdis passed but unused inTimelineView.
setHoveredNodeIdis declared inTimelineViewProps(line 1035), destructured (line 1058), and passed from the parent (line 966), but it is never referenced insideTimelineView. OnlyhandleHoverChangeis used for hover state transitions. Consider removing it to keep the component API minimal.Suggested fix
treeScrollRef: React.RefObject<HTMLDivElement>; hoveredNodeId: string | null; - setHoveredNodeId: (nodeId: string | null) => void; handleHoverChange: (nodeId: string | null) => void; handleScroll: (scrollTop: number) => void;And in the parent where props are passed:
hoveredNodeId={hoveredNodeId} - setHoveredNodeId={setHoveredNodeId} handleHoverChange={handleHoverChange} handleScroll={handleScroll}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx around lines 1034 - 1060, TimelineView declares and destructures setHoveredNodeId in TimelineViewProps and the TimelineView parameter list but never uses it; remove this unused prop to simplify the API by deleting setHoveredNodeId from TimelineViewProps, removing it from the TimelineView function parameter destructuring, and removing the argument passed from the parent component that supplies TimelineView (where setHoveredNodeId is currently forwarded), ensuring only handleHoverChange remains for hover handling.
729-738: UnusedscrollTopparameter.
scrollTopis accepted but never referenced in the function body. Remove it or prefix with_to signal intent.Suggested fix
- const handleScroll = useCallback((scrollTop: number) => { + const handleScroll = useCallback((_scrollTop: number) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/routes/_app.orgs`.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx around lines 729 - 738, The handleScroll function currently takes an unused parameter scrollTop; update the signature to either remove the parameter entirely or rename it to _scrollTop to indicate it's intentionally unused (change in the handleScroll definition where scrollTop is declared). No other logic changes needed — keep the setIsScrolling, clearTimeout/scrollTimeoutRef, pendingHoverIdRef, and setHoveredNodeId behavior intact.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx
🧰 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
**/*.{ts,tsx}: Always import tasks from@trigger.dev/sdk, never use@trigger.dev/sdk/v3or deprecatedclient.defineJobpattern
Every Trigger.dev task must be exported and have a uniqueidproperty with no timeouts in the run function
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.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/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Import from
@trigger.dev/coreusing subpaths only, never import from root
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.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/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.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 webappAccess environment variables via
envexport fromapps/webapp/app/env.server.ts, never useprocess.envdirectly
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx
🧠 Learnings (7)
📚 Learning: 2026-02-11T16:50:07.201Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3019
File: apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboards.$dashboardId.widgets.tsx:126-131
Timestamp: 2026-02-11T16:50:07.201Z
Learning: In apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboards.$dashboardId.widgets.tsx, MetricsDashboard entities are intentionally scoped to the organization level, not the project level. The dashboard lookup should filter by organizationId only (not projectId), allowing dashboards to be accessed across projects within the same organization. The optional projectId field on MetricsDashboard serves other purposes and should not be used as an authorization constraint.
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx
📚 Learning: 2025-12-08T15:19:56.823Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2760
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx:278-281
Timestamp: 2025-12-08T15:19:56.823Z
Learning: In apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx, the tableState search parameter uses intentional double-encoding: the parameter value contains a URL-encoded URLSearchParams string, so decodeURIComponent(value("tableState") ?? "") is required to fully decode it before parsing with new URLSearchParams(). This pattern allows bundling multiple filter/pagination params as a single search parameter.
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx
📚 Learning: 2026-02-03T18:27:40.429Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2994
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx:553-555
Timestamp: 2026-02-03T18:27:40.429Z
Learning: In apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx, the menu buttons (e.g., Edit with PencilSquareIcon) in the TableCellMenu are intentionally icon-only with no text labels as a compact UI pattern. This is a deliberate design choice for this route; preserve the icon-only behavior for consistency in this file.
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Attach metadata to task runs using the metadata option when triggering, and access/update it inside runs using metadata functions
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Export tasks with unique IDs within the project to enable proper task discovery and execution
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use `metadata.parent` and `metadata.root` to update parent and root task metadata from child tasks
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx
📚 Learning: 2026-02-11T16:37:32.429Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3019
File: apps/webapp/app/components/primitives/charts/Card.tsx:26-30
Timestamp: 2026-02-11T16:37:32.429Z
Learning: In projects using react-grid-layout, avoid relying on drag-handle class to imply draggability. Ensure drag-handle elements only affect dragging when the parent grid item is configured draggable in the layout; conditionally apply cursor styles based on the draggable prop. This improves correctness and accessibility.
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx
🧬 Code graph analysis (1)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx (1)
apps/webapp/app/utils/cn.ts (1)
cn(77-79)
⏰ 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). (26)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: sdk-compat / Node.js 20.20 (ubuntu-latest)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: sdk-compat / Deno Runtime
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: sdk-compat / Node.js 22.12 (ubuntu-latest)
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: sdk-compat / Bun Runtime
- GitHub Check: typecheck / typecheck
🔇 Additional comments (5)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx (5)
697-709: LGTM! State declarations and cleanup effect are well-structured. The cleanup properly clears any pending timeout on unmount to prevent state updates on an unmounted component.
722-727: LGTM! The callback correctly defers hover updates while scrolling and stores the pending value for later application.
854-873: LGTM! Hover detection and styling logic are correctly applied, andonMouseEnter/onMouseLeavehandlers are properly wired. The styling matches the corresponding timeline view logic for visual consistency.
1206-1225: LGTM! Hover logic in the timeline view mirrors the tree view, ensuring synchronized highlighting across both panels.
932-938: LGTM! Both scroll handlers properly integratehandleScrollalongside the existing scroll synchronization logic.Also applies to: 1335-1341
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const handleHoverChange = useCallback((nodeId: string | null) => { | ||
| pendingHoverIdRef.current = nodeId; | ||
| if (!isScrolling) { | ||
| setHoveredNodeId(nodeId); | ||
| } | ||
| }, [isScrolling]); |
There was a problem hiding this comment.
🚩 handleHoverChange recreated on every isScrolling toggle
handleHoverChange has [isScrolling] as its useCallback dependency (route.tsx:727). Every time scrolling starts or stops, isScrolling toggles, creating a new function reference. This new reference propagates to TimelineView via the handleHoverChange prop, causing TimelineView to re-render (and all its renderNode callbacks to receive the new function). For traces with many spans, this means two extra full re-renders per scroll gesture (one when isScrolling flips to true, one when it flips back). Consider using a ref for isScrolling instead of state to avoid this, since the scroll-suppression logic doesn't actually need to trigger re-renders — only the timeout callback's setHoveredNodeId(pendingHoverIdRef.current) needs to cause a render.
Was this helpful? React with 👍 or 👎 to provide feedback.
3 participants
Closes #
✅ Checklist
Testing
Checked in the interface
Changelog
Synchronized row between spans and timeline on hover
Screenshots