refactor(app): carve resume-session out of app.js (R3a)#892
Open
byrongamatos wants to merge 1 commit into
Open
refactor(app): carve resume-session out of app.js (R3a)#892byrongamatos wants to merge 1 commit into
byrongamatos wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughResume-session persistence and resume-pill UI are extracted into a dedicated module. Playback now shares pending resume data through ChangesResume session integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PlayerScreen
participant ResumeSession as resume-session.js
participant Storage as localStorage
participant Host as host.js
PlayerScreen->>ResumeSession: show resume pill
ResumeSession->>Storage: read and validate snapshot
PlayerScreen->>ResumeSession: click Resume
ResumeSession->>Host: call playSong with resume payload
Host->>Storage: clear snapshot after success
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 |
![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: 3
🧹 Nitpick comments (5)
static/js/count-in.js (1)
376-379: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePotential stale-listener accumulation if
armCreditsHideOnPlay()fires twice before asong:play.Each call overwrites
_creditsHideOnPlaywith a new closure, but the previously-registered{once:true}listener onwindow.feedBackis never removed (only the latest reference can be.off()'d viahideSongCreditsOverlay). In practice each stale listener is harmless (it self-removes after firing once and just re-hides an already-hidden overlay), but it's worth a defensive dedupe ifarmCreditsHideOnPlaycan realistically be invoked back-to-back without an interveninghideSongCreditsOverlay().🤖 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/count-in.js` around lines 376 - 379, Update armCreditsHideOnPlay to remove any previously registered song:play listener before replacing _creditsHideOnPlay and registering the new { once: true } listener. Preserve the existing callback behavior and hideSongCreditsOverlay cleanup.static/js/resume-session.js (1)
62-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMalformed snapshot data is silently ignored but not cleared.
The stale-timestamp branch (Line 68) clears storage via
_clearResumeSession(), but the malformed-shape branch (Line 67, missing.f/.t) does not. Any corrupted/legacy snapshot will keep returningnullon every check without ever being cleaned up fromlocalStorage.♻️ Proposed fix
const snap = JSON.parse(raw); - if (!snap || !snap.f || !(Number(snap.t) > 0)) return null; + if (!snap || !snap.f || !(Number(snap.t) > 0)) { _clearResumeSession(); return null; } if (!snap.ts || Date.now() - snap.ts > _RESUME_MAX_AGE_MS) { _clearResumeSession(); return null; }🤖 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/resume-session.js` around lines 62 - 71, Update _readResumeSession so malformed snapshots failing the !snap, !snap.f, or invalid snap.t validation are cleared via _clearResumeSession() before returning null, matching the stale-timestamp branch; preserve the existing valid-snapshot return and error handling behavior.static/app.js (1)
5579-5591: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale comment references old
_pendingResumevariable.The comment still says "arms
_pendingResume" but the code below now writesS.pendingResume(per player-state.js). Update the comment to avoid confusing future readers about which container holds the resume request.✏️ Proposed comment fix
- // 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. + // 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. 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 + // arms S.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.🤖 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/app.js` around lines 5579 - 5591, Update the comment above the resume-handling logic to reference S.pendingResume instead of the obsolete _pendingResume variable, keeping the explanation consistent with the assignment and its consumption behavior.static/js/player-controls.js (1)
85-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant
handleSliderInputcall
#speed-slideralready has aninputpath throughsetSpeed(), andsetSpeed()callshost.handleSliderInput(speedSlider). The direct call here makes the same update run twice; the syntheticinputevent is enough.🤖 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/player-controls.js` around lines 85 - 96, Remove the direct host.handleSliderInput(slider) call from applySpeedPreset and retain the synthetic input event so the existing setSpeed() path performs the update exactly once.tests/js/song_restart.test.js (1)
55-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLocal
SinloadRestartis a snapshot, not a live reference tosandbox.S.
var S = { isPlaying: ${sandbox.S.isPlaying}, ... }captures the boolean at load time;__togglePlay()mutatessandbox.S.isPlaying, a separate object, sorestartCurrentSong's internalS.isPlayingwon't reflect that mutation. Current assertions don't exercise this (the function only readsS.isPlayingonce before any toggle), but it's worth noting this indirection could mask aS-consistency bug in future test additions.🤖 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 `@tests/js/song_restart.test.js` around lines 55 - 70, Update loadRestart so the generated restartCurrentSong function uses the live sandbox.S object rather than a copied S snapshot. Preserve the existing lastAudioTime initialization while ensuring __togglePlay mutations to sandbox.S.isPlaying are visible through S during execution.
🤖 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/loops.js`:
- Around line 182-261: Check response.ok immediately after each fetch in
loadSavedLoops, saveCurrentLoop, and deleteSelectedLoop, and handle non-OK
responses as failures before parsing JSON or updating UI state. Preserve the
existing UI updates only on successful requests: do not hide the Save button,
clear the loop, or refresh saved loops when the corresponding server operation
fails.
In `@static/js/resume-session.js`:
- Around line 78-96: Re-invoke _maybeShowResumePill() in the catch block of
resumeLastSession after preserving the failed snapshot, so the retry affordance
is immediately restored when host.playSong fails. Keep the existing failure
return and snapshot retention behavior unchanged.
In `@static/js/section-practice.js`:
- Around line 73-112: Reset _sectionPracticeActiveParent to its sentinel value
in both the off branch of _setSectionPracticeMode and the exported
resetSelection function, alongside the existing selection-state resets. Preserve
_sectionPracticeResetSelectionUi and _hideSectionPracticeBar behavior so
_updateSectionPracticeHighlight cannot retain a stale selected chip after
disabling practice or clearing a loop.
---
Nitpick comments:
In `@static/app.js`:
- Around line 5579-5591: Update the comment above the resume-handling logic to
reference S.pendingResume instead of the obsolete _pendingResume variable,
keeping the explanation consistent with the assignment and its consumption
behavior.
In `@static/js/count-in.js`:
- Around line 376-379: Update armCreditsHideOnPlay to remove any previously
registered song:play listener before replacing _creditsHideOnPlay and
registering the new { once: true } listener. Preserve the existing callback
behavior and hideSongCreditsOverlay cleanup.
In `@static/js/player-controls.js`:
- Around line 85-96: Remove the direct host.handleSliderInput(slider) call from
applySpeedPreset and retain the synthetic input event so the existing setSpeed()
path performs the update exactly once.
In `@static/js/resume-session.js`:
- Around line 62-71: Update _readResumeSession so malformed snapshots failing
the !snap, !snap.f, or invalid snap.t validation are cleared via
_clearResumeSession() before returning null, matching the stale-timestamp
branch; preserve the existing valid-snapshot return and error handling behavior.
In `@tests/js/song_restart.test.js`:
- Around line 55-70: Update loadRestart so the generated restartCurrentSong
function uses the live sandbox.S object rather than a copied S snapshot.
Preserve the existing lastAudioTime initialization while ensuring __togglePlay
mutations to sandbox.S.isPlaying are visible through S during execution.
🪄 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: a94b021c-3ed2-4704-a5a6-d26cb2fbbd9b
📒 Files selected for processing (21)
static/app.jsstatic/js/count-in.jsstatic/js/host.jsstatic/js/loops.jsstatic/js/player-controls.jsstatic/js/player-state.jsstatic/js/resume-session.jsstatic/js/section-practice.jstests/js/autoplay_exit.test.jstests/js/host_contract.test.jstests/js/juce_engine_reroute.test.jstests/js/loop_api.test.jstests/js/loop_restart.test.jstests/js/play_button_reroute_guard.test.jstests/js/playback_app_adapter.test.jstests/js/section_practice_dismiss.test.jstests/js/song_credits_overlay.test.jstests/js/song_restart.test.jstests/js/song_seek.test.jstests/js/speed_reset.test.jstests/test_plugin_runtime_idempotence.py
Comment on lines
+182
to
+261
| export async function loadSavedLoops() { | ||
| const sel = document.getElementById('saved-loops'); | ||
| const delBtn = document.getElementById('btn-loop-delete'); | ||
| if (!host.currentFilename()) { sel.classList.add('hidden'); delBtn.classList.add('hidden'); return; } | ||
|
|
||
| const resp = await fetch(`/api/loops?filename=${encodeURIComponent(decodeURIComponent(host.currentFilename()))}`); | ||
| const loops = await resp.json(); | ||
|
|
||
| sel.innerHTML = '<option value="">Saved Loops</option>'; | ||
| for (const l of loops) { | ||
| sel.innerHTML += `<option value="${l.id}" data-start="${l.start}" data-end="${l.end}">${esc(l.name)} (${host.formatTime(l.start)}→${host.formatTime(l.end)})</option>`; | ||
| } | ||
| if (loops.length > 0) { | ||
| sel.classList.remove('hidden'); | ||
| } else { | ||
| sel.classList.add('hidden'); | ||
| } | ||
| delBtn.classList.add('hidden'); | ||
| } | ||
|
|
||
| export async function loadSavedLoop(loopId) { | ||
| const sel = document.getElementById('saved-loops'); | ||
| const opt = sel.selectedOptions[0]; | ||
| const delBtn = document.getElementById('btn-loop-delete'); | ||
| if (!loopId || !opt?.dataset.start) { | ||
| delBtn.classList.add('hidden'); | ||
| return; | ||
| } | ||
| let ok = false; | ||
| try { | ||
| // Pass raw strings — setLoop's Number() coercion is stricter than | ||
| // parseFloat (rejects "12abc") so malformed dataset values throw | ||
| // and fall into the catch instead of silently truncating. | ||
| ok = await setLoop(opt.dataset.start, opt.dataset.end); | ||
| } catch (err) { | ||
| // Malformed dataset (server returned bad data): treat the same as | ||
| // a failed seek so the dropdown resyncs and we don't propagate an | ||
| // uncaught rejection out of the onchange handler. | ||
| console.warn('[loadSavedLoop] setLoop threw:', err); | ||
| ok = false; | ||
| } | ||
| if (!ok) { | ||
| // Seek aborted, landed off-target, or input was malformed. | ||
| // Resync the dropdown with the still-active loop so the UI | ||
| // doesn't lie about which loop is loaded. | ||
| _syncSavedLoopSelection(); | ||
| return; | ||
| } | ||
| // Success path: setLoop already called _syncSavedLoopSelection, | ||
| // which surfaces the delete button when the new loop matches a | ||
| // saved option (which the dropdown selection guarantees here). | ||
| } | ||
|
|
||
| export async function saveCurrentLoop() { | ||
| if (loopA === null || loopB === null || !host.currentFilename()) return; | ||
| const name = await uiPrompt({ title: 'Save Loop', label: 'Loop name', value: 'Loop', okLabel: 'Save' }); | ||
| if (name === null) return; // cancelled | ||
| const finalName = name.trim() || 'Loop'; // never persist an empty name | ||
| await fetch('/api/loops', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ | ||
| filename: decodeURIComponent(host.currentFilename()), | ||
| name: finalName, | ||
| start: loopA, | ||
| end: loopB, | ||
| }), | ||
| }); | ||
| await loadSavedLoops(); | ||
| document.getElementById('btn-loop-save').classList.add('hidden'); | ||
| } | ||
|
|
||
| export async function deleteSelectedLoop() { | ||
| const sel = document.getElementById('saved-loops'); | ||
| const loopId = sel.value; | ||
| if (!loopId) return; | ||
| await fetch(`/api/loops/${loopId}`, { method: 'DELETE' }); | ||
| clearLoop(); | ||
| await loadSavedLoops(); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fetch calls never check response.ok.
loadSavedLoops, saveCurrentLoop, and deleteSelectedLoop all call fetch() without checking resp.ok. An HTTP error (500/404/etc.) doesn't throw — execution continues as if the write/delete succeeded: saveCurrentLoop still hides the Save button, deleteSelectedLoop still calls clearLoop(), and loadSavedLoops will call resp.json() on a non-JSON error body, which can throw and silently abort whatever awaited it (e.g. cutting saveCurrentLoop short before it hides the Save button, or deleteSelectedLoop before the dropdown refresh). The UI ends up out of sync with what actually persisted server-side.
🛡️ Suggested fix
export async function loadSavedLoops() {
const sel = document.getElementById('saved-loops');
const delBtn = document.getElementById('btn-loop-delete');
if (!host.currentFilename()) { sel.classList.add('hidden'); delBtn.classList.add('hidden'); return; }
const resp = await fetch(`/api/loops?filename=${encodeURIComponent(decodeURIComponent(host.currentFilename()))}`);
+ if (!resp.ok) {
+ console.error('[loadSavedLoops] request failed:', resp.status);
+ sel.classList.add('hidden');
+ delBtn.classList.add('hidden');
+ return;
+ }
const loops = await resp.json();
...
export async function saveCurrentLoop() {
...
- await fetch('/api/loops', {
+ const resp = await fetch('/api/loops', {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ ... }),
});
+ if (!resp.ok) {
+ console.error('[saveCurrentLoop] save failed:', resp.status);
+ return;
+ }
await loadSavedLoops();
...
export async function deleteSelectedLoop() {
...
- await fetch(`/api/loops/${loopId}`, { method: 'DELETE' });
- clearLoop();
+ const resp = await fetch(`/api/loops/${loopId}`, { method: 'DELETE' });
+ if (!resp.ok) {
+ console.error('[deleteSelectedLoop] delete failed:', resp.status);
+ return;
+ }
+ clearLoop();
await loadSavedLoops();
}📝 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 loadSavedLoops() { | |
| const sel = document.getElementById('saved-loops'); | |
| const delBtn = document.getElementById('btn-loop-delete'); | |
| if (!host.currentFilename()) { sel.classList.add('hidden'); delBtn.classList.add('hidden'); return; } | |
| const resp = await fetch(`/api/loops?filename=${encodeURIComponent(decodeURIComponent(host.currentFilename()))}`); | |
| const loops = await resp.json(); | |
| sel.innerHTML = '<option value="">Saved Loops</option>'; | |
| for (const l of loops) { | |
| sel.innerHTML += `<option value="${l.id}" data-start="${l.start}" data-end="${l.end}">${esc(l.name)} (${host.formatTime(l.start)}→${host.formatTime(l.end)})</option>`; | |
| } | |
| if (loops.length > 0) { | |
| sel.classList.remove('hidden'); | |
| } else { | |
| sel.classList.add('hidden'); | |
| } | |
| delBtn.classList.add('hidden'); | |
| } | |
| export async function loadSavedLoop(loopId) { | |
| const sel = document.getElementById('saved-loops'); | |
| const opt = sel.selectedOptions[0]; | |
| const delBtn = document.getElementById('btn-loop-delete'); | |
| if (!loopId || !opt?.dataset.start) { | |
| delBtn.classList.add('hidden'); | |
| return; | |
| } | |
| let ok = false; | |
| try { | |
| // Pass raw strings — setLoop's Number() coercion is stricter than | |
| // parseFloat (rejects "12abc") so malformed dataset values throw | |
| // and fall into the catch instead of silently truncating. | |
| ok = await setLoop(opt.dataset.start, opt.dataset.end); | |
| } catch (err) { | |
| // Malformed dataset (server returned bad data): treat the same as | |
| // a failed seek so the dropdown resyncs and we don't propagate an | |
| // uncaught rejection out of the onchange handler. | |
| console.warn('[loadSavedLoop] setLoop threw:', err); | |
| ok = false; | |
| } | |
| if (!ok) { | |
| // Seek aborted, landed off-target, or input was malformed. | |
| // Resync the dropdown with the still-active loop so the UI | |
| // doesn't lie about which loop is loaded. | |
| _syncSavedLoopSelection(); | |
| return; | |
| } | |
| // Success path: setLoop already called _syncSavedLoopSelection, | |
| // which surfaces the delete button when the new loop matches a | |
| // saved option (which the dropdown selection guarantees here). | |
| } | |
| export async function saveCurrentLoop() { | |
| if (loopA === null || loopB === null || !host.currentFilename()) return; | |
| const name = await uiPrompt({ title: 'Save Loop', label: 'Loop name', value: 'Loop', okLabel: 'Save' }); | |
| if (name === null) return; // cancelled | |
| const finalName = name.trim() || 'Loop'; // never persist an empty name | |
| await fetch('/api/loops', { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify({ | |
| filename: decodeURIComponent(host.currentFilename()), | |
| name: finalName, | |
| start: loopA, | |
| end: loopB, | |
| }), | |
| }); | |
| await loadSavedLoops(); | |
| document.getElementById('btn-loop-save').classList.add('hidden'); | |
| } | |
| export async function deleteSelectedLoop() { | |
| const sel = document.getElementById('saved-loops'); | |
| const loopId = sel.value; | |
| if (!loopId) return; | |
| await fetch(`/api/loops/${loopId}`, { method: 'DELETE' }); | |
| clearLoop(); | |
| await loadSavedLoops(); | |
| } | |
| export async function loadSavedLoops() { | |
| const sel = document.getElementById('saved-loops'); | |
| const delBtn = document.getElementById('btn-loop-delete'); | |
| if (!host.currentFilename()) { sel.classList.add('hidden'); delBtn.classList.add('hidden'); return; } | |
| const resp = await fetch(`/api/loops?filename=${encodeURIComponent(decodeURIComponent(host.currentFilename()))}`); | |
| if (!resp.ok) { | |
| console.error('[loadSavedLoops] request failed:', resp.status); | |
| sel.classList.add('hidden'); | |
| delBtn.classList.add('hidden'); | |
| return; | |
| } | |
| const loops = await resp.json(); | |
| sel.innerHTML = '<option value="">Saved Loops</option>'; | |
| for (const l of loops) { | |
| sel.innerHTML += `<option value="${l.id}" data-start="${l.start}" data-end="${l.end}">${esc(l.name)} (${host.formatTime(l.start)}→${host.formatTime(l.end)})</option>`; | |
| } | |
| if (loops.length > 0) { | |
| sel.classList.remove('hidden'); | |
| } else { | |
| sel.classList.add('hidden'); | |
| } | |
| delBtn.classList.add('hidden'); | |
| } | |
| export async function loadSavedLoop(loopId) { | |
| const sel = document.getElementById('saved-loops'); | |
| const opt = sel.selectedOptions[0]; | |
| const delBtn = document.getElementById('btn-loop-delete'); | |
| if (!loopId || !opt?.dataset.start) { | |
| delBtn.classList.add('hidden'); | |
| return; | |
| } | |
| let ok = false; | |
| try { | |
| // Pass raw strings — setLoop's Number() coercion is stricter than | |
| // parseFloat (rejects "12abc") so malformed dataset values throw | |
| // and fall into the catch instead of silently truncating. | |
| ok = await setLoop(opt.dataset.start, opt.dataset.end); | |
| } catch (err) { | |
| // Malformed dataset (server returned bad data): treat the same as | |
| // a failed seek so the dropdown resyncs and we don't propagate an | |
| // uncaught rejection out of the onchange handler. | |
| console.warn('[loadSavedLoop] setLoop threw:', err); | |
| ok = false; | |
| } | |
| if (!ok) { | |
| // Seek aborted, landed off-target, or input was malformed. | |
| // Resync the dropdown with the still-active loop so the UI | |
| // doesn't lie about which loop is loaded. | |
| _syncSavedLoopSelection(); | |
| return; | |
| } | |
| // Success path: setLoop already called _syncSavedLoopSelection, | |
| // which surfaces the delete button when the new loop matches a | |
| // saved option (which the dropdown selection guarantees here). | |
| } | |
| export async function saveCurrentLoop() { | |
| if (loopA === null || loopB === null || !host.currentFilename()) return; | |
| const name = await uiPrompt({ title: 'Save Loop', label: 'Loop name', value: 'Loop', okLabel: 'Save' }); | |
| if (name === null) return; // cancelled | |
| const finalName = name.trim() || 'Loop'; // never persist an empty name | |
| const resp = await fetch('/api/loops', { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify({ | |
| filename: decodeURIComponent(host.currentFilename()), | |
| name: finalName, | |
| start: loopA, | |
| end: loopB, | |
| }), | |
| }); | |
| if (!resp.ok) { | |
| console.error('[saveCurrentLoop] save failed:', resp.status); | |
| return; | |
| } | |
| await loadSavedLoops(); | |
| document.getElementById('btn-loop-save').classList.add('hidden'); | |
| } | |
| export async function deleteSelectedLoop() { | |
| const sel = document.getElementById('saved-loops'); | |
| const loopId = sel.value; | |
| if (!loopId) return; | |
| const resp = await fetch(`/api/loops/${loopId}`, { method: 'DELETE' }); | |
| if (!resp.ok) { | |
| console.error('[deleteSelectedLoop] delete failed:', resp.status); | |
| return; | |
| } | |
| clearLoop(); | |
| await loadSavedLoops(); | |
| } |
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 214-214: React's useState should not be directly called
Context: setLoop(opt.dataset.start, opt.dataset.end)
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/loops.js` around lines 182 - 261, Check response.ok immediately
after each fetch in loadSavedLoops, saveCurrentLoop, and deleteSelectedLoop, and
handle non-OK responses as failures before parsing JSON or updating UI state.
Preserve the existing UI updates only on successful requests: do not hide the
Save button, clear the loop, or refresh saved loops when the corresponding
server operation fails.
Comment on lines
+78
to
+96
| export async function resumeLastSession() { | ||
| const snap = _readResumeSession(); | ||
| if (!snap) { _hideResumePill(); return false; } | ||
| _hideResumePill(); | ||
| try { | ||
| await host.playSong(snap.f, snap.a, { | ||
| resume: { position: Number(snap.t) || 0, speed: Number(snap.sp) || 1 }, | ||
| }); | ||
| } catch (err) { | ||
| // A transient load/connect failure must not strand the user: keep the | ||
| // snapshot so the pill can re-offer it on the next non-player screen, | ||
| // rather than consuming the only copy before the song actually loaded. | ||
| console.warn('[app] resume failed to load; keeping snapshot:', err); | ||
| S.pendingResume = null; | ||
| return false; | ||
| } | ||
| _clearResumeSession(); // consumed only after a successful load | ||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Pill isn't re-offered after a failed resume attempt.
_hideResumePill() runs before host.playSong (Line 81). On failure (Line 86-93), the snapshot is kept but the pill is never shown again — only a console.warn is emitted. The user is left with no visible affordance to retry, and no indication anything went wrong, until some unrelated navigation happens to call _maybeShowResumePill().
Consider re-invoking _maybeShowResumePill() in the catch block so the pill reappears with the still-valid snapshot.
💡 Proposed fix
} catch (err) {
// A transient load/connect failure must not strand the user: keep the
// snapshot so the pill can re-offer it on the next non-player screen,
// rather than consuming the only copy before the song actually loaded.
console.warn('[app] resume failed to load; keeping snapshot:', err);
S.pendingResume = null;
+ _maybeShowResumePill();
return false;
}📝 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 resumeLastSession() { | |
| const snap = _readResumeSession(); | |
| if (!snap) { _hideResumePill(); return false; } | |
| _hideResumePill(); | |
| try { | |
| await host.playSong(snap.f, snap.a, { | |
| resume: { position: Number(snap.t) || 0, speed: Number(snap.sp) || 1 }, | |
| }); | |
| } catch (err) { | |
| // A transient load/connect failure must not strand the user: keep the | |
| // snapshot so the pill can re-offer it on the next non-player screen, | |
| // rather than consuming the only copy before the song actually loaded. | |
| console.warn('[app] resume failed to load; keeping snapshot:', err); | |
| S.pendingResume = null; | |
| return false; | |
| } | |
| _clearResumeSession(); // consumed only after a successful load | |
| return true; | |
| } | |
| export async function resumeLastSession() { | |
| const snap = _readResumeSession(); | |
| if (!snap) { _hideResumePill(); return false; } | |
| _hideResumePill(); | |
| try { | |
| await host.playSong(snap.f, snap.a, { | |
| resume: { position: Number(snap.t) || 0, speed: Number(snap.sp) || 1 }, | |
| }); | |
| } catch (err) { | |
| // A transient load/connect failure must not strand the user: keep the | |
| // snapshot so the pill can re-offer it on the next non-player screen, | |
| // rather than consuming the only copy before the song actually loaded. | |
| console.warn('[app] resume failed to load; keeping snapshot:', err); | |
| S.pendingResume = null; | |
| _maybeShowResumePill(); | |
| return false; | |
| } | |
| _clearResumeSession(); // consumed only after a successful load | |
| return true; | |
| } |
🤖 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/resume-session.js` around lines 78 - 96, Re-invoke
_maybeShowResumePill() in the catch block of resumeLastSession after preserving
the failed snapshot, so the retry affordance is immediately restored when
host.playSong fails. Keep the existing failure return and snapshot retention
behavior unchanged.
Comment on lines
+73
to
+112
| export function _setSectionPracticeMode(on, opts = {}) { | ||
| const next = !!on; | ||
| if (next === _sectionPracticeMode && !opts.force) return; | ||
| _sectionPracticeMode = next; | ||
| const cb = document.getElementById('section-practice-mode'); | ||
| if (cb) cb.checked = _sectionPracticeMode; | ||
| // Surface the "looping" state on the collapsed pill so the user can tell | ||
| // Section Practice is armed without opening the popover. | ||
| const pill = document.getElementById('section-practice-pill'); | ||
| if (pill) pill.classList.toggle('section-practice-pill--active', _sectionPracticeMode); | ||
| _sectionPracticeFollowParent = -1; | ||
| if (_sectionPracticeMode) { | ||
| if (opts.defaultWholeOn) { | ||
| _sectionPracticeWholeSection = true; | ||
| } | ||
| _updateSectionPracticeHighlight(host._audioTime()); | ||
| if (opts.defaultWholeOn) { | ||
| _syncSectionPracticePieceUi(); | ||
| } | ||
| } else { | ||
| // Turning the feature off must cancel any in-flight practiceSection() | ||
| // retry: otherwise a stale setLoop() that lands after the user unchecks | ||
| // Section Practice would re-arm the loop, flip the mode back on via | ||
| // _syncSectionPracticeFromLoop(), and restart playback through | ||
| // startCountIn(). Bumping the request gen makes the pending retry bail. | ||
| _sectionPracticeRequestGen++; | ||
| // Cancel any pending count-in: every section-practice teardown routes | ||
| // through here (mode toggle off, clearLoop, and _hideSectionPracticeBar | ||
| // on song/arrangement change), so a countdown started by a prior section | ||
| // click must not resume playback after the user has turned practice off. | ||
| host._cancelCountIn(); | ||
| _sectionPracticeSelected = -1; | ||
| _sectionPracticeWholeSection = false; | ||
| _sectionPracticeSavedPartIndex = 0; | ||
| _updateSectionPracticeHighlight(host._audioTime()); | ||
| if (!opts.skipClearLoop && (host.loopA() !== null || host.loopB() !== null)) { | ||
| host.clearLoop(); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Turning off practice mode never clears _sectionPracticeActiveParent, leaving a stale chip highlighted.
The off-branch (Lines 104-106) resets _sectionPracticeSelected/_sectionPracticeWholeSection/_sectionPracticeSavedPartIndex but not _sectionPracticeActiveParent. The exported resetSelection() (Lines 1201-1206, called by loops.js's clearLoop() at Line 74) has the identical gap. Contrast with _sectionPracticeResetSelectionUi() (Line 344-350) and _hideSectionPracticeBar() (Line 805), which both correctly reset it.
_updateSectionPracticeHighlight() toggles is-selected purely off _sectionPracticeActiveParent (Line 1159), unconditionally of practice mode — so after clearing a loop or unchecking "Practice Section", the previously active section chip stays visually marked selected indefinitely (until the user clicks another chip or a song/arrangement change forces the full _hideSectionPracticeBar() reset), even while the time-following is-playing highlight is simultaneously showing a different chip.
🐛 Suggested fix (both locations)
_sectionPracticeRequestGen++;
host._cancelCountIn();
_sectionPracticeSelected = -1;
_sectionPracticeWholeSection = false;
_sectionPracticeSavedPartIndex = 0;
+ _sectionPracticeActiveParent = -1;
_updateSectionPracticeHighlight(host._audioTime()); export function resetSelection() {
_sectionPracticeSelected = -1;
_sectionPracticeWholeSection = false;
_sectionPracticeSavedPartIndex = 0;
+ _sectionPracticeActiveParent = -1;
}🤖 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 73 - 112, Reset
_sectionPracticeActiveParent to its sentinel value in both the off branch of
_setSectionPracticeMode and the exported resetSelection function, alongside the
existing selection-state resets. Preserve _sectionPracticeResetSelectionUi and
_hideSectionPracticeBar behavior so _updateSectionPracticeHighlight cannot
retain a stale selected chip after disabling practice or clearing a loop.
static/js/resume-session.js (157) — the snapshot taken when you leave a song and the
pill that offers it back. Bodies VERBATIM. app.js 7,727 → 7,601.
Fifth slice out of the strongly-connected core. ONE hook (playSong) + a
currentFilename getter.
S.pendingResume JOINS THE CONTAINER — on demand, exactly as intended. app.js WRITES
it (playSong({ resume }) arms it; the song:ready listener consumes it) while this
module reads it, so it cannot be a plain export: an imported binding is read-only.
Same reason isPlaying is there. The container grows one field per carve that needs
it, never speculatively.
THE CONTRACT TEST CAUGHT THE MISSING HOOK, again on a path nothing executes:
"playSong is read by a module but never wired by app.js — it would throw at runtime".
Second time it has caught a real wiring gap the moment it appeared.
A REAL TRAP, worth remembering: I first did the S.pendingResume rewrite by feeding
acorn's identifier RANGES from node into python, and it corrupted the file
(`_pS.pendingResume null;`). **Acorn's offsets are UTF-16 code units; Python's string
indices are code points.** static/app.js contains emoji, so every offset past one
drifts. Do an AST-driven rewrite in the SAME language that produced the offsets.
`node --check` caught it; a silent version of that bug is very easy to imagine.
VERIFIED. A/B against origin/main in two browsers, real song: the window API
(resumeLastSession / _snapshotResumeSession / _readResumeSession /
_clearResumeSession), snapshot, read-back, and clear — IDENTICAL, zero page errors.
HONEST LIMIT: my probe never got the snapshot to actually PERSIST (there is a guard
beyond the 3s minimum position that a scripted playSong does not satisfy), so that
path is verified only as identical-to-main, not as observed-working. The real
coverage is tests/browser/resume-session.spec.ts, which drives the flow properly.
Zero harnesses broke. 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-carve-resume
branch
from
July 11, 2026 20:22
702eb7c to
1c6d2d0
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
static/app.js (1)
5580-5591: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate stale comment referencing
_pendingResume.The arming logic is correct, but the comment above still describes arming
_pendingResume, which has been moved to the shared container asS.pendingResume. Since this PR documents the shared-state lifecycle, aligning the comment avoids future confusion.📝 Proposed comment update
// 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 + // arms S.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.🤖 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/app.js` around lines 5580 - 5591, Update the comment above the autoplay/resume arming logic to reference the shared state property S.pendingResume instead of _pendingResume, while preserving its existing explanation of resume consumption and playback behavior.
🤖 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/app.js`:
- Around line 5580-5591: Update the comment above the autoplay/resume arming
logic to reference the shared state property S.pendingResume instead of
_pendingResume, while preserving its existing explanation of resume consumption
and playback behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 730b590f-2a0f-4fef-8ccb-b3a53a7a255e
📒 Files selected for processing (3)
static/app.jsstatic/js/player-state.jsstatic/js/resume-session.js
✅ Files skipped from review due to trivial changes (1)
- static/js/player-state.js
🚧 Files skipped from review as they are similar to previous changes (1)
- static/js/resume-session.js
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.
Fifth slice out of the strongly-connected core. One hook (
playSong) plus acurrentFilenamegetter.S.pendingResumejoins the container — on demand, exactly as intendedapp.js writes it (
playSong({ resume })arms it; thesong:readylistener consumes it) while this module reads it. So it can't be a plain export — an imported binding is read-only. Same reasonisPlayingis there.The container grows one field per carve that needs it, never speculatively. That's the discipline that keeps it from becoming the ~977-site big-bang lift.
The contract test caught the missing hook — again
Second time it has caught a real wiring gap the moment it appeared, on a path nothing executes.
A trap worth recording
I first did the
S.pendingResumerewrite by feeding acorn's identifier ranges from node into python. It corrupted the file:Acorn's offsets are UTF-16 code units; Python's string indices are code points.
static/app.jscontains emoji, so every offset past one drifts. Do an AST-driven rewrite in the same language that produced the offsets.node --checkcaught it — but a silent version of that bug is very easy to imagine, and it would have landed as a one-character corruption somewhere in a 7,700-line file.Verification
A/B against
origin/mainin two browsers, real song: the window API (resumeLastSession/_snapshotResumeSession/_readResumeSession/_clearResumeSession), snapshot, read-back, clear — identical, zero page errors.Honest limit
My probe never got the snapshot to actually persist — there's a guard beyond the 3-second minimum position that a scripted
playSong()doesn't satisfy. So that path is verified as identical to main, not as observed working. The real coverage istests/browser/resume-session.spec.ts, which drives the flow properly.Zero harnesses broke. 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