Improve offline sync + fix sync system issues#3737
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new SyncBottomSheet widget. While visually well-structured, there are opportunities to improve the architecture by moving business logic and service calls out of the UI widget and into the SyncProvider to enhance maintainability and testability. Additionally, a duplicated calculation should be refactored to improve efficiency and reduce redundancy, aligning with principles of consistent value usage.
| final hasOrphanedFiles = ServiceManager.instance().wal.getSyncs().flashPage.hasOrphanedFiles; | ||
| final orphanedCount = ServiceManager.instance().wal.getSyncs().flashPage.orphanedFilesCount; | ||
|
|
||
| // Calculate time ago for pending data | ||
| String timeAgo = ''; | ||
| if (hasPendingData && pendingFlashPages.isNotEmpty) { | ||
| final oldestWal = pendingFlashPages.reduce((a, b) => a.timerStart < b.timerStart ? a : b); | ||
| final minutesAgo = ((DateTime.now().millisecondsSinceEpoch ~/ 1000) - oldestWal.timerStart) ~/ 60; | ||
| if (minutesAgo < 60) { | ||
| timeAgo = '$minutesAgo minutes ago'; | ||
| } else if (minutesAgo < 1440) { | ||
| timeAgo = '${minutesAgo ~/ 60} hours ago'; | ||
| } else { | ||
| timeAgo = '${minutesAgo ~/ 1440} days ago'; | ||
| } | ||
| } |
There was a problem hiding this comment.
To maintain a clear separation of concerns and improve testability, business logic and service calls should be handled within the SyncProvider rather than directly in the widget's build method.
Currently, ServiceManager is accessed directly to get information about orphaned files (lines 38-39), calculate timeAgo (lines 41-53), and to trigger uploads (line 320). This logic should be encapsulated within the SyncProvider.
Please consider the following refactoring:
- Add getters for
hasOrphanedFilesandorphanedCounttoSyncProvider. - Add a method
uploadOrphanedFiles()toSyncProvider. - Move the
timeAgocalculation logic into a getter inSyncProvider. Also, the check on line 43if (hasPendingData && pendingFlashPages.isNotEmpty)is redundant, ashasPendingDatais alreadypendingFlashPages.isNotEmpty.
In SyncProvider:
bool get hasOrphanedFiles => _walService.getSyncs().flashPage.hasOrphanedFiles;
int get orphanedFilesCount => _walService.getSyncs().flashPage.orphanedFilesCount;
Future<void> uploadOrphanedFiles() async {
await _walService.getSyncs().flashPage.uploadOrphanedFiles();
// You might need to refresh state and notify listeners after this.
await refreshWals();
}
String get oldestPendingWalTimeAgo {
final pendingFlashPages = allWals.where((w) => w.storage == WalStorage.flashPage && w.status == WalStatus.miss).toList();
if (pendingFlashPages.isEmpty) return '';
final oldestWal = pendingFlashPages.reduce((a, b) => a.timerStart < b.timerStart ? a : b);
final nowInSeconds = DateTime.now().millisecondsSinceEpoch ~/ 1000;
final minutesAgo = (nowInSeconds - oldestWal.timerStart) ~/ 60;
if (minutesAgo < 60) {
return '$minutesAgo minutes ago';
} else if (minutesAgo < 1440) { // 24 * 60
return '${minutesAgo ~/ 60} hours ago';
} else {
return '${minutesAgo ~/ 1440} days ago';
}
}Then, in SyncBottomSheet, you can access this data from syncProvider and simplify the build method significantly. This makes the widget more declarative and easier to maintain.
References
- When separating UI and provider logic, provider methods (e.g.,
clearFilters,removeFilter) should handle state management, while the UI layer is responsible for invoking subsequent asynchronous actions (e.g.,applyFiltersfor server-side updates) after the state has been modified.
| TextSpan( | ||
| text: _formatDuration(pendingFlashPages.fold(0, (sum, w) => sum + w.seconds)), | ||
| style: const TextStyle(color: Colors.deepPurpleAccent), | ||
| ), | ||
| const TextSpan(text: ' of offline recordings to sync.'), |
There was a problem hiding this comment.
The calculation for the total duration of pending recordings, pendingFlashPages.fold(0, (sum, w) => sum + w.seconds), is performed twice in the build method (here on line 148 and again on line 232).
To avoid redundant computation and improve code clarity and maintainability, you should calculate this value once at the beginning of the build method and store it in a variable.
For example, you can add this after line 35:
final totalPendingSeconds = pendingFlashPages.fold<int>(0, (sum, w) => sum + w.seconds);Then you can use _formatDuration(totalPendingSeconds) in both places. This makes the code more efficient and ensures that if the calculation logic changes, it only needs to be updated in one spot.
References
- Ensure consistency in constant values used for calculations across different parts of the codebase, especially when those calculations affect user-facing metrics like progress reporting. Define a single, well-defined constant and reuse it to avoid inaccuracies.
|
/gemini summary |
Summary of ChangesThis pull request delivers a comprehensive overhaul of the offline synchronization system, primarily targeting Limitless devices. It provides users with a clearer view of their sync status through a new dedicated bottom sheet and significantly improves the reliability of data transfer from the device to the cloud. The changes ensure that recordings are safely stored locally before upload, with mechanisms for recovering from interruptions, and that backend processing accurately reflects the conversation timelines and origins. Highlights
Changelog
Activity
|
ScreenRecording_12-13-2025.14-07-22_1.MP4