Compare radar chart: relative/full scale toggle, removable usage axes, no false zeros - #1628
Compare radar chart: relative/full scale toggle, removable usage axes, no false zeros#1628tawnymanticore wants to merge 4 commits into
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe comparison page adds hidden usage-metric filtering and score-axis metadata. The radar chart supports relative/full-scale modes, shared-score filtering, usage axes, enhanced tooltips and legends, and distinct empty states. ChangesComparison and radar chart flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ComparePage
participant URL
participant CompareRadarChart
participant ECharts
participant ChartNoData
ComparePage->>URL: read and persist hidden usage metrics
ComparePage->>CompareRadarChart: pass visible features and scoreAxisMaxes
CompareRadarChart->>CompareRadarChart: generate shared scaled radar data
CompareRadarChart->>ECharts: render chart, legends, and tooltips
CompareRadarChart->>ChartNoData: render contextual empty state when needed
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
📊 Coverage ReportOverall Coverage: 92% Diff: origin/main...HEADNo lines with coverage information in this diff.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/web_ui/src/lib/components/compare_radar_chart.svelte (2)
472-482: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
scoreAxisMaxes &&doesn't gate on readiness.An empty
{}is truthy, so this only serves as a reactive dependency, not a "maxes are loaded" guard. Effect: in Full Scale mode the chart can draw with 0–1 fallback axes whileeval_data_cacheis still populating, then rescale. Self-recovering, but if the intent was to wait, gate onObject.keys(scoreAxisMaxes).length > 0(or drop the misleading comment).🤖 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 `@app/web_ui/src/lib/components/compare_radar_chart.svelte` around lines 472 - 482, Update the reactive updateChart guard so scoreAxisMaxes is considered ready only when it contains at least one key, using Object.keys(scoreAxisMaxes).length > 0. Preserve scoreAxisMaxes as a reactive dependency and keep the other readiness checks unchanged.
251-354: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive chart data once instead of recomputing it in two places.
generateChartData()is invoked from both thechartSummaryreactive block andupdateChart(), duplicating the full O(keys × configs) walk (eachgetModelValueRawalso does a.findover eval results) and leaving two independent computations that must stay in sync. A single reactive$: chartData = generateChartData()consumed by both keeps them consistent and halves the work.Also note the max loop at Line 312 iterates
selectedRunConfigIdsrather thanplottedConfigs; harmless today (excluded configs are all-null by construction) but easy to drift — iteratingplottedConfigsmatches the series being drawn.♻️ Sketch
+ $: chartData = + dataKeys && selectedRunConfigIds && axisScaleMode && scoreAxisMaxes + ? generateChartData() + : null $: chartSummary = (() => { - if (!dataKeys || dataKeys.length === 0 || !selectedRunConfigIds) { + if (!chartData) { return { hasData: false, noResultKeyCount: 0 } } - const { indicators, series, noResultKeyCount } = generateChartData() + const { indicators, series, noResultKeyCount } = chartData return { hasData: indicators.length > 0 && series.length > 0, noResultKeyCount, } })()and in
updateChart():- const { indicators, series, legend, axisMaxes, keys } = generateChartData() + if (!chartData) return + const { indicators, series, legend, axisMaxes, keys } = chartData🤖 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 `@app/web_ui/src/lib/components/compare_radar_chart.svelte` around lines 251 - 354, Derive the result of generateChartData() once in a reactive chartData assignment, then have both chartSummary and updateChart consume that shared object instead of invoking generateChartData independently. In generateChartData, update the axis maximum loop to iterate plottedConfigs rather than selectedRunConfigIds, keeping max calculations aligned with the series that are rendered.
🤖 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.
Inline comments:
In `@app/web_ui/src/lib/components/compare_radar_chart.svelte`:
- Around line 203-231: Escape all user- or project-authored strings before
interpolating them into the ECharts tooltip HTML. Add or reuse a shared
escapeHtml helper in the tooltip-building logic, applying it to score.label from
getKeyLabel(key) and the model/prompt names interpolated in the surrounding
HTML; preserve numeric formatting and existing tooltip behavior.
---
Nitpick comments:
In `@app/web_ui/src/lib/components/compare_radar_chart.svelte`:
- Around line 472-482: Update the reactive updateChart guard so scoreAxisMaxes
is considered ready only when it contains at least one key, using
Object.keys(scoreAxisMaxes).length > 0. Preserve scoreAxisMaxes as a reactive
dependency and keep the other readiness checks unchanged.
- Around line 251-354: Derive the result of generateChartData() once in a
reactive chartData assignment, then have both chartSummary and updateChart
consume that shared object instead of invoking generateChartData independently.
In generateChartData, update the axis maximum loop to iterate plottedConfigs
rather than selectedRunConfigIds, keeping max calculations aligned with the
series that are rendered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 84913d9d-3997-418d-ac9b-60bc9bb5ef7a
📒 Files selected for processing (3)
app/web_ui/src/lib/components/chart_no_data.svelteapp/web_ui/src/lib/components/compare_radar_chart.svelteapp/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/compare/+page.svelte
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/web_ui/src/lib/components/compare_radar_chart.svelte (2)
604-618: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
run_configsis missing from the reactive dependency list.Series names, legend entries and
plottedConfigsall derive fromrun_configs, but it isn't referenced in this guard, so a later-arrivingrun_configsupdate won't redraw unlesscomparisonFeatureshappens to change at the same time.♻️ Add the dependency
$: if ( chartInstance && comparisonFeatures && selectedRunConfigIds && + run_configs && axisScaleMode &&🤖 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 `@app/web_ui/src/lib/components/compare_radar_chart.svelte` around lines 604 - 618, Add run_configs to the reactive guard surrounding updateChart() in the chart component, alongside the other inputs that control rendering. Ensure the guard waits for run_configs to be available while preserving support for its explicit null state, so later run_configs updates trigger a redraw and refresh series names, legends, and plottedConfigs.
386-402: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAlign the radar-axis threshold with the final axis set.
generateChartData()counts usage axes inkeys, but the chart is only rendered through{#ifdataKeys.length >= MIN_RADAR_AXES}. That can hide valid charts with enough shared eval+usage axes; use the same axis set for both gates (ideallykeys.lengthwith the chart container guard).🤖 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 `@app/web_ui/src/lib/components/compare_radar_chart.svelte` around lines 386 - 402, Align the chart-rendering guard with the final filtered axis set produced by generateChartData(): use keys.length, or the returned equivalent, for the MIN_RADAR_AXES check instead of dataKeys.length. Ensure charts with enough shared evaluation and usage axes render consistently with the existing generateChartData threshold.
🧹 Nitpick comments (1)
app/web_ui/src/lib/components/compare_radar_chart.svelte (1)
470-505: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
generateChartData()runs twice per update.
chartSummarycomputes it reactively andupdateChart()recomputes it immediately after. Each pass doeskeys × configsgetModelValueRawcalls, and each of those does a linearfindovereval_results. Storing the full result onchartSummaryand havingupdateChart()consume it removes the duplicate work and guarantees both paths agree on the same snapshot.🤖 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 `@app/web_ui/src/lib/components/compare_radar_chart.svelte` around lines 470 - 505, Store the full generateChartData() result in the reactive chartSummary computation, including indicators, series, legend, axisMaxes, and keys, while preserving its existing summary fields. Update updateChart() to consume that stored result instead of calling generateChartData() again, ensuring the empty-data path still clears the chart and both paths use the same snapshot.
🤖 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.
Inline comments:
In
`@app/web_ui/src/routes/`(app)/specs/[project_id]/[task_id]/compare/+page.svelte:
- Around line 460-486: The hidden-evals dropdown styling relies on positional
selectors that incorrectly prefix the inserted “Show Metric” header and misalign
dividers. Update the `hiddenEvalsMenuItems`/`FloatingMenu` item rendering and
the `.hidden-evals-dropdown` styles to identify headers or divider items by an
explicit type/class instead of `:nth-child` positions, preserving the intended
“+” prefix only for applicable menu entries.
- Around line 376-412: Update filterVisibleFeatures so the kiln_cost_section is
removed when hiddenUsageKeys filters out all of its items, preventing the empty
cost section from reaching the generic no-scores error state. Preserve the
existing behavior for partially hidden cost sections, hidden evaluation
sections, and unchanged feature lists.
---
Outside diff comments:
In `@app/web_ui/src/lib/components/compare_radar_chart.svelte`:
- Around line 604-618: Add run_configs to the reactive guard surrounding
updateChart() in the chart component, alongside the other inputs that control
rendering. Ensure the guard waits for run_configs to be available while
preserving support for its explicit null state, so later run_configs updates
trigger a redraw and refresh series names, legends, and plottedConfigs.
- Around line 386-402: Align the chart-rendering guard with the final filtered
axis set produced by generateChartData(): use keys.length, or the returned
equivalent, for the MIN_RADAR_AXES check instead of dataKeys.length. Ensure
charts with enough shared evaluation and usage axes render consistently with the
existing generateChartData threshold.
---
Nitpick comments:
In `@app/web_ui/src/lib/components/compare_radar_chart.svelte`:
- Around line 470-505: Store the full generateChartData() result in the reactive
chartSummary computation, including indicators, series, legend, axisMaxes, and
keys, while preserving its existing summary fields. Update updateChart() to
consume that stored result instead of calling generateChartData() again,
ensuring the empty-data path still clears the chart and both paths use the same
snapshot.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 40678c33-68f3-42e7-9ef0-4c9048020471
📒 Files selected for processing (2)
app/web_ui/src/lib/components/compare_radar_chart.svelteapp/web_ui/src/routes/(app)/specs/[project_id]/[task_id]/compare/+page.svelte
6d4e706 to
619013f
Compare
tawnymanticore
left a comment
tawnymanticore
left a comment
There was a problem hiding this comment.
A few things I hit reading this through.
| section.eval_id === "kiln_cost_section" && hiddenUsage.length > 0 | ||
| ? { | ||
| ...section, | ||
| items: section.items.filter( |
There was a problem hiding this comment.
Hiding all five cost rows leaves this section with items: []. The table's {#if section.items.length == 0} branch then falls through to Unknown issue - no scores found, because the cost section's has_default_eval_config is undefined rather than false. Easy to hit now that each row has its own ✕.
There was a problem hiding this comment.
Fixed in 0de4b1a0e: the empty-section branch is now skipped for kiln_cost_section, so an emptied cost section renders as just its header.
Every radar axis was scaled to the highest value observed across the selected run configs. With a single run config selected that config is the max on every axis, so it sat on the outer ring everywhere and the chart carried no information at all. "Full Scale" scales each axis to the score's own range instead: 1 for pass/fail and pass/fail/critical, 5 for 5-star, taken from the eval definitions the compare page already caches. Scores with no known range (unbounded custom scores) and any data that exceeds its expected range keep the relative max, so nothing is clipped. The chart defaults to Full Scale when only one run config has results, since there is nothing to compare against; an explicit click on either mode wins from then on. Also gives the plot room to be read at that scale. For one or two run configs the radar is centered with the legend underneath, instead of always reserving the right 40% of the card for a legend column, and the container is taller. Long axis names wrap rather than colliding with their neighbours, a single series is drawn filled (one polygon reads far better filled, several read as mud), and the tooltip is confined to the chart instead of covering the table above. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cost and latency were already radar axes; the three token counts were not on the chart at all. All five are now axes, scored by metricToScore as a position within the selected run configs on a shared 0-100 axis. They are named for the direction that's better - Cost Efficiency, Speed, Token Efficiency, Input Token Efficiency, Output Token Efficiency - since a bigger value means less cost, less time, fewer tokens. Because they score a config against the others rather than against a fixed range, they stay relative in Full Scale mode too, which the scale tooltip explains. Input and output tokens sum to total, so drawing all three at once repeats the same spoke three times. Rather than invent a control for that, each usage axis is dropped with the same x the eval sections already have, moved onto the rows of the cost section where the numbers are. Hidden rows are tracked in the URL as hidden_usage and restorable from the "Hidden (n)" dropdown under a "Show Metric" group, alongside hidden evals. The chart's usage axes are just whatever cost rows survive, so hiding a row removes its axis. An emptied cost section renders as just its header: the table's empty-section branch would otherwise fall through to "Unknown issue - no scores found", since the cost section's has_default_eval_config is undefined rather than false. Splitting the usage keys out of dataKeys also means the card's "at least three axes" gate has to count them, or a task with one or two eval scores would hide a chart that has six or seven axes to draw. The tooltip keeps the raw mean cost, latency and total tokens, since the axes plot a relative score rather than the quantity itself - and for the same reason the usage axes are left out of the tooltip's weakest-scores list, where a 0-100 relative score was being compared against pass rates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A run config with no result for a score was plotted at 0, which on the radar is indistinguishable from actually scoring 0 - the shape said "failed everything" while the tooltip said "N/A". On one of my run configs, 26 of 28 scores are null and none are genuinely zero, and the chart drew all 26 at the center. Plotting null instead doesn't help: echarts maps a missing radar value to the center of the chart by design (radarLayout's getValueMissingPoint), so there's no way to draw a gap. The chart now plots only the scores that every selected run config has a result for, and reports the count of the rest under the title. A run config with no results at all is left out rather than emptying every axis, and axis maxima are taken over those plotted configs rather than every selected one. Below three shared scores there's no shape to read, so ChartNoData takes a title and message and says which of the two reasons it is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
updateURL() records existing UI state in the query string, but goto() scrolls to the top by default, so hiding a row, hiding an eval, adding a column or picking a run config all threw the page back to the top - and hiding a metric near the bottom of a long table meant scrolling all the way back down to hide the next one. It also dropped focus from the button that was just clicked. noScroll and keepFocus. Measured on the real page: hiding a row from a scroll position of 3219px left it at 3219px, where it previously went to 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
619013f to
73c2dd6
Compare
tawnymanticore
changed the title
|
We need cost and speed on the chart. It's essential to every pitch/demo we have done. Up for improving it, but the values have meaning even if relative |
@scosman Right I agree, this PR just unifies all eval rows to make any X'able in the Compare Run Config screen (today all are X'able except for speed/tokens/latency)... the user may want to make the radar chart demo-able and may need to modify any of it. Also if there is just 1 run config shown on the radar chart, then speed/cost/tokens don't make any sense on the Radar Chart because it is hard coded to be at 50 out of 100, its a relative measure so requires >1 run config to make any sense since we don't have an upper bound on Low Cost or High Cost, or Low Latency or High Latency etc unlike the rest of the radar chart axes which are bounded by the eval scores (fail --> pass, 0-5 stars) |
2 participants
What does this PR do?
Makes the compare screen's radar chart readable with a small number of run configs. Before: every axis was scaled to the max across the selected configs, so a single run config sat on the outer ring of every axis and the chart said nothing — and a score it had no result for was plotted at 0, indistinguishable on a radar from scoring 0.
radarLayout'sgetValueMissingPoint), so a gap can't be drawn. The chart now plots only the scores every selected run config has a result for, and reports the count of the rest under the title.✕the eval sections already have, on its row in the cost table — tracked in the URL ashidden_usage, restorable from the "Hidden (n)" dropdown.updateURL()now passesnoScroll/keepFocustogoto(). Every state change on the compare page used to jump back to the top and drop focus. Not specific to the chart.Not included: skipping scores whose direction is
lower_is_betterorinformational, since a radar reads "further out is better". That needsEvalOutputScore.direction, which isn't on main yet.Related Issues
None — this wasn't tracked in an issue.
Contributor License Agreement
I, @, confirm that I have read and agree to the Contributors License Agreement.
Checklists
Web UI only, so the
/libitem doesn't apply. Fromapp/web_ui:npm run check(0 errors),npm run lint,npm run format_checkandnpx vitest run(1163 tests) all pass.🤖 Generated with Claude Code