From 13e622d77def788eeeb59b6ca3e4113a8d09749e Mon Sep 17 00:00:00 2001 From: Michael Hackner Date: Fri, 2 Oct 2026 12:31:50 -0700 Subject: [PATCH] fix(ui): stop wheel scroll and stale hover from drifting comment targets off the highlighted line A 'c' comment could open its draft a few lines (or one collapsed gap) away from the highlighted line. Two paths let the line cursor drift away from the current line: - Wheeling past the highlighted line clamped the cursor to a viewport edge, and wheeling back never restored it; the strand survived the whole roundtrip and deepened with distance. - A hover add-note affordance stayed armed after the cursor stepped away, so the next implicit comment targeted the hovered row instead of the highlight. The clamp now remembers the stop it displaced, unwinding one hop per scroll step back, and a deliberate cursor move ends the unwind; cursor movement retires a stale hover. Keyboard-only commenting was always exact and is unchanged. A vertical drag across exactly two rows was also swallowed: the click slop that ignores one cell of pointer jitter applied to the row axis, so a two-row gesture read as a jittered click and painted no selection or action bar. The slop is now horizontal only. Co-authored-by: GLM-5.3-Flash --- .changeset/wheel-roundtrip-comment-target.md | 5 + packages/hunk/src/ui/App.tsx | 7 + .../hunk/src/ui/AppHost.interactions.test.tsx | 58 ++++ .../hunk/src/ui/components/panes/DiffPane.tsx | 24 ++ .../ui/components/panes/copySelection.test.ts | 21 +- .../src/ui/components/panes/copySelection.ts | 10 +- packages/hunk/src/ui/lib/lineCursors.test.ts | 26 ++ packages/hunk/src/ui/lib/lineCursors.ts | 23 ++ test/pty/comment-target.test.ts | 248 ++++++++++++++++++ 9 files changed, 418 insertions(+), 4 deletions(-) create mode 100644 .changeset/wheel-roundtrip-comment-target.md create mode 100644 test/pty/comment-target.test.ts 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(); + } + }); +});