From b088b94019001a7fc0bdb6d3dd9767dd180f01e9 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 29 Aug 2026 02:55:27 -0400 Subject: [PATCH] test(core): make the unsubscribe test actually verify unsubscription Copilot review round 3. "stops listening on unmount" asserted that the rendered value stopped changing after unmount, which proves nothing: React drops a store notification for an unmounted component whether or not the listener is still attached, so the test passed with the leak it existed to catch. Spy the store's add/removeEventListener and assert the exact function attached is the one detached. Verified it discriminates by deleting the cleanup, watching this test fail, and restoring. Signed-off-by: cliffhall --- .../test/core/react/useStoreSnapshot.test.tsx | 23 +++++++++++++------ 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/clients/web/src/test/core/react/useStoreSnapshot.test.tsx b/clients/web/src/test/core/react/useStoreSnapshot.test.tsx index 9b277eff..ed4a8019 100644 --- a/clients/web/src/test/core/react/useStoreSnapshot.test.tsx +++ b/clients/web/src/test/core/react/useStoreSnapshot.test.tsx @@ -1,4 +1,4 @@ -import { describe, it, expect, beforeEach } from "vitest"; +import { describe, it, expect, beforeEach, vi } from "vitest"; import { act, renderHook } from "@testing-library/react"; import { TypedEventTarget } from "@inspector/core/mcp/typedEventTarget"; import { useStoreSnapshot } from "@inspector/core/react/useStoreSnapshot"; @@ -257,16 +257,25 @@ describe("useStoreSnapshot", () => { }); it("stops listening on unmount", () => { - const { result, unmount } = renderHook(() => + // Assert on the listener, not on the rendered value. Watching + // `result.current` stay put after unmount proves nothing: React drops a + // store notification for an unmounted component either way, so that test + // passes with the listener still attached — which is the leak it was + // supposed to catch. + const addSpy = vi.spyOn(store, "addEventListener"); + const removeSpy = vi.spyOn(store, "removeEventListener"); + + const { unmount } = renderHook(() => useStoreSnapshot(store, "itemsChange", readItems, NO_ITEMS), ); - const before = result.current; + expect(addSpy).toHaveBeenCalledWith("itemsChange", expect.any(Function)); + const subscribed = addSpy.mock.calls[0]?.[1]; + unmount(); - // No act(): after unmount there is nothing to re-render, and a listener - // still attached here would be a leak rather than an update. - store.add("a"); - expect(result.current).toBe(before); + // The exact function that was attached is the one detached — a cleanup + // that removed some other listener, or none, fails here. + expect(removeSpy).toHaveBeenCalledWith("itemsChange", subscribed); }); it("unmounts cleanly with no store to unsubscribe from", () => {