From 65e6759d7d86e83861fcd8a17235a0d3d70c76ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Almeida?= <57680069+zeval@users.noreply.github.com> Date: Wed, 2 Sep 2026 12:31:35 +0000 Subject: [PATCH] fix(web): preserve plan comments across session switches (#3272) --- .../editors/tiptap/comment-mark.tsx | 30 ++-- .../tiptap/tiptap-plan-editor.test.tsx | 96 +++++++++++ .../task-plan-panel.session-switch.test.tsx | 46 +++++ apps/web/components/task/task-plan-panel.tsx | 33 +--- .../web/components/task/use-plan-selection.ts | 49 ++++++ .../tests/session/multi-session-ux.spec.ts | 90 ++++++++++ .../plan.md | 123 +++++++++++++ .../task-01-preserve-plan-comments.md | 121 +++++++++++++ docs/specs/ui/README.md | 10 +- .../ui/requirements/plan-comment-drafts.md | 72 ++++++++ .../ui/system-design/plan-comment-drafts.md | 161 ++++++++++++++++++ 11 files changed, 784 insertions(+), 47 deletions(-) create mode 100644 apps/web/components/task/task-plan-panel.session-switch.test.tsx create mode 100644 apps/web/components/task/use-plan-selection.ts create mode 100644 docs/plans/plan-comment-session-switch-preservation/plan.md create mode 100644 docs/plans/plan-comment-session-switch-preservation/task-01-preserve-plan-comments.md create mode 100644 docs/specs/ui/requirements/plan-comment-drafts.md create mode 100644 docs/specs/ui/system-design/plan-comment-drafts.md diff --git a/apps/web/components/editors/tiptap/comment-mark.tsx b/apps/web/components/editors/tiptap/comment-mark.tsx index 68f9b6e99f..87658867c7 100644 --- a/apps/web/components/editors/tiptap/comment-mark.tsx +++ b/apps/web/components/editors/tiptap/comment-mark.tsx @@ -230,6 +230,7 @@ function buildBadgeDecorations(doc: DocLike, markTypeName: string): DecorationSe const orphanPluginKey = new PluginKey("commentMark-orphanDetection"); const badgePluginKey = new PluginKey("commentMark-badges"); +const projectionReconciliationKey = new PluginKey("commentMark-projectionReconciliation"); // --------------------------------------------------------------------------- // Helper: find commentId at a document position @@ -361,20 +362,26 @@ export const CommentMark = Mark.create({ // Orphan detection — fire callback when a commentId disappears from the doc new Plugin({ key: orphanPluginKey, - appendTransaction(transactions, oldState, newState) { - const docChanged = transactions.some((tr) => tr.docChanged); - if (!docChanged) return null; + appendTransaction(transactions, _, newState) { + const finalIds = collectCommentIds(newState.doc, markName); + const orphaned = new Set(); - const oldIds = collectCommentIds(oldState.doc, markName); - const newIds = collectCommentIds(newState.doc, markName); - - const orphaned: string[] = []; - for (const id of oldIds) { - if (!newIds.has(id)) orphaned.push(id); + for (const transaction of transactions) { + if ( + !transaction.docChanged || + transaction.getMeta(projectionReconciliationKey) === true + ) { + continue; + } + const beforeIds = collectCommentIds(transaction.before, markName); + const afterIds = collectCommentIds(transaction.doc, markName); + for (const id of beforeIds) { + if (!afterIds.has(id) && !finalIds.has(id)) orphaned.add(id); + } } - if (orphaned.length > 0) { - setTimeout(() => onOrphanedComments(orphaned), 0); + if (orphaned.size > 0) { + setTimeout(() => onOrphanedComments([...orphaned]), 0); } return null; @@ -468,6 +475,7 @@ export function rehydrateCommentMarks( } if (changed) { + tr.setMeta(projectionReconciliationKey, true); editor.view.dispatch(tr); } }, 0); diff --git a/apps/web/components/editors/tiptap/tiptap-plan-editor.test.tsx b/apps/web/components/editors/tiptap/tiptap-plan-editor.test.tsx index 960eca0384..794e6baa47 100644 --- a/apps/web/components/editors/tiptap/tiptap-plan-editor.test.tsx +++ b/apps/web/components/editors/tiptap/tiptap-plan-editor.test.tsx @@ -78,6 +78,12 @@ async function renderReadyEditor(onChange: (value: string) => void): Promise { + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)); + }); +} + function getFirstTableHeader(editor: Editor) { let headerPosition: number | null = null; editor.state.doc.descendants((node, position) => { @@ -229,3 +235,93 @@ describe("TipTapPlanEditor mobile formatting clearance", () => { ); }); }); + +describe("TipTapPlanEditor comment projection", () => { + const comment = { + id: "primary-comment", + selectedText: "Primary", + from: 1, + to: 8, + }; + + // @covers AC-UI-PLAN-COMMENT-DRAFTS-001.2 + it("does not report a projected comment as deleted when the projection changes", async () => { + const onCommentDeleted = vi.fn(); + const view = render( + undefined} + comments={[comment]} + onCommentDeleted={onCommentDeleted} + />, + ); + + await waitFor(() => { + expect( + view.container.querySelector('.comment-badge[data-comment-id="primary-comment"]'), + ).not.toBeNull(); + }); + + view.rerender( + undefined} + comments={[]} + onCommentDeleted={onCommentDeleted} + />, + ); + + await waitFor(() => { + expect( + view.container.querySelector('.comment-badge[data-comment-id="primary-comment"]'), + ).toBeNull(); + }); + await flushDeferredCommentCallbacks(); + + expect(onCommentDeleted).not.toHaveBeenCalled(); + }); + + // @covers AC-UI-PLAN-COMMENT-DRAFTS-001.4 + it("reports an untagged comment mark removal exactly once", async () => { + const onCommentDeleted = vi.fn(); + const readyEditors: Editor[] = []; + const view = render( + undefined} + comments={[comment]} + onCommentDeleted={onCommentDeleted} + onEditorReady={(readyEditor) => { + readyEditors.push(readyEditor); + }} + />, + ); + + await waitFor(() => { + expect( + view.container.querySelector('.comment-badge[data-comment-id="primary-comment"]'), + ).not.toBeNull(); + }); + const readyEditor = readyEditors[0]; + if (!readyEditor) throw new Error("editor did not become ready"); + const markType = readyEditor.state.schema.marks.commentMark; + + act(() => { + readyEditor.view.dispatch( + readyEditor.state.tr.removeMark( + 0, + readyEditor.state.doc.content.size, + markType.create({ commentId: comment.id }), + ), + ); + }); + + await waitFor(() => { + expect(onCommentDeleted).toHaveBeenCalledWith([comment.id]); + }); + expect(onCommentDeleted).toHaveBeenCalledTimes(1); + }); +}); diff --git a/apps/web/components/task/task-plan-panel.session-switch.test.tsx b/apps/web/components/task/task-plan-panel.session-switch.test.tsx new file mode 100644 index 0000000000..fc0c0861b1 --- /dev/null +++ b/apps/web/components/task/task-plan-panel.session-switch.test.tsx @@ -0,0 +1,46 @@ +import { act, renderHook, waitFor } from "@testing-library/react"; +import type { PlanComment } from "@/lib/state/slices/comments"; +import { describe, expect, it, vi } from "vitest"; +import { usePlanSelection } from "./use-plan-selection"; + +const PRIMARY_SESSION_ID = "session-primary"; +const SECONDARY_SESSION_ID = "session-secondary"; + +const primaryComment: PlanComment = { + id: "primary-comment", + sessionId: PRIMARY_SESSION_ID, + source: "plan", + text: "Keep this feedback", + selectedText: "Primary", + from: 1, + to: 8, + createdAt: "2026-09-02T00:00:00.000Z", + status: "pending", +}; + +describe("plan selection session ownership", () => { + // @covers AC-UI-PLAN-COMMENT-DRAFTS-001.5 + it("closes comment editing state when the active session changes", async () => { + const setEditingCommentId = vi.fn(); + const view = renderHook( + ({ sessionId, comments }) => usePlanSelection(sessionId, { comments, setEditingCommentId }), + { + initialProps: { + sessionId: PRIMARY_SESSION_ID, + comments: [primaryComment], + }, + }, + ); + + act(() => { + view.result.current.handleCommentHighlightClick(primaryComment.id, { x: 20, y: 20 }); + }); + expect(view.result.current.textSelection?.text).toBe(primaryComment.selectedText); + setEditingCommentId.mockClear(); + + view.rerender({ sessionId: SECONDARY_SESSION_ID, comments: [] }); + + await waitFor(() => expect(view.result.current.textSelection).toBeNull()); + expect(setEditingCommentId).toHaveBeenLastCalledWith(null); + }); +}); diff --git a/apps/web/components/task/task-plan-panel.tsx b/apps/web/components/task/task-plan-panel.tsx index 2632637b9b..aff40fa9c0 100644 --- a/apps/web/components/task/task-plan-panel.tsx +++ b/apps/web/components/task/task-plan-panel.tsx @@ -19,6 +19,7 @@ import type { import type { Editor } from "@tiptap/core"; import { PanelSearchBar } from "@/components/search/panel-search-bar"; import { usePlanFindShortcut } from "./use-plan-find-shortcut"; +import { usePlanSelection } from "./use-plan-selection"; import { Trans, useTranslation } from "react-i18next"; import { t } from "@/lib/i18n"; @@ -459,38 +460,6 @@ function usePlanDraft( }; } -/** Text selection state for comment popover */ -function usePlanSelection( - activeSessionId: string | null | undefined, - commentState: ReturnType, -) { - const [textSelection, setTextSelection] = useState(null); - - const handleCommentHighlightClick = useCallback( - (id: string, position: { x: number; y: number }) => { - const comment = commentState.comments.find((c) => c.id === id); - if (comment) { - commentState.setEditingCommentId(id); - setTextSelection({ - text: comment.selectedText, - from: comment.from, - to: comment.to, - position, - }); - } - }, - [commentState], - ); - - const handleSelectionClose = useCallback(() => { - setTextSelection(null); - commentState.setEditingCommentId(null); - window.getSelection()?.removeAllRanges(); - }, [commentState]); - - return { textSelection, setTextSelection, handleCommentHighlightClick, handleSelectionClose }; -} - /** Ctrl+S save shortcut */ function useSaveShortcut( hasUnsavedChanges: boolean, diff --git a/apps/web/components/task/use-plan-selection.ts b/apps/web/components/task/use-plan-selection.ts new file mode 100644 index 0000000000..7f4b2e755a --- /dev/null +++ b/apps/web/components/task/use-plan-selection.ts @@ -0,0 +1,49 @@ +import { useCallback, useEffect, useRef, useState } from "react"; +import type { TextSelection } from "@/components/editors/tiptap/tiptap-plan-editor"; +import type { usePlanComments } from "@/hooks/domains/comments/use-plan-comments"; + +type PlanSelectionCommentState = Pick< + ReturnType, + "comments" | "setEditingCommentId" +>; + +/** Own the transient plan-comment selection for the currently active session. */ +export function usePlanSelection( + activeSessionId: string | null | undefined, + commentState: PlanSelectionCommentState, +) { + const [textSelection, setTextSelection] = useState(null); + const previousSessionIdRef = useRef(activeSessionId); + + useEffect(() => { + if (previousSessionIdRef.current === activeSessionId) return; + previousSessionIdRef.current = activeSessionId; + // eslint-disable-next-line react-hooks/set-state-in-effect -- session identity owns this transient editor state + setTextSelection(null); + commentState.setEditingCommentId(null); + window.getSelection()?.removeAllRanges(); + }, [activeSessionId, commentState.setEditingCommentId]); + + const handleCommentHighlightClick = useCallback( + (id: string, position: { x: number; y: number }) => { + const comment = commentState.comments.find((candidate) => candidate.id === id); + if (!comment) return; + commentState.setEditingCommentId(id); + setTextSelection({ + text: comment.selectedText, + from: comment.from, + to: comment.to, + position, + }); + }, + [commentState], + ); + + const handleSelectionClose = useCallback(() => { + setTextSelection(null); + commentState.setEditingCommentId(null); + window.getSelection()?.removeAllRanges(); + }, [commentState]); + + return { textSelection, setTextSelection, handleCommentHighlightClick, handleSelectionClose }; +} diff --git a/apps/web/e2e/tests/session/multi-session-ux.spec.ts b/apps/web/e2e/tests/session/multi-session-ux.spec.ts index 3ce58fba98..87819d23ef 100644 --- a/apps/web/e2e/tests/session/multi-session-ux.spec.ts +++ b/apps/web/e2e/tests/session/multi-session-ux.spec.ts @@ -5,6 +5,7 @@ import { join } from "node:path"; import { test, expect } from "../../fixtures/test-base"; import { KanbanPage } from "../../pages/kanban-page"; import { SessionPage } from "../../pages/session-page"; +import { waitForStableActiveSession } from "../../helpers/session-store"; const fsExists = (p: string) => existsSync(p); @@ -142,6 +143,95 @@ test.describe("Multi-session UX", () => { }); }); + // @covers AC-UI-PLAN-COMMENT-DRAFTS-001.2 + // @covers AC-UI-PLAN-COMMENT-DRAFTS-001.3 + test("preserves pending plan comments across session switches", async ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(120_000); + + const { task, session } = await createTaskAndNavigate( + testPage, + apiClient, + seedData, + "Plan comment session task", + ); + const { sessions: primarySessions } = await apiClient.listTaskSessions(task.id); + const primarySessionId = primarySessions[0]?.id; + if (!primarySessionId) throw new Error("expected a primary session"); + await apiClient.wsRequest("task.plan.create", { + task_id: task.id, + title: "Plan", + content: "## Plan\n\nKeep this primary-session step", + created_by: "user", + }); + + await session.openNewSessionDialog(); + await session.newSessionPromptInput().fill("/e2e:simple-message"); + await session.newSessionStartButton().click(); + await expect(session.newSessionDialog()).not.toBeVisible({ timeout: 10_000 }); + await expect + .poll( + async () => { + const { sessions } = await apiClient.listTaskSessions(task.id); + return sessions.filter((item) => DONE_STATES.includes(item.state)).length; + }, + { timeout: 60_000, message: "Waiting for the secondary session" }, + ) + .toBe(2); + const { sessions } = await apiClient.listTaskSessions(task.id); + const secondarySessionId = sessions.find((item) => item.id !== primarySessionId)?.id; + if (!secondarySessionId) throw new Error("expected a secondary session"); + + const primaryTab = session.sessionTabBySessionId(primarySessionId); + const secondaryTab = session.sessionTabBySessionId(secondarySessionId); + await expect(primaryTab).toBeVisible({ timeout: 10_000 }); + await expect(secondaryTab).toBeVisible({ timeout: 10_000 }); + await primaryTab.click(); + await waitForStableActiveSession(testPage, primarySessionId); + + const planToggle = session.activeChat().getByTestId("plan-mode-toggle-button"); + await expect(planToggle).toBeVisible({ timeout: 10_000 }); + await expect(planToggle).toHaveAttribute("data-plan-available", "true", { timeout: 10_000 }); + await planToggle.click(); + await expect(session.planPanel.getByText("Keep this primary-session step")).toBeVisible({ + timeout: 10_000, + }); + + const editor = session.planPanel.locator(".ProseMirror"); + await editor.click(); + const modifier = process.platform === "darwin" ? "Meta" : "Control"; + await testPage.keyboard.press(`${modifier}+a`); + await testPage.keyboard.press(`${modifier}+Shift+c`); + + const commentText = "Preserve this exact feedback"; + const textarea = testPage.locator('textarea[placeholder="Add your comment or instruction..."]'); + await expect(textarea).toBeVisible({ timeout: 5_000 }); + await textarea.fill(commentText); + await testPage.getByRole("button", { name: "Add", exact: true }).click(); + + const badge = session.planPanel.locator(".comment-badge[data-comment-id]"); + await expect(badge).toHaveCount(1); + + await secondaryTab.click(); + await waitForStableActiveSession(testPage, secondarySessionId); + await expect(badge).toHaveCount(0); + await testPage.evaluate( + () => + new Promise((resolve) => { + requestAnimationFrame(() => requestAnimationFrame(() => resolve())); + }), + ); + + await primaryTab.click(); + await waitForStableActiveSession(testPage, primarySessionId); + await expect(badge).toHaveCount(1, { timeout: 10_000 }); + await badge.locator("svg").click(); + await expect(textarea).toHaveValue(commentText); + }); + // @covers AC-UI-TASK-AGENT-TAB-RECONCILIATION-001.6 test("reload restores the selected Agent tab", async ({ testPage, diff --git a/docs/plans/plan-comment-session-switch-preservation/plan.md b/docs/plans/plan-comment-session-switch-preservation/plan.md new file mode 100644 index 0000000000..590a241c50 --- /dev/null +++ b/docs/plans/plan-comment-session-switch-preservation/plan.md @@ -0,0 +1,123 @@ +--- +created: 2026-09-02 +status: complete +requirements: + - REQ-UI-PLAN-COMMENT-DRAFTS-001 +system_design: + - ../../specs/ui/system-design/plan-comment-drafts.md +legacy_specs: [] +--- + +# Implementation Plan: Preserve Plan Comments Across Session Switches + +## Overview + +Prevent Agent-session navigation from deleting another session's pending Plan +comments. Implement transaction provenance at the Tiptap mark boundary, reset +stale editor state at the session boundary, then prove the complete primary to +sibling to primary flow. + +## Confirmed root cause + +`TaskPlanPanel` supplies session A's comments to a task-scoped Tiptap editor. +Selecting session B supplies a different projection. `rehydrateCommentMarks` +programmatically removes A's marks, but `CommentMark` reports that removal as a +user-created orphan. `TaskPlanPanel` responds by deleting A's comment from the +store and `sessionStorage`. + +A temporary focused Vitest reproduction passed before planning: rerendering +`TipTapPlanEditor` from one comment to an empty projection emitted +`onCommentDeleted` with the original ID. The temporary test was removed after +capturing the evidence. + +## Scope + +### In scope + +- Distinguish projection reconciliation from destructive user edits. +- Preserve session-scoped comment state and storage across session switches. +- Close stale comment editing/selection state when active session changes. +- Restore marks, badges, feedback, and composer context when returning to the + owning session. +- Add transaction, component, and desktop multi-session regression coverage. + +### Out of scope + +- Backend or WebSocket changes. +- Task-scoped or server-persisted pending comments. +- Plan editor, Popover, Drawer, or navigation redesign. +- Recovery of comments already removed by released versions. + +## Technical approach + +### Mark reconciliation + +Update `comment-mark.tsx` so `rehydrateCommentMarks` tags its own mark-only +transaction. Orphan detection skips tagged projection removals and continues to +report IDs removed by untagged user document changes. Before reporting, confirm +the ID remains absent from final document state. + +### Session boundary + +Update `TaskPlanPanel` selection handling so a changed `activeSessionId` closes +the open comment editor, clears editor/browser selection, and clears +`editingCommentId`. Keep `usePlanComments` and comment persistence keyed by +session. + +### Regression coverage + +Add failing-first unit/component tests for projection replacement, real orphan +cleanup, and stale edit-state reset. Extend `multi-session-ux.spec.ts` with one +primary to sibling to primary scenario that restores a pending comment's badge +and editable feedback. + +## Tests + +| Acceptance criterion | Evidence | +| --- | --- | +| `AC-UI-PLAN-COMMENT-DRAFTS-001.1` | Existing add/persistence coverage plus the multi-session E2E setup | +| `AC-UI-PLAN-COMMENT-DRAFTS-001.2` | Tiptap projection unit regression and multi-session E2E | +| `AC-UI-PLAN-COMMENT-DRAFTS-001.3` | Multi-session E2E restores badge and feedback | +| `AC-UI-PLAN-COMMENT-DRAFTS-001.4` | Paired tagged-reconciliation and untagged-destructive-edit unit tests | +| `AC-UI-PLAN-COMMENT-DRAFTS-001.5` | Focused TaskPlanPanel session-change test | +| `AC-UI-PLAN-COMMENT-DRAFTS-001.6` | Shared editor/state unit evidence; no viewport-specific production branch changes | + +## E2E tests + +Extend `apps/web/e2e/tests/session/multi-session-ux.spec.ts` with +`preserves pending plan comments across session switches` under the Chromium +project. It creates a plan and two sessions, adds feedback with **Add**, selects +the sibling session, returns to the primary session, and opens the restored +badge to verify exact feedback text. + +No new mobile spec is planned because the repair changes shared state and +Tiptap transaction semantics only. Existing mobile Plan comment flows continue +to cover the unchanged Drawer/touch presentation. + +## Work orders + +- [x] [Task 01: Preserve plan comments across session switches](task-01-preserve-plan-comments.md) + +## Verification results + +- RED: the permanent projection regression observed + `onCommentDeleted(["primary-comment"])`, and the session-boundary regression + observed the stale textarea remain mounted. +- RED browser proof: returning to the primary session found zero badges instead + of one in the production-built Chromium flow. +- GREEN: the focused Vitest command passed all 7 tests across the editor and + session-boundary suites. +- GREEN browser proof: the focused Chromium flow passed, restoring one badge + and the exact feedback text after primary to sibling to primary navigation. +- `pnpm run typecheck` and focused ESLint passed. The implementation adds no + viewport-specific branch, so desktop and mobile share the repaired state and + transaction path; mobile layout and touch presentation remain unchanged. + +## Risks + +- Suppressing every orphan callback would leak comments after real plan edits; + regression coverage must prove only tagged reconciliation is ignored. +- Deferred Tiptap transactions can race a session change; transaction metadata, + not the current React projection, must identify the event source. +- Global `editingCommentId` can cross session boundaries unless transient edit + state is reset with the session identity. diff --git a/docs/plans/plan-comment-session-switch-preservation/task-01-preserve-plan-comments.md b/docs/plans/plan-comment-session-switch-preservation/task-01-preserve-plan-comments.md new file mode 100644 index 0000000000..bcd38a0833 --- /dev/null +++ b/docs/plans/plan-comment-session-switch-preservation/task-01-preserve-plan-comments.md @@ -0,0 +1,121 @@ +--- +id: "01-preserve-plan-comments" +title: "Preserve plan comments across session switches" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-UI-PLAN-COMMENT-DRAFTS-001 +acceptance_criteria: + - AC-UI-PLAN-COMMENT-DRAFTS-001.1 + - AC-UI-PLAN-COMMENT-DRAFTS-001.2 + - AC-UI-PLAN-COMMENT-DRAFTS-001.3 + - AC-UI-PLAN-COMMENT-DRAFTS-001.4 + - AC-UI-PLAN-COMMENT-DRAFTS-001.5 + - AC-UI-PLAN-COMMENT-DRAFTS-001.6 +system_design: + - ../../specs/ui/system-design/plan-comment-drafts.md +--- + +# Task 01: Preserve Plan Comments Across Session Switches + +## Summary + +Separate Tiptap projection reconciliation from user-authored mark deletion so +changing Agent sessions cannot remove pending comments from browser state. +Reset stale comment-editor state on session changes and verify the full user +flow with focused tests. + +## In scope + +- Add private transaction provenance to programmatic comment-mark + reconciliation. +- Preserve orphan cleanup for real destructive plan edits. +- Reset open plan-comment selection/edit state when `activeSessionId` changes. +- Add unit, component, and desktop multi-session E2E regressions. + +## Out of scope + +- Comment data-model, storage-key, API, backend, or WebSocket changes. +- Moving comments between sessions. +- Mobile or desktop layout changes. +- Recovering previously deleted comments. + +## Acceptance + +- Replacing the editor's session comment projection removes old visible marks + without calling the destructive comment callback or changing stored drafts. +- A real user document edit that removes a marked range still deletes its + pending comment exactly once, and a session change closes stale edit state. +- Returning to the owning session restores the pending comment's badge and + exact feedback in the production-built desktop user flow. + +## Verification + +```bash +cd apps && pnpm install --frozen-lockfile +cd apps/web && pnpm exec vitest run components/editors/tiptap/tiptap-plan-editor.test.tsx components/task/task-plan-panel.session-switch.test.tsx +cd apps/web && pnpm e2e:run tests/session/multi-session-ux.spec.ts -- --grep "preserves pending plan comments across session switches" +``` + +After specification or plan edits: + +```bash +python3 scripts/lint-spec-files.py --all +git diff --check +``` + +## Files likely touched + +- `apps/web/components/editors/tiptap/comment-mark.tsx` +- `apps/web/components/editors/tiptap/tiptap-plan-editor.test.tsx` +- `apps/web/components/task/task-plan-panel.tsx` +- `apps/web/components/task/use-plan-selection.ts` +- `apps/web/components/task/task-plan-panel.session-switch.test.tsx` +- `apps/web/e2e/tests/session/multi-session-ux.spec.ts` +- `docs/plans/plan-comment-session-switch-preservation/plan.md` +- `docs/plans/plan-comment-session-switch-preservation/task-01-preserve-plan-comments.md` + +## Dependencies + +None. + +## Risks + +- Transaction suppression must be limited to tagged projection reconciliation; + otherwise genuine orphaned comments could remain. +- The session-change reset must not run on ordinary comment-store updates or + close an editor while the owning session remains selected. + +## Parallelism + +`sequential` + +## Inputs + +- `REQ-UI-PLAN-COMMENT-DRAFTS-001` and its system design. +- Current `CommentMark`, `TipTapPlanEditor`, `TaskPlanPanel`, and comment-store + behavior. +- Existing multi-session and Plan comment Playwright patterns. +- Confirmed temporary Vitest reproduction recorded in `plan.md`. + +## Results + +- Tagged every comment-mark projection transaction with private provenance and + limited orphan cleanup to IDs removed by untagged document transactions and + still absent from final document state. +- Cleared local selection, browser selection, and global comment-edit identity + when `activeSessionId` changes. +- RED unit evidence: projection replacement invoked the destructive callback; + session replacement left the old comment textarea open. +- RED browser evidence: the primary-session badge count remained zero after a + primary to sibling to primary round trip. +- GREEN unit command: + `cd apps/web && pnpm exec vitest run components/editors/tiptap/tiptap-plan-editor.test.tsx components/task/task-plan-panel.session-switch.test.tsx` + passed 7 tests in 2 files. +- GREEN browser command: + `cd apps/web && pnpm e2e:run tests/session/multi-session-ux.spec.ts -- --grep "preserves pending plan comments across session switches"` + passed its Chromium scenario against the production build. +- `cd apps/web && pnpm run typecheck` and focused ESLint passed. No backend, + API, schema, persistence-key, mobile layout, or touch behavior changed. diff --git a/docs/specs/ui/README.md b/docs/specs/ui/README.md index 75c8054fc0..2f47d03896 100644 --- a/docs/specs/ui/README.md +++ b/docs/specs/ui/README.md @@ -66,16 +66,17 @@ feature behavior and state. - [Mermaid Rendering](requirements/mermaid-rendering.md) - [Message favorite star mobile sizing](requirements/message-favorite-star-mobile-size.md) - [Message metadata dialog scroll containment](requirements/message-metadata-overflow.md) -- [Automatically Merge Consecutive Queued Messages](requirements/message-queue-auto-merge.md) +- [Message queue auto-merge](requirements/message-queue-auto-merge.md) - [Manage Pending Message Queues](requirements/message-queue-management.md) - [Merge Enqueued Messages Individually](requirements/message-queue-merge.md) - [Pin the Message Queue Panel](requirements/message-queue-pin.md) - [Reorder Queued Messages](requirements/message-queue-reorder.md) - [Control Pending Message Auto-run](requirements/message-queue-run.md) - [Send Queued Messages Now](requirements/message-queue-send-now.md) -- [Mobile Workspace Topbar Actions](requirements/mobile-quick-chat-topbar.md) +- [Mobile workspace topbar](requirements/mobile-quick-chat-topbar.md) - [Mobile Task Chrome](requirements/mobile-task-chrome.md) - [Mobile Task Navigation](requirements/mobile-task-navigation.md) +- [Plan comments](requirements/plan-comment-drafts.md) - [Task-scoped port-forwarding discovery](requirements/port-forwarding-discovery.md) - [Open proxy URLs in the browser panel](requirements/port-proxy-browser-panel.md) - [Responsive PR Detail Header](requirements/pr-detail-header-width.md) @@ -85,8 +86,8 @@ feature behavior and state. - [Preview Sprites Transient Retry](requirements/preview-sprites-transient-retry.md) - [Persistent status motion](requirements/persistent-status-motion.md) - [Prompt History Panel](requirements/prompt-history-panel.md) -- [Render Nerd Font glyphs pasted from a styled terminal](requirements/prompt-paste-nerd-font-glyphs.md) -- [Prompt Turn Duration on Message Hover](requirements/prompt-turn-duration.md) +- [Prompt paste: Nerd Font glyphs](requirements/prompt-paste-nerd-font-glyphs.md) +- [Prompt turn duration](requirements/prompt-turn-duration.md) - [Published Docs Preview Reliability](requirements/published-docs-preview-reliability.md) - [Quick Chat elevation](requirements/quick-chat-elevation.md) - [Quick Chat viewport layout](requirements/quick-chat-viewport-layout.md) @@ -167,6 +168,7 @@ feature behavior and state. - [Quick Chat and terminal elevation](system-design/quick-chat-elevation.md) - [Quick Chat viewport layout](system-design/quick-chat-viewport-layout.md) - [Quick Chat and Terminal Tabs](system-design/quick-terminal.md) +- [Plan comments](system-design/plan-comment-drafts.md) - [Responsive Plan Formatting](system-design/responsive-plan-formatting.md) - [Confirmations](system-design/confirmation-warning-hierarchy.md) - [Resizable Markdown Table Columns](system-design/resizable-markdown-tables.md) diff --git a/docs/specs/ui/requirements/plan-comment-drafts.md b/docs/specs/ui/requirements/plan-comment-drafts.md new file mode 100644 index 0000000000..d4c4f9b05e --- /dev/null +++ b/docs/specs/ui/requirements/plan-comment-drafts.md @@ -0,0 +1,72 @@ +--- +status: active +system: ui +created: 2026-09-02 +owners: + - kandev +--- + +# Plan Comment Drafts Requirements + +## Overview + +Task plans are shared by every session in a task, while pending plan comments +are browser-local prompt context owned by one task session. The UI system owns +the projection between those session-scoped drafts and the shared Plan editor. +Changing the selected Agent session can change which annotations are visible, +but it must never turn that presentation change into destructive user intent. + +## Terminology + +- **Owning session:** The task session selected when a pending plan comment is + created and whose composer can submit that comment. +- **Comment projection:** The owning session's pending plan comments currently + rendered as Plan highlights, badges, and composer context. +- **Projection reconciliation:** A UI-driven change to rendered comment marks + after a session switch, hydration, or editor remount. +- **Destructive edit:** An explicit delete action or a user edit that removes a + pending comment's marked plan range. + +## Requirements + +### REQ-UI-PLAN-COMMENT-DRAFTS-001: Preserve session-scoped plan comment drafts + +**Intent:** Users can move between Agent sessions without losing pending +feedback they prepared against the shared task plan. + +**User story:** As a user reviewing a task plan, I want pending comments to +survive Agent-session navigation, so that inspecting another session cannot +erase feedback I have not sent. + +#### Acceptance criteria + +- **AC-UI-PLAN-COMMENT-DRAFTS-001.1:** When a user adds a pending plan comment, + the UI shall persist it in browser `sessionStorage` for the owning session and + show its highlight, badge, and composer context while that session is + selected. +- **AC-UI-PLAN-COMMENT-DRAFTS-001.2:** When the user selects a different session + in the same task, the UI shall show that session's comment projection without + deleting, modifying, or moving pending plan comments owned by the previous + session. +- **AC-UI-PLAN-COMMENT-DRAFTS-001.3:** When the user returns to a session that + owns pending plan comments, the UI shall restore their highlights, badges, + feedback text, and composer context without requiring a reload or another + user action. +- **AC-UI-PLAN-COMMENT-DRAFTS-001.4:** Projection reconciliation shall not count + as comment deletion. Pending plan comments shall be removed only by an + explicit delete action, a destructive edit of their marked plan range, or a + successful delivery path. Failed delivery shall preserve them. +- **AC-UI-PLAN-COMMENT-DRAFTS-001.5:** When the selected session changes while a + plan comment editor is open, the UI shall close that transient editor and + clear its selection state. A later action in the new session shall not update, + delete, or deliver the previous session's comment. +- **AC-UI-PLAN-COMMENT-DRAFTS-001.6:** Desktop and mobile task surfaces shall + apply the same draft-preservation and session-ownership behavior. + +## Out of scope + +- Changing pending plan comments from session-scoped to task-scoped data. +- Persisting pending comments in the backend or synchronizing them across + browsers or tabs. +- Changing Plan comment popover, Drawer, toolbar, or task-navigation layout. +- Recovering comments already removed by versions that predate this contract. diff --git a/docs/specs/ui/system-design/plan-comment-drafts.md b/docs/specs/ui/system-design/plan-comment-drafts.md new file mode 100644 index 0000000000..56a39eccf9 --- /dev/null +++ b/docs/specs/ui/system-design/plan-comment-drafts.md @@ -0,0 +1,161 @@ +--- +status: current +system: ui +requirements: + - REQ-UI-PLAN-COMMENT-DRAFTS-001 +--- + +# Plan Comment Drafts System Design + +## Purpose and boundaries + +This design keeps session-scoped pending plan comments safe while the shared +task Plan editor changes its visible comment projection. The UI system owns the +browser-local comment store, Tiptap marks, session-selection response, and +composer context. The task system continues to own plan content, revisions, +sessions, and message delivery. + +No backend, API, WebSocket, task-plan schema, or comment data-model change is +required. `PlanComment.sessionId` remains the delivery and persistence owner. + +## Root cause and evidence + +`useTaskPlan(taskId)` correctly treats the plan document as task-scoped. +`TaskPlanPanel` separately calls `usePlanComments(activeSessionId)`, so selecting +a sibling Agent session replaces the editor's `comments` prop with that +session's projection. + +Before this repair, `rehydrateCommentMarks` removed every Tiptap `commentMark` +absent from the new projection without recording why. That programmatic +document transaction reached the `CommentMark` orphan detector. The detector +interpreted the removed mark as a destructive edit and called +`onOrphanedComments`. `TaskPlanPanel.handleCommentDeleted` then removed the +comment from the Zustand store. `persistSessionComments` removed the owning +session's `sessionStorage` key when no comments remained, making the loss +permanent. + +A focused temporary Vitest reproduction rendered one comment, rerendered the +same editor with an empty comment projection, and observed +`onCommentDeleted([commentId])`. It passed against the current code. The test +was removed after diagnosis; the implementation work order replaces it with a +permanent failing-first regression. + +## Requirement mapping + +| Requirement | Design section | +| --- | --- | +| `REQ-UI-PLAN-COMMENT-DRAFTS-001` | [State and persistence](#state-and-persistence), [Transaction provenance](#transaction-provenance), [Session-switch flow](#session-switch-flow), [Verification](#verification) | + +## Components and responsibilities + +- `useCommentsStore` remains the authoritative browser-local comment state and + persists each session under `kandev.comments.`. +- `usePlanComments` selects only `PlanComment` records owned by the selected + session and creates new comments for that session. +- `TaskPlanPanel` projects the selected session's comments into the shared Plan + editor and clears transient selection/edit state when session identity + changes. +- `TipTapPlanEditor` asks `rehydrateCommentMarks` to reconcile rendered marks + with the supplied projection. +- `CommentMark` distinguishes projection-reconciliation transactions from + user-authored document transactions before reporting orphaned comments. +- Task chat continues to include and clear only the active session's pending + plan comments after successful delivery. + +## State and persistence + +The existing `CommentBase.sessionId`, `CommentsState.bySession`, and +`kandev.comments.` storage keys remain unchanged. Switching sessions +does not migrate or clone comments. The selected session controls which comment +IDs become editor marks and composer context. + +An empty projection means no plan-comment marks are rendered for the selected +session. It does not mean comments belonging to another session were deleted. +Returning to the owning session reads its unchanged store entries and +rehydrates their marks from saved positions or selected-text fallback search. + +## Transaction provenance + +`rehydrateCommentMarks` shall tag the transaction it dispatches with a private +comment-projection reconciliation marker. The `CommentMark` orphan-detection +plugin shall ignore mark removals performed by tagged transactions. + +Untagged user document transactions retain current orphan cleanup. For every +untagged document-changing transaction, the plugin compares comment IDs before +and after that transaction, then reports only IDs still absent from the final +document state. This keeps a real destructive edit authoritative without +mistaking projection changes for user intent. + +Explicit deletion remains safe: the UI removes the rendered mark and store +entry as one user action, and any deferred orphan cleanup is idempotent. Tagged +additions and removals from projection changes never affect store state. + +## Session-switch flow + +1. The user adds a plan comment while session A is selected. Store and + `sessionStorage` record session A as owner. +2. The user selects session B. `usePlanComments` publishes session B's comment + projection. +3. Tiptap dispatches one tagged reconciliation transaction that removes A's + visible marks and adds B's marks. Orphan cleanup ignores those presentation + changes. +4. `TaskPlanPanel` closes any open comment editor and clears global + `editingCommentId` plus local text-selection state for the old session. +5. Selecting session A publishes its unchanged comments. Tagged reconciliation + restores their marks, badges, and editable feedback; task chat restores the + same session's context chip. + +## Failure and recovery + +- If mark position metadata is stale after plan editing, existing selected-text + fallback search remains responsible for finding the range. +- If a destructive user edit removes a mark, untagged orphan detection removes + the associated pending comment as before. +- A failed direct, queued, or passthrough send leaves comment state unchanged. +- A session switch during an open editor closes the editor rather than applying + its draft to another session. +- Comments already deleted by the old path have no backend copy and cannot be + reconstructed by this repair. + +## Responsive behavior + +Desktop and mobile reuse the same comment store, Plan editor, mark extension, +and session identity. The repair changes no composition, overlay geometry, +scroll owner, safe-area handling, or touch target. Existing desktop Popover and +mobile Drawer presentation remain unchanged. + +Because this is state and transaction normalization inside the shared editor, +focused unit coverage plus one desktop multi-session E2E flow provides the +cross-viewport logic evidence. No new mobile Playwright scenario is required; +existing mobile Plan comment coverage continues to own touch presentation. + +## Verification + +- `tiptap-plan-editor.test.tsx` changes the `comments` projection and proves the + previous comment is visually unmounted without invoking deletion. A paired + case performs a real destructive editor transaction and proves orphan cleanup + still fires. +- A focused `TaskPlanPanel` test changes active session identity while a comment + editor is open and proves transient editing state closes without mutating the + old comment. +- `multi-session-ux.spec.ts` creates a plan and two sessions, adds a pending + comment in the primary session, selects the sibling, returns to the primary, + and proves the badge and feedback text are restored. + +No production telemetry is added. The failure is deterministic and covered at +the transaction, component, and user-flow boundaries. + +## Alternatives considered + +- Keying Tiptap editor instances by session would avoid the callback by + destroying the editor, but it would add expensive remounts and focus/selection + churn to a task-scoped document. +- Guarding deletion only in `TaskPlanPanel` would leave transaction provenance + ambiguous and remain vulnerable to deferred callback races. +- Making plan comments task-scoped would change which Agent receives pending + prompt context and is outside this repair. + +## Related decisions + +None. The design separates programmatic reconciliation from existing user +intent without creating a new public or architectural boundary.