refactor(ui): load app.js as an ES module (R3a) — the flip - #876
Conversation
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>
📝 WalkthroughWalkthroughThe application entrypoint in ChangesApp entry loading
Estimated code review effort: 1 (Trivial) | ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@static/v3/index.html`:
- Line 1247: The module script at /static/app.js no longer exposes functions
used by inline handlers globally. Update the app.js initialization/export flow
to attach every referenced handler—such as showScreen, uploadSongs,
setLibraryProvider, setLibView, sortLibrary, toggleLibFilters, setFavView,
setAvOffsetMs, persistSetting, setLoopStart, changeArrangement, setSpeed, and
togglePlay—to window, or replace the inline handlers with module-managed
listeners while preserving their behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
@coderabbitai Thanks — but this is a false positive, and interestingly it's the exact mirror of the one Codex raised on this same PR. app.js's window surface is split across two forms:
Your script collected only the first form: …so all 66 block names look "missing". Every one of the 13 you listed is in the block: And at runtime, with More broadly: I drove all 336 inline handlers in the shell under module scope and resolved each one's leading identifier against Exposing these 66 names is precisely what #874 did, as a standalone no-op PR, ahead of this flip. No change needed. That said, two independent reviewers misread the same split surface in opposite directions, which is a fair signal that consolidating the 47 scattered assignments into the single block is worth a follow-up cleanup — just not inside a move-only PR. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
fd app.js static/ | head
echo "---scattered window.X assignments---"
rg -n 'window\.[A-Za-z_$][\w$]*\s*=' static/app.js | wc -l
echo "---Object.assign(window block---"
rg -n 'Object\.assign\(window' static/app.js -A 80 | head -100
echo "---specific names check---"
rg -n 'showScreen|togglePlay|setSpeed|uploadSongs|setLibraryProvider' static/app.js | head -40
echo "---window_contract test---"
fd window_contract.test.js
cat tests/js/window_contract.test.js 2>/dev/null | head -100Length of output: 10195
I also checked Thanks for the detailed breakdown — this is a false positive on my end. I agree consolidating the two forms into one would reduce this kind of confusion going forward, but that's a reasonable follow-up rather than something needed in this move-only PR. (=^･ω･^=) ﾉ good catch — noted for next time ✏️ Learnings added
|
1 participant
The flip. One attribute. #871, #872, #874 and #875 exist to make this line safe.
After this,
app.jscanimport— the monolith-killing enabler is done, and carving it intostatic/js/*.jsbecomes routine follow-on work.What changes
app.js's 385 top-level
functiondeclarations stop being implicitwindowproperties:Object.assignblock plus 47 pre-existing scatteredwindow.X = XassignmentsVerified: no unexposed name is read from outside app.js.
Strict mode
Modules are always strict, and app.js has never run under it. Checked before flipping:
sourceType: "module"(no octal literals, duplicate params,with)eval/new Function, no top-levelthisregisterShortcutis called bare at 15 top-level sites. It's assigned atwindow.registerShortcut(app.js:10387), before its first call (10648), and a bare identifier in a module still resolves through the global object. Confirmedtypeof window.registerShortcut === 'function'in the browser.Hard gate — app.js is the plugin loader
/api/pluginsscript_typepassthrough"module"/api/plugins/stems/src/main.jsstems/src/transport.js,editor/src/state.jswindow.loadPluginsfunction<script type="module">All 336 inline handlers in the shell resolve on
windowunder module scope; the A–Z rail and pagination execute 6/6 with no ReferenceError.A/B against
origin/main: the only unresolvable handler iseditorToggleStemMixer— equally broken on main. It's a dead handler in the editor plugin (not defined anywhere in its source — looks like rename skew). Pre-existing, unrelated to this PR, worth its own issue.Codex preflight — one [P1], a false positive
Codex claimed
restartCurrentSong,requestExitSong,editRegionInEditorandreturnToEditorFromHighwaywouldReferenceErrorbecause they're absent from #874'sObject.assignblock.They are absent from it — because they were already explicit, among app.js's 47 scattered
window.X = Xassignments:With app.js loaded as
type="module", all four resolve asfunctionin the browser, andrequestExitSong()is live as a realonclicksource. The contract test already covers this — it collects both the scattered assignments and the block. Codex's scan only read the block.Suites
pytest 2396 passed · node 1032/1032 · ESLint 0 errors · tailwind-fresh clean.
🤖 Generated with Claude Code
Summary by CodeRabbit