Code-quality review flagged convention drift from sibling library
pages. Aligning so the history page matches the same shape:
- queryStore + $derived($queryStore) pattern (single subscription
point) instead of $query.X everywhere.
- h1 uses font-display text-2xl font-medium per FabledSword design
system (weights 400/500 only — font-semibold drifted to 600).
- h2 uses font-medium + z-10 for sticky-header layering.
- InfiniteScrollSentinel uses its `enabled` prop (which exists; the
earlier spec note claiming otherwise was wrong) so the observer
isn't recreated on every fetch cycle.
- Loading more… / End of history footers added to match albums.
- onRetry passes query.refetch by reference, not wrapped.
- Dropped redundant `as HistoryEvent[]` cast.
Also fixed test case 4 which trivially passed regardless of the
conditional gate's correctness — now queries the sentinel's actual
DOM root (div[aria-hidden="true"].h-px) and asserts presence/absence.
Added a positive twin test for hasNextPage=true.
CI test-web was blocking on:
- QueueDrawer.test close-button query: getByLabelText(/close queue/i)
matched BOTH the backdrop button (aria-label="Close queue backdrop")
and the close button (aria-label="Close queue"). Switched to exact
string match.
- QueueDrawer.test inert assertion: Svelte 5 + jsdom uses property
binding for `inert`, not attribute, so hasAttribute('inert') returns
false. Check the DOM property (with attribute fallback for
robustness) instead.
- queue-row-math.ts -0 normalization: Math.round(-0.5) returns -0,
and Object.is(-0, 0) is false — vitest's .toBe(0) fails. Added
`|| 0` to normalize -0 to +0 at the function boundary so consumers
never see -0.
- player/store.svelte.ts user import guard: persistence $effect.root
reads `user.value?.id` at module-init, but in test files where
auth/store.svelte.ts is imported transitively (auth/store.test,
playlists/playlists.test), the player module loads via auth's
import chain before auth's `user` binding is assigned. `user?.value`
guards against that init-order race. The proper fix is to invert
the auth↔player import dep (filed as I2 follow-up under #375).
CI svelte-check was blocking on:
- 1 ERROR I introduced in the previous cleanup: QueueDrawer.test.ts
passed { hidden: true } to getByLabelText, but that option only
exists on getByRole. Fixed by switching to a direct
document.querySelector for the aria-hidden assertion.
- 13 WARNINGS pre-existing in the codebase from M7 #352/#372/#349
era, never surfaced because earlier CI runs failed before reaching
type-check. Now that CI gets that far, they accumulate. Cleared:
- FlagPopover, RemoveTrackPopover, AddToPlaylistMenu, TrackMenu:
interactive role divs gain tabindex="-1" + onkeydown that stops
propagation and closes on Escape (functional, not just warning-
suppression).
- FlagPopover state-from-props: $state(untrack(() => prop ?? default))
for explicit initial-snapshot semantics; const isUpdate switched
to $derived so it reacts to prop changes.
- PlaylistTrackRow drag-div: role="listitem" added.
- playlists/+page.svelte: autofocus replaced with bind:this + $effect.
CI svelte-check failed on 5 type errors caused by the slice using
duration_ms/album_name fields that don't exist on TrackRef (the real
shape uses duration_sec + album_id/album_title). Fixed across
QueueDrawer, QueueDrawer.test, QueueTrackRow.test, persisted.test,
and the inline `state.current = null` in PlayerBar.test (TrackRef |
undefined, not | null).
Final whole-slice review concerns also rolled in:
- C1 (Critical): persisted position was restored to UI but never
seeked into the audio element — first timeupdate snapped _position
back to 0. Added a one-shot _pendingRestorePosition module ref +
consumePendingRestorePosition() export; layout's loadedmetadata
handler now seeks once on restore.
- I1 (Important): throttled-write effect now wraps _queue / _index
reads in untrack so the dependency tracking matches the design
intent. Position remains the only tracked dep.
- I4 (Important): drawer gets inert={!queueDrawerOpen} so its inner
buttons drop out of tab order when closed (aria-hidden alone
doesn't do that). Removed redundant role="complementary" from
<aside> (implied by the element). New test asserts inert binding.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Wrap the _position read inside untrack() in the immediate-write $effect
so it no longer registers as a reactive dependency, preventing the effect
from firing on every 4Hz position tick. Also adds three missing
restoreQueue unit tests with vi.mock for the auth store.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
M2 — Add TODO(#375) tripwire comments where the dark/light theme-color
hex pair is duplicated outside tokens.json (vite.config.ts and
applyMetaThemeColor.svelte.ts). Refactoring is out of scope for #363;
the comments are stop signs for the next person who edits these.
M5 — The SPA never reads window.__MINSTREL__.description. The OG meta
description is server-rendered and complete on its own. Drop the dead
inline-script field and the corresponding helpers in branding.ts.
Server-side BrandingConfig.Description stays — it's still used for
<meta name="description"> and og:description.
Copy nit — Playlist detail page title becomes "Playlist · Foo"
matching the singular form already used by Artist · / Album ·.
Add browser tab titles to all 18 static page routes using the pageTitle()
helper from $lib/branding. Titles follow the Minstrel · Section format.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Those actions don't have meaningful semantics for the track you're
already listening to. New TrackMenu prop hideQueueActions gates the
pair; PlayerBar passes it true. Other mount sites (track rows in
lists) keep the full 8-entry menu.
- AddToPlaylistMenu: anchor submenu bottom to parent bottom so it
grows upward (top-0 → bottom-0) — kebab row is usually near the
bottom of the track menu and the submenu kept extending off-screen.
- HorizontalScrollRow: scrollbar-width: none + ::-webkit-scrollbar
display:none. Page arrows already drive the scroll; the scrollbar
was just consuming vertical real estate.
The <img alt=""> production element renders as role="presentation"
(decorative — playlist name is in sibling text). screen.getByRole('img')
doesn't match presentation-role elements. Switch to
container.querySelector('img') so the test doesn't fight the
correct a11y choice. Production code unchanged.
svelte-check failed with 8 type errors: $app/stores' page.params.id
is typed `string | undefined`, and the page passed `id` raw to
helpers that need `string`. Two changes:
- Switch to `$app/state` (the project canonical for new pages — see
routes/albums/[id]/+page.svelte). page.params.id is a direct
property read, not a store subscription. Fall back to "" so id is
always a string; SvelteKit won't actually render the page without
a populated id, but the type-checker doesn't know that.
- Drop the now-unused `writable` import in the test file and update
the mock to expose the new state-shape (object with .params, not a
writable store).
The remaining a11y warnings (tabindex on role=menu/dialog, click
handlers without keyboard events, draggable div without role,
autofocus) are svelte-check warnings — they don't block the build.
Some are pre-existing in FlagPopover; others are tech debt from
slice 1's drag-and-drop and modal patterns. Address as a follow-up
polish task.
Header shows collage, name, description, public/private chip, track
count, owner attribution (when not owner). Edit + delete buttons
visible to the owner only. Track list uses PlaylistTrackRow with
HTML5 drag-and-drop; drop fires PUT /tracks with the new ordered
positions and TanStack Query invalidates the playlist + index cache.
Toast surface is a placeholder browser alert in slice 1 — a real
toast is a polish task whenever it lands.
Replaces the placeholder route. Two sections: "Your playlists" (owned)
and "From other users" (public). Inline create form in the header
with Enter-to-submit / Esc-to-cancel. Empty-state copy when the
operator has nothing yet. Routes to /playlists/{id} on card click via
PlaylistCard.
The "Add to playlist…" entry that #372 reserved as a disabled slot
is now active. Submenu lists the operator's own playlists
alphabetically; "New playlist…" toggles an inline create form that
makes the playlist + appends the track in two API calls.
TrackMenu's existing test asserts the entry is enabled and opens the
submenu instead of the previous "is disabled with tooltip" check.
Variant of TrackRow specialised for playlist detail. Drag handle
visible to owner only; remove button (X) visible to owner only;
greyed-out + strikethrough when track_id is null (upstream track
removed from library). Reuses the existing LikeButton and TrackMenu
when the track is still alive.
Square card with cover (or "No tracks yet" glyph fallback), name,
track count, and owner attribution when the playlist isn't the
current user's. Click navigates to /playlists/{id}.
Wire shapes mirror the server's snake_case envelope. URL helpers use
the existing apiFetch + ApiError surface so error codes (not_found,
not_authorized, bad_request, server_error) propagate via the same
copyForCode chain as everything else.
bvandeusen