Skip to content

fix(documents): stop partial buffers and external changes from destroying work - #374

Merged
PathGao merged 1 commit into
sftwrdotdev:masterfrom
PathGao:fix/truncated-buffer-and-reload-guards
Aug 2, 2026
Merged

fix(documents): stop partial buffers and external changes from destroying work#374
PathGao merged 1 commit into
sftwrdotdev:masterfrom
PathGao:fix/truncated-buffer-and-reload-guards

Conversation

@PathGao

@PathGao PathGao commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

A truncated preview buffer could be written back over the whole file

Opening a document larger than 50 KB starts with open_markdown_preview(maxBytes: 50000), and setTabRawContent made that partial text both the buffer and the baseline — isDirty=false, nothing marking it incomplete. The background full read is abandoned if the tab changes mode or gets edited meanwhile, and toggleSplitView only re-read when !tab.rawContent; a partial buffer is not empty, so it did not.

Four routes reached that state, not the one in the original report:

Route What the user did
Split view Ctrl+\ during the ~2 s window, then typed — auto-save wrote the partial buffer
Task checkbox ticked a checkbox from reading mode on a large document
Front matter edited a property from reading mode
reloadFromDisk / F5 handed the editor itself a partial buffer

A tab now records whether its buffer is partial, every editable entry point completes it from disk first, and saving refuses a partial buffer as a backstop.

Detaching completes it too. TransferableTab has no field for the flag and validateTransferPayload rebuilds the tab explicitly, so an unknown field is dropped — the destination window would have inherited a short buffer that looked authoritative, with its own auto-save timer. Fixed on the source side, with canTransfer/canDetach as a fail-closed backstop.

An external change overwrote unsaved edits silently

The watcher listener checked live mode and the self-write grace window but never tab.isDirty — and setTabRawContent rewrites originalContent too, so a git checkout, a cloud sync, or another window saving the same file took the edits with no trace that anything had been dirty. A dirty tab now raises a conflict the user answers (reload / keep mine) instead of being reloaded under them.

The debounced auto-save is held back while a conflict is unanswered. Otherwise the timer writes 1.5 s later and drops the external change while the bar is still asking which version to keep. Explicit saves still go through — pressing Save is the answer "keep mine", and the save path clears the conflict so the bar comes down rather than re-asking. The debounce is the only thing that ever writes without being asked, so it is the only thing suppressed. That rule covers the close and mode-toggle dialogs too, which reach disk through the same wrapper.

Live Mode needed no change

Cmd/Ctrl+L overwriting the buffer was already fixed by #296 — toggling now installs the watcher without reloading. Verified on this baseline; the existing regression lock is kept and repointed at the new tests.

Validation

  • npm run check — 0 errors, 0 warnings
  • npm test — 226/226

Baseline counter-check: the two new files were extracted onto an untouched tree and run there — 19 of 21 assertions fail on master. The two that pass are deliberate: a negative control ("a fully loaded buffer is not marked as incomplete") and the #296 Live Mode lock. The check was re-run after rebasing onto #371 to confirm that PR's Rust read-path changes did not accidentally satisfy any of them; every assertion that was red before is still red.

These are behaviour tests, not source-pattern matching: they install a rune shim, import the real tabManager and createDocumentSession, and stub the Tauri bridge at window.__TAURI_INTERNALS__. Only four assertions that live inside .svelte markup are source checks.

Note

Four i18n keys are added for the conflict bar and the partial-document toast, filled for en and zh-CN; the other locales fall back to English, which is the existing pattern in this file for recently added strings.

…ying work

**A truncated preview buffer could be written back over the whole file.**
Opening a document larger than 50 KB starts with
`open_markdown_preview(maxBytes: 50000)`, and `setTabRawContent` made that
partial text both the buffer and the baseline, with `isDirty=false` and
nothing marking it incomplete. The background full read is abandoned if the
tab changes mode or gets edited in the meantime, and `toggleSplitView` only
re-read when `!tab.rawContent` — a partial buffer is not empty, so it did
not. Anything that then wrote the buffer truncated the file at 50 KB.

Four routes reached that state, not one: entering split view, toggling a
task checkbox from reading mode, editing front matter from reading mode, and
`reloadFromDisk` (F5), which handed the editor itself a partial buffer.

A tab now records whether its buffer is partial, every editable entry point
completes it from disk first, and saving refuses a partial buffer as a
backstop. Detaching a tab to another window completes it too: the transfer
payload has no field for the flag and rebuilds the tab explicitly, so the
destination would have inherited a short buffer that looked authoritative,
with its own auto-save timer.

**An external change overwrote unsaved edits silently.** The watcher
listener checked live mode and the self-write grace window but never
`tab.isDirty`, and `setTabRawContent` rewrites `originalContent` too, so a
`git checkout`, a cloud sync, or another window saving the same file took
the edits with no trace that anything had been dirty. A dirty tab now raises
a conflict the user answers — reload, or keep mine — instead of reloading
under them.

The debounced auto-save is held back while a conflict is unanswered.
Otherwise the timer would write 1.5 s later and drop the external change
while the bar was still asking which version to keep. Explicit saves still
go through: pressing Save *is* the answer "keep mine", and the save path
clears the conflict so the bar comes down rather than re-asking. The
debounce is the only thing that ever writes without being asked, so it is
the only thing suppressed.

Live Mode itself needed no change here — #296 already made toggling it
install the watcher without reloading. The regression lock for that is kept
and now points at the new tests.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@PathGao
PathGao merged commit 41c7965 into sftwrdotdev:master Aug 2, 2026
4 checks passed
@PathGao
PathGao deleted the fix/truncated-buffer-and-reload-guards branch August 2, 2026 19:56
PathGao added a commit that referenced this pull request Aug 6, 2026
…itch (#471)

One editor, one implicit model, and a tab switch that overwrote it with
`editor.setValue(content)`. `TextModel.setValue` is *defined* to throw
the undo stack away — `_setValueFromTextBuffer` runs
`this._commandManager.clear()` under the comment "Destroy my edit history
and settings" — so leaving a document and coming back cost the user
everything they could have undone (#391).

Each tab now has its own `ITextModel`, and a switch is
`editor.setModel(...)`. Undo belongs to the document, which is Monaco's
intended usage and how VS Code works. `tabs.svelte.ts` already said so in
a comment: "Text undo belongs to Monaco's model, not to the tab."

## Lifetime

Monaco models are registered with the model service and are NOT collected
while registered, so the invariant is: the set of live models is exactly
the set of live tabs. It is held by reconciliation, not by a dispose
paired with each removal — `retainTabModels(liveTabIds)` asks "which
models have no tab?" after every removal, so a route nobody has thought
of yet is still covered. Three call sites, because there are three places
a tab stops existing: `closeTab` (the close button, Ctrl/Cmd+W,
close-others, close-to-the-right, a clean tab losing its path in
`claimPath`, a tab moved to another window, a rolled-back transfer),
`closeAll`, and `restoreState`, which replaces the whole array and is the
one removal that never calls `closeTab`.

Disposing a model the editor has attached needs no special case:
`CodeEditorWidget._attachModel` registers
`model.onWillDispose(() => this.setModel(null))`, so the editor detaches
itself and the activation effect attaches the new tab's model in the same
flush. A window close destroys the webview, which frees everything.

The models live in `utils/tabModels.ts`, not on `Tab`: a model field on
the store would either drag Monaco back into the startup chunk (~86% of
the startup JavaScript, kept out by a dynamic import and locked by
`monacoStartupGraph.test.ts`) or be typed `any`. That module imports
Monaco for its TYPES only, which is erased.

## Close and reopen: the undo history is gone, and says so

Monaco cannot preserve an undo stack across `dispose()`. Closing a tab
disposes its model, so reopening the document gives a new tab, a new
model and an empty history — same as VS Code, and the same reason. A
cross-window move is the same case: a model is a JavaScript object in one
webview and cannot travel, so the arriving tab starts fresh. Both are
pinned by tests rather than left to be discovered.

## Assumptions that had to change

- Keyed by TAB ID, not by path. A path is not stable for the life of a
  tab (Save As, rename, following a link, back/forward), a model's URI
  cannot change after creation, untitled tabs have no path, and
  `createModel` throws on a duplicate URI. A UUID collides with nothing,
  including the two-tabs-one-path window that #413/#416 are about.
- The language was applied once, at `create`. Now that the model outlives
  every route that repoints a tab, it is re-applied on acquire — which
  also fixes a tab that follows a link from `.md` into `.ts` keeping the
  old language.
- The word count and the language reading arrived for free, because
  `setValue` fires a content change as a side effect. `setModel` does
  not — no content changed, a different document arrived — so the switch
  asks for them explicitly (`syncStatusFromModel`, which also replaces
  two hand-rolled copies of the word count).
- `editor.dispose()` no longer takes the model with it. That is the
  point: `StandaloneEditor` only disposes a model it built itself, and
  this component is unmounted every time a tab goes to reading mode.
- The view state is still saved and restored by hand. It is the editor's,
  not the model's, and it has to be applied after the model it describes
  is attached.

## What `setValue` is still for

External writes — a reload from disk, an accepted external change, a
truncated buffer completed, a task checkbox toggled from the preview, a
link followed inside the tab — hand the tab a DIFFERENT document, and an
undo stack from the old one would splice two texts together. Those still
go through `setValue` and still clear undo, unchanged from today; the
`getValue()` comparison is what tells them from an ordinary switch.
Making them undoable with `pushEditOperation` is what #391 suggests and
is deliberately NOT here: it would let Ctrl+Z resurrect a buffer that the
truncation and lossy-decode guards (#374, #379) exist to keep away from
the file, which is a second behaviour change and deserves its own.

## Verification

`npm run check` 0 errors, `npm test` 700/700, `cargo test` 157/157,
`npm audit` 0 vulnerabilities, `npm run build` clean and Monaco still in
its own chunk.

Falsification: with `scripts/undoHistoryPerTab.test.ts` kept and the two
source files reverted to master, 11 of its 17 assertions go red —
including "undo did nothing after a round trip through another tab" and
"models built during this test: 0 !== 2" on every closing path. The six
that stay green are deliberate locks on behaviour this change preserves
(an external write still clears undo, a navigate clears it, a rename does
not, the word count, the view state, and a negative control that undo
does not reach past the opened state).

Co-authored-by: Claude Opus 5 <noreply@anthropic.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