Merge pull request #2267 from modelcontextprotocol/v2/fix/2219-toast-teardown-set

fix: swap the toast ref Set on teardown instead of clearing it

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(<Probe c={next} />)),
     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") }),
+    );
+  });
 });

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]);
 }

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(<Probe c={next} />)),
     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");

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]);