Skip to content

Fix auto updater modal stacking up #304

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
arnaudbreton wants to merge 1 commit into from
Closed

Fix auto updater modal stacking up #304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions apps/desktop/src/main/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,14 @@ registerStorageHandlers();
await makeAppSetup(() => MainWindow());
setupAutoUpdater();

// TESTING: Import and run updater test after 10 seconds
import("./lib/__test-updater").then((module) => {
setTimeout(() => {
console.log("[main] Running updater test simulation...");
module.simulateMultipleUpdateEvents();
}, 10000);
});
Comment on lines +49 to +55
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Do not run the updater test harness unconditionally in main

This block dynamically imports __test-updater and runs simulateMultipleUpdateEvents() on every app start, with no environment gating. Combined with the helper’s own 5‑second auto‑run, this will trigger multiple waves of fake update-downloaded events in any environment, including production, and can surface test dialogs to real users.

Given the // TESTING comment, this looks like temporary scaffolding that shouldn’t ship as‑is.

I’d strongly recommend either:

  • Removing this block before merging, relying on manual/local wiring when debugging; or
  • Gating it strictly to dev builds and using the alias path, e.g.:
+import { ENVIRONMENT } from "shared/constants";
@@
-	// TESTING: Import and run updater test after 10 seconds
-	import("./lib/__test-updater").then((module) => {
-		setTimeout(() => {
-			console.log("[main] Running updater test simulation...");
-			module.simulateMultipleUpdateEvents();
-		}, 10000);
-	});
+	// TESTING: dev‑only updater simulation hook
+	if (ENVIRONMENT.IS_DEV) {
+		void import("lib/__test-updater").then(({ simulateMultipleUpdateEvents }) => {
+			setTimeout(() => {
+				console.log("[main] Running updater test simulation...");
+				simulateMultipleUpdateEvents();
+			}, 10000);
+		});
+	}

Either way, this needs to be made non‑operational for production to avoid confusing users with artificial update flows.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In apps/desktop/src/main/index.ts around lines 49-55, the test updater harness
is unconditionally imported and executed on every app start; remove this block
or gate it to development only. Replace the unconditional dynamic import with a
guarded path that runs only in dev (e.g., check process.env.NODE_ENV ===
'development' or process.env.ELECTRON_IS_DEV) and avoid auto-executing the
helper on import (import the module lazily and call
simulateMultipleUpdateEvents() only when explicitly requested), or simply delete
the entire TESTING import before merging so production builds never trigger fake
update events.


// Clean up all terminals when app is quitting
app.on("before-quit", async () => {
await terminalManager.cleanup();
Expand Down
35 changes: 35 additions & 0 deletions apps/desktop/src/main/lib/__test-updater.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/**
* Test script to simulate multiple update-downloaded events firing rapidly
*
* Usage:
* 1. Import this in main/index.ts temporarily: import '.lib/__test-updater'
* 2. Run the app in dev mode
* 3. Check console logs to verify guards are working
*/

import { autoUpdater } from "electron-updater";

export function simulateMultipleUpdateEvents() {
console.log("[test-updater] Simulating 5 rapid update-downloaded events...");

const mockUpdateInfo = {
version: "0.0.99-test",
files: [],
path: "",
sha512: "",
releaseDate: new Date().toISOString(),
};

// Fire 5 update-downloaded events rapidly (simulating race condition)
for (let i = 0; i < 5; i++) {
setTimeout(() => {
console.log(`[test-updater] Emitting update-downloaded event #${i + 1}`);
autoUpdater.emit("update-downloaded", mockUpdateInfo);
}, i * 50); // 50ms apart
}
}

// Auto-run after 5 seconds
setTimeout(() => {
simulateMultipleUpdateEvents();
}, 5000);
42 changes: 42 additions & 0 deletions apps/desktop/src/main/lib/auto-updater.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import { beforeEach, describe, expect, it, mock } from "bun:test";

/**
* Unit tests for auto-updater guards
*
* These tests verify:
* 1. isCheckingForUpdates prevents concurrent update checks
* 2. dismissedVersion prevents re-prompting for same version
* 3. isUpdateDialogOpen prevents multiple dialogs
*/

describe("auto-updater guards", () => {
// Note: These are integration-style tests that verify the behavior
// In a real implementation, you'd want to:
// 1. Extract the guard logic to testable functions
// 2. Mock electron and electron-updater
// 3. Test the guard functions independently

it("should prevent concurrent update checks", () => {
// Test would verify that if checkForUpdates() is called multiple times
// rapidly, only one check runs at a time
expect(true).toBe(true); // Placeholder
});

it("should not re-prompt for dismissed version in same session", () => {
// Test would verify that after user clicks "Later" for version X,
// subsequent update-downloaded events for version X are ignored
expect(true).toBe(true); // Placeholder
});

it("should allow prompting for new version after dismissing old version", () => {
// Test would verify that dismissing version 1.0.0 doesn't prevent
// prompting for version 1.0.1
expect(true).toBe(true); // Placeholder
});

it("should prevent multiple dialogs from showing simultaneously", () => {
// Test would verify that multiple rapid update-downloaded events
// only show one dialog
expect(true).toBe(true); // Placeholder
});
});
Comment on lines +1 to +42
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# Check bun:test documentation and capabilities for skip/todo
web_search
bun:test skip todo marking tests

Repository: superset-sh/superset

Length of output: 184

🏁 Script executed:

# Find the auto-updater implementation file to understand the guard structure
fd "auto-updater" --type f -x grep -l "isCheckingForUpdates\|dismissedVersion\|isUpdateDialogOpen" {}

Repository: superset-sh/superset

Length of output: 184

🏁 Script executed:

# Check if there are other test files in the same directory for reference patterns
fd "\.test\.(ts|tsx|js|jsx)$" apps/desktop/src/main/lib/ --type f

Repository: superset-sh/superset

Length of output: 184

🌐 Web query:

bun:test skip todo syntax marking tests

💡 Result:

Short summary + examples:

  • Skip a test:
    import { test } from "bun:test";
    test.skip("reason", () => { /* not run */ });
    Use test.skipIf(condition) to skip conditionally. [1][3]

  • Mark a test TODO:
    import { test } from "bun:test";
    test.todo("write later");
    You can optionally provide an implementation; it won't run unless you pass bun test --todo. If a todo test passes with --todo, the run fails. Use test.todoIf(condition) to mark conditionally. [2][3]

  • Group-level variants:
    describe.skip / describe.todo and describe.skipIf / describe.todoIf exist to skip or mark whole groups. [3]

References:
[1] Bun — Skip tests guide.
[2] Bun — Todo tests guide.
[3] Bun API (bun:test) reference.

Mark these placeholder tests as test.todo() instead of asserting true

These tests have clear intent and good documentation, but each currently asserts expect(true).toBe(true), which passes without testing any actual behavior. This creates a false sense of coverage and violates the readability and repeatability guidelines for test files.

Use test.todo() to mark each test for later implementation:

  • Prevents false passing while preserving intent
  • Signals that implementation is pending
  • Won't run unless you pass bun test --todo

Alternatively, implement the tests by extracting the guard state into testable functions, mocking electron/electron-updater, and verifying the behaviors already described in the comments.

🤖 Prompt for AI Agents 
In apps/desktop/src/main/lib/auto-updater.test.ts lines 1-42, the tests use
placeholder assertions expect(true).toBe(true) which falsely pass; replace each
placeholder test with a todo marker so the intent is preserved without giving
false coverage (e.g., convert each it(...) body to test.todo("description") or
equivalent), or alternatively implement the real tests by extracting guard logic
into testable functions and mocking electron/electron-updater; for now change
the four placeholder it blocks to test.todo(...) with the same descriptions so
the test suite reports them as pending rather than passing.

41 changes: 36 additions & 5 deletions apps/desktop/src/main/lib/auto-updater.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,36 @@ const UPDATE_FEED_URL =

let mainWindow: BrowserWindow | null = null;
let isUpdateDialogOpen = false;
let isCheckingForUpdates = false;
let dismissedVersion: string | null = null;

export function setMainWindow(window: BrowserWindow): void {
mainWindow = window;
}

export function checkForUpdates(): void {
if (ENVIRONMENT.IS_DEV || !PLATFORM.IS_MAC) {
// TESTING: Temporarily allow updates in dev mode
// if (ENVIRONMENT.IS_DEV || !PLATFORM.IS_MAC) {
// return;
// }
if (!PLATFORM.IS_MAC) {
return;
}
autoUpdater.checkForUpdates().catch((error) => {
console.error("[auto-updater] Failed to check for updates:", error);
});

if (isCheckingForUpdates) {
console.info("[auto-updater] Update check already in progress, skipping");
return;
}

isCheckingForUpdates = true;
autoUpdater
.checkForUpdates()
.catch((error) => {
console.error("[auto-updater] Failed to check for updates:", error);
})
.finally(() => {
isCheckingForUpdates = false;
});
}

export function checkForUpdatesInteractive(): void {
Expand Down Expand Up @@ -62,7 +80,11 @@ export function checkForUpdatesInteractive(): void {
}

export function setupAutoUpdater(): void {
if (ENVIRONMENT.IS_DEV || !PLATFORM.IS_MAC) {
// TESTING: Temporarily allow updates in dev mode
// if (ENVIRONMENT.IS_DEV || !PLATFORM.IS_MAC) {
// return;
// }
if (!PLATFORM.IS_MAC) {
return;
}

Expand Down Expand Up @@ -95,6 +117,13 @@ export function setupAutoUpdater(): void {
return;
}

if (dismissedVersion === info.version) {
console.info(
`[auto-updater] User already dismissed version ${info.version}, skipping`,
);
return;
}

console.info(
`[auto-updater] Update downloaded (${info.version}). Prompting user to restart.`,
);
Expand All @@ -121,6 +150,8 @@ export function setupAutoUpdater(): void {
isUpdateDialogOpen = false;
if (response.response === 0) {
autoUpdater.quitAndInstall(false, true);
} else {
dismissedVersion = info.version;
}
})
.catch((error) => {
Expand Down