diff --git a/apps/web/components/editors/tiptap/tiptap-plan-editor.test.tsx b/apps/web/components/editors/tiptap/tiptap-plan-editor.test.tsx index 15b00eca42..19b2bc5f8a 100644 --- a/apps/web/components/editors/tiptap/tiptap-plan-editor.test.tsx +++ b/apps/web/components/editors/tiptap/tiptap-plan-editor.test.tsx @@ -46,6 +46,7 @@ vi.mock("./plan-slash-menu", () => ({ PlanSlashMenu: () => null })); import { TipTapPlanEditor } from "./tiptap-plan-editor"; +const EDITOR_SELECTOR = ".ProseMirror"; const TABLE_MARKDOWN = ["| Left | Right |", "| --- | --- |", "| One | Two |"].join("\n"); function hasPluginKeyPrefix(editor: Editor, prefix: string): boolean { @@ -226,7 +227,7 @@ describe("TipTapPlanEditor mobile formatting clearance", () => { act(() => planBubble.onMobileVisibilityChange?.(true, 300, true)); - const editorContent = editorScrollContainer?.querySelector(".ProseMirror"); + const editorContent = editorScrollContainer?.querySelector(EDITOR_SELECTOR); expect(editorContent?.style.getPropertyValue("--plan-toolbar-clearance")).toBe( "max(48px, calc(348px - 3.25rem - env(safe-area-inset-bottom, 0px)))", ); @@ -341,3 +342,28 @@ describe("TipTapPlanEditor comment projection", () => { expect(onCommentDeleted).toHaveBeenCalledTimes(1); }); }); + +// @covers AC-TASKS-PLAN-COMMENTS-001.9 +it("toggles read-only without replacing the editor or publishing content", async () => { + const onChange = vi.fn(); + const onReady = vi.fn(); + const props = { taskId: "task-1", value: TABLE_MARKDOWN, onChange, onEditorReady: onReady }; + const view = render(); + await waitFor(() => expect(onReady).toHaveBeenCalledTimes(1)); + const editor = onReady.mock.calls[0][0] as Editor; + const input = view.container.querySelector(EDITOR_SELECTOR); + expect(editor.isEditable).toBe(true); + + view.rerender(); + expect(editor.isEditable).toBe(false); + expect(view.container.querySelector(EDITOR_SELECTOR)).toBe(input); + expect(input?.getAttribute("contenteditable")).toBe("false"); + + view.rerender(); + expect(editor.isEditable).toBe(true); + expect(view.container.querySelector(EDITOR_SELECTOR)).toBe(input); + expect(input?.getAttribute("contenteditable")).toBe("true"); + expect(onReady).toHaveBeenCalledTimes(1); + expect(onChange).not.toHaveBeenCalled(); + expect(getMarkdown(editor)).toContain("Left"); +}); diff --git a/apps/web/components/editors/tiptap/tiptap-plan-editor.tsx b/apps/web/components/editors/tiptap/tiptap-plan-editor.tsx index fb93e2c1ea..83d5eaa4e4 100644 --- a/apps/web/components/editors/tiptap/tiptap-plan-editor.tsx +++ b/apps/web/components/editors/tiptap/tiptap-plan-editor.tsx @@ -69,6 +69,7 @@ type TipTapPlanEditorProps = { value: string; onChange: (value: string) => void; placeholder?: string; + readOnly?: boolean; onSelectionChange?: (selection: TextSelection | null) => void; comments?: CommentForEditor[]; onCommentClick?: (id: string, position: { x: number; y: number }) => void; @@ -395,6 +396,7 @@ function usePlanEditor(props: TipTapPlanEditorProps): PlanEditorState { const editor = useEditor( { immediatelyRender: false, + editable: !props.readOnly, extensions, content: value, editorProps: { @@ -424,6 +426,10 @@ function usePlanEditor(props: TipTapPlanEditorProps): PlanEditorState { [tableResizeEnabled], ); + useLayoutEffect(() => { + editor?.setEditable(!props.readOnly, false); + }, [editor, props.readOnly]); + useEffect(() => { editorRef.current = editor; }, [editor]); @@ -496,7 +502,7 @@ export function TipTapPlanEditor(props: TipTapPlanEditorProps) { data-testid="plan-editor-scroll-container" className={cn("h-full min-h-0 overflow-y-auto overscroll-contain")} /> - {editor && isReady && ( + {editor && isReady && !props.readOnly && ( <> )} - + {!props.readOnly && ( + + )} {!isReady && (
{t("editors:loadingEditor")} diff --git a/apps/web/components/task/task-plan-panel.refresh.test.tsx b/apps/web/components/task/task-plan-panel.refresh.test.tsx new file mode 100644 index 0000000000..9d1b18d36b --- /dev/null +++ b/apps/web/components/task/task-plan-panel.refresh.test.tsx @@ -0,0 +1,230 @@ +import { act, cleanup, fireEvent, render, screen } from "@testing-library/react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import type { StoreApi } from "zustand"; +import { StateProvider, useAppStoreApi } from "@/components/state-provider"; +import type { AppState } from "@/lib/state/store"; +import type { TaskPlan, TaskPlanCommentSnapshot } from "@/lib/types/http"; +import type { TextSelection } from "@/components/editors/tiptap/tiptap-plan-editor"; + +const mocks = vi.hoisted(() => ({ + touch: false, + getTaskPlan: vi.fn(), + getTaskPlanComments: vi.fn(), + createTaskPlanComment: vi.fn(), + updateTaskPlanComment: vi.fn(), + deleteTaskPlanComment: vi.fn(), +})); +vi.mock("@/lib/api/domains/plan-api", () => ({ + getTaskPlan: mocks.getTaskPlan, + listPlanRevisions: async () => [], +})); +vi.mock("@/lib/api/domains/plan-comment-api", () => mocks); +vi.mock("@/hooks/use-compact-task-chrome", () => ({ useTouchDrawer: () => mocks.touch })); +vi.mock("./task-plan-panel-header", () => ({ PlanPanelHeader: () => null })); +vi.mock("@/hooks/domains/comments/use-plan-comment-migration", () => ({ + usePlanCommentMigration: () => ({ needsAttention: false }), +})); +vi.mock("@/lib/routing/client-dynamic", () => ({ + default: + () => + ({ + onSelectionChange, + onCommentClick, + }: { + onSelectionChange: (selection: TextSelection) => void; + onCommentClick: (id: string, position: { x: number; y: number }) => void; + }) => ( + <> + + + + ), +})); + +import { TaskPlanPanel } from "./task-plan-panel"; + +const TASK = "task-1"; +const TIMESTAMP = "2026-09-13T00:00:00Z"; +const DRAFT = "Unfinished feedback with\na second line"; +const PLACEHOLDER = "Add your comment or instruction..."; +const plan: TaskPlan = { + id: "plan-1", + task_id: TASK, + title: "Plan", + content: "Plan step", + created_by: "agent", + created_at: TIMESTAMP, + updated_at: TIMESTAMP, +}; +const snapshot: TaskPlanCommentSnapshot = { + task_id: TASK, + plan_id: plan.id, + revision: 1, + comments: [ + { + id: "comment-1", + task_id: TASK, + plan_id: plan.id, + body: "Saved feedback", + selected_text: "Plan step", + anchor_from: 1, + anchor_to: 10, + version: 1, + created_at: TIMESTAMP, + updated_at: TIMESTAMP, + }, + ], +}; +let store: StoreApi; +function CaptureStore() { + store = useAppStoreApi(); + return null; +} +function mountPanel(taskId = TASK) { + return render( + + + + , + ); +} +function openDraft(editing = false) { + fireEvent.click( + screen.getByRole("button", { name: editing ? "Edit saved comment" : "Select plan text" }), + ); + const input = screen.getByPlaceholderText(PLACEHOLDER) as HTMLTextAreaElement; + fireEvent.change(input, { target: { value: DRAFT } }); + return input; +} + +beforeEach(() => { + vi.clearAllMocks(); + mocks.getTaskPlan.mockResolvedValue(plan); + mocks.getTaskPlanComments.mockResolvedValue(snapshot); +}); +afterEach(() => { + cleanup(); + mocks.touch = false; + vi.restoreAllMocks(); +}); + +// @covers AC-TASKS-PLAN-COMMENTS-001.9, AC-TASKS-PLAN-COMMENTS-001.10 +for (const touch of [false, true]) { + describe(touch ? "phone comment refresh" : "desktop comment refresh", () => { + for (const editing of [false, true]) { + for (const failure of [false, true]) { + it(`preserves ${editing ? "unsaved edits" : "a new comment"} through ${failure ? "failed" : "successful"} foreground refresh`, async () => { + mocks.touch = touch; + mountPanel(); + await act(async () => { + store.getState().setTaskPlan(TASK, plan); + store.getState().setTaskPlanComments(TASK, snapshot); + store.getState().setConnectionStatus("connected"); + }); + const input = openDraft(editing); + let resolve!: (value: TaskPlan) => void; + let reject!: (error: Error) => void; + mocks.getTaskPlan.mockImplementationOnce( + () => + new Promise((yes, no) => { + resolve = yes; + reject = no; + }), + ); + const before = mocks.getTaskPlan.mock.calls.length; + vi.spyOn(Date, "now").mockReturnValue(Date.now() + 1000); + await act(async () => { + window.dispatchEvent(new Event("focus")); + }); + expect(mocks.getTaskPlan).toHaveBeenCalledTimes(before + 1); + expect(store.getState().taskPlans.loadingByTaskId[TASK]).toBe(true); + try { + expect(screen.queryByPlaceholderText(PLACEHOLDER)).toBe(input); + expect(input.value).toBe(DRAFT); + } finally { + await act(async () => { + if (failure) reject(new Error("offline")); + else resolve(plan); + }); + } + expect(screen.getByPlaceholderText(PLACEHOLDER)).toBe(input); + expect(input.value).toBe(DRAFT); + expect(screen.getByText(/Plan step/)).toBeTruthy(); + expect(mocks.createTaskPlanComment).not.toHaveBeenCalled(); + expect(mocks.updateTaskPlanComment).not.toHaveBeenCalled(); + const updated = { + ...snapshot, + revision: 2, + comments: [{ ...snapshot.comments[0], body: DRAFT, version: 2 }], + }; + mocks.createTaskPlanComment.mockImplementation(async ({ id }: { id: string }) => ({ + ...updated, + comments: [...snapshot.comments, { ...updated.comments[0], id }], + })); + mocks.updateTaskPlanComment.mockResolvedValue(updated); + await act(async () => { + fireEvent.click(screen.getByRole("button", { name: editing ? "Update" : "Add" })); + }); + const mutation = editing ? mocks.updateTaskPlanComment : mocks.createTaskPlanComment; + expect(mutation).toHaveBeenCalledTimes(1); + expect(mutation).toHaveBeenCalledWith( + expect.objectContaining({ body: DRAFT, taskId: TASK, planId: plan.id }), + ); + if (!editing) { + expect(mutation).toHaveBeenCalledWith( + expect.objectContaining({ selectedText: "Plan step", anchorFrom: 1, anchorTo: 10 }), + ); + } + expect(screen.queryByPlaceholderText(PLACEHOLDER)).toBeNull(); + }); + } + } + }); +} + +// @covers AC-TASKS-PLAN-COMMENTS-001.11 +it("shows initial loading without a current plan", () => { + mountPanel(); + act(() => store.getState().setTaskPlanLoading(TASK, true)); + expect(screen.queryByTestId("plan-panel")).toBeNull(); + expect(screen.getByText("Loading plan...")).toBeTruthy(); +}); + +for (const replacement of [null, { ...plan, id: "plan-2" }]) { + it(`clears the draft on confirmed plan ${replacement ? "replacement" : "deletion"}`, () => { + mountPanel(); + act(() => { + store.getState().setTaskPlan(TASK, plan); + }); + openDraft(); + act(() => { + store.getState().setTaskPlan(TASK, replacement); + }); + expect(screen.queryByPlaceholderText(PLACEHOLDER)).toBeNull(); + }); +} + +it("does not carry a draft to a different task that is loading", () => { + const view = mountPanel(); + act(() => { + store.getState().setTaskPlan(TASK, plan); + }); + openDraft(); + act(() => store.getState().setTaskPlanLoading("task-2", true)); + view.rerender( + + + + , + ); + expect(screen.queryByPlaceholderText(PLACEHOLDER)).toBeNull(); + expect(screen.queryByTestId("plan-panel")).toBeNull(); +}); diff --git a/apps/web/components/task/task-plan-panel.tsx b/apps/web/components/task/task-plan-panel.tsx index d62727008f..09b3e0862c 100644 --- a/apps/web/components/task/task-plan-panel.tsx +++ b/apps/web/components/task/task-plan-panel.tsx @@ -187,7 +187,7 @@ export const TaskPlanPanel = memo(function TaskPlanPanel({ title: state.plan?.title, }); - if (state.isLoading) { + if (state.isLoading && !state.plan) { return (
@@ -260,6 +260,7 @@ function PlanPanelContent({ key={`${taskId}-${state.editorKey}`} taskId={taskId} value={state.draftContent} + readOnly={state.isLoading} onChange={state.setDraftContent} placeholder={t("task:startTypingYourPlan")} mobileBottomOffset={mobileBottomOffset} diff --git a/apps/web/e2e/tests/kanban/swimlane-height.spec.ts b/apps/web/e2e/tests/kanban/swimlane-height.spec.ts index fbaae5d8d1..d5106444ff 100644 --- a/apps/web/e2e/tests/kanban/swimlane-height.spec.ts +++ b/apps/web/e2e/tests/kanban/swimlane-height.spec.ts @@ -129,10 +129,13 @@ test("dense and sparse workflows size independently and keep the final task reac await testPage.setViewportSize({ width: 1440, height: 900 }); await withHeightWorkflows(apiClient, seedData, async (second) => { const denseTaskCount = 439; - await seedLargeColumnTasks(apiClient, seedData, "Dense height", denseTaskCount); - const { tasks } = await apiClient.listTasks(seedData.workspaceId); - const finalTask = tasks.find((task) => task.title === `Dense height ${denseTaskCount}`); - if (!finalTask) throw new Error("final dense task was not created"); + await seedLargeColumnTasks(apiClient, seedData, "Dense height", denseTaskCount - 1); + // Create the tail after concurrent seeding so its assigned position is last. + const finalTask = await apiClient.createTask( + seedData.workspaceId, + `Dense height ${denseTaskCount}`, + { workflow_id: seedData.workflowId, workflow_step_id: seedData.startStepId }, + ); const kanban = new KanbanPage(testPage); await kanban.goto(); const dense = kanban.columnByStepId(seedData.startStepId); diff --git a/apps/web/e2e/tests/session/mobile-task-plan-comments.spec.ts b/apps/web/e2e/tests/session/mobile-task-plan-comments.spec.ts index ed3a0c2764..922676de2a 100644 --- a/apps/web/e2e/tests/session/mobile-task-plan-comments.spec.ts +++ b/apps/web/e2e/tests/session/mobile-task-plan-comments.spec.ts @@ -1,3 +1,4 @@ +import { assertCommentSurvivesForeground } from "./plan-comment-foreground-helpers"; // Filename starts with "mobile-" so this runs under the mobile-chrome project. import { test, expect } from "../../fixtures/test-base"; import { assertNoDocumentHorizontalOverflow } from "../../helpers/layout-assertions"; @@ -230,3 +231,24 @@ test.describe("mobile: task-owned plan comments", () => { ).toBe(false); }); }); + +// @covers AC-TASKS-PLAN-COMMENTS-001.9, AC-TASKS-PLAN-COMMENTS-001.10 +for (const editing of [false, true]) { + for (const failedRead of [false, true]) { + test(`foreground refresh preserves ${editing ? "edited" : "new"} comment on ${failedRead ? "failure" : "success"}`, async ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(120_000); + await assertCommentSurvivesForeground({ + testPage, + apiClient, + seedData, + mobile: true, + editing, + failedRead, + }); + }); + } +} diff --git a/apps/web/e2e/tests/session/plan-comment-foreground-helpers.ts b/apps/web/e2e/tests/session/plan-comment-foreground-helpers.ts new file mode 100644 index 0000000000..c6262f2c96 --- /dev/null +++ b/apps/web/e2e/tests/session/plan-comment-foreground-helpers.ts @@ -0,0 +1,170 @@ +import type { Page } from "@playwright/test"; +import { test, expect, type SeedData } from "../../fixtures/test-base"; +import type { ApiClient } from "../../helpers/api-client"; +import { planScript } from "../../helpers/seed-session-messages"; +import { assertNoDocumentHorizontalOverflow } from "../../helpers/layout-assertions"; +import { SessionPage } from "../../pages/session-page"; + +type Frame = { id?: string; type?: string; action?: string; payload?: { task_id?: string } }; +type Options = { + testPage: Page; + apiClient: ApiClient; + seedData: SeedData; + mobile: boolean; + editing: boolean; + failedRead: boolean; +}; +const DRAFT = "Keep this unfinished feedback\nincluding the second line"; + +async function holdForegroundRead(page: Page, taskId: string, fail: boolean) { + let armed = false; + let release: (() => void) | undefined; + let mutations = 0; + await page.routeWebSocket(/\/ws$/, (ws) => { + const server = ws.connectToServer(); + server.onMessage((message) => ws.send(message)); + ws.onMessage((message) => { + if (typeof message !== "string") return server.send(message); + for (const part of message.split("\n").filter(Boolean)) { + const frame = JSON.parse(part) as Frame; + const ownsRequest = frame.type === "request" && frame.payload?.task_id === taskId; + if ( + armed && + ownsRequest && + /task\.plan\.comments\.(create|update)/.test(frame.action ?? "") + ) + mutations++; + if (armed && !release && ownsRequest && frame.action === "task.plan.get") { + release = () => { + armed = false; + if (fail) + ws.send( + JSON.stringify({ + type: "error", + id: frame.id, + action: frame.action, + payload: { code: "INTERNAL_ERROR", message: "Injected foreground read failure" }, + }), + ); + else server.send(part); + }; + } else server.send(part); + } + }); + }); + return { + arm: () => { + armed = true; + }, + held: () => Boolean(release), + mutations: () => mutations, + release: () => release?.(), + }; +} + +async function openComment(page: Page, session: SessionPage, mobile: boolean) { + const editor = session.planPanel.locator(".ProseMirror:visible"); + await expect(editor).toBeVisible(); + await editor.focus(); + const modifier = process.platform === "darwin" ? "Meta" : "Control"; + await page.keyboard.press(`${modifier}+a`); + if (mobile) await page.getByTestId("plan-formatting-action-comment").tap(); + else await page.keyboard.press(`${modifier}+Shift+c`); +} + +export async function assertCommentSurvivesForeground(options: Options) { + const { testPage: page, apiClient, seedData, mobile, editing, failedRead } = options; + const task = await apiClient.createTaskWithAgent( + seedData.workspaceId, + "Foreground comment draft", + seedData.agentProfileId, + { + description: planScript("## Review plan\n\nPreserve this plan comment"), + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + repository_ids: [seedData.repositoryId], + }, + ); + await expect.poll(() => apiClient.getTaskPlan(task.id), { timeout: 30_000 }).not.toBeNull(); + const control = await holdForegroundRead(page, task.id, failedRead); + await page.goto(`/t/${task.id}`); + const session = new SessionPage(page); + await session.waitForLoad(); + await session.waitForChatIdle(); + await session.togglePlanMode(); + if (mobile) + await page.getByRole("navigation").getByRole("button", { name: "Plan", exact: true }).tap(); + await openComment(page, session, mobile); + const input = page.locator('textarea[placeholder="Add your comment or instruction..."]:visible'); + if (editing) { + await input.fill("Original saved feedback"); + await page.getByRole("button", { name: "Add", exact: true }).click(); + const badge = session.planPanel.locator(".comment-badge[data-comment-id]"); + await expect(badge).toHaveCount(1); + const highlight = session.planPanel + .locator(".comment-highlight") + .filter({ hasText: "Preserve this plan comment" }); + if (mobile) await highlight.tap(); + else await highlight.click(); + await expect(input).toHaveValue("Original saved feedback"); + } + await input.fill(DRAFT); + const originalInput = await input.elementHandle(); + control.arm(); + if (!mobile) { + const otherTab = await page.context().newPage(); + try { + await otherTab.bringToFront(); + await page.bringToFront(); + } finally { + await otherTab.close(); + } + } + // Headless browsers may not emit OS focus events for bringToFront. + // Dispatch the foreground event too; the production hook coalesces duplicates. + await page.evaluate(() => window.dispatchEvent(new Event("focus"))); + await expect.poll(control.held).toBe(true); + try { + await expect(session.planPanel.locator(".ProseMirror")).toHaveAttribute( + "contenteditable", + "false", + ); + await expect(input).toBeEditable(); + await expect(input).toHaveValue(DRAFT); + expect(await originalInput!.evaluate((element) => element.isConnected)).toBe(true); + expect(control.mutations()).toBe(0); + } finally { + control.release(); + } + await expect + .poll(() => + page.evaluate((taskId) => { + const win = window as Window & { + __KANDEV_E2E_STORE__?: { + getState: () => { taskPlans: { loadingByTaskId: Record } }; + }; + }; + return win.__KANDEV_E2E_STORE__?.getState().taskPlans.loadingByTaskId[taskId]; + }, task.id), + ) + .toBe(false); + await expect(input).toHaveValue(DRAFT); + expect(await originalInput!.evaluate((element) => element.isConnected)).toBe(true); + await expect(session.planPanel.locator(".ProseMirror")).toHaveAttribute( + "contenteditable", + "true", + ); + if (mobile) await assertNoDocumentHorizontalOverflow(page, "foreground comment draft"); + await page.screenshot({ path: test.info().outputPath("foreground-comment.png") }); + await page.getByRole("button", { name: editing ? "Update" : "Add", exact: true }).click(); + await expect + .poll(async () => { + const snapshot = await apiClient.wsRequest<{ comments: Array<{ body: string }> }>( + "task.plan.comments.list", + { task_id: task.id }, + ); + return snapshot.comments.map((comment) => comment.body); + }) + .toEqual([DRAFT]); + await originalInput?.dispose(); +} diff --git a/apps/web/e2e/tests/session/task-plan-comments.spec.ts b/apps/web/e2e/tests/session/task-plan-comments.spec.ts index 16294fc150..160a846367 100644 --- a/apps/web/e2e/tests/session/task-plan-comments.spec.ts +++ b/apps/web/e2e/tests/session/task-plan-comments.spec.ts @@ -1,3 +1,4 @@ +import { assertCommentSurvivesForeground } from "./plan-comment-foreground-helpers"; import type { Page } from "@playwright/test"; import { test, expect } from "../../fixtures/test-base"; import { planScript } from "../../helpers/seed-session-messages"; @@ -191,3 +192,24 @@ test.describe("task-owned plan comments", () => { }); }); }); + +// @covers AC-TASKS-PLAN-COMMENTS-001.9, AC-TASKS-PLAN-COMMENTS-001.10 +for (const editing of [false, true]) { + for (const failedRead of [false, true]) { + test(`foreground refresh preserves ${editing ? "edited" : "new"} comment on ${failedRead ? "failure" : "success"}`, async ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(120_000); + await assertCommentSurvivesForeground({ + testPage, + apiClient, + seedData, + mobile: false, + editing, + failedRead, + }); + }); + } +} diff --git a/docs/plans/plan-comment-foreground-preservation/plan.md b/docs/plans/plan-comment-foreground-preservation/plan.md new file mode 100644 index 0000000000..cca672dcef --- /dev/null +++ b/docs/plans/plan-comment-foreground-preservation/plan.md @@ -0,0 +1,185 @@ +--- +created: 2026-09-13 +status: implemented +requirements: + - REQ-TASKS-PLAN-COMMENTS-001 +system_design: + - ../../specs/tasks/system-design/plan-comments.md +legacy_specs: [] +--- + +# Implementation Plan: Plan comment foreground preservation + +## Overview + +Keep unfinished comments intact when returning to the browser triggers plan +refresh. One sequential work order owns the panel correction and component, +loader, desktop, and phone evidence. Tasks owns the contract because these +comments are feedback on its current plan. + +## Confirmed defect and evidence + +Read-only source trace on 2026-09-13: + +1. `use-plan-comments.ts` registers `useForegroundRefresh(wake, ...)`. +2. `use-foreground-refresh.ts` handles focus, visible visibilitychange, + pageshow, and online. `PlanCommentLoader.wake()` calls `load(true)`. +3. `plan-comment-loading.ts:resolvePlan` sets task-plan loading to true even + with a cached plan, retaining the cached plan during the request. +4. `task-plan-panel.tsx:TaskPlanPanel` returns only a spinner for any loading + state, unmounting `PlanPanelContent` and the comment input. Its parent hook + `usePlanSelection` retains the selection for the same task and plan. +5. After loading, both variants in `plan-selection-popover.tsx` remount with + local text initialized from `editingComment` or an empty string. New text + disappears; an unsaved edit reverts to saved text. + +Reproduce: open a plan, select text, open Comment, type without Add, switch +browser focus away and back. The refresh spinner destroys the draft. Editing +an existing comment takes the same path. This sequence was established by source trace and subsequently reproduced by +the component tests and a Chromium regression before the correction. + +## Scope + +### In scope + +- Preserve new-comment and edit-comment text through same-plan foreground, + reconnect, and background reads, including failure. +- Keep initial loading and task/plan identity isolation correct. +- Preserve existing desktop Popover and phone Drawer behavior. + +### Out of scope + +Draft recovery across reload, explicit dismissal, or task navigation; backend, +API, storage, delivery, mutation concurrency, and layout changes. + +## Technical approach + +In `apps/web/components/task/task-plan-panel.tsx`, restrict the full-panel +loading placeholder to loading without a current plan. Keep the content +subtree mounted when the current task has a cached plan. Leave shared loader +network-state semantics intact. Use current task plan state, not a sticky +"ever loaded" flag that could show task A's content for task B. +Keep `usePlanSelection` resets for task change and confirmed deletion or +replacement. While loading, keep plan content read-only and hide its editing +controls; leave the comment input editable. Restore plan editing on settlement. +Do not add autosave for unfinished comments or disable refresh. + +This follows completed [task-owned comments](../task-owned-plan-comments/plan.md) +and [recovery](../plan-comment-recovery/plan.md) packages. Their results remain +historical. Existing affected desktop/phone suites are rerun for this package; +no backend or migration work orders are reopened. + +## ASCII UI preview + +UI-01: Desktop, Plan > select text > Comment, after browser return. + +```text +Before: [Plan] -> [Loading plan...] -> [Comment: empty] +After: +Plan remains visible + +----------------------------------+ + | "Selected plan text" | + | Keep this unfinished feedback... | + | [Add] [Run] | + +----------------------------------+ +``` + +UI-02: Phone, Plan destination > Comment, after foreground return. + +```text +| Plan | +| | +| +--------------------------+ | +| | Comment | | +| | "Selected plan text" | | +| | Unfinished feedback... | | +| | [Add] [Run] | | +| +--------------------------+ | +| safe-area clearance | +``` + +Both views map to AC-001.9 through .11 below. Editing retains Update/Delete +and existing inline failure feedback. Continuous input identity, preserved +text/selection, and explicit actions are requirements; spacing is illustrative. + +The shipped `PlanSelectionDrawer` is the phone exemplar: a focused bottom +drawer for a short comment, header, one internal scroll owner, dynamic height, +safe-area padding, and 44 px actions. Keep the desktop anchored Popover. +Shared panel/loader logic owns preservation. Do not forcibly reclaim focus +from another application; the user can resume typing once foregrounded. + +## Tests + +New `apps/web/components/task/task-plan-panel.refresh.test.tsx`: + +- `preserves a new comment during plan refresh` and `preserves unsaved edits + during plan refresh`, parameterized over desktop/phone and success/failure. + Hold the read pending and assert the same textarea node, exact text, and + selected plan text before and after settlement (AC-001.9). +- No create/update/Run during refresh; one explicit Add/Update receives the + preserved body. Existing popover mutation-failure tests remain green (.10). +- Initial loading retains its placeholder. Task change and confirmed plan + deletion/replacement clear outgoing selection without stale submission (.11). + +Keep the actual panel and comment input; mock heavyweight editor/transport +boundaries. Add a real-hook/loader focus case to +`hooks/domains/comments/use-plan-comments.test.tsx` proving a request actually +occurs. Rerun loader and same-task session-switch coverage. +All abbreviated ACs in this package refer to `AC-TASKS-PLAN-COMMENTS-001`. + +## E2E tests + +Extend `apps/web/e2e/tests/session/task-plan-comments.spec.ts` (`chromium`) +and `mobile-task-plan-comments.spec.ts` (`mobile-chrome`) for new and edited +comments surviving pending, successful, and failed foreground reads (.9, .10). +Use correlated transport interception and causal waits to hold/release the +refresh response. Desktop uses a second page and `bringToFront` where +supported; deterministic foreground events supplement the loader evidence. +A synthetic event alone does not prove OS Alt-Tab behavior. Phone uses the +actual Drawer and proves preserved text, reachable actions, and no overflow. + +## Work orders + +- [x] [Task 01: Preserve the mounted comment editor](task-01-preserve-comment-editor.md) + +## Verification results + +Pre-implementation artifact checks passed on 2026-09-13: `python3 scripts/list-docs.py validate` +(267 decisions, 868 specifications), `python3 scripts/lint-spec-files.py --all`, +and `git diff --check`. The added design section was shortened to satisfy the +existing size limit. Work-order requirement IDs, design paths, and existing +verification inputs were checked. That design-only step changed no production +or permanent tests; implementation subsequently changed the panel and added +the refresh regression file. +Implementation complete: 48 targeted tests across five files, TypeScript, +zero-warning changed-file ESLint, and both full browser suites pass. Chromium +passes 8 scenarios (2.1 minutes); Pixel 5 passes 8 (1.3 minutes), both with +zero retries and `E2E_PORT_OFFSET=17`. The managed RED run built backend and +fixture artifacts; a fresh `build:e2e` after the frontend-only fix supplied +both final `--no-build` browser runs. Desktop and phone screenshots were +inspected. See the [work-order results](task-01-preserve-comment-editor.md#results) +for exact commands, initial test-setup corrections, and headless limitations. +Public docs are unchanged because this package records implementation intent. + +## Risks + +- Mocking the input hides its local-state loss. Assert actual DOM identity. +- An overbroad loading guard can expose stale content for another task. +- Headless focus differs from OS Alt-Tab; record the event mechanism tested + and a manual return check when available. + +### Review remediation + +Plan content is temporarily read-only during background reads, while comment +input remains editable. Formatting, slash, and drag controls pause until the +read settles. A browser regression failed before this guard; all 16 desktop/ +phone scenarios and 69 focused tests now pass. See the work order for exact +checks. PR CI and review completion are tracked separately. + +### CI fixture correction + +The full CI suite exposed a last-card assertion using a nondeterministically +ordered seed batch in the existing swimlane height test. The exact failure was +reproduced locally, then corrected by creating the final card after the batch. +All six swimlane cases pass with retries disabled. This test-only remediation +preserves the runtime scope; the work order records the failure and evidence. diff --git a/docs/plans/plan-comment-foreground-preservation/task-01-preserve-comment-editor.md b/docs/plans/plan-comment-foreground-preservation/task-01-preserve-comment-editor.md new file mode 100644 index 0000000000..b629ea5517 --- /dev/null +++ b/docs/plans/plan-comment-foreground-preservation/task-01-preserve-comment-editor.md @@ -0,0 +1,228 @@ +--- +id: "01-preserve-comment-editor" +title: "Preserve the mounted comment editor" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-TASKS-PLAN-COMMENTS-001 +acceptance_criteria: + - AC-TASKS-PLAN-COMMENTS-001.9 + - AC-TASKS-PLAN-COMMENTS-001.10 + - AC-TASKS-PLAN-COMMENTS-001.11 +system_design: + - ../../specs/tasks/system-design/plan-comments.md +--- + +# Task 01: Preserve the mounted comment editor + +## Summary + +Keep loaded Plan content mounted during refresh. Prove new comment text and +unsaved edits survive browser foreground return on desktop and phone. + +## In scope + +Narrow the panel loading branch, add failing-first component regressions and +real loader/foreground evidence, and add desktop/phone browser regressions. + +## Out of scope + +Backend/storage changes, refresh suppression, layout changes, recovery after +reload/dismissal/task navigation, and unrelated comments. + +## Acceptance + +1. Pending, successful, and failed same-plan refresh preserves the same input + node, text, selected text, and add/edit mode on desktop and phone. +2. Foreground return still requests authoritative state, never saves or sends + draft text, and explicit Add/Update receives the preserved value. +3. Initial loading, task change, and confirmed plan deletion/replacement never + expose or act on the outgoing draft. Existing mutation guards still pass. + +## ASCII UI preview + +Excerpt of [UI-01 and UI-02](plan.md#ascii-ui-preview), AC-001.9 through .11: + +```text +Desktop Popover: Phone bottom Drawer: + "Selected text" | Comment | + [Unfinished feedback...] | "Selected text" | + [Add] [Run] | [Unfinished feedback...] | + | [Add] [Run] | +``` + +Refresh leaves these surfaces mounted. Editing retains Update/Delete and +existing inline failure feedback. Keep phone dynamic height, one internal +scroll owner, safe-area clearance, and 44 px actions. No geometry changes. + +## Verification + +Run from repository root. Install dependencies once in this fresh worktree. +Run new component tests first against unchanged production code and record +expected failure (input removed or body reset), then implement and run: + +```bash +(cd apps && pnpm install --frozen-lockfile) +(cd apps/web && pnpm test -- components/task/task-plan-panel.refresh.test.tsx components/task/task-plan-panel.session-switch.test.tsx components/task/plan-selection-popover.test.tsx hooks/domains/comments/use-plan-comments.test.tsx hooks/domains/comments/plan-comment-loading.test.ts) +(cd apps/web && pnpm run typecheck) +(cd apps/web && pnpm exec eslint --max-warnings 0 components/task/task-plan-panel.tsx components/task/task-plan-panel.refresh.test.tsx hooks/domains/comments/use-plan-comments.test.tsx hooks/domains/comments/plan-comment-loading.test.ts e2e/tests/session/task-plan-comments.spec.ts e2e/tests/session/mobile-task-plan-comments.spec.ts e2e/tests/session/plan-comment-foreground-helpers.ts) +# Initial managed RED run builds backend and fixture plugin. If either source +# changed or those artifacts are absent, run the managed commands without --no-build. +(cd apps/web && pnpm run build:e2e) +(cd apps/web && E2E_PORT_OFFSET=17 pnpm e2e:run --no-build --project chromium tests/session/task-plan-comments.spec.ts --retries=0) +(cd apps/web && E2E_PORT_OFFSET=17 pnpm e2e:run --no-build --project mobile-chrome tests/session/mobile-task-plan-comments.spec.ts --retries=0) +python3 scripts/list-docs.py validate +python3 scripts/lint-spec-files.py --all +git diff --check +``` + +The initial managed RED run builds all backend binaries and the fixture plugin. +After the frontend-only fix, explicitly rebuild `build:e2e` before reusing those +unchanged backend artifacts with `--no-build`; never use stale assets. Check +that offset 17's backend/agentctl range is free before running, or choose and +record another free offset. Run desktop and phone sequentially against +test-base isolated data. Use causal transport +waits rather than arbitrary sleeps. Inspect the rendered phone Drawer and +desktop Popover after return. Record any headless limitation and the manual +Alt-Tab result separately. Add no automatic focus stealing. + +## Files likely touched + +- `apps/web/components/task/task-plan-panel.tsx` +- `apps/web/components/task/task-plan-panel.refresh.test.tsx` (new) +- `apps/web/hooks/domains/comments/use-plan-comments.test.tsx` +- `apps/web/hooks/domains/comments/plan-comment-loading.test.ts` +- `apps/web/e2e/tests/session/task-plan-comments.spec.ts` +- `apps/web/e2e/tests/session/mobile-task-plan-comments.spec.ts` + +## Dependencies + +None. Current backend and loader behavior supply the refresh path. + +## Risks + +Keep shared loading semantics and owner isolation. Mocking the Popover/Drawer +would hide the regression; use real inputs and assert mounted identity. + +## Parallelism + +`sequential` + +## Inputs + +- [Requirements](../../specs/tasks/requirements/plan-comments.md), AC-001.9 to .11. +- [Design](../../specs/tasks/system-design/plan-comments.md#open-comment-editor-during-background-reads). +- Existing `task-plan-panel.session-switch.test.tsx`, + `plan-selection-popover.test.tsx`, and plan-comment E2E suites. +- [Historical recovery package](../plan-comment-recovery/plan.md). + +## Results + +Implemented on 2026-09-13. + +RED: `pnpm test -- components/task/task-plan-panel.refresh.test.tsx` ran the +real panel, Popover/Drawer, Zustand store, `usePlanComments`, foreground hook, +and plan loader with deferred API responses. Eight new/edit desktop/phone +success/failure cases failed because the input became absent during loading; +four initial-load and identity-change controls passed. The regression also +asserts explicit Add/Update receives the preserved body after settlement. + +The foreground integration coverage lives in the new panel test because it +exercises the real hook/loader and input together. Existing hook and loader +test files need no changes; their suites remain in the verification command. +The E2E suites share the new `plan-comment-foreground-helpers.ts` helper; include +it in changed-file lint. Browser checks are complete. + +Final verification: + +- The five-file `pnpm test -- ...` command above passes 48 tests, including + all 12 new panel regressions (eight previously failing preservation cases + and four identity/loading controls). +- `pnpm run typecheck` and the full listed zero-warning ESLint command pass. + The corrected E2E helper also passes a final targeted ESLint/typecheck run. +- The initial managed Chromium run built backend binaries, web assets, and the + fixture plugin. Its new-comment foreground regression failed because the + textarea disappeared during the held refresh, confirming browser RED. +- After the one-condition production fix, `pnpm run build:e2e` passes and + produces fresh web assets. The backend source and fixture plugin are unchanged. +- With `E2E_PORT_OFFSET=17`, the exact `--no-build` Chromium command above + passes all 8 scenarios in 2.1 minutes, and the phone command passes all 8 + in 1.3 minutes. Both use one worker, strict WS accounting, and zero retries. +- `python3 scripts/list-docs.py validate` passes (267 decisions and 868 specs), + `python3 scripts/lint-spec-files.py --all` passes, and `git diff --check` passes. +- Desktop Popover and Pixel 5 Drawer screenshots were inspected against UI-01 + and UI-02: entered text, selected text, and explicit actions remain visible. + The phone flow also asserts no document horizontal overflow. + +An initial full desktop run encountered a fixture workspace PATCH 404 before +UI setup; its cause was not established. The full suite subsequently passed +on an explicitly free port range. The new edit scenario initially clicked a +zero-size badge wrapper; it now clicks the visible marked plan text. The stale +run was interrupted before rerunning all scenarios. No production changes were +made for either test setup issue. + +Desktop coverage changes browser tabs with `bringToFront` and also dispatches +focus because headless browsers may not emit the OS event. Phone coverage +uses the real foreground handler and Drawer. A manual OS Alt-Tab check was not +performed in this headless environment. The managed fixtures cleaned up their +owned runtime data. Screenshots and build logs are ignored or under `/tmp`. +No public-doc changes are needed: this repairs the existing comment interaction +without changing navigation, copy, APIs, or persistence. + +### Review remediation: pending plan edits + +Codex identified that keeping the plan mounted also allowed content edits while +an incoming revision could replace them. `TaskPlanPanel` now passes loading as +Tiptap read-only state. The editor remains mounted, suppresses formatting, +slash, and drag controls during the read, and resumes editing on success or +failure. The separate comment input stays editable on desktop and phone. + +The browser RED assertion expected `contenteditable=false` during the held read +and received `true` on the previous implementation. After the fix, both full +comment suites pass (8 Chromium and 8 mobile-chrome, zero retries), using +`E2E_PORT_OFFSET=17`, a fresh `pnpm run build:e2e`, and unchanged backend/plugin +binaries from the earlier managed build. The shared helper now asserts both +read-only and restored editing, plus comment editability while pending. + +The original five focused test files still pass (48 tests). Running +`pnpm test -- components/editors/tiptap/tiptap-plan-editor.test.tsx components/editors/tiptap/plan-bubble-menu.test.tsx` +adds 21 passing tests, including a real Tiptap read-only transition proving +stable editor/input identity and no content publication. TypeScript, +changed-file ESLint, documentation catalog, and specification checks pass. +Desktop/phone screenshots were recaptured after the production change. + +PR CI and reviewer completion remain pending; local checks do not establish +remote CI success. + +### Aggregate review follow-up + +CodeRabbit and Claude requested explicit create-path selection checks and a +clearer historical verification record. The refresh tests now assert +`selectedText`, `anchorFrom`, and `anchorTo` after each desktop/phone successful +or failed read. All 12 cases pass; this strengthens coverage of existing +behavior without a production change. The plan now labels the design-only +validation as pre-implementation and records the subsequent code/test changes. + +### CI remediation: deterministic last-card fixture + +E2E run 34773291709, shard 14, failed the dense/sparse swimlane last-card check +on every CI attempt. The exact test also failed locally with retries disabled. +Its concurrent task-seeding batch did not guarantee that the highest numbered +title received the final server-assigned arrival position. The UI orders by +position, so scrolling to the end did not guarantee that particular card was +visible. This expectation was introduced with the task-reordering change. + +The fixture now seeds all but one task concurrently and creates the intended +tail afterward, retaining 440 cards and every virtualization, viewport, sizing, +and navigation assertion. This is a test-only CI correction; no Kanban runtime +behavior or documented contract changes. The full six-case swimlane suite passes +with zero retries using the same fresh web assets and unchanged backend build. + +The focused case also passes four consecutive runs with `--repeat-each=4` and +`--retries=0` (33.1 seconds), following the full suite's six passes (25.3 +seconds). Commands use `E2E_PORT_OFFSET=17 pnpm e2e:run --no-build --project chromium tests/kanban/swimlane-height.spec.ts`; +the repeated run additionally selects `--grep 'dense and sparse workflows size independently'`. +Changed-file ESLint, Prettier, catalog/spec validation, and whitespace checks +pass. Final CI/review state is tracked in the task's external plan. diff --git a/docs/specs/tasks/requirements/plan-comments.md b/docs/specs/tasks/requirements/plan-comments.md index 8259e5db11..1b258b0627 100644 --- a/docs/specs/tasks/requirements/plan-comments.md +++ b/docs/specs/tasks/requirements/plan-comments.md @@ -2,7 +2,7 @@ status: active system: tasks created: 2026-09-02 -updated: 2026-09-11 +updated: 2026-09-13 owners: - kandev --- @@ -19,6 +19,9 @@ chooses a destination only when the user sends a message or selects **Run**. ## Terminology +- **Open comment draft:** Text being entered before Add or Update succeeds; + it is not yet part of the persisted pending set. + - **Pending plan comment:** An unsent comment anchored to selected text in the task's current plan. - **Selected session:** The Agent session whose composer the user is currently @@ -68,6 +71,20 @@ unsent feedback. shall hold at most 100 pending comments and 1 MiB of combined bodies and selected text. An over-limit mutation shall leave the previous snapshot intact. +- **AC-TASKS-PLAN-COMMENTS-001.9:** While adding or editing a comment on the + same current plan, switching away from the browser and returning shall + preserve the open editor, entered text, and selected plan text. Background + refresh and reconnect shall preserve that draft during pending, successful, + and failed reads on desktop and phone. Plan content editing shall pause during + the read and resume on settlement; the open comment remains editable. +- **AC-TASKS-PLAN-COMMENTS-001.10:** Preserving an open comment draft shall + not automatically persist or deliver it. Add, Update, and Run remain explicit + actions; failed mutations shall preserve the entered text for retry. +- **AC-TASKS-PLAN-COMMENTS-001.11:** Initial plan loading shall not display + another task's plan or draft. A task change, confirmed plan deletion, or + replacement of the current plan shall clear the outgoing transient editor + so its draft cannot be applied to the new context. + ### REQ-TASKS-PLAN-COMMENTS-002: Selected-session composer delivery **Intent:** Pending plan feedback can accompany a message to whichever session @@ -205,6 +222,8 @@ migration gate without changing task ownership or backend acceptance semantics. ## Implementation plans +- [Plan comment foreground preservation](../../../plans/plan-comment-foreground-preservation/plan.md) + - [Task-owned plan comments](../../../plans/task-owned-plan-comments/plan.md) records the original delivery. - [Plan comment recovery](../../../plans/plan-comment-recovery/plan.md) @@ -212,6 +231,10 @@ migration gate without changing task ownership or backend acceptance semantics. ## Out of scope +- Restoring an open, unsubmitted comment editor after page reload, task + navigation, or explicit dismissal; synchronizing that transient draft to + another browser or device. + - Changing the task-session ownership of diff, file, pull-request, walkthrough, or agent-message comments. - Keeping a separate sent-comment history. Accepted message and queue content diff --git a/docs/specs/tasks/system-design/plan-comments.md b/docs/specs/tasks/system-design/plan-comments.md index 5368c7858b..b3c26361d8 100644 --- a/docs/specs/tasks/system-design/plan-comments.md +++ b/docs/specs/tasks/system-design/plan-comments.md @@ -315,6 +315,22 @@ prompt than the user reviewed. ## Plan lifecycle +### Open comment editor during background reads + +For `AC-TASKS-PLAN-COMMENTS-001.9` through `.11`, `TaskPlanPanel` shows its +full-panel loading placeholder only without a current plan. Revalidation keeps +the content and comment Popover/Drawer mounted. Tiptap becomes read-only and +hides formatting, slash, and drag controls until settlement, preventing plan +edits that an incoming revision could overwrite. Comment input stays editable. +No draft storage, backend writes, or loader semantics change. Same-plan refresh +must not reset comment text. Task/plan identity changes still clear selection. + +Tests assert input identity and text across pending, successful, and failed +reads, initial loading and owner changes, plus desktop/phone Add/Update and +plan read-only transitions. + +### Persisted comment lifecycle + Plan content updates and revision reverts keep the same current plan row and therefore keep pending comments. On projection, the editor first tries saved positions and then the existing selected-text fallback. If an untagged user @@ -565,5 +581,7 @@ correctness boundary. ## Implementation plans +- [Plan comment foreground preservation](../../../plans/plan-comment-foreground-preservation/plan.md) + - [Task-owned plan comments](../../../plans/task-owned-plan-comments/plan.md) - [Plan comment recovery](../../../plans/plan-comment-recovery/plan.md)