From b4d97bd8881909c09542df70f47cbef9fee1c8da Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Thu, 2 Apr 2026 22:29:54 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=97=9C=EF=B8=8F=20refactor:=20Eliminate?= =?UTF-8?q?=20Unstable=20React=20Keys=20During=20SSE=20Lifecycle=20(#12536?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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. --- .../src/components/Chat/Messages/Message.tsx | 1 - .../components/Chat/Messages/MessageIcon.tsx | 72 ++++--- .../components/Chat/Messages/MessageParts.tsx | 1 - .../components/Chat/Messages/MessagesView.tsx | 1 - .../components/Chat/Messages/MultiMessage.tsx | 61 +++--- .../__tests__/MessageIcon.render.test.tsx | 193 ++++++++++++++++++ .../components/Messages/MessageContent.tsx | 1 - 7 files changed, 264 insertions(+), 66 deletions(-) create mode 100644 client/src/components/Chat/Messages/__tests__/MessageIcon.render.test.tsx diff --git a/client/src/components/Chat/Messages/Message.tsx b/client/src/components/Chat/Messages/Message.tsx index 53aef812fc..fc2a79fca0 100644 --- a/client/src/components/Chat/Messages/Message.tsx +++ b/client/src/components/Chat/Messages/Message.tsx @@ -47,7 +47,6 @@ export default function Message(props: TMessageProps) { { - 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 ?? ''; diff --git a/client/src/components/Chat/Messages/MessageParts.tsx b/client/src/components/Chat/Messages/MessageParts.tsx index 3d13fa6ae0..2d0e3d512b 100644 --- a/client/src/components/Chat/Messages/MessageParts.tsx +++ b/client/src/components/Chat/Messages/MessageParts.tsx @@ -179,7 +179,6 @@ export default function Message(props: TMessageProps) {
- ); + return ; } else if (message.content) { - return ( - - ); + return ; } - return ( - - ); + return ; } diff --git a/client/src/components/Chat/Messages/__tests__/MessageIcon.render.test.tsx b/client/src/components/Chat/Messages/__tests__/MessageIcon.render.test.tsx new file mode 100644 index 0000000000..1c7b3c1aec --- /dev/null +++ b/client/src/components/Chat/Messages/__tests__/MessageIcon.render.test.tsx @@ -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) => ( +
+ ); + ConvoIconURL.displayName = 'ConvoIconURL'; + return { __esModule: true, default: ConvoIconURL }; +}); +jest.mock('~/components/Endpoints/Icon', () => { + const Icon = (props: Record) => ( +
+ ); + Icon.displayName = 'Icon'; + return { __esModule: true, default: Icon }; +}); + +import MessageIcon from '../MessageIcon'; + +const makeAgent = (overrides?: Partial): 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(); + 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(); + logCalls.length = 0; + + // Simulate parent re-render: new iconData object (same field values), new agent object (same data) + rerender(); + + 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(); + logCalls.length = 0; + + // New agent object with different id but same display fields + const agent2 = makeAgent({ id: 'agent_456' }); + rerender(); + + 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(); + logCalls.length = 0; + + const agent2 = makeAgent({ avatar: { filepath: '/images/new-avatar.png' } }); + rerender(); + + 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(); + logCalls.length = 0; + + rerender(); + + 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(); + + // 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(); + + 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(); + 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(); + } + + 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(); + logCalls.length = 0; + + // agentsMap refetched → new agent object, same display data + const agent2 = makeAgent(); + expect(agent1).not.toBe(agent2); // different reference + rerender(); + + const iconDataCalls = logCalls.filter((c) => c[0] === 'icon_data'); + expect(iconDataCalls).toHaveLength(0); + }); + }); +}); diff --git a/client/src/components/Messages/MessageContent.tsx b/client/src/components/Messages/MessageContent.tsx index 977e397022..67865ed397 100644 --- a/client/src/components/Messages/MessageContent.tsx +++ b/client/src/components/Messages/MessageContent.tsx @@ -48,7 +48,6 @@ export default function MessageContent(props: TMessageProps) {