Compare commits

..
Author SHA1 Message Date
logan 1f43234b80 fix(loop): document the model fallback chain and foreground launches
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 3m23s
CI / e2e (pull_request) Successful in 12m13s
Two #31-tick findings that would strand an unattended run. The qwen
worker hit its weekly 429 mid-tick; the obvious fallback
deepseek/deepseek-v4-pro is wrong because the deepseek provider has no
models (only catalog overrides) and fails silently — the model lives on
the go gateway as go/deepseek-v4-pro, with go/glm-5.3-flash the next
rung. And the async subagent runner has died without persisting a
session, so legs launch in the foreground and a dead worker is recovered
by completing, never re-implementing.

Closes #243
2026-09-03 23:20:33 -04:00
logan ca00f8a803 Merge pull request 'feat(library): play all and shuffle all on every track list' (#242) from feat/31-play-all-shuffle-all into main
CI / check (push) Successful in 3m12s
CI / e2e (push) Successful in 11m24s
default
2026-09-04 02:48:27 +00:00
logan 0be7b4fdc8 feat(library): play all and shuffle all on every track list
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 3m38s
CI / e2e (pull_request) Successful in 12m3s
Four pages that list tracks — Tracks, genre, artist, and both playlist
views — had no way to start the whole list, or had a broken one. One
shared helper (utils/play-all.ts) now 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 is toggled before the queue is set. Each host passes an honest
queue Source (#14): anything that builds a queue names what it built it
from, so "Playing from" stops lying.

Two behaviour changes ride along, both flagged: smart-playlist-details'
Shuffle was a live no-op (shuffleStart without enabling mode played
track 1 in order) and is fixed; playlist-details' Play all drops its
shuffleStart:true, so with shuffle mode already on it now starts at the
first row instead of a random one — the album page's existing
semantics.

Verified: make ui-test (1147, incl. a case that fails when the
smart-playlist fix is reverted), npx tsc --noEmit, make e2e (255,
incl. new play-all and header-fit specs), make lint, make test,
make bindings-check, make css-check; artist header read from
screenshots at 424/320/900 (the pair wraps below the name on a phone).

Closes #31
2026-09-03 17:06:55 -04:00
logan 18f10e966b Merge pull request 'fix(loop): worktree provisioning, fresh fetch, corruption halt' (#241) from fix/240-loop-operational-fixes into main
CI / check (push) Successful in 3m13s
CI / e2e (push) Successful in 10m44s
default
2026-09-03 18:17:02 +00:00
logan e897364a73 fix(loop): worktree provisioning, fresh fetch, corruption halt
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 3m18s
CI / e2e (pull_request) Successful in 11m3s
Three P1 adoption-wave findings, all of which break an unattended run.
The worktree needs make build-frontend and make testdata once before its
first push — the pre-push go-test hook embeds frontend/dist and the
fixture library and refused the push without them. The merge leg now
says to fetch origin/main in the same breath as the refresh merge: a
cached origin/main merged against the wrong base, CI went green on it,
and the merge came back 405 "behind base" — a full cycle wasted. And a
killed fetch leaves 0-byte objects in the shared store whose signature
(error: object file … is empty / unpack-objects failed) must halt the
loop for a human instead of churning, with the repair recipe written
down.

Closes #240
2026-09-03 13:56:58 -04:00
logan 5fa68fcf43 Merge pull request 'ci(skill-check): scan the docs a contributor reads' (#229) from docs/220-skill-check-scope into main
CI / check (push) Successful in 3m6s
CI / e2e (push) Successful in 10m59s
default
2026-09-03 17:35:40 +00:00
logan b1368bbc7e Merge remote-tracking branch 'origin/main' into docs/220-skill-check-scope
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 3m10s
CI / e2e (pull_request) Successful in 11m6s
2026-09-03 13:20:23 -04:00
logan 9432f68c8b Merge remote-tracking branch 'origin/main' into docs/220-skill-check-scope
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 3m11s
CI / e2e (pull_request) Successful in 10m44s
2026-09-03 13:04:17 -04:00
logan 4c921ed1ba Merge pull request 'fix(config): put the old value back when a setter is rejected' (#233) from fix/231-setter-rollback into main
CI / check (push) Successful in 3m52s
CI / e2e (push) Successful in 11m13s
default
2026-09-03 16:48:05 +00:00
logan f8800ca1f8 Merge remote-tracking branch 'origin/main' into fix/231-setter-rollback
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 3m12s
CI / e2e (pull_request) Successful in 13m24s
2026-09-03 12:29:50 -04:00
logan dddc8aaf55 Merge pull request 'fix(settings): stop offering a column the backend rejects' (#232) from fix/197-duplicate-column-label into main
CI / check (push) Successful in 3m7s
CI / e2e (push) Successful in 11m3s
default
2026-09-03 16:01:50 +00:00
logan f6e9df2f68 Merge remote-tracking branch 'origin/main' into fix/197-duplicate-column-label
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 3m7s
CI / e2e (pull_request) Successful in 10m54s
2026-09-03 11:44:45 -04:00
logan 47f65dad89 Merge pull request 'fix(shell): dismiss the wizard when a library exists' (#234) from fix/175-wizard-follows-the-library into main
CI / check (push) Successful in 3m17s
CI / e2e (push) Successful in 11m51s
default
2026-09-03 15:28:32 +00:00
logan f5dae71050 Merge remote-tracking branch 'origin/main' into fix/175-wizard-follows-the-library
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 4m31s
CI / e2e (pull_request) Successful in 12m3s
2026-09-03 11:09:01 -04:00
logan b0bda625e0 Merge pull request 'test(download): write the yt-dlp stub under ForkLock' (#235) from fix/146-stub-etxtbsy into main
CI / check (push) Successful in 3m12s
CI / e2e (push) Successful in 11m5s
default
2026-09-03 14:54:13 +00:00
logan 19ba5f0394 Merge remote-tracking branch 'origin/main' into fix/146-stub-etxtbsy
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 2m59s
CI / e2e (pull_request) Successful in 11m1s
2026-09-03 10:38:47 -04:00
logan 439a6cd77b Merge pull request 'docs(skill): the WAV fixtures scan tagged, and have since #104' (#230) from docs/225-fixtures-wav-tags into main
CI / check (push) Successful in 3m20s
CI / e2e (push) Successful in 11m7s
default
2026-09-03 14:22:59 +00:00
logan 49445ded77 test(download): write the yt-dlp stub under ForkLock
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 2m48s
CI / e2e (pull_request) Successful in 10m21s
The kernel refuses to exec a file that is open for writing anywhere in
the process, and these tests are parallel: a sibling's fork duplicates
stubYtDlp's write descriptor in the moment it is open and carries it
past our close, so the exec a moment later fails with ETXTBSY. That is
the flake seen once locally and once in CI, both times on a tree with
no Go in its diff.

Closing sooner is not available -- os.WriteFile has already closed the
file before anything execs it -- and O_CLOEXEC does not help, because
the window is between another goroutine's fork and its own exec.
syscall.ForkLock is the lock forkExec takes across that fork, so
holding it over the write means no child can exist while the
descriptor does.

Measured on the helper itself under 12 concurrent writers: 176-189 of
2400 execs refused before, 0 of 2400 after, three runs each.

Closes #146
2026-08-30 07:40:32 -04:00
logan b9e60bdb0a fix(shell): dismiss the wizard when a library exists
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 2m58s
CI / e2e (pull_request) Successful in 10m25s
The first-run wizard dismissed itself as a step in its own flow, so a
library appearing by any other route left a full-screen modal up over
an app that was already set up — intercepting every pointer event,
which is also what keeps Settings out of reach while it is there.

AddLibrary emits LibraryAdded whoever calls it, so one subscription
makes the dismissal follow the state the wizard exists to wait for
rather than the button being pressed. It is registered before the
initial read, or a library arriving while that call is in flight is
answered with a stale empty list; both routes out now end in one
dismiss(), which sets `finished` before asking the dialog to close
because preventClose cancels the hide otherwise.

Closes #175
2026-08-30 06:40:32 -04:00
logan bcf3856b6f fix(config): put the old value back when a setter is rejected
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 2m46s
CI / e2e (pull_request) Successful in 10m7s
Config.Save() validates the whole config, so a setter that assigned
before validating did not merely fail its own call: the rejected value
stayed in memory and failed every later save, of every unrelated
setting, silently and for the rest of the session. Nothing reached
disk, so a restart cleared it — which is what made the fault invisible
and unreportable.

The defect is precisely "assignment precedes a validation that can
reject that argument", and that predicate enumerates seven setters
rather than the whole file. Each snapshots the field and restores it on
the error path.

The remaining setters were read rather than assumed and are unchanged:
shortcuts.Config.Validate returns nil unconditionally, the bools and
SetFavoritesPlaylistID pass through no validation that inspects them,
Config.Validate does not validate Downloads at all, and SetViewVisible
refuses an unknown, non-hideable or launch-page view before assigning.
SetLibraryDirectory was already correct and is the precedent the new
comment points at: it validates a candidate before assigning, so there
is nothing to undo.

The rationale sits above the setter section rather than on Save(),
which is bound — a doc comment there renders into frontend/bindings
for an audience with no use for it.

Closes #231
2026-08-30 05:40:36 -04:00
logan d225f922fb fix(settings): stop offering a column the backend rejects
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 2m50s
CI / e2e (pull_request) Successful in 10m11s
Settings -> Track List Columns listed two rows both called "Track
Name", one of which could not be ticked, and a screen reader heard
"Show the Track Name column" twice with nothing to tell them apart.

They are `titleArtist` and `trackName`, and the issue left open which
way to fix it: a second label so the sort dropdown and the configurator
can say different words, or drop the row because the user cannot select
it. The code settles that. `titleArtist` is not in
`tracklist.AllColumnIDs`, so `Config.Validate()` returns `unknown
track-list column ID: "titleArtist"` -- ticking the row sends a column
set the backend rejects, `config-page` swallows the rejection into a
`console.error`, and the tick reverts. A second label would have named
a control that cannot work.

So a definition says whether it is a *choice*, and the configurator
reads `CONFIGURABLE_COLUMN_IDS` rather than `Object.keys(COLUMN_DEFS)`.

The new test reads Go's own list out of `backend/tracklist/config.go`
rather than writing it down a third time, since a third copy is the
fault one step earlier. Verified by planting: with the filter removed
all four assertions fail with the defect's own numbers.

Not fixed here, and filed as #231: `SetTrackListColumns` assigns before
it validates, so a rejected list stays in memory and `Save()` validates
the whole config -- one tick and no setting saves for the rest of the
session. That is reachable from any invalid input, not from this row.

Closes #197
2026-08-30 04:41:54 -04:00
logan 26251badda ci(skill-check): scan the docs a contributor reads
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 3m0s
CI / e2e (pull_request) Successful in 10m18s
The check asserts that every make target named in a doc exists, and its
scanned set was .pi/ plus CLAUDE.md.  Since #50, CONTRIBUTING.md is the
document a *human* goes to for a build command, and it names 21 targets
that nothing verified; README.md names none today and is in for the same
reason.  The script's own header sentence is the argument — a renamed
target sends a person off the same cliff it sends an agent off.

The file list is now one `docs` variable used twice, because the failure
message carried a second copy of it and a second list is a second thing
to forget.  The `[ -d .pi ]` guard went with it: gating the whole run on
.pi/ would make the human-facing half conditional on the agent-facing
one, and an empty list is the same "nothing to scan" exit without the
coupling.

The lefthook glob is that scanned set now rather than
{Makefile,.pi/**/*.md} — #220's smaller half, and it did not fire on
CLAUDE.md either, which the script had read for months.

Verified by planting a bad target rather than by reading the diff: both
matched forms in each of the four scanned surfaces, each naming the
right file; the same two plants pass on the pre-change script; unfenced
prose still does not match; and the hook fires on a staged
CONTRIBUTING.md under the new glob where the old one skipped it.  The
count is unchanged at 47 — the set is a union — so coverage is the only
thing that moved.

Closes #220
2026-08-28 03:38:28 -04:00
24 changed files with 1726 additions and 67 deletions
+37 -4
View File
@@ -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.
+44 -1
View File
@@ -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
@@ -3286,6 +3308,27 @@ its own duplicates apart) — and changing either is invisible against an
existing `YJ_HOME`, whose `config.toml` already holds the old list, so
`make sandbox-seed NAME=default` before believing the app.
**And the *valid* columns are declared twice too, which is the pair
that drifted.** `tracklist.AllColumnIDs` is what the backend accepts;
`COLUMN_DEFS` is what the frontend knows how to draw, and they are not
the same set — `titleArtist` is a definition and not a choice, since it
is the phone's stacked column and is picked by width in
`PHONE_COLUMN_IDS`. Settings built its list from `Object.keys(
COLUMN_DEFS)` and so offered it: **two rows both called "Track Name"**
(#197), the second unselectable, because ticking it sends a column set
Go rejects with `unknown track-list column ID` and `config-page`
swallows that into a `console.error`. `CONFIGURABLE_COLUMN_IDS` is what
the configurator reads now, derived from a `configurable` flag on the
definition, and `settings-column-list.test.ts` reads Go's own list out
of the source rather than writing it down a third time — the rule being
about every column, so checking one checks nothing.
One thing it does **not** fix, because it is reachable from any invalid
input rather than from that row: `SetTrackListColumns` assigns before it
validates, so a rejected list stays in memory and `Save()` validates the
whole config — one tick and **no setting saves for the rest of the
session**, silently. That is #231.
**Event-driven communication**: Backend emits events via Wails runtime; frontend stores subscribe to them. Event names are constants in `backend/events/`.
`frontend/src/events.ts` is **generated** from `backend/events/events.go`
+1 -1
View File
@@ -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
+39
View File
@@ -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,
)
+262
View File
@@ -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)
}
+21 -3
View File
@@ -7,6 +7,7 @@ import (
"path/filepath"
"runtime"
"strings"
"syscall"
"testing"
)
@@ -17,6 +18,21 @@ import (
// stubYtDlp writes an executable script that echoes the given stdout
// and returns it as a provider config binary path.
//
// The write is held under syscall.ForkLock, and that is not tidiness:
// the kernel refuses to exec a file that is open for writing anywhere
// in the process, and these tests are parallel, so a *sibling* test's
// fork can duplicate this descriptor in the moment it is open and
// carry it past our close — the exec a moment later then fails with
// ETXTBSY, "text file busy". That is #146, seen once in CI and once
// locally, on trees containing no Go at all. Closing sooner is not
// available (os.WriteFile has already closed the file before anything
// execs it) and O_CLOEXEC does not help, because the window is between
// another goroutine's fork and its own exec. ForkLock is the lock
// syscall.forkExec takes across that fork, so holding it here means no
// child can exist while the descriptor does. Measured on this helper
// under 12 concurrent writers: 176-189 of 2400 execs refused without
// it, 0 of 2400 with it.
func stubYtDlp(t *testing.T, script string) string {
t.Helper()
@@ -26,9 +42,11 @@ func stubYtDlp(t *testing.T, script string) string {
path := filepath.Join(t.TempDir(), "yt-dlp")
if err := os.WriteFile(
path, []byte("#!/bin/sh\n"+script), 0o700,
); err != nil {
syscall.ForkLock.Lock()
err := os.WriteFile(path, []byte("#!/bin/sh\n"+script), 0o700)
syscall.ForkLock.Unlock()
if err != nil {
t.Fatalf("write stub: %v", err)
}
+59 -4
View File
@@ -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();
}
});
});
+220
View File
@@ -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,
});
});
});
+1 -1
View File
@@ -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 artists 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>
`;
}
}
@@ -51,7 +51,7 @@ import type { BackgroundShade } from '@store/theme-store';
import type { IconStyle } from '@store/favorites-store';
import {
COLUMN_DEFS,
ALL_COLUMN_IDS,
CONFIGURABLE_COLUMN_IDS,
} from '@components/track-list/columns';
import './config-field';
@@ -1640,7 +1640,7 @@ export class ConfigPage extends ViewLifecycleMixin(LitElement) {
...this.trackListCtrl.columnIds,
];
const disabledIds = ALL_COLUMN_IDS.filter(
const disabledIds = CONFIGURABLE_COLUMN_IDS.filter(
(id) => !enabledIds.includes(id),
);
@@ -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. */
@@ -2,12 +2,14 @@ import { LitElement, html, css, nothing } from 'lit';
import { customElement, state, query } from 'lit/decorators.js';
import '@awesome.me/webawesome/dist/components/dialog/dialog.js';
import '@awesome.me/webawesome/dist/components/icon/icon.js';
import { EventsOn } from '@runtime/runtime';
import {
AddLibrary,
GetAllLibrariesWithTrackCounts,
} from '@go/library/library.js';
import { describeError, explainError } from '@utils/describe-error';
import { nameDialogsIn } from '@utils/name-dialog';
import { Events } from '../../events';
import { pickDirectory } from '../../utils/pick-directory';
/**
@@ -19,6 +21,13 @@ import { pickDirectory } from '../../utils/pick-directory';
* prompting the user to pick their music folder, registers it through the
* library CRUD API, and dismisses itself. AddLibrary emits LibraryAdded
* and kicks off the initial scan automatically.
*
* **The dismissal follows the library existing, not the button being
* pressed.** `AddLibrary` emits `LibraryAdded` whoever calls it, so the
* wizard waits on the state it exists to wait for rather than on a step
* in its own flow — a library arriving by any other route (Settings, a
* direct call) leaves a full-screen modal up otherwise, intercepting
* every pointer event.
*/
@customElement('first-run-wizard')
export class FirstRunWizard extends LitElement {
@@ -37,9 +46,18 @@ export class FirstRunWizard extends LitElement {
/** Error message from a failed pick/save, if any. */
@state() private errorMessage = '';
/** Unsubscribe from LibraryAdded, while this element is connected. */
private cancelLibraryAdded?: () => void;
override async connectedCallback(): Promise<void> {
super.connectedCallback();
// Subscribed before the read below, so a library arriving while
// that call is in flight is not answered with a stale empty list.
this.cancelLibraryAdded = EventsOn(Events.LibraryAdded, () => {
this.dismiss();
});
try {
const existing = await GetAllLibrariesWithTrackCounts();
@@ -54,6 +72,8 @@ export class FirstRunWizard extends LitElement {
return;
}
if (this.finished) return;
this.active = true;
await this.updateComplete;
@@ -61,6 +81,13 @@ export class FirstRunWizard extends LitElement {
if (this.dialog) this.dialog.open = true;
}
override disconnectedCallback(): void {
this.cancelLibraryAdded?.();
this.cancelLibraryAdded = undefined;
super.disconnectedCallback();
}
static override styles = css`
wa-dialog {
--width: 480px;
@@ -239,6 +266,20 @@ export class FirstRunWizard extends LitElement {
if (!this.finished) e.preventDefault();
};
/**
* Close, and stay closed: a library exists, so setup is over.
*
* `finished` is set first, or `preventClose` cancels the hide this
* asks for.
*/
private dismiss(): void {
this.finished = true;
if (this.dialog) this.dialog.open = false;
this.active = false;
}
private handleChoose = async (): Promise<void> => {
this.errorMessage = '';
@@ -264,11 +305,7 @@ export class FirstRunWizard extends LitElement {
try {
await AddLibrary(this.selectedDirectory);
this.finished = true;
if (this.dialog) this.dialog.open = false;
this.active = false;
this.dismiss();
} catch (err) {
this.errorMessage = explainError(
err,
@@ -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() {
+29 -7
View File
@@ -32,6 +32,18 @@ export interface ColumnDef {
id: string;
/** Human-readable header label. */
label: string;
/**
* Whether Settings may offer this column. Defaults to true.
*
* A definition is not the same thing as a *choice*. `titleArtist`
* is the phone's stacked column, picked by width in
* `PHONE_COLUMN_IDS`, and `tracklist.AllColumnIDs` in Go does not
* list it — so a tick in the configurator sends a column set the
* backend rejects with `unknown track-list column ID`, the tick
* reverts on the next render, and the only trace is a
* `console.error` (#197).
*/
configurable?: boolean;
/** Extracts the display value from a track. */
accessor: (track: library.Track) => string;
/** Default CSS width (used when no saved width exists). */
@@ -98,11 +110,16 @@ export const COLUMN_DEFS: Record<string, ColumnDef> = {
},
titleArtist: {
id: 'titleArtist',
// Named for what it sorts by, since that is the only place the
// label is user-visible: the phone has no column headers, and
// the page header's sort list is built from the *configured*
// columns rather than the drawn ones.
// Named for what it sorts by. That label is drawn nowhere
// today: the phone has no column headers, and the page header's
// sort list is built from the *configured* columns, which this
// one can never be — see `configurable` below.
label: 'Track Name',
// Chosen by width, never by the user, and rejected by the
// backend if it ever were. #197: Settings listed it anyway, so
// there were two rows called "Track Name" and the second one
// could not be selected.
configurable: false,
accessor: (t) => t.TrackName,
defaultWidth: '1fr',
comparator: (a, b) => compareStr(a.TrackName, b.TrackName),
@@ -266,10 +283,15 @@ export const COLUMN_DEFS: Record<string, ColumnDef> = {
};
/**
* All column IDs in default display order.
* Used by the settings UI to list available columns.
* The column IDs Settings may offer, in default display order.
*
* Not every definition is one: a column the user cannot choose has no
* row in the configurator, because a checkbox that cannot change
* anything is worse than an absent one — see `ColumnDef.configurable`.
*/
export const ALL_COLUMN_IDS: string[] = Object.keys(COLUMN_DEFS);
export const CONFIGURABLE_COLUMN_IDS: string[] = Object.keys(
COLUMN_DEFS,
).filter((id) => COLUMN_DEFS[id]?.configurable !== false);
/**
* Column IDs that are always searched regardless of visibility.
@@ -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' }>,
) => {
+26
View File
@@ -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,123 @@
/**
* #175: the first-run wizard's dismissal follows the library existing,
* not its own button being pressed.
*
* The wizard is a modal that blocks every pointer event, so a library
* arriving by another route — Settings, a direct call — used to leave
* it up over an app that was already set up. `LibraryAdded` is emitted
* by `AddLibrary` whoever calls it, which is what makes one
* subscription the whole fix.
*/
import { beforeEach, describe, expect, it } from 'vitest';
import { Events } from '../../src/events';
import { emit, stub } from '../support/harness';
import { fixture, shadow, shadowAll } from '../support/render';
import { wails } from '../support/wails-fake';
import '@components/first-run-wizard/first-run-wizard';
import type { FirstRunWizard } from '@components/first-run-wizard/first-run-wizard';
/** A library row, as `GetAllLibrariesWithTrackCounts` returns one. */
const aLibrary = {
id: 1,
name: 'Music',
path: '/home/logan/Music',
trackCount: 9,
};
/** Mount the wizard on a fresh install: no libraries yet. */
async function wizardOnAFreshInstall(): Promise<FirstRunWizard> {
stub('library.Library.GetAllLibrariesWithTrackCounts', []);
return fixture<FirstRunWizard>('first-run-wizard');
}
/** Whether the wizard is rendering its modal at all. */
function isShowing(el: FirstRunWizard): boolean {
return shadow(el, 'wa-dialog') !== null;
}
beforeEach(() => {
stub('library.Library.AddLibrary', aLibrary);
});
describe('first-run-wizard', () => {
it('shows on a fresh install and stays up until a library exists', async () => {
const el = await wizardOnAFreshInstall();
expect(isShowing(el)).toBe(true);
});
it('stays hidden when a library is already configured', async () => {
stub('library.Library.GetAllLibrariesWithTrackCounts', [aLibrary]);
const el = await fixture<FirstRunWizard>('first-run-wizard');
expect(isShowing(el)).toBe(false);
});
it('dismisses when a library appears by another route', async () => {
const el = await wizardOnAFreshInstall();
expect(isShowing(el)).toBe(true);
emit(Events.LibraryAdded, aLibrary);
await el.updateComplete;
expect(isShowing(el)).toBe(false);
});
it('does not raise itself when a library arrives while it is asking', async () => {
// The read is still in flight when the event lands, so its
// answer — an empty list — is stale by the time it returns.
let answer: (libraries: unknown[]) => void = () => {};
stub(
'library.Library.GetAllLibrariesWithTrackCounts',
() =>
new Promise((resolve) => {
answer = resolve;
}),
);
const el = await fixture<FirstRunWizard>('first-run-wizard');
emit(Events.LibraryAdded, aLibrary);
answer([]);
await el.updateComplete;
await new Promise((r) => setTimeout(r, 0));
await el.updateComplete;
expect(isShowing(el)).toBe(false);
});
it('still dismisses through its own Get Started button', async () => {
stub('frontendutil.FrontendUtil.HasNativeDirectoryPicker', true);
stub('frontendutil.FrontendUtil.DirectoryPicker', '/home/logan/Music');
const { resetDirectoryPickerCache } = await import(
'@utils/pick-directory'
);
resetDirectoryPickerCache();
const el = await wizardOnAFreshInstall();
const [choose, finish] = shadowAll<HTMLButtonElement>(el, '.btn');
choose?.click();
await new Promise((r) => setTimeout(r, 0));
await el.updateComplete;
finish?.click();
await new Promise((r) => setTimeout(r, 0));
await el.updateComplete;
expect(
wails.calls.filter((c) => c.path === 'library.Library.AddLibrary'),
).toHaveLength(1);
expect(isShowing(el)).toBe(false);
});
});
+373
View File
@@ -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' },
});
});
});
@@ -0,0 +1,190 @@
/**
* Settings offers the columns the backend will accept, and no others.
*
* The list is built from `COLUMN_DEFS`, which is the *drawing* table:
* every definition the track list knows how to render, including
* `titleArtist` — the phone's stacked column, chosen by width in
* `PHONE_COLUMN_IDS` and never by a person. `tracklist.AllColumnIDs` in
* Go does not list that id, so the configurator offered a nineteenth
* row that could not be ticked:
*
* ```
* validate = unknown track-list column ID: "titleArtist"
* titleArtist valid = false
* ```
*
* What a user saw was **two rows both called "Track Name"** (#197), one
* of which did nothing — and a screen reader heard "Show the Track Name
* column" twice with nothing to tell them apart, which is `a11y.32`'s
* complaint inside the list that was fixed for exactly that.
*
* It is worse than an inert control, which is why the duplicate name
* was not the thing to fix. `SetTrackListColumns` assigns before it
* validates, so a rejected list stays in memory and `Save()` validates
* the whole config:
*
* ```
* later, unrelated SetThemeAccentColor = could not save config: invalid
* config: ... unknown track-list column ID: "titleArtist"
* ```
*
* — one tick and no setting saves for the rest of the session. That
* half is filed separately; this file keeps the row from being offered.
*
* The last test is the one that would have caught it when the column
* was added: the two lists are in different languages, so nothing but a
* sweep can hold them together.
*/
import { beforeEach, describe, expect, it } from 'vitest';
import '@components/config-page/config-page';
import {
COLUMN_DEFS,
CONFIGURABLE_COLUMN_IDS,
} from '@components/track-list/columns';
import { flush, stub } from '@test/support/harness';
import { fixture, shadowAll } from '@test/support/render';
/** Go's own list of column ids, as text. */
const GO_CONFIG = Object.values(
import.meta.glob<string>('../../../backend/tracklist/config.go', {
eager: true,
query: '?raw',
import: 'default',
}),
)[0];
/**
* The ids `tracklist.AllColumnIDs` actually contains.
*
* Read out of the source rather than written down here, because a
* third copy of this list is a third thing to forget — which is the
* defect, one copy earlier.
*/
function goColumnIDs(source: string): string[] {
const constants = new Map<string, string>();
const constBlock = /const \(([\s\S]*?)\n\)/.exec(source)?.[1] ?? '';
for (const [, name, id] of constBlock.matchAll(
/(\w+)\s+ColumnID\s*=\s*"([^"]+)"/g,
)) {
constants.set(name!, id!);
}
const listBlock =
/var AllColumnIDs = \[\]ColumnID\{([\s\S]*?)\n\}/.exec(source)?.[1] ?? '';
return [...listBlock.matchAll(/(\w+),/g)]
.map(([, name]) => constants.get(name!))
.filter((id): id is string => id !== undefined);
}
/**
* The column rows, and only those.
*
* Settings view-visibility list (#25) is drawn with the same two
* classes, so a bare `.column-label` sweeps 29 rows across two
* sections — and "Albums" the destination sitting beside "Album" the
* column is not the fault this file is about. The `for`/`id` prefix is
* what tells them apart.
*/
const COLUMN_ROW_LABEL = 'label.column-label[for^="column-"]';
const COLUMN_ROW_BOX = 'input.column-toggle[id^="column-"]';
/** The rows the configurator draws, by their visible name. */
async function columnRowNames(): Promise<string[]> {
const page = await fixture('config-page');
await flush();
await page.updateComplete;
// Every section renders collapsed, and a collapsed body is `hidden`.
for (const section of shadowAll<HTMLElement>(page, 'config-section')) {
section.shadowRoot
?.querySelector<HTMLButtonElement>('button[aria-expanded="false"]')
?.click();
}
await flush();
await page.updateComplete;
return shadowAll<HTMLElement>(page, COLUMN_ROW_LABEL).map(
(label) => label.textContent?.trim() ?? '',
);
}
describe('the Settings column list', () => {
beforeEach(() => {
for (const path of [
'library.Library.GetAllLibrariesWithTrackCounts',
'jobs.Service.GetJobs',
'download.Service.ListProviders',
'download.Service.ProviderKinds',
]) {
stub(path, []);
}
stub('config.Config.GetShortcuts', {});
stub('config.Config.GetDownloadPreferences', {});
stub('config.Config.GetThemeAccentColor', '#ffd43b');
stub('config.Config.GetThemeBackgroundShade', 'dark');
});
it('names each row once', async () => {
const names = await columnRowNames();
// A sweep over nothing passes.
expect(names.length, 'the page draws column rows').toBeGreaterThan(5);
const seen = new Set<string>();
const duplicated = names.filter((name) => !seen.add(name));
expect(duplicated).toEqual([]);
expect(names.filter((n) => n === 'Track Name')).toHaveLength(1);
});
it('gives each checkbox a name that identifies it', async () => {
// The visible half above is what was reported; this is the half a
// screen reader gets, and it is the one `config-page` computes
// from the same string.
const page = await fixture('config-page');
await flush();
await page.updateComplete;
const labels = shadowAll<HTMLInputElement>(page, COLUMN_ROW_BOX).map(
(box) => box.getAttribute('aria-label') ?? '',
);
expect(labels.length, 'the page draws column checkboxes').toBeGreaterThan(5);
expect(new Set(labels).size).toBe(labels.length);
});
});
describe('the column table', () => {
it('offers no column the backend would reject', async () => {
const accepted = goColumnIDs(GO_CONFIG ?? '');
// Two non-vacuity guards: a glob that stopped matching, and a
// parse that stopped finding the list it names.
expect(GO_CONFIG, 'backend/tracklist/config.go is readable').toBeTruthy();
expect(accepted.length, 'AllColumnIDs was parsed').toBeGreaterThan(10);
expect(
CONFIGURABLE_COLUMN_IDS.filter((id) => !accepted.includes(id)),
).toEqual([]);
});
it('still knows how to draw every column it offers', async () => {
// The filter must not have taken a column *out* of the drawing
// table: `configurable` says what Settings may list, not what the
// list may render.
expect(
CONFIGURABLE_COLUMN_IDS.filter((id) => COLUMN_DEFS[id] === undefined),
).toEqual([]);
expect(CONFIGURABLE_COLUMN_IDS).not.toContain('titleArtist');
expect(COLUMN_DEFS['titleArtist'], 'the phone still has its column')
.toBeTruthy();
});
});
+7 -3
View File
@@ -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
View File
@@ -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