fix(save): stop a stale lossy-save refusal from silencing real auto-save failures

[PathGao]

Two guards in documentSession.svelte.ts were reported to me as the same shape — a guard whose name promises more than its condition delivers. One of them is a real bug and is fixed here. The other does not reproduce, and this PR does not change it; the investigation is written up below so the next person does not repeat it.

---

1. A tab that once refused a lossy save went permanently silent about all auto-save failures

The two readers of one predicate

lossySaveWarnedTabs is a Set<string> of tab ids, added to the first time refuseIfLossilyDecoded blocks a write. isLossySaveRefused exposed it verbatim:

function isLossySaveRefused(tabId: string): boolean {
	return lossySaveWarnedTabs.has(tabId);
}

Two call sites read it, and only one of them was safe.

The auto-save eligibility gate (MarkdownViewer.svelte:1833) — correct, because the && supplies the missing half:

const eligible = … && !(s.decodedLossily && documentSession.isLossySaveRefused(s.id));

The auto-save failure handler (MarkdownViewer.svelte:1873) — unconditional:

if (!ok) {
	console.error('Auto-save failed for tab', s.id);
	if (documentSession.isLossySaveRefused(s.id)) return;   // ← no decodedLossily
	addToast(t('toast.autoSaveFailed', settings.language), 'error');
}

The comment at that site explains the intent, and the intent is right: a refusal has already named the file and the way out, so "auto-save failed" on top says nothing new. The condition was broader than the intent.

The set records what was said; it does not track whether it is still true

The set is emptied in exactly three places: loadMarkdown (both branches) and saveContentAs. But hasReplacementChars — the fact the refusal is about — is re-decided by four other reads, each of which fills a writable buffer and therefore carries its own fidelity verdict since #379:

site	clears hasReplacementChars for a file since converted to UTF-8	clears lossySaveWarnedTabs
loadMarkdown (preview + full)	✓	✓
saveContentAs	✓	✓
ensureFullContent	✓	✗
toggleEdit (MarkdownViewer.svelte:1688)	✓	✗
toggleSplitView (MarkdownViewer.svelte:2408)	✓	✗
cross-window arrival (windowSession.svelte.ts:207)	✓	✗

So the set can outlive the condition. And the eligibility gate lets it: once decodedLossily goes false the tab starts auto-saving again — which is correct and intended — while the failure handler stays permanently muted.

  open legacy.md, decoded lossily        hasReplacementChars = true   warned = ∅
  type → auto-save → refused             …                     true   warned = {t}
      → the one toast the user should get: "cannot save, file was decoded lossily"
  leave edit mode; user converts the file to UTF-8 externally
  re-enter edit mode → toggleEdit re-reads it
                                         hasReplacementChars = FALSE  warned = {t}  ← stale
  type → eligible again → auto-save → disk full / permission denied / volume gone
      → console.error, and isLossySaveRefused(t) still says "already explained"
      → t('toast.autoSaveFailed') is suppressed, for the rest of that tab's life

Severity, stated accurately

I was told this leaves "nothing on screen". That is not quite right, and the difference matters for how the fix should be judged.

saveContent's catch already calls options.onError('Failed to save file', error), and MarkdownViewer wires onError to addToast. So a write that throws — the disk-full case — still surfaces a toast, an unlocalized one reading Failed to save file: Os { code: 28, … }. What the stale predicate suppresses is the localized toast.autoSaveFailed on top of it.

The genuinely silent case is a saveContent that returns false without throwing. Today, reached from the auto-save timer, that is only !tab — the tab closed between the timer arming and firing. It is about to get bigger: the per-tab in-flight save guard on fix/one-write-per-tab-at-a-time adds a second such return, in this same file.

So: a real defect in the user-visible messaging path, currently degrading a localized message into a raw one rather than erasing all feedback, and sitting directly in front of a change that widens it. Not a data-loss bug, and this PR does not claim to be one.

The fix

The predicate now means what its name says — is this tab's save being refused for the lossy-decode reason, right now — by asking the tab as well as the memory:

function isLossySaveRefused(tabId: string): boolean {
	if (!lossySaveWarnedTabs.has(tabId)) return false;
	const tab = tabManager.tabs.find((item) => item.id === tabId);
	return tab?.hasReplacementChars === true;
}

Both halves are load-bearing. The set is still what makes the first refusal produce its explanation and the rest stay quiet. The tab is what makes "still" true.

Why not "also clear the set in closeTab". I looked; it is not needed, and the structural obstacle is real — closeTab is a splice with no dispose hook, so there is nowhere to hang the cleanup without introducing one, which is out of scope here. Asking the tab answers the closed-tab case for free: a tab nobody can find is refusing nothing. What remains is that the set keeps closed ids forever. Ids are crypto.randomUUID() and never reused, so this is a bounded leak — one string per lossy-refused tab per window session — not a correctness problem. Left as-is and noted below.

Why the fix is in the session and not at the call site. The predicate is the thing that was wrong, and it has two readers. Fixing the handler would have left the next reader to rediscover it — and isLossySaveRefused exists precisely so that MarkdownViewer does not have to know how refusals are tracked.

MarkdownViewer.svelte is untouched. Notably the s.decodedLossily && in the eligibility gate stays, and is not now redundant: it is the reactive read. isLossySaveRefused is called from inside untrack(…), so its own read of the tab creates no dependency; s.decodedLossily in the snapshot is what re-runs the effect when the flag changes and re-opens the tab for auto-save.

Tests

scripts/lossySaveRefusalScope.test.ts, five cases against the real TabManager and the real documentSession with a stubbed backend. Each starts from a tab that decoded lossily, was edited, and was refused once.

• a tab that no longer decodes lossily is no longer being refused — the core claim;
• a real save failure on such a tab (save_file_content rejects with StorageFull) returns false while the predicate stays false, so the timer is free to report it — the bug, end to end in the shape the timer sees it;
• a closed tab is not remembered as refused;
• the lossy case did not get noisy again: three consecutive refusals on a still-lossy tab produce exactly one message, and the predicate stays true throughout;
• symmetry — a tab that becomes lossy again is refused again, and having been told once is not told twice.

The last two are the regression guard on the behaviour this must not break. They pass on master and must keep passing; the first three are the red-to-green.

Counter-proofs:

mutation	result
predicate reverted to lossySaveWarnedTabs.has(tabId) (i.e. master)	3 of 5 fail — the two preservation cases correctly stay green
predicate hard-wired to return false	5 of 5 fail — the deduplication guard bites

One existing source-text assertion in checkedReadMigration.test.ts pinned the old one-line body; it is updated to pin the property instead (the tab is consulted), pointing at the new file for the behaviour. The two source-text tests over the MarkdownViewer call sites are unchanged and still pass, which is the check that the component side did not need to move.

---

2. resolveExternalChange's raw === — investigated, not changed

resolveExternalChange picks the tab that owns a changed file with string equality, in a file that imports isSameFilePath and uses it three times elsewhere:

active && active.path === changedPath
	? active
	: tabManager.tabs.find((tab) => tab.path === changedPath);

Two findings, and the second is why nothing changed.

The echo is byte-identical, so this is latent, not live

watch_file (src-tauri/src/window_runtime.rs:419) clones the path argument into the notify callback and emits that, never anything from the notify::Event:

let watched_path = path.clone();
let mut watcher = RecommendedWatcher::new(
	move |result: Result<notify::Event, notify::Error>| {
		if result.is_ok() {
			let _ = handle.emit_to(event_label.as_str(), "file-changed", watched_path.clone());
		}
	}, …

The frontend hands it currentFile, which is $derived(tabManager.activeTab?.path ?? '') — the very string resolveExternalChange then compares against. The chain closes: payload ≡ activeTab.path at arm time, and liveModeWatchedPath.test.ts already pins both ends of it. macOS case folding, NFD/NFC, a Windows drive-letter difference and a symlinked directory can none of them get between the two, because the OS's own path never enters the payload.

The proposed fix would not change behaviour even if it were live

This is the part that decided it. isSameFilePath trusts keys only when both sides have one, and degrades to exact path equality otherwise — deliberately, and documented as such. The watcher payload is a bare string with no pathKey. So isSameFilePath({ path: changedPath }, tab) is changedPath === tab.path, including against a fully-resolved tab:

                      ===      isSameFilePath   differs
macOS case fold       false    false            false
NFD vs NFC            false    false            false
Windows drive case    false    false            false
symlinked directory   false    false            false

(run against the real isSameFilePath, with the tab given a pathKey — the best case for the identity comparison)

Making it bite would mean canonicalizing changedPath, which is a Tauri round-trip on every external-change event and would turn a synchronous resolver into an async one — restructuring the external-change flow, which is a design decision for the maintainer rather than a drive-by. The performance question is moot for the same reason: isSameFilePath here compiles to the same string compare, and the cost only appears in the version that would actually do something.

Changing the line as proposed would have been a diff that reads like a fix, passes review, and does nothing. Left alone.

---

Verification

On upstream/master (b63ff2e):

npm test        533 / 533   (528 before this branch, + 5 new)
npm run check   632 files, 0 errors, 0 warnings
npm run build   clean

cargo test not run: no Rust changed. The Rust in section 2 was read, not modified.

Not covered

• No run in the real app. Both defects were exercised against the real TabManager and the real documentSession with a stubbed Tauri bridge. Nobody filled a disk under a converted-to-UTF-8 file by hand.
• The unlocalized Failed to save file: <error> toast is untouched. It is what a throwing write shows today, now alongside the localized one. Whether a failed auto-save should raise one message or two, and whether a raw Os { code: 28 } belongs in front of a user, is a messaging question this PR does not open.
• lossySaveWarnedTabs still grows by one entry per lossy-refused tab, for the window's lifetime. Correctness is now independent of it. The dispose hook closeTab would need is a separate structural change.
• resolveExternalChange still compares raw strings, for the reasons in section 2. The invariant it depends on is enforced by liveModeWatchedPath.test.ts on both the Rust and the frontend side; no new test was added, since a third assertion of the same fact would be noise.
• Rebase. This branch does not touch saveContent, so it should not collide with the in-flight save guard on fix/one-write-per-tab-at-a-time. If that lands first, note that its new "already saving" early return is a saveContent returning false without throwing — exactly the silent case section 1 restores the toast for.

🤖 Generated with Claude Code