fix: show move/trash progress as a count-up toward the batch total#29
fix: show move/trash progress as a count-up toward the batch total#29f4nu wants to merge 4 commits into
Conversation
The header pinned its count to BULK_OP_CONCURRENCY (3) items in flight for the whole batch, so moving/trashing dozens of items looked stuck at "3" the entire time even though the queue was draining correctly behind the scenes. Threads the real batch size through to MoveProgress so the header can show "3 of 76 items" instead.
📝 WalkthroughWalkthroughBulk move and trash operations now propagate batch totals through tracked progress. The transfer widget displays completed item counts against the full batch size, while single-file trash operations seed item progress for accurate completion reporting. ChangesBatch progress reporting
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant RemoteBrowser
participant BrowserService
participant runTracked
participant TransferWidget
RemoteBrowser->>BrowserService: start bulk move or trash with batchTotal
BrowserService->>runTracked: forward batchTotal
runTracked->>TransferWidget: publish MoveProgress
TransferWidget-->>RemoteBrowser: display completed count of batch total
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/services/appServices.ts (1)
452-467: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTrack file deletions to display bulk trash progress in the widget.
Currently,
browser.deletebypassesrunTrackedfor files. This means that if a user bulk-deletes files to the Trash,batchTotalis discarded and the operation won't appear in the transfer widget. In contrast,browser.movecorrectly wraps both files and folders inrunTracked.Wrap the file deletion path in
runTrackedso that the start and completion events are captured, and the batch count is properly displayed in the UI.🛠️ Proposed fix to unify tracking
delete: async (connectionId: string, key: string, batchTotal?: number): Promise<void> => { const { client, conn } = await this.getClient(connectionId); const deletedAtMs = Date.now(); - if (isFolderKey(key)) { - await this.runTracked( - connectionId, - key, - key, - "trash", - (emit) => s3MoveFolderToTrash(client, conn.bucket, key, deletedAtMs, emit), - batchTotal, - ); - } else { - await s3MoveFileToTrash(client, conn.bucket, key, deletedAtMs); - } + await this.runTracked( + connectionId, + key, + key, + "trash", + async (emit) => { + if (isFolderKey(key)) { + await s3MoveFolderToTrash(client, conn.bucket, key, deletedAtMs, emit); + } else { + await s3MoveFileToTrash(client, conn.bucket, key, deletedAtMs); + } + }, + batchTotal, + ); },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/appServices.ts` around lines 452 - 467, Update the file branch of browser.delete to invoke runTracked, matching the folder branch, with connectionId, key, the "trash" operation, and a callback that calls s3MoveFileToTrash with the existing client, bucket, key, and deletedAtMs. Pass batchTotal so bulk file deletions emit start and completion events and retain their batch count.src/ui/RemoteBrowser.tsx (1)
577-591: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winHandle bulk trash errors per-item to prevent UI desync.
Because
mapWithConcurrencythrows on the first error, the currenttry-catchblock aborts the entire batch midway and callsrollback(). However, because it misses arefreshSilently()in the catch block, any items that were successfully moved to the Trash before the failure occurred will be wrongly restored to the UI, causing a permanent state desync and 404s on subsequent interactions.Refactoring this to match the
handleMovepattern (catching errors per-item and evaluatinganyFailedat the end) guarantees thatrefreshSilently()is always called, which resyncs the UI with the true state on the server.🔄 Proposed fix to synchronize state and unify error handling
- try { - const batchTotal = deleted.length; - await mapWithConcurrency(deleted, BULK_OP_CONCURRENCY, (entry) => - services.browser.delete(connectionId, entry.key, batchTotal), - ); - for (const entry of deleted) invalidateForKey(connectionId, entry.key); - await refreshSilently(); - } catch (err) { - rollback(); - toasts.add({ - variant: "error", - title: "Couldn't move to Trash", - description: err instanceof Error ? err.message : "Something went wrong.", - }); - } + let anyFailed = false; + const batchTotal = deleted.length; + void mapWithConcurrency(deleted, BULK_OP_CONCURRENCY, async (entry) => { + try { + await services.browser.delete(connectionId, entry.key, batchTotal); + } catch (err) { + anyFailed = true; + toasts.add({ + variant: "error", + title: "Couldn't move to Trash", + description: err instanceof Error ? err.message : "Something went wrong.", + }); + } + }).then(() => { + if (anyFailed) rollback(); + for (const entry of deleted) invalidateForKey(connectionId, entry.key); + void refreshSilently(); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/RemoteBrowser.tsx` around lines 577 - 591, Update the bulk-delete flow around mapWithConcurrency to catch errors per entry, track whether any deletion failed, and allow all items to be attempted instead of aborting on the first error. Match the existing handleMove error-handling pattern, rolling back only when appropriate, showing the error notification for failures, and always calling refreshSilently() so the UI reflects the server state.
🧹 Nitpick comments (1)
src/ui/TransferWidget.tsx (1)
387-389: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify unreachable pluralization check.
Since
nis guaranteed to be at least1(enforced by thep.movingMoves.length > 0guard block), the conditionbatchTotal > nensures thatbatchTotalis at least2. Thus,batchTotal === 1is strictly impossible in the true branch, and we can safely default to the plural"items".🧹 Proposed refactor to simplify the string
return batchTotal > n - ? `${moveVerb} ${n} of ${batchTotal} item${batchTotal === 1 ? "" : "s"}…` + ? `${moveVerb} ${n} of ${batchTotal} items…` : `${moveVerb} ${n} item${n === 1 ? "" : "s"}…`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/TransferWidget.tsx` around lines 387 - 389, Update the true branch of the move-label expression in the TransferWidget formatting logic to use the plural “items” directly, removing the unreachable batchTotal === 1 check. Preserve the existing singular/plural handling in the false branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/services/appServices.ts`:
- Around line 452-467: Update the file branch of browser.delete to invoke
runTracked, matching the folder branch, with connectionId, key, the "trash"
operation, and a callback that calls s3MoveFileToTrash with the existing client,
bucket, key, and deletedAtMs. Pass batchTotal so bulk file deletions emit start
and completion events and retain their batch count.
In `@src/ui/RemoteBrowser.tsx`:
- Around line 577-591: Update the bulk-delete flow around mapWithConcurrency to
catch errors per entry, track whether any deletion failed, and allow all items
to be attempted instead of aborting on the first error. Match the existing
handleMove error-handling pattern, rolling back only when appropriate, showing
the error notification for failures, and always calling refreshSilently() so the
UI reflects the server state.
---
Nitpick comments:
In `@src/ui/TransferWidget.tsx`:
- Around line 387-389: Update the true branch of the move-label expression in
the TransferWidget formatting logic to use the plural “items” directly, removing
the unreachable batchTotal === 1 check. Preserve the existing singular/plural
handling in the false branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d23adef0-8cb0-4dab-ac80-128f831317c2
📒 Files selected for processing (5)
src/services/appServices.tssrc/ui/RemoteBrowser.tsxsrc/ui/TransferWidget.tsxsrc/ui/services.tstests/unit/ui/MoveProgress.test.tsx
- browser.delete's file path bypassed runTracked entirely, so bulk- deleting files silently dropped batchTotal and never showed up in the transfer widget. Wrap both branches in runTracked; single files now report totalItems: 1 since s3MoveFileToTrash has no sub-object progress of its own to emit. - handleBulkDeleteToTrash aborted the whole batch on the first error and rolled back every item, including ones already trashed successfully server-side — a permanent UI/server desync. Match handleMove's per-item catch pattern so partial failures roll back only what didn't succeed and refreshSilently always runs. - Drop an unreachable batchTotal === 1 branch in widgetTitle: the surrounding guard already ensures batchTotal >= 2 there.
The previous "N of M" showed the in-flight count, which sits pinned at BULK_OP_CONCURRENCY then ticks 3 → 2 → 1 as the queue drains — a countdown that reads as the opposite of progress. Count the items that have actually finished instead, so the header climbs "Moved 0 of 76" → "1 of 76" → … toward the total.
f4nu
changed the title
# Conflicts: # tests/unit/ui/MoveProgress.test.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/TransferWidget.tsx (1)
528-537: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEnsure failed moves are cleared by the broom button.
hasProcessedevaluates totruefor ANY settled move, including"failed". However, ifclearCompleted()only clears moves with"completed"status (as its name implies), any"failed"moves will remain in the list. This leaves the broom button permanently visible and stuck, as clicking it won't dismiss the failed item.Explicitly dismissing all settled moves guarantees both completed and failed items are cleared, mirroring the behavior of
clearAll(). (Note: You can then safely removeclearCompletedfrom theuseMoveProgressdestructuring on line 453).🧹 Proposed fix
- const hasProcessed = settledTransfers.length > 0 || visibleMoves.some((m) => m.status !== "moving"); + const settledMoves = visibleMoves.filter((m) => m.status !== "moving"); + const hasProcessed = settledTransfers.length > 0 || settledMoves.length > 0; function clearProcessed() { setDismissed((prev) => { const next = new Set(prev); for (const t of settledTransfers) next.add(t.id); return next; }); for (const t of settledTransfers) void services.engine.dismiss(t.id); - clearCompleted(); + for (const m of settledMoves) dismissMove(m.moveId); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/TransferWidget.tsx` around lines 528 - 537, Update clearProcessed in TransferWidget to dismiss every settled transfer, including failed moves, rather than relying on clearCompleted’s completed-only behavior. Preserve the existing dismissed-state updates and engine dismissal loop, remove the clearCompleted invocation, and remove clearCompleted from the useMoveProgress destructuring.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ui/TransferWidget.tsx`:
- Line 390: Add a batchId field to the MoveProgress interface, generate and pass
a unique batch ID from each bulk-action call site, and update the count near
completedMoves.length to include only completed moves matching the active
batchId. Preserve existing progress behavior while preventing retained results
from earlier batches from affecting the current count.
- Around line 388-393: In the batch progress branch of the transfer widget,
replace the hardcoded “Moved” text with the existing completedVerb value so
progress messages use the localized verb for each operation kind, including
purge. Keep the batch counts and surrounding formatting unchanged.
---
Outside diff comments:
In `@src/ui/TransferWidget.tsx`:
- Around line 528-537: Update clearProcessed in TransferWidget to dismiss every
settled transfer, including failed moves, rather than relying on
clearCompleted’s completed-only behavior. Preserve the existing dismissed-state
updates and engine dismissal loop, remove the clearCompleted invocation, and
remove clearCompleted from the useMoveProgress destructuring.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce92f04e-bbb0-466e-9164-59f6ebb60aca
📒 Files selected for processing (4)
src/services/appServices.tssrc/ui/RemoteBrowser.tsxsrc/ui/TransferWidget.tsxtests/unit/ui/MoveProgress.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ui/RemoteBrowser.tsx
| const batchTotal = Math.max(0, ...p.movingMoves.map((m) => m.batchTotal ?? 0)); | ||
| if (batchTotal > 1) { | ||
| const done = p.completedMoves.length; | ||
| return `Moved ${done} of ${batchTotal} items…`; | ||
| } | ||
| return `${moveVerb} ${n} item${n === 1 ? "" : "s"}…`; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a dynamic verb based on the operation kind.
The hardcoded "Moved" string applies to all operations, including "purge" (Empty Trash). This results in confusing progress text like "Moved 1 of 5 items…" during a bulk deletion.
Use completedVerb (which is already imported and used on line 406) to properly localize the verb to the specific operation kind.
💡 Proposed fix
- const batchTotal = Math.max(0, ...p.movingMoves.map((m) => m.batchTotal ?? 0));
- if (batchTotal > 1) {
- const done = p.completedMoves.length;
- return `Moved ${done} of ${batchTotal} items…`;
- }
+ const batchTotal = Math.max(0, ...p.movingMoves.map((m) => m.batchTotal ?? 0));
+ if (batchTotal > 1) {
+ const done = p.completedMoves.length;
+ const verbed = kinds.size === 1 ? completedVerb(p.movingMoves[0].kind) : "moved";
+ const capitalizedVerbed = verbed.charAt(0).toUpperCase() + verbed.slice(1);
+ return `${capitalizedVerbed} ${done} of ${batchTotal} items…`;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const batchTotal = Math.max(0, ...p.movingMoves.map((m) => m.batchTotal ?? 0)); | |
| if (batchTotal > 1) { | |
| const done = p.completedMoves.length; | |
| return `Moved ${done} of ${batchTotal} items…`; | |
| } | |
| return `${moveVerb} ${n} item${n === 1 ? "" : "s"}…`; | |
| const batchTotal = Math.max(0, ...p.movingMoves.map((m) => m.batchTotal ?? 0)); | |
| if (batchTotal > 1) { | |
| const done = p.completedMoves.length; | |
| const verbed = kinds.size === 1 ? completedVerb(p.movingMoves[0].kind) : "moved"; | |
| const capitalizedVerbed = verbed.charAt(0).toUpperCase() + verbed.slice(1); | |
| return `${capitalizedVerbed} ${done} of ${batchTotal} items…`; | |
| } | |
| return `${moveVerb} ${n} item${n === 1 ? "" : "s"}…`; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/TransferWidget.tsx` around lines 388 - 393, In the batch progress
branch of the transfer widget, replace the hardcoded “Moved” text with the
existing completedVerb value so progress messages use the localized verb for
each operation kind, including purge. Keep the batch counts and surrounding
formatting unchanged.
| // (the larger) — add a batchId if that ever actually comes up. | ||
| const batchTotal = Math.max(0, ...p.movingMoves.map((m) => m.batchTotal ?? 0)); | ||
| if (batchTotal > 1) { | ||
| const done = p.completedMoves.length; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Incorrect count of completed items across sequential batches.
p.completedMoves contains all completed moves currently retained in the widget. If a user performs a bulk move, does not manually click the "Clear finished" broom, and then performs a second bulk move, done will incorrectly include the first batch's completed items. This results in an artificially inflated count (e.g., displaying "Moved 10 of 5 items…") while the second batch processes.
Since there is no way to distinguish which batch a completed move belongs to, the reliable fix is to add a batchId property to the MoveProgress interface, populate it in the bulk action call sites (e.g., using crypto.randomUUID()), and filter p.completedMoves here to only count items matching the active batchId.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/TransferWidget.tsx` at line 390, Add a batchId field to the
MoveProgress interface, generate and pass a unique batch ID from each
bulk-action call site, and update the count near completedMoves.length to
include only completed moves matching the active batchId. Preserve existing
progress behavior while preventing retained results from earlier batches from
affecting the current count.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. An unexpected error occurred while generating fixes: Not Found - https://docs.github.com/rest/git/refs#get-a-reference |
1 participant
Summary
The transfer widget's move/trash header counted only the items currently in flight (
movingMoves.length). Because bulk moves and trashes are throttled toBULK_OP_CONCURRENCY(3) items at a time, that count sat pinned at 3 for almost the whole operation, then ticked down 3 → 2 → 1 as the queue drained — a countdown that read as the opposite of progress, and gave no sense of how big the batch actually was.This PR makes the header a true progress indicator that counts up toward the batch total:
How it works
batchTotaltoMoveProgress, set once per batch at the call sites inRemoteBrowser.tsx(handleMove/handleBulkDeleteToTrash), where the selection size is already known before the concurrency-limited loop starts. Threaded throughrunTracked/browser.move/browser.delete.widgetTitle()showsMoved {completed} of {batchTotal} items…for real batches (batchTotal > 1), counting only items that have actually finished, not the in-flight ones.Also addressed (CodeRabbit review)
browser.deletefile path bypassedrunTracked— bulk-deleting files (vs folders) to Trash silently droppedbatchTotaland never appeared in the widget. Both branches now run throughrunTracked; a single file reportstotalItems: 1sinces3MoveFileToTrashhas no sub-object progress of its own.handleBulkDeleteToTrashaborted the whole batch on the first error and rolled back every item, including ones already trashed server-side — a permanent UI/server desync. Now matcheshandleMove's per-item catch pattern, so partial failures roll back only what didn't succeed andrefreshSilently()always runs.Test plan
bun run typecheckpassesbun test tests/unit— full suite (305 tests) passes🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests