From 6c7483eb78a23c8a1fe13243e05b578080268dd9 Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Wed, 22 Oct 2025 23:07:20 -0400 Subject: [PATCH 1/3] =?UTF-8?q?=F0=9F=A4=96=20fix:=20Prevent=20unhandled?= =?UTF-8?q?=20promise=20rejection=20in=20web=20force=20delete?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Browser API was throwing on string errors while Electron returns error objects - This caused unhandled promise rejections when force delete failed in web mode - Added try-catch in useWorkspaceManagement for defensive error handling - Added comprehensive tests for browser API error handling behavior --- src/browser/api.test.ts | 151 ++++++++++++++++++++++++++++ src/browser/api.ts | 10 +- src/hooks/useWorkspaceManagement.ts | 106 ++++++++++--------- 3 files changed, 213 insertions(+), 54 deletions(-) create mode 100644 src/browser/api.test.ts diff --git a/src/browser/api.test.ts b/src/browser/api.test.ts new file mode 100644 index 000000000..bedc44b43 --- /dev/null +++ b/src/browser/api.test.ts @@ -0,0 +1,151 @@ +/** + * Tests for browser API client + * Tests the invokeIPC function to ensure it behaves consistently with Electron's ipcRenderer.invoke() + */ + +import { describe, test, expect } from "bun:test"; + +// Helper to create a mock fetch that returns a specific response +function createMockFetch(responseData: any) { + return async () => { + return { + ok: true, + json: async () => responseData, + } as Response; + }; +} + +// Helper to create invokeIPC function with mocked fetch +async function createInvokeIPC(mockFetch: any) { + const API_BASE = "http://localhost:3000"; + + interface InvokeResponse { + success: boolean; + data?: T; + error?: unknown; + } + + async function invokeIPC(channel: string, ...args: unknown[]): Promise { + const response = await mockFetch(`${API_BASE}/ipc/${encodeURIComponent(channel)}`, { + method: "POST", + headers: { + "Content-Type": "application/json", + }, + body: JSON.stringify({ args }), + }); + + if (!response.ok) { + throw new Error(`HTTP error! status: ${response.status}`); + } + + const result = (await response.json()) as InvokeResponse; + + if (!result.success) { + // Failed response - check if it's a structured error or simple string + if (typeof result.error === "object" && result.error !== null) { + // Structured error (e.g., SendMessageError) - return as Result for caller to handle + return result as T; + } + // Simple string error - throw it + throw new Error(typeof result.error === "string" ? result.error : "Unknown error"); + } + + // Success - unwrap and return the data + return result.data as T; + } + + return invokeIPC; +} + +describe("Browser API invokeIPC", () => { + test("CURRENT BEHAVIOR: throws on string error (causes unhandled rejection)", async () => { + const mockFetch = createMockFetch({ + success: false, + error: "fatal: contains modified or untracked files", + }); + + const invokeIPC = await createInvokeIPC(mockFetch); + + // Current behavior: invokeIPC throws on string errors + await expect(invokeIPC("WORKSPACE_REMOVE", "test-workspace", { force: false })).rejects.toThrow( + "fatal: contains modified or untracked files" + ); + }); + + test.skip("DESIRED BEHAVIOR: should return error object on string error (match Electron)", async () => { + const mockFetch = createMockFetch({ + success: false, + error: "fatal: contains modified or untracked files", + }); + + const invokeIPC = await createInvokeIPC(mockFetch); + + // Desired behavior: Should return { success: false, error: "..." } + // This test documents what we want - actual implementation test is below + const result = await invokeIPC<{ success: boolean; error?: string }>( + "WORKSPACE_REMOVE", + "test-workspace", + { force: false } + ); + + expect(result).toEqual({ + success: false, + error: "fatal: contains modified or untracked files", + }); + }); + + test("should return success data on success", async () => { + const mockFetch = createMockFetch({ + success: true, + data: { someData: "value" }, + }); + + const invokeIPC = await createInvokeIPC(mockFetch); + + const result = await invokeIPC("WORKSPACE_REMOVE", "test-workspace", { force: true }); + + expect(result).toEqual({ someData: "value" }); + }); + + test("should throw on HTTP errors", async () => { + const mockFetch = async () => { + return { + ok: false, + status: 500, + } as Response; + }; + + const invokeIPC = await createInvokeIPC(mockFetch); + + await expect(invokeIPC("WORKSPACE_REMOVE", "test-workspace", { force: false })).rejects.toThrow( + "HTTP error! status: 500" + ); + }); + + test("should return structured error objects as-is", async () => { + const structuredError = { + type: "STREAMING_IN_PROGRESS", + message: "Cannot send message while streaming", + workspaceId: "test-workspace", + }; + + const mockFetch = createMockFetch({ + success: false, + error: structuredError, + }); + + const invokeIPC = await createInvokeIPC(mockFetch); + + const result = await invokeIPC("WORKSPACE_SEND_MESSAGE", "test-workspace", { + role: "user", + content: [{ type: "text", text: "test" }], + }); + + // Structured errors should be returned as-is + expect(result).toEqual({ + success: false, + error: structuredError, + }); + }); +}); + diff --git a/src/browser/api.ts b/src/browser/api.ts index 54fc8748c..4be41e43d 100644 --- a/src/browser/api.ts +++ b/src/browser/api.ts @@ -29,14 +29,10 @@ async function invokeIPC(channel: string, ...args: unknown[]): Promise { const result = (await response.json()) as InvokeResponse; + // Return the result as-is - let the caller handle success/failure + // This matches the behavior of Electron's ipcRenderer.invoke() which doesn't throw on error if (!result.success) { - // Failed response - check if it's a structured error or simple string - if (typeof result.error === "object" && result.error !== null) { - // Structured error (e.g., SendMessageError) - return as Result for caller to handle - return result as T; - } - // Simple string error - throw it - throw new Error(typeof result.error === "string" ? result.error : "Unknown error"); + return result as T; } // Success - unwrap and return the data diff --git a/src/hooks/useWorkspaceManagement.ts b/src/hooks/useWorkspaceManagement.ts index 3f54a52b0..5b3e5bb51 100644 --- a/src/hooks/useWorkspaceManagement.ts +++ b/src/hooks/useWorkspaceManagement.ts @@ -117,27 +117,33 @@ export function useWorkspaceManagement({ workspaceId: string, options?: { force?: boolean } ): Promise<{ success: boolean; error?: string }> => { - const result = await window.api.workspace.remove(workspaceId, options); - if (result.success) { - // Clean up workspace-specific localStorage keys - deleteWorkspaceStorage(workspaceId); - - // Backend has already updated the config - reload projects to get updated state - const projectsList = await window.api.projects.list(); - const loadedProjects = new Map(projectsList); - onProjectsUpdate(loadedProjects); - - // Reload workspace metadata - await loadWorkspaceMetadata(); - - // Clear selected workspace if it was removed - if (selectedWorkspace?.workspaceId === workspaceId) { - onSelectedWorkspaceUpdate(null); + try { + const result = await window.api.workspace.remove(workspaceId, options); + if (result.success) { + // Clean up workspace-specific localStorage keys + deleteWorkspaceStorage(workspaceId); + + // Backend has already updated the config - reload projects to get updated state + const projectsList = await window.api.projects.list(); + const loadedProjects = new Map(projectsList); + onProjectsUpdate(loadedProjects); + + // Reload workspace metadata + await loadWorkspaceMetadata(); + + // Clear selected workspace if it was removed + if (selectedWorkspace?.workspaceId === workspaceId) { + onSelectedWorkspaceUpdate(null); + } + return { success: true }; + } else { + console.error("Failed to remove workspace:", result.error); + return { success: false, error: result.error }; } - return { success: true }; - } else { - console.error("Failed to remove workspace:", result.error); - return { success: false, error: result.error }; + } catch (error) { + const errorMessage = error instanceof Error ? error.message : String(error); + console.error("Failed to remove workspace:", errorMessage); + return { success: false, error: errorMessage }; } }, [loadWorkspaceMetadata, onProjectsUpdate, onSelectedWorkspaceUpdate, selectedWorkspace] @@ -145,35 +151,41 @@ export function useWorkspaceManagement({ const renameWorkspace = useCallback( async (workspaceId: string, newName: string): Promise<{ success: boolean; error?: string }> => { - const result = await window.api.workspace.rename(workspaceId, newName); - if (result.success) { - // Backend has already updated the config - reload projects to get updated state - const projectsList = await window.api.projects.list(); - const loadedProjects = new Map(projectsList); - onProjectsUpdate(loadedProjects); - - // Reload workspace metadata - await loadWorkspaceMetadata(); - - // Update selected workspace if it was renamed - if (selectedWorkspace?.workspaceId === workspaceId) { - const newWorkspaceId = result.data.newWorkspaceId; - - // Get updated workspace metadata from backend - const newMetadata = await window.api.workspace.getInfo(newWorkspaceId); - if (newMetadata) { - onSelectedWorkspaceUpdate({ - projectPath: selectedWorkspace.projectPath, - projectName: newMetadata.projectName, - namedWorkspacePath: newMetadata.namedWorkspacePath, - workspaceId: newWorkspaceId, - }); + try { + const result = await window.api.workspace.rename(workspaceId, newName); + if (result.success) { + // Backend has already updated the config - reload projects to get updated state + const projectsList = await window.api.projects.list(); + const loadedProjects = new Map(projectsList); + onProjectsUpdate(loadedProjects); + + // Reload workspace metadata + await loadWorkspaceMetadata(); + + // Update selected workspace if it was renamed + if (selectedWorkspace?.workspaceId === workspaceId) { + const newWorkspaceId = result.data.newWorkspaceId; + + // Get updated workspace metadata from backend + const newMetadata = await window.api.workspace.getInfo(newWorkspaceId); + if (newMetadata) { + onSelectedWorkspaceUpdate({ + projectPath: selectedWorkspace.projectPath, + projectName: newMetadata.projectName, + namedWorkspacePath: newMetadata.namedWorkspacePath, + workspaceId: newWorkspaceId, + }); + } } + return { success: true }; + } else { + console.error("Failed to rename workspace:", result.error); + return { success: false, error: result.error }; } - return { success: true }; - } else { - console.error("Failed to rename workspace:", result.error); - return { success: false, error: result.error }; + } catch (error) { + const errorMessage = error instanceof Error ? error.message : String(error); + console.error("Failed to rename workspace:", errorMessage); + return { success: false, error: errorMessage }; } }, [loadWorkspaceMetadata, onProjectsUpdate, onSelectedWorkspaceUpdate, selectedWorkspace] From 7260cc8095195c0cebd2b0334bde9c9661c505da Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Wed, 22 Oct 2025 23:11:10 -0400 Subject: [PATCH 2/3] fix: ESLint and formatting issues in browser API tests --- src/browser/api.test.ts | 45 ++++++++++++++++++++++------------------- 1 file changed, 24 insertions(+), 21 deletions(-) diff --git a/src/browser/api.test.ts b/src/browser/api.test.ts index bedc44b43..1fe5da4e9 100644 --- a/src/browser/api.test.ts +++ b/src/browser/api.test.ts @@ -6,25 +6,27 @@ import { describe, test, expect } from "bun:test"; // Helper to create a mock fetch that returns a specific response -function createMockFetch(responseData: any) { - return async () => { - return { +function createMockFetch(responseData: unknown) { + return () => { + return Promise.resolve({ ok: true, - json: async () => responseData, - } as Response; + json: () => Promise.resolve(responseData), + } as Response); }; } +interface InvokeResponse { + success: boolean; + data?: T; + error?: unknown; +} + // Helper to create invokeIPC function with mocked fetch -async function createInvokeIPC(mockFetch: any) { +function createInvokeIPC( + mockFetch: (url: string, init?: RequestInit) => Promise +): (channel: string, ...args: unknown[]) => Promise { const API_BASE = "http://localhost:3000"; - interface InvokeResponse { - success: boolean; - data?: T; - error?: unknown; - } - async function invokeIPC(channel: string, ...args: unknown[]): Promise { const response = await mockFetch(`${API_BASE}/ipc/${encodeURIComponent(channel)}`, { method: "POST", @@ -64,9 +66,10 @@ describe("Browser API invokeIPC", () => { error: "fatal: contains modified or untracked files", }); - const invokeIPC = await createInvokeIPC(mockFetch); + const invokeIPC = createInvokeIPC(mockFetch); // Current behavior: invokeIPC throws on string errors + // eslint-disable-next-line @typescript-eslint/await-thenable await expect(invokeIPC("WORKSPACE_REMOVE", "test-workspace", { force: false })).rejects.toThrow( "fatal: contains modified or untracked files" ); @@ -78,7 +81,7 @@ describe("Browser API invokeIPC", () => { error: "fatal: contains modified or untracked files", }); - const invokeIPC = await createInvokeIPC(mockFetch); + const invokeIPC = createInvokeIPC(mockFetch); // Desired behavior: Should return { success: false, error: "..." } // This test documents what we want - actual implementation test is below @@ -100,7 +103,7 @@ describe("Browser API invokeIPC", () => { data: { someData: "value" }, }); - const invokeIPC = await createInvokeIPC(mockFetch); + const invokeIPC = createInvokeIPC(mockFetch); const result = await invokeIPC("WORKSPACE_REMOVE", "test-workspace", { force: true }); @@ -108,15 +111,16 @@ describe("Browser API invokeIPC", () => { }); test("should throw on HTTP errors", async () => { - const mockFetch = async () => { - return { + const mockFetch = () => { + return Promise.resolve({ ok: false, status: 500, - } as Response; + } as Response); }; - const invokeIPC = await createInvokeIPC(mockFetch); + const invokeIPC = createInvokeIPC(mockFetch); + // eslint-disable-next-line @typescript-eslint/await-thenable await expect(invokeIPC("WORKSPACE_REMOVE", "test-workspace", { force: false })).rejects.toThrow( "HTTP error! status: 500" ); @@ -134,7 +138,7 @@ describe("Browser API invokeIPC", () => { error: structuredError, }); - const invokeIPC = await createInvokeIPC(mockFetch); + const invokeIPC = createInvokeIPC(mockFetch); const result = await invokeIPC("WORKSPACE_SEND_MESSAGE", "test-workspace", { role: "user", @@ -148,4 +152,3 @@ describe("Browser API invokeIPC", () => { }); }); }); - From 693a5a233750cc7ccabea2edad9e14db889dc782 Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Wed, 22 Oct 2025 23:13:05 -0400 Subject: [PATCH 3/3] fix: Update useAutoCompactContinue to handle error objects instead of .catch() The browser API now returns error objects matching Electron's behavior. Updated the only caller using .catch() to properly check result.success. --- src/browser/api.test.ts | 31 ++++++----------------------- src/hooks/useAutoCompactContinue.ts | 24 +++++++++++++++++----- 2 files changed, 25 insertions(+), 30 deletions(-) diff --git a/src/browser/api.test.ts b/src/browser/api.test.ts index 1fe5da4e9..12503230b 100644 --- a/src/browser/api.test.ts +++ b/src/browser/api.test.ts @@ -42,14 +42,10 @@ function createInvokeIPC( const result = (await response.json()) as InvokeResponse; + // Return the result as-is - let the caller handle success/failure + // This matches the behavior of Electron's ipcRenderer.invoke() which doesn't throw on error if (!result.success) { - // Failed response - check if it's a structured error or simple string - if (typeof result.error === "object" && result.error !== null) { - // Structured error (e.g., SendMessageError) - return as Result for caller to handle - return result as T; - } - // Simple string error - throw it - throw new Error(typeof result.error === "string" ? result.error : "Unknown error"); + return result as T; } // Success - unwrap and return the data @@ -60,7 +56,7 @@ function createInvokeIPC( } describe("Browser API invokeIPC", () => { - test("CURRENT BEHAVIOR: throws on string error (causes unhandled rejection)", async () => { + test("should return error object on failure (matches Electron behavior)", async () => { const mockFetch = createMockFetch({ success: false, error: "fatal: contains modified or untracked files", @@ -68,23 +64,8 @@ describe("Browser API invokeIPC", () => { const invokeIPC = createInvokeIPC(mockFetch); - // Current behavior: invokeIPC throws on string errors - // eslint-disable-next-line @typescript-eslint/await-thenable - await expect(invokeIPC("WORKSPACE_REMOVE", "test-workspace", { force: false })).rejects.toThrow( - "fatal: contains modified or untracked files" - ); - }); - - test.skip("DESIRED BEHAVIOR: should return error object on string error (match Electron)", async () => { - const mockFetch = createMockFetch({ - success: false, - error: "fatal: contains modified or untracked files", - }); - - const invokeIPC = createInvokeIPC(mockFetch); - - // Desired behavior: Should return { success: false, error: "..." } - // This test documents what we want - actual implementation test is below + // Fixed behavior: invokeIPC returns error object instead of throwing + // This matches Electron's ipcRenderer.invoke() which never throws on error const result = await invokeIPC<{ success: boolean; error?: string }>( "WORKSPACE_REMOVE", "test-workspace", diff --git a/src/hooks/useAutoCompactContinue.ts b/src/hooks/useAutoCompactContinue.ts index bb23f0406..b23533792 100644 --- a/src/hooks/useAutoCompactContinue.ts +++ b/src/hooks/useAutoCompactContinue.ts @@ -81,11 +81,25 @@ export function useAutoCompactContinue() { // Build options and send message directly const options = buildSendMessageOptions(workspaceId); - window.api.workspace.sendMessage(workspaceId, continueMessage, options).catch((error) => { - console.error("Failed to send continue message:", error); - // If sending failed, remove from processed set to allow retry - processedMessageIds.current.delete(idForGuard); - }); + void (async () => { + try { + const result = await window.api.workspace.sendMessage( + workspaceId, + continueMessage, + options + ); + // Check if send failed (browser API returns error object, not throw) + if (!result.success && "error" in result) { + console.error("Failed to send continue message:", result.error); + // If sending failed, remove from processed set to allow retry + processedMessageIds.current.delete(idForGuard); + } + } catch (error) { + // Handle network/parsing errors (HTTP errors, etc.) + console.error("Failed to send continue message:", error); + processedMessageIds.current.delete(idForGuard); + } + })(); } };