Skip to content

fix(tabs): one tab per file path - #413

Merged
PathGao merged 1 commit into
fix/reopen-dirty-documentfrom
fix/one-tab-per-path
Aug 3, 2026
Merged

fix(tabs): one tab per file path#413
PathGao merged 1 commit into
fix/reopen-dirty-documentfrom
fix/one-tab-per-path

Conversation

@PathGao

@PathGao PathGao commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

2 / 2. Base: #412 — and it must land first. See "Why this is safe now" below.

The defect

navigate, updateTabPath, renameTab, goBack, goForward and insertTransferredTab all assigned tab.path without checking whether another tab already held it, and openMarkdownTargetInNewTab called addTab unconditionally. So:

  • right-click a link to a file that is already open → Open in new tab, or
  • Save As from an untitled tab onto a file that is already open

…gave you two tabs on one path. Each then carried its own isDirty and its own auto-save timer, and they wrote over each other.

loadMarkdown already de-duplicated, but only on the ordinary open path.

What the invariant should be

Neither VS Code nor Sublime forbids two tabs. Both forbid two buffers.

  • VS Code keys one ITextModel per URI; editors are views onto it, so edits appear in all of them and there is exactly one save path. Two divergent buffers for one URI is not a representable state.
  • Sublime Text: opening an already-open file links you to the existing view. Getting a second tab requires clone_file, which explicitly creates a second view of the same buffer — never a second buffer.

Markpad has no buffer/view split — a tab is the buffer — so the faithful translation of that invariant is one tab per path. Only real file paths are exclusive; several untitled tabs remain normal.

Conflict resolution, by what the loser stands to lose

loser outcome
clean closed — its buffer is a copy of the file the winner now holds, so nothing is lost, and it lands on the reopen-closed-tab stack like any other close
dirty kept, and releases the path — becomes untitled, buffer and title intact. Nothing is discarded, nothing can auto-save over the file behind the user's back, and saving it asks where to put it, which is the one decision only the user can make

Why not "refuse the claim and activate the existing tab"

That is the naive reading of what VS Code does, and here it would be worse than the bug. loadMarkdown captures activeId before calling navigate, then writes the newly read content into that captured tab. Declining the path change leaves the tab pointing at file A while receiving file B's text — and the next save writes B over A.

Why this is safe now, and was not before

De-duplicating means "open link in new tab" resolves to an already-open tab and hands it to a loader that previously would have written disk content into a brand-new one. If that tab has unsaved edits, the de-duplication is what makes them reachable.

#412 closes that. The receipt is a single test, deliberately narrowed to one assertion so its verdict is unambiguous:

state following a link into a new tab never costs the target tab its unsaved edits
master greenaddTab makes a second tab, so nothing is overwritten
this PR without #412 red ← the trade
#412 alone green
both green

I ran that third state explicitly rather than reasoning about it.

Tests

scripts/tabPathIdentity.test.ts (10) plus one assertion added to reopenDirtyDocument.test.ts.

vs #412 8 red / 14 green

By file: tabPathIdentity 7 red / 3 green — the three greens are the don't-over-fire boundaries (untitled tabs do not collide, the HOME sentinel is a singleton on its own terms, and "re-opening doesn't overwrite edits", which passes on #412 precisely because #412 landed). reopenDirtyDocument 1 red / 11 green, the red being the new tab-identity assertion.

npm run check   435 files, 0 errors
npm test        500 / 500
cargo test      131 / 131

Not covered

🤖 Generated with Claude Code

`navigate`, `updateTabPath`, `renameTab`, `goBack`, `goForward` and
`insertTransferredTab` all assigned `tab.path` without checking whether
another tab already held it, and `openMarkdownTargetInNewTab` called
`addTab` unconditionally - so "open link in new tab" on an already-open
file, or "Save As" onto one, produced two tabs on one path. Each then
carried its own dirty flag and its own auto-save timer, and they wrote
over each other.

Neither VS Code nor Sublime forbids two tabs; both forbid two buffers.
VS Code keys one `ITextModel` per URI and lets editors be views onto it;
Sublime's `clone_file` explicitly makes a second view of the same
buffer. Markpad has no buffer/view split - a tab *is* the buffer - so the
faithful translation of that invariant is one tab per path, which is
also what `loadMarkdown` already did on the ordinary open path.

Conflicts are resolved by what the loser stands to lose. A clean loser is
closed: its buffer is a copy of the file the winner now holds, so nothing
is lost, and it lands on the reopen-closed-tab stack like any other
close. A dirty loser is kept and releases the path instead, becoming
untitled with its buffer and title intact - nothing is discarded, nothing
can auto-save over the file behind the user's back, and saving it asks
where to put it, which is the one decision only the user can make.

Refusing the claim and activating the existing tab - the naive reading of
what VS Code does - would be worse than the bug: `loadMarkdown` captures
`activeId` before calling `navigate` and writes the newly read content
into that captured tab, so declining the path change hands one document's
text to a tab pointing at another file.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@PathGao
PathGao merged commit 206c8cd into master Aug 3, 2026
4 checks passed
@PathGao
PathGao deleted the fix/one-tab-per-path branch August 3, 2026 04:40
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