diff --git a/.changeset/wheel-roundtrip-comment-target.md b/.changeset/wheel-roundtrip-comment-target.md
new file mode 100644
index 000000000..9d290bcd2
--- /dev/null
+++ b/.changeset/wheel-roundtrip-comment-target.md
@@ -0,0 +1,5 @@
+---
+"hunkdiff": patch
+---
+
+Fix stale comment targets after scrolling or stepping the cursor.
diff --git a/packages/hunk/src/ui/App.tsx b/packages/hunk/src/ui/App.tsx
index cc5499283..6b5db5ec0 100644
--- a/packages/hunk/src/ui/App.tsx
+++ b/packages/hunk/src/ui/App.tsx
@@ -1261,6 +1261,13 @@ export function App({
const activeReplyableNoteId = selectActiveReplyableReviewNoteId(reviewSnapshot);
const activeRemovableNote = selectActiveRemovableReviewNote(reviewSnapshot);
+ // The current line owns implicit comment targeting. A hover affordance armed at an
+ // earlier pointer position retires whenever the cursor moves, so stepping or scrolling
+ // cannot leave a stale hover target between the visible highlight and the next note.
+ useEffect(() => {
+ onActiveAddNoteAffordanceChange(null);
+ }, [onActiveAddNoteAffordanceChange, review.lineCursor]);
+
// One dispatch table for every app-level shortcut: the built-in commands
// over App's live callbacks, then extension commands, so built-ins always
// win a key and extension order follows load order.
diff --git a/packages/hunk/src/ui/AppHost.interactions.test.tsx b/packages/hunk/src/ui/AppHost.interactions.test.tsx
index 74e2d6360..db78100ce 100644
--- a/packages/hunk/src/ui/AppHost.interactions.test.tsx
+++ b/packages/hunk/src/ui/AppHost.interactions.test.tsx
@@ -3039,6 +3039,64 @@ describe("App interactions", () => {
}
});
+ test("stepping the cursor retires a stale hover before the next draft opens", async () => {
+ const setup = await testRender(
+ ,
+ {
+ width: 120,
+ height: 26,
+ },
+ );
+
+ try {
+ await flush(setup);
+ await act(async () => {
+ await Bun.sleep(60);
+ await setup.renderOnce();
+ });
+
+ // Hover the row showing line02 so its add-note affordance arms.
+ const frame = setup.captureCharFrame();
+ const hover = frame
+ .split("\n")
+ .map((line, index) => [index, line] as const)
+ .find(([, line]) => line.includes("export const line02 = 2;"));
+ expect(hover).toBeDefined();
+ await act(async () => {
+ await setup.mockMouse.moveTo(10, hover![0]);
+ await setup.renderOnce();
+ });
+ await waitForFrame(setup, (next) => next.includes("[+]"));
+
+ // Step the cursor down several lines; the pointer stays where it was.
+ await act(async () => {
+ await setup.mockInput.pressKeys(["\x1b[B", "\x1b[B", "\x1b[B"]);
+ });
+ await flush(setup);
+ await act(async () => {
+ await Bun.sleep(60);
+ await setup.renderOnce();
+ });
+
+ // The draft must anchor at the stepped cursor, not the stale hover row.
+ await act(async () => {
+ await setup.mockInput.pressKey("c");
+ });
+ await flush(setup);
+ await act(async () => {
+ await Bun.sleep(60);
+ await setup.renderOnce();
+ });
+
+ const withDraft = setup.captureCharFrame();
+ expect(withDraft).toMatch(/Draft note.*L4/);
+ } finally {
+ await act(async () => {
+ setup.renderer.destroy();
+ });
+ }
+ });
+
test("draft note focus suppresses app shortcuts while accepting typed shortcut keys", async () => {
const setup = await testRender(, {
width: 240,
diff --git a/packages/hunk/src/ui/components/panes/DiffPane.tsx b/packages/hunk/src/ui/components/panes/DiffPane.tsx
index 45afc9f10..686477c3b 100644
--- a/packages/hunk/src/ui/components/panes/DiffPane.tsx
+++ b/packages/hunk/src/ui/components/panes/DiffPane.tsx
@@ -884,6 +884,12 @@ export function DiffPane({
// Initialized to null so the first render never fires a selection change; a real scroll
// is required before passive viewport-follow selection can trigger.
const lastViewportSelectionTopRef = useRef(null);
+ // Stops that successive clamps moved the current line away from, innermost last, so a
+ // multi-hop wheel roundtrip can unwind back to the pre-clamp stop.
+ const lineCursorBeforeClampRef = useRef([]);
+ // The cursor this effect last acted on, so deliberate cursor moves outside a clamp reset
+ // the unwind memory above instead of leaving a displaced stop behind.
+ const lastClampCursorRef = useRef(null);
const lastViewportRowAnchorRef = useRef(null);
// Track the previous selected anchor to detect actual selection changes.
const prevSelectedAnchorIdRef = useRef(null);
@@ -1618,15 +1624,33 @@ export function DiffPane({
return;
}
+ // A deliberate cursor move (a click, a step) ends any clamp unwind: the reviewer chose
+ // a new stop, so a later scroll must not restore the displaced one.
+ if (lineCursor !== lastClampCursorRef.current) {
+ lineCursorBeforeClampRef.current = [];
+ lastClampCursorRef.current = lineCursor;
+ }
+
if (lineCursors.length > 0) {
+ // Each clamp away from the current stop pushes where it came from, and each restore
+ // pops it back, so a multi-hop wheel roundtrip unwinds to the pre-clamp stop instead
+ // of parking on an edge the scroll crossed.
+ const clampedFrom = lineCursorBeforeClampRef.current.at(-1) ?? null;
const clampedCursor = clampLineCursorToViewport({
boundsOf: lineCursorBoundsOf,
current: lineCursor,
+ clampedFrom,
cursors: lineCursors,
scrollTop: scrollViewport.top,
viewportHeight: scrollViewport.height,
});
if (clampedCursor && clampedCursor !== lineCursor) {
+ if (clampedCursor === clampedFrom) {
+ lineCursorBeforeClampRef.current.pop();
+ } else if (lineCursor) {
+ lineCursorBeforeClampRef.current.push(lineCursor);
+ }
+ lastClampCursorRef.current = clampedCursor;
onViewportLineCursorChange?.(clampedCursor);
}
return;
diff --git a/packages/hunk/src/ui/components/panes/copySelection.test.ts b/packages/hunk/src/ui/components/panes/copySelection.test.ts
index 0fa1c42a7..daa12d1f7 100644
--- a/packages/hunk/src/ui/components/panes/copySelection.test.ts
+++ b/packages/hunk/src/ui/components/panes/copySelection.test.ts
@@ -996,16 +996,33 @@ describe("copySelectionDragIsClick", () => {
visualRow,
});
- test("accepts one-cell mouse jitter around a click", () => {
+ test("accepts one-cell mouse jitter within the clicked row", () => {
expect(
copySelectionDragIsClick({
anchor: point(20, 8),
- focus: point(21, 9),
+ focus: point(21, 8),
moved: true,
}),
).toBe(true);
});
+ test("treats a drag onto a second visual row as a selection, not a jittered click", () => {
+ expect(
+ copySelectionDragIsClick({
+ anchor: point(20, 8),
+ focus: point(21, 9),
+ moved: true,
+ }),
+ ).toBe(false);
+ expect(
+ copySelectionDragIsClick({
+ anchor: point(20, 8),
+ focus: point(20, 9),
+ moved: true,
+ }),
+ ).toBe(false);
+ });
+
test("rejects deliberate drags and double-click expansion", () => {
expect(
copySelectionDragIsClick({
diff --git a/packages/hunk/src/ui/components/panes/copySelection.ts b/packages/hunk/src/ui/components/panes/copySelection.ts
index 352e0afa9..7668d6088 100644
--- a/packages/hunk/src/ui/components/panes/copySelection.ts
+++ b/packages/hunk/src/ui/components/panes/copySelection.ts
@@ -169,7 +169,13 @@ export function findLineCursorForClick({
const COPY_SELECTION_CLICK_SLOP_CELLS = 1;
-/** Treat one-cell pointer jitter as a click while preserving deliberate and expanded drags. */
+/**
+ * Treat pointer jitter around a click as a click while preserving deliberate drags.
+ *
+ * Jitter slop is horizontal only: a drag that reaches a different visual row is a
+ * deliberate selection, so a two-row range commits instead of being swallowed as a
+ * jittered click. Double-click expansion always copies.
+ */
export function copySelectionDragIsClick(drag: CopySelectionDrag) {
if (drag.expanded) {
return false;
@@ -197,7 +203,7 @@ export function copySelectionDragIsClick(drag: CopySelectionDrag) {
return (
drag.anchor.kind === "review-row" &&
drag.focus.kind === "review-row" &&
- Math.abs(drag.anchor.visualRow - drag.focus.visualRow) <= COPY_SELECTION_CLICK_SLOP_CELLS
+ drag.anchor.visualRow === drag.focus.visualRow
);
}
diff --git a/packages/hunk/src/ui/lib/lineCursors.test.ts b/packages/hunk/src/ui/lib/lineCursors.test.ts
index 53ecba141..043b0cb48 100644
--- a/packages/hunk/src/ui/lib/lineCursors.test.ts
+++ b/packages/hunk/src/ui/lib/lineCursors.test.ts
@@ -491,4 +491,30 @@ describe("clampLineCursorToViewport", () => {
}),
).toEqual(cursors[0]!);
});
+
+ test("restores the remembered stop once the viewport returns to it", () => {
+ expect(
+ clampLineCursorToViewport({
+ boundsOf,
+ current: cursors[2]!,
+ clampedFrom: cursors[0]!,
+ cursors,
+ scrollTop: 0,
+ viewportHeight: 3,
+ }),
+ ).toEqual(cursors[0]!);
+ });
+
+ test("keeps the clamped stop while the remembered one is still off-screen", () => {
+ expect(
+ clampLineCursorToViewport({
+ boundsOf,
+ current: cursors[2]!,
+ clampedFrom: cursors[0]!,
+ cursors,
+ scrollTop: 2,
+ viewportHeight: 2,
+ }),
+ ).toEqual(cursors[2]!);
+ });
});
diff --git a/packages/hunk/src/ui/lib/lineCursors.ts b/packages/hunk/src/ui/lib/lineCursors.ts
index 34f76a6ba..31852feab 100644
--- a/packages/hunk/src/ui/lib/lineCursors.ts
+++ b/packages/hunk/src/ui/lib/lineCursors.ts
@@ -302,18 +302,41 @@ export function clampLineCursorToViewport({
cursors,
scrollTop,
viewportHeight,
+ clampedFrom,
}: {
boundsOf: LineCursorBoundsLookup;
current: LineCursor | null;
cursors: LineCursor[];
scrollTop: number;
viewportHeight: number;
+ /**
+ * The stop the current line was on before the clamp moved it, when the caller knows it.
+ *
+ * Wheel roundtrips move the viewport past a line and back; without this memory the clamp
+ * re-adopts an edge stop on the way out and then keeps it on the way home, so the comment
+ * target ends up a few lines (or one gap) away from the line the reviewer is reading.
+ */
+ clampedFrom?: LineCursor | null;
}): LineCursor | null {
if (cursors.length === 0 || viewportHeight <= 0) {
return current;
}
const viewportBottom = scrollTop + viewportHeight;
+
+ // A return journey restores the stop the last clamp moved away from, when that stop is
+ // visible again and the viewport only moves while the pointer scrolls.
+ if (clampedFrom) {
+ const fromBounds = boundsOf(clampedFrom);
+ if (
+ fromBounds &&
+ fromBounds.top >= scrollTop &&
+ fromBounds.top + fromBounds.height <= viewportBottom
+ ) {
+ return clampedFrom;
+ }
+ }
+
const currentBounds = current ? boundsOf(current) : undefined;
if (
currentBounds &&
diff --git a/test/pty/comment-target.test.ts b/test/pty/comment-target.test.ts
new file mode 100644
index 000000000..a398ebf1e
--- /dev/null
+++ b/test/pty/comment-target.test.ts
@@ -0,0 +1,248 @@
+import { afterEach, describe, expect, setDefaultTimeout, test } from "bun:test";
+import type { Session } from "tuistory";
+import { createPtyHarness, lineIndexOf, moveMouse, rowCellBackgrounds, sleep } from "./harness";
+
+const harness = createPtyHarness();
+
+/** Give PTY-backed startup and redraws enough headroom for slower CI machines. */
+setDefaultTimeout(20_000);
+
+afterEach(() => {
+ harness.cleanup();
+});
+
+/** Read the [LR]-line label the draft card reports, or null while none is visible. */
+function draftTarget(text: string): string | null {
+ return /Draft note[^ ]* - [^R]*([LR]\d+)/.exec(text)?.[1] ?? null;
+}
+
+/** Find the terminal row carrying the current-line highlight tint and return its text. */
+function highlightedRowText(session: Session): string | null {
+ // The two hexes are the runtime blends cursorLineHighlightBg paints over context and
+ // changed rows for the default theme; a theme or blend-ratio change invalidates them.
+ const cursorTints = ["#373737", "#2a2a2a"];
+ const data = session.getTerminalData();
+ for (const line of data.lines) {
+ const bgs = line.spans.map((span) => span.bg ?? "");
+ if (bgs.some((bg) => cursorTints.includes(bg))) {
+ return line.spans.map((span) => span.text).join("");
+ }
+ }
+ return null;
+}
+
+describe("PTY comment target", () => {
+ test("a wheel roundtrip does not move the comment target off the clicked line", async () => {
+ // The reported drift: click a line (cursor adopts it, pointer rests there),
+ // fidget the wheel down a few and back up, press 'c'. The viewport returns to
+ // where it started, but the line cursor can end up parked on a different stop —
+ // and the draft opens a line or two (or a whole gap) away from the clicked line.
+ const fixture = harness.createMultiHunkFilePair();
+ const session = await harness.launchHunk({
+ args: ["diff", "--files", fixture.before, fixture.after, "--mode", "split"],
+ cols: 120,
+ rows: 16,
+ });
+
+ try {
+ await session.waitForText(/line1 = 100/, { timeout: 15_000 });
+ await session.waitIdle({ timeout: 300 });
+
+ // Click the row showing line2 (terminal data row 6; SGR encodes +1).
+ session.writeRaw("\x1b[<0;9;7M");
+ session.writeRaw("\x1b[<0;9;7m");
+ await session.waitIdle({ timeout: 300 });
+ expect(highlightedRowText(session)).toContain("line2 = 2;");
+
+ // Fidget: wheel down 3, then up 3 — net zero viewport movement.
+ for (let i = 0; i < 3; i += 1) {
+ await session.scrollDown(1);
+ await session.waitIdle({ timeout: 120 });
+ }
+ for (let i = 0; i < 3; i += 1) {
+ await session.scrollUp(1);
+ await session.waitIdle({ timeout: 120 });
+ }
+ await session.waitIdle({ timeout: 400 });
+
+ // The viewport is back where it started with no deliberate cursor move in
+ // between. The highlight must still sit on line2 — and so must the draft.
+ const highlight = highlightedRowText(session);
+
+ await session.press("c");
+ await session.waitIdle({ timeout: 400 });
+ const text = await session.text({ immediate: true });
+ const target = draftTarget(text);
+
+ expect(highlight).toContain("line2 = 2;");
+ expect(target).toBe("R2");
+ } finally {
+ session.close();
+ }
+ });
+
+ test("a longer wheel roundtrip does not strand the cursor in a scrolled-away hunk", async () => {
+ const fixture = harness.createMultiHunkFilePair();
+ const session = await harness.launchHunk({
+ args: ["diff", "--files", fixture.before, fixture.after, "--mode", "split"],
+ cols: 120,
+ rows: 16,
+ });
+
+ try {
+ await session.waitForText(/line1 = 100/, { timeout: 15_000 });
+ await session.waitIdle({ timeout: 300 });
+
+ session.writeRaw("\x1b[<0;9;7M");
+ session.writeRaw("\x1b[<0;9;7m"); // click line2
+ await session.waitIdle({ timeout: 300 });
+
+ // Down 5, up 5: the roundtrip crosses the hunk boundary and back.
+ for (let i = 0; i < 5; i += 1) {
+ await session.scrollDown(1);
+ await session.waitIdle({ timeout: 120 });
+ }
+ for (let i = 0; i < 5; i += 1) {
+ await session.scrollUp(1);
+ await session.waitIdle({ timeout: 120 });
+ }
+ await session.waitIdle({ timeout: 400 });
+
+ await session.press("c");
+ await session.waitIdle({ timeout: 400 });
+ const text = await session.text({ immediate: true });
+ const target = draftTarget(text);
+
+ // The draft must not anchor in the second hunk (R5x) after the viewport
+ // returned to the first one — the classic "comment opened far away" report.
+ expect(target).toBe("R2");
+ } finally {
+ session.close();
+ }
+ });
+
+ test("stepping mid-roundtrip ends the unwind at the new stop", async () => {
+ // The unwind memory exists for wheel-only journeys: a deliberate cursor step
+ // between the two wheel legs ends the unwind, so the journey home must keep
+ // the stepped stop instead of restoring the pre-clamp stop.
+ const fixture = harness.createMultiHunkFilePair();
+ const session = await harness.launchHunk({
+ args: ["diff", "--files", fixture.before, fixture.after, "--mode", "split"],
+ cols: 120,
+ rows: 16,
+ });
+
+ try {
+ await session.waitForText(/line1 = 100/, { timeout: 15_000 });
+ await session.waitIdle({ timeout: 300 });
+
+ session.writeRaw("\x1b[<0;9;7M");
+ session.writeRaw("\x1b[<0;9;7m"); // click line2
+ await session.waitIdle({ timeout: 300 });
+
+ await session.scrollDown(3);
+ await session.waitIdle({ timeout: 200 });
+ await session.press("j");
+ await session.waitIdle({ timeout: 200 });
+ await session.scrollUp(3);
+ await session.waitIdle({ timeout: 400 });
+
+ await session.press("c");
+ await session.waitIdle({ timeout: 400 });
+ const text = await session.text({ immediate: true });
+
+ // The stepped stop owns the draft. The step lands on R4, not R3: the wheel
+ // leg down first clamped the cursor to a viewport-edge stop, and `j` stepped
+ // from there. What matters is that the unwind does not restore R2 — without
+ // the deliberate-move reset, this roundtrip opens the draft on the clicked
+ // line the reviewer stepped away from.
+ expect(draftTarget(text)).toBe("R4");
+ } finally {
+ session.close();
+ }
+ });
+
+ test("stepping the cursor retires a stale hover target for the next comment", async () => {
+ // Hovering arms an add-note affordance on the hovered row. Arrow keys and `j` move
+ // the current line but leave the pointer stationary, so the affordance used to stay
+ // armed at the old row and the next `c` opened the draft there instead of on the
+ // highlighted line the reviewer moved to.
+ const fixture = harness.createMultiHunkFilePair();
+ const session = await harness.launchHunk({
+ args: ["diff", "--files", fixture.before, fixture.after, "--mode", "split"],
+ cols: 120,
+ rows: 16,
+ });
+
+ try {
+ const initial = await session.waitForText(/line1 = 100/, { timeout: 15_000 });
+ await session.waitIdle({ timeout: 300 });
+
+ // Hover the row showing line2. The badge appears on the same row.
+ const hoverRow = initial.split("\n").findIndex((line) => line.includes("line2 = 2;"));
+ expect(hoverRow).toBeGreaterThan(0);
+ await moveMouse(session, 8, hoverRow - 1);
+ await session.waitForText(/\[\+\]/, { timeout: 5_000 });
+
+ // Step the cursor to line4 without moving the pointer or scrolling.
+ for (let step = 0; step < 3; step += 1) {
+ await session.press("j");
+ }
+ await session.waitIdle({ timeout: 300 });
+
+ // The highlight is on line3 after three steps from the initial old-side line1
+ // stop (the first step adopts line1's new side); the pointer still rests over
+ // line2's row. The current line owns the comment; the retired hover must not.
+ const draft = await harness.pressAndWaitForText(session, "c", /Draft note/, {
+ timeout: 5_000,
+ });
+ expect(draftTarget(draft)).toBe("R3");
+
+ // The retirement is not permanent: hovering a new row re-arms its affordance,
+ // on the hovered row itself.
+ const withDraft = await session.text({ immediate: true });
+ const newRow = withDraft.split("\n").findIndex((line) => line.includes("line4 = 4;"));
+ expect(newRow).toBeGreaterThan(0);
+ await moveMouse(session, 8, newRow - 1);
+ const rearmed = await session.waitForText(/\[\+\]/, { timeout: 5_000 });
+ const badgeRow = rearmed
+ .split("\n")
+ .findIndex((line) => line.includes("line4 = 4;") && line.includes("[+]"));
+ expect(badgeRow).toBe(newRow);
+ } finally {
+ session.close();
+ }
+ });
+
+ test("a drag across exactly two rows commits and shows the selection actions", async () => {
+ // A two-row vertical drag used to fall inside the click slop and was swallowed as a
+ // jittered click: no selection painted and no action bar. It is a deliberate gesture.
+ const fixture = harness.createScrollableFilePair();
+ const session = await harness.launchHunk({
+ args: ["diff", "--files", fixture.before, fixture.after, "--mode", "split"],
+ cols: 120,
+ rows: 16,
+ });
+
+ try {
+ const initial = await session.waitForText(/export const line06 = 6;/, { timeout: 15_000 });
+ await session.waitIdle({ timeout: 300 });
+ const startRow = lineIndexOf(initial, "export const line02 = 2;") - 1;
+ expect(startRow).toBeGreaterThan(0);
+
+ // Drag from line2's row to the adjacent row below, then release.
+ const before = rowCellBackgrounds(session, startRow);
+ session.writeRaw(`\x1b[<0;31;${startRow + 1}M`);
+ await sleep(20);
+ session.writeRaw(`\x1b[<32;31;${startRow + 2}M`);
+ await sleep(20);
+ session.writeRaw(`\x1b[<0;31;${startRow + 2}m`);
+ await session.waitForText(/c Comment\s+y Copy\s+Esc Clear/, { timeout: 5_000 });
+
+ // The first row's paint changed: the drag committed as a real selection.
+ expect(rowCellBackgrounds(session, startRow)).not.toEqual(before);
+ } finally {
+ session.close();
+ }
+ });
+});