fix(gui): bind a document to its file, so Save cannot truncate another one - #452
Merged
Conversation
…r one
`Save deck` cloned the LIVE `deck_path`. That field is global chrome, editable on
the Editor tab itself, so: load deck A, retype the box to B without loading it,
click Save — and A's text truncated B. Ordinary click sequence, default config,
an unrelated file destroyed (FND-103).
Measured through `AppState::apply` before touching anything:
after loading A: deck_path=/tmp/deckA.nec
after retyping B (no load): deck_path=/tmp/deckB.nec <- Save writes HERE
after Save-as to C: deck_path=/tmp/deckB.nec <- a later Save too
The third line is a second defect the ledger row does not record: **Save-as never
rebound the document**, so after saving as C the next plain Save went back to the
previous file. No editor does that.
Both fall out of one field. `EditorState::file_path` is the file the document
belongs to, set by an accepted load and by `DeckSaved(_, Ok(path))` — and since
both save routes end at that message, Save-as is fixed for free.
`AppState::save_target()` names the decision so a test can reach it, which
nothing could before: the defect lived in the binary's `spawn_save`, the one
place the suite cannot look, under a comment asserting it wrote "back over the
loaded path". The reducer now decides both whether a save happens and where it
goes; the binary follows the armed id and can no longer pick a path.
A document with no file refuses rather than guessing. Unreachable today — Save
exists only once a deck is loaded — but the tempting fallback IS the defect:
`to_deck_string()` succeeds on an empty document, so falling back to `deck_path`
would truncate whatever was typed to an empty deck.
The GUI shows an `Editing:` line naming the target. Fixing where Save goes
without showing it would leave the user unable to tell except by clicking, which
is how the next row gets written.
**The design review predicted a hole that does not exist, and my own sabotage
caught me asserting otherwise.** It reasoned that a save completing after a load
would rebind the new document to the old file, so the load arm must retire the
in-flight save. I added that line and wrote a test comment calling it a test of
the design. Removing the line then changed nothing: an accepted load ends by
calling `refresh_editor_preview()`, which clears both run ids unconditionally
(FND-133/#445). The line is gone and the comment now says what is true. The test
survives because it does discriminate — against the real mechanism, covering the
LOAD path where `an_edit_retires_a_deck_write_still_in_flight` covers the edit
path; deleting that clear fails both.
Sabotage, three ways: bind to the live `deck_path` -> three of the four new tests
fail; remove the (redundant) clear from the load arm -> nothing fails, which is
how the false claim was found; remove the real clear from
`refresh_editor_preview` -> the load-path and edit-path tests fail together.
Split out rather than bundled: FND-152 (both deck writes are non-atomic
`std::fs::write`, untouched here) and FND-153 (saving a deck loaded with
`--vars` writes the substituted text back over its template, pre-existing and
sharpened by an explicit binding).
Gate: scripts/check-all.sh, EXIT=0, 15/15.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018p7FxX7QMWNJVNp9LbaLkB
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 defect
Save deckcloned the livedeck_path. That field is global chrome, editable on the Editor tab itself — so:Ordinary click sequence, default config, an unrelated file destroyed (FND-103).
Measured through
AppState::applybefore touching anything:That third line is a second defect the ledger row does not record.
Save as…never rebound the document, so after saving as C the next plain Save went back to the previous file. No editor does that.The fix
EditorState::file_path— the file the document belongs to. Set by an accepted load, and byDeckSaved(_, Ok(path)); since both save routes end at that message, Save-as is fixed for free.AppState::save_target()names the decision so a test can reach it. Nothing could before: the defect lived in the binary'sspawn_save, the one place the suite cannot look, under a comment asserting it wrote "back over the loaded path". The reducer now decides both whether a save happens and where it goes; the binary follows the armed id and can no longer pick a path of its own.A document with no file refuses rather than guessing. Unreachable today — Save exists only once a deck is loaded — but the tempting fallback is the defect:
to_deck_string()succeeds on an empty document, so falling back todeck_pathwould truncate whatever was typed to an empty deck.The editor shows an
Editing:line naming the target. Fixing where Save goes without showing it would leave the user unable to tell except by clicking — which is how the next ledger row gets written.The review predicted a hole that does not exist, and my own sabotage caught me asserting otherwise
The design review reasoned that a save completing after a load would rebind the new document to the old file, so the load arm must retire the in-flight save. I added that line and wrote a test comment calling it "a test of the design".
Removing the line then changed nothing. An accepted load ends by calling
refresh_editor_preview(), which clears both run ids unconditionally (FND-133/#445) — the reviewer read the load arm but not the call at the end of it. The line is gone; the comment now says what is true.The test survives because it does discriminate — against the real mechanism, covering the load path where
an_edit_retires_a_deck_write_still_in_flightcovers the edit path.deck_pathrefresh_editor_previewSplit out rather than bundled
FND-103 bundled three things. This closes the path-binding defects and records the rest honestly:
std::fs::write; write-to-temp-then-rename has its own failure modes and no test exercises an interrupted write.--varswrites14.2back over$FREQ. Pre-existing, and sharpened by an explicit binding.Gate
scripts/check-all.sh→ EXIT=0, 15/15.docs/gui-guide.md's claim that Save "writes back over the loaded path" was false and is now true and explained.🤖 Generated with Claude Code
https://claude.ai/code/session_018p7FxX7QMWNJVNp9LbaLkB