Add download manager with realtime updates #204
Conversation
Introduces a new DownloadManager component in the status bar to track scenario downloads in the background. Refactors DownloadForm to initiate downloads via a new Zustand-based downloadStore, which manages download state, progress, and socket events. Users can now monitor, download, and delete prepared downloads from a unified interface.
Track downloading items locally in DownloadManager to provide accurate loading indicators and prevent duplicate download actions. Update DownloadItem to use the new isDownloading prop for button loading state. In downloadStore, add a status check before triggering file download to ensure the file is ready and provide better error handling.
|
Warning Rate limit exceeded@reyery has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis PR replaces inline XHR download logic with a Zustand-backed download store, adds a DownloadManager UI (socket-driven) to the status bar, and updates DownloadForm to call the store's prepareDownload flow and use isSubmitting UI states. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DF as DownloadForm
participant Store as DownloadStore
participant API
participant Socket
participant DM as DownloadManager
User->>DF: Submit form
DF->>Store: prepareDownload(project, scenarios, inputFiles, outputFiles)
Store->>API: POST /downloads/prepare
API-->>Store: download object (PENDING)
Store->>Store: upsertDownload(download)
Note right of Socket: server emits lifecycle events
Socket->>Store: progress / ready / error / downloaded
Store->>Store: upsertDownload(update)
DM->>Store: fetchDownloads() / initializeSocketListeners()
DM->>User: show badge, list, progress
User->>DM: click "Download" (when READY)
DM->>Store: downloadFile(downloadId)
Store->>API: GET /downloads/:id/file (validate/serve)
API-->>User: file blob (browser download)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Removes the mapping of outputFiles to uppercase before passing to prepareDownload. Now, outputFiles are sent as provided by the form values.
Updated DownloadManager to display the actual progress percent during the PREPARING state, using the new progress_percent field from the download store. The store now tracks and updates progress_percent for each download.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/features/upload-download/stores/downloadStore.js (1)
93-102: Consider improving anchor element cleanup.The anchor element cleanup is only in
setTimeout. While unlikely, if an error occurs after appending but before the timeout, the element won't be removed. Consider moving the cleanup to afinallyblock or ensuring it's always removed.Apply this diff to ensure cleanup:
const a = document.createElement('a'); a.href = downloadUrl; a.style.display = 'none'; - document.body.appendChild(a); - a.click(); - - // Cleanup - setTimeout(() => { + try { + document.body.appendChild(a); + a.click(); + } finally { + // Cleanup after a brief delay to ensure download starts + setTimeout(() => { + if (a.parentNode) { + document.body.removeChild(a); + } + }, 100); - document.body.removeChild(a); - }, 100); + }src/features/upload-download/components/DownloadManager.jsx (2)
27-31: Use the store'sgetDownloadsArraymethod to avoid duplication.The sorting logic here duplicates the
getDownloadsArraymethod from the store (lines 27-32 in downloadStore.js). Using the store method ensures consistency and reduces code duplication.Apply this diff:
+ const getDownloadsArray = useDownloadStore( + (state) => state.getDownloadsArray, + ); + - // Convert downloads map to array, sorted by creation date - const downloads = Object.values(downloadsMap).sort((a, b) => { - const dateA = a.created_at ? new Date(a.created_at) : new Date(0); - const dateB = b.created_at ? new Date(b.created_at) : new Date(0); - return dateB - dateA; - }); + // Get downloads as sorted array + const downloads = getDownloadsArray();
48-69: Consider removing the arbitrary 1-second delay.The
setTimeoutwith a fixed 1-second delay to clear the loading state is arbitrary and may not reflect the actual download state. Since you have socket events (download-started,download-downloaded), consider relying on those events to update the UI state instead, or simply remove the downloading tracking since the download state is already managed by the store.If you want to simplify, you could remove the local
downloadingIdsstate entirely and disable the button based on the download state from the store:- const [downloadingIds, setDownloadingIds] = useState(new Set()); ... const handleDownload = async (downloadId) => { - // Add to downloading set - setDownloadingIds((prev) => new Set([...prev, downloadId])); - try { await downloadFile(downloadId); message.success('Download started'); } catch (error) { const errorMessage = error.response?.data?.detail || 'Failed to download file'; message.error(errorMessage); - } finally { - // Remove from downloading set after a short delay - setTimeout(() => { - setDownloadingIds((prev) => { - const newSet = new Set(prev); - newSet.delete(downloadId); - return newSet; - }); - }, 1000); } };Then in DownloadItem, remove the
isDownloadingprop and simply rely on the download state.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/features/upload-download/components/DownloadForm.jsx(7 hunks)src/features/upload-download/components/DownloadManager.jsx(1 hunks)src/features/upload-download/stores/downloadStore.js(1 hunks)
🔇 Additional comments (11)
src/features/upload-download/stores/downloadStore.js (4)
1-32: LGTM! Clean store initialization and basic operations.The Zustand store setup, upsert/remove operations, and array conversion with sorting are well-implemented.
35-71: LGTM! API operations are well-structured.Both
prepareDownloadandfetchDownloadshave proper error handling and state updates.
112-121: LGTM! Delete operation is correctly implemented.
124-222: LGTM! Socket listeners are well-implemented with defensive checks.All socket event handlers properly check for existing downloads before updating, include debug logging in dev mode, and have matching cleanup. The defensive programming approach prevents errors from unexpected events.
src/features/upload-download/components/DownloadManager.jsx (3)
34-46: LGTM! Proper initialization and cleanup.Socket listeners are correctly initialized on mount and cleaned up on unmount. The active downloads count is computed efficiently.
71-127: LGTM! Clean rendering and user feedback.The delete handler, empty state, and popover UI are well-implemented with appropriate user feedback via messages.
129-250: LGTM! DownloadItem component is well-structured.Good separation of concerns with clear visual states, appropriate icons, and conditional rendering based on download state. The progress bar integration for the PREPARING state enhances user feedback.
src/features/upload-download/components/DownloadForm.jsx (4)
1-23: LGTM! Good UX improvement with clear user guidance.The updated description effectively communicates the background preparation flow and where users can track progress.
96-107: LGTM! Good defensive check and improved state naming.The project validation check prevents errors, and
isSubmittingis more semantically clear than the previousdisableFormstate.
109-130: LGTM! Clean refactor to use the store.The submit handler is well-structured with proper error handling, success feedback, and form reset. The migration from inline download logic to the store-backed flow is cleanly implemented.
134-220: LGTM! UI updates are consistent with the new flow.The button label change to "Prepare Download" and consistent use of
isSubmittingthroughout all form controls properly reflect the background preparation model.
Enhanced the error message logic to handle cases where the error detail is an object with 'message' and optional 'suggestion' fields, providing more informative feedback to users.
Updated the download logic to fetch a pre-signed URL from the API before triggering the file download. This enhances security and ensures the download link expires after a set period.
Refactors DownloadManager to use a ref for managing the timeout that removes download IDs from the downloading set. Ensures timeouts are cleared on component unmount and before setting new ones, preventing potential memory leaks or unexpected behavior.
Updated the download URL construction to check if the provided URL is already absolute (e.g., S3 presigned URL). This prevents incorrect URL concatenation and ensures downloads work for both absolute and relative URLs.
Moved the cleanupSocketListeners function above initializeSocketListeners and ensured it is called before adding new listeners to prevent duplicate event handlers on reconnect. This improves socket event management and prevents potential memory leaks.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/features/upload-download/stores/downloadStore.js (1)
74-120: Verify pre-signed URL approach handles authentication correctly.While the code now uses a pre-signed URL pattern (
/api/downloads/${downloadId}/url), the anchor element approach from the past review comment may still have issues:
- Cross-origin in dev: If frontend and backend are on different ports, the anchor element won't send cookies unless the pre-signed URL includes auth in query parameters
- No error handling: If the download fails (expired URL, network error), users get no feedback
- Token refresh: Cannot use axios interceptors if the token expires between getting the URL and clicking
If the
/urlendpoint returns a fully authenticated pre-signed URL (e.g., S3 presigned URL with auth in query params), this approach works. Otherwise, the past review's suggestion to use fetch with credentials and blob URL is correct.Run the following script to verify the URL endpoint implementation:
#!/bin/bash # Description: Check if the /url endpoint returns pre-signed URLs with embedded auth # Search for download URL endpoint implementation rg -n "downloads.*url|presigned.*url|signed.*url" --type py -A 10 # Check for S3 or cloud storage integration rg -n "boto3|s3|presign|generate_presigned_url" --type py -C 5
🧹 Nitpick comments (2)
src/features/upload-download/components/DownloadForm.jsx (1)
104-107: Consider validating project selection earlier.The project validation occurs after form validation passes, meaning users can fill out the entire form before discovering no project is selected. Consider adding a Form.Item validator or disabling the form entirely when
!currentProject.Apply this diff to add form-level validation:
const onFinish = async (values) => { if (!values.scenarios.length) return; - if (!currentProject) { - message.error('No project selected'); - return; - } setIsSubmitting(true); try {And add this at the form level (after line 144):
<Form form={form} onFinish={onFinish}> {!currentProject && ( <div style={{ marginBottom: 12, padding: 12, background: '#fff7e6', border: '1px solid #ffd666', borderRadius: 4 }}> ⚠️ No project selected. Please select a project before downloading. </div> )} <Button type="primary" htmlType="submit" loading={isSubmitting} disabled={!currentProject}>src/features/upload-download/stores/downloadStore.js (1)
159-172: Guard against race conditions with missing downloads.All socket event handlers (except
download-created) check forexistingDownloadbut silently ignore events when the download isn't in state. This could occur if:
- Socket events arrive before the initial
prepareDownloadAPI response- User loads the page and socket events fire before
fetchDownloadscompletes- Socket reconnects after state is cleared
Consider fetching the download details if it's missing, or at minimum log a warning to aid debugging:
Apply this pattern to each handler (example for
download-progress):socket.on('download-progress', (data) => { if (import.meta.env.DEV) { console.log('download-progress:', data); } const existingDownload = get().downloads[data.download_id]; if (existingDownload) { get().upsertDownload({ ...existingDownload, state: data.state, progress_message: data.progress_message, progress_percent: data.progress_percent, }); + } else { + console.warn(`Received download-progress for unknown download ${data.download_id}`); + // Optionally: fetch the download details + // get().fetchDownloads(); } });Also applies to: 175-188, 191-202, 205-218, 221-233
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/features/upload-download/components/DownloadForm.jsx(7 hunks)src/features/upload-download/components/DownloadManager.jsx(1 hunks)src/features/upload-download/stores/downloadStore.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/features/upload-download/components/DownloadManager.jsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/features/upload-download/stores/downloadStore.js (2)
src/features/upload-download/components/DownloadManager.jsx (3)
download(143-151)downloads(28-32)downloadsMap(16-16)src/lib/socket.js (1)
socket(4-14)
🔇 Additional comments (9)
src/features/upload-download/components/DownloadForm.jsx (4)
1-1: LGTM!The import and description changes correctly reflect the new download preparation flow with background processing and status bar tracking.
Also applies to: 5-5, 14-17
98-98: LGTM!Replacing progress-based disable logic with
isSubmittingstate simplifies the component and clearly communicates the form's submission state.Also applies to: 100-100
122-140: LGTM!The error handling comprehensively addresses different error response formats and includes suggestion text for better user guidance.
145-146: LGTM!The UI updates consistently apply
isSubmittingstate across all form controls and the button text accurately reflects the new background preparation flow.Also applies to: 166-166, 183-183, 198-198, 231-231
src/features/upload-download/stores/downloadStore.js (5)
35-51: LGTM!The
prepareDownloadmethod correctly posts to the API, upserts the returned download, and properly handles errors by logging and rethrowing.
54-71: LGTM!The
fetchDownloadsmethod correctly retrieves downloads and converts the array response to the map structure expected by the store.
123-132: LGTM!The
deleteDownloadmethod correctly deletes via API and removes from local state with proper error handling.
135-142: LGTM!The cleanup method correctly removes all socket event listeners to prevent memory leaks and duplicate handlers.
151-156: LGTM!The
download-createdhandler correctly upserts without checking for existing, as this event signals a new download creation.
Introduces a disabled 'Downloaded' button with a tooltip for files that have already been downloaded, providing clearer user feedback in the DownloadManager component.
Updated sorting logic in DownloadManager and downloadStore to safely handle cases where the created_at property may be missing or undefined, preventing potential errors during sorting.
Removed custom styles and icon from the Downloaded button in DownloadManager. The button now uses default styling for a cleaner and more consistent appearance.
Added display of 'Created' and 'Downloaded' timestamps for each download item in the DownloadManager component. Introduced a date formatting helper and updated the UI to conditionally show these fields based on download state and data availability.
The background color logic for 'READY' and 'DOWNLOADED' download states has been removed, defaulting them to white. This simplifies the UI color scheme for download items.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/features/upload-download/components/DownloadManager.jsx (4)
44-58: Add loading state during initial download fetch.The component fetches downloads on mount but provides no loading feedback. If the API is slow, users see an empty list (or stale data) without indication that data is loading. Consider adding a loading state and displaying a skeleton or spinner in the popover content.
Apply this pattern:
const [isOpen, setIsOpen] = useState(false); + const [isLoading, setIsLoading] = useState(true); const [downloadingIds, setDownloadingIds] = useState(new Set()); useEffect(() => { initializeSocketListeners(); - fetchDownloads().catch((error) => { + fetchDownloads() + .catch((error) => { console.error('Failed to fetch downloads:', error); - }); + }) + .finally(() => setIsLoading(false)); return () => { // ... cleanup }; }, [initializeSocketListeners, cleanupSocketListeners, fetchDownloads]);Then in the popover content:
{isLoading ? ( <Spin /> ) : downloads.length === 0 ? ( <Empty description="No downloads yet" /> ) : ( // ... existing list )}
60-61: Reconsider whether ERROR state downloads should count as "active".The badge shows downloads where
state !== 'DOWNLOADED', which includes ERROR state downloads. From a UX perspective, failed downloads might not feel "active" to users, potentially causing confusion about the badge count. Consider whether ERROR downloads should be excluded or visually distinguished.If ERROR downloads should not count as active:
- const activeDownloads = downloads.filter((d) => d.state !== 'DOWNLOADED'); + const activeDownloads = downloads.filter( + (d) => d.state !== 'DOWNLOADED' && d.state !== 'ERROR' + );
167-175: Log date parsing errors for debugging.The
formatDatefunction silently catches and swallows errors, making it difficult to debug invalid date formats from the backend.const formatDate = (dateString) => { if (!dateString) return null; try { const date = new Date(dateString); return date.toLocaleString(); } catch (error) { + console.warn('Invalid date format:', dateString, error); return null; } };
278-286: Consider disabling delete during active operations.The delete button is hidden only when
isDownloadingis true, but it remains visible (and clickable) during PREPARING, PENDING, or other in-flight states. Deleting a download while it's being prepared might lead to race conditions or orphaned resources on the backend.Disable the delete button during non-terminal states:
- {!isDownloading && ( + {!isDownloading && !['PREPARING', 'PENDING'].includes(state) && ( <Button type="text" size="small" danger icon={<DeleteOutlined />} onClick={() => onDelete(id)} /> )}Or disable rather than hide:
<Button type="text" size="small" danger icon={<DeleteOutlined />} + disabled={isDownloading || ['PREPARING', 'PENDING'].includes(state)} onClick={() => onDelete(id)} />src/features/upload-download/stores/downloadStore.js (1)
161-235: Consider extracting repetitive socket handler logic.The six socket event handlers (
download-progress,download-ready,download-started,download-error,download-downloaded) follow an identical pattern: dev logging, existence check, and partial upsert. This repetition increases maintenance burden and the risk of inconsistent updates.Extract a helper function:
const createSocketHandler = (eventName, updateFields) => (data) => { if (import.meta.env.DEV) { console.log(`${eventName}:`, data); } const existingDownload = get().downloads[data.download_id]; if (existingDownload) { get().upsertDownload({ ...existingDownload, ...updateFields(data), }); } };Then simplify handler registration:
socket.on('download-progress', createSocketHandler('download-progress', (data) => ({ state: data.state, progress_message: data.progress_message, progress_percent: data.progress_percent, }))); socket.on('download-ready', createSocketHandler('download-ready', (data) => ({ state: data.state, file_size_mb: data.file_size_mb, progress_message: data.progress_message, }))); // ... etc
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/features/upload-download/components/DownloadManager.jsx(1 hunks)src/features/upload-download/stores/downloadStore.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/features/upload-download/stores/downloadStore.js (2)
src/features/upload-download/components/DownloadManager.jsx (3)
download(152-162)downloads(37-41)downloadsMap(25-25)src/lib/socket.js (1)
socket(4-14)
🔇 Additional comments (2)
src/features/upload-download/stores/downloadStore.js (1)
1-240: LGTM: Well-structured Zustand store with comprehensive socket integration.The store provides a clean API for download management with:
- Proper immutable state updates
- Comprehensive error handling with logging
- Defensive coding in
getDownloadsArray(created_at fallback)- Thorough socket event coverage for real-time updates
- Appropriate cleanup lifecycle management
The existing past review comments address the main concerns (authentication for direct downloads). Once those are resolved, this implementation will be solid.
src/features/upload-download/components/DownloadManager.jsx (1)
248-252: Add null safety to file size formatting.
file_size_mb.toFixed(2)will throw a TypeError iffile_size_mbisnullorundefined. While the condition checks for truthiness, it's safer to use optional chaining for clarity and defensive coding.- {file_size_mb && ( + {file_size_mb != null && ( <div style={{ fontSize: 12, color: '#666' }}> Size: {file_size_mb.toFixed(2)} MB </div> )}Likely an incorrect or invalid review comment.
Introduces a 10-second timeout for socket connection in the download store to prevent orphaned callbacks and improve error handling. Adds a removeConnectionCallback helper in socket.js to allow removal of pending connection callbacks if the connection does not succeed in time.
Removes unnecessary dependencies from the DownloadManager useEffect and eliminates redundant socket listener cleanup in downloadStore. This prevents premature cleanup and ensures socket listeners are managed correctly.
Summary by CodeRabbit