mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-10-07 16:19:09 +00:00
Align plan-review e2e + UI with the HEAD (pre-#1635) backend
The merge left a split plan vertical: the backend save_plan/plan_api are HEAD (we deferred the editable-plan/sandbox-publish features #1610/#1635/ #1637 per #80), but the plan UI and e2e harness were upstream's. The fake_llm scenario called save_plan(plan_file_path=...) — upstream's file-based #1635 contract — while HEAD save_plan takes plan_markdown, so the plan never saved and PlanReview never rendered (E2E failure on the plan-review locator). Pass plan_markdown to save_plan, and revert PlanReview.tsx / plan.ts / $threadId_.plan.tsx / plan_review.spec.ts to the dev baseline so the whole plan flow (save -> render -> approve -> implement) is consistent with the HEAD backend.
This commit is contained in:
parent
5c3ca6e615
commit
4b2772288d
5 changed files with 56 additions and 164 deletions
|
|
@ -226,7 +226,7 @@ def _save_plan_step(_messages: list[BaseMessage]) -> AIMessage:
|
|||
tool_calls=[
|
||||
{
|
||||
"name": "save_plan",
|
||||
"args": {"plan_file_path": PLAN_FILE_PATH},
|
||||
"args": {"plan_markdown": PLAN_MARKDOWN},
|
||||
"id": "call-save-plan",
|
||||
}
|
||||
],
|
||||
|
|
|
|||
|
|
@ -126,11 +126,10 @@ test.describe("Plan review (HTTP comments)", () => {
|
|||
await addComment(collab, "Reviewer: please also add a docstring.");
|
||||
await expect(collab.getByTestId("plan-comment")).toHaveCount(2);
|
||||
|
||||
// 5. The owner sees the collaborator's comment (polled), then approves and
|
||||
// returns to the main conversation while implementation starts.
|
||||
// 5. The owner sees the collaborator's comment (polled), then approves.
|
||||
await expect(owner.getByTestId("plan-comment")).toHaveCount(2, { timeout: 30_000 });
|
||||
await owner.getByTestId("approve-plan").click();
|
||||
await expect(owner).toHaveURL(new RegExp(`/agents/${threadId}$`));
|
||||
await expect(owner.getByTestId("plan-decision")).toContainText(/implementing/i);
|
||||
|
||||
// 6. The agent implements, opens a PR, and links it back in the Slack thread,
|
||||
// echoing the reviewers' feedback — which proves the comments were stored
|
||||
|
|
|
|||
|
|
@ -1,5 +1,4 @@
|
|||
import { useCallback, useEffect, useState } from "react"
|
||||
import { useNavigate } from "@tanstack/react-router"
|
||||
|
||||
import type { PlanComment, PlanData } from "@/lib/plan"
|
||||
import {
|
||||
|
|
@ -8,7 +7,6 @@ import {
|
|||
deletePlanComment,
|
||||
getPlanComments,
|
||||
rejectPlan,
|
||||
updatePlan,
|
||||
} from "@/lib/plan"
|
||||
import { Button } from "@/components/ui/button"
|
||||
import { Markdown } from "@/components/agents/ported"
|
||||
|
|
@ -49,7 +47,6 @@ async function copyToClipboard(text: string): Promise<boolean> {
|
|||
}
|
||||
|
||||
export function PlanReview({ plan }: { plan: PlanData }) {
|
||||
const navigate = useNavigate()
|
||||
const resolvedTheme = useResolvedTheme()
|
||||
const [comments, setComments] = useState<Array<PlanComment>>([])
|
||||
const [draft, setDraft] = useState("")
|
||||
|
|
@ -58,50 +55,6 @@ export function PlanReview({ plan }: { plan: PlanData }) {
|
|||
const [busy, setBusy] = useState<"approve" | "reject" | null>(null)
|
||||
const [error, setError] = useState<string | null>(null)
|
||||
const [copied, setCopied] = useState(false)
|
||||
// Locally track the displayed markdown so a manual edit shows immediately; the
|
||||
// route's query stops polling once a plan exists, so the prop won't refetch.
|
||||
const [markdown, setMarkdown] = useState(plan.markdown)
|
||||
const [editing, setEditing] = useState(false)
|
||||
const [editDraft, setEditDraft] = useState(plan.markdown)
|
||||
const [saving, setSaving] = useState(false)
|
||||
|
||||
// Reflect external plan updates (e.g. an agent revision) while not editing.
|
||||
useEffect(() => {
|
||||
if (!editing) setMarkdown(plan.markdown)
|
||||
}, [plan.markdown, editing])
|
||||
|
||||
const canEdit =
|
||||
plan.isOwner && plan.status !== "approved" && plan.status !== "cancelled"
|
||||
|
||||
const startEditing = useCallback(() => {
|
||||
setEditDraft(markdown)
|
||||
setEditing(true)
|
||||
setError(null)
|
||||
}, [markdown])
|
||||
|
||||
const cancelEditing = useCallback(() => {
|
||||
setEditing(false)
|
||||
setError(null)
|
||||
}, [])
|
||||
|
||||
const saveEdit = useCallback(async () => {
|
||||
const next = editDraft.trim()
|
||||
if (!next) {
|
||||
setError("The plan cannot be empty.")
|
||||
return
|
||||
}
|
||||
setSaving(true)
|
||||
setError(null)
|
||||
try {
|
||||
const result = await updatePlan(plan.threadId, next)
|
||||
setMarkdown(result.markdown)
|
||||
setEditing(false)
|
||||
} catch (e) {
|
||||
setError((e as Error).message)
|
||||
} finally {
|
||||
setSaving(false)
|
||||
}
|
||||
}, [editDraft, plan.threadId])
|
||||
|
||||
// Poll so reviewers see each other's comments without a realtime transport.
|
||||
useEffect(() => {
|
||||
|
|
@ -155,42 +108,39 @@ export function PlanReview({ plan }: { plan: PlanData }) {
|
|||
setBusy(kind)
|
||||
setError(null)
|
||||
try {
|
||||
if (kind === "approve") {
|
||||
await approvePlan(plan.threadId)
|
||||
await navigate({
|
||||
to: "/agents/$threadId",
|
||||
params: { threadId: plan.threadId },
|
||||
})
|
||||
return
|
||||
}
|
||||
await rejectPlan(plan.threadId)
|
||||
setDecision("Changes requested — the agent is revising the plan.")
|
||||
if (kind === "approve") await approvePlan(plan.threadId)
|
||||
else await rejectPlan(plan.threadId)
|
||||
setDecision(
|
||||
kind === "approve"
|
||||
? "Plan approved — the agent is implementing it."
|
||||
: "Changes requested — the agent is revising the plan."
|
||||
)
|
||||
} catch (e) {
|
||||
setError((e as Error).message)
|
||||
} finally {
|
||||
setBusy(null)
|
||||
}
|
||||
},
|
||||
[navigate, plan.threadId]
|
||||
[plan.threadId]
|
||||
)
|
||||
|
||||
const copyPlan = useCallback(async () => {
|
||||
setError(null)
|
||||
if (await copyToClipboard(markdown)) {
|
||||
if (await copyToClipboard(plan.markdown)) {
|
||||
setCopied(true)
|
||||
window.setTimeout(() => setCopied(false), 1500)
|
||||
} else {
|
||||
setError("Couldn't copy the plan to the clipboard.")
|
||||
}
|
||||
}, [markdown])
|
||||
}, [plan.markdown])
|
||||
|
||||
return (
|
||||
<div
|
||||
data-testid="plan-review"
|
||||
className="flex min-h-0 flex-1 flex-col bg-[var(--ui-bg)] text-[var(--ui-text)]"
|
||||
>
|
||||
<div className="flex flex-col gap-3 border-b border-[var(--ui-border)] px-4 py-3 md:flex-row md:items-center md:justify-between md:gap-4 md:px-6">
|
||||
<div className="min-w-0">
|
||||
<div className="flex items-center justify-between gap-4 border-b border-[var(--ui-border)] px-6 py-3">
|
||||
<div>
|
||||
<h1 className="text-base font-semibold text-[var(--ui-text)]">
|
||||
Implementation plan
|
||||
</h1>
|
||||
|
|
@ -200,105 +150,58 @@ export function PlanReview({ plan }: { plan: PlanData }) {
|
|||
<span data-testid="plan-status">{plan.status}</span>
|
||||
</p>
|
||||
</div>
|
||||
<div className="flex min-w-0 flex-wrap items-center gap-2 md:shrink-0 md:justify-end">
|
||||
<div className="flex shrink-0 items-center gap-2">
|
||||
{decision && (
|
||||
<span
|
||||
data-testid="plan-decision"
|
||||
className="w-full text-xs text-[var(--ui-text-dim)] md:w-auto"
|
||||
className="text-xs text-[var(--ui-text-dim)]"
|
||||
>
|
||||
{decision}
|
||||
</span>
|
||||
)}
|
||||
{editing ? (
|
||||
<>
|
||||
<Button
|
||||
data-testid="cancel-edit-plan"
|
||||
variant="secondary"
|
||||
disabled={saving}
|
||||
onClick={cancelEditing}
|
||||
>
|
||||
Cancel
|
||||
</Button>
|
||||
<Button
|
||||
data-testid="save-plan"
|
||||
disabled={saving || !editDraft.trim()}
|
||||
onClick={() => void saveEdit()}
|
||||
>
|
||||
{saving ? "Saving…" : "Save"}
|
||||
</Button>
|
||||
</>
|
||||
) : (
|
||||
<>
|
||||
{canEdit && (
|
||||
<Button
|
||||
data-testid="edit-plan"
|
||||
variant="secondary"
|
||||
disabled={busy !== null || decision !== null}
|
||||
onClick={startEditing}
|
||||
>
|
||||
Edit
|
||||
</Button>
|
||||
)}
|
||||
<Button
|
||||
data-testid="copy-plan"
|
||||
variant="secondary"
|
||||
disabled={!markdown.trim()}
|
||||
onClick={() => void copyPlan()}
|
||||
>
|
||||
{copied ? "Copied!" : "Copy markdown"}
|
||||
</Button>
|
||||
{plan.isOwner && (
|
||||
<Button
|
||||
data-testid="approve-plan"
|
||||
disabled={busy !== null || decision !== null}
|
||||
onClick={() => void decide("approve")}
|
||||
>
|
||||
Approve
|
||||
</Button>
|
||||
)}
|
||||
<Button
|
||||
data-testid="reject-plan"
|
||||
variant="secondary"
|
||||
// Requesting changes feeds the comments to the agent, so it's
|
||||
// meaningless with none — disable until at least one is left.
|
||||
disabled={
|
||||
busy !== null || decision !== null || comments.length === 0
|
||||
}
|
||||
title={
|
||||
comments.length === 0
|
||||
? "Leave a comment first to request changes"
|
||||
: undefined
|
||||
}
|
||||
onClick={() => void decide("reject")}
|
||||
>
|
||||
Request changes
|
||||
</Button>
|
||||
</>
|
||||
<Button
|
||||
data-testid="copy-plan"
|
||||
variant="secondary"
|
||||
disabled={!plan.markdown.trim()}
|
||||
onClick={() => void copyPlan()}
|
||||
>
|
||||
{copied ? "Copied!" : "Copy markdown"}
|
||||
</Button>
|
||||
{plan.isOwner && (
|
||||
<Button
|
||||
data-testid="approve-plan"
|
||||
disabled={busy !== null || decision !== null}
|
||||
onClick={() => void decide("approve")}
|
||||
>
|
||||
Approve
|
||||
</Button>
|
||||
)}
|
||||
<Button
|
||||
data-testid="reject-plan"
|
||||
variant="secondary"
|
||||
// Requesting changes feeds the comments to the agent, so it's
|
||||
// meaningless with none — disable until at least one is left.
|
||||
disabled={busy !== null || decision !== null || comments.length === 0}
|
||||
title={
|
||||
comments.length === 0
|
||||
? "Leave a comment first to request changes"
|
||||
: undefined
|
||||
}
|
||||
onClick={() => void decide("reject")}
|
||||
>
|
||||
Request changes
|
||||
</Button>
|
||||
</div>
|
||||
</div>
|
||||
|
||||
<div className="flex min-h-0 flex-1 flex-col overflow-y-auto md:flex-row md:overflow-hidden">
|
||||
<div className="flex min-h-0 flex-1 overflow-hidden">
|
||||
<div
|
||||
className="min-w-0 px-4 py-4 md:min-h-0 md:flex-1 md:overflow-auto md:px-6"
|
||||
className="min-h-0 flex-1 overflow-auto px-6 py-4"
|
||||
data-testid="plan-document"
|
||||
data-color-scheme={resolvedTheme}
|
||||
>
|
||||
{editing ? (
|
||||
<div className="flex h-full flex-col gap-2">
|
||||
{error && (
|
||||
<p className="text-xs text-[color:var(--ui-danger)]">{error}</p>
|
||||
)}
|
||||
<textarea
|
||||
data-testid="plan-editor"
|
||||
value={editDraft}
|
||||
onChange={(e) => setEditDraft(e.target.value)}
|
||||
spellCheck={false}
|
||||
className="min-h-[20rem] w-full flex-1 resize-none rounded-md border border-[var(--ui-border)] bg-[var(--ui-bg)] px-3 py-2 font-mono text-sm text-[var(--ui-text)] outline-none focus:border-[var(--ui-accent)]"
|
||||
/>
|
||||
</div>
|
||||
) : markdown.trim() ? (
|
||||
<Markdown content={markdown} />
|
||||
{plan.markdown.trim() ? (
|
||||
<Markdown content={plan.markdown} />
|
||||
) : (
|
||||
<p className="text-sm text-[var(--ui-text-dim)]">
|
||||
The plan hasn't been written yet.
|
||||
|
|
@ -306,14 +209,14 @@ export function PlanReview({ plan }: { plan: PlanData }) {
|
|||
)}
|
||||
</div>
|
||||
|
||||
<aside className="flex shrink-0 flex-col border-t border-[var(--ui-border)] md:w-80 md:border-t-0 md:border-l">
|
||||
<aside className="flex w-80 shrink-0 flex-col border-l border-[var(--ui-border)]">
|
||||
<div className="border-b border-[var(--ui-border)] px-4 py-3">
|
||||
<h2 className="text-sm font-semibold text-[var(--ui-text)]">
|
||||
Comments
|
||||
</h2>
|
||||
</div>
|
||||
<div
|
||||
className="max-h-80 space-y-3 overflow-auto px-4 py-3 md:max-h-none md:min-h-0 md:flex-1"
|
||||
className="min-h-0 flex-1 space-y-3 overflow-auto px-4 py-3"
|
||||
data-testid="plan-comments"
|
||||
>
|
||||
{comments.length === 0 ? (
|
||||
|
|
|
|||
|
|
@ -113,16 +113,6 @@ export function deletePlanComment(
|
|||
)
|
||||
}
|
||||
|
||||
export function updatePlan(
|
||||
threadId: string,
|
||||
markdown: string
|
||||
): Promise<{ status: PlanStatus; markdown: string }> {
|
||||
return req(`/plan/${encodeURIComponent(threadId)}`, {
|
||||
method: "PUT",
|
||||
body: JSON.stringify({ markdown }),
|
||||
})
|
||||
}
|
||||
|
||||
export function approvePlan(threadId: string): Promise<{ status: string }> {
|
||||
return req(`/plan/${encodeURIComponent(threadId)}/approve`, {
|
||||
method: "POST",
|
||||
|
|
|
|||
|
|
@ -13,7 +13,7 @@ export const Route = createFileRoute("/agents/$threadId_/plan")({
|
|||
|
||||
function Centered({ children }: { children: React.ReactNode }) {
|
||||
return (
|
||||
<div className="flex min-w-0 flex-1 items-center justify-center px-4 py-6 max-md:pt-14 md:p-6">
|
||||
<div className="flex min-w-0 flex-1 items-center justify-center p-6">
|
||||
{children}
|
||||
</div>
|
||||
)
|
||||
|
|
@ -100,7 +100,7 @@ function PlanPage() {
|
|||
|
||||
return (
|
||||
<div className="flex min-w-0 flex-1 flex-col">
|
||||
<div className="border-b border-[var(--ui-border)] px-4 pt-14 md:px-6 md:pt-3">
|
||||
<div className="border-b border-[var(--ui-border)] px-6 pt-3">
|
||||
<BackLink threadId={threadId} />
|
||||
</div>
|
||||
<PlanReview plan={plan} />
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue