feat(download): add re-download action from Completed + History - #102
Merged
Conversation
… (task 09) Cloning path: IPC command `download_redownload(sourceKind, sourceId, overwriteMode?)` creates a brand-new `Download` (new DownloadId) seeded from the source — URL, filename, destination, plus segments/priority/module/account when the source is an existing Download. Tagged outcome returns `fileExists` with a pre-resolved rename suggestion so the frontend can show the overwrite dialog without a second backend round trip to compute the suggestion. Frontend ships a reusable `<OverwriteDialog>` and a `useRedownload` hook mounted in `DownloadsTable` (Completed rows only) and `HistoryView`; both invalidate the download and history query keys on success.
📝 WalkthroughWalkthroughImplements a full "re-download" flow: new Tauri IPC command, backend RedownloadCommand and handler, domain builder extensions, frontend hook and overwrite dialog, integration into Downloads/History views and tests, plus new i18n keys and a changelog entry. Changes
Sequence DiagramsequenceDiagram
actor User
participant Frontend as Frontend UI
participant IPC as Tauri IPC
participant QueryBus as Query Bus
participant CommandBus as Command Bus
participant Repo as Repository
participant EventBus as Event Bus
User->>Frontend: Click "Redownload"
Frontend->>IPC: download_redownload(sourceKind, sourceId, overwriteMode=null)
IPC->>QueryBus: Resolve download or history template
QueryBus-->>IPC: Template data
IPC->>IPC: Compute destination path & check filesystem
alt collision detected
IPC-->>Frontend: FileExists(originalPath, suggestedPath)
Frontend->>Frontend: Show OverwriteDialog
User->>Frontend: Choose overwrite/rename
Frontend->>IPC: download_redownload(sourceKind, sourceId, overwriteMode)
end
IPC->>CommandBus: RedownloadCommand{source, destination_override}
CommandBus->>CommandBus: Create new Download (new ID) and apply metadata
CommandBus->>Repo: Save new Download
CommandBus->>EventBus: Publish DownloadCreated
EventBus-->>Frontend: Trigger query invalidation
IPC-->>Frontend: Created{id}
Frontend->>Frontend: Show success toast, close dialog
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR adds a re-download action to Completed rows in Confidence Score: 4/5Safe to merge with minor open concerns; two P1 findings from prior reviews (TOCTOU on rename path, cache invalidation on fileExists) are still unresolved. The implementation is solid and the previously-requested Completed-state guard has been added. The two remaining P1 concerns from earlier review rounds (TOCTOU where the user-confirmed suggested path can differ from the actual save path on a second unique_destination call; unnecessary query invalidation on fileExists) are carried forward unaddressed. All new findings in this pass are P2. Score is 4 rather than 5 because the TOCTOU is a present correctness issue where a user-confirmed path may silently diverge at write time. src-tauri/src/adapters/driving/tauri_ipc.rs (TOCTOU on rename), src/hooks/useRedownload.tsx (cache invalidation on fileExists)
|
| Filename | Overview |
|---|---|
| src-tauri/src/adapters/driving/tauri_ipc.rs | Adds download_redownload IPC: resolves source via query bus, checks file existence, calls unique_destination for rename suggestion/path. TOCTOU noted in previous review (suggested path computed twice, second call may return a different path). |
| src-tauri/src/application/commands/redownload.rs | New command handler: loads template from Download (now with Completed-state guard) or HistoryEntry, clones into a new Download with fresh ID, saves and emits DownloadCreated. Six unit tests cover happy path, event emission, destination override, history source, non-completed rejection, and not-found cases. |
| src/hooks/useRedownload.tsx | New hook wiring mutation → overwrite dialog → second mutation. invalidateKeys fires on both "created" and "fileExists" outcomes (noted in previous review). Returns a fresh object every render, causing instability in callers' dep arrays. |
| src/components/ui/OverwriteDialog.tsx | Reusable dialog with decidedRef guard to prevent double-firing on Escape + button click. Clean implementation with correct i18n keys. |
| src/views/DownloadsView/DownloadsTable.tsx | Adds redownload action for Completed rows; renders redownload.dialog above the table. rowActions useMemo lists the full redownload object as a dep instead of redownload.trigger, causing unnecessary context value churn on each render. |
| src/views/HistoryView/HistoryView.tsx | Integrates useRedownload with custom history toast keys; renders dialog in return tree. handleRedownload useCallback dep on full redownload object instead of redownload.trigger causes unnecessary recreations. |
| src-tauri/src/domain/model/download.rs | Adds three builder methods: with_segments_count, with_module_name, with_account_id. Straightforward field assignments with matching unit tests. |
Sequence Diagram
sequenceDiagram
participant UI as Frontend (trigger)
participant IPC as download_redownload IPC
participant QB as QueryBus
participant FS as Filesystem
participant CB as CommandBus
UI->>IPC: download_redownload(sourceKind, sourceId, null)
IPC->>QB: resolve source (GetDownloadDetail / GetHistoryEntry)
QB-->>IPC: destination_path
IPC->>FS: dest_path.exists()?
alt file does not exist
IPC->>CB: handle_redownload(source, None)
CB-->>IPC: new DownloadId
IPC-->>UI: { kind: created, id }
UI->>UI: toast.success + invalidate queries
else file exists, no overwriteMode
FS-->>IPC: true
IPC->>FS: unique_destination(dir, file_name)
FS-->>IPC: suggestedPath
IPC-->>UI: { kind: fileExists, originalPath, suggestedPath }
UI->>UI: show OverwriteDialog
UI->>IPC: download_redownload(sourceKind, sourceId, overwrite or rename)
IPC->>FS: unique_destination again (rename path)
IPC->>CB: handle_redownload(source, destination_override)
CB-->>IPC: new DownloadId
IPC-->>UI: { kind: created, id }
UI->>UI: toast.success + invalidate queries
end
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/views/DownloadsView/DownloadsTable.tsx
Line: 408
Comment:
The entire `redownload` object (a new reference on every render because the hook creates a fresh JSX `dialog` element inline) is listed as a dep of `rowActions`. This makes `rowActions` regenerate on every parent render, which means `RowActionsContext.value` is always a new object and every `useRowActions()` consumer re-renders unconditionally. For a virtualized table that re-renders frequently during active downloads, this produces continuous unnecessary work. Only the stable `trigger` function is actually used in the memo body, so narrowing the dep is both correct and sufficient.
```suggestion
[pauseMut, resumeMut, retryMut, removeMut, priorityMut, openFileMut, openFolderMut, redownload.trigger],
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/views/HistoryView/HistoryView.tsx
Line: 72-77
Comment:
Same object-reference instability as in `DownloadsTable`: `redownload` is a new object every render, so `handleRedownload` (and through it `rowActions`) is recreated on every render. Only `redownload.trigger` is actually called inside, so it is the correct dep.
```suggestion
const handleRedownload = useCallback(
(entry: HistoryEntry) => {
redownload.trigger("history", entry.entryId);
},
[redownload.trigger],
);
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (2): Last reviewed commit: "fix(download): address PR #102 review co..." | Re-trigger Greptile
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/views/HistoryView/__tests__/HistoryView.test.tsx (1)
62-66:⚠️ Potential issue | 🟡 MinorStabilize locale-dependent assertions in test setup.
These tests rely on English UI labels but don’t force locale inbeforeEach, which can make runs order-dependent.Suggested test setup tweak
beforeEach(() => { + window.localStorage.setItem("i18nextLng", "en"); mockInvoke.mockReset(); mockSave.mockReset(); mockToastSuccess.mockClear(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/HistoryView/__tests__/HistoryView.test.tsx` around lines 62 - 66, The tests rely on English UI labels but don't force the locale, making them order-dependent; update the beforeEach block to explicitly set the test locale to English (for example by calling your i18n locale setter or mocking navigator.language / Intl locale utilities) before resetting mocks so the UI labels/assertions are stable; reference the existing beforeEach, mockInvoke, mockSave, and mockToastSuccess to locate where to add the locale-forcing call.
🧹 Nitpick comments (1)
src/components/ui/OverwriteDialog.tsx (1)
31-44: Potential doubleonDecision("cancel")callback.When the Cancel button is clicked,
handle("cancel")callsonDecision("cancel")and thenonOpenChange(false). This triggers theDialog'sonOpenChangecallback (lines 39-44), which callsonDecision("cancel")again whennextisfalse.The current consumer (
useRedownload) handles this gracefully becausehandleDecisionchecksif (!pending) return;and clearspendingimmediately, so the second call is a no-op. However, this is fragile coupling—future consumers may not expect a double callback.Consider guarding against this by tracking whether a decision has been emitted:
♻️ Suggested guard to prevent double callback
export function OverwriteDialog({ open, onOpenChange, originalPath, suggestedPath, onDecision, }: OverwriteDialogProps) { const { t } = useTranslation(); + const decidedRef = useRef(false); + + // Reset decided state when dialog opens + useEffect(() => { + if (open) decidedRef.current = false; + }, [open]); const handle = (decision: OverwriteDecision) => { + if (decidedRef.current) return; + decidedRef.current = true; onDecision(decision); onOpenChange(false); }; return ( <Dialog open={open} onOpenChange={(next) => { if (!next) { - onDecision("cancel"); + if (!decidedRef.current) { + decidedRef.current = true; + onDecision("cancel"); + } } onOpenChange(next); }} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/OverwriteDialog.tsx` around lines 31 - 44, The component currently calls onDecision("cancel") both in handle and again in the Dialog onOpenChange, causing a possible double emit; add a local guard (e.g., a ref or state like decisionEmitted) and set it when calling onDecision from handle (or from the onOpenChange path) and check that guard before calling onDecision inside the Dialog onOpenChange callback so onDecision is only invoked once; update the handle function and the Dialog onOpenChange closure (referencing handle, onDecision, onOpenChange, and Dialog) to use that guard and reset it appropriately if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/views/HistoryView/HistoryView.tsx`:
- Around line 72-76: The current handleRedownload uses Number(entry.entryId)
which can lose precision for 64-bit IDs; either (preferred) update the backend
command download_redownload to accept source_id: String (like
history_delete_entry) and parse to u64 server-side, or (frontend fallback) stop
converting to Number in handleRedownload and pass the ID as a string (invoke
redownload.trigger("history", entry.entryId)) after validating with BigInt/safer
checks to ensure it's a valid u64; reference handleRedownload,
redownload.trigger, entry.entryId, download_redownload, and history_delete_entry
when making the change.
---
Outside diff comments:
In `@src/views/HistoryView/__tests__/HistoryView.test.tsx`:
- Around line 62-66: The tests rely on English UI labels but don't force the
locale, making them order-dependent; update the beforeEach block to explicitly
set the test locale to English (for example by calling your i18n locale setter
or mocking navigator.language / Intl locale utilities) before resetting mocks so
the UI labels/assertions are stable; reference the existing beforeEach,
mockInvoke, mockSave, and mockToastSuccess to locate where to add the
locale-forcing call.
---
Nitpick comments:
In `@src/components/ui/OverwriteDialog.tsx`:
- Around line 31-44: The component currently calls onDecision("cancel") both in
handle and again in the Dialog onOpenChange, causing a possible double emit; add
a local guard (e.g., a ref or state like decisionEmitted) and set it when
calling onDecision from handle (or from the onOpenChange path) and check that
guard before calling onDecision inside the Dialog onOpenChange callback so
onDecision is only invoked once; update the handle function and the Dialog
onOpenChange closure (referencing handle, onDecision, onOpenChange, and Dialog)
to use that guard and reset it appropriately if needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4ae05f4e-e068-4905-940a-253508519d10
📒 Files selected for processing (15)
CHANGELOG.mdsrc-tauri/src/adapters/driving/tauri_ipc.rssrc-tauri/src/application/commands/mod.rssrc-tauri/src/application/commands/redownload.rssrc-tauri/src/domain/model/download.rssrc-tauri/src/lib.rssrc/components/__tests__/OverwriteDialog.test.tsxsrc/components/ui/OverwriteDialog.tsxsrc/hooks/useRedownload.tsxsrc/i18n/locales/en.jsonsrc/i18n/locales/fr.jsonsrc/views/DownloadsView/DownloadsTable.tsxsrc/views/DownloadsView/__tests__/DownloadsTable.test.tsxsrc/views/HistoryView/HistoryView.tsxsrc/views/HistoryView/__tests__/HistoryView.test.tsx
There was a problem hiding this comment.
2 issues found across 15 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/views/HistoryView/HistoryView.tsx">
<violation number="1" location="src/views/HistoryView/HistoryView.tsx:74">
P1: Converting `entry.entryId` (string u64) with `Number(...)` can lose precision and send an incorrect history ID to redownload.</violation>
</file>
<file name="src-tauri/src/application/commands/redownload.rs">
<violation number="1" location="src-tauri/src/application/commands/redownload.rs:57">
P2: Validate that `RedownloadSource::Download` is in `Completed` state before cloning; the new handler currently accepts any download state.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Reject non-completed downloads in `handle_redownload` with `AppError::Validation`, mirroring `open_download_file` guard. Covered by new unit test. - Change `download_redownload` IPC to accept `source_id: String` and parse it server-side (like `history_delete_entry`), eliminating precision loss on 64-bit IDs that would slip through `Number(entry.entryId)`. - `useRedownload` trigger now takes `sourceId: string`; callers in `DownloadsTable` and `HistoryView` pass the ID through without `Number()` conversion. - `OverwriteDialog` guards `onDecision` with a ref so Cancel via button + Radix close callback can't double-emit. - HistoryView test forces `i18nextLng=en` in beforeEach for stable label matching.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/hooks/useRedownload.tsx (1)
35-44: JSDoc example is inconsistent with the actual type signature.The JSDoc example at Line 43 shows
trigger('download', Number(id)), but the actualtriggerfunction signature at Line 83 expectssourceId: string. This could lead to confusion since callers might convert IDs to numbers when they should pass strings directly.📝 Update JSDoc to match the string signature
/** * Redownload trigger paired with the overwrite dialog. * * Usage: * const { trigger, dialog } = useRedownload(); * // ... somewhere in JSX: * <>{dialog}</> * // ... somewhere in a handler: - * trigger('download', Number(id)); + * trigger('download', id); // id should be a string */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useRedownload.tsx` around lines 35 - 44, The JSDoc example for useRedownload is inconsistent with the trigger signature; update the example so callers pass a string for sourceId (matching trigger(source: 'download' | 'upload', sourceId: string)) instead of Number(id) — e.g., use trigger('download', id.toString()) or simply trigger('download', id) if id is already a string; ensure the example shows useRedownload, trigger, and the string sourceId to match the actual function signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/hooks/useRedownload.tsx`:
- Around line 35-44: The JSDoc example for useRedownload is inconsistent with
the trigger signature; update the example so callers pass a string for sourceId
(matching trigger(source: 'download' | 'upload', sourceId: string)) instead of
Number(id) — e.g., use trigger('download', id.toString()) or simply
trigger('download', id) if id is already a string; ensure the example shows
useRedownload, trigger, and the string sourceId to match the actual function
signature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 654566d9-e25b-48f6-af4f-35c03028671a
📒 Files selected for processing (8)
src-tauri/src/adapters/driving/tauri_ipc.rssrc-tauri/src/application/commands/redownload.rssrc/components/ui/OverwriteDialog.tsxsrc/hooks/useRedownload.tsxsrc/views/DownloadsView/DownloadsTable.tsxsrc/views/DownloadsView/__tests__/DownloadsTable.test.tsxsrc/views/HistoryView/HistoryView.tsxsrc/views/HistoryView/__tests__/HistoryView.test.tsx
✅ Files skipped from review due to trivial changes (1)
- src-tauri/src/application/commands/redownload.rs
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
• IPC command
download_redownload(sourceKind, sourceId, overwriteMode?)creates new Download from Completed row or History entry with fresh DownloadId• Clones URL, filename, destination; Download source also preserves segments, priority, module_name, account_id
• Tagged outcome returns
fileExistswith pre-resolved rename suggestion viaunique_destinationhelper — no second backend call needed• Reusable
<OverwriteDialog>component +useRedownloadhook for frontend (DownloadsTable + HistoryView)• Domain Download gains builder methods:
with_segments_count,with_module_name,with_account_id• i18n: new keys
common.overwriteDialog.*+downloads.table.{actions.redownload, toast.redownload*}(en/fr)• Invalidates download + history query keys on success for cache coherence
• Backend: 6 unit tests (command handler), Frontend: 10 RTL tests (component + integration)
Type
feat
Summary by cubic
Add a Re-download action to Completed and History so users can quickly re-add a past download with a simple overwrite/rename flow. Aligns with Task 09; only completed downloads are eligible, and file conflicts are handled without extra backend calls.
New Features
download_redownload(sourceKind, sourceId, overwriteMode?)creates a new DownloadId and clones URL, filename, and destination; when cloning a download it also preserves segments, priority, module name, and account id.{ kind: "created", id }or{ kind: "fileExists", originalPath, suggestedPath }with the rename suggestion computed server-side.OverwriteDialoganduseRedownload, wired into Completed rows inDownloadsTableand intoHistoryView; invalidates downloads and history queries on success.with_segments_count,with_module_name,with_account_id; i18n:common.overwriteDialog.*anddownloads.table.{actions.redownload,toast.redownload*}(en/fr).Bug Fixes
sourceIdas a string to avoid 64‑bit precision issues;useRedownloadand callers pass string IDs through.OverwriteDialogprevents double-emitting decisions when closing via cancel actions.Written for commit 1842f73. Summary will update on new commits.
Summary by CodeRabbit
New Features
Localization
Tests
Documentation