Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions extensions/ql-vscode/src/compare/compare-view.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import { assertNever, getErrorMessage } from "../pure/helpers-pure";
import { HistoryItemLabelProvider } from "../query-history/history-item-label-provider";
import { AbstractWebview, WebviewPanelConfig } from "../abstract-webview";
import { telemetryListener } from "../telemetry";
import { redactableError } from "../pure/errors";
import { showAndLogExceptionWithTelemetry } from "../helpers";

interface ComparePair {
from: CompletedLocalQueryInfo;
Expand Down Expand Up @@ -139,6 +141,14 @@ export class CompareView extends AbstractWebview<
telemetryListener?.sendUIInteraction(msg.action);
break;

case "unhandledError":
void showAndLogExceptionWithTelemetry(
redactableError(
msg.error,
)`Unhandled error in result comparison view: ${msg.error.message}`,
);
break;

default:
assertNever(msg);
}
Expand Down
7 changes: 7 additions & 0 deletions extensions/ql-vscode/src/interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,13 @@ export class ResultsView extends AbstractWebview<
case "telemetry":
telemetryListener?.sendUIInteraction(msg.action);
break;
case "unhandledError":
void showAndLogExceptionWithTelemetry(
redactableError(
msg.error,
)`Unhandled error in results view: ${msg.error.message}`,
);
break;
default:
assertNever(msg);
}
Expand Down
23 changes: 19 additions & 4 deletions extensions/ql-vscode/src/pure/errors.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
export class RedactableError extends Error {
constructor(
cause: Error | undefined,
cause: ErrorLike | undefined,
private readonly strings: TemplateStringsArray,
private readonly values: unknown[],
) {
Expand Down Expand Up @@ -54,19 +54,34 @@ export function redactableError(
...values: unknown[]
): RedactableError;
export function redactableError(
error: Error,
error: ErrorLike,
): (strings: TemplateStringsArray, ...values: unknown[]) => RedactableError;

export function redactableError(
errorOrStrings: Error | TemplateStringsArray,
errorOrStrings: ErrorLike | TemplateStringsArray,
...values: unknown[]
):
| ((strings: TemplateStringsArray, ...values: unknown[]) => RedactableError)
| RedactableError {
if (errorOrStrings instanceof Error) {
if (isErrorLike(errorOrStrings)) {
return (strings: TemplateStringsArray, ...values: unknown[]) =>
new RedactableError(errorOrStrings, strings, values);
} else {
return new RedactableError(undefined, errorOrStrings, values);
}
}

export interface ErrorLike {
message: string;
stack?: string;
}

function isErrorLike(error: any): error is ErrorLike {
if (
typeof error.message === "string" &&
(typeof error.stack === "undefined" || typeof error.stack === "string")
Comment thread
robertbrignull marked this conversation as resolved.
Outdated
) {
return true;
}
return false;
}
15 changes: 12 additions & 3 deletions extensions/ql-vscode/src/pure/interface-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
VariantAnalysisScannedRepositoryState,
} from "../variant-analysis/shared/variant-analysis";
import { RepositoriesFilterSortStateWithIds } from "./variant-analysis-filter-sort";
import { ErrorLike } from "./errors";

/**
* This module contains types and code that are shared between
Expand Down Expand Up @@ -189,7 +190,8 @@ export type FromResultsViewMsg =
| ViewLoadedMsg
| ChangePage
| OpenFileMsg
| TelemetryMessage;
| TelemetryMessage
| UnhandledErrorMessage;
Comment thread
robertbrignull marked this conversation as resolved.
Outdated

/**
* Message from the results view to open a database source
Expand Down Expand Up @@ -291,7 +293,8 @@ export type FromCompareViewMessage =
| ChangeCompareMessage
| ViewSourceFileMsg
| OpenQueryMessage
| TelemetryMessage;
| TelemetryMessage
| UnhandledErrorMessage;

/**
* Message from the compare view to request opening a query.
Expand Down Expand Up @@ -439,6 +442,11 @@ export interface TelemetryMessage {
action: string;
}

export interface UnhandledErrorMessage {
t: "unhandledError";
error: ErrorLike;
}

export type ToVariantAnalysisMessage =
| SetVariantAnalysisMessage
| SetRepoResultsMessage
Expand All @@ -453,4 +461,5 @@ export type FromVariantAnalysisMessage =
| ExportResultsMessage
| OpenLogsMessage
| CancelVariantAnalysisMessage
| TelemetryMessage;
| TelemetryMessage
| UnhandledErrorMessage;
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,12 @@ import {
VariantAnalysisViewInterface,
VariantAnalysisViewManager,
} from "./variant-analysis-view-manager";
import { showAndLogWarningMessage } from "../helpers";
import {
showAndLogExceptionWithTelemetry,
showAndLogWarningMessage,
} from "../helpers";
import { telemetryListener } from "../telemetry";
import { redactableError } from "../pure/errors";

export class VariantAnalysisView
extends AbstractWebview<ToVariantAnalysisMessage, FromVariantAnalysisMessage>
Expand Down Expand Up @@ -153,6 +157,13 @@ export class VariantAnalysisView
case "telemetry":
telemetryListener?.sendUIInteraction(msg.action);
break;
case "unhandledError":
void showAndLogExceptionWithTelemetry(
redactableError(
msg.error,
)`Unhandled error in variant analysis results view: ${msg.error.message}`,
);
break;
default:
assertNever(msg);
}
Expand Down
54 changes: 54 additions & 0 deletions extensions/ql-vscode/src/view/common/errors.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import { getErrorMessage, getErrorStack } from "../../pure/helpers-pure";
import { vscode } from "../vscode-api";

// Keep track of previous errors that have happened.
// The listeners for uncaught errors and rejections can get triggered
// twice for each error. This is believed to be an effect caused
// by React's error boundaries. Adding an error boundary stops
// this duplicate reporting for errors that happen during component
// rendering, but unfortunately errors from event handlers and
// timeouts are still duplicated and there does not appear to be
// a way around this.
const previousErrors: Set<Error> = new Set();

function shouldReportError(error: Error): boolean {
const seenBefore = previousErrors.has(error);
previousErrors.add(error);
setTimeout(() => {
previousErrors.delete(error);
}, 1000);
return !seenBefore;
}

const unhandledErrorListener = (event: ErrorEvent) => {
if (shouldReportError(event.error)) {
vscode.postMessage({
t: "unhandledError",
error: {
message: getErrorMessage(event.error),
stack: getErrorStack(event.error),
},
});
}
};

const unhandledRejectionListener = (event: PromiseRejectionEvent) => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be making a distinction between the unhandled errors and unhandled rejections? As far as I am aware, unhandled rejections don't cause any user-visible behaviour (except perhaps some data not loading), but unhandled errors might cause the complete view to disappear. It would be useful to make that distinction in the telemetry.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now I'm going to leave them combined. I don't think it's quite as well defined as saying unhandled errors are always more important. Missing or incorrect data is also bad, and maybe worse if the user don't know the data they're looking at is incorrect. Unhandled errors can also happen outside of rendering.

Also, we can certainly split these up in the future, and doing it after this PR will be fine. The telemetry is quite schema-less and we only keep it for a short period, so changing the format is quite cheap to do.

if (shouldReportError(event.reason)) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure but I think the problem of duplicate errors only happens for thrown errors and not rejected promises. So this check here may be unnecessary, but it also shouldn't hurt so I decided to keep it in. I'm not really sure about this though and I'm happy to remove it.

vscode.postMessage({
t: "unhandledError",
error: {
message: getErrorMessage(event.reason),
stack: getErrorStack(event.reason),
},
});
}
};

/**
* Adds listeners for unhandled errors / rejected promises.
* When an error is detected a "unhandledError" message is posted to the view.
*/
export function registerUnhandledErrorListener() {
window.addEventListener("error", unhandledErrorListener);
window.addEventListener("unhandledrejection", unhandledRejectionListener);
}
3 changes: 3 additions & 0 deletions extensions/ql-vscode/src/view/webview.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,11 @@ import { WebviewDefinition } from "./webview-definition";

// Allow all views to use Codicons
import "@vscode/codicons/dist/codicon.css";
import { registerUnhandledErrorListener } from "./common/errors";

const render = () => {
registerUnhandledErrorListener();

const element = document.getElementById("root");

if (!element) {
Expand Down