mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-04-05 15:27:20 +02:00
🗜️ refactor: Eliminate Unstable React Keys During SSE Lifecycle (#12536)
* debug: add instrumentation to MessageIcon arePropsEqual + render cycle tests
Temporary debug commit to identify which field triggers MessageIcon
re-renders during message creation and streaming.
arePropsEqual now logs 'icon_memo_diff' with the exact field name and
prev/next values whenever it returns false. Filter browser console for
'icon_memo_diff' to see the trigger.
Also adds render-level integration tests that simulate the message
lifecycle (initial mount, streaming chunks, context updates) and
assert render counts via logger spy.
* perf: stabilize MultiMessage key to prevent unmount/remount during SSE lifecycle
messageId changes 3 times during the SSE message lifecycle:
1. useChatFunctions creates initialResponse with client-generated UUID
2. createdHandler replaces it with userMessageId + '_'
3. finalHandler replaces it with server-assigned messageId
Since MultiMessage used key={message.messageId}, each change caused
React to destroy and recreate the entire message component subtree,
unmounting MessageIcon and all memoized children. This produced visible
icon/image flickering that no memo comparator could prevent.
Switch to key={parentMessageId + '_' + siblingIdx}:
- parentMessageId is stable from creation through final response
- siblingIdx ensures sibling switches still get clean remounts
- Eliminates 2 unnecessary unmount/remount cycles per message
Add key stability tests verifying:
- Current key={messageId} causes 3 mounts / 2 unmounts per lifecycle
- Stable key causes 1 mount / 0 unmounts per lifecycle
- Sibling switches still trigger clean remounts with stable key
* perf: stabilize root MultiMessage key across new conversation lifecycle
When a user sends their first message in a new conversation,
conversationId transitions from null/'new' to the server-assigned
UUID. MessagesView used key={conversationId} on the root MultiMessage,
so this transition destroyed the entire message tree and rebuilt it
from scratch — causing all MessageIcons to unmount/remount (visible
as image flickering).
Use a ref-based stable key that captures the first real conversationId
and only changes on genuine conversation switches (navigating to a
different conversation), not on the null→UUID transition within the
same conversation.
* debug: add mount/unmount lifecycle tracking to MessageIcon
Adds icon_lifecycle logs (MOUNT/UNMOUNT) and render count to
distinguish between fresh mounts (memo comparator not called)
and internal re-renders (hook bypassing memo).
Enable: localStorage.setItem('DEBUG_LOGGING', 'icon_lifecycle,icon_data,icon_memo_diff')
* debug: add key and root tracking to MultiMessage and MessagesView
Logs multi_message_key (stableKey, messageId, parentMessageId, route)
and messages_view_key (rootKey, conversationId) to trace which key
changes trigger unmount/remount cycles.
Enable: localStorage.setItem('DEBUG_LOGGING', 'icon_lifecycle,icon_data,icon_memo_diff,multi_message_key,messages_view_key')
* perf: remove key from root MultiMessage to prevent tree destruction
The ref-based stable key still changed during 'new' → real UUID
transition, destroying the entire tree. The root MultiMessage is the
sole child at its position, so React reuses the instance via
positional reconciliation without any key. The messageId prop
(conversationId) naturally resets Recoil siblingIdxFamily state on
conversation switches.
* perf: remove unstable keys from MultiMessage to prevent SSE lifecycle remounts
Both messageId and parentMessageId change during the SSE lifecycle
(client UUID → CREATED server ID → FINAL server ID), making neither
viable as a stable React key. Each key change caused React to destroy
and recreate the entire message component subtree, including all
memoized children — visible as icon/image flickering.
Remove explicit keys entirely and rely on React's positional
reconciliation. MultiMessage always renders exactly one child at
the same position, so React reuses the component instance and
updates props in place. The existing memo comparators on
ContentRender/MessageRender handle field-level diffing correctly.
Update tests to verify: key={messageId} causes 3 mounts/2 unmounts
per lifecycle, while no key causes 1 mount/0 unmounts.
* perf: remove unstable keys from child MultiMessage in message wrappers
Message.tsx, MessageContent.tsx, and MessageParts.tsx each render a
child MultiMessage with key={messageId} for the current message's
children. Since messageId changes during the SSE lifecycle (CREATED
event replaces the user message ID), the child MultiMessage gets
destroyed and recreated, unmounting the entire agent response subtree
including its MessageIcon.
Remove these keys for the same reason as the parent MultiMessage:
each child MultiMessage renders exactly one child at a fixed position,
so positional reconciliation correctly reuses the instance.
* chore: remove MultiMessage key tests — they test React behavior, not our code
The tests verified that key={messageId} causes remounts while no key
doesn't, but this is React's own reconciliation behavior. No unit test
can prevent someone from re-adding a key prop to JSX. The JSDoc comments
on MultiMessage document the decision and rationale.
This commit is contained in:
parent
fa4a43da21
commit
b4d97bd888
7 changed files with 264 additions and 66 deletions
|
|
@ -47,7 +47,6 @@ export default function Message(props: TMessageProps) {
|
|||
</div>
|
||||
</MessageContainer>
|
||||
<MultiMessage
|
||||
key={messageId}
|
||||
messageId={messageId}
|
||||
conversation={conversation}
|
||||
messagesTree={children ?? []}
|
||||
|
|
|
|||
|
|
@ -1,4 +1,4 @@
|
|||
import { useMemo, memo } from 'react';
|
||||
import { useMemo, useEffect, useRef, memo } from 'react';
|
||||
import { getEndpointField } from 'librechat-data-provider';
|
||||
import type { Assistant, Agent } from 'librechat-data-provider';
|
||||
import type { TMessageIcon } from '~/common';
|
||||
|
|
@ -19,38 +19,54 @@ type MessageIconProps = {
|
|||
* this component renders display properties only, not identity-derived content.
|
||||
*/
|
||||
export function arePropsEqual(prev: MessageIconProps, next: MessageIconProps): boolean {
|
||||
if (prev.iconData?.endpoint !== next.iconData?.endpoint) {
|
||||
return false;
|
||||
}
|
||||
if (prev.iconData?.model !== next.iconData?.model) {
|
||||
return false;
|
||||
}
|
||||
if (prev.iconData?.iconURL !== next.iconData?.iconURL) {
|
||||
return false;
|
||||
}
|
||||
if (prev.iconData?.modelLabel !== next.iconData?.modelLabel) {
|
||||
return false;
|
||||
}
|
||||
if (prev.iconData?.isCreatedByUser !== next.iconData?.isCreatedByUser) {
|
||||
return false;
|
||||
}
|
||||
if (prev.agent?.name !== next.agent?.name) {
|
||||
return false;
|
||||
}
|
||||
if (prev.agent?.avatar?.filepath !== next.agent?.avatar?.filepath) {
|
||||
return false;
|
||||
}
|
||||
if (prev.assistant?.name !== next.assistant?.name) {
|
||||
return false;
|
||||
}
|
||||
if (prev.assistant?.metadata?.avatar !== next.assistant?.metadata?.avatar) {
|
||||
return false;
|
||||
const checks: [string, unknown, unknown][] = [
|
||||
['iconData.endpoint', prev.iconData?.endpoint, next.iconData?.endpoint],
|
||||
['iconData.model', prev.iconData?.model, next.iconData?.model],
|
||||
['iconData.iconURL', prev.iconData?.iconURL, next.iconData?.iconURL],
|
||||
['iconData.modelLabel', prev.iconData?.modelLabel, next.iconData?.modelLabel],
|
||||
['iconData.isCreatedByUser', prev.iconData?.isCreatedByUser, next.iconData?.isCreatedByUser],
|
||||
['agent.name', prev.agent?.name, next.agent?.name],
|
||||
['agent.avatar.filepath', prev.agent?.avatar?.filepath, next.agent?.avatar?.filepath],
|
||||
['assistant.name', prev.assistant?.name, next.assistant?.name],
|
||||
[
|
||||
'assistant.metadata.avatar',
|
||||
prev.assistant?.metadata?.avatar,
|
||||
next.assistant?.metadata?.avatar,
|
||||
],
|
||||
];
|
||||
|
||||
for (const [field, prevVal, nextVal] of checks) {
|
||||
if (prevVal !== nextVal) {
|
||||
logger.log('icon_memo_diff', `field "${field}" changed:`, prevVal, '→', nextVal);
|
||||
return false;
|
||||
}
|
||||
}
|
||||
return true;
|
||||
}
|
||||
|
||||
const MessageIcon = memo(({ iconData, assistant, agent }: MessageIconProps) => {
|
||||
logger.log('icon_data', iconData, assistant, agent);
|
||||
const renderCountRef = useRef(0);
|
||||
renderCountRef.current += 1;
|
||||
|
||||
useEffect(() => {
|
||||
logger.log(
|
||||
'icon_lifecycle',
|
||||
'MOUNT',
|
||||
iconData?.modelLabel,
|
||||
`render #${renderCountRef.current}`,
|
||||
);
|
||||
return () => {
|
||||
logger.log('icon_lifecycle', 'UNMOUNT', iconData?.modelLabel);
|
||||
};
|
||||
// eslint-disable-next-line react-hooks/exhaustive-deps
|
||||
}, []);
|
||||
|
||||
logger.log(
|
||||
'icon_data',
|
||||
`render #${renderCountRef.current}`,
|
||||
iconData?.isCreatedByUser ? 'user' : iconData?.modelLabel,
|
||||
iconData,
|
||||
);
|
||||
const { data: endpointsConfig } = useGetEndpointsQuery();
|
||||
|
||||
const agentName = agent?.name ?? '';
|
||||
|
|
|
|||
|
|
@ -179,7 +179,6 @@ export default function Message(props: TMessageProps) {
|
|||
</div>
|
||||
</div>
|
||||
<MultiMessage
|
||||
key={messageId}
|
||||
messageId={messageId}
|
||||
conversation={conversation}
|
||||
messagesTree={children ?? []}
|
||||
|
|
|
|||
|
|
@ -62,7 +62,6 @@ function MessagesViewContent({
|
|||
<>
|
||||
<div ref={screenshotTargetRef}>
|
||||
<MultiMessage
|
||||
key={conversationId}
|
||||
messagesTree={_messagesTree}
|
||||
messageId={conversationId ?? null}
|
||||
setCurrentEditId={setCurrentEditId}
|
||||
|
|
|
|||
|
|
@ -39,47 +39,40 @@ export default function MultiMessage({
|
|||
return null;
|
||||
}
|
||||
|
||||
const message = messagesTree[messagesTree.length - siblingIdx - 1] as TMessage | undefined;
|
||||
const currentSiblingIdx = messagesTree.length - siblingIdx - 1;
|
||||
const message = messagesTree[currentSiblingIdx] as TMessage | undefined;
|
||||
|
||||
if (!message) {
|
||||
return null;
|
||||
}
|
||||
|
||||
/**
|
||||
* No explicit key — React uses positional reconciliation since MultiMessage
|
||||
* always renders exactly one child at this position.
|
||||
*
|
||||
* Both messageId and parentMessageId change during the SSE lifecycle
|
||||
* (client UUID → createdHandler ID → server ID), so neither can serve as a
|
||||
* stable key. Using either caused React to unmount/remount the entire subtree
|
||||
* on each SSE event, destroying memoized state and causing visible flickering.
|
||||
*
|
||||
* Without a key, React reuses the component instance and updates props in place.
|
||||
* The memo comparators on ContentRender/MessageRender handle field-level diffing,
|
||||
* and sibling switches work correctly because the message prop changes entirely.
|
||||
*/
|
||||
const sharedProps = {
|
||||
message,
|
||||
currentEditId,
|
||||
setCurrentEditId,
|
||||
siblingIdx: currentSiblingIdx,
|
||||
siblingCount: messagesTree.length,
|
||||
setSiblingIdx: setSiblingIdxRev,
|
||||
};
|
||||
|
||||
if (isAssistantsEndpoint(message.endpoint) && message.content) {
|
||||
return (
|
||||
<MessageParts
|
||||
key={message.messageId}
|
||||
message={message}
|
||||
currentEditId={currentEditId}
|
||||
setCurrentEditId={setCurrentEditId}
|
||||
siblingIdx={messagesTree.length - siblingIdx - 1}
|
||||
siblingCount={messagesTree.length}
|
||||
setSiblingIdx={setSiblingIdxRev}
|
||||
/>
|
||||
);
|
||||
return <MessageParts {...sharedProps} />;
|
||||
} else if (message.content) {
|
||||
return (
|
||||
<MessageContent
|
||||
key={message.messageId}
|
||||
message={message}
|
||||
currentEditId={currentEditId}
|
||||
setCurrentEditId={setCurrentEditId}
|
||||
siblingIdx={messagesTree.length - siblingIdx - 1}
|
||||
siblingCount={messagesTree.length}
|
||||
setSiblingIdx={setSiblingIdxRev}
|
||||
/>
|
||||
);
|
||||
return <MessageContent {...sharedProps} />;
|
||||
}
|
||||
|
||||
return (
|
||||
<Message
|
||||
key={message.messageId}
|
||||
message={message}
|
||||
currentEditId={currentEditId}
|
||||
setCurrentEditId={setCurrentEditId}
|
||||
siblingIdx={messagesTree.length - siblingIdx - 1}
|
||||
siblingCount={messagesTree.length}
|
||||
setSiblingIdx={setSiblingIdxRev}
|
||||
/>
|
||||
);
|
||||
return <Message {...sharedProps} />;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -0,0 +1,193 @@
|
|||
import React from 'react';
|
||||
import { render } from '@testing-library/react';
|
||||
import { EModelEndpoint } from 'librechat-data-provider';
|
||||
import type { Agent } from 'librechat-data-provider';
|
||||
import type { TMessageIcon } from '~/common';
|
||||
|
||||
jest.mock('librechat-data-provider', () => ({
|
||||
...jest.requireActual('librechat-data-provider'),
|
||||
getEndpointField: jest.fn(() => ''),
|
||||
}));
|
||||
jest.mock('~/data-provider', () => ({
|
||||
useGetEndpointsQuery: jest.fn(() => ({ data: {} })),
|
||||
}));
|
||||
|
||||
// logger is a plain object with a real function — not a jest.fn() —
|
||||
// so restoreMocks/clearMocks won't touch it. We spy on it per-test instead.
|
||||
const logCalls: unknown[][] = [];
|
||||
jest.mock('~/utils', () => ({
|
||||
getIconEndpoint: jest.fn(() => 'agents'),
|
||||
logger: {
|
||||
log: (...args: unknown[]) => {
|
||||
logCalls.push(args);
|
||||
},
|
||||
},
|
||||
}));
|
||||
jest.mock('~/components/Endpoints/ConvoIconURL', () => {
|
||||
const ConvoIconURL = (props: Record<string, unknown>) => (
|
||||
<div data-testid="convo-icon-url" data-icon-url={props.iconURL as string} />
|
||||
);
|
||||
ConvoIconURL.displayName = 'ConvoIconURL';
|
||||
return { __esModule: true, default: ConvoIconURL };
|
||||
});
|
||||
jest.mock('~/components/Endpoints/Icon', () => {
|
||||
const Icon = (props: Record<string, unknown>) => (
|
||||
<div data-testid="icon" data-icon-url={props.iconURL as string} />
|
||||
);
|
||||
Icon.displayName = 'Icon';
|
||||
return { __esModule: true, default: Icon };
|
||||
});
|
||||
|
||||
import MessageIcon from '../MessageIcon';
|
||||
|
||||
const makeAgent = (overrides?: Partial<Agent>): Agent =>
|
||||
({
|
||||
id: 'agent_123',
|
||||
name: 'GitHub Agent',
|
||||
avatar: { filepath: '/images/agent-avatar.png' },
|
||||
...overrides,
|
||||
}) as Agent;
|
||||
|
||||
const baseIconData: TMessageIcon = {
|
||||
endpoint: EModelEndpoint.agents,
|
||||
model: 'agent_123',
|
||||
iconURL: undefined,
|
||||
modelLabel: 'GitHub Agent',
|
||||
isCreatedByUser: false,
|
||||
};
|
||||
|
||||
describe('MessageIcon render cycles', () => {
|
||||
beforeEach(() => {
|
||||
logCalls.length = 0;
|
||||
});
|
||||
|
||||
it('renders once on initial mount', () => {
|
||||
render(<MessageIcon iconData={baseIconData} agent={makeAgent()} />);
|
||||
const iconDataCalls = logCalls.filter((c) => c[0] === 'icon_data');
|
||||
expect(iconDataCalls).toHaveLength(1);
|
||||
});
|
||||
|
||||
it('does not re-render when parent re-renders with same field values but new object references', () => {
|
||||
const agent = makeAgent();
|
||||
const { rerender } = render(<MessageIcon iconData={baseIconData} agent={agent} />);
|
||||
logCalls.length = 0;
|
||||
|
||||
// Simulate parent re-render: new iconData object (same field values), new agent object (same data)
|
||||
rerender(<MessageIcon iconData={{ ...baseIconData }} agent={makeAgent()} />);
|
||||
|
||||
const iconDataCalls = logCalls.filter((c) => c[0] === 'icon_data');
|
||||
expect(iconDataCalls).toHaveLength(0);
|
||||
});
|
||||
|
||||
it('does not re-render when agent object reference changes but name and avatar are the same', () => {
|
||||
const agent1 = makeAgent();
|
||||
const { rerender } = render(<MessageIcon iconData={baseIconData} agent={agent1} />);
|
||||
logCalls.length = 0;
|
||||
|
||||
// New agent object with different id but same display fields
|
||||
const agent2 = makeAgent({ id: 'agent_456' });
|
||||
rerender(<MessageIcon iconData={baseIconData} agent={agent2} />);
|
||||
|
||||
const iconDataCalls = logCalls.filter((c) => c[0] === 'icon_data');
|
||||
expect(iconDataCalls).toHaveLength(0);
|
||||
});
|
||||
|
||||
it('re-renders when agent avatar filepath changes', () => {
|
||||
const agent1 = makeAgent();
|
||||
const { rerender } = render(<MessageIcon iconData={baseIconData} agent={agent1} />);
|
||||
logCalls.length = 0;
|
||||
|
||||
const agent2 = makeAgent({ avatar: { filepath: '/images/new-avatar.png' } });
|
||||
rerender(<MessageIcon iconData={baseIconData} agent={agent2} />);
|
||||
|
||||
const iconDataCalls = logCalls.filter((c) => c[0] === 'icon_data');
|
||||
expect(iconDataCalls).toHaveLength(1);
|
||||
});
|
||||
|
||||
it('re-renders when agent goes from undefined to defined (name changes from undefined to string)', () => {
|
||||
const { rerender } = render(<MessageIcon iconData={baseIconData} agent={undefined} />);
|
||||
logCalls.length = 0;
|
||||
|
||||
rerender(<MessageIcon iconData={baseIconData} agent={makeAgent()} />);
|
||||
|
||||
const iconDataCalls = logCalls.filter((c) => c[0] === 'icon_data');
|
||||
expect(iconDataCalls).toHaveLength(1);
|
||||
});
|
||||
|
||||
describe('simulates message lifecycle', () => {
|
||||
it('renders exactly twice during new message + streaming start: initial render + modelLabel update', () => {
|
||||
// Phase 1: Initial response message created by useChatFunctions
|
||||
// model is set to agent_id, iconURL is undefined, modelLabel is '' or sender
|
||||
const initialIconData: TMessageIcon = {
|
||||
endpoint: EModelEndpoint.agents,
|
||||
model: 'agent_123',
|
||||
iconURL: undefined,
|
||||
modelLabel: '', // Not yet resolved
|
||||
isCreatedByUser: false,
|
||||
};
|
||||
const agent = makeAgent();
|
||||
|
||||
const { rerender } = render(<MessageIcon iconData={initialIconData} agent={agent} />);
|
||||
|
||||
// Phase 2: First streaming chunk arrives, messageLabel resolves to agent name
|
||||
// This is a legitimate re-render — modelLabel changed from '' to 'GitHub Agent'
|
||||
const streamingIconData: TMessageIcon = {
|
||||
...initialIconData,
|
||||
modelLabel: 'GitHub Agent',
|
||||
};
|
||||
|
||||
rerender(<MessageIcon iconData={streamingIconData} agent={agent} />);
|
||||
|
||||
const iconDataCalls = logCalls.filter((c) => c[0] === 'icon_data');
|
||||
// Exactly 2: initial mount + modelLabel change
|
||||
expect(iconDataCalls).toHaveLength(2);
|
||||
});
|
||||
|
||||
it('does NOT re-render on subsequent streaming chunks (content changes, isSubmitting stays true)', () => {
|
||||
const iconData: TMessageIcon = {
|
||||
endpoint: EModelEndpoint.agents,
|
||||
model: 'agent_123',
|
||||
iconURL: undefined,
|
||||
modelLabel: 'GitHub Agent',
|
||||
isCreatedByUser: false,
|
||||
};
|
||||
const agent = makeAgent();
|
||||
|
||||
const { rerender } = render(<MessageIcon iconData={iconData} agent={agent} />);
|
||||
logCalls.length = 0;
|
||||
|
||||
// Simulate multiple parent re-renders from streaming chunks
|
||||
// Parent (ContentRender) re-renders because chatContext changed,
|
||||
// but MessageIcon props are identical field-by-field
|
||||
for (let i = 0; i < 5; i++) {
|
||||
rerender(<MessageIcon iconData={{ ...iconData }} agent={makeAgent()} />);
|
||||
}
|
||||
|
||||
const iconDataCalls = logCalls.filter((c) => c[0] === 'icon_data');
|
||||
expect(iconDataCalls).toHaveLength(0);
|
||||
});
|
||||
|
||||
it('does NOT re-render when agentsMap context updates with same agent data', () => {
|
||||
const iconData: TMessageIcon = {
|
||||
endpoint: EModelEndpoint.agents,
|
||||
model: 'agent_123',
|
||||
iconURL: undefined,
|
||||
modelLabel: 'GitHub Agent',
|
||||
isCreatedByUser: false,
|
||||
};
|
||||
|
||||
// First render with agent from original agentsMap
|
||||
const agent1 = makeAgent();
|
||||
const { rerender } = render(<MessageIcon iconData={iconData} agent={agent1} />);
|
||||
logCalls.length = 0;
|
||||
|
||||
// agentsMap refetched → new agent object, same display data
|
||||
const agent2 = makeAgent();
|
||||
expect(agent1).not.toBe(agent2); // different reference
|
||||
rerender(<MessageIcon iconData={iconData} agent={agent2} />);
|
||||
|
||||
const iconDataCalls = logCalls.filter((c) => c[0] === 'icon_data');
|
||||
expect(iconDataCalls).toHaveLength(0);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
@ -48,7 +48,6 @@ export default function MessageContent(props: TMessageProps) {
|
|||
</div>
|
||||
</MessageContainer>
|
||||
<MultiMessage
|
||||
key={messageId}
|
||||
messageId={messageId}
|
||||
conversation={conversation}
|
||||
messagesTree={children ?? []}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue