From 5e25b3d96cc7f34f4a7f8f5d4db08e49bcd47a0e Mon Sep 17 00:00:00 2001 From: hayyaksi <193020925+hayaksi1@users.noreply.github.com> Date: Thu, 6 Aug 2026 01:44:55 +0300 Subject: [PATCH] Surface an error when a downloaded file can't be opened instead of failing silently (#33998) Pressing "Open" on the download-complete toast called shell.openPath and discarded the result with `void`. openPath resolves to a non-empty error string on failure (an empty string means success), so when the file had been moved or deleted the open silently did nothing and the user saw no response at all (#32273). Await the result and, on a non-empty error, log it and show an error dialog carrying the underlying error as its detail. Co-authored-by: R Midhun Suresh --- apps/desktop/src/i18n/strings/en_EN.json | 4 + apps/desktop/src/webcontents-handler.test.ts | 125 +++++++++++++++++++ apps/desktop/src/webcontents-handler.ts | 18 ++- 3 files changed, 143 insertions(+), 4 deletions(-) create mode 100644 apps/desktop/src/webcontents-handler.test.ts diff --git a/apps/desktop/src/i18n/strings/en_EN.json b/apps/desktop/src/i18n/strings/en_EN.json index fdcdf662d1..c4e0bad993 100644 --- a/apps/desktop/src/i18n/strings/en_EN.json +++ b/apps/desktop/src/i18n/strings/en_EN.json @@ -27,6 +27,10 @@ "yes": "Yes" }, "confirm_quit": "Are you sure you want to quit?", + "download": { + "unable_to_open_description": "The file could not be opened. It may have been moved or deleted.", + "unable_to_open_title": "Unable to open file" + }, "edit_menu": { "speech": "Speech", "speech_start_speaking": "Start Speaking", diff --git a/apps/desktop/src/webcontents-handler.test.ts b/apps/desktop/src/webcontents-handler.test.ts new file mode 100644 index 0000000000..b2cc105afa --- /dev/null +++ b/apps/desktop/src/webcontents-handler.test.ts @@ -0,0 +1,125 @@ +/* +Copyright 2026 hayaksi1 + +SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial +Please see LICENSE files in the repository root for full details. +*/ + +import { beforeEach, describe, expect, it, vi, type Mock } from "vitest"; +import { dialog, shell, type WebContents } from "electron"; + +// The `userDownloadAction` listener is registered at import time, so capture the callbacks that +// `ipcMain.on` receives in order to invoke the handler under test directly. +const { ipcHandlers } = vi.hoisted(() => ({ + ipcHandlers: {} as Record unknown>, +})); + +vi.mock("electron", () => ({ + clipboard: {}, + Menu: class {}, + MenuItem: class {}, + shell: { openExternal: vi.fn(), openPath: vi.fn() }, + dialog: { showMessageBox: vi.fn(), showSaveDialog: vi.fn() }, + ipcMain: { + on: vi.fn((channel: string, cb: (...args: unknown[]) => unknown) => { + ipcHandlers[channel] = cb; + }), + }, +})); +vi.mock("./language-helper.js", () => ({ _t: (key: string): string => key })); +vi.mock("./config.js", () => ({ getConfig: (): Record => ({}) })); +vi.mock("./save-image.js", () => ({ saveImageToFile: vi.fn() })); + +const registerWebContentsHandlers = (await import("./webcontents-handler.js")).default; + +interface MockWebContents { + setWindowOpenHandler: Mock; + on: Mock; + send: Mock; + session: { on: Mock }; + sessionHandlers: Record void>; +} + +function makeWebContents(): MockWebContents { + const sessionHandlers: Record void> = {}; + return { + setWindowOpenHandler: vi.fn(), + on: vi.fn(), + send: vi.fn(), + session: { + on: vi.fn((ev: string, cb: (...args: unknown[]) => void): void => { + sessionHandlers[ev] = cb; + }), + }, + sessionHandlers, + }; +} + +/** + * Drives the real will-download → done("completed") flow so the download is registered the way it is + * in production, and returns the id the handler assigned to it. + */ +function completeDownload(wc: MockWebContents, savePath: string): number { + const doneHandlers: Record void> = {}; + const item = { + once: (ev: string, cb: (...args: unknown[]) => void): void => { + doneHandlers[ev] = cb; + }, + getSavePath: (): string => savePath, + }; + wc.sessionHandlers["will-download"]({}, item); + doneHandlers["done"]({}, "completed"); + const completed = wc.send.mock.calls.find((c) => c[0] === "userDownloadCompleted"); + return (completed![1] as { id: number }).id; +} + +describe("userDownloadAction handler", () => { + let wc: MockWebContents; + + beforeEach(() => { + vi.clearAllMocks(); + wc = makeWebContents(); + registerWebContentsHandlers(wc as unknown as WebContents); + }); + + it("opens the file when the user clicks Open on a known download", async () => { + vi.mocked(shell.openPath).mockResolvedValue(""); + const id = completeDownload(wc, "/tmp/file.pdf"); + + await ipcHandlers["userDownloadAction"]({}, { id, open: true }); + + expect(shell.openPath).toHaveBeenCalledWith("/tmp/file.pdf"); + expect(dialog.showMessageBox).not.toHaveBeenCalled(); + }); + + it("shows the underlying error when the open fails, rather than failing silently", async () => { + vi.mocked(shell.openPath).mockResolvedValue("LSOpenURLsWithRole failed"); + const id = completeDownload(wc, "/tmp/file.pdf"); + + await ipcHandlers["userDownloadAction"]({}, { id, open: true }); + + expect(shell.openPath).toHaveBeenCalledWith("/tmp/file.pdf"); + expect(dialog.showMessageBox).toHaveBeenCalledWith( + expect.objectContaining({ type: "error", detail: "LSOpenURLsWithRole failed" }), + ); + }); + + it("does not open anything on a plain dismiss", async () => { + const id = completeDownload(wc, "/tmp/file.pdf"); + + await ipcHandlers["userDownloadAction"]({}, { id, open: false }); + + expect(shell.openPath).not.toHaveBeenCalled(); + }); + + it("removes the entry so a repeated open is a no-op", async () => { + vi.mocked(shell.openPath).mockResolvedValue(""); + const id = completeDownload(wc, "/tmp/file.pdf"); + + await ipcHandlers["userDownloadAction"]({}, { id, open: true }); + vi.mocked(shell.openPath).mockClear(); + await ipcHandlers["userDownloadAction"]({}, { id, open: true }); + + expect(shell.openPath).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/desktop/src/webcontents-handler.ts b/apps/desktop/src/webcontents-handler.ts index 3dc3d26ca0..4e473f9929 100644 --- a/apps/desktop/src/webcontents-handler.ts +++ b/apps/desktop/src/webcontents-handler.ts @@ -243,12 +243,22 @@ function onEditableContextMenu(ev: Event, params: ContextMenuParams, webContents let userDownloadIndex = 0; const userDownloadMap = new Map(); // Map from id to path -ipcMain.on("userDownloadAction", function (ev: IpcMainEvent, { id, open = false }) { +ipcMain.on("userDownloadAction", async function (ev: IpcMainEvent, { id, open = false }) { const path = userDownloadMap.get(id); - if (open && path) { - void shell.openPath(path); - } userDownloadMap.delete(id); + if (open && path) { + // openPath resolves to a non-empty error string on failure, an empty one on success. + const error = await shell.openPath(path); + if (error) { + console.error(`Failed to open downloaded file ${path}: ${error}`); + void dialog.showMessageBox({ + type: "error", + title: _t("download|unable_to_open_title"), + message: _t("download|unable_to_open_description"), + detail: error, + }); + } + } }); export default (webContents: WebContents): void => {