Fix/stats sync - #212
Merged
Merged
Conversation
austin047
marked this pull request as ready for review
July 25, 2026 05:00
Code Review SummaryThis PR significantly hardens the synchronization engine by introducing failure classification (exponential backoff/quarantine) and a parking mechanism for items with missing local dependencies. It also refactors the Reports screen to use server-side statistics when available, while maintaining local fallbacks for offline or pending-sync scenarios. 🚀 Key Improvements
💡 Minor Suggestions
🚨 Critical Issues
|
![sourceant[bot]](https://avatars.githubusercontent.com/in/1161781?s=60&v=4){kind=small}
| (apiResponse.data as Map).cast<String, dynamic>()); | ||
| } | ||
|
|
||
| String _ymd(DateTime d) => |
There was a problem hiding this comment.
Using DateFormat from the intl package is generally preferred for standardized date-to-string conversion in this project, as it's cleaner and less prone to manual padding errors.
Suggested change
| String _ymd(DateTime d) => | |
| String _ymd(DateTime d) => DateFormat('yyyy-MM-dd').format(d); |
austin047
force-pushed
the
fix/stats-sync
branch
from
July 25, 2026 09:05
9590011 to
ca269bf
Compare
austin047
force-pushed
the
fix/stats-sync
branch
2 times, most recently
from
July 25, 2026 10:20
e00a5b8 to
b2c08f7
Compare
nfebe
approved these changes
Jul 25, 2026
…ently A change whose push failed once (HTTP 5xx, validation error) was marked errored and excluded from every future upload pass. A transaction gated on such a dependency (e.g. a custom category) then never reached the server, so server-side stats and web reported it as missing forever. Errored changes now re-enter the upload queue after a retry delay; dismissing a change from sync history still excludes it. Also scope the error write in concludeLocalChange by entityType so an errored entity cannot mark an unrelated change with the same entityId as failed. Refs #209
Reports and Month-in-Review counted transfer legs as income/expense, inflating both totals; web and /stats exclude them. Both calculators now skip transfer legs, covering unsynced transfers (transferClientId) as well — a hole the budget calculator shared and gets patched here too. Month-in-Review now anchors to the current month like web instead of walking back to the most recent active month, so both platforms recap the same period. Refs #209
Reports KPIs, the expense breakdown and ratios now render the server's /stats numbers when available, so mobile matches web including currency conversion. Daily-granularity charts and Month-in-Review stay locally computed from raw transactions, exactly as web's useReportData does. Local numbers win in three cases, without any persisted cache: the user has no account (their only truth, unlabeled), transactions are still waiting to sync (server totals would be missing them), or the fetch fails. The last two show a "Local estimate" note; a completed sync run triggers a refetch. Partial conversions surface the excluded currencies like web does. Refs #209
Category on the full form opened a bottom sheet containing a second collapsed dropdown; it is now the same in-form CustomDropdownSearch as the party field, validating the selected entity directly. Edit mode never pre-filled category or party (the party init sat in the new-transaction branch); both now pre-fill, and the compact layout passes initialValue so its autocomplete fields show them. Intent moves from choice chips to the same dropdown, without the search box or search icon, so it keeps the form's field height and scales as more intents are added.
The proposed-action card accepts one category instead of multi-select chips, sent as a one-element list so the overrides payload shape is unchanged. Wallet, party, category, wallet-type and enum fields all use CustomDropdownSearch, so options open below the field like the transaction form; party and category get a clear button in place of a "None" item.
…8 parking migration Sync had three compounding bugs causing entity types (especially transfers) to re-download on every cycle indefinitely: - The cursor advanced on last_synced_at instead of updated_at, the field the server actually filters synced_since against. When the two drift apart (deletes, clock skew), the incremental window never closes. getCursorTimestamp lets each handler report the correct field, with an optional cursorRewind to close same-second races. - TransferSyncHandler.upsertAllLocal was missing the empty-clientId upsert guard every other entity already had, so server-created transfers without a client ID collided at the '' primary key and blocked their own client-id reconciliation. - Client-id reconciliation treated any local row matching a server id as "already adopted," even one with an empty clientId — now it only skips rows with a real client id. Also adds schema v8: deferred_remote_items parks down-synced entities whose local dependencies aren't met yet (e.g. a transfer whose leg transaction hasn't synced) and retries them each cycle instead of silently dropping them past the cursor. shouldPersistLocal is the new per-entity hook driving this. Fixes a cast crash (`type 'Null' is not a subtype of type 'bool'`) on transaction snapshots queued by pre-v7 builds that predate the is_refund column — TransactionConverter now defaults it like it already does for intent. Merges origin/dev (schema v7: refunds, recurring transactions, reminders) as the base for the v8 migration.
…ion and quarantine
Client-id reconciliation previously claimed a server record via a full
entity PUT, so unrelated business validation (e.g. amount >= 0.01) could
permanently reject the claim and, because one failed item blocked the
whole phase, stall every entity type that depended on it. Two changes
fix this:
- claimClientId is a new handler hook sending a minimal
{client_id, updated_at} payload instead of the full entity — attaching
an id is bookkeeping, not a business update, so it shouldn't be
vetoable by unrelated field rules. Implemented for every entity type
whose server update endpoint verifies safe (all but notification,
which has no update endpoint).
- Failures are classified transient / permanent / unknown
(FailureClass). A permanent claim failure is reported and the item is
skipped, but no longer marks the phase as failed — sibling entity
types keep reconciling instead of being skipped as "dependencies not
synced."
Local changes that fail permanently are quarantined (attempt_count,
quarantined_at columns) instead of retrying forever on the same
cooldown.
Also points drift_sync_core at the published v0.3.0 tag, replacing the
local path dependency_override used while this work was in progress.
…eview UI - Replaces the flat 5-minute retry cooldown with exponential backoff (1 minute base, doubling, capped at 1 hour) driven by attempt_count, so a change that keeps failing backs off instead of hammering the server every cycle. - Adds a "Needs attention" section listing quarantined changes, with Retry (clears the error, lifts quarantine, resets attempts) and a details view. - Wraps the screen body in SafeArea with bottom padding so the last card isn't hidden behind the system navigation bar. - Removes the Dismiss action. It deleted the queued change but left the entity's local edit in place, silently diverging from the server until a later sync reverted it — a footgun most offline-sync clients (Firestore, Replicache) don't expose. Recovery is now Retry for transient failures, or editing/deleting the entity, which replaces the queued change cleanly with the corrected data.
- hasPendingTransactionChanges now also excludes quarantined changes,
not just dismissed ones. Quarantine (added earlier in this branch)
replaced Dismiss as the terminal state for a permanently-failed
change, but this method was never updated to know about it — a
quarantined transaction/transfer change kept the reports screen
stuck on "local estimate" forever with no way to clear it.
- getPendingLocalChanges pushes the dismissed filter into SQL instead
of loading and discarding rows in Dart; dismissed is a hard
exclusion with no other condition, so it belongs next to
quarantinedAt in the WHERE clause.
- retryBackoff caps its exponent at 6 instead of relying on the
`shift >= 30` guard: Dart compiled to JS truncates << to 32-bit
signed semantics, so attemptCount 17-30 could overflow and wrap
negative on web (this repo has a web/ target), silently disabling
backoff for a repeatedly-failing change instead of capping it.
- Reports date formatting now uses DateFormat('yyyy-MM-dd') instead of
hand-rolled zero-padding.
austin047
force-pushed
the
fix/stats-sync
branch
from
July 25, 2026 20:14
b2c08f7 to
78b0d31
Compare
| _authService = authService, | ||
| _db = db, | ||
| super(const ReportStatsState()) { | ||
| _syncSubscription = syncStream?.listen((syncState) { |
There was a problem hiding this comment.
To avoid redundant HTTP requests, the cubit should only trigger a reload when the synchronization state actually transitions from active to idle. The current implementation triggers a fetch on every sync state emission where isSynchronizing is false, which can happen multiple times (e.g., initial state, errors, or multiple consecutive idle states).
Suggested change
| _syncSubscription = syncStream?.listen((syncState) { | |
| _syncSubscription = syncStream?.map((s) => s.isSynchronizing).distinct().listen((isSynchronizing) { | |
| if (!isSynchronizing) load(state.periodDays); | |
| }); |
| width: double.infinity, | ||
| padding: EdgeInsets.all(8.r), | ||
| decoration: BoxDecoration( | ||
| color: Theme.of(context).colorScheme.surface, |
There was a problem hiding this comment.
Using colorScheme.surface as the background for the payload container may result in zero visual contrast if the dialog itself is already using the surface color. Consider using surfaceContainerHighest or a fixed slight opacity to make the monospace block clearly distinguishable from the dialog background.
Suggested change
| color: Theme.of(context).colorScheme.surface, | |
| color: Theme.of(context).colorScheme.surfaceContainerHighest, |
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
2 participants
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.
Description
Cursor update, transfer recovery, and failure resilience
Type of Change