diff --git a/apps/hook/server/index.ts b/apps/hook/server/index.ts index 7572d43c..389cb05a 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, getVcsContext, runVcsDiff, gitRuntime } from "@plannotator/server/vcs"; +import { type DiffType, getVcsContext, runVcsDiff, gitRuntime, detectManagedVcs } from "@plannotator/server/vcs"; import { loadConfig, resolveDefaultDiffType } from "@plannotator/shared/config"; import { fetchRef, createWorktree, removeWorktree, ensureObjectAvailable } from "@plannotator/shared/worktree"; import { parsePRUrl, checkPRAuth, fetchPR, getCliName, getCliInstallUrl, getMRLabel, getMRNumberLabel, getDisplayRepo } from "@plannotator/server/pr"; @@ -89,6 +89,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 @@ -196,8 +197,10 @@ if (args[0] === "sessions") { const noLocalIdx = args.indexOf("--no-local"); if (noLocalIdx !== -1) args.splice(noLocalIdx, 1); - const urlArg = args[1]; - const isPRMode = urlArg?.startsWith("http://") || urlArg?.startsWith("https://"); + const urlArgs = args.slice(1).filter((arg) => arg.startsWith("http://") || arg.startsWith("https://")); + const urlArg = urlArgs[0]; + const isPRMode = urlArgs.length > 0; + const isMultiPRMode = urlArgs.length > 1; const useLocal = isPRMode && noLocalIdx === -1; let rawPatch: string; @@ -208,8 +211,13 @@ if (args[0] === "sessions") { let initialDiffType: DiffType | undefined; let agentCwd: string | undefined; let worktreeCleanup: (() => void | Promise) | undefined; + let workspaceRepos: Awaited> | undefined; - if (isPRMode) { + if (isMultiPRMode) { + workspaceRepos = await buildWorkspacePRRepos(urlArgs); + rawPatch = ""; + gitRef = "Workspace review"; + } else if (isPRMode) { // --- PR Review Mode --- const prRef = parsePRUrl(urlArg); if (!prRef) { @@ -379,12 +387,22 @@ if (args[0] === "sessions") { } } else { // --- Local Review Mode --- - gitContext = await getVcsContext(); - initialDiffType = gitContext.vcsType === "p4" ? "p4-default" : resolveDefaultDiffType(loadConfig()); - const diffResult = await runVcsDiff(initialDiffType, gitContext.defaultBranch); - rawPatch = diffResult.patch; - gitRef = diffResult.label; - diffError = diffResult.error; + const managedVcs = await detectManagedVcs(process.cwd()); + if (managedVcs) { + gitContext = await getVcsContext(); + initialDiffType = gitContext.vcsType === "p4" ? "p4-default" : resolveDefaultDiffType(loadConfig()); + const diffResult = await runVcsDiff(initialDiffType, gitContext.defaultBranch); + rawPatch = diffResult.patch; + gitRef = diffResult.label; + diffError = diffResult.error; + } else { + 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"; @@ -398,6 +416,7 @@ if (args[0] === "sessions") { diffType: gitContext ? (initialDiffType ?? "unstaged") : undefined, gitContext, prMetadata, + workspaceRepos, agentCwd, sharingEnabled, shareBaseUrl, diff --git a/apps/opencode-plugin/commands.ts b/apps/opencode-plugin/commands.ts index 523f4723..2871f73f 100644 --- a/apps/opencode-plugin/commands.ts +++ b/apps/opencode-plugin/commands.ts @@ -19,9 +19,11 @@ import { handleAnnotateServerReady, } from "@plannotator/server/annotate"; import { getGitContext, runGitDiffWithContext } from "@plannotator/server/git"; +import { detectManagedVcs } from "@plannotator/server/vcs"; import { parsePRUrl, checkPRAuth, fetchPR, getCliName, getMRLabel, getMRNumberLabel, getDisplayRepo } from "@plannotator/server/pr"; import { loadConfig, resolveDefaultDiffType } from "@plannotator/shared/config"; import { resolveMarkdownFile } from "@plannotator/shared/resolve-file"; +import { buildWorkspaceLocalRepos, buildWorkspacePRRepos } from "@plannotator/server/review-workspace"; /** Shared dependencies injected by the plugin */ export interface CommandDeps { @@ -41,7 +43,9 @@ export async function handleReviewCommand( // @ts-ignore - Event properties contain arguments const urlArg: string = event.properties?.arguments || ""; - const isPRMode = urlArg?.startsWith("http://") || urlArg?.startsWith("https://"); + const urlArgs = urlArg.split(/\s+/).filter((arg: string) => arg.startsWith("http://") || arg.startsWith("https://")); + const isPRMode = urlArgs.length > 0; + const isMultiPRMode = urlArgs.length > 1; let rawPatch: string; let gitRef: string; @@ -49,11 +53,16 @@ export async function handleReviewCommand( let userDiffType: import("@plannotator/shared/config").DefaultDiffType | undefined; let gitContext: Awaited> | undefined; let prMetadata: Awaited>["metadata"] | undefined; - - if (isPRMode) { - const prRef = parsePRUrl(urlArg); + let workspaceRepos: Awaited> | undefined; + + if (isMultiPRMode) { + workspaceRepos = await buildWorkspacePRRepos(urlArgs); + rawPatch = ""; + gitRef = "Workspace review"; + } else if (isPRMode) { + const prRef = parsePRUrl(urlArgs[0]); if (!prRef) { - client.app.log({ level: "error", message: `Invalid PR/MR URL: ${urlArg}` }); + client.app.log({ level: "error", message: `Invalid PR/MR URL: ${urlArgs[0]}` }); return; } @@ -79,12 +88,27 @@ export async function handleReviewCommand( } else { client.app.log({ level: "info", message: "Opening code review UI..." }); - gitContext = await getGitContext(directory); - userDiffType = resolveDefaultDiffType(loadConfig()); - const diffResult = await runGitDiffWithContext(userDiffType, gitContext); - rawPatch = diffResult.patch; - gitRef = diffResult.label; - diffError = diffResult.error; + const managedVcs = await detectManagedVcs(directory); + if (managedVcs) { + gitContext = await getGitContext(directory); + userDiffType = resolveDefaultDiffType(loadConfig()); + const diffResult = await runGitDiffWithContext(userDiffType, gitContext); + rawPatch = diffResult.patch; + gitRef = diffResult.label; + diffError = diffResult.error; + } else { + workspaceRepos = await buildWorkspaceLocalRepos(directory || process.cwd()); + if (workspaceRepos.length === 0) { + client.app.log({ level: "error", message: "Not in a git repo and no nested repositories were found." }); + return; + } + client.app.log({ + level: "info", + message: `Workspace mode: found ${workspaceRepos.length} repos (${workspaceRepos.filter((repo) => repo.selected).length} selected with changes).`, + }); + rawPatch = ""; + gitRef = "Workspace review"; + } } const server = await startReviewServer({ @@ -95,6 +119,7 @@ export async function handleReviewCommand( diffType: isPRMode ? undefined : userDiffType, gitContext, prMetadata, + workspaceRepos, sharingEnabled: await getSharingEnabled(), shareBaseUrl: getShareBaseUrl(), htmlContent: reviewHtmlContent, diff --git a/bun.lock b/bun.lock index 786f3433..683bedae 100644 --- a/bun.lock +++ b/bun.lock @@ -63,7 +63,7 @@ }, "apps/opencode-plugin": { "name": "@plannotator/opencode", - "version": "0.17.7", + "version": "0.17.8", "dependencies": { "@opencode-ai/plugin": "^1.1.10", }, @@ -85,7 +85,7 @@ }, "apps/pi-extension": { "name": "@plannotator/pi-extension", - "version": "0.17.7", + "version": "0.17.8", "peerDependencies": { "@mariozechner/pi-coding-agent": ">=0.53.0", }, @@ -171,7 +171,7 @@ }, "packages/server": { "name": "@plannotator/server", - "version": "0.17.7", + "version": "0.17.8", "dependencies": { "@plannotator/ai": "workspace:*", "@plannotator/shared": "workspace:*", diff --git a/packages/review-editor/App.tsx b/packages/review-editor/App.tsx index 86205844..ccafaaba 100644 --- a/packages/review-editor/App.tsx +++ b/packages/review-editor/App.tsx @@ -58,6 +58,7 @@ import { import type { DiffFile } from './types'; import type { DiffOption, WorktreeInfo, GitContext } from '@plannotator/shared/types'; import type { PRMetadata } from '@plannotator/shared/pr-provider'; +import type { WorkspaceReviewState, WorkspaceRepoState } from '@plannotator/shared/review-workspace'; import { altKey } from '@plannotator/ui/utils/platform'; declare const __APP_VERSION__: string; @@ -167,12 +168,22 @@ const ReviewApp: React.FC = () => { const [showExitWarning, setShowExitWarning] = useState(false); const [sharingEnabled, setSharingEnabled] = useState(true); const [repoInfo, setRepoInfo] = useState<{ display: string; branch?: string } | null>(null); + const [workspace, setWorkspace] = useState(null); useEffect(() => { document.title = repoInfo ? `${repoInfo.display} · Code Review` : "Code Review"; }, [repoInfo]); - const [prMetadata, setPrMetadata] = useState(null); + useEffect(() => { + if (!workspace) return; + const selectedCount = workspace.repos.filter(repo => repo.selected).length; + setRepoInfo({ + display: selectedCount > 0 ? `${selectedCount} selected repos` : 'Workspace Review', + branch: 'Workspace', + }); + }, [workspace]); + + const [singularPrMetadata, setSingularPrMetadata] = useState(null); const [reviewDestination, setReviewDestination] = useState<'agent' | 'platform'>(() => { const stored = storage.getItem('plannotator-review-dest'); return stored === 'agent' ? 'agent' : 'platform'; // 'github' (legacy) → 'platform' @@ -180,7 +191,7 @@ const ReviewApp: React.FC = () => { const [showDestinationMenu, setShowDestinationMenu] = useState(false); const [isPlatformActioning, setIsPlatformActioning] = useState(false); const [platformActionError, setPlatformActionError] = useState(null); - const [platformUser, setPlatformUser] = useState(null); + const [singularPlatformUser, setSingularPlatformUser] = useState(null); const [platformCommentDialog, setPlatformCommentDialog] = useState<{ action: 'approve' | 'comment' } | null>(null); const [platformGeneralComment, setPlatformGeneralComment] = useState(''); const [platformOpenPR, setPlatformOpenPR] = useState(() => { @@ -197,6 +208,22 @@ const ReviewApp: React.FC = () => { }); // Derived: Platform mode is active when destination is platform AND we have PR/MR metadata + const findWorkspaceRepoForPath = useCallback((filePath?: string | null): WorkspaceRepoState | null => { + if (!workspace || !filePath) return null; + return workspace.repos.find(repo => filePath === repo.label || filePath.startsWith(`${repo.label}/`)) ?? null; + }, [workspace]); + + const activeWorkspaceRepo = useMemo(() => { + if (!workspace) return null; + return findWorkspaceRepoForPath(files[activeFileIndex]?.path) + ?? workspace.repos.find(repo => repo.selected) + ?? workspace.repos[0] + ?? null; + }, [workspace, files, activeFileIndex, findWorkspaceRepoForPath]); + + const prMetadata = workspace ? activeWorkspaceRepo?.prMetadata ?? null : singularPrMetadata; + const platformUser = workspace ? activeWorkspaceRepo?.platformUser ?? null : singularPlatformUser; + const platformMode = reviewDestination === 'platform' && !!prMetadata; // Platform-aware labels @@ -231,7 +258,16 @@ const ReviewApp: React.FC = () => { const needsInitialDiffPanel = useRef(true); // PR context (lifted from sidebar so center dock PR panels can access it) - const { prContext, isLoading: isPRContextLoading, error: prContextError, fetchContext: fetchPRContext } = usePRContext(prMetadata ?? null); + const { prContext, isLoading: isPRContextLoading, error: prContextError, fetchContext: fetchPRContext } = usePRContext(prMetadata ?? null, workspace ? activeWorkspaceRepo?.id ?? null : null); + + useEffect(() => { + if (!workspace || !activeWorkspaceRepo) return; + fetch('/api/workspace/active', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ repoId: activeWorkspaceRepo.id }), + }).catch(() => {}); + }, [workspace, activeWorkspaceRepo]); // Sync activeFileIndex from dockview's active panel (wired in handleDockReady) @@ -635,6 +671,7 @@ const ReviewApp: React.FC = () => { prMetadata?: PRMetadata; platformUser?: string; viewedFiles?: string[]; + workspace?: WorkspaceReviewState; error?: string; isWSL?: boolean; serverConfig?: { displayName?: string; gitUser?: string }; @@ -660,10 +697,17 @@ const ReviewApp: React.FC = () => { if (data.agentCwd) setAgentCwd(data.agentCwd); if (data.sharingEnabled !== undefined) setSharingEnabled(data.sharingEnabled); if (data.repoInfo) setRepoInfo(data.repoInfo); - if (data.prMetadata) setPrMetadata(data.prMetadata); - if (data.platformUser) setPlatformUser(data.platformUser); + if (data.workspace) { + setWorkspace(data.workspace); + const workspaceViewed = data.workspace.repos.flatMap(repo => repo.viewedFiles ?? []); + if (workspaceViewed.length > 0) { + setViewedFiles(new Set(workspaceViewed)); + } + } + if (data.prMetadata) setSingularPrMetadata(data.prMetadata); + if (data.platformUser) setSingularPlatformUser(data.platformUser); // Initialize viewed files from GitHub's state (set before draft restore so draft takes precedence) - if (data.viewedFiles && data.viewedFiles.length > 0) { + if (!data.workspace && data.viewedFiles && data.viewedFiles.length > 0) { setViewedFiles(new Set(data.viewedFiles)); } if (data.error) setDiffError(data.error); @@ -846,7 +890,15 @@ const ReviewApp: React.FC = () => { } // Sync viewed state to GitHub (fire and forget — best effort) // Capture willBeViewed inside the callback to ensure correctness with React batching - if (prMetadata && prMetadata.platform === 'github') { + if (workspace && activeWorkspaceRepo?.prMetadata?.platform === 'github') { + fetch('/api/pr-viewed', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ repoId: activeWorkspaceRepo.id, filePaths: [filePath], viewed: willBeViewed }), + }).catch(() => { + // Silently ignore — viewed sync is best-effort + }); + } else if (prMetadata && prMetadata.platform === 'github') { fetch('/api/pr-viewed', { method: 'POST', headers: { 'Content-Type': 'application/json' }, @@ -857,12 +909,14 @@ const ReviewApp: React.FC = () => { } return next; }); - }, [prMetadata]); + }, [workspace, activeWorkspaceRepo, prMetadata]); // Derive worktree path and base diff type from the composite diffType string + const effectiveDiffType = workspace ? (activeWorkspaceRepo?.diffType || 'uncommitted') : diffType; + const { activeWorktreePath, activeDiffBase } = useMemo(() => { - if (diffType.startsWith('worktree:')) { - const rest = diffType.slice('worktree:'.length); + if (effectiveDiffType.startsWith('worktree:')) { + const rest = effectiveDiffType.slice('worktree:'.length); const lastColon = rest.lastIndexOf(':'); if (lastColon !== -1) { const sub = rest.slice(lastColon + 1); @@ -872,8 +926,8 @@ const ReviewApp: React.FC = () => { } return { activeWorktreePath: rest, activeDiffBase: 'uncommitted' }; } - return { activeWorktreePath: null, activeDiffBase: diffType }; - }, [diffType]); + return { activeWorktreePath: null, activeDiffBase: effectiveDiffType }; + }, [effectiveDiffType]); // Git add/staging logic const handleFileViewedFromStage = useCallback( @@ -884,17 +938,39 @@ const ReviewApp: React.FC = () => { activeDiffBase, onFileViewed: handleFileViewedFromStage, }); - // Staging is never available in PR review mode — the server rejects it and the UI shouldn't offer it. - const canStageFiles = canStageRaw && !prMetadata; + const canStageFiles = canStageRaw && !(workspace ? activeWorkspaceRepo?.source === 'pr' : prMetadata); + + const applyServerDiffPayload = useCallback((data: { + rawPatch: string; + gitRef: string; + diffType?: string; + workspace?: WorkspaceReviewState; + error?: string; + }) => { + const nextFiles = parseDiffToFiles(data.rawPatch); + dockApi?.getPanel(REVIEW_DIFF_PANEL_ID)?.api.close(); + needsInitialDiffPanel.current = true; + setDiffData(prev => prev ? { ...prev, rawPatch: data.rawPatch, gitRef: data.gitRef, diffType: data.diffType } : prev); + setFiles(nextFiles); + if (data.diffType) setDiffType(data.diffType); + if (data.workspace) { + setWorkspace(data.workspace); + setViewedFiles(new Set(data.workspace.repos.flatMap(repo => repo.viewedFiles ?? []))); + } + setActiveFileIndex(0); + setPendingSelection(null); + setDiffError(data.error || null); + resetStagedFiles(); + }, [dockApi, resetStagedFiles]); // Shared helper: fetch a diff switch and update state - const fetchDiffSwitch = useCallback(async (fullDiffType: string) => { + const fetchDiffSwitch = useCallback(async (fullDiffType: string, repoId?: string) => { setIsLoadingDiff(true); try { const res = await fetch('/api/diff/switch', { method: 'POST', headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ diffType: fullDiffType }), + body: JSON.stringify(repoId ? { repoId, diffType: fullDiffType } : { diffType: fullDiffType }), }); if (!res.ok) throw new Error('Failed to switch diff'); @@ -902,45 +978,66 @@ const ReviewApp: React.FC = () => { const data = await res.json() as { rawPatch: string; gitRef: string; - diffType: string; + diffType?: string; + workspace?: WorkspaceReviewState; error?: string; }; - const nextFiles = parseDiffToFiles(data.rawPatch); - dockApi?.getPanel(REVIEW_DIFF_PANEL_ID)?.api.close(); - needsInitialDiffPanel.current = true; - setDiffData(prev => prev ? { ...prev, rawPatch: data.rawPatch, gitRef: data.gitRef, diffType: data.diffType } : prev); - setFiles(nextFiles); - setDiffType(data.diffType); - setActiveFileIndex(0); - setPendingSelection(null); - setDiffError(data.error || null); - resetStagedFiles(); + applyServerDiffPayload(data); } catch (err) { console.error('Failed to switch diff:', err); setDiffError(err instanceof Error ? err.message : 'Failed to switch diff'); } finally { setIsLoadingDiff(false); } - }, [dockApi, resetStagedFiles]); + }, [applyServerDiffPayload]); + + const updateWorkspaceRepo = useCallback(async (repoId: string, changes: { selected?: boolean; source?: 'local' | 'pr'; prUrl?: string }) => { + setIsLoadingDiff(true); + try { + const res = await fetch('/api/workspace/repo', { + method: 'PATCH', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ repoId, ...changes }), + }); + if (!res.ok) throw new Error('Failed to update workspace repo'); + const data = await res.json() as { + rawPatch: string; + gitRef: string; + workspace?: WorkspaceReviewState; + error?: string; + }; + applyServerDiffPayload(data); + } catch (err) { + console.error('Failed to update workspace repo:', err); + setDiffError(err instanceof Error ? err.message : 'Failed to update workspace repo'); + } finally { + setIsLoadingDiff(false); + } + }, [applyServerDiffPayload]); // Switch diff type (uncommitted, last-commit, branch) — composes worktree prefix if active const handleDiffSwitch = useCallback(async (baseDiffType: string) => { + if (workspace && activeWorkspaceRepo) { + await fetchDiffSwitch(baseDiffType, activeWorkspaceRepo.id); + return; + } const fullDiffType = activeWorktreePath ? `worktree:${activeWorktreePath}:${baseDiffType}` : baseDiffType; if (fullDiffType === diffType) return; await fetchDiffSwitch(fullDiffType); - }, [diffType, activeWorktreePath, fetchDiffSwitch]); + }, [workspace, activeWorkspaceRepo, diffType, activeWorktreePath, fetchDiffSwitch]); // Switch worktree context (or back to main repo) const handleWorktreeSwitch = useCallback(async (worktreePath: string | null) => { + if (workspace) return; if (worktreePath === activeWorktreePath) return; const fullDiffType = worktreePath ? `worktree:${worktreePath}:uncommitted` : 'uncommitted'; await fetchDiffSwitch(fullDiffType); - }, [activeWorktreePath, fetchDiffSwitch]); + }, [workspace, activeWorktreePath, fetchDiffSwitch]); // Select annotation - switches file if needed and scrolls to it const handleSelectAnnotation = useCallback((id: string | null) => { @@ -1056,7 +1153,7 @@ const ReviewApp: React.FC = () => { return; } try { - const feedback = exportReviewFeedback(allAnnotations, prMetadata); + const feedback = exportReviewFeedback(allAnnotations, workspace ? undefined : prMetadata); await navigator.clipboard.writeText(feedback); setCopyFeedback('Feedback copied!'); setTimeout(() => setCopyFeedback(null), 2000); @@ -1065,15 +1162,15 @@ const ReviewApp: React.FC = () => { setCopyFeedback('Failed to copy'); setTimeout(() => setCopyFeedback(null), 2000); } - }, [allAnnotations, prMetadata]); + }, [workspace, allAnnotations, prMetadata]); const feedbackMarkdown = useMemo(() => { - let output = exportReviewFeedback(allAnnotations, prMetadata); + let output = exportReviewFeedback(allAnnotations, workspace ? undefined : prMetadata); if (editorAnnotations.length > 0) { output += exportEditorAnnotations(editorAnnotations); } return output; - }, [allAnnotations, prMetadata, editorAnnotations]); + }, [workspace, allAnnotations, prMetadata, editorAnnotations]); const totalAnnotationCount = allAnnotations.length + editorAnnotations.length; @@ -1155,8 +1252,18 @@ const ReviewApp: React.FC = () => { // Build the payload for /api/pr-action from current annotations const buildPRReviewPayload = useCallback((action: 'approve' | 'comment', generalComment?: string) => { - const fileAnnotations = allAnnotations.filter(a => (a.scope ?? 'line') === 'line'); - const fileScoped = allAnnotations.filter(a => a.scope === 'file'); + const repoPrefix = workspace && activeWorkspaceRepo ? `${activeWorkspaceRepo.label}/` : null; + const scopedAnnotations = repoPrefix + ? allAnnotations.filter(annotation => annotation.filePath.startsWith(repoPrefix)) + : allAnnotations; + const scopedEditorAnnotations = repoPrefix + ? editorAnnotations.filter(annotation => annotation.filePath.startsWith(repoPrefix)) + : editorAnnotations; + const scopedFiles = repoPrefix + ? files.filter(file => file.path.startsWith(repoPrefix)) + : files; + const fileAnnotations = scopedAnnotations.filter(a => (a.scope ?? 'line') === 'line'); + const fileScoped = scopedAnnotations.filter(a => a.scope === 'file'); // Top-level body: file-scoped comments const bodyParts: string[] = []; @@ -1192,8 +1299,8 @@ const ReviewApp: React.FC = () => { // Editor annotations (VS Code extension) — always on new/RIGHT side // Only include annotations targeting files in the diff to avoid GitHub API rejection - const diffPaths = new Set(files.map(f => f.path)); - for (const ea of editorAnnotations) { + const diffPaths = new Set(scopedFiles.map(f => f.path)); + for (const ea of scopedEditorAnnotations) { if (!diffPaths.has(ea.filePath)) continue; const body = ea.comment || `> ${ea.selectedText}`; if (!body.trim()) continue; @@ -1210,8 +1317,13 @@ const ReviewApp: React.FC = () => { }); } - return { action, body, fileComments }; - }, [allAnnotations, editorAnnotations, files]); + return { + ...(workspace && activeWorkspaceRepo && { repoId: activeWorkspaceRepo.id }), + action, + body, + fileComments, + }; + }, [workspace, activeWorkspaceRepo, allAnnotations, editorAnnotations, files]); // Submit a review directly to GitHub const handlePlatformAction = useCallback(async (action: 'approve' | 'comment', generalComment?: string) => { @@ -1637,6 +1749,81 @@ const ReviewApp: React.FC = () => { + {workspace && ( +
+
+ {workspace.repos.map((repo) => ( +
+
+ + {repo.error && ( + + issue + + )} +
+
+ + {repo.source === 'local' ? ( + + ) : ( + + )} +
+
+ ))} +
+
+ )} + {/* Main content */}
{/* Left sidebar stays mounted whenever it provides navigation or context. */} @@ -1653,15 +1840,15 @@ const ReviewApp: React.FC = () => { hideViewedFiles={hideViewedFiles} onToggleHideViewed={() => setHideViewedFiles(prev => !prev)} enableKeyboardNav={!showExportModal && hasSearchableFiles} - diffOptions={gitContext?.diffOptions} + diffOptions={workspace ? activeWorkspaceRepo?.gitContext?.diffOptions : gitContext?.diffOptions} activeDiffType={activeDiffBase} onSelectDiff={handleDiffSwitch} isLoadingDiff={isLoadingDiff} width={fileTreeResize.width} - worktrees={gitContext?.worktrees} + worktrees={workspace ? activeWorkspaceRepo?.gitContext?.worktrees : gitContext?.worktrees} activeWorktreePath={activeWorktreePath} onSelectWorktree={handleWorktreeSwitch} - currentBranch={gitContext?.currentBranch} + currentBranch={workspace ? activeWorkspaceRepo?.gitContext?.currentBranch : gitContext?.currentBranch} stagedFiles={stagedFiles} onCopyRawDiff={handleCopyDiff} canCopyRawDiff={!!diffData?.rawPatch} diff --git a/packages/review-editor/hooks/usePRContext.ts b/packages/review-editor/hooks/usePRContext.ts index c7fb098f..58a26369 100644 --- a/packages/review-editor/hooks/usePRContext.ts +++ b/packages/review-editor/hooks/usePRContext.ts @@ -1,13 +1,19 @@ -import { useState, useRef, useCallback } from 'react'; +import { useState, useRef, useCallback, useEffect } from 'react'; import type { PRContext } from '@plannotator/shared/pr-provider'; import type { PRMetadata } from '@plannotator/shared/pr-provider'; -export function usePRContext(prMetadata: PRMetadata | null) { +export function usePRContext(prMetadata: PRMetadata | null, repoId?: string | null) { const [prContext, setPRContext] = useState(null); const [isLoading, setIsLoading] = useState(false); const [error, setError] = useState(null); const fetched = useRef(false); + useEffect(() => { + fetched.current = false; + setPRContext(null); + setError(null); + }, [prMetadata, repoId]); + const fetchContext = useCallback(async () => { if (!prMetadata || fetched.current) return; fetched.current = true; @@ -15,7 +21,8 @@ export function usePRContext(prMetadata: PRMetadata | null) { setError(null); try { - const res = await fetch('/api/pr-context'); + const suffix = repoId ? `?repoId=${encodeURIComponent(repoId)}` : ''; + const res = await fetch(`/api/pr-context${suffix}`); if (!res.ok) { const data = await res.json().catch(() => ({})); throw new Error(data.error || `HTTP ${res.status}`); @@ -29,7 +36,7 @@ export function usePRContext(prMetadata: PRMetadata | null) { } finally { setIsLoading(false); } - }, [prMetadata]); + }, [prMetadata, repoId]); return { prContext, isLoading, error, fetchContext }; } 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 00000000..c3d22c3e --- /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 00000000..0a57ef16 --- /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 00000000..400727cc --- /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 55dd167f..3b50e86d 100644 --- a/packages/server/claude-review.ts +++ b/packages/server/claude-review.ts @@ -277,6 +277,7 @@ export function transformClaudeFindings( findings: ClaudeFinding[], source: string, cwd?: string, + pathTransform?: (path: string) => string, ): Array<{ source: string; filePath: string; @@ -294,7 +295,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 62abef7e..8dcf7ee4 100644 --- a/packages/server/codex-review.ts +++ b/packages/server/codex-review.ts @@ -347,6 +347,7 @@ export function transformReviewFindings( source: string, cwd?: string, author?: string, + pathTransform?: (path: string) => string, ): ReviewAnnotationInput[] { const annotations = findings .filter((f) => @@ -356,7 +357,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 b3c8c056..0a7f628d 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 00000000..c21ed18c --- /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 00000000..4ad49c82 --- /dev/null +++ b/packages/server/review-workspace.ts @@ -0,0 +1,404 @@ +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 { + if (line.startsWith("diff --git a/")) { + const match = line.match(/^diff --git a\/(.+) b\/(.+)$/); + if (!match) return line; + return `diff --git a/${prefixRepoPath(label, match[1])} b/${prefixRepoPath(label, match[2])}`; + } + + 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; + } + + 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 { + validateFilePath(prefixedPath); + + for (const repo of [...repos].sort((a, b) => b.label.length - a.label.length)) { + const prefix = `${normalizePath(repo.label)}/`; + if (prefixedPath.startsWith(prefix)) { + return { + repo, + repoRelativePath: prefixedPath.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; +} + +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 fetchPR(ref); + 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 fetchPR(ref); + 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()); + const usedLabels = new Set(); + + const repos = await Promise.all(repoPaths.map(async (cwd, index) => { + const label = buildRepoLabel(root, cwd, usedLabels); + 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 { + const usedLabels = new Set(); + 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 function aggregateWorkspacePatch(repos: WorkspaceRepoRuntimeState[]): { + rawPatch: string; + gitRef: string; + errors: string[]; +} { + const selected = repos.filter((repo) => repo.selected); + return { + rawPatch: selected.map((repo) => repo.rawPatch.trim()).filter(Boolean).join("\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/review.ts b/packages/server/review.ts index 4c785c8f..a9719333 100644 --- a/packages/server/review.ts +++ b/packages/server/review.ts @@ -36,6 +36,8 @@ import { saveConfig, detectGitUser, getServerConfig } from "./config"; import { type PRMetadata, type PRReviewFileComment, fetchPRFileContent, fetchPRContext, submitPRReview, fetchPRViewedFiles, markPRFilesViewed, getPRUser, prRefFromMetadata, getDisplayRepo, getMRLabel, getMRNumberLabel } from "./pr"; import { createAIEndpoints, ProviderRegistry, SessionManager, createProvider, type AIEndpoints, type PiSDKConfig } from "@plannotator/ai"; import { isWSL } from "./browser"; +import { aggregateWorkspacePatch, prefixPatchPaths, resolveWorkspaceFilePath, type WorkspaceRepoRuntimeState } from "./review-workspace"; +import type { WorkspaceReviewState } from "@plannotator/shared/review-workspace"; // Re-export utilities export { isRemoteSession, getServerPort } from "./remote"; @@ -75,6 +77,8 @@ export interface ReviewServerOptions { agentCwd?: string; /** Cleanup callback invoked when server stops (e.g., remove temp worktree) */ onCleanup?: () => void | Promise; + /** Workspace-mode repo state for multi-repo review */ + workspaceRepos?: WorkspaceRepoRuntimeState[]; } export interface ReviewServerResult { @@ -112,32 +116,66 @@ const RETRY_DELAY_MS = 500; export async function startReviewServer( options: ReviewServerOptions ): Promise { - const { htmlContent, origin, gitContext, sharingEnabled = true, shareBaseUrl, onReady, prMetadata } = options; + const { htmlContent, origin, gitContext, sharingEnabled = true, shareBaseUrl, onReady, prMetadata, workspaceRepos } = options; const isPRMode = !!prMetadata; + const isWorkspaceMode = !!workspaceRepos?.length; const hasLocalAccess = !!gitContext; const draftKey = contentHash(options.rawPatch); const editorAnnotations = createEditorAnnotationHandler(); const externalAnnotations = createExternalAnnotationHandler("review"); // Mutable state for diff switching - let currentPatch = options.rawPatch; - let currentGitRef = options.gitRef; + const initialWorkspaceSnapshot = workspaceRepos ? aggregateWorkspacePatch(workspaceRepos) : null; + let currentPatch = initialWorkspaceSnapshot?.rawPatch ?? options.rawPatch; + let currentGitRef = initialWorkspaceSnapshot?.gitRef ?? options.gitRef; let currentDiffType: DiffType = options.diffType || "uncommitted"; - let currentError = options.error; + let currentError = initialWorkspaceSnapshot?.errors.join("\n") || options.error; + let currentActiveRepoId = workspaceRepos?.find((repo) => repo.selected)?.id ?? workspaceRepos?.[0]?.id ?? null; + + const getWorkspaceState = (): WorkspaceReviewState | null => { + if (!workspaceRepos) return null; + return { + mode: "workspace", + repos: workspaceRepos.map(({ rawPatch: _rawPatch, gitRef: _gitRef, ...repo }) => repo), + }; + }; + + const refreshWorkspaceAggregate = () => { + if (!workspaceRepos) return; + const snapshot = aggregateWorkspacePatch(workspaceRepos); + currentPatch = snapshot.rawPatch; + currentGitRef = snapshot.gitRef; + currentError = snapshot.errors.length > 0 ? snapshot.errors.join("\n") : undefined; + }; + + const getActiveRepo = (): WorkspaceRepoRuntimeState | null => { + if (!workspaceRepos?.length) return null; + return workspaceRepos.find((repo) => repo.id === currentActiveRepoId) + ?? workspaceRepos.find((repo) => repo.selected) + ?? workspaceRepos[0] + ?? null; + }; + + const getRepoForFilePath = (filePath: string): { repo: WorkspaceRepoRuntimeState; repoRelativePath: string } | null => { + if (!workspaceRepos) return null; + return resolveWorkspaceFilePath(workspaceRepos, filePath); + }; // Agent jobs — background process manager (late-binds serverUrl via getter) let serverUrl = ""; const agentJobs = createAgentJobHandler({ mode: "review", getServerUrl: () => serverUrl, - getCwd: () => { - if (options.agentCwd) return options.agentCwd; - return resolveVcsCwd(currentDiffType, gitContext?.cwd) ?? process.cwd(); - }, + getCwd: () => { + const activeRepo = getActiveRepo(); + if (activeRepo) return activeRepo.cwd; + if (options.agentCwd) return options.agentCwd; + return resolveVcsCwd(currentDiffType, gitContext?.cwd) ?? process.cwd(); + }, async buildCommand(provider) { - const cwd = options.agentCwd ?? resolveVcsCwd(currentDiffType, gitContext?.cwd) ?? process.cwd(); + const cwd = getActiveRepo()?.cwd ?? options.agentCwd ?? resolveVcsCwd(currentDiffType, gitContext?.cwd) ?? process.cwd(); const hasAgentLocalAccess = !!options.agentCwd || !!gitContext; const userMessage = buildCodexReviewUserMessage( currentPatch, @@ -180,7 +218,14 @@ export async function startReviewServer( }; if (output.findings.length > 0) { - const annotations = transformReviewFindings(output.findings, job.source, cwd, "Codex"); + const activeRepo = getActiveRepo(); + const annotations = transformReviewFindings( + output.findings, + job.source, + cwd, + "Codex", + activeRepo ? (filePath) => `${activeRepo.label}/${filePath}` : undefined, + ); const result = externalAnnotations.addAnnotations({ annotations }); if ("error" in result) console.error(`[codex-review] addAnnotations error:`, result.error); } @@ -203,7 +248,13 @@ export async function startReviewServer( }; if (output.findings.length > 0) { - const annotations = transformClaudeFindings(output.findings, job.source, cwd); + const activeRepo = getActiveRepo(); + const annotations = transformClaudeFindings( + output.findings, + job.source, + cwd, + activeRepo ? (filePath) => `${activeRepo.label}/${filePath}` : undefined, + ); const result = externalAnnotations.addAnnotations({ annotations }); if ("error" in result) console.error(`[claude-review] addAnnotations error:`, result.error); } @@ -289,10 +340,12 @@ export async function startReviewServer( aiEndpoints = createAIEndpoints({ registry: aiRegistry, sessionManager: aiSessionManager, - getCwd: () => { - if (options.agentCwd) return options.agentCwd; - return resolveVcsCwd(currentDiffType, gitContext?.cwd) ?? process.cwd(); - }, + getCwd: () => { + const activeRepo = getActiveRepo(); + if (activeRepo) return activeRepo.cwd; + if (options.agentCwd) return options.agentCwd; + return resolveVcsCwd(currentDiffType, gitContext?.cwd) ?? process.cwd(); + }, }); } @@ -303,7 +356,9 @@ export async function startReviewServer( // Detect repo info (cached for this session) // In PR mode, derive from metadata instead of local git - const repoInfo = isPRMode + const repoInfo = isWorkspaceMode + ? { display: `${workspaceRepos?.filter((repo) => repo.selected).length ?? 0} repos`, branch: "Workspace Review" } + : isPRMode ? { display: getDisplayRepo(prMetadata), branch: `${getMRLabel(prMetadata)} ${getMRNumberLabel(prMetadata)}` } : await getRepoInfo(); @@ -324,6 +379,24 @@ export async function startReviewServer( } } + if (workspaceRepos?.length) { + await Promise.all(workspaceRepos.map(async (repo) => { + if (!repo.prMetadata) return; + const repoPrRef = prRefFromMetadata(repo.prMetadata); + repo.platformUser = await getPRUser(repoPrRef); + if (repo.prMetadata.platform === "github") { + try { + const viewedMap = await fetchPRViewedFiles(repoPrRef); + repo.viewedFiles = Object.entries(viewedMap) + .filter(([, isViewed]) => isViewed) + .map(([path]) => `${repo.label}/${path}`); + } catch { + // Best effort + } + } + })); + } + // Decision promise let resolveDecision: (result: { approved: boolean; @@ -359,11 +432,12 @@ export async function startReviewServer( rawPatch: currentPatch, gitRef: currentGitRef, origin, - diffType: hasLocalAccess ? currentDiffType : undefined, - gitContext: hasLocalAccess ? gitContext : undefined, + diffType: hasLocalAccess && !isWorkspaceMode ? currentDiffType : undefined, + gitContext: hasLocalAccess && !isWorkspaceMode ? gitContext : undefined, sharingEnabled, shareBaseUrl, repoInfo, + workspace: getWorkspaceState(), isWSL: wslFlag, ...(options.agentCwd && { agentCwd: options.agentCwd }), ...(isPRMode && { prMetadata, platformUser }), @@ -375,6 +449,37 @@ export async function startReviewServer( // API: Switch diff type (requires local file access) if (url.pathname === "/api/diff/switch" && req.method === "POST") { + if (isWorkspaceMode) { + try { + const body = (await req.json()) as { repoId: string; diffType: DiffType }; + const repo = workspaceRepos?.find((candidate) => candidate.id === body.repoId); + if (!repo) { + return Response.json({ error: "Unknown repo" }, { status: 404 }); + } + if (!repo.gitContext) { + return Response.json({ error: "Diff switching unavailable for this repo" }, { status: 400 }); + } + + const nextDiffType = body.diffType || (repo.diffType as DiffType | undefined) || "uncommitted"; + const result = await runVcsDiff(nextDiffType, repo.gitContext.defaultBranch, repo.cwd); + repo.diffType = nextDiffType; + repo.rawPatch = prefixPatchPaths(result.patch, repo.label); + repo.gitRef = result.label; + repo.error = result.error; + refreshWorkspaceAggregate(); + + return Response.json({ + rawPatch: currentPatch, + gitRef: currentGitRef, + workspace: getWorkspaceState(), + ...(currentError && { error: currentError }), + }); + } catch (err) { + const message = err instanceof Error ? err.message : "Failed to switch workspace diff"; + return Response.json({ error: message }, { status: 500 }); + } + } + if (!hasLocalAccess) { return Response.json( { error: "Not available without local file access" }, @@ -417,8 +522,100 @@ export async function startReviewServer( } } + if (url.pathname === "/api/workspace/repo" && req.method === "PATCH") { + if (!isWorkspaceMode || !workspaceRepos) { + return Response.json({ error: "Not in workspace mode" }, { status: 400 }); + } + try { + const body = (await req.json()) as { + repoId: string; + selected?: boolean; + source?: "local" | "pr"; + prUrl?: string; + }; + const repo = workspaceRepos.find((candidate) => candidate.id === body.repoId); + if (!repo) return Response.json({ error: "Unknown repo" }, { status: 404 }); + + if (typeof body.selected === "boolean") repo.selected = body.selected; + if (body.source) repo.source = body.source; + + if (repo.source === "pr") { + const nextUrl = body.prUrl || repo.prMetadata?.url || repo.discoveredPRs?.[0]?.url; + if (!nextUrl) { + return Response.json({ error: "No PR/MR available for this repo" }, { status: 400 }); + } + const ref = parsePRUrl(nextUrl); + if (!ref) return Response.json({ error: "Invalid PR/MR URL" }, { status: 400 }); + const pr = await fetchPR(ref); + repo.prMetadata = pr.metadata; + repo.rawPatch = prefixPatchPaths(pr.rawPatch, repo.label); + repo.gitRef = pr.metadata.url; + repo.platformUser = await getPRUser(prRefFromMetadata(pr.metadata)); + if (pr.metadata.platform === "github") { + try { + const viewedMap = await fetchPRViewedFiles(prRefFromMetadata(pr.metadata)); + repo.viewedFiles = Object.entries(viewedMap) + .filter(([, isViewed]) => isViewed) + .map(([path]) => `${repo.label}/${path}`); + } catch { + repo.viewedFiles = []; + } + } + repo.error = undefined; + } else if (repo.gitContext) { + const nextDiffType = (repo.diffType as DiffType | undefined) || "uncommitted"; + const result = await runVcsDiff(nextDiffType, repo.gitContext.defaultBranch, repo.cwd); + repo.rawPatch = prefixPatchPaths(result.patch, repo.label); + repo.gitRef = result.label; + repo.prMetadata = undefined; + repo.platformUser = null; + repo.viewedFiles = []; + repo.error = result.error; + } + + refreshWorkspaceAggregate(); + return Response.json({ + rawPatch: currentPatch, + gitRef: currentGitRef, + workspace: getWorkspaceState(), + ...(currentError && { error: currentError }), + }); + } catch (err) { + const message = err instanceof Error ? err.message : "Failed to update workspace repo"; + return Response.json({ error: message }, { status: 500 }); + } + } + + if (url.pathname === "/api/workspace/active" && req.method === "POST") { + if (!isWorkspaceMode) { + return Response.json({ error: "Not in workspace mode" }, { status: 400 }); + } + try { + const body = (await req.json()) as { repoId?: string }; + if (body.repoId) currentActiveRepoId = body.repoId; + return Response.json({ ok: true }); + } catch { + return Response.json({ error: "Invalid request" }, { status: 400 }); + } + } + // API: Fetch PR context (comments, checks, merge status) — PR mode only if (url.pathname === "/api/pr-context" && req.method === "GET") { + if (isWorkspaceMode && workspaceRepos) { + const repoId = url.searchParams.get("repoId"); + const repo = repoId ? workspaceRepos.find((candidate) => candidate.id === repoId) : getActiveRepo(); + if (!repo?.prMetadata) { + return Response.json({ error: "Repo is not in PR mode" }, { status: 400 }); + } + try { + const context = await fetchPRContext(prRefFromMetadata(repo.prMetadata)); + return Response.json(context); + } catch (err) { + const message = err instanceof Error ? err.message : "Failed to fetch PR context"; + return Response.json({ error: message }, { status: 500 }); + } + } + if (!isPRMode) { return Response.json( { error: "Not in PR mode" }, @@ -451,6 +648,35 @@ export async function startReviewServer( } } + if (isWorkspaceMode && workspaceRepos) { + const resolved = getRepoForFilePath(filePath); + if (!resolved) { + return Response.json({ error: "Unknown repo path" }, { status: 404 }); + } + const resolvedOld = oldPath ? getRepoForFilePath(oldPath) : null; + + if (resolved.repo.source === "pr" && resolved.repo.prMetadata) { + const ref = prRefFromMetadata(resolved.repo.prMetadata); + const oldSha = resolved.repo.prMetadata.mergeBaseSha ?? resolved.repo.prMetadata.baseSha; + const [oldContent, newContent] = await Promise.all([ + fetchPRFileContent(ref, oldSha, resolvedOld?.repoRelativePath || resolved.repoRelativePath), + fetchPRFileContent(ref, resolved.repo.prMetadata.headSha, resolved.repoRelativePath), + ]); + return Response.json({ oldContent, newContent }); + } + + if (resolved.repo.gitContext) { + const result = await getVcsFileContentsForDiff( + (resolved.repo.diffType as DiffType) || "uncommitted", + resolved.repo.gitContext.defaultBranch, + resolved.repoRelativePath, + resolvedOld?.repoRelativePath, + resolved.repo.cwd, + ); + return Response.json(result); + } + } + // Local review: read file contents from local git if (hasLocalAccess) { const defaultBranch = gitContext?.defaultBranch || "main"; @@ -482,6 +708,31 @@ export async function startReviewServer( // API: Stage / unstage a file (disabled when VCS doesn't support it) if (url.pathname === "/api/git-add" && req.method === "POST") { + if (isWorkspaceMode && workspaceRepos) { + try { + const body = (await req.json()) as { filePath: string; undo?: boolean }; + const resolved = getRepoForFilePath(body.filePath); + if (!resolved) { + return Response.json({ error: "Unknown repo path" }, { status: 404 }); + } + if (resolved.repo.source !== "local" || !resolved.repo.gitContext) { + return Response.json({ error: "Staging not available" }, { status: 400 }); + } + + const repoDiffType = (resolved.repo.diffType as DiffType) || "uncommitted"; + const cwd = resolveVcsCwd(repoDiffType, resolved.repo.cwd); + if (body.undo) { + await unstageFile(repoDiffType, resolved.repoRelativePath, cwd); + } else { + await stageFile(repoDiffType, resolved.repoRelativePath, cwd); + } + return Response.json({ ok: true }); + } catch (err) { + const message = err instanceof Error ? err.message : "Failed to stage file"; + return Response.json({ error: message }, { status: 500 }); + } + } + if (isPRMode || !canStageFiles(currentDiffType)) { return Response.json( { error: "Staging not available" }, @@ -598,6 +849,36 @@ export async function startReviewServer( // API: Submit PR review directly to GitHub (PR mode only) if (url.pathname === "/api/pr-action" && req.method === "POST") { + if (isWorkspaceMode && workspaceRepos) { + const body = (await req.json()) as { + repoId: string; + action: "approve" | "comment"; + body: string; + fileComments: PRReviewFileComment[]; + }; + const repo = workspaceRepos.find((candidate) => candidate.id === body.repoId); + if (!repo?.prMetadata) { + return Response.json({ error: "Repo is not in PR mode" }, { status: 400 }); + } + try { + const prRefForRepo = prRefFromMetadata(repo.prMetadata); + await submitPRReview( + prRefForRepo, + repo.prMetadata.headSha, + body.action, + body.body, + body.fileComments.map((comment) => ({ + ...comment, + path: resolveWorkspaceFilePath(workspaceRepos, comment.path)?.repoRelativePath ?? comment.path, + })), + ); + return Response.json({ ok: true, prUrl: repo.prMetadata.url }); + } catch (err) { + const message = err instanceof Error ? err.message : "Failed to submit PR review"; + return Response.json({ error: message }, { status: 500 }); + } + } + if (!isPRMode || !prMetadata) { return Response.json({ error: "Not in PR mode" }, { status: 400 }); } @@ -630,6 +911,30 @@ export async function startReviewServer( // API: Mark/unmark PR files as viewed on GitHub (PR mode, GitHub only) if (url.pathname === "/api/pr-viewed" && req.method === "POST") { + if (isWorkspaceMode && workspaceRepos) { + const body = (await req.json()) as { + repoId: string; + filePaths: string[]; + viewed: boolean; + }; + const repo = workspaceRepos.find((candidate) => candidate.id === body.repoId); + if (!repo?.prMetadata || repo.prMetadata.platform !== "github" || !repo.prMetadata.prNodeId) { + return Response.json({ error: "Viewed sync not available for this repo" }, { status: 400 }); + } + try { + await markPRFilesViewed( + prRefFromMetadata(repo.prMetadata), + repo.prMetadata.prNodeId, + body.filePaths.map((filePath) => resolveWorkspaceFilePath(workspaceRepos, filePath)?.repoRelativePath ?? filePath), + body.viewed, + ); + return Response.json({ ok: true }); + } catch (err) { + const message = err instanceof Error ? err.message : "Failed to update viewed state"; + return Response.json({ error: message }, { status: 500 }); + } + } + if (!isPRMode || !prMetadata) { return Response.json({ error: "Not in PR mode" }, { status: 400 }); } @@ -667,7 +972,12 @@ export async function startReviewServer( // Serve embedded HTML for all other routes (SPA) return new Response(htmlContent, { - headers: { "Content-Type": "text/html" }, + headers: { + "Content-Type": "text/html", + "Cache-Control": "no-store, no-cache, must-revalidate", + "Pragma": "no-cache", + "Expires": "0", + }, }); }, diff --git a/packages/server/shared-handlers.ts b/packages/server/shared-handlers.ts index 83675d3d..941e93a9 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.test.ts b/packages/server/vcs.test.ts new file mode 100644 index 00000000..75aa34f4 --- /dev/null +++ b/packages/server/vcs.test.ts @@ -0,0 +1,209 @@ +/** + * VCS Detection Tests + * + * Tests for VCS provider detection and the workspace root fallback behavior. + * Run: bun test packages/server/vcs.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 } from "node:path"; +import { spawnSync } from "node:child_process"; + +import { detectManagedVcs, detectVcs } from "./vcs"; + +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("vcs detection", () => { + describe("detectManagedVcs", () => { + it("returns git provider when inside a git repo", async () => { + const repoDir = makeTempDir("plannotator-vcs-git-"); + initRepo(repoDir); + + const provider = await detectManagedVcs(repoDir); + + expect(provider).not.toBeNull(); + expect(provider?.id).toBe("git"); + }); + + it("returns null when not in any VCS repo", async () => { + const nonVcsDir = makeTempDir("plannotator-vcs-none-"); + // Just create a regular file, no git init + writeFileSync(join(nonVcsDir, "file.txt"), "content", "utf-8"); + + const provider = await detectManagedVcs(nonVcsDir); + + expect(provider).toBeNull(); + }); + + it("returns null for non-git workspace root with nested git repos", async () => { + // This is the key regression test: a workspace root that is NOT a git repo + // but contains multiple git repos should NOT be detected as having a VCS + const workspaceRoot = makeTempDir("plannotator-vcs-workspace-"); + + // Create nested git repos + const frontend = join(workspaceRoot, "frontend"); + const backend = join(workspaceRoot, "backend"); + mkdirSync(frontend, { recursive: true }); + mkdirSync(backend, { recursive: true }); + + initRepo(frontend); + initRepo(backend); + + // Verify the workspace root itself is NOT a git repo + const gitDir = join(workspaceRoot, ".git"); + const fs = await import("node:fs"); + expect(fs.existsSync(gitDir)).toBe(false); + + // The workspace root should NOT be detected as having a VCS + const provider = await detectManagedVcs(workspaceRoot); + + expect(provider).toBeNull(); + }); + }); + + describe("detectVcs", () => { + it("returns git provider when inside a git repo", async () => { + const repoDir = makeTempDir("plannotator-vcs-detect-git-"); + initRepo(repoDir); + + const provider = await detectVcs(repoDir); + + expect(provider.id).toBe("git"); + }); + + it("falls back to git provider when no VCS detected (legacy behavior)", async () => { + // This tests the current fallback behavior - when no VCS is detected, + // it defaults to git provider. This is the existing behavior that + // the workspace review feature needs to handle carefully. + const nonVcsDir = makeTempDir("plannotator-vcs-fallback-"); + writeFileSync(join(nonVcsDir, "file.txt"), "content", "utf-8"); + + const provider = await detectVcs(nonVcsDir); + + // Falls back to git provider even though not in a git repo + expect(provider.id).toBe("git"); + }); + + it("caches provider detection results", async () => { + const repoDir = makeTempDir("plannotator-vcs-cache-"); + initRepo(repoDir); + + // First call should detect and cache + const provider1 = await detectVcs(repoDir); + + // Second call should return cached result + const provider2 = await detectVcs(repoDir); + + expect(provider1).toBe(provider2); + }); + }); + + describe("regression: non-git workspace roots", () => { + it("workspace root without .git should not be treated as single-repo", async () => { + // Regression test for: non-git workspace roots were incorrectly treated + // as single-repo due to VCS fallback behavior + // + // Before the fix, running `plannotator review` in a workspace root that + // contained multiple git repos but wasn't itself a git repo would: + // 1. detectVcs() would fall back to git provider + // 2. The review would treat the entire workspace as a single repo + // 3. This would show incorrect diff or fail to find git context + // + // After the fix, workspace review mode should: + // 1. Check if the CWD is a git repo using detectManagedVcs() + // 2. If not, discover nested repos and enter workspace mode + // 3. Only fall back to single-repo mode if detectManagedVcs() returns a provider + + const workspaceRoot = makeTempDir("plannotator-regression-workspace-"); + + // Create multiple nested git repos (simulating a typical workspace) + const packages = join(workspaceRoot, "packages"); + const apps = join(workspaceRoot, "apps"); + + const uiPkg = join(packages, "ui"); + const apiPkg = join(packages, "api"); + const webApp = join(apps, "web"); + + mkdirSync(uiPkg, { recursive: true }); + mkdirSync(apiPkg, { recursive: true }); + mkdirSync(webApp, { recursive: true }); + + initRepo(uiPkg); + initRepo(apiPkg); + initRepo(webApp); + + // Verify workspace root is NOT a git repo + const fs = await import("node:fs"); + expect(fs.existsSync(join(workspaceRoot, ".git"))).toBe(false); + + // detectManagedVcs should return null (no VCS at workspace root) + const managedVcs = await detectManagedVcs(workspaceRoot); + expect(managedVcs).toBeNull(); + + // But detectVcs (with fallback) returns git provider + // This is the legacy behavior that workspace review must handle + const fallbackVcs = await detectVcs(workspaceRoot); + expect(fallbackVcs.id).toBe("git"); + + // The key assertion: workspace review code should use detectManagedVcs() + // to determine if it's in a repo, NOT detectVcs() which has fallback + }); + + it("single git repo at CWD should use single-repo mode", async () => { + // When CWD is itself a git repo, we should use single-repo review mode + const repoDir = makeTempDir("plannotator-regression-single-"); + initRepo(repoDir); + + const managedVcs = await detectManagedVcs(repoDir); + + expect(managedVcs).not.toBeNull(); + expect(managedVcs?.id).toBe("git"); + }); + + it("nested git repo at CWD should use single-repo mode", async () => { + // When CWD is a nested git repo within a workspace, use single-repo mode + const workspaceRoot = makeTempDir("plannotator-regression-nested-"); + + const frontend = join(workspaceRoot, "frontend"); + mkdirSync(frontend, { recursive: true }); + initRepo(frontend); + + const managedVcs = await detectManagedVcs(frontend); + + expect(managedVcs).not.toBeNull(); + expect(managedVcs?.id).toBe("git"); + }); + }); +}); diff --git a/packages/server/vcs.ts b/packages/server/vcs.ts index 5c15d02f..0fb0b06d 100644 --- a/packages/server/vcs.ts +++ b/packages/server/vcs.ts @@ -162,17 +162,26 @@ export { parseWorktreeDiffType, validateFilePath, runtime as gitRuntime } from " const vcsCache = new Map(); +/** Detect which VCS manages the given directory, without fallback. */ +export async function detectManagedVcs(cwd?: string): Promise { + for (const provider of providers) { + if (await provider.detect(cwd)) { + return provider; + } + } + return null; +} + /** Detect which VCS manages the given directory */ export async function detectVcs(cwd?: string): Promise { const key = cwd ?? process.cwd(); const cached = vcsCache.get(key); if (cached) return cached; - for (const provider of providers) { - if (await provider.detect(cwd)) { - vcsCache.set(key, provider); - return provider; - } + const detected = await detectManagedVcs(cwd); + if (detected) { + vcsCache.set(key, detected); + return detected; } // Default to git (existing behavior) diff --git a/packages/shared/package.json b/packages/shared/package.json index cdbb4b0c..725ae02b 100644 --- a/packages/shared/package.json +++ b/packages/shared/package.json @@ -8,6 +8,7 @@ "./crypto": "./crypto.ts", "./feedback-templates": "./feedback-templates.ts", "./review-core": "./review-core.ts", + "./review-workspace": "./review-workspace.ts", "./checklist": "./checklist.ts", "./types": "./types.ts", "./pr-provider": "./pr-provider.ts", diff --git a/packages/shared/review-workspace.ts b/packages/shared/review-workspace.ts new file mode 100644 index 00000000..34d9e2b5 --- /dev/null +++ b/packages/shared/review-workspace.ts @@ -0,0 +1,30 @@ +import type { 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?: string; + 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 d12a50d0..57caccc2 100644 --- a/packages/shared/types.ts +++ b/packages/shared/types.ts @@ -15,3 +15,10 @@ export type { WorktreeInfo, GitContext, } from "./review-core"; + +export type { + WorkspaceRepoSource, + WorkspacePRCandidate, + WorkspaceRepoState, + WorkspaceReviewState, +} from "./review-workspace";