PE-9132: Turbo free-tier support — allowance-aware upload UX and typed payment failures - #2166
Conversation
10 MiB free pool per wallet, 105 KiB per-item eligibility, paid-only after exhaustion, credits never replenish free. Inventories the 15 silent-free posting paths, the current failure behavior on payment rejection, and phases the work: failure honesty (unblocked now), pool-aware eligibility (needs turbo API contract), surfacing UX. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…E-9132 Prepares the client for the restricted free tier (10 MiB pool, 105 KiB per-item, paid-only after exhaustion). Policy-independent hardening: - decode HTTP 402 into TurboPaymentRequiredException and 429 into TurboRateLimitException in the app-side TurboUploadService (pure turboExceptionForStatusCode mapping, unit tested); the uploader package maps 402 to its existing UnderFundException and excludes 402/429 from its 8-attempt retry loops (retrying payment rejections multiplies load and metered usage) - rename (file/folder) failures now dismiss the progress dialog and show an honest error - payment-specific copy when the rejection was 402 (previously: spinner forever) - move gains a failure state and dialog handling, no longer emits Success after an error (removes the TODO admitting it), and is reordered to post-then-commit: data items are prepared and posted BEFORE the local database transaction, so a rejected move can no longer leave local state claiming a move the chain never saw - TurboUploadService fetches maxItemBytes from GET /v1/info once at construction (maxFreeItemSizeBytes, config fallback) - the server-driven per-item free threshold per the descoped plan; wiring it into UploadPaymentEvaluator is the next commit - implementation plan doc updated with the decision: no pool tracking, no balance-endpoint dependency, static free-tier messaging Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTurbo upload handling now decodes payment and rate-limit responses, uses server-provided free-item limits and wallet allowance status, propagates payment failures through operation states, prevents premature move database commits, records worker errors, supports migration transport fallback, and presents localized payment-specific dialogs. ChangesTurbo free-tier handling
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Visit the preview URL for this PR (updated for commit a60a001): https://ardrive-web--pr2166-feat-turbo-free-tier-7cde8kwx.web.app (expires Thu, 30 Jul 2026 20:35:00 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: a224ebaee2f0939e7665e7630e7d3d6cd7d0f8b0 |
…hold PE-9132 Completes the free-tier client readiness: every operation that posts to Turbo now recognizes a payment rejection and shows one consistent, actionable message instead of a generic error or (previously) a hang. - new shared TurboPaymentRequired dialog (ArDriveStandardModalNew with a "Buy Credits" action into the existing top-up flow) as the single source of truth for the free-allowance-used-up UX; localized strings freeAllowanceUsedUpTitle/Description added to all six ARB files - new isTurboPaymentError() classifier; each op's failure state carries an isPaymentError flag set from the caught exception: rename, move, drive rename, folder create, drive create, hide/unhide, pin, license, ghost fixer - every corresponding form/dialog branches to the shared payment dialog on payment errors and keeps its existing generic error otherwise; folder-create, drive-rename and ghost-fixer gained the failure handling they previously lacked - UploadPaymentEvaluator now resolves the free per-item threshold from the server (TurboUploadService.maxFreeItemSizeBytes via /v1/info), falling back to allowedDataItemSizeForTurbo; wired through DI for the main upload flow (metadata/manifest paths keep the config fallback) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/blocs/fs_entry_move/fs_entry_move_bloc.dart (1)
274-310: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftHandle partial Turbo upload success before deferring every local commit.
postDataItemwrites each item independently. If item N fails after earlier items succeeded, those moves exist remotely, but the transaction at Lines 293-310 commits none locally. Retrying then publishes duplicate move revisions from stale local state.Persist each successfully accepted Turbo item, track resumable progress, or use an atomic batch operation before treating this as all-or-nothing.
🤖 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/fs_entry_move/fs_entry_move_bloc.dart` around lines 274 - 310, Update the Turbo upload path in the move transaction flow around postDataItem so partial success is recoverable: persist each successfully accepted item or equivalent resumable progress before continuing, or replace the per-item calls with an atomic batch operation. Ensure a failure after item N does not leave earlier remote moves absent from local state or cause retries to publish duplicate move revisions, while preserving the existing local commit behavior for successful non-Turbo uploads.
🧹 Nitpick comments (1)
lib/turbo/services/upload_service.dart (1)
228-235: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename one of the
TurboRateLimitExceptiontypes — the same class name is defined in bothlib/turbo/services/upload_service.dartandpackages/ardrive_uploader/lib/src/exceptions.dart, which makes unprefixedis TurboRateLimitExceptionchecks easy to confuse when both libraries are in scope.🤖 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/turbo/services/upload_service.dart` around lines 228 - 235, Rename the TurboRateLimitException declaration in upload_service.dart to a distinct, upload-service-specific exception name, and update all references in that file and related upload handling to use the new name. Keep the existing rate-limit semantics and leave packages/ardrive_uploader’s TurboRateLimitException unchanged.
🤖 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 `@docs/implementation_plan_turbo_free_tier.md`:
- Line 23: Update the multi-item move implementation around
fs_entry_move_bloc.dart’s move flow to define and implement explicit
partial-failure semantics, choosing atomic, resumable, or rollback behavior for
cases where an earlier item commits locally and a later remote post fails.
Document the chosen contract in the implementation plan and add tests covering
the partial-failure scenario.
In `@lib/blocs/drive_create/drive_create_state.dart`:
- Around line 54-60: Update the failure-state equality props to include
isPaymentError in DriveCreateFailure at
lib/blocs/drive_create/drive_create_state.dart:54-60, DriveRenameFailure at
lib/blocs/drive_rename/drive_rename_state.dart:16-18, and FolderCreateFailure at
lib/blocs/folder_create/folder_create_state.dart:15-18, preserving each state’s
existing equality fields.
In `@lib/blocs/fs_entry_license/fs_entry_license_bloc.dart`:
- Around line 165-168: Update the catch block in the licensing flow to include
the captured error in the addError call, preserving the original exception type
and message alongside the existing context and trace. Keep the
FsEntryLicenseFailure emission and isTurboPaymentError(error) handling
unchanged.
In `@lib/blocs/ghost_fixer/ghost_fixer_state.dart`:
- Around line 41-44: Include isPaymentError in Equatable equality for
GhostFixerFailure in lib/blocs/ghost_fixer/ghost_fixer_state.dart lines 41-44 by
overriding props with the superclass properties plus isPaymentError. Apply the
same props override to the corresponding hide failure state in
lib/blocs/hide/hide_state.dart lines 68-73.
In `@lib/components/drive_rename_form.dart`:
- Around line 94-95: Localize the hardcoded error descriptions by adding
corresponding ARB entries under lib/l10n/ and retrieving them through
appLocalizationsOf(context), matching the existing localized dialog-title
pattern. Apply this to lib/components/drive_rename_form.dart lines 94-95,
lib/components/folder_create_form.dart lines 92-93,
lib/components/fs_entry_rename_form.dart lines 112-113, and
lib/components/ghost_fixer_form.dart lines 84-85, preserving each message’s
meaning.
In `@lib/components/fs_entry_license_form.dart`:
- Around line 572-586: Update the payment-error UI in the licensing form so it
offers a recovery action for adding Credits instead of the existing immediate
Try Again retry. When state.isPaymentError is true, invoke the shared Turbo
payment dialog or reuse the established Add Credits action; preserve the current
retry behavior for non-payment failures.
In `@lib/l10n/app_es.arb`:
- Around line 172-173: Translate both free-tier message values in
lib/l10n/app_es.arb lines 172-173 into Spanish, and translate the corresponding
values in lib/l10n/app_hi.arb lines 172-173 into Hindi; preserve the existing
ARB keys and formatting.
In `@lib/l10n/app_ja.arb`:
- Around line 172-173: Replace the English values for freeAllowanceUsedUpTitle
and freeAllowanceUsedUpDescription in app_ja.arb with reviewed, natural Japanese
translations, preserving both message keys and their payment-related meaning.
In `@lib/l10n/app_zh-HK.arb`:
- Around line 172-173: Localize freeAllowanceUsedUpTitle and
freeAllowanceUsedUpDescription in lib/l10n/app_zh-HK.arb lines 172-173 with
Traditional Chinese (Hong Kong) text, and apply Simplified Chinese translations
for the same keys in lib/l10n/app_zh.arb lines 172-173; replace the English
fallback values while preserving the existing ARB keys and structure.
---
Outside diff comments:
In `@lib/blocs/fs_entry_move/fs_entry_move_bloc.dart`:
- Around line 274-310: Update the Turbo upload path in the move transaction flow
around postDataItem so partial success is recoverable: persist each successfully
accepted item or equivalent resumable progress before continuing, or replace the
per-item calls with an atomic batch operation. Ensure a failure after item N
does not leave earlier remote moves absent from local state or cause retries to
publish duplicate move revisions, while preserving the existing local commit
behavior for successful non-Turbo uploads.
---
Nitpick comments:
In `@lib/turbo/services/upload_service.dart`:
- Around line 228-235: Rename the TurboRateLimitException declaration in
upload_service.dart to a distinct, upload-service-specific exception name, and
update all references in that file and related upload handling to use the new
name. Keep the existing rate-limit semantics and leave
packages/ardrive_uploader’s TurboRateLimitException unchanged.
🪄 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: e8d07440-f6a6-435b-9599-96bbb1bcd997
📒 Files selected for processing (41)
docs/implementation_plan_turbo_free_tier.mdlib/blocs/drive_create/drive_create_cubit.dartlib/blocs/drive_create/drive_create_state.dartlib/blocs/drive_rename/drive_rename_cubit.dartlib/blocs/drive_rename/drive_rename_state.dartlib/blocs/folder_create/folder_create_cubit.dartlib/blocs/folder_create/folder_create_state.dartlib/blocs/fs_entry_license/fs_entry_license_bloc.dartlib/blocs/fs_entry_license/fs_entry_license_state.dartlib/blocs/fs_entry_move/fs_entry_move_bloc.dartlib/blocs/fs_entry_move/fs_entry_move_state.dartlib/blocs/fs_entry_rename/fs_entry_rename_cubit.dartlib/blocs/fs_entry_rename/fs_entry_rename_state.dartlib/blocs/ghost_fixer/ghost_fixer_cubit.dartlib/blocs/ghost_fixer/ghost_fixer_state.dartlib/blocs/hide/hide_bloc.dartlib/blocs/hide/hide_state.dartlib/blocs/pin_file/pin_file_bloc.dartlib/blocs/pin_file/pin_file_state.dartlib/components/drive_create_form.dartlib/components/drive_rename_form.dartlib/components/folder_create_form.dartlib/components/fs_entry_license_form.dartlib/components/fs_entry_move_form.dartlib/components/fs_entry_rename_form.dartlib/components/ghost_fixer_form.dartlib/components/hide_dialog.dartlib/components/pin_file_dialog.dartlib/components/turbo_payment_required_dialog.dartlib/core/upload/uploader.dartlib/l10n/app_en.arblib/l10n/app_es.arblib/l10n/app_hi.arblib/l10n/app_ja.arblib/l10n/app_zh-HK.arblib/l10n/app_zh.arblib/turbo/services/upload_service.dartlib/utils/dependency_injection_utils.dartpackages/ardrive_uploader/lib/src/exceptions.dartpackages/ardrive_uploader/lib/src/turbo_upload_service.darttest/turbo/services/turbo_exception_mapping_test.dart
| |---|---| | ||
| | File/folder rename | `lib/blocs/fs_entry_rename/fs_entry_rename_cubit.dart:176/:101` | | ||
| | Drive rename | `lib/blocs/drive_rename/drive_rename_cubit.dart:71` | | ||
| | Move (per item!) | `lib/blocs/fs_entry_move/fs_entry_move_bloc.dart:275` | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Define partial-failure semantics for multi-item moves.
The inventory says moves post one item at a time. Posting before committing prevents one-item divergence, but item 1 can still commit locally while item 2 fails remotely. Specify whether the operation is atomic, resumable, or rolled back, and test that behavior.
Also applies to: 63-65
🤖 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 `@docs/implementation_plan_turbo_free_tier.md` at line 23, Update the
multi-item move implementation around fs_entry_move_bloc.dart’s move flow to
define and implement explicit partial-failure semantics, choosing atomic,
resumable, or rollback behavior for cases where an earlier item commits locally
and a later remote post fails. Document the chosen contract in the
implementation plan and add tests covering the partial-failure scenario.
| final bool isPaymentError; | ||
| const DriveCreateFailure({required super.privacy, this.isPaymentError = false}); | ||
|
|
||
| @override | ||
| DriveCreateFailure copyWith({DrivePrivacy? privacy}) { | ||
| return DriveCreateFailure(privacy: privacy ?? this.privacy); | ||
| return DriveCreateFailure( | ||
| privacy: privacy ?? this.privacy, isPaymentError: isPaymentError); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include isPaymentError in failure-state equality.
The new payment classification is not included in Equatable equality, so payment and non-payment failures can compare equal and suppress distinct state transitions.
lib/blocs/drive_create/drive_create_state.dart#L54-L60: override failure-statepropsto includeisPaymentError.lib/blocs/drive_rename/drive_rename_state.dart#L16-L18: override failure-statepropsto includeisPaymentError.lib/blocs/folder_create/folder_create_state.dart#L15-L18: override failure-statepropsto includeisPaymentError.
📍 Affects 3 files
lib/blocs/drive_create/drive_create_state.dart#L54-L60(this comment)lib/blocs/drive_rename/drive_rename_state.dart#L16-L18lib/blocs/folder_create/folder_create_state.dart#L15-L18
🤖 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/drive_create/drive_create_state.dart` around lines 54 - 60, Update
the failure-state equality props to include isPaymentError in DriveCreateFailure
at lib/blocs/drive_create/drive_create_state.dart:54-60, DriveRenameFailure at
lib/blocs/drive_rename/drive_rename_state.dart:16-18, and FolderCreateFailure at
lib/blocs/folder_create/folder_create_state.dart:15-18, preserving each state’s
existing equality fields.
| } catch (error, trace) { | ||
| addError('Error licensing entities', trace); | ||
| emit(const FsEntryLicenseFailure()); | ||
| emit(FsEntryLicenseFailure( | ||
| isPaymentError: isTurboPaymentError(error))); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Preserve the original exception in diagnostics.
The catch block captures error, but addError records only 'Error licensing entities'. The Bloc’s error log therefore loses the exception type and message, making payment/network failures difficult to diagnose.
- addError('Error licensing entities', trace);
+ addError(error, trace);Based on the changed exception-handling path, the original error is available but discarded from diagnostics.
📝 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.
| } catch (error, trace) { | |
| addError('Error licensing entities', trace); | |
| emit(const FsEntryLicenseFailure()); | |
| emit(FsEntryLicenseFailure( | |
| isPaymentError: isTurboPaymentError(error))); | |
| } catch (error, trace) { | |
| addError(error, trace); | |
| emit(FsEntryLicenseFailure( | |
| isPaymentError: isTurboPaymentError(error))); |
🤖 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/fs_entry_license/fs_entry_license_bloc.dart` around lines 165 -
168, Update the catch block in the licensing flow to include the captured error
in the addError call, preserving the original exception type and message
alongside the existing context and trace. Keep the FsEntryLicenseFailure
emission and isTurboPaymentError(error) handling unchanged.
| class GhostFixerFailure extends GhostFixerState { | ||
| final bool isPaymentError; | ||
| GhostFixerFailure({this.isPaymentError = false}); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include the new payment flags in Equatable equality.
Both state subclasses add isPaymentError without overriding props, so payment and non-payment failures can compare equal and suppress state updates.
lib/blocs/ghost_fixer/ghost_fixer_state.dart#L41-L44: overridepropswith[...super.props, isPaymentError].lib/blocs/hide/hide_state.dart#L68-L73: overridepropswith[...super.props, isPaymentError].
📍 Affects 2 files
lib/blocs/ghost_fixer/ghost_fixer_state.dart#L41-L44(this comment)lib/blocs/hide/hide_state.dart#L68-L73
🤖 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/ghost_fixer/ghost_fixer_state.dart` around lines 41 - 44, Include
isPaymentError in Equatable equality for GhostFixerFailure in
lib/blocs/ghost_fixer/ghost_fixer_state.dart lines 41-44 by overriding props
with the superclass properties plus isPaymentError. Apply the same props
override to the corresponding hide failure state in
lib/blocs/hide/hide_state.dart lines 68-73.
| "freeAllowanceUsedUpTitle": "Free allowance used up", | ||
| "freeAllowanceUsedUpDescription": "Your free upload allowance has been used up, so this action now requires Credits. Add Credits and try again.", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Translate the new free-tier messages in every locale.
The new entries are English in both localized ARB files, causing Spanish and Hindi users to see untranslated payment-required messaging.
lib/l10n/app_es.arb#L172-L173: replace both values with Spanish translations.lib/l10n/app_hi.arb#L172-L173: replace both values with Hindi translations.
📍 Affects 2 files
lib/l10n/app_es.arb#L172-L173(this comment)lib/l10n/app_hi.arb#L172-L173
🤖 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/l10n/app_es.arb` around lines 172 - 173, Translate both free-tier message
values in lib/l10n/app_es.arb lines 172-173 into Spanish, and translate the
corresponding values in lib/l10n/app_hi.arb lines 172-173 into Hindi; preserve
the existing ARB keys and formatting.
Source: Coding guidelines
| "freeAllowanceUsedUpTitle": "Free allowance used up", | ||
| "freeAllowanceUsedUpDescription": "Your free upload allowance has been used up, so this action now requires Credits. Add Credits and try again.", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Provide Japanese translations for the new messages.
These values are English in app_ja.arb, so Japanese users will see untranslated payment-required dialog text. Replace both strings with reviewed Japanese translations.
As per coding guidelines, lib/l10n/** must provide localized ARB content for the supported languages.
🤖 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/l10n/app_ja.arb` around lines 172 - 173, Replace the English values for
freeAllowanceUsedUpTitle and freeAllowanceUsedUpDescription in app_ja.arb with
reviewed, natural Japanese translations, preserving both message keys and their
payment-related meaning.
Source: Coding guidelines
| "freeAllowanceUsedUpTitle": "Free allowance used up", | ||
| "freeAllowanceUsedUpDescription": "Your free upload allowance has been used up, so this action now requires Credits. Add Credits and try again.", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Localize the new free-allowance messages for both Chinese locales.
Both Chinese ARB files currently display the English fallback text.
lib/l10n/app_zh-HK.arb#L172-L173: add Traditional Chinese (Hong Kong) translations.lib/l10n/app_zh.arb#L172-L173: add Simplified Chinese translations.
As per coding guidelines, localization must support Simplified and Hong Kong Chinese through ARB files in lib/l10n.
📍 Affects 2 files
lib/l10n/app_zh-HK.arb#L172-L173(this comment)lib/l10n/app_zh.arb#L172-L173
🤖 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/l10n/app_zh-HK.arb` around lines 172 - 173, Localize
freeAllowanceUsedUpTitle and freeAllowanceUsedUpDescription in
lib/l10n/app_zh-HK.arb lines 172-173 with Traditional Chinese (Hong Kong) text,
and apply Simplified Chinese translations for the same keys in
lib/l10n/app_zh.arb lines 172-173; replace the English fallback values while
preserving the existing ARB keys and structure.
Source: Coding guidelines
- include isPaymentError in Equatable props on every failure state (drive/folder create, drive/folder/file rename, hide, ghost fixer, license) so a payment failure emitted after a generic one is not treated as an equal state and actually re-triggers the listener - license: preserve the original exception via logger.e before addError - license failure card: on a payment error the action becomes "Buy Credits" (opens the shared payment dialog) instead of retrying the same rejected operation - localize the generic metadata-op failure message via a shared actionFailedTryAgain key across all locales, replacing hardcoded English descriptions Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the review findings in f40205b:
Deferred (tracked, not in this PR):
|
…t paths PE-9132 Audit found the metadata ops were covered but the highest-visibility upload surfaces were not. Closes those gaps. Cross-cutting fix — the two upload services throw different payment exceptions (app-side TurboPaymentRequiredException vs ardrive_uploader package UnderFundException, sometimes wrapped in UploadStrategyException). isTurboPaymentError() now recognizes all of them; ardrive_uploader exports its exceptions so the app can classify them. Newly covered: - main file upload and folder upload: UploadCubit classifies a payment rejection from the failed-task list into UploadErrors.turboPaymentRequired (UploadFailure gains isPaymentError-carrying props); the failure widget shows the Buy-Credits dialog instead of a "Re-Upload" that would 402 again - post-upload ArNS name assignment: wrapped in try/catch so a rejected name data item can no longer hang an already-successful upload (the name can be reassigned later) - snapshot creation, manifest creation (unwrapping the task-list error through ManifestCreationException), standalone ArNS assignment, bulk import (unwrapping FileMetadataUploadException.originalError), and single/multi thumbnail creation all classify payment errors and show the shared dialog Deferred (documented): private-drive migration and login verification posts — tiny, effectively always-free signature items where a payment dialog mid-flow would be worse UX than the near-impossible failure. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
lib/l10n/app_ja.arb (1)
172-174: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winProvide Japanese translations for the new messages.
These values are English in
app_ja.arb, so Japanese users will see untranslated text. Replace these strings with natural Japanese translations.🌐 Proposed translations
- "actionFailedTryAgain": "Something went wrong. Please check your connection and try again.", - "freeAllowanceUsedUpTitle": "Free allowance used up", - "freeAllowanceUsedUpDescription": "Your free upload allowance has been used up, so this action now requires Credits. Add Credits and try again.", + "actionFailedTryAgain": "問題が発生しました。接続を確認してもう一度お試しください。", + "freeAllowanceUsedUpTitle": "無料の許容量を使い切りました", + "freeAllowanceUsedUpDescription": "無料のアップロード許容量を使い切ったため、この操作にはクレジットが必要です。クレジットを追加して、もう一度お試しください。",🤖 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/l10n/app_ja.arb` around lines 172 - 174, Replace the English values for actionFailedTryAgain, freeAllowanceUsedUpTitle, and freeAllowanceUsedUpDescription in app_ja.arb with natural Japanese translations, preserving the existing message meanings and ARB structure.
🧹 Nitpick comments (2)
lib/arns/presentation/assign_name_modal.dart (1)
421-436: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winHide the "Try again" action for payment errors.
Since the operation cannot succeed without adding credits, the "Try again" button should be hidden when a payment error occurs. This aligns with the PR objective to avoid retrying operations that have been rejected due to insufficient credits.
♻️ Proposed fix
if (state is SelectionFailed) { return [ ModalAction( action: () { Navigator.of(context).pop(); }, title: 'Cancel', ), - ModalAction( - action: () { - context.read<AssignNameBloc>().add(ConfirmSelectionAndUpload()); - }, - title: 'Try again', - ), + if (!state.isPaymentError) + ModalAction( + action: () { + context.read<AssignNameBloc>().add(ConfirmSelectionAndUpload()); + }, + title: 'Try again', + ), ]; }🤖 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/arns/presentation/assign_name_modal.dart` around lines 421 - 436, Update the SelectionFailed action construction in the assign-name modal to detect payment or insufficient-credit errors and omit the “Try again” ModalAction for those failures. Preserve the Cancel action and existing retry behavior for other SelectionFailed cases.lib/blocs/create_manifest/create_manifest_state.dart (1)
230-236: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
constconstructors for state object instantiations.When adding the
isPaymentErrorfield to these state classes, theconstmodifier was omitted from their constructors. Based on learnings, you should prefer usingconstconstructors whenever the constructor is declared as const and all arguments are compile-time constants. This enables canonicalized instances and potential compile-time optimizations.
lib/blocs/create_manifest/create_manifest_state.dart#L230-L236: AddconsttoCreateManifestFailure({this.isPaymentError = false});.lib/drive_explorer/multi_thumbnail_creation/bloc/multi_thumbnail_creation_state.dart#L73-L77: AddconsttoMultiThumbnailCreationError({this.isPaymentError = false});.lib/drive_explorer/thumbnail_creation/bloc/thumbnail_creation_state.dart#L16-L22: AddconsttoThumbnailCreationError({this.isPaymentError = false});.🤖 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/create_manifest/create_manifest_state.dart` around lines 230 - 236, Update the constructors for CreateManifestFailure in lib/blocs/create_manifest/create_manifest_state.dart (lines 230-236), MultiThumbnailCreationError in lib/drive_explorer/multi_thumbnail_creation/bloc/multi_thumbnail_creation_state.dart (lines 73-77), and ThumbnailCreationError in lib/drive_explorer/thumbnail_creation/bloc/thumbnail_creation_state.dart (lines 16-22) to be const, preserving their existing isPaymentError defaults and props behavior.Source: Learnings
🤖 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/arns/presentation/assign_name_modal.dart`:
- Around line 347-351: Remove the showTurboPaymentRequiredDialog call and
post-frame callback from the builder’s state.isPaymentError branch, and handle
the payment-error side effect in the BlocConsumer listener instead. In the
listener, detect SelectionFailed states with isPaymentError and invoke
showTurboPaymentRequiredDialog(context) once per emitted state.
In `@lib/components/create_snapshot_dialog.dart`:
- Around line 97-103: Move payment-error dialog side effects from each
BlocConsumer builder into its listener: in
lib/components/create_snapshot_dialog.dart#L97-L103, handle
SnapshotUploadFailure with isPaymentError by popping the current dialog and
showing showTurboPaymentRequiredDialog; in
lib/components/create_manifest_form.dart#L175-L180, move the
CreateManifestFailure payment handling and existing Navigator.pop; and in
lib/drive_explorer/multi_thumbnail_creation/multi_thumbnail_creation_modal.dart#L164-L169,
handle the MultiThumbnailCreationError payment case in the listener by popping
and showing the payment dialog. Remove these builder branches so each builder
falls back to its normal content or close behavior without returning an empty
modal.
In `@lib/drive_explorer/thumbnail_creation/page/thumbnail_creation_modal.dart`:
- Around line 52-58: Move showTurboPaymentRequiredDialog from the BlocConsumer
builder into its listener, triggering it when the payment-error state changes,
and leave the builder responsible only for returning the free-allowance
description. Also inspect related components such as CreateSnapshotDialog and
CreateManifestForm for dialog or Navigator.pop calls inside builders, moving
those side effects into their listeners while preserving their existing
state-dependent UI.
In `@lib/l10n/app_zh.arb`:
- Around line 172-174: Replace the English values for actionFailedTryAgain,
freeAllowanceUsedUpTitle, and freeAllowanceUsedUpDescription in app_zh.arb with
natural Chinese translations, preserving the existing keys and message meanings.
---
Duplicate comments:
In `@lib/l10n/app_ja.arb`:
- Around line 172-174: Replace the English values for actionFailedTryAgain,
freeAllowanceUsedUpTitle, and freeAllowanceUsedUpDescription in app_ja.arb with
natural Japanese translations, preserving the existing message meanings and ARB
structure.
---
Nitpick comments:
In `@lib/arns/presentation/assign_name_modal.dart`:
- Around line 421-436: Update the SelectionFailed action construction in the
assign-name modal to detect payment or insufficient-credit errors and omit the
“Try again” ModalAction for those failures. Preserve the Cancel action and
existing retry behavior for other SelectionFailed cases.
In `@lib/blocs/create_manifest/create_manifest_state.dart`:
- Around line 230-236: Update the constructors for CreateManifestFailure in
lib/blocs/create_manifest/create_manifest_state.dart (lines 230-236),
MultiThumbnailCreationError in
lib/drive_explorer/multi_thumbnail_creation/bloc/multi_thumbnail_creation_state.dart
(lines 73-77), and ThumbnailCreationError in
lib/drive_explorer/thumbnail_creation/bloc/thumbnail_creation_state.dart (lines
16-22) to be const, preserving their existing isPaymentError defaults and props
behavior.
🪄 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: 6cab44dc-0d6e-42dc-b7a7-da69460c0d2b
📒 Files selected for processing (44)
lib/arns/presentation/assign_name_bloc/assign_name_bloc.dartlib/arns/presentation/assign_name_bloc/assign_name_state.dartlib/arns/presentation/assign_name_modal.dartlib/blocs/bulk_import/bulk_import_bloc.dartlib/blocs/bulk_import/bulk_import_state.dartlib/blocs/create_manifest/create_manifest_cubit.dartlib/blocs/create_manifest/create_manifest_state.dartlib/blocs/create_snapshot/create_snapshot_cubit.dartlib/blocs/create_snapshot/create_snapshot_state.dartlib/blocs/drive_create/drive_create_state.dartlib/blocs/drive_rename/drive_rename_state.dartlib/blocs/folder_create/folder_create_state.dartlib/blocs/fs_entry_license/fs_entry_license_bloc.dartlib/blocs/fs_entry_license/fs_entry_license_state.dartlib/blocs/fs_entry_rename/fs_entry_rename_state.dartlib/blocs/ghost_fixer/ghost_fixer_state.dartlib/blocs/hide/hide_state.dartlib/blocs/upload/upload_cubit.dartlib/blocs/upload/upload_state.dartlib/components/create_manifest_form.dartlib/components/create_snapshot_dialog.dartlib/components/drive_rename_form.dartlib/components/folder_create_form.dartlib/components/fs_entry_license_form.dartlib/components/fs_entry_move_form.dartlib/components/fs_entry_rename_form.dartlib/components/ghost_fixer_form.dartlib/components/upload_form.dartlib/drive_explorer/multi_thumbnail_creation/bloc/multi_thumbnail_creation_bloc.dartlib/drive_explorer/multi_thumbnail_creation/bloc/multi_thumbnail_creation_state.dartlib/drive_explorer/multi_thumbnail_creation/multi_thumbnail_creation_modal.dartlib/drive_explorer/thumbnail_creation/bloc/thumbnail_creation_bloc.dartlib/drive_explorer/thumbnail_creation/bloc/thumbnail_creation_state.dartlib/drive_explorer/thumbnail_creation/page/thumbnail_creation_modal.dartlib/l10n/app_en.arblib/l10n/app_es.arblib/l10n/app_hi.arblib/l10n/app_ja.arblib/l10n/app_zh-HK.arblib/l10n/app_zh.arblib/manifest/domain/manifest_repository.dartlib/pages/drive_detail/components/bulk_import_modal.dartlib/turbo/services/upload_service.dartpackages/ardrive_uploader/lib/ardrive_uploader.dart
🚧 Files skipped from review as they are similar to previous changes (15)
- lib/blocs/fs_entry_license/fs_entry_license_state.dart
- lib/l10n/app_hi.arb
- lib/l10n/app_es.arb
- lib/blocs/ghost_fixer/ghost_fixer_state.dart
- lib/blocs/drive_rename/drive_rename_state.dart
- lib/blocs/folder_create/folder_create_state.dart
- lib/blocs/drive_create/drive_create_state.dart
- lib/components/folder_create_form.dart
- lib/l10n/app_zh-HK.arb
- lib/blocs/hide/hide_state.dart
- lib/components/ghost_fixer_form.dart
- lib/components/fs_entry_move_form.dart
- lib/blocs/fs_entry_license/fs_entry_license_bloc.dart
- lib/components/drive_rename_form.dart
- lib/components/fs_entry_rename_form.dart
| "actionFailedTryAgain": "Something went wrong. Please check your connection and try again.", | ||
| "freeAllowanceUsedUpTitle": "Free allowance used up", | ||
| "freeAllowanceUsedUpDescription": "Your free upload allowance has been used up, so this action now requires Credits. Add Credits and try again.", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Provide Chinese translations for the new messages.
These values are English in app_zh.arb, so Chinese users will see untranslated text. Replace these strings with natural Chinese translations.
🌐 Proposed translations
- "actionFailedTryAgain": "Something went wrong. Please check your connection and try again.",
- "freeAllowanceUsedUpTitle": "Free allowance used up",
- "freeAllowanceUsedUpDescription": "Your free upload allowance has been used up, so this action now requires Credits. Add Credits and try again.",
+ "actionFailedTryAgain": "出现问题。请检查您的连接并重试。",
+ "freeAllowanceUsedUpTitle": "免费额度已用完",
+ "freeAllowanceUsedUpDescription": "您的免费上传额度已用完,因此此操作现在需要积分。请添加积分并重试。",📝 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.
| "actionFailedTryAgain": "Something went wrong. Please check your connection and try again.", | |
| "freeAllowanceUsedUpTitle": "Free allowance used up", | |
| "freeAllowanceUsedUpDescription": "Your free upload allowance has been used up, so this action now requires Credits. Add Credits and try again.", | |
| "actionFailedTryAgain": "出现问题。请检查您的连接并重试。", | |
| "freeAllowanceUsedUpTitle": "免费额度已用完", | |
| "freeAllowanceUsedUpDescription": "您的免费上传额度已用完,因此此操作现在需要积分。请添加积分并重试。", |
🤖 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/l10n/app_zh.arb` around lines 172 - 174, Replace the English values for
actionFailedTryAgain, freeAllowanceUsedUpTitle, and
freeAllowanceUsedUpDescription in app_zh.arb with natural Chinese translations,
preserving the existing keys and message meanings.
- hide the ardrive_uploader package's TurboUploadTimeoutException / TurboRateLimitException in upload_cubit (they collide with the app-side classes of the same name now that the package exports its exceptions); the app-side types are the ones _emitError intends - add missing app_localizations imports to assign_name and thumbnail creation modals - drop the unused shared-dialog import in upload_form (its failure widget builds the payment modal inline) - const the thumbnail error state constructors - remove now-redundant direct exceptions.dart imports in three ardrive_uploader files (the barrel provides them) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…E-9132 Verification audit found my earlier thumbnail/bulk-import fixes were in the wrong place — the failure never reached the bloc catch I'd wired. - thumbnail_repository: the upload controller's onError only logged and never completed the completer, so a 402 (which fires onError, not onDone) hung single AND multi thumbnail creation in Loading forever. onError now errors the completer, unwrapping the task list via anyTaskIsTurboPaymentError (previously dead) into the typed exception so the blocs classify it. The onDone body (which posts the thumbnail metadata data item) is also guarded so a payment rejection there errors the completer instead of hanging. - bulk_import_bloc: the actual import-execution catch swallowed the error and emitted a const BulkImportError (isPaymentError always false), so a 402 during real bulk import showed a generic dialog. The error is now captured and classified via _isBulkImportPaymentError (unwrapping FileMetadataUploadException.originalError). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Final verification found multi-thumbnail and bulk import still swallowed 402s: the ardrive_utils WorkerPool caught task exceptions in Worker._execute and passed only the task (not the exception) to onWorkerError, so the pool completed normally and the awaiting bloc never saw the failure. - WorkerPool.onWorkerError now receives the exception (Function(T, Object)) - multi thumbnail: onWorkerError captures a payment rejection into a flag and, after onAllTasksCompleted, emits MultiThumbnailCreationError( isPaymentError: true) instead of reporting completion - bulk import: FileImportFailure now preserves originalError; the worker records failures into BulkImportResult (was: only logged); the bloc captures the result and classifies the terminal error from the failures' originalError as well as the outer catch - fixes a scope bug: importResult is declared before the try so it is visible in the post-catch classification This closes the last two surfaces; single-thumbnail and the other seven were already verified. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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 (1)
lib/blocs/bulk_import/bulk_import_bloc.dart (1)
259-284: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix inaccurate
failedFilescount and preserve swallowed errors.There are two issues in this block:
- Inaccurate failure count on partial success:
failedFilesis assigned fromfailedPaths. However,failedPathsis only populated if the overall_bulkImportFilescall throws an exception. When_bulkImportFilescompletes with individual file failures, it returns them inimportResult.failureswithout throwing. This results infailedPaths.lengthevaluating to 0, which incorrectly reports no failed files inBulkImportSuccess.- Swallowed errors on total failure: When all files fail and
_bulkImportFilesreturns those failures inimportResult.failures(meaningsuccessfulFiles == 0),lastImportErrorevaluates tonull. PassingnulltoBulkImportErrorswallows the root cause of the failure.Use
importResult.failuresto accurately count failures and extract the original error.🐛 Proposed fix
final totalFiles = files.length; final successfulFiles = processedFiles; - final failedFiles = failedPaths; + final failedFilesCount = importResult?.failures.length ?? (totalFiles - successfulFiles); if (successfulFiles == 0) { final paymentError = _isBulkImportPaymentError(lastImportError) || (importResult?.failures.any( (f) => _isBulkImportPaymentError(f.originalError)) ?? false); + + Object? errorToReport = lastImportError; + if (errorToReport == null && importResult != null && importResult.failures.isNotEmpty) { + errorToReport = importResult.failures.first.originalError; + } + emit(BulkImportError( paymentError ? 'Your free upload allowance has been used up. Add Credits to ' 'continue importing.' : 'Failed to import any files. Please check the manifest and ' 'try again.', - lastImportError, + errorToReport, paymentError, )); } else { emit(BulkImportSuccess( manifestTxId: manifestTxId, totalFiles: totalFiles, successfulFiles: successfulFiles, - failedFiles: failedFiles.length, + failedFiles: failedFilesCount, )); }🤖 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/bulk_import/bulk_import_bloc.dart` around lines 259 - 284, Update the bulk import result handling around _bulkImportFiles, importResult, and the successfulFiles branches to derive failedFiles from importResult.failures, falling back to failedPaths only when appropriate. When all files fail and returned failures exist, extract an original error from importResult.failures and pass it to BulkImportError instead of leaving lastImportError null; preserve thrown-error handling and payment-error detection.
🧹 Nitpick comments (3)
lib/drive_explorer/thumbnail/repository/thumbnail_repository.dart (1)
234-241: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
try/catchover.catchError()inasyncfunctions.In Dart, when working within an
asyncfunction, using atry/catchblock around the awaitedFutureis more idiomatic and robust than appending.catchError(). It also ensures that any synchronous exceptions thrown prior to theFuturegeneration are safely caught.♻️ Proposed refactor
Wrap the
await _driveDao.transaction(...)call (starting on line 201) in atryblock, and replace.catchErrorwith acatchblock at the end:- }).catchError((Object e) { - // A failure while posting the thumbnail metadata (e.g. a payment - // rejection) must error the completer, not hang the awaiting bloc. - logger.e('Error finalizing thumbnail upload', e); - if (!completer.isCompleted) { - completer.completeError(e); - } - }); + }); + } catch (e) { + // A failure while posting the thumbnail metadata (e.g. a payment + // rejection) must error the completer, not hang the awaiting bloc. + logger.e('Error finalizing thumbnail upload', e); + if (!completer.isCompleted) { + completer.completeError(e); + } + }🤖 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/drive_explorer/thumbnail/repository/thumbnail_repository.dart` around lines 234 - 241, Replace the .catchError handler on the awaited _driveDao.transaction call with a surrounding try/catch in the containing async method. Preserve the existing error logging and guarded completer.completeError(e) behavior, while ensuring synchronous and asynchronous transaction failures are both caught.lib/core/arfs/use_cases/bulk_import_files.dart (1)
422-422: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid using
StackTrace.currentfor logging caught exceptions.Passing
StackTrace.currentto the logger inside an error callback captures the stack trace of the callback's execution, not the origin of the actual exception. This obscures the root cause and creates misleading logs during debugging. If the upstream API does not provide a stack trace, it is better to omit it entirely.
lib/core/arfs/use_cases/bulk_import_files.dart#L422-L422: RemoveStackTrace.currentfrom thelogger.ecall inonWorkerError.lib/drive_explorer/thumbnail/repository/thumbnail_repository.dart#L183-L184: RemoveStackTrace.currentfrom thelogger.ecall incontroller.onError.🤖 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/arfs/use_cases/bulk_import_files.dart` at line 422, Remove StackTrace.current from the logger.e call in onWorkerError in lib/core/arfs/use_cases/bulk_import_files.dart at lines 422-422, and from the logger.e call in controller.onError in lib/drive_explorer/thumbnail/repository/thumbnail_repository.dart at lines 183-184. Keep logging the existing error details without supplying a fabricated stack trace.packages/ardrive_utils/lib/src/worker.dart (1)
51-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider propagating
StackTracealongside the exception.The
onWorkerErrorcallback successfully exposes the underlyingObjecterror, but it lacks the originalStackTrace. Propagating the stack trace here would significantly improve debuggability for consumers ofWorkerPooland prevent them from resorting toStackTrace.current(which masks the original error's trace).💡 Suggested enhancement
Consider updating
WorkerPooland the internalWorkerexecution block to capture and pass the stack trace in a future iteration:// In WorkerPool: final Function(T, Object, [StackTrace?]) onWorkerError; // In Worker (internal execute block): } catch (e, st) { onError(task, e, st); }🤖 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 `@packages/ardrive_utils/lib/src/worker.dart` at line 51, Update the WorkerPool onWorkerError callback contract to accept the original StackTrace alongside the task and error, then modify the internal Worker execution catch block to capture the thrown trace and pass it through onError. Propagate the updated signature consistently to all callback declarations and invocations while preserving existing error handling behavior.
🤖 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 `@lib/blocs/bulk_import/bulk_import_bloc.dart`:
- Around line 259-284: Update the bulk import result handling around
_bulkImportFiles, importResult, and the successfulFiles branches to derive
failedFiles from importResult.failures, falling back to failedPaths only when
appropriate. When all files fail and returned failures exist, extract an
original error from importResult.failures and pass it to BulkImportError instead
of leaving lastImportError null; preserve thrown-error handling and
payment-error detection.
---
Nitpick comments:
In `@lib/core/arfs/use_cases/bulk_import_files.dart`:
- Line 422: Remove StackTrace.current from the logger.e call in onWorkerError in
lib/core/arfs/use_cases/bulk_import_files.dart at lines 422-422, and from the
logger.e call in controller.onError in
lib/drive_explorer/thumbnail/repository/thumbnail_repository.dart at lines
183-184. Keep logging the existing error details without supplying a fabricated
stack trace.
In `@lib/drive_explorer/thumbnail/repository/thumbnail_repository.dart`:
- Around line 234-241: Replace the .catchError handler on the awaited
_driveDao.transaction call with a surrounding try/catch in the containing async
method. Preserve the existing error logging and guarded
completer.completeError(e) behavior, while ensuring synchronous and asynchronous
transaction failures are both caught.
In `@packages/ardrive_utils/lib/src/worker.dart`:
- Line 51: Update the WorkerPool onWorkerError callback contract to accept the
original StackTrace alongside the task and error, then modify the internal
Worker execution catch block to capture the thrown trace and pass it through
onError. Propagate the updated signature consistently to all callback
declarations and invocations while preserving existing error handling behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c3564f7e-cd63-4d62-a85a-33c2e4a280be
📒 Files selected for processing (14)
lib/arns/presentation/assign_name_modal.dartlib/blocs/bulk_import/bulk_import_bloc.dartlib/blocs/upload/upload_cubit.dartlib/components/upload_form.dartlib/core/arfs/use_cases/bulk_import_files.dartlib/drive_explorer/multi_thumbnail_creation/bloc/multi_thumbnail_creation_bloc.dartlib/drive_explorer/multi_thumbnail_creation/bloc/multi_thumbnail_creation_state.dartlib/drive_explorer/thumbnail/repository/thumbnail_repository.dartlib/drive_explorer/thumbnail_creation/bloc/thumbnail_creation_state.dartlib/drive_explorer/thumbnail_creation/page/thumbnail_creation_modal.dartpackages/ardrive_uploader/lib/src/turbo_streamed_upload.dartpackages/ardrive_uploader/lib/src/upload_controller.dartpackages/ardrive_uploader/lib/src/upload_strategy.dartpackages/ardrive_utils/lib/src/worker.dart
💤 Files with no reviewable changes (4)
- packages/ardrive_uploader/lib/src/upload_controller.dart
- packages/ardrive_uploader/lib/src/turbo_streamed_upload.dart
- packages/ardrive_uploader/lib/src/upload_strategy.dart
- lib/components/upload_form.dart
🚧 Files skipped from review as they are similar to previous changes (5)
- lib/drive_explorer/thumbnail_creation/page/thumbnail_creation_modal.dart
- lib/drive_explorer/thumbnail_creation/bloc/thumbnail_creation_state.dart
- lib/drive_explorer/multi_thumbnail_creation/bloc/multi_thumbnail_creation_state.dart
- lib/arns/presentation/assign_name_modal.dart
- lib/blocs/upload/upload_cubit.dart
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tion PE-9132 Private-drive migration was the one ArFS metadata op with no L1 fallback — it posted the drive-signature data item only via Turbo. It now uses the same config-based branch every other op has: post via Turbo when useTurboUpload is enabled, otherwise wrap the already-signed data item in a DataBundle and post directly to the network via ArweaveService.postTx (pays AR from the wallet). Closes the deferred migration item from the free-tier work. Note this is a config switch (useTurboUpload=false), not an automatic on-402 fallback; migration signature items are ~1 KB and effectively always free-eligible, so pool exhaustion here is negligible. Login wallet-creation verification posts are intentionally NOT given this branch: the wallet is brand-new with no AR during creation, and the ETH path involves cross-chain signing — an L1 fallback there would fail, not help. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…E-9132 CodeRabbit (Critical/Major) caught the payment dialog being triggered from inside the builder via addPostFrameCallback in five modals. A builder can run many times for the same state (repaints, resizes, ancestor rebuilds), each queuing another dialog → duplicate/stacked modals. Moved every trigger to the BlocConsumer listener, which fires once per state transition: snapshot, single + multi thumbnail, standalone ArNS assign, and manifest (the last two CodeRabbit didn't flag but had the identical bug). Builders now render only the static fallback content. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…PE-9132 Turbo's new GET /v1/account/free?address=<wallet> reports how many free bytes a wallet has left. Until now isFreeThanksToTurbo was derived purely from item size, so a user whose free pool was used up was shown "this transaction is free thanks to Turbo", had the payment method selector hidden, and then hit a 402 mid-upload. The 402 handling recovered gracefully but the promise should never have been made. - add TurboFreeAllowance, modelling unlimited / limited / disabled / unknown, plus covers() and isExhaustedFor() - add PaymentService.getFreeAllowance and a non-throwing TurboBalanceRetriever.getFreeAllowance wrapper - require both size eligibility and allowance coverage before treating an upload as free, in the entity, bundle and snapshot paths - explain the switch to a payment selector when the allowance ran out, instead of silently swapping the UI - fetch the allowance per preparation, unlike the static item-size limit The value is advisory: Turbo's response stays the authority on whether an upload was free, and an unreachable endpoint falls back to the previous size-only behaviour rather than telling a user with allowance left to pay. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PsV2WFHZMGD9DUfX65cxuW
An audit of every surface that promises a free upload found two the first pass missed, both in the manifest flow: - UploadManifestModel.freeThanksToTurbo was still derived from item size alone, in both prepareManifestUpload and prepareUploadPlanAndCostEstimates. This is worse than a cosmetic false promise: when every manifest is marked free, UploadCubit skips the payment method selection entirely, so an exhausted wallet went straight to an upload that 402s. - create_manifest_form showed the payment options with no explanation when the allowance ran out, unlike the upload and snapshot dialogs. Also stores arDriveUploadManager, until now a required UploadCubit constructor parameter that was never assigned to a field, so the cubit can reach the allowance without a new dependency. Adds a regression test for the manifest path, verified to fail without the fix, plus the missing allowance stubs for the shared setUpAll mocks. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PsV2WFHZMGD9DUfX65cxuW
…get PE-9132 The free-tier UX was carried by two parallel booleans (isFreeThanksToTurbo and isFreeAllowanceExhausted) threaded through four state classes, where "free" and "allowance used up" could both be true, and by six hand-rolled conditionals across three dialogs. That duplication is how the manifest form ended up promising "free" without ever explaining what happened when it stopped being free. - add FreeUploadStatus (free / allowanceUsedUp / notEligible) and a freeUploadStatusFor helper holding the two rules in one place - store that single value in UploadPaymentInfo, UploadPaymentMethodInfo, ConfirmingSnapshotCreation and CreateManifestUploadReview, keeping the existing booleans as derived getters so no consumer changes - add TurboFreeStatusMessage, the one widget that renders the one status line, collapsing entirely when there is nothing to say - tighten the used-up copy to lead with the fact Behaviour is unchanged: same 720 tests pass, including the ~35 existing isFreeUploadPossibleUsingTurbo assertions untouched, and the manifest regression test still fails when the underlying fix is reverted. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PsV2WFHZMGD9DUfX65cxuW
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/blocs/upload/upload_cubit.dart (1)
145-190: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the server-fetched Turbo item-size limit for manifest free eligibility. These checks use the static
configService.config.allowedDataItemSizeForTurbowhile the shared upload payment path usesUploadPaymentEvaluator._maxFreeItemBytes, which prefers Turbo’s/v1/infovalue. Add an accessor such asArDriveUploadPreparationManager.getMaxFreeItemBytes()and use it here inprepareManifestUploadand the existing-manifest-entries loop.🤖 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 145 - 190, The manifest free-eligibility checks in prepareManifestUpload and the existing-manifest-entries loop must use the server-fetched Turbo limit from ArDriveUploadPreparationManager instead of configService.config.allowedDataItemSizeForTurbo. Add or reuse a getMaxFreeItemBytes() accessor backed by the same value used by UploadPaymentEvaluator._maxFreeItemBytes, and apply it at both affected sites in lib/blocs/upload/upload_cubit.dart:145-190 and lib/blocs/upload/upload_cubit.dart:1068-1080.
🤖 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/create_snapshot/create_snapshot_cubit.dart`:
- Around line 81-83: Update the _useTurboUpload getter and related upload-button
logic to honor appConfig.useTurboUpload before selecting Turbo based on
_freeStatus == FreeUploadStatus.free. Ensure freeStatus cannot bypass the
configured Turbo disablement, including the flows covered by
_computeIsFreeThanksToTurbo and the referenced upload handling sections.
- Around line 564-571: Update _computeIsFreeThanksToTurbo around
getFreeAllowance so allowance lookup exceptions are caught locally; on failure,
mark the item as not free and continue returning the paid AR/Turbo choices
instead of propagating the error to confirmDriveAndHeighRange().
In `@lib/core/upload/uploader.dart`:
- Around line 390-408: Update the _determineUploadMethod call in
getUploadPaymentInfoForEntities to pass allowedDataItemSizeForTurbo as the
allowedSizeForTurbo argument instead of dataItemSize. Preserve dataItemSize as
the turboBundleSizes value so the free-tier per-item size cap is enforced
consistently with freeStatus.
In
`@lib/drive_explorer/multi_thumbnail_creation/multi_thumbnail_creation_modal.dart`:
- Around line 111-115: Dismiss the current route before opening the payment
dialog in both payment-error listeners: the listener around
multi_thumbnail_creation_modal.dart lines 111-115 and the listener around
create_snapshot_dialog.dart lines 88-90. Once dismissal is guaranteed, remove
the empty placeholder branches at multi_thumbnail_creation_modal.dart lines
168-171 and create_snapshot_dialog.dart lines 101-103.
---
Outside diff comments:
In `@lib/blocs/upload/upload_cubit.dart`:
- Around line 145-190: The manifest free-eligibility checks in
prepareManifestUpload and the existing-manifest-entries loop must use the
server-fetched Turbo limit from ArDriveUploadPreparationManager instead of
configService.config.allowedDataItemSizeForTurbo. Add or reuse a
getMaxFreeItemBytes() accessor backed by the same value used by
UploadPaymentEvaluator._maxFreeItemBytes, and apply it at both affected sites in
lib/blocs/upload/upload_cubit.dart:145-190 and
lib/blocs/upload/upload_cubit.dart:1068-1080.
🪄 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 Plus
Run ID: 01f215cf-8d84-4191-9f19-4f251cce9114
📒 Files selected for processing (30)
lib/arns/presentation/assign_name_modal.dartlib/blocs/create_manifest/create_manifest_cubit.dartlib/blocs/create_manifest/create_manifest_state.dartlib/blocs/create_snapshot/create_snapshot_cubit.dartlib/blocs/create_snapshot/create_snapshot_state.dartlib/blocs/upload/models/payment_method_info.dartlib/blocs/upload/payment_method/bloc/upload_payment_method_bloc.dartlib/blocs/upload/upload_cubit.dartlib/components/create_manifest_form.dartlib/components/create_snapshot_dialog.dartlib/components/turbo_free_status_message.dartlib/components/upload_form.dartlib/core/upload/uploader.dartlib/drive_explorer/multi_thumbnail_creation/multi_thumbnail_creation_modal.dartlib/drive_explorer/thumbnail_creation/page/thumbnail_creation_modal.dartlib/l10n/app_en.arblib/l10n/app_es.arblib/l10n/app_hi.arblib/l10n/app_ja.arblib/l10n/app_zh-HK.arblib/l10n/app_zh.arblib/pages/app_router_delegate.dartlib/turbo/models/free_upload_status.dartlib/turbo/models/turbo_free_allowance.dartlib/turbo/services/payment_service.dartlib/turbo/turbo.darttest/blocs/create_snapshot_cubit_test.darttest/blocs/upload_cubit_test.darttest/core/upload/uploader_test.darttest/turbo/models/turbo_free_allowance_test.dart
💤 Files with no reviewable changes (1)
- lib/pages/app_router_delegate.dart
🚧 Files skipped from review as they are similar to previous changes (9)
- lib/l10n/app_zh.arb
- lib/l10n/app_ja.arb
- lib/l10n/app_es.arb
- lib/arns/presentation/assign_name_modal.dart
- lib/l10n/app_en.arb
- lib/l10n/app_zh-HK.arb
- lib/l10n/app_hi.arb
- lib/drive_explorer/thumbnail_creation/page/thumbnail_creation_modal.dart
- lib/components/upload_form.dart
| final freeAllowance = | ||
| await turboBalanceRetriever.getFreeAllowance(auth.currentUser.wallet); | ||
|
|
||
| _freeStatus = freeUploadStatusFor( | ||
| isSizeEligible: true, | ||
| byteCount: snapshotSize, | ||
| allowance: freeAllowance, | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not let allowance lookup failures block paid uploads.
If getFreeAllowance() throws, the exception escapes _computeIsFreeThanksToTurbo() and is caught by confirmDriveAndHeighRange(), which emits ComputeSnapshotDataFailure before ConfirmingSnapshotCreation. A transient allowance-service failure therefore prevents both paid AR and paid Turbo options, even though only free eligibility is unknown. Treat the item as not free and continue to the paid choices.
Proposed fix
- final freeAllowance =
- await turboBalanceRetriever.getFreeAllowance(auth.currentUser.wallet);
-
- _freeStatus = freeUploadStatusFor(
- isSizeEligible: true,
- byteCount: snapshotSize,
- allowance: freeAllowance,
- );
+ try {
+ final freeAllowance = await turboBalanceRetriever
+ .getFreeAllowance(auth.currentUser.wallet);
+ _freeStatus = freeUploadStatusFor(
+ isSizeEligible: true,
+ byteCount: snapshotSize,
+ allowance: freeAllowance,
+ );
+ } catch (e) {
+ logger.w('Free allowance lookup failed; continuing as paid: $e');
+ _freeStatus = FreeUploadStatus.notEligible;
+ }📝 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.
| final freeAllowance = | |
| await turboBalanceRetriever.getFreeAllowance(auth.currentUser.wallet); | |
| _freeStatus = freeUploadStatusFor( | |
| isSizeEligible: true, | |
| byteCount: snapshotSize, | |
| allowance: freeAllowance, | |
| ); | |
| try { | |
| final freeAllowance = await turboBalanceRetriever | |
| .getFreeAllowance(auth.currentUser.wallet); | |
| _freeStatus = freeUploadStatusFor( | |
| isSizeEligible: true, | |
| byteCount: snapshotSize, | |
| allowance: freeAllowance, | |
| ); | |
| } catch (e) { | |
| logger.w('Free allowance lookup failed; continuing as paid: $e'); | |
| _freeStatus = FreeUploadStatus.notEligible; | |
| } |
🤖 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/create_snapshot/create_snapshot_cubit.dart` around lines 564 - 571,
Update _computeIsFreeThanksToTurbo around getFreeAllowance so allowance lookup
exceptions are caught locally; on failure, mark the item as not free and
continue returning the paid AR/Turbo choices instead of propagating the error to
confirmDriveAndHeighRange().
Three of the five findings were valid:
- manifest free-eligibility used the static config item-size limit while the
shared upload path prefers Turbo's /v1/info value. Adds
ArDriveUploadPreparationManager.getMaxFreeItemBytes(), alongside the
existing getFreeAllowance(), and uses it at both manifest sites. This also
removes UploadCubit's last read of the global configService from main.dart.
- getUploadPaymentInfoForEntities passed the item size as its own size limit,
so the free-tier per-item cap was vacuously satisfied (x <= x). Passes the
actual limit. No practical change for metadata items, which are far below
it, but the cap is now real.
- the snapshot dialog is shown with barrierDismissible: false and did not pop
itself before opening the payment dialog, leaving an invisible undismissable
barrier over the app once that dialog was closed. It now pops first, like
create_manifest_form already did.
Declined, with reasons:
- gating snapshot free-status on appConfig.useTurboUpload: free uploads
deliberately bypass that flag ("Even if this feature flag is off, it will be
possible to upload using turbo for free files"), and gating it would restore
the false "free" promise this PR exists to remove.
- catching getFreeAllowance exceptions in the snapshot cubit: the retriever
wrapper already catches everything and returns unknown, and auth.currentUser
is read earlier in the same try by _computeBalanceEstimate.
- popping the route in multi_thumbnail_creation_modal: it is an OverlayEntry,
not a route, so Navigator.pop would dismiss the drive page underneath it.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsV2WFHZMGD9DUfX65cxuW
… overlay PE-9132
Resolves the two CodeRabbit findings I had previously declined, correctly:
- The free allowance was fetched only when the useTurboUpload flag was on,
but free uploads deliberately bypass that flag. So with the flag off a
small item still uploaded via Turbo yet was promised "free" without ever
checking the allowance — the exact bug this PR removes, hidden behind a
flag — and the snapshot path (which checks unconditionally) disagreed.
CodeRabbit proposed making snapshot honor the flag; that is the wrong
direction, as it would send free-eligible items down the paid path against
the documented intent. Instead getFreeAllowance is now unconditional, so
free-ness is always verified. The paid-turbo gate on _getTurboBalance is
unchanged. No runtime effect today (useTurboUpload is true in all flavors).
- The multi-thumbnail modal is an OverlayEntry, not a route, so Navigator.pop
would have dismissed the drive page underneath — which is why popping was
declined. But the overlay was still left behind the payment dialog. It now
dismisses through its own CloseMultiThumbnailCreation event, the mechanism
the modal already uses for closing.
Adds an assertion that the allowance is consulted even with the flag off.
Endpoint host confirmed empirically: GET payment.ardrive.io/v1/account/free
returns 200 {"bytesRemaining":10485760} (10 MiB), and upload.ardrive.io 404s,
so turboPaymentUri is the correct host. An unknown address returns the full
allowance rather than 404, so new wallets correctly read as free.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsV2WFHZMGD9DUfX65cxuW
…g used up PE-9132 A bulk/folder upload of small files whose total is larger than the wallet's remaining free pool was labelled "Free allowance used up" — wrong when the user still has most of their allowance and the upload simply exceeds it. Adds FreeUploadStatus.exceedsAllowance, distinct from allowanceUsedUp, chosen when the wallet still has a positive allowance but the upload is bigger than it. TurboFreeStatusMessage now shows an honest note for that case: "This upload exceeds your free allowance and will need Credits or AR." Deliberately robust rather than precise. The message states the fact (upload > remaining) and the outcome (needs payment) without predicting how many bytes end up free, because the client cannot know that: Turbo applies the free tier server-side, does not expose whether it bills per-item or per-bundle, and enforces a second per-IP pool that /v1/account/free does not report. So it is true whether Turbo frees part of the upload or none of it, and the 402 remains the authority on what is actually charged. This also corrects a stale comment that asserted all-or-nothing billing as fact. Behaviour is otherwise unchanged: exceedsAllowance is not free, so the payment selector still shows and upload-method selection is untouched. Single item / snapshot / manifest paths are unaffected in practice. The one existing assertion that expected "used up" for a partial multi-item upload is updated to expect the new, more accurate status; the derived getters and the widget's now-exhaustive switch keep every other consumer compiling unchanged. Adds unit coverage for freeUploadStatusFor (including boundaries and fail-open) and a widget test asserting each status renders the right message, both verified to fail under a collapsing mutation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PsV2WFHZMGD9DUfX65cxuW
…oad prep PE-9132 (#2169) getFreeAllowance() (added in #2166) was awaited late in each upload-prep method, after the balance and cost round-trips that already run serially. The allowance call is independent — it only needs the wallet — so it was adding an extra sequential Turbo round-trip to every upload-modal open, and doubling the worst-case wait when payment.ardrive.io is unavailable (two 8s timeouts back to back instead of one) before it fails open. Start the future up front in both getUploadPaymentInfoForEntities and getUploadPaymentInfoForUploadPlans and await it where the free status is computed, so it overlaps the balance, size and cost work. Timing only — no value changes; getFreeAllowance is a non-throwing wrapper, so the in-flight future cannot become an unhandled rejection. 733 tests unchanged. Claude-Session: https://claude.ai/code/session_01PsV2WFHZMGD9DUfX65cxuW Co-authored-by: vilenarios <philip.mataras@gmail.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Client readiness for Turbo's free tier (10 MiB per-wallet pool, 105 KiB per-item cap; see
docs/implementation_plan_turbo_free_tier.md). The app now makes an honest free-vs-paid promise on every upload surface, backed by theGET /v1/account/freeendpoint, and never leaves a payment rejection hanging.Two bodies of work:
1. Typed payment failures & honest failure UX
402→TurboPaymentRequiredException(app) /UnderFundException(uploader package);429→ typed rate-limit. Both are excluded from the uploader's retry loops — retrying a payment rejection just multiplies metered load.WorkerPoolwas silently swallowing (bulk import, multi-thumbnail).2. Allowance-aware free-vs-paid (this endpoint's payoff)
Previously "free" was decided from item size alone, so a user whose pool was exhausted was told an upload was free, had the payment selector hidden, and then hit a 402. Now free requires both size-eligibility and that the wallet's remaining allowance covers the upload.
Wired across the file/folder, snapshot, and manifest paths (including manifest re-upload, which previously skipped payment selection entirely when it wrongly judged everything free). The four states are modeled as one
FreeUploadStatusvalue rendered by one widget, so the surfaces can't drift apart.Design principles
bytesRemainingis a point-in-time, wallet-level snapshot. It only decides what we promise; Turbo's402on upload remains the sole authority on what's actually charged.unknown→ the previous size-only behavior. A failed check never tells a user with allowance left that they must pay./v1/account/freedoesn't report). True whether Turbo frees part of the upload or none of it.Endpoint host confirmed against prod:
GET payment.ardrive.io/v1/account/free→200 {"bytesRemaining":10485760}.Tests
733 passing. New coverage for the status-code → exception mapping,
/v1/account/freeparsing (includingnull= unlimited,0= off, unparseable = unknown/fail-open), thefreeUploadStatusForderivation (boundaries + fail-open), and a widget test asserting each status renders the right message. Core logic and widget both mutation-verified.Notes for review
/v1/account/freeis placed onturboPaymentUri(verified in prod). Confirming with the Turbo team that the endpoint is a committed, stable contract would let us later tighten "exceeds your allowance" into an exact count — nothing depends on it otherwise.🤖 Generated with Claude Code
https://claude.ai/code/session_01PsV2WFHZMGD9DUfX65cxuW
Summary by CodeRabbit