For issue #20, vibe-write some unit tests for handleClose().

codedread · codedread · commit 4f8b61805a6f · 2025-09-20T22:13:08.000-07:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,14 @@
 
 All notable changes to this project will be documented in this file.
 
+## [1.1.7] - 2025-??-??
+
+### Changes
+
+- Fixed [issue #20](https://github.com/codedread/spaces/issues/20): Close browser windows from the Spaces window.
+- Increased unit test coverage from 10.75% to 11.41%.
+
+
 ## [1.1.6] - 2025-09-17
 
 ### Changes
diff --git a/js/background/background.js b/js/background/background.js
@@ -330,16 +330,16 @@ async function processMessage(request, sender) {
 
         case 'closeWindow':
             windowId = cleanParameter(request.windowId);
-            if(windowId) {
-                try {
-                    const window = await chrome.windows.get(windowId);
-                    if (window) {
-                        await chrome.windows.remove(windowId);
-                        return true;
-                    }
-                } catch (error) {
-                    console.error("Error closing window:", error);
-                }
+            if (!windowId) {
+                return false;
+            }
+            
+            try {
+                await chrome.windows.get(windowId);
+                await chrome.windows.remove(windowId);
+                return true;
+            } catch (error) {
+                console.error("Error closing window:", error);
                 return false;
             }
 
diff --git a/js/spaces.js b/js/spaces.js
@@ -369,7 +369,16 @@ async function handleDelete() {
     }
 }
 
-async function handleClose() {
+/**
+ * Closes the currently selected space's window after user confirmation (if unnamed).
+ * The arguments are only for testing purposes to allow dependency injection.
+ * @param {Function} updateSpacesListFn Function to refresh the spaces list after closing
+ * @param {Function} renderSpaceDetailFn Function to render space details (called with false, false to clear)
+ * @returns {Promise<void>}
+ */
+async function handleClose(
+    updateSpacesListFn = updateSpacesList,
+    renderSpaceDetailFn = renderSpaceDetail) {
   if (!globalSelectedSpace || !globalSelectedSpace.windowId) {
     console.error("No opened window is currently selected.");
     return;
@@ -378,18 +387,18 @@ async function handleClose() {
 
   // Only show confirm if the space is unnamed
   if (!sessionId) {
-    const confirm = window.confirm(
-      "Are you sure you want to close this window?"
-    );
+    const confirm = window.confirm("Are you sure you want to close this window?");
     if (!confirm) return;
   }
 
-  chrome.runtime.sendMessage({ action: 'closeWindow', windowId });
-  await updateSpacesList();
+  const success = await chrome.runtime.sendMessage({ action: 'closeWindow', windowId });
+  if (!success) {
+    console.warn("Failed to close window - it may have already been closed");
+  }
 
-  // Clear the detail view since the closed window was selected
+  await updateSpacesListFn();
   globalSelectedSpace = null;
-  renderSpaceDetail(false, false);
+  renderSpaceDetailFn(false, false);
 }
 
 // import accepts either a newline separated list of urls or a json backup object
@@ -816,4 +825,13 @@ async function getSpacesForBackup() {
 }
 
 // Export for testing
-export { addDuplicateMetadata, getSpacesForBackup, normaliseTabUrl };
+export {
+    addDuplicateMetadata,
+    getSpacesForBackup,
+    handleClose,
+    normaliseTabUrl,
+};
+
+// Export globalSelectedSpace for testing (mutable reference)
+export function setGlobalSelectedSpace(space) { globalSelectedSpace = space; }
+export function getGlobalSelectedSpace() { return globalSelectedSpace; }
diff --git a/manifest.json b/manifest.json
@@ -1,7 +1,7 @@
 {
     "name": "Spaces",
     "description": "Intuitive tab management",
-    "version": "1.1.6",
+    "version": "1.1.7.0",
     "permissions": [
         "contextMenus",
         "favicon",
diff --git a/tests/handleClose.test.js b/tests/handleClose.test.js
@@ -0,0 +1,245 @@
+/**
+ * Unit tests for handleClose function in spaces.js
+ * Tests all code paths and edge cases for closing windows
+ */
+
+import { jest, setupTestMocks, mockConsole } from './helpers.js';
+import { handleClose, setGlobalSelectedSpace, getGlobalSelectedSpace } from '../js/spaces.js';
+
+// Setup all test mocks (includes window.confirm)
+setupTestMocks();
+
+const WINDOW_ID = 123;
+const SESSION_ID = 'test-session';
+const TEST_SPACES = {
+    NO_WINDOW: { sessionId: SESSION_ID },
+    UNNAMED: { windowId: WINDOW_ID },
+    NAMED: { windowId: WINDOW_ID, sessionId: SESSION_ID }
+};
+
+describe('handleClose Function', () => {
+    let consoleErrorSpy;
+    let consoleWarnSpy;
+    let mockUpdateSpacesList;
+    let mockRenderSpaceDetail;
+
+    beforeEach(() => {
+        jest.clearAllMocks();
+        
+        // Create simple mock functions for UI updates
+        mockUpdateSpacesList = jest.fn().mockResolvedValue();
+        mockRenderSpaceDetail = jest.fn();
+        
+        // Reset mocks
+        chrome.runtime.sendMessage.mockClear();
+        window.confirm.mockClear();
+        
+        // Mock the Chrome API responses
+        chrome.runtime.sendMessage.mockImplementation(async (request) => {
+            if (request.action === 'closeWindow') {
+                return true; // Default to success for closeWindow
+            }
+            return undefined;
+        });
+        
+        // Setup console spies
+        consoleErrorSpy = mockConsole('error');
+        consoleWarnSpy = mockConsole('warn');
+        
+        // Clear globalSelectedSpace
+        setGlobalSelectedSpace(null);
+    });
+
+    afterEach(() => {
+        consoleErrorSpy.restore();
+        consoleWarnSpy.restore();
+    });
+
+    test('should return early and log error when no space is selected', async () => {
+        // Setup: No selected space
+        setGlobalSelectedSpace(null);
+
+        // Execute
+        await handleClose(mockUpdateSpacesList, mockRenderSpaceDetail);
+
+        // Verify
+        expect(consoleErrorSpy.called).toBe(true);
+        expect(consoleErrorSpy.args[0]).toBe('No opened window is currently selected.');
+        expect(chrome.runtime.sendMessage).not.toHaveBeenCalled();
+        expect(mockUpdateSpacesList).not.toHaveBeenCalled();
+        expect(mockRenderSpaceDetail).not.toHaveBeenCalled();
+        expect(getGlobalSelectedSpace()).toBe(null); // Should remain null
+    });
+
+    test('should return early and log error when selected space has no windowId', async () => {
+        // Setup: Selected space without windowId
+        setGlobalSelectedSpace(TEST_SPACES.NO_WINDOW);
+
+        // Execute
+        await handleClose(mockUpdateSpacesList, mockRenderSpaceDetail);
+
+        // Verify
+        expect(consoleErrorSpy.called).toBe(true);
+        expect(consoleErrorSpy.args[0]).toBe('No opened window is currently selected.');
+        expect(chrome.runtime.sendMessage).not.toHaveBeenCalled();
+        expect(mockUpdateSpacesList).not.toHaveBeenCalled();
+        expect(mockRenderSpaceDetail).not.toHaveBeenCalled();
+        // globalSelectedSpace should remain unchanged since function returns early
+        expect(getGlobalSelectedSpace()).toEqual(TEST_SPACES.NO_WINDOW);
+    });
+
+    test('should show confirmation dialog for unnamed space and return early when user cancels', async () => {
+        // Setup: Unnamed space (no sessionId) and user cancels
+        setGlobalSelectedSpace(TEST_SPACES.UNNAMED);
+        window.confirm.mockReturnValue(false); // User cancels
+
+        // Execute
+        await handleClose(mockUpdateSpacesList, mockRenderSpaceDetail);
+
+        // Verify
+        expect(window.confirm).toHaveBeenCalledWith('Are you sure you want to close this window?');
+        expect(chrome.runtime.sendMessage).not.toHaveBeenCalled();
+        expect(mockUpdateSpacesList).not.toHaveBeenCalled();
+        expect(mockRenderSpaceDetail).not.toHaveBeenCalled();
+        // globalSelectedSpace should remain unchanged since function returns early
+        expect(getGlobalSelectedSpace()).toEqual(TEST_SPACES.UNNAMED);
+    });
+
+    test('should skip confirmation for named space and proceed with close', async () => {
+        // Setup: Named space (has sessionId)
+        setGlobalSelectedSpace(TEST_SPACES.NAMED);
+
+        // Execute
+        await handleClose(mockUpdateSpacesList, mockRenderSpaceDetail);
+
+        // Verify
+        expect(window.confirm).not.toHaveBeenCalled(); // No confirmation for named spaces
+        expect(chrome.runtime.sendMessage).toHaveBeenCalledWith({
+            action: 'closeWindow',
+            windowId: WINDOW_ID
+        });
+        expect(mockUpdateSpacesList).toHaveBeenCalled();
+        expect(mockRenderSpaceDetail).toHaveBeenCalledWith(false, false);
+        expect(getGlobalSelectedSpace()).toBe(null); // Selection cleared
+    });
+
+    test('should show confirmation for unnamed space and proceed when user confirms', async () => {
+        // Setup: Unnamed space and user confirms
+        setGlobalSelectedSpace(TEST_SPACES.UNNAMED);
+        window.confirm.mockReturnValue(true); // User confirms
+
+        // Execute
+        await handleClose(mockUpdateSpacesList, mockRenderSpaceDetail);
+
+        // Verify
+        expect(window.confirm).toHaveBeenCalledWith('Are you sure you want to close this window?');
+        expect(chrome.runtime.sendMessage).toHaveBeenCalledWith({
+            action: 'closeWindow',
+            windowId: WINDOW_ID
+        });
+        expect(mockUpdateSpacesList).toHaveBeenCalled();
+        expect(mockRenderSpaceDetail).toHaveBeenCalledWith(false, false);
+        expect(getGlobalSelectedSpace()).toBe(null); // Selection cleared
+    });
+
+    test('should handle successful window close without warning', async () => {
+        // Setup: Successful close
+        setGlobalSelectedSpace(TEST_SPACES.NAMED);
+
+        // Execute
+        await handleClose(mockUpdateSpacesList, mockRenderSpaceDetail);
+
+        // Verify
+        expect(chrome.runtime.sendMessage).toHaveBeenCalledWith({
+            action: 'closeWindow',
+            windowId: WINDOW_ID
+        });
+        expect(consoleWarnSpy.called).toBe(false); // No warning for success
+        expect(mockUpdateSpacesList).toHaveBeenCalled();
+        expect(mockRenderSpaceDetail).toHaveBeenCalledWith(false, false);
+        expect(getGlobalSelectedSpace()).toBe(null);
+    });
+
+    test('should handle failed window close with warning and still update UI', async () => {
+        // Setup: Failed close - override the mock for this specific test
+        setGlobalSelectedSpace(TEST_SPACES.NAMED);
+        
+        chrome.runtime.sendMessage.mockImplementation(async (request) => {
+            if (request.action === 'closeWindow') {
+                return false; // Return false for this test case
+            }
+            return undefined;
+        });
+
+        // Execute
+        await handleClose(mockUpdateSpacesList, mockRenderSpaceDetail);
+
+        // Verify
+        expect(chrome.runtime.sendMessage).toHaveBeenCalledWith({
+            action: 'closeWindow',
+            windowId: WINDOW_ID
+        });
+        expect(consoleWarnSpy.called).toBe(true);
+        expect(consoleWarnSpy.args[0]).toBe('Failed to close window - it may have already been closed');
+        expect(mockUpdateSpacesList).toHaveBeenCalled();
+        expect(mockRenderSpaceDetail).toHaveBeenCalledWith(false, false);
+        expect(getGlobalSelectedSpace()).toBe(null); // Still clears selection
+    });
+
+    test('should handle chrome.runtime.sendMessage rejection and still update UI', async () => {
+        // Setup: Message sending throws error
+        setGlobalSelectedSpace(TEST_SPACES.NAMED);
+        const mockError = new Error('Chrome API error');
+        
+        chrome.runtime.sendMessage.mockImplementation(async (request) => {
+            if (request.action === 'closeWindow') {
+                throw mockError; // Throw error for closeWindow
+            }
+            return undefined;
+        });
+
+        // Execute - this should throw since handleClose doesn't catch promise rejections
+        // Execute and verify it throws
+        await expect(handleClose(mockUpdateSpacesList, mockRenderSpaceDetail)).rejects.toThrow('Chrome API error');
+
+        // Verify the message was attempted
+        expect(chrome.runtime.sendMessage).toHaveBeenCalledWith({
+            action: 'closeWindow',
+            windowId: WINDOW_ID
+        });
+        // The UI functions should not be called since function threw before reaching them
+        expect(mockUpdateSpacesList).not.toHaveBeenCalled();
+        expect(mockRenderSpaceDetail).not.toHaveBeenCalled();
+        // The globalSelectedSpace should remain unchanged since function threw before clearing it
+        expect(getGlobalSelectedSpace()).toEqual(TEST_SPACES.NAMED);
+    });
+
+    test('should preserve original globalSelectedSpace values during execution', async () => {
+        // Setup: Verify that windowId and sessionId are extracted before any async operations
+        setGlobalSelectedSpace(TEST_SPACES.NAMED);
+        
+        chrome.runtime.sendMessage.mockImplementation(async (request) => {
+            if (request.action === 'closeWindow') {
+                // Simulate the space being modified during the async operation
+                const currentSpace = getGlobalSelectedSpace();
+                if (currentSpace) {
+                    currentSpace.windowId = 'MODIFIED';
+                }
+                return true;
+            }
+            return undefined;
+        });
+
+        // Execute
+        await handleClose(mockUpdateSpacesList, mockRenderSpaceDetail);
+
+        // Verify that the original windowId was used in the message
+        expect(chrome.runtime.sendMessage).toHaveBeenCalledWith({
+            action: 'closeWindow',
+            windowId: WINDOW_ID // Original value, not 'MODIFIED'
+        });
+        expect(mockUpdateSpacesList).toHaveBeenCalled();
+        expect(mockRenderSpaceDetail).toHaveBeenCalledWith(false, false);
+        expect(getGlobalSelectedSpace()).toBe(null); // Still cleared at end
+    });
+});
diff --git a/tests/helpers.js b/tests/helpers.js
@@ -60,6 +60,7 @@ export const setupDOMMocks = () => {
             reload: jest.fn(),
         },
         close: jest.fn(),
+        confirm: jest.fn(),
     };
 };
 

Original file line number	Diff line number	Diff line change
`@@ -1,7 +1,7 @@`
`1`	`1`	`{`
`2`	`2`	`"name": "Spaces",`
`3`	`3`	`"description": "Intuitive tab management",`
`4`		`- "version": "1.1.6",`
	`4`	`+ "version": "1.1.7.0",`
`5`	`5`	`"permissions": [`
`6`	`6`	`"contextMenus",`
`7`	`7`	`"favicon",`