Fix auto updater modal stacking up#304
Conversation
|
@arnaudbreton is attempting to deploy a commit to the superset Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis pull request adds runtime testing hooks and guard logic to the auto-updater system. It introduces a test utility module that simulates rapid update-downloaded events, adds placeholder integration tests, and modifies the auto-updater to prevent concurrent checks, suppress duplicate prompts for dismissed versions, and restrict updates to macOS. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/desktop/src/main/lib/__test-updater.ts (1)
1-35: Make the test helper side‑effect free and update the usage docRight now this module auto‑runs
simulateMultipleUpdateEvents()5 seconds after import and the usage comment still suggests a (slightly wrong) static import path. That’s surprising in asrc/main/libfile and becomes problematic once it’s wired fromindex.ts, since any import (including production) will silently schedule simulated update events.Consider:
- Removing the module‑level auto‑run block and keeping this as a pure helper that callers invoke explicitly.
- Updating the usage comment (or moving it to where you wire this up) so it reflects the actual, dev‑only import pattern and a correct path.
- Optionally annotating
mockUpdateInfowith the appropriateUpdateInfotype fromelectron-updaterfor clarity.This keeps test tooling clearly opt‑in and avoids accidental behavior changes if the helper is ever imported outside of a dev scenario.
apps/desktop/src/main/lib/auto-updater.ts (1)
11-41: Guards correctly address stacked dialogs; clarify dev gating and consider minor consistency tweaksThe new guard logic looks solid:
isCheckingForUpdatesprevents overlapping background checks and is safely reset in a.finally().dismissedVersionplusisUpdateDialogOpenmeans:
- Only one dialog shows while it’s open.
- After “Later” on a given version, further
update-downloadedevents for that same version are ignored for the rest of the session.This directly targets the modal‑stacking issue.
A couple of follow‑ups to consider:
- The
ENVIRONMENT.IS_DEVchecks in bothcheckForUpdatesandsetupAutoUpdaterare now commented out, so automatic checks do run in dev, whilecheckForUpdatesInteractivestill reports “Auto-updates are disabled in development mode.” If that’s intentional, it’d be good to:
- Remove or reword the
// TESTING: Temporarily allow updates in dev modecomments, or- Reintroduce a consistent gating strategy (e.g., allow in both dev and prod, or disable in both for non‑macOS).
- If overlapping checks from the interactive path were part of the original problem, you might also want to reuse
isCheckingForUpdatesincheckForUpdatesInteractive()to ensure there’s at most one in‑flight check at a time.These are mostly consistency/clarity points; the core fix for stacked dialogs and re‑prompting behavior looks correct.
Also applies to: 83-89, 120-125, 154-155
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src/main/index.ts(1 hunks)apps/desktop/src/main/lib/__test-updater.ts(1 hunks)apps/desktop/src/main/lib/auto-updater.test.ts(1 hunks)apps/desktop/src/main/lib/auto-updater.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/main/lib/__test-updater.tsapps/desktop/src/main/lib/auto-updater.test.tsapps/desktop/src/main/lib/auto-updater.tsapps/desktop/src/main/index.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/main/lib/__test-updater.tsapps/desktop/src/main/lib/auto-updater.test.tsapps/desktop/src/main/lib/auto-updater.tsapps/desktop/src/main/index.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Maintain type safety and avoid using
anyunless absolutely necessary in TypeScript code
Files:
apps/desktop/src/main/lib/__test-updater.tsapps/desktop/src/main/lib/auto-updater.test.tsapps/desktop/src/main/lib/auto-updater.tsapps/desktop/src/main/index.ts
apps/desktop/**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.test.{ts,tsx,js,jsx}: Tests should have one assert per test
Tests should be readable
Tests should be fast
Tests should be independent
Tests should be repeatable
Files:
apps/desktop/src/main/lib/auto-updater.test.ts
apps/desktop/src/main/index.ts
📄 CodeRabbit inference engine (AGENTS.md)
Load environment variables from monorepo root .env file in src/main/index.ts with override: true
Files:
apps/desktop/src/main/index.ts
🧠 Learnings (7)
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.test.{ts,tsx,js,jsx} : Tests should be repeatable
Applied to files:
apps/desktop/src/main/lib/__test-updater.tsapps/desktop/src/main/lib/auto-updater.test.tsapps/desktop/src/main/index.ts
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.test.{ts,tsx,js,jsx} : Tests should be independent
Applied to files:
apps/desktop/src/main/lib/__test-updater.tsapps/desktop/src/main/lib/auto-updater.test.tsapps/desktop/src/main/index.ts
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.test.{ts,tsx,js,jsx} : Tests should be fast
Applied to files:
apps/desktop/src/main/lib/__test-updater.tsapps/desktop/src/main/lib/auto-updater.test.tsapps/desktop/src/main/index.ts
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.test.{ts,tsx,js,jsx} : Tests should be readable
Applied to files:
apps/desktop/src/main/lib/auto-updater.test.tsapps/desktop/src/main/index.ts
📚 Learning: 2025-12-08T23:32:32.232Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T23:32:32.232Z
Learning: Applies to apps/desktop/src/main/lib/*{workspace,worktree}-*.ts : Use git worktree-based workspace management in the desktop app through workspace-manager.ts and worktree-manager.ts
Applied to files:
apps/desktop/src/main/lib/auto-updater.test.ts
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.test.{ts,tsx,js,jsx} : Tests should have one assert per test
Applied to files:
apps/desktop/src/main/lib/auto-updater.test.ts
📚 Learning: 2025-12-08T23:32:32.232Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T23:32:32.232Z
Learning: Applies to apps/desktop/src/renderer/**/*.{ts,tsx} : Never import Node.js modules in renderer process or shared code - only in src/main/
Applied to files:
apps/desktop/src/main/index.ts
🧬 Code graph analysis (1)
apps/desktop/src/main/lib/auto-updater.ts (1)
apps/desktop/src/shared/constants.ts (1)
PLATFORM(5-9)
| // 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); | ||
| }); |
There was a problem hiding this comment.
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.
| 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 | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check bun:test documentation and capabilities for skip/todo
web_search
bun:test skip todo marking testsRepository: 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 fRepository: 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.
|
hey @arnaudbreton thanks for the PR! |
|
Closing as fixed, thanks for the PR! |
|
Thanks @Kitenite, sorry for the delay i was working out of my local build for a few days and I finally reverted to the main version today, I haven't seen the issue. Love the tool! |
|
@arnaudbreton great to hear! Let me know if you run into any issues or have requests :) |
2 participants
Description
I tried to use Claude Code to fix an issue where the auto updater modal keep stacking up when checking updates without feedback from the user.
Related Issues
Type of Change
Testing
Used the app locally for a full day without issue
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.