diff --git a/backend/home/home.go b/backend/home/home.go index 915ae62..ae8052a 100644 --- a/backend/home/home.go +++ b/backend/home/home.go @@ -149,9 +149,24 @@ func (s *Service) GetShelves() ([]Shelf, error) { shelves := make([]Shelf, 0, 8) //nolint:mnd // rough capacity hint add := func(shelf Shelf, ok bool) { - if ok && len(shelf.Albums) > 0 { - shelves = append(shelves, shelf) + if !ok || len(shelf.Albums) == 0 { + return } + + // A repeat is only a fault if a different row was possible. A + // shelf showing the *entire* library answers every question with + // the same albums because there are no others, and suppressing + // those rows punishes a small library for being small — measured + // against a fixed shelf size instead, this let the fixture + // library keep three identical shelves while a library one album + // larger lost them. + if len(shelf.Albums) < len(albums) && + len(shelves) > 0 && + duplicates(shelf, shelves[len(shelves)-1]) { + return + } + + shelves = append(shelves, shelf) } add(s.recentlyPlayed(ctx, byID)) @@ -169,6 +184,56 @@ func (s *Service) GetShelves() ([]Shelf, error) { return shelves, nil } +// duplicateThreshold is the share of a shelf that has to be in the +// shelf above before the second one is not worth showing. Two thirds: +// one album in common between two rows of four is a coincidence, three +// is the same row with a different heading. +const duplicateThreshold = 2.0 / 3.0 + +// duplicateMinimum is the shortest shelf this rule judges at all. +// +// Below it the ratio says nothing: two rows of one album overlap by +// 100% whenever they agree at all. The first version of this had no +// floor and no library-size guard, and collapsed a four-album library +// to a single shelf — caught by the existing tests, not by the one +// written for the change. +const duplicateMinimum = 3 + +// duplicates reports whether a shelf is substantially the shelf above +// it wearing a different reason. +// +// A small library has one signal, not six: everything recently played +// is also everything most played is also everything recently added, so +// "Pick up where you left off" and "On repeat" render the same four +// covers in a different order and the page reads as repeating itself +// (H-9). This is the same rule as omitting an empty shelf, one step +// further: a shelf has to be worth its own row. +// +// Only the shelf immediately above is compared, deliberately. Two rows +// that share content are only jarring when they are adjacent, and +// comparing against everything already shown would delete the genre and +// random shelves on any library small enough to reach this at all. +func duplicates(shelf, previous Shelf) bool { + if len(shelf.Albums) < duplicateMinimum { + return false + } + + above := make(map[int64]struct{}, len(previous.Albums)) + for _, album := range previous.Albums { + above[album.ID] = struct{}{} + } + + shared := 0 + + for _, album := range shelf.Albums { + if _, ok := above[album.ID]; ok { + shared++ + } + } + + return float64(shared)/float64(len(shelf.Albums)) >= duplicateThreshold +} + // resolve turns album ids into albums, dropping any the library no // longer has (a scan can remove an album between the two queries). func resolve(ids []int64, byID map[int64]library.Album) []library.Album { diff --git a/backend/home/home_test.go b/backend/home/home_test.go index db1c564..0e0ac42 100644 --- a/backend/home/home_test.go +++ b/backend/home/home_test.go @@ -257,3 +257,82 @@ func TestGetShelvesSkipsAlbumsTheLibraryNoLongerHas(t *testing.T) { } } } + +// A shelf has to be worth its own row. +// +// A library with one signal answers several questions with the same +// albums, so shelves render the same covers in different orders under +// different reasons and the page reads as repeating itself (H-9). +// Confirmed in the running app on the fixture library before this +// existed: "On repeat" was "Pick up where you left off" reordered. +// +// The library has to be bigger than one shelf for the rule to apply at +// all, which is the point: a repeat is only a fault if a different row +// was possible. +func TestGetShelvesOmitsAShelfThatRepeatsTheOneAboveIt(t *testing.T) { + db := database.NewTestDB(t) + + albums := make([]library.Album, 0, 16) + + // Four albums carry the whole play history, so "what you played + // last" and "what you play most" can only answer with those four. + for i := range 4 { + name := "Played " + string(rune('A'+i)) + id := seed(t, db, name, "Solo", "", 50+i, "2026-08-1"+string(rune('0'+i))+" 00:00:00") + + albums = append(albums, library.Album{ID: id, Name: name, ArtistName: "Solo"}) + } + + // …and ten more the user has never touched, so the library is + // larger than a single shelf and a different row was possible. + for i := range 10 { + name := "Quiet " + string(rune('A'+i)) + id := seed(t, db, name, "Nobody", "", 0, "") + + albums = append(albums, library.Album{ID: id, Name: name, ArtistName: "Nobody"}) + } + + svc := home.NewService(slog.Default(), db, fakeLibrary{albums: albums}) + + shelves, err := svc.GetShelves() + if err != nil { + t.Fatalf("GetShelves: %v", err) + } + + // The first of a duplicate pair survives: the reason the user sees + // is the one that came first, not the one built last. + if !hasKind(shelves, home.KindRecentlyPlayed) { + t.Fatalf("expected a recently-played shelf, got %v", shelfKinds(shelves)) + } + + // And the page still has somewhere to start. + if len(shelves) < 2 { + t.Fatalf("suppression left %d shelves: %v", len(shelves), shelfKinds(shelves)) + } + + for i := 1; i < len(shelves); i++ { + above := shelves[i-1] + shared := 0 + + for _, a := range shelves[i].Albums { + for _, b := range above.Albums { + if a.ID == b.ID { + shared++ + + break + } + } + } + + if len(shelves[i].Albums) < 3 { + continue + } + + if ratio := float64(shared) / float64(len(shelves[i].Albums)); ratio >= 2.0/3.0 { + t.Errorf( + "shelf %q repeats %q (%d of %d albums)", + shelves[i].Kind, above.Kind, shared, len(shelves[i].Albums), + ) + } + } +} diff --git a/e2e/specs/library.spec.ts b/e2e/specs/library.spec.ts index 55ce75f..6aed304 100644 --- a/e2e/specs/library.spec.ts +++ b/e2e/specs/library.spec.ts @@ -17,6 +17,7 @@ test.describe('library views', () => { // library exists, so its presence proves nothing. Playwright's own // actionability check fails a covered click with "intercepts pointer // events", which is exactly the condition worth catching. + await app.getByTestId('nav-tracks').click({ timeout: 5_000 }); await expect(app.getByTestId('track-row').first()).toBeVisible(); await app.getByTestId('nav-artists').click({ timeout: 5_000 }); @@ -30,6 +31,8 @@ test.describe('library views', () => { app, testctl, }) => { + await app.getByTestId('nav-tracks').click(); + const health = await testctl.health(); const rows = app.getByTestId('track-row'); @@ -42,6 +45,27 @@ test.describe('library views', () => { await expect(app.getByText('مرحبا بالعالم')).toBeVisible(); }); + test('opens on Home, which is the page built to answer what to play', async ({ + app, + }) => { + // H-8: the app opened on Tracks — an alphabetical list of + // everything, the one entry point that is identical every time and + // therefore gives the user nothing to start from. Home is listed + // first in the nav and was never what anybody saw. + await expect(app.getByTestId('main-content')).toHaveAttribute( + 'data-active-view', + 'home', + ); + + // …and the sidebar agrees. It does not hear a `navigate` it did not + // send, so this is a separate claim from the one above and was + // briefly false. + await expect(app.getByTestId('nav-home')).toHaveAttribute( + 'aria-current', + 'page', + ); + }); + test('the sidebar navigates between primary views', async ({ app }) => { const main = app.getByTestId('main-content'); @@ -62,8 +86,14 @@ test.describe('library views', () => { 'artists', ); - await expect(app.getByText('Aurora Fields').first()).toBeVisible(); - await expect(app.getByText('Pale Circuit').first()).toBeVisible(); + // Scoped to the view: every primary view stays in the DOM, and + // Home's shelves name the same artists — so an unscoped `.first()` + // matches a card on a page that is `.view-hidden`, and asserting it + // is visible fails for a reason that has nothing to do with Artists. + const artists = app.locator('artists-view'); + + await expect(artists.getByText('Aurora Fields').first()).toBeVisible(); + await expect(artists.getByText('Pale Circuit').first()).toBeVisible(); }); test('a library can be renamed from its own name', async ({ app }) => { diff --git a/e2e/specs/playback.spec.ts b/e2e/specs/playback.spec.ts index 613f906..6cd8959 100644 --- a/e2e/specs/playback.spec.ts +++ b/e2e/specs/playback.spec.ts @@ -23,6 +23,9 @@ const longRow = (app: import('@playwright/test').Page) => */ test.describe('playback', () => { test.beforeEach(async ({ app }) => { + // The app lands on Home now (H-8), so the track list is a + // navigation away rather than the first thing on screen. + await app.getByTestId('nav-tracks').click(); await callBinding(app, 'queue.Queue.Clear').catch(() => { /* older builds may not expose Clear; the specs below do not need it */ }); @@ -84,6 +87,11 @@ test.describe('playback', () => { }); test.describe('queue', () => { + test.beforeEach(async ({ app }) => { + // The app lands on Home now (H-8). + await app.getByTestId('nav-tracks').click(); + }); + test('playing a track populates the queue panel', async ({ app }) => { await resetEvents(app); await longRow(app).dblclick(); diff --git a/e2e/specs/player-truth.spec.ts b/e2e/specs/player-truth.spec.ts index 3220612..8abffd4 100644 --- a/e2e/specs/player-truth.spec.ts +++ b/e2e/specs/player-truth.spec.ts @@ -95,6 +95,8 @@ async function blurDeepActive(app: Page): Promise { test.describe('the player reports its real position', () => { test.beforeEach(async ({ app }) => { + // The app lands on Home now (H-8); these specs start from a row. + await app.getByTestId('nav-tracks').click(); await callBinding(app, 'queue.Queue.Clear'); await resetEvents(app); }); diff --git a/e2e/specs/view-lifecycle.spec.ts b/e2e/specs/view-lifecycle.spec.ts index bb53faf..7bd9d08 100644 --- a/e2e/specs/view-lifecycle.spec.ts +++ b/e2e/specs/view-lifecycle.spec.ts @@ -123,6 +123,11 @@ test.describe('keyboard reach', () => { test('tabs out of the header straight into the sidebar', async ({ app, }) => { + // On Tracks, because the header search box is disabled on Home — + // it keeps its slot everywhere and says why, but a disabled input + // cannot hold focus, and this spec is about what follows it. + await app.getByTestId('nav-tracks').click(); + // Started from the search box rather than from the top of the page: // the header's leading controls come and go (the index-status button // is only there while the index builds), so counting stops from the diff --git a/frontend/index.ts b/frontend/index.ts index 47fbb51..1f5c4ca 100644 --- a/frontend/index.ts +++ b/frontend/index.ts @@ -478,4 +478,24 @@ if (queueButton && queuePanel) { void Player.EmitCurrentState(); void Queue.EmitCurrentState(); +// --------------------------------------------------------------- +// Land on Home +// --------------------------------------------------------------- +// The app opened on Tracks — an alphabetical list of everything, which +// is the one entry point that is identical every time and therefore +// gives the user nothing to start from. Home is listed first in the nav +// and is the page built to answer "what should I play", and it was +// never what anybody saw (H-8). +// +// index.html still renders the track list eagerly and it is still what +// paints first: it is the cached 'tracks' view, so this navigation is a +// class toggle plus one chunk, not a second render of the shell. Doing +// it here rather than by changing the markup keeps the first paint +// exactly as Phase 4 left it. +document.dispatchEvent( + new CustomEvent('navigate', { + detail: { view: 'home' }, + }), +); + warmViewChunks(); diff --git a/frontend/src/components/home-view/home-view.ts b/frontend/src/components/home-view/home-view.ts index ec9935c..a027e88 100644 --- a/frontend/src/components/home-view/home-view.ts +++ b/frontend/src/components/home-view/home-view.ts @@ -137,6 +137,28 @@ export class HomeView extends ViewLifecycleMixin(LitElement) { display: block; } + /* An album with no cover used to be a small dim icon on a + surface the same colour as the page, so a shelf read as + having holes in it (H-9) — while the Albums and Artists + grids both drew a letter tile. This is that tile, and the + gradient is what makes it a tile rather than a gap. */ + .art .placeholder { + width: 100%; + height: 100%; + display: flex; + align-items: center; + justify-content: center; + background: linear-gradient( + 135deg, + var(--yj-bg-overlay, #404040) 0%, + var(--yj-bg-surface, #282828) 100% + ); + color: var(--yj-text-secondary, #b3b3b3); + font-size: 48px; + font-weight: 300; + user-select: none; + } + .play { position: absolute; right: 8px; @@ -238,6 +260,11 @@ export class HomeView extends ViewLifecycleMixin(LitElement) { override render() { return html` + void this.load()} > - Shuffle + Shuffle suggestions

Somewhere to start listening.

@@ -304,8 +331,10 @@ export class HomeView extends ViewLifecycleMixin(LitElement) { >
${art - ? html`` - : html``} + ? html`` + : html``}