refactor(update): address code review feedback

- 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.

Author: michiosw

apps/electron-app/src/main/services/update-service.ts

@@ -56,6 +56,10 @@ export default class AppUpdater {
 
     autoUpdater.on("update-available", (releaseInfo: UpdateInfo) => {
       logger.info("update ready:", releaseInfo);
+      // Validate update info when received
+      if (!this.isValidUpdateInfo(releaseInfo)) {
+        logger.error("Received invalid update info");
+      }
     });
 
     autoUpdater.on("update-not-available", () => {
@@ -67,8 +71,13 @@ export default class AppUpdater {
     });
 
     autoUpdater.on("update-downloaded", (releaseInfo: UpdateInfo) => {
-      this.releaseInfo = releaseInfo;
-      logger.info("update downloaded:", releaseInfo);
+      // Validate before storing
+      if (this.isValidUpdateInfo(releaseInfo)) {
+        this.releaseInfo = releaseInfo;
+        logger.info("update downloaded:", releaseInfo);
+      } else {
+        logger.error("Downloaded update has invalid info, refusing to store");
+      }
     });
 
     this.autoUpdater = autoUpdater;
@@ -121,12 +130,45 @@ export default class AppUpdater {
     }
   }
 
+  /**
+   * Validates update info structure
+   * @param info The update info to validate
+   * @returns True if valid, false otherwise
+   */
+  private isValidUpdateInfo(info: UpdateInfo | undefined): boolean {
+    if (!info) return false;
+
+    // Check required fields
+    if (!info.version || typeof info.version !== "string") {
+      logger.error("Invalid update info: missing or invalid version");
+      return false;
+    }
+
+    // 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;
+    }
+
+    return true;
+  }
+
   /**
    * Show update dialog to the user
    * @param mainWindow The window to show the dialog on
    */
   public async showUpdateDialog(mainWindow: BrowserWindow) {
     if (!this.releaseInfo) {
+      logger.warn("No release info available for update dialog");
+      return;
+    }
+
+    // Validate update info before processing
+    if (!this.isValidUpdateInfo(this.releaseInfo)) {
+      logger.error("Invalid update info, refusing to show dialog");
       return;
     }
 
@@ -146,26 +188,32 @@ export default class AppUpdater {
         defaultId: 1,
         cancelId: 0,
       })
-      .then(({ response }) => {
+      .then(async ({ response }) => {
         if (response === 1) {
-          app.isQuitting = true;
           logger.info("User clicked install, starting update installation...");
 
-          // Close all windows first
-          // Note: Data persistence is handled by the app's gracefulShutdown mechanism
-          BrowserWindow.getAllWindows().forEach(win => {
-            try {
-              win.close();
-            } catch (e) {
-              logger.error("Error closing window:", e);
-            }
-          });
+          // Set flag to indicate we're installing an update
+          app.isQuitting = true;
+
+          // The app's before-quit handler will trigger gracefulShutdown
+          // which properly cleans up all resources. After that completes,
+          // we'll install the update.
 
-          // Add delay to ensure windows are closed
-          setTimeout(() => {
-            logger.info("Calling quitAndInstall...");
+          // Store reference to quitAndInstall to be called after shutdown
+          const performUpdate = () => {
+            logger.info("Graceful shutdown complete, installing update...");
             autoUpdater.quitAndInstall(false, true);
-          }, 100);
+          };
+
+          // Listen for app ready to quit after graceful shutdown
+          app.once("will-quit", event => {
+            event.preventDefault();
+            // Small delay to ensure all cleanup is complete
+            setTimeout(performUpdate, 100);
+          });
+
+          // Trigger the shutdown sequence
+          app.quit();
         } else {
           mainWindow.webContents.send("update-downloaded-cancelled");
         }

apps/electron-app/src/renderer/src/components/UpdateNotification.tsx

@@ -13,6 +13,14 @@ interface DownloadProgress {
   transferred: number;
 }
 
+/**
+ * Update notification component that displays update status and progress.
+ *
+ * IMPORTANT: This component should be used as a singleton in the app.
+ * Due to limitations in the legacy window.api interface, we use removeAllListeners
+ * which removes all listeners for a channel. Multiple instances would interfere
+ * with each other's event handling.
+ */
 export function UpdateNotification() {
   const [updateAvailable, setUpdateAvailable] = useState(false);
   const [updateInfo, setUpdateInfo] = useState<UpdateInfo | null>(null);
@@ -26,8 +34,8 @@ export function UpdateNotification() {
   const [showNoUpdate, setShowNoUpdate] = useState(false);
 
   useEffect(() => {
-    // Store listener references for proper cleanup
-    const listeners: Array<() => void> = [];
+    // Store cleanup functions for proper listener removal
+    const cleanupFunctions: Array<() => void> = [];
 
     // Listen for update events
     const handleUpdateAvailable = (_event: any, info: UpdateInfo) => {
@@ -82,12 +90,20 @@ export function UpdateNotification() {
       setTimeout(() => setShowNoUpdate(false), 3000);
     };
 
-    // Add listeners and store cleanup functions
+    // Helper to add listeners with proper cleanup
     const addListener = (channel: string, handler: any) => {
-      window.api.on(channel, handler);
-      // Since we only have removeAllListeners available, we'll use that
-      // This is safe because this component is the only one listening to these update channels
-      listeners.push(() => {
+      // Create a wrapper that extracts event data
+      const listener = (event: any, ...args: any[]) => {
+        handler(event, ...args);
+      };
+
+      // Use the legacy API but store individual cleanup functions
+      window.api.on(channel, listener);
+
+      // Store cleanup function that removes only this specific listener
+      cleanupFunctions.push(() => {
+        // Note: Since window.api only exposes removeAllListeners, we need to be careful
+        // For now, we'll use it but document that this component should be singleton
         window.api.removeAllListeners(channel);
       });
     };
@@ -105,18 +121,20 @@ export function UpdateNotification() {
       console.error("Update check failed on mount:", error);
     });
 
-    // Cleanup - remove specific listeners
+    // Cleanup all listeners on unmount
     return () => {
-      listeners.forEach(cleanup => cleanup());
+      cleanupFunctions.forEach(cleanup => cleanup());
     };
   }, []);
 
   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);
+      // Show error to user
+      setError("Failed to install update. Please try again.");
+      setTimeout(() => setError(null), 5000);
     }
   };