refactor(app): the host seam + carve section practice out of app.js (R3a)#887
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughSection Practice logic moves from ChangesSection Practice extraction
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant section_practice_js
participant host_js
participant app_js
User->>section_practice_js: select section or phrase part
section_practice_js->>host_js: request loop and seek
host_js->>app_js: invoke playback hooks
app_js-->>host_js: return playback state
host_js-->>section_practice_js: complete practice request
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
byrongamatos
force-pushed
the
feat/r3-host-seam
branch
from
July 11, 2026 18:43
0c9e7ff to
ca16e6f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
static/js/section-practice.js (1)
1045-1065: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUnconditional final sleep wastes time in the failure path.
On the last retry attempt (
attempt === 4), ifokis stillfalse, the loop still awaitssetTimeout(res, 60 + attempt * 90)(≈420ms) before exiting, even though no further attempt will use that delay. This adds an unnecessary user-visible stall beforepracticeSection()falls back to turning off Section Practice mode on total failure.⏱️ Skip the trailing sleep on the last attempt
if (ok) break; - await new Promise(res => setTimeout(res, 60 + attempt * 90)); + if (attempt < 4) await new Promise(res => setTimeout(res, 60 + attempt * 90)); }🤖 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/section-practice.js` around lines 1045 - 1065, Update the retry loop around setLoop so it only waits before another attempt; after a failed attempt, skip the delay when attempt is the final iteration (attempt === 4). Preserve the existing retry timing and success break behavior for earlier attempts.
🤖 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/js/section-practice.js`:
- Around line 1045-1065: Update the retry loop around setLoop so it only waits
before another attempt; after a failed attempt, skip the delay when attempt is
the final iteration (attempt === 4). Preserve the existing retry timing and
success break behavior for earlier attempts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e95ce0ba-8124-402c-a4a7-ebd6d29a9639
📒 Files selected for processing (6)
static/app.jsstatic/js/host.jsstatic/js/section-practice.jstests/js/host_contract.test.jstests/js/loop_api.test.jstests/js/section_practice_dismiss.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- static/js/host.js
- static/app.js
…R3a)
static/js/host.js (99) + static/js/section-practice.js (1,214).
app.js 9,461 → 8,409.
THE FIRST SLICE OUT OF THE STRONGLY-CONNECTED CORE. What is left in app.js is not
a tree, it is a cycle: seeding a dependency closure from section-practice, from
loops, from count-in, or from the JUCE seek shim all return the SAME 178-function
set, and setLoop() and practiceSection() call each other directly. No closure-based
carve can cut it at any seed. So it is cut BY NAME, and the calls back into app.js
go through a host seam.
61 functions + its own 24 _sectionPractice*/_sectionParents* scalars (read nowhere
else) move out. 11 hooks come back in. 4 of those are read-only GETTERS —
loopA/loopB/_audioSeekGen/_loopMutationGen are only ever READ here, never written,
so app.js keeps owning them and NO state container is needed (a 977-site lift
avoided).
app.js used to reach IN and reset the module's state by hand (clearLoop() zeroed the
selection; changeArrangement() invalidated the parent count). It cannot now — an
imported binding is read-only — so those are exported as resetSelection() and
invalidateParentCount(). Strictly better: the module owns its own invariants instead
of trusting two callers on the far side of the file to zero the right three fields.
═══ THE SILENT-NO-OP PROBLEM, SOLVED ═══
The obvious host seam is an object of no-op defaults. That is a TRAP and we walked
into it once: the plugin loader's seam defaulted populateVizPicker to `() => {}`, so
a dropped wiring line would have left the viz picker quietly not refreshing with NO
test, boot check, or bot noticing. Two layers stop it here:
1. RUNTIME — host.js is a Proxy with NO defaults and NO stubs. Reading an unwired
hook THROWS. An unwired hook cannot degrade into a no-op because there is
nothing to degrade INTO. configureHost() also rejects a non-function at WIRE
time, and refuses to run twice.
2. STATIC — tests/js/host_contract.test.js asserts the hooks the modules USE are
exactly the hooks app.js WIRES. This is the layer that matters: a runtime throw
only fires if the broken path executes, and the whole danger of a seam is the
paths that never run in a smoke test. VERIFIED TO BITE in all three drift
directions: drop a hook from configureHost -> fails; rename host.setLoop in the
module -> fails; wire a hook nobody uses -> fails.
Writing that guard took three tries and each failure is instructive: (a) the
configureHost regex anchored `});` at column 0, ran past the indented close, and
swallowed app.js's 66-name window contract — 77 "hooks"; (b) an import-stripping
regex with `[\s\S]*?` ate 14,000 characters INCLUDING the drift the bite test was
meant to catch — a guard with a hole is worse than no guard, because you trust it;
(c) `host.js'` in the import path backtracked from `js` to a "hook" called `j`.
The bite tests are what surfaced all three.
CODEX FOUND A REAL RACE [P2]. configureHost() was inside the async boot function,
after several awaits — but the window handlers (onPhraseNext, …) go live during
app.js's SYNCHRONOUS module evaluation. A user clicking one in that window would hit
"[host] … was read before configureHost() ran". It is now a bare top-level statement
sitting immediately before the window contract, so the seam is always wired before a
handler can be reached. Verified live: invoking a handler 1.2s in — well before the
boot awaits settle — works.
VERIFIED. A/B against origin/main in two browsers with a REAL song loaded: popover
toggle, practice-mode change, phrase-next, and clearLoop (all of which cross the seam
— setLoop/clearLoop/_audioTime/loopA/loopB) — IDENTICAL, zero page errors. Since an
unwired hook throws, a live app is itself proof the seam is wired.
Harnesses: section_practice_dismiss retargeted; loop_api's clearLoop sandbox gains a
resetSelection SPY (not a stub) and ASSERTS it fires — the guarantee is still tested,
just through the seam.
pytest 2396, node 1040/1040, ESLint 0 (no-cycle clean), tailwind clean, Codex 0.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
force-pushed
the
feat/r3-host-seam
branch
from
July 11, 2026 18:50
ca16e6f to
d3e6357
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
static/js/section-practice.js (1)
344-350: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the three duplicated selection-reset implementations.
_sectionPracticeResetSelectionUi(), the tail of_hideSectionPracticeBar(), and the exportedresetSelection()each re-zero the same selection scalars independently. The module's own header comment argues the point of owning this state is that a caller shouldn't have to "zero the right fields" by hand — the same risk now exists internally across three call sites that can drift from each other.♻️ Proposed consolidation
+function _resetSectionPracticeSelectionScalars() { + _sectionPracticeSelected = -1; + _sectionPracticeWholeSection = false; + _sectionPracticeSavedPartIndex = 0; +} + function _sectionPracticeResetSelectionUi() { _sectionPracticeActiveParent = -1; - _sectionPracticeSelected = -1; - _sectionPracticeWholeSection = false; - _sectionPracticeSavedPartIndex = 0; + _resetSectionPracticeSelectionUi(); _sectionPracticeRanges = []; } export function resetSelection() { - _sectionPracticeSelected = -1; - _sectionPracticeWholeSection = false; - _sectionPracticeSavedPartIndex = 0; + _resetSectionPracticeSelectionUi(); }Also applies to: 790-814, 1201-1206
🤖 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/section-practice.js` around lines 344 - 350, Consolidate selection-state resets through _sectionPracticeResetSelectionUi: update the tail of _hideSectionPracticeBar() and the exported resetSelection() to call this helper instead of independently assigning the selection scalars. Keep the helper as the single owner of resetting _sectionPracticeActiveParent, _sectionPracticeSelected, _sectionPracticeWholeSection, _sectionPracticeSavedPartIndex, and _sectionPracticeRanges.
🤖 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/js/section-practice.js`:
- Around line 344-350: Consolidate selection-state resets through
_sectionPracticeResetSelectionUi: update the tail of _hideSectionPracticeBar()
and the exported resetSelection() to call this helper instead of independently
assigning the selection scalars. Keep the helper as the single owner of
resetting _sectionPracticeActiveParent, _sectionPracticeSelected,
_sectionPracticeWholeSection, _sectionPracticeSavedPartIndex, and
_sectionPracticeRanges.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 532ed184-48d4-4dde-96cb-27a97bbcd171
📒 Files selected for processing (6)
static/app.jsstatic/js/host.jsstatic/js/section-practice.jstests/js/host_contract.test.jstests/js/loop_api.test.jstests/js/section_practice_dismiss.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- static/js/host.js
This was referenced Jul 11, 2026
Merged
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 slice out of the strongly-connected core
What's left in app.js isn't a tree, it's a cycle. Seeding a dependency closure from section-practice, from loops, from count-in, or from the JUCE seek shim all return the same 178-function set — and
setLoop()andpracticeSection()call each other directly. No closure-based carve can cut it, at any seed.So it's cut by name, and the calls back into app.js go through a host seam.
_sectionPractice*/_sectionParents*scalars (read nowhere else) move outloopA/loopB/_audioSeekGen/_loopMutationGenare only ever read here, never written, so app.js keeps owning them and no state container is needed. That avoids a 977-site lift.app.js used to reach in and reset the module's state by hand (
clearLoop()zeroed the selection;changeArrangement()invalidated the parent count). It can't now — an imported binding is read-only — so those becameresetSelection()andinvalidateParentCount(). That's strictly better: the module owns its own invariants instead of trusting two callers on the far side of the file to zero the right three fields.The silent-no-op problem, solved
The obvious host seam is an object of no-op defaults. That is a trap, and we walked into it once: the plugin loader's seam defaulted
populateVizPickerto() => {}, so a dropped wiring line would have left the viz picker quietly not refreshing — with no test, boot check, or bot noticing.Two layers stop it:
1. Runtime.
host.jsis aProxywith no defaults and no stubs. Reading an unwired hook throws. An unwired hook cannot degrade into a no-op, because there is nothing to degrade into.configureHost()also rejects a non-function at wire time, and refuses to run twice.2. Static.
tests/js/host_contract.test.jsasserts the hooks the modules use are exactly the hooks app.js wires. This is the layer that matters — a runtime throw only fires if the broken path executes, and the whole danger of a seam is the paths that never run in a smoke test.Verified to bite in all three drift directions:
configureHosthost.setLoopin the moduleWriting that guard took three attempts, and each failure is worth knowing:
configureHostregex anchored});at column 0, ran past the indented close, and swallowed app.js's 66-name window contract — reporting 77 "hooks"[\s\S]*?ate 14,000 characters, including the very drift the bite test was meant to catch. A guard with a hole is worse than no guard, because you trust it.host.js'in the import path backtracked fromjsto a "hook" calledjThe bite tests are what surfaced all three. A guard nobody has tried to break is a guess.
Codex found a real race [P2]
configureHost()was inside the async boot function, after several awaits — but the window handlers (onPhraseNext, …) go live during app.js's synchronous module evaluation. A user clicking one in that window would hit[host] … was read before configureHost() ran.It's now a bare top-level statement, immediately before the window contract, so the seam is always wired before a handler can be reached. Verified live: invoking a handler 1.2s in — well before the boot awaits settle — works.
Verification
A/B against
origin/mainin two browsers with a real song loaded: popover toggle, practice-mode change, phrase-next, andclearLoop— all of which cross the seam (setLoop/clearLoop/_audioTime/loopA/loopB). Identical, zero page errors. Since an unwired hook throws, a live app is itself proof the seam is wired.Harnesses:
section_practice_dismissretargeted;loop_api'sclearLoopsandbox gains aresetSelectionspy (not a stub) and asserts it fires — the guarantee is still tested, just through the seam.pytest 2396 · node 1040/1040 · ESLint 0 (no-cycle clean) · tailwind clean · Codex 0.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes