#2174: Replace unmaintained `react-toast-notifications` (and use one style for all toasts) #2836

fregante · 2022-02-28T06:57:23Z

Closes Replace unmaintained react-toast-notifications (and use one style for all toasts) #2174
Follows discussion in https://github.com/pixiebrix/pixiebrix-app/pull/1252
Unblocks https://github.com/pixiebrix/pixiebrix-app/pull/883
Related to #2214: Notification improvements (timer, dismissal) #2665
Finally fixes Move editor toasts to less bothering position #729 (comment)

Status: Rrrrrrrrrready

One notify.* API to rule them all. It replaces:

addToast
showNotification
useNotifications

Also standardizes the notify.error => automatically reportError

Currently-supported pages

sidebar
options
editor
contentScript

Demo

Notice:

the new style of notifications
the really-always-on-top z-index that notifications should have

Screen.Recording.3.mov

fregante · 2022-02-28T07:09:04Z

src/auth/ScopeSettings.tsx

-          notify.error("Error updating account alias", {
-            error,
-          });
+          notify.error({ message: "Error updating account alias", error });


API improvement: Either call it with a plain string or provide a regular Notification object.

Pros:

no awkward { error } wrap

simpler types

safer: no unknown as the first parameter of notify.error just to allow notify.error(new Error())

Cons:

8 extra characters 😌 message: if you need to include the error

8 extra characters 😌 message: if you need to include the error

I'm not sure we have the budget for the extra storage

fregante · 2022-02-28T07:28:33Z

src/contrib/pipedrive/create.ts

-        message: `Error adding ${name} to Pipedrive`,
-        type: "error",
-      });
+      notify.error(`Error adding ${name} to Pipedrive`);


As @twschiller suggested in https://github.com/pixiebrix/pixiebrix-app/pull/1252, errors are automatically reported. This was not the case with the previous showNotification implementation so let me know if I should set report: false to some of these.

These should actually be logger statements and BusinessErrors. Will change these

fregante · 2022-02-28T07:30:46Z

src/extensionPoints/menuItemExtension.ts

    MENU_INSTALL_ERROR_DEBOUNCE_MS,
    {
      leading: true,
      trailing: false,
    }
-  );
+  ) as typeof notify.error; // `debounce` loses the overloads


Side note: we might want to use memoize instead since this debounce may lump together unrelated errors.

Memoize won't work unless we define a custom equality function (over message). Debounce should be fine here in practice, since having multiple specific errors doesn't help much. This is more about not spamming the end-user with error notifications

fregante · 2022-02-28T07:33:06Z

src/hooks/useDeployments.ts

-        { error, report: true }
-      );
+      notify.warning({
+        message: `Error fetching latest bricks from server: ${getErrorMessage(


"notify.warning" … "Error"

Is it a warning or an error? When should an error be displayed as a warning?

The nuance is that with a warning, the user still might be OK continuing. But with an error, they would expect to no be able to proceed successfully. I'll change the message to clarify

fregante · 2022-02-28T07:41:36Z

src/hooks/useDeployments.ts

+        message:
+          "You must update the PixieBrix browser extension to activate the deployment",
+      });
+      reportEvent("DeploymentRejectVersion");


Error notifications almost always need to be reported and sometimes are reported with the exact same user message.

Events on the other hand have zero data overlap with notifications and so far only appear ~8 times in the whole extension, one of which was actually reported twice. Having events as an optional parameter means that its use must always be deliberate and explicit.

Events may also have additional attached data which would further complicate the notifier for little benefit.

For these reasons I dropped the event option from the notifier.

For these reasons I dropped the event option from the notifier

Sure, I think this makes sense. We'll give it a try

fregante · 2022-02-28T07:49:17Z

src/background/contextMenus.ts

    } else {
      const message = `Error handling context menu action: ${getErrorMessage(
        error
      )}`;
-      reportError(new Error(message));
-      void showNotification(target, { message, type: "error" });
+      reportError(error);


I think we want to report the original error here. The stack trace will point to this block anyway, but its origin will be better defined.

Here it's called explicitly because the messaging API does not automatically serialize errors, especially deep inside other objects.

fregante · 2022-02-28T07:51:48Z

src/options/pages/blueprints/useInstallableActions.ts

+    notify.success(`Removed brick ${getLabel(installable)}`);
    reportEvent("ExtensionRemove", {
      extensionId: installable.id,
    });


In this case reportEvent was called implicitly and explicitly

fregante · 2022-02-28T07:55:36Z

src/options/pages/services/ServicesEditor.tsx

+          message:
+            "Error refreshing service configurations, restart the PixieBrix extension",
+
+          error,


notify.warning never reported this error. @twschiller Pick one:

Drop line

Add report: true

Change to report.error

Same thing for the error below, in the same file

fregante · 2022-02-28T08:26:32Z

src/utils/notify.tsx

+const notify = {
+  /**
+   * @example notify.error('User message')
+   * @example notify.error({error: new Error('Error that can be shown to the user')})


This notifyError({error}) only happens a couple of times in the extension. We should probably not show raw errors to the user without adding some context, so I suggest requiring a message as always and only use error for the reporting:

} catch (error) { notifyError({ error, message: "Stuff failed because: " + getErrorMessage(error) }) }

…e-notify

fregante · 2022-03-01T06:47:08Z

src/components/logViewer/LogToolbar.tsx

-        appearance: "error",
-        autoDismiss: true,
+      notify.success("Cleared the log entries for this extension");
+    } catch (error) {


This error was not reported, now it is.

…e-notify

twschiller · 2022-03-03T01:00:57Z

I'm good with this for now. Future considerations:

We may potentially want to re-add as a hook if we want control of showing/hiding based on other application state. For just triggering toasts this PR is cleaner
Instead of having ad-hoc toast message creation (some including error details, some using getHumanDetail, some using getErrorMessage), I think we'll want to standardize to have:

// The message describing the operation that failed
message: string;

// The error detail, which the getHumanDetail will be called on
error: unknown;

// True to report the error to Rollbar (default: true)
reportError?: boolean;

// True to show the error detail to the user (default: true)
showDetails?: boolean;

In this world, the error detail will probably be less prominent (e.g., smaller font size)

twschiller · 2022-03-03T01:02:48Z

fregante added 8 commits February 28, 2022 14:47

Replace useNotifications with new notifier

d25e72a

Only accept one parameter in notifyError

ba8a251

Drop previous showNotification API

5d15dfd

Use new API consistently across the extension

d86906f

Restore warning notifications

fead813

Lint

fa9685a

DRY, centralize logic and prevent forks

26808c8

Update type mismatch

c0f6dcb

fregante commented Feb 28, 2022

View reviewed changes

fregante added 4 commits March 1, 2022 14:05

Merge remote-tracking branch 'origin/main' into F/feature/one-love-on…

aea6a63

…e-notify

Drop useToasts and AddToast

5435bd9

Drop ToastProvider

7c3b739

initToaster

27b64dc

fregante marked this pull request as ready for review March 1, 2022 06:43

fregante added 3 commits March 1, 2022 14:50

Avoid Notification object where string suffices

3907d04

Simplify mock

1e420d8

Drop last CSS remnant

fc8c8fa

fregante commented Mar 1, 2022

View reviewed changes

Merge remote-tracking branch 'origin/main' into F/feature/one-love-on…

8a54ce3

…e-notify

twschiller self-requested a review March 3, 2022 00:30

twschiller added 2 commits March 2, 2022 19:33

Merge remote-tracking branch 'origin/main' into F/feature/one-love-on…

12196f8

…e-notify

Address PR comments

4394530

twschiller added this to the 1.5.6 milestone Mar 3, 2022

twschiller approved these changes Mar 3, 2022

View reviewed changes

twschiller merged commit 5f3169e into main Mar 3, 2022

twschiller deleted the F/feature/one-love-one-notify branch March 3, 2022 01:03

This was referenced Mar 4, 2022

Further notification improvements #2865

Closed

Automatically append error details to notifications #2867

Merged

This was referenced Mar 7, 2022

Fix notification display #2895

Merged

#2900: Avoid duplicate reporting in menuItemExtension #2901

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#2174: Replace unmaintained `react-toast-notifications` (and use one style for all toasts) #2836

#2174: Replace unmaintained `react-toast-notifications` (and use one style for all toasts) #2836

fregante commented Feb 28, 2022 •

edited

Loading

fregante Feb 28, 2022 •

edited

Loading

twschiller Mar 3, 2022 •

edited

Loading

fregante Feb 28, 2022

twschiller Mar 3, 2022

fregante Feb 28, 2022

twschiller Mar 3, 2022 •

edited

Loading

fregante Feb 28, 2022 •

edited

Loading

twschiller Mar 3, 2022

fregante Feb 28, 2022

twschiller Mar 3, 2022 •

edited

Loading

fregante Feb 28, 2022 •

edited

Loading

fregante Feb 28, 2022

fregante Feb 28, 2022

fregante Feb 28, 2022

fregante Mar 1, 2022

twschiller commented Mar 3, 2022 •

edited

Loading

twschiller commented Mar 3, 2022

#2174: Replace unmaintained react-toast-notifications (and use one style for all toasts) #2836

#2174: Replace unmaintained react-toast-notifications (and use one style for all toasts) #2836

Conversation

fregante commented Feb 28, 2022 • edited Loading

Status: Rrrrrrrrrready

Currently-supported pages

Demo

fregante Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

twschiller Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twschiller Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

fregante Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twschiller Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

fregante Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twschiller commented Mar 3, 2022 • edited Loading

twschiller commented Mar 3, 2022

#2174: Replace unmaintained `react-toast-notifications` (and use one style for all toasts) #2836

#2174: Replace unmaintained `react-toast-notifications` (and use one style for all toasts) #2836

fregante commented Feb 28, 2022 •

edited

Loading

fregante Feb 28, 2022 •

edited

Loading

twschiller Mar 3, 2022 •

edited

Loading

twschiller Mar 3, 2022 •

edited

Loading

fregante Feb 28, 2022 •

edited

Loading

twschiller Mar 3, 2022 •

edited

Loading

fregante Feb 28, 2022 •

edited

Loading

twschiller commented Mar 3, 2022 •

edited

Loading