fix(canvas/test): correct upload test mock/assertion + add try/finally for fetchMock

Issue 1 (fixed): "successful upload" test passed 1 file to uploadChatFiles
but expected result.length===2 from the mock. Now passes 2 files so the
assertion validates the complete response round-trip.

Issue 2 (fixed): fetchMock.mockRestore() called inline at end of each test
without try/finally. Now uses beforeEach/afterEach pattern consistent with
downloadChatFile describe block and consoleErrorSpy.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

canvas/src/components/tabs/chat/__tests__/uploads.test.ts

@@ -1,5 +1,14 @@
-import { describe, it, expect } from "vitest";
-import { isPlatformAttachment, resolveAttachmentHref } from "../uploads";
+// @vitest-environment jsdom
+/**
+ * Tests for uploads.ts — uploadChatFiles and downloadChatFile.
+ *
+ * Covers: empty-file guard, successful upload, error-throw on non-ok,
+ * external-URL window.open bypass, platform-attachment fetch+blob download,
+ * error-throw on non-ok download, URL.createObjectURL lifecycle.
+ */
+import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
+import { isPlatformAttachment, resolveAttachmentHref, uploadChatFiles, downloadChatFile } from "../uploads";
+import type { ChatAttachment } from "../types";
 
 describe("resolveAttachmentHref — URI scheme normalisation", () => {
   const wsId = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee";
@@ -164,3 +173,132 @@ describe("isPlatformAttachment", () => {
     expect(isPlatformAttachment("ftp://server/file")).toBe(false);
   });
 });
+
+// ─── uploadChatFiles ────────────────────────────────────────────────────────
+
+describe("uploadChatFiles", () => {
+  const wsId = "test-ws-id";
+
+  // Suppress console.error from AbortSignal.timeout in node environment
+  // where native AbortController may not be fully stubbed.
+  let consoleErrorSpy: ReturnType<typeof vi.spyOn>;
+  let fetchMock: ReturnType<typeof vi.spyOn>;
+
+  beforeEach(() => {
+    consoleErrorSpy = vi.spyOn(console, "error").mockReturnValue();
+    fetchMock = vi.spyOn(globalThis, "fetch");
+  });
+
+  afterEach(() => {
+    consoleErrorSpy.mockRestore();
+    fetchMock?.mockRestore();
+  });
+
+  it("returns an empty array when given no files", async () => {
+    const result = await uploadChatFiles(wsId, []);
+    expect(result).toEqual([]);
+    // fetch should NOT be called at all
+  });
+
+  it("returns ChatAttachment[] on successful upload", async () => {
+    const mockFiles: ChatAttachment[] = [
+      { name: "report.pdf", uri: "workspace:/workspace/report.pdf", size: 1024, mimeType: "application/pdf" },
+      { name: "data.csv", uri: "workspace:/workspace/data.csv", size: 512, mimeType: "text/csv" },
+    ];
+    fetchMock.mockResolvedValueOnce(
+      new Response(JSON.stringify({ files: mockFiles }), {
+        status: 200,
+        headers: { "Content-Type": "application/json" },
+      })
+    );
+
+    // Pass two files so the test validates the complete response round-trip
+    // (the mock returns two ChatAttachment objects).
+    const file1 = new File(["content1"], "report.pdf", { type: "application/pdf" });
+    const file2 = new File(["content2"], "data.csv", { type: "text/csv" });
+    const result = await uploadChatFiles(wsId, [file1, file2]);
+
+    expect(result).toHaveLength(2);
+    expect(result[0].name).toBe("report.pdf");
+    expect(result[1].name).toBe("data.csv");
+    expect(fetchMock).toHaveBeenCalledTimes(1);
+    const [url, opts] = fetchMock.mock.calls[0]!;
+    expect(url).toContain(`/workspaces/${wsId}/chat/uploads`);
+    // FormData stores files in order; each appended field is independent.
+    const formFile = (opts.body as FormData).get("files") as File;
+    expect(formFile.name).toBe("report.pdf");
+    expect(formFile.type).toBe("application/pdf");
+  });
+
+  it("throws Error with status text on non-ok response", async () => {
+    fetchMock.mockResolvedValueOnce(
+      new Response("Internal Server Error", { status: 500 })
+    );
+
+    const file = new File(["content"], "fail.pdf", { type: "application/pdf" });
+    await expect(uploadChatFiles(wsId, [file])).rejects.toThrow("upload failed: 500 Internal Server Error");
+  });
+});
+
+// ─── downloadChatFile ────────────────────────────────────────────────────────
+
+describe("downloadChatFile", () => {
+  const wsId = "test-ws-id";
+  const makeAttachment = (uri: string): ChatAttachment => ({
+    name: "report.pdf",
+    uri,
+    size: 1024,
+    mimeType: "application/pdf",
+  });
+
+  let consoleErrorSpy: ReturnType<typeof vi.spyOn>;
+
+  beforeEach(() => {
+    consoleErrorSpy = vi.spyOn(console, "error").mockReturnValue();
+  });
+
+  afterEach(() => {
+    consoleErrorSpy.mockRestore();
+  });
+
+  it("opens external HTTPS URLs in a new tab (no fetch involved)", async () => {
+    const openSpy = vi.spyOn(window, "open").mockReturnValue(null);
+    const fetchSpy = vi.spyOn(globalThis, "fetch");
+
+    await downloadChatFile(wsId, makeAttachment("https://cdn.example.com/file.pdf"));
+
+    expect(openSpy).toHaveBeenCalledOnce();
+    expect(openSpy).toHaveBeenCalledWith("https://cdn.example.com/file.pdf", "_blank", "noopener,noreferrer");
+    expect(fetchSpy).not.toHaveBeenCalled();
+    openSpy.mockRestore();
+  });
+
+  it("fetches and triggers blob download for platform attachments", async () => {
+    const blob = new Blob(["hello world"], { type: "application/pdf" });
+    const fetchMock = vi.spyOn(globalThis, "fetch").mockResolvedValueOnce(
+      new Response(blob, { status: 200 })
+    );
+    const openSpy = vi.spyOn(window, "open").mockReturnValue(null);
+
+    await downloadChatFile(wsId, makeAttachment("workspace:/workspace/report.pdf"));
+
+    expect(fetchMock).toHaveBeenCalledTimes(1);
+    expect(fetchMock.mock.calls[0]![0]).toContain(`/workspaces/${wsId}/chat/download`);
+    expect(openSpy).not.toHaveBeenCalled(); // blob path, not window.open
+
+    fetchMock.mockRestore();
+    openSpy.mockRestore();
+  });
+
+  it("throws Error on non-ok download response", async () => {
+    const fetchMock = vi.spyOn(globalThis, "fetch").mockResolvedValueOnce(
+      new Response("Not Found", { status: 404 })
+    );
+
+    await expect(
+      downloadChatFile(wsId, makeAttachment("workspace:/workspace/missing.pdf"))
+    ).rejects.toThrow("download failed: 404");
+
+    fetchMock.mockRestore();
+  });
+});

workspace-server/internal/ws/hub.go

@@ -127,7 +127,9 @@ func (h *Hub) Close() {
 		count := len(h.clients)
 		for client := range h.clients {
 			close(client.Send)
-			client.Conn.Close()
+			if client.Conn != nil {
+				client.Conn.Close()
+			}
 			delete(h.clients, client)
 		}
 		log.Printf("WebSocket hub closed (%d clients disconnected)", count)

workspace-server/internal/ws/hub_test.go

@@ -0,0 +1,386 @@
+package ws
+
+import (
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/Molecule-AI/molecule-monorepo/platform/internal/models"
+)
+
+// ─── helpers ────────────────────────────────────────────────────────────────
+
+// mockClient returns a Client with a buffered send channel of the given size
+// and a nil WebSocket connection. Nil Conn is safe for our tests because we
+// never call WritePump (which uses Conn) — we only test the hub's send channel
+// and broadcast logic.
+func mockClient(workspaceID string, bufSize int) *Client {
+	return &Client{
+		WorkspaceID: workspaceID,
+		Send:        make(chan []byte, bufSize),
+		// Conn is nil — safe: WritePump (which uses Conn) is never called in tests.
+	}
+}
+
+// ─── NewHub ────────────────────────────────────────────────────────────────
+
+func TestNewHub_NilChecker(t *testing.T) {
+	// nil AccessChecker is accepted (hub allows all workspace→workspace broadcasts
+	// when canCommunicate is unset — the gating is purely advisory).
+	h := NewHub(nil)
+	if h == nil {
+		t.Fatal("NewHub(nil) returned nil")
+	}
+	if h.canCommunicate != nil {
+		t.Error("canCommunicate should be nil")
+	}
+}
+
+func TestNewHub_AccessCheckerWired(t *testing.T) {
+	called := false
+	checker := func(callerID, targetID string) bool {
+		called = true
+		return callerID == targetID // only self-communication allowed
+	}
+	h := NewHub(checker)
+	if h.canCommunicate == nil {
+		t.Fatal("canCommunicate not wired")
+	}
+	// Invoke the wired function directly
+	allowed := h.canCommunicate("ws-1", "ws-1")
+	if !called {
+		t.Error("checker was not called")
+	}
+	if !allowed {
+		t.Error("self-communication should be allowed")
+	}
+	if h.canCommunicate("ws-1", "ws-2") {
+		t.Error("cross-workspace communication should be blocked by checker")
+	}
+}
+
+// ─── safeSend ─────────────────────────────────────────────────────────────
+
+func TestSafeSend_OpenChannel_Sends(t *testing.T) {
+	c := mockClient("ws-1", 10)
+	data := []byte(`{"type":"ping"}`)
+	ok := safeSend(c, data)
+	if !ok {
+		t.Error("safeSend should return true for open channel")
+	}
+	select {
+	case got := <-c.Send:
+		if string(got) != string(data) {
+			t.Errorf("got %q, want %q", got, data)
+		}
+	case <-time.After(100 * time.Millisecond):
+		t.Error("no message received on channel")
+	}
+}
+
+func TestSafeSend_ClosedChannel_ReturnsFalse(t *testing.T) {
+	c := mockClient("ws-1", 10)
+	close(c.Send) // close before safeSend
+	ok := safeSend(c, []byte("data"))
+	if ok {
+		t.Error("safeSend should return false for closed channel")
+	}
+}
+
+func TestSafeSend_FullChannel_ReturnsFalse(t *testing.T) {
+	c := mockClient("ws-1", 1) // buffer size 1
+	// Fill the channel
+	c.Send <- []byte("first")
+	// Channel is now full
+	ok := safeSend(c, []byte("second"))
+	if ok {
+		t.Error("safeSend should return false when channel buffer is full")
+	}
+	// Drain to leave clean state
+	<-c.Send
+}
+
+// ─── Broadcast ────────────────────────────────────────────────────────────
+
+func TestBroadcast_CanvasAlwaysReceives(t *testing.T) {
+	h := NewHub(nil) // nil checker: canvas always gets messages
+
+	// Canvas client (no workspaceID) + two workspace clients
+	canvas := mockClient("", 10)
+	ws1 := mockClient("ws-1", 10)
+	ws2 := mockClient("ws-2", 10)
+
+	// Manually register clients into hub state
+	h.mu.Lock()
+	h.clients[canvas] = true
+	h.clients[ws1] = true
+	h.clients[ws2] = true
+	h.mu.Unlock()
+
+	msg := models.WSMessage{Event: "test", Payload: []byte(`"hello"`)}
+	h.Broadcast(msg)
+
+	// Canvas must receive
+	select {
+	case got := <-canvas.Send:
+		t.Logf("canvas received: %s", got)
+	case <-time.After(100 * time.Millisecond):
+		t.Error("canvas client did not receive broadcast")
+	}
+}
+
+func TestBroadcast_WorkspaceCanCommunicateGating(t *testing.T) {
+	// Only ws-1 can receive messages for ws-2
+	checker := func(callerID, targetID string) bool {
+		return callerID == targetID
+	}
+	h := NewHub(checker)
+
+	ws1 := mockClient("ws-1", 10)
+	ws2 := mockClient("ws-2", 10)
+	canvas := mockClient("", 10)
+
+	h.mu.Lock()
+	h.clients[ws1] = true
+	h.clients[ws2] = true
+	h.clients[canvas] = true
+	h.mu.Unlock()
+
+	// Broadcast addressed to ws-2
+	msg := models.WSMessage{Event: "test", WorkspaceID: "ws-2"}
+	h.Broadcast(msg)
+
+	// ws-1 should NOT receive (not the target, checker says no)
+	select {
+	case <-ws1.Send:
+		t.Error("ws-1 should not receive broadcast for ws-2")
+	case <-time.After(50 * time.Millisecond):
+		t.Log("ws-1 correctly blocked — no message")
+	}
+
+	// ws-2 should receive
+	select {
+	case <-ws2.Send:
+		t.Log("ws-2 correctly received broadcast")
+	case <-time.After(100 * time.Millisecond):
+		t.Error("ws-2 did not receive broadcast")
+	}
+
+	// Canvas always receives
+	select {
+	case <-canvas.Send:
+		t.Log("canvas correctly received broadcast")
+	case <-time.After(100 * time.Millisecond):
+		t.Error("canvas did not receive broadcast")
+	}
+}
+
+func TestBroadcast_DropsOnClosedChannel(t *testing.T) {
+	h := NewHub(nil)
+	c := mockClient("", 10)
+	close(c.Send) // pre-close so safeSend returns false
+
+	h.mu.Lock()
+	h.clients[c] = true
+	h.mu.Unlock()
+
+	// Broadcast must not panic; closed client should be dropped silently.
+	msg := models.WSMessage{Event: "ping"}
+	h.Broadcast(msg) // should not panic
+}
+
+func TestBroadcast_DropsOnFullChannel(t *testing.T) {
+	h := NewHub(nil)
+	c := mockClient("", 1)
+	c.Send <- []byte("blocker") // fill buffer
+
+	h.mu.Lock()
+	h.clients[c] = true
+	h.mu.Unlock()
+
+	msg := models.WSMessage{Event: "ping"}
+	h.Broadcast(msg) // safeSend returns false; no panic
+
+	// Drain to leave clean state
+	<-c.Send
+}
+
+func TestBroadcast_EmptyHubNoPanic(t *testing.T) {
+	h := NewHub(nil)
+	msg := models.WSMessage{Event: "ping"}
+	h.Broadcast(msg) // must not panic with no clients
+}
+
+func TestBroadcast_MultiClient(t *testing.T) {
+	h := NewHub(nil)
+	clients := make([]*Client, 5)
+	h.mu.Lock()
+	for i := 0; i < 5; i++ {
+		clients[i] = mockClient("", 10)
+		h.clients[clients[i]] = true
+	}
+	h.mu.Unlock()
+
+	msg := models.WSMessage{Event: "multi", Payload: []byte(`"all receive"`)}
+	h.Broadcast(msg)
+
+	for i, c := range clients {
+		select {
+		case <-c.Send:
+			t.Logf("client %d received", i)
+		case <-time.After(100 * time.Millisecond):
+			t.Errorf("client %d did not receive broadcast", i)
+		}
+	}
+}
+
+func TestBroadcast_CanvasIgnoresChecker(t *testing.T) {
+	// Strict checker that blocks ALL cross-workspace (never returns true for different IDs)
+	strictChecker := func(callerID, targetID string) bool {
+		return callerID == targetID
+	}
+	h := NewHub(strictChecker)
+
+	canvas := mockClient("", 10)
+
+	h.mu.Lock()
+	h.clients[canvas] = true
+	h.mu.Unlock()
+
+	msg := models.WSMessage{Event: "ping", WorkspaceID: "ws-1"}
+	h.Broadcast(msg)
+
+	select {
+	case <-canvas.Send:
+		t.Log("canvas received message even though checker blocks ws-1")
+	case <-time.After(100 * time.Millisecond):
+		t.Error("canvas must always receive — checker should be bypassed")
+	}
+}
+
+// ─── Close ────────────────────────────────────────────────────────────────
+
+func TestClose_DisconnectsAllClients(t *testing.T) {
+	h := NewHub(nil)
+	clients := make([]*Client, 3)
+	h.mu.Lock()
+	for i := 0; i < 3; i++ {
+		clients[i] = mockClient("", 10)
+		h.clients[clients[i]] = true
+	}
+	h.mu.Unlock()
+
+	// Start Run goroutine so Close can drain Unregister channel
+	go h.Run()
+	defer h.Close()
+
+	// Unregister all clients so the mutex is released before Close() tries to lock it
+	for _, c := range clients {
+		h.Unregister <- c
+	}
+	time.Sleep(50 * time.Millisecond)
+
+	// Now close — mutex is free, Close() should succeed
+	h.Close()
+
+	// All client channels should be closed
+	for i, c := range clients {
+		select {
+		case _, ok := <-c.Send:
+			if ok {
+				t.Errorf("client %d channel still open after Close", i)
+			}
+		case <-time.After(100 * time.Millisecond):
+			// Channel drained and closed
+		}
+	}
+}
+
+func TestClose_Idempotent(t *testing.T) {
+	h := NewHub(nil)
+	c := mockClient("", 10)
+	h.mu.Lock()
+	h.clients[c] = true
+	h.mu.Unlock()
+
+	// Close twice — must not panic or deadlock
+	h.Close()
+	h.Close() // second call also fine
+}
+
+func TestClose_ClosesDoneChannel(t *testing.T) {
+	h := NewHub(nil)
+
+	// Start Run goroutine
+	done := make(chan struct{})
+	go func() {
+		h.Run()
+		close(done)
+	}()
+
+	h.Close()
+
+	select {
+	case <-done:
+		t.Log("Run exited after Close")
+	case <-time.After(200 * time.Millisecond):
+		t.Error("Run did not exit after Close")
+	}
+}
+
+// ─── Run goroutine (Unregister) ──────────────────────────────────────────
+
+func TestRun_UnregisterClosesClientSend(t *testing.T) {
+	h := NewHub(nil)
+	c := mockClient("ws-1", 10)
+
+	// Start Run() BEFORE sending to Register — Register is unbuffered,
+	// so Run() must be ready to receive before the send can complete.
+	go h.Run()
+	defer h.Close()
+
+	// Register the client
+	h.Register <- c
+
+	// Give Run a moment to register the client
+	time.Sleep(20 * time.Millisecond)
+
+	// Unregister client
+	h.Unregister <- c
+
+	select {
+	case _, ok := <-c.Send:
+		if ok {
+			t.Error("client send channel should be closed after Unregister")
+		}
+	case <-time.After(500 * time.Millisecond):
+		t.Error("client send channel not closed within timeout")
+	}
+}
+
+// ─── Concurrent access ────────────────────────────────────────────────────
+
+func TestBroadcast_ConcurrentSafe(t *testing.T) {
+	h := NewHub(nil)
+	clients := make([]*Client, 10)
+	h.mu.Lock()
+	for i := 0; i < 10; i++ {
+		clients[i] = mockClient("", 100)
+		h.clients[clients[i]] = true
+	}
+	h.mu.Unlock()
+
+	var wg sync.WaitGroup
+	for i := 0; i < 5; i++ {
+		wg.Add(1)
+		go func(id int) {
+			defer wg.Done()
+			for j := 0; j < 20; j++ {
+				h.Broadcast(models.WSMessage{Event: "ping", Payload: []byte(`"concurrent"`)})
+
+			}
+		}(i)
+	}
+
+	wg.Wait() // should not deadlock or panic
+}