PE-9103: Parallelize upload preparation pipeline - #2161
Conversation
…ation, plans PE-9103 - parallelize conflict detection: check all files concurrently via Future.wait instead of sequential DB query per file - pre-compute all file lengths in parallel at start of preparation, cache in _fileLengthCache to avoid redundant sequential reads - parallelize cost estimation: turbo balance + AR sizes fetched in parallel, then AR and Turbo cost calculations run concurrently - parallelize upload plan creation: AR and Turbo plans created simultaneously via Future.wait instead of sequentially - remove duplicate wallet mismatch check (called twice in prepareUploadPlanAndCostEstimates) - skip ANT records re-fetch if already populated from initial fire-and-forget fetch - reduce 100ms artificial UI delay to Duration.zero (just yields to event loop for state update) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughUpload preparation now caches file lengths, parallelizes conflict checks and upload setup, and runs upload plan mounting, balance retrieval, size calculation, and cost estimation concurrently. ChangesUpload preparation concurrency
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lib/core/upload/uploader.dart (1)
440-451: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse the Turbo estimate instead of recalculating it.
For non-free Turbo uploads,
turboCostFuturecalculates the Turbo cost, then_determineUploadMethod()calls_turboUploadCostCalculator.calculateCost()again. ReuseturboCostEstimateto avoid the duplicate network-bound cost estimate and inconsistent fallback behavior.Proposed refactor
uploadMethod = isFreeUploadPossibleUsingTurbo ? UploadMethod.turbo - : await _determineUploadMethod( - turboBalance.balance, - turboBundleSizes, - _appConfig.allowedDataItemSizeForTurbo, - _isTurboAvailableToUploadAllFiles, - ); + : _isTurboAvailableToUploadAllFiles && + turboBalance.balance >= turboCostEstimate.totalCost + ? UploadMethod.turbo + : UploadMethod.ar;Also applies to: 473-480
🤖 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 `@lib/core/upload/uploader.dart` around lines 440 - 451, Reuse the existing Turbo cost result instead of calling the cost calculator twice: the `turboCostFuture` in `uploader.dart` already produces `turboCostEstimate`, so update `_determineUploadMethod()` (and the related non-free Turbo path in the surrounding upload selection flow) to consume that cached estimate rather than invoking `_turboUploadCostCalculator.calculateCost()` again. Keep the fallback behavior centralized in the `catchError` handling for `turboCostFuture`, and ensure the upload method decision uses the already awaited `turboCostEstimate` value consistently.
🤖 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 `@lib/blocs/upload/upload_cubit.dart`:
- Around line 105-107: The file length cache in UploadCubit is using a derived
string key from getIdentifier(), which can collide for different files and
overwrite cached lengths. Update _fileLengthCache and its read/write sites in
UploadCubit, especially around _getTotalSize() and the upload preparation logic,
to key by the file object or IOFile identity instead of a string identifier.
Make sure the cache lookup and population use the same identity-based key so
distinct files cannot share entries.
- Around line 1078-1080: The manifest reload guard in the upload cubit is
checking for a null `_ants` value even though `_ants` is initialized as an empty
list, so the reload path never runs. Update the condition in the upload flow
within the `UploadCubit` manifest handling logic to use the empty-list fallback
by checking `_ants.isEmpty` instead of `_ants == null`, keeping the existing
reload behavior intact when manifest entries are present.
---
Nitpick comments:
In `@lib/core/upload/uploader.dart`:
- Around line 440-451: Reuse the existing Turbo cost result instead of calling
the cost calculator twice: the `turboCostFuture` in `uploader.dart` already
produces `turboCostEstimate`, so update `_determineUploadMethod()` (and the
related non-free Turbo path in the surrounding upload selection flow) to consume
that cached estimate rather than invoking
`_turboUploadCostCalculator.calculateCost()` again. Keep the fallback behavior
centralized in the `catchError` handling for `turboCostFuture`, and ensure the
upload method decision uses the already awaited `turboCostEstimate` value
consistently.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 092002cb-3c6c-4110-ad14-4031aa496e51
📒 Files selected for processing (2)
lib/blocs/upload/upload_cubit.dartlib/core/upload/uploader.dart
| /// Pre-computed file lengths cache. Populated in parallel at the start of | ||
| /// upload preparation to avoid redundant sequential file.ioFile.length reads. | ||
| Map<String, int> _fileLengthCache = {}; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid key collisions in the file length cache.
getIdentifier() can collapse distinct files to the same key, especially for empty/blob paths where it returns only ioFile.name. That can overwrite one file’s cached length and make _getTotalSize() report the wrong total. Key the cache by the file object/IOFile identity instead of a derived string.
Proposed fix
- Map<String, int> _fileLengthCache = {};
+ Map<IOFile, int> _fileLengthCache = {};
@@
- size += _fileLengthCache[file.getIdentifier()] ??
+ size += _fileLengthCache[file.ioFile] ??
await file.ioFile.length;
@@
- _files[i].getIdentifier(): lengths[i],
+ _files[i].ioFile: lengths[i],Also applies to: 557-558, 941-945
🤖 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 `@lib/blocs/upload/upload_cubit.dart` around lines 105 - 107, The file length
cache in UploadCubit is using a derived string key from getIdentifier(), which
can collide for different files and overwrite cached lengths. Update
_fileLengthCache and its read/write sites in UploadCubit, especially around
_getTotalSize() and the upload preparation logic, to key by the file object or
IOFile identity instead of a string identifier. Make sure the cache lookup and
population use the same identity-based key so distinct files cannot share
entries.
IOFile.length returns FutureOr<int>, not Future<int>. Future.wait requires Future<T>, so wrap in async closure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Mocktail's thenThrow throws synchronously before returning a Future, so .catchError never gets attached. Wrapping in async IIFE with try/catch handles both sync and async throws correctly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Visit the preview URL for this PR (updated for commit 831070f): https://ardrive-web--pr2161-pe-9103-upload-prep-xabdgr5j.web.app (expires Thu, 06 Aug 2026 14:18:40 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: a224ebaee2f0939e7665e7630e7d3d6cd7d0f8b0 |
…timization # Conflicts: # lib/core/upload/uploader.dart
|
@coderabbitai review Note for this pass: the latest commit merges |
|
✅ Action performedReview finished.
|
Brings in everything that landed after the branch was cut: #2161 parallelize upload preparation #2174 read the Turbo free-item cap from /v1/info #2175 self-describing share links, authenticated downloads, recipient page #2176 vendor pdf.js and add pdfx #2177 CLAUDE.md #2178 gateway fallback for every fetch, vendored CDN scripts, dead code #2179 sync reads a single gateway, pooled instead of chunked No conflicts. Verified on the merged tree: analyze clean, 1319 app tests, 43 crypto tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Summary
Optimizes the upload preparation phase by parallelizing sequential operations that were previously blocking on each other unnecessarily.
Changes:
Future.waitinstead of one sequential query per file. For 100 files: 100 sequential queries → 1 parallel batch.startUploadPreparation()and cached in_fileLengthCache. Eliminates 2-3x redundant sequential reads per file across warning check, plan creation, and total size calculation.Future.waitinstead of sequentially.Duration.zero.Impact
For multi-file uploads (50+ files), preparation time reduced from sequential O(n) DB queries to parallel O(1). Network-bound cost estimation parallelized (saves ~1-2s).
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Performance Improvements
New Features