refactor(highway): carve the 2D drawing layer into highway-draw.js (R3c)

[byrongamatos]

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're 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, so they're 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's a deterministic xorshift table, byte-for-byte identical for every instance, so sharing it isn't 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. I 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. I 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 referencing an hwState it was never given.

Purity has to be judged from the body as it will be. The classifier now iterates to a fixed point: a function needs hwState if it mentions it, or calls anything that now takes it. That correctly moved _computeChordBox to the stateful side.

Verification

• 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 genuinely 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.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Improvements
  • Improved highway rendering for notes, chords, sustains, lyrics, techniques, and unison bends.
  • Enhanced chord previews, muted/repeat indicators, harmony annotations, and open-string displays.
  • Added smoother, consistent shimmer effects for held notes and sustains.
  • Improved lyric wrapping, timing, highlighting, and readability.
• Reliability
  • Improved rendering consistency across different highway instances and playback states.
  • Expanded automated coverage for note states, chord rendering, and teaching markers.

[coderabbitai]

📝 Walkthrough

Walkthrough

The highway renderer is split across geometry and drawing modules. Rendering helpers now use hwState for per-instance caches, chord previews, lyric measurement, and draw-loop state. Tests load and inspect the combined modularized source.

Changes

Highway renderer modularization

Layer / File(s)	Summary
Renderer state and module wiring static/highway.js, tests/js/highway_note_state.test.js	Drawing imports, hwState-first calls, per-instance caches, reset behavior, and modular source assertions are updated.
Note and sustain rendering static/js/highway-draw.js, static/js/highway-geometry.js, tests/js/highway_teaching_marks.test.js	Note, sustain, strum-group, unison-bend, and shimmer rendering are implemented in the drawing modules with related source-based tests.
Chord, lyric, and preview rendering static/js/highway-draw.js, tests/js/highway_chord_render_cache.test.js	Chord caching, chord previews, lyric layout, binary searches, technique classification, and corresponding cache tests are moved to the modular source structure.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

• got-feedBack/feedBack#729: Both changes affect held-sustain glow rendering.
• got-feedBack/feedBack#849: Earlier per-instance hwState lifting connects directly to this renderer rewiring.

🚥 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 summarizes the main change: extracting Highway's 2D drawing layer into highway-draw.js.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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/draw

---

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

[coderabbitai]

🧹 Nitpick comments (2)

static/js/highway-draw.js (1)

1076-1094: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Optional: bsearch and bsearchChords are identical.

Both are lower-bound searches keyed on .t; bsearchChords adds nothing over bsearch. If the separate export exists only for a stable bundle reference (lowerBoundT = bsearch), a re-export alias would remove the duplicate implementation while preserving the name.

🤖 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/js/highway-draw.js` around lines 1076 - 1094, Remove the duplicate
search implementation from bsearchChords and make it reuse bsearch while
preserving bsearchChords as a named export. Keep the existing lower-bound
behavior and stable exported symbol unchanged.

tests/js/highway_note_state.test.js (1)

44-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff

Optional: highwaySources() is duplicated verbatim in tests/js/highway_chord_render_cache.test.js.

Consider extracting this loader into a shared test helper so future source-shape changes only touch one place.

🤖 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 `@tests/js/highway_note_state.test.js` around lines 44 - 52, Extract the
duplicated highway source-loading logic from highwaySources() into a shared test
helper, then update both highway_note_state.test.js and
highway_chord_render_cache.test.js to use it. Preserve the existing file
discovery, sorting, and concatenation behavior.

🤖 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.

Nitpick comments:
In `@static/js/highway-draw.js`:
- Around line 1076-1094: Remove the duplicate search implementation from
bsearchChords and make it reuse bsearch while preserving bsearchChords as a
named export. Keep the existing lower-bound behavior and stable exported symbol
unchanged.

In `@tests/js/highway_note_state.test.js`:
- Around line 44-52: Extract the duplicated highway source-loading logic from
highwaySources() into a shared test helper, then update both
highway_note_state.test.js and highway_chord_render_cache.test.js to use it.
Preserve the existing file discovery, sorting, and concatenation behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: f101cb4f-03f8-4cb4-8476-8484b5e28fb0

📥 Commits

Reviewing files that changed from the base of the PR and between 12eb73a and 32d1768.

📒 Files selected for processing (6)

• static/highway.js
• static/js/highway-draw.js
• static/js/highway-geometry.js
• tests/js/highway_chord_render_cache.test.js
• tests/js/highway_note_state.test.js
• tests/js/highway_teaching_marks.test.js