PE-9129: Sync and login resiliency quick wins - #2162
Open
vilenarios wants to merge 29 commits into
Open
Conversation
…ches - extract calculateTransactionParseBatchSize helper from syncAllDrives - clamp result to at least 1: wallets with 200+ drives previously computed a batch size of 0, causing BatchProcessor to throw and every drive sync to fail - guard the denominator so drivesSynced >= drivesCount cannot divide by zero - add unit tests covering the budget split and both clamp edge cases Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- getUser previously awaited getWalletBalance with no retry, timeout, or error handling, so a transient gateway failure locked the user out even though the password was correct and the profile is stored locally - balance fetch now falls back to zero on error or after a 5s timeout; ArDriveAuth._updateBalance already refreshes it asynchronously right after login and pushes the updated user to the auth stream - healthy-gateway behavior is unchanged (fresh balance at login) - deduplicate the double wallet.getAddress() call - add tests covering sync and async balance-fetch failures Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- syncAllDrives previously started every drive's full sync pipeline at once via Future.wait; accounts with many drives fanned out dozens of concurrent snapshot/GraphQL/data-fetch pipelines and could trip gateway rate limits - introduce runBoundedWorkers, a small worker-pool helper mirroring Future.wait(eagerError: false) semantics: all tasks run even if some fail, first error is reported after all complete - per-drive error handling, cancellation, and progress reporting are unchanged (the per-drive closure body is untouched) - add kMaxConcurrentDriveSyncs constant and unit tests for the pool (bounded in-flight count, failure isolation, edge cases) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- createDriveEntityHistoryFromTransactions rebuilt MetadataCache from a new SharedPreferences cache store on every parsed batch of every drive during sync; it now lazily constructs one instance and reuses it - behavior is otherwise unchanged: same store, same put/get semantics Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- BulkImportResult.importedFiles was always empty: WorkerPool discards the execute callback's return value, so the FileEntry produced by each successful _importFile call was never collected - successful imports are now added to importedFiles inside the worker callback; no current consumer reads the field (the bloc tracks progress via callbacks), so this only makes the result truthful Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- getInfoOfTxsToBePinned issued one GraphQL round-trip per 5 ids; bulk imports of large manifests paid 20x more sequential requests than needed - add first: 100 to the InfoOfTransactionsToBePinned query (previously it relied on the gateway's default page size, which is why the batch size had to stay tiny) and raise the batch size to 100, matching the established pattern in TransactionStatuses and LicenseAssertions queries - generated artemis code is gitignored and regenerated by scr setup Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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:
📝 WalkthroughWalkthroughThe PR adds persistent gateway caching and refresh controls, bounded concurrent drive synchronization, configurable GraphQL pagination with fallback handling, gateway-preserving config migration, and improved failure handling for imports, balances, snapshots, and metadata caching. ChangesGateway management
Synchronization and configuration
GraphQL history retrieval
Runtime reliability
Estimated code review effort: 5 (Critical) | ~100 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant GarModal as Gateway Modal
participant GarBloc
participant GarRepository
participant DataGatewayFallback
participant KeyValueStore
participant ArioSDK
User->>GarModal: Select Refresh list
GarModal->>GarBloc: Dispatch RefreshGateways
GarBloc->>GarRepository: refreshGateways()
GarRepository->>DataGatewayFallback: refreshGateways()
DataGatewayFallback->>ArioSDK: fetch gateways
ArioSDK-->>DataGatewayFallback: gateway list
DataGatewayFallback->>KeyValueStore: persist JSON
DataGatewayFallback-->>GarRepository: updated list
GarRepository-->>GarBloc: GatewaysLoaded
GarBloc-->>GarModal: render refreshed gateways
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- equatable 2.1.0 (released after the last green dev build) deprecates EquatableMixin, which data_table.dart uses; package lockfiles are not committed, so CI's per-package pub get floated to 2.1.0 and scr test failed on the analyze step for every PR and dev push - the main app's committed lockfile resolves equatable 2.0.7, where Equatable cannot be used as a mixin, so migrating the code instead of pinning would break the app build; pin until both contexts can move to 2.1.x together Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Visit the preview URL for this PR (updated for commit 916216e): https://ardrive-web--pr2162-fix-sync-quick-wins-20z4ixcp.web.app (expires Wed, 05 Aug 2026 18:24:38 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: a224ebaee2f0939e7665e7630e7d3d6cd7d0f8b0 |
- switch defaultArweaveGatewayUrl from the ardrive.net proxy to turbo-gateway.com in all three flavors; ardrive.net proxies to turbo-gateway anyway and its proxy pool served 503s during the 2026-07-08 outage, taking primary GraphQL down with it - bump configVersion 2 -> 3 so existing users' stored configs are replaced with the new default on next load - change the GraphQLRetry fallback from arweave.net/graphql to the Goldsky search index it proxies to (arweave-search.goldsky.com), avoiding arweave.net's aggressive rate limiting on the fallback path - document Goldsky's page-size behavior: requests above 100 items are silently clamped with hasNextPage falsely reporting false, so fallback queries must never exceed 100 per page Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- equatable 2.1.0 (released after the last green dev build) deprecates EquatableMixin, which data_table.dart uses; package lockfiles are not committed, so CI's per-package pub get floated to 2.1.0 and scr test failed on the analyze step for every PR and dev push - the main app's committed lockfile resolves equatable 2.0.7, where Equatable cannot be used as a mixin, so migrating the code instead of pinning would break the app build; pin until both contexts can move to 2.1.x together Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 54 minutes. |
- every sync (auto-sync every ~5 minutes, tab-focus restarts, manual syncs) forced a full ArNS sweep on Solana via getAntRecordsForWallet(update: true), bypassing the repository's 15-minute cache; each sweep makes several RPC calls per owned name - ArDrive currently has no ArNS integration, so the sweep and the post-sync saveAllFilesWithAssignedNames pass were pure RPC cost with no user-facing effect; both are removed along with the ARNSRepository dependency on SyncRepository - ArNS lookups elsewhere (upload flows, profile name) are untouched and fetch on demand; sync-time integration can be reintroduced later if the feature returns Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- the AR.IO gateway list was fetched from Solana RPC once per app session (data-fetch fallback, snapshot validation) and on every open of the gateway settings modal; the registry rarely changes, so this was recurring RPC cost for static data - DataGatewayFallback now persists the list in local storage: memory -> persisted cache -> single SDK fetch (persisted on success); across sessions the network is hit at most once ever - gateway settings serve the cached list; an explicit 'Refresh list' action (new RefreshGateways event + modal button) force-fetches and persists - SnapshotValidationService reads through the same shared cache - corrupt or missing persisted entries fall back to a normal fetch; fetch failures are cached in memory only (never persisted) so the next session retries - add DataGatewayFallback persistence tests and update gar repository tests Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- equatable 2.1.0 (released after the last green dev build) deprecates EquatableMixin, which data_table.dart uses; package lockfiles are not committed, so CI's per-package pub get floated to 2.1.0 and scr test failed on the analyze step for every PR and dev push - the main app's committed lockfile resolves equatable 2.0.7, where Equatable cannot be used as a mixin, so migrating the code instead of pinning would break the app build; pin until both contexts can move to 2.1.x together Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ix/sync-quick-wins
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
lib/services/arweave/data_gateway_fallback.dart (1)
64-87: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard against concurrent duplicate SDK fetches in
getGatewaysCached().The memory-cache check (
cachedGateways != null) and the assignment (cachedGateways = fetched) are separated byawaitpoints. Two concurrent callers (e.g.,_buildClientListduring sync andSnapshotValidationServicerunning in parallel) can both pass the null check and both trigger_arioSDK.getGateways(), violating the "at most once ever" contract and issuing duplicate Solana RPC calls.Memoize the in-flight fetch with a
Futureso all concurrent callers share a single SDK call:♻️ Proposed refactor: Future-based memoization
+ Future<List<Gateway>>? _gatewaysFuture; + Future<List<Gateway>> getGatewaysCached() async { if (cachedGateways != null) return cachedGateways!; + _gatewaysFuture ??= _fetchAndCacheGateways(); + return _gatewaysFuture!; + } + + Future<List<Gateway>> _fetchAndCacheGateways() async { final persisted = await _loadPersistedGateways(); if (persisted != null) { cachedGateways = persisted; return persisted; } - try { final fetched = await _arioSDK .getGateways() .timeout(_garListTimeout, onTimeout: () => <Gateway>[]); cachedGateways = fetched; if (fetched.isNotEmpty) { await _persistGateways(fetched); } } catch (e) { logger.w('GAR list unavailable, will not retry this session: $e'); cachedGateways = []; } return cachedGateways!; }🤖 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/services/arweave/data_gateway_fallback.dart` around lines 64 - 87, getGatewaysCached() in data_gateway_fallback.dart can race because cachedGateways is checked before an await and assigned later, so concurrent callers may each call _arioSDK.getGateways(). Add in-flight memoization with a shared Future so only one fetch runs at a time and all callers await the same result; keep the existing persisted-cache and timeout/persist behavior, but route the fetch through a single memoized path using getGatewaysCached(), _loadPersistedGateways(), and _persistGateways().lib/gar/domain/repositories/gar_repository.dart (1)
24-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused
arioSDKdependency fromGarRepositoryImpl. The implementation no longer references it, so the constructor and call sites can drop the parameter and simplify wiring.🤖 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/gar/domain/repositories/gar_repository.dart` around lines 24 - 35, GarRepositoryImpl still accepts an unused arioSDK dependency, so remove it from the class wiring and constructor. Update GarRepositoryImpl to drop the ArioSDK field and required parameter, then adjust any instantiations/call sites to stop passing it so the repository only depends on the services it actually uses.lib/services/arweave/arweave_service.dart (1)
155-166: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRace condition in lazy
_metadataCacheinitialization under concurrent sync.
_metadataCache ??= await ...is not atomic across anawaitboundary. With the new 5-concurrent-worker drive sync, two concurrent calls tocreateDriveEntityHistoryFromTransactionscan both see_metadataCacheas null, both create separateMetadataCacheinstances, and the second overwrites the first. Any dataputinto the discarded cache is lost, causing redundant metadata re-fetches on subsequent batches — partially defeating the goal of reusing a single cache.Cache the initialization
Futureitself so concurrent callers share the same in-flight creation:♻️ Suggested fix
- MetadataCache? _metadataCache; - - Future<MetadataCache> _getMetadataCache() async { - _metadataCache ??= await MetadataCache.fromCacheStore( - await newSharedPreferencesCacheStore(), - ); - return _metadataCache!; - } + Future<MetadataCache>? _metadataCacheFuture; + + Future<MetadataCache> _getMetadataCache() { + return _metadataCacheFuture ??= () async { + final cache = await MetadataCache.fromCacheStore( + await newSharedPreferencesCacheStore(), + ); + return cache; + }(); + }🤖 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/services/arweave/arweave_service.dart` around lines 155 - 166, The lazy _metadataCache initialization in _getMetadataCache is racy because the null-coalescing assignment spans an await, so concurrent calls can create and overwrite separate MetadataCache instances. Change _getMetadataCache to cache the in-flight initialization Future (or otherwise synchronize initialization) so all callers share the same first creation, and ensure createDriveEntityHistoryFromTransactions always receives the same MetadataCache instance even under concurrent sync.
🤖 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/services/arweave/data_gateway_fallback.dart`:
- Around line 91-98: Add a timeout to refreshGateways() because it currently
awaits _arioSDK.getGateways() directly and can hang indefinitely. Update the
fetch path in refreshGateways() to use the same _garListTimeout behavior as
getGatewaysCached(), so stalled AR.IO/Solana RPC calls fail fast instead of
leaving the gateway refresh stuck in LoadingGateways; keep the existing cache
update and _persistGateways flow in place after the timed fetch succeeds.
In `@test/services/arweave/data_gateway_fallback_test.dart`:
- Around line 165-193: The `DataGatewayFallback.refreshGateways()` tests only
cover the successful refresh path; add a failure-path test where
`sdk.getGateways()` throws. Make the test assert the exception propagates from
`refreshGateways()` and that the existing cached list remains unchanged by
verifying `getGatewaysCached()` still returns the original gateway after the
failed refresh.
---
Nitpick comments:
In `@lib/gar/domain/repositories/gar_repository.dart`:
- Around line 24-35: GarRepositoryImpl still accepts an unused arioSDK
dependency, so remove it from the class wiring and constructor. Update
GarRepositoryImpl to drop the ArioSDK field and required parameter, then adjust
any instantiations/call sites to stop passing it so the repository only depends
on the services it actually uses.
In `@lib/services/arweave/arweave_service.dart`:
- Around line 155-166: The lazy _metadataCache initialization in
_getMetadataCache is racy because the null-coalescing assignment spans an await,
so concurrent calls can create and overwrite separate MetadataCache instances.
Change _getMetadataCache to cache the in-flight initialization Future (or
otherwise synchronize initialization) so all callers share the same first
creation, and ensure createDriveEntityHistoryFromTransactions always receives
the same MetadataCache instance even under concurrent sync.
In `@lib/services/arweave/data_gateway_fallback.dart`:
- Around line 64-87: getGatewaysCached() in data_gateway_fallback.dart can race
because cachedGateways is checked before an await and assigned later, so
concurrent callers may each call _arioSDK.getGateways(). Add in-flight
memoization with a shared Future so only one fetch runs at a time and all
callers await the same result; keep the existing persisted-cache and
timeout/persist behavior, but route the fetch through a single memoized path
using getGatewaysCached(), _loadPersistedGateways(), and _persistGateways().
🪄 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: 1d198ccb-b197-4b9b-a558-3e423bad735b
📒 Files selected for processing (25)
assets/config/dev.jsonassets/config/prod.jsonassets/config/staging.jsonlib/core/arfs/use_cases/bulk_import_files.dartlib/gar/domain/repositories/gar_repository.dartlib/gar/presentation/bloc/gar_bloc.dartlib/gar/presentation/bloc/gar_event.dartlib/gar/presentation/widgets/gar_modal.dartlib/main.dartlib/services/arweave/arweave_service.dartlib/services/arweave/data_gateway_fallback.dartlib/services/arweave/graphql/queries/InfoOfTransactionsToBePinned.graphqllib/sync/constants.dartlib/sync/data/snapshot_validation_service.dartlib/sync/domain/repositories/sync_repository.dartlib/sync/utils/bounded_worker_pool.dartlib/user/repositories/user_repository.dartlib/utils/graphql_retry.dartpackages/ardrive_ui/pubspec.yamltest/gar/domain/repository/gar_repository_test.darttest/services/arweave/data_gateway_fallback_test.darttest/sync/domain/sync_repository_optimization_test.darttest/sync/domain/transaction_parse_batch_size_test.darttest/sync/utils/bounded_worker_pool_test.darttest/user/repositories/user_repository_test.dart
💤 Files with no reviewable changes (1)
- lib/main.dart
- refreshGateways awaited the SDK with no timeout; a stalled AR.IO/Solana RPC left gateway settings stuck in LoadingGateways with no recovery (CodeRabbit finding) - on timeout it throws so the bloc emits GatewaysError with its retry action; cache and persisted list are untouched on failure - add failure-path tests: error propagation preserves existing cache, stalled call throws TimeoutException Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…llback - drive-history GraphQL pagination now requests 1000 items per page on the primary gateway (turbo-gateway supports it; page size becomes the $pageSize query variable, config-driven via driveHistoryGqlPageSize), cutting round-trips up to 10x since cursor pagination is inherently sequential - pagination is an endpoint-sticky three-phase ladder: primary at the configured size, primary at 100 (rules out size-specific indexer failures such as turbo's rows-read limit on very large owners), then the Goldsky fallback at 100; a phase restarts the range and a seen-id set deduplicates, so cursors are never reused across gateways and the fallback is never asked for more than 100 items (it silently clamps larger pages while misreporting hasNextPage) - owners the primary cannot serve are remembered for the session so a multi-drive wallet pays the probing cost once, not per drive - clamp guard: a full page claiming hasNextPage=false when more than 100 was requested triggers a verification page instead of being trusted - fixes a pre-existing pagination bug: the cursor advanced from the ArFS-version-filtered list, so a page of only unsupported transactions reset the cursor to null and restarted the range - GraphQLRetry gains allowFallback / useFallbackEndpoint so paginated callers control endpoint stickiness; single-shot queries are unchanged - maxConcurrentDriveSyncs promoted from constant to AppConfig so sync pacing is tunable without a release; both new config fields default safely when absent from stored configs - add strategy pagination tests (ladder, clamp guard, dedup, owner memo, cursor-from-raw-edges) and config default tests Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
lib/services/arweave/arweave_service.dart (1)
155-165: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCache the
Futurerather than the value to avoid a lazy-init race.
_metadataCache ??= await ...has anawaitgap between the null-check and the assignment. With up to 5 concurrent sync workers, multiple calls can all see_metadataCacheas null, each creating a separateMetadataCache— only the last survives, and the others' in-memory entries are lost. Caching theFutureitself makes the??=assignment synchronous, so all callers share one initialization.🔒 Proposed fix: cache the Future, not the value
- MetadataCache? _metadataCache; - - Future<MetadataCache> _getMetadataCache() async { - _metadataCache ??= await MetadataCache.fromCacheStore( - await newSharedPreferencesCacheStore(), - ); - return _metadataCache!; - } + Future<MetadataCache>? _metadataCacheFuture; + + Future<MetadataCache> _getMetadataCache() { + return _metadataCacheFuture ??= _initMetadataCache(); + } + + Future<MetadataCache> _initMetadataCache() async { + return MetadataCache.fromCacheStore( + await newSharedPreferencesCacheStore(), + ); + }🤖 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/services/arweave/arweave_service.dart` around lines 155 - 165, Update _metadataCache to store a Future<MetadataCache> instead of a MetadataCache value, and in _getMetadataCache assign the initialization Future synchronously with ??= before awaiting it. Return await _metadataCache so concurrent callers share the same initialization and cache instance.test/services/arweave/data_gateway_fallback_test.dart (1)
216-225: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider using
FakeAsyncto avoid the 5-second real-time wait.The test waits 5 real seconds for
_garListTimeoutto fire. Wrapping withFakeAsyncwould allow instant time advancement, keeping the test fast.⚡ Proposed refactor using FakeAsync
- test('throws on a stalled SDK call instead of hanging', () async { - when(() => sdk.getGateways()).thenAnswer( - (_) => Completer<List<Gateway>>().future, // never completes - ); - - await expectLater( - fallback.refreshGateways(), - throwsA(isA<TimeoutException>()), - ); - }); + test('throws on a stalled SDK call instead of hanging', () { + when(() => sdk.getGateways()).thenAnswer( + (_) => Completer<List<Gateway>>().future, // never completes + ); + + fakeAsync(() { + expectLater( + fallback.refreshGateways(), + throwsA(isA<TimeoutException>()), + ); + fakeAsync.elapse(Duration(seconds: 5)); + }); + });Note: import
package:fake_async/fake_async.dartand wrap withFakeAsync().run((fakeAsync) { ... })if not usingflutter_test's built-infakeAsynchelper.🤖 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 `@test/services/arweave/data_gateway_fallback_test.dart` around lines 216 - 225, Replace the real-time timeout in the stalled SDK call test with FakeAsync, importing its package or using the available flutter_test helper. Wrap the setup and refresh invocation in FakeAsync, advance the fake clock beyond _garListTimeout, and then assert the returned future throws TimeoutException without waiting five seconds.lib/utils/graphql_retry.dart (1)
48-58: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffReuse the fallback
ArtemisClientacross paginated fallback reads
_paginate(... useFallbackEndpoint: true)callsexecute()once per page, and each call creates and disposes a new fallback client. Hoist that client out of the loop, or cache it onGraphQLRetry, so fallback pagination doesn’t churn a client per page.🤖 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/utils/graphql_retry.dart` around lines 48 - 58, Reuse a single fallback ArtemisClient across paginated reads instead of constructing and disposing one in every execute() call. Update GraphQLRetry pagination flow and the useFallbackEndpoint branch in execute() to hoist or cache the client, while preserving retry handling, disposal, and cleanup when pagination completes.
🤖 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.
Nitpick comments:
In `@lib/services/arweave/arweave_service.dart`:
- Around line 155-165: Update _metadataCache to store a Future<MetadataCache>
instead of a MetadataCache value, and in _getMetadataCache assign the
initialization Future synchronously with ??= before awaiting it. Return await
_metadataCache so concurrent callers share the same initialization and cache
instance.
In `@lib/utils/graphql_retry.dart`:
- Around line 48-58: Reuse a single fallback ArtemisClient across paginated
reads instead of constructing and disposing one in every execute() call. Update
GraphQLRetry pagination flow and the useFallbackEndpoint branch in execute() to
hoist or cache the client, while preserving retry handling, disposal, and
cleanup when pagination completes.
In `@test/services/arweave/data_gateway_fallback_test.dart`:
- Around line 216-225: Replace the real-time timeout in the stalled SDK call
test with FakeAsync, importing its package or using the available flutter_test
helper. Wrap the setup and refresh invocation in FakeAsync, advance the fake
clock beyond _garListTimeout, and then assert the returned future throws
TimeoutException without waiting five seconds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6b7f88c6-ced6-42c1-bd60-dd56c7a187a8
📒 Files selected for processing (15)
assets/config/dev.jsonassets/config/prod.jsonassets/config/staging.jsonlib/services/arweave/arweave_service.dartlib/services/arweave/data_gateway_fallback.dartlib/services/arweave/get_segmented_transaction_from_drive_strategy.dartlib/services/arweave/graphql/queries/DriveEntityHistoryWithEntityTypeFilter.graphqllib/services/config/app_config.dartlib/services/config/app_config.g.dartlib/sync/constants.dartlib/sync/domain/repositories/sync_repository.dartlib/utils/graphql_retry.darttest/services/arweave/data_gateway_fallback_test.darttest/services/arweave/get_segmented_transaction_from_drive_strategy_test.darttest/services/config/app_config_defaults_test.dart
💤 Files with no reviewable changes (1)
- lib/sync/constants.dart
✅ Files skipped from review due to trivial changes (1)
- lib/services/config/app_config.g.dart
🚧 Files skipped from review as they are similar to previous changes (5)
- assets/config/prod.json
- assets/config/dev.json
- assets/config/staging.json
- lib/services/arweave/data_gateway_fallback.dart
- lib/sync/domain/repositories/sync_repository.dart
- errors from a yield*'d async* stream are delivered to the listener as stream events and bypass the surrounding try/catch, so the pagination ladder never actually fell through to its next phase; phases now re-yield via await-for, where stream errors throw at the await point - fix config round-trip test to go through a real jsonEncode/jsonDecode cycle (AppConfig.toJson embeds SelectedGateway as an object) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Closes the silent-truncation family found in review: downstream, stream completion is treated as proof of completeness (sync watermarks advance, created snapshots claim their full range on-chain), so pagination must never end a range quietly. - empty page with hasNextPage=true now throws instead of breaking; the phase ladder retries on the next endpoint and a final failure surfaces as a failed drive (was: silently treated as end of data) - null-data responses with no errors also throw instead of breaking - clamp guard broadened: every non-empty final page of an oversized request gets one verification page, catching gateways that clamp to any size (Goldsky: 100, some forks: 10), not just >=100 - driveHistoryGqlPageSize clamped to 1..1000 at the consumption site so a bad config value cannot produce 'successfully empty' drives - owners are no longer marked fallback-preferring on connectivity loss (offline fails every endpoint; laddering would poison the session), and switching GraphQL endpoints clears the owner memo - config version bumps preserve gateway choices that differ from the previous defaults (deliberate user/detection choices survive the 2->3 migration instead of being silently reset) - folder revisions gain the same dateCreated guard files already had, so out-of-order arrival from phase restarts cannot regress the latest folder state - refreshBalance awaits the fetch so ProfileCubit re-emits the fresh value on the first refresh, shortening the post-login zero-balance window - bulk import records worker-stage failures in BulkImportResult.failures (WorkerPool passes the failed task; previously these files vanished from both result lists) - GraphQLRetry caches its fallback ArtemisClient (was: one client construct/dispose per page on the fallback path) - GAR gateway cache memoizes the in-flight future (concurrent first callers share one Solana RPC fetch) and treats an empty persisted list as not cached - tests updated for verification-page semantics; new tests: sub-100 clamp recovery, empty-page and null-data hard failures, gateway-choice preservation across config version bumps Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
vilenarios
force-pushed
the
fix/sync-quick-wins
branch
from
July 10, 2026 03:22
ba04082 to
6b714e4
Compare
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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)
test/services/config/config_fetcher_test.dart (1)
158-187: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winTest "replaces local config with asset config if local config has no version" is inconsistent with the new migration logic.
The
oldConfighasarweaveGatewayUrl: 'old-gateway'andarweaveGatewayForDataRequest: SelectedGateway(label: 'old', url: 'old'), both differing from the previous defaults ('https://ardrive.net','https://turbo-gateway.com') and the new defaults ('new-gateway','new'). The migration logic at lines 76-88 would preserve both viacopyWith, so the saved config would NOT equalnewConfigString. The assertion at line 186 (verify(() => localStore.putString('config', newConfigString)).called(1)) would fail.The updated test at lines 98-129 correctly uses
any()and asserts gateway preservation — this test needs the same treatment.🐛 Proposed fix: align test with migration behavior
// Act final result = await configFetcher.fetchConfig(Flavor.development); // Assert expect(result.configVersion, 2); expect(result.stripePublishableKey, 'new-key'); - verify(() => localStore.putString('config', newConfigString)).called(1); + expect(result.arweaveGatewayUrl, 'old-gateway'); + expect(result.arweaveGatewayForDataRequest.url, 'old'); + verify(() => localStore.putString('config', any())).called(1); });🤖 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 `@test/services/config/config_fetcher_test.dart` around lines 158 - 187, Update the test “replaces local config with asset config if local config has no version” to reflect migration preservation behavior: replace the exact newConfigString verification with a captured or any() persisted value, then assert the saved config preserves oldConfig’s arweaveGatewayUrl and arweaveGatewayForDataRequest while adopting the new config version and other expected fields, matching the approach used by the updated migration test.
🧹 Nitpick comments (1)
lib/authentication/ardrive_auth.dart (1)
437-448: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate
_updateBalance()withrefreshBalance().The new
refreshBalance()correctly awaits the balance fetch and handles errors. However,_updateBalance()(lines 375–383) duplicates the same fetch-update-emit logic in a fire-and-forget pattern and is still called from_addUser(line 368) andunlockUser(line 206). Consider replacing those_updateBalance()calls withrefreshBalance()(called without await where fire-and-forget is acceptable) to eliminate the duplication and ensure consistent error handling.♻️ Proposed consolidation
- void _updateBalance() { - _userRepository.getBalance(currentUser.wallet).then((value) { - _currentUser = _currentUser!.copyWith(walletBalance: value); - _userStreamController.add(_currentUser); - }).catchError((e) { - logger.e('Error fetching wallet balance', e); - // Don't update balance on error - keep previous value - }); - }Then replace call sites:
- _updateBalance(); + refreshBalance();🤖 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/authentication/ardrive_auth.dart` around lines 437 - 448, Consolidate the duplicate balance-refresh logic by removing `_updateBalance()` and using `refreshBalance()` instead. Update the call sites in `_addUser` and `unlockUser` to invoke `refreshBalance()` without awaiting where fire-and-forget behavior is intended, preserving the existing sequencing while reusing its awaited fetch, update, emit, and error handling.
🤖 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/core/arfs/use_cases/bulk_import_files.dart`:
- Around line 421-436: The onWorkerError callback in the bulk import flow loses
the original exception and treats cancellation as a regular file failure. Update
the WorkerPool/execute error propagation so onWorkerError receives or can
retrieve the thrown exception, record that exception’s message and stack trace
in FileImportFailure and logging, and handle BulkImportException with “Bulk
import cancelled” separately rather than adding it to normal failures.
---
Outside diff comments:
In `@test/services/config/config_fetcher_test.dart`:
- Around line 158-187: Update the test “replaces local config with asset config
if local config has no version” to reflect migration preservation behavior:
replace the exact newConfigString verification with a captured or any()
persisted value, then assert the saved config preserves oldConfig’s
arweaveGatewayUrl and arweaveGatewayForDataRequest while adopting the new config
version and other expected fields, matching the approach used by the updated
migration test.
---
Nitpick comments:
In `@lib/authentication/ardrive_auth.dart`:
- Around line 437-448: Consolidate the duplicate balance-refresh logic by
removing `_updateBalance()` and using `refreshBalance()` instead. Update the
call sites in `_addUser` and `unlockUser` to invoke `refreshBalance()` without
awaiting where fire-and-forget behavior is intended, preserving the existing
sequencing while reusing its awaited fetch, update, emit, and error handling.
🪄 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: 467c123f-0fe9-4f4c-9b89-0cfc2c4453ea
📒 Files selected for processing (11)
lib/authentication/ardrive_auth.dartlib/core/arfs/use_cases/bulk_import_files.dartlib/services/arweave/arweave_service.dartlib/services/arweave/data_gateway_fallback.dartlib/services/arweave/get_segmented_transaction_from_drive_strategy.dartlib/services/config/config_fetcher.dartlib/sync/domain/repositories/sync_repository.dartlib/utils/graphql_retry.darttest/services/arweave/get_segmented_transaction_from_drive_strategy_test.darttest/services/config/app_config_defaults_test.darttest/services/config/config_fetcher_test.dart
🚧 Files skipped from review as they are similar to previous changes (5)
- test/services/config/app_config_defaults_test.dart
- lib/services/arweave/data_gateway_fallback.dart
- lib/utils/graphql_retry.dart
- lib/services/arweave/get_segmented_transaction_from_drive_strategy.dart
- lib/sync/domain/repositories/sync_repository.dart
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 35 minutes. |
… to 15 Field test on a 21-drive wallet showed the branch slower than prod (82s vs 27s) with more GraphQL calls (120 vs 39). Two causes addressed: - verification pages fired for every non-empty final page of an oversized request; 19 of the 21 drives fit in a single page, so this was pure overhead for small drives. The guard now skips tails smaller than 10 raw edges: no known gateway clamps below 10 (Goldsky: 100, forks: 10), so a sub-10 tail cannot be a silent clamp. The Goldsky clamp-lie defense is unchanged for full-looking pages. - maxConcurrentDriveSyncs default raised 5 -> 15 (config-tunable). Per-drive sync is latency-bound for small drives, so 5-wide pacing serialized a previously fully-parallel 21-drive account into ~4 waves, dominating wall time. turbo comfortably served 21 concurrent full-range first:1000 probes in testing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Field testing on a 21-drive wallet showed 1000-item pages slower end-to-end in the app despite fast id-only probes: full TransactionCommon fragments make a 1000-node page a multi-megabyte response, parsed on the browser main thread, multiplied by concurrent drive syncs. Round-trip savings did not offset payload and parse cost. - driveHistoryGqlPageSize now defaults to 100 in the Dart model and all flavor configs; at 100 the wire behavior matches prod (single-phase pagination, no verification pages, which only apply above 100) - the pagination machinery is unchanged and still tested at 1000: the endpoint-sticky fallback ladder, clamp guard, seen-id dedup, and cursor fixes all remain, and larger pages stay one config edit away if future measurements justify them Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Final review flagged a resilience regression: phase A used 2 attempts regardless of page size, so at the default 100 a ~1s transient blip failed over to the fallback index and memoized the owner there for the entire session (prod rode out blips with 8 attempts of backoff). - primary-phase attempts now scale: 2 at oversized page sizes (failures there are deterministic indexer limits; fast downshift is right), 8 at the safe page size (failures there are transient; ride them out) - the owner fallback memo is cleared with the other sync-end caches, so a degraded sync heals on the next one instead of pinning the session to the fallback index Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Field measurements showed no end-to-end win from larger GraphQL pages (full-fragment payload and main-thread parse costs offset the saved round-trips), so the pagination machinery is removed rather than kept at parity: the strategy, GraphQLRetry, and the drive-history query return to their dev-proven behavior, and driveHistoryGqlPageSize is dropped from AppConfig and all flavor configs. Kept from the sync work (measured or correctness wins): - Solana RPC eliminated from sync (ArNS removal, GAR persistence) - login balance best-effort, 200+-drive batch clamp, bulk import fixes - GraphQL endpoints: turbo-gateway primary, Goldsky fallback (retained in the reverted GraphQLRetry), configVersion migration preserving custom gateways - bounded drive syncs via maxConcurrentDriveSyncs config (15) - folder revision dateCreated guard, awaited refreshBalance, MetadataCache reuse, GAR cache future-memoization The pagination work (endpoint-sticky ladder, clamp-lie defenses, cursor fixes) remains in branch history for a future data-driven retry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- 15-wide pacing split a 21-drive account into two waves, roughly doubling wall time vs prod's fully-parallel sync when per-drive work is latency-bound - 50 syncs virtually every real account in a single wave, matching prod-equivalent wall time, while still bounding pathological many-drive wallets (pool clamp allows up to 64 via config) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # assets/config/dev.json # lib/core/arfs/use_cases/bulk_import_files.dart
… PE-9129 calculateTransactionParseBatchSize divided the 200-tx budget by every remaining drive, but only maxConcurrentDriveSyncs drives sync at once. Once an account exceeds the concurrency bound the batch size under-shot badly (200 drives -> batch 1 instead of ~4), needlessly slowing large-account syncs — a case beyond what the branch was field-tested against (21 drives). Divide by min(remaining, maxConcurrent) so each concurrently-syncing drive gets its fair share of the budget; smaller accounts and the div-by-zero guards are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PsV2WFHZMGD9DUfX65cxuW
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
First pass of the sync/data-fetching remediation plan: the highest-impact, lowest-risk fixes identified in the end-to-end sync analysis. Each change is a separate commit with its own tests; none alters happy-path behavior.
Bug fixes
getUserawaitedgetWalletBalancewith no retry/timeout, locking out users with valid passwords during gateway blips. Balance is now best-effort (5s timeout, falls back to 0);ArDriveAuth._updateBalancerefreshes it asynchronously right after login as before.transactionParseBatchSizecomputed200 ~/ drivesCount, which reaches 0 at 201 drives and makesBatchProcessorthrow. ExtractedcalculateTransactionParseBatchSizeclamped to ≥ 1.BulkImportResult.importedFileswas always empty — WorkerPool discards the execute callback's return value; successful imports are now collected in the callback.Performance
runBoundedWorkershelper mirroringFuture.wait(eagerError: false)semantics — per-drive error handling, cancellation, and progress reporting unchanged.MetadataCacheconstructed once instead of rebuilt from SharedPreferences on every parsed batch of every drive.first: 100to theInfoOfTransactionsToBePinnedquery (it previously relied on the gateway's default page size, which is why the batch had to stay tiny), matching theTransactionStatuses/LicenseAssertionspattern.Test plan
flutter analyze: cleanscr setup🤖 Generated with Claude Code
Summary by CodeRabbit