refactor(app): carve the song session out of app.js — playSong, showScreen, closeCurrentSong (R3d)#921
Merged
Merged
Conversation
…creen, closeCurrentSong (R3d) 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>
Contributor
📝 WalkthroughWalkthroughThe PR moves player navigation, playback, wake-lock, autoplay, and auto-exit orchestration from ChangesSession lifecycle refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Plugin
participant App
participant Session
participant Highway
participant PlayerUI
Plugin->>App: invoke window.feedBack playback or navigation helper
App->>Session: call playSong or showScreen
Session->>PlayerUI: enter player screen and reset state
Session->>Highway: connect with song and arrangement parameters
Highway-->>Session: emit song:ready
Session->>PlayerUI: start count-in or playback
Highway-->>App: emit song:ended
Session->>App: schedule origin-screen return
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/session.js`:
- Around line 107-111: Update showScreen so it resolves and validates the target
element from id before removing the active class from any screens; if the target
is missing, return without changing the current UI. Only after validation should
it deactivate existing screens and activate the target.
- Around line 577-677: Update playSong to use an invocation-generation guard
that invalidates superseded calls when a newer playSong starts. After the 500ms
delay and before highway.init/connect, verify the invocation is still current
and return early when it is stale, preventing older calls from reconnecting or
emitting events into the newer session.
🪄 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: 9ab95c46-457b-458f-abb3-da11e5f67855
📒 Files selected for processing (6)
static/app.jsstatic/js/session.jstests/js/autoplay_exit.test.jstests/js/song_close.test.jstests/js/speed_reset.test.jstests/js/tuner_auto_open.test.js
Comment on lines
+107
to
+111
| export async function showScreen(id) { | ||
| // Capture the previous screen before changing active classes | ||
| const prevScreenId = document.querySelector('.screen.active')?.id; | ||
| document.querySelectorAll('.screen').forEach(s => s.classList.remove('active')); | ||
| document.getElementById(id).classList.add('active'); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate the target before clearing the active screen.
Line 110 deactivates every screen before Line 111 dereferences the target. An invalid or removed plugin screen ID therefore throws and leaves the UI blank.
Proposed fix
export async function showScreen(id) {
+ const nextScreen = document.getElementById(id);
+ if (!nextScreen) throw new Error(`Unknown screen: ${id}`);
const prevScreenId = document.querySelector('.screen.active')?.id;
document.querySelectorAll('.screen').forEach(s => s.classList.remove('active'));
- document.getElementById(id).classList.add('active');
+ nextScreen.classList.add('active');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export async function showScreen(id) { | |
| // Capture the previous screen before changing active classes | |
| const prevScreenId = document.querySelector('.screen.active')?.id; | |
| document.querySelectorAll('.screen').forEach(s => s.classList.remove('active')); | |
| document.getElementById(id).classList.add('active'); | |
| export async function showScreen(id) { | |
| const nextScreen = document.getElementById(id); | |
| if (!nextScreen) throw new Error(`Unknown screen: ${id}`); | |
| // Capture the previous screen before changing active classes | |
| const prevScreenId = document.querySelector('.screen.active')?.id; | |
| document.querySelectorAll('.screen').forEach(s => s.classList.remove('active')); | |
| nextScreen.classList.add('active'); |
🤖 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/session.js` around lines 107 - 111, Update showScreen so it
resolves and validates the target element from id before removing the active
class from any screens; if the target is missing, return without changing the
current UI. Only after validation should it deactivate existing screens and
activate the target.
Comment on lines
+577
to
+677
| export async function playSong(filename, arrangement, options) { | ||
| console.log('playSong called:', filename); | ||
| // A manual (non-queue) play abandons any active play-queue, so a stale queue | ||
| // can't hijack the next song's end. The queue passes fromQueue to keep itself. | ||
| if ((!options || !options.fromQueue) && window.feedBack && window.feedBack.playQueue) { | ||
| window.feedBack.playQueue.clear(); | ||
| } | ||
| if (!options || options.bridge !== false) { | ||
| _recordPlaybackBridge('playback.window-play-song', 'window.playSong', 'legacy playSong entry point used'); | ||
| } | ||
| // Invalidate any prior song's autoplay gate before plugins re-claim it on the | ||
| // song:loading emit below. | ||
| _clearAutoplayHold(); | ||
| window.feedBack.emit('song:loading', { filename, arrangement: arrangement ?? null }); | ||
|
|
||
| // Cancel any pending art/metadata requests | ||
| if (artAbortController) artAbortController.abort(); | ||
| artAbortController = null; | ||
|
|
||
| window.highway.stop(); | ||
| // Cancel any active count-in: clear timers/RAF and bump the gen so | ||
| // delayed callbacks (rewind frames, post-seek then, count-in ticks, | ||
| // post-count play) bail before mutating the new session. | ||
| _cancelCountIn(); | ||
| // Reset the JUCE shim BEFORE awaiting jucePlayer.stop() so any in-flight | ||
| // shim closures see a stale generation after their await and bail out | ||
| // before mutating isPlaying / button label / song:* events for the | ||
| // outgoing song. | ||
| _resetJuceAudioShimChain(); | ||
| // Cancel queued _audioSeek calls from the previous song: bumping the | ||
| // generation makes their chained callbacks bail out. | ||
| _resetAudioSeekState(); | ||
| if (window._juceMode) { | ||
| // Mirror the showScreen teardown: emit song:pause for the JUCE | ||
| // path so plugins don't see a stale "playing" state on song | ||
| // change. (HTML5 fires it via the audio element 'pause' event.) | ||
| // Snapshot payload BEFORE stop() resets _pos so audioT/chartT | ||
| // capture the actual paused position. | ||
| const payload = _songEventPayload(); | ||
| const wasPlaying = S.isPlaying; | ||
| await jucePlayer.stop().catch(() => {}); | ||
| if (wasPlaying && window.feedBack) { | ||
| window.feedBack.isPlaying = false; | ||
| window.feedBack.emit('song:pause', payload); | ||
| } | ||
| window._juceMode = false; | ||
| window._juceAudioUrl = null; | ||
| } | ||
| audio.pause(); | ||
| audio.src = ''; | ||
| // Stale until the incoming song's WS handler (window.highway.js) sets it again. | ||
| window._currentSongAudio = null; | ||
| // Fresh JUCE routing attempt for whatever song loads next. | ||
| window._clearJuceRerouteMemo?.(); | ||
| S.isPlaying = false; | ||
| setPlayButtonState(false); | ||
| _resetPlaybackSpeedForNewSong(); | ||
| clearLoop(); | ||
| _resetSectionPracticeLog(); | ||
| _hideSectionPracticeBar(); | ||
| // Reset so the jump-fix (setInterval, ~line 8979) doesn't mistake the new | ||
| // song starting at t=0 for an unexpected seek from the previous song's | ||
| // position. audio.currentTime may not reset synchronously when src is cleared. | ||
| S.lastAudioTime = 0; | ||
|
|
||
| currentFilename = filename; | ||
| // A fresh load arms autoplay; a pending auto-exit from the previous | ||
| // song is no longer relevant. A *resume* load (options.resume) instead | ||
| // arms _pendingResume — consumed at song:ready to restore speed + seek to | ||
| // the saved position, then start — so autostart and resume don't both try | ||
| // to begin playback from different positions. | ||
| if (options && options.resume && Number(options.resume.position) > 0) { | ||
| S.pendingResume = options.resume; | ||
| _pendingAutostart = false; | ||
| } else { | ||
| S.pendingResume = null; | ||
| _pendingAutostart = true; | ||
| } | ||
| _clearAutoExit(); | ||
| // Remember which screen the player was launched from so Esc / | ||
| // navigation back from the player (and auto-exit) returns the user | ||
| // there (feedBack#126). | ||
| _playerOriginScreen = _resolvePlayerOrigin(); | ||
| showScreen('player'); | ||
|
|
||
| // Wait for previous WebSocket to fully close before opening new one | ||
| await new Promise(r => setTimeout(r, 500)); | ||
| window.highway.init(document.getElementById('highway')); | ||
|
|
||
| const wsParams = new URLSearchParams(); | ||
| if (arrangement !== undefined) wsParams.set('arrangement', arrangement); | ||
| wsParams.set('naming_mode', _getArrangementNamingMode()); | ||
| const wsUrl = `${location.protocol === 'https:' ? 'wss:' : 'ws:'}//${location.host}/ws/highway/${decodeURIComponent(filename)}?${wsParams.toString()}`; | ||
| window.highway.connect(wsUrl); | ||
| _resetSectionPracticeLog(); | ||
| _scheduleSectionPracticeRetries(); | ||
| loadSavedLoops(); | ||
| document.getElementById('quality-select').value = window.highway.getRenderScale(); | ||
| const _minScaleSel = document.getElementById('min-scale-select'); | ||
| if (_minScaleSel && window.highway.getMinRenderScale) _minScaleSel.value = String(window.highway.getMinRenderScale()); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Cancel superseded playSong invocations before reconnecting.
Line 663 leaves every invocation alive across the fixed delay. A rapid second selection cannot invalidate the first, allowing the stale call to reconnect the previous song and emit events into the newer session.
Proposed generation guard
export let artAbortController = null;
+let _playSongGen = 0;
export async function playSong(filename, arrangement, options) {
+ const playGen = ++_playSongGen;
console.log('playSong called:', filename);
@@
await jucePlayer.stop().catch(() => {});
+ if (playGen !== _playSongGen) return;
@@
await new Promise(r => setTimeout(r, 500));
+ if (playGen !== _playSongGen) return;
window.highway.init(document.getElementById('highway'));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export async function playSong(filename, arrangement, options) { | |
| console.log('playSong called:', filename); | |
| // A manual (non-queue) play abandons any active play-queue, so a stale queue | |
| // can't hijack the next song's end. The queue passes fromQueue to keep itself. | |
| if ((!options || !options.fromQueue) && window.feedBack && window.feedBack.playQueue) { | |
| window.feedBack.playQueue.clear(); | |
| } | |
| if (!options || options.bridge !== false) { | |
| _recordPlaybackBridge('playback.window-play-song', 'window.playSong', 'legacy playSong entry point used'); | |
| } | |
| // Invalidate any prior song's autoplay gate before plugins re-claim it on the | |
| // song:loading emit below. | |
| _clearAutoplayHold(); | |
| window.feedBack.emit('song:loading', { filename, arrangement: arrangement ?? null }); | |
| // Cancel any pending art/metadata requests | |
| if (artAbortController) artAbortController.abort(); | |
| artAbortController = null; | |
| window.highway.stop(); | |
| // Cancel any active count-in: clear timers/RAF and bump the gen so | |
| // delayed callbacks (rewind frames, post-seek then, count-in ticks, | |
| // post-count play) bail before mutating the new session. | |
| _cancelCountIn(); | |
| // Reset the JUCE shim BEFORE awaiting jucePlayer.stop() so any in-flight | |
| // shim closures see a stale generation after their await and bail out | |
| // before mutating isPlaying / button label / song:* events for the | |
| // outgoing song. | |
| _resetJuceAudioShimChain(); | |
| // Cancel queued _audioSeek calls from the previous song: bumping the | |
| // generation makes their chained callbacks bail out. | |
| _resetAudioSeekState(); | |
| if (window._juceMode) { | |
| // Mirror the showScreen teardown: emit song:pause for the JUCE | |
| // path so plugins don't see a stale "playing" state on song | |
| // change. (HTML5 fires it via the audio element 'pause' event.) | |
| // Snapshot payload BEFORE stop() resets _pos so audioT/chartT | |
| // capture the actual paused position. | |
| const payload = _songEventPayload(); | |
| const wasPlaying = S.isPlaying; | |
| await jucePlayer.stop().catch(() => {}); | |
| if (wasPlaying && window.feedBack) { | |
| window.feedBack.isPlaying = false; | |
| window.feedBack.emit('song:pause', payload); | |
| } | |
| window._juceMode = false; | |
| window._juceAudioUrl = null; | |
| } | |
| audio.pause(); | |
| audio.src = ''; | |
| // Stale until the incoming song's WS handler (window.highway.js) sets it again. | |
| window._currentSongAudio = null; | |
| // Fresh JUCE routing attempt for whatever song loads next. | |
| window._clearJuceRerouteMemo?.(); | |
| S.isPlaying = false; | |
| setPlayButtonState(false); | |
| _resetPlaybackSpeedForNewSong(); | |
| clearLoop(); | |
| _resetSectionPracticeLog(); | |
| _hideSectionPracticeBar(); | |
| // Reset so the jump-fix (setInterval, ~line 8979) doesn't mistake the new | |
| // song starting at t=0 for an unexpected seek from the previous song's | |
| // position. audio.currentTime may not reset synchronously when src is cleared. | |
| S.lastAudioTime = 0; | |
| currentFilename = filename; | |
| // A fresh load arms autoplay; a pending auto-exit from the previous | |
| // song is no longer relevant. A *resume* load (options.resume) instead | |
| // arms _pendingResume — consumed at song:ready to restore speed + seek to | |
| // the saved position, then start — so autostart and resume don't both try | |
| // to begin playback from different positions. | |
| if (options && options.resume && Number(options.resume.position) > 0) { | |
| S.pendingResume = options.resume; | |
| _pendingAutostart = false; | |
| } else { | |
| S.pendingResume = null; | |
| _pendingAutostart = true; | |
| } | |
| _clearAutoExit(); | |
| // Remember which screen the player was launched from so Esc / | |
| // navigation back from the player (and auto-exit) returns the user | |
| // there (feedBack#126). | |
| _playerOriginScreen = _resolvePlayerOrigin(); | |
| showScreen('player'); | |
| // Wait for previous WebSocket to fully close before opening new one | |
| await new Promise(r => setTimeout(r, 500)); | |
| window.highway.init(document.getElementById('highway')); | |
| const wsParams = new URLSearchParams(); | |
| if (arrangement !== undefined) wsParams.set('arrangement', arrangement); | |
| wsParams.set('naming_mode', _getArrangementNamingMode()); | |
| const wsUrl = `${location.protocol === 'https:' ? 'wss:' : 'ws:'}//${location.host}/ws/highway/${decodeURIComponent(filename)}?${wsParams.toString()}`; | |
| window.highway.connect(wsUrl); | |
| _resetSectionPracticeLog(); | |
| _scheduleSectionPracticeRetries(); | |
| loadSavedLoops(); | |
| document.getElementById('quality-select').value = window.highway.getRenderScale(); | |
| const _minScaleSel = document.getElementById('min-scale-select'); | |
| if (_minScaleSel && window.highway.getMinRenderScale) _minScaleSel.value = String(window.highway.getMinRenderScale()); | |
| } | |
| let _playSongGen = 0; | |
| export async function playSong(filename, arrangement, options) { | |
| const playGen = ++_playSongGen; | |
| console.log('playSong called:', filename); | |
| // A manual (non-queue) play abandons any active play-queue, so a stale queue | |
| // can't hijack the next song's end. The queue passes fromQueue to keep itself. | |
| if ((!options || !options.fromQueue) && window.feedBack && window.feedBack.playQueue) { | |
| window.feedBack.playQueue.clear(); | |
| } | |
| if (!options || options.bridge !== false) { | |
| _recordPlaybackBridge('playback.window-play-song', 'window.playSong', 'legacy playSong entry point used'); | |
| } | |
| // Invalidate any prior song's autoplay gate before plugins re-claim it on the | |
| // song:loading emit below. | |
| _clearAutoplayHold(); | |
| window.feedBack.emit('song:loading', { filename, arrangement: arrangement ?? null }); | |
| // Cancel any pending art/metadata requests | |
| if (artAbortController) artAbortController.abort(); | |
| artAbortController = null; | |
| window.highway.stop(); | |
| // Cancel any active count-in: clear timers/RAF and bump the gen so | |
| // delayed callbacks (rewind frames, post-seek then, count-in ticks, | |
| // post-count play) bail before mutating the new session. | |
| _cancelCountIn(); | |
| // Reset the JUCE shim BEFORE awaiting jucePlayer.stop() so any in-flight | |
| // shim closures see a stale generation after their await and bail out | |
| // before mutating isPlaying / button label / song:* events for the | |
| // outgoing song. | |
| _resetJuceAudioShimChain(); | |
| // Cancel queued _audioSeek calls from the previous song: bumping the | |
| // generation makes their chained callbacks bail out. | |
| _resetAudioSeekState(); | |
| if (window._juceMode) { | |
| // Mirror the showScreen teardown: emit song:pause for the JUCE | |
| // path so plugins don't see a stale "playing" state on song | |
| // change. (HTML5 fires it via the audio element 'pause' event.) | |
| // Snapshot payload BEFORE stop() resets _pos so audioT/chartT | |
| // capture the actual paused position. | |
| const payload = _songEventPayload(); | |
| const wasPlaying = S.isPlaying; | |
| await jucePlayer.stop().catch(() => {}); | |
| if (playGen !== _playSongGen) return; | |
| if (wasPlaying && window.feedBack) { | |
| window.feedBack.isPlaying = false; | |
| window.feedBack.emit('song:pause', payload); | |
| } | |
| window._juceMode = false; | |
| window._juceAudioUrl = null; | |
| } | |
| audio.pause(); | |
| audio.src = ''; | |
| // Stale until the incoming song's WS handler (window.highway.js) sets it again. | |
| window._currentSongAudio = null; | |
| // Fresh JUCE routing attempt for whatever song loads next. | |
| window._clearJuceRerouteMemo?.(); | |
| S.isPlaying = false; | |
| setPlayButtonState(false); | |
| _resetPlaybackSpeedForNewSong(); | |
| clearLoop(); | |
| _resetSectionPracticeLog(); | |
| _hideSectionPracticeBar(); | |
| // Reset so the jump-fix (setInterval, ~line 8979) doesn't mistake the new | |
| // song starting at t=0 for an unexpected seek from the previous song's | |
| // position. audio.currentTime may not reset synchronously when src is cleared. | |
| S.lastAudioTime = 0; | |
| currentFilename = filename; | |
| // A fresh load arms autoplay; a pending auto-exit from the previous | |
| // song is no longer relevant. A *resume* load (options.resume) instead | |
| // arms _pendingResume — consumed at song:ready to restore speed + seek to | |
| // the saved position, then start — so autostart and resume don't both try | |
| // to begin playback from different positions. | |
| if (options && options.resume && Number(options.resume.position) > 0) { | |
| S.pendingResume = options.resume; | |
| _pendingAutostart = false; | |
| } else { | |
| S.pendingResume = null; | |
| _pendingAutostart = true; | |
| } | |
| _clearAutoExit(); | |
| // Remember which screen the player was launched from so Esc / | |
| // navigation back from the player (and auto-exit) returns the user | |
| // there (feedBack#126). | |
| _playerOriginScreen = _resolvePlayerOrigin(); | |
| showScreen('player'); | |
| // Wait for previous WebSocket to fully close before opening new one | |
| await new Promise(r => setTimeout(r, 500)); | |
| if (playGen !== _playSongGen) return; | |
| window.highway.init(document.getElementById('highway')); | |
| const wsParams = new URLSearchParams(); | |
| if (arrangement !== undefined) wsParams.set('arrangement', arrangement); | |
| wsParams.set('naming_mode', _getArrangementNamingMode()); | |
| const wsUrl = `${location.protocol === 'https:' ? 'wss:' : 'ws:'}//${location.host}/ws/highway/${decodeURIComponent(filename)}?${wsParams.toString()}`; | |
| window.highway.connect(wsUrl); | |
| _resetSectionPracticeLog(); | |
| _scheduleSectionPracticeRetries(); | |
| loadSavedLoops(); | |
| document.getElementById('quality-select').value = window.highway.getRenderScale(); | |
| const _minScaleSel = document.getElementById('min-scale-select'); | |
| if (_minScaleSel && window.highway.getMinRenderScale) _minScaleSel.value = String(window.highway.getMinRenderScale()); | |
| } |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 662-662: Avoid using the initial state variable in setState
Context: setTimeout(r, 500)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
[error] 631-631: React's useState should not be directly called
Context: setPlayButtonState(false)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(usestate-direct-usage)
🤖 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/session.js` around lines 577 - 677, Update playSong to use an
invocation-generation guard that invalidates superseded calls when a newer
playSong starts. After the 500ms delay and before highway.init/connect, verify
the invocation is still current and return early when it is stale, preventing
older calls from reconnecting or emitting events into the newer session.
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.
36 declarations + the 4 autoplay/auto-exit gate statements. 359 lines.
app.js3,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.
playSongandshowScreencalled 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
The first cut passed every gate —
no-undefclean,no-cycleclean, 1045/1045, pytest green — and threw in the browser:window.feedBack.holdAutoplay/holdAutoExitand 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 of them 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.
Verification
A/B against
origin/mainin two browsers — identical, zero page errors — including the autoplay gate driven end to end:That is the exact machinery that was throwing.
node 1045 · pytest 2425 · ESLint 0 (no-cycle clean) · host contract 2/2 · Codex 0.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests