refactor(app): carve the edit-song modal into static/js/edit-modal.js (R3d) - #919
Merged
Conversation
… (R3d) 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>
Contributor
📝 WalkthroughWalkthroughThe edit-song modal handlers were extracted from ChangesEdit modal flow
Estimated code review effort: 3 (Moderate) | ~20 minutes 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: 1
🧹 Nitpick comments (1)
tests/js/edit_metadata_modal.test.js (1)
66-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the art-upload path.
edit-art-filealways mocks{ files: null }, sosaveEditModal's art-upload branch (and its unawaited-FileReader/missing-error-handling issue flagged inedit-modal.js) is never exercised by this regression suite.🤖 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/edit_metadata_modal.test.js` around lines 66 - 72, The test fixture for edit-art-file only covers the no-file path, leaving saveEditModal’s art-upload branch untested. Extend the mock and regression tests in the edit metadata modal suite to provide a file and exercise upload behavior, including the FileReader flow and its error handling; preserve the existing null-files coverage.
🤖 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/edit-modal.js`:
- Around line 162-205: Update saveEditModal to await and validate both the
metadata and art-upload requests, treating non-OK responses and network errors
as failures and surfacing them consistently with deleteSongFromModal. Wrap the
FileReader operation in a Promise so the upload completes before modal removal
and view refresh; only close and refresh after all saves succeed, while
preserving the existing focus restoration and favorites/library refresh
behavior.
---
Nitpick comments:
In `@tests/js/edit_metadata_modal.test.js`:
- Around line 66-72: The test fixture for edit-art-file only covers the no-file
path, leaving saveEditModal’s art-upload branch untested. Extend the mock and
regression tests in the edit metadata modal suite to provide a file and exercise
upload behavior, including the FileReader flow and its error handling; preserve
the existing null-files coverage.
🪄 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: 998cde45-0c3d-45fd-a4b0-86b8db87ce34
📒 Files selected for processing (3)
static/app.jsstatic/js/edit-modal.jstests/js/edit_metadata_modal.test.js
Comment on lines
+162
to
+205
| export async function saveEditModal(encodedFilename) { | ||
| const filename = decodeURIComponent(encodedFilename); | ||
|
|
||
| // Save metadata | ||
| await fetch(`/api/song/${encodeURIComponent(filename)}/meta`, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ | ||
| title: document.getElementById('edit-title').value.trim(), | ||
| artist: document.getElementById('edit-artist').value.trim(), | ||
| album: document.getElementById('edit-album').value.trim(), | ||
| // Year is normalised server-side (non-numeric/empty → ""), so a | ||
| // blank or cleared field round-trips safely. | ||
| year: document.getElementById('edit-year').value.trim(), | ||
| }), | ||
| }); | ||
|
|
||
| // Upload art if changed | ||
| const fileInput = document.getElementById('edit-art-file'); | ||
| if (fileInput.files && fileInput.files[0]) { | ||
| const reader = new FileReader(); | ||
| reader.onload = async (e) => { | ||
| await fetch(`/api/song/${encodeURIComponent(filename)}/art/upload`, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ image: e.target.result }), | ||
| }); | ||
| }; | ||
| reader.readAsDataURL(fileInput.files[0]); | ||
| } | ||
|
|
||
| const modal = document.getElementById('edit-modal'); | ||
| const opener = modal ? modal._opener : null; | ||
| if (modal) modal.remove(); | ||
| // Restore focus to the entry the modal was opened from so subsequent | ||
| // keyboard navigation resumes correctly (same as Esc / Cancel paths). | ||
| const focusTarget = (opener && document.body.contains(opener)) ? opener | ||
| : (_lastLibSelected && document.body.contains(_lastLibSelected) ? _lastLibSelected : null); | ||
| if (focusTarget) focusTarget.focus({ preventScroll: true }); | ||
| // Refresh current view | ||
| const activeScreen = document.querySelector('.screen.active'); | ||
| if (activeScreen?.id === 'favorites') loadFavorites(); | ||
| else loadLibrary(); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
saveEditModal swallows fetch failures and doesn't await the art upload before refreshing.
Two issues in this function, unlike deleteSongFromModal below which does this correctly:
- Neither the metadata
fetch(166-177) nor the art-uploadfetch(184-188) checksresp.okor is wrapped in try/catch. A failed save (network error or non-2xx response) is silently ignored — the modal still closes and the view refreshes as if the save succeeded, misleading the user into thinking their edit was persisted. - The art upload runs inside
reader.onload, which is never awaited. The code falls through tomodal.remove()andloadLibrary()/loadFavorites()(193-205) immediately, so the grid/tree refresh can render before the new art has actually been uploaded to the server — a race condition that shows stale artwork.
Wrapping the FileReader read in a Promise (the standard pattern for combining FileReader with async/await) and awaiting it, plus adding basic error surfacing consistent with deleteSongFromModal, would fix both.
🛡️ Proposed fix
export async function saveEditModal(encodedFilename) {
const filename = decodeURIComponent(encodedFilename);
- // Save metadata
- await fetch(`/api/song/${encodeURIComponent(filename)}/meta`, {
- method: 'POST',
- headers: { 'Content-Type': 'application/json' },
- body: JSON.stringify({
- title: document.getElementById('edit-title').value.trim(),
- artist: document.getElementById('edit-artist').value.trim(),
- album: document.getElementById('edit-album').value.trim(),
- // Year is normalised server-side (non-numeric/empty → ""), so a
- // blank or cleared field round-trips safely.
- year: document.getElementById('edit-year').value.trim(),
- }),
- });
-
- // Upload art if changed
- const fileInput = document.getElementById('edit-art-file');
- if (fileInput.files && fileInput.files[0]) {
- const reader = new FileReader();
- reader.onload = async (e) => {
- await fetch(`/api/song/${encodeURIComponent(filename)}/art/upload`, {
- method: 'POST',
- headers: { 'Content-Type': 'application/json' },
- body: JSON.stringify({ image: e.target.result }),
- });
- };
- reader.readAsDataURL(fileInput.files[0]);
- }
+ try {
+ const metaResp = await fetch(`/api/song/${encodeURIComponent(filename)}/meta`, {
+ method: 'POST',
+ headers: { 'Content-Type': 'application/json' },
+ body: JSON.stringify({
+ title: document.getElementById('edit-title').value.trim(),
+ artist: document.getElementById('edit-artist').value.trim(),
+ album: document.getElementById('edit-album').value.trim(),
+ year: document.getElementById('edit-year').value.trim(),
+ }),
+ });
+ if (!metaResp.ok) throw new Error(metaResp.statusText);
+
+ const fileInput = document.getElementById('edit-art-file');
+ if (fileInput.files && fileInput.files[0]) {
+ const dataUrl = await new Promise((resolve, reject) => {
+ const reader = new FileReader();
+ reader.onload = (e) => resolve(e.target.result);
+ reader.onerror = reject;
+ reader.readAsDataURL(fileInput.files[0]);
+ });
+ const artResp = await fetch(`/api/song/${encodeURIComponent(filename)}/art/upload`, {
+ method: 'POST',
+ headers: { 'Content-Type': 'application/json' },
+ body: JSON.stringify({ image: dataUrl }),
+ });
+ if (!artResp.ok) throw new Error(artResp.statusText);
+ }
+ } catch (e) {
+ alert(`Save failed: ${e.message}`);
+ return;
+ }
const modal = document.getElementById('edit-modal');📝 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 saveEditModal(encodedFilename) { | |
| const filename = decodeURIComponent(encodedFilename); | |
| // Save metadata | |
| await fetch(`/api/song/${encodeURIComponent(filename)}/meta`, { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify({ | |
| title: document.getElementById('edit-title').value.trim(), | |
| artist: document.getElementById('edit-artist').value.trim(), | |
| album: document.getElementById('edit-album').value.trim(), | |
| // Year is normalised server-side (non-numeric/empty → ""), so a | |
| // blank or cleared field round-trips safely. | |
| year: document.getElementById('edit-year').value.trim(), | |
| }), | |
| }); | |
| // Upload art if changed | |
| const fileInput = document.getElementById('edit-art-file'); | |
| if (fileInput.files && fileInput.files[0]) { | |
| const reader = new FileReader(); | |
| reader.onload = async (e) => { | |
| await fetch(`/api/song/${encodeURIComponent(filename)}/art/upload`, { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify({ image: e.target.result }), | |
| }); | |
| }; | |
| reader.readAsDataURL(fileInput.files[0]); | |
| } | |
| const modal = document.getElementById('edit-modal'); | |
| const opener = modal ? modal._opener : null; | |
| if (modal) modal.remove(); | |
| // Restore focus to the entry the modal was opened from so subsequent | |
| // keyboard navigation resumes correctly (same as Esc / Cancel paths). | |
| const focusTarget = (opener && document.body.contains(opener)) ? opener | |
| : (_lastLibSelected && document.body.contains(_lastLibSelected) ? _lastLibSelected : null); | |
| if (focusTarget) focusTarget.focus({ preventScroll: true }); | |
| // Refresh current view | |
| const activeScreen = document.querySelector('.screen.active'); | |
| if (activeScreen?.id === 'favorites') loadFavorites(); | |
| else loadLibrary(); | |
| } | |
| export async function saveEditModal(encodedFilename) { | |
| const filename = decodeURIComponent(encodedFilename); | |
| try { | |
| const metaResp = await fetch(`/api/song/${encodeURIComponent(filename)}/meta`, { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify({ | |
| title: document.getElementById('edit-title').value.trim(), | |
| artist: document.getElementById('edit-artist').value.trim(), | |
| album: document.getElementById('edit-album').value.trim(), | |
| year: document.getElementById('edit-year').value.trim(), | |
| }), | |
| }); | |
| if (!metaResp.ok) throw new Error(metaResp.statusText); | |
| const fileInput = document.getElementById('edit-art-file'); | |
| if (fileInput.files && fileInput.files[0]) { | |
| const dataUrl = await new Promise((resolve, reject) => { | |
| const reader = new FileReader(); | |
| reader.onload = (e) => resolve(e.target.result); | |
| reader.onerror = reject; | |
| reader.readAsDataURL(fileInput.files[0]); | |
| }); | |
| const artResp = await fetch(`/api/song/${encodeURIComponent(filename)}/art/upload`, { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify({ image: dataUrl }), | |
| }); | |
| if (!artResp.ok) throw new Error(artResp.statusText); | |
| } | |
| } catch (e) { | |
| alert(`Save failed: ${e.message}`); | |
| return; | |
| } | |
| const modal = document.getElementById('edit-modal'); | |
| const opener = modal ? modal._opener : null; | |
| if (modal) modal.remove(); | |
| // Restore focus to the entry the modal was opened from so subsequent | |
| // keyboard navigation resumes correctly (same as Esc / Cancel paths). | |
| const focusTarget = (opener && document.body.contains(opener)) ? opener | |
| : (_lastLibSelected && document.body.contains(_lastLibSelected) ? _lastLibSelected : null); | |
| if (focusTarget) focusTarget.focus({ preventScroll: true }); | |
| // Refresh current view | |
| const activeScreen = document.querySelector('.screen.active'); | |
| if (activeScreen?.id === 'favorites') loadFavorites(); | |
| else loadLibrary(); | |
| } |
🤖 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/edit-modal.js` around lines 162 - 205, Update saveEditModal to
await and validate both the metadata and art-upload requests, treating non-OK
responses and network errors as failures and surfacing them consistently with
deleteSongFromModal. Wrap the FileReader operation in a Promise so the upload
completes before modal removal and view refresh; only close and refresh after
all saves succeed, while preserving the existing focus restoration and
favorites/library refresh behavior.
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.
4 functions, 234 lines.
app.js4,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's 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), plusdom.jsand theLcontainer. 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.Verification
A/B against
origin/mainin two browsers — identical, no new page errors:openEditModalet al.)data-edit-savewiring holds — rather than an inlineonclickembedding the filename, which is the fix this cluster's harness exists to guardnode 1045 · pytest 2425 · ESLint 0 (no-cycle clean) · host contract 2/2 · Codex 0.
🤖 Generated with Claude Code
Summary by CodeRabbit