fix(session): turning off Restore State left the session file on disk - #642
Merged
Conversation
…rase The snapshot moved from localStorage to a file on the Rust side. The branch that runs when "Restore State on Reopen" is off went on clearing the two localStorage keys — which nothing writes any more — and left the file alone. That branch is the whole meaning of the setting: while the snapshot lived in localStorage, switching restore off ended the session and the record of it. After the move the record survived instead, so the list of every document the user had open stayed in the config directory indefinitely, including for the user who turned the setting off to stop it being kept. Nothing rewrites the file while the setting is off (every `persistWindowState` call site is gated on it), so the stale session also comes back if a later launch reads it. `discardPersistedState` already clears both stores; this branch now calls it. The two new tests also pin the other half of the migration: an older build's localStorage snapshot — pre-v2 shape, untitled entry, HOME sentinel and all — still restores and is written through to the Rust side on the way past. 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
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.
The todo asked to finish a migration
windowSession.svelte.ts:98-112describes as unfinished. It isn't — that comment describes the endpoint, and the endpoint is reached. But auditing it turned up something the migration did leave behind.The bug
restore()'s!shouldRestoreState()branch is what makes the "Restore State on Reopen" setting take effect. While the snapshot lived in localStorage, its tworemoveItemcalls ended the session and its record together.The snapshot moved to a file on the Rust side in #214. The branch was not updated. It now clears two keys nothing writes and leaves
window-state-v2.jsonon disk — so the list of every document the user had open persists in the config directory indefinitely, for exactly the user who turned the setting off to stop it being kept. Nothing rewrites the file while the setting is off, so a later launch can also resurrect a stale session.Fix is the existing
discardPersistedState(), which already clears both stores. Net +1 line of code. Test written first against master's behaviour, confirmed failing.Two things had to hold before adding a destructive call to a startup path, and both do:
settings.restoreStateOnReopenis loaded synchronously in theSettingsStoreconstructor, beforeMarkdownViewer.initruns, so there is no async window where it reads as its default; andrestore()returns on!isMainWindowabove this branch, so secondary windows cannot reach it.On the two localStorage keys
Every occurrence in production code is
getItemorremoveItem— zerosetItem. Both are compat reads for a user upgrading over a pre-#214 (snapshot) or pre-#501 (breadcrumb) build.Kept, not deleted. They cost four lines and buy that user a clean upgrade; deleting them silently drops their tabs. Added a test that drives the path end to end — pre-v2 records, an untitled entry, a
HOMEsentinel — so the compat path is asserted rather than assumed. Falsified by stubbing the legacy read tonull.Why this data is on the Rust side at all
Worth recording, because #638's rule does not decide it. Rust does not read either file —
load_window_stateandload_restore_progressareread_to_stringhanding bytes to the frontend unparsed. By #638's ownership test ("Rust owns it because Rust reads it", as withpinned_tags) this is not Rust's data.It is there for a different and correct reason:
setItemis an async message to the WebKit storage process, and both records exist specifically to survive the event that loses that message — the last window closing, or the process being killed. That is durability, not ownership — a third category neither precedent covers.npm test872 ·vitest305 ·cargo test148 ·check0 errors.🤖 Generated with Claude Code