-
Notifications
You must be signed in to change notification settings - Fork 150
feat(dashboard): traces timeline ui #4056
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 01-27-refactor_rivetkit_make_traces_server_only_lib
Are you sure you want to change the base?
feat(dashboard): traces timeline ui #4056
Conversation
|
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.
How to use the Graphite Merge QueueAdd 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. |
PR Review: Traces Timeline UIThis 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
🟡 Code Quality IssuesType Safety Problemsfrontend/src/components/actors/traces/types.ts:10-21
Recommendation: Clean up the Unused Variables and Dead Codefrontend/src/components/actors/traces/utils.ts:32-59
frontend/src/components/actors/traces/span-sidebar.tsx:28-65
Non-Functional Codefrontend/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:
Inconsistent Null Checksfrontend/src/components/actors/traces/utils.ts:5 return span.endNs === undefined;But elsewhere in the codebase, return span.endNs === undefined || span.endNs === null;frontend/src/components/actors/traces/traces-timeline.tsx:467-471 Potential Performance Issuesfrontend/src/components/actors/traces/traces-timeline.tsx:320-323
frontend/src/components/actors/actor-traces.tsx:35
🔴 Critical Issues1. Missing Null Safetyfrontend/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 MismatchesThe
This will lead to runtime errors when data doesn't match expectations. 3. Incorrect Null Check Logicfrontend/src/components/actors/traces/span-sidebar.tsx:118 onClick={() => onSelectSpan(span.spanId)}But 🔒 Security ConcernsNo security issues identified. The code operates on client-side data visualization without user input validation concerns. 📊 Performance Considerations
✅ Recommendations
📝 Minor Issues
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. |
63d40ba to
d77a4fc
Compare
2a858f0 to
d559f28
Compare

No description provided.