fix(settings): give theme, zoom and preview width the persistence every other setting has - #618
Merged
Merged
Conversation
…ry other setting has
`settings.svelte.ts` has a `PersistedSetting` mechanism -- one `$effect` per
key, a compare-and-set write, a `storage` listener -- and the comment on
`installPersistedSettings` says what it is for: Markpad opens several windows,
each a separate webview with its own store instance, all sharing one
localStorage. Three values sat outside it and wrote localStorage by hand.
`theme` is the visible bug. Changing it in window A's settings panel left every
other window on the old theme until it restarted, while every switch beside it
in that same panel synced live -- because those were persisted entries and got
the listener for free, and the theme was not: the whole app had two `storage`
listeners and neither was looking at `theme`. Its write had no compare-and-set
either. It is an entry now, keyed `theme` still, because `src/app.html` reads
that key in its first-paint script and renaming it brings back the white flash
that script exists to prevent.
`zoomLevel` was read back with `parseInt(localStorage.getItem('zoomLevel') ||
'100', 10)` and no validation, so a corrupt key became NaN -- and NaN is a trap
rather than a glitch: the preview renders `zoom: NaN`, and both ways out are
`Math.min(NaN + 10, 500)`, which is NaN too. Wheel and chords were dead and
only the reset button could recover the window. It goes through
`NumericSettingRange` now, like the five font and width settings whose own
`nullpx` story is recorded in Settings.svelte. The 25/500 pair had been copied
into three files and the neutral 100 into three more; `ZOOM_LEVEL_RANGE` and
the three store operations that read it are the only copy left.
`preview.fullWidth` had the same bare read and write plus a legacy-key
migration. The migration survives as `LEGACY_PREVIEW_FULL_WIDTH_KEY`, read and
never written, the treatment `editor.openFileMode` and `editor.autoSaveEdits`
already give theirs. The old key is no longer deleted, because a `read` that
touches exactly one field -- the property the whole mechanism rests on -- cannot
remove a second key. It is inert either way: `??` prefers the new one from the
first write onwards.
`theme` also gets a union type, the way `LanguageCode` has one, which removes
both `as any` casts in Settings.svelte. `resolveTheme` is what the untyped
edges (the `<select>`, the title bar, localStorage) go through, so an
unrecognised theme lands on `system` instead of on the document.
And `save_theme` now stores the resolved appearance rather than the theme name.
`theme.txt` has exactly one reader -- `app.rs`, picking a window's background
colour before any webview exists -- and its vocabulary there is "dark",
"light", and "anything else means ask the OS". Every `vscode:` theme fell into
that last arm, so a dark VS Code theme on a light desktop flashed a white window
on every launch. Whether such a theme is dark is inside its own JSON, which only
the frontend parses; sending the answer avoids a second theme parser in Rust and
needs no change to `app.rs` at all. A file written in the older format still
lands in the fallback arm and self-corrects on the next theme application.
The keymap harness records writes to the settings store now, not only to
component locals, or the preview-width chords would come back as firing
nothing. `shortcutRegistry.test.ts` asks which zoom operation each chord runs;
that zoomIn really raises the level is asserted against the real store, where
the arithmetic moved to.
Falsified by deleting the three entries from `createSettingsPersistence` and
putting the bare `setItem` back: six of the new assertions go red, including
cross-window theme and `zoomLevel = 'banana'`. 1016 tests pass, 145 Rust tests
pass, `npm run check` is clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous commit moved three values into `PersistedSetting`. This is the rule that stops a fourth appearing, because nothing else could see one: the values were persisted, the app worked in a single window, and the defect was visible only when a second window was open. The marker is the bare `localStorage.setItem(` call, which is the defect itself rather than a proxy for it -- it is the write no `storage` listener answers, no compare-and-set guards and no range validates. `writeStoredSetting` is all three, so the file that defines it is the one allowed site. `utils/recentFiles.ts` was checked and is deliberately absent rather than allowed. It does keep persistence logic of its own -- a read-modify-write and its own `storage` listener -- and that is correct: the recent-file list is a collection every window appends to, not a scalar preference, so re-reading before each change is the point and folding it into a `read`/`load` pair would lose it. But its write already goes through `writeStoredSetting` (#370), so it does not match this marker at all and needs no exception. `stores/tabs.svelte.ts` is listed, and the comment says it is a known gap rather than an exemption: `editor.splitScrollSync` is an ordinary scalar preference that belongs in `createSettingsPersistence`, and it has the milder half of the same defect -- one key and one write, so it cannot clobber its neighbours, but no listener, so a second window keeps its own answer until it restarts. It is allowed only because collecting it means editing the tab store, which was outside the scope of this change. Falsified by putting `localStorage.setItem('theme', theme)` back into MarkdownViewer.svelte: the rule goes red and names the file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ttings from it (#619) `editor.preZenState` was the one entry in `createSettingsPersistence` that trusted what it found. Its neighbours all validate -- `normalizeEditorToolbarOrder`, `parseStoredNumber`, `isSupportedLanguage`, `raw === 'left' || raw === 'right'` -- while this one did `JSON.parse(raw)` straight onto the field and caught only the parse error, which is the failure a corrupt record is least likely to be. It is also the entry whose value is copied onto six other settings: leaving zen mode assigns `this.showTabs = this.preZenState.showTabs` and five more like it. So a record whose shape this version cannot read did not stay contained. A snapshot missing `showTabs` -- an older release, a half-written key -- restored `showTabs = undefined`; the write effect stores `String(undefined)`, so localStorage got the *string* `"undefined"`; and `raw === 'true'` is false forever after. The tab bar was gone on that launch and on every later one, with only the settings dialog to bring it back. A field of the wrong type is the same chain with `"yes"` in place of `"undefined"`. `normalizePreZenState` accepts all six fields at the right types or nothing. Partial repair was the other option and is worse: a record this version cannot read comes from a version whose remaining five fields it cannot vouch for either, and mixing kept fields with invented ones restores a state the user never had. Rejecting it whole leaves one rule to reason about -- either the snapshot is the user's or there is no snapshot. That makes "no snapshot" a state zen mode has to leave through, so it now does. `toggleZenMode` restores from `this.preZenState ?? DEFAULT_PRE_ZEN_STATE` instead of skipping the restore when there is nothing to restore from. The old `if (this.preZenState)` guard turned "no snapshot" into "stay flattened", which is exactly the symptom being fixed: the tab bar, status bar and line numbers stay hidden with nothing in the UI saying zen mode was ever involved. The defaults are the store's own field initializers, and a test pins them to those initializers so the two copies cannot drift. `load` now reads like `titlebar.toolbarPlacement` above it -- `parseStoredRecord` for the JSON, then a normalizer for the shape -- so unparseable text, a non-object and an array all arrive as `null` without a `try`/`catch` of its own. Falsified twice, each half separately. Restoring the bare `JSON.parse` turns two tests red, and the probe shows the whole chain per corrupt form: a wrong type stores `"yes"` and restarts to `false`, a missing field and a non-object both store `"undefined"` and restart to `false`. Restoring the `if (this.preZenState)` guard instead turns the end-to-end test red on its own, which is what says both halves carry weight -- unparseable JSON left the field `null` even before this change, and only the fallback gets the tab bar back from there. Not fixed, deliberately: a window that enters zen mode inside the few milliseconds between receiving a sibling's `showTabs=false` and its `preZenState` snapshots the already-flattened `false`. That snapshot is well-formed, so no validation can tell it from a real one. It needs a keystroke inside a millisecond window, and the damage it can still do is now one boolean mislaid once -- recoverable from the settings dialog and persisted correctly after that -- rather than a tab bar that cannot come back. Co-authored-by: PathGao <gaoyanbo@gaoyanbodeMacBook-Air.local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
PathGao
added a commit
that referenced
this pull request
Aug 12, 2026
…echanism (#638) * fix(settings): give the split scroll-sync preference the storage listener `editor.splitScrollSync` was the last localStorage write outside `writeStoredSetting`: the tab store read it in its constructor and wrote it with a bare `setItem`. That is the milder half of the defect #618 collected the other three writers for — one key and one write, so it could not clobber its neighbours the way a whole-snapshot write did — but it still had the other half. Nothing listened for `storage`, so flipping scroll sync in one window left every other window seeding its next split from its own construction-time answer until it was restarted, while every preference beside it synced live. It is a preference, not tab state. `Tab.isScrollSynced` is the per-tab value: it is what the title bar toggles, it lives in the window-state snapshot, and it stays where it is. This is the single sticky scalar every tab and every window seeds a new split from, so it moves to `SettingsStore` as one more entry in `createSettingsPersistence` — compare-and-set on write and the store's own `storage` listener, both for free. `TabManager` keeps the name the tab code reads it under, because `splitScrollSyncPreference` and `isScrollSynced` being spelled differently is what says they are not the same value. The known-gap line in the `localStorage is written through one function` rule goes with it: the allowed list is now `settings.svelte.ts` alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(recent-files): re-apply the change a sibling window overwrote `updateStoredRecentFiles` re-reads before writing, which fixed the reported defect — a window publishing a snapshot of its own stale array — and its own comment recorded what was left: the read and the write are still two calls, localStorage has no compare-and-swap, and the spec's storage mutex is implemented nowhere. A sibling's `setItem` landing between them overwrites the entry that sibling had just added with a list computed before it existed. The cycle now reads back what actually landed and, if that is not what it just wrote, applies the mutation again over the list that won. It converges because every mutation here is "apply my one change to whatever list you hand me": promote, drop and rename are each idempotent and each merge, so re-applying folds this window's change into the sibling's rather than dropping one of the two. Both windows run the same code, so the loser of an interleaving is the one that retries. The attempt cap is what stops two busy windows live-locking on a synchronous click; exhausting it loses one entry, which is what every interleaving used to cost. The surviving hole is a sibling write that lands after the read-back — and that window is reading back too, so it merges into what this one published. Not the other answer: Rust owning the list under a `Mutex`, the way `update_pinned_tags` in window_runtime.rs owns the pin list. That lock is available there because every window is a thread of one Rust process; these writers are separate webview processes with no primitive between them, so a backend list would still have to be pushed out to each window — which is what the `storage` event already does. And Rust is the authority on pins because Rust reads them: it owns the window list and consults a tag when opening and closing windows. Nothing in the backend reads recent files. Moving them there would turn a list the home screen renders synchronously into an async IPC round trip at every reader, to buy a lock over a value whose worst-case loss is one row the next open restores. The list also keeps its own read-modify-write and its own `storage` listener rather than becoming a `PersistedSetting`: it is a collection every window appends to, not a scalar preference, and folding it into that mechanism would throw away the re-read this change is about. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: PathGao <gaoyanbo@gaoyanbodeMacBook-Air.local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
settings.svelte.tshas aPersistedSettingmechanism: one$effectper key, compare-and-set to stop the echo, and astoragelistener for the other windows. Its comment says why — Markpad opens multiple windows, each webview gets its own store instance, and they share one localStorage.Three values were never wired into it and wrote localStorage by hand.
The bug you can see
Open two windows and change the theme in one. Every other switch in that same appearance panel reaches the other window live. The theme
<select>, sitting in the same panel, does not — the second window stays on the old theme until it restarts.The bug you can't see yet
zoomLevelwas read withparseInt(localStorage.getItem('zoomLevel') || '100', 10)and never validated. One corrupt value givesNaN, which renders asstyle="zoom: NaN"— andMath.min(NaN + 10, 500)is alsoNaN, so neither the wheel nor the shortcuts can get out of it. Only the reset button could.The repo already had the cure:
parseStoredNumber/clampToRange/stepWithinRange, used by all five font and width settings.Settings.svelte:47-60even records the bug they were written for (an empty input becomingfont-size: nullpx).What changed
All three are now ordinary
PersistedSettingentries — same shape as the ~35 already there, no parallel mechanism.themethemesrc/app.html:8reads that exact key in its first-paint scriptzoomLevelzoomLevelnumberSetting(…, ZOOM_LEVEL_RANGE, …), pluszoomIn()/zoomOut()/resetZoom()on the storeisFullWidthpreview.fullWidthreadStoredKey, never written — howeditor.openFileModealready handles its ownDuplicated constants collapsed:
25/500from 3 copies to 1 (ZOOM_LEVEL_RANGE); reset-to-100from 3 copies to 1.Editor.svelte'szoomLevelstopped being$bindable— nothing writes it back any more.Both
as anyare gone.ThemeSetting = 'system' | 'light' | 'dark' | \vscode:${string}`plusresolveTheme(). The two casts inSettings.svelteexisted only becausethemewas a barestring`.The VS Code theme startup flash, fixed without touching Rust
app.rsreadstheme.txtto pick the window's background before the webview paints, and its vocabulary is"dark"/"light"/ everything else. Everyvscode:*theme fell into the last arm and followed the system appearance — so a dark VS Code theme on a light system flashed white at startup.save_themenow receives the resolved appearance rather than the theme name, read from thedataset.themeTypethatparseAndApplyVscodeThemealready publishes from the theme's owntypefield. No Rust logic changed —app.rsalready matched those two strings. An old-formattheme.txtlands in the fallback arm and self-corrects on the first theme application.The guardrail
A
singleImplementationConventionrow:localStorage.setItemmay only appear insettings.svelte.ts, which defineswriteStoredSetting.recentFiles.tswas checked and is deliberately not exempted. Its read-modify-write and its ownstoragelistener are justified — the recent list is a collection every window appends to, not a scalar — but its write already goes throughwriteStoredSetting, so it does not match the marker and needs no exception.tabs.svelte.tsis listed and labelled a known gap, not an exemption.editor.splitScrollSynchas the milder half of the same defect: one key, one write, so nothing clobbers — but no listener, so a sibling window stays stale until restart. It is allowed only because it sat outside this change's boundary. Filed as follow-up.Falsification
Removing the three entries and putting
localStorage.setItem('theme', theme)back, with the tests untouched:Restored: all green.
Rebased onto #615
This branch was written before the vitest pilot landed, so its new
settingsPersistencetests were rewritten onto that runner: the hand-rolledlocalStorageShimand capturedstorageListenersbecame jsdom's reallocalStorageand a real dispatchedStorageEvent,flushSync()after construction (the listener is installed inside an$effect), andreadSourceswitched to the cwd-relative form becauseimport.meta.urlishttp:under vitest.Verification
npm test964 pass / 0 failnpm run test:vitest62 pass / 0 failnpm run check769 files / 0 errors / 0 warningscargo test145 pass / 0 fail🤖 Generated with Claude Code