diff --git a/.changeset/bright-ranges-comment.md b/.changeset/bright-ranges-comment.md new file mode 100644 index 000000000..97557cf16 --- /dev/null +++ b/.changeset/bright-ranges-comment.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": minor +--- + +Add persistent mouse and keyboard diff selections with explicit Comment, Copy, and Clear actions, including multiline review-note anchors. diff --git a/docs/keybindings.md b/docs/keybindings.md index 356b8f1e3..1b9eba58c 100644 --- a/docs/keybindings.md +++ b/docs/keybindings.md @@ -28,6 +28,9 @@ Rules worth knowing: - Two entries claiming one chord is a conflict: the first in the file wins and the session reports the other. Unknown command ids and unusable chords are reported the same way, and the rest of the table still applies. +- **Escape clears an active visual selection contextually** after overlays and + extension modes have had their normal ownership. Without a selection, Escape + remains available to extension commands because clear-selection is unbound. Chords are `ctrl`, `alt`/`option`, `cmd`/`meta`, and `shift` joined with `+` around a base key: a character (`"y"`, `"["`), an uppercase letter for its @@ -55,6 +58,8 @@ The built-in commands and the keys they ship with: | `hunk.review.alignCurrentLineBottom` | Align current line to viewport bottom | _(none)_ | | `hunk.review.alignCurrentLineCenter` | Center current line in viewport | _(none)_ | | `hunk.review.alignCurrentLineTop` | Align current line to viewport top | _(none)_ | +| `hunk.review.clearSelection` | Clear the active visual selection | _(none)_ | +| `hunk.review.copySelection` | Copy the active visual selection | `y` | | `hunk.review.editActiveNote` | Edit the active review note | `E` | | `hunk.review.editSelectedFile` | Open the selected file in your editor | `e` | | `hunk.review.focusFilter` | Focus the file filter | `/` | @@ -76,6 +81,7 @@ The built-in commands and the keys they ship with: | `hunk.review.scrollCodeLeft` | Scroll code left (shifted scrolls fast) | `left`, `shift+left` | | `hunk.review.scrollCodeRight` | Scroll code right (shifted scrolls fast) | `right`, `shift+right` | | `hunk.review.startNote` | Add a review note | `c` | +| `hunk.review.startVisualSelection` | Start visual line selection | `v` | | `hunk.review.stepDown` | Scroll down one row | `down`, `j` | | `hunk.review.stepUp` | Scroll up one row | `up`, `k` | | `hunk.review.toggleHunkGap` | Expand or collapse the selected context | `z` | diff --git a/examples/extensions/review-triage/README.md b/examples/extensions/review-triage/README.md index 31d4f606b..73a1dc77b 100644 --- a/examples/extensions/review-triage/README.md +++ b/examples/extensions/review-triage/README.md @@ -12,7 +12,7 @@ Or copy the directory to your Hunk extensions directory and keep its `package.js ## Use -Open **Extensions → Toggle review triage** (`y`). The right pane lists each visible file's hunks; click a hunk to navigate the review stream. Use **Extensions → Mark selected hunk…** (`x`) to choose a status and enter an optional rationale. **Center current review line**, **Set review focus…**, and **Clear triage decisions** are menu-only commands. +Open **Extensions → Toggle review triage** (`Y`). The right pane lists each visible file's hunks; click a hunk to navigate the review stream. Use **Extensions → Mark selected hunk…** (`x`) to choose a status and enter an optional rationale. **Center current review line**, **Set review focus…**, and **Clear triage decisions** are menu-only commands. The board intentionally keeps state only for the running Hunk session. Reloading reconciles decisions against the newly parsed hunks and drops entries that no longer match, rather than silently transferring a decision to changed code. diff --git a/examples/extensions/review-triage/index.tsx b/examples/extensions/review-triage/index.tsx index d7c725d0c..47f543184 100644 --- a/examples/extensions/review-triage/index.tsx +++ b/examples/extensions/review-triage/index.tsx @@ -232,7 +232,7 @@ export default function registerReviewTriage(hunk: HunkExtensionAPI) { component: ReviewTriagePane, }); - hunk.registerCommand({ id: "toggle", title: "Toggle review triage", key: "y" }, (ctx) => + hunk.registerCommand({ id: "toggle", title: "Toggle review triage", key: "Y" }, (ctx) => ctx.panes.toggle("triage"), ); diff --git a/src/app/session/reviewCommands.test.ts b/src/app/session/reviewCommands.test.ts index e76d5afa9..843d29816 100644 --- a/src/app/session/reviewCommands.test.ts +++ b/src/app/session/reviewCommands.test.ts @@ -121,6 +121,141 @@ describe("applySessionReviewAction", () => { expect(result).toMatchObject({ ok: false, code: "file-not-found" }); }); + test("accepts only ranges fully covered by visible patch rows", () => { + const { producer, publication, file } = createTestProducer(); + const visibleTarget = { + newRange: [1, 3] as const, + preferred: { side: "new" as const, line: 2 }, + }; + + expect( + applySessionReviewAction( + producer, + envelope( + { + type: "notes/start-draft", + fileKey: file.key, + hunkIndex: 0, + target: visibleTarget, + }, + publication.generation, + ), + ).ok, + ).toBe(true); + producer.applyIntent( + { type: "notes/create-user", consumeDraft: true }, + { + noteId: "discard", + timestamp: "2026-01-01T00:00:00.000Z", + }, + ); + + const collapsed = applySessionReviewAction( + producer, + envelope( + { + type: "notes/start-draft", + fileKey: file.key, + hunkIndex: 0, + target: { newRange: [3, 17], preferred: { side: "new", line: 3 } }, + }, + publication.generation, + ), + ); + expect(collapsed).toMatchObject({ ok: false, code: "invalid-request" }); + }); + + test("rejects a range start whose requested hunk differs from its resolved owner", () => { + const { producer, publication, file } = createTestProducer(); + + const result = applySessionReviewAction( + producer, + envelope( + { + type: "notes/start-draft", + fileKey: file.key, + hunkIndex: 0, + target: { newRange: [17, 19], preferred: { side: "new", line: 18 } }, + }, + publication.generation, + ), + ); + + expect(result).toMatchObject({ ok: false, code: "invalid-request" }); + expect(producer.getReviewState()!.draftNote).toBeNull(); + }); + + test("requires a save precondition to match the draft's exact range", () => { + const { producer, publication, store, file } = createTestProducer(); + const target = { newRange: [1, 3] as const, preferred: { side: "new" as const, line: 2 } }; + expect( + applySessionReviewAction( + producer, + envelope( + { type: "notes/start-draft", fileKey: file.key, hunkIndex: 0, target }, + publication.generation, + ), + ).ok, + ).toBe(true); + store.dispatch({ type: "draft/update", body: "exact range" }); + + const competing = applySessionReviewAction( + producer, + envelope( + { + type: "notes/create-user", + consumeDraft: true, + fileKey: file.key, + hunkIndex: 0, + target: { newRange: [2, 3], preferred: { side: "new", line: 2 } }, + }, + publication.generation, + ), + ); + expect(competing).toMatchObject({ ok: false, code: "draft-missing" }); + expect(producer.getReviewState()!.draftNote).not.toBeNull(); + + const identitylessRange = applySessionReviewAction( + producer, + envelope({ type: "notes/create-user", consumeDraft: true, target }, publication.generation), + ); + expect(identitylessRange).toMatchObject({ ok: false, code: "invalid-request" }); + expect(producer.getReviewState()!.draftNote).not.toBeNull(); + + const wrongOwner = applySessionReviewAction( + producer, + envelope( + { + type: "notes/create-user", + consumeDraft: true, + fileKey: file.key, + hunkIndex: 1, + target, + }, + publication.generation, + ), + ); + expect(wrongOwner).toMatchObject({ ok: false, code: "draft-missing" }); + expect(producer.getReviewState()!.draftNote).not.toBeNull(); + + expect( + applySessionReviewAction( + producer, + envelope( + { + type: "notes/create-user", + consumeDraft: true, + fileKey: file.key, + hunkIndex: 0, + target, + }, + publication.generation, + ), + ).ok, + ).toBe(true); + expect(producer.getReviewState()!.userNotes.at(-1)!.note.anchor.newRange).toEqual([1, 3]); + }); + // Intent: B10 — a note on a line inside an expanded gap is expressible remotely, and the // hunk that ends up owning it is core's answer through the shared anchor path, never one // this tier recomputed (D3). @@ -156,6 +291,10 @@ describe("applySessionReviewAction", () => { line, hunkIndex: 1, }); + expect(draft.expandedLineSource).toEqual({ + sourceIdentity: file.sourceIdentity, + sourceAttested: true, + }); // Remote composition travels through the same semantic body-update intent as the terminal. expect( @@ -168,6 +307,22 @@ describe("applySessionReviewAction", () => { ).ok, ).toBe(true); + const disguisedRangeSave = applySessionReviewAction( + producer, + envelope( + { + type: "notes/create-user", + consumeDraft: true, + fileKey: file.key, + hunkIndex: 1, + target: { newRange: [line, line], preferred: { side: "new", line } }, + }, + publication.generation, + ), + ); + expect(disguisedRangeSave).toMatchObject({ ok: false, code: "draft-missing" }); + expect(producer.getReviewState()!.draftNote).not.toBeNull(); + // Saving with the same target as a precondition persists the note; its owner hunk is // the fallback the anchor resolver chose, which is the hunk the reviewer was reading. const saved = applySessionReviewAction( @@ -324,7 +479,13 @@ describe("applySessionReviewAction", () => { const result = applySessionReviewAction( producer, envelope( - { type: "notes/create-user", consumeDraft: true, target: { side: "new", line: 999 } }, + { + type: "notes/create-user", + consumeDraft: true, + fileKey: file.key, + hunkIndex: 0, + target: { side: "new", line: 999 }, + }, publication.generation, ), ); diff --git a/src/app/session/reviewCommands.ts b/src/app/session/reviewCommands.ts index a3e9107be..b18d7d00b 100644 --- a/src/app/session/reviewCommands.ts +++ b/src/app/session/reviewCommands.ts @@ -23,9 +23,16 @@ import { randomUUID } from "node:crypto"; import type { ReviewProducer } from "../review/producer"; import { classifyReviewPublication } from "../../core/review/generationOrder"; import { resolveReviewExpandedLine } from "../../core/review/expansion"; +import { reviewRangeTargetCoverageIssue } from "../../core/review/geometry"; import { requireReviewFile, ReviewIntentPlanningError } from "../../core/review/intents"; -import type { ReviewState } from "../../core/review/state"; -import type { ReviewFileV1, ReviewLineAddressV1 } from "../../core/review/types"; +import type { ReviewDraftNote, ReviewState } from "../../core/review/state"; +import type { + ReviewFileV1, + ReviewLineAddressV1, + ReviewLineRange, + ReviewNoteTargetV1, + ReviewRangeTargetV1, +} from "../../core/review/types"; import { toReviewIntent, type HunkReviewActionEnvelopeV1, @@ -134,6 +141,62 @@ function checkExpandedLine( ); } +/** Compare inclusive ranges without treating tuple identity as semantic identity. */ +function rangesEqual(left: ReviewLineRange | undefined, right: ReviewLineRange | undefined) { + return left === undefined + ? right === undefined + : right !== undefined && left[0] === right[0] && left[1] === right[1]; +} + +/** Return whether a save precondition names the active draft's exact anchor. */ +function draftMatchesTarget(draft: ReviewDraftNote, target: ReviewNoteTargetV1) { + const anchor = draft.anchor; + if ("line" in target) { + if (draft.targetKind === "range") return false; + if (!anchor) return draft.side === target.side && draft.line === target.line; + const expectedRange = [target.line, target.line] as const; + return ( + anchor.preferred?.side === target.side && + anchor.preferred.line === target.line && + rangesEqual(anchor.oldRange, target.side === "old" ? expectedRange : undefined) && + rangesEqual(anchor.newRange, target.side === "new" ? expectedRange : undefined) + ); + } + if (draft.targetKind === "line") return false; + return ( + anchor !== undefined && + rangesEqual(anchor.oldRange, target.oldRange) && + rangesEqual(anchor.newRange, target.newRange) && + anchor.preferred?.side === target.preferred.side && + anchor.preferred.line === target.preferred.line + ); +} + +/** Reject ranges that name absent rows, collapsed gaps, or an unrelated preferred line. */ +function checkRangeTarget( + producer: ReviewProducer, + file: ReviewFileV1, + target: ReviewRangeTargetV1, +): HunkReviewFailureV1 | undefined { + const issue = reviewRangeTargetCoverageIssue(file.hunks, target); + if (!issue) return undefined; + if (issue === "preferred") { + return fail( + producer, + "invalid-request", + `The preferred ${target.preferred.side} line is outside the review range it places.`, + ); + } + const range = issue === "old" ? target.oldRange : issue === "new" ? target.newRange : undefined; + return fail( + producer, + "invalid-request", + range + ? `The ${issue} range ${range[0]}-${range[1]} includes lines not visible in the current patch.` + : "The review range does not contain any source lines.", + ); +} + /** * Validate everything about one action that needs the current review to be known. * @@ -146,28 +209,51 @@ function checkAgainstReview( state: ReviewState, action: HunkReviewActionV1, ): HunkReviewFailureV1 | undefined { - if (action.type === "notes/start-draft") { - if (!action.expandedLineProof || !action.target) { - return undefined; - } + if (action.type === "notes/start-draft" && action.target) { const file = requireReviewFile(state, action.fileKey); - return checkExpandedLine(producer, file, action.target, action.expandedLineProof); + if (!("line" in action.target)) { + if (action.expandedLineProof) { + return fail( + producer, + "invalid-request", + "A one-line expansion proof cannot attest a range.", + ); + } + return checkRangeTarget(producer, file, action.target); + } + return action.expandedLineProof + ? checkExpandedLine(producer, file, action.target, action.expandedLineProof) + : undefined; } if (action.type === "notes/create-user" && action.target) { // A stated target is a precondition on the draft being saved, so two surfaces cannot // silently save each other's work: the draft must still be the one the caller opened. const draft = state.draftNote; - if (!draft || draft.side !== action.target.side || draft.line !== action.target.line) { + if ( + !("line" in action.target) && + (action.fileKey === undefined || action.hunkIndex === undefined) + ) { return fail( producer, - "draft-missing", - `No review note draft is open at ${action.target.side} line ${action.target.line}.`, + "invalid-request", + "A range save precondition requires its file and owner hunk.", ); } - if (!action.expandedLineProof) { - return undefined; + if ( + !draft || + (action.fileKey !== undefined && draft.fileKey !== action.fileKey) || + (action.hunkIndex !== undefined && draft.hunkIndex !== action.hunkIndex) || + !draftMatchesTarget(draft, action.target) + ) { + return fail(producer, "draft-missing", "No review note draft is open at that exact anchor."); + } + if (!("line" in action.target)) { + return action.expandedLineProof + ? fail(producer, "invalid-request", "A one-line expansion proof cannot attest a range.") + : undefined; } + if (!action.expandedLineProof) return undefined; const file = requireReviewFile(state, draft.fileKey); return checkExpandedLine(producer, file, action.target, action.expandedLineProof); } diff --git a/src/core/review/anchors.ts b/src/core/review/anchors.ts index e050b239b..48d87a887 100644 --- a/src/core/review/anchors.ts +++ b/src/core/review/anchors.ts @@ -12,7 +12,12 @@ * reading a verdict out of the anchor. */ import { reviewHunkRange, reviewRangesOverlap, type ReviewHunkSpan } from "./geometry"; -import type { ReviewLineRange, ReviewRangeAnchorV1, ReviewSide } from "./types"; +import type { + ReviewLineRange, + ReviewRangeAnchorV1, + ReviewRangeTargetV1, + ReviewSide, +} from "./types"; /** * Names the hunk a line outside every hunk hangs from. @@ -91,6 +96,19 @@ export function resolveReviewNoteAnchor( }; } +/** Resolve a caller-supplied range while retaining its declared fallback owner. */ +export function reviewRangeAnchor( + hunks: readonly ReviewHunkSpan[], + target: ReviewRangeTargetV1 & { hunkIndex: number }, +): ReviewRangeAnchorV1 { + return resolveReviewNoteAnchor(hunks, { + ...(target.oldRange ? { oldRange: target.oldRange } : {}), + ...(target.newRange ? { newRange: target.newRange } : {}), + preferred: target.preferred, + fallbackOwnerHunkIndex: target.hunkIndex, + }); +} + /** * Anchor one note to a single line inside one hunk. * diff --git a/src/core/review/geometry.test.ts b/src/core/review/geometry.test.ts index 262897ef2..1fca4f78a 100644 --- a/src/core/review/geometry.test.ts +++ b/src/core/review/geometry.test.ts @@ -7,6 +7,8 @@ import { reviewHunkIndexForLine, reviewHunkRange, reviewHunkRanges, + reviewRangeCoveredByHunks, + reviewRangeTargetCoverageIssue, reviewRangesOverlap, } from "./geometry"; @@ -59,6 +61,52 @@ describe("reviewHunkIndexForLine", () => { }); }); +describe("reviewRangeCoveredByHunks", () => { + test("requires every line to be backed by real rows, across contiguous hunks", () => { + expect(reviewRangeCoveredByHunks([span(1, 3), span(4, 2)], "new", [2, 5])).toBe(true); + expect(reviewRangeCoveredByHunks([span(1, 3), span(5, 2)], "new", [2, 5])).toBe(false); + }); + + test("does not treat a zero-count side's synthetic position as visible coverage", () => { + expect( + reviewRangeCoveredByHunks( + [{ additionStart: 7, additionCount: 1, deletionStart: 6, deletionCount: 0 }], + "old", + [6, 6], + ), + ).toBe(false); + }); +}); + +describe("reviewRangeTargetCoverageIssue", () => { + const hunks = [span(1, 3), span(4, 2)]; + + test("accepts dual ranges backed entirely by visible rows", () => { + expect( + reviewRangeTargetCoverageIssue(hunks, { + oldRange: [2, 4], + newRange: [2, 5], + preferred: { side: "new", line: 5 }, + }), + ).toBeUndefined(); + }); + + test("distinguishes unbacked sides and unrelated preferred lines", () => { + expect( + reviewRangeTargetCoverageIssue(hunks, { + newRange: [2, 6], + preferred: { side: "new", line: 6 }, + }), + ).toBe("new"); + expect( + reviewRangeTargetCoverageIssue(hunks, { + newRange: [2, 5], + preferred: { side: "old", line: 2 }, + }), + ).toBe("preferred"); + }); +}); + describe("reviewRangesOverlap", () => { test("treats touching endpoints as overlapping and gaps as not", () => { expect(reviewRangesOverlap([1, 3], [3, 5])).toBe(true); diff --git a/src/core/review/geometry.ts b/src/core/review/geometry.ts index 2f2f06831..72a8a3f34 100644 --- a/src/core/review/geometry.ts +++ b/src/core/review/geometry.ts @@ -11,7 +11,12 @@ * `ReviewHunkV1` both satisfy them, so the terminal can call these primitives from its * render path without projecting a whole semantic document first. */ -import type { ReviewLineAddressV1, ReviewLineRange, ReviewSide } from "./types"; +import type { + ReviewLineAddressV1, + ReviewLineRange, + ReviewRangeTargetV1, + ReviewSide, +} from "./types"; /** The per-side extent of one hunk: 1-based start plus the rows it spans on that side. */ export interface ReviewHunkSpan { @@ -59,6 +64,54 @@ export function reviewRangesOverlap(left: ReviewLineRange, right: ReviewLineRang return left[0] <= right[1] && right[0] <= left[1]; } +/** Return whether every line in a range is backed by visible rows in the patch hunks. */ +export function reviewRangeCoveredByHunks( + hunks: readonly ReviewHunkSpan[], + side: ReviewSide, + range: ReviewLineRange, +) { + let nextLine = range[0]; + const covered = hunks + .filter((hunk) => (side === "new" ? hunk.additionCount : hunk.deletionCount) > 0) + .map((hunk) => reviewHunkRange(hunk, side)) + .sort((left, right) => left[0] - right[0]); + + for (const [start, end] of covered) { + if (end < nextLine) continue; + if (start > nextLine) return false; + nextLine = Math.max(nextLine, end + 1); + if (nextLine > range[1]) return true; + } + return false; +} + +export type ReviewRangeCoverageIssue = "preferred" | "old" | "new" | "empty"; + +/** Report why one range target is not fully backed by visible patch rows. */ +export function reviewRangeTargetCoverageIssue( + hunks: readonly ReviewHunkSpan[], + target: ReviewRangeTargetV1, +): ReviewRangeCoverageIssue | undefined { + const preferredRange = target.preferred.side === "old" ? target.oldRange : target.newRange; + if ( + !preferredRange || + target.preferred.line < preferredRange[0] || + target.preferred.line > preferredRange[1] + ) { + return "preferred"; + } + if (!target.oldRange && !target.newRange) { + return "empty"; + } + if (target.oldRange && !reviewRangeCoveredByHunks(hunks, "old", target.oldRange)) { + return "old"; + } + if (target.newRange && !reviewRangeCoveredByHunks(hunks, "new", target.newRange)) { + return "new"; + } + return undefined; +} + /** Find the first hunk whose extent on one side covers one line, or -1. */ export function reviewHunkIndexForLine( hunks: readonly ReviewHunkSpan[], diff --git a/src/core/review/intents.test.ts b/src/core/review/intents.test.ts index 05737999c..37e28bc45 100644 --- a/src/core/review/intents.test.ts +++ b/src/core/review/intents.test.ts @@ -276,6 +276,21 @@ describe("user note creation", () => { ).toThrow(ReviewIntentPlanningError); }); + test("rejects saving after reconciliation changes the draft file's content", () => { + const reloaded = reduceReviewState(stateWithDraft("body"), { + type: "document/reconcile", + document: createTestReviewDocument([ + { key: "alpha", contentIdentity: "content:alpha:changed" }, + { key: "beta" }, + ]), + }); + + expect(reloaded.draftNote).toBeNull(); + expect(() => + planReviewIntent(reloaded, { type: "notes/create-user", consumeDraft: true }, FACTS), + ).toThrow(new ReviewIntentPlanningError("draft-missing", "No user note draft is active.")); + }); + test("rejects a draft anchored to a hunk the file no longer has", () => { const reloaded = reduceReviewState(stateWithDraft("body"), { type: "document/reconcile", @@ -334,6 +349,44 @@ describe("user note editing", () => { expect(next.userNotes[0]?.note.anchor).toEqual(original.note.anchor); }); + test("preserves a multiline anchor and preferred endpoint through editing", () => { + const original = createTestStoredNote({ + id: "user-range", + fileKey: "alpha", + source: "user", + editable: true, + }); + original.note.anchor = { + oldRange: [1, 2], + newRange: [11, 12], + preferred: { side: "new", line: 12 }, + intersectingHunkIndices: [0, 1], + ownerHunkIndex: 1, + }; + const state = { ...createTestReviewState(), userNotes: [original] }; + const started = planReviewIntent( + state, + { type: "notes/start-edit", noteId: original.note.id }, + { draftId: "draft:range-edit" }, + ).actions.reduce(reduceReviewState, state); + + expect(started.draftNote).toMatchObject({ + targetKind: "range", + hunkIndex: 1, + side: "new", + line: 12, + anchor: original.note.anchor, + }); + const written = reduceReviewState(started, { type: "draft/update", body: "edited range" }); + const saved = planReviewIntent( + written, + { type: "notes/update-user", noteId: original.note.id, consumeDraft: true }, + { timestamp: FACTS.timestamp }, + ).actions.reduce(reduceReviewState, written); + + expect(saved.userNotes[0]?.note.anchor).toEqual(original.note.anchor); + }); + test("reopens retained notes after reload clamps away their former hunk", () => { const original = createTestStoredNote({ id: "user-1", @@ -416,6 +469,42 @@ describe("threaded replies", () => { plan.outcome?.type === "notes/created" ? plan.outcome.note.note.anchor : undefined, ).toEqual(parent.note.anchor); }); + + test("preserves a multiline parent anchor through a nested reply", () => { + const parent = createTestStoredNote({ id: "live-range", fileKey: "alpha" }); + parent.note.anchor = { + oldRange: [1, 2], + newRange: [11, 12], + preferred: { side: "new", line: 12 }, + intersectingHunkIndices: [0, 1], + ownerHunkIndex: 1, + }; + const state = { ...createTestReviewState(), liveNotes: [parent] }; + const started = planReviewIntent( + state, + { type: "notes/start-reply", noteId: parent.note.id }, + { draftId: "draft:range-reply" }, + ).actions.reduce(reduceReviewState, state); + + expect(started.draftNote).toMatchObject({ + targetKind: "range", + hunkIndex: 1, + side: "new", + line: 12, + anchor: parent.note.anchor, + }); + const written = reduceReviewState(started, { type: "draft/update", body: "range reply" }); + const saved = planReviewIntent( + written, + { type: "notes/create-user", consumeDraft: true }, + FACTS, + ); + + expect(saved.outcome).toMatchObject({ + type: "notes/created", + note: { note: { parentId: parent.note.id, anchor: parent.note.anchor } }, + }); + }); }); describe("note removal", () => { @@ -575,6 +664,13 @@ describe("notes/start-draft", () => { // The second test hunk starts at line 11 with one context line before the change. side: "new", line: 12, + targetKind: "line", + anchor: { + newRange: [12, 12], + preferred: { side: "new", line: 12 }, + intersectingHunkIndices: [1], + ownerHunkIndex: 1, + }, body: "", }, }, @@ -594,6 +690,13 @@ describe("notes/start-draft", () => { hunkIndex: 1, side: "new", line: 12, + targetKind: "line", + anchor: { + newRange: [12, 12], + preferred: { side: "new", line: 12 }, + intersectingHunkIndices: [1], + ownerHunkIndex: 1, + }, body: "", }, }); @@ -614,11 +717,124 @@ describe("notes/start-draft", () => { expect(plan.actions[0]).toMatchObject({ type: "draft/start", - draft: { side: "old", line: 2 }, + draft: { side: "old", line: 2, targetKind: "line" }, }); expect(plan.actions[1]).toMatchObject({ reveal: { anchor: "none", scrollToNote: false } }); }); + test("preserves a legacy line target in an expanded gap with its source authority", () => { + const plan = planReviewIntent( + createTestReviewState([ + { key: "alpha", sourceIdentity: "source:alpha", sourceAttested: true }, + ]), + { + type: "notes/start-draft", + fileKey: "alpha", + hunkIndex: 1, + target: { side: "new", line: 7 }, + }, + { draftId: "draft:gap-line" }, + ); + + expect(plan.outcome).toMatchObject({ + type: "notes/draft-started", + draft: { + targetKind: "line", + side: "new", + line: 7, + expandedLineSource: { + sourceIdentity: "source:alpha", + sourceAttested: true, + }, + anchor: { newRange: [7, 7], ownerHunkIndex: 1 }, + }, + }); + }); + + test("does not attach source authority to an ordinary patch line", () => { + const plan = planReviewIntent( + createTestReviewState([ + { key: "alpha", sourceIdentity: "source:alpha", sourceAttested: true }, + ]), + { + type: "notes/start-draft", + fileKey: "alpha", + hunkIndex: 0, + target: { side: "new", line: 2 }, + }, + { draftId: "draft:patch-line" }, + ); + + expect(plan.outcome?.type).toBe("notes/draft-started"); + if (plan.outcome?.type !== "notes/draft-started") throw new Error("draft not started"); + expect(plan.outcome.draft.expandedLineSource).toBeUndefined(); + }); + + test("rejects a range target that includes a line omitted from the patch", () => { + expect(() => + planReviewIntent( + createTestReviewState(), + { + type: "notes/start-draft", + fileKey: "alpha", + hunkIndex: 1, + target: { newRange: [7, 7], preferred: { side: "new", line: 7 } }, + }, + { draftId: "draft:gap-range" }, + ), + ).toThrow("Review range target is not covered by the current patch (new)."); + }); + + test("rejects a covered range whose requested hunk is not its resolved owner", () => { + expect(() => + planReviewIntent( + createTestReviewState(), + { + type: "notes/start-draft", + fileKey: "alpha", + hunkIndex: 0, + target: { newRange: [11, 13], preferred: { side: "new", line: 12 } }, + }, + { draftId: "draft:wrong-owner" }, + ), + ).toThrow("Review range resolves to hunk 1, not requested hunk 0."); + }); + + test("retains a multiline range unchanged through draft creation and save", () => { + const initial = createTestReviewState(); + const startPlan = planReviewIntent( + initial, + { + type: "notes/start-draft", + fileKey: "alpha", + hunkIndex: 1, + target: { newRange: [11, 13], preferred: { side: "new", line: 13 } }, + }, + { draftId: "draft:range" }, + ); + if (startPlan.outcome?.type !== "notes/draft-started") throw new Error("draft not started"); + const started = startPlan.outcome; + expect(started.draft.targetKind).toBe("range"); + expect(started.draft.anchor).toEqual({ + newRange: [11, 13], + preferred: { side: "new", line: 13 }, + intersectingHunkIndices: [1], + ownerHunkIndex: 1, + }); + + const withDraft = { + ...initial, + draftNote: { ...started.draft, body: "Range feedback" }, + }; + const saved = planReviewIntent( + withDraft, + { type: "notes/create-user", consumeDraft: true }, + { noteId: "user:range", timestamp: "2026-01-01T00:00:00.000Z" }, + ); + if (saved.outcome?.type !== "notes/created") throw new Error("note not created"); + expect(saved.outcome.note.note.anchor).toEqual(started.draft.anchor!); + }); + test("requires the caller to own the draft's identity", () => { expect(() => planReviewIntent(createTestReviewState(), { diff --git a/src/core/review/intents.ts b/src/core/review/intents.ts index 1c9da0bb8..d030746cf 100644 --- a/src/core/review/intents.ts +++ b/src/core/review/intents.ts @@ -10,9 +10,13 @@ * live-agent note lifecycle still resolve at their current owners. */ import type { ReviewAction } from "./actions"; -import { reviewLineAnchor } from "./anchors"; +import { reviewLineAnchor, reviewRangeAnchor } from "./anchors"; import { reviewExpansionSide, reviewGapAddress, reviewGapSourceForFile } from "./expansion"; -import { reviewDefaultHunkLineTarget } from "./geometry"; +import { + reviewDefaultHunkLineTarget, + reviewHunkIndexForLine, + reviewRangeTargetCoverageIssue, +} from "./geometry"; import { reviewNoteWithinSizeLimit } from "./noteSize"; import { EMPTY_REVIEW_ANNOTATION_INDEX, @@ -41,7 +45,14 @@ import { type ReviewStoredNote, } from "./state"; import type { ReviewStore } from "./store"; -import type { ReviewFileV1, ReviewLineAddressV1, ReviewLineRange, ReviewSide } from "./types"; +import type { + ReviewFileV1, + ReviewLineAddressV1, + ReviewLineRange, + ReviewNoteTargetV1, + ReviewRangeTargetV1, + ReviewSide, +} from "./types"; /** * The facts core refuses to invent, supplied by whoever submits an intent. @@ -91,7 +102,7 @@ export type ReviewIntent = type: "notes/start-draft"; fileKey: string; hunkIndex: number; - target?: ReviewLineAddressV1; + target?: ReviewNoteTargetV1; reveal?: ReviewRevealRequest; } /** Open an editable reviewer note in the shared composer. */ @@ -267,7 +278,8 @@ export type ReviewIntentPlanningErrorCode = | "invalid-note-parent" | "blank-note" | "note-too-large" - | "missing-fact"; + | "missing-fact" + | "invalid-request"; /** Typed semantic rejection raised before any review state is reduced or published. */ export class ReviewIntentPlanningError extends Error { @@ -394,18 +406,51 @@ function planDraftStart( requireHunk(file, intent.hunkIndex); const hunk = file.hunks[intent.hunkIndex]!; // Where a note about the whole hunk belongs is one shared answer; a caller that - // measured a specific line the reviewer put a cursor on overrides it. + // measured a specific line or range the reviewer selected overrides it. const target = intent.target ?? reviewDefaultHunkLineTarget(hunk); if (state.draftNote) { throw new ReviewIntentPlanningError("draft-active", "A review note draft is already active."); } + const rangeTarget = "preferred" in target ? (target as ReviewRangeTargetV1) : null; + const lineTarget = rangeTarget ? null : (target as ReviewLineAddressV1); + if (rangeTarget) { + const coverageIssue = reviewRangeTargetCoverageIssue(file.hunks, rangeTarget); + if (coverageIssue) { + throw new ReviewIntentPlanningError( + "invalid-request", + `Review range target is not covered by the current patch (${coverageIssue}).`, + ); + } + } + const preferred = rangeTarget?.preferred ?? lineTarget!; + const anchor = rangeTarget + ? reviewRangeAnchor(file.hunks, { ...rangeTarget, hunkIndex: intent.hunkIndex }) + : reviewLineAnchor(file.hunks, { ...lineTarget!, hunkIndex: intent.hunkIndex }); + if (rangeTarget && anchor.ownerHunkIndex !== intent.hunkIndex) { + throw new ReviewIntentPlanningError( + "invalid-request", + `Review range resolves to hunk ${anchor.ownerHunkIndex ?? "none"}, not requested hunk ${intent.hunkIndex}.`, + ); + } + const expandedLineTarget = + lineTarget !== null && reviewHunkIndexForLine(file.hunks, lineTarget.side, lineTarget.line) < 0; const draft: ReviewDraftNote = { kind: "create", id: requireFact(facts.draftId, "draftId"), fileKey: file.key, - hunkIndex: intent.hunkIndex, - side: target.side, - line: target.line, + hunkIndex: anchor.ownerHunkIndex ?? intent.hunkIndex, + side: preferred.side, + line: preferred.line, + targetKind: rangeTarget ? "range" : "line", + ...(expandedLineTarget + ? { + expandedLineSource: { + ...(file.sourceIdentity !== undefined ? { sourceIdentity: file.sourceIdentity } : {}), + sourceAttested: file.sourceAttested === true, + }, + } + : {}), + anchor, body: "", }; return { @@ -432,13 +477,25 @@ function draftForStoredNote( const file = requireReviewFile(state, entry.note.fileKey); const hunkIndex = reviewNoteCurrentOwnerHunkIndex(entry.note, file); requireHunk(file, hunkIndex); - const target = entry.note.anchor.preferred ?? { side: "new" as const, line: 1 }; + const anchor = entry.note.anchor; + const target = anchor.preferred ?? { side: "new" as const, line: 1 }; + const oldRangeIsMultiline = + anchor.oldRange !== undefined && anchor.oldRange[0] !== anchor.oldRange[1]; + const newRangeIsMultiline = + anchor.newRange !== undefined && anchor.newRange[0] !== anchor.newRange[1]; const common = { id: requireFact(facts.draftId, "draftId"), fileKey: file.key, hunkIndex, side: target.side, line: target.line, + targetKind: + (anchor.oldRange !== undefined && anchor.newRange !== undefined) || + oldRangeIsMultiline || + newRangeIsMultiline + ? ("range" as const) + : ("line" as const), + anchor, }; return mode === "edit" ? { ...common, kind: "edit", targetNoteId: entry.note.id, body: entry.note.summary } @@ -594,7 +651,7 @@ function planUserNoteCreation(state: ReviewState, facts: ReviewIntentFacts): Rev source: "user", originalSource: "user", fileKey: file.key, - anchor: parent ? parent.note.anchor : reviewLineAnchor(file.hunks, draft), + anchor: parent ? parent.note.anchor : (draft.anchor ?? reviewLineAnchor(file.hunks, draft)), summary: draft.body.trim(), author: "user", createdAt: requireFact(facts.timestamp, "timestamp"), diff --git a/src/core/review/reducer.test.ts b/src/core/review/reducer.test.ts index 37439f151..860c8931f 100644 --- a/src/core/review/reducer.test.ts +++ b/src/core/review/reducer.test.ts @@ -12,6 +12,38 @@ function reduceAll(state: ReviewState, ...actions: Parameters { test("clamps the hunk index into the addressed file", () => { const next = reduceReviewState(createTestReviewState([{ key: "alpha", hunkCount: 2 }]), { @@ -185,6 +217,116 @@ describe("document reconciliation", () => { expect(next.expandedGaps).toEqual([]); }); + test("preserves and re-resolves an unchanged expanded-gap line draft", () => { + const next = reduceReviewState(createExpandedLineDraftState(), { + type: "document/reconcile", + document: createTestReviewDocument([ + { key: "alpha", sourceIdentity: "source:alpha", sourceAttested: true }, + { key: "beta" }, + ]), + }); + + expect(next.draftNote).toMatchObject({ + id: "draft-gap", + hunkIndex: 1, + side: "new", + line: 7, + targetKind: "line", + expandedLineSource: { + sourceIdentity: "source:alpha", + sourceAttested: true, + }, + body: "gap feedback", + anchor: { + newRange: [7, 7], + preferred: { side: "new", line: 7 }, + intersectingHunkIndices: [], + ownerHunkIndex: 1, + }, + }); + }); + + test("retires an expanded-gap line draft when source identity or attestation changes", () => { + const changedIdentity = reduceReviewState(createExpandedLineDraftState(), { + type: "document/reconcile", + document: createTestReviewDocument([ + { key: "alpha", sourceIdentity: "source:changed", sourceAttested: true }, + { key: "beta" }, + ]), + }); + const changedAttestation = reduceReviewState(createExpandedLineDraftState(), { + type: "document/reconcile", + document: createTestReviewDocument([ + { key: "alpha", sourceIdentity: "source:alpha", sourceAttested: false }, + { key: "beta" }, + ]), + }); + + expect(changedIdentity.draftNote).toBeNull(); + expect(changedAttestation.draftNote).toBeNull(); + }); + + test("keeps ordinary patch-line and range drafts when only source authority changes", () => { + const state = reduceReviewState( + createTestReviewState([ + { key: "alpha", sourceIdentity: "source:one", sourceAttested: true }, + { key: "beta" }, + ]), + { + type: "draft/start", + draft: { + id: "draft-patch", + fileKey: "alpha", + hunkIndex: 0, + side: "new", + line: 2, + targetKind: "line", + body: "patch feedback", + }, + }, + ); + + const replacementDocument = createTestReviewDocument([ + { key: "alpha", sourceIdentity: "source:two", sourceAttested: false }, + { key: "beta" }, + ]); + const next = reduceReviewState(state, { + type: "document/reconcile", + document: replacementDocument, + }); + const rangeState = reduceReviewState( + createTestReviewState([ + { key: "alpha", sourceIdentity: "source:one", sourceAttested: true }, + { key: "beta" }, + ]), + { + type: "draft/start", + draft: { + id: "draft-range", + fileKey: "alpha", + hunkIndex: 0, + side: "new", + line: 2, + targetKind: "range", + anchor: { + newRange: [1, 3], + preferred: { side: "new", line: 2 }, + intersectingHunkIndices: [0], + ownerHunkIndex: 0, + }, + body: "range feedback", + }, + }, + ); + const rangeNext = reduceReviewState(rangeState, { + type: "document/reconcile", + document: replacementDocument, + }); + + expect(next.draftNote).toMatchObject({ id: "draft-patch", body: "patch feedback" }); + expect(rangeNext.draftNote).toMatchObject({ id: "draft-range", body: "range feedback" }); + }); + test("keeps notes and the active draft across a reload", () => { const state = reduceAll( createTestReviewState(), @@ -196,7 +338,7 @@ describe("document reconciliation", () => { fileKey: "alpha", hunkIndex: 0, side: "new", - line: 4, + line: 2, body: "wip", }, }, @@ -208,7 +350,16 @@ describe("document reconciliation", () => { }); expect(next.liveNotes).toHaveLength(1); - expect(next.draftNote?.body).toBe("wip"); + expect(next.draftNote).toMatchObject({ + body: "wip", + targetKind: "line", + hunkIndex: 0, + anchor: { + newRange: [2, 2], + preferred: { side: "new", line: 2 }, + ownerHunkIndex: 0, + }, + }); }); }); diff --git a/src/core/review/reducer.ts b/src/core/review/reducer.ts index 8b70e58d9..16b4761be 100644 --- a/src/core/review/reducer.ts +++ b/src/core/review/reducer.ts @@ -6,6 +6,8 @@ * Timestamps and ids never originate here — callers put them on the action. */ import type { ReviewAction } from "./actions"; +import { resolveReviewNoteAnchor, reviewGapOwnerHunkIndex, reviewLineAnchor } from "./anchors"; +import { reviewHunkIndexForLine, reviewRangeTargetCoverageIssue } from "./geometry"; import { clamp } from "./navigation"; import { isReviewNoteWithinClearScope, @@ -15,6 +17,7 @@ import { import { applyReviewRevealRequest, reviewRevealIntentsEqual, + type ReviewDraftNote, type ReviewSourceStatus, type ReviewState, type ReviewStoredNote, @@ -45,6 +48,75 @@ function withoutNote(notes: ReviewStoredNote[], noteId: string) { return next; } +/** Preserve and re-resolve a draft only while its addressed content still exists. */ +function reconcileDraftNote( + state: ReviewState, + document: ReviewState["document"], +): ReviewDraftNote | null { + const draft = state.draftNote; + if (!draft) return null; + const previousFile = state.document.files.find((file) => file.key === draft.fileKey); + const file = document.files.find((candidate) => candidate.key === draft.fileKey); + if (!previousFile || !file || previousFile.contentIdentity !== file.contentIdentity) return null; + + const oldRange = draft.anchor?.oldRange; + const newRange = draft.anchor?.newRange; + const legacyRangeDraft = + draft.targetKind === undefined && + ((oldRange !== undefined && newRange !== undefined) || + (oldRange !== undefined && oldRange[0] !== oldRange[1]) || + (newRange !== undefined && newRange[0] !== newRange[1])); + const isRangeDraft = draft.targetKind === "range" || legacyRangeDraft; + + if (!isRangeDraft) { + const wasExpandedLine = reviewHunkIndexForLine(previousFile.hunks, draft.side, draft.line) < 0; + if (wasExpandedLine) { + const source = draft.expandedLineSource; + if ( + !source || + source.sourceIdentity !== file.sourceIdentity || + source.sourceAttested !== (file.sourceAttested === true) + ) { + return null; + } + } + if (!file.hunks[draft.hunkIndex]) return null; + const fallbackOwnerHunkIndex = reviewGapOwnerHunkIndex(file.hunks, draft.side, draft.line); + if (fallbackOwnerHunkIndex === undefined) return null; + const anchor = reviewLineAnchor(file.hunks, { + hunkIndex: fallbackOwnerHunkIndex, + side: draft.side, + line: draft.line, + }); + if (anchor.ownerHunkIndex === undefined) return null; + return { ...draft, targetKind: "line", hunkIndex: anchor.ownerHunkIndex, anchor }; + } + + const preferred = draft.anchor?.preferred ?? { side: draft.side, line: draft.line }; + const target = { + ...(oldRange ? { oldRange } : {}), + ...(newRange ? { newRange } : {}), + preferred, + }; + if (reviewRangeTargetCoverageIssue(file.hunks, target)) return null; + + const anchor = resolveReviewNoteAnchor(file.hunks, { + ...(target.oldRange ? { oldRange: target.oldRange } : {}), + ...(target.newRange ? { newRange: target.newRange } : {}), + preferred: target.preferred, + fallbackOwnerHunkIndex: draft.hunkIndex, + }); + if (anchor.ownerHunkIndex === undefined) return null; + return { + ...draft, + targetKind: "range", + hunkIndex: anchor.ownerHunkIndex, + side: target.preferred.side, + line: target.preferred.line, + anchor, + }; +} + /** Apply one named semantic action without renderer or framework dependencies. */ export function reduceReviewState(state: ReviewState, action: ReviewAction): ReviewState { switch (action.type) { @@ -69,7 +141,14 @@ export function reduceReviewState(state: ReviewState, action: ReviewAction): Rev ([fileKey]) => !retired.has(fileKey) && attested.has(fileKey), ), ); - return { ...state, document: action.document, expandedGaps, sourceStatusByFileKey }; + const draftNote = reconcileDraftNote(state, action.document); + return { + ...state, + document: action.document, + draftNote, + expandedGaps, + sourceStatusByFileKey, + }; } case "selection/select": { const file = selectReviewFileByKey(state, action.fileKey); diff --git a/src/core/review/state.ts b/src/core/review/state.ts index 11770a6fe..f485d9d8d 100644 --- a/src/core/review/state.ts +++ b/src/core/review/state.ts @@ -11,7 +11,13 @@ * they read, so "which notes are visible" or "where does a note hang" gets one named * answer instead of an inline conditional per consumer. */ -import type { ReviewDocumentV1, ReviewLineAddressV1, ReviewNoteV1, ReviewSide } from "./types"; +import type { + ReviewDocumentV1, + ReviewLineAddressV1, + ReviewNoteV1, + ReviewRangeAnchorV1, + ReviewSide, +} from "./types"; export type ReviewNoteResolution = "active" | "stale" | "orphaned"; @@ -143,8 +149,18 @@ interface ReviewDraftNoteBase { id: string; fileKey: string; hunkIndex: number; + /** Preferred placement retained for existing terminal and remote projections. */ side: ReviewSide; line: number; + /** Distinguishes proof-backed legacy lines from ranges that require patch coverage. */ + targetKind?: "line" | "range"; + /** Source authority retained only for a line the patch itself does not cover. */ + expandedLineSource?: { + sourceIdentity?: string; + sourceAttested: boolean; + }; + /** Full resolved anchor retained unchanged when the draft is saved. */ + anchor?: ReviewRangeAnchorV1; body: string; } diff --git a/src/core/review/types.ts b/src/core/review/types.ts index 0efcf44e8..2054f1478 100644 --- a/src/core/review/types.ts +++ b/src/core/review/types.ts @@ -20,6 +20,17 @@ export interface ReviewLineAddressV1 { line: number; } +/** A caller-supplied range before hunk intersections and ownership are resolved. */ +export interface ReviewRangeTargetV1 { + oldRange?: ReviewLineRange; + newRange?: ReviewLineRange; + /** The active endpoint used for placement and owner-hunk resolution. */ + preferred: ReviewLineAddressV1; +} + +/** A note target may address one line or one inclusive source range. */ +export type ReviewNoteTargetV1 = ReviewLineAddressV1 | ReviewRangeTargetV1; + export interface ReviewRangeAnchorV1 { oldRange?: ReviewLineRange; newRange?: ReviewLineRange; diff --git a/src/core/run/commandCatalog.ts b/src/core/run/commandCatalog.ts index 473151191..e648649a4 100644 --- a/src/core/run/commandCatalog.ts +++ b/src/core/run/commandCatalog.ts @@ -34,7 +34,7 @@ import { selectReviewGapForSelection, } from "../review/selectors"; import type { ReviewState } from "../review/state"; -import type { ReviewLineAddressV1 } from "../review/types"; +import type { ReviewNoteTargetV1 } from "../review/types"; /** Where one command's effect resolves, and therefore who may invoke it. */ export type AppCommandLocus = "semantic" | "client-local" | "host-only"; @@ -158,6 +158,30 @@ const BUILTIN_COMMANDS = [ locus: "client-local", publicToExtensions: true, }, + { + id: "hunk.review.startVisualSelection", + title: "Start visual selection", + category: "review", + defaultKeys: ["v"], + locus: "client-local", + publicToExtensions: true, + }, + { + id: "hunk.review.copySelection", + title: "Copy selection", + category: "review", + defaultKeys: ["y"], + locus: "client-local", + publicToExtensions: true, + }, + { + id: "hunk.review.clearSelection", + title: "Clear selection", + category: "review", + defaultKeys: [], + locus: "client-local", + publicToExtensions: true, + }, { id: "hunk.review.startNote", title: "Add a review note", @@ -582,7 +606,7 @@ export interface AppCommandLoweringContext { * knows which line the reviewer's cursor was on. Omitted, the note lands on the shared * default for the whole hunk. */ - noteTarget?: ReviewLineAddressV1; + noteTarget?: ReviewNoteTargetV1; /** * The file and hunk the invoking client's note affordance addressed. * diff --git a/src/extension-api/types.ts b/src/extension-api/types.ts index 7367b3eb6..a275310e5 100644 --- a/src/extension-api/types.ts +++ b/src/extension-api/types.ts @@ -1871,6 +1871,10 @@ export interface ExtensionReviewNote { hunkIndex: number; side: "old" | "new"; line: number; + /** Inclusive old-side source range when the note covers more than one line. */ + oldRange?: readonly [number, number]; + /** Inclusive new-side source range when the note covers more than one line. */ + newRange?: readonly [number, number]; body: string; /** True while the note is still being composed rather than saved. */ draft: boolean; diff --git a/src/session/reviewProtocol.test.ts b/src/session/reviewProtocol.test.ts index 238f2c76f..f8b4af54f 100644 --- a/src/session/reviewProtocol.test.ts +++ b/src/session/reviewProtocol.test.ts @@ -116,6 +116,80 @@ describe("review action round trip", () => { }); }); +describe("range note targets", () => { + test("preserves an exact multiline target on the wire", () => { + const action = { + type: "notes/start-draft", + fileKey: FILE_KEY, + hunkIndex: 1, + target: { newRange: [5, 8], preferred: { side: "new", line: 8 } }, + }; + const parsed = parseHunkReviewAction(action); + expect(parsed.ok && parsed.value).toEqual(action as never); + expect(parsed.ok && toReviewIntent(parsed.value)).toEqual(action as never); + }); + + test("preserves old and new ranges together", () => { + const action = { + type: "notes/start-draft", + fileKey: FILE_KEY, + hunkIndex: 1, + target: { + oldRange: [5, 7], + newRange: [5, 8], + preferred: { side: "new", line: 8 }, + }, + }; + const parsed = parseHunkReviewAction(action); + expect(parsed.ok && parsed.value).toEqual(action as never); + expect(parsed.ok && toReviewIntent(parsed.value)).toEqual(action as never); + }); + + test("requires identity on exact range save preconditions", () => { + const action = { + type: "notes/create-user", + consumeDraft: true, + fileKey: FILE_KEY, + hunkIndex: 1, + target: { oldRange: [5, 8], preferred: { side: "old", line: 5 } }, + }; + const parsed = parseHunkReviewAction(action); + expect(parsed.ok && parsed.value).toEqual(action as never); + expect( + parseHunkReviewAction({ + type: "notes/create-user", + consumeDraft: true, + target: action.target, + }).ok, + ).toBe(false); + expect( + parseHunkReviewAction({ + type: "notes/create-user", + consumeDraft: true, + fileKey: FILE_KEY, + target: action.target, + }).ok, + ).toBe(false); + }); + + test("rejects inverted ranges and preferred lines outside or opposite the range", () => { + for (const target of [ + { newRange: [8, 5], preferred: { side: "new", line: 8 } }, + { newRange: [5, 8], preferred: { side: "new", line: 9 } }, + { newRange: [5, 8], preferred: { side: "old", line: 5 } }, + ]) { + expect( + parseHunkReviewAction({ + type: "notes/start-draft", + fileKey: FILE_KEY, + hunkIndex: 1, + target, + }).ok, + ).toBe(false); + } + }); +}); + describe("expanded-line proof", () => { const proof = { gapId: "before:1", diff --git a/src/session/reviewProtocol.ts b/src/session/reviewProtocol.ts index 3a131e28f..5ce06aece 100644 --- a/src/session/reviewProtocol.ts +++ b/src/session/reviewProtocol.ts @@ -53,7 +53,13 @@ import { type ReviewPublicationAddress, } from "../core/review/generationOrder"; import type { ReviewRevealAnchor, ReviewRevealRequest } from "../core/review/state"; -import type { ReviewLineAddressV1, ReviewSide } from "../core/review/types"; +import type { + ReviewLineAddressV1, + ReviewLineRange, + ReviewNoteTargetV1, + ReviewRangeTargetV1, + ReviewSide, +} from "../core/review/types"; import { asRecord, hasExactKeys, @@ -166,7 +172,10 @@ interface HunkReviewActionWireFields { * A precondition, not a relocation: the producer rejects the save when the draft has * moved, so two clients cannot silently save each other's drafts. */ - target?: ReviewLineAddressV1; + target?: ReviewNoteTargetV1; + /** Stable file and owner-hunk identity; protocol-v1 clients may omit both. */ + fileKey?: string; + hunkIndex?: number; expandedLineProof?: HunkReviewExpandedLineProofV1; }; } @@ -313,6 +322,45 @@ function parseLineAddress(value: unknown): ReviewLineAddressV1 | undefined { : undefined; } +/** Parse one inclusive 1-based source line range. */ +function parseLineRange(value: unknown): ReviewLineRange | undefined { + return Array.isArray(value) && + value.length === 2 && + isLineNumber(value[0]) && + isLineNumber(value[1]) && + value[0] <= value[1] + ? ([value[0], value[1]] as const) + : undefined; +} + +/** Parse a single-line or range note target without deriving its hunk ownership. */ +function parseNoteTarget(value: unknown): ReviewNoteTargetV1 | undefined { + const line = parseLineAddress(value); + if (line) return line; + + const record = asRecord(value); + if ( + !record || + !hasExactKeys( + record, + keysWith(["preferred"], { oldRange: record.oldRange, newRange: record.newRange }), + ) + ) { + return undefined; + } + const preferred = parseLineAddress(record.preferred); + const oldRange = record.oldRange === undefined ? undefined : parseLineRange(record.oldRange); + const newRange = record.newRange === undefined ? undefined : parseLineRange(record.newRange); + if (!preferred || (!oldRange && !newRange)) return undefined; + if (record.oldRange !== undefined && !oldRange) return undefined; + if (record.newRange !== undefined && !newRange) return undefined; + const preferredRange = preferred.side === "old" ? oldRange : newRange; + if (!preferredRange || preferred.line < preferredRange[0] || preferred.line > preferredRange[1]) { + return undefined; + } + return record as unknown as ReviewRangeTargetV1; +} + /** Parse one expanded-line proof. Its fields are exactly what core resolves it by. */ export function parseHunkReviewExpandedLineProof( value: unknown, @@ -389,12 +437,13 @@ const ACTION_PARSERS: Record) ) && isIdentifier(record.fileKey) && isIndex(record.hunkIndex) && - (record.target === undefined || parseLineAddress(record.target) !== undefined) && + (record.target === undefined || parseNoteTarget(record.target) !== undefined) && (record.reveal === undefined || parseReveal(record.reveal) !== undefined) && (record.expandedLineProof === undefined || parseHunkReviewExpandedLineProof(record.expandedLineProof) !== undefined) && - // A proof is evidence about a line, so it is meaningless without one to be about. - (record.expandedLineProof === undefined || record.target !== undefined), + // Expanded-line proofs remain line-specific; ranges spanning source gaps are not + // remotely writable until the protocol can attest every covered line. + (record.expandedLineProof === undefined || parseLineAddress(record.target) !== undefined), "notes/start-edit": (record) => hasExactKeys(record, keysWith(["type", "noteId"], { reveal: record.reveal })) && isIdentifier(record.noteId) && @@ -413,14 +462,22 @@ const ACTION_PARSERS: Record) record, keysWith(["type", "consumeDraft"], { target: record.target, + fileKey: record.fileKey, + hunkIndex: record.hunkIndex, expandedLineProof: record.expandedLineProof, }), ) && record.consumeDraft === true && - (record.target === undefined || parseLineAddress(record.target) !== undefined) && + (record.target === undefined || parseNoteTarget(record.target) !== undefined) && + (record.target === undefined + ? record.fileKey === undefined && record.hunkIndex === undefined + : (record.fileKey === undefined && + record.hunkIndex === undefined && + parseLineAddress(record.target) !== undefined) || + (isIdentifier(record.fileKey) && isIndex(record.hunkIndex))) && (record.expandedLineProof === undefined || parseHunkReviewExpandedLineProof(record.expandedLineProof) !== undefined) && - (record.expandedLineProof === undefined || record.target !== undefined), + (record.expandedLineProof === undefined || parseLineAddress(record.target) !== undefined), "notes/update-user": (record) => hasExactKeys(record, ["type", "noteId", "consumeDraft"]) && isIdentifier(record.noteId) && @@ -478,7 +535,13 @@ export function toReviewIntent(action: HunkReviewActionV1): ReviewIntent { return intent; } if (action.type === "notes/create-user") { - const { expandedLineProof: _proof, target: _target, ...intent } = action; + const { + expandedLineProof: _proof, + target: _target, + fileKey: _fileKey, + hunkIndex: _hunkIndex, + ...intent + } = action; return intent; } return action; diff --git a/src/ui/App.extension-command-controls.test.tsx b/src/ui/App.extension-command-controls.test.tsx index cb89d00d4..eb201f804 100644 --- a/src/ui/App.extension-command-controls.test.tsx +++ b/src/ui/App.extension-command-controls.test.tsx @@ -36,7 +36,7 @@ describe("extension command control authority", () => { const initial = createBootstrap(); initial.extensions!.registry.commands.push({ extensionId: "probe", - command: { id: "capture", title: "Capture controls", key: "y" }, + command: { id: "capture", title: "Capture controls", key: "Y" }, handler(ctx) { capturedControls = ctx.commands; capturedWorkspace = ctx.workspace; @@ -78,7 +78,7 @@ describe("extension command control authority", () => { try { await flush(setup); await act(async () => { - await setup.mockInput.typeText("y"); + await setup.mockInput.typeText("Y"); }); await flush(setup); expect(capturedControls).not.toBeNull(); diff --git a/src/ui/App.tsx b/src/ui/App.tsx index ac62e8205..8cf780fc4 100644 --- a/src/ui/App.tsx +++ b/src/ui/App.tsx @@ -42,7 +42,7 @@ import { ConfirmDialog, confirmDialogHeight } from "./components/chrome/ConfirmD import { ExtensionDialog } from "./components/chrome/ExtensionDialog"; import { ExtensionToast } from "./components/chrome/ExtensionToast"; import { StatusBar } from "./components/chrome/StatusBar"; -import { DiffPane } from "./components/panes/DiffPane"; +import { DiffPane, type ReviewSelectionActionsHandle } from "./components/panes/DiffPane"; import { ExtensionPaneHost } from "./components/panes/ExtensionPane"; import { PaneDivider } from "./components/panes/PaneDivider"; import { @@ -203,6 +203,7 @@ export function App({ const wrapToggleScrollTopRef = useRef(null); const layoutToggleScrollTopRef = useRef(null); const cancelCopySelectionRef = useRef<(() => void) | null>(null); + const selectionActionsRef = useRef(null); const [layoutToggleRequestId, setLayoutToggleRequestId] = useState(0); const [scrollEdgeRequest, setScrollEdgeRequest] = useState<{ id: number; @@ -274,6 +275,13 @@ export function App({ ], ); const filteredFiles = review.visibleFiles; + const semanticFileIdentities = useMemo( + () => + filteredFiles.map( + (file) => review.semanticFileIdentityByFileId.get(file.id) ?? `runtime:${file.id}`, + ), + [filteredFiles, review.semanticFileIdentityByFileId], + ); const selectedFile = review.selectedFile; const selectedHunkIndex = review.selectedHunkIndex; const selectedFileId = selectedFile?.id ?? null; @@ -636,6 +644,8 @@ export function App({ }); const diffPaneWidth = paneLayout.reviewBounds.width; const diffPaneHeight = paneLayout.reviewBounds.height; + // Diff content leaves two outer columns: the first carries the annotation range rail and the + // second remains safety space beside the pane edge. Neither belongs to copy or wrap geometry. const diffContentWidth = Math.max(0, diffPaneWidth - 2); // Publish the live note geometry for daemon-driven markup validation; the // note markup width mirrors what AgentInlineNote lays STML out at. @@ -762,6 +772,7 @@ export function App({ /** Step one line: move the current line, or scroll the viewport when there is no marker. */ const stepDiffLine = (delta: number) => { + if (selectionActionsRef.current?.move(delta)) return; if (!activeLineCursor) { scrollDiff(delta, "step"); return; @@ -1039,7 +1050,13 @@ export function App({ stepDiffLine, selectCursorLine: setCursorLine, selectLayoutMode, - startUserNote: () => startUserNote(), + hasVisualSelection: () => selectionActionsRef.current?.hasSelection() ?? false, + startVisualSelection: () => selectionActionsRef.current?.beginKeyboardSelection(), + copySelection: () => selectionActionsRef.current?.copy(), + clearSelection: () => selectionActionsRef.current?.clear(), + startUserNote: () => { + if (!selectionActionsRef.current?.comment()) startUserNote(); + }, toggleAgentNotes, toggleCopyDecorations, toggleFocusArea, @@ -1122,6 +1139,7 @@ export function App({ closeThemeSelector, closeExtensionTrustPrompt, commands: appCommands, + clearVisualSelection: () => selectionActionsRef.current?.clear() ?? false, denyRepoExtensions, extensionDialog, acceptExtensionDialog, @@ -1297,6 +1315,13 @@ export function App({ endPaneResize(event); closeMenu(); cancelCopySelectionRef.current?.(); + const reviewLeft = bodyPadding / 2 + paneLayout.reviewBounds.x; + const outsideReview = + event.x < reviewLeft || + event.x >= reviewLeft + diffPaneWidth || + event.y < diffPaneScreenTop || + event.y >= diffPaneScreenTop + diffPaneHeight; + if (outsideReview) selectionActionsRef.current?.clear(); }} > {paneLayout.panes.map(renderPane)} @@ -1312,12 +1337,14 @@ export function App({ > {\n` + + ` hunk.registerCommand({ id: "ask", title: "Ask", key: "Y" }, async (ctx) => {\n` + ` const answer = await ${askSource};\n` + ` appendFileSync(${JSON.stringify(logPath)}, "answer " + String(answer) + "\\n");\n` + ` });\n` + @@ -235,7 +235,7 @@ describe("extension dialogs", () => { ); await act(async () => { - await setup.mockInput.typeText("y"); + await setup.mockInput.typeText("Y"); }); await flushUntil( setup, @@ -291,7 +291,7 @@ describe("extension dialogs", () => { bootstrap, async (setup) => { await act(async () => { - await setup.mockInput.typeText("y"); + await setup.mockInput.typeText("Y"); }); await flushUntil( setup, @@ -325,7 +325,7 @@ describe("extension dialogs", () => { ); await act(async () => { - await setup.mockInput.typeText("y"); + await setup.mockInput.typeText("Y"); }); await flushUntil( setup, @@ -364,7 +364,7 @@ describe("extension dialogs", () => { ); await act(async () => { - await setup.mockInput.typeText("y"); + await setup.mockInput.typeText("Y"); }); await flushUntil( setup, @@ -404,7 +404,7 @@ describe("extension dialogs", () => { const bootstrap = await launchWithExtension(repo, extPath); await withAppHost(bootstrap, async (setup) => { await act(async () => { - await setup.mockInput.typeText("y"); + await setup.mockInput.typeText("Y"); }); await flushUntil( setup, @@ -445,7 +445,7 @@ describe("extension dialogs", () => { ); await act(async () => { - await setup.mockInput.typeText("y"); + await setup.mockInput.typeText("Y"); }); await flushUntil( setup, @@ -497,7 +497,7 @@ describe("extension dialogs", () => { ); await act(async () => { - await setup.mockInput.typeText("y"); + await setup.mockInput.typeText("Y"); }); await flushUntil( setup, diff --git a/src/ui/AppHost.extension-navigation.test.tsx b/src/ui/AppHost.extension-navigation.test.tsx index a75a067c9..e9ad08a02 100644 --- a/src/ui/AppHost.extension-navigation.test.tsx +++ b/src/ui/AppHost.extension-navigation.test.tsx @@ -150,7 +150,7 @@ describe("extension command navigation", () => { ` const label = fileId === fileIds[2] ? "third" : "other";\n` + ` appendFileSync(${JSON.stringify(logPath)}, "selected " + label + " " + hunkIndex + "\\n");\n` + ` });\n` + - ` hunk.registerCommand({ id: "probe", title: "Probe", key: "y" }, (ctx) => {\n` + + ` hunk.registerCommand({ id: "probe", title: "Probe", key: "Y" }, (ctx) => {\n` + ` appendFileSync(${JSON.stringify(logPath)}, "enabled " + ctx.commands.isEnabled("hunk.review.nextHunk") + "\\n");\n` + ` appendFileSync(${JSON.stringify(logPath)}, "own " + ctx.commands.execute("ext.probe") + "\\n");\n` + ` appendFileSync(${JSON.stringify(logPath)}, "move " + ctx.commands.execute("hunk.review.nextHunk", { count: 2 }) + "\\n");\n` + @@ -167,7 +167,7 @@ describe("extension command navigation", () => { "the review to render", ); await act(async () => { - await setup.mockInput.typeText("y"); + await setup.mockInput.typeText("Y"); }); await flushUntil( setup, @@ -201,7 +201,7 @@ describe("extension command navigation", () => { ` });\n` + // An out-of-range index on purpose: the guard clamps it into the // file's real hunk range before it reaches the review controller. - ` hunk.registerCommand({ id: "jump", title: "Jump", key: "y" }, (ctx) => {\n` + + ` hunk.registerCommand({ id: "jump", title: "Jump", key: "Y" }, (ctx) => {\n` + ` ctx.navigation.selectHunk(fileIds[1], 99);\n` + ` });\n` + `}\n`, @@ -216,7 +216,7 @@ describe("extension command navigation", () => { ); await act(async () => { - await setup.mockInput.typeText("y"); + await setup.mockInput.typeText("Y"); }); // `selection_changed` firing for the second file is the whole proof: @@ -242,7 +242,7 @@ describe("extension command navigation", () => { ` hunk.on("selection_changed", ({ fileId }) => {\n` + ` appendFileSync(${JSON.stringify(logPath)}, "selected " + fileId + "\\n");\n` + ` });\n` + - ` hunk.registerCommand({ id: "bogus", title: "Bogus", key: "y" }, (ctx) => {\n` + + ` hunk.registerCommand({ id: "bogus", title: "Bogus", key: "Y" }, (ctx) => {\n` + ` ctx.navigation.selectFile("no-such-file");\n` + ` appendFileSync(${JSON.stringify(logPath)}, "handler-finished\\n");\n` + ` });\n` + @@ -258,7 +258,7 @@ describe("extension command navigation", () => { ); await act(async () => { - await setup.mockInput.typeText("y"); + await setup.mockInput.typeText("Y"); }); await flushUntil( setup, diff --git a/src/ui/AppHost.extension-sidebar.test.tsx b/src/ui/AppHost.extension-sidebar.test.tsx index 82cc7c45b..49651c066 100644 --- a/src/ui/AppHost.extension-sidebar.test.tsx +++ b/src/ui/AppHost.extension-sidebar.test.tsx @@ -187,7 +187,7 @@ describe("extension sidebar views", () => { ` });\n` + ` },\n` + ` });\n` + - ` hunk.registerCommand({ id: "toggle-probe", title: "Toggle probe", key: "y" }, (ctx) => {\n` + + ` hunk.registerCommand({ id: "toggle-probe", title: "Toggle probe", key: "Y" }, (ctx) => {\n` + ` ctx.sidebars.toggle("probe");\n` + ` });\n` + `}\n`, @@ -204,7 +204,7 @@ describe("extension sidebar views", () => { expect(setup.captureCharFrame()).not.toContain("EXTSIDEBAR"); await act(async () => { - await setup.mockInput.typeText("y"); + await setup.mockInput.typeText("Y"); }); await flushUntil( setup, @@ -229,7 +229,7 @@ describe("extension sidebar views", () => { // The same key closes it again. await act(async () => { - await setup.mockInput.typeText("y"); + await setup.mockInput.typeText("Y"); }); await flushUntil( setup, @@ -290,7 +290,7 @@ describe("extension sidebar views", () => { extPath, `import { appendFileSync } from "node:fs";\n` + `export default function (hunk) {\n` + - ` hunk.registerCommand({ id: "probe", title: "Probe selection", key: "y" }, (ctx) => {\n` + + ` hunk.registerCommand({ id: "probe", title: "Probe selection", key: "Y" }, (ctx) => {\n` + ` const file = ctx.selection.file;\n` + ` const line = ctx.selection.currentLine;\n` + ` appendFileSync(\n` + @@ -319,7 +319,7 @@ describe("extension sidebar views", () => { // The review opens on the first file's first hunk, and the handler sees // exactly that without having tracked anything itself. await act(async () => { - await setup.mockInput.typeText("y"); + await setup.mockInput.typeText("Y"); }); await flushUntil( setup, @@ -337,7 +337,7 @@ describe("extension sidebar views", () => { await act(async () => { setup.renderer.keyInput.emit("keypress", createTestKeyEvent({ name: "x", sequence: "x" })); setup.renderer.keyInput.emit("keypress", createTestKeyEvent({ name: "j", sequence: "j" })); - setup.renderer.keyInput.emit("keypress", createTestKeyEvent({ name: "y", sequence: "y" })); + setup.renderer.keyInput.emit("keypress", createTestKeyEvent({ name: "Y", sequence: "Y" })); }); await flushUntil( setup, @@ -360,7 +360,7 @@ describe("extension sidebar views", () => { // one input flush. The command must receive them as one coherent address. await act(async () => { setup.renderer.keyInput.emit("keypress", createTestKeyEvent({ name: "j", sequence: "j" })); - setup.renderer.keyInput.emit("keypress", createTestKeyEvent({ name: "y", sequence: "y" })); + setup.renderer.keyInput.emit("keypress", createTestKeyEvent({ name: "Y", sequence: "Y" })); }); await flushUntil( setup, @@ -382,7 +382,7 @@ describe("extension sidebar views", () => { extPath, `import { writeFileSync } from "node:fs";\n` + `export default function (hunk) {\n` + - ` hunk.registerCommand({ id: "snapshot", title: "Snapshot review", key: "y" }, (ctx) => {\n` + + ` hunk.registerCommand({ id: "snapshot", title: "Snapshot review", key: "Y" }, (ctx) => {\n` + ` const snapshot = ctx.review.snapshot();\n` + ` writeFileSync(${JSON.stringify(snapshotPath)}, JSON.stringify(snapshot));\n` + ` });\n` + @@ -416,7 +416,7 @@ describe("extension sidebar views", () => { ); await act(async () => { - await setup.mockInput.typeText("y"); + await setup.mockInput.typeText("Y"); }); await flushUntil(setup, () => existsSync(snapshotPath), "the extension snapshot to write"); @@ -454,7 +454,7 @@ describe("extension sidebar views", () => { extPath, `import { appendFileSync } from "node:fs";\n` + `export default function (hunk) {\n` + - ` hunk.registerCommand({ id: "probe", title: "Probe selection", key: "y" }, (ctx) => {\n` + + ` hunk.registerCommand({ id: "probe", title: "Probe selection", key: "Y" }, (ctx) => {\n` + ` appendFileSync(${JSON.stringify(logPath)}, String(ctx.selection.currentLine === null) + "\\n");\n` + ` });\n` + `}\n`, @@ -469,7 +469,7 @@ describe("extension sidebar views", () => { "the review to render with its current-line marker disabled", ); await act(async () => { - await setup.mockInput.typeText("y"); + await setup.mockInput.typeText("Y"); }); await flushUntil( setup, @@ -490,7 +490,7 @@ describe("extension sidebar views", () => { ` id: "probe",\n` + ` component: () => createElement("text", { content: "EXTSIDEBAR" }),\n` + ` });\n` + - ` hunk.registerCommand({ id: "open-probe", title: "Open probe", key: "y" }, (ctx) => {\n` + + ` hunk.registerCommand({ id: "open-probe", title: "Open probe", key: "Y" }, (ctx) => {\n` + ` ctx.sidebars.open("probe");\n` + ` });\n` + `}\n`, @@ -518,7 +518,7 @@ describe("extension sidebar views", () => { // Opening an extension pane reveals only that pane; `s` closed the files // pane rather than hiding one shared area around both pane states. await act(async () => { - await setup.mockInput.typeText("y"); + await setup.mockInput.typeText("Y"); }); await flushUntil( setup, diff --git a/src/ui/AppHost.keybindings.test.tsx b/src/ui/AppHost.keybindings.test.tsx index 1416daf55..6c70a884a 100644 --- a/src/ui/AppHost.keybindings.test.tsx +++ b/src/ui/AppHost.keybindings.test.tsx @@ -271,7 +271,7 @@ describe("user keybindings", () => { const seen: string[] = []; extensions.registry.commands.push({ extensionId: "coach", - command: { id: "toggle-lines", title: "Toggle lines", key: "y" }, + command: { id: "toggle-lines", title: "Toggle lines", key: "Y" }, handler: (ctx) => { ctx.commands.execute("hunk.view.toggleLineNumbers"); }, @@ -286,7 +286,7 @@ describe("user keybindings", () => { await withAppHost(bootstrap, async (setup) => { await act(async () => { - await setup.mockInput.typeText("y"); + await setup.mockInput.typeText("Y"); }); await flush(setup); expect(seen).toEqual(["hunk.view.toggleLineNumbers", "coach.toggle-lines"]); diff --git a/src/ui/AppHost.selection.test.tsx b/src/ui/AppHost.selection.test.tsx index 7ce52e12c..9fd51f1b5 100644 --- a/src/ui/AppHost.selection.test.tsx +++ b/src/ui/AppHost.selection.test.tsx @@ -72,6 +72,12 @@ async function flush(setup: Harness) { }); } +/** Invoke the explicit Copy Selection action after a gesture commits its range. */ +async function copyCommittedSelection(setup: Harness) { + await act(async () => setup.mockInput.pressKey("y")); + await flush(setup); +} + /** Poll rendered frames until `predicate` matches, resilient to async repaints. */ async function waitForFrame( setup: Harness, @@ -90,10 +96,9 @@ async function waitForFrame( }); frame = setup.captureCharFrame(); } - // Surface the timeout so a follow-up assertion failure points at the unmet condition - // rather than a generic "string does not contain" message during flake investigations. - console.warn(`waitForFrame: "${description}" never matched after ${attempts} attempts`); - return frame; + throw new Error( + `Timed out waiting for "${description}" after ${attempts} attempts. Last frame:\n${frame}`, + ); } /** Find the screen position of the first occurrence of `needle` in the rendered frame. */ @@ -160,6 +165,7 @@ describe("DiffPane copy selection", () => { await setup.mockMouse.drag(start!.x + 2, start!.y, end!.x + 4, end!.y, MouseButtons.LEFT); }); await flush(setup); + await copyCommittedSelection(setup); // The drag moved across rows, so release copies the rendered text and shows feedback. expect(copied.length).toBeGreaterThan(0); @@ -177,6 +183,57 @@ describe("DiffPane copy selection", () => { } }); + test("vertical scrolling preserves a committed selection for keyboard actions", async () => { + const { setup, copied } = await renderSelectionApp(createSelectionBootstrap()); + + try { + const frame = setup.captureCharFrame(); + const start = locateText(frame, "item01"); + const end = locateText(frame, "item03"); + expect(start).not.toBeNull(); + expect(end).not.toBeNull(); + await act(async () => { + await setup.mockMouse.drag(start!.x + 2, start!.y, end!.x + 4, end!.y, MouseButtons.LEFT); + }); + await flush(setup); + await act(async () => setup.mockInput.pressKey("PAGEDOWN")); + await flush(setup); + await copyCommittedSelection(setup); + + expect(copied.at(-1)).toContain("em01"); + expect(copied.at(-1)).toContain("item02"); + } finally { + await act(async () => setup.renderer.destroy()); + } + }); + + test("a height-only resize clears the committed selection", async () => { + const { setup, copied } = await renderSelectionApp(createSelectionBootstrap()); + + try { + const frame = setup.captureCharFrame(); + const start = locateText(frame, "item01"); + const end = locateText(frame, "item03"); + expect(start).not.toBeNull(); + expect(end).not.toBeNull(); + await act(async () => { + await setup.mockMouse.drag(start!.x + 2, start!.y, end!.x + 4, end!.y, MouseButtons.LEFT); + }); + await flush(setup); + expect(setup.captureCharFrame()).toContain("c Comment"); + + await act(async () => setup.resize(110, 24)); + await flush(setup); + await act(async () => setup.mockInput.pressKey("y")); + await flush(setup); + + expect(copied).toEqual([]); + expect(setup.captureCharFrame()).not.toContain("c Comment"); + } finally { + await act(async () => setup.renderer.destroy()); + } + }); + test("mouse-up extends a copy through drag events coalesced by the terminal host", async () => { const { setup, copied } = await renderSelectionApp(createSelectionBootstrap()); @@ -195,6 +252,7 @@ describe("DiffPane copy selection", () => { await setup.mockMouse.release(end!.x + 8, end!.y, MouseButtons.LEFT); }); await flush(setup); + await copyCommittedSelection(setup); expect(copied.length).toBeGreaterThan(0); expect(copied.at(-1)).toContain("item05"); @@ -230,6 +288,7 @@ describe("DiffPane copy selection", () => { ); }); await flush(setup); + await copyCommittedSelection(setup); // Cell-aware slicing keeps the copy aligned with the drag: without it, each full-width // character shifted the endpoint and the clipboard over-included "'; //". @@ -254,6 +313,7 @@ describe("DiffPane copy selection", () => { await setup.mockMouse.doubleClick(target!.x + 2, target!.y, MouseButtons.LEFT); }); await flush(setup); + await copyCommittedSelection(setup); expect(copied.length).toBeGreaterThan(0); // Word expansion copies a single contiguous token, not a whole multi-token line. @@ -280,6 +340,7 @@ describe("DiffPane copy selection", () => { await setup.mockMouse.click(target!.x + 2, target!.y, MouseButtons.LEFT); }); await flush(setup); + await copyCommittedSelection(setup); expect(copied.length).toBeGreaterThan(0); // Line expansion copies the full token sequence including the assignment. @@ -291,6 +352,143 @@ describe("DiffPane copy selection", () => { } }); + test("an invalid metadata overlap shows its disabled Comment reason beside the selection", async () => { + const { setup } = await renderSelectionApp(createSelectionBootstrap()); + + try { + const frame = setup.captureCharFrame(); + const header = locateText(frame, "@@"); + const end = locateText(frame, "item03"); + expect(header).not.toBeNull(); + expect(end).not.toBeNull(); + + await act(async () => { + await setup.mockMouse.drag(header!.x, header!.y, end!.x + 4, end!.y, MouseButtons.LEFT); + }); + const invalidFrame = await waitForFrame( + setup, + (text) => + text.includes("Comment requires contiguous") && text.includes("code from one file"), + "selection Comment reason", + ); + expect(invalidFrame).toContain("Comment requires contiguous"); + expect(invalidFrame).toContain("code from one file"); + } finally { + await act(async () => setup.renderer.destroy()); + } + }); + + test("a compact invalid action bar wraps its reason below vertical actions", async () => { + const { setup } = await renderSelectionApp(createSelectionBootstrap(), { + width: 32, + height: 26, + }); + + try { + const frame = setup.captureCharFrame(); + const header = locateText(frame, "@@"); + expect(header).not.toBeNull(); + + await act(async () => { + await setup.mockMouse.drag( + header!.x, + header!.y, + 30, + Math.min(header!.y + 3, 24), + MouseButtons.LEFT, + ); + }); + const invalidFrame = await waitForFrame( + setup, + (text) => text.includes("Comment requires") && text.includes("contiguous code from one"), + "wrapped compact selection reason", + ); + const rows = invalidFrame.split("\n"); + const commentRow = rows.findIndex((row) => row.includes("c Comment")); + const copyRow = rows.findIndex((row) => row.includes("y Copy")); + const clearRow = rows.findIndex((row) => row.includes("Esc Clear")); + expect(copyRow).toBe(commentRow + 1); + expect(clearRow).toBe(copyRow + 1); + expect(rows[clearRow + 1]).toContain("Comment requires"); + expect(rows[clearRow + 2]).toContain("contiguous code"); + expect(rows[clearRow + 3]).toContain("file"); + } finally { + await act(async () => setup.renderer.destroy()); + } + }); + + test("a committed mouse range opens the inline composer with c", async () => { + const { setup } = await renderSelectionApp(createSelectionBootstrap()); + + try { + const frame = setup.captureCharFrame(); + const start = locateText(frame, "item01"); + const end = locateText(frame, "item03"); + expect(start).not.toBeNull(); + expect(end).not.toBeNull(); + + await act(async () => { + await setup.mockMouse.drag(start!.x + 2, start!.y, end!.x + 4, end!.y, MouseButtons.LEFT); + }); + await flush(setup); + await act(async () => setup.mockInput.pressKey("c")); + const composerFrame = await waitForFrame( + setup, + (text) => text.includes("Write a note"), + "range note composer", + ); + expect(composerFrame).toContain("Write a note"); + } finally { + await act(async () => setup.renderer.destroy()); + } + }); + + test("Escape contextually clears an active selection without a global command binding", async () => { + const { setup } = await renderSelectionApp(createSelectionBootstrap()); + + try { + const frame = setup.captureCharFrame(); + const start = locateText(frame, "item01"); + const end = locateText(frame, "item03"); + expect(start).not.toBeNull(); + expect(end).not.toBeNull(); + await act(async () => { + await setup.mockMouse.drag(start!.x + 2, start!.y, end!.x + 4, end!.y, MouseButtons.LEFT); + }); + await flush(setup); + expect(setup.captureCharFrame()).toContain("c Comment"); + + await act(async () => setup.mockInput.pressEscape()); + const clearedFrame = await waitForFrame( + setup, + (text) => !text.includes("c Comment"), + "selection to clear on Escape", + ); + expect(clearedFrame).not.toContain("c Comment"); + } finally { + await act(async () => setup.renderer.destroy()); + } + }); + + test("keyboard visual selection extends with line navigation and copies with y", async () => { + const { setup, copied } = await renderSelectionApp(createSelectionBootstrap()); + + try { + await act(async () => { + await setup.mockInput.pressKey("v"); + await setup.mockInput.pressKey("j"); + await setup.mockInput.pressKey("j"); + await setup.mockInput.pressKey("y"); + }); + await flush(setup); + + expect(copied.length).toBeGreaterThan(0); + expect(copied.at(-1)).toContain("item"); + } finally { + await act(async () => setup.renderer.destroy()); + } + }); + test("a press and release without movement does not copy anything", async () => { const { setup, copied } = await renderSelectionApp(createSelectionBootstrap()); @@ -335,6 +533,7 @@ describe("DiffPane copy selection", () => { ); }); await flush(setup); + await copyCommittedSelection(setup); expect(copied.length).toBeGreaterThan(0); } finally { @@ -409,6 +608,7 @@ describe("DiffPane copy selection", () => { await setup.mockMouse.drag(start!.x + 2, start!.y, end!.x + 4, end!.y, MouseButtons.LEFT); }); await flush(setup); + await copyCommittedSelection(setup); // The drag still resolves a selection, but the unsupported terminal falls back to a notice. expect(copied.length).toBe(0); @@ -446,6 +646,7 @@ describe("DiffPane copy selection", () => { ); }); await flush(setup); + await copyCommittedSelection(setup); // Dragging from the pinned header into the body still produces a copied selection. expect(copied.length).toBeGreaterThan(0); @@ -472,6 +673,7 @@ describe("DiffPane copy selection", () => { await setup.mockMouse.drag(start!.x + 2, start!.y, end!.x + 2, end!.y, MouseButtons.LEFT); }); await flush(setup); + await copyCommittedSelection(setup); expect(copied.length).toBeGreaterThan(0); } finally { diff --git a/src/ui/AppHost.workspace.test.tsx b/src/ui/AppHost.workspace.test.tsx index dee65d146..200d75ec9 100644 --- a/src/ui/AppHost.workspace.test.tsx +++ b/src/ui/AppHost.workspace.test.tsx @@ -142,7 +142,7 @@ function writeWorkspaceFixture(extPath: string, logPath: string) { extPath, `import { appendFileSync } from "node:fs";\n` + `export default function (hunk) {\n` + - ` hunk.registerCommand({ id: "rewrite", title: "Rewrite", key: "y" }, async (ctx) => {\n` + + ` hunk.registerCommand({ id: "rewrite", title: "Rewrite", key: "Y" }, async (ctx) => {\n` + ` const file = ctx.selection.file;\n` + ` if (!file) return;\n` + ` const log = (line) => appendFileSync(${JSON.stringify(logPath)}, line + "\\n");\n` + @@ -163,7 +163,7 @@ function writeReadFixture(extPath: string, logPath: string) { extPath, `import { appendFileSync } from "node:fs";\n` + `export default function (hunk) {\n` + - ` hunk.registerCommand({ id: "read", title: "Read", key: "y" }, async (ctx) => {\n` + + ` hunk.registerCommand({ id: "read", title: "Read", key: "Y" }, async (ctx) => {\n` + ` const file = ctx.selection.file;\n` + ` if (!file) return;\n` + ` const log = (line) => appendFileSync(${JSON.stringify(logPath)}, line + "\\n");\n` + @@ -185,7 +185,7 @@ function writeReadWriteFixture(extPath: string, logPath: string) { extPath, `import { appendFileSync } from "node:fs";\n` + `export default function (hunk) {\n` + - ` hunk.registerCommand({ id: "shout", title: "Shout", key: "y" }, async (ctx) => {\n` + + ` hunk.registerCommand({ id: "shout", title: "Shout", key: "Y" }, async (ctx) => {\n` + ` const file = ctx.selection.file;\n` + ` if (!file) return;\n` + ` const log = (line) => appendFileSync(${JSON.stringify(logPath)}, line + "\\n");\n` + @@ -314,7 +314,7 @@ describe("extension workspace reads", () => { extPath, `import { appendFileSync } from "node:fs";\n` + `export default function (hunk) {\n` + - ` hunk.registerCommand({ id: "read", title: "Read", key: "y" }, async (ctx) => {\n` + + ` hunk.registerCommand({ id: "read", title: "Read", key: "Y" }, async (ctx) => {\n` + ` const file = ctx.selection.file;\n` + ` if (!file) return;\n` + ` const text = await ctx.workspace.readDocument(file.id, "new");\n` + @@ -348,7 +348,7 @@ describe("extension workspace reads", () => { await withAppHost( bootstrap, async (setup) => { - await act(async () => setup.mockInput.typeText("y")); + await act(async () => setup.mockInput.typeText("Y")); await readStarted; let reloadFinished = false; @@ -389,7 +389,7 @@ describe("extension workspace reads", () => { ); await act(async () => { - await setup.mockInput.typeText("y"); + await setup.mockInput.typeText("Y"); }); await flushUntil( setup, @@ -427,7 +427,7 @@ describe("extension workspace reads", () => { ); await act(async () => { - await setup.mockInput.typeText("y"); + await setup.mockInput.typeText("Y"); }); await flushUntil( setup, @@ -465,7 +465,7 @@ describe("extension workspace reads", () => { ); await act(async () => { - await setup.mockInput.typeText("y"); + await setup.mockInput.typeText("Y"); }); await flushUntil( setup, @@ -498,7 +498,7 @@ describe("extension workspace reads", () => { ); await act(async () => { - await setup.mockInput.typeText("y"); + await setup.mockInput.typeText("Y"); }); await flushUntil( setup, @@ -543,7 +543,7 @@ describe("extension workspace writes", () => { ); await act(async () => { - await setup.mockInput.typeText("y"); + await setup.mockInput.typeText("Y"); }); await flushUntil( setup, @@ -598,7 +598,7 @@ describe("extension workspace writes", () => { await withAppHost( bootstrap, async (setup) => { - await act(async () => setup.mockInput.typeText("y")); + await act(async () => setup.mockInput.typeText("Y")); await flushUntil( setup, () => setup.captureCharFrame().includes("Write alpha.txt?"), @@ -654,7 +654,7 @@ describe("extension workspace writes", () => { await withAppHost( bootstrap, async (setup) => { - await act(async () => setup.mockInput.typeText("y")); + await act(async () => setup.mockInput.typeText("Y")); await flushUntil( setup, () => setup.captureCharFrame().includes("Write alpha.txt?"), @@ -705,7 +705,7 @@ describe("extension workspace writes", () => { "the review to render", ); await act(async () => { - await setup.mockInput.typeText("y"); + await setup.mockInput.typeText("Y"); }); await flushUntil( setup, @@ -753,7 +753,7 @@ describe("extension workspace writes", () => { "the review to render", ); await act(async () => { - await setup.mockInput.typeText("y"); + await setup.mockInput.typeText("Y"); }); await flushUntil( setup, @@ -798,7 +798,7 @@ describe("extension workspace writes", () => { ); await act(async () => { - await setup.mockInput.typeText("y"); + await setup.mockInput.typeText("Y"); }); await flushUntil( setup, @@ -842,7 +842,7 @@ describe("extension workspace writes", () => { ); await act(async () => { - await setup.mockInput.typeText("y"); + await setup.mockInput.typeText("Y"); }); await flushUntil( setup, @@ -883,7 +883,7 @@ describe("extension workspace writes", () => { ); await act(async () => { - await setup.mockInput.typeText("y"); + await setup.mockInput.typeText("Y"); }); await flushUntil( setup, diff --git a/src/ui/components/panes/AgentInlineNote.test.tsx b/src/ui/components/panes/AgentInlineNote.test.tsx index b62a54b30..34f7a77f3 100644 --- a/src/ui/components/panes/AgentInlineNote.test.tsx +++ b/src/ui/components/panes/AgentInlineNote.test.tsx @@ -26,6 +26,62 @@ describe("shortReviewNoteAge", () => { }); }); +test("AgentInlineNote connects a ranged card to the external annotation rail", async () => { + const setup = await testRender( + , + { width: 61, height: 5 }, + ); + + try { + await act(async () => { + await setup.renderOnce(); + }); + const top = setup.captureCharFrame().split("\n")[0] ?? ""; + + expect(top).toContain("┬"); + expect(top[60]).toBe("┘"); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } +}); + +test("AgentInlineNote continues an aggregate rail through the card", async () => { + const setup = await testRender( + , + { width: 61, height: 5 }, + ); + + try { + await act(async () => { + await setup.renderOnce(); + }); + const lines = setup.captureCharFrame().split("\n"); + + expect(lines[0]?.[60]).toBe("┤"); + expect(lines.slice(1, 4).every((line) => line[60] === "│")).toBe(true); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } +}); + describe("draftVisualLineCount", () => { const cases: Array<[string, string, number, number]> = [ // [label, text, width, expected rows] diff --git a/src/ui/components/panes/AgentInlineNote.tsx b/src/ui/components/panes/AgentInlineNote.tsx index c22798d0e..d004dd031 100644 --- a/src/ui/components/panes/AgentInlineNote.tsx +++ b/src/ui/components/panes/AgentInlineNote.tsx @@ -190,6 +190,7 @@ export function AgentInlineNote({ layout, noteCount = 1, noteIndex = 0, + rangeGuideConnection, draft, actions, onClose, @@ -204,6 +205,8 @@ export function AgentInlineNote({ layout: Exclude; noteCount?: number; noteIndex?: number; + /** Join the card's top-right corner to the external range rail. */ + rangeGuideConnection?: "terminate" | "continue"; draft?: { body: string; focused: boolean; @@ -280,6 +283,40 @@ export function AgentInlineNote({ width, threadDepth, }); + const rangeConnectorGap = Math.max(0, width - boxLeft - boxWidth); + const renderRangeGuideConnector = () => + rangeGuideConnection ? ( + + + {`${"─".repeat(rangeConnectorGap)}${rangeGuideConnection === "continue" ? "┤" : "┘"}`} + + + ) : null; + const renderRangeGuideContinuation = (height: number) => + rangeGuideConnection === "continue" ? ( + + {Array.from({ length: Math.max(0, height - 1) }, (_, index) => ( + + │ + + ))} + + ) : null; const visualThreadDepth = Math.min(Math.max(0, threadDepth), 3); const connectorWidth = visualThreadDepth * 2; const connectorLeft = Math.max(0, boxLeft - connectorWidth); @@ -466,7 +503,7 @@ export function AgentInlineNote({ if (draft) { const draftVisibleLineCount = draftVisibleRows; const draftTitleText = fitText(` ${titleText} `, Math.max(0, boxWidth - 4)); - const draftTopBorderSuffix = `${"─".repeat(Math.max(0, boxWidth - 3 - draftTitleText.length))}╮`; + const draftTopBorderSuffix = `${"─".repeat(Math.max(0, boxWidth - 3 - draftTitleText.length))}${rangeGuideConnection ? "┬" : "╮"}`; const draftActionItems: BorderActionItem[] = [ { id: "save", keyLabel: "^S", label: "save", onMouseUp: draft.onSave }, { id: "cancel", keyLabel: "Esc", label: "cancel", onMouseUp: draft.onCancel }, @@ -503,7 +540,15 @@ export function AgentInlineNote({ )); return ( - + @@ -525,6 +570,7 @@ export function AgentInlineNote({ + {renderRangeGuideConnector()} {renderDraftBodyPaddingRows("draft-body-top-padding", draftTopPaddingRows)} @@ -628,6 +674,7 @@ export function AgentInlineNote({ {renderBottomBorder(draftActionItems)} + {renderRangeGuideContinuation(draftTextareaRows + 3)} ); } @@ -716,8 +763,18 @@ export function AgentInlineNote({ const bottomBorderInnerWidth = Math.max(0, boxWidth - 2); const showActionOverlay = actionsHovered && savedActionItems.length > 0; + const savedHeight = (markupLines?.length ?? lines.length) + 3; + return ( - + @@ -765,10 +822,11 @@ export function AgentInlineNote({ ) : null} - ╮ + {rangeGuideConnection ? "┬" : "╮"} + {renderRangeGuideConnector()} {renderSavedBodyRow("saved-note-top-padding", "", "summary")} @@ -799,6 +857,7 @@ export function AgentInlineNote({ )} + {renderRangeGuideContinuation(savedHeight)} ); } diff --git a/src/ui/components/panes/DiffPane.tsx b/src/ui/components/panes/DiffPane.tsx index 68c789690..71ddf3ec6 100644 --- a/src/ui/components/panes/DiffPane.tsx +++ b/src/ui/components/panes/DiffPane.tsx @@ -17,7 +17,8 @@ import { DEFAULT_FILE_GAP, DEFAULT_HUNK_GAP } from "../../../core/run/reviewGap" import { DEFAULT_TAB_WIDTH } from "../../../core/run/tabWidth"; import type { DiffFile } from "../../../core/changeset/model"; import type { CursorLine, LayoutMode } from "../../../core/run/commandInputs"; -import type { UserNoteLineTarget } from "../../../core/liveComments"; +import { resolveSplitPaneWidths } from "../../diff/codeColumns"; +import type { ReviewNoteTargetV1 } from "../../../core/review/types"; import type { AgentAnnotation } from "../../../extension-api/types"; import { resolveReviewRevealNoteId } from "../../../core/review/selectors"; import { @@ -47,7 +48,7 @@ import { type CurrentLineAlignment, type LineRevealPlacement, } from "../../lib/hunkScroll"; -import { inlineNoteStableKey } from "../../diff/reviewRenderPlan"; +import { contextLineStableKeySides, inlineNoteStableKey } from "../../diff/reviewRenderPlan"; import { buildLineCursors, clampLineCursorToViewport, @@ -113,8 +114,11 @@ import { findCopySelectionPoint, findLineCursorForClick, normalizeCopySelectionRange, + planSelectionActionBar, + projectCommentSelection, renderCopySelectionText, resolveCopySelectionSide, + selectionInvalidationIdentity, type CopySelectionContext, type CopySelectionDrag, type CopySelectionPoint, @@ -122,6 +126,21 @@ import { } from "./copySelection"; const EMPTY_VISIBLE_AGENT_NOTES: VisibleAgentNote[] = []; +const SELECTION_ACTION_BAR_WIDTH = 34; + +type StartUserNoteAtHunk = { + bivarianceHack(fileId: string, hunkIndex: number, target?: ReviewNoteTargetV1): void; +}["bivarianceHack"]; + +/** Commands App may route to the pane's active persistent selection. */ +export interface ReviewSelectionActionsHandle { + hasSelection: () => boolean; + beginKeyboardSelection: () => boolean; + copy: () => boolean; + comment: () => boolean; + clear: () => boolean; + move: (delta: number) => boolean; +} /** Read terminal-only semantic note metadata without granting it to static sidecars. */ function storedReviewNoteMetadata( @@ -133,6 +152,26 @@ function storedReviewNoteMetadata( : undefined; } +/** Read the semantic placement retained on stored terminal note projections. */ +function storedReviewNoteTarget( + annotation: AgentAnnotation, +): { hunkIndex: number; side: "old" | "new"; line: number } | undefined { + const candidate = annotation as AgentAnnotation & { + hunkIndex?: unknown; + side?: unknown; + line?: unknown; + }; + return Number.isInteger(candidate.hunkIndex) && + (candidate.side === "old" || candidate.side === "new") && + Number.isInteger(candidate.line) + ? { + hunkIndex: candidate.hunkIndex as number, + side: candidate.side, + line: candidate.line as number, + } + : undefined; +} + /** Grant saved-note card actions from semantic ownership rather than presentation labels. */ export function storedReviewNoteActions({ editable, @@ -301,6 +340,7 @@ export function DiffPane({ expandedGapsByFileId = EMPTY_EXPANDED_GAPS_BY_FILE_ID, fileViews = EMPTY_FILE_VIEWS, files, + semanticFileIdentities, offloadLargeDiff = false, lineHighlights = EMPTY_LINE_HIGHLIGHTS, headerLabelWidth, @@ -339,6 +379,7 @@ export function DiffPane({ width, height, cancelCopySelectionRef, + selectionActionsRef, onActiveAddNoteAffordanceChange, onEditUserNote, onReplyToNote, @@ -368,6 +409,8 @@ export function DiffPane({ /** Validated alternate layouts, keyed by file id; raw Pierre remains the fallback. */ fileViews?: ReadonlyMap; files: DiffFile[]; + /** Already-projected semantic identities for selection invalidation. */ + semanticFileIdentities?: readonly string[]; /** Offload eligible syntax highlighting for this launch. */ offloadLargeDiff?: boolean; /** Validated extension line marks, keyed by file id. */ @@ -408,6 +451,7 @@ export function DiffPane({ width: number; height?: number; cancelCopySelectionRef?: RefObject<(() => void) | null>; + selectionActionsRef?: RefObject; onActiveAddNoteAffordanceChange?: ( affordance: (ActiveAddNoteAffordance & { fileId: string }) | null, ) => void; @@ -416,7 +460,7 @@ export function DiffPane({ onRemoveLiveNote?: (noteId: string) => void; onRemoveUserNote?: (noteId: string) => void; onSaveDraftNote?: () => void; - onStartUserNoteAtHunk?: (fileId: string, hunkIndex: number, target?: UserNoteLineTarget) => void; + onStartUserNoteAtHunk?: StartUserNoteAtHunk; onUpdateDraftNote?: (body: string) => void; onBlurDraftNote?: () => void; onCancelDraftNote?: () => void; @@ -490,7 +534,7 @@ export function DiffPane({ const onStartUserNoteAtHunkRef = useRef(onStartUserNoteAtHunk); onStartUserNoteAtHunkRef.current = onStartUserNoteAtHunk; const startUserNoteAtHunkCallbacksRef = useRef( - new Map void>(), + new Map void>(), ); const startUserNoteAtHunkCallback = useCallback((fileId: string) => { let callback = startUserNoteAtHunkCallbacksRef.current.get(fileId); @@ -589,6 +633,7 @@ export function DiffPane({ const notes: VisibleAgentNote[] = annotations.flatMap((annotation, index) => { const source = reviewNoteSource(annotation); const metadata = storedReviewNoteMetadata(annotation); + const storedTarget = metadata ? storedReviewNoteTarget(annotation) : undefined; if ( metadata && draftNote?.kind === "edit" && @@ -622,6 +667,7 @@ export function DiffPane({ annotation, source, editable: source === "user" && annotation.editable === true, + ...(storedTarget ? { target: storedTarget } : {}), ...(metadata ? { thread: { @@ -796,9 +842,24 @@ export function DiffPane({ const [rapidScrollOverscanRows, setRapidScrollOverscanRows] = useState(0); const [hoveredFileId, setHoveredFileId] = useState(null); const [copySelectionDrag, setCopySelectionDrag] = useState(null); - // Mirror the drag state in a ref so updateCopySelection can suppress native selection - // on the very first drag event, before React has re-rendered with the new state. + // Pointer gestures and committed selections are separate: mouse-up retires capture while + // the committed range remains painted and available to Comment/Copy/Clear. const copySelectionDragRef = useRef(null); + const committedCopySelectionRef = useRef(null); + const keyboardSelectionAnchorRef = useRef<{ + top: number; + bottom: number; + side: "old" | "new"; + } | null>(null); + const clearCopySelection = useCallback(() => { + const hadSelection = + copySelectionDragRef.current !== null || committedCopySelectionRef.current !== null; + copySelectionDragRef.current = null; + committedCopySelectionRef.current = null; + keyboardSelectionAnchorRef.current = null; + setCopySelectionDrag(null); + return hadSelection; + }, []); const lastClickTimeRef = useRef(0); const clickCountRef = useRef(0); const lastClickPointRef = useRef(null); @@ -1155,6 +1216,47 @@ export function DiffPane({ scrollRef, totalContentHeight, ]); + + const selectionContentIdentities = useMemo( + () => semanticFileIdentities ?? files.map((file) => file.id), + [files, semanticFileIdentities], + ); + const selectionGeometryKey = useMemo( + () => + selectionInvalidationIdentity({ + layout, + wrapLines, + width: diffContentWidth, + viewportHeight: scrollViewport.height || height || 0, + codeHorizontalOffset, + showLineNumbers, + showHunkHeaders, + fileIdentities: selectionContentIdentities, + rowIdentities: sectionGeometry.flatMap((geometry) => + geometry.rowBounds.map((row) => `${row.key}:${row.height}`), + ), + }), + [ + codeHorizontalOffset, + diffContentWidth, + height, + layout, + scrollViewport.height, + sectionGeometry, + selectionContentIdentities, + showHunkHeaders, + showLineNumbers, + wrapLines, + ], + ); + const previousSelectionGeometryKeyRef = useRef(selectionGeometryKey); + useEffect(() => { + if (previousSelectionGeometryKeyRef.current !== selectionGeometryKey) { + clearCopySelection(); + previousSelectionGeometryKeyRef.current = selectionGeometryKey; + } + }, [clearCopySelection, selectionGeometryKey]); + const fileSectionIndexById = useMemo( () => buildFileSectionIndexById(fileSectionLayouts), [fileSectionLayouts], @@ -1411,6 +1513,178 @@ export function DiffPane({ [onCopyFeedback, onCopySelectionText, renderer], ); + const commentSelection = useMemo( + () => + projectCommentSelection({ + drag: copySelectionDrag, + fileSectionLayouts, + sectionGeometry, + side: copySelectionSide, + }), + [copySelectionDrag, copySelectionSide, fileSectionLayouts, sectionGeometry], + ); + const selectionActionPlacement = useMemo(() => { + if (!copySelectionDrag || committedCopySelectionRef.current === null) return null; + const focusVisualRow = + copySelectionDrag.focus.kind === "review-row" + ? copySelectionDrag.focus.visualRow + : copySelectionDrag.focus.nextVisualRow - 1; + const splitWidths = layout === "split" ? resolveSplitPaneWidths(diffContentWidth) : null; + const selectedPaneLeft = + splitWidths && copySelectionSide === "right" ? splitWidths.leftWidth : 0; + const selectedPaneWidth = splitWidths + ? copySelectionSide === "right" + ? splitWidths.rightWidth + : splitWidths.leftWidth + : diffContentWidth; + const placement = planSelectionActionBar({ + focusVisualRow, + scrollTop: effectiveScrollTop, + viewportHeight: + scrollViewport.height || + scrollRef.current?.viewport.height || + Math.max(0, (height ?? 0) - 1), + paneWidth: selectedPaneWidth, + preferredWidth: SELECTION_ACTION_BAR_WIDTH, + reason: commentSelection.ok ? undefined : commentSelection.reason, + }); + return placement ? { ...placement, left: placement.left + selectedPaneLeft } : null; + }, [ + copySelectionDrag, + commentSelection, + copySelectionSide, + diffContentWidth, + effectiveScrollTop, + height, + layout, + scrollRef, + scrollViewport.height, + ]); + + /** Copy the committed range without coupling selection acquisition to clipboard support. */ + const copyCommittedSelection = useCallback(() => { + const selection = committedCopySelectionRef.current; + if (!selection) return false; + const { start, end } = normalizeCopySelectionRange(selection.anchor, selection.focus); + copySelectionText( + renderCopySelectionText({ + context: copySelectionContext, + end, + side: resolveCopySelectionSide(selection.anchor.column, layout, diffContentWidth), + start, + }), + ); + return true; + }, [copySelectionContext, copySelectionText, diffContentWidth, layout]); + + /** Start a range note when the committed visual selection has one semantic projection. */ + const commentOnCommittedSelection = useCallback(() => { + if (!committedCopySelectionRef.current) return false; + if (!commentSelection.ok) { + onCopyFeedback?.(commentSelection.reason); + return true; + } + onStartUserNoteAtHunk?.( + commentSelection.selection.fileId, + commentSelection.selection.hunkIndex, + commentSelection.selection.target, + ); + clearCopySelection(); + return true; + }, [clearCopySelection, commentSelection, onCopyFeedback, onStartUserNoteAtHunk]); + + /** Return full-line terminal columns for one source side in the active layout. */ + const keyboardSelectionColumns = useCallback( + (side: "old" | "new") => { + if (layout !== "split") return { start: 0, end: Math.max(0, diffContentWidth - 1) }; + const { leftWidth } = resolveSplitPaneWidths(diffContentWidth); + return side === "old" + ? { start: 0, end: Math.max(0, leftWidth - 1) } + : { start: leftWidth, end: Math.max(leftWidth, diffContentWidth - 1) }; + }, + [diffContentWidth, layout], + ); + + /** Begin keyboard range acquisition at the current measured source row. */ + const beginKeyboardSelection = useCallback(() => { + if (!renderedLineCursor) return false; + const bounds = lineCursorBoundsOf(renderedLineCursor); + if (!bounds) return false; + const columns = keyboardSelectionColumns(renderedLineCursor.target.side); + const selection: CopySelectionDrag = { + anchor: { kind: "review-row", visualRow: bounds.top, column: columns.start }, + focus: { + kind: "review-row", + visualRow: bounds.top + Math.max(0, bounds.height - 1), + column: columns.end, + }, + moved: true, + expanded: true, + }; + keyboardSelectionAnchorRef.current = { + top: bounds.top, + bottom: bounds.top + Math.max(0, bounds.height - 1), + side: renderedLineCursor.target.side, + }; + copySelectionDragRef.current = null; + committedCopySelectionRef.current = selection; + setCopySelectionDrag(selection); + return true; + }, [keyboardSelectionColumns, lineCursorBoundsOf, renderedLineCursor]); + + /** Move the keyboard selection focus through source rows on its anchored side. */ + const moveKeyboardSelection = useCallback( + (delta: number) => { + const anchor = keyboardSelectionAnchorRef.current; + if (!anchor || !renderedLineCursor || !onViewportLineCursorChange) return false; + const candidates = lineCursors.flatMap((cursor) => { + if (cursor.target.side === anchor.side) return [cursor]; + const context = contextLineStableKeySides(cursor.stableKey); + return anchor.side === "old" && context + ? [{ ...cursor, target: { side: "old" as const, line: context.oldLine } }] + : []; + }); + const currentIndex = candidates.findIndex( + (cursor) => + cursor.fileId === renderedLineCursor.fileId && + cursor.stableKey === renderedLineCursor.stableKey, + ); + if (currentIndex < 0) return false; + const next = candidates[Math.min(candidates.length - 1, Math.max(0, currentIndex + delta))]; + if (!next) return true; + onViewportLineCursorChange(next); + return true; + }, + [lineCursors, onViewportLineCursorChange, renderedLineCursor], + ); + + useEffect(() => { + const anchor = keyboardSelectionAnchorRef.current; + if (!anchor || !renderedLineCursor || renderedLineCursor.target.side !== anchor.side) return; + const bounds = lineCursorBoundsOf(renderedLineCursor); + if (!bounds) return; + const focusTop = bounds.top; + const focusBottom = bounds.top + Math.max(0, bounds.height - 1); + const columns = keyboardSelectionColumns(anchor.side); + const movingDown = focusTop >= anchor.top; + const selection: CopySelectionDrag = { + anchor: { + kind: "review-row", + visualRow: movingDown ? anchor.top : anchor.bottom, + column: movingDown ? columns.start : columns.end, + }, + focus: { + kind: "review-row", + visualRow: movingDown ? focusBottom : focusTop, + column: movingDown ? columns.end : columns.start, + }, + moved: true, + expanded: true, + }; + committedCopySelectionRef.current = selection; + setCopySelectionDrag(selection); + }, [keyboardSelectionColumns, lineCursorBoundsOf, renderedLineCursor]); + /** Convert one mouse event into a review-stream copy-selection point. */ const resolveCopySelectionPoint = useCallback( (event: TuiMouseEvent): CopySelectionPoint | null => { @@ -1478,13 +1752,15 @@ export function DiffPane({ const point = resolveCopySelectionPoint(event); if (!point) { - copySelectionDragRef.current = null; + clearCopySelection(); clickCountRef.current = 0; lastClickPointRef.current = null; - setCopySelectionDrag(null); return; } + committedCopySelectionRef.current = null; + keyboardSelectionAnchorRef.current = null; + // Detect double-click and triple-click for word/line selection. const now = Date.now(); const timeSinceLastClick = now - lastClickTimeRef.current; @@ -1529,7 +1805,7 @@ export function DiffPane({ event.preventDefault(); event.stopPropagation(); }, - [copySelectionContext, resolveCopySelectionPoint, suppressNativeSelection], + [clearCopySelection, copySelectionContext, resolveCopySelectionPoint, suppressNativeSelection], ); /** Extend the active diff text selection while the pointer moves. */ @@ -1604,11 +1880,12 @@ export function DiffPane({ : pending; copySelectionDragRef.current = null; - setCopySelectionDrag(null); event?.preventDefault(); event?.stopPropagation(); if (copySelectionDragIsClick(current)) { + committedCopySelectionRef.current = null; + setCopySelectionDrag(null); if (event && isNestedRowMouseAction(event)) { return; } @@ -1629,19 +1906,12 @@ export function DiffPane({ } } - const { start, end } = normalizeCopySelectionRange(current.anchor, current.focus); - const text = renderCopySelectionText({ - context: copySelectionContext, - end, - side: copySelectionSide, - start, - }); - copySelectionText(text); + // Mouse-up commits the range without choosing an action. Copy and Comment are + // explicit so the same acquired selection works for pointer and keyboard users. + committedCopySelectionRef.current = current; + setCopySelectionDrag(current); }, [ - copySelectionContext, - copySelectionSide, - copySelectionText, diffContentWidth, fileSectionLayouts, layout, @@ -1658,13 +1928,37 @@ export function DiffPane({ if (!cancelCopySelectionRef) { return; } - cancelCopySelectionRef.current = () => endCopySelection(); + cancelCopySelectionRef.current = () => { + if (copySelectionDragRef.current) endCopySelection(); + }; return () => { if (cancelCopySelectionRef.current) { cancelCopySelectionRef.current = null; } }; - }, [cancelCopySelectionRef, endCopySelection]); + }, [cancelCopySelectionRef, clearCopySelection, endCopySelection]); + + useEffect(() => { + if (!selectionActionsRef) return; + selectionActionsRef.current = { + hasSelection: () => committedCopySelectionRef.current !== null, + beginKeyboardSelection, + copy: copyCommittedSelection, + comment: commentOnCommittedSelection, + clear: clearCopySelection, + move: moveKeyboardSelection, + }; + return () => { + selectionActionsRef.current = null; + }; + }, [ + beginKeyboardSelection, + clearCopySelection, + commentOnCommittedSelection, + copyCommittedSelection, + moveKeyboardSelection, + selectionActionsRef, + ]); /** Clamp one requested review scroll target against the latest planned content height. */ const clampReviewScrollTop = useCallback( @@ -2803,6 +3097,86 @@ export function DiffPane({ })} + {selectionActionPlacement ? ( + { + event.preventDefault(); + event.stopPropagation(); + }} + onMouseUp={(event) => { + event.preventDefault(); + event.stopPropagation(); + }} + > + + event.stopPropagation()} + onMouseUp={(event) => { + event.preventDefault(); + event.stopPropagation(); + commentOnCommittedSelection(); + }} + > + c Comment + + event.stopPropagation()} + onMouseUp={(event) => { + event.preventDefault(); + event.stopPropagation(); + copyCommittedSelection(); + }} + > + y Copy + + event.stopPropagation()} + onMouseUp={(event) => { + event.preventDefault(); + event.stopPropagation(); + clearCopySelection(); + }} + > + Esc Clear + + + {selectionActionPlacement.reasonLines?.map((line, index) => ( + + {line} + + ))} + + ) : null} { + test("accounts for horizontal, compact vertical, and disabled-reason heights", () => { + expect( + planSelectionActionBar({ + focusVisualRow: 12, + scrollTop: 10, + viewportHeight: 8, + paneWidth: 80, + preferredWidth: 34, + }), + ).toEqual({ top: 3, left: 46, height: 3, compact: false }); + expect( + planSelectionActionBar({ + focusVisualRow: 17, + scrollTop: 10, + viewportHeight: 8, + paneWidth: 20, + preferredWidth: 34, + }), + ).toEqual({ top: 2, left: 0, height: 5, compact: true }); + expect( + planSelectionActionBar({ + focusVisualRow: 15, + scrollTop: 10, + viewportHeight: 8, + paneWidth: 80, + preferredWidth: 34, + reason: "Unavailable", + }), + ).toEqual({ + top: 1, + left: 46, + height: 4, + compact: false, + reasonLines: ["Unavailable"], + }); + expect( + planSelectionActionBar({ + focusVisualRow: 17, + scrollTop: 10, + viewportHeight: 12, + paneWidth: 20, + preferredWidth: 34, + reason: "Comment requires contiguous code from one file", + }), + ).toEqual({ + top: 4, + left: 0, + height: 8, + compact: true, + reasonLines: ["Comment requires", "contiguous code", "from one file"], + }); + expect( + planSelectionActionBar({ + focusVisualRow: 9, + scrollTop: 10, + viewportHeight: 8, + paneWidth: 80, + preferredWidth: 34, + }), + ).toBeNull(); + expect( + planSelectionActionBar({ + focusVisualRow: 12, + scrollTop: 10, + viewportHeight: 8, + paneWidth: 12, + preferredWidth: 34, + }), + ).toBeNull(); + }); +}); + +describe("selectionInvalidationIdentity", () => { + const base = { + layout: "stack" as const, + wrapLines: false, + width: 80, + viewportHeight: 20, + codeHorizontalOffset: 0, + showLineNumbers: true, + showHunkHeaders: true, + fileIdentities: ["file:key:content-a"], + rowIdentities: ["row:a:1"], + }; + + test("changes for semantic content and height-only resize facts", () => { + expect(selectionInvalidationIdentity({ ...base, viewportHeight: 19 })).not.toBe( + selectionInvalidationIdentity(base), + ); + expect( + selectionInvalidationIdentity({ ...base, fileIdentities: ["file:key:content-b"] }), + ).not.toBe(selectionInvalidationIdentity(base)); + }); +}); + const OSC52_CLIPBOARD = "\x1b]52;c;SGVsbG8=\x07"; const CSI_CLEAR_SCREEN = "\x1b[2J"; const DCS_PAYLOAD = "\x1bPqpayload\x1b\\"; @@ -389,6 +491,409 @@ describe("findLineCursorForClick", () => { }); }); +describe("projectCommentSelection", () => { + test("projects split context rows onto the explicitly selected side", () => { + const file = createDiffFile(); + const { fileSectionLayouts, sectionGeometry } = buildContext("split", 120, file); + const geometry = sectionGeometry[0]!; + const section = fileSectionLayouts[0]!; + const contextBounds = geometry.rowBounds.find((bounds) => + bounds.stableKey.includes(":context:"), + )!; + const context = contextBounds.stableKey.split(":"); + const oldLine = Number(context[3]); + const newLine = Number(context[4]); + const drag: CopySelectionDrag = { + anchor: { + kind: "review-row", + visualRow: section.bodyTop + contextBounds.top, + column: 8, + }, + focus: { + kind: "review-row", + visualRow: section.bodyTop + contextBounds.top, + column: 14, + }, + moved: true, + }; + + expect( + projectCommentSelection({ + drag, + fileSectionLayouts, + sectionGeometry, + side: "left", + }), + ).toMatchObject({ + ok: true, + selection: { target: { oldRange: [oldLine, oldLine] } }, + }); + expect( + projectCommentSelection({ + drag, + fileSectionLayouts, + sectionGeometry, + side: "right", + }), + ).toMatchObject({ + ok: true, + selection: { target: { newRange: [newLine, newLine] } }, + }); + }); + + test("uses the wrapped focus row as preferred in forward and reverse selections", () => { + const file = createTestDiffFile({ + id: "wrapped-preferred", + path: "wrapped-preferred.ts", + before: "", + after: `${"first-line-content-".repeat(5)}\n${"second-line-content-".repeat(5)}\n`, + }); + const { fileSectionLayouts, sectionGeometry } = buildMultiFileTestContext({ + files: [file], + width: 24, + wrapLines: true, + }); + const section = fileSectionLayouts[0]!; + const geometry = sectionGeometry[0]!; + const cursors = buildLineCursors([file], sectionGeometry).filter( + (cursor) => cursor.target.side === "new", + ); + const boundsForLine = (line: number) => { + const cursor = cursors.find((candidate) => candidate.target.line === line)!; + return geometry.rowBounds.find( + (bounds) => + bounds.stableKey === cursor.stableKey || bounds.stableKeys.includes(cursor.stableKey), + )!; + }; + const first = boundsForLine(1); + const second = boundsForLine(2); + expect(first.height).toBeGreaterThan(1); + expect(second.height).toBeGreaterThan(1); + const point = (bounds: typeof first) => ({ + kind: "review-row" as const, + visualRow: section.bodyTop + bounds.top + bounds.height - 1, + column: 12, + }); + + const forward = projectCommentSelection({ + drag: { anchor: point(first), focus: point(second), moved: true }, + fileSectionLayouts, + sectionGeometry, + }); + const reverse = projectCommentSelection({ + drag: { anchor: point(second), focus: point(first), moved: true }, + fileSectionLayouts, + sectionGeometry, + }); + + expect(forward).toMatchObject({ + ok: true, + selection: { target: { preferred: { side: "new", line: 2 } } }, + }); + expect(reverse).toMatchObject({ + ok: true, + selection: { target: { preferred: { side: "new", line: 1 } } }, + }); + }); + + test("projects contiguous code and rejects selected non-code rows", () => { + const file = createDiffFile(); + const { fileSectionLayouts, sectionGeometry } = buildContext("stack", 120, file); + const section = fileSectionLayouts[0]!; + const geometry = sectionGeometry[0]!; + const cursors = buildLineCursors([file], sectionGeometry); + const rows = (cursor: (typeof cursors)[number]) => { + const bounds = geometry.rowBounds.find( + (candidate) => + candidate.stableKey === cursor.stableKey || + candidate.stableKeys.includes(cursor.stableKey), + )!; + return section.bodyTop + bounds.top; + }; + const sameSide = cursors.find((cursor, index) => { + const next = cursors[index + 1]; + return ( + next?.target.side === cursor.target.side && next.target.line === cursor.target.line + 1 + ); + })!; + const next = cursors[cursors.indexOf(sameSide) + 1]!; + const projected = projectCommentSelection({ + drag: { + anchor: { kind: "review-row", visualRow: rows(sameSide), column: 1 }, + focus: { kind: "review-row", visualRow: rows(next), column: 8 }, + moved: true, + }, + fileSectionLayouts, + sectionGeometry, + }); + expect(projected.ok).toBe(true); + + const sideTransitionIndex = cursors.findIndex((cursor, index) => { + const following = cursors[index + 1]; + return following !== undefined && following.target.side !== cursor.target.side; + }); + const transitionStart = cursors[sideTransitionIndex]!; + const transitionEnd = cursors[sideTransitionIndex + 1]!; + const mixedProjection = projectCommentSelection({ + drag: { + anchor: { kind: "review-row", visualRow: rows(transitionStart), column: 8 }, + focus: { kind: "review-row", visualRow: rows(transitionEnd), column: 12 }, + moved: true, + }, + fileSectionLayouts, + sectionGeometry, + }); + expect(mixedProjection).toEqual({ + ok: true, + selection: { + fileId: file.id, + hunkIndex: transitionEnd.hunkIndex, + target: { + oldRange: + transitionStart.target.side === "old" + ? [transitionStart.target.line, transitionStart.target.line] + : [transitionEnd.target.line, transitionEnd.target.line], + newRange: + transitionStart.target.side === "new" + ? [transitionStart.target.line, transitionStart.target.line] + : [transitionEnd.target.line, transitionEnd.target.line], + preferred: transitionEnd.target, + }, + }, + }); + + expect( + projectCommentSelection({ + drag: { + anchor: { kind: "review-row", visualRow: rows(sameSide), column: 1 }, + focus: { kind: "review-row", visualRow: rows(next), column: 8 }, + moved: true, + }, + fileSectionLayouts, + sectionGeometry: [{ ...geometry, fileViewRows: [] }], + }), + ).toEqual({ + ok: false, + reason: "Comment requires contiguous code from one file", + }); + + const cursorKeys = new Set(cursors.map((cursor) => cursor.stableKey)); + const nonCode = geometry.rowBounds.find( + (bounds) => + ![bounds.stableKey, ...bounds.stableKeys].some((stableKey) => cursorKeys.has(stableKey)), + )!; + expect( + projectCommentSelection({ + drag: { + anchor: { kind: "review-row", visualRow: section.bodyTop + nonCode.top, column: 1 }, + focus: { kind: "review-row", visualRow: section.bodyTop + nonCode.top, column: 8 }, + moved: true, + }, + fileSectionLayouts, + sectionGeometry, + }), + ).toEqual({ + ok: false, + reason: "Comment requires contiguous code from one file", + }); + }); +}); + +describe("projectCommentSelection presentation crossings", () => { + const linePlan = (key: string, stableKey: string, hunkIndex: number) => + ({ + kind: "diff-row", + key, + stableKey, + fileId: "synthetic", + hunkIndex, + row: { + type: "stack-line", + key, + fileId: "synthetic", + hunkIndex, + cell: { kind: "addition", sign: "+", newLineNumber: hunkIndex + 1, spans: [] }, + }, + }) as const; + const first = linePlan("line-1", "line:0:new:1", 0); + const second = linePlan("line-2", "line:1:new:2", 1); + const inline = { + kind: "inline-note", + key: "note", + stableKey: "inline-note:note", + fileId: "synthetic", + hunkIndex: 0, + } as never; + const gap = { + kind: "hunk-gap", + key: "gap", + stableKey: "hunk-gap:1", + fileId: "synthetic", + hunkIndex: 1, + height: 1, + } as const; + const header = { + kind: "diff-row", + key: "header", + stableKey: "hunk:1", + fileId: "synthetic", + hunkIndex: 1, + row: { type: "hunk-header", key: "header", fileId: "synthetic", hunkIndex: 1, text: "@@" }, + } as const; + const plannedRows = [first, inline, gap, header, second] as DiffSectionGeometry["plannedRows"]; + const rowBounds = plannedRows.map((row, top) => ({ + key: row.key, + stableKey: row.stableKey, + stableKeys: [row.stableKey], + top, + height: 1, + })); + const geometry: DiffSectionGeometry = { + bodyHeight: 5, + hunkAnchorRows: new Map(), + hunkBounds: new Map(), + hunkSpans: [ + { additionStart: 1, additionCount: 1, deletionStart: 1, deletionCount: 1 }, + { additionStart: 2, additionCount: 1, deletionStart: 2, deletionCount: 1 }, + ], + lineNumberDigits: 1, + plannedRows, + rowBounds, + rowBoundsByKey: new Map(rowBounds.map((row) => [row.key, row])), + rowBoundsByStableKey: new Map(rowBounds.map((row) => [row.stableKey, row])), + }; + const layouts = [ + { + fileId: "synthetic", + sectionIndex: 0, + sectionTop: 0, + headerTop: 0, + bodyTop: 0, + bodyHeight: 5, + sectionBottom: 5, + }, + ]; + + test("ignores inline notes, hunk gaps, and headers between contiguous code endpoints", () => { + for (const reverse of [false, true]) { + expect( + projectCommentSelection({ + drag: { + anchor: { kind: "review-row", visualRow: reverse ? 4 : 0, column: 8 }, + focus: { kind: "review-row", visualRow: reverse ? 0 : 4, column: 12 }, + moved: true, + }, + fileSectionLayouts: layouts, + sectionGeometry: [geometry], + }), + ).toEqual({ + ok: true, + selection: { + fileId: "synthetic", + hunkIndex: reverse ? 0 : 1, + target: { + newRange: [1, 2], + preferred: { side: "new", line: reverse ? 1 : 2 }, + }, + }, + }); + } + }); + + test("rejects presentation endpoints and expanded or collapsed source gaps", () => { + expect( + projectCommentSelection({ + drag: { + anchor: { kind: "review-row", visualRow: 3, column: 1 }, + focus: { kind: "review-row", visualRow: 4, column: 8 }, + moved: true, + }, + fileSectionLayouts: layouts, + sectionGeometry: [geometry], + }).ok, + ).toBe(false); + + const expandedGeometry: DiffSectionGeometry = { + ...geometry, + rowBounds: geometry.rowBounds.map((row, index) => + index === 2 ? { ...row, expandedGapKey: "before:1" } : row, + ), + }; + const selectionAcross = (candidate: DiffSectionGeometry) => + projectCommentSelection({ + drag: { + anchor: { kind: "review-row", visualRow: 0, column: 8 }, + focus: { kind: "review-row", visualRow: 4, column: 8 }, + moved: true, + }, + fileSectionLayouts: layouts, + sectionGeometry: [candidate], + }); + expect(selectionAcross(expandedGeometry).ok).toBe(false); + + const collapsedGeometry: DiffSectionGeometry = { + ...geometry, + plannedRows: geometry.plannedRows.map((row, index) => + index === 2 + ? ({ + kind: "diff-row", + key: "gap", + stableKey: "hunk-gap:1", + fileId: "synthetic", + hunkIndex: 1, + row: { + type: "collapsed", + key: "gap", + fileId: "synthetic", + hunkIndex: 1, + text: "collapsed", + position: "before", + oldRange: [2, 2], + newRange: [2, 2], + }, + } as const) + : row, + ), + }; + expect(selectionAcross(collapsedGeometry).ok).toBe(false); + }); +}); + +test("projectCommentSelection rejects code endpoints from different files", () => { + const files = [ + createTestDiffFile({ + id: "one", + path: "one.ts", + before: "export const one = 1;\n", + after: "export const one = 2;\n", + }), + createTestDiffFile({ + id: "two", + path: "two.ts", + before: "export const two = 1;\n", + after: "export const two = 2;\n", + }), + ]; + const { fileSectionLayouts, sectionGeometry } = buildMultiFileTestContext({ files }); + const endpoint = (index: number) => { + const bounds = sectionGeometry[index]!.rowBounds.find((row) => + row.stableKeys.some((key) => key.startsWith("line:")), + )!; + return fileSectionLayouts[index]!.bodyTop + bounds.top; + }; + expect( + projectCommentSelection({ + drag: { + anchor: { kind: "review-row", visualRow: endpoint(0), column: 8 }, + focus: { kind: "review-row", visualRow: endpoint(1), column: 12 }, + moved: true, + }, + fileSectionLayouts, + sectionGeometry, + }).ok, + ).toBe(false); +}); + describe("copySelectionDragIsClick", () => { const point = (column: number, visualRow: number): CopySelectionPoint => ({ kind: "review-row", @@ -932,6 +1437,7 @@ describe("buildCopySelectedRowKeys", () => { bodyHeight: 20, hunkAnchorRows: new Map(), hunkBounds: new Map(), + hunkSpans: [], lineNumberDigits: 1, plannedRows: [], rowBounds: [rowBounds], diff --git a/src/ui/components/panes/copySelection.ts b/src/ui/components/panes/copySelection.ts index 6a8cdade0..efecf772f 100644 --- a/src/ui/components/panes/copySelection.ts +++ b/src/ui/components/panes/copySelection.ts @@ -1,4 +1,6 @@ import type { DiffFile } from "../../../core/changeset/model"; +import { reviewRangeTargetCoverageIssue } from "../../../core/review/geometry"; +import type { ReviewRangeTargetV1, ReviewSide } from "../../../core/review/types"; import type { LayoutMode } from "../../../core/run/commandInputs"; import { resolveSplitPaneWidths } from "../../diff/codeColumns"; import { planCodeRowLayout } from "../../diff/codeRowLayout"; @@ -11,11 +13,16 @@ import { type DiffSectionRowBounds, } from "../../diff/diffSectionGeometry"; import type { CopySelectedRowRange } from "../../lib/diffSpatial"; -import type { FileSectionLayout } from "../../lib/fileSectionLayout"; +import { findFileSectionAtOffset, type FileSectionLayout } from "../../lib/fileSectionLayout"; import { fileHeaderStats, fitFileHeaderLabel } from "../../lib/fileHeader"; -import { cellRangeToCharRange, measureTextWidth, sliceTextByWidth } from "../../lib/text"; +import { cellRangeToCharRange, measureTextWidth, sliceTextByWidth, wrapText } from "../../lib/text"; import type { LineCursor } from "../../lib/lineCursors"; -import { contextLineStableKeySides, type PlannedReviewRow } from "../../diff/reviewRenderPlan"; +import { + contextLineStableKeySides, + contextLineStableKeyTarget, + lineStableKeyTarget, + type PlannedReviewRow, +} from "../../diff/reviewRenderPlan"; export type CopySelectionPoint = | { @@ -42,6 +49,16 @@ export interface CopySelectionDrag { expanded?: boolean; } +export interface CommentableSelection { + fileId: string; + hunkIndex: number; + target: ReviewRangeTargetV1; +} + +export type CommentSelectionProjection = + | { ok: true; selection: CommentableSelection } + | { ok: false; reason: string }; + export interface CopySelectionContext { codeHorizontalOffset: number; copyDecorations: boolean; @@ -708,6 +725,227 @@ export function expandSelectionPoint( return null; } +/** Project rendered selection rows into contiguous source ranges for one file. */ +export function projectCommentSelection({ + drag, + fileSectionLayouts, + sectionGeometry, + side, +}: { + drag: CopySelectionDrag | null; + fileSectionLayouts: FileSectionLayout[]; + sectionGeometry: DiffSectionGeometry[]; + side?: CopySelectionSide; +}): CommentSelectionProjection { + const invalid = (): CommentSelectionProjection => ({ + ok: false, + reason: "Comment requires contiguous code from one file", + }); + if (!drag?.moved || drag.anchor.kind !== "review-row" || drag.focus.kind !== "review-row") { + return invalid(); + } + + const { start, end } = normalizeCopySelectionRange(drag.anchor, drag.focus); + const { startRow, endRow } = copySelectionBodyRange(start, end); + const startSection = findFileSectionAtOffset(fileSectionLayouts, startRow); + const endSection = findFileSectionAtOffset(fileSectionLayouts, endRow); + if (!startSection || !endSection || startSection.fileId !== endSection.fileId) return invalid(); + const selected: Array<{ + fileId: string; + hunkIndex: number; + side: ReviewSide; + line: number; + visualRow: number; + visualEndRow: number; + }> = []; + let selectedInvalidRow = false; + let selectedGeometry: DiffSectionGeometry | undefined; + for (const section of fileSectionLayouts) { + if (section.bodyTop + section.bodyHeight <= startRow || section.bodyTop > endRow) continue; + const geometry = sectionGeometry[section.sectionIndex]; + if (!geometry || geometry.fileViewRows !== undefined) return invalid(); + selectedGeometry = geometry; + const plannedRowsByKey = new Map(geometry.plannedRows.map((row) => [row.key, row] as const)); + + for (const bounds of geometry.rowBounds) { + const rowTop = section.bodyTop + bounds.top; + const rowBottom = rowTop + bounds.height; + if (bounds.height <= 0 || rowBottom <= startRow || rowTop > endRow) continue; + + if (bounds.expandedGapKey) { + selectedInvalidRow = true; + continue; + } + + const context = contextLineStableKeySides(bounds.stableKey); + if (context) { + const targetSide: ReviewSide = side === "left" ? "old" : "new"; + selected.push({ + fileId: section.fileId, + hunkIndex: context.hunkIndex, + side: targetSide, + line: targetSide === "old" ? context.oldLine : context.newLine, + visualRow: rowTop, + visualEndRow: rowBottom, + }); + continue; + } + + const fallbackContext = contextLineStableKeyTarget(bounds.stableKey); + if (fallbackContext && side !== "left") { + selected.push({ + fileId: section.fileId, + ...fallbackContext, + visualRow: rowTop, + visualEndRow: rowBottom, + }); + continue; + } + + const targets = bounds.stableKeys + .map(lineStableKeyTarget) + .filter((target): target is NonNullable => target !== null) + .filter((target) => + side === undefined ? true : target.side === (side === "left" ? "old" : "new"), + ); + if (targets.length === 0) { + const plannedRow = plannedRowsByKey.get(bounds.key); + const presentationOnly = + plannedRow?.kind === "inline-note" || + plannedRow?.kind === "hunk-gap" || + (plannedRow?.kind === "diff-row" && plannedRow.row.type === "hunk-header"); + if (!presentationOnly) selectedInvalidRow = true; + continue; + } + for (const target of targets) { + selected.push({ + fileId: section.fileId, + ...target, + visualRow: rowTop, + visualEndRow: rowBottom, + }); + } + } + } + + if (selectedInvalidRow || selected.length === 0 || !selectedGeometry) return invalid(); + const endpointsAreCode = [drag.anchor.visualRow, drag.focus.visualRow].every((visualRow) => + selected.some((target) => visualRow >= target.visualRow && visualRow < target.visualEndRow), + ); + if (!endpointsAreCode) return invalid(); + const fileIds = new Set(selected.map((target) => target.fileId)); + if (fileIds.size !== 1) return invalid(); + + const rangeForSide = (targetSide: ReviewSide) => { + const lines = [ + ...new Set( + selected.filter((target) => target.side === targetSide).map((target) => target.line), + ), + ].sort((a, b) => a - b); + if (lines.length === 0) return undefined; + if (lines.some((line, index) => index > 0 && line !== lines[index - 1]! + 1)) return null; + return [lines[0]!, lines.at(-1)!] as const; + }; + const oldRange = rangeForSide("old"); + const newRange = rangeForSide("new"); + if (oldRange === null || newRange === null || (!oldRange && !newRange)) return invalid(); + + const focusVisualRow = drag.focus.visualRow; + const distanceFromVisualRange = (candidate: (typeof selected)[number]) => { + if (focusVisualRow < candidate.visualRow) return candidate.visualRow - focusVisualRow; + if (focusVisualRow >= candidate.visualEndRow) { + return focusVisualRow - (candidate.visualEndRow - 1); + } + return 0; + }; + const preferred = [...selected].sort( + (left, right) => distanceFromVisualRange(left) - distanceFromVisualRange(right), + )[0]!; + const target: ReviewRangeTargetV1 = { + ...(oldRange ? { oldRange } : {}), + ...(newRange ? { newRange } : {}), + preferred: { side: preferred.side, line: preferred.line }, + }; + if (reviewRangeTargetCoverageIssue(selectedGeometry.hunkSpans, target)) return invalid(); + return { + ok: true, + selection: { + fileId: preferred.fileId, + hunkIndex: preferred.hunkIndex, + target, + }, + }; +} + +export interface SelectionInvalidationFacts { + layout: Exclude; + wrapLines: boolean; + width: number; + viewportHeight: number; + codeHorizontalOffset: number; + showLineNumbers: boolean; + showHunkHeaders: boolean; + fileIdentities: readonly string[]; + rowIdentities: readonly string[]; +} + +/** Identify geometry and semantic content changes that retire a committed selection. */ +export function selectionInvalidationIdentity(facts: SelectionInvalidationFacts) { + return JSON.stringify(facts); +} + +export interface SelectionActionBarPlacement { + top: number; + left: number; + height: number; + compact: boolean; + reasonLines?: readonly string[]; +} + +/** Place the contextual action bar beside a visible focus row. */ +export function planSelectionActionBar({ + focusVisualRow, + scrollTop, + viewportHeight, + paneWidth, + preferredWidth, + reason, +}: { + focusVisualRow: number; + scrollTop: number; + viewportHeight: number; + paneWidth: number; + preferredWidth: number; + reason?: string; +}): SelectionActionBarPlacement | null { + const focusRow = focusVisualRow - scrollTop; + // Eleven inner cells fit the longest compact label (` Esc Clear `); the border needs two. + if (focusRow < 0 || focusRow >= viewportHeight || viewportHeight <= 0 || paneWidth < 13) { + return null; + } + const compact = paneWidth < preferredWidth; + const width = Math.min(paneWidth, preferredWidth); + const reasonLines = reason === undefined ? [] : wrapText(reason, Math.max(1, width - 4)); + // The outer two rows belong to the border; action and reason rows occupy its interior. + const height = (compact ? 3 : 1) + reasonLines.length + 2; + if (height > viewportHeight) return null; + const below = focusRow + 1; + const above = focusRow - height; + const top = + below + height <= viewportHeight + ? below + : above >= 0 + ? above + : Math.max(0, Math.min(below, viewportHeight - height)); + return { + top, + left: Math.max(0, paneWidth - width), + height, + compact, + ...(reasonLines.length > 0 ? { reasonLines } : {}), + }; +} + /** Build file-local row key ranges for the visible copy-selection highlight. */ export function buildCopySelectedRowKeys({ drag, diff --git a/src/ui/components/ui-components.test.tsx b/src/ui/components/ui-components.test.tsx index 74d9acd5c..2e658ef19 100644 --- a/src/ui/components/ui-components.test.tsx +++ b/src/ui/components/ui-components.test.tsx @@ -1,6 +1,7 @@ import { describe, expect, mock, spyOn, test } from "bun:test"; import type { ScrollBoxRenderable } from "@opentui/core"; import { MouseButtons } from "@opentui/core/testing"; +import { useKeyboard } from "@opentui/react"; import { testRender } from "@opentui/react/test-utils"; import { act, createRef, useCallback, useEffect, useRef, useState, type ReactNode } from "react"; import type { AppBootstrap } from "../../core/bootstrap"; @@ -22,6 +23,7 @@ import { builtinCommandKeyDefaults, builtinCommandMatchProbes } from "../lib/app import { resolveCommandKeys } from "../lib/keymap"; import type { CurrentLineAlignment, LineRevealPlacement } from "../lib/hunkScroll"; import type { LineCursor } from "../lib/lineCursors"; +import type { ReviewSelectionActionsHandle } from "./panes/DiffPane"; const { AppHost } = await import("../AppHost"); const { toReadOnlyFileViews } = await import("../../extensions/events"); @@ -662,17 +664,24 @@ describe("UI components", () => { const copyText = mock((_text: string) => undefined); const selectLine = mock((_cursor: LineCursor) => undefined); const startUserNote = mock(() => undefined); - const setup = await testRender( - , - { width: 80, height: 8 }, - ); + const SelectionDiffPane = () => { + const selectionActionsRef = useRef(null); + useKeyboard((key) => { + if (key.name === "y" || key.sequence === "y") selectionActionsRef.current?.copy(); + }); + return ( + + ); + }; + const setup = await testRender(, { width: 80, height: 8 }); try { await settleDiffPane(setup); @@ -722,9 +731,12 @@ describe("UI components", () => { selectLine.mockClear(); await setup.mockMouse.drag(oldX, changedY, oldX + 4, changedY, MouseButtons.LEFT); }); - expect(copyText).toHaveBeenCalled(); + expect(copyText).not.toHaveBeenCalled(); expect(selectLine).not.toHaveBeenCalled(); + await act(async () => setup.mockInput.typeText("y")); + expect(copyText).toHaveBeenCalled(); + await act(async () => { await setup.mockMouse.moveTo(newX, changedY); await setup.renderOnce(); @@ -1069,7 +1081,7 @@ describe("UI components", () => { capturedTestColorToHex(span.bg)?.toLowerCase() === theme.addedBg.toLowerCase(), ), ).length; - expect(measuredHeight).toBe(reserveAddNoteColumn ? 3 : 2); + expect(measuredHeight).toBe(reserveAddNoteColumn ? 2 : 1); expect(renderedHeight).toBe(measuredHeight); } finally { await act(async () => { @@ -3406,7 +3418,9 @@ describe("UI components", () => { ); const lines = frame.split("\n"); - const noteTopIndex = lines.findIndex((line) => line.includes("╭") && line.includes("╮")); + const noteTopIndex = lines.findIndex( + (line) => line.includes("╭") && (line.includes("╮") || line.includes("┬")), + ); expect(noteTopIndex).toBeGreaterThan(0); expect(lines[noteTopIndex - 1]).toContain("export const add = true;"); expect(lines[noteTopIndex - 1]?.trim()).not.toBe("│"); diff --git a/src/ui/diff/CodeCellView.tsx b/src/ui/diff/CodeCellView.tsx index 0fd347a76..729a688fa 100644 --- a/src/ui/diff/CodeCellView.tsx +++ b/src/ui/diff/CodeCellView.tsx @@ -270,6 +270,13 @@ function isChunkCompatibleWrappedHighlight(highlight: CodeCellHighlight | undefi return !highlight?.colRange || highlight.colRange === FULL_CODE_CELL_COL_RANGE; } +/** Whether a highlight paints cell chrome as well as source-text columns. */ +function highlightsWholeCell(highlight: CodeCellHighlight | undefined) { + return Boolean( + highlight && (!highlight.colRange || highlight.colRange === FULL_CODE_CELL_COL_RANGE), + ); +} + /** Append one wrapped cell without constructing intermediate React span elements. */ function appendWrappedCellChunks( chunks: TextChunk[], @@ -626,7 +633,9 @@ const SplitCellContent = memo(function SplitCellContent({ paneOffset: number; }) { const basePalette = splitCellPalette(cell.kind, theme, cell.moveKind); - const palette = highlight ? applyHighlightPalette(basePalette, highlight.bg) : basePalette; + const palette = highlightsWholeCell(highlight) + ? applyHighlightPalette(basePalette, highlight!.bg) + : basePalette; const gutterText = splitGutterText(cell, lineNumberDigits, showLineNumbers).padEnd(gutterWidth); const globalContentStart = paneOffset + prefixWidth + gutterWidth; const colRange = highlight?.colRange; @@ -674,7 +683,8 @@ function renderSplitCell( highlight?: CodeCellHighlight, paneOffset = 0, ) { - const resolvedPrefix = highlight && prefix ? applyHighlightPrefix(prefix, highlight.bg) : prefix; + const resolvedPrefix = + highlightsWholeCell(highlight) && prefix ? applyHighlightPrefix(prefix, highlight!.bg) : prefix; const prefixWidth = resolvedPrefix?.text.length ?? 0; return ( @@ -727,7 +737,9 @@ const StackCellContent = memo(function StackCellContent({ highlight?: CodeCellHighlight; }) { const basePalette = stackCellPalette(cell.kind, theme, cell.moveKind); - const palette = highlight ? applyHighlightPalette(basePalette, highlight.bg) : basePalette; + const palette = highlightsWholeCell(highlight) + ? applyHighlightPalette(basePalette, highlight!.bg) + : basePalette; const globalContentStart = prefixWidth + gutterWidth; const colRange = highlight?.colRange; const localColRange = @@ -773,7 +785,8 @@ function renderStackCell( }, highlight?: CodeCellHighlight, ) { - const resolvedPrefix = highlight && prefix ? applyHighlightPrefix(prefix, highlight.bg) : prefix; + const resolvedPrefix = + highlightsWholeCell(highlight) && prefix ? applyHighlightPrefix(prefix, highlight!.bg) : prefix; const prefixWidth = resolvedPrefix?.text.length ?? 0; return ( @@ -815,8 +828,11 @@ function renderWrappedSplitCellLine( highlight?: CodeCellHighlight, paneOffset = 0, ) { - const resolvedPalette = highlight ? applyHighlightPalette(palette, highlight.bg) : palette; - const resolvedPrefix = highlight ? applyHighlightPrefix(prefix, highlight.bg) : prefix; + const wholeCellHighlight = highlightsWholeCell(highlight); + const resolvedPalette = wholeCellHighlight + ? applyHighlightPalette(palette, highlight!.bg) + : palette; + const resolvedPrefix = wholeCellHighlight ? applyHighlightPrefix(prefix, highlight!.bg) : prefix; const prefixWidth = prefix.text.length; const gutterWidth = line.gutterText.length; @@ -871,8 +887,11 @@ function renderWrappedStackCellLine( }, highlight?: CodeCellHighlight, ) { - const resolvedPalette = highlight ? applyHighlightPalette(palette, highlight.bg) : palette; - const resolvedPrefix = highlight ? applyHighlightPrefix(prefix, highlight.bg) : prefix; + const wholeCellHighlight = highlightsWholeCell(highlight); + const resolvedPalette = wholeCellHighlight + ? applyHighlightPalette(palette, highlight!.bg) + : palette; + const resolvedPrefix = wholeCellHighlight ? applyHighlightPrefix(prefix, highlight!.bg) : prefix; const prefixWidth = prefix.text.length; const gutterWidth = line.gutterText.length; diff --git a/src/ui/diff/CodeRowView.test.tsx b/src/ui/diff/CodeRowView.test.tsx index 8091c8a12..bf21f51e1 100644 --- a/src/ui/diff/CodeRowView.test.tsx +++ b/src/ui/diff/CodeRowView.test.tsx @@ -4,7 +4,12 @@ import { act } from "react"; import { capturedTestColorToHex } from "../../../test/helpers/test-color-helpers"; import { resolveTheme } from "../themes"; import { CodeRowView, type PlannedCodeReviewRow } from "./CodeRowView"; -import { cursorLineHighlightBg, selectionHighlightBg, stackCellPalette } from "./rowStyle"; +import { + cursorLineHighlightBg, + selectionHighlightBg, + stackCellPalette, + stackRailColor, +} from "./rowStyle"; /** Return the normalized background painted behind matching captured text. */ function backgroundForText( @@ -17,6 +22,77 @@ function backgroundForText( return capturedTestColorToHex(span?.bg)?.toLowerCase(); } +/** Return the normalized foreground of the first captured span carrying text. */ +function foregroundForText( + capture: ReturnType>["captureSpans"]>, + text: string, +) { + const span = capture.lines + .flatMap((line) => line.spans) + .find((candidate) => candidate.text.includes(text)); + return capturedTestColorToHex(span?.fg)?.toLowerCase(); +} + +test("CodeRowView limits character selections to source text instead of cell chrome", async () => { + const theme = resolveTheme("github-dark-default", null); + const plannedRow: PlannedCodeReviewRow = { + kind: "diff-row", + key: "diff-row:character-range", + stableKey: "line:0:new:1", + fileId: "paint", + hunkIndex: 0, + row: { + type: "stack-line", + key: "character-range", + fileId: "paint", + hunkIndex: 0, + cell: { + kind: "addition", + sign: "+", + newLineNumber: 1, + spans: [{ text: "selected" }], + }, + }, + }; + const setup = await testRender( + , + { width: 20, height: 2 }, + ); + + try { + await act(async () => { + await setup.renderOnce(); + }); + const spans = setup.captureSpans(); + const palette = stackCellPalette("addition", theme); + + expect(backgroundForText(spans, "lec")).toBe( + selectionHighlightBg(palette.contentBg, theme).toLowerCase(), + ); + expect(backgroundForText(spans, "se")).toBe(palette.contentBg.toLowerCase()); + expect(backgroundForText(spans, "+ ")).toBe(palette.gutterBg.toLowerCase()); + expect(backgroundForText(spans, "▌")).toBe(theme.panel.toLowerCase()); + expect(foregroundForText(spans, "+ ")).toBe(palette.numberColor.toLowerCase()); + expect(foregroundForText(spans, "▌")).toBe( + stackRailColor("addition", theme, false).toLowerCase(), + ); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } +}); + test("CodeRowView gives copy selection precedence over cursor paint", async () => { const theme = resolveTheme("github-dark-default", null); const plannedRow: PlannedCodeReviewRow = { @@ -69,3 +145,58 @@ test("CodeRowView gives copy selection precedence over cursor paint", async () = }); } }); + +test("CodeRowView overlays the nowrap add-note badge instead of shifting the note guide", async () => { + const theme = resolveTheme("github-dark-default", null); + const plannedRow: PlannedCodeReviewRow = { + kind: "diff-row", + key: "diff-row:note-guide-hover", + stableKey: "line:0:new:1", + fileId: "paint", + hunkIndex: 0, + anchorId: "note-guide", + noteGuideSide: "new", + row: { + type: "stack-line", + key: "note-guide-hover", + fileId: "paint", + hunkIndex: 0, + cell: { + kind: "addition", + sign: "+", + newLineNumber: 1, + spans: [{ text: "selected" }], + }, + }, + }; + const setup = await testRender( + {}} + />, + { width: 17, height: 2 }, + ); + + try { + await act(async () => { + await setup.renderOnce(); + }); + const line = setup.captureCharFrame().split("\n")[0] ?? ""; + + expect(line.slice(0, 16)).toEndWith("[+]"); + expect(line.slice(0, 16)).not.toContain("│[+]"); + expect(line[16]).toBe("│"); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } +}); diff --git a/src/ui/diff/CodeRowView.tsx b/src/ui/diff/CodeRowView.tsx index 88c466349..a85e777e3 100644 --- a/src/ui/diff/CodeRowView.tsx +++ b/src/ui/diff/CodeRowView.tsx @@ -65,11 +65,18 @@ function renderAddNoteButton( hunkIndex: number, target: UserNoteLineTarget | undefined, onStartUserNoteAtHunk?: (hunkIndex: number, target?: UserNoteLineTarget) => void, + overlayColumn?: number, ) { return ( { markNestedRowMouseAction(event); onStartUserNoteAtHunk?.(hunkIndex, target); @@ -82,6 +89,17 @@ function renderAddNoteButton( ); } +/** Paint one range guide in the annotation gutter immediately outside diff content. */ +function renderExternalRangeGuide(key: string, width: number, theme: AppTheme) { + return ( + + + │ + + + ); +} + /** Fill the reserved wrapped-row hover column so row backgrounds do not visibly shrink. */ function renderAddNoteSpacer(key: string, width: number, bg: string) { if (width <= 0) { @@ -124,7 +142,9 @@ export function CodeRowView({ const codeRowLayout = planCodeRowLayout(plannedRow, { lineNumberDigits, reserveAddNoteColumn: Boolean(onStartUserNoteAtHunk), - showAddNoteBadge, + // Nowrap rows paint the hover affordance over their trailing cells so the note guide stays + // fixed. Wrapped rows reserve the column because overlaying continuation text would hide code. + showAddNoteBadge: wrapLines && showAddNoteBadge, showLineNumbers, width, wrapLines, @@ -173,8 +193,7 @@ export function CodeRowView({ if (row.type === "split-line") { // The planner and row type are derived from the same complete planned row. const splitLayout = codeRowLayout as Extract; - const guideOnOldSide = splitLayout.noteGuideSide === "old"; - const guideOnNewSide = splitLayout.noteGuideSide === "new"; + const hasRangeGuide = splitLayout.noteGuideSide !== undefined; const addNoteTarget: UserNoteLineTarget | undefined = row.right.lineNumber !== undefined ? { side: "new", line: row.right.lineNumber } @@ -184,15 +203,13 @@ export function CodeRowView({ const addBadgeWidth = splitLayout.addNoteBadgeWidth; const leftPrefix = { - text: guideOnOldSide ? "│" : diffRailMarker(), - fg: guideOnOldSide - ? theme.noteBorder - : splitLeftRailColor(row.left.kind, theme, selected || hasCopySelection), + text: diffRailMarker(), + fg: splitLeftRailColor(row.left.kind, theme, selected), bg: theme.panel, }; const rightPrefix = { text: "▌", - fg: splitRightRailColor(row.right.kind, theme, selected || hasCopySelection), + fg: splitRightRailColor(row.right.kind, theme, selected), bg: theme.panel, }; @@ -200,15 +217,16 @@ export function CodeRowView({ return ( onHoverRow?.(row.key)} > - + {codeCellView.renderNowrapSplit({ row, layout: splitLayout, @@ -220,7 +238,7 @@ export function CodeRowView({ rightPrefix, leftHighlight, rightHighlight, - guideOnNewSide, + guideOnNewSide: false, })} {showAddNoteBadge @@ -230,8 +248,10 @@ export function CodeRowView({ row.hunkIndex, addNoteTarget, onStartUserNoteAtHunk, + Math.max(0, width - CODE_ROW_ADD_NOTE_BADGE_WIDTH), ) : null} + {hasRangeGuide ? renderExternalRangeGuide(`${row.key}:range-guide`, width, theme) : null} ); } @@ -246,40 +266,46 @@ export function CodeRowView({ rightPrefix, leftHighlight, rightHighlight, - guideOnNewSide, + guideOnNewSide: false, }); return ( - + {Array.from({ length: wrapped.lineCount }, (_, index) => { const showBadgeOnLine = showAddNoteBadge && index === 0; const styledRow = wrapped.paintLine(index, showBadgeOnLine ? 0 : addBadgeWidth); - if (!showBadgeOnLine) { - return ( - onHoverRow?.(row.key)} - /> - ); - } return ( onHoverRow?.(row.key)} > - + {showBadgeOnLine ? ( + <> + + + + {renderAddNoteButton( + `${row.key}:add-note:${index}`, + theme, + row.hunkIndex, + addNoteTarget, + onStartUserNoteAtHunk, + )} + + ) : ( - - {renderAddNoteButton( - `${row.key}:add-note:${index}`, - theme, - row.hunkIndex, - addNoteTarget, - onStartUserNoteAtHunk, )} + {hasRangeGuide + ? renderExternalRangeGuide(`${row.key}:range-guide:${index}`, width, theme) + : null} ); })} @@ -289,8 +315,7 @@ export function CodeRowView({ // The planner and row type are derived from the same complete planned row. const stackLayout = codeRowLayout as Extract; - const guideOnOldSide = stackLayout.noteGuideSide === "old"; - const guideOnNewSide = stackLayout.noteGuideSide === "new"; + const hasRangeGuide = stackLayout.noteGuideSide !== undefined; const addNoteTarget: UserNoteLineTarget | undefined = row.cell.newLineNumber !== undefined ? { side: "new", line: row.cell.newLineNumber } @@ -299,10 +324,8 @@ export function CodeRowView({ : undefined; const addBadgeWidth = stackLayout.addNoteBadgeWidth; const prefix = { - text: guideOnOldSide ? "│" : diffRailMarker(), - fg: guideOnOldSide - ? theme.noteBorder - : stackRailColor(row.cell.kind, theme, selected || hasCopySelection), + text: diffRailMarker(), + fg: stackRailColor(row.cell.kind, theme, selected), bg: theme.panel, }; @@ -310,15 +333,16 @@ export function CodeRowView({ return ( onHoverRow?.(row.key)} > - + {codeCellView.renderNowrapStack({ row, layout: stackLayout, @@ -328,7 +352,7 @@ export function CodeRowView({ horizontalOffset: codeHorizontalOffset, prefix, highlight: cellHighlight, - guideOnNewSide, + guideOnNewSide: false, })} {showAddNoteBadge @@ -338,8 +362,10 @@ export function CodeRowView({ row.hunkIndex, addNoteTarget, onStartUserNoteAtHunk, + Math.max(0, width - CODE_ROW_ADD_NOTE_BADGE_WIDTH), ) : null} + {hasRangeGuide ? renderExternalRangeGuide(`${row.key}:range-guide`, width, theme) : null} ); } @@ -352,11 +378,11 @@ export function CodeRowView({ theme, prefix, highlight: cellHighlight, - guideOnNewSide, + guideOnNewSide: false, }); return ( - + {Array.from({ length: wrapped.lineCount }, (_, index) => { const showBadgeOnLine = showAddNoteBadge && index === 0; const styledRow = wrapped.paintLine(index); @@ -364,7 +390,13 @@ export function CodeRowView({ return ( onHoverRow?.(row.key)} > ); })} diff --git a/src/ui/diff/DiffSectionBody.tsx b/src/ui/diff/DiffSectionBody.tsx index f52cffd4a..9c61d301d 100644 --- a/src/ui/diff/DiffSectionBody.tsx +++ b/src/ui/diff/DiffSectionBody.tsx @@ -415,6 +415,7 @@ export function DiffSectionBody({ layout={layout} noteCount={plannedRow.noteCount} noteIndex={plannedRow.noteIndex} + rangeGuideConnection={plannedRow.rangeGuideConnection} theme={theme} width={width} /> diff --git a/src/ui/diff/codeRowLayout.test.ts b/src/ui/diff/codeRowLayout.test.ts index 5c7fd13e2..64d52f009 100644 --- a/src/ui/diff/codeRowLayout.test.ts +++ b/src/ui/diff/codeRowLayout.test.ts @@ -76,7 +76,7 @@ function decoratedLines(row: PlannedReviewRow, options: CodeRowLayoutOptions) { } describe("planned code-row layout", () => { - test("split measurement and decorated rendering reserve a new-side guide at an exact wrap boundary", () => { + test("split measurement keeps an external new-side guide outside canonical row text", () => { const options = { width: 20, lineNumberDigits: 1, @@ -94,16 +94,16 @@ describe("planned code-row layout", () => { }); expect(planCodeRowLayout(guided, options)).toMatchObject({ kind: "split", - right: { contentWidth: 6, wrappedLineCount: 2 }, - trailingGuideWidth: 1, - wrappedLineCount: 2, + right: { contentWidth: 7, wrappedLineCount: 1 }, + trailingGuideWidth: 0, + wrappedLineCount: 1, }); - expect(measurePlannedRenderedRowHeight(guided, { ...options, showHunkHeaders: true })).toBe(2); - expect(decoratedLines(guided, options)).toHaveLength(2); - expect(decoratedLines(guided, options).every((line) => line.endsWith("│"))).toBe(true); + expect(measurePlannedRenderedRowHeight(guided, { ...options, showHunkHeaders: true })).toBe(1); + expect(decoratedLines(guided, options)).toHaveLength(1); + expect(decoratedLines(guided, options).some((line) => line.endsWith("│"))).toBe(false); }); - test("stack measurement and decorated rendering reserve a new-side guide at an exact wrap boundary", () => { + test("stack measurement keeps an external new-side guide outside canonical row text", () => { const options = { width: 10, lineNumberDigits: 1, @@ -121,13 +121,13 @@ describe("planned code-row layout", () => { }); expect(planCodeRowLayout(guided, options)).toMatchObject({ kind: "stack", - cell: { contentWidth: 6, wrappedLineCount: 2 }, - trailingGuideWidth: 1, - wrappedLineCount: 2, + cell: { contentWidth: 7, wrappedLineCount: 1 }, + trailingGuideWidth: 0, + wrappedLineCount: 1, }); - expect(measurePlannedRenderedRowHeight(guided, { ...options, showHunkHeaders: true })).toBe(2); - expect(decoratedLines(guided, options)).toHaveLength(2); - expect(decoratedLines(guided, options).every((line) => line.endsWith("│"))).toBe(true); + expect(measurePlannedRenderedRowHeight(guided, { ...options, showHunkHeaders: true })).toBe(1); + expect(decoratedLines(guided, options)).toHaveLength(1); + expect(decoratedLines(guided, options).some((line) => line.endsWith("│"))).toBe(false); }); test("memoizes wrapped measurement while preserving lazy plan construction", () => { @@ -191,9 +191,9 @@ describe("planned code-row layout", () => { ? CODE_ROW_ADD_NOTE_BADGE_WIDTH : 0; expect(plan.addNoteBadgeWidth).toBe(expectedBadgeWidth); - expect(plan.trailingGuideWidth).toBe(noteGuideSide === "new" ? 1 : 0); + expect(plan.trailingGuideWidth).toBe(0); - const reservedWidth = plan.trailingGuideWidth + plan.addNoteBadgeWidth; + const reservedWidth = plan.addNoteBadgeWidth; if (plan.kind === "split") { expect(plan.left.width + plan.right.width + reservedWidth).toBe(options.width); expect(plan.left.prefixWidth).toBe(1); diff --git a/src/ui/diff/codeRowLayout.ts b/src/ui/diff/codeRowLayout.ts index 9a105f8c7..2bdf0a83a 100644 --- a/src/ui/diff/codeRowLayout.ts +++ b/src/ui/diff/codeRowLayout.ts @@ -100,7 +100,8 @@ export function planCodeRowLayout( } const prefixWidth = 1; - const trailingGuideWidth = plannedRow.noteGuideSide === "new" ? 1 : 0; + // Range guides render in the pane's external annotation gutter and never consume code width. + const trailingGuideWidth = 0; const addNoteBadgeWidth = showAddNoteBadge || (wrapLines && reserveAddNoteColumn) ? CODE_ROW_ADD_NOTE_BADGE_WIDTH : 0; diff --git a/src/ui/diff/diffSectionGeometry.ts b/src/ui/diff/diffSectionGeometry.ts index e01de989f..5594b7de3 100644 --- a/src/ui/diff/diffSectionGeometry.ts +++ b/src/ui/diff/diffSectionGeometry.ts @@ -1,6 +1,7 @@ import { DEFAULT_HUNK_GAP } from "../../core/run/reviewGap"; import { DEFAULT_TAB_WIDTH } from "../../core/run/tabWidth"; import type { DiffFile } from "../../core/changeset/model"; +import type { ReviewHunkSpan } from "../../core/review/geometry"; import type { LayoutMode } from "../../core/run/commandInputs"; import { measureAgentInlineNoteHeight } from "../components/panes/AgentInlineNote"; import type { VisibleAgentNote } from "../lib/agentAnnotations"; @@ -36,6 +37,8 @@ export interface DiffSectionRowBounds extends VerticalBounds { * implementations keep that row stream lazy so ordinary scrolling/navigation retain only bounds. */ export interface DiffSectionGeometry extends SectionGeometry { + /** Visible patch extents used by shared comment-range coverage validation. */ + hunkSpans: readonly ReviewHunkSpan[]; lineNumberDigits: number; plannedRows: PlannedReviewRow[]; /** Alternate-view rows consume the same measured bounds while raw copy remains unavailable. */ @@ -294,6 +297,7 @@ export function measureDiffSectionGeometry( bodyHeight: 1, hunkAnchorRows: new Map(), hunkBounds: new Map(), + hunkSpans: file.metadata.hunks, lineNumberDigits: String(findMaxLineNumber(file)).length, plannedRows: [], rowBounds: [], @@ -418,6 +422,7 @@ export function measureDiffSectionGeometry( bodyHeight, hunkAnchorRows, hunkBounds, + hunkSpans: file.metadata.hunks, lineNumberDigits, get plannedRows() { return resolvePlannedRows(); diff --git a/src/ui/diff/plannedRowText.ts b/src/ui/diff/plannedRowText.ts index 19e2c6071..1a6c535e4 100644 --- a/src/ui/diff/plannedRowText.ts +++ b/src/ui/diff/plannedRowText.ts @@ -210,9 +210,8 @@ export function renderDecoratedPlannedRowText( CodeRowLayoutPlan, { kind: "split" } >; - const guideOnOldSide = codeLayout.noteGuideSide === "old"; - const guideOnNewSide = codeLayout.noteGuideSide === "new"; - const leftPrefix = guideOnOldSide ? "│" : diffRailMarker(); + // The external range rail is presentation chrome outside this canonical text width. + const leftPrefix = diffRailMarker(); const rightPrefix = "▌"; const leftCell = buildPlainSplitCellLines( @@ -254,10 +253,10 @@ export function renderDecoratedPlannedRowText( return normalizedLeft; } if (side === "right") { - return `${normalizedRight}${guideOnNewSide ? "│" : ""}`; + return normalizedRight; } - return `${normalizedLeft}${normalizedRight}${guideOnNewSide ? "│" : ""}`; + return `${normalizedLeft}${normalizedRight}`; }); } @@ -269,9 +268,8 @@ export function renderDecoratedPlannedRowText( CodeRowLayoutPlan, { kind: "stack" } >; - const guideOnOldSide = codeLayout.noteGuideSide === "old"; - const guideOnNewSide = codeLayout.noteGuideSide === "new"; - const prefix = guideOnOldSide ? "│" : diffRailMarker(); + // The external range rail is presentation chrome outside this canonical text width. + const prefix = diffRailMarker(); const cellLines = buildPlainStackCellLines( preparedRow.cell, codeLayout.cell, @@ -284,7 +282,7 @@ export function renderDecoratedPlannedRowText( return cellLines.map((line) => { const visibleLine = `${prefix}${line.spansText}`; const normalized = padText(visibleLine, Math.max(1, codeLayout.cell.width)); - return `${normalized}${guideOnNewSide ? "│" : ""}`; + return normalized; }); } diff --git a/src/ui/diff/reviewRenderPlan.test.ts b/src/ui/diff/reviewRenderPlan.test.ts index fcedb60fe..e874cad90 100644 --- a/src/ui/diff/reviewRenderPlan.test.ts +++ b/src/ui/diff/reviewRenderPlan.test.ts @@ -85,7 +85,7 @@ function guidedSplitLineNumbers(plannedRows: PlannedReviewRow[], side: "old" | " } describe("review render plan", () => { - test("inserts an inline note after the anchor row and starts the guide below the note", () => { + test("connects the full annotated range directly into its inline note", () => { const theme = resolveTheme("github-dark-default", null); const file = createDiffFile( "alpha", @@ -117,6 +117,7 @@ describe("review render plan", () => { expect(note.anchorSide).toBe("new"); expect(note.noteCount).toBe(1); expect(note.noteIndex).toBe(0); + expect(note.rangeGuideConnection).toBe("continue"); } const anchoredRow = inlineNoteAnchorRow(plannedRows); @@ -128,10 +129,42 @@ describe("review render plan", () => { } } - expect(guidedSplitLineNumbers(plannedRows, "new")).toEqual([3]); + expect(guidedSplitLineNumbers(plannedRows, "new")).toEqual([2, 3]); }); - test("anchors deletion-only notes to old-side rows without a dangling guide above the note", () => { + test("keeps the aggregate rail connected through overlapping cards", () => { + const theme = resolveTheme("github-dark-default", null); + const file = createDiffFile( + "overlap", + "overlap.ts", + "export const alpha = 1;\n", + "export const alpha = 2;\nexport const beta = 3;\nexport const gamma = 4;\n", + ); + const rows = buildSplitRows(file, null, theme); + const notes = [ + createVisibleAgentNote(file.metadata.hunks, { + id: "long", + annotation: { newRange: [2, 3], summary: "long range" }, + }), + createVisibleAgentNote(file.metadata.hunks, { + id: "range-less", + annotation: { summary: "hunk note" }, + target: { hunkIndex: 0, side: "new", line: 2 }, + }), + ]; + const plannedRows = buildReviewRenderPlan({ + fileId: file.id, + rows, + showHunkHeaders: true, + visibleAgentNotes: notes, + }); + const inlineNotes = plannedRows.filter((row) => row.kind === "inline-note"); + + expect(inlineNotes).toHaveLength(2); + expect(inlineNotes.map((row) => row.rangeGuideConnection)).toEqual(["continue", "continue"]); + }); + + test("connects a deletion-only anchor row directly into its inline note", () => { const theme = resolveTheme("github-dark-default", null); const file = createDiffFile( "deleted", @@ -173,7 +206,7 @@ describe("review render plan", () => { } } - expect(guidedSplitLineNumbers(plannedRows, "old")).toEqual([]); + expect(guidedSplitLineNumbers(plannedRows, "old")).toEqual([1]); }); test("assigns hunk anchor ids from the first visible row for every hunk when hunk headers are hidden", () => { diff --git a/src/ui/diff/reviewRenderPlan.ts b/src/ui/diff/reviewRenderPlan.ts index 830932581..0e940f79d 100644 --- a/src/ui/diff/reviewRenderPlan.ts +++ b/src/ui/diff/reviewRenderPlan.ts @@ -46,6 +46,8 @@ export type PlannedReviewRow = anchorSide?: "old" | "new"; noteCount: number; noteIndex: number; + /** Connect this ranged note to the external annotation rail. */ + rangeGuideConnection?: "terminate" | "continue"; } | { kind: "hunk-gap"; @@ -284,8 +286,7 @@ function buildInlineVisibleNotePlacements(rows: DiffRow[], visibleAgentNotes: Vi } const anchorSide = note.anchor.preferred?.side; - const coveredRows = fileLineRows.filter((row) => rowOverlapsNoteRange(row, note.anchor)); - const guideRows = coveredRows.filter((row) => row.key !== anchorRow.key); + const guideRows = fileLineRows.filter((row) => rowOverlapsNoteRange(row, note.anchor)); const anchorPlacements = placementsByAnchor.get(anchorRow.key) ?? []; anchorPlacements.push({ @@ -333,6 +334,33 @@ function buildNoteGuideSideByRowKey(placementsByAnchor: Map, +) { + const lineRowIndexByKey = new Map(lineRows(rows).map((row, index) => [row.key, index])); + const continuationRowKeys = new Set(); + + for (const placements of placementsByAnchor.values()) { + for (const placement of placements) { + const guidedIndices = [...placement.guidedRowKeys].flatMap((key) => { + const index = lineRowIndexByKey.get(key); + return index === undefined ? [] : [index]; + }); + const lastGuidedIndex = Math.max(-1, ...guidedIndices); + for (const key of placement.guidedRowKeys) { + const index = lineRowIndexByKey.get(key); + if (index !== undefined && index < lastGuidedIndex) { + continuationRowKeys.add(key); + } + } + } + } + + return continuationRowKeys; +} + function rowCanAnchorHunk(row: DiffRow, showHunkHeaders: boolean) { if (showHunkHeaders) { return row.type === "hunk-header"; @@ -370,6 +398,7 @@ export function buildReviewRenderPlan({ }) { const placementsByAnchor = buildInlineVisibleNotePlacements(rows, visibleAgentNotes); const noteGuideSideByRowKey = buildNoteGuideSideByRowKey(placementsByAnchor); + const rangeGuideContinuationRows = rangeGuideContinuationRowKeys(rows, placementsByAnchor); const plannedRows: PlannedReviewRow[] = []; const anchoredHunks = new Set(); @@ -422,6 +451,11 @@ export function buildReviewRenderPlan({ anchorSide: placement.anchorSide, noteCount: placement.noteCount, noteIndex: placement.noteIndex, + rangeGuideConnection: !noteGuideSideByRowKey.has(row.key) + ? undefined + : placement.noteIndex < placement.noteCount - 1 || rangeGuideContinuationRows.has(row.key) + ? "continue" + : "terminate", }); }); } diff --git a/src/ui/diff/rowWindowing.test.ts b/src/ui/diff/rowWindowing.test.ts index 1ba9eb794..08df5fe6d 100644 --- a/src/ui/diff/rowWindowing.test.ts +++ b/src/ui/diff/rowWindowing.test.ts @@ -37,6 +37,7 @@ function createTestSectionGeometry( bodyHeight, hunkAnchorRows: new Map(), hunkBounds: new Map(), + hunkSpans: [], lineNumberDigits: 1, plannedRows, rowBounds: normalizedRowBounds, diff --git a/src/ui/fileViews/geometry.ts b/src/ui/fileViews/geometry.ts index fc0c5b804..997125e75 100644 --- a/src/ui/fileViews/geometry.ts +++ b/src/ui/fileViews/geometry.ts @@ -104,6 +104,7 @@ export function measureFileViewGeometry({ bodyHeight, hunkAnchorRows, hunkBounds, + hunkSpans: [], lineNumberDigits: 1, // Alternate rows are not Pierre rows, so raw copy selection intentionally remains unavailable. plannedRows: [], diff --git a/src/ui/hooks/useAppKeyboardShortcuts.ts b/src/ui/hooks/useAppKeyboardShortcuts.ts index ac288555c..6d52d162f 100644 --- a/src/ui/hooks/useAppKeyboardShortcuts.ts +++ b/src/ui/hooks/useAppKeyboardShortcuts.ts @@ -35,6 +35,7 @@ export interface UseAppKeyboardShortcutsOptions { * order. Modal navigation stays in this hook; commands own the rest. */ commands: readonly AppCommand[]; + clearVisualSelection?: () => boolean; denyRepoExtensions: () => void; /** The extension dialog currently on screen, or `null` when none is. */ extensionDialog: ExtensionDialogRequest | null; @@ -106,6 +107,7 @@ export function useAppKeyboardShortcuts({ closeThemeSelector, closeExtensionTrustPrompt, commands, + clearVisualSelection, denyRepoExtensions, extensionDialog, acceptExtensionDialog, @@ -137,6 +139,7 @@ export function useAppKeyboardShortcuts({ }: UseAppKeyboardShortcutsOptions) { const activeMenuIdRef = useRef(activeMenuId); const commandsRef = useRef(commands); + const clearVisualSelectionRef = useRef(clearVisualSelection); const focusAreaRef = useRef(focusArea); const showAgentSkillRef = useRef(showAgentSkill); const showHelpRef = useRef(showHelp); @@ -160,6 +163,7 @@ export function useAppKeyboardShortcuts({ activeMenuIdRef.current = activeMenuId; commandsRef.current = commands; + clearVisualSelectionRef.current = clearVisualSelection; focusAreaRef.current = focusArea; showAgentSkillRef.current = showAgentSkill; showHelpRef.current = showHelp; @@ -615,6 +619,12 @@ export function useAppKeyboardShortcuts({ ); if (reviewOwned) return; + // Clear only when a selection is active; otherwise Escape remains available to an + // extension command because Clear Selection no longer owns a global binding. + if (isEscapeKey(key) && clearVisualSelectionRef.current?.()) { + consumeKey(key); + return; + } dispatchCommandShortcut(key); }); } diff --git a/src/ui/hooks/useTerminalReview.ts b/src/ui/hooks/useTerminalReview.ts index ac562a32f..883bb3b32 100644 --- a/src/ui/hooks/useTerminalReview.ts +++ b/src/ui/hooks/useTerminalReview.ts @@ -23,7 +23,6 @@ import { buildLiveComment, findDiffFileByPath, resolveCommentTarget, - type UserNoteLineTarget, } from "../../core/liveComments"; import { builtinAppCommand, @@ -50,6 +49,7 @@ import { selectVisibleThreadedStoredReviewNotes, } from "../../core/review/selectors"; import { REVIEW_VIEWPORT_ANCHOR_REVEAL, type ReviewRevealRequest } from "../../core/review/state"; +import type { ReviewNoteTargetV1 } from "../../core/review/types"; import { createReviewStore, type ReviewStore } from "../../core/review/store"; import { noDiffFileMatchesMessage } from "../../session/agent/errors"; import type { DiffFile } from "../../core/changeset/model"; @@ -188,6 +188,8 @@ function revealRequestFor(options?: ReviewSelectionOptions): ReviewRevealRequest export interface TerminalReview { allFiles: DiffFile[]; + /** Projected content and source identities keyed by runtime file id. */ + semanticFileIdentityByFileId: ReadonlyMap; /** * The semantic review store this controller owns. * @@ -273,7 +275,7 @@ export interface TerminalReview { startUserNote: ( fileId?: string, hunkIndex?: number, - target?: UserNoteLineTarget, + target?: ReviewNoteTargetV1, options?: { preserveViewport?: boolean }, ) => DraftReviewNote | null; setFilter: (value: string) => void; @@ -407,6 +409,19 @@ export function useTerminalReview({ () => new Map(document.files.map((file) => [file.runtimeId, file.key] as const)), [document], ); + const semanticFileIdentityByFileId = useMemo( + () => + new Map( + document.files.map( + (file) => + [ + file.runtimeId, + `${file.key}:${file.contentIdentity}:${file.sourceIdentity ?? ""}`, + ] as const, + ), + ), + [document], + ); const fileByKey = useMemo(() => { const byRuntimeId = new Map(files.map((file) => [file.id, file] as const)); return new Map( @@ -1291,7 +1306,7 @@ export function useTerminalReview({ ( fileId = selectedFile?.id, hunkIndex = selectedHunkIndex, - requestedTarget?: UserNoteLineTarget, + requestedTarget?: ReviewNoteTargetV1, options?: { preserveViewport?: boolean }, ): DraftReviewNote | null => { const file = allFiles.find((candidate) => candidate.id === fileId); @@ -1512,6 +1527,7 @@ export function useTerminalReview({ return { allFiles, + semanticFileIdentityByFileId, store, stateRevision: state.stateRevision, draftNote, diff --git a/src/ui/hooks/useUserNoteComposer.test.tsx b/src/ui/hooks/useUserNoteComposer.test.tsx index f588393e1..340ee42a1 100644 --- a/src/ui/hooks/useUserNoteComposer.test.tsx +++ b/src/ui/hooks/useUserNoteComposer.test.tsx @@ -280,8 +280,8 @@ describe("useUserNoteComposer", () => { let reviewFocusCount = 0; const harness = await renderComposer( baseOptions({ - draftNote, - saveDraft: () => (++saveCount === 1 ? savedNote : null), + draftNote: { ...draftNote, newRange: [41, 43] }, + saveDraft: () => (++saveCount === 1 ? { ...savedNote, newRange: [41, 43] } : null), focus: { draft: () => {}, review: () => { @@ -312,6 +312,7 @@ describe("useUserNoteComposer", () => { hunkIndex: 1, side: "new", line: 42, + newRange: [41, 43], body: "saved body", draft: false, }, @@ -369,7 +370,7 @@ describe("useUserNoteComposer", () => { const events: Array<{ event: string; payload: unknown }> = []; const harness = await renderComposer( baseOptions({ - draftNote, + draftNote: { ...draftNote, newRange: [41, 43] }, updateDraft: (body) => bodies.push(body), publishEvent: (event, payload) => events.push({ event, payload }), }), @@ -390,6 +391,7 @@ describe("useUserNoteComposer", () => { hunkIndex: 1, side: "new", line: 42, + newRange: [41, 43], body: "current editor body", draft: true, }, @@ -543,6 +545,9 @@ describe("projectExtensionReviewNote", () => { body: draftNote.body, draft: true, }); + expect( + projectExtensionReviewNote({ ...draftNote, oldRange: [40, 42], newRange: [41, 43] }, true), + ).toMatchObject({ oldRange: [40, 42], newRange: [41, 43] }); expect( projectExtensionReviewNote( { ...savedNote, parentId: "user:parent", fileId: "runtime-alpha" }, diff --git a/src/ui/hooks/useUserNoteComposer.ts b/src/ui/hooks/useUserNoteComposer.ts index 59cdbfa04..bcb80e290 100644 --- a/src/ui/hooks/useUserNoteComposer.ts +++ b/src/ui/hooks/useUserNoteComposer.ts @@ -3,7 +3,7 @@ * Semantic draft and saved-note transitions remain owned by the terminal review controller. */ import { useCallback, useState } from "react"; -import type { UserNoteLineTarget } from "../../core/liveComments"; +import type { ReviewNoteTargetV1 } from "../../core/review/types"; import type { ExtensionEventPayloads, ExtensionReviewNote } from "../../extensions/types"; import type { ActiveAddNoteAffordance } from "../diff/DiffSectionBody"; import type { LineCursor } from "../lib/lineCursors"; @@ -13,7 +13,15 @@ type ActiveAddNoteTarget = ActiveAddNoteAffordance & { fileId: string }; type UserNoteEventPayloads = Pick; type ProjectableReviewNote = Pick< DraftReviewNote, - "id" | "fileId" | "filePath" | "hunkIndex" | "side" | "line" | "parentId" + | "id" + | "fileId" + | "filePath" + | "hunkIndex" + | "side" + | "line" + | "parentId" + | "oldRange" + | "newRange" > & { body?: string; summary?: string; @@ -38,6 +46,8 @@ export function projectExtensionReviewNote( hunkIndex: note.hunkIndex, side: note.side, line: note.line, + ...(note.oldRange ? { oldRange: note.oldRange } : {}), + ...(note.newRange ? { newRange: note.newRange } : {}), body: note.body ?? note.summary ?? "", draft, }; @@ -51,7 +61,7 @@ export interface UseUserNoteComposerOptions { startDraft: ( fileId?: string, hunkIndex?: number, - target?: UserNoteLineTarget, + target?: ReviewNoteTargetV1, options?: { preserveViewport?: boolean }, ) => DraftReviewNote | null; startEdit?: (noteId: string, options?: { preserveViewport?: boolean }) => DraftReviewNote | null; @@ -89,7 +99,7 @@ export function useUserNoteComposer({ /** Start a draft at an explicit target, hovered affordance, or enabled line cursor. */ const startUserNote = useCallback( - (fileId?: string, hunkIndex?: number, target?: UserNoteLineTarget) => { + (fileId?: string, hunkIndex?: number, target?: ReviewNoteTargetV1) => { // Hover and the current line are fallbacks only for a fully implicit start. Any // explicit location fact must not inherit whichever row happened to remain hovered. const hasExplicitTarget = diff --git a/src/ui/lib/appCommands.test.ts b/src/ui/lib/appCommands.test.ts index 7e0ddd04b..ae1f9366c 100644 --- a/src/ui/lib/appCommands.test.ts +++ b/src/ui/lib/appCommands.test.ts @@ -268,7 +268,7 @@ describe("builtinCommandKeyDefaults", () => { "u", "ctrl+u", ]); - // The menu-only commands ship unbound, and are reported so users can bind them. + // Commands with contextual or menu routing ship unbound and remain user-bindable. expect( defaults .filter((entry) => entry.defaultKeys.length === 0) @@ -279,6 +279,7 @@ describe("builtinCommandKeyDefaults", () => { "hunk.review.alignCurrentLineBottom", "hunk.review.alignCurrentLineCenter", "hunk.review.alignCurrentLineTop", + "hunk.review.clearSelection", "hunk.review.nextAnnotatedFile", "hunk.review.previousAnnotatedFile", "hunk.view.applyFilePresentationToAllMatching", diff --git a/src/ui/lib/appCommands.ts b/src/ui/lib/appCommands.ts index 09160f14d..51f1f046c 100644 --- a/src/ui/lib/appCommands.ts +++ b/src/ui/lib/appCommands.ts @@ -130,6 +130,10 @@ export interface BuildAppCommandsOptions { stepDiffLine: (delta: number) => void; selectCursorLine: (style: CursorLine) => void; selectLayoutMode: (mode: LayoutMode) => void; + hasVisualSelection?: () => boolean; + startVisualSelection?: () => void; + copySelection?: () => void; + clearSelection?: () => void; startUserNote: () => void; toggleAgentNotes: () => void; toggleCopyDecorations: () => void; @@ -182,6 +186,15 @@ function builtinCommandHandlers( "hunk.app.openAgentSkill": { run: () => options.openAgentSkill() }, "hunk.app.toggleFocusArea": { run: () => options.toggleFocusArea() }, "hunk.review.focusFilter": { run: () => options.focusFilter() }, + "hunk.review.startVisualSelection": { run: () => options.startVisualSelection?.() }, + "hunk.review.copySelection": { + isEnabled: () => options.hasVisualSelection?.() ?? false, + run: () => options.copySelection?.(), + }, + "hunk.review.clearSelection": { + isEnabled: () => options.hasVisualSelection?.() ?? false, + run: () => options.clearSelection?.(), + }, "hunk.review.startNote": { run: () => options.startUserNote() }, "hunk.review.editActiveNote": { isEnabled: () => Boolean(options.canEditActiveNote), @@ -335,6 +348,10 @@ const NOOP_COMMAND_OPTIONS: BuildAppCommandsOptions = (() => { stepDiffLine: noop, selectCursorLine: noop, selectLayoutMode: noop, + hasVisualSelection: () => false, + startVisualSelection: noop, + copySelection: noop, + clearSelection: noop, startUserNote: noop, toggleAgentNotes: noop, toggleCopyDecorations: noop, diff --git a/src/ui/lib/appMenus.test.ts b/src/ui/lib/appMenus.test.ts index a738816da..98164ea38 100644 --- a/src/ui/lib/appMenus.test.ts +++ b/src/ui/lib/appMenus.test.ts @@ -286,7 +286,8 @@ describe("the Extensions menu", () => { ]); expect(items(menus.extensions).map((item) => [item.label, item.hint])).toEqual([ - ["Sync notes", "y"], + // Built-in Copy Selection owns y; extension commands remain menu-invocable when unbound. + ["Sync notes", undefined], ["Stash notes", undefined], ["Quiet mode", undefined], ]); diff --git a/src/ui/lib/extensionCommands.test.ts b/src/ui/lib/extensionCommands.test.ts index 4f3caef36..3a4264985 100644 --- a/src/ui/lib/extensionCommands.test.ts +++ b/src/ui/lib/extensionCommands.test.ts @@ -26,7 +26,7 @@ describe("buildExtensionAppCommands", () => { test("adapts bound commands into dispatchable review-scope entries", () => { const ran: string[] = []; const { commands, conflicts } = buildExtensionAppCommands({ - registered: [registeredCommand("meta", "toggle", "y"), registeredCommand("meta", "silent")], + registered: [registeredCommand("meta", "toggle", "Y"), registeredCommand("meta", "silent")], builtins: builtinCommandMatchProbes(), runCommand: (registered) => ran.push(`${registered.extensionId}.${registered.command.id}`), }); @@ -34,23 +34,23 @@ describe("buildExtensionAppCommands", () => { expect(conflicts).toEqual([]); // Both are listed for the Extensions menu; only the bound one has a key. expect(commands.map((command) => command.id)).toEqual(["meta.toggle", "meta.silent"]); - expect(commands.map((command) => command.keyLabels)).toEqual([["y"], []]); + expect(commands.map((command) => command.keyLabels)).toEqual([["Y"], []]); expect(commands.every((command) => !command.publicToExtensions)).toBe(true); - expect(dispatchAppCommand(commands, chordEvent("y"))?.id).toBe("meta.toggle"); + expect(dispatchAppCommand(commands, chordEvent("Y"))?.id).toBe("meta.toggle"); expect(ran).toEqual(["meta.toggle"]); }); test("refuses chords owned by built-in shortcuts", () => { const { commands, conflicts } = buildExtensionAppCommands({ // "s" toggles the files pane and "[" is hunk navigation; both are taken. - registered: [registeredCommand("meta", "steal-s", "s"), registeredCommand("meta", "ok", "y")], + registered: [registeredCommand("meta", "steal-s", "s"), registeredCommand("meta", "ok", "Y")], builtins: builtinCommandMatchProbes(), runCommand: () => {}, }); // The refused command stays in the table, just without the key it wanted. expect(commands.map((command) => command.id)).toEqual(["meta.steal-s", "meta.ok"]); - expect(commands.map((command) => command.keyLabels)).toEqual([[], ["y"]]); + expect(commands.map((command) => command.keyLabels)).toEqual([[], ["Y"]]); expect(conflicts).toEqual([ { extensionId: "meta", @@ -64,15 +64,15 @@ describe("buildExtensionAppCommands", () => { test("resolves chords between extensions by load order", () => { const { commands, conflicts } = buildExtensionAppCommands({ registered: [ - registeredCommand("first", "mine", "y"), - registeredCommand("second", "mine", "y"), + registeredCommand("first", "mine", "Y"), + registeredCommand("second", "mine", "Y"), ], builtins: builtinCommandMatchProbes(), runCommand: () => {}, }); expect(commands.map((command) => command.id)).toEqual(["first.mine", "second.mine"]); - expect(commands.map((command) => command.keyLabels)).toEqual([["y"], []]); + expect(commands.map((command) => command.keyLabels)).toEqual([["Y"], []]); expect(conflicts.map((conflict) => conflict.fullId)).toEqual(["second.mine"]); expect(conflicts[0]?.conflictingId).toBe("first.mine"); }); @@ -80,7 +80,7 @@ describe("buildExtensionAppCommands", () => { test("binds one command to every chord it declares", () => { const ran: string[] = []; const { commands, conflicts } = buildExtensionAppCommands({ - registered: [registeredCommand("meta", "toggle", ["y", "ctrl+o"])], + registered: [registeredCommand("meta", "toggle", ["Y", "ctrl+o"])], builtins: builtinCommandMatchProbes(), runCommand: (registered) => ran.push(`${registered.extensionId}.${registered.command.id}`), }); @@ -88,16 +88,16 @@ describe("buildExtensionAppCommands", () => { expect(conflicts).toEqual([]); // One command, one dispatch entry, matching either chord. expect(commands).toHaveLength(1); - expect(commands[0]?.keyLabels).toEqual(["y", "Ctrl+O"]); - expect(dispatchAppCommand(commands, chordEvent("y"))?.id).toBe("meta.toggle"); + expect(commands[0]?.keyLabels).toEqual(["Y", "Ctrl+O"]); + expect(dispatchAppCommand(commands, chordEvent("Y"))?.id).toBe("meta.toggle"); expect(dispatchAppCommand(commands, chordEvent("ctrl+o"))?.id).toBe("meta.toggle"); expect(ran).toEqual(["meta.toggle", "meta.toggle"]); }); test("drops only the conflicting chord of a multi-key command", () => { const { commands, conflicts } = buildExtensionAppCommands({ - // "s" toggles the files pane; "y" is free. - registered: [registeredCommand("meta", "toggle", ["s", "y"])], + // "s" toggles the files pane; "Y" is free. + registered: [registeredCommand("meta", "toggle", ["s", "Y"])], builtins: builtinCommandMatchProbes(), runCommand: () => {}, }); @@ -112,20 +112,20 @@ describe("buildExtensionAppCommands", () => { ]); // The command stays registered and keeps the chord nobody else owns. expect(commands.map((command) => command.id)).toEqual(["meta.toggle"]); - expect(dispatchAppCommand(commands, chordEvent("y"))?.id).toBe("meta.toggle"); + expect(dispatchAppCommand(commands, chordEvent("Y"))?.id).toBe("meta.toggle"); }); test("a user keybinding replaces the chords an extension declared", () => { const resolvedKeys = new Map([["meta.toggle", ["ctrl+j"]]]); const { commands } = buildExtensionAppCommands({ - registered: [registeredCommand("meta", "toggle", "y")], + registered: [registeredCommand("meta", "toggle", "Y")], builtins: builtinCommandMatchProbes(), resolvedKeys, runCommand: () => {}, }); expect(dispatchAppCommand(commands, chordEvent("ctrl+j"))?.id).toBe("meta.toggle"); - expect(dispatchAppCommand(commands, chordEvent("y"))).toBeUndefined(); + expect(dispatchAppCommand(commands, chordEvent("Y"))).toBeUndefined(); }); test("a chord a built-in released is free for an extension to claim", () => { diff --git a/src/ui/lib/reviewNoteMapping.ts b/src/ui/lib/reviewNoteMapping.ts index 941295b7f..9e0d0cb94 100644 --- a/src/ui/lib/reviewNoteMapping.ts +++ b/src/ui/lib/reviewNoteMapping.ts @@ -172,7 +172,7 @@ export function storedNoteToUserNote( /** Render the semantic draft as the draft the diff pane places and edits. */ export function storedDraftToDraftNote(draft: ReviewDraftNote, file: DiffFile): DraftReviewNote { - const anchor = reviewLineAnchor(file.metadata.hunks, draft); + const anchor = draft.anchor ?? reviewLineAnchor(file.metadata.hunks, draft); return { id: draft.id, kind: draft.kind ?? "create", diff --git a/test/pty/cursor-line.test.ts b/test/pty/cursor-line.test.ts index 91b2154c1..97ad06aad 100644 --- a/test/pty/cursor-line.test.ts +++ b/test/pty/cursor-line.test.ts @@ -138,6 +138,7 @@ describe("PTY current line", () => { } session.writeRaw(`\x1b[<0;31;${endRow + 1}m`); + await session.press("y"); await session.waitForText(/Copied selection to clipboard/, { timeout: 5_000 }); } finally { session.close(); diff --git a/test/pty/extensions-integration.test.ts b/test/pty/extensions-integration.test.ts index ac4467832..d35e32b65 100644 --- a/test/pty/extensions-integration.test.ts +++ b/test/pty/extensions-integration.test.ts @@ -90,7 +90,7 @@ export default function (hunk) { }); }, }); - hunk.registerCommand({ id: "toggle-fixture", title: "Toggle fixture", key: "y" }, (ctx) => { + hunk.registerCommand({ id: "toggle-fixture", title: "Toggle fixture", key: "Y" }, (ctx) => { ctx.sidebars.toggle("fixture-sidebar"); }); } @@ -135,7 +135,7 @@ export default function (hunk) { }), }); } - hunk.registerCommand({ id: "toggle-edges", title: "Toggle edge panes", key: "y" }, (ctx) => { + hunk.registerCommand({ id: "toggle-edges", title: "Toggle edge panes", key: "Y" }, (ctx) => { ctx.panes.toggle("top"); ctx.panes.toggle("bottom"); }); @@ -245,7 +245,7 @@ export default function (hunk) { `; const DIALOG_EXTENSION_SOURCE = `export default function (hunk) { - hunk.registerCommand({ id: "ask", title: "Ask", key: "y" }, async (ctx) => { + hunk.registerCommand({ id: "ask", title: "Ask", key: "Y" }, async (ctx) => { const proceed = await ctx.dialogs.confirm({ title: "Reformat the changeset?", body: "Nothing is written to disk. This deliberately long explanation wraps across many terminal rows while the actions remain pinned below it.", @@ -431,7 +431,7 @@ describe("PTY extensions", () => { await session.click(/Extensions/); // The dropdown names the command by its title and advertises its key. const menu = await session.waitForText(/Toggle fixture/, { timeout: 20_000 }); - expect(menu).toMatch(/Toggle fixture\s+y/); + expect(menu).toMatch(/Toggle fixture\s+Y/); await session.click(/Toggle fixture/); const opened = await session.waitForText(/EXTSIDEBAR 2 FILES/, { timeout: 20_000 }); @@ -475,12 +475,12 @@ describe("PTY extensions", () => { // The registered key dispatches through the shared command table and // opens the extension's right-hand pane beside the built-in one. - await session.press("y"); + session.writeRaw("Y"); const opened = await session.waitForText(/EXTSIDEBAR 2 FILES/, { timeout: 20_000 }); expect(opened).toContain("alpha.ts"); // The same key toggles it away again. - await session.press("y"); + session.writeRaw("Y"); await harness.waitForSnapshot(session, (text) => !text.includes("EXTSIDEBAR"), 20_000); } finally { session.close(); @@ -554,7 +554,7 @@ describe("PTY extensions", () => { }); try { await harness.ensureKeyboardIsLive(session); - await session.press("y"); + session.writeRaw("Y"); const frame = await harness.waitForSnapshot( session, (text) => @@ -569,7 +569,7 @@ describe("PTY extensions", () => { await dragMouse(session, 70, 4, 70, 6); await session.waitForText(/PANE TOP 138x4/, { timeout: 5_000 }); - await session.press("y"); + session.writeRaw("Y"); await harness.waitForSnapshot( session, (text) => !text.includes("PANE TOP") && !text.includes("PANE BOTTOM"), @@ -607,7 +607,7 @@ describe("PTY extensions", () => { expect(before).not.toContain("Reformat the changeset?"); await harness.ensureKeyboardIsLive(session); - await session.press("y"); + session.writeRaw("Y"); const prompt = await harness.waitForSnapshot( session, (text) => text.includes("Reformat the changeset?"), @@ -774,7 +774,7 @@ describe("PTY extensions", () => { } } expect(menu).not.toBeNull(); - expect(menu!).toMatch(/Toggle review triage\s+y/); + expect(menu!).toMatch(/Toggle review triage\s+Y/); expect(menu).toMatch(/Mark selected hunk…\s+x/); expect(menu).toContain("Center current review line"); expect(menu).toContain("Set review focus…"); diff --git a/test/pty/notes.test.ts b/test/pty/notes.test.ts index bd2dab034..1c9e12aa3 100644 --- a/test/pty/notes.test.ts +++ b/test/pty/notes.test.ts @@ -1,6 +1,7 @@ import { afterEach, describe, expect, setDefaultTimeout, test } from "bun:test"; import { createPtyHarness, + dragMouse, lineIndexOf, moveMouse, revealAddNoteAffordance, @@ -378,6 +379,39 @@ describe("PTY notes", () => { } }); + test("mouse range across replacement sides opens and saves the exact multiline draft", async () => { + const fixture = harness.createWideCharacterFilePair(); + const session = await harness.launchHunk({ + args: ["diff", fixture.before, fixture.after, "--mode", "stack"], + cols: 110, + rows: 22, + }); + + try { + const initial = await session.waitForText(/export const plain = 'after';/, { + timeout: 15_000, + }); + const oldEndRow = lineIndexOf(initial, "export const plain = 'before';") - 1; + const newStartRow = lineIndexOf(initial, "export const wide = '한국어';") - 1; + expect(oldEndRow).toBeGreaterThan(0); + expect(newStartRow).toBe(oldEndRow + 1); + + await dragMouse(session, 12, oldEndRow, 24, newStartRow); + await session.waitForText(/c Comment\s+y Copy\s+Esc Clear/, { timeout: 5_000 }); + await session.press("c"); + const draft = await session.waitForText(/Draft note/, { timeout: 5_000 }); + expect(draft).toContain("L2 → R1"); + + await session.type("Mixed replacement feedback."); + await session.type("\x13"); + const saved = await session.waitForText(/Your note/, { timeout: 5_000 }); + expect(saved).toContain("L2 → R1"); + expect(saved).toContain("Mixed replacement feedback."); + } finally { + session.close(); + } + }); + test("CJK draft notes wrap instead of scrolling out of view in a real PTY", async () => { const fixture = harness.createLongWrapFilePair(); const session = await harness.launchHunk({