Repository navigation
fix(chunk upload): update chunk upload logic to include data key - #73
Conversation
|
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:
📝 WalkthroughWalkthroughAdds exponential-backoff controls for chunk uploads, propagation-aware handling of non-200 chunk responses, posts transaction headers with an explicit empty Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TransactionUploader
participant ChunkUploader
participant API
Client->>TransactionUploader: create & sign transaction (with data)
TransactionUploader->>ChunkUploader: determine uploadInBody?
alt Single-chunk
ChunkUploader->>API: POST /tx (Content-Type: application/json, data: base64)
API-->>ChunkUploader: 200 OK
ChunkUploader-->>TransactionUploader: success
else Chunked
TransactionUploader->>API: POST /tx (Content-Type: application/json, data: "")
API-->>TransactionUploader: 200 OK
TransactionUploader->>ChunkUploader: start parallel chunk uploads
loop per chunk
ChunkUploader->>API: POST /chunk (Content-Type: application/json, chunk payload)
alt 200
API-->>ChunkUploader: 200 OK
else non-200
ChunkUploader->>ChunkUploader: read bodyString & isChunkPropagationRetriableResponse(status, bodyString)
alt retriable
ChunkUploader->>ChunkUploader: throw retriable error → retry with backoff
else fatal
ChunkUploader-->>TransactionUploader: fatal error
end
end
end
ChunkUploader-->>TransactionUploader: completion notify
end
TransactionUploader-->>Client: final result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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.
🧹 Nitpick comments (1)
test/transaction_uploader_test.dart (1)
15-69: Harden chunked-path test by asserting/chunkis actually used.Right now the test validates
/txbody shape, but it stops after the first event (header post). Add a/chunkcall counter and assert it’s> 0to lock in the full chunked behavior.♻️ Suggested patch
- String? capturedTxBody; + String? capturedTxBody; + var chunkPostCount = 0; final mockClient = MockClient((request) async { @@ if (request.url.path.endsWith('chunk')) { + chunkPostCount++; return http.Response('', 200); } @@ - final events = uploader.upload().take(1); + final events = uploader.upload().take(2); await for (final _ in events) { - break; // Consume first event (after header is posted). + break; // Consume through first chunk completion. } @@ expect(txJson['data'], equals(''), reason: 'Chunked upload must send empty data; payload goes via /chunk'); + expect(chunkPostCount, greaterThan(0), + reason: 'Chunked upload must send payload chunks to /chunk');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/transaction_uploader_test.dart` around lines 15 - 69, The test currently stops after consuming the first uploader event and never asserts that the /chunk endpoint was actually hit; modify the MockClient handler to increment a counter (e.g., chunkPostCount) whenever a POST request.url.path.endsWith('chunk') is handled, then drive the uploader to complete (consume all events from uploader.upload() instead of breaking after the first event) and add an expectation that chunkPostCount > 0; update references in this test (capturedTxBody, mockClient, uploader.upload()) to use the new counter and final assertion to ensure the chunked upload path was exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/transaction_uploader_test.dart`:
- Around line 15-69: The test currently stops after consuming the first uploader
event and never asserts that the /chunk endpoint was actually hit; modify the
MockClient handler to increment a counter (e.g., chunkPostCount) whenever a POST
request.url.path.endsWith('chunk') is handled, then drive the uploader to
complete (consume all events from uploader.upload() instead of breaking after
the first event) and add an expectation that chunkPostCount > 0; update
references in this test (capturedTxBody, mockClient, uploader.upload()) to use
the new counter and final assertion to ensure the chunked upload path was
exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb16147e-56e1-4db0-84e1-9da0066d5328
📒 Files selected for processing (2)
lib/src/models/transaction_uploader.darttest/transaction_uploader_test.dart
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/src/streams/transaction_uploader.dart (1)
234-258:⚠️ Potential issue | 🟠 MajorThis non-200 classification path never executes with Dio's default configuration.
The
Dio()instance at line 230 has no configuration override. By default, Dio throwsDioExceptionfor any status code outside 2xx—this exception is caught by theretry()wrapper'sonRetryhandler before execution reaches theres.statusCode != 200block. The fatal error classification (_fatalChunkUploadErrors) and propagation-retriable response detection logic are therefore unreachable.To inspect responses with non-2xx status codes, either set
validateStatusto accept those codes or catchDioExceptionand examineexception.responsebefore deciding whether to retry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/streams/transaction_uploader.dart` around lines 234 - 258, The non-200 handling in the chunk upload never runs because Dio() throws DioException for non-2xx by default; update the upload logic in transaction_uploader.dart to either configure the Dio instance passed to the POST call with a validateStatus that accepts non-2xx responses OR wrap the await dio.post(...) in a try/catch for DioException and inspect exception.response (then call getResponseErrorFromDioRespose, compute bodyString and isChunkPropagationRetriableResponse, and consult _fatalChunkUploadErrors) before rethrowing or converting to the same error types you currently produce; ensure this integrates with the existing retry() wrapper so the onRetry semantics still apply.
🧹 Nitpick comments (1)
lib/src/models/transaction_uploader.dart (1)
149-168: Avoid serializing the full payload before overwritingdata.
_transaction.toJson()already base64-encodesdata, and the chunked path immediately replaces that field with''. On large uploads this does a full payload allocation only to discard it; a header-only serializer would keep this fix without the extra memory churn.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/models/transaction_uploader.dart` around lines 149 - 168, The code calls _transaction.toJson() (which base64-encodes the full data) and then immediately replaces txJson['data'] = '' on the chunked path, causing unnecessary large allocations; change the logic to avoid serializing the full payload when uploadInBody is false by either adding an option to Transaction.toJson({bool includeData = true}) and calling _transaction.toJson(includeData: false) or implement a lightweight method like _transaction.toHeaderJson() that returns a map without the base64 data, and use that result for the chunked-path branch (keep the existing txJson['data'] = '' behavior for node compatibility).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/src/utils.dart`:
- Around line 107-109: The helper currently treats responses as retriable unless
successCount is an int > 0; instead require successCount == 0 to mark retriable.
Locate the block that reads final successCount = map['successCount']; and change
the logic so that the function returns true only when successCount is an int and
equals 0, and returns false for any missing or non-integer successCount (and for
any successCount > 0). Ensure the updated condition replaces the current if
(successCount is int && successCount > 0) return false; return true; behavior.
---
Outside diff comments:
In `@lib/src/streams/transaction_uploader.dart`:
- Around line 234-258: The non-200 handling in the chunk upload never runs
because Dio() throws DioException for non-2xx by default; update the upload
logic in transaction_uploader.dart to either configure the Dio instance passed
to the POST call with a validateStatus that accepts non-2xx responses OR wrap
the await dio.post(...) in a try/catch for DioException and inspect
exception.response (then call getResponseErrorFromDioRespose, compute bodyString
and isChunkPropagationRetriableResponse, and consult _fatalChunkUploadErrors)
before rethrowing or converting to the same error types you currently produce;
ensure this integrates with the existing retry() wrapper so the onRetry
semantics still apply.
---
Nitpick comments:
In `@lib/src/models/transaction_uploader.dart`:
- Around line 149-168: The code calls _transaction.toJson() (which
base64-encodes the full data) and then immediately replaces txJson['data'] = ''
on the chunked path, causing unnecessary large allocations; change the logic to
avoid serializing the full payload when uploadInBody is false by either adding
an option to Transaction.toJson({bool includeData = true}) and calling
_transaction.toJson(includeData: false) or implement a lightweight method like
_transaction.toHeaderJson() that returns a map without the base64 data, and use
that result for the chunked-path branch (keep the existing txJson['data'] = ''
behavior for node compatibility).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9de35f0c-cf29-462c-ac72-3576010b2e2c
📒 Files selected for processing (3)
lib/src/models/transaction_uploader.dartlib/src/streams/transaction_uploader.dartlib/src/utils.dart
| final successCount = map['successCount']; | ||
| if (successCount is int && successCount > 0) return false; | ||
| return true; |
There was a problem hiding this comment.
Require successCount == 0 before marking the response retriable.
If successCount is missing or non-integer, this helper currently falls through to true, so any 400 payload with a non-empty results list gets treated as propagation-retriable. That can stretch unrelated validation responses into the full backoff loop.
Proposed fix
final successCount = map['successCount'];
- if (successCount is int && successCount > 0) return false;
+ if (successCount is! int || successCount != 0) return false;
return true;📝 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 successCount = map['successCount']; | |
| if (successCount is int && successCount > 0) return false; | |
| return true; | |
| final successCount = map['successCount']; | |
| if (successCount is! int || successCount != 0) return false; | |
| return true; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/src/utils.dart` around lines 107 - 109, The helper currently treats
responses as retriable unless successCount is an int > 0; instead require
successCount == 0 to mark retriable. Locate the block that reads final
successCount = map['successCount']; and change the logic so that the function
returns true only when successCount is an int and equals 0, and returns false
for any missing or non-integer successCount (and for any successCount > 0).
Ensure the updated condition replaces the current if (successCount is int &&
successCount > 0) return false; return true; behavior.
There was a problem hiding this comment.
thats a fine behavior for other general 400s for now
There was a problem hiding this comment.
@atticusofsparta, understood! The intentional fallthrough behavior — treating any 400 with a non-empty results list as propagation-retriable (even when successCount is missing or non-integer) — is acceptable for now.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/src/streams/transaction_uploader.dart`:
- Around line 52-58: The Dio POST calls (the dio.post(...) that posts
json.encode(headers) and the other POST used for chunk propagation) currently
throw DioException on non-2xx responses and never let you inspect
res.statusCode/res.data; update both calls to pass an Options with
validateStatus: (_) => true (added alongside contentType in the Options passed
to dio.post) so non-2xx responses are returned for inspection, keeping existing
error handling for network/timeouts intact; locate the calls by the symbols
dio.post, endpoint, data: json.encode(headers) and the companion
chunk-propagation POST and add validateStatus to their Options objects.
- Around line 28-32: Create a dedicated retriable exception type (e.g.,
RetriableChunkException) and change _uploadChunk() to throw that type only for
propagation/transient failures (the cases that should back off), while
continuing to throw plain Exception or specific non-retriable errors for
permanent 4xx/other failures; then update the retry(...) invocations that wrap
chunk uploads to pass retryIf: (e) => e is RetriableChunkException so the
exponential backoff
(_chunkRetryDelayFactor/_chunkRetryMaxDelay/_chunkRetryMaxAttempts) only applies
to propagation-related transient errors — apply the same change to the other
chunk-upload retry usages in this file that currently retry all Exceptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 14c36ab4-6305-4a4c-880a-2750ce3d1a98
📒 Files selected for processing (2)
lib/src/api/api.dartlib/src/streams/transaction_uploader.dart
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/transaction_uploader_test.dart (1)
62-64: Minor: Clarify or correct the comment about upload events.The comment states "First event: header posted; second: at least one chunk completed" but
_postChunksstream yields(uploadedChunks, totalChunks)tuples only for chunk completions, not for header posting. The header is posted before the stream begins yielding. Consider updating the comment to reflect the actual event semantics:- // First event: header posted; second: at least one chunk completed (POST /chunk). + // Each event represents a chunk completion; header is posted before stream begins. await uploader.upload().take(2).toList();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/transaction_uploader_test.dart` around lines 62 - 64, Comment is inaccurate: update the test comment near client.transactions.getUploader and uploader.upload to state that the header is posted before uploader.upload() begins yielding and that the stream yields only chunk-completion tuples from _postChunks (uploadedChunks, totalChunks); replace the current "First event: header posted; second: at least one chunk completed" with a note like "header is posted before the stream; stream emits (uploadedChunks, totalChunks) for chunk completions" to accurately describe uploader.upload() semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/transaction_uploader_test.dart`:
- Around line 62-64: Comment is inaccurate: update the test comment near
client.transactions.getUploader and uploader.upload to state that the header is
posted before uploader.upload() begins yielding and that the stream yields only
chunk-completion tuples from _postChunks (uploadedChunks, totalChunks); replace
the current "First event: header posted; second: at least one chunk completed"
with a note like "header is posted before the stream; stream emits
(uploadedChunks, totalChunks) for chunk completions" to accurately describe
uploader.upload() semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 878454a0-25de-4ac0-bece-62d95b641ae9
📒 Files selected for processing (2)
lib/src/streams/transaction_uploader.darttest/transaction_uploader_test.dart
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/transaction_uploader_test.dart (2)
91-143: LGTM! Clean test for single-chunk upload path.The test correctly verifies that single-chunk transactions include base64-encoded data in the POST /tx body.
Consider adding an assertion to explicitly verify no POST /chunk requests occur, which would make the test more robust against regressions:
💡 Optional: Assert no chunk POST occurs
String? capturedTxBody; http.Request? capturedTxRequest; + var chunkPostCount = 0; final mockClient = MockClient((request) async { if (request.method == 'GET') { ... } if (request.method == 'POST' && request.url.path.endsWith('tx')) { capturedTxRequest = request; capturedTxBody = request.body; return http.Response('', 200); } + if (request.method == 'POST' && request.url.path.endsWith('chunk')) { + chunkPostCount++; + return http.Response('', 200); + } return http.Response('', 404); }); ... expect((txJson['data'] as String).length, greaterThan(0), reason: 'Single-chunk upload must send base64 data in body'); + expect(chunkPostCount, equals(0), + reason: 'Single-chunk upload should not POST to /chunk');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/transaction_uploader_test.dart` around lines 91 - 143, Add an assertion that no POST /chunk occurred by capturing any chunk requests in the same test ("single-chunk upload sends transaction JSON with data key set to base64 payload"); introduce a new variable (e.g., capturedChunkRequest) inside the MockClient handler and set it when request.method == 'POST' && request.url.path.contains('chunk'), then after await client.transactions.upload(transaction).drain() assert capturedChunkRequest is null (similar to the existing checks for capturedTxRequest/capturedTxBody) so the test fails if a chunk upload is attempted.
5-5: Use public API import forArweaveSigner.Replace
import 'package:arweave/src/signer.dart';withimport 'package:arweave/arweave.dart';(orimport 'package:arweave';). The signer module is re-exported from the main package barrel file, and importing from the public API ensures better forward compatibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/transaction_uploader_test.dart` at line 5, Replace the direct private import of the signer implementation with the public package barrel export: change the import of 'package:arweave/src/signer.dart' to the public API import (e.g., 'package:arweave/arweave.dart' or 'package:arweave') so tests use the re-exported ArweaveSigner from the package's public API; update the import statement where ArweaveSigner is referenced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/transaction_uploader_test.dart`:
- Around line 91-143: Add an assertion that no POST /chunk occurred by capturing
any chunk requests in the same test ("single-chunk upload sends transaction JSON
with data key set to base64 payload"); introduce a new variable (e.g.,
capturedChunkRequest) inside the MockClient handler and set it when
request.method == 'POST' && request.url.path.contains('chunk'), then after await
client.transactions.upload(transaction).drain() assert capturedChunkRequest is
null (similar to the existing checks for capturedTxRequest/capturedTxBody) so
the test fails if a chunk upload is attempted.
- Line 5: Replace the direct private import of the signer implementation with
the public package barrel export: change the import of
'package:arweave/src/signer.dart' to the public API import (e.g.,
'package:arweave/arweave.dart' or 'package:arweave') so tests use the
re-exported ArweaveSigner from the package's public API; update the import
statement where ArweaveSigner is referenced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d85fbc83-6c6d-422f-9e4e-492cc06a7328
📒 Files selected for processing (1)
test/transaction_uploader_test.dart
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests