Keep git review state fresh after reconnect (#1491)

packages/app/src/review/store.test.ts

@@ -9,15 +9,25 @@ import {
   addCommentToState,
   clearReviewInState,
   deleteCommentFromState,
+  type DiffModeOverride,
+  expireStaleDiffModeOverridesInState,
   normalizePersistedState,
+  resolveDiffMode,
   type ReviewDraftComment,
   type ReviewDraftStoreState,
-  setActiveModeInState,
+  serializeReviewDraftState,
+  setDiffModeOverrideInState,
   updateCommentInState,
 } from "./state";
 
 function emptyState(): ReviewDraftStoreState {
-  return { drafts: {}, activeModesByScope: {} };
+  return { drafts: {}, diffModeOverrides: {} };
+}
+
+function makeOverride(
+  input: Pick<DiffModeOverride, "mode" | "isDirtyAtSelection">,
+): DiffModeOverride {
+  return { serverId: "server-1", cwd: "/repo", ...input };
 }
 
 function makeComment(overrides: Partial<ReviewDraftComment> = {}): ReviewDraftComment {
@@ -95,7 +105,7 @@ describe("buildReviewDraftKey", () => {
     ).toBe("review:server=local:cwd=%2Frepo:mode=base:base=main:ignoreWhitespace=false");
   });
 
-  it("builds a mode-free scope key for active review mode sharing", () => {
+  it("builds a mode-free scope key for diff mode override sharing", () => {
     const scope = buildReviewDraftScopeKey({
       serverId: "local",
       workspaceId: "workspace-1",
@@ -112,7 +122,7 @@ describe("buildReviewDraftKey", () => {
 });
 
 describe("normalizePersistedState", () => {
-  it("filters invalid draft comments and drops unknown active modes", () => {
+  it("filters invalid draft comments and ignores activeModesByScope from old persisted JSON", () => {
     const normalized = normalizePersistedState({
       drafts: {
         "review:key": [
@@ -128,17 +138,14 @@ describe("normalizePersistedState", () => {
           { id: "bad", filePath: "src/example.ts" },
         ],
       },
+      // Old persisted field — must be tolerated and ignored, not migrated.
       activeModesByScope: {
         "review:scope:base": "base",
         "review:scope:dirty": "uncommitted",
-        "review:scope:bad": "other",
       },
     });
 
-    expect(normalized.activeModesByScope).toEqual({
-      "review:scope:base": "base",
-      "review:scope:dirty": "uncommitted",
-    });
+    expect(normalized.diffModeOverrides).toEqual({});
     expect(normalized.drafts["review:key"]).toEqual([
       {
         id: "comment-1",
@@ -153,15 +160,144 @@ describe("normalizePersistedState", () => {
   });
 
   it("returns empty state for null, non-object, or malformed inputs", () => {
-    expect(normalizePersistedState(null)).toEqual({ drafts: {}, activeModesByScope: {} });
-    expect(normalizePersistedState("nope")).toEqual({ drafts: {}, activeModesByScope: {} });
+    expect(normalizePersistedState(null)).toEqual({ drafts: {}, diffModeOverrides: {} });
+    expect(normalizePersistedState("nope")).toEqual({ drafts: {}, diffModeOverrides: {} });
     expect(normalizePersistedState({ drafts: [] })).toEqual({
       drafts: {},
-      activeModesByScope: {},
+      diffModeOverrides: {},
     });
   });
 });
 
+describe("serializeReviewDraftState", () => {
+  it("serialized output does not contain activeModesByScope or diffModeOverrides", () => {
+    const state = setDiffModeOverrideInState(
+      addCommentToState(emptyState(), { key: "review:key", comment: makeComment() }),
+      {
+        scopeKey: "review:scope",
+        override: makeOverride({ mode: "uncommitted", isDirtyAtSelection: true }),
+      },
+    );
+
+    const serialized = serializeReviewDraftState(state);
+
+    expect(Object.keys(serialized)).toEqual(["drafts"]);
+    expect("activeModesByScope" in serialized).toBe(false);
+    expect("diffModeOverrides" in serialized).toBe(false);
+    expect(serialized.drafts["review:key"]).toHaveLength(1);
+  });
+});
+
+describe("diff mode override", () => {
+  it("resolves to auto mode from the dirty state when no override exists", () => {
+    expect(resolveDiffMode({ override: undefined, hasUncommittedChanges: true })).toBe(
+      "uncommitted",
+    );
+    expect(resolveDiffMode({ override: undefined, hasUncommittedChanges: false })).toBe("base");
+  });
+
+  it("honors the override while isDirty matches the value at selection, across remounts", () => {
+    const state = setDiffModeOverrideInState(emptyState(), {
+      scopeKey: "review:scope",
+      override: makeOverride({ mode: "base", isDirtyAtSelection: true }),
+    });
+
+    // Resolution is a plain store read, so it gives the same answer on every (re)mount.
+    const override = state.diffModeOverrides["review:scope"];
+    expect(resolveDiffMode({ override, hasUncommittedChanges: true })).toBe("base");
+  });
+
+  it("masks a stale override before its expiry lands", () => {
+    const state = setDiffModeOverrideInState(emptyState(), {
+      scopeKey: "review:scope",
+      override: makeOverride({ mode: "uncommitted", isDirtyAtSelection: true }),
+    });
+
+    const override = state.diffModeOverrides["review:scope"];
+    expect(resolveDiffMode({ override, hasUncommittedChanges: false })).toBe("base");
+  });
+
+  it("expires overrides for the checkout whose dirty state flipped, keeping the rest", () => {
+    let state = setDiffModeOverrideInState(emptyState(), {
+      scopeKey: "review:scope-a",
+      override: makeOverride({ mode: "base", isDirtyAtSelection: true }),
+    });
+    state = setDiffModeOverrideInState(state, {
+      scopeKey: "review:scope-b",
+      override: makeOverride({ mode: "uncommitted", isDirtyAtSelection: false }),
+    });
+    state = setDiffModeOverrideInState(state, {
+      scopeKey: "review:scope-other",
+      override: {
+        serverId: "server-2",
+        cwd: "/other",
+        mode: "base",
+        isDirtyAtSelection: true,
+      },
+    });
+
+    const next = expireStaleDiffModeOverridesInState(state, {
+      serverId: "server-1",
+      cwd: "/repo",
+      isDirty: false,
+    });
+
+    expect(next.diffModeOverrides["review:scope-a"]).toBeUndefined();
+    expect(next.diffModeOverrides["review:scope-b"]).toBeDefined();
+    expect(next.diffModeOverrides["review:scope-other"]).toBeDefined();
+  });
+
+  it("does not resurrect an expired override when the dirty state flips back", () => {
+    let state = setDiffModeOverrideInState(emptyState(), {
+      scopeKey: "review:scope",
+      override: makeOverride({ mode: "base", isDirtyAtSelection: true }),
+    });
+
+    // The checkout goes clean (e.g. agent commits): the override expires at the boundary.
+    state = expireStaleDiffModeOverridesInState(state, {
+      serverId: "server-1",
+      cwd: "/repo",
+      isDirty: false,
+    });
+    // The checkout goes dirty again: nothing to expire, and nothing comes back.
+    state = expireStaleDiffModeOverridesInState(state, {
+      serverId: "server-1",
+      cwd: "/repo",
+      isDirty: true,
+    });
+
+    expect(state.diffModeOverrides["review:scope"]).toBeUndefined();
+    expect(
+      resolveDiffMode({
+        override: state.diffModeOverrides["review:scope"],
+        hasUncommittedChanges: true,
+      }),
+    ).toBe("uncommitted");
+  });
+
+  it("keeps state identity when no override is stale", () => {
+    const state = setDiffModeOverrideInState(emptyState(), {
+      scopeKey: "review:scope",
+      override: makeOverride({ mode: "base", isDirtyAtSelection: true }),
+    });
+
+    expect(
+      expireStaleDiffModeOverridesInState(state, {
+        serverId: "server-1",
+        cwd: "/repo",
+        isDirty: true,
+      }),
+    ).toBe(state);
+    expect(
+      expireStaleDiffModeOverridesInState(emptyState(), {
+        serverId: "server-1",
+        cwd: "/repo",
+        isDirty: false,
+      }),
+    ).toEqual(emptyState());
+  });
+});
+
 describe("review draft reducers", () => {
   it("adds, updates, and deletes draft comments by key", () => {
     let state = emptyState();
@@ -203,18 +339,6 @@ describe("review draft reducers", () => {
     expect(deleteCommentFromState(state, { key: "review:key", id: "missing" })).toBe(state);
     expect(clearReviewInState(state, { key: "other-key" })).toBe(state);
   });
-
-  it("keeps state identity when setActiveMode does not change the mode", () => {
-    const state = setActiveModeInState(emptyState(), {
-      scopeKey: "review:scope",
-      mode: "base",
-    });
-
-    expect(setActiveModeInState(state, { scopeKey: "review:scope", mode: "base" })).toBe(state);
-    expect(setActiveModeInState(state, { scopeKey: "review:scope", mode: "uncommitted" })).not.toBe(
-      state,
-    );
-  });
 });
 
 describe("buildReviewAttachmentSnapshot", () => {