Fix multiconsole hyperlink support by tracking active sessions in MainThreadConsoleService

extensions/positron-r/src/hyperlink.ts

@@ -47,8 +47,8 @@ function handleNotRunnable(code: string) {
 	));
 }
 
-async function handleManuallyRunnable(_runtime: RSession, code: string) {
-	const console = await positron.window.getConsoleForLanguage('r');
+async function handleManuallyRunnable(runtime: RSession, code: string) {
+	const console = await positron.window.getConsoleForSessionId(runtime.metadata.sessionId);
 
 	if (!console) {
 		// Not an expected path, but technically possible,

src/positron-dts/positron.d.ts

@@ -1529,14 +1529,14 @@ declare module 'positron' {
 			okButtonTitle?: string): Thenable<null>;
 
 		/**
-		 * Get the `Console` for a runtime `languageId`
+		 * Get the `positron.Console` that is tied to this `sessionId`
 		 *
-		 * @param languageId The runtime language id to retrieve a `Console` for, i.e. 'r' or 'python'.
+		 * @param sessionId The session id to retrieve a `positron.Console` for.
 		 *
 		 * @returns A Thenable that resolves to a `Console` or `undefined` if no `Console` for
-		 *   that `languageId` exists.
+		 *   that `sessionId` exists.
 		 */
-		export function getConsoleForLanguage(languageId: string): Thenable<Console | undefined>;
+		export function getConsoleForSessionId(sessionId: string): Thenable<Console | undefined>;
 
 		/**
 		 * Fires when the width of the console input changes. The new width is passed as

src/vs/workbench/api/browser/positron/mainThreadConsoleService.ts

@@ -7,24 +7,12 @@ import { DisposableStore } from '../../../../base/common/lifecycle.js';
 import { ExtHostConsoleServiceShape, ExtHostPositronContext, MainPositronContext, MainThreadConsoleServiceShape } from '../../common/positron/extHost.positron.protocol.js';
 import { extHostNamedCustomer, IExtHostContext } from '../../../services/extensions/common/extHostCustomers.js';
 import { IPositronConsoleInstance, IPositronConsoleService } from '../../../services/positronConsole/browser/interfaces/positronConsoleService.js';
-import { MainThreadConsole } from './mainThreadConsole.js';
 
 @extHostNamedCustomer(MainPositronContext.MainThreadConsoleService)
 export class MainThreadConsoleService implements MainThreadConsoleServiceShape {
 
 	private readonly _disposables = new DisposableStore();
 
-	/**
-	 * A Map of session ids to the respective console.
-	 * Each session id maps to a single console.
-	 * Multiple sessions could map to the same console, this happens
-	 * when a user power-cycles the session for a console instance
-	 * (i.e. shutdown session for console instance, then start a session for console instance)
-	 *
-	 * Kept in sync with consoles in `ExtHostConsoleService`
-	 */
-	private readonly _mainThreadConsolesBySessionId = new Map<string, MainThreadConsole>();
-
 	private readonly _proxy: ExtHostConsoleServiceShape;
 
 	constructor(
@@ -42,95 +30,42 @@ export class MainThreadConsoleService implements MainThreadConsoleServiceShape {
 				this._proxy.$onDidChangeConsoleWidth(newWidth);
 			}));
 
-		// Forward new positron console session id to the extension host, and then register it
-		// in the main thread too
+		// Forward new positron console session id to the extension host
 		this._disposables.add(
 			this._positronConsoleService.onDidStartPositronConsoleInstance((console) => {
-				const sessionId = console.sessionMetadata.sessionId;
-
-				// First update ext host
-				this._proxy.$addConsole(sessionId);
-
-				// Then update main thread
-				this.addConsole(sessionId, console);
+				this._proxy.$onDidStartPositronConsoleInstance(console.sessionMetadata.sessionId);
 			})
 		);
 
-		// TODO:
-		// As of right now, we never delete console instances from the maps in
-		// `MainThreadConsoleService` and `ExtHostConsoleService` because we don't have a hook to
-		// know when a console is stopped. In particular, we should really call the `ExtHostConsole`
-		// `dispose()` method, which will ensure that any API callers who use the corresponding
-		// `Console` object will get a warning / error when calling the API of a closed console.
-		//
-		// this._disposables.add(
-		// 	this._positronConsoleService.onDidRemovePositronConsoleInstance((console) => {
-		// 		const sessionId = console.session.sessionId;
-		//
-		// 		// First update ext host
-		// 		this._proxy.$removeConsole(sessionId);
-		//
-		// 		// Then update main thread
-		// 		this.removeConsole(sessionId);
-		// 	})
-		// )
+		// Forward deleted positron console session id to the extension host
+		this._disposables.add(
+			this._positronConsoleService.onDidDeletePositronConsoleInstance((console) => {
+				this._proxy.$onDidDeletePositronConsoleInstance(console.sessionMetadata.sessionId);
+			})
+		)
 	}
 
 	dispose(): void {
 		this._disposables.dispose();
 	}
 
-	private addConsole(sessionId: string, console: IPositronConsoleInstance) {
-		const mainThreadConsole = new MainThreadConsole(console);
-		this._mainThreadConsolesBySessionId.set(sessionId, mainThreadConsole);
+	private getConsoleForSessionId(sessionId: string): IPositronConsoleInstance | undefined {
+		return this._positronConsoleService.positronConsoleInstances.find((console) => console.sessionMetadata.sessionId === sessionId);
 	}
 
-	// TODO:
-	// See comment in constructor
-	//
-	// private removeConsole(id: string) {
-	// 	// No dispose() method to call
-	// 	this._mainThreadConsolesByLanguageId.delete(id);
-	// }
-
 	// --- from extension host process
 
 	$getConsoleWidth(): Promise<number> {
 		return Promise.resolve(this._positronConsoleService.getConsoleWidth());
 	}
 
-	/**
-	 * Get the session id of the active console for a particular language id
-	 *
-	 * @param languageId The language id to find a session id for.
-	 */
-	$getSessionIdForLanguage(languageId: string): Promise<string | undefined> {
-		// TODO: This is wrong in a multi-session world. It finds the
-		// first matching `languageId` in the map, but we likely want the "most
-		// recently activated and still alive" one. Reprex to prove it is wrong,
-		// which should eventually become a test:
-		// - Start R console 1
-		// - Start R console 2
-		// - Run `cli::cli_alert("{.run revdepcheck::cloud_summary()}")` in R
-		//   console 2 and click the hyperlink.
-		// - The pasted code will incorrectly end up in R console 1.
-
-		for (let [sessionId, console] of this._mainThreadConsolesBySessionId.entries()) {
-			if (console.getLanguageId() === languageId) {
-				return Promise.resolve(sessionId);
-			}
-		}
-
-		return Promise.resolve(undefined);
-	}
-
-	$tryPasteText(sessionId: string, text: string): void {
-		const mainThreadConsole = this._mainThreadConsolesBySessionId.get(sessionId);
+	$pasteText(sessionId: string, text: string): void {
+		const console = this.getConsoleForSessionId(sessionId);
 
-		if (!mainThreadConsole) {
+		if (!console) {
 			return;
 		}
 
-		mainThreadConsole.pasteText(text);
+		console.pasteText(text);
 	}
 }

src/vs/workbench/api/common/positron/extHost.positron.api.impl.ts

@@ -183,8 +183,8 @@ export function createPositronApiFactoryAndRegisterActors(accessor: ServicesAcce
 			showSimpleModalDialogMessage(title: string, message: string, okButtonTitle?: string): Thenable<null> {
 				return extHostModalDialogs.showSimpleModalDialogMessage(title, message, okButtonTitle);
 			},
-			getConsoleForLanguage(languageId: string) {
-				return extHostConsoleService.getConsoleForLanguage(languageId);
+			getConsoleForSessionId(sessionId: string) {
+				return extHostConsoleService.getConsoleForSessionId(sessionId);
 			},
 			get onDidChangeConsoleWidth() {
 				return extHostConsoleService.onDidChangeConsoleWidth;

src/vs/workbench/api/common/positron/extHost.positron.protocol.ts

@@ -108,14 +108,13 @@ export interface ExtHostContextKeyServiceShape { }
 
 export interface MainThreadConsoleServiceShape {
 	$getConsoleWidth(): Promise<number>;
-	$getSessionIdForLanguage(languageId: string): Promise<string | undefined>;
-	$tryPasteText(sessionId: string, text: string): void;
+	$pasteText(sessionId: string, text: string): void;
 }
 
 export interface ExtHostConsoleServiceShape {
 	$onDidChangeConsoleWidth(newWidth: number): void;
-	$addConsole(sessionId: string): void;
-	$removeConsole(sessionId: string): void;
+	$onDidStartPositronConsoleInstance(sessionId: string): void;
+	$onDidDeletePositronConsoleInstance(sessionId: string): void;
 }
 
 export interface MainThreadMethodsShape { }

src/vs/workbench/api/common/positron/extHostConsole.ts

@@ -4,22 +4,22 @@
  *--------------------------------------------------------------------------------------------*/
 
 import * as positron from 'positron';
+import { Disposable } from 'vscode';
 import { MainThreadConsoleServiceShape } from './extHost.positron.protocol.js';
 import { ILogService } from '../../../../platform/log/common/log.js';
 
 /**
  * The extension host's view of a console instance
  *
- * Cousin to `MainThreadConsole`
- *
  * Do not add more methods to this class directly. Instead, add them to the
  * `positron.Console` API and implement them in `Object.freeze()` below by
  * calling out to the main thread.
  *
- * `positron.Console` is modeled after the design of `vscode.TextEditor`, which
- * similarly has both `ExtHostTextEditor` and `MainThreadTextEditor`.
+ * `positron.Console` is roughly modeled after the design of
+ * `vscode.TextEditor`, which has both `ExtHostTextEditor` and
+ * `MainThreadTextEditor`. We currently only have `ExtHostConsole`.
  */
-export class ExtHostConsole {
+export class ExtHostConsole implements Disposable {
 
 	private _disposed: boolean = false;
 
@@ -41,7 +41,7 @@ export class ExtHostConsole {
 					logService.warn('Console is closed/disposed.');
 					return;
 				}
-				proxy.$tryPasteText(sessionId, text);
+				proxy.$pasteText(sessionId, text);
 			}
 		});
 	}

src/vs/workbench/api/common/positron/extHostConsoleService.ts

@@ -18,9 +18,16 @@ export class ExtHostConsoleService implements extHostProtocol.ExtHostConsoleServ
 	 * Multiple sessions could map to the same console, this happens
 	 * when a user power-cycles the session for a console instance
 	 * (i.e. shutdown session for console instance, then start a session for console instance)
-	 *
-	 * Kept in sync with consoles in `MainThreadConsoleService`
 	 */
+	// TODO!: This doesn't survive a browser reload, and we don't really have a good way
+	// to revive it. We want to make sure of two things:
+	// - If a console is deleted, we update any existing handles to a `positron.Console` to
+	//   ensure that it looks disposed and warns on any API usage
+	// - If the window is reloaded, we need to be able to still look up the console for
+	//   a valid sessionId. The positron console service seems to maintain an up to date
+	//   set of consoles, and is probably our source of truth.
+	// Is it even possible to hand out a `positron.Console` that survives a browser reload?
+	// Or should this be more of an `executeCode()`-like API that works off a `sessionId`?
 	private readonly _extHostConsolesBySessionId = new Map<string, ExtHostConsole>();
 
 	private readonly _onDidChangeConsoleWidth = new Emitter<number>();
@@ -46,28 +53,17 @@ export class ExtHostConsoleService implements extHostProtocol.ExtHostConsoleServ
 	}
 
 	/**
-	 * Queries the main thread for the console that aligns with this
-	 * `languageId`.
+	 * Get the `positron.Console` that is tied to this `sessionId`
 	 *
-	 * @param languageId The language id to find a console for.
+	 * @param sessionId The session id to retrieve a `positron.Console` for.
 	 * @returns A promise that resolves to a `positron.Console` or `undefined`
 	 * if no console can be found.
 	 */
-	async getConsoleForLanguage(languageId: string): Promise<positron.Console | undefined> {
-		const sessionId = await this._proxy.$getSessionIdForLanguage(languageId);
-
-		if (!sessionId) {
-			// Main thread says there is no `sessionId` for this `languageId`
-			return undefined;
-		}
-
-		// Now find the console on the extension host side
+	async getConsoleForSessionId(sessionId: string): Promise<positron.Console | undefined> {
 		const extHostConsole = this._extHostConsolesBySessionId.get(sessionId);
 
 		if (!extHostConsole) {
 			// Extension host says there is no console for this `sessionId`
-			// (Should be extremely rare, if not impossible, for main thread and extension host to
-			// be out of sync here)
 			return undefined;
 		}
 
@@ -83,13 +79,13 @@ export class ExtHostConsoleService implements extHostProtocol.ExtHostConsoleServ
 	}
 
 	// Called when a new console instance is started
-	$addConsole(sessionId: string): void {
+	$onDidStartPositronConsoleInstance(sessionId: string): void {
 		const extHostConsole = new ExtHostConsole(sessionId, this._proxy, this._logService);
 		this._extHostConsolesBySessionId.set(sessionId, extHostConsole);
 	}
 
-	// Called when a console instance is removed
-	$removeConsole(sessionId: string): void {
+	// Called when a console instance is deleted
+	$onDidDeletePositronConsoleInstance(sessionId: string): void {
 		const extHostConsole = this._extHostConsolesBySessionId.get(sessionId);
 		this._extHostConsolesBySessionId.delete(sessionId);
 		// "Dispose" of an `ExtHostConsole`, ensuring that future API calls warn / error