diff --git a/frontend/src/store/controllers/player-controller.ts b/frontend/src/store/controllers/player-controller.ts index 5edb89b..63eae27 100644 --- a/frontend/src/store/controllers/player-controller.ts +++ b/frontend/src/store/controllers/player-controller.ts @@ -1,5 +1,9 @@ import type { ReactiveController, ReactiveControllerHost } from 'lit'; -import type { PlayerState, TrackInfo } from '../player-store'; +import type { + PlayerState, + PositionInfo, + TrackInfo, +} from '../player-store'; import { playerStore } from '../player-store'; /** @@ -66,6 +70,11 @@ export class PlayerController implements ReactiveController { return this.state.muted; } + /** The backend's last position report, or null before the first. */ + get position(): PositionInfo | null { + return this.state.position; + } + // =================================================================== // ACTIONS // Delegate to store (which delegates to backend) diff --git a/frontend/src/store/explore-cache.ts b/frontend/src/store/explore-cache.ts index fc991e7..30d2b3c 100644 --- a/frontend/src/store/explore-cache.ts +++ b/frontend/src/store/explore-cache.ts @@ -10,9 +10,27 @@ * album detail page → check cache before API calls */ -import type { explore } from '@go/models'; -type MBReleaseGroup = explore.MBReleaseGroup; -type LBTopRecording = explore.LBTopRecording; +import { registerCacheProbe } from '../utils/cache-stats'; +import { LRUMap } from '../utils/lru-map'; + +/** + * Cap for artist entries (`perf.M8`). + * + * An entry's `imageURL` is the artist photo's base64 data URL — ~128 kB + * measured — and it is the *same string* `explore-view`'s own + * `artistImageCache` holds. Two unbounded maps referencing one string + * means bounding either alone frees nothing, so this constant is + * exported and both use it. Changing it here changes both. + */ +export const ARTIST_IMAGE_CACHE_LIMIT = 32; + +/** + * Cap for album entries. These are small — measured at 61 chars each, + * since `coverArt` is a local `/coverart/…` path rather than a data URL + * — so the cap is generous and exists to make the map bounded at all + * rather than because it costs anything today. + */ +const ALBUM_CACHE_LIMIT = 512; /** Cached artist data from search results. */ export interface CachedArtist { @@ -33,10 +51,8 @@ export interface CachedAlbum { } class ExploreCacheStore { - private artists = new Map(); - private albums = new Map(); - private artistAlbums = new Map(); - private artistTopTracks = new Map(); + private artists = new LRUMap(ARTIST_IMAGE_CACHE_LIMIT); + private albums = new LRUMap(ALBUM_CACHE_LIMIT); // -- Artists -- @@ -58,26 +74,6 @@ class ExploreCacheStore { return this.albums.get(mbid); } - // -- Artist → Albums (release groups) -- - - setArtistAlbums(artistMBID: string, albums: MBReleaseGroup[]) { - if (artistMBID) this.artistAlbums.set(artistMBID, albums); - } - - getArtistAlbums(artistMBID: string): MBReleaseGroup[] | undefined { - return this.artistAlbums.get(artistMBID); - } - - // -- Artist → Top tracks -- - - setArtistTopTracks(artistMBID: string, tracks: LBTopRecording[]) { - if (artistMBID) this.artistTopTracks.set(artistMBID, tracks); - } - - getArtistTopTracks(artistMBID: string): LBTopRecording[] | undefined { - return this.artistTopTracks.get(artistMBID); - } - // -- Bulk populate from search results -- populateFromSearch(artists: any[], releaseGroups: any[]) { @@ -104,6 +100,34 @@ class ExploreCacheStore { } } } + + /** Entries and retained string length, for `window.__yjCacheStats()`. */ + stats() { + const size = (o: object) => { + let n = 0; + + for (const v of Object.values(o)) { + if (typeof v === 'string') n += v.length; + } + + return n; + }; + const of = (m: LRUMap) => { + let chars = 0; + + for (const v of m.values()) chars += size(v); + + return { entries: m.size, chars, limit: m.limit }; + }; + + return { + artists: of(this.artists as LRUMap), + albums: of(this.albums as LRUMap), + }; + } } export const exploreCache = new ExploreCacheStore(); + +registerCacheProbe('exploreCache.artists', () => exploreCache.stats().artists); +registerCacheProbe('exploreCache.albums', () => exploreCache.stats().albums); diff --git a/frontend/src/store/favorites-store.ts b/frontend/src/store/favorites-store.ts index 08d96b4..fc470e9 100644 --- a/frontend/src/store/favorites-store.ts +++ b/frontend/src/store/favorites-store.ts @@ -15,9 +15,28 @@ import { SetPinDefaultPlaylist, } from '@go/config/Config'; import { Events } from '../events'; +import { describeError } from '@utils/describe-error'; +import { notificationStore } from './notification-store'; export type IconStyle = 'heart' | 'star'; +/** + * A revert the user can see is not an explanation (errors.m2): the + * heart fills, and half a second later it empties again. Transient by + * the plan's rule — the state has already put itself back, so there is + * nothing to do but say why. + */ +function reportRevert(what: string, err: unknown): void { + console.error(`favorites: ${what} failed`, err); + notificationStore.transient({ + key: 'favorites', + text: `Could not ${what}. ${describeError(err)}`, + detail: String(err), + coalescedText: (count) => + `Could not ${what} — ${count} changes were undone.`, + }); +} + export interface FavoritesState { playlistId: number; playlistName: string; @@ -152,7 +171,7 @@ class FavoritesStore { try { await ToggleDefaultPlaylistTrack(filePath); - } catch { + } catch (err) { // Revert optimistic update. if (wasIn) { this.favoritedPaths.add(filePath); @@ -161,6 +180,10 @@ class FavoritesStore { } this.notify(); + reportRevert( + wasIn ? 'remove that favourite' : 'save that favourite', + err, + ); } } @@ -175,8 +198,9 @@ class FavoritesStore { try { await AddToDefaultPlaylist(filePaths); - } catch { + } catch (err) { void this.loadPaths(); + reportRevert('save those favourites', err); } } @@ -191,8 +215,9 @@ class FavoritesStore { try { await RemoveFromDefaultPlaylist(filePaths); - } catch { + } catch (err) { void this.loadPaths(); + reportRevert('remove those favourites', err); } } diff --git a/frontend/src/store/job-store.ts b/frontend/src/store/job-store.ts index f86fc8e..f4d8603 100644 --- a/frontend/src/store/job-store.ts +++ b/frontend/src/store/job-store.ts @@ -27,7 +27,7 @@ export type JobState = | 'error'; /** Job kinds. Mirrors backend/jobs.Kind. */ -export type JobKind = 'library-scan' | 'index-build'; +export type JobKind = 'library-scan' | 'index-build' | 'autotag-apply'; type Subscriber = () => void; diff --git a/frontend/src/store/library-store.ts b/frontend/src/store/library-store.ts index 449f4d2..0a72db9 100644 --- a/frontend/src/store/library-store.ts +++ b/frontend/src/store/library-store.ts @@ -57,6 +57,17 @@ class LibraryStore { private subscribers = new Set(); private notifyScheduled = false; + /** + * The one in-flight request per collection. + * + * Concurrent readers used to be deduplicated by deriving a promise + * from subscriber notifications, which never settled when the fetch + * *failed* (errors.M1) — the waiter tested for "loaded and not + * loading", and a failed fetch is neither. Holding the request + * itself makes every waiter settle exactly as the fetch did. + */ + private inFlight = new Map>(); + /** * Monotonic counter incremented only when actual data changes * (not loading flag transitions). Subscribers can compare against @@ -64,6 +75,15 @@ class LibraryStore { */ private changeGen = 0; + /** + * Incremented only by invalidation — a scan, a retag, or a library + * filter switch. A fetch captures it at request time and discards + * its answer if it no longer matches, which is what stops library + * A's tracks being cached while library B is selected (errors.C4). + * Separate from `changeGen`, which any collection landing bumps. + */ + private cacheGen = 0; + constructor() { EventsOn(Events.LibraryScanComplete, () => { this.invalidate(); @@ -85,6 +105,9 @@ class LibraryStore { EventsOn(Events.TrackMetadataChanged, () => { this.invalidate(); }); + EventsOn(Events.TrackPlayCountChanged, (payload: unknown) => { + this.applyPlayCount(payload); + }); this.loadCoverSize(); this.deferEagerFetch(); @@ -130,32 +153,87 @@ class LibraryStore { return this.albums; } + /** + * Track a fetch: guard its answer against an invalidation that + * happened while it was in flight, hold it as *the* in-flight + * request for its collection, and clear the loading flag when it + * settles either way. + * + * A stale answer is not cached and not returned — the caller is + * chained onto whatever the current selection is fetching instead, + * so "give me the tracks" always answers with the tracks of the + * library that is selected when it answers. + */ + private track( + slot: ViewName, + request: Promise, + commit: (value: T) => void, + current: () => Promise, + ): Promise { + const gen = this.cacheGen; + const guarded = request.then((value) => { + if (gen !== this.cacheGen) return current(); + + commit(value); + this.changeGen++; + + return value; + }); + const tracked: Promise = guarded.finally(() => { + if (this.inFlight.get(slot) !== tracked) return; + + this.inFlight.delete(slot); + this.setLoading(slot, false); + this.notify(); + }); + + this.inFlight.set(slot, tracked); + this.setLoading(slot, true); + this.notify(); + + return tracked; + } + + private pending(slot: ViewName): Promise | undefined { + return this.inFlight.get(slot) as Promise | undefined; + } + + private setLoading(slot: ViewName, loading: boolean): void { + switch (slot) { + case 'tracks': + this.tracksLoading = loading; + break; + case 'albums': + this.albumsLoading = loading; + break; + case 'artists': + this.artistsLoading = loading; + break; + case 'genres': + this.genresLoading = loading; + break; + } + } + async getTracks(): Promise { if (this.tracks !== null) { return this.tracks; } - if (this.tracksLoading) { - return this.waitForTracks(); - } + const pending = this.pending('tracks'); - this.tracksLoading = true; - this.notify(); + if (pending) return pending; - try { - const id = this.selectedLibraryIdValue; - const tracks = id !== null - ? await GetAllTracksByLibrary(id) - : await GetAllTracks(); + const id = this.selectedLibraryIdValue; - this.tracks = tracks; - this.changeGen++; - - return tracks; - } finally { - this.tracksLoading = false; - this.notify(); - } + return this.track( + 'tracks', + id !== null ? GetAllTracksByLibrary(id) : GetAllTracks(), + (tracks) => { + this.tracks = tracks; + }, + () => this.getTracks(), + ); } async getAlbums(): Promise { @@ -163,27 +241,20 @@ class LibraryStore { return this.albums; } - if (this.albumsLoading) { - return this.waitForAlbums(); - } + const pending = this.pending('albums'); - this.albumsLoading = true; - this.notify(); + if (pending) return pending; - try { - const id = this.selectedLibraryIdValue; - const albums = id !== null - ? await GetAllAlbumsByLibrary(id) - : await GetAllAlbums(); + const id = this.selectedLibraryIdValue; - this.albums = albums; - this.changeGen++; - - return albums; - } finally { - this.albumsLoading = false; - this.notify(); - } + return this.track( + 'albums', + id !== null ? GetAllAlbumsByLibrary(id) : GetAllAlbums(), + (albums) => { + this.albums = albums; + }, + () => this.getAlbums(), + ); } async getArtists(): Promise { @@ -191,27 +262,20 @@ class LibraryStore { return this.artists; } - if (this.artistsLoading) { - return this.waitForArtists(); - } + const pending = this.pending('artists'); - this.artistsLoading = true; - this.notify(); + if (pending) return pending; - try { - const id = this.selectedLibraryIdValue; - const artists = id !== null - ? await GetAllArtistsByLibrary(id) - : await GetAllArtists(); + const id = this.selectedLibraryIdValue; - this.artists = artists; - this.changeGen++; - - return artists; - } finally { - this.artistsLoading = false; - this.notify(); - } + return this.track( + 'artists', + id !== null ? GetAllArtistsByLibrary(id) : GetAllArtists(), + (artists) => { + this.artists = artists; + }, + () => this.getArtists(), + ); } async getGenres(): Promise { @@ -219,27 +283,22 @@ class LibraryStore { return this.genres; } - if (this.genresLoading) { - return this.waitForGenres(); - } + const pending = this.pending('genres'); - this.genresLoading = true; - this.notify(); + if (pending) return pending; - try { - const id = this.selectedLibraryIdValue; - const genres = id !== null - ? await GetAllGenresWithCountsByLibrary(id) - : await GetAllGenresWithCounts(); + const id = this.selectedLibraryIdValue; - this.genres = genres; - this.changeGen++; - - return genres; - } finally { - this.genresLoading = false; - this.notify(); - } + return this.track( + 'genres', + id !== null + ? GetAllGenresWithCountsByLibrary(id) + : GetAllGenresWithCounts(), + (genres) => { + this.genres = genres; + }, + () => this.getGenres(), + ); } async getAlbumsByArtist( @@ -442,11 +501,75 @@ class LibraryStore { // INVALIDATION // =================================================================== + /** + * Patch one track's play statistics in place. + * + * Finishing a track used to arrive as TrackMetadataChanged, so every + * song invalidated the whole cache and refetched tracks, albums, + * artists and genres — ~37 MB across the IPC and ~0.8 s of blocked + * main thread per song at 50 000 tracks (perf.C1), and it cleared + * the user's track-list selection while it did (perf.C2). + * + * A play count changes one integer on one row. The backend sends + * everything needed to write it, so nothing is refetched and no + * collection identity changes except the tracks array itself. + * + * The array *is* replaced rather than mutated: consumers key their + * memoized filter/sort caches on its identity (`track-list`'s + * `cachedTracks` check is the load-bearing one), so an in-place + * mutation would be invisible to every one of them. The individual + * Track objects other than the patched one are shared, which is + * what keeps this cheap. + */ + private applyPlayCount(payload: unknown): void { + if (this.tracks === null) return; + + const p = payload as { + filePath?: string; + playCount?: number; + lastPlayed?: string; + } | null; + + if (!p?.filePath) return; + + const idx = this.tracks.findIndex((t) => t.FilePath === p.filePath); + + if (idx === -1) return; + + const existing = this.tracks[idx]; + + if (existing === undefined) return; + + const patched = Object.assign( + Object.create(Object.getPrototypeOf(existing) as object), + existing, + { + PlayCount: p.playCount ?? existing.PlayCount, + LastPlayed: p.lastPlayed ?? existing.LastPlayed, + }, + ) as library.Track; + + this.tracks = [ + ...this.tracks.slice(0, idx), + patched, + ...this.tracks.slice(idx + 1), + ]; + + this.changeGen++; + this.notify(); + } + private invalidate(): void { this.tracks = null; this.albums = null; this.artists = null; this.genres = null; + // Anything still in flight was asked for on behalf of a + // selection that no longer applies: forget it, so the eager + // refetch below starts a request for the current one rather + // than adopting the old one's answer. + this.inFlight.clear(); + this.cacheGen++; this.changeGen++; this.scrollPositions = { tracks: 0, albums: 0, artists: 0, genres: 0 }; this.notify(); @@ -461,10 +584,17 @@ class LibraryStore { * needing their own LibraryScanComplete listener. */ private eagerFetch(): void { - void this.getTracks(); - void this.getAlbums(); - void this.getArtists(); - void this.getGenres(); + // A failed fetch is reported by whichever view asked for the + // data (it is that panel's failure, not the app's), but the + // eager refetch has no caller to reject to — without a catch it + // is an unhandled rejection. + const logged = (what: string) => (err: unknown) => + console.error(`library: could not load ${what}`, err); + + void this.getTracks().catch(logged('tracks')); + void this.getAlbums().catch(logged('albums')); + void this.getArtists().catch(logged('artists')); + void this.getGenres().catch(logged('genres')); } // =================================================================== @@ -486,54 +616,6 @@ class LibraryStore { }); } - // =================================================================== - // HELPERS - // Wait for an in-flight fetch to complete. - // =================================================================== - - private waitForTracks(): Promise { - return new Promise((resolve) => { - const unsub = this.subscribe(() => { - if (!this.tracksLoading && this.tracks !== null) { - unsub(); - resolve(this.tracks); - } - }); - }); - } - - private waitForAlbums(): Promise { - return new Promise((resolve) => { - const unsub = this.subscribe(() => { - if (!this.albumsLoading && this.albums !== null) { - unsub(); - resolve(this.albums); - } - }); - }); - } - - private waitForArtists(): Promise { - return new Promise((resolve) => { - const unsub = this.subscribe(() => { - if (!this.artistsLoading && this.artists !== null) { - unsub(); - resolve(this.artists); - } - }); - }); - } - - private waitForGenres(): Promise { - return new Promise((resolve) => { - const unsub = this.subscribe(() => { - if (!this.genresLoading && this.genres !== null) { - unsub(); - resolve(this.genres); - } - }); - }); - } } // Singleton instance. diff --git a/frontend/src/store/player-store.ts b/frontend/src/store/player-store.ts index 6e41778..b462ca2 100644 --- a/frontend/src/store/player-store.ts +++ b/frontend/src/store/player-store.ts @@ -1,6 +1,10 @@ import { EventsOn } from '@runtime/runtime'; import { Events } from '../events'; import * as Player from '@go/player/Player'; +import { notificationStore } from './notification-store'; + +/** The region `` renders these in: the player bar. */ +export const PlayerRegion = 'player'; // TrackInfo mirrors the player.TrackInfo struct in the Go backend. // Fields are serialized as camelCase JSON via struct tags. @@ -23,6 +27,26 @@ export interface TrackInfo { recordingMbid: string; // MusicBrainz recording ID or empty string } +// PositionInfo mirrors player.PositionInfo in the Go backend: the +// player's own answer to "where are we", pushed once a second while +// playing and immediately after any seek, pause, resume or track +// change. +export interface PositionInfo { + positionSeconds: number; + trackLength: number; + trackChangeId: number; + seq: number; // increments per report, so an unchanged second is still a fresh reading + playing: boolean; +} + +// PlaybackFailure mirrors queue.PlaybackFailure. +export interface PlaybackFailure { + filePath: string; + title: string; + artist: string; + reason: string; +} + export interface PlayerState { // Cached from backend isPlaying: boolean; @@ -30,19 +54,30 @@ export interface PlayerState { volume: number; // 0-100 muted: boolean; // silenced independently of the volume level - // Frontend-only state (for future use) - // selectedTrackIds: Set; - // isQueuePanelOpen: boolean; + // The backend's position report. The seek bar renders this and + // interpolates only between reports; it does not count. + position: PositionInfo | null; } type Subscriber = () => void; +/** + * Bindings are fire-and-forget by design — the backend reports what + * happened through events, not return values — but a rejected bridge + * call still has to land somewhere other than an unhandled rejection + * (errors.m1). + */ +function reportBindingFailure(name: string): (err: unknown) => void { + return (err: unknown) => console.error(`${name} failed`, err); +} + class PlayerStore { private state: PlayerState = { isPlaying: false, currentTrack: null, volume: 50, muted: false, + position: null, }; private subscribers = new Set(); @@ -66,6 +101,41 @@ class PlayerStore { this.update({ currentTrack: trackInfo ?? null }); }); + EventsOn(Events.PlaybackPositionChanged, (position: PositionInfo) => { + this.update({ position }); + }); + + EventsOn(Events.PlaybackFailed, (failure: PlaybackFailure) => { + // The raw Go reason is a debugging tool, not a sentence; it stays + // in the console and rides along as `detail`. + console.error(`playback failed: ${failure.filePath}: ${failure.reason}`); + + const name = failure.title || failure.filePath; + + // Inline by the plan's own rule: the useful response to a track + // that will not play is to keep playing, which the backend is + // already doing by skipping it. + notificationStore.inline(PlayerRegion, { + key: 'playback-failed', + tone: 'warning', + text: `Could not play “${name}” — the file may have moved.`, + coalescedText: (count) => + `Skipped ${count} tracks that could not be played.`, + detail: failure.reason, + }); + }); + + EventsOn(Events.SeekFailed, () => { + // The backend re-reports its real position alongside this, so the + // optimistic move the seek bar made is already being taken back; + // all that is missing is saying why. + notificationStore.inline(PlayerRegion, { + key: 'seek-failed', + tone: 'warning', + text: 'Could not seek in this track.', + }); + }); + EventsOn(Events.PlaybackFinished, () => { this.update({ isPlaying: false }); // Queue auto-advance is handled by the backend queue package. @@ -94,23 +164,27 @@ class PlayerStore { // =================================================================== pause(): void { - Player.Pause(); + void Player.Pause().catch(reportBindingFailure('Player.Pause')); } loadTrack(filePath: string): void { - Player.LoadFile(filePath); + void Player.LoadFile(filePath).catch( + reportBindingFailure('Player.LoadFile'), + ); } seek(seconds: number): void { - Player.Seek(seconds); + void Player.Seek(seconds).catch(reportBindingFailure('Player.Seek')); } setVolume(level: number): void { - Player.SetVolume(level); + void Player.SetVolume(level).catch( + reportBindingFailure('Player.SetVolume'), + ); } toggleMute(): void { - void Player.MuteToggle(); + void Player.MuteToggle().catch(reportBindingFailure('Player.MuteToggle')); } // =================================================================== diff --git a/frontend/src/store/playlist-store.ts b/frontend/src/store/playlist-store.ts index 0d1523b..fbe4c69 100644 --- a/frontend/src/store/playlist-store.ts +++ b/frontend/src/store/playlist-store.ts @@ -1,5 +1,9 @@ import { EventsOn } from '@runtime/runtime'; -import { GetAllPlaylistsWithTracks } from '@go/playlist/Service'; +import { + GetAllPlaylists, + GetAllPlaylistsWithTracks, + GetPlaylistTracks, +} from '@go/playlist/Service'; import type { playlist } from '@go/models'; import { Events } from '../events'; @@ -10,6 +14,7 @@ class PlaylistStore { private playlistsLoading = false; private scrollPosition = 0; private subscribers = new Set(); + private notifyScheduled = false; constructor() { EventsOn(Events.LibraryScanComplete, () => { @@ -28,15 +33,29 @@ class PlaylistStore { this.invalidate(); }); - EventsOn(Events.PlaylistTracksChanged, () => { - this.invalidate(); - }); + // `PlaylistTracksChanged` carries the playlist that changed, + // and toggling one heart in the track list is by far its most + // frequent source. Answering it with a full invalidate meant + // `GetAllPlaylistsWithTracks` — every row of every playlist, + // with full track metadata — for a one-row edit: measured at + // 2.61 MB and 172 ms across ten 500-track playlists. Patch the + // one playlist instead; the id is right there. + EventsOn( + Events.PlaylistTracksChanged, + (playlistId?: number | null) => { + void this.patchPlaylist(playlistId); + }, + ); EventsOn(Events.PlaylistsRestored, () => { this.invalidate(); }); - void this.getPlaylists(); + // Deliberately no eager fetch here. This store is a singleton + // constructed at import time, so warming it put every track of + // every playlist on the path to first paint — for a view the + // user may never open. `getPlaylists()` fetches on first + // access, and `playlist-view` awaits it when it loads. } // =================================================================== @@ -125,7 +144,115 @@ class PlaylistStore { this.playlists = null; this.scrollPosition = 0; this.notify(); - void this.getPlaylists(); + + // Only refetch eagerly if something is actually rendering + // playlists. `playlist-view` is the sole subscriber and is + // created lazily on first navigation, so before it has ever + // been opened this used to fetch every track of every playlist + // in answer to an event nobody was listening for. Once it + // exists it is a cached view and stays subscribed, so the + // refetch-then-rerender path it depends on is unchanged. + if (this.subscribers.size > 0) { + void this.getPlaylists(); + } + } + + // =================================================================== + // PATCHING + // Refetch one playlist rather than all of them. + // =================================================================== + + /** + * Replace a single playlist's tracks in the cache. + * + * Falls back to a full invalidate whenever the patch cannot be + * shown to be equivalent: an event with no id (the bulk restore and + * reorder paths emit one), a cold cache, an id we have never seen, + * or a fetch already in flight — which would otherwise land on top + * of the patch and undo it. + * + * Summaries come along because `updated_at` moves with the edit and + * `playlist-view` sorts on it; `GetAllPlaylists` is summaries only, + * so its cost does not scale with how many tracks a playlist holds. + */ + private async patchPlaylist( + playlistId?: number | null, + ): Promise { + const cached = this.playlists; + + if ( + typeof playlistId !== 'number' || + cached === null || + this.playlistsLoading || + !cached.some((p) => p.Summary?.ID === playlistId) + ) { + this.invalidate(); + + return; + } + + try { + const [summaries, tracks] = await Promise.all([ + GetAllPlaylists(), + GetPlaylistTracks(playlistId), + ]); + + // The cache may have been replaced while those were in + // flight, in which case this patch describes a state that + // no longer exists and the newer one is already correct. + if (this.playlists !== cached) return; + + const summaryById = new Map( + (summaries ?? []).map((s) => [s.ID, s]), + ); + + // A new array identity, because `playlist-view` keys its + // reload off it — while sharing the `Tracks` array of every + // playlist that did not change. + // + // `WithTracks` is a generated *class* (it carries + // `convertValues`), so an object spread would produce + // something that no longer is one. Clone through the + // prototype instead. + const withChanges = ( + entry: playlist.WithTracks, + changes: Partial, + ): playlist.WithTracks => + Object.assign( + Object.create( + Object.getPrototypeOf(entry) as object, + ) as playlist.WithTracks, + entry, + changes, + ); + + this.playlists = cached.map((entry) => { + const summary = + summaryById.get(entry.Summary?.ID) ?? + entry.Summary; + + if (entry.Summary?.ID !== playlistId) { + return summary === entry.Summary + ? entry + : withChanges(entry, { + Summary: summary, + }); + } + + return withChanges(entry, { + Summary: summary, + Tracks: tracks ?? [], + }); + }); + + this.notify(); + } catch (err) { + console.error( + 'Failed to patch playlist after track change:', + err, + ); + this.invalidate(); + } } // =================================================================== @@ -138,8 +265,24 @@ class PlaylistStore { return () => this.subscribers.delete(callback); } + /** + * Coalesced to a microtask, the way every other store in this app + * does it (perf.p3). A patch writes the tracks and the summaries in two statements; + * without coalescing that is two synchronous passes over every + * subscriber. + * Lit batches the resulting `requestUpdate()`s anyway, so the win is + * small; the point is that five stores doing this and two not is + * where a real double-notify hides. + */ private notify(): void { - this.subscribers.forEach((callback) => callback()); + if (this.notifyScheduled) return; + + this.notifyScheduled = true; + + queueMicrotask(() => { + this.notifyScheduled = false; + this.subscribers.forEach((callback) => callback()); + }); } // =================================================================== diff --git a/frontend/src/store/queue-store.ts b/frontend/src/store/queue-store.ts index a03e366..576dc56 100644 --- a/frontend/src/store/queue-store.ts +++ b/frontend/src/store/queue-store.ts @@ -47,6 +47,16 @@ interface TracksModified { type Subscriber = () => void; +/** + * Queue bindings are fire-and-forget by design: what happened is + * reported by events (QueueChanged, PlaybackFailed), not by a return + * value. A rejected bridge call still needs somewhere to land other + * than an unhandled rejection (errors.m1). + */ +function reportBindingFailure(name: string): (err: unknown) => void { + return (err: unknown) => console.error(`${name} failed`, err); +} + class QueueStore { private state: QueueState = { tracks: [], @@ -190,15 +200,21 @@ class QueueStore { // =================================================================== play(): void { - Queue.Play(); + void Queue.Play().catch( + reportBindingFailure('Queue.Play'), + ); } next(): void { - Queue.Next(); + void Queue.Next().catch( + reportBindingFailure('Queue.Next'), + ); } previous(): void { - Queue.Previous(); + void Queue.Previous().catch( + reportBindingFailure('Queue.Previous'), + ); } setQueue( @@ -206,61 +222,87 @@ class QueueStore { startIndex: number, shuffleStart = false, ): void { - Queue.SetQueue(filePaths, startIndex, shuffleStart); + void Queue.SetQueue(filePaths, startIndex, shuffleStart).catch( + reportBindingFailure('Queue.SetQueue'), + ); } addToQueue(filePath: string): void { - Queue.AddTrack(filePath); + void Queue.AddTrack(filePath).catch( + reportBindingFailure('Queue.AddTrack'), + ); } playNext(filePath: string): void { - Queue.InsertNext(filePath); + void Queue.InsertNext(filePath).catch( + reportBindingFailure('Queue.InsertNext'), + ); } removeFromQueue(position: number): void { - Queue.RemoveTrack(position); + void Queue.RemoveTrack(position).catch( + reportBindingFailure('Queue.RemoveTrack'), + ); } removeTracksFromQueue(positions: number[]): void { - Queue.RemoveTracks(positions); + void Queue.RemoveTracks(positions).catch( + reportBindingFailure('Queue.RemoveTracks'), + ); } addTracksToQueue(filePaths: string[]): void { - Queue.AddTracks(filePaths); + void Queue.AddTracks(filePaths).catch( + reportBindingFailure('Queue.AddTracks'), + ); } playTracksNext(filePaths: string[]): void { - Queue.InsertNextTracks(filePaths); + void Queue.InsertNextTracks(filePaths).catch( + reportBindingFailure('Queue.InsertNextTracks'), + ); } toggleShuffle(): void { - Queue.ToggleShuffle(); + void Queue.ToggleShuffle().catch( + reportBindingFailure('Queue.ToggleShuffle'), + ); } cycleRepeat(): void { - Queue.CycleRepeat(); + void Queue.CycleRepeat().catch( + reportBindingFailure('Queue.CycleRepeat'), + ); } playAtIndex(index: number): void { - Queue.PlayIndex(index); + void Queue.PlayIndex(index).catch( + reportBindingFailure('Queue.PlayIndex'), + ); } insertTracksAtIndex( filePaths: string[], index: number, ): void { - Queue.InsertTracksAt(filePaths, index); + void Queue.InsertTracksAt(filePaths, index).catch( + reportBindingFailure('Queue.InsertTracksAt'), + ); } moveTracksInQueue( fromIndices: number[], toIndex: number, ): void { - Queue.MoveQueueTracks(fromIndices, toIndex); + void Queue.MoveQueueTracks(fromIndices, toIndex).catch( + reportBindingFailure('Queue.MoveQueueTracks'), + ); } clearQueue(): void { - Queue.Clear(); + void Queue.Clear().catch( + reportBindingFailure('Queue.Clear'), + ); } // =================================================================== diff --git a/frontend/src/store/search-store.ts b/frontend/src/store/search-store.ts index 0a6ca43..0afdf9b 100644 --- a/frontend/src/store/search-store.ts +++ b/frontend/src/store/search-store.ts @@ -52,6 +52,19 @@ class SearchStore { return () => this.subscribers.delete(callback); } + /** + * Deliberately *not* microtask-coalesced, unlike every other store + * here (perf.p3 asks for it, and it is wrong about this one). + * + * Two reasons. Deferring makes an unsubscribe that happens + * synchronously after a set drop the notification entirely, which + * is a semantic change, not an optimisation — `view-stores.test.ts` + * pins both halves. And this store is on the keystroke path, where + * the batching the audit says would hide the cost is Lit's, not + * ours: the `requestUpdate()`s are already coalesced one layer + * down, so the microtask buys nothing and costs a frame of term + * staleness in the one place a frame is visible. + */ private notify(): void { this.subscribers.forEach((callback) => callback()); } diff --git a/frontend/src/utils/selection-controller.ts b/frontend/src/utils/selection-controller.ts index 42f7f34..658a176 100644 --- a/frontend/src/utils/selection-controller.ts +++ b/frontend/src/utils/selection-controller.ts @@ -125,6 +125,17 @@ export class SelectionController implements ReactiveController { } } + /** Whether `next` holds exactly the currently selected keys. */ + private sameMembership(next: ReadonlySet): boolean { + if (next.size !== this._selectedItems.size) return false; + + for (const key of next) { + if (!this._selectedItems.has(key)) return false; + } + + return true; + } + /** Clear the entire selection. */ clear(): void { if (this._selectedItems.size === 0) return; @@ -135,6 +146,40 @@ export class SelectionController implements ReactiveController { this.host.onSelectionChanged?.(); } + /** + * Drop selected keys that are no longer in the list, keeping the + * rest. + * + * The reason this exists rather than `clear()`: a refetch is not a + * deselection. Every naturally finished track used to invalidate + * the library cache, and `track-list` answered the new array by + * clearing the selection — so selecting forty tracks to drag into a + * playlist was impossible while music was playing (audit perf.C2). + * Keys are file paths, which survive a refetch, so the selection + * survives with them. + * + * `lastSelectedIndex` is dropped regardless: it is an index into a + * list that has just been replaced, and a shift-click against a + * stale one selects the wrong range. + */ + retain(isStillPresent: (key: string) => boolean): void { + if (this._selectedItems.size === 0) return; + + const next = new Set(); + + for (const key of this._selectedItems) { + if (isStillPresent(key)) next.add(key); + } + + this.lastSelectedIndex = null; + + if (next.size === this._selectedItems.size) return; + + this._selectedItems = next; + this.host.requestUpdate(); + this.host.onSelectionChanged?.(); + } + /** Select all items. */ selectAll(): void { const count = this.host.getItemCount(); @@ -145,7 +190,12 @@ export class SelectionController implements ReactiveController { if (key !== undefined) next.add(key); } - if (next.size === this._selectedItems.size) return; + // Membership, not cardinality. The guard used to compare sizes + // alone, so selecting four rows and then Select All over a + // *different* four was a no-op (audit perf.p5) — reachable now + // that `retain()` above carries a selection across a refetch + // that replaced the list. + if (this.sameMembership(next)) return; this._selectedItems = next; this.lastSelectedIndex = count > 0 ? count - 1 : null; @@ -156,8 +206,26 @@ export class SelectionController implements ReactiveController { /** * Return the selected keys in the order they appear in the host's * item list. This preserves positional ordering for queue operations. + * + * This walks the *list*, not the selection, which audit `perf.m6` + * calls out: every `dragstart`, every context-menu action and every + * favourite toggle pays it. Measured at 50 000 tracks it is **3 ms** + * — real, and a fifth of a frame, so the loop stays. What it does + * not do any more is keep going after it has found everything. + * + * It walks the list rather than the selection *deliberately*: the + * only way to order the selection directly is to store each key's + * index with it, and an index goes stale whenever the list is + * re-sorted, re-filtered or refetched, while the keys (file paths) + * survive all three — which is exactly why `retain()` drops + * `lastSelectedIndex` and keeps the keys. Trading 3 ms for a + * silently mis-ordered queue insert is not a trade. */ getSelectedKeysOrdered(): string[] { + const wanted = this._selectedItems.size; + + if (wanted === 0) return []; + const count = this.host.getItemCount(); const result: string[] = []; @@ -166,6 +234,7 @@ export class SelectionController implements ReactiveController { if (key !== undefined && this._selectedItems.has(key)) { result.push(key); + if (result.length === wanted) break; } } @@ -174,8 +243,15 @@ export class SelectionController implements ReactiveController { /** * Return the selected indices in ascending order. + * + * Same shape, same reasoning, same early exit as + * `getSelectedKeysOrdered()` above. */ getSelectedIndices(): number[] { + const wanted = this._selectedItems.size; + + if (wanted === 0) return []; + const count = this.host.getItemCount(); const result: number[] = []; @@ -184,6 +260,7 @@ export class SelectionController implements ReactiveController { if (key !== undefined && this._selectedItems.has(key)) { result.push(i); + if (result.length === wanted) break; } }