refactor(highway): carve the STATEFUL primitives, threading hwState explicitly (R3c) - #916
Merged
Merged
Conversation
…xplicitly (R3c)
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>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe Highway renderer now uses shared stateful primitives with explicit per-instance ChangesHighway state plumbing
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
byrongamatos
added a commit
that referenced
this pull request
Jul 12, 2026
…3c) (#917) 18 functions, 1,245 lines. highway.js 3,972 -> 2,727 (-31%). The biggest R3c slice: notes, sustains, chords, strum groups, unison bends and lyrics — everything the default renderer paints each frame. ━━━ MUTABILITY, NOT LOCATION, DECIDES WHERE A THING BELONGS ━━━ Three per-instance caches came out with this slice, and they are why it needed care: _frameMismatchWarned a warn-once Set of chord ids (feedBack#88) _chordRenderInfo a WeakMap of chord -> chain info _lyricMeasureCache Map<fontSize, Map<text, width>> All three are MUTATED. Left at module scope they would be SHARED ACROSS PANELS — one highway's lyric widths and chord chains stomping another's, silently, with nothing throwing. createHighway() is a factory (the constitution publishes window.createHighway so a plugin can build a second highway), so they are lifted onto hwState, which is exactly what hwState is for. The shimmer LUT went the OTHER way — to MODULE scope in highway-geometry.js. It is a deterministic xorshift table, byte-for-byte identical for every instance, so sharing it is not merely safe but BETTER: built once for the page rather than once per panel. Same slice, opposite directions, decided entirely by whether the thing mutates. ━━━ MY SCRIPT WAS WRONG TWICE. THE GATES CAUGHT BOTH. ━━━ 1. HAND-LISTED THE MOVE SET. I listed 10 functions and missed six that drawChords needs (_ensureChordRenderCache, bsearchChords, getChordTemplateInfo, _computeChordBox, _updateFretLinePreview, _drawFretLineChordPreview). The no-undef gate named every one. The set is now DERIVED from the dependency closure — 18, not 10. 2. JUDGED PURITY TOO EARLY, and this one is subtle. I classified _computeChordBox as pure because its ORIGINAL body never mentions hwState. Then the call-site rewriter injected `fretX(hwState, …)` INTO it — fretX takes hwState now (#916) — leaving a function that references an hwState it was never given. Purity has to be judged from the body AS IT WILL BE, so the classifier iterates to a fixed point: a function needs hwState if it mentions it, OR calls anything that now takes it. That moved _computeChordBox to the stateful side. VERIFIED. A/B against origin/main: IDENTICAL, zero page errors. The PLUGIN BUNDLE contract is byte-identical (b.fretX arity 3, b.getNoteState arity 2, both stable references, both correct under the old calling convention). PERF GATE PASSES AT 1.92ms against its 12ms budget — and this is the slice that could really have cost something: the ENTIRE per-frame drawing path is now cross-module. It costs nothing measurable. TESTS. highway_teaching_marks follows strumGroupBuckets to the new module. The two source-shape harnesses now read highway.js AND every static/js/highway-*.js, rather than being re-pinned at whichever file currently holds a function — re-pinning breaks again next time, and a shape assertion that silently stops finding its target is indistinguishable from one that passes. node 1045, pytest 2416, ESLint 0, no-undef 0, Codex 0. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fretX,fillTextReadable,_noteState,_paintGemGlow→static/js/highway-state-primitives.js. 50 call sites rewritten.highway.js4,105 → 3,965.The first slice that changes signatures. Each gains
hwStateas an explicit first parameter.hwStateis a parameter, not an importcreateHighway()is a factory. The constitution publisheswindow.createHighwayso a plugin can build a second highway for its own panel.Import
hwStateas 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.jsandlibrary-state.jsare 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:
highway_3d calls both every frame, with the old arity. Handing out the new 3-arg versions would have passed
notewherehwStatebelongs — 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
hwStateis 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.projectneeds none of this —project()is pure and its arity never changed.Verified in a browser, against the real bundle, on both builds
fretXaritygetNoteStatearityfretX(5,1,800)lands in 0..800getNoteState→ null without a providergetNoteStatehonours a providerfretXis a stable referenceIdentical. Without the bound views,
fretXwould have reported arity 4 and computed garbage.Also caught on the way
My generated module imported
STRING_BRIGHT_FALLBACK— a namehighway-constants.jsdoes not export. ESLint does not flag that. But importing a name a module doesn't export is a runtimeSyntaxErrorthat kills the whole module. These four need no constants at all; the import is gone.Tests
highway_note_statepins 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 bygetNoteState: _noteState.Perf gate passes at 1.94ms against its 12ms budget —
fretXand_noteStateare now cross-module calls, per note, per frame. It costs nothing measurable.node 1045 · pytest 2416 · ESLint 0 · Codex 0.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Refactor