Skip to content

fix(#2881): temporary notification set 0 to stop auto-dismiss #2882

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

Closed
vanessatran-ddi wants to merge 2 commits into from
Closed

fix(#2881): temporary notification set 0 to stop auto-dismiss #2882

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
14bcb36
fix(#2881): temporary notification set 0 to stop auto-dismiss
vanessatran-ddi Jul 11, 2025
d9f4859
fix(#2850): temporary notification browser test
vanessatran-ddi Jul 11, 2025
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
3 changes: 1 addition & 2 deletions libs/common/src/lib/temporary-notification-controller/temporary-notification-controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,8 @@ const TypesRequiringDuration: GoabTemporaryNotificationType[] = ["basic", "succe
function show(message: string, opts?: Partial<GoabNotificationOptions>): string {
const uuid = crypto.randomUUID();
opts = { uuid, type: "basic", ...(opts || {}) };

// set default duration for certain notification types
if (!opts.duration && opts.type && TypesRequiringDuration.includes(opts.type)) {
if (opts.duration === undefined && opts.type && TypesRequiringDuration.includes(opts.type)) {
Copy link
Copy Markdown
Collaborator

@chrisolsen chrisolsen Jul 14, 2025

Choose a reason for hiding this comment

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

This works as well, but what is the reason for the change?

Copy link
Copy Markdown
Collaborator Author

@vanessatran-ddi vanessatran-ddi Jul 14, 2025

Choose a reason for hiding this comment

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

When we pass 0 as a number, the !opts.duration is true, and it doesn't work.

opts.duration = "short";
}

Expand Down
26 changes: 13 additions & 13 deletions libs/react-components/specs/temporary-notification-ctrl.browser.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,24 +217,24 @@ describe("Temporary Notification Controller", () => {
};

const result = render(<Component />);
const notification = result.getByTestId("cancel-notification");
const actionButton = result.getByRole("button");


await vi.waitFor(async () => {
sendCancellableNotificationWithoutNotificationResponse("Notification with cancel action", "cancel-notification");

// Verify the notification is displayed
const notification = result.getByTestId("cancel-notification");
Copy link
Copy Markdown
Collaborator Author

@vanessatran-ddi vanessatran-ddi Jul 11, 2025

Choose a reason for hiding this comment

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

The test failed in headless mode (passed in browser mode) because of a race condition. We query the notification outside the waitFor block, so we query it even before we trigger the temporary notification, that is why it failed.

Copy link
Copy Markdown
Collaborator

@chrisolsen chrisolsen Jul 14, 2025

Choose a reason for hiding this comment

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

I always run in headless mode and it never failed. I am also not clear on what you mean by us querying it before triggering it.

expect(notification.elements().length).toBe(1);
}, { timeout: 5000 });

// Click the cancel button
expect(actionButton.elements().length).toBe(1);
await actionButton.click();
// Get the action button and click it
const actionButton = result.getByRole("button");
expect(actionButton.elements().length).toBe(1);
await actionButton.click();

// Verify the notification is removed
await vi.waitFor(() => {
expect(notification.elements().length).toBe(0);
});
});
// Verify the notification is removed
await vi.waitFor(() => {
const notification = result.getByTestId("cancel-notification");
expect(notification.elements().length).toBe(0);
}, { timeout: 5000 });
})

it.skip("should handle progress updates", async function () {
Expand Down