feat: better filters - #504
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
skill-check — worker0 verified, 42 skipped (no docs/).
Four for four. Nicely done. |
📝 WalkthroughWalkthroughTraces V2 now shares span-hidden logic between timeline rendering and trace-list filtering. A new hook derives visibility verdicts from feed and fetched spans, while pagination, statistics, and empty states use the filtered trace set. ChangesSpan filter visibility
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TracesV2
participant useSpanFilteredTraceRows
participant reconcileTraceVisibility
participant mergeFetchedVerdicts
TracesV2->>useSpanFilteredTraceRows: rows, feed spans, selection
useSpanFilteredTraceRows->>reconcileTraceVisibility: reconcile feed-based verdicts
useSpanFilteredTraceRows->>mergeFetchedVerdicts: merge fetched span verdicts
mergeFetchedVerdicts-->>useSpanFilteredTraceRows: updated verdict map
useSpanFilteredTraceRows-->>TracesV2: visible trace rows
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
console/web/src/pages/TracesV2/hooks/useSpanFilteredTraceRows.ts (1)
145-154: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffDuplicate span→bar mapping on the live-streaming hot path.
verdictBarsre-runsstoredSpansToTimelineSpans(feedSpans)independently ofTimelineStrip's own equivalent mapping over the sameStoredSpan[](TimelineStrip.tsx'sallSpansmemo). SincefeedSpans/allSpanschanges frequently while streaming, this doubles the O(n) span-mapping cost every time new spans arrive. Consider lifting the sharedTimelineSpan[]mapping up toindex.tsxand passing it to bothTimelineStripand this hook (filtering builtins from the already-computed bars) instead of recomputing from raw spans twice.Also applies to: 184-193
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@console/web/src/pages/TracesV2/hooks/useSpanFilteredTraceRows.ts` around lines 145 - 154, Eliminate the duplicate span-to-bar conversion between verdictBars and TimelineStrip by lifting the storedSpansToTimelineSpans mapping into the shared index.tsx flow. Pass the computed TimelineSpan[] to both TimelineStrip and useSpanFilteredTraceRows, and update verdictBars to filter engine builtins from those precomputed bars rather than remapping raw StoredSpan[].
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@console/web/src/pages/TracesV2/hooks/useSpanFilteredTraceRows.ts`:
- Around line 145-154: Eliminate the duplicate span-to-bar conversion between
verdictBars and TimelineStrip by lifting the storedSpansToTimelineSpans mapping
into the shared index.tsx flow. Pass the computed TimelineSpan[] to both
TimelineStrip and useSpanFilteredTraceRows, and update verdictBars to filter
engine builtins from those precomputed bars rather than remapping raw
StoredSpan[].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a7f0dfcd-296b-477b-ba9a-f7eba82587b8
📒 Files selected for processing (6)
console/web/src/pages/TracesV2/components/timeline/TimelineStrip.tsxconsole/web/src/pages/TracesV2/components/timeline/spanVisibility.tsconsole/web/src/pages/TracesV2/hooks/useSpanFilteredTraceRows.test.tsconsole/web/src/pages/TracesV2/hooks/useSpanFilteredTraceRows.tsconsole/web/src/pages/TracesV2/index.tsxconsole/web/src/pages/TracesV2/lib/timelineSpans.ts
Summary by CodeRabbit
New Features
Bug Fixes
Tests