🔲 fix: Anchor Quote Popup to Visible Selection - #16458
Conversation
|
Review head 5c04f90: clip the quote popup anchor to the visible selection inside nested scroll containers, with a focused regression for long code lines, sideways scrolling, hidden selections, and preserved quote text. |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c04f906af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const clipper of clippers) { | ||
| const bounds = clipper.getBoundingClientRect(); |
There was a problem hiding this comment.
Account for selections that leave the starting scroller
When a selection begins inside a code block or table scroller and extends into later prose in the same .message-render, readSelection collects clipping ancestors only from measured.startContainer, but this loop applies all of those rectangles to the bounding box of the entire range. This discards the visible selected prose outside the starting scroller and pins the popup to the code/table instead of the overall visible selection. Clip each range fragment only by ancestors that contain it, or avoid applying the starting node's clippers to a cross-container range.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8b1fde7. The popup now groups selected text by clipping context, intersects each group with its own scroll ancestors, and merges only visible bounds. Added red-before-green coverage for selections crossing a code scroller and prose in both directions, including scrolling the code fully out of view while retaining the complete quoted text. QuoteButton tests: 8/8; staged ESLint, Prettier, and import-order checks: passed.
|
Review head 8b1fde7: address the cross-container selection finding by clipping each range group to its own scroller. Regression covers code-first and prose-first selections, scrolling the code out of view, and preserving the complete quote. Focused Jest 8/8; staged static checks passed. |
|
Review head 786dcb1: cross-container selections are clipped per fragment, and text-node traversal is now bounded to the selected range for long messages. Two new red-before-green regressions cover code-first and prose-first selections and preserve the full quote. Focused Jest: 8/8; staged lint, Prettier and import order: passed. |
Summary
Selecting part of a long, horizontally scrolling code line can leave “Add to chat” at the far right of the viewport instead of next to the highlighted text. The selection's DOM range includes text hidden outside the code block, so its full bounding rectangle gives the popup a misleading center.
The popup now uses only the selected text visible within the viewport and each fragment’s own clipping ancestors. This keeps later prose visible when a selection starts inside a scrolling code block. The popup follows the visible portion while scrolling and disappears when none of the selection remains visible. The queued quote still contains the selected text, not just its visible slice.
How it works
QuoteButtongroups consecutive selected text nodes by clipping context, intersects each group with its own nested scroll containers and the viewport, then positions the popup using their visible union. A single text node retains a fast path. An empty union hides the popup. Focused regressions cover a long code line, scrolling fully out of view, selections crossing code and prose in both directions, and quoting the original selection.Type of change
Testing
Tested environments/configuration: Client Jest with jsdom and nested scroll-container geometry.
Automated tests:
QuoteButton.test.tsx: 8 passed, including wide-code-line and both directions of the cross-container regression.npm run sort-imports -- client/src/components/Chat/Input/QuoteButton.tsx client/src/components/Chat/Input/__tests__/QuoteButton.test.tsx: passed.npm run static-checks: passed on the two staged files (ESLint, Prettier, import order).cd client && npx tsc --noEmit: attempted, but the shared localnode_modulesresolves stale builds of other workspaces, producing errors in unrelated frontend files. CI should validate against freshly built packages.Screenshots / recordings
A matching before/after running-app capture was not available locally because no app server is running in this workspace. The reporter's screenshot documents the before state.
Risk / compatibility
No API or stored-data changes. The only behavior change is popup placement and visibility for selections clipped by scroll containers.
Checklist