From 67615312878d58da1f9ea3cd40da5ea454d9e699 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Wed, 29 Apr 2026 07:58:14 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=A9=B9=20fix:=20Make=20code-file=20artifa?= =?UTF-8?q?cts=20click-to-open=20only?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removes mount-time auto-open from `ToolArtifactCard`. Streaming arrivals no longer hijack the panel — even a freshly-emitted SSE artifact registers silently in `artifactsState` and waits for the user to click. Combined with `Presentation`'s `currentArtifactId != null` render gate, the panel stays closed across history navigation, page reload, and SSE arrival. Click is the only path that opens the panel. `handleOpen` is unchanged: first click focuses + reveals, second click on the same chip closes. Dropped: - `useRecoilCallback` snapshot read of `isSubmittingFamily(0)` - `mountedDuringStreamRef` ref + lazy-init block - The whole focus + visibility effect (was effect 3) - `useRef` import (now unused) Tests: - `ArtifactRouting.test.tsx` rewritten to exercise the click path: registers-on-mount-without-focus, click-to-open-then-close, multi- card-no-auto-focus, click-when-visibility-was-false. The streaming state is no longer seeded; both `renderWith` and `renderWithProbe` collapsed back to plain `RecoilRoot`. - `LogContent.test.tsx` flips its panel-routing assertions from `pressed: true` (which asserted auto-focus) to `pressed: false` with a chip-title check (which asserts the panel card rendered but stayed unfocused). --- .../Content/Parts/ToolArtifactCard.tsx | 81 ++------ .../Parts/__tests__/ArtifactRouting.test.tsx | 178 +++++++----------- .../Parts/__tests__/LogContent.test.tsx | 33 ++-- 3 files changed, 93 insertions(+), 199 deletions(-) diff --git a/client/src/components/Chat/Messages/Content/Parts/ToolArtifactCard.tsx b/client/src/components/Chat/Messages/Content/Parts/ToolArtifactCard.tsx index c786df4e97..12d6590298 100644 --- a/client/src/components/Chat/Messages/Content/Parts/ToolArtifactCard.tsx +++ b/client/src/components/Chat/Messages/Content/Parts/ToolArtifactCard.tsx @@ -1,12 +1,6 @@ -import { memo, useEffect, useId, useLayoutEffect, useRef } from 'react'; +import { memo, useEffect, useId, useLayoutEffect } from 'react'; import { Download } from 'lucide-react'; -import { - useRecoilCallback, - useRecoilState, - useRecoilValue, - useResetRecoilState, - useSetRecoilState, -} from 'recoil'; +import { useRecoilState, useRecoilValue, useResetRecoilState, useSetRecoilState } from 'recoil'; import type { TAttachment, TFile, TAttachmentMetadata } from 'librechat-data-provider'; import type { Artifact } from '~/common'; import FilePreview from '~/components/Chat/Input/Files/FilePreview'; @@ -24,7 +18,7 @@ interface ToolArtifactCardProps { /** * Card that opens a code-execution-produced artifact in the side panel. * - * Three effects, separately scoped: + * Two effects, separately scoped: * * 1. **Dedup claim** (`useLayoutEffect`, runs synchronously before * paint). The same file can appear in multiple tool calls within a @@ -48,20 +42,15 @@ interface ToolArtifactCardProps { * Without that guard, both cards would observe each other's write * and trade overwrites in a loop. * - * 3. **Focus + open on mount** (deps: artifact.id) — gated on - * `isSubmitting` captured at first render via a ref. A card mounted - * *during* streaming means a new artifact arrived from SSE; we - * steal panel focus AND force `artifactsVisibility = true` so the - * panel actually opens (matching the legacy SSE auto-open UX). - * Toggling visibility on every streaming arrival means a previously - * closed panel re-opens for a fresh artifact — that's the explicit - * "auto-open when first created" behavior. A card mounted while - * `isSubmitting === false` is part of conversation history (page - * load, navigating back to an old conversation) and must not steal - * focus or open the panel — `Presentation`'s render condition gates - * on `currentArtifactId != null`, so leaving both alone keeps the - * panel closed on history load. Click-to-open path (`handleOpen`) - * remains unchanged so users can manually open historical chips. + * **No auto-open.** The card never sets `currentArtifactId` or + * `artifactsVisibility` on mount, regardless of whether streaming is + * active. Code-file artifacts are click-to-open only — the user + * explicitly chooses to surface a file in the panel rather than having + * a streaming arrival hijack their viewport. Combined with + * `Presentation`'s `currentArtifactId != null` render gate, this means + * the panel stays closed across both history navigation and SSE + * arrival until the user clicks a chip. `handleOpen` is the sole + * focus + visibility toggle. */ const ToolArtifactCard = memo(({ attachment, artifact }: ToolArtifactCardProps) => { const localize = useLocalize(); @@ -75,30 +64,6 @@ const ToolArtifactCard = memo(({ attachment, artifact }: ToolArtifactCardProps) const [claim, setClaim] = useRecoilState(store.toolArtifactClaim(artifact.id)); const isSelected = artifact.id === currentArtifactId; const isMyClaim = claim === claimKey; - /** - * Captured at first render via a non-subscribing snapshot read so the - * downstream effect doesn't re-fire (and the component doesn't - * re-render) every time `isSubmittingFamily(0)` flips. Cards that mount - * mid-stream stay "fresh" for the rest of their lifetime; cards that - * mount post-stream stay "history" even if the user sends a new - * message while this card stays mounted. - */ - const readInitialIsSubmitting = useRecoilCallback( - ({ snapshot }) => - () => - // `valueMaybe()` returns `undefined` if the atom is in an error - // or loading state instead of throwing — defensive against an - // upstream selector failure surfacing during card mount. The - // `?? false` default is correct because a card we can't classify - // as streaming is one we should treat as history (don't steal - // focus / open the panel). - snapshot.getLoadable(store.isSubmittingFamily(0)).valueMaybe() ?? false, - [], - ); - const mountedDuringStreamRef = useRef(null); - if (mountedDuringStreamRef.current === null) { - mountedDuringStreamRef.current = readInitialIsSubmitting(); - } useLayoutEffect(() => { // Always (re)claim on mount — a later card for the same id displaces @@ -131,21 +96,6 @@ const ToolArtifactCard = memo(({ attachment, artifact }: ToolArtifactCardProps) setArtifacts((prev) => ({ ...(prev ?? {}), [artifact.id]: artifact })); }, [artifact, existingEntry, isMyClaim, setArtifacts]); - useEffect(() => { - if (!mountedDuringStreamRef.current) { - // Card mounted as part of conversation history — leave focus and - // visibility alone so the side panel doesn't auto-open on navigation. - return; - } - // Streaming arrival: focus the new artifact AND force the panel - // visible. Without `setVisible(true)`, a session where the user had - // previously closed the panel (visibility=false) would surface the - // selection in the chip ("click to close") but never actually open - // — `Presentation` gates rendering on visibility. - setCurrentArtifactId(artifact.id); - setVisible(true); - }, [artifact.id, setCurrentArtifactId, setVisible]); - const file = attachment as TFile & TAttachmentMetadata; const { handleDownload } = useAttachmentLink({ href: attachment.filepath ?? '', @@ -161,8 +111,11 @@ const ToolArtifactCard = memo(({ attachment, artifact }: ToolArtifactCardProps) setVisible(false); return; } - // Registration already happened in the mount effect; the click only - // needs to focus + reveal the panel for users who have closed it. + // The self-heal registration effect already wrote the artifact to + // `artifactsState`; clicking just focuses it and forces visibility + // so the panel reveals itself. This is the *only* path that opens + // the panel for tool-artifact files — there is no auto-open on + // streaming arrival. setCurrentArtifactId(artifact.id); setVisible(true); }; diff --git a/client/src/components/Chat/Messages/Content/Parts/__tests__/ArtifactRouting.test.tsx b/client/src/components/Chat/Messages/Content/Parts/__tests__/ArtifactRouting.test.tsx index 76f4e77478..7902580d24 100644 --- a/client/src/components/Chat/Messages/Content/Parts/__tests__/ArtifactRouting.test.tsx +++ b/client/src/components/Chat/Messages/Content/Parts/__tests__/ArtifactRouting.test.tsx @@ -1,7 +1,6 @@ import React from 'react'; import { render, screen, fireEvent, act } from '@testing-library/react'; import { RecoilRoot, useRecoilValue } from 'recoil'; -import type { MutableSnapshot } from 'recoil'; import type { TAttachment } from 'librechat-data-provider'; import Attachment, { AttachmentGroup } from '../Attachment'; import store from '~/store'; @@ -58,18 +57,12 @@ const baseAttachment = (overrides: Partial = {}): TAttachment => }) as TAttachment; /** - * Seeds `isSubmittingFamily(0) = streaming` so `ToolArtifactCard`'s - * mount-time focus effect (which is gated on streaming state) behaves - * the way each test expects. Default `streaming: true` matches the - * legacy SSE-arrival flow that the bulk of these tests exercise. + * `ToolArtifactCard` no longer reads `isSubmittingFamily` — code-file + * artifacts are click-to-open only — so tests don't need to seed + * streaming state. Kept as a thin wrapper so the call sites stay + * uniform and a future shared seed has one place to land. */ -const renderWith = (ui: React.ReactElement, opts: { streaming?: boolean } = {}) => { - const streaming = opts.streaming ?? true; - const initializeState = (snapshot: MutableSnapshot) => { - snapshot.set(store.isSubmittingFamily(0), streaming); - }; - return render({ui}); -}; +const renderWith = (ui: React.ReactElement) => render({ui}); interface ArtifactsSnapshot { visibility: boolean; @@ -91,18 +84,14 @@ const StateProbe = ({ onSnapshot }: { onSnapshot: (snap: ArtifactsSnapshot) => v return null; }; -const renderWithProbe = (ui: React.ReactElement, opts: { streaming?: boolean } = {}) => { - const streaming = opts.streaming ?? true; - const initializeState = (snap: MutableSnapshot) => { - snap.set(store.isSubmittingFamily(0), streaming); - }; +const renderWithProbe = (ui: React.ReactElement) => { let snapshot: ArtifactsSnapshot = { visibility: false, currentArtifactId: null, artifactIds: [], }; const utils = render( - + { snapshot = snap; @@ -127,9 +116,10 @@ describe('Attachment routing for tool artifacts', () => { // Card body shows the artifact title expect(screen.getByText('index.html')).toBeInTheDocument(); - // Open-panel button carries aria-pressed (auto-focused on mount per the - // legacy auto-open behaviour). - expect(screen.getByRole('button', { pressed: true })).toBeInTheDocument(); + // Open-panel button is rendered but unpressed — code-file artifacts + // never auto-focus on mount. + expect(screen.getByRole('button', { pressed: false })).toBeInTheDocument(); + expect(screen.queryByRole('button', { pressed: true })).not.toBeInTheDocument(); // Download button has the download aria-label expect( screen.getByRole('button', { name: /com_ui_download.*index\.html/i }), @@ -202,40 +192,52 @@ describe('ToolArtifactCard click behaviour', () => { text: '

hi

', } as Partial); - // Auto-open invariant: rendering a tool-artifact card must (a) register - // the artifact in `artifactsState` and (b) focus it via - // `currentArtifactId`. With `artifactsVisibility` defaulting to `true`, - // this surfaces the latest tool artifact in the side panel — matching - // the legacy streaming-artifact UX. - it('registers and auto-focuses the artifact on mount', () => { + // Mount registers the artifact in `artifactsState` (so the panel can + // find it on click) but does NOT focus it or open the panel — + // code-file artifacts are click-to-open only. + it('registers the artifact on mount but does not auto-focus or open the panel', () => { const { getSnapshot } = renderWithProbe(); const snap = getSnapshot(); expect(snap.artifactIds).toContain('tool-artifact-html-1'); - expect(snap.currentArtifactId).toBe('tool-artifact-html-1'); + expect(snap.currentArtifactId).toBeNull(); }); - it('toggles closed when the user clicks the (already-selected) card', () => { + it('opens on first click and closes on a second click of the same chip', () => { const { getSnapshot } = renderWithProbe(); - // Mount auto-focuses; the open button is in the pressed state. + // No card is focused on mount; the open button is unpressed. + const openButton = screen.getByRole('button', { pressed: false }); + act(() => { + fireEvent.click(openButton); + }); + const opened = getSnapshot(); + expect(opened.currentArtifactId).toBe('tool-artifact-html-1'); + expect(opened.visibility).toBe(true); + // Click the (now-pressed) button to close. const closeButton = screen.getByRole('button', { pressed: true }); act(() => { fireEvent.click(closeButton); }); - const snap = getSnapshot(); - expect(snap.currentArtifactId).toBeNull(); - expect(snap.visibility).toBe(false); + const closed = getSnapshot(); + expect(closed.currentArtifactId).toBeNull(); + expect(closed.visibility).toBe(false); // Self-heal effect keeps the artifact registered so re-opening works. - expect(snap.artifactIds).toContain('tool-artifact-html-1'); + expect(closed.artifactIds).toContain('tool-artifact-html-1'); }); it('reopens after a close (regression: artifact still openable post-close)', () => { const { getSnapshot } = renderWithProbe(); + // First click: open. + act(() => { + fireEvent.click(screen.getByRole('button', { pressed: false })); + }); + expect(getSnapshot().currentArtifactId).toBe('tool-artifact-html-1'); + // Second click: close. act(() => { fireEvent.click(screen.getByRole('button', { pressed: true })); }); expect(getSnapshot().visibility).toBe(false); expect(getSnapshot().currentArtifactId).toBeNull(); - // Click the now-unpressed open button again. + // Third click: re-open. act(() => { fireEvent.click(screen.getByRole('button', { pressed: false })); }); @@ -350,8 +352,12 @@ describe('ToolArtifactCard click behaviour', () => { expect(screen.getAllByTestId('mermaid-render')).toHaveLength(1); }); - it('focuses the latest card when multiple artifacts mount (legacy parity)', () => { - // Order: older first, newer last. Last-mounted should win currentArtifactId. + it('does NOT auto-focus any card when multiple artifacts mount in one group', () => { + // Mount-time focus was removed: code-file artifacts are + // click-to-open only. Even mounting many panel-eligible cards in a + // single group leaves `currentArtifactId` null until the user + // clicks one. All artifacts still get registered so they're + // available the moment a click happens. const olderHtml = baseAttachment({ file_id: 'older', filename: 'old.html', @@ -365,89 +371,33 @@ describe('ToolArtifactCard click behaviour', () => { const { getSnapshot } = renderWithProbe( , ); - expect(getSnapshot().currentArtifactId).toBe('tool-artifact-newer'); + expect(getSnapshot().currentArtifactId).toBeNull(); expect(getSnapshot().artifactIds).toEqual( expect.arrayContaining(['tool-artifact-older', 'tool-artifact-newer']), ); }); - it('does NOT auto-focus when the card mounts outside an active stream (history load)', () => { - // Reproduces "navigating to a previous conversation". Cards mount - // for already-completed turns; `isSubmitting` is false. The legacy - // behavior would auto-focus the latest artifact and re-open the - // panel — we don't want that. + it('does NOT toggle visibility on mount (closed panel stays closed across both fresh streams and history)', () => { + /** Single behavior across all mount contexts: tool-artifact files + * never auto-toggle the panel. A previously-closed panel stays + * closed even when a fresh artifact arrives via SSE — the user + * has to click to surface it. */ const html = baseAttachment({ - file_id: 'history-html', - filename: 'previous.html', - text: '

prev

', - } as Partial); - const { getSnapshot } = renderWithProbe(, { - streaming: false, - }); - const snap = getSnapshot(); - // Artifact still gets registered (so the panel can find it on click)… - expect(snap.artifactIds).toContain('tool-artifact-history-html'); - // …but currentArtifactId stays null, which means - // `Presentation`'s render gate (`currentArtifactId != null`) keeps - // the panel closed. - expect(snap.currentArtifactId).toBeNull(); - }); - - it('forces panel visibility on streaming mount even if visibility was previously false', () => { - // Repro: user closed the panel earlier in the session - // (`artifactsVisibility = false`), then a new tool artifact - // arrives via SSE. The old behavior set `currentArtifactId` but - // left visibility alone, so the chip showed "click to close" while - // the panel stayed hidden. Streaming arrivals must re-open the - // panel — that's the explicit "auto-open when first created" rule. - const html = baseAttachment({ - file_id: 'fresh-stream', + file_id: 'no-auto-open', filename: 'fresh.html', text: '

fresh

', } as Partial); - const initializeState = (snap: MutableSnapshot) => { - snap.set(store.isSubmittingFamily(0), true); - snap.set(store.artifactsVisibility, false); - }; let snapshot: ArtifactsSnapshot = { - visibility: false, + visibility: true, currentArtifactId: null, artifactIds: [], }; render( - - { - snapshot = snap; - }} - /> - - , - ); - expect(snapshot.visibility).toBe(true); - expect(snapshot.currentArtifactId).toBe('tool-artifact-fresh-stream'); - }); - - it('does NOT force visibility on history mount (closed panel stays closed)', () => { - // The flip-side of the prior test: a card mounted from history - // (isSubmitting=false) must not toggle visibility, so a user who - // explicitly closed the panel keeps it closed when revisiting. - const html = baseAttachment({ - file_id: 'history-no-vis', - filename: 'historic.html', - text: '

old

', - } as Partial); - const initializeState = (snap: MutableSnapshot) => { - snap.set(store.isSubmittingFamily(0), false); - snap.set(store.artifactsVisibility, false); - }; - let snapshot: ArtifactsSnapshot = { - visibility: false, - currentArtifactId: null, - artifactIds: [], - }; - render( - + { + snap.set(store.artifactsVisibility, false); + }} + > { snapshot = snap; @@ -458,20 +408,18 @@ describe('ToolArtifactCard click behaviour', () => { ); expect(snapshot.visibility).toBe(false); expect(snapshot.currentArtifactId).toBeNull(); + expect(snapshot.artifactIds).toContain('tool-artifact-no-auto-open'); }); - it('clicking a history-loaded card focuses it (user-initiated open)', () => { - // Even though the mount-time auto-focus is suppressed for history, - // the click handler is unconditional — users explicitly opening a - // chip must always work. + it('clicking a card focuses it and forces the panel visible', () => { + // The click handler is the *only* path that opens the panel — + // mount never does it. This is the user-explicit open flow. const html = baseAttachment({ - file_id: 'history-click', + file_id: 'click-open', filename: 'previous.html', text: '

prev

', } as Partial); - const { getSnapshot } = renderWithProbe(, { - streaming: false, - }); + const { getSnapshot } = renderWithProbe(); expect(getSnapshot().currentArtifactId).toBeNull(); /** Pin to the panel-open button by name — the download button has no * `aria-pressed`, but `getByRole('button', { pressed: false })` @@ -481,7 +429,7 @@ describe('ToolArtifactCard click behaviour', () => { act(() => { fireEvent.click(openButton); }); - expect(getSnapshot().currentArtifactId).toBe('tool-artifact-history-click'); + expect(getSnapshot().currentArtifactId).toBe('tool-artifact-click-open'); expect(getSnapshot().visibility).toBe(true); }); }); diff --git a/client/src/components/Chat/Messages/Content/Parts/__tests__/LogContent.test.tsx b/client/src/components/Chat/Messages/Content/Parts/__tests__/LogContent.test.tsx index fa14cca11c..0cdfc11a2f 100644 --- a/client/src/components/Chat/Messages/Content/Parts/__tests__/LogContent.test.tsx +++ b/client/src/components/Chat/Messages/Content/Parts/__tests__/LogContent.test.tsx @@ -1,10 +1,8 @@ import React from 'react'; import { render, screen } from '@testing-library/react'; import { RecoilRoot } from 'recoil'; -import type { MutableSnapshot } from 'recoil'; import type { TAttachment } from 'librechat-data-provider'; import LogContent from '../LogContent'; -import store from '~/store'; jest.mock('~/hooks', () => ({ useLocalize: @@ -65,18 +63,10 @@ const baseAttachment = (overrides: Partial = {}): TAttachment => }) as TAttachment; /** - * Default `streaming: true` so `ToolArtifactCard`'s mount-time - * auto-focus (gated on `isSubmittingFamily(0)`) fires for these tests. - * The legacy SSE-arrival flow is what every "panel routing" assertion - * implicitly assumes. + * Tool-artifact files are click-to-open only — no streaming state to + * seed. Plain `RecoilRoot` is enough. */ -const renderWith = (ui: React.ReactElement, opts: { streaming?: boolean } = {}) => { - const streaming = opts.streaming ?? true; - const initializeState = (snapshot: MutableSnapshot) => { - snapshot.set(store.isSubmittingFamily(0), streaming); - }; - return render({ui}); -}; +const renderWith = (ui: React.ReactElement) => render({ui}); describe('LogContent attachment routing', () => { it('routes HTML attachments through ToolArtifactCard (panel)', () => { @@ -86,8 +76,10 @@ describe('LogContent attachment routing', () => { text: '

hi

', } as Partial); renderWith(); - // The panel card carries an aria-pressed state; auto-focused on mount. - expect(screen.getByRole('button', { pressed: true })).toBeInTheDocument(); + // The panel card renders with an unpressed open button — code-file + // artifacts are click-to-open, not auto-focused on mount. The + // `aria-pressed` button + chip title is the proxy for "panel-routed". + expect(screen.getByRole('button', { pressed: false })).toBeInTheDocument(); expect(screen.getByText('index.html')).toBeInTheDocument(); }); @@ -99,8 +91,8 @@ describe('LogContent attachment routing', () => { } as Partial); renderWith(); expect(screen.getByTestId('mermaid-render')).toHaveTextContent('graph TD'); - // Panel card not rendered for mermaid - expect(screen.queryByRole('button', { pressed: true })).not.toBeInTheDocument(); + // Panel card not rendered for mermaid (no aria-pressed button at all) + expect(screen.queryByRole('button', { pressed: false })).not.toBeInTheDocument(); }); it('routes text-bearing CSV through inline 
, not the panel', () => {
@@ -111,7 +103,7 @@ describe('LogContent attachment routing', () => {
     } as Partial);
     const { container } = renderWith();
     expect(container.querySelector('pre')).not.toBeNull();
-    expect(screen.queryByRole('button', { pressed: true })).not.toBeInTheDocument();
+    expect(screen.queryByRole('button', { pressed: false })).not.toBeInTheDocument();
     expect(screen.queryByTestId('mermaid-render')).not.toBeInTheDocument();
   });
 
@@ -151,7 +143,7 @@ describe('LogContent attachment routing', () => {
     } as Partial);
     renderWith();
     // No panel card and no log-link (the expired branch returns plain text).
-    expect(screen.queryByRole('button', { pressed: true })).not.toBeInTheDocument();
+    expect(screen.queryByRole('button', { pressed: false })).not.toBeInTheDocument();
     expect(screen.queryByTestId('log-link')).not.toBeInTheDocument();
     // The localize mock returns the key, so we assert the expired-message key
     // appears in the rendered output alongside the filename.
@@ -166,7 +158,8 @@ describe('LogContent attachment routing', () => {
       expiresAt: Date.now() + 60_000,
     } as Partial);
     renderWith();
-    expect(screen.getByRole('button', { pressed: true })).toBeInTheDocument();
+    expect(screen.getByRole('button', { pressed: false })).toBeInTheDocument();
+    expect(screen.getByText('index.html')).toBeInTheDocument();
   });
 
   it('splits a mixed list into the right buckets', () => {