diff --git a/.claude/agent-memory/code-reviewer/MEMORY.md b/.claude/agent-memory/code-reviewer/MEMORY.md index 83f1270ac..e252dcc4a 100644 --- a/.claude/agent-memory/code-reviewer/MEMORY.md +++ b/.claude/agent-memory/code-reviewer/MEMORY.md @@ -16,6 +16,14 @@ - `[animation-fill-mode:backwards]` survives twMerge when animate-[] class is replaced — benign but semantically imprecise on infinite animations +## Dead Code Pattern (Confirmed) + +- When removing a prop from a call site, also check the destructuring site of the hook that + originally provided the handler. E.g., removing `onReviewPR={handleOpenPR}` from `` + leaves `handleOpenPR` destructured-but-unused in `MainContent.tsx` (from `useWorkspaceActions`). + TypeScript does not error on unused destructured variables — only ESLint's `no-unused-vars` rule + catches this. Always grep for all references after removing a prop. + ## Common Pitfalls Seen - `useLayoutEffect` SSR warning: hook uses `typeof window` guard for SSR safety in `useState` @@ -150,3 +158,40 @@ - `bg-accent-green`, `bg-accent-gold`, `bg-accent-red` ARE valid Tailwind tokens — defined in `global.css` `@theme` block as `--color-accent-green`, `--color-accent-gold`, `--color-accent-red`. Not hardcoded colors; they route through CSS variables to theme values. +- `bg-warning`, `bg-success`, `text-warning` ARE valid — `--color-warning` and `--color-success` + defined in `@theme` block (lines 30, 32 of global.css). Safe to use in action button variants. + +## PRStatus / GhCliStatus Patterns (Confirmed) + +- `PRStatus.pr_url` is optional (`pr_url?: string`). Falling back to `""` on undefined produces + `href=""` in a Tauri WebView which navigates to `tauri://localhost/` — silent app reload. Always + guard: use `pr_url ?? null` and skip rendering the anchor when null. +- `PRStatus.pr_state` includes `"closed"` (abandoned PR, not merged). Failing to handle this case + causes the state machine to fall through to actionable states (Fix CI, Resolve Conflicts) on a + dead branch — prompting the agent to work on an already-closed PR. +- `ghStatus` being `null`/`undefined` (TanStack Query loading) should be treated as "unknown, do + not surface action buttons" — not as "gh is available." The guard `if (ghStatus && ...)` silently + passes null/undefined through to PR state evaluation. +- `derivePRActionState` in `src/features/workspace/lib/prState.ts` is the single source of truth + for PR state machine. Pure function, no tests exist yet — high-value test target. +- `PRActionState` discriminated union: 11 variants (added `closed` and `error`). `match().exhaustive()` + used in PRActions.tsx main render. PRLink uses a non-exhaustive if-chain guard (early return for + `gh_unavailable`, `no_pr`, `error`) — this remains a type-safety gap when new variants are added. +- `prUrl = prStatus.pr_url ?? ""` in `derivePRActionState` (line 75). The `""` fallback still produces + `href=""` → `tauri://localhost/` reload risk, BUT `PRLink` filters on `!= "gh_unavailable" | "no_pr" | "error"` + so the only states that render the anchor are states where `pr_url` is always set by the backend (has_pr=true path). + Risk is low in practice but the type system doesn't enforce it (string, not string & URL). +- `FAILING_CONCLUSIONS` and `PENDING_STATES` sets are defined INSIDE the request handler function + (inside the `app.get(...)` callback), so they are re-created on every request. Move to module scope. +- `lastError` logic: `runGh` returns only `'unknown'` for non-specific errors, so the check + `lastError === 'unknown' ? 'network' : null` always maps to `'network'` when set. Correct but + the conditional is redundant — could just be `lastError ? 'network' : null`. +- CI `hasPending` check has a subtle issue: when `c.conclusion == null` AND `PENDING_STATES.has(c.state)` + are both truthy, the OR short-circuits at `c.conclusion == null`. This is correct for in-progress checks. + However, a check with `conclusion === null` and `state` NOT in PENDING_STATES (e.g. a weird state) + would still mark it pending — acceptable given the unknown = pending safety heuristic. +- `query.state.data` in `usePRStatus` refetchInterval callback is cast to a loose object type instead + of `PRStatus | null`. Should use `import type { PRStatus }` and cast to `PRStatus | null | undefined`. +- `review_required` and `approved` review statuses are not mapped to PRActionState variants. They both + fall through to `awaiting_review`. This is intentional (safest default) but `review_required` could + deserve its own state in a future iteration. diff --git a/backend/src/routes/workspaces.ts b/backend/src/routes/workspaces.ts index b8ae8f38d..adada9232 100644 --- a/backend/src/routes/workspaces.ts +++ b/backend/src/routes/workspaces.ts @@ -209,6 +209,18 @@ app.get('/gh-status', async (c) => { return c.json({ isInstalled: true, isAuthenticated: authResult.success }); }); +// GitHub Check Suite conclusions that indicate a non-passing terminal state. +// Full GraphQL enum: ACTION_REQUIRED, CANCELLED, FAILURE, NEUTRAL, SKIPPED, +// STALE, STARTUP_FAILURE, SUCCESS, TIMED_OUT. +// NEUTRAL/SKIPPED are intentionally non-blocking (count as passing). +// STALE means re-run is needed (count as pending below). +const FAILING_CONCLUSIONS = new Set([ + 'FAILURE', 'ERROR', 'TIMED_OUT', 'STARTUP_FAILURE', 'ACTION_REQUIRED', 'CANCELLED', +]); +// CheckRun `status` values that indicate the check hasn't completed yet. +// Note: CheckRun uses `status` field, StatusContext uses `state` field. +const PENDING_STATUSES = new Set(['PENDING', 'QUEUED', 'IN_PROGRESS', 'WAITING', 'REQUESTED']); + // PR status — async, fork-aware, explicit errors app.get('/workspaces/:id/pr-status', withWorkspace, async (c) => { const workspacePath = c.get('workspacePath'); @@ -247,6 +259,9 @@ app.get('/workspaces/:id/pr-status', withWorkspace, async (c) => { if (isFork) attempts.push({ repoArg: upstreamUrl, headArg: headBranch }); attempts.push({ repoArg: originUrl, headArg: headBranch }); + let lastError: string | null = null; + let hadSuccessfulResponse = false; + for (const { repoArg, headArg } of attempts) { const args = ['pr', 'list', '--head', headArg, '--author', '@me', '--state', 'all', '--json', 'number,title,url,state,mergeable,mergeStateStatus,statusCheckRollup,reviewDecision,isDraft']; @@ -254,33 +269,76 @@ app.get('/workspaces/:id/pr-status', withWorkspace, async (c) => { const result = await runGh(args, { cwd: workspacePath }); if (!result.success) { - // Surface specific errors (installed/auth) to the frontend + // Surface specific errors (installed/auth) to the frontend immediately if (result.error === 'gh_not_installed' || result.error === 'gh_not_authenticated' || result.error === 'timeout') { return c.json({ has_pr: false, error: result.error }); } - continue; // Try next attempt for unknown errors (e.g. repo not found) + lastError = result.error; // Track for surfacing if all attempts fail + continue; } let prs: any[]; try { prs = JSON.parse(result.stdout || '[]'); } catch { continue; } if (!Array.isArray(prs)) continue; + hadSuccessfulResponse = true; + // Priority: OPEN > MERGED > CLOSED. Open PRs are actionable, + // merged PRs show archive, closed PRs show a non-actionable status. const openPr = prs.find((pr: any) => pr.state?.toUpperCase() === 'OPEN'); const mergedPr = prs.find((pr: any) => pr.state?.toUpperCase() === 'MERGED'); - const pr = openPr ?? mergedPr; + const closedPr = prs.find((pr: any) => pr.state?.toUpperCase() === 'CLOSED'); + const pr = openPr ?? mergedPr ?? closedPr; if (pr) { - const state = pr.state?.toUpperCase() === 'MERGED' ? 'merged' : 'open'; + const upperState = pr.state?.toUpperCase(); + const state: 'open' | 'merged' | 'closed' = + upperState === 'MERGED' ? 'merged' : + upperState === 'CLOSED' ? 'closed' : 'open'; + + // Closed PRs are terminal — no CI or merge status is relevant + if (state === 'closed') { + return c.json({ + has_pr: true, + pr_number: pr.number, + pr_title: pr.title, + pr_url: pr.url, + pr_state: 'closed', + merge_status: 'blocked', + is_draft: pr.isDraft === true, + has_conflicts: false, + ci_status: 'unknown', + review_status: 'none', + error: null, + }); + } + let mergeStatus: 'ready' | 'blocked' | 'merged' = 'blocked'; if (state === 'merged') mergeStatus = 'merged'; else if (pr.mergeable === 'MERGEABLE') mergeStatus = 'ready'; - // Derive CI status from statusCheckRollup + // Derive CI status from statusCheckRollup. + // The array contains TWO different object types: + // - CheckRun (__typename: "CheckRun"): uses `conclusion` (SUCCESS/FAILURE/null) + `status` + // - StatusContext (__typename: "StatusContext"): uses `state` (SUCCESS/FAILURE/PENDING/ERROR) + // We must handle both — StatusContext has no `conclusion` field at all. const checks: any[] = pr.statusCheckRollup ?? []; let ciStatus: 'passing' | 'failing' | 'pending' | 'unknown' = 'unknown'; if (checks.length > 0) { - const hasFailing = checks.some((c: any) => c.conclusion === 'FAILURE' || c.conclusion === 'ERROR' || c.conclusion === 'TIMED_OUT'); - const hasPending = checks.some((c: any) => !c.conclusion || c.state === 'PENDING' || c.state === 'QUEUED' || c.state === 'IN_PROGRESS'); + const hasFailing = checks.some((c: any) => { + if (c.__typename === 'StatusContext') { + return c.state === 'FAILURE' || c.state === 'ERROR'; + } + return FAILING_CONCLUSIONS.has(c.conclusion); + }); + const hasPending = checks.some((c: any) => { + if (c.__typename === 'StatusContext') { + return c.state === 'PENDING' || c.state === 'EXPECTED'; + } + // CheckRun: null conclusion means still running, STALE means re-run needed + return c.conclusion === 'STALE' || + c.conclusion == null || + PENDING_STATUSES.has(c.status); + }); if (hasFailing) ciStatus = 'failing'; else if (hasPending) ciStatus = 'pending'; else ciStatus = 'passing'; @@ -308,7 +366,9 @@ app.get('/workspaces/:id/pr-status', withWorkspace, async (c) => { } } - return c.json({ has_pr: false, error: null }); + // If all attempts failed with errors, surface it instead of silently showing "no PR". + // lastError is only set for 'unknown' errors (timeout/auth/install return immediately). + return c.json({ has_pr: false, error: (!hadSuccessfulResponse && lastError) ? 'network' : null }); }); // Pen files diff --git a/shared/types/github.ts b/shared/types/github.ts index 4568cc1b0..6f9a1a408 100644 --- a/shared/types/github.ts +++ b/shared/types/github.ts @@ -18,7 +18,7 @@ export interface PRStatus { has_conflicts?: boolean; ci_status?: "passing" | "failing" | "pending" | "unknown"; review_status?: "approved" | "changes_requested" | "review_required" | "none"; - error?: "gh_not_installed" | "gh_not_authenticated" | "timeout" | null; + error?: "gh_not_installed" | "gh_not_authenticated" | "timeout" | "network" | null; } /** diff --git a/src/app/layouts/MainContent.tsx b/src/app/layouts/MainContent.tsx index f6829c4f4..377afe206 100644 --- a/src/app/layouts/MainContent.tsx +++ b/src/app/layouts/MainContent.tsx @@ -34,6 +34,7 @@ import { cn } from "@/shared/lib/utils"; import { ResizablePanelGroup, ResizablePanel, ResizableHandle } from "@/components/ui/resizable"; import { PanelLeft } from "lucide-react"; import type { Workspace, PRStatus, GhCliStatus } from "@/shared/types"; +import { REVIEW_CODE } from "@/features/session/lib/sessionPrompts"; import { emit } from "@/platform/tauri"; import { useBrowserWindowStore } from "@/features/browser/store"; import { ChatArea } from "./ChatArea"; @@ -91,7 +92,6 @@ export function MainContent({ setSelectedTargetBranch, handleCreatePR, handleSendAgentMessage, - handleOpenPR, handleArchive, handleRetrySetup, handleViewSetupLogs, @@ -100,7 +100,6 @@ export function MainContent({ handleRunTask, } = useWorkspaceActions({ selectedWorkspace, - prStatus, setRightSideTab, }); @@ -201,6 +200,11 @@ export function MainContent({ [setRightSideTab] ); + // Insert code review prompt into chat input + const handleInsertReviewPrompt = useCallback(() => { + workspaceChatPanelRef.current?.insertText(REVIEW_CODE); + }, [workspaceChatPanelRef]); + return (
diff --git a/src/app/layouts/RightSidePanel.tsx b/src/app/layouts/RightSidePanel.tsx index 7166cc78c..e497e236c 100644 --- a/src/app/layouts/RightSidePanel.tsx +++ b/src/app/layouts/RightSidePanel.tsx @@ -39,6 +39,8 @@ interface RightSidePanelProps { onOpenFilePreview: (filePath: string) => void; /** Whether file watcher is active -- disables polling in useFileChanges */ isWatched?: boolean; + /** Insert a code review prompt into the chat input */ + onReview?: () => void; } export function RightSidePanel({ @@ -47,6 +49,7 @@ export function RightSidePanel({ onOpenDiffTab, onOpenFilePreview, isWatched = false, + onReview, }: RightSidePanelProps) { const { selectedFilePath, setSelectedFilePath, rightPanelTab, setRightPanelTab } = useWorkspaceLayout(workspace.id); @@ -167,6 +170,7 @@ export function RightSidePanel({ filterMode={filterMode} onFilterModeChange={handleFilterModeChange} workspaceGitInfo={workspaceGitInfo} + onReview={onReview} /> )} diff --git a/src/app/layouts/hooks/useWorkspaceActions.ts b/src/app/layouts/hooks/useWorkspaceActions.ts index a41be5c73..807c96eec 100644 --- a/src/app/layouts/hooks/useWorkspaceActions.ts +++ b/src/app/layouts/hooks/useWorkspaceActions.ts @@ -19,17 +19,15 @@ import { import { WorkspaceService } from "@/features/workspace/api/workspace.service"; import { queueTerminalTask } from "@/features/terminal/store/terminalTaskStore"; import type { RightSideTab } from "@/features/workspace/store"; -import type { Workspace, PRStatus } from "@/shared/types"; +import type { Workspace } from "@/shared/types"; interface UseWorkspaceActionsOptions { selectedWorkspace: Workspace | null; - prStatus: PRStatus | null; setRightSideTab: (tab: RightSideTab) => void; } export function useWorkspaceActions({ selectedWorkspace, - prStatus, setRightSideTab, }: UseWorkspaceActionsOptions) { // PR handler bridge: ChatArea sets it, WorkspaceHeader consumes it. @@ -76,14 +74,6 @@ export function useWorkspaceActions({ [sendAgentMessageHandler] ); - const handleOpenPR = useCallback(() => { - if (!prStatus?.pr_url) { - toast.error("PR link not available."); - return; - } - window.open(prStatus.pr_url, "_blank", "noopener,noreferrer"); - }, [prStatus]); - // --- Archive & retry --- const { mutate: archiveWorkspace } = useArchiveWorkspace(); @@ -154,7 +144,6 @@ export function useWorkspaceActions({ // Action handlers handleCreatePR, handleSendAgentMessage, - handleOpenPR, handleArchive, handleRetrySetup, handleViewSetupLogs, diff --git a/src/features/session/lib/sessionPrompts.ts b/src/features/session/lib/sessionPrompts.ts index 44ece19fb..e93035ae4 100644 --- a/src/features/session/lib/sessionPrompts.ts +++ b/src/features/session/lib/sessionPrompts.ts @@ -36,6 +36,16 @@ export const ADDRESS_REVIEW = "Address the review comments on the PR"; /** Instructs the agent to merge the PR */ export const MERGE_PR = "Merge the PR"; +/** Code review prompt — inserted into chat input from the Code panel Review button */ +export const REVIEW_CODE = `Review the current code changes in this workspace. Analyze the diff for: +- Bugs, logic errors, or edge cases +- Performance issues or unnecessary complexity +- Security concerns (hardcoded secrets, injection, unsafe patterns) +- Code style and consistency with the existing codebase +- Missing error handling or tests + +Provide a concise summary of findings with specific file and line references.`; + // --------------------------------------------------------------------------- // Workspace setup // --------------------------------------------------------------------------- diff --git a/src/features/workspace/api/workspace.queries.ts b/src/features/workspace/api/workspace.queries.ts index 0522b7c34..5dda66aa7 100644 --- a/src/features/workspace/api/workspace.queries.ts +++ b/src/features/workspace/api/workspace.queries.ts @@ -11,6 +11,7 @@ import { RepoService } from "@/features/repository/api/repository.service"; import { queryKeys } from "@/shared/api/queryKeys"; import { API_CONFIG } from "@/shared/config/api.config"; import type { Workspace, RepoGroup, DiffStats } from "../types"; +import type { PRStatus } from "@/shared/types"; import { createOptimisticWorkspace } from "../lib/workspace.utils"; /** @@ -279,7 +280,8 @@ export function useGhStatus() { * * Gated on gh CLI being installed + authenticated (like Codex). * Polls every 30s while agent is working (to detect PR creation), - * stops polling when idle. Auto-refetched on session completion + * 60s while CI is pending post-session (CI runs 2-15min after push), + * stops polling otherwise. Auto-refetched on session completion * via useSessionEvents invalidation. */ export function usePRStatus( @@ -292,10 +294,18 @@ export function usePRStatus( queryFn: () => WorkspaceService.fetchPRStatus(workspaceId!), enabled: !!workspaceId && ghInstalled && ghAuthenticated, staleTime: 10_000, - // Poll while agent is working so we detect PR creation without manual refresh. - // 30s aligns with CLAUDE.md polling budget; event-driven invalidation via - // useSessionEvents handles the fast path (agent just created/updated a PR). - refetchInterval: sessionStatus === "working" ? 30_000 : false, + refetchInterval: (query) => { + // While agent is working: poll every 30s to detect PR creation/updates. + if (sessionStatus === "working") return 30_000; + + // After agent stops: CI checks typically run 2-15min. Poll every 60s + // while CI is pending so the user sees updates without manual refresh. + const data = query.state.data as PRStatus | null | undefined; + if (data?.has_pr && data?.ci_status === "pending") return 60_000; + + // No PR or CI resolved: stop polling. Window focus refetch handles edge cases. + return false; + }, refetchOnWindowFocus: true, }); } diff --git a/src/features/workspace/lib/prState.ts b/src/features/workspace/lib/prState.ts new file mode 100644 index 000000000..c90bd63d7 --- /dev/null +++ b/src/features/workspace/lib/prState.ts @@ -0,0 +1,116 @@ +/** + * PR View State -- derives a single discriminated union from raw PR + GH CLI status. + * + * This is the ONLY place that interprets PRStatus into what the UI should show. + * The priority ordering is explicit: earlier checks shadow later ones. + * + * Pure function. No React. Trivially testable. + */ + +import type { PRStatus, GhCliStatus } from "@/shared/types"; + +/** + * Discriminated union of all possible PR action states. + * Each variant carries only the data its rendering path needs. + * + * Priority order (highest to lowest): + * 1. gh CLI unavailable (blocks everything) + * 2. error (GitHub unreachable — timeout or network failure) + * 3. no PR (initial state) + * 4. merged (terminal) + * 5. closed (terminal — not merged) + * 6. conflicts (must resolve before anything else) + * 7. CI failing (fixable, blocks merge) + * 8. changes requested (reviewer feedback pending) + * 9. CI pending (waiting, no action) + * 10. ready to merge (all green) + * 11. awaiting review (CI passed, needs review) + */ +export type PRActionState = + | { type: "gh_unavailable"; reason: "not_installed" | "not_authenticated" } + | { type: "error"; reason: "timeout" | "network" } + | { type: "no_pr" } + | { type: "merged"; prNumber: number; prUrl: string } + | { type: "closed"; prNumber: number; prUrl: string } + | { type: "conflicts"; prNumber: number; prUrl: string } + | { type: "ci_failing"; prNumber: number; prUrl: string } + | { type: "changes_requested"; prNumber: number; prUrl: string } + | { type: "ci_pending"; prNumber: number; prUrl: string } + | { type: "ready_to_merge"; prNumber: number; prUrl: string; targetBranch: string } + | { type: "awaiting_review"; prNumber: number; prUrl: string }; + +/** + * Collapses PRStatus + GhCliStatus into a single discriminated state. + * The priority ordering ensures that when multiple flags are true + * (e.g., conflicts + CI failing), only the highest-priority state wins. + */ +export function derivePRActionState( + prStatus: PRStatus | null, + ghStatus: GhCliStatus | null | undefined, + targetBranch: string, +): PRActionState { + // gh CLI gates everything + if (ghStatus && !ghStatus.isInstalled) { + return { type: "gh_unavailable", reason: "not_installed" }; + } + if (ghStatus && ghStatus.isInstalled && !ghStatus.isAuthenticated) { + return { type: "gh_unavailable", reason: "not_authenticated" }; + } + + // gh CLI errors from the PR status endpoint — can arrive during the ghStatus + // 5-minute stale window (e.g., gh is uninstalled between status checks). + if (prStatus?.error === "gh_not_installed") { + return { type: "gh_unavailable", reason: "not_installed" }; + } + if (prStatus?.error === "gh_not_authenticated") { + return { type: "gh_unavailable", reason: "not_authenticated" }; + } + + // GitHub unreachable — surface error before interpreting has_pr, + // since has_pr: false might just mean the request failed. + if (prStatus?.error === "timeout") { + return { type: "error", reason: "timeout" }; + } + if (prStatus?.error === "network") { + return { type: "error", reason: "network" }; + } + + // No PR exists yet + if (!prStatus?.has_pr || !prStatus.pr_number) { + return { type: "no_pr" }; + } + + const prNumber = prStatus.pr_number; + const prUrl = prStatus.pr_url ?? ""; + + // Merged is terminal + if (prStatus.merge_status === "merged") { + return { type: "merged", prNumber, prUrl }; + } + + // Closed (not merged) is terminal — no actionable state + if (prStatus.pr_state === "closed") { + return { type: "closed", prNumber, prUrl }; + } + + // Priority-ordered actionable states for open PRs + if (prStatus.has_conflicts) { + return { type: "conflicts", prNumber, prUrl }; + } + if (prStatus.ci_status === "failing") { + return { type: "ci_failing", prNumber, prUrl }; + } + if (prStatus.review_status === "changes_requested") { + return { type: "changes_requested", prNumber, prUrl }; + } + if (prStatus.ci_status === "pending") { + return { type: "ci_pending", prNumber, prUrl }; + } + if (prStatus.merge_status === "ready") { + return { type: "ready_to_merge", prNumber, prUrl, targetBranch }; + } + + // Fallback: CI passing + review needed, or any other open PR state + // (draft, unknown CI, etc.) — safest default, no destructive action + return { type: "awaiting_review", prNumber, prUrl }; +} diff --git a/src/features/workspace/ui/CodePanelContent.tsx b/src/features/workspace/ui/CodePanelContent.tsx index f93a5fa31..1b9fab19f 100644 --- a/src/features/workspace/ui/CodePanelContent.tsx +++ b/src/features/workspace/ui/CodePanelContent.tsx @@ -57,6 +57,8 @@ interface CodePanelContentProps { onFilterModeChange?: (mode: FilterMode) => void; /** Git info for fetching diffs */ workspaceGitInfo: WorkspaceGitInfo; + /** Callback to insert a code review prompt into the chat input */ + onReview?: () => void; } export function CodePanelContent({ @@ -71,6 +73,7 @@ export function CodePanelContent({ filterMode = "changes", onFilterModeChange, workspaceGitInfo, + onReview, }: CodePanelContentProps) { const diffViewerRef = useRef(null); const [changesFilter, setChangesFilter] = useState("all-changes"); @@ -142,7 +145,7 @@ export function CodePanelContent({ {filterMode === "changes" && ( - + + )} ); } diff --git a/src/features/workspace/ui/PRActions.tsx b/src/features/workspace/ui/PRActions.tsx index da40ca5fd..d4ef0cc50 100644 --- a/src/features/workspace/ui/PRActions.tsx +++ b/src/features/workspace/ui/PRActions.tsx @@ -1,26 +1,32 @@ /** * PR Actions -- right-side actions for the content panel header. * - * Renders PR status chips, Review button, and the Create PR / Merge - * split button. Extracted from WorkspaceHeader so the actions live - * in the right panel's tab header (per the new layout design). + * Architecture: PRStatus (bag of optionals) -> derivePRActionState (pure function) + * -> PRActionState (discriminated union) -> exhaustive match -> JSX. + * + * Each PR state maps to exactly one visual representation: + * - "#N" link + contextual action button, OR + * - "#N" link + status text (non-actionable), OR + * - Create PR split button (pre-PR) + * + * No redundant chips. The button IS the status indicator. */ import { useState, useEffect } from "react"; import { - Eye, GitMerge, GitPullRequestCreate, + GitPullRequestClosed, ChevronDown, Archive, AlertTriangle, - CircleCheck, CircleX, Loader2, MessageSquareWarning, FileWarning, + WifiOff, } from "lucide-react"; -import { match, P } from "ts-pattern"; +import { match } from "ts-pattern"; import { Tooltip, TooltipContent, TooltipTrigger } from "@/components/ui/tooltip"; import { cn } from "@/shared/lib/utils"; import { BranchSelector } from "./BranchSelector"; @@ -30,26 +36,33 @@ import { ADDRESS_REVIEW, MERGE_PR, } from "@/features/session/lib/sessionPrompts"; +import { derivePRActionState, type PRActionState } from "../lib/prState"; import type { PRStatus, GhCliStatus } from "@/shared/types"; +// --------------------------------------------------------------------------- +// Props +// --------------------------------------------------------------------------- + interface PRActionsProps { prStatus: PRStatus | null; ghStatus?: GhCliStatus | null; onCreatePR?: () => void; onSendAgentMessage?: (text: string) => void; - onReviewPR?: () => void; onArchive?: () => void; targetBranch: string; onTargetBranchChange: (branch: string) => void; workspacePath: string | null; } +// --------------------------------------------------------------------------- +// Main component +// --------------------------------------------------------------------------- + export function PRActions({ prStatus, ghStatus, onCreatePR, onSendAgentMessage, - onReviewPR, onArchive, targetBranch: targetBranchProp = "main", onTargetBranchChange, @@ -59,311 +72,319 @@ export function PRActions({ useEffect(() => { setLocalTargetBranch(targetBranchProp); }, [targetBranchProp]); - const effectiveTarget = localTargetBranch; - - const hasPR = Boolean(prStatus?.has_pr && prStatus?.pr_number); - const isMerged = prStatus?.merge_status === "merged"; - const isReady = prStatus?.merge_status === "ready"; - const hasConflicts = prStatus?.has_conflicts === true; - const ciStatus = prStatus?.ci_status; - const reviewStatus = prStatus?.review_status; - const isDraft = prStatus?.is_draft === true; - - const ghMissing = ghStatus !== undefined && ghStatus !== null && !ghStatus.isInstalled; - const ghUnauthenticated = - ghStatus !== undefined && - ghStatus !== null && - ghStatus.isInstalled && - !ghStatus.isAuthenticated; const handleBranchSelect = (name: string) => { setLocalTargetBranch(name); onTargetBranchChange(name); }; + const state = derivePRActionState(prStatus, ghStatus, localTargetBranch); + return (
- {/* PR status indicators */} - {hasPR && !isMerged && ( - - )} + {/* PR number link -- shown for all states that have a PR */} + - {/* Review button */} - {hasPR && onReviewPR && ( - - )} - - {/* Primary action — dispatches on gh CLI status, PR state, and merge readiness */} - {match({ ghMissing, ghUnauthenticated, isMerged, hasPR, hasConflicts, ciStatus, reviewStatus, isReady, isDraft }) - .with(P.union({ ghMissing: true }, { ghUnauthenticated: true }), () => ( - - - - - -

- {ghMissing - ? "GitHub CLI not installed — install gh to manage PRs" - : "Not authenticated — run gh auth login"} -

- - + {/* State-specific rendering: status text OR action button */} + {match(state) + .with({ type: "gh_unavailable" }, (s) => ( + )) - .with({ isMerged: true }, () => - onArchive ? ( - - ) : null - ) - .with({ hasPR: true, hasConflicts: true }, () => ( - } - label="Resolve Conflicts" - onLeftClick={() => onSendAgentMessage?.(RESOLVE_CONFLICTS)} - leftDisabled={!onSendAgentMessage} - branchLabel={effectiveTarget} + .with({ type: "error" }, (s) => ( + + )) + .with({ type: "no_pr" }, () => ( + + )) + .with({ type: "ci_pending" }, () => ( + } + label="Checks running" + variant="pending" + /> + )) + .with({ type: "awaiting_review" }, () => ( + + )) + .with({ type: "closed" }, () => ( + } + label="Closed" + variant="closed" /> )) - .with({ hasPR: true, ciStatus: "failing" }, () => ( - ( + } label="Fix CI" - onLeftClick={() => onSendAgentMessage?.(FIX_CI)} - leftDisabled={!onSendAgentMessage} - branchLabel={effectiveTarget} - workspacePath={workspacePath} - currentBranch={effectiveTarget} - onBranchSelect={handleBranchSelect} - branchEditable={false} + variant="destructive" + onClick={() => onSendAgentMessage?.(FIX_CI)} + disabled={!onSendAgentMessage} + /> + )) + .with({ type: "conflicts" }, () => ( + } + label="Resolve Conflicts" + variant="destructive" + onClick={() => onSendAgentMessage?.(RESOLVE_CONFLICTS)} + disabled={!onSendAgentMessage} /> )) - .with({ hasPR: true, reviewStatus: "changes_requested" }, () => ( - ( + } label="Address Review" - onLeftClick={() => onSendAgentMessage?.(ADDRESS_REVIEW)} - leftDisabled={!onSendAgentMessage} - branchLabel={effectiveTarget} - workspacePath={workspacePath} - currentBranch={effectiveTarget} - onBranchSelect={handleBranchSelect} - branchEditable={false} + variant="warning" + onClick={() => onSendAgentMessage?.(ADDRESS_REVIEW)} + disabled={!onSendAgentMessage} /> )) - .with({ hasPR: true, isReady: true }, () => ( - ( + } - label="Merge" - onLeftClick={() => onSendAgentMessage?.(MERGE_PR)} - leftDisabled={!onSendAgentMessage} - branchLabel={effectiveTarget} - workspacePath={workspacePath} - currentBranch={effectiveTarget} - onBranchSelect={handleBranchSelect} - branchEditable={false} + label={`Merge into ${s.targetBranch}`} + variant="success" + onClick={() => onSendAgentMessage?.(MERGE_PR)} + disabled={!onSendAgentMessage} /> )) - .with({ hasPR: true }, () => ( - } - label={isDraft ? "Draft" : ciStatus === "pending" ? "CI Running" : "Blocked"} - onLeftClick={() => {}} - leftDisabled - branchLabel={effectiveTarget} - workspacePath={workspacePath} - currentBranch={effectiveTarget} - onBranchSelect={handleBranchSelect} - branchEditable={false} - /> - )) - .with({ hasPR: false, isMerged: false }, () => ( - } - label="Create PR" - onLeftClick={onCreatePR ?? (() => {})} - leftDisabled={!onCreatePR} - branchLabel={effectiveTarget} - workspacePath={workspacePath} - currentBranch={effectiveTarget} - onBranchSelect={handleBranchSelect} - /> - )) - .otherwise(() => null)} + .with({ type: "merged" }, () => + onArchive ? ( + } + label="Archive" + variant="primary" + onClick={onArchive} + /> + ) : null + ) + .exhaustive()}
); } // --------------------------------------------------------------------------- -// PRStatusChips +// PRLink -- "#N" clickable link to open PR in browser // --------------------------------------------------------------------------- -function PRStatusChips({ - ciStatus, - reviewStatus, - hasConflicts, - isDraft, -}: { - ciStatus?: string; - reviewStatus?: string; - hasConflicts?: boolean; - isDraft?: boolean; -}) { - const chips: { icon: React.ReactNode; label: string; color: string }[] = []; +function PRLink({ state }: { state: PRActionState }) { + if (state.type === "gh_unavailable" || state.type === "no_pr" || state.type === "error") return null; + + return ( + + + + + #{state.prNumber} + + + +

Open PR in browser

+ + + ); +} - if (isDraft) { - chips.push({ icon: null, label: "Draft", color: "text-text-muted" }); - } - if (hasConflicts) { - chips.push({ - icon: , - label: "Conflicts", - color: "text-destructive", - }); - } - const ciChip = match(ciStatus) - .with("passing", () => ({ - icon: , - label: "CI" as const, - color: "text-success", - })) - .with("failing", () => ({ - icon: , - label: "CI" as const, - color: "text-destructive", - })) - .with("pending", () => ({ - icon: , - label: "CI" as const, - color: "text-warning", - })) - .otherwise(() => null); - if (ciChip) chips.push(ciChip); +// --------------------------------------------------------------------------- +// GhWarning -- tooltip for missing/unauthenticated gh CLI +// --------------------------------------------------------------------------- - const reviewChip = match(reviewStatus) - .with("approved", () => ({ - icon: , - label: "Approved" as const, - color: "text-success", - })) - .with("changes_requested", () => ({ - icon: , - label: "Changes" as const, - color: "text-warning", - })) - .otherwise(() => null); - if (reviewChip) chips.push(reviewChip); +function GhWarning({ reason }: { reason: "not_installed" | "not_authenticated" }) { + return ( + + + + + +

+ {reason === "not_installed" + ? "GitHub CLI not installed \u2014 install gh to manage PRs" + : "Not authenticated \u2014 run gh auth login"} +

+ + + ); +} - if (chips.length === 0) return null; +// --------------------------------------------------------------------------- +// ErrorWarning -- tooltip for GitHub connectivity errors +// --------------------------------------------------------------------------- +function ErrorWarning({ reason }: { reason: "timeout" | "network" }) { return ( -
- {chips.map((chip) => ( - + +
+ + PR + + + +

+ {reason === "timeout" + ? "GitHub request timed out \u2014 will retry automatically" + : "Could not reach GitHub \u2014 will retry automatically"} +

+ + ); } // --------------------------------------------------------------------------- -// SplitButton +// StatusText -- non-actionable status (CI pending, awaiting review, closed) // --------------------------------------------------------------------------- -interface SplitButtonProps { - icon: React.ReactNode; +const STATUS_VARIANT_CLASSES = { + pending: "bg-warning/10 text-warning", + review: "bg-primary/10 text-primary", + closed: "bg-muted text-muted-foreground", +} as const; + +function StatusText({ + icon, + label, + variant, +}: { + icon?: React.ReactNode; label: string; - onLeftClick: () => void; - leftDisabled?: boolean; - branchLabel: string; - workspacePath: string | null; - currentBranch: string; - onBranchSelect: (branch: string) => void; - /** When false, branch is shown as static text (no dropdown). Use for post-PR states. */ - branchEditable?: boolean; + variant: keyof typeof STATUS_VARIANT_CLASSES; +}) { + return ( + + {icon} + {label} + + ); } -function SplitButton({ +// --------------------------------------------------------------------------- +// ActionButton -- colored action button for fixable / actionable states +// --------------------------------------------------------------------------- + +const VARIANT_CLASSES = { + primary: "bg-primary text-primary-foreground", + destructive: "bg-destructive text-destructive-foreground", + warning: "bg-warning text-warning-foreground", + success: "bg-success text-success-foreground", +} as const; + +/** Maps PR state → link color so #N reads as part of the status unit. */ +const PR_LINK_COLORS: Record = { + ci_pending: "text-warning/80 hover:text-warning", + awaiting_review: "text-primary/80 hover:text-primary", + ci_failing: "text-destructive/80 hover:text-destructive", + conflicts: "text-destructive/80 hover:text-destructive", + changes_requested: "text-warning/80 hover:text-warning", + ready_to_merge: "text-success/80 hover:text-success", + merged: "text-text-secondary hover:text-text-primary", + closed: "text-muted-foreground/80 hover:text-muted-foreground", +}; + +function ActionButton({ icon, label, - onLeftClick, - leftDisabled, - branchLabel, + variant, + onClick, + disabled, +}: { + icon: React.ReactNode; + label: string; + variant: keyof typeof VARIANT_CLASSES; + onClick: () => void; + disabled?: boolean; +}) { + return ( + + ); +} + +// --------------------------------------------------------------------------- +// CreatePRButton -- split button with branch selector (only for no-PR state) +// --------------------------------------------------------------------------- + +function CreatePRButton({ + targetBranch, workspacePath, - currentBranch, onBranchSelect, - branchEditable = true, -}: SplitButtonProps) { + onCreatePR, +}: { + targetBranch: string; + workspacePath: string | null; + onBranchSelect: (branch: string) => void; + onCreatePR?: () => void; +}) { return (
- {branchEditable && ( - + - - )} + {targetBranch} + + +
); } diff --git a/src/features/workspace/ui/PRStatusBar.tsx b/src/features/workspace/ui/PRStatusBar.tsx deleted file mode 100644 index 8a5114b91..000000000 --- a/src/features/workspace/ui/PRStatusBar.tsx +++ /dev/null @@ -1,92 +0,0 @@ -import { GitPullRequest } from "lucide-react"; -import { Button } from "@/components/ui/button"; -import { cn } from "@/shared/lib/utils"; -import type { PRStatus } from "@/shared/types"; - -interface PRStatusBarProps { - prStatus?: PRStatus | null; - onCreatePR?: () => void; - onReviewPR?: () => void; - onMergePR?: () => void; - className?: string; - /** Compact mode — hides PR title to fit narrow panel */ - compact?: boolean; -} - -export function PRStatusBar({ - prStatus, - onCreatePR, - onReviewPR, - onMergePR, - className, - compact, -}: PRStatusBarProps) { - const hasPR = Boolean(prStatus?.has_pr && prStatus?.pr_number); - const prLabel = prStatus?.pr_number ? `PR #${prStatus.pr_number}` : "No PR"; - const mergeStatus = prStatus?.merge_status; - const isMerged = mergeStatus === "merged"; - const isBlocked = mergeStatus === "blocked"; - const mergeDisabled = !prStatus?.pr_url || isMerged || isBlocked; - const showMergeButton = Boolean(onMergePR); - const reviewLabel = showMergeButton ? "Review" : "View PR"; - - return ( -
-
-
- - {prLabel} -
- {!compact && hasPR && prStatus?.pr_title && ( - - {prStatus.pr_title} - - )} -
- -
- {!hasPR ? ( - - ) : ( - <> - - {showMergeButton && ( - - )} - - )} -
-
- ); -}