diff --git a/client/src/components/Chat/Messages/Content/Parts/Attachment.tsx b/client/src/components/Chat/Messages/Content/Parts/Attachment.tsx index c7c51b584b1..891f8a711b4 100644 --- a/client/src/components/Chat/Messages/Content/Parts/Attachment.tsx +++ b/client/src/components/Chat/Messages/Content/Parts/Attachment.tsx @@ -15,13 +15,14 @@ import { renderAttachmentKey, } from './attachmentTypes'; import { useLocalize, useAttachmentPreviewSync, useExpandCollapse } from '~/hooks'; -import FileContainer from '~/components/Chat/Input/Files/FileContainer'; import { fileToArtifact, TOOL_ARTIFACT_TYPES } from '~/utils/artifacts'; +import FileContainer from '~/components/Chat/Input/Files/FileContainer'; import Image from '~/components/Chat/Messages/Content/Image'; import { ROW_GLYPH_SLOT, TOOL_ROW_CLASSES } from '../rows'; import ToolMermaidArtifact from './ToolMermaidArtifact'; import ToolArtifactCard from './ToolArtifactCard'; import { useAttachmentLink } from './LogLink'; +import { fileIdentity } from '~/utils/map'; import { cn } from '~/utils'; const COLLAPSED_MAX_HEIGHT = 320; @@ -179,10 +180,22 @@ const FileAttachmentGroup = memo(({ attachments }: { attachments: TAttachment[] const panelId = useId(); const [isExpanded, setIsExpanded] = useState(false); const { style: expandStyle, ref: expandRef } = useExpandCollapse(isExpanded); - const visibleAttachments = useMemo( - () => attachments.filter((attachment) => Boolean(attachment.filepath)), - [attachments], - ); + const visibleAttachments = useMemo(() => { + // Same file identity can arrive twice (e.g. two tool calls touching + // one file in a message) — keep the last occurrence so the folded + // row lists it once instead of listing the same name twice. + const byIdentity = new Map(); + let unidentifiedCount = 0; + for (const attachment of attachments) { + if (!attachment.filepath) { + continue; + } + const key = fileIdentity(attachment) ?? `__unidentified-${unidentifiedCount++}`; + byIdentity.delete(key); + byIdentity.set(key, attachment); + } + return Array.from(byIdentity.values()); + }, [attachments]); const count = visibleAttachments.length; const summary = useMemo(() => { const names = visibleAttachments.map((attachment) => displayFilename(attachment.filename)); diff --git a/client/src/components/Chat/Messages/Content/Parts/ToolArtifactCard.tsx b/client/src/components/Chat/Messages/Content/Parts/ToolArtifactCard.tsx index de6940ddda3..99dc80665ca 100644 --- a/client/src/components/Chat/Messages/Content/Parts/ToolArtifactCard.tsx +++ b/client/src/components/Chat/Messages/Content/Parts/ToolArtifactCard.tsx @@ -1,4 +1,4 @@ -import { memo, useEffect, useId, useLayoutEffect, useRef } from 'react'; +import { memo, useEffect, useLayoutEffect, useRef } from 'react'; import { useRecoilCallback, useRecoilState, @@ -9,6 +9,7 @@ import { import type { TAttachment, TFile, TAttachmentMetadata } from 'librechat-data-provider'; import type { Artifact } from '~/common'; import { artifactRowKind, isCodeOnlyArtifact } from '~/utils/artifacts'; +import useToolArtifactClaim, { isStrictlyNewer } from './claim'; import { displayFilename } from './attachmentTypes'; import { useAttachmentLink } from './LogLink'; import ArtifactRow from './ArtifactRow'; @@ -24,27 +25,30 @@ interface ToolArtifactCardProps { * * Three effects, separately scoped: * - * 1. **Dedup claim** (`useLayoutEffect`, runs synchronously before - * paint). The same file can appear in multiple tool calls within a - * single message (e.g. the agent reads back what it just wrote) or - * across messages. Each card claims `toolArtifactClaim(artifact.id)` - * with its unique component-instance key on mount; the latest card - * to mount wins, so older duplicates re-render to `null`. The atom - * is family-keyed by artifact id, so claims for unrelated artifacts - * don't trigger re-renders here. Cleanup releases the claim if it's - * still ours so a subsequent re-mount can take it. + * 1. **Dedup claim** (`useToolArtifactClaim`, via `useLayoutEffect` + * inside it, runs synchronously before paint). The same file can + * appear in multiple tool calls within a single message (e.g. the + * agent reads back what it just wrote) or across messages. The + * shared hook scopes the claim to the mounting message (falling + * back to the bare artifact id where no message is known), so the + * same file shows one card per message instead of one card total. + * Within one message the latest card to mount wins, so older + * duplicates re-render to `null`. * * 2. **Self-heal registration** subscribes to the per-id selector * `artifactByIdSelector(artifact.id)` and writes only when the - * entry is missing or the cached content/type/title drifted. The + * entry is missing or the cached content/type/title drifted AND + * this card's version is at least as new (`lastUpdateTime`). The * panel's `useArtifacts` hook resets `artifactsState` on close, so * this re-fires deterministically once the slice transitions back - * to `undefined` — without the no-deps render-loop pattern. The - * write is also gated on `isMyClaim`, making the registration - * single-writer per id: when two cards exist for the same file - * across turns, only the latest (claim-holder) updates state. - * Without that guard, both cards would observe each other's write - * and trade overwrites in a loop. + * to `undefined` — without the no-deps render-loop pattern. A + * strictly newer version always wins regardless of mount order; + * when two versions share a `lastUpdateTime` (no timestamp signal), + * the tie breaks on a SEPARATE global "latest mount" claim kept + * only for this purpose — today's semantics, scoped to the id alone + * so it survives across messages. That single-writer tie-break is + * what stops two cards for the same file from observing each + * other's write and trading overwrites in a loop. * * 3. **Focus + open on mount** (deps: artifact.id, artifact.type) — * gated on `isSubmitting` captured at first render via a ref AND @@ -65,7 +69,6 @@ interface ToolArtifactCardProps { * of context. */ const ToolArtifactCard = memo(({ attachment, artifact }: ToolArtifactCardProps) => { - const claimKey = useId(); const file = attachment as TFile & TAttachmentMetadata; const fileId = file.file_id; const setVisible = useSetRecoilState(store.artifactsVisibility); @@ -74,9 +77,15 @@ const ToolArtifactCard = memo(({ attachment, artifact }: ToolArtifactCardProps) const resetCurrentArtifactId = useResetRecoilState(store.currentArtifactId); const currentArtifactId = useRecoilValue(store.currentArtifactId); const existingEntry = useRecoilValue(store.artifactByIdSelector(artifact.id)); - const [claim, setClaim] = useRecoilState(store.toolArtifactClaim(artifact.id)); + const { isMyClaim: canRender, claimKey } = useToolArtifactClaim(artifact.id); + // Global (message-independent) claim — used only as the registration + // tie-break below, never for the render-null gate. Reuses the same + // `claimKey` as the display claim above so the two never fight over the + // SAME atom entry in the no-message fallback, where both keys collapse + // to `store.toolArtifactClaim(artifact.id)`. + const [globalClaim, setGlobalClaim] = useRecoilState(store.toolArtifactClaim(artifact.id)); + const isMyGlobalClaim = globalClaim === claimKey; const isSelected = artifact.id === currentArtifactId; - const isMyClaim = claim === claimKey; /* Read+reset on mount only — `useRecoilCallback` avoids subscribing * to the per-file_id flag (no re-renders when other files resolve). * The deferred-preview hook flips this to `true` on the pending→ready @@ -119,35 +128,41 @@ const ToolArtifactCard = memo(({ attachment, artifact }: ToolArtifactCardProps) } useLayoutEffect(() => { - // Always (re)claim on mount — a later card for the same id displaces - // an earlier one, so the chip migrates to the most recent mention. - setClaim(claimKey); + // Always (re)claim the global slot on mount — keeps today's + // latest-mount semantics as the tie-break input for the registration + // effect below, independent of which message a card renders under. + setGlobalClaim(claimKey); return () => { // Only release when the claim is still ours; if a sibling already // took over we don't want to clobber its claim. - setClaim((prev) => (prev === claimKey ? null : prev)); + setGlobalClaim((prev) => (prev === claimKey ? null : prev)); }; - }, [claimKey, setClaim]); + }, [claimKey, setGlobalClaim]); useEffect(() => { - // Only the claim-winner writes. Two cards with the same `artifact.id` - // but divergent content (same file_id reused across turns) would - // otherwise see each other's write through `existingEntry`, detect - // drift, and trade overwrites in a loop. Gating on `isMyClaim` - // makes registration single-writer per id. - if (!isMyClaim) { - return; - } - if ( + const contentDrifted = !( existingEntry != null && existingEntry.content === artifact.content && existingEntry.type === artifact.type && existingEntry.title === artifact.title - ) { + ); + if (!contentDrifted) { + return; + } + // A strictly newer version always wins, regardless of mount order. + // When two versions share a `lastUpdateTime` (no timestamp signal to + // order them), fall back to the global "latest mount" claim so a + // ping-ponging pair still converges on ONE writer instead of trading + // overwrites in a loop. + const isNewerOrTied = + existingEntry == null || + isStrictlyNewer(artifact, existingEntry) || + (artifact.lastUpdateTime === existingEntry.lastUpdateTime && isMyGlobalClaim); + if (!isNewerOrTied) { return; } setArtifacts((prev) => ({ ...(prev ?? {}), [artifact.id]: artifact })); - }, [artifact, existingEntry, isMyClaim, setArtifacts]); + }, [artifact, existingEntry, isMyGlobalClaim, setArtifacts]); useEffect(() => { if (isCodeOnlyArtifact(artifact.type)) { @@ -207,9 +222,9 @@ const ToolArtifactCard = memo(({ attachment, artifact }: ToolArtifactCardProps) setVisible(true); }; - // Another card with the same artifact id has the active claim — render - // nothing here, that row is the canonical trigger for this file. - if (claim != null && !isMyClaim) { + // Another card holds the message-scoped display claim for this file — + // render nothing here, that row is the canonical trigger for this file. + if (!canRender) { return null; } diff --git a/client/src/components/Chat/Messages/Content/Parts/ToolMermaidArtifact.tsx b/client/src/components/Chat/Messages/Content/Parts/ToolMermaidArtifact.tsx index 1431c3052ab..448d1fc2434 100644 --- a/client/src/components/Chat/Messages/Content/Parts/ToolMermaidArtifact.tsx +++ b/client/src/components/Chat/Messages/Content/Parts/ToolMermaidArtifact.tsx @@ -1,14 +1,14 @@ -import { memo, useId, useLayoutEffect, useMemo, useState } from 'react'; +import { memo, useLayoutEffect, useMemo, useState } from 'react'; +import { useAtom } from 'jotai'; import { Download } from 'lucide-react'; -import { useRecoilState } from 'recoil'; import type { TAttachment, TFile, TAttachmentMetadata } from 'librechat-data-provider'; +import useToolArtifactClaim, { isStrictlyNewer, newestToolArtifactFamily } from './claim'; import { fileToArtifact, TOOL_ARTIFACT_TYPES, toolArtifactKey } from '~/utils/artifacts'; import Mermaid from '~/components/Messages/Content/Mermaid/Mermaid'; import { displayFilename } from './attachmentTypes'; import { useAttachmentLink } from './LogLink'; import { useLocalize } from '~/hooks'; import { cn } from '~/utils'; -import store from '~/store'; interface ToolMermaidArtifactProps { attachment: TAttachment; @@ -20,27 +20,29 @@ interface ToolMermaidArtifactProps { * user opens it in the Artifact panel. The compact card keeps the file * available in chat without rendering the same diagram twice. * - * Shares the `toolArtifactClaim` dedup atom with `ToolArtifactCard` so - * the same `.mmd` file can't double-render across tool calls / messages. + * Shares `useToolArtifactClaim` with `ToolArtifactCard` so the same + * `.mmd` file dedups identically: one card per message (falling back to + * one card total where no message is known). + * + * A diagram remade in a later turn (same file identity, newer + * `lastUpdateTime`) must still open the newest version from an EARLIER + * message's card. Every mount offers this instance's own artifact to + * `newestToolArtifactFamily`, keyed by file identity; the artifact + * handed to `Mermaid` (which is what gets registered once opened) is + * upgraded to that newest entry when it's ahead of this instance's own + * version, while the inline diagram below keeps rendering `text` — this + * message's own source — unchanged. */ const ToolMermaidArtifact = memo(({ attachment, text }: ToolMermaidArtifactProps) => { const localize = useLocalize(); const file = attachment as TFile & TAttachmentMetadata; - const claimKey = useId(); - const [claim, setClaim] = useRecoilState(store.toolArtifactClaim(toolArtifactKey(file))); - const isMyClaim = claim === claimKey; + const fileKey = toolArtifactKey(file); + const { isMyClaim } = useToolArtifactClaim(fileKey); /* Once the diagram collapses into its trigger row, that row carries the * filename and the download itself, so this header would repeat both * beside it. */ const [isRowMode, setIsRowMode] = useState(false); - useLayoutEffect(() => { - setClaim(claimKey); - return () => { - setClaim((prev) => (prev === claimKey ? null : prev)); - }; - }, [claimKey, setClaim]); - const { handleDownload } = useAttachmentLink({ href: attachment.filepath ?? '', filename: attachment.filename ?? '', @@ -53,12 +55,32 @@ const ToolMermaidArtifact = memo(({ attachment, text }: ToolMermaidArtifactProps fileToArtifact({ ...attachment, text }, { preClassifiedType: TOOL_ARTIFACT_TYPES.MERMAID }), [attachment, text], ); + const [newestArtifact, setNewestArtifact] = useAtom(newestToolArtifactFamily(fileKey)); + + useLayoutEffect(() => { + if (artifact == null) { + return; + } + setNewestArtifact((current) => + current == null || isStrictlyNewer(artifact, current) ? artifact : current, + ); + }, [artifact, setNewestArtifact]); - if (claim != null && !isMyClaim) { + if (!isMyClaim) { return null; } const visibleFilename = displayFilename(attachment.filename); + // Adopt the shared family's entry unless THIS instance's own artifact is + // strictly newer than it (can't happen once its own mount effect has + // offered it, but guards the pre-effect render). A tied `lastUpdateTime` + // is "not strictly newer" in either direction, so every mounted instance + // converges on whichever version the family already settled on instead + // of each preferring its own on a tie. + const registeredArtifact = + artifact != null && newestArtifact != null && !isStrictlyNewer(artifact, newestArtifact) + ? newestArtifact + : artifact; return (
@@ -95,7 +117,7 @@ const ToolMermaidArtifact = memo(({ attachment, text }: ToolMermaidArtifactProps {file.file_id ? ( ) : ( ({ + useLocalize: + () => + (key: string): string => + key, + useAttachmentPreviewSync: () => ({ status: 'ready', previewError: undefined, isPolling: false }), + useExpandCollapse: (isExpanded: boolean) => ({ + style: { display: 'grid', gridTemplateRows: isExpanded ? '1fr' : '0fr' }, + ref: { current: null }, + }), +})); + +jest.mock('../LogLink', () => ({ + useAttachmentLink: () => ({ handleDownload: jest.fn() }), +})); + +jest.mock('~/components/Chat/Input/Files/FileContainer', () => ({ + __esModule: true, + default: ({ file, displayName }: { file: { filename?: string }; displayName?: string }) => ( +
{displayName ?? file.filename ?? ''}
+ ), +})); + +jest.mock('~/components/Chat/Input/Files/FilePreview', () => ({ + __esModule: true, + default: () =>
, +})); + +jest.mock('~/components/Chat/Messages/Content/Image', () => ({ + __esModule: true, + default: ({ altText }: { altText?: string }) => {altText, +})); + +jest.mock('~/components/Messages/Content/Mermaid/Mermaid', () => ({ + __esModule: true, + default: ({ children }: { children: string }) => ( +
{children}
+ ), +})); + +jest.mock('~/utils', () => ({ + cn: (...classes: Array) => classes.filter(Boolean).join(' '), + getFileType: () => ({ paths: [], color: '', title: 'Artifact' }), + logger: { log: jest.fn(), warn: jest.fn(), error: jest.fn() }, + isArtifactRoute: () => false, +})); + +const baseAttachment = (overrides: Partial = {}): TAttachment => + ({ + file_id: 'file-1', + filename: 'unset', + filepath: '/files/file-1', + type: 'application/octet-stream', + ...overrides, + }) as TAttachment; + +const renderWith = (ui: React.ReactElement) => render({ui}); + +describe('FileAttachmentGroup identity dedup', () => { + it('collapses two attachments sharing a file identity into a single, non-folded chip', () => { + const first = baseAttachment({ file_id: 'dup', filename: 'report.pptx', bytes: 100 }); + const second = baseAttachment({ file_id: 'dup', filename: 'report.pptx', bytes: 100 }); + const { container } = renderWith(); + + // A folded row only appears once the unique count exceeds one — dedup + // must run before the count that decides whether to render it. + expect(screen.queryByRole('button', { name: 'com_ui_show_n_files' })).not.toBeInTheDocument(); + const chips = container.querySelectorAll('[data-testid="file-container"]'); + expect(chips.length).toBe(1); + expect(chips[0].textContent).toBe('report.pptx'); + }); + + it('folds distinct identities only, using the last occurrence for a repeated identity', () => { + const older = baseAttachment({ file_id: 'dup', filename: 'old-name.zip', bytes: 100 }); + const distinct = baseAttachment({ file_id: 'other', filename: 'notes.zip', bytes: 50 }); + const newer = baseAttachment({ file_id: 'dup', filename: 'new-name.zip', bytes: 100 }); + + const { container } = renderWith(); + + const toggle = screen.getByRole('button', { name: 'com_ui_show_n_files' }); + fireEvent.click(toggle); + const chips = Array.from(container.querySelectorAll('[data-testid="file-container"]')); + const names = chips.map((chip) => chip.textContent); + expect(chips.length).toBe(2); + expect(names).toContain('new-name.zip'); + expect(names).not.toContain('old-name.zip'); + }); +}); + +describe('FileAttachmentGroup identity dedup for id-less files', () => { + it('keeps two id-less attachments with the same filename but different filepaths as two chips', () => { + const first = baseAttachment({ + file_id: undefined, + filename: 'output.csv', + filepath: '/uploads/session-a/output.csv', + }); + const second = baseAttachment({ + file_id: undefined, + filename: 'output.csv', + filepath: '/uploads/session-b/output.csv', + }); + + const { container } = renderWith(); + + const toggle = screen.getByRole('button', { name: 'com_ui_show_n_files' }); + fireEvent.click(toggle); + const chips = container.querySelectorAll('[data-testid="file-container"]'); + expect(chips.length).toBe(2); + }); + + it('collapses two id-less attachments sharing the same filepath into one chip', () => { + const first = baseAttachment({ + file_id: undefined, + filename: 'output.csv', + filepath: '/uploads/session-a/output.csv', + }); + const second = baseAttachment({ + file_id: undefined, + filename: 'output.csv', + filepath: '/uploads/session-a/output.csv', + }); + + const { container } = renderWith(); + + expect(screen.queryByRole('button', { name: 'com_ui_show_n_files' })).not.toBeInTheDocument(); + const chips = container.querySelectorAll('[data-testid="file-container"]'); + expect(chips.length).toBe(1); + }); +}); diff --git a/client/src/components/Chat/Messages/Content/Parts/__tests__/ToolArtifactCard.test.tsx b/client/src/components/Chat/Messages/Content/Parts/__tests__/ToolArtifactCard.test.tsx new file mode 100644 index 00000000000..78196379c53 --- /dev/null +++ b/client/src/components/Chat/Messages/Content/Parts/__tests__/ToolArtifactCard.test.tsx @@ -0,0 +1,650 @@ +import React from 'react'; +import { RecoilRoot, useRecoilValue } from 'recoil'; +import { ContentTypes, Tools } from 'librechat-data-provider'; +import { render, screen, fireEvent } from '@testing-library/react'; +import type { TAttachment, TMessage, TMessageContentParts } from 'librechat-data-provider'; +import type { Artifact } from '~/common'; +import SearchContent from '~/components/Chat/Messages/Content/SearchContent'; +import { AttachmentGroup } from '../Attachment'; +import { MessageContext } from '~/Providers'; +import store from '~/store'; + +jest.mock('~/hooks', () => ({ + useLocalize: + () => + (key: string): string => + key, + useAttachmentPreviewSync: () => ({ status: 'ready', previewError: undefined, isPolling: false }), + useExpandCollapse: (isExpanded: boolean) => ({ + style: { display: 'grid', gridTemplateRows: isExpanded ? '1fr' : '0fr' }, + ref: { current: null }, + }), +})); + +jest.mock('../LogLink', () => ({ + useAttachmentLink: () => ({ handleDownload: jest.fn() }), +})); + +jest.mock('~/components/Chat/Input/Files/FileContainer', () => ({ + __esModule: true, + default: ({ file, displayName }: { file: { filename?: string }; displayName?: string }) => ( +
{displayName ?? file.filename ?? ''}
+ ), +})); + +jest.mock('~/components/Chat/Input/Files/FilePreview', () => ({ + __esModule: true, + default: () =>
, +})); + +jest.mock('~/components/Chat/Messages/Content/Image', () => ({ + __esModule: true, + default: ({ altText }: { altText?: string }) => {altText, +})); + +jest.mock('~/components/Messages/Content/Mermaid/Mermaid', () => ({ + __esModule: true, + default: ({ + children, + artifact, + }: { + children: string; + artifact?: { id: string; title?: string; type?: string; content?: string }; + }) => ( +
+ {children} +
+ ), +})); + +jest.mock('~/utils', () => ({ + cn: (...classes: Array) => classes.filter(Boolean).join(' '), + getFileType: () => ({ paths: [], color: '', title: 'Artifact' }), + logger: { log: jest.fn(), warn: jest.fn(), error: jest.fn() }, + isArtifactRoute: () => false, + // `SearchContent` (rendered by the search-result tests below) maps attachments to + // their owning tool call with the real implementation. + mapAttachments: jest.requireActual('~/utils/map').mapAttachments, +})); + +/** + * `SearchContent` routes an `execute_code` tool call through the real + * `Part` -> `Parts` barrel -> `ExecuteCode`, whose unconditional + * `PtcToolTrace` child needs MCP query hooks (and thus a `QueryClient`) + * this suite doesn't set up. Only that one card is replaced with the + * same `AttachmentGroup` it renders for real — the file-identity + * routing under test (`SearchContent` -> `MessageContext` -> + * `AttachmentGroup` -> `ToolArtifactCard`) stays real. + */ +jest.mock('..', () => { + const actual = jest.requireActual('..'); + return { + __esModule: true, + ...actual, + ExecuteCode: ({ attachments }: { attachments?: TAttachment[] }) => ( + + ), + }; +}); + +const baseAttachment = (overrides: Partial = {}): TAttachment => + ({ + file_id: 'file-1', + filename: 'unset', + filepath: '/files/file-1', + type: 'application/octet-stream', + ...overrides, + }) as TAttachment; + +/** Minimal message context; `isExpanded` is required by the type but unused here. */ +const messageScope = (messageId: string) => ({ messageId, isExpanded: false }); + +const ArtifactContentProbe = ({ + artifactId, + onSnapshot, +}: { + artifactId: string; + onSnapshot: (content: string | null) => void; +}) => { + const artifacts = useRecoilValue(store.artifactsState); + React.useEffect(() => { + onSnapshot(artifacts?.[artifactId]?.content ?? null); + }); + return null; +}; + +describe('ToolArtifactCard message-scoped dedup and newest-version selection', () => { + it('shows a card on every message that holds the same file identity', () => { + const html = () => + baseAttachment({ file_id: 'shared-file', filename: 'index.html', text: '

hi

' }); + const { container } = render( + + + + + + + + , + ); + expect(container.querySelectorAll('[data-artifact-trigger]')).toHaveLength(2); + expect(screen.getAllByText('index.html')).toHaveLength(2); + }); + + it('collapses two cards for the same file within one message to one card', () => { + const dup = baseAttachment({ + file_id: 'dup-in-message', + filename: 'index.html', + text: '

v1

', + }); + const { container } = render( + + + + + + , + ); + expect(container.querySelectorAll('[data-artifact-trigger]')).toHaveLength(1); + }); + + it('keeps the panel on the newest version even when the older card mounts after the newer one', () => { + const newer = baseAttachment({ + file_id: 'versioned', + filename: 'report.html', + text: '

v2 (newer)

', + updatedAt: '2024-01-02T00:00:00.000Z', + }); + const older = baseAttachment({ + file_id: 'versioned', + filename: 'report.html', + text: '

v1 (older)

', + updatedAt: '2024-01-01T00:00:00.000Z', + }); + let content: string | null = null; + render( + + { + content = snapshot; + }} + /> + + + + + + + , + ); + expect(content).toBe('

v2 (newer)

'); + }); + + it('keeps the newer content after the newer card unmounts and the older card mounts fresh', () => { + const newer = baseAttachment({ + file_id: 'remount', + filename: 'notes.html', + text: '

v2 (newer)

', + updatedAt: '2024-01-02T00:00:00.000Z', + }); + const older = baseAttachment({ + file_id: 'remount', + filename: 'notes.html', + text: '

v1 (older)

', + updatedAt: '2024-01-01T00:00:00.000Z', + }); + let content: string | null = null; + const onSnapshot = (snapshot: string | null) => { + content = snapshot; + }; + const { rerender } = render( + + + + + + , + ); + expect(content).toBe('

v2 (newer)

'); + + rerender( + + + + + + , + ); + expect(content).toBe('

v2 (newer)

'); + }); +}); + +describe('ToolArtifactCard file identity for id-less attachments', () => { + it('renders two card triggers for two id-less files sharing a filename but differing by filepath, each opening its own content', () => { + const first = baseAttachment({ + file_id: undefined, + filename: 'index.html', + filepath: '/uploads/session-a/index.html', + text: '

A

', + }); + const second = baseAttachment({ + file_id: undefined, + filename: 'index.html', + filepath: '/uploads/session-b/index.html', + text: '

B

', + }); + + let snapshot: Record = {}; + const ArtifactsSnapshotProbe = () => { + const artifacts = useRecoilValue(store.artifactsState); + React.useEffect(() => { + snapshot = artifacts ?? {}; + }); + return null; + }; + let currentId: string | null = null; + const CurrentArtifactProbe = () => { + const id = useRecoilValue(store.currentArtifactId); + React.useEffect(() => { + currentId = id; + }); + return null; + }; + + const { container } = render( + + + + + + + , + ); + + const triggers = container.querySelectorAll('[data-artifact-trigger]'); + expect(triggers).toHaveLength(2); + + const firstId = 'tool-artifact-/uploads/session-a/index.html'; + const secondId = 'tool-artifact-/uploads/session-b/index.html'; + const triggerIds = Array.from(triggers).map((el) => el.getAttribute('data-artifact-trigger')); + expect([...triggerIds].sort()).toEqual([firstId, secondId].sort()); + + // Both files' own content registers, keyed by their own identity — + // a colliding key would have one file's mount overwrite the other's. + expect(snapshot[firstId]?.content).toBe('

A

'); + expect(snapshot[secondId]?.content).toBe('

B

'); + + const firstTrigger = container.querySelector(`[data-artifact-trigger="${firstId}"]`); + const secondTrigger = container.querySelector(`[data-artifact-trigger="${secondId}"]`); + expect(firstTrigger).not.toBeNull(); + expect(secondTrigger).not.toBeNull(); + + fireEvent.click(firstTrigger as HTMLElement); + expect(currentId).toBe(firstId); + + fireEvent.click(secondTrigger as HTMLElement); + expect(currentId).toBe(secondId); + }); +}); + +describe('ToolMermaidArtifact message-scoped dedup', () => { + it('shows a diagram card on every message that holds the same file identity', () => { + const mmd = () => + baseAttachment({ file_id: 'diagram', filename: 'flow.mmd', text: 'graph TD\nA-->B' }); + render( + + + + + + + + , + ); + expect(screen.getAllByTestId('mermaid-render')).toHaveLength(2); + }); + + it('collapses two diagram cards for the same file within one message to one', () => { + const mmd = baseAttachment({ + file_id: 'diagram-dup', + filename: 'flow.mmd', + text: 'graph TD\nA-->B', + }); + render( + + + + + + , + ); + expect(screen.getAllByTestId('mermaid-render')).toHaveLength(1); + }); +}); + +describe('ToolMermaidArtifact newest-version selection', () => { + const mermaidCards = () => screen.getAllByTestId('mermaid-render'); + + const ArtifactKeysProbe = ({ onSnapshot }: { onSnapshot: (keys: string[]) => void }) => { + const artifacts = useRecoilValue(store.artifactsState); + React.useEffect(() => { + onSnapshot(Object.keys(artifacts ?? {})); + }); + return null; + }; + + it('offers the older message card the newer content when the newer message mounts after it', () => { + const older = baseAttachment({ + file_id: 'diagram-mount-order-a', + filename: 'flow.mmd', + text: 'graph TD\nA-->B', + updatedAt: '2024-01-01T00:00:00.000Z', + }); + const newer = baseAttachment({ + file_id: 'diagram-mount-order-a', + filename: 'flow.mmd', + text: 'graph TD\nA-->C', + updatedAt: '2024-01-02T00:00:00.000Z', + }); + let artifactKeys: string[] = []; + render( + + (artifactKeys = keys)} /> + + + + + + + , + ); + const cards = mermaidCards(); + expect(cards).toHaveLength(2); + // Opening EITHER card — including the older message's — would register + // the newest version. + cards.forEach((card) => { + expect(card).toHaveAttribute('data-artifact-content', 'graph TD\nA-->C'); + }); + // Each inline diagram still renders its own message's source. + expect(cards[0].textContent).toBe('graph TD\nA-->B'); + expect(cards[1].textContent).toBe('graph TD\nA-->C'); + // Mounting alone never writes into artifactsState (navigator unaffected). + expect(artifactKeys).toHaveLength(0); + }); + + it('keeps the newest content when the newer message mounts before the older one', () => { + const newer = baseAttachment({ + file_id: 'diagram-mount-order-b', + filename: 'flow.mmd', + text: 'graph TD\nA-->C', + updatedAt: '2024-01-02T00:00:00.000Z', + }); + const older = baseAttachment({ + file_id: 'diagram-mount-order-b', + filename: 'flow.mmd', + text: 'graph TD\nA-->B', + updatedAt: '2024-01-01T00:00:00.000Z', + }); + render( + + + + + + + + , + ); + const cards = mermaidCards(); + expect(cards).toHaveLength(2); + cards.forEach((card) => { + expect(card).toHaveAttribute('data-artifact-content', 'graph TD\nA-->C'); + }); + expect(cards[0].textContent).toBe('graph TD\nA-->C'); + expect(cards[1].textContent).toBe('graph TD\nA-->B'); + }); + + it('keeps the newer content after the newer card unmounts and the older card mounts fresh', () => { + const fileId = 'diagram-remount'; + const newer = baseAttachment({ + file_id: fileId, + filename: 'flow.mmd', + text: 'graph TD\nA-->C', + updatedAt: '2024-01-02T00:00:00.000Z', + }); + const older = baseAttachment({ + file_id: fileId, + filename: 'flow.mmd', + text: 'graph TD\nA-->B', + updatedAt: '2024-01-01T00:00:00.000Z', + }); + const { unmount } = render( + + + + + , + ); + expect(screen.getByTestId('mermaid-render')).toHaveAttribute( + 'data-artifact-content', + 'graph TD\nA-->C', + ); + unmount(); + + render( + + + + + , + ); + const reopened = screen.getByTestId('mermaid-render'); + expect(reopened).toHaveAttribute('data-artifact-content', 'graph TD\nA-->C'); + expect(reopened.textContent).toBe('graph TD\nA-->B'); + }); + + it('settles on one version when two diagrams for the same file share lastUpdateTime (no ping-pong)', () => { + const fileId = 'diagram-tie'; + const versionA = () => + baseAttachment({ file_id: fileId, filename: 'tie.mmd', text: 'graph TD\nA-->B' }); + const versionB = () => + baseAttachment({ file_id: fileId, filename: 'tie.mmd', text: 'graph TD\nA-->C' }); + + const renderPair = () => + render( + + + + + + + + , + ); + + const { unmount } = renderPair(); + const firstCards = mermaidCards(); + const settled = firstCards[0].getAttribute('data-artifact-content'); + expect(settled).not.toBeNull(); + expect(['graph TD\nA-->B', 'graph TD\nA-->C']).toContain(settled); + expect(firstCards[1]).toHaveAttribute('data-artifact-content', settled as string); + unmount(); + + renderPair(); + const secondCards = mermaidCards(); + expect(secondCards[0]).toHaveAttribute('data-artifact-content', settled as string); + expect(secondCards[1]).toHaveAttribute('data-artifact-content', settled as string); + }); +}); + +describe('ToolArtifactCard tied writes settle once and do not ping-pong', () => { + const tieId = 'tool-artifact-tie-file'; + const versionA = () => + baseAttachment({ file_id: 'tie-file', filename: 'tie.html', text: '

version A

' }); + const versionB = () => + baseAttachment({ file_id: 'tie-file', filename: 'tie.html', text: '

version B

' }); + + /** Records each distinct object identity `artifactsState[tieId]` takes on, + * i.e. one entry per real write — not one per render. */ + const WriteHistoryProbe = ({ onWrite }: { onWrite: (content: string | null) => void }) => { + const artifacts = useRecoilValue(store.artifactsState); + const entry = artifacts?.[tieId]; + const lastSeen = React.useRef(undefined); + React.useEffect(() => { + if (entry !== lastSeen.current) { + lastSeen.current = entry; + onWrite(entry?.content ?? null); + } + }); + return null; + }; + + it('settles on one version when the second card mounts after the first (sequential)', () => { + const cardA = versionA(); + const cardB = versionB(); + const writes: (string | null)[] = []; + const onWrite = (content: string | null) => writes.push(content); + + const { rerender } = render( + + + + + + , + ); + + rerender( + + + + + + + + + , + ); + + expect(writes.length).toBeLessThanOrEqual(2); + const settled = writes[writes.length - 1]; + expect(['

version A

', '

version B

']).toContain(settled); + + // Flush again with an unchanged tree — must not drift or write again. + rerender( + + + + + + + + + , + ); + expect(writes.length).toBeLessThanOrEqual(2); + expect(writes[writes.length - 1]).toBe(settled); + }); + + it('settles on one version when both cards mount together', () => { + const cardA = versionA(); + const cardB = versionB(); + const writes: (string | null)[] = []; + const onWrite = (content: string | null) => writes.push(content); + + const { rerender } = render( + + + + + + + + + , + ); + + expect(writes.length).toBeLessThanOrEqual(2); + const settled = writes[writes.length - 1]; + expect(['

version A

', '

version B

']).toContain(settled); + + // Flush again with an unchanged tree — must not drift or write again. + rerender( + + + + + + + + + , + ); + expect(writes.length).toBeLessThanOrEqual(2); + expect(writes[writes.length - 1]).toBe(settled); + }); +}); + +describe('SearchContent places cards per message', () => { + const searchMessage = (overrides: Partial = {}): TMessage => + ({ messageId: 'm', text: '', ...overrides }) as TMessage; + + const toolCallPart = (toolCallId: string): TMessageContentParts => + ({ + type: ContentTypes.TOOL_CALL, + [ContentTypes.TOOL_CALL]: { id: toolCallId, name: Tools.execute_code, args: '{}' }, + }) as unknown as TMessageContentParts; + + it('shows a card under each search-result message that holds the same file identity', () => { + const fileAttachment = (toolCallId: string) => + baseAttachment({ + file_id: 'search-shared-file', + filename: 'result.html', + text: '

result

', + toolCallId, + } as Partial); + + const { container } = render( + + + + , + ); + + expect(container.querySelectorAll('[data-artifact-trigger]')).toHaveLength(2); + expect(screen.getAllByText('result.html')).toHaveLength(2); + }); + + it('collapses two cards for the same file within one search-result message to one card', () => { + const dup = baseAttachment({ + file_id: 'search-dup-file', + filename: 'dup.html', + text: '

dup

', + toolCallId: 'call-dup', + } as Partial); + + const { container } = render( + + + , + ); + + expect(container.querySelectorAll('[data-artifact-trigger]')).toHaveLength(1); + }); +}); diff --git a/client/src/components/Chat/Messages/Content/Parts/claim.ts b/client/src/components/Chat/Messages/Content/Parts/claim.ts new file mode 100644 index 00000000000..e8c7a788eb7 --- /dev/null +++ b/client/src/components/Chat/Messages/Content/Parts/claim.ts @@ -0,0 +1,79 @@ +import { useId, useLayoutEffect } from 'react'; +import { atom } from 'jotai'; +import { useRecoilState } from 'recoil'; +import { atomFamily } from 'jotai/utils'; +import type { Artifact } from '~/common'; +import { useMessageContext } from '~/Providers'; +import store from '~/store'; + +interface ToolArtifactClaim { + /** False only while another mounted instance holds this display key. */ + isMyClaim: boolean; + /** + * This instance's stable claim key. `ToolArtifactCard` reuses it to also + * claim the id-only (message-independent) `toolArtifactClaim(id)` atom for + * its registration tie-break, so that claim and this hook's display claim + * never fight over the SAME atom entry in the no-message fallback, where + * both keys collapse to the bare `id`. + */ + claimKey: string; +} + +/** + * Scopes a tool artifact's chat-row dedup to the message that mounts it, so + * the same file shows one card per message but collapses repeat + * mounts within a single message to one. Falls back to the bare + * file id when no message is known — only the search-results route + * (`routes/Search.tsx` → `SearchMessage`) renders without an ambient + * `MessageContext`; `Share/Message.tsx` already provides one with + * `messageId` set. That fallback keeps today's cross-message dedup for + * the search route — `ArtifactRouting.test.tsx`'s "latest mount wins" and + * "does not ping-pong" cases render this way and must stay green. + * + * Shared by `ToolArtifactCard` and `ToolMermaidArtifact` so the same file + * dedups identically whether it renders as a panel card or an inline + * diagram. + */ +export default function useToolArtifactClaim(id: string): ToolArtifactClaim { + const claimKey = useId(); + const { messageId } = useMessageContext(); + const displayKey = messageId ? `${messageId}::${id}` : id; + const [claim, setClaim] = useRecoilState(store.toolArtifactClaim(displayKey)); + + useLayoutEffect(() => { + // Always (re)claim on mount — a later card for the same key displaces + // an earlier one, so the chip migrates to the most recent mention. + setClaim(claimKey); + return () => { + // Only release when the claim is still ours; if a sibling already + // took over we don't want to clobber its claim. + setClaim((prev) => (prev === claimKey ? null : prev)); + }; + }, [claimKey, setClaim]); + + return { isMyClaim: claim == null || claim === claimKey, claimKey }; +} + +/** + * Per-file-identity "newest version seen" for tool-diagram artifacts. + * Every mounted `ToolMermaidArtifact` offers its own artifact here on + * mount, keeping whichever entry has the larger `lastUpdateTime` (ties + * keep the current entry, so equal-time offers don't drift). Entries + * persist after a card unmounts, so a message whose diagram was + * superseded stays discoverable for any other message sharing the same + * file identity even after the newer card leaves the DOM. + */ +export const newestToolArtifactFamily = atomFamily((_id: string) => atom(null)); + +/** + * Shared by `ToolArtifactCard`'s self-heal registration and + * `ToolMermaidArtifact`/`Mermaid`'s newest-version handling so both use + * the same "is `candidate` strictly newer than `other`" rule instead of + * two copies that could drift out of sync. + */ +export function isStrictlyNewer( + candidate: Pick, + other: Pick | null | undefined, +): boolean { + return other != null && candidate.lastUpdateTime > other.lastUpdateTime; +} diff --git a/client/src/components/Chat/Messages/Content/SearchContent.tsx b/client/src/components/Chat/Messages/Content/SearchContent.tsx index 82a1a242e84..c18f0de8bde 100644 --- a/client/src/components/Chat/Messages/Content/SearchContent.tsx +++ b/client/src/components/Chat/Messages/Content/SearchContent.tsx @@ -70,7 +70,6 @@ const SearchContent = ({ } const partElement: ReactElement = ( ); - /** An error part resolves the agent a handoff made active from its own position, - * which it reads from `MessageContext`; persisted content is compacted, so `idx` is + /** Every part gets the row's message id so file-card dedup + * (`useToolArtifactClaim`) scopes correctly regardless of + * caller: the search-results route renders `SearchContent` + * with no ambient `MessageContext`, while `Share/Message.tsx` + * already provides one with `messageId` set — this explicit + * per-part provider keeps both paths consistent instead of + * relying on whichever context happens to be ambient. + * An error part additionally resolves the agent a handoff made + * active from its own position, which it reads from + * `MessageContext`; persisted content is compacted, so `idx` is * that position in `message.content`. */ - const rendered: ReactElement = - part.type === ContentTypes.ERROR ? ( - - {partElement} - - ) : ( - partElement - ); + const rendered: ReactElement = ( + + {partElement} + + ); if (!resumesAfterSteer) { return rendered; } diff --git a/client/src/components/Messages/Content/Mermaid/Mermaid.tsx b/client/src/components/Messages/Content/Mermaid/Mermaid.tsx index 3e98d5e8fd7..e9ae8e6057d 100644 --- a/client/src/components/Messages/Content/Mermaid/Mermaid.tsx +++ b/client/src/components/Messages/Content/Mermaid/Mermaid.tsx @@ -4,6 +4,7 @@ import { useLocation } from 'react-router-dom'; import { Button, Spinner } from '@librechat/client'; import { useRecoilValue, useSetRecoilState } from 'recoil'; import type { ProcessedMermaidSvg } from '~/utils/diagram/export'; +import { isStrictlyNewer } from '~/components/Chat/Messages/Content/Parts/claim'; import ArtifactRow from '~/components/Chat/Messages/Content/Parts/ArtifactRow'; import { MERMAID_ARTIFACT_TYPE, type Artifact } from '~/common/artifacts'; import { artifactRowKind } from '~/utils/artifacts'; @@ -81,11 +82,15 @@ const Mermaid: React.FC = memo((props) => { const artifactId = `mermaid-artifact-${artifactScope}-${id || instanceId}`; const artifact = useMemo(() => { if (artifactProp != null) { + /* Unlike the no-`artifactProp` branch below, `content` is NOT forced + * to `children` here: `ToolMermaidArtifact` may hand this a newer + * sibling's content (via `newestToolArtifactFamily`) so opening THIS + * card registers that newer version, while `children` below (the + * inline diagram) keeps rendering this message's own source. */ return { ...artifactProp, type: artifactProp.type ?? MERMAID_ARTIFACT_TYPE, title: artifactProp.title ?? defaultTitle, - content: children, messageId: artifactProp.messageId ?? messageId, }; } @@ -114,6 +119,13 @@ const Mermaid: React.FC = memo((props) => { ) { return previousArtifacts; } + // Never downgrade an already-registered version that's strictly + // newer than what this instance is about to write — protects + // against a stale self-heal re-fire racing a sibling card's newer + // write for the same file identity. + if (existingArtifact != null && isStrictlyNewer(existingArtifact, artifact)) { + return previousArtifacts; + } return { ...(previousArtifacts ?? {}), [artifact.id]: artifact }; }); diff --git a/client/src/utils/__tests__/artifacts.test.ts b/client/src/utils/__tests__/artifacts.test.ts index 11d2fedb6de..f221569e7c6 100644 --- a/client/src/utils/__tests__/artifacts.test.ts +++ b/client/src/utils/__tests__/artifacts.test.ts @@ -14,6 +14,7 @@ import { isPreviewOnlyArtifact, isSvgArtifactType, languageForFilename, + toolArtifactKey, TOOL_ARTIFACT_TYPES, } from '../artifacts'; @@ -654,6 +655,35 @@ describe('languageForFilename', () => { }); }); +describe('toolArtifactKey', () => { + it('prefers file_id over filepath and filename', () => { + expect( + toolArtifactKey({ + file_id: 'fid-1', + filepath: '/uploads/session-a/index.html', + filename: 'index.html', + }), + ).toBe('tool-artifact-fid-1'); + }); + + it('falls back to filepath before filename when file_id is missing', () => { + /* Id-less attachments are download fallbacks with a unique per-session + * filepath; keying by filename would merge genuinely different files + * that happen to share a display name. */ + expect( + toolArtifactKey({ filepath: '/uploads/session-a/index.html', filename: 'index.html' }), + ).toBe('tool-artifact-/uploads/session-a/index.html'); + }); + + it('falls back to filename when neither file_id nor filepath is present', () => { + expect(toolArtifactKey({ filename: 'index.html' })).toBe('tool-artifact-index.html'); + }); + + it("falls back to 'unknown' when nothing identifies the file", () => { + expect(toolArtifactKey({})).toBe('tool-artifact-unknown'); + }); +}); + describe('fileToArtifact', () => { const baseFile = { file_id: 'fid-1', diff --git a/client/src/utils/__tests__/map.test.ts b/client/src/utils/__tests__/map.test.ts index 6dc54dc42b1..25fdc1b4d4d 100644 --- a/client/src/utils/__tests__/map.test.ts +++ b/client/src/utils/__tests__/map.test.ts @@ -62,7 +62,110 @@ describe('filterAttachmentsForPart', () => { describe('mapAttachments', () => { it('groups by toolCallId and drops unkeyed entries', () => { - const map = mapAttachments([att({}), att({ toolCallId: 'call_1' }), att({ toolCallId: '' })]); + const map = mapAttachments([ + att({ file_id: 'f1' }), + att({ toolCallId: 'call_1', file_id: 'f2' }), + att({ toolCallId: '', file_id: 'f3' }), + ]); expect(Object.keys(map).sort()).toEqual(['call_0', 'call_1']); }); + + it('keeps a repeated file_id only under its later toolCallId', () => { + const first = att({ toolCallId: 'call_0', file_id: 'f1' }); + const second = att({ toolCallId: 'call_1', file_id: 'f1' }); + const map = mapAttachments([first, second]); + expect(map['call_0']).toBeUndefined(); + expect(map['call_1']).toEqual([second]); + }); + + it('drops an earlier duplicate of the same file within one toolCallId', () => { + const first = att({ toolCallId: 'call_0', file_id: 'f1' }); + const second = att({ toolCallId: 'call_0', file_id: 'f1' }); + const map = mapAttachments([first, second]); + expect(map['call_0']).toEqual([second]); + }); + + it('keeps two attachments with the same filename but different file_ids', () => { + const first = att({ toolCallId: 'call_0', file_id: 'f1', filename: 'data.zip' }); + const second = att({ toolCallId: 'call_0', file_id: 'f2', filename: 'data.zip' }); + const map = mapAttachments([first, second]); + expect(map['call_0']).toEqual([first, second]); + }); + + it('keeps every non-file attachment even when they share a toolCallId', () => { + const first = att({ toolCallId: 'call_0', file_id: undefined }); + const second = att({ toolCallId: 'call_0', file_id: undefined }); + const map = mapAttachments([first, second]); + expect(map['call_0']).toEqual([first, second]); + }); + + it('skips null and undefined entries', () => { + const map = mapAttachments([null, att({ toolCallId: 'call_0' }), undefined]); + expect(map['call_0']).toHaveLength(1); + }); + + it('keeps the newer write when copies are already in chronological order', () => { + const older = att({ toolCallId: 'call_0', file_id: 'f1', updatedAt: '2024-01-01T00:00:00Z' }); + const newer = att({ toolCallId: 'call_1', file_id: 'f1', updatedAt: '2024-01-02T00:00:00Z' }); + const map = mapAttachments([older, newer]); + expect(map['call_0']).toBeUndefined(); + expect(map['call_1']).toEqual([newer]); + }); + + it('keeps the newer write when an older duplicate sits at a higher array index', () => { + const newer = att({ toolCallId: 'call_1', file_id: 'f1', updatedAt: '2024-01-02T00:00:00Z' }); + const older = att({ toolCallId: 'call_0', file_id: 'f1', updatedAt: '2024-01-01T00:00:00Z' }); + const map = mapAttachments([newer, older]); + expect(map['call_0']).toBeUndefined(); + expect(map['call_1']).toEqual([newer]); + }); + + it('breaks a tie in write time by keeping the higher array index', () => { + const first = att({ toolCallId: 'call_0', file_id: 'f1', updatedAt: '2024-01-01T00:00:00Z' }); + const second = att({ toolCallId: 'call_1', file_id: 'f1', updatedAt: '2024-01-01T00:00:00Z' }); + const map = mapAttachments([first, second]); + expect(map['call_0']).toBeUndefined(); + expect(map['call_1']).toEqual([second]); + }); + + it('keeps the linked copy when an unlinked duplicate of the same file follows it', () => { + const linked = att({ toolCallId: 'call_0', file_id: 'f1' }); + const unlinked = att({ toolCallId: '', file_id: 'f1' }); + const map = mapAttachments([linked, unlinked]); + expect(map['call_0']).toEqual([linked]); + }); + + it('keeps two id-less attachments with the same filename but different filepaths', () => { + const first = att({ + toolCallId: 'call_0', + file_id: undefined, + filename: 'data.zip', + filepath: '/uploads/session-a/data.zip', + }); + const second = att({ + toolCallId: 'call_1', + file_id: undefined, + filename: 'data.zip', + filepath: '/uploads/session-b/data.zip', + }); + const map = mapAttachments([first, second]); + expect(map['call_0']).toEqual([first]); + expect(map['call_1']).toEqual([second]); + }); + + it('drops an earlier id-less duplicate at the same filepath', () => { + const first = att({ + toolCallId: 'call_0', + file_id: undefined, + filepath: '/uploads/session-a/data.zip', + }); + const second = att({ + toolCallId: 'call_1', + file_id: undefined, + filepath: '/uploads/session-a/data.zip', + }); + const map = mapAttachments([first, second]); + expect(map['call_0']).toBeUndefined(); + expect(map['call_1']).toEqual([second]); + }); }); diff --git a/client/src/utils/artifacts.ts b/client/src/utils/artifacts.ts index e192e93061f..cc92254fcaa 100644 --- a/client/src/utils/artifacts.ts +++ b/client/src/utils/artifacts.ts @@ -1106,12 +1106,14 @@ export function detectArtifactTypeFromFile( * Stable per-file key used for both the artifactsState entry and the * `toolArtifactClaim` atom that dedups duplicate cards. Same call shape * everywhere so a panel card and a mermaid card for the same file share - * the same claim. Falls through `file_id` → `filename` → `filepath` to - * minimise collision risk for any caller that (rarely) lacks `file_id`. + * the same claim. Falls through `file_id` → `filepath` → `filename`: + * id-less attachments are download fallbacks with a unique per-session + * filepath, so keying them by display name would merge genuinely + * different files that happen to share a filename. */ export const toolArtifactKey = ( file: Partial>, -): string => `tool-artifact-${file.file_id ?? file.filename ?? file.filepath ?? 'unknown'}`; +): string => `tool-artifact-${file.file_id ?? file.filepath ?? file.filename ?? 'unknown'}`; /** * Stable epoch fallback (instead of `Date.now()`) when neither timestamp diff --git a/client/src/utils/map.ts b/client/src/utils/map.ts index 9f43d5a5023..748d2de6e6b 100644 --- a/client/src/utils/map.ts +++ b/client/src/utils/map.ts @@ -1,17 +1,84 @@ import type * as t from 'librechat-data-provider'; import type { TPluginMap } from '~/common'; +import { toolArtifactKey } from './artifacts'; -/** Maps Attachments by `toolCallId` for quick lookup */ +/** + * Identity for a file-backed attachment (one with a `file_id` or a + * `filepath`), or `null` for anything else (e.g. web search results), which + * is never collapsed. For file-backed attachments this is exactly the + * artifact card's key, `toolArtifactKey` (`file_id` → `filepath` → + * `filename`), so an id-less file keys by its unique per-session filepath + * rather than a display name two different files can share. An attachment + * with only a `filename` is not treated as a file here, although + * `toolArtifactKey` would still key it by that name. + */ +export const fileIdentity = (attachment: t.TAttachment): string | null => { + const file = attachment as Partial; + if (file.file_id != null || file.filepath != null) { + return toolArtifactKey(file); + } + return null; +}; + +/** `updatedAt ?? createdAt`, parsed to ms; missing or unparseable → 0. */ +const writeTimeMs = (attachment: t.TAttachment): number => { + const file = attachment as Partial; + const value = file.updatedAt ?? file.createdAt; + if (value == null) { + return 0; + } + const ms = new Date(value as string | number | Date).getTime(); + return Number.isFinite(ms) ? ms : 0; +}; + +/** + * Maps Attachments by `toolCallId` for quick lookup. Attachments are assumed + * to belong to one message: when the same file repeats — e.g. a tool call + * rewrites the file it produced earlier in the message — only the copy with + * the newest write time survives (ties keep the higher array index), so a + * message never shows the same file twice. Array order isn't chronological + * (a background-run harvest can append an older copy after a newer + * foreground rewrite), so only entries that will actually be grouped + * (non-empty `toolCallId`) compete for survivorship; an unlinked duplicate + * is dropped as always but never hides a linked copy. Non-file attachments + * (no `file_id`/`filepath`) never collapse. + */ export function mapAttachments(attachments: Array) { const attachmentMap: Record = {}; - for (const attachment of attachments) { + const identities = attachments.map((attachment) => + attachment == null ? null : fileIdentity(attachment), + ); + const survivorByIdentity = new Map(); + attachments.forEach((attachment, index) => { + if (attachment == null) { + return; + } + const identity = identities[index]; + if (identity == null || !attachment.toolCallId) { + return; + } + const time = writeTimeMs(attachment); + const current = survivorByIdentity.get(identity); + if (!current || time > current.time || (time === current.time && index > current.index)) { + survivorByIdentity.set(identity, { index, time }); + } + }); + + attachments.forEach((attachment, index) => { if (attachment === null || attachment === undefined) { - continue; + return; + } + const identity = identities[index]; + if (identity != null) { + const survivor = survivorByIdentity.get(identity); + if (survivor && survivor.index !== index) { + return; + } } const key = attachment.toolCallId || ''; if (key.length === 0) { - continue; + return; } if (!attachmentMap[key]) { @@ -19,7 +86,7 @@ export function mapAttachments(attachments: Array { * on resize never measures an already-transformed box. */ expect(html).toContain('lcNativeW'); }); + + test('initializes pptx-preview with only a width so it never boxes the deck into a fixed-height viewport', async () => { + /* Passing `height` bounded the librarys own render box, which + * produced a nested scroll region for multi-slide decks instead + * of letting the panel itself scroll to the last slide. Width + * alone is enough — the wrap+scale step above fits each slide + * to the panel. */ + const pptx = await buildPptx([{ title: 'A' }]); + const html = await _internal.pptxToHtmlViaCdn( + pptx, + '
  1. fb
', + ); + expect(html).toContain('pptxPreview.init(container, { width: SLIDE_W })'); + expect(html).not.toContain('height: SLIDE_H'); + }); + + test('wraps each rendered slide directly instead of the containers immediate children', async () => { + /* pptx-preview nests every `.pptx-preview-slide-wrapper` inside + * one library-owned `.pptx-preview-wrapper` box, so + * `container.children` only ever finds that single box — + * wrapping it as one unit jammed every slide into one shared + * block. Querying `.pptx-preview-slide-wrapper` directly finds + * each slide wherever the library actually nested it, and each + * wrap is inserted next to its own slide via `slide.parentNode` + * rather than `container`. */ + const pptx = await buildPptx([{ title: 'A' }, { title: 'B' }]); + const html = await _internal.pptxToHtmlViaCdn( + pptx, + '
  1. fb
', + ); + expect(html).toContain("container.querySelectorAll('.pptx-preview-slide-wrapper')"); + expect(html).not.toContain('container.children'); + expect(html).toContain('slide.parentNode.insertBefore(wrap, slide)'); + }); + + test('overrides the librarys own wrapper box to hug the stacked slides width with a transparent background', async () => { + const pptx = await buildPptx([{ title: 'A' }]); + const html = await _internal.pptxToHtmlViaCdn( + pptx, + '
  1. fb
', + ); + expect(html).toMatch(/\.pptx-preview-wrapper\s*\{[^}]*width:\s*auto\s*!important/); + expect(html).toMatch( + /\.pptx-preview-wrapper\s*\{[^}]*background:\s*transparent\s*!important/, + ); + }); }); describe('OFFICE_PREVIEW_DISABLE_CDN escape hatch', () => { diff --git a/packages/api/src/files/documents/html.ts b/packages/api/src/files/documents/html.ts index 2ee0ff163b9..4c1611c52bc 100644 --- a/packages/api/src/files/documents/html.ts +++ b/packages/api/src/files/documents/html.ts @@ -1124,6 +1124,12 @@ function buildPptxCdnDocument(base64: string, slideListFallbackBody: string): st