fix: App Now Remembers Your Download Folder Again

[jeanfbrito]

🐛 What Was Broken

In version 4.8.1, the Rocket.Chat Desktop app stopped remembering where you last saved a download. This happened due to a breaking change in one of the underlying libraries we use. This meant every time you downloaded a file, you had to manually navigate to your preferred folder - very annoying if you frequently download files to specific locations like Desktop, Documents, or project folders.

https://rocketchat.atlassian.net/browse/SUP-870

What users experienced:

• ✅ Versions 4.7.1 and 4.8.0: App remembered your last download location
• ❌ Version 4.8.1: Always defaulted to Downloads folder, no memory

✅ What's Fixed

The app now properly remembers your download folder preference again! Here's what works now:

🎯 Smart Download Memory

• First download: App opens to your default Downloads folder
• Choose a different folder: App remembers this choice
• Next download: App automatically opens to your last-used folder
• Works across app restarts: Your preference is saved permanently

💡 How It Works for Users

1. Download a file - Choose "Save As" and pick any folder you want
2. Download another file - The app opens directly to that same folder
3. Restart the app - Your folder preference is still remembered
4. Change folders anytime - Just save to a different location and the app learns the new preference

🎉 User Benefits

Before this fix:

• Navigate to your preferred folder every single time
• Frustrating experience for users who organize downloads
• Lost productivity from repetitive folder navigation

After this fix:

• Seamless download experience
• App learns and adapts to your preferences
• Saves time and improves workflow
• Works exactly like users expect

🔒 What We Added for Quality

Added comprehensive testing to make sure this feature never breaks again. The tests cover all the ways people actually use downloads - different file types, special characters in names, various folder structures, and edge cases.

🎯 Bottom Line

Your Rocket.Chat Desktop app now works the way it should - it remembers where you like to save files and makes downloading smooth and efficient again.

Summary by CodeRabbit

• New Features

  • App persists and preselects the last download folder, falls back to OS temp dir when needed, and shows a localized completion notification.

• Refactor

  • Download handling moved to a dedicated setup module and initialized from main; download preferences/store surfaced for integration/testing.

• Tests

  • Added comprehensive tests for folder persistence, edge cases, session continuity, failure modes, and notification localization.

[coderabbitai]

Walkthrough

Adds a new downloads setup module that centralizes electron-dl configuration and persistent download directory tracking, removes the inline implementation from src/main.ts in favor of the new module, updates notification title to use i18n, and adds tests for persistence and localization behaviors.

Changes

Cohort / File(s)	Summary
New download setup module src/downloads/main/setup.ts	Adds setupElectronDlWithTracking() and exported downloadStore. Implements default path resolution, isValidDirectory, onStarted (resolve/store last directory, set save dialog defaultPath, locate valid webContents, call handleWillDownloadEvent), and onCompleted (persist directory, emit localized notification).
Main entry point (invocation change) src/main.ts	Removes the inline setupElectronDlWithTracking implementation and calls the imported setupElectronDlWithTracking() from the new module.
Download notification localization src/downloads/main.ts	Replaces static notification title 'Downloads' with i18n call t('downloads.title', { defaultValue: 'Downloads' }) and guards subtitle/body filename usage on completion.
Tests: persistence & i18n src/downloads/main/download-persistence.spec.ts, src/main.spec.ts	Adds download-persistence.spec.ts with extensive mocks (electron, electron-dl, electron-store, fs, os, i18next, notifications) to validate onStarted/onCompleted, directory persistence, defaults/fallbacks, store errors, webContents selection, and update src/main.spec.ts expectations for localized title usage.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant Renderer as Renderer / WebContents
  participant Main as Main Process
  participant Setup as downloads/setup.ts
  participant DL as electron-dl
  participant Store as ElectronStore
  participant FS as File System
  participant Notify as Notification

  Note over Main,Setup: App startup
  Main->>Setup: setupElectronDlWithTracking()

  Note over Setup,DL: configure electron-dl (saveAs: true, callbacks)

  User->>Renderer: initiate download
  Renderer->>DL: start download
  DL-->>Setup: onStarted(item, webContents)
  Setup->>Store: get("lastDownloadDirectory")
  Store-->>Setup: lastDir or default
  Setup->>FS: isValidDirectory(lastDir)
  Setup->>DL: set save dialog defaultPath = lastDir/filename
  Setup->>Main: find valid webContents -> handleWillDownloadEvent (try/catch)

  DL->>FS: write file
  DL-->>Setup: onCompleted(item)
  Setup->>Store: set("lastDownloadDirectory", dirname(savedPath))
  Setup->>Notify: show localized "done" notification (t('downloads.title', { defaultValue: 'Downloads' }))

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

I hop where folders softly grow,
I nudge the path where downloads go,
I save the trail with careful paw,
I sing a bell when files you saw,
Hooray — your downloads safe, hoorah! 🥕🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Out of Scope Changes Check	⚠️ Warning	In addition to the folder persistence logic, the pull request introduces localization changes for notification titles and corresponding test updates that are not related to restoring download folder behavior as specified in SUP-870.	Consider separating the notification localization and related test adjustments into a distinct pull request to keep the scope focused on download folder persistence.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title clearly and concisely summarizes the primary change of the pull request by stating that the application will once again remember the download folder, which directly reflects the main objective of restoring download folder persistence after a regression.
Linked Issues Check	✅ Passed	The implementation restores and persists the last-used download directory using ElectronStore, applies it as the default save path on start, updates the stored path on completion, and includes comprehensive tests for multiple download scenarios, fully satisfying the requirements of SUP-870 to remember and restore the download folder across sessions.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/last-download-folder

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/main.ts (1)

97-106: Bug: onCompleted param shape — use DownloadItem APIs

electron-dl’s onCompleted typically provides a DownloadItem; it does not guarantee plain properties path/filename. Use getSavePath()/getFilename() and guard for undefined.

-    onCompleted: (file) => {
-      // Remember the directory where the file was saved
-      const downloadDirectory = path.dirname(file.path);
-      downloadStore.set('lastDownloadDirectory', downloadDirectory);
-
-      createNotification({
-        title: 'Downloads',
-        body: file.filename,
-        subtitle: t('downloads.notifications.downloadFinished'),
-      });
-    },
+    onCompleted: (item) => {
+      // Remember the directory where the file was saved
+      const savePath =
+        typeof (item as any)?.getSavePath === 'function'
+          ? (item as any).getSavePath()
+          : (item as any)?.path;
+      if (savePath) {
+        downloadStore.set('lastDownloadDirectory', path.dirname(savePath));
+      }
+
+      const filename =
+        typeof (item as any)?.getFilename === 'function'
+          ? (item as any).getFilename()
+          : (item as any)?.filename ?? (savePath ? path.basename(savePath) : '');
+
+      createNotification({
+        title: 'Downloads',
+        body: filename,
+        subtitle: t('downloads.notifications.downloadFinished'),
+      });
+    },

🧹 Nitpick comments (3)

src/main.ts (3)

50-56: Type the ElectronStore and validate shape

Use ElectronStore generics (and optionally schema) to prevent drift and avoid casts.

-const downloadStore = new ElectronStore({
+const downloadStore = new ElectronStore<DownloadPrefs>({
   name: 'download-preferences',
   defaults: {
     lastDownloadDirectory: app.getPath('downloads'),
   },
+  schema: {
+    lastDownloadDirectory: { type: 'string' },
+  },
 });

Add this type near the top of the file:

type DownloadPrefs = {
  lastDownloadDirectory: string;
};

---

62-73: Guard against invalid or missing directory in store

If the stored directory no longer exists (network drive, removed folder, corrupted value), fall back to app.getPath('downloads') to avoid a broken defaultPath.

-      const lastDownloadDir = downloadStore.get(
-        'lastDownloadDirectory',
-        app.getPath('downloads')
-      ) as string;
-
-      const fullPath = path.join(lastDownloadDir, item.getFilename());
+      const configuredDir = downloadStore.get(
+        'lastDownloadDirectory',
+        app.getPath('downloads')
+      ) as string;
+      const lastDownloadDir =
+        configuredDir && isValidDirectory(configuredDir)
+          ? configuredDir
+          : app.getPath('downloads');
+      const fullPath = path.join(lastDownloadDir, item.getFilename());

Add these helpers/imports:

import { existsSync, statSync } from 'fs';

function isValidDirectory(dir: string): boolean {
  try {
    return !!dir && existsSync(dir) && statSync(dir).isDirectory();
  } catch {
    return false;
  }
}

---

102-106: Localize notification title

Hardcoded 'Downloads' bypasses i18n. Use a translation key with a sensible default.

-        title: 'Downloads',
+        title: t('downloads.notifications.title', { defaultValue: 'Downloads' }),

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between a167ecf and e867f9a.

📒 Files selected for processing (2)

• src/downloads/main/download-persistence.spec.ts (1 hunks)
• src/main.ts (3 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

• GitHub Check: check (windows-latest)
• GitHub Check: check (ubuntu-latest)
• GitHub Check: check (macos-latest)

🔇 Additional comments (1)

src/main.ts (1)

61-61: Don't modify onStarted signature – it only takes the download item
electron-dl v4’s onStarted callback doesn’t pass a BrowserWindow, so the proposed (item, win) signature is unsupported.

Likely an incorrect or invalid review comment.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (4)

src/downloads/main/download-persistence.spec.ts (3)

147-159: Make path assertions OS-agnostic (use path.join).

Hard-coded “/”-based expectations will fail on Windows runners. Use path.join (or assert with path.sep) in all defaultPath expectations.

Apply pattern like:

+import path from 'path';
...
-      expect(mockItem.setSaveDialogOptions).toHaveBeenCalledWith({
-        defaultPath: '/Users/test/Documents/document.pdf',
-      });
+      expect(mockItem.setSaveDialogOptions).toHaveBeenCalledWith({
+        defaultPath: path.join('/Users/test/Documents', 'document.pdf'),
+      });

Repeat similarly for other expectations (e.g., lines asserting /Users/test/Documents/My File (2023) [Copy].pdf, ${dir}/${filename}, etc.).

Also applies to: 168-172, 198-207, 352-355, 394-398, 523-525, 567-569

---

175-186: Add explicit test for empty-string store value.

Current tests don’t cover the case where the store returns ''. After adjusting production code to guard against empty/invalid paths, add a case:

 it('should use fallback directory when store returns empty', () => {
   expect(onStartedCallback).toBeDefined();

   const mockItem = createMockDownloadItem('test.pdf');
-  sharedMockStore.get.mockReturnValue('/Users/test/Downloads'); // Fallback value
+  sharedMockStore.get.mockReturnValue(''); // Simulate bad persisted value

   onStartedCallback(mockItem);

-  expect(mockItem.setSaveDialogOptions).toHaveBeenCalledWith({
-    defaultPath: expect.stringContaining('test.pdf'),
-  });
+  expect(mockItem.setSaveDialogOptions).toHaveBeenCalledWith({
+    defaultPath: path.join('/Users/test/Downloads', 'test.pdf'),
+  });
 });

---

588-618: The “complete failure” test doesn’t actually simulate module init failures.

defaultDownloadPath is computed at import time; clearing mocks isn’t enough. Use isolated module reload to simulate app.getPath throwing during module initialization.

-  it('should handle complete failure scenarios gracefully', () => {
-    // Test when everything fails
-    sharedMockStore.get.mockImplementation(() => {
-      throw new Error('Complete store failure');
-    });
-    sharedMockStore.set.mockImplementation(() => {
-      throw new Error('Complete store failure');
-    });
-    appMock.getPath.mockImplementation(() => {
-      throw new Error('App path failure');
-    });
+  it('should handle complete failure scenarios gracefully', async () => {
+    jest.resetModules();
+    jest.doMock('electron', () => ({
+      app: { getPath: jest.fn(() => { throw new Error('App path failure'); }) },
+      webContents: { getAllWebContents: jest.fn(() => []) },
+    }));
+    jest.doMock('electron-dl', () => jest.fn());
+    jest.doMock('electron-store', () => jest.fn(() => ({
+      get: jest.fn(() => { throw new Error('Complete store failure'); }),
+      set: jest.fn(() => { throw new Error('Complete store failure'); }),
+    })));
+    const { setupElectronDlWithTracking } = require('./setup');
 
-    expect(() => {
-      setupElectronDlWithTracking();
+    expect(() => {
+      setupElectronDlWithTracking();
       const electronDlCall = (require('electron-dl') as jest.Mock).mock.calls[0];
       const callbacks = electronDlCall?.[0];
       if (callbacks?.onStarted) {
         const mockItem = createMockDownloadItem('fail-test.pdf');
         callbacks.onStarted(mockItem);
       }
       if (callbacks?.onCompleted) {
         (callbacks.onCompleted as any)({
           filename: 'fail-test.pdf',
           path: '/some/path/fail-test.pdf',
         });
       }
     }).not.toThrow();
   });

src/downloads/main/setup.ts (1)

11-24: Resolve default download path lazily and harden fallbacks.

Computing and persisting a fallback at module load can lock in a bad default (e.g., before app is ready). Also, guard against empty/invalid stored values.

Apply this diff:

-// Simple store for download directory persistence
-let defaultDownloadPath: string;
-try {
-  defaultDownloadPath = app.getPath('downloads');
-} catch {
-  defaultDownloadPath = '/tmp';
-}
+// Helper to resolve default download path at use time
+const resolveDefaultDownloadPath = (): string => {
+  try {
+    return app.getPath('downloads');
+  } catch {
+    // Conservative fallback; Electron should normally provide a valid path
+    return '/tmp';
+  }
+};
 
-const downloadStore = new ElectronStore({
+const downloadStore = new ElectronStore({
   name: 'download-preferences',
   defaults: {
-    lastDownloadDirectory: defaultDownloadPath,
+    lastDownloadDirectory: resolveDefaultDownloadPath(),
   },
 });
 
 ...
-        let lastDownloadDir: string;
+        let lastDownloadDir: string;
         try {
-          lastDownloadDir = downloadStore.get(
-            'lastDownloadDirectory',
-            defaultDownloadPath
-          ) as string;
+          lastDownloadDir = downloadStore.get(
+            'lastDownloadDirectory',
+            resolveDefaultDownloadPath()
+          ) as string;
         } catch {
-          // Fallback if store fails
-          lastDownloadDir = defaultDownloadPath;
+          // Fallback if store fails
+          lastDownloadDir = resolveDefaultDownloadPath();
         }
+        // Guard against empty/relative values
+        if (
+          typeof lastDownloadDir !== 'string' ||
+          lastDownloadDir.trim() === '' ||
+          !path.isAbsolute(lastDownloadDir)
+        ) {
+          lastDownloadDir = resolveDefaultDownloadPath();
+        }
 
 ...
-        const downloadDirectory = path.dirname(file.path);
-        downloadStore.set('lastDownloadDirectory', downloadDirectory);
+        const downloadDirectory = path.dirname(file.path);
+        if (
+          downloadDirectory &&
+          downloadDirectory !== '.' &&
+          path.isAbsolute(downloadDirectory)
+        ) {
+          downloadStore.set('lastDownloadDirectory', downloadDirectory);
+        }

Also applies to: 31-41, 79-83

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between e867f9a and bd6707d.

📒 Files selected for processing (3)

• src/downloads/main/download-persistence.spec.ts (1 hunks)
• src/downloads/main/setup.ts (1 hunks)
• src/main.ts (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

src/downloads/main/download-persistence.spec.ts (1)

src/downloads/main/setup.ts (1)

• setupElectronDlWithTracking (26-94)

src/downloads/main/setup.ts (2)

src/downloads/main.ts (1)

• handleWillDownloadEvent (19-110)

src/notifications/preload.ts (1)

• createNotification (32-64)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

• GitHub Check: check (ubuntu-latest)
• GitHub Check: check (macos-latest)
• GitHub Check: check (windows-latest)

🔇 Additional comments (5)

src/downloads/main/download-persistence.spec.ts (2)

1-620: Good: tests now exercise the real setup helper (addresses prior duplication).

Importing and using setupElectronDlWithTracking from ./setup fixes the earlier duplication risk where the test reimplemented production logic.

---

425-443: Verify that handleWillDownloadEvent is invoked when a valid webContents exists.

You mock it but don’t assert it’s called. Add one assertion with a non-destroyed wc to ensure tracking stays wired.

 webContentsMock.getAllWebContents.mockReturnValue([]);
 const mockItem = createMockDownloadItem('test.pdf');

 expect(() => onStartedCallback(mockItem)).not.toThrow();
+
+// Now provide a valid webContents and assert handleWillDownloadEvent is called
+jest.clearAllMocks();
+const mockWc: any = { isDestroyed: () => false };
+webContentsMock.getAllWebContents.mockReturnValue([mockWc]);
+onStartedCallback(mockItem);
+const { handleWillDownloadEvent } = require('../main'); // mocked
+expect(handleWillDownloadEvent).toHaveBeenCalledWith(
+  expect.any(Object),
+  mockItem,
+  mockWc
+);

src/downloads/main/setup.ts (1)

26-77: LGTM: electron-dl setup and Save As defaults.

• saveAs: true with defaultPath including filename is correct.
• Defensive try/catch around store and tracking prevents crashes.

src/main.ts (2)

54-56: Initialize electron-dl setup after store creation and app ready.

Order looks correct: store is created, then setupElectronDlWithTracking() runs post-app.whenReady().

---

11-12: No duplicate will-download listeners detected
setupDownloads() only registers IPC handlers and does not attach any will-download listener— all download tracking is wired exclusively via setupElectronDlWithTracking().

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

src/downloads/main.ts (1)

171-177: Guard against missing download when retrying

If downloads[itemId] does not exist, new URL(download.url) will throw. Add a null-check before using it.

   handle('downloads/retry', async (_webContent, itemId) => {
     const { download, webContentsId } = select(({ downloads, servers }) => {
       const download = downloads[itemId];
       const { webContentsId } =
         servers.find((server) => server.url === download.serverUrl) ?? {};
       return { download, webContentsId };
     });
 
+    if (!download?.url) {
+      return;
+    }
+
     const downloadStartTimestamp = new URL(download.url).searchParams.get(
       'X-Amz-Date'
     );

Also applies to: 178-181

src/main.spec.ts (2)

22-24: Fix i18n mock to honor defaultValue (tests currently inconsistent and will fail)

The tests expect title to be 'Downloads' while the t mock returns the key. Make the mock return options.defaultValue when provided.

-jest.mock('i18next', () => ({
-  t: jest.fn((key: string) => key),
-}));
+jest.mock('i18next', () => ({
+  t: jest.fn((key: string, options?: { defaultValue?: string }) => options?.defaultValue ?? key),
+}));

---

363-377: Limit translation override to the subtitle in this test

mockReturnValue forces all t() calls to the same string, breaking the title expectation. Override only the finished subtitle key.

-      it('should use translated subtitle', () => {
+      it('should use translated subtitle', () => {
         const mockFile = { filename: 'test.pdf' };
         const tMock = t as jest.MockedFunction<typeof t>;
-        tMock.mockReturnValue('Download finished');
+        tMock.mockImplementation((key: string, options?: { defaultValue?: string }) => {
+          if (key === 'downloads.notifications.downloadFinished') return 'Download finished';
+          return options?.defaultValue ?? key;
+        });
 
         onCompletedCallback(mockFile);
 
         expect(tMock).toHaveBeenCalledWith(
           'downloads.notifications.downloadFinished'
         );
         expect(createNotificationMock).toHaveBeenCalledWith({
           title: 'Downloads',
           body: 'test.pdf',
           subtitle: 'Download finished',
         });
         expect(tMock).toHaveBeenCalledWith('downloads.title', {
           defaultValue: 'Downloads',
         });
       });

🧹 Nitpick comments (3)

src/downloads/main.ts (3)

82-86: Add default fallbacks for subtitle translations

If the i18n keys are missing, users will see raw keys. Provide defaults for both finished/cancelled subtitles.

   item.on('done', (_event, state) => {
     createNotification({
-      title: t('downloads.title', { defaultValue: 'Downloads' }),
+      title: t('downloads.title', { defaultValue: 'Downloads' }),
       body: item.getFilename(),
       subtitle:
         state === 'completed'
-          ? t('downloads.notifications.downloadFinished')
-          : t('downloads.notifications.downloadCancelled'),
+          ? t('downloads.notifications.downloadFinished', { defaultValue: 'Download finished' })
+          : t('downloads.notifications.downloadCancelled', { defaultValue: 'Download cancelled' }),
     });

---

17-26: Avoid potential itemId collisions generated by Date.now()

Multiple downloads starting in the same millisecond can collide. Use a monotonic counter to ensure uniqueness.

 const items = new Map<Download['itemId'], DownloadItem>();
 
+let lastItemId = 0;
+
 export const handleWillDownloadEvent = async (
   _event: Event,
   item: DownloadItem,
   serverWebContents: WebContents
 ): Promise<void> => {
-  const itemId = Date.now();
+  const now = Date.now();
+  const itemId = Math.max(now, lastItemId + 1);
+  lastItemId = itemId;
   items.set(itemId, item);

---

233-236: Remove the any cast on the IPC channel

Prefer proper typing for the 'downloads/clear-all' IPC channel to avoid any.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between bd6707d and 079f2eb.

📒 Files selected for processing (4)

• src/downloads/main.ts (1 hunks)
• src/downloads/main/download-persistence.spec.ts (1 hunks)
• src/downloads/main/setup.ts (1 hunks)
• src/main.spec.ts (3 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

src/downloads/main/setup.ts (2)

src/downloads/main.ts (1)

• handleWillDownloadEvent (19-110)

src/notifications/preload.ts (1)

• createNotification (32-64)

src/downloads/main/download-persistence.spec.ts (1)

src/downloads/main/setup.ts (1)

• setupElectronDlWithTracking (42-109)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

• GitHub Check: check (ubuntu-latest)
• GitHub Check: Analyze (javascript)
• GitHub Check: Analyze (actions)

🔇 Additional comments (4)

src/downloads/main/setup.ts (1)

99-103: Remove the duplicate download completion notification

Line 99 reintroduces the duplicate toast that handleWillDownloadEvent already fires inside the download item's 'done' handler (see Line 78 in src/downloads/main.ts). Users now get two “Download finished” popups per file again. Please drop this notification here and rely on the existing one.

-        createNotification({
-          title: t('downloads.title', { defaultValue: 'Downloads' }),
-          body: file.filename || 'Unknown file',
-          subtitle: t('downloads.notifications.downloadFinished'),
-        });

src/downloads/main/download-persistence.spec.ts (1)

212-248: Do not redeclare setupElectronDlWithTracking in the spec

Lines 212-248 reintroduce a local setupElectronDlWithTracking function, shadowing the imported helper. This causes a compilation error (Identifier 'setupElectronDlWithTracking' has already been declared) and—if it somehow slipped past the compiler—means the tests never execute the production implementation, defeating the regression coverage we need. Please delete this block and exercise the real exported helper instead.

-  // Test the electron-dl setup function directly
-  const setupElectronDlWithTracking = () => {
-    electronDl({
-      saveAs: true,
-      onStarted: (item) => {
-        const webContentsArray = webContents.getAllWebContents();
-
-        let sourceWebContents = null;
-        for (const wc of webContentsArray) {
-          if (wc && !wc.isDestroyed()) {
-            sourceWebContents = wc;
-            break;
-          }
-        }
-
-        if (sourceWebContents) {
-          const fakeEvent = {
-            defaultPrevented: false,
-            preventDefault: () => {},
-          };
-          handleWillDownloadEvent(
-            fakeEvent as any,
-            item,
-            sourceWebContents
-          ).catch(() => {
-            // Silently handle tracking errors
-          });
-        }
-      },
-      onCompleted: (file) => {
-        createNotification({
-          title: t('downloads.title', { defaultValue: 'Downloads' }),
-          body: file.filename || 'Unknown file',
-          subtitle: t('downloads.notifications.downloadFinished'),
-        });
-      },
-    });
-  };

src/downloads/main.ts (1)

78-86: LGTM: Localized notification title with default fallback

The switch to t('downloads.title', { defaultValue: 'Downloads' }) looks good.

src/main.spec.ts (1)

3-3: electron-dl compatibility OK
Project uses Electron v37.2.4 and electron-dl v4.0.0, satisfying the requirement of Electron >= 30.

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (6)

src/downloads/main/setup.ts (3)

1-3: Import os for cross‑platform tmp dir fallback.

Apply this diff:

 import { existsSync, statSync } from 'fs';
 import path from 'path';
+import os from 'os';

---

4-11: Add type-only imports for BrowserWindow/WebContents used in the new onStarted signature.

Apply this diff:

-import { app, webContents } from 'electron';
+import { app, webContents } from 'electron';
+import type { BrowserWindow, WebContents } from 'electron';

---

89-91: Don’t swallow errors silently; log at debug level.

At least emit debug logs so issues are diagnosable in support cases.

I can wire this to your existing logger (electron-log or similar) if you point me to it.

Also applies to: 106-108

src/downloads/main/download-persistence.spec.ts (3)

340-351: Add a test ensuring we use the webContents provided by electron-dl (no global scanning).

Prevents regressions where downloads get attributed to the wrong server.

Apply this diff:

   beforeEach(() => {
     setupElectronDlWithTracking();
     const electronDlCall = electronDlMock.mock.calls[0];
     if (electronDlCall?.[0]) {
       onStartedCallback = electronDlCall[0].onStarted!;
       onCompletedCallback = electronDlCall[0].onCompleted! as any;
     }
   });
 
   it('should remember folder across multiple downloads', () => {
@@
       expect(secondItem.setSaveDialogOptions).toHaveBeenCalledWith({
         defaultPath: '/Users/test/Documents/second-file.xlsx',
       });
     });
+
+    it('should use the webContents passed by electron-dl', () => {
+      const electronDlCall = electronDlMock.mock.calls[0];
+      const onStarted = electronDlCall?.[0]?.onStarted as any;
+      const mockItem = createMockDownloadItem('provided.txt');
+      const mockWc = { id: 42, isDestroyed: () => false, getType: () => 'window' } as any;
+      const { handleWillDownloadEvent } = require('../main');
+
+      onStarted(mockItem, { webContents: mockWc });
+
+      expect(handleWillDownloadEvent).toHaveBeenCalledWith(
+        expect.anything(),
+        mockItem,
+        mockWc
+      );
+      expect(webContentsMock.getAllWebContents).not.toHaveBeenCalled();
+    });

---

249-338: Add a test to ensure no duplicate notifications are emitted from onCompleted.

Guards against regressions once the onCompleted notification is removed.

Apply this diff:

   describe('onCompleted - Directory Storage', () => {
     let onCompletedCallback: (file: { filename: string; path: string }) => void;
@@
     it('should handle invalid file paths', () => {
       expect(onCompletedCallback).toBeDefined();
@@
       invalidPaths.forEach((invalidPath) => {
         const mockFile = {
           filename: 'test.pdf',
           path: invalidPath,
         };
 
         expect(() => onCompletedCallback(mockFile)).not.toThrow();
       });
     });
+
+    it('should not emit a notification here (handled by handleWillDownloadEvent)', () => {
+      const { createNotification } = require('../../notifications/preload');
+      onCompletedCallback({
+        filename: 'done.txt',
+        path: '/Users/test/Downloads/done.txt',
+      });
+      expect(createNotification).not.toHaveBeenCalled();
+    });
   });

---

15-23: Consider asserting that getAllWebContents is not called when a webContents is provided.

Once production uses the passed webContents, extend mocks here to verify we’re not scanning globally in that path.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 6d211eb and f3aac0f.

📒 Files selected for processing (2)

• src/downloads/main/download-persistence.spec.ts (1 hunks)
• src/downloads/main/setup.ts (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

src/downloads/main/setup.ts (2)

src/downloads/main.ts (1)

• handleWillDownloadEvent (19-110)

src/notifications/preload.ts (1)

• createNotification (32-64)

src/downloads/main/download-persistence.spec.ts (1)

src/downloads/main/setup.ts (1)

• setupElectronDlWithTracking (42-111)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

• GitHub Check: check (ubuntu-latest)
• GitHub Check: check (macos-latest)
• GitHub Check: check (windows-latest)

🔇 Additional comments (2)

src/downloads/main/setup.ts (2)

101-105: Remove duplicate download notification (already emitted in handleWillDownloadEvent).

This emits a second notification for the same download. The item.on('done') handler in handleWillDownloadEvent already notifies.

Apply this diff:

-        createNotification({
-          title: t('downloads.title', { defaultValue: 'Downloads' }),
-          body: file.filename || 'Unknown file',
-          subtitle: t('downloads.notifications.downloadFinished'),
-        });

---

62-76: Remove unsupported WebContents parameter in onStarted
electron-dl v4.0.0’s onStarted callback only receives the DownloadItem (no BrowserWindow/WebContents argument). Continue scanning webContents or upgrade to ≥v5 for native support.

Likely an incorrect or invalid review comment.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (4)

src/downloads/main/download-persistence.spec.ts (1)

334-347: Assert that no preference is persisted when the file path is invalid.

Currently this test only checks that no exception is thrown. Strengthen it by verifying that the store is not written when path is falsy or malformed.

Apply this diff within the test:

   it('should handle invalid file paths', () => {
     expect(onCompletedCallback).toBeDefined();

-    const invalidPaths = ['', '/'];
+    const invalidPaths = ['', '/'];

-    invalidPaths.forEach((invalidPath) => {
+    sharedMockStore.set.mockClear();
+    invalidPaths.forEach((invalidPath) => {
       const mockFile = {
         filename: 'test.pdf',
         path: invalidPath,
       };

       expect(() => onCompletedCallback(mockFile)).not.toThrow();
+      if (!invalidPath) {
+        expect(sharedMockStore.set).not.toHaveBeenCalled();
+      }
     });
   });

src/main.spec.ts (1)

243-245: Avoid duplicating the electron‑dl setup in this spec; import the real helper.

This suite still redefines setupElectronDlWithTracking locally, so it won’t catch regressions in the production module. Prefer importing the real function from src/downloads/main/setup.ts or centralizing the test helper there.

For example:

import { setupElectronDlWithTracking } from './downloads/main/setup';
// ...use setupElectronDlWithTracking() in tests and remove the local definition.

src/downloads/main/setup.ts (2)

31-37: Simplify directory validation; avoid redundant existsSync call.

statSync throws if the path doesn’t exist; the existsSync check is redundant.

Apply this diff:

 function isValidDirectory(dir: string): boolean {
   try {
-    return !!dir && existsSync(dir) && statSync(dir).isDirectory();
+    return !!dir && statSync(dir).isDirectory();
   } catch {
     return false;
   }
 }

---

39-44: Harden ElectronStore against invalid persisted values.

Enable clearInvalidConfig so bad values are auto-removed instead of propagating errors.

Apply this diff:

 const downloadStore = new ElectronStore<DownloadPrefs>({
   name: 'download-preferences',
+  clearInvalidConfig: true,
   schema: {
     lastDownloadDirectory: { type: 'string' },
   },
 });

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between f3aac0f and 8adc467.

📒 Files selected for processing (3)

• src/downloads/main/download-persistence.spec.ts (1 hunks)
• src/downloads/main/setup.ts (1 hunks)
• src/main.spec.ts (3 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

src/downloads/main/setup.ts (2)

src/downloads/main.ts (1)

• handleWillDownloadEvent (19-110)

src/notifications/preload.ts (1)

• createNotification (32-64)

src/downloads/main/download-persistence.spec.ts (1)

src/downloads/main/setup.ts (1)

• setupElectronDlWithTracking (46-115)

🔇 Additional comments (9)

src/downloads/main/download-persistence.spec.ts (2)

135-146: Good setup validation; exercising the real setup module.

Electron-dl is wired with saveAs and both callbacks; this guards against regressions. Nice.

---

246-257: Graceful error-path coverage looks good.

Catching store read errors in onStarted without crashing is correct and matches the production code’s intent.

src/main.spec.ts (3)

243-245: LGTM: Localized notification title and safe body fallback.

Using t('downloads.title', { defaultValue: 'Downloads' }) and guarding filename is correct and aligns with i18n changes.

---

354-360: LGTM: Test asserts translation hook is invoked with defaultValue.

The expectation verifies both the key and default usage; good safeguard for i18n regressions.

---

366-376: i18n mock extension is adequate for the new title and subtitle checks.

The mock covers both title and subtitle keys; expectations reflect localized values.

Also applies to: 388-390

src/downloads/main/setup.ts (4)

20-29: LGTM: Lazy, cached default path with os.tmpdir fallback.

Correctly avoids app.getPath before readiness and is cross‑platform.

---

97-103: file.path is the correct property for saved file path Both electron-dl v3.x and v4.x expose the full save path in file.path (there is no file.savePath).

---

105-110: Remove duplicate completion notification to avoid double toasts.

handleWillDownloadEvent already emits a notification on item 'done'. This onCompleted notification will produce a second, duplicate toast.

Apply this diff:

-        createNotification({
-          title: t('downloads.title', { defaultValue: 'Downloads' }),
-          body: file.filename || 'Unknown file',
-          subtitle: t('downloads.notifications.downloadFinished'),
-        });

Based on learnings

---

49-96: Retain current fallback logic for webContents
electron-dl v4.0.0’s onStarted callback only receives the DownloadItem—no second BrowserWindow/WebContents parameter exists, so the proposed diff isn’t applicable.

Likely an incorrect or invalid review comment.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (5)

src/downloads/main/download-persistence.spec.ts (5)

57-60: Assert positive path: handleWillDownloadEvent is called when a live webContents exists.

You cover absence/malformed webContents, but not the positive invocation. Add a focused assertion.

Add this test (near the other onStarted tests):

it('should invoke handleWillDownloadEvent when a live webContents exists', () => {
  const { handleWillDownloadEvent } = require('../main');
  const mockWC = { isDestroyed: () => false } as any;
  webContentsMock.getAllWebContents.mockReturnValue([mockWC]);

  const mockItem = createMockDownloadItem('file.txt');
  onStartedCallback(mockItem);

  expect(handleWillDownloadEvent).toHaveBeenCalledWith(
    expect.objectContaining({ defaultPrevented: false }),
    mockItem,
    mockWC
  );
});

---

270-284: Also assert notification and i18n title on completion.

Strengthen the regression guard by validating the localized title and payload.

Apply this diff:

       expect(sharedMockStore.set).toHaveBeenCalledWith(
         'lastDownloadDirectory',
         path.dirname('/Users/test/Documents/Projects/completed-file.pdf')
       );
+      const { createNotification } = require('../../notifications/preload');
+      const { t } = require('i18next');
+      expect(createNotification).toHaveBeenCalledWith({
+        title: 'downloads.title',
+        body: 'completed-file.pdf',
+        subtitle: 'downloads.notifications.downloadFinished',
+      });
+      expect(t).toHaveBeenCalledWith('downloads.title', { defaultValue: 'Downloads' });

---

470-479: Guard against rejected handleWillDownloadEvent (silent failure).

Add a test to ensure a rejection is swallowed per implementation.

Add this test:

it('should swallow errors if handleWillDownloadEvent rejects', () => {
  const { handleWillDownloadEvent } = require('../main');
  (handleWillDownloadEvent as jest.Mock).mockImplementationOnce(() => Promise.reject(new Error('boom')));

  const mockWC = { isDestroyed: () => false } as any;
  webContentsMock.getAllWebContents.mockReturnValue([mockWC]);

  const mockItem = createMockDownloadItem('test.pdf');
  expect(() => onStartedCallback(mockItem)).not.toThrow();
  expect(handleWillDownloadEvent).toHaveBeenCalled();
});

---

206-223: Add a case for "exists but not a directory".

You cover non-existent paths; add the non-directory case to fully exercise isValidDirectory.

Example test:

it('should fallback when configured path exists but is not a directory', () => {
  const { existsSync, statSync } = require('fs');
  existsSync.mockReturnValueOnce(true);
  statSync.mockReturnValueOnce({ isDirectory: () => false });

  const mockItem = createMockDownloadItem('test.pdf');
  sharedMockStore.get.mockReturnValue('/Users/test/NotADir');

  onStartedCallback(mockItem);

  expect(mockItem.setSaveDialogOptions).toHaveBeenCalledWith({
    defaultPath: path.join('/Users/test/Downloads', 'test.pdf'),
  });
});

---

514-521: DRY: hoist createMockDownloadItem; remove duplicate helper here.

Define the helper once at file scope and reuse it across describes to reduce duplication.

Apply this diff to remove the duplicate:

-  const createMockDownloadItem = (
-    filename = 'test-file.pdf'
-  ): jest.Mocked<DownloadItem> =>
-    ({
-      getFilename: jest.fn(() => filename),
-      setSaveDialogOptions: jest.fn(),
-      on: jest.fn(),
-    }) as any;

Then add a single top-level helper (place it before the first describe):

const createMockDownloadItem = (
  filename = 'test-file.pdf'
): jest.Mocked<DownloadItem> =>
  ({
    getFilename: jest.fn(() => filename),
    setSaveDialogOptions: jest.fn(),
    on: jest.fn(),
    // Other methods as needed by tests can be stubbed here.
  } as any);

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 8adc467 and 4945054.

📒 Files selected for processing (1)

• src/downloads/main/download-persistence.spec.ts (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

src/downloads/main/download-persistence.spec.ts (1)

src/downloads/main/setup.ts (1)

• setupElectronDlWithTracking (46-115)

🔇 Additional comments (3)

src/downloads/main/download-persistence.spec.ts (3)

7-8: Good move: tests exercise the real setup (no duplication).

Importing and using the production setupElectronDlWithTracking removes the prior duplication and ensures the spec guards real regressions.

---

139-145: electron-dl config assertion looks correct.

Verifies saveAs: true and the presence of both callbacks as intended.

---

624-655: Great failure-tolerance coverage.

Complete failure scenario is well contained and asserts non-throwing behavior.