Skip to content

fix(settings): persist each setting on its own key and sync across windows - #370

Merged
PathGao merged 2 commits into
sftwrdotdev:masterfrom
PathGao:fix/settings-persistence
Aug 2, 2026
Merged

fix(settings): persist each setting on its own key and sync across windows#370
PathGao merged 2 commits into
sftwrdotdev:masterfrom
PathGao:fix/settings-persistence

Conversation

@PathGao

@PathGao PathGao commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Summary

Every window runs its own SettingsStore instance, and one $effect read about thirty fields and rewrote every localStorage key whenever any of them changed. A second window's store still held the snapshot it was constructed with, so changing one toggle there rewrote all the keys from that stale snapshot.

window A   set font size 20, language 日本語     → keys written
window B   flip "Show Tabs"
           its store still holds the values it
           was constructed with
           the one effect rewrites all ~30 keys  → A's changes overwritten
restart    font size 14, language English

Nothing anywhere listened for storage, so no window ever learned of another's edits.

Persistence is now a table of one entry per key, each installed as its own effect, so an effect only re-runs for the field it owns. A storage listener adopts remote changes through the same entries.

Why compare-and-set rather than a flag

The echo a storage listener invites — adopt a remote value, the local effect fires, write it straight back — is stopped by reading before writing. A flag raised inside the handler is already gone by the time Svelte's asynchronous flush runs the effect, and holding it until the flush would also swallow genuine local edits landing in the same tick. A value that arrived from localStorage already equals what is stored, so the echo dies at its source with no timing assumption at all.

Also fixed

Font sizes reverted on restart. The spin buttons went to 48 while the load path clamped to 24 (editor, code) and 28 (preview): pick 30, restart, get 24. Bounds are single constants now, shared by the UI and the load path, widened to the 48 the UI has been offering — profiles already holding 30 or 40 survive instead of shrinking on upgrade.

Typed values were unbounded, and clearing the field wrote null. HTML min/max only constrain the spinners; an empty field produced font-size: nullpx and stored the string "null". The inputs are one-way now: while typing only in-range values commit — so the "1" of "18" is not clamped to 10 — and blur/Enter/change clamps and writes the corrected value back to the DOM, which is necessary because a clamp landing on the current state produces no re-render to correct the text.

Norwegian and traditional Chinese were never detected. nb and nn are the real BCP-47 primary subtags; startsWith('no') missed both. Matching the exact primary subtag also removes the nl collision by construction. Chinese now reads the script subtag, so zh-Hant, zh-Hant-TW and zh-Hant-HK resolve to traditional.

The dialog lost focus placement. An effect both read and wrote appVersion, so resolving the version re-ran it and re-captured previousActiveElement from inside the dialog. The same shape appeared twice more in that effect (loadFonts() reads the loaded flag it later sets; previousActiveElement was itself reactive). None are rendered, so all three are plain variables now.

Validation

  • npm run check — 0 errors, 0 warnings
  • npm test — 225/225, including 27 new tests in scripts/settingsPersistence.test.ts

The headline test is "no store field is read by two persisted entries": it runs every entry's read() against a Proxy-recorded store and asserts each reads exactly one property, and that field→entry is a bijection. Svelte's dependency tracking is "which properties did this effect read", so that is a direct proof that changing field A cannot rewrite key B, not a proxy for it. Backed by an end-to-end two-instance simulation of the scenario above.

Note on scripts/previewWidth.test.ts

Two assertions there grep-matched the literal source of the bug — localStorage.setItem('preview.maxWidth', …) inside the giant effect — which no correct fix can preserve. They are replaced with equivalent-intent assertions against that key's persistence entry. The other assertions in the file are untouched and still pass.

PathGao and others added 2 commits August 3, 2026 03:00
…ndows

Every window runs its own `SettingsStore` instance, and one `$effect` read
about thirty fields and rewrote every localStorage key whenever any of them
changed. A second window's store still held the snapshot it was constructed
with, so changing one toggle there rewrote all the keys from that stale
snapshot: change the font size and language in window A, flip Show Tabs in
window B, and A's changes are gone after a restart. Nothing anywhere
listened for `storage`, so no window ever learned of another's edits.

Persistence is now a table of one entry per key, each installed as its own
effect, so an effect only re-runs for the field it owns. A `storage`
listener adopts remote changes through the same entries.

The echo that a listener like that invites — adopt a remote value, the local
effect fires, write it straight back — is stopped by comparing before
writing rather than by a flag. A flag raised inside the handler is already
gone by the time Svelte's asynchronous flush runs the effect, and holding it
until the flush would also swallow genuine local edits landing in the same
tick. A value that arrived *from* localStorage already equals what is
stored, so the echo dies at its source with no timing assumption.

**Font sizes reverted on restart.** The spin buttons went to 48 while the
load path clamped to 24 (editor, code) and 28 (preview): pick 30, restart,
get 24. Bounds are now single constants shared by the UI and the load path,
widened to the 48 the UI has been offering — profiles already holding 30 or
40 now survive instead of shrinking on upgrade.

**Typed values were unbounded, and clearing the field wrote `null`.** The
HTML `min`/`max` only constrain the spinners. An empty field produced
`font-size: nullpx` and stored the string `"null"`. The inputs are one-way
now: while typing, only in-range values commit — so the "1" of "18" is not
clamped to 10 — and blur, Enter or change clamps and writes the corrected
value back to the DOM, since a clamp landing on the current state produces
no re-render to correct the text.

**Norwegian and traditional Chinese were never detected.** `nb` and `nn` are
the real BCP-47 primary subtags; matching `startsWith('no')` missed both.
Matching on the exact primary subtag also removes the `nl` collision by
construction. Chinese now reads the script subtag, so `zh-Hant`,
`zh-Hant-TW` and `zh-Hant-HK` resolve to traditional.

**The settings dialog lost focus placement.** An effect both read and wrote
`appVersion`, so resolving the version re-ran it and re-captured
`previousActiveElement` from inside the dialog. The same shape appeared
twice more in that effect — `loadFonts()` reads the `loaded` flag it later
sets, and `previousActiveElement` was itself reactive. None are rendered, so
all three are plain variables now.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Preview width already reads `px · 640–1600 · default 880`, so the bounds
and the value to return to are visible without trial and error. The three
font sizes and the editor width showed only their unit.

Now that each of those has a single range constant shared by the UI and the
load path, the same line can be rendered from it — which also means the
displayed bounds cannot drift from the enforced ones.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@PathGao
PathGao merged commit 3293d30 into sftwrdotdev:master Aug 2, 2026
4 checks passed
@PathGao
PathGao deleted the fix/settings-persistence branch August 2, 2026 19:39
PathGao added a commit that referenced this pull request Aug 3, 2026
…405)

Three call sites serialised the window's own copy of the list back to
localStorage with no re-read and no `storage` listener, so with two
windows open the last one to touch the list discarded whatever the other
had recorded.

#370 already solved this shape for settings, so the fix reuses it rather
than inventing a second mechanism: a new `recentFiles.ts` holds the pure
list transforms plus `updateStoredRecentFiles(mutate)`, which re-reads,
applies, and writes through #370's `writeStoredSetting`. That write is
compare-and-set, so a no-op fires no `storage` event and the propagation
loop terminates after one hop - #370's argument for why an
`isApplyingRemote` flag cannot work applies here unchanged. A `storage`
listener in the viewer folds in what sibling windows record.

No change to settings.svelte.ts was needed; `writeStoredSetting` was
already exported.

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
…ry other setting has (#618)

* fix(settings): give theme, zoom and preview width the persistence every 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>

* test(convention): forbid a second place that writes localStorage by hand

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>

* fix(settings): validate the zen-mode snapshot before restoring six settings 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>

---------

Co-authored-by: PathGao <gaoyanbo@gaoyanbodeMacBook-Air.local>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant