Skip to content

Conversation

@nfebe
Copy link
Contributor

@nfebe nfebe commented Dec 26, 2025

Syncrhonization bugs and improvements on transaction form

@nfebe nfebe requested a review from austin047 December 26, 2025 14:48
@sourceant
Copy link

sourceant bot commented Dec 26, 2025

Code Review Summary

This pull request introduces several important improvements across the synchronization core and related UI components. Key enhancements include robust error handling, efficient data fetching through pagination, improved dependency sorting for local changes, and better consistency in UI interactions when adding new entities. The overall changes make the synchronization process more reliable, performant, and maintainable.

🚀 Key Improvements

  • Implemented pagination in all *RemoteDataSource getAll* methods, drastically improving performance and resource management for large datasets.
  • Ensured proper awaiting of SynchAppDatabase().doSync() calls in AppWidget and OnboardSettingsScreen for reliable synchronization flow.
  • Improved dependency sorting for PendingLocalChange entities in DriftSynchronizer's uploadLocalChanges, ensuring entities are uploaded in the correct order based on their relationships.
  • Fixed a critical bug in CategorySyncHandler.getLocalByClientId that caused a recursive call, replacing it with a correct database query.
  • Enhanced user experience in AddTransactionForm by automatically updating selected wallets, parties, and categories after adding new ones, and filtering available wallets by selected currency.

💡 Minor Suggestions

  • While the ConfigSyncHandler currently uses an int for TServerKey and converts it to String for remote calls, a refactoring to declare TServerKey as String directly would provide better type consistency and clarity, as configuration entities are primarily identified by a String key remotely.

@sourceant
Copy link

sourceant bot commented Dec 26, 2025

🔍 Code Review

💡 1. **lib/data/sync/config_sync_handler.dart** (Lines 57-59) - REFACTOR

Following the change of TServerKey to String for ConfigSyncHandler, the restGetRemote method signature must be updated to accept a String identifier, aligning with the ConfigRemoteDataSource's getConfig(String key) method.

Suggested Code:

  @override
  Future<Config?> restGetRemote(String key) async {
    return await remoteDataSource.getConfig(key);
  }

Current Code:

  @override
  Future<Config?> restGetRemote(int id) async {
      // return await remoteDataSource.getConfig(id);
    return await remoteDataSource.getConfig(id.toString());
  }

Verdict: APPROVE

Posted as a comment because posting a review failed.

@nfebe nfebe force-pushed the fix/sync-and-ux-bugs branch from 5c9ecea to 4875ab1 Compare December 26, 2025 14:50
Copy link

@sourceant sourceant bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review complete. See the overview comment for a summary.

@nfebe nfebe force-pushed the fix/sync-and-ux-bugs branch from dcb3e9c to a443fd3 Compare December 26, 2025 15:31
Copy link

@sourceant sourceant bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review complete. See the overview comment for a summary.

@nfebe nfebe force-pushed the fix/sync-and-ux-bugs branch from 4aafb37 to c60d164 Compare December 26, 2025 15:56
@nfebe nfebe force-pushed the fix/sync-and-ux-bugs branch from 8019ce6 to f368a38 Compare December 27, 2025 09:11
@nfebe nfebe requested a review from austin047 December 27, 2025 09:12
nfebe added 13 commits December 27, 2025 14:58
Previously, remote datasources only fetched the first page of results
during sync, causing data loss when more than 20 items needed syncing.
Now all datasources loop through all pages using PaginationResponse.hasMore.

Signed-off-by: nfebe <fenn25.fn@gmail.com>
Changed full sync strategy from delete-then-upsert to upsert-then-delete.
This ensures existing local data is preserved if the upsert operation fails.

Added deleteLocalNotIn method to SyncTypeHandler interface and all
implementations to safely remove only items not in the server response.

Signed-off-by: nfebe <fenn25.fn@gmail.com>
Wrapped unawaited sync calls with error logging to prevent silent
failures. Sync errors are now logged instead of being swallowed.

Signed-off-by: nfebe <fenn25.fn@gmail.com>
Entities skipped due to missing dependencies are now logged for
debugging. The change stays in queue for retry on next sync cycle.

Signed-off-by: nfebe <fenn25.fn@gmail.com>
Entities with no dependencies (wallet, category, party, group) are now
uploaded before dependent entities (transaction). This prevents 403
errors when a transaction references entities not yet on the server.

Signed-off-by: nfebe <fenn25.fn@gmail.com>
Related wallet/party/group entities are now only inserted if they
don't exist locally, preventing overwrite of recent local changes.

Signed-off-by: nfebe <fenn25.fn@gmail.com>
The FAB now uses the user's configured default wallet when opening
the add transaction screen, instead of the carousel's current index.

Signed-off-by: nfebe <fenn25.fn@gmail.com>
When a currency is selected, only show wallets that use that currency.
Also clears wallet selection if its currency doesn't match the new one.

Signed-off-by: nfebe <fenn25.fn@gmail.com>
When adding a new wallet, category, or party via the + button,
the form now auto-selects the newly created item on return.

Signed-off-by: nfebe <fenn25.fn@gmail.com>
Ensures newly created configs, wallets, and groups are synced to
the server when first-time users complete the onboarding flow.

Signed-off-by: nfebe <fenn25.fn@gmail.com>
Prevents ConstraintError when related entities exist by serverId but
not by clientId. Uses insertOrReplace as fallback for edge cases.

Also fixes:
- setAmountController crash when creating new transactions
- getLocalByClientId infinite recursion in CategorySyncHandler

Signed-off-by: nfebe <fenn25.fn@gmail.com>
- Remove redundant serverId check in _doOperation (both branches identical)
- Memoize getDependencyDepth results to avoid redundant calculations

Signed-off-by: nfebe <fenn25.fn@gmail.com>
- Move sync error logging to SyncEntityRepository base class
- Remove redundant _syncWithErrorHandling wrapper from TransactionRepository
- Remove unnecessary existence checks before insertOrReplace
  (insertOrReplace handles both PK and UNIQUE constraint conflicts)

Signed-off-by: nfebe <fenn25.fn@gmail.com>
@nfebe nfebe force-pushed the fix/sync-and-ux-bugs branch from f368a38 to 076aa69 Compare December 27, 2025 13:58
nfebe and others added 3 commits December 28, 2025 20:35
- Await doSync() to complete before onboarding check
- Check for existing group/wallet by name before creating new ones
- Set existing entity as default if found during onboarding
- Improve sync history display for config entity type
@austin047 austin047 merged commit 45259bb into main Dec 29, 2025
5 checks passed
@austin047 austin047 deleted the fix/sync-and-ux-bugs branch December 29, 2025 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants