S10.3 Playlists UX (core): CRUD + add-to-playlist everywhere + play/reorder/remove#59
Merged
Conversation
Folds in architect-reviewer / swiftui-pro / qa-expert / the-Fool review:
- Corrects false premises: no scan-folder drop target exists (folders via
open-panel; US-LIB-07 move-on-drag deferred) → reference-add safety is
type-level (LibraryTrackDragItem{trackID:Int64} + declared UTType, drop
rejects .fileURL); FacetTrackListView is List(selection:) (drop/reorder
dead on macOS 26) → detail body reuses the queue's LazyVStack/PlaylistItemRow,
header only from the facet view; QueueDragItem carries a queue UUID not
tracks.id.
- Separate PlaylistsModel peer (not extend the read-only browse cache);
epoch-guarded re-read + libraryRevision refresh for CASCADE shrink.
- Built-in 'current' filtered from every surface + DAO-guarded append/insert/
reorder (was unguarded → queue-mirror corruption).
- Add-to-playlist = context menu (New + recent) → searchable sheet (submenu
doesn't scale); context-menu is the primary Songs path (Table drag is a
refactor).
- QA: US-PLIST-04 proven at the app layer (pure PlaylistDropRouter + FileMover
spy + strict-gate grep; DAO test is vacuous), US-PLIST-08 via the real
scanner, + reorder-isolation / explain-plan / write-during-scan checks.
- 4 open founder-brainstorm decisions (IA, play-semantics, delete-UX,
loose-file R1 scope); estimate flagged as likely light.
Deep-research pass across Roon/MusicBee/Audirvana/foobar2000 + Apple Music/ Spotify (19 sources → 25 claims 3-vote-verified → 23 confirmed). Key shifts for the design: IA → a dedicated sidebar Playlists SECTION (not a 5th browse category); reference-add-only + hidden current-queue + skip-on-play for dead files all validated by category norms; verb set Play/Play Next/Add to Queue; undo for destructive queue-replace is a differentiation opening (absent in comparables). New scope questions: playlist folders/nesting + smart playlists are category norms currently out of scope.
…rs + smart playlists) Founder scope calls post-research: sidebar Playlists SECTION (not 5th browse category); playlist folders/nesting; smart (rules-based) playlists; Play/Play Next/Add-to-Queue verb set with a 'restore previous queue' undo. Folders + smart playlists are net-new schema + DAO (S10.1 covered static only): playlist_folders self-ref tree + playlists.folder_id; playlists.is_smart + a normalized playlist_rules table with a pure rule→SQL WHERE builder; smart membership is derived, read-only. Reference-add stays type-level; built-in hidden+guarded; dead files skip-on-play + indicator + remove-missing. FLAGGED: (1) re-estimate ~15-18 SP (was 6) → phased chunks A-H, R1 can gate on static+folders+IA with smart as fast-follow; (2) DATA-LOSS RISK — user playlists/folders/rules are NOT a rebuildable cache, so a schema bump under eraseDatabaseOnSchemaChange would wipe them; §9.5 decision needed before the schema bump lands. 5 open founder-brainstorm sub-decisions in §9.
Data-loss myth corrected (appending migrations is safe; verified vs GRDB source + repo v4). Founder decisions locked: - Smart playlists DECOUPLED to post-R1 (design preserved in a Deferred section) → R1 = static + folders + sidebar IA + Play/Next/Queue+undo + dead-file (~10-12 SP). - Separate user-data.sqlite3 store (additive-only, never erased); library cache stays nuke-and-rebuild. Cross-store soft-refs; the track-delete CASCADE becomes an app-layer orphan sweep on libraryRevision (= founder's 'track deleted → remove from playlist' rule; moved+re-matched track survives). - Global-unique playlist names. - Delete a playlist folder = cascade-delete contents, guarded by undo (subtree snapshot). - Sidebar = one ScrollView/LazyVStack + unified selection enum; flattened folder tree (no OutlineGroup). Detail = new PlaylistDetailList reusing PlaylistItemRow. Undo snapshots the in-memory queue synchronously. Phased chunks A0-G; A0 (store split + relocate S10.2 tables) is the load-bearing refactor, gated by a ud-survives-schema-bump green check.
…lit) A QA break-it (qa-expert + Fool) overturned the separate-user-data-store decision: its cross-store soft ref was tracks.id (a bare rowid, no AUTOINCREMENT) that a library rebuild reassigns → playlists silently resolved to the WRONG track (worse than a clean wipe); and it protected only playlists while play_count/loved/rating/ frecency stayed in the erased cache. Root cause: library.sqlite3 was never a pure cache — it holds user data too. Founder decision: ONE store, stop erasing user data. - LibraryStore.makeMigrator: eraseDatabaseOnSchemaChange = false; migrations are additive-only, frozen-body. Playlists stay in library.sqlite3 with the same-file FK ON DELETE CASCADE (atomic deleted-track cleanup, no app-layer sweep), id stability, and US-PLIST-08 (move-match keeps id). The derived cache is rebuilt by re-scan, not by wiping the file. - Replaced checkEraseOnSchemaChange with additive-preserve-schema-bump (proves an appended migration PRESERVES seeded user data) — the meaningful invariant now. - Reverted the separate UserDataStore + DAO relocation + consumer repoints (the A0 detour, discarded). - Updated comments (LibraryStore/Schema), memory (feedback-delete-rebuild-dev-db scoped/reversed), and the design doc to rev.5 with the full store-decision history. make strict-gate green.
…+ checks Schema v5 (additive, durable under erase=false): playlist_folders adjacency-list tree (parent_id self-ref, ON DELETE CASCADE) + playlists.folder_id (ON DELETE CASCADE). Deleting a folder cascades to subfolders + its playlists + their entries — 'folder owns its contents' (D-folder-delete). Folder DAO (LibraryStore+PlaylistFolders): folders()/createFolder/renameFolder/ setPlaylistFolder + reparentFolder with an in-txn WITH RECURSIVE cycle-guard (a folder can't become its own ancestor) + deleteFolder that snapshots the whole subtree (folders+playlists+entries, value structs) BEFORE the cascade and restoreFolderSubtree that re-inserts it verbatim (ids preserved, deferred FKs) — the undo. Single store, no cross-store anything. VerifyLibraryStore: pl-folder-crud, pl-folder-reparent-cycle-reject, pl-folder-cascade-delete-restore (117/117). fullMigrator() helper + expectedTables + currentSchemaVersion bumped to v5.
… detail Chunk B (folds A2): the Playlists view-model over the same store as the rest of the library (one store, D-store rev.5) + a rebuilt Library sidebar with a dedicated Playlists section + a minimal read-only playlist detail. - PlaylistsModel: @mainactor @observable peer (tree + open detail, per-surface epoch guards, built-in "current" filtered out). Verbs used here: loadTree / loadDetail / createPlaylist / renamePlaylist / deletePlaylist / reloadOnLibraryChange. The audio dep + play/remove/reorder verbs land in C, folder verbs in D — sliced so nothing sits as dead code under Periphery. - Sidebar rebuilt to ONE ScrollView { LazyVStack } of Button rows with a unified SidebarSelection (drops List(selection:) — its row .dropDestination never fires, needed for Chunk E's drag-to-playlist, and it races row gestures). Playlists section: create -> inline-rename, delete, keyboard up/down + Return. Music Folders footer preserved (moved to a same-file extension for length). - Read-only PlaylistDetailView reusing PlaylistItemRow (nil dragPayload); C wires play/remove/reorder onto the same rows, F renders the unavailable state. - Nav on LibraryBrowseModel (+Sidebar extension): sidebarSelection / selectCategory / selectPlaylist; LibraryRoute.playlist(Int64) rendered through the existing path.last -> LibraryRouteView seam. Reviews folded in: - swift-expert (Chunk A DAO): dedicated .wouldCreateCycle error; setPlaylistFolder throws .notFound/.builtinImmutable; restore-undo best-effort doc note. - swiftui-pro (Chunk B): delete-open-playlist nav redirect; click-away commits the rename (reverts only on conflict); deferred rename focus (no self-close); VoiceOver .isSelected traits; drill-down guard on up/down; loadDetail clears stale rows on switch. strict-gate PASSED; VerifyLibraryStore 117/117.
Every finding from the qa-expert + architect-reviewer/Fool break-it on chunks
A+B, plus the three founder decisions. Both reviewers steelmanned the one-store
reversal + the sidebar rebuild as correct; these harden the edges.
Migrator posture (architect BLOCKER — the additive-preserve check only proved
GRDB, not our migrator):
- strict-gate grep forbids `eraseDatabaseOnSchemaChange = true` / asserts `= false`
(Attack A: silent re-enable of drop-and-recreate).
- new `additive-migration-convergence` check: staged-upgrade == fresh-install +
a golden FNV-1a schema-fingerprint hash that trips on any EDITED shipped
migration body (Attack B: fresh installs diverge from migrated users).
- `makeMigrator` invariant reworded: data-preserving, APPEND-NEVER-EDIT (+ the
12-step table-rebuild escape hatch for destructive changes).
Corruption/quarantine (founder: signal now, backup fast-follow):
- LibraryStore.quarantinedFrom surfaces a rebuild-recoverable wipe to the shell
error banner instead of silently losing playlists + play history.
Names (founder: case-insensitive):
- v6 migration recreates idx_playlists_name_user COLLATE NOCASE + NOCASE conflict
probe ("Rock" == "rock"), matching the reserved-name guard + the sort.
Folder DAO:
- reparentFolder re-positions to end-of-new-group (drag-drop "append").
- new checks: pl-folder-restore-conflict-rolls-back, pl-set-playlist-folder-rejects,
pl-folder-edge-cases (empty / reparent-nil / 40-level). VerifyLibraryStore 121/121.
App layer:
- closeDetail bumps detailEpoch (no in-flight republish of a deleted playlist).
- delete redirects nav only on confirmed success.
- rename: re-checks the row after the await + a reserved-name message.
- built-in exclusion extracted to the pure, unit-tested PlaylistBrowseVisibility (3/3).
Docs: design §8 (break-it outcomes + carry-forward deferrals for C/D/E/F);
sprint-plan §Status (S10.4 shipped, S10.3 A+B landed).
strict-gate PASSED.
…order + remove Makes the playlist detail interactive (design §4 / D-play), reusing the queue's row scaffold. - PlaylistItemRow generalized to PlaylistItemRow<DragPayload: Transferable> so the queue (QueueDragItem) and the playlist detail (PlaylistEntryDragItem) share one row (architect review #4). Queue behavior unchanged. - PlaylistsModel play verbs over `audio` (now wired): playPlaylist (REPLACE with a one-level restore-queue undo), playPlaylistNext / appendPlaylist, per-row playEntryNext; removeEntry (+ loadTree for the count badge) and reorderEntries. resolvedEntries drops unavailable tracks = skip-on-play (F adds the badge). - AudioViewModel.playNowWithUndo + restorePreviousQueue + QueueRestorePoint: snapshot the queue before a Play-replace; restore = empty→clear, else resume at the prior track (no-selection→restore slots + stop the engine). - PlaylistDetailView: header Play/Next/Queue, tap-to-play-from-row, grip-drag reorder (queue's from<to?to+1:to convention — last slot reachable), remove, ↑/↓/Return/⌫ over resolved rows, transient "Restore previous queue" toast. swiftui-pro review folded in: reorder off-by-one (MAJOR) fixed to match the queue; dead wasPlaying dropped + coherent no-selection restore; per-row Play Next scoped to the row; keyboard skips unavailable rows; toast only on a real replace; post-delete neighbor selection. strict-gate PASSED; VerifyLibraryStore 121/121.
Founder by-ear: a new "New Playlist" couldn't be renamed. - Add double-click-to-rename on a playlist row (Finder/Music convention) — the discoverable gesture alongside the context-menu Rename + the Return key. - Move the rename field's focus into its own .onAppear (reliable once it's in the hierarchy) instead of a deferred @focusstate set from beginRename, which bounced on a freshly-inserted LazyVStack row and let the blur handler self-close the field before the user could type.
The piece that lets you put songs INTO a playlist (US-PLIST-02). Reference-add by track id — NEVER a file move/copy (US-PLIST-04). Drag-to-sidebar is E2. - PlaylistAddDecision (pure, LibraryBrowseKit): dedupe the selection (first-seen order), toast copy. Unit-tested (3/3). Dupes vs the existing playlist are allowed (by design); only the incoming selection is de-duplicated. - PlaylistsModel.addTracks(_:toPlaylist:) + createPlaylist(withTracks:) — append via the DAO's appendEntries (reference-add), reload tree (count badge) + open detail. - AddToPlaylistMenu: context-menu submenu (New Playlist + first-N playlists + an "Add to Playlist…" overflow) reused across library rows; PlaylistPickerSheet: a searchable picker (reuses LibraryFilterField) that scales past the submenu. - Wired into the Songs table context menu (single + multi-select, in sort order); the host owns the sheet (a context menu can't present one). - LibraryBrowseModel.showToast (+ shared raiseToast) for the "Added N songs to …" confirmation on the existing shell toast surface; play actions split to LibraryBrowseModel+Play for file length. strict-gate PASSED; VerifyLibraryStore 121/121; PlaylistAddDecision 3/3.
Founder-reported: playlist rename was impossible. Root-caused from a runtime log trace, then reviewed by swiftui-pro (fix + codebase-wide) + architect/Fool against the SwiftUI focus/keyboard programming model. The bug: the sidebar ScrollView (.focusable + .onKeyPress) sat over a focused rename TextField, so Return LEAKED UP to the ancestor's .onKeyPress(.return) (re-invoking beginRename → wiping the typed draft) instead of the field's onSubmit. Fix (the correct, generalizable template — an ancestor onKeyPress fires because the field is its hierarchical descendant regardless of which @focusstate is active, so the gate is the fix, not an enum): - Sidebar .onKeyPress(.up/.down/.return) return .ignored while editingPlaylistID != nil. - beginRename re-entry guard + yields sidebarFocused; focus set in the field's own .onAppear; restore list focus on the KEYBOARD close paths only (finishRename). Codebase focus audit outcome — we broadly ADHERE; the collision was ISOLATED to LibrarySidebar (every other focusable-list view has one @focusstate + no editable descendant). Fixes applied from the audit: - Extract .suppressesTransportSpace(while:) — replaces the copy-pasted .focused + onChange + onDisappear Space-gate ritual in all 4 text fields (LibraryFilterField, SongsHeader, SaveCustomPresetView, sidebar rename), so a field can't half-wire it and silently re-break space-typing (S4 SW1). - PlaylistDetailView: post-delete neighbor from the resolved-row set + .defaultFocus. - Nits: removed the dead .onKeyPress(.space) in the queue (Controls-menu equivalent preempts it); .ignored on empty-selection Return in the facet lists; auto-focus the Save-Preset + playlist-picker fields (LibraryFilterField focusesOnAppear). Deferred to Chunk D (documented, design §8): a single `enum SidebarEdit` owner + the thin keyboardListNavigation scaffold, so folder-rename can't re-detonate the hijack one field over. Founder by-ear PASSED (rename commits + keeps kb-nav; space-in-filter inserts). strict-gate PASSED.
… file move) Completes the add-to-playlist story (US-PLIST-03) on top of E1's context menu. - LibraryTrackDragItem (track-id payload), draggable from the Album/Artist/Genre detail List rows — a List row's drag SOURCE works (only its .dropDestination doesn't, which is why the sidebar is a LazyVStack). - Sidebar playlist rows are .dropDestination(for: LibraryTrackDragItem.self) with a drop-ring highlight → PlaylistDropRouter → addTracks + confirmation toast. A file-URL/audio drag can't match the typed payload, so it never reaches the handler. - PlaylistDropRouter (pure, LibraryBrowseKit): PlaylistDropOutcome has EXACTLY ONE case (.addTracks) — "a drop can never touch the filesystem" is a type-level guarantee (US-PLIST-04), not a convention. Unit-tested (2/2). - strict-gate: new guard greps the playlist add/drop path (sidebar + model + router + playlist DAO) for moveItem/copyItem and fails if present. Folded-in cleanup (SME focus-audit): extracted LibrarySidebar+Rename.swift (type-body length); the .suppressesTransportSpace modifier now owns the transport-Space gate, so the sidebar dropped its manual keyboardFocus wiring. strict-gate PASSED (incl. the new drop-path guard); VerifyLibraryStore 121/121; PlaylistDropRouter 2/2.
…iles) Founder by-ear found "Add to Playlist" was missing everywhere except the Songs table (E1 only wired that one) — right-clicking an album/artist showed no such item. Now it sits next to "Add to Queue" on ALL browse surfaces. - AddToPlaylistMenu generalized to an ASYNC id-resolver (`resolveTrackIDs`) so a grid TILE (which holds only an albumID/FacetRef, not loaded tracks) can add on demand, while a detail row just returns its ids. The searchable-picker overflow is optional (`onChooseMore`) — shown on surfaces that host the sheet, hidden on tile menus (which have nowhere to present one: inline playlists + New Playlist). - Wired into: album detail (⋯ = whole album, rows = the track), artist/genre detail (⋯ = whole facet, rows = the track), album grid tiles, artist/genre grid tiles. LibraryBrowseModel.facetTrackIDs added for the facet tiles. - All still reference-add by id (no file move) — covered by the E2 drop-path grep + PlaylistDropRouter's type-level guarantee. strict-gate PASSED.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
S10.3 — Playlists UX (core slice)
The last R1 gate: static playlists + folders data layer + a full add/play/organize loop, grounded in a cited market-research memo, three SME/Fool review rounds, and a QA break-it that overturned the store architecture before any code was built.
Founder by-ear verified: create / inline-rename / delete playlists; add tracks to a playlist from every browse surface; play (replace-queue with restore-queue undo) / Play Next / Add to Queue; drag-reorder; remove; drag a track onto a sidebar playlist; multi-word filtering (space) still types.
Landed in this PR
library.sqlite3now setseraseDatabaseOnSchemaChange = falsewith additive-only, frozen-body migrations, so it protects all non-rebuildable user data (playlists/folders +play_count/loved/rating/frecency), not just a derived cache. A break-it proved the earlier separate-store split silently mis-resolved playlists to the wrong tracks after a rescan (reusedtracks.idrowids). Enforced by a strict-gate guard (never re-enable erase) + a schema-convergence check (an edited shipped migration fails loudly).playlist_folders(additive v5) + case-insensitive name uniqueness (v6); folder DAO — create/rename/reparent (in-txnWITH RECURSIVEcycle-guard) / cascade-delete-with-subtree-snapshot for undo. (Folders UI is a follow-up — see below.)@Observablepeer over the one store; the sidebar rebuilt as oneScrollView { LazyVStack }of Button rows with a unified selection enum (dropsList(selection:)— its row drops don't fire, needed for drag-to-playlist).PlaylistDropRouteroutcome + a strict-gate grep over the whole add/drop path).suppressesTransportSpacemodifier so the Space-gate can't be half-wired.Reviews & gates
swift-expert (folder DAO), swiftui-pro (Chunk B, Chunk C, rename-fix, codebase focus audit), qa-expert + architect/Fool (A+B break-it). VerifyLibraryStore 121/121; LibraryBrowseKit unit tests (PlaylistBrowseVisibility, PlaylistAddDecision, PlaylistDropRouter);
make strict-gategreen on every commit (incl. the migrator-posture, schema-convergence, and add/drop-no-file-move guards).NOT in this PR — remaining S10.3 (follow-ups)
SidebarEditowner; collapse the open-playlist source-of-truth; extract akeyboardListNavigationscaffold). The folder data layer is already in; only the UI is pending.pl-reorder-isolation/pl-explain-plan/pl-write-during-scanchecks, and the final qa+Fool break-it.VACUUM INTOof the user DB (belt-and-braces for the corruption path) — documented, out of R1 scope.Merge the core now (folders/dead-files as follow-up PRs), or hold this open until D/F/G land — your call.
Test plan
make strict-gate(build + swiftformat/swiftlint --strict + Periphery + semgrep + migrator/drop-path guards + VerifyLibraryStore 121/121 + unit tests + C++ null tests) passes. By-ear on-device: the full create → add (menu/picker/drag, all surfaces) → play/reorder/remove loop.