changes from self-review

Author: gantoine

apps/code/src/main/platform-adapters/electron-updater.ts

@@ -67,13 +67,10 @@ export class ElectronUpdater implements IUpdater {
   }
 
   public quitAndInstall(): void {
-    // An unpackaged (dev) build has no staged artifact to swap, so the real
-    // quitAndInstall would no-op or throw. Relaunch instead, so a simulated
-    // update can still exercise the post-update startup path (e.g. fullscreen
-    // restore) during local testing.
+    // An unpackaged (dev) build has no staged artifact to swap and
+    // the real quitAndInstall would no-op or throw, so relaunch instead.
     if (!app.isPackaged) {
       app.relaunch();
-      app.exit(0);
       return;
     }
     autoUpdater.quitAndInstall(false, true);

apps/code/src/main/services/app-lifecycle/service.test.ts

@@ -129,17 +129,16 @@ describe("AppLifecycleService", () => {
       expect(service.isQuittingForUpdate).toBe(false);
     });
 
-    it("remembers fullscreen for the next launch when quitting for update in fullscreen", () => {
-      mockGetFullScreenState.mockReturnValue(true);
-      service.setQuittingForUpdate();
-      expect(mockSetRestoreFullScreenOnNextLaunch).toHaveBeenCalledWith(true);
-    });
-
-    it("does not schedule a fullscreen restore when not fullscreen", () => {
-      mockGetFullScreenState.mockReturnValue(false);
-      service.setQuittingForUpdate();
-      expect(mockSetRestoreFullScreenOnNextLaunch).toHaveBeenCalledWith(false);
-    });
+    it.each([[true], [false]])(
+      "schedules restore-fullscreen=%s on update-quit",
+      (isFullScreen) => {
+        mockGetFullScreenState.mockReturnValue(isFullScreen);
+        service.setQuittingForUpdate();
+        expect(mockSetRestoreFullScreenOnNextLaunch).toHaveBeenCalledWith(
+          isFullScreen,
+        );
+      },
+    );
 
     it("clears the fullscreen-restore flag when the update handoff is aborted", () => {
       mockGetFullScreenState.mockReturnValue(true);

apps/code/src/main/services/app-lifecycle/service.ts

@@ -57,16 +57,13 @@ export class AppLifecycleService {
 
   setQuittingForUpdate(): void {
     this._isQuittingForUpdate = true;
-    // Remember whether we were fullscreen so the post-update relaunch restores
-    // it. A normal quit never runs this, so the flag stays false and the app
-    // reopens windowed.
+    // Remember fullscreen state so the post-update relaunch restores it.
     setRestoreFullScreenOnNextLaunch(getFullScreenState());
   }
 
   clearQuittingForUpdate(): void {
     this._isQuittingForUpdate = false;
-    // The install handoff was aborted; don't let a stale flag restore
-    // fullscreen on the next manual launch.
+    // The install handoff was aborted.
     setRestoreFullScreenOnNextLaunch(false);
   }
 

apps/code/src/main/services/dev-actions/service.ts

@@ -1,10 +1,10 @@
+import { UPDATES_SERVICE } from "@posthog/core/updates/identifiers";
+import type { UpdatesService } from "@posthog/core/updates/updates";
 import {
   DEV_HOST_ACTIONS_SERVICE,
   type IDevHostActions,
 } from "@posthog/platform/dev-host-actions";
 import { TypedEventEmitter } from "@posthog/shared";
-import { UPDATES_SERVICE } from "@posthog/core/updates/identifiers";
-import type { UpdatesService } from "@posthog/core/updates/updates";
 import { inject, injectable } from "inversify";
 import { DEV_NETWORK_SERVICE } from "../../di/tokens";
 import { getUserDataDir } from "../../utils/env";
@@ -52,8 +52,7 @@ export class DevActionsService extends TypedEventEmitter<DevActionsEvents> {
 
   /**
    * Surfaces the real update banner in the sidebar without a packaged build or
-   * a downloaded artifact. Clicking the banner's Restart then runs the normal
-   * install path (which persists the restore-fullscreen intent and relaunches),
+   * a downloaded artifact. Clicking Restart runs the normal install path,
    * so the update-restart behaviour can be exercised in local dev.
    */
   simulateUpdate(): void {

apps/code/src/main/utils/store.ts

@@ -72,10 +72,9 @@ export function getFullScreenState(): boolean {
 }
 
 /**
- * One-shot flag that survives a single relaunch. Set only when the app quits
- * to install an update, so a fullscreen session is restored after the
- * "restart to apply" handoff — but a normal quit (Cmd+Q, window close) leaves
- * it false and launches windowed.
+ * Set only when the app quits to install an update, so a fullscreen session
+ * is restored after the "restart to apply" handoff.
+ * A normal quit leaves it false and launches windowed.
  */
 export function setRestoreFullScreenOnNextLaunch(restore: boolean): void {
   windowStateStore.set("restoreFullScreenOnNextLaunch", restore);

apps/code/src/main/window.ts

@@ -24,7 +24,6 @@ import { collectMemorySnapshot } from "./utils/crash-diagnostics";
 import { isDevBuild } from "./utils/env";
 import { logger, readChromiumLogTail } from "./utils/logger";
 import {
-  getRestoreFullScreenOnNextLaunch,
   saveFullScreenState,
   saveZoomLevel,
   setRestoreFullScreenOnNextLaunch,
@@ -79,10 +78,8 @@ export function saveWindowState(window: BrowserWindow): void {
   const isMaximized = window.isMaximized();
   windowStateStore.set("isMaximized", isMaximized);
 
-  // Only save bounds when the window is in its normal state. Persisting the
-  // maximized or fullscreen bounds would clobber the user's windowed
-  // size/position, so a later "revert to windowed" launch has nothing to
-  // restore to.
+  // Only save bounds when not maximized, so restoring from maximized
+  // gives the user their previous windowed size/position
   if (!isMaximized && !window.isFullScreen()) {
     const bounds = window.getBounds();
     windowStateStore.set("x", bounds.x);
@@ -170,9 +167,8 @@ export function createWindow(): void {
   const savedState = getSavedWindowState();
 
   // Read the one-shot fullscreen-restore flag and clear it immediately, so it
-  // only ever affects the single launch that follows an update restart. A
-  // normal quit never sets it, so this is false and the app launches windowed.
-  const restoreFullScreen = getRestoreFullScreenOnNextLaunch();
+  // only ever affects the single launch that follows an update restart.
+  const restoreFullScreen = savedState.restoreFullScreenOnNextLaunch;
   if (restoreFullScreen) {
     setRestoreFullScreenOnNextLaunch(false);
   }

apps/code/src/renderer/features/dev-toolbar/components/DevToolbar.tsx

@@ -1,3 +1,4 @@
+import { updateStore } from "@posthog/core/updates/updateStore";
 import {
   DropdownMenu,
   DropdownMenuContent,
@@ -11,7 +12,6 @@ import {
   ItemDescription,
   ItemTitle,
 } from "@posthog/quill";
-import { updateStore } from "@posthog/core/updates/updateStore";
 import { REGION_LABELS } from "@posthog/shared";
 import { useOptionalAuthenticatedClient } from "@posthog/ui/features/auth/authClient";
 import {
@@ -816,8 +816,8 @@ function QuickActionsMenu() {
   };
 
   const handleSimulateUpdate = () => {
-    // Updates report as disabled in dev, which hides the sidebar banner. Force
-    // it on so the simulated "ready" state surfaces the real prompt.
+    // Updates report as disabled in dev, which hides the sidebar banner.
+    // Force it on so the simulated "ready" state surfaces the real prompt.
     updateStore.getState().setEnabled(true);
     void trpcClient.dev.simulateUpdate.mutate();
   };

packages/core/src/updates/updates.ts

@@ -230,11 +230,8 @@ export class UpdatesService extends TypedEventEmitter<UpdatesEvents> {
   }
 
   /**
-   * Dev/testing affordance (reached only through the dev-mode-gated toolbar):
-   * forces a "ready" state with a fake version so the real update banner
-   * surfaces, without a packaged build or a real downloaded artifact. Clicking
-   * the banner's Restart then runs the normal installUpdate() path; in an
-   * unpackaged build the updater relaunches instead of swapping binaries.
+   * Forces a "ready" state with a fake version so the real update banner
+   * surfaces, without a packaged build or a real downloaded artifact (dev only).
    */
   simulateUpdateReady(version = "Update"): void {
     this.downloadedVersion = version;