Increase the local sync's ready to flush seconds to 120 #1014
Conversation
|
WalkthroughThis update enhances the synchronization progress display by showing it as an integer percentage value. It also refactors the timer handling logic in the Changes
Uplevel your code reviews with Entelligence.AI ProEntelligence.AI ProIf you like this project, please support us by purchasing the Pro version. The Pro version has advanced context, superior noise reduction and several proprietary improvements compared to the open source version. Moreover, Entelligence.AI Pro is free for open source projects. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Files selected (2)
- app/lib/pages/memories/sync_page.dart (1)
- app/lib/pages/memories/widgets/local_sync.dart (4)
Review comments generated (5)
- Review: 4
- LGTM: 1
Tips
Chat with Entelligence.AI Bot (@Entelligence.AI)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@Entelligence.AIin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@Entelligence.AI: ignoreanywhere in the PR description to pause further reviews from the bot.
| @@ -76,7 +76,7 @@ class _SyncPageState extends State<SyncPage> { | |||
| ), | |||
| memoryProvider.isSyncing | |||
| ? Text( | |||
| '${memoryProvider.walsSyncedProgress}% synced', | |||
| '${(memoryProvider.walsSyncedProgress * 100).toInt()}% synced', | |||
There was a problem hiding this comment.
The change from displaying the sync progress as a floating-point number to an integer is a good one for readability. However, it might lead to less precision in the progress display. If the syncing process is slow, users might not see the progress update for a while because the percentage will only increase when it reaches the next whole number. Consider keeping the floating-point precision up to 1 or 2 decimal places.
- '${(memoryProvider.walsSyncedProgress * 100).toInt()}% synced',
+ '${(memoryProvider.walsSyncedProgress * 100).toStringAsFixed(1)}% synced',| bool _missSecondsInEstTimerEnabled = false; | ||
| int _missSeconds = 0; | ||
|
|
||
| @override | ||
| void initState() { | ||
| super.initState(); | ||
|
|
||
| _missSecondsInEstTimer = Timer.periodic(const Duration(seconds: 1), (t) { | ||
| if (_missSecondsInEstTimerEnabled) { | ||
| setState(() { | ||
| _missSeconds++; | ||
| }); | ||
| } | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
The timer is being initialized in the initState method and it runs every second regardless of whether it's needed or not. This could lead to unnecessary CPU usage. It would be better to start the timer only when it's needed and cancel it when it's not.
- _missSecondsInEstTimer = Timer.periodic(const Duration(seconds: 1), (t) {
- if (_missSecondsInEstTimerEnabled) {
- setState(() {
- _missSeconds++;
- });
- }
- });
+ // Move timer initialization to a separate method
+ void startTimer() {
+ _missSecondsInEstTimer = Timer.periodic(const Duration(seconds: 1), (t) {
+ if (_missSecondsInEstTimerEnabled) {
+ setState(() {
+ _missSeconds++;
+ });
+ }
+ });
+ }| if (provider.missingWalsInSeconds >= 120) { | ||
| _status = LocalSyncStatus.flush; | ||
| } else if (!captureProvider.isWalSupported) { | ||
| _status = LocalSyncStatus.disabled; | ||
| } else if (provider.missingWalsInSeconds > 0) { | ||
| } else if (!captureProvider.transcriptServiceReady && captureProvider.recordingDeviceServiceReady) { |
There was a problem hiding this comment.
The condition for setting _status to LocalSyncStatus.flush has been changed from provider.missingWalsInSeconds > 120 to provider.missingWalsInSeconds >= 120. This change aligns with the PR title, so it seems correct. However, please ensure that this change doesn't introduce any off-by-one errors in your logic.
| _missSecondsInEstTimerEnabled = true; | ||
| } else { | ||
| _missSecondsInEstTimer?.cancel(); | ||
| _missSecondsInEstTimer = null; | ||
| _missSecondsInEstTimerEnabled = false; |
There was a problem hiding this comment.
The timer control logic has been simplified by introducing the _missSecondsInEstTimerEnabled flag. This is a good approach as it makes the code easier to read and understand. However, remember to call startTimer() and cancelTimer() methods appropriately based on the _missSecondsInEstTimerEnabled flag value.
- _missSecondsInEstTimerEnabled = true;
+ if (!_missSecondsInEstTimerEnabled) {
+ _missSecondsInEstTimerEnabled = true;
+ startTimer();
+ }- _missSecondsInEstTimerEnabled = false;
+ if (_missSecondsInEstTimerEnabled) {
+ _missSecondsInEstTimerEnabled = false;
+ cancelTimer();
+ }
Summary by Entelligence.AI