Keep a file's extension when it is renamed in the desktop save dialog (#34601)

Electron's save dialog offers only "All Files" unless it is handed filters,
so someone who replaced "photo.jpg" with "photo" got a file with no extension
at all and no way to open it. The browser does not do this: it offers the
file's own type, and puts the extension back when the name arrives without
one.

The download now names its type before the dialog goes up, and the "Save
image as..." context menu passes the same filters. A file with no extension
to preserve is left alone rather than being given a meaningless filter, and
"All Files" stays on the list so anyone who really wants an extensionless
name can still pick it.

Tests: a download with an extension gets filters, one without does not, and
the context menu path is covered as well.

Co-authored-by: Michael Telatynski <7t3chguy@gmail.com>

apps/desktop/src/i18n/strings/en_EN.json

@@ -72,6 +72,10 @@
         "save_image_as_error_description": "The image failed to save",
         "save_image_as_error_title": "Failed to save image"
     },
+    "save_dialog": {
+        "all_files": "All Files",
+        "named_file_type": "%(extension)s File"
+    },
     "store": {
         "error": {
             "backend_changed": "Clear data and reload?",

apps/desktop/src/webcontents-handler.test.ts

@@ -10,14 +10,41 @@ 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(() => ({
+const { ipcHandlers, menus } = vi.hoisted(() => ({
     ipcHandlers: {} as Record<string, (...args: unknown[]) => unknown>,
+    // Every context menu the handler builds, in construction order, so a test can click an entry.
+    menus: [] as MenuStub[],
 }));
 
+interface MenuItemStub {
+    label: string;
+    click?: () => void | Promise<void>;
+}
+
+interface MenuStub {
+    items: MenuItemStub[];
+}
+
 vi.mock("electron", () => ({
-    clipboard: {},
-    Menu: class {},
-    MenuItem: class {},
+    clipboard: { writeText: vi.fn() },
+    Menu: class {
+        public readonly items: MenuItemStub[] = [];
+        public constructor() {
+            menus.push(this);
+        }
+        public append(item: MenuItemStub): void {
+            this.items.push(item);
+        }
+        public popup(): void {}
+    },
+    MenuItem: class {
+        public readonly label: string;
+        public readonly click?: () => void | Promise<void>;
+        public constructor(options: MenuItemStub) {
+            this.label = options.label;
+            this.click = options.click;
+        }
+    },
     shell: { openExternal: vi.fn(), openPath: vi.fn() },
     dialog: { showMessageBox: vi.fn(), showSaveDialog: vi.fn() },
     ipcMain: {
@@ -36,39 +63,61 @@ interface MockWebContents {
     setWindowOpenHandler: Mock;
     on: Mock;
     send: Mock;
+    copyImageAt: Mock;
     session: { on: Mock };
+    handlers: Record<string, (...args: unknown[]) => void>;
     sessionHandlers: Record<string, (...args: unknown[]) => void>;
 }
 
 function makeWebContents(): MockWebContents {
+    const handlers: Record<string, (...args: unknown[]) => void> = {};
     const sessionHandlers: Record<string, (...args: unknown[]) => void> = {};
     return {
         setWindowOpenHandler: vi.fn(),
-        on: vi.fn(),
+        on: vi.fn((ev: string, cb: (...args: unknown[]) => void): void => {
+            handlers[ev] = cb;
+        }),
         send: vi.fn(),
+        copyImageAt: vi.fn(),
         session: {
             on: vi.fn((ev: string, cb: (...args: unknown[]) => void): void => {
                 sessionHandlers[ev] = cb;
             }),
         },
+        handlers,
         sessionHandlers,
     };
 }
 
+interface MockDownloadItem {
+    once: (ev: string, cb: (...args: unknown[]) => void) => void;
+    getFilename: () => string;
+    getSavePath: () => string;
+    setSaveDialogOptions: Mock;
+    doneHandlers: Record<string, (...args: unknown[]) => void>;
+}
+
+function makeDownloadItem(savePath: string): MockDownloadItem {
+    const doneHandlers: Record<string, (...args: unknown[]) => void> = {};
+    return {
+        once: (ev: string, cb: (...args: unknown[]) => void): void => {
+            doneHandlers[ev] = cb;
+        },
+        getFilename: (): string => savePath.split("/").pop()!,
+        getSavePath: (): string => savePath,
+        setSaveDialogOptions: vi.fn(),
+        doneHandlers,
+    };
+}
+
 /**
  * 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<string, (...args: unknown[]) => void> = {};
-    const item = {
-        once: (ev: string, cb: (...args: unknown[]) => void): void => {
-            doneHandlers[ev] = cb;
-        },
-        getSavePath: (): string => savePath,
-    };
+    const item = makeDownloadItem(savePath);
     wc.sessionHandlers["will-download"]({}, item);
-    doneHandlers["done"]({}, "completed");
+    item.doneHandlers["done"]({}, "completed");
     const completed = wc.send.mock.calls.find((c) => c[0] === "userDownloadCompleted");
     return (completed![1] as { id: number }).id;
 }
@@ -123,3 +172,53 @@ describe("userDownloadAction handler", () => {
         expect(shell.openPath).not.toHaveBeenCalled();
     });
 });
+
+describe("save dialog filters", () => {
+    let wc: MockWebContents;
+
+    beforeEach(() => {
+        vi.clearAllMocks();
+        menus.length = 0;
+        wc = makeWebContents();
+        registerWebContentsHandlers(wc as unknown as WebContents);
+    });
+
+    it("names a download's own file type, so renaming it does not strip the extension", () => {
+        const item = makeDownloadItem("/tmp/photo.jpg");
+
+        wc.sessionHandlers["will-download"]({}, item);
+
+        expect(item.setSaveDialogOptions).toHaveBeenCalledWith({
+            filters: [expect.objectContaining({ extensions: ["jpg"] }), expect.objectContaining({ extensions: ["*"] })],
+        });
+    });
+
+    it("leaves the dialog alone for a download which has no extension to preserve", () => {
+        const item = makeDownloadItem("/tmp/archive");
+
+        wc.sessionHandlers["will-download"]({}, item);
+
+        expect(item.setSaveDialogOptions).not.toHaveBeenCalled();
+    });
+
+    it("names the file type when saving an image from the context menu too", async () => {
+        vi.mocked(dialog.showSaveDialog).mockResolvedValue({ canceled: false, filePath: "/tmp/renamed.jpg" });
+
+        wc.handlers["context-menu"](
+            { preventDefault: vi.fn() },
+            { srcURL: "https://example.org/photo.jpg", hasImageContents: true, suggestedFilename: "photo.jpg" },
+        );
+        const saveAs = menus[0].items.find((item) => item.label === "right_click_menu|save_image_as");
+        await saveAs!.click!();
+
+        expect(dialog.showSaveDialog).toHaveBeenCalledWith(
+            expect.objectContaining({
+                defaultPath: "photo.jpg",
+                filters: [
+                    expect.objectContaining({ extensions: ["jpg"] }),
+                    expect.objectContaining({ extensions: ["*"] }),
+                ],
+            }),
+        );
+    });
+});

apps/desktop/src/webcontents-handler.ts

@@ -15,6 +15,7 @@ import {
     type WebContents,
     type ContextMenuParams,
     type DownloadItem,
+    type FileFilter,
     type MenuItemConstructorOptions,
     type IpcMainEvent,
     type Event,
@@ -30,6 +31,27 @@ const MAILTO_PREFIX = "mailto:";
 
 const PERMITTED_URL_SCHEMES: string[] = ["http:", "https:", MAILTO_PREFIX];
 
+/**
+ * Work out the filters a save dialog should offer so that a file keeps its own extension.
+ *
+ * A dialog which only offers "All Files" lets someone replace "photo.jpg" with "photo" and end up
+ * with a file the shell no longer knows how to open — the extension is simply gone. Naming the
+ * file's own type first means the dialog puts the extension back, which is what a browser already
+ * does for the same download.
+ *
+ * @param fileName - The name being suggested to the user, which may carry no extension at all.
+ * @returns Filters to pass to a save dialog, or undefined when there is no extension to preserve.
+ */
+function saveDialogFilters(fileName: string): FileFilter[] | undefined {
+    // extname() keeps the leading dot, and returns an empty string for a name which has none.
+    const extension = path.extname(fileName).slice(1);
+    if (!extension) return undefined;
+    return [
+        { name: _t("save_dialog|named_file_type", { extension: extension.toUpperCase() }), extensions: [extension] },
+        { name: _t("save_dialog|all_files"), extensions: ["*"] },
+    ];
+}
+
 function safeOpenURL(target: string): void {
     // openExternal passes the target to open/start/xdg-open,
     // so put fairly stringent limits on what can be opened
@@ -129,6 +151,7 @@ function onLinkContextMenu(ev: Event, params: ContextMenuParams, webContents: We
                     const targetFileName = params.suggestedFilename || params.altText || "image.png";
                     const { filePath } = await dialog.showSaveDialog({
                         defaultPath: targetFileName,
+                        filters: saveDialogFilters(targetFileName),
                     });
 
                     if (!filePath) return; // user cancelled dialog
@@ -283,6 +306,11 @@ export default (webContents: WebContents): void => {
     });
 
     webContents.session.on("will-download", (event: Event, item: DownloadItem): void => {
+        // Electron only offers "All Files" unless it is told otherwise, so say what this download is
+        // before it puts the save dialog up.
+        const filters = saveDialogFilters(item.getFilename());
+        if (filters) item.setSaveDialogOptions({ filters });
+
         item.once("done", (event, state) => {
             if (state === "completed") {
                 const savePath = item.getSavePath();