diff --git a/CLAUDE.md b/CLAUDE.md index 664eccf..94a4f67 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -689,6 +689,37 @@ retained chars and cap in one eval, rather than the next session having to rebuild the twenty-four-search reproduction before it can tell whether the ceiling still holds. +**Expanding an album shows its tracks, and the code to do it was +written and never called.** `cover-grid`'s dropdown — the album's +tracks drawn between the two halves of a split grid — was reachable +only from Enter/Space on a focused card (a plain *click* navigates to +`explore-album-details`), and that path fetched the tracks over the +IPC, ran the whole split state machine and then rendered the single +grid, because `render()` never consulted `splitMode`. +`connectedCallback` referenced `renderSplitGrid` purely to satisfy +`noUnusedLocals`. `perf.p2` files this as dead code in the bundle; it +is the only route from the albums grid to `track-details`. + +Two things it needed that are not in the audit. **The grid could not +scroll at all**: `.grid-scroll-container` is the same markup +`artists-view` and `genres-view` use, and `cover-grid` had the class +with *no rule for it*, so the container grew to its full content height +inside an `overflow: hidden` host — 186 984 px of albums in a 772 px +box at 5 000 albums, unreachable by wheel, keyboard or scrollbar, and +invisible on the eight-album fixture. That is also the element +`scroll-manager.ts` saves and restores, so its `scrollTop` was +permanently 0; with a real scroller the manager works as designed +(2891 preserved exactly across an expand). And the shared context-menu +panel was **labelled "Album actions" unconditionally**, which nothing +could observe while the only menu that could open on a track was +unreachable. + +The manager **moves the scroll to reveal the dropdown** rather than +preserving it — on a small library that is most of the way back to the +top (80 → 4, with the content *taller* after, so it is not clamping). +"The position is preserved" is the wrong assertion; "the dropdown is on +screen" is the contract. + **A list pays per row, and only while scrolling.** The track list's Art column rendered `CoverArtPath` — the original artwork — into a 24 px box while `CoverArtSmall` sat unused on the same model, and diff --git a/e2e/specs/album-dropdown.spec.ts b/e2e/specs/album-dropdown.spec.ts new file mode 100644 index 0000000..8a97ae4 --- /dev/null +++ b/e2e/specs/album-dropdown.spec.ts @@ -0,0 +1,223 @@ +import { test, expect } from '../support/fixtures.js'; +import type { Page } from '@playwright/test'; + +/** + * Plan 007 phase 5: expanding an album shows its tracks. + * + * `perf.p2` files `cover-grid`'s `renderSplitGrid` as dead code carried + * in the bundle. It was a **missing feature** whose data path already + * worked: Enter on an album card fetched the album's tracks over the + * IPC and ran the whole split state machine, and then `render()` drew + * the single grid regardless because it never consulted `splitMode`. + * + * This spec is here rather than only in the component tier because two + * of the three things that had to be true are about the real app: that + * the route from a card to `track-details` exists at all (a plain click + * navigates to the catalog page instead, so the dropdown is the only + * one), and that the grid keeps its scroll position when the dropdown + * opens — which it did not until the scroll container was given an + * overflow, having never scrolled in its life. + */ +test.describe('the album dropdown', () => { + test.beforeEach(async ({ app }) => { + await app.getByTestId('nav-albums').click(); + await expect(app.getByTestId('main-content')).toHaveAttribute( + 'data-active-view', + 'albums', + ); + }); + + test.afterEach(async ({ app }) => { + // The suite shares one backend process in file order, and an open + // dropdown changes what the next spec's selectors match. + await closeDropdown(app); + await app.getByTestId('nav-tracks').click(); + }); + + test('Enter on a card draws that album’s tracks', async ({ app }) => { + await expandCard(app, 1); + + await expect + .poll(() => dropdownState(app)) + .toMatchObject({ present: true, split: true }); + + const state = await dropdownState(app); + + expect(state.rows).toBeGreaterThan(0); + expect(state.rows).toBe(state.tracks); + }); + + test('a track in it reaches Track Details', async ({ app }) => { + // The only route from the albums grid to a track. A plain click on + // a card navigates to `explore-album-details` instead. + await expandCard(app, 1); + await expect.poll(() => dropdownState(app)).toMatchObject({ + present: true, + }); + + await app.evaluate(() => { + document + .querySelector('cover-grid') + ?.shadowRoot?.querySelector('album-dropdown') + ?.shadowRoot?.querySelector('.track-row') + ?.dispatchEvent( + new MouseEvent('contextmenu', { + bubbles: true, + composed: true, + clientX: 300, + clientY: 400, + }), + ); + }); + + // The panel is shared with the album menu and used to be labelled + // "Album actions" unconditionally — which nothing could observe + // while the only menu that could open on a track was unreachable. + await expect(app.getByRole('menu', { name: 'Track actions' })) + .toBeVisible(); + + await app.getByRole('menuitem', { name: 'Track Details' }).click(); + + await expect( + app.getByRole('dialog', { name: 'Track Details' }), + ).toBeVisible(); + + await app.keyboard.press('Escape'); + await expect( + app.getByRole('dialog', { name: 'Track Details' }), + ).toHaveCount(0); + }); + + test('the grid it opens in can be scrolled', async ({ app }) => { + // `cover-grid` carried the same `.grid-scroll-container` markup as + // `artists-view` with no rule for the class, so it never scrolled: + // the container grew to its full content height inside an + // `overflow: hidden` host and everything past the first screenful + // was unreachable. Invisible on eight albums, fatal on a real + // library — measured at 5 000 albums, 186 984 px of content in a + // 772 px box. + // + // The fixture does not scroll at the default viewport, so this + // shrinks the window until it does. Without that, every form of + // this assertion passes against a scrollTop that is 0 both times + // and could not have moved. + await app.setViewportSize({ width: 900, height: 600 }); + + try { + await expect.poll(() => scrollRange(app)).toMatchObject({ + scrollable: true, + overflowY: 'auto', + }); + + await app.evaluate(() => { + const sc = document + .querySelector('cover-grid') + ?.shadowRoot?.querySelector('.grid-scroll-container'); + + if (sc) sc.scrollTop = 80; + }); + + expect(await scrollTop(app)).toBe(80); + + // And the dropdown it opens is on screen, wherever the manager + // decides that leaves the scroll. It is *not* "the position is + // preserved": `scrollToShowDropdown` deliberately moves it to + // reveal the dropdown, which on a library this small is most of + // the way back to the top (80 → 4, with the content *taller* + // than before, so it is not clamping). At 5 000 albums, with the + // expanded card mid-viewport, the same code preserved 2891 + // exactly. + await expandCard(app, 1); + await expect.poll(() => dropdownState(app)).toMatchObject({ + present: true, + }); + + await expect.poll(() => dropdownOnScreen(app)).toBe(true); + } finally { + await app.setViewportSize({ width: 1440, height: 900 }); + } + }); +}); + +/** Focus a card and press Enter, which is the only thing that expands one. */ +async function expandCard(app: Page, index: number): Promise { + await app.evaluate((i) => { + const card = document + .querySelector('cover-grid') + ?.shadowRoot?.querySelectorAll('.album-card')[i]; + + card?.focus(); + card?.dispatchEvent( + new KeyboardEvent('keydown', { + key: 'Enter', + bubbles: true, + composed: true, + }), + ); + }, index); +} + +async function closeDropdown(app: Page): Promise { + await app.evaluate(() => { + const grid = document.querySelector('cover-grid') as + | (Element & { expandedAlbumId: number | null }) + | null; + + if (grid) grid.expandedAlbumId = null; + }); +} + +/** Whether the grid can scroll at all, which decides if a probe can move. */ +async function scrollRange(app: Page) { + return app.evaluate(() => { + const sc = document + .querySelector('cover-grid') + ?.shadowRoot?.querySelector('.grid-scroll-container'); + + return { + scrollable: !!sc && sc.scrollHeight > sc.clientHeight + 40, + overflowY: sc ? getComputedStyle(sc).overflowY : '', + }; + }); +} + +async function scrollTop(app: Page): Promise { + return app.evaluate( + () => + document + .querySelector('cover-grid') + ?.shadowRoot?.querySelector('.grid-scroll-container')?.scrollTop ?? -1, + ); +} + +/** Whether the open dropdown is inside the scroll container's viewport. */ +async function dropdownOnScreen(app: Page): Promise { + return app.evaluate(() => { + const grid = document.querySelector('cover-grid'); + const sc = grid?.shadowRoot?.querySelector('.grid-scroll-container'); + const dd = grid?.shadowRoot?.querySelector('album-dropdown'); + + if (!sc || !dd) return false; + + const box = sc.getBoundingClientRect(); + const it = dd.getBoundingClientRect(); + + return it.bottom > box.top && it.top < box.bottom; + }); +} + +async function dropdownState(app: Page) { + return app.evaluate(() => { + const grid = document.querySelector('cover-grid') as + | (Element & { splitMode: boolean; expandedTracks: unknown[] }) + | null; + const dropdown = grid?.shadowRoot?.querySelector('album-dropdown'); + + return { + present: !!dropdown, + split: grid?.splitMode ?? false, + tracks: grid?.expandedTracks?.length ?? 0, + rows: dropdown?.shadowRoot?.querySelectorAll('.track-row').length ?? 0, + }; + }); +} diff --git a/frontend/src/components/cover-grid/cover-grid-styles.ts b/frontend/src/components/cover-grid/cover-grid-styles.ts index 68a2f1b..946283f 100644 --- a/frontend/src/components/cover-grid/cover-grid-styles.ts +++ b/frontend/src/components/cover-grid/cover-grid-styles.ts @@ -13,6 +13,27 @@ const gridStyles = css` contain: layout style; } + /* + * The scroller. artists-view and genres-view carry the same + * markup with this rule; cover-grid had the class and no rule for + * it, so nothing in the albums view scrolled — the container grew to + * its full content height inside an overflow: hidden host and + * everything past the first screenful was unreachable by wheel, + * keyboard or scrollbar. Invisible on the eight-album fixture and + * fatal on a real library: measured at 5 000 albums, 186 984 px of + * content in a 772 px box. + * + * It is also what the dropdown's scroll manager was written + * against — it saves and restores this element's scrollTop, which + * was permanently 0. + */ + .grid-scroll-container { + flex: 1; + overflow-y: auto; + overflow-x: hidden; + contain: paint; + } + /* ======================================== * Album card * ======================================== */ diff --git a/frontend/src/components/cover-grid/cover-grid.ts b/frontend/src/components/cover-grid/cover-grid.ts index 93e5eab..2c8bcc8 100644 --- a/frontend/src/components/cover-grid/cover-grid.ts +++ b/frontend/src/components/cover-grid/cover-grid.ts @@ -430,10 +430,6 @@ export class CoverGrid override connectedCallback() { super.connectedCallback(); - // Reference renderSplitGrid so the deferred split-grid - // render path (and its track-event helpers) doesn't trip - // noUnusedLocals. Never invoked at runtime. - void this.renderSplitGrid; this.restoreSortPreferences(); this.loadAlbums(); @@ -1783,7 +1779,18 @@ export class CoverGrid `; } - const gridContent = this.renderSingleGrid(); + // The split path draws the dropdown between two grids. Until + // this was wired up, `render()` ignored `splitMode` entirely: + // pressing Enter on an album card fetched its tracks over the + // IPC, ran the whole split state machine (`splitMode: true`, + // `splitIndex: 6`, measured against the real container) and + // then drew the single grid regardless, so the only route from + // the albums grid to a track was the plain click that + // navigates away to the catalog page. + const gridContent = + this.splitMode && this.expandedTracks.length > 0 + ? this.renderSplitGrid() + : this.renderSingleGrid(); return html` ${this.renderPageHeader()} @@ -1821,10 +1828,12 @@ export class CoverGrid } /** - * Dual virtualizer — dropdown sandwiched between - * "before" and "after" grids. Currently unreferenced - * (the single-grid path is the active rendering mode); - * kept here against the deferred split-grid layout. + * Dual virtualizer — dropdown sandwiched between the "before" and + * "after" halves of the grid. + * + * Both halves carry the same listbox semantics as the single grid: + * they are one control to the user, and a selection that spans the + * dropdown must be announced the same way on either side of it. */ private renderSplitGrid() { const sm = this.scrollMgr; @@ -1836,6 +1845,9 @@ export class CoverGrid return html` entry.album.ID} @@ -1865,6 +1877,9 @@ export class CoverGrid ? html` entry.album.ID} @@ -1904,7 +1919,13 @@ export class CoverGrid > ${ctxMenu.contextMenuOpen ? html` -