Skip to content

fix: backward selection across blank-line boundary#473

Merged
tomasz-tomczyk merged 1 commit intomainfrom
fix-backward-selection
May 6, 2026
Merged

fix: backward selection across blank-line boundary#473
tomasz-tomczyk merged 1 commit intomainfrom
fix-backward-selection

Conversation

@tomasz-tomczyk
Copy link
Copy Markdown
Owner

Summary

  • Rewrite getLineRangeFromSelection to walk every .line-block and [data-diff-line-num] element and union the line ranges of those the selection's Range intersects, instead of only inspecting selection.anchorNode / focusNode.
  • Fixes user-reported bug: selecting text "backwards" (later → earlier) across a blank-line/paragraph boundary attached the comment only to the first part of the selection (or failed to open the form). One Selection endpoint can snap to the parent container between line-blocks where closest('.line-block') returns null; the new approach is direction-agnostic and robust to gap-endpoint positions.
  • Adds an e2e regression test that reproduces the gap-endpoint condition programmatically (headless Playwright doesn't reproduce the browser snap behavior with a plain mouse drag).

Review

  • Code review: passed
  • Parity audit: mirrors crit-web/ change
  • `go test -race ./...`: pass
  • `golangci-lint run ./...`: 0 issues
  • `gofmt`: clean
  • e2e select-to-comment + drag-selection + comments suites: pass

Test plan

  • `cd e2e && npx playwright test tests/select-to-comment.spec.ts`
  • See also: tomasz-tomczyk/crit-web (paired fix in assets/js/document-renderer.js)

🤖 Generated with Claude Code

Replace anchorNode/focusNode inspection in getLineRangeFromSelection
with Range.intersectsNode() so selection-to-comment is direction-
agnostic. Backward drags across a blank-line/paragraph boundary could
leave one endpoint snapped to the parent container between line-blocks
(no .line-block ancestor), causing the range to collapse to a single
endpoint or the comment form to fail to open.

Walks .line-block and diff-line elements and unions the line ranges
of every element the selection range intersects. Same semantics for
single-block selections; robust for multi-block and gap-endpoint cases.

Adds e2e regression test that constructs the gap-endpoint Selection
programmatically (Playwright headless doesn't reproduce the browser
snap behavior with a simple mouse drag).
@tomasz-tomczyk tomasz-tomczyk merged commit 1b48658 into main May 6, 2026
6 checks passed
@tomasz-tomczyk tomasz-tomczyk deleted the fix-backward-selection branch May 6, 2026 08:20
@codecov
Copy link
Copy Markdown

codecov Bot commented May 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.22%. Comparing base (75fa7d5) to head (f9008b7).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #473      +/-   ##
==========================================
+ Coverage   69.17%   69.22%   +0.05%     
==========================================
  Files          36       36              
  Lines       10744    10744              
==========================================
+ Hits         7432     7438       +6     
+ Misses       2747     2743       -4     
+ Partials      565      563       -2     
Flag Coverage Δ
e2e 32.39% <ø> (ø)
unit 66.75% <ø> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant