From 54f3c004144a2547edeb3e7d5a7ed315be6d23b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=A8=8B=E5=BA=8F=E5=91=98=E9=98=BF=E6=B1=9F=28Relakkes?= =?UTF-8?q?=29?= Date: Wed, 3 Jun 2026 11:38:31 +0800 Subject: [PATCH] fix(desktop): stabilize chat selection references (#351) The chat reference action was still tied too closely to message-local mouseup timing, and the floating fixed-position control stayed inside virtualized message DOM. In Electron Chromium that made multi-line ranges race selection settlement and let transformed ancestors offset the button away from the viewport position we calculated. This reads settled document selections after pointerup/selectionchange, portals the action to document.body, and prefers right-side placement for multi-line selections. Constraint: Electron Chromium selectionchange can settle after message-local mouse events Rejected: Keep the popover inside the message node | transformed/virtualized ancestors offset fixed positioning in real browser layout Confidence: high Scope-risk: narrow Directive: Keep chat selection actions portaled; do not reparent under virtualized message rows without browser-coordinate verification Tested: cd desktop && bun run test -- src/components/chat/MessageList.test.tsx Tested: bun run check:desktop Tested: Playwright Chromium smoke for multi-line selection, single-line selection, and outside-click dismissal on local desktop harness --- .../src/components/chat/MessageList.test.tsx | 211 +++++++++++++++++- desktop/src/components/chat/MessageList.tsx | 58 +++-- .../src/hooks/useSelectionPopoverDismiss.ts | 63 ++++-- 3 files changed, 295 insertions(+), 37 deletions(-) diff --git a/desktop/src/components/chat/MessageList.test.tsx b/desktop/src/components/chat/MessageList.test.tsx index e1bcfaf6..e0edd062 100644 --- a/desktop/src/components/chat/MessageList.test.tsx +++ b/desktop/src/components/chat/MessageList.test.tsx @@ -63,6 +63,9 @@ async function waitForSelectionMenuUpdate() { await new Promise((resolve) => { requestAnimationFrame(() => resolve()) }) + await new Promise((resolve) => { + requestAnimationFrame(() => resolve()) + }) }) } @@ -119,10 +122,93 @@ async function selectMessageText( prepareMessageTextSelection(element, text, rect) await act(async () => { + fireEvent.pointerDown(element, { + button: 0, + clientX: rect.left ?? 160, + clientY: rect.top ?? 80, + pointerId: 1, + pointerType: 'mouse', + }) + fireEvent.pointerUp(element, { + button: 0, + clientX: rect.right ?? 280, + clientY: rect.bottom ?? 98, + pointerId: 1, + pointerType: 'mouse', + }) fireEvent.mouseUp(element, { clientX: 260, clientY: 104 }) await Promise.resolve() }) await waitForSelectionMenuUpdate() + await waitFor(() => { + expect(screen.getByRole('button', { name: 'Add to chat' })).toBeTruthy() + }) +} + +async function selectAcrossMessageText( + startElement: Element, + startText: string, + endElement: Element, + endText: string, + rect: Partial = {}, +) { + const startNode = findTextNodeContaining(startElement, startText) + const endNode = findTextNodeContaining(endElement, endText) + const startOffset = startNode.textContent?.indexOf(startText) ?? -1 + const endOffset = (endNode.textContent?.indexOf(endText) ?? -1) + endText.length + const range = document.createRange() + range.setStart(startNode, startOffset) + range.setEnd(endNode, endOffset) + Object.assign(range, { + getBoundingClientRect: () => ({ + left: rect.left ?? 160, + top: rect.top ?? 80, + right: rect.right ?? 520, + bottom: rect.bottom ?? 150, + width: rect.width ?? 360, + height: rect.height ?? 70, + x: rect.x ?? rect.left ?? 160, + y: rect.y ?? rect.top ?? 80, + toJSON: () => ({}), + }), + }) + + const selectableRoot = startElement.closest('[data-message-shell]')?.parentElement?.parentElement + Object.assign(selectableRoot ?? startElement, { + getBoundingClientRect: () => ({ + left: 120, + top: 48, + right: 720, + bottom: 320, + width: 600, + height: 272, + x: 120, + y: 48, + toJSON: () => ({}), + }), + }) + + window.getSelection()?.removeAllRanges() + window.getSelection()?.addRange(range) + + await act(async () => { + fireEvent.pointerDown(startElement, { + button: 0, + clientX: rect.left ?? 160, + clientY: rect.top ?? 80, + pointerId: 1, + pointerType: 'mouse', + }) + fireEvent.pointerUp(endElement, { + button: 0, + clientX: rect.right ?? 520, + clientY: rect.bottom ?? 150, + pointerId: 1, + pointerType: 'mouse', + }) + await Promise.resolve() + }) + await waitForSelectionMenuUpdate() } describe('MessageList nested tool calls', () => { @@ -1757,6 +1843,125 @@ describe('MessageList nested tool calls', () => { expect(floatingAddButton.style.top).toBe('12px') }) + it('adds multi-line assistant reply selections across markdown blocks to the composer context', async () => { + useChatStore.setState({ + sessions: { + [ACTIVE_TAB]: makeSessionState({ + messages: [{ + id: 'assistant-1', + type: 'assistant_text', + content: [ + 'First line can start the selection.', + '', + 'Second paragraph should still belong to the same chat message.', + '', + '- Third block can finish the selection.', + ].join('\n'), + timestamp: 1, + }], + }), + }, + }) + + render() + + const firstParagraph = screen.getByText('First line can start the selection.') + const listItem = screen.getByText('Third block can finish the selection.') + await selectAcrossMessageText( + firstParagraph, + 'First line', + listItem, + 'finish the selection', + { left: 160, top: 80, right: 520, bottom: 160, width: 360, height: 80 }, + ) + const floatingAddButton = screen.getByRole('button', { name: 'Add to chat' }) + + expect(floatingAddButton.style.left).toBe('530px') + expect(floatingAddButton.style.top).toBe('98px') + + fireEvent.click(floatingAddButton) + + expect(useWorkspaceChatContextStore.getState().referencesBySession[ACTIVE_TAB]).toMatchObject([ + { + kind: 'chat-selection', + messageId: 'assistant-1', + sourceRole: 'assistant', + }, + ]) + expect(useWorkspaceChatContextStore.getState().referencesBySession[ACTIVE_TAB]?.[0]?.quote).toContain('First line') + expect(useWorkspaceChatContextStore.getState().referencesBySession[ACTIVE_TAB]?.[0]?.quote).toContain('finish the selection') + }) + + it('shows the selected-message action after browser selectionchange for multi-line replies', async () => { + useChatStore.setState({ + sessions: { + [ACTIVE_TAB]: makeSessionState({ + messages: [{ + id: 'assistant-1', + type: 'assistant_text', + content: [ + 'Browser selection can settle after pointerup.', + '', + 'The document selectionchange event should be enough to show the action.', + ].join('\n'), + timestamp: 1, + }], + }), + }, + }) + + render() + + const firstParagraph = screen.getByText('Browser selection can settle after pointerup.') + const secondParagraph = screen.getByText('The document selectionchange event should be enough to show the action.') + const startNode = findTextNodeContaining(firstParagraph, 'Browser selection') + const endNode = findTextNodeContaining(secondParagraph, 'show the action') + const range = document.createRange() + range.setStart(startNode, startNode.textContent?.indexOf('Browser selection') ?? 0) + range.setEnd( + endNode, + (endNode.textContent?.indexOf('show the action') ?? 0) + 'show the action'.length, + ) + Object.assign(range, { + getBoundingClientRect: () => ({ + left: 150, + top: 76, + right: 500, + bottom: 140, + width: 350, + height: 64, + x: 150, + y: 76, + toJSON: () => ({}), + }), + }) + + const selectableRoot = firstParagraph.closest('[data-chat-selectable-message]') + Object.assign(selectableRoot ?? firstParagraph, { + getBoundingClientRect: () => ({ + left: 120, + top: 48, + right: 720, + bottom: 280, + width: 600, + height: 232, + x: 120, + y: 48, + toJSON: () => ({}), + }), + }) + + window.getSelection()?.removeAllRanges() + window.getSelection()?.addRange(range) + + await act(async () => { + document.dispatchEvent(new Event('selectionchange')) + }) + await waitForSelectionMenuUpdate() + + expect(screen.getByRole('button', { name: 'Add to chat' })).toBeTruthy() + }) + it('adds selected assistant reply text to the composer context', async () => { useChatStore.setState({ sessions: { @@ -1775,7 +1980,11 @@ describe('MessageList nested tool calls', () => { const assistantText = screen.getByText(/First inspect the file tree/) await selectMessageText(assistantText, 'quote the selected lines') - fireEvent.click(screen.getByRole('button', { name: 'Add to chat' })) + const floatingAddButton = screen.getByRole('button', { name: 'Add to chat' }) + + expect(floatingAddButton.closest('[data-chat-selectable-message]')).toBeNull() + + fireEvent.click(floatingAddButton) expect(useWorkspaceChatContextStore.getState().referencesBySession[ACTIVE_TAB]).toMatchObject([ { diff --git a/desktop/src/components/chat/MessageList.tsx b/desktop/src/components/chat/MessageList.tsx index a6395fed..72273f5f 100644 --- a/desktop/src/components/chat/MessageList.tsx +++ b/desktop/src/components/chat/MessageList.tsx @@ -1,4 +1,5 @@ import { useRef, useEffect, useMemo, memo, useState, useCallback, useDeferredValue, useLayoutEffect, type ReactNode } from 'react' +import { createPortal } from 'react-dom' import { ArrowDown, BookMarked, Bot, CheckCircle2, ChevronDown, ChevronRight, CircleStop, FileStack, LoaderCircle, MessageCircle, Settings, Target, XCircle } from 'lucide-react' import { ApiError } from '../../api/client' import { sessionsApi, type SessionTurnCheckpoint } from '../../api/sessions' @@ -141,7 +142,7 @@ function ChatSelectionMenu({ const t = useTranslation() if (!selection) return null - return ( + return createPortal( + , + document.body, ) } @@ -400,7 +402,7 @@ function SelectableChatMessage({ }) { const rootRef = useRef(null) const selectionMenuRef = useRef(null) - const selectionStartedInsideRef = useRef(false) + const lastSelectionPointerRef = useRef(null) const selectionUpdateFrameRef = useRef(null) const addReference = useWorkspaceChatContextStore((state) => state.addReference) const [selectionMenu, setSelectionMenu] = useState(null) @@ -411,21 +413,31 @@ function SelectableChatMessage({ useEffect(() => { setSelectionMenu(null) - selectionStartedInsideRef.current = false + lastSelectionPointerRef.current = null }, [content, messageId]) const dismissSelectionMenu = useCallback(() => { setSelectionMenu(null) }, []) - const queueSelectionMenuUpdate = useCallback((pointer: SelectionPointer) => { + const queueSelectionMenuUpdate = useCallback((pointer?: SelectionPointer) => { + if (pointer) lastSelectionPointerRef.current = pointer + if (selectionUpdateFrameRef.current !== null) { window.cancelAnimationFrame(selectionUpdateFrameRef.current) } selectionUpdateFrameRef.current = window.requestAnimationFrame(() => { - selectionUpdateFrameRef.current = null - setSelectionMenu(getChatSelectionFromContainer(rootRef.current, pointer)) + selectionUpdateFrameRef.current = window.requestAnimationFrame(() => { + selectionUpdateFrameRef.current = null + const root = rootRef.current + const rootRect = root?.getBoundingClientRect() + const fallbackPointer = lastSelectionPointerRef.current ?? { + clientX: (rootRect?.left ?? 0) + 24, + clientY: (rootRect?.top ?? 0) + 24, + } + setSelectionMenu(getChatSelectionFromContainer(root, fallbackPointer)) + }) }) }, []) @@ -438,21 +450,37 @@ function SelectableChatMessage({ }, []) useEffect(() => { + const handlePointerDown = (event: PointerEvent) => { + lastSelectionPointerRef.current = getSelectionPointer(event) + } + const handlePointerUp = (event: PointerEvent) => { - if (!selectionStartedInsideRef.current) return - selectionStartedInsideRef.current = false queueSelectionMenuUpdate(getSelectionPointer(event)) } - const handlePointerCancel = () => { - selectionStartedInsideRef.current = false + const handleMouseUp = (event: MouseEvent) => { + queueSelectionMenuUpdate(getSelectionPointer(event)) } + const handleSelectionChange = () => { + queueSelectionMenuUpdate() + } + + const handleKeyUp = () => { + queueSelectionMenuUpdate() + } + + document.addEventListener('pointerdown', handlePointerDown, true) document.addEventListener('pointerup', handlePointerUp, true) - document.addEventListener('pointercancel', handlePointerCancel, true) + document.addEventListener('mouseup', handleMouseUp, true) + document.addEventListener('selectionchange', handleSelectionChange) + document.addEventListener('keyup', handleKeyUp, true) return () => { + document.removeEventListener('pointerdown', handlePointerDown, true) document.removeEventListener('pointerup', handlePointerUp, true) - document.removeEventListener('pointercancel', handlePointerCancel, true) + document.removeEventListener('mouseup', handleMouseUp, true) + document.removeEventListener('selectionchange', handleSelectionChange) + document.removeEventListener('keyup', handleKeyUp, true) } }, [queueSelectionMenuUpdate]) @@ -479,12 +507,12 @@ function SelectableChatMessage({ return (
{ if (event.pointerType === 'mouse' && event.button !== 0) return - selectionStartedInsideRef.current = true + lastSelectionPointerRef.current = getSelectionPointer(event) }} onMouseUp={(event) => { - selectionStartedInsideRef.current = false queueSelectionMenuUpdate(getSelectionPointer(event)) }} onKeyDown={(event) => { diff --git a/desktop/src/hooks/useSelectionPopoverDismiss.ts b/desktop/src/hooks/useSelectionPopoverDismiss.ts index 32442e56..4129cf36 100644 --- a/desktop/src/hooks/useSelectionPopoverDismiss.ts +++ b/desktop/src/hooks/useSelectionPopoverDismiss.ts @@ -15,6 +15,11 @@ type SelectionRect = { height: number } +type SelectionGeometry = { + rect: SelectionRect | DOMRect + isMultiLine: boolean +} + function clampValue(value: number, min: number, max: number) { return Math.max(min, Math.min(value, max)) } @@ -23,29 +28,42 @@ function isUsableRect(rect: SelectionRect | DOMRect) { return rect.width > 0 || rect.height > 0 } -function getRangeSelectionRect(range: Range): SelectionRect | null { - const boundingRect = typeof range.getBoundingClientRect === 'function' - ? range.getBoundingClientRect() - : null - if (boundingRect && isUsableRect(boundingRect)) return boundingRect - +function getRangeSelectionGeometry(range: Range): SelectionGeometry | null { const clientRects = typeof range.getClientRects === 'function' ? Array.from(range.getClientRects()).filter(isUsableRect) : [] - if (clientRects.length === 0) return null + const boundingRect = typeof range.getBoundingClientRect === 'function' + ? range.getBoundingClientRect() + : null - const left = Math.min(...clientRects.map((rect) => rect.left)) - const top = Math.min(...clientRects.map((rect) => rect.top)) - const right = Math.max(...clientRects.map((rect) => rect.right)) - const bottom = Math.max(...clientRects.map((rect) => rect.bottom)) - return { - left, - top, - right, - bottom, - width: right - left, - height: bottom - top, + if (clientRects.length > 0) { + const left = Math.min(...clientRects.map((rect) => rect.left)) + const top = Math.min(...clientRects.map((rect) => rect.top)) + const right = Math.max(...clientRects.map((rect) => rect.right)) + const bottom = Math.max(...clientRects.map((rect) => rect.bottom)) + const unionRect = { + left, + top, + right, + bottom, + width: right - left, + height: bottom - top, + } + const maxLineHeight = Math.max(...clientRects.map((rect) => rect.height)) + return { + rect: boundingRect && isUsableRect(boundingRect) ? boundingRect : unionRect, + isMultiLine: clientRects.length > 1 || unionRect.height > maxLineHeight * 1.6, + } } + + if (boundingRect && isUsableRect(boundingRect)) { + return { + rect: boundingRect, + isMultiLine: boundingRect.height > 32, + } + } + + return null } export function clearWindowSelection() { @@ -67,7 +85,7 @@ export function getSelectionPopoverPosition( fallbackPointer?: { clientX: number; clientY: number } }, ) { - const rect = getRangeSelectionRect(range) + const geometry = getRangeSelectionGeometry(range) const rootRect = root.getBoundingClientRect() const pointerInsideRoot = fallbackPointer && fallbackPointer.clientX >= rootRect.left @@ -76,7 +94,7 @@ export function getSelectionPopoverPosition( && fallbackPointer.clientY <= rootRect.bottom const fallbackX = pointerInsideRoot ? fallbackPointer.clientX : rootRect.left + 24 const fallbackY = pointerInsideRoot ? fallbackPointer.clientY : rootRect.top + 24 - const selectionRect = rect ?? { + const selectionRect = geometry?.rect ?? { left: fallbackX, top: fallbackY, right: fallbackX, @@ -100,12 +118,15 @@ export function getSelectionPopoverPosition( x: centerX - menuWidth / 2, y: selectionRect.top - menuHeight - offset, } - if (above.y >= VIEWPORT_MARGIN) return clampPosition(above) const right = { x: selectionRect.right + offset, y: centerY - menuHeight / 2, } + if (geometry?.isMultiLine && right.x + menuWidth <= viewportWidth - VIEWPORT_MARGIN) return clampPosition(right) + + if (above.y >= VIEWPORT_MARGIN) return clampPosition(above) + if (right.x + menuWidth <= viewportWidth - VIEWPORT_MARGIN) return clampPosition(right) const below = {