diff --git a/apps/hook/server/index.ts b/apps/hook/server/index.ts index 8c1849996..b0d8d588a 100644 --- a/apps/hook/server/index.ts +++ b/apps/hook/server/index.ts @@ -63,7 +63,7 @@ import { startAnnotateServer, handleAnnotateServerReady, } from "@plannotator/server/annotate"; -import { type DiffType, prepareLocalReviewDiff, gitRuntime } from "@plannotator/server/vcs"; +import { type DiffType, gitRuntime, prepareLocalReviewDiff } from "@plannotator/server/vcs"; import { loadConfig, resolveDefaultDiffType, resolveUseJina } from "@plannotator/shared/config"; import { parseReviewArgs } from "@plannotator/shared/review-args"; import { stripAtPrefix, resolveAtReference } from "@plannotator/shared/at-reference"; @@ -108,6 +108,7 @@ import { } from "./cli"; import path from "path"; import { tmpdir } from "os"; +import { buildWorkspaceLocalRepos, buildWorkspacePRRepos } from "@plannotator/server/review-workspace"; // Embed the built HTML at compile time // @ts-ignore - Bun import attribute for text @@ -295,6 +296,11 @@ if (args[0] === "sessions") { const isPRMode = urlArg !== undefined; const useLocal = isPRMode && reviewArgs.useLocal; + // Multi-PR workspace: detect additional PR URLs beyond the first parsed one + const extraUrlArgs = args.slice(1).filter((arg) => arg.startsWith("http://") || arg.startsWith("https://")); + const allPRUrls = urlArg ? [urlArg, ...extraUrlArgs.filter(u => u !== urlArg)] : extraUrlArgs; + const isMultiPRMode = allPRUrls.length > 1; + let rawPatch: string; let gitRef: string; let diffError: string | undefined; @@ -304,8 +310,13 @@ if (args[0] === "sessions") { let agentCwd: string | undefined; let worktreePool: WorktreePool | undefined; let worktreeCleanup: (() => void | Promise) | undefined; + let workspaceRepos: Awaited> | undefined; - if (isPRMode) { + if (isMultiPRMode) { + workspaceRepos = await buildWorkspacePRRepos(allPRUrls); + rawPatch = ""; + gitRef = "Workspace review"; + } else if (isPRMode) { // --- PR Review Mode --- const prRef = parsePRUrl(urlArg); if (!prRef) { @@ -504,6 +515,16 @@ if (args[0] === "sessions") { rawPatch = diffResult.rawPatch; gitRef = diffResult.gitRef; diffError = diffResult.error; + + // Fallback: if no VCS detected, try workspace review (multi-repo / poly-repo setups) + if (!gitContext) { + workspaceRepos = await buildWorkspaceLocalRepos(process.cwd()); + if (workspaceRepos.length === 0) { + throw new Error("Not in a git repo and no nested repositories were found."); + } + rawPatch = ""; + gitRef = "Workspace review"; + } } const reviewProject = (await detectProjectName()) ?? "_unknown"; @@ -517,6 +538,7 @@ if (args[0] === "sessions") { diffType: gitContext ? (initialDiffType ?? "unstaged") : undefined, gitContext, prMetadata, + workspaceRepos, agentCwd, worktreePool, sharingEnabled, diff --git a/packages/review-editor/utils/buildFileTree.workspace.test.ts b/packages/review-editor/utils/buildFileTree.workspace.test.ts new file mode 100644 index 000000000..c3d22c3e1 --- /dev/null +++ b/packages/review-editor/utils/buildFileTree.workspace.test.ts @@ -0,0 +1,243 @@ +import { describe, it, expect } from "bun:test"; +import { buildFileTree, getAncestorPaths, getAllFolderPaths } from "./buildFileTree"; +import type { DiffFile } from "../types"; + +const diffFile = (path: string, overrides: Partial = {}): DiffFile => ({ + path, + patch: "", + additions: 0, + deletions: 0, + ...overrides, +}); + +describe("buildFileTree - workspace mode with repo-prefixed paths", () => { + it("builds separate trees for different repo prefixes", () => { + const files = [ + diffFile("repo-a/src/index.ts"), + diffFile("repo-b/src/index.ts"), + ]; + const tree = buildFileTree(files); + + // With flat fallback: single root folder with only file children gets unwrapped + // But here we have two repos at root level, so they stay as folders + expect(tree.length).toBeGreaterThanOrEqual(2); + // After collapseSingleChild, paths like repo-a/src/index.ts become: + // folder: "repo-a/src" with file child "index.ts" + const names = tree.map(n => n.name); + expect(names).toContain("repo-a/src"); + expect(names).toContain("repo-b/src"); + }); + + it("handles same relative paths in different repos", () => { + const files = [ + diffFile("repo-a/src/utils/helper.ts", { additions: 5, deletions: 2 }), + diffFile("repo-b/src/utils/helper.ts", { additions: 3, deletions: 1 }), + ]; + const tree = buildFileTree(files); + + // After collapseSingleChild: repo-a/src/utils becomes a single folder node + const repoA = tree.find(n => n.name === "repo-a/src/utils"); + const repoB = tree.find(n => n.name === "repo-b/src/utils"); + + expect(repoA).toBeDefined(); + expect(repoB).toBeDefined(); + + // Each should have the helper.ts file as a child + const repoAFile = repoA?.children?.find(n => n.name === "helper.ts"); + const repoBFile = repoB?.children?.find(n => n.name === "helper.ts"); + + expect(repoAFile).toBeDefined(); + expect(repoBFile).toBeDefined(); + expect(repoAFile?.path).toBe("repo-a/src/utils/helper.ts"); + expect(repoBFile?.path).toBe("repo-b/src/utils/helper.ts"); + expect(repoAFile?.additions).toBe(5); + expect(repoBFile?.additions).toBe(3); + }); + + it("handles nested repo labels (longest prefix)", () => { + // Simulates repos like "apps", "apps/api", "apps/web" + const files = [ + diffFile("apps/src/main.ts"), + diffFile("apps/api/src/server.ts"), + diffFile("apps/web/src/app.ts"), + ]; + const tree = buildFileTree(files); + + // All under single "apps" root, with children for each sub-repo + // After collapseSingleChild: api/src and web/src collapse + expect(tree).toHaveLength(1); + expect(tree[0].name).toBe("apps"); + expect(tree[0].type).toBe("folder"); + + // Children: "api/src" (collapsed), "src" (from apps/src), "web/src" (collapsed) + const childNames = tree[0].children?.map(n => n.name).sort(); + expect(childNames).toEqual(["api/src", "src", "web/src"]); + }); + + it("handles deeply nested repo labels", () => { + const files = [ + diffFile("packages/shared/utils/helpers/string.ts"), + diffFile("packages/core/src/index.ts"), + ]; + const tree = buildFileTree(files); + + expect(tree).toHaveLength(1); + expect(tree[0].name).toBe("packages"); + expect(tree[0].type).toBe("folder"); + + // After collapseSingleChild, packages/core/src collapses to "core/src" + // and packages/shared/utils/helpers collapses to "shared/utils/helpers" + const children = tree[0].children?.map(n => n.name).sort(); + expect(children).toEqual(["core/src", "shared/utils/helpers"]); + }); + + it("collapses single-child folders correctly with repo prefixes", () => { + const files = [ + diffFile("repo-a/src/components/Button.tsx"), + ]; + const tree = buildFileTree(files); + + // After collapseSingleChild: repo-a/src/components collapses to single path + // Then flat fallback kicks in: single folder with only file children gets unwrapped + // Result: just the file at root level + expect(tree).toHaveLength(1); + expect(tree[0].name).toBe("Button.tsx"); + expect(tree[0].type).toBe("file"); + expect(tree[0].path).toBe("repo-a/src/components/Button.tsx"); + }); + + it("aggregates stats correctly across repo boundaries", () => { + const files = [ + diffFile("repo-a/src/index.ts", { additions: 10, deletions: 5 }), + diffFile("repo-a/src/utils.ts", { additions: 5, deletions: 2 }), + diffFile("repo-b/src/index.ts", { additions: 8, deletions: 3 }), + ]; + const tree = buildFileTree(files); + + // After collapseSingleChild: repo-a/src contains both files + const repoA = tree.find(n => n.name === "repo-a/src"); + const repoB = tree.find(n => n.name === "repo-b/src"); + + expect(repoA?.additions).toBe(15); // 10 + 5 + expect(repoA?.deletions).toBe(7); // 5 + 2 + expect(repoB?.additions).toBe(8); + expect(repoB?.deletions).toBe(3); + }); + + it("handles repo labels with special characters", () => { + const files = [ + diffFile("my-repo_2.0/src/index.ts"), + diffFile("my-repo_2.0-beta/src/app.ts"), + ]; + const tree = buildFileTree(files); + + // Two separate root-level folders after collapse + expect(tree.length).toBeGreaterThanOrEqual(2); + const names = tree.map(n => n.name); + expect(names).toContain("my-repo_2.0/src"); + expect(names).toContain("my-repo_2.0-beta/src"); + }); + + it("preserves full prefixed path in node path property", () => { + const files = [ + diffFile("owner/repo/src/index.ts"), + ]; + const tree = buildFileTree(files); + + // Collapses to "owner/repo/src" folder, then flat fallback unwraps + // Result: just the file with full path preserved + expect(tree[0].name).toBe("index.ts"); + expect(tree[0].path).toBe("owner/repo/src/index.ts"); + }); + + it("handles empty file list", () => { + const tree = buildFileTree([]); + expect(tree).toHaveLength(0); + }); + + it("handles single file in repo (flat fallback)", () => { + const files = [diffFile("repo-a/README.md")]; + const tree = buildFileTree(files); + + // Flat fallback: single root folder with only file children gets unwrapped + // But first collapseSingleChild collapses repo-a to contain README.md + // Then flat fallback sees single folder "repo-a" with file child, unwraps it + expect(tree).toHaveLength(1); + expect(tree[0].name).toBe("README.md"); + expect(tree[0].type).toBe("file"); + expect(tree[0].path).toBe("repo-a/README.md"); + }); + + it("handles multiple files in same repo subdirectories", () => { + const files = [ + diffFile("repo-a/src/index.ts"), + diffFile("repo-a/src/app.ts"), + diffFile("repo-a/lib/helpers.ts"), + ]; + const tree = buildFileTree(files); + + // repo-a has two children: src (with 2 files) and lib (with 1 file) + // So it doesn't get unwrapped by flat fallback + expect(tree).toHaveLength(1); + expect(tree[0].name).toBe("repo-a"); + expect(tree[0].type).toBe("folder"); + + const children = tree[0].children?.map(n => n.name).sort(); + expect(children).toEqual(["lib", "src"]); + }); +}); + +describe("getAncestorPaths - workspace mode", () => { + it("returns ancestor paths for repo-prefixed file", () => { + const paths = getAncestorPaths("repo-a/src/utils/helper.ts"); + expect(paths).toEqual([ + "repo-a", + "repo-a/src", + "repo-a/src/utils", + ]); + }); + + it("handles deeply nested repo labels", () => { + const paths = getAncestorPaths("packages/shared/utils/helpers/string.ts"); + expect(paths).toEqual([ + "packages", + "packages/shared", + "packages/shared/utils", + "packages/shared/utils/helpers", + ]); + }); + + it("handles flat repo structure", () => { + const paths = getAncestorPaths("repo-a/file.ts"); + expect(paths).toEqual(["repo-a"]); + }); +}); + +describe("getAllFolderPaths - workspace mode", () => { + it("collects all folder paths from repo-prefixed tree", () => { + const files = [ + diffFile("repo-a/src/index.ts"), + diffFile("repo-b/src/app.ts"), + ]; + const tree = buildFileTree(files); + const folders = getAllFolderPaths(tree); + + // After collapseSingleChild, we get "repo-a/src" and "repo-b/src" + expect(folders).toContain("repo-a/src"); + expect(folders).toContain("repo-b/src"); + }); + + it("collects nested repo label folders", () => { + const files = [ + diffFile("apps/api/src/server.ts"), + diffFile("apps/web/src/app.ts"), + ]; + const tree = buildFileTree(files); + const folders = getAllFolderPaths(tree); + + // After collapseSingleChild: apps stays, apps/api/src and apps/web/src + expect(folders).toContain("apps"); + expect(folders).toContain("apps/api/src"); + expect(folders).toContain("apps/web/src"); + }); +}); diff --git a/packages/review-editor/utils/exportFeedback.workspace.test.ts b/packages/review-editor/utils/exportFeedback.workspace.test.ts new file mode 100644 index 000000000..0a57ef16b --- /dev/null +++ b/packages/review-editor/utils/exportFeedback.workspace.test.ts @@ -0,0 +1,122 @@ +import { describe, it, expect } from "bun:test"; +import { exportReviewFeedback } from "./exportFeedback"; +import type { CodeAnnotation } from "@plannotator/ui/types"; + +const ann = (overrides: Partial = {}): CodeAnnotation => ({ + id: "1", + type: "comment", + filePath: "src/index.ts", + lineStart: 10, + lineEnd: 10, + side: "new", + text: "This looks wrong", + createdAt: Date.now(), + ...overrides, +}); + +describe("exportReviewFeedback - workspace mode", () => { + it("workspace mode: uses generic header, no PR content (same as local mode)", () => { + // In workspace mode, prMetadata is explicitly undefined even if workspace exists + const result = exportReviewFeedback([ann()], undefined); + expect(result).toStartWith("# Code Review Feedback\n\n"); + expect(result).not.toContain("PR Review"); + expect(result).not.toContain("github.com"); + expect(result).not.toContain("Branch:"); + }); + + it("groups annotations by repo-prefixed file paths", () => { + const result = exportReviewFeedback([ + ann({ filePath: "repo-a/src/index.ts", lineStart: 5, text: "first" }), + ann({ filePath: "repo-b/src/index.ts", lineStart: 1, text: "second" }), + ]); + // Different repos with same relative path should be separate groups + expect(result).toContain("## repo-a/src/index.ts"); + expect(result).toContain("## repo-b/src/index.ts"); + }); + + it("sorts annotations by line number within each repo-prefixed file", () => { + const result = exportReviewFeedback([ + ann({ filePath: "repo-a/src/index.ts", lineStart: 20, text: "later" }), + ann({ filePath: "repo-a/src/index.ts", lineStart: 5, text: "earlier" }), + ann({ filePath: "repo-b/src/index.ts", lineStart: 15, text: "middle in repo-b" }), + ]); + const earlierIdx = result.indexOf("earlier"); + const laterIdx = result.indexOf("later"); + const middleInRepoB = result.indexOf("middle in repo-b"); + expect(earlierIdx).toBeLessThan(laterIdx); + // Both repo-a annotations should come before repo-b (alphabetical by path) + expect(laterIdx).toBeLessThan(middleInRepoB); + }); + + it("handles nested repo labels with overlapping paths", () => { + // Tests the longest-prefix matching behavior from resolveWorkspaceFilePath + const result = exportReviewFeedback([ + ann({ filePath: "apps/api/src/server.ts", text: "in nested repo" }), + ann({ filePath: "apps/web/src/app.ts", text: "in sibling repo" }), + ann({ filePath: "apps/src/main.ts", text: "in parent repo" }), + ]); + expect(result).toContain("## apps/api/src/server.ts"); + expect(result).toContain("## apps/web/src/app.ts"); + expect(result).toContain("## apps/src/main.ts"); + }); + + it("handles deeply nested repo labels", () => { + const result = exportReviewFeedback([ + ann({ filePath: "packages/shared/utils/helpers/string.ts", text: "deep path" }), + ]); + expect(result).toContain("## packages/shared/utils/helpers/string.ts"); + expect(result).toContain("### Line 10 (new)"); + }); + + it("groups multiple annotations on same repo-prefixed file together", () => { + const result = exportReviewFeedback([ + ann({ filePath: "repo-a/src/index.ts", lineStart: 5, text: "first comment" }), + ann({ filePath: "repo-b/src/index.ts", lineStart: 10, text: "second comment" }), + ann({ filePath: "repo-a/src/index.ts", lineStart: 15, text: "third comment" }), + ]); + // All repo-a comments should be grouped together + const repoAHeaderIdx = result.indexOf("## repo-a/src/index.ts"); + const repoBHeaderIdx = result.indexOf("## repo-b/src/index.ts"); + const firstCommentIdx = result.indexOf("first comment"); + const thirdCommentIdx = result.indexOf("third comment"); + const secondCommentIdx = result.indexOf("second comment"); + + expect(repoAHeaderIdx).toBeLessThan(repoBHeaderIdx); + expect(firstCommentIdx).toBeLessThan(thirdCommentIdx); + expect(thirdCommentIdx).toBeLessThan(repoBHeaderIdx); + expect(repoBHeaderIdx).toBeLessThan(secondCommentIdx); + }); + + it("handles file-scoped annotations with repo-prefixed paths", () => { + const result = exportReviewFeedback([ + ann({ filePath: "repo-a/src/index.ts", scope: "file", text: "file comment" }), + ann({ filePath: "repo-a/src/index.ts", lineStart: 1, lineEnd: 1, text: "line comment" }), + ]); + expect(result).toContain("## repo-a/src/index.ts"); + expect(result).toContain("### File Comment"); + expect(result).toContain("### Line 1"); + const fileIdx = result.indexOf("File Comment"); + const lineIdx = result.indexOf("Line 1"); + expect(fileIdx).toBeLessThan(lineIdx); + }); + + it("handles repo labels with special characters in paths", () => { + const result = exportReviewFeedback([ + ann({ filePath: "my-repo_2.0/src/index.ts", text: "special chars" }), + ]); + expect(result).toContain("## my-repo_2.0/src/index.ts"); + }); + + it("empty annotations returns generic message regardless of workspace mode", () => { + expect(exportReviewFeedback([], undefined)).toBe("# Code Review\n\nNo feedback provided."); + }); + + it("contains exactly one top-level heading in workspace mode", () => { + const result = exportReviewFeedback([ + ann({ filePath: "repo-a/src/a.ts" }), + ann({ filePath: "repo-b/src/b.ts" }), + ]); + const headingMatches = result.match(/^# /gm) || []; + expect(headingMatches).toHaveLength(1); + }); +}); diff --git a/packages/review-editor/utils/reviewSearch.workspace.test.ts b/packages/review-editor/utils/reviewSearch.workspace.test.ts new file mode 100644 index 000000000..400727cc5 --- /dev/null +++ b/packages/review-editor/utils/reviewSearch.workspace.test.ts @@ -0,0 +1,206 @@ +import { describe, it, expect } from "bun:test"; +import { + buildSearchIndex, + findMatchesInIndex, + findReviewSearchMatches, + groupReviewSearchMatches, +} from "./reviewSearch"; +import type { ReviewSearchableDiffFile } from "./reviewSearch"; + +const patchFile = (path: string, patch: string): ReviewSearchableDiffFile => ({ + path, + patch, + additions: 0, + deletions: 0, +}); + +describe("reviewSearch - workspace mode with repo-prefixed paths", () => { + const samplePatch = [ + "diff --git a/src/index.ts b/src/index.ts", + "--- a/src/index.ts", + "+++ b/src/index.ts", + "@@ -1,3 +1,3 @@", + " function greet() {", + "- return 'hello';", + "+ return 'hello world';", + " }", + ].join("\n"); + + it("builds search index with repo-prefixed file paths", () => { + const files = [ + patchFile("repo-a/src/index.ts", samplePatch), + patchFile("repo-b/src/index.ts", samplePatch), + ]; + const index = buildSearchIndex(files); + + // All lines should have repo-prefixed file paths + expect(index.every(line => line.filePath.startsWith("repo-"))).toBe(true); + expect(index.some(line => line.filePath === "repo-a/src/index.ts")).toBe(true); + expect(index.some(line => line.filePath === "repo-b/src/index.ts")).toBe(true); + }); + + it("finds matches across different repos with same relative path", () => { + const files = [ + patchFile("repo-a/src/utils.ts", [ + "diff --git a/src/utils.ts b/src/utils.ts", + "@@ -1 +1 @@", + "-const x = 1;", + "+const x = 2;", + ].join("\n")), + patchFile("repo-b/src/utils.ts", [ + "diff --git a/src/utils.ts b/src/utils.ts", + "@@ -1 +1 @@", + "-const y = 1;", + "+const y = 2;", + ].join("\n")), + ]; + const matches = findReviewSearchMatches(files, "const"); + + // Should find matches in both repos + const repoAMatches = matches.filter(m => m.filePath === "repo-a/src/utils.ts"); + const repoBMatches = matches.filter(m => m.filePath === "repo-b/src/utils.ts"); + + expect(repoAMatches.length).toBeGreaterThan(0); + expect(repoBMatches.length).toBeGreaterThan(0); + }); + + it("distinguishes same content in different repos", () => { + const files = [ + patchFile("repo-a/src/index.ts", [ + "diff --git a/src/index.ts b/src/index.ts", + "@@ -1 +1 @@", + "-old content", + "+new content", + ].join("\n")), + patchFile("repo-b/src/index.ts", [ + "diff --git a/src/index.ts b/src/index.ts", + "@@ -1 +1 @@", + "-old content", + "+new content", + ].join("\n")), + ]; + const matches = findReviewSearchMatches(files, "content"); + + // Should have separate match entries for each repo + const repoAIds = matches.filter(m => m.filePath === "repo-a/src/index.ts"); + const repoBIds = matches.filter(m => m.filePath === "repo-b/src/index.ts"); + + expect(repoAIds.length).toBe(2); // "old content" and "new content" + expect(repoBIds.length).toBe(2); + + // IDs should be different + const repoAIdSet = new Set(repoAIds.map(m => m.id)); + const repoBIdSet = new Set(repoBIds.map(m => m.id)); + expect(repoAIdSet.intersection(repoBIdSet).size).toBe(0); + }); + + it("handles deeply nested repo labels in search", () => { + const files = [ + patchFile("packages/shared/utils/helpers.ts", [ + "diff --git a/utils/helpers.ts b/utils/helpers.ts", + "@@ -1 +1 @@", + "-helper function", + "+improved helper", + ].join("\n")), + ]; + const matches = findReviewSearchMatches(files, "helper"); + + expect(matches.length).toBe(2); + expect(matches.every(m => m.filePath === "packages/shared/utils/helpers.ts")).toBe(true); + }); + + it("groups matches by repo-prefixed file path", () => { + const files = [ + patchFile("repo-a/src/index.ts", samplePatch), + patchFile("repo-b/src/other.ts", [ + "diff --git a/src/other.ts b/src/other.ts", + "@@ -1 +1 @@", + "-hello there", + "+goodbye there", + ].join("\n")), + ]; + const matches = findReviewSearchMatches(files, "hello"); + const groups = groupReviewSearchMatches(files, matches); + + // "hello" appears in both files (repo-a: "hello world", repo-b: "hello there") + expect(groups).toHaveLength(2); + const paths = groups.map(g => g.filePath).sort(); + expect(paths).toEqual(["repo-a/src/index.ts", "repo-b/src/other.ts"]); + }); + + it("maintains correct file indices with repo-prefixed paths", () => { + const files = [ + patchFile("repo-a/src/a.ts", samplePatch), + patchFile("repo-b/src/b.ts", samplePatch), + patchFile("repo-c/src/c.ts", samplePatch), + ]; + const matches = findReviewSearchMatches(files, "hello"); + const groups = groupReviewSearchMatches(files, matches); + + // Each group should have correct file index + const groupA = groups.find(g => g.filePath === "repo-a/src/a.ts"); + const groupB = groups.find(g => g.filePath === "repo-b/src/b.ts"); + const groupC = groups.find(g => g.filePath === "repo-c/src/c.ts"); + + expect(groupA?.fileIndex).toBe(0); + expect(groupB?.fileIndex).toBe(1); + expect(groupC?.fileIndex).toBe(2); + }); + + it("handles search in nested repo labels (longest prefix)", () => { + const files = [ + patchFile("apps/api/src/server.ts", [ + "diff --git a/src/server.ts b/src/server.ts", + "@@ -1 +1 @@", + "-server code", + "+better server", + ].join("\n")), + patchFile("apps/web/src/app.ts", [ + "diff --git a/src/app.ts b/src/app.ts", + "@@ -1 +1 @@", + "-app code", + "+better app", + ].join("\n")), + ]; + const matches = findReviewSearchMatches(files, "code"); + + const apiMatch = matches.find(m => m.filePath === "apps/api/src/server.ts"); + const webMatch = matches.find(m => m.filePath === "apps/web/src/app.ts"); + + expect(apiMatch).toBeDefined(); + expect(webMatch).toBeDefined(); + }); + + it("returns empty results for non-matching query in workspace", () => { + const files = [ + patchFile("repo-a/src/index.ts", samplePatch), + ]; + const matches = findReviewSearchMatches(files, "nonexistent"); + + expect(matches).toHaveLength(0); + }); + + it("handles empty file list in workspace mode", () => { + const index = buildSearchIndex([]); + expect(index).toHaveLength(0); + + const matches = findMatchesInIndex(index, "query"); + expect(matches).toHaveLength(0); + }); + + it("handles multiple matches on same line in same repo", () => { + const files = [ + patchFile("repo-a/src/index.ts", [ + "diff --git a/src/index.ts b/src/index.ts", + "@@ -1 +1 @@", + "-foo bar foo", + "+foo baz foo", + ].join("\n")), + ]; + const matches = findReviewSearchMatches(files, "foo"); + + // Should find 4 matches (2 on old line, 2 on new line) + expect(matches.length).toBe(4); + expect(matches.every(m => m.filePath === "repo-a/src/index.ts")).toBe(true); + }); +}); diff --git a/packages/server/claude-review.ts b/packages/server/claude-review.ts index 5b0ec0d9c..ea1387ceb 100644 --- a/packages/server/claude-review.ts +++ b/packages/server/claude-review.ts @@ -282,6 +282,7 @@ export function transformClaudeFindings( findings: ClaudeFinding[], source: string, cwd?: string, + pathTransform?: (path: string) => string, ): Array<{ source: string; filePath: string; @@ -299,7 +300,9 @@ export function transformClaudeFindings( .filter(f => f.file && typeof f.line === "number") .map(f => ({ source, - filePath: toRelativePath(f.file, cwd), + filePath: pathTransform + ? pathTransform(toRelativePath(f.file, cwd)) + : toRelativePath(f.file, cwd), lineStart: f.line, lineEnd: f.end_line ?? f.line, type: "comment", diff --git a/packages/server/codex-review.ts b/packages/server/codex-review.ts index 25619cc94..7c1f19eb7 100644 --- a/packages/server/codex-review.ts +++ b/packages/server/codex-review.ts @@ -291,6 +291,7 @@ export function transformReviewFindings( source: string, cwd?: string, author?: string, + pathTransform?: (path: string) => string, ): ReviewAnnotationInput[] { const annotations = findings .filter((f) => @@ -300,7 +301,9 @@ export function transformReviewFindings( ) .map((f) => ({ source, - filePath: toRelativePath(f.code_location.absolute_file_path, cwd), + filePath: pathTransform + ? pathTransform(toRelativePath(f.code_location.absolute_file_path, cwd)) + : toRelativePath(f.code_location.absolute_file_path, cwd), lineStart: f.code_location.line_range.start, lineEnd: f.code_location.line_range.end, type: "comment", diff --git a/packages/server/package.json b/packages/server/package.json index b44e9fe3c..1bfdda255 100644 --- a/packages/server/package.json +++ b/packages/server/package.json @@ -16,6 +16,7 @@ "./p4": "./p4.ts", "./vcs": "./vcs.ts", "./repo": "./repo.ts", + "./review-workspace": "./review-workspace.ts", "./share-url": "./share-url.ts", "./sessions": "./sessions.ts", "./project": "./project.ts", diff --git a/packages/server/review-workspace.test.ts b/packages/server/review-workspace.test.ts new file mode 100644 index 000000000..c21ed18c3 --- /dev/null +++ b/packages/server/review-workspace.test.ts @@ -0,0 +1,306 @@ +/** + * Workspace Review Tests + * + * Tests for workspace repo discovery, label building, and path resolution. + * Run: bun test packages/server/review-workspace.test.ts + */ + +import { afterEach, describe, expect, it } from "bun:test"; +import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import { spawnSync } from "node:child_process"; + +import { + prefixPatchPaths, + resolveWorkspaceFilePath, + discoverWorkspaceRepoPaths, + type WorkspaceRepoRuntimeState, +} from "./review-workspace"; + +const tempDirs: string[] = []; + +function makeTempDir(prefix: string): string { + const dir = mkdtempSync(join(tmpdir(), prefix)); + tempDirs.push(dir); + return dir; +} + +function git(cwd: string, args: string[]): string { + const result = spawnSync("git", args, { cwd, encoding: "utf-8" }); + if (result.status !== 0) { + throw new Error(result.stderr || `git ${args.join(" ")} failed`); + } + return result.stdout.trim(); +} + +function initRepo(dir: string, initialBranch = "main"): void { + git(dir, ["init"]); + git(dir, ["branch", "-M", initialBranch]); + git(dir, ["config", "user.email", "test@example.com"]); + git(dir, ["config", "user.name", "Test User"]); + writeFileSync(join(dir, "README.md"), "# Test\n", "utf-8"); + git(dir, ["add", "README.md"]); + git(dir, ["commit", "-m", "initial"]); +} + +afterEach(() => { + for (const dir of tempDirs.splice(0)) { + rmSync(dir, { recursive: true, force: true }); + } +}); + +describe("review-workspace", () => { + describe("prefixPatchPaths", () => { + it("prefixes diff headers with the repo label", () => { + const patch = [ + "diff --git a/src/index.ts b/src/index.ts", + "--- a/src/index.ts", + "+++ b/src/index.ts", + "@@ -1 +1 @@", + "-old", + "+new", + ].join("\n"); + + const result = prefixPatchPaths(patch, "repo-a"); + + expect(result).toContain("diff --git a/repo-a/src/index.ts b/repo-a/src/index.ts"); + expect(result).toContain("--- a/repo-a/src/index.ts"); + expect(result).toContain("+++ b/repo-a/src/index.ts"); + }); + + it("handles /dev/null paths correctly", () => { + const patch = [ + "diff --git a/src/index.ts b/src/index.ts", + "--- a/src/index.ts", + "+++ /dev/null", + "@@ -1 +0,0 @@", + "-content", + ].join("\n"); + + const result = prefixPatchPaths(patch, "repo-a"); + + expect(result).toContain("+++ /dev/null"); + expect(result).not.toContain("+++ b/repo-a/dev/null"); + }); + + it("handles empty patches", () => { + expect(prefixPatchPaths("", "repo-a")).toBe(""); + expect(prefixPatchPaths(" ", "repo-a")).toBe(" "); + }); + + it("handles nested paths correctly", () => { + const patch = [ + "diff --git a/packages/ui/src/index.ts b/packages/ui/src/index.ts", + "--- a/packages/ui/src/index.ts", + "+++ b/packages/ui/src/index.ts", + ].join("\n"); + + const result = prefixPatchPaths(patch, "frontend"); + + expect(result).toContain("diff --git a/frontend/packages/ui/src/index.ts b/frontend/packages/ui/src/index.ts"); + }); + }); + + describe("resolveWorkspaceFilePath", () => { + it("resolves the longest matching repo label first", () => { + const repos = [ + { id: "1", label: "apps", cwd: "/tmp/apps", selected: true, source: "local", rawPatch: "", gitRef: "" }, + { id: "2", label: "apps/api", cwd: "/tmp/apps-api", selected: true, source: "local", rawPatch: "", gitRef: "" }, + ] as WorkspaceRepoRuntimeState[]; + + const resolved = resolveWorkspaceFilePath(repos, "apps/api/src/index.ts"); + + expect(resolved?.repo.id).toBe("2"); + expect(resolved?.repoRelativePath).toBe("src/index.ts"); + }); + + it("returns null when no repo matches", () => { + const repos = [ + { id: "1", label: "frontend", cwd: "/tmp/frontend", selected: true, source: "local", rawPatch: "", gitRef: "" }, + ] as WorkspaceRepoRuntimeState[]; + + const resolved = resolveWorkspaceFilePath(repos, "backend/src/index.ts"); + + expect(resolved).toBeNull(); + }); + + it("handles exact label matches", () => { + const repos = [ + { id: "1", label: "repo-a", cwd: "/tmp/repo-a", selected: true, source: "local", rawPatch: "", gitRef: "" }, + ] as WorkspaceRepoRuntimeState[]; + + const resolved = resolveWorkspaceFilePath(repos, "repo-a/file.ts"); + + expect(resolved?.repo.id).toBe("1"); + expect(resolved?.repoRelativePath).toBe("file.ts"); + }); + + it("validates file paths for directory traversal attacks", () => { + const repos = [ + { id: "1", label: "repo", cwd: "/tmp/repo", selected: true, source: "local", rawPatch: "", gitRef: "" }, + ] as WorkspaceRepoRuntimeState[]; + + expect(() => resolveWorkspaceFilePath(repos, "repo/../../../etc/passwd")).toThrow(); + }); + }); + + describe("discoverWorkspaceRepoPaths", () => { + it("excludes the root itself even if it is a git repo", () => { + // The function is designed to discover repos WITHIN a workspace root, + // not the root itself. This allows the workspace root to be a git repo + // (e.g., a meta-repo) while still discovering nested repos. + const root = makeTempDir("plannotator-workspace-root-repo-"); + initRepo(root); + + const repos = discoverWorkspaceRepoPaths(root); + + // Root itself is excluded even though it's a git repo + expect(repos).toHaveLength(0); + expect(repos).not.toContain(root); + }); + + it("discovers multiple nested git repos", () => { + const root = makeTempDir("plannotator-workspace-multi-"); + + // Create nested repos + const frontend = join(root, "frontend"); + const backend = join(root, "backend"); + const backendApi = join(backend, "api"); + + mkdirSync(frontend, { recursive: true }); + mkdirSync(backendApi, { recursive: true }); + + initRepo(frontend); + initRepo(backendApi); + + const repos = discoverWorkspaceRepoPaths(root); + + expect(repos).toHaveLength(2); + expect(repos).toContain(frontend); + expect(repos).toContain(backendApi); + expect(repos).not.toContain(root); + expect(repos).not.toContain(backend); // backend itself is not a repo + }); + + it("stops recursion at git repo boundaries (does not discover nested repos inside other repos)", () => { + const root = makeTempDir("plannotator-workspace-boundary-"); + + // Create a repo with a nested directory that would be a repo + const parentRepo = join(root, "parent"); + const childDir = join(parentRepo, "child"); + + mkdirSync(childDir, { recursive: true }); + initRepo(parentRepo); + + // Create a git repo inside the child (should NOT be discovered separately + // because parent repo stops recursion - we don't traverse into git repos) + const grandchildRepo = join(childDir, "grandchild"); + mkdirSync(grandchildRepo, { recursive: true }); + initRepo(grandchildRepo); + + const repos = discoverWorkspaceRepoPaths(root); + + // Only the parent should be discovered - grandchild is inside a git repo + expect(repos).toHaveLength(1); + expect(repos).toContain(parentRepo); + expect(repos).not.toContain(grandchildRepo); + }); + + it("skips ignored directories", () => { + const root = makeTempDir("plannotator-workspace-skip-"); + + // Create node_modules with a fake .git (should be skipped) + const nodeModules = join(root, "node_modules", "some-pkg"); + mkdirSync(nodeModules, { recursive: true }); + mkdirSync(join(nodeModules, ".git"), { recursive: true }); + + // Create a real repo + const realRepo = join(root, "src"); + mkdirSync(realRepo, { recursive: true }); + initRepo(realRepo); + + const repos = discoverWorkspaceRepoPaths(root); + + expect(repos).toHaveLength(1); + expect(repos[0]).toBe(realRepo); + }); + + it("returns empty array when root has no git repos", () => { + const root = makeTempDir("plannotator-workspace-empty-"); + + // Create some non-git directories + mkdirSync(join(root, "src"), { recursive: true }); + mkdirSync(join(root, "docs"), { recursive: true }); + writeFileSync(join(root, "README.md"), "# Project\n", "utf-8"); + + const repos = discoverWorkspaceRepoPaths(root); + + expect(repos).toHaveLength(0); + }); + + it("sorts results alphabetically", () => { + const root = makeTempDir("plannotator-workspace-sort-"); + + const zebra = join(root, "zebra"); + const alpha = join(root, "alpha"); + const beta = join(root, "beta"); + + mkdirSync(zebra, { recursive: true }); + mkdirSync(alpha, { recursive: true }); + mkdirSync(beta, { recursive: true }); + + initRepo(zebra); + initRepo(alpha); + initRepo(beta); + + const repos = discoverWorkspaceRepoPaths(root); + + expect(repos).toEqual([alpha, beta, zebra]); + }); + + it("handles deeply nested repos", () => { + const root = makeTempDir("plannotator-workspace-deep-"); + + const deepRepo = join(root, "a", "b", "c", "d", "repo"); + mkdirSync(deepRepo, { recursive: true }); + initRepo(deepRepo); + + const repos = discoverWorkspaceRepoPaths(root); + + expect(repos).toHaveLength(1); + expect(repos[0]).toBe(deepRepo); + }); + }); + + describe("buildRepoLabel (via discoverWorkspaceRepoPaths integration)", () => { + it("uses relative path as label when possible", () => { + // This is tested indirectly through the full workspace flow + // The label building logic is internal, but we verify it works + // through resolveWorkspaceFilePath tests with realistic labels + const repos = [ + { id: "1", label: "packages/frontend", cwd: "/tmp/packages/frontend", selected: true, source: "local", rawPatch: "", gitRef: "" }, + { id: "2", label: "packages/backend", cwd: "/tmp/packages/backend", selected: true, source: "local", rawPatch: "", gitRef: "" }, + ] as WorkspaceRepoRuntimeState[]; + + const resolved1 = resolveWorkspaceFilePath(repos, "packages/frontend/src/index.ts"); + const resolved2 = resolveWorkspaceFilePath(repos, "packages/backend/api.ts"); + + expect(resolved1?.repo.id).toBe("1"); + expect(resolved2?.repo.id).toBe("2"); + }); + + it("handles duplicate basename fallback", () => { + // When two repos have the same basename but different paths, + // the second should get a numbered suffix + const repos = [ + { id: "1", label: "api", cwd: "/tmp/apps/api", selected: true, source: "local", rawPatch: "", gitRef: "" }, + { id: "2", label: "api-2", cwd: "/tmp/services/api", selected: true, source: "local", rawPatch: "", gitRef: "" }, + ] as WorkspaceRepoRuntimeState[]; + + const resolved = resolveWorkspaceFilePath(repos, "api-2/src/index.ts"); + + expect(resolved?.repo.id).toBe("2"); + }); + }); +}); diff --git a/packages/server/review-workspace.ts b/packages/server/review-workspace.ts new file mode 100644 index 000000000..42c700944 --- /dev/null +++ b/packages/server/review-workspace.ts @@ -0,0 +1,462 @@ +import { existsSync, readdirSync, statSync } from "node:fs"; +import { basename, relative, resolve } from "node:path"; + +import type { DiffType, GitContext } from "./vcs"; +import { getVcsContext, runVcsDiff, validateFilePath } from "./vcs"; +import { fetchPR, parsePRUrl, type PRMetadata } from "./pr"; +import type { + WorkspacePRCandidate, + WorkspaceRepoSource, + WorkspaceRepoState, +} from "@plannotator/shared/review-workspace"; +import { resolveDefaultDiffType, loadConfig } from "@plannotator/shared/config"; +import { parseRemoteUrl } from "@plannotator/shared/repo"; + +const SKIP_DIRS = new Set([ + ".git", + "node_modules", + ".turbo", + ".next", + "dist", + "build", + "coverage", +]); + +export interface WorkspaceRepoRuntimeState extends WorkspaceRepoState { + rawPatch: string; + gitRef: string; +} + +function normalizePath(path: string): string { + return path.replace(/\\/g, "/").replace(/^\/+/, ""); +} + +function prefixRepoPath(label: string, filePath: string): string { + const normalizedFilePath = normalizePath(filePath); + if (normalizedFilePath === "/dev/null") return normalizedFilePath; + return `${normalizePath(label)}/${normalizedFilePath}`; +} + +function rewritePatchLine(line: string, label: string): string { + // diff --git with optional quotes around paths + if (line.startsWith("diff --git a/") || line.startsWith('diff --git "a/')) { + const match = line.match(/^diff --git (?:"?a\/(.+?)"?|\/?a\/(.+?)) (?:"?b\/(.+?)"?|\/?b\/(.+?))$/); + if (!match) return line; + const oldPath = match[1] ?? match[2]; + const newPath = match[3] ?? match[4]; + return `diff --git a/${prefixRepoPath(label, oldPath)} b/${prefixRepoPath(label, newPath)}`; + } + + if (line.startsWith("--- ")) { + const path = line.slice(4); + if (path === "/dev/null") return line; + if (path.startsWith("a/")) return `--- a/${prefixRepoPath(label, path.slice(2))}`; + return line; + } + + if (line.startsWith("+++ ")) { + const path = line.slice(4); + if (path === "/dev/null") return line; + if (path.startsWith("b/")) return `+++ b/${prefixRepoPath(label, path.slice(2))}`; + return line; + } + + // rename/copy headers + if (line.startsWith("rename from ") || line.startsWith("copy from ")) { + const prefix = line.slice(0, line.indexOf(" ") + 1); + const path = line.slice(prefix.length); + return `${prefix}a/${prefixRepoPath(label, path)}`; + } + if (line.startsWith("rename to ") || line.startsWith("copy to ")) { + const prefix = line.slice(0, line.indexOf(" ") + 1); + const path = line.slice(prefix.length); + return `${prefix}b/${prefixRepoPath(label, path)}`; + } + + return line; +} + +export function prefixPatchPaths(rawPatch: string, label: string): string { + if (!rawPatch.trim()) return rawPatch; + return rawPatch + .split("\n") + .map((line) => rewritePatchLine(line, label)) + .join("\n"); +} + +export function resolveWorkspaceFilePath( + repos: WorkspaceRepoRuntimeState[], + prefixedPath: string, +): { repo: WorkspaceRepoRuntimeState; repoRelativePath: string } | null { + const normalizedPath = normalizePath(prefixedPath); + validateFilePath(normalizedPath); + + // Pre-sort once — callers should pass pre-sorted arrays for performance. + const sorted = repos[0]?.__sorted + ? repos + : [...repos].sort((a, b) => b.label.length - a.label.length); + + for (const repo of sorted) { + const prefix = `${normalizePath(repo.label)}/`; + if (normalizedPath === normalizePath(repo.label) || normalizedPath.startsWith(prefix)) { + return { + repo, + repoRelativePath: normalizedPath.slice(prefix.length), + }; + } + } + + return null; +} + +function hasGitMarker(dirPath: string): boolean { + return existsSync(resolve(dirPath, ".git")); +} + +function collectGitRepos(root: string, current: string, results: string[]): void { + let entries: ReturnType; + try { + entries = readdirSync(current, { withFileTypes: true }); + } catch { + return; + } + + if (current !== root && hasGitMarker(current)) { + results.push(current); + return; + } + + for (const entry of entries) { + if (!entry.isDirectory()) continue; + if (SKIP_DIRS.has(entry.name)) continue; + collectGitRepos(root, resolve(current, entry.name), results); + } +} + +export function discoverWorkspaceRepoPaths(root: string): string[] { + const resolvedRoot = resolve(root); + const results: string[] = []; + collectGitRepos(resolvedRoot, resolvedRoot, results); + return results.sort(); +} + +function buildRepoLabel(root: string, cwd: string, used = new Set()): string { + const rel = normalizePath(relative(root, cwd)); + const preferred = rel && rel !== "" ? rel : basename(cwd); + if (!used.has(preferred)) { + used.add(preferred); + return preferred; + } + + const fallback = normalizePath(basename(cwd)); + if (!used.has(fallback)) { + used.add(fallback); + return fallback; + } + + let counter = 2; + let next = `${fallback}-${counter}`; + while (used.has(next)) { + counter += 1; + next = `${fallback}-${counter}`; + } + used.add(next); + return next; +} + +function buildUniqueLabel(preferred: string, used = new Set()): string { + const normalized = normalizePath(preferred); + if (!used.has(normalized)) { + used.add(normalized); + return normalized; + } + + let counter = 2; + let next = `${normalized}-${counter}`; + while (used.has(next)) { + counter += 1; + next = `${normalized}-${counter}`; + } + used.add(next); + return next; +} + +function withTimeout(promise: Promise, ms: number, context: string): Promise { + return Promise.race([ + promise.then((v) => v as T | null), + new Promise((resolve) => setTimeout(() => { + console.error(`[workspace] Timeout: ${context}`); + resolve(null); + }, ms)), + ]); +} + +async function discoverGitHubPRCandidate(cwd: string, gitContext: GitContext): Promise { + const branch = gitContext.currentBranch; + if (!branch || branch === "HEAD") return null; + + const remoteProc = Bun.spawn(["git", "remote", "get-url", "origin"], { + cwd, + stdout: "pipe", + stderr: "pipe", + }); + const [remoteUrl, remoteCode] = await Promise.all([ + new Response(remoteProc.stdout).text(), + remoteProc.exited, + ]); + if (remoteCode !== 0) return null; + + const repo = parseRemoteUrl(remoteUrl.trim()); + if (!repo) return null; + + const hostMatch = remoteUrl.trim().match(/^[^@]+@([^:]+):/)?.[1]; + const httpsHost = (() => { + try { + return new URL(remoteUrl.trim()).hostname; + } catch { + return null; + } + })(); + const host = hostMatch || httpsHost || "github.com"; + + const ghArgs = [ + "pr", + "list", + "--state", + "open", + "--head", + branch, + "--json", + "url", + "--limit", + "1", + ]; + const env = host === "github.com" ? process.env : { ...process.env, GH_HOST: host }; + let proc: ReturnType; + try { + proc = Bun.spawn(["gh", ...ghArgs], { + cwd, + env, + stdout: "pipe", + stderr: "ignore", + }); + } catch { + return null; + } + const [stdout, exitCode] = await Promise.all([ + new Response(proc.stdout).text(), + proc.exited, + ]); + if (exitCode !== 0) return null; + + let entries: Array<{ url?: string }>; + try { + entries = JSON.parse(stdout); + } catch { + return null; + } + + const url = entries[0]?.url; + if (!url) return null; + const ref = parsePRUrl(url); + if (!ref) return null; + + try { + const pr = await withTimeout(fetchPR(ref), 15000, `fetchPR ${url}`); + if (!pr) return null; + return { url, metadata: pr.metadata }; + } catch { + return null; + } +} + +async function discoverGitLabPRCandidate(cwd: string, gitContext: GitContext): Promise { + const branch = gitContext.currentBranch; + if (!branch || branch === "HEAD") return null; + + let proc: ReturnType; + try { + proc = Bun.spawn([ + "glab", + "mr", + "list", + "--source-branch", + branch, + "--state", + "opened", + "--output", + "json", + ], { + cwd, + stdout: "pipe", + stderr: "ignore", + }); + } catch { + return null; + } + const [stdout, exitCode] = await Promise.all([ + new Response(proc.stdout).text(), + proc.exited, + ]); + if (exitCode !== 0) return null; + + let entries: Array<{ web_url?: string }>; + try { + entries = JSON.parse(stdout); + } catch { + return null; + } + + const url = entries[0]?.web_url; + if (!url) return null; + const ref = parsePRUrl(url); + if (!ref) return null; + + try { + const pr = await withTimeout(fetchPR(ref), 15000, `fetchPR ${url}`); + if (!pr) return null; + return { url, metadata: pr.metadata }; + } catch { + return null; + } +} + +export async function discoverPRCandidates(cwd: string, gitContext: GitContext): Promise { + const candidates = await Promise.all([ + discoverGitHubPRCandidate(cwd, gitContext), + discoverGitLabPRCandidate(cwd, gitContext), + ]); + + return candidates.filter((candidate): candidate is WorkspacePRCandidate => !!candidate); +} + +export async function buildWorkspaceLocalRepos(root: string): Promise { + const repoPaths = discoverWorkspaceRepoPaths(root); + const defaultDiffType = resolveDefaultDiffType(loadConfig()); + + // Pre-compute labels sequentially to avoid race conditions in parallel map + const usedLabels = new Set(); + const labels = repoPaths.map((cwd) => buildRepoLabel(root, cwd, usedLabels)); + + const repos = await Promise.all(repoPaths.map(async (cwd, index) => { + const label = labels[index]; + try { + const gitContext = await getVcsContext(cwd); + const diffType = gitContext.vcsType === "p4" ? "p4-default" : defaultDiffType; + const diffResult = await runVcsDiff(diffType, gitContext.defaultBranch, cwd); + const discoveredPRs = await discoverPRCandidates(cwd, gitContext); + return { + id: `repo-${index + 1}`, + label, + cwd, + selected: !!diffResult.patch.trim(), + source: "local" as WorkspaceRepoSource, + diffType, + gitContext, + diffOptions: gitContext.diffOptions, + discoveredPRs, + rawPatch: prefixPatchPaths(diffResult.patch, label), + gitRef: diffResult.label, + error: diffResult.error, + } satisfies WorkspaceRepoRuntimeState; + } catch (error) { + return { + id: `repo-${index + 1}`, + label, + cwd, + selected: false, + source: "local" as WorkspaceRepoSource, + rawPatch: "", + gitRef: "", + error: error instanceof Error ? error.message : String(error), + } satisfies WorkspaceRepoRuntimeState; + } + })); + + return repos; +} + +export async function buildWorkspacePRRepos(urls: string[]): Promise { + // Pre-compute labels sequentially to avoid race conditions in parallel map + const usedLabels = new Set(); + const prefetched = await Promise.all(urls.map(async (url) => { + const ref = parsePRUrl(url); + if (!ref) return null; + try { + const pr = await fetchPR(ref); + const baseLabel = pr.metadata.platform === "github" + ? `${pr.metadata.owner}/${pr.metadata.repo}` + : pr.metadata.projectPath; + return { pr, baseLabel }; + } catch { + return null; + } + })); + + const labels = prefetched.map((p) => + p ? buildUniqueLabel(p.baseLabel, usedLabels) : buildUniqueLabel(`invalid-${prefetched.indexOf(p) + 1}`, usedLabels) + ); + + const repos = await Promise.all(urls.map(async (url, index) => { + const ref = parsePRUrl(url); + if (!ref) { + return { + id: `repo-${index + 1}`, + label: `invalid-${index + 1}`, + cwd: process.cwd(), + selected: false, + source: "pr" as WorkspaceRepoSource, + rawPatch: "", + gitRef: "", + error: `Invalid PR/MR URL: ${url}`, + } satisfies WorkspaceRepoRuntimeState; + } + + try { + const pr = await fetchPR(ref); + const baseLabel = pr.metadata.platform === "github" + ? `${pr.metadata.owner}/${pr.metadata.repo}` + : pr.metadata.projectPath; + const label = buildUniqueLabel(baseLabel, usedLabels); + return { + id: `repo-${index + 1}`, + label, + cwd: process.cwd(), + selected: true, + source: "pr" as WorkspaceRepoSource, + prMetadata: pr.metadata, + rawPatch: prefixPatchPaths(pr.rawPatch, label), + gitRef: pr.metadata.url, + } satisfies WorkspaceRepoRuntimeState; + } catch (error) { + return { + id: `repo-${index + 1}`, + label: `pr-${index + 1}`, + cwd: process.cwd(), + selected: false, + source: "pr" as WorkspaceRepoSource, + rawPatch: "", + gitRef: "", + error: error instanceof Error ? error.message : String(error), + } satisfies WorkspaceRepoRuntimeState; + } + })); + + return repos; +} + +export interface WorkspacePatchAggregate { + rawPatch: string; + gitRef: string; + errors: string[]; +} + +export function aggregateWorkspacePatch(repos: WorkspaceRepoRuntimeState[]): WorkspacePatchAggregate { + const selected = repos.filter((repo) => repo.selected); + const trimmedPatches = selected.map((repo) => repo.rawPatch.trim()).filter(Boolean); + return { + rawPatch: trimmedPatches.join("\n\n"), + gitRef: selected.map((repo) => repo.gitRef || repo.label).filter(Boolean).join(" | ") || "Workspace review", + errors: selected.flatMap((repo) => repo.error ? [`${repo.label}: ${repo.error}`] : []), + }; +} diff --git a/packages/server/shared-handlers.ts b/packages/server/shared-handlers.ts index 83675d3d5..941e93a95 100644 --- a/packages/server/shared-handlers.ts +++ b/packages/server/shared-handlers.ts @@ -141,5 +141,7 @@ export async function handleServerReady( isRemote: boolean, _port: number, ): Promise { - await openBrowser(url, { isRemote }); + const freshUrl = new URL(url); + freshUrl.searchParams.set("session", `${Date.now()}-${Math.random().toString(36).slice(2, 8)}`); + await openBrowser(freshUrl.toString(), { isRemote }); } diff --git a/packages/server/vcs.ts b/packages/server/vcs.ts index 08871fd3d..e5b16d14a 100644 --- a/packages/server/vcs.ts +++ b/packages/server/vcs.ts @@ -77,3 +77,5 @@ export { parseWorktreeDiffType, validateFilePath, } from "@plannotator/shared/vcs-core"; + + diff --git a/packages/shared/agent-jobs.ts b/packages/shared/agent-jobs.ts index 1cfedf391..993f10519 100644 --- a/packages/shared/agent-jobs.ts +++ b/packages/shared/agent-jobs.ts @@ -75,6 +75,10 @@ export interface AgentJobInfo { diffScope?: string; /** Diff context at launch time (see AgentJobDiffContext). */ diffContext?: AgentJobDiffContext; + /** Workspace repo snapshot at launch — prevents race with UI selection changes. */ + repoId?: string; + repoLabel?: string; + repoCwd?: string; } export interface AgentCapability { diff --git a/packages/shared/package.json b/packages/shared/package.json index a36fae68e..73ce04786 100644 --- a/packages/shared/package.json +++ b/packages/shared/package.json @@ -9,6 +9,7 @@ "./feedback-templates": "./feedback-templates.ts", "./jj-core": "./jj-core.ts", "./review-core": "./review-core.ts", + "./review-workspace": "./review-workspace.ts", "./review-args": "./review-args.ts", "./vcs-core": "./vcs-core.ts", "./checklist": "./checklist.ts", diff --git a/packages/shared/review-workspace.ts b/packages/shared/review-workspace.ts new file mode 100644 index 000000000..ffe2322b1 --- /dev/null +++ b/packages/shared/review-workspace.ts @@ -0,0 +1,30 @@ +import type { DiffType, DiffOption, GitContext } from "./review-core"; +import type { PRMetadata } from "./pr-provider"; + +export type WorkspaceRepoSource = "local" | "pr"; + +export interface WorkspacePRCandidate { + url: string; + metadata: PRMetadata; +} + +export interface WorkspaceRepoState { + id: string; + label: string; + cwd: string; + selected: boolean; + source: WorkspaceRepoSource; + diffType?: DiffType; + gitContext?: GitContext; + prMetadata?: PRMetadata; + discoveredPRs?: WorkspacePRCandidate[]; + diffOptions?: DiffOption[]; + platformUser: string | null; + viewedFiles?: string[]; + error?: string; +} + +export interface WorkspaceReviewState { + mode: "workspace"; + repos: WorkspaceRepoState[]; +} diff --git a/packages/shared/types.ts b/packages/shared/types.ts index d249ee62e..fea6ecef8 100644 --- a/packages/shared/types.ts +++ b/packages/shared/types.ts @@ -19,3 +19,10 @@ export type { CompareTargetPickerCopy, RepositoryContext, } from "./review-core"; + +export type { + WorkspaceRepoSource, + WorkspacePRCandidate, + WorkspaceRepoState, + WorkspaceReviewState, +} from "./review-workspace";