refactor(highway): make the highway global explicit before the module flip (R3c)#912
Conversation
…le flip (R3c) 73 bare `highway.x` references -> `window.highway.x`, across app.js and 10 other files. Provably a NO-OP today. It is the precondition for flipping highway.js to a module. ━━━ WHY THIS HAS TO LAND FIRST ━━━ highway.js is a CLASSIC script. Its top-level `const highway = createHighway()` therefore creates a GLOBAL LEXICAL BINDING — visible as a bare name to every other classic script AND to every ES module. 73 call sites quietly rely on that. The moment highway.js becomes a module, that binding is gone. `const` in a module is module-scoped, not global. Every one of those 73 sites becomes a ReferenceError, and the flip is impossible until they say what they mean. `window.highway = highway` is already set, to the same object, on the same line. So this is an identity rewrite — verified in the browser below. ━━━ THE REWRITE BIT ME THREE TIMES. REGEX IS NOT ENOUGH FOR THIS. ━━━ 1. A SHADOWED LOCAL. capabilities/note-detection.js does `const highway = window.highway`. Its 9 bare uses are LOCAL and already correct; a blind rewrite would have emitted `const window.highway = window.highway`. Excluded. 2. HALF-CONVERTED GUARDS — the dangerous one. Six sites read `typeof highway !== 'undefined' && highway && typeof highway.setTime === 'function'`. The regex converted the CONSEQUENT and left the TEST, which is WORSE than not touching them: after the flip `typeof highway` is 'undefined', so each guard is PERMANENTLY FALSE and the code behind it silently never runs. transport.js's was the seek->setTime sync: the chart clock would have quietly desynced after every seek, with nothing failing. All six now test window.highway. 3. TWO MORE BARE REFERENCES, found by Codex [P2] and confirmed by an AST scan: app.js:3114 and :3176 use `highway && typeof window.highway.getSections === 'function'`. My grep searched for `typeof highway`, not `highway &&`. After the flip these throw, the catch swallows it, and the editor silently falls back to a ±4s edit window and arrangement 0. Regex missed a shadow, a half-conversion, and two bare reads. The final check is an AST pass that resolves scopes and reports every `highway` identifier not bound locally. It now reports ZERO. VERIFIED. A/B against origin/main in two browsers, 15 probes, IDENTICAL, zero page errors: window.highway is the same object as the bare global, the whole API surface resolves, a real song plays, the chart clock advances, getPerf().drawMs > 0 — and `seek syncs chart` passes, which is the exact guard I nearly broke in (2). node 1045, pytest 2416, ESLint 0, Codex 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe change standardizes highway access through ChangesWindow highway access migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PlayerScreen
participant resume-session
participant window.highway
PlayerScreen->>resume-session: snapshot resume position
resume-session->>window.highway: read song info and chart time
PlayerScreen->>window.highway: stop playback and transport
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
static/app.js (1)
1774-1774: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment references a nonexistent "window.highway.js" file.
The module is
highway.js; it only exposes awindow.highwayglobal. Wording it as "(window.highway.js)" reads like a filename and may confuse future readers, especially once this file becomes an ES module.✏️ Suggested wording
-// window._juceMode is otherwise decided once, at song-load time (window.highway.js), +// window._juceMode is otherwise decided once, at song-load time (highway.js, via window.highway),- // Stale until the incoming song's WS handler (window.highway.js) sets it again. + // Stale until the incoming song's WS handler (highway.js, via window.highway) sets it again.Also applies to: 2621-2621
🤖 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/app.js` at line 1774, Update the comments around the _juceMode references in static/app.js to identify the module as highway.js and its exposed window.highway global, removing the nonexistent “window.highway.js” filename wording. Apply the same clarification at both referenced locations without changing the surrounding logic.static/js/juce-audio.js (1)
114-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSame "window.highway.js" phrasing nit as in app.js.
These comments say "window.highway.js" as if it were a filename; the actual module is
highway.js, exposed globally aswindow.highway. Purely cosmetic, but worth aligning with the actual filename to avoid confusion once the module conversion lands.✏️ Suggested wording
- // window.highway.js's initial song-load routing consults this for the same + // highway.js's initial song-load routing (via window.highway) consults this for the same- // Don't race window.highway.js's own initial song-load routing: it owns + // Don't race highway.js's own initial song-load routing (via window.highway): it ownsAlso applies to: 386-386
🤖 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/juce-audio.js` at line 114, Update the comments near the initial song-load routing references in juce-audio.js to call the module “highway.js” and its global exposure “window.highway,” removing the incorrect “window.highway.js” filename phrasing. Apply the same wording correction to both occurrences.static/js/viz.js (1)
359-359: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent guarding of
window.highwayaccess.
_dropStaleNotationHint,_maybeShowNotationViewHint, and_autoMatchVizguardgetSongInfo()with optional chaining (window.highway?.getSongInfo()), but_installVizRenderer(Line 359) and_autoMatchViz's no-match fallback (Line 723) callwindow.highway.setRenderer(...)unguarded. This mirrors the pre-existing pattern (same asymmetry existed with barehighway.xbefore this migration), so it's not a regression from this PR, but worth tightening for consistency now that the seam is explicit.Also applies to: 582-583, 596-597, 644-645, 717-727
🤖 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/viz.js` at line 359, Make all window.highway accesses consistently guarded in _installVizRenderer and _autoMatchViz, including both setRenderer calls and the no-match fallback. Use optional chaining so these operations are skipped when window.highway is unavailable, while preserving existing behavior when it exists.
🤖 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/app.js`:
- Line 1774: Update the comments around the _juceMode references in
static/app.js to identify the module as highway.js and its exposed
window.highway global, removing the nonexistent “window.highway.js” filename
wording. Apply the same clarification at both referenced locations without
changing the surrounding logic.
In `@static/js/juce-audio.js`:
- Line 114: Update the comments near the initial song-load routing references in
juce-audio.js to call the module “highway.js” and its global exposure
“window.highway,” removing the incorrect “window.highway.js” filename phrasing.
Apply the same wording correction to both occurrences.
In `@static/js/viz.js`:
- Line 359: Make all window.highway accesses consistently guarded in
_installVizRenderer and _autoMatchViz, including both setRenderer calls and the
no-match fallback. Use optional chaining so these operations are skipped when
window.highway is unavailable, while preserving existing behavior when it
exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ac2a3d9a-d335-467b-b5da-56cef5f2695a
📒 Files selected for processing (14)
static/app.jsstatic/js/count-in.jsstatic/js/highway-colors.jsstatic/js/juce-audio.jsstatic/js/player-controls.jsstatic/js/resume-session.jsstatic/js/section-practice.jsstatic/js/transport.jsstatic/js/viz.jsstatic/v3/player-chrome.jsstatic/v3/venue-scene-3d.jstests/js/loop_restart.test.jstests/js/song_event_payload.test.jstests/js/speed_reset.test.js
Correction: this PR's stated premise was wrongPosting this against the merged PR so the commit message doesn't mislead whoever reads it next. I claimed:
That is false. I have now actually flipped highway.js to a module and measured it:
What that means for this PRThe rewrite is still defensible as hygiene — Worse: the "six half-converted guards" and "two bare reads" I reported as finds were bugs my own regex introduced and then repaired. They were not pre-existing. Net, this PR is churn plus a small hygiene gain, and I over-sold it. What the real hazard turned out to beNot A classic script's top-level Verified by removing the explicit assignment from the flip: That is the thing the flip had to preserve, and it is handled in the flip PR. It is also precisely the class of breakage this PR's premise claimed to be preventing — I just had the wrong name. |
Two lines. index.html: defer -> type="module". highway.js: one explicit assignment.
highway.js can now `import`, which is the whole point — the carve can begin.
━━━ THE ONE THING THE FLIP ACTUALLY BREAKS: window.createHighway ━━━
A top-level `function createHighway()` in a CLASSIC script IMPLICITLY becomes
window.createHighway. In a module it does not — module declarations are module-scoped, and
the name vanishes from the global object the instant the tag grows type="module".
The constitution names window.createHighway as PUBLIC EXTENSION CONTRACT (alongside
window.playSong / showScreen / feedBack). NOTHING IN-TREE CALLS IT. That is exactly why this
would have shipped: the only consumers are third-party plugins rendering their own highway
panel, and I cannot grep those. Green CI, green tests, and a broken plugin API.
Verified by removing the assignment and reloading:
flip WITHOUT an explicit assignment: window.createHighway === undefined <-- gone
flip WITH it: window.createHighway === function
So it is assigned explicitly now — same object, same behaviour, no longer an accident of how
the file happens to be loaded.
━━━ AND A CORRECTION TO #912 ━━━
#912 (merged) rewrote 73 bare `highway.x` -> `window.highway.x` on the stated grounds that
the flip would turn every one of them into a ReferenceError. HAVING NOW ACTUALLY FLIPPED IT,
THAT WAS WRONG. highway.js already did `window.highway = highway`, which puts the name on the
GLOBAL OBJECT — and bare-identifier resolution falls back to the global object whether or not
a lexical global binding exists. Measured on both builds: bare `highway` resolves either way.
#912 is defensible as hygiene and it does not hurt, but it was not a precondition and it fixed
no latent bug. A correction is posted on the PR so its commit message does not mislead. The
real hazard was the factory, not the instance — same class of breakage, wrong name.
ORDERING is unchanged: classic-defer and non-async type="module" share ONE post-parse
execution queue, in document order, so highway.js keeps its position at index.html:1244.
VERIFIED. A/B against origin/main: 15 probes IDENTICAL, zero page errors — window.highway,
window.createHighway, the full API surface, a real song playing, the chart clock advancing,
and the seek->setTime sync. THE PERF GATE PASSES at 1.85ms against its 12ms budget (module
evaluation costs nothing at render time), which is exactly what #910 was built to tell me.
node 1045, pytest 2416, ESLint 0, Codex 0.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
73 bare
highway.xreferences →window.highway.x, across app.js and 10 other files. Provably a no-op today. It is the precondition for flipping highway.js to a module.Why this has to land first
highway.js is a classic script. Its top-level
const highway = createHighway()therefore creates a global lexical binding — visible as a bare name to every other classic script and to every ES module. 73 call sites quietly rely on that.The moment highway.js becomes a module, that binding is gone.
constin a module is module-scoped, not global. Every one of those 73 sites becomes aReferenceError, and the flip is impossible until they say what they mean.window.highway = highwayis already set, to the same object, on the same line — so this is an identity rewrite. Verified in the browser below.The rewrite bit me three times. Regex is not enough for this.
1. A shadowed local.
capabilities/note-detection.jsdoesconst highway = window.highway. Its 9 bare uses are local and already correct; a blind rewrite would have emittedconst window.highway = window.highway. Excluded.2. Half-converted guards — the dangerous one. Six sites read:
The regex converted the consequent and left the test — which is worse than not touching them. After the flip,
typeof highwayis'undefined', so each guard is permanently false and the code behind it silently never runs.transport.js's was the seek →
setTimesync: the chart clock would have quietly desynced after every seek, with nothing failing. All six now testwindow.highway.3. Two more bare references — found by Codex [P2] and confirmed by an AST scan.
app.js:3114and:3176usehighway && typeof window.highway.getSections === 'function'. My grep searched fortypeof highway, nothighway &&. After the flip these throw, thecatchswallows it, and the editor silently falls back to a ±4s edit window and arrangement 0.Regex missed a shadow, a half-conversion, and two bare reads. The final check is an AST pass that resolves scopes and reports every
highwayidentifier not bound locally. It now reports zero.Verification
A/B against
origin/mainin two browsers — 15 probes, identical, zero page errors:window.highwayis the bare global (same object)getTime,setTime,getBPM,getSections,setAvOffset,getSongInfo,getPerf)getPerf().drawMs > 0seek syncs chart— the exact guard I nearly broke in (2)node 1045 · pytest 2416 · ESLint 0 · Codex 0.
🤖 Generated with Claude Code
Summary by CodeRabbit