diff --git a/.changeset/download-progress-in-one-place.md b/.changeset/download-progress-in-one-place.md new file mode 100644 index 00000000..ecafbe03 --- /dev/null +++ b/.changeset/download-progress-in-one-place.md @@ -0,0 +1,42 @@ +--- +'frontend': patch +--- + +Report a download in one place, and let a failed one be dismissed + +A download that hit a network error left a row nothing could remove, so the +panel showing it stayed up until the page was reloaded. The store gave +`completed` and `cancelled` rows a delay after which they clear themselves but +had no case for `error`; the operations panel mapped `upload` and `delete` to +their remove actions and never handled `download`, in both the row's remove and +the panel's close button; and the panel only hides once its list is empty. So +the close button did nothing, however many times it was pressed. + +A failed row also had no button on it at all. The row drew its trailing control +for active, then completed, then cancelled operations, and anything that had +gone wrong fell past all three to nothing - so the one row a reader most wants +rid of was the only one with nothing to press. That was true of failed uploads +and deletes too, and is fixed for all three. + +Settled downloads now wait to be dismissed instead of clearing themselves. They +used to go on a timer, three seconds for a completion and two for a cancel, +which read as tidy until a failure needed the same treatment: a reason for a +failure that takes itself off the screen is no use to anyone who was not looking +at that moment, and the panel can be collapsed. Uploads and deletes have always +waited to be dismissed, and downloads now match them. + +The same download was also announcing itself three times over: a toast, a card +of its own in the top right, and a row on the operations card in the bottom +right. The operations card is the one that stays, since it already carried every +status the separate card did, down to the queue position and the reason for a +failure. The separate card is gone, along with the toasts for starting, +cancelling and failing. One toast is left for a download that throws before the +service can record it, which leaves no row to read. The row now shows the +percentage while downloading, the only thing the removed card said that it did +not. + +The three copies of "which statuses count as still running" are now one, and it +knows about downloads. Each listed upload and delete statuses only, so a running +download read as settled - which would have mattered the moment the close button +learned to remove downloads, since it would have dropped the row of a transfer +still in flight rather than offering to cancel it. diff --git a/frontend/src/app/dashboard/layout.tsx b/frontend/src/app/dashboard/layout.tsx index 49f7c869..51d1339a 100644 --- a/frontend/src/app/dashboard/layout.tsx +++ b/frontend/src/app/dashboard/layout.tsx @@ -16,7 +16,6 @@ import { DetailsManager } from '@/features/dashboard/components/ui/details/detai import { FilePreviewModal } from '@/components/file-preview'; import { RenameModalManager } from '@/features/dashboard/components/ui/dialogs/rename-modal-manager'; import { ShareModalManager } from '@/features/dashboard/components/ui/dialogs/share-modal-manager'; -import { DownloadProgressManager } from '@/features/dashboard/components/ui/download-progress-manager'; import { useAuth } from '@/hooks/use-auth'; import { UploadCard } from '@/features/upload/components/upload-card'; import { useDeleteUnloadGuard } from '@/features/upload/hooks/use-delete-unload-guard'; @@ -150,10 +149,10 @@ const LayoutShell = ({ children }: { children: React.ReactNode }) => { + {/* Downloads report into this same card. They used to have a second one + of their own in the opposite corner, showing the same transfer twice. */} - - diff --git a/frontend/src/features/dashboard/components/ui/download-progress-manager.tsx b/frontend/src/features/dashboard/components/ui/download-progress-manager.tsx deleted file mode 100644 index 2e47a47d..00000000 --- a/frontend/src/features/dashboard/components/ui/download-progress-manager.tsx +++ /dev/null @@ -1,115 +0,0 @@ -'use client'; - -import React from 'react'; -import { X, Download, CheckCircle, AlertCircle } from 'lucide-react'; -import { useDownloadList } from '@/features/dashboard/hooks/use-download'; -import { cn } from '@/shared/utils/utils'; -import { AriaLabel } from '@/shared/components/custom-aria-label'; - -export const DownloadProgressManager: React.FC = () => { - const { getAllDownloads, cancelDownload } = useDownloadList(); - const downloads = getAllDownloads(); - - if (downloads.length === 0) return null; - - return ( -
- {downloads.map((download) => ( - cancelDownload(download.fileId)} - /> - ))} -
- ); -}; - -interface DownloadProgressItemProps { - download: { - fileId: string; - fileName: string; - progress: number; - status: 'queued' | 'pending' | 'downloading' | 'completed' | 'error' | 'cancelled'; - error?: string; - queuePosition?: number; - }; - onCancel: () => void; -} - -const DownloadProgressItem: React.FC = ({ download, onCancel }) => { - const getStatusIcon = () => { - switch (download.status) { - case 'completed': - return ; - case 'error': - return ; - default: - return ; - } - }; - - const getStatusText = () => { - switch (download.status) { - case 'queued': - return download.queuePosition - ? `Queued (position ${download.queuePosition})` - : 'Queued for download'; - case 'pending': - return 'Starting download...'; - case 'downloading': - return `${download.progress.toFixed(0)}% downloaded`; - case 'completed': - return 'Download completed'; - case 'cancelled': - return 'Download cancelled'; - case 'error': - return `Error: ${download.error || 'Download failed'}`; - default: - return 'Downloading...'; - } - }; - - const shouldShowProgress = - download.status === 'downloading' || - download.status === 'pending' || - download.status === 'queued'; - - return ( -
-
- {getStatusIcon()} -
-
{download.fileName}
-
{getStatusText()}
- {shouldShowProgress && ( -
-
-
- )} -
- {(download.status === 'downloading' || - download.status === 'pending' || - download.status === 'queued') && ( - - - - )} -
-
- ); -}; diff --git a/frontend/src/features/dashboard/components/ui/index.ts b/frontend/src/features/dashboard/components/ui/index.ts index 094c7052..f9c19182 100644 --- a/frontend/src/features/dashboard/components/ui/index.ts +++ b/frontend/src/features/dashboard/components/ui/index.ts @@ -14,8 +14,6 @@ export { CreateMenu } from './menus/create-menu'; export { FileOverflowMenu } from './menus/file-overflow-menu'; export { FolderOverflowMenu } from './menus/folder-overflow-menu'; -export { DownloadProgressManager } from './download-progress-manager'; - export { SuggestedSectionSkeleton, DashboardLoading } from './skeletons/dashboard-skeleton'; export { FileSkeletonGrid, diff --git a/frontend/src/features/dashboard/hooks/use-download.ts b/frontend/src/features/dashboard/hooks/use-download.ts index 7e81a5f9..db9acdef 100644 --- a/frontend/src/features/dashboard/hooks/use-download.ts +++ b/frontend/src/features/dashboard/hooks/use-download.ts @@ -40,18 +40,11 @@ async function withConcurrency( * row in the listing on every chunk of every download. */ export const useDownloadActions = () => { - const { error: showError, info } = useNotification(); + const { error: showError } = useNotification(); const { apiS3 } = useAuthGuard(); const startDownload = useDownloadStore((state) => state.startDownload); - const cancelInStore = useDownloadStore((state) => state.cancelDownload); - - const handleError = useCallback( - (_fileId: string, error: string) => { - showError(error); - }, - [showError] - ); + const cancelDownload = useDownloadStore((state) => state.cancelDownload); const downloadFile = useCallback( async (file: FileItem) => { @@ -61,30 +54,34 @@ export const useDownloadActions = () => { if (!apiS3) return; try { - await startDownload(apiS3, file, { onError: handleError }); + await startDownload(apiS3, file); } catch (error) { + /** + * The last download toast, and the only one worth keeping. + * + * Starting, cancelling and failing are all written to the operations + * card as they happen, so a toast saying the same thing was the same + * news twice, in two corners of the screen at once. + * + * This branch is different, and rare: the service reports its own + * failures through `onProgress` and never rejects, so the only way here + * is something throwing before it takes over - building the service for + * a new provider, say. That leaves no row on the card to read, and it is + * the one failure that would otherwise pass in silence. + */ showError(`Failed to download ${file.name}, ${error}`); } }, - [apiS3, startDownload, handleError, showError] + [apiS3, startDownload, showError] ); const downloadMultipleFiles = useCallback( async (files: FileItem[]) => { if (!apiS3 || files.length === 0) return; - info(`Downloading ${files.length} file${files.length > 1 ? 's' : ''}...`); await withConcurrency(files, MULTI_DOWNLOAD_CONCURRENCY, downloadFile); }, - [apiS3, downloadFile, info] - ); - - const cancelDownload = useCallback( - (fileId: string) => { - cancelInStore(fileId); - info('Download cancelled'); - }, - [cancelInStore, info] + [apiS3, downloadFile] ); return { downloadFile, downloadMultipleFiles, cancelDownload }; @@ -109,6 +106,13 @@ export const useIsFileDownloading = (fileId: string): boolean => export const useDownloadList = () => { const downloads = useDownloadStore((state) => state.downloads); const { cancelDownload } = useDownloadActions(); + /** + * Drops a settled row without touching the transfer, which is what a failed + * download needs: cancelling one is a no-op because there is nothing left in + * flight to abort, so the panels had no way to clear it. Selecting the action + * on its own is free - zustand action identities are stable. + */ + const removeDownload = useDownloadStore((state) => state.removeDownload); const downloadProgress = useMemo(() => Array.from(downloads.values()), [downloads]); const getAllDownloads = useCallback( @@ -116,5 +120,5 @@ export const useDownloadList = () => { [downloadProgress] ); - return { downloadProgress, getAllDownloads, cancelDownload }; + return { downloadProgress, getAllDownloads, cancelDownload, removeDownload }; }; diff --git a/frontend/src/features/dashboard/stores/use-download-store.test.ts b/frontend/src/features/dashboard/stores/use-download-store.test.ts index 8c08d0fc..a2f21de2 100644 --- a/frontend/src/features/dashboard/stores/use-download-store.test.ts +++ b/frontend/src/features/dashboard/stores/use-download-store.test.ts @@ -172,39 +172,33 @@ describe('startDownload', () => { expect(store().downloads.get('file-1')!.progress).toBe(25); }); - it('clears a completed download after the linger delay', async () => { - vi.useFakeTimers(); - const service = fakeService(); - service.downloadFile.mockImplementation(async (_f, opts) => { - opts.onComplete('file-1'); - }); - createDownloadServiceMock.mockReturnValue(service as never); - - store().setProgress(progress({ status: 'completed', progress: 100 })); - await store().startDownload(apiA, file); - - // The row stays briefly so the user sees it finish. - expect(store().downloads.has('file-1')).toBe(true); - vi.advanceTimersByTime(3000); - expect(store().downloads.has('file-1')).toBe(false); - }); - - it('clears a cancelled download sooner than a completed one', async () => { - vi.useFakeTimers(); - const service = fakeService(); - service.downloadFile.mockImplementation(async (_f, opts) => { - opts.onProgress(progress({ status: 'cancelled' })); - }); - createDownloadServiceMock.mockReturnValue(service as never); - - await store().startDownload(apiA, file); - - expect(store().downloads.has('file-1')).toBe(true); - vi.advanceTimersByTime(2000); - expect(store().downloads.has('file-1')).toBe(false); - }); - - it('does not schedule removal for ordinary progress updates', async () => { + it.each(['completed', 'cancelled', 'error'] as const)( + 'keeps a %s download on the list until it is removed', + async (status) => { + vi.useFakeTimers(); + const service = fakeService(); + service.downloadFile.mockImplementation(async (_f, opts) => { + opts.onProgress( + progress({ status, error: status === 'error' ? 'Network error' : undefined }) + ); + }); + createDownloadServiceMock.mockReturnValue(service as never); + + await store().startDownload(apiA, file); + + // Nothing takes it away on a timer. A row that removes itself is a + // reason for a failure that disappears before anyone reads it, and the + // panel it sits on can be collapsed at the time. Uploads and deletes + // have always waited to be dismissed. + vi.advanceTimersByTime(60_000); + expect(store().downloads.has('file-1')).toBe(true); + + store().removeDownload('file-1'); + expect(store().downloads.has('file-1')).toBe(false); + } + ); + + it('leaves an in-flight download on the list', async () => { vi.useFakeTimers(); const service = fakeService(); service.downloadFile.mockImplementation(async (_f, opts) => { @@ -213,9 +207,8 @@ describe('startDownload', () => { createDownloadServiceMock.mockReturnValue(service as never); await store().startDownload(apiA, file); - vi.advanceTimersByTime(10_000); + vi.advanceTimersByTime(60_000); - // An in-flight download must never disappear from the list on a timer. expect(store().downloads.has('file-1')).toBe(true); }); diff --git a/frontend/src/features/dashboard/stores/use-download-store.ts b/frontend/src/features/dashboard/stores/use-download-store.ts index f8263b51..46124a93 100644 --- a/frontend/src/features/dashboard/stores/use-download-store.ts +++ b/frontend/src/features/dashboard/stores/use-download-store.ts @@ -19,10 +19,6 @@ import { } from '../services/download-service'; import type { FileItem } from '../types/file'; -/** How long a finished row lingers before it clears itself from the list. */ -const COMPLETED_LINGER_MS = 3000; -const CANCELLED_LINGER_MS = 2000; - interface DownloadState { downloads: Map; /** @@ -77,19 +73,21 @@ export const useDownloadStore = create((set, get) => ({ return { downloads }; }), + /** + * A settled row stays until someone removes it. + * + * Downloads used to clear themselves on a timer - three seconds for a + * completion, two for a cancel - which read as tidy until a failure needed + * the same treatment. A reason for the failure that takes itself off the + * screen is no use to anyone who was not looking at that moment, and the + * panel can be collapsed. Uploads and deletes have always waited to be + * dismissed; downloads now do too, and every settled row has a button on it. + */ startDownload: async (api, file, handlers) => { - const { getService, setProgress, removeDownload } = get(); + const { getService, setProgress } = get(); await getService(api).downloadFile(file, { - onProgress: (progress) => { - setProgress(progress); - if (progress.status === 'cancelled') { - setTimeout(() => removeDownload(file.id), CANCELLED_LINGER_MS); - } - }, - onComplete: (fileId) => { - setTimeout(() => removeDownload(fileId), COMPLETED_LINGER_MS); - }, + onProgress: setProgress, onError: handlers?.onError, }); }, diff --git a/frontend/src/features/upload/components/operation-row.tsx b/frontend/src/features/upload/components/operation-row.tsx index 5c80efb7..45a6269e 100644 --- a/frontend/src/features/upload/components/operation-row.tsx +++ b/frontend/src/features/upload/components/operation-row.tsx @@ -167,8 +167,11 @@ const OperationRowInner: React.FC = ({

)} {status === 'downloading' && ( + // With the percentage, which is the one thing the separate download + // card said that this row did not. The ring beside it has always + // shown the same figure as a shape; this puts a number on it.

- Downloading... + Downloading... {Math.round(progress)}%

)} {status === 'pending' && ( @@ -375,7 +378,13 @@ const OperationRowInner: React.FC = ({
- ) : status === 'cancelled' ? ( + ) : status === 'cancelled' || status === 'error' || status === 'failed' ? ( + // A settled row that did not succeed, which until now meant no + // control at all: the chain ran active, then completed, then + // cancelled, and anything that had gone wrong fell past all three + // to null. So the one row a reader most wants rid of was the only + // one with nothing to press. ('failed' is what a delete reports; + // uploads and downloads say 'error'.)