From bbe39ef32dc95c33ab7cccaf5317fa526afaf85e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=A8=8B=E5=BA=8F=E5=91=98=E9=98=BF=E6=B1=9F=28Relakkes?= =?UTF-8?q?=29?= Date: Sun, 4 Oct 2026 22:13:38 +0800 Subject: [PATCH] fix(desktop): show no error for images that are not there A reply named the screenshots it had taken as `/tmp/cc-haha-ui-review/0*.png`, and a red "Unable to load image" tile with a retry button appeared under it. The inline gallery took the glob for a file: its absolute-path pattern accepted any character but whitespace and quotes. Wildcards and substitutions (`*`, `?`, `{name}`, `${id}`, `$NAME`, `%03d`) now end a path there. Brackets stay, since real directories use them. The tile was the larger problem. In the local session history we checked, 45 of the 115 pictures the gallery tried to show were red tiles and only 18 existed: files cleaned out of /tmp, outputs deleted since, web routes, example paths. A retry fixes none of them. Every failure went red because an error carries no status, but the authenticated fetch that follows it does. A 400, 403, 404 or 413 from the file routes now means the picture is not there to show: the inline tile disappears, and a Markdown image falls back to its alt text. A server fault, a dropped connection, a refused credential or bytes that do not decode still show the retryable error, whose hint now names those causes. The guessed-name versus spelled-out-path split from #1429 goes: why the load failed decides, not how the path was written. A server test pins the 404 for a file missing from an allowed root, which the rule relies on. --- .../src/components/chat/AuthedImage.test.tsx | 32 +++ desktop/src/components/chat/AuthedImage.tsx | 6 +- .../chat/InlineImageGallery.test.tsx | 188 ++++++++++++++---- .../components/chat/InlineImageGallery.tsx | 62 +++--- .../components/markdown/MarkdownHtml.test.tsx | 53 ++++- .../src/components/markdown/MarkdownHtml.tsx | 17 +- .../MarkdownRenderer.authedImage.test.tsx | 30 ++- desktop/src/i18n/locales/en.ts | 2 +- desktop/src/i18n/locales/jp.ts | 2 +- desktop/src/i18n/locales/kr.ts | 2 +- desktop/src/i18n/locales/zh-TW.ts | 2 +- desktop/src/i18n/locales/zh.ts | 2 +- desktop/src/lib/useAuthedImageFallback.ts | 38 +++- src/server/__tests__/filesystem.test.ts | 16 ++ 14 files changed, 345 insertions(+), 107 deletions(-) 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