diff --git a/CLAUDE.md b/CLAUDE.md index d3b5942..dc3057d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -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 diff --git a/backend/config/config.go b/backend/config/config.go index e116553..c6548a9 100644 --- a/backend/config/config.go +++ b/backend/config/config.go @@ -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, ) diff --git a/backend/config/setter_rollback_test.go b/backend/config/setter_rollback_test.go new file mode 100644 index 0000000..6df3535 --- /dev/null +++ b/backend/config/setter_rollback_test.go @@ -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) +}