quieter provider errors with better origin information

- OpenCtx no longer uses vscode.window.showErrorMessage, which is annoying and louder than most provider errors deserve. - The output channel includes information about which provider the error came from, to make debugging easier.
sourcegraph · Sep 20, 2024 · 3a78f80 · 3a78f80
1 parent cd62b15
commit 3a78f80
Show file tree

Hide file tree

Showing 3 changed files with 12 additions and 101 deletions.
diff --git a/client/vscode-lib/src/controller.ts b/client/vscode-lib/src/controller.ts
@@ -23,7 +23,6 @@ import { createCodeLensProvider } from './ui/editor/codeLens.js'
 import { createHoverProvider } from './ui/editor/hover.js'
 import { createShowFileItemsList } from './ui/fileItemsList.js'
 import { createStatusBarItem } from './ui/statusBarItem.js'
-import { Cache, bestEffort } from './util/cache.js'
 import { ErrorReporterController, UserAction } from './util/errorReporter.js'
 import { importProvider } from './util/importHelpers.js'
 import { observeWorkspaceConfigurationChanges, toEventEmitter } from './util/observable.js'
@@ -122,18 +121,15 @@ export function createController({
     })
     disposables.push(client)
 
-    const errorLog = (error: any) => {
-        console.error(error)
-        outputChannel.appendLine(error)
-    }
-
     // errorReporter contains a lot of logic and state on how we notify and log
     // errors, as well as state around if we should turn off a feature (see
     // skipIfImplicitAction)
-    const errorReporter = new ErrorReporterController(
-        createErrorNotifier(outputChannel, extensionId, client),
-        errorLog,
-    )
+    const errorReporter = new ErrorReporterController((error: any, providerUri: string | undefined) => {
+        const prefix = `OpenCtx error (from provider ${providerUri ?? 'unknown'}): `
+        console.error(prefix, error)
+        const errorDetail = error.stack ? `${error} (stack trace follows)\n${error.stack}` : error
+        outputChannel.appendLine(`${prefix}${errorDetail}`)
+    })
     disposables.push(errorReporter)
 
     // Note: We distingiush between an explicit user action and an implicit
@@ -211,54 +207,3 @@ function ignoreDoc(params: AnnotationsParams): boolean {
 function makeRange(range: Range): vscode.Range {
     return new vscode.Range(range.start.line, range.start.character, range.end.line, range.end.character)
 }
-
-function createErrorNotifier(
-    outputChannel: vscode.OutputChannel,
-    extensionId: string,
-    client: Pick<VSCodeClient, 'meta'>,
-) {
-    // Fetching the name can be slow or fail. So we use a cache + timeout when
-    // getting the name of a provider.
-    const nameCache = new Cache<string>({ ttlMs: 10 * 1000 })
-    const getName = async (providerUri: string | undefined) => {
-        if (providerUri === undefined) {
-            return undefined
-        }
-        const fill = async () => {
-            const meta = await bestEffort(client.meta({}, { providerUri: providerUri }), {
-                defaultValue: [],
-                delay: 250,
-            })
-            return meta.pop()?.name ?? providerUri
-        }
-        return await nameCache.getOrFill(providerUri, fill)
-    }
-
-    const actionItems = [
-        {
-            title: 'Show Logs',
-            do: () => {
-                outputChannel.show()
-            },
-        },
-        {
-            title: 'Open Settings',
-            do: () => {
-                vscode.commands.executeCommand('workbench.action.openSettings', {
-                    query: `@ext:${extensionId} openctx.providers`,
-                })
-            },
-        },
-    ] satisfies (vscode.MessageItem & { do: () => void })[]
-
-    return async (providerUri: string | undefined, error: any) => {
-        const name = await getName(providerUri)
-        const message = name
-            ? `Error from OpenCtx provider "${name}": ${error}`
-            : `Error from OpenCtx: ${error}`
-        const action = await vscode.window.showErrorMessage(message, ...actionItems)
-        if (action) {
-            action.do()
-        }
-    }
-}
diff --git a/client/vscode-lib/src/util/cache.ts b/client/vscode-lib/src/util/cache.ts
@@ -22,7 +22,10 @@ export class Cache<T> {
     }
 }
 
-/** resolves promise, but will return defaultValue if promise throws or takes longer than delay ms */
+/**
+ * Try to resolve {@link promise}, but return {@link opts.defaultValue} if it throws or takes longer than
+ * {@link opts.delay} ms.
+ */
 export async function bestEffort<T>(
     promise: Promise<T>,
     opts: {

diff --git a/client/vscode-lib/src/util/errorReporter.ts b/client/vscode-lib/src/util/errorReporter.ts
@@ -4,8 +4,6 @@ import { Observable } from 'observable-fns'
 import type * as vscode from 'vscode'
 import { type ErrorWaiter, createErrorWaiter } from './errorWaiter.js'
 
-const MIN_TIME_SINCE_LAST_ERROR = 1000 * 60 * 15 /* 15 min */
-
 /**
  * The users action affects our behaviour around reporting, so we explicitly
  * tell ErrorReporter the intent.
@@ -29,12 +27,8 @@ export enum UserAction {
  */
 export class ErrorReporterController implements vscode.Disposable {
     private errorWaiters = new Map<string, ErrorWaiter>()
-    private errorNotificationVisible = new Set<string>()
 
-    constructor(
-        private showErrorNotification: (providerUri: string | undefined, error: any) => Thenable<any>,
-        private errorLog: (error: any) => void,
-    ) {}
+    constructor(private errorLog: (error: any, providerUri: string | undefined) => void) {}
 
     /**
      * wraps providerMethod to ensure it reports errors to the user.
@@ -120,7 +114,7 @@ export class ErrorReporterController implements vscode.Disposable {
             errorByUri.set(providerUri, errors)
 
             // Always log the error.
-            this.errorLog(error)
+            this.errorLog(error, providerUri)
         }
 
         // report takes the seen errors so far and decides if they should be
@@ -137,21 +131,6 @@ export class ErrorReporterController implements vscode.Disposable {
 
                 const errorWaiter = this.getErrorWaiter(providerUri)
                 errorWaiter.gotError(hasError)
-
-                // From here on out it is about notifying for an error
-                if (!hasError) {
-                    continue
-                }
-
-                // Show an error notification unless we've recently shown one
-                // (to avoid annoying the user).
-                const shouldNotify =
-                    userAction === UserAction.Explicit ||
-                    errorWaiter.timeSinceLastError() > MIN_TIME_SINCE_LAST_ERROR
-                if (shouldNotify) {
-                    const error = errors.length === 1 ? errors[0] : new AggregateError(errors)
-                    this.maybeShowErrorNotification(providerUri, error)
-                }
             }
 
             // Clear out seen errors so that report may be called again.
@@ -181,22 +160,6 @@ export class ErrorReporterController implements vscode.Disposable {
         return errorWaiter
     }
 
-    private maybeShowErrorNotification(providerUri: string, error: any) {
-        // We store the notification thenable so we can tell if the last
-        // notification for providerUri is still showing.
-        if (this.errorNotificationVisible.has(providerUri)) {
-            return
-        }
-        this.errorNotificationVisible.add(providerUri)
-        const onfinally = () => this.errorNotificationVisible.delete(providerUri)
-
-        // If providerUri is the empty string communicate that via undefined
-        this.showErrorNotification(providerUri === '' ? undefined : providerUri, error).then(
-            onfinally,
-            onfinally,
-        )
-    }
-
     public dispose() {
         for (const errorWaiter of this.errorWaiters.values()) {
             errorWaiter.dispose()