fix(library): Edit Metadata modal — editable Year + don't close on drag-release outside (#623)

* fix(library): Edit Metadata modal — editable Year + no close on drag-release

Two fixes to the Songs -> Edit Metadata modal (openEditModal/saveEditModal in
static/app.js), both reported on macOS for 0.3.0.

1) Year is now editable. A year can be set when authoring a pak but the modal
   had no Year field, so it could never be changed. The backend
   (POST /api/song/<f>/meta) already accepts + normalizes `year` and writes it
   into the file via songmeta (survives a rescan) -- only the UI omitted it.
   Add a Year input (populated from the song's current year) and include
   `year` in the save POST body. Both the v3 card menu and the legacy edit
   button already pass the year through, so both surfaces get the field.

2) The modal no longer closes when a click-drag is released on the backdrop.
   Selecting text inside a field and releasing the mouse past the modal edge
   dismissed the form without warning (the `click` event's target resolves to
   the backdrop, the common ancestor) -- discarding the edit. Backdrop
   dismissal now also requires the mousedown to have STARTED on the backdrop,
   tracked per-modal and decided by a new pure helper
   _editModalShouldClose(clickTarget, modalEl, downOnBackdrop). Cancel / X
   still close on a normal click.

Tests: tests/js/edit_metadata_modal.test.js extracts the real functions from
app.js and asserts (a) openEditModal renders #edit-year, (b) saveEditModal's
meta POST body carries `year`, and (c) the backdrop-close decision table
(Cancel always closes; backdrop needs down+up on the backdrop; a drag from a
field released on the backdrop does NOT close).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QbexxfTt8q2tAn436MqGWF

* fix(library): wire Edit Metadata Save via listener, not an inline onclick

encodeURIComponent does not escape "'", so embedding the filename in the
single-quoted inline onclick="saveEditModal('…')" handler produced a
malformed handler for any song whose filename contains an apostrophe
(e.g. Bob's Song.sloppak) — clicking Save threw a syntax error and the
edit silently failed. Replace the inline onclick with a data-edit-save
hook wired in JS from the closure filename (mirrors the existing Delete
button pattern), so the filename never has to survive attribute-string
embedding. Pre-existing bug surfaced during review of this modal.

Adds a regression assertion (no inline saveEditModal onclick; Save wired
via data-edit-save).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: byrongamatos <xasiklas@gmail.com>

CHANGELOG.md

@@ -38,6 +38,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - **v3 library: exact artist/album filters + scroll/page-depth restore** (feedBack#857). The v3 Songs toolbar gains Artist and Album dropdowns (Album populates from the selected artist and stays disabled until one is chosen), backed by new exact, case-insensitive (`COLLATE NOCASE`) `artist` / `album` query params threaded through `MetadataDB._build_where` → `query_page` / `query_artists` / `query_stats` and the `/api/library`, `/api/library/artists`, `/api/library/stats` endpoints (the free-text `q` search stays fuzzy and composes with the exact filters). The artist/album catalog is fetched independently of the active artist/album selection so the dropdowns always list the full set for the current provider/search. The toolbar is now sticky so filter controls stay reachable when browsing deep libraries, and returning from the player restores the previous scroll position **and** the loaded infinite-scroll page depth via a `sessionStorage` snapshot keyed by a filter/sort/view state hash (invalidated whenever those change, so a filter change still resets to the top). Tests: `tests/test_library_filters.py` (backend artist/album filters), `tests/js/v3_songs_scroll.test.js` (state-hash + snapshot helpers).
 
 ### Fixed
+- **Edit Metadata modal: the Year is now editable.** You could set a year when authoring a pak but the Songs → Edit Metadata modal had no Year field, so it could never be changed afterward. The backend (`POST /api/song/<f>/meta`) already accepted and normalized `year` (writes it into the file via `songmeta`, survives a rescan) — only the UI omitted it. Added a **Year** input to `openEditModal()` (populated from the song's existing year) and included `year` in `saveEditModal()`'s POST body (`static/app.js`). Both the v3 card menu and the legacy edit button already pass the year through, so both surfaces get the field.
+- **Edit Metadata modal no longer closes when a click-drag is released on the backdrop.** Selecting text inside a field and releasing the mouse past the modal's edge dismissed the form without warning (the `click` event's target resolved to the backdrop), discarding the edit. Backdrop dismissal now requires the **mousedown to have started on the backdrop** too — tracked per-modal and decided by a new pure `_editModalShouldClose(clickTarget, modalEl, downOnBackdrop)` helper (`static/app.js`). Cancel / ✕ still close on a normal click. Tests: `tests/js/edit_metadata_modal.test.js` (year in the POST body + the backdrop-close decision table).
 - **Built-in diagnostic sloppak rebranded "Slopsmith" → "FeedBack" in the song name.** PR #586 renamed the file to `feedBack-diagnostic-basic-guitar.sloppak` but never regenerated the archive, so the manifest inside still carried `title: Slopsmith Diagnostic — Basic Guitar` / `artist: Slopsmith` (and the same heading in `DIAGNOSTIC.md`) — the stale name testers saw in the library/player and the onboarding calibration step, even though the build script, server, and docs all already say "FeedBack Diagnostic — Basic Guitar". Regenerated `docs/diagnostics/feedBack-diagnostic-basic-guitar.sloppak` from `docs/diagnostics/build_diagnostic_basic_guitar.py` so the committed artifact matches its source generator (title/artist/heading now "FeedBack"; chart, stem, and `diagnostic:` metadata unchanged). No code change — the rename in #586 just needed the rebuild.
 - **v3 song/lesson accuracy badges now refresh on the first return from a song — no restart needed.** PR #574 added a `stats:recorded` → in-place badge repaint, but the repaint never matched a card. The event (like `song:loading`) carries the filename **`encodeURIComponent`'d** — exactly as `playCard` hands it to `playSong` (the highway WS `decodeURIComponent`s it back) — whereas library cards key on the **decoded** `localFilename` (`data-fn`), and `/api/stats/best` is server-canonicalized to that same decoded key (`server.py` `_canonical_song_filename`). So `repaintAccuracy`'s `data-fn !== key` check rejected every card and `state.accuracy[encoded]` was `undefined`, leaving the just-earned badge stale until a full `render()` (app restart / search / re-enter the screen) — which is why it "came back after a restart." `static/v3/songs.js` now decodes the `stats:recorded` filename back into the card / `state.accuracy` key space via a small `decFn` helper before marking dirty and repainting (idempotent for already-decoded names; falls back to the original on malformed input so a real filename containing a literal `%` is never corrupted), so both the immediate repaint and the `onV3SongsScreenEnter` deferred path land on the right card. Tests: `tests/js/v3_songs_score_badge_refresh.test.js`.
 - **Escape now exits a song (and leaves Settings) even when a transport/rail control button holds keyboard focus.** Clicking a player control (Play / FF / RW / Restart) left that `<button>` focused, and `_shortcutDispatchBlocked()` in `static/app.js` treats any focused `INPUT/SELECT/TEXTAREA/BUTTON` as an "interactive control" and bails before the shortcut registry runs — so the player-scope `Escape → Back` shortcut never fired until the user clicked empty canvas to blur the control ("Escape in song not consistent"). Space already had a player-screen carve-out (#593) that let it fire through a focused control; Escape did not. Generalized that carve-out to Escape, scoped to the player **and** settings screens (both register an `Escape = Back` shortcut, and settings had the identical latent bug). The earlier guards are preserved and still win: text inputs are exempted first (Escape there clears/blurs the field), the Section Practice popover already claims Escape before the carve-out, and a true modal layered over the screen (`[role="dialog"][aria-modal="true"]` / `.feedBack-modal`) still traps Escape so it closes the modal rather than ejecting past it. Escape becomes a reliable, focus-independent "Back" — making it monotonic groundwork for an optional exit-confirm. Plugins that register a player-scope `Escape` shortcut benefit identically (they were broken the same way). Tests: `tests/browser/keyboard-shortcuts.spec.ts` (focused-button repro, text-input no-exit, no-escape-past-modal, Section Practice popover, settings twin-bug).

static/app.js

@@ -10123,9 +10123,14 @@ function openEditModal(songData, openerEl) {
                     <input type="text" id="edit-album" value="${_escAttr(songData.al)}"
                         class="w-full bg-dark-600 border border-gray-700 rounded-lg px-3 py-2 text-sm text-gray-200 outline-none focus:border-accent/50">
                 </div>
+                <div>
+                    <label class="text-xs text-gray-400 mb-1 block">Year</label>
+                    <input type="text" inputmode="numeric" id="edit-year" value="${_escAttr(songData.y)}" placeholder="e.g. 2024"
+                        class="w-full bg-dark-600 border border-gray-700 rounded-lg px-3 py-2 text-sm text-gray-200 outline-none focus:border-accent/50">
+                </div>
             </div>
             <div class="flex gap-3 mt-5">
-                <button onclick="saveEditModal('${encodeURIComponent(songData.f)}')"
+                <button data-edit-save
                     class="flex-1 bg-accent hover:bg-accent-light px-4 py-2 rounded-xl text-sm font-semibold text-white transition">Save</button>
                 <button data-edit-close
                     class="px-4 py-2 bg-dark-600 hover:bg-dark-500 rounded-xl text-sm text-gray-300 transition">Cancel</button>
@@ -10161,6 +10166,16 @@ function openEditModal(songData, openerEl) {
         document.getElementById('edit-art-file').click();
     });
 
+    // Save — wired in JS (not an inline onclick) so the filename never has to
+    // survive embedding in a single-quoted attribute string. encodeURIComponent
+    // does NOT escape `'`, so a filename like `Bob's Song.sloppak` used to break
+    // the inline `saveEditModal('…')` handler and silently fail the save. The
+    // raw filename lives in the closure; encode it here for saveEditModal.
+    const saveBtn = modal.querySelector('[data-edit-save]');
+    if (saveBtn) {
+        saveBtn.addEventListener('click', () => saveEditModal(encodeURIComponent(songData.f)));
+    }
+
     const deleteBtn = modal.querySelector('[data-delete-filename]');
     if (deleteBtn) {
         deleteBtn.addEventListener('click', () => {
@@ -10169,17 +10184,34 @@ function openEditModal(songData, openerEl) {
     }
 
     // Close on backdrop click or Cancel button; restore focus to opener.
+    // Backdrop dismissal requires the gesture's mousedown to have STARTED on
+    // the backdrop — not just the click/mouseup to land there. Otherwise a
+    // click-drag that begins inside a field (e.g. selecting text) and is
+    // released past the modal edge resolves its `click` target to the backdrop
+    // and silently discards the edit. Cancel / ✕ (data-edit-close) always close.
+    let _downOnBackdrop = false;
+    modal.addEventListener('mousedown', (e) => { _downOnBackdrop = (e.target === modal); });
     modal.addEventListener('click', (e) => {
-        if (e.target === modal || e.target.closest('[data-edit-close]')) {
+        if (!_editModalShouldClose(e.target, modal, _downOnBackdrop)) return;
-            const opener = modal._opener;
+        const opener = modal._opener;
-            modal.remove();
+        modal.remove();
-            const focusTarget = (opener && document.body.contains(opener)) ? opener
+        const focusTarget = (opener && document.body.contains(opener)) ? opener
-                : (_lastLibSelected && document.body.contains(_lastLibSelected) ? _lastLibSelected : null);
+            : (_lastLibSelected && document.body.contains(_lastLibSelected) ? _lastLibSelected : null);
-            if (focusTarget) focusTarget.focus({ preventScroll: true });
+        if (focusTarget) focusTarget.focus({ preventScroll: true });
-        }
     });
 }
 
+// Whether a click on the edit-metadata modal should dismiss it. The Cancel / ✕
+// control (data-edit-close) always dismisses. A backdrop dismissal needs BOTH
+// the click target to be the backdrop element itself AND the gesture to have
+// started there (downOnBackdrop) — so a click-drag begun inside a field and
+// released on the backdrop does not discard the form. Pure + top-level so it's
+// unit-testable in isolation.
+function _editModalShouldClose(clickTarget, modalEl, downOnBackdrop) {
+    if (clickTarget && clickTarget.closest && clickTarget.closest('[data-edit-close]')) return true;
+    return clickTarget === modalEl && downOnBackdrop === true;
+}
+
 function previewEditArt(input) {
     if (!input.files || !input.files[0]) return;
     const reader = new FileReader();
@@ -10200,6 +10232,9 @@ async function saveEditModal(encodedFilename) {
             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(),
         }),
     });
 

tests/js/edit_metadata_modal.test.js

@@ -0,0 +1,111 @@
+// Regression guards for two Edit-Metadata modal fixes (static/app.js):
+//
+//  1. Year is editable — the modal renders an `edit-year` field and
+//     saveEditModal() includes `year` in the POST /api/song/<f>/meta body.
+//     (Backend already accepts/normalizes year; only the UI omitted it.)
+//
+//  2. A click-drag that starts inside a field and is released on the backdrop
+//     must NOT dismiss the modal. _editModalShouldClose() gates backdrop
+//     dismissal on the mousedown having started on the backdrop too.
+//
+// Functions are extracted from the real shipped source and run in a vm — no
+// mirror copies.
+
+'use strict';
+const { test } = require('node:test');
+const assert = require('node:assert/strict');
+const fs = require('node:fs');
+const path = require('node:path');
+const vm = require('node:vm');
+const { extractFunction } = require('./test_utils');
+
+const APP_JS = path.join(__dirname, '..', '..', 'static', 'app.js');
+const readApp = () => fs.readFileSync(APP_JS, 'utf8');
+
+function loadFn(signature, sandbox, exportAs) {
+    const fnSrc = extractFunction(readApp(), signature);
+    const ctx = vm.createContext(sandbox);
+    vm.runInContext(`${fnSrc}\nglobalThis.${exportAs} = ${exportAs};`, ctx);
+    return sandbox[exportAs];
+}
+
+// ── Issue: Edit Metadata does not allow changing Year ────────────────────────
+
+test('openEditModal renders a Year field bound to songData.y', () => {
+    const src = extractFunction(readApp(), 'function openEditModal');
+    assert.match(src, /id="edit-year"/, 'modal must render an #edit-year input');
+    assert.match(src, /_escAttr\(songData\.y\)/, 'year input must be populated from songData.y');
+});
+
+test('Save button wires via data-edit-save, not an inline onclick that embeds the filename', () => {
+    // encodeURIComponent does NOT escape `'`, so embedding the filename in a
+    // single-quoted inline `saveEditModal('…')` handler breaks the save for a
+    // song whose filename contains an apostrophe (e.g. `Bob's Song.sloppak`).
+    // The Save button must use the data-attr + JS-listener pattern instead.
+    const src = extractFunction(readApp(), 'function openEditModal');
+    assert.doesNotMatch(src, /onclick="saveEditModal\('/, 'Save must not embed the filename in an inline onclick');
+    assert.match(src, /data-edit-save/, 'Save button must carry the data-edit-save hook');
+    assert.match(src, /querySelector\('\[data-edit-save\]'\)/, 'Save must be wired via addEventListener');
+});
+
+test('saveEditModal includes year in the metadata POST body', async () => {
+    const calls = [];
+    const values = {
+        'edit-title': 'My Title', 'edit-artist': 'My Artist',
+        'edit-album': 'My Album', 'edit-year': '1998',
+        'edit-art-file': null,    // signals the file branch via .files below
+        'edit-modal': null,
+    };
+    const sandbox = {
+        decodeURIComponent, encodeURIComponent, JSON, Promise,
+        _lastLibSelected: null,
+        loadLibrary: () => {}, loadFavorites: () => {},
+        fetch: (url, opts) => { calls.push({ url, opts }); return Promise.resolve({ ok: true }); },
+        document: {
+            getElementById: (id) => {
+                if (id === 'edit-art-file') return { files: null };
+                if (id === 'edit-modal') return null;
+                return id in values ? { value: values[id] } : null;
+            },
+            querySelector: () => null,            // no active screen
+            body: { contains: () => false },
+        },
+    };
+    const saveEditModal = loadFn('async function saveEditModal', sandbox, 'saveEditModal');
+
+    await saveEditModal(encodeURIComponent('Song With Spaces.sloppak'));
+
+    const metaCall = calls.find((c) => /\/api\/song\/.+\/meta$/.test(c.url));
+    assert.ok(metaCall, 'expected a POST to /api/song/<filename>/meta');
+    const body = JSON.parse(metaCall.opts.body);
+    assert.equal(body.year, '1998', 'meta POST body must carry the edited year');
+    assert.deepEqual(
+        body,
+        { title: 'My Title', artist: 'My Artist', album: 'My Album', year: '1998' },
+        'meta POST body shape',
+    );
+});
+
+// ── Issue: Renaming Metadata Closes Modal (click-drag release on backdrop) ────
+
+test('_editModalShouldClose: backdrop needs mousedown to have started there', () => {
+    const fn = loadFn('function _editModalShouldClose', {}, '_editModalShouldClose');
+
+    const modalEl = { closest: () => null };                 // the backdrop element
+    const innerEl = { closest: () => null };                 // a field inside the modal
+    const cancelBtn = { closest: (s) => (s === '[data-edit-close]' ? { tag: 'button' } : null) };
+
+    // Cancel / ✕ always closes, regardless of where the mousedown began.
+    assert.equal(fn(cancelBtn, modalEl, false), true, 'Cancel/✕ closes');
+    assert.equal(fn(cancelBtn, modalEl, true), true, 'Cancel/✕ closes (down-on-backdrop irrelevant)');
+
+    // Genuine backdrop click: down AND up on the backdrop.
+    assert.equal(fn(modalEl, modalEl, true), true, 'backdrop down+up closes');
+
+    // The reported bug: drag began inside a field (down NOT on backdrop), click
+    // resolves to the backdrop on release — must NOT close.
+    assert.equal(fn(modalEl, modalEl, false), false, 'drag-from-field release on backdrop does NOT close');
+
+    // A click that lands on inner content never closes via the backdrop path.
+    assert.equal(fn(innerEl, modalEl, true), false, 'click on inner content does not close');
+});