fix(web): preserve plan comments across session switches (#3272)
This commit is contained in:
@@ -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<CommentMarkOptions>({
|
||||
// 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<string>();
|
||||
|
||||
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);
|
||||
|
||||
@@ -78,6 +78,12 @@ async function renderReadyEditor(onChange: (value: string) => void): Promise<Edi
|
||||
return readyEditor!;
|
||||
}
|
||||
|
||||
async function flushDeferredCommentCallbacks(): Promise<void> {
|
||||
await act(async () => {
|
||||
await new Promise<void>((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(
|
||||
<TipTapPlanEditor
|
||||
taskId="task-1"
|
||||
value="Primary plan text"
|
||||
onChange={() => undefined}
|
||||
comments={[comment]}
|
||||
onCommentDeleted={onCommentDeleted}
|
||||
/>,
|
||||
);
|
||||
|
||||
await waitFor(() => {
|
||||
expect(
|
||||
view.container.querySelector('.comment-badge[data-comment-id="primary-comment"]'),
|
||||
).not.toBeNull();
|
||||
});
|
||||
|
||||
view.rerender(
|
||||
<TipTapPlanEditor
|
||||
taskId="task-1"
|
||||
value="Primary plan text"
|
||||
onChange={() => 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(
|
||||
<TipTapPlanEditor
|
||||
taskId="task-1"
|
||||
value="Primary plan text"
|
||||
onChange={() => 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);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
@@ -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<typeof usePlanComments>,
|
||||
) {
|
||||
const [textSelection, setTextSelection] = useState<TextSelection | null>(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,
|
||||
|
||||
@@ -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<typeof usePlanComments>,
|
||||
"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<TextSelection | null>(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 };
|
||||
}
|
||||
@@ -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<void>((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,
|
||||
|
||||
@@ -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.
|
||||
+121
@@ -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.
|
||||
@@ -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)
|
||||
|
||||
@@ -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.
|
||||
@@ -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.<sessionId>`.
|
||||
- `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.<sessionId>` 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.
|
||||
Reference in New Issue
Block a user