fix: only show artifact download success after the file is delivered

useAttachmentLink swallows fetch errors (an expired code-output URL or a
404 share download) and resolves without throwing, so the preview-panel
download button flipped to the success checkmark even when no file was
downloaded.

Return a boolean from handleDownload (true once a download is initiated,
false on error/empty response) and only mark the artifact download as
succeeded when a file was actually delivered. The return value is
ignored by the existing onClick callers.
This commit is contained in:
Marco Beretta 2026-07-01 01:10:54 +02:00
parent 149a0dce9d
commit 1f8394757f
No known key found for this signature in database
GPG key ID: D918033D8E74CC11
3 changed files with 42 additions and 9 deletions

View file

@ -71,8 +71,13 @@ const DownloadArtifact = ({ artifact }: { artifact: Artifact }) => {
const handleDownload = async (event: React.MouseEvent<HTMLButtonElement>) => {
try {
if (downloadOriginalFile) {
await downloadAttachment(event);
markDownloaded();
// Only flag success when a file was actually delivered; the
// attachment helper swallows fetch errors (e.g. an expired
// code-output URL or a 404 share download) and resolves either way.
const downloaded = await downloadAttachment(event);
if (downloaded) {
markDownloaded();
}
return;
}
downloadContent();

View file

@ -87,7 +87,9 @@ describe('DownloadArtifact', () => {
let anchorClick: jest.SpyInstance;
beforeEach(() => {
mockFileDownload.mockClear();
mockFileDownload.mockReset();
// The attachment helper resolves to `true` when a file was delivered.
mockFileDownload.mockResolvedValue(true);
createObjectURL = jest.fn(() => 'blob:mock');
revokeObjectURL = jest.fn();
Object.defineProperty(window.URL, 'createObjectURL', {
@ -107,14 +109,29 @@ describe('DownloadArtifact', () => {
anchorClick.mockRestore();
});
it('downloads the original file (not the preview) for an office artifact', async () => {
render(<DownloadArtifact artifact={officeArtifact} />);
it('downloads the original file (not the preview) for an office artifact and shows success', async () => {
const { container } = render(<DownloadArtifact artifact={officeArtifact} />);
await act(async () => {
fireEvent.click(screen.getByRole('button'));
});
expect(mockFileDownload).toHaveBeenCalledTimes(1);
// The preview HTML must NOT be serialized into a blob download.
expect(createObjectURL).not.toHaveBeenCalled();
// A delivered file flips the button to the success checkmark.
expect(container.querySelector('.lucide-circle-check-big')).not.toBeNull();
});
it('does NOT show success when the original-file download fails', async () => {
// Expired code-output URL / 404 share download: the helper resolves
// to false instead of throwing. The checkmark must stay hidden.
mockFileDownload.mockResolvedValueOnce(false);
const { container } = render(<DownloadArtifact artifact={officeArtifact} />);
await act(async () => {
fireEvent.click(screen.getByRole('button'));
});
expect(mockFileDownload).toHaveBeenCalledTimes(1);
expect(container.querySelector('.lucide-circle-check-big')).toBeNull();
expect(container.querySelector('.lucide-download')).not.toBeNull();
});
it('serializes content as a blob for a non-file-backed (LLM-authored) artifact', async () => {

View file

@ -54,7 +54,16 @@ export const useAttachmentLink = ({
const { refetch: downloadFromApi } = useFileDownload(user, file_id, { source });
const { refetch: downloadFromUrl } = useCodeOutputDownload(href);
const handleDownload = async (event: React.MouseEvent<HTMLAnchorElement | HTMLButtonElement>) => {
/**
* Triggers the download and reports whether a file was actually
* delivered: `true` once a download is initiated, `false` on a fetch
* error or an empty/denied response (e.g. an expired code-output URL or
* a 404 share download). Callers that show success feedback should gate
* it on this result rather than on the promise merely resolving.
*/
const handleDownload = async (
event: React.MouseEvent<HTMLAnchorElement | HTMLButtonElement>,
): Promise<boolean> => {
event.preventDefault();
try {
// In a shared view, a snapshotted file's href is rewritten to the share
@ -63,12 +72,12 @@ export const useAttachmentLink = ({
// original href / code-output path still works when snapshots are disabled.
if (shareId && file_id && href.startsWith('/api/share/')) {
triggerDownload(sharedFileDownload(shareId, file_id), filename);
return;
return true;
}
if (!useLocalDownload && isHttpDownloadTarget(href)) {
triggerDownload(href, filename);
return;
return true;
}
const stream = useLocalDownload ? await downloadFromApi() : await downloadFromUrl();
@ -78,11 +87,13 @@ export const useAttachmentLink = ({
status: 'error',
message: 'Error downloading file',
});
return;
return false;
}
triggerDownload(stream.data, filename);
return true;
} catch (error) {
console.error('Error downloading file:', error);
return false;
}
};