Compare document identity when deciding if the preview needs updating - #333712
Compare document identity when deciding if the preview needs updating#333712Jaeke Barkin (jrbarkin) wants to merge 2 commits into
Conversation
Part of microsoft#331900 A markdown preview opened from the explorer has no editor holding the document open, so the document is kept alive only by the lease taken in #updatePreview, which lapses after a few minutes. Once it lapses, the next update re-opens the file as a new text document whose version numbering restarts at 1. PreviewDocumentVersion compares only the resource and the version number, so that re-opened document can compare equal to the version last rendered even though the file on disk has changed, and #updatePreview returns early without rendering. The preview then stays stale for that write and every write after it. Compare document identity instead, so a version recorded from a different document object can never suppress an update. Identity implies the uris match, so they are no longer compared separately. The same restarted version number also defeats TokenCache in markdownEngine.ts, which keys on uri and version, so both layers need fixing before a preview recovers. That second layer is a separate bug with its own reproduction that does not involve the preview at all, and is already open as microsoft#332081; this change does not duplicate it. This commit is therefore "part of" rather than "fixes", so the issue is not closed while half the fix is still outstanding.
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Pull request overview
Updates Markdown preview version tracking to distinguish reopened document instances and avoid suppressing required refreshes.
Changes:
- Compares document identity alongside version.
- Adds regression tests for identity and version behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
preview.ts |
Tracks document identity during preview updates. |
previewDocumentVersion.test.ts |
Tests version and identity comparisons. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| public constructor(document: vscode.TextDocument) { | ||
| this.resource = document.uri; | ||
| this.#document = document; |
There was a problem hiding this comment.
Good catch — verified and fixed in 8e0623a.
I checked this against the source rather than assuming: MirrorTextModel.dispose() does clear the
text (this._lines.length = 0), but ExtHostDocumentData deliberately overrides dispose() and
does not call super — "we don't really dispose documents but let extensions still read from them" —
so _lines survives. Since every getter on the facade closes over that object, holding the facade
did keep a closed document's full text alive for the life of the preview, which defeats exactly the
reclamation the document lease exists to perform.
Switched to a WeakMap<vscode.TextDocument, number> handing out identity tokens. The map doesn't
retain its keys and the stored token is a primitive, so identity stays comparable without holding
the document.
Re-verified after the change: typecheck and eslint clean, 96 passing / 0 failing (95 passing /
1 failing against main, so the regression test still discriminates), and the end-to-end check on
a build from source still shows the preview updating after the lease lapses.
Documents are not really disposed when they close: ExtHostDocumentData overrides dispose so extensions can keep reading from them, which means it does not clear the underlying lines the way MirrorTextModel.dispose does. Every getter on the TextDocument facade closes over that data, so holding the facade kept a closed document's full text alive for as long as the preview lived, defeating the reclamation the document lease exists to perform. Hand out identity tokens from a WeakMap keyed on the document instead. The map does not retain its keys and the token is a primitive, so identity stays comparable without holding on to the document.
Part of #331900 — deliberately not
Fixes, because this is one of two changes needed and theissue shouldn't auto-close when this merges. See "Needs #332081 as well" below.
#331900 isn't labelled yet, so per the contributing guide I'm happy to close this if an external
contribution isn't wanted here — #332081 is already open against the same issue, so I've assumed
it is. Glad to move the discussion to the issue first if that's preferred.
Same underlying bug as #265277, #271233 and #316067. I've referenced #331900 rather than #265277
because #265277 carries
team-low-hanging("No external contributions will be accepted") — happyto re-target if a maintainer prefers.
The bug
A markdown preview opened from the explorer has no editor holding the document open. The document
stays alive only through the lease taken by
#updatePreview'sopenTextDocumentcall(
BoundModelReferenceCollection,_maxAgeof a few minutes), which is never renewed. Observed inan instrumented extension host:
After that, the next update re-opens the file as a new text document, whose version numbering
restarts at 1.
PreviewDocumentVersioncompares only resource and version:So the re-opened document compares equal to the version last rendered even though the file changed
on disk, and
#updatePreviewreturns early at its!forceUpdate && this.#currentVersion?.equals(pendingVersion)guard without rendering. The preview stays stale for that write and every write after it.
This is why the reports cluster around code generators, formatters and CLI agents: they write to a
file nobody has open in an editor, then keep writing to it.
Reproduction
mkdir /tmp/mdbug && printf '# V0\n' > /tmp/mdbug/test.mdtest.md→ Open Preview.Do not open the file in an editor.
printf '# CHANGED\n' > /tmp/mdbug/test.mdV0, and stays that way.The wait in step 3 is why this is awkward to reproduce interactively: before the lease lapses the
document is still resident and
onDidChangeTextDocumentdrives the refresh, so everything works.The change
Compare document identity in
PreviewDocumentVersion, so a version recorded from a differentdocument object can never suppress an update. Identity implies the uris match —
TextDocument.uriis set once in
MirrorTextModel's constructor and the facade is memoized and frozen — so the uricomparison is dropped rather than kept alongside.
#currentVersionnow retains a reference to a possibly-closedTextDocument. It is bounded to oneper live preview, replaced on every update, and only ever compared by identity — never dereferenced.
Needs #332081 as well
These are two independent bugs on the same root pattern, and both must be fixed before a preview
recovers.
TokenCacheinmarkdownEngine.tskeys on uri and version, so it hits the same restartedversion number and serves the previously cached tokens — meaning even a successful re-render draws
the old content. That is #332081, which I have not duplicated here.
Verified by building each combination and measuring the rendered preview DOM after the lease lapsed.
Each fix's necessity comes from the row where the other fix is already applied and the bug survives:
preview.tsmarkdownEngine.tsScope of that result: measured on macOS (darwin 25.5.0) against
mainat 717304b, using thereproduction below. I haven't tested Windows or Linux, and I can't rule out some other path where
one change alone is enough.
Approaches that don't work
Measured, in case it saves anyone time:
onDidCreatedoes nothing. I assumed atomic writers(temp file +
rename) would surface only as a create event and that theonDidChange-onlysubscription was the bug. Logging watcher events for four write styles — in-place, same-directory
rename, cross-directory rename, unlink-then-create — every one emits
onDidChangeas well asonDidCreate. The existing subscription already receives the event. Built it, measured it, noimprovement.
refresh(true)) from the watcher does not work either. The token cachedefeats it one layer down, and it costs a full
webview.htmlreload per write instead of thecheap
postMessagepath. Separately,refresh()drops the call when#throttleTimeris alreadyarmed, so the force is racy anyway.
This also answers the question asked when closing #297649 ("Have you explored why the existing
events aren't working?"): the events fire correctly. The update is discarded afterwards — first by
this version comparison, then by the token cache.
Tests
Adds
PreviewDocumentVersiontests. The identity case fails onmainand passes with this change:Run with
npx vscode-test --label markdown-language-features.