Fix quick-fix lifecycle: session provenance, aria suppression, and dedup simplification

- Add session-provenance stamping to the output cache so outputs produced
  this session keep Fix/Explain buttons even after a save-as (file->file
  or untitled->file) reloads them from the cache. Previous behavior
  unconditionally marked all cache-restored outputs as stale.
- Suppress screen-reader announcements for Fix/Explain button insertions
  by adding aria-live='off' to the quick-fix container (buttons mount
  asynchronously inside the output's role='log' live region).
- Simplify the duplicate-mount guard from a replace-then-dispose dance
  with a separate _quickFixContainersByKey map to a first-wins has() skip
  (the extra map is deleted).
- Resolve getCellContext at click time via a reverse view-zone lookup
  instead of capturing cellId in a closure that must be manually rebound
  at every remap site.
- Re-key the stash on untitled->file save-as so outputs restore from the
  stash (preserving live flags) instead of re-reading from the just-
  transferred cache.
- Remove the redundant in-memory seeding in _transferCacheFromUntitled
  that caused doubled outputs after a stash round trip.

Author: nstrayer

src/vs/workbench/contrib/positronQuarto/browser/quartoOutputCacheService.ts

@@ -43,6 +43,7 @@ interface IpynbCell {
 		quarto_cell_id: string;
 		quarto_content_hash: string;
 		quarto_label?: string;
+		quarto_session_id?: string;
 	};
 	source: string[];
 	outputs: IpynbOutput[];
@@ -104,6 +105,8 @@ interface CellCacheEntry {
 	label?: string;
 	source?: string;
 	outputs: ICellOutput[];
+	/** Session that last wrote this cell's outputs; see _sessionId. */
+	sessionId?: string;
 }
 
 /**
@@ -114,6 +117,15 @@ export class QuartoOutputCacheService extends Disposable implements IQuartoOutpu
 	declare readonly _serviceBrand: undefined;
 
 	private readonly _cacheDir: URI;
+
+	/**
+	 * Identifies this window session in cache entries. Cells written via
+	 * saveOutput are stamped with it (in memory and on disk), so restores can
+	 * tell outputs produced this session (e.g. re-read after a save-as) from
+	 * outputs that survive from a previous session.
+	 */
+	private readonly _sessionId = generateUuid();
+
 	private readonly _documentCaches = new Map<string, DocumentCacheEntry>();
 	private readonly _dirtyDocuments = new Set<string>();
 	private readonly _pendingWrites = new Map<string, Promise<void>>();
@@ -280,6 +292,8 @@ export class QuartoOutputCacheService extends Disposable implements IQuartoOutpu
 					label: cell.label,
 					source: cell.source,
 					outputs: [...cell.outputs],
+					// Preserve session provenance across the disk round trip.
+					sessionId: cell.fromCurrentSession ? this._sessionId : undefined,
 				});
 			}
 
@@ -345,6 +359,10 @@ export class QuartoOutputCacheService extends Disposable implements IQuartoOutpu
 			cellEntry.source = source;
 		}
 
+		// Stamp the cell with the current session so restores can tell these
+		// outputs from ones cached in a previous session.
+		cellEntry.sessionId = this._sessionId;
+
 		// Filter out the data explorer MIME type before caching. The data
 		// explorer requires a live runtime connection that won't exist when
 		// restoring from cache; text/plain is kept as the fallback.
@@ -674,6 +692,8 @@ export class QuartoOutputCacheService extends Disposable implements IQuartoOutpu
 								label: cell.label,
 								source: cell.source,
 								outputs: [...cell.outputs],
+								// Preserve session provenance across the disk round trip.
+								sessionId: cell.fromCurrentSession ? this._sessionId : undefined,
 							});
 						}
 
@@ -775,6 +795,7 @@ export class QuartoOutputCacheService extends Disposable implements IQuartoOutpu
 					quarto_cell_id: cellEntry.cellId,
 					quarto_content_hash: cellEntry.contentHash,
 					quarto_label: cellEntry.label,
+					quarto_session_id: cellEntry.sessionId,
 				},
 				source: cellEntry.source
 					? cellEntry.source.split('\n').map((line, i, arr) => i < arr.length - 1 ? line + '\n' : line)
@@ -808,6 +829,7 @@ export class QuartoOutputCacheService extends Disposable implements IQuartoOutpu
 				label: cellEntry.label,
 				source: cellEntry.source,
 				outputs: [...cellEntry.outputs],
+				fromCurrentSession: cellEntry.sessionId === this._sessionId,
 			});
 		}
 
@@ -841,6 +863,7 @@ export class QuartoOutputCacheService extends Disposable implements IQuartoOutpu
 				label: cell.metadata?.quarto_label,
 				source,
 				outputs,
+				fromCurrentSession: cell.metadata?.quarto_session_id === this._sessionId,
 			});
 		}
 

src/vs/workbench/contrib/positronQuarto/browser/quartoOutputManager.ts

@@ -296,6 +296,18 @@ export class QuartoOutputContribution extends Disposable implements IEditorContr
 				this._documentUri.scheme === 'file' &&
 				this._isQuartoDocument()) {
 				this._transferCacheFromUntitled(previousUri, this._documentUri);
+
+				// Save-as keeps the same session's outputs live: move the
+				// stash captured above to the new URI so _loadCachedOutputs
+				// restores the outputs as-is. Without this it would re-read
+				// them from the just-transferred cache and mark them
+				// restoredFromCache, wrongly hiding Fix/Explain on errors the
+				// user produced this session.
+				const stash = this._stashedOutputsByUri.get(previousUri.toString());
+				if (stash) {
+					this._stashedOutputsByUri.delete(previousUri.toString());
+					this._stashedOutputsByUri.set(this._documentUri.toString(), stash);
+				}
 			}
 
 			// Reset initialization flags so we can re-initialize for the new document
@@ -729,16 +741,15 @@ export class QuartoOutputContribution extends Disposable implements IEditorContr
 				);
 				const source = model.getValueInRange(codeRange);
 
-				// Save outputs to cache under the new file URI
+				// Save outputs to cache under the new file URI. The in-memory
+				// output state is restored separately by _loadCachedOutputs:
+				// from the re-keyed stash when the outputs were live in this
+				// session, or from this transferred cache otherwise. Seeding
+				// _outputsByCell here as well would duplicate every output.
 				for (const output of outputs) {
 					this._cacheService.saveOutput(toUri, cell.id, cell.contentHash, cell.label, output, source);
 				}
 
-				// Also restore the in-memory output state so view zones can be created
-				// This is needed because onDidChangeModel clears _outputsByCell before calling this method
-				this._outputsByCell.set(cell.id, [...outputs]);
-				this._contentHashByCellId.set(cell.id, cell.contentHash);
-
 				transferredCount++;
 				this._logService.debug('[QuartoOutputContribution] Transferred outputs for cell', cellId, '-> new cell', cell.id);
 			} else {
@@ -845,11 +856,13 @@ export class QuartoOutputContribution extends Disposable implements IEditorContr
 
 				// Restore outputs for this cell
 				for (const cachedOutput of cachedCell.outputs) {
-					// Mark the output as cache-restored so stale-sensitive
-					// actions (e.g. Fix/Explain on errors) are suppressed.
-					// The marked copy is what we store, so the flag survives
-					// tab-switch stashes too.
-					const output: ICellOutput = { ...cachedOutput, restoredFromCache: true };
+					// Mark outputs that survive from a previous session so
+					// stale-sensitive actions (e.g. Fix/Explain on errors) are
+					// suppressed; outputs the cache saw written this session
+					// (e.g. re-read after a save-as) stay fresh. The marked
+					// copy is what we store, so the flag survives tab-switch
+					// stashes too.
+					const output: ICellOutput = { ...cachedOutput, restoredFromCache: !cachedCell.fromCurrentSession };
 
 					// Store in memory
 					const outputs = this._outputsByCell.get(cell.id) ?? [];
@@ -1047,8 +1060,13 @@ export class QuartoOutputContribution extends Disposable implements IEditorContr
 			}
 		};
 
-		// Resolve the failing code chunk for the assistant quick-fix buttons
-		viewZone.getCellContext = () => this._resolveCellContext(cellId);
+		// Resolve the failing code chunk for the assistant quick-fix buttons.
+		// The view zone's cell ID is looked up at call time so the closure
+		// stays valid when _updateViewZonePositionsImmediate remaps IDs.
+		viewZone.getCellContext = () => {
+			const currentCellId = this._findCellIdForViewZone(viewZone);
+			return currentCellId !== undefined ? this._resolveCellContext(currentCellId) : undefined;
+		};
 
 		// Set up copy handler
 		this._outputHandlingDisposables.add(viewZone.onCopyRequested(request => {
@@ -1093,6 +1111,20 @@ export class QuartoOutputContribution extends Disposable implements IEditorContr
 		return viewZone;
 	}
 
+	/**
+	 * Reverse-lookup a view zone's current cell ID. Cell IDs embed the cell's
+	 * index, so _updateViewZonePositionsImmediate re-keys view zones when
+	 * cells move; callbacks that need the ID must resolve it at call time.
+	 */
+	private _findCellIdForViewZone(viewZone: QuartoOutputViewZone): string | undefined {
+		for (const [cellId, zone] of this._viewZones) {
+			if (zone === viewZone) {
+				return cellId;
+			}
+		}
+		return undefined;
+	}
+
 	/**
 	 * Resolve the code chunk behind a cell's output so the assistant
 	 * quick-fix buttons can attach the failing code and its location.
@@ -1653,10 +1685,6 @@ export class QuartoOutputContribution extends Disposable implements IEditorContr
 			// Add with new ID
 			if (viewZone) {
 				this._viewZones.set(newId, viewZone);
-				// Rebind the quick-fix context resolver: the closure set in
-				// _createViewZone captured the old cell ID, whose bookkeeping
-				// entries are deleted just above.
-				viewZone.getCellContext = () => this._resolveCellContext(newId);
 			}
 			if (outputs) {
 				this._outputsByCell.set(newId, outputs);

src/vs/workbench/contrib/positronQuarto/browser/quartoOutputViewZone.ts

@@ -197,10 +197,6 @@ export class QuartoOutputViewZone extends Disposable implements IViewZone {
 	// React renderers for inline data explorer outputs
 	private readonly _reactRenderersByOutputId = new Map<string, PositronReactRenderer>();
 
-	// Quick-fix mount points by renderer key, so a replaced mount's container
-	// can be removed from the DOM along with its disposed renderer.
-	private readonly _quickFixContainersByKey = new Map<string, HTMLElement>();
-
 	// Configuration service for reading settings
 	private readonly _configurationService: IConfigurationService | undefined;
 
@@ -1119,9 +1115,6 @@ export class QuartoOutputViewZone extends Disposable implements IViewZone {
 			renderer.dispose();
 		}
 		this._reactRenderersByOutputId.clear();
-		// The container elements are removed with the broader DOM teardown;
-		// this just drops the stale references.
-		this._quickFixContainersByKey.clear();
 	}
 
 	/**
@@ -2532,26 +2525,30 @@ export class QuartoOutputViewZone extends Disposable implements IViewZone {
 		// availability and renders nothing when the assistant is unavailable.
 		// The renderer is registered so _disposeAllReactRenderers tears it down
 		// when outputs are cleared or the view zone is disposed.
-		const quickFixContainer = document.createElement('div');
-		quickFixContainer.className = 'quarto-output-error-quick-fix';
-		container.appendChild(quickFixContainer);
-
-		const renderer = new PositronReactRenderer(quickFixContainer);
 		// _renderError runs once per error item, so an output with multiple
-		// error items would reuse this key; dispose any renderer it replaces
-		// and remove the replaced mount's now-empty container.
+		// error items would reuse this key; only the first item mounts the
+		// buttons.
 		const rendererKey = `${output.outputId}::quick-fix`;
-		this._reactRenderersByOutputId.get(rendererKey)?.dispose();
-		this._quickFixContainersByKey.get(rendererKey)?.remove();
-		this._quickFixContainersByKey.set(rendererKey, quickFixContainer);
-		this._reactRenderersByOutputId.set(rendererKey, renderer);
-		renderer.render(
-			React.createElement(QuartoOutputQuickFix, {
-				errorContent: errorText,
-				getCellContext: () => this._getCellContext?.(),
-				restoredFromCache: output.restoredFromCache,
-			})
-		);
+		if (!this._reactRenderersByOutputId.has(rendererKey)) {
+			const quickFixContainer = document.createElement('div');
+			quickFixContainer.className = 'quarto-output-error-quick-fix';
+			// The buttons mount asynchronously (React commit, assistant
+			// availability flipping) inside the output element's role='log'
+			// live region; suppress announcements for this subtree so their
+			// insertion isn't read out after the error alert.
+			quickFixContainer.setAttribute('aria-live', 'off');
+			container.appendChild(quickFixContainer);
+
+			const renderer = new PositronReactRenderer(quickFixContainer);
+			this._reactRenderersByOutputId.set(rendererKey, renderer);
+			renderer.render(
+				React.createElement(QuartoOutputQuickFix, {
+					errorContent: errorText,
+					getCellContext: () => this._getCellContext?.(),
+					restoredFromCache: output.restoredFromCache,
+				})
+			);
+		}
 
 		return container;
 	}

src/vs/workbench/contrib/positronQuarto/common/quartoExecutionTypes.ts

@@ -298,6 +298,13 @@ export interface ICachedCellOutput {
 	readonly source?: string;
 	/** Stored outputs */
 	readonly outputs: ICellOutput[];
+	/**
+	 * Whether this cell's outputs were last written to the cache during the
+	 * current window session. False for outputs restored from a previous
+	 * session, whose stale-sensitive actions (e.g. Fix/Explain on errors)
+	 * should be suppressed.
+	 */
+	readonly fromCurrentSession: boolean;
 }
 
 /**