Skip to content

Commit b7af87a

Browse files
authored
Improve error reporting when sagas crash (#8712)
Errors within the sagas are critical. They are reported currently, but if a simple failing network request brings down the entire sagas (most have a retry mechanism but some don't), the current error reporting only shows the failing network request (which doesn't really show the severity). ### URL of deployed dev instance (used for testing): - https://___.webknossos.xyz ### Steps to test: - I added a `throw new Error("bla")` to the brush saga. you can see the report for it here: https://scm.airbrake.io/projects/95438/groups/4097474921126226079?tab=notice-detail - no need to test further imo ### Issues: - improves error reporting
1 parent 6286020 commit b7af87a

File tree

2 files changed

+40
-21
lines changed

2 files changed

+40
-21
lines changed

frontend/javascripts/libs/error_handling.ts

Lines changed: 36 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,31 @@ export function handleGenericError(
5454
console.warn(error);
5555
}
5656

57+
function mutateErrorMessage(error: Error, newMessage: string) {
58+
try {
59+
// In the past, we had records of the following line not working because message was read-only.
60+
// However, with Chrome, Firefox, Safari and Edge, that bug could not be reproduced in July 2024.
61+
// Therefore, we assume that the code should work. If not, we don't augment the error message.
62+
// Instead, the exceptionType is passed to the notify() method below.
63+
error.message = newMessage;
64+
} catch {}
65+
}
66+
67+
function anyToError(maybeError: any): Error {
68+
return maybeError instanceof Error ? maybeError : new Error(maybeError);
69+
}
70+
71+
function getPrefixedErrorMessage(
72+
prefix: string,
73+
maybeError: unknown,
74+
): { prefixedMessage: string; fullMessage: string } {
75+
const fullMessage =
76+
maybeError instanceof Error ? maybeError.toString() : JSON.stringify(maybeError);
77+
const prefixedMessage = prefix + fullMessage.slice(0, 80);
78+
79+
return { prefixedMessage, fullMessage };
80+
}
81+
5782
class ErrorHandling {
5883
// @ts-expect-error ts-migrate(2564) FIXME: Property 'throwAssertions' has no initializer and ... Remove this comment to see the full error message
5984
throwAssertions: boolean;
@@ -126,20 +151,7 @@ class ErrorHandling {
126151
window.removeEventListener("unhandledrejection", this.airbrake.onUnhandledrejection);
127152
window.addEventListener("unhandledrejection", (event) => {
128153
// Create our own error for unhandled rejections here to get additional information for [Object object] errors in airbrake
129-
const reasonAsString = event.reason instanceof Error ? event.reason.toString() : event.reason;
130-
let wrappedError = event.reason instanceof Error ? event.reason : new Error(event.reason);
131-
132-
const newMessage = UNHANDLED_REJECTION_PREFIX + JSON.stringify(reasonAsString).slice(0, 80);
133-
try {
134-
// In the past, we had records of the following line not working because message was read-only.
135-
// However, with Chrome, Firefox, Safari and Edge, that bug could not be reproduced in July 2024.
136-
// Therefore, we assume that the code should work. If not, we don't augment the error message.
137-
// Instead, the exceptionType is passed to the notify() method below.
138-
wrappedError.message = newMessage;
139-
} catch {}
140-
141-
this.notify(wrappedError, {
142-
originalError: reasonAsString,
154+
this.notifyWithPrefix(event.reason, UNHANDLED_REJECTION_PREFIX, {
143155
exceptionType: "unhandledrejection",
144156
});
145157
});
@@ -214,6 +226,16 @@ class ErrorHandling {
214226
});
215227
}
216228

229+
notifyWithPrefix(error: unknown, prefix: string, optParams: Record<string, any> = {}) {
230+
const wrappedError = anyToError(error);
231+
const { prefixedMessage, fullMessage } = getPrefixedErrorMessage(prefix, error);
232+
mutateErrorMessage(wrappedError, prefixedMessage);
233+
this.notify(wrappedError, {
234+
originalError: fullMessage,
235+
...optParams,
236+
});
237+
}
238+
217239
assertExtendContext(additionalContext: Record<string, any>) {
218240
this.airbrake.addFilter((notice) => {
219241
// @ts-expect-error ts-migrate(7006) FIXME: Parameter 'error' implicitly has an 'any' type.

frontend/javascripts/viewer/model/sagas/root_saga.ts

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -88,13 +88,13 @@ function* restartableSaga(): Saga<void> {
8888
call(splitBoundaryMeshSaga),
8989
call(toolSaga),
9090
]);
91-
} catch (err) {
91+
} catch (err: any) {
9292
rootSagaCrashed = true;
9393
console.error("The sagas crashed because of the following error:", err);
9494

9595
if (!process.env.IS_TESTING) {
96-
// @ts-ignore
97-
ErrorHandling.notify(err, {});
96+
ErrorHandling.notifyWithPrefix(err, "Root saga crashed: ");
97+
9898
// Hide potentially old error highlighting which mentions a retry mechanism.
9999
toggleErrorHighlighting(false);
100100
// Show error highlighting which mentions the permanent error.
@@ -103,10 +103,7 @@ function* restartableSaga(): Saga<void> {
103103
Internal error.
104104
Please reload the page to avoid losing data.
105105
106-
${
107-
JSON.stringify(err)
108-
// @ts-ignore
109-
} ${err.stack || ""}`);
106+
${JSON.stringify(err)} ${err.stack || ""}`);
110107
}
111108
}
112109
}

0 commit comments

Comments
 (0)