diff --git a/docs/architecture.md b/docs/architecture.md index f11c2ab2ee..0ed8c46ade 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -355,13 +355,13 @@ Two workspaces can share the same `cwd` (e.g. a `directory` workspace and a `loc | State | Key builder / store | Source | | ---------------------------- | -------------------------------------------------- | ------------------------------------------------------------- | -| Review draft comments | `buildReviewDraftKey` / `buildReviewDraftScopeKey` | `packages/app/src/review/store.ts` | -| Diff mode override | review-draft scope key (in-memory) | `packages/app/src/review/state.ts` | +| Review draft comments | `buildReviewDraftKey` | `packages/app/src/review/store.ts` | +| Working diff comparison | `workingDiffComparisonKey` (in-memory) | `packages/app/src/git/working-diff-comparison/state.ts` | | Composer attachments | `buildWorkspaceAttachmentScopeKey` | `packages/app/src/attachments/workspace-attachments-store.ts` | | File explorer nav/open state | `fileExplorer` map keyed `workspace:{workspaceId}` | `packages/app/src/hooks/use-file-explorer-actions.ts` | | File explorer expanded paths | `expandedPathsByWorkspace[workspaceStateKey]` | `packages/app/src/stores/panel-store/state.ts` | -`diff-pane.tsx` is the canonical wiring site: it passes `{ serverId, cwd }` to the git queries and `{ serverId, workspaceId, cwd }` to the draft/override/attachment scope keys. +`diff-pane.tsx` is the canonical wiring site: it passes `{ serverId, cwd }` to the git queries and `{ serverId, workspaceId, cwd }` to the draft/comparison/attachment scope keys. **Do not "fix" the sharing away.** Re-keying a directory-backed query by `workspaceId` makes same-`cwd` workspaces diverge (two windows onto the same git tree showing different diffs). Re-keying owned state (drafts, expanded paths) by `cwd` makes them leak between distinct workspaces on the same folder. The `workspaceId`-keyed builders carry a `// workspaceId is opaque; do not parse this key back into a path.` comment — the opaque-id fallback to `cwd` exists only for old payloads without a `workspaceId`, not as a content-sharing mechanism. diff --git a/packages/app/e2e/browser/changes-pane.spec.ts b/packages/app/e2e/browser/changes-pane.spec.ts index 5da64049bb..655577883f 100644 --- a/packages/app/e2e/browser/changes-pane.spec.ts +++ b/packages/app/e2e/browser/changes-pane.spec.ts @@ -1,5 +1,5 @@ import { execFileSync } from "node:child_process"; -import { readFile, unlink, writeFile } from "node:fs/promises"; +import { mkdir, readFile, unlink, writeFile } from "node:fs/promises"; import path from "node:path"; import { type Locator, type Page } from "@playwright/test"; import { buildHostWorkspaceRoute, buildSettingsSectionRoute } from "../../src/utils/host-routes"; @@ -312,6 +312,7 @@ test("an empty Changes comparison links to the populated comparison", async ({ p await page.getByTestId("changes-diff-mode-uncommitted").click(); await expect(tree.getByText("No uncommitted changes", { exact: true })).toBeVisible(); + await expect(panel.getByText("No uncommitted changes", { exact: true })).toBeVisible(); const seeCommitted = tree.getByRole("button", { name: "See committed changes" }); await expect(seeCommitted).toBeVisible(); await seeCommitted.click(); @@ -319,6 +320,21 @@ test("an empty Changes comparison links to the populated comparison", async ({ p await expect(panel.getByTestId("diff-file-0")).toHaveAccessibleName("committed-only.ts, +1, -0"); }); +test("Changes comparison controls the working diff and tree selection focuses its file", async ({ + page, +}) => { + const workspace = await createWorkspaceWithScrollableComparisons(); + await openWorkspaceChangesSurface(page, workspace); + + await selectChangesComparison(page, "Committed"); + await expectWorkingComparisonFiles(page, "committed"); + await selectChangedFileAndExpectFocused(page, "committed/50-target.ts"); + + await selectChangesComparison(page, "Uncommitted"); + await expectWorkingComparisonFiles(page, "uncommitted"); + await selectChangedFileAndExpectFocused(page, "uncommitted/50-target.ts"); +}); + test("changes file actions open below the right-click without a reserved kebab", async ({ page, }) => { @@ -1466,6 +1482,46 @@ async function createWorkspaceWithCommittedDiff(): Promise { return { id: created.workspace.id, repoPath: repo.path }; } +async function createWorkspaceWithScrollableComparisons(): Promise { + const repo = await createTempGitRepo("changes-comparison-focus-", { + files: [{ path: "tracked.ts", content: "export const tracked = true;\n" }], + }); + const client = await connectSeedClient(); + cleanupTasks.push({ + run: async () => { + await client.close().catch(() => undefined); + await repo.cleanup().catch(() => undefined); + }, + }); + + const longFile = Array.from( + { length: 120 }, + (_, index) => `export const line${index} = ${index};`, + ).join("\n"); + execFileSync("git", ["checkout", "-b", "feature"], { cwd: repo.path }); + await writeComparisonFiles(repo.path, "committed", longFile); + execFileSync("git", ["add", "committed"], { cwd: repo.path }); + execFileSync("git", ["commit", "-m", "Add committed comparison files"], { cwd: repo.path }); + await writeComparisonFiles(repo.path, "uncommitted", longFile); + + const created = await client.createWorkspace({ source: { kind: "directory", path: repo.path } }); + if (!created.workspace) throw new Error(created.error ?? "Failed to create comparison workspace"); + return { id: created.workspace.id, repoPath: repo.path }; +} + +async function writeComparisonFiles( + repoPath: string, + directory: "committed" | "uncommitted", + longFile: string, +): Promise { + await mkdir(path.join(repoPath, directory), { recursive: true }); + await Promise.all([ + writeFile(path.join(repoPath, directory, "00-before.ts"), `${longFile}\n`), + writeFile(path.join(repoPath, directory, "50-target.ts"), "export const target = true;\n"), + writeFile(path.join(repoPath, directory, "99-after.ts"), `${longFile}\n`), + ]); +} + async function createWorkspaceWithExactSelectionDiff(content: string): Promise { const repo = await createTempGitRepo("changes-canvas-selection-", { files: [{ path: "src/selection.ts", content: "" }], @@ -1583,6 +1639,54 @@ async function openChangesInVisibleExplorer(page: Page): Promise { }); } +async function selectChangesComparison( + page: Page, + comparison: "Committed" | "Uncommitted", +): Promise { + const tree = page.getByTestId("changes-tree-panel").filter({ visible: true }); + await tree.getByTestId("changes-diff-status-trigger").click(); + await page.getByTestId(`changes-diff-mode-${comparison.toLowerCase()}`).click(); + await expect(tree.getByTestId("changes-diff-status-trigger")).toContainText(comparison); +} + +async function expectWorkingComparisonFiles( + page: Page, + comparison: "committed" | "uncommitted", +): Promise { + const tree = changesTree(page); + const panel = page.getByTestId("working-diff-panel").filter({ visible: true }); + const paths = ["00-before.ts", "50-target.ts", "99-after.ts"]; + await expect(tree.locator('[data-testid^="diff-tree-file-"][data-testid$="-name"]')).toHaveText( + paths, + ); + for (const fileName of paths) { + await expect(diffHeaderForPath(panel, `${comparison}/${fileName}`)).toBeAttached(); + } +} + +async function selectChangedFileAndExpectFocused(page: Page, filePath: string): Promise { + const tree = changesTree(page); + const panel = page.getByTestId("working-diff-panel").filter({ visible: true }); + const scroller = panel.getByTestId("git-diff-scroll"); + await scroller.evaluate((element) => { + element.scrollTop = 0; + element.dispatchEvent(new Event("scroll", { bubbles: false })); + }); + + await tree.getByText(path.basename(filePath), { exact: true }).click(); + const header = diffHeaderForPath(panel, filePath); + await expect(header).toBeVisible(); + await expect.poll(() => scroller.evaluate((element) => element.scrollTop)).toBeGreaterThan(0); + + const [scrollBounds, headerBounds] = await Promise.all([ + scroller.boundingBox(), + header.boundingBox(), + ]); + if (!scrollBounds || !headerBounds) + throw new Error("Focused diff header geometry is unavailable"); + expect(headerBounds.y - scrollBounds.y).toBeCloseTo(0, 0); +} + async function expectExpandedMountedTabDiff(page: Page): Promise { await expect(page.getByTestId("diff-file-0-body")).toBeVisible({ timeout: 30_000 }); await expect(page.getByTestId("git-diff-canvas")).toBeVisible({ timeout: 30_000 }); diff --git a/packages/app/src/components/compact-explorer-sidebar.tsx b/packages/app/src/components/compact-explorer-sidebar.tsx index fc4d00cf2f..0f51f68a52 100644 --- a/packages/app/src/components/compact-explorer-sidebar.tsx +++ b/packages/app/src/components/compact-explorer-sidebar.tsx @@ -462,7 +462,6 @@ function ChangedFilesPane({ workspaceId={workspaceId} cwd={workspaceRoot} enabled={isOpen} - modeScope="compact-explorer" onOpenFile={onOpenFile} onAddToChat={canAddToChat ? addFile : undefined} state={changesState} diff --git a/packages/app/src/git/checkout-status-cache.test.ts b/packages/app/src/git/checkout-status-cache.test.ts index df8952749c..8ab38178e4 100644 --- a/packages/app/src/git/checkout-status-cache.test.ts +++ b/packages/app/src/git/checkout-status-cache.test.ts @@ -11,7 +11,11 @@ import { prPanePipelineQueryKey, prPaneTimelineQueryKey, } from "@/git/pull-request-panel/query-keys"; -import { resetReviewDraftStore, useReviewDraftStore } from "@/review/store"; +import { + resetWorkingDiffComparisons, + resolveWorkingDiffComparison, + selectWorkingDiffComparison, +} from "@/git/working-diff-comparison"; import { applyCheckoutStatusUpdateFromEvent, ensureCheckoutStatus, @@ -87,10 +91,12 @@ function checkoutStatusUpdate( }; } -function setDiffModeOverride(isDirtyAtSelection: boolean): void { - useReviewDraftStore.getState().setDiffModeOverride({ - scopeKey: "review:scope", - override: { serverId, cwd, mode: "base", isDirtyAtSelection }, +function selectBaseComparison(isDirtyAtSelection: boolean): void { + selectWorkingDiffComparison({ + serverId, + cwd, + comparison: "base", + isDirty: isDirtyAtSelection, }); } @@ -99,7 +105,7 @@ function createQueryClient(): QueryClient { } beforeEach(() => { - resetReviewDraftStore(); + resetWorkingDiffComparisons(); }); describe("fetchCheckoutStatus", () => { @@ -113,13 +119,14 @@ describe("fetchCheckoutStatus", () => { expect(client.getCheckoutStatus).toHaveBeenCalledExactlyOnceWith(cwd); }); - it("expires a manual diff-mode override when the fetched dirty state flipped", async () => { - setDiffModeOverride(true); + it("expires a manual working-diff comparison when the fetched dirty state flipped", async () => { + selectBaseComparison(true); const client = { getCheckoutStatus: vi.fn(async () => checkoutStatus({ isDirty: false })) }; await fetchCheckoutStatus({ client, serverId, cwd }); - expect(useReviewDraftStore.getState().diffModeOverrides["review:scope"]).toBeUndefined(); + expect(resolveWorkingDiffComparison({ serverId, cwd, isDirty: false })).toBe("base"); + expect(resolveWorkingDiffComparison({ serverId, cwd, isDirty: true })).toBe("uncommitted"); }); }); @@ -233,9 +240,9 @@ describe("applyCheckoutStatusUpdateFromEvent", () => { ).toBe("unauthenticated"); }); - it("expires a manual diff-mode override when the pushed dirty state flipped", () => { + it("expires a manual working-diff comparison when the pushed dirty state flipped", () => { const queryClient = createQueryClient(); - setDiffModeOverride(false); + selectBaseComparison(false); applyCheckoutStatusUpdateFromEvent({ queryClient, @@ -243,12 +250,12 @@ describe("applyCheckoutStatusUpdateFromEvent", () => { message: checkoutStatusUpdate(checkoutStatus({ isDirty: true })), }); - expect(useReviewDraftStore.getState().diffModeOverrides["review:scope"]).toBeUndefined(); + expect(resolveWorkingDiffComparison({ serverId, cwd, isDirty: true })).toBe("uncommitted"); }); - it("keeps a manual diff-mode override while the pushed dirty state still matches", () => { + it("keeps a manual working-diff comparison while the pushed dirty state still matches", () => { const queryClient = createQueryClient(); - setDiffModeOverride(true); + selectBaseComparison(true); applyCheckoutStatusUpdateFromEvent({ queryClient, @@ -256,7 +263,7 @@ describe("applyCheckoutStatusUpdateFromEvent", () => { message: checkoutStatusUpdate(checkoutStatus({ isDirty: true })), }); - expect(useReviewDraftStore.getState().diffModeOverrides["review:scope"]).toBeDefined(); + expect(resolveWorkingDiffComparison({ serverId, cwd, isDirty: true })).toBe("base"); }); it("invalidates PR detail queries when the prStatus changes, ignoring the volatile requestId", () => { diff --git a/packages/app/src/git/checkout-status-cache.ts b/packages/app/src/git/checkout-status-cache.ts index 1f76dc67b1..97a26e099b 100644 --- a/packages/app/src/git/checkout-status-cache.ts +++ b/packages/app/src/git/checkout-status-cache.ts @@ -8,7 +8,7 @@ import { invalidatePrPaneTimelineForCheckout, } from "@/git/query-keys"; import { type CheckoutPrStatusPayload, normalizeCheckoutPrStatusPayload } from "@/git/pr-status"; -import { expireStaleDiffModeOverrides } from "@/review/store"; +import { expireWorkingDiffComparisons } from "@/git/working-diff-comparison"; export type CheckoutStatusPayload = CheckoutStatusResponse["payload"]; export type { CheckoutPrStatusPayload } from "@/git/pr-status"; @@ -31,7 +31,7 @@ export async function fetchCheckoutStatus({ cwd: string; }): Promise { const payload = await client.getCheckoutStatus(cwd); - expireStaleDiffModeOverrides({ serverId, cwd, isDirty: payload.isGit && payload.isDirty }); + expireWorkingDiffComparisons({ serverId, cwd, isDirty: payload.isGit && payload.isDirty }); return payload; } @@ -71,7 +71,7 @@ export function applyCheckoutStatusUpdateFromEvent({ void queryClient.invalidateQueries({ queryKey: checkoutCommitsQueryKey(serverId, payload.cwd), }); - expireStaleDiffModeOverrides({ + expireWorkingDiffComparisons({ serverId, cwd: payload.cwd, isDirty: payload.isGit && payload.isDirty, diff --git a/packages/app/src/git/diff-pane.tsx b/packages/app/src/git/diff-pane.tsx index 71470b2646..b934d56b25 100644 --- a/packages/app/src/git/diff-pane.tsx +++ b/packages/app/src/git/diff-pane.tsx @@ -187,7 +187,6 @@ interface ChangesSurfaceProps { cwd: string; enabled?: boolean; presentation?: ChangesPresentation; - modeScope: string; focusPath?: string; focusRequestId?: number; onOpenFile?: (path: string) => void; @@ -1565,7 +1564,6 @@ export function ChangesSurface({ cwd, enabled, presentation = "combined", - modeScope, focusPath, focusRequestId, onOpenFile, @@ -1679,7 +1677,6 @@ export function ChangesSurface({ cwd, ignoreWhitespace: preferences.hideWhitespace, enabled: enabled !== false, - modeScope, }); usePublishWorkingDiffAttachment({ serverId, diff --git a/packages/app/src/git/use-working-diff.ts b/packages/app/src/git/use-working-diff.ts index ed150aa81f..341cc4946d 100644 --- a/packages/app/src/git/use-working-diff.ts +++ b/packages/app/src/git/use-working-diff.ts @@ -5,14 +5,12 @@ import { } from "@/attachments/workspace-attachments-store"; import { buildReviewDraftKey, - buildReviewDraftScopeKey, useInlineReviewController, - useResolvedDiffMode, useReviewAttachmentSnapshot, - useSetDiffModeOverride, } from "@/review"; import { useCheckoutDiffQuery } from "@/git/use-diff-query"; import { useCheckoutStatusQuery } from "@/git/use-status-query"; +import { useWorkingDiffComparison } from "@/git/working-diff-comparison"; interface UseWorkingDiffOptions { serverId: string; @@ -21,7 +19,6 @@ interface UseWorkingDiffOptions { ignoreWhitespace: boolean; enabled: boolean; queryScope?: string; - modeScope: string; } export function useWorkingDiff({ @@ -31,7 +28,6 @@ export function useWorkingDiff({ ignoreWhitespace, enabled, queryScope, - modeScope, }: UseWorkingDiffOptions) { const { status, @@ -50,39 +46,14 @@ export function useWorkingDiff({ const currentBranchName = gitStatus?.currentBranch && gitStatus.currentBranch !== "HEAD" ? gitStatus.currentBranch : null; - const reviewDraftScopeKey = useMemo( - () => - buildReviewDraftScopeKey({ - serverId, - workspaceId, - cwd, - baseRef, - ignoreWhitespace, - }), - [baseRef, cwd, ignoreWhitespace, serverId, workspaceId], - ); - const modeScopeKey = `${reviewDraftScopeKey}:surface=${encodeURIComponent(modeScope)}`; - const diffMode = useResolvedDiffMode({ - scopeKey: modeScopeKey, - hasUncommittedChanges, + const { comparison: diffMode, selectComparison } = useWorkingDiffComparison({ + serverId, + workspaceId, + cwd, + isDirty: hasUncommittedChanges, }); - const setDiffModeOverride = useSetDiffModeOverride(); - const selectDiffMode = useCallback( - (nextMode: "uncommitted" | "base") => { - setDiffModeOverride({ - scopeKey: modeScopeKey, - override: { - serverId, - cwd, - mode: nextMode, - isDirtyAtSelection: hasUncommittedChanges, - }, - }); - }, - [cwd, hasUncommittedChanges, modeScopeKey, serverId, setDiffModeOverride], - ); - const selectUncommitted = useCallback(() => selectDiffMode("uncommitted"), [selectDiffMode]); - const selectBase = useCallback(() => selectDiffMode("base"), [selectDiffMode]); + const selectUncommitted = useCallback(() => selectComparison("uncommitted"), [selectComparison]); + const selectBase = useCallback(() => selectComparison("base"), [selectComparison]); const { files, diff --git a/packages/app/src/git/working-diff-comparison/index.ts b/packages/app/src/git/working-diff-comparison/index.ts new file mode 100644 index 0000000000..bee2c81b22 --- /dev/null +++ b/packages/app/src/git/working-diff-comparison/index.ts @@ -0,0 +1,72 @@ +import { useCallback } from "react"; +import { create } from "zustand"; +import { + expireWorkingDiffComparisonsInState, + resolveWorkingDiffComparisonFromState, + selectWorkingDiffComparisonInState, + type WorkingDiffCheckoutIdentity, + type WorkingDiffComparison, + type WorkingDiffComparisonState, +} from "./state"; + +interface WorkingDiffComparisonStore extends WorkingDiffComparisonState { + select: ( + input: WorkingDiffCheckoutIdentity & { + comparison: WorkingDiffComparison; + isDirty: boolean; + }, + ) => void; +} + +const useWorkingDiffComparisonStore = create((set) => ({ + overrides: {}, + select: (input) => set((state) => selectWorkingDiffComparisonInState(state, input)), +})); + +export function useWorkingDiffComparison( + input: WorkingDiffCheckoutIdentity & { isDirty: boolean }, +): { + comparison: WorkingDiffComparison; + selectComparison: (comparison: WorkingDiffComparison) => void; +} { + const { serverId, workspaceId, cwd, isDirty } = input; + const comparison = useWorkingDiffComparisonStore((state) => + resolveWorkingDiffComparisonFromState(state, { serverId, workspaceId, cwd, isDirty }), + ); + const select = useWorkingDiffComparisonStore((state) => state.select); + const selectComparison = useCallback( + (next: WorkingDiffComparison) => + select({ serverId, workspaceId, cwd, isDirty, comparison: next }), + [cwd, isDirty, select, serverId, workspaceId], + ); + return { comparison, selectComparison }; +} + +export function selectWorkingDiffComparison( + input: WorkingDiffCheckoutIdentity & { + comparison: WorkingDiffComparison; + isDirty: boolean; + }, +): void { + useWorkingDiffComparisonStore.getState().select(input); +} + +export function resolveWorkingDiffComparison( + input: WorkingDiffCheckoutIdentity & { isDirty: boolean }, +): WorkingDiffComparison { + return resolveWorkingDiffComparisonFromState(useWorkingDiffComparisonStore.getState(), input); +} + +export function expireWorkingDiffComparisons(input: { + serverId: string; + cwd: string; + isDirty: boolean; +}): void { + useWorkingDiffComparisonStore.setState((state) => + expireWorkingDiffComparisonsInState(state, input), + ); +} + +export function resetWorkingDiffComparisons(): void { + useWorkingDiffComparisonStore.setState({ overrides: {} }); +} diff --git a/packages/app/src/git/working-diff-comparison/state.test.ts b/packages/app/src/git/working-diff-comparison/state.test.ts new file mode 100644 index 0000000000..f86038bdc1 --- /dev/null +++ b/packages/app/src/git/working-diff-comparison/state.test.ts @@ -0,0 +1,104 @@ +import { describe, expect, it } from "vitest"; +import { + expireWorkingDiffComparisonsInState, + resolveWorkingDiffComparisonFromState, + selectWorkingDiffComparisonInState, + type WorkingDiffComparisonState, + workingDiffComparisonKey, +} from "./state"; + +const checkout = { serverId: "server-1", workspaceId: "workspace-1", cwd: "/repo" }; + +function emptyState(): WorkingDiffComparisonState { + return { overrides: {} }; +} + +describe("working diff comparison", () => { + it("scopes selection to the workspace checkout rather than a panel", () => { + expect(workingDiffComparisonKey(checkout)).toBe( + "working-diff:server=server-1:workspace=workspace-1", + ); + expect(workingDiffComparisonKey({ ...checkout, workspaceId: "workspace-2" })).not.toBe( + workingDiffComparisonKey(checkout), + ); + expect(workingDiffComparisonKey({ ...checkout, workspaceId: null, cwd: "/repo/" })).toBe( + "working-diff:server=server-1:cwd=%2Frepo", + ); + }); + + it("defaults from checkout dirtiness and honors a matching manual selection", () => { + expect( + resolveWorkingDiffComparisonFromState(emptyState(), { ...checkout, isDirty: true }), + ).toBe("uncommitted"); + expect( + resolveWorkingDiffComparisonFromState(emptyState(), { ...checkout, isDirty: false }), + ).toBe("base"); + + const selected = selectWorkingDiffComparisonInState(emptyState(), { + ...checkout, + comparison: "base", + isDirty: true, + }); + expect(resolveWorkingDiffComparisonFromState(selected, { ...checkout, isDirty: true })).toBe( + "base", + ); + }); + + it("masks and expires stale selections for every workspace on the checkout", () => { + let state = selectWorkingDiffComparisonInState(emptyState(), { + ...checkout, + comparison: "base", + isDirty: true, + }); + state = selectWorkingDiffComparisonInState(state, { + ...checkout, + workspaceId: "workspace-2", + comparison: "uncommitted", + isDirty: false, + }); + state = selectWorkingDiffComparisonInState(state, { + serverId: "server-2", + workspaceId: "workspace-3", + cwd: "/other", + comparison: "base", + isDirty: true, + }); + + expect(resolveWorkingDiffComparisonFromState(state, { ...checkout, isDirty: false })).toBe( + "base", + ); + const expired = expireWorkingDiffComparisonsInState(state, { + serverId: checkout.serverId, + cwd: checkout.cwd, + isDirty: false, + }); + expect(expired.overrides[workingDiffComparisonKey(checkout)]).toBeUndefined(); + expect( + expired.overrides[workingDiffComparisonKey({ ...checkout, workspaceId: "workspace-2" })], + ).toBeDefined(); + expect( + expired.overrides[ + workingDiffComparisonKey({ + serverId: "server-2", + workspaceId: "workspace-3", + cwd: "/other", + }) + ], + ).toBeDefined(); + }); + + it("keeps state identity when nothing expires", () => { + const state = selectWorkingDiffComparisonInState(emptyState(), { + ...checkout, + comparison: "base", + isDirty: true, + }); + expect( + expireWorkingDiffComparisonsInState(state, { + serverId: checkout.serverId, + cwd: checkout.cwd, + isDirty: true, + }), + ).toBe(state); + }); +}); diff --git a/packages/app/src/git/working-diff-comparison/state.ts b/packages/app/src/git/working-diff-comparison/state.ts new file mode 100644 index 0000000000..5c7c141376 --- /dev/null +++ b/packages/app/src/git/working-diff-comparison/state.ts @@ -0,0 +1,85 @@ +export type WorkingDiffComparison = "uncommitted" | "base"; + +export interface WorkingDiffComparisonOverride { + serverId: string; + cwd: string; + comparison: WorkingDiffComparison; + isDirtyAtSelection: boolean; +} + +export interface WorkingDiffComparisonState { + overrides: Record; +} + +export interface WorkingDiffCheckoutIdentity { + serverId: string; + workspaceId?: string | null; + cwd: string; +} + +function normalizeCwd(cwd: string): string { + const trimmed = cwd.trim(); + return trimmed === "/" ? trimmed : trimmed.replace(/\/+$/, ""); +} + +export function workingDiffComparisonKey(input: WorkingDiffCheckoutIdentity): string { + const workspaceId = input.workspaceId?.trim(); + const checkout = workspaceId + ? `workspace=${encodeURIComponent(workspaceId)}` + : `cwd=${encodeURIComponent(normalizeCwd(input.cwd))}`; + return `working-diff:server=${encodeURIComponent(input.serverId.trim())}:${checkout}`; +} + +export function selectWorkingDiffComparisonInState( + state: WorkingDiffComparisonState, + input: WorkingDiffCheckoutIdentity & { + comparison: WorkingDiffComparison; + isDirty: boolean; + }, +): WorkingDiffComparisonState { + return { + overrides: { + ...state.overrides, + [workingDiffComparisonKey(input)]: { + serverId: input.serverId.trim(), + cwd: normalizeCwd(input.cwd), + comparison: input.comparison, + isDirtyAtSelection: input.isDirty, + }, + }, + }; +} + +export function resolveWorkingDiffComparisonFromState( + state: WorkingDiffComparisonState, + input: WorkingDiffCheckoutIdentity & { isDirty: boolean }, +): WorkingDiffComparison { + const override = state.overrides[workingDiffComparisonKey(input)]; + // Status can render before boundary expiry runs, so resolution must also mask a stale + // selection under any ordering of the two updates. + if (override?.isDirtyAtSelection === input.isDirty) { + return override.comparison; + } + return input.isDirty ? "uncommitted" : "base"; +} + +export function expireWorkingDiffComparisonsInState( + state: WorkingDiffComparisonState, + input: { serverId: string; cwd: string; isDirty: boolean }, +): WorkingDiffComparisonState { + // Expire at the status boundary so selections cannot return after an unmounted surface + // misses a dirty-state transition. + const staleKeys = Object.entries(state.overrides) + .filter( + ([, override]) => + override.serverId === input.serverId.trim() && + override.cwd === normalizeCwd(input.cwd) && + override.isDirtyAtSelection !== input.isDirty, + ) + .map(([key]) => key); + if (staleKeys.length === 0) return state; + + const overrides = { ...state.overrides }; + for (const key of staleKeys) delete overrides[key]; + return { overrides }; +} diff --git a/packages/app/src/panels/diff-panel.tsx b/packages/app/src/panels/diff-panel.tsx index adad98e690..1297b278bb 100644 --- a/packages/app/src/panels/diff-panel.tsx +++ b/packages/app/src/panels/diff-panel.tsx @@ -135,7 +135,6 @@ function ChangesPanel() { cwd={cwd} enabled={isActive} presentation={presentation} - modeScope={tabId} focusPath={target.kind === "working_diff" ? target.focusPath : undefined} focusRequestId={target.kind === "working_diff" ? target.focusRequestId : undefined} onSelectDiffFile={isTree ? handleSelectDiffFile : undefined} diff --git a/packages/app/src/review/index.ts b/packages/app/src/review/index.ts index a98a545793..014447090b 100644 --- a/packages/app/src/review/index.ts +++ b/packages/app/src/review/index.ts @@ -1,18 +1,12 @@ export { buildReviewAttachmentSnapshot, buildReviewDraftKey, - buildReviewDraftScopeKey, - expireStaleDiffModeOverrides, getReviewDraftComments, resetReviewDraftStore, useClearReviewDraft, useReviewAttachmentSnapshot, - useResolvedDiffMode, - useSetDiffModeOverride, addReviewDraftComment, type BuildReviewDraftKeyInput, - type BuildReviewDraftScopeKeyInput, - type DiffModeOverride, type ReviewDraftCommentInput, type ReviewDraftComment, type ReviewDraftMode, diff --git a/packages/app/src/review/state.ts b/packages/app/src/review/state.ts index 1b5057a648..20fe98e098 100644 --- a/packages/app/src/review/state.ts +++ b/packages/app/src/review/state.ts @@ -13,23 +13,10 @@ export interface ReviewDraftComment { updatedAt: string; } -// A manual mode selection is valid only while the checkout's dirty state matches the -// value at the time of selection. serverId/cwd identify the checkout so the override can -// be expired when its dirty state changes (see expireStaleDiffModeOverridesInState). -export interface DiffModeOverride { - serverId: string; - cwd: string; - mode: ReviewDraftMode; - isDirtyAtSelection: boolean; -} - export interface ReviewDraftStoreState { drafts: Record; - // In-memory only — not persisted. Keyed by scope key. - diffModeOverrides: Record; } -// Only drafts are persisted; diffModeOverrides is intentionally excluded. export interface SerializedReviewDraftState { drafts: Record; activeModesByScope?: Record; @@ -53,59 +40,6 @@ export const SerializedReviewDraftStateSchema: z.ZodType - override.serverId === input.serverId && - override.cwd === input.cwd && - override.isDirtyAtSelection !== input.isDirty, - ) - .map(([scopeKey]) => scopeKey); - if (staleScopeKeys.length === 0) { - return state; - } - const next = { ...state.diffModeOverrides }; - for (const scopeKey of staleScopeKeys) { - delete next[scopeKey]; - } - return { ...state, diffModeOverrides: next }; -} - -// Pure read — returns the effective mode without mutating state. The staleness check is -// kept even though stale overrides are expired at the data boundary: a render can observe -// a fresh dirty state before the expiry lands, and resolution must be correct under any -// interleaving. -export function resolveDiffMode(input: { - override: DiffModeOverride | undefined; - hasUncommittedChanges: boolean; -}): ReviewDraftMode { - const { override, hasUncommittedChanges } = input; - if (override && override.isDirtyAtSelection === hasUncommittedChanges) { - return override.mode; - } - return hasUncommittedChanges ? "uncommitted" : "base"; -} - export function addCommentToState( state: ReviewDraftStoreState, input: { key: string; comment: ReviewDraftComment }, @@ -184,7 +118,6 @@ export function normalizePersistedState(state: unknown): ReviewDraftStoreState { const result = SerializedReviewDraftStateSchema.safeParse(state); return { drafts: result.success ? result.data.drafts : {}, - diffModeOverrides: {}, }; } diff --git a/packages/app/src/review/store.test.ts b/packages/app/src/review/store.test.ts index 8c8dea9463..344b28b61d 100644 --- a/packages/app/src/review/store.test.ts +++ b/packages/app/src/review/store.test.ts @@ -2,35 +2,21 @@ import { describe, expect, it } from "vitest"; import type { StateStorage } from "zustand/middleware"; import type { ParsedDiffFile } from "@/git/use-diff-query"; import { createValidatedPersistStorage } from "@/storage/validated-persist-storage"; -import { - buildReviewAttachmentSnapshot, - buildReviewDraftKey, - buildReviewDraftScopeKey, -} from "./store"; +import { buildReviewAttachmentSnapshot, buildReviewDraftKey } from "./store"; import { addCommentToState, clearReviewInState, deleteCommentFromState, - type DiffModeOverride, - expireStaleDiffModeOverridesInState, normalizePersistedState, - resolveDiffMode, type ReviewDraftComment, type ReviewDraftStoreState, serializeReviewDraftState, SerializedReviewDraftStateSchema, - setDiffModeOverrideInState, updateCommentInState, } from "./state"; function emptyState(): ReviewDraftStoreState { - return { drafts: {}, diffModeOverrides: {} }; -} - -function makeOverride( - input: Pick, -): DiffModeOverride { - return { serverId: "server-1", cwd: "/repo", ...input }; + return { drafts: {} }; } function makeComment(overrides: Partial = {}): ReviewDraftComment { @@ -121,21 +107,6 @@ describe("buildReviewDraftKey", () => { }), ).toBe("review:server=local:cwd=%2Frepo:mode=base:base=main:ignoreWhitespace=false"); }); - - it("builds a mode-free scope key for diff mode override sharing", () => { - const scope = buildReviewDraftScopeKey({ - serverId: "local", - workspaceId: "workspace-1", - cwd: "/repo", - baseRef: "main", - ignoreWhitespace: false, - }); - - expect(scope).toBe( - "review:server=local:workspace=workspace-1:base=main:ignoreWhitespace=false", - ); - expect(scope).not.toContain("mode="); - }); }); describe("normalizePersistedState", () => { @@ -155,7 +126,6 @@ describe("normalizePersistedState", () => { const normalized = normalizePersistedState(stored?.state); expect(normalized.drafts["review:key"]).toEqual([makeComment()]); - expect(normalized.diffModeOverrides).toEqual({}); expect(backing.values.has("@paseo:review-draft-store")).toBe(true); }); @@ -182,149 +152,28 @@ describe("normalizePersistedState", () => { }, }); - expect(normalized.diffModeOverrides).toEqual({}); expect(normalized.drafts).toEqual({}); }); it("returns empty state for null, non-object, or malformed inputs", () => { - expect(normalizePersistedState(null)).toEqual({ drafts: {}, diffModeOverrides: {} }); - expect(normalizePersistedState("nope")).toEqual({ drafts: {}, diffModeOverrides: {} }); - expect(normalizePersistedState({ drafts: [] })).toEqual({ - drafts: {}, - diffModeOverrides: {}, - }); + expect(normalizePersistedState(null)).toEqual({ drafts: {} }); + expect(normalizePersistedState("nope")).toEqual({ drafts: {} }); + expect(normalizePersistedState({ drafts: [] })).toEqual({ drafts: {} }); }); }); 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 }), - }, - ); + it("serialized output does not contain the legacy activeModesByScope field", () => { + const state = addCommentToState(emptyState(), { key: "review:key", comment: makeComment() }); 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(); diff --git a/packages/app/src/review/store.ts b/packages/app/src/review/store.ts index 3f9d7c55ea..bac1659751 100644 --- a/packages/app/src/review/store.ts +++ b/packages/app/src/review/store.ts @@ -8,17 +8,13 @@ import { addCommentToState, clearReviewInState, deleteCommentFromState, - type DiffModeOverride, - expireStaleDiffModeOverridesInState, normalizePersistedState, - resolveDiffMode, type ReviewDraftComment, type ReviewDraftMode, type ReviewDraftSide, type ReviewDraftStoreState, serializeReviewDraftState, SerializedReviewDraftStateSchema, - setDiffModeOverrideInState, updateCommentInState, } from "@/review/state"; import { generateMessageId } from "@/types/stream"; @@ -26,14 +22,9 @@ import { buildNumberedDiffHunks, type NumberedDiffLine } from "@/utils/diff-layo import type { AgentAttachment } from "@getpaseo/protocol/messages"; import { createValidatedPersistStorage } from "@/storage/validated-persist-storage"; -export type { - DiffModeOverride, - ReviewDraftComment, - ReviewDraftMode, - ReviewDraftSide, -} from "@/review/state"; +export type { ReviewDraftComment, ReviewDraftMode, ReviewDraftSide } from "@/review/state"; -// v2 dropped persisted activeModesByScope (diff mode overrides are in-memory only). +// v2 dropped the legacy persisted activeModesByScope field. const STORE_VERSION = 2; const CONTEXT_RADIUS = 3; const EMPTY_REVIEW_DRAFT_COMMENTS: ReviewDraftComment[] = []; @@ -51,7 +42,7 @@ export interface BuildReviewDraftKeyInput { ignoreWhitespace: boolean; } -export type BuildReviewDraftScopeKeyInput = Omit; +type BuildReviewDraftScopeKeyInput = Omit; export interface BuildReviewAttachmentSnapshotInput { reviewDraftKey: string; @@ -66,7 +57,6 @@ export type ReviewDraftCommentInput = Omit>; interface ReviewDraftStoreActions { - setDiffModeOverride: (input: { scopeKey: string; override: DiffModeOverride }) => void; addComment: (input: { key: string; comment: ReviewDraftCommentInput }) => ReviewDraftComment; updateComment: (input: { key: string; @@ -112,10 +102,6 @@ function buildReviewDraftScopeParts(input: BuildReviewDraftScopeKeyInput): strin ]; } -export function buildReviewDraftScopeKey(input: BuildReviewDraftScopeKeyInput): string { - return buildReviewDraftScopeParts(input).join(":"); -} - export function buildReviewDraftKey(input: BuildReviewDraftKeyInput): string { const [prefix, serverPart, workspacePart, basePart, whitespacePart] = buildReviewDraftScopeParts(input); @@ -141,10 +127,6 @@ export const useReviewDraftStore = create()( persist( (set) => ({ drafts: {}, - diffModeOverrides: {}, - setDiffModeOverride: (input) => { - set((state) => setDiffModeOverrideInState(state, input)); - }, addComment: ({ key, comment }) => { const nextComment = createDraftComment(comment); set((state) => addCommentToState(state, { key, comment: nextComment })); @@ -283,20 +265,6 @@ export function useReviewDraftComments(key: string): ReviewDraftComment[] { return useReviewDraftStore((state) => state.drafts[key] ?? EMPTY_REVIEW_DRAFT_COMMENTS); } -export function useSetDiffModeOverride(): ReviewDraftStoreActions["setDiffModeOverride"] { - return useReviewDraftStore((state) => state.setDiffModeOverride); -} - -// Non-React entry point: called from the checkout-status data boundary, where dirty-state -// changes enter the app regardless of which screens are mounted. -export function expireStaleDiffModeOverrides(input: { - serverId: string; - cwd: string; - isDirty: boolean; -}): void { - useReviewDraftStore.setState((state) => expireStaleDiffModeOverridesInState(state, input)); -} - export function useClearReviewDraft(): ReviewDraftStoreActions["clearReview"] { return useReviewDraftStore((state) => state.clearReview); } @@ -313,7 +281,7 @@ export function getReviewDraftComments(key: string): ReviewDraftComment[] | unde } export function resetReviewDraftStore(): void { - useReviewDraftStore.setState({ drafts: {}, diffModeOverrides: {} }); + useReviewDraftStore.setState({ drafts: {} }); } export function useReviewDraftCommentsForAttachment(input: { @@ -331,18 +299,6 @@ export function useReviewCommentCount(key: string): number { return useReviewDraftStore((state) => state.drafts[key]?.length ?? 0); } -export function useResolvedDiffMode(input: { - scopeKey: string; - hasUncommittedChanges: boolean; -}): ReviewDraftMode { - return useReviewDraftStore((state) => - resolveDiffMode({ - override: state.diffModeOverrides[input.scopeKey], - hasUncommittedChanges: input.hasUncommittedChanges, - }), - ); -} - export function useReviewAttachmentSnapshot(input: { key: string; diffFiles: readonly ParsedDiffFile[]; diff --git a/packages/app/src/review/surface.test.tsx b/packages/app/src/review/surface.test.tsx index 8ae0c4d97a..e23f321d38 100644 --- a/packages/app/src/review/surface.test.tsx +++ b/packages/app/src/review/surface.test.tsx @@ -168,7 +168,7 @@ function comment(overrides: Partial = {}): ReviewDraftCommen describe("useInlineReviewController", () => { beforeEach(() => { - useReviewDraftStore.setState({ drafts: {}, diffModeOverrides: {} }); + useReviewDraftStore.setState({ drafts: {} }); }); afterEach(() => {