fix(config): put the old value back when a setter is rejected #233

2026-08-30T09:41:41Z

logan commented

2026-08-30 09:41:41 +00:00

The issue

Config.SetTrackListColumns assigned the new column list to the in-memory config before validating it, and returned the validation error without putting the old list back.

Config.Save() validates the whole config, so a rejected value did not merely fail its own call — it failed every later save, of every unrelated setting, silently and for the rest of the session. Theme, launch page, shortcuts, libraries: none of them would stick. Nothing reached disk, so a restart cleared it, which is exactly what makes this invisible to the user and unreportable.

Scope, decided explicitly

The Findings on #231 ask whether this is one fix or one pass over the file, and the previous run declined to fix it in passing on the grounds that every setter has this shape. That turns out to be false, and checking it is what bounded the diff.

The defect is precisely assignment precedes a validation that can reject that argument. That predicate is mechanical, not a judgement, and it enumerates seven setters:

SetScanConcurrency, SetThemeAccentColor, SetThemeBackgroundShade, SetDefaultPage, SetQueueFallback, SetTrackListColumns, SetFavoritesIconStyle.

Each now snapshots the field it is about to overwrite and restores it on the error path — three lines apiece.

The rest were read rather than assumed, and are unchanged:

Setter	Why it cannot be poisoned by its own argument
`SetShortcuts`, `SetShortcut`	`shortcuts.Config.Validate` returns `nil` unconditionally
`SetFavoritesPlaylistID`, `SetPinDefaultPlaylist`, `SetAllowMeteredCatalogDownload`, `SetPopupVolume`	an `int64` and bools that no `Validate` inspects
`SetDownloadPreferences`	`Config.Validate` does not validate `Downloads` at all
`SetViewVisible`	refuses an unknown, non-hideable or launch-page view before assigning; `GeneralConfig.Validate` only normalizes the visibility map rather than rejecting it
`SetLibraryDirectory`	already correct, and the precedent: it builds and validates a candidate via `library.NewConfig` before assigning, so there is nothing to undo

This is not asserted from reading alone — see "planted" below: on the unfixed code the two already-correct setters passed while all seven others failed.

Deliberately not done. A Save() failure — a disk error rather than a rejection — still leaves memory ahead of disk. That value has already passed validation, so nothing is poisoned and no later save is blocked. Whether the in-memory config should roll back to match the file is a different question needing its own argument, and it is not what #231 reports.

Verification

go test ./backend/config/ — green. New backend/config/setter_rollback_test.go; each row asserts three things in order: the setter errors, the getter still reports the old value, and an unrelated save still works — the third being the one the user actually feels.

Planted, not read. Against the unfixed code all eight rows fail with the defect's own numbers:

value after a rejected write = "telepathy", want the previous "auto"
value after a rejected write = "not-a-hex", want the previous "#ffd43b"
value after a rejected write = "chartreuse", want the previous "dark"
value after a rejected write = "nowhere", want the previous "home"
value after a rejected write = "improvise", want the previous "favorites"
value after a rejected write = "asterisk", want the previous "heart"
value after a rejected write = "titleArtist", want the previous "trackName,artistName,album,trackLength"
value after a rejected write = "trackName,trackName", want the previous "trackName,artistName,album,trackLength"

each followed by Save() failed after a rejected write: invalid config: .... The two tests covering the setters left alone passed on that same run.

Two extra cases pin the setters deliberately not changed (SetLibraryDirectory, SetViewVisible), so a later refactor that breaks their up-front validation is visible rather than silent.
The duplicate-id case matters more than the unknown-id one now: #197 closed the UI route to an invalid id, but a client still assembles the list itself and is free to send a duplicate — and a stale frontend against a newer backend is the same shape.
make lint — 0 issues across all three build configurations.
make test — green, all three passes (default, indexbuild, dev).
make bindings-check — current. make skill-check — 47 targets, all present.
Not run, with reasons: make generate (no .sql/.templ), make ui-test (no frontend source touched), make e2e — #197 removed the only UI route to an invalid value, so there is no flow left to drive; the Go tier is where this behaviour lives.

One thing worth knowing

The rationale was first written as a doc comment on Save(), and make bindings-check caught that Save is bound — v3's generator is a static analyser that carries doc comments across, so fourteen lines of backend reasoning rendered into frontend/bindings/yellowjacket/backend/config/config.ts for an audience with no use for it. It is a free-floating comment above the setter section instead.

Coordination

Does not collide with #232 (fix/197-duplicate-column-label), which touches config-page.ts, columns.ts and a frontend test, and no Go. The one shared file is CLAUDE.md, and this entry is on the config package bullet (~line 664) rather than in #232's block (~line 3275), so the two should merge cleanly.

Closes #231

## The issue `Config.SetTrackListColumns` assigned the new column list to the in-memory config **before** validating it, and returned the validation error without putting the old list back. `Config.Save()` validates the *whole* config, so a rejected value did not merely fail its own call — it failed **every later save, of every unrelated setting**, silently and for the rest of the session. Theme, launch page, shortcuts, libraries: none of them would stick. Nothing reached disk, so a restart cleared it, which is exactly what makes this invisible to the user and unreportable. ## Scope, decided explicitly The Findings on #231 ask whether this is one fix or one pass over the file, and the previous run declined to fix it in passing on the grounds that *every* setter has this shape. **That turns out to be false, and checking it is what bounded the diff.** The defect is precisely *assignment precedes a validation that can reject that argument*. That predicate is mechanical, not a judgement, and it enumerates **seven** setters: `SetScanConcurrency`, `SetThemeAccentColor`, `SetThemeBackgroundShade`, `SetDefaultPage`, `SetQueueFallback`, `SetTrackListColumns`, `SetFavoritesIconStyle`. Each now snapshots the field it is about to overwrite and restores it on the error path — three lines apiece. **The rest were read rather than assumed, and are unchanged:** | Setter | Why it cannot be poisoned by its own argument | |---|---| | `SetShortcuts`, `SetShortcut` | `shortcuts.Config.Validate` returns `nil` unconditionally | | `SetFavoritesPlaylistID`, `SetPinDefaultPlaylist`, `SetAllowMeteredCatalogDownload`, `SetPopupVolume` | an `int64` and bools that no `Validate` inspects | | `SetDownloadPreferences` | `Config.Validate` does not validate `Downloads` at all | | `SetViewVisible` | refuses an unknown, non-hideable or launch-page view **before** assigning; `GeneralConfig.Validate` only *normalizes* the visibility map rather than rejecting it | | `SetLibraryDirectory` | already correct, and the **precedent**: it builds and validates a candidate via `library.NewConfig` before assigning, so there is nothing to undo | This is not asserted from reading alone — see "planted" below: on the unfixed code the two already-correct setters passed while all seven others failed. **Deliberately not done.** A *`Save()`* failure — a disk error rather than a rejection — still leaves memory ahead of disk. That value has already passed validation, so nothing is poisoned and no later save is blocked. Whether the in-memory config should roll back to match the file is a different question needing its own argument, and it is not what #231 reports. ## Verification - **`go test ./backend/config/`** — green. New `backend/config/setter_rollback_test.go`; each row asserts three things in order: the setter errors, the getter still reports the old value, and **an unrelated save still works** — the third being the one the user actually feels. - **Planted, not read.** Against the unfixed code all eight rows fail with the defect's own numbers: ``` value after a rejected write = "telepathy", want the previous "auto" value after a rejected write = "not-a-hex", want the previous "#ffd43b" value after a rejected write = "chartreuse", want the previous "dark" value after a rejected write = "nowhere", want the previous "home" value after a rejected write = "improvise", want the previous "favorites" value after a rejected write = "asterisk", want the previous "heart" value after a rejected write = "titleArtist", want the previous "trackName,artistName,album,trackLength" value after a rejected write = "trackName,trackName", want the previous "trackName,artistName,album,trackLength" ``` each followed by `Save() failed after a rejected write: invalid config: ...`. The two tests covering the setters left alone passed on that same run. - Two extra cases pin the setters deliberately **not** changed (`SetLibraryDirectory`, `SetViewVisible`), so a later refactor that breaks their up-front validation is visible rather than silent. - The **duplicate-id** case matters more than the unknown-id one now: #197 closed the UI route to an invalid id, but a client still assembles the list itself and is free to send a duplicate — and a stale frontend against a newer backend is the same shape. - **`make lint`** — 0 issues across all three build configurations. - **`make test`** — green, all three passes (default, `indexbuild`, `dev`). - **`make bindings-check`** — current. **`make skill-check`** — 47 targets, all present. - Not run, with reasons: `make generate` (no `.sql`/`.templ`), `make ui-test` (no frontend source touched), `make e2e` — #197 removed the only UI route to an invalid value, so there is no flow left to drive; the Go tier is where this behaviour lives. ## One thing worth knowing The rationale was first written as a doc comment on `Save()`, and `make bindings-check` caught that `Save` is **bound** — v3's generator is a static analyser that carries doc comments across, so fourteen lines of backend reasoning rendered into `frontend/bindings/yellowjacket/backend/config/config.ts` for an audience with no use for it. It is a free-floating comment above the setter section instead. ## Coordination Does not collide with #232 (`fix/197-duplicate-column-label`), which touches `config-page.ts`, `columns.ts` and a frontend test, and no Go. The one shared file is `CLAUDE.md`, and this entry is on the `config` package bullet (~line 664) rather than in #232's block (~line 3275), so the two should merge cleanly. Closes #231

logan added 1 commit 2026-08-30 09:41:41 +00:00

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

Details

CI / e2e (pull_request) Successful in 10m7s

Details

bcf3856b6f

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

logan commented

2026-08-30 09:55:03 +00:00

CI green — run 18472, both required jobs.

check success: commit messages, make lint, make test, frontend typecheck, CSS literals, make ui-test, make bindings-check, make skill-check — every step passed.
e2e success: both browser steps ran and passed (E2E — chromium ✔, E2E — webkit ✔). Checked the per-step status rather than the job conclusion, since a chromium failure is what silently skips the WebKit step and that is the one source of WebKit signal.

Nothing was skipped except the two failure-only steps (App log on failure, Upload traces and screenshots), as expected on a passing run.

**CI green** — run 18472, both required jobs. - **`check`** success: commit messages, `make lint`, `make test`, frontend typecheck, CSS literals, `make ui-test`, `make bindings-check`, `make skill-check` — every step passed. - **`e2e`** success: **both** browser steps ran and passed (`E2E — chromium` ✔, `E2E — webkit` ✔). Checked the per-step status rather than the job conclusion, since a chromium failure is what silently skips the WebKit step and that is the one source of WebKit signal. Nothing was skipped except the two failure-only steps (`App log on failure`, `Upload traces and screenshots`), as expected on a passing run.

logan referenced this pull request

2026-08-30 09:55:04 +00:00

An invalid setter argument poisons the in-memory config, so nothing saves until restart #231

All checks were successful

CI / check (push) Skipped

Required

CI / e2e (push) Skipped

Required

CI / check (pull_request) Successful in 2m46s

Required

Details

CI / e2e (pull_request) Successful in 10m7s

Required

Details

You are not authorized to merge this pull request.

This pull request can be merged automatically.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin fix/231-setter-rollback:fix/231-setter-rollback

git checkout fix/231-setter-rollback

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: yonlu/yellowjacket#233