From 4f7529c3155a233a402015920ae60f3176c8ee52 Mon Sep 17 00:00:00 2001 From: Logan Date: Wed, 19 Aug 2026 22:59:55 -0400 Subject: [PATCH 1/2] test(queue): pin the panel's mouse model, and bound the highlight MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Single click selects, ctrl and shift extend, double click plays from that row — all four already worked, and nothing in either tier pinned any of them, which is why the report could be made and could not be settled. `queue-reorder.spec.ts` covers the keyboard and `queue-overlay.spec.ts` the panel's mode; the pointer path had no coverage at all, so "selection is broken here" and "selection is fine here" were equally consistent with a green suite. Measured with real mouse events rather than dispatched ones, because a synthetic click aimed at the row bypasses the only thing that could be swallowing it: click row 1 selects 1, ctrl+click 4 gives 1 and 4, shift+click 7 extends to 1,4,5,6,7, a plain click collapses to one, and a double click on row 3 leaves the backend playing row 3. The three candidates the issue lists are all answered. The repaint was already correct, and already correct on the day the issue was filed. `resolveTrackIndexFromEvent` reads data-index, and DOM order matches data order. A row control does swallow the click — `explore-link` stops propagation on purpose, so a click on a name navigates and selects nothing — but a hit-scan across a row makes the queue 12% link against the track list's 21%, so the panel called broken is *less* covered by links than the list called correct. That measurement killed the fix this started out as. Two traps are written into the spec because both faked a defect while measuring. Fixture tracks are 2 seconds, so "double click row 3" read a moment later reports whatever auto-advance moved on to — recorded twice as an off-by-one that is not one, which is what `LONG_TRACK` exists for. And the selection assertions are bounded at 500ms rather than polled with the default 5s: `queue-panel` repaints two ways, the explicit `requestUpdate()` and a per-render `keyFunction` arrow, and with *both* removed the highlight still arrives — at 134ms, 3.9s and 5.8s against 5-17ms healthy. Four seconds is indistinguishable from broken to a user and invisible to a generous poll. Mutation-tested rather than trusted: `playAtIndex(index + 1)` fails both double-click tests, treating every click as ctrl+click fails both selection tests, and removing both repaint mechanisms fails all three selection tests — the last only because of the bound. Closes #43 --- e2e/specs/queue-selection.spec.ts | 271 ++++++++++++++++++ .../src/components/queue-panel/queue-panel.ts | 20 ++ 2 files changed, 291 insertions(+) create mode 100644 e2e/specs/queue-selection.spec.ts diff --git a/e2e/specs/queue-selection.spec.ts b/e2e/specs/queue-selection.spec.ts new file mode 100644 index 0000000..6c37eae --- /dev/null +++ b/e2e/specs/queue-selection.spec.ts @@ -0,0 +1,271 @@ +import { + test, + expect, + callBinding, + navigateTo, + LONG_TRACK, + NO_QUEUE_SOURCE, +} from '../support/fixtures.js'; +import type { Page } from '@playwright/test'; + +/** + * The queue panel's mouse model (#43): single click selects, ctrl and + * shift extend, double click plays from that row. + * + * **All four already worked, and nothing pinned any of them** — which is + * the whole reason the report could be made and could not be settled. + * `queue-reorder.spec.ts` covers the keyboard, `queue-overlay.spec.ts` + * covers the panel's mode, and the component tier has the reorder + * arithmetic; the pointer path had no coverage in either tier, so + * "selection is broken here" and "selection is fine here" were equally + * consistent with a green suite. + * + * Two things this spec is deliberately shaped around. + * + * **The clicks are real.** A `dispatchEvent(new MouseEvent('click'))` + * on a row exercises the delegated handler and *not* the question being + * asked, which is what the pointer lands on: the rows carry + * `explore-link` names that take their own clicks, and a synthetic + * event aimed at the row reports a selection the mouse would never have + * produced. Every click here goes through Playwright. + * + * **The playing assertions use the 90-second fixture.** Every other + * track is 2–6 seconds, so "double click plays row 3" read against a + * 2-second track reports whatever auto-advance moved on to — measured + * during this work as row 3 double-clicked and row 4 playing, which + * reads exactly like an off-by-one in `PlayIndex` and is not one. + */ + +/** + * How long a click may take to show up as a highlight. + * + * **A poll with the default 5s timeout cannot see this defect**, and + * that is the point of naming it. `queue-panel` repaints its rows two + * ways — `onSelectionChanged()` calls `virtualizer.requestUpdate()`, + * and `.keyFunction` is a per-render arrow, which is itself a changed + * property the virtualizer reacts to. With **both** removed the + * highlight still arrives, on whatever unrelated render happens next: + * measured at 134ms, 3,866ms and 5,816ms for three clicks, against + * 5ms, 16ms and 17ms on a healthy build. + * + * A user cannot tell "four seconds late" from "broken", which is very + * close to what this issue reports. So the assertion is that the + * highlight is *prompt*, with a bound ~30x the measured healthy case + * and an order of magnitude under the degraded one. + */ +const HIGHLIGHT_MS = 500; + +/** The queue's own answer, never the DOM's. */ +async function playing(app: Page): Promise<{ index: number; title: string }> { + const state = await callBinding<{ + currentIndex: number; + tracks: { title: string }[]; + }>(app, 'queue.Queue.GetState'); + + return { + index: state.currentIndex, + title: state.tracks[state.currentIndex]?.title ?? '', + }; +} + +/** Which rows are selected, as the accessibility tree sees it. */ +const selected = (app: Page) => + app.evaluate(() => + [ + ...document + .querySelector('queue-panel')! + .shadowRoot!.querySelectorAll('[data-index]'), + ] + .filter((row) => row.getAttribute('aria-selected') === 'true') + .map((row) => Number((row as HTMLElement).dataset['index'])), + ); + +/** + * Six tracks with the long one in the middle, so a "play from here" + * assertion has something to land on that will still be playing when it + * is read back. + */ +async function queueSixAndOpen(app: Page): Promise { + const paths = await app.evaluate(async (longTitle) => { + // `TrackName`, not `Title`: the library model names it after the + // tag, and the *queue* is what calls it `title`. + const tracks = (await window.__yjEvents.call( + 'library.Library.GetTracks', + [0], + 10_000, + )) as { FilePath: string; TrackName: string }[]; + + const long = tracks.find((t) => t.TrackName === longTitle); + const rest = tracks.filter((t) => t.TrackName !== longTitle).slice(0, 5); + + // Index 3 is the long one: far enough down that a shift-extend has + // room either side of it. + return [ + ...rest.slice(0, 3).map((t) => t.FilePath), + long!.FilePath, + ...rest.slice(3).map((t) => t.FilePath), + ]; + }, LONG_TRACK); + + await callBinding(app, 'queue.Queue.SetQueue', [ + paths, + 0, + false, + NO_QUEUE_SOURCE, + ]); + + // A closed panel renders no list at all. + await app.locator('#queue-button').click(); + await expect(app.locator('queue-panel .track-item').first()).toBeVisible(); + await expect(app.locator('queue-panel .track-item')).toHaveCount(6); +} + +/** The row at a data-index, not the nth child: see the note in the file. */ +const row = (app: Page, index: number) => + app.locator(`queue-panel .track-item[data-index="${index}"]`); + +test.describe('selecting in the queue with a mouse', () => { + // The suite shares one backend in file order, and a queue and an open + // panel both outlive the page. `queue-reorder.spec.ts` sets the + // precedent and the reason: a spec that spends state fails the next + // one, in a list that reads like a regression in whatever you hold. + test.afterEach(async ({ app }) => { + await callBinding(app, 'queue.Queue.Clear').catch(() => { + /* an empty queue is the state we were asking for */ + }); + + const open = await app.locator('queue-panel[open]').count(); + + if (open > 0) await app.locator('#queue-button').click(); + }); + + test('a single click selects that row and only that row', async ({ app }) => { + await queueSixAndOpen(app); + + await row(app, 1).click(); + await expect + .poll(() => selected(app), { timeout: HIGHLIGHT_MS }) + .toEqual([1]); + + // And it *replaces* rather than accumulating, which is the half a + // test of one click cannot see. + await row(app, 4).click(); + await expect + .poll(() => selected(app), { timeout: HIGHLIGHT_MS }) + .toEqual([4]); + }); + + test('ctrl adds a row and shift extends a range', async ({ app }) => { + await queueSixAndOpen(app); + + await row(app, 1).click(); + await row(app, 3).click({ modifiers: ['Control'] }); + await expect + .poll(() => selected(app), { timeout: HIGHLIGHT_MS }) + .toEqual([1, 3]); + + // From the last row touched, so 3→5, keeping the ctrl-picked 1. + await row(app, 5).click({ modifiers: ['Shift'] }); + await expect + .poll(() => selected(app), { timeout: HIGHLIGHT_MS }) + .toEqual([1, 3, 4, 5]); + + // A plain click collapses the whole thing back to one. + await row(app, 2).click(); + await expect + .poll(() => selected(app), { timeout: HIGHLIGHT_MS }) + .toEqual([2]); + }); + + test('a double click plays from that row', async ({ app }) => { + await queueSixAndOpen(app); + + // Row 3 is the 90-second track. Asked of the backend, because the + // panel's own highlight is a different claim. + await row(app, 3).dblclick(); + + await expect.poll(() => playing(app)).toEqual({ + index: 3, + title: LONG_TRACK, + }); + + // Playing is not selecting: the double click clears the selection + // it made on the way through, or every play leaves a row looking + // picked out for an action the user did not ask for. + await expect.poll(() => selected(app)).toEqual([]); + }); + + /** + * The one collision the report is actually about. + * + * Every track, album and artist name in the app navigates + * (`utils/explore-link.ts`), and it does that by **stopping the + * click's propagation** — in its own words, "the row must not also + * treat it as a selection". So a click that lands on the name text + * navigates and selects nothing, in the queue panel and in the track + * list alike. + * + * That is deliberate and it is pinned here rather than argued with, + * because the measurement says the queue is not the surface where it + * hurts: a horizontal hit-scan of a row at three heights makes the + * queue row **12%** link and the track list's row **21%** — the panel + * the report calls broken is *less* covered by links than the list it + * calls correct. What is left is one deliberate exception, and a + * change to it should have to fail a test. + */ + test('a click on a name navigates instead, and that is the exception', async ({ + app, + }) => { + await queueSixAndOpen(app); + + await row(app, 1).click(); + await expect + .poll(() => selected(app), { timeout: HIGHLIGHT_MS }) + .toEqual([1]); + + await row(app, 2).locator('.explore-link').first().click(); + + await expect(app.getByTestId('main-content')).toHaveAttribute( + 'data-active-view', + 'explore-album-details', + ); + + // Row 2 did not join the selection — the link took the click. + await expect.poll(() => selected(app)).toEqual([1]); + + await navigateTo(app, 'tracks'); + }); + + /** + * And the other half of that bargain: the link holds its navigation + * for one double-click interval and drops it if a second click + * arrives, so double-clicking a *name* still plays the row rather + * than navigating away from it. That is what makes the exception + * above survivable, and it is the part most likely to break silently + * if the grace interval is ever removed. + */ + test('a double click on a name plays rather than navigating', async ({ + app, + }) => { + await queueSixAndOpen(app); + + // Read rather than assumed: which view the app lands on is the + // user's `DefaultPage`, so naming one here would be asserting on a + // config value in a test about a double click. + const before = await app + .getByTestId('main-content') + .getAttribute('data-active-view'); + + await row(app, 3).locator('.explore-link').first().dblclick(); + + await expect.poll(() => playing(app)).toEqual({ + index: 3, + title: LONG_TRACK, + }); + + await expect(app.getByTestId('main-content')).toHaveAttribute( + 'data-active-view', + before!, + ); + }); +}); diff --git a/frontend/src/components/queue-panel/queue-panel.ts b/frontend/src/components/queue-panel/queue-panel.ts index 397785e..86434c3 100644 --- a/frontend/src/components/queue-panel/queue-panel.ts +++ b/frontend/src/components/queue-panel/queue-panel.ts @@ -277,6 +277,26 @@ export class QueuePanel return this.queue.tracks.length; } + /** + * Repaint the rows when the selection changes. + * + * `` renders through the `virtualize` directive, + * which reacts to its *own* properties and not to the host having + * re-rendered, so host state like a selection reaches the rows only + * if it is pushed. `track-list` has always done this and both + * playlist views had to be taught it. + * + * **There is a second, accidental mechanism here and it must not be + * mistaken for this one**: `.keyFunction` below is a per-render + * arrow, so it is a changed property on every host update and + * repaints the rows by itself. Removing *either* alone changes + * nothing observable, which is why #43 could not be settled by + * reading the code. With both gone the highlight still arrives — + * on whatever unrelated render happens next, measured at 134ms, + * 3,866ms and 5,816ms against 5–17ms healthy, which a user cannot + * tell from broken. `queue-selection.spec.ts` asserts the + * *promptness* rather than the eventual state for that reason. + */ onSelectionChanged(): void { this.virtualizer?.requestUpdate(); } From 70ab3ddf948262b17efbad546d0da0a6a04597f9 Mon Sep 17 00:00:00 2001 From: Logan Date: Wed, 19 Aug 2026 23:00:03 -0400 Subject: [PATCH 2/2] docs: record two measurements from the queue selection work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first is a second instance of a rule CLAUDE.md already states, with numbers: a virtualized list can be repainting for a reason you are about to delete, and here there are two such reasons — so removing either alone changes nothing observable, and removing both leaves the highlight seconds late rather than absent. That is the shape a poll cannot see, which is the general lesson worth keeping. The second is the hit-scan, because it stopped a wrong fix: the queue panel is 12% link and the track list 21%, which is the opposite of the assumption the fix was being built on. --- .planning/NOTES.md | 67 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/.planning/NOTES.md b/.planning/NOTES.md index aa40065..06379cd 100644 --- a/.planning/NOTES.md +++ b/.planning/NOTES.md @@ -3824,3 +3824,70 @@ Staging it is `/__test/emit` with a `JobsChanged` snapshot; a job with `state: "running"` never completes, so it stays up until an empty snapshot is emitted, which is what makes an idle re-measurement look like the fix not working. + +## Two repaint mechanisms, and neither is pinned alone (measured 2026-08-20) + +`CLAUDE.md` already states the rule — *a virtualized list repaints only +when you tell it to, and the accidental way you were telling it may be +the thing you are about to delete* — found in `artists-view` and +`genres-view`. `queue-panel` is a second instance with numbers, and the +numbers are the part worth keeping. + +It repaints its rows **two** ways: + +- `onSelectionChanged()` calls `virtualizer.requestUpdate()`, which is + the intended one and the one `track-list` has always had; +- `.keyFunction=${(track) => track.id}` is a **per-render arrow**, so it + is a changed property on every host update and repaints the rows by + itself. + +Removing *either* alone changes nothing observable. That is why #43 +could not be settled by reading the code: the hypothesis in its Findings +(the repaint is missing) was checkable, false, and would have looked +identical either way. + +Removing **both** does not break selection either — it delays it. Time +from click to `aria-selected`, three clicks each: + +| build | ms to highlight | +|---|---| +| healthy | 5, 16, 17 | +| both mechanisms removed | 134, 3,866, 5,816 | + +The highlight arrives on whatever unrelated render happens next (the +player's 1 Hz position report is the usual candidate). **Four seconds is +indistinguishable from broken to a user, and invisible to a spec** — +`expect.poll`'s default 5 s timeout passes the degraded build on every +assertion. `queue-selection.spec.ts` bounds its selection assertions at +500 ms for that reason, which is ~30x the healthy case and an order of +magnitude under the degraded one. + +The general form, for the next spec about anything push-driven: **a poll +generous enough to be stable is generous enough to miss a latency +regression entirely.** If "late" is a failure mode worth having, the +timeout has to say so. + +## A hit-scan says how much of a row is not selectable (measured 2026-08-20) + +`explore-link` stops the click's propagation on purpose — "the row must +not also treat it as a selection" — so a click on a track, album or +artist *name* navigates and selects nothing. That is app-wide and +deliberate, and the useful question about any given list is how much of +its row it costs. + +Asking `elementFromPoint` what is under each x across a row, at three +heights: + +| list | link coverage | +|---|---| +| queue panel | 12% | +| track list | 21% | + +This killed a fix in progress. #43 reads as "selection is broken in the +queue panel, and fine in the track list", the obvious mechanism is that +the queue's narrow rows are mostly name, and it is **wrong**: the panel +is *less* link-covered than the list it is being compared against. The +scan takes a minute and is worth running before demoting anybody's links +— `explore-album-details`'s tracklist (number / title / artist / +duration) is the one that plausibly *is* mostly link, and is the one +#5 is about to add selection to.