CERBERUS/LibreChat
1
0
Fork 0
mirror of https://github.com/danny-avila/LibreChat.git synced 2026-10-23 18:43:21 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 9

🧭 fix: Address Codex Round 1 on the Message Rail

Browse source 
All three findings were real.

An empty preview is not evidence of generation. `buildEntry` returns nothing
for image-, tool-call- and reasoning-only content too, so a reopened thread
labelled those settled rows "Generating" forever. Only the tail of a live
submission earns that wording now; every other text-free row first reads the
text its rendered card already puts on screen, and falls back to an explicit
"No preview" rather than a blank label and an empty preview panel.

A roving tab stop has to travel with focus. Deriving `tabIndex=0` from the
scroll-spy's current rib alone left a second stop behind the moment an arrow
key moved focus, so Tab re-entered the rail it had just left and Shift+Tab
walked backwards into it instead of out.

And a snap point only exists inside the range the container can 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 read as
"there is still something above you" at the top of every conversation. That
kept the up chevron live with nowhere to go — a defect that predates this
branch — and the new origin rib inherited it, rendering as out of view while
the reader sat at the absolute top. `snapPointFor` now clamps, and the jump
helpers share it so they answer that question the same way the chevrons do.

Re-verified against the real layout: the harness now reproduces the 56px
padding and 64px scroll margin exactly, where the previous run had masked
the negative snap.
This commit is contained in:
Danny Avila 2026-08-26 17:56:26 -04:00
parent d10ca65026
commit 51180fae7b
3 changed files with 202 additions and 19 deletions
Download patch file Download diff file Expand all files Collapse all files

91
client/src/components/Chat/Messages/MessageNav.tsx
View file

@ -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<typeof useLocalize>;
* 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<string, TMessage>): MessageEntry[] {
@ -138,7 +151,7 @@ function getMessageEntries(root: ParentNode, messagesById: Map<string, TMessage>
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<HTMLDivE
const [tip, setTip] = useState<{ id: string; top: number; right: number } | null>(null);
const [hoveredId, setHoveredId] = useState<string | null>(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<string | null>(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<HTMLDivE
(e: React.FocusEvent<HTMLDivElement>) => {
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<HTMLDivE
if (col && next && col.contains(next)) {
return;
}
setFocusedRibId(null);
handlePointerLeave();
},
[handlePointerLeave],
@ -1178,7 +1214,7 @@ function MessageNav({ scrollableRef }: { scrollableRef: React.RefObject<HTMLDivE
if (offsetsTop[i] === Number.POSITIVE_INFINITY) {
continue;
}
const snap = Math.min(offsetsTop[i] - scrollMargin, containerMaxScrollTop);
const snap = snapPointFor(offsetsTop[i], scrollMargin, containerMaxScrollTop);
if (snap > scrollTop + JUMP_EPS) {
nextCanDown = true;
break;
@ -1391,6 +1427,7 @@ function MessageNav({ scrollableRef }: { scrollableRef: React.RefObject<HTMLDivE
return;
}
const scrollTop = container.scrollTop;
const maxScrollTop = Math.max(0, container.scrollHeight - container.clientHeight);
const scrollMargin =
scrollMarginRef.current !== 0
? scrollMarginRef.current
@ -1400,7 +1437,10 @@ function MessageNav({ scrollableRef }: { scrollableRef: React.RefObject<HTMLDivE
if (!el) {
continue;
}
if (entryTop(el, container) - scrollMargin < scrollTop - JUMP_EPS) {
if (
snapPointFor(entryTop(el, container), scrollMargin, maxScrollTop) <
scrollTop - JUMP_EPS
) {
scrollToStart(entries[i].id);
return;
}
@ -1414,6 +1454,7 @@ function MessageNav({ scrollableRef }: { scrollableRef: React.RefObject<HTMLDivE
return;
}
const scrollTop = container.scrollTop;
const maxScrollTop = Math.max(0, container.scrollHeight - container.clientHeight);
const scrollMargin =
scrollMarginRef.current !== 0
? scrollMarginRef.current
@ -1423,7 +1464,10 @@ function MessageNav({ scrollableRef }: { scrollableRef: React.RefObject<HTMLDivE
if (!el) {
continue;
}
if (entryTop(el, container) - scrollMargin > 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<HTMLDivE
return null;
}
/** Only the tail of a live submission is actually generating. Every other
* entry with no preview is a settled message whose content simply has no
* text part. */
const pendingId = isSubmitting ? messageEntries[messageEntries.length - 1].id : null;
const tipEntry = tip ? entryById.get(tip.id) : undefined;
let tipText = '';
if (tipEntry) {
tipText = previewTextFor(tipEntry, localize);
tipText = previewTextFor(tipEntry, localize, tipEntry.id === pendingId);
}
/** One tab stop for the whole column; the arrows move within it. The current
* rib carries it so Tab lands where the reader already is — and at the very
* bottom, where current is the pinned terminus, on the last message rather
* than back at the top of the thread. */
/** One tab stop for the whole column; the arrows move within it. It rides the
* focused rib while the keyboard is in the rail, and otherwise the current
* one, so Tab lands where the reader already is — and at the very bottom,
* where current is the pinned terminus, on the last message rather than back
* at the top of the thread. */
let rovingId = messageEntries[0].id;
if (currentId === MESSAGES_END_ID) {
rovingId = messageEntries[messageEntries.length - 1].id;
} else if (currentId != null && entryById.has(currentId)) {
rovingId = currentId;
}
if (focusedRibId != null && !isTerminusId(focusedRibId) && entryById.has(focusedRibId)) {
rovingId = focusedRibId;
}
return (
<nav
@ -1557,7 +1610,7 @@ function MessageNav({ scrollableRef }: { scrollableRef: React.RefObject<HTMLDivE
{messageEntries.map((entry) => {
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 (
<MessageIndicator

129
client/src/components/Chat/Messages/__tests__/MessageNav.spec.tsx
View file

@ -782,6 +782,47 @@ describe('MessageNav', () => {
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<HTMLDivElement>;
const { container } = render(<MessageNav scrollableRef={scrollableRef} />);
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);

1
client/src/locales/en/translation.json
View file

@ -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",