Skip to content

fix(benchmark): any win is green — yellow is for losing only - #319

Merged
hugo-ccabral merged 2 commits into
mainfrom
fix/benchmark-any-win-is-green
Aug 25, 2026
Merged

fix(benchmark): any win is green — yellow is for losing only#319
hugo-ccabral merged 2 commits into
mainfrom
fix/benchmark-any-win-is-green

Conversation

@hugo-ccabral

@hugo-ccabral hugo-ccabral commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Problem

deltaTone in the benchmark HTML report needed a 5% margin before it would
paint the candidate green:

if (candMs < prodMs * 0.95) return DECK.soft; // meaningfully faster
if (candMs > prodMs * 1.05) return DECK.bad;  // meaningfully slower
return DECK.warn;                             // roughly even

So a real win of, say, 3% on LCP — or a step 30ms faster — came out the same
yellow as a tie or a small regression. In a client-facing report that reads
as "no better", which is the wrong story to tell about a win.

Fix

if (candMs < prodMs) return DECK.soft;        // ANY win, however small
if (candMs > prodMs * 1.05) return DECK.bad;  // meaningfully slower
return DECK.warn;                             // tie, or a loss too small to call meaningful

Yellow now only ever means tie-or-losing. The 5% dead band stays on the
losing side, where it belongs — a 2% regression really is run-to-run noise,
and reddening it would be the same false-signal bug mirrored.

All four call sites route through deltaTone — per-step deltas, the stat tiles,
the Lighthouse vitals cells and the bars — so navigation timings and Web Vitals
both pick this up with no further changes.

Test

Two cases added to tests/engine/benchmark-stats.test.ts pinning the new
contract (a 0.1% win is green; a 2% loss is yellow, not red). 14 pass.

🤖 Generated with Claude Code


Summary by cubic

Any faster candidate now renders green in the benchmark HTML report; yellow is only for ties or small losses. Previously green needed a 5% win; red still starts at >5% slower, and sub‑1% deltas now show one decimal to avoid "-0%" on green cells.

  • All views using deltaTone (step deltas, stat tiles, Lighthouse vitals, bars) inherit this behavior.
  • Tests pin a 0.1% win as green, a 2% loss as yellow, and ±0.3% display a decimal.

Written for commit 1df3349. Summary will update on new commits.

Review in cubic

`deltaTone` required a 5% margin before painting the candidate green, so a
real-but-modest win (say -3% on LCP, or a step 30ms faster) rendered in the
same yellow as a tie or a small regression. In a client-facing report that
reads as "no better", which is the wrong story for a win.

Now: candidate faster at all -> green. Slower by >5% -> red. Yellow means a
tie or a loss too small to call meaningful. The 5% dead band stays on the
losing side, where it belongs — a 2% regression really is noise.

All four call sites (step deltas, stat tiles, Lighthouse vitals cells, bars)
route through `deltaTone`, so navigation timings and Web Vitals both pick it up.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@hugo-ccabral

Copy link
Copy Markdown
Contributor Author

Verdict: COMMENT — the asymmetry is defensible and no higher-is-better metric goes through deltaTone, but the safety argument in the PR body ("the report also prints the signed percentage next to the color") is defeated by rounding, and the zero-value case still renders the biggest possible win as grey.

Worth fixing

  • src/report/benchmark-html.ts:96 + :104-106 — the exact range this PR newly turns green renders as a green -0%. pctChange does change.toFixed(0), so every win under 0.5% prints as -0%. Trigger: prod step 1000ms, cand 997ms → the stat tile (:284) and the step-delta header (:199) both render -0% in win-green. Before this change that cell was yellow, which at least agreed with the number printed inside it; now the colour asserts a win and the digits next to it say zero. That printed percentage was the stated reason the dead band was safe to remove, and it isn't doing the job. One decimal when |change| < 1, or keep sub-1% neutral.

  • src/report/benchmark-html.ts:95if (prodMs <= 0 || candMs <= 0) return DECK.muted treats zero as "no data", but for two of the five metrics routed through here zero is the best possible value, and Lighthouse returns it routinely. Trigger: prod home CLS 0.21, cand CLS 0deltaTone(0.21, 0) → muted grey rendered immediately next to 0.000 (:232). Same for TBT 0 on a light page. A perfect score is the largest win the report can express and it comes out looking like a missing measurement — exactly the failure mode this PR set out to fix, at the boundary. Needs candMs === 0 && prodMs > 0 → green, prodMs === 0 && candMs > 0 → bad, keep muted only for non-finite / both-zero (the ratio is genuinely undefined when prod is 0).

  • src/report/benchmark-html.ts:275-287 — the stat-tile grid never consults cs.ok, while renderStep:190-200 explicitly refuses to "present the delta as a win" for a failed step. Trigger: cand PDP route 404s and the error page paints in 300ms against prod's 1800ms → the tile reads 300ms · -83% in green while the step card right below it reads ⚠ error. Pre-existing, not introduced here, but it is the one surface where a green number can still hide a broken page.

  • src/report/benchmark-html.ts:268-269, 302hero-speedup prints ${suLabel} faster unconditionally. Trigger: prod total 4000ms, cand 5000ms → su = 0.8 → the largest number in a client-facing deck reads "0.80× mais rápido". The su >= 1 ? toFixed(1) : toFixed(2) branch shows sub-1 was anticipated; only the precision was handled, not the word. Out of this diff, same theme.

Correction to the PR body

Bars do not route through deltaTone. :209-210 hardcode DECK.faint for Fresh and DECK.soft for TanStack, and :163 colours the value label with the same token — so the candidate bar and its number are win-green even when the candidate loses badly (a step 40% slower paints a green bar beside a red +40%). Pre-existing, but the "all four call sites inherit this" claim covers three sites, not four. If "colour means win/loss" is the contract being established here, the bars are the one place still opting out.

Checked, fine

All five metrics reaching deltaTone (LCP/FCP/TTFB/TBT/CLS via vitalCell:228-233) are lower-is-better; LhSample.scores — the only higher-is-better field on the Lighthouse result — is never passed in, so the documented "lower is better" contract holds at every call site (:143, :231, :280). No other module imports deltaTone (deck-html.ts has its own tone tokens, no shared logic to keep in sync). No stale copy to update: the benchmark report ships no colour legend, and neither the info modal (:341-383), the footer methodology (:592-613), README nor CHANGELOG ever documented the 5% margin. The asymmetric band itself I'd keep — a 2% regression really is run-to-run noise and reddening it would be the mirrored bug.

Could not execute the two new cases (vitest isn't installed in my checkout); verified the thresholds arithmetically instead — deltaTone(1000, 999) → green, (1000, 1020) → warn, (1000, 1200) → bad, all as asserted.

Follow-up on the review of #319. `pctChange` rounds with `toFixed(0)`, so the
exact band this PR newly greens — a win under 0.5% — printed as a win-green
`-0%`: a cell whose colour and digits contradict each other. Sub-1% deltas now
carry one decimal (`-0.3%`), on both the stat tiles and the step-delta header.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@hugo-ccabral

Copy link
Copy Markdown
Contributor Author

Thanks — acted on the one finding this diff actually caused, left the rest alone.

Fixed (1df3349): the -0% green. pctChange rounded with toFixed(0), so the exact band this PR newly greens (a win under 0.5%) printed as a win-green cell whose own digits said "no change". Sub-1% deltas now carry a decimal — -0.3% — on both the stat tiles and the step-delta header. That restores the "the printed percentage keeps you honest" argument.

Correction accepted: the bars don't route through deltaToneDECK.soft is doing double duty as both the TanStack side colour and the win colour, which is what made them look like they participated. Three verdict call sites, not four: step delta, vitals cell, stat tile.

Not fixed here, all pre-existing and independent of this diff:

  • deltaTone's candMs <= 0 → muted guard greys a measured CLS/TBT of 0. Real, but the fix needs the stat tiles to distinguish "measured zero" from "step didn't run" first — for timings, 0ms genuinely means no measurement, and renderStep only survives it today because naNote/ok === false intercept first.
  • Stat tiles never check cs.ok, so a cand page that 404s fast tiles as a green win. Same root cause as the above: the tile grid has none of the failure guards renderStep grew.
  • hero-speedup prints "mais rápido" unconditionally, so a slower candidate announces "0.80× faster" as the loudest number in the deck.

Those three are one coherent follow-up — teaching the tile grid and the hero the failure/direction guards renderStep already has — not three patches bolted onto a colour-threshold change.

The asymmetric band is deliberate, not an oversight: green on any win is the requested behaviour, and the 5% dead band stays on the losing side because a 2% regression really is run-to-run noise.

@hugo-ccabral
hugo-ccabral merged commit eef601a into main Aug 25, 2026
4 checks passed
@hugo-ccabral
hugo-ccabral deleted the fix/benchmark-any-win-is-green branch August 25, 2026 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant