Skip to content

fix(save): let Save As be the way out of a partial buffer (#547) - #553

Merged
PathGao merged 1 commit into
masterfrom
fix/save-as-escape-for-partial-buffer
Aug 8, 2026
Merged

fix(save): let Save As be the way out of a partial buffer (#547)#553
PathGao merged 1 commit into
masterfrom
fix/save-as-escape-for-partial-buffer

Conversation

@PathGao

@PathGao PathGao commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator

What this is

A tab holding only the first 50KB of its document refused both writes: the
ordinary save, correctly — it would truncate the file — and Save As as well. So a
buffer in that state could go nowhere.

overwrite the original   refused   correct: it would truncate the file
write a copy elsewhere   refused   <- this one
reopen to clear it       discards whatever was typed

Anything typed into that state had no exit but selecting the text and copying it
out by hand. This opens the copy.

Related to #547 but not its fix — that is #552, which stops the state being
reached. This is about what happens to a reader who is already in it.

Mechanism

A copy is a NEW file at a path the user chose. It cannot destroy anything they
already have, which is what makes it the one write a partial buffer may make.
refuseIfLossilyDecoded already reasons exactly this way — it refuses the
overwrite and points at Save As in so many words — and the trade is the same here:
a short copy the reader is told about beats edits with nowhere to go.

Completed first where that is possible, so the copy is whole whenever it can be.
ensureFullContent declines on a dirty buffer, since reading the file over unsaved
edits would discard the very thing being rescued, and that is exactly the case
where the copy really is short.

Three things had to move together, or the rescue frees the text and loses it again:

why
the refusal leaves saveContentAs replaced by the completion attempt
isTruncated cleared after the write it described the file this tab used to point at
the overwrite refusal names the exit it could not, before there was one

The middle one is the part that is easy to miss. updateTabPath repoints the tab
at the copy, and the two encoding flags are already cleared right there for exactly
this reason — "the buffer now has a UTF-8 file of its own that it matches exactly".
The truncation flag needed the same treatment: whatever the buffer was a slice OF,
it is the whole of what was just written. Left on, every later save would be
refused from a tab holding a complete document.

On the message: it was tried against toast.partialDocument first and that is
wrong here. That sentence says "cannot edit yet", which is right at the five entry
points that block entering the editor, and wrong by the time a save is refused,
because the reader is already editing. toast.partialSaveBlocked names the load
and points at Save As — which it could only do once Save As worked.

Scope

onPartialCopySaved, not an error. The write succeeded; a copy that stops
where the load did is a fact about the file, not a failure. It follows
onCloseSaveNewerEdits, the existing shape for a session-level notice that is not
an error. Required rather than optional, like every other callback in that type, so
a caller cannot quietly leave the reader unwarned — which is why eleven test stubs
gained a line.

Two new keys, six locales (en, zh-CN, ja, zh-TW, ko, ru). Toast keys in this
file are not held to all 26: t() falls back to English on a miss,
partialDocument carries three locales and lossySaveBlocked six. This matches
its sibling rather than the menu labels, which are held to 26 for a different
reason. The ja, ko and ru strings are machine-produced and want a native
reader
— the same standing as the lossySaveBlocked entries beside them.

Not fixed here: the auto-save timer still stacks "auto-save failed" behind the
refusal.
It suppresses that for the lossy refusal, on the grounds that a refusal
has already explained itself, and the same is now true of this one — but that
predicate is read by the eligibility gate too, with a different meaning, so
widening it is its own change.

Tests

truncatedBufferGuard had a test asserting Save As refuses. That behaviour is what
this changes, so it is replaced by two pinning the new one:

  • a clean tab gets a whole copy and no notice — reaching for Save As is not a
    reason to write less than the document;
  • a dirty tab gets a short copy, the notice, a cleared flag, and a save that works
    afterwards.

Revert the change in saveContentAs and both go red.

Verification

npm audit       0 vulnerabilities
npm run check   674 files, 0 errors, 0 warnings
npm test        937 pass, 0 fail
cargo test      145 passed, 0 failed   (no Rust file changes on this branch)

Not verified: any of this by hand in a running build. The state it operates on
is one this branch cannot reach on demand — see #552 for why the race that produces
it did not reproduce outside the test harness. What is exercised is the state
itself, constructed directly, and the writes that follow from it.

On #547 more generally, and this is worth saying plainly: #552 and this PR both
close real defects, and neither is confirmed to be what the reporter actually hit.
The observation was a blank editor pane and then every auto-save failing. #552 fixes
a race that produces the second half of that, but it was never reproduced in a
packaged build. The blank pane is a different cause — a restore that could not read
its file — and is untouched by both. So: the defect surface is smaller than it was,
and the original incident remains unexplained and currently not reproducible.

Ordering

Independent of #552 — different functions, no overlapping hunks — and can merge in
either order. Both are based on a71d4d7.

🤖 Generated with Claude Code

A tab holding only the first 50KB of its document refused both writes: the
ordinary save, correctly — it would truncate the file — and Save As as well. So a
buffer in that state could go nowhere. Anything typed into it had no exit but
selecting the text and copying it out by hand, and reopening the file to clear the
state discarded it.

A copy is a NEW file at a path the user chose. It cannot destroy anything they
already have, which is what makes it the one write a partial buffer may make.
`refuseIfLossilyDecoded` has always reasoned exactly this way — it refuses the
overwrite and points at Save As, in so many words — and the same trade applies
here: a short copy the user is told about is a better answer than edits with
nowhere to go.

Completed first where that is possible, so the copy is whole whenever it can be.
`ensureFullContent` declines on a dirty buffer, since reading the file over unsaved
edits would discard the very thing being rescued, and that is exactly the case
where the copy really is short.

Three things had to move together for the rescue to actually free the text:

- The refusal is gone from `saveContentAs`, replaced by the completion attempt.
- `isTruncated` is cleared after the write, beside the two encoding flags already
  cleared there and for the same reason: the flag described the file this tab used
  to point at. Whatever the buffer was a slice OF, it is the whole of what was just
  written, and the tab now points at that. Left on, every later save would be
  refused from a tab holding a complete document — the rescue would free the text
  and then trap it again.
- The overwrite refusal now says `toast.partialSaveBlocked`, which names the load
  and points at the exit. It could not say that before, because there was no exit.
  The old string was untranslated English plus `Error: /path`, and it was tried
  against `toast.partialDocument` first — that sentence says "cannot edit yet",
  which is right where it is used, at the five entry points that block *entering*
  the editor, and wrong by the time a save is refused, because the reader is
  already editing.

`onPartialCopySaved` rather than an error: the write succeeded, and a copy that
stops where the load did is a fact about the file, not a failure. It follows
`onCloseSaveNewerEdits`, which is the existing shape for a session-level notice
that is not an error. Required rather than optional, like every other callback in
that type, so a caller cannot quietly leave the reader unwarned — which is why
eleven test stubs gained a line.

Two new keys, in the six locales `lossySaveBlocked` carries (en, zh-CN, ja, zh-TW,
ko, ru). Toast keys in this file are not held to all 26 — `t()` falls back to
English on a miss, `partialDocument` has three locales and `lossySaveBlocked` six —
so this matches its sibling rather than the menu labels, which are held to 26 for a
different reason. The ja, ko and ru strings are machine-produced and want a native
reader, the same standing as the `lossySaveBlocked` entries beside them.

Tests. `truncatedBufferGuard` had a test asserting Save As refuses; that behaviour
is what this changes, so it is replaced by two that pin the new one — a clean tab
gets a whole copy and no notice, and a dirty one gets a short copy, the notice, a
cleared flag, and a save that works afterwards. Revert the change in
`saveContentAs` and both go red.

Not fixed here: the auto-save timer still stacks "auto-save failed" behind the
refusal. It suppresses that for the lossy refusal, on the grounds that a refusal
has already explained itself, and the same is true now that this one does too — but
that predicate is read by the eligibility gate as well, with a different meaning,
so widening it is its own change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@PathGao
PathGao merged commit dd9543f into master Aug 8, 2026
4 checks passed
@PathGao
PathGao deleted the fix/save-as-escape-for-partial-buffer branch August 8, 2026 13:08
@PathGao

PathGao commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator Author

逻辑正确。Save As 写的是新文件,不能破坏任何已存在的东西,所以它是截断缓冲唯一应该允许的写入操作。

三个一起动的点都是对的:

  • 允许 Save As:新路径,没风险。
  • 先尝试完整化:clean 的 tab 可以先读完文件再写副本,dirty 的 tab 不能(读完会覆盖未保存编辑),后者才真正写入短副本。
  • 写完清 isTruncated:tab 现在指向新文件,buffer 对它来说就是完整的。不清的话后续每次保存还被拒绝——从用户视角文件已经 "Save As" 成功了,再弹 "cannot save" 是 bug。

即使 #552 堵住了竞态,这个逃生舱也应该存在。两个原因:

  1. fix(load): stop an overtaken load from stranding a document on its slice (#547) #552 修的是 STARTUP 竞态,但 isTruncated 还可以从 session restore 路径进入(markTabContentUnavailable)——那个路径不在 fix(load): stop an overtaken load from stranding a document on its slice (#547) #552 覆盖范围。
  2. 有逃生舱意味着即使未来有新的路径进入这个状态,用户也不会被困住。

PathGao added a commit to PathGao/Markpad that referenced this pull request Aug 8, 2026
…vision test

PR sftwrdotdev#553 added the onPartialCopySaved callback to DocumentSessionOptions.
The test stub in largeFileLoadRevision.test.ts was missing it, causing
svelte-check to fail.
PathGao added a commit that referenced this pull request Aug 8, 2026
PR #552 introduced largeFileLoadRevision.test.ts.  PR #553 added the
onPartialCopySaved callback to DocumentSessionOptions but did not update
this test stub.  svelte-check fails on master because the required
property is missing.
PathGao added a commit that referenced this pull request Aug 8, 2026
* fix(load): raise large-file preview threshold from 50KB to 5MB

The 50KB two-stage load threshold was never measured.  On a 121KB file
the full read is 0.032ms vs 0.022ms for the first 50KB — a 0.01ms
difference.  The mechanism only earns its keep on multi-MB files (roughly
two orders of magnitude above where it currently engages), yet the
50KB threshold means every document over ~50KB incurs the complexity of
isTruncated flags, revision guards, and save refusals.

At 5MB the threshold still protects genuine large files while letting
99.9% of real-world Markdown documents bypass the entire two-stage path.

Tests are decoupled from the threshold: the preview mock always returns
isFull=false regardless of content size, so the test exercises the
mechanism without needing multi-MB test strings.

* fix(test): add missing onPartialCopySaved callback in largeFileLoadRevision test

PR #553 added the onPartialCopySaved callback to DocumentSessionOptions.
The test stub in largeFileLoadRevision.test.ts was missing it, causing
svelte-check to fail.

---------

Co-authored-by: PathGao <PathGao@users.noreply.github.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