From e490711550bbafca87d96b015b0211323cbdc6e0 Mon Sep 17 00:00:00 2001 From: Marie Idleman Date: Tue, 4 Aug 2026 14:42:22 -0500 Subject: [PATCH 1/3] fix(quarto): rebind the output cache when an untitled document is saved in web The untitled->saved transition transfers the Quarto output cache to the saved document's URI, but required a `file` scheme. In a remote or web window a saved document is `vscode-remote`, so the transfer never ran and the cache stayed keyed to the untitled URI. Output still rendered right after the save because the content-hash fallback matched the live untitled cache in memory, but a window reload found nothing under the saved URI and restored no view zones. Accept any non-untitled scheme, and cover the rebind with a vitest. --- .../browser/quartoOutputManager.ts | 7 +- .../quartoOutputManagerSaveAs.vitest.ts | 172 ++++++++++++++++++ 2 files changed, 177 insertions(+), 2 deletions(-) create mode 100644 src/vs/workbench/contrib/positronQuarto/test/browser/quartoOutputManagerSaveAs.vitest.ts diff --git a/src/vs/workbench/contrib/positronQuarto/browser/quartoOutputManager.ts b/src/vs/workbench/contrib/positronQuarto/browser/quartoOutputManager.ts index 4cd61980e4ac..7fe285fe6804 100644 --- a/src/vs/workbench/contrib/positronQuarto/browser/quartoOutputManager.ts +++ b/src/vs/workbench/contrib/positronQuarto/browser/quartoOutputManager.ts @@ -381,10 +381,13 @@ export class QuartoOutputContribution extends Disposable implements IEditorContr this._documentUri = newModel?.uri; // Handle untitled->saved transition: transfer cache from old URI to new URI - // This happens when a user saves an untitled Quarto document to a file + // This happens when a user saves an untitled Quarto document to a file. + // Any non-untitled scheme counts as saved: in a remote or web window the + // saved document is `vscode-remote`, and requiring `file` here left the + // cache keyed to the untitled URI, so outputs were lost on reload. if (previousUri && this._documentUri && previousUri.scheme === 'untitled' && - this._documentUri.scheme === 'file' && + this._documentUri.scheme !== 'untitled' && this._isQuartoDocument()) { this._transferCacheFromUntitled(previousUri, this._documentUri); } diff --git a/src/vs/workbench/contrib/positronQuarto/test/browser/quartoOutputManagerSaveAs.vitest.ts b/src/vs/workbench/contrib/positronQuarto/test/browser/quartoOutputManagerSaveAs.vitest.ts new file mode 100644 index 000000000000..b067e869d0be --- /dev/null +++ b/src/vs/workbench/contrib/positronQuarto/test/browser/quartoOutputManagerSaveAs.vitest.ts @@ -0,0 +1,172 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (C) 2026 Posit Software, PBC. All rights reserved. + * Licensed under the Elastic License 2.0. See LICENSE.txt for license information. + *--------------------------------------------------------------------------------------------*/ + +/// + +import { URI } from '../../../../../base/common/uri.js'; +import { Event, Emitter } from '../../../../../base/common/event.js'; +import { createTestContainer } from '../../../../../test/vitest/positronTestContainer.js'; +import { createTextModel } from '../../../../../editor/test/common/testTextModel.js'; +import { stubInterface } from '../../../../../test/vitest/stubInterface.js'; +import { ICodeEditor } from '../../../../../editor/browser/editorBrowser.js'; +import { ITextModel } from '../../../../../editor/common/model.js'; +import { IModelChangedEvent } from '../../../../../editor/common/editorCommon.js'; +import { IContextKeyService } from '../../../../../platform/contextkey/common/contextkey.js'; +import { QuartoOutputContribution, IQuartoOutputManager } from '../../browser/quartoOutputManager.js'; +import { IQuartoDocumentModelService } from '../../browser/quartoDocumentModelService.js'; +import { IQuartoKernelManager } from '../../browser/quartoKernelManager.js'; +import { IQuartoExecutionManager, IQuartoOutputCacheService, ICellOutput } from '../../common/quartoExecutionTypes.js'; +import { IQuartoDocumentModel, QuartoCodeCell } from '../../common/quartoTypes.js'; +import { QUARTO_INLINE_OUTPUT_ENABLED } from '../../common/positronQuartoConfig.js'; +import { IPositronNotebookOutputWebviewService } from '../../../positronOutputWebview/browser/notebookOutputWebviewService.js'; +import { IResourceUsageHistoryService } from '../../../../services/positronConsole/browser/resourceUsageHistoryService.js'; + +/** + * Regression coverage for inline output disappearing after a window reload when + * an untitled Quarto document was saved with Save As (71% on sles/chromium). + * + * On the untitled->saved transition the contribution rebinds the output cache + * from the untitled URI to the saved document's URI. That rebind used to require + * a `file` scheme, so in a remote or web window -- where a saved document is + * `vscode-remote` -- it never ran and the cache stayed keyed to the untitled URI. + * Output still rendered right after the save, because the content-hash fallback + * matched the still-live untitled cache in memory, but nothing existed under the + * saved URI to restore from once the window reloaded. + * + * These tests drive the real contribution across an `onDidChangeModel` from an + * untitled model to a saved one, and assert which URI the cache was written to. + */ +describe('QuartoOutputContribution -- cache rebind on Save As', () => { + const cellId = '0-abchash-unlabeled'; + const contentHash = 'abchash'; + const untitledUri = URI.from({ scheme: 'untitled', path: '/Untitled-1.qmd' }); + const output: ICellOutput = { outputId: 'out-1', items: [{ mime: 'text/plain', data: 'plot' }] }; + + // Describe-scope so the container's stubs capture stable references at + // build() time; reset per test (see beforeEach) for isolation. + const modelChangeEmitter = new Emitter(); + let liveCells: QuartoCodeCell[] = []; + let untitledCache = new Map(); + let currentModel: ITextModel | undefined; + + /** URIs the contribution wrote cached output to, in call order. */ + let savedToUris: string[] = []; + /** URIs whose cache the contribution cleared, in call order. */ + let clearedUris: string[] = []; + + const quartoModel = stubInterface({ + get cells() { return liveCells; }, + onDidParse: Event.None, + findCellByContentHash: (hash: string) => liveCells.find(c => c.contentHash === hash), + getCellById: (id: string) => liveCells.find(c => c.id === id), + }); + + const ctx = createTestContainer() + .withWorkbenchServices() + .withContributionServices() + .stub(IQuartoDocumentModelService, { getModel: () => quartoModel }) + .stub(IQuartoOutputCacheService, { + loadCache: async () => undefined, + findCacheByContentHash: async () => undefined, + getCachedOutputs: (uri: URI) => uri.toString() === untitledUri.toString() ? untitledCache : new Map(), + saveOutput: (uri: URI) => { savedToUris.push(uri.toString()); }, + clearCache: (uri: URI) => { clearedUris.push(uri.toString()); }, + }) + .stub(IQuartoExecutionManager, { + onDidReceiveOutput: Event.None, + onDidChangeExecutionState: Event.None, + onWillExecute: Event.None, + }) + .stub(IQuartoKernelManager, { + onDidChangeKernelState: Event.None, + getSessionForDocument: () => undefined, + }) + .stub(IQuartoOutputManager, { + onDidChangeOutputs: Event.None, + onDidRequestClearDocument: Event.None, + onDidRequestClearAll: Event.None, + }) + .stub(IPositronNotebookOutputWebviewService, {}) + .stub(IResourceUsageHistoryService, {}) + .build(); + + beforeEach(() => { + liveCells = []; + untitledCache = new Map([[cellId, [output]]]); + savedToUris = []; + clearedUris = []; + currentModel = undefined; + }); + + /** A parsed cell for the one-line code range the text models below carry. */ + function cell(): QuartoCodeCell { + return stubInterface({ + id: cellId, + contentHash, + label: undefined, + codeStartLine: 1, + codeEndLine: 1, + }); + } + + /** Swap in a text model for the given URI; returns it for the editor stub. */ + function modelFor(uri: URI): ITextModel { + currentModel = ctx.disposables.add(createTextModel('print("hi")', 'quarto', undefined, uri)); + return currentModel; + } + + /** + * Instantiate the contribution over an untitled document, then fire the + * model change that a Save As produces, landing on `savedUri`. + */ + function saveAsTo(savedUri: URI): void { + modelFor(untitledUri); + const editor = stubInterface({ + hasModel: (() => true) as ICodeEditor['hasModel'], + getModel: () => currentModel ?? null, + getOption: (() => false) as ICodeEditor['getOption'], + onDidChangeModel: modelChangeEmitter.event, + onDidScrollChange: Event.None, + }); + QUARTO_INLINE_OUTPUT_ENABLED.bindTo(ctx.get(IContextKeyService)).set(true); + ctx.disposables.add(ctx.instantiationService.createInstance(QuartoOutputContribution, editor)); + + // The save swaps the editor's model for the saved document, which the + // document model reports as parsed by the time the change is handled. + modelFor(savedUri); + liveCells = [cell()]; + modelChangeEmitter.fire({ oldModelUrl: untitledUri, newModelUrl: savedUri }); + } + + it('rebinds the cache to a vscode-remote document saved from untitled', () => { + const savedUri = URI.from({ scheme: 'vscode-remote', authority: 'localhost:9000', path: '/w/saved.qmd' }); + + saveAsTo(savedUri); + + // With the bug the rebind was skipped for any non-file scheme, so the + // cache stayed under the untitled URI and the reload had nothing to load. + expect({ savedToUris, clearedUris }).toEqual({ + savedToUris: [savedUri.toString()], + clearedUris: [untitledUri.toString()], + }); + }); + + it('rebinds the cache to a file document saved from untitled', () => { + const savedUri = URI.file('/w/saved.qmd'); + + saveAsTo(savedUri); + + expect({ savedToUris, clearedUris }).toEqual({ + savedToUris: [savedUri.toString()], + clearedUris: [untitledUri.toString()], + }); + }); + + it('does not rebind when an untitled document is swapped for another untitled one', () => { + saveAsTo(URI.from({ scheme: 'untitled', path: '/Untitled-2.qmd' })); + + expect({ savedToUris, clearedUris }).toEqual({ savedToUris: [], clearedUris: [] }); + }); +}); From 3cc342aab944eaf7ac353b8b114cfbd3b2ae58b4 Mon Sep 17 00:00:00 2001 From: Marie Idleman Date: Tue, 4 Aug 2026 15:09:46 -0500 Subject: [PATCH 2/3] test(quarto): assert the rebind's visible outcome and cover its cell-matching branches The rebind tests only checked which URIs the cache service was called with, so a change that wrote the cache but dropped the in-memory re-attach would have passed. Assert the outputs land on the saved document too, and add the two matching partitions that had no coverage: a cell whose index shifted (matched by content-hash prefix) and a cached cell that matches nothing in the saved document. Drop the redundant workbench preset, which the contribution preset already implies. --- .../quartoOutputManagerSaveAs.vitest.ts | 102 ++++++++++++++---- 1 file changed, 81 insertions(+), 21 deletions(-) diff --git a/src/vs/workbench/contrib/positronQuarto/test/browser/quartoOutputManagerSaveAs.vitest.ts b/src/vs/workbench/contrib/positronQuarto/test/browser/quartoOutputManagerSaveAs.vitest.ts index b067e869d0be..9fd8fd8df6ad 100644 --- a/src/vs/workbench/contrib/positronQuarto/test/browser/quartoOutputManagerSaveAs.vitest.ts +++ b/src/vs/workbench/contrib/positronQuarto/test/browser/quartoOutputManagerSaveAs.vitest.ts @@ -36,7 +36,8 @@ import { IResourceUsageHistoryService } from '../../../../services/positronConso * saved URI to restore from once the window reloaded. * * These tests drive the real contribution across an `onDidChangeModel` from an - * untitled model to a saved one, and assert which URI the cache was written to. + * untitled model to a saved one, and assert what the cache was written under -- + * the URI, and the cell id and hash a reload would have to look the outputs up by. */ describe('QuartoOutputContribution -- cache rebind on Save As', () => { const cellId = '0-abchash-unlabeled'; @@ -51,27 +52,32 @@ describe('QuartoOutputContribution -- cache rebind on Save As', () => { let untitledCache = new Map(); let currentModel: ITextModel | undefined; - /** URIs the contribution wrote cached output to, in call order. */ - let savedToUris: string[] = []; + /** Cache writes the contribution made, in call order. */ + let cacheWrites: { uri: string; cellId: string; contentHash: string }[] = []; /** URIs whose cache the contribution cleared, in call order. */ let clearedUris: string[] = []; const quartoModel = stubInterface({ get cells() { return liveCells; }, onDidParse: Event.None, - findCellByContentHash: (hash: string) => liveCells.find(c => c.contentHash === hash), getCellById: (id: string) => liveCells.find(c => c.id === id), }); const ctx = createTestContainer() - .withWorkbenchServices() .withContributionServices() .stub(IQuartoDocumentModelService, { getModel: () => quartoModel }) .stub(IQuartoOutputCacheService, { + // No cache exists under the saved URI until the rebind writes one. + // These two are read inside the restore pass's try/catch, so leaving + // them unstubbed would abort that pass silently rather than loudly. loadCache: async () => undefined, findCacheByContentHash: async () => undefined, getCachedOutputs: (uri: URI) => uri.toString() === untitledUri.toString() ? untitledCache : new Map(), - saveOutput: (uri: URI) => { savedToUris.push(uri.toString()); }, + // A reload restores by cell id and hash, so record those alongside the + // URI -- a write under the pre-save id is as lost as no write at all. + saveOutput: (uri: URI, cellId: string, contentHash: string) => { + cacheWrites.push({ uri: uri.toString(), cellId, contentHash }); + }, clearCache: (uri: URI) => { clearedUris.push(uri.toString()); }, }) .stub(IQuartoExecutionManager, { @@ -95,16 +101,16 @@ describe('QuartoOutputContribution -- cache rebind on Save As', () => { beforeEach(() => { liveCells = []; untitledCache = new Map([[cellId, [output]]]); - savedToUris = []; + cacheWrites = []; clearedUris = []; currentModel = undefined; }); /** A parsed cell for the one-line code range the text models below carry. */ - function cell(): QuartoCodeCell { + function cell(overrides: { id?: string; contentHash?: string } = {}): QuartoCodeCell { return stubInterface({ - id: cellId, - contentHash, + id: overrides.id ?? cellId, + contentHash: overrides.contentHash ?? contentHash, label: undefined, codeStartLine: 1, codeEndLine: 1, @@ -119,9 +125,10 @@ describe('QuartoOutputContribution -- cache rebind on Save As', () => { /** * Instantiate the contribution over an untitled document, then fire the - * model change that a Save As produces, landing on `savedUri`. + * model change that a Save As produces, landing on `savedUri`. The saved + * document parses as `savedCells`, which defaults to the cached cell. */ - function saveAsTo(savedUri: URI): void { + function saveAsTo(savedUri: URI, savedCells: QuartoCodeCell[] = [cell()]): QuartoOutputContribution { modelFor(untitledUri); const editor = stubInterface({ hasModel: (() => true) as ICodeEditor['hasModel'], @@ -131,42 +138,95 @@ describe('QuartoOutputContribution -- cache rebind on Save As', () => { onDidScrollChange: Event.None, }); QUARTO_INLINE_OUTPUT_ENABLED.bindTo(ctx.get(IContextKeyService)).set(true); - ctx.disposables.add(ctx.instantiationService.createInstance(QuartoOutputContribution, editor)); + const contribution = ctx.disposables.add(ctx.instantiationService.createInstance(QuartoOutputContribution, editor)); // The save swaps the editor's model for the saved document, which the // document model reports as parsed by the time the change is handled. modelFor(savedUri); - liveCells = [cell()]; + liveCells = savedCells; modelChangeEmitter.fire({ oldModelUrl: untitledUri, newModelUrl: savedUri }); + return contribution; } it('rebinds the cache to a vscode-remote document saved from untitled', () => { const savedUri = URI.from({ scheme: 'vscode-remote', authority: 'localhost:9000', path: '/w/saved.qmd' }); - saveAsTo(savedUri); + const contribution = saveAsTo(savedUri); // With the bug the rebind was skipped for any non-file scheme, so the // cache stayed under the untitled URI and the reload had nothing to load. - expect({ savedToUris, clearedUris }).toEqual({ - savedToUris: [savedUri.toString()], + // The in-memory outputs are what keep the output on screen across the save. + expect({ + cacheWrites, + clearedUris, + outputs: contribution.getOutputsForCell(cellId), + }).toEqual({ + cacheWrites: [{ uri: savedUri.toString(), cellId, contentHash }], clearedUris: [untitledUri.toString()], + outputs: [output], }); }); it('rebinds the cache to a file document saved from untitled', () => { const savedUri = URI.file('/w/saved.qmd'); - saveAsTo(savedUri); + const contribution = saveAsTo(savedUri); - expect({ savedToUris, clearedUris }).toEqual({ - savedToUris: [savedUri.toString()], + expect({ + cacheWrites, + clearedUris, + outputs: contribution.getOutputsForCell(cellId), + }).toEqual({ + cacheWrites: [{ uri: savedUri.toString(), cellId, contentHash }], clearedUris: [untitledUri.toString()], + outputs: [output], + }); + }); + + it('rebinds to a cell whose index shifted, matching on the content hash prefix', () => { + // The cached id encodes the cell's index (`0-abchash-unlabeled`), so an + // edit above the cell changes its id. The rebind falls back to matching + // the hash prefix, and the outputs must follow the cell's new id. + const savedUri = URI.file('/w/saved.qmd'); + const shifted = cell({ id: '1-abchash-unlabeled', contentHash: `${contentHash}9f` }); + + const contribution = saveAsTo(savedUri, [shifted]); + + expect({ + cacheWrites, + outputsUnderNewId: contribution.getOutputsForCell(shifted.id), + outputsUnderCachedId: contribution.getOutputsForCell(cellId), + }).toEqual({ + // Written under the cell's new id, not the cached one it matched by. + cacheWrites: [{ uri: savedUri.toString(), cellId: shifted.id, contentHash: shifted.contentHash }], + outputsUnderNewId: [output], + outputsUnderCachedId: [], + }); + }); + + it('writes nothing for a cached cell that matches no cell in the saved document', () => { + const savedUri = URI.file('/w/saved.qmd'); + const unrelated = cell({ id: '0-zzzhash-unlabeled', contentHash: 'zzzhash' }); + + const contribution = saveAsTo(savedUri, [unrelated]); + + // Nothing transfers, and the untitled cache is cleared regardless, so a + // cell edited during the save loses its output. Asserting the clear pins + // today's behavior rather than endorsing it. + expect({ + cacheWrites, + clearedUris, + outputs: contribution.getOutputsForCell(unrelated.id), + }).toEqual({ + cacheWrites: [], + clearedUris: [untitledUri.toString()], + outputs: [], }); }); it('does not rebind when an untitled document is swapped for another untitled one', () => { saveAsTo(URI.from({ scheme: 'untitled', path: '/Untitled-2.qmd' })); - expect({ savedToUris, clearedUris }).toEqual({ savedToUris: [], clearedUris: [] }); + expect({ cacheWrites, clearedUris }).toEqual({ cacheWrites: [], clearedUris: [] }); }); }); From 03334f398a867210fc96887c5357502c5ca7e66a Mon Sep 17 00:00:00 2001 From: Marie Idleman Date: Tue, 4 Aug 2026 18:06:20 -0500 Subject: [PATCH 3/3] test(quarto): parameterize the save-as rebind over the saved scheme The vscode-remote and file cases were identical apart from the URI. Fold them into one it.each so the scheme-independence is stated rather than implied, and trim the file header and the rebind comment to the part the code cannot show. --- .../browser/quartoOutputManager.ts | 8 ++-- .../quartoOutputManagerSaveAs.vitest.ts | 47 +++++-------------- 2 files changed, 16 insertions(+), 39 deletions(-) diff --git a/src/vs/workbench/contrib/positronQuarto/browser/quartoOutputManager.ts b/src/vs/workbench/contrib/positronQuarto/browser/quartoOutputManager.ts index 7fe285fe6804..ffba9f29bcb3 100644 --- a/src/vs/workbench/contrib/positronQuarto/browser/quartoOutputManager.ts +++ b/src/vs/workbench/contrib/positronQuarto/browser/quartoOutputManager.ts @@ -380,11 +380,9 @@ export class QuartoOutputContribution extends Disposable implements IEditorContr const newModel = this._editor.getModel(); this._documentUri = newModel?.uri; - // Handle untitled->saved transition: transfer cache from old URI to new URI - // This happens when a user saves an untitled Quarto document to a file. - // Any non-untitled scheme counts as saved: in a remote or web window the - // saved document is `vscode-remote`, and requiring `file` here left the - // cache keyed to the untitled URI, so outputs were lost on reload. + // Handle untitled->saved transition: transfer cache from old URI to new URI. + // Any non-untitled scheme counts as saved; a remote or web window saves + // to `vscode-remote`, not `file`. if (previousUri && this._documentUri && previousUri.scheme === 'untitled' && this._documentUri.scheme !== 'untitled' && diff --git a/src/vs/workbench/contrib/positronQuarto/test/browser/quartoOutputManagerSaveAs.vitest.ts b/src/vs/workbench/contrib/positronQuarto/test/browser/quartoOutputManagerSaveAs.vitest.ts index 9fd8fd8df6ad..9dc70ac6c0be 100644 --- a/src/vs/workbench/contrib/positronQuarto/test/browser/quartoOutputManagerSaveAs.vitest.ts +++ b/src/vs/workbench/contrib/positronQuarto/test/browser/quartoOutputManagerSaveAs.vitest.ts @@ -25,19 +25,12 @@ import { IResourceUsageHistoryService } from '../../../../services/positronConso /** * Regression coverage for inline output disappearing after a window reload when - * an untitled Quarto document was saved with Save As (71% on sles/chromium). + * an untitled Quarto document was saved with Save As. * - * On the untitled->saved transition the contribution rebinds the output cache - * from the untitled URI to the saved document's URI. That rebind used to require - * a `file` scheme, so in a remote or web window -- where a saved document is - * `vscode-remote` -- it never ran and the cache stayed keyed to the untitled URI. - * Output still rendered right after the save, because the content-hash fallback - * matched the still-live untitled cache in memory, but nothing existed under the - * saved URI to restore from once the window reloaded. - * - * These tests drive the real contribution across an `onDidChangeModel` from an - * untitled model to a saved one, and assert what the cache was written under -- - * the URI, and the cell id and hash a reload would have to look the outputs up by. + * On the untitled->saved transition the contribution rebinds the output cache to + * the saved document's URI. The rebind used to require a `file` scheme, so a + * remote or web save (`vscode-remote`) left the cache keyed to the untitled URI + * with nothing for the reload to restore from. */ describe('QuartoOutputContribution -- cache rebind on Save As', () => { const cellId = '0-abchash-unlabeled'; @@ -148,28 +141,14 @@ describe('QuartoOutputContribution -- cache rebind on Save As', () => { return contribution; } - it('rebinds the cache to a vscode-remote document saved from untitled', () => { - const savedUri = URI.from({ scheme: 'vscode-remote', authority: 'localhost:9000', path: '/w/saved.qmd' }); - - const contribution = saveAsTo(savedUri); - - // With the bug the rebind was skipped for any non-file scheme, so the - // cache stayed under the untitled URI and the reload had nothing to load. - // The in-memory outputs are what keep the output on screen across the save. - expect({ - cacheWrites, - clearedUris, - outputs: contribution.getOutputsForCell(cellId), - }).toEqual({ - cacheWrites: [{ uri: savedUri.toString(), cellId, contentHash }], - clearedUris: [untitledUri.toString()], - outputs: [output], - }); - }); - - it('rebinds the cache to a file document saved from untitled', () => { - const savedUri = URI.file('/w/saved.qmd'); - + // The rebind is scheme-independent: with the bug it was skipped for any + // non-file scheme, so the cache stayed under the untitled URI and the reload + // had nothing to load. The in-memory outputs keep the output on screen + // across the save either way. + it.each([ + ['vscode-remote', URI.from({ scheme: 'vscode-remote', authority: 'localhost:9000', path: '/w/saved.qmd' })], + ['file', URI.file('/w/saved.qmd')], + ])('rebinds the cache to a %s document saved from untitled', (_scheme, savedUri) => { const contribution = saveAsTo(savedUri); expect({