Skip to content
81 changes: 81 additions & 0 deletions PR_REVIEW_PLAN.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
# PR Review Completion Plan

Goal: make `okcode` PR Review feel comprehensive, reliable, and maintainer-grade on the existing `/_chat/pr-review` surface.

## Phase 1 — Finish the current review cockpit

### 1. Action rail polish

- [ ] Tighten the action rail layout so decision, blockers, file impact, and recent maintainer reviews read cleanly at a glance.
- [ ] Make approval blockers explicit: conflicts, failing checks, pending checks, blocked workflow steps.
- [ ] Ensure copy is compact and maintainer-oriented.

### 2. Review state clarity

- [ ] Show the current PR review decision clearly and consistently.
- [ ] Confirm the latest submitted review state refreshes correctly after comment / approve / request changes.
- [ ] Verify draft review body reset and query invalidation behavior after submit.

### 3. Review history quality

- [ ] Improve recent maintainer reviews presentation for scanability.
- [ ] Distinguish maintainer review state from plain discussion/comment noise.
- [ ] Decide whether the compact summary is enough or needs a deeper expandable history view.

## Phase 2 — Deepen review context

### 4. File-level review signal

- [ ] Surface clearer per-file review context: commented files, unresolved-thread files, reviewed files.
- [ ] Make it easier to see where maintainer attention is still needed.
- [ ] Verify selected-file behavior stays stable when data refreshes.

### 5. Blockers and workflow visibility

- [ ] Make mergeability, required checks, conflicts, and workflow blockers easier to understand from the page.
- [ ] Improve “why can’t I approve yet?” guidance.
- [ ] Ensure blocker state is visible without needing to hunt through inspector panels.

### 6. Maintainer workflow integration

- [ ] Confirm PR Review is fully discoverable through navigation and command palette.
- [ ] Identify any remaining maintainer entry points that should route into `/_chat/pr-review`.
- [ ] Avoid creating duplicate surfaces or split workflows.

## Phase 3 — End-to-end reliability

### 7. Validation and cleanup

- [ ] Run full validation for the changed slice, including typecheck if possible.
- [ ] Fix any lint/type/test issues introduced by the PR Review work.
- [ ] Keep scope tight and avoid unrelated file churn.

### 8. Real PR walkthrough

- [ ] Test the full maintainer flow against a real PR.
- [ ] Verify open review, inspect files, inspect threads, view recent maintainer reviews, submit review, and refresh behavior.
- [ ] Capture any UX or state-sync gaps found in real usage.

### 9. Final completeness pass

- [ ] Review the page as a whole for maintainer usability.
- [ ] Confirm the implementation feels like the canonical PR Review workflow, not a partial bolt-on.
- [ ] Produce a final summary of what is complete vs what is intentionally deferred.

## Execution order

1. Action rail polish
2. Review state clarity
3. Review history quality
4. File-level review signal
5. Blockers and workflow visibility
6. Maintainer workflow integration
7. Validation and cleanup
8. Real PR walkthrough
9. Final completeness pass

## Definition of done

- A maintainer can open `/_chat/pr-review`, understand PR state quickly, inspect review context, see recent maintainer activity, submit a review confidently, and trust the page to stay in sync afterward.
- The page is clearly the main PR review workflow in `okcode`.
- Validation is clean enough that we trust the implementation, not just the visuals.
3 changes: 2 additions & 1 deletion apps/server/src/doctor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,9 @@ const doctorProgram = Effect.gen(function* () {
console.log("");
console.log(" Codex: npm install -g @openai/codex && codex login");
console.log(
" Claude Code: npm install -g @anthropic-ai/claude-code && set ANTHROPIC_API_KEY or ANTHROPIC_AUTH_TOKEN",
" Claude Code: npm install -g @anthropic-ai/claude-code && claude auth login (or set ANTHROPIC_API_KEY / ANTHROPIC_AUTH_TOKEN)",
);
console.log(" Copilot: npm install -g @github/copilot && copilot login");
} else if (readyCount === statuses.length) {
console.log("All providers are ready.");
} else {
Expand Down
16 changes: 16 additions & 0 deletions apps/web/src/appSettings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,22 @@ export function getProviderStartOptions(
},
}
: {}),
...(settings.copilotBinaryPath || settings.copilotConfigDir
? {
copilot: {
...(settings.copilotBinaryPath ? { binaryPath: settings.copilotBinaryPath } : {}),
...(settings.copilotConfigDir ? { configDir: settings.copilotConfigDir } : {}),
},
}
: {}),
...(settings.openclawGatewayUrl || settings.openclawPassword
? {
openclaw: {
...(settings.openclawGatewayUrl ? { gatewayUrl: settings.openclawGatewayUrl } : {}),
...(settings.openclawPassword ? { password: settings.openclawPassword } : {}),
},
}
: {}),
};

return Object.keys(providerOptions).length > 0 ? providerOptions : undefined;
Expand Down
5 changes: 5 additions & 0 deletions apps/web/src/components/chat/ProviderSetupCard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ const PROVIDER_CONFIG = {
verifyCmd: "gh auth status",
note: undefined,
},
copilot: {
installCmd: "npm install -g @github/copilot",
authCmd: "copilot login",
verifyCmd: "gh auth status",
},
} as const;

function StatusIcon({ status }: { status: ServerProviderStatus["status"] }) {
Expand Down
189 changes: 133 additions & 56 deletions apps/web/src/components/pr-review/PrReviewShell.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,8 @@ import {

const BOOL_SCHEMA = Schema.Boolean;

function resolvePrReviewConfigPath(projectCwd: string, configPath: string): string {
if (/^(?:[A-Za-z]:[\\/]|\/)/.test(configPath)) {
return configPath;
}
return joinPath(projectCwd, configPath);
}

function formatReviewDecision(decision: string | null | undefined): string {
if (!decision) return "No decision";
if (!decision) return "No decision yet";
return decision.toLowerCase().replaceAll("_", " ");
}

Expand All @@ -78,6 +71,20 @@ function reviewDecisionTone(decision: string | null | undefined): string {
}
}

function formatConflictStatus(status: string | null | undefined): string {
if (!status) return "Conflict status unknown";
if (status === "clean") return "No merge conflicts";
if (status === "conflicted") return "Merge conflicts";
return status.replaceAll("_", " ");
}

function resolvePrReviewConfigPath(projectCwd: string, configPath: string): string {
if (/^(?:[A-Za-z]:[\\/]|\/)/.test(configPath)) {
return configPath;
}
return joinPath(projectCwd, configPath);
}

function formatReviewTimestamp(value: string): string {
const date = new Date(value);
if (Number.isNaN(date.getTime())) return value;
Expand Down Expand Up @@ -388,6 +395,28 @@ export function PrReviewShell({
const blockingWorkflowStepsComputed = (dashboardQuery.data?.workflowSteps ?? []).filter(
(step) => step.status === "blocked" || step.status === "failed",
);
const fileStats = useMemo(() => {
const files = dashboardQuery.data?.files ?? [];
return files.reduce(
(totals, file) => ({
changedFileCount: totals.changedFileCount + 1,
additions: totals.additions + file.additions,
deletions: totals.deletions + file.deletions,
}),
{ changedFileCount: 0, additions: 0, deletions: 0 },
);
}, [dashboardQuery.data?.files]);
const approvalBlockers = [
...(conflictQuery.data?.status === "conflicted" ? ["Merge conflicts must be resolved"] : []),
...checksSummary.failing.map((name) => `Failing check: ${name}`),
...checksSummary.pending.map((name) => `Pending check: ${name}`),
...blockingWorkflowStepsComputed.map((step) => `Workflow blocked: ${step.title}`),
];
const approveDisabled =
submitReviewMutation.isPending ||
conflictQuery.data?.status === "conflicted" ||
checksSummary.failing.length > 0 ||
checksSummary.pending.length > 0;
const recentReviews = dashboardQuery.data?.pullRequest.recentReviews ?? [];
const displayedRecentReviews = recentReviews.slice(0, 3);

Expand Down Expand Up @@ -521,26 +550,40 @@ export function PrReviewShell({
{/* Collapsed bar */}
<div
className={cn(
"flex h-10 items-center justify-between gap-3 px-4",
"flex min-h-10 items-center justify-between gap-3 px-4 py-2",
actionRailExpanded && "border-b border-border/50",
)}
>
<div className="flex items-center gap-3 text-xs text-muted-foreground">
<div className="flex flex-wrap items-center gap-x-3 gap-y-1 text-xs text-muted-foreground">
<span className="font-medium text-foreground">Submit review</span>
<span
className={cn(
"capitalize font-medium",
reviewDecisionTone(dashboardQuery.data?.pullRequest.reviewDecision),
)}
>
{formatReviewDecision(dashboardQuery.data?.pullRequest.reviewDecision)}
</span>
<span className="flex items-center gap-1">
<MessageSquareIcon className="size-3" />
{dashboardQuery.data?.pullRequest.unresolvedThreadCount ?? 0} open
</span>
<span>{fileStats.changedFileCount} files</span>
<span className="flex items-center gap-1">
<ShieldCheckIcon className="size-3" />
{conflictQuery.data?.status ?? "unknown"}
{formatConflictStatus(conflictQuery.data?.status)}
</span>
{blockingWorkflowStepsComputed.length > 0 ? (
{approvalBlockers.length > 0 ? (
<span className="flex items-center gap-1 text-amber-600 dark:text-amber-400">
<SparklesIcon className="size-3" />
blocked
{approvalBlockers.length} blocker{approvalBlockers.length === 1 ? "" : "s"}
</span>
) : (
<span className="flex items-center gap-1 text-emerald-600 dark:text-emerald-400">
<CheckCircle2Icon className="size-3" />
ready to approve
</span>
) : null}
)}
</div>
<Button
onClick={() => setActionRailExpanded(!actionRailExpanded)}
Expand All @@ -562,20 +605,50 @@ export function PrReviewShell({
>
<div className="overflow-hidden">
<div className="space-y-3 px-4 py-3">
<div className="flex flex-wrap items-start gap-x-4 gap-y-2 rounded-xl border border-border/60 bg-muted/30 px-3 py-2.5 text-xs">
<div className="space-y-0.5">
<div className="grid gap-2 lg:grid-cols-[minmax(0,1fr)_minmax(0,1fr)_minmax(0,1.3fr)]">
<div className="rounded-xl border border-border/60 bg-muted/30 px-3 py-2.5 text-xs">
<div className="text-[11px] uppercase tracking-wide text-muted-foreground">
Review decision
</div>
<div
className={cn(
"font-medium capitalize",
"mt-1 font-medium capitalize",
reviewDecisionTone(dashboardQuery.data?.pullRequest.reviewDecision),
)}
>
{formatReviewDecision(dashboardQuery.data?.pullRequest.reviewDecision)}
</div>
</div>
<div className="rounded-xl border border-border/60 bg-muted/30 px-3 py-2.5 text-xs">
<div className="text-[11px] uppercase tracking-wide text-muted-foreground">
File impact
</div>
<div className="mt-1 font-medium text-foreground">
{fileStats.changedFileCount}{" "}
{fileStats.changedFileCount === 1 ? "file" : "files"}, +{fileStats.additions} /
-{fileStats.deletions}
</div>
</div>
<div className="rounded-xl border border-border/60 bg-muted/30 px-3 py-2.5 text-xs">
<div className="text-[11px] uppercase tracking-wide text-muted-foreground">
Approval status
</div>
{approvalBlockers.length > 0 ? (
<ul className="mt-1 space-y-1 text-muted-foreground">
{approvalBlockers.slice(0, 4).map((blocker) => (
<li className="flex items-start gap-1.5" key={blocker}>
<AlertTriangleIcon className="mt-0.5 size-3 shrink-0 text-amber-500" />
<span>{blocker}</span>
</li>
))}
</ul>
) : (
<div className="mt-1 flex items-center gap-1.5 font-medium text-emerald-600 dark:text-emerald-400">
<CheckCircle2Icon className="size-3.5" />
Ready to approve
</div>
)}
</div>
</div>
<div className="space-y-1.5">
<div className="text-[11px] uppercase tracking-wide text-muted-foreground">
Expand Down Expand Up @@ -626,45 +699,49 @@ export function PrReviewShell({
}
}}
/>
<div className="flex flex-wrap items-center justify-end gap-2">
<Button
disabled={submitReviewMutation.isPending}
onClick={() => {
void submitReviewMutation.mutateAsync("COMMENT");
}}
size="sm"
variant="outline"
>
<MessageSquareIcon className="size-3.5" />
Comment
</Button>
<Button
disabled={
submitReviewMutation.isPending ||
conflictQuery.data?.status === "conflicted" ||
checksSummary.failing.length > 0 ||
checksSummary.pending.length > 0
}
onClick={() => {
void submitReviewMutation.mutateAsync("APPROVE");
}}
size="sm"
variant="secondary"
>
<CheckCircle2Icon className="size-3.5" />
Approve
</Button>
<Button
disabled={submitReviewMutation.isPending}
onClick={() => {
void submitReviewMutation.mutateAsync("REQUEST_CHANGES");
}}
size="sm"
variant={resolveRequestChangesButtonVariant(settings.prReviewRequestChangesTone)}
>
<AlertTriangleIcon className="size-3.5" />
Request changes
</Button>
<div className="flex flex-wrap items-center justify-between gap-2">
<div className="text-xs text-muted-foreground">
{approveDisabled
? "Approval is gated until blockers are cleared."
: "Approval is available once your summary is ready."}
</div>
<div className="flex flex-wrap items-center justify-end gap-2">
<Button
disabled={submitReviewMutation.isPending}
onClick={() => {
void submitReviewMutation.mutateAsync("COMMENT");
}}
size="sm"
variant="outline"
>
<MessageSquareIcon className="size-3.5" />
Comment
</Button>
<Button
disabled={approveDisabled}
onClick={() => {
void submitReviewMutation.mutateAsync("APPROVE");
}}
size="sm"
variant="secondary"
>
<CheckCircle2Icon className="size-3.5" />
Approve
</Button>
<Button
disabled={submitReviewMutation.isPending}
onClick={() => {
void submitReviewMutation.mutateAsync("REQUEST_CHANGES");
}}
size="sm"
variant={resolveRequestChangesButtonVariant(
settings.prReviewRequestChangesTone,
)}
>
<AlertTriangleIcon className="size-3.5" />
Request changes
</Button>
</div>
</div>
</div>
</div>
Expand Down