From bcf3856b6f9bcd89bbbc892dc562b43bb6ed5485 Mon Sep 17 00:00:00 2001 From: Logan Date: Sun, 30 Aug 2026 05:40:36 -0400 Subject: [PATCH] fix(config): put the old value back when a setter is rejected MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- CLAUDE.md | 22 +++ backend/config/config.go | 39 ++++ backend/config/setter_rollback_test.go | 262 +++++++++++++++++++++++++ 3 files changed, 323 insertions(+) create mode 100644 backend/config/setter_rollback_test.go 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) +} -- 2.54.0