From e6e096645aa2672141346ff88a17f97537b1ccbf Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 24 Jun 2026 19:05:11 -0700 Subject: [PATCH] fix(workspace): address code-review findings on the workspace diff + Git Manager MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the multi-agent /ce-code-review of PR #1749 (no P0/P1 correctness bugs; these are perf, race-hardening, and convention fixes): - P1 (perf/reliability): the workspace diff ran git subprocesses serially per sub-repo AND per file — an N×M explosion with no aggregate cap. Add a bounded mapWithConcurrency helper (order-preserving) and parallelize the per-file patch loop (cap 8) and the per-sub-repo loop (cap 4). Deleted files still fetch their patch (skipping it would drop deletes from /file-diffs and zero /diff stats). - P2 (frontend race): GitManagerModal's workspace-detection could be clobbered by a previous project's in-flight fetch on a rapid projectId switch / close-reopen. Add a detectionGenerationRef guard — only the latest detection run may mutate state; the effect cleanup bumps the generation to abandon superseded runs. - P2 (DRY): reuse the existing parseStatusCode instead of re-inlining the status-code mapping. - P2 (convention): FNXC-tag the new functions/branches per CLAUDE.md. - P3: extract DIFF_TIMEOUT_MS/FILE_DIFFS_TIMEOUT_MS constants, drop a dead catch-assignment, note the done-fallback oldPath limitation. Tests: order-preservation after parallelization; rapid-project-switch generation guard (a stale workspace verdict must not suppress a new project's real error). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../app/components/GitManagerModal.tsx | 24 +++- .../app/components/TaskChangesTab.tsx | 1 + .../__tests__/GitManagerModal.test.tsx | 24 ++++ .../__tests__/routes-diff-workspace.test.ts | 36 ++++++ .../routes/register-session-diff-routes.ts | 120 +++++++++++++----- 5 files changed, 168 insertions(+), 37 deletions(-) diff --git a/packages/dashboard/app/components/GitManagerModal.tsx b/packages/dashboard/app/components/GitManagerModal.tsx index 3be5667be1..1771c5a851 100644 --- a/packages/dashboard/app/components/GitManagerModal.tsx +++ b/packages/dashboard/app/components/GitManagerModal.tsx @@ -285,6 +285,16 @@ export function GitManagerModal({ isOpen, onClose, tasks: _tasks, addToast, proj // common non-workspace-OK path (where the first fetch already succeeded). const suppressedRootRaceRef = useRef(false); const [detectionResolved, setDetectionResolved] = useState(false); + /* + FNXC:Workspace 2026-06-25-09:40 (detection generation guard): + A rapid projectId switch (or close→reopen) can leave a previous project's fetchWorkspaceRepos + promise in flight. When it resolves it must NOT overwrite the CURRENT project's detection verdict — + doing so could suppress a real error for the new project or mis-fire the re-surface effect. Each + detection run is stamped with a monotonically increasing generation; only the latest run is allowed + to mutate detection state, and the effect cleanup bumps the generation so a superseded/unmounted run + is abandoned. + */ + const detectionGenerationRef = useRef(0); // ── Changes state const [fileChanges, setFileChanges] = useState([]); @@ -950,11 +960,15 @@ export function GitManagerModal({ isOpen, onClose, tasks: _tasks, addToast, proj selectedRepo in the effect deps, preserving the projectId-keyed intent. */ useEffect(() => { - // Reset detection on project switch so a stale verdict can't suppress a real error. + // Reset detection on project switch so a stale verdict can't suppress a real error. The + // generation guard (see ref note above) makes a superseded in-flight resolution a no-op. + const gen = ++detectionGenerationRef.current; workspaceDetectionRef.current = { resolved: false, isWorkspace: false }; + suppressedRootRaceRef.current = false; setDetectionResolved(false); fetchWorkspaceRepos(projectId) .then((result) => { + if (gen !== detectionGenerationRef.current) return; const repos = result.repos; workspaceDetectionRef.current = { resolved: true, isWorkspace: repos.length > 0 }; setWorkspaceRepos(repos); @@ -963,11 +977,17 @@ export function GitManagerModal({ isOpen, onClose, tasks: _tasks, addToast, proj ); }) .catch(() => { + if (gen !== detectionGenerationRef.current) return; workspaceDetectionRef.current = { resolved: true, isWorkspace: false }; setWorkspaceRepos([]); setSelectedRepo(null); }) - .finally(() => setDetectionResolved(true)); + .finally(() => { + if (gen !== detectionGenerationRef.current) return; + setDetectionResolved(true); + }); + // Bump the generation on cleanup so an unmounted/superseded run's late resolution is abandoned. + return () => { detectionGenerationRef.current++; }; }, [projectId]); // keyed on projectId; selectedRepo is revalidated via the functional updater // FNXC:Workspace 2026-06-25-00:10: once detection settles, re-surface a suppressed root-race error diff --git a/packages/dashboard/app/components/TaskChangesTab.tsx b/packages/dashboard/app/components/TaskChangesTab.tsx index 1ac2815139..582d3d67fb 100644 --- a/packages/dashboard/app/components/TaskChangesTab.tsx +++ b/packages/dashboard/app/components/TaskChangesTab.tsx @@ -20,6 +20,7 @@ interface TaskChangesTabProps { column?: ColumnId; mergeDetails?: MergeDetails; /** + * FNXC:Workspace 2026-06-25-09:40: * True for a workspace (multi-repo) task. Such a task has no singular * `worktree`/`branch` — its changes live in per-sub-repo worktrees, which the * backend `/tasks/:id/diff` now aggregates (repo-prefixed paths). Used to skip diff --git a/packages/dashboard/app/components/__tests__/GitManagerModal.test.tsx b/packages/dashboard/app/components/__tests__/GitManagerModal.test.tsx index 32f2859119..e00773951a 100644 --- a/packages/dashboard/app/components/__tests__/GitManagerModal.test.tsx +++ b/packages/dashboard/app/components/__tests__/GitManagerModal.test.tsx @@ -323,6 +323,30 @@ describe("GitManagerModal", () => { }); }); + it("does not let a stale workspace project's late detection suppress a real error after a rapid project switch", async () => { + // FNXC:Workspace 2026-06-25-09:40 (generation guard): switch from workspace project A (whose + // fetchWorkspaceRepos resolves LATE) to broken non-workspace project B before A resolves. A's late + // "workspace" verdict must be abandoned (generation guard) so it can't suppress B's real error. + let resolveA: (v: { repos: string[] }) => void = () => {}; + const aPromise = new Promise<{ repos: string[] }>((r) => { resolveA = r; }); + (fetchWorkspaceRepos as any).mockImplementation((pid: string) => + pid === "projA" ? aPromise : Promise.resolve({ repos: [] })); + (fetchGitStatus as any).mockRejectedValue(new Error("Not a git repository")); + + const { rerender } = render( + , + ); + // Switch to B before A's detection resolves. + rerender(); + // A resolves late as a workspace — must be ignored for the now-current project B. + resolveA({ repos: ["openvide"] }); + + // B is a genuinely broken non-workspace repo → its error must still surface. + await waitFor(() => { + expect(mockAddToast).toHaveBeenCalledWith(expect.stringMatching(/not a git repository/i), "error"); + }); + }); + // ── Basic Rendering ───────────────────────────────────────── it("renders nothing when not open", () => { diff --git a/packages/dashboard/src/__tests__/routes-diff-workspace.test.ts b/packages/dashboard/src/__tests__/routes-diff-workspace.test.ts index d1fa0d2c47..15ac0b266b 100644 --- a/packages/dashboard/src/__tests__/routes-diff-workspace.test.ts +++ b/packages/dashboard/src/__tests__/routes-diff-workspace.test.ts @@ -103,6 +103,42 @@ describe("workspace task diff aggregation", () => { expect(res.body.stats).toEqual({ filesChanged: 2, additions: 3, deletions: 1 }); }); + it("preserves deterministic repo-sorted order across the concurrent (parallelized) aggregation", async () => { + // FNXC:WorkspaceDiff 2026-06-25-09:40: sub-repos are now diffed concurrently; the output must + // still be sorted by repo key regardless of which sub-repo's git calls finish first. Three repos + // inserted out of order, with the first-sorted repo deliberately given the slowest git response. + const task = workspaceTask(); + (task as any).workspaceWorktrees = { + zulu: { worktreePath: "/wt/zulu", branch: "fusion/mult-002", baseCommitSha: "baseZ" }, + alpha: { worktreePath: "/wt/alpha", branch: "fusion/mult-002", baseCommitSha: "baseA" }, + mike: { worktreePath: "/wt/mike", branch: "fusion/mult-002", baseCommitSha: "baseM" }, + }; + const resp: Record> = { + "/wt/alpha": { "diff --name-status -M baseA..HEAD": "A\ta.ts", "diff --cached --name-status -M": "", "diff --name-status -M": "", "diff baseA -- a.ts": "+x\n" }, + "/wt/mike": { "diff --name-status -M baseM..HEAD": "A\tm.ts", "diff --cached --name-status -M": "", "diff --name-status -M": "", "diff baseM -- m.ts": "+y\n" }, + "/wt/zulu": { "diff --name-status -M baseZ..HEAD": "A\tz.ts", "diff --cached --name-status -M": "", "diff --name-status -M": "", "diff baseZ -- z.ts": "+w\n" }, + }; + runGitCommandMock.mockImplementation(async (gitArgs: string[], cwd?: string) => { + const key = gitArgs.join(" "); + const repo = (cwd && resp[cwd]) || {}; + if (key in repo) { + // Make the first-sorted repo (alpha) resolve LAST to prove order is by key, not completion. + if (cwd === "/wt/alpha") await new Promise((r) => setTimeout(r, 5)); + return repo[key] ?? ""; + } + throw new Error(`Unexpected git command [${cwd}]: ${key}`); + }); + + const store = new MockStore(); + store.addTask(task); + const app = createServer(store as any); + const { get } = await import("../test-request.js"); + const res = await get(app, "/api/tasks/MULT-002/diff"); + + expect(res.status).toBe(200); + expect(res.body.files.map((f: any) => f.path)).toEqual(["alpha/a.ts", "mike/m.ts", "zulu/z.ts"]); + }); + it("/file-diffs returns repo-prefixed per-file patches", async () => { const store = new MockStore(); store.addTask(workspaceTask()); diff --git a/packages/dashboard/src/routes/register-session-diff-routes.ts b/packages/dashboard/src/routes/register-session-diff-routes.ts index 3b848688ab..5ddc7785cc 100644 --- a/packages/dashboard/src/routes/register-session-diff-routes.ts +++ b/packages/dashboard/src/routes/register-session-diff-routes.ts @@ -383,6 +383,38 @@ interface WorktreeDetailedFile { oldPath?: string; } +/* +FNXC:WorkspaceDiff 2026-06-25-09:40: +Per-call git timeouts for the task-diff endpoints. /diff allows a longer budget than /file-diffs +because the former drives the primary Changes view; both are named so the difference is visible at a +glance and the literals are not duplicated across call sites. +*/ +const DIFF_TIMEOUT_MS = 10_000; +const FILE_DIFFS_TIMEOUT_MS = 5_000; + +/* +FNXC:WorkspaceDiff 2026-06-25-09:40: +Bounded-concurrency mapper. A workspace task fans the diff out across N sub-repos × M files; running +those git subprocesses strictly serially makes the Changes tab block for a long time on large +multi-repo tasks (each per-file `git diff` is an independent subprocess). Run them concurrently with a +cap so we get parallel wall-clock without spawning an unbounded herd of git processes. Output order is +preserved (results indexed by input position) so the aggregated diff stays deterministic. +*/ +async function mapWithConcurrency(items: T[], limit: number, fn: (item: T, index: number) => Promise): Promise { + const results = new Array(items.length); + let cursor = 0; + const workerCount = Math.max(1, Math.min(limit, items.length)); + const workers = Array.from({ length: workerCount }, async () => { + for (;;) { + const index = cursor++; + if (index >= items.length) return; + results[index] = await fn(items[index]!, index); + } + }); + await Promise.all(workers); + return results; +} + /** * Build the per-file detailed diff for a SINGLE worktree: committed * (diffBase..HEAD) + staged + unstaged, with the committed set scoped to the @@ -445,14 +477,18 @@ async function computeWorktreeDetailedFiles( // working tree diff failed } - const results: WorktreeDetailedFile[] = []; - for (const [filePath, { statusCode, oldPath }] of fileMap.entries()) { - if (!filePath) continue; - - let status: "added" | "modified" | "deleted" | "renamed" = "modified"; - if (statusCode.startsWith("A")) status = "added"; - else if (statusCode.startsWith("D")) status = "deleted"; - else if (statusCode.startsWith("R")) status = "renamed"; + /* + FNXC:WorkspaceDiff 2026-06-25-09:40: + The per-file `git diff` patch fetch is the dominant cost (one subprocess per changed file). Run it + with bounded concurrency instead of a serial await loop — independent files do not depend on each + other, so this collapses M serial git spawns to ~M/limit wall-clock. We deliberately do NOT skip the + patch for deleted files: /file-diffs filters out empty-patch entries and the patch supplies the + additions/deletions counts, so a delete needs its real patch to stay visible and counted. Status + uses the shared parseStatusCode helper (single source of truth for the A/D/R/M mapping). + */ + const entries = Array.from(fileMap.entries()).filter(([filePath]) => Boolean(filePath)); + const results = await mapWithConcurrency(entries, 8, async ([filePath, { statusCode, oldPath }]) => { + const status = parseStatusCode(statusCode); let patch = ""; try { @@ -464,8 +500,10 @@ async function computeWorktreeDetailedFiles( } const { additions, deletions } = countPatchLines(patch); - results.push(oldPath ? { path: filePath, status, additions, deletions, patch, oldPath } : { path: filePath, status, additions, deletions, patch }); - } + return oldPath + ? { path: filePath, status, additions, deletions, patch, oldPath } + : { path: filePath, status, additions, deletions, patch }; + }); return results; } @@ -491,23 +529,30 @@ async function computeWorkspaceTaskFiles( timeoutMs: number, ): Promise { const worktrees = task.workspaceWorktrees ?? {}; - const all: WorktreeDetailedFile[] = []; - // Deterministic, repo-sorted order so the aggregated list is stable. - for (const repoRel of Object.keys(worktrees).sort()) { + /* + FNXC:WorkspaceDiff 2026-06-25-09:40: + Resolve each sub-repo's diff CONCURRENTLY (bounded) rather than awaiting them one at a time: every + sub-repo's git work is independent, so a serial loop made the aggregate cost N×(per-repo) and could + block the response for a long time on a many-repo task. Keys are sorted first and mapped by position, + so the aggregated output stays in deterministic repo-sorted order regardless of completion order. + */ + const repoRels = Object.keys(worktrees).sort(); + const perRepo = await mapWithConcurrency(repoRels, 4, async (repoRel) => { const entry = worktrees[repoRel]; - if (!entry) continue; + if (!entry) return [] as WorktreeDetailedFile[]; let repoFiles: WorktreeDetailedFile[] = []; - // Prefer the live sub-repo worktree (in-progress / in-review). + // Prefer the live sub-repo worktree (in-progress / in-review). The access() + // probe is an optimistic fast-path skip; the try/catch below is the real guard. let worktreeUsable = false; if (entry.worktreePath) { try { await access(entry.worktreePath); worktreeUsable = true; } catch { - worktreeUsable = false; + // worktree gone → fall through to the landed-range fallback } } if (worktreeUsable) { @@ -528,6 +573,9 @@ async function computeWorkspaceTaskFiles( // Fallback: landed range in the sub-repo root (a done task whose per-repo // worktree was already cleaned up). Each sub-repo lands independently with // its own baseCommitSha → landedSha. + // FNXC:WorkspaceDiff 2026-06-25-09:40: collectDoneRangeFiles returns AggregatedDoneTaskFile, which + // carries no oldPath, so a renamed file's rename-SOURCE is unavailable on this done fallback (the + // file still shows under its new path). The live-worktree path above does preserve oldPath. if (repoFiles.length === 0 && entry.baseCommitSha && entry.landedSha) { const repoRootDir = join(rootDir, repoRel); try { @@ -544,16 +592,15 @@ async function computeWorkspaceTaskFiles( } } - for (const file of repoFiles) { - all.push({ - ...file, - path: `${repoRel}/${file.path}`, - oldPath: file.oldPath ? `${repoRel}/${file.oldPath}` : undefined, - }); - } - } + // Prefix every path with the sub-repo key so the Changes tab shows which repo each file is in. + return repoFiles.map((file) => ({ + ...file, + path: `${repoRel}/${file.path}`, + oldPath: file.oldPath ? `${repoRel}/${file.oldPath}` : undefined, + })); + }); - return all; + return perRepo.flat(); } function extractCommitShaCandidate(event: { target?: unknown; metadata?: unknown; payload?: unknown; newValue?: unknown }): string | undefined { @@ -914,12 +961,13 @@ export function registerSessionDiffRoutes(router: Router, deps: SessionDiffRoute return; } - // Workspace tasks have no singular worktree/branch; their changes live in - // per-sub-repo worktrees. Aggregate across them (repo-prefixed paths) and - // short-circuit before the single-repo logic, which would diff the non-git - // workspace root and return empty. + // FNXC:WorkspaceDiff 2026-06-25-09:40: + // Workspace tasks have no singular worktree/branch; their changes live in per-sub-repo + // worktrees. Aggregate across them (repo-prefixed paths) and short-circuit BEFORE the single-repo + // logic, which would diff the non-git workspace root and return empty. renamed→modified is folded + // to match the /diff contract (which has no 'renamed' status; /file-diffs keeps it). if (isWorkspaceTask(task)) { - const workspaceFiles = await computeWorkspaceTaskFiles(task, scopedStore.getRootDir(), 10000); + const workspaceFiles = await computeWorkspaceTaskFiles(task, scopedStore.getRootDir(), DIFF_TIMEOUT_MS); const files = workspaceFiles.map((file) => ({ path: file.path, status: file.status === "renamed" ? "modified" : file.status, @@ -1118,7 +1166,7 @@ export function registerSessionDiffRoutes(router: Router, deps: SessionDiffRoute // shared with the per-sub-repo workspace aggregation. Renames fold to // "modified" here (the /diff shape has no "renamed" status), matching the // previous inline behaviour. - const detailed = await computeWorktreeDetailedFiles(task, cwd, 10000); + const detailed = await computeWorktreeDetailedFiles(task, cwd, DIFF_TIMEOUT_MS); const files = detailed.map((file) => ({ path: file.path, status: file.status === "renamed" ? ("modified" as const) : file.status, @@ -1151,10 +1199,12 @@ export function registerSessionDiffRoutes(router: Router, deps: SessionDiffRoute return; } - // Workspace tasks aggregate per-sub-repo patches (repo-prefixed paths); - // short-circuit before the single-repo logic that diffs the non-git root. + // FNXC:WorkspaceDiff 2026-06-25-09:40: + // Workspace tasks aggregate per-sub-repo patches (repo-prefixed paths); short-circuit before the + // single-repo logic that diffs the non-git root. Unlike /diff, /file-diffs preserves the + // 'renamed' status + oldPath. Empty-patch entries are dropped (parity with the single-repo path). if (isWorkspaceTask(task)) { - const workspaceFiles = (await computeWorkspaceTaskFiles(task, scopedStore.getRootDir(), 5000)) + const workspaceFiles = (await computeWorkspaceTaskFiles(task, scopedStore.getRootDir(), FILE_DIFFS_TIMEOUT_MS)) .filter((file) => file.patch) .map((file) => (file.oldPath ? { path: file.path, status: file.status, diff: file.patch, oldPath: file.oldPath } @@ -1311,7 +1361,7 @@ export function registerSessionDiffRoutes(router: Router, deps: SessionDiffRoute // shared with the per-sub-repo workspace aggregation. Files with an empty // patch (e.g. pure renames with no content change) are dropped, matching // the previous inline behaviour. - const detailed = await computeWorktreeDetailedFiles(task, cwd, 5000); + const detailed = await computeWorktreeDetailedFiles(task, cwd, FILE_DIFFS_TIMEOUT_MS); const files = detailed .filter((file) => file.patch) .map((file) => (file.oldPath