fix(diff): ignore double-click in blank area outside diff lines

[FourWindff]

Summary

Double-clicking in the empty area below the last file (or between files) caused two glitches:

1. The browser's "snap-to-nearest-text" behavior collapsed the selection onto the last diff line, causing a stray blue highlight flash
2. That ghost selection also incorrectly opened the inline review/ask input

[johannesjo]

Thanks for the fix! The mousedown preventDefault direction is right — that's the correct way to kill the snap-flash. A few concerns about how the "is on a diff line" predicate is defined, though:

1. Removed lines are accidentally treated as "outside a diff line."

DiffLineView renders data-new-line={props.line.newLine ?? undefined} (ScrollingDiffView.tsx:172). For type: 'remove', newLine is null, so the attribute is omitted from the DOM. That means target.closest('[data-new-line]') returns null on a - line, and:

• The new mousedown handler preventDefault()s on a double-click on a removed line, which breaks word-select-to-copy on - lines (a common reason to double-click in a diff).
• The new mouseup early-return drops legitimate selections that end on a - line.

Suggested predicate: target?.closest('[data-line-type]') — every diff row carries that attribute regardless of side. getDiffSelection already filters lineType === 'remove' itself, so deferring to it stays correct.

2. mouseup.target ≠ selection range.

A user drag-selecting from line 5 down past the last hunk and releasing over the blank padding has mouseup.target = container/padding, but window.getSelection() still holds a valid selection. The early-return discards it. Keying off getDiffSelection() returning null instead of off the click target avoids that — and matches what the existing if (\!sel) branch already does.

3. The mouseup early-return may be redundant once mousedown is fixed.

With preventDefault on the double-click mousedown, the snap-selection never forms, so the existing if (\!sel) branch already clears the highlight (gated by pendingInput()). Worth verifying empirically — if it's redundant, dropping it sidesteps both regressions above and shrinks the diff.

Minor: e.detail >= 2 is correct (covers double + triple click), but a one-line note that this only fires on the second mousedown of the sequence (the first has detail === 1) would help future readers.

Listener pairing + cleanup and the "why" comments look good — just want to make sure removed-line interactions and drag-selects ending in blank space aren't collateral damage.

[FourWindff]

Applied all three suggestions:

1. data-new-line → data-line-type — fixes - lines being misclassified
2. Removed mouseup target check — redundant after the mousedown fix
3. Added comment explaining detail >= 2

[johannesjo]

Thanks for the quick turnaround — the three changes look correct. A few smaller follow-ups before merge:

1. Form-field carve-out (important). The new mousedown predicate prevents default on any double-click outside [data-line-type], including inside the <input> rendered by InlineInput (InlineInput.tsx:103) and the edit <input> in ReviewCommentCard (ReviewCommentCard.tsx:100) — both are siblings of diff rows inside containerRef. mousedown.preventDefault() cancels the default selection action regardless of where in the bubble chain it fires, so double-click word-select inside those inputs silently stops working. (This regression actually pre-existed your PR with the old [data-new-line] predicate — worth fixing now while the predicate is being touched.)

2. Gate on e.button. Snap-flash is a primary-button artifact, but the handler currently fires for middle/secondary too. preventDefault() on a non-primary mousedown can quietly suppress middle-click autoscroll (Win/Linux) and middle-click paste-from-primary-selection (Linux). The window is narrow but the carve-out is essentially free.

3. Type narrowing instead of as. e.target as HTMLElement | null is broader than needed and skips TS's checks (we're on strict: true). closest() is on Element, so narrowing is enough.

All three roll into something like:

function onMouseDown(e: MouseEvent) {
  if (e.button \!== 0 || e.detail < 2) return;
  const target = e.target;
  if (\!(target instanceof Element)) return;
  if (target instanceof HTMLElement && (
    target.tagName === 'INPUT' || target.tagName === 'TEXTAREA' || target.isContentEditable
  )) return;
  if (\!target.closest('[data-line-type]')) e.preventDefault();
}

Otherwise LGTM.

[FourWindff]

Thanks for the detailed review! I'm genuinely curious — how did you spot these edge cases? Would love to understand the thought process so I can catch similar issues myself next time. Thanks again!

[johannesjo]

Hey hey @FourWindff ! I am using claude code to do a code review and then I usually do a quick superficial check if the findings make any sense or not. A manual review would be better of course, but I simply don't have the time to do it super thourougly each time.

[brooksc]

Hey hey @FourWindff ! I am using claude code to do a code review and then I usually do a quick superficial check if the findings make any sense or not. A manual review would be better of course, but I simply don't have the time to do it super thourougly each time.

Would you mind sharing what your process or skill is? I can then do this before I submit a PR :)

FYI I've been using a combination of claude, codex and occasionally gemini (when I hit quota). I've found GPT-5.4+ is stronger than Opus 4.7 but I still mostly use claude (sonnet 4.6)