feat(electron): add auto-update functionality with electron-updater - #221
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces robust auto-update capabilities to the Electron application, significantly enhancing the user experience by ensuring that users always have the latest version without manual intervention. It streamlines the update process from checking and downloading to installation, complete with user notifications and integration into the application's system tray. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| nativeImage, | ||
| shell, | ||
| session, | ||
| dialog, |
There was a problem hiding this comment.
WARNING: Unused import - dialog is imported but never used in the codebase. Either remove this import or implement dialog functionality.
| // ── Auto-Updater Configuration ────────────────────────────── | ||
| autoUpdater.autoDownload = false; | ||
| autoUpdater.autoInstallOnAppQuit = true; | ||
| autoUpdater.logger = console; |
There was a problem hiding this comment.
SUGGESTION: Using console as the logger could expose sensitive information in production. Consider using a proper logger or setting up environment-specific logging levels.
| body: `Version ${info.version} is ready to install. Click to restart.`, | ||
| }); | ||
| notification.on("click", () => { | ||
| autoUpdater.quitAndInstall(); |
There was a problem hiding this comment.
WARNING: quitAndInstall() is called without any cleanup of the Next.js server or saving application state. This could lead to data loss or incomplete operations. Consider graceful shutdown before installing updates.
| return { success: true }; | ||
| }); | ||
|
|
||
| ipcMain.handle("get-app-version", () => app.getVersion()); |
There was a problem hiding this comment.
CRITICAL: The get-app-version handler doesn't return the version. It calls app.getVersion() but doesn't return the result, so callers will always receive undefined instead of the actual version string.
| // Auto-update IPC handlers | ||
| ipcMain.handle("check-for-updates", async () => { | ||
| await checkForUpdates(false); | ||
| return { success: true }; |
There was a problem hiding this comment.
WARNING: This IPC handler returns { success: true } regardless of whether the update check actually succeeded or failed. The renderer process cannot distinguish between success and failure. Consider returning error information when the update check fails.
|
|
||
| ipcMain.handle("download-update", async () => { | ||
| await downloadUpdate(); | ||
| return { success: true }; |
There was a problem hiding this comment.
WARNING: This IPC handler returns { success: true } even if downloadUpdate() throws an error. The error is logged but not returned to the caller, making it impossible for the renderer to know if the download failed.
|
|
||
| ipcMain.handle("install-update", () => { | ||
| installUpdate(); | ||
| return { success: true }; |
There was a problem hiding this comment.
WARNING: This IPC handler returns { success: true } even though installUpdate() (quitAndInstall) will quit the application immediately. The return value is meaningless in this context.
Code Review SummaryStatus: 11 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
MEDIUM
Other Observations (not in diff)No additional issues found in unchanged code. Files Reviewed (3 files)
Fix Link: Fix these issues in Kilo Cloud |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a solid foundation for auto-updates using electron-updater. The implementation in the main process is well-structured, covering event handling, IPC communication, and user notifications. My feedback includes suggestions to improve error propagation for IPC handlers and align with best practices for configuration and logging. A key point to consider is that the feature appears incomplete on the renderer side; the new APIs exposed in preload.js are not yet consumed by any frontend hooks or components, and the TypeScript definitions have not been updated. This will need to be addressed for the feature to be fully functional.
| console.error("[Electron] Check for updates failed:", error); | ||
| if (!silent) { | ||
| sendToRenderer("update-status", { status: "error", message: error.message }); | ||
| } |
There was a problem hiding this comment.
This catch block currently handles the error by logging it and sending an IPC message, but it effectively swallows the error. This prevents the promise returned by ipcMain.handle from rejecting, which can be misleading for the renderer process. To ensure the renderer can properly handle failures (e.g., in a try/catch block around the invoke call), you should re-throw the error after logging it.
console.error("[Electron] Check for updates failed:", error);
if (!silent) {
sendToRenderer("update-status", { status: "error", message: error.message });
}
throw error;| console.error("[Electron] Download update failed:", error); | ||
| sendToRenderer("update-status", { status: "error", message: error.message }); |
There was a problem hiding this comment.
Similar to the checkForUpdates function, this catch block swallows the error. This prevents the corresponding ipcMain.handle from rejecting on failure. You should re-throw the error here as well to ensure the renderer process is aware of the failure and can handle it appropriately.
console.error("[Electron] Download update failed:", error);
sendToRenderer("update-status", { status: "error", message: error.message });
throw error;| // ── Auto-Updater Configuration ────────────────────────────── | ||
| autoUpdater.autoDownload = false; | ||
| autoUpdater.autoInstallOnAppQuit = true; | ||
| autoUpdater.logger = console; |
There was a problem hiding this comment.
While using console for logging is acceptable during development, electron-updater strongly recommends using electron-log in production. It provides out-of-the-box file logging, which is invaluable for diagnosing update-related issues on user machines. You'll need to add electron-log to your dependencies in package.json.
| autoUpdater.logger = console; | |
| autoUpdater.logger = require("electron-log"); |
| "publish": { | ||
| "provider": "github", | ||
| "owner": "diegosouzapw", | ||
| "repo": "OmniRoute" | ||
| }, |
There was a problem hiding this comment.
Hardcoding the owner and repo here works, but it makes the configuration less portable. A more maintainable approach is to let electron-builder infer these values from a repository field in your package.json. You can remove the owner and repo keys and add a top-level repository field instead:
"repository": {
"type": "git",
"url": "https://github.com/diegosouzapw/OmniRoute.git"
} "publish": {
"provider": "github"
},- Remove unused dialog import - Stop Next.js server before quitAndInstall() to prevent data loss - Propagate errors from checkForUpdates/downloadUpdate to IPC handlers so renderer can distinguish success from failure - Remove meaningless return value from install-update handler
|
Thanks @benzntech for this great contribution! 🎉 The auto-update functionality is now merged and will be part of the next release. We appreciate your effort in adding this important feature! |
feat(electron): add auto-update functionality with electron-updater
Two more HIGH alerts arrived after the first sweep: #220 js-yaml (root package-lock.json) >= 4.0.0, < 4.3.2 #219 js-yaml (electron/package-lock.json) >= 4.0.0, < 4.3.2 The root's own js-yaml was already on 5.4.1; the vulnerable copies were the ones nested under @yarnpkg/parsers, lockfile-lint, xmlbuilder2 (root) and the direct dependency in electron. All now 4.3.2. Four version lines, nothing else. #221 smol-toml (HIGH, <= 1.7.0) is NOT closed here. The root is on 1.8.0; the vulnerable 1.6.1 sits under @openai/codex-security, which pins it as an EXACT version rather than a range, so `npm update` cannot move it. Bumping codex-security itself (0.1.24 -> 0.1.26) does not help — 0.1.26 pins the same 1.6.1 — so that bump was reverted rather than carried along for no benefit. Closing #221 needs an upstream codex-security release or an `overrides` entry, the same trade already declined for #210/tsup: forcing a transitive pin from outside is how a build breaks silently. Note that @openai/codex-security is also the package carrying the unpatched extract-zip (#218), so one upstream release would likely clear both.
Closes #221 (smol-toml, HIGH, DoS via malformed TOML, vulnerable <= 1.7.0). @openai/codex-security pins smol-toml at 1.6.1 as an EXACT version, so no `npm update` reaches it. This repo already uses `overrides` as its standard tool for exactly that situation — the block carries 20+ entries, including the scoped-by-parent form and the `qs`/`fast-uri`/`ip-address` entries that back earlier security bumps — so a scoped override is the idiomatic fix here, not a new mechanism: "@openai/codex-security": { "smol-toml": "^1.8.0" } The nested copy deduplicates to the root's existing 1.8.0, which two other consumers (the root itself and knip) already run, so the version is proven in this tree. The whole lockfile diff is the 14 lines of the removed 1.6.1 entry. Also raised the `@yarnpkg/parsers` js-yaml floor from ^4.3.1 to ^4.3.2, so the override documents the patched version rather than permitting the vulnerable one it was written against. Not fixed, and not fixable by version — verified against the npm registry rather than trusting the advisory metadata: #218 extract-zip — latest published IS 2.0.1, the vulnerable version. Dev scope, via @openai/codex-security. No release to move to. #214 adm-zip — latest published IS 0.6.0, the top of the vulnerable range (>= 0.5.9, <= 0.6.0). RUNTIME scope, via onnxruntime-node's ^0.5.16, and the repo already overrides adm-zip to ^0.6.0. No release to move to. Both need an upstream fix or a decision to replace the dependency; neither is a lockfile change. adm-zip being runtime rather than dev makes it the one worth tracking. #210 esbuild stays open too. A flat `overrides: { esbuild: ^0.28.2 }` in opencode-plugin-v2 does close it — npm then reports 0 vulnerabilities — but it requires regenerating that lockfile from scratch: 823 lines, 96 packages moved, for a LOW dev-only alert, and a major esbuild bump inside tsup cannot be validated here without a real install of that package. Tried, measured, reverted. Left for an upstream tsup release. check:lockfile OK on all lockfiles including the workspace consistency check; check:tracked-artifacts OK; prettier clean.
* chore(deps): drain the Dependabot queue — 7 of 10 alerts Lockfile-only bumps; no manifest touched, so nothing changes for consumers. Root package-lock.json: hono 4.13.0 -> 4.13.7 (#215 #216 #217, medium, patched 4.13.5) csv-parse 7.0.1 -> 7.0.2 (#213, medium) joi 18.2.3 -> 18.2.8 (#211 #212, low, patched 18.2.4/18.2.5) @omniroute/opencode-plugin: toml 4.1.1 -> 4.3.0 (#209, HIGH, patched 4.1.2) @omniroute/opencode-plugin-v2: esbuild 0.28.1 -> 0.28.2 (#210, low) — the direct copy only; see below. The plugin-v2 diff looks large but is one package: esbuild ships 27 platform binaries, each carrying version + resolved + integrity. Three alerts stay open, deliberately: #218 extract-zip (HIGH) and #214 adm-zip (medium) have NO published patch. Both are dev-scope. Closing them needs an upstream release or a decision to replace the dependency — neither belongs in a lockfile bump. #210 esbuild is only half-closed. `node_modules/esbuild` is on 0.28.2, but `tsup` pins `esbuild: ^0.27.0`, so its nested copy stays at 0.27.7 — inside the vulnerable range (>= 0.27.3, < 0.28.1). Updating tsup does not move it (8.5.1 is already current). Forcing it would take an `overrides` entry pushing a major of esbuild inside the bundler, which is exactly the change that breaks a build silently, for a LOW dev-only alert. Left for an upstream tsup release. check:lockfile passes on all three, including the workspace lock/manifest consistency check. check:tracked-artifacts OK. * chore(deps): bump js-yaml to 4.3.2 (root + electron) Two more HIGH alerts arrived after the first sweep: #220 js-yaml (root package-lock.json) >= 4.0.0, < 4.3.2 #219 js-yaml (electron/package-lock.json) >= 4.0.0, < 4.3.2 The root's own js-yaml was already on 5.4.1; the vulnerable copies were the ones nested under @yarnpkg/parsers, lockfile-lint, xmlbuilder2 (root) and the direct dependency in electron. All now 4.3.2. Four version lines, nothing else. #221 smol-toml (HIGH, <= 1.7.0) is NOT closed here. The root is on 1.8.0; the vulnerable 1.6.1 sits under @openai/codex-security, which pins it as an EXACT version rather than a range, so `npm update` cannot move it. Bumping codex-security itself (0.1.24 -> 0.1.26) does not help — 0.1.26 pins the same 1.6.1 — so that bump was reverted rather than carried along for no benefit. Closing #221 needs an upstream codex-security release or an `overrides` entry, the same trade already declined for #210/tsup: forcing a transitive pin from outside is how a build breaks silently. Note that @openai/codex-security is also the package carrying the unpatched extract-zip (#218), so one upstream release would likely clear both. * chore(deps): override smol-toml to 1.8.0 and raise the js-yaml floor Closes #221 (smol-toml, HIGH, DoS via malformed TOML, vulnerable <= 1.7.0). @openai/codex-security pins smol-toml at 1.6.1 as an EXACT version, so no `npm update` reaches it. This repo already uses `overrides` as its standard tool for exactly that situation — the block carries 20+ entries, including the scoped-by-parent form and the `qs`/`fast-uri`/`ip-address` entries that back earlier security bumps — so a scoped override is the idiomatic fix here, not a new mechanism: "@openai/codex-security": { "smol-toml": "^1.8.0" } The nested copy deduplicates to the root's existing 1.8.0, which two other consumers (the root itself and knip) already run, so the version is proven in this tree. The whole lockfile diff is the 14 lines of the removed 1.6.1 entry. Also raised the `@yarnpkg/parsers` js-yaml floor from ^4.3.1 to ^4.3.2, so the override documents the patched version rather than permitting the vulnerable one it was written against. Not fixed, and not fixable by version — verified against the npm registry rather than trusting the advisory metadata: #218 extract-zip — latest published IS 2.0.1, the vulnerable version. Dev scope, via @openai/codex-security. No release to move to. #214 adm-zip — latest published IS 0.6.0, the top of the vulnerable range (>= 0.5.9, <= 0.6.0). RUNTIME scope, via onnxruntime-node's ^0.5.16, and the repo already overrides adm-zip to ^0.6.0. No release to move to. Both need an upstream fix or a decision to replace the dependency; neither is a lockfile change. adm-zip being runtime rather than dev makes it the one worth tracking. #210 esbuild stays open too. A flat `overrides: { esbuild: ^0.28.2 }` in opencode-plugin-v2 does close it — npm then reports 0 vulnerabilities — but it requires regenerating that lockfile from scratch: 823 lines, 96 packages moved, for a LOW dev-only alert, and a major esbuild bump inside tsup cannot be validated here without a real install of that package. Tried, measured, reverted. Left for an upstream tsup release. check:lockfile OK on all lockfiles including the workspace consistency check; check:tracked-artifacts OK; prettier clean.
2 participants
Summary
electron-updaterdependency for auto-update supportChanges
electron/package.json: Added electron-updater dependency and GitHub publish configelectron/main.js: Auto-updater setup, IPC handlers, tray menu integrationelectron/preload.js: Exposed checkForUpdates, downloadUpdate, installUpdate, onUpdateStatus APIs