Skip to content

vscode: edit + preview-side delete on review comments (make the markdown reviewer functional) #1055

Description

@amrmelsayed

Problem

The review-comment surfaces shipped across #859 (add from preview), #860 (summary panel), #861 (preview nav), #863 (inline cards + minimap), and #1121 (inline composer) let a reviewer create and read comments. Two capabilities are missing that make the reviewer feel half-finished:

  • Edit — not possible from either surface. Fixing a typo requires deleting and re-adding the comment.
  • Delete from the preview — works only from the editor gutter. Users have to context-switch away from the preview surface they're reading to remove a comment they're looking at.

Without both, the markdown reviewer isn't a functional reviewing tool — comments are effectively add-only from the preview, and edit doesn't exist anywhere.

Approach: line + content identity, no format changes

Both operations work against the current flat marker format (<!-- REVIEW(@author): body -->) without a stable ID. The preview knows which card the user clicked; each card corresponds to a specific marker at a known file line (not just anchor line). Line-identity uniquely locates the marker within the file — one marker per line, no ambiguity even for stacked comments on the same block.

The preview auto-refreshes on file changes, so if someone else edits the file between the user's click and the host's write, the preview re-pushes with updated line numbers. The host's write path verifies the marker at the expected line still matches the expected author + body-prefix before mutating; a race fails loudly with a refresh, not silently by editing the wrong marker.

Format v2 (adding a stable ID field, resolved state, etc.) was originally proposed here but explicitly dropped: line+content identity solves edit+delete without it, and speculative format changes carry cross-host compatibility risk against the dashboard's bundled codec of @cluesmith/codev-core/review-markers. Format v2 becomes a derived requirement of the future feature that actually needs a new field — see #1131 (resolve state), which now owns that design work.

What ships in this PR

  1. Edit review comment — editor Comments-API surface (packages/vscode/src/comments/plan-review.ts). Adds an edit action to the CommentThread; opens VS Code's native inline edit surface; on submit rewrites the marker line via WorkspaceEdit, preserving the same author and updating the body.
  2. Edit review comment — preview composer surface (packages/vscode/src/markdown-preview/preview-provider.ts + the artifact-canvas card component). Adds an edit affordance to comment cards; opens the inline composer with the current body prefilled; on submit sends editComment { markerLine, expectedAuthor, expectedBodyPrefix, newBody } to the host, which verifies and rewrites.
  3. Delete review comment — preview surface (packages/vscode/src/markdown-preview/preview-provider.ts + the artifact-canvas card component). Adds a delete affordance to comment cards; on click sends deleteComment { markerLine, expectedAuthor, expectedBodyPrefix } to the host, which verifies the line is still the expected marker and deletes it.

The expectedAuthor + expectedBodyPrefix in the message payloads implement optimistic concurrency control: if the file changed between click and write, the check fails, the preview refreshes to show the current state, and the write path errors legibly rather than corrupting a different marker.

Plan-gate decisions to lock

  • Preview delete affordance UI: × icon on the card corner, trash icon inline with the body, or ... menu with delete inside. Plan-gate bikeshed at fidelity / discoverability tradeoff.
  • Edit affordance in preview: pencil icon vs ... menu vs double-click-body-to-edit. Plan-gate.
  • Message payload shape: expectedAuthor + expectedBodyPrefix for optimistic concurrency, or send the entire current raw marker text and byte-match against it? First is more forgiving of whitespace differences.
  • Error UX on race: the write fails, the preview refreshes automatically — but should we surface a toast ("this comment changed while you were editing; showing the latest") or just refresh silently? Plan-gate.

Acceptance criteria

Explicitly out of scope

Protocol

PIR. The mechanic is well-understood but the UX has multiple design decisions (affordance placement, message payload shape, race UX) worth plan-gate discussion, and the two-surface (editor + preview) implementation benefits from dev-gate verification on a running instance before PR — race conditions in particular need eyes-on-a-running-preview to confirm the failure mode is legible.

Related

Metadata

Metadata

Assignees

Labels

area/vscodeArea: VS Code extension

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions