Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(OverlayToaster): Fix toasts being cut off if show() called too quickly #7049

Merged
merged 4 commits into from
Nov 7, 2024
Merged

fix(OverlayToaster): Fix toasts being cut off if show() called too quickly #7049

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fee2746
fix(OverlayToaster): Fix toasts being cut off if show() called too qu…
jscheiny Nov 7, 2024
70c23ac
Add generated changelog entries
svc-changelog Nov 7, 2024
2dfd9f6
Fix comment
jscheiny Nov 7, 2024
50eaa23
CR
jscheiny Nov 7, 2024
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
6 changes: 6 additions & 0 deletions packages/core/changelog/@unreleased/pr-7049.v2.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
type: fix
fix:
description: 'fix(OverlayToaster): Fix toasts being cut off if show() called too
quickly'
links:
- https://github.com/palantir/blueprint/pull/7049
75 changes: 65 additions & 10 deletions packages/core/src/components/toast/overlayToaster.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,13 @@ export interface OverlayToasterState {

export type OverlayToasterCreateOptions = DOMMountOptions<OverlayToasterProps>;

interface OverlayToasterQueueState {
cancel: (() => void) | undefined;
isRunning: boolean;
toasts: ToastOptions[];
}

const QUEUE_TIMEOUT_MS = 50;
Copy link
Contributor

Choose a reason for hiding this comment

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

What determines that the timeout should be 50ms? Is this something that we should potentially make configurable?

Copy link
Contributor Author

@jscheiny jscheiny Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This was based on experimentation on my part, it was a value that seemed to never exhibit issues with this problem (+ some buffer to be safe) and not have too much apparent effect on the UX of the component. I don't see a particular reason right now to make this configurable.

/**
* OverlayToaster component.
*
Expand Down Expand Up @@ -132,6 +139,14 @@ export class OverlayToaster extends AbstractPureComponent<OverlayToasterProps, O
toasts: [],
};

// Queue of toasts to be displayed. If toasts are shown too quickly back to back, it can result in cut off toasts.
// The queue ensures that toasts are only displayed in QUEUE_TIMEOUT_MS increments.
private queue: OverlayToasterQueueState = {
cancel: undefined,
isRunning: false,
toasts: [],
};

// auto-incrementing identifier for un-keyed toasts
private toastId = 0;

Expand All @@ -152,24 +167,66 @@ export class OverlayToaster extends AbstractPureComponent<OverlayToasterProps, O
return options.key;
}

if (this.queue.isRunning) {
// If a toast has been shown recently, push to the queued toasts to prevent toasts from being shown too
// quickly for the animations to keep up
this.queue.toasts.push(options);
} else {
// If we have not recently shown a toast, we can immediately show the given toast
this.immediatelyShowToast(options);
this.maybeStartQueueTimeout();
}

return options.key;
}

private maybeUpdateExistingToast(options: ToastOptions, key: string | undefined) {
if (key == null) {
return false;
}

const isExistingQueuedToast = this.queue.toasts.some(toast => toast.key === key);
if (isExistingQueuedToast) {
this.queue.toasts = this.queue.toasts.map(t => (t.key === key ? options : t));
return true;
}

const isExistingShownToast = this.state.toasts.some(toast => toast.key === key);
if (isExistingShownToast) {
this.updateToastsInState(toasts => toasts.map(t => (t.key === key ? options : t)));
return true;
}

return false;
}

private immediatelyShowToast(options: ToastOptions) {
if (this.props.maxToasts) {
// check if active number of toasts are at the maxToasts limit
this.dismissIfAtLimit();
}

this.updateToastsInState(toasts => [options, ...toasts]);
return options.key;
}

private maybeUpdateExistingToast(options: ToastOptions, key: string | undefined) {
if (key == null || this.isNewToastKey(key)) {
return false;
private maybeStartQueueTimeout() {
if (this.queue.isRunning) {
return;
}

this.updateToastsInState(toasts => toasts.map(t => (t.key === key ? options : t)));
return true;
this.queue.isRunning = true;
this.queue.cancel = this.setTimeout(this.handleQueueTimeout, QUEUE_TIMEOUT_MS);
}

private handleQueueTimeout = () => {
const nextToast = this.queue.toasts.shift();
this.queue.isRunning = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have to set isRunning to false during each execution of the timeout? Would it make sense to check that the queue is empty first? Something like:

if (this.queue.toasts.length === 0) {
    this.queue.isRunning = false;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah good call. I ended up implementing this slightly differently but with the same intent.

if (nextToast != null) {
this.immediatelyShowToast(nextToast);
this.maybeStartQueueTimeout();
}
};

private updateToastsInState(getNewToasts: (toasts: ToastOptions[]) => ToastOptions[]) {
this.setState(prevState => {
const toasts = getNewToasts(prevState.toasts);
Expand All @@ -191,6 +248,8 @@ export class OverlayToaster extends AbstractPureComponent<OverlayToasterProps, O
}

public clear() {
this.queue.cancel?.();
this.queue = { cancel: undefined, isRunning: false, toasts: [] };
this.state.toasts.forEach(t => t.onDismiss?.(false));
this.setState({ toasts: [], toastRefs: {} });
}
Expand Down Expand Up @@ -252,10 +311,6 @@ export class OverlayToaster extends AbstractPureComponent<OverlayToasterProps, O
});
}

private isNewToastKey(key: string) {
return this.state.toasts.every(toast => toast.key !== key);
}

private dismissIfAtLimit() {
if (this.state.toasts.length === this.props.maxToasts) {
// dismiss the oldest toast to stay within the maxToasts limit
Expand Down
50 changes: 44 additions & 6 deletions packages/core/test/toast/overlayToasterTests.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,13 +108,31 @@ describe("OverlayToaster", () => {
});
});

it("multiple show()s renders them all", () => {
it("multiple show()s renders them all", async () => {
toaster.show({ message: "one" });
toaster.show({ message: "two" });
toaster.show({ message: "six" });
await delay(150);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any way we can tie these hardcoded values to the value of QUEUE_TIMEOUT_MS? That might make them a bit less ambiguous in the context of these tests. Perhaps we could leave a comment or even export reference the constant itself in code:

await delay(3 * QUEUE_TIMEOUT_MS);

Also, should we be using timer mocks in our tests?

Copy link
Contributor Author

@jscheiny jscheiny Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah good call on the delay, will export that. I attempted to user the timer mocks and I wasn't able to get them to work, will give them another shot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Figured out what I messed up and switched to fake timers 👍

assert.lengthOf(toaster.getToasts(), 3, "expected 3 toasts");
});

it("multiple shows() get queued if provided too quickly", async () => {
toaster.show({ message: "one" });
toaster.show({ message: "two" });
toaster.show({ message: "three" });
assert.lengthOf(toaster.getToasts(), 1, "expected 1 toast");
await delay(150);
assert.lengthOf(toaster.getToasts(), 3, "expected 3 toasts after delay");
});

it("show() immediately displays a toast when waiting after the previous show()", async () => {
toaster.show({ message: "one" });
assert.lengthOf(toaster.getToasts(), 1, "expected 1 toast");
await delay(100);
toaster.show({ message: "two" });
assert.lengthOf(toaster.getToasts(), 2, "expected 2 toasts");
});

it("show() updates existing toast", () => {
const key = toaster.show({ message: "one" });
assert.deepEqual(toaster.getToasts()[0].message, "one");
Expand All @@ -123,24 +141,38 @@ describe("OverlayToaster", () => {
assert.deepEqual(toaster.getToasts()[0].message, "two");
});

it("dismiss() removes just the toast in question", () => {
it("show() updates existing toast in queue", async () => {
toaster.show({ message: "one" });
const key = toaster.show({ message: "two" });
toaster.show({ message: "two updated" }, key);
await delay(100);
assert.lengthOf(toaster.getToasts(), 2, "expected 2 toasts");
assert.deepEqual(toaster.getToasts()[0].message, "two updated");
});

it("dismiss() removes just the toast in question", async () => {
toaster.show({ message: "one" });
const key = toaster.show({ message: "two" });
toaster.show({ message: "six" });
await delay(150);
toaster.dismiss(key);
assert.deepEqual(
toaster.getToasts().map(t => t.message),
["six", "one"],
);
});

it("clear() removes all toasts", () => {
it("clear() removes all toasts", async () => {
toaster.show({ message: "one" });
toaster.show({ message: "two" });
toaster.show({ message: "six" });
assert.lengthOf(toaster.getToasts(), 3, "expected 3 toasts");
await delay(50);
assert.lengthOf(toaster.getToasts(), 2, "expected 2 toasts");
toaster.clear();
assert.lengthOf(toaster.getToasts(), 0, "expected 0 toasts");
// Ensure the queue is cleared
await delay(100);
assert.lengthOf(toaster.getToasts(), 0, "expected 0 toasts");
});

it("action onClick callback invoked when action clicked", () => {
Expand Down Expand Up @@ -214,19 +246,21 @@ describe("OverlayToaster", () => {
document.documentElement.removeChild(testsContainerElement);
});

it("does not exceed the maximum toast limit set", () => {
it("does not exceed the maximum toast limit set", async () => {
toaster.show({ message: "one" });
toaster.show({ message: "two" });
toaster.show({ message: "three" });
toaster.show({ message: "oh no" });
await delay(200);
assert.lengthOf(toaster.getToasts(), 3, "expected 3 toasts");
});

it("does not dismiss toasts when updating an existing toast at the limit", () => {
it("does not dismiss toasts when updating an existing toast at the limit", async () => {
toaster.show({ message: "one" });
toaster.show({ message: "two" });
toaster.show({ message: "three" }, "3");
toaster.show({ message: "three updated" }, "3");
await delay(200);
assert.lengthOf(toaster.getToasts(), 3, "expected 3 toasts");
});
});
Expand Down Expand Up @@ -282,3 +316,7 @@ describe("OverlayToaster", () => {
});
});
});

function delay(ms: number) {
return new Promise(resolve => setTimeout(resolve, ms));
}