refactor(highway): carve the STATEFUL primitives, threading hwState explicitly (R3c) (#916)

fretX, fillTextReadable, _noteState, _paintGemGlow -> static/js/highway-state-primitives.js.
50 call sites rewritten. highway.js 4,105 -> 3,965.

The first slice that changes signatures. Each of these four gains hwState as an explicit
FIRST PARAMETER.

━━━ hwState IS A PARAMETER, NOT AN IMPORT ━━━

createHighway() is a FACTORY. The constitution publishes window.createHighway so a plugin can
build a SECOND highway for its own panel, and highway.js says so itself. Import hwState as a
module singleton and two panels silently share one clock, one render scale, one string
palette — each driving the other. Nothing throws. The picture is just wrong, in a way no test
would catch.

(The exact opposite of the app.js carve, where player-state.js and library-state.js ARE
module singletons — correctly, because there is exactly one app. Same epic, same language,
opposite answer, decided entirely by whether the thing is a factory.)

━━━ THE PLUGIN BUNDLE NEARLY BROKE, SILENTLY ━━━

The renderer bundle hands two of these STRAIGHT TO PLUGINS:

    b.fretX = fretX;
    b.getNoteState = _noteState;   // stable reference

highway_3d calls both EVERY FRAME, with the old arity. Handing out the new 3-arg versions
would have passed `note` where hwState belongs — no throw, no error, just wrong geometry and
wrong judgment state INSIDE A PLUGIN, which no core test would ever see. Green CI, broken 3D
highway.

So hwState is bound ONCE per instance, in the factory, and the bundle hands out those views.
A per-frame arrow would have fixed the arity and reintroduced exactly the per-frame allocation
the bundle's stable-reference contract (feedBack#254) exists to prevent. b.project needs none
of this — project() is pure and its arity never changed.

VERIFIED IN A BROWSER, against the real bundle, on both builds:

    fretX arity                      3    3     (NOT 4 — the bound view preserves it)
    getNoteState arity               2    2
    fretX(5,1,800) in 0..800      True True
    getNoteState null w/o provider True True
    getNoteState honours provider  True True
    fretX is a stable reference    True True

IDENTICAL. Without the bound views fretX would have reported arity 4 and computed garbage.

Also caught on the way: my generated module imported STRING_BRIGHT_FALLBACK, a name
highway-constants.js does not export. ESLint does not flag that — but importing a name a
module does not export is a runtime SyntaxError that kills the WHOLE module. These four need
no constants at all; the import is gone.

TESTS. highway_note_state pins the signature AND the stable-reference contract — it caught the
bundle break. Retargeted at the module and the new arity; both contracts still asserted, and
the "no fresh arrow per frame" rule is now asserted explicitly rather than implied by
`getNoteState: _noteState`.

PERF GATE PASSES AT 1.94ms against its 12ms budget — fretX and _noteState are now CROSS-MODULE
calls, per note, per frame. It costs nothing measurable.

node 1045, pytest 2416, ESLint 0, Codex 0.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Byron Gamatos
2026-07-12 12:42:38 +02:00
committed by GitHub
co-authored by Claude Opus 4.8
parent 1a386c272d
commit 12eb73aee9
3 changed files with 268 additions and 200 deletions
+23 -4
View File
@@ -12,6 +12,10 @@ const fs = require('node:fs');
const path = require('node:path');
const highwayJs = path.join(__dirname, '..', '..', 'static', 'highway.js');
// R3c: _noteState moved to static/js/highway-state-primitives.js and gained an explicit
// hwState first parameter — it has to, because createHighway() is a factory and a module
// cannot import per-instance state without two panels sharing it.
const primitivesJs = path.join(__dirname, '..', '..', 'static', 'js', 'highway-state-primitives.js');
const highway3dJs = path.join(__dirname, '..', '..', 'plugins', 'highway_3d', 'screen.js');
// Brace-balanced extraction (same helper shape as highway_visibility.test.js).
@@ -51,7 +55,15 @@ test('_makeBundle exposes getNoteState (stable reference, no per-frame alloc)',
const fn = extractBlock(src, 'function _makeBundle()');
// The bundle field must point straight at _noteState — not a fresh
// arrow each frame (the per-frame allocation the review flagged).
assert.match(fn, /getNoteState\s*[:=]\s*_noteState\b/, 'bundle.getNoteState must be the stable _noteState reference');
// R3c: _noteState now takes hwState first, so the bundle hands out a per-INSTANCE bound
// view created ONCE in the factory (boundNoteState) rather than the raw function. The
// contract that matters is unchanged and still asserted: ONE stable reference, never a
// fresh arrow per frame (feedBack#254). Assert it is a bare identifier, not an inline
// function expression.
assert.match(fn, /getNoteState\s*[:=]\s*(?:boundNoteState|_noteState)\b/,
'bundle.getNoteState must be a stable reference (a name), not a per-frame arrow');
assert.doesNotMatch(fn, /getNoteState\s*[:=]\s*(?:\(|function)/,
'bundle.getNoteState must NOT be a fresh function per frame');
});
test('_makeBundle exposes getNoteStateProvider as a stable reference (feedBack#254)', () => {
@@ -79,7 +91,7 @@ test('_makeBundle exposes getNoteStateProvider as a stable reference (feedBack#2
test('_noteState normalizes provider output as documented', () => {
const src = fs.readFileSync(highwayJs, 'utf8');
const fn = extractBlock(src, 'function _noteState(note, chartTime)');
const fn = extractBlock(fs.readFileSync(primitivesJs, 'utf8'), 'function _noteState(hwState, note, chartTime)');
assert.match(fn, /if\s*\(\s*!hwState\._noteStateProvider\s*\)\s*return\s+null/, 'must short-circuit when no provider is registered');
assert.match(fn, /try\s*\{[\s\S]*_noteStateProvider\s*\([\s\S]*catch[\s\S]*return\s+null/, 'must call the provider inside try/catch and return null on throw');
assert.match(fn, /state\s*!==\s*['"]hit['"]\s*&&\s*state\s*!==\s*['"]active['"]\s*&&\s*state\s*!==\s*['"]miss['"]/, 'must reject states other than hit/active/miss');
@@ -94,8 +106,15 @@ test('default 2D renderer threads note state into drawNote / drawSustains / chor
// drawNote takes the trailing `ns` param.
assert.match(src, /function\s+drawNote\(\s*W\s*,\s*H\s*,\s*x\s*,\s*y\s*,\s*scale\s*,\s*string\s*,\s*fret\s*,\s*opts\s*,\s*ns\s*\)/, 'drawNote must accept the trailing ns param');
// drawNotes / drawSustains / drawChords gate the lookup on the provider.
assert.match(src, /_noteStateProvider\s*\?\s*_noteState\(\s*n\s*,\s*n\.t\s*\)\s*:\s*null/, 'visible-note paths must skip the lookup when no provider is set');
assert.match(src, /_noteStateProvider\s*\?\s*_noteState\(\s*cn\s*,\s*ch\.t\s*\)\s*:\s*null/, 'chord-note path must key the lookup by the chord time and gate on the provider');
// R3c: _noteState gained an explicit hwState first arg (it lives in a module now, and
// createHighway is a factory). The CONTRACT here is unchanged and still the point: skip
// the lookup entirely when no provider is set — a per-visible-note call on every frame.
assert.match(src, /_noteStateProvider\s*\?\s*_noteState\(\s*hwState\s*,\s*n\s*,\s*n\.t\s*\)\s*:\s*null/,
'visible-note paths must skip the lookup when no provider is set');
// Same, for the chord path: keyed by the CHORD's time (ch.t), not the note's, and still
// gated on the provider. Only the hwState arg is new.
assert.match(src, /_noteStateProvider\s*\?\s*_noteState\(\s*hwState\s*,\s*cn\s*,\s*ch\.t\s*\)\s*:\s*null/,
'chord-note path must key the lookup by the chord time and gate on the provider');
});
test('3D highway captures bundle.getNoteState and overrides legacy hit/miss with the provider verdict', () => {