diff --git a/clients/web/src/hooks/useProgressToasts.test.tsx b/clients/web/src/hooks/useProgressToasts.test.tsx index 57bdcd22..037dd438 100644 --- a/clients/web/src/hooks/useProgressToasts.test.tsx +++ b/clients/web/src/hooks/useProgressToasts.test.tsx @@ -43,6 +43,18 @@ function harness(client: InspectorClientEventTarget) { total, }); }), + /** Same as `progress`, but from a client swapped in after mount. */ + progressOn: ( + next: InspectorClientEventTarget, + progressToken: string, + progress: number, + ) => + act(() => { + next.dispatchTypedEvent("progressNotification", { + progressToken, + progress, + }); + }), swapClient: (next: InspectorClientEventTarget | null) => act(() => rerender()), unmount: () => act(() => unmount()), @@ -141,4 +153,31 @@ describe("useProgressToasts", () => { progressToastId("tok-1"), ); }); + + // #2219: `hide()` defers `onClose` past the exit transition, so an outgoing + // toast's callback can fire *after* the new session has re-shown the same id + // (ids come from the progress token, so a reconnect replays them exactly). + // The teardown swaps in a fresh Set rather than clearing the shared one, so + // that late callback must not touch the new session's bookkeeping. No timers + // needed: calling the captured `onClose` by hand is exactly that late fire. + it("keeps the new session's bookkeeping when a hidden toast's onClose fires late", () => { + const h = harness(fakeClient()); + h.progress("tok-1", 1); + const { onClose: staleOnClose } = notificationsMock.show.mock + .calls[0][0] as { onClose: () => void }; + + const next = fakeClient(); + h.swapClient(next); + h.progressOn(next, "tok-1", 1); + expect(notificationsMock.show).toHaveBeenCalledTimes(2); + + // The outgoing session's toast finishes its exit transition here. + staleOnClose(); + + h.progressOn(next, "tok-1", 2); + expect(notificationsMock.show).toHaveBeenCalledTimes(2); + expect(notificationsMock.update).toHaveBeenCalledWith( + expect.objectContaining({ id: progressToastId("tok-1") }), + ); + }); }); diff --git a/clients/web/src/hooks/useProgressToasts.ts b/clients/web/src/hooks/useProgressToasts.ts index 6fbaf8be..acfe8d7e 100644 --- a/clients/web/src/hooks/useProgressToasts.ts +++ b/clients/web/src/hooks/useProgressToasts.ts @@ -86,8 +86,22 @@ export function useProgressToasts( // "Tool progress" toast from lingering into the next session, and avoids // a race where the lingering toast's `onClose` would later delete an id // from the *new* session's set and trigger a duplicate-id re-show. + // + // The bookkeeping is dropped by **swapping in a fresh Set**, not by + // calling `liveToastIds.clear()` — the distinction is load-bearing and + // invisible otherwise (#2219). `hide()` does not fire `onClose` + // synchronously; it plays the toast's exit transition first. Every + // outgoing toast's `onClose` closes over *this* Set object, so clearing + // it in place would leave those callbacks pointed at the very container + // the next session refills — and a delayed `onClose` for an id the new + // session has already re-shown (both hooks derive ids from data, so a + // reconnect replays them exactly) would delete the new entry. The stream + // would then take the `show` branch instead of `update`, Mantine would + // refuse the duplicate id, and the toast would freeze mid-progress. + // Reassigning detaches the outgoing session's callbacks onto a Set + // nothing reads again. liveToastIds.forEach((id) => notifications.hide(id)); - liveToastIds.clear(); + progressToastIdsRef.current = new Set(); }; }, [inspectorClient]); } diff --git a/clients/web/src/hooks/useTaskToasts.test.tsx b/clients/web/src/hooks/useTaskToasts.test.tsx index 581558a1..1542ab1e 100644 --- a/clients/web/src/hooks/useTaskToasts.test.tsx +++ b/clients/web/src/hooks/useTaskToasts.test.tsx @@ -83,6 +83,18 @@ function harness(client: InspectorClientEventTarget) { if (!latest) throw new Error("hook did not render"); fn(latest); }), + /** Same as `statusChange`, but from a client swapped in after mount. */ + statusChangeOn: ( + next: InspectorClientEventTarget, + taskId: string, + status: Task["status"], + ) => + dispatch(() => + next.dispatchTypedEvent("taskStatusChange", { + taskId, + task: task(taskId, status), + }), + ), swapClient: (next: InspectorClientEventTarget | null) => act(() => rerender()), unmount: () => act(() => unmount()), @@ -270,6 +282,31 @@ describe("useTaskToasts", () => { expect(notificationsMock.show).not.toHaveBeenCalled(); }); + // #2219: mirrors the `useProgressToasts` case — `hide()` defers `onClose` + // past the exit transition, and task ids are equally replayable across a + // reconnect, so the teardown swaps in a fresh Set instead of clearing the + // one the outgoing toasts' callbacks close over. + it("keeps the new session's bookkeeping when a hidden toast's onClose fires late", () => { + const h = harness(fakeClient()); + h.statusChange("t1", "working"); + const { onClose: staleOnClose } = notificationsMock.show.mock + .calls[0][0] as { onClose: () => void }; + + const next = fakeClient(); + h.swapClient(next); + h.statusChangeOn(next, "t1", "working"); + expect(notificationsMock.show).toHaveBeenCalledTimes(2); + + // The outgoing session's toast finishes its exit transition here. + staleOnClose(); + + h.statusChangeOn(next, "t1", "working"); + expect(notificationsMock.show).toHaveBeenCalledTimes(2); + expect(notificationsMock.update).toHaveBeenCalledWith( + expect.objectContaining({ id: taskToastId("t1") }), + ); + }); + it("hides the live toasts on unmount", () => { const h = harness(fakeClient()); h.statusChange("t1", "working"); diff --git a/clients/web/src/hooks/useTaskToasts.ts b/clients/web/src/hooks/useTaskToasts.ts index a48df8b7..7ea942cc 100644 --- a/clients/web/src/hooks/useTaskToasts.ts +++ b/clients/web/src/hooks/useTaskToasts.ts @@ -220,9 +220,14 @@ export function useTaskToasts( inspectorClient.removeEventListener("taskCancelled", onTaskCancelled); // Hide any still-visible task toasts on client swap so they don't linger // into the next session, then drop the bookkeeping (mirrors the progress- - // toast teardown). + // toast teardown). As there, the drop is a **swap to a fresh Set** rather + // than `liveToastIds.clear()`: `hide()` defers `onClose` until the exit + // transition ends, and those callbacks close over this Set object, so + // clearing in place would let a late `onClose` delete the *next* + // session's entry for the same task id and freeze that task's toast + // (#2219). See `useProgressToasts` for the full reasoning. liveToastIds.forEach((id) => notifications.hide(id)); - liveToastIds.clear(); + taskToastIdsRef.current = new Set(); }; }, [inspectorClient]);