60s delay from the first device's connectes to activate wals, 120 mis… #996
Conversation
…s seconds as threshold before displaying the 'sync now' to user.
|
WalkthroughThis update enhances the handling of Write-Ahead Logs (WALs) by introducing a threshold-based 'sync now' option and simplifying missing WALs calculation. It also improves device connection time tracking and codec type-based WALS feature enablement. Lastly, it adds a check for 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 (5)
- app/lib/pages/memories/page.dart (2)
- app/lib/pages/onboarding/wrapper.dart (1)
- app/lib/providers/capture_provider.dart (3)
- app/lib/providers/memory_provider.dart (4)
- app/lib/services/devices.dart (3)
Files skipped from review due to trivial changes (1)
- app/lib/services/devices.dart
Review comments generated (10)
- Review: 5
- LGTM: 5
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.
| if (mounted) { | ||
| context.read<HomeProvider>().setupHasSpeakerProfile(); | ||
| _goNext(); | ||
| } |
There was a problem hiding this comment.
Good addition of the mounted check before executing setup functions. This will prevent potential exceptions that could occur if these methods are called after the widget is disposed.
+ if (mounted) {
+ context.read<HomeProvider>().setupHasSpeakerProfile();
+ _goNext();
+ }This change ensures that the setup functions are only called when the widget is still active in the widget tree, which improves the robustness of the code.
| @@ -79,7 +79,7 @@ class _MemoriesPageState extends State<MemoriesPage> with AutomaticKeepAliveClie | |||
| child: CustomScrollView( | |||
| slivers: [ | |||
| SliverToBoxAdapter( | |||
| child: memoryProvider.missingWals.isNotEmpty | |||
| child: memoryProvider.missingWalsInSeconds > 120 | |||
There was a problem hiding this comment.
The change from checking if missingWals is not empty to checking if missingWalsInSeconds is greater than 120 seconds seems to be a significant logic change. Ensure that this new condition aligns with the intended functionality. If the intention is to only show the 'sync now' option when there are more than 120 seconds of unsynced data, then this change is correct.
- child: memoryProvider.missingWals.isNotEmpty
+ child: memoryProvider.missingWalsInSeconds > 120| @@ -95,7 +95,7 @@ class _MemoriesPageState extends State<MemoriesPage> with AutomaticKeepAliveClie | |||
| child: ListTile( | |||
| leading: const Icon(Icons.record_voice_over_rounded), | |||
| title: Text( | |||
| 'You have ${secondsToHumanReadable(memoryProvider.missingWals.map((val) => val.seconds).reduce((a, b) => a + b))} of conversation locally, sync now?', | |||
| 'You have ${secondsToHumanReadable(memoryProvider.missingWalsInSeconds)} of conversation locally, sync now?', | |||
There was a problem hiding this comment.
The change in how the human-readable time for missing WALs is calculated simplifies the code and makes it easier to understand. However, ensure that missingWalsInSeconds is always updated correctly whenever a new WAL is added or an existing one is removed or modified.
- 'You have ${secondsToHumanReadable(memoryProvider.missingWals.map((val) => val.seconds).reduce((a, b) => a + b))} of conversation locally, sync now?'
+ 'You have ${secondsToHumanReadable(memoryProvider.missingWalsInSeconds)} of conversation locally, sync now?'| bool _walFeatureEnabled = true; | ||
| IWalService get _walService => ServiceManager.instance().wal; | ||
| IDeviceService get _deviceService => ServiceManager.instance().device; |
There was a problem hiding this comment.
The renaming of _wal to _walService and the introduction of _deviceService are good changes for clarity and maintainability. However, I would suggest adding null checks before using these services throughout your code to prevent potential null pointer exceptions.
- IWalService get _walService => ServiceManager.instance().wal;
- IDeviceService get _deviceService => ServiceManager.instance().device;
+ IWalService? get _walService => ServiceManager.instance().wal;
+ IDeviceService? get _deviceService => ServiceManager.instance().device;| // support: opus codec, 1m from the first device connectes | ||
| var deviceFirstConnectedAt = _deviceService.getFirstConnectedAt(); | ||
| var isWalEnabled = codec == BleAudioCodec.opus && | ||
| (deviceFirstConnectedAt != null && | ||
| deviceFirstConnectedAt.isBefore(DateTime.now().subtract(const Duration(seconds: 60)))) && | ||
| _walFeatureEnabled; | ||
| if (isWalEnabled) { | ||
| _walService.onByteStream(value); | ||
| } | ||
|
|
||
| // send ws | ||
| if (_socket?.state == SocketServiceState.connected) { | ||
| final trimmedValue = value.sublist(3); | ||
| _socket?.send(trimmedValue); | ||
|
|
||
| // synced | ||
| if (isWalSupported) { | ||
| _wal.onBytesSync(value); | ||
| if (isWalEnabled) { | ||
| _walService.onBytesSync(value); |
There was a problem hiding this comment.
The logic here is a bit complex and could be simplified for better readability. Also, it's important to handle the case when getFirstConnectedAt() returns null. Here's a suggested refactor:
- var deviceFirstConnectedAt = _deviceService.getFirstConnectedAt();
- var isWalEnabled = codec == BleAudioCodec.opus &&
- (deviceFirstConnectedAt != null &&
- deviceFirstConnectedAt.isBefore(DateTime.now().subtract(const Duration(seconds: 60)))) &&
- _walFeatureEnabled;
- if (isWalEnabled) {
- _walService.onByteStream(value);
- }
-
- // send ws
- if (_socket?.state == SocketServiceState.connected) {
- final trimmedValue = value.sublist(3);
- _socket?.send(trimmedValue);
-
- // synced
- if (isWalEnabled) {
- _walService.onBytesSync(value);
- }
- }
+ var deviceFirstConnectedAt = _deviceService?.getFirstConnectedAt();
+ var isWalEnabled = _walFeatureEnabled && codec == BleAudioCodec.opus &&
+ deviceFirstConnectedAt?.isBefore(DateTime.now().subtract(const Duration(seconds: 60))) == true;
+
+ if (isWalEnabled) {
+ _walService?.onByteStream(value);
+ }
+
+ if (_socket?.state == SocketServiceState.connected) {
+ final trimmedValue = value.sublist(3);
+ _socket?.send(trimmedValue);
+
+ if (isWalEnabled) {
+ _walService?.onBytesSync(value);
+ }
+ }
…s seconds as threshold before displaying the 'sync now' to user.
Summary by Entelligence.AI
mountedin the onboarding wrapper to prevent potential errors during setup functions execution.