🩹 fix: Make code-file artifacts click-to-open only

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).
This commit is contained in:
Danny Avila 2026-04-29 07:58:14 -04:00
parent 03fd68288b
commit 6761531287
3 changed files with 93 additions and 199 deletions

View file

@ -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<boolean | null>(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);
};

View file

@ -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> = {}): 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(<RecoilRoot initializeState={initializeState}>{ui}</RecoilRoot>);
};
const renderWith = (ui: React.ReactElement) => render(<RecoilRoot>{ui}</RecoilRoot>);
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(
<RecoilRoot initializeState={initializeState}>
<RecoilRoot>
<StateProbe
onSnapshot={(snap) => {
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: '<h1>hi</h1>',
} as Partial<TAttachment>);
// 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(<Attachment attachment={html()} />);
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(<Attachment attachment={html()} />);
// 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(<Attachment attachment={html()} />);
// 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(
<AttachmentGroup attachments={[olderHtml, newerHtml]} />,
);
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: '<h1>prev</h1>',
} as Partial<TAttachment>);
const { getSnapshot } = renderWithProbe(<Attachment attachment={html} />, {
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: '<h1>fresh</h1>',
} as Partial<TAttachment>);
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(
<RecoilRoot initializeState={initializeState}>
<StateProbe
onSnapshot={(snap) => {
snapshot = snap;
}}
/>
<Attachment attachment={html} />
</RecoilRoot>,
);
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: '<h1>old</h1>',
} as Partial<TAttachment>);
const initializeState = (snap: MutableSnapshot) => {
snap.set(store.isSubmittingFamily(0), false);
snap.set(store.artifactsVisibility, false);
};
let snapshot: ArtifactsSnapshot = {
visibility: false,
currentArtifactId: null,
artifactIds: [],
};
render(
<RecoilRoot initializeState={initializeState}>
<RecoilRoot
initializeState={(snap) => {
snap.set(store.artifactsVisibility, false);
}}
>
<StateProbe
onSnapshot={(snap) => {
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: '<h1>prev</h1>',
} as Partial<TAttachment>);
const { getSnapshot } = renderWithProbe(<Attachment attachment={html} />, {
streaming: false,
});
const { getSnapshot } = renderWithProbe(<Attachment attachment={html} />);
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);
});
});

View file

@ -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> = {}): 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(<RecoilRoot initializeState={initializeState}>{ui}</RecoilRoot>);
};
const renderWith = (ui: React.ReactElement) => render(<RecoilRoot>{ui}</RecoilRoot>);
describe('LogContent attachment routing', () => {
it('routes HTML attachments through ToolArtifactCard (panel)', () => {
@ -86,8 +76,10 @@ describe('LogContent attachment routing', () => {
text: '<h1>hi</h1>',
} as Partial<TAttachment>);
renderWith(<LogContent output="" attachments={[html]} />);
// 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<TAttachment>);
renderWith(<LogContent output="" attachments={[mmd]} />);
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 <pre>, not the panel', () => {
@ -111,7 +103,7 @@ describe('LogContent attachment routing', () => {
} as Partial<TAttachment>);
const { container } = renderWith(<LogContent output="" attachments={[csv]} />);
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<TAttachment>);
renderWith(<LogContent output="" attachments={[expired]} />);
// 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<TAttachment>);
renderWith(<LogContent output="" attachments={[fresh]} />);
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', () => {