Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions apps/nec-gui/src/app_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,20 @@ pub struct EditorState {
pub error: Option<String>,
/// Result of the most recent Save, shown to the user.
pub save_status: String,
/// **The file this document belongs to** — the path a plain `Save` writes to.
///
/// Deliberately *not* `deck_path`. That field is global chrome, editable on
/// the Editor tab itself, and `Save` used to clone it live: load deck A,
/// retype the path to B without loading it, click Save, and A's text
/// truncated B (FND-103). The document's file is a different fact from the
/// path the chrome is pointing at, and conflating them cost an unrelated
/// file.
///
/// `None` means the document has no file yet. Unreachable in the shipped UI
/// today — Save exists only once `loaded` is true, and only an accepted load
/// sets that — but `Save` refuses rather than guessing, so a future "New
/// deck" button cannot inherit the truncation.
pub file_path: Option<String>,
}

/// State of the GPU 3-D viewport. The camera and mesh are pure data (rendered by
Expand Down Expand Up @@ -644,6 +658,17 @@ impl AppState {
self.editor_save_run
}

/// The file a plain `Save` writes to, or `None` if this document has none.
///
/// Exists so the decision is reachable by a test. It used to be one inline
/// `self.state.deck_path.clone()` in the binary's `spawn_save`, which no test
/// could see — which is why FND-103 shipped: the defect lived in the one
/// place the suite could not look, under a comment that said it did the right
/// thing ("write it back over the loaded path").
pub fn save_target(&self) -> Option<&str> {
self.editor.file_path.as_deref()
}

/// As [`AppState::current_solve_run`], for the viewport's geometry leg.
pub fn current_geometry_run(&self) -> Option<RunId> {
self.viewport.pending_geometry
Expand Down Expand Up @@ -907,6 +932,23 @@ impl AppState {
}
Message::EditDeckLoaded(_, Ok(doc)) => {
self.editor_load_run = None;
// A save still in flight is retired here too — but NOT by this
// arm. `refresh_editor_preview()`, called at the end of it,
// already clears both run ids unconditionally (FND-133/#445), so
// the sequence "Save A, load B, save completes" cannot rebind
// this document to A. The design review of this change predicted
// that hole and it does not exist; an explicit clear added here
// was removed after a sabotage showed it changed nothing. Pinned
// by `a_completed_load_retires_a_save_still_in_flight`.
//
// The path this document now belongs to. Not carried in the
// message: an ACCEPTED load implies `deck_path` is unchanged
// since the load was armed, because `DeckPathChanged` retires the
// load run — pinned by
// `a_deck_path_change_retires_a_load_spawned_for_the_old_path`.
// Empty is normalised to `None` so a state that never had a path
// does not acquire an empty one.
self.editor.file_path = Some(self.deck_path.clone()).filter(|p| !p.is_empty());
self.editor.doc = doc.clone();
self.editor.history.reset();
self.editor.loaded = true;
Expand Down Expand Up @@ -1002,12 +1044,29 @@ impl AppState {
self.refresh_editor_preview();
}
Message::SaveDeck => {
// Decided here rather than in the binary, so it is reachable by a
// test: `spawn_save` follows the armed id and no longer chooses a
// path of its own. Falling back to `deck_path` would BE the
// defect — `to_deck_string()` succeeds on an empty document, so
// the fallback would truncate whatever path was typed to an empty
// deck.
if self.editor.file_path.is_none() {
self.editor.save_status = "This document has no file yet — use Save as…".into();
return;
}
self.editor.save_status = "Saving…".into();
let id = self.mint_run();
self.editor_save_run = Some(id);
}
Message::DeckSaved(_, Ok(path)) => {
self.editor_save_run = None;
// Both save routes end here — the async task from `SaveDeck` and
// the inline write from "Save as…" — so binding the document to
// the file it was just written to fixes the second half of
// FND-103 for free: Save-as to C used to leave the document bound
// to whatever it was before, so the NEXT plain Save went back to
// the old file rather than to C.
self.editor.file_path = Some(path.clone());
self.editor.doc.mark_saved();
self.editor.save_status = format!("Saved to {path}");
}
Expand Down
37 changes: 31 additions & 6 deletions apps/nec-gui/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -314,12 +314,21 @@ impl FnecGui {
move |r| Message::EditDeckLoaded(run, r),
)
} else if spawn_save {
// Render the edited deck and write it back over the loaded path.
let path = self.state.deck_path.clone();
let run = self
.state
.current_edit_save_run()
.expect("the save was just armed");
// Render the edited deck and write it back over the file this
// document belongs to — which is NOT `deck_path`. That field is
// chrome the user can retype without loading, and cloning it live
// meant "load A, retype B, Save" truncated B with A's text (FND-103).
//
// Both values come from the reducer now: it decides whether a save
// happens at all (a document with no file refuses and arms nothing)
// and which file it targets. This branch follows, and can no longer
// pick a path of its own.
let (Some(run), Some(path)) = (
self.state.current_edit_save_run(),
self.state.save_target().map(str::to_owned),
) else {
return Task::none();
};
match self.state.editor.doc.to_deck_string() {
Ok(text) => Task::perform(
async move {
Expand Down Expand Up @@ -1197,6 +1206,21 @@ impl FnecGui {
};
let save_status = text(self.state.editor.save_status.clone()).width(Length::Fill);

// Which file `Save` will write to, stated rather than implied.
//
// Not decoration. `Save` targets the document's own file, which can
// differ from the deck path in the chrome above — retype that box, or
// use "Save as…", and the two diverge. Before FND-103 they could not
// diverge because Save simply used the chrome, which is exactly how it
// came to truncate an unrelated file. Fixing the target without showing
// it would leave the user unable to tell where Save goes except by
// clicking it.
let editing_line = text(match self.state.save_target() {
Some(p) => format!("Editing: {p}"),
None => "Editing: (no file yet — use Save as…)".to_string(),
})
.width(Length::Fill);

// ── Sources & environment (EX/GN/LD/FR editors) ──────────────────────
let add_bar = row![
text("Sources & environment"),
Expand Down Expand Up @@ -1241,6 +1265,7 @@ impl FnecGui {
status,
controls,
solve_line,
editing_line,
save_status,
]
.spacing(8)
Expand Down
153 changes: 153 additions & 0 deletions apps/nec-gui/tests/gui_smoke.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1008,9 +1008,17 @@ FR 0 1 21.0 0
EN
";

/// The path `loaded_editor()` pretends the document came from.
const LOADED_FROM: &str = "/tmp/fnec-loaded-from.nec";

fn loaded_editor() -> AppState {
let mut state = AppState::default();
let doc = load_model_doc_str(EDITOR_DECK).expect("parse doc");
// A path, because a load binds the document to whatever `deck_path` said
// when it was armed (FND-103). Without one every `Save` in this suite would
// exercise the has-no-file refusal instead of the save it means to test —
// passing for the wrong reason.
state.apply(&Message::DeckPathChanged(LOADED_FROM.into()));
state.apply(&Message::EditDeckLoad);
let run = state
.current_edit_load_run()
Expand Down Expand Up @@ -3126,3 +3134,148 @@ fn a_deck_with_no_frequency_is_refused_by_both_gui_seams() {
);
}
}

/// `Save` writes to the file the document was loaded from, not to whatever the
/// deck-path box currently says.
///
/// The defect (FND-103): the path box is global chrome, editable on the Editor
/// tab itself, and `spawn_save` cloned it live. 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.
///
/// The assertion is on `save_target()` rather than on a written file because the
/// write happens in the binary's `Task`. Extracting that decision into the
/// reducer is half the fix: the defect lived in the one place the suite could
/// not look, under a comment claiming it wrote "back over the loaded path".
#[test]
fn save_targets_the_loaded_file_not_the_retyped_path() {
let mut state = loaded_editor();
assert_eq!(
state.save_target(),
Some(LOADED_FROM),
"a load must bind the document to the file it came from"
);

state.apply(&Message::DeckPathChanged(
"/tmp/fnec-unrelated-B.nec".into(),
));
assert_eq!(
state.deck_path, "/tmp/fnec-unrelated-B.nec",
"the chrome follows the user's typing"
);
assert_eq!(
state.save_target(),
Some(LOADED_FROM),
"but the document still belongs to the file it was loaded from — this is \
the truncation FND-103 caused"
);
}

/// "Save as…" rebinds the document, so the next plain `Save` goes to the new
/// file.
///
/// Not in the FND-103 row; found by measuring it. `BrowseSaveDeck` wrote the
/// file and marked the document clean but bound nothing, so after saving as C a
/// later `Save` went back to the previous file — which no editor does.
#[test]
fn save_as_rebinds_the_document_to_the_new_file() {
let mut state = loaded_editor();
state.apply(&Message::BrowseSaveDeck);
let run = state
.current_edit_save_run()
.expect("Save as… arms a save run");
state.apply(&Message::DeckSaved(
run,
Ok("/tmp/fnec-saved-as-C.nec".into()),
));

assert_eq!(
state.save_target(),
Some("/tmp/fnec-saved-as-C.nec"),
"after Save as… the document belongs to the new file"
);
assert!(
!state.editor.doc.dirty,
"and is clean, as it already was before this change"
);
}

/// A load retires a save still in flight, so a completed save cannot rebind a
/// document it was not written from.
///
/// The risk is real: run identity rejects a *superseded* run, and a save spawned
/// for file A is not superseded by a **load**. Save A is armed, the user loads
/// deck B, the load lands and binds the document to B, and then the older
/// `DeckSaved(Ok(A))` arrives — if it were accepted it would rebind B's document
/// to A, putting B's text one click of Save away from overwriting A. FND-103
/// again, one load later.
///
/// **It is already prevented, and not by this change.** The design review
/// predicted this hole; sabotage showed it does not exist. An accepted load ends
/// by calling `refresh_editor_preview()`, which clears both run ids
/// unconditionally (FND-133/#445) — so an explicit clear added to the load arm
/// changed nothing and was removed. What this test adds is coverage of the
/// **load** path for that mechanism, where
/// `an_edit_retires_a_deck_write_still_in_flight` covers the edit path: removing
/// the clear from `refresh_editor_preview` fails both.
#[test]
fn a_completed_load_retires_a_save_still_in_flight() {
let mut state = loaded_editor();
state.apply(&Message::SaveDeck);
let stale_save = state
.current_edit_save_run()
.expect("SaveDeck arms the save");

// The user points at another deck and loads it before the write finishes.
state.apply(&Message::DeckPathChanged("/tmp/fnec-deck-B.nec".into()));
state.apply(&Message::EditDeckLoad);
let load = state
.current_edit_load_run()
.expect("EditDeckLoad arms the load");
let doc_b = load_model_doc_str(EDITOR_DECK_ALT).expect("parse alt doc");
state.apply(&Message::EditDeckLoaded(load, Ok(doc_b)));
assert_eq!(
state.save_target(),
Some("/tmp/fnec-deck-B.nec"),
"the load binds the document to its own file"
);

// The write for the PREVIOUS document now completes.
state.apply(&Message::DeckSaved(stale_save, Ok(LOADED_FROM.into())));
assert_eq!(
state.save_target(),
Some("/tmp/fnec-deck-B.nec"),
"a save that completed for the previous document must not rebind this one"
);
}

/// A document with no file refuses to plain-`Save` rather than guessing.
///
/// Unreachable in the shipped UI today — Save exists only once a deck is loaded,
/// and only an accepted load sets that — so this is a guard for the future "New
/// deck" button. It matters because the tempting fallback, "use `deck_path`", is
/// the defect: `to_deck_string()` succeeds on an empty document, so the fallback
/// would truncate whatever path was typed to an empty deck.
#[test]
fn a_document_with_no_file_refuses_to_save() {
let mut state = AppState::default();
state.apply(&Message::DeckPathChanged(
"/tmp/fnec-typed-but-never-loaded.nec".into(),
));
assert_eq!(
state.save_target(),
None,
"nothing was ever loaded or saved"
);

state.apply(&Message::SaveDeck);
assert!(
state.current_edit_save_run().is_none(),
"no save may be armed for a document with no file"
);
assert!(
state.editor.save_status.contains("Save as"),
"and the user is told what to do instead: {}",
state.editor.save_status
);
}
16 changes: 16 additions & 0 deletions docs/changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,22 @@ from 0.13.0 and earlier predate the Keep a Changelog headings and are left as wr

## [Unreleased]

### Fixed

- **The GUI's `Save deck` writes to the file the document belongs to, not to
whatever the deck-path box says.** That box is global chrome, editable on the
Editor tab itself, and `Save` cloned it live — so loading deck A, retyping the
box to B without loading it, and clicking Save truncated B with A's text
(FND-103). Ordinary click sequence, default config, an unrelated file
destroyed.
- **`Save as…` now rebinds the document**, so a later `Save deck` goes to the
file just written rather than back to the previous one. Not recorded in the
ledger row; found by measuring it.
- The editor shows an **`Editing:`** line naming the file `Save` will write to.
The document's file and the chrome's deck path are two different facts and can
legitimately differ; before this they could not, because Save simply used the
chrome, which is how it came to truncate an unrelated file.

### Changed

- **A deck with no frequency at all is refused instead of silently succeeding.**
Expand Down
13 changes: 10 additions & 3 deletions docs/gui-guide.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
project: fnec-rust
doc: docs/gui-guide.md
status: living
last_updated: 2026-08-27
last_updated: 2026-09-08
---

# fnec-gui user guide
Expand Down Expand Up @@ -124,8 +124,15 @@ have yet, and each row's **Del** to remove one.

- **Undo / Redo** (or `Ctrl+Z` / `Ctrl+Shift+Z` / `Ctrl+Y`) — full edit history;
typing a value coalesces into one undo step.
- **Save deck** writes back over the loaded path; **Save as…** opens a native
save dialog.
- **Save deck** writes back over the file the document belongs to — the one it
was loaded from, or the one the last **Save as…** wrote. That file is shown on
the *Editing:* line under the editor, and it is **not** necessarily the deck
path in the box at the top: retyping that box points the Load/Solve chrome
somewhere else without moving the document. Until v0.18.0 `Save deck` used the
box, so loading deck A, retyping the box to B and clicking Save truncated B
with A's text (FND-103).
- **Save as…** opens a native save dialog and rebinds the document to the file it
writes, so a later **Save deck** goes there too.
- **Apply + Solve** solves the edited in-memory deck and shows the impedance,
without saving first.

Expand Down
Loading
Loading