Move toasts to browser (BL-15932) by andrew-polk · Pull Request #7724 · BloomBooks/BloomDesktop

andrew-polk · 2026-03-07T05:04:43Z

This change is

JohnThomson

@JohnThomson reviewed 13 files and all commit messages, and made 13 comments.
Reviewable status: 13 of 19 files reviewed, 13 unresolved discussions (waiting on andrew-polk).

a discussion (no related file):
The current design is very oriented towards toasts originating in C#. Toast actions, for example, are always strings that will activate a C# action via a post. Should we at all anticipate that as UI continues to migrate to JS, we will want toasts to originate there? Of course we can address that when it happens, but it seemed worth asking the question.

src/BloomBrowserUI/bookEdit/workspaceRoot.ts line 565 at r6 (raw file):

    workspaceModeInitializationStarted = true;
    initializeWorkspaceRootDocument();

Currently, unfortunately, the workspaceRoot.ts file is importted by other bundles and inline code in it gets executed in more than one iframe. We have various workarounds to make sure initialization code only executes in the actual workspace iframe. This should probably be inside something like that, though perhaps the absence of a #workspace-toast-host-root in any other iframe is enough.

src/BloomBrowserUI/toast/README.md line 9 at r6 (raw file):

- `ToastHost.tsx` subscribes to websocket `toast/show` events, de-duplicates repeated toasts, renders stacked toasts, and dispatches toast actions.
- `Toast.tsx` renders a single toast and defines the browser-side toast payload types.
- `toastUtils.ts` provides window-event helpers used by the devtools testing hooks.

If this is test-only code, shouldn't it be X.test.ts or X.spec.ts or something so it doesn't get included in production bundles?

src/BloomBrowserUI/toast/README.md line 25 at r6 (raw file):

- `ToastHost.tsx` currently de-duplicates by `l10nId`, falling back to `text`.
- Backend callbacks are one-shot: `toast/performAction` removes the callback registration before invoking it.
- If repeated calls represent the same logical toast and carry an action, the backend caller should pass a `toastId` so repeated show events reuse one callback slot instead of leaking registrations the UI will never expose.

I think it might be worth explaining a little more here, perhaps with examples. What is de-duplicated? Multiple attempts to make a Toast with the same content? Does that mean you can NEVER send the same message as a toast again later, or only while the first one is up, or...? From what I've read so far I'm not sure what a backend callback is or what it means for it to be registered or what I might call repeatedly or how two calls might represent the same logical toast...if this is meant to introduce the whole subsystem maybe you need a bit more in the overview?

src/BloomBrowserUI/toast/README.md line 70 at r6 (raw file):

  { text: "Duplicate check", severity: "notice", durationSeconds: 30 },
]);

In what sense are some tests (all in js) back-end? Why do only the front-end tests have durations?

src/BloomBrowserUI/toast/README.md line 87 at r6 (raw file):

Backend callback registration behavior is covered in `src/BloomTests/web/ToastServiceTests.cs`.

The most important regression here is repeated actionable toasts that are visually de-duplicated by the browser. The existing test verifies that repeated `ShowToast(..., toastId: ...)` calls reuse a single registered callback and keep the latest callback.

"most important" = the regression we most want to be sure does not happen?

src/BloomBrowserUI/toast/Toast.tsx line 112 at r6 (raw file):

                    return;
                }
                props.onClose(props.toast);

I'm puzzled what's going on here, maybe because I don't know enough about Snackbar. Seems like the way toasts go away is by calling props.OnClose(). So the early return means it will NOT go away? That is, Snackbar has some way of detecting a click outside itself, and that normally closes it, but because of the early return it will not? Might be worth a comment; the expectation is that if something calls onClose, it's going to close.
Will whatever is detecting the click away eat a click directed at something else? Seems like toasts should not be that intrusive.

src/BloomBrowserUI/toast/Toast.tsx line 141 at r6 (raw file):

                    if (props.toast.action) {
                        props.onAction(props.toast);
                    }

It seems strange that props includes both an onAction function and an object with a property that is called an action but is only a string. There are probably good reasons but anything you could do to explain would probably be helpful. (It makes more sense after reading the C# side: the string is an identifier for a C# action. Maybe it would help to call it actionId or even cSharpActionId?

src/BloomBrowserUI/toast/Toast.tsx line 190 at r6 (raw file):

                            {localizedActionLabel}
                        </button>
                    ) : null}

The usual React idiom for something that should only be shown if X is true is X && (component), not X ? (component) : null.

src/BloomBrowserUI/toast/ToastHost.tsx line 34 at r6 (raw file):

                if (!isDuplicate) {
                    nextToasts.push(incomingToast);
                }

If it is a duplicate but the time it ought to expire is later than the existing one, should we extend the wait time?

src/BloomBrowserUI/toast/ToastHost.tsx line 58 at r6 (raw file):

            if (toast.action.url) {
                window.location.href = toast.action.url;

This will replace our whole main window UI with whatever the URL points to? We don't want to launch it in a separate browser?
Also surprises me a little that we don't want to remove the toast, but I guess that makes sense since we're going to navigate away from the whole page that has the toasts.

src/BloomBrowserUI/toast/ToastHost.tsx line 66 at r6 (raw file):

                    callbackId: toast.action.callbackId,
                }).then(() => {
                    removeToast(toast);

Worth commenting why we want to wait for a successful return to remove the toast?

src/BloomBrowserUI/toast/ToastHost.tsx line 116 at r6 (raw file):

    return (
        <>

Slightly surprising that there is no CSS here at all. I guess this means we will just allow the div that this gets rendered into to stack them vertically? We don't need any padding or margin between them or overflow handling if the stack gets higher than the window...

JohnThomson

@JohnThomson reviewed 6 files.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on andrew-polk).

andrew-polk

@andrew-polk made 13 comments.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on JohnThomson).

a discussion (no related file):