From 4b2772288d8b2f1a8e67e7a250c685a7a70d7f2c Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Tue, 30 Jun 2026 16:30:36 -0400 Subject: [PATCH] Align plan-review e2e + UI with the HEAD (pre-#1635) backend MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- tests/e2e/fake_llm.py | 2 +- tests/e2e/tests/plan_review.spec.ts | 5 +- ui/src/components/agents/PlanReview.tsx | 199 ++++++----------------- ui/src/lib/plan.ts | 10 -- ui/src/routes/agents/$threadId_.plan.tsx | 4 +- 5 files changed, 56 insertions(+), 164 deletions(-) diff --git a/tests/e2e/fake_llm.py b/tests/e2e/fake_llm.py index e1f23c59..b970b0d6 100644 --- a/tests/e2e/fake_llm.py +++ b/tests/e2e/fake_llm.py @@ -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", } ], diff --git a/tests/e2e/tests/plan_review.spec.ts b/tests/e2e/tests/plan_review.spec.ts index 37d19ffa..e7637a7b 100644 --- a/tests/e2e/tests/plan_review.spec.ts +++ b/tests/e2e/tests/plan_review.spec.ts @@ -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 diff --git a/ui/src/components/agents/PlanReview.tsx b/ui/src/components/agents/PlanReview.tsx index 1dd64768..1220c4a3 100644 --- a/ui/src/components/agents/PlanReview.tsx +++ b/ui/src/components/agents/PlanReview.tsx @@ -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 { } export function PlanReview({ plan }: { plan: PlanData }) { - const navigate = useNavigate() const resolvedTheme = useResolvedTheme() const [comments, setComments] = useState>([]) 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(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 (
-
-
+
+

Implementation plan

@@ -200,105 +150,58 @@ export function PlanReview({ plan }: { plan: PlanData }) { {plan.status}

-
+
{decision && ( {decision} )} - {editing ? ( - <> - - - - ) : ( - <> - {canEdit && ( - - )} - - {plan.isOwner && ( - - )} - - + + {plan.isOwner && ( + )} +
-
+
- {editing ? ( -
- {error && ( -

{error}

- )} -