#2900: Avoid duplicate reporting in menuItemExtension #2901
Conversation
|
I think these are the only affected lines, at least around these APIs:
They used to map directly to jQuery.notify without reporting the error anywhere. My recent PR added automatic error reporting to all notifications so there might be more elsewhere, but I think I checked those in my last PR (these already used the new API) |
src/telemetry/BackgroundLogger.ts
Outdated
| @@ -71,8 +71,7 @@ class BackgroundLogger implements Logger { | |||
| } | |||
|
|
|||
| async error(error: unknown, data: JsonObject): Promise<void> { | |||
| console.error("An error occurred: %s", getErrorMessage(error), { | |||
| error, | |||
| console.error(error, { | |||
| void this.runTrigger(element); | ||
| } | ||
|
|
||
| void this.runTriggersAndNotify(...$element); |
There was a problem hiding this comment.
In this mode, notify was never called
| await Promise.allSettled( | ||
| $root.toArray().flatMap(async (root) => this.runTrigger(root)) | ||
| ); | ||
| await this.runTriggersAndNotify(...$root); |
There was a problem hiding this comment.
In this mode, notify was never called
| static notifyErrors(results: Array<PromiseSettledResult<unknown[]>>): void { | ||
| const errors = compact( | ||
| results.flatMap((x) => (x.status === "fulfilled" ? x.value : [x.reason])) | ||
| private async runTriggersAndNotify(...roots: ReaderRoot[]): Promise<void> { |
There was a problem hiding this comment.
This code was at least partially repeated in 3 places, so I merged it under a single method. However no errors actually ever reach this point due to #2900 (comment)
| showNotification({ | ||
| message, | ||
| type: className as NotificationType, | ||
| reportError: false, |
There was a problem hiding this comment.
This restored the old no-report behavior of notifyResult.
Since this function called by a brick which depends on the jQuery.notify types, I have not fully refactored the function. We should probably look into it at some point. Added to #2865 (comment)
src/telemetry/BackgroundLogger.ts
Outdated
| @@ -71,8 +71,7 @@ class BackgroundLogger implements Logger { | |||
| } | |||
|
|
|||
| async error(error: unknown, data: JsonObject): Promise<void> { | |||
| console.error("An error occurred: %s", getErrorMessage(error), { | |||
| error, | |||
| console.error(error, { | |||
There was a problem hiding this comment.
In practice this should be OK (because this should always be an object). But technically this is not safe/correct because you want to avoid user-defined format strings. In the case of console.log, I think the worst they can do is make the message non-sense
src/telemetry/BackgroundLogger.ts
Outdated
| // Avoid user-defined format strings | ||
| console.error("%s", error, { |
There was a problem hiding this comment.
This solution is ok for now, why is the user able to throw strings? We should wrap them as soon as possible I think.
There was a problem hiding this comment.
Theoretically the user isn't able to throw strings. However, the type of error is unknown, so from a defensive coding perspective you want to either: 1) update the type to be accurate, or 2) handle the possibilities
There was a problem hiding this comment.
We can use selectError in that case to ensure we have a real Error (it wraps strings in new Error)
There was a problem hiding this comment.
I'm kinda surprised and annoyed that console.log supports this stuff with random strings 😂
Related:
react-toast-notifications(and use one style for all toasts) #2836