Conversation
Contributor
|
WalkthroughThis update introduces local synchronization capabilities, internet connection status monitoring, and a new time format conversion utility. It also includes UI enhancements for managing the local sync feature and modifies the WebSocket service pool's start method. 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}
Contributor
There was a problem hiding this comment.
Files selected (9)
- app/lib/backend/preferences.dart (1)
- app/lib/main.dart (2)
- app/lib/pages/memories/page.dart (3)
- app/lib/pages/memories/widgets/local_sync.dart (1)
- app/lib/pages/settings/developer.dart (1)
- app/lib/providers/capture_provider.dart (8)
- app/lib/providers/developer_mode_provider.dart (3)
- app/lib/services/sockets.dart (1)
- app/lib/utils/other/string_utils.dart (1)
Files skipped from review due to trivial changes (3)
- app/lib/pages/settings/developer.dart
- app/lib/services/sockets.dart
- app/lib/utils/other/string_utils.dart
Review comments generated (22)
- Review: 17
- 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.
Comment on lines
+19
to
+26
| bool localSyncEnabled = false; | ||
|
|
||
| void initialize() { | ||
| gcpCredentialsController.text = SharedPreferencesUtil().gcpCredentials; | ||
| gcpBucketNameController.text = SharedPreferencesUtil().gcpBucketName; | ||
| webhookOnMemoryCreated.text = SharedPreferencesUtil().webhookOnMemoryCreated; | ||
| webhookOnTranscriptReceived.text = SharedPreferencesUtil().webhookOnTranscriptReceived; | ||
| localSyncEnabled = SharedPreferencesUtil().localSyncEnabled; |
Contributor
There was a problem hiding this comment.
The initialize() method is not asynchronous, but it's calling SharedPreferencesUtil() which might involve disk I/O operations. This could potentially block the main thread and degrade the app performance. Consider making this method asynchronous and use await with SharedPreferencesUtil() calls.
- void initialize() {
+ Future<void> initialize() async {
gcpCredentialsController.text = SharedPreferencesUtil().gcpCredentials;
gcpBucketNameController.text = SharedPreferencesUtil().gcpBucketName;
webhookOnMemoryCreated.text = SharedPreferencesUtil().webhookOnMemoryCreated;
webhookOnTranscriptReceived.text = SharedPreferencesUtil().webhookOnTranscriptReceived;
localSyncEnabled = await SharedPreferencesUtil().localSyncEnabled;
notifyListeners();
}
Comment on lines
+75
to
+78
| void onLocalSyncEnabledChanged(var value) { | ||
| localSyncEnabled = value; | ||
| notifyListeners(); | ||
| } |
Contributor
There was a problem hiding this comment.
The onLocalSyncEnabledChanged method does not validate the input value before assigning it to localSyncEnabled. It's a good practice to validate inputs in setter methods to prevent potential issues. If value is expected to be a boolean, consider adding a type check.
- void onLocalSyncEnabledChanged(var value) {
+ void onLocalSyncEnabledChanged(bool value) {
if (value is bool) {
localSyncEnabled = value;
notifyListeners();
} else {
throw ArgumentError('Expected a boolean value');
}
}
Comment on lines
400
to
403
| set wals(List<Wal> wals) { | ||
| final List<String> value = wals.map((e) => jsonEncode(e.toJson())).toList(); | ||
| saveStringList('v3/wals', value); | ||
| saveStringList('wals', value); | ||
| } |
Contributor
There was a problem hiding this comment.
The setter for wals is missing null safety checks. If a null list is passed, it will cause a NullPointerException at the map function.
- set wals(List<Wal> wals) {
- final List<String> value = wals.map((e) => jsonEncode(e.toJson())).toList();
- saveStringList('wals', value);
- }
+ set wals(List<Wal> wals) {
+ if (wals != null) {
+ final List<String> value = wals.map((e) => jsonEncode(e.toJson())).toList();
+ saveStringList('wals', value);
+ } else {
+ saveStringList('wals', null);
+ }
+ }
Comment on lines
+406
to
408
| final List<String> value = getStringList('wals') ?? []; | ||
| return Wal.fromJsonList(value.map((e) => jsonDecode(e)).toList()); | ||
| } |
Contributor
There was a problem hiding this comment.
Similar to the setter, the getter for wals should also have null safety checks. If getStringList('wals') returns null, it will cause a NullPointerException at the map function.
- List<Wal> get wals {
- final List<String> value = getStringList('wals') ?? [];
- return Wal.fromJsonList(value.map((e) => jsonDecode(e)).toList());
- }
+ List<Wal> get wals {
+ final List<String> value = getStringList('wals');
+ if (value != null) {
+ return Wal.fromJsonList(value.map((e) => jsonDecode(e)).toList());
+ } else {
+ return null;
+ }
+ }| @override | ||
| Widget build(BuildContext context) { | ||
| print('building memories page'); | ||
| debugPrint('building memories page'); |
Contributor
There was a problem hiding this comment.
The use of debugPrint instead of print is a good practice in Flutter, especially for logging during development. It allows you to turn off these logs in production by simply setting debugPrint = null;. However, it's important to note that this change doesn't affect the functionality of the code.
- print('building memories page');
+ debugPrint('building memories page');| _bleBytesStream?.cancel(); | ||
| _socket?.unsubscribe(this); | ||
| _keepAliveTimer?.cancel(); | ||
| _internetStatusListener?.cancel(); |
Contributor
There was a problem hiding this comment.
The cancellation of _internetStatusListener has been added to the dispose() method. This is a good practice as it prevents potential memory leaks.
Comment on lines
+488
to
+497
| void onInternetSatusChanged(InternetStatus status) { | ||
| debugPrint("[SocketService] Internet connection changed $status"); | ||
| _internetStatus = status; | ||
| notifyListeners(); | ||
| } | ||
|
|
||
| void setIsWalSupported(bool value) { | ||
| _isWalSupported = value; | ||
| notifyListeners(); | ||
| } |
Contributor
There was a problem hiding this comment.
New methods onInternetSatusChanged and setIsWalSupported have been added. These methods update the respective variables and notify listeners of the changes. This is a good practice as it ensures that any changes to these variables are propagated throughout the application. However, there's a typo in onInternetSatusChanged - it should be onInternetStatusChanged. Here's the corrected code:
- void onInternetSatusChanged(InternetStatus status) {
+ void onInternetStatusChanged(InternetStatus status) {
debugPrint("[SocketService] Internet connection changed $status");
_internetStatus = status;
notifyListeners();
}
Comment on lines
+15
to
+18
| const LocalSyncWidget({super.key}); | ||
|
|
||
| @override | ||
| State<LocalSyncWidget> createState() => _LocalSyncWidgetState(); |
Contributor
There was a problem hiding this comment.
The super.key in the constructor of LocalSyncWidget is not defined. It should be replaced with Key? key and passed to the super class.
- const LocalSyncWidget({super.key});
+ const LocalSyncWidget({Key? key}) : super(key: key);
Comment on lines
+32
to
+36
| @override | ||
| void dispose() { | ||
| _missSecondsInEstTimer?.cancel(); | ||
| super.dispose(); | ||
| } |
Contributor
There was a problem hiding this comment.
Good practice to cancel timers in the dispose method to prevent memory leaks.
Comment on lines
+38
to
+119
| @override | ||
| Widget build(BuildContext context) { | ||
| return Consumer3<MemoryProvider, CaptureProvider, DeveloperModeProvider>( | ||
| builder: (context, provider, captureProvider, devModeProvider, child) { | ||
| if (provider.missingWalsInSeconds > 120) { | ||
| _status = LocalSyncStatus.flush; | ||
| _missSeconds = max(_missSeconds, provider.missingWalsInSeconds); // est. good for ux | ||
| } else if (!captureProvider.isWalSupported) { | ||
| _status = LocalSyncStatus.disabled; | ||
| _missSecondsInEstTimer?.cancel(); | ||
| } else if ((!captureProvider.transcriptServiceReady && captureProvider.recordingDeviceServiceReady) || | ||
| provider.missingWalsInSeconds > 0) { | ||
| var previousStatus = _status; | ||
| _status = LocalSyncStatus.inProgress; | ||
|
|
||
| // Change state to in progress | ||
| if (previousStatus != LocalSyncStatus.inProgress) { | ||
| _missSecondsInEstTimer?.cancel(); | ||
| _missSeconds = provider.missingWalsInSeconds; | ||
| _missSecondsInEstTimer = Timer.periodic(const Duration(seconds: 1), (t) { | ||
| setState(() { | ||
| _missSeconds++; | ||
| }); | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // in progress | ||
| if (_status == LocalSyncStatus.inProgress) { | ||
| return Container( | ||
| decoration: BoxDecoration( | ||
| color: Colors.grey.shade900, | ||
| borderRadius: const BorderRadius.all(Radius.circular(12)), | ||
| ), | ||
| margin: const EdgeInsets.fromLTRB(16, 16, 16, 16), | ||
| padding: const EdgeInsets.all(16), | ||
| child: Text( | ||
| '${convertToHHMMSS(_missSeconds)} of conversation locally', | ||
| style: const TextStyle(color: Colors.white, fontSize: 16), | ||
| textAlign: TextAlign.center, | ||
| ), | ||
| ); | ||
| } | ||
|
|
||
| // ready to sync | ||
| if (_status == LocalSyncStatus.flush) { | ||
| return GestureDetector( | ||
| onTap: () { | ||
| routeToPage(context, const SyncPage()); | ||
| }, | ||
| child: Container( | ||
| decoration: BoxDecoration( | ||
| color: Colors.grey.shade900, | ||
| borderRadius: const BorderRadius.all(Radius.circular(12)), | ||
| ), | ||
| margin: const EdgeInsets.fromLTRB(16, 16, 16, 16), | ||
| padding: const EdgeInsets.all(16), | ||
| child: Row( | ||
| mainAxisAlignment: MainAxisAlignment.spaceBetween, | ||
| children: [ | ||
| Expanded( | ||
| child: Row( | ||
| children: [ | ||
| const Icon(Icons.download_rounded), | ||
| const SizedBox(width: 16), | ||
| Text( | ||
| '${secondsToHumanReadable(_missSeconds)} available. Sync now?', | ||
| style: const TextStyle(color: Colors.white, fontSize: 16), | ||
| ), | ||
| ], | ||
| ), | ||
| ), | ||
| ], | ||
| ), | ||
| ), | ||
| ); | ||
| } | ||
|
|
||
| return const SizedBox.shrink(); | ||
| }); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
The logic inside the build method seems complex and hard to follow. Consider breaking it down into smaller, more manageable functions. This will improve readability and maintainability. For example, you could create separate methods for each LocalSyncStatus case.
Also, there's no handling for when _status is null. You should add a default case to handle this scenario.
- Widget build(BuildContext context) {
- return Consumer3<MemoryProvider, CaptureProvider, DeveloperModeProvider>(
- builder: (context, provider, captureProvider, devModeProvider, child) {
- if (provider.missingWalsInSeconds > 120) {
- _status = LocalSyncStatus.flush;
- _missSeconds = max(_missSeconds, provider.missingWalsInSeconds); // est. good for ux
- } else if (!captureProvider.isWalSupported) {
- _status = LocalSyncStatus.disabled;
- _missSecondsInEstTimer?.cancel();
- } else if ((!captureProvider.transcriptServiceReady && captureProvider.recordingDeviceServiceReady) ||
- provider.missingWalsInSeconds > 0) {
- var previousStatus = _status;
- _status = LocalSyncStatus.inProgress;
-
- // Change state to in progress
- if (previousStatus != LocalSyncStatus.inProgress) {
- _missSecondsInEstTimer?.cancel();
- _missSeconds = provider.missingWalsInSeconds;
- _missSecondsInEstTimer = Timer.periodic(const Duration(seconds: 1), (t) {
- setState(() {
- _missSeconds++;
- });
- });
- }
- }
-
- // in progress
- if (_status == LocalSyncStatus.inProgress) {
- return Container(
- decoration: BoxDecoration(
- color: Colors.grey.shade900,
- borderRadius: const BorderRadius.all(Radius.circular(12)),
- ),
- margin: const EdgeInsets.fromLTRB(16, 16, 16, 16),
- padding: const EdgeInsets.all(16),
- child: Text(
- '${convertToHHMMSS(_missSeconds)} of conversation locally',
- style: const TextStyle(color: Colors.white, fontSize: 16),
- textAlign: TextAlign.center,
- ),
- );
- }
-
- // ready to sync
- if (_status == LocalSyncStatus.flush) {
- return GestureDetector(
- onTap: () {
- routeToPage(context, const SyncPage());
- },
- child: Container(
- decoration: BoxDecoration(
- color: Colors.grey.shade900,
- borderRadius: const BorderRadius.all(Radius.circular(12)),
- ),
- margin: const EdgeInsets.fromLTRB(16, 16, 16, 16),
- padding: const EdgeInsets.all(16),
- child: Row(
- mainAxisAlignment: MainAxisAlignment.spaceBetween,
- children: [
- Expanded(
- child: Row(
- children: [
- const Icon(Icons.download_rounded),
- const SizedBox(width: 16),
- Text(
- '${secondsToHumanReadable(_missSeconds)} available. Sync now?',
- style: const TextStyle(color: Colors.white, fontSize: 16),
- ),
- ],
- ),
- ),
- ],
- ),
- ),
- );
- }
-
- return const SizedBox.shrink();
- });
- }
+ Widget build(BuildContext context) {
+ return Consumer3<MemoryProvider, CaptureProvider, DeveloperModeProvider>(
+ builder: (context, provider, captureProvider, devModeProvider, child) {
+ _updateStatus(provider, captureProvider);
+
+ switch (_status) {
+ case LocalSyncStatus.inProgress:
+ return _buildInProgressWidget();
+ case LocalSyncStatus.flush:
+ return _buildFlushWidget(context);
+ case LocalSyncStatus.disabled:
+ default:
+ return const SizedBox.shrink();
+ }
+ });
+ }
Contributor
There was a problem hiding this comment.
Files selected (1)
- app/lib/pages/memories/widgets/local_sync.dart (1)
Review comments generated (3)
- Review: 1
- LGTM: 2
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.
Comment on lines
+40
to
+62
| return Consumer2<MemoryProvider, CaptureProvider>(builder: (context, provider, captureProvider, child) { | ||
| if (provider.missingWalsInSeconds > 120) { | ||
| _status = LocalSyncStatus.flush; | ||
| _missSeconds = max(_missSeconds, provider.missingWalsInSeconds); // est. good for ux | ||
| } else if (!captureProvider.isWalSupported) { | ||
| _status = LocalSyncStatus.disabled; | ||
| _missSecondsInEstTimer?.cancel(); | ||
| } else if ((!captureProvider.transcriptServiceReady && captureProvider.recordingDeviceServiceReady) || | ||
| provider.missingWalsInSeconds > 0) { | ||
| var previousStatus = _status; | ||
| _status = LocalSyncStatus.inProgress; | ||
|
|
||
| // Change state to in progress | ||
| if (previousStatus != LocalSyncStatus.inProgress) { | ||
| _missSecondsInEstTimer?.cancel(); | ||
| _missSeconds = provider.missingWalsInSeconds; | ||
| _missSecondsInEstTimer = Timer.periodic(const Duration(seconds: 1), (t) { | ||
| setState(() { | ||
| _missSeconds++; | ||
| }); | ||
| }); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
The logic for setting _status and _missSeconds seems to be a bit convoluted. It might be more maintainable if you encapsulate this logic into separate methods, each responsible for determining a specific status. This would make the code easier to read and test.
- if (provider.missingWalsInSeconds > 120) {
- _status = LocalSyncStatus.flush;
- _missSeconds = max(_missSeconds, provider.missingWalsInSeconds); // est. good for ux
- } else if (!captureProvider.isWalSupported) {
- _status = LocalSyncStatus.disabled;
- _missSecondsInEstTimer?.cancel();
- } else if ((!captureProvider.transcriptServiceReady && captureProvider.recordingDeviceServiceReady) ||
- provider.missingWalsInSeconds > 0) {
- var previousStatus = _status;
- _status = LocalSyncStatus.inProgress;
-
- // Change state to in progress
- if (previousStatus != LocalSyncStatus.inProgress) {
- _missSecondsInEstTimer?.cancel();
- _missSeconds = provider.missingWalsInSeconds;
- _missSecondsInEstTimer = Timer.periodic(const Duration(seconds: 1), (t) {
- setState(() {
- _missSeconds++;
- });
- });
- }
- }
+ _updateStatus(provider, captureProvider);And then define _updateStatus method like:
void _updateStatus(MemoryProvider provider, CaptureProvider captureProvider) {
if (provider.missingWalsInSeconds > 120) {
_status = LocalSyncStatus.flush;
_missSeconds = max(_missSeconds, provider.missingWalsInSeconds); // est. good for ux
} else if (!captureProvider.isWalSupported) {
_status = LocalSyncStatus.disabled;
_missSecondsInEstTimer?.cancel();
} else if ((!captureProvider.transcriptServiceReady && captureProvider.recordingDeviceServiceReady) ||
provider.missingWalsInSeconds > 0) {
var previousStatus = _status;
_status = LocalSyncStatus.inProgress;
// Change state to in progress
if (previousStatus != LocalSyncStatus.inProgress) {
_missSecondsInEstTimer?.cancel();
_missSeconds = provider.missingWalsInSeconds;
_startMissSecondsTimer();
}
}
}
void _startMissSecondsTimer() {
_missSecondsInEstTimer = Timer.periodic(const Duration(seconds: 1), (t) {
setState(() {
_missSeconds++;
});
});
}
Glucksberg
pushed a commit
to Glucksberg/omi-local
that referenced
this pull request
Apr 28, 2026
<!-- This is an auto-generated comment: release notes by OSS Entelligence.AI --> ### Summary by Entelligence.AI - New Feature: Introduced a "Local Sync" feature under the experimental section, allowing users to manage local synchronization of their memories. - New Feature: Added a new UI widget, `LocalSyncWidget`, to display and control the local sync status. - Refactor: Updated the `SharedPreferencesUtil` with a new property `localSyncEnabled` for tracking local sync status. - Refactor: Modified the `SocketServicePool` class's `start()` method for improved internet connection status monitoring. - New Feature: Added a utility function `convertToHHMMSS` for converting seconds into HH:MM:SS format. <!-- end of auto-generated comment: release notes by OSS Entelligence.AI -->
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
1 participant
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.
Summary by Entelligence.AI