From d17812fdb1a6084cfa63f1f3f8c5f713b8cdc486 Mon Sep 17 00:00:00 2001 From: Alassane <79718216+yattdev@users.noreply.github.com> Date: Mon, 7 Sep 2026 16:31:06 -0400 Subject: [PATCH] fix(web): preserve pending git action ownership (#3373) Co-authored-by: ayattara Co-authored-by: Carlos Florencio --- .../task/changes-panel-file-row.test.tsx | 25 + .../e2e/tests/git/git-changes-panel.spec.ts | 119 +++- .../session/use-session-git-pending.test.tsx | 539 ++++++++++++++++++ .../session/use-session-git-pending.ts | 226 ++++++++ .../session/use-session-git-status.test.tsx | 41 ++ .../domains/session/use-session-git-status.ts | 26 + .../hooks/domains/session/use-session-git.ts | 128 ++++- apps/web/lib/state/app-state-types.ts | 2 + apps/web/lib/state/default-state.ts | 5 + .../session-runtime/session-runtime-slice.ts | 10 + .../lib/state/slices/session-runtime/types.ts | 8 + apps/web/lib/state/store-overrides.ts | 1 + apps/web/lib/types/git-events.ts | 2 + .../web/lib/ws/handlers/agent-session.test.ts | 7 + apps/web/lib/ws/handlers/agent-session.ts | 2 + apps/web/lib/ws/handlers/git-status.test.ts | 10 +- apps/web/lib/ws/handlers/git-status.ts | 18 +- .../changes-file-action-feedback.md | 34 +- .../changes-file-action-feedback.md | 55 +- 19 files changed, 1193 insertions(+), 65 deletions(-) create mode 100644 apps/web/hooks/domains/session/use-session-git-pending.test.tsx create mode 100644 apps/web/hooks/domains/session/use-session-git-pending.ts create mode 100644 apps/web/hooks/domains/session/use-session-git-status.test.tsx diff --git a/apps/web/components/task/changes-panel-file-row.test.tsx b/apps/web/components/task/changes-panel-file-row.test.tsx index 63666d80a9..6fc288717e 100644 --- a/apps/web/components/task/changes-panel-file-row.test.tsx +++ b/apps/web/components/task/changes-panel-file-row.test.tsx @@ -244,6 +244,31 @@ describe("FileRow hover swap (stats <-> actions occupy same cell)", () => { expect(statsLayer.className.split(/\s+/)).toContain("opacity-0"); expect(actionsLayer.className.split(/\s+/)).toContain("opacity-100"); }); + + it("keeps the pending spinner inside a touch-sized target for coarse pointers", () => { + responsive.isFinePointer = false; + const { container } = render( + +
    + +
+ , + ); + + const spinner = container.querySelector("svg.animate-spin"); + expect(spinner).not.toBeNull(); + expect(spinner?.parentElement?.className).toContain("min-h-11"); + expect(spinner?.parentElement?.className).toContain("min-w-11"); + }); }); describe("FileRow tree-mode hover stage action", () => { diff --git a/apps/web/e2e/tests/git/git-changes-panel.spec.ts b/apps/web/e2e/tests/git/git-changes-panel.spec.ts index 2245965fd7..391494efa6 100644 --- a/apps/web/e2e/tests/git/git-changes-panel.spec.ts +++ b/apps/web/e2e/tests/git/git-changes-panel.spec.ts @@ -99,26 +99,56 @@ type WorktreeFileAction = "worktree.stage" | "worktree.unstage"; type PausedWorktreeFileAction = { action: WorktreeFileAction; frame: string; + requestId: string; server: WebSocketRoute; }; -function isWorktreeFileActionFrame(part: string, action: WorktreeFileAction): boolean { - if (!part.trim()) return false; +type AwaitedWorktreeFileState = { + action: WorktreeFileAction; + requestId: string; + path: string; + staged: boolean; + response: { success: boolean; error?: string } | null; + fileStateObserved: boolean; +}; + +type WorktreeFrame = { + id?: unknown; + type?: unknown; + action?: unknown; + payload?: { + success?: unknown; + error?: unknown; + type?: unknown; + status?: { files?: Record }; + }; +}; + +function parseWorktreeFrame(part: string): WorktreeFrame | null { + if (!part.trim()) return null; try { - const frame = JSON.parse(part) as { type?: unknown; action?: unknown }; - return frame.type === "request" && frame.action === action; + return JSON.parse(part) as WorktreeFrame; } catch { - return false; + return null; } } +function worktreeFileActionRequestId(part: string, action: WorktreeFileAction): string | null { + const frame = parseWorktreeFrame(part); + return frame?.type === "request" && frame.action === action && typeof frame.id === "string" + ? frame.id + : null; +} + function partitionWorktreeFileAction( message: string | Buffer, action: WorktreeFileAction, ): { frame: string; passthrough: string | null } | null { if (typeof message !== "string") return null; const frames = message.split("\n"); - const actionIndex = frames.findIndex((part) => isWorktreeFileActionFrame(part, action)); + const actionIndex = frames.findIndex( + (part) => worktreeFileActionRequestId(part, action) !== null, + ); if (actionIndex === -1) return null; const passthrough = frames.filter((_, index) => index !== actionIndex).join("\n"); return { @@ -127,23 +157,63 @@ function partitionWorktreeFileAction( }; } +function observeWorktreeResponse(frame: WorktreeFrame, awaited: AwaitedWorktreeFileState) { + if ( + frame.type !== "response" || + frame.action !== awaited.action || + frame.id !== awaited.requestId + ) { + return; + } + awaited.response = { + success: frame.payload?.success === true, + ...(typeof frame.payload?.error === "string" ? { error: frame.payload.error } : {}), + }; +} + +function observeWorktreeFileState(frame: WorktreeFrame, awaited: AwaitedWorktreeFileState) { + if ( + frame.action === "session.git.event" && + frame.payload?.type === "status_update" && + frame.payload.status?.files?.[awaited.path]?.staged === awaited.staged + ) { + awaited.fileStateObserved = true; + } +} + +function observeWorktreeFileAction(message: string | Buffer, awaited: AwaitedWorktreeFileState) { + const parts = (typeof message === "string" ? message : message.toString("utf8")).split("\n"); + for (const part of parts) { + const frame = parseWorktreeFrame(part); + if (!frame) continue; + observeWorktreeResponse(frame, awaited); + observeWorktreeFileState(frame, awaited); + } +} + async function pauseNextWorktreeFileAction(page: Page) { let armedAction: WorktreeFileAction | null = null; let paused: PausedWorktreeFileAction | null = null; + let awaitedState: AwaitedWorktreeFileState | null = null; - await page.routeWebSocket(/\/ws$/, (socket) => { + await page.context().routeWebSocket(/\/ws$/, (socket) => { const server = socket.connectToServer(); socket.onMessage((message) => { const partition = armedAction ? partitionWorktreeFileAction(message, armedAction) : null; if (partition && armedAction) { - paused = { action: armedAction, frame: partition.frame, server }; + const requestId = worktreeFileActionRequestId(partition.frame, armedAction); + if (!requestId) throw new Error(`Paused ${armedAction} request has no ID`); + paused = { action: armedAction, frame: partition.frame, requestId, server }; armedAction = null; if (partition.passthrough) server.send(partition.passthrough); return; } server.send(message); }); - server.onMessage((message) => socket.send(message)); + server.onMessage((message) => { + if (awaitedState) observeWorktreeFileAction(message, awaitedState); + socket.send(message); + }); }); return { @@ -158,10 +228,33 @@ async function pauseNextWorktreeFileAction(page: Page) { }) .toBe(action); }, - release(action: WorktreeFileAction) { + async releaseAndWaitForFileState(action: WorktreeFileAction, path: string, staged: boolean) { if (paused?.action !== action) throw new Error(`No paused ${action} request`); - paused.server.send(paused.frame); + const request = paused; + awaitedState = { + action, + requestId: request.requestId, + path, + staged, + response: null, + fileStateObserved: false, + }; paused = null; + request.server.send(request.frame); + await expect + .poll(() => awaitedState?.response ?? null, { + message: `${action} should return a response`, + }) + .not.toBeNull(); + if (!awaitedState?.response?.success) { + throw new Error(awaitedState?.response?.error ?? `${action} failed`); + } + await expect + .poll(() => awaitedState?.fileStateObserved ?? false, { + message: `${action} should publish ${path} with staged=${staged}`, + }) + .toBe(true); + awaitedState = null; }, }; } @@ -349,7 +442,7 @@ test.describe("Git Changes Panel", () => { await expect(pendingStageSlot.locator("svg.animate-spin")).toBeVisible(); await expect(siblingRow.locator("svg.animate-spin")).toHaveCount(0); - pausedAction.release("worktree.stage"); + await pausedAction.releaseAndWaitForFileState("worktree.stage", "pending-action.txt", true); await session.expandChangesSection("staged-files-section"); const stagedSection = testPage.getByTestId("staged-files-section"); const stagedRow = stagedSection.getByTestId("file-row-pending-action.txt"); @@ -377,7 +470,7 @@ test.describe("Git Changes Panel", () => { ); await expect(pendingUnstageSlot.locator("svg.animate-spin")).toBeVisible(); - pausedAction.release("worktree.unstage"); + await pausedAction.releaseAndWaitForFileState("worktree.unstage", "pending-action.txt", false); await expect(unstagedSection.getByTestId("file-row-pending-action.txt")).toBeVisible(); await expect(stagedRow).toHaveCount(0); }); diff --git a/apps/web/hooks/domains/session/use-session-git-pending.test.tsx b/apps/web/hooks/domains/session/use-session-git-pending.test.tsx new file mode 100644 index 0000000000..74ab2d7b28 --- /dev/null +++ b/apps/web/hooks/domains/session/use-session-git-pending.test.tsx @@ -0,0 +1,539 @@ +import { act, cleanup, renderHook, waitFor } from "@testing-library/react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import type { GitStatusEntry } from "@/lib/state/slices/session-runtime/types"; +import { pendingKey, useSessionGit } from "./use-session-git"; +import { buildPendingFileStateIndex } from "./use-session-git-pending"; + +const mocks = vi.hoisted(() => ({ + statuses: [] as Array<{ repository_name: string; status: GitStatusEntry }>, + checkoutGenerations: {} as Record, + gitOps: { + isLoading: false, + loadingOperation: null, + pull: vi.fn(), + push: vi.fn(), + rebase: vi.fn(), + merge: vi.fn(), + abort: vi.fn(), + commit: vi.fn(), + stage: vi.fn(), + unstage: vi.fn(), + discard: vi.fn(), + revertCommit: vi.fn(), + renameBranch: vi.fn(), + reset: vi.fn(), + createPR: vi.fn(), + }, +})); + +vi.mock("./use-session-git-status", () => ({ + useSessionGitStatus: () => undefined, + useSessionGitStatusByRepo: () => mocks.statuses, + useSessionGitPendingCheckoutGenerations: () => mocks.checkoutGenerations, + useSessionGitPendingScope: (sessionId: string | null) => + sessionId ? `${sessionId}:environment` : "", +})); + +vi.mock("./use-session-commits", () => ({ + useSessionCommits: () => ({ commits: [], loading: false }), +})); + +vi.mock("./use-cumulative-diff", () => ({ + useCumulativeDiff: () => ({ diff: null }), +})); + +vi.mock("@/hooks/use-git-operations", () => ({ + useGitOperations: () => mocks.gitOps, +})); + +function status(path: string, staged: boolean, branch = "main"): GitStatusEntry { + return { + branch, + remote_branch: null, + modified: [path], + added: [], + deleted: [], + untracked: [], + renamed: [], + ahead: 0, + behind: 0, + files: { + [path]: { + path, + status: "modified", + staged, + }, + }, + timestamp: null, + }; +} + +type OperationResult = { + success: boolean; + operation: string; + output: string; + error?: string; +}; + +function deferredResult() { + let resolve!: (value: OperationResult) => void; + const promise = new Promise((done) => { + resolve = done; + }); + return { promise, resolve }; +} + +beforeEach(() => { + vi.clearAllMocks(); + mocks.statuses = []; + mocks.checkoutGenerations = {}; +}); + +afterEach(cleanup); + +describe("useSessionGit pending target-state reconciliation", () => { + it.each([ + { operation: "stage" as const, initialStaged: false, completedStaged: true }, + { operation: "unstage" as const, initialStaged: true, completedStaged: false }, + ])( + "keeps $operation pending through stale repository refreshes", + async ({ operation, initialStaged, completedStaged }) => { + const path = `${operation}-pending.txt`; + const pending = deferredResult(); + mocks.gitOps[operation].mockReturnValueOnce(pending.promise); + mocks.statuses = [{ repository_name: "", status: status(path, initialStaged) }]; + + const hook = renderHook(() => useSessionGit("session-1")); + + act(() => { + void hook.result.current[`${operation}File`]([path], ""); + }); + await waitFor(() => { + expect(hook.result.current.pendingStageFiles).toContain(pendingKey("", path)); + }); + + mocks.statuses = [{ repository_name: "", status: status(path, initialStaged) }]; + hook.rerender(); + + await waitFor(() => { + expect(hook.result.current.pendingStageFiles).toContain(pendingKey("", path)); + }); + + mocks.statuses = [{ repository_name: "", status: status(path, completedStaged) }]; + hook.rerender(); + + await waitFor(() => { + expect(hook.result.current.pendingStageFiles).toContain(pendingKey("", path)); + }); + + await act(async () => { + pending.resolve({ success: true, operation, output: "" }); + await pending.promise; + }); + + await waitFor(() => { + expect(hook.result.current.pendingStageFiles).not.toContain(pendingKey("", path)); + }); + }, + ); + + it("clears an unstage after the refreshed status removes a clean file", async () => { + const path = "clean-after-unstage.txt"; + const pending = deferredResult(); + mocks.gitOps.unstage.mockReturnValueOnce(pending.promise); + mocks.statuses = [{ repository_name: "", status: status(path, true) }]; + + const hook = renderHook(() => useSessionGit("session-1")); + + act(() => { + void hook.result.current.unstageFile([path], ""); + }); + await waitFor(() => { + expect(hook.result.current.pendingStageFiles).toContain(pendingKey("", path)); + }); + + mocks.statuses = []; + hook.rerender(); + await waitFor(() => { + expect(hook.result.current.pendingStageFiles).toContain(pendingKey("", path)); + }); + + await act(async () => { + pending.resolve({ success: true, operation: "unstage", output: "" }); + await pending.promise; + }); + + await waitFor(() => { + expect(hook.result.current.pendingStageFiles).not.toContain(pendingKey("", path)); + }); + }); +}); + +describe("useSessionGit failed request cleanup", () => { + it("clears pending state when the operation returns a failure without a status change", async () => { + const path = "failed-stage.txt"; + mocks.gitOps.stage.mockResolvedValueOnce({ + success: false, + operation: "stage", + output: "", + error: "stage failed", + }); + mocks.statuses = [{ repository_name: "", status: status(path, false) }]; + + const hook = renderHook(() => useSessionGit("session-1")); + + await act(async () => { + await hook.result.current.stageFile([path], ""); + }); + + expect(hook.result.current.pendingStageFiles).not.toContain(pendingKey("", path)); + }); +}); + +describe("useSessionGit partial repository failures", () => { + it.each([ + { operation: "stage" as const, initialStaged: false, completedStaged: true }, + { operation: "unstage" as const, initialStaged: true, completedStaged: false }, + ])( + "keeps successful repository scopes pending after a partial $operation failure", + async ({ operation, initialStaged, completedStaged }) => { + const successfulPath = `successful-${operation}.txt`; + const failedPath = `failed-${operation}.txt`; + mocks.statuses = [ + { repository_name: "repo-a", status: status(successfulPath, initialStaged) }, + { repository_name: "repo-b", status: status(failedPath, initialStaged) }, + ]; + mocks.gitOps[operation].mockImplementation( + async (_paths: string[] | undefined, repositoryName: string | undefined) => ({ + success: repositoryName === "repo-a", + operation, + output: "", + error: repositoryName === "repo-a" ? undefined : `${operation} failed`, + }), + ); + + const hook = renderHook(() => useSessionGit("session-1")); + + await act(async () => { + await hook.result.current[`${operation}File`]([successfulPath, failedPath]); + }); + + expect(hook.result.current.pendingStageFiles).toEqual( + new Set([pendingKey("repo-a", successfulPath)]), + ); + + mocks.statuses = [ + { repository_name: "repo-a", status: status(successfulPath, completedStaged) }, + { repository_name: "repo-b", status: status(failedPath, initialStaged) }, + ]; + hook.rerender(); + await waitFor(() => { + expect(hook.result.current.pendingStageFiles).not.toContain( + pendingKey("repo-a", successfulPath), + ); + }); + }, + ); +}); + +describe("useSessionGit bulk response ownership", () => { + it.each([ + { operation: "stage" as const, initialStaged: false, completedStaged: true }, + { operation: "unstage" as const, initialStaged: true, completedStaged: false }, + ])( + "does not clear a sibling $operation key when one repository reaches target state before the aggregate response", + async ({ operation, initialStaged, completedStaged }) => { + const firstPath = `first-${operation}.txt`; + const secondPath = `second-${operation}.txt`; + const first = deferredResult(); + const second = deferredResult(); + mocks.statuses = [ + { repository_name: "repo-a", status: status(firstPath, initialStaged) }, + { repository_name: "repo-b", status: status(secondPath, initialStaged) }, + ]; + mocks.gitOps[operation].mockImplementation((_paths, repositoryName) => + repositoryName === "repo-a" ? first.promise : second.promise, + ); + + const hook = renderHook(() => useSessionGit("session-1")); + + let aggregate!: Promise; + act(() => { + aggregate = hook.result.current[`${operation}File`]([firstPath, secondPath]); + }); + await waitFor(() => { + expect(hook.result.current.pendingStageFiles).toEqual( + new Set([pendingKey("repo-a", firstPath), pendingKey("repo-b", secondPath)]), + ); + }); + + mocks.statuses = [ + { repository_name: "repo-a", status: status(firstPath, completedStaged) }, + { repository_name: "repo-b", status: status(secondPath, initialStaged) }, + ]; + hook.rerender(); + + await waitFor(() => { + expect(hook.result.current.pendingStageFiles).toContain(pendingKey("repo-a", firstPath)); + }); + + first.resolve({ success: true, operation, output: "" }); + await waitFor(() => expect(mocks.gitOps[operation]).toHaveBeenCalledTimes(2)); + second.resolve({ success: true, operation, output: "" }); + await aggregate; + + await waitFor(() => { + expect(hook.result.current.pendingStageFiles).toEqual( + new Set([pendingKey("repo-b", secondPath)]), + ); + }); + + mocks.statuses = [ + { repository_name: "repo-a", status: status(firstPath, completedStaged) }, + { repository_name: "repo-b", status: status(secondPath, completedStaged) }, + ]; + hook.rerender(); + + await waitFor(() => { + expect(hook.result.current.pendingStageFiles).toEqual(new Set()); + }); + }, + ); +}); + +describe("useSessionGit overlapping request ownership", () => { + it.each([ + { operation: "stage" as const, initialStaged: false, completedStaged: true }, + { operation: "unstage" as const, initialStaged: true, completedStaged: false }, + ])( + "keeps a newer same-direction $operation request pending when the older request fails", + async ({ operation, initialStaged, completedStaged }) => { + const path = `overlapping-${operation}.txt`; + const older = deferredResult(); + const newer = deferredResult(); + mocks.statuses = [{ repository_name: "", status: status(path, initialStaged) }]; + mocks.gitOps[operation].mockReturnValueOnce(older.promise).mockReturnValueOnce(newer.promise); + + const hook = renderHook(() => useSessionGit("session-1")); + + act(() => { + void hook.result.current[`${operation}File`]([path], ""); + }); + await waitFor(() => { + expect(hook.result.current.pendingStageFiles).toContain(pendingKey("", path)); + }); + + act(() => { + void hook.result.current[`${operation}File`]([path], ""); + }); + await act(async () => { + older.resolve({ + success: false, + operation, + output: "", + error: `older ${operation} failed`, + }); + await older.promise; + }); + + expect(hook.result.current.pendingStageFiles).toContain(pendingKey("", path)); + + newer.resolve({ success: true, operation, output: "" }); + mocks.statuses = [{ repository_name: "", status: status(path, completedStaged) }]; + hook.rerender(); + await waitFor(() => { + expect(hook.result.current.pendingStageFiles).not.toContain(pendingKey("", path)); + }); + }, + ); + + it.each([ + { operation: "stage" as const, initialStaged: false, completedStaged: true }, + { operation: "unstage" as const, initialStaged: true, completedStaged: false }, + ])( + "does not clear a newer same-direction $operation request from an older success status", + async ({ operation, initialStaged, completedStaged }) => { + const path = `overlapping-success-${operation}.txt`; + const older = deferredResult(); + const newer = deferredResult(); + mocks.statuses = [{ repository_name: "", status: status(path, initialStaged) }]; + mocks.gitOps[operation].mockReturnValueOnce(older.promise).mockReturnValueOnce(newer.promise); + + const hook = renderHook(() => useSessionGit("session-1")); + + act(() => { + void hook.result.current[`${operation}File`]([path], ""); + }); + await waitFor(() => { + expect(hook.result.current.pendingStageFiles).toContain(pendingKey("", path)); + }); + + act(() => { + void hook.result.current[`${operation}File`]([path], ""); + }); + + await act(async () => { + older.resolve({ success: true, operation, output: "" }); + await older.promise; + }); + + mocks.statuses = [{ repository_name: "", status: status(path, completedStaged) }]; + hook.rerender(); + + await waitFor(() => { + expect(hook.result.current.pendingStageFiles).toContain(pendingKey("", path)); + }); + + await act(async () => { + newer.resolve({ success: true, operation, output: "" }); + await newer.promise; + }); + + await waitFor(() => { + expect(hook.result.current.pendingStageFiles).not.toContain(pendingKey("", path)); + }); + }, + ); +}); + +describe("pending file state indexing", () => { + it("tracks staged and unstaged facets for each repository and path", () => { + const index = buildPendingFileStateIndex([ + { + path: "shared.ts", + repository_name: "repo-a", + status: "modified", + staged: true, + staged_change: { status: "modified" }, + unstaged_change: { status: "modified" }, + }, + { + path: "only-staged.ts", + repository_name: "repo-b", + status: "modified", + staged: true, + }, + ]); + + expect(index.get(pendingKey("repo-a", "shared.ts"))).toEqual({ + hasStaged: true, + hasUnstaged: true, + }); + expect(index.get(pendingKey("repo-b", "only-staged.ts"))).toEqual({ + hasStaged: true, + hasUnstaged: false, + }); + }); +}); + +describe("useSessionGit pending scope ownership", () => { + it("resets pending ownership when the active session changes", async () => { + const path = "shared-worktree-path.txt"; + const older = deferredResult(); + const newer = deferredResult(); + mocks.statuses = [{ repository_name: "", status: status(path, false) }]; + mocks.gitOps.stage.mockReturnValueOnce(older.promise).mockReturnValueOnce(newer.promise); + + const hook = renderHook(({ sessionId }) => useSessionGit(sessionId), { + initialProps: { sessionId: "session-1" }, + }); + + act(() => { + void hook.result.current.stageFile([path], ""); + }); + await waitFor(() => { + expect(hook.result.current.pendingStageFiles).toContain(pendingKey("", path)); + }); + + hook.rerender({ sessionId: "session-2" }); + await waitFor(() => { + expect(hook.result.current.pendingStageFiles).not.toContain(pendingKey("", path)); + }); + + act(() => { + void hook.result.current.stageFile([path], ""); + }); + await waitFor(() => { + expect(hook.result.current.pendingStageFiles).toContain(pendingKey("", path)); + }); + + await act(async () => { + older.resolve({ + success: false, + operation: "stage", + output: "", + error: "previous session failed", + }); + await older.promise; + }); + + expect(hook.result.current.pendingStageFiles).toContain(pendingKey("", path)); + + newer.resolve({ success: true, operation: "stage", output: "" }); + }); + + it("resets pending ownership when the checked-out branch changes", async () => { + const path = "branch-owned-stage.txt"; + const pending = deferredResult(); + mocks.statuses = [{ repository_name: "", status: status(path, false, "feature/old") }]; + mocks.gitOps.stage.mockReturnValueOnce(pending.promise); + + const hook = renderHook(() => useSessionGit("session-1")); + + act(() => { + void hook.result.current.stageFile([path], ""); + }); + await waitFor(() => { + expect(hook.result.current.pendingStageFiles).toContain(pendingKey("", path)); + }); + + mocks.checkoutGenerations = { "": 1 }; + mocks.statuses = [{ repository_name: "", status: status(path, false, "feature/new") }]; + hook.rerender(); + + await waitFor(() => { + expect(hook.result.current.pendingStageFiles).not.toContain(pendingKey("", path)); + }); + + pending.resolve({ success: true, operation: "stage", output: "" }); + }); + + it("resets only the repository affected by a checkout generation", async () => { + const repoAPath = "repo-a-stage.txt"; + const repoBPath = "repo-b-stage.txt"; + const repoAPending = deferredResult(); + const repoBPending = deferredResult(); + mocks.statuses = [ + { repository_name: "repo-a", status: status(repoAPath, false) }, + { repository_name: "repo-b", status: status(repoBPath, false) }, + ]; + mocks.gitOps.stage + .mockReturnValueOnce(repoAPending.promise) + .mockReturnValueOnce(repoBPending.promise); + + const hook = renderHook(() => useSessionGit("session-1")); + act(() => { + void hook.result.current.stageFile([repoAPath], "repo-a"); + void hook.result.current.stageFile([repoBPath], "repo-b"); + }); + await waitFor(() => { + expect(hook.result.current.pendingStageFiles).toEqual( + new Set([pendingKey("repo-a", repoAPath), pendingKey("repo-b", repoBPath)]), + ); + }); + + mocks.checkoutGenerations = { "repo-a": 1 }; + mocks.statuses = [ + { repository_name: "repo-a", status: status(repoAPath, false) }, + { repository_name: "repo-b", status: status(repoBPath, false) }, + ]; + hook.rerender(); + + expect(hook.result.current.pendingStageFiles).toEqual( + new Set([pendingKey("repo-b", repoBPath)]), + ); + + repoAPending.resolve({ success: true, operation: "stage", output: "" }); + repoBPending.resolve({ success: true, operation: "stage", output: "" }); + }); +}); diff --git a/apps/web/hooks/domains/session/use-session-git-pending.ts b/apps/web/hooks/domains/session/use-session-git-pending.ts new file mode 100644 index 0000000000..12a7ef6df2 --- /dev/null +++ b/apps/web/hooks/domains/session/use-session-git-pending.ts @@ -0,0 +1,226 @@ +import { + useEffect, + useRef, + type Dispatch, + type MutableRefObject, + type SetStateAction, +} from "react"; +import type { FileInfo } from "@/lib/state/slices/session-runtime/types"; +import type { useSessionGitStatusByRepo } from "./use-session-git-status"; +import { splitFilesByChangeLayer } from "./git-change-facets"; + +export type PendingFileOperation = "stage" | "unstage"; + +export type PendingFileOperationOwner = { + operation: PendingFileOperation; + requestId: number; + scopeIdentity: string; + responseSucceeded: boolean; + targetStateObservedKeys: Set; +}; + +export function pendingKey(repo: string | undefined, path: string): string { + return `${repo ?? ""}::${path}`; +} + +export function pendingKeysForFailedRepositories( + buckets: Map, + perRepoResults: Array<{ repository_name: string; success: boolean }> | undefined, +): string[] { + const allKeys = Array.from(buckets).flatMap(([repositoryName, paths]) => + paths.map((path) => pendingKey(repositoryName, path)), + ); + if (!perRepoResults) return allKeys; + + const failedRepositories = new Set( + perRepoResults.filter((entry) => !entry.success).map((entry) => entry.repository_name), + ); + if (failedRepositories.size === 0) return allKeys; + + return Array.from(buckets).flatMap(([repositoryName, paths]) => + failedRepositories.has(repositoryName) + ? paths.map((path) => pendingKey(repositoryName, path)) + : [], + ); +} + +export function clearPendingFileOperations( + keys: string[], + owner: PendingFileOperationOwner, + pendingFileOperations: MutableRefObject>, + setPendingStageFiles: Dispatch>>, +) { + const cleared = keys.filter((key) => pendingFileOperations.current.get(key) === owner); + if (cleared.length === 0) return; + for (const key of cleared) pendingFileOperations.current.delete(key); + setPendingStageFiles((prev) => { + const next = new Set(prev); + for (const key of cleared) next.delete(key); + return next; + }); +} + +/** Marks the current owner's response as successful and clears keys already observed at target state. */ +export function markPendingFileOperationsSucceeded( + keys: string[], + owner: PendingFileOperationOwner, + pendingFileOperations: MutableRefObject>, + setPendingStageFiles: Dispatch>>, +) { + for (const key of keys) { + const current = pendingFileOperations.current.get(key); + if (current === owner) current.responseSucceeded = true; + } + clearPendingFileOperations( + keys.filter( + (key) => + pendingFileOperations.current.get(key) === owner && owner.targetStateObservedKeys.has(key), + ), + owner, + pendingFileOperations, + setPendingStageFiles, + ); +} + +export function usePendingFileOperationScope( + scopeIdentity: string, + pendingFileOperations: MutableRefObject>, + setPendingStageFiles: Dispatch>>, +): boolean { + const activeScopeIdentity = useRef(scopeIdentity); + const scopeMatches = activeScopeIdentity.current === scopeIdentity; + + useEffect(() => { + if (activeScopeIdentity.current === scopeIdentity) return; + activeScopeIdentity.current = scopeIdentity; + pendingFileOperations.current.clear(); + setPendingStageFiles(new Set()); + }, [pendingFileOperations, scopeIdentity, setPendingStageFiles]); + + return scopeMatches; +} + +/** Clears pending operations only for repositories whose checkout generation changed. */ +export function usePendingFileOperationRepositoryScope( + generations: Record, + pendingFileOperations: MutableRefObject>, + setPendingStageFiles: Dispatch>>, +) { + const previousGenerations = useRef(generations); + + useEffect(() => { + const changedScopes = new Set(); + const scopes = new Set([ + ...Object.keys(previousGenerations.current), + ...Object.keys(generations), + ]); + for (const scope of scopes) { + if (previousGenerations.current[scope] !== generations[scope]) changedScopes.add(scope); + } + previousGenerations.current = generations; + if (changedScopes.size === 0) return; + + setPendingStageFiles((prev) => { + if (prev.size === 0) return prev; + const next = new Set(prev); + for (const key of prev) { + const separator = key.indexOf("::"); + const repositoryName = separator === -1 ? "" : key.slice(0, separator); + if (!changedScopes.has("") && !changedScopes.has(repositoryName)) continue; + next.delete(key); + pendingFileOperations.current.delete(key); + } + return next; + }); + }, [generations, pendingFileOperations, setPendingStageFiles]); +} + +type PendingFileState = { + hasStaged: boolean; + hasUnstaged: boolean; +}; + +/** Indexes the current staged/unstaged facets once so each pending key is O(1) to reconcile. */ +export function buildPendingFileStateIndex(allFiles: FileInfo[]): Map { + const index = new Map(); + const { stagedFiles, unstagedFiles } = splitFilesByChangeLayer(allFiles); + const mark = (files: FileInfo[], layer: keyof PendingFileState) => { + for (const file of files) { + const key = pendingKey(file.repository_name, file.path); + const current = index.get(key) ?? { hasStaged: false, hasUnstaged: false }; + current[layer] = true; + index.set(key, current); + } + }; + mark(stagedFiles, "hasStaged"); + mark(unstagedFiles, "hasUnstaged"); + return index; +} + +function pendingFileOperationCompleted( + key: string, + operation: PendingFileOperation, + fileStateIndex: Map, +): boolean { + const state = fileStateIndex.get(key); + // Unstaging a staged-only file can make it clean, so the refreshed status + // legitimately omits it entirely. + if (!state) return operation === "unstage"; + return operation === "stage" + ? state.hasStaged && !state.hasUnstaged + : state.hasUnstaged && !state.hasStaged; +} + +/** + * Reconciles pending markers only when a refreshed repository status reaches + * the requested staged or unstaged state. Repository polling can publish the + * previous state after the next action starts, so refresh identity alone is + * not a completion signal. + */ +export function usePerRepoPendingClear( + statusByRepo: ReturnType, + allFiles: FileInfo[], + setPendingStageFiles: Dispatch>>, + pendingFileOperations: MutableRefObject>, +) { + const prevStatusRef = useRef>(new Map()); + useEffect(() => { + const next = new Map(); + const refreshed: string[] = []; + for (const { repository_name, status } of statusByRepo) { + next.set(repository_name, status); + if (prevStatusRef.current.get(repository_name) !== status) { + refreshed.push(repository_name); + } + } + const isLegacySingleRepo = statusByRepo.length === 0; + prevStatusRef.current = next; + if (refreshed.length === 0 && !isLegacySingleRepo) return; + const fileStateIndex = buildPendingFileStateIndex(allFiles); + setPendingStageFiles((prev) => { + if (prev.size === 0) return prev; + const out = new Set(); + for (const key of prev) { + const separator = key.indexOf("::"); + const repositoryName = separator === -1 ? "" : key.slice(0, separator); + const owner = pendingFileOperations.current.get(key); + const repositoryRefreshed = isLegacySingleRepo || refreshed.includes(repositoryName); + if ( + !repositoryRefreshed || + !owner || + !pendingFileOperationCompleted(key, owner.operation, fileStateIndex) + ) { + out.add(key); + continue; + } + owner.targetStateObservedKeys.add(key); + if (owner.responseSucceeded) { + pendingFileOperations.current.delete(key); + continue; + } + out.add(key); + } + return out; + }); + }, [allFiles, statusByRepo, setPendingStageFiles, pendingFileOperations]); +} diff --git a/apps/web/hooks/domains/session/use-session-git-status.test.tsx b/apps/web/hooks/domains/session/use-session-git-status.test.tsx new file mode 100644 index 0000000000..52f5d288f7 --- /dev/null +++ b/apps/web/hooks/domains/session/use-session-git-status.test.tsx @@ -0,0 +1,41 @@ +import { cleanup, renderHook } from "@testing-library/react"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { useSessionGitPendingScope } from "./use-session-git-status"; + +const mocks = vi.hoisted(() => ({ + state: { + environmentIdBySessionId: {} as Record, + sessionCommits: { refetchTrigger: {} as Record }, + }, +})); + +vi.mock("@/components/state-provider", () => ({ + useAppStore: (selector: (state: typeof mocks.state) => unknown) => selector(mocks.state), +})); + +describe("useSessionGitPendingScope", () => { + afterEach(cleanup); + + // Reviewer-required contract coverage for the pending ownership boundary. + it("changes across session and environment scopes but ignores commit refetches", () => { + mocks.state.environmentIdBySessionId = { "session-1": "environment-1" }; + mocks.state.sessionCommits.refetchTrigger = { "environment-1": 0 }; + + const hook = renderHook(({ sessionId }) => useSessionGitPendingScope(sessionId), { + initialProps: { sessionId: "session-1" as string | null }, + }); + const initial = hook.result.current; + + hook.rerender({ sessionId: "session-2" }); + expect(hook.result.current).not.toBe(initial); + + mocks.state.environmentIdBySessionId["session-2"] = "environment-2"; + hook.rerender({ sessionId: "session-2" }); + const environmentScoped = hook.result.current; + expect(environmentScoped).not.toBe(initial); + + mocks.state.sessionCommits.refetchTrigger["environment-2"] = 1; + hook.rerender({ sessionId: "session-2" }); + expect(hook.result.current).toBe(environmentScoped); + }); +}); diff --git a/apps/web/hooks/domains/session/use-session-git-status.ts b/apps/web/hooks/domains/session/use-session-git-status.ts index bcc2ee1dbe..1c958d8369 100644 --- a/apps/web/hooks/domains/session/use-session-git-status.ts +++ b/apps/web/hooks/domains/session/use-session-git-status.ts @@ -81,3 +81,29 @@ export function useSessionGitStatusByRepo( .sort((a, b) => a.repository_name.localeCompare(b.repository_name)); }, [map]); } + +/** Identifies the active pending-operation scope across session/environment replacement. */ +export function useSessionGitPendingScope(sessionId: string | null): string { + return useAppStore((state) => { + if (!sessionId) return ""; + const envKey = state.environmentIdBySessionId[sessionId] ?? sessionId; + return `${sessionId}\u0000${envKey}`; + }); +} + +/** + * Checkout/reset generations are repository-scoped. Commit refetches and + * comparison-base refreshes intentionally do not participate here because + * they do not replace the checked-out worktree. + */ +export function useSessionGitPendingCheckoutGenerations( + sessionId: string | null, +): Record { + return useAppStore( + useShallow((state) => { + if (!sessionId) return {}; + const envKey = state.environmentIdBySessionId[sessionId] ?? sessionId; + return state.gitCheckoutGeneration.byEnvironmentId[envKey] ?? {}; + }), + ); +} diff --git a/apps/web/hooks/domains/session/use-session-git.ts b/apps/web/hooks/domains/session/use-session-git.ts index 4ad67f8260..c99883ec4e 100644 --- a/apps/web/hooks/domains/session/use-session-git.ts +++ b/apps/web/hooks/domains/session/use-session-git.ts @@ -1,7 +1,14 @@ "use client"; -import { useState, useCallback, useEffect, useMemo } from "react"; -import { useSessionGitStatus, useSessionGitStatusByRepo } from "./use-session-git-status"; +/* eslint-disable max-lines */ + +import { useState, useCallback, useEffect, useMemo, useRef, type MutableRefObject } from "react"; +import { + useSessionGitPendingCheckoutGenerations, + useSessionGitPendingScope, + useSessionGitStatus, + useSessionGitStatusByRepo, +} from "./use-session-git-status"; import { useSessionCommits } from "./use-session-commits"; import { useCumulativeDiff } from "./use-cumulative-diff"; import { useGitOperations } from "@/hooks/use-git-operations"; @@ -26,6 +33,18 @@ import { deriveComparisonValues, deriveSessionGitValues } from "./use-session-gi import { useScopedStageOperations } from "./use-scoped-stage-operations"; import { normalizeGitStatusFiles } from "@/lib/state/slices/session-runtime/git-status-normalizer"; import { splitFilesByChangeLayer } from "./git-change-facets"; +import { + clearPendingFileOperations, + markPendingFileOperationsSucceeded, + pendingKey, + pendingKeysForFailedRepositories, + usePendingFileOperationRepositoryScope, + usePendingFileOperationScope, + usePerRepoPendingClear, + type PendingFileOperationOwner, +} from "./use-session-git-pending"; + +export { pendingKey } from "./use-session-git-pending"; /** * Per-repo result emitted by frontend-side fan-outs (commit, push, pull, @@ -248,19 +267,10 @@ type StageDispatchArgs = { reposInFiles: string[]; stagedFiles: FileInfo[]; setPendingStageFiles: React.Dispatch>>; + pendingFileOperations: MutableRefObject>; + pendingScopeIdentity: string; }; -/** - * Encodes a (repo, path) pair as a single Set entry. We need a per-repo key - * because two repos can have files at the same relative path (README.md, - * .gitignore, etc.) and a flat path-only Set would conflate them. The - * separator "::" keeps the key trivially parseable for debugging — repo - * names don't contain "::". - */ -export function pendingKey(repo: string | undefined, path: string): string { - return `${repo ?? ""}::${path}`; -} - /** * Aggregates a list of per-repo results into a single GitOperationResult. * - `success` is true only if every repo succeeded @@ -357,7 +367,10 @@ function useStageDispatch({ reposInFiles, stagedFiles, setPendingStageFiles, + pendingFileOperations, + pendingScopeIdentity, }: StageDispatchArgs) { + const nextPendingRequestId = useRef(0); const groupPathsByRepo = useCallback( (paths: string[]): Map => groupPathsByRepoName(paths, repoForPath), [repoForPath], @@ -433,31 +446,75 @@ function useStageDispatch({ paths: string[], repo: string | undefined, op: (rp: string[], r: string | undefined) => Promise, - operation: string, + operation: PendingFileOperationOwner["operation"], ) => { - // Key pending entries by `repo::path` so concurrent operations in - // different repositories remain scoped. The consumer `FileRow` checks - // membership via the same encoding. + // Track the requested transition with each repo/path key so an unrelated + // or stale status refresh cannot clear a newer pending action. const buckets = repo !== undefined ? new Map([[repo, paths]]) : groupPathsByRepo(paths); const keys: string[] = []; for (const [r, rp] of buckets) for (const p of rp) keys.push(pendingKey(r, p)); + const owner: PendingFileOperationOwner = { + operation, + requestId: ++nextPendingRequestId.current, + scopeIdentity: pendingScopeIdentity, + responseSucceeded: false, + targetStateObservedKeys: new Set(), + }; + for (const key of keys) pendingFileOperations.current.set(key, owner); setPendingStageFiles((prev) => { const next = new Set(prev); for (const k of keys) next.add(k); return next; }); try { - return await runPerRepo(paths, repo, operation, op); - } finally { - setPendingStageFiles((prev) => { - if (prev.size === 0) return prev; - const next = new Set(prev); - for (const k of keys) next.delete(k); - return next; - }); + const result = await runPerRepo(paths, repo, operation, op); + if (result.success) { + markPendingFileOperationsSucceeded( + keys, + owner, + pendingFileOperations, + setPendingStageFiles, + ); + } else if (result.per_repo) { + const successfulKeys = Array.from(buckets).flatMap( + ([repositoryName, repositoryPaths]) => { + const repositoryResult = result.per_repo?.find( + (entry) => entry.repository_name === repositoryName, + ); + return repositoryResult?.success + ? repositoryPaths.map((path) => pendingKey(repositoryName, path)) + : []; + }, + ); + markPendingFileOperationsSucceeded( + successfulKeys, + owner, + pendingFileOperations, + setPendingStageFiles, + ); + const failedKeys = pendingKeysForFailedRepositories(buckets, result.per_repo); + clearPendingFileOperations( + failedKeys, + owner, + pendingFileOperations, + setPendingStageFiles, + ); + } else { + clearPendingFileOperations(keys, owner, pendingFileOperations, setPendingStageFiles); + } + return result; + } catch (err) { + clearPendingFileOperations(keys, owner, pendingFileOperations, setPendingStageFiles); + throw err; } }, - [runPerRepo, setPendingStageFiles, groupPathsByRepo], + [ + runPerRepo, + setPendingStageFiles, + groupPathsByRepo, + pendingFileOperations, + pendingScopeIdentity, + ], ); const stageFile = useCallback( async (paths: string[], repo?: string) => @@ -624,10 +681,13 @@ export function useSessionGit(sessionId: string | null | undefined): SessionGit const sid = sessionId ?? null; const gitStatus = useSessionGitStatus(sid); const statusByRepo = useSessionGitStatusByRepo(sid); + const pendingScopeIdentity = useSessionGitPendingScope(sid); + const pendingCheckoutGenerations = useSessionGitPendingCheckoutGenerations(sid); const { commits, loading: commitsLoading } = useSessionCommits(sid); const { diff: cumulativeDiff } = useCumulativeDiff(sid); const gitOps = useGitOperations(sid); const [pendingStageFiles, setPendingStageFiles] = useState>(new Set()); + const pendingFileOperations = useRef>(new Map()); const { allFiles, unstagedFiles, @@ -637,12 +697,26 @@ export function useSessionGit(sessionId: string | null | undefined): SessionGit repoNamesForControls, perRepoStatus, } = useFileDerivations(statusByRepo, gitStatus); + const pendingScopeMatches = usePendingFileOperationScope( + pendingScopeIdentity, + pendingFileOperations, + setPendingStageFiles, + ); + usePendingFileOperationRepositoryScope( + pendingCheckoutGenerations, + pendingFileOperations, + setPendingStageFiles, + ); + usePerRepoPendingClear(statusByRepo, allFiles, setPendingStageFiles, pendingFileOperations); + const stageOps = useStageDispatch({ gitOps, repoForPath, reposInFiles, stagedFiles, setPendingStageFiles, + pendingFileOperations, + pendingScopeIdentity, }); const { stageAll, unstageAll, commit, stageFile, unstageFile, discard } = stageOps; const derived = deriveSessionGitValues( @@ -682,7 +756,7 @@ export function useSessionGit(sessionId: string | null | undefined): SessionGit isLoading: gitOps.isLoading, loadingOperation: gitOps.loadingOperation, - pendingStageFiles, + pendingStageFiles: pendingScopeMatches ? pendingStageFiles : new Set(), pull: remoteOps.pull, push: remoteOps.push, diff --git a/apps/web/lib/state/app-state-types.ts b/apps/web/lib/state/app-state-types.ts index 52767261b6..552a1cdb08 100644 --- a/apps/web/lib/state/app-state-types.ts +++ b/apps/web/lib/state/app-state-types.ts @@ -139,6 +139,7 @@ export type AppState = KanbanSlice & { gitStatus: (typeof defaultSessionRuntimeState)["gitStatus"]; environmentIdBySessionId: (typeof defaultSessionRuntimeState)["environmentIdBySessionId"]; sessionCommits: (typeof defaultSessionRuntimeState)["sessionCommits"]; + gitCheckoutGeneration: (typeof defaultSessionRuntimeState)["gitCheckoutGeneration"]; contextWindow: (typeof defaultSessionRuntimeState)["contextWindow"]; agents: (typeof defaultSessionRuntimeState)["agents"]; availableCommands: (typeof defaultSessionRuntimeState)["availableCommands"]; @@ -490,6 +491,7 @@ export type AppState = KanbanSlice & { addSessionCommit: (sessionId: string, commit: SessionCommit) => void; clearSessionCommits: (sessionId: string) => void; bumpSessionCommitsRefetch: (sessionId: string) => void; + bumpSessionGitCheckoutGeneration: (sessionId: string, repositoryName?: string) => void; setContextWindow: (sessionId: string, contextWindow: ContextWindowEntry) => void; clearContextWindow: (sessionId: string) => void; bumpAgentProfilesVersion: () => void; diff --git a/apps/web/lib/state/default-state.ts b/apps/web/lib/state/default-state.ts index 1567fbd4b5..4d9b9a3ecd 100644 --- a/apps/web/lib/state/default-state.ts +++ b/apps/web/lib/state/default-state.ts @@ -72,6 +72,7 @@ export const defaultState = { gitStatus: defaultSessionRuntimeState.gitStatus, environmentIdBySessionId: defaultSessionRuntimeState.environmentIdBySessionId, sessionCommits: defaultSessionRuntimeState.sessionCommits, + gitCheckoutGeneration: defaultSessionRuntimeState.gitCheckoutGeneration, contextWindow: defaultSessionRuntimeState.contextWindow, agents: defaultSessionRuntimeState.agents, availableCommands: defaultSessionRuntimeState.availableCommands, @@ -445,6 +446,10 @@ export function mergeInitialState(initialState?: HydrationState): DefaultState { processes: { ...defaultState.processes, ...initialState.processes }, gitStatus: { ...defaultState.gitStatus, ...initialState.gitStatus }, sessionCommits: { ...defaultState.sessionCommits, ...initialState.sessionCommits }, + gitCheckoutGeneration: { + ...defaultState.gitCheckoutGeneration, + ...initialState.gitCheckoutGeneration, + }, contextWindow: { ...defaultState.contextWindow, ...initialState.contextWindow }, agents: { ...defaultState.agents, ...initialState.agents }, sessionMode: { ...defaultState.sessionMode, ...initialState.sessionMode }, diff --git a/apps/web/lib/state/slices/session-runtime/session-runtime-slice.ts b/apps/web/lib/state/slices/session-runtime/session-runtime-slice.ts index 8d3faea693..e94ebc307a 100644 --- a/apps/web/lib/state/slices/session-runtime/session-runtime-slice.ts +++ b/apps/web/lib/state/slices/session-runtime/session-runtime-slice.ts @@ -72,6 +72,7 @@ function purgeEnvScopedRuntime(state: SessionRuntimeSliceState, envKey: string) delete state.sessionCommits.byEnvironmentId[envKey]; delete state.sessionCommits.loading[envKey]; delete state.sessionCommits.refetchTrigger[envKey]; + delete state.gitCheckoutGeneration.byEnvironmentId[envKey]; delete state.userShells.byEnvironmentId[envKey]; delete state.userShells.dismissedByEnvironmentId[envKey]; delete state.userShells.loading[envKey]; @@ -101,6 +102,7 @@ export const defaultSessionRuntimeState: SessionRuntimeSliceState = { gitStatus: { byEnvironmentId: {}, byEnvironmentRepo: {} }, environmentIdBySessionId: {}, sessionCommits: { byEnvironmentId: {}, loading: {}, refetchTrigger: {} }, + gitCheckoutGeneration: { byEnvironmentId: {} }, contextWindow: { bySessionId: {} }, agents: { agents: [] }, availableCommands: { bySessionId: {} }, @@ -236,6 +238,13 @@ function buildSessionCommitActions(set: ImmerSet) { const prev = draft.sessionCommits.refetchTrigger[envKey] ?? 0; draft.sessionCommits.refetchTrigger[envKey] = prev + 1; }), + bumpSessionGitCheckoutGeneration: (sessionId: string, repositoryName?: string) => + set((draft) => { + const envKey = draft.environmentIdBySessionId[sessionId] ?? sessionId; + const byRepository = (draft.gitCheckoutGeneration.byEnvironmentId[envKey] ??= {}); + const scope = repositoryName ?? ""; + byRepository[scope] = (byRepository[scope] ?? 0) + 1; + }), }; } @@ -325,6 +334,7 @@ export function migrateEnvKeyedData( migrate(draft.gitStatus.byEnvironmentRepo); migrate(draft.sessionCommits.loading); migrate(draft.sessionCommits.refetchTrigger); + migrate(draft.gitCheckoutGeneration.byEnvironmentId); migrate(draft.gitStatus.byEnvironmentId); migrate(draft.shell.outputs); migrate(draft.shell.statuses); diff --git a/apps/web/lib/state/slices/session-runtime/types.ts b/apps/web/lib/state/slices/session-runtime/types.ts index 63ff6e8fe3..aac6d9fd76 100644 --- a/apps/web/lib/state/slices/session-runtime/types.ts +++ b/apps/web/lib/state/slices/session-runtime/types.ts @@ -178,6 +178,11 @@ export type SessionCommitsState = { refetchTrigger: Record; }; +/** Checkout generations keyed by environment and repository scope. */ +export type GitCheckoutGenerationState = { + byEnvironmentId: Record>; +}; + export type ContextWindowEntry = { size: number; used: number; @@ -441,6 +446,7 @@ export type SessionRuntimeSliceState = { /** Maps sessionId → environmentId for workspace state sharing. */ environmentIdBySessionId: Record; sessionCommits: SessionCommitsState; + gitCheckoutGeneration: GitCheckoutGenerationState; contextWindow: ContextWindowState; agents: AgentState; availableCommands: AvailableCommandsState; @@ -493,6 +499,8 @@ export type SessionRuntimeSliceActions = { // Signal a refetch without clearing the visible list — see // SessionCommitsState.refetchTrigger. bumpSessionCommitsRefetch: (sessionId: string) => void; + /** Bump only the affected repository's checkout generation. */ + bumpSessionGitCheckoutGeneration: (sessionId: string, repositoryName?: string) => void; // Available commands actions setAvailableCommands: (sessionId: string, commands: AvailableCommand[]) => void; clearAvailableCommands: (sessionId: string) => void; diff --git a/apps/web/lib/state/store-overrides.ts b/apps/web/lib/state/store-overrides.ts index 0b2f6f9b52..a460afcc30 100644 --- a/apps/web/lib/state/store-overrides.ts +++ b/apps/web/lib/state/store-overrides.ts @@ -43,6 +43,7 @@ export function buildStateOverrides(m: DefaultState) { shell: m.shell, processes: m.processes, gitStatus: m.gitStatus, + gitCheckoutGeneration: m.gitCheckoutGeneration, contextWindow: m.contextWindow, agents: m.agents, sessionMode: m.sessionMode, diff --git a/apps/web/lib/types/git-events.ts b/apps/web/lib/types/git-events.ts index 8228693101..1677159a51 100644 --- a/apps/web/lib/types/git-events.ts +++ b/apps/web/lib/types/git-events.ts @@ -60,6 +60,7 @@ export type GitCommitData = { // Git reset data export type GitResetData = { + repository_name?: string; previous_head: string; current_head: string; deleted_count: number; @@ -67,6 +68,7 @@ export type GitResetData = { // Git branch switch data export type GitBranchSwitchData = { + repository_name?: string; previous_branch: string; current_branch: string; current_head: string; diff --git a/apps/web/lib/ws/handlers/agent-session.test.ts b/apps/web/lib/ws/handlers/agent-session.test.ts index 2b6e385770..5a2354ccc4 100644 --- a/apps/web/lib/ws/handlers/agent-session.test.ts +++ b/apps/web/lib/ws/handlers/agent-session.test.ts @@ -36,6 +36,7 @@ function makeStore(overrides: Record = {}) { queue: { bySessionId: {}, metaBySessionId: {} }, setQueueEntries: vi.fn(), clearLegacyGitStatusEntry: vi.fn(), + bumpSessionGitCheckoutGeneration: vi.fn(), bumpSessionCommitsRefetch: vi.fn(), bumpWorkspaceFilesRefresh: vi.fn(), reconcileWorkspaceSourcesAdopted: vi.fn(), @@ -475,12 +476,14 @@ describe("session.workspace_sources.updated handler", () => { it("adopts the workspace root and bumps the Files refresh key", () => { const setTaskSession = vi.fn(); const bumpWorkspaceFilesRefresh = vi.fn(); + const bumpSessionGitCheckoutGeneration = vi.fn(); const store = makeStore({ taskSessions: { items: { "s-1": { id: "s-1", task_id: "t-1", state: "IDLE", worktree_path: "/old" } }, }, setTaskSession, bumpWorkspaceFilesRefresh, + bumpSessionGitCheckoutGeneration, }); const handler = registerTaskSessionHandlers(store)["session.workspace_sources.updated"]!; @@ -496,6 +499,7 @@ describe("session.workspace_sources.updated handler", () => { expect.objectContaining({ id: "s-1", worktree_path: "/old", workspace_path: "/new" }), ); expect(bumpWorkspaceFilesRefresh).toHaveBeenCalledWith("s-1"); + expect(bumpSessionGitCheckoutGeneration).toHaveBeenCalledWith("s-1"); // The server-issued envelope timestamp is forwarded as the adoption // boundary so the client clock can never retire legitimate turns. expect(store.getState().reconcileWorkspaceSourcesAdopted).toHaveBeenCalledWith( @@ -1240,6 +1244,7 @@ describe("session.state_changed → agentctl ready fallback", () => { it("preserves the primary worktree when a sibling agentctl_ready arrives", () => { const upsertTaskSessionFromEvent = vi.fn(); const setTaskSession = vi.fn(); + const bumpSessionGitCheckoutGeneration = vi.fn(); const store = makeStore({ taskSessions: { items: { @@ -1258,6 +1263,7 @@ describe("session.state_changed → agentctl ready fallback", () => { sessionWorktreesBySessionId: { itemsBySessionId: { "s-1": ["primary-worktree"] } }, setSessionAgentctlStatus: vi.fn(), setTaskSession, + bumpSessionGitCheckoutGeneration, upsertTaskSessionFromEvent, setWorktree: vi.fn(), setSessionWorktrees: vi.fn(), @@ -1300,6 +1306,7 @@ describe("session.state_changed → agentctl ready fallback", () => { workspace_path: TASK_ROOT, }), ); + expect(bumpSessionGitCheckoutGeneration).toHaveBeenCalledWith("s-1"); }); it("does not call upsertTaskSessionFromEvent when agentctl payload omits task_environment_id", () => { diff --git a/apps/web/lib/ws/handlers/agent-session.ts b/apps/web/lib/ws/handlers/agent-session.ts index b2d55adb31..a837097186 100644 --- a/apps/web/lib/ws/handlers/agent-session.ts +++ b/apps/web/lib/ws/handlers/agent-session.ts @@ -535,6 +535,7 @@ function handleAgentctlReady(store: StoreApi, payload: any): void { // surfacing the frozen snapshot until the user reloads the tab, masking // the per-repo updates streaming in for both the primary and the sibling. store.getState().clearLegacyGitStatusEntry(payload.session_id); + store.getState().bumpSessionGitCheckoutGeneration(payload.session_id); store.getState().bumpSessionCommitsRefetch(payload.session_id); } } @@ -615,6 +616,7 @@ function handleWorkspaceSourcesUpdated( .reconcileWorkspaceSourcesAdopted(adoptedSessionIds ?? [sessionId], boundaryTimestamp); store.getState().bumpWorkspaceFilesRefresh(sessionId); store.getState().clearLegacyGitStatusEntry(sessionId); + store.getState().bumpSessionGitCheckoutGeneration(sessionId); store.getState().bumpSessionCommitsRefetch(sessionId); } diff --git a/apps/web/lib/ws/handlers/git-status.test.ts b/apps/web/lib/ws/handlers/git-status.test.ts index 305ebb919c..e3d3589624 100644 --- a/apps/web/lib/ws/handlers/git-status.test.ts +++ b/apps/web/lib/ws/handlers/git-status.test.ts @@ -160,13 +160,19 @@ describe("git-status WS handler — commit events", () => { type: "commits_reset", session_id: SESSION, timestamp: "2026-05-28T00:00:01Z", - reset: { previous_head: "old-head", current_head: "new-head", deleted_count: 1 }, + reset: { + previous_head: "old-head", + current_head: "new-head", + deleted_count: 1, + repository_name: "repo-a", + }, }), ); const state = store.getState(); // Trigger bumped — useSessionCommits will refetch. expect(state.sessionCommits.refetchTrigger[SESSION]).toBe(1); + expect(state.gitCheckoutGeneration.byEnvironmentId[SESSION]?.["repo-a"]).toBe(1); // Existing commits remain — this is the whole point. Clearing would make // the Changes panel briefly render its empty state until the refetch // resolved. @@ -188,12 +194,14 @@ describe("git-status WS handler — commit events", () => { current_branch: "new", current_head: "head", base_commit: "base", + repository_name: "repo-b", }, }), ); const state = store.getState(); expect(state.sessionCommits.refetchTrigger[SESSION]).toBe(1); + expect(state.gitCheckoutGeneration.byEnvironmentId[SESSION]?.["repo-b"]).toBe(1); expect(state.sessionCommits.byEnvironmentId[SESSION]).toHaveLength(1); }); diff --git a/apps/web/lib/ws/handlers/git-status.ts b/apps/web/lib/ws/handlers/git-status.ts index 8a735fcfc9..797fc56645 100644 --- a/apps/web/lib/ws/handlers/git-status.ts +++ b/apps/web/lib/ws/handlers/git-status.ts @@ -125,22 +125,36 @@ const gitEventHandlers: GitEventHandlers = { }, commits_reset: (store, event) => { - if (isDebug()) debug("commits_reset", { sessionId: event.session_id }); + if (isDebug()) + debug("commits_reset", { + sessionId: event.session_id, + repositoryName: event.reset.repository_name ?? null, + }); // Trigger a refetch without clearing the visible commits — the Changes // panel would otherwise flicker through its empty state ("Your changed // files will appear here") while the refetch is in flight, because // useSessionCommits returns `commits ?? []` and the panel's hasAnything // gate flips to false the moment commits goes undefined. store.getState().bumpSessionCommitsRefetch(event.session_id); + store + .getState() + .bumpSessionGitCheckoutGeneration(event.session_id, event.reset.repository_name); // Invalidate cumulative diff cache when commits are reset invalidateCumulativeDiffCache(resolveEnvKey(store, event.session_id)); }, branch_switched: (store, event) => { - if (isDebug()) debug("branch_switched", { sessionId: event.session_id }); + if (isDebug()) + debug("branch_switched", { + sessionId: event.session_id, + repositoryName: event.branch_switch.repository_name ?? null, + }); // Stale-while-revalidate (see commits_reset above): refetch with the new // base commit but keep the old list visible until the new one arrives. store.getState().bumpSessionCommitsRefetch(event.session_id); + store + .getState() + .bumpSessionGitCheckoutGeneration(event.session_id, event.branch_switch.repository_name); // Invalidate cumulative diff cache when branch switches invalidateCumulativeDiffCache(resolveEnvKey(store, event.session_id)); }, diff --git a/docs/specs/ui/requirements/changes-file-action-feedback.md b/docs/specs/ui/requirements/changes-file-action-feedback.md index 5909a19ebe..ccf80abf4d 100644 --- a/docs/specs/ui/requirements/changes-file-action-feedback.md +++ b/docs/specs/ui/requirements/changes-file-action-feedback.md @@ -15,15 +15,18 @@ On fine-pointer layouts, the action shares the file-type icon slot and normally appears only while the row is hovered. Once an action starts, its progress feedback must remain visible until the associated request finishes, even when the pointer leaves the row. The UI system owns this transient presentation -contract; Git operation execution and pending-state lifetime remain owned by -their existing systems. +contract and the per-file pending lifetime that drives it; Git operation +execution and refreshed repository status remain owned by their existing +systems. ## Terminology - **File action slot:** The leading Changes-row position that shows either the file-type icon, the stage or unstage action, or its pending indicator. -- **Pending file action:** A stage or unstage request whose per-file pending - state has started and has not yet cleared. +- **Pending file action:** The latest stage or unstage request within the active + session, environment, and checked-out branch generation that owns the pending + state for one repository and file and has not reached its target state or + failed. ## Requirements @@ -56,12 +59,29 @@ the action is complete. shall retain their always-visible, touch-sized file actions and show the same pending indicator without introducing a hover dependency or reducing the existing touch target. +- **AC-UI-CHANGES-FILE-ACTION-FEEDBACK-001.6:** A successful stage or unstage + request shall remain pending through stale status updates and clear only after + both the successful request response and refreshed status reach the requested + staged or unstaged state. Either signal may arrive first. +- **AC-UI-CHANGES-FILE-ACTION-FEEDBACK-001.7:** When the current stage or + unstage request fails, its pending feedback shall clear. If a newer request in + either direction has superseded it for the same repository and file, the + older request's completion or failure shall not clear the newer request's + pending feedback. +- **AC-UI-CHANGES-FILE-ACTION-FEEDBACK-001.8:** When one request spans multiple + repositories and only some repository scopes fail, failed scopes shall clear + immediately while successful scopes remain pending until their refreshed + status reaches the requested state. +- **AC-UI-CHANGES-FILE-ACTION-FEEDBACK-001.9:** Pending ownership shall reset + when the active session, environment, checked-out branch generation, or + workspace source generation changes. A callback from the prior scope shall + not add, retain, or clear feedback in the successor scope. ## Out of scope -- Changing Git stage or unstage execution, pending-state creation, or - pending-state cleanup. -- Changing bulk stage, bulk unstage, discard, edit, or commit feedback. +- Changing Git stage or unstage execution, transport, or repository status + refresh. +- Changing discard, edit, commit, or stage-all control presentation. - Adding user-facing copy, notifications, cancellation, or progress estimates. - Changing Changes-panel navigation, grouping, row density, or scroll ownership. diff --git a/docs/specs/ui/system-design/changes-file-action-feedback.md b/docs/specs/ui/system-design/changes-file-action-feedback.md index 9a94b922df..a38a730f55 100644 --- a/docs/specs/ui/system-design/changes-file-action-feedback.md +++ b/docs/specs/ui/system-design/changes-file-action-feedback.md @@ -10,24 +10,34 @@ requirements: ## Purpose and boundaries This design keeps the existing per-file stage and unstage spinner visible after -a fine-pointer user leaves the affected Changes row. It changes only the -presentation precedence inside the shared file-action slot. Git operation -dispatch, repository scoping, status refresh, and pending-state cleanup remain -unchanged. +a fine-pointer user leaves the affected Changes row. It defines both the +presentation precedence inside the shared file-action slot and operation-owned +pending cleanup in `useSessionGit`. Git operation transport, repository +scoping, and status acquisition remain unchanged. ## Requirement mapping -| Requirement | Design section | -| --- | --- | -| `REQ-UI-CHANGES-FILE-ACTION-FEEDBACK-001` | [Components and responsibilities](#components-and-responsibilities), [Visibility precedence](#visibility-precedence), [Responsive behavior](#responsive-behavior), and [Verification](#verification) | +| Requirement | Design section | +| ----------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `REQ-UI-CHANGES-FILE-ACTION-FEEDBACK-001` | [Components and responsibilities](#components-and-responsibilities), [Visibility precedence](#visibility-precedence), [Responsive behavior](#responsive-behavior), [Failure and recovery](#failure-and-recovery), and [Verification](#verification) | ## Components and responsibilities - `useSessionGit` continues to own `pendingStageFiles`, keyed by repository and - path. It starts the per-file pending state before dispatching - `worktree.stage` or `worktree.unstage` and clears it when the associated - request completes, including the failure path. Independent status frames do - not clear an in-flight marker. + path, plus a unique request owner containing the requested stage or unstage + transition and the active session/environment/branch generation. A newer + request in either direction replaces the owner for overlapping keys. +- Pending reconciliation clears a successful operation only after both its + successful response and refreshed status reach the requested staged or + unstaged state. Either signal can arrive first. Partial failures clear only + failed repository scopes; successful scopes remain pending for status + reconciliation. Failure cleanup clears a key only while the failed request + still owns it, so stale status and an older superseded request cannot clear a + newer action. +- Session changes, environment replacement, branch switches, and related + workspace-source generations reset the ownership map and visible pending set. + Late callbacks retain their prior request owner and cannot mutate the + successor scope. - `changes-panel-tree.tsx` and `changes-panel-timeline.tsx` continue to derive `FileRow.isPending` from that set. No additional local loading state is added. - `FileRow` continues to share one implementation across the desktop Changes @@ -79,10 +89,19 @@ section. ## Failure and recovery The UI does not infer completion from hover or elapsed time. It follows the -existing `isPending` input. Request completion or failure clears that state; -the slot then restores the idle icon/action presentation. Independent status -refreshes cannot clear an in-flight marker. Backend and WebSocket errors remain -surfaced through their current paths. +existing `isPending` input. Pending state clears only after the operation that +currently owns the repository/path key has returned success and a refreshed +status shows the target state. Either event can arrive first. + +A failed or thrown request clears its keys only when it remains their current +owner. For a multi-repository result, failure cleanup selects only unsuccessful +or skipped repository scopes. If any newer operation supersedes the request, +its later completion or failure cannot clear the newer pending state. The newer +action remains pending until its own target state appears or its request fails. +Changing the session/environment/branch/workspace-source generation clears the +visible state before the successor scope can reuse the same repository/path +key. After cleanup, the slot restores the idle icon/action presentation, and +backend or WebSocket errors remain surfaced through their current paths. ## Verification @@ -92,6 +111,12 @@ surfaced through their current paths. staged section. - The same regression pauses `worktree.unstage`, repeats the pointer-leave assertion, and then proves the file returns to the unstaged section. +- A focused hook regression publishes a stale same-repository snapshot during + each paused action and proves pending state survives until status reflects + the requested transition. +- Focused hook regressions cover stage and unstage partial failures across two + repositories, same-direction overlapping requests, active-session changes, + and checked-out branch generation changes. - The test intercepts only the selected worktree request at the WebSocket transport boundary and forwards every unrelated frame. - Existing coarse-pointer component and Pixel 5 Changes tests continue to prove