parent
5bcf55f808
commit
9b6bd8929e
File diff suppressed because it is too large
Load Diff
File diff suppressed because it is too large
Load Diff
@ -1,327 +0,0 @@ |
|||||||
# Add "New Playlist…" to the Add-to-Playlist menu — Implementation Plan |
|
||||||
|
|
||||||
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. |
|
||||||
|
|
||||||
**Goal:** Let a user create a new regular playlist from a track's "Add to Playlist" context submenu, name it via a prompt, and have the track added to it on save. |
|
||||||
|
|
||||||
**Architecture:** A new "New Playlist…" item in the existing `Menu("Add to Playlist")` (in `TrackContextMenuModifier`) calls a new optional closure on `TrackContextMenuConfig`. `ContentView` owns that closure: it stashes the pending track and presents an `.alert` + `TextField` (mirroring the app's existing New Playlist alert). On save it calls a new `PlaylistViewModel.createPlaylistAndAddTrack(name:track:)`, which creates the regular playlist and adds the track in one step. |
|
||||||
|
|
||||||
**Tech Stack:** Swift, SwiftUI, GRDB, Swift Testing (`@Test`), Xcode (`Music` scheme). |
|
||||||
|
|
||||||
**Git note:** This project's owner never auto-commits — commits are made by the user via the `/commit` skill. The "Commit" steps below describe the *suggested grouping* of changes for when the user chooses to commit; do **not** run `git commit` yourself. |
|
||||||
|
|
||||||
**Spec:** `docs/superpowers/specs/2026-05-30-add-to-new-playlist-design.md` |
|
||||||
|
|
||||||
--- |
|
||||||
|
|
||||||
## File Structure |
|
||||||
|
|
||||||
- `Music/ViewModels/PlaylistViewModel.swift` — **Modify.** Add `createPlaylistAndAddTrack(name:track:)` orchestration method. |
|
||||||
- `MusicTests/PlaylistViewModelTests.swift` — **Create.** Unit test for the new method. |
|
||||||
- `Music/Models/TrackContextMenuConfig.swift` — **Modify.** Add `onAddToNewPlaylist` optional closure (+ init param, default `nil`). |
|
||||||
- `Music/Views/TrackContextMenuModifier.swift` — **Modify.** Add "New Playlist…" button + relax the submenu visibility guard. |
|
||||||
- `Music/ContentView.swift` — **Modify.** New `@State` for the pending track, wire `onAddToNewPlaylist`, present the name alert. |
|
||||||
|
|
||||||
--- |
|
||||||
|
|
||||||
## Task 1: `PlaylistViewModel.createPlaylistAndAddTrack` |
|
||||||
|
|
||||||
**Files:** |
|
||||||
- Test: `MusicTests/PlaylistViewModelTests.swift` (create) |
|
||||||
- Modify: `Music/ViewModels/PlaylistViewModel.swift` (add method after `addTrack`, ~line 77) |
|
||||||
|
|
||||||
- [ ] **Step 1: Write the failing test** |
|
||||||
|
|
||||||
Create `MusicTests/PlaylistViewModelTests.swift`: |
|
||||||
|
|
||||||
```swift |
|
||||||
import Testing |
|
||||||
import Foundation |
|
||||||
@testable import Music |
|
||||||
|
|
||||||
@MainActor |
|
||||||
struct PlaylistViewModelTests { |
|
||||||
|
|
||||||
// Verifies createPlaylistAndAddTrack does the full job in one call: |
|
||||||
// 1. Seed a track into an in-memory DB and build a PlaylistViewModel over it. |
|
||||||
// 2. Call createPlaylistAndAddTrack with a name and the seeded track. |
|
||||||
// 3. The returned playlist has the given name and a real (non-nil) id. |
|
||||||
// 4. The DB shows that playlist now contains exactly the seeded track. |
|
||||||
// 5. The new playlist is recorded as the last-used playlist. |
|
||||||
@Test func createPlaylistAndAddTrackCreatesPlaylistAndAddsTrack() throws { |
|
||||||
// 1. Seed a track and build the view model. |
|
||||||
let db = try DatabaseService(inMemory: true) |
|
||||||
var track = Track.fixture(fileURL: "/song.mp3", title: "Song A") |
|
||||||
try db.insert(&track) |
|
||||||
let vm = PlaylistViewModel(db: db) |
|
||||||
|
|
||||||
// 2. Create a new playlist and add the track in one step. |
|
||||||
let created = try vm.createPlaylistAndAddTrack(name: "Road Trip", track: track) |
|
||||||
|
|
||||||
// 3. The returned playlist is well-formed. |
|
||||||
#expect(created.id != nil) |
|
||||||
#expect(created.name == "Road Trip") |
|
||||||
|
|
||||||
// 4. The playlist contains exactly the seeded track. |
|
||||||
let tracks = try db.fetchPlaylistTracks(playlistId: created.id!) |
|
||||||
#expect(tracks.count == 1) |
|
||||||
#expect(tracks[0].id == track.id) |
|
||||||
|
|
||||||
// 5. The new playlist became the last-used playlist. |
|
||||||
#expect(vm.lastUsedPlaylistId == created.id) |
|
||||||
} |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 2: Run the test to verify it fails** |
|
||||||
|
|
||||||
Run: |
|
||||||
```bash |
|
||||||
xcodebuild test -scheme Music -destination 'platform=macOS' -only-testing:MusicTests/PlaylistViewModelTests |
|
||||||
``` |
|
||||||
Expected: **build/compile failure** — `value of type 'PlaylistViewModel' has no member 'createPlaylistAndAddTrack'`. |
|
||||||
|
|
||||||
- [ ] **Step 3: Write the minimal implementation** |
|
||||||
|
|
||||||
In `Music/ViewModels/PlaylistViewModel.swift`, add this method directly after `addTrack(_:to:)` (after line 77): |
|
||||||
|
|
||||||
```swift |
|
||||||
@discardableResult |
|
||||||
func createPlaylistAndAddTrack(name: String, track: Track) throws -> Playlist { |
|
||||||
let playlist = try db.createPlaylist(name: name) |
|
||||||
try addTrack(track, to: playlist) |
|
||||||
return playlist |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
`db.createPlaylist` returns a `Playlist` with its assigned `id`; the existing `addTrack` inserts the join row and sets `lastUsedPlaylistId`. |
|
||||||
|
|
||||||
- [ ] **Step 4: Run the test to verify it passes** |
|
||||||
|
|
||||||
Run: |
|
||||||
```bash |
|
||||||
xcodebuild test -scheme Music -destination 'platform=macOS' -only-testing:MusicTests/PlaylistViewModelTests |
|
||||||
``` |
|
||||||
Expected: **PASS** (`createPlaylistAndAddTrackCreatesPlaylistAndAddsTrack`). |
|
||||||
|
|
||||||
- [ ] **Step 5: Commit (suggested grouping — leave to the user / `/commit`)** |
|
||||||
|
|
||||||
Changed files: `MusicTests/PlaylistViewModelTests.swift`, `Music/ViewModels/PlaylistViewModel.swift`. |
|
||||||
Suggested message: `feat: add PlaylistViewModel.createPlaylistAndAddTrack` |
|
||||||
|
|
||||||
--- |
|
||||||
|
|
||||||
## Task 2: Add `onAddToNewPlaylist` to `TrackContextMenuConfig` |
|
||||||
|
|
||||||
This is a pure data struct; the new field is optional with a `nil` default, so existing call sites and `TrackContextMenuConfigTests` keep compiling. No new unit test (the struct just stores a closure). |
|
||||||
|
|
||||||
**Files:** |
|
||||||
- Modify: `Music/Models/TrackContextMenuConfig.swift` |
|
||||||
|
|
||||||
- [ ] **Step 1: Add the stored property** |
|
||||||
|
|
||||||
In `TrackContextMenuConfig`, add the property after `onAddToQueue` (after line 15): |
|
||||||
|
|
||||||
```swift |
|
||||||
// nil hides the "New Playlist…" item (e.g. tests that don't supply it). |
|
||||||
let onAddToNewPlaylist: ((Track) -> Void)? |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 2: Add the init parameter (with default `nil`)** |
|
||||||
|
|
||||||
In the explicit `init`, add the parameter after `onAddToQueue` (line 30): |
|
||||||
|
|
||||||
```swift |
|
||||||
onAddToQueue: ((Track) -> Void)? = nil, |
|
||||||
onAddToNewPlaylist: ((Track) -> Void)? = nil, |
|
||||||
onGetInfo: (([Track]) -> Void)? = nil |
|
||||||
``` |
|
||||||
|
|
||||||
And add the assignment in the body, after `self.onAddToQueue = onAddToQueue` (line 40): |
|
||||||
|
|
||||||
```swift |
|
||||||
self.onAddToNewPlaylist = onAddToNewPlaylist |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 3: Build to verify it compiles** |
|
||||||
|
|
||||||
Run: |
|
||||||
```bash |
|
||||||
xcodebuild build -scheme Music -destination 'platform=macOS' |
|
||||||
``` |
|
||||||
Expected: **BUILD SUCCEEDED** (existing call sites still compile because the new param defaults to `nil`). |
|
||||||
|
|
||||||
- [ ] **Step 4: Run the existing config tests to confirm no regression** |
|
||||||
|
|
||||||
Run: |
|
||||||
```bash |
|
||||||
xcodebuild test -scheme Music -destination 'platform=macOS' -only-testing:MusicTests/TrackContextMenuConfigTests |
|
||||||
``` |
|
||||||
Expected: **PASS** (all existing tests). |
|
||||||
|
|
||||||
- [ ] **Step 5: Commit (suggested grouping — leave to the user / `/commit`)** |
|
||||||
|
|
||||||
Changed file: `Music/Models/TrackContextMenuConfig.swift`. |
|
||||||
Suggested message: `feat: add onAddToNewPlaylist to TrackContextMenuConfig` |
|
||||||
|
|
||||||
--- |
|
||||||
|
|
||||||
## Task 3: Add "New Playlist…" to the submenu |
|
||||||
|
|
||||||
SwiftUI view code; no unit test (consistent with the codebase). Verify by build. |
|
||||||
|
|
||||||
**Files:** |
|
||||||
- Modify: `Music/Views/TrackContextMenuModifier.swift:30-38` |
|
||||||
|
|
||||||
- [ ] **Step 1: Replace the "Add to Playlist" submenu block** |
|
||||||
|
|
||||||
Replace the current block (lines 30–38): |
|
||||||
|
|
||||||
```swift |
|
||||||
if !config.playlists.isEmpty { |
|
||||||
Menu("Add to Playlist") { |
|
||||||
ForEach(config.playlists) { playlist in |
|
||||||
Button(playlist.name) { |
|
||||||
config.onAddToPlaylist(track, playlist) |
|
||||||
} |
|
||||||
} |
|
||||||
} |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
with: |
|
||||||
|
|
||||||
```swift |
|
||||||
if !config.playlists.isEmpty || config.onAddToNewPlaylist != nil { |
|
||||||
Menu("Add to Playlist") { |
|
||||||
if let onAddToNewPlaylist = config.onAddToNewPlaylist { |
|
||||||
Button("New Playlist…") { |
|
||||||
onAddToNewPlaylist(track) |
|
||||||
} |
|
||||||
if !config.playlists.isEmpty { |
|
||||||
Divider() |
|
||||||
} |
|
||||||
} |
|
||||||
ForEach(config.playlists) { playlist in |
|
||||||
Button(playlist.name) { |
|
||||||
config.onAddToPlaylist(track, playlist) |
|
||||||
} |
|
||||||
} |
|
||||||
} |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
Notes: the button label uses a real ellipsis character `…` (macOS convention for "opens a prompt"). The submenu now appears even when the user has zero playlists, showing just "New Playlist…". The `Divider` only appears when there are existing playlists to separate from. |
|
||||||
|
|
||||||
- [ ] **Step 2: Build to verify it compiles** |
|
||||||
|
|
||||||
Run: |
|
||||||
```bash |
|
||||||
xcodebuild build -scheme Music -destination 'platform=macOS' |
|
||||||
``` |
|
||||||
Expected: **BUILD SUCCEEDED**. |
|
||||||
|
|
||||||
- [ ] **Step 3: Commit (suggested grouping — leave to the user / `/commit`)** |
|
||||||
|
|
||||||
Changed file: `Music/Views/TrackContextMenuModifier.swift`. |
|
||||||
Suggested message: `feat: add "New Playlist…" item to Add to Playlist submenu` |
|
||||||
|
|
||||||
--- |
|
||||||
|
|
||||||
## Task 4: Wire the prompt in `ContentView` |
|
||||||
|
|
||||||
SwiftUI view code; no unit test. Verify by build + manual check. |
|
||||||
|
|
||||||
**Files:** |
|
||||||
- Modify: `Music/ContentView.swift` — add `@State` (near the other playlist alert state, e.g. by `playlistNameInput`/`showNewPlaylistAlert`), wire the closure in `trackContextMenuConfig` (line 433–434 area), add the alert (by the existing New Playlist alert at line 273). |
|
||||||
|
|
||||||
- [ ] **Step 1: Add state for the pending track** |
|
||||||
|
|
||||||
Find the existing `@State` declarations for the playlist alerts (the same scope that declares `showNewPlaylistAlert` and `playlistNameInput`). Add nearby: |
|
||||||
|
|
||||||
```swift |
|
||||||
@State private var newPlaylistTrack: Track? |
|
||||||
@State private var newPlaylistNameInput = "" |
|
||||||
``` |
|
||||||
|
|
||||||
(`newPlaylistNameInput` is kept separate from the sidebar's `playlistNameInput` so the two flows can't clobber each other's text.) |
|
||||||
|
|
||||||
- [ ] **Step 2: Wire the closure in `trackContextMenuConfig`** |
|
||||||
|
|
||||||
In `trackContextMenuConfig` (line 412), add `onAddToNewPlaylist` to the `TrackContextMenuConfig(...)` call. Insert it after the `onAddToQueue:` argument (line 433), before `onGetInfo:`: |
|
||||||
|
|
||||||
```swift |
|
||||||
onAddToQueue: queueEnabled ? { track in player.addToQueue(track) } : nil, |
|
||||||
onAddToNewPlaylist: { track in newPlaylistTrack = track }, |
|
||||||
onGetInfo: { tracks in |
|
||||||
if !tracks.isEmpty { infoRequest = TrackInfoRequest(tracks: tracks) } |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
(Wired unconditionally — matches `onAddToPlaylist`, which is also not gated on `queueEnabled`.) |
|
||||||
|
|
||||||
- [ ] **Step 3: Add the name-prompt alert** |
|
||||||
|
|
||||||
Immediately after the existing New Playlist alert block (after line 283, the closing `}` of `.alert("New Playlist", isPresented: $showNewPlaylistAlert)`), add: |
|
||||||
|
|
||||||
```swift |
|
||||||
.alert("New Playlist", isPresented: Binding( |
|
||||||
get: { newPlaylistTrack != nil }, |
|
||||||
set: { if !$0 { newPlaylistTrack = nil; newPlaylistNameInput = "" } } |
|
||||||
)) { |
|
||||||
TextField("Playlist name", text: $newPlaylistNameInput) |
|
||||||
Button("Cancel", role: .cancel) { |
|
||||||
newPlaylistNameInput = "" |
|
||||||
newPlaylistTrack = nil |
|
||||||
} |
|
||||||
Button("Create") { |
|
||||||
let name = newPlaylistNameInput.trimmingCharacters(in: .whitespaces) |
|
||||||
if !name.isEmpty, let track = newPlaylistTrack { |
|
||||||
try? playlist.createPlaylistAndAddTrack(name: name, track: track) |
|
||||||
} |
|
||||||
newPlaylistNameInput = "" |
|
||||||
newPlaylistTrack = nil |
|
||||||
} |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 4: Build to verify it compiles** |
|
||||||
|
|
||||||
Run: |
|
||||||
```bash |
|
||||||
xcodebuild build -scheme Music -destination 'platform=macOS' |
|
||||||
``` |
|
||||||
Expected: **BUILD SUCCEEDED**. |
|
||||||
|
|
||||||
- [ ] **Step 5: Manual verification** |
|
||||||
|
|
||||||
Launch the app. Right-click a track → **Add to Playlist** → **New Playlist…**. Enter a name, click **Create**. Confirm: |
|
||||||
- A new playlist with that name appears in the sidebar. |
|
||||||
- Selecting it shows the track you added. |
|
||||||
- The sidebar selection did **not** change automatically (stayed on the current view). |
|
||||||
- Re-open the context menu: "Add to <new name>" now appears as the last-used playlist shortcut. |
|
||||||
- Empty name → clicking Create does nothing (no empty playlist created). |
|
||||||
|
|
||||||
- [ ] **Step 6: Commit (suggested grouping — leave to the user / `/commit`)** |
|
||||||
|
|
||||||
Changed file: `Music/ContentView.swift`. |
|
||||||
Suggested message: `feat: prompt for name and add track when creating playlist from menu` |
|
||||||
|
|
||||||
--- |
|
||||||
|
|
||||||
## Final verification |
|
||||||
|
|
||||||
- [ ] Run the full test target once: |
|
||||||
|
|
||||||
```bash |
|
||||||
xcodebuild test -scheme Music -destination 'platform=macOS' |
|
||||||
``` |
|
||||||
Expected: **all tests PASS**, including the new `PlaylistViewModelTests`. |
|
||||||
|
|
||||||
--- |
|
||||||
|
|
||||||
## Self-review (done while writing this plan) |
|
||||||
|
|
||||||
- **Spec coverage:** view-model create+add (Task 1), config closure (Task 2), submenu item + empty-list handling (Task 3), prompt + wiring + behavior decisions: single clicked track, no navigation, empty-name no-op (Task 4). All spec sections map to a task. |
|
||||||
- **Placeholder scan:** none — every code step shows the exact code. |
|
||||||
- **Type/name consistency:** `createPlaylistAndAddTrack(name:track:)` and `onAddToNewPlaylist` are used identically across Tasks 1–4; `newPlaylistTrack` / `newPlaylistNameInput` are consistent within Task 4. |
|
||||||
@ -1,607 +0,0 @@ |
|||||||
# Fix `bitrate = 0` Tracks Implementation Plan |
|
||||||
|
|
||||||
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. |
|
||||||
|
|
||||||
**Goal:** Stop the importer from ever storing a bitrate of `0`, and backfill the real bitrate onto existing tracks where it is `0` or `NULL`. |
|
||||||
|
|
||||||
**Architecture:** Two independent pieces. (1) A pure, unit-tested Swift function `ScannerService.resolveBitrate` that derives kbps from AVFoundation's estimate with a file-size/duration fallback, returning `nil` (never `0`) when nothing is derivable; wired into `extractMetadata`. (2) A stdlib-only Python backfill script `scripts/backfill_bitrate.py` (modeled on the existing `backfill_itunes_dates.py`) that recomputes bitrate via `ffprobe`, falling back to the same formula, with dry-run default and `--apply` + timestamped backup. |
|
||||||
|
|
||||||
**Tech Stack:** Swift (AVFoundation, swift-testing), Python 3 stdlib (`sqlite3`, `subprocess`), `ffprobe` (optional external tool). |
|
||||||
|
|
||||||
**Project conventions to respect:** |
|
||||||
- User's global rule: **never auto-commit.** Each "Commit checkpoint" step means *stop and run the `/commit` skill* — do not run `git commit` directly. |
|
||||||
- Tests use swift-testing (`import Testing`, `@Test`, `#expect`), one struct per file, every test carries a step-by-step comment. |
|
||||||
- Swift static utilities in `ScannerService` are `nonisolated static`. |
|
||||||
|
|
||||||
**Spec:** `docs/superpowers/specs/2026-05-30-fix-zero-bitrate-design.md` |
|
||||||
|
|
||||||
--- |
|
||||||
|
|
||||||
### Task 1: `ScannerService.resolveBitrate` pure function (Swift, TDD) |
|
||||||
|
|
||||||
**Files:** |
|
||||||
- Test: `MusicTests/ScannerServiceTests.swift` (add tests to the existing `ScannerServiceTests` struct) |
|
||||||
- Modify: `Music/Services/ScannerService.swift` (add the static function) |
|
||||||
|
|
||||||
- [ ] **Step 1: Write the failing tests** |
|
||||||
|
|
||||||
Add these five tests inside the existing `struct ScannerServiceTests { ... }` in `MusicTests/ScannerServiceTests.swift`, just before the closing brace: |
|
||||||
|
|
||||||
```swift |
|
||||||
// Verifies resolveBitrate uses the OS estimate when it is positive. |
|
||||||
// 1. Passes a positive estimatedDataRate in bits/sec (320450). |
|
||||||
// 2. Expects it rounded to kbps (320450/1000 = 320.45 -> 320), ignoring size/duration. |
|
||||||
@Test func resolveBitrateUsesEstimateWhenPositive() { |
|
||||||
let kbps = ScannerService.resolveBitrate(estimatedDataRate: 320_450, |
|
||||||
fileSizeBytes: 5_000_000, |
|
||||||
durationSeconds: 200) |
|
||||||
#expect(kbps == 320) |
|
||||||
} |
|
||||||
|
|
||||||
// Verifies the size/duration fallback when the OS estimate is 0 (the AVFoundation bug). |
|
||||||
// 1. Passes estimatedDataRate 0 with a real file size and duration. |
|
||||||
// 2. Expects 230_358_479 * 8 / 7198.54 / 1000 -> ~256.0 -> 256 kbps (matches ffprobe). |
|
||||||
@Test func resolveBitrateFallsBackToSizeAndDuration() { |
|
||||||
let kbps = ScannerService.resolveBitrate(estimatedDataRate: 0, |
|
||||||
fileSizeBytes: 230_358_479, |
|
||||||
durationSeconds: 7198.5371428571425) |
|
||||||
#expect(kbps == 256) |
|
||||||
} |
|
||||||
|
|
||||||
// Verifies nil (never 0) when the estimate is 0 and duration is unusable. |
|
||||||
// 1. Zero duration cannot yield a value -> nil. |
|
||||||
// 2. A NaN duration (CMTimeGetSeconds can return NaN) is also nil, not 0. |
|
||||||
@Test func resolveBitrateReturnsNilWhenNoDuration() { |
|
||||||
#expect(ScannerService.resolveBitrate(estimatedDataRate: 0, |
|
||||||
fileSizeBytes: 230_358_479, |
|
||||||
durationSeconds: 0) == nil) |
|
||||||
#expect(ScannerService.resolveBitrate(estimatedDataRate: 0, |
|
||||||
fileSizeBytes: 230_358_479, |
|
||||||
durationSeconds: .nan) == nil) |
|
||||||
} |
|
||||||
|
|
||||||
// Verifies nil when the estimate is 0 and there is no file size. |
|
||||||
// 1. Missing fileSizeBytes with estimate 0 -> nil (never 0). |
|
||||||
@Test func resolveBitrateReturnsNilWhenNoFileSize() { |
|
||||||
#expect(ScannerService.resolveBitrate(estimatedDataRate: 0, |
|
||||||
fileSizeBytes: nil, |
|
||||||
durationSeconds: 200) == nil) |
|
||||||
} |
|
||||||
|
|
||||||
// Verifies the core invariant: no input combination ever yields 0. |
|
||||||
// 1. All-zero inputs return nil so the UI renders "—" instead of "0 kbps". |
|
||||||
@Test func resolveBitrateNeverReturnsZero() { |
|
||||||
#expect(ScannerService.resolveBitrate(estimatedDataRate: 0, |
|
||||||
fileSizeBytes: 0, |
|
||||||
durationSeconds: 0) == nil) |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 2: Run the tests to verify they fail** |
|
||||||
|
|
||||||
Run: |
|
||||||
```bash |
|
||||||
xcodebuild test -project /Users/laurentmorvillier/code/Music/Music.xcodeproj \ |
|
||||||
-scheme Music -destination 'platform=macOS' \ |
|
||||||
-only-testing:MusicTests/ScannerServiceTests 2>&1 | tail -30 |
|
||||||
``` |
|
||||||
Expected: **compile failure** — `type 'ScannerService' has no member 'resolveBitrate'`. |
|
||||||
(If the destination errors, list options with `xcodebuild -showdestinations -project Music.xcodeproj -scheme Music` and use the macOS one.) |
|
||||||
|
|
||||||
- [ ] **Step 3: Write the minimal implementation** |
|
||||||
|
|
||||||
In `Music/Services/ScannerService.swift`, add this method right after the `discoverAudioFiles` static function (after line 35, inside the class): |
|
||||||
|
|
||||||
```swift |
|
||||||
/// Resolve a track's bitrate in kbps from the OS estimate, falling back to a |
|
||||||
/// size/duration average. Returns nil when nothing can be derived — never 0, |
|
||||||
/// so the UI shows "—" instead of a meaningless "0 kbps". |
|
||||||
/// |
|
||||||
/// AVFoundation's `estimatedDataRate` returns 0 for some files (observed on |
|
||||||
/// long/VBR MP3s); for those we compute the true average bitrate from the |
|
||||||
/// file size and duration, which matches ffprobe to the kbps. |
|
||||||
nonisolated static func resolveBitrate(estimatedDataRate: Double, |
|
||||||
fileSizeBytes: Int64?, |
|
||||||
durationSeconds: Double?) -> Int? { |
|
||||||
if estimatedDataRate > 0 { |
|
||||||
return Int((estimatedDataRate / 1000).rounded()) |
|
||||||
} |
|
||||||
// NaN-safe: `dur > 0` is false for .nan, so we return nil rather than 0. |
|
||||||
if let size = fileSizeBytes, size > 0, |
|
||||||
let dur = durationSeconds, dur > 0 { |
|
||||||
return Int((Double(size) * 8 / dur / 1000).rounded()) |
|
||||||
} |
|
||||||
return nil |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 4: Run the tests to verify they pass** |
|
||||||
|
|
||||||
Run: |
|
||||||
```bash |
|
||||||
xcodebuild test -project /Users/laurentmorvillier/code/Music/Music.xcodeproj \ |
|
||||||
-scheme Music -destination 'platform=macOS' \ |
|
||||||
-only-testing:MusicTests/ScannerServiceTests 2>&1 | tail -30 |
|
||||||
``` |
|
||||||
Expected: **TEST SUCCEEDED**, all `ScannerServiceTests` pass (the new five plus the existing `discoverAudioFiles`). |
|
||||||
|
|
||||||
- [ ] **Step 5: Commit checkpoint** |
|
||||||
|
|
||||||
Stop and run the `/commit` skill (do not `git commit` directly). Suggested message: `feat: add ScannerService.resolveBitrate with size/duration fallback`. |
|
||||||
|
|
||||||
--- |
|
||||||
|
|
||||||
### Task 2: Wire `resolveBitrate` into `extractMetadata` |
|
||||||
|
|
||||||
**Files:** |
|
||||||
- Modify: `Music/Services/ScannerService.swift:146-162` (the bitrate block inside `extractMetadata`) |
|
||||||
|
|
||||||
- [ ] **Step 1: Move the file-stats computation above the bitrate block** |
|
||||||
|
|
||||||
In `extractMetadata`, `TrackFileStats.compute` currently runs at line 162, *after* the bitrate block. Move it up so its `fileSize` is available to `resolveBitrate`. Replace the current block that spans from the duration load through the bitrate computation (lines 146-160): |
|
||||||
|
|
||||||
```swift |
|
||||||
let duration = try await asset.load(.duration) |
|
||||||
let durationSeconds = CMTimeGetSeconds(duration) |
|
||||||
|
|
||||||
var bitrate: Int? |
|
||||||
var sampleRate: Int? |
|
||||||
if let audioTrack = try await asset.loadTracks(withMediaType: .audio).first { |
|
||||||
let estimatedRate = try await audioTrack.load(.estimatedDataRate) |
|
||||||
bitrate = Int(estimatedRate / 1000) |
|
||||||
let descriptions = try await audioTrack.load(.formatDescriptions) |
|
||||||
if let desc = descriptions.first { |
|
||||||
if let asbd = CMAudioFormatDescriptionGetStreamBasicDescription(desc) { |
|
||||||
sampleRate = Int(asbd.pointee.mSampleRate) |
|
||||||
} |
|
||||||
} |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
with this: |
|
||||||
|
|
||||||
```swift |
|
||||||
let duration = try await asset.load(.duration) |
|
||||||
let durationSeconds = CMTimeGetSeconds(duration) |
|
||||||
|
|
||||||
let stats = try TrackFileStats.compute(for: url) |
|
||||||
|
|
||||||
var bitrate: Int? |
|
||||||
var sampleRate: Int? |
|
||||||
if let audioTrack = try await asset.loadTracks(withMediaType: .audio).first { |
|
||||||
let estimatedRate = try await audioTrack.load(.estimatedDataRate) |
|
||||||
bitrate = Self.resolveBitrate(estimatedDataRate: estimatedRate, |
|
||||||
fileSizeBytes: stats.fileSize, |
|
||||||
durationSeconds: durationSeconds) |
|
||||||
let descriptions = try await audioTrack.load(.formatDescriptions) |
|
||||||
if let desc = descriptions.first { |
|
||||||
if let asbd = CMAudioFormatDescriptionGetStreamBasicDescription(desc) { |
|
||||||
sampleRate = Int(asbd.pointee.mSampleRate) |
|
||||||
} |
|
||||||
} |
|
||||||
} else { |
|
||||||
// No audio track loaded — still attempt the size/duration fallback |
|
||||||
// so we never silently lose the bitrate. |
|
||||||
bitrate = Self.resolveBitrate(estimatedDataRate: 0, |
|
||||||
fileSizeBytes: stats.fileSize, |
|
||||||
durationSeconds: durationSeconds) |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 2: Remove the now-duplicate `TrackFileStats.compute` call** |
|
||||||
|
|
||||||
The original line 162 `let stats = try TrackFileStats.compute(for: url)` (now appearing just above the `return Track(` call) is a duplicate — delete that single line. The `Track(...)` initializer still references `stats.fileSize`, `stats.dateModified`, and `stats.fileHash`, which now come from the moved-up computation. Confirm exactly one `let stats = try TrackFileStats.compute(for: url)` remains in the function. |
|
||||||
|
|
||||||
- [ ] **Step 3: Build to verify it compiles** |
|
||||||
|
|
||||||
Run: |
|
||||||
```bash |
|
||||||
xcodebuild build -project /Users/laurentmorvillier/code/Music/Music.xcodeproj \ |
|
||||||
-scheme Music -destination 'platform=macOS' 2>&1 | tail -15 |
|
||||||
``` |
|
||||||
Expected: **BUILD SUCCEEDED**. |
|
||||||
|
|
||||||
- [ ] **Step 4: Re-run the full test target to confirm no regressions** |
|
||||||
|
|
||||||
Run: |
|
||||||
```bash |
|
||||||
xcodebuild test -project /Users/laurentmorvillier/code/Music/Music.xcodeproj \ |
|
||||||
-scheme Music -destination 'platform=macOS' \ |
|
||||||
-only-testing:MusicTests 2>&1 | tail -20 |
|
||||||
``` |
|
||||||
Expected: **TEST SUCCEEDED** for the whole `MusicTests` target. |
|
||||||
|
|
||||||
- [ ] **Step 5: Commit checkpoint** |
|
||||||
|
|
||||||
Stop and run the `/commit` skill. Suggested message: `fix: importer derives bitrate via resolveBitrate instead of storing 0`. |
|
||||||
|
|
||||||
--- |
|
||||||
|
|
||||||
### Task 3: Backfill script — pure helpers + self-test (Python, TDD) |
|
||||||
|
|
||||||
**Files:** |
|
||||||
- Create: `scripts/backfill_bitrate.py` |
|
||||||
|
|
||||||
- [ ] **Step 1: Create the script with helpers and a failing self-test** |
|
||||||
|
|
||||||
Create `scripts/backfill_bitrate.py` with exactly this content: |
|
||||||
|
|
||||||
```python |
|
||||||
#!/usr/bin/env python3 |
|
||||||
"""One-time backfill of real bitrate onto tracks stored with bitrate 0 or NULL. |
|
||||||
|
|
||||||
ScannerService writes `bitrate = Int(estimatedDataRate / 1000)` at scan time. |
|
||||||
AVFoundation's estimatedDataRate returns 0 for some files (long/VBR MP3s), so a |
|
||||||
literal 0 gets stored; other tracks were imported before bitrate existed and are |
|
||||||
NULL. This script recomputes bitrate for those rows using ffprobe, falling back |
|
||||||
to fileSize*8/duration (the same average the app's importer now uses) when |
|
||||||
ffprobe is unavailable or can't determine a value. |
|
||||||
|
|
||||||
Dry-run by default. Pass --apply to write (a timestamped backup is made first). |
|
||||||
|
|
||||||
Usage: |
|
||||||
python3 backfill_bitrate.py [--db <path>] [--apply] |
|
||||||
python3 backfill_bitrate.py --self-test |
|
||||||
|
|
||||||
Stdlib only; uses ffprobe if present on PATH (optional). |
|
||||||
""" |
|
||||||
|
|
||||||
import argparse |
|
||||||
import os |
|
||||||
import shutil |
|
||||||
import sqlite3 |
|
||||||
import subprocess |
|
||||||
import sys |
|
||||||
import unicodedata |
|
||||||
from datetime import datetime |
|
||||||
from urllib.parse import unquote |
|
||||||
|
|
||||||
# Default DB path for the sandboxed app (bundle id com.staxriver.mu). Computed from |
|
||||||
# $HOME so it resolves to the right user on whichever Mac the script runs on. |
|
||||||
DEFAULT_DB = os.path.expanduser( |
|
||||||
"~/Library/Containers/com.staxriver.mu/Data/Library/" |
|
||||||
"Application Support/Music/db.sqlite" |
|
||||||
) |
|
||||||
|
|
||||||
|
|
||||||
def norm_path(u): |
|
||||||
"""Reduce a file:// URL (or bare path) to a comparable, on-disk POSIX path. |
|
||||||
|
|
||||||
The app stores `fileURL` as Foundation's url.absoluteString (a percent-encoded |
|
||||||
file URL). Decode it, drop the file:// (or file://localhost) prefix, NFC- |
|
||||||
normalize, and strip a trailing slash so it can be stat'd on APFS. |
|
||||||
""" |
|
||||||
s = u |
|
||||||
if s.startswith("file://"): |
|
||||||
s = s[len("file://"):] |
|
||||||
if s.startswith("localhost/"): |
|
||||||
s = s[len("localhost"):] # leaves the leading "/" |
|
||||||
s = unquote(s) |
|
||||||
s = unicodedata.normalize("NFC", s) |
|
||||||
if len(s) > 1 and s.endswith("/"): |
|
||||||
s = s[:-1] |
|
||||||
return s |
|
||||||
|
|
||||||
|
|
||||||
def parse_ffprobe_bitrate(stdout): |
|
||||||
"""Parse ffprobe's bit_rate stdout (bits/sec) into integer kbps, or None. |
|
||||||
|
|
||||||
Returns None for empty output, 'N/A', or any non-integer text so the caller |
|
||||||
falls back to the formula. |
|
||||||
""" |
|
||||||
s = stdout.strip() |
|
||||||
if not s or s == "N/A": |
|
||||||
return None |
|
||||||
try: |
|
||||||
return round(int(s) / 1000) |
|
||||||
except ValueError: |
|
||||||
return None |
|
||||||
|
|
||||||
|
|
||||||
def kbps_from_ffprobe(path): |
|
||||||
"""Return integer kbps from ffprobe's format bit_rate, or None if unavailable. |
|
||||||
|
|
||||||
None on: ffprobe not installed, ffprobe error, or N/A/empty/non-integer output. |
|
||||||
""" |
|
||||||
try: |
|
||||||
out = subprocess.run( |
|
||||||
["ffprobe", "-v", "error", "-show_entries", "format=bit_rate", |
|
||||||
"-of", "default=nw=1:nk=1", path], |
|
||||||
capture_output=True, text=True, timeout=30, |
|
||||||
) |
|
||||||
except (FileNotFoundError, subprocess.SubprocessError): |
|
||||||
return None |
|
||||||
return parse_ffprobe_bitrate(out.stdout) |
|
||||||
|
|
||||||
|
|
||||||
def kbps_from_formula(file_size, duration): |
|
||||||
"""Average kbps from size (bytes) and duration (seconds): size*8/duration/1000. |
|
||||||
|
|
||||||
Returns None when inputs can't yield a meaningful value (missing size, or |
|
||||||
non-positive/missing duration). |
|
||||||
""" |
|
||||||
if not file_size or not duration or duration <= 0: |
|
||||||
return None |
|
||||||
return round(file_size * 8 / duration / 1000) |
|
||||||
|
|
||||||
|
|
||||||
def resolve_bitrate(path, duration): |
|
||||||
"""Best available kbps for an on-disk file: ffprobe first, formula fallback. |
|
||||||
|
|
||||||
`duration` is the DB's stored seconds; file size is read from disk. Returns |
|
||||||
None if neither method can produce a positive value. |
|
||||||
""" |
|
||||||
kbps = kbps_from_ffprobe(path) |
|
||||||
if kbps and kbps > 0: |
|
||||||
return kbps |
|
||||||
try: |
|
||||||
size = os.path.getsize(path) |
|
||||||
except OSError: |
|
||||||
size = None |
|
||||||
return kbps_from_formula(size, duration) |
|
||||||
|
|
||||||
|
|
||||||
def ffprobe_available(): |
|
||||||
return shutil.which("ffprobe") is not None |
|
||||||
|
|
||||||
|
|
||||||
def self_test(): |
|
||||||
"""Fast smoke check of the pure helpers (no DB, no ffprobe needed).""" |
|
||||||
# ffprobe stdout parsing |
|
||||||
assert parse_ffprobe_bitrate("256005\n") == 256 |
|
||||||
assert parse_ffprobe_bitrate("N/A") is None |
|
||||||
assert parse_ffprobe_bitrate("") is None |
|
||||||
assert parse_ffprobe_bitrate("garbage") is None |
|
||||||
|
|
||||||
# formula: 230_358_479 bytes over 7198.54 s -> 256 kbps (matches ffprobe sample) |
|
||||||
assert kbps_from_formula(230_358_479, 7198.5371428571425) == 256 |
|
||||||
assert kbps_from_formula(None, 100) is None |
|
||||||
assert kbps_from_formula(1000, 0) is None |
|
||||||
assert kbps_from_formula(1000, None) is None |
|
||||||
|
|
||||||
# path normalization (NFD vs NFC accents, percent-encoding, localhost host) |
|
||||||
nfc = norm_path("file:///Users/x/Mu%CC%81sica/Cafe%CC%81.mp3") |
|
||||||
nfd = norm_path("file://localhost/Users/x/M%C3%BAsica/Caf%C3%A9.mp3") |
|
||||||
assert nfc == nfd == "/Users/x/Música/Café.mp3", (nfc, nfd) |
|
||||||
assert norm_path("file:///a/b%20c%23d.mp3") == "/a/b c#d.mp3" |
|
||||||
|
|
||||||
print("self-test OK") |
|
||||||
|
|
||||||
|
|
||||||
def main(argv=None): |
|
||||||
p = argparse.ArgumentParser(description=__doc__) |
|
||||||
p.add_argument("--db", default=DEFAULT_DB, help=f"App DB path (default: {DEFAULT_DB})") |
|
||||||
p.add_argument("--apply", action="store_true", help="Write changes (default: dry run).") |
|
||||||
p.add_argument("--self-test", action="store_true", help="Run the built-in smoke test.") |
|
||||||
args = p.parse_args(argv) |
|
||||||
|
|
||||||
if args.self_test: |
|
||||||
self_test() |
|
||||||
return 0 |
|
||||||
|
|
||||||
if not os.path.exists(args.db): |
|
||||||
p.error(f"DB not found: {args.db}") |
|
||||||
|
|
||||||
run(args.db, args.apply) |
|
||||||
return 0 |
|
||||||
|
|
||||||
|
|
||||||
if __name__ == "__main__": |
|
||||||
sys.exit(main()) |
|
||||||
``` |
|
||||||
|
|
||||||
Note: `run(...)` is referenced by `main` but not yet defined — Task 4 adds it. The self-test does not call `run`, so `--self-test` works now. |
|
||||||
|
|
||||||
- [ ] **Step 2: Run the self-test to verify the helpers pass** |
|
||||||
|
|
||||||
Run: |
|
||||||
```bash |
|
||||||
python3 /Users/laurentmorvillier/code/Music/scripts/backfill_bitrate.py --self-test |
|
||||||
``` |
|
||||||
Expected: `self-test OK`. |
|
||||||
|
|
||||||
- [ ] **Step 3: Verify the dry-run path fails cleanly (run undefined)** |
|
||||||
|
|
||||||
Run: |
|
||||||
```bash |
|
||||||
python3 /Users/laurentmorvillier/code/Music/scripts/backfill_bitrate.py --db /nonexistent.sqlite |
|
||||||
``` |
|
||||||
Expected: argparse error `DB not found: /nonexistent.sqlite` (exit 2) — confirms arg handling before `run` is implemented. |
|
||||||
|
|
||||||
- [ ] **Step 4: Commit checkpoint** |
|
||||||
|
|
||||||
Stop and run the `/commit` skill. Suggested message: `feat: add backfill_bitrate.py helpers + self-test`. |
|
||||||
|
|
||||||
--- |
|
||||||
|
|
||||||
### Task 4: Backfill script — DB wiring, dry-run report, `--apply` |
|
||||||
|
|
||||||
**Files:** |
|
||||||
- Modify: `scripts/backfill_bitrate.py` (add `fetch_rows`, `build_updates`, `backup_db`, `apply_updates`, `run`) |
|
||||||
|
|
||||||
- [ ] **Step 1: Add the DB + reporting functions** |
|
||||||
|
|
||||||
Insert these functions into `scripts/backfill_bitrate.py` immediately before `def main(`: |
|
||||||
|
|
||||||
```python |
|
||||||
def fetch_rows(db_path): |
|
||||||
"""Return candidate rows: (id, fileURL, duration, bitrate) where bitrate is 0/NULL.""" |
|
||||||
con = sqlite3.connect(db_path) |
|
||||||
try: |
|
||||||
return con.execute( |
|
||||||
"SELECT id, fileURL, duration, bitrate FROM tracks " |
|
||||||
"WHERE bitrate = 0 OR bitrate IS NULL" |
|
||||||
).fetchall() |
|
||||||
finally: |
|
||||||
con.close() |
|
||||||
|
|
||||||
|
|
||||||
def build_updates(rows): |
|
||||||
"""Resolve a new bitrate for each candidate row. |
|
||||||
|
|
||||||
Returns (updates, missing, undeterminable): |
|
||||||
- updates: list of {id, file_url, old, new} where new is a positive kbps |
|
||||||
- missing: (id, path) for rows whose file is not on disk (left untouched) |
|
||||||
- undeterminable: (id, path) for on-disk files whose bitrate couldn't be found |
|
||||||
""" |
|
||||||
updates, missing, undeterminable = [], [], [] |
|
||||||
for row_id, file_url, duration, old in rows: |
|
||||||
path = norm_path(file_url) |
|
||||||
if not os.path.exists(path): |
|
||||||
missing.append((row_id, path)) |
|
||||||
continue |
|
||||||
new = resolve_bitrate(path, duration) |
|
||||||
if not new or new <= 0: |
|
||||||
undeterminable.append((row_id, path)) |
|
||||||
continue |
|
||||||
updates.append({"id": row_id, "file_url": file_url, "old": old, "new": new}) |
|
||||||
return updates, missing, undeterminable |
|
||||||
|
|
||||||
|
|
||||||
def backup_db(db_path): |
|
||||||
"""Copy db.sqlite (+ -wal, -shm) under backups/<timestamp>/ next to the DB.""" |
|
||||||
stamp = datetime.now().strftime("%Y%m%d-%H%M%S") |
|
||||||
backup_dir = os.path.join(os.path.dirname(db_path), "backups", stamp) |
|
||||||
os.makedirs(backup_dir, exist_ok=True) |
|
||||||
for suffix in ("", "-wal", "-shm"): |
|
||||||
src = db_path + suffix |
|
||||||
if os.path.exists(src): |
|
||||||
shutil.copy2(src, os.path.join(backup_dir, os.path.basename(src))) |
|
||||||
return backup_dir |
|
||||||
|
|
||||||
|
|
||||||
def apply_updates(db_path, updates): |
|
||||||
"""Write bitrate updates in a single transaction, then checkpoint the WAL.""" |
|
||||||
con = sqlite3.connect(db_path) |
|
||||||
try: |
|
||||||
con.execute("BEGIN") |
|
||||||
con.executemany("UPDATE tracks SET bitrate=:new WHERE id=:id", updates) |
|
||||||
con.commit() |
|
||||||
con.execute("PRAGMA wal_checkpoint(TRUNCATE)") |
|
||||||
finally: |
|
||||||
con.close() |
|
||||||
|
|
||||||
|
|
||||||
def run(db_path, apply): |
|
||||||
rows = fetch_rows(db_path) |
|
||||||
updates, missing, undeterminable = build_updates(rows) |
|
||||||
|
|
||||||
print(f"Candidate rows (bitrate 0 or NULL): {len(rows)}") |
|
||||||
print(f"Resolvable (will set): {len(updates)}") |
|
||||||
print(f"Skipped — file missing on disk: {len(missing)}") |
|
||||||
print(f"Skipped — could not determine: {len(undeterminable)}") |
|
||||||
if not ffprobe_available(): |
|
||||||
print("NOTE: ffprobe not on PATH — used the filesize/duration formula for all rows.") |
|
||||||
print() |
|
||||||
|
|
||||||
for u in updates[:15]: |
|
||||||
name = os.path.basename(norm_path(u["file_url"])) |
|
||||||
old = "NULL" if u["old"] is None else u["old"] |
|
||||||
print(f" • {name}") |
|
||||||
print(f" bitrate {old} -> {u['new']} kbps") |
|
||||||
if len(updates) > 15: |
|
||||||
print(f" ... and {len(updates) - 15} more") |
|
||||||
print() |
|
||||||
|
|
||||||
if missing[:5]: |
|
||||||
print("Sample of skipped (file missing on disk, left untouched):") |
|
||||||
for row_id, path in missing[:5]: |
|
||||||
print(f" - [{row_id}] {os.path.basename(path)}") |
|
||||||
print() |
|
||||||
|
|
||||||
if undeterminable[:5]: |
|
||||||
print("Sample of skipped (could not determine bitrate, left untouched):") |
|
||||||
for row_id, path in undeterminable[:5]: |
|
||||||
print(f" - [{row_id}] {os.path.basename(path)}") |
|
||||||
print() |
|
||||||
|
|
||||||
if not apply: |
|
||||||
print("DRY RUN — nothing written. Re-run with --apply to commit these changes.") |
|
||||||
return |
|
||||||
|
|
||||||
if not updates: |
|
||||||
print("Nothing to apply.") |
|
||||||
return |
|
||||||
|
|
||||||
backup_dir = backup_db(db_path) |
|
||||||
print(f"Backup written to: {backup_dir}") |
|
||||||
apply_updates(db_path, updates) |
|
||||||
print(f"Applied {len(updates)} bitrate updates to {db_path}") |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 2: Re-run the self-test (ensure the new code didn't break helpers)** |
|
||||||
|
|
||||||
Run: |
|
||||||
```bash |
|
||||||
python3 /Users/laurentmorvillier/code/Music/scripts/backfill_bitrate.py --self-test |
|
||||||
``` |
|
||||||
Expected: `self-test OK`. |
|
||||||
|
|
||||||
- [ ] **Step 3: Dry-run against a temp DB to verify end-to-end wiring** |
|
||||||
|
|
||||||
This builds a tiny throwaway DB with one real bitrate=0 row, so the run path is exercised without touching the app DB: |
|
||||||
|
|
||||||
```bash |
|
||||||
python3 - <<'PY' |
|
||||||
import sqlite3, os, tempfile |
|
||||||
d = tempfile.mkdtemp() |
|
||||||
db = os.path.join(d, "db.sqlite") |
|
||||||
con = sqlite3.connect(db) |
|
||||||
con.execute("CREATE TABLE tracks (id INTEGER PRIMARY KEY, fileURL TEXT, duration REAL, bitrate INTEGER)") |
|
||||||
# A file that does not exist -> should be reported as 'missing', not crash. |
|
||||||
con.execute("INSERT INTO tracks (fileURL, duration, bitrate) VALUES (?,?,?)", |
|
||||||
("file:///no/such/file.mp3", 100.0, 0)) |
|
||||||
con.commit(); con.close() |
|
||||||
print(db) |
|
||||||
PY |
|
||||||
``` |
|
||||||
Take the printed path and run: |
|
||||||
```bash |
|
||||||
python3 /Users/laurentmorvillier/code/Music/scripts/backfill_bitrate.py --db <printed-path> |
|
||||||
``` |
|
||||||
Expected: a summary with `Candidate rows ... : 1`, `Skipped — file missing on disk: 1`, and `DRY RUN — nothing written.` (no traceback). |
|
||||||
|
|
||||||
- [ ] **Step 4: Commit checkpoint** |
|
||||||
|
|
||||||
Stop and run the `/commit` skill. Suggested message: `feat: backfill_bitrate.py DB wiring, dry-run report, --apply`. |
|
||||||
|
|
||||||
--- |
|
||||||
|
|
||||||
### Task 5: Verify against the real library (manual, no code change) |
|
||||||
|
|
||||||
**Files:** none. |
|
||||||
|
|
||||||
- [ ] **Step 1: Dry-run against the real app DB** |
|
||||||
|
|
||||||
Quit the Music app first (avoids WAL/lock contention). Then: |
|
||||||
```bash |
|
||||||
python3 /Users/laurentmorvillier/code/Music/scripts/backfill_bitrate.py |
|
||||||
``` |
|
||||||
Expected: a non-zero `Resolvable (will set)` count and a sample of `bitrate 0 -> NNN kbps` lines. Eyeball a few values for plausibility (typical 128–320 kbps). |
|
||||||
|
|
||||||
- [ ] **Step 2: Apply** |
|
||||||
|
|
||||||
```bash |
|
||||||
python3 /Users/laurentmorvillier/code/Music/scripts/backfill_bitrate.py --apply |
|
||||||
``` |
|
||||||
Expected: `Backup written to: .../backups/<timestamp>` then `Applied N bitrate updates`. |
|
||||||
|
|
||||||
- [ ] **Step 3: Confirm the DB no longer has 0/NULL bitrates (except undeterminable)** |
|
||||||
|
|
||||||
```bash |
|
||||||
DB="$HOME/Library/Containers/com.staxriver.mu/Data/Library/Application Support/Music/db.sqlite" |
|
||||||
sqlite3 "$DB" "SELECT COUNT(*) AS still_zero_or_null FROM tracks WHERE bitrate = 0 OR bitrate IS NULL;" |
|
||||||
``` |
|
||||||
Expected: `0`, or the small count the dry-run reported as "could not determine"/"file missing". |
|
||||||
|
|
||||||
- [ ] **Step 4: Reopen the app and spot-check** |
|
||||||
|
|
||||||
Open a previously-0 track's Get Info (or the Bit Rate column) and confirm it now shows a real value. |
|
||||||
|
|
||||||
--- |
|
||||||
|
|
||||||
## Self-Review Notes |
|
||||||
|
|
||||||
- **Spec coverage:** Root cause + invariant → Tasks 1–2 (`resolveBitrate`, never stores 0). Backfill (0 and NULL, ffprobe→formula, missing-file skip, dry-run/backup/apply) → Tasks 3–4. Manual verification → Task 5. All spec sections covered. |
|
||||||
- **Type consistency:** `resolveBitrate(estimatedDataRate:fileSizeBytes:durationSeconds:)` is defined identically in Task 1 and called identically in Task 2; `stats.fileSize` is `Int64` (matches `fileSizeBytes: Int64?`). Python helper names (`parse_ffprobe_bitrate`, `kbps_from_ffprobe`, `kbps_from_formula`, `resolve_bitrate`, `fetch_rows`, `build_updates`, `backup_db`, `apply_updates`, `run`) are defined once and referenced consistently. |
|
||||||
- **No placeholders:** every code step shows complete code; every run step shows the command and expected output. |
|
||||||
@ -1,780 +0,0 @@ |
|||||||
# Playing Queue Implementation Plan |
|
||||||
|
|
||||||
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. |
|
||||||
|
|
||||||
**Goal:** Add a Spotify-style priority "Up Next" queue: tracks can be pushed to the front ("Play Next") or end ("Add to Queue") via the track context menu, played before the playlist/album context resumes, and managed in a right-docked panel. |
|
||||||
|
|
||||||
**Architecture:** `PlayerViewModel` keeps its existing `queue`/`currentIndex` as the playback **context** and gains a parallel `manualQueue` of `QueueEntry` (a track + stable UUID). `next()` drains the manual queue before advancing the context; a dedicated `playManual(_:)` plays a queued track without moving `currentIndex`, so the context resumes correctly. A new `QueueView` renders the panel; the context menu and `ContentView` are wired up. Local-only for v1 — queue actions are hidden when driving a remote device. |
|
||||||
|
|
||||||
**Tech Stack:** Swift, SwiftUI + AppKit (`NSTableView`), Swift Testing (`import Testing`), Xcode 16 synchronized groups (no `pbxproj` edits for new files). |
|
||||||
|
|
||||||
**Spec:** `docs/superpowers/specs/2026-05-30-playing-queue-design.md` |
|
||||||
|
|
||||||
> **Project rule — commits:** This repo's CLAUDE.md says *never commit unless triggered by the `/commit` skill*. The "Commit" steps below are checkpoints: stage the listed files and ask the user to run `/commit` (or run it when they direct). Do **not** commit autonomously. |
|
||||||
|
|
||||||
> **Test command:** `xcodebuild test -scheme Music -destination 'platform=macOS' -only-testing:<TestTarget/Suite> 2>&1 | tail -30`. Full build: `xcodebuild build -scheme Music -destination 'platform=macOS' 2>&1 | tail -20`. |
|
||||||
|
|
||||||
--- |
|
||||||
|
|
||||||
## Task 1: `QueueEntry` model + queue state and add actions |
|
||||||
|
|
||||||
**Files:** |
|
||||||
- Create: `Music/Models/QueueEntry.swift` |
|
||||||
- Modify: `Music/ViewModels/PlayerViewModel.swift` (add state + methods) |
|
||||||
- Test: `MusicTests/PlayerViewModelTests.swift` |
|
||||||
|
|
||||||
- [ ] **Step 1: Write the failing tests** |
|
||||||
|
|
||||||
Add these tests inside the `PlayerViewModelTests` struct in `MusicTests/PlayerViewModelTests.swift` (after the existing tests, before the closing `}` of the struct): |
|
||||||
|
|
||||||
```swift |
|
||||||
// Step 1: context [1,2,3], track 1 playing. |
|
||||||
// Step 2: addToQueue twice → manual queue holds those tracks in arrival order. |
|
||||||
// Step 3: playNext jumps a track to the FRONT of the manual queue. |
|
||||||
@Test func addToQueueAppendsAndPlayNextInsertsFront() { |
|
||||||
let vm = PlayerViewModel(provider: AudioService(), db: nil) |
|
||||||
let tracks = makeTracks(6) |
|
||||||
vm.setQueue(Array(tracks[0..<3])) |
|
||||||
vm.play(tracks[0]) |
|
||||||
|
|
||||||
vm.addToQueue(tracks[3]) // id 4 |
|
||||||
vm.addToQueue(tracks[4]) // id 5 |
|
||||||
#expect(vm.manualQueue.map { $0.track.id } == [4, 5]) |
|
||||||
|
|
||||||
vm.playNext(tracks[5]) // id 6 to the front |
|
||||||
#expect(vm.manualQueue.map { $0.track.id } == [6, 4, 5]) |
|
||||||
} |
|
||||||
|
|
||||||
// Step 1: a view model with nothing playing (idle). |
|
||||||
// Step 2: addToQueue should start playback immediately (queue-while-idle) and |
|
||||||
// leave the manual queue empty because the track was consumed to play. |
|
||||||
@Test func queueWhileIdleStartsPlayback() { |
|
||||||
let vm = PlayerViewModel(provider: AudioService(), db: nil) |
|
||||||
let track = Track.fixture(id: 1, fileURL: "/a.mp3", title: "A") |
|
||||||
|
|
||||||
vm.addToQueue(track) |
|
||||||
|
|
||||||
#expect(vm.currentTrack?.id == 1) |
|
||||||
#expect(vm.manualQueue.isEmpty) |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 2: Run the tests to verify they fail** |
|
||||||
|
|
||||||
Run: `xcodebuild test -scheme Music -destination 'platform=macOS' -only-testing:MusicTests/PlayerViewModelTests 2>&1 | tail -30` |
|
||||||
Expected: FAIL to compile — `value of type 'PlayerViewModel' has no member 'manualQueue' / 'addToQueue' / 'playNext'`. |
|
||||||
|
|
||||||
- [ ] **Step 3: Create the `QueueEntry` model** |
|
||||||
|
|
||||||
Create `Music/Models/QueueEntry.swift`: |
|
||||||
|
|
||||||
```swift |
|
||||||
import Foundation |
|
||||||
|
|
||||||
// A single slot in the manual "Up Next" queue. Carries its own stable identity so |
|
||||||
// the same track can be queued more than once without SwiftUI confusing the rows — |
|
||||||
// Track.id alone is not unique across duplicate queue entries. |
|
||||||
nonisolated struct QueueEntry: Identifiable { |
|
||||||
let id = UUID() |
|
||||||
let track: Track |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 4: Add queue state to `PlayerViewModel`** |
|
||||||
|
|
||||||
In `Music/ViewModels/PlayerViewModel.swift`, add the new stored properties immediately after the `originalQueue` line (currently line 20): |
|
||||||
|
|
||||||
```swift |
|
||||||
private var originalQueue: [Track] = [] |
|
||||||
/// The manual "Up Next" queue. Plays ahead of `queue` (the context) and survives |
|
||||||
/// starting a new context. `queue`/`currentIndex` remain the CONTEXT position. |
|
||||||
private(set) var manualQueue: [QueueEntry] = [] |
|
||||||
/// Display label for the panel's "Next from: <name>" section. |
|
||||||
private(set) var contextName: String? |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 5: Add the manual-queue methods and `playManual`** |
|
||||||
|
|
||||||
In the same file, add a new section just after the `// MARK: - Queue Management` block (after `setQueue`'s closing brace, currently line 99). Note the `remoteProvider == nil` guard — the manual queue is local-only for v1: |
|
||||||
|
|
||||||
```swift |
|
||||||
// MARK: - Manual Queue |
|
||||||
|
|
||||||
func playNext(_ track: Track) { |
|
||||||
guard remoteProvider == nil else { return } |
|
||||||
manualQueue.insert(QueueEntry(track: track), at: 0) |
|
||||||
startQueuedTrackIfIdle() |
|
||||||
} |
|
||||||
|
|
||||||
func addToQueue(_ track: Track) { |
|
||||||
guard remoteProvider == nil else { return } |
|
||||||
manualQueue.append(QueueEntry(track: track)) |
|
||||||
startQueuedTrackIfIdle() |
|
||||||
} |
|
||||||
|
|
||||||
func removeFromQueue(at offsets: IndexSet) { |
|
||||||
manualQueue.remove(atOffsets: offsets) |
|
||||||
} |
|
||||||
|
|
||||||
func moveInQueue(from source: IndexSet, to destination: Int) { |
|
||||||
manualQueue.move(fromOffsets: source, toOffset: destination) |
|
||||||
} |
|
||||||
|
|
||||||
/// Context tracks after the current context position — the panel's "Next from" |
|
||||||
/// section. Empty when there is no context or we are at its end. |
|
||||||
var upcomingContext: [Track] { |
|
||||||
guard let idx = currentIndex, idx + 1 < queue.count else { return [] } |
|
||||||
return Array(queue[(idx + 1)...]) |
|
||||||
} |
|
||||||
|
|
||||||
// If nothing is playing, start the just-queued track immediately rather than |
|
||||||
// parking it — matches Spotify's "queue while idle starts playback". |
|
||||||
private func startQueuedTrackIfIdle() { |
|
||||||
guard currentTrack == nil, !manualQueue.isEmpty else { return } |
|
||||||
let entry = manualQueue.removeFirst() |
|
||||||
playManual(entry.track) |
|
||||||
} |
|
||||||
|
|
||||||
// Plays a track pulled from the manual queue. Mirrors play(_:) but deliberately |
|
||||||
// does NOT touch currentIndex, so the context position is preserved and resumes |
|
||||||
// once the manual queue drains. |
|
||||||
private func playManual(_ track: Track) { |
|
||||||
currentTrack = track |
|
||||||
halfwayReported = false |
|
||||||
isPlaying = true |
|
||||||
currentTime = 0 |
|
||||||
duration = track.duration |
|
||||||
guard let url = provider.urlForTrack(track) else { return } |
|
||||||
provider.play(url: url) |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 6: Run the tests to verify they pass** |
|
||||||
|
|
||||||
Run: `xcodebuild test -scheme Music -destination 'platform=macOS' -only-testing:MusicTests/PlayerViewModelTests 2>&1 | tail -30` |
|
||||||
Expected: PASS (the two new tests plus all existing PlayerViewModel tests). |
|
||||||
|
|
||||||
- [ ] **Step 7: Commit (checkpoint — via `/commit`)** |
|
||||||
|
|
||||||
Stage and request a commit: |
|
||||||
|
|
||||||
```bash |
|
||||||
git add Music/Models/QueueEntry.swift Music/ViewModels/PlayerViewModel.swift MusicTests/PlayerViewModelTests.swift |
|
||||||
# Then ask the user to run /commit (suggested message: "feat: add manual queue state and Play Next / Add to Queue to PlayerViewModel") |
|
||||||
``` |
|
||||||
|
|
||||||
--- |
|
||||||
|
|
||||||
## Task 2: `next()` drains the manual queue, then resumes the context |
|
||||||
|
|
||||||
**Files:** |
|
||||||
- Modify: `Music/ViewModels/PlayerViewModel.swift:160-172` (the `next()` method) |
|
||||||
- Test: `MusicTests/PlayerViewModelTests.swift` |
|
||||||
|
|
||||||
- [ ] **Step 1: Write the failing tests** |
|
||||||
|
|
||||||
Add to `PlayerViewModelTests`: |
|
||||||
|
|
||||||
```swift |
|
||||||
// Step 1: context [1,2,3], track 1 playing (currentIndex 0). |
|
||||||
// Step 2: queue track 5. next() must play the QUEUED track, not context track 2, |
|
||||||
// consume it from the queue, and leave currentIndex at 0 (context held). |
|
||||||
@Test func nextConsumesManualQueueBeforeContext() { |
|
||||||
let vm = PlayerViewModel(provider: AudioService(), db: nil) |
|
||||||
let tracks = makeTracks(6) |
|
||||||
vm.setQueue(Array(tracks[0..<3])) |
|
||||||
vm.play(tracks[0]) |
|
||||||
|
|
||||||
vm.addToQueue(tracks[4]) // id 5 |
|
||||||
vm.next() |
|
||||||
|
|
||||||
#expect(vm.currentTrack?.id == 5) |
|
||||||
#expect(vm.manualQueue.isEmpty) |
|
||||||
#expect(vm.currentIndex == 0) |
|
||||||
} |
|
||||||
|
|
||||||
// Step 1: context [1,2,3], track 1 playing; queue track 5. |
|
||||||
// Step 2: first next() plays the queued track 5 (context still at index 0). |
|
||||||
// Step 3: second next() finds the queue empty and resumes the context at index 1 |
|
||||||
// → track 2. |
|
||||||
@Test func contextResumesAfterManualQueueDrains() { |
|
||||||
let vm = PlayerViewModel(provider: AudioService(), db: nil) |
|
||||||
let tracks = makeTracks(6) |
|
||||||
vm.setQueue(Array(tracks[0..<3])) |
|
||||||
vm.play(tracks[0]) |
|
||||||
|
|
||||||
vm.addToQueue(tracks[4]) // id 5 |
|
||||||
vm.next() // plays queued track 5 |
|
||||||
vm.next() // resumes context |
|
||||||
|
|
||||||
#expect(vm.currentTrack?.id == 2) |
|
||||||
#expect(vm.currentIndex == 1) |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 2: Run the tests to verify they fail** |
|
||||||
|
|
||||||
Run: `xcodebuild test -scheme Music -destination 'platform=macOS' -only-testing:MusicTests/PlayerViewModelTests/nextConsumesManualQueueBeforeContext 2>&1 | tail -30` |
|
||||||
Expected: FAIL — `next()` currently advances the context, so `currentTrack?.id` is `2`, not `5`. |
|
||||||
|
|
||||||
- [ ] **Step 3: Update `next()`** |
|
||||||
|
|
||||||
In `Music/ViewModels/PlayerViewModel.swift`, replace the existing `next()` method: |
|
||||||
|
|
||||||
```swift |
|
||||||
func next() { |
|
||||||
if let remote = remoteProvider { |
|
||||||
remote.sendNext() |
|
||||||
return |
|
||||||
} |
|
||||||
guard let idx = currentIndex else { return } |
|
||||||
let nextIdx = idx + 1 |
|
||||||
if nextIdx < queue.count { |
|
||||||
play(queue[nextIdx]) |
|
||||||
} else { |
|
||||||
stop() |
|
||||||
} |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
with: |
|
||||||
|
|
||||||
```swift |
|
||||||
func next() { |
|
||||||
if let remote = remoteProvider { |
|
||||||
remote.sendNext() |
|
||||||
return |
|
||||||
} |
|
||||||
// Manual queue takes priority and is consumed as it plays. |
|
||||||
if !manualQueue.isEmpty { |
|
||||||
let entry = manualQueue.removeFirst() |
|
||||||
playManual(entry.track) |
|
||||||
return |
|
||||||
} |
|
||||||
// Otherwise advance the context from the preserved position. |
|
||||||
guard let idx = currentIndex else { return } |
|
||||||
let nextIdx = idx + 1 |
|
||||||
if nextIdx < queue.count { |
|
||||||
play(queue[nextIdx]) |
|
||||||
} else { |
|
||||||
stop() |
|
||||||
} |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 4: Run the tests to verify they pass** |
|
||||||
|
|
||||||
Run: `xcodebuild test -scheme Music -destination 'platform=macOS' -only-testing:MusicTests/PlayerViewModelTests 2>&1 | tail -30` |
|
||||||
Expected: PASS (new tests + existing tests, including `nextAtEndStops` and `nextAdvancesToNextTrack`, still green). |
|
||||||
|
|
||||||
- [ ] **Step 5: Commit (checkpoint — via `/commit`)** |
|
||||||
|
|
||||||
```bash |
|
||||||
git add Music/ViewModels/PlayerViewModel.swift MusicTests/PlayerViewModelTests.swift |
|
||||||
# Suggested message: "feat: next() drains the manual queue before advancing the context" |
|
||||||
``` |
|
||||||
|
|
||||||
--- |
|
||||||
|
|
||||||
## Task 3: Edit ops, `upcomingContext`, `setQueue(contextName:)`, shuffle isolation |
|
||||||
|
|
||||||
**Files:** |
|
||||||
- Modify: `Music/ViewModels/PlayerViewModel.swift` (`setQueue`, `setProvider`) |
|
||||||
- Test: `MusicTests/PlayerViewModelTests.swift` |
|
||||||
|
|
||||||
- [ ] **Step 1: Write the failing tests** |
|
||||||
|
|
||||||
Add to `PlayerViewModelTests`: |
|
||||||
|
|
||||||
```swift |
|
||||||
// Step 1: context [1,2,3], track 1 playing; queue tracks 4,5,6. |
|
||||||
// Step 2: removeFromQueue removes the middle entry → [4,6]. |
|
||||||
// Step 3: moveInQueue moves the last entry to the front → [6,4]. |
|
||||||
@Test func removeAndMoveMutateManualQueue() { |
|
||||||
let vm = PlayerViewModel(provider: AudioService(), db: nil) |
|
||||||
let tracks = makeTracks(6) |
|
||||||
vm.setQueue(Array(tracks[0..<3])) |
|
||||||
vm.play(tracks[0]) |
|
||||||
vm.addToQueue(tracks[3]); vm.addToQueue(tracks[4]); vm.addToQueue(tracks[5]) |
|
||||||
#expect(vm.manualQueue.map { $0.track.id } == [4, 5, 6]) |
|
||||||
|
|
||||||
vm.removeFromQueue(at: IndexSet(integer: 1)) |
|
||||||
#expect(vm.manualQueue.map { $0.track.id } == [4, 6]) |
|
||||||
|
|
||||||
vm.moveInQueue(from: IndexSet(integer: 1), to: 0) |
|
||||||
#expect(vm.manualQueue.map { $0.track.id } == [6, 4]) |
|
||||||
} |
|
||||||
|
|
||||||
// Step 1: context [1,2,3,4], track 2 playing (currentIndex 1). |
|
||||||
// Step 2: upcomingContext is the slice after the current position → [3,4]. |
|
||||||
@Test func upcomingContextReturnsTracksAfterCurrent() { |
|
||||||
let vm = PlayerViewModel(provider: AudioService(), db: nil) |
|
||||||
let tracks = makeTracks(4) |
|
||||||
vm.setQueue(tracks) |
|
||||||
vm.play(tracks[1]) |
|
||||||
|
|
||||||
#expect(vm.upcomingContext.map { $0.id } == [3, 4]) |
|
||||||
} |
|
||||||
|
|
||||||
// Step 1: 10-track context, one playing; queue tracks 11 and 12 in order. |
|
||||||
// Step 2: toggling shuffle reorders only the context — the manual queue order |
|
||||||
// must be left exactly as the user arranged it. |
|
||||||
@Test func shuffleLeavesManualQueueIntact() { |
|
||||||
let vm = PlayerViewModel(provider: AudioService(), db: nil) |
|
||||||
let tracks = makeTracks(12) |
|
||||||
vm.setQueue(Array(tracks[0..<10])) |
|
||||||
vm.play(tracks[0]) |
|
||||||
vm.addToQueue(tracks[10]) // id 11 |
|
||||||
vm.addToQueue(tracks[11]) // id 12 |
|
||||||
|
|
||||||
vm.toggleShuffle() |
|
||||||
|
|
||||||
#expect(vm.manualQueue.map { $0.track.id } == [11, 12]) |
|
||||||
} |
|
||||||
|
|
||||||
// Step 1: setQueue accepts an optional context label for the panel header. |
|
||||||
@Test func setQueueStoresContextName() { |
|
||||||
let vm = PlayerViewModel(provider: AudioService(), db: nil) |
|
||||||
vm.setQueue(makeTracks(2), contextName: "Synthwave") |
|
||||||
#expect(vm.contextName == "Synthwave") |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 2: Run the tests to verify they fail** |
|
||||||
|
|
||||||
Run: `xcodebuild test -scheme Music -destination 'platform=macOS' -only-testing:MusicTests/PlayerViewModelTests/setQueueStoresContextName 2>&1 | tail -30` |
|
||||||
Expected: FAIL to compile — `setQueue` has no `contextName:` parameter. (`removeAndMoveMutateManualQueue`, `upcomingContext...`, and `shuffleLeaves...` already pass from Task 1's methods, except the `contextName` compile error blocks the whole suite.) |
|
||||||
|
|
||||||
- [ ] **Step 3: Add the `contextName` parameter to `setQueue`** |
|
||||||
|
|
||||||
In `Music/ViewModels/PlayerViewModel.swift`, replace the `setQueue` signature line: |
|
||||||
|
|
||||||
```swift |
|
||||||
func setQueue(_ tracks: [Track]) { |
|
||||||
originalQueue = tracks |
|
||||||
``` |
|
||||||
|
|
||||||
with: |
|
||||||
|
|
||||||
```swift |
|
||||||
func setQueue(_ tracks: [Track], contextName: String? = nil) { |
|
||||||
self.contextName = contextName |
|
||||||
originalQueue = tracks |
|
||||||
``` |
|
||||||
|
|
||||||
(The default `nil` keeps existing callers and tests compiling.) |
|
||||||
|
|
||||||
- [ ] **Step 4: Reset the new state in `setProvider`** |
|
||||||
|
|
||||||
In the `setProvider(_:)` method, add the two resets next to the existing `queue = []` / `originalQueue = []` lines (currently lines 55-56): |
|
||||||
|
|
||||||
```swift |
|
||||||
queue = [] |
|
||||||
originalQueue = [] |
|
||||||
manualQueue = [] |
|
||||||
contextName = nil |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 5: Run the tests to verify they pass** |
|
||||||
|
|
||||||
Run: `xcodebuild test -scheme Music -destination 'platform=macOS' -only-testing:MusicTests/PlayerViewModelTests 2>&1 | tail -30` |
|
||||||
Expected: PASS (all PlayerViewModel tests, old and new). |
|
||||||
|
|
||||||
- [ ] **Step 6: Commit (checkpoint — via `/commit`)** |
|
||||||
|
|
||||||
```bash |
|
||||||
git add Music/ViewModels/PlayerViewModel.swift MusicTests/PlayerViewModelTests.swift |
|
||||||
# Suggested message: "feat: queue edit ops, upcomingContext, and contextName label" |
|
||||||
``` |
|
||||||
|
|
||||||
--- |
|
||||||
|
|
||||||
## Task 4: Context-menu config — `onPlayNext` / `onAddToQueue` + both menu builders |
|
||||||
|
|
||||||
**Files:** |
|
||||||
- Modify: `Music/Models/TrackContextMenuConfig.swift` |
|
||||||
- Modify: `Music/Views/TrackContextMenuModifier.swift` (SwiftUI menu) |
|
||||||
- Modify: `Music/Views/TrackTableView.swift:328-394` (AppKit menu + actions) |
|
||||||
- Test: `MusicTests/TrackContextMenuConfigTests.swift` |
|
||||||
|
|
||||||
- [ ] **Step 1: Write the failing test** |
|
||||||
|
|
||||||
The two new config fields get `= nil` defaults (Step 3), so the existing |
|
||||||
`TrackContextMenuConfig(...)` constructions in this file and in `ContentView` |
|
||||||
keep compiling untouched. Just add a new test to `TrackContextMenuConfigTests`: |
|
||||||
|
|
||||||
```swift |
|
||||||
// Verifies the queue callbacks fire with the right track. |
|
||||||
@Test func queueCallbacksFire() { |
|
||||||
let track = Track.fixture(id: 7, title: "Q") |
|
||||||
var playNextTrack: Track? = nil |
|
||||||
var addQueueTrack: Track? = nil |
|
||||||
|
|
||||||
let config = TrackContextMenuConfig( |
|
||||||
playlists: [], |
|
||||||
lastUsedPlaylistName: nil, |
|
||||||
selectedPlaylist: nil, |
|
||||||
onAddToPlaylist: { _, _ in }, |
|
||||||
onAddToLastPlaylist: nil, |
|
||||||
onRemoveFromPlaylist: nil, |
|
||||||
onPlayNext: { t in playNextTrack = t }, |
|
||||||
onAddToQueue: { t in addQueueTrack = t } |
|
||||||
) |
|
||||||
|
|
||||||
config.onPlayNext?(track) |
|
||||||
config.onAddToQueue?(track) |
|
||||||
|
|
||||||
#expect(playNextTrack?.id == 7) |
|
||||||
#expect(addQueueTrack?.id == 7) |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 2: Run the test to verify it fails** |
|
||||||
|
|
||||||
Run: `xcodebuild test -scheme Music -destination 'platform=macOS' -only-testing:MusicTests/TrackContextMenuConfigTests 2>&1 | tail -30` |
|
||||||
Expected: FAIL to compile — `extra arguments 'onPlayNext', 'onAddToQueue'` (the struct has no such members yet). |
|
||||||
|
|
||||||
- [ ] **Step 3: Add the closures to the config struct** |
|
||||||
|
|
||||||
In `Music/Models/TrackContextMenuConfig.swift`, add the two fields after `onRemoveFromPlaylist`. The `= nil` defaults flow into the synthesized memberwise initializer, so existing callers (ContentView + the other config test) keep compiling and the items stay hidden until wired: |
|
||||||
|
|
||||||
```swift |
|
||||||
let onRemoveFromPlaylist: ((Track) -> Void)? |
|
||||||
// nil hides the corresponding item (e.g. when driving a remote device). |
|
||||||
let onPlayNext: ((Track) -> Void)? = nil |
|
||||||
let onAddToQueue: ((Track) -> Void)? = nil |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 4: Run the config test to verify it passes** |
|
||||||
|
|
||||||
Run: `xcodebuild test -scheme Music -destination 'platform=macOS' -only-testing:MusicTests/TrackContextMenuConfigTests 2>&1 | tail -30` |
|
||||||
Expected: PASS, and the app target still builds (existing callers use the `nil` defaults). |
|
||||||
|
|
||||||
> Note: the menu items are wired with real closures in Task 6. Until then `ContentView` passes the default `nil`, so the items stay hidden — expected interim state. |
|
||||||
|
|
||||||
- [ ] **Step 5: Render the items in the SwiftUI menu** |
|
||||||
|
|
||||||
In `Music/Views/TrackContextMenuModifier.swift`, inside `if let track, let config {`, insert this block **before** the existing `lastUsedPlaylistName` block (so Play Next / Add to Queue appear at the top): |
|
||||||
|
|
||||||
```swift |
|
||||||
if let onPlayNext = config.onPlayNext { |
|
||||||
Button("Play Next") { onPlayNext(track) } |
|
||||||
} |
|
||||||
if let onAddToQueue = config.onAddToQueue { |
|
||||||
Button("Add to Queue") { onAddToQueue(track) } |
|
||||||
} |
|
||||||
if config.onPlayNext != nil || config.onAddToQueue != nil { |
|
||||||
Divider() |
|
||||||
} |
|
||||||
|
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 6: Render the items in the AppKit menu** |
|
||||||
|
|
||||||
In `Music/Views/TrackTableView.swift`, in `menuNeedsUpdate(_:)`, insert this block immediately after the two `guard` lines (after `guard let config = parent.contextMenuConfig else { return }`) and **before** the `if let lastPlaylistName` block: |
|
||||||
|
|
||||||
```swift |
|
||||||
if config.onPlayNext != nil { |
|
||||||
let item = NSMenuItem(title: "Play Next", action: #selector(playNext(_:)), keyEquivalent: "") |
|
||||||
item.target = self |
|
||||||
menu.addItem(item) |
|
||||||
} |
|
||||||
if config.onAddToQueue != nil { |
|
||||||
let item = NSMenuItem(title: "Add to Queue", action: #selector(addToQueue(_:)), keyEquivalent: "") |
|
||||||
item.target = self |
|
||||||
menu.addItem(item) |
|
||||||
} |
|
||||||
if config.onPlayNext != nil || config.onAddToQueue != nil { |
|
||||||
menu.addItem(.separator()) |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
Then add the two action handlers next to the existing `addToLastPlaylist` / `removeFromPlaylist` handlers (after `removeFromPlaylist(_:)`'s closing brace, currently line 394): |
|
||||||
|
|
||||||
```swift |
|
||||||
@objc func playNext(_ sender: NSMenuItem) { |
|
||||||
guard let tableView, tableView.clickedRow >= 0, tableView.clickedRow < tracks.count else { return } |
|
||||||
guard let config = parent.contextMenuConfig else { return } |
|
||||||
config.onPlayNext?(tracks[tableView.clickedRow]) |
|
||||||
} |
|
||||||
|
|
||||||
@objc func addToQueue(_ sender: NSMenuItem) { |
|
||||||
guard let tableView, tableView.clickedRow >= 0, tableView.clickedRow < tracks.count else { return } |
|
||||||
guard let config = parent.contextMenuConfig else { return } |
|
||||||
config.onAddToQueue?(tracks[tableView.clickedRow]) |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 7: Commit (checkpoint — via `/commit`)** |
|
||||||
|
|
||||||
```bash |
|
||||||
git add Music/Models/TrackContextMenuConfig.swift Music/Views/TrackContextMenuModifier.swift Music/Views/TrackTableView.swift MusicTests/TrackContextMenuConfigTests.swift |
|
||||||
# Suggested message: "feat: add Play Next / Add to Queue context-menu items" |
|
||||||
``` |
|
||||||
|
|
||||||
--- |
|
||||||
|
|
||||||
## Task 5: `QueueView` panel + `PlayerControlsView` toggle button |
|
||||||
|
|
||||||
**Files:** |
|
||||||
- Create: `Music/Views/QueueView.swift` |
|
||||||
- Modify: `Music/Views/PlayerControlsView.swift` |
|
||||||
|
|
||||||
This task is UI; it is verified by a clean build and (optionally) by running the app, not by unit tests. |
|
||||||
|
|
||||||
- [ ] **Step 1: Create `QueueView`** |
|
||||||
|
|
||||||
Create `Music/Views/QueueView.swift`: |
|
||||||
|
|
||||||
```swift |
|
||||||
import SwiftUI |
|
||||||
|
|
||||||
// The right-docked "Up Next" panel. The manual "Queue" section is reorderable and |
|
||||||
// removable; the "Next from" section is the read-only upcoming context (double-click |
|
||||||
// a row to jump to it). |
|
||||||
struct QueueView: View { |
|
||||||
var player: PlayerViewModel |
|
||||||
|
|
||||||
var body: some View { |
|
||||||
List { |
|
||||||
if player.manualQueue.isEmpty && player.upcomingContext.isEmpty { |
|
||||||
Text("Queue is empty.\nRight-click a track → Add to Queue.") |
|
||||||
.font(.system(size: 12)) |
|
||||||
.foregroundStyle(.secondary) |
|
||||||
.multilineTextAlignment(.center) |
|
||||||
.frame(maxWidth: .infinity, alignment: .center) |
|
||||||
.padding(.vertical, 24) |
|
||||||
.listRowSeparator(.hidden) |
|
||||||
} |
|
||||||
|
|
||||||
if !player.manualQueue.isEmpty { |
|
||||||
Section("Queue") { |
|
||||||
ForEach(player.manualQueue) { entry in |
|
||||||
HStack(spacing: 8) { |
|
||||||
trackRow(entry.track) |
|
||||||
Spacer() |
|
||||||
Button { |
|
||||||
if let idx = player.manualQueue.firstIndex(where: { $0.id == entry.id }) { |
|
||||||
player.removeFromQueue(at: IndexSet(integer: idx)) |
|
||||||
} |
|
||||||
} label: { |
|
||||||
Image(systemName: "xmark.circle.fill") |
|
||||||
.foregroundStyle(.tertiary) |
|
||||||
} |
|
||||||
.buttonStyle(.plain) |
|
||||||
} |
|
||||||
} |
|
||||||
.onMove(perform: player.moveInQueue) |
|
||||||
} |
|
||||||
} |
|
||||||
|
|
||||||
if !player.upcomingContext.isEmpty { |
|
||||||
Section("Next from: \(player.contextName ?? "Library")") { |
|
||||||
ForEach(Array(player.upcomingContext.enumerated()), id: \.offset) { _, track in |
|
||||||
trackRow(track) |
|
||||||
.contentShape(Rectangle()) |
|
||||||
.onTapGesture(count: 2) { player.play(track) } |
|
||||||
} |
|
||||||
} |
|
||||||
} |
|
||||||
} |
|
||||||
.listStyle(.inset) |
|
||||||
.frame(width: 280) |
|
||||||
} |
|
||||||
|
|
||||||
private func trackRow(_ track: Track) -> some View { |
|
||||||
VStack(alignment: .leading, spacing: 2) { |
|
||||||
Text(track.title) |
|
||||||
.font(.system(size: 12, weight: .medium)) |
|
||||||
.lineLimit(1) |
|
||||||
Text(track.artist) |
|
||||||
.font(.system(size: 10)) |
|
||||||
.foregroundStyle(.secondary) |
|
||||||
.lineLimit(1) |
|
||||||
} |
|
||||||
} |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 2: Add toggle inputs to `PlayerControlsView`** |
|
||||||
|
|
||||||
In `Music/Views/PlayerControlsView.swift`, add three properties immediately after `var contextMenuConfig: TrackContextMenuConfig? = nil` (line 23): |
|
||||||
|
|
||||||
```swift |
|
||||||
var isQueueVisible: Bool = false |
|
||||||
var showQueueButton: Bool = true |
|
||||||
var onToggleQueue: (() -> Void)? = nil |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 3: Render the queue button** |
|
||||||
|
|
||||||
In the same file, replace the start of `volumeSection`: |
|
||||||
|
|
||||||
```swift |
|
||||||
private var volumeSection: some View { |
|
||||||
HStack(spacing: 8) { |
|
||||||
Image(systemName: volumeIconName) |
|
||||||
``` |
|
||||||
|
|
||||||
with: |
|
||||||
|
|
||||||
```swift |
|
||||||
private var volumeSection: some View { |
|
||||||
HStack(spacing: 8) { |
|
||||||
if showQueueButton { |
|
||||||
Button(action: { onToggleQueue?() }) { |
|
||||||
Image(systemName: "list.bullet") |
|
||||||
.font(.system(size: 13)) |
|
||||||
.foregroundStyle(isQueueVisible ? .blue : .secondary) |
|
||||||
} |
|
||||||
.buttonStyle(.plain) |
|
||||||
} |
|
||||||
|
|
||||||
Image(systemName: volumeIconName) |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 4: Build to verify it compiles** |
|
||||||
|
|
||||||
Run: `xcodebuild build -scheme Music -destination 'platform=macOS' 2>&1 | tail -20` |
|
||||||
Expected: `** BUILD SUCCEEDED **` (the new toggle props are unused until Task 6 wires them — defaults keep `ContentView` compiling). |
|
||||||
|
|
||||||
- [ ] **Step 5: Commit (checkpoint — via `/commit`)** |
|
||||||
|
|
||||||
```bash |
|
||||||
git add Music/Views/QueueView.swift Music/Views/PlayerControlsView.swift |
|
||||||
# Suggested message: "feat: add Up Next QueueView panel and transport queue toggle" |
|
||||||
``` |
|
||||||
|
|
||||||
--- |
|
||||||
|
|
||||||
## Task 6: Wire everything into `ContentView` |
|
||||||
|
|
||||||
**Files:** |
|
||||||
- Modify: `Music/ContentView.swift` |
|
||||||
|
|
||||||
UI integration — verified by a full build and the complete test suite. |
|
||||||
|
|
||||||
- [ ] **Step 1: Add panel visibility state** |
|
||||||
|
|
||||||
In `Music/ContentView.swift`, add to the `@State` block (e.g. after `@State private var showHome = false`, line 24): |
|
||||||
|
|
||||||
```swift |
|
||||||
@State private var showQueue = false |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 2: Wire the queue closures into the context-menu config** |
|
||||||
|
|
||||||
Replace the whole `trackContextMenuConfig` computed property (lines 358-377) with: |
|
||||||
|
|
||||||
```swift |
|
||||||
private var trackContextMenuConfig: TrackContextMenuConfig { |
|
||||||
// Queue actions are local-only for v1: hidden when driving a remote device. |
|
||||||
let queueEnabled = !(networkStatus?.isRemoteMode ?? false) |
|
||||||
return TrackContextMenuConfig( |
|
||||||
playlists: playlist.playlists, |
|
||||||
lastUsedPlaylistName: playlist.lastUsedPlaylistName, |
|
||||||
selectedPlaylist: playlist.selectedPlaylist, |
|
||||||
onAddToPlaylist: { track, targetPlaylist in |
|
||||||
try? playlist.addTrack(track, to: targetPlaylist) |
|
||||||
}, |
|
||||||
onAddToLastPlaylist: { track in |
|
||||||
try? playlist.addTrackToLastUsedPlaylist(track) |
|
||||||
}, |
|
||||||
// Outer nil hides the "Remove from Playlist" menu item when not in a playlist view. |
|
||||||
// Inner re-check defends against the playlist being deselected between menu display and action. |
|
||||||
onRemoveFromPlaylist: playlist.selectedPlaylist != nil ? { track in |
|
||||||
if let selected = playlist.selectedPlaylist { |
|
||||||
try? playlist.removeTrack(track, from: selected) |
|
||||||
} |
|
||||||
} : nil, |
|
||||||
onPlayNext: queueEnabled ? { track in player.playNext(track) } : nil, |
|
||||||
onAddToQueue: queueEnabled ? { track in player.addToQueue(track) } : nil |
|
||||||
) |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 3: Dock the panel beside the main content** |
|
||||||
|
|
||||||
In `body`, wrap the main-content region in an `HStack` and append the panel. Replace the opening of that region (currently lines 118-119): |
|
||||||
|
|
||||||
```swift |
|
||||||
VStack(spacing: 0) { |
|
||||||
if showHome || playlist.selectedItem != nil { |
|
||||||
``` |
|
||||||
|
|
||||||
with: |
|
||||||
|
|
||||||
```swift |
|
||||||
HStack(spacing: 0) { |
|
||||||
VStack(spacing: 0) { |
|
||||||
if showHome || playlist.selectedItem != nil { |
|
||||||
``` |
|
||||||
|
|
||||||
Then replace its closing `.frame(maxHeight: .infinity)` (currently line 191) with: |
|
||||||
|
|
||||||
```swift |
|
||||||
} |
|
||||||
.frame(maxHeight: .infinity) |
|
||||||
|
|
||||||
if showQueue { |
|
||||||
Divider() |
|
||||||
QueueView(player: player) |
|
||||||
} |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
(The first `}` closes the existing inner `VStack`; the new outer `}` closes the added `HStack`.) |
|
||||||
|
|
||||||
- [ ] **Step 4: Pass context labels at every `setQueue` call site** |
|
||||||
|
|
||||||
Make these four edits in `ContentView.swift`: |
|
||||||
|
|
||||||
1. HomeView `onTrackDoubleClick` (line 155): `player.setQueue(recentTracks)` → `player.setQueue(recentTracks, contextName: "Recently Added")` |
|
||||||
2. TrackTableView `onDoubleClick` (line 178): `player.setQueue(trackList)` → `player.setQueue(trackList, contextName: playlist.selectedItem?.name ?? "Library")` |
|
||||||
3. `onPlayPause` empty-state (line 393): `player.setQueue(trackList)` → `player.setQueue(trackList, contextName: playlist.selectedItem?.name ?? "Library")` |
|
||||||
4. Keyboard space handler (line 426): `player.setQueue(trackList)` → `player.setQueue(trackList, contextName: playlist.selectedItem?.name ?? "Library")` |
|
||||||
|
|
||||||
- [ ] **Step 5: Pass toggle props to `PlayerControlsView`** |
|
||||||
|
|
||||||
In the `playerControls` computed property, add these arguments after `onNowPlayingTap:` and before `contextMenuConfig:` (line 408-409): |
|
||||||
|
|
||||||
```swift |
|
||||||
onNowPlayingTap: { scrollToPlayingTrigger = UUID() }, |
|
||||||
isQueueVisible: showQueue, |
|
||||||
showQueueButton: !(networkStatus?.isRemoteMode ?? false), |
|
||||||
onToggleQueue: { showQueue.toggle() }, |
|
||||||
contextMenuConfig: trackContextMenuConfig |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 6: Build to verify it compiles** |
|
||||||
|
|
||||||
Run: `xcodebuild build -scheme Music -destination 'platform=macOS' 2>&1 | tail -20` |
|
||||||
Expected: `** BUILD SUCCEEDED **`. |
|
||||||
|
|
||||||
- [ ] **Step 7: Run the full test suite** |
|
||||||
|
|
||||||
Run: `xcodebuild test -scheme Music -destination 'platform=macOS' 2>&1 | tail -30` |
|
||||||
Expected: all tests pass, including the new queue tests and all pre-existing suites. |
|
||||||
|
|
||||||
- [ ] **Step 8: Manual verification (optional but recommended)** |
|
||||||
|
|
||||||
Use the `/run` or `/verify` skill to launch the app and confirm: |
|
||||||
- Right-click a track → "Play Next" and "Add to Queue" appear and work. |
|
||||||
- The transport `list.bullet` button toggles the right panel. |
|
||||||
- Queued tracks show under "Queue", reorder by drag, and remove via the × button. |
|
||||||
- Playing a queued track removes it from the panel; after the queue drains, the original playlist resumes at the right spot. |
|
||||||
|
|
||||||
- [ ] **Step 9: Commit (checkpoint — via `/commit`)** |
|
||||||
|
|
||||||
```bash |
|
||||||
git add Music/ContentView.swift |
|
||||||
# Suggested message: "feat: wire Up Next panel, queue toggle, and queue actions into ContentView" |
|
||||||
``` |
|
||||||
|
|
||||||
--- |
|
||||||
|
|
||||||
## Self-Review Notes (for the implementer) |
|
||||||
|
|
||||||
- **Backward compatibility:** `queue`/`currentIndex` keep meaning the *context*; every pre-existing `PlayerViewModelTests` case must stay green at each task. If one breaks, the new logic touched the context path incorrectly. |
|
||||||
- **Remote gate:** queue methods early-return when `remoteProvider != nil`, and `ContentView` passes `nil` queue closures + hides the button when `networkStatus.isRemoteMode`. Streaming-client mode is *not* gated (it plays locally). |
|
||||||
- **Duplicates:** `QueueEntry.id` (UUID) is the SwiftUI identity, so the same track can be queued multiple times without row glitches; removal looks up the entry by `id`, never by track. |
|
||||||
File diff suppressed because it is too large
Load Diff
@ -1,525 +0,0 @@ |
|||||||
# Track Context Menu on Bottom Controls — Implementation Plan |
|
||||||
|
|
||||||
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. |
|
||||||
|
|
||||||
**Goal:** Right-clicking the now-playing area at bottom-left shows the same Add/Remove playlist context menu as right-clicking a track row in the track table. |
|
||||||
|
|
||||||
**Architecture:** Introduce a `TrackContextMenuConfig` value type that bundles all menu data + callbacks. A new `TrackContextMenuModifier` SwiftUI view modifier applies `.contextMenu` using that config. `TrackTableView` is refactored to accept a single `contextMenuConfig` parameter (replacing six individual playlist params), and `PlayerControlsView` gains the same optional parameter with the modifier applied to `nowPlayingSection`. `ContentView` constructs one config and passes it to both views. |
|
||||||
|
|
||||||
**Tech Stack:** Swift 5.9+, SwiftUI `.contextMenu`, AppKit `NSMenu` (table view keeps existing AppKit path), Swift Testing framework. |
|
||||||
|
|
||||||
--- |
|
||||||
|
|
||||||
## File Map |
|
||||||
|
|
||||||
| File | Action | |
|
||||||
|------|--------| |
|
||||||
| `Music/Models/TrackContextMenuConfig.swift` | **Create** — value type holding playlists + callbacks | |
|
||||||
| `Music/Views/TrackContextMenuModifier.swift` | **Create** — SwiftUI ViewModifier + View extension | |
|
||||||
| `MusicTests/TrackContextMenuConfigTests.swift` | **Create** — unit tests for config struct | |
|
||||||
| `Music/Views/TrackTableView.swift` | **Modify** — replace 6 playlist params with `contextMenuConfig`, update `menuNeedsUpdate` | |
|
||||||
| `Music/Views/PlayerControlsView.swift` | **Modify** — add `contextMenuConfig` param, apply modifier to `nowPlayingSection` | |
|
||||||
| `Music/ContentView.swift` | **Modify** — construct and pass `TrackContextMenuConfig` to both views | |
|
||||||
|
|
||||||
--- |
|
||||||
|
|
||||||
### Task 1: Create `TrackContextMenuConfig` |
|
||||||
|
|
||||||
**Files:** |
|
||||||
- Create: `Music/Models/TrackContextMenuConfig.swift` |
|
||||||
- Test: `MusicTests/TrackContextMenuConfigTests.swift` |
|
||||||
|
|
||||||
- [ ] **Step 1: Write the failing test** |
|
||||||
|
|
||||||
Create `MusicTests/TrackContextMenuConfigTests.swift`: |
|
||||||
|
|
||||||
```swift |
|
||||||
import Testing |
|
||||||
@testable import Music |
|
||||||
|
|
||||||
struct TrackContextMenuConfigTests { |
|
||||||
// Builds a config with all fields set and verifies: |
|
||||||
// - stored playlists, lastUsedPlaylistName, selectedPlaylist match the inputs |
|
||||||
// - onAddToPlaylist callback fires with the correct track and playlist |
|
||||||
// - onAddToLastPlaylist callback fires with the correct track |
|
||||||
// - onRemoveFromPlaylist callback fires with the correct track |
|
||||||
// - when optional callbacks are nil, optionally calling them is safe |
|
||||||
|
|
||||||
@Test func storesPropertiesAndFiresCallbacks() { |
|
||||||
// 1. Create fixture data |
|
||||||
let pl1 = Playlist.fixture(id: 1, name: "Favorites") |
|
||||||
let pl2 = Playlist.fixture(id: 2, name: "Chill") |
|
||||||
let track = Track.fixture(id: 42, title: "Test") |
|
||||||
|
|
||||||
var addedTrack: Track? = nil |
|
||||||
var addedPlaylist: Playlist? = nil |
|
||||||
var lastTrack: Track? = nil |
|
||||||
var removedTrack: Track? = nil |
|
||||||
|
|
||||||
// 2. Build config with all callbacks |
|
||||||
let config = TrackContextMenuConfig( |
|
||||||
playlists: [pl1, pl2], |
|
||||||
lastUsedPlaylistName: "Favorites", |
|
||||||
selectedPlaylist: pl1, |
|
||||||
onAddToPlaylist: { t, p in addedTrack = t; addedPlaylist = p }, |
|
||||||
onAddToLastPlaylist: { t in lastTrack = t }, |
|
||||||
onRemoveFromPlaylist: { t in removedTrack = t } |
|
||||||
) |
|
||||||
|
|
||||||
// 3. Verify stored properties |
|
||||||
#expect(config.playlists.count == 2) |
|
||||||
#expect(config.playlists[0].name == "Favorites") |
|
||||||
#expect(config.lastUsedPlaylistName == "Favorites") |
|
||||||
#expect(config.selectedPlaylist == pl1) |
|
||||||
|
|
||||||
// 4. Invoke callbacks and verify they fire correctly |
|
||||||
config.onAddToPlaylist(track, pl2) |
|
||||||
config.onAddToLastPlaylist?(track) |
|
||||||
config.onRemoveFromPlaylist?(track) |
|
||||||
|
|
||||||
#expect(addedTrack?.id == track.id) |
|
||||||
#expect(addedPlaylist?.id == pl2.id) |
|
||||||
#expect(lastTrack?.id == track.id) |
|
||||||
#expect(removedTrack?.id == track.id) |
|
||||||
} |
|
||||||
|
|
||||||
@Test func nilOptionalCallbacksAreSafe() { |
|
||||||
// Verifies that a config with nil optional callbacks does not crash |
|
||||||
// when you call them via optional chaining (the normal usage pattern) |
|
||||||
let pl = Playlist.fixture(id: 1, name: "Rock") |
|
||||||
let track = Track.fixture() |
|
||||||
|
|
||||||
let config = TrackContextMenuConfig( |
|
||||||
playlists: [pl], |
|
||||||
lastUsedPlaylistName: nil, |
|
||||||
selectedPlaylist: nil, |
|
||||||
onAddToPlaylist: { _, _ in }, |
|
||||||
onAddToLastPlaylist: nil, |
|
||||||
onRemoveFromPlaylist: nil |
|
||||||
) |
|
||||||
|
|
||||||
// These must not crash |
|
||||||
config.onAddToLastPlaylist?(track) |
|
||||||
config.onRemoveFromPlaylist?(track) |
|
||||||
|
|
||||||
#expect(config.lastUsedPlaylistName == nil) |
|
||||||
#expect(config.selectedPlaylist == nil) |
|
||||||
} |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 2: Run tests to confirm they fail (type not found)** |
|
||||||
|
|
||||||
``` |
|
||||||
xcodebuild test -scheme Music -destination 'platform=macOS' -only-testing MusicTests/TrackContextMenuConfigTests 2>&1 | grep -E "error:|FAIL|PASS|warning: cannot" |
|
||||||
``` |
|
||||||
|
|
||||||
Expected: compile error — `TrackContextMenuConfig` not found. |
|
||||||
|
|
||||||
- [ ] **Step 3: Create `Music/Models/TrackContextMenuConfig.swift`** |
|
||||||
|
|
||||||
```swift |
|
||||||
import Foundation |
|
||||||
|
|
||||||
struct TrackContextMenuConfig { |
|
||||||
let playlists: [Playlist] |
|
||||||
let lastUsedPlaylistName: String? |
|
||||||
let selectedPlaylist: Playlist? |
|
||||||
let onAddToPlaylist: (Track, Playlist) -> Void |
|
||||||
let onAddToLastPlaylist: ((Track) -> Void)? |
|
||||||
let onRemoveFromPlaylist: ((Track) -> Void)? |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 4: Add the new file to the Xcode project** |
|
||||||
|
|
||||||
In Xcode, right-click the `Models` group in the project navigator → **Add Files to "Music"** → select `TrackContextMenuConfig.swift`. Make sure "Add to targets: Music" is checked. |
|
||||||
|
|
||||||
- [ ] **Step 5: Run tests to confirm they pass** |
|
||||||
|
|
||||||
``` |
|
||||||
xcodebuild test -scheme Music -destination 'platform=macOS' -only-testing MusicTests/TrackContextMenuConfigTests 2>&1 | grep -E "Test.*passed|Test.*failed|error:" |
|
||||||
``` |
|
||||||
|
|
||||||
Expected: both tests pass. |
|
||||||
|
|
||||||
- [ ] **Step 6: Commit** |
|
||||||
|
|
||||||
```bash |
|
||||||
git add Music/Models/TrackContextMenuConfig.swift MusicTests/TrackContextMenuConfigTests.swift |
|
||||||
git commit -m "feat: add TrackContextMenuConfig value type" |
|
||||||
``` |
|
||||||
|
|
||||||
--- |
|
||||||
|
|
||||||
### Task 2: Create `TrackContextMenuModifier` |
|
||||||
|
|
||||||
**Files:** |
|
||||||
- Create: `Music/Views/TrackContextMenuModifier.swift` |
|
||||||
|
|
||||||
> No unit test for this task — SwiftUI view modifier behaviour requires UI/snapshot testing not set up in this project. Manual verification is in Task 5. |
|
||||||
|
|
||||||
- [ ] **Step 1: Create `Music/Views/TrackContextMenuModifier.swift`** |
|
||||||
|
|
||||||
```swift |
|
||||||
import SwiftUI |
|
||||||
|
|
||||||
// Attaches a context menu matching the track table's right-click menu. |
|
||||||
// No-ops silently when track or config is nil so callers can pass optionals freely. |
|
||||||
struct TrackContextMenuModifier: ViewModifier { |
|
||||||
let track: Track? |
|
||||||
let config: TrackContextMenuConfig? |
|
||||||
|
|
||||||
func body(content: Content) -> some View { |
|
||||||
if let track, let config { |
|
||||||
content.contextMenu { |
|
||||||
if let lastPlaylistName = config.lastUsedPlaylistName, |
|
||||||
let onAddToLastPlaylist = config.onAddToLastPlaylist { |
|
||||||
Button("Add to \(lastPlaylistName)") { |
|
||||||
onAddToLastPlaylist(track) |
|
||||||
} |
|
||||||
Divider() |
|
||||||
} |
|
||||||
|
|
||||||
if !config.playlists.isEmpty { |
|
||||||
Menu("Add to Playlist") { |
|
||||||
ForEach(config.playlists) { playlist in |
|
||||||
Button(playlist.name) { |
|
||||||
config.onAddToPlaylist(track, playlist) |
|
||||||
} |
|
||||||
} |
|
||||||
} |
|
||||||
} |
|
||||||
|
|
||||||
if config.selectedPlaylist != nil, |
|
||||||
let onRemoveFromPlaylist = config.onRemoveFromPlaylist { |
|
||||||
Divider() |
|
||||||
Button("Remove from Playlist") { |
|
||||||
onRemoveFromPlaylist(track) |
|
||||||
} |
|
||||||
} |
|
||||||
} |
|
||||||
} else { |
|
||||||
content |
|
||||||
} |
|
||||||
} |
|
||||||
} |
|
||||||
|
|
||||||
extension View { |
|
||||||
func trackContextMenu(track: Track?, config: TrackContextMenuConfig?) -> some View { |
|
||||||
modifier(TrackContextMenuModifier(track: track, config: config)) |
|
||||||
} |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 2: Add the new file to the Xcode project** |
|
||||||
|
|
||||||
In Xcode, right-click the `Views` group → **Add Files to "Music"** → select `TrackContextMenuModifier.swift`. Make sure "Add to targets: Music" is checked. |
|
||||||
|
|
||||||
- [ ] **Step 3: Build to confirm it compiles** |
|
||||||
|
|
||||||
``` |
|
||||||
xcodebuild build -scheme Music -destination 'platform=macOS' 2>&1 | grep -E "error:|BUILD SUCCEEDED|BUILD FAILED" |
|
||||||
``` |
|
||||||
|
|
||||||
Expected: `BUILD SUCCEEDED`. |
|
||||||
|
|
||||||
- [ ] **Step 4: Commit** |
|
||||||
|
|
||||||
```bash |
|
||||||
git add Music/Views/TrackContextMenuModifier.swift |
|
||||||
git commit -m "feat: add TrackContextMenuModifier SwiftUI view modifier" |
|
||||||
``` |
|
||||||
|
|
||||||
--- |
|
||||||
|
|
||||||
### Task 3: Refactor `TrackTableView` to use `contextMenuConfig` |
|
||||||
|
|
||||||
**Files:** |
|
||||||
- Modify: `Music/Views/TrackTableView.swift:45-50` (replace 6 playlist params) |
|
||||||
- Modify: `Music/Views/TrackTableView.swift:333-394` (update `menuNeedsUpdate` + action handlers) |
|
||||||
|
|
||||||
- [ ] **Step 1: Replace the 6 individual playlist properties with `contextMenuConfig`** |
|
||||||
|
|
||||||
In `Music/Views/TrackTableView.swift`, find lines 45–51: |
|
||||||
|
|
||||||
```swift |
|
||||||
var playlists: [Playlist] |
|
||||||
var lastUsedPlaylistName: String? |
|
||||||
var selectedPlaylist: Playlist? |
|
||||||
var onAddToPlaylist: ((Track, Playlist) -> Void)? |
|
||||||
var onAddToLastPlaylist: ((Track) -> Void)? |
|
||||||
var onRemoveFromPlaylist: ((Track) -> Void)? |
|
||||||
var onReorder: ((Int, Int) -> Void)? |
|
||||||
``` |
|
||||||
|
|
||||||
Replace with: |
|
||||||
|
|
||||||
```swift |
|
||||||
var contextMenuConfig: TrackContextMenuConfig? |
|
||||||
var onReorder: ((Int, Int) -> Void)? |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 2: Update `menuNeedsUpdate` to read from `contextMenuConfig`** |
|
||||||
|
|
||||||
Find the entire `menuNeedsUpdate` method (lines ~333–375) and replace it: |
|
||||||
|
|
||||||
```swift |
|
||||||
func menuNeedsUpdate(_ menu: NSMenu) { |
|
||||||
menu.removeAllItems() |
|
||||||
guard let tableView, tableView.clickedRow >= 0, tableView.clickedRow < tracks.count else { return } |
|
||||||
guard let config = parent.contextMenuConfig else { return } |
|
||||||
|
|
||||||
if let lastPlaylistName = config.lastUsedPlaylistName, config.onAddToLastPlaylist != nil { |
|
||||||
let lastItem = NSMenuItem( |
|
||||||
title: "Add to \(lastPlaylistName)", |
|
||||||
action: #selector(addToLastPlaylist(_:)), |
|
||||||
keyEquivalent: "" |
|
||||||
) |
|
||||||
lastItem.target = self |
|
||||||
menu.addItem(lastItem) |
|
||||||
menu.addItem(.separator()) |
|
||||||
} |
|
||||||
|
|
||||||
if !config.playlists.isEmpty { |
|
||||||
let submenu = NSMenu() |
|
||||||
for (index, playlist) in config.playlists.enumerated() { |
|
||||||
let item = NSMenuItem( |
|
||||||
title: playlist.name, |
|
||||||
action: #selector(addToPlaylist(_:)), |
|
||||||
keyEquivalent: "" |
|
||||||
) |
|
||||||
item.target = self |
|
||||||
item.tag = index |
|
||||||
submenu.addItem(item) |
|
||||||
} |
|
||||||
let submenuItem = NSMenuItem(title: "Add to Playlist", action: nil, keyEquivalent: "") |
|
||||||
submenuItem.submenu = submenu |
|
||||||
menu.addItem(submenuItem) |
|
||||||
} |
|
||||||
|
|
||||||
if config.selectedPlaylist != nil, config.onRemoveFromPlaylist != nil { |
|
||||||
menu.addItem(.separator()) |
|
||||||
let removeItem = NSMenuItem( |
|
||||||
title: "Remove from Playlist", |
|
||||||
action: #selector(removeFromPlaylist(_:)), |
|
||||||
keyEquivalent: "" |
|
||||||
) |
|
||||||
removeItem.target = self |
|
||||||
menu.addItem(removeItem) |
|
||||||
} |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 3: Update the three `@objc` menu action methods to use `contextMenuConfig`** |
|
||||||
|
|
||||||
Find `addToPlaylist`, `addToLastPlaylist`, and `removeFromPlaylist` (lines ~377–394) and replace all three: |
|
||||||
|
|
||||||
```swift |
|
||||||
@objc func addToPlaylist(_ sender: NSMenuItem) { |
|
||||||
guard let tableView, tableView.clickedRow >= 0, tableView.clickedRow < tracks.count else { return } |
|
||||||
guard let config = parent.contextMenuConfig else { return } |
|
||||||
let track = tracks[tableView.clickedRow] |
|
||||||
let playlist = config.playlists[sender.tag] |
|
||||||
config.onAddToPlaylist(track, playlist) |
|
||||||
} |
|
||||||
|
|
||||||
@objc func addToLastPlaylist(_ sender: NSMenuItem) { |
|
||||||
guard let tableView, tableView.clickedRow >= 0, tableView.clickedRow < tracks.count else { return } |
|
||||||
guard let config = parent.contextMenuConfig else { return } |
|
||||||
let track = tracks[tableView.clickedRow] |
|
||||||
config.onAddToLastPlaylist?(track) |
|
||||||
} |
|
||||||
|
|
||||||
@objc func removeFromPlaylist(_ sender: NSMenuItem) { |
|
||||||
guard let tableView, tableView.clickedRow >= 0, tableView.clickedRow < tracks.count else { return } |
|
||||||
guard let config = parent.contextMenuConfig else { return } |
|
||||||
let track = tracks[tableView.clickedRow] |
|
||||||
config.onRemoveFromPlaylist?(track) |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 4: Build — expect ContentView compile errors (call site not yet updated)** |
|
||||||
|
|
||||||
``` |
|
||||||
xcodebuild build -scheme Music -destination 'platform=macOS' 2>&1 | grep "error:" |
|
||||||
``` |
|
||||||
|
|
||||||
Expected: errors in `ContentView.swift` about removed parameters. `TrackTableView.swift` itself should be clean. |
|
||||||
|
|
||||||
- [ ] **Step 5: Commit** |
|
||||||
|
|
||||||
```bash |
|
||||||
git add Music/Views/TrackTableView.swift |
|
||||||
git commit -m "refactor: replace TrackTableView playlist params with TrackContextMenuConfig" |
|
||||||
``` |
|
||||||
|
|
||||||
--- |
|
||||||
|
|
||||||
### Task 4: Add `contextMenuConfig` to `PlayerControlsView` |
|
||||||
|
|
||||||
**Files:** |
|
||||||
- Modify: `Music/Views/PlayerControlsView.swift:22` (add param after `onNowPlayingTap`) |
|
||||||
- Modify: `Music/Views/PlayerControlsView.swift:112-118` (apply modifier to `nowPlayingSection`) |
|
||||||
|
|
||||||
- [ ] **Step 1: Add the new parameter to `PlayerControlsView`** |
|
||||||
|
|
||||||
In `Music/Views/PlayerControlsView.swift`, find line 22: |
|
||||||
|
|
||||||
```swift |
|
||||||
let onNowPlayingTap: () -> Void |
|
||||||
``` |
|
||||||
|
|
||||||
Add after it: |
|
||||||
|
|
||||||
```swift |
|
||||||
let onNowPlayingTap: () -> Void |
|
||||||
var contextMenuConfig: TrackContextMenuConfig? = nil |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 2: Apply the modifier to `nowPlayingSection`** |
|
||||||
|
|
||||||
In `PlayerControlsView.swift`, find the closing of `nowPlayingSection` (lines ~112–118): |
|
||||||
|
|
||||||
```swift |
|
||||||
.contentShape(Rectangle()) |
|
||||||
.onTapGesture { |
|
||||||
if currentTrack != nil { |
|
||||||
onNowPlayingTap() |
|
||||||
} |
|
||||||
} |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
Replace with: |
|
||||||
|
|
||||||
```swift |
|
||||||
.contentShape(Rectangle()) |
|
||||||
.onTapGesture { |
|
||||||
if currentTrack != nil { |
|
||||||
onNowPlayingTap() |
|
||||||
} |
|
||||||
} |
|
||||||
.trackContextMenu(track: currentTrack, config: contextMenuConfig) |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 3: Build — expect ContentView errors only** |
|
||||||
|
|
||||||
``` |
|
||||||
xcodebuild build -scheme Music -destination 'platform=macOS' 2>&1 | grep "error:" |
|
||||||
``` |
|
||||||
|
|
||||||
Expected: only `ContentView.swift` errors remain (old params still passed there). |
|
||||||
|
|
||||||
- [ ] **Step 4: Commit** |
|
||||||
|
|
||||||
```bash |
|
||||||
git add Music/Views/PlayerControlsView.swift |
|
||||||
git commit -m "feat: add contextMenuConfig param to PlayerControlsView" |
|
||||||
``` |
|
||||||
|
|
||||||
--- |
|
||||||
|
|
||||||
### Task 5: Update `ContentView` — wire both call sites |
|
||||||
|
|
||||||
**Files:** |
|
||||||
- Modify: `Music/ContentView.swift:181-194` (TrackTableView call site) |
|
||||||
- Modify: `Music/ContentView.swift:333-362` (PlayerControlsView call site) |
|
||||||
|
|
||||||
- [ ] **Step 1: Replace the TrackTableView playlist params with `contextMenuConfig`** |
|
||||||
|
|
||||||
In `Music/ContentView.swift`, find lines 181–200 (inside `TrackTableView(...)`): |
|
||||||
|
|
||||||
```swift |
|
||||||
playlists: playlist.playlists, |
|
||||||
lastUsedPlaylistName: playlist.lastUsedPlaylistName, |
|
||||||
selectedPlaylist: playlist.selectedPlaylist, |
|
||||||
onAddToPlaylist: { track, targetPlaylist in |
|
||||||
try? playlist.addTrack(track, to: targetPlaylist) |
|
||||||
}, |
|
||||||
onAddToLastPlaylist: { track in |
|
||||||
try? playlist.addTrackToLastUsedPlaylist(track) |
|
||||||
}, |
|
||||||
onRemoveFromPlaylist: playlist.selectedPlaylist != nil ? { track in |
|
||||||
if let selected = playlist.selectedPlaylist { |
|
||||||
try? playlist.removeTrack(track, from: selected) |
|
||||||
} |
|
||||||
} : nil, |
|
||||||
``` |
|
||||||
|
|
||||||
Replace with: |
|
||||||
|
|
||||||
```swift |
|
||||||
contextMenuConfig: TrackContextMenuConfig( |
|
||||||
playlists: playlist.playlists, |
|
||||||
lastUsedPlaylistName: playlist.lastUsedPlaylistName, |
|
||||||
selectedPlaylist: playlist.selectedPlaylist, |
|
||||||
onAddToPlaylist: { track, targetPlaylist in |
|
||||||
try? playlist.addTrack(track, to: targetPlaylist) |
|
||||||
}, |
|
||||||
onAddToLastPlaylist: { track in |
|
||||||
try? playlist.addTrackToLastUsedPlaylist(track) |
|
||||||
}, |
|
||||||
onRemoveFromPlaylist: playlist.selectedPlaylist != nil ? { track in |
|
||||||
if let selected = playlist.selectedPlaylist { |
|
||||||
try? playlist.removeTrack(track, from: selected) |
|
||||||
} |
|
||||||
} : nil |
|
||||||
), |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 2: Pass `contextMenuConfig` to `PlayerControlsView`** |
|
||||||
|
|
||||||
In `Music/ContentView.swift`, find the `PlayerControlsView(...)` block (lines ~333–362). Add `contextMenuConfig` after `onNowPlayingTap`: |
|
||||||
|
|
||||||
```swift |
|
||||||
onNowPlayingTap: { scrollToPlayingTrigger = UUID() }, |
|
||||||
contextMenuConfig: TrackContextMenuConfig( |
|
||||||
playlists: playlist.playlists, |
|
||||||
lastUsedPlaylistName: playlist.lastUsedPlaylistName, |
|
||||||
selectedPlaylist: playlist.selectedPlaylist, |
|
||||||
onAddToPlaylist: { track, targetPlaylist in |
|
||||||
try? playlist.addTrack(track, to: targetPlaylist) |
|
||||||
}, |
|
||||||
onAddToLastPlaylist: { track in |
|
||||||
try? playlist.addTrackToLastUsedPlaylist(track) |
|
||||||
}, |
|
||||||
onRemoveFromPlaylist: playlist.selectedPlaylist != nil ? { track in |
|
||||||
if let selected = playlist.selectedPlaylist { |
|
||||||
try? playlist.removeTrack(track, from: selected) |
|
||||||
} |
|
||||||
} : nil |
|
||||||
) |
|
||||||
``` |
|
||||||
|
|
||||||
- [ ] **Step 3: Build cleanly** |
|
||||||
|
|
||||||
``` |
|
||||||
xcodebuild build -scheme Music -destination 'platform=macOS' 2>&1 | grep -E "error:|BUILD SUCCEEDED|BUILD FAILED" |
|
||||||
``` |
|
||||||
|
|
||||||
Expected: `BUILD SUCCEEDED` with no errors. |
|
||||||
|
|
||||||
- [ ] **Step 4: Run the full test suite** |
|
||||||
|
|
||||||
``` |
|
||||||
xcodebuild test -scheme Music -destination 'platform=macOS' 2>&1 | grep -E "Test.*passed|Test.*failed|error:|BUILD" |
|
||||||
``` |
|
||||||
|
|
||||||
Expected: all tests pass, including `TrackContextMenuConfigTests`. |
|
||||||
|
|
||||||
- [ ] **Step 5: Manual verification** |
|
||||||
|
|
||||||
Run the app. With at least one playlist created: |
|
||||||
|
|
||||||
1. **Track table:** right-click any track row → confirm the menu shows "Add to [last]" / "Add to Playlist" submenu / "Remove from Playlist" (when in a playlist view). Behaviour must be unchanged. |
|
||||||
2. **Bottom controls:** play a track, then right-click anywhere on the now-playing area (album art + title + artist) → confirm the same menu appears. |
|
||||||
3. **No track playing:** right-click the empty now-playing area → confirm no menu appears (the modifier is a no-op when `currentTrack` is nil). |
|
||||||
|
|
||||||
- [ ] **Step 6: Commit** |
|
||||||
|
|
||||||
```bash |
|
||||||
git add Music/ContentView.swift |
|
||||||
git commit -m "feat: wire TrackContextMenuConfig to bottom controls and track table" |
|
||||||
``` |
|
||||||
File diff suppressed because it is too large
Load Diff
@ -1,276 +0,0 @@ |
|||||||
# Music Streaming — Design Spec |
|
||||||
|
|
||||||
## Overview |
|
||||||
|
|
||||||
Add internet-based music streaming to the Music app. A **host** serves its MP3 library as HLS streams over HTTPS. A **client** downloads the host's database for local browsing and plays audio by streaming from the host. Audio plays on the client, not the host. |
|
||||||
|
|
||||||
This is distinct from the existing remote-mode design (LAN, audio on host). Here the client is an independent player that happens to source its audio and library from a remote host. |
|
||||||
|
|
||||||
## Architecture |
|
||||||
|
|
||||||
``` |
|
||||||
┌──────────────────────────┐ ┌──────────────────────────┐ |
|
||||||
│ HOST (Mac) │ │ CLIENT (Mac/iOS) │ |
|
||||||
│ │ │ │ |
|
||||||
│ Existing App │ │ Existing App │ |
|
||||||
│ ┌────────────────────┐ │ │ ┌────────────────────┐ │ |
|
||||||
│ │ DatabaseService │──┼── GET /db ──► │ Local DB copy │ │ |
|
||||||
│ │ (source of truth) │ │ │ │ (read-only browse) │ │ |
|
||||||
│ └────────────────────┘ │ │ └────────────────────┘ │ |
|
||||||
│ ┌────────────────────┐ │ │ ┌────────────────────┐ │ |
|
||||||
│ │ StreamingServer │ │◄── HLS ─────│ │ AVPlayer │ │ |
|
||||||
│ │ - Hummingbird HTTP │ │ requests │ │ (buffered playback)│ │ |
|
||||||
│ │ - HLS segmenter │ │ │ └────────────────────┘ │ |
|
||||||
│ │ - WebSocket │ │ │ ┌────────────────────┐ │ |
|
||||||
│ └────────────────────┘ │ │ │ WebSocket client │ │ |
|
||||||
│ ┌────────────────────┐ │◄── cmds ────│ │ (RemoteCommand / │ │ |
|
||||||
│ │ Cloudflare Tunnel │ │── events ──►│ │ HostEvent) │ │ |
|
||||||
│ │ (cloudflared) │ │ │ └────────────────────┘ │ |
|
||||||
│ └────────────────────┘ │ │ │ |
|
||||||
└──────────────────────────┘ └──────────────────────────┘ |
|
||||||
│ |
|
||||||
▼ |
|
||||||
https://music.yourdomain.com |
|
||||||
(Cloudflare edge) |
|
||||||
``` |
|
||||||
|
|
||||||
### Key Difference from Remote Mode |
|
||||||
|
|
||||||
In remote mode, the client is a remote control — audio plays on the host. In streaming mode, the client is an independent player — it streams audio from the host and plays it locally. The host doesn't play anything when serving a streaming client. |
|
||||||
|
|
||||||
## MusicShared Swift Package |
|
||||||
|
|
||||||
A local Swift package inside the repo, holding code shared between host and client (and later, an iOS target). |
|
||||||
|
|
||||||
Contents: |
|
||||||
|
|
||||||
- **`RemoteProtocol.swift`** — moved from `Music/Remote/`. Contains `RemoteCommand`, `HostEvent`, `PlaybackStatePayload`, `HandshakeMessage`, `RemoteProtocolVersion`. |
|
||||||
- **`HLSManifestGenerator.swift`** — pure function: given track duration and segment size, produces `.m3u8` playlist text. No I/O. |
|
||||||
- **`APIModels.swift`** — shared DTOs: `AuthResponse` (host info, protocol version), `DBMetadata` (version, checksum for conditional re-download). |
|
||||||
- **`Routes.swift`** — route path constants so host and client stay in sync. |
|
||||||
- **`StreamingConstants.swift`** — segment duration (6s), default port (8420), protocol version. |
|
||||||
|
|
||||||
## HTTP Endpoints |
|
||||||
|
|
||||||
All endpoints require the `Authorization: Bearer <api-key>` header. Served by Hummingbird on the host. |
|
||||||
|
|
||||||
| Method | Path | Purpose | |
|
||||||
|--------|------|---------| |
|
||||||
| `GET` | `/auth` | Validate API key, return host name + protocol version | |
|
||||||
| `GET` | `/db` | Download the full SQLite database file | |
|
||||||
| `GET` | `/tracks/:id/stream.m3u8` | HLS manifest for a track | |
|
||||||
| `GET` | `/tracks/:id/segments/:index.mp3` | Individual MP3 audio segment | |
|
||||||
| `GET` | `/ws` | WebSocket upgrade for real-time command/event channel | |
|
||||||
|
|
||||||
No REST API for browsing — the client downloads the full database and queries it locally using existing `DatabaseService` code. |
|
||||||
|
|
||||||
## HLS Streaming |
|
||||||
|
|
||||||
### On-the-Fly Segmentation |
|
||||||
|
|
||||||
When a client requests a track's manifest: |
|
||||||
|
|
||||||
1. Look up the track's file path in the database. |
|
||||||
2. Read MP3 duration from file metadata (cached after first read). |
|
||||||
3. Generate a `.m3u8` playlist with N segments of 6 seconds each. |
|
||||||
|
|
||||||
When a client requests a segment: |
|
||||||
|
|
||||||
1. Use `AVAssetReader` with a time range (`CMTimeRange`) for the requested segment (e.g., segment 2 = 12s–18s). |
|
||||||
2. `AVAssetReader` handles frame-boundary alignment and VBR files correctly. |
|
||||||
3. Return the extracted audio bytes. |
|
||||||
|
|
||||||
This avoids raw byte slicing, which breaks on VBR files and frame-boundary misalignment. |
|
||||||
|
|
||||||
### Manifest Format |
|
||||||
|
|
||||||
``` |
|
||||||
#EXTM3U |
|
||||||
#EXT-X-VERSION:3 |
|
||||||
#EXT-X-TARGETDURATION:6 |
|
||||||
#EXT-X-MEDIA-SEQUENCE:0 |
|
||||||
#EXTINF:6.0, |
|
||||||
segments/0.mp3 |
|
||||||
#EXTINF:6.0, |
|
||||||
segments/1.mp3 |
|
||||||
#EXTINF:4.2, |
|
||||||
segments/2.mp3 |
|
||||||
#EXT-X-ENDLIST |
|
||||||
``` |
|
||||||
|
|
||||||
### Design Decisions |
|
||||||
|
|
||||||
- **MP3 byte-range slicing** instead of transcoding to AAC. Avoids CPU overhead; `AVPlayer` handles MP3 segments without issues. |
|
||||||
- **6-second segments**: HLS standard. Short enough for responsive seeking, long enough to avoid excessive HTTP requests for a single listener. |
|
||||||
- **No adaptive bitrate**: the source files are fixed-bitrate MP3s. No need for multiple quality renditions. |
|
||||||
|
|
||||||
## Cloudflare Tunnel |
|
||||||
|
|
||||||
The host uses `cloudflared` to expose its local Hummingbird server to the internet. |
|
||||||
|
|
||||||
### Quick Tunnel (Development) |
|
||||||
|
|
||||||
``` |
|
||||||
cloudflared tunnel --url http://localhost:8420 |
|
||||||
``` |
|
||||||
|
|
||||||
- Zero config, no account required. |
|
||||||
- Generates a random `https://xxx-yyy-zzz.trycloudflare.com` URL. |
|
||||||
- URL changes on every restart — must be copied to the client each time. |
|
||||||
|
|
||||||
### Named Tunnel (Recommended for Daily Use) |
|
||||||
|
|
||||||
One-time setup with a Cloudflare account and domain: |
|
||||||
|
|
||||||
``` |
|
||||||
cloudflared tunnel create music |
|
||||||
cloudflared tunnel route dns music music.yourdomain.com |
|
||||||
``` |
|
||||||
|
|
||||||
Then the app launches: |
|
||||||
|
|
||||||
``` |
|
||||||
cloudflared tunnel run --url http://localhost:8420 music |
|
||||||
``` |
|
||||||
|
|
||||||
- Stable URL: `https://music.yourdomain.com`. |
|
||||||
- Configure the client once, never touch it again. |
|
||||||
|
|
||||||
### App Integration |
|
||||||
|
|
||||||
- The host manages the `cloudflared` process as a child process (`Process` in Swift). |
|
||||||
- On host start: launch `cloudflared`, parse the tunnel URL from stdout. |
|
||||||
- On host stop: terminate the `cloudflared` process. |
|
||||||
- The host UI displays the current tunnel URL for the user to share with the client. |
|
||||||
- The app supports both modes: a toggle or setting to choose quick vs named tunnel. |
|
||||||
|
|
||||||
### Prerequisite |
|
||||||
|
|
||||||
`cloudflared` must be installed separately (`brew install cloudflared`). The app checks for its presence on host startup and shows a clear error with install instructions if missing. |
|
||||||
|
|
||||||
## Authentication |
|
||||||
|
|
||||||
Static API key for personal use. |
|
||||||
|
|
||||||
- The host generates a random API key on first setup (or the user sets one manually). |
|
||||||
- The key is stored in the host's Keychain. |
|
||||||
- The client stores the host URL + API key in its Keychain. |
|
||||||
- Every HTTP request and WebSocket upgrade includes `Authorization: Bearer <api-key>`. |
|
||||||
- Invalid keys receive HTTP 401. No retry, no session tokens, no expiry — a static secret over HTTPS is sufficient for single-user. |
|
||||||
|
|
||||||
## Client-Side Playback |
|
||||||
|
|
||||||
### Connection Flow |
|
||||||
|
|
||||||
1. User enters host URL + API key in the connection settings (one-time). |
|
||||||
2. Client calls `GET /auth` to validate credentials and check protocol version. |
|
||||||
3. Client calls `GET /db` to download the SQLite database, saved to `Application Support/Music/streaming_db.sqlite`. |
|
||||||
4. Client opens the DB with `DatabaseService` in read-only mode. |
|
||||||
5. Client establishes WebSocket connection to `/ws`. |
|
||||||
6. App transitions to streaming mode — existing views reload from the downloaded DB. |
|
||||||
|
|
||||||
### Playback |
|
||||||
|
|
||||||
When the user picks a track: |
|
||||||
|
|
||||||
1. Client constructs the HLS URL: `https://<host>/tracks/<id>/stream.m3u8`. |
|
||||||
2. Creates an `AVPlayer` with an `AVURLAsset`, injecting the API key via a custom `AVAssetResourceLoaderDelegate` or URL request headers. |
|
||||||
3. `AVPlayer` fetches the manifest, then segments on demand. Buffering, seeking, and playback are handled natively. |
|
||||||
4. Client sends `RemoteCommand` over WebSocket to keep the host aware of what's playing (for state sync if multiple clients in the future). |
|
||||||
|
|
||||||
### AudioService Abstraction |
|
||||||
|
|
||||||
The existing `AudioService` plays local files. For streaming, a parallel `StreamingAudioService` wraps `AVPlayer` with HLS URLs. Both conform to a shared protocol so `PlayerViewModel` works with either. |
|
||||||
|
|
||||||
### Database Refresh |
|
||||||
|
|
||||||
Same as remote mode: send `RemoteCommand.refreshDB` over WebSocket → host signals `HostEvent.dbReady` → client re-downloads the DB and reloads views. |
|
||||||
|
|
||||||
## WebSocket Channel |
|
||||||
|
|
||||||
Reuses the existing `RemoteCommand` / `HostEvent` protocol (JSON over WebSocket). |
|
||||||
|
|
||||||
### Client → Host |
|
||||||
|
|
||||||
| Command | Purpose | |
|
||||||
|---------|---------| |
|
||||||
| `play(trackId, queueIds)` | Inform host what the client is playing | |
|
||||||
| `pause` | Client paused | |
|
||||||
| `resume` | Client resumed | |
|
||||||
| `next` / `previous` | Client changed track | |
|
||||||
| `seek(position)` | Client seeked | |
|
||||||
| `setVolume(level)` | Client volume changed | |
|
||||||
| `toggleShuffle` | Client toggled shuffle | |
|
||||||
| `refreshDB` | Request fresh database | |
|
||||||
|
|
||||||
In streaming mode, these commands are informational (the client controls its own playback). They keep the host aware of client state for logging and potential future multi-client coordination. |
|
||||||
|
|
||||||
### Host → Client |
|
||||||
|
|
||||||
| Event | Purpose | |
|
||||||
|-------|---------| |
|
||||||
| `dbReady` | New database available for download | |
|
||||||
| `error(message)` | Server-side error (track file missing, etc.) | |
|
||||||
|
|
||||||
`playbackState` events are less critical in streaming mode since the client drives its own playback, but can be used for sync verification. |
|
||||||
|
|
||||||
### Handshake & Keep-Alive |
|
||||||
|
|
||||||
- On WebSocket connect: exchange `HandshakeMessage` with protocol version and app version. |
|
||||||
- Ping/pong every 5 seconds. Connection declared lost after 3 missed pings (15s). |
|
||||||
|
|
||||||
## UI Changes |
|
||||||
|
|
||||||
### Host Mode |
|
||||||
|
|
||||||
- **"Start Streaming Server"** menu toggle — starts Hummingbird + `cloudflared`. |
|
||||||
- Status indicator: "Streaming server running · `https://music.yourdomain.com`". |
|
||||||
- Settings panel: API key display/regenerate, tunnel mode (quick/named), named tunnel config. |
|
||||||
|
|
||||||
### Client Mode |
|
||||||
|
|
||||||
- **Connection settings**: host URL + API key fields, "Connect" / "Disconnect" button. |
|
||||||
- Status indicator: "Connected to [host]" or "Disconnected". |
|
||||||
- "Refresh Library" action to re-download the database. |
|
||||||
- All existing views (HomeView, TrackTableView, playlists, search, player controls) work unchanged against the local DB copy. |
|
||||||
- Playlist creation/editing disabled (read-only snapshot). |
|
||||||
|
|
||||||
### Mode Selection |
|
||||||
|
|
||||||
A setting to choose the app's role: **Local** (default, current behavior), **Host** (serves library), or **Client** (streams from host). Persisted in UserDefaults. |
|
||||||
|
|
||||||
## Testing |
|
||||||
|
|
||||||
### Unit Tests |
|
||||||
|
|
||||||
- `HLSManifestGenerator`: correct `.m3u8` output for various track durations, edge cases (very short tracks, exact multiples of segment duration). |
|
||||||
- `RemoteCommand` / `HostEvent` Codable round-trip (already partially covered in `RemoteProtocolTests.swift`). |
|
||||||
- API key validation logic. |
|
||||||
- Segment extraction: `AVAssetReader` produces valid audio for each segment time range, including edge cases (VBR files, last segment shorter than 6s). |
|
||||||
|
|
||||||
### Integration Tests |
|
||||||
|
|
||||||
- Loopback streaming: start Hummingbird server in-process, request manifest + segments over localhost, verify valid HLS output. |
|
||||||
- Database download: verify downloaded DB matches source schema and row counts. |
|
||||||
- Auth rejection: requests without or with wrong API key receive 401. |
|
||||||
- WebSocket handshake: version mismatch is caught and reported. |
|
||||||
|
|
||||||
### Manual Test Scenarios |
|
||||||
|
|
||||||
- Happy path: start host → connect client → browse library → play track → audio streams and plays on client. |
|
||||||
- Seek mid-track → playback resumes from correct position. |
|
||||||
- Network interruption → client buffers, resumes when connection returns. |
|
||||||
- Kill host mid-playback → client shows error cleanly. |
|
||||||
- Add tracks on host → client refreshes DB → new tracks appear. |
|
||||||
- Wrong API key → client shows auth error. |
|
||||||
- `cloudflared` not installed → host shows clear install instructions. |
|
||||||
|
|
||||||
## Scope & Constraints |
|
||||||
|
|
||||||
- **Single client** for v1. No concurrent listener handling. |
|
||||||
- **Read-only client**: no playlist or library modifications from the client. |
|
||||||
- **MP3 only**: HLS segmentation assumes MP3 source files (matches current library). |
|
||||||
- **`cloudflared` required**: not bundled, must be installed separately. |
|
||||||
- **No offline mode**: client requires active connection to stream. Downloaded DB enables browsing but not playback without the host. |
|
||||||
- **No transcoding**: segments served as raw MP3 byte ranges. |
|
||||||
- **Hummingbird dependency**: added via Swift Package Manager for the embedded HTTP server. |
|
||||||
@ -1,270 +0,0 @@ |
|||||||
# Remote Mode — Design Spec |
|
||||||
|
|
||||||
## Overview |
|
||||||
|
|
||||||
Add a Host/Remote mode to the Music app so a MacBook can control playback on a Mac Mini over the local network. The remote sees the full library and controls playback, but audio plays on the host. The remote is read-only — no playlist or library modifications for v1. |
|
||||||
|
|
||||||
## Architecture |
|
||||||
|
|
||||||
Two roles the app can operate in, one at a time: |
|
||||||
|
|
||||||
- **Host:** Runs a network server, advertises via Bonjour, serves its database, accepts playback commands, streams playback state. |
|
||||||
- **Remote:** Discovers hosts via Bonjour, downloads the host's database, sends playback commands, displays synced playback state. |
|
||||||
|
|
||||||
``` |
|
||||||
┌──────────────────────┐ ┌──────────────────────┐ |
|
||||||
│ MAC MINI │ │ MACBOOK │ |
|
||||||
│ (Host) │ │ (Remote) │ |
|
||||||
│ │ │ │ |
|
||||||
│ Existing App │ │ Existing App │ |
|
||||||
│ ┌────────────────┐ │ │ ┌────────────────┐ │ |
|
||||||
│ │ AudioService │ │◄────────│ │ RemoteClient │ │ |
|
||||||
│ │ PlayerViewModel│ │ WebSocket│ │ │ │ |
|
||||||
│ │ DatabaseService│──┼────────►│ │ Local DB copy │ │ |
|
||||||
│ └────────────────┘ │ HTTP │ └────────────────┘ │ |
|
||||||
│ ┌────────────────┐ │ │ │ |
|
||||||
│ │ HostServer │ │ │ Reuses all existing │ |
|
||||||
│ │ - HTTP (DB) │ │ │ ViewModels & Views │ |
|
||||||
│ │ - WebSocket │ │ │ for browsing │ |
|
||||||
│ │ - Bonjour │ │ │ │ |
|
||||||
│ └────────────────┘ │ │ │ |
|
||||||
└──────────────────────┘ └──────────────────────┘ |
|
||||||
``` |
|
||||||
|
|
||||||
### Playback Abstraction |
|
||||||
|
|
||||||
A `PlaybackController` protocol abstracts where playback happens: |
|
||||||
|
|
||||||
- `LocalPlaybackController` — wraps the existing `AudioService` + `PlayerViewModel` logic. This is what the app uses today. |
|
||||||
- `RemotePlaybackController` — sends commands over WebSocket to the host, receives state updates, and updates the `PlayerViewModel` accordingly. |
|
||||||
|
|
||||||
Views and ViewModels call the same methods (`play`, `pause`, `next`, `seek`, etc.) regardless of which controller is active. The active controller decides whether that's local audio or a network command. |
|
||||||
|
|
||||||
## Host Server |
|
||||||
|
|
||||||
New service: `HostServer`. |
|
||||||
|
|
||||||
### Bonjour Advertisement |
|
||||||
|
|
||||||
- Uses `NWListener` with service type `_musicremote._tcp`. |
|
||||||
- Service name is the computer's local name. |
|
||||||
- Automatically discoverable on the local network when hosting is enabled. |
|
||||||
|
|
||||||
### HTTP — Database Download |
|
||||||
|
|
||||||
- When a remote connects, its first request is `GET /db`. |
|
||||||
- The host reads `db.sqlite` from its Application Support directory and streams it as a binary response. |
|
||||||
- Typically a few MB, under a second on WiFi. |
|
||||||
|
|
||||||
### WebSocket — Command & State Channel |
|
||||||
|
|
||||||
After the DB download, the remote establishes a WebSocket connection for bidirectional communication. |
|
||||||
|
|
||||||
**Remote → Host (commands):** |
|
||||||
|
|
||||||
| Command | Payload | |
|
||||||
|---------|---------| |
|
||||||
| `play` | `trackId`, `queueIds` (array of track IDs) | |
|
||||||
| `pause` | — | |
|
||||||
| `resume` | — | |
|
||||||
| `next` | — | |
|
||||||
| `previous` | — | |
|
||||||
| `seek` | `position` (seconds) | |
|
||||||
| `setVolume` | `level` (0.0–1.0) | |
|
||||||
| `toggleShuffle` | — | |
|
||||||
| `refreshDB` | — | |
|
||||||
|
|
||||||
**Host → Remote (events):** |
|
||||||
|
|
||||||
| Event | Payload | |
|
||||||
|-------|---------| |
|
||||||
| `playbackState` | `trackId`, `isPlaying`, `currentTime`, `duration`, `volume`, `isShuffled` | |
|
||||||
| `dbReady` | — (sent after refreshDB, signals new DB is available for download) | |
|
||||||
| `error` | `message` (human-readable) | |
|
||||||
|
|
||||||
### State Update Frequency |
|
||||||
|
|
||||||
- Immediate on discrete events: play, pause, track change, volume change, shuffle toggle. |
|
||||||
- Every ~1 second while playing for progress bar position. |
|
||||||
- The remote interpolates locally between updates for smooth scrubber movement. |
|
||||||
|
|
||||||
### Connection Limits |
|
||||||
|
|
||||||
Single remote connection at a time for v1. A second connection attempt is rejected with a clear error. |
|
||||||
|
|
||||||
## Remote Client |
|
||||||
|
|
||||||
New service: `RemoteClient`. |
|
||||||
|
|
||||||
### Discovery |
|
||||||
|
|
||||||
- Uses `NWBrowser` to scan for `_musicremote._tcp` services. |
|
||||||
- Presents discovered hosts by computer name in the connection sheet. |
|
||||||
- Resolves the selected endpoint to get IP/port. |
|
||||||
|
|
||||||
### Connection Flow |
|
||||||
|
|
||||||
1. Connect to the host's HTTP endpoint. |
|
||||||
2. Download `db.sqlite`, save to `Application Support/Music/remote_db.sqlite`. |
|
||||||
3. Open the downloaded DB with `DatabaseService` in read-only mode. |
|
||||||
4. Establish the WebSocket connection. |
|
||||||
5. App transitions to remote mode — existing ViewModels reload from the downloaded DB. |
|
||||||
|
|
||||||
### Command Forwarding |
|
||||||
|
|
||||||
In remote mode, the `RemotePlaybackController` intercepts all playback calls and sends them as WebSocket commands instead of calling the local `AudioService`. |
|
||||||
|
|
||||||
### State Sync |
|
||||||
|
|
||||||
The remote listens for `playbackState` messages and updates the `PlayerViewModel`: |
|
||||||
- Current track is looked up by ID from the local DB copy. |
|
||||||
- `isPlaying`, `currentTime`, `duration`, `volume`, `isShuffled` are set directly. |
|
||||||
- SwiftUI observation triggers UI updates automatically. |
|
||||||
|
|
||||||
### DB Refresh |
|
||||||
|
|
||||||
A "Refresh Library" action sends `refreshDB`, the host signals `dbReady`, the remote re-downloads the DB and reloads the ViewModels. |
|
||||||
|
|
||||||
### Disconnection |
|
||||||
|
|
||||||
On disconnect (user-initiated or connection drop), the app returns to local mode. The temporary remote DB file is deleted. |
|
||||||
|
|
||||||
## Message Protocol |
|
||||||
|
|
||||||
JSON over WebSocket. Swift `Codable` enums for type safety. |
|
||||||
|
|
||||||
```json |
|
||||||
// Remote → Host |
|
||||||
{"type": "play", "payload": {"trackId": 42, "queueIds": [42, 43, 44, 45]}} |
|
||||||
{"type": "pause"} |
|
||||||
{"type": "resume"} |
|
||||||
{"type": "next"} |
|
||||||
{"type": "previous"} |
|
||||||
{"type": "seek", "payload": {"position": 65.3}} |
|
||||||
{"type": "setVolume", "payload": {"level": 0.75}} |
|
||||||
{"type": "toggleShuffle"} |
|
||||||
{"type": "refreshDB"} |
|
||||||
|
|
||||||
// Host → Remote |
|
||||||
{"type": "playbackState", "payload": { |
|
||||||
"trackId": 42, |
|
||||||
"isPlaying": true, |
|
||||||
"currentTime": 65.3, |
|
||||||
"duration": 210.0, |
|
||||||
"volume": 0.75, |
|
||||||
"isShuffled": false |
|
||||||
}} |
|
||||||
{"type": "dbReady"} |
|
||||||
{"type": "error", "payload": {"message": "Track file not found"}} |
|
||||||
``` |
|
||||||
|
|
||||||
### Handshake |
|
||||||
|
|
||||||
On WebSocket connect, host and client exchange a handshake message with app version. Version mismatches are caught early and logged. |
|
||||||
|
|
||||||
### Keep-Alive |
|
||||||
|
|
||||||
WebSocket ping/pong at 5-second intervals. If 3 consecutive pings go unanswered, the connection is declared lost. |
|
||||||
|
|
||||||
## UI Changes |
|
||||||
|
|
||||||
### Menu Bar |
|
||||||
|
|
||||||
- **"Enable Host Mode"** — toggle menu item. Starts/stops the `HostServer`. |
|
||||||
- **"Connect to Remote..."** — opens the connection sheet. |
|
||||||
|
|
||||||
### Connection Sheet (Remote Side) |
|
||||||
|
|
||||||
Modal sheet showing: |
|
||||||
- List of discovered Bonjour hosts (computer name + connectivity indicator). |
|
||||||
- "Connect" button for the selected host. |
|
||||||
- Progress indicator during DB download. |
|
||||||
- Error state with retry if connection fails. |
|
||||||
|
|
||||||
### Remote Mode Indicators |
|
||||||
|
|
||||||
When connected as a remote: |
|
||||||
- A persistent banner/badge showing "Connected to [host name]" with a disconnect button. |
|
||||||
- Playlist creation/editing UI disabled (greyed out context menus, hidden "New Playlist"). |
|
||||||
- "Open Music Folder..." menu item disabled. |
|
||||||
- "Refresh Library" action available (triggers DB re-download). |
|
||||||
|
|
||||||
### Host Mode Indicators |
|
||||||
|
|
||||||
When hosting: |
|
||||||
- Status indicator showing "Hosting" or "Hosting · [remote name] connected". |
|
||||||
|
|
||||||
### Unchanged |
|
||||||
|
|
||||||
Track table, player controls, search bar, home view, playlist bar — all work as-is against the local DB copy and the `PlaybackController` abstraction. |
|
||||||
|
|
||||||
## Observability |
|
||||||
|
|
||||||
### Structured Logging |
|
||||||
|
|
||||||
`os.Logger` with subsystem `com.music.remote` and two categories: `host` and `client`. Logs are filterable in Console.app. |
|
||||||
|
|
||||||
| Level | Examples | |
|
||||||
|-------|---------| |
|
||||||
| Info | "Host started on port 8432", "Remote connected: Laurent's MacBook", "DB download complete (2.4 MB, 340ms)" | |
|
||||||
| Debug | Command send/receive, state update cycle, Bonjour browse events, connection lifecycle transitions | |
|
||||||
| Error | Connection refused, DB read failure, WebSocket decode failure, unexpected disconnect with reason | |
|
||||||
|
|
||||||
### Connection State Machine |
|
||||||
|
|
||||||
Every state transition is logged and drives user-visible status: |
|
||||||
|
|
||||||
``` |
|
||||||
Disconnected → Discovering → Found Host → Downloading DB → Connecting WebSocket → Connected |
|
||||||
↑ │ |
|
||||||
└──── Connection Lost ◄───────────────────────────────────────────────────────────┘ |
|
||||||
``` |
|
||||||
|
|
||||||
User-visible status messages: "Searching for hosts...", "Connecting to [name]...", "Downloading library...", "Connected to [name]", "Connection lost — Reconnect?" |
|
||||||
|
|
||||||
### Error Messages |
|
||||||
|
|
||||||
Every error includes: |
|
||||||
- A clean, human-readable summary for the user (shown in UI). |
|
||||||
- The underlying `NWError` description in the log for debugging. |
|
||||||
|
|
||||||
Examples: "Host refused connection", "Download timed out after 10s", "Network changed", "Host stopped hosting". |
|
||||||
|
|
||||||
### Diagnostics |
|
||||||
|
|
||||||
- Version handshake on connect catches protocol mismatches early. |
|
||||||
- WebSocket keep-alive detects stale connections within 15 seconds. |
|
||||||
- All incoming commands logged at debug level on the host for traceability. |
|
||||||
|
|
||||||
## Testing |
|
||||||
|
|
||||||
### Unit Tests |
|
||||||
|
|
||||||
- `RemoteCommand` / `HostEvent` Codable round-trip for every message type. |
|
||||||
- `RemotePlaybackController` sends correct WebSocket messages for each action. |
|
||||||
- Connection state machine: valid transitions succeed, invalid transitions are rejected. |
|
||||||
- `HostServer` command dispatch: incoming commands map to correct `PlayerViewModel` calls. |
|
||||||
|
|
||||||
### Integration Tests |
|
||||||
|
|
||||||
- Loopback connection: `HostServer` + `RemoteClient` in the same process over localhost — full flow from DB download through command/response round-trip. |
|
||||||
- DB download integrity: downloaded DB matches source schema and row counts. |
|
||||||
- State sync accuracy: play a track on host, verify remote receives correct `playbackState` values. |
|
||||||
|
|
||||||
Real network connections in integration tests — no mocks for the networking layer. |
|
||||||
|
|
||||||
### Manual Test Scenarios |
|
||||||
|
|
||||||
- Happy path: enable host → connect remote → browse → play → verify audio on host, UI synced on remote. |
|
||||||
- Kill host app mid-playback → remote shows "Connection lost" cleanly. |
|
||||||
- Disconnect WiFi on remote → reconnect flow works. |
|
||||||
- Scan new folder on host while remote connected → remote can refresh and see new tracks. |
|
||||||
- Attempt playlist creation on remote → properly disabled. |
|
||||||
|
|
||||||
## Scope & Constraints |
|
||||||
|
|
||||||
- **v1 only:** Single remote, read-only, no authentication, local network only. |
|
||||||
- **No changes to existing playback logic:** The `HostServer` wraps `PlayerViewModel` and `AudioService`, it doesn't modify them. |
|
||||||
- **No dependencies added:** All networking uses Apple's Network.framework. |
|
||||||
- **Existing UI untouched:** Only additions are menu items, connection sheet, and status indicators. |
|
||||||
- **Play counts track on the host:** Since the host is playing the audio, play count increments happen on the host's database. The remote's local DB copy is a read-only snapshot and is not written to. |
|
||||||
@ -1,110 +0,0 @@ |
|||||||
# Add "New Playlist…" to the Add-to-Playlist menu |
|
||||||
|
|
||||||
**Date:** 2026-05-30 |
|
||||||
**Status:** Approved design |
|
||||||
|
|
||||||
## Goal |
|
||||||
|
|
||||||
In a track's right-click "Add to Playlist" submenu, let the user create a brand-new |
|
||||||
regular playlist on the fly: pick **New Playlist…**, enter a name, and on save the |
|
||||||
playlist is created and the track is added to it. |
|
||||||
|
|
||||||
## Background |
|
||||||
|
|
||||||
The "Add to Playlist" submenu lives in `TrackContextMenuModifier.swift` and is driven by |
|
||||||
the data-only `TrackContextMenuConfig` struct (a `playlists` array plus action closures), |
|
||||||
built in `ContentView.trackContextMenuConfig` (`ContentView.swift:415`). |
|
||||||
|
|
||||||
The app already creates playlists from the sidebar via an `.alert` + `TextField` |
|
||||||
(`ContentView.swift:273`), calling `PlaylistViewModel.createPlaylist(name:)` → |
|
||||||
`DatabaseService.createPlaylist(name:) -> Playlist`. Adding a track is |
|
||||||
`PlaylistViewModel.addTrack(_:to:)`, which also records the playlist as last-used. |
|
||||||
|
|
||||||
Regular and smart playlists are separate tables; this feature only creates **regular** |
|
||||||
playlists (`Playlist`). |
|
||||||
|
|
||||||
## Approach |
|
||||||
|
|
||||||
Route the name prompt up to `ContentView`, which already owns the new-playlist alert. |
|
||||||
|
|
||||||
SwiftUI alerts do not present reliably when attached *inside* a context menu's content |
|
||||||
(the menu dismisses, and the per-row modifier is re-instantiated). So the menu item only |
|
||||||
signals intent — "create a new playlist for this track" — and `ContentView` owns the |
|
||||||
prompt and the orchestration. This reuses the existing alert pattern rather than |
|
||||||
duplicating it inside the modifier (the rejected alternative, which would also need the |
|
||||||
modifier to reach into `PlaylistViewModel`). |
|
||||||
|
|
||||||
## Behavior decisions |
|
||||||
|
|
||||||
- Adds **only the clicked track** (matches the current single-track "Add to Playlist"). |
|
||||||
- **No navigation** — after create+add the sidebar selection is unchanged. |
|
||||||
- Empty / whitespace-only name → no-op (matches the existing create flow). |
|
||||||
- When the user has **no** existing playlists, the submenu still appears showing just |
|
||||||
"New Playlist…" (today the whole submenu is hidden when `playlists` is empty). |
|
||||||
- Remote mode → wired the same way as the existing "Add to Playlist" action |
|
||||||
(unconditionally), keeping parity with current behavior. |
|
||||||
|
|
||||||
## Changes |
|
||||||
|
|
||||||
### 1. `PlaylistViewModel` (`Music/ViewModels/PlaylistViewModel.swift`) |
|
||||||
|
|
||||||
Add one orchestration method — the unit under test: |
|
||||||
|
|
||||||
```swift |
|
||||||
@discardableResult |
|
||||||
func createPlaylistAndAddTrack(name: String, track: Track) throws -> Playlist { |
|
||||||
let playlist = try db.createPlaylist(name: name) |
|
||||||
try addTrack(track, to: playlist) |
|
||||||
return playlist |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
`db.createPlaylist` returns a `Playlist` with its assigned `id`; `addTrack` adds the track |
|
||||||
and sets `lastUsedPlaylistId` to the new playlist (so the "Add to <last>" item updates too). |
|
||||||
|
|
||||||
### 2. `TrackContextMenuConfig` (`Music/Models/TrackContextMenuConfig.swift`) |
|
||||||
|
|
||||||
Add a new optional closure, defaulting to `nil` (so all existing call sites and tests |
|
||||||
compile unchanged): |
|
||||||
|
|
||||||
```swift |
|
||||||
let onAddToNewPlaylist: ((Track) -> Void)? |
|
||||||
``` |
|
||||||
|
|
||||||
Add it to the explicit `init` with a `= nil` default, alongside the other optionals. |
|
||||||
|
|
||||||
### 3. `TrackContextMenuModifier` (`Music/Views/TrackContextMenuModifier.swift`) |
|
||||||
|
|
||||||
Inside the "Add to Playlist" submenu: |
|
||||||
|
|
||||||
- Add a **New Playlist…** button at the top (ellipsis = opens a prompt) when |
|
||||||
`onAddToNewPlaylist != nil`, followed by a `Divider` before the list of existing |
|
||||||
playlists. |
|
||||||
- Relax the visibility guard so the submenu shows when |
|
||||||
`!config.playlists.isEmpty || config.onAddToNewPlaylist != nil` (previously hidden |
|
||||||
whenever `playlists` was empty). |
|
||||||
|
|
||||||
### 4. `ContentView` (`Music/ContentView.swift`) |
|
||||||
|
|
||||||
- Add state: `@State private var newPlaylistTrack: Track?` and a name field |
|
||||||
(reuse/parallel the existing `playlistNameInput` style; a dedicated field is fine). |
|
||||||
- In `trackContextMenuConfig`, wire `onAddToNewPlaylist: { track in newPlaylistTrack = track }`. |
|
||||||
- Present an alert (mirroring the existing New Playlist alert) gated on |
|
||||||
`newPlaylistTrack != nil`. **Create** trims whitespace, and if non-empty calls |
|
||||||
`playlist.createPlaylistAndAddTrack(name:track:)` with the pending track; **Cancel** and |
|
||||||
completion both clear `newPlaylistTrack` and the name field. |
|
||||||
|
|
||||||
## Testing (TDD) |
|
||||||
|
|
||||||
Unit-test `PlaylistViewModel.createPlaylistAndAddTrack(name:track:)` against an in-memory |
|
||||||
database: |
|
||||||
|
|
||||||
1. Seed a track in the DB. |
|
||||||
2. Call `createPlaylistAndAddTrack(name:track:)`. |
|
||||||
3. Assert a regular playlist with that name now exists. |
|
||||||
4. Assert the playlist's tracks contain the seeded track. |
|
||||||
5. Assert `lastUsedPlaylistId` equals the new playlist's id. |
|
||||||
|
|
||||||
The SwiftUI menu/alert rendering is not unit-tested, consistent with the rest of the |
|
||||||
codebase. `TrackContextMenuConfig`'s new optional defaults to `nil`, so existing |
|
||||||
`TrackContextMenuConfigTests` are unaffected. |
|
||||||
@ -1,166 +0,0 @@ |
|||||||
# Fix `bitrate = 0` Tracks — Design |
|
||||||
|
|
||||||
**Date:** 2026-05-30 |
|
||||||
**Status:** Approved (design) |
|
||||||
|
|
||||||
## Problem |
|
||||||
|
|
||||||
Many tracks in the library have a stored `bitrate` of `0`. Bitrate of `0` is |
|
||||||
displayed literally and is meaningless. Users want these tracks to show their |
|
||||||
real bitrate, and want new imports to stop producing the problem. |
|
||||||
|
|
||||||
## Root Cause |
|
||||||
|
|
||||||
`ScannerService.extractMetadata()` extracts bitrate via AVFoundation: |
|
||||||
|
|
||||||
```swift |
|
||||||
// Music/Services/ScannerService.swift (~line 151) |
|
||||||
let estimatedRate = try await audioTrack.load(.estimatedDataRate) |
|
||||||
bitrate = Int(estimatedRate / 1000) // bits/sec -> kbps |
|
||||||
``` |
|
||||||
|
|
||||||
For some files (observed: long / VBR MP3s such as 2-hour DJ "Essential Mix" |
|
||||||
recordings), `AVAsset.estimatedDataRate` returns `0`. `Int(0 / 1000)` is `0`, so |
|
||||||
a literal `0` is written to the DB. The `duration` field is extracted correctly |
|
||||||
for the same files, which is what makes recovery reliable. |
|
||||||
|
|
||||||
Tracks with `bitrate IS NULL` exist as well — the importer never produced any |
|
||||||
value. These display as "—" and are treated as equally "missing" for this work. |
|
||||||
|
|
||||||
### Evidence |
|
||||||
|
|
||||||
Validated against a real `bitrate = 0` row (a ~120 min MP3): |
|
||||||
|
|
||||||
| Method | Result | |
|
||||||
|------------------------------------------|---------------| |
|
||||||
| `fileSize × 8 ÷ duration ÷ 1000` | 256.0 kbps | |
|
||||||
| `ffprobe -show_entries format=bit_rate` | 256005 → 256.0 kbps | |
|
||||||
|
|
||||||
The two agree to the kbps. For VBR, both methods yield the true **average** |
|
||||||
bitrate, which is the meaningful value. |
|
||||||
|
|
||||||
## Scope |
|
||||||
|
|
||||||
Both halves of the problem: |
|
||||||
|
|
||||||
1. **Backfill** existing rows where `bitrate = 0 OR bitrate IS NULL`. |
|
||||||
2. **Fix the importer** so future imports never store `0` again. |
|
||||||
|
|
||||||
No mass rescan is required: the script repairs today's rows; the importer fix |
|
||||||
only needs to guarantee correctness for new imports. |
|
||||||
|
|
||||||
## Component 1 — Importer Fix (`ScannerService`) |
|
||||||
|
|
||||||
Extract the bitrate decision into a pure, unit-testable function: |
|
||||||
|
|
||||||
```swift |
|
||||||
/// Resolve a track's bitrate (kbps) from the OS estimate, with a |
|
||||||
/// file-size/duration fallback. Returns nil when no value can be derived — |
|
||||||
/// never 0. |
|
||||||
static func resolveBitrate(estimatedDataRate: Double, |
|
||||||
fileSizeBytes: Int64?, |
|
||||||
durationSeconds: Double?) -> Int? |
|
||||||
``` |
|
||||||
|
|
||||||
Logic: |
|
||||||
|
|
||||||
- `estimatedDataRate > 0` |
|
||||||
→ `Int((estimatedDataRate / 1000).rounded())` (current behaviour, now rounded |
|
||||||
rather than truncated). |
|
||||||
- else if `fileSizeBytes != nil` **and** `durationSeconds != nil && > 0` |
|
||||||
→ `Int((Double(fileSizeBytes) * 8 / durationSeconds / 1000).rounded())`. |
|
||||||
- else → `nil`. |
|
||||||
|
|
||||||
**Key invariant: the importer never stores `0`.** When nothing can be derived it |
|
||||||
stores `nil`, which the UI already renders as "—". |
|
||||||
|
|
||||||
`extractMetadata()` is updated to: |
|
||||||
|
|
||||||
- read the file size it can already obtain via `FileManager.attributesOfItem(atPath:)`, |
|
||||||
- pass `estimatedDataRate`, file size, and the loaded `duration` into |
|
||||||
`resolveBitrate`, |
|
||||||
- assign the result to `bitrate`. |
|
||||||
|
|
||||||
This keeps the AVFoundation I/O in `extractMetadata` and the arithmetic in a pure |
|
||||||
function that tests can drive directly. |
|
||||||
|
|
||||||
## Component 2 — Backfill Script (`scripts/backfill_bitrate.py`) |
|
||||||
|
|
||||||
Mirrors the conventions of the existing `scripts/backfill_itunes_dates.py`: |
|
||||||
|
|
||||||
- Same `DEFAULT_DB` resolution (`~/Library/Containers/com.staxriver.mu/Data/ |
|
||||||
Library/Application Support/Music/db.sqlite`), with `--db` override. |
|
||||||
- Reuses the `norm_path` / percent-decoding approach to turn a stored `fileURL` |
|
||||||
into a POSIX path. |
|
||||||
- **Dry-run by default**; `--apply` writes after a timestamped backup of the DB. |
|
||||||
- `--self-test` runs offline unit checks and exits. |
|
||||||
- Stdlib only (`sqlite3`, `subprocess`, `os`, `urllib`, …), plus `ffprobe` as an |
|
||||||
optional external tool. |
|
||||||
|
|
||||||
### Selection |
|
||||||
|
|
||||||
```sql |
|
||||||
SELECT id, fileURL, duration, bitrate |
|
||||||
FROM tracks |
|
||||||
WHERE bitrate = 0 OR bitrate IS NULL; |
|
||||||
``` |
|
||||||
|
|
||||||
### Per-row bitrate determination |
|
||||||
|
|
||||||
1. Resolve `fileURL` → POSIX path. If the file does not exist on disk → |
|
||||||
report as **skipped (missing file)**, do not update. |
|
||||||
2. Run `ffprobe -v error -show_entries format=bit_rate -of default=nw=1:nk=1 <path>`. |
|
||||||
- Parse an integer bps → `round(bps / 1000)` kbps. |
|
||||||
3. If ffprobe is **absent**, **errors**, or returns **`N/A`/empty**, fall back to |
|
||||||
the formula: `round(fileSizeBytes * 8 / durationSeconds / 1000)`. |
|
||||||
- If there is also no usable duration → report as |
|
||||||
**skipped (undeterminable)**, do not update. |
|
||||||
|
|
||||||
### Output |
|
||||||
|
|
||||||
- **Dry-run:** a table of `path · old → new` plus a summary. |
|
||||||
- **`--apply`:** create `db.sqlite.bak-<timestamp>`, then `UPDATE tracks SET |
|
||||||
bitrate = ? WHERE id = ?` per resolved row, in a single transaction. |
|
||||||
- **Summary (both modes):** counts for updated, skipped-missing-file, |
|
||||||
skipped-undeterminable, and whether ffprobe was available. |
|
||||||
|
|
||||||
## Testing (TDD — tests written before implementation) |
|
||||||
|
|
||||||
### Swift (`MusicTests`) |
|
||||||
|
|
||||||
Unit tests for `ScannerService.resolveBitrate`: |
|
||||||
|
|
||||||
1. Positive `estimatedDataRate` → rounded kbps (e.g. `320450.0` → `320`). |
|
||||||
2. `estimatedDataRate == 0` with valid size + duration → formula result |
|
||||||
(e.g. 230_358_479 bytes, 7198.54 s → `256`). |
|
||||||
3. `estimatedDataRate == 0`, valid size, **no/zero duration** → `nil`. |
|
||||||
4. `estimatedDataRate == 0`, **no file size** → `nil`. |
|
||||||
5. Confirms the function never returns `0`. |
|
||||||
|
|
||||||
Each test carries a step-by-step comment describing what it exercises. |
|
||||||
|
|
||||||
### Python (`--self-test`) |
|
||||||
|
|
||||||
1. ffprobe-output parsing: `"256005\n"` → `256`. |
|
||||||
2. `N/A`/empty ffprobe output → triggers formula fallback. |
|
||||||
3. Formula math: `(230_358_479 * 8 / 7198.54 / 1000)` rounds to `256`. |
|
||||||
4. `norm_path` edge cases (percent-encoding, `file://localhost/`, NFC, trailing |
|
||||||
slash) — mirrored from the existing script's expectations. |
|
||||||
|
|
||||||
### Manual verification |
|
||||||
|
|
||||||
Run the script in dry-run against the real library DB and eyeball a sample |
|
||||||
(ffprobe vs formula agreement) before `--apply`. |
|
||||||
|
|
||||||
## Operational Notes |
|
||||||
|
|
||||||
- `--apply` writes directly to the SQLite file. **Quit the app first** to avoid |
|
||||||
WAL/lock contention — same caveat as `backfill_itunes_dates.py`. |
|
||||||
- A timestamped backup is created before any write; restore by copying it back. |
|
||||||
|
|
||||||
## Out of Scope |
|
||||||
|
|
||||||
- No new UI (no in-app "repair bitrates" command); the script covers existing |
|
||||||
rows and the importer covers future ones. |
|
||||||
- No change to how bitrate is displayed. |
|
||||||
- No re-encoding or modification of audio files — read-only analysis only. |
|
||||||
@ -1,97 +0,0 @@ |
|||||||
# iTunes/Music → app DB date & stats backfill (one-time) |
|
||||||
|
|
||||||
Date: 2026-05-30 |
|
||||||
|
|
||||||
## Problem |
|
||||||
|
|
||||||
`ScannerService.extractMetadata` sets `dateAdded: Date()` at scan time |
|
||||||
(`Music/Services/ScannerService.swift:186`), so every track's "added date" in the |
|
||||||
app DB is really its *scan* date, not the date the user originally added it in |
|
||||||
Apple Music. The user wants the **true `Date Added`** (and, since Music.app tracks |
|
||||||
them too, `Play Count`, `Rating`, and last-played) copied from their Apple Music |
|
||||||
library into the app's SQLite database. |
|
||||||
|
|
||||||
## Context (verified) |
|
||||||
|
|
||||||
- The app is **sandboxed**. Current `PRODUCT_BUNDLE_IDENTIFIER` is `com.staxriver.mu` |
|
||||||
(HEAD and working tree; not part of the uncommitted diff). The live DB is therefore at: |
|
||||||
`~/Library/Containers/com.staxriver.mu/Data/Library/Application Support/Music/db.sqlite` |
|
||||||
- **The real app and real library are on a different computer.** This machine only has a |
|
||||||
3-track dev DB. The script must be portable and is intended to run on the other Mac. |
|
||||||
- The user confirmed the audio files are the **same files** Apple Music references (they |
|
||||||
live inside Apple Music's media folder, e.g. |
|
||||||
`~/Music/Music/Media.localized/Music/...`). So the join key is the **file path**. |
|
||||||
- `tracks.dateAdded` is a GRDB `.datetime` column, stored as the string |
|
||||||
`YYYY-MM-DD HH:MM:SS.SSS` in UTC (confirmed from existing rows, e.g. |
|
||||||
`2026-05-24 06:46:01.713`). GRDB is lenient on read, so `....000` round-trips. |
|
||||||
- App rating scale is **0–5 stars** (`TrackTableView.swift:284` renders |
|
||||||
`String(repeating: "★", count: track.rating)`). Music.app stores 0–100, so map `// 20`. |
|
||||||
|
|
||||||
## Approach |
|
||||||
|
|
||||||
A single **stdlib-only Python 3 script**, run once. Source of truth is the Music.app |
|
||||||
**File ▸ Library ▸ Export Library…** XML plist (chosen over live AppleScript: no |
|
||||||
Automation prompt, no timeouts, trivially parseable with `plistlib`). On matched tracks |
|
||||||
it does a **blunt overwrite** of all four fields, with Music.app as the source of truth. |
|
||||||
|
|
||||||
## Matching (the bug-prone part) |
|
||||||
|
|
||||||
Join Music.app `Location` to `tracks.fileURL` on a **normalized decoded POSIX path**, |
|
||||||
not raw URL strings. `norm_path()`: |
|
||||||
|
|
||||||
1. strip leading `file://`, then optional `localhost` host segment, |
|
||||||
2. percent-decode (`urllib.parse.unquote`), |
|
||||||
3. `unicodedata.normalize("NFC", …)` — neutralizes the accented-filename NFC/NFD mismatch |
|
||||||
between APFS storage and the two URL string sources, |
|
||||||
4. strip a trailing slash. |
|
||||||
|
|
||||||
Music tracks with no `Location` (Apple Music streaming entries) are skipped. |
|
||||||
|
|
||||||
## Field mapping (matched rows only; blunt overwrite) |
|
||||||
|
|
||||||
| Column | XML key | Rule | |
|
||||||
|----------------|------------------|-------------------------------------------------------------| |
|
||||||
| `dateAdded` | `Date Added` | `%Y-%m-%d %H:%M:%S.000` UTC. If absent, keep existing (col is NOT NULL). | |
|
||||||
| `playCount` | `Play Count` | integer, `0` if absent. | |
|
||||||
| `rating` | `Rating` (0–100) | `// 20` → 0–5, `0` if absent. | |
|
||||||
| `lastPlayedAt` | `Play Date UTC` | same date format, or `NULL` if absent. | |
|
||||||
|
|
||||||
## Safety |
|
||||||
|
|
||||||
- **Dry-run by default**: prints match rate, a sample of before→after changes, and the |
|
||||||
counts + samples of unmatched-in-DB and unmatched-in-XML. Writes nothing. |
|
||||||
- `--apply`: first copies `db.sqlite` + `-wal` + `-shm` to a timestamped backup, then |
|
||||||
performs all writes in a single transaction, then `PRAGMA wal_checkpoint(TRUNCATE)`. |
|
||||||
Reversible by restoring the backup. |
|
||||||
- The app must be **quit** before running so the sandbox DB isn't mid-write. |
|
||||||
|
|
||||||
## CLI |
|
||||||
|
|
||||||
``` |
|
||||||
python3 scripts/backfill_itunes_dates.py --xml <Library.xml> [--db <path>] [--apply] [--self-test] |
|
||||||
``` |
|
||||||
|
|
||||||
- Default `--db`: `~/Library/Containers/com.staxriver.mu/Data/Library/Application Support/Music/db.sqlite` |
|
||||||
computed from `$HOME`, so it resolves on the other Mac too. |
|
||||||
|
|
||||||
## Testing |
|
||||||
|
|
||||||
`scripts/test_backfill_itunes_dates.py` (stdlib `unittest`): |
|
||||||
|
|
||||||
- `norm_path`: NFC/NFD equivalence, `file://localhost/` form, percent-encoding, |
|
||||||
filenames with spaces/`#`/parentheses/apostrophes. |
|
||||||
- `build_updates`: date formatting, rating `// 20`, playCount & lastPlayed present/absent, |
|
||||||
unmatched-row handling. |
|
||||||
- Integration: a temp SQLite DB with the real `tracks` schema seeded with the user's actual |
|
||||||
3 track paths + scan dates and a synthetic Library.xml → `apply` → assert rows updated. |
|
||||||
|
|
||||||
## Delivery |
|
||||||
|
|
||||||
Script + test live in the repo under `scripts/`. The user commits (via `/commit`), pushes, |
|
||||||
pulls on the real machine, runs **File ▸ Library ▸ Export Library…** there, then runs the |
|
||||||
dry-run, eyeballs the match rate, and re-runs with `--apply`. |
|
||||||
|
|
||||||
## Out of scope |
|
||||||
|
|
||||||
Scanning the library into the app (the user does that in-app first), ongoing/automatic sync, |
|
||||||
and non-file (streaming) tracks. |
|
||||||
@ -1,243 +0,0 @@ |
|||||||
# Playing Queue — Design |
|
||||||
|
|
||||||
**Date:** 2026-05-30 |
|
||||||
**Status:** Approved (pending spec review) |
|
||||||
|
|
||||||
## Overview |
|
||||||
|
|
||||||
Add a Spotify-style **priority "Up Next" queue** to the Music app. Users can push |
|
||||||
tracks to the front ("Play Next") or end ("Add to Queue") of a manual queue via the |
|
||||||
track context menu. The manual queue plays before the current playback context |
|
||||||
(playlist/album) resumes, survives starting a new context, and is visible and |
|
||||||
editable in a right-docked "Up Next" panel. |
|
||||||
|
|
||||||
## Goals |
|
||||||
|
|
||||||
- "Play Next" and "Add to Queue" actions on the track context menu. |
|
||||||
- A manual queue that takes priority over the playback context and persists when a |
|
||||||
new context (playlist/album/library) starts playing. |
|
||||||
- A visible, right-docked "Up Next" panel showing the manual queue and the upcoming |
|
||||||
context tracks. |
|
||||||
- Drag-to-reorder and remove **within the manual queue**. |
|
||||||
|
|
||||||
## Non-Goals (v1) |
|
||||||
|
|
||||||
- **Remote/streaming support.** When this app is driving a remote device, queue |
|
||||||
actions are hidden and `next()` continues to delegate over the wire. No |
|
||||||
`RemoteProtocol` changes. (Possible follow-up.) |
|
||||||
- **Persistence across app restart.** The queue is in-memory in `PlayerViewModel`. |
|
||||||
- **Reordering the context** from the panel (that is playlist editing, already |
|
||||||
handled elsewhere). The "Next from" section is read-only. |
|
||||||
- **Clear-all-queue** action (not requested). |
|
||||||
- **Multi-select queueing.** Actions operate on the single right-clicked track, |
|
||||||
consistent with the existing "Add to Playlist". |
|
||||||
|
|
||||||
## Resolved Decisions |
|
||||||
|
|
||||||
| Decision | Choice | Reason | |
|
||||||
|---|---|---| |
|
||||||
| Queue model | Spotify-style priority queue (manual queue distinct from context) | User selection | |
|
||||||
| Panel placement | Right-docked slide-out, toggled from transport bar | User selection (mockup A) | |
|
||||||
| Remote scope | Local-only for v1; hide actions in remote mode | User selection | |
|
||||||
| Persistence | In-memory only | User selection | |
|
||||||
| `queue`/`currentIndex` naming | Keep as the **context**; add doc comments | Backward-compatible; avoids touching remote-sync + existing tests | |
|
||||||
| "Next from" section | Read-only, double-click to jump | Context reordering is out of scope | |
|
||||||
|
|
||||||
## Data Model (`PlayerViewModel`) |
|
||||||
|
|
||||||
Existing `queue` / `originalQueue` / `currentIndex` are **retained and keep their |
|
||||||
current meaning: the playback CONTEXT** (playlist/album, with shuffle applied). New |
|
||||||
state is added alongside: |
|
||||||
|
|
||||||
```swift |
|
||||||
private(set) var queue: [Track] // UNCHANGED — the context (shuffled view) |
|
||||||
private var originalQueue: [Track] // UNCHANGED — context, original order |
|
||||||
var currentIndex: Int? // UNCHANGED — index into `queue` (the CONTEXT |
|
||||||
// position), held even while a manual track plays |
|
||||||
private(set) var manualQueue: [QueueEntry] = [] // NEW — priority "Up Next" entries |
|
||||||
private(set) var contextName: String? // NEW — label for "Next from: <name>" |
|
||||||
``` |
|
||||||
|
|
||||||
Each manual entry carries its own identity so the same track can be queued twice |
|
||||||
without SwiftUI confusing the rows (the codebase has prior id-collision bugs): |
|
||||||
|
|
||||||
```swift |
|
||||||
nonisolated struct QueueEntry: Identifiable { |
|
||||||
let id = UUID() |
|
||||||
let track: Track |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
A dedicated `playManual(_:)` path plays a queued track **without** touching |
|
||||||
`currentIndex`, so the context position is preserved automatically — no extra "am I |
|
||||||
playing from the queue?" flag is needed. |
|
||||||
|
|
||||||
Computed for the panel: |
|
||||||
|
|
||||||
```swift |
|
||||||
// Context tracks after the current context position; the "Next from" section. |
|
||||||
var upcomingContext: [Track] { |
|
||||||
guard let idx = currentIndex, idx + 1 < queue.count else { return [] } |
|
||||||
return Array(queue[(idx + 1)...]) |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
`currentIndex` deliberately tracks the **context** position, not "what is playing." |
|
||||||
While a manual-queue track plays, `currentIndex` stays put so the context resumes at |
|
||||||
the correct spot once the manual queue drains. |
|
||||||
|
|
||||||
## Behavior |
|
||||||
|
|
||||||
### Adding to the queue |
|
||||||
|
|
||||||
```swift |
|
||||||
func playNext(_ track: Track) // insert at front of manualQueue |
|
||||||
func addToQueue(_ track: Track) // append to end of manualQueue |
|
||||||
``` |
|
||||||
|
|
||||||
Both: if **nothing is currently playing** (`currentTrack == nil`), immediately pop |
|
||||||
and play the just-queued track instead of leaving it parked (queue-while-idle starts |
|
||||||
playback). |
|
||||||
|
|
||||||
### Advancing |
|
||||||
|
|
||||||
```swift |
|
||||||
func next() { |
|
||||||
if remoteProvider != nil { remote.sendNext(); return } // UNCHANGED remote path |
|
||||||
if !manualQueue.isEmpty { |
|
||||||
let entry = manualQueue.removeFirst() // consume-on-play |
|
||||||
playManual(entry.track) // currentIndex unchanged |
|
||||||
} else { |
|
||||||
// existing context-advance logic (currentIndex + 1, stop at end) |
|
||||||
} |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
- **Consume-on-play:** `removeFirst()` removes the track from "Up Next" the instant |
|
||||||
it starts. The Queue section only ever shows not-yet-played tracks. |
|
||||||
- **Resume point:** when `manualQueue` empties, `next()` advances the context from |
|
||||||
the preserved `currentIndex`. |
|
||||||
- Triggered identically by user-pressed Next and by auto-advance (`trackDidFinish`). |
|
||||||
|
|
||||||
### Previous |
|
||||||
|
|
||||||
Unchanged: steps back through the **context** (`currentIndex − 1`, clamped at 0). |
|
||||||
It never re-adds consumed queue items and does not consult `manualQueue` (accepted |
|
||||||
v1 simplification). |
|
||||||
|
|
||||||
### Shuffle |
|
||||||
|
|
||||||
Unchanged: only the context (`queue`) is shuffled. `manualQueue` order is always |
|
||||||
preserved. |
|
||||||
|
|
||||||
### Explicit play / new context |
|
||||||
|
|
||||||
`play(_:)` (double-click a row, space-bar, Home) sets a new **context** via |
|
||||||
`setQueue(_:contextName:)` and plays from it (setting `currentIndex`). The manual |
|
||||||
queue is **not** cleared — it survives the new context, matching the chosen Spotify |
|
||||||
model. |
|
||||||
|
|
||||||
### Queue editing |
|
||||||
|
|
||||||
```swift |
|
||||||
func moveInQueue(from: IndexSet, to: Int) // reorder manualQueue (panel drag) |
|
||||||
func removeFromQueue(at: IndexSet) // remove from manualQueue (panel × / swipe) |
|
||||||
``` |
|
||||||
|
|
||||||
## UI |
|
||||||
|
|
||||||
### `setQueue` signature change |
|
||||||
|
|
||||||
```swift |
|
||||||
func setQueue(_ tracks: [Track], contextName: String? = nil) |
|
||||||
``` |
|
||||||
|
|
||||||
Call sites in `ContentView` pass the context label: playlist/smart-playlist name, |
|
||||||
`"Library"`, or `"Recently Added"` (Home). Default `nil` keeps existing tests and |
|
||||||
any unlabeled callers compiling. |
|
||||||
|
|
||||||
### Up Next panel — new `QueueView` |
|
||||||
|
|
||||||
A SwiftUI `List`, docked on the right of the main content: |
|
||||||
|
|
||||||
- **Section "Queue"** — `player.manualQueue`. `.onMove` → `moveInQueue`; row × / |
|
||||||
`.swipeActions` → `removeFromQueue`. |
|
||||||
- **Section "Next from: \<contextName\>"** — `player.upcomingContext`, read-only. |
|
||||||
Double-click a row to jump to it in the context (sets `currentIndex` and plays). |
|
||||||
- **Empty state** — "Queue is empty. Right-click a track → Add to Queue." |
|
||||||
|
|
||||||
### Integration into `ContentView` |
|
||||||
|
|
||||||
- New `@State private var showQueue = false`. |
|
||||||
- Wrap the main-content region (the `VStack` holding `HomeView`/`TrackTableView`) in |
|
||||||
`HStack(spacing: 0) { mainContent; if showQueue { QueueView(player: player) } }`. |
|
||||||
- `PlayerControlsView` gains a queue-toggle button (bottom-right of the transport |
|
||||||
bar) bound to `showQueue`. The button is **hidden when `networkStatus` indicates |
|
||||||
remote-drive mode**. |
|
||||||
|
|
||||||
### Context menu — `TrackContextMenuConfig` |
|
||||||
|
|
||||||
Add two optional closures: |
|
||||||
|
|
||||||
```swift |
|
||||||
let onPlayNext: ((Track) -> Void)? |
|
||||||
let onAddToQueue: ((Track) -> Void)? |
|
||||||
``` |
|
||||||
|
|
||||||
Rendered in **both** menu builders, as a group above "Add to Playlist": |
|
||||||
|
|
||||||
- `TrackTableView.Coordinator.menuNeedsUpdate(_:)` (AppKit `NSMenuItem`s + actions). |
|
||||||
- `TrackContextMenuModifier` (SwiftUI `Button`s). |
|
||||||
|
|
||||||
Each is shown only when its closure is non-nil. In `ContentView.trackContextMenuConfig` |
|
||||||
they are wired to `player.playNext` / `player.addToQueue`, and passed as **`nil` |
|
||||||
when driving a remote device** so the items are hidden. |
|
||||||
|
|
||||||
## Remote / Edge Handling (local-only v1) |
|
||||||
|
|
||||||
- The disable gate is specifically the **`RemotePlaybackProvider`** case (driving a |
|
||||||
separate remote device, `networkStatus.mode == .remote`): `next()` delegates over |
|
||||||
the wire (unchanged) and never consults `manualQueue`; queue menu items hidden (nil |
|
||||||
closures); queue-toggle button hidden. No protocol changes. |
|
||||||
- **Streaming client** mode plays locally through `StreamingPlaybackProvider`, so it |
|
||||||
is **not** gated — the queue works there, as it does for local and streaming-host |
|
||||||
playback. |
|
||||||
- Empty manual queue + empty upcoming context: panel shows empty state; `next()` |
|
||||||
at the end of the context stops, as today. |
|
||||||
|
|
||||||
## Testing (TDD) |
|
||||||
|
|
||||||
New `PlayerViewModelTests` cases (reusing the `AudioService()` / `FakeStreamingProvider` |
|
||||||
pattern already in the file). Each test carries a step-by-step comment: |
|
||||||
|
|
||||||
1. `addToQueue` appends to `manualQueue`; `playNext` inserts at the front. |
|
||||||
2. `next()` plays the front of `manualQueue` before advancing the context, and |
|
||||||
`removeFirst` consumes it. |
|
||||||
3. After `manualQueue` drains, `next()` resumes the context at `currentIndex + 1`. |
|
||||||
4. Queue-while-idle (`currentTrack == nil`) starts playback immediately. |
|
||||||
5. `toggleShuffle()` leaves `manualQueue` order unchanged. |
|
||||||
6. `removeFromQueue` / `moveInQueue` mutate `manualQueue` correctly. |
|
||||||
7. `upcomingContext` returns the correct slice of the context. |
|
||||||
8. Existing PlayerViewModel tests remain green (backward-compatible model). |
|
||||||
|
|
||||||
New `TrackContextMenuConfigTests` case: the `onPlayNext` / `onAddToQueue` closures |
|
||||||
fire with the expected track. |
|
||||||
|
|
||||||
## Files Touched |
|
||||||
|
|
||||||
- `Music/ViewModels/PlayerViewModel.swift` — state, `playNext`/`addToQueue`/ |
|
||||||
`moveInQueue`/`removeFromQueue`, `next()` priority logic, `upcomingContext`, |
|
||||||
`setQueue(contextName:)`. |
|
||||||
- `Music/Models/QueueEntry.swift` — **new** identity wrapper for queued tracks. |
|
||||||
- `Music/Models/TrackContextMenuConfig.swift` — two new closures. |
|
||||||
- `Music/Views/TrackTableView.swift` — AppKit menu items + actions. |
|
||||||
- `Music/Views/TrackContextMenuModifier.swift` — SwiftUI menu buttons. |
|
||||||
- `Music/Views/PlayerControlsView.swift` — queue-toggle button. |
|
||||||
- `Music/Views/QueueView.swift` — **new** Up Next panel. |
|
||||||
- `Music/ContentView.swift` — `showQueue` state, panel layout, wiring, `contextName` |
|
||||||
at `setQueue` call sites. |
|
||||||
- `MusicTests/PlayerViewModelTests.swift`, `MusicTests/TrackContextMenuConfigTests.swift` |
|
||||||
— new tests. |
|
||||||
|
|
||||||
No `project.pbxproj` edit is needed: the project uses Xcode 16 file-system |
|
||||||
synchronized groups, so new files under `Music/` are picked up automatically. |
|
||||||
@ -1,166 +0,0 @@ |
|||||||
# Smart Playlist Conditions — Design Spec |
|
||||||
|
|
||||||
**Date:** 2026-05-30 |
|
||||||
**Status:** Approved |
|
||||||
|
|
||||||
## Overview |
|
||||||
|
|
||||||
Extend smart playlists to support structured metadata-based conditions (e.g. artist = "Alicia Keys", year > 2015). Multiple conditions combine with AND. The existing FTS free-text smart playlist flow is preserved unchanged. |
|
||||||
|
|
||||||
## Data Model |
|
||||||
|
|
||||||
### SmartPlaylist (extended) |
|
||||||
|
|
||||||
Add one new optional field to the existing `SmartPlaylist` struct: |
|
||||||
|
|
||||||
```swift |
|
||||||
var conditions: [SmartPlaylistCondition]? |
|
||||||
``` |
|
||||||
|
|
||||||
- `nil` → FTS mode (existing behavior, no change) |
|
||||||
- non-nil → structured SQL WHERE mode (new behavior) |
|
||||||
|
|
||||||
### SmartPlaylistCondition |
|
||||||
|
|
||||||
```swift |
|
||||||
struct SmartPlaylistCondition: Codable, Equatable, Sendable { |
|
||||||
var field: TrackField |
|
||||||
var op: ConditionOperator |
|
||||||
var value: ConditionValue |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
### TrackField |
|
||||||
|
|
||||||
`String` raw-value enum. Raw value matches the SQLite column name exactly (used directly in query building): |
|
||||||
|
|
||||||
``` |
|
||||||
title, artist, albumArtist, album, genre, composer, |
|
||||||
year, bpm, rating, playCount, trackNumber, discNumber, |
|
||||||
duration, bitrate, sampleRate, fileSize, |
|
||||||
dateAdded, dateModified, lastPlayedAt, fileFormat |
|
||||||
``` |
|
||||||
|
|
||||||
Each field has an associated `FieldType`: `.string`, `.int`, `.double`, `.date`. |
|
||||||
|
|
||||||
### ConditionOperator |
|
||||||
|
|
||||||
```swift |
|
||||||
enum ConditionOperator: String, Codable { |
|
||||||
case equals |
|
||||||
case startsWith // strings only |
|
||||||
case greaterThan // numbers and dates only |
|
||||||
case lessThan // numbers and dates only |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
Valid operators per field type: |
|
||||||
- String: `equals`, `startsWith` |
|
||||||
- Number (Int, Double): `equals`, `greaterThan`, `lessThan` |
|
||||||
- Date: `equals`, `greaterThan`, `lessThan` |
|
||||||
|
|
||||||
### ConditionValue |
|
||||||
|
|
||||||
Tagged Codable union: |
|
||||||
|
|
||||||
```swift |
|
||||||
enum ConditionValue: Codable, Equatable, Sendable { |
|
||||||
case string(String) |
|
||||||
case int(Int) |
|
||||||
case double(Double) |
|
||||||
case date(Date) |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
## Persistence |
|
||||||
|
|
||||||
### DB Migration (v5) |
|
||||||
|
|
||||||
```sql |
|
||||||
ALTER TABLE smart_playlists ADD COLUMN conditions TEXT; |
|
||||||
``` |
|
||||||
|
|
||||||
Nullable, no default. Existing rows stay `NULL` (FTS mode). |
|
||||||
|
|
||||||
### Encoding |
|
||||||
|
|
||||||
`SmartPlaylist.conditions` is encoded as a JSON string when writing to the `conditions` column and decoded on read. GRDB's `Codable` conformance handles this automatically via a custom `columnEncodingStrategy` or manual encode/decode. |
|
||||||
|
|
||||||
## Query Evaluation |
|
||||||
|
|
||||||
### SQL Generation |
|
||||||
|
|
||||||
New private method in `DatabaseService`: |
|
||||||
|
|
||||||
```swift |
|
||||||
private func buildWhereClause(_ conditions: [SmartPlaylistCondition]) -> (sql: String, args: StatementArguments) |
|
||||||
``` |
|
||||||
|
|
||||||
Mapping: |
|
||||||
|
|
||||||
| Field type | Operator | SQL fragment | |
|
||||||
|------------|--------------|-------------------------------------------| |
|
||||||
| String | equals | `LOWER({col}) = LOWER(?)` | |
|
||||||
| String | startsWith | `LOWER({col}) LIKE LOWER(?) || '%'` | |
|
||||||
| Number | equals | `{col} = ?` | |
|
||||||
| Number | greaterThan | `{col} > ?` | |
|
||||||
| Number | lessThan | `{col} < ?` | |
|
||||||
| Date | equals | `{col} = ?` | |
|
||||||
| Date | greaterThan | `{col} > ?` | |
|
||||||
| Date | lessThan | `{col} < ?` | |
|
||||||
|
|
||||||
Fragments joined with ` AND `. Final query: |
|
||||||
|
|
||||||
```sql |
|
||||||
SELECT * FROM tracks WHERE <clause> ORDER BY <sortCol> COLLATE NOCASE <asc|desc> |
|
||||||
``` |
|
||||||
|
|
||||||
### fetchTracks branch |
|
||||||
|
|
||||||
`PlaylistViewModel.observeSmartPlaylistTracks` branches on `smartPlaylist.conditions`: |
|
||||||
- `nil` → existing FTS `ValueObservation` (unchanged) |
|
||||||
- non-nil → new SQL WHERE `ValueObservation` using `buildWhereClause` |
|
||||||
|
|
||||||
## UI |
|
||||||
|
|
||||||
### Entry Point |
|
||||||
|
|
||||||
New "New Smart Playlist…" item in the app menu (alongside existing "New Playlist…"). Triggers a sheet (not an alert) since the form has multiple fields. |
|
||||||
|
|
||||||
### Condition Builder Sheet |
|
||||||
|
|
||||||
``` |
|
||||||
┌─────────────────────────────────────────┐ |
|
||||||
│ New Smart Playlist │ |
|
||||||
│ │ |
|
||||||
│ NAME │ |
|
||||||
│ [________________________] │ |
|
||||||
│ │ |
|
||||||
│ CONDITIONS (all must match) │ |
|
||||||
│ [Field ▾] [Operator ▾] [Value ] [−] │ |
|
||||||
│ [Field ▾] [Operator ▾] [Value ] [−] │ |
|
||||||
│ │ |
|
||||||
│ [+ Add Condition ] │ |
|
||||||
│ │ |
|
||||||
│ [Cancel] [Save] │ |
|
||||||
└─────────────────────────────────────────┘ |
|
||||||
``` |
|
||||||
|
|
||||||
- One condition row shown by default |
|
||||||
- Operator picker options update when field changes (string → is/starts with; number/date → is/greater than/less than) |
|
||||||
- Value input adapts to field type: `TextField` for strings and numbers, `DatePicker` for date fields |
|
||||||
- Remove (−) button disabled when only one condition remains |
|
||||||
- Save disabled until name is non-empty and all condition values are non-empty |
|
||||||
|
|
||||||
### Edit Flow |
|
||||||
|
|
||||||
- **Structured smart playlists:** context menu shows "Edit…" → reopens the builder sheet pre-populated |
|
||||||
- **FTS smart playlists:** context menu keeps existing "Edit Search Query…" → existing text alert (no change) |
|
||||||
|
|
||||||
## Out of Scope |
|
||||||
|
|
||||||
- OR logic between conditions |
|
||||||
- Nested condition groups |
|
||||||
- Track limit per playlist ("limit to N songs") |
|
||||||
- Migrating existing FTS playlists to structured format |
|
||||||
- Live-updating toggle (not needed — `ValueObservation` already handles this automatically) |
|
||||||
@ -1,86 +0,0 @@ |
|||||||
--- |
|
||||||
title: Track Context Menu on Bottom Controls |
|
||||||
date: 2026-05-30 |
|
||||||
status: approved |
|
||||||
--- |
|
||||||
|
|
||||||
## Goal |
|
||||||
|
|
||||||
Right-clicking the now-playing area (bottom-left of the window) shows the same context menu as right-clicking a track in the track table: add to last playlist, add to any playlist via submenu, and remove from the current playlist when one is open. |
|
||||||
|
|
||||||
## Shared Config Struct |
|
||||||
|
|
||||||
A new `TrackContextMenuConfig` struct captures everything the menu needs: |
|
||||||
|
|
||||||
```swift |
|
||||||
struct TrackContextMenuConfig { |
|
||||||
let playlists: [Playlist] |
|
||||||
let lastUsedPlaylistName: String? |
|
||||||
let selectedPlaylist: Playlist? |
|
||||||
let onAddToPlaylist: (Track, Playlist) -> Void |
|
||||||
let onAddToLastPlaylist: ((Track) -> Void)? |
|
||||||
let onRemoveFromPlaylist: ((Track) -> Void)? |
|
||||||
} |
|
||||||
``` |
|
||||||
|
|
||||||
This is the single source of truth for menu data. Both `TrackTableView` and `PlayerControlsView` receive one instance, constructed by `ContentView`. |
|
||||||
|
|
||||||
## Shared ViewModifier |
|
||||||
|
|
||||||
`TrackContextMenuModifier` is a SwiftUI `ViewModifier` that takes a `Track?` and `TrackContextMenuConfig?` and applies `.contextMenu` when both are non-nil: |
|
||||||
|
|
||||||
- **"Add to [last]"** button — shown only when `lastUsedPlaylistName` is set and `onAddToLastPlaylist` is non-nil. |
|
||||||
- **"Add to Playlist"** submenu — one `Button` per playlist in `playlists`. Calls `onAddToPlaylist(track, playlist)`. |
|
||||||
- **Divider + "Remove from Playlist"** — shown only when `selectedPlaylist != nil` and `onRemoveFromPlaylist` is non-nil. |
|
||||||
|
|
||||||
Menu is omitted entirely (no empty menu flicker) when `track` or `config` is nil. |
|
||||||
|
|
||||||
## PlayerControlsView Changes |
|
||||||
|
|
||||||
`PlayerControlsView` gains one new optional parameter: |
|
||||||
|
|
||||||
```swift |
|
||||||
let contextMenuConfig: TrackContextMenuConfig? |
|
||||||
``` |
|
||||||
|
|
||||||
The `nowPlayingSection` view applies `.trackContextMenu(track: currentTrack, config: contextMenuConfig)` (a convenience extension wrapping `TrackContextMenuModifier`). |
|
||||||
|
|
||||||
## TrackTableView Refactor |
|
||||||
|
|
||||||
`TrackTableView`'s existing four playlist-related parameters (`playlists`, `lastUsedPlaylistName`, `selectedPlaylist`, `onAddToLastPlaylist`, `onRemoveFromPlaylist`) are replaced by a single `contextMenuConfig: TrackContextMenuConfig?`. The `Coordinator.menuNeedsUpdate` builds its `NSMenu` from this config. This makes both call sites symmetric. |
|
||||||
|
|
||||||
> The AppKit `NSMenu` path in `TrackTableView` is kept — SwiftUI `.contextMenu` does not attach per-row in an `NSTableView`, so the table continues using `menuNeedsUpdate`. |
|
||||||
|
|
||||||
## ContentView Changes |
|
||||||
|
|
||||||
`ContentView` constructs one `TrackContextMenuConfig` and passes it to both views: |
|
||||||
|
|
||||||
```swift |
|
||||||
let menuConfig = TrackContextMenuConfig( |
|
||||||
playlists: playlist.allPlaylists, |
|
||||||
lastUsedPlaylistName: playlist.lastUsedPlaylistName, |
|
||||||
selectedPlaylist: playlist.selectedPlaylist, |
|
||||||
onAddToPlaylist: { track, pl in try? playlist.addTrack(track, to: pl) }, |
|
||||||
onAddToLastPlaylist: { track in try? playlist.addTrackToLastUsedPlaylist(track) }, |
|
||||||
onRemoveFromPlaylist: playlist.selectedPlaylist != nil ? { track in |
|
||||||
if let selected = playlist.selectedPlaylist { |
|
||||||
try? playlist.removeTrack(track, from: selected) |
|
||||||
} |
|
||||||
} : nil |
|
||||||
) |
|
||||||
``` |
|
||||||
|
|
||||||
## Files Affected |
|
||||||
|
|
||||||
| File | Change | |
|
||||||
|------|--------| |
|
||||||
| `Music/Models/TrackContextMenuConfig.swift` | New file — struct definition | |
|
||||||
| `Music/Views/TrackContextMenuModifier.swift` | New file — SwiftUI ViewModifier | |
|
||||||
| `Music/Views/PlayerControlsView.swift` | Add `contextMenuConfig` param, apply modifier to `nowPlayingSection` | |
|
||||||
| `Music/Views/TrackTableView.swift` | Replace individual playlist params with `contextMenuConfig`, adapt `menuNeedsUpdate` | |
|
||||||
| `Music/ContentView.swift` | Construct and pass `TrackContextMenuConfig` to both views | |
|
||||||
|
|
||||||
## Out of Scope |
|
||||||
|
|
||||||
- Keyboard shortcut for the menu |
|
||||||
- Any new menu items not already in the track table menu |
|
||||||
@ -1,206 +0,0 @@ |
|||||||
# Track "Get Info" — Design Spec |
|
||||||
|
|
||||||
**Date:** 2026-05-30 |
|
||||||
**Status:** Approved (design); pending implementation plan |
|
||||||
**Branch:** feat/music-streaming |
|
||||||
|
|
||||||
## Goal |
|
||||||
|
|
||||||
Replicate the macOS Music app's right-click **Get Info** experience: a dialog that |
|
||||||
shows all of a track's metadata and lets the user edit it. Edits persist to the |
|
||||||
app's library (SQLite DB) **and** are written back into the audio file's embedded |
|
||||||
tags, mirroring how the real Music app mutates files. |
|
||||||
|
|
||||||
## Decisions (locked) |
|
||||||
|
|
||||||
| Question | Decision | |
|
||||||
|---|---| |
|
||||||
| Where edits are saved | **DB + write file tags** (best-effort file writeback) | |
|
||||||
| Single vs multi-track | **Both** — single track shows all fields; multiple selected tracks use mixed-value handling | |
|
||||||
| Field scope | **Existing model fields only** — no schema migration; read-only File info section | |
|
||||||
| Writeback formats (v1) | **mp3** (ID3TagEditor) + **m4a / alac / aac** (AVFoundation). flac/wav/aiff → DB only with a UI note | |
|
||||||
| Tag library strategy | **Lighter path, TagLib-ready** — abstract behind a `TagWriter` protocol so a TagLib writer can be added later for flac/wav/aiff with no rework | |
|
||||||
| Layout | **Tabbed** (Details / File), like macOS Music | |
|
||||||
| Failure model | **DB is always saved; file writeback is best-effort** with a non-blocking warning on failure | |
|
||||||
|
|
||||||
## Non-goals (v1) |
|
||||||
|
|
||||||
- No new metadata fields (no comments, lyrics, sorting, artwork). Editing is limited to |
|
||||||
fields already present on the `Track` model. |
|
||||||
- No album-artwork display or editing. |
|
||||||
- No flac/wav/aiff **file-tag** writeback (those edits save to the DB only for now). |
|
||||||
The architecture leaves a clean seam to add TagLib later. |
|
||||||
|
|
||||||
## Data model context |
|
||||||
|
|
||||||
`Track` (Music/Models/Track.swift) already holds every field we need. No migration. |
|
||||||
|
|
||||||
**Editable fields** (surfaced on the Details tab): |
|
||||||
`title`, `artist`, `albumArtist`, `album`, `genre`, `composer` (String); |
|
||||||
`year`, `trackNumber`, `discNumber`, `bpm` (Int?); `rating` (Int 0–5). |
|
||||||
|
|
||||||
> **rating is DB-only in v1.** It's an app/iTunes concept with format-specific |
|
||||||
> scales (ID3 POPM 0–255, iTunes atom 0–100), so writing it to file tags is |
|
||||||
> deferred. `EditableTrackFields` includes `rating`, but the `TagWriter`s ignore it |
|
||||||
> — rating persists only via `updateTrack`. All other editable fields are written to |
|
||||||
> both file and DB (where a writer exists). |
|
||||||
|
|
||||||
**Read-only fields** (File tab): `fileURL` (path), `fileFormat`, `bitrate`, |
|
||||||
`sampleRate`, `fileSize`, `duration`, `playCount`, `lastPlayedAt`, `dateAdded`, |
|
||||||
`dateModified`. (`playCount`/`lastPlayedAt`/`rating` are app-managed, not file tags.) |
|
||||||
|
|
||||||
> Metadata source today: tracks are read from file tags via AVFoundation **once at |
|
||||||
> import** (ScannerService), then the DB is the source of truth. `insertBatch` uses |
|
||||||
> `.ignore` on conflict, so re-scans do not clobber DB edits. |
|
||||||
|
|
||||||
## Architecture & components |
|
||||||
|
|
||||||
Each unit has one responsibility and a well-defined interface. |
|
||||||
|
|
||||||
### `EditableTrackFields` (new value type) |
|
||||||
A plain struct of the ~11 editable fields. The unit of "what the user can change" |
|
||||||
and "what changed." Decouples the sheet and the writer from the full `Track`. |
|
||||||
|
|
||||||
### `TagWriter` protocol + factory (new) |
|
||||||
``` |
|
||||||
protocol TagWriter { |
|
||||||
func write(_ fields: EditableTrackFields, to fileURL: URL) throws |
|
||||||
} |
|
||||||
``` |
|
||||||
- `TagWriterFactory.writer(for: URL) -> TagWriter?` selects by file extension; |
|
||||||
returns `nil` for unsupported formats (flac/wav/aiff in v1). |
|
||||||
- Implementations: |
|
||||||
- `ID3TagWriter` — mp3, via the **ID3TagEditor** SPM package. |
|
||||||
- `MP4TagWriter` — m4a / alac / aac, via **AVFoundation** `AVAssetExportSession` |
|
||||||
(`AVAssetExportPresetPassthrough`) writing iTunes/common metadata items. |
|
||||||
- **No code outside the writers knows how tags are encoded per format.** |
|
||||||
|
|
||||||
### `TrackEditService` (new) |
|
||||||
Orchestrates one save. For each target track: |
|
||||||
1. Diff `EditableTrackFields` against the original → set of changed fields. |
|
||||||
2. If a `TagWriter` exists for the format: write tags to a **temp copy**, then |
|
||||||
`FileManager.replaceItemAt` to atomically swap the original (never leaves a |
|
||||||
half-written file). |
|
||||||
3. Recompute `fileSize`, `dateModified`, `fileHash` from the new file. |
|
||||||
4. Build the updated `Track` (changed fields + refreshed stats) and persist via |
|
||||||
`DatabaseService.updateTrack`. |
|
||||||
5. On file-write failure (or unsupported format): still persist DB changes; collect |
|
||||||
a warning for that track. |
|
||||||
|
|
||||||
Owns the **single- and multi-track diff logic** as pure, testable functions |
|
||||||
(no UI, no I/O in the diff step). |
|
||||||
|
|
||||||
### `TrackInfoSheet` (new SwiftUI view) |
|
||||||
The Get Info dialog (see Layout). Holds local `@State` for the edited fields, |
|
||||||
prefilled from the target(s). On Save, hands `EditableTrackFields` + the target |
|
||||||
track set to `TrackEditService`. Models the `.sheet` pattern already used by |
|
||||||
`SmartPlaylistBuilderSheet`. |
|
||||||
|
|
||||||
### `DatabaseService.updateTrack(_ track: Track) throws` (new method) |
|
||||||
GRDB `track.update(db)` inside `dbPool.write`. **Implementation plan must verify |
|
||||||
whether the `tracks_ft` FTS5 table is kept in sync automatically (triggers / |
|
||||||
external-content) and, if not, update it here.** |
|
||||||
|
|
||||||
### Context-menu integration (edits) |
|
||||||
- Add **Get Info** (⌘I) to `TrackContextMenuConfig`, `TrackTableView`'s `NSMenu` |
|
||||||
(`menuNeedsUpdate`), and `TrackContextMenuModifier`. |
|
||||||
- Target resolution: the menu operates on the **current selection if the |
|
||||||
right-clicked row is part of it**, otherwise just the clicked row (matches macOS |
|
||||||
Music). This requires the config to expose the current multi-selection (or a |
|
||||||
callback that returns the target set), not just a single `Track`. |
|
||||||
- `ContentView` holds the presented-target state and shows the `.sheet`. |
|
||||||
|
|
||||||
## Save sequence (per track) |
|
||||||
|
|
||||||
``` |
|
||||||
edit fields ─▶ diff vs original ─▶ writer for format? |
|
||||||
│ yes │ no / unsupported |
|
||||||
▼ ▼ |
|
||||||
write tags to temp copy (skip file write, |
|
||||||
─▶ atomic replace original collect "DB-only" note) |
|
||||||
│ |
|
||||||
success? │ fail ─────────┐ |
|
||||||
▼ ▼ |
|
||||||
recompute size/mod/hash keep old stats + warning |
|
||||||
│ │ |
|
||||||
└──────┬────────┘ |
|
||||||
▼ |
|
||||||
updateTrack(...) in DB |
|
||||||
``` |
|
||||||
|
|
||||||
Result: the **DB edit always lands**; file writeback is best-effort. Failures |
|
||||||
surface as a single non-blocking summary alert ("Saved to library. Couldn't write |
|
||||||
tags to N file(s): <reason>"). |
|
||||||
|
|
||||||
## Multi-track behavior |
|
||||||
|
|
||||||
- Prefill: fields with one shared value across all targets prefill normally; |
|
||||||
fields that differ show a **"Mixed"** placeholder and start empty. |
|
||||||
- Apply: **only fields the user actually edits** are applied — to all targets. |
|
||||||
Untouched "Mixed" fields are left per-track unchanged. |
|
||||||
- Saving N tracks runs in a background `Task`, sequentially, with a small progress |
|
||||||
indicator when N is large. Per-track failures aggregate into one summary. |
|
||||||
|
|
||||||
## UI layout (tabbed) |
|
||||||
|
|
||||||
``` |
|
||||||
┌─ Get Info ──────────────────────────────┐ |
|
||||||
│ [ Details ] [ File ] │ |
|
||||||
├──────────────────────────────────────────┤ |
|
||||||
│ Title [_______________________] │ |
|
||||||
│ Artist [_______________________] │ |
|
||||||
│ Album Artist[_______________________] │ |
|
||||||
│ Album [_______________________] │ |
|
||||||
│ Genre [_______________________] │ |
|
||||||
│ Composer [_______________________] │ |
|
||||||
│ Year [____] Track [__]/[__] Disc [__]/[__] |
|
||||||
│ BPM [____] Rating ★★★☆☆ │ |
|
||||||
├──────────────────────────────────────────┤ |
|
||||||
│ File tab: format, bitrate, sample rate, │ |
|
||||||
│ size, duration, path, date added, │ |
|
||||||
│ play count, last played — read-only │ |
|
||||||
├──────────────────────────────────────────┤ |
|
||||||
│ [ Cancel ] [ Save ] │ |
|
||||||
└──────────────────────────────────────────┘ |
|
||||||
``` |
|
||||||
|
|
||||||
- Numeric fields (year / track / disc / bpm) validate on input. |
|
||||||
- flac/wav/aiff targets show a subtle note under the tabs: *"Edits save to your |
|
||||||
library only — tag writing isn't supported for .flac yet."* |
|
||||||
- Cancel = `.cancelAction`, Save = `.defaultAction`. |
|
||||||
|
|
||||||
## Error handling & risks |
|
||||||
|
|
||||||
- **Atomic replace** (temp file + `replaceItemAt`) prevents audio-file corruption on |
|
||||||
a failed/interrupted write. |
|
||||||
- **File-write failure** → DB still saved + non-blocking warning with the reason. |
|
||||||
- **Sandbox / permissions (must verify early):** if the app is sandboxed, writing to |
|
||||||
user audio files requires the appropriate entitlement and/or a security-scoped |
|
||||||
bookmark for the music folder. If writing is blocked, file writeback cannot work |
|
||||||
regardless of library — DB edits still work. Verify before building the writers. |
|
||||||
- **New dependency:** ID3TagEditor added via SPM to the Xcode project. |
|
||||||
- **Hash drift:** writing tags changes file size/mod-date → `fileHash`. Step 3 |
|
||||||
refreshes these so the next scan doesn't treat the file as changed. |
|
||||||
|
|
||||||
## Testing (TDD) |
|
||||||
|
|
||||||
- **TagWriter round-trip:** write `EditableTrackFields` to bundled mp3 and m4a |
|
||||||
fixtures, re-read via AVFoundation, assert each field; assert the file remains a |
|
||||||
valid, playable asset after atomic replace. |
|
||||||
- **Diff / multi-track logic:** pure-function, table-driven tests for "which fields |
|
||||||
changed," "shared vs Mixed across N tracks," and "apply only edited fields to all." |
|
||||||
- **Stat refresh:** `fileSize` / `dateModified` / `fileHash` recomputed after |
|
||||||
writeback. |
|
||||||
- **Factory:** correct writer per extension; `nil` for flac/wav/aiff. |
|
||||||
- **`updateTrack`:** persists edited fields and keeps `tracks_ft` in sync. |
|
||||||
|
|
||||||
## Implementation outline |
|
||||||
|
|
||||||
1. Add ID3TagEditor SPM dependency; confirm sandbox/file-write permissions. |
|
||||||
2. `EditableTrackFields` + diff/multi-track pure logic (+ tests). |
|
||||||
3. `TagWriter` protocol, factory, `ID3TagWriter`, `MP4TagWriter` (+ round-trip tests). |
|
||||||
4. `DatabaseService.updateTrack` (+ FTS sync) (+ test). |
|
||||||
5. `TrackEditService` wiring the save sequence (+ tests). |
|
||||||
6. `TrackInfoSheet` UI (tabs, validation, Mixed handling). |
|
||||||
7. Context-menu "Get Info" (⌘I) + target resolution + `ContentView` sheet presentation. |
|
||||||
8. Manual verification against real mp3/m4a/flac files. |
|
||||||
Loading…
Reference in new issue