feat: auto-update notification UI #81
Conversation
WalkthroughThis update introduces a robust application update notification and handling system for the Electron app. It adds a singleton updater service, IPC channels for update checks and dialogs, a renderer-side React notification component, and new menu integration. Configuration files and typings are revised to support the new update flow, with legacy API compatibility maintained. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Renderer (UI)
participant Preload (Bridge)
participant Main (Electron)
participant AppUpdater (Singleton)
participant GitHub (Update Provider)
User->>Renderer (UI): Click "Check for Updates..."
Renderer (UI)->>Preload (Bridge): window.api.app.checkForUpdate()
Preload (Bridge)->>Main (Electron): IPC "app:check-for-update"
Main (Electron)->>AppUpdater (Singleton): getAppUpdater().checkForUpdates()
AppUpdater (Singleton)->>GitHub (Update Provider): Fetch update info
GitHub (Update Provider)-->>AppUpdater (Singleton): Update info / none
AppUpdater (Singleton)-->>Main (Electron): Update info
Main (Electron)-->>Preload (Bridge): IPC response
Preload (Bridge)-->>Renderer (UI): Promise resolves
Renderer (UI)->>Renderer (UI): Show notification / progress
Note over Renderer (UI),Main (Electron): Update events (available, progress, downloaded, error) are forwarded via IPC and displayed in UI.
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Claude finished @michiosw's task —— View job Code Review for Auto-Update Notification UIReview Progress:
📋 SummaryThis PR implements a comprehensive auto-update notification system with clean UI and proper state management. The implementation follows good patterns but has some areas for improvement. ✅ StrengthsCode Quality
User Experience
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
apps/electron-app/src/renderer/src/components/UpdateNotification.tsx (2)
30-30: Add proper typing for event parameters.The event handlers are using
anytype for the event parameter, which reduces type safety. Consider adding proper typing or use_prefix if the parameter is unused.- const handleUpdateAvailable = (_event: any, info: UpdateInfo) => { + const handleUpdateAvailable = (_event: unknown, info: UpdateInfo) => {Also applies to: 38-40, 46-46, 52-52
137-146: Consider extracting status text logic for better readability.The nested ternary operators make the status text logic hard to follow. Consider extracting this into a helper function.
+ const getStatusText = () => { + if (error) return "Update Error"; + if (showNoUpdate) return "Up to date"; + if (checking) return "Checking..."; + if (updateReady) return `v${updateInfo?.version} ready`; + return `v${updateInfo?.version} available`; + }; + <p className="font-medium text-gray-900 dark:text-gray-100 truncate"> - {error - ? "Update Error" - : showNoUpdate - ? "Up to date" - : checking - ? "Checking..." - : updateReady - ? `v${updateInfo?.version} ready` - : `v${updateInfo?.version} available`} + {getStatusText()} </p>apps/electron-app/src/main/ipc/app/update.ts (1)
75-75: Improve type safety by avoidingas anyassertion.The
as anytype assertion reduces type safety. Consider using a more specific type or union type for the event names.+ type UpdateEventName = + | "checking-for-update" + | "update-available" + | "update-not-available" + | "download-progress" + | "update-downloaded" + | "error"; + events.forEach(eventName => { - updater.autoUpdater.on(eventName as any, (...args: any[]) => { + updater.autoUpdater.on(eventName as UpdateEventName, (...args: any[]) => {apps/electron-app/src/main/services/update-service.ts (1)
19-21: Remove unnecessary empty constructor.The static analysis correctly identifies that this empty constructor is unnecessary for the singleton pattern.
- constructor() { - // Private constructor for singleton pattern - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
apps/electron-app/app-update.yml(1 hunks)apps/electron-app/dev-app-update.yml(0 hunks)apps/electron-app/electron-builder.js(2 hunks)apps/electron-app/src/main/index.ts(2 hunks)apps/electron-app/src/main/ipc/app/update.ts(1 hunks)apps/electron-app/src/main/ipc/index.ts(2 hunks)apps/electron-app/src/main/menu/items/help.ts(2 hunks)apps/electron-app/src/main/services/index.ts(1 hunks)apps/electron-app/src/main/services/update-service.ts(3 hunks)apps/electron-app/src/preload/index.ts(1 hunks)apps/electron-app/src/renderer/src/App.tsx(2 hunks)apps/electron-app/src/renderer/src/components/UpdateNotification.tsx(1 hunks)apps/electron-app/src/renderer/src/components/main/MainApp.tsx(1 hunks)apps/electron-app/src/renderer/src/env.d.ts(1 hunks)packages/shared-types/src/constants/index.ts(0 hunks)packages/shared-types/src/interfaces/index.ts(1 hunks)
💤 Files with no reviewable changes (2)
- apps/electron-app/dev-app-update.yml
- packages/shared-types/src/constants/index.ts
🧰 Additional context used
🧠 Learnings (8)
apps/electron-app/src/renderer/src/components/main/MainApp.tsx (1)
Learnt from: maceip
PR: co-browser/vibe#37
File: apps/electron-app/src/main/store/desktop-store.ts:0-0
Timestamp: 2025-07-01T18:22:27.574Z
Learning: In the desktop store implementation (apps/electron-app/src/main/store/desktop-store.ts), the team takes a performance-data-driven approach to optimization decisions. They prefer to keep the current implementation (including double encryption scenarios) until they can properly evaluate performance impact, rather than making premature optimizations.
apps/electron-app/app-update.yml (1)
Learnt from: maceip
PR: co-browser/vibe#38
File: apps/electron-app/src/main/services/profile-service.ts:131-135
Timestamp: 2025-07-01T18:24:21.101Z
Learning: In the ProfileService implementation (apps/electron-app/src/main/services/profile-service.ts), the team prefers to keep the updateProfile method simple without filtering out immutable fields like `id` and `createdAt` from updates, since these fields are not currently being used elsewhere in the codebase. They prioritize code simplicity over theoretical protection against field overwriting.
apps/electron-app/src/renderer/src/App.tsx (1)
Learnt from: maceip
PR: co-browser/vibe#38
File: apps/electron-app/src/main/services/profile-service.ts:131-135
Timestamp: 2025-07-01T18:24:21.101Z
Learning: In the ProfileService implementation (apps/electron-app/src/main/services/profile-service.ts), the team prefers to keep the updateProfile method simple without filtering out immutable fields like `id` and `createdAt` from updates, since these fields are not currently being used elsewhere in the codebase. They prioritize code simplicity over theoretical protection against field overwriting.
apps/electron-app/src/main/index.ts (3)
Learnt from: maceip
PR: co-browser/vibe#38
File: apps/electron-app/src/main/services/profile-service.ts:131-135
Timestamp: 2025-07-01T18:24:21.101Z
Learning: In the ProfileService implementation (apps/electron-app/src/main/services/profile-service.ts), the team prefers to keep the updateProfile method simple without filtering out immutable fields like `id` and `createdAt` from updates, since these fields are not currently being used elsewhere in the codebase. They prioritize code simplicity over theoretical protection against field overwriting.
Learnt from: maceip
PR: co-browser/vibe#37
File: apps/electron-app/src/main/store/desktop-store.ts:0-0
Timestamp: 2025-07-01T18:22:27.574Z
Learning: In the desktop store implementation (apps/electron-app/src/main/store/desktop-store.ts), the team takes a performance-data-driven approach to optimization decisions. They prefer to keep the current implementation (including double encryption scenarios) until they can properly evaluate performance impact, rather than making premature optimizations.
Learnt from: maceip
PR: co-browser/vibe#37
File: apps/electron-app/src/main/store/desktop-store.ts:277-294
Timestamp: 2025-07-01T15:07:39.741Z
Learning: In apps/electron-app/src/main/store/desktop-store.ts, the user prefers to maintain key-level encryption in the helper functions getRecoveredData and getAllRecoveredData rather than removing the double encryption approach.
apps/electron-app/src/main/services/index.ts (1)
Learnt from: maceip
PR: co-browser/vibe#38
File: apps/electron-app/src/main/services/profile-service.ts:131-135
Timestamp: 2025-07-01T18:24:21.101Z
Learning: In the ProfileService implementation (apps/electron-app/src/main/services/profile-service.ts), the team prefers to keep the updateProfile method simple without filtering out immutable fields like `id` and `createdAt` from updates, since these fields are not currently being used elsewhere in the codebase. They prioritize code simplicity over theoretical protection against field overwriting.
apps/electron-app/src/preload/index.ts (1)
Learnt from: maceip
PR: co-browser/vibe#38
File: apps/electron-app/src/main/services/profile-service.ts:131-135
Timestamp: 2025-07-01T18:24:21.101Z
Learning: In the ProfileService implementation (apps/electron-app/src/main/services/profile-service.ts), the team prefers to keep the updateProfile method simple without filtering out immutable fields like `id` and `createdAt` from updates, since these fields are not currently being used elsewhere in the codebase. They prioritize code simplicity over theoretical protection against field overwriting.
apps/electron-app/src/renderer/src/env.d.ts (1)
Learnt from: maceip
PR: co-browser/vibe#37
File: apps/electron-app/src/main/store/desktop-store.ts:0-0
Timestamp: 2025-07-01T18:22:27.574Z
Learning: In the desktop store implementation (apps/electron-app/src/main/store/desktop-store.ts), the team takes a performance-data-driven approach to optimization decisions. They prefer to keep the current implementation (including double encryption scenarios) until they can properly evaluate performance impact, rather than making premature optimizations.
apps/electron-app/src/main/services/update-service.ts (1)
Learnt from: maceip
PR: co-browser/vibe#38
File: apps/electron-app/src/main/services/profile-service.ts:131-135
Timestamp: 2025-07-01T18:24:21.101Z
Learning: In the ProfileService implementation (apps/electron-app/src/main/services/profile-service.ts), the team prefers to keep the updateProfile method simple without filtering out immutable fields like `id` and `createdAt` from updates, since these fields are not currently being used elsewhere in the codebase. They prioritize code simplicity over theoretical protection against field overwriting.
🧬 Code Graph Analysis (4)
apps/electron-app/src/main/ipc/index.ts (1)
apps/electron-app/src/main/ipc/app/update.ts (2)
registerUpdateHandlers(10-52)setupUpdateEventForwarding(57-86)
apps/electron-app/src/renderer/src/App.tsx (1)
apps/electron-app/src/renderer/src/components/UpdateNotification.tsx (1)
UpdateNotification(16-186)
apps/electron-app/src/main/index.ts (1)
apps/electron-app/src/main/services/index.ts (1)
getAppUpdater(22-29)
apps/electron-app/src/main/services/index.ts (1)
apps/electron-app/src/main/services/update-service.ts (1)
AppUpdater(13-188)
🪛 YAMLlint (1.37.1)
apps/electron-app/app-update.yml
[error] 3-3: no new line character at the end of file
(new-line-at-end-of-file)
🪛 Biome (1.9.4)
apps/electron-app/src/main/services/update-service.ts
[error] 19-21: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
🔇 Additional comments (13)
apps/electron-app/src/renderer/src/components/main/MainApp.tsx (1)
13-13: Good refactoring to centralize type definitions.Moving the Window interface declaration to a central location (
src/renderer/src/env.d.ts) eliminates duplication and improves maintainability. The comment clearly indicates where the declarations can now be found.apps/electron-app/src/main/ipc/index.ts (2)
13-16: LGTM: Clean integration of update IPC handlers.The import and integration of update handlers is well-structured and follows the existing pattern in the file.
57-59: LGTM: Proper initialization of update functionality.The registration of update handlers and event forwarding is correctly placed in the initialization sequence, ensuring update functionality is available when the application starts.
apps/electron-app/src/renderer/src/App.tsx (2)
9-9: LGTM: Clean import of UpdateNotification component.The import follows the existing pattern and is correctly placed.
28-33: LGTM: Proper integration of update notification UI.The UpdateNotification component is correctly added to the App component, ensuring it renders globally across all routes. The use of a React fragment maintains clean JSX structure.
apps/electron-app/electron-builder.js (2)
14-14: LGTM: Proper inclusion of new update configuration file.The
app-update.ymlfile is correctly included in the build files to support the new GitHub-based update system.
24-27: LGTM: Correct configuration of update file as extra resource.The
app-update.ymlfile is properly configured as an extra resource with explicit source and destination paths, ensuring it's accessible at runtime for the update system.apps/electron-app/src/main/services/index.ts (1)
15-29: Good implementation of safe singleton accessor.The
getAppUpdater()function provides a robust way to access the singleton instance with proper error handling, preventing exceptions from propagating to calling code.apps/electron-app/src/preload/index.ts (1)
706-717: Appropriate backward compatibility bridge.The temporary bridge properly exposes update methods from the new
updateAPIwhile maintaining legacy API compatibility. The generic event listener methods provide necessary flexibility for existing consumers.apps/electron-app/src/main/index.ts (2)
38-38: Good use of the safe accessor pattern.Using
getAppUpdaterinstead of direct import aligns with the new safe singleton access pattern.
736-741: Proper initialization sequence with safety checks.The conditional check prevents errors if the updater instance can't be created, and the explicit
initialize()call ensures proper setup before checking for updates.packages/shared-types/src/interfaces/index.ts (1)
118-129: Excellent interface improvement with strict typing.The transition from a generic interface to strictly typed methods provides better type safety and clearer API contracts. The return types are well-defined and appropriate for the update functionality.
apps/electron-app/src/main/services/update-service.ts (1)
124-124: Confirmedapp.isQuittingis defined
TheisQuittingboolean is declared inapps/electron-app/src/main/electron.d.ts, so no changes are needed.
apps/electron-app/src/renderer/src/components/UpdateNotification.tsx
Outdated
Show resolved
Hide resolved
|
Claude finished @michiosw's task —— View job Code Review for Auto-Update Notification UIReview Progress:
📋 SummaryThis PR implements a comprehensive auto-update notification system with clean UI and proper state management. The implementation follows good patterns but has some areas for improvement. ✅ StrengthsCode Quality
User Experience
|
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
apps/electron-app/src/main/services/update-service.ts (2)
154-168: The update installation flow has been improved.The new implementation addresses the previous concern about aggressive timeouts by using a more reasonable 100ms delay instead of 1 second force quit. The window closing is also properly handled with error catching.
However, the comment mentions that data persistence is handled by "gracefulShutdown mechanism" but this should be verified to ensure no data loss occurs.
#!/bin/bash # Description: Verify that gracefulShutdown mechanism exists and handles data persistence # Expected: Find gracefulShutdown implementation that saves data before app exit # Search for gracefulShutdown implementation rg -A 10 "gracefulShutdown" --type ts # Search for app quit handlers that might save data rg -A 5 "before-quit|will-quit|window-all-closed" --type ts # Search for data persistence logic during app shutdown rg -A 5 "save.*before.*quit|persist.*shutdown|cleanup.*exit" --type ts
154-168: Update installation flow still needs improvement.The implementation still has the same issues mentioned in previous reviews:
- Closing all windows without explicit data persistence verification
- Relying on a comment about "gracefulShutdown mechanism" without visible implementation
- Very short 100ms delay before quitAndInstall
Consider implementing explicit data persistence checks:
// Close all windows first - // Note: Data persistence is handled by the app's gracefulShutdown mechanism + // Ensure all windows save their data before closing + const windows = BrowserWindow.getAllWindows(); + const closePromises = windows.map(win => { + return new Promise<void>((resolve) => { + win.webContents.send('prepare-for-update'); + win.once('closed', resolve); + setTimeout(() => { + try { + win.close(); + } catch (e) { + logger.error("Error closing window:", e); + } + resolve(); + }, 1000); // Give more time for cleanup + }); + }); + + Promise.all(closePromises).then(() => { + logger.info("All windows closed, calling quitAndInstall..."); + autoUpdater.quitAndInstall(false, true); + }); - BrowserWindow.getAllWindows().forEach(win => { - try { - win.close(); - } catch (e) { - logger.error("Error closing window:", e); - } - }); - - // Add delay to ensure windows are closed - setTimeout(() => { - logger.info("Calling quitAndInstall..."); - autoUpdater.quitAndInstall(false, true); - }, 100);
🧹 Nitpick comments (9)
apps/electron-app/src/renderer/src/components/UpdateNotification.tsx (6)
87-88: Use optional chaining for safer property access.The static analysis tool correctly identifies that this can be simplified using optional chaining.
- const api = window.api as any; - if (api._events && api._events[channel]) { + const api = window.api as any; + if (api._events?.[channel]) {
117-122: Consider user feedback for update dialog errors.While logging errors is good, users might be confused if the install button doesn't work without any visible feedback.
const handleInstallUpdate = async () => { try { await window.vibe.update.showUpdateDialog(); } catch (error) { - // Log error but don't show to user console.error("Failed to show update dialog:", error); + // Optionally show user-friendly error + setError("Failed to show update dialog"); + setTimeout(() => setError(null), 3000); } };
16-26: Consider using useReducer for complex state management.The component has 8 different state variables that are often updated together. This could benefit from useReducer for better state management and fewer re-renders.
This would consolidate state updates and make the component more predictable:
interface UpdateState { updateAvailable: boolean; updateInfo: UpdateInfo | null; downloading: boolean; downloadProgress: DownloadProgress | null; updateReady: boolean; dismissed: boolean; checking: boolean; error: string | null; showNoUpdate: boolean; } const updateReducer = (state: UpdateState, action: any) => { switch (action.type) { case 'UPDATE_AVAILABLE': return { ...state, updateInfo: action.payload, updateAvailable: true, dismissed: false, checking: false, error: null }; // ... other cases } };
87-88: Use optional chaining for cleaner code.The static analysis tool correctly identifies an opportunity to use optional chaining.
- const api = window.api as any; - if (api._events && api._events[channel]) { + const api = window.api as any; + if (api._events?.[channel]) {
33-39: Consider adding type safety for event handlers.The event handler parameters use
anytypes, which reduces type safety.- const handleUpdateAvailable = (_event: any, info: UpdateInfo) => { + const handleUpdateAvailable = (_event: unknown, info: UpdateInfo) => {Apply similar changes to other event handlers for better type safety.
115-122: Improve error handling in update installation.The error handling silently logs errors without providing user feedback, which could lead to confusion if the installation fails.
const handleInstallUpdate = async () => { try { await window.vibe.update.showUpdateDialog(); } catch (error) { - // Log error but don't show to user console.error("Failed to show update dialog:", error); + setError("Failed to show update dialog"); + setTimeout(() => setError(null), 5000); } };This provides better user experience by showing error feedback.
apps/electron-app/src/main/services/update-service.ts (3)
20-22: Remove unnecessary constructor.The static analysis tool correctly identifies that this constructor is unnecessary since it doesn't perform any initialization.
- constructor() { - // Private constructor for singleton pattern - } + private constructor() {}Or if you want to keep it explicit for documentation purposes, make it private:
- constructor() { + private constructor() { // Private constructor for singleton pattern }
38-42: Add thread safety to singleton initialization.The initialization check could have race conditions in multi-threaded scenarios. Consider making the initialization atomic.
initialize(): void { if (this.initialized) { logger.warn("AppUpdater already initialized"); return; } + this.initialized = true; // Set immediately to prevent race conditions
20-22: Remove unnecessary constructor.The static analysis tool correctly identifies that this constructor is unnecessary in the singleton pattern.
- constructor() { - // Private constructor for singleton pattern - } + private constructor() { + // Private constructor for singleton pattern + }The constructor should be private to prevent external instantiation, but the current empty constructor can be made private or removed entirely if no initialization is needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/electron-app/src/main/ipc/app/update.ts(1 hunks)apps/electron-app/src/main/menu/items/help.ts(2 hunks)apps/electron-app/src/main/services/update-service.ts(4 hunks)apps/electron-app/src/renderer/src/components/UpdateNotification.tsx(1 hunks)apps/electron-app/src/renderer/src/env.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/electron-app/src/main/menu/items/help.ts
- apps/electron-app/src/main/ipc/app/update.ts
- apps/electron-app/src/renderer/src/env.d.ts
🧰 Additional context used
🧠 Learnings (1)
apps/electron-app/src/main/services/update-service.ts (4)
Learnt from: maceip
PR: co-browser/vibe#38
File: apps/electron-app/src/main/services/profile-service.ts:131-135
Timestamp: 2025-07-01T18:24:21.101Z
Learning: In the ProfileService implementation (apps/electron-app/src/main/services/profile-service.ts), the team prefers to keep the updateProfile method simple without filtering out immutable fields like `id` and `createdAt` from updates, since these fields are not currently being used elsewhere in the codebase. They prioritize code simplicity over theoretical protection against field overwriting.
Learnt from: maceip
PR: co-browser/vibe#37
File: apps/electron-app/src/main/store/desktop-store.ts:0-0
Timestamp: 2025-07-01T18:22:27.574Z
Learning: In the desktop store implementation (apps/electron-app/src/main/store/desktop-store.ts), the team takes a performance-data-driven approach to optimization decisions. They prefer to keep the current implementation (including double encryption scenarios) until they can properly evaluate performance impact, rather than making premature optimizations.
Learnt from: maceip
PR: co-browser/vibe#37
File: apps/electron-app/src/main/store/desktop-store.ts:254-261
Timestamp: 2025-07-01T15:07:58.879Z
Learning: In the desktop store implementation (apps/electron-app/src/main/store/desktop-store.ts), the team prefers to maintain key-level encryption even when it results in double encryption scenarios like in the UserDataRecover function. They want to keep the consistent encryption handling approach rather than optimizing for performance by storing some data unencrypted.
Learnt from: maceip
PR: co-browser/vibe#37
File: apps/electron-app/src/main/store/desktop-store.ts:277-294
Timestamp: 2025-07-01T15:07:39.741Z
Learning: In apps/electron-app/src/main/store/desktop-store.ts, the user prefers to maintain key-level encryption in the helper functions getRecoveredData and getAllRecoveredData rather than removing the double encryption approach.
🪛 Biome (1.9.4)
apps/electron-app/src/renderer/src/components/UpdateNotification.tsx
[error] 88-88: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/electron-app/src/main/services/update-service.ts
[error] 20-22: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
⏰ 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). (2)
- GitHub Check: claude-review
- GitHub Check: quality
🔇 Additional comments (7)
apps/electron-app/src/main/services/update-service.ts (6)
190-207: The release notes formatting is comprehensive.The implementation handles HTML stripping, entity conversion, and list formatting well. The regex patterns cover common HTML entities and formatting needs.
27-32: Singleton implementation is correct.The getInstance method properly implements the singleton pattern with lazy initialization and null checks.
27-32: Solid singleton implementation.The singleton pattern is correctly implemented with proper lazy initialization and type safety.
92-101: Excellent concurrency control for update checks.The implementation properly prevents concurrent update checks, which is crucial for avoiding race conditions and duplicate operations.
189-208: Well-implemented release notes formatting.The release notes formatting logic properly handles both string and array formats, strips HTML tags, converts entities, and formats the output for native dialogs.
38-76: Proper initialization pattern with guards.The initialization logic correctly prevents double initialization and properly sets up the auto-updater configuration and event handlers.
apps/electron-app/src/renderer/src/components/UpdateNotification.tsx (1)
138-144: Good implementation of draggable region handling.The component correctly handles the draggable region styling to prevent interference with the app's window dragging behavior.
apps/electron-app/src/renderer/src/components/UpdateNotification.tsx
Outdated
Show resolved
Hide resolved
apps/electron-app/src/renderer/src/components/UpdateNotification.tsx
Outdated
Show resolved
Hide resolved
|
Claude finished @michiosw's task —— View job Code Review for Auto-Update Notification UIReview Progress:
📋 SummaryThis PR implements a comprehensive auto-update notification system with clean UI and proper state management. The implementation follows good patterns but has some areas for improvement. ✅ StrengthsCode Quality
User Experience
|
|
Claude finished @michiosw's task —— View job Code Review for Auto-Update Notification UIReview Progress:
📋 SummaryThis PR implements a comprehensive auto-update notification system with clean UI and proper state management. The implementation follows good patterns but has some areas for improvement. ✅ StrengthsCode Quality
User Experience
|
|
Claude finished @michiosw's task —— View job Code Review for Auto-Update Notification UIReview Progress:
📋 SummaryThis PR implements a comprehensive auto-update notification system with clean UI and proper state management. The implementation follows good patterns but has some areas for improvement. ✅ StrengthsCode Quality
User Experience
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/electron-app/src/main/services/update-service.ts (2)
92-99: Inconsistent return value for concurrent update checksThe early return when
this.isCheckingis true returns a different data structure than successful checks, which could cause issues for consumers expecting consistent return types.Consider returning
nullor throwing an error to indicate concurrent access, or ensure the return structure matches successful checks.
154-168: Data persistence handling needs verificationWhile the comment mentions that "Data persistence is handled by the app's gracefulShutdown mechanism," there's no explicit verification that this mechanism is properly implemented and will execute before windows are closed.
Consider adding explicit data persistence checks or ensuring the gracefulShutdown mechanism is properly coordinated with the update flow.
🧹 Nitpick comments (1)
apps/electron-app/src/main/services/update-service.ts (1)
20-32: Consider removing the empty constructorThe static analysis tool correctly identifies that this empty constructor is unnecessary. The singleton pattern can be achieved without it since TypeScript will implicitly create a private constructor when needed.
- constructor() { - // Private constructor for singleton pattern - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/electron-app/src/main/services/update-service.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
apps/electron-app/src/main/services/update-service.ts (4)
Learnt from: maceip
PR: co-browser/vibe#38
File: apps/electron-app/src/main/services/profile-service.ts:131-135
Timestamp: 2025-07-01T18:24:21.101Z
Learning: In the ProfileService implementation (apps/electron-app/src/main/services/profile-service.ts), the team prefers to keep the updateProfile method simple without filtering out immutable fields like `id` and `createdAt` from updates, since these fields are not currently being used elsewhere in the codebase. They prioritize code simplicity over theoretical protection against field overwriting.
Learnt from: maceip
PR: co-browser/vibe#37
File: apps/electron-app/src/main/store/desktop-store.ts:0-0
Timestamp: 2025-07-01T18:22:27.574Z
Learning: In the desktop store implementation (apps/electron-app/src/main/store/desktop-store.ts), the team takes a performance-data-driven approach to optimization decisions. They prefer to keep the current implementation (including double encryption scenarios) until they can properly evaluate performance impact, rather than making premature optimizations.
Learnt from: maceip
PR: co-browser/vibe#37
File: apps/electron-app/src/main/store/desktop-store.ts:254-261
Timestamp: 2025-07-01T15:07:58.879Z
Learning: In the desktop store implementation (apps/electron-app/src/main/store/desktop-store.ts), the team prefers to maintain key-level encryption even when it results in double encryption scenarios like in the UserDataRecover function. They want to keep the consistent encryption handling approach rather than optimizing for performance by storing some data unencrypted.
Learnt from: maceip
PR: co-browser/vibe#37
File: apps/electron-app/src/main/store/desktop-store.ts:277-294
Timestamp: 2025-07-01T15:07:39.741Z
Learning: In apps/electron-app/src/main/store/desktop-store.ts, the user prefers to maintain key-level encryption in the helper functions getRecoveredData and getAllRecoveredData rather than removing the double encryption approach.
🪛 Biome (1.9.4)
apps/electron-app/src/main/services/update-service.ts
[error] 20-22: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
⏰ 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). (2)
- GitHub Check: claude-review
- GitHub Check: quality
🔇 Additional comments (4)
apps/electron-app/src/main/services/update-service.ts (4)
1-8: LGTM: Clean imports and logger setupThe imports are well-organized and the logger is appropriately configured for the AppUpdater service.
34-76: LGTM: Well-structured initialization with proper guardsThe initialization method correctly implements:
- Guard against double initialization
- Proper autoUpdater configuration
- Event handlers that focus on logging rather than direct IPC communication
This aligns well with the architectural refactoring to centralize IPC communication in a separate module.
175-208: LGTM: Comprehensive release notes formatting with improved type safetyThe implementation properly addresses the previous type safety concern by using
note.note || ""and includes comprehensive HTML stripping and formatting for native dialogs.The formatting logic handles:
- HTML tag removal
- Entity decoding
- List formatting
- Whitespace normalization
211-214: LGTM: Well-defined interface for release note informationThe interface properly defines the structure with readonly modifiers and nullable note property that aligns with the type safety improvements in the formatting method.
|
Claude finished @michiosw's task —— View job Code Review for Auto-Update Notification UIReview Progress:
📋 SummaryThis PR implements a comprehensive auto-update notification system with clean UI and proper state management. The implementation follows good patterns but has some areas for improvement. ✅ StrengthsCode Quality
User Experience
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
apps/electron-app/src/main/services/update-service.ts (3)
101-108: Fix inconsistent return type for concurrent update checks.The early return when
isCheckingis true provides a different data structure than successful checks, which could cause issues for consumers expecting consistent return types.- if (this.isChecking) { - logger.info("Update check already in progress, skipping"); - return { - currentVersion: app.getVersion(), - updateInfo: null, - }; - } + if (this.isChecking) { + logger.info("Update check already in progress, skipping"); + return null; + }
191-216: Review the update installation flow for potential data loss.The current implementation immediately sets
app.isQuitting = trueand callsapp.quit()without ensuring user data is persisted. This could lead to data loss if users have unsaved changes.Consider implementing data persistence checks before initiating the update process:
+ // Check for unsaved data before proceeding + const hasUnsavedData = await this.checkForUnsavedData(); + if (hasUnsavedData) { + const saveResult = await this.promptToSaveData(); + if (!saveResult) { + logger.info("User cancelled update due to unsaved data"); + return; + } + } + // Set flag to indicate we're installing an update app.isQuitting = true;#!/bin/bash # Description: Check if there are data persistence mechanisms in place # Expected: Find handlers that ensure data is saved before app quits # Search for data persistence and save mechanisms rg -A 5 -B 5 "before.*quit|will.*quit|save.*data|persist.*data" --type ts # Search for window close handlers rg -A 3 "beforeunload|window.*close|close.*window" --type ts
234-235: Improve type safety for release notes handling.The code assumes array elements have a
noteproperty but provides a fallback with|| "". This is good, but the type safety concern from past reviews has been addressed.The current implementation correctly handles the case where
notemight be null or undefined:notes = releaseNotes.map(note => note.note || "").join("\n");This is the proper way to handle potentially undefined properties.
🧹 Nitpick comments (2)
apps/electron-app/src/main/services/update-service.ts (2)
20-22: Remove the unnecessary constructor.The empty constructor serves no purpose in the singleton pattern implementation and can be safely removed.
- constructor() { - // Private constructor for singleton pattern - } + private constructor() {}
147-154: Strengthen semver validation with proper parsing.The current regex-based validation is fragile and may not catch all invalid semver formats. Consider using a proper semver parsing library for more robust validation.
- // Validate version format (basic semver check) - const semverRegex = /^\d+\.\d+\.\d+(-[a-zA-Z0-9.-]+)?(\+[a-zA-Z0-9.-]+)?$/; - if (!semverRegex.test(info.version)) { - logger.error( - `Invalid update info: invalid version format ${info.version}`, - ); - return false; - } + // Validate version format using proper semver parsing + try { + // Consider using a semver library like 'semver' npm package + const versionParts = info.version.split('.'); + if (versionParts.length < 3 || + versionParts.slice(0, 3).some(part => isNaN(Number(part)))) { + throw new Error('Invalid semver format'); + } + } catch (error) { + logger.error( + `Invalid update info: invalid version format ${info.version}`, + ); + return false; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/electron-app/src/main/services/update-service.ts(4 hunks)apps/electron-app/src/renderer/src/components/UpdateNotification.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/electron-app/src/renderer/src/components/UpdateNotification.tsx
🧰 Additional context used
🧠 Learnings (1)
apps/electron-app/src/main/services/update-service.ts (4)
Learnt from: maceip
PR: co-browser/vibe#38
File: apps/electron-app/src/main/services/profile-service.ts:131-135
Timestamp: 2025-07-01T18:24:21.101Z
Learning: In the ProfileService implementation (apps/electron-app/src/main/services/profile-service.ts), the team prefers to keep the updateProfile method simple without filtering out immutable fields like `id` and `createdAt` from updates, since these fields are not currently being used elsewhere in the codebase. They prioritize code simplicity over theoretical protection against field overwriting.
Learnt from: maceip
PR: co-browser/vibe#37
File: apps/electron-app/src/main/store/desktop-store.ts:0-0
Timestamp: 2025-07-01T18:22:27.574Z
Learning: In the desktop store implementation (apps/electron-app/src/main/store/desktop-store.ts), the team takes a performance-data-driven approach to optimization decisions. They prefer to keep the current implementation (including double encryption scenarios) until they can properly evaluate performance impact, rather than making premature optimizations.
Learnt from: maceip
PR: co-browser/vibe#37
File: apps/electron-app/src/main/store/desktop-store.ts:254-261
Timestamp: 2025-07-01T15:07:58.879Z
Learning: In the desktop store implementation (apps/electron-app/src/main/store/desktop-store.ts), the team prefers to maintain key-level encryption even when it results in double encryption scenarios like in the UserDataRecover function. They want to keep the consistent encryption handling approach rather than optimizing for performance by storing some data unencrypted.
Learnt from: maceip
PR: co-browser/vibe#37
File: apps/electron-app/src/main/store/desktop-store.ts:277-294
Timestamp: 2025-07-01T15:07:39.741Z
Learning: In apps/electron-app/src/main/store/desktop-store.ts, the user prefers to maintain key-level encryption in the helper functions getRecoveredData and getAllRecoveredData rather than removing the double encryption approach.
🪛 Biome (1.9.4)
apps/electron-app/src/main/services/update-service.ts
[error] 20-22: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
⏰ 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). (2)
- GitHub Check: claude-review
- GitHub Check: quality
🔇 Additional comments (2)
apps/electron-app/src/main/services/update-service.ts (2)
237-255: LGTM: Comprehensive HTML sanitization for release notes.The release notes formatting properly handles HTML tag removal, entity conversion, and text normalization. The implementation is thorough and handles various edge cases well.
38-84: LGTM: Proper singleton initialization with safeguards.The initialization method correctly prevents multiple initializations and properly sets up all event handlers. The validation of update info on receipt is a good security practice.
- Add electron-updater integration with GitHub releases - Create singleton UpdateService for managing updates - Add update notification UI component - Configure app-update.yml for GitHub provider - Add IPC handlers for renderer-main communication - Support automatic download and installation prompts - Handle update errors and edge cases gracefully - Format release notes for native dialogs 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add clean pill-style update notification in tab bar area - Show update status with progress indicator for downloads - Add 'Check for Updates...' menu item in Help menu - Handle all update states: checking, available, downloading, ready, error - Dismissible notification that reappears on app restart - Non-blocking UI that doesn't interfere with browsing 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Replace removeAllListeners with specific listener removal to prevent memory leaks - Add race condition prevention for concurrent update checks - Improve error handling with user-friendly messages - Add JSDoc comments for public methods - Wrap event forwarding in try-catch to handle window destruction 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add missing newline at end of app-update.yml - Use safe accessor pattern getAppUpdater() in help menu - Remove update methods from legacy API (already in window.vibe.update) - Simplify update flow by removing redundant force-quit logic - Add comment about data persistence handled by gracefulShutdown 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add conditional notarization based on NOTARIZE env variable - Implement electron fuses for security hardening - Enable cookie encryption - Disable dangerous Node.js features - Enable ASAR integrity validation - Fix V8 snapshot compatibility issue - Add entitlementsInherit for macOS helper processes - Update CI/CD workflow to enable notarization for releases - Configure auto-updater for GitHub releases 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…eanup - Add error logging for update check failures on mount - Add type safety to error handler parameters - Simplify event listener cleanup to avoid accessing private properties - Use window.api.removeAllListeners instead of fragile _events manipulation 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Handle null values in release notes array properly - Convert null notes to empty strings instead of 'null' - Prevents potential runtime issues with malformed release notes
- Improve event listener cleanup with proper documentation - Add error feedback to user when update dialog fails - Integrate graceful shutdown with update installation - Add update metadata validation for security - Validate version format using semver regex - Store only validated update info to prevent malformed data These changes improve reliability, security, and user experience of the auto-update feature based on comprehensive code review.
michiosw
force-pushed
the
feat/auto-update
branch
from
7d9b34a to
fdfab37
Compare
|
Claude finished @michiosw's task —— View job Code Review for Auto-Update Notification UIReview Progress:
📋 SummaryThis PR implements a comprehensive auto-update notification system with clean UI and proper state management. The implementation follows good patterns but has some areas for improvement. ✅ StrengthsCode Quality
User Experience
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/electron-app/app-update.yml (1)
1-3: Fix missing newline at end of file.The YAML configuration looks correct for GitHub-based updates. However, there's a missing newline at the end of the file as indicated by the YAMLlint tool.
apps/electron-app/src/main/services/update-service.ts (1)
101-108: Inconsistent return structure for concurrent checks.When a check is already in progress, the method returns a different structure than a successful check, which could cause issues for consumers expecting consistent data.
🧹 Nitpick comments (1)
apps/electron-app/src/main/services/update-service.ts (1)
20-22: Remove unnecessary empty constructor.The empty private constructor serves no purpose beyond making it private. You can achieve the same singleton pattern without it.
- constructor() { - // Private constructor for singleton pattern - } + private constructor() { + // Private constructor for singleton pattern + }Or remove it entirely and make the
instancefield initialization handle the singleton pattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.github/workflows/release.yml(1 hunks)apps/electron-app/app-update.yml(1 hunks)apps/electron-app/dev-app-update.yml(0 hunks)apps/electron-app/electron-builder.js(3 hunks)apps/electron-app/src/main/index.ts(2 hunks)apps/electron-app/src/main/ipc/app/update.ts(1 hunks)apps/electron-app/src/main/ipc/index.ts(2 hunks)apps/electron-app/src/main/menu/items/help.ts(2 hunks)apps/electron-app/src/main/services/index.ts(1 hunks)apps/electron-app/src/main/services/update-service.ts(4 hunks)apps/electron-app/src/preload/index.ts(1 hunks)apps/electron-app/src/renderer/src/App.tsx(2 hunks)apps/electron-app/src/renderer/src/components/UpdateNotification.tsx(1 hunks)apps/electron-app/src/renderer/src/components/main/MainApp.tsx(1 hunks)apps/electron-app/src/renderer/src/env.d.ts(1 hunks)packages/shared-types/src/constants/index.ts(0 hunks)packages/shared-types/src/interfaces/index.ts(1 hunks)
💤 Files with no reviewable changes (2)
- apps/electron-app/dev-app-update.yml
- packages/shared-types/src/constants/index.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- .github/workflows/release.yml
- apps/electron-app/src/main/ipc/index.ts
- apps/electron-app/src/renderer/src/App.tsx
- apps/electron-app/src/renderer/src/components/main/MainApp.tsx
- apps/electron-app/src/main/services/index.ts
- apps/electron-app/src/main/menu/items/help.ts
- apps/electron-app/src/main/index.ts
- apps/electron-app/src/preload/index.ts
- packages/shared-types/src/interfaces/index.ts
- apps/electron-app/src/main/ipc/app/update.ts
- apps/electron-app/electron-builder.js
- apps/electron-app/src/renderer/src/env.d.ts
🧰 Additional context used
🧠 Learnings (2)
apps/electron-app/src/main/services/update-service.ts (4)
Learnt from: maceip
PR: co-browser/vibe#38
File: apps/electron-app/src/main/services/profile-service.ts:131-135
Timestamp: 2025-07-01T18:24:21.101Z
Learning: In the ProfileService implementation (apps/electron-app/src/main/services/profile-service.ts), the team prefers to keep the updateProfile method simple without filtering out immutable fields like `id` and `createdAt` from updates, since these fields are not currently being used elsewhere in the codebase. They prioritize code simplicity over theoretical protection against field overwriting.
Learnt from: maceip
PR: co-browser/vibe#37
File: apps/electron-app/src/main/store/desktop-store.ts:0-0
Timestamp: 2025-07-01T18:22:27.574Z
Learning: In the desktop store implementation (apps/electron-app/src/main/store/desktop-store.ts), the team takes a performance-data-driven approach to optimization decisions. They prefer to keep the current implementation (including double encryption scenarios) until they can properly evaluate performance impact, rather than making premature optimizations.
Learnt from: maceip
PR: co-browser/vibe#37
File: apps/electron-app/src/main/store/desktop-store.ts:254-261
Timestamp: 2025-07-01T15:07:58.879Z
Learning: In the desktop store implementation (apps/electron-app/src/main/store/desktop-store.ts), the team prefers to maintain key-level encryption even when it results in double encryption scenarios like in the UserDataRecover function. They want to keep the consistent encryption handling approach rather than optimizing for performance by storing some data unencrypted.
Learnt from: maceip
PR: co-browser/vibe#37
File: apps/electron-app/src/main/store/desktop-store.ts:277-294
Timestamp: 2025-07-01T15:07:39.741Z
Learning: In apps/electron-app/src/main/store/desktop-store.ts, the user prefers to maintain key-level encryption in the helper functions getRecoveredData and getAllRecoveredData rather than removing the double encryption approach.
apps/electron-app/app-update.yml (1)
Learnt from: maceip
PR: co-browser/vibe#38
File: apps/electron-app/src/main/services/profile-service.ts:131-135
Timestamp: 2025-07-01T18:24:21.101Z
Learning: In the ProfileService implementation (apps/electron-app/src/main/services/profile-service.ts), the team prefers to keep the updateProfile method simple without filtering out immutable fields like `id` and `createdAt` from updates, since these fields are not currently being used elsewhere in the codebase. They prioritize code simplicity over theoretical protection against field overwriting.
🧬 Code Graph Analysis (1)
apps/electron-app/src/main/services/update-service.ts (4)
packages/shared-types/src/logger/index.ts (1)
createLogger(65-76)apps/electron-app/src/main/services/index.ts (1)
AppUpdater(17-17)infrastructure/oauth-proxy-server/src/index.js (1)
app(21-21)packages/shared-types/src/constants/index.ts (1)
UPDATER(72-74)
🪛 Biome (1.9.4)
apps/electron-app/src/main/services/update-service.ts
[error] 20-22: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
🪛 YAMLlint (1.37.1)
apps/electron-app/app-update.yml
[error] 3-3: no new line character at the end of file
(new-line-at-end-of-file)
⏰ 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). (2)
- GitHub Check: claude-review
- GitHub Check: quality
🔇 Additional comments (9)
apps/electron-app/src/renderer/src/components/UpdateNotification.tsx (4)
119-122: Error handling improvement looks good.The change from silently ignoring errors to logging them with
console.erroraddresses the previous feedback and provides better debugging visibility.
63-66: Type safety improvement for error handling.The error parameter is now properly typed with
{ code?: string; message?: string }, addressing the previous type safety concern.
104-108: Event listener cleanup approach acknowledged.The use of
removeAllListenersis now properly documented with the singleton requirement comment (lines 19-22). This addresses the previous concern about the cleanup mechanism, though it still has the limitation mentioned in past reviews.
16-23: Good documentation of singleton requirement.The comment clearly explains why this component should be used as a singleton due to the
removeAllListenerslimitation. This provides important context for future developers.apps/electron-app/src/main/services/update-service.ts (5)
138-157: Excellent addition of update info validation.The
isValidUpdateInfomethod adds important safety checks for version format and required fields. This prevents invalid update data from causing issues downstream.
191-216: Improved update installation flow with graceful shutdown.The new installation flow properly handles graceful shutdown by listening for the
will-quitevent and delaying the update installation. This addresses previous concerns about data loss during updates.
230-256: Enhanced release notes formatting with safety.The improved
formatReleaseNotesmethod now handles the null/undefined case for individual notes (note.note || ""), addressing the previous type safety concern. The HTML stripping and formatting logic is also more comprehensive.
14-32: Well-implemented singleton pattern.The singleton implementation follows best practices with a private static instance and proper getInstance method. The initialization guard prevents multiple setups.
38-85: Proper initialization with logging integration.The initialization method properly sets up the auto-updater configuration and event handlers. The integration with the shared logger and validation on update events improves reliability.
Summary
Changes
Test Plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores