refactor(app): carve the plugin loader out of app.js into static/js/ (R3a) - #878
Merged
Merged
Conversation
…(R3a)
The first carve, and deliberately the riskiest: app.js IS the plugin loader (the
R0 host rails), so it goes first while the module graph is still one edge deep.
static/js/plugin-loader.js (829 lines) — bodies VERBATIM. app.js 12,217 → 11,439.
Core's first `static/js/` module, exactly as constitution II anticipates.
CLOSURE (measured with acorn, not regex — brace-matching stripped source drifted):
the block at app.js:11246-12031 is contiguous and self-contained. It needs only
TWO things from the rest of app.js, and exports only TWO:
exports: loadPlugins (the window contract), bootstrapPluginsAndUi (boot)
inbound: window.showScreen — already the public host contract (constitution II),
so it is called through `window`, not re-coupled as an import
_populateVizPicker — injected via configurePluginLoader()
WHY A SEAM, NOT AN IMPORT. plugin-loader must not import app.js: app.js imports
it, so that would close a cycle. I checked whether _populateVizPicker could just
move into the module instead (which would delete the seam entirely) — it drags 9
further symbols (_canRun3D, _autoMatchViz, _showPromotionNag, …), i.e. a whole
viz cluster. That is its own carve, so the seam stays.
THE SEAM'S DEFAULT IS LOUD, ON PURPOSE. A no-op stub is the classic silent
failure for this pattern (see the editor's setHostHooks trap, hit twice): drop the
wiring call and the loader keeps working while the viz picker quietly stops
refreshing — no test, no boot check says a word. The default now console.errors,
so the smoke harness catches it. VERIFIED BY BITE TEST: removing
configurePluginLoader() from app.js surfaces
"[plugin-loader] host seam not configured" at boot. The seam IS exercised on the
plugin-startup path, so an unwired hook cannot pass silently.
no-cycle is now LIVE on core's own graph for the first time. eslint.config.js
gains `static/app.js` + `static/js/**` to the module block — app.js now `import`s,
so parsing it as a script would be a syntax error. VERIFIED BY BITE TEST: making
plugin-loader import app.js back fails with "Dependency cycle detected".
HARNESSES (the R3a note said budget one conversion per carve — it was five):
retargeted capability_inspector_nav, plugin_hydration_wipe,
plugin_loader_script_type, plugin_style_injection, legacy_shim_hits (SPLIT — one
test needs the loader, one still needs app.js) + test_plugin_runtime_idempotence.
legacy_shim_hits was missed by a symbol-name grep because it greps for a code
STRING; only the failing run found it. test_capability_events' NEGATIVE asserts
now span app.js + the loader — carving code out of app.js would otherwise make
them vacuous instead of failing.
VERIFIED: A/B against origin/main in two browsers — mounted plugin screens, 14
loaded plugin scripts, the 3 module plugins injected as <script type="module">,
37 capability participants, 14 shims, window.loadPlugins: IDENTICAL, zero
console/page errors on both. /static/js/plugin-loader.js serves 200; R0 rails
intact (src/main.js 200, conditional GET 304, script_type passthrough).
pytest 2396, node 1032/1032, ESLint 0, Codex 0.
Codex preflight caught a REAL [P1] first pass: static/js/plugin-loader.js was
untracked, so a checkout would have served an app.js importing a nonexistent
module — a failed static import kills the whole module and every window handler
with it. Now tracked.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe plugin loader is extracted from ChangesPlugin loader extraction and integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant App
participant PluginLoader
participant PluginAPI
App->>PluginLoader: bootstrapPluginsAndUi()
PluginLoader->>PluginAPI: Fetch plugin manifests
PluginAPI-->>PluginLoader: Return plugin list
PluginLoader->>PluginLoader: Reconcile navigation and plugin assets
PluginLoader->>PluginAPI: Subscribe to startup status
PluginAPI-->>PluginLoader: Report plugin registration or error
PluginLoader->>PluginLoader: Schedule plugin reload
PluginLoader-->>App: Populate visualization picker
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 |
Contributor
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/js/plugin-loader.js`:
- Around line 683-706: Update the module script URL construction in the plugin
loading flow around the script element setup and loadedScripts tracking so every
reload receives a fresh, monotonic cache-busting token, including when returning
to an older plugin.version. Preserve the existing version bookkeeping while
ensuring type="module" scripts never reuse a previously evaluated module URL.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 54ea3e73-3478-4a1d-b061-aa3d76064063
📒 Files selected for processing (9)
eslint.config.jsstatic/app.jsstatic/js/plugin-loader.jstests/js/capability_inspector_nav.test.jstests/js/legacy_shim_hits.test.jstests/js/plugin_hydration_wipe.test.jstests/js/plugin_loader_script_type.test.jstests/js/plugin_style_injection.test.jstests/test_plugin_runtime_idempotence.py
Contributor
Author
|
@coderabbitai Good catch, and the analysis is correct — but it's pre-existing, not introduced here, so I'm deferring it rather than riding it along on a move-only PR. It's a verbatim move. The URL construction and module injection are byte-identical to
(same for The bug is real though. Deferring because the fix is a behaviour change to the plugin-loading path (a monotonic load token changes the URL every reload, which also defeats the ETag/304 live-edit caching the R0 rails deliberately rely on — that interaction needs thinking about, not a one-liner). This PR is a verbatim carve whose entire value is being provably behaviour-neutral; folding a real fix into it would destroy that property and give the on-device pass nothing to compare against. Filing it as its own issue with your analysis. Thanks — this is a genuinely non-obvious one. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
If you'd like, I can open a follow-up GitHub issue capturing this analysis (repo: ✏️ Learnings added
|
byrongamatos
added a commit
that referenced
this pull request
Jul 11, 2026
…der seam (R3a) (#880) static/js/viz.js (770 lines) — the viz picker, renderer selection, Auto-match, the WebGL2 probe, the 3D-promotion nag, the notation hints. Bodies VERBATIM. app.js 11,603 → 10,857. THE SEAM IS GONE. #878's plugin-loader needed configurePluginLoader({ populateVizPicker }) purely because _populateVizPicker lived in app.js and importing app.js would have closed a cycle. viz.js is a LEAF — it imports NOTHING — so plugin-loader now imports _populateVizPicker straight from it. The _host object, the configure function, its loud-default guard, and the wiring line in app.js are all deleted. The second carve simplifies the first. app.js -> { plugin-loader, viz } plugin-loader -> viz viz -> (nothing) NOT A PURE MOVE — one listener block had to be SPLIT. app.js had a single top-level `if (window.feedBack) { … }` registering four handlers, and only two were viz. song:loaded / arrangement:changed / song:ready (the mastery slider) stay in app.js and now call the imported _autoMatchViz / _maybeShowNotationViewHint. The viz:reverted handler MOVES, because it REASSIGNS _cancelPendingAutoLabel and an imported binding is read-only — `_cancelPendingAutoLabel = null` would throw if the listener stayed behind while the state moved. ORDER CHECKED, NOT ASSUMED: viz.js's song:ready listener now registers BEFORE app.js's own (imports evaluate first). Safe — _pendingPromotionNag is only ever set inside _populateVizPicker, which runs at boot/plugin-refresh, never from inside the other song:ready handler, so the two are independent. VERIFIED — the listeners are the risk here, so they were DRIVEN, not just booted. A/B against origin/main in two browsers: * viz picker: 6 options (auto|default|venue|drum_highway_3d|keys_highway_3d| highway_3d), selected highway_3d, Auto label — IDENTICAL. This alone proves plugin-loader's direct import of viz.js works. * emit('viz:reverted') -> picker resets to default, localStorage resets to default, the warning logs — IDENTICAL. The MOVED listener fires. * emit('song:ready') -> mastery slider enables, no throw — IDENTICAL. The SPLIT listener still does both halves. * plugin screens, module injections, 37 capability participants — IDENTICAL. * zero console/page errors on both. pytest 2396, node 1038/1038, ESLint 0, tailwind-fresh clean. no-cycle re-bitten on the 3-module graph (viz -> plugin-loader fails). Codex preflight raised a [P2] claiming viz.js's top-level bus guards would be false because "app.js only creates the event bus later" — FALSE POSITIVE. app.js does not create the bus; capabilities.js does, from its own <script type="module"> at index.html:122, and module scripts execute in document order, so the bus exists long before app.js's import graph evaluates. Instrumented the setter: by viz.js's turn `window.feedBack.on` is already a function, and the viz:reverted listener is provably attached (firing it resets the picker). The ordering is also enforced by test_app_shell_loads_capability_registry_before_app_runtime. Harnesses: 5 tests retargeted to viz.js across legacy_shim_hits, venue_scene_3d, venue_viz (each SPLIT — their non-viz tests still read app.js). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 11, 2026
byrongamatos
added a commit
that referenced
this pull request
Jul 11, 2026
static/js/audio-el.js — one exported const. app.js's diff is 5 lines.
This is a HINGE, not a carve: nothing shrinks, but almost everything left in
app.js is blocked behind it.
WHY. `audio` is `document.getElementById('audio')` with 162 references in app.js
and 173 outside any one cluster. Every remaining cluster measured — settings,
app-updates, count-in (208 fns), exit-confirm (212), library-render (220) — lists
`audio` among its inbound symbols, because they all touch playback and playback
reaches for the element directly. A module that needs it cannot import app.js to
get it (that closes a cycle and fails import-x/no-cycle), so today the only way to
carve any of them would be a host seam — the exact thing #878 had to build and
#880 had to tear out.
WHY IT'S SAFE. `audio` is a `const` and is NEVER reassigned anywhere in core, so a
read-only import binding is exactly right and no state container is needed. The
162 call sites are untouched — the binding keeps its name, it is just imported
instead of declared. (Contrast the reassigned scalars — isPlaying, _avOffsetMs —
which CANNOT be shared this way: an imported binding cannot be written to. Those
still need containers, and that is the next problem, not this one.)
TIMING. app.js is <script type="module">, so it evaluates after the HTML is parsed
and its imports evaluate just before its body — the same moment app.js used to run
this exact lookup. If the element had not been in the document, `audio` would be
null and app.js's top-level `audio.addEventListener(...)` calls would throw and
kill the module. They don't.
VERIFIED WITH REAL PLAYBACK, not a boot check. A/B against origin/main in two
browsers: app alive with zero page errors (which is itself the proof the import
resolved), #audio is an AUDIO element, togglePlay/seekBy live, and playSong() on a
real library song sets audio.src and the element reports a duration — IDENTICAL on
both sides.
pytest 2396, node 1038/1038, ESLint 0 (no-cycle clean), tailwind clean, Codex 0.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
added a commit
that referenced
this pull request
Jul 11, 2026
static/js/audio-el.js — one exported const. app.js's diff is 5 lines.
This is a HINGE, not a carve: nothing shrinks, but almost everything left in
app.js is blocked behind it.
WHY. `audio` is `document.getElementById('audio')` with 162 references in app.js
and 173 outside any one cluster. Every remaining cluster measured — settings,
app-updates, count-in (208 fns), exit-confirm (212), library-render (220) — lists
`audio` among its inbound symbols, because they all touch playback and playback
reaches for the element directly. A module that needs it cannot import app.js to
get it (that closes a cycle and fails import-x/no-cycle), so today the only way to
carve any of them would be a host seam — the exact thing #878 had to build and
#880 had to tear out.
WHY IT'S SAFE. `audio` is a `const` and is NEVER reassigned anywhere in core, so a
read-only import binding is exactly right and no state container is needed. The
162 call sites are untouched — the binding keeps its name, it is just imported
instead of declared. (Contrast the reassigned scalars — isPlaying, _avOffsetMs —
which CANNOT be shared this way: an imported binding cannot be written to. Those
still need containers, and that is the next problem, not this one.)
TIMING. app.js is <script type="module">, so it evaluates after the HTML is parsed
and its imports evaluate just before its body — the same moment app.js used to run
this exact lookup. If the element had not been in the document, `audio` would be
null and app.js's top-level `audio.addEventListener(...)` calls would throw and
kill the module. They don't.
VERIFIED WITH REAL PLAYBACK, not a boot check. A/B against origin/main in two
browsers: app alive with zero page errors (which is itself the proof the import
resolved), #audio is an AUDIO element, togglePlay/seekBy live, and playSong() on a
real library song sets audio.src and the element reports a duration — IDENTICAL on
both sides.
pytest 2396, node 1038/1038, ESLint 0 (no-cycle clean), tailwind clean, 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.
The first carve, and deliberately the riskiest one: app.js is the plugin loader (the R0 host rails). It goes first, while the module graph is still one edge deep and a mistake is easy to see.
static/js/plugin-loader.js(829 lines) — bodies verbatim.app.js12,217 → 11,439. Core's firststatic/js/module, which is exactly the shape constitution §II anticipates.The closure
Measured with acorn, not regex — brace-matching the comment-stripped source drifted, and this is precisely the kind of file where an off-by-one is a silent disaster.
The block at
app.js:11246-12031turns out to be contiguous and self-contained. It needs two things from the rest of app.js and exports two:loadPluginswindowcontractbootstrapPluginsAndUiwindow.showScreenwindow, not re-coupled as an import_populateVizPickerconfigurePluginLoader()Why a seam and not an import
plugin-loadermust not importapp.js— app.js imports it, so that closes a cycle.I checked whether
_populateVizPickercould simply move into the module, which would delete the seam entirely. It drags 9 further symbols (_canRun3D,_autoMatchViz,_showPromotionNag,_ensureVenueVizOption, …) — a whole viz cluster. That's its own carve. So the seam stays.The seam's default is loud, on purpose
A silent no-op stub is the classic failure for this pattern — the editor hit it twice. Drop the wiring call and the loader keeps working while the viz picker quietly stops refreshing: no test, no boot check, nothing says a word.
So the default hook
console.errors instead, which the smoke harness (it treats console errors as failures) will catch.Bite-tested. Removing
configurePluginLoader()from app.js surfaces:That also proves the seam is exercised on the plugin-startup path, so an unwired hook cannot pass silently. Without this check, my A/B would have been blind to it: the viz picker is empty in the smoke env either way.
no-cycleis live on core's own graph for the first timeeslint.config.jsgainsstatic/app.js+static/js/**to the module block — app.js nowimports, so parsing it as a script would be a syntax error.Bite-tested: making
plugin-loaderimportapp.jsback fails withDependency cycle detected.Harnesses — the R3a note said budget one conversion per carve. It was five.
Retargeted:
capability_inspector_nav,plugin_hydration_wipe,plugin_loader_script_type,plugin_style_injection,legacy_shim_hits, plustest_plugin_runtime_idempotence.py.Two things worth knowing for the next carve:
legacy_shim_hitswas missed by a symbol-name grep because it greps for a code string (script.src = `/api/plugins/${plugin.id}/screen.js`), not a symbol. Only the failing run found it. It's also split — one test needs the loader, one still needs app.js.test_capability_events' assertions are NEGATIVE (assert token not in source). Carving code out of app.js would have made them vacuous instead of failing. They now span app.js + the loader.Verification
A/B against
origin/mainin two browsers — mounted plugin screens, 14 loaded plugin scripts, the 3 module plugins injected as<script type="module">, 37 capability participants, 14 compat shims,window.loadPlugins: IDENTICAL, zero console/page errors on both./static/js/plugin-loader.jsserves 200. R0 rails intact:stems/src/main.js→ 200, conditional GET → 304,script_typepassthrough.pytest 2396 · node 1032/1032 · ESLint 0 errors · tailwind-fresh clean · Codex 0.
Codex caught a real P1 on the first pass
static/js/plugin-loader.jswas untracked — I'd nevergit added it. A checkout of the patch would have served anapp.jsimporting a module that doesn't exist, and a failed static import kills the entire module and everywindowhandler with it. Now tracked. Worth flagging because a green local run says nothing about this.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests