Skip to content

Conversation

@jog1t
Copy link
Contributor

@jog1t jog1t commented Jan 28, 2026

No description provided.

Copy link
Contributor Author

jog1t commented Jan 28, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link

claude bot commented Jan 28, 2026

PR Review: Traces Timeline UI

This PR adds a timeline visualization feature to the actor traces dashboard. Overall, the implementation is well-structured with good separation of concerns. Below are my findings:

🟢 Strengths

  1. Good Component Organization: The code is well-organized with clear separation between components (SpanSidebar, TimelineView, utility functions)
  2. Sophisticated Timeline Logic: The gap detection and compression algorithm is well-thought-out for handling sparse trace data
  3. User Experience: Good UX features like zoom controls, pan/drag, collapsible tree, and synchronized scrolling
  4. Type Safety: Proper TypeScript usage with well-defined interfaces

🟡 Code Quality Issues

Type Safety Problems

frontend/src/components/actors/traces/types.ts:10-21

  • SpanNode interface has several any types and inconsistencies:
    • endTimeUnixNano: any and startTimeUnixNano: any should be typed as bigint | undefined
    • name: ReactNode is overly permissive - should likely be string
    • spanId: string | null is inconsistent with usage (accessed as span.spanId without null checks)
    • Duplicate fields: both timeUnixNano and startTimeUnixNano/endTimeUnixNano; both startNs/endNs and the Unix nano fields

Recommendation: Clean up the SpanNode interface to remove duplicate fields and properly type all properties.

Unused Variables and Dead Code

frontend/src/components/actors/traces/utils.ts:32-59

  • buildSpanTree function references span.parentSpanId which doesn't exist in the SpanNode type
  • This function appears to be dead code since buildSpanTree is already implemented in actor-traces.tsx:488

frontend/src/components/actors/traces/span-sidebar.tsx:28-65

  • SpanSidebar component receives but doesn't use selectedSpanId, selectedEventIndex, onSelectSpan, and onSelectEvent props
  • These are passed through to children but the parent in actor-traces.tsx:266-267 passes null and throws errors for the callbacks

Non-Functional Code

frontend/src/components/actors/traces/actor-traces.tsx:272-280

onSelectSpan={(spanId: string | null): void => {
    throw new Error("Function not implemented.");
}}
onSelectEvent={(spanId: string, eventIndex: number): void => {
    throw new Error("Function not implemented.");
}}

The timeline view will crash if users try to select spans or events. This should either be:

  1. Implemented with actual state management
  2. Removed if not needed yet
  3. Made optional in the component props

Inconsistent Null Checks

frontend/src/components/actors/traces/utils.ts:5

return span.endNs === undefined;

But elsewhere in the codebase, endNs is typed as bigint | null, so this should check for both undefined and null:

return span.endNs === undefined || span.endNs === null;

frontend/src/components/actors/traces/traces-timeline.tsx:467-471
Uses span.endTimeUnixNano which is typed as any and not validated before use.

Potential Performance Issues

frontend/src/components/actors/traces/traces-timeline.tsx:320-323

  • mapTimeToPixel is recreated in useMemo but is also used as a dependency in other memos (line 356)
  • This could cause unnecessary recalculations

frontend/src/components/actors/actor-traces.tsx:35

  • Unused import: TracesTimeline is imported but never used

🔴 Critical Issues

1. Missing Null Safety

frontend/src/components/actors/actor-queue.tsx:21

refetchInterval: 1000,

This change adds aggressive 1-second polling which could cause performance issues. This appears unrelated to the traces timeline feature and should be in a separate PR.

2. Type Mismatches

The SpanNode type has overlapping fields that are used inconsistently:

  • startNs vs startTimeUnixNano
  • endNs vs endTimeUnixNano
  • Some code uses one, some uses the other

This will lead to runtime errors when data doesn't match expectations.

3. Incorrect Null Check Logic

frontend/src/components/actors/traces/span-sidebar.tsx:118

onClick={() => onSelectSpan(span.spanId)}

But spanId is typed as string | null, so this could pass null to a function expecting behavior for selection.

🔒 Security Concerns

No security issues identified. The code operates on client-side data visualization without user input validation concerns.

📊 Performance Considerations

  1. Excessive Re-renders: The timeline view has many useMemo hooks that depend on each other, which could cause cascading recalculations
  2. Large Dataset Handling: With the 1000 span limit, performance should be acceptable, but consider virtualization for very large trace sets
  3. Polling Interval: The 1-second refetch in both traces and queue could be resource-intensive in live mode

✅ Recommendations

  1. High Priority:

    • Fix the non-functional onSelectSpan/onSelectEvent callbacks
    • Clean up the SpanNode type definition to remove duplicates and any types
    • Remove dead code (buildSpanTree in utils.ts)
    • Move the queue polling change to a separate PR
  2. Medium Priority:

    • Add proper null/undefined checks for endNs throughout
    • Fix the inconsistent usage of timestamp fields
    • Consider making selection features optional or fully implement them
  3. Nice to Have:

    • Add error boundaries around the timeline component
    • Consider memoization strategy to prevent unnecessary recalculations
    • Add loading states for timeline calculations on large datasets

📝 Minor Issues

  • frontend/src/components/ui/collapsible.tsx: Good addition, follows existing patterns
  • frontend/src/components/actors/traces/span-sidebar.tsx:38: Redundant border-border class (border color is already border by default)
  • Line 35 in actor-traces.tsx: Remove unused TracesTimeline import

Overall Assessment: The feature implementation shows solid architecture and good UX thinking, but needs refinement on type safety and completion of selection functionality before merging. The unrelated queue polling change should be separated out.

@jog1t jog1t force-pushed the 01-27-refactor_rivetkit_make_traces_server_only_lib branch from 63d40ba to d77a4fc Compare January 30, 2026 21:29
@jog1t jog1t force-pushed the 01-28-feat_dashboard_traces_timeline_ui branch from 2a858f0 to d559f28 Compare January 30, 2026 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants