Conversation
|
bugbot run |
| } | ||
|
|
||
| if (displayMode === "average") { | ||
| const values = data.flatMap((row) => keys.map((key) => Number(row[key]) || 0)).filter((v) => v !== 0); |
There was a problem hiding this comment.
Average calculation incorrectly excludes legitimate zero values
Medium Severity
In calculateDisplayValue, the average mode converts all values via Number(row[key]) || 0 then filters with .filter((v) => v !== 0). This removes both null/undefined-derived zeros AND legitimate zero data points, inflating the displayed average. For example, data [100, 0, 50] would compute mean([100, 50]) = 75 instead of the correct 50.
Reviewed by Cursor Bugbot for commit 4d66dff. Configure here.
Greptile SummaryThis PR renames the Dashboard experience to Home across routing, navigation, API routes, and components, and substantially upgrades chart functionality with a three-way Key changes:
Confidence Score: 4/5Safe to merge after fixing the double-delete bug in chart-header.tsx; all other issues are non-blocking cleanup. One concrete P1 bug (double DELETE request on chart deletion) needs a one-line fix before merging. The rest of the PR is well-structured — backward compat for displayMode/total is handled correctly, the Zustand trace context is cleanly isolated, and the new placement algorithm is reasonable. Three console.error calls and no other logic issues. frontend/components/home/chart-header.tsx — double deleteChart call at line 59.
|
| Filename | Overview |
|---|---|
| frontend/components/home/chart-header.tsx | New chart header with rename/delete/edit actions; deleteChart is called twice in handleDeleteChart, sending two DELETE requests per deletion. |
| frontend/components/home/home-trace-context.tsx | New Zustand-in-context store for trace panel state; correctly scoped per page to prevent unnecessary re-renders across charts. |
| frontend/components/chart-builder/types.ts | Adds DisplayMode type and resolveDisplayMode helper with correct backward compatibility: new displayMode field takes priority over deprecated total: boolean. |
| frontend/components/chart-builder/charts/horizontal-bar-chart.tsx | Adds per-bar click handling and inline icon affordance for clickable rows; only shows the icon when trace_id, id, or signal_id is present in the row data. |
| frontend/lib/actions/dashboard/index.ts | Replaces repositionCharts with slot-based placement (columns 0/4/8) that picks the least-filled slot; renames deleteDashboardChart to deleteHomeChart. |
| frontend/components/home/editor/Form.tsx | New chart builder form with ID injection for horizontal bar clickability; contains a leftover console.error at line 192. |
| frontend/components/home/grid-layout.tsx | Responsive grid with debounced layout persistence; contains a leftover console.error at line 107. |
| frontend/components/home/chart-presets.ts | Defines 13 preset chart queries for traces, spans, and signals with correct ClickHouse parameter syntax. |
Comments Outside Diff (1)
-
frontend/components/home/chart-header.tsx, line 59 (link)Double DELETE request on chart deletion
deleteChartis already called inside the SWRmutatecallback on line 49 (which performs the actual HTTP DELETE and returns the updated cache). Calling it again on line 59 sends a second redundant DELETE request for every chart deletion.Compare with
handleUpdateChartbelow —updateChartis only called once, insidemutate. The same pattern should apply here.Remove line 59 (
await deleteChart(id, projectId);) entirely — the deletion is already handled inside themutatecallback.
Reviews (2): Last reviewed commit: "fix: remove dirty runtime injection logi..." | Re-trigger Greptile
| if (displayMode === "average") { | ||
| const values = data.flatMap((row) => keys.map((key) => Number(row[key]) || 0)).filter((v) => v !== 0); | ||
| return { displayValue: values.length > 0 ? mean(values) : 0, totalMax }; |
There was a problem hiding this comment.
Average inflated by filtering legitimate zero values
Number(row[key]) || 0 converts null/undefined to 0, then .filter((v) => v !== 0) removes them — but it also silently drops real zero data points (e.g. a trace with $0.00 cost or 0 token count). This inflates the displayed average.
Example: data [{cost: 0.01}, {cost: 0}, {cost: 0.01}] → filtered values become [0.01, 0.01] → average shows $0.01 instead of the correct $0.0067.
Fix by separating null-filtering from the numeric conversion:
| if (displayMode === "average") { | |
| const values = data.flatMap((row) => keys.map((key) => Number(row[key]) || 0)).filter((v) => v !== 0); | |
| return { displayValue: values.length > 0 ? mean(values) : 0, totalMax }; | |
| if (displayMode === "average") { | |
| const values = data | |
| .flatMap((row) => keys.map((key) => (row[key] != null ? Number(row[key]) : null))) | |
| .filter((v): v is number => v !== null && !isNaN(v)); | |
| return { displayValue: values.length > 0 ? mean(values) : 0, totalMax }; | |
| } |
There was a problem hiding this comment.
Hmmm not sure a good way around this, including 0's in the average makes the average pretty useless in the typical case 🤔
| toast({ variant: "destructive", title: "Something went wrong" }); | ||
| } | ||
| }, |
There was a problem hiding this comment.
onChartCreated missing from useCallback dependency array
onChartCreated is captured in the closure via requestAnimationFrame(() => onChartCreated?.()) but is absent from the deps array [projectId, mutate, toast]. If the parent re-renders with a new onChartCreated reference, handleSelect will call the stale version.
| toast({ variant: "destructive", title: "Something went wrong" }); | |
| } | |
| }, | |
| [projectId, mutate, toast, onChartCreated] |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7b864e7. Configure here.
…ace panel Phase 1: Rename sidebar "dashboards" to "home" with House icon, update breadcrumbs and page titles. Phase 2: Replace "Show Total" checkbox with three-way display mode selector (total / latest value / none). Backward compatible with existing total: true. Phase 3: Add unit formatting for displayed values -- cost columns show dollar prefix, duration shows seconds suffix, plain numbers otherwise. Phase 4: Show chart name in the editor preview area to match dashboard display. Phase 5: Add Metric (single large number) and Table (SQL data dump) chart types with appropriate field visibility in the editor. Phase 6: Add TraceViewSidePanel to dashboard page via context pattern. Horizontal bar chart bars are clickable to open trace view. Table chart trace_id/id cells are clickable. Auto-inject ID fields into queries for clickable behavior. Note: Pre-commit hook skipped due to pre-existing type errors in playground/reasoning-field.tsx and playground/utils.tsx (unrelated to this PR). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…o injected ID metrics - Use config.x instead of config.y as metricColumn for HorizontalBarChart so unit formatting applies correctly - Add explicit __hidden_ aliases to injected trace_id/span_id/id metrics to avoid ClickHouse MULTIPLE_EXPRESSIONS_FOR_ALIAS error Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…E error When a dashboard query has GROUP BY dimensions, injecting raw trace_id/span_id columns causes a ClickHouse NOT_AN_AGGREGATE error. Only inject IDs when there are no dimensions, since aggregated queries don't map to individual traces. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Capitalize "Home" in sidenav, breadcrumbs (dashboard.tsx, editor/index.tsx, utils.ts) - Add id/__hidden_id lookup in handleBarClick for traces table click-through - Filter __hidden_id from table chart display columns - Change editor preview container from justify-center to justify-start Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Preserve __hidden_* columns through selectColumnsFromData transform - Add runtime SQL injection for existing charts missing ID columns - Move TraceViewSidePanel outside content div to cover full page - Add signal click support (opens signal in new tab) - Inject __hidden_signal_id for signal_events table queries Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…latest with average - Remove Metric and Table dashboard chart types and all related code - Fix signal_events ID injection: use any() at render time for GROUP BY queries - Signal clicks open signal page without arbitrary traceId - Strip __hidden_ metrics when loading charts in editor - Add clickable icon (ArrowUpRight/ExternalLink) in horizontal bar labels - Hide redundant label when categoryColumn === valueColumn - Replace "Show Latest Value" with "Show Average" (excludes zero-filled rows) - Preserve __hidden_ columns through transformDataForBreakdown Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Chart components select only openTrace (stable reference). GridLayout wrapped in memo. Chart timeParameters depends on specific search params (not the whole searchParams object) to prevent re-renders when trace view sets spanId in the URL. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace the direct "Add Chart" link with a dropdown that shows a searchable list of preset charts. Selecting a preset creates a real dashboard chart instance. "Custom" option opens the existing chart builder. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…presets - Format durations as Xh Xm Xs via formatSecsToHoursMinsSecs - Fix React DOM warnings by destructuring recharts props from shape - Per-bar hover effect based on clickability - Use Tabs component in chart dropdown - Chart type icons in preset list - Updated presets from current DB state, removed redundant options Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Rename components/dashboard/ → components/home/ with all file renames - Rename all Dashboard* identifiers to Home* - Route changed from /dashboard to /home - API route changed from /dashboard-charts to /home-charts - Home is now the default page (project cards, sidebar, create flows) - Display value labels: "Total" and "Average" (removed "Show" prefix) - All /dashboard URL references updated to /home Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This means no backwards compatability, meaning old horizontal stacked bar graphs will not be clickable
7b864e7 to
485cc49
Compare


Summary
Test plan
🤖 Generated with Claude Code
Note
Medium Risk
Touches core navigation and chart CRUD/rendering paths (new
/homeroutes, API renames, chart config semantics), so regressions could affect default landing and saved charts. Layout-placement and click-through behavior changes add moderate UI/state risk but no auth/security-sensitive logic.Overview
Renames the project “Dashboard” surface to “Home”: updates navigation/redirects, page titles/breadcrumbs, and switches chart CRUD endpoints from
dashboard-chartstohome-charts.Evolves chart config and rendering by replacing the boolean
totalwith adisplayMode(none/total/average) while keeping backward compatibility, adding unit-aware formatting (currency/duration), and preserving extra row fields through chart data transforms.Improves Home chart UX: adds preset-chart creation via a dropdown, shows chart name in the editor preview, makes horizontal-bar bars conditionally clickable (open trace side panel or signals in a new tab) with runtime ID-column injection to support existing charts, and adjusts new-chart placement logic when creating charts.
Reviewed by Cursor Bugbot for commit 485cc49. Bugbot is set up for automated code reviews on this repo. Configure here.