Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
1b9868ddd0 | ||
|
|
6aeac42a46 | ||
|
|
68e7edb8c9 | ||
|
|
a3b5b43777 | ||
|
|
5fae61cdf1 | ||
|
|
1f43234b80 | ||
|
|
ca00f8a803 | ||
|
|
0be7b4fdc8 | ||
|
|
18f10e966b | ||
|
|
e897364a73 | ||
|
|
5fa68fcf43 | ||
|
|
b1368bbc7e | ||
|
|
9432f68c8b | ||
|
|
4c921ed1ba | ||
|
|
f8800ca1f8 | ||
|
|
dddc8aaf55 | ||
|
|
bcf3856b6f | ||
|
|
26251badda |
@@ -81,6 +81,24 @@ rules. Never "go fix it" — the leg contract is in this file.
|
||||
| escalation | `yj-loop.escalate` | go/kimi-k3 (T3) | same leg re-run, seeded with failure summary |
|
||||
| prose (PR body, commit msgs, journal) | `yj-loop.scribe` | go/mimo-v2.5 (T0) | text only, from supplied facts |
|
||||
|
||||
**Model fallback on quota exhaustion.** The pinned models are the
|
||||
intent, not a guarantee. The qwen token plan is a weekly pool and has
|
||||
run dry mid-tick (`429 … 1-week quota exhausted`). When a leg's launch
|
||||
fails with a 429, re-run it with a per-run `model` override one rung
|
||||
down and journal the substitution — never spend the T3 escalation
|
||||
model on a quota substitution. The qwen-pinned legs (`work`,
|
||||
`diffreview`) fall back `qwen/deepseek-v4-pro-0813` → `go/deepseek-v4-pro`
|
||||
→ `go/glm-5.3-flash`. Do **not** use the `deepseek/...` provider: it has
|
||||
no models, only catalog overrides, and fails silently (empty artifact,
|
||||
no session) — the model lives on the `go` gateway.
|
||||
|
||||
**Launch legs in the foreground.** The async subagent runner has died
|
||||
without persisting a child session (nothing to resume) and emits
|
||||
spurious "needs attention" nudges on runs that are already complete.
|
||||
Foreground `subagent` calls are the reliable mode here. A worker that
|
||||
dies mid-leg leaves uncommitted work: inspect the tree, then relaunch
|
||||
to *complete* — never to re-implement.
|
||||
|
||||
Orchestrator-only legs: **claim** (`issue.sh claim --branch` — atomic,
|
||||
refuses if held), **ship's PR/CI polling** (REST API below — `gitea_ci`
|
||||
job_logs 404s on this Gitea; the REST endpoints are the way), **merge**
|
||||
@@ -168,8 +186,12 @@ Merge when, and only when, **all** hold:
|
||||
`block_on_outdated_branch: true` refuses it anyway; never
|
||||
`force_manually_merged` around it.
|
||||
|
||||
**Refresh before every merge.** In the loop worktree: fetch, then
|
||||
`git merge origin/main` on the PR branch, push. A textual conflict
|
||||
**Refresh before every merge.** In the loop worktree: `git fetch origin`
|
||||
in the same breath, then `git merge origin/main` on the PR branch,
|
||||
push. The fetch must be immediate — a cached `origin/main` merges
|
||||
against the wrong base, CI goes green on it, and the merge comes back
|
||||
405 "behind base", one whole CI cycle wasted (measured on the adoption
|
||||
wave). A textual conflict
|
||||
stops the leg there — as diff text, not as a failed merge click: hunks
|
||||
the loop authored are resolved by the loop; anything else is left with
|
||||
`⟦loop⟧` comment for a human, never forced. After any refresh push,
|
||||
@@ -247,7 +269,10 @@ AVD), then `make android-emulator` per session.
|
||||
|
||||
- **Worktree:** `git worktree add ~/.paseo/worktrees/loop/jumpy-hound
|
||||
origin/main` (from any clone; branch from origin/main in the loop
|
||||
tree, never `git checkout main`).
|
||||
tree, never `git checkout main`). **Provision it once before the
|
||||
first push:** `make build-frontend` and `make testdata` — the pre-push
|
||||
`go-test` hook needs `frontend/dist` (the `//go:embed` in `main.go`)
|
||||
and the fixture library, and refuses the push without them.
|
||||
- **Session:** pi in that worktree, `/name loop`. Add the job via
|
||||
`/schedule-prompt` (name `yj-loop`, cron
|
||||
`0 0 10-18 * * 1-5`, prompt: "Read `.pi/skills/yj-loop/SKILL.md` and
|
||||
@@ -270,4 +295,12 @@ AVD), then `make android-emulator` per session.
|
||||
which leg it really is.
|
||||
- The job did not fire — the scheduler fires only while a session is
|
||||
open in its directory (documented); "the loop is off" is the correct
|
||||
reading, not a bug.
|
||||
reading, not a bug.
|
||||
- `error: object file … is empty` / `unpack-objects failed` / `bad
|
||||
object refs/heads/…` during a fetch or checkout — the shared object
|
||||
store was corrupted (a killed fetch leaves 0-byte object files, and a
|
||||
local ref can end up pointing at the dead sha1). **Halt and report**;
|
||||
do not retry, the churn only deepens it. Human repair: delete the
|
||||
0-byte objects, `git fetch origin --prune`, delete any ref that
|
||||
still dangles (`git update-ref -d refs/heads/<b>`), re-checkout the
|
||||
worktree at `origin/main`, then `git fsck --full`.
|
||||
@@ -112,7 +112,9 @@ in the job's directory — that limitation is the switch:
|
||||
|
||||
- **Worktree:** `git worktree add` a dedicated clone at
|
||||
`~/.paseo/worktrees/loop/jumpy-hound`. Loop edits happen only there; a
|
||||
dirty tree there is the loop's business and nobody else's.
|
||||
dirty tree there is the loop's business and nobody else's. **Provision
|
||||
it once before its first push:** `make build-frontend` + `make testdata`
|
||||
— the pre-push `go-test` hook needs both and refuses without them.
|
||||
- **Session:** pi in that worktree, `/name loop`. The job is bound to that
|
||||
session, so another pi elsewhere in the same directory does not
|
||||
double-fire it.
|
||||
|
||||
@@ -0,0 +1,263 @@
|
||||
# 021 — Listening accounting: smart plays, skips, and a real history
|
||||
|
||||
**Issue:** none yet — open one before the first edit (tracker is the
|
||||
source of truth; `./scripts/issue.sh search "skip play count"` comes
|
||||
back empty as of this writing).
|
||||
|
||||
**Status:** plan — not started.
|
||||
|
||||
**Relates:** play-count rendering (`frontend/src/components/track-list/columns.ts`),
|
||||
smart playlists (`backend/smartplaylist/`), the event contract
|
||||
(`TrackPlayCountChanged`), and any future Wrapped / "minutes listened"
|
||||
surface.
|
||||
|
||||
---
|
||||
|
||||
## What exists now
|
||||
|
||||
Three facts, all load-bearing.
|
||||
|
||||
**A "play" is recorded only on a natural finish.** `recordPlay`
|
||||
(`backend/queue/playhistory.go:9`) is called from exactly one place —
|
||||
`OnPlaybackFinished` (`backend/queue/handlers.go:14`), and only when
|
||||
`srcErr == nil`. A track the user skips past at 90% is *not* a play;
|
||||
neither is one they pause at 60% and abandon. `play_count` /
|
||||
`last_played` on `audio_files` reflect "finished to the end," nothing
|
||||
more.
|
||||
|
||||
**There is no skip concept at all.** Skipping is indistinguishable
|
||||
from a natural finish, a pause, or a shutdown. Nothing records "the
|
||||
user rejected this track," so no downstream feature (smart playlists,
|
||||
shuffle, the revisit shelf, a future skip-rate heuristic) can ask
|
||||
about it.
|
||||
|
||||
**`play_history` is a write-only log.** It holds
|
||||
`(audio_file_id, played_at)` and nothing reads it — no sqlc query
|
||||
touches it, no `PlayHistory` read path exists. Its only recorded
|
||||
purpose is the timestamps a future "minutes listened over time"
|
||||
feature would need. It is classified `Authored, Cascade` in
|
||||
`backend/datamap/datamap.go:272` ("Listening history").
|
||||
|
||||
So the gaps are: (1) skips are invisible, and (2) "played" is
|
||||
under-counted — the opposite of the usual over-counting fear. The
|
||||
scrobble intuition (count a play once `min(50%, 4:00)` has been
|
||||
*heard*, independent of how it ends) is the fix for both.
|
||||
|
||||
---
|
||||
|
||||
## What we're building
|
||||
|
||||
A single classification of every track *exit*, plus one row per exit in
|
||||
a listening log, plus the existing denormalized `play_count` /
|
||||
`last_played` updated to match the new meaning. Three exit kinds:
|
||||
|
||||
| kind | condition |
|
||||
|---|---|
|
||||
| `complete` | reached natural end, **or** abandoned with `remaining <= tail` |
|
||||
| `play` | heard `>= playThreshold`, abandoned before the tail |
|
||||
| `skip` | user moved to a *different* track before `playThreshold` |
|
||||
|
||||
Not counted, not any kind: decode failure, pause/stop/shutdown before
|
||||
the threshold, and tracks shorter than `minTrackLength`.
|
||||
|
||||
### The thresholds — named judgements, one file
|
||||
|
||||
Follow the `PreviousRestartThreshold` precedent (`backend/queue/queue.go:28`,
|
||||
a bare `const` with a comment). A new `backend/queue/listen.go` (or a
|
||||
tiny `backend/listencount` package) declares:
|
||||
|
||||
```go
|
||||
const (
|
||||
// A track this short is deliberated jingle / interstitial and is
|
||||
// never counted, either way.
|
||||
minTrackLength = 30 * time.Second
|
||||
// The scrobble rule: half the track, or four minutes, whichever
|
||||
// comes first (Last.fm / ListenBrainz).
|
||||
playThresholdMax = 4 * time.Minute
|
||||
// "Finished enough": within 15s of the end, or the last 10%,
|
||||
// whichever is larger. A 10:00 ambient track gets a 60s fade
|
||||
// window; a 2:00 pop song gets 15s.
|
||||
tailWindowFloor = 15 * time.Second
|
||||
tailWindowFraction = 0.10
|
||||
)
|
||||
|
||||
func playThreshold(d time.Duration) time.Duration {
|
||||
return min(d/2, playThresholdMax)
|
||||
}
|
||||
func tailWindow(d time.Duration) time.Duration {
|
||||
return max(d/10, tailWindowFloor)
|
||||
}
|
||||
```
|
||||
|
||||
Classification is a pure function of `(reason, position, duration)` and
|
||||
*therefore unit-testable without a player*:
|
||||
|
||||
```go
|
||||
func classify(reason ExitReason, pos, dur time.Duration) Kind
|
||||
```
|
||||
|
||||
`ExitReason` is `finished | skipped | failed | abandoned`. `skipped`
|
||||
means the queue moved to a different track by user action (Next,
|
||||
Previous past the restart threshold, PlayIndex, queue replacement,
|
||||
select-from-a-list). `failed` is the decode-error path. `abandoned` is
|
||||
pause/stop/unload/shutdown — and in v1 is a no-op (see open question 3).
|
||||
|
||||
**"Heard" is approximated by the position at exit.** We read
|
||||
`player.CurrentPositionSeconds()` at the moment of the transition, not
|
||||
an accumulated listen-time ledger. A user who seeks to 80% and listens
|
||||
5 seconds reads as "heard 80%." That is deliberately accepted for v1:
|
||||
it is how most players actually behave, it is drastically simpler, and
|
||||
the failure mode ("counted a track you skimmed as played") is mild and
|
||||
exactly what the scrobble threshold already forgives. Written down
|
||||
because "position is not listen time" is the one assumption that will
|
||||
look like a bug if it is not.
|
||||
|
||||
**Fires once per listen.** Leaving a track already leaves it; the
|
||||
`chainID` guard in `player.onPlaybackFinished` (`backend/player/player.go:633`)
|
||||
already swallows a stale finish callback, and a transition advances
|
||||
`currentIndex` past the finished track. The classifier needs the same
|
||||
guard so a Next-then-stale-finish cannot produce two rows. Key it on the
|
||||
`(audioFileID, chainID)` the transition was about.
|
||||
|
||||
---
|
||||
|
||||
## Schema — resolved: fresh design, no migration
|
||||
|
||||
The A/B migration agonizing is moot. This app has two users and both
|
||||
are devs, and play counts are explicitly not worth preserving yet — so
|
||||
the schema is written as if listening accounting had been designed in
|
||||
from the start, and the existing two databases rebuild what they need
|
||||
(see below). There is no migration step and none is re-introduced.
|
||||
|
||||
**`play_history` is renamed to `listening_events`** and grows the three
|
||||
kinds, plus the raw position/duration the classification was made from:
|
||||
|
||||
```sql
|
||||
CREATE TABLE IF NOT EXISTS listening_events (
|
||||
id INTEGER PRIMARY KEY,
|
||||
audio_file_id INTEGER NOT NULL,
|
||||
kind TEXT NOT NULL DEFAULT 'complete'
|
||||
CHECK (kind IN ('complete','play','skip')),
|
||||
position_seconds INTEGER NOT NULL DEFAULT 0,
|
||||
duration_seconds INTEGER NOT NULL DEFAULT 0,
|
||||
occurred_at DATETIME NOT NULL DEFAULT (datetime('now')),
|
||||
FOREIGN KEY(audio_file_id) REFERENCES audio_files(id) ON DELETE CASCADE
|
||||
);
|
||||
CREATE INDEX IF NOT EXISTS idx_listening_events_audio_file_id
|
||||
ON listening_events(audio_file_id);
|
||||
CREATE INDEX IF NOT EXISTS idx_listening_events_occurred_at
|
||||
ON listening_events(occurred_at);
|
||||
```
|
||||
|
||||
`position_seconds`/`duration_seconds` are kept raw so a future re-tune
|
||||
of the threshold does not force the events to be re-recorded. `kind`
|
||||
stays the write-time classification; the raw reading is evidence, not
|
||||
a second copy of the rule.
|
||||
|
||||
**The counters are denormalized onto `audio_files`** — `skip_count` /
|
||||
`last_skipped` join the existing `play_count` / `last_played`, because
|
||||
that is where the hot read path already lives and a log join per track
|
||||
row is not acceptable. This does grow the MIXED-KIND wart (see the
|
||||
survey below for the structural answer), but it is the *continuation* of
|
||||
the existing design, not a new leak: play counts sat on `audio_files`
|
||||
from before this feature existed.
|
||||
|
||||
**What happens to the two real databases on next launch.**
|
||||
`listening_events` is a new table, created verbatim. `audio_files`
|
||||
gains two columns, which `retireStaleTables` treats as a stale Owned
|
||||
table and rebuilds by rescan — dropping `play_count` / `last_played` /
|
||||
`tag_status` with it, which is the accepted cost stated in the issue.
|
||||
`play_history` is gone from the schema and the datamap, so
|
||||
`obsoleteTables` drops it; its (natural-finish-only) timestamp rows go
|
||||
with it. Nothing here is wrong on a fresh install, and on the two dev
|
||||
machines the answer is the documented "delete and rescan."
|
||||
|
||||
---
|
||||
|
||||
## Wiring: where the classifier is called
|
||||
|
||||
The risk is not the classifier — it is that **every track-replacement
|
||||
path must classify the outgoing track**, and there are many: `Next`,
|
||||
`Previous` (past the 3s restart threshold), `PlayIndex`, `playFromStart`,
|
||||
`SetQueue` / clear-and-play, remove-current, and select-from-a-list.
|
||||
Miss one and that path silently never records a skip.
|
||||
|
||||
So the classification is centralized in one queue method —
|
||||
|
||||
```go
|
||||
// leaveCurrent(reason) classifies the track at currentIndex as it is
|
||||
// about to be replaced, and records exactly one listening event.
|
||||
// Must be called without q.mu held (it writes to SQLite).
|
||||
func (q *Queue) leaveCurrent(reason ExitReason)
|
||||
```
|
||||
|
||||
— which reads position/duration from the player, calls `classify`, and
|
||||
emits the play/skip row + `TrackPlayCountChanged` when `kind != skip`.
|
||||
`OnPlaybackFinished(nil)` routes through `leaveCurrent(finished)`, the
|
||||
navigation methods route through `leaveCurrent(skipped)` before they
|
||||
advance, and `recordPlay` becomes the "did a play happen" half of it.
|
||||
|
||||
Because "one path forgot to call it" is the failure mode, a **source
|
||||
sweep** pins it, on the pattern of `TestNoDirectRuntimeEmits`
|
||||
(`backend/events/noemit_test.go`) and `TestCatalogCoversSchema`: a test
|
||||
walks `backend/queue` for assignments to `currentIndex` (and the
|
||||
`SetQueue` / remove paths) and fails if a mutation site does not sit
|
||||
adjacent to a `leaveCurrent` call. The sweep is the enforcement; the
|
||||
central method is the convenience.
|
||||
|
||||
`recordPlay` keeps its existing contract *when a play happens* —
|
||||
`TrackPlayCountChanged` with `{audioFileId, filePath, playCount,
|
||||
lastPlayed}` — so the frontend patch path and
|
||||
`playhistory_test.go` keep passing. A skip emits no per-track event in
|
||||
v1 (open question 4).
|
||||
|
||||
---
|
||||
|
||||
## Phases
|
||||
|
||||
1. **The classifier.** `listen.go`: the constants, `playThreshold`,
|
||||
`tailWindow`, `classify`. Table-driven unit tests covering every
|
||||
cell of the tristate, the <30s exemption, the tail window on both a
|
||||
10:00 and a 2:00 track, and the clip at the 4:00 cap. No I/O.
|
||||
2. **Schema.** *Done in this session.* `listening_events` replaces
|
||||
`play_history`; `skip_count` / `last_skipped` added to
|
||||
`audio_files`; datamap entry and `TestAuthoredCascadesAreDeliberate`
|
||||
allow-list renamed; `recordPlay` writes `listening_events
|
||||
('complete')`. `make generate` run; database / datamap / queue
|
||||
tests green.
|
||||
3. **Wiring.** `leaveCurrent`, the navigation/finish/error call sites,
|
||||
the `fires once per listen` guard, and the source sweep. Extend
|
||||
`playhistory_test.go` for skip/complete classification through the
|
||||
queue rather than the pure function.
|
||||
4. **Smart-playlist field.** `skip_count` (and optionally
|
||||
`days_since_skipped`) in `smartplaylist.go` field/numeric maps and
|
||||
the editor's field list, via subquery. A frontend event for skip —
|
||||
if a UI wants a skip column — follows separately.
|
||||
|
||||
## Verification
|
||||
|
||||
- **Go:** the classifier is pure and exhaustively unit-tested; the
|
||||
queue wiring is tested in-process with `events.WithSink`
|
||||
(`backend/queue/emit_test.go` is the model), asserting a Next at 90%
|
||||
emits a *play*, a Next at 10% emits a *skip and no play*, a natural
|
||||
finish emits a *complete*.
|
||||
- **Database:** schema + datamap tests fail-loud on any new or
|
||||
reclassified table; `database_test.go`'s listening-events round-trip
|
||||
asserts the new table and the four denormalized counter columns.
|
||||
- **e2e:** `e2e/specs/play-count.spec.ts` already awaits
|
||||
`TrackPlayCountChanged`; add the skip case (advance early, assert no
|
||||
`TrackPlayCountChanged` and a `skip` row via the `__/test/sql`
|
||||
endpoint if convenient, or via the playlist effect).
|
||||
- No visual/component tier needed unless a skip column ships (phase 4).
|
||||
|
||||
## Open questions / decisions needed
|
||||
|
||||
1. **Migration mechanism.** Resolved — fresh design, no migration (see the schema section). Play counts are not worth preserving, both users are devs, and `audio_files` / `play_history` rebuild-or-drop on next launch.
|
||||
2. **"Position is not listen time."** Accept the approximation for v1,
|
||||
or track accumulated listen seconds (a real ledger on the player) now?
|
||||
3. **Abandon on shutdown.** A track paused at 70% and then app-killed:
|
||||
count a `play` (scrobble says heard) or leave it unrecorded? v1
|
||||
proposes *unrecorded* — same as today — to keep the write path off
|
||||
the shutdown critical path.
|
||||
4. **Skip event to the frontend.** Emit now (parallel to
|
||||
`TrackPlayCountChanged`) or only when a surface consumes it?
|
||||
@@ -172,7 +172,7 @@ make ui-test # Vitest component/store suite in a real browser (no app)
|
||||
make ui-visual # Same, including toMatchScreenshot comparisons
|
||||
make ui-setup # Install the Vitest provider's own Chromium (once)
|
||||
make bindings-check # Fail if frontend/bindings is stale vs the Go bindings
|
||||
make skill-check # Fail if .pi/ documents a make target that doesn't exist
|
||||
make skill-check # Fail if a doc names a make target that doesn't exist
|
||||
make commit-check # Fail if a commit subject is not a Conventional Commit
|
||||
make lint # golangci-lint v2 (strict), all three build configurations
|
||||
make test # All tests with race detector, all three build configurations
|
||||
@@ -673,6 +673,28 @@ rather than renaming them.
|
||||
one of the shell's rows, which is what the skip link is absolutely
|
||||
positioned to avoid.
|
||||
- `config` — TOML-based settings. Settings page uses HTMX + templ for server-rendered HTML fragments.
|
||||
|
||||
**A setter that can reject its argument puts the old value back**, and
|
||||
that is a correctness rule rather than hygiene (#231). `Save()`
|
||||
validates the *whole* config, so a value left behind by a failed write
|
||||
does not merely fail its own call: it fails every later save, of every
|
||||
unrelated setting — theme, launch page, shortcuts, libraries — for the
|
||||
rest of the session. Nothing reaches disk, so a restart clears it,
|
||||
which is exactly what makes the fault invisible and unreportable. One
|
||||
rejected track-list column list was enough to stop the app saving
|
||||
anything at all.
|
||||
|
||||
Two shapes are safe and a third is the trap. A setter that assigns and
|
||||
*then* validates snapshots the field first and restores it on the
|
||||
error path — seven do. `SetLibraryDirectory` is the better shape where
|
||||
the value can be built on its own: it validates a candidate *before*
|
||||
assigning, so there is nothing to undo. And a setter whose argument no
|
||||
validation inspects needs neither — the bools, the favourites playlist
|
||||
id and the shortcut bindings, plus `SetViewVisible`, which refuses an
|
||||
unknown, non-hideable or launch-page view up front so
|
||||
`GeneralConfig.Validate` never sees one it would fail on. Which set a
|
||||
new setter joins is decided by whether its own `Validate` can reject
|
||||
it, not by preference.
|
||||
- `playlist` / `smartplaylist` — Playlist CRUD and rule-based smart playlists.
|
||||
- `mediacontrols` — OS media controls behind one `Handler`: MPRIS over
|
||||
D-Bus on desktop Linux, a MediaSession on Android, a no-op stub
|
||||
|
||||
@@ -192,7 +192,7 @@ css-check: ## Fail on a css`` literal ended early by a backtick, or a nested rul
|
||||
# Every command in them is a make target on purpose, so this is
|
||||
# checkable. It also asserts AGENTS.md is a symlink to CLAUDE.md, so the
|
||||
# two harnesses cannot drift onto two descriptions of one project.
|
||||
skill-check: ## Fail if the agent docs name a missing make target, or AGENTS.md is not a symlink
|
||||
skill-check: ## Fail if the docs name a missing make target, or AGENTS.md is not a symlink
|
||||
@./scripts/skill-check.sh
|
||||
|
||||
# Conventional Commits, which CLAUDE.md claimed CI enforced for a long
|
||||
|
||||
@@ -303,6 +303,24 @@ func (c *Config) GetLibraryDirectory() string {
|
||||
return string(c.Library.DirectoryPath)
|
||||
}
|
||||
|
||||
// A rejected setter puts the old value back, and that is not tidiness
|
||||
// (#231). Save validates the *whole* config, so a value left behind by
|
||||
// a failed write does not merely fail its own call: it fails every
|
||||
// later save, of every unrelated setting, silently and for the rest of
|
||||
// the session. Nothing reaches disk, so a restart clears it -- which
|
||||
// is exactly what makes the fault hard to see and impossible to report.
|
||||
//
|
||||
// The setters below that assign and then validate therefore snapshot
|
||||
// the field first and restore it on the error path. SetLibraryDirectory
|
||||
// is the other safe shape and the better one where the value can be
|
||||
// built on its own: it validates a candidate *before* assigning
|
||||
// anything, so there is nothing to undo.
|
||||
//
|
||||
// Not every setter needs either. A bool, an int64 and the shortcut
|
||||
// bindings pass through no validation that can reject them, and
|
||||
// SetViewVisible refuses an unknown, non-hideable or launch-page view
|
||||
// up front, so GeneralConfig.Validate never sees one it would fail on.
|
||||
|
||||
// SetLibraryDirectory validates and saves a new library directory,
|
||||
// then emits the LibraryConfigChanged event so listeners (e.g. the
|
||||
// Library scanner) can react.
|
||||
@@ -360,11 +378,14 @@ func (c *Config) SetScanConcurrency(mode string) error {
|
||||
c.Library.ApplyDefaults()
|
||||
}
|
||||
|
||||
previous := c.Library.ScanConcurrency
|
||||
c.Library.ScanConcurrency = library.ScanConcurrency(
|
||||
mode,
|
||||
)
|
||||
|
||||
if err := c.Library.Validate(); err != nil {
|
||||
c.Library.ScanConcurrency = previous
|
||||
|
||||
return fmt.Errorf(
|
||||
"invalid scan concurrency mode: %w", err,
|
||||
)
|
||||
@@ -455,9 +476,12 @@ func (c *Config) SetThemeAccentColor(
|
||||
c.Theme.ApplyDefaults()
|
||||
}
|
||||
|
||||
previous := c.Theme.AccentColor
|
||||
c.Theme.AccentColor = color
|
||||
|
||||
if err := c.Theme.Validate(); err != nil {
|
||||
c.Theme.AccentColor = previous
|
||||
|
||||
return fmt.Errorf(
|
||||
"invalid theme accent color: %w", err,
|
||||
)
|
||||
@@ -488,9 +512,12 @@ func (c *Config) SetThemeBackgroundShade(
|
||||
c.Theme.ApplyDefaults()
|
||||
}
|
||||
|
||||
previous := c.Theme.BackgroundShade
|
||||
c.Theme.BackgroundShade = theme.BackgroundShade(shade)
|
||||
|
||||
if err := c.Theme.Validate(); err != nil {
|
||||
c.Theme.BackgroundShade = previous
|
||||
|
||||
return fmt.Errorf(
|
||||
"invalid theme background shade: %w", err,
|
||||
)
|
||||
@@ -544,9 +571,12 @@ func (c *Config) SetDefaultPage(page string) error {
|
||||
c.General.ApplyDefaults()
|
||||
}
|
||||
|
||||
previous := c.General.DefaultPage
|
||||
c.General.DefaultPage = View(page)
|
||||
|
||||
if err := c.General.Validate(); err != nil {
|
||||
c.General.DefaultPage = previous
|
||||
|
||||
return fmt.Errorf(
|
||||
"invalid default page: %w", err,
|
||||
)
|
||||
@@ -591,9 +621,12 @@ func (c *Config) SetQueueFallback(mode string) error {
|
||||
c.General.ApplyDefaults()
|
||||
}
|
||||
|
||||
previous := c.General.QueueFallback
|
||||
c.General.QueueFallback = QueueFallback(mode)
|
||||
|
||||
if err := c.General.Validate(); err != nil {
|
||||
c.General.QueueFallback = previous
|
||||
|
||||
return fmt.Errorf(
|
||||
"invalid queue fallback: %w", err,
|
||||
)
|
||||
@@ -801,9 +834,12 @@ func (c *Config) SetTrackListColumns(
|
||||
c.TrackList = &tracklist.Config{}
|
||||
}
|
||||
|
||||
previous := c.TrackList.Columns
|
||||
c.TrackList.Columns = columns
|
||||
|
||||
if err := c.TrackList.Validate(); err != nil {
|
||||
c.TrackList.Columns = previous
|
||||
|
||||
return fmt.Errorf(
|
||||
"invalid track-list columns: %w", err,
|
||||
)
|
||||
@@ -901,9 +937,12 @@ func (c *Config) SetFavoritesIconStyle(
|
||||
c.Favorites.ApplyDefaults()
|
||||
}
|
||||
|
||||
previous := c.Favorites.IconStyle
|
||||
c.Favorites.IconStyle = favorites.IconStyle(style)
|
||||
|
||||
if err := c.Favorites.Validate(); err != nil {
|
||||
c.Favorites.IconStyle = previous
|
||||
|
||||
return fmt.Errorf(
|
||||
"invalid favorites icon style: %w", err,
|
||||
)
|
||||
|
||||
@@ -0,0 +1,262 @@
|
||||
package config
|
||||
|
||||
import (
|
||||
"log/slog"
|
||||
"path/filepath"
|
||||
"testing"
|
||||
|
||||
"yellowjacket/backend/library"
|
||||
"yellowjacket/backend/tracklist"
|
||||
)
|
||||
|
||||
// newSavableConfig builds a loaded, valid config in a temp directory,
|
||||
// so Save() writes rather than refusing with errSaveBeforeLoad.
|
||||
//
|
||||
// The library directory is real and set, because Config.Validate only
|
||||
// validates the Library section when DirectoryPath is non-empty -- an
|
||||
// empty one would hide a poisoned ScanConcurrency from the whole-config
|
||||
// save that is the symptom under test.
|
||||
func newSavableConfig(t *testing.T) *Config {
|
||||
t.Helper()
|
||||
|
||||
c := &Config{
|
||||
logger: slog.Default(),
|
||||
filePath: filepath.Join(t.TempDir(), "config.toml"),
|
||||
Library: &library.Config{
|
||||
DirectoryPath: library.Directory(t.TempDir()),
|
||||
},
|
||||
}
|
||||
|
||||
c.applyDefaults()
|
||||
|
||||
if err := c.Load(); err != nil {
|
||||
t.Fatalf("Load() error: %v", err)
|
||||
}
|
||||
|
||||
if err := c.Save(); err != nil {
|
||||
t.Fatalf("Save() on a fresh config error: %v", err)
|
||||
}
|
||||
|
||||
return c
|
||||
}
|
||||
|
||||
// TestSetterRejectionDoesNotPoisonTheConfig is the whole of #231.
|
||||
//
|
||||
// Every setter here assigns to the in-memory config and then validates.
|
||||
// When the validation rejects the argument, the rejected value has to go
|
||||
// back -- not because the caller sees it (it gets an error either way),
|
||||
// but because Config.Save() validates the *whole* config. A value left
|
||||
// behind by a failed setter therefore fails every later save, of every
|
||||
// unrelated setting, silently and for the rest of the session.
|
||||
//
|
||||
// So each case asserts three things in order: the setter reports the
|
||||
// error, the getter still reports the old value, and an unrelated save
|
||||
// still works. The third is the one the user feels.
|
||||
func TestSetterRejectionDoesNotPoisonTheConfig(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
cases := []struct {
|
||||
name string
|
||||
// reject calls the setter with an argument its own Validate
|
||||
// refuses.
|
||||
reject func(*Config) error
|
||||
// read reports the value the setter writes, so the rollback is
|
||||
// asserted on the config rather than only on the save.
|
||||
read func(*Config) string
|
||||
}{
|
||||
{
|
||||
name: "scan concurrency",
|
||||
reject: func(c *Config) error {
|
||||
return c.SetScanConcurrency("telepathy")
|
||||
},
|
||||
read: (*Config).GetScanConcurrency,
|
||||
},
|
||||
{
|
||||
name: "theme accent colour",
|
||||
reject: func(c *Config) error {
|
||||
return c.SetThemeAccentColor("not-a-hex")
|
||||
},
|
||||
read: (*Config).GetThemeAccentColor,
|
||||
},
|
||||
{
|
||||
name: "theme background shade",
|
||||
reject: func(c *Config) error {
|
||||
return c.SetThemeBackgroundShade("chartreuse")
|
||||
},
|
||||
read: (*Config).GetThemeBackgroundShade,
|
||||
},
|
||||
{
|
||||
name: "default page",
|
||||
reject: func(c *Config) error {
|
||||
return c.SetDefaultPage("nowhere")
|
||||
},
|
||||
read: (*Config).GetDefaultPage,
|
||||
},
|
||||
{
|
||||
name: "queue fallback",
|
||||
reject: func(c *Config) error {
|
||||
return c.SetQueueFallback("improvise")
|
||||
},
|
||||
read: (*Config).GetQueueFallback,
|
||||
},
|
||||
{
|
||||
name: "favorites icon style",
|
||||
reject: func(c *Config) error {
|
||||
return c.SetFavoritesIconStyle("asterisk")
|
||||
},
|
||||
read: (*Config).GetFavoritesIconStyle,
|
||||
},
|
||||
{
|
||||
name: "track-list columns",
|
||||
reject: func(c *Config) error {
|
||||
// titleArtist is a drawing definition, not a
|
||||
// configurable column (#197), so it is exactly what
|
||||
// the frontend used to be able to send.
|
||||
return c.SetTrackListColumns([]tracklist.Column{
|
||||
{ID: "titleArtist"},
|
||||
})
|
||||
},
|
||||
read: func(c *Config) string {
|
||||
return columnIDs(c.GetTrackListColumns())
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "track-list columns, duplicated",
|
||||
reject: func(c *Config) error {
|
||||
// The route #197 closed was one invalid id; a
|
||||
// duplicate is the one still reachable from a client
|
||||
// that assembles the list itself.
|
||||
return c.SetTrackListColumns([]tracklist.Column{
|
||||
{ID: tracklist.ColTrackName},
|
||||
{ID: tracklist.ColTrackName},
|
||||
})
|
||||
},
|
||||
read: func(c *Config) string {
|
||||
return columnIDs(c.GetTrackListColumns())
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
for _, tc := range cases {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
c := newSavableConfig(t)
|
||||
before := tc.read(c)
|
||||
|
||||
if err := tc.reject(c); err == nil {
|
||||
t.Fatal("setter accepted an invalid value, want an error")
|
||||
}
|
||||
|
||||
if after := tc.read(c); after != before {
|
||||
t.Errorf(
|
||||
"value after a rejected write = %q, want the previous %q",
|
||||
after, before,
|
||||
)
|
||||
}
|
||||
|
||||
// The symptom: an unrelated setting can no longer be saved.
|
||||
if err := c.SetPopupVolume(true); err != nil {
|
||||
t.Errorf("an unrelated setter failed after a rejected write: %v", err)
|
||||
}
|
||||
|
||||
if err := c.Save(); err != nil {
|
||||
t.Errorf("Save() failed after a rejected write: %v", err)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestRejectedSetterLeavesNothingOnDisk pairs with the sweep above: the
|
||||
// rollback must not be undone by what the file already holds, so a
|
||||
// config reloaded from disk after a rejected write agrees with memory.
|
||||
func TestRejectedSetterLeavesNothingOnDisk(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
c := newSavableConfig(t)
|
||||
|
||||
if err := c.SetThemeAccentColor("#123456"); err != nil {
|
||||
t.Fatalf("SetThemeAccentColor() error: %v", err)
|
||||
}
|
||||
|
||||
if err := c.SetThemeAccentColor("not-a-hex"); err == nil {
|
||||
t.Fatal("SetThemeAccentColor accepted a non-colour, want an error")
|
||||
}
|
||||
|
||||
reloaded := &Config{logger: slog.Default(), filePath: c.filePath}
|
||||
reloaded.applyDefaults()
|
||||
|
||||
if err := reloaded.Load(); err != nil {
|
||||
t.Fatalf("Load() error: %v", err)
|
||||
}
|
||||
|
||||
if got := reloaded.GetThemeAccentColor(); got != "#123456" {
|
||||
t.Errorf("accent colour on disk = %q, want %q", got, "#123456")
|
||||
}
|
||||
|
||||
if c.GetThemeAccentColor() != reloaded.GetThemeAccentColor() {
|
||||
t.Errorf(
|
||||
"in-memory accent %q disagrees with disk %q after a rejected write",
|
||||
c.GetThemeAccentColor(), reloaded.GetThemeAccentColor(),
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
// TestSetLibraryDirectoryValidatesBeforeAssigning pins the precedent the
|
||||
// seven rolled-back setters follow: this one has always built and
|
||||
// validated a candidate before assigning, so a bad path never reaches
|
||||
// the config at all.
|
||||
func TestSetLibraryDirectoryValidatesBeforeAssigning(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
c := newSavableConfig(t)
|
||||
before := c.GetLibraryDirectory()
|
||||
|
||||
if err := c.SetLibraryDirectory(filepath.Join(t.TempDir(), "no-such-dir")); err == nil {
|
||||
t.Fatal("SetLibraryDirectory accepted a missing directory, want an error")
|
||||
}
|
||||
|
||||
if after := c.GetLibraryDirectory(); after != before {
|
||||
t.Errorf("library directory = %q, want the previous %q", after, before)
|
||||
}
|
||||
|
||||
if err := c.Save(); err != nil {
|
||||
t.Errorf("Save() failed after a rejected library directory: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
// TestSetViewVisibleRefusesBeforeAssigning covers the other setter left
|
||||
// out of the rollback pass: it guards its own argument up front, so
|
||||
// GeneralConfig.Validate never sees a view it would reject.
|
||||
func TestSetViewVisibleRefusesBeforeAssigning(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
c := newSavableConfig(t)
|
||||
|
||||
if err := c.SetViewVisible("no-such-view", false); err == nil {
|
||||
t.Fatal("SetViewVisible accepted an unknown view, want an error")
|
||||
}
|
||||
|
||||
if err := c.SetViewVisible(c.GetDefaultPage(), false); err == nil {
|
||||
t.Fatal("SetViewVisible hid the launch page, want an error")
|
||||
}
|
||||
|
||||
if err := c.Save(); err != nil {
|
||||
t.Errorf("Save() failed after a refused view visibility change: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
// columnIDs renders a column list for comparison in the table above.
|
||||
func columnIDs(cols []tracklist.Column) string {
|
||||
ids := make([]byte, 0, len(cols)*8)
|
||||
|
||||
for i, col := range cols {
|
||||
if i > 0 {
|
||||
ids = append(ids, ',')
|
||||
}
|
||||
|
||||
ids = append(ids, col.ID...)
|
||||
}
|
||||
|
||||
return string(ids)
|
||||
}
|
||||
@@ -662,19 +662,19 @@ func TestSmartPlaylistColumns(t *testing.T) {
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Migration 10 — play history tracking
|
||||
// Listening events tracking
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
func TestPlayHistoryTable(t *testing.T) {
|
||||
func TestListeningEventsTable(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
db := NewTestDB(t)
|
||||
|
||||
// Verify play_history table exists.
|
||||
// Verify listening_events table exists.
|
||||
var tableCount int64
|
||||
|
||||
tblRows, err := db.QueryContext(
|
||||
"SELECT COUNT(*) FROM sqlite_master WHERE type='table' AND name='play_history'",
|
||||
"SELECT COUNT(*) FROM sqlite_master WHERE type='table' AND name='listening_events'",
|
||||
)
|
||||
if err != nil {
|
||||
t.Fatalf("query sqlite_master: %v", err)
|
||||
@@ -695,12 +695,14 @@ func TestPlayHistoryTable(t *testing.T) {
|
||||
_ = tblRows.Close()
|
||||
|
||||
if tableCount != 1 {
|
||||
t.Errorf("play_history table count = %d, want 1", tableCount)
|
||||
t.Errorf("listening_events table count = %d, want 1", tableCount)
|
||||
}
|
||||
|
||||
// Verify audio_files has play_count and last_played columns.
|
||||
// Verify audio_files has the denormalized listening counters.
|
||||
hasPlayCount := false
|
||||
hasLastPlayed := false
|
||||
hasSkipCount := false
|
||||
hasLastSkipped := false
|
||||
|
||||
colRows, err := db.QueryContext("PRAGMA table_info(audio_files)")
|
||||
if err != nil {
|
||||
@@ -732,6 +734,14 @@ func TestPlayHistoryTable(t *testing.T) {
|
||||
if name == "last_played" {
|
||||
hasLastPlayed = true
|
||||
}
|
||||
|
||||
if name == "skip_count" {
|
||||
hasSkipCount = true
|
||||
}
|
||||
|
||||
if name == "last_skipped" {
|
||||
hasLastSkipped = true
|
||||
}
|
||||
}
|
||||
|
||||
_ = colRows.Close()
|
||||
@@ -744,6 +754,14 @@ func TestPlayHistoryTable(t *testing.T) {
|
||||
t.Error("audio_files missing last_played column")
|
||||
}
|
||||
|
||||
if !hasSkipCount {
|
||||
t.Error("audio_files missing skip_count column")
|
||||
}
|
||||
|
||||
if !hasLastSkipped {
|
||||
t.Error("audio_files missing last_skipped column")
|
||||
}
|
||||
|
||||
// Verify track_metadata VIEW includes play_count and last_played.
|
||||
viewCols := map[string]bool{}
|
||||
|
||||
@@ -783,7 +801,7 @@ func TestPlayHistoryTable(t *testing.T) {
|
||||
t.Error("track_metadata VIEW missing last_played column")
|
||||
}
|
||||
|
||||
// Round-trip: insert a play_history row and verify play_count update.
|
||||
// Round-trip: insert a listening_events row and verify play_count update.
|
||||
// First, set up test data. The test DB already has library id=0.
|
||||
InsertTestTrack(t, db, TestTrack{
|
||||
FilePath: "/test/play_history.mp3",
|
||||
@@ -821,12 +839,12 @@ func TestPlayHistoryTable(t *testing.T) {
|
||||
t.Errorf("initial play_count = %d, want 0", playCount)
|
||||
}
|
||||
|
||||
// Insert a play_history row and update play_count.
|
||||
// Insert a listening_events row (kind defaults to 'complete').
|
||||
_, err = db.ExecContext(
|
||||
"INSERT INTO play_history (audio_file_id) VALUES (1)",
|
||||
"INSERT INTO listening_events (audio_file_id, kind) VALUES (1, 'complete')",
|
||||
)
|
||||
if err != nil {
|
||||
t.Fatalf("insert play_history: %v", err)
|
||||
t.Fatalf("insert listening_events: %v", err)
|
||||
}
|
||||
|
||||
_, err = db.ExecContext(
|
||||
|
||||
@@ -0,0 +1,177 @@
|
||||
package database
|
||||
|
||||
import (
|
||||
"context"
|
||||
"database/sql"
|
||||
"fmt"
|
||||
"log/slog"
|
||||
"strings"
|
||||
)
|
||||
|
||||
// Preserving a playlist entry across the loss of its track is two
|
||||
// statements, not one, and the split is not tidiness -- it is what
|
||||
// makes the important half work in the situation that needs it most.
|
||||
//
|
||||
// `playlist_tracks.audio_file_id` is ON DELETE SET NULL, so an entry
|
||||
// outlives its file as an id-less row that says nothing about what the
|
||||
// user put in the playlist. The phantom_* columns carry the answer
|
||||
// across and ResolvePhantomTracksAfterScan re-links them afterwards --
|
||||
// but only if something fills them *before* the rows go.
|
||||
//
|
||||
// The two halves are not equally important and are not equally
|
||||
// available:
|
||||
//
|
||||
// - **phantom_file_path is the one that matters.**
|
||||
// ResolvePhantomTracksAfterScan matches it against
|
||||
// `audio_files.file_path`, so without it an entry can never be
|
||||
// re-linked and the playlist is empty for good. It comes straight
|
||||
// off `audio_files`, whose `file_path` is the table's natural key
|
||||
// and has been present in every shape it has ever had -- including
|
||||
// the pre-013 stub of `(id, file_path, recording_id)`.
|
||||
// - The rest is *display* for a phantom entry before a rescan
|
||||
// re-links it, and it comes from the `track_metadata` view, which
|
||||
// is the one definition of a track row and not worth restating.
|
||||
//
|
||||
// Reading the view is what cannot be relied on here, and that is the
|
||||
// whole reason for the split. This runs *before* applySchema, which is
|
||||
// precisely the moment the schema is inconsistent: the view is whatever
|
||||
// the last launch's schema declared, while `audio_files` is whatever
|
||||
// the launch before that left behind. A view over columns the table no
|
||||
// longer has is not merely empty -- `pragma_table_info` on it *errors*,
|
||||
// and so does selecting from it. `cmd/indexbuild`'s fixture is exactly
|
||||
// that shape and is what caught this.
|
||||
//
|
||||
// COALESCE keeps an existing phantom value in both halves: an entry
|
||||
// already phantom is one whose file went missing in an earlier pass,
|
||||
// and its recorded metadata is the only copy left. Overwriting that
|
||||
// from a NULL join erases the rows this exists to protect.
|
||||
const (
|
||||
preservePhantomPathSQL = `
|
||||
UPDATE playlist_tracks
|
||||
SET phantom_file_path = COALESCE(phantom_file_path, (
|
||||
SELECT af.file_path FROM audio_files af
|
||||
WHERE af.id = playlist_tracks.audio_file_id
|
||||
))
|
||||
WHERE audio_file_id IS NOT NULL
|
||||
`
|
||||
|
||||
preservePhantomDisplaySQL = `
|
||||
UPDATE playlist_tracks
|
||||
SET
|
||||
phantom_title = COALESCE(phantom_title, (
|
||||
SELECT tm.title FROM track_metadata tm
|
||||
WHERE tm.id = playlist_tracks.audio_file_id
|
||||
)),
|
||||
phantom_artist = COALESCE(phantom_artist, (
|
||||
SELECT tm.artist_name FROM track_metadata tm
|
||||
WHERE tm.id = playlist_tracks.audio_file_id
|
||||
)),
|
||||
phantom_album = COALESCE(phantom_album, (
|
||||
SELECT tm.album FROM track_metadata tm
|
||||
WHERE tm.id = playlist_tracks.audio_file_id
|
||||
)),
|
||||
phantom_duration_ms = COALESCE(phantom_duration_ms, (
|
||||
SELECT af.length_milliseconds FROM audio_files af
|
||||
WHERE af.id = playlist_tracks.audio_file_id
|
||||
)),
|
||||
phantom_genre = COALESCE(phantom_genre, (
|
||||
SELECT tm.genre FROM track_metadata tm
|
||||
WHERE tm.id = playlist_tracks.audio_file_id
|
||||
)),
|
||||
phantom_cover_art_path = COALESCE(phantom_cover_art_path, (
|
||||
SELECT tm.cover_art_path FROM track_metadata tm
|
||||
WHERE tm.id = playlist_tracks.audio_file_id
|
||||
))
|
||||
WHERE audio_file_id IS NOT NULL
|
||||
`
|
||||
)
|
||||
|
||||
// PreservePlaylistPhantoms records every linked playlist entry's track
|
||||
// metadata on the entry itself, so the entry survives the rows being
|
||||
// deleted underneath it.
|
||||
//
|
||||
// Every path that empties `audio_files` must call this (or the scoped
|
||||
// variant below) first, inside the same transaction as the delete.
|
||||
// These paths have drifted before: the full rescan in backend/library
|
||||
// did this and the stale-shape retire in this package did not, so the
|
||||
// *documented* repair ("delete and rescan") preserved playlists while
|
||||
// the automatic one that exists to spare the user that work silently
|
||||
// emptied them (#183). The incremental scan's orphan cleanup and
|
||||
// RemoveFromLibrary drifted the same way and are #246.
|
||||
//
|
||||
// The display half is skipped, with a warning, when `track_metadata`
|
||||
// cannot answer -- see the note above. Skipping it costs a phantom
|
||||
// entry its title until a rescan re-links it; skipping the path half
|
||||
// would cost the entry outright, so that one is an error.
|
||||
func PreservePlaylistPhantoms(
|
||||
ctx context.Context, tx *sql.Tx, logger *slog.Logger,
|
||||
) error {
|
||||
return preservePlaylistPhantoms(ctx, tx, nil, logger)
|
||||
}
|
||||
|
||||
// PreservePlaylistPhantomsForFiles is PreservePlaylistPhantoms scoped to
|
||||
// the given audio file ids, for the two removal paths that delete a
|
||||
// known subset of the table rather than all of it: the incremental
|
||||
// scan's orphan cleanup and RemoveFromLibrary. A bulk pass there would
|
||||
// rewrite every linked playlist row on every scan for nothing.
|
||||
func PreservePlaylistPhantomsForFiles(
|
||||
ctx context.Context, tx *sql.Tx, ids []int64, logger *slog.Logger,
|
||||
) error {
|
||||
return preservePlaylistPhantoms(ctx, tx, ids, logger)
|
||||
}
|
||||
|
||||
func preservePlaylistPhantoms(
|
||||
ctx context.Context, tx *sql.Tx, ids []int64, logger *slog.Logger,
|
||||
) error {
|
||||
clause, args, skip := phantomIDFilter(ids)
|
||||
if skip {
|
||||
return nil
|
||||
}
|
||||
|
||||
if _, err := tx.ExecContext(
|
||||
ctx, preservePhantomPathSQL+clause, args...,
|
||||
); err != nil {
|
||||
return fmt.Errorf(
|
||||
"could not preserve playlist track file paths: %w", err,
|
||||
)
|
||||
}
|
||||
|
||||
if _, err := tx.ExecContext(
|
||||
ctx, preservePhantomDisplaySQL+clause, args...,
|
||||
); err != nil {
|
||||
// A failed statement does not roll back a SQLite transaction,
|
||||
// so the path half above stands and the entries remain
|
||||
// re-linkable.
|
||||
logger.Warn(
|
||||
"could not record display metadata for playlist entries; "+
|
||||
"they will be re-linked by the next scan but read as "+
|
||||
"unknown until then",
|
||||
"err", err,
|
||||
)
|
||||
}
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
// phantomIDFilter builds the extra WHERE terms and arguments that scope
|
||||
// a preservation pass to a set of audio file ids. A nil ids returns the
|
||||
// empty clause (a bulk run over every linked entry); an empty slice
|
||||
// reports skip, since there is nothing to preserve.
|
||||
func phantomIDFilter(ids []int64) (clause string, args []any, skip bool) {
|
||||
switch {
|
||||
case ids == nil:
|
||||
return "", nil, false
|
||||
case len(ids) == 0:
|
||||
return "", nil, true
|
||||
}
|
||||
|
||||
clause = " AND audio_file_id IN (" +
|
||||
strings.Repeat("?,", len(ids)-1) + "?)"
|
||||
|
||||
args = make([]any, len(ids))
|
||||
for i, id := range ids {
|
||||
args[i] = id
|
||||
}
|
||||
|
||||
return clause, args, false
|
||||
}
|
||||
@@ -0,0 +1,94 @@
|
||||
package database
|
||||
|
||||
import (
|
||||
"context"
|
||||
"database/sql"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// TestPreservePlaylistPhantomsForFilesScopesToTheRequestedIDs is the
|
||||
// scoping half of the scoped variant: a run over one file's id must
|
||||
// fill that file's playlist entries and leave every other entry alone,
|
||||
// because the incremental scan calls this once per orphan batch and a
|
||||
// pass that rewrote the whole table would touch every playlist row on
|
||||
// every scan.
|
||||
func TestPreservePlaylistPhantomsForFilesScopesToTheRequestedIDs(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
db := openRaw(t, t.TempDir())
|
||||
|
||||
if _, err := db.ExecContext(ctx, "PRAGMA foreign_keys = ON"); err != nil {
|
||||
t.Fatalf("pragma: %v", err)
|
||||
}
|
||||
|
||||
if err := applySchema(ctx, db); err != nil {
|
||||
t.Fatalf("applySchema: %v", err)
|
||||
}
|
||||
|
||||
if _, err := db.ExecContext(ctx, `
|
||||
INSERT INTO playlists (id, name) VALUES (1, 'keepme');
|
||||
INSERT INTO libraries (id, name, path) VALUES (0, 'test', '/music');
|
||||
INSERT INTO artists (id, name) VALUES (3, 'Aurora Fields');
|
||||
INSERT INTO cover_art (id, file_path, mime_type)
|
||||
VALUES (9, 'covers/7.jpg', 'image/jpeg');
|
||||
INSERT INTO albums (id, name, artist_id, cover_art_id)
|
||||
VALUES (4, 'Tideline', 3, 9);
|
||||
INSERT INTO audio_files
|
||||
(id, file_path, file_type_id, length_milliseconds,
|
||||
title, artist_credit, artist_id, album_id)
|
||||
VALUES
|
||||
(7, '/music/a.flac', 1, 1000,
|
||||
'Slack Water', 'Aurora Fields', 3, 4),
|
||||
(8, '/music/b.flac', 1, 2000,
|
||||
'Second Tide', 'Aurora Fields', 3, 4);
|
||||
INSERT INTO playlist_tracks (playlist_id, audio_file_id, position)
|
||||
VALUES (1, 7, 0), (1, 8, 1);
|
||||
`); err != nil {
|
||||
t.Fatalf("seed: %v", err)
|
||||
}
|
||||
|
||||
tx, err := db.BeginTx(ctx, nil)
|
||||
if err != nil {
|
||||
t.Fatalf("begin: %v", err)
|
||||
}
|
||||
|
||||
defer func() { _ = tx.Rollback() }()
|
||||
|
||||
if err := PreservePlaylistPhantomsForFiles(
|
||||
ctx, tx, []int64{7}, testLogger(),
|
||||
); err != nil {
|
||||
t.Fatalf("preserve: %v", err)
|
||||
}
|
||||
|
||||
if err := tx.Commit(); err != nil {
|
||||
t.Fatalf("commit: %v", err)
|
||||
}
|
||||
|
||||
var filled, untouched sql.NullString
|
||||
|
||||
if err := db.QueryRowContext(ctx,
|
||||
"SELECT phantom_file_path FROM playlist_tracks WHERE audio_file_id = 7",
|
||||
).Scan(&filled); err != nil {
|
||||
t.Fatalf("read the requested entry: %v", err)
|
||||
}
|
||||
|
||||
if filled.String != "/music/a.flac" {
|
||||
t.Errorf(
|
||||
"requested entry phantom_file_path = %q, want %q",
|
||||
filled.String, "/music/a.flac",
|
||||
)
|
||||
}
|
||||
|
||||
if err := db.QueryRowContext(ctx,
|
||||
"SELECT phantom_file_path FROM playlist_tracks WHERE audio_file_id = 8",
|
||||
).Scan(&untouched); err != nil {
|
||||
t.Fatalf("read the untouched entry: %v", err)
|
||||
}
|
||||
|
||||
if untouched.Valid {
|
||||
t.Errorf(
|
||||
"untouched entry got phantom_file_path = %q, want NULL "+
|
||||
"(a scoped run must not rewrite the whole table)",
|
||||
untouched.String,
|
||||
)
|
||||
}
|
||||
}
|
||||
@@ -68,8 +68,14 @@ CREATE TABLE IF NOT EXISTS audio_files (
|
||||
-- compared against the on-disk mtime during a scan to detect files
|
||||
-- another application retagged in place.
|
||||
modified_at INTEGER NOT NULL DEFAULT 0,
|
||||
-- Listening counts, denormalized from listening_events so the hot
|
||||
-- read path (track list sort, shelves, smart playlists) never joins
|
||||
-- a log table. Authored: a rescan cannot rebuild them. This is the
|
||||
-- "MIXED KIND" half of audio_files the datamap notes.
|
||||
play_count INTEGER NOT NULL DEFAULT 0,
|
||||
last_played DATETIME,
|
||||
skip_count INTEGER NOT NULL DEFAULT 0,
|
||||
last_skipped DATETIME,
|
||||
tag_status TEXT NOT NULL DEFAULT 'untagged'
|
||||
CHECK(tag_status IN (
|
||||
'untagged', 'auto_matched', 'user_confirmed', 'user_skipped_permanent'
|
||||
|
||||
@@ -45,8 +45,6 @@ CREATE INDEX IF NOT EXISTS idx_download_items_live
|
||||
CREATE INDEX IF NOT EXISTS idx_download_items_state
|
||||
ON download_items(state);
|
||||
|
||||
-- idx_download_items_download is deliberately NOT declared here: on an
|
||||
-- existing database this table already exists at schema-pass time with
|
||||
-- its old column still named request_id, so an inline CREATE INDEX on
|
||||
-- download_id would fail outright. See ensureDownloadIndexes in
|
||||
-- backend/database/download_rename_migration.go.
|
||||
-- ListDownloadItemsForDownload filters on the parent download.
|
||||
CREATE INDEX IF NOT EXISTS idx_download_items_download
|
||||
ON download_items(download_id);
|
||||
|
||||
@@ -66,14 +66,11 @@ CREATE TABLE IF NOT EXISTS download_requests (
|
||||
FOREIGN KEY(parent_id) REFERENCES download_requests(id) ON DELETE CASCADE
|
||||
);
|
||||
|
||||
-- idx_download_requests_{due,entity,parent} are deliberately NOT
|
||||
-- declared here. This table name is reused from the old one-shot
|
||||
-- attempt table (also called download_requests before the Want/Request
|
||||
-- rename), so on an existing database this CREATE TABLE is a no-op
|
||||
-- against a table that, at schema-pass time, is still shaped like the
|
||||
-- OLD attempts table and lacks these columns entirely — an inline
|
||||
-- CREATE INDEX here would fail outright rather than just no-op. See
|
||||
-- migrateDownloadRename/ensureDownloadIndexes in
|
||||
-- backend/database/download_rename_migration.go, which create these
|
||||
-- once the rename has actually happened (or immediately, on a fresh
|
||||
-- database where the columns exist from the start).
|
||||
CREATE INDEX IF NOT EXISTS idx_download_requests_due
|
||||
ON download_requests(state, next_try_at);
|
||||
|
||||
CREATE INDEX IF NOT EXISTS idx_download_requests_entity
|
||||
ON download_requests(entity, state);
|
||||
|
||||
CREATE INDEX IF NOT EXISTS idx_download_requests_parent
|
||||
ON download_requests(parent_id) WHERE parent_id IS NOT NULL;
|
||||
|
||||
@@ -0,0 +1,42 @@
|
||||
-- One row per track *exit*, three ways a listen can end: it reached
|
||||
-- the end, it was heard enough to count and then skipped past, or it
|
||||
-- was abandoned for another track before anyone had really listened.
|
||||
--
|
||||
-- This is the source of truth for listening behaviour. The
|
||||
-- denormalized `play_count` / `last_played` / `skip_count` /
|
||||
-- `last_skipped` on audio_files are materialized from it, because the
|
||||
-- hot read path (track-list sort, the shelves, smart playlists) must
|
||||
-- not join a log that grows by one row per song forever.
|
||||
--
|
||||
-- `kind` is the classification, applied at write time:
|
||||
--
|
||||
-- complete the track reached its natural end, or was skipped in
|
||||
-- its tail window (the last few seconds of a long fade).
|
||||
-- play the scrobble threshold was heard — half the track or
|
||||
-- four minutes, whichever is less — and the user moved on
|
||||
-- before the end.
|
||||
-- skip the user moved to a different track before that.
|
||||
--
|
||||
-- `position_seconds` / `duration_seconds` are the raw reading the
|
||||
-- classification was made from, kept so a future re-tune of the
|
||||
-- threshold does not need the events re-recorded. 0/0 on a row means
|
||||
-- "not captured for this event" (e.g. a natural finish recorded before
|
||||
-- these columns existed), not "a zero-second track".
|
||||
CREATE TABLE IF NOT EXISTS listening_events (
|
||||
id INTEGER PRIMARY KEY,
|
||||
audio_file_id INTEGER NOT NULL,
|
||||
kind TEXT NOT NULL DEFAULT 'complete'
|
||||
CHECK (kind IN ('complete', 'play', 'skip')),
|
||||
position_seconds INTEGER NOT NULL DEFAULT 0,
|
||||
duration_seconds INTEGER NOT NULL DEFAULT 0,
|
||||
occurred_at DATETIME NOT NULL DEFAULT (datetime('now')),
|
||||
FOREIGN KEY(audio_file_id) REFERENCES audio_files(id) ON DELETE CASCADE
|
||||
);
|
||||
|
||||
CREATE INDEX IF NOT EXISTS idx_listening_events_audio_file_id
|
||||
ON listening_events(audio_file_id);
|
||||
|
||||
-- "What did I listen to this month" walks this, rather than the
|
||||
-- per-track index above.
|
||||
CREATE INDEX IF NOT EXISTS idx_listening_events_occurred_at
|
||||
ON listening_events(occurred_at);
|
||||
@@ -1,9 +0,0 @@
|
||||
CREATE TABLE IF NOT EXISTS play_history (
|
||||
id INTEGER PRIMARY KEY,
|
||||
audio_file_id INTEGER NOT NULL,
|
||||
played_at DATETIME NOT NULL DEFAULT (datetime('now')),
|
||||
FOREIGN KEY(audio_file_id) REFERENCES audio_files(id) ON DELETE CASCADE
|
||||
);
|
||||
|
||||
CREATE INDEX IF NOT EXISTS idx_play_history_audio_file_id
|
||||
ON play_history(audio_file_id);
|
||||
@@ -1,18 +1,15 @@
|
||||
CREATE TABLE IF NOT EXISTS queue (
|
||||
id INTEGER PRIMARY KEY CHECK(id = 1),
|
||||
source_playlist_id INTEGER,
|
||||
current_position INTEGER NOT NULL DEFAULT 0,
|
||||
shuffle_mode BOOLEAN NOT NULL DEFAULT false,
|
||||
repeat_mode TEXT NOT NULL DEFAULT 'off',
|
||||
shuffle_order TEXT,
|
||||
-- source_playlist_id above is unused dead weight (nothing has ever
|
||||
-- written it a nonzero value); source_type/source_id/source_label
|
||||
-- below are its generalized replacement, covering albums, playlists,
|
||||
-- smart playlists, genres and artists rather than playlists alone.
|
||||
-- What the queue was built from ("Playing from: X"): an album,
|
||||
-- playlist, smart playlist, genre or artist, identified by the id
|
||||
-- that source_type's namespace gives it.
|
||||
source_type TEXT NOT NULL DEFAULT '',
|
||||
source_id INTEGER NOT NULL DEFAULT 0,
|
||||
source_label TEXT NOT NULL DEFAULT '',
|
||||
FOREIGN KEY(source_playlist_id) REFERENCES playlists(id) ON DELETE SET NULL
|
||||
source_label TEXT NOT NULL DEFAULT ''
|
||||
);
|
||||
|
||||
-- Singleton row: there is exactly one playback queue.
|
||||
|
||||
@@ -26,17 +26,10 @@ CREATE TABLE IF NOT EXISTS tagging_items (
|
||||
-- complete rip of their own directory. parent_group_key is the
|
||||
-- original folder group they were split from.
|
||||
--
|
||||
-- These two columns are declared LAST, after created_at, even
|
||||
-- though that reads oddly next to the rest of the table: sql/
|
||||
-- migrations/0001 brings a pre-existing tagging_items up to date
|
||||
-- with `ALTER TABLE ADD COLUMN`, which SQLite always appends at
|
||||
-- the end of the column list. A fresh install (this file) and an
|
||||
-- upgraded database (this file + the migration) must end up with
|
||||
-- IDENTICAL column order, because sqlc-generated `SELECT *` scans
|
||||
-- (e.g. GetTaggingItem) bind columns positionally — see the
|
||||
-- schema/migration column-order test in database_test.go. Put
|
||||
-- new columns wherever reads best when adding a table for the
|
||||
-- first time; append-only from the second migration on.
|
||||
-- These columns are appended after created_at rather than grouped
|
||||
-- with the rest of the row: sqlc's `SELECT *` scans (GetTaggingItem)
|
||||
-- bind column order positionally, so new columns always go at the
|
||||
-- end.
|
||||
synthetic INTEGER NOT NULL DEFAULT 0,
|
||||
parent_group_key TEXT NOT NULL DEFAULT '',
|
||||
-- album_artist_conflict latches to 1 the first time two tracks
|
||||
@@ -58,10 +51,3 @@ CREATE INDEX IF NOT EXISTS idx_tagging_items_library_status
|
||||
|
||||
CREATE INDEX IF NOT EXISTS idx_tagging_items_status_pending
|
||||
ON tagging_items(library_id) WHERE status = 'pending';
|
||||
|
||||
-- idx_tagging_items_parent_group_key is NOT declared here on
|
||||
-- purpose: this file runs unconditionally, before migrations, even
|
||||
-- against a database that hasn't run 0001 yet — an index predicate
|
||||
-- referencing parent_group_key would fail on that table. It lives
|
||||
-- solely in sql/migrations/0001_tagging_items_synthetic.sql, which
|
||||
-- runs after the column exists either way (see database.go).
|
||||
|
||||
@@ -39,7 +39,7 @@ INSERT INTO audio_files (
|
||||
?, ?, ?, ?, ?, ?,
|
||||
?, ?, ?, ?, ?
|
||||
)
|
||||
RETURNING id, file_path, library_id, file_type_id, length_milliseconds, sample_rate, bit_depth, channels, bitrate, file_size, title, artist_credit, artist_id, album_id, track_number, disc_number, total_tracks, year, composer, comment, recording_mbid, basename, group_key, modified_at, play_count, last_played, tag_status
|
||||
RETURNING id, file_path, library_id, file_type_id, length_milliseconds, sample_rate, bit_depth, channels, bitrate, file_size, title, artist_credit, artist_id, album_id, track_number, disc_number, total_tracks, year, composer, comment, recording_mbid, basename, group_key, modified_at, play_count, last_played, skip_count, last_skipped, tag_status
|
||||
`
|
||||
|
||||
type CreateAudioFileParams struct {
|
||||
@@ -135,6 +135,8 @@ func (q *Queries) CreateAudioFile(ctx context.Context, arg CreateAudioFileParams
|
||||
&i.ModifiedAt,
|
||||
&i.PlayCount,
|
||||
&i.LastPlayed,
|
||||
&i.SkipCount,
|
||||
&i.LastSkipped,
|
||||
&i.TagStatus,
|
||||
)
|
||||
return i, err
|
||||
@@ -192,7 +194,7 @@ func (q *Queries) GetAllAudioFilePaths(ctx context.Context) ([]GetAllAudioFilePa
|
||||
|
||||
const getAudioFile = `-- name: GetAudioFile :one
|
||||
|
||||
SELECT id, file_path, library_id, file_type_id, length_milliseconds, sample_rate, bit_depth, channels, bitrate, file_size, title, artist_credit, artist_id, album_id, track_number, disc_number, total_tracks, year, composer, comment, recording_mbid, basename, group_key, modified_at, play_count, last_played, tag_status FROM audio_files WHERE id = ? LIMIT 1
|
||||
SELECT id, file_path, library_id, file_type_id, length_milliseconds, sample_rate, bit_depth, channels, bitrate, file_size, title, artist_credit, artist_id, album_id, track_number, disc_number, total_tracks, year, composer, comment, recording_mbid, basename, group_key, modified_at, play_count, last_played, skip_count, last_skipped, tag_status FROM audio_files WHERE id = ? LIMIT 1
|
||||
`
|
||||
|
||||
// ---------------------------------------------------------------------
|
||||
@@ -228,13 +230,15 @@ func (q *Queries) GetAudioFile(ctx context.Context, id int64) (AudioFile, error)
|
||||
&i.ModifiedAt,
|
||||
&i.PlayCount,
|
||||
&i.LastPlayed,
|
||||
&i.SkipCount,
|
||||
&i.LastSkipped,
|
||||
&i.TagStatus,
|
||||
)
|
||||
return i, err
|
||||
}
|
||||
|
||||
const getAudioFileByPath = `-- name: GetAudioFileByPath :one
|
||||
SELECT id, file_path, library_id, file_type_id, length_milliseconds, sample_rate, bit_depth, channels, bitrate, file_size, title, artist_credit, artist_id, album_id, track_number, disc_number, total_tracks, year, composer, comment, recording_mbid, basename, group_key, modified_at, play_count, last_played, tag_status FROM audio_files WHERE file_path = ? LIMIT 1
|
||||
SELECT id, file_path, library_id, file_type_id, length_milliseconds, sample_rate, bit_depth, channels, bitrate, file_size, title, artist_credit, artist_id, album_id, track_number, disc_number, total_tracks, year, composer, comment, recording_mbid, basename, group_key, modified_at, play_count, last_played, skip_count, last_skipped, tag_status FROM audio_files WHERE file_path = ? LIMIT 1
|
||||
`
|
||||
|
||||
func (q *Queries) GetAudioFileByPath(ctx context.Context, filePath string) (AudioFile, error) {
|
||||
@@ -267,6 +271,8 @@ func (q *Queries) GetAudioFileByPath(ctx context.Context, filePath string) (Audi
|
||||
&i.ModifiedAt,
|
||||
&i.PlayCount,
|
||||
&i.LastPlayed,
|
||||
&i.SkipCount,
|
||||
&i.LastSkipped,
|
||||
&i.TagStatus,
|
||||
)
|
||||
return i, err
|
||||
@@ -334,7 +340,7 @@ func (q *Queries) GetAudioFilesByPaths(ctx context.Context, paths []string) ([]G
|
||||
}
|
||||
|
||||
const getAudioFilesInLibrary = `-- name: GetAudioFilesInLibrary :many
|
||||
SELECT id, file_path, library_id, file_type_id, length_milliseconds, sample_rate, bit_depth, channels, bitrate, file_size, title, artist_credit, artist_id, album_id, track_number, disc_number, total_tracks, year, composer, comment, recording_mbid, basename, group_key, modified_at, play_count, last_played, tag_status FROM audio_files WHERE library_id = ?
|
||||
SELECT id, file_path, library_id, file_type_id, length_milliseconds, sample_rate, bit_depth, channels, bitrate, file_size, title, artist_credit, artist_id, album_id, track_number, disc_number, total_tracks, year, composer, comment, recording_mbid, basename, group_key, modified_at, play_count, last_played, skip_count, last_skipped, tag_status FROM audio_files WHERE library_id = ?
|
||||
`
|
||||
|
||||
func (q *Queries) GetAudioFilesInLibrary(ctx context.Context, libraryID int64) ([]AudioFile, error) {
|
||||
@@ -373,6 +379,8 @@ func (q *Queries) GetAudioFilesInLibrary(ctx context.Context, libraryID int64) (
|
||||
&i.ModifiedAt,
|
||||
&i.PlayCount,
|
||||
&i.LastPlayed,
|
||||
&i.SkipCount,
|
||||
&i.LastSkipped,
|
||||
&i.TagStatus,
|
||||
); err != nil {
|
||||
return nil, err
|
||||
|
||||
@@ -94,6 +94,8 @@ type AudioFile struct {
|
||||
ModifiedAt int64
|
||||
PlayCount int64
|
||||
LastPlayed sql.NullTime
|
||||
SkipCount int64
|
||||
LastSkipped sql.NullTime
|
||||
TagStatus string
|
||||
}
|
||||
|
||||
@@ -261,6 +263,15 @@ type Library struct {
|
||||
AutotagWarningAcked int64
|
||||
}
|
||||
|
||||
type ListeningEvent struct {
|
||||
ID int64
|
||||
AudioFileID int64
|
||||
Kind string
|
||||
PositionSeconds int64
|
||||
DurationSeconds int64
|
||||
OccurredAt time.Time
|
||||
}
|
||||
|
||||
type Lyric struct {
|
||||
AudioFileID int64
|
||||
Text string
|
||||
@@ -273,12 +284,6 @@ type LyricsIndex struct {
|
||||
Lyrics string
|
||||
}
|
||||
|
||||
type PlayHistory struct {
|
||||
ID int64
|
||||
AudioFileID int64
|
||||
PlayedAt time.Time
|
||||
}
|
||||
|
||||
type PlayerState struct {
|
||||
ID int64
|
||||
Volume int64
|
||||
@@ -312,15 +317,14 @@ type PlaylistTrack struct {
|
||||
}
|
||||
|
||||
type Queue struct {
|
||||
ID int64
|
||||
SourcePlaylistID sql.NullInt64
|
||||
CurrentPosition int64
|
||||
ShuffleMode bool
|
||||
RepeatMode string
|
||||
ShuffleOrder sql.NullString
|
||||
SourceType string
|
||||
SourceID int64
|
||||
SourceLabel string
|
||||
ID int64
|
||||
CurrentPosition int64
|
||||
ShuffleMode bool
|
||||
RepeatMode string
|
||||
ShuffleOrder sql.NullString
|
||||
SourceType string
|
||||
SourceID int64
|
||||
SourceLabel string
|
||||
}
|
||||
|
||||
type QueueTrack struct {
|
||||
|
||||
@@ -245,6 +245,13 @@ func dropDeferred(
|
||||
ctx context.Context, db *sql.DB, logger *slog.Logger,
|
||||
drop map[string]string,
|
||||
) error {
|
||||
// Asked before the transaction opens, because the answer is about
|
||||
// which tables are live and that cannot change underneath us here.
|
||||
preserve, err := shouldPreservePhantoms(ctx, db, drop)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
tx, err := db.BeginTx(ctx, nil)
|
||||
if err != nil {
|
||||
return fmt.Errorf("could not begin the retire transaction: %w", err)
|
||||
@@ -256,6 +263,22 @@ func dropDeferred(
|
||||
return fmt.Errorf("could not defer foreign keys: %w", err)
|
||||
}
|
||||
|
||||
// Before any drop, so every entry still has a track to read. It is
|
||||
// in this transaction rather than beside it because the preservation
|
||||
// and the delete have to succeed or fail together: a commit that
|
||||
// dropped the files without the phantoms is the bug, and a commit
|
||||
// that wrote phantoms without dropping anything is a lie about rows
|
||||
// that are still there.
|
||||
if preserve {
|
||||
logger.Info(
|
||||
"preserving playlist entries across the retire of audio_files",
|
||||
)
|
||||
|
||||
if err := PreservePlaylistPhantoms(ctx, tx, logger); err != nil {
|
||||
return err
|
||||
}
|
||||
}
|
||||
|
||||
// Sorted, so a failure is reproducible. Map order is random, and a
|
||||
// bug that depends on which table happens to go first reproduces on
|
||||
// one run in three and passes review on the other two -- which is
|
||||
@@ -283,6 +306,38 @@ func dropDeferred(
|
||||
return nil
|
||||
}
|
||||
|
||||
// shouldPreservePhantoms reports whether this retire is about to take
|
||||
// `audio_files` out from under the playlists.
|
||||
//
|
||||
// The `playlist_tracks` check is not defensive padding. This runs
|
||||
// *before* applySchema, which is the moment the schema is by definition
|
||||
// mid-repair, and the preservation reads a table it does not drop. A
|
||||
// database old enough not to have it would otherwise fail here, and
|
||||
// failing here means the app does not open at all -- while nothing is
|
||||
// lost by skipping, since an absent `playlist_tracks` holds no
|
||||
// playlists to save.
|
||||
//
|
||||
// It deliberately does *not* ask after `track_metadata`. Whether that
|
||||
// view can answer is PreservePlaylistPhantoms's own business, because a
|
||||
// view broken against an older `audio_files` is a state this function
|
||||
// cannot detect without hitting the same error it is trying to avoid:
|
||||
// pragma_table_info on such a view errors rather than reporting no
|
||||
// columns.
|
||||
func shouldPreservePhantoms(
|
||||
ctx context.Context, db *sql.DB, drop map[string]string,
|
||||
) (bool, error) {
|
||||
if _, going := drop["audio_files"]; !going {
|
||||
return false, nil
|
||||
}
|
||||
|
||||
cols, err := liveColumns(ctx, db, "playlist_tracks")
|
||||
if err != nil {
|
||||
return false, err
|
||||
}
|
||||
|
||||
return len(cols) > 0, nil
|
||||
}
|
||||
|
||||
// staleReason reports why a live table disagrees with its declaration,
|
||||
// or "" when it agrees. A column the live table does not have is the
|
||||
// additive case; a column whose declared type changed is the one an
|
||||
|
||||
@@ -497,3 +497,180 @@ func TestParseCreateTablesReadsTheRealSchema(t *testing.T) {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// TestRetiringAudioFilesKeepsPlaylistContents is the symptom this
|
||||
// repair exists for: a playlist survived the retire as a row count and
|
||||
// nothing else.
|
||||
//
|
||||
// TestRetiringOwnedTablesDoesNotDangle already asserts the entry does
|
||||
// not keep a stale id, which is the *dangerous* half. It is satisfied
|
||||
// just as well by an entry that says nothing at all, which is the
|
||||
// half that quietly emptied every playlist -- so this asserts what the
|
||||
// entry still knows, and specifically phantom_file_path, because that
|
||||
// is the column ResolvePhantomTracksAfterScan matches back against
|
||||
// audio_files.file_path.
|
||||
//
|
||||
// Note the seed drops `comment`, not `artist_credit`: the mutation has
|
||||
// to leave `track_metadata` standing, since a real launch reaches the
|
||||
// retire with the view the previous launch created. A test that drops
|
||||
// the view first is testing the skip path, not this one.
|
||||
func TestRetiringAudioFilesKeepsPlaylistContents(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
db := openRaw(t, t.TempDir())
|
||||
|
||||
if _, err := db.ExecContext(ctx, "PRAGMA foreign_keys = ON"); err != nil {
|
||||
t.Fatalf("pragma: %v", err)
|
||||
}
|
||||
|
||||
if err := applySchema(ctx, db); err != nil {
|
||||
t.Fatalf("applySchema: %v", err)
|
||||
}
|
||||
|
||||
if _, err := db.ExecContext(ctx, `
|
||||
INSERT INTO playlists (id, name) VALUES (1, 'keepme');
|
||||
INSERT INTO libraries (id, name, path) VALUES (0, 'test', '/music');
|
||||
INSERT INTO artists (id, name) VALUES (3, 'Aurora Fields');
|
||||
INSERT INTO cover_art (id, file_path, mime_type)
|
||||
VALUES (9, 'covers/7.jpg', 'image/jpeg');
|
||||
INSERT INTO genres (id, name) VALUES (5, 'Ambient');
|
||||
INSERT INTO albums (id, name, artist_id, cover_art_id)
|
||||
VALUES (4, 'Tideline', 3, 9);
|
||||
INSERT INTO audio_files
|
||||
(id, file_path, file_type_id, length_milliseconds,
|
||||
title, artist_credit, artist_id, album_id)
|
||||
VALUES (7, '/music/a.flac', 1, 1000,
|
||||
'Slack Water', 'Aurora Fields', 3, 4);
|
||||
INSERT INTO file_genres (audio_file_id, genre_id) VALUES (7, 5);
|
||||
INSERT INTO playlist_tracks (playlist_id, audio_file_id, position)
|
||||
VALUES (1, 7, 0);
|
||||
ALTER TABLE audio_files DROP COLUMN comment;
|
||||
`); err != nil {
|
||||
t.Fatalf("seed: %v", err)
|
||||
}
|
||||
|
||||
if err := retireStaleTables(ctx, db, testLogger()); err != nil {
|
||||
t.Fatalf("retire: %v", err)
|
||||
}
|
||||
|
||||
if err := applySchema(ctx, db); err != nil {
|
||||
t.Fatalf("applySchema: %v", err)
|
||||
}
|
||||
|
||||
var (
|
||||
path, title, artist, album, genre, cover sql.NullString
|
||||
duration sql.NullInt64
|
||||
)
|
||||
|
||||
if err := db.QueryRowContext(ctx, `
|
||||
SELECT phantom_file_path, phantom_title, phantom_artist,
|
||||
phantom_album, phantom_duration_ms, phantom_genre,
|
||||
phantom_cover_art_path
|
||||
FROM playlist_tracks WHERE playlist_id = 1
|
||||
`).Scan(&path, &title, &artist, &album, &duration, &genre, &cover); err != nil {
|
||||
t.Fatalf("read the surviving entry: %v", err)
|
||||
}
|
||||
|
||||
// The one that matters: without it the entry can never be re-linked
|
||||
// by the rescan the retire itself provokes.
|
||||
if path.String != "/music/a.flac" {
|
||||
t.Fatalf(
|
||||
"phantom_file_path is %q, want %q -- the playlist entry "+
|
||||
"cannot be re-linked and the playlist is empty for good",
|
||||
path.String, "/music/a.flac",
|
||||
)
|
||||
}
|
||||
|
||||
if title.String != "Slack Water" {
|
||||
t.Errorf("phantom_title is %q, want %q", title.String, "Slack Water")
|
||||
}
|
||||
|
||||
if artist.String != "Aurora Fields" {
|
||||
t.Errorf("phantom_artist is %q, want %q", artist.String, "Aurora Fields")
|
||||
}
|
||||
|
||||
if album.String != "Tideline" {
|
||||
t.Errorf("phantom_album is %q, want %q", album.String, "Tideline")
|
||||
}
|
||||
|
||||
if duration.Int64 != 1000 {
|
||||
t.Errorf("phantom_duration_ms is %d, want 1000", duration.Int64)
|
||||
}
|
||||
|
||||
if genre.String != "Ambient" {
|
||||
t.Errorf("phantom_genre is %q, want %q", genre.String, "Ambient")
|
||||
}
|
||||
|
||||
if cover.String != "covers/7.jpg" {
|
||||
t.Errorf("phantom_cover_art_path is %q, want %q", cover.String, "covers/7.jpg")
|
||||
}
|
||||
}
|
||||
|
||||
// TestRetiringAudioFilesKeepsPathsWhenTheViewCannotAnswer is the case
|
||||
// that broke cmd/indexbuild: this repair runs *before* applySchema, so
|
||||
// `track_metadata` is whatever the last launch declared while
|
||||
// `audio_files` is whatever the launch before that left behind, and a
|
||||
// view over columns the table no longer has does not read as empty --
|
||||
// it errors.
|
||||
//
|
||||
// The pre-013 stub shape below is the real one that fixture carries.
|
||||
// What must survive is phantom_file_path, because `file_path` is the
|
||||
// table's natural key and has been in every shape it ever had; the
|
||||
// display columns are allowed to be absent, and the open must not fail.
|
||||
func TestRetiringAudioFilesKeepsPathsWhenTheViewCannotAnswer(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
db := openRaw(t, t.TempDir())
|
||||
|
||||
if _, err := db.ExecContext(ctx, "PRAGMA foreign_keys = ON"); err != nil {
|
||||
t.Fatalf("pragma: %v", err)
|
||||
}
|
||||
|
||||
if err := applySchema(ctx, db); err != nil {
|
||||
t.Fatalf("applySchema: %v", err)
|
||||
}
|
||||
|
||||
// The rows go in *after* the reshape: dropping audio_files with
|
||||
// foreign keys on would fire the ON DELETE SET NULL and null the
|
||||
// entry this test is about, which would pass for the wrong reason.
|
||||
if _, err := db.ExecContext(ctx, `
|
||||
DROP TABLE audio_files;
|
||||
CREATE TABLE audio_files (
|
||||
id INTEGER PRIMARY KEY,
|
||||
file_path TEXT NOT NULL UNIQUE,
|
||||
recording_id INTEGER
|
||||
);
|
||||
INSERT INTO playlists (id, name) VALUES (1, 'keepme');
|
||||
INSERT INTO audio_files (id, file_path) VALUES (7, '/music/a.flac');
|
||||
INSERT INTO playlist_tracks (playlist_id, audio_file_id, position)
|
||||
VALUES (1, 7, 0);
|
||||
`); err != nil {
|
||||
t.Fatalf("seed: %v", err)
|
||||
}
|
||||
|
||||
// The symptom this guards: the repair must not turn a recoverable
|
||||
// database into one the app refuses to open.
|
||||
if err := retireStaleTables(ctx, db, testLogger()); err != nil {
|
||||
t.Fatalf(
|
||||
"the retire failed on a view it could not read, so the app "+
|
||||
"would not open at all: %v", err,
|
||||
)
|
||||
}
|
||||
|
||||
if err := applySchema(ctx, db); err != nil {
|
||||
t.Fatalf("applySchema: %v", err)
|
||||
}
|
||||
|
||||
var path sql.NullString
|
||||
if err := db.QueryRowContext(ctx,
|
||||
"SELECT phantom_file_path FROM playlist_tracks WHERE playlist_id = 1",
|
||||
).Scan(&path); err != nil {
|
||||
t.Fatalf("read the surviving entry: %v", err)
|
||||
}
|
||||
|
||||
if path.String != "/music/a.flac" {
|
||||
t.Fatalf(
|
||||
"phantom_file_path is %q, want %q -- the display half being "+
|
||||
"unavailable must not cost the entry its one re-link key",
|
||||
path.String, "/music/a.flac",
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -269,10 +269,11 @@ var tables = []Table{
|
||||
"from owned files plus the LRCLIB backfill.",
|
||||
},
|
||||
{
|
||||
Name: "play_history", Kind: Authored, Lifetime: Cascade,
|
||||
Note: "Listening history. Authored, but intentionally cascades " +
|
||||
"with its track — history for a file no longer in the library " +
|
||||
"has nothing to point at.",
|
||||
Name: "listening_events", Kind: Authored, Lifetime: Cascade,
|
||||
Note: "Listening history, one row per track exit (complete, play " +
|
||||
"or skip). Authored, but intentionally cascades with its " +
|
||||
"track — history for a file no longer in the library has " +
|
||||
"nothing to point at.",
|
||||
},
|
||||
{
|
||||
Name: "player_state", Kind: Authored, Lifetime: Retained,
|
||||
|
||||
@@ -212,14 +212,14 @@ func TestLifetimesMatchSchema(t *testing.T) {
|
||||
|
||||
// Authored data is unrecoverable, so it must never be removed as a side
|
||||
// effect of deleting owned data. Cascade is allowed only where the
|
||||
// catalog explains why (play_history, queue_tracks); this test pins the
|
||||
// catalog explains why (listening_events, queue_tracks); this test pins the
|
||||
// set so a new cascade onto authored data is a deliberate decision.
|
||||
func TestAuthoredCascadesAreDeliberate(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
allowed := map[string]bool{
|
||||
"play_history": true,
|
||||
"queue_tracks": true,
|
||||
"listening_events": true,
|
||||
"queue_tracks": true,
|
||||
|
||||
// Download history is scoped to the library it imported into.
|
||||
// When that library is removed the files it acquired go with
|
||||
|
||||
+92
-30
@@ -911,30 +911,96 @@ func (l *Library) scanInternal(
|
||||
|
||||
orphanStart := time.Now()
|
||||
|
||||
// Snapshot the orphan set first. The playlist-phantom
|
||||
// preservation and the deletes are one transaction (the
|
||||
// preservation has to land before the ON DELETE SET NULL, and
|
||||
// both have to succeed or neither does), and the ids are what
|
||||
// scope that preservation to just these files instead of
|
||||
// rewriting every playlist row on a routine scan.
|
||||
var (
|
||||
orphans []sqlcgen.AudioFile
|
||||
orphanPaths []string
|
||||
)
|
||||
|
||||
existingPaths.Range(func(key, value any) bool {
|
||||
path := key.(string)
|
||||
audioFile := value.(sqlcgen.AudioFile)
|
||||
orphanPaths = append(orphanPaths, key.(string))
|
||||
orphans = append(orphans, value.(sqlcgen.AudioFile))
|
||||
|
||||
l.logger.Debug(
|
||||
"removing orphaned database entry",
|
||||
"path", path, "id", audioFile.ID,
|
||||
)
|
||||
return true
|
||||
})
|
||||
|
||||
if err := l.db.Queries.DeleteAudioFile(
|
||||
l.ctx, audioFile.ID,
|
||||
); err != nil {
|
||||
l.logger.Warn(
|
||||
"failed to delete orphaned audio file",
|
||||
"path", path,
|
||||
"id", audioFile.ID,
|
||||
"err", err,
|
||||
)
|
||||
deleted := make([]bool, len(orphans))
|
||||
|
||||
metrics.addWarning(path, "orphan", err)
|
||||
|
||||
return true
|
||||
if len(orphans) > 0 {
|
||||
orphanIDs := make([]int64, len(orphans))
|
||||
for i, f := range orphans {
|
||||
orphanIDs[i] = f.ID
|
||||
}
|
||||
|
||||
tx, beginErr := l.db.BeginTx()
|
||||
if beginErr != nil {
|
||||
metrics.addWarning("", "orphan", beginErr)
|
||||
} else {
|
||||
defer func() { _ = tx.Rollback() }() // no-op after commit
|
||||
|
||||
if err := database.PreservePlaylistPhantomsForFiles(
|
||||
l.ctx, tx, orphanIDs, l.logger,
|
||||
); err != nil {
|
||||
// Deleting without the phantoms is exactly the
|
||||
// playlist-emptying bug the preservation exists to
|
||||
// prevent, so leave the rows for the next scan
|
||||
// rather than empty the playlists now.
|
||||
l.logger.Error(
|
||||
"skipping orphan deletion: could not preserve "+
|
||||
"playlist entries",
|
||||
"err", err,
|
||||
)
|
||||
|
||||
metrics.addWarning("", "orphan", err)
|
||||
|
||||
_ = tx.Rollback()
|
||||
} else {
|
||||
txq := l.db.Queries.WithTx(tx)
|
||||
|
||||
for i, f := range orphans {
|
||||
if err := txq.DeleteAudioFile(
|
||||
l.ctx, f.ID,
|
||||
); err != nil {
|
||||
l.logger.Warn(
|
||||
"failed to delete orphaned audio file",
|
||||
"path", orphanPaths[i],
|
||||
"id", f.ID,
|
||||
"err", err,
|
||||
)
|
||||
|
||||
metrics.addWarning(orphanPaths[i], "orphan", err)
|
||||
|
||||
continue
|
||||
}
|
||||
|
||||
deleted[i] = true
|
||||
}
|
||||
|
||||
if err := tx.Commit(); err != nil {
|
||||
l.logger.Error(
|
||||
"could not commit orphan deletion",
|
||||
"err", err,
|
||||
)
|
||||
|
||||
metrics.addWarning("", "orphan", err)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Post-commit bookkeeping for the files that actually went.
|
||||
for i, f := range orphans {
|
||||
if !deleted[i] {
|
||||
continue
|
||||
}
|
||||
|
||||
path := orphanPaths[i]
|
||||
|
||||
// Keep the file's tagging group in sync: drop the group's
|
||||
// track count and clear it out once empty, mirroring the
|
||||
// bookkeeping maybeRebindTaggingGroup does for a group_key
|
||||
@@ -942,25 +1008,25 @@ func (l *Library) scanInternal(
|
||||
// and replaced leaves a stale tagging_items row behind —
|
||||
// its track_count still counts the deleted files, and it
|
||||
// never clears from the autotag queue.
|
||||
if audioFile.GroupKey != "" {
|
||||
if f.GroupKey != "" {
|
||||
if err := l.db.Queries.DecrementTaggingItemTrackCount(
|
||||
l.ctx, audioFile.GroupKey,
|
||||
l.ctx, f.GroupKey,
|
||||
); err != nil {
|
||||
l.logger.Warn(
|
||||
"failed to decrement tagging group for orphan",
|
||||
"path", path,
|
||||
"group_key", audioFile.GroupKey,
|
||||
"group_key", f.GroupKey,
|
||||
"err", err,
|
||||
)
|
||||
|
||||
metrics.addWarning(path, "orphan", err)
|
||||
} else if err := l.db.Queries.DeleteTaggingItemIfEmpty(
|
||||
l.ctx, audioFile.GroupKey,
|
||||
l.ctx, f.GroupKey,
|
||||
); err != nil {
|
||||
l.logger.Warn(
|
||||
"failed to clean up emptied tagging group for orphan",
|
||||
"path", path,
|
||||
"group_key", audioFile.GroupKey,
|
||||
"group_key", f.GroupKey,
|
||||
"err", err,
|
||||
)
|
||||
|
||||
@@ -969,12 +1035,10 @@ func (l *Library) scanInternal(
|
||||
}
|
||||
|
||||
// Remove from FTS5 search index.
|
||||
if err := l.db.DeleteSearchIndex(
|
||||
audioFile.ID,
|
||||
); err != nil {
|
||||
if err := l.db.DeleteSearchIndex(f.ID); err != nil {
|
||||
l.logger.Warn(
|
||||
"failed to delete FTS entry for orphan",
|
||||
"id", audioFile.ID,
|
||||
"id", f.ID,
|
||||
"err", err,
|
||||
)
|
||||
|
||||
@@ -982,9 +1046,7 @@ func (l *Library) scanInternal(
|
||||
}
|
||||
|
||||
removed.Add(1)
|
||||
|
||||
return true
|
||||
})
|
||||
}
|
||||
|
||||
metrics.OrphanCleanup = time.Since(orphanStart)
|
||||
|
||||
|
||||
@@ -94,6 +94,57 @@ func countRows(
|
||||
return n
|
||||
}
|
||||
|
||||
// RemoveFromLibrary is the one path that empties a track *deliberately*:
|
||||
// the file stays on disk but is excluded, so nothing re-imports it. The
|
||||
// playlist entry must still survive as a re-linkable phantom rather than
|
||||
// an empty row, because a later full rescan clears the exclusion and is
|
||||
// what re-links the entry then (#246).
|
||||
func TestRemoveFromLibrary_PreservesPlaylistPhantoms(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
lib, db := setupTestLibrary(t)
|
||||
|
||||
seedRemovableLibrary(t, lib, "/nonexistent/cover.jpg")
|
||||
|
||||
if _, err := lib.db.ExecContext(
|
||||
`INSERT INTO playlists (name) VALUES ('keepme')`,
|
||||
); err != nil {
|
||||
t.Fatalf("seed playlist: %v", err)
|
||||
}
|
||||
|
||||
playlistID := queryInt(
|
||||
t, db, `SELECT id FROM playlists WHERE name = 'keepme'`,
|
||||
)
|
||||
|
||||
if _, err := lib.db.ExecContext(
|
||||
`INSERT INTO playlist_tracks (playlist_id, audio_file_id, position)
|
||||
SELECT ?, id, 0 FROM audio_files
|
||||
WHERE file_path = '/music/song.mp3'`,
|
||||
playlistID,
|
||||
); err != nil {
|
||||
t.Fatalf("seed playlist_tracks: %v", err)
|
||||
}
|
||||
|
||||
if _, err := lib.RemoveFromLibrary([]string{"/music/song.mp3"}); err != nil {
|
||||
t.Fatalf("RemoveFromLibrary: %v", err)
|
||||
}
|
||||
|
||||
phantomPath := queryString(
|
||||
t, db,
|
||||
`SELECT phantom_file_path FROM playlist_tracks
|
||||
WHERE playlist_id = ?`,
|
||||
playlistID,
|
||||
)
|
||||
|
||||
if phantomPath != "/music/song.mp3" {
|
||||
t.Fatalf(
|
||||
"phantom_file_path is %q, want %q -- the entry cannot be "+
|
||||
"re-linked after a later full rescan",
|
||||
phantomPath, "/music/song.mp3",
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
// A library with tagging_items must still be removable. tagging_items
|
||||
// FK-references libraries with no ON DELETE clause, so leaving those
|
||||
// rows behind fails the DELETE and rolls back the entire removal.
|
||||
|
||||
@@ -4,6 +4,7 @@ import (
|
||||
"errors"
|
||||
"fmt"
|
||||
|
||||
"yellowjacket/backend/database"
|
||||
"yellowjacket/backend/events"
|
||||
)
|
||||
|
||||
@@ -57,6 +58,25 @@ func (l *Library) RemoveFromLibrary(filePaths []string) (*RemovalResult, error)
|
||||
|
||||
var result RemovalResult
|
||||
|
||||
// Preserve the playlist entries before the rows go, so they survive
|
||||
// as re-linkable phantoms rather than empty rows. The track is
|
||||
// excluded and will not be re-imported on its own, but a later full
|
||||
// rescan clears the exclusion and this is what lets the entry
|
||||
// re-link then — the same preservation every other path that empties
|
||||
// audio_files performs (#246).
|
||||
rowIDs := make([]int64, len(rows))
|
||||
for i, row := range rows {
|
||||
rowIDs[i] = row.ID
|
||||
}
|
||||
|
||||
if err := database.PreservePlaylistPhantomsForFiles(
|
||||
l.ctx, tx, rowIDs, l.logger,
|
||||
); err != nil {
|
||||
return nil, fmt.Errorf(
|
||||
"could not preserve playlist entries for removal: %w", err,
|
||||
)
|
||||
}
|
||||
|
||||
// Exclude every path the caller named, including one whose row has
|
||||
// already gone: the user asked for that file to stay out, and a row
|
||||
// that disappeared between the click and the commit is not a reason
|
||||
|
||||
@@ -8,6 +8,7 @@ import (
|
||||
"time"
|
||||
|
||||
"yellowjacket/backend/coverart"
|
||||
"yellowjacket/backend/database"
|
||||
)
|
||||
|
||||
var errNoLibrariesConfigured = errors.New(
|
||||
@@ -137,34 +138,12 @@ func (l *Library) clearLibraryTables() error {
|
||||
// metadata for all linked tracks before audio_files are deleted.
|
||||
// ON DELETE SET NULL will null out audio_file_id, converting them
|
||||
// to phantoms that ResolvePhantomTracksAfterScan can re-link.
|
||||
if _, err := tx.ExecContext(l.ctx, `
|
||||
UPDATE playlist_tracks
|
||||
SET
|
||||
phantom_title = COALESCE(phantom_title, (
|
||||
SELECT tm.title FROM track_metadata tm
|
||||
WHERE tm.id = playlist_tracks.audio_file_id
|
||||
)),
|
||||
phantom_artist = COALESCE(phantom_artist, (
|
||||
SELECT tm.artist_name FROM track_metadata tm
|
||||
WHERE tm.id = playlist_tracks.audio_file_id
|
||||
)),
|
||||
phantom_album = COALESCE(phantom_album, (
|
||||
SELECT tm.album FROM track_metadata tm
|
||||
WHERE tm.id = playlist_tracks.audio_file_id
|
||||
)),
|
||||
phantom_duration_ms = COALESCE(phantom_duration_ms, (
|
||||
SELECT af.length_milliseconds FROM audio_files af
|
||||
WHERE af.id = playlist_tracks.audio_file_id
|
||||
)),
|
||||
phantom_file_path = COALESCE(phantom_file_path, (
|
||||
SELECT af.file_path FROM audio_files af
|
||||
WHERE af.id = playlist_tracks.audio_file_id
|
||||
))
|
||||
WHERE audio_file_id IS NOT NULL
|
||||
`); err != nil {
|
||||
return fmt.Errorf(
|
||||
"could not preserve playlist track metadata: %w", err,
|
||||
)
|
||||
//
|
||||
// Shared with the stale-shape retire in backend/database, which is
|
||||
// the other path that empties this table and which did not do this
|
||||
// (#183): the statement lives there so the two cannot drift again.
|
||||
if err := database.PreservePlaylistPhantoms(l.ctx, tx, l.logger); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
// Phase 2: the files. file_genres cascades with them.
|
||||
|
||||
@@ -231,3 +231,98 @@ func TestScan_MultipleDirectoriesDoNotCrossContaminate(t *testing.T) {
|
||||
t.Errorf("Album A and Album B must not share a group_key: %+v", keys)
|
||||
}
|
||||
}
|
||||
|
||||
// TestScan_OrphanCleanupPreservesPlaylistPhantoms guards #246: a file
|
||||
// deleted from the library folder *outside* YellowJacket is discovered
|
||||
// as an orphan by the next scan, and its playlist entry must survive as
|
||||
// a re-linkable phantom — the same preservation the full rescan and
|
||||
// stale-retire paths already perform, scoped here to just the orphaned
|
||||
// file. Before the fix the entry became an empty row (audio_file_id
|
||||
// NULL and no phantom_file_path), which nothing can ever re-link.
|
||||
func TestScan_OrphanCleanupPreservesPlaylistPhantoms(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
lib, db := setupTestLibrary(t)
|
||||
|
||||
root := t.TempDir()
|
||||
track := filepath.Join(root, "gone.mp3")
|
||||
|
||||
writeTestTrack(t, track, 0)
|
||||
|
||||
library, err := db.Queries.CreateLibrary(lib.ctx, sqlcgen.CreateLibraryParams{
|
||||
Name: "orphans",
|
||||
Path: root,
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("create library: %v", err)
|
||||
}
|
||||
|
||||
if metrics := lib.scanInternal(library.ID, library.Name, library.Path); metrics == nil {
|
||||
t.Fatal("first scan returned nil metrics")
|
||||
}
|
||||
|
||||
trackID := queryInt(
|
||||
t, db, "SELECT id FROM audio_files WHERE file_path = ?", track,
|
||||
)
|
||||
if trackID == 0 {
|
||||
t.Fatal("first scan did not import the track")
|
||||
}
|
||||
|
||||
if _, err := db.ExecContext(
|
||||
`INSERT INTO playlists (name) VALUES ('keepme')`,
|
||||
); err != nil {
|
||||
t.Fatalf("seed playlist: %v", err)
|
||||
}
|
||||
|
||||
playlistID := queryInt(
|
||||
t, db, "SELECT id FROM playlists WHERE name = 'keepme'",
|
||||
)
|
||||
|
||||
if _, err := db.ExecContext(
|
||||
`INSERT INTO playlist_tracks (playlist_id, audio_file_id, position)
|
||||
VALUES (?, ?, 0)`,
|
||||
playlistID, trackID,
|
||||
); err != nil {
|
||||
t.Fatalf("seed playlist_tracks: %v", err)
|
||||
}
|
||||
|
||||
// The file goes away outside the app.
|
||||
if err := os.Remove(track); err != nil {
|
||||
t.Fatalf("remove track: %v", err)
|
||||
}
|
||||
|
||||
if metrics := lib.scanInternal(library.ID, library.Name, library.Path); metrics == nil {
|
||||
t.Fatal("second scan returned nil metrics")
|
||||
}
|
||||
|
||||
if n := queryInt(
|
||||
t, db, "SELECT COUNT(*) FROM audio_files WHERE file_path = ?", track,
|
||||
); n != 0 {
|
||||
t.Fatalf("audio_files still holds the removed path: %d rows", n)
|
||||
}
|
||||
|
||||
if n := queryInt(
|
||||
t, db,
|
||||
"SELECT COUNT(*) FROM playlist_tracks WHERE playlist_id = ? "+
|
||||
"AND audio_file_id IS NULL",
|
||||
playlistID,
|
||||
); n != 1 {
|
||||
t.Fatalf(
|
||||
"playlist entry did not become a phantom: %d null-id rows, want 1",
|
||||
n,
|
||||
)
|
||||
}
|
||||
|
||||
phantomPath := queryString(
|
||||
t, db,
|
||||
"SELECT phantom_file_path FROM playlist_tracks WHERE playlist_id = ?",
|
||||
playlistID,
|
||||
)
|
||||
if phantomPath != track {
|
||||
t.Fatalf(
|
||||
"phantom_file_path = %q, want %q -- the entry cannot be "+
|
||||
"re-linked if the file comes back",
|
||||
phantomPath, track,
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -6,7 +6,7 @@ import (
|
||||
"yellowjacket/backend/events"
|
||||
)
|
||||
|
||||
// recordPlay inserts a play_history row and updates the denormalized
|
||||
// recordPlay inserts a listening_events row and updates the denormalized
|
||||
// play_count / last_played columns on audio_files. Called from
|
||||
// OnPlaybackFinished for the track that just finished.
|
||||
//
|
||||
@@ -20,10 +20,12 @@ func (q *Queue) recordPlay(audioFileID int64) {
|
||||
|
||||
now := time.Now().UTC().Format(time.DateTime)
|
||||
|
||||
// Insert play_history row.
|
||||
// Insert the listening event. A natural finish is a 'complete' by
|
||||
// construction; position/duration are the classifier's to fill once
|
||||
// skips are recorded (see .planning/plans/active/021).
|
||||
_, err := q.db.ExecContext(
|
||||
`INSERT INTO play_history (audio_file_id, played_at)
|
||||
VALUES (?, ?)`,
|
||||
`INSERT INTO listening_events (audio_file_id, kind, occurred_at)
|
||||
VALUES (?, 'complete', ?)`,
|
||||
audioFileID, now,
|
||||
)
|
||||
if err != nil {
|
||||
|
||||
@@ -42,10 +42,10 @@ const ACTIONS = ['Import', 'New Playlist', 'New Smart Playlist'];
|
||||
* because the number this issue is about (a button 48px wider than the
|
||||
* box holding it) is not in the accessibility tree at all.
|
||||
*/
|
||||
const headerFit = (page: import('@playwright/test').Page) =>
|
||||
page.evaluate(() => {
|
||||
const headerFit = (page: import('@playwright/test').Page, view = 'playlist-view') =>
|
||||
page.evaluate((tag) => {
|
||||
const root = document
|
||||
.querySelector('[data-testid="main-content"] playlist-view')
|
||||
.querySelector(`[data-testid="main-content"] ${tag}`)
|
||||
?.shadowRoot?.querySelector('page-header')?.shadowRoot;
|
||||
|
||||
if (!root) return null;
|
||||
@@ -76,7 +76,7 @@ const headerFit = (page: import('@playwright/test').Page) =>
|
||||
...root.querySelectorAll('#page-header-overflow wa-dropdown-item'),
|
||||
].map((i) => i.textContent?.trim() ?? ''),
|
||||
};
|
||||
});
|
||||
}, view);
|
||||
|
||||
test.describe('the page header never clips an action', () => {
|
||||
test.beforeEach(async ({ app }) => {
|
||||
@@ -316,3 +316,58 @@ test.describe('the page header never clips an action', () => {
|
||||
await expect.poll(async () => (await headerFit(app))?.menu).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* The Tracks header carries the play-all/shuffle-all pair (#31), so
|
||||
* the promise above has to hold for it too — the same per-button
|
||||
* measurement, one view over. Its two actions are the whole of the
|
||||
* header's declared set, and the pair is what plays the list the row
|
||||
* is in, so a button rendered 20px of its 90px is a queue of nothing.
|
||||
*/
|
||||
const TRACK_ACTIONS = ['Play all', 'Shuffle all'];
|
||||
|
||||
test.describe('the Tracks header never clips an action', () => {
|
||||
test.beforeEach(async ({ app }) => {
|
||||
await app.getByTestId('nav-tracks').click();
|
||||
await expect(app.getByTestId('main-content')).toHaveAttribute(
|
||||
'data-active-view',
|
||||
'tracks',
|
||||
);
|
||||
});
|
||||
|
||||
test.afterEach(async ({ app }) => {
|
||||
await app.setViewportSize({ width: 1280, height: 800 });
|
||||
});
|
||||
|
||||
for (const vp of VIEWPORTS) {
|
||||
test(`every action is reachable at ${vp.name}`, async ({ app }) => {
|
||||
await app.setViewportSize({ width: vp.width, height: vp.height });
|
||||
|
||||
await expect
|
||||
.poll(async () => (await headerFit(app, 'track-list'))?.clipped)
|
||||
.toEqual([]);
|
||||
|
||||
const fit = (await headerFit(app, 'track-list'))!;
|
||||
|
||||
expect(fit.overflow).toBeLessThanOrEqual(0);
|
||||
|
||||
// Between them, buttons and menu account for both actions —
|
||||
// not "it fits" but "nothing was dropped to make it fit".
|
||||
expect([...fit.buttons, ...fit.menu].sort()).toEqual(
|
||||
[...TRACK_ACTIONS].sort(),
|
||||
);
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* The pair's names, through the accessibility tree — a shadow query
|
||||
* measures, but it cannot say what a screen reader is offered.
|
||||
*/
|
||||
test('both actions are named controls', async ({ app }) => {
|
||||
for (const label of TRACK_ACTIONS) {
|
||||
await expect(
|
||||
app.getByRole('button', { name: label, exact: true }),
|
||||
).toBeVisible();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -0,0 +1,220 @@
|
||||
import { test, expect, callBinding, resetEvents, waitForEvent } from '../support/fixtures.js';
|
||||
|
||||
type Page = import('@playwright/test').Page;
|
||||
|
||||
/**
|
||||
* Play-all/Shuffle-all, asserted on what the backend queued rather than
|
||||
* on playback pixels.
|
||||
*
|
||||
* `SetQueue` reports the queue through `QueueChanged`, and `GetState`
|
||||
* says exactly what it holds: the tracks in order, whether shuffle is
|
||||
* on, and the `Source` the "Playing from" link is built from. That is
|
||||
* the honest contract here — the buttons are only as good as the queue
|
||||
* they build, and the queue is only as good as the source it names.
|
||||
*/
|
||||
|
||||
interface QueueState {
|
||||
tracks: { filePath: string; title: string }[];
|
||||
currentIndex: number;
|
||||
shuffleMode: boolean;
|
||||
source: { type: string; id: number; label: string };
|
||||
}
|
||||
|
||||
const TRACKS_SOURCE = { type: 'tracks', id: 0, label: 'All Tracks' };
|
||||
|
||||
const getQueue = (app: Page) =>
|
||||
callBinding<QueueState>(app, 'queue.Queue.GetState');
|
||||
|
||||
/** The track paths a rendered track list shows, in row order. */
|
||||
function displayedPaths(app: Page, scope: string): Promise<string[]> {
|
||||
return app
|
||||
.locator(`${scope} [data-testid="track-row"]`)
|
||||
.evaluateAll((els) =>
|
||||
els.map((el) => el.getAttribute('data-file-path') ?? ''),
|
||||
);
|
||||
}
|
||||
|
||||
/** Leave shuffle in a known state. The mode persists across specs in
|
||||
* one backend process, so a test that asserts on it has to set it. */
|
||||
async function setShuffleMode(app: Page, on: boolean): Promise<void> {
|
||||
const state = await getQueue(app);
|
||||
|
||||
if (state.shuffleMode !== on) {
|
||||
await resetEvents(app);
|
||||
await callBinding(app, 'queue.Queue.ToggleShuffle');
|
||||
await waitForEvent(app, 'QueueModeChanged');
|
||||
}
|
||||
}
|
||||
|
||||
test.describe('play-all/shuffle-all on the track list', () => {
|
||||
test.beforeEach(async ({ app }) => {
|
||||
await callBinding(app, 'queue.Queue.Clear').catch(() => {
|
||||
/* the queue is clearable on every build these specs run against */
|
||||
});
|
||||
await setShuffleMode(app, false);
|
||||
});
|
||||
|
||||
test('Tracks Play all queues the displayed list with an honest source', async ({
|
||||
app,
|
||||
}) => {
|
||||
await app.getByTestId('nav-tracks').click();
|
||||
await expect(app.getByTestId('main-content')).toHaveAttribute(
|
||||
'data-active-view',
|
||||
'tracks',
|
||||
);
|
||||
await expect(
|
||||
app.locator('track-list [data-testid="track-row"]').first(),
|
||||
).toBeVisible();
|
||||
|
||||
const paths = await displayedPaths(app, 'track-list');
|
||||
|
||||
await resetEvents(app);
|
||||
await app.getByTestId('page-action-play-all').click();
|
||||
await waitForEvent(app, 'QueueChanged');
|
||||
|
||||
const state = await getQueue(app);
|
||||
|
||||
expect(state.tracks.map((t) => t.filePath)).toEqual(paths);
|
||||
expect(state.currentIndex).toBe(0);
|
||||
expect(state.shuffleMode).toBe(false);
|
||||
expect(state.source).toEqual(TRACKS_SOURCE);
|
||||
});
|
||||
|
||||
test('Tracks Shuffle all turns shuffle on and keeps the source', async ({
|
||||
app,
|
||||
}) => {
|
||||
await app.getByTestId('nav-tracks').click();
|
||||
await expect(
|
||||
app.locator('track-list [data-testid="track-row"]').first(),
|
||||
).toBeVisible();
|
||||
|
||||
const paths = await displayedPaths(app, 'track-list');
|
||||
|
||||
await resetEvents(app);
|
||||
await app.getByTestId('page-action-shuffle-all').click();
|
||||
await waitForEvent(app, 'QueueChanged');
|
||||
|
||||
const state = await getQueue(app);
|
||||
|
||||
expect(state.tracks.map((t) => t.filePath)).toEqual(paths);
|
||||
expect(state.shuffleMode).toBe(true);
|
||||
expect(state.source).toEqual(TRACKS_SOURCE);
|
||||
});
|
||||
});
|
||||
|
||||
test.describe('play-all on an embedded track list', () => {
|
||||
test.beforeEach(async ({ app }) => {
|
||||
await callBinding(app, 'queue.Queue.Clear').catch(() => {});
|
||||
await setShuffleMode(app, false);
|
||||
});
|
||||
|
||||
test('a genre page queues the genre with its name as the source', async ({
|
||||
app,
|
||||
}) => {
|
||||
await app.getByTestId('nav-genres').click();
|
||||
await expect(app.getByTestId('main-content')).toHaveAttribute(
|
||||
'data-active-view',
|
||||
'genres',
|
||||
);
|
||||
|
||||
const first = app.locator('genres-view .genre-card').first();
|
||||
|
||||
await expect(first).toBeVisible();
|
||||
await first.click();
|
||||
|
||||
await expect(app.getByTestId('main-content')).toHaveAttribute(
|
||||
'data-active-view',
|
||||
'genre-details',
|
||||
);
|
||||
await expect(
|
||||
app.locator('genre-details [data-testid="track-row"]').first(),
|
||||
).toBeVisible();
|
||||
|
||||
const genreName = (await app
|
||||
.locator('genre-details .genre-title')
|
||||
.textContent())?.trim();
|
||||
const paths = await displayedPaths(app, 'genre-details');
|
||||
|
||||
await resetEvents(app);
|
||||
await app
|
||||
.locator('genre-details [data-testid="page-action-play-all"]')
|
||||
.click();
|
||||
await waitForEvent(app, 'QueueChanged');
|
||||
|
||||
const state = await getQueue(app);
|
||||
|
||||
expect(state.tracks.map((t) => t.filePath)).toEqual(paths);
|
||||
expect(state.currentIndex).toBe(0);
|
||||
expect(state.source).toEqual({ type: 'genre', id: 0, label: genreName });
|
||||
});
|
||||
});
|
||||
|
||||
test.describe('play-all on the library artist page', () => {
|
||||
test.beforeEach(async ({ app }) => {
|
||||
await callBinding(app, 'queue.Queue.Clear').catch(() => {});
|
||||
await setShuffleMode(app, false);
|
||||
});
|
||||
|
||||
test('an artist page queues album paths in album order with the artist source', async ({
|
||||
app,
|
||||
}) => {
|
||||
const artists = await callBinding<{ ID: number; Name: string }[]>(
|
||||
app,
|
||||
'library.Library.GetArtists',
|
||||
[0],
|
||||
);
|
||||
const first = artists[0]!;
|
||||
|
||||
await app.evaluate(
|
||||
([id, name]) => {
|
||||
document.dispatchEvent(
|
||||
new CustomEvent('navigate', {
|
||||
detail: {
|
||||
view: 'artist-details',
|
||||
artistId: id,
|
||||
artistName: name,
|
||||
},
|
||||
bubbles: true,
|
||||
composed: true,
|
||||
}),
|
||||
);
|
||||
},
|
||||
[first.ID, first.Name] as const,
|
||||
);
|
||||
|
||||
await expect(app.getByTestId('main-content')).toHaveAttribute(
|
||||
'data-active-view',
|
||||
'artist-details',
|
||||
);
|
||||
await expect(app.getByTestId('artist-play-all')).toBeEnabled();
|
||||
|
||||
const albums = await callBinding<{ ID: number }[]>(
|
||||
app,
|
||||
'library.Library.GetAlbumsByArtist',
|
||||
[first.Name, 0],
|
||||
);
|
||||
const byAlbum = await callBinding<Record<string, string[]>>(
|
||||
app,
|
||||
'library.Library.GetFilePathsByAlbums',
|
||||
[albums.map((a) => a.ID), 0],
|
||||
);
|
||||
const expected: string[] = [];
|
||||
|
||||
for (const album of albums) {
|
||||
expected.push(...(byAlbum[String(album.ID)] ?? []));
|
||||
}
|
||||
|
||||
await resetEvents(app);
|
||||
await app.getByTestId('artist-play-all').click();
|
||||
await waitForEvent(app, 'QueueChanged');
|
||||
|
||||
const state = await getQueue(app);
|
||||
|
||||
expect(state.tracks.map((t) => t.filePath)).toEqual(expected);
|
||||
expect(state.source).toEqual({
|
||||
type: 'artist',
|
||||
id: first.ID,
|
||||
label: first.Name,
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -137,7 +137,7 @@ test.describe('queue', () => {
|
||||
});
|
||||
|
||||
test('shuffle and repeat toggles report their state', async ({ app }) => {
|
||||
const shuffle = app.getByRole('button', { name: 'Shuffle' });
|
||||
const shuffle = app.getByRole('button', { name: 'Shuffle', exact: true });
|
||||
|
||||
await resetEvents(app);
|
||||
await shuffle.click();
|
||||
|
||||
@@ -11,11 +11,22 @@ import {
|
||||
GetArtistImageCachedPath,
|
||||
GetArtistMBID,
|
||||
} from '@go/explore/service.js';
|
||||
import { GetFilePathsByAlbums } from '@go/library/library.js';
|
||||
import { libraryStore } from '@store/library-store';
|
||||
import { notificationStore } from '@store/notification-store';
|
||||
import { dict } from '@utils/binding';
|
||||
import { playAll } from '@utils/play-all';
|
||||
import { describeError } from '@utils/describe-error';
|
||||
import { ICON_PLAY, ICON_SHUFFLE } from '@utils/icon-language';
|
||||
import '@awesome.me/webawesome/dist/components/icon/icon.js';
|
||||
import '@components/cover-grid/cover-grid.js';
|
||||
import '../notifications/inline-notice';
|
||||
import { designTokens } from '../../styles/tokens.css';
|
||||
import { backButton } from '../../styles/back-button.css';
|
||||
|
||||
/** The region the artist header's own failures are rendered in. */
|
||||
const ArtistRegion = 'library-artist';
|
||||
|
||||
@customElement('artist-details')
|
||||
export class ArtistDetails extends LitElement {
|
||||
@property({ type: Number, attribute: 'artist-id' })
|
||||
@@ -131,6 +142,39 @@ export class ArtistDetails extends LitElement {
|
||||
);
|
||||
}
|
||||
|
||||
.header-actions {
|
||||
margin-left: auto;
|
||||
display: flex;
|
||||
align-items: center;
|
||||
gap: 8px;
|
||||
flex-shrink: 0;
|
||||
}
|
||||
|
||||
.header-action {
|
||||
background: none;
|
||||
border: 1px solid var(--yj-border-subtle, #555);
|
||||
border-radius: 4px;
|
||||
color: var(--yj-text-primary, #fff);
|
||||
padding: 6px 12px;
|
||||
font-size: var(--yj-text-md, 13px);
|
||||
font-family: inherit;
|
||||
cursor: pointer;
|
||||
display: flex;
|
||||
align-items: center;
|
||||
gap: 6px;
|
||||
white-space: nowrap;
|
||||
}
|
||||
|
||||
.header-action:hover {
|
||||
border-color: var(--yj-accent, #ffd43b);
|
||||
color: var(--yj-accent-text, #ffd43b);
|
||||
}
|
||||
|
||||
.header-action:disabled {
|
||||
opacity: 0.5;
|
||||
cursor: default;
|
||||
}
|
||||
|
||||
/* ====================================
|
||||
* Content
|
||||
* ==================================== */
|
||||
@@ -145,6 +189,23 @@ export class ArtistDetails extends LitElement {
|
||||
height: 100%;
|
||||
}
|
||||
|
||||
/* Phone widths: the header's flex row squeezed .artist-info to
|
||||
* nothing, so the title ellipsised away entirely and the
|
||||
* actions clipped against the host's own overflow — the album
|
||||
* page's fault one detail view over (#66). The pair takes its
|
||||
* own row instead. Written last, because a media query adds no
|
||||
* specificity and a rule placed above the plain ones it
|
||||
* overrides is silently dead. */
|
||||
@media (max-width: 599px) {
|
||||
.artist-header {
|
||||
flex-wrap: wrap;
|
||||
}
|
||||
|
||||
.header-actions {
|
||||
flex-basis: 100%;
|
||||
margin-left: 0;
|
||||
}
|
||||
}
|
||||
`];
|
||||
|
||||
override connectedCallback() {
|
||||
@@ -302,6 +363,45 @@ export class ArtistDetails extends LitElement {
|
||||
return name.charAt(0).toUpperCase();
|
||||
}
|
||||
|
||||
/**
|
||||
* Play every track on this artist's albums, in album order.
|
||||
*
|
||||
* One `GetFilePathsByAlbums` call returns the paths grouped by
|
||||
* album id; the caller owns the ordering, so they are flattened in
|
||||
* `this.albums` order rather than by id.
|
||||
*/
|
||||
private async playAllTracks(shuffle: boolean): Promise<void> {
|
||||
if (this.albums.length === 0) return;
|
||||
|
||||
try {
|
||||
const libId = libraryStore.getSelectedLibraryId() ?? 0;
|
||||
const ids = this.albums.map((a) => a.ID);
|
||||
const byAlbum = await dict(
|
||||
GetFilePathsByAlbums(ids, libId),
|
||||
);
|
||||
const paths: string[] = [];
|
||||
|
||||
for (const id of ids) {
|
||||
paths.push(...(byAlbum[id] ?? []));
|
||||
}
|
||||
|
||||
playAll(
|
||||
paths,
|
||||
{
|
||||
type: 'artist',
|
||||
id: this.artistId,
|
||||
label: this.artistName,
|
||||
},
|
||||
shuffle,
|
||||
);
|
||||
} catch (error) {
|
||||
console.error('Could not play artist:', error);
|
||||
notificationStore.inline(ArtistRegion, {
|
||||
text: describeError(error, 'Could not play this artist’s tracks.'),
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
/* ================================================================
|
||||
* Rendering
|
||||
* ================================================================ */
|
||||
@@ -351,12 +451,38 @@ export class ArtistDetails extends LitElement {
|
||||
`
|
||||
: ''}
|
||||
</div>
|
||||
<div class="header-actions">
|
||||
<button
|
||||
class="header-action"
|
||||
data-testid="artist-play-all"
|
||||
?disabled=${this.albums.length === 0}
|
||||
@click=${() =>
|
||||
void this.playAllTracks(false)}
|
||||
>
|
||||
<wa-icon name=${ICON_PLAY}></wa-icon>
|
||||
Play all
|
||||
</button>
|
||||
<button
|
||||
class="header-action"
|
||||
data-testid="artist-shuffle-all"
|
||||
?disabled=${this.albums.length === 0}
|
||||
@click=${() =>
|
||||
void this.playAllTracks(true)}
|
||||
>
|
||||
<wa-icon name=${ICON_SHUFFLE}></wa-icon>
|
||||
Shuffle all
|
||||
</button>
|
||||
</div>
|
||||
</div>
|
||||
<div class="content">
|
||||
<cover-grid
|
||||
.externalAlbums=${this.albums}
|
||||
></cover-grid>
|
||||
</div>
|
||||
<inline-notice
|
||||
region=${ArtistRegion}
|
||||
testid="artist-play-message"
|
||||
></inline-notice>
|
||||
`;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -43,6 +43,7 @@ import type * as autotagservice from '@go/autotagservice/models.js';
|
||||
import { confirmAction } from '../confirm-dialog/confirm-dialog';
|
||||
import { queueStore } from '../../store/queue-store';
|
||||
import type { QueueSource } from '../../store/queue-store';
|
||||
import { playAll } from '@utils/play-all';
|
||||
import { notificationStore } from '../../store/notification-store';
|
||||
import '../notifications/inline-notice';
|
||||
import {
|
||||
@@ -2771,20 +2772,11 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost {
|
||||
|
||||
/** Play what the user owns of this release, optionally shuffled. */
|
||||
private playOwned(shuffle: boolean): void {
|
||||
const paths = this.ownedFilePaths();
|
||||
|
||||
// The button is only rendered when there is something to play,
|
||||
// so an empty set here is not a state the user can reach.
|
||||
if (paths.length === 0) return;
|
||||
|
||||
// `shuffleStart` only picks a random first track when shuffle
|
||||
// mode is *already* on — it does not turn it on — so the mode
|
||||
// has to be set before the queue, not after.
|
||||
if (shuffle && !queueStore.getState().shuffleMode) {
|
||||
queueStore.toggleShuffle();
|
||||
}
|
||||
|
||||
queueStore.setQueue(paths, 0, shuffle, this.queueSource());
|
||||
// so an empty set here is not a state the user can reach. The
|
||||
// shuffle-mode semantics live in `playAll`, shared with the
|
||||
// play-all/shuffle-all pair on every track list.
|
||||
playAll(this.ownedFilePaths(), this.queueSource(), shuffle);
|
||||
}
|
||||
|
||||
/** Append what the user owns of this release to the queue. */
|
||||
|
||||
@@ -85,7 +85,9 @@ import {
|
||||
ICON_PLAYLIST,
|
||||
ICON_QUEUE,
|
||||
ICON_REMOVE,
|
||||
ICON_SHUFFLE,
|
||||
} from '@utils/icon-language';
|
||||
import { playAll } from '@utils/play-all';
|
||||
|
||||
/** One playlist row: the track and its position in the *playlist*,
|
||||
* which is not its position in the filtered view. */
|
||||
@@ -355,14 +357,32 @@ export class PlaylistDetails
|
||||
// Track interactions
|
||||
// =================================================================
|
||||
|
||||
private handlePlayAll() {
|
||||
const filePaths = this.tracks
|
||||
private playableFilePaths(): string[] {
|
||||
return this.tracks
|
||||
.filter((t) => !t.Phantom)
|
||||
.map((t) => t.FilePath);
|
||||
}
|
||||
|
||||
if (filePaths.length === 0) return;
|
||||
private handlePlayAll() {
|
||||
// Start at the first row, not at a random one: the old `true`
|
||||
// was `shuffleStart`, which only picks a random first track
|
||||
// when shuffle mode is already on — so "Play All" quietly did
|
||||
// "play from the top" while leaving the mode as it was. The
|
||||
// mode semantics now live in `playAll`, shared with the other
|
||||
// track lists.
|
||||
playAll(
|
||||
this.playableFilePaths(),
|
||||
{ type: 'playlist', id: this.playlistId, label: this.playlistName },
|
||||
false,
|
||||
);
|
||||
}
|
||||
|
||||
queueStore.setQueue(filePaths, 0, true, { type: 'playlist', id: this.playlistId, label: this.playlistName });
|
||||
private handleShuffleAll() {
|
||||
playAll(
|
||||
this.playableFilePaths(),
|
||||
{ type: 'playlist', id: this.playlistId, label: this.playlistName },
|
||||
true,
|
||||
);
|
||||
}
|
||||
|
||||
private handleTrackClick(
|
||||
@@ -1599,9 +1619,16 @@ export class PlaylistDetails
|
||||
class="play-all-button"
|
||||
@click=${() => this.handlePlayAll()}
|
||||
>
|
||||
<wa-icon name="play"></wa-icon>
|
||||
<wa-icon name=${ICON_PLAY}></wa-icon>
|
||||
Play All
|
||||
</button>
|
||||
<button
|
||||
class="play-all-button"
|
||||
@click=${() => this.handleShuffleAll()}
|
||||
>
|
||||
<wa-icon name=${ICON_SHUFFLE}></wa-icon>
|
||||
Shuffle All
|
||||
</button>
|
||||
</div>
|
||||
<div class="track-header">
|
||||
<div class="header-cell col-number">#</div>
|
||||
|
||||
@@ -15,6 +15,7 @@ import {
|
||||
import { EventsOn } from '@runtime/runtime';
|
||||
import { Events } from '../../events';
|
||||
import { queueStore } from '@store/queue-store';
|
||||
import { playAll } from '@utils/play-all';
|
||||
import { creditStore } from '@store/credit-store';
|
||||
import { PlayerController } from '@store/controllers/player-controller';
|
||||
import { SearchController } from '@store/controllers/search-controller';
|
||||
@@ -771,24 +772,29 @@ export class SmartPlaylistDetails
|
||||
// Actions
|
||||
// =================================================================
|
||||
|
||||
private handlePlay() {
|
||||
const filePaths = this.tracks
|
||||
private playableFilePaths(): string[] {
|
||||
return this.tracks
|
||||
.filter((t) => !t.Phantom)
|
||||
.map((t) => t.FilePath);
|
||||
}
|
||||
|
||||
if (filePaths.length === 0) return;
|
||||
|
||||
queueStore.setQueue(filePaths, 0, false, { type: 'smartPlaylist', id: this.playlistId, label: this.playlistName });
|
||||
private handlePlay() {
|
||||
playAll(
|
||||
this.playableFilePaths(),
|
||||
{ type: 'smartPlaylist', id: this.playlistId, label: this.playlistName },
|
||||
false,
|
||||
);
|
||||
}
|
||||
|
||||
private handleShuffle() {
|
||||
const filePaths = this.tracks
|
||||
.filter((t) => !t.Phantom)
|
||||
.map((t) => t.FilePath);
|
||||
|
||||
if (filePaths.length === 0) return;
|
||||
|
||||
queueStore.setQueue(filePaths, 0, true, { type: 'smartPlaylist', id: this.playlistId, label: this.playlistName });
|
||||
// This used to be a no-op when shuffle mode was off: it passed
|
||||
// `shuffleStart` without turning the mode on, so the queue
|
||||
// started at track 1 in order. `playAll` sets the mode first.
|
||||
playAll(
|
||||
this.playableFilePaths(),
|
||||
{ type: 'smartPlaylist', id: this.playlistId, label: this.playlistName },
|
||||
true,
|
||||
);
|
||||
}
|
||||
|
||||
private async handleRefresh() {
|
||||
|
||||
@@ -26,7 +26,10 @@ import type { ContextMenuHost, MenuTarget } from '@utils/context-menu-controller
|
||||
import { PlayerController } from '@store/controllers/player-controller';
|
||||
import { SearchController } from '@store/controllers/search-controller';
|
||||
import '@components/page-header/page-header';
|
||||
import type { SortOption } from '@components/page-header/page-header';
|
||||
import type {
|
||||
SortOption,
|
||||
PageAction,
|
||||
} from '@components/page-header/page-header';
|
||||
import { TrackListController } from '@store/controllers/tracklist-controller';
|
||||
import { FavoritesController } from '@store/controllers/favorites-controller';
|
||||
import { queueStore } from '@store/queue-store';
|
||||
@@ -87,7 +90,9 @@ import {
|
||||
ICON_PLAYLIST,
|
||||
ICON_PLAY_NEXT,
|
||||
ICON_QUEUE,
|
||||
ICON_SHUFFLE,
|
||||
} from '@utils/icon-language';
|
||||
import { playAll } from '@utils/play-all';
|
||||
|
||||
const COLUMN_STORAGE_KEY = 'track-list-column-widths';
|
||||
const SORT_FIELD_KEY = 'track-list-sort-field';
|
||||
@@ -2393,6 +2398,27 @@ export class TrackList
|
||||
.map((c) => ({ id: c.id, label: c.label })),
|
||||
];
|
||||
|
||||
const hasTracks = this.cachedSortedTracks.length > 0;
|
||||
|
||||
const actions: PageAction[] = [
|
||||
{
|
||||
id: 'play-all',
|
||||
label: 'Play all',
|
||||
icon: ICON_PLAY,
|
||||
priority: 1,
|
||||
disabled: !hasTracks,
|
||||
onSelect: this.handlePlayAll,
|
||||
},
|
||||
{
|
||||
id: 'shuffle-all',
|
||||
label: 'Shuffle all',
|
||||
icon: ICON_SHUFFLE,
|
||||
priority: 0,
|
||||
disabled: !hasTracks,
|
||||
onSelect: this.handleShuffleAll,
|
||||
},
|
||||
];
|
||||
|
||||
return html`
|
||||
<page-header
|
||||
heading=${this.externalTracks === undefined ? 'Tracks' : ''}
|
||||
@@ -2404,11 +2430,29 @@ export class TrackList
|
||||
sort-field=${this.sortField ?? ''}
|
||||
sort-direction=${this.sortDirection}
|
||||
search-term=${this.searchCtrl.term}
|
||||
.actions=${actions}
|
||||
@sort-change=${this.onPageHeaderSort}
|
||||
></page-header>
|
||||
`;
|
||||
}
|
||||
|
||||
/** The queue is the list as displayed, in the order the user sees. */
|
||||
private handlePlayAll = (): void => {
|
||||
playAll(
|
||||
this.cachedSortedTracks.map((t) => t.FilePath),
|
||||
this.effectiveQueueSource,
|
||||
false,
|
||||
);
|
||||
};
|
||||
|
||||
private handleShuffleAll = (): void => {
|
||||
playAll(
|
||||
this.cachedSortedTracks.map((t) => t.FilePath),
|
||||
this.effectiveQueueSource,
|
||||
true,
|
||||
);
|
||||
};
|
||||
|
||||
private onPageHeaderSort = (
|
||||
e: CustomEvent<{ field: string; direction: 'asc' | 'desc' }>,
|
||||
) => {
|
||||
|
||||
@@ -0,0 +1,26 @@
|
||||
import { queueStore } from '@store/queue-store';
|
||||
import type { QueueSource } from '@store/queue-store';
|
||||
|
||||
/**
|
||||
* Queue a list and start it, optionally shuffled.
|
||||
*
|
||||
* This is the one place that owns what "shuffle this collection" means.
|
||||
* `SetQueue`'s `shuffleStart` only picks a random first track when
|
||||
* shuffle mode is *already* on — it does not turn it on — so the mode
|
||||
* has to be set before the queue, not after. The album page used to
|
||||
* carry that rule privately; the play-all/shuffle-all pair on every
|
||||
* track list now shares it.
|
||||
*/
|
||||
export function playAll(
|
||||
paths: string[],
|
||||
source: QueueSource | undefined,
|
||||
shuffle: boolean,
|
||||
): void {
|
||||
if (paths.length === 0) return;
|
||||
|
||||
if (shuffle && !queueStore.getState().shuffleMode) {
|
||||
queueStore.toggleShuffle();
|
||||
}
|
||||
|
||||
queueStore.setQueue(paths, 0, shuffle, source);
|
||||
}
|
||||
@@ -0,0 +1,373 @@
|
||||
/**
|
||||
* The play-all/shuffle-all pair on every page that lists tracks.
|
||||
*
|
||||
* The pair is driven by one helper (`utils/play-all`) that owns the
|
||||
* one rule the album page already carried: `shuffleStart` does not turn
|
||||
* shuffle on, it only picks a random first track once the mode is on —
|
||||
* so the mode has to be set *before* the queue, not after. The hosts
|
||||
* differ only in where their paths come from and what `Source` they
|
||||
* hand over.
|
||||
*/
|
||||
import { describe, expect, it, beforeEach } from 'vitest';
|
||||
import type { LitElement } from 'lit';
|
||||
|
||||
import '@components/track-list/track-list';
|
||||
import '@components/artist-details/artist-details';
|
||||
import '@components/playlist-details/playlist-details';
|
||||
import '@components/smart-playlist-details/smart-playlist-details';
|
||||
import {
|
||||
stub,
|
||||
flush,
|
||||
resetHarness,
|
||||
calls,
|
||||
lastArgs,
|
||||
emit,
|
||||
} from '@test/support/harness';
|
||||
import {
|
||||
fixture,
|
||||
shadowAll,
|
||||
deepShadow,
|
||||
} from '@test/support/render';
|
||||
|
||||
/** The action button rendered by `<page-header>`, through the nested
|
||||
* shadow roots (track-list → page-header). */
|
||||
function pageAction(
|
||||
host: LitElement,
|
||||
id: string,
|
||||
): HTMLElement | null {
|
||||
return deepShadow<HTMLElement>(host, `[data-testid="page-action-${id}"]`);
|
||||
}
|
||||
|
||||
function setShuffleMode(on: boolean): void {
|
||||
emit('QueueModeChanged', { shuffleMode: on, repeatMode: 'off' });
|
||||
}
|
||||
|
||||
/** The queue's `SetQueue` args, with the shuffle flag and source. */
|
||||
function queued(): {
|
||||
paths: string[];
|
||||
startIndex: number;
|
||||
shuffleStart: boolean;
|
||||
source: unknown;
|
||||
} {
|
||||
const args = lastArgs('queue.Queue.SetQueue');
|
||||
|
||||
if (!args) throw new Error('nothing was queued');
|
||||
|
||||
return {
|
||||
paths: args[0] as string[],
|
||||
startIndex: args[1] as number,
|
||||
shuffleStart: args[2] as boolean,
|
||||
source: args[3],
|
||||
};
|
||||
}
|
||||
|
||||
// =====================================================================
|
||||
// The track list (Tracks, and every embedding detail view)
|
||||
// =====================================================================
|
||||
|
||||
const PATHS = Array.from({ length: 12 }, (_, i) => `/music/track-${i}.mp3`);
|
||||
|
||||
const LIST = PATHS.map((FilePath, i) => ({
|
||||
FilePath,
|
||||
TrackName: `Track ${i}`,
|
||||
ArtistName: 'An Artist',
|
||||
Album: 'An Album',
|
||||
Duration: 180,
|
||||
}));
|
||||
|
||||
const GENRE_SOURCE = { type: 'genre', id: 0, label: 'Dream Pop' };
|
||||
|
||||
async function embeddedTrackList(): Promise<LitElement> {
|
||||
resetHarness();
|
||||
localStorage.removeItem('track-list-column-widths');
|
||||
|
||||
const el = await fixture<LitElement>('track-list', {
|
||||
externalTracks: LIST,
|
||||
queueSource: GENRE_SOURCE,
|
||||
});
|
||||
|
||||
// Say which order is being asserted rather than inheriting a
|
||||
// persisted sort. See play-in-context.test.ts for the same trap.
|
||||
(el as unknown as { sortField: string | null }).sortField = null;
|
||||
|
||||
el.style.display = 'block';
|
||||
el.style.height = '600px';
|
||||
await flush();
|
||||
await el.updateComplete;
|
||||
await new Promise((r) => setTimeout(r, 60));
|
||||
|
||||
return el;
|
||||
}
|
||||
|
||||
describe('the track-list play-all/shuffle-all pair', () => {
|
||||
beforeEach(() => {
|
||||
resetHarness();
|
||||
localStorage.removeItem('track-list-column-widths');
|
||||
});
|
||||
|
||||
it('renders in the primary Tracks header and is disabled while empty', async () => {
|
||||
const el = await fixture<LitElement>('track-list', {});
|
||||
|
||||
const play = pageAction(el, 'play-all');
|
||||
const shuffle = pageAction(el, 'shuffle-all');
|
||||
|
||||
expect(play).not.toBeNull();
|
||||
expect(shuffle).not.toBeNull();
|
||||
expect(play?.hasAttribute('disabled')).toBe(true);
|
||||
expect(shuffle?.hasAttribute('disabled')).toBe(true);
|
||||
});
|
||||
|
||||
it('renders in an embedded track-list header too', async () => {
|
||||
const el = await embeddedTrackList();
|
||||
|
||||
expect(pageAction(el, 'play-all')).not.toBeNull();
|
||||
expect(pageAction(el, 'shuffle-all')).not.toBeNull();
|
||||
expect(pageAction(el, 'play-all')?.hasAttribute('disabled')).toBe(false);
|
||||
expect(pageAction(el, 'shuffle-all')?.hasAttribute('disabled')).toBe(false);
|
||||
});
|
||||
|
||||
it('Play all queues the displayed list in order, unshuffled', async () => {
|
||||
const el = await embeddedTrackList();
|
||||
|
||||
pageAction(el, 'play-all')!.click();
|
||||
await flush();
|
||||
|
||||
expect(queued()).toEqual({
|
||||
paths: PATHS,
|
||||
startIndex: 0,
|
||||
shuffleStart: false,
|
||||
source: GENRE_SOURCE,
|
||||
});
|
||||
});
|
||||
|
||||
it('Shuffle all turns shuffle on before queueing when the mode is off', async () => {
|
||||
const el = await embeddedTrackList();
|
||||
|
||||
setShuffleMode(false);
|
||||
await flush();
|
||||
|
||||
pageAction(el, 'shuffle-all')!.click();
|
||||
await flush();
|
||||
|
||||
const all = calls();
|
||||
const toggle = all.findLastIndex(
|
||||
(c) => c.path === 'queue.Queue.ToggleShuffle',
|
||||
);
|
||||
const setQueue = all.findLastIndex(
|
||||
(c) => c.path === 'queue.Queue.SetQueue',
|
||||
);
|
||||
|
||||
expect(toggle).toBeGreaterThanOrEqual(0);
|
||||
expect(toggle).toBeLessThan(setQueue);
|
||||
|
||||
expect(queued()).toEqual({
|
||||
paths: PATHS,
|
||||
startIndex: 0,
|
||||
shuffleStart: true,
|
||||
source: GENRE_SOURCE,
|
||||
});
|
||||
});
|
||||
|
||||
it('Shuffle all does not toggle when the mode is already on', async () => {
|
||||
const el = await embeddedTrackList();
|
||||
|
||||
setShuffleMode(true);
|
||||
await flush();
|
||||
|
||||
pageAction(el, 'shuffle-all')!.click();
|
||||
await flush();
|
||||
|
||||
expect(calls('queue.Queue.ToggleShuffle')).toHaveLength(0);
|
||||
|
||||
expect(queued()).toEqual({
|
||||
paths: PATHS,
|
||||
startIndex: 0,
|
||||
shuffleStart: true,
|
||||
source: GENRE_SOURCE,
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
// =====================================================================
|
||||
// The library artist page
|
||||
// =====================================================================
|
||||
|
||||
const ALBUMS = [
|
||||
{ ID: 3, Name: 'Third', ArtistName: 'Aurora Fields', ArtistMBID: '', MBID: '', CoverArtPath: '', CoverArtSmall: '', CoverArtMedium: '', CoverArtLarge: '', Year: 0, ReleaseYear: 0 },
|
||||
{ ID: 1, Name: 'First', ArtistName: 'Aurora Fields', ArtistMBID: '', MBID: '', CoverArtPath: '', CoverArtSmall: '', CoverArtMedium: '', CoverArtLarge: '', Year: 0, ReleaseYear: 0 },
|
||||
{ ID: 2, Name: 'Second', ArtistName: 'Aurora Fields', ArtistMBID: '', MBID: '', CoverArtPath: '', CoverArtSmall: '', CoverArtMedium: '', CoverArtLarge: '', Year: 0, ReleaseYear: 0 },
|
||||
];
|
||||
|
||||
describe('the artist page play-all pair', () => {
|
||||
beforeEach(() => {
|
||||
resetHarness();
|
||||
stub('explore.Service.GetArtistMBID', '');
|
||||
stub('library.Library.GetAlbumsByArtist', ALBUMS);
|
||||
stub('library.Library.GetFilePathsByAlbums', {
|
||||
'3': ['/a3-1', '/a3-2'],
|
||||
'1': ['/a1'],
|
||||
'2': ['/a2-1', '/a2-2', '/a2-3'],
|
||||
});
|
||||
});
|
||||
|
||||
it('flattens album paths in the album list order', async () => {
|
||||
const el = await fixture<LitElement>('artist-details', {
|
||||
artistId: 7,
|
||||
artistName: 'Aurora Fields',
|
||||
artistMBID: '',
|
||||
});
|
||||
|
||||
await flush();
|
||||
await el.updateComplete;
|
||||
|
||||
const play = shadowAll<HTMLElement>(el, '[data-testid="artist-play-all"]')[0];
|
||||
|
||||
play!.click();
|
||||
await flush();
|
||||
|
||||
expect(queued()).toEqual({
|
||||
paths: ['/a3-1', '/a3-2', '/a1', '/a2-1', '/a2-2', '/a2-3'],
|
||||
startIndex: 0,
|
||||
shuffleStart: false,
|
||||
source: { type: 'artist', id: 7, label: 'Aurora Fields' },
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
// =====================================================================
|
||||
// A smart playlist
|
||||
// =====================================================================
|
||||
|
||||
function smartPlaylistTracks(n: number) {
|
||||
return Array.from({ length: n }, (_, i) => ({
|
||||
ID: i + 1,
|
||||
FilePath: `/music/track-${i}.mp3`,
|
||||
Title: `Track ${i}`,
|
||||
Artist: 'An Artist',
|
||||
Album: 'An Album',
|
||||
Duration: 180000,
|
||||
CoverArtSmall: `/covers/${i}_sm.jpg`,
|
||||
CoverArtMedium: `/covers/${i}_md.jpg`,
|
||||
CoverArtPath: `/covers/${i}.jpg`,
|
||||
Phantom: false,
|
||||
}));
|
||||
}
|
||||
|
||||
describe('the smart-playlist play-all pair', () => {
|
||||
beforeEach(() => {
|
||||
resetHarness();
|
||||
stub('playlist.Service.GetSmartPlaylistTracks', smartPlaylistTracks(8));
|
||||
stub('playlist.Service.GetSmartPlaylistRules', '{"rules":[]}');
|
||||
stub('playlist.Service.GetAllPlaylists', []);
|
||||
});
|
||||
|
||||
/** The details header's own action row, not a page-header action. */
|
||||
function actionButton(el: LitElement, label: string): HTMLElement {
|
||||
const button = shadowAll<HTMLElement>(el, '.action-button').find(
|
||||
(b) => b.textContent?.trim() === label,
|
||||
);
|
||||
|
||||
if (!button) throw new Error(`no "${label}" action button rendered`);
|
||||
|
||||
return button;
|
||||
}
|
||||
|
||||
it('Shuffle turns the mode on before queueing, so the queue starts shuffled', async () => {
|
||||
const el = await fixture<LitElement>('smart-playlist-details', {
|
||||
playlistId: 1,
|
||||
playlistName: 'A smart playlist',
|
||||
});
|
||||
|
||||
el.style.display = 'block';
|
||||
el.style.height = '600px';
|
||||
await flush();
|
||||
await el.updateComplete;
|
||||
await new Promise((r) => setTimeout(r, 60));
|
||||
|
||||
setShuffleMode(false);
|
||||
await flush();
|
||||
|
||||
actionButton(el, 'Shuffle').click();
|
||||
await flush();
|
||||
|
||||
// The issue's headline fix. `SetQueue`'s `shuffleStart` only picks
|
||||
// a random first track when shuffle mode is already on — it does
|
||||
// not turn it on — so reverting the mode toggle puts the queue back
|
||||
// to track 1 in order while every assertion about the queue's
|
||||
// contents still passes. Order of the calls is the assertion.
|
||||
const all = calls();
|
||||
const toggle = all.findLastIndex(
|
||||
(c) => c.path === 'queue.Queue.ToggleShuffle',
|
||||
);
|
||||
const setQueue = all.findLastIndex(
|
||||
(c) => c.path === 'queue.Queue.SetQueue',
|
||||
);
|
||||
|
||||
expect(toggle).toBeGreaterThanOrEqual(0);
|
||||
expect(toggle).toBeLessThan(setQueue);
|
||||
|
||||
expect(queued()).toEqual({
|
||||
paths: Array.from({ length: 8 }, (_, i) => `/music/track-${i}.mp3`),
|
||||
startIndex: 0,
|
||||
shuffleStart: true,
|
||||
source: { type: 'smartPlaylist', id: 1, label: 'A smart playlist' },
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
// =====================================================================
|
||||
// A regular playlist
|
||||
// =====================================================================
|
||||
|
||||
function playlistTracks(n: number) {
|
||||
return Array.from({ length: n }, (_, i) => ({
|
||||
ID: i + 1,
|
||||
FilePath: `/music/track-${i}.mp3`,
|
||||
Title: `Track ${i}`,
|
||||
Artist: 'An Artist',
|
||||
Album: 'An Album',
|
||||
Duration: 180000,
|
||||
Phantom: false,
|
||||
}));
|
||||
}
|
||||
|
||||
describe('the playlist play-all pair', () => {
|
||||
beforeEach(() => {
|
||||
resetHarness();
|
||||
stub('playlist.Service.GetPlaylistTracks', playlistTracks(8));
|
||||
stub('playlist.Service.GetAllPlaylists', []);
|
||||
});
|
||||
|
||||
it('offers Shuffle All beside Play All, both through the helper', async () => {
|
||||
const el = await fixture<LitElement>('playlist-details', {
|
||||
playlistId: 1,
|
||||
playlistName: 'A playlist',
|
||||
});
|
||||
|
||||
el.style.display = 'block';
|
||||
el.style.height = '600px';
|
||||
await flush();
|
||||
await el.updateComplete;
|
||||
await new Promise((r) => setTimeout(r, 60));
|
||||
|
||||
const buttons = shadowAll<HTMLElement>(el, '.play-all-button');
|
||||
|
||||
expect(buttons.map((b) => b.textContent?.trim())).toEqual([
|
||||
'Play All',
|
||||
'Shuffle All',
|
||||
]);
|
||||
|
||||
setShuffleMode(false);
|
||||
await flush();
|
||||
|
||||
buttons[1]!.click();
|
||||
await flush();
|
||||
|
||||
expect(queued()).toEqual({
|
||||
paths: Array.from({ length: 8 }, (_, i) => `/music/track-${i}.mp3`),
|
||||
startIndex: 0,
|
||||
shuffleStart: true,
|
||||
source: { type: 'playlist', id: 1, label: 'A playlist' },
|
||||
});
|
||||
});
|
||||
});
|
||||
+7
-3
@@ -38,10 +38,14 @@ pre-commit:
|
||||
glob: "*.go"
|
||||
run: ./scripts/bindings-check.sh
|
||||
|
||||
# .pi/ documents make targets; a stale one sends an agent off a
|
||||
# cliff with total confidence. Instant.
|
||||
# The docs document make targets; a stale one sends an agent — or a
|
||||
# contributor reading CONTRIBUTING.md — off a cliff with total
|
||||
# confidence. The glob is the script's own scanned set, because a
|
||||
# hook that does not fire on a file the check reads is the drift the
|
||||
# check exists to prevent: it was `{Makefile,.pi/**/*.md}` while the
|
||||
# script already read CLAUDE.md. Instant.
|
||||
skill-check:
|
||||
glob: "{Makefile,.pi/**/*.md}"
|
||||
glob: "{Makefile,.pi/**/*.md,AGENTS.md,CLAUDE.md,README.md,CONTRIBUTING.md}"
|
||||
run: ./scripts/skill-check.sh
|
||||
|
||||
frontend-typecheck:
|
||||
|
||||
+21
-4
@@ -14,6 +14,11 @@
|
||||
# missing: CLAUDE.md names 27 targets and nothing verified one of them,
|
||||
# so the file the agents trust most was the file least checked.
|
||||
#
|
||||
# README.md and CONTRIBUTING.md are in it too, and the header sentence
|
||||
# above is why: a person who has *not* read the Makefile goes looking in
|
||||
# the contributor-facing doc, so a renamed target sends them off the
|
||||
# same cliff it sends an agent off. CONTRIBUTING.md names 21 targets.
|
||||
#
|
||||
# **AGENTS.md is a symlink to CLAUDE.md.** This repo is worked on by
|
||||
# two agent harnesses that read different files by convention — Claude
|
||||
# Code reads CLAUDE.md, others read AGENTS.md — and two harnesses
|
||||
@@ -43,7 +48,19 @@ if [ -e AGENTS.md ] || [ -L AGENTS.md ]; then
|
||||
fi
|
||||
fi
|
||||
|
||||
[ -d .pi ] || exit 0
|
||||
# The scan is over the docs that are actually there: a checkout without
|
||||
# .pi/ still has README.md and CONTRIBUTING.md to check, and gating the
|
||||
# whole run on .pi/ would have made the human-facing half conditional on
|
||||
# the agent-facing one. This list is used twice — once to read the
|
||||
# mentions out and once to say which file a missing target came from —
|
||||
# because a second list is a second thing to forget.
|
||||
# `ls` exits non-zero when *any* of its arguments is missing while still
|
||||
# printing the ones that are there, and under `set -e` that would sink
|
||||
# the assignment rather than scanning what exists, so swallow it.
|
||||
docs="$({ find .pi -name '*.md' 2>/dev/null
|
||||
ls CLAUDE.md README.md CONTRIBUTING.md 2>/dev/null || true; })"
|
||||
|
||||
[ -n "$docs" ] || exit 0
|
||||
|
||||
# `make -pq` prints the database including every rule, without running
|
||||
# anything. It exits non-zero when a target is out of date, and under
|
||||
@@ -68,7 +85,7 @@ targets="$({ make -pqRr 2>/dev/null || true; } |
|
||||
# AGENTS.md is deliberately not in this list: it is a symlink to
|
||||
# CLAUDE.md, asserted above, so scanning it would report every failure
|
||||
# twice under two names.
|
||||
mentioned="$({ find .pi -name '*.md' 2>/dev/null; echo CLAUDE.md; } |
|
||||
mentioned="$(printf '%s\n' "$docs" |
|
||||
xargs awk '
|
||||
FNR == 1 { fence = 0 }
|
||||
/^```/ { fence = !fence; next }
|
||||
@@ -93,10 +110,10 @@ for t in $mentioned; do
|
||||
done
|
||||
|
||||
if [ -n "$missing" ]; then
|
||||
echo "skill-check: the agent docs name make targets that do not exist:" >&2
|
||||
echo "skill-check: the docs name make targets that do not exist:" >&2
|
||||
for t in $missing; do
|
||||
echo " make $t" >&2
|
||||
grep -rln "make $t" .pi CLAUDE.md --include='*.md' | sed 's/^/ /' >&2
|
||||
printf '%s\n' "$docs" | xargs grep -ln "make $t" | sed 's/^/ /' >&2
|
||||
done
|
||||
echo "Fix the docs, or restore the target." >&2
|
||||
exit 1
|
||||
|
||||
Reference in New Issue
Block a user