test(highway): the R3c perf gate — measure the render loop before carving it

[byrongamatos]

highway.getPerf() (additive) + tests/browser/highway-perf-baseline.spec.ts. No behaviour change.

This lands before highway.js is touched, because a perf-gated refactor without a perf gate is just a refactor.

Frame rate is the wrong thing to measure

The highway auto-scales. When the smoothed draw cost passes _DRAW_BUDGET_HI_MS (12ms) it lowers the render resolution to protect the frame rate (#654).

That's exactly right for players. It also means a real performance regression does not show up as dropped frames — it shows up as a blurrier picture at a perfectly healthy 60fps.

Benchmark fps and you measure the feedback loop, not the renderer, and conclude nothing changed while the image quietly degrades.

So the gate pins the scale (setRenderScale(1) + setMinRenderScale(1), clamping autoScale to [1,1]) and measures drawMs — the renderer's own cost. None of that was reachable before: neither drawMs nor the effective scale escaped the closure. Hence getPerf().

The threshold is the app's own: _DRAW_BUDGET_HI_MS is the cost at which the highway itself starts sacrificing resolution in production. Exceeding it isn't an arbitrary benchmark line — it's the renderer failing its own budget. Current cost ~2.2ms, so ~5× headroom: far more than headless-CI variance, far less than any regression worth shipping.

I wrote this gate wrong three times. Each time it passed.

1. A vacuous assertion. My first cut asserted "the auto-scaler wasn't forced to intervene" — i.e. effectiveScale == 1. I injected a 10× regression (drawMs 2.4 → 22.4ms, nearly double the budget) and it passed.

Of course it did. setMinRenderScale(1) sets the scaler's floor to 1, so effectiveScale cannot drop below it. The very pinning that stops the scaler hiding a regression also stops it ever reporting one. A guard that cannot fail.

2. Measuring an idle renderer (Codex [P2]). playSong() takes ~3–4s to actually start — it's fetching and decoding stems. My "if not playing after 2s, togglePlay()" fired before autoplay, started playback, and then the app's own autoplay toggled it straight back to paused. The renderer idled through the entire measurement.

It now waits for playback rather than racing it, and asserts the chart clock advanced during the sampling window — not merely at some point beforehand, which my first fix would have accepted.

3. An unencoded filename (Codex [P2]). playSong() decodes its argument before building the /ws/highway path, so every real caller passes encodeURIComponent(filename) (app.js:2879, 4137). Raw, a name containing #, ?, % or / yields an invalid WebSocket URL and the song never loads — and on those libraries the gate would have silently measured an idle renderer instead of failing.

Every one of those bugs made the gate pass. That is the whole hazard of a perf test: it fails safe in the wrong direction.

Bite-tested — the only reason I trust it

	drawMs	result
10× regression injected into the draw path	22.0ms	❌ fails (budget 12ms)
clean build	2.1ms	✅ passes

…both under live playback, with the chart clock advancing 5.6s across the sample.

node 1045 · pytest 2412 · ESLint 0 · Codex 0.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Added a performance snapshot API for monitoring rendering cost, frame timing, scaling, and adaptive render thresholds.

• Tests

  • Added browser coverage to verify active rendering remains within its performance budget.
  • Added checks to prevent false passes caused by idle playback or stale measurements.

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: f36a31e9-4cce-4c71-ad9c-39617325a658

📥 Commits

Reviewing files that changed from the base of the PR and between dbbb1bd and 1783d58.

📒 Files selected for processing (2)

• static/highway.js
• tests/browser/highway-perf-baseline.spec.ts

---

📝 Walkthrough

Walkthrough

The highway API now exposes render-loop timing, scaling, and budget metrics. A Playwright test starts active chart playback, samples draw cost, and verifies it remains below the configured upper render budget.

Changes

Highway performance measurement

Layer / File(s)	Summary
Expose highway performance snapshot static/highway.js	Adds getPerf() with smoothed draw and frame timing, render-scale state, effective scale, and adaptive budget thresholds.
Validate active render budget tests/browser/highway-perf-baseline.spec.ts	Loads a real chart, starts playback, samples performance during clock advancement, and asserts valid draw metrics below the upper budget.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Playwright
  participant LibraryAPI
  participant Highway
  participant ChartClock
  Playwright->>LibraryAPI: load first real song
  Playwright->>Highway: pin render scale and start playback
  Highway->>ChartClock: advance chart clock
  Playwright->>Highway: sample getPerf()
  Highway-->>Playwright: return drawMs and drawBudgetHiMs
  Playwright->>Playwright: assert drawMs is finite and below budget

Loading

Possibly related PRs

• got-feedBack/feedBack#849: Moves highway render-time and adaptive-scaling state into hwState, which is read by the new performance API.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly references the highway performance gate and render-loop measurement added in this PR.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch r3c/highway-baseline

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

tests/browser/highway-perf-baseline.spec.ts

Parsing error: Unexpected token as

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

static/highway.js (1)

3149-3159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

getPerf() largely duplicates the pre-existing getPerfStats().

Both return drawMs, autoScale, renderScale, and effectiveScale; getPerf() just adds frameMs and the two budget constants. Two near-identical read-only snapshot methods on the same object invite drift (a future perf tweak could update one and forget the other). Consider having getPerfStats() delegate to getPerf() (or vice versa) to keep a single source of truth.

♻️ Example consolidation

 getPerfStats() {
-    return {
-        drawMs: hwState._drawMsEMA,
-        autoScale: hwState._autoScale,
-        renderScale: hwState._renderScale,
-        effectiveScale: _effectiveRenderScale(),
-    };
+    const p = api.getPerf();
+    return { drawMs: p.drawMs, autoScale: p.autoScale, renderScale: p.renderScale, effectiveScale: p.effectiveScale };
 },

🤖 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 `@static/highway.js` around lines 3149 - 3159, Consolidate the duplicate
performance snapshot logic in getPerf() and getPerfStats() by making one method
delegate to the other, preserving the existing shared fields and additional
frame/budget fields. Keep a single source of truth so future performance metrics
cannot diverge between the two methods.

🤖 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 `@static/highway.js`:
- Around line 3149-3159: Update _updatePerfHud so frame-timing bookkeeping runs
regardless of hwState._hudOn: calculate and smooth _frameMsEMA from
_lastFramePerf, then record the current timestamp before the HUD visibility
early return. Keep the existing HUD element creation/removal logic gated by
_hudOn, ensuring getPerf().frameMs remains live when the debug HUD is disabled.

---

Nitpick comments:
In `@static/highway.js`:
- Around line 3149-3159: Consolidate the duplicate performance snapshot logic in
getPerf() and getPerfStats() by making one method delegate to the other,
preserving the existing shared fields and additional frame/budget fields. Keep a
single source of truth so future performance metrics cannot diverge between the
two methods.

🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 57e539e3-4b6c-4f83-85b3-595dd32b15ec

📥 Commits

Reviewing files that changed from the base of the PR and between db3ca34 and dbbb1bd.

📒 Files selected for processing (3)

• static/highway.js
• static/tailwind.min.css
• tests/browser/highway-perf-baseline.spec.ts

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

getPerf().frameMs returns a stale/zero value unless the debug HUD flag is enabled.

frameMs is documented as "smoothed frame interval" but hwState._frameMsEMA is only ever mutated inside _updatePerfHud() (lines ~1379-1414), and only when hwState._hudOn is true — which itself only flips true when localStorage.getItem('highwayPerfHud') === '1'. For any caller of the new public API that hasn't manually set that debug flag, getPerf().frameMs will silently report 0 forever, since _lastFramePerf/_frameMsEMA are never touched otherwise. This isn't hit by the accompanying test (it only logs frameMs, never asserts on it), but it makes a documented field of the new public contract non-functional by default.

Root cause: the frame-timing bookkeeping is nested inside the same if (!hwState._hudOn) return; guard that also gates the DOM/HUD element lifecycle — those are two unrelated concerns sharing one early-return.

🐛 Proposed fix — decouple frame-timing EMA from the HUD-visibility gate (in `_updatePerfHud`, ~line 1379)

function _updatePerfHud() {
    if (typeof document === 'undefined' || !document.body) return;
    const nowP = performance.now();
    if (nowP - hwState._hudFlagAt > 500) {
        hwState._hudFlagAt = nowP;
        try { hwState._hudOn = localStorage.getItem('highwayPerfHud') === '1'; } catch (_) { hwState._hudOn = false; }
    }
    // Always update the EMA, independent of whether the HUD is displayed —
    // getPerf().frameMs must be live for any caller, not just when the debug
    // overlay is on.
    if (hwState._lastFramePerf) {
        const d = nowP - hwState._lastFramePerf;
        hwState._frameMsEMA = hwState._frameMsEMA === 0 ? d : hwState._frameMsEMA * 0.9 + d * 0.1;
    }
    hwState._lastFramePerf = nowP;
    if (!hwState._hudOn) {
        if (hwState._perfHud) { hwState._perfHud.remove(); hwState._perfHud = null; }
        return;
    }
    if (!hwState._perfHud) {
        // ... unchanged HUD element creation

🤖 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 `@static/highway.js` around lines 3149 - 3159, Update _updatePerfHud so
frame-timing bookkeeping runs regardless of hwState._hudOn: calculate and smooth
_frameMsEMA from _lastFramePerf, then record the current timestamp before the
HUD visibility early return. Keep the existing HUD element creation/removal
logic gated by _hudOn, ensuring getPerf().frameMs remains live when the debug
HUD is disabled.