refactor(ui): load the capabilities as ES modules (R3a) - #875
Merged
Conversation
The 12 capability <script> tags become type="module". No JS changes — the capability scripts already self-register on the window.feedBack bus, version-negotiate (`capabilities.version !== 1` → bail), and self-guard for idempotency. They never import or call app.js; it is pure pub/sub. Verified they export nothing by name: no top-level declaration in capabilities.js or capabilities/*.js is read by any other script, so losing global scope costs nothing. This is the first REAL exercise of the ordering fix from #872. A module defers to after HTML parse, so the capabilities now execute AFTER the document is parsed — while app.js still calls `window.feedBack.on(...)` at its top level. That only works because #872 put every classic script into the same deferred queue, where document order IS execution order: capabilities.js (line 122) still runs before app.js (line 1237). Had app.js stayed a plain classic script it would have run during parse, hit a bare `{}`, and died on `.on is not a function`. A/B against origin/main, 11 probes — capabilities.version, registered participants (37), compatibility shims (14), the bus, workingTuning, theme, setViz/showScreen/playSong, mounted plugin screens: IDENTICAL, zero console/page errors on both. 12 module tags served and executed; pytest 2396, node 1032/1032, 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 (1)
📝 WalkthroughWalkthroughThe v3 page changes capability-related script tags from deferred classic scripts to ES module scripts, covering the main bundle and capability submodules. ChangesCapability module loading
Estimated code review effort: 2 (Simple) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
byrongamatos
added a commit
that referenced
this pull request
Jul 11, 2026
One attribute. #871/#872/#874/#875 exist to make this line safe. app.js's 385 top-level `function` declarations stop being implicit `window` properties: 87 stay reachable via the explicit contract (#874's Object.assign block + the 47 pre-existing `window.X = X` assignments), and 298 become module-private. Verified NO unexposed name is read from outside app.js. Strict mode (modules are always strict) checked ahead of the flip: app.js parses clean as `sourceType: module` (no octal, dup params, `with`), and has no implicit globals, no `eval`/`new Function`, no top-level `this`. `registerShortcut` is called bare at 15 top-level sites but is assigned at `window.registerShortcut` (app.js:10387) before its first call (10648), and a bare identifier in a module still resolves through the global object — verified `typeof window.registerShortcut === 'function'` in the browser. HARD GATE — app.js IS the plugin loader: - /api/plugins script_type passthrough: editor/stems/studio = "module" - /api/plugins/stems/src/main.js -> 200; conditional GET -> 304 (live-edit ETag) - deep graph: stems/src/transport.js, editor/src/state.js -> 200 - window.loadPlugins present; 5 plugin screens mount; the 3 migrated plugins injected as <script type="module"> - 37 capability participants, 14 compatibility shims, bus + capabilities v1 Every one of the shell's 336 inline handlers resolves on window under module scope, and the A-Z rail / pagination execute 6/6 with no ReferenceError. A/B against origin/main: the ONLY unresolvable handler is `editorToggleStemMixer`, which is equally broken on main (a dead handler in the editor plugin — not defined anywhere in its source; pre-existing, flagged separately). Codex preflight raised a [P1] claiming restartCurrentSong / requestExitSong / editRegionInEditor / returnToEditorFromHighway would ReferenceError — FALSE POSITIVE. It scanned only #874's new Object.assign block and missed app.js's 47 scattered `window.X = X` assignments; all four are at app.js:7086/7204/8492/8511 and all four resolve as `function` in the browser with app.js loaded as a module. pytest 2396, node 1032/1032, ESLint 0 errors, tailwind-fresh clean. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
5 tasks
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.
Step 3 of the core-frontend ES-module migration (R3a), on top of #871/#872/#874. One file, 12 lines — the capability
<script>tags becometype="module". No JS changes.Why it's safe
The capability scripts already communicate only through the
window.feedBackbus: they self-register, version-negotiate (capabilities.version !== 1→ bail), and self-guard for idempotency. They never import or call app.js — it's pure pub/sub.I checked they export nothing by name: no top-level declaration in
capabilities.jsorcapabilities/*.jsis read by any other script, so losing global scope costs nothing.This is the first real exercise of #872's ordering fix
A module defers to after HTML parse. So the capabilities now execute after the document is parsed — while
app.jsstill callswindow.feedBack.on(...)at its top level (11 sites).That only works because #872 put every classic script into the same deferred queue, where document order is execution order:
capabilities.js(line 122) still runs beforeapp.js(line 1237).Had app.js stayed a plain classic script, it would have run during parse — ahead of the deferred capability modules — hit the bare
{}fromwindow.feedBack = window.feedBack || {}, and died onwindow.feedBack.on is not a function. That is precisely the failure the original plan's "zero-risk warm-up" would have shipped, and why #872 came first.Verification
A/B against
origin/main, 11 probes —capabilities.version, registered participants (37), compatibility shims (14), the bus,workingTuning,theme,setViz/showScreen/playSong, mounted plugin screens:IDENTICAL on every probe. Zero console/page errors on both.
12
<script type="module">tags served and executed; 5 plugin screens still mount. pytest 2396, node 1032/1032, ESLint 0 errors, tailwind-fresh clean, Codex preflight 0.Next: app.js →
type="module"— the flip, with the boot-a-plugin hard gate (app.js is the plugin loader).🤖 Generated with Claude Code
Summary by CodeRabbit