diff --git a/apps/web/components/task/chat/chat-input-area.test.tsx b/apps/web/components/task/chat/chat-input-area.test.tsx index b09602f54b..bb7631976d 100644 --- a/apps/web/components/task/chat/chat-input-area.test.tsx +++ b/apps/web/components/task/chat/chat-input-area.test.tsx @@ -1,6 +1,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { act, cleanup, fireEvent, render, renderHook, screen } from "@testing-library/react"; import type { ReactNode } from "react"; +import { planCommentRecovery } from "@/lib/plan-comment-recovery"; const toastMock = vi.fn(); const handleSendMessageMock = vi.fn(); @@ -175,7 +176,15 @@ function panelState(overrides = {}) { clearEphemeral: vi.fn(), addContextFile: vi.fn(), planModeEnabled: false, - planCommentMigration: { status: "complete", isReady: true, isBlocking: false, retry: vi.fn() }, + planCommentMigration: { + status: "complete", + pendingCount: 0, + failure: null, + needsAttention: false, + isReady: true, + isBlocking: false, + retry: vi.fn(), + }, ...overrides, } as never; } @@ -381,31 +390,61 @@ describe("useSubmitHandler plan mode", () => { }); describe("useSubmitHandler task plan comments", () => { - it("blocks delivery while legacy comments still need migration", async () => { - const { result } = renderHook(() => - useSubmitHandler( - panelState({ - planCommentMigration: { - status: "failed", - isReady: false, - isBlocking: true, - retry: vi.fn(), - }, - }), - ), - ); + it.each(["idle", "retrying", "failed"] as const)( + "accepts plain Send with no identified drafts during %s recovery", + async (status) => { + const { result } = renderHook(() => + useSubmitHandler( + panelState({ + planCommentMigration: { + ...planCommentRecovery({ status, pendingCount: 0, failure: "transient" }), + retry: vi.fn(), + }, + }), + ), + ); + await act(async () => { + await expect(result.current.handleSubmit({ message: "Send my message" })).resolves.toBe( + true, + ); + }); + expect(handleSendMessageMock).toHaveBeenCalledWith({ message: "Send my message" }); + expect(toastMock).not.toHaveBeenCalled(); + }, + ); + it.each(["transient", "conflict", "rejected"] as const)( + "preserves blocked delivery without promising retries for %s recovery", + async (failure) => { + const { result } = renderHook(() => + useSubmitHandler( + panelState({ + planCommentMigration: { + status: "failed", + pendingCount: 1, + failure, + needsAttention: true, + isReady: false, + isBlocking: true, + retry: vi.fn(), + }, + }), + ), + ); - await act(async () => { - await expect(result.current.handleSubmit({ message: "Keep my draft" })).resolves.toBe(false); - }); + await act(async () => { + await expect(result.current.handleSubmit({ message: "Keep my draft" })).resolves.toBe( + false, + ); + }); - expect(handleSendMessageMock).not.toHaveBeenCalled(); - expect(toastMock).toHaveBeenCalledWith({ - title: "Message not sent", - description: "Saved plan comments are still being restored. Retry before sending.", - variant: "error", - }); - }); + expect(handleSendMessageMock).not.toHaveBeenCalled(); + expect(toastMock).toHaveBeenCalledWith({ + title: "Message not sent", + description: "Saved plan comments are still being restored. Your message is kept.", + variant: "error", + }); + }, + ); it("submits displayed IDs and versions without clearing the shared snapshot locally", async () => { const clearSessionPlanComments = vi.fn(); @@ -418,7 +457,16 @@ describe("useSubmitHandler task plan comments", () => { selectedText: "Large step", }; const { result } = renderHook(() => - useSubmitHandler(panelState({ planComments: [comment], clearSessionPlanComments })), + useSubmitHandler( + panelState({ + planComments: [comment], + clearSessionPlanComments, + planCommentMigration: { + ...planCommentRecovery({ status: "failed", pendingCount: 0, failure: "transient" }), + retry: vi.fn(), + }, + }), + ), ); await act(async () => { diff --git a/apps/web/components/task/passthrough-chat-composer.test.ts b/apps/web/components/task/passthrough-chat-composer.test.ts index c88c08d01f..91e166c41e 100644 --- a/apps/web/components/task/passthrough-chat-composer.test.ts +++ b/apps/web/components/task/passthrough-chat-composer.test.ts @@ -2,6 +2,7 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import type { ContextFile } from "@/lib/state/context-files-store"; import type { DiffComment } from "@/lib/diff/types"; import type { TaskMentionData } from "@/hooks/use-inline-mention"; +import { planCommentRecovery } from "@/lib/plan-comment-recovery"; import { buildContextFilesMeta, buildPassthroughFinalMessage, @@ -88,7 +89,15 @@ function panelState(overrides: Record = {}) { walkthroughComments: [], messageComments: [], planModeEnabled: false, - planCommentMigration: { status: "complete", isReady: true, isBlocking: false, retry: vi.fn() }, + planCommentMigration: { + status: "complete", + pendingCount: 0, + failure: null, + needsAttention: false, + isReady: true, + isBlocking: false, + retry: vi.fn(), + }, handleClearPRFeedback: vi.fn(), clearSessionPlanComments: vi.fn(), handleClearWalkthroughComments: vi.fn(), @@ -286,20 +295,38 @@ describe("passthrough chat composer plan context", () => { }); describe("passthrough chat composer cleanup", () => { - it("rejects delivery while legacy plan comments are not migrated", () => { + it("accepts plain delivery after background read failures without identified drafts", () => { expect(() => assertPlanCommentMigrationReady( panelState({ planCommentMigration: { - status: "failed", - isReady: false, - isBlocking: true, + ...planCommentRecovery({ status: "failed", pendingCount: 0, failure: "transient" }), retry: vi.fn(), }, }), ), - ).toThrow("Saved plan comments are still being restored. Retry before sending."); + ).not.toThrow(); }); + it.each(["transient", "conflict", "rejected"] as const)( + "preserves blocked delivery without promising retries for %s recovery", + (failure) => { + expect(() => + assertPlanCommentMigrationReady( + panelState({ + planCommentMigration: { + status: "failed", + pendingCount: 1, + failure, + needsAttention: true, + isReady: false, + isBlocking: true, + retry: vi.fn(), + }, + }), + ), + ).toThrow("Saved plan comments are still being restored. Your message is kept."); + }, + ); it("clears session context but leaves task plan comments to the backend snapshot", () => { const state = panelState({ diff --git a/apps/web/components/task/plan-comment-migration-notice.test.tsx b/apps/web/components/task/plan-comment-migration-notice.test.tsx new file mode 100644 index 0000000000..2703f78153 --- /dev/null +++ b/apps/web/components/task/plan-comment-migration-notice.test.tsx @@ -0,0 +1,59 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { cleanup, fireEvent, render, screen } from "@testing-library/react"; +import { planCommentRecovery } from "@/lib/plan-comment-recovery"; +import { PlanCommentMigrationNotice } from "./plan-comment-migration-notice"; + +afterEach(cleanup); + +describe("plan comment recovery feedback", () => { + it.each(["idle", "running", "retrying", "complete", "failed", "waiting_for_plan"] as const)( + "stays quiet for empty %s recovery", + (status) => { + render( + , + ); + expect(screen.queryByTestId("plan-comment-migration-notice")).toBeNull(); + }, + ); + + it("stays quiet while real drafts retry automatically", () => { + render( + , + ); + expect(screen.queryByRole("alert")).toBeNull(); + expect(screen.queryByTestId("plan-comment-migration-notice")).toBeNull(); + }); + + it("offers explicit Retry for identified feedback needing attention", () => { + const retry = vi.fn(); + render( + , + ); + expect(screen.getByRole("alert")).toBeTruthy(); + fireEvent.click(screen.getByRole("button", { name: "Retry" })); + expect(retry).toHaveBeenCalledOnce(); + }); + + it("offers Retry with a status notice when saved feedback needs a plan", () => { + const retry = vi.fn(); + render( + , + ); + expect(screen.getByRole("status")).toBeTruthy(); + expect(screen.queryByRole("alert")).toBeNull(); + fireEvent.click(screen.getByRole("button", { name: "Retry" })); + expect(retry).toHaveBeenCalledOnce(); + }); +}); diff --git a/apps/web/components/task/plan-comment-migration-notice.tsx b/apps/web/components/task/plan-comment-migration-notice.tsx index 9e490787fd..2bab8769e8 100644 --- a/apps/web/components/task/plan-comment-migration-notice.tsx +++ b/apps/web/components/task/plan-comment-migration-notice.tsx @@ -1,48 +1,44 @@ "use client"; -import { IconAlertTriangle, IconLoader2 } from "@tabler/icons-react"; +import { IconAlertTriangle } from "@tabler/icons-react"; import { Button } from "@kandev/ui/button"; import { useTranslation } from "react-i18next"; import type { PlanCommentMigrationStatus } from "@/lib/state/slices/session/types"; type PlanCommentMigrationNoticeProps = { status: PlanCommentMigrationStatus; + needsAttention: boolean; retry: () => void; }; -export function PlanCommentMigrationNotice({ status, retry }: PlanCommentMigrationNoticeProps) { +export function PlanCommentMigrationNotice({ + status, + needsAttention, + retry, +}: PlanCommentMigrationNoticeProps) { const { t } = useTranslation("task"); - if (status === "complete") return null; + if (!needsAttention) return null; const isFailed = status === "failed"; - const isWaitingForPlan = status === "waiting_for_plan"; - let message = t("restoringSavedPlanComments"); - if (isFailed) message = t("planCommentMigrationFailed"); - else if (isWaitingForPlan) message = t("planCommentMigrationNeedsPlan"); + const message = isFailed ? t("planCommentMigrationFailed") : t("planCommentMigrationNeedsPlan"); return (
- {isFailed || isWaitingForPlan ? ( - - ) : ( - - )} + {message} - {isFailed && ( - - )} +
); } diff --git a/apps/web/components/task/task-plan-panel.tsx b/apps/web/components/task/task-plan-panel.tsx index 5ab5277a05..d62727008f 100644 --- a/apps/web/components/task/task-plan-panel.tsx +++ b/apps/web/components/task/task-plan-panel.tsx @@ -309,10 +309,8 @@ function removeCommentMark(editor: Editor | null, commentId: string) { } function planCommentRunDisabledReason( - migrationReady: boolean, reason: ReturnType["reason"], ) { - if (!migrationReady) return t("task:planCommentMigrationPending"); if (reason === "no-primary-session") return t("task:noPrimarySessionForPlanComment"); if (reason === "primary-session-unavailable") { return t("task:primarySessionUnavailableForPlanComment"); @@ -434,10 +432,7 @@ function PlanSelectionPopoverWrapper({ const runUnavailableReason = useAppStore( (state) => resolvePlanCommentRunAvailability(state, taskId).reason, ); - const migrationReady = useAppStore((state) => - taskId ? state.taskPlans.commentsMigrationStatusByTaskId[taskId] === "complete" : false, - ); - const runDisabledReason = planCommentRunDisabledReason(migrationReady, runUnavailableReason); + const runDisabledReason = planCommentRunDisabledReason(runUnavailableReason); const { handleAdd, handleAddAndRun, runError } = usePlanSelectionCommentActions({ textSelection, commentState, diff --git a/apps/web/e2e/pages/session-page.ts b/apps/web/e2e/pages/session-page.ts index 43187f24f1..d5dc172f7b 100644 --- a/apps/web/e2e/pages/session-page.ts +++ b/apps/web/e2e/pages/session-page.ts @@ -654,9 +654,12 @@ export class SessionPage { return this.page.getByTestId("reset-context-confirm"); } - /** "Resume session" button shown after agent crash. */ + /** Observe the first visible recovery action, including while it is already resuming. */ recoveryResumeButton(): Locator { - return this.page.getByTestId("recovery-resume-button"); + return this.activeChat() + .getByTestId("recovery-resume-button") + .filter({ visible: true }) + .first(); } /** Error returned by a manual session recovery action. */ 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 484d9b6b36..ed3a0c2764 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 @@ -3,6 +3,10 @@ import { test, expect } from "../../fixtures/test-base"; import { assertNoDocumentHorizontalOverflow } from "../../helpers/layout-assertions"; import { planScript } from "../../helpers/seed-session-messages"; import { SessionPage } from "../../pages/session-page"; +import { + assertPlainSendDuringRecovery, + assertLegacyRecoveryPreservesDraft, +} from "./plan-comment-recovery-helpers"; const PLAN_CONTENT = "## Mobile shared plan\n\nReview this mobile implementation step"; const DONE_STATES = ["COMPLETED", "WAITING_FOR_INPUT"]; @@ -10,6 +14,29 @@ const LEGACY_PRIMARY_ID = "11111111-1111-4111-8111-111111111111"; const LEGACY_SECONDARY_ID = "22222222-2222-4222-8222-222222222222"; test.describe("mobile: task-owned plan comments", () => { + // @covers AC-TASKS-PLAN-COMMENTS-004.5, AC-TASKS-PLAN-COMMENTS-004.8 + for (const withPlan of [false, true]) { + test(`plain Send survives failed comment reads ${withPlan ? "with" : "without"} a plan`, async ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(120_000); + await assertPlainSendDuringRecovery( + { testPage, apiClient, seedData, mobile: true }, + withPlan, + ); + }); + } + // @covers AC-TASKS-PLAN-COMMENTS-004.4, AC-TASKS-PLAN-COMMENTS-004.6, AC-TASKS-PLAN-COMMENTS-004.8 + test("resumes feedback recovery and retains the mobile draft and focus", async ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(120_000); + await assertLegacyRecoveryPreservesDraft({ testPage, apiClient, seedData, mobile: true }); + }); // @covers AC-TASKS-PLAN-COMMENTS-001.7 // @covers AC-TASKS-PLAN-COMMENTS-003.2 // @covers AC-TASKS-PLAN-COMMENTS-004.1 diff --git a/apps/web/e2e/tests/session/plan-comment-recovery-helpers.ts b/apps/web/e2e/tests/session/plan-comment-recovery-helpers.ts new file mode 100644 index 0000000000..f66965ff48 --- /dev/null +++ b/apps/web/e2e/tests/session/plan-comment-recovery-helpers.ts @@ -0,0 +1,297 @@ +import type { Page } from "@playwright/test"; +import { test, expect, type SeedData } from "../../fixtures/test-base"; +import type { ApiClient } from "../../helpers/api-client"; +import { assertNoDocumentHorizontalOverflow } from "../../helpers/layout-assertions"; +import { planScript } from "../../helpers/seed-session-messages"; +import { SessionPage } from "../../pages/session-page"; + +type Frame = { + id?: string; + type?: string; + action?: string; + payload?: { task_id?: string; id?: string }; +}; +type RecoveryOptions = { + testPage: Page; + apiClient: ApiClient; + seedData: SeedData; + mobile: boolean; +}; +const LEGACY_ID = "33333333-3333-4333-8333-333333333333"; +const FEEDBACK = "Keep this recovered feedback"; + +function frame(value: string): Frame | null { + try { + return JSON.parse(value) as Frame; + } catch { + return null; + } +} + +function matchesRequest( + parsed: Frame | null, + taskId: string, + actions: string[], +): parsed is Frame & { id: string } { + return ( + parsed?.type === "request" && + typeof parsed.id === "string" && + parsed.payload?.task_id === taskId && + actions.includes(parsed.action ?? "") + ); +} + +/** Reject only the selected task's correlated reads/uploads; keep chat and other frames live. */ +async function interceptPlanRequests( + page: Page, + taskId: string, + actions: string[], + failedCommentId?: string, +) { + let blocked = true; + let rejected = 0; + let succeeded = 0; + await page.routeWebSocket(/\/ws$/, (ws) => { + const server = ws.connectToServer(); + const requestIds = new Set(); + ws.onMessage((message) => { + if (typeof message !== "string") { + server.send(message); + return; + } + const forwarded: string[] = []; + for (const part of message.split("\n")) { + const parsed = frame(part); + const matches = matchesRequest(parsed, taskId, actions); + if (matches && blocked && (!failedCommentId || parsed.payload?.id === failedCommentId)) { + rejected++; + ws.send( + JSON.stringify({ + type: "error", + id: parsed.id, + action: parsed.action, + payload: { + code: "INTERNAL_ERROR", + message: "Injected transient plan request failure", + }, + }), + ); + } else { + if (matches) requestIds.add(parsed.id!); + if (part) forwarded.push(part); + } + } + if (forwarded.length) server.send(forwarded.join("\n")); + }); + server.onMessage((message) => { + if (typeof message === "string") + for (const part of message.split("\n")) { + const parsed = frame(part); + if (parsed?.type === "response" && parsed.id && requestIds.delete(parsed.id)) succeeded++; + } + ws.send(message); + }); + }); + return { + rejected: () => rejected, + succeeded: () => succeeded, + release: () => { + blocked = false; + }, + }; +} + +async function seedRecoveryTask({ apiClient, seedData }: RecoveryOptions, withPlan: boolean) { + const task = await apiClient.createTaskWithAgent( + seedData.workspaceId, + "Plan comment recovery", + seedData.agentProfileId, + { + description: withPlan + ? planScript("## Recovery plan\n\nPreserve feedback") + : "/e2e:simple-message", + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + repository_ids: [seedData.repositoryId], + }, + ); + if (!task.session_id) throw new Error("Expected a task session"); + await expect + .poll( + async () => { + const { sessions } = await apiClient.listTaskSessions(task.id); + return sessions.some( + (session) => + session.id === task.session_id && + ["COMPLETED", "WAITING_FOR_INPUT"].includes(session.state), + ); + }, + { timeout: 60_000 }, + ) + .toBe(true); + if (withPlan) + await expect.poll(() => apiClient.getTaskPlan(task.id), { timeout: 30_000 }).not.toBeNull(); + return { ...task, session_id: task.session_id }; +} + +async function showComposer(page: Page, taskId: string) { + await page.goto(`/t/${taskId}`); + const session = new SessionPage(page); + // Do not use the reload-capable readiness helpers while proving recovery. + await expect(session.activeChat()).toBeVisible({ timeout: 30_000 }); + await expect(session.anyIdleInput()).toBeVisible({ timeout: 30_000 }); + const editor = session + .activeChat() + .locator('.tiptap.ProseMirror[contenteditable="true"]') + .first(); + await expect(editor).toBeEditable(); + return { session, editor }; +} + +async function submit(session: SessionPage, mobile: boolean) { + if (mobile) await session.tapSubmitWhenReady(); + else await session.clickSubmitWhenReady(); +} + +export async function assertPlainSendDuringRecovery(options: RecoveryOptions, withPlan: boolean) { + const { testPage, apiClient, mobile } = options; + const task = await seedRecoveryTask(options, withPlan); + const control = await interceptPlanRequests(testPage, task.id, [ + "task.plan.get", + "task.plan.comments.list", + ]); + const { session, editor } = await showComposer(testPage, task.id); + await expect.poll(control.rejected, { timeout: 15_000 }).toBeGreaterThan(0); + const notice = testPage.getByTestId("plan-comment-migration-notice"); + await expect(notice).toHaveCount(0); + const message = "Plain message while comment reads are unavailable"; + if (mobile) await editor.tap(); + else await editor.click(); + await editor.fill(message); + await testPage.screenshot({ path: test.info().outputPath("plan-comment-recovery-empty.png") }); + await submit(session, mobile); + await expect + .poll( + async () => + (await apiClient.listSessionMessages(task.session_id)).messages.filter( + (entry) => entry.author_type === "user" && entry.content.includes(message), + ).length, + { timeout: 30_000 }, + ) + .toBe(1); + await expect(editor).toHaveText(""); + control.release(); + await testPage.evaluate(() => window.dispatchEvent(new Event("focus"))); + await expect.poll(control.succeeded, { timeout: 15_000 }).toBeGreaterThan(0); + await expect(notice).toHaveCount(0); + if (mobile) await assertNoDocumentHorizontalOverflow(testPage, "empty plan comment recovery"); +} + +async function seedMixedFeedback(page: Page, sessionId: string) { + const legacy = { + sessionId, + source: "plan", + selectedText: "Preserve feedback", + from: 1, + to: 10, + createdAt: "2026-09-02T00:00:00Z", + status: "pending", + }; + const diff = { + id: "legacy-diff", + sessionId, + source: "diff", + text: "Keep this diff feedback", + filePath: "src/app.ts", + startLine: 1, + endLine: 1, + side: "additions", + codeContent: "code", + createdAt: legacy.createdAt, + status: "pending", + }; + const rows = [ + { ...legacy, id: "44444444-4444-4444-8444-444444444444", text: "Already recovered feedback" }, + { ...legacy, id: LEGACY_ID, text: FEEDBACK }, + diff, + ]; + await page.addInitScript( + ({ sessionId, rows }) => { + sessionStorage.setItem(`kandev.comments.${sessionId}`, JSON.stringify(rows)); + }, + { sessionId, rows }, + ); + return diff; +} + +export async function assertLegacyRecoveryPreservesDraft(options: RecoveryOptions) { + const { testPage, apiClient, mobile } = options; + const task = await seedRecoveryTask(options, true); + const control = await interceptPlanRequests( + testPage, + task.id, + ["task.plan.comments.create"], + LEGACY_ID, + ); + const diff = await seedMixedFeedback(testPage, task.session_id); + const { session, editor } = await showComposer(testPage, task.id); + await expect.poll(control.rejected, { timeout: 15_000 }).toBeGreaterThanOrEqual(3); + await expect(session.activeChat().getByText("1 plan comment", { exact: true })).toBeVisible(); + const notice = session.activeChat().getByTestId("plan-comment-migration-notice"); + await expect(notice).toBeVisible(); + await testPage.screenshot({ + path: test.info().outputPath("plan-comment-recovery-attention.png"), + }); + if (mobile) { + const retrySize = await notice + .getByRole("button", { name: "Retry", exact: true }) + .boundingBox(); + expect(retrySize?.height).toBeGreaterThanOrEqual(44); + await assertNoDocumentHorizontalOverflow(testPage, "actionable plan comment recovery"); + await editor.tap(); + } else await editor.click(); + const message = "Retain this draft until feedback recovers"; + await editor.fill(message); + await submit(session, mobile); + await expect(testPage.getByText("Message not sent", { exact: true })).toBeVisible(); + await expect(editor).toHaveText(message); + await editor.focus(); + control.release(); + // A browser resume is enough; no Retry, reload, or submit is invoked by recovery. + await testPage.evaluate(() => { + window.dispatchEvent(new Event("focus")); + window.dispatchEvent(new Event("pageshow")); + document.dispatchEvent(new Event("visibilitychange")); + }); + await expect.poll(control.succeeded, { timeout: 15_000 }).toBe(2); + await expect(session.activeChat().getByText("2 plan comments", { exact: true })).toBeVisible(); + await expect(notice).toHaveCount(0); + await expect(editor).toHaveText(message); + await expect(editor).toBeFocused(); + expect( + (await apiClient.listSessionMessages(task.session_id)).messages.some((entry) => + entry.content.includes(message), + ), + ).toBe(false); + expect( + await testPage.evaluate( + (sessionId) => JSON.parse(sessionStorage.getItem(`kandev.comments.${sessionId}`) ?? "[]"), + task.session_id, + ), + ).toEqual([diff]); + expect(control.succeeded()).toBe(2); + await submit(session, mobile); + await expect + .poll( + async () => + (await apiClient.listSessionMessages(task.session_id)).messages.filter( + (entry) => + entry.author_type === "user" && + entry.content.includes(message) && + entry.content.includes(FEEDBACK) && + entry.content.includes(diff.text), + ).length, + { timeout: 30_000 }, + ) + .toBe(1); +} diff --git a/apps/web/e2e/tests/session/session-page-recovery.spec.ts b/apps/web/e2e/tests/session/session-page-recovery.spec.ts new file mode 100644 index 0000000000..38d42b765e --- /dev/null +++ b/apps/web/e2e/tests/session/session-page-recovery.spec.ts @@ -0,0 +1,37 @@ +import { test, expect } from "../../fixtures/test-base"; +import { SessionPage } from "../../pages/session-page"; + +test("idle recovery ignores background controls and observes an in-flight resume", async ({ + testPage, +}) => { + await testPage.setContent(` + +
+ + + +
+ `); + await testPage.evaluate(() => { + document.getElementById("duplicate")!.addEventListener("click", () => { + document.body.dataset.duplicateClicked = "true"; + }); + document.getElementById("primary")!.addEventListener("click", () => { + document.getElementById("primary")!.remove(); + document.getElementById("duplicate")!.remove(); + document.getElementById("editor")!.hidden = false; + }); + }); + const session = new SessionPage(testPage); + await expect(session.recoveryResumeButton()).toHaveCount(1); + await expect(session.recoveryResumeButton()).toBeDisabled(); + await testPage.locator("#primary").evaluate((button: HTMLButtonElement) => { + button.disabled = false; + }); + await session.waitForChatIdle({ timeout: 5_000, requireEditable: true }); + await expect(session.anyIdleInput()).toBeVisible(); + await expect(testPage.locator("body")).not.toHaveAttribute("data-duplicate-clicked", "true"); +}); 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 e5b7bdf8c5..16294fc150 100644 --- a/apps/web/e2e/tests/session/task-plan-comments.spec.ts +++ b/apps/web/e2e/tests/session/task-plan-comments.spec.ts @@ -5,6 +5,10 @@ import { waitForStableActiveSession } from "../../helpers/session-store"; import type { ApiClient } from "../../helpers/api-client"; import type { SeedData } from "../../fixtures/test-base"; import { SessionPage } from "../../pages/session-page"; +import { + assertPlainSendDuringRecovery, + assertLegacyRecoveryPreservesDraft, +} from "./plan-comment-recovery-helpers"; const DONE_STATES = ["COMPLETED", "WAITING_FOR_INPUT"]; const PLAN_CONTENT = "## Shared plan\n\nReview this shared implementation step"; @@ -65,6 +69,29 @@ async function openPlanCommentComposer(page: Page, session: SessionPage) { } test.describe("task-owned plan comments", () => { + // @covers AC-TASKS-PLAN-COMMENTS-004.5, AC-TASKS-PLAN-COMMENTS-004.6 + for (const withPlan of [false, true]) { + test(`plain Send survives failed comment reads ${withPlan ? "with" : "without"} a plan`, async ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(120_000); + await assertPlainSendDuringRecovery( + { testPage, apiClient, seedData, mobile: false }, + withPlan, + ); + }); + } + // @covers AC-TASKS-PLAN-COMMENTS-004.3, AC-TASKS-PLAN-COMMENTS-004.4, AC-TASKS-PLAN-COMMENTS-004.6 + test("automatically restores real feedback without submitting or losing the draft", async ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(120_000); + await assertLegacyRecoveryPreservesDraft({ testPage, apiClient, seedData, mobile: false }); + }); // @covers AC-TASKS-PLAN-COMMENTS-001.2 // @covers AC-TASKS-PLAN-COMMENTS-002.2 // @covers AC-TASKS-PLAN-COMMENTS-002.5 diff --git a/apps/web/hooks/domains/comments/plan-comment-loading.test.ts b/apps/web/hooks/domains/comments/plan-comment-loading.test.ts new file mode 100644 index 0000000000..a2018ddcf0 --- /dev/null +++ b/apps/web/hooks/domains/comments/plan-comment-loading.test.ts @@ -0,0 +1,147 @@ +import { afterEach, beforeEach, expect, it, vi } from "vitest"; +import { createAppStore } from "@/lib/state/store"; +import type { TaskPlan, TaskPlanCommentSnapshot } from "@/lib/types/http"; +import { planCommentLoaderFor } from "./plan-comment-loading"; + +const api = vi.hoisted(() => ({ getTaskPlan: vi.fn() })); +vi.mock("@/lib/api/domains/plan-api", () => api); +const comments = vi.hoisted(() => ({ getTaskPlanComments: vi.fn() })); +vi.mock("@/lib/api/domains/plan-comment-api", () => comments); +const releases: Array<() => void> = []; +const DATE = "2026-09-02T00:00:00Z"; +const ERROR_MESSAGE = "Could not load comments"; + +beforeEach(() => { + vi.useFakeTimers(); + vi.resetAllMocks(); +}); +afterEach(() => { + releases.splice(0).forEach((release) => release()); + vi.restoreAllMocks(); + vi.useRealTimers(); +}); + +it("releases loading flags when plan hydration invalidates an in-flight read", async () => { + const taskId = "task-1"; + const plan: TaskPlan = { + id: "plan-1", + task_id: taskId, + title: "Plan", + content: "Step", + created_by: "agent", + created_at: DATE, + updated_at: DATE, + }; + const snapshot: TaskPlanCommentSnapshot = { + task_id: taskId, + plan_id: plan.id, + revision: 1, + comments: [], + }; + const store = createAppStore(); + store.getState().setTaskPlan(taskId, plan); + store.setState((state) => ({ taskPlans: { ...state.taskPlans, loadedByTaskId: {} } })); + store.getState().setConnectionStatus("connected"); + const loader = planCommentLoaderFor(store, taskId, ERROR_MESSAGE); + releases.push(loader.attach()); + let resolve!: (value: TaskPlanCommentSnapshot) => void; + comments.getTaskPlanComments.mockImplementationOnce( + () => + new Promise((done) => { + resolve = done; + }), + ); + const reading = loader.load(false); + await vi.advanceTimersByTimeAsync(0); + expect(store.getState().taskPlans.commentsLoadingByTaskId[taskId]).toBe(true); + store.getState().setTaskPlan(taskId, plan); + resolve(snapshot); + await reading; + expect(store.getState().taskPlans.commentsLoadingByTaskId[taskId]).toBe(false); + expect(store.getState().taskPlans.loadingByTaskId[taskId]).toBe(false); + expect(store.getState().taskPlans.commentsByTaskId[taskId]).toBeUndefined(); + comments.getTaskPlanComments.mockResolvedValue(snapshot); + await loader.load(false); + expect(store.getState().taskPlans.commentsByTaskId[taskId]).toEqual(snapshot); +}); + +it("restarts the background retry delay when the plan is replaced", async () => { + const taskId = "task-1"; + const plan: TaskPlan = { + id: "plan-1", + task_id: taskId, + title: "Plan", + content: "Step", + created_by: "agent", + created_at: DATE, + updated_at: DATE, + }; + const store = createAppStore(); + store.getState().setTaskPlan(taskId, plan); + store.getState().setConnectionStatus("connected"); + api.getTaskPlan.mockResolvedValue(plan); + comments.getTaskPlanComments.mockRejectedValue(new Error("offline")); + const loader = planCommentLoaderFor(store, taskId, ERROR_MESSAGE); + releases.push(loader.attach()); + await loader.load(false); + await vi.advanceTimersByTimeAsync(1000); + expect(comments.getTaskPlanComments).toHaveBeenCalledTimes(2); + + const replacement = { ...plan, id: "plan-2" }; + store.getState().setTaskPlan(taskId, replacement); + api.getTaskPlan.mockResolvedValue(replacement); + await loader.load(false); + expect(comments.getTaskPlanComments).toHaveBeenCalledTimes(3); + await vi.advanceTimersByTimeAsync(999); + expect(comments.getTaskPlanComments).toHaveBeenCalledTimes(3); + await vi.advanceTimersByTimeAsync(1); + expect(comments.getTaskPlanComments).toHaveBeenCalledTimes(4); +}); + +it("does not let a hidden wake suppress immediate visible recovery", async () => { + const store = createAppStore(); + store.getState().setConnectionStatus("connected"); + const loader = planCommentLoaderFor(store, "task-1", ERROR_MESSAGE); + releases.push(loader.attach()); + api.getTaskPlan.mockRejectedValueOnce(new Error("offline")).mockResolvedValue(null); + await loader.load(true); + const visibility = vi.spyOn(document, "visibilityState", "get").mockReturnValue("hidden"); + document.dispatchEvent(new Event("visibilitychange")); + await loader.wake(); + visibility.mockReturnValue("visible"); + await loader.wake(); + expect(store.getState().taskPlans.commentsErrorByTaskId["task-1"]).toBeUndefined(); + expect(api.getTaskPlan).toHaveBeenCalledTimes(2); +}); + +it("uses the current localized error when a retained loader fails again", async () => { + const store = createAppStore(); + store.getState().setConnectionStatus("connected"); + const loader = planCommentLoaderFor(store, "task-1", "Old localized error"); + releases.push(loader.attach()); + api.getTaskPlan.mockRejectedValue(new Error("offline")); + await loader.load(true); + const translated = planCommentLoaderFor(store, "task-1", "Current localized error"); + expect(translated).toBe(loader); + await translated.load(true); + expect(store.getState().taskPlans.commentsErrorByTaskId["task-1"]).toBe( + "Current localized error", + ); +}); + +it("resumes failed ordinary reads when a cached task surface remounts", async () => { + const store = createAppStore(); + store.getState().setTaskPlan("task-1", null); + store.getState().setConnectionStatus("connected"); + const loader = planCommentLoaderFor(store, "task-1", ERROR_MESSAGE); + api.getTaskPlan.mockRejectedValueOnce(new Error("offline")).mockResolvedValue(null); + const detach = loader.attach(); + await loader.load(true); + detach(); + await vi.advanceTimersByTimeAsync(120000); + expect(api.getTaskPlan).toHaveBeenCalledOnce(); + releases.push(loader.attach()); + await vi.advanceTimersByTimeAsync(0); + expect(api.getTaskPlan).toHaveBeenCalledTimes(2); + expect(store.getState().taskPlans.commentsErrorByTaskId["task-1"]).toBeUndefined(); +}); diff --git a/apps/web/hooks/domains/comments/plan-comment-loading.ts b/apps/web/hooks/domains/comments/plan-comment-loading.ts new file mode 100644 index 0000000000..940cf2834d --- /dev/null +++ b/apps/web/hooks/domains/comments/plan-comment-loading.ts @@ -0,0 +1,192 @@ +import type { StoreApi } from "zustand"; +import { getTaskPlan } from "@/lib/api/domains/plan-api"; +import { getTaskPlanComments } from "@/lib/api/domains/plan-comment-api"; +import { planCommentRecoveryDelay } from "@/lib/plan-comment-recovery"; +import type { AppState } from "@/lib/state/store"; + +// Keep one loader per visited task for the store's lifetime, including across plan resets. +// Last-detach releases timers and subscriptions; remounts retain the same retry owner. +const loaders = new WeakMap, Map>(); + +/** Ordinary reads retain the last snapshot and never participate in Send admission. */ +class PlanCommentLoader { + private consumers = 0; + private generation = 0; + private planEpoch = 0; + private failures = 0; + private lastWakeAt = -Infinity; + private timer: ReturnType | undefined; + private inFlight: Promise | undefined; + private activeReadEpoch = 0; + private needsReload = false; + private unsubscribe: (() => void) | undefined; + + constructor( + private store: StoreApi, + private taskId: string, + private errorMessage: string, + ) {} + + setErrorMessage(errorMessage: string) { + this.errorMessage = errorMessage; + } + + attach() { + this.consumers++; + if (!this.unsubscribe) this.observe(); + if (this.failures) void this.load(true); + return () => { + this.consumers--; + if (this.consumers) return; + this.generation++; + this.pause(); + this.unsubscribe?.(); + this.unsubscribe = undefined; + }; + } + + private pause() { + clearTimeout(this.timer); + this.timer = undefined; + } + + private observe() { + const unsubscribe = this.store.subscribe((state, previous) => { + const planChanged = + state.taskPlans.byTaskId[this.taskId]?.id !== previous.taskPlans.byTaskId[this.taskId]?.id; + if (planChanged) this.failures = 0; + if ( + planChanged || + state.taskPlans.loadedByTaskId[this.taskId] !== + previous.taskPlans.loadedByTaskId[this.taskId] + ) + this.planEpoch++; + if (state.connection.status !== "connected") this.pause(); + }); + const onHidden = () => { + if (document.visibilityState !== "visible") this.pause(); + }; + document.addEventListener("visibilitychange", onHidden); + this.unsubscribe = () => { + unsubscribe(); + document.removeEventListener("visibilitychange", onHidden); + }; + } + + wake() { + if (!this.ready()) return this.inFlight ?? Promise.resolve(); + if (Date.now() - this.lastWakeAt < 250) return this.inFlight ?? Promise.resolve(); + this.lastWakeAt = Date.now(); + return this.load(true); + } + + private ready() { + return ( + this.consumers > 0 && + this.store.getState().connection.status === "connected" && + document.visibilityState === "visible" + ); + } + + load(force: boolean): Promise { + if (this.inFlight) { + if (this.planEpoch !== this.activeReadEpoch) this.needsReload = true; + return this.inFlight; + } + if (!this.ready()) return Promise.resolve(); + clearTimeout(this.timer); + this.timer = undefined; + const generation = this.generation; + this.activeReadEpoch = this.planEpoch; + this.needsReload = false; + const promise = Promise.resolve() + .then(() => this.read(force, generation)) + .finally(() => { + if (this.inFlight === promise) this.inFlight = undefined; + if (generation !== this.generation && this.ready()) void this.load(true); + else if (this.needsReload && this.ready()) void this.load(false); + }); + this.inFlight = promise; + return promise; + } + + private current(generation: number) { + return this.consumers > 0 && generation === this.generation; + } + + private async resolvePlan(force: boolean, generation: number) { + const state = this.store.getState(); + const cached = state.taskPlans.byTaskId[this.taskId]; + if (!force && cached !== undefined) return cached; + const epoch = this.planEpoch; + state.setTaskPlanLoading(this.taskId, true); + const next = await getTaskPlan(this.taskId); + if (!this.current(generation)) return undefined; + const current = this.store.getState(); + if (this.planEpoch !== epoch || current.taskPlans.byTaskId[this.taskId] !== cached) + return current.taskPlans.byTaskId[this.taskId]; + current.setTaskPlan(this.taskId, next); + this.activeReadEpoch = this.planEpoch; + return next; + } + + private async read(force: boolean, generation: number) { + if (!this.current(generation) || !this.ready()) return; + const state = this.store.getState(); + let expectedEpoch = this.planEpoch; + state.setTaskPlanCommentsError(this.taskId); + try { + const plan = await this.resolvePlan(force, generation); + if (!this.current(generation)) return; + expectedEpoch = this.planEpoch; + this.activeReadEpoch = expectedEpoch; + this.needsReload = false; + if (plan && this.ready()) { + this.store.getState().setTaskPlanCommentsLoading(this.taskId, true); + const snapshot = await getTaskPlanComments(this.taskId); + if (!this.current(generation) || expectedEpoch !== this.planEpoch) return; + this.store.getState().setTaskPlanComments(this.taskId, snapshot); + } + this.failures = 0; + } catch { + this.failedRead(generation, expectedEpoch); + } finally { + this.finishRead(); + } + } + + private failedRead(generation: number, epoch: number) { + if (!this.current(generation) || epoch !== this.planEpoch) return; + this.store.getState().setTaskPlanCommentsError(this.taskId, this.errorMessage); + this.failures++; + if (!this.ready()) return; + this.timer = setTimeout(() => { + this.timer = undefined; + void this.load(true); + }, planCommentRecoveryDelay(this.failures)); + } + + private finishRead() { + const current = this.store.getState(); + current.setTaskPlanLoading(this.taskId, false); + current.setTaskPlanCommentsLoading(this.taskId, false); + } +} + +export function planCommentLoaderFor( + store: StoreApi, + taskId: string, + errorMessage: string, +) { + let tasks = loaders.get(store); + if (!tasks) { + tasks = new Map(); + loaders.set(store, tasks); + } + let loader = tasks.get(taskId); + if (!loader) { + loader = new PlanCommentLoader(store, taskId, errorMessage); + tasks.set(taskId, loader); + } else loader.setErrorMessage(errorMessage); + return loader; +} diff --git a/apps/web/hooks/domains/comments/plan-comment-migration.test.ts b/apps/web/hooks/domains/comments/plan-comment-migration.test.ts new file mode 100644 index 0000000000..3a89b9b8a3 --- /dev/null +++ b/apps/web/hooks/domains/comments/plan-comment-migration.test.ts @@ -0,0 +1,567 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { createAppStore } from "@/lib/state/store"; +import { COMMENTS_STORAGE_PREFIX } from "@/lib/state/slices/comments/persistence"; +import type { PlanComment } from "@/lib/state/slices/comments"; +import type { TaskPlan, TaskPlanCommentSnapshot } from "@/lib/types/http"; +import { WebSocketRequestError, toWebSocketRequestError } from "@/lib/ws/request-error"; +import { planCommentMigrationFor } from "./plan-comment-migration"; + +const api = vi.hoisted(() => ({ + createTaskPlanComment: vi.fn(), + updateTaskPlanComment: vi.fn(), +})); +const plans = vi.hoisted(() => ({ getTaskPlan: vi.fn() })); +vi.mock("@/lib/api/domains/plan-comment-api", () => api); +vi.mock("@/lib/api/domains/plan-api", () => plans); + +const TASK = "task-1"; +const SESSION = "session-1"; +const PLAN = "plan-1"; +const DATE = "2026-09-02T00:00:00Z"; +const EDITED_BODY = "Edited feedback"; +const plan: TaskPlan = { + id: PLAN, + task_id: TASK, + title: "Plan", + content: "Step", + created_by: "agent", + created_at: DATE, + updated_at: DATE, +}; +const comment: PlanComment = { + id: "legacy-1", + sessionId: SESSION, + source: "plan", + text: "Feedback", + selectedText: "Step", + from: 1, + to: 5, + createdAt: DATE, + status: "pending", +}; +const snapshot: TaskPlanCommentSnapshot = { + task_id: TASK, + plan_id: PLAN, + revision: 1, + comments: [ + { + id: comment.id, + task_id: TASK, + plan_id: PLAN, + body: comment.text, + selected_text: comment.selectedText, + anchor_from: 1, + anchor_to: 5, + version: 1, + created_at: DATE, + updated_at: DATE, + }, + ], +}; +let releases: Array<() => void>; + +function write(rows = [comment], sessionId = SESSION) { + window.sessionStorage.setItem(`${COMMENTS_STORAGE_PREFIX}${sessionId}`, JSON.stringify(rows)); +} +function saved() { + return JSON.parse(window.sessionStorage.getItem(`${COMMENTS_STORAGE_PREFIX}${SESSION}`) ?? "[]"); +} +function setup(connected = true) { + const store = createAppStore(); + store.getState().setTaskPlan(TASK, plan); + store.getState().setConnectionStatus(connected ? "connected" : "disconnected"); + const recovery = planCommentMigrationFor(store, TASK); + const detach = recovery.attach(vi.fn().mockResolvedValue(undefined)); + releases.push(detach); + recovery.update({ sessionIds: [SESSION], complete: true, loading: false }); + const state = () => store.getState().taskPlans.commentsMigrationByTaskId[TASK]; + return { store, recovery, detach, state }; +} +function settle() { + return vi.advanceTimersByTimeAsync(0); +} +function deferred() { + let resolve!: (value: T) => void; + const promise = new Promise((done) => { + resolve = done; + }); + return { promise, resolve }; +} + +function editedSnapshot(body = EDITED_BODY, version = 2): TaskPlanCommentSnapshot { + return { ...snapshot, revision: version, comments: [{ ...snapshot.comments[0], body, version }] }; +} + +async function acknowledgeDuringLocalEdit(edited = { ...comment, text: EDITED_BODY }) { + write(); + const pending = deferred(); + api.createTaskPlanComment + .mockReturnValueOnce(pending.promise) + .mockRejectedValue(new WebSocketRequestError("UUID already used", "plan_comments_changed")); + const fixture = setup(); + await settle(); + write([edited]); + fixture.recovery.update({ sessionIds: [SESSION], complete: true, loading: false }); + pending.resolve(snapshot); + await settle(); + return fixture; +} + +beforeEach(() => { + vi.useFakeTimers(); + vi.resetAllMocks(); + window.sessionStorage.clear(); + releases = []; + api.createTaskPlanComment.mockResolvedValue(snapshot); + plans.getTaskPlan.mockResolvedValue(plan); +}); +afterEach(() => { + for (const release of releases) release(); + vi.restoreAllMocks(); + vi.useRealTimers(); +}); + +describe("task-scoped plan comment recovery", () => { + it("resolves an unknown plan without relying on an ordinary comment loader", async () => { + write(); + const store = createAppStore(); + store.getState().setConnectionStatus("connected"); + const recovery = planCommentMigrationFor(store, TASK); + releases.push(recovery.attach(vi.fn())); + recovery.update({ sessionIds: [SESSION], complete: true, loading: false }); + await settle(); + expect(store.getState().taskPlans.commentsMigrationByTaskId[TASK]).toMatchObject({ + status: "complete", + pendingCount: 0, + }); + expect(plans.getTaskPlan).toHaveBeenCalledOnce(); + expect(saved()).toEqual([]); + }); + + // @covers AC-TASKS-PLAN-COMMENTS-004.2, AC-TASKS-PLAN-COMMENTS-004.6 + it("uses three connected attempts then bounded background backoff with the original UUID", async () => { + write(); + api.createTaskPlanComment.mockRejectedValue(new Error("offline")); + const { state } = setup(); + await settle(); + expect(state()).toMatchObject({ status: "retrying", pendingCount: 1 }); + for (const [index, delay] of [1000, 2000, 30000, 60000, 120000, 120000].entries()) { + await vi.advanceTimersByTimeAsync(delay - 1); + expect(api.createTaskPlanComment).toHaveBeenCalledTimes(index + 1); + await vi.advanceTimersByTimeAsync(1); + expect(api.createTaskPlanComment).toHaveBeenCalledTimes(index + 2); + } + expect(state()).toMatchObject({ status: "failed", pendingCount: 1, failure: "transient" }); + expect( + api.createTaskPlanComment.mock.calls.every( + ([input]) => input.id === comment.id && input.body === comment.text, + ), + ).toBe(true); + expect(saved()).toEqual([comment]); + }); + + it("gives a replacement plan a fresh quiet retry burst", async () => { + write(); + api.createTaskPlanComment.mockRejectedValue(new Error("offline")); + const { store, state } = setup(); + await settle(); + await vi.advanceTimersByTimeAsync(1000); + expect(api.createTaskPlanComment).toHaveBeenCalledTimes(2); + + store.getState().setTaskPlan(TASK, { ...plan, id: "plan-2" }); + await settle(); + expect(api.createTaskPlanComment).toHaveBeenCalledTimes(3); + expect(api.createTaskPlanComment).toHaveBeenLastCalledWith( + expect.objectContaining({ planId: "plan-2", id: comment.id }), + ); + expect(state()).toMatchObject({ status: "retrying", pendingCount: 1 }); + await vi.advanceTimersByTimeAsync(1000); + expect(api.createTaskPlanComment).toHaveBeenCalledTimes(4); + expect(state()?.status).toBe("retrying"); + await vi.advanceTimersByTimeAsync(2000); + expect(api.createTaskPlanComment).toHaveBeenCalledTimes(5); + expect(state()).toMatchObject({ status: "failed", pendingCount: 1, failure: "transient" }); + expect(saved()).toEqual([comment]); + }); + + it("coalesces multiple surfaces and resume signals around one in-flight upload", async () => { + write(); + const pending = deferred(); + api.createTaskPlanComment.mockReturnValue(pending.promise); + const { store, recovery, state, detach } = setup(); + const other = planCommentMigrationFor(store, TASK); + expect(other).toBe(recovery); + releases.push(other.attach(vi.fn())); + other.update({ sessionIds: [SESSION], complete: true, loading: false }); + recovery.wake(); + other.wake(); + await settle(); + detach(); + pending.resolve(snapshot); + await settle(); + expect(api.createTaskPlanComment).toHaveBeenCalledOnce(); + expect(state()).toMatchObject({ status: "complete", pendingCount: 0 }); + }); + + it("spends no attempts while hidden or offline and resumes without a manual retry", async () => { + write(); + const { store, recovery, state } = setup(false); + await vi.advanceTimersByTimeAsync(120000); + expect(api.createTaskPlanComment).not.toHaveBeenCalled(); + expect(state()?.pendingCount).toBe(1); + const visibility = vi.spyOn(document, "visibilityState", "get").mockReturnValue("hidden"); + store.getState().setConnectionStatus("connected"); + await settle(); + expect(api.createTaskPlanComment).not.toHaveBeenCalled(); + visibility.mockReturnValue("visible"); + await recovery.wake(); + expect(state()?.status).toBe("complete"); + }); +}); + +describe("legacy recovery scope lifecycle", () => { + it("keeps an in-flight upload when a load confirms the same plan identity", async () => { + write(); + const pending = deferred(); + api.createTaskPlanComment.mockReturnValueOnce(pending.promise); + const { store, state } = setup(false); + store.setState((previous) => ({ + taskPlans: { + ...previous.taskPlans, + loadedByTaskId: { ...previous.taskPlans.loadedByTaskId, [TASK]: false }, + }, + })); + store.getState().setConnectionStatus("connected"); + await settle(); + expect(api.createTaskPlanComment).toHaveBeenCalledOnce(); + store.getState().setTaskPlan(TASK, plan); + pending.resolve(snapshot); + await settle(); + expect(api.createTaskPlanComment).toHaveBeenCalledOnce(); + expect(state()).toMatchObject({ status: "complete", pendingCount: 0 }); + expect(saved()).toEqual([]); + }); + + it("stops timers when the last surface unmounts and resumes on remount", async () => { + write(); + api.createTaskPlanComment.mockRejectedValueOnce(new Error("offline")); + const { recovery, detach, state } = setup(); + await settle(); + detach(); + await vi.advanceTimersByTimeAsync(120000); + expect(api.createTaskPlanComment).toHaveBeenCalledOnce(); + releases.push(recovery.attach(vi.fn())); + recovery.update({ sessionIds: [SESSION], complete: true, loading: false }); + await settle(); + expect(state()?.status).toBe("complete"); + }); + + it("does not acknowledge a late result after task departure, including immediate remount", async () => { + write(); + const old = deferred(); + const next = deferred(); + api.createTaskPlanComment.mockReturnValueOnce(old.promise).mockReturnValueOnce(next.promise); + const { recovery, detach, store } = setup(); + await settle(); + detach(); + releases.push(recovery.attach(vi.fn())); + recovery.update({ sessionIds: [SESSION], complete: true, loading: false }); + old.resolve(snapshot); + await settle(); + expect(saved()).toEqual([comment]); + expect(store.getState().taskPlans.commentsByTaskId[TASK]).toBeUndefined(); + next.resolve(snapshot); + await settle(); + expect(saved()).toEqual([]); + }); +}); + +describe("legacy recovery wire errors", () => { + // @covers AC-TASKS-PLAN-COMMENTS-004.6 + it.each(["INTERNAL_ERROR", "internal_error"])("automatically retries %s", async (code) => { + write(); + api.createTaskPlanComment.mockRejectedValueOnce( + toWebSocketRequestError({ code, message: "Temporary server failure" }), + ); + const { state } = setup(); + await settle(); + expect(state()).toMatchObject({ status: "retrying", pendingCount: 1 }); + expect(saved()).toEqual([comment]); + await vi.advanceTimersByTimeAsync(1000); + expect(api.createTaskPlanComment).toHaveBeenCalledTimes(2); + expect(state()).toMatchObject({ status: "complete", pendingCount: 0 }); + expect(saved()).toEqual([]); + }); + + it.each([ + ["NOT_FOUND", "automatic"], + ["not_found", "automatic"], + ["NOT_FOUND", "manual"], + ["not_found", "manual"], + ])("refreshes a missing plan after %s through %s retry", async (code, trigger) => { + write(); + const replacement = { ...plan, id: "plan-2" }; + const recovered = { + ...snapshot, + plan_id: replacement.id, + comments: snapshot.comments.map((row) => ({ ...row, plan_id: replacement.id })), + }; + plans.getTaskPlan.mockResolvedValue(replacement); + api.createTaskPlanComment + .mockRejectedValueOnce(toWebSocketRequestError({ code, message: "Plan not found" })) + .mockResolvedValue(recovered); + const { store, recovery, state } = setup(); + await settle(); + expect(saved()).toEqual([comment]); + if (trigger === "manual") await recovery.retry(); + else await vi.advanceTimersByTimeAsync(1000); + await settle(); + expect(plans.getTaskPlan).toHaveBeenCalledOnce(); + expect(api.createTaskPlanComment).toHaveBeenLastCalledWith( + expect.objectContaining({ planId: replacement.id, id: comment.id, body: comment.text }), + ); + expect(store.getState().taskPlans.commentsByTaskId[TASK]).toEqual(recovered); + expect(state()).toMatchObject({ status: "complete", pendingCount: 0 }); + expect(saved()).toEqual([]); + }); +}); + +describe("legacy recovery identity", () => { + it("ignores a late plan lookup after another reader confirms absence", async () => { + write(); + const lookup = deferred(); + plans.getTaskPlan.mockReturnValueOnce(lookup.promise); + const store = createAppStore(); + store.getState().setConnectionStatus("connected"); + const recovery = planCommentMigrationFor(store, TASK); + releases.push(recovery.attach(vi.fn())); + recovery.update({ sessionIds: [SESSION], complete: true, loading: false }); + await settle(); + expect(plans.getTaskPlan).toHaveBeenCalledOnce(); + store.getState().setTaskPlan(TASK, null); + lookup.resolve(plan); + await settle(); + expect(store.getState().taskPlans.byTaskId[TASK]).toBeNull(); + expect(api.createTaskPlanComment).not.toHaveBeenCalled(); + expect(saved()).toEqual([comment]); + expect(store.getState().taskPlans.commentsMigrationByTaskId[TASK]).toMatchObject({ + status: "waiting_for_plan", + pendingCount: 1, + }); + }); + + it("surfaces exhausted plan lookup failures for known drafts and later recovers", async () => { + write(); + plans.getTaskPlan.mockRejectedValue(new Error("offline")); + const store = createAppStore(); + store.getState().setConnectionStatus("connected"); + const recovery = planCommentMigrationFor(store, TASK); + releases.push(recovery.attach(vi.fn())); + recovery.update({ sessionIds: [SESSION], complete: true, loading: false }); + await vi.advanceTimersByTimeAsync(3000); + expect(plans.getTaskPlan).toHaveBeenCalledTimes(3); + expect(store.getState().taskPlans.commentsMigrationByTaskId[TASK]).toMatchObject({ + status: "failed", + pendingCount: 1, + failure: "transient", + }); + expect(saved()).toEqual([comment]); + plans.getTaskPlan.mockResolvedValue(plan); + await vi.advanceTimersByTimeAsync(30000); + expect(store.getState().taskPlans.commentsMigrationByTaskId[TASK]).toMatchObject({ + status: "complete", + pendingCount: 0, + }); + expect(saved()).toEqual([]); + }); + + it("does not let a hidden wake suppress the immediate visible retry", async () => { + write(); + api.createTaskPlanComment.mockRejectedValueOnce(new Error("offline")); + const { recovery, state } = setup(); + await settle(); + const visibility = vi.spyOn(document, "visibilityState", "get").mockReturnValue("hidden"); + document.dispatchEvent(new Event("visibilitychange")); + await recovery.wake(); + visibility.mockReturnValue("visible"); + await recovery.wake(); + expect(api.createTaskPlanComment).toHaveBeenCalledTimes(2); + expect(state()).toMatchObject({ status: "complete", pendingCount: 0 }); + }); + + it("does not acknowledge a stale generation after a plan is deleted and restored", async () => { + write(); + const old = deferred(); + const next = deferred(); + api.createTaskPlanComment.mockReturnValueOnce(old.promise).mockReturnValueOnce(next.promise); + const { store } = setup(); + await settle(); + store.getState().setTaskPlan(TASK, null); + store.getState().setTaskPlan(TASK, plan); + old.resolve(snapshot); + await settle(); + expect(saved()).toEqual([comment]); + expect(store.getState().taskPlans.commentsByTaskId[TASK]).toBeUndefined(); + next.resolve(snapshot); + await settle(); + expect(saved()).toEqual([]); + }); +}); + +describe("legacy edits during acknowledgement", () => { + it("updates an acknowledged body at its original UUID and exact version", async () => { + api.updateTaskPlanComment.mockResolvedValue(editedSnapshot()); + const { store, state } = await acknowledgeDuringLocalEdit(); + expect(saved()).toEqual([{ ...comment, text: EDITED_BODY }]); + await vi.advanceTimersByTimeAsync(1000); + expect(state()).toMatchObject({ status: "complete", pendingCount: 0 }); + expect(store.getState().taskPlans.commentsByTaskId[TASK]).toEqual(editedSnapshot()); + expect(api.createTaskPlanComment).toHaveBeenCalledOnce(); + expect(api.updateTaskPlanComment).toHaveBeenCalledWith({ + taskId: TASK, + planId: PLAN, + id: comment.id, + expectedVersion: 1, + body: EDITED_BODY, + }); + expect(saved()).toEqual([]); + }); + + it("retains local feedback without overwriting a conflicting server edit", async () => { + const remote = editedSnapshot("Another client's feedback", 3); + api.updateTaskPlanComment.mockRejectedValue( + new WebSocketRequestError("Changed version", "plan_comments_changed", { snapshot: remote }), + ); + const { store, recovery, state } = await acknowledgeDuringLocalEdit(); + await vi.advanceTimersByTimeAsync(121000); + expect(api.updateTaskPlanComment).toHaveBeenCalledOnce(); + expect(state()).toMatchObject({ status: "failed", pendingCount: 1, failure: "conflict" }); + expect(saved()).toEqual([{ ...comment, text: EDITED_BODY }]); + expect(store.getState().taskPlans.commentsByTaskId[TASK]).toEqual(remote); + + await recovery.retry(); + expect(api.updateTaskPlanComment).toHaveBeenCalledTimes(2); + expect(api.updateTaskPlanComment).toHaveBeenLastCalledWith({ + taskId: TASK, + planId: PLAN, + id: comment.id, + expectedVersion: 1, + body: EDITED_BODY, + }); + expect(state()).toMatchObject({ status: "failed", pendingCount: 1, failure: "conflict" }); + expect(store.getState().taskPlans.commentsByTaskId[TASK]).toEqual(remote); + expect(saved()).toEqual([{ ...comment, text: EDITED_BODY }]); + + const resolved = editedSnapshot(EDITED_BODY, 4); + api.updateTaskPlanComment.mockRejectedValueOnce( + new WebSocketRequestError("Changed version", "plan_comments_changed", { snapshot: resolved }), + ); + await recovery.retry(); + expect(state()).toMatchObject({ status: "complete", pendingCount: 0 }); + expect(store.getState().taskPlans.commentsByTaskId[TASK]).toEqual(resolved); + expect(saved()).toEqual([]); + }); + + it("reconciles an accepted update whose response was lost", async () => { + api.updateTaskPlanComment.mockRejectedValueOnce(new Error("response lost")).mockRejectedValue( + new WebSocketRequestError("Changed version", "plan_comments_changed", { + snapshot: editedSnapshot(), + }), + ); + const { state } = await acknowledgeDuringLocalEdit(); + await vi.advanceTimersByTimeAsync(3000); + expect(state()).toMatchObject({ status: "complete", pendingCount: 0 }); + expect(api.createTaskPlanComment).toHaveBeenCalledOnce(); + expect(api.updateTaskPlanComment).toHaveBeenCalledTimes(2); + expect(saved()).toEqual([]); + }); + + it("does not reinterpret a changed legacy selection as a body-only update", async () => { + const edited = { ...comment, text: EDITED_BODY, selectedText: "Other step" }; + const { state } = await acknowledgeDuringLocalEdit(edited); + await vi.advanceTimersByTimeAsync(1000); + expect(state()).toMatchObject({ status: "failed", pendingCount: 1, failure: "conflict" }); + expect(api.createTaskPlanComment).toHaveBeenCalledOnce(); + expect(api.updateTaskPlanComment).not.toHaveBeenCalled(); + expect(saved()).toEqual([edited]); + }); +}); + +describe("legacy feedback retention", () => { + it("keeps a paused partial recovery quiet when a plan still exists", async () => { + write([comment, { ...comment, id: "legacy-2" }]); + const pending = deferred(); + api.createTaskPlanComment.mockReturnValueOnce(pending.promise); + const { store, state } = setup(); + await settle(); + store.getState().setConnectionStatus("disconnected"); + pending.resolve(snapshot); + await settle(); + expect(state()).toMatchObject({ status: "idle", pendingCount: 1 }); + expect(api.createTaskPlanComment).toHaveBeenCalledOnce(); + }); + + it("retains identified drafts when later storage scans are empty or unavailable", async () => { + write(); + const { recovery, state } = setup(false); + window.sessionStorage.clear(); + recovery.update({ sessionIds: [SESSION], complete: true, loading: false }); + expect(state()?.pendingCount).toBe(1); + const read = vi.spyOn(window.sessionStorage, "getItem").mockImplementation(() => { + throw new DOMException("denied", "SecurityError"); + }); + recovery.update({ sessionIds: [SESSION], complete: true, loading: false }); + expect(state()?.pendingCount).toBe(1); + expect(read).toHaveBeenCalled(); + }); + + it("retries refused cleanup without uploading the acknowledged feedback again", async () => { + write(); + const removal = vi.spyOn(window.sessionStorage, "removeItem").mockImplementation(() => { + throw new DOMException("denied", "SecurityError"); + }); + const { state } = setup(); + await settle(); + expect(state()?.pendingCount).toBe(1); + expect(saved()).toEqual([comment]); + removal.mockRestore(); + await vi.advanceTimersByTimeAsync(1000); + expect(api.createTaskPlanComment).toHaveBeenCalledOnce(); + expect(saved()).toEqual([]); + expect(state()?.status).toBe("complete"); + }); + + it.each(["validation_error", "unauthorized", "plan_comments_changed"])( + "requires explicit retry for %s instead of looping mutations", + async (code) => { + write(); + api.createTaskPlanComment.mockRejectedValue(new WebSocketRequestError("rejected", code)); + const { recovery, state } = setup(); + await settle(); + recovery.wake(); + await vi.advanceTimersByTimeAsync(120000); + expect(api.createTaskPlanComment).toHaveBeenCalledOnce(); + expect(state()).toMatchObject({ status: "failed", pendingCount: 1 }); + api.createTaskPlanComment.mockResolvedValue(snapshot); + await recovery.retry(); + expect(saved()).toEqual([]); + }, + ); + + it("rescans newly discovered task sessions without touching another task's records", async () => { + write([comment], "foreign-session"); + const { recovery, state } = setup(); + await settle(); + expect(state()?.status).toBe("complete"); + expect(api.createTaskPlanComment).not.toHaveBeenCalled(); + write(); + recovery.update({ sessionIds: [SESSION], complete: true, loading: false }); + await settle(); + expect(state()?.pendingCount).toBe(0); + expect( + JSON.parse( + window.sessionStorage.getItem(`${COMMENTS_STORAGE_PREFIX}foreign-session`) ?? "[]", + ), + ).toEqual([comment]); + }); +}); diff --git a/apps/web/hooks/domains/comments/plan-comment-migration.ts b/apps/web/hooks/domains/comments/plan-comment-migration.ts new file mode 100644 index 0000000000..7c9cb0c1d9 --- /dev/null +++ b/apps/web/hooks/domains/comments/plan-comment-migration.ts @@ -0,0 +1,403 @@ +import type { StoreApi } from "zustand"; +import { createTaskPlanComment, updateTaskPlanComment } from "@/lib/api/domains/plan-comment-api"; +import { getTaskPlan } from "@/lib/api/domains/plan-api"; +import { planCommentAdmissionConflict } from "@/lib/plan-comment-refs"; +import { planCommentRecoveryDelay } from "@/lib/plan-comment-recovery"; +import { useCommentsStore, type PlanComment } from "@/lib/state/slices/comments"; +import { + readLegacyPlanComments, + removeAcknowledgedLegacyPlanComment, + type LegacyPlanCommentRecord, +} from "@/lib/state/slices/comments/persistence"; +import type { PlanCommentMigrationState } from "@/lib/state/slices/session/types"; +import type { AppState } from "@/lib/state/store"; +import type { TaskPlanComment, TaskPlanCommentSnapshot } from "@/lib/types/http"; +import { WebSocketRequestError } from "@/lib/ws/request-error"; + +type Failure = PlanCommentMigrationState["failure"]; +type PendingRecord = { record: LegacyPlanCommentRecord; acknowledged?: TaskPlanComment }; +type Discovery = { sessionIds: string[]; complete: boolean; loading: boolean }; + +// Keep one recovery per visited task for the store's lifetime, including across plan resets. +// Last-detach releases timers and subscriptions, retaining pending drafts and acknowledgements. +const recoveries = new WeakMap, Map>(); + +function legacyAnchor(comment: PlanComment) { + const anchorFrom = comment.from ?? 0; + return { + selectedText: comment.selectedText, + anchorFrom, + anchorTo: comment.to ?? anchorFrom + Math.max(1, comment.selectedText.length), + }; +} + +function sameAnchor(persisted: TaskPlanComment, comment: PlanComment) { + const anchor = legacyAnchor(comment); + return ( + persisted.selected_text === anchor.selectedText && + persisted.anchor_from === anchor.anchorFrom && + persisted.anchor_to === anchor.anchorTo + ); +} + +function acknowledgeLegacyRecord({ sessionId, comment }: LegacyPlanCommentRecord): Failure { + if (!removeAcknowledgedLegacyPlanComment(sessionId, comment)) return "transient"; + useCommentsStore.getState().forgetMigratedPlanComment(sessionId, comment.id); + return null; +} + +function classifyFailure(error: unknown): Failure { + if (planCommentAdmissionConflict(error)) return "conflict"; + if ( + error instanceof WebSocketRequestError && + error.code && + !["internal_error", "internal", "timeout", "not_found"].includes(error.code.toLowerCase()) + ) + return "rejected"; + return "transient"; +} + +/** One retry owner per task, independent of the number of mounted chat/plan surfaces. */ +export class PlanCommentMigration { + private pending = new Map(); + private consumers = new Map Promise>(); + private discovery: Discovery = { sessionIds: [], complete: false, loading: false }; + private storageAvailable = true; + private failures = 0; + private failure: Failure = null; + private dueAt = 0; + private lastWakeAt = -Infinity; + private generation = 0; + private refreshPlan = false; + private inFlight: Promise | undefined; + private timer: ReturnType | undefined; + private unsubscribe: (() => void) | undefined; + + constructor( + private store: StoreApi, + private taskId: string, + ) {} + + /** Consumers of the same task must provide equivalent task-wide session discovery. */ + attach(discover: () => Promise) { + const consumer = Symbol(); + this.consumers.set(consumer, discover); + if (!this.unsubscribe) this.observe(); + return () => { + this.consumers.delete(consumer); + if (this.consumers.size) return; + this.generation++; + this.cancelTimer(); + this.unsubscribe?.(); + this.unsubscribe = undefined; + }; + } + + update(discovery: Discovery) { + this.discovery = discovery; + this.scan(); + this.kick(); + } + + wake() { + if (!this.ready()) return this.inFlight; + if (Date.now() - this.lastWakeAt < 250) return this.inFlight; + this.lastWakeAt = Date.now(); + this.dueAt = 0; + this.cancelTimer(); + this.scan(); + return this.kick(); + } + + retry() { + this.failures = 0; + this.failure = null; + this.dueAt = 0; + this.refreshPlan ||= this.pending.size > 0 && !this.plan(); + this.cancelTimer(); + this.scan(); + return this.kick(); + } + + hasPendingComment(commentId: string) { + return [...this.pending.values()].some(({ record }) => record.comment.id === commentId); + } + + private plan() { + return this.store.getState().taskPlans.byTaskId[this.taskId]; + } + + private ready() { + return ( + this.consumers.size > 0 && + this.store.getState().connection.status === "connected" && + document.visibilityState === "visible" + ); + } + + private observe() { + const unsubscribe = this.store.subscribe((state, previous) => { + const plan = state.taskPlans.byTaskId[this.taskId]; + const previousPlan = previous.taskPlans.byTaskId[this.taskId]; + // Unknown and confirmed-absent plans are different recovery scopes. + const planChanged = + plan?.id !== previousPlan?.id || (plan === null) !== (previousPlan === null); + const loadedChanged = + state.taskPlans.loadedByTaskId[this.taskId] !== + previous.taskPlans.loadedByTaskId[this.taskId]; + const connectionChanged = state.connection.status !== previous.connection.status; + if (!planChanged && !loadedChanged && !connectionChanged) return; + if (planChanged) { + this.generation++; + this.failure = null; + this.failures = 0; + this.refreshPlan = false; + } + this.dueAt = 0; + this.cancelTimer(); + // Finish the store update before publishing derived recovery state. + void Promise.resolve().then(() => this.kick()); + }); + const onHidden = () => { + if (document.visibilityState !== "visible") this.cancelTimer(); + }; + document.addEventListener("visibilitychange", onHidden); + this.unsubscribe = () => { + unsubscribe(); + document.removeEventListener("visibilitychange", onHidden); + }; + } + + private scan() { + const { records, available } = readLegacyPlanComments(this.discovery.sessionIds); + this.storageAvailable = available; + for (const record of records) { + const key = `${record.sessionId}:${record.comment.id}`; + const known = this.pending.get(key); + if (known) known.record = record; + else this.pending.set(key, { record }); + } + } + + private publish(status: PlanCommentMigrationState["status"]) { + const next = { status, pendingCount: this.pending.size, failure: this.failure }; + const current = this.store.getState().taskPlans.commentsMigrationByTaskId[this.taskId]; + if ( + current?.status === next.status && + current.pendingCount === next.pendingCount && + current.failure === next.failure + ) + return; + this.store.getState().setTaskPlanCommentMigrationState(this.taskId, next); + } + + private cancelTimer() { + clearTimeout(this.timer); + this.timer = undefined; + } + + private schedule() { + if (this.timer || !this.ready()) return; + this.timer = setTimeout( + () => { + this.timer = undefined; + void this.kick(); + }, + Math.max(0, this.dueAt - Date.now()), + ); + } + + private kick(): Promise | undefined { + if (!this.consumers.size) return this.inFlight; + this.scan(); + if (this.inFlight) { + const state = this.store.getState().taskPlans.commentsMigrationByTaskId[this.taskId]; + this.publish(state?.status ?? "idle"); + return this.inFlight; + } + if (this.failure === "conflict" || this.failure === "rejected") { + this.publish("failed"); + return; + } + if (!this.pending.size && this.discovery.complete && this.storageAvailable) { + this.failure = null; + this.failures = 0; + this.publish("complete"); + return; + } + if (this.pending.size && this.plan() === null && !this.refreshPlan) { + this.publish("waiting_for_plan"); + return; + } + if (!this.ready()) { + this.publish("idle"); + return; + } + if (this.dueAt > Date.now()) { + this.schedule(); + return; + } + const generation = this.generation; + this.inFlight = Promise.resolve() + .then(() => this.run(generation)) + .finally(() => { + this.inFlight = undefined; + if (generation !== this.generation) void this.kick(); + else if (this.failure === "transient") this.schedule(); + else if (!this.failure && this.pending.size && this.plan() && this.ready()) + void this.kick(); + }); + return this.inFlight; + } + + private current(generation: number) { + return this.consumers.size > 0 && generation === this.generation; + } + + private async discover(generation: number) { + if (this.refreshPlan || (this.pending.size > 0 && this.plan() === undefined)) { + const next = await getTaskPlan(this.taskId); + if (!this.current(generation)) return; + this.refreshPlan = false; + this.store.getState().setTaskPlan(this.taskId, next); + } + if (!this.discovery.complete && !this.discovery.loading) { + // One live consumer performs the shared discovery for all mounted surfaces. + await this.consumers.values().next().value?.(); + } + } + + private async run(generation: number) { + if (!this.current(generation) || !this.ready()) return; + this.publish(this.pending.size ? "running" : "idle"); + let failure: Failure = null; + try { + await this.discover(generation); + } catch { + failure = "transient"; + } + if (!this.current(generation)) return; + failure = (await this.uploadPending(generation)) ?? failure; + if (!this.current(generation)) return; + this.scan(); + if (!this.discovery.complete || !this.storageAvailable) failure ??= "transient"; + this.finishRun(failure); + } + + private async uploadPending(generation: number): Promise { + const plan = this.plan(); + if (!plan) return null; + let failure: Failure = null; + for (const [key, pending] of this.pending) { + if (!this.current(generation) || !this.ready()) return failure; + const outcome = await this.upload(pending, plan.id, generation); + if (!this.current(generation)) return failure; + if (!outcome) this.pending.delete(key); + else if (failure !== "conflict" && failure !== "rejected") failure = outcome; + } + return failure; + } + + private finishRun(failure: Failure) { + this.failure = failure; + if (failure) { + this.failures++; + this.dueAt = Date.now() + planCommentRecoveryDelay(this.failures); + this.publish(failure !== "transient" || this.failures >= 3 ? "failed" : "retrying"); + } else if (this.pending.size) { + this.publish(this.plan() === null ? "waiting_for_plan" : "idle"); + } else { + this.failures = 0; + this.publish("complete"); + } + } + + private async upload( + pending: PendingRecord, + planId: string, + generation: number, + ): Promise { + const record = pending.record; + const { comment } = record; + try { + const acknowledged = + pending.acknowledged?.plan_id === planId ? pending.acknowledged : undefined; + if (acknowledged && !sameAnchor(acknowledged, comment)) return "conflict"; + if (!acknowledged || acknowledged.body !== comment.text) { + const input = { + taskId: this.taskId, + planId, + id: comment.id, + body: comment.text, + }; + const snapshot = acknowledged + ? await updateTaskPlanComment({ ...input, expectedVersion: acknowledged.version }) + : await createTaskPlanComment({ ...input, ...legacyAnchor(comment) }); + if (!this.current(generation)) return "transient"; + this.store.getState().setTaskPlanComments(this.taskId, snapshot); + if (!this.recordAcknowledgement(pending, planId, comment, snapshot)) return "conflict"; + } + if (!this.current(generation)) return "transient"; + return acknowledgeLegacyRecord(record); + } catch (error) { + if (!this.current(generation)) return "transient"; + return this.handleUploadFailure(error, pending, planId, record); + } + } + + private handleUploadFailure( + error: unknown, + pending: PendingRecord, + planId: string, + record: LegacyPlanCommentRecord, + ): Failure { + const snapshot = planCommentAdmissionConflict(error)?.snapshot; + if (snapshot) this.store.getState().setTaskPlanComments(this.taskId, snapshot); + if ( + snapshot && + pending.acknowledged?.plan_id === planId && + this.recordAcknowledgement(pending, planId, record.comment, snapshot) + ) { + return acknowledgeLegacyRecord(record); + } + if (error instanceof WebSocketRequestError && error.code?.toLowerCase() === "not_found") + this.refreshPlan = true; + return classifyFailure(error); + } + + private recordAcknowledgement( + pending: PendingRecord, + planId: string, + comment: PlanComment, + snapshot: TaskPlanCommentSnapshot, + ) { + if (snapshot.task_id !== this.taskId || snapshot.plan_id !== planId) return false; + const persisted = snapshot.comments.find((row) => row.id === comment.id); + if (!persisted || persisted.body !== comment.text || !sameAnchor(persisted, comment)) + return false; + pending.acknowledged = persisted; + return true; + } +} + +export function planCommentMigrationFor(store: StoreApi, taskId: string) { + let tasks = recoveries.get(store); + if (!tasks) { + tasks = new Map(); + recoveries.set(store, tasks); + } + let recovery = tasks.get(taskId); + if (!recovery) { + recovery = new PlanCommentMigration(store, taskId); + tasks.set(taskId, recovery); + } + return recovery; +} + +export function hasPendingPlanCommentMigration( + store: StoreApi, + taskId: string, + commentId: string, +) { + return recoveries.get(store)?.get(taskId)?.hasPendingComment(commentId) ?? false; +} diff --git a/apps/web/hooks/domains/comments/use-plan-comment-migration.test.tsx b/apps/web/hooks/domains/comments/use-plan-comment-migration.test.tsx index 50d167ed50..0289298e4e 100644 --- a/apps/web/hooks/domains/comments/use-plan-comment-migration.test.tsx +++ b/apps/web/hooks/domains/comments/use-plan-comment-migration.test.tsx @@ -1,14 +1,16 @@ -import { act, renderHook, waitFor } from "@testing-library/react"; -import { beforeEach, describe, expect, it, vi } from "vitest"; +import { act, cleanup, renderHook, waitFor } from "@testing-library/react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { StateProvider, useAppStoreApi } from "@/components/state-provider"; import { useCommentsStore } from "@/lib/state/slices/comments"; import { COMMENTS_STORAGE_PREFIX } from "@/lib/state/slices/comments/persistence"; import type { DiffComment, PlanComment } from "@/lib/state/slices/comments"; import type { TaskPlan, TaskPlanComment, TaskPlanCommentSnapshot } from "@/lib/types/http"; +import { sessionId as toSessionId, taskId as toTaskId } from "@/lib/types/http"; import { WebSocketRequestError } from "@/lib/ws/request-error"; const api = vi.hoisted(() => ({ createTaskPlanComment: vi.fn(), + updateTaskPlanComment: vi.fn(), getTaskPlanComments: vi.fn(), })); const sessionsHook = vi.hoisted(() => ({ @@ -28,6 +30,7 @@ import { usePlanCommentMigration } from "./use-plan-comment-migration"; const TASK_ID = "task-1"; const PLAN_ID = "plan-1"; +const FOREIGN_SESSION = "foreign-session"; const PLAN_TIMESTAMP = "2026-09-02T00:00:00Z"; const taskPlan: TaskPlan = { @@ -78,7 +81,7 @@ function serverComment(comment: PlanComment, version = 1): TaskPlanComment { body: comment.text, selected_text: comment.selectedText, anchor_from: comment.from ?? 0, - anchor_to: comment.to ?? comment.selectedText.length, + anchor_to: comment.to ?? (comment.from ?? 0) + Math.max(1, comment.selectedText.length), version, created_at: comment.createdAt, updated_at: comment.createdAt, @@ -103,7 +106,13 @@ function readSession(sessionId: string): unknown[] { } function wrapper({ children }: { children: React.ReactNode }) { - return {children}; + return ( + + {children} + + ); } function useHarness() { @@ -115,7 +124,8 @@ function useHarness() { // eslint-disable-next-line max-lines-per-function -- Migration recovery cases share browser-storage and store fixtures. describe("usePlanCommentMigration", () => { beforeEach(() => { - vi.clearAllMocks(); + vi.resetAllMocks(); + sessionsHook.sessions = [{ id: "session-1" }, { id: "session-2" }]; sessionsHook.isLoaded = true; sessionsHook.error = null; sessionsHook.loadSessions.mockResolvedValue(undefined); @@ -128,6 +138,43 @@ describe("usePlanCommentMigration", () => { }); }); + afterEach(() => { + cleanup(); + vi.useRealTimers(); + }); + + it("does not request a migration refresh or block Send when storage is empty", async () => { + api.getTaskPlanComments.mockRejectedValue(new Error("offline")); + const { result } = renderHook(useHarness, { wrapper }); + expect(result.current.migration.isBlocking).toBe(false); + act(() => result.current.store.getState().setTaskPlan(TASK_ID, taskPlan)); + + await waitFor(() => expect(result.current.migration.status).toBe("complete")); + expect(api.getTaskPlanComments).not.toHaveBeenCalled(); + expect(api.createTaskPlanComment).not.toHaveBeenCalled(); + expect(result.current.migration.isBlocking).toBe(false); + }); + + it("automatically retries a transient legacy upload without manual Retry", async () => { + vi.useFakeTimers(); + const comment = legacyPlanComment("comment-1", "session-1"); + writeSession("session-1", [comment]); + api.createTaskPlanComment + .mockRejectedValueOnce(new Error("offline")) + .mockResolvedValueOnce(snapshot([serverComment(comment)])); + api.getTaskPlanComments.mockResolvedValue(snapshot([serverComment(comment)])); + const { result } = renderHook(useHarness, { wrapper }); + await act(async () => result.current.store.getState().setTaskPlan(TASK_ID, taskPlan)); + + expect(readSession("session-1")).toEqual([comment]); + expect(result.current.migration.isBlocking).toBe(true); + await act(async () => vi.advanceTimersByTimeAsync(1000)); + + expect(result.current.migration.status).toBe("complete"); + expect(api.createTaskPlanComment).toHaveBeenCalledTimes(2); + expect(readSession("session-1")).toEqual([]); + }); + it("migrates every known session row by UUID and preserves non-plan records", async () => { const first = legacyPlanComment("comment-1", "session-1"); const second = legacyPlanComment("comment-2", "session-2"); @@ -166,7 +213,8 @@ describe("usePlanCommentMigration", () => { it("preserves failed rows and retries only what remains", async () => { const first = legacyPlanComment("comment-1", "session-1"); const second = legacyPlanComment("comment-2", "session-2"); - writeSession("session-1", [first]); + const diff = diffComment(); + writeSession("session-1", [first, diff]); writeSession("session-2", [second]); api.createTaskPlanComment .mockResolvedValueOnce(snapshot([serverComment(first)], 1)) @@ -175,9 +223,9 @@ describe("usePlanCommentMigration", () => { const { result } = renderHook(useHarness, { wrapper }); act(() => result.current.store.getState().setTaskPlan(TASK_ID, taskPlan)); - await waitFor(() => expect(result.current.migration.status).toBe("failed")); + await waitFor(() => expect(result.current.migration.status).toBe("retrying")); - expect(readSession("session-1")).toEqual([]); + expect(readSession("session-1")).toEqual([diff]); expect(readSession("session-2")).toEqual([second]); const finalSnapshot = snapshot([serverComment(first), serverComment(second)], 2); @@ -243,32 +291,70 @@ describe("usePlanCommentMigration", () => { ); }); - it("surfaces session discovery failure and force reloads on retry", async () => { + it("does not block plain Send when session discovery fails without identified drafts", async () => { sessionsHook.isLoaded = false; sessionsHook.error = "offline"; const { result, rerender } = renderHook(useHarness, { wrapper }); act(() => result.current.store.getState().setTaskPlan(TASK_ID, taskPlan)); - await waitFor(() => expect(result.current.migration.status).toBe("failed")); + expect(result.current.migration.isBlocking).toBe(false); sessionsHook.error = null; sessionsHook.isLoaded = true; - await act(async () => result.current.migration.retry()); rerender(); - expect(sessionsHook.loadSessions).toHaveBeenCalledWith(true); await waitFor(() => expect(result.current.migration.status).toBe("complete")); }); - it("surfaces current-plan load failure and clears it on retry", async () => { + it("protects already-hydrated task drafts when the session list is unavailable", async () => { + sessionsHook.sessions = []; + sessionsHook.isLoaded = false; + sessionsHook.error = "offline"; + const legacy = legacyPlanComment("comment-1", "session-1"); + const unrelated = legacyPlanComment("comment-2", FOREIGN_SESSION); + writeSession("session-1", [legacy]); + writeSession(FOREIGN_SESSION, [unrelated]); + const { result } = renderHook(useHarness, { wrapper }); + act(() => { + result.current.store.getState().setConnectionStatus("disconnected"); + for (const [id, task_id] of [ + ["session-1", TASK_ID], + [FOREIGN_SESSION, "other-task"], + ]) { + result.current.store.getState().setTaskSession({ + id: toSessionId(id), + task_id: toTaskId(task_id), + state: "WAITING_FOR_INPUT", + started_at: PLAN_TIMESTAMP, + updated_at: PLAN_TIMESTAMP, + }); + } + }); + expect( + result.current.store.getState().taskSessionsByTask.itemsByTaskId[TASK_ID], + ).toBeUndefined(); + expect(result.current.migration.pendingCount).toBe(1); + expect(result.current.migration.isBlocking).toBe(true); + expect(readSession("session-1")).toEqual([legacy]); + expect(readSession(FOREIGN_SESSION)).toEqual([unrelated]); + }); + + it("failed plan discovery does not block plain Send before or after a null plan", async () => { const { result } = renderHook(useHarness, { wrapper }); act(() => result.current.store.getState().setTaskPlanCommentsError(TASK_ID, "offline")); - await waitFor(() => expect(result.current.migration.status).toBe("failed")); - await act(async () => result.current.migration.retry()); + expect(result.current.migration.isBlocking).toBe(false); + act(() => result.current.store.getState().setTaskPlan(TASK_ID, null)); + expect(result.current.migration.isBlocking).toBe(false); + }); - expect( - result.current.store.getState().taskPlans.commentsErrorByTaskId[TASK_ID], - ).toBeUndefined(); - expect(sessionsHook.loadSessions).toHaveBeenCalledWith(true); + it("successful empty snapshot recovery leaves no obsolete migration restriction", async () => { + const { result } = renderHook(useHarness, { wrapper }); + act(() => result.current.store.getState().setTaskPlanCommentsError(TASK_ID, "offline")); + act(() => { + result.current.store.getState().setTaskPlan(TASK_ID, taskPlan); + result.current.store.getState().setTaskPlanComments(TASK_ID, snapshot([])); + }); + await waitFor(() => expect(result.current.migration.isBlocking).toBe(false)); + expect(api.createTaskPlanComment).not.toHaveBeenCalled(); }); }); diff --git a/apps/web/hooks/domains/comments/use-plan-comment-migration.ts b/apps/web/hooks/domains/comments/use-plan-comment-migration.ts index df460d46f4..55169000ed 100644 --- a/apps/web/hooks/domains/comments/use-plan-comment-migration.ts +++ b/apps/web/hooks/domains/comments/use-plan-comment-migration.ts @@ -1,201 +1,45 @@ -import { useCallback, useEffect, useMemo } from "react"; +import { useCallback, useEffect, useMemo, useRef } from "react"; +import { useShallow } from "zustand/react/shallow"; import { useAppStore, useAppStoreApi } from "@/components/state-provider"; import { useTaskSessions } from "@/hooks/use-task-sessions"; -import { createTaskPlanComment, getTaskPlanComments } from "@/lib/api/domains/plan-comment-api"; -import { useCommentsStore } from "@/lib/state/slices/comments"; -import { - listLegacyPlanComments, - removeAcknowledgedLegacyPlanComment, -} from "@/lib/state/slices/comments/persistence"; -import type { AppState } from "@/lib/state/store"; -import type { TaskPlan } from "@/lib/types/http"; -import { planCommentAdmissionConflict } from "@/lib/plan-comment-refs"; +import { useForegroundRefresh } from "@/hooks/use-foreground-refresh"; +import { planCommentRecovery } from "@/lib/plan-comment-recovery"; +import { planCommentMigrationFor } from "./plan-comment-migration"; -type AppStore = ReturnType; - -const migrationRunsByStore = new WeakMap>>(); - -function migrationRunsFor(store: AppStore) { - const existing = migrationRunsByStore.get(store); - if (existing) return existing; - const runs = new Map>(); - migrationRunsByStore.set(store, runs); - return runs; -} - -function planIdentityMatches(state: AppState, taskId: string, plan: TaskPlan | null) { - return (state.taskPlans.byTaskId[taskId]?.id ?? null) === (plan?.id ?? null); -} - -function setStatusIfCurrent( - store: AppStore, - taskId: string, - plan: TaskPlan | null, - status: "running" | "complete" | "waiting_for_plan" | "failed", -) { - const state = store.getState(); - if (planIdentityMatches(state, taskId, plan)) { - state.setTaskPlanCommentMigrationStatus(taskId, status); - } -} - -type LegacyPlanCommentRecord = ReturnType[number]; - -function acknowledgeLegacyComment({ sessionId, comment }: LegacyPlanCommentRecord) { - const removed = removeAcknowledgedLegacyPlanComment(sessionId, comment); - const sameIdStillStored = listLegacyPlanComments([sessionId]).some( - (record) => record.comment.id === comment.id, - ); - if (!removed && sameIdStillStored) return false; - useCommentsStore.getState().forgetMigratedPlanComment(sessionId, comment.id); - return true; -} - -async function migrateLegacyComment( - taskId: string, - plan: TaskPlan, - record: LegacyPlanCommentRecord, - store: AppStore, -) { - const { comment } = record; - try { - const anchorFrom = comment.from ?? 0; - const snapshot = await createTaskPlanComment({ - taskId, - planId: plan.id, - id: comment.id, - body: comment.text, - selectedText: comment.selectedText, - anchorFrom, - anchorTo: comment.to ?? anchorFrom + Math.max(1, comment.selectedText.length), - }); - store.getState().setTaskPlanComments(taskId, snapshot); - return acknowledgeLegacyComment(record); - } catch (error) { - const snapshot = planCommentAdmissionConflict(error)?.snapshot; - if (snapshot) store.getState().setTaskPlanComments(taskId, snapshot); - // i18n-exempt: developer diagnostic; the migration notice is localized. - console.error("Failed to migrate legacy plan comment:", error); - return false; - } -} - -async function refreshMigratedComments(taskId: string, store: AppStore) { - try { - const snapshot = await getTaskPlanComments(taskId); - store.getState().setTaskPlanComments(taskId, snapshot); - return true; - } catch (error) { - // i18n-exempt: developer diagnostic; the migration notice is localized. - console.error("Failed to refresh task plan comments after migration:", error); - return false; - } -} - -async function migrateLegacyComments( - taskId: string, - plan: TaskPlan | null, - sessionIds: string[], - store: AppStore, -) { - const records = listLegacyPlanComments(sessionIds); - if (!plan) { - setStatusIfCurrent(store, taskId, plan, records.length === 0 ? "complete" : "waiting_for_plan"); - return; - } - - setStatusIfCurrent(store, taskId, plan, "running"); - let failed = false; - for (const record of records) { - if (!planIdentityMatches(store.getState(), taskId, plan)) return; - if (!(await migrateLegacyComment(taskId, plan, record, store))) failed = true; - } - if (!(await refreshMigratedComments(taskId, store))) failed = true; - setStatusIfCurrent(store, taskId, plan, failed ? "failed" : "complete"); -} - -function startMigration( - taskId: string, - plan: TaskPlan | null, - sessionIds: string[], - store: AppStore, -) { - const migrationRuns = migrationRunsFor(store); - const runKey = `${taskId}:${plan?.id ?? "none"}`; - const existing = migrationRuns.get(runKey); - if (existing) return existing; - const request = migrateLegacyComments(taskId, plan, sessionIds, store).finally(() => { - if (migrationRuns.get(runKey) === request) migrationRuns.delete(runKey); - }); - migrationRuns.set(runKey, request); - return request; -} - -/** Losslessly promotes sessionStorage plan drafts into the task-owned collection. */ +/** Losslessly promotes identified browser drafts, independently of ordinary comment reads. */ export function usePlanCommentMigration(taskId: string | null | undefined) { const store = useAppStoreApi(); - const { - sessions, - isLoaded: sessionsLoaded, - error: sessionsError, - loadSessions, - } = useTaskSessions(taskId ?? null); - const plan = useAppStore((state) => (taskId ? state.taskPlans.byTaskId[taskId] : undefined)); - const planLoaded = useAppStore((state) => - taskId ? (state.taskPlans.loadedByTaskId[taskId] ?? false) : true, + const { sessions, isLoaded, isLoading, error, loadSessions } = useTaskSessions(taskId ?? null); + const knownSessionIds = useAppStore( + useShallow((state) => + Object.values(state.taskSessions.items) + .filter((session) => session.task_id === taskId) + .map((session) => session.id), + ), ); - const storedStatus = useAppStore((state) => - taskId ? state.taskPlans.commentsMigrationStatusByTaskId[taskId] : "complete", + const connected = useAppStore((state) => state.connection.status === "connected"); + const state = useAppStore((state) => + taskId ? state.taskPlans.commentsMigrationByTaskId[taskId] : undefined, ); - const planLoadError = useAppStore((state) => - taskId ? (state.taskPlans.commentsErrorByTaskId[taskId] ?? null) : null, + const recovery = useMemo( + () => (taskId ? planCommentMigrationFor(store, taskId) : null), + [store, taskId], ); - const status = storedStatus ?? "idle"; - const sessionIds = useMemo(() => sessions.map((session) => session.id), [sessions]); - + const discover = useRef(loadSessions); useEffect(() => { - if ( - taskId && - status !== "complete" && - status !== "running" && - (sessionsError || planLoadError) - ) { - store.getState().setTaskPlanCommentMigrationStatus(taskId, "failed"); - return; - } - if (!taskId || !sessionsLoaded || !planLoaded || plan === undefined) return; - if (status === "running" || status === "complete" || status === "failed") return; - if (status === "waiting_for_plan" && plan === null) return; - void startMigration(taskId, plan, sessionIds, store); - }, [ - plan, - planLoadError, - planLoaded, - sessionIds, - sessionsError, - sessionsLoaded, - status, - store, - taskId, - ]); - + discover.current = loadSessions; + }, [loadSessions]); + useEffect(() => recovery?.attach(() => discover.current(true)), [recovery]); + useEffect(() => { + recovery?.update({ + sessionIds: [...new Set([...sessions.map((session) => session.id), ...knownSessionIds])], + complete: isLoaded && !error, + loading: isLoading, + }); + }, [recovery, sessions, knownSessionIds, isLoaded, isLoading, error]); + useForegroundRefresh(() => recovery?.wake(), Boolean(taskId) && connected, recovery); const retry = useCallback(async () => { - if (!taskId) return; - const state = store.getState(); - state.setTaskPlanCommentMigrationStatus(taskId, "running"); - state.setTaskPlanCommentsError(taskId); - await loadSessions(true); - const next = store.getState(); - next.setTaskPlanCommentMigrationStatus( - taskId, - next.taskSessionsByTask.errorByTaskId?.[taskId] ? "failed" : "idle", - ); - }, [loadSessions, store, taskId]); - - return { - status, - isReady: status === "complete", - isBlocking: Boolean(taskId) && status !== "complete", - retry, - }; + await recovery?.retry(); + }, [recovery]); + return { ...planCommentRecovery(state), retry }; } diff --git a/apps/web/hooks/domains/comments/use-plan-comments.test.tsx b/apps/web/hooks/domains/comments/use-plan-comments.test.tsx index 3745896584..8c70b2ca0d 100644 --- a/apps/web/hooks/domains/comments/use-plan-comments.test.tsx +++ b/apps/web/hooks/domains/comments/use-plan-comments.test.tsx @@ -1,5 +1,5 @@ -import { act, renderHook, waitFor } from "@testing-library/react"; -import { beforeEach, describe, expect, it, vi } from "vitest"; +import { act, cleanup, renderHook, waitFor } from "@testing-library/react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { StateProvider, useAppStoreApi } from "@/components/state-provider"; import type { TaskPlan, TaskPlanCommentSnapshot } from "@/lib/types/http"; @@ -70,10 +70,88 @@ function setUpApis() { api.getTaskPlanComments.mockResolvedValue(snapshot()); } +afterEach(() => { + cleanup(); + vi.useRealTimers(); +}); + // eslint-disable-next-line max-lines-per-function -- Loading and reconnect cases share one task-plan store fixture. describe("usePlanComments loading", () => { beforeEach(setUpApis); + it("loads a replacement plan after an older in-flight read settles", async () => { + let finishOld!: (value: TaskPlanCommentSnapshot) => void; + const replacement = { ...snapshot(2), plan_id: "plan-2", comments: [] }; + api.getTaskPlanComments + .mockImplementationOnce( + () => + new Promise((resolve) => { + finishOld = resolve; + }), + ) + .mockResolvedValueOnce(replacement); + const { result } = renderHook(useTwoTaskCommentConsumers, { wrapper }); + await act(async () => { + result.current.store.getState().setTaskPlan(TASK_ID, taskPlan); + result.current.store.getState().setConnectionStatus("connected"); + }); + await act(async () => + result.current.store.getState().setTaskPlan(TASK_ID, { ...taskPlan, id: "plan-2" }), + ); + await act(async () => finishOld(snapshot())); + expect(result.current.first.snapshot).toEqual(replacement); + }); + + it("ignores an old response after deletion and restoration of the same plan", async () => { + let rejectOld!: (error: Error) => void; + api.getTaskPlanComments.mockImplementationOnce( + () => + new Promise((_resolve, reject) => { + rejectOld = reject; + }), + ); + const { result } = renderHook(useTwoTaskCommentConsumers, { wrapper }); + await act(async () => { + result.current.store.getState().setTaskPlan(TASK_ID, taskPlan); + result.current.store.getState().setConnectionStatus("connected"); + }); + await act(async () => { + result.current.store.getState().setTaskPlan(TASK_ID, null); + result.current.store.getState().setTaskPlan(TASK_ID, taskPlan); + result.current.store.getState().setTaskPlanComments(TASK_ID, snapshot(5)); + rejectOld(new Error("old generation failed")); + }); + expect(result.current.first.loadError).toBeNull(); + expect(result.current.first.comments[0]?.version).toBe(5); + }); + + it("retries failed background discovery without losing displayed context", async () => { + vi.useFakeTimers(); + planApi.getTaskPlan.mockRejectedValueOnce(new Error("offline")).mockResolvedValue(taskPlan); + const { result } = renderHook(useTwoTaskCommentConsumers, { wrapper }); + await act(async () => { + result.current.store.getState().setTaskPlan(TASK_ID, taskPlan); + result.current.store.getState().setTaskPlanComments(TASK_ID, snapshot()); + result.current.store.getState().setConnectionStatus("connected"); + }); + expect(result.current.first.comments[0]?.version).toBe(1); + await act(async () => vi.advanceTimersByTimeAsync(1000)); + expect(planApi.getTaskPlan).toHaveBeenCalledTimes(2); + expect(result.current.first.loadError).toBeNull(); + expect(result.current.first.comments[0]?.version).toBe(1); + }); + + it("does not request plan comments on foreground events while disconnected", async () => { + const { result } = renderHook(useTwoTaskCommentConsumers, { wrapper }); + await act(async () => result.current.store.getState().setTaskPlan(TASK_ID, taskPlan)); + await act(async () => window.dispatchEvent(new Event("focus"))); + expect(planApi.getTaskPlan).not.toHaveBeenCalled(); + expect(api.getTaskPlanComments).not.toHaveBeenCalled(); + await act(async () => result.current.store.getState().setConnectionStatus("connected")); + await act(async () => window.dispatchEvent(new Event("focus"))); + await waitFor(() => expect(api.getTaskPlanComments).toHaveBeenCalled()); + }); + it("loads the current plan before comments when only a task composer is mounted", async () => { const { result } = renderHook( () => { @@ -201,35 +279,32 @@ describe("usePlanComments loading", () => { it("does not let a stale plan fetch clear the replacement plan loading state", async () => { let rejectOld!: (error: Error) => void; - api.getTaskPlanComments.mockImplementationOnce( - () => new Promise((_resolve, reject) => (rejectOld = reject)), - ); - const { result } = renderHook( - () => { - const store = useAppStoreApi(); - return { store, comments: usePlanComments(TASK_ID) }; - }, - { wrapper }, - ); - act(() => { + let finishReplacement!: (value: TaskPlanCommentSnapshot) => void; + const replacement = { ...snapshot(2), plan_id: "plan-2", comments: [] }; + api.getTaskPlanComments + .mockImplementationOnce(() => new Promise((_resolve, reject) => (rejectOld = reject))) + .mockImplementationOnce(() => new Promise((resolve) => (finishReplacement = resolve))); + const { result } = renderHook(useTwoTaskCommentConsumers, { wrapper }); + await act(async () => { result.current.store.getState().setTaskPlan(TASK_ID, taskPlan); - result.current.store.setState({ - connection: { status: "connected", error: null, issueSeverity: "none" }, - }); + result.current.store.getState().setConnectionStatus("connected"); }); - await waitFor(() => expect(result.current.comments.isLoading).toBe(true)); - act(() => { + expect(result.current.first.isLoading).toBe(true); + await act(async () => { result.current.store.getState().setTaskPlan(TASK_ID, { ...taskPlan, id: "plan-2" }); - result.current.store.getState().setTaskPlanCommentsLoading(TASK_ID, true); - rejectOld(new Error("old plan failed")); - }); - - await waitFor(() => { - expect(result.current.store.getState().taskPlans.commentsLoadingByTaskId[TASK_ID]).toBe(true); - expect( - result.current.store.getState().taskPlans.commentsErrorByTaskId[TASK_ID], - ).toBeUndefined(); }); + expect(api.getTaskPlanComments).toHaveBeenCalledOnce(); + await act(async () => rejectOld(new Error("old plan failed"))); + expect(api.getTaskPlanComments).toHaveBeenCalledTimes(2); + expect(result.current.first.isLoading).toBe(true); + expect(result.current.second.isLoading).toBe(true); + expect(result.current.first.loadError).toBeNull(); + expect(result.current.second.loadError).toBeNull(); + await act(async () => finishReplacement(replacement)); + expect(result.current.first.isLoading).toBe(false); + expect(result.current.second.isLoading).toBe(false); + expect(result.current.first.snapshot).toEqual(replacement); + expect(result.current.second.snapshot).toEqual(replacement); }); }); diff --git a/apps/web/hooks/domains/comments/use-plan-comments.ts b/apps/web/hooks/domains/comments/use-plan-comments.ts index 3bc7ffc6d3..d3d2f6603e 100644 --- a/apps/web/hooks/domains/comments/use-plan-comments.ts +++ b/apps/web/hooks/domains/comments/use-plan-comments.ts @@ -13,10 +13,10 @@ import { useAppStore, useAppStoreApi } from "@/components/state-provider"; import { createTaskPlanComment, deleteTaskPlanComment, - getTaskPlanComments, updateTaskPlanComment, } from "@/lib/api/domains/plan-comment-api"; -import { getTaskPlan } from "@/lib/api/domains/plan-api"; +import { useForegroundRefresh } from "@/hooks/use-foreground-refresh"; +import { planCommentLoaderFor } from "./plan-comment-loading"; import type { PlanComment } from "@/lib/state/slices/comments"; import type { AppState } from "@/lib/state/store"; import type { TaskPlan, TaskPlanComment } from "@/lib/types/http"; @@ -24,24 +24,8 @@ import { generateUUID } from "@/lib/utils"; import { planCommentAdmissionConflict } from "@/lib/plan-comment-refs"; const EMPTY_COMMENTS: PlanComment[] = []; -interface CommentLoad { - planId: string; - promise: Promise; -} - type AppStore = ReturnType; -const commentLoadsByStore = new WeakMap>(); -const planLoadsByStore = new WeakMap>>(); - -function loadsFor(registry: WeakMap>, store: AppStore) { - const existing = registry.get(store); - if (existing) return existing; - const loads = new Map(); - registry.set(store, loads); - return loads; -} - function projectPlanComment(comment: TaskPlanComment): PlanComment { return { id: comment.id, @@ -60,73 +44,6 @@ function projectPlanComment(comment: TaskPlanComment): PlanComment { }; } -function loadComments( - taskId: string, - planId: string, - store: AppStore, - errorMessage: string, -): Promise { - const commentLoads = loadsFor(commentLoadsByStore, store); - const existing = commentLoads.get(taskId); - if (existing?.planId === planId) return existing.promise; - const state = store.getState(); - state.setTaskPlanCommentsLoading(taskId, true); - state.setTaskPlanCommentsError(taskId); - const request = getTaskPlanComments(taskId) - .then((snapshot) => store.getState().setTaskPlanComments(taskId, snapshot)) - .catch((error) => { - // i18n-exempt: developer diagnostic; localized copy is stored in state. - console.error("Failed to load task plan comments:", error); - const current = store.getState(); - if (current.taskPlans.byTaskId[taskId]?.id === planId) { - current.setTaskPlanCommentsError(taskId, errorMessage); - } - }) - .finally(() => { - setTimeout(() => { - const active = commentLoads.get(taskId); - if (active?.promise === request) commentLoads.delete(taskId); - }, 0); - const current = store.getState(); - if (current.taskPlans.byTaskId[taskId]?.id === planId) { - current.setTaskPlanCommentsLoading(taskId, false); - } - }); - commentLoads.set(taskId, { planId, promise: request }); - return request; -} - -function loadPlan( - taskId: string, - store: AppStore, - errorMessage: string, - force: boolean, -): Promise { - const planLoads = loadsFor(planLoadsByStore, store); - const existing = planLoads.get(taskId); - if (existing) return existing; - const state = store.getState(); - const cached = state.taskPlans.byTaskId[taskId]; - if (!force && cached !== undefined) return Promise.resolve(cached); - state.setTaskPlanLoading(taskId, true); - state.setTaskPlanCommentsError(taskId); - const request = getTaskPlan(taskId) - .then((next) => { - store.getState().setTaskPlan(taskId, next); - return next; - }) - .catch((error) => { - // i18n-exempt: developer diagnostic; localized copy is stored below. - console.error("Failed to load task plan for comments:", error); - store.getState().setTaskPlanCommentsError(taskId, errorMessage); - store.getState().setTaskPlanLoading(taskId, false); - return null; - }) - .finally(() => planLoads.delete(taskId)); - planLoads.set(taskId, request); - return request; -} - function applyMutationFailure(error: unknown, taskId: string, state: AppState) { const snapshot = planCommentAdmissionConflict(error)?.snapshot; if (snapshot) state.setTaskPlanComments(taskId, snapshot); @@ -369,21 +286,6 @@ function usePlanCommentReconnectRefresh(connectionStatus: string, refetch: () => }, [connectionStatus, refetch]); } -function usePlanCommentForegroundRefresh(refetch: () => Promise) { - useEffect(() => { - const refreshOnFocus = () => void refetch(); - const refreshWhenVisible = () => { - if (document.visibilityState === "visible") void refetch(); - }; - window.addEventListener("focus", refreshOnFocus); - document.addEventListener("visibilitychange", refreshWhenVisible); - return () => { - window.removeEventListener("focus", refreshOnFocus); - document.removeEventListener("visibilitychange", refreshWhenVisible); - }; - }, [refetch]); -} - /** Task-owned current-plan comments shared by every session surface. */ export function usePlanComments(taskId: string | null | undefined) { const { t } = useTranslation("task"); @@ -405,16 +307,13 @@ export function usePlanComments(taskId: string | null | undefined) { const isPlanLoading = useAppStore((state) => taskId ? (state.taskPlans.loadingByTaskId[taskId] ?? false) : false, ); - const refresh = useCallback( - async (forcePlan: boolean) => { - if (!taskId) return; - const resolvedPlan = await loadPlan(taskId, store, t("failedToLoadPlanComments"), forcePlan); - if (!resolvedPlan) return; - await loadComments(taskId, resolvedPlan.id, store, t("failedToLoadPlanComments")); - }, + const loader = useMemo( + () => (taskId ? planCommentLoaderFor(store, taskId, t("failedToLoadPlanComments")) : null), [store, t, taskId], ); - const refetch = useCallback(() => refresh(true), [refresh]); + useEffect(() => loader?.attach(), [loader]); + const refetch = useCallback(() => loader?.load(true) ?? Promise.resolve(), [loader]); + const wake = useCallback(() => loader?.wake() ?? Promise.resolve(), [loader]); useEffect(() => { if ( @@ -427,10 +326,10 @@ export function usePlanComments(taskId: string | null | undefined) { ) { return; } - void refresh(false); - }, [connectionStatus, isLoaded, isLoading, isPlanLoading, loadError, refresh, taskId]); - usePlanCommentReconnectRefresh(connectionStatus, refetch); - usePlanCommentForegroundRefresh(refetch); + void loader?.load(false); + }, [connectionStatus, isLoaded, isLoading, isPlanLoading, loadError, loader, taskId]); + usePlanCommentReconnectRefresh(connectionStatus, wake); + useForegroundRefresh(wake, Boolean(taskId) && connectionStatus === "connected", loader); const comments = useMemo(() => { if (!snapshot || snapshot.comments.length === 0) return EMPTY_COMMENTS; diff --git a/apps/web/hooks/domains/comments/use-run-comment-legacy-recovery.test.tsx b/apps/web/hooks/domains/comments/use-run-comment-legacy-recovery.test.tsx new file mode 100644 index 0000000000..197bfe8924 --- /dev/null +++ b/apps/web/hooks/domains/comments/use-run-comment-legacy-recovery.test.tsx @@ -0,0 +1,191 @@ +import { cleanup, renderHook } from "@testing-library/react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { createAppStore } from "@/lib/state/store"; +import type { PlanComment } from "@/lib/state/slices/comments"; +import { COMMENTS_STORAGE_PREFIX } from "@/lib/state/slices/comments/persistence"; +import type { TaskPlan, TaskPlanCommentSnapshot, TaskSessionState } from "@/lib/types/http"; +import { sessionId, taskId } from "@/lib/types/ids"; +import { planCommentMigrationFor } from "./plan-comment-migration"; +import { useRunComment } from "./use-run-comment"; + +const api = vi.hoisted(() => ({ + createTaskPlanComment: vi.fn(), + updateTaskPlanComment: vi.fn(), + getTaskPlanComments: vi.fn(), + sendMessageRequest: vi.fn(), + queueMessage: vi.fn(), +})); +vi.mock("@/components/state-provider", () => ({ useAppStoreApi: () => mockStore })); +vi.mock("@/lib/api/domains/plan-comment-api", () => api); +vi.mock("@/hooks/use-message-handler", () => api); +vi.mock("@/lib/api/domains/queue-api", () => ({ ...api, appendToQueue: vi.fn() })); + +const TASK = taskId("task-legacy-run"); +const SESSION = sessionId("session-legacy-run"); +const PLAN = "plan-legacy-run"; +const DATE = "2026-09-02T00:00:00Z"; +const KEY = `${COMMENTS_STORAGE_PREFIX}${SESSION}`; +const legacy: PlanComment = { + id: "legacy-selected", + source: "plan", + sessionId: SESSION, + text: "Keep this feedback", + selectedText: "Step", + from: 1, + to: 5, + status: "pending", + createdAt: DATE, +}; +const persisted: PlanComment = { + ...legacy, + taskId: TASK, + planId: PLAN, + version: 1, +}; +const plan: TaskPlan = { + id: PLAN, + task_id: TASK, + title: "Plan", + content: "Step", + created_by: "agent", + created_at: DATE, + updated_at: DATE, +}; +let mockStore: ReturnType; +let detachments: Array<() => void>; +let admitted: boolean; +let serverHasComment: boolean; + +function serverSnapshot(): TaskPlanCommentSnapshot { + return { + task_id: TASK, + plan_id: PLAN, + revision: serverHasComment ? 1 : 2, + comments: serverHasComment + ? [ + { + id: legacy.id, + task_id: TASK, + plan_id: PLAN, + body: legacy.text, + selected_text: legacy.selectedText, + anchor_from: 1, + anchor_to: 5, + version: 1, + created_at: DATE, + updated_at: DATE, + }, + ] + : [], + }; +} + +async function mountRecovery(state: TaskSessionState = "WAITING_FOR_INPUT") { + mockStore = createAppStore(); + mockStore.getState().setTaskPlan(TASK, plan); + mockStore.getState().setTaskSession({ + id: SESSION, + task_id: TASK, + state, + is_primary: true, + queue_incarnation_id: "inc-legacy-run", + started_at: DATE, + updated_at: DATE, + }); + mockStore.getState().setConnectionStatus("connected"); + const recovery = planCommentMigrationFor(mockStore, TASK); + const detach = recovery.attach(vi.fn()); + detachments.push(detach); + recovery.update({ sessionIds: [SESSION], complete: true, loading: false }); + await vi.advanceTimersByTimeAsync(0); + const hook = renderHook(() => useRunComment({ sessionId: SESSION, taskId: TASK })); + return { ...hook, detach, store: mockStore }; +} + +beforeEach(() => { + vi.useFakeTimers(); + vi.resetAllMocks(); + window.sessionStorage.clear(); + window.sessionStorage.setItem(KEY, JSON.stringify([legacy])); + detachments = []; + admitted = false; + serverHasComment = false; + vi.spyOn(console, "error").mockImplementation(() => undefined); + api.createTaskPlanComment.mockImplementation(async () => { + // Replayed admission returns the current snapshot, never recreating a consumed UUID. + if (!admitted) serverHasComment = true; + admitted = true; + return serverSnapshot(); + }); + api.getTaskPlanComments.mockImplementation(async () => serverSnapshot()); + const consume = async () => { + serverHasComment = false; + }; + api.sendMessageRequest.mockImplementation(consume); + api.queueMessage.mockImplementation(consume); +}); + +afterEach(() => { + cleanup(); + for (const detach of detachments) detach(); + vi.restoreAllMocks(); + vi.useRealTimers(); + window.sessionStorage.clear(); +}); + +describe("Run during selected legacy recovery", () => { + // @covers AC-TASKS-PLAN-COMMENTS-004.3, AC-TASKS-PLAN-COMMENTS-004.7 + it.each(["WAITING_FOR_INPUT", "RUNNING"] as const)( + "waits for selected cleanup before Run and reload in %s", + async (state) => { + const removal = vi.spyOn(window.sessionStorage, "removeItem").mockImplementation(() => { + throw new DOMException("denied", "SecurityError"); + }); + const first = await mountRecovery(state); + expect(first.store.getState().taskPlans.commentsByTaskId[TASK]?.comments).toHaveLength(1); + await expect(first.result.current.runComment(persisted)).rejects.toMatchObject({ + code: "plan-comment-not-persisted", + }); + expect(api.sendMessageRequest).not.toHaveBeenCalled(); + expect(api.queueMessage).not.toHaveBeenCalled(); + expect(JSON.parse(window.sessionStorage.getItem(KEY)!)).toEqual([legacy]); + first.unmount(); + first.detach(); + removal.mockRestore(); + + const reloaded = await mountRecovery(state); + expect(window.sessionStorage.getItem(KEY)).toBeNull(); + await expect(reloaded.result.current.runComment(persisted)).resolves.toEqual({ + queued: state === "RUNNING", + }); + expect(api.sendMessageRequest.mock.calls.length + api.queueMessage.mock.calls.length).toBe(1); + expect(serverHasComment).toBe(false); + reloaded.unmount(); + reloaded.detach(); + + const afterRun = await mountRecovery(state); + expect(api.createTaskPlanComment).toHaveBeenCalledTimes(2); + expect(afterRun.store.getState().taskPlans.commentsMigrationByTaskId[TASK]).toMatchObject({ + status: "complete", + pendingCount: 0, + }); + expect(window.sessionStorage.getItem(KEY)).toBeNull(); + }, + ); + + it("allows Run once its own row is cleaned while another legacy row is pending", async () => { + const other = { ...legacy, id: "legacy-other" }; + window.sessionStorage.setItem(KEY, JSON.stringify([legacy, other])); + const create = api.createTaskPlanComment.getMockImplementation()!; + api.createTaskPlanComment.mockImplementation(async (input: { id: string }) => { + if (input.id === other.id) throw new Error("temporarily unavailable"); + return create(input); + }); + const { store, result } = await mountRecovery(); + expect(store.getState().taskPlans.commentsMigrationByTaskId[TASK]?.pendingCount).toBe(1); + await expect(result.current.runComment(persisted)).resolves.toEqual({ queued: false }); + expect(api.sendMessageRequest).toHaveBeenCalledOnce(); + expect(JSON.parse(window.sessionStorage.getItem(KEY)!)).toEqual([other]); + expect(store.getState().taskPlans.commentsMigrationByTaskId[TASK]?.pendingCount).toBe(1); + }); +}); diff --git a/apps/web/hooks/domains/comments/use-run-comment-primary-recovery.test.ts b/apps/web/hooks/domains/comments/use-run-comment-primary-recovery.test.ts index b25a7eb46a..b41c7e25b6 100644 --- a/apps/web/hooks/domains/comments/use-run-comment-primary-recovery.test.ts +++ b/apps/web/hooks/domains/comments/use-run-comment-primary-recovery.test.ts @@ -122,7 +122,11 @@ function makeStoreState() { taskSessionsByTask: { itemsByTaskId: { [TASK_ID]: [primary, selected] } as Record, }, - taskPlans: { commentsMigrationStatusByTaskId: { [TASK_ID]: "complete" } }, + taskPlans: { + commentsMigrationByTaskId: { + [TASK_ID]: { status: "complete", pendingCount: 0, failure: null }, + }, + }, queue: { metaBySessionId: {} }, chatInput: { planModeBySessionId: {} }, addMessage: mockAddMessage, @@ -175,6 +179,23 @@ function setup() { describe("useRunComment primary-session recovery", () => { beforeEach(setup); + it("runs a persisted comment despite unrelated legacy recovery", async () => { + mockStoreState.taskPlans.commentsMigrationByTaskId[TASK_ID] = { + status: "failed", + pendingCount: 1, + failure: null, + }; + const { result } = renderCommentHook(); + await result.current.runComment(makePlanComment()); + expect(mockSendMessageRequest).toHaveBeenCalledWith( + expect.objectContaining({ + resolvedSessionId: PRIMARY_SESSION_ID, + planCommentRefs: [{ id: "comment-1", version: 2 }], + }), + ); + expect(mockQueueMessage).not.toHaveBeenCalled(); + }); + it("applies refreshed primary details before retry", async () => { const replacement = makeSession(NEW_PRIMARY_ID, "STARTING", false, "inc-new-primary"); mockStoreState.taskSessions.items[NEW_PRIMARY_ID] = replacement; diff --git a/apps/web/hooks/domains/comments/use-run-comment.test.ts b/apps/web/hooks/domains/comments/use-run-comment.test.ts index 271efe876f..d567ec624a 100644 --- a/apps/web/hooks/domains/comments/use-run-comment.test.ts +++ b/apps/web/hooks/domains/comments/use-run-comment.test.ts @@ -142,9 +142,6 @@ function makeStoreState(sessionState: string, planMode = false, foregroundActivi chatInput: { planModeBySessionId: { "sess-1": planMode }, }, - taskPlans: { - commentsMigrationStatusByTaskId: { "task-1": "complete" }, - }, queue: { metaBySessionId: {} as Record }, addMessage: mockAddMessage, setTaskPlanComments: mockSetTaskPlanComments, @@ -487,19 +484,6 @@ describe("useRunComment — plan routing", () => { ); }); - it("rejects Run until legacy plan comments finish migrating", async () => { - const state = makeStoreState("WAITING_FOR_INPUT"); - state.taskPlans.commentsMigrationStatusByTaskId["task-1"] = "failed"; - mockStoreState = state; - const { result } = renderCommentHook(); - - await expect(result.current.runComment(makePlanComment())).rejects.toMatchObject({ - code: "plan-comment-migration-pending", - }); - expect(mockSendMessageRequest).not.toHaveBeenCalled(); - expect(mockQueueMessage).not.toHaveBeenCalled(); - }); - it("queues a busy primary as a distinct idempotent entry instead of appending", async () => { const state = makeStoreState("WAITING_FOR_INPUT"); (state.taskSessions.items["primary-session"] as { state: string }).state = "RUNNING"; diff --git a/apps/web/hooks/domains/comments/use-run-comment.ts b/apps/web/hooks/domains/comments/use-run-comment.ts index e04c9154e4..eced8e685e 100644 --- a/apps/web/hooks/domains/comments/use-run-comment.ts +++ b/apps/web/hooks/domains/comments/use-run-comment.ts @@ -30,6 +30,7 @@ import { getTaskPlanComments } from "@/lib/api/domains/plan-comment-api"; import { listTaskSessions } from "@/lib/api/domains/session-api"; import { findTaskInSnapshots } from "@/lib/kanban/find-task"; import { captureTaskSessionActivityEpochs } from "@/lib/state/slices/session/activity-epochs"; +import { hasPendingPlanCommentMigration } from "./plan-comment-migration"; /** * Format a single comment into markdown suitable for sending to the agent. @@ -211,7 +212,6 @@ export class PlanCommentRunError extends Error { readonly code: | PlanCommentRunUnavailableReason | "plan-comment-not-persisted" - | "plan-comment-migration-pending" | "plan-comments-changed" | "primary-session-changed" | "delivery-failed", @@ -227,8 +227,6 @@ function planCommentRunErrorMessage(code: PlanCommentRunError["code"]): string { return t("task:noPrimarySessionForPlanComment"); case "primary-session-unavailable": return t("task:primarySessionUnavailableForPlanComment"); - case "plan-comment-migration-pending": - return t("task:planCommentMigrationPending"); case "plan-comment-not-persisted": return t("task:planCommentNotReadyToRun"); case "plan-comments-changed": @@ -246,10 +244,11 @@ async function runTaskPlanComment( storeApi: ReturnType, clientAdmissionId: string, ): Promise<{ queued: boolean }> { - if (storeApi.getState().taskPlans.commentsMigrationStatusByTaskId[taskId] !== "complete") { - throw new PlanCommentRunError("plan-comment-migration-pending"); - } - if (!comment.version || comment.version < 1) { + if ( + !comment.version || + comment.version < 1 || + hasPendingPlanCommentMigration(storeApi, taskId, comment.id) + ) { throw new PlanCommentRunError("plan-comment-not-persisted"); } const availability = resolvePlanCommentRunAvailability(storeApi.getState(), taskId); diff --git a/apps/web/lib/plan-comment-recovery.test.ts b/apps/web/lib/plan-comment-recovery.test.ts new file mode 100644 index 0000000000..81410240f6 --- /dev/null +++ b/apps/web/lib/plan-comment-recovery.test.ts @@ -0,0 +1,33 @@ +import { describe, expect, it } from "vitest"; +import { planCommentRecovery, planCommentRecoveryDelay } from "./plan-comment-recovery"; + +describe("plan comment recovery admission", () => { + it.each(["idle", "running", "retrying", "complete", "waiting_for_plan", "failed"] as const)( + "does not block empty %s state", + (status) => { + expect(planCommentRecovery({ status, pendingCount: 0, failure: "transient" })).toMatchObject({ + isBlocking: false, + needsAttention: false, + }); + }, + ); + it("blocks mixed feedback until the remaining identified row is acknowledged", () => { + expect( + planCommentRecovery({ status: "retrying", pendingCount: 1, failure: "transient" }), + ).toMatchObject({ isBlocking: true, needsAttention: false }); + }); + it.each(["failed", "waiting_for_plan"] as const)( + "requests attention for identified feedback in %s state", + (status) => { + expect(planCommentRecovery({ status, pendingCount: 1, failure: null })).toMatchObject({ + isBlocking: true, + needsAttention: true, + }); + }, + ); + it("caps retries after the three-attempt burst", () => { + expect([1, 2, 3, 4, 5, 6, 99].map(planCommentRecoveryDelay)).toEqual([ + 1000, 2000, 30000, 60000, 120000, 120000, 120000, + ]); + }); +}); diff --git a/apps/web/lib/plan-comment-recovery.ts b/apps/web/lib/plan-comment-recovery.ts new file mode 100644 index 0000000000..cddcbf9ad0 --- /dev/null +++ b/apps/web/lib/plan-comment-recovery.ts @@ -0,0 +1,25 @@ +import type { PlanCommentMigrationState } from "@/lib/state/slices/session/types"; + +export const EMPTY_PLAN_COMMENT_MIGRATION: PlanCommentMigrationState = { + status: "idle", + pendingCount: 0, + failure: null, +}; + +/** Only identified, unacknowledged feedback can be omitted by composer Send. */ +export function planCommentRecovery(state = EMPTY_PLAN_COMMENT_MIGRATION) { + const isBlocking = state.pendingCount > 0; + return { + ...state, + isBlocking, + isReady: !isBlocking, + needsAttention: + isBlocking && (state.status === "failed" || state.status === "waiting_for_plan"), + }; +} + +/** Three connected attempts, then a capped background backoff. */ +export function planCommentRecoveryDelay(failures: number) { + if (failures < 3) return failures * 1000; + return Math.min(30_000 * 2 ** (failures - 3), 120_000); +} diff --git a/apps/web/lib/state/app-state-types.ts b/apps/web/lib/state/app-state-types.ts index 6042c32db9..43c382f37b 100644 --- a/apps/web/lib/state/app-state-types.ts +++ b/apps/web/lib/state/app-state-types.ts @@ -518,9 +518,9 @@ export type AppState = KanbanSlice & { setTaskPlanComments: (taskId: string, snapshot: TaskPlanCommentSnapshot) => void; setTaskPlanCommentsLoading: (taskId: string, loading: boolean) => void; setTaskPlanCommentsError: (taskId: string, error?: string) => void; - setTaskPlanCommentMigrationStatus: ( + setTaskPlanCommentMigrationState: ( taskId: string, - status: import("./slices/session/types").PlanCommentMigrationStatus, + state: import("./slices/session/types").PlanCommentMigrationState, ) => void; clearTaskPlan: (taskId: string) => void; markTaskPlanSeen: (taskId: string) => void; diff --git a/apps/web/lib/state/slices/comments/persistence.test.ts b/apps/web/lib/state/slices/comments/persistence.test.ts index 1ff9b406b1..ea8a1eb6b1 100644 --- a/apps/web/lib/state/slices/comments/persistence.test.ts +++ b/apps/web/lib/state/slices/comments/persistence.test.ts @@ -1,4 +1,4 @@ -import { beforeEach, describe, expect, it } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { DiffComment, PlanComment } from "./types"; import { COMMENTS_STORAGE_PREFIX, @@ -46,6 +46,21 @@ function stored(): unknown[] { describe("legacy plan comment persistence", () => { beforeEach(() => window.sessionStorage.clear()); + afterEach(() => vi.restoreAllMocks()); + + it("does not acknowledge a row when browser storage refuses cleanup", () => { + const comment = planComment("plan-1"); + window.sessionStorage.setItem( + `${COMMENTS_STORAGE_PREFIX}${SESSION_ID}`, + JSON.stringify([comment]), + ); + vi.spyOn(Storage.prototype, "removeItem").mockImplementation(() => { + throw new DOMException("Storage unavailable", "SecurityError"); + }); + + expect(removeAcknowledgedLegacyPlanComment(SESSION_ID, comment)).toBe(false); + expect(stored()).toEqual([comment]); + }); it("lists only readable pending plan rows for the requested sessions", () => { window.sessionStorage.setItem( diff --git a/apps/web/lib/state/slices/comments/persistence.ts b/apps/web/lib/state/slices/comments/persistence.ts index 2d5eb5241e..80936fce10 100644 --- a/apps/web/lib/state/slices/comments/persistence.ts +++ b/apps/web/lib/state/slices/comments/persistence.ts @@ -40,20 +40,33 @@ function isLegacyPlanComment(value: unknown): value is PlanComment { ); } -function rawSessionComments(sessionId: string): unknown[] { - const stored = getSessionStorage(`${STORAGE_PREFIX}${sessionId}`, [] as Comment[]) as unknown; - return Array.isArray(stored) ? stored : []; +function rawSessionComments(sessionId: string): unknown[] | null { + try { + const raw = window.sessionStorage.getItem(`${STORAGE_PREFIX}${sessionId}`); + const stored: unknown = raw === null ? [] : JSON.parse(raw); + return Array.isArray(stored) ? stored : null; + } catch { + return null; + } } /** Discover only valid pending legacy plan rows for known sessions. */ export function listLegacyPlanComments(sessionIds: string[]): LegacyPlanCommentRecord[] { - const result: LegacyPlanCommentRecord[] = []; + return readLegacyPlanComments(sessionIds).records; +} + +/** An unavailable scan cannot prove that previously identified drafts disappeared. */ +export function readLegacyPlanComments(sessionIds: string[]) { + const records: LegacyPlanCommentRecord[] = []; + let available = true; for (const sessionId of sessionIds) { - for (const value of rawSessionComments(sessionId)) { - if (isLegacyPlanComment(value)) result.push({ sessionId, comment: value }); + const values = rawSessionComments(sessionId); + if (values === null) available = false; + for (const value of values ?? []) { + if (isLegacyPlanComment(value)) records.push({ sessionId, comment: value }); } } - return result; + return { records, available }; } function sameLegacyPlanComment(value: unknown, expected: PlanComment): boolean { @@ -76,12 +89,21 @@ export function removeAcknowledgedLegacyPlanComment( expected: PlanComment, ): boolean { const values = rawSessionComments(sessionId); + if (values === null) return false; const index = values.findIndex((value) => sameLegacyPlanComment(value, expected)); - if (index < 0) return false; - values.splice(index, 1); - if (values.length === 0) removeSessionStorage(`${STORAGE_PREFIX}${sessionId}`); - else setSessionStorage(`${STORAGE_PREFIX}${sessionId}`, values as Comment[]); - return true; + if (index < 0) return !values.some((value) => isRecord(value) && value.id === expected.id); + try { + values.splice(index, 1); + if (values.length === 0) window.sessionStorage.removeItem(`${STORAGE_PREFIX}${sessionId}`); + else window.sessionStorage.setItem(`${STORAGE_PREFIX}${sessionId}`, JSON.stringify(values)); + // Readback confirms cleanup; a completed write alone cannot acknowledge the draft's removal. + const remaining = rawSessionComments(sessionId); + return ( + remaining !== null && !remaining.some((value) => isRecord(value) && value.id === expected.id) + ); + } catch { + return false; + } } export const COMMENTS_STORAGE_PREFIX = STORAGE_PREFIX; diff --git a/apps/web/lib/state/slices/session/session-slice.ts b/apps/web/lib/state/slices/session/session-slice.ts index e5ed68989d..245076d35a 100644 --- a/apps/web/lib/state/slices/session/session-slice.ts +++ b/apps/web/lib/state/slices/session/session-slice.ts @@ -220,7 +220,7 @@ export const defaultSessionState: SessionSliceState = { commentsLoadingByTaskId: {}, commentsLoadedByTaskId: {}, commentsErrorByTaskId: {}, - commentsMigrationStatusByTaskId: {}, + commentsMigrationByTaskId: {}, revisionsByTaskId: {}, revisionsLoadingByTaskId: {}, revisionsLoadedByTaskId: {}, @@ -378,7 +378,8 @@ function reconcilePlanCommentIdentity( const previousPlanId = previousPlan?.id ?? null; if (previousPlan !== undefined && previousPlanId === nextPlanId) return; if (previousPlanId !== nextPlanId) { - taskPlans.commentsMigrationStatusByTaskId[taskId] = "idle"; + const pendingCount = taskPlans.commentsMigrationByTaskId[taskId]?.pendingCount ?? 0; + taskPlans.commentsMigrationByTaskId[taskId] = { status: "idle", pendingCount, failure: null }; } const snapshot = taskPlans.commentsByTaskId[taskId]; if (nextPlanId !== null && snapshot?.plan_id === nextPlanId) return; @@ -414,12 +415,12 @@ function buildTaskPlanCommentActions(set: ImmerSet) { if (error) draft.taskPlans.commentsErrorByTaskId[taskId] = error; else delete draft.taskPlans.commentsErrorByTaskId[taskId]; }), - setTaskPlanCommentMigrationStatus: ( + setTaskPlanCommentMigrationState: ( taskId: string, - status: Parameters[1], + state: Parameters[1], ) => set((draft) => { - draft.taskPlans.commentsMigrationStatusByTaskId[taskId] = status; + draft.taskPlans.commentsMigrationByTaskId[taskId] = state; }), }; } @@ -468,7 +469,7 @@ function buildTaskPlanActions(set: ImmerSet, get: ImmerGet) { delete draft.taskPlans.commentsLoadingByTaskId[taskId]; delete draft.taskPlans.commentsLoadedByTaskId[taskId]; delete draft.taskPlans.commentsErrorByTaskId[taskId]; - delete draft.taskPlans.commentsMigrationStatusByTaskId[taskId]; + delete draft.taskPlans.commentsMigrationByTaskId[taskId]; delete draft.taskPlans.revisionsByTaskId[taskId]; delete draft.taskPlans.revisionsLoadingByTaskId[taskId]; delete draft.taskPlans.revisionsLoadedByTaskId[taskId]; diff --git a/apps/web/lib/state/slices/session/task-plan-comment-actions.test.ts b/apps/web/lib/state/slices/session/task-plan-comment-actions.test.ts index 81b42b0a84..acb9e01df3 100644 --- a/apps/web/lib/state/slices/session/task-plan-comment-actions.test.ts +++ b/apps/web/lib/state/slices/session/task-plan-comment-actions.test.ts @@ -56,27 +56,43 @@ describe("task plan comment state", () => { const store = createAppStore(); store.getState().setTaskPlan(TASK_ID, plan()); store.getState().setTaskPlanComments(TASK_ID, snapshot(2)); - store.getState().setTaskPlanCommentMigrationStatus(TASK_ID, "complete"); + store.getState().setTaskPlanCommentMigrationState(TASK_ID, { + status: "failed", + pendingCount: 2, + failure: "conflict", + }); store.getState().setTaskPlanComments(TASK_ID, snapshot(99, "old-plan")); expect(store.getState().taskPlans.commentsByTaskId[TASK_ID]).toEqual(snapshot(2)); store.getState().setTaskPlan(TASK_ID, plan("plan-2")); expect(store.getState().taskPlans.commentsByTaskId[TASK_ID]).toBeUndefined(); expect(store.getState().taskPlans.commentsLoadedByTaskId[TASK_ID]).toBe(false); - expect(store.getState().taskPlans.commentsMigrationStatusByTaskId[TASK_ID]).toBe("idle"); + expect(store.getState().taskPlans.commentsMigrationByTaskId[TASK_ID]).toEqual({ + status: "idle", + pendingCount: 2, + failure: null, + }); }); it("clears the task snapshot when the plan is deleted", () => { const store = createAppStore(); store.getState().setTaskPlan(TASK_ID, plan()); store.getState().setTaskPlanComments(TASK_ID, snapshot(1)); - store.getState().setTaskPlanCommentMigrationStatus(TASK_ID, "complete"); + store.getState().setTaskPlanCommentMigrationState(TASK_ID, { + status: "failed", + pendingCount: 2, + failure: "conflict", + }); store.getState().setTaskPlan(TASK_ID, null); expect(store.getState().taskPlans.commentsByTaskId[TASK_ID]).toBeUndefined(); expect(store.getState().taskPlans.commentsLoadedByTaskId[TASK_ID]).toBe(true); - expect(store.getState().taskPlans.commentsMigrationStatusByTaskId[TASK_ID]).toBe("idle"); + expect(store.getState().taskPlans.commentsMigrationByTaskId[TASK_ID]).toEqual({ + status: "idle", + pendingCount: 2, + failure: null, + }); }); it("keeps comment load state when the same plan is refreshed", () => { diff --git a/apps/web/lib/state/slices/session/types.ts b/apps/web/lib/state/slices/session/types.ts index b3c7b681bb..15c40559ec 100644 --- a/apps/web/lib/state/slices/session/types.ts +++ b/apps/web/lib/state/slices/session/types.ts @@ -129,10 +129,17 @@ export type ComparePair = [string | null, string | null]; export type PlanCommentMigrationStatus = | "idle" | "running" + | "retrying" | "complete" | "waiting_for_plan" | "failed"; +export type PlanCommentMigrationState = { + status: PlanCommentMigrationStatus; + pendingCount: number; + failure: "transient" | "conflict" | "rejected" | null; +}; + export type TaskPlansState = { byTaskId: Record; loadingByTaskId: Record; @@ -142,7 +149,7 @@ export type TaskPlansState = { commentsLoadingByTaskId: Record; commentsLoadedByTaskId: Record; commentsErrorByTaskId: Record; - commentsMigrationStatusByTaskId: Record; + commentsMigrationByTaskId: Record; revisionsByTaskId: Record; revisionsLoadingByTaskId: Record; revisionsLoadedByTaskId: Record; @@ -402,7 +409,7 @@ export type SessionSliceActions = { setTaskPlanComments: (taskId: string, snapshot: TaskPlanCommentSnapshot) => void; setTaskPlanCommentsLoading: (taskId: string, loading: boolean) => void; setTaskPlanCommentsError: (taskId: string, error?: string) => void; - setTaskPlanCommentMigrationStatus: (taskId: string, status: PlanCommentMigrationStatus) => void; + setTaskPlanCommentMigrationState: (taskId: string, state: PlanCommentMigrationState) => void; clearTaskPlan: (taskId: string) => void; markTaskPlanSeen: (taskId: string) => void; // Revision actions diff --git a/apps/web/scripts/lib/zh-hant-overrides.json b/apps/web/scripts/lib/zh-hant-overrides.json index 2def6c4e7d..523b370f20 100644 --- a/apps/web/scripts/lib/zh-hant-overrides.json +++ b/apps/web/scripts/lib/zh-hant-overrides.json @@ -19,6 +19,7 @@ "withManagedWorkspaceCredentialsKandevConfigures": "使用受管理的工作區認證資訊時，Kandev 會在新啟動的任務處理程序中，將 agentctl 設為 Git 的認證輔助程式。當 Git 要求取得已附加 GitHub HTTPS 儲存庫的認證資訊時，輔助程式會兌換僅適用於該任務與儲存庫的 broker 租約。認證資訊會在需要時傳回 Git，且不會寫入儲存庫或 Git 設定。gh 命令會使用 Kandev 支援 broker 的轉接程式。繼承執行器設定則會略過這兩者，改用所選執行器可見的認證資訊。變更會套用至新啟動的任務及下次繼續執行，不會影響已在執行的處理程序。" }, "task": { + "planCommentMigrationPending": "仍在恢復已儲存的計劃評論。您的訊息會保留。", "proxy": "代理伺服器", "pathBasedProxyWorksForApis": "路徑型代理伺服器。適用於 API，但可能導致預期由 / 提供服務的 Web 應用程式無法正常運作。" }, diff --git a/apps/web/src/locales/en/task.json b/apps/web/src/locales/en/task.json index a87b1ef649..2338211e5d 100644 --- a/apps/web/src/locales/en/task.json +++ b/apps/web/src/locales/en/task.json @@ -2122,8 +2122,7 @@ "planCommentNotReadyToRun": "This plan comment is still being saved. Try Run again.", "planCommentsChangedRetry": "The plan comments changed. Review them and send again.", "primarySessionChangedRetry": "The primary session changed. Try Run again.", - "restoringSavedPlanComments": "Restoring saved plan comments...", "planCommentMigrationFailed": "Some saved plan comments could not be restored.", "planCommentMigrationNeedsPlan": "Create or restore a plan to recover saved comments.", - "planCommentMigrationPending": "Saved plan comments are still being restored. Retry before sending." + "planCommentMigrationPending": "Saved plan comments are still being restored. Your message is kept." } diff --git a/apps/web/src/locales/pseudo/task.json b/apps/web/src/locales/pseudo/task.json index fa0f3162a3..da30ea6739 100644 --- a/apps/web/src/locales/pseudo/task.json +++ b/apps/web/src/locales/pseudo/task.json @@ -2122,8 +2122,7 @@ "planCommentNotReadyToRun": "Ţĥĩś ƥĺàń ćōḿḿēńţ ĩś śţĩĺĺ ƀēĩńĝ śàvēď. Ţŕŷ Ŕũń àĝàĩń.", "planCommentsChangedRetry": "Ţĥē ƥĺàń ćōḿḿēńţś ćĥàńĝēď. Ŕēvĩēŵ ţĥēḿ àńď śēńď àĝàĩń.", "primarySessionChangedRetry": "Ţĥē ƥŕĩḿàŕŷ śēśśĩōń ćĥàńĝēď. Ţŕŷ Ŕũń àĝàĩń.", - "restoringSavedPlanComments": "Ŕēśţōŕĩńĝ śàvēď ƥĺàń ćōḿḿēńţś...", "planCommentMigrationFailed": "Śōḿē śàvēď ƥĺàń ćōḿḿēńţś ćōũĺď ńōţ ƀē ŕēśţōŕēď.", "planCommentMigrationNeedsPlan": "Ćŕēàţē ōŕ ŕēśţōŕē à ƥĺàń ţō ŕēćōvēŕ śàvēď ćōḿḿēńţś.", - "planCommentMigrationPending": "Śàvēď ƥĺàń ćōḿḿēńţś àŕē śţĩĺĺ ƀēĩńĝ ŕēśţōŕēď. Ŕēţŕŷ ƀēƒōŕē śēńďĩńĝ." + "planCommentMigrationPending": "Śàvēď ƥĺàń ćōḿḿēńţś àŕē śţĩĺĺ ƀēĩńĝ ŕēśţōŕēď. Ŷōũŕ ḿēśśàĝē ĩś ķēƥţ." } diff --git a/apps/web/src/locales/pt-pt/task.json b/apps/web/src/locales/pt-pt/task.json index 943a3154f0..4b4a1e8f65 100644 --- a/apps/web/src/locales/pt-pt/task.json +++ b/apps/web/src/locales/pt-pt/task.json @@ -2122,8 +2122,7 @@ "planCommentNotReadyToRun": "Este comentário do plano ainda está a ser guardado. Tente executar novamente.", "planCommentsChangedRetry": "Os comentários do plano foram alterados. Reveja-os e envie novamente.", "primarySessionChangedRetry": "A sessão principal foi alterada. Tente executar novamente.", - "restoringSavedPlanComments": "A restaurar comentários do plano guardados...", "planCommentMigrationFailed": "Não foi possível restaurar alguns comentários do plano guardados.", "planCommentMigrationNeedsPlan": "Crie ou restaure um plano para recuperar os comentários guardados.", - "planCommentMigrationPending": "Os comentários do plano guardados ainda estão a ser restaurados. Tente novamente antes de enviar." + "planCommentMigrationPending": "Os comentários do plano guardados ainda estão a ser restaurados. A sua mensagem é mantida." } diff --git a/apps/web/src/locales/zh-cn/task.json b/apps/web/src/locales/zh-cn/task.json index 554db52ce9..e47384cb5c 100644 --- a/apps/web/src/locales/zh-cn/task.json +++ b/apps/web/src/locales/zh-cn/task.json @@ -2122,8 +2122,7 @@ "planCommentNotReadyToRun": "此计划评论仍在保存。请再次运行。", "planCommentsChangedRetry": "计划评论已更改。请检查后重新发送。", "primarySessionChangedRetry": "主会话已更改。请再次运行。", - "restoringSavedPlanComments": "正在恢复已保存的计划评论...", "planCommentMigrationFailed": "无法恢复部分已保存的计划评论。", "planCommentMigrationNeedsPlan": "请创建或恢复计划，以恢复已保存的评论。", - "planCommentMigrationPending": "仍在恢复已保存的计划评论。请重试后再发送。" + "planCommentMigrationPending": "仍在恢复已保存的计划评论。您的消息会保留。" } diff --git a/apps/web/src/locales/zh-hk/task.json b/apps/web/src/locales/zh-hk/task.json index 4bd4d7cd69..f6bec76ca8 100644 --- a/apps/web/src/locales/zh-hk/task.json +++ b/apps/web/src/locales/zh-hk/task.json @@ -2122,8 +2122,7 @@ "planCommentNotReadyToRun": "此計劃評論仍在儲存。請再次執行。", "planCommentsChangedRetry": "計劃評論已更改。請檢查後重新傳送。", "primarySessionChangedRetry": "主工作階段已更改。請再次執行。", - "restoringSavedPlanComments": "正在恢復已儲存的計劃評論...", "planCommentMigrationFailed": "無法恢復部分已儲存的計劃評論。", "planCommentMigrationNeedsPlan": "請建立或恢復計劃，以恢復已儲存的評論。", - "planCommentMigrationPending": "仍在恢復已儲存的計劃評論。請重試後再傳送。" + "planCommentMigrationPending": "仍在恢復已儲存的計劃評論。您的訊息會保留。" } diff --git a/apps/web/src/locales/zh-tw/task.json b/apps/web/src/locales/zh-tw/task.json index c27da9ba72..633f0029cb 100644 --- a/apps/web/src/locales/zh-tw/task.json +++ b/apps/web/src/locales/zh-tw/task.json @@ -2122,8 +2122,7 @@ "planCommentNotReadyToRun": "此計劃評論仍在儲存。請再次執行。", "planCommentsChangedRetry": "計劃評論已更改。請檢查後重新傳送。", "primarySessionChangedRetry": "主工作階段已更改。請再次執行。", - "restoringSavedPlanComments": "正在恢復已儲存的計劃評論...", "planCommentMigrationFailed": "無法恢復部分已儲存的計劃評論。", "planCommentMigrationNeedsPlan": "請建立或恢復計劃，以恢復已儲存的評論。", - "planCommentMigrationPending": "仍在恢復已儲存的計劃評論。請重試後再傳送。" + "planCommentMigrationPending": "仍在恢復已儲存的計劃評論。您的訊息會保留。" } diff --git a/docs/plans/plan-comment-recovery/plan.md b/docs/plans/plan-comment-recovery/plan.md new file mode 100644 index 0000000000..97b9cf4dfe --- /dev/null +++ b/docs/plans/plan-comment-recovery/plan.md @@ -0,0 +1,280 @@ +--- +created: 2026-09-11 +status: implemented +requirements: + - REQ-TASKS-PLAN-COMMENTS-001 + - REQ-TASKS-PLAN-COMMENTS-002 + - REQ-TASKS-PLAN-COMMENTS-003 + - REQ-TASKS-PLAN-COMMENTS-004 +system_design: + - ../../specs/tasks/system-design/plan-comments.md +legacy_specs: [] +--- + +# Implementation Plan: Plan comment recovery + +## Overview + +Keep ordinary chat usable when background plan-comment reads fail, and recover +actual legacy feedback without a manual Retry action. One focused work order +owns the frontend correction and its desktop/mobile regression evidence. The +task system owns this repair because it controls feedback persistence and +delivery; the existing Plan and composer surfaces present that state. + +This follows the completed +[task-owned plan-comment package](../task-owned-plan-comments/plan.md), especially +its [migration work](../task-owned-plan-comments/task-04-migrate-legacy-browser-drafts.md) +and [responsive scenarios](../task-owned-plan-comments/task-05-prove-responsive-multi-session-behavior.md). +The earlier verification results are historical and do not certify this repair. + +## Confirmed defect + +PR #3332, commit `f45450fe8`, added an unconditional comment-list request after +legacy migration, including when the legacy collection is empty. Its failure +marks the task's migration failed. Session discovery and ordinary plan/comment +read errors can set the same failure before a single legacy row is identified. +The effect then exits on `failed`, so a later successful read cannot release +the gate. Structured and passthrough Send reject every message on that gate. + +A temporary harness executed the actual migration source with simulated +transport and hook/state boundaries. It reproduced empty-storage failure, +successful refresh leaving Send blocked, explicit Retry recovery, and failed +plan discovery staying blocked after confirming there is no plan. These are +source-level reproductions, not a completed React or browser regression suite. +The live frontend diagnostic bundle contained no browser logs, so the exact +network event on the reporting phone remains unconfirmed. + +## Scope + +### In scope + +- Background task-scoped legacy discovery, exact acknowledgement cleanup, + quiet transient recovery, and bounded retries. +- Separate ordinary snapshot loading from legacy recovery restrictions. +- Standard and passthrough Send, selected-comment Run, and inline actionable + recovery feedback in Plan and chat. +- Shared desktop/phone behavior, translated pending-action copy, focused tests, + and one recovery paragraph in the existing public task guide. + +### Out of scope + +- Backend schema, WebSocket payloads, queue admission, primary routing, or + comment ownership changes. +- Global WebSocket timeout/retry changes, browser-wide storage migration, + recovery modals, new settings, or layout redesign. +- Reconstructing drafts already deleted by older releases or changing other + comment types' persistence. +- Creating platform tasks/sessions, delegation, publication, or changes to the + user's running instance. + +## Technical approach + +- Replace the task-plan status-only migration map with the recovery projection + described in the [system design](../../specs/tasks/system-design/plan-comments.md#recovery-state-and-lifetime). + Remove all consumers of the old map; do not keep parallel readiness flags. +- Keep orchestration behind `usePlanCommentMigration`. A small adjacent + coordinator can hold store-scoped promises, timers, known pending records, + and subscriptions. Use the existing `useForegroundRefresh` pattern and + connection state; coalesce duplicate consumers and foreground signals. +- Stop importing `getTaskPlanComments` into migration merely for a final + refresh. Reconcile acknowledged create snapshots, and finish empty scans + without a migration network request. `usePlanComments` remains the read owner. +- Preserve identified pending records across failed storage reads, partial + upload, task/plan changes, and retry. Never treat another session's row or a + generic load error as proof of drafts belonging to this task. +- Share the ordinary-Send restriction predicate between structured and + passthrough consumers. Keep displayed persisted refs and backend validation. + Run checks only its selected persisted comment and primary destination. +- Render `PlanCommentMigrationNotice` only for actionable identified draft + recovery. Background phases have no banner. Pending-Send feedback preserves + the draft and explains recovery without requiring migration terminology. + +The accepted ownership/admission +[ADR](../../decisions/2026-09-02-task-owned-plan-comments.md) still applies. +The recovery rationale fits the requirement/design; no new ADR is needed. + +## ASCII UI preview + +### UI-01: Empty task, or no identified legacy feedback + +Entry: open any task composer, including after a failed background read. +The current failure and corrected shared composition are: + +```text +Before (source-confirmed): +[Some saved plan comments could not be restored.] [Retry] +[Your message ] [Send] + -> rejected + +After (desktop and phone): +[Existing context, when present] +[Your message ] [Send] + -> delivered +``` + +Required: no restoration banner or migration restriction solely from a pending +or failed read. This does not override actual offline/session input constraints. +The phone uses the current task chat layout and its existing reachable Send +control; the diagram describes hierarchy, not a new horizontal phone layout. +Maps to `AC-TASKS-PLAN-COMMENTS-004.5`, `.6`, and `.8`. + +### UI-02: Actual legacy feedback requires attention + +Entry: the same Plan or chat surface, after known draft recovery exhausts its +initial retry burst or encounters an actionable rejection. + +```text +Desktop: +[Saved plan feedback needs attention. ] [Retry] +[Your retained message ] [Send blocked] + +Phone (same inline region, wrapping when needed): +[Saved plan feedback needs attention.] +[Retry: touch target >=44px ] +[Your retained message ] +[Existing composer actions + Send blocked] +``` + +Required: only genuine unresolved task feedback can produce this row; Send +preserves the draft while that feedback would be omitted. Automatic transient +attempts do not mount the row. A persisted selected-comment Run remains usable +if its other eligibility conditions pass. Copy is illustrative and localized; +reuse the existing actionable copy where it already describes the state. +Maps to `AC-TASKS-PLAN-COMMENTS-004.1`, `.3`, `.4`, `.7`, and `.8`. + +The closest shipped composition is `task-layout.tsx` with its dedicated mobile +chat and Plan Drawer; the existing `PlanCommentMigrationNotice` is the inline +region being corrected. Inline feedback fits a brief recovery action without +displacing chat. Preserve its scroll owner, keyboard focus, and safe-area +clearance. Keep desktop Retry at 28 px and coarse-pointer Retry at least 44 px; +do not create an overlay, navigation destination, or extra scroll region. + +## Tests + +| Regression evidence | Acceptance criteria | +| --- | --- | +| `use-plan-comment-migration.test.tsx`: no legacy rows, unknown/absent/current plan, failed discovery, later successful empty read, unrelated task records | `004.5`, `004.6` | +| Migration coordinator and persistence tests: partial success, same-UUID replay, unreadable storage after discovery, conflict, missing plan, plan change, unmount, simultaneous consumers | `004.1`, `004.2`, `004.3`, `004.6`, `004.7`, `004.8` | +| `use-plan-comments.test.tsx`: connected/foreground retries, coalescing, empty success, preserved displayed snapshot, no migration failure from a read error | `001.3`, `004.5`, `004.6`, `004.7` | +| Structured/passthrough submission tests and Run tests: plain Send allowed, identified unresolved row blocks Send, persisted selected Run allowed, retained text and refs | `002.2`, `002.6`, `003.1`, `003.6`, `004.7` | +| Notice component tests: quiet background phases, actual actionable state, manual Retry, focus and draft preservation | `004.4`, `004.8` | + +All abbreviated IDs in this table use the prefix `AC-TASKS-PLAN-COMMENTS-`. +The [work order](task-01-recover-plan-comment-context.md) names the mandatory +RED cases and exact commands. Preserve existing scenario coverage. + +## E2E tests + +Extend the existing files, with shared failure-injection helpers beside them: + +- `apps/web/e2e/tests/session/task-plan-comments.spec.ts`, project `chromium`: + keep the existing Send/Run routing scenario. Add no-draft failures for a + present plan and a lookup eventually confirming no plan. Leave chat transport + healthy and prove ordinary Send works without Retry. Add actual mixed legacy + recovery, preserving a colocated diff comment and preventing omission. +- `apps/web/e2e/tests/session/mobile-task-plan-comments.spec.ts`, project + `mobile-chrome`: retain the current session-picker/Plan-Drawer migration and + routing scenario. Exercise a foreground/reconnect sequence with no drafts, + then send through the visible phone control without manual recovery. In a + separate scenario, fail one real legacy upload, allow recovery, and prove the + comment appears and is delivered once. Check the genuine failure Retry target + and absence of horizontal overflow. + +Use a correlated `page.routeWebSocket` interceptor following `ws-response-hold.ts` +and `ws-drop.ts`. Target the exact task and action, leaving unrelated frames +live; cover session-discovery failure separately at the `useTaskSessions` hook +boundary. Record the injected +failure before allowing success. Prove automatic requests and passive backend +state before any helper that can reload or click Retry. Arm causal waits before +the stimulus; no wall-clock success sleeps. Scope controls to the active chat. + +These flows cover `AC-TASKS-PLAN-COMMENTS-004.1` through `.8` and preserve the +existing `001`/`002`/`003` routing coverage. Rendered phone checks compare UI-01 +and UI-02 with the existing composition, not a new layout. + +## Work orders + +- [x] [Task 01: Recover plan comment context](task-01-recover-plan-comment-context.md) + +Execution is sequential in the primary conversation. No subagents authorized. + +## Verification results + +Design-package checks on 2026-09-11: + +- `python3 scripts/lint-spec-files.test.py`: passed all 36 tests. +- `python3 scripts/lint-spec-files.py --all`: all specification files passed. +- Read-only local-link and work-order reference audit: 31 local document links + and all 18 requirement/acceptance references passed. +- `git diff --check -- docs/specs docs/plans`: passed. +- `git status --short -- docs/specs docs/plans`: confirmed both new package + files and the intended existing specification/companion-plan edits. + +Implementation is complete. The final focused suite passes all 130 tests across +13 files. Changed-file ESLint, TypeScript, i18n checks and ratchet, specification +validation, and public-doc validation pass. The final desktop build passes all +four browser scenarios. The final phone rerun also passed all four scenarios +against that fresh build after a rebuild was terminated before tests started. +The states captured on desktop and phone were inspected; draft retention, partial +recovery, Retry sizing, and absence of horizontal overflow were verified. +The work order records the final results and the scoped public-guide update. + +PR review follow-up on 2026-09-12 adds regressions for unknown-plan failures, +known-session discovery, concurrent local edits, resume coalescing, retained +locales, and stale loading flags. The focused suite now passes 145 tests across +the same 13 files; 24 locale-generator tests also pass. Changed-file lint, +typecheck, i18n, specification and public-doc checks pass. The +[work order](task-01-recover-plan-comment-context.md#pr-review-follow-up-2026-09-12) +records review dispositions and scoped validation. Remote CI/review completion +remains an exact-head delivery check, not a claim made by this tracked plan. + +Claude's follow-up suggestions are addressed with invariant comments for shared +discovery, store-lifetime task caches, and authoritative cleanup readback. No +executable behavior changed. All 55 focused tests across the five affected +coordinator, hook, and persistence test files pass, as does changed-file ESLint. + +Post-update review remediation on 2026-09-13 preserves the user's base merge, +recognizes actual uppercase transient wire errors, and retains missing-plan +refresh intent during manual Retry. Stronger deferred-read and conflict tests +confirm serialized loading cleanup and the existing no-overwrite contract. The +browser assertion now requires delivery of both plan and diff feedback. The +focused suite passes 151 unit/component tests plus 24 locale-generator tests; +static and documentation checks pass. All eight Chromium/Pixel 5 scenarios +pass with zero retries. The work order records local verification; exact-head +remote delivery checks remain a separate gate. + +The next review follow-up scopes Run eligibility to cleanup of its selected +legacy ID, preserves in-flight uploads when a load confirms the same plan, +and uses neutral blocked-Send copy for every recovery failure class. The +work order records RED-to-GREEN evidence, 159 focused unit/component tests +plus 24 locale-generator tests, and eight zero-retry desktop/phone scenarios. +The public Run guidance and blocked-Send previews now state their eligibility +conditions explicitly. Remote exact-head checks remain separate delivery gates. + +Further current-head review distinguishes unknown from confirmed-absent plan +state, retaining the same-ID metadata exception. CI also exposed an unchanged +readiness helper matching overlapping Resume controls during SSH reconnect. +The helper now observes the first visible active-chat recovery action. The +work order records both reproductions and their scoped validation; SSH product +behavior and CI timeout/retry policy are unchanged. + +The final retry-scope follow-up resets accumulated failures when a plan is +replaced, preserving a fresh quiet burst without resetting the budget on +same-ID metadata confirmation. The work order records both failing regressions, +186 focused tests, and eight fresh zero-retry desktop/phone scenarios. Completion +wording now requires selective browser cleanup, and the plain-Send discovery +regression explicitly excludes tasks with identified unresolved drafts. + +## Risks + +- Unknown ownership must not be mistaken for task-owned feedback. Plain Send + before legacy discovery includes only visible persisted context; later + recovery never amends or resends that prompt. +- A successful subset must not release a Send that would omit the remaining + legacy rows. A failed subsequent storage read must not forget known rows. +- Retry promises and timers must remain shared across mounted Plan/composer + consumers and stop when their scope is gone. +- New retries apply to reads and idempotent draft promotion only. They must not + replay user messages, Run actions, or accepted queue entries. +- A fresh worktree needs its own frozen-lockfile install. The interrupted + diagnostic install was completed successfully before product tests. diff --git a/docs/plans/plan-comment-recovery/task-01-recover-plan-comment-context.md b/docs/plans/plan-comment-recovery/task-01-recover-plan-comment-context.md new file mode 100644 index 0000000000..8f3613a774 --- /dev/null +++ b/docs/plans/plan-comment-recovery/task-01-recover-plan-comment-context.md @@ -0,0 +1,519 @@ +--- +id: "01-recover-plan-comment-context" +title: "Recover plan comment context" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-TASKS-PLAN-COMMENTS-001 + - REQ-TASKS-PLAN-COMMENTS-002 + - REQ-TASKS-PLAN-COMMENTS-003 + - REQ-TASKS-PLAN-COMMENTS-004 +acceptance_criteria: + - AC-TASKS-PLAN-COMMENTS-001.3 + - AC-TASKS-PLAN-COMMENTS-001.7 + - AC-TASKS-PLAN-COMMENTS-002.2 + - AC-TASKS-PLAN-COMMENTS-002.6 + - AC-TASKS-PLAN-COMMENTS-003.1 + - AC-TASKS-PLAN-COMMENTS-003.6 + - AC-TASKS-PLAN-COMMENTS-004.1 + - AC-TASKS-PLAN-COMMENTS-004.2 + - AC-TASKS-PLAN-COMMENTS-004.3 + - AC-TASKS-PLAN-COMMENTS-004.4 + - AC-TASKS-PLAN-COMMENTS-004.5 + - AC-TASKS-PLAN-COMMENTS-004.6 + - AC-TASKS-PLAN-COMMENTS-004.7 + - AC-TASKS-PLAN-COMMENTS-004.8 +system_design: + - ../../specs/tasks/system-design/plan-comments.md +--- + +# Task 01: Recover plan comment context + +## Summary + +Implement the existing task plan-comment design's recovery refinements as one +working frontend change. Make empty-task Send independent of background +comment reads while automatically recovering and protecting real legacy drafts. + +## In scope + +- Task-scoped recovery state/coordinator, bounded retries, foreground and + connection readiness, exact acknowledgement, and identity/lifetime guards. +- Quiet background reads, scoped structured/passthrough Send, selected-comment + Run, and inline actionable recovery state. +- RED-first unit/component tests, focused desktop/mobile E2E, translated pending + feedback, and the existing public task guide's recovery paragraph. + +## Out of scope + +Backend persistence/admission changes, global transport behavior, new settings, +layout redesign, other comment sources' ownership, and publication. + +## Acceptance + +1. Plain Send succeeds under the no-draft failure scenarios in UI-01; recovery + errors cannot erase displayed refs, latch an empty-task block, or turn an + unrelated task's records into local restoration feedback. +2. Actual legacy recovery follows the design's bounded schedule and preserves + exact records until acknowledgement. Mixed recovered/unrecovered rows still + guard composer Send; Run of a persisted selected comment remains scoped to + that comment. Automatic recovery never submits a message. +3. UI-02 appears only for real actionable pending feedback. Structured, + passthrough, and mobile paths preserve drafts/focus and pass the named unit, + browser, i18n, specification, and public-doc checks. + +## ASCII UI preview + +Excerpts from the [full previews](plan.md#ascii-ui-preview): + +```text +UI-01 (desktop + phone hierarchy): +[Existing context, when present] +[Your message] [Existing Send control] +No restoration row for empty/discovering/transient-read states. + +UI-02 (real unresolved feedback requiring attention): +Desktop: [Saved plan feedback needs attention.] [Retry] + [Your retained message ] [Send blocked] +Phone: [Saved plan feedback needs attention.] + [Retry: touch target >=44px ] + [Your retained message ] + [Existing composer actions + Send blocked] +``` + +UI-01 covers `AC-TASKS-PLAN-COMMENTS-004.5`, `.6`, `.8`; UI-02 covers `.1`, +`.3`, `.4`, `.7`, `.8`. Reuse the existing task mobile chat and Plan Drawer +entry points. Inline wrapping, one existing scroll owner, safe-area clearance, +28 px desktop Retry, and a minimum 44 px touch target are required; spacing and +example copy are illustrative. Compare the rendered phone surface to both +states during the focused E2E run. + +## TDD sequence + +First add these named behavioral regressions against the unfixed source: + +- `empty legacy storage does not request migration refresh or block Send`. +- `failed plan discovery does not block plain Send before or after a null plan when no legacy drafts are identified`. +- `successful empty snapshot recovery clears obsolete migration restrictions`. +- `a transient legacy upload recovers without manual Retry`. +- `Run of a persisted comment ignores unrelated legacy recovery`. + +Use the actual React hook/store in `use-plan-comment-migration.test.tsx` for +the first four, real submit hooks for message acceptance, and +`use-run-comment.test.ts` for Run. Assert the expected failure before changing +production code. Replace the existing tests that intentionally equate generic +read failure with migration failure; retain real partial failure, UUID conflict, +missing-plan, exact cleanup, and primary-routing coverage. +Assert that the discovery-failure case has no identified pending rows. A +separate known-draft case must keep Send blocked until those rows are resolved. + +Add fake-timer cases for bounded attempts, coalesced resume events, connected +readiness, multiple consumers, and unmount. Keep responses deferred when proving +deduplication and late task/plan results. Include a mixed set containing an +acknowledged row, a failed row, and a non-plan row; a storage read failure after +discovery must not falsely empty the known pending set. + +For E2E, install targeted transport interception before navigation and prove the +failure stimulus was reached. Preserve existing scenarios in both plan-comment +specs. If hook RED owns a timing path that cannot be recreated faithfully in the +browser fixture, record why and add its end-to-end outcome after the fix using +a fresh managed build; do not count a missing selector as behavioral RED. + +## Files likely touched + +Existing production boundaries: + +- `apps/web/hooks/domains/comments/use-plan-comment-migration.ts` +- `apps/web/hooks/domains/comments/use-plan-comments.ts` +- `apps/web/hooks/domains/comments/use-run-comment.ts` +- `apps/web/lib/state/slices/comments/persistence.ts` +- `apps/web/lib/state/slices/session/types.ts` +- `apps/web/lib/state/slices/session/session-slice.ts` +- `apps/web/lib/state/app-state-types.ts` +- `apps/web/components/task/plan-comment-migration-notice.tsx` +- `apps/web/components/task/task-plan-panel.tsx` +- `apps/web/components/task/chat/chat-input-area.tsx` +- `apps/web/components/task/passthrough-chat-composer.tsx` + +Planned focused additions: + +- `apps/web/hooks/domains/comments/plan-comment-migration.ts`: coordinator + extracted from the hook, with `plan-comment-migration.test.ts` for retry and + exact acknowledgement behavior. +- `apps/web/hooks/domains/comments/plan-comment-loading.ts`: shared ordinary + reads and bounded retry lifecycle, exercised by `plan-comment-loading.test.ts` + and `use-plan-comments.test.tsx`. +- `apps/web/lib/plan-comment-recovery.ts`: shared recovery projection and + Send restriction policy, with `plan-comment-recovery.test.ts`. +- `apps/web/components/task/plan-comment-migration-notice.test.tsx`. +- `apps/web/hooks/domains/comments/use-run-comment-legacy-recovery.test.tsx`: + real-store recovery and Run integration for selected cleanup, reload, direct + delivery, queue admission, and unrelated pending rows. +- CI remediation: `apps/web/e2e/pages/session-page.ts` and + `apps/web/e2e/tests/session/session-page-recovery.spec.ts` scope readiness to + the active recovery action when restart surfaces overlap. +- `apps/web/e2e/tests/session/plan-comment-recovery-helpers.ts`: correlated + failure controls shared by the desktop and phone specs. + +Update the adjacent tests named in Verification, the existing two E2E specs, +and `src/locales/{en,pt-pt,zh-cn,zh-hk,zh-tw,pseudo}/task.json` when changing +pending-action copy. Generate zh-hk/zh-tw through `pnpm run i18n:zh-hant` and +follow the existing pseudo-catalog workflow. Do not remove historical i18n +guard entries. + +Add concise recovery guidance to `docs/public/tasks-and-workflows.md`, a how-to +guide: explain automatic retry, preserved feedback, and the actual Retry action +without exposing migration internals. Keep docs/spec status and package results +accurate. Root/scoped AGENTS guidance needs no change unless implementation +introduces an additional persistent architectural convention. + +## Verification + +Run from the repository root. The install is required because the previous +diagnostic turn's offline install was interrupted. The new test paths below are +part of this work order; confirm they exist and collect tests before marking +the command complete. + +```bash +(cd apps && pnpm install --frozen-lockfile) +(cd apps && pnpm --filter @kandev/web test -- lib/plan-comment-recovery.test.ts lib/state/slices/comments/persistence.test.ts lib/state/slices/session/task-plan-comment-actions.test.ts hooks/domains/comments/plan-comment-migration.test.ts hooks/domains/comments/plan-comment-loading.test.ts hooks/domains/comments/use-plan-comment-migration.test.tsx hooks/domains/comments/use-plan-comments.test.tsx hooks/domains/comments/use-run-comment.test.ts hooks/domains/comments/use-run-comment-primary-recovery.test.ts components/task/chat/chat-input-area.test.tsx components/task/passthrough-chat-composer.test.ts components/task/plan-comment-migration-notice.test.tsx components/task/task-plan-panel.session-switch.test.tsx) +(cd apps/web && pnpm run typecheck) +(cd apps/web && pnpm test -- hooks/domains/comments/use-run-comment-legacy-recovery.test.tsx) +(cd apps/web && pnpm exec eslint hooks/domains/comments/use-run-comment-legacy-recovery.test.tsx --max-warnings 0) +(cd apps/web && pnpm e2e:run --project chromium tests/session/session-page-recovery.spec.ts -- --retries=0) +(cd apps/web && pnpm e2e:run --project containers tests/ssh/add-workspace-sources.spec.ts -- --retries=0) +(cd apps/web && pnpm exec eslint lib/plan-comment-recovery.ts lib/state/slices/comments/persistence.ts lib/state/slices/session/types.ts lib/state/slices/session/session-slice.ts lib/state/app-state-types.ts hooks/domains/comments/plan-comment-migration.ts hooks/domains/comments/plan-comment-loading.ts hooks/domains/comments/use-plan-comment-migration.ts hooks/domains/comments/use-plan-comments.ts hooks/domains/comments/use-run-comment.ts components/task/plan-comment-migration-notice.tsx components/task/task-plan-panel.tsx components/task/chat/chat-input-area.tsx components/task/passthrough-chat-composer.tsx) +(cd apps/web && pnpm exec eslint lib/plan-comment-recovery.test.ts lib/state/slices/comments/persistence.test.ts lib/state/slices/session/task-plan-comment-actions.test.ts hooks/domains/comments/plan-comment-migration.test.ts hooks/domains/comments/plan-comment-loading.test.ts hooks/domains/comments/use-plan-comment-migration.test.tsx hooks/domains/comments/use-plan-comments.test.tsx hooks/domains/comments/use-run-comment.test.ts hooks/domains/comments/use-run-comment-primary-recovery.test.ts components/task/chat/chat-input-area.test.tsx components/task/passthrough-chat-composer.test.ts components/task/plan-comment-migration-notice.test.tsx components/task/task-plan-panel.session-switch.test.tsx e2e/tests/session/plan-comment-recovery-helpers.ts e2e/tests/session/task-plan-comments.spec.ts e2e/tests/session/mobile-task-plan-comments.spec.ts) +(cd apps/web && pnpm run i18n:check) +(cd apps/web && pnpm run i18n:ratchet) +(cd apps/web && pnpm e2e:run --project chromium tests/session/task-plan-comments.spec.ts -- --retries=0) +(cd apps/web && pnpm e2e:run --project mobile-chrome tests/session/mobile-task-plan-comments.spec.ts -- --retries=0) +python3 scripts/lint-spec-files.test.py +python3 scripts/lint-spec-files.py --all +node --test scripts/validate-public-docs.test.mjs +node scripts/validate-public-docs.mjs +git diff --check +``` + +The two managed browser commands are sequential and rebuild current assets; +do not overlap them or expand to the full suites. Record every command and +actual result in Results. If implementation changes the file decomposition, +update these commands and the owned-file list together before verification. + +## Dependencies + +None. The original task-owned persistence/admission package is already merged. + +## Risks + +Do not mistake unavailable storage for an authoritative empty scan after drafts +are known. Guard plan/task generations and keep one active retry owner across +multiple consumers. Sending before legacy discovery finishes uses only visible +persisted context; retain later-discovered drafts for the next explicit action. +Never apply automatic read/migration retries to message or queue delivery. + +## Parallelism + +`sequential` + +## Inputs + +- [Requirements](../../specs/tasks/requirements/plan-comments.md), especially + requirement 004 and its recovery compatibility note. +- [System design](../../specs/tasks/system-design/plan-comments.md), especially + Legacy migration, Failure and recovery, and Responsive behavior. +- [Ownership/admission ADR](../../decisions/2026-09-02-task-owned-plan-comments.md). +- Existing `useForegroundRefresh`, `use-session-turns-hydration.ts` bounded + recovery, `ws-response-hold.ts` and `ws-drop.ts` correlated frame interception. +- `apps/web/AGENTS.md` plus `/tdd`, `/mobile-parity`, `/e2e`, and + `/docs-maintainer` during implementation. + +## Results + +Implemented the identified-draft recovery boundary. Ordinary plan/comment reads +have their own store-scoped loader; migration retains and retries only identified +legacy records, consumes authoritative create snapshots, and removes only exact +acknowledged storage rows. Unavailable storage and task/plan generation changes +cannot silently clear pending feedback. Selected persisted-comment Run remains +independent of unrelated recovery. The public guide and six locale catalogs +describe automatic retry and retained messages. + +TDD evidence: + +- The initial RED run produced six failures for empty/discovery gating, + automatic migration retry, and persisted-comment Run. +- Additional RED runs covered disconnected foreground reads, automatic read + recovery, refused cleanup, quiet notices, stale plan-generation errors, cached + surface remount, and replacement-plan reads behind an older in-flight request. +- The final command listed above passes all 130 tests across 13 files. Run's + unrelated-recovery regression now lives beside the primary-recovery tests. +- Browser injection targets task/action and, for partial migration, comment UUID. + One row is acknowledged while another fails; a colocated diff row survives. + Both composers' unit paths preserve visible references and reject only actual + pending feedback. Session-discovery failure is injected at the hook boundary. + +Validation: + +- Offline install passed with + `pnpm install --frozen-lockfile --offline --package-import-method=hardlink`. +- Changed-file ESLint: passed with no warnings after helper extraction. +- `pnpm run typecheck`, `pnpm run i18n:check`, and + `pnpm run i18n:ratchet`: passed. Existing catalog orphan reporting is + non-fatal; all required locale keys and placeholders remain valid. +- `python3 scripts/lint-spec-files.test.py`: 36 tests passed. +- `python3 scripts/lint-spec-files.py --all`: passed. +- `node --test scripts/validate-public-docs.test.mjs`: passed. +- `node scripts/validate-public-docs.mjs`: 46 published pages validated. +- `git diff --check`: passed. +- Final managed Chromium command: four scenarios passed, zero retries (1.8m). + Inspected the empty and actionable-recovery screenshots. +- Initial managed Pixel 5 command: four scenarios passed, zero retries (1.0m). + Inspected both screenshots; Retry met 44 px and no overflow was detected. + The final mobile rebuild after the last read-generation correction was + terminated with exit 143 before tests began. The final phone rerun uses + `pnpm e2e:run --no-build --project mobile-chrome tests/session/mobile-task-plan-comments.spec.ts -- --retries=0` + against the fresh final desktop build: four scenarios passed with zero + retries (1.6m). No application source changed after that build, and the + managed runner enforced artifact freshness. + +The first sandboxed browser build could not write the Go cache. The same +isolated managed runner succeeded with escalated build-cache/local-server access. +No user's running instance was mutated. No delegation was used. At the local +implementation handoff, the changes were not yet committed or published. + +### PR review follow-up (2026-09-12) + +Review regressions now cover unknown-plan lookup and exhausted lookup retries, +already-hydrated task sessions when list discovery fails, hidden/visible wake +coalescing in both coordinators, same-plan hydration invalidating a read, and +locale changes with a retained loader. Identified drafts publish their pending +count even while discovery is in flight. Concurrent body edits retain the +acknowledged row/version and use conditional updates; changed anchors, remote +conflicts, and lost responses cannot silently discard local feedback. + +Added the missing waiting-for-plan notice/Retry and attention-projection cases, +strengthened disconnected-read coverage, removed the unused restoration key +from all six catalogs, and generated the Taiwan connection-term correction +through a reviewed key override. Explicit browser setup/recovery timeouts retain +the exact two-successful-uploads assertion, including after recovery settles: +this fixture acknowledges two drafts and must detect redundant acknowledged +uploads. Kept the two small private registries separate: the loader refreshes +localized errors on reuse, while migration retains acknowledgement state. No +shared eviction policy is introduced. + +The focused 13-file command in Verification passes 145 tests. Changed-file +ESLint (zero warnings), web typecheck, i18n checks/ratchet, full specification +lint, and the public-doc validator pass. The locale conversion command was +`pnpm run i18n:zh-hant --namespace task`; unrelated generated terminology was +not retained. Its 24 tests pass with +`pnpm test -- scripts/convert-zh-cn-to-zh-hant.test.mjs`. The first attempt used +the wrong test runner; the Vitest subprocess test then hit sandbox `EPERM` and +passed with subprocess access. Temporary diagnostics were removed. + +The existing public recovery guidance already covers these corrections. Only +the internal design needs the acknowledged-version reconciliation clarification; +no ownership, backend payload, or admission boundary changed. Exact-head remote +review/check evidence remains a separate delivery gate. + +Final local browser verification passed all four Chromium scenarios (1.0m) and +all four Pixel 5 scenarios (48.8s), with zero retries. The fresh desktop command +used `pnpm e2e:run --project chromium tests/session/task-plan-comments.spec.ts -- --retries=0`; +the phone used `pnpm e2e:run --no-build --project mobile-chrome tests/session/mobile-task-plan-comments.spec.ts -- --retries=0` +against those unchanged assets. Both ran with +`GOCACHE=/tmp/pr-3616-go-cache.J336QV` after the shared cache lost a compilation +artifact before tests started. The isolated rebuild succeeded; no cache or +application change was needed outside the task-owned test environment. + +### Claude follow-up clarifications (2026-09-12) + +Documented that attached consumers provide equivalent task-wide discovery, that +the registries retain one entry per visited task for the store's lifetime while +last-detach releases timers/subscriptions, and that storage readback confirms +cleanup. These are comment-only clarifications of the existing implementation; +no new behavior, tests, public guidance, or mobile screenshots are needed. + +Focused validation from `apps/` passed all 55 tests across five files: + +```bash +pnpm --filter @kandev/web test -- \ + lib/state/slices/comments/persistence.test.ts \ + hooks/domains/comments/plan-comment-migration.test.ts \ + hooks/domains/comments/plan-comment-loading.test.ts \ + hooks/domains/comments/use-plan-comment-migration.test.tsx \ + hooks/domains/comments/use-plan-comments.test.tsx +``` + +From `apps/web`, the following ESLint command passed with zero warnings: + +```bash +pnpm exec eslint hooks/domains/comments/plan-comment-loading.ts \ + hooks/domains/comments/plan-comment-migration.ts \ + lib/state/slices/comments/persistence.ts --max-warnings 0 +``` + +### Post-update review remediation (2026-09-13) + +Preserved the user's GitHub merge of the current base and refreshed frozen +workspace dependencies. Six wire-error regressions produced four expected RED +failures before the fix: uppercase transient errors were rejected permanently, +and manual Retry discarded a pending missing-plan refresh. Normalize transient +error comparisons and retain that refresh intent. The backend wire contract, +retry schedule, and draft/admission ownership are unchanged. + +Strengthened two existing regressions without changing their production +contracts. Deferred old and replacement reads prove the shared loader finishes +old cleanup before starting the replacement; both consumers remain loading +until that replacement settles. Conflict coverage proves Retry cannot adopt a +newer server version to overwrite a different body. Recovery acknowledges only +an authoritative exact body/anchor match, preserving both copies otherwise. + +The browser fixture now injects the backend's uppercase `INTERNAL_ERROR` and +requires the final delivered message to contain both plan and diff feedback. +The public task how-to already describes these outcomes; no new public copy, +controls, layout, or mobile interaction pattern is introduced. + +Local verification: + +- The 13-file focused command in Verification passes 151 tests; adding + `scripts/convert-zh-cn-to-zh-hant.test.mjs` passes 175 tests across 14 files. +- `pnpm run typecheck`, `pnpm run i18n:check`, and `pnpm run i18n:ratchet` pass. +- Changed-file ESLint with `--max-warnings 0` passes for the migration source, + its tests, the loading hook tests, and the shared browser recovery helper. +- Harness regression tests (19), all-file harness lint (198 files), targeted + `pre-commit run harness-lint --files` for incoming guidance, specification + regression tests (36), all-file spec lint, and `python3 scripts/list-docs.py + validate` pass. +- Both public-doc validators pass, including all 46 published pages. +- Fresh Chromium verification passes four scenarios (1.1m) with + `pnpm e2e:run --project chromium tests/session/task-plan-comments.spec.ts -- --retries=0`. + Pixel 5 verification passes four scenarios (52.2s) against the same unchanged + build with `pnpm e2e:run --no-build --project mobile-chrome tests/session/mobile-task-plan-comments.spec.ts -- --retries=0`. + Both commands run from `apps/web`, with zero retries. Exact-head remote CI, + reviewer replies, and the requested observation window remain delivery gates, + not completed checks in this tracked document. + +### Follow-up selected cleanup and aggregate suggestions (2026-09-13) + +Codex identified the selected legacy row awaiting browser cleanup. The backend's +durable UUID admission ledger already prevents resurrection: existing SQLite +`TestPlanCommentCreateReplayDoesNotResurrectRetiredComment` passes. The remaining +problem is a stranded local row after Run consumes the acknowledgement source. +Run now queries the existing task recovery owner for only that selected ID, +without creating a new owner or depending on unrelated recovery status. + +The real-store hook regression first failed for both immediate and queued Run +because admission succeeded before cleanup. Both now wait for their own cleanup, +allow one delivery after reload, and retain no draft on a subsequent reload. +A separate case allows a cleaned selected row while another legacy row remains. +The fixture models the actual backend replay ledger, including consumed IDs. + +Claude's two aggregate suggestions are addressed. A deferred upload regression +first observed two calls when a load confirmed the same plan; metadata changes +now wake recovery without invalidating its generation. Both composers' blocked +message is neutral for transient, conflict, and rejected recovery. All six copy +cases failed against the old wording before updating all five locales and the +pseudo catalog. Traditional Chinese is generator-derived with the existing +override preserved; unrelated generated translation changes are excluded. + +CodeRabbit's grouped suggestions and nitpick are addressed: both desktop/phone +previews mark Send blocked, public Run availability includes selected-comment +and primary-session eligibility, post-release recovery polling allows 15 seconds, +and plan replacement/deletion tests preserve a nonzero pending count while +resetting status and failure. + +Local verification: + +- The focused commands in Verification pass 159 unit/component tests across + 14 files. Adding the locale-generator tests passes 183 tests across 15 files. + A final targeted run after test organization and translation cleanup passes + 67 tests across the four directly affected coordinator/composer/Run files. +- Typecheck, changed-file ESLint with zero warnings, i18n checks/ratchet, + specification regression tests (36), all-file spec lint, docs catalog, and + both public-doc validators pass. +- Fresh Chromium passes four scenarios in 1.0m; fresh Pixel 5 passes four in + 48.9s, both with `--retries=0`. They cover existing Send/Run routing, empty-chat + read failures, automatic recovery, draft/focus retention, and delivery of + both plan and diff context. Cleanup denial/reload and metadata ordering are + deterministic real-store regressions with identical mobile/desktop logic; + no new control, layout, gesture, or scroll owner is introduced. +- Exact-head remote CI and bot dispositions remain delivery gates. No success + is inferred from unavailable GitHub evidence. + +### Confirmed absence and CI recovery targeting (2026-09-13) + +Codex's later current-head finding reproduced a stale lookup restoring a plan +after an independent reader confirmed absence. The scope comparison now treats +unknown (`undefined`) and absent (`null`) as different states. Its new deferred +test first restored the old plan incorrectly, then passed with no upload, the +local draft retained, and `waiting_for_plan` status. The same-ID metadata test +still confirms a single upload. + +CI run 34733427521, job 103661040449, failed the SSH workspace-source reconnect +test because `waitForChatIdle` matched both a resuming startup card and the +failed-session banner. The helper and spec match the unchanged authoritative +base df3c9142; no upstream fix was available. The exact spec reproduced the +strict-mode failure locally with retries disabled. A deterministic browser +regression separately reproduced the unscoped three-control match. The page +object now targets the first visible recovery control in the active chat, +including disabled/in-flight controls, rather than another surface's action. +No production SSH behavior, public contract, retry count, or timeout changed. + +Local checks completed before the full-shard validation: + +- 184 focused tests across 15 files (160 unit/component plus 24 locale-generator) + and 62 coordinator/loader/hook tests pass. Typecheck, zero-warning changed-file + ESLint, and i18n ratchet pass. +- Fresh desktop and phone plan-comment scenarios pass with zero retries: + four Chromium cases (1.0m) and four Pixel 5 cases (55.4s). +- The new helper-level browser regression passes (2.6s), and the exact SSH + reconnect test passes (26.7s), both with zero retries. +- Public-doc validation, all-file specification lint, and `git diff --check` + pass. +- All 13 tests in the failed shard pass in 3.8m with `CI=true`, one worker, + the six manifest-selected files in their original order, and `--retries=0`: + `tests/docker/agent-config-copy.spec.ts`, + `tests/docker/lsp-file-intelligence.spec.ts`, + `tests/docker/managed-runtime-npm-recovery.spec.ts`, + `tests/ssh/add-workspace-sources.spec.ts`, `tests/ssh/executor-crud.spec.ts`, + and `tests/ssh/managed-runtime-npm-recovery.spec.ts`, using + `pnpm e2e:run --no-build --project containers` from `apps/web`. + Exact-head remote CI remains a separate gate. +- Existing worktree recovery callers also pass with zero retries: desktop + `session-resume-recovery.spec.ts` together with the new helper regression + (two tests, 16.8s), and `mobile-session-resume-recovery.spec.ts` (one test, + 20.2s). Both use the matching managed-runner project and unchanged fresh build. + +### Plan-local retry budgets and review wording (2026-09-13) + +Claude's replacement-plan finding reproduced accumulated failures from the +previous plan immediately surfacing a Retry notice and extending ordinary-read +backoff. Two fake-timer regressions failed against the prior implementation: +the first upload failure on the new plan published `failed`, and its first +background-read retry did not run after one second. Both coordinators now reset +the failure count on plan identity change. Same-ID metadata confirmation keeps +the existing budget, and migration still enters capped backoff after the new +plan's own three-attempt burst. No layout, touch control, or navigation changes. + +The requested `needsAttention: true` fixtures already exist in +`lib/plan-comment-recovery.test.ts` for `failed` and `waiting_for_plan` with a +pending row; those tests pass unchanged. CodeRabbit's grouped wording findings +are addressed: the no-plan discovery Send regression explicitly has no identified +drafts, and migration completion requires both backend acknowledgement and +confirmed selective browser cleanup. + +Local verification: + +- The focused command in Verification passes 186 tests across 15 files: + 162 unit/component tests plus 24 locale-generator tests. +- `pnpm test -- hooks/domains/comments/plan-comment-migration.test.ts + hooks/domains/comments/plan-comment-loading.test.ts + lib/plan-comment-recovery.test.ts` passes 44 tests. The loader's five tests + pass again after extracting duplicate fixture literals for lint. +- Typecheck, changed-file ESLint with zero warnings, and i18n ratchet pass. +- Fresh Chromium passes four plan-comment scenarios (1.0m); Pixel 5 passes + four (51.4s) using the same build and the existing Verification commands, + both with `--retries=0`. +- Exact-head remote CI, review disposition, and media publication remain + separate delivery gates; these local results do not claim remote completion. diff --git a/docs/plans/task-owned-plan-comments/plan.md b/docs/plans/task-owned-plan-comments/plan.md index ccd60de9c9..994f13db89 100644 --- a/docs/plans/task-owned-plan-comments/plan.md +++ b/docs/plans/task-owned-plan-comments/plan.md @@ -170,6 +170,11 @@ owns dialect parity. ## Verification results +The [Plan comment recovery follow-up](../plan-comment-recovery/plan.md) refines +the migration and read-failure behavior from Tasks 04 and 05. Its completed work +order records the correction and current verification; the checkboxes and +results here describe the original delivery. + The original implementation checks below are historical. Current rebase and review-remediation evidence is recorded separately; these results do not certify the latest PR head. diff --git a/docs/plans/task-owned-plan-comments/task-04-migrate-legacy-browser-drafts.md b/docs/plans/task-owned-plan-comments/task-04-migrate-legacy-browser-drafts.md index 2ee6970450..9ae1d192cf 100644 --- a/docs/plans/task-owned-plan-comments/task-04-migrate-legacy-browser-drafts.md +++ b/docs/plans/task-owned-plan-comments/task-04-migrate-legacy-browser-drafts.md @@ -91,6 +91,11 @@ Task 03. ## Results +Historical implementation results follow. The +[Plan comment recovery work order](../plan-comment-recovery/task-01-recover-plan-comment-context.md) +records the implemented quiet retries and narrowed migration gate; the results +below predate that correction. + - Added per-task migration that scans all known session payloads and uploads legacy plan comments with their original UUIDs. - Cleanup rereads storage and removes only acknowledged plan IDs, preserving diff --git a/docs/plans/task-owned-plan-comments/task-05-prove-responsive-multi-session-behavior.md b/docs/plans/task-owned-plan-comments/task-05-prove-responsive-multi-session-behavior.md index fbfe5aa55d..b051585419 100644 --- a/docs/plans/task-owned-plan-comments/task-05-prove-responsive-multi-session-behavior.md +++ b/docs/plans/task-owned-plan-comments/task-05-prove-responsive-multi-session-behavior.md @@ -102,6 +102,11 @@ Tasks 03 and 04. ## Results +The [Plan comment recovery package](../plan-comment-recovery/plan.md#e2e-tests) +adds empty-task failure and automatic recovery scenarios to these same desktop +and phone suites. Their passing results are recorded in that package; the +results below certify the original routing and migration scenarios only. + - Added focused desktop and Pixel 5 Playwright scenarios using real backend transcripts to distinguish selected-session Send from primary-session Run. - Desktop proves shared comments across tabs and reload plus task-wide diff --git a/docs/public/tasks-and-workflows.md b/docs/public/tasks-and-workflows.md index a33083e0fa..b3651e4f86 100644 --- a/docs/public/tasks-and-workflows.md +++ b/docs/public/tasks-and-workflows.md @@ -680,6 +680,15 @@ also fit within 1 MiB. If a limit is exceeded, shorten the feedback, selection, or message and retry; rejected changes and deliveries do not remove pending comments. +Temporary connection issues are retried automatically in the background. If +saved feedback still needs attention, an inline notice offers **Retry**. Your +message stays in the composer while that feedback is being restored; recovery +never sends it for you. If no saved feedback needs recovery, you can keep +sending messages normally. **Run** remains available when the selected comment +and primary session are eligible, even if other feedback is still being +restored. A recovered comment must finish its own browser-draft cleanup before +it can be run. + Agents use `create_task_plan_kandev`, `get_task_plan_kandev`, `update_task_plan_kandev`, and `delete_task_plan_kandev`. Human edits are therefore visible to the next agent that reads the plan. A plan records intent; verify that code and review still match it. Revision history is not an immutable record of every autosave. Consecutive writes from the same author name and author kind coalesce into the latest revision for five minutes by default. Operators can set `KANDEV_PLAN_COALESCE_WINDOW_MS`; `0` disables coalescing, while an invalid or negative value falls back to five minutes. diff --git a/docs/specs/tasks/requirements/plan-comments.md b/docs/specs/tasks/requirements/plan-comments.md index 9cc39cdfb2..8259e5db11 100644 --- a/docs/specs/tasks/requirements/plan-comments.md +++ b/docs/specs/tasks/requirements/plan-comments.md @@ -2,6 +2,7 @@ status: active system: tasks created: 2026-09-02 +updated: 2026-09-11 owners: - kandev --- @@ -145,10 +146,11 @@ must move to task ownership without another comment-loss window. #### Acceptance criteria -- **AC-TASKS-PLAN-COMMENTS-004.1:** When a browser contains session-scoped - pending plan comments for sessions belonging to the open task, Kandev shall - migrate those comments to the task's current plan before enabling a delivery - that could omit them. +- **AC-TASKS-PLAN-COMMENTS-004.1:** When Kandev identifies session-scoped + pending plan comments belonging to the open task, it shall migrate those + comments to the task's current plan before accepting a composer Send that + would omit them. Discovering or recovering comments shall not itself deliver + a prompt or consume pending feedback. - **AC-TASKS-PLAN-COMMENTS-004.2:** Migration shall retain each comment's client-generated identifier. Retrying the same identifier and content shall be idempotent; distinct identifiers shall remain distinct even when their @@ -157,9 +159,56 @@ must move to task ownership without another comment-loss window. record from browser storage only after backend persistence is acknowledged. Failed plan comments and every non-plan comment in the same session record shall remain unchanged. -- **AC-TASKS-PLAN-COMMENTS-004.4:** A migration failure shall be visible and - retryable. It shall not silently discard a draft or send a message without - the legacy plan comments that the composer represented. +- **AC-TASKS-PLAN-COMMENTS-004.4:** A recovery failure shall show comment-specific + feedback only when at least one identified legacy comment for the open task + still needs recovery and either automatic retries have been exhausted or + user action is required. The feedback shall offer retry where retry can help. + Background discovery and transient recovery attempts shall not display a + restoration warning. Failed recovery shall preserve both the saved comments + and any attempted composer message. +- **AC-TASKS-PLAN-COMMENTS-004.5:** When no unresolved legacy comments have been + identified for the open task and a composer represents no pending plan + comments, ordinary Send shall remain available despite pending or failed + comment discovery, plan lookup, or comment refresh. This applies to tasks + with a plan, without a plan, and before the presence of a plan is known. + Comments belonging to another task shall not block that Send or produce a + restoration warning on the open task. Ordinary connection and session-input + constraints still apply. +- **AC-TASKS-PLAN-COMMENTS-004.6:** Transient discovery and recovery failures + shall retry automatically with bounded request frequency while the task is + open and connectivity permits progress. Returning to the foreground or + reconnecting shall resume recovery without requiring a reload or Retry + action. Success shall clear obsolete recovery feedback and restrictions; + recovery shall never automatically submit a previously blocked message. +- **AC-TASKS-PLAN-COMMENTS-004.7:** Ordinary Send shall remain blocked while at + least one identified legacy comment for its task would be omitted, including + a mixed set of recovered and unrecovered comments. Run of an already + persisted comment shall depend only on that selected comment and its eligible + primary destination; unrelated unrecovered comments shall remain pending. + A background read failure shall not remove visible persisted comments from + a submitted snapshot or bypass their acceptance checks. +- **AC-TASKS-PLAN-COMMENTS-004.8:** Desktop, phone, structured chat, and + passthrough composers shall apply the same recovery and delivery rules. + Recovery feedback requiring action shall remain inline with the affected + comment context, preserve the composer draft and focus, and expose a + touch-accessible action on phones. Task or plan changes shall not apply a + previous context's failure, cleanup, or retry to the new context. + +## Recovery compatibility + +Discovery is background work, not an implicit comment selection. Before any +legacy feedback is identified for the task, Send uses the visible persisted +snapshot under `REQ-TASKS-PLAN-COMMENTS-002`. Discovery that finishes later +preserves and exposes the recovered feedback for a subsequent explicit action; +it does not amend or resend an earlier prompt. This narrows the former blanket +migration gate without changing task ownership or backend acceptance semantics. + +## Implementation plans + +- [Task-owned plan comments](../../../plans/task-owned-plan-comments/plan.md) + records the original delivery. +- [Plan comment recovery](../../../plans/plan-comment-recovery/plan.md) + implements the recovery and delivery refinements in requirement 004. ## Out of scope diff --git a/docs/specs/tasks/system-design/plan-comments.md b/docs/specs/tasks/system-design/plan-comments.md index 52503f6532..5368c7858b 100644 --- a/docs/specs/tasks/system-design/plan-comments.md +++ b/docs/specs/tasks/system-design/plan-comments.md @@ -23,6 +23,11 @@ the superseded [Plan Comment Drafts design](../../ui/system-design/plan-comment- Other comment sources remain session-scoped and keep their existing browser persistence and client-side formatting. +The recovery refinements are implemented by the +[Plan comment recovery package](../../../plans/plan-comment-recovery/plan.md). +The existing persistence, task ownership, and atomic admission boundaries remain +the basis for implementation. + ## Requirement mapping | Requirement | Design sections | @@ -30,7 +35,7 @@ persistence and client-side formatting. | `REQ-TASKS-PLAN-COMMENTS-001` | [Persistence model](#persistence-model), [Frontend projection](#frontend-projection), [Plan lifecycle](#plan-lifecycle) | | `REQ-TASKS-PLAN-COMMENTS-002` | [Composer delivery](#composer-delivery), [Atomic acceptance](#atomic-acceptance), [Failure and recovery](#failure-and-recovery) | | `REQ-TASKS-PLAN-COMMENTS-003` | [Run routing](#run-routing), [Atomic acceptance](#atomic-acceptance) | -| `REQ-TASKS-PLAN-COMMENTS-004` | [Legacy migration](#legacy-migration) | +| `REQ-TASKS-PLAN-COMMENTS-004` | [Legacy migration](#legacy-migration), [Failure and recovery](#failure-and-recovery), [Responsive and accessibility behavior](#responsive-and-accessibility-behavior) | ## Ownership decision @@ -324,28 +329,141 @@ new plan ID and an empty comment collection. ## Legacy migration -The first frontend version with backend comments runs a bounded migration for -the open task after it knows the current plan and the task's session IDs: +### Separate discovery, migration, and snapshot loading -1. Inspect `kandev.comments.` for every known session in the task. -2. Extract only records whose source is `plan`; retain their existing UUID, - text, selected text, and anchor positions. -3. Create each backend row against the current plan. Exact UUID replays are - successful, so interrupted migration can restart safely. -4. After each acknowledgement, remove only that plan record from its legacy - session payload. Preserve all other comment sources and every failed plan - record. -5. Fetch the authoritative snapshot and mark migration complete for the task. +`usePlanCommentMigration` owns only promotion of legacy browser drafts. +`usePlanComments` continues to own current-plan and authoritative comment reads. +The latter's `commentsErrorByTaskId` is not evidence that a legacy draft exists +and must not be converted into migration failure. -Composer Send and plan-comment Run wait for this migration gate. Comment CRUD -and other local comment types are not blocked. If no current plan exists, the -legacy records remain in storage for recovery rather than being discarded. +Discovery reads `kandev.comments.` only for sessions attributed to the +task by `useTaskSessions` or current task-session state. It can inspect known +session records while the authoritative session list is pending, and rescans +when membership becomes available or the browser returns to the foreground. +Failed or incomplete discovery is not a completed scan, but has no delivery +restriction in the absence of identified task-owned draft content. Do not +attribute an unrelated session's stored rows to the open task. No server request +is required by migration when its task scan contains zero legacy records. + +Once identified, retain each pending record in the recovery state until exact +backend acknowledgement and selective storage cleanup. A later failed or empty +storage read must not erase that known pending set. Keep other comment sources, +malformed-but-readable payload entries, original UUIDs, and concurrent local +edits intact. `listLegacyPlanComments` and +`removeAcknowledgedLegacyPlanComment` remain the persistence boundaries; any +additional read outcome must distinguish unavailable storage from an +authoritative empty result without changing unrelated storage helpers. + +For a task with identified drafts: + +1. Resolve its current plan while connected. If no plan exists, retain the + drafts and wait for plan availability; do not repeatedly upload or delete. +2. Upload each draft with its existing UUID and content. Each create response + already supplies an authoritative snapshot; reconcile it by task, plan ID, + and revision before acknowledging the exact stored record. +3. Re-read after acknowledgement and retain failed or concurrently edited rows. + Keep the acknowledged row/version when its legacy body changes, and reconcile + that body with the existing version-checked update API, never another create + for the same UUID. Changed anchors or conflicting server edits remain pending; + do not overwrite them. A lost update response can reconcile only when the + authoritative row matches the intended body and anchor. A conflict snapshot + alone does not acknowledge a different local body. +4. Complete migration only after every identified row has backend + acknowledgement and confirmed selective browser-storage cleanup. Do not + append an unconditional list request: a failed background snapshot refresh + must not undo successful migration or keep an empty migration blocked. + +### Recovery state and lifetime + +Replace the status-only `commentsMigrationStatusByTaskId` projection with one +task-keyed recovery record in the existing task-plan slice. The proposed +`PlanCommentMigrationState` contains status, identified pending-record count, +and actionable failure classification; the +`commentsMigrationByTaskId` map and `setTaskPlanCommentMigrationState` action +update that projection together. The old map and setter are removed, not kept +as a second source of truth. Phases distinguish discovery, active migration, +retry waiting, missing plan, actionable failure, and completion. + +Promise, timer, pending-record, and subscriber bookkeeping stays in one +store-scoped coordinator keyed by task with a plan-identity generation. React +consumers share it, including simultaneous +Plan, structured-composer, and passthrough mounts. At most one operation and one +retry timer per task are active. StrictMode and repeated focus events must not +create duplicate uploads or reset the retry budget on every render. + +Use connected, visible consumers to admit network attempts. Replace the +uncoalesced focus/visibility listeners in `usePlanComments` with +`useForegroundRefresh`; defer work while disconnected and resume on the next +connected transition. Discovery and legacy recovery use the same foreground +trigger and connection check. `plan-comment-loading.ts` owns the ordinary +read promise, bounded retry timer, and plan generation behind `usePlanComments`; +`plan-comment-migration.ts` owns identified drafts and acknowledgements behind +`usePlanCommentMigration`. Both layers preserve successful snapshots through +read failures; no application-wide transport timeout or retry policy changes. + +Transient failures get three connected attempts with delays of one and two +seconds after the first two failures. After that burst, schedule single +background attempts at 30, 60, then at most once per 120 seconds while visible +and connected. These are implementation constants, not operator settings. +Coalesced reconnect, foreground, and explicit Retry triggers can bring the next +attempt forward, but share the in-flight operation. Explicit Retry, successful +recovery, or a change of plan identity resets the failure budget. A replacement +plan gets its own quiet retry burst; confirming loaded metadata for the same +plan does not reset its accumulated failures. Disconnected or hidden periods +do not spend attempts. Stop scheduled work when the last consumer unmounts; +remount rescans and resumes without a browser-global completed marker. + +Typed content, limit, authorization, and UUID-conflict rejections are actionable +and do not enter an automatic mutation loop. Resolve a plan-not-found response +through current-plan lookup. Untyped transport failures and server read failures +use bounded retry. Manual Retry rechecks prerequisites as well as remaining +records, and cannot clear identified draft evidence just to release Send. + +Each operation captures task/plan identity and a generation. Recheck before +each upload, state write, and acknowledgement. Task or plan invalidation cancels +future work and rejects late completion from the previous generation. A +transition between unknown and confirmed-absent plan state also invalidates +lookups; confirming loaded metadata for the same existing plan only wakes work. +A successful current-generation upload removes only its exact legacy row; it +does not delete an entire storage key containing other records. + +### Delivery restriction + +Use one shared selector for composer migration blocking: at least one +identified legacy record for that task remains unresolved. Neither an idle +phase nor a general read error satisfies that predicate. The structured +`useSubmitHandler` and passthrough submission guard both use it. A blocked +attempt preserves the typed message and returns localized pending-context +feedback; recovery never resubmits it. + +Persisted snapshot comments remain visible during failed background reads. +Normal Send continues to freeze their IDs and versions with +`toTaskPlanCommentRefs`, so backend exact-version admission protects their +content. A successful refresh, including an empty snapshot or confirmed absent +plan, clears obsolete read errors but never acknowledges unresolved local rows. + +Run selects one persisted comment. Remove the unrelated task-wide migration +status check from `runTaskPlanComment` and the selection toolbar; keep persisted +version, primary availability, exact reference, and server acceptance checks. +The recovery owner must also report no identified pending legacy record for +that selected ID, including an acknowledged upload awaiting browser cleanup. +Consuming its server row first would leave the retained local row without a +matching acknowledgement on reload; durable UUID admission prevents recreation +but does not finish browser cleanup. Unrelated records do not participate in +this selected-ID check and remain pending. Add-and-Run must still await persistence +of the selected new comment before invoking Run. ## Failure and recovery -- Failed load or migration shows a retryable comment-context error. Kandev does - not allow a comment-bearing delivery until it can establish the task - snapshot, preventing silent omission. +- Discovery and transient background recovery are quiet. A comment restoration + notice is eligible only for identified unresolved drafts after the initial + retry burst or for an actionable failure, including an absent current plan. + General read failures never produce that notice. A task with no identified + drafts has no migration restriction, independently of plan availability. +- Known pending feedback remains protected as described in + [Delivery restriction](#delivery-restriction). A partial upload cannot release + a Send that would omit a remaining row; an unrelated pending row cannot block + Run of an already persisted comment. - Failed create or edit keeps the entered body and selection in the open editor. Failed delete keeps the annotation visible. Desktop outside-click or Escape dismissal and mobile Drawer dismissal are ignored while any mutation @@ -387,6 +505,18 @@ Run control exposes why it is unavailable when no eligible primary exists. Session switching must not dismiss or mutate a persisted comment merely as a side effect of changing responsive navigation. +`PlanCommentMigrationNotice` remains an inline context notice in the existing +Plan surface and both composers, but renders only actionable recovery state. +No progress banner is mounted for discovery or automatic retry. Retain draft +focus when it appears or disappears. The existing task mobile composition in +`task-layout.tsx` and the Plan Drawer are the shipped exemplars: chat remains +the primary phone destination and its scroll/safe-area ownership is unchanged. +Do not add a recovery modal or a second scroll region. Retry keeps a 28 px +desktop control and a minimum 44 px coarse-pointer target. Shared logic owns +eligibility; responsive wrappers own wrapping and containment. Pending-Send +copy describes preserved feedback and automatic recovery through translations, +without instructing users to perform a migration. + ## Verification - Repository tests cover schema replay on SQLite and Postgres, task/plan @@ -400,12 +530,21 @@ side effect of changing responsive navigation. session changes do not filter it, async editors retain failed input, Run chooses the primary, and legacy migration removes only acknowledged plan records. +- Recovery tests cover empty and unknown plan state, initial discovery failure, + automatic reconnect and foreground recovery, exhausted retries, mixed + acknowledged/unacknowledged rows, unmount and plan-identity races, unrelated + task records, and persisted-comment Run during another draft's recovery. - Desktop Playwright coverage distinguishes selected-session Send from primary-session Run in a two-session task and proves task-wide removal after acceptance. - Mobile Playwright coverage proves the same shared context and routing through the session picker and Plan Drawer, with no horizontal overflow or undersized actions. +- Desktop and phone recovery scenarios inject correlated comment/read failures + while leaving unrelated chat frames live, send plain text without Retry on + empty tasks, and prove actual legacy feedback recovers without a manual + action. Existing selected-session Send and primary-session Run scenarios + remain part of those focused suites. ## Observability @@ -423,3 +562,8 @@ correctness boundary. - [Keep Queue Auto-run Server Owned](../../../decisions/2026-08-16-server-owned-queue-auto-run.md) - [Separate Message Queue Provenance, Cancellation, and Capacity](../../../decisions/2026-08-03-separate-message-queue-provenance-cancellation-and-capacity.md) - [Keep Saved-Prompt Expansion Server-Owned](../../../decisions/2026-09-01-server-owned-saved-prompt-expansion.md) + +## Implementation plans + +- [Task-owned plan comments](../../../plans/task-owned-plan-comments/plan.md) +- [Plan comment recovery](../../../plans/plan-comment-recovery/plan.md)