fix: persist GitHub sync state across Publish tab navigation (#3151)

AGENTS.md

@@ -22,6 +22,7 @@ Detailed rules and learnings are in the `rules/` directory. Read the relevant fi
 | [rules/adding-settings.md](rules/adding-settings.md)                 | Adding a new user-facing setting or toggle to the Settings page                                  |
 | [rules/chat-message-indicators.md](rules/chat-message-indicators.md) | Using `<dyad-status>` tags in chat messages for system indicators                                |
 | [rules/product-principles.md](rules/product-principles.md)           | Planning new features, especially via `dyad:swarm-to-plan`, to guide design trade-offs           |
+| [rules/jotai-testing.md](rules/jotai-testing.md)                     | Unit-testing Jotai atoms/hooks with `renderHook`, especially across unmount/remount              |
 
 ## Project setup and lints
 

rules/jotai-testing.md

+# Jotai Testing
+
+Learnings for writing unit tests against components/hooks that read or write Jotai atoms.
+
+## Sharing a store across `renderHook` calls in a single test
+
+When a test needs to render a hook, unmount it, and then render the hook again (e.g., to verify state persists across an unmount/remount — the exact scenario for atoms that replace local `useState`), all `renderHook` calls must share the **same Jotai store**. Otherwise each `renderHook`'s `Provider` wrapper creates its own isolated store and writes made by the first hook are invisible to the second.
+
+**Wrong** — each call to `makeWrapper()` returns a component that creates a fresh `<Provider>` (no store prop), so every `renderHook` gets a new default store:
+
+```tsx
+function makeWrapper() {
+  return function Wrapper({ children }) {
+    return <Provider>{children}</Provider>;
+  };
+}
+```
+
+**Right** — create one store per test and bind every `renderHook` in that test to it:
+
+```tsx
+import { createStore, Provider } from "jotai";
+
+function makeWrapper() {
+  const store = createStore();
+  return function Wrapper({ children }) {
+    return <Provider store={store}>{children}</Provider>;
+  };
+}
+
+// In the test:
+const wrapper = makeWrapper();
+const first = renderHook(() => useMyAtomHook(id), { wrapper });
+// ... mutate state ...
+first.unmount();
+const second = renderHook(() => useMyAtomHook(id), { wrapper });
+// second now sees state written by first
+```
+
+The symptom when you get this wrong is assertions like `expected false to be true` on the remounted hook's state, even though the setter clearly ran against the first hook.
+
+See `src/atoms/githubSyncAtoms.test.tsx` for a complete example covering unmount/remount, cross-unmount completion, and per-key isolation.

src/atoms/githubSyncAtoms.test.tsx

+import { renderHook, act } from "@testing-library/react";
+import { describe, it, expect } from "vitest";
+import { createStore, Provider } from "jotai";
+import type { PropsWithChildren } from "react";
+
+import {
+  DEFAULT_GITHUB_SYNC_STATE,
+  useGithubSyncState,
+} from "@/atoms/githubSyncAtoms";
+
+// Returns a wrapper bound to a fresh Jotai store so tests are isolated and
+// sibling `renderHook` calls in the same test share the same store (modeling
+// the production app which has a single global store).
+function makeWrapper() {
+  const store = createStore();
+  return function Wrapper({ children }: PropsWithChildren) {
+    return <Provider store={store}>{children}</Provider>;
+  };
+}
+
+describe("useGithubSyncState", () => {
+  it("returns the default state when no app has synced yet", () => {
+    const { result } = renderHook(() => useGithubSyncState(1), {
+      wrapper: makeWrapper(),
+    });
+
+    expect(result.current[0]).toEqual(DEFAULT_GITHUB_SYNC_STATE);
+  });
+
+  it("preserves sync state across unmount/remount for the same appId", () => {
+    // This is the regression test for the navigation-mid-sync bug: when the
+    // user leaves the Publish tab while a push is running, the component
+    // unmounts. When they return, the re-mounted component must still show
+    // the in-flight / completed state.
+    const wrapper = makeWrapper();
+
+    const first = renderHook(() => useGithubSyncState(42), { wrapper });
+
+    act(() => {
+      first.result.current[1]({ isSyncing: true });
+    });
+    expect(first.result.current[0].isSyncing).toBe(true);
+
+    first.unmount();
+
+    const second = renderHook(() => useGithubSyncState(42), { wrapper });
+    // Without the atom, this would be `false` (fresh local state).
+    expect(second.result.current[0].isSyncing).toBe(true);
+
+    // Completing the push after remount should update the new hook instance.
+    act(() => {
+      second.result.current[1]({ isSyncing: false, syncSuccess: true });
+    });
+    expect(second.result.current[0].isSyncing).toBe(false);
+    expect(second.result.current[0].syncSuccess).toBe(true);
+  });
+
+  it("completes a push while the component is unmounted and shows the result on remount", () => {
+    // Models the exact bug scenario: a push is in flight, the user navigates
+    // away (component unmounts), the IPC completion handler still runs and
+    // writes to the atom, and on remount the success is visible.
+    const wrapper = makeWrapper();
+
+    const first = renderHook(() => useGithubSyncState(7), { wrapper });
+    // Capture the stable setter from the first mount (this is what the
+    // unmounted async handler would hold on to).
+    const updaterFromFirstMount = first.result.current[1];
+
+    act(() => {
+      updaterFromFirstMount({ isSyncing: true });
+    });
+
+    first.unmount();
+
+    // While the component is unmounted, the background IPC completes and
+    // writes to the atom via the stale setter captured above.
+    act(() => {
+      updaterFromFirstMount({ isSyncing: false, syncSuccess: true });
+    });
+
+    const second = renderHook(() => useGithubSyncState(7), { wrapper });
+    expect(second.result.current[0]).toEqual({
+      ...DEFAULT_GITHUB_SYNC_STATE,
+      isSyncing: false,
+      syncSuccess: true,
+    });
+  });
+
+  it("isolates state between different appIds", () => {
+    const wrapper = makeWrapper();
+
+    const appA = renderHook(() => useGithubSyncState(1), { wrapper });
+    const appB = renderHook(() => useGithubSyncState(2), { wrapper });
+
+    act(() => {
+      appA.result.current[1]({ isSyncing: true, syncError: "oops" });
+    });
+
+    expect(appA.result.current[0].isSyncing).toBe(true);
+    expect(appA.result.current[0].syncError).toBe("oops");
+    // App B should be untouched.
+    expect(appB.result.current[0]).toEqual(DEFAULT_GITHUB_SYNC_STATE);
+  });
+
+  it("no-ops when appId is null", () => {
+    const { result } = renderHook(() => useGithubSyncState(null), {
+      wrapper: makeWrapper(),
+    });
+
+    expect(result.current[0]).toEqual(DEFAULT_GITHUB_SYNC_STATE);
+
+    act(() => {
+      result.current[1]({ isSyncing: true });
+    });
+
+    // Still default — a null appId cannot own any persistent state.
+    expect(result.current[0]).toEqual(DEFAULT_GITHUB_SYNC_STATE);
+  });
+});

src/atoms/githubSyncAtoms.ts

+import { atom, useAtomValue, useSetAtom } from "jotai";
+import { useCallback, useMemo } from "react";
+
+export type RebaseAction = "abort" | "continue" | "safe-push" | null;
+
+export interface GithubSyncState {
+  isSyncing: boolean;
+  syncError: string | null;
+  syncSuccess: boolean;
+  conflicts: string[];
+  rebaseInProgress: boolean;
+  rebaseStatusMessage: string | null;
+  rebaseAction: RebaseAction;
+}
+
+export const DEFAULT_GITHUB_SYNC_STATE: GithubSyncState = {
+  isSyncing: false,
+  syncError: null,
+  syncSuccess: false,
+  conflicts: [],
+  rebaseInProgress: false,
+  rebaseStatusMessage: null,
+  rebaseAction: null,
+};
+
+// Sync state is held in a global atom keyed by appId so that it survives
+// unmounts when the user navigates away from the Publish tab while a push
+// is in flight. The IPC push operation runs in the main process and its
+// completion callback updates this atom — the next time the component
+// mounts it will reflect the final result (success/failure).
+export const githubSyncStatesAtom = atom<Record<number, GithubSyncState>>({});
+
+type SyncPatch =
+  | Partial<GithubSyncState>
+  | ((prev: GithubSyncState) => Partial<GithubSyncState>);
+
+export function useGithubSyncState(appId: number | null) {
+  const allStates = useAtomValue(githubSyncStatesAtom);
+  const setAllStates = useSetAtom(githubSyncStatesAtom);
+
+  const state = useMemo<GithubSyncState>(() => {
+    if (appId == null) return DEFAULT_GITHUB_SYNC_STATE;
+    return allStates[appId] ?? DEFAULT_GITHUB_SYNC_STATE;
+  }, [allStates, appId]);
+
+  const updateSyncState = useCallback(
+    (patch: SyncPatch) => {
+      if (appId == null) return;
+      setAllStates((prev) => {
+        const current = prev[appId] ?? DEFAULT_GITHUB_SYNC_STATE;
+        const resolved = typeof patch === "function" ? patch(current) : patch;
+        return {
+          ...prev,
+          [appId]: { ...current, ...resolved },
+        };
+      });
+    },
+    [appId, setAllStates],
+  );
+
+  return [state, updateSyncState] as const;
+}

src/components/GitHubConnector.tsx

@@ -32,6 +32,7 @@ import { Label } from "@/components/ui/label";
 import { GithubBranchManager } from "@/components/GithubBranchManager";
 import { useResolveMergeConflictsWithAI } from "@/hooks/useResolveMergeConflictsWithAI";
 import { showSuccess, showError } from "@/lib/toast";
+import { useGithubSyncState } from "@/atoms/githubSyncAtoms";
 
 type SyncResult =
   | { error: Error; handled?: boolean }
@@ -79,20 +80,22 @@ function ConnectedGitHubConnector({
   onAutoSyncComplete,
 }: ConnectedGitHubConnectorProps) {
   const { t } = useTranslation(["home", "common"]);
-  const [isSyncing, setIsSyncing] = useState(false);
-  const [syncError, setSyncError] = useState<string | null>(null);
-  const [syncSuccess, setSyncSuccess] = useState<boolean>(false);
+  // Sync state is stored in a global atom keyed by appId so it survives
+  // unmounts when the user navigates away from the Publish tab while a push
+  // is still running. See githubSyncAtoms.ts.
+  const [syncState, updateSyncState] = useGithubSyncState(appId);
+  const {
+    isSyncing,
+    syncError,
+    syncSuccess,
+    conflicts,
+    rebaseInProgress,
+    rebaseStatusMessage,
+    rebaseAction,
+  } = syncState;
   const [showForceDialog, setShowForceDialog] = useState(false);
   const [isDisconnecting, setIsDisconnecting] = useState(false);
   const [disconnectError, setDisconnectError] = useState<string | null>(null);
-  const [conflicts, setConflicts] = useState<string[]>([]);
-  const [rebaseStatusMessage, setRebaseStatusMessage] = useState<string | null>(
-    null,
-  );
-  const [rebaseAction, setRebaseAction] = useState<
-    "abort" | "continue" | "safe-push" | null
-  >(null);
-  const [rebaseInProgress, setRebaseInProgress] = useState(false);
   const [isCancellingSync, setIsCancellingSync] = useState(false);
   const lastAutoSyncedAppIdRef = useRef<number | null>(null);
 
@@ -101,8 +104,7 @@ function ConnectedGitHubConnector({
     conflicts,
     onStartResolving: () => {
       // Clear conflicts state when starting AI resolution since user will be navigated to chat
-      setConflicts([]);
-      setSyncError(null);
+      updateSyncState({ conflicts: [], syncError: null });
     },
   });
 
@@ -113,15 +115,16 @@ function ConnectedGitHubConnector({
       let aborted = false;
       if (state.rebaseInProgress) {
         await ipc.github.rebaseAbort({ appId });
-        setRebaseInProgress(false);
-        setRebaseStatusMessage("Rebase aborted.");
+        updateSyncState({
+          rebaseInProgress: false,
+          rebaseStatusMessage: "Rebase aborted.",
+        });
         aborted = true;
       } else if (state.mergeInProgress) {
         await ipc.github.mergeAbort({ appId });
         aborted = true;
       }
-      setConflicts([]);
-      setSyncError(null);
+      updateSyncState({ conflicts: [], syncError: null });
       if (aborted) {
         showSuccess("Sync cancelled");
       }
@@ -137,6 +140,17 @@ function ConnectedGitHubConnector({
     setDisconnectError(null);
     try {
       await ipc.github.disconnect({ appId });
+      // Clear stale sync state so reconnecting to a different repo doesn't
+      // show a success/error message from the previous repo.
+      updateSyncState({
+        isSyncing: false,
+        syncError: null,
+        syncSuccess: false,
+        conflicts: [],
+        rebaseInProgress: false,
+        rebaseStatusMessage: null,
+        rebaseAction: null,
+      });
       refreshApp();
     } catch (err: any) {
       setDisconnectError(
@@ -152,12 +166,14 @@ function ConnectedGitHubConnector({
       force = false,
       forceWithLease = false,
     }: GithubSyncOptions = {}): Promise<SyncResult> => {
-      setIsSyncing(true);
-      setSyncError(null);
-      setSyncSuccess(false);
+      updateSyncState({
+        isSyncing: true,
+        syncError: null,
+        syncSuccess: false,
+        rebaseInProgress: false,
+        conflicts: [], // Clear conflicts when starting a new sync
+      });
       setShowForceDialog(false);
-      setRebaseInProgress(false);
-      setConflicts([]); // Clear conflicts when starting a new sync
 
       try {
         await ipc.github.push({
@@ -165,10 +181,15 @@ function ConnectedGitHubConnector({
           force,
           forceWithLease,
         });
-        setSyncSuccess(true);
-        setRebaseInProgress(false);
-        setConflicts([]); // Clear conflicts on successful sync
-        setRebaseStatusMessage(null);
+        updateSyncState({
+          syncSuccess: true,
+          rebaseInProgress: false,
+          conflicts: [], // Clear conflicts on successful sync
+          rebaseStatusMessage: null,
+        });
+        // Toast so the user sees the result even if they navigated away
+        // from the Publish tab while the push was running.
+        showSuccess("Successfully pushed to GitHub!");
         return {};
       } catch (err: any) {
         // Always check for conflicts when sync fails, regardless of error type
@@ -186,10 +207,12 @@ function ConnectedGitHubConnector({
 
         if (conflictsDetected.length > 0) {
           // Conflicts were detected - show resolution buttons below
-          setConflicts(conflictsDetected);
-          setSyncError(
+          updateSyncState({
+            conflicts: conflictsDetected,
+            syncError:
               "Merge conflicts detected. Use the buttons below to resolve them.",
-          );
+          });
+          showError("Merge conflicts detected while syncing to GitHub.");
           (err as Error & { handled?: boolean }).handled = true;
           return { error: err, handled: true };
         }
@@ -202,9 +225,10 @@ function ConnectedGitHubConnector({
         if (isConflict) {
           // Conflict error detected but no conflicts found - this shouldn't happen
           // but we'll show an error message
-          setSyncError(
-            "Merge conflict detected, but no conflicting files were returned. Please check git status and try again.",
-          );
+          const msg =
+            "Merge conflict detected, but no conflicting files were returned. Please check git status and try again.";
+          updateSyncState({ syncError: msg });
+          showError(msg);
           return { error: err };
         }
 
@@ -243,69 +267,84 @@ function ConnectedGitHubConnector({
               ? " Conflict check failed."
               : "";
         const finalErrorMessage = `${baseErrorMessage}${conflictCheckMessage}`;
-        setSyncError(finalErrorMessage);
-        setRebaseInProgress(rebaseInProgressState);
-        setRebaseStatusMessage(null);
+        updateSyncState({
+          syncError: finalErrorMessage,
+          rebaseInProgress: rebaseInProgressState,
+          rebaseStatusMessage: null,
+        });
+        showError(`Failed to sync to GitHub: ${finalErrorMessage}`);
         return { error: err };
       } finally {
-        setIsSyncing(false);
+        updateSyncState({ isSyncing: false });
       }
     },
-    [appId],
+    [appId, updateSyncState],
   );
 
   const handleAbortRebase = useCallback(async () => {
-    setRebaseAction("abort");
-    setSyncError(null);
-    setRebaseStatusMessage(null);
-    setSyncSuccess(false);
+    updateSyncState({
+      rebaseAction: "abort",
+      syncError: null,
+      rebaseStatusMessage: null,
+      syncSuccess: false,
+    });
     try {
       await ipc.github.rebaseAbort({ appId });
-      setRebaseInProgress(false);
-      setRebaseStatusMessage("Rebase aborted. You can try syncing again.");
+      updateSyncState({
+        rebaseInProgress: false,
+        rebaseStatusMessage: "Rebase aborted. You can try syncing again.",
+      });
     } catch (err: any) {
-      setSyncError(err.message || "Failed to abort rebase.");
-      setRebaseInProgress(true);
+      updateSyncState({
+        syncError: err.message || "Failed to abort rebase.",
+        rebaseInProgress: true,
+      });
     } finally {
-      setRebaseAction(null);
+      updateSyncState({ rebaseAction: null });
     }
-  }, [appId]);
+  }, [appId, updateSyncState]);
 
   const handleContinueRebase = useCallback(async () => {
-    setRebaseAction("continue");
-    setSyncError(null);
-    setRebaseStatusMessage(null);
-    setSyncSuccess(false);
+    updateSyncState({
+      rebaseAction: "continue",
+      syncError: null,
+      rebaseStatusMessage: null,
+      syncSuccess: false,
+    });
     try {
       await ipc.github.rebaseContinue({ appId });
-      setRebaseInProgress(false);
-      setRebaseStatusMessage("Rebase continued. You can sync when ready.");
+      updateSyncState({
+        rebaseInProgress: false,
+        rebaseStatusMessage: "Rebase continued. You can sync when ready.",
+      });
     } catch (err: any) {
-      setSyncError(err.message || "Failed to continue rebase.");
-      setRebaseInProgress(true);
+      updateSyncState({
+        syncError: err.message || "Failed to continue rebase.",
+        rebaseInProgress: true,
+      });
     } finally {
-      setRebaseAction(null);
+      updateSyncState({ rebaseAction: null });
     }
-  }, [appId]);
+  }, [appId, updateSyncState]);
 
   const handleSafeForcePush = useCallback(async () => {
-    setRebaseAction("safe-push");
+    updateSyncState({ rebaseAction: "safe-push" });
     try {
       await handleSyncToGithub({
         force: false,
         forceWithLease: true,
       });
     } finally {
-      setRebaseAction(null);
+      updateSyncState({ rebaseAction: null });
     }
-  }, [handleSyncToGithub]);
+  }, [handleSyncToGithub, updateSyncState]);
 
   const handleRebaseAndSync = useCallback(async () => {
-    setIsSyncing(true);
+    updateSyncState({ isSyncing: true });
     try {
       // First, perform the rebase
       await ipc.github.rebase({ appId });
-      setRebaseStatusMessage(null);
+      updateSyncState({ rebaseStatusMessage: null });
       const syncResult = await handleSyncToGithub();
       if (syncResult?.error) {
         if (!syncResult.handled) {
@@ -313,30 +352,35 @@ function ConnectedGitHubConnector({
         }
         return;
       }
-      setRebaseStatusMessage("Rebase and push completed successfully.");
+      updateSyncState({
+        rebaseStatusMessage: "Rebase and push completed successfully.",
+      });
     } catch (err: any) {
       if (err?.handled) {
         return;
       }
       const errorMessage =
         err?.message || "Failed to rebase and sync to GitHub.";
-      setSyncError(errorMessage);
-      setRebaseInProgress(errorMessage.includes("rebase-merge"));
+      updateSyncState({
+        syncError: errorMessage,
+        rebaseInProgress: errorMessage.includes("rebase-merge"),
+      });
       // If rebase failed, show appropriate message
       if (errorMessage.includes("rebase")) {
-        setRebaseStatusMessage(
+        updateSyncState({
+          rebaseStatusMessage:
             "Rebase failed. You may need to resolve conflicts or abort the rebase.",
-        );
+        });
       }
       // Clear any stale rebase success message if sync failed after rebase
       if (errorMessage.includes("sync") || errorMessage.includes("push")) {
-        setRebaseStatusMessage(null);
+        updateSyncState({ rebaseStatusMessage: null });
       }
     } finally {
       // Ensure syncing state is reset whether rebase or sync fails before handleSyncToGithub runs its own cleanup
-      setIsSyncing(false);
+      updateSyncState({ isSyncing: false });
     }
-  }, [appId, handleSyncToGithub]);
+  }, [appId, handleSyncToGithub, updateSyncState]);
 
   // Auto-sync when triggerAutoSync prop is true
   useEffect(() => {