Compare commits

..
Author SHA1 Message Date
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
6 changed files with 332 additions and 242 deletions
+22 -21
View File
@@ -662,6 +662,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
@@ -3275,27 +3297,6 @@ 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`
+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)
}
@@ -51,7 +51,7 @@ import type { BackgroundShade } from '@store/theme-store';
import type { IconStyle } from '@store/favorites-store';
import {
COLUMN_DEFS,
CONFIGURABLE_COLUMN_IDS,
ALL_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 = CONFIGURABLE_COLUMN_IDS.filter(
const disabledIds = ALL_COLUMN_IDS.filter(
(id) => !enabledIds.includes(id),
);
+7 -29
View File
@@ -32,18 +32,6 @@ 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). */
@@ -110,16 +98,11 @@ export const COLUMN_DEFS: Record<string, ColumnDef> = {
},
titleArtist: {
id: 'titleArtist',
// 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.
// 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.
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),
@@ -283,15 +266,10 @@ export const COLUMN_DEFS: Record<string, ColumnDef> = {
};
/**
* 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`.
* All column IDs in default display order.
* Used by the settings UI to list available columns.
*/
export const CONFIGURABLE_COLUMN_IDS: string[] = Object.keys(
COLUMN_DEFS,
).filter((id) => COLUMN_DEFS[id]?.configurable !== false);
export const ALL_COLUMN_IDS: string[] = Object.keys(COLUMN_DEFS);
/**
* Column IDs that are always searched regardless of visibility.
@@ -1,190 +0,0 @@
/**
* 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();
});
});