Hotfix: sync data-loss fixes, exchange-rate integrity, and observability (v1.0.6) - #214
Conversation
Code Review SummaryThis hotfix successfully addresses several critical sync and data integrity issues. Key highlights include the truncation of datetimes to meet API requirements, a new reconciliation process for orphaned records, and improved Dio error observability. However, a critical reference to an undefined method in the crash reporting service must be fixed before merging. 🚀 Key Improvements
💡 Minor Suggestions
🚨 Critical Issues
|
![sourceant[bot]](https://avatars.githubusercontent.com/in/1161781?s=60&v=4){kind=small}
| final status = err.response?.statusCode; | ||
| if (shouldReport(status)) { | ||
| final options = err.requestOptions; | ||
| final key = '${options.method} ${options.uri.path} $status'; |
There was a problem hiding this comment.
The deduplication key uses options.uri.path. If your API paths include dynamic IDs (e.g., /transactions/uuid-123), the _reported set will grow indefinitely and fail to deduplicate the same error across different entities. Consider normalizing the path by replacing UUIDs/IDs with placeholders to improve deduplication effectiveness and prevent memory leaks in long-running sessions.
| final key = '${options.method} ${options.uri.path} $status'; | |
| // Simple normalization to strip UUIDs/IDs for better deduplication | |
| final normalizedPath = options.uri.path.replaceAll(RegExp(r'/[0-9a-fA-F-]{8,}|/\d+'), '/{id}'); | |
| final key = '${options.method} $normalizedPath $status'; |
1cae54e to
c6076e1
Compare
|
|
||
| /// The UI keys transfer presentation on the legs' transferClientId, so | ||
| /// legs that arrived before their transfer must be linked here. | ||
| Future<void> _linkLegs(Transfer entity) async { |
There was a problem hiding this comment.
Instead of performing two separate database updates in a loop, you can use a single update query with an IN clause for better performance. This reduces the number of database roundtrips during synchronization, which is especially important if this handler is invoked within a batch operation.
| Future<void> _linkLegs(Transfer entity) async { | |
| Future<void> _linkLegs(Transfer entity) async { | |
| if (entity.deletedAt != null || entity.clientId.isEmpty) return; | |
| final legIds = [ | |
| entity.expenseTransactionClientId, | |
| entity.incomeTransactionClientId, | |
| ].where((id) => id != null && id.isNotEmpty).cast<String>().toList(); | |
| if (legIds.isEmpty) return; | |
| await (db.update(db.transactions) | |
| ..where((t) => t.clientId.inIterable(legIds))) | |
| .write(TransactionsCompanion(transferClientId: Value(entity.clientId))); | |
| } |
c6076e1 to
c22b4e6
Compare
c22b4e6 to
8736883
Compare
| if (isConnectivityError(e)) { | ||
| return left(const NetworkFailure()); | ||
| } | ||
| logger.e('UnknownFailure', error: e, stackTrace: stackTrace); |
There was a problem hiding this comment.
When a DioException is not a connectivity error (e.g., a 422 Validation Error or 400 Bad Request), simply returning an UnknownFailure hides the server's error message from the user. Since this PR aims to fix 'sync 422 divergence', we should extract and display the server's response message if available.
| logger.e('UnknownFailure', error: e, stackTrace: stackTrace); | |
| final serverMessage = e.response?.data?['message']?.toString(); | |
| if (serverMessage != null) { | |
| return left(ServerFailure(serverMessage)); | |
| } | |
| logger.e('UnknownFailure', error: e, stackTrace: stackTrace); | |
| return left(const UnknownFailure()); |
| patched['slug'] ??= | ||
| ((patched['name'] as String?) ?? '').toLowerCase().replaceAll(' ', '-'); | ||
| patched['owner_type'] ??= 'user'; | ||
| final amount = patched['amount']; |
There was a problem hiding this comment.
The logic here seems inverted for legacy support. If the Budget entity (generated by Drift) expects a double for the amount field, converting a num to a String will cause a TypeError in Budget.fromJson. If legacy payloads sent amounts as strings, you should parse them into numbers instead.
| final amount = patched['amount']; | |
| final amount = patched['amount']; | |
| if (amount is String) patched['amount'] = double.tryParse(amount); |
| Expanded( | ||
| child: Text( | ||
| LocaleKeys.aiChatError.tr(), | ||
| state.failure?.maybeMap( |
There was a problem hiding this comment.
The logic for rendering the error message is redundant. maybeMap with orElse already handles the fallback, and the outer null-aware operator is duplicated by the internal orElse logic. Additionally, checking for networkError specifically while defaulting to aiChatError for others is fine, but can be simplified.
| state.failure?.maybeMap( | |
| state.failure?.maybeMap( | |
| networkError: (f) => f.customMessage, | |
| orElse: () => LocaleKeys.aiChatError.tr(), | |
| ) ?? '', |
There was a problem hiding this comment.
The 422 work is solid, but the bug #213 was filed for is still here: a transaction queued by an older build still cannot upload.
TransactionSyncHandler.unmarshal (transaction_sync_handler.dart:40) parses queued snapshots with the generated TransactionCompleteDto.fromJson. The guard in this PR is on fromServerJson, which only runs on downloads, so the queue path never sees it. On this branch:
snapshot without files: _TypeError: type 'Null' is not a subtype of type 'List<dynamic>' in type cast
transaction_complete_dto.g.dart 24:68
snapshot without categories: same, transaction_complete_dto.g.dart 14:39
classified as: unknown
unknown means ten retries before quarantine, so the change is stuck for days and then parked. Anything already quarantined by this crash will crash again the moment it is released.
Three changes close it:
@JsonKey(defaultValue: [])oncategoriesandfilesinTransactionCompleteDto, then regenerate. Cover it insync_snapshot_unmarshal_test.dartby removing the outer keys, not keys insidetransaction.- Classify an unmarshal
TypeErroras permanent. A payload that will not parse now will not parse on the tenth attempt. - Record the unmet dependency where
shouldPersistRemoterefuses (transfer_sync_handler.dart:51-77). That path returns without concluding or recording, so a blocked transfer sits in pending forever with nothing to explain it, and sync history has no way to show why.describeSyncErrorand the payload excerpt do not reach it; both only fire on a throw.
Smaller one: transaction_remote_datasource.dart:91 still sends the cursor as syncedSince.toIso8601String(). It is the only synced_since sender not going through formatServerIsoDateTimeString, the helper this PR changed to drop microseconds.
_linkLegs is better than what the issue asked for. Linking on every local write covers the claim path, the download path and the parked retry, so it no longer depends on the claim beating the download. Suite is green on Flutter 3.38.9.
Unverified: whether a web-created transfer now shows up on a device. Nothing in the PR covers it, and a sync-history capture plus the local transfers rows would settle it.
The body says Closes #213. On this branch it closes two of eight items, so either add the three above or drop the close and split them out.
| #213 item | Status |
|---|---|
Default categories/files in fromJson, test with outer keys stripped |
No |
| Guard the same cast on the server-response path | Yes |
| Classify an unmarshal failure as permanent | No |
| Confirm already-quarantined changes unmarshal after the guard | No |
| Record and show what a dependency-blocked change waits on | No |
| Reproduce a web-created transfer on a device | Unverified |
Backfill transferClientId on the legs |
Yes |
| Normalise the transactions cursor | No |
… records Server datetimes are truncated to milliseconds (the API rejects Dart's 6-digit microseconds with 422), multipart booleans go as 1/0, transactions post as JSON unless files are attached, budgets marshal round-trippably for the local change queue (with a patch for legacy server-shaped payloads), and transactions tolerate payloads without categories. A reconciliation sweep before each sync cycle re-enqueues records that have no server id and no local_changes row, and transfer upserts backfill transferClientId on their legs so legs that arrive before their transfer stop rendering as editable plain transactions.
…ontext Dio errors recorded to Crashlytics now include method, URL, status and a truncated response body (never headers), the unhandled-error hook routes through the same enrichment, and a Dio interceptor reports 5xx and 422 responses as non-fatals deduplicated per method+path+status per session.
Restores the sync-history quarantine strings that existed only in the generated keys file, and adds keys for the budget card, transfer rate validation and the gated currency switch.
Connection-level Dio failures map to the network failure message instead of a generic error (Financial Position, Holdings, Ask Trakli). Exchange-rate refreshes never crash the stream and are fetched right after login; a stale cached rate survives a failed refresh. Cross- currency transfers require an explicit rate when none is known (empty field, blocked submit) instead of a silent 1:1. Switching an established default currency is gated on rate availability for the new base, except during a total outage when the current base has no rates either; the first selection stays ungated so onboarding cannot block.
Status chip, scope pills, status-colored progress with net spent of effective limit and percent, and a remaining footer with refunds chip, matching BudgetCard.vue. Removes the drawer's duplicate budget entry now that Budgets is a bottom-navigation tab.
drift_sync_core v0.3.2 enqueues pending changes before remote attempts and attaches queued payloads to upload crash reports. The v1.0.5 tag shipped still reading 1.0.4+4, so installed builds under-reported their version.
8736883 to
f01fa6d
Compare
|
|
||
| PlatformDispatcher.instance.onError = (error, stack) { | ||
| FirebaseCrashlytics.instance.recordError(error, stack, fatal: true); | ||
| unawaited(_record(error, stackTrace: stack, fatal: true)); |
There was a problem hiding this comment.
The method _record is not defined in this class. Based on the class definition and the CrashReportingInterface, you should call recordError instead. This is critical as it will cause a crash when a global error occurs.
| unawaited(_record(error, stackTrace: stack, fatal: true)); | |
| unawaited(recordError(error, stackTrace: stack, fatal: true)); |
nfebe
left a comment
nfebe
left a comment
There was a problem hiding this comment.
Can we match this : https://github.com/trakli/webservice/releases/tag/v2.0.0-beta.1 for the release ?
Since the mobile must work with a particular web version
2 participants
Problem
Since v1.0.5 Tansactions and transfers created on devices have been silently failing to sync: the API rejects Dart's 6-digit-microsecond datetimes with 422, and any non-network upload failure skipped the pending-change write completely, leaving records stranded locally with no retry, no quarantine entry, and no visibility anywhere else. Several related defects were found and fixed along the way.
Changes
Datetimes truncated to milliseconds (API contract), multipart booleans as 1/0, transactions post as JSON unless files are attached, budgets marshal round-trippably for the queue (legacy payload patch included), and transactions tolerate payloads without categories.
Now a reconciliation sweep re-enqueues orphaned records before each sync cycle, and when a transfer is written locally, its expense and income transactions are tagged with the transfer's client id — previously, legs that arrived before their transfer rendered as standalone transactions with edit and delete enabled.
Crashlytics Dio reports carry method/URL/status/body (never headers); a Dio interceptor reports 5xx and 422 as deduplicated non-fatal errors.
Restores sync-history quarantine strings missing from every locale; adds keys for the new UI.
Connection failures show the network message instead of a generic error; exchange-rate refreshes are generally non-fatal and fetched at login; a stale cached rate survives a failed refresh; cross-currency transfers require an explicit rate when none is known;
Switching the default currency is blocked (loader + snackbar, nothing changes) when rates for the new base are unavailable, the first selection exempt so onboarding never blocks.
Budget card aligned with the web (status chip, scope pills, progress, remaining/refunds);
Budget duplicate drawer entry removed.
Closes #213