(MOT-4338) fix(dashboard): make efficiency cards read one coherent population - #689
Conversation
The sparklines summed raw per-scenario averages across every reported scenario, so a missing report or a newly added scenario moved the line for structural reasons while the delta chip on the same card honestly compared only the comparable cohort. Sum the same cohort the chip uses and skip executions that lack any cohort contract instead of fabricating a dip.
The stat tiles mixed three quantities: the headline summed every reported scenario, the delta chip compared only the comparable cohort, and the sparkline plotted raw sums, so the three elements of one card could contradict each other. Value, delta, and trend now all read the comparable cohort; the sparkline gains a dashed baseline-median reference and per-point hover values, the delta names its baseline, and the section copy states the population once.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
skill-check — worker0 verified, 52 skipped (no docs/).
Four for four. Nicely done. |
|
Warning Review limit reached
Next review available in: 41 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe benchmark site now calculates efficiency values and trends from unchanged scenarios with passing latest runs. It adds cohort-filtered metric sparklines, baseline guides, execution tooltips, and updated accessibility and metric labels. ChangesComparable efficiency reporting
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant EfficiencyCards
participant cohortMetricSparkline
participant Executions
participant Sparkline
EfficiencyCards->>cohortMetricSparkline: Request comparable metric points
cohortMetricSparkline->>Executions: Match complete cohort executions
Executions-->>cohortMetricSparkline: Return metric values
cohortMetricSparkline-->>EfficiencyCards: Return chronological points
EfficiencyCards->>Sparkline: Render points and baseline
Sparkline-->>EfficiencyCards: Show execution tooltips
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/benchmark-site/execution-data.test.cjs (1)
418-448: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a multi-row cohort fixture.
This test uses one cohort row. It cannot detect a failure to sum multiple cohort contracts. It also cannot detect an execution that has one cohort contract but misses another required contract.
Add two cohort rows. Verify the combined total. Add one execution that lacks the second contract and verify that the helper excludes it.
🤖 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 @.github/benchmark-site/execution-data.test.cjs around lines 418 - 448, Update the cohortMetricSparkline test to define two cohort rows with distinct contract fingerprints, include matching metrics for both, and assert their combined value per execution. Add an execution containing only one required cohort contract and verify it is excluded, while retaining coverage for partial metrics and unrelated contracts.
🤖 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 @.github/benchmark-site/execution-data.js:
- Around line 900-923: Update cohortMetricSparkline to sort executions by
completed_at, falling back to started_at, in descending chronological order
before the limit-controlled loop; preserve the existing complete-metric
filtering and reverse the selected points only after selection for rendering.
Add coverage for executions whose array order differs from timestamp order.
In @.github/benchmark-site/overview.js:
- Around line 466-478: Update the card rendering flow around
renderEfficiencySparkline so cards using metric.operational when cohortRows is
empty receive a visible and accessible full-suite fallback label, while
comparable-cohort cards retain their existing wording. In
.github/benchmark-site/index.html lines 67-70, revise the section description to
cover both populations; in lines 80-80, replace or dynamically update the static
accessible label so it remains accurate during fallback.
---
Nitpick comments:
In @.github/benchmark-site/execution-data.test.cjs:
- Around line 418-448: Update the cohortMetricSparkline test to define two
cohort rows with distinct contract fingerprints, include matching metrics for
both, and assert their combined value per execution. Add an execution containing
only one required cohort contract and verify it is excluded, while retaining
coverage for partial metrics and unrelated contracts.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 21f774e5-e929-4eb4-b842-f883356c6175
📒 Files selected for processing (5)
.github/benchmark-site/execution-data.js.github/benchmark-site/execution-data.test.cjs.github/benchmark-site/index.html.github/benchmark-site/overview.js.github/benchmark-site/styles.css
| for (const execution of executions || []) { | ||
| if (points.length >= limit) break; | ||
| const metrics = execution?.scenario_metrics || []; | ||
| if (!metrics.length) continue; | ||
| let total = 0; | ||
| let complete = true; | ||
| for (const item of basket) { | ||
| const scenario = metrics.find( | ||
| (candidate) => | ||
| `${candidate?.subject_id || ""}::${candidate?.scenario_id || ""}` === | ||
| item.key && | ||
| (candidate?.contract_fingerprint || "") === item.fingerprint, | ||
| ); | ||
| const value = numberOrNull(scenario?.averages?.[metricId]); | ||
| if (value === null) { | ||
| complete = false; | ||
| break; | ||
| } | ||
| total += value; | ||
| } | ||
| if (!complete) continue; | ||
| points.push({ executionId: execution.id, value: total }); | ||
| } | ||
| return points.reverse(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Sort executions before applying limit.
cohortMetricSparkline uses caller order, stops at limit, and then reverses the points. This only returns the latest chronological points when executions is already newest-first. Ascending or unsorted input can render a reversed trend and omit newer executions.
Sort by completed_at or started_at in descending order before the loop. Then reverse the selected complete points for rendering. Add a test where array order differs from timestamp order.
Proposed fix
function cohortMetricSparkline(executions, cohortRows, metricId, limit = 14) {
+ const orderedExecutions = [...(executions || [])].sort((left, right) => {
+ const leftDate = Date.parse(left?.completed_at || left?.started_at || "") || 0;
+ const rightDate =
+ Date.parse(right?.completed_at || right?.started_at || "") || 0;
+ return rightDate - leftDate;
+ });
const basket = (cohortRows || [])
.filter((row) => row && row.scenarioId)
.map((row) => ({
key: `${row.subjectId || ""}::${row.scenarioId}`,
fingerprint: row.fingerprint || "",
}));
if (!basket.length) return [];
const points = [];
- for (const execution of executions || []) {
+ for (const execution of orderedExecutions) {🤖 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 @.github/benchmark-site/execution-data.js around lines 900 - 923, Update
cohortMetricSparkline to sort executions by completed_at, falling back to
started_at, in descending chronological order before the limit-controlled loop;
preserve the existing complete-metric filtering and reverse the selected points
only after selection for rendering. Add coverage for executions whose array
order differs from timestamp order.
| card.value.textContent = card.format( | ||
| cohortRows.length ? metric?.comparableCurrent : metric?.operational, | ||
| ); | ||
| const meta = deltaMeta(metric?.delta); | ||
| card.delta.textContent = meta.label; | ||
| card.delta.className = `efficiency-delta delta-${meta.css}`; | ||
| renderEfficiencySparkline(card.sparkline, card.metricId, card.color); | ||
| renderEfficiencySparkline( | ||
| card.sparkline, | ||
| card.metricId, | ||
| card.color, | ||
| cohortRows, | ||
| cohortRows.length ? metric?.comparableBaseline : null, | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Disclose the full-suite fallback.
When cohortRows.length is zero, metric.operational is a full-suite total. The section text and accessible label still state that every card is a comparable-cohort total. This gives users the wrong population during baseline collection.
.github/benchmark-site/overview.js#L466-L478: Set a visible and accessible fallback label when the card usesmetric.operational..github/benchmark-site/index.html#L67-L70: Change the description so it accurately describes both comparable-cohort and full-suite fallback states..github/benchmark-site/index.html#L80-L80: Replace the static accessible label with wording that remains accurate during fallback, or update it dynamically.
📍 Affects 2 files
.github/benchmark-site/overview.js#L466-L478(this comment).github/benchmark-site/index.html#L67-L70.github/benchmark-site/index.html#L80-L80
🤖 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 @.github/benchmark-site/overview.js around lines 466 - 478, Update the card
rendering flow around renderEfficiencySparkline so cards using
metric.operational when cohortRows is empty receive a visible and accessible
full-suite fallback label, while comparable-cohort cards retain their existing
wording. In .github/benchmark-site/index.html lines 67-70, revise the section
description to cover both populations; in lines 80-80, replace or dynamically
update the static accessible label so it remains accurate during fallback.
Each efficiency card now prints its baseline median under the delta chip, the dashed baseline line carries a tooltip explaining the 7-run median, and each sparkline point's hover shows run id, date, value, and its delta against the baseline.
1 participant
Summary
The efficiency overview cards on the harness E2E dashboard were unreadable because each card mixed three different populations:
Two commits:
83966dc6(orphaned from (MOT-4305) feat(harness): discriminative judge-backed scenarios and scored hard-gate failures #672 — the PR was merged before this commit was pushed, so the cohort sparkline filter never reached main):cohortMetricSparkline()sums only the comparable-cohort contracts and skips executions missing any of them, with unit tests.Run <id>: <value>), the delta chip names its baseline ("↓ 22% vs baseline median"), the section copy states the population once, and "Suite cost" is renamed "Cost" since the figure is no longer the full suite. While no cohort exists yet, cards fall back to full-suite totals with the existing "Collecting comparable baseline" chip.Test plan
node --test .github/benchmark-site/*.test.cjs— 24/24Fixes MOT-4338
Summary by CodeRabbit
New Features
Improvements