refactor(highway): carve the constants into static/js/highway-constants.js (R3c)#914
Merged
Conversation
…ts.js (R3c)
29 constants, 190 lines. highway.js 4,267 -> 4,158. The first real slice, and the one that
every later one imports.
━━━ WHY ONLY THE CONSTANTS MAY LIVE AT MODULE SCOPE ━━━
createHighway() is a FACTORY, not a singleton. The constitution publishes
window.createHighway precisely so a plugin can build a SECOND highway for its own panel, and
highway.js already says so at the top of the closure:
// R3c: per-instance mutable state in one object, so extracted renderer/ws
// modules can close over it as a factory arg without cross-panel sharing.
So hwState — all 79 mutable properties — must NEVER become a module-level singleton: two
highways would silently share it, and one panel would drive the other's clock, scale and
colour tables. Extracted functions will take it as an ARGUMENT.
That is the OPPOSITE of the app.js carve, where a single state container (player-state.js,
library-state.js) was exactly right, because there is exactly one app. Same epic, same
language, opposite answer — because one is a singleton and the other is a factory.
These 29 are pure literals: numbers, strings and colour tables, never reassigned, never
mutated. Sharing them across instances is not merely safe, it is what you want — one copy of
the shimmer LUT bounds and the string palettes rather than one per panel. Anything with a
runtime dependency (document, window, performance, localStorage) stays in the factory;
checked, and none of these has one.
ESLint now knows static/highway.js is a module. It could not have known before this commit:
the flip (#913) changed the SCRIPT TAG, but the file had no import/export yet, so it still
parsed as a script and lint stayed green. The first `import` is what makes the config wrong.
TESTS. Four source-shape harnesses asserted `const _AUTO_SCALE_MIN = …` etc. lived in
highway.js. They now read highway.js AND every static/js/highway-*.js — deliberately, rather
than being re-pinned at whichever file currently holds a constant. Re-pinning just breaks
again on the next carve, and a source-shape assertion that silently stops finding its target
is indistinguishable from one that passes. Bite-tested: renaming two constants away fails
them.
VERIFIED. A/B against origin/main: 15 probes IDENTICAL, zero page errors. AND THE PERF GATE
PASSES AT 1.97ms against its 12ms budget — which is the point of having built it (#910)
first: these constants moved from closure scope to module scope, and V8 does not treat those
identically. It does here. Now I know rather than hope.
node 1045, pytest 2416, ESLint 0, Codex 0.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe change extracts highway rendering constants into a shared ES module, updates ChangesHighway constants modularization
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/js/highway_adaptive_scale.test.js (1)
30-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract
highwaySources()into a shared test helper.This exact helper (read
static/highway.js+ allstatic/js/highway-*.js, sorted, joined) is duplicated verbatim inhighway_monotonic_clock.test.js,highway_pause_throttle.test.js, andhighway_string_colors.test.js. Any future change to how highway modules are discovered/read (e.g. excluding a file, changing the join separator) requires updating four copies in lockstep, and a fix applied to only one will silently drift.♻️ Proposed fix: shared helper module
// tests/js/helpers/highway-sources.js const fs = require('node:fs'); const path = require('node:path'); function highwaySources() { const root = path.join(__dirname, '..', '..', '..'); const jsDir = path.join(root, 'static', 'js'); const parts = [fs.readFileSync(path.join(root, 'static', 'highway.js'), 'utf8')]; for (const f of fs.readdirSync(jsDir).sort()) { if (f.startsWith('highway-') && f.endsWith('.js')) { parts.push(fs.readFileSync(path.join(jsDir, f), 'utf8')); } } return parts.join('\n'); } module.exports = { highwaySources };Then in each test file:
-function highwaySources() { - const root = path.join(__dirname, '..', '..'); - const jsDir = path.join(root, 'static', 'js'); - const parts = [fs.readFileSync(path.join(root, 'static', 'highway.js'), 'utf8')]; - for (const f of fs.readdirSync(jsDir).sort()) { - if (f.startsWith('highway-') && f.endsWith('.js')) { - parts.push(fs.readFileSync(path.join(jsDir, f), 'utf8')); - } - } - return parts.join('\n'); -} +const { highwaySources } = require('./helpers/highway-sources');🤖 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_adaptive_scale.test.js` around lines 30 - 45, Extract the duplicated highwaySources() implementation into a shared tests/js/helpers/highway-sources.js module, preserving its current file discovery, sorting, reading, and newline-joining behavior. Update highway_adaptive_scale.test.js, highway_monotonic_clock.test.js, highway_pause_throttle.test.js, and highway_string_colors.test.js to import and reuse the shared highwaySources helper, removing their local copies.static/highway.js (1)
273-275: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOrphaned comments left behind after constant extraction.
These three lines are stray one-line comments (note the inconsistent 2/3-space indentation vs. the surrounding 4-space body) with no code attached — they used to sit beside the inline
_DRAW_BUDGET_HI_MS,_DRAW_BUDGET_LO_MS, and_AUTO_SCALE_MINdeclarations that were removed in this refactor. Meanwhilehighway-constants.jsnow declares those same constants (Lines 62-66 there) with no explanatory comment at all, so this context was dropped rather than moved. Move the comment content to the constants file next to their respective exports and delete this dangling remnant.♻️ Proposed fix
- // sustained draw cost above this -> scale down - // sustained draw cost below this -> scale back up - // hard floor (lowest the user-configurable floor may be set) // User-configurable floor for the load-adaptive render scale (`#654`):And in
static/js/highway-constants.js:+// Sustained draw cost above this -> scale down. export const _DRAW_BUDGET_HI_MS = 12; +// Sustained draw cost below this -> scale back up. export const _DRAW_BUDGET_LO_MS = 7; +// Hard floor (lowest the user-configurable floor may be set). export const _AUTO_SCALE_MIN = 0.25;🤖 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 273 - 275, Remove the three orphaned threshold comments from the area around the former declarations in static/highway.js, then move their explanatory text next to the corresponding _DRAW_BUDGET_HI_MS, _DRAW_BUDGET_LO_MS, and _AUTO_SCALE_MIN exports in highway-constants.js. Preserve the existing comment-to-constant mapping and match the constants file’s surrounding formatting.static/js/highway-constants.js (1)
1-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComment claims "frozen" but arrays aren't
Object.freezed.The header comment states these are highway.js's immutable constants: geometry, colour tables, timing budgets, and the load-adaptive render-scale thresholds. and later that they're pure literals: frozen numbers, strings and colour tables, never reassigned and never mutated — but
DEFAULT_STRING_COLORS/DEFAULT_STRING_DIM/DEFAULT_STRING_BRIGHTare plainconstarrays, notObject.freezed.constonly prevents rebinding the identifier; array contents remain mutable. Since these arrays are intentionally shared across allcreateHighway()instances (per the module's own stated design goal), an accidental in-place mutation anywhere (e.g. a future edit that indexes intoDEFAULT_STRING_COLORS[i] = ...instead of the per-instancehwState.STRING_COLORS) would silently corrupt colors for every highway instance, including splitscreen panels — exactly the cross-instance sharing bug this module is designed to avoid.Separately, the comment directly above the array declarations (These are
let, notconst: setStringColors() ... overrides per-index entries at runtime ... DEFAULT_* keep the originals so a reset restores them byte-for-byte.) reads as describing the array immediately below it, but it's actually contrasting with the per-instancehwState.STRING_COLORS/etc. declared inhighway.js— which aren'tletthere either (they're plain assignments to an object property). Consider rewording to explicitly namehwState.STRING_COLORSetc. so the contrast is unambiguous in this new file.🛡️ Proposed fix: freeze the shared default arrays
-export const DEFAULT_STRING_COLORS = [ +export const DEFAULT_STRING_COLORS = Object.freeze([ '`#cc0000`', '`#cca800`', '`#0066cc`', '`#cc6600`', '`#00cc66`', '`#9900cc`', '`#cc00aa`', '`#00cccc`', // 7th = magenta, 8th = teal -]; +]); -export const DEFAULT_STRING_DIM = [ +export const DEFAULT_STRING_DIM = Object.freeze([ '`#520000`', '`#524200`', '`#002952`', '`#522900`', '`#005229`', '`#3d0052`', '`#520042`', '`#005252`', -]; +]); -export const DEFAULT_STRING_BRIGHT = [ +export const DEFAULT_STRING_BRIGHT = Object.freeze([ '`#ff3c3c`', '`#ffe040`', '`#3c9cff`', '`#ff9c3c`', '`#3cff9c`', '`#cc3cff`', '`#ff3ce0`', '`#3ce0e0`', -]; +]);Also applies to: 120-133
🤖 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-constants.js` around lines 1 - 23, Freeze the shared default arrays DEFAULT_STRING_COLORS, DEFAULT_STRING_DIM, and DEFAULT_STRING_BRIGHT at their declarations so their contents cannot be mutated across createHighway instances. Update the nearby explanatory comment to explicitly identify hwState.STRING_COLORS and the corresponding per-instance properties as the runtime-overridden values, while describing DEFAULT_* as immutable reset originals.
🤖 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/highway.js`:
- Around line 273-275: Remove the three orphaned threshold comments from the
area around the former declarations in static/highway.js, then move their
explanatory text next to the corresponding _DRAW_BUDGET_HI_MS,
_DRAW_BUDGET_LO_MS, and _AUTO_SCALE_MIN exports in highway-constants.js.
Preserve the existing comment-to-constant mapping and match the constants file’s
surrounding formatting.
In `@static/js/highway-constants.js`:
- Around line 1-23: Freeze the shared default arrays DEFAULT_STRING_COLORS,
DEFAULT_STRING_DIM, and DEFAULT_STRING_BRIGHT at their declarations so their
contents cannot be mutated across createHighway instances. Update the nearby
explanatory comment to explicitly identify hwState.STRING_COLORS and the
corresponding per-instance properties as the runtime-overridden values, while
describing DEFAULT_* as immutable reset originals.
In `@tests/js/highway_adaptive_scale.test.js`:
- Around line 30-45: Extract the duplicated highwaySources() implementation into
a shared tests/js/helpers/highway-sources.js module, preserving its current file
discovery, sorting, reading, and newline-joining behavior. Update
highway_adaptive_scale.test.js, highway_monotonic_clock.test.js,
highway_pause_throttle.test.js, and highway_string_colors.test.js to import and
reuse the shared highwaySources helper, removing their local copies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3665c097-0663-436b-be91-acf91c0ce886
📒 Files selected for processing (7)
eslint.config.jsstatic/highway.jsstatic/js/highway-constants.jstests/js/highway_adaptive_scale.test.jstests/js/highway_monotonic_clock.test.jstests/js/highway_pause_throttle.test.jstests/js/highway_string_colors.test.js
byrongamatos
added a commit
that referenced
this pull request
Jul 12, 2026
…ometry.js (R3c) (#915) 6 functions, 53 lines. highway.js 4,158 -> 4,105. NOT ONE CALL SITE CHANGES. project, roundRect, bnvNormalizedPoints, teachingFingerLabel, teachingDegreeLabel, chordHarmonyLabels — the shared primitives every drawing function leans on. ━━━ PURITY IS THE WHOLE POINT OF THIS SLICE ━━━ Every one of these is a pure function of its arguments. None touches hwState. None closes over the canvas context — roundRect() already took `ctx` explicitly, and the rest need nothing but numbers. project() reads only the module-level constants from #914. That matters because createHighway() is a FACTORY: a plugin can build a second highway for its own panel, so anything holding per-instance state must be PASSED hwState rather than importing it, or two panels silently share one clock and palette. These six hold no state at all, so they move VERBATIM — the module boundary is invisible to every caller. The asserts are mechanical and in the extractor: it REFUSES to move a function whose body mentions hwState, or that references `ctx` without taking it as a parameter. Purity is checked, not assumed. ━━━ WHAT IS DELIBERATELY LEFT BEHIND ━━━ The four primitives that DO need hwState — fretX, fillTextReadable, _noteState, _paintGemGlow — stay in the factory for now. They need an explicit hwState parameter threaded through 53 call sites, which is a real behavioural change and belongs in its own commit rather than smuggled in beside a provably-identical move. Separating the provable from the risky is the whole discipline of this epic. TESTS. Three harnesses brace-match these functions out of the source and run them in a sandbox; they now read static/js/highway-geometry.js. `export function x` still contains `function x`, so the extractor needed no change — only the path. VERIFIED. A/B against origin/main: 15 probes IDENTICAL, zero page errors. PERF GATE PASSES AT 1.91ms against its 12ms budget — and this is the one that could plausibly have cost something: project() runs for every visible note on every frame and is now a CROSS-MODULE call. It costs nothing measurable. That is the answer #910 was built to give. node 1045, pytest 2416, ESLint 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.
29 constants, 190 lines.
highway.js4,267 → 4,158. The first real slice — and the one every later slice imports.Why only the constants may live at module scope
createHighway()is a factory, not a singleton. The constitution publisheswindow.createHighwayprecisely so a plugin can build a second highway for its own panel — and highway.js already says so at the top of the closure:So
hwState— all 79 mutable properties — must never become a module-level singleton. Two highways would silently share it, and one panel would drive the other's clock, scale and colour tables. Extracted functions will take it as an argument.That is the opposite of the app.js carve, where a single state container (
player-state.js,library-state.js) was exactly right — because there is exactly one app. Same epic, same language, opposite answer, because one is a singleton and the other is a factory.These 29 are pure literals: numbers, strings and colour tables, never reassigned, never mutated. Sharing them across instances isn't merely safe — it's what you want: one copy of the shimmer LUT bounds and the string palettes, not one per panel. Anything with a runtime dependency (
document,window,performance,localStorage) stays in the factory. Checked; none of these has one.A note on ESLint
The config now knows
static/highway.jsis a module. It could not have known before this commit: the flip (#913) changed the script tag, but the file had noimport/exportyet, so it still parsed as a script and lint stayed green. The firstimportis what makes the config wrong.Tests
Four source-shape harnesses asserted
const _AUTO_SCALE_MIN = …etc. lived in highway.js. They now read highway.js and everystatic/js/highway-*.js— deliberately, rather than being re-pinned at whichever file currently holds a constant.Re-pinning just breaks again on the next carve, and a source-shape assertion that silently stops finding its target is indistinguishable from one that passes. Bite-tested: renaming two constants away fails them.
Verification
A/B against
origin/main: 15 probes identical, zero page errors.And the perf gate passes at 1.97ms against its 12ms budget — which is exactly why it was built (#910) first. These constants moved from closure scope to module scope, and V8 does not treat those identically. It does here. Now I know, rather than hope.
node 1045 · pytest 2416 · ESLint 0 · Codex 0.
🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Tests