From bc149637d1a75dabb08007cb365197884aebfd79 Mon Sep 17 00:00:00 2001 From: TomasPalsson Date: Mon, 28 Sep 2026 16:03:14 +0000 Subject: [PATCH 01/15] fix: list each duplicate file identity once in the folded file group --- .../Messages/Content/Parts/Attachment.tsx | 21 ++++- .../__tests__/FileAttachmentGroup.test.tsx | 94 +++++++++++++++++++ 2 files changed, 110 insertions(+), 5 deletions(-) create mode 100644 client/src/components/Chat/Messages/Content/Parts/__tests__/FileAttachmentGroup.test.tsx diff --git a/client/src/components/Chat/Messages/Content/Parts/Attachment.tsx b/client/src/components/Chat/Messages/Content/Parts/Attachment.tsx index c7c51b584b1..d92d8f9276f 100644 --- a/client/src/components/Chat/Messages/Content/Parts/Attachment.tsx +++ b/client/src/components/Chat/Messages/Content/Parts/Attachment.tsx @@ -14,9 +14,9 @@ import { isTextAttachment, renderAttachmentKey, } from './attachmentTypes'; +import { fileToArtifact, toolArtifactKey, TOOL_ARTIFACT_TYPES } from '~/utils/artifacts'; import { useLocalize, useAttachmentPreviewSync, useExpandCollapse } from '~/hooks'; import FileContainer from '~/components/Chat/Input/Files/FileContainer'; -import { fileToArtifact, TOOL_ARTIFACT_TYPES } from '~/utils/artifacts'; import Image from '~/components/Chat/Messages/Content/Image'; import { ROW_GLYPH_SLOT, TOOL_ROW_CLASSES } from '../rows'; import ToolMermaidArtifact from './ToolMermaidArtifact'; @@ -179,10 +179,21 @@ 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(); + for (const attachment of attachments) { + if (!attachment.filepath) { + continue; + } + const key = toolArtifactKey(attachment as Partial); + 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/__tests__/FileAttachmentGroup.test.tsx b/client/src/components/Chat/Messages/Content/Parts/__tests__/FileAttachmentGroup.test.tsx new file mode 100644 index 00000000000..00a3a3ecf04 --- /dev/null +++ b/client/src/components/Chat/Messages/Content/Parts/__tests__/FileAttachmentGroup.test.tsx @@ -0,0 +1,94 @@ +import React from 'react'; +import { RecoilRoot } from 'recoil'; +import { render, screen, fireEvent } from '@testing-library/react'; +import type { TAttachment } from 'librechat-data-provider'; +import { AttachmentGroup } from '../Attachment'; + +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 }: { 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 (B12)', () => { + 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'); + }); +}); From 79f94a7bc29e57be2c40aaa580548ea65efd5a5f Mon Sep 17 00:00:00 2001 From: TomasPalsson Date: Mon, 28 Sep 2026 16:15:37 +0000 Subject: [PATCH 02/15] fix: scope file card dedup to its message and always show the newest version Two cards for the same file across different messages now both render instead of one message winning the only visible chip. Registration into the artifact panel now compares update timestamps (falling back to a mount-order tie-break) so an older card mounting after a newer one, or remounting after the newer card unmounts, can never clobber the newer content. Search and shared conversation views now scope every rendered part to its own message so the same fix applies there. --- .../Content/Parts/ToolArtifactCard.tsx | 93 ++++--- .../Content/Parts/ToolMermaidArtifact.tsx | 23 +- .../Parts/__tests__/ToolArtifactCard.test.tsx | 235 ++++++++++++++++++ .../Chat/Messages/Content/Parts/claim.ts | 50 ++++ .../Chat/Messages/Content/SearchContent.tsx | 32 +-- 5 files changed, 364 insertions(+), 69 deletions(-) create mode 100644 client/src/components/Chat/Messages/Content/Parts/__tests__/ToolArtifactCard.test.tsx create mode 100644 client/src/components/Chat/Messages/Content/Parts/claim.ts diff --git a/client/src/components/Chat/Messages/Content/Parts/ToolArtifactCard.tsx b/client/src/components/Chat/Messages/Content/Parts/ToolArtifactCard.tsx index de6940ddda3..e8fc7fe13a8 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, @@ -11,6 +11,7 @@ import type { Artifact } from '~/common'; import { artifactRowKind, isCodeOnlyArtifact } from '~/utils/artifacts'; import { displayFilename } from './attachmentTypes'; import { useAttachmentLink } from './LogLink'; +import useToolArtifactClaim from './claim'; import ArtifactRow from './ArtifactRow'; import store from '~/store'; @@ -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 || + artifact.lastUpdateTime > existingEntry.lastUpdateTime || + (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..68055318912 100644 --- a/client/src/components/Chat/Messages/Content/Parts/ToolMermaidArtifact.tsx +++ b/client/src/components/Chat/Messages/Content/Parts/ToolMermaidArtifact.tsx @@ -1,14 +1,13 @@ -import { memo, useId, useLayoutEffect, useMemo, useState } from 'react'; +import { memo, useMemo, useState } from 'react'; import { Download } from 'lucide-react'; -import { useRecoilState } from 'recoil'; import type { TAttachment, TFile, TAttachmentMetadata } from 'librechat-data-provider'; 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 useToolArtifactClaim from './claim'; import { useLocalize } from '~/hooks'; import { cn } from '~/utils'; -import store from '~/store'; interface ToolMermaidArtifactProps { attachment: TAttachment; @@ -20,27 +19,19 @@ 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). */ 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 { isMyClaim } = useToolArtifactClaim(toolArtifactKey(file)); /* 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 ?? '', @@ -54,7 +45,7 @@ const ToolMermaidArtifact = memo(({ attachment, text }: ToolMermaidArtifactProps [attachment, text], ); - if (claim != null && !isMyClaim) { + if (!isMyClaim) { return null; } 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..a0f19252f80 --- /dev/null +++ b/client/src/components/Chat/Messages/Content/Parts/__tests__/ToolArtifactCard.test.tsx @@ -0,0 +1,235 @@ +import React from 'react'; +import { RecoilRoot, useRecoilValue } from 'recoil'; +import { render, screen } from '@testing-library/react'; +import type { TAttachment } from 'librechat-data-provider'; +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 }; + }) => ( +
+ {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; + +/** 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 (FR-06)', () => { + 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 (FR-07)', () => { + 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 (FR-08)', () => { + 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 (FR-08)', () => { + 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('ToolMermaidArtifact message-scoped dedup (FR-09)', () => { + 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); + }); +}); 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..03590ddcb76 --- /dev/null +++ b/client/src/components/Chat/Messages/Content/Parts/claim.ts @@ -0,0 +1,50 @@ +import { useId, useLayoutEffect } from 'react'; +import { useRecoilState } from 'recoil'; +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 (FR-06) but collapses repeat + * mounts within a single message to one (FR-07). Falls back to the bare + * file id when no message is known (search/shared views without an + * ambient `MessageContext`), which keeps today's cross-message dedup for + * that caller — `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 }; +} diff --git a/client/src/components/Chat/Messages/Content/SearchContent.tsx b/client/src/components/Chat/Messages/Content/SearchContent.tsx index 82a1a242e84..76eec8bc201 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 here and in shared + * views, which mount with no ambient `MessageContext.Provider`. + * 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; } From fdb5e75393be4c6a880c20561efb0556c11af49a Mon Sep 17 00:00:00 2001 From: TomasPalsson Date: Mon, 28 Sep 2026 16:18:22 +0000 Subject: [PATCH 03/15] fix: stack each pptx slide as its own panel-width block --- packages/api/src/files/documents/html.spec.ts | 46 ++++++++ packages/api/src/files/documents/html.ts | 35 ++++-- .../api/src/files/documents/layout.spec.ts | 110 ++++++++++++++++++ 3 files changed, 183 insertions(+), 8 deletions(-) create mode 100644 packages/api/src/files/documents/layout.spec.ts diff --git a/packages/api/src/files/documents/html.spec.ts b/packages/api/src/files/documents/html.spec.ts index e320abddf9c..ef01ef96885 100644 --- a/packages/api/src/files/documents/html.spec.ts +++ b/packages/api/src/files/documents/html.spec.ts @@ -618,6 +618,52 @@ describe('Office HTML producers', () => { * 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..ad78a40c536 100644 --- a/packages/api/src/files/documents/html.ts +++ b/packages/api/src/files/documents/html.ts @@ -1148,6 +1148,15 @@ html, body { margin: 0; padding: 0; background: var(--bg); color: var(--fg); fon left: 0; transform-origin: top left; } +/* pptx-preview's own init() sets a fixed inline width (and an opaque + * background) on the wrapper box it creates around the slides. + * Override so it hugs the stacked .lc-slide-wrap blocks' actual width + * instead of clipping them, and drops its own background so it + * doesn't paint a second boxed panel behind the per-slide cards. */ +.pptx-preview-wrapper { + width: auto !important; + background: transparent !important; +} #lc-fallback { padding: 16px; font-size: 14px; line-height: 1.5; color: var(--fg); } #lc-fallback-notice { font-size: 12px; color: var(--muted); border-bottom: 1px solid var(--border); padding-bottom: 8px; margin: 0 0 16px; } .lc-pptx-loading { display: flex; align-items: center; justify-content: center; height: 60vh; color: var(--muted); font-size: 14px; } @@ -1244,7 +1253,12 @@ ${PPTX_SLIDE_LIST_CSS} * doesnt see the unscaled 960px-wide flash. We reveal once * wrap+scale has settled. */ container.style.visibility = 'hidden'; - var previewer = pptxPreview.init(container, { width: SLIDE_W, height: SLIDE_H }); + /* No height option: passing one bounds the librarys own render + * box to that pixel height, which nests a scrollable region inside + * the panel instead of letting the panel itself scroll to the last + * slide. Width alone is enough — wrapSlides() below fits each + * slide to the panel on its own. */ + var previewer = pptxPreview.init(container, { width: SLIDE_W }); function availableWidth() { /* clientWidth includes the 16px padding on each side via @@ -1279,12 +1293,17 @@ ${PPTX_SLIDE_LIST_CSS} * move slides out from under the librarys references and break * its internal state. */ function wrapSlides() { - var children = Array.prototype.slice.call(container.children); - for (var i = 0; i < children.length; i++) { - var slide = children[i]; - if (!slide.classList || slide.classList.contains('lc-slide-wrap') || slide.classList.contains('lc-pptx-loading')) { - continue; - } + /* pptx-preview nests every rendered slide inside its own + * .pptx-preview-wrapper box rather than appending them directly + * to the render container — walking the containers immediate + * children only ever finds that one box, wrapping the whole deck + * as a single unit instead of one block per slide. Querying the + * slides directly finds each one wherever the library nested it, + * and each wrap is inserted next to its own slide via + * slide.parentNode. */ + var slides = Array.prototype.slice.call(container.querySelectorAll('.pptx-preview-slide-wrapper')); + for (var i = 0; i < slides.length; i++) { + var slide = slides[i]; /* Cache the slides actual rendered size BEFORE applying any * transform — measurements after a CSS scale no longer reflect * native pixels and would feed back into wrong sizing on @@ -1302,7 +1321,7 @@ ${PPTX_SLIDE_LIST_CSS} wrap.style.height = (nativeH * scale) + 'px'; slide.style.transformOrigin = 'top left'; slide.style.transform = 'scale(' + scale + ')'; - container.insertBefore(wrap, slide); + slide.parentNode.insertBefore(wrap, slide); wrap.appendChild(slide); } } diff --git a/packages/api/src/files/documents/layout.spec.ts b/packages/api/src/files/documents/layout.spec.ts new file mode 100644 index 00000000000..3711ce4c231 --- /dev/null +++ b/packages/api/src/files/documents/layout.spec.ts @@ -0,0 +1,110 @@ +import { JSDOM } from 'jsdom'; +import { _internal } from './html'; + +/** + * Options `pptxPreview.init` is called with, captured by the fake below so + * tests can assert what the bootstrap script actually passed at runtime. + */ +interface FakePptxPreviewInitOptions { + width: number; + height?: number; +} + +/** + * Runs the actual `