diff --git a/client/src/components/Chat/Messages/MessageNav.tsx b/client/src/components/Chat/Messages/MessageNav.tsx index 6bb9bd331e..f18870070e 100644 --- a/client/src/components/Chat/Messages/MessageNav.tsx +++ b/client/src/components/Chat/Messages/MessageNav.tsx @@ -44,13 +44,18 @@ export function extractPreviewFromContent(content?: TMessageContentParts[]): str return ''; } -export function buildEntry(id: string, msg: TMessage): MessageEntry { +export function buildEntry(id: string, msg: TMessage, node?: HTMLElement): MessageEntry { const raw = msg.text?.trim() ? msg.text : extractPreviewFromContent(msg.content); const trimmed = raw.trim(); + /** Image-, tool-call- and reasoning-only messages carry no text part at all, + * so the message alone yields nothing to say. Their rendered row does say + * something, and reading it is what keeps a settled message from being + * mistaken for one that is still generating. */ + const preview = trimmed !== '' ? trimmed : (node?.textContent ?? '').trim(); return { id, isUser: !!msg.isCreatedByUser, - preview: trimmed.slice(0, 80) + (trimmed.length > 80 ? '...' : ''), + preview: preview.slice(0, 80) + (preview.length > 80 ? '...' : ''), }; } @@ -107,19 +112,27 @@ type LocalizeFn = ReturnType; * What a rib says it will take you to. * * A response enters the rail the instant its row mounts, which is one frame - * before its first token — and a run that is still thinking has no text at all. - * Falling through to the raw preview there labelled the rib with nothing and - * opened an empty preview card beside it; naming the pending state instead - * keeps the rail readable for the whole of a long generation. + * before its first token, so falling through to the raw preview there labelled + * the rib with nothing and opened an empty preview card beside it. Only the + * tail of a live submission earns the pending wording, though: an empty preview + * is not evidence of generation, and a reopened thread whose rows are settled + * would otherwise announce "Generating" forever. */ -export function previewTextFor(entry: MessageEntry, localize: LocalizeFn): string { +export function previewTextFor( + entry: MessageEntry, + localize: LocalizeFn, + isPending = false, +): string { if (entry.isStart === true) { return localize('com_ui_scroll_to_top'); } if (entry.isEnd === true) { return localize('com_ui_scroll_to_bottom'); } - return entry.preview !== '' ? entry.preview : localize('com_ui_generating'); + if (entry.preview !== '') { + return entry.preview; + } + return localize(isPending ? 'com_ui_generating' : 'com_ui_message_nav_no_preview'); } function getMessageEntries(root: ParentNode, messagesById: Map): MessageEntry[] { @@ -138,7 +151,7 @@ function getMessageEntries(root: ParentNode, messagesById: Map continue; } const msg = messagesById.get(id); - entries.push(msg ? buildEntry(id, msg) : buildFallbackEntry(node, id)); + entries.push(msg ? buildEntry(id, msg, node) : buildFallbackEntry(node, id)); } if (entries.length > 0 && root.querySelector('#' + MESSAGES_END_ID)) { entries.push({ id: MESSAGES_END_ID, isUser: false, preview: '', isEnd: true }); @@ -163,6 +176,20 @@ function readScrollMargin(el: HTMLElement | null): number { return Number.isFinite(value) ? value : 0; } +/** + * Where the container lands when an entry is snapped to the top, clamped to the + * range it can actually reach. + * + * The rows carry `scroll-margin-top: 4rem` against `pt-14` of content padding, + * so the first entry's raw snap point is -8px. Compared unclamped it reads as + * "there is still something above you" at the very top of every conversation, + * which left the up chevron live with nowhere to go and the origin rib showing + * as out of view while the reader sat at the top. + */ +function snapPointFor(top: number, scrollMargin: number, maxScrollTop: number): number { + return Math.max(0, Math.min(top - scrollMargin, maxScrollTop)); +} + function computeTargetScroll( container: HTMLElement, el: HTMLElement, @@ -375,6 +402,12 @@ function MessageNav({ scrollableRef }: { scrollableRef: React.RefObject(null); const [hoveredId, setHoveredId] = useState(null); + /** The rib the keyboard last landed on. A roving tab stop has to travel with + * focus, not sit on the scroll-spy's current rib: leave it behind and the + * column holds two tab stops, so Tab re-enters the rail it just left and + * Shift+Tab walks back into it instead of out. Distinct from `hoveredId`, + * which the pointer also drives — the tab order must not follow the mouse. */ + const [focusedRibId, setFocusedRibId] = useState(null); /** The terminus rib is pinned beside the down chevron rather than living in * the scrolling column, so it stays reachable however far the rail scrolls. @@ -998,9 +1031,11 @@ function MessageNav({ scrollableRef }: { scrollableRef: React.RefObject) => { const col = columnRef.current; const target = e.target as HTMLElement; - if (!col || !target.getAttribute?.('data-msg-id')) { + const id = target.getAttribute?.('data-msg-id'); + if (!col || id == null) { return; } + setFocusedRibId(id); interactingRef.current = true; pointerYRef.current = target.offsetTop + target.offsetHeight / 2; if (magRafRef.current == null) { @@ -1017,6 +1052,7 @@ function MessageNav({ scrollableRef }: { scrollableRef: React.RefObject scrollTop + JUMP_EPS) { nextCanDown = true; break; @@ -1391,6 +1427,7 @@ function MessageNav({ scrollableRef }: { scrollableRef: React.RefObject scrollTop + JUMP_EPS) { + if ( + snapPointFor(entryTop(el, container), scrollMargin, maxScrollTop) > + scrollTop + JUMP_EPS + ) { scrollToStart(entries[i].id); return; } @@ -1483,22 +1527,31 @@ function MessageNav({ scrollableRef }: { scrollableRef: React.RefObject { const label = localize( entry.isUser ? 'com_ui_message_nav_go_to_user' : 'com_ui_message_nav_go_to_assistant', - { 0: previewTextFor(entry, localize).slice(0, 30) }, + { 0: previewTextFor(entry, localize, entry.id === pendingId).slice(0, 30) }, ); return ( { expect(current[0]).toHaveAttribute('data-msg-id', 'messages-end'); }); + it('disables the up chevron at the top even when the rows out-margin the padding', () => { + // `.message-render` carries `scroll-margin-top: 4rem` against `pt-14` of + // content padding, so the first entry's raw snap point is -8px. Compared + // unclamped it reads as "there is still something above you" at the very + // top of every conversation. + const messages = [ + buildMessage({ messageId: 'a', text: 'alpha', isCreatedByUser: true }), + buildMessage({ messageId: 'b', text: 'bravo' }), + buildMessage({ messageId: 'c', text: 'charlie', isCreatedByUser: true }), + ]; + mockUseGetMessagesByConvoId.mockReturnValue({ data: messages }); + const { scrollable, content } = buildDom(messages); + /** Mirror the real layout: content padding 56px, row scroll margin 64px. */ + for (let i = 0; i < content.children.length; i++) { + Object.defineProperty(content.children[i], 'offsetTop', { + value: 56 + i * 200, + configurable: true, + }); + } + const marginSpy = jest + .spyOn(window, 'getComputedStyle') + .mockImplementation(() => ({ scrollMarginTop: '64px' }) as CSSStyleDeclaration); + + const scrollableRef = { current: scrollable } as RefObject; + const { container } = render(); + act(() => { + jest.advanceTimersByTime(250); + fireEvent.scroll(scrollable); + jest.advanceTimersByTime(32); + }); + + const prev = container.querySelector( + 'button[aria-label="com_ui_message_nav_previous"]', + ) as HTMLButtonElement; + expect(prev.disabled).toBe(true); + /** And the origin rib reads as reached, not as somewhere still to go. */ + const origin = container.querySelector('[data-msg-id="messages-start"] span'); + expect(origin?.className).not.toContain('bg-text-tertiary'); + marginSpy.mockRestore(); + }); + it('chevron buttons expose a disabled state when there is nothing to navigate to', () => { const messages = [ buildMessage({ messageId: 'a', text: 'one', isCreatedByUser: true }), @@ -2116,6 +2157,7 @@ describe('MessageNav', () => { } it('names the pending state instead of labelling the rib with nothing', () => { + mockUseMessagesSubmission.mockReturnValue({ isSubmitting: true }); const { container } = renderNav(streamingMessages()); const pending = container.querySelector('[data-msg-id="d"]') as HTMLElement; expect(pending.getAttribute('aria-label')).toBe( @@ -2124,6 +2166,7 @@ describe('MessageNav', () => { }); it('never opens an empty preview beside a rib with no text yet', () => { + mockUseMessagesSubmission.mockReturnValue({ isSubmitting: true }); const messages = streamingMessages(); const restoreLayout = stubRibLayout(messages.map((m) => m.messageId)); const { container } = renderNav(messages); @@ -2140,6 +2183,49 @@ describe('MessageNav', () => { expect(tip).toHaveTextContent('com_ui_generating'); restoreLayout(); }); + + it('does not call a settled message generating just because it has no text', () => { + // Image-, tool-call- and reasoning-only messages carry no text part, so + // reopening a finished thread would otherwise announce "Generating" + // against them forever. + mockUseMessagesSubmission.mockReturnValue({ isSubmitting: false }); + const { container } = renderNav(streamingMessages()); + const settled = container.querySelector('[data-msg-id="d"]') as HTMLElement; + expect(settled.getAttribute('aria-label')).toBe( + 'com_ui_message_nav_go_to_assistant|{"0":"com_ui_message_nav_no_preview"}', + ); + }); + + it('only the tail of a live submission is pending, never an earlier gap', () => { + mockUseMessagesSubmission.mockReturnValue({ isSubmitting: true }); + const { container } = renderNav([ + buildMessage({ messageId: 'a', text: 'alpha', isCreatedByUser: true }), + /** A tool-call-only response part way up the thread. */ + buildMessage({ messageId: 'b', text: '', content: [] }), + buildMessage({ messageId: 'c', text: 'charlie', isCreatedByUser: true }), + buildMessage({ messageId: 'd', text: '', content: [] }), + ]); + expect(container.querySelector('[data-msg-id="b"]')?.getAttribute('aria-label')).toContain( + 'com_ui_message_nav_no_preview', + ); + expect(container.querySelector('[data-msg-id="d"]')?.getAttribute('aria-label')).toContain( + 'com_ui_generating', + ); + }); + + it('reads the rendered row when the message itself carries no text part', () => { + // A tool card still says something on screen; that is a better preview + // than any placeholder. + const node = document.createElement('div'); + node.className = 'message-render'; + node.textContent = 'Searched the web'; + const entry = buildEntry( + 'a', + asTMessage(buildMessage({ messageId: 'a', text: '', content: [] })), + node, + ); + expect(entry.preview).toBe('Searched the web'); + }); }); describe('rail navigation', () => { @@ -2186,6 +2272,49 @@ describe('MessageNav', () => { expect(document.activeElement).toBe(ribs[0]); }); + it('moves the tab stop with arrow-key focus so the column keeps one stop', () => { + // A roving tab stop left behind on the scroll-spy's current rib gives the + // column two stops: Tab re-enters the rail it just left, and Shift+Tab + // walks back into it instead of out. + const { container } = renderRail(); + const column = getColumn(container); + const ribs = messageRibs(container); + expect(ribs[0].getAttribute('tabindex')).toBe('0'); + + ribs[0].focus(); + act(() => { + fireEvent.keyDown(column, { key: 'ArrowDown' }); + fireEvent.keyDown(column, { key: 'ArrowDown' }); + }); + + expect(document.activeElement).toBe(ribs[2]); + const stops = messageRibs(container).filter((rib) => rib.getAttribute('tabindex') === '0'); + expect(stops).toHaveLength(1); + expect(stops[0]).toBe(ribs[2]); + }); + + it('returns the tab stop to the current rib once focus leaves the rail', () => { + const { container } = renderRail(); + const column = getColumn(container); + const ribs = messageRibs(container); + + ribs[0].focus(); + act(() => { + fireEvent.keyDown(column, { key: 'End' }); + }); + expect(messageRibs(container).filter((r) => r.getAttribute('tabindex') === '0')[0]).toBe( + ribs[ribs.length - 1], + ); + + act(() => { + fireEvent.blur(column, { relatedTarget: document.body }); + }); + + const stops = messageRibs(container).filter((rib) => rib.getAttribute('tabindex') === '0'); + expect(stops).toHaveLength(1); + expect(stops[0].getAttribute('aria-current')).toBe('true'); + }); + it('does not run off either end of the rail', () => { const { container } = renderRail(); const column = getColumn(container); diff --git a/client/src/locales/en/translation.json b/client/src/locales/en/translation.json index 2159758cce..2d7ede4e2f 100644 --- a/client/src/locales/en/translation.json +++ b/client/src/locales/en/translation.json @@ -1590,6 +1590,7 @@ "com_ui_message_nav_go_to_assistant": "Go to assistant message: {{0}}", "com_ui_message_nav_go_to_user": "Go to user message: {{0}}", "com_ui_message_nav_next": "Navigate to next message", + "com_ui_message_nav_no_preview": "No preview", "com_ui_message_nav_previous": "Navigate to previous message", "com_ui_message_part_empty": "Message content cannot be empty.", "com_ui_method": "Method",