Skip to content

fix(client): keep All Comments open when a click finishes a text selection - #473

Open
nikvakhrameev wants to merge 1 commit into
yoshiko-pg:mainfrom
nikvakhrameev:fix/comments-list-text-selection
Open

nikvakhrameev wants to merge 1 commit into
yoshiko-pg:mainfrom
nikvakhrameev:fix/comments-list-text-selection

Conversation

@nikvakhrameev

Copy link
Copy Markdown

Motivation

Selecting text inside the All Comments modal closes the modal.

Reproduction:

  1. Open a diff with at least one comment and open All Comments.
  2. Press the mouse inside a comment body, drag to select some text, release.
  3. The modal navigates to that thread and closes; the selection is lost.

Browsers fire click after any mousedown/mouseup pair inside the same element, including a drag that only selects text. Each thread card in CommentsListModal handles click by calling onNavigate(thread) and onClose(), so a text selection is treated exactly like a click on the card.

Expected: selecting text inside the modal leaves it open with the selection intact, while a plain click still navigates to the thread and closes the modal.

Implementation notes

  • src/client/utils/domUtils.ts: add hasActiveTextSelection(), which reports whether document.getSelection() is non-collapsed and non-empty.
  • src/client/components/CommentsListModal.tsx: in the card onClick, return early when hasActiveTextSelection() is true (after e.stopPropagation(), so the click still does not fall through to the backdrop). No other behaviour changes; setSelectedIndex and handleThreadClick run as before for a regular click.
  • src/client/components/CommentsListModal.test.tsx: add a regression test next to navigates when clicking a thread. It builds a real Range over a comment body (happy-dom implements Selection/Range), fires click, and asserts onNavigate/onClose are not called; it then clears the selection, clicks again, and asserts the normal navigate-and-close path still works. The test fails on main and passes with this change.

Manual verification

  • pnpm check, pnpm format, pnpm knip, pnpm test (861 passed, 2 skipped), pnpm build all pass locally; lefthook pre-commit and pre-push hooks ran and passed.
  • pnpm dev, open All Comments, drag-select text inside a comment body and release: the modal stays open and the text stays selected.
  • Click a comment card without dragging: the modal navigates to the thread and closes as before.
  • Clicking the backdrop or the close button still closes the modal.

UI changes

None visible. No new elements or styling; only the click handling changes, so no screenshots or docs/images updates are needed.

🤖 Generated with Claude Code

…ction

Browsers fire `click` after any mousedown/mouseup pair inside the same
element, including a mouse drag that only selects text. In the All
Comments modal every thread card handles `click` by navigating to the
thread and closing the modal, so selecting text inside a comment
closed the modal instead of leaving the selection in place.

Add a `hasActiveTextSelection()` helper to `domUtils` and bail out of
the card's click handler while a non-empty selection exists. A plain
click still navigates and closes the modal as before.

This branch has not been deployed

No deployments
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