diff --git a/backend/shortcuts/config.go b/backend/shortcuts/config.go index cf8521b..6ce12c1 100644 --- a/backend/shortcuts/config.go +++ b/backend/shortcuts/config.go @@ -33,15 +33,16 @@ func DefaultBindings() map[string]string { "app.selectAll": "Ctrl+A", "app.shortcuts": "?", - // Panel-specific (track list). There is no `tracklist.delete`: - // it was bound to Delete and advertised in Settings as - // configurable while nothing listened for it, because "remove - // from library" does not exist and it is not clear what it would - // remove — the row (which the next scan puts back unless the path - // is also excluded) or the file (a delete-your-music button one - // keystroke from a focused row). Advertise it again when it does - // something. - "tracklist.play": "Enter", + // Panel-specific (track list). `tracklist.delete` spent six + // phases advertised in Settings with nothing on the other end of + // it, because "remove from library" did not exist and it was not + // clear what it would remove. It now removes the row and + // excludes the path from future scans, and leaves the file on + // disk — and the key only *opens the confirmation*, never + // performs the removal, which is the only version defensible one + // keystroke from a focused row. + "tracklist.play": "Enter", + "tracklist.delete": "Delete", // Panel-specific (autotag review). These are the keys the // autotag page used to bind on its own document listener, which diff --git a/e2e/specs/remove-from-library.spec.ts b/e2e/specs/remove-from-library.spec.ts new file mode 100644 index 0000000..bacd015 --- /dev/null +++ b/e2e/specs/remove-from-library.spec.ts @@ -0,0 +1,149 @@ +import { existsSync } from 'node:fs'; + +import { test, expect, resetEvents, callBinding, eventNames } from '../support/fixtures.js'; + +/** + * "Remove from library" removes the row and leaves the file. + * + * Two assertions carry this spec and neither is about the row count. + * The first is that the **file is still on disk** — that is the promise + * the confirmation copy makes, and the only thing standing between this + * feature and a user's music. The second is that a **real scan does not + * bring the row back**: without the exclusion the operation undoes + * itself on the next scan, which is worse than not having it. + * + * The suite shares one backend process in file order, so this restores + * the database it spent. + */ +const SNAPSHOT = 'e2e-pre-remove'; + +/** The file paths of the first n rows, in list order. */ +const firstPaths = (n: number): string[] => Array.from( + document.querySelector('track-list') + ?.shadowRoot?.querySelectorAll('[data-file-path]') ?? [], +).map((r) => r.getAttribute('data-file-path') ?? '').slice(0, n); + +test.describe('remove from library', () => { + test.beforeAll(async ({ baseURL }) => { + // VACUUM INTO copies the whole file and the restore copies every row + // back, which is well over the 30 s a hook gets by default once + // earlier specs have staged an explore catalog. + test.setTimeout(180_000); + + const res = await fetch(`${baseURL}/__test/db/snapshot?name=${SNAPSHOT}`, { + method: 'POST', + signal: AbortSignal.timeout(120_000), + }); + + expect(res.ok, 'could not snapshot the database before spending it').toBe(true); + }); + + test.afterAll(async ({ baseURL }) => { + test.setTimeout(180_000); + + const res = await fetch(`${baseURL}/__test/db/restore?name=${SNAPSHOT}`, { + method: 'POST', + signal: AbortSignal.timeout(120_000), + }); + + expect(res.ok, 'could not restore the database this spec spent').toBe(true); + }); + + test.beforeEach(async ({ app }) => { + await app.getByTestId('nav-tracks').click(); + await expect(app.getByTestId('track-row').first()).toBeVisible(); + }); + + /** + * Delete is bound to *opening* the confirmation and to nothing else. + * A key that asks is defensible one row from the user's music; a key + * that acts is not. + */ + test('Delete asks, and cancelling is a true no-op', async ({ app }) => { + const before = await app.getByTestId('track-row').count(); + + await resetEvents(app); + await app.evaluate(() => { + const rows = document.querySelector('track-list') + ?.shadowRoot?.querySelectorAll('[data-testid="track-row"]'); + + rows?.[2]?.dispatchEvent(new MouseEvent('click', { + bubbles: true, composed: true, + })); + }); + + await app.keyboard.press('Delete'); + + const dialog = app.getByRole('dialog', { name: /from the library\?/ }); + + await expect(dialog).toBeVisible(); + // The copy is the user's only protection, so it is asserted rather + // than assumed: it has to say the file is not deleted. + await expect(app.getByTestId('confirm-dialog')).toContainText( + /not deleted/, + ); + + await app.getByTestId('confirm-cancel').click(); + await expect(dialog).toBeHidden(); + + expect(await app.getByTestId('track-row').count()).toBe(before); + expect((await eventNames(app))['TracksRemovedFromLibrary'] ?? 0).toBe(0); + }); + + test('confirming removes the row, keeps the file, and survives a scan', async ({ + app, + }) => { + test.setTimeout(120_000); + + const [target, control] = await app.evaluate(firstPaths, 2); + + expect(target, 'no tracks in the library to remove').toBeTruthy(); + expect(existsSync(target!), 'fixture file missing before the test').toBe(true); + + const before = await app.getByTestId('track-row').count(); + + await resetEvents(app); + await app.getByTestId('track-row').first().click({ button: 'right' }); + await app.getByRole('menuitem', { name: 'Remove from Library' }).click(); + await app.getByTestId('confirm-accept').click(); + + const removed = await app.evaluate( + () => window.__yjEvents.wait('TracksRemovedFromLibrary', { + timeoutMs: 15_000, + }), + ); + + expect((removed.data as Array>)[0]).toMatchObject({ + filePaths: [target], + count: 1, + }); + + await expect(app.getByTestId('track-row')).toHaveCount(before - 1); + + // The promise the copy makes. + expect(existsSync(target!), 'the file was deleted from disk').toBe(true); + + // And the half that makes the rest true: a real scan of the real + // directory must not import it again. + await resetEvents(app); + await callBinding(app, 'library.Library.ScanAllLibraries', []); + await app.evaluate( + () => window.__yjEvents.wait('LibraryScanComplete', { timeoutMs: 90_000 }), + ); + await app.waitForTimeout(1000); + + const paths = await app.evaluate( + () => Array.from( + document.querySelector('track-list') + ?.shadowRoot?.querySelectorAll('[data-file-path]') ?? [], + ).map((r) => r.getAttribute('data-file-path') ?? ''), + ); + + expect(paths, 'the excluded path came back on the next scan') + .not.toContain(target); + // The positive half: a guard that excluded everything would pass + // the assertion above for free. + expect(paths, 'the scan lost a path nobody excluded').toContain(control); + expect(existsSync(target!), 'the file was deleted from disk').toBe(true); + }); +}); diff --git a/frontend/src/components/track-list/track-list.ts b/frontend/src/components/track-list/track-list.ts index 7d2a8eb..1c17f26 100644 --- a/frontend/src/components/track-list/track-list.ts +++ b/frontend/src/components/track-list/track-list.ts @@ -1191,8 +1191,29 @@ export class TrackList 'shortcut:tracklist-play', this.handleShortcutPlay, ); + this.listenWhileActive( + document, + 'shortcut:tracklist-delete', + this.handleShortcutDelete, + ); } + /** + * Delete opens the confirmation and does nothing else. + * + * That is the whole design of the binding: one keystroke from a + * focused row, a key that *asks* is defensible and a key that + * *acts* is not — so this is the same dialog the menu command + * opens, reached by a different route. + */ + private handleShortcutDelete = (): void => { + const filePaths = this.selection.getSelectedKeysOrdered(); + + if (filePaths.length === 0) return; + + void this.removeFromLibrary(filePaths); + }; + /** Enter plays the selection — the `tracklist.play` binding, which * has existed in the defaults and in Settings since it was written * and has never had anything on the other end of it. */ diff --git a/frontend/src/services/keyboard-shortcut-service.ts b/frontend/src/services/keyboard-shortcut-service.ts index 035c20a..9e0053d 100644 --- a/frontend/src/services/keyboard-shortcut-service.ts +++ b/frontend/src/services/keyboard-shortcut-service.ts @@ -438,9 +438,14 @@ async function dispatch(action: string): Promise { ); break; - // No `tracklist.delete`: it dispatched an event nothing - // listened for, from a binding Settings advertised as - // configurable. See backend/shortcuts/config.go. + // `tracklist.delete` opens the confirmation and nothing else: + // the key is a request, not an action. See + // backend/shortcuts/config.go. + case 'tracklist.delete': + document.dispatchEvent( + new CustomEvent('shortcut:tracklist-delete'), + ); + break; // Panel-specific: autotag review. The view listens for these // while it is the view on screen, and for nothing while it is diff --git a/frontend/src/services/shortcut-meta.ts b/frontend/src/services/shortcut-meta.ts index 1ea3b59..eb1d0f9 100644 --- a/frontend/src/services/shortcut-meta.ts +++ b/frontend/src/services/shortcut-meta.ts @@ -121,6 +121,12 @@ export const SHORTCUT_META: Record = { scope: 'panel:track-list', defaultKey: 'Enter', }, + 'tracklist.delete': { + label: 'Remove from Library', + category: 'Navigation', + scope: 'panel:track-list', + defaultKey: 'Delete', + }, 'autotag.apply': { label: 'Apply Match', category: 'Autotag', diff --git a/frontend/test/stores/keyboard-shortcuts.test.ts b/frontend/test/stores/keyboard-shortcuts.test.ts index 2085c0d..8d4dffa 100644 --- a/frontend/test/stores/keyboard-shortcuts.test.ts +++ b/frontend/test/stores/keyboard-shortcuts.test.ts @@ -383,6 +383,35 @@ describe('shortcut dispatch: scope', () => { expect(fired).toBe(1); }); + + /** + * `tracklist.delete` was advertised in Settings for six phases with + * nothing dispatching for it. What it dispatches now only *opens* a + * confirmation, which is what makes a destructive action defensible + * on an unmodified key one row from the user's music. + */ + it('dispatches for tracklist.delete, which had nothing on the other end', () => { + bindings({ 'tracklist.delete': 'Delete' }); + + const panel = mount(document.createElement('div')); + const row = document.createElement('div'); + + row.tabIndex = 0; + panel.dataset['shortcutScope'] = 'tracklist'; + panel.append(row); + row.focus(); + + let fired = 0; + const listener = (): void => { + fired += 1; + }; + + document.addEventListener('shortcut:tracklist-delete', listener); + press('Delete'); + document.removeEventListener('shortcut:tracklist-delete', listener); + + expect(fired).toBe(1); + }); }); // ===================================================================