diff --git a/desktop/src/components/chat/AuthedImage.test.tsx b/desktop/src/components/chat/AuthedImage.test.tsx index f2dc55a8..9066809e 100644 --- a/desktop/src/components/chat/AuthedImage.test.tsx +++ b/desktop/src/components/chat/AuthedImage.test.tsx @@ -5,6 +5,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' const fetchServerImageBlobUrl = vi.hoisted(() => vi.fn()) vi.mock('../../lib/authedImage', () => ({ fetchServerImageBlobUrl })) +import { ApiError } from '../../api/client' import { AuthedImage } from './AuthedImage' beforeEach(() => { @@ -42,6 +43,35 @@ describe('AuthedImage', () => { await waitFor(() => expect(onFailure).toHaveBeenCalledTimes(1)) }) + // A file the server will not serve is not there to show; anything else is a + // load that should have worked. + it.each([ + [404, 'unavailable'], + [403, 'unavailable'], + [400, 'unavailable'], + [413, 'unavailable'], + [401, 'failed'], + [500, 'failed'], + ] as const)('reports HTTP %i from the authenticated attempt as %s', async (status, failure) => { + fetchServerImageBlobUrl.mockRejectedValue(new ApiError(status, {})) + const onFailure = vi.fn() + render() + + fireEvent.error(screen.getByRole('img')) + + await waitFor(() => expect(onFailure).toHaveBeenCalledWith(failure)) + }) + + it('reports a dropped connection as a failure, not as a file that is not there', async () => { + fetchServerImageBlobUrl.mockRejectedValue(new TypeError('Failed to fetch')) + const onFailure = vi.fn() + render() + + fireEvent.error(screen.getByRole('img')) + + await waitFor(() => expect(onFailure).toHaveBeenCalledWith('failed')) + }) + it('does not apply a result that arrives after the image is gone, and frees it', async () => { let finish!: (url: string) => void fetchServerImageBlobUrl.mockReturnValue(new Promise((resolve) => { finish = resolve })) @@ -89,6 +119,8 @@ describe('AuthedImage', () => { fireEvent.error(screen.getByRole('img')) fireEvent.error(screen.getByRole('img')) expect(onFailure).toHaveBeenCalledTimes(1) + // The server sent the file and it did not decode: there, and broken. + expect(onFailure).toHaveBeenCalledWith('failed') }) it('discards a late copy after switching sources, even when returning to the original source', async () => { diff --git a/desktop/src/components/chat/AuthedImage.tsx b/desktop/src/components/chat/AuthedImage.tsx index 61175ba0..912144ea 100644 --- a/desktop/src/components/chat/AuthedImage.tsx +++ b/desktop/src/components/chat/AuthedImage.tsx @@ -1,9 +1,9 @@ import type { ImgHTMLAttributes } from 'react' -import { useAuthedImageFallback } from '@/lib/useAuthedImageFallback' +import { useAuthedImageFallback, type ImageFailure } from '@/lib/useAuthedImageFallback' type Props = Omit, 'onError'> & { - /** Runs once the image has failed even with the app's credential. */ - onFailure?: () => void + /** Runs once the image has failed even with the app's credential, saying why. */ + onFailure?: (failure: ImageFailure) => void /** A user retry skips the potentially cached bare failure and fetches afresh. */ retryWithCredential?: boolean } diff --git a/desktop/src/components/chat/InlineImageGallery.test.tsx b/desktop/src/components/chat/InlineImageGallery.test.tsx index 507db689..a878ff32 100644 --- a/desktop/src/components/chat/InlineImageGallery.test.tsx +++ b/desktop/src/components/chat/InlineImageGallery.test.tsx @@ -2,10 +2,12 @@ import '@testing-library/jest-dom' import { fireEvent, render, screen, waitFor } from '@testing-library/react' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { ApiError } from '../../api/client' import { browserHost } from '../../lib/desktopHost/browserHost' // getBaseUrl backs the absolute-path src (/api/filesystem/file). -vi.mock('../../api/client', () => ({ +vi.mock('../../api/client', async (original) => ({ + ...(await original>()), getBaseUrl: () => 'http://127.0.0.1:3456', })) @@ -14,8 +16,9 @@ vi.mock('../../lib/desktopRuntime', () => ({ getServerBaseUrl: () => 'http://127.0.0.1:4321', })) -// The authenticated fallback an error falls back to. It rejects by default, -// which is what a missing or denied file does, so the failure notice shows. +// The authenticated fallback an error falls back to. It rejects by default +// the way a server fault does, so the failure notice shows; a file the server will +// not serve is answered with its 4xx status instead. const fetchServerImageBlobUrl = vi.hoisted(() => vi.fn()) vi.mock('../../lib/authedImage', () => ({ fetchServerImageBlobUrl })) @@ -29,7 +32,7 @@ import { resetDiskListingCacheForTests } from '../../hooks/useDiskConfirmedTarge beforeEach(() => { resetDiskListingCacheForTests() getWorkspaceTree.mockReset().mockResolvedValue({ state: 'missing', path: '', entries: [] }) - fetchServerImageBlobUrl.mockReset().mockRejectedValue(new Error('403')) + fetchServerImageBlobUrl.mockReset().mockRejectedValue(new ApiError(500, { error: 'Internal error' })) // jsdom ships no object-URL support. Object.defineProperty(URL, 'revokeObjectURL', { value: vi.fn(), configurable: true, writable: true }) }) @@ -39,37 +42,97 @@ function imgSrcs(): string[] { } describe('InlineImageGallery', () => { - it('shows a failed image notice and filename instead of hiding the gallery entry', async () => { - render() + it('shows a retryable notice when an image the server would serve fails to load', async () => { + render() fireEvent.error(screen.getByRole('img')) const notice = await screen.findByRole('alert') expect(notice).toBeVisible() expect(notice).toHaveTextContent('Unable to load image') - expect(notice).toHaveTextContent('denied.png') - expect(notice).toHaveTextContent('The file may be missing or access may be denied.') + expect(notice).toHaveTextContent('chart.png') + expect(notice).toHaveTextContent('The file may be damaged, or the local server did not respond.') expect(screen.getByRole('button', { name: 'Retry' })).toBeVisible() }) it('keeps other images usable and tracks failures by source when the list changes', async () => { - const { rerender } = render() - fireEvent.error(screen.getByRole('img', { name: 'denied.png' })) + const { rerender } = render() + fireEvent.error(screen.getByRole('img', { name: 'broken.png' })) await screen.findByRole('alert') - expect(screen.getByRole('img', { name: 'allowed.png' })).toBeVisible() - fireEvent.click(screen.getByRole('button', { name: /allowed.png/ })) + expect(screen.getByRole('img', { name: 'chart.png' })).toBeVisible() + fireEvent.click(screen.getByRole('button', { name: /chart.png/ })) expect(screen.getByRole('dialog')).toBeVisible() fireEvent.click(screen.getByRole('button', { name: 'Close' })) - rerender() + rerender() expect(screen.getByRole('img', { name: 'new.png' })).toBeVisible() - expect(screen.getByRole('alert')).toHaveTextContent('denied.png') - expect(screen.queryByRole('img', { name: 'denied.png' })).not.toBeInTheDocument() + expect(screen.getByRole('alert')).toHaveTextContent('broken.png') + expect(screen.queryByRole('img', { name: 'broken.png' })).not.toBeInTheDocument() }) - it('retries the same protected URL and keeps feedback if the retry fails', async () => { - render() + describe('an image the server will not serve', () => { + // Most paths a reply names that do not load are not there at all: a file + // cleaned out of /tmp since, a web route, a name quoted from a log. That is + // no fault and no retry fixes it, yet every such reply in the history carried + // a red "unable to load" notice. + it.each([ + [404, 'it is missing'], + [403, 'it is outside the readable roots'], + [400, 'it is not an image file'], + ])('leaves no trace when the server answers %i: %s', async (status) => { + fetchServerImageBlobUrl.mockRejectedValue(new ApiError(status, { error: 'refused' })) + render() + + fireEvent.error(screen.getByRole('img')) + + await waitFor(() => expect(screen.queryByRole('img')).not.toBeInTheDocument()) + expect(screen.queryByRole('alert')).not.toBeInTheDocument() + expect(screen.queryByText('1 image')).not.toBeInTheDocument() + }) + + it('keeps the images that load beside one that is gone', async () => { + fetchServerImageBlobUrl.mockRejectedValue(new ApiError(404, { error: 'File not found' })) + render() + + fireEvent.error(screen.getByRole('img', { name: 'gone.png' })) + + await waitFor(() => expect(screen.queryByRole('img', { name: 'gone.png' })).not.toBeInTheDocument()) + expect(screen.getByRole('img', { name: 'chart.png' })).toBeVisible() + expect(screen.getByText('1 image')).toBeInTheDocument() + }) + + it('leaves no trace for a missing workspace image the prose spelled out with its directory', async () => { + fetchServerImageBlobUrl.mockRejectedValue(new ApiError(404, 'not found')) + render( + , + ) + + fireEvent.error(screen.getByRole('img')) + + await waitFor(() => expect(screen.queryByRole('img')).not.toBeInTheDocument()) + expect(screen.queryByRole('alert')).not.toBeInTheDocument() + }) + + it('tries again in another workspace, where the same path can exist', async () => { + fetchServerImageBlobUrl.mockRejectedValue(new ApiError(404, { error: 'File not found' })) + const { rerender } = render() + fireEvent.error(screen.getByRole('img')) + await waitFor(() => expect(screen.queryByRole('img')).not.toBeInTheDocument()) + + rerender() + + expect(screen.getByRole('img', { name: 'chart.png' })).toBeVisible() + }) + }) + + it('retries the same URL and keeps feedback if the retry fails', async () => { + render() const source = screen.getByRole('img').getAttribute('src') fireEvent.error(screen.getByRole('img')) fireEvent.click(await screen.findByRole('button', { name: 'Retry' })) @@ -140,10 +203,10 @@ describe('InlineImageGallery', () => { { sessionId: 'new-session', workDir: '/tmp/old' }, { sessionId: 'old-session', workDir: '/tmp/new' }, ])('clears a failed absolute source when context changes to %j', (context) => { - const { rerender } = render() + const { rerender } = render() const source = screen.getByRole('img').getAttribute('src') fireEvent.error(screen.getByRole('img')) - rerender() + rerender() expect(screen.queryByRole('alert')).not.toBeInTheDocument() expect(screen.getByRole('img')).toHaveAttribute('src', source) }) @@ -190,9 +253,10 @@ describe('InlineImageGallery', () => { describe('an image the prose only names, without a path', () => { // A read-only turn ("which commit swapped nodemaven_banner_sep.png?") names a - // file it never wrote. Nothing proves it sits at the workdir root, so a failed - // load is a wrong guess, not a broken deliverable worth a red error block. - it('does not raise the error block when a guessed bare name fails to load', async () => { + // file it never wrote. Resolved at the workdir root it is not there, and a file + // that is not there leaves no trace. + it('does not raise the error block when a guessed bare name is not there', async () => { + fetchServerImageBlobUrl.mockRejectedValue(new ApiError(404, 'not found')) render( { expect(screen.queryByText('1 image')).not.toBeInTheDocument() }) - it('still raises the error block for a path the prose spelled out, though the checkpoint missed it', async () => { - // A shell-rendered image never reaches changedFiles; its failure is real. - render( - , - ) - - fireEvent.error(screen.getByRole('img')) - - expect(await screen.findByRole('alert')).toHaveTextContent('frame.png') - }) - it('shows the image the workspace really holds when a verb is glued to its name', async () => { getWorkspaceTree.mockResolvedValue({ state: 'ok', @@ -251,19 +299,75 @@ describe('InlineImageGallery', () => { ])) }) - it('still raises the error block for a name the turn really wrote', async () => { + it('shows the notice for a guessed name too when its load fails for another reason', async () => { + // How the reply named a file does not decide the notice; why the load failed does. render( , ) fireEvent.error(screen.getByRole('img')) - expect(await screen.findByRole('alert')).toHaveTextContent('banner.png') + expect(await screen.findByRole('alert')).toHaveTextContent('nodemaven_banner_sep.png') + }) + }) + + describe('a pattern the text quotes, not a file', () => { + // The reply pointed at the screenshots it had taken as a glob. No URL loads + // `0*.png`, so the gallery showed a red "unable to load" tile with a retry. + it('shows nothing for a glob quoted in the reply', () => { + render( + , + ) + + expect(screen.queryAllByRole('img')).toHaveLength(0) + expect(screen.queryByText('1 image')).not.toBeInTheDocument() + }) + + it.each([ + ['a wildcard directory listing', 'zsh: no matches found: /tmp/pres-verify/steps/*.png'], + ['a wildcard name', '截图在 /private/tmp/todo_*.png'], + ['a single-character wildcard', '帧序列 /tmp/frames/shot-??.png'], + ['a format-string placeholder', 'plt.savefig(f"/tmp/plots/{name}.png")'], + ['a template variable', 'logo 路径是 /connectors/${brand.id}.svg'], + ['a brace expansion', '两张图 /tmp/{before,after}.png'], + ['a shell variable', 'cp shot.png /tmp/$NAME.png'], + ['a printf frame pattern', 'ffmpeg -i in.mp4 /tmp/frames/f%03d.png'], + ])('shows nothing for %s', (_shape, text) => { + render() + + expect(screen.queryAllByRole('img')).toHaveLength(0) + }) + + it('still shows the concrete file named beside the pattern', () => { + render( + , + ) + + expect(imgSrcs()).toEqual([ + 'http://127.0.0.1:3456/api/filesystem/file?path=' + encodeURIComponent('/tmp/cc-haha-ui-review/01-initial.png'), + ]) + }) + + it('keeps a real directory whose name has brackets', () => { + render() + + expect(imgSrcs()).toEqual([ + 'http://127.0.0.1:3456/api/filesystem/file?path=' + encodeURIComponent('/w/app/blog/[slug]/opengraph-image.png'), + ]) }) }) diff --git a/desktop/src/components/chat/InlineImageGallery.tsx b/desktop/src/components/chat/InlineImageGallery.tsx index 71252486..cafee46d 100644 --- a/desktop/src/components/chat/InlineImageGallery.tsx +++ b/desktop/src/components/chat/InlineImageGallery.tsx @@ -18,11 +18,18 @@ const IMAGE_EXTENSIONS = /\.(png|jpe?g|gif|webp|svg|bmp|avif|ico)$/i /** * Extracts absolute image file paths from text content. * Matches paths like /Users/.../image.png, /tmp/output.jpg, etc. + * + * Wildcards (`*`, `?`) and substitutions (`{name}`, `${id}`, `$NAME`, `%03d`) + * are not path characters: the character that opens one ends the match. A path + * like `/tmp/shots/0*.png` or `/tmp/frames/f%03d.png` names a set of files, or a + * template for one, that no URL can load; taken as a file, it became a red + * "unable to load" tile under a reply that only quoted it. Brackets stay: real + * directories use them (`app/[slug]/opengraph-image.png`). */ export function extractImagePaths(text: string): string[] { // Match absolute paths ending with image extensions // Handles paths that may be wrapped in backticks, quotes, or standalone - const regex = /(?:^|[\s`"'(])(\/?(?:[A-Za-z]:[\\/]|\/)[^\s`"')<>]+\.(?:png|jpe?g|gif|webp|svg|bmp|avif|ico))/gim + const regex = /(?:^|[\s`"'(])(\/?(?:[A-Za-z]:[\\/]|\/)[^\s`"')<>*?{$%]+\.(?:png|jpe?g|gif|webp|svg|bmp|avif|ico))/gim const paths: string[] = [] const seen = new Set() @@ -58,16 +65,6 @@ type GalleryImage = { name: string /** Where the file is, for "open in system app". Relative until the workdir is known. */ path: string - /** - * The prose gave only a bare name and nothing the turn wrote corroborates where - * the file is, so the URL is a guess. A failed load is then a wrong guess, not - * a broken deliverable. - */ - inferred?: boolean -} - -function samePath(left: string, right: string): boolean { - return left.replaceAll('\\', '/').toLowerCase() === right.replaceAll('\\', '/').toLowerCase() } type Props = { @@ -87,12 +84,17 @@ type Props = { export function InlineImageGallery({ text, sessionId, workDir, changedFiles, suppressManagedGeneratedImages = false }: Props) { const t = useTranslation() const [activeIndex, setActiveIndex] = useState(null) - const [failureState, setFailureState] = useState(() => ({ sessionId, workDir, sources: new Set() })) + const [failureState, setFailureState] = useState(() => ({ + sessionId, + workDir, + failed: new Set(), + unavailable: new Set(), + })) // The same absolute URL can become readable in a different workspace/session. if (failureState.sessionId !== sessionId || failureState.workDir !== workDir) { - setFailureState({ sessionId, workDir, sources: new Set() }) + setFailureState({ sessionId, workDir, failed: new Set(), unavailable: new Set() }) } - const failedSources = failureState.sources + const failedSources = failureState.failed const markdownImageSources = useMemo( () => new Set(extractMarkdownImageSources(text).map(normalizeImageReference)), @@ -153,7 +155,6 @@ export function InlineImageGallery({ text, sessionId, workDir, changedFiles, sup // Dedup: an absolute path inside the workspace can be caught by BOTH sources. // Skip a relative target whose basename already appears among the absolute // images, and also collapse duplicate relative targets by resolved src. - const proseText = text.replaceAll('\\', '/') const absoluteNames = new Set(absolute.map((img) => img.name)) const seenSrc = new Set(absolute.map((img) => img.src)) const relative: GalleryImage[] = [] @@ -171,21 +172,17 @@ export function InlineImageGallery({ text, sessionId, workDir, changedFiles, sup continue } seenSrc.add(src) - const openPath = resolveAbsoluteOpenPath(relPath, workDir ?? undefined) - const corroborated = changedFileEvidence?.some((file) => samePath(file, openPath)) ?? false - // A path the prose spells out with its directory is a claim, even when the - // checkpoint missed it (shell writes are invisible there); only a bare name, - // placed at the root or in an inferred directory, is a guess. - const namedWithDirectory = /[\\/]/.test(relPath) && proseText.includes(relPath.replaceAll('\\', '/')) - relative.push({ src, name, path: openPath, inferred: !corroborated && !namedWithDirectory }) + relative.push({ src, name, path: resolveAbsoluteOpenPath(relPath, workDir ?? undefined) }) } return [...absolute, ...relative] - }, [changedFileEvidence, imagePaths, relativeTargets, sessionId, text, workDir]) + }, [imagePaths, relativeTargets, sessionId, workDir]) - // A guessed image that failed to load leaves no trace: there is nothing to retry - // when the file was never claimed to be there. - const visibleImages = images.filter((img) => !(img.inferred && failedSources.has(img.src))) + // A picture the server will not serve — missing, outside the readable roots, not + // an image — leaves no trace. The reply only named it, and its text still does: + // a file cleaned out of /tmp, a web route, a name quoted from a log. Only a load + // that should have worked is an error worth a retry. + const visibleImages = images.filter((img) => !failureState.unavailable.has(img.src)) if (visibleImages.length === 0) return null return ( @@ -203,9 +200,9 @@ export function InlineImageGallery({ text, sessionId, workDir, changedFiles, sup title={t('chat.imageLoadFailed')} retryLabel={t('common.retry')} onRetry={() => setFailureState((previous) => { - const sources = new Set(previous.sources) - sources.delete(img.src) - return { ...previous, sources } + const failed = new Set(previous.failed) + failed.delete(img.src) + return { ...previous, failed } })} detail={( <> @@ -227,9 +224,10 @@ export function InlineImageGallery({ text, sessionId, workDir, changedFiles, sup loading="lazy" className="w-full object-cover" style={{ maxHeight: visibleImages.length === 1 ? 400 : 240 }} - // img errors expose no HTTP status: a denied, missing or invalid - // image needs visible feedback without claiming a specific cause. - onFailure={() => setFailureState((previous) => ({ ...previous, sources: new Set(previous.sources).add(img.src) }))} + onFailure={(failure) => setFailureState((previous) => ({ + ...previous, + [failure]: new Set(previous[failure]).add(img.src), + }))} />
diff --git a/desktop/src/components/markdown/MarkdownHtml.test.tsx b/desktop/src/components/markdown/MarkdownHtml.test.tsx index b00ec4f7..54115e3e 100644 --- a/desktop/src/components/markdown/MarkdownHtml.test.tsx +++ b/desktop/src/components/markdown/MarkdownHtml.test.tsx @@ -1,6 +1,7 @@ import '@testing-library/jest-dom' import { act, fireEvent, render, screen, waitFor } from '@testing-library/react' import { beforeEach, describe, expect, it, vi } from 'vitest' +import { ApiError } from '@/api/client' import { useSettingsStore } from '@/stores/settingsStore' import { MarkdownHtml } from './MarkdownHtml' @@ -8,7 +9,8 @@ const fetchServerImageBlobUrl = vi.hoisted(() => vi.fn()) vi.mock('@/lib/authedImage', () => ({ fetchServerImageBlobUrl })) beforeEach(() => { - fetchServerImageBlobUrl.mockReset().mockRejectedValue(new Error('404')) + // A server fault: the load should have worked, so the retryable notice shows. + fetchServerImageBlobUrl.mockReset().mockRejectedValue(new ApiError(500, { error: 'Internal error' })) useSettingsStore.setState({ locale: 'en' }) Object.defineProperty(URL, 'revokeObjectURL', { value: vi.fn(), configurable: true, writable: true }) }) @@ -44,26 +46,65 @@ describe('MarkdownHtml image lifecycle', () => { }) it('reacts to a locale change while the failure is visible', async () => { - render('} />) + render('} />) fireEvent.error(screen.getByRole('img')) await screen.findByRole('alert') act(() => useSettingsStore.setState({ locale: 'zh' })) expect(screen.getByRole('alert')).toHaveTextContent('无法加载图片') - expect(screen.getByRole('button', { name: '重试图片：missing.png' })).toBeInTheDocument() + expect(screen.getByRole('button', { name: '重试图片：broken.png' })).toBeInTheDocument() }) it('keeps the placeholder when the retry returns a body that cannot decode', async () => { - fetchServerImageBlobUrl.mockRejectedValueOnce(new Error('404')).mockResolvedValueOnce('blob:invalid') - const { container } = render('} />) + fetchServerImageBlobUrl.mockRejectedValueOnce(new ApiError(500, {})).mockResolvedValueOnce('blob:invalid') + const { container } = render('} />) fireEvent.error(screen.getByRole('img')) await screen.findByRole('alert') fireEvent.click(screen.getByRole('button', { name: /Retry image/ })) await waitFor(() => expect(container.querySelector('img')).toHaveAttribute('src', 'blob:invalid')) fireEvent.error(container.querySelector('img')!) - expect(screen.getByRole('alert')).toHaveTextContent('missing.png') + expect(screen.getByRole('alert')).toHaveTextContent('broken.png') expect(screen.getByRole('button', { name: /Retry image/ })).toBeEnabled() }) + describe('an image the server will not serve', () => { + // A picture the reply embedded that is missing or outside the readable roots + // is not a fault to retry: like any image that cannot be shown, it falls back + // to its text alternative instead of a red notice. + it.each([404, 403])('falls back to its description after HTTP %i', async (status) => { + fetchServerImageBlobUrl.mockRejectedValue(new ApiError(status, { error: 'refused' })) + render(效果如下：架构图

'} />) + + fireEvent.error(screen.getByRole('img')) + + expect(await screen.findByText('架构图')).toBeVisible() + expect(screen.queryByRole('img')).not.toBeInTheDocument() + expect(screen.queryByRole('alert')).not.toBeInTheDocument() + expect(screen.queryByRole('button', { name: /Retry image/ })).not.toBeInTheDocument() + }) + + it('leaves nothing in place of one without a description', async () => { + fetchServerImageBlobUrl.mockRejectedValue(new ApiError(404, { error: 'File not found' })) + const { container } = render(效果如下：

'} />) + + fireEvent.error(container.querySelector('img')!) + + await waitFor(() => expect(container.querySelector('img')).not.toBeVisible()) + expect(container).toHaveTextContent(/^效果如下：$/) + expect(screen.queryByRole('alert')).not.toBeInTheDocument() + }) + + it('settles on the description when a retry finds the file gone', async () => { + fetchServerImageBlobUrl.mockRejectedValueOnce(new ApiError(500, {})) + .mockRejectedValueOnce(new ApiError(404, { error: 'File not found' })) + render('} />) + fireEvent.error(screen.getByRole('img')) + fireEvent.click(await screen.findByRole('button', { name: /Retry image/ })) + + expect(await screen.findByText('chart')).toBeVisible() + expect(screen.queryByRole('alert')).not.toBeInTheDocument() + }) + }) + it('frees the old image copy and resets failures when the Markdown changes', async () => { fetchServerImageBlobUrl.mockResolvedValue('blob:old') const { rerender } = render('} />) diff --git a/desktop/src/components/markdown/MarkdownHtml.tsx b/desktop/src/components/markdown/MarkdownHtml.tsx index 18f67999..bfabfd60 100644 --- a/desktop/src/components/markdown/MarkdownHtml.tsx +++ b/desktop/src/components/markdown/MarkdownHtml.tsx @@ -23,10 +23,11 @@ function imageName(src: string, fallback: string): string { function MarkdownImage(props: ImageProps) { const t = useTranslation() const errorId = useId() - const [status, setStatus] = useState<'initial' | 'failed' | 'retrying'>('initial') + const [status, setStatus] = useState<'initial' | 'unavailable' | 'failed' | 'retrying'>('initial') const [attempt, setAttempt] = useState(0) const name = imageName(props.src ?? '', props.alt || t('assistantOutputs.kind.image')) - const showError = status !== 'initial' + const hidden = status !== 'initial' + const showError = status === 'failed' || status === 'retrying' return ( <> @@ -34,11 +35,17 @@ function MarkdownImage(props: ImageProps) { {...props} key={attempt} retryWithCredential={attempt > 0} - hidden={showError} - style={{ display: showError ? 'none' : undefined }} + hidden={hidden} + style={{ display: hidden ? 'none' : undefined }} onLoad={() => setStatus('initial')} - onFailure={() => setStatus('failed')} + onFailure={setStatus} /> + {/* A picture the server will not serve (missing, outside the readable roots) + is not a fault to retry. Like any image that cannot be shown, it falls + back to its text alternative. */} + {status === 'unavailable' && props.alt && ( + {props.alt} + )} {showError && ( diff --git a/desktop/src/components/markdown/MarkdownRenderer.authedImage.test.tsx b/desktop/src/components/markdown/MarkdownRenderer.authedImage.test.tsx index 37aa25b3..9121c2c2 100644 --- a/desktop/src/components/markdown/MarkdownRenderer.authedImage.test.tsx +++ b/desktop/src/components/markdown/MarkdownRenderer.authedImage.test.tsx @@ -10,6 +10,7 @@ vi.mock('../../api/client', async (original) => ({ getBaseUrl: () => 'http://127.0.0.1:3456', })) +import { ApiError } from '../../api/client' import { MarkdownRenderer } from './MarkdownRenderer' import { useSettingsStore } from '@/stores/settingsStore' @@ -23,10 +24,25 @@ beforeEach(() => { }) describe('MarkdownRenderer local images', () => { + // A missing or unreadable picture is no fault, and a red notice for each one — + // every embed of a file since cleaned out of /tmp — was noise. It falls back to + // its description, as an image that cannot be shown does. + it.each([404, 403])('shows the description instead of an error after HTTP %s', async (status) => { + apiGetBlob.mockRejectedValue(new ApiError(status, { error: 'refused' })) + const { container } = render( + LOCAL.replace('chart.png', 'missing.png')} />, + ) + fireEvent.error(container.querySelector('img')!) + + expect(await screen.findByText('description')).toBeVisible() + expect(screen.queryByRole('alert')).not.toBeInTheDocument() + expect(screen.queryByRole('img')).not.toBeInTheDocument() + }) + // QA-002: a terminal image error used to leave only the browser's broken icon. - it.each([404, 403])('keeps a named error placeholder after HTTP %s and lets a retry recover', async (status) => { - apiGetBlob.mockRejectedValueOnce(new Error(String(status))) - .mockRejectedValueOnce(new Error(String(status))) + it('keeps a named error placeholder after a server fault and lets a retry recover', async () => { + apiGetBlob.mockRejectedValueOnce(new ApiError(500, { error: 'Internal error' })) + .mockRejectedValueOnce(new ApiError(500, { error: 'Internal error' })) .mockResolvedValueOnce(new Blob(['png'], { type: 'image/png' })) const { container } = render( LOCAL.replace('chart.png', 'missing.png')} />, @@ -36,7 +52,7 @@ describe('MarkdownRenderer local images', () => { const alert = await screen.findByRole('alert') expect(alert).toHaveTextContent('Unable to load image') expect(alert).toHaveTextContent('missing.png') - expect(alert).toHaveTextContent('The file may be missing or access may be denied.') + expect(alert).toHaveTextContent('The file may be damaged, or the local server did not respond.') expect(screen.queryByRole('img')).not.toBeInTheDocument() const retry = screen.getByRole('button', { name: 'Retry image: missing.png' }) expect(retry).toHaveAttribute('aria-describedby') @@ -56,7 +72,7 @@ describe('MarkdownRenderer local images', () => { }) it('keeps failures local to one image and excludes it from the viewer', async () => { - apiGetBlob.mockRejectedValue(new Error('404')) + apiGetBlob.mockRejectedValue(new ApiError(500, { error: 'Internal error' })) const onImageClick = vi.fn() const { container } = render( LOCAL.replace('chart.png', src)} onImageClick={onImageClick} />, @@ -68,7 +84,7 @@ describe('MarkdownRenderer local images', () => { }) it('does not navigate a surrounding link when retry is clicked', async () => { - apiGetBlob.mockRejectedValue(new Error('404')) + apiGetBlob.mockRejectedValue(new ApiError(500, { error: 'Internal error' })) const onLinkClick = vi.fn() const { container } = render( LOCAL} onLinkClick={onLinkClick} />, @@ -82,7 +98,7 @@ describe('MarkdownRenderer local images', () => { }) it('does not reuse a failure from another renderer with the same parsed Markdown', async () => { - apiGetBlob.mockRejectedValue(new Error('404')) + apiGetBlob.mockRejectedValue(new ApiError(500, { error: 'Internal error' })) const content = '![missing](missing.png)' const { container } = render(<> LOCAL} /> LOCAL} />) const prose = container.querySelectorAll('.markdown-prose') diff --git a/desktop/src/i18n/locales/en.ts b/desktop/src/i18n/locales/en.ts index 22ee5578..7bbbac6b 100644 --- a/desktop/src/i18n/locales/en.ts +++ b/desktop/src/i18n/locales/en.ts @@ -2652,7 +2652,7 @@ Row 9, all 8 cells: continuing from straight down, turning left through lower-le 'chat.contextReferencesOnly': 'Added {count} references', 'chat.addSelectionToChat': 'Add to chat', 'chat.imageLoadFailed': 'Unable to load image', - 'chat.imageLoadFailedHint': 'The file may be missing or access may be denied.', + 'chat.imageLoadFailedHint': 'The file may be damaged, or the local server did not respond.', 'chat.retryImage': 'Retry image: {name}', 'chat.imageRetrying': 'Retrying…', 'chat.branchFromHere': 'Fork a new conversation', diff --git a/desktop/src/i18n/locales/jp.ts b/desktop/src/i18n/locales/jp.ts index 472e433f..e72a8d1b 100644 --- a/desktop/src/i18n/locales/jp.ts +++ b/desktop/src/i18n/locales/jp.ts @@ -2653,7 +2653,7 @@ export const jp: Record = { 'chat.contextReferencesOnly': '参照を {count} 件追加しました', 'chat.addSelectionToChat': 'チャットに追加', 'chat.imageLoadFailed': '画像を読み込めません', - 'chat.imageLoadFailedHint': 'ファイルが存在しないか、アクセスが許可されていない可能性があります。', + 'chat.imageLoadFailedHint': 'ファイルが破損しているか、ローカルサーバーが応答しませんでした。', 'chat.retryImage': '画像を再読み込み: {name}', 'chat.imageRetrying': '再読み込み中…', 'chat.branchFromHere': '新しい会話を分岐', diff --git a/desktop/src/i18n/locales/kr.ts b/desktop/src/i18n/locales/kr.ts index 61324129..d0f5da58 100644 --- a/desktop/src/i18n/locales/kr.ts +++ b/desktop/src/i18n/locales/kr.ts @@ -2655,7 +2655,7 @@ export const kr: Record = { 'chat.contextReferencesOnly': '참조 {count}개를 추가했습니다', 'chat.addSelectionToChat': '채팅에 추가', 'chat.imageLoadFailed': '이미지를 불러올 수 없습니다', - 'chat.imageLoadFailedHint': '파일이 없거나 접근 권한이 없을 수 있습니다.', + 'chat.imageLoadFailedHint': '파일이 손상되었거나 로컬 서버가 응답하지 않았습니다.', 'chat.retryImage': '이미지 다시 시도: {name}', 'chat.imageRetrying': '다시 시도 중…', 'chat.branchFromHere': '새 대화 분기', diff --git a/desktop/src/i18n/locales/zh-TW.ts b/desktop/src/i18n/locales/zh-TW.ts index aa839801..a866ad4d 100644 --- a/desktop/src/i18n/locales/zh-TW.ts +++ b/desktop/src/i18n/locales/zh-TW.ts @@ -2652,7 +2652,7 @@ export const zh: Record = { 'chat.contextReferencesOnly': '已新增 {count} 個引用', 'chat.addSelectionToChat': '新增到對話', 'chat.imageLoadFailed': '無法載入圖片', - 'chat.imageLoadFailedHint': '檔案可能不存在，或沒有存取權限。', + 'chat.imageLoadFailedHint': '檔案可能已損壞，或本機服務暫時沒有回應。', 'chat.retryImage': '重試圖片：{name}', 'chat.imageRetrying': '正在重試…', 'chat.branchFromHere': 'Fork 一個新對話', diff --git a/desktop/src/i18n/locales/zh.ts b/desktop/src/i18n/locales/zh.ts index f8a883d2..f912eedc 100644 --- a/desktop/src/i18n/locales/zh.ts +++ b/desktop/src/i18n/locales/zh.ts @@ -2651,7 +2651,7 @@ export const zh: Record = { 'chat.contextReferencesOnly': '已添加 {count} 个引用', 'chat.addSelectionToChat': '添加到对话', 'chat.imageLoadFailed': '无法加载图片', - 'chat.imageLoadFailedHint': '文件可能不存在，或没有访问权限。', + 'chat.imageLoadFailedHint': '文件可能已损坏，或本地服务暂时没有响应。', 'chat.retryImage': '重试图片：{name}', 'chat.imageRetrying': '正在重试…', 'chat.branchFromHere': 'Fork 一个新对话', diff --git a/desktop/src/lib/useAuthedImageFallback.ts b/desktop/src/lib/useAuthedImageFallback.ts index 2399844e..8178500d 100644 --- a/desktop/src/lib/useAuthedImageFallback.ts +++ b/desktop/src/lib/useAuthedImageFallback.ts @@ -1,16 +1,38 @@ import { useCallback, useEffect, useRef, useState } from 'react' +import { ApiError } from '../api/client' import { fetchServerImageBlobUrl } from './authedImage' +/** + * Why an image did not load. + * + * `unavailable`: the local server answered that it will not serve the path — the + * file is missing, outside what this client may read, or not an image it serves. + * A path pulled from a reply often names nothing at all (a pattern, a web route, + * a file cleaned out of /tmp since), so this is no fault and no retry fixes it. + * + * `failed`: something that should have worked did not — a server fault, a dropped + * connection, a refused credential, or bytes that do not decode as an image. + */ +export type ImageFailure = 'unavailable' | 'failed' + +// What the file routes answer for a path they will not serve: 404 missing, 403 +// outside the allowed roots, 400 not an image or not a file, 413 too large. +const UNAVAILABLE_STATUSES: ReadonlySet = new Set([400, 403, 404, 413]) + +function classifyFailure(error: unknown): ImageFailure { + return error instanceof ApiError && UNAVAILABLE_STATUSES.has(error.status) ? 'unavailable' : 'failed' +} + /** * Let an `` that a plain request could not load try once more with the app's * credential (see {@link fetchServerImageBlobUrl}). * * Spread `src` and `onError` onto the image. `onFailure` runs only when the - * authenticated attempt has also failed — a missing or denied file, or a body that - * is not an image — so callers keep their own failure notice for real failures and - * never flash it for a request that was merely missing a header. + * authenticated attempt has also failed, and says why (see {@link ImageFailure}), + * so callers keep their own failure notice for real failures and never flash it + * for a request that was merely missing a header. */ -export function useAuthedImageFallback(src: string | undefined, onFailure?: () => void, retryWithCredential = false) { +export function useAuthedImageFallback(src: string | undefined, onFailure?: (failure: ImageFailure) => void, retryWithCredential = false) { const attempt = useRef({ src, state: 'idle' as 'idle' | 'fetching' | 'resolved' | 'failed' }) if (attempt.current.src !== src) attempt.current = { src, state: 'idle' } const current = attempt.current @@ -34,7 +56,9 @@ export function useAuthedImageFallback(src: string | undefined, onFailure?: () = if (attempt.current !== current || current.state === 'fetching' || current.state === 'failed') return if (!src || current.state === 'resolved') { current.state = 'failed' - onFailureRef.current?.() + // No source names nothing to show. A copy the server did send that still + // errors is a file that is there and does not decode. + onFailureRef.current?.(src ? 'failed' : 'unavailable') return } current.state = 'fetching' @@ -47,10 +71,10 @@ export function useAuthedImageFallback(src: string | undefined, onFailure?: () = current.state = 'resolved' objectUrls.current.push(url) setResolved({ attempt: current, url }) - }).catch(() => { + }).catch((error: unknown) => { if (alive.current && attempt.current === current) { current.state = 'failed' - onFailureRef.current?.() + onFailureRef.current?.(classifyFailure(error)) } }) }, [src, current, retryWithCredential]) diff --git a/src/server/__tests__/filesystem.test.ts b/src/server/__tests__/filesystem.test.ts index f03a449e..b8a9a462 100644 --- a/src/server/__tests__/filesystem.test.ts +++ b/src/server/__tests__/filesystem.test.ts @@ -376,6 +376,22 @@ describe('filesystem API', () => { expect(Buffer.from(await res.arrayBuffer()).equals(PNG)).toBe(true) }) + it('answers 404 for a picture missing from an allowed root', async () => { + // The chat reads 400/403/404 as "not there to show" and leaves the picture out + // quietly; any other answer is an error it offers to retry. A file that is + // simply missing — cleaned out of /tmp since the reply named it — stays a 404. + const dir = fs.realpathSync(await fsp.mkdtemp(path.join(os.tmpdir(), 'claude-filesystem-test-'))) + cleanupDirs.add(dir) + registerFilesystemAccessRoot(dir) + + const res = await handleFilesystemRoute( + '/api/filesystem/file', + makeUrl('/api/filesystem/file', { path: path.join(dir, 'cleaned', 'chart.png') }), + ) + + expect(res.status).toBe(404) + }) + it('still keeps a home-relative path inside the allowed roots', async () => { // A picture that really exists, outside $HOME, the temp directories and every // registered root, reached by climbing out of the home directory. A missing