pr feedback

Artmann · Artmann · commit 2677f00348ed · 2025-10-17T13:52:15.000+02:00
diff --git a/src/kernels/deepnote/deepnoteServerStarter.node.ts b/src/kernels/deepnote/deepnoteServerStarter.node.ts
@@ -117,12 +117,9 @@ export class DeepnoteServerStarter implements IDeepnoteServerStarter, IExtension
 
         Cancellation.throwIfCanceled(token);
 
-        // Ensure toolkit is installed
+        // Ensure toolkit is installed (will throw typed errors on failure)
         logger.info(`Ensuring deepnote-toolkit is installed for ${fileKey}...`);
-        const installed = await this.toolkitInstaller.ensureInstalled(interpreter, deepnoteFileUri, token);
-        if (!installed) {
-            throw new Error('Failed to install deepnote-toolkit. Please check the output for details.');
-        }
+        await this.toolkitInstaller.ensureInstalled(interpreter, deepnoteFileUri, token);
 
         Cancellation.throwIfCanceled(token);
 
@@ -227,22 +224,27 @@ export class DeepnoteServerStarter implements IDeepnoteServerStarter, IExtension
                 throw new DeepnoteServerTimeoutError(serverInfo.url, 120000, output?.stderr || undefined);
             }
         } catch (error) {
-            // Clean up leaked server before rethrowing
-            await this.stopServerImpl(deepnoteFileUri);
-
-            // If this is already a DeepnoteKernelError, rethrow it
+            // If this is already a DeepnoteKernelError, clean up and rethrow it
             if (error instanceof DeepnoteServerTimeoutError || error instanceof DeepnoteServerStartupError) {
+                await this.stopServerImpl(deepnoteFileUri);
                 throw error;
             }
 
-            // Otherwise wrap in a generic server startup error
+            // Capture output BEFORE cleaning up (stopServerImpl deletes it)
             const output = this.serverOutputByFile.get(fileKey);
+            const capturedStdout = output?.stdout || '';
+            const capturedStderr = output?.stderr || '';
+
+            // Clean up leaked server after capturing output
+            await this.stopServerImpl(deepnoteFileUri);
+
+            // Wrap in a generic server startup error with captured output
             throw new DeepnoteServerStartupError(
                 interpreter.uri.fsPath,
                 port,
                 'unknown',
-                output?.stdout || '',
-                output?.stderr || '',
+                capturedStdout,
+                capturedStderr,
                 error instanceof Error ? error : undefined
             );
         }
diff --git a/src/kernels/deepnote/deepnoteToolkitInstaller.node.ts b/src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
@@ -20,7 +20,7 @@ import { DeepnoteVenvCreationError, DeepnoteToolkitInstallError } from '../../pl
 export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller {
     private readonly venvPythonPaths: Map<string, Uri> = new Map();
     // Track in-flight installations per venv path to prevent concurrent installs
-    private readonly pendingInstallations: Map<string, Promise<PythonEnvironment | undefined>> = new Map();
+    private readonly pendingInstallations: Map<string, Promise<PythonEnvironment>> = new Map();
 
     constructor(
         @inject(IProcessServiceFactory) private readonly processServiceFactory: IProcessServiceFactory,
@@ -63,7 +63,7 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller {
         baseInterpreter: PythonEnvironment,
         deepnoteFileUri: Uri,
         token?: CancellationToken
-    ): Promise<PythonEnvironment | undefined> {
+    ): Promise<PythonEnvironment> {
         const venvPath = this.getVenvPath(deepnoteFileUri);
         const venvKey = venvPath.fsPath;
 
@@ -120,7 +120,7 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller {
         deepnoteFileUri: Uri,
         venvPath: Uri,
         token?: CancellationToken
-    ): Promise<PythonEnvironment | undefined> {
+    ): Promise<PythonEnvironment> {
         try {
             Cancellation.throwIfCanceled(token);
 
@@ -271,9 +271,9 @@ export class DeepnoteToolkitInstaller implements IDeepnoteToolkitInstaller {
                 throw ex;
             }
 
-            // Otherwise, log and wrap in a generic toolkit install error
+            // Otherwise, log full details and wrap in a generic toolkit install error
             logger.error(`Failed to set up deepnote-toolkit: ${ex}`);
-            this.outputChannel.appendLine(`Error setting up deepnote-toolkit: ${ex}`);
+            this.outputChannel.appendLine('Failed to set up deepnote-toolkit; see logs for details');
 
             throw new DeepnoteToolkitInstallError(
                 baseInterpreter.uri.fsPath,
diff --git a/src/kernels/deepnote/types.ts b/src/kernels/deepnote/types.ts
@@ -72,13 +72,15 @@ export interface IDeepnoteToolkitInstaller {
      * @param baseInterpreter The base Python interpreter to use for creating the venv
      * @param deepnoteFileUri The URI of the .deepnote file (used to create a unique venv per file)
      * @param token Cancellation token to cancel the operation
-     * @returns The Python interpreter from the venv if installed successfully, undefined otherwise
+     * @returns The Python interpreter from the venv
+     * @throws {DeepnoteVenvCreationError} If venv creation fails
+     * @throws {DeepnoteToolkitInstallError} If toolkit installation fails
      */
     ensureInstalled(
         baseInterpreter: PythonEnvironment,
         deepnoteFileUri: vscode.Uri,
         token?: vscode.CancellationToken
-    ): Promise<PythonEnvironment | undefined>;
+    ): Promise<PythonEnvironment>;
 
     /**
      * Gets the venv Python interpreter if toolkit is installed, undefined otherwise.
diff --git a/src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts b/src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
@@ -345,16 +345,13 @@ export class DeepnoteKernelAutoSelector implements IDeepnoteKernelAutoSelector,
                     logger.info(`Using base interpreter: ${getDisplayPath(interpreter.uri)}`);
 
                     // Ensure deepnote-toolkit is installed in a venv and get the venv interpreter
+                    // Will throw typed errors on failure (DeepnoteVenvCreationError or DeepnoteToolkitInstallError)
                     progress.report({ message: l10n.t('Installing Deepnote toolkit...') });
                     const venvInterpreter = await this.toolkitInstaller.ensureInstalled(
                         interpreter,
                         baseFileUri,
                         progressToken
                     );
-                    if (!venvInterpreter) {
-                        logger.error('Failed to set up Deepnote toolkit environment');
-                        return; // Exit gracefully
-                    }
 
                     logger.info(`Deepnote toolkit venv ready at: ${getDisplayPath(venvInterpreter.uri)}`);
 
@@ -567,22 +564,32 @@ export class DeepnoteKernelAutoSelector implements IDeepnoteKernelAutoSelector,
             logger.error(error.getErrorReport());
 
             // Show user-friendly error with actions
-            const actions: string[] = ['Show Output', 'Copy Error Details'];
+            const showOutputAction = l10n.t('Show Output');
+            const copyErrorAction = l10n.t('Copy Error Details');
+            const actions: string[] = [showOutputAction, copyErrorAction];
+
+            const troubleshootingHeader = l10n.t('Troubleshooting:');
+            const troubleshootingSteps = error.troubleshootingSteps
+                .slice(0, 3)
+                .map((step, i) => `${i + 1}. ${step}`)
+                .join('\n');
 
             const selectedAction = await window.showErrorMessage(
-                `${error.userMessage}\n\nTroubleshooting:\n${error.troubleshootingSteps
-                    .slice(0, 3)
-                    .map((step, i) => `${i + 1}. ${step}`)
-                    .join('\n')}`,
+                `${error.userMessage}\n\n${troubleshootingHeader}\n${troubleshootingSteps}`,
                 { modal: false },
                 ...actions
             );
 
-            if (selectedAction === 'Show Output') {
+            if (selectedAction === showOutputAction) {
                 this.outputChannel.show();
-            } else if (selectedAction === 'Copy Error Details') {
-                await env.clipboard.writeText(error.getErrorReport());
-                void window.showInformationMessage('Error details copied to clipboard');
+            } else if (selectedAction === copyErrorAction) {
+                try {
+                    await env.clipboard.writeText(error.getErrorReport());
+                    void window.showInformationMessage(l10n.t('Error details copied to clipboard'));
+                } catch (clipboardError) {
+                    logger.error('Failed to copy error details to clipboard', clipboardError);
+                    void window.showErrorMessage(l10n.t('Failed to copy error details to clipboard'));
+                }
             }
 
             return;
@@ -592,13 +599,14 @@ export class DeepnoteKernelAutoSelector implements IDeepnoteKernelAutoSelector,
         const errorMessage = error instanceof Error ? error.message : String(error);
         logger.error(`Deepnote kernel error: ${errorMessage}`);
 
+        const showOutputAction = l10n.t('Show Output');
         const selectedAction = await window.showErrorMessage(
             l10n.t('Failed to load Deepnote kernel: {0}', errorMessage),
             { modal: false },
-            'Show Output'
+            showOutputAction
         );
 
-        if (selectedAction === 'Show Output') {
+        if (selectedAction === showOutputAction) {
             this.outputChannel.show();
         }
     }
