Fix multiconsole hyperlink support by tracking active sessions in MainThreadConsoleService

extensions/positron-r/src/hyperlink.ts

@@ -48,7 +48,7 @@ function handleNotRunnable(code: string) {
 }
 
 async function handleManuallyRunnable(_runtime: RSession, code: string) {
-	const console = await positron.window.getConsoleForLanguage('r');
+	const console = await positron.window.getActiveConsoleForLanguage('r');
 
 	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 active `Console` for a runtime `languageId`
 		 *
 		 * @param languageId The runtime language id to retrieve a `Console` for, i.e. 'r' or 'python'.
 		 *
 		 * @returns A Thenable that resolves to a `Console` or `undefined` if no `Console` for
 		 *   that `languageId` exists.
 		 */
-		export function getConsoleForLanguage(languageId: string): Thenable<Console | undefined>;
+		export function getActiveConsoleForLanguage(languageId: 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/mainThreadConsole.ts

@@ -3,6 +3,7 @@
  *  Licensed under the Elastic License 2.0. See LICENSE.txt for license information.
  *--------------------------------------------------------------------------------------------*/
 
+import { Disposable } from 'vscode';
 import { IPositronConsoleInstance } from '../../../services/positronConsole/browser/interfaces/positronConsoleService.js';
 
 /**
@@ -13,17 +14,17 @@ import { IPositronConsoleInstance } from '../../../services/positronConsole/brow
  * When the extension host requests console behavior from the main thread, it
  * typically ends up here.
  */
-export class MainThreadConsole {
+export class MainThreadConsole implements Disposable {
 	constructor(
 		private readonly _console: IPositronConsoleInstance
 	) {
 	}
 
-	getLanguageId(): string {
-		return this._console.runtimeMetadata.languageId;
-	}
-
 	pasteText(text: string): void {
 		this._console.pasteText(text);
 	}
+
+	dispose() {
+		// Currently a no-op
+	}
 }

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

@@ -8,14 +8,15 @@ import { ExtHostConsoleServiceShape, ExtHostPositronContext, MainPositronContext
 import { extHostNamedCustomer, IExtHostContext } from '../../../services/extensions/common/extHostCustomers.js';
 import { IPositronConsoleInstance, IPositronConsoleService } from '../../../services/positronConsole/browser/interfaces/positronConsoleService.js';
 import { MainThreadConsole } from './mainThreadConsole.js';
+import { dispose } from '../../../../base/common/lifecycle.js';
 
 @extHostNamedCustomer(MainPositronContext.MainThreadConsoleService)
 export class MainThreadConsoleService implements MainThreadConsoleServiceShape {
 
 	private readonly _disposables = new DisposableStore();
 
 	/**
-	 * A Map of session ids to the respective console.
+	 * A Map from session id 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
@@ -25,6 +26,11 @@ export class MainThreadConsoleService implements MainThreadConsoleServiceShape {
 	 */
 	private readonly _mainThreadConsolesBySessionId = new Map<string, MainThreadConsole>();
 
+	/**
+	 * A Map from language id to the active console session id for that language.
+	 */
+	private readonly _activeSessionIdsByLanguage = new Map<string, string>();
+
 	private readonly _proxy: ExtHostConsoleServiceShape;
 
 	constructor(
@@ -56,24 +62,32 @@ export class MainThreadConsoleService implements MainThreadConsoleServiceShape {
 			})
 		);
 
-		// 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);
-		// 	})
-		// )
+		this._disposables.add(
+			this._positronConsoleService.onDidDeletePositronConsoleInstance((console) => {
+				const sessionId = console.sessionMetadata.sessionId;
+
+				// First update ext host
+				this._proxy.$deleteConsole(sessionId);
+
+				// Then update main thread
+				this.deleteConsole(sessionId);
+			})
+		)
+
+		this._disposables.add(
+			this._positronConsoleService.onDidChangeActivePositronConsoleInstance((console) => {
+				if (!console) {
+					// No console is active currently. Nothing for us to update.
+					// We only remove active session ids on console deletion.
+					return;
+				}
+
+				const languageId = console.runtimeMetadata.languageId;
+				const sessionId = console.sessionMetadata.sessionId;
+
+				this._activeSessionIdsByLanguage.set(languageId, sessionId);
+			})
+		)
 	}
 
 	dispose(): void {
@@ -85,13 +99,20 @@ export class MainThreadConsoleService implements MainThreadConsoleServiceShape {
 		this._mainThreadConsolesBySessionId.set(sessionId, mainThreadConsole);
 	}
 
-	// TODO:
-	// See comment in constructor
-	//
-	// private removeConsole(id: string) {
-	// 	// No dispose() method to call
-	// 	this._mainThreadConsolesByLanguageId.delete(id);
-	// }
+	private deleteConsole(sessionId: string) {
+		const mainThreadConsole = this._mainThreadConsolesBySessionId.get(sessionId);
+		this._mainThreadConsolesBySessionId.delete(sessionId);
+		dispose(mainThreadConsole);
+
+		// Removing the console implies it is no longer active for that language, so remove it from
+		// `this._activeSessionIdsByLanguage` as well.
+		for (const [entryLanguageId, entrySessionId] of this._activeSessionIdsByLanguage.entries()) {
+			if (sessionId === entrySessionId) {
+				this._activeSessionIdsByLanguage.delete(entryLanguageId);
+				break;
+			}
+		}
+	}
 
 	// --- from extension host process
 
@@ -104,24 +125,20 @@ export class MainThreadConsoleService implements MainThreadConsoleServiceShape {
 	 *
 	 * @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);
-			}
+	$getActiveSessionIdForLanguage(languageId: string): Promise<string | undefined> {
+		const sessionId = this._activeSessionIdsByLanguage.get(languageId);
+
+		if (!sessionId) {
+			return Promise.resolve(undefined);
+		}
+
+		// Double check that we know about this console on the main thread side, for added safety.
+		// If we don't, something is probably out of sync.
+		if (!this._mainThreadConsolesBySessionId.get(sessionId)) {
+			return Promise.resolve(undefined);
 		}
 
-		return Promise.resolve(undefined);
+		return Promise.resolve(sessionId);
 	}
 
 	$tryPasteText(sessionId: string, text: string): void {

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);
+			getActiveConsoleForLanguage(languageId: string) {
+				return extHostConsoleService.getActiveConsoleForLanguage(languageId);
 			},
 			get onDidChangeConsoleWidth() {
 				return extHostConsoleService.onDidChangeConsoleWidth;

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

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

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

@@ -4,6 +4,7 @@
  *--------------------------------------------------------------------------------------------*/
 
 import * as positron from 'positron';
+import { Disposable } from 'vscode';
 import { MainThreadConsoleServiceShape } from './extHost.positron.protocol.js';
 import { ILogService } from '../../../../platform/log/common/log.js';
 
@@ -19,7 +20,7 @@ import { ILogService } from '../../../../platform/log/common/log.js';
  * `positron.Console` is modeled after the design of `vscode.TextEditor`, which
  * similarly has both `ExtHostTextEditor` and `MainThreadTextEditor`.
  */
-export class ExtHostConsole {
+export class ExtHostConsole implements Disposable {
 
 	private _disposed: boolean = false;
 

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

@@ -46,18 +46,18 @@ export class ExtHostConsoleService implements extHostProtocol.ExtHostConsoleServ
 	}
 
 	/**
-	 * Queries the main thread for the console that aligns with this
+	 * Queries the main thread for the active console that aligns with this
 	 * `languageId`.
 	 *
 	 * @param languageId The language id to find a 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);
+	async getActiveConsoleForLanguage(languageId: string): Promise<positron.Console | undefined> {
+		const sessionId = await this._proxy.$getActiveSessionIdForLanguage(languageId);
 
 		if (!sessionId) {
-			// Main thread says there is no `sessionId` for this `languageId`
+			// Main thread says there is no active `sessionId` for this `languageId`
 			return undefined;
 		}
 
@@ -88,8 +88,8 @@ export class ExtHostConsoleService implements extHostProtocol.ExtHostConsoleServ
 		this._extHostConsolesBySessionId.set(sessionId, extHostConsole);
 	}
 
-	// Called when a console instance is removed
-	$removeConsole(sessionId: string): void {
+	// Called when a console instance is deleted
+	$deleteConsole(sessionId: string): void {
 		const extHostConsole = this._extHostConsolesBySessionId.get(sessionId);
 		this._extHostConsolesBySessionId.delete(sessionId);
 		// "Dispose" of an `ExtHostConsole`, ensuring that future API calls warn / error