refactor(app): carve the library out of app.js (R3a)#896
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughLibrary UI behavior is extracted from ChangesLibrary module extraction
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant UI
participant Library as library.js
participant ScanAPI as /api/scan-status
participant SongsGrid as v3 Songs grid
UI->>Library: rescanLibrary()
Library->>ScanAPI: poll scan status
ScanAPI-->>Library: scan completion
Library->>Library: reset caches and reload library
Library->>SongsGrid: emit library:changed
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
static/js/library.js (1,988) + static/js/library-state.js (29) — bodies VERBATIM.
app.js 6,313 -> 4,451.
THE BIGGEST SLICE OF THE CARVE: 145 declarations, ~1,900 lines, 30% of what was left.
The grid, the artist tree, the A-Z rail, filters, pagination, selection, favourites, the
scan banner, and the library-provider plumbing.
A LOW module: it imports only leaves (./dom.js, ./format.js, ./library-state.js,
./tuning-display.js — all four import nothing themselves) and needs ZERO host hooks. It
calls nothing in app.js. That is not luck; it is why this cluster was picked. Two entry
points that WOULD have dragged the playback core in were left behind in app.js:
* syncLibrarySong reaches showScreen/playSong
* _handleLibArrowNav Enter on a selected row plays the song
Both are one hop from the library, and app.js is the root, so it imports from both sides
for free. Pulling them in swallows playSong, showScreen and the whole remaining core — I
measured it: the closure jumps from 145 declarations to 189.
library-state.js holds exactly FIVE fields. An imported binding is read-only, and of the
library's outward bindings only these five are genuinely WRITTEN from outside — by
showScreen, deleteSongFromModal and syncLibrarySong, none of which can move in. The other
23 are read-only from outside, so they stay plain exports (ES live bindings mean app.js
still sees every reassignment).
━━━ THE EXPORT LIST NEARLY SHIPPED A DEAD A-Z RAIL ━━━
59 exports — and 43 of them CANNOT be found by a call-graph scan. They are referenced only
from app.js's TOP-LEVEL statements: the Object.assign(window, {...}) contract and the
scattered window.X = X lines, which live outside every function, so a closure walk over
declarations never sees them. Among them are the four handler names app.js composes AT
RUNTIME into onclick="" strings — filterTreeLetter, filterFavTreeLetter, goTreePage,
goFavTreePage — the library A-Z rail and its pagination. No static tool can see those at
all. Had I trusted the call-graph, the rail would have died silently on click with nothing
failing in CI.
━━━ AND MY OWN SCANNER LIED ━━━
The cycle-risk pass reported "(none)" for this carve. It was wrong, and it could not have
been right: a dangling `else if` bound to an inner `if` instead of the outer chain, so its
`imported` map was ALWAYS empty and the check reported clean no matter what. A guard that
cannot fail is worse than no guard. Fixed, and it then found the real edges — dom.js,
format.js, tuning-display.js, library-state.js. All four are leaves, so the carve is
genuinely acyclic; I just now know it instead of assuming it.
(The AST rewriter had its own trap: `MAP[name]` with an object literal and name ===
'constructor' hits Object.prototype.constructor — truthy — and it happily rewrote
`constructor(id)` into `L.function Object() { [native code] }(id)`. Every identifier in
the file is looked up, so the lookup must not see the prototype chain. It is a Map now.)
TESTS. legacy_shim_hits SPLIT (loadLibraryProviders + setLibraryProvider -> the module;
syncLibrarySong stayed in app.js). v3_library_refresh now reads app.js AND the module,
rather than being re-pinned to whichever file happens to hold the emit this week.
VERIFIED. A/B against origin/main in two browsers: the whole window contract, cards render,
grid/tree/sort/filter/clear round-trip — and, specifically, the A-Z rail: 28 onclick
handlers composed at runtime, identical on both, and a real .click() on a letter works.
IDENTICAL on all 33 + 7 probes, no new page errors.
pytest 2396, node 1040/1040, host contract 2/2, ESLint 0 (no-cycle clean).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
force-pushed
the
feat/r3-carve-library
branch
from
July 11, 2026 21:27
07a8deb to
a05b9d8
Compare
byrongamatos
added a commit
that referenced
this pull request
Jul 12, 2026
… (R3d) (#919) 4 functions, 234 lines. app.js 4,452 -> 4,218. Bodies VERBATIM. INTERFACE WIDTH ZERO — nothing in app.js calls into this cluster. app.js needs only the names on the window contract, so the markup's onclick= handlers resolve. That is what makes it the cleanest slice left. AND IT ONLY BECAME CLEAN BECAUSE THE LIBRARY CAME OUT FIRST (#896). Every dependency the modal has is a module now: it reads six bindings out of ./library.js (loadLibrary, loadFavorites, loadTreeView, _removeLibCardsForFilename, libView, _lastLibSelected) plus dom.js and the L container. Before that carve, extracting this would have dragged the whole library with it. Checked, and it matters: the modal never WRITES any of those six. An imported binding is READ-ONLY, so a single write would have forced a setter or a state container. Every use is a read, so plain imports suffice. Acyclic: edit-modal -> { dom, library-state, library }, and library imports none of them back. VERIFIED. A/B against origin/main in two browsers: the window contract, the modal actually OPENING off a real library row, its title and year fields rendering, and the data-edit-save wiring (rather than an inline onclick embedding the filename — the fix this cluster's harness exists to guard). IDENTICAL, no new page errors. node 1045, pytest 2425, ESLint 0 (no-cycle clean), host contract 2/2, Codex 0. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
added a commit
that referenced
this pull request
Jul 12, 2026
…creen, closeCurrentSong (R3d) (#921) 36 declarations + the 4 autoplay/auto-exit gate statements. 359 lines. app.js 3,772 -> 3,242. Bodies VERBATIM. ━━━ THIS WAS "THE UNCUTTABLE HEART", AND IT IS 359 LINES ━━━ At the start of this epic, seeding a dependency closure from count-in, from loops, from section-practice, or from the JUCE seek shim all returned the SAME 178-function, 3,360-line set. playSong and showScreen called each other; everything called them; nothing could be cut anywhere. The conclusion — correct at the time — was that NO closure-based carve could touch it at any seed, and the answer was a host seam. That was true THEN. Every slice taken out since (transport, loops, count-in, section-practice, the library, the edit modal, settings) removed edges, and the strongly-connected component DISSOLVED. This closure is 36 declarations with an interface width of FOUR. The lesson is not that the seam was wrong — the seam is what MADE this possible, by letting the carves proceed against a cyclic core instead of stalling on it. The lesson is to RE-MEASURE. An SCC is a fact about a graph at a moment, not a property of the code. ━━━ THE BUG NO SCAN COULD SEE, AND THE A/B DID ━━━ First cut passed every gate — no-undef clean, no-cycle clean, 1045/1045, pytest green — and THREW IN THE BROWSER: "Assignment to constant variable." window.feedBack.holdAutoplay / holdAutoExit and their two event handlers are TOP-LEVEL STATEMENTS, not declarations. They WRITE this cluster's state (_autoplayHeld, _autoExitTimer, …), and an imported binding is READ-ONLY — so left behind in app.js, every one threw the instant the module existed. A dependency scan that walks DECLARATIONS cannot see them. Mine didn't. This is the same blind spot that nearly shipped a dead library A-Z rail (#896): app.js keeps its public API in top-level statements, and a call-graph is blind to every one of them. The extractor now finds them by construction — any top-level statement that WRITES a moved binding comes with the carve — and the gate statements live beside the machinery they drive, which is where they belonged anyway. ━━━ ZERO OUTSIDE WRITES, BY MOVING THE BOUNDARY RATHER THAN BUILDING MACHINERY ━━━ The autoplay scalars and the wake-lock state were written from outside the cluster, which would have forced a setter or a state container. But the writers — _releaseAutoplay, _acquireWakeLock — plainly belong here. Pulling them in left ZERO outside writes, so every export is a plain import. Same move as settings (#920): measure the writers before you reach for a container. VERIFIED. A/B against origin/main in two browsers, IDENTICAL, zero page errors — including the autoplay gate driven end to end: a plugin HOLDS autoplay, the song loads but does not start, the RELEASE fires it, and a stale release is a no-op. That is the exact machinery that was throwing. node 1045, pytest 2425, ESLint 0 (no-cycle clean), host contract 2/2, Codex 0. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
added a commit
that referenced
this pull request
Jul 12, 2026
…shortcuts.js (R3d) (#922) 19 declarations + 23 TOP-LEVEL STATEMENTS. 922 lines. app.js 3,243 -> 2,325 (-28%). The panel registry, both global keydown dispatchers, the library arrow-nav, and the whole plugin-facing shortcut API. ━━━ MOST OF THIS SUBSYSTEM WAS NOT DECLARATIONS ━━━ A declaration-seeded dependency closure reports this cluster as 10 names, 246 lines. It is 42 statements and 922. window.registerShortcut, createShortcutPanel, getAllShortcuts, unregisterShortcut, clearWindowShortcuts, the panel registry, and BOTH global keydown dispatchers are bare TOP-LEVEL STATEMENTS at app.js's top level. A call-graph scan sees NONE of them. That blind spot has now cost three times: * it nearly shipped a dead library A-Z rail (#896) — 43 of library.js's exports were referenced only from app.js's window contract; * it threw "Assignment to constant variable" in the session carve (#921), where the autoplay gate's top-level statements wrote state that had just become a read-only import; * and here it under-reported the slice by 3x. The extractor takes them by construction now — any top-level statement that TOUCHES a moved binding comes along — and the SEED is closed to a FIXED POINT, because those statements have their own dependencies (_modifiersMatch, _isShortcutActive, _handleLibArrowNav, _gridColumns…) that the declaration closure never walked. Seed -> pull the statements -> the statements need more names -> re-seed. Iterate until it stops growing. ━━━ syncLibrarySong GOES ACROSS THE SEAM, NOT THROUGH AN IMPORT ━━━ The library arrow-nav calls it on Enter. It cannot be imported: syncLibrarySong reaches showScreen/playSong, and a module importing app.js closes a cycle. It is the ONE name here that had to stay behind, so it comes across the host seam — which is exactly what the seam is for. host.js throws loudly if the wiring is ever dropped, and tests/js/host_contract.test.js fails in CI if the hook drifts. VERIFIED. A/B against origin/main in two browsers, IDENTICAL, zero page errors — and driven for real, not merely present: the plugin API (register / unregister / getAll / panels), THE GLOBAL KEYDOWN DISPATCHER actually firing a registered shortcut, that same shortcut correctly SUPPRESSED while typing in a text input, and `?` opening the help modal. node 1045, pytest 2425, ESLint 0 (no-cycle clean), host contract 2/2, 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 biggest slice of the whole carve: 145 declarations, ~1,900 lines, 30% of what was left. The grid, the artist tree, the A–Z rail, filters, pagination, selection, favourites, the scan banner, and the library-provider plumbing.
A low module: it imports only leaves (
dom.js,format.js,library-state.js,tuning-display.js— all four import nothing themselves) and needs zero host hooks. It calls nothing in app.js.That's not luck; it's why this cluster was picked. Two entry points that would have dragged the playback core in were left behind in app.js:
syncLibrarySongshowScreen/playSong_handleLibArrowNavBoth are one hop from the library, and app.js is the root, so it imports from both sides for free. Pulling them in swallows
playSong,showScreenand the whole remaining core — measured: the closure jumps from 145 declarations to 189.library-state.jsholds exactly five fields. An imported binding is read-only, and of the library's outward bindings only these five are genuinely written from outside — byshowScreen,deleteSongFromModalandsyncLibrarySong, none of which can move in. The other 23 are read-only from outside, so they stay plain exports (ES live bindings mean app.js still sees every reassignment).The export list nearly shipped a dead A–Z rail
59 exports — and 43 of them cannot be found by a call-graph scan.
They are referenced only from app.js's top-level statements: the
Object.assign(window, {…})contract and the scatteredwindow.X = Xlines. Those live outside every function, so a closure walk over declarations never sees them.Among them are the four handler names app.js composes at runtime into
onclick=""strings —filterTreeLetter,filterFavTreeLetter,goTreePage,goFavTreePage— the library A–Z rail and its pagination. No static tool can see those at all. Had I trusted the call graph, the rail would have died silently on click, with nothing failing in CI.So I verified it the only way that counts: 28 onclick handlers composed at runtime, identical to
main, and a real.click()on a letter works.And my own scanner lied
The cycle-risk pass reported
(none)for this carve. It was wrong — and it could not have been right: a danglingelse ifbound to an innerifinstead of the outer chain, so itsimportedmap was always empty and the check reported clean no matter what. A guard that cannot fail is worse than no guard.Fixed. It then found the real edges —
dom.js,format.js,tuning-display.js,library-state.js. All four are leaves, so the carve is genuinely acyclic. I just know that now instead of assuming it.(The AST rewriter had its own trap:
MAP[name]on an object literal withname === 'constructor'hitsObject.prototype.constructor— truthy — and it cheerfully rewroteconstructor(id)intoL.function Object() { [native code] }(id). Every identifier in the file gets looked up, so the lookup must not see the prototype chain. It's aMapnow.)Tests
legacy_shim_hitsis split (loadLibraryProviders+setLibraryProvider→ the module;syncLibrarySongstayed in app.js).v3_library_refreshnow reads app.js and the module, rather than being re-pinned to whichever file happens to hold the emit this week.Verification
A/B against
origin/mainin two browsers — identical on all 33 + 7 probes, no new page errors: the whole window contract, cards render, grid/tree/sort/filter/clear round-trip, and the A–Z rail click above.pytest 2396 · node 1040/1040 · host contract 2/2 · ESLint 0 (no-cycle clean) · Codex 0.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes