Zero memory loss by syncing missing audio frames to server backward #980
Conversation
|
WalkthroughThis update introduces a Write-Ahead Logging (WAL) service to manage audio frame data, with backward synchronization capabilities. It includes changes in memory and capture providers, services, and backend preferences. The WAL service is integrated into the existing system, providing mechanisms for handling missing data and prompting users to sync when necessary. 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 (1)
- app/lib/providers/capture_provider.dart (3)
- app/lib/providers/memory_provider.dart (3)
- app/lib/services/services.dart (4)
- app/lib/services/wals.dart (1)
Review comments generated (18)
- Review: 12
- LGTM: 6
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.
app/lib/services/services.dart
Outdated
| _wal.start(); | ||
| } | ||
|
|
||
| void deinit() { | ||
| _mic.stop(); | ||
| _device.stop(); | ||
| _wal.stop(); |
There was a problem hiding this comment.
The start and stop methods of _wal are not awaited, which could lead to race conditions or unexpected behavior if these operations are asynchronous. If these methods return a Future, they should be awaited.
Future<void> start() async {
_device.start();
- _wal.start();
+ await _wal.start();
}
void deinit() {
- _mic.stop();
- _device.stop();
- _wal.stop();
+ _mic.stop(); // Assuming this is synchronous
+ _device.stop(); // Assuming this is synchronous
+ await _wal.stop();
}Please ensure that the other stop/start calls (_mic.stop(), _device.stop(), _device.start()) are handled correctly as well. If they are asynchronous, they should also be awaited.
| _bleBytesStream?.cancel(); | ||
| _bleBytesStream = await _getBleAudioBytesListener(id, onAudioBytesReceived: (List<int> value) { | ||
| if (value.isEmpty) return; | ||
| wal.onByteStream(value); | ||
| if (_socket?.state == SocketServiceState.connected) { | ||
| final trimmedValue = value.sublist(3); | ||
| _socket?.send(trimmedValue); | ||
| wal.onBytesSync(value); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The new implementation introduces a Write-Ahead Logging (WAL) service to the audio streaming operation. However, there's a potential issue with the order of operations in this block. The wal.onByteStream(value); is called before checking if the socket state is connected. If the socket is not connected, the WAL service will still log the byte stream, which might not be the intended behavior. Consider moving the wal.onByteStream(value); call inside the condition where the socket state is checked.
- wal.onByteStream(value);
if (_socket?.state == SocketServiceState.connected) {
+ wal.onByteStream(value);
final trimmedValue = value.sublist(3);
_socket?.send(trimmedValue);
wal.onBytesSync(value);
}This way, the byte stream is only logged when the socket is connected.
app/lib/pages/memories/page.dart
Outdated
| // TODO: FIXME | ||
| SliverToBoxAdapter( | ||
| child: memoryProvider.missingWals.isNotEmpty | ||
| ? Padding( | ||
| padding: const EdgeInsets.all(32.0), | ||
| child: Center( | ||
| child: Text( | ||
| "You have ${memoryProvider.missingWals.map((val) => val.seconds).reduce((a, b) => a + b)}s stereo localy, sync now?"), | ||
| ), | ||
| ) | ||
| : SizedBox.shrink()), |
There was a problem hiding this comment.
The code block here is responsible for displaying a message to the user if there are any missing WALs. However, it seems like the calculation of total seconds of missing WALs could potentially be an expensive operation if there are many missing WALs, as it involves mapping and reducing over the entire list of missing WALs every time the UI needs to be rendered.
Consider moving this calculation to the MemoryProvider class and only updating it when a new WAL is added or an existing one is synced. This way, you avoid unnecessary computations and improve performance.
- "You have ${memoryProvider.missingWals.map((val) => val.seconds).reduce((a, b) => a + b)}s stereo localy, sync now?")
+ "You have ${memoryProvider.totalMissingWalsSeconds}s stereo localy, sync now?")In MemoryProvider:
int get totalMissingWalsSeconds {
return _missingWals.map((val) => val.seconds).reduce((a, b) => a + b);
}And update this value in onNewMissingWal and onWalSynced methods.
| IWalService get _wal => ServiceManager.instance().wal; | ||
| List<Wal> _missingWals = []; | ||
| List<Wal> get missingWals => _missingWals; | ||
|
|
||
| MemoryProvider() { | ||
| _wal.subscribe(this, this); | ||
| _preload(); | ||
| } | ||
|
|
||
| _preload() async { | ||
| _missingWals = await _wal.getMissingWals(); | ||
| notifyListeners(); | ||
| } |
There was a problem hiding this comment.
The _preload method is called in the constructor of MemoryProvider. This could potentially lead to unhandled exceptions if _wal.getMissingWals() throws an error. It's generally not a good practice to call async methods without handling errors in constructors. Consider moving this logic to an initialization method and handle potential exceptions.
- MemoryProvider() {
- _wal.subscribe(this, this);
- _preload();
- }
+ MemoryProvider() {
+ _wal.subscribe(this, this);
+ init();
+ }
+
+ Future<void> init() async {
+ try {
+ await _preload();
+ } catch (e) {
+ // Handle or log exception
+ }
+ }
| _wal.unsubscribe(this); | ||
| super.dispose(); | ||
| } | ||
|
|
||
| @override | ||
| void onNewMissingWal(Wal wal) async { | ||
| _missingWals = await _wal.getMissingWals(); | ||
| notifyListeners(); | ||
| } | ||
|
|
||
| @override | ||
| void onWalSynced(Wal wal, ServerMemory memory) async { | ||
| _missingWals = await _wal.getMissingWals(); | ||
| notifyListeners(); | ||
| } | ||
|
|
||
| @override | ||
| void onStatusChanged(WalServiceStatus status) {} |
There was a problem hiding this comment.
In the dispose method, you're unsubscribing from _wal, which is good for resource cleanup. However, in the onNewMissingWal and onWalSynced methods, you're calling _wal.getMissingWals() which might throw an error if the object has been disposed. You should add checks to ensure that the object has not been disposed before making these calls.
- void onNewMissingWal(Wal wal) async {
- _missingWals = await _wal.getMissingWals();
- notifyListeners();
- }
+ void onNewMissingWal(Wal wal) async {
+ if (!disposed) {
+ _missingWals = await _wal.getMissingWals();
+ notifyListeners();
+ }
+ }
- void onWalSynced(Wal wal, ServerMemory memory) async {
- _missingWals = await _wal.getMissingWals();
- notifyListeners();
- }
+ void onWalSynced(Wal wal, ServerMemory memory) async {
+ if (!disposed) {
+ _missingWals = await _wal.getMissingWals();
+ notifyListeners();
+ }
+ }In the above code, disposed is a hypothetical boolean that should be set to true in the dispose method. If you don't have such a flag, consider adding one.
app/lib/services/wals.dart
Outdated
| // TODO: FIXME, calculate seconds | ||
| get seconds => 10; |
There was a problem hiding this comment.
The seconds getter is currently hardcoded to return 10. If this is a placeholder, make sure to replace it with the correct implementation before merging.
app/lib/services/wals.dart
Outdated
| void stop() async { | ||
| await _flush(); | ||
|
|
||
| _status = WalServiceStatus.stop; | ||
| _onStatusChanged(_status); | ||
| _subscriptions.clear(); | ||
| } |
There was a problem hiding this comment.
The stop method is marked as void but contains async operations. This could lead to unexpected behavior as the caller might assume that the operation has completed when it hasn't. Change the return type to Future<void>.
- void stop() async {
+ Future<void> stop() async {
app/lib/services/wals.dart
Outdated
| Future syncAll(IWalSyncProgressListener progress) async { | ||
| // TODO: implement syncAll | ||
| throw UnimplementedError(); |
There was a problem hiding this comment.
There are several methods in the WalService class that are not yet implemented and some of them return mock data. Ensure these methods are properly implemented before merging this PR to avoid potential issues in production.
app/lib/services/wals.dart
Outdated
| final Map<Object, IWalServiceListener> _subscriptions = {}; | ||
| WalServiceStatus _status = WalServiceStatus.init; | ||
| WalServiceStatus get status => _status; | ||
|
|
||
| @override | ||
| void subscribe(IWalServiceListener subscription, Object context) { | ||
| _subscriptions.remove(context.hashCode); | ||
| _subscriptions.putIfAbsent(context.hashCode, () => subscription); | ||
|
|
||
| // Retains | ||
| subscription.onStatusChanged(_status); | ||
| } |
There was a problem hiding this comment.
The subscribe method removes an existing subscription before adding a new one. This could potentially lead to unexpected behavior if multiple components subscribe with the same context. Consider allowing multiple subscriptions from the same context or provide clear documentation about this behavior.
- _subscriptions.remove(context.hashCode);
- _subscriptions.putIfAbsent(context.hashCode, () => subscription);
+ _subscriptions[context.hashCode] = subscription;| _subscriptions.clear(); | ||
| } |
There was a problem hiding this comment.
The stop method clears all subscriptions. If the service is restarted, subscribers will not be notified of status changes. Consider keeping the subscriptions intact even when the service is stopped, unless there's a specific reason to remove them.
There was a problem hiding this comment.
Files selected (2)
- app/lib/backend/preferences.dart (2)
- app/lib/services/wals.dart (1)
Review comments generated (9)
- Review: 8
- 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.
app/lib/backend/preferences.dart
Outdated
|
|
||
| Future<void> walsSet(List<Wal> wals) async { | ||
| final List<String> value = wals.map((e) => jsonEncode(e.toJson())).toList(); | ||
| await saveStringList('wals', value); | ||
| } | ||
|
|
||
| List<Wal> get wals { | ||
| final List<String> value = getStringList('wals') ?? []; | ||
| return Wal.fromJsonList(value.map((e) => jsonDecode(e)).toList()); | ||
| } |
There was a problem hiding this comment.
The walsSet and wals methods are handling JSON encoding/decoding manually. This could lead to potential issues if the Wal object's structure changes in the future, as you would need to update these methods accordingly. It would be better to encapsulate this logic within the Wal class itself, by implementing methods like toJsonString() and fromJsonString(). This way, the SharedPreferencesUtil class doesn't need to know about the internal structure of Wal.
Future<void> walsSet(List<Wal> wals) async {
final List<String> value = wals.map((e) => jsonEncode(e.toJson())).toList();
await saveStringList('wals', value);
}
List<Wal> get wals {
final List<String> value = getStringList('wals') ?? [];
return Wal.fromJsonList(value.map((e) => jsonDecode(e)).toList());
}
+ Future<void> walsSet(List<Wal> wals) async {
+ final List<String> value = wals.map((e) => e.toJsonString()).toList();
+ await saveStringList('wals', value);
+ }
+
+ List<Wal> get wals {
+ final List<String> value = getStringList('wals') ?? [];
+ return value.map((e) => Wal.fromJsonString(e)).toList();
+ }In the Wal class, you can add these methods:
class Wal {
// existing code...
String toJsonString() {
return jsonEncode(this.toJson());
}
static Wal fromJsonString(String jsonString) {
return Wal.fromJson(jsonDecode(jsonString));
}
static List<Wal> fromJsonList(List<String> jsonStringList) {
return jsonStringList.map((e) => Wal.fromJsonString(e)).toList();
}
}This change will make your code more maintainable and less error-prone.
| import 'dart:async'; | ||
| import 'dart:collection'; | ||
| import 'dart:ffi'; | ||
| import 'dart:io'; |
There was a problem hiding this comment.
It seems like you're importing dart:ffi and dart:io, but I don't see any usage of these libraries in the provided code. If they are not used, it's better to remove them to keep the imports clean.
- import 'dart:ffi';
- import 'dart:io';
app/lib/services/wals.dart
Outdated
| abstract class IWalService { | ||
| void start(); | ||
| void stop(); | ||
|
|
||
| void subscribe(IWalServiceListener subscription, Object context); | ||
| void unsubscribe(Object context); | ||
|
|
||
| void onByteStream(List<int> value); | ||
| void onBytesSync(List<int> value); | ||
| Future syncAll(IWalSyncProgressListener progress); | ||
| Future<bool> syncWal(Wal wal); | ||
| Future<bool> deleteWal(Wal wal); | ||
| Future<List<Wal>> getMissingWals(); | ||
| } |
There was a problem hiding this comment.
The IWalService interface is well defined with clear methods for managing Wals. However, it would be beneficial to add comments to each method to explain what they do. This will improve maintainability and readability of the code.
app/lib/services/wals.dart
Outdated
| class Wal { | ||
| int timestamp; // in seconds | ||
| String codec; | ||
| int channel; | ||
| int sampleRate; | ||
|
|
||
| WalStatus status; | ||
| WalStorage storage; | ||
|
|
||
| String? filePath; | ||
| List<List<int>> data = []; | ||
|
|
||
| String get id => '$timestamp'; | ||
|
|
||
| Wal( | ||
| {required this.timestamp, | ||
| this.codec = "opus", | ||
| this.sampleRate = 16000, | ||
| this.channel = 1, | ||
| this.status = WalStatus.inProgress, | ||
| this.storage = WalStorage.mem, | ||
| this.filePath, | ||
| this.data = const []}); | ||
|
|
||
| get seconds => ChunkSizeInSeconds; | ||
|
|
||
| factory Wal.fromJson(Map<String, dynamic> json) { | ||
| return Wal( | ||
| timestamp: json['timestamp'], | ||
| codec: json['codec'], | ||
| channel: json['channel'], | ||
| sampleRate: json['sample_rate'], | ||
| status: WalStatus.values.asNameMap()[json['status']] ?? WalStatus.inProgress, | ||
| storage: WalStorage.values.asNameMap()[json['storage']] ?? WalStorage.mem, | ||
| filePath: json['file_path'], | ||
| ); | ||
| } | ||
|
|
||
| Map<String, dynamic> toJson() { | ||
| return { | ||
| 'timestamp': timestamp, | ||
| 'codec': codec, | ||
| 'channel': channel, | ||
| 'sample_rate': sampleRate, | ||
| 'status': status.name, | ||
| 'storage': storage.name, | ||
| 'file_path': filePath, | ||
| }; | ||
| } | ||
|
|
||
| static List<Wal> fromJsonList(List<dynamic> jsonList) => jsonList.map((e) => Wal.fromJson(e)).toList(); | ||
|
|
||
| static Wal mock() { | ||
| var ts = DateTime.now().millisecondsSinceEpoch; | ||
| return Wal( | ||
| timestamp: ts, | ||
| codec: "opus", | ||
| sampleRate: 16000, | ||
| channel: 1, | ||
| status: WalStatus.inProgress, | ||
| storage: WalStorage.mem, | ||
| ); | ||
| } | ||
|
|
||
| getFileName() { | ||
| return "audio_${timestamp}_${codec}_${sampleRate}_$channel.bin"; | ||
| } | ||
| } |
There was a problem hiding this comment.
The Wal class has a good structure and includes JSON serialization/deserialization methods. However, there are no null checks or error handling in the fromJson factory method. If the JSON does not contain expected keys, this could lead to runtime errors. Consider adding error handling or default values to prevent potential issues.
app/lib/services/wals.dart
Outdated
| factory Wal.fromJson(Map<String, dynamic> json) { | ||
| return Wal( | ||
| timestamp: json['timestamp'], | ||
| codec: json['codec'], | ||
| channel: json['channel'], | ||
| sampleRate: json['sample_rate'], | ||
| status: WalStatus.values.asNameMap()[json['status']] ?? WalStatus.inProgress, | ||
| storage: WalStorage.values.asNameMap()[json['storage']] ?? WalStorage.mem, | ||
| filePath: json['file_path'], | ||
| ); | ||
| } |
There was a problem hiding this comment.
In the fromJson method, you are directly accessing the map values which can cause a NoSuchMethodError if the key doesn't exist in the map. It's safer to use the .containsKey() method before accessing the value.
- timestamp: json['timestamp'],
+ timestamp: json.containsKey('timestamp') ? json['timestamp'] : 0,Repeat this pattern for all fields in the fromJson method.
app/lib/services/wals.dart
Outdated
| await _flush(); | ||
| }); | ||
| _status = WalServiceStatus.ready; | ||
| } | ||
|
|
||
| Future _chunk() async { | ||
| if (_frames.isEmpty) { | ||
| debugPrint("Frames are empty"); | ||
| return; | ||
| } | ||
|
|
||
| var framesPerSeconds = 100; | ||
| var ts = DateTime.now().millisecondsSinceEpoch; | ||
| var pivot = _frames.length; | ||
| var high = pivot; | ||
| while (high > 0) { | ||
| var low = high - framesPerSeconds * ChunkSizeInSeconds; | ||
| if (low < 0) { | ||
| low = 0; | ||
| } | ||
| var synced = true; | ||
| var chunk = _frames.sublist(low, high); | ||
| for (var f in chunk) { | ||
| var head = f.sublist(0, 3); // first 3 bytes | ||
| var seq = Uint8List.fromList(head).buffer.asByteData().getInt32(0); | ||
| if (!_syncFrameSeq.contains(seq)) { | ||
| synced = false; | ||
| break; | ||
| } | ||
| } | ||
| if (!synced) { | ||
| var missWal = Wal( | ||
| timestamp: ts ~/ 1000, // in seconds | ||
| data: chunk, | ||
| storage: WalStorage.mem, | ||
| status: WalStatus.miss, | ||
| ); | ||
| _wals.add(missWal); | ||
|
|
||
| // send | ||
| for (var sub in _subscriptions.values) { | ||
| sub.onNewMissingWal(missWal); | ||
| } | ||
| } | ||
|
|
||
| // next | ||
| ts -= ChunkSizeInSeconds * 1000; | ||
| high = low; | ||
| } | ||
|
|
||
| // clean | ||
| _frames.removeRange(0, pivot); | ||
| } | ||
|
|
||
| Future _flush() async { | ||
| // Clean synced wals | ||
| _wals.removeWhere((wal) => wal.status == WalStatus.synced); | ||
|
|
||
| // Storage file | ||
| for (var i = 0; i < _wals.length; i++) { | ||
| final wal = _wals[i]; | ||
|
|
||
| if (wal.storage == WalStorage.mem) { | ||
| // flush to disk | ||
| final directory = await getTemporaryDirectory(); | ||
| String filePath = '${directory.path}/${wal.getFileName()}'; | ||
| Uint8List data = Uint8List.fromList([]); | ||
| for (int i = 0; i < wal.data.length; i++) { | ||
| for (int j = 0; j < wal.data[i].length; j++) { | ||
| data.add(wal.data[i][j]); | ||
| } | ||
| } | ||
| final file = File(filePath); | ||
| await file.writeAsBytes(data.buffer.asUint8List()); | ||
| wal.filePath = filePath; | ||
| wal.storage = WalStorage.disk; | ||
|
|
||
| _wals[i] = wal; | ||
| } | ||
| } | ||
|
|
||
| await SharedPreferencesUtil().walsSet(_wals); | ||
| } | ||
|
|
||
| @override | ||
| void stop() async { | ||
| _chunkingTimer?.cancel(); | ||
| _flushingTimer?.cancel(); | ||
|
|
||
| await _chunk(); | ||
| await _flush(); | ||
|
|
||
| _frames = []; | ||
| _syncFrameSeq.clear(); | ||
| _wals = []; | ||
|
|
||
| _status = WalServiceStatus.stop; | ||
| _onStatusChanged(_status); | ||
| _subscriptions.clear(); | ||
| } |
There was a problem hiding this comment.
The WalService class implementation looks good overall. However, there are some areas where error handling could be improved. For instance, in the _flush method, when writing bytes to a file, an exception could occur if there's an issue with the file system. It would be beneficial to add a try-catch block around this operation to handle potential exceptions.
app/lib/services/wals.dart
Outdated
| final directory = await getTemporaryDirectory(); | ||
| String filePath = '${directory.path}/${wal.getFileName()}'; | ||
| Uint8List data = Uint8List.fromList([]); | ||
| for (int i = 0; i < wal.data.length; i++) { | ||
| for (int j = 0; j < wal.data[i].length; j++) { | ||
| data.add(wal.data[i][j]); | ||
| } | ||
| } | ||
| final file = File(filePath); | ||
| await file.writeAsBytes(data.buffer.asUint8List()); |
There was a problem hiding this comment.
When writing data to a file, it's important to handle potential IO exceptions that could occur. Consider wrapping this section in a try-catch block.
+ try {
final directory = await getTemporaryDirectory();
String filePath = '${directory.path}/${wal.getFileName()}';
Uint8List data = Uint8List.fromList([]);
for (int i = 0; i < wal.data.length; i++) {
for (int j = 0; j < wal.data[i].length; j++) {
data.add(wal.data[i][j]);
}
}
final file = File(filePath);
await file.writeAsBytes(data.buffer.asUint8List());
wal.filePath = filePath;
wal.storage = WalStorage.disk;
_wals[i] = wal;
+ } catch (e) {
+ // Handle or log the exception
+ }
app/lib/services/wals.dart
Outdated
| // TODO: implement syncWal | ||
| return true; | ||
| } | ||
|
|
||
| @override | ||
| Future syncAll(IWalSyncProgressListener progress) async { | ||
| // TODO: implement syncAll | ||
| throw UnimplementedError(); | ||
| } |
There was a problem hiding this comment.
The syncWal and syncAll methods are not implemented yet. Make sure to implement these methods before merging the PR, as they are part of the IWalService interface and leaving them unimplemented could lead to runtime errors.
beastoin
changed the title
beastoin
force-pushed
the
jzecm_wal_local_sync_958
branch
from
45330e1 to
9bd32b8
Compare
There was a problem hiding this comment.
Files selected (6)
- app/lib/backend/preferences.dart (2)
- app/lib/main.dart (2)
- app/lib/pages/memories/page.dart (1)
- app/lib/providers/memory_provider.dart (3)
- app/lib/services/services.dart (4)
- app/lib/services/wals.dart (1)
Files not summarized due to errors (1)
- app/lib/services/wals.dart (diff tokens exceeds limit)
Files skipped from review due to trivial changes (1)
- app/lib/main.dart
Review comments generated (13)
- Review: 7
- LGTM: 6
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.
| _wal.start(); | ||
| } | ||
|
|
||
| void deinit() { | ||
| void deinit() async { | ||
| await _wal.stop(); |
There was a problem hiding this comment.
There's a potential issue with the deinit method. It's declared as a synchronous method but you're using await inside it which is an asynchronous operation. This could lead to unexpected behavior. You should declare deinit as an asynchronous method.
- void deinit() async {
+ Future<void> deinit() async {
await _wal.stop();
_mic.stop();
_device.stop();
}| // TODO: FIXME | ||
| SliverToBoxAdapter( | ||
| child: memoryProvider.missingWals.isNotEmpty | ||
| ? Padding( | ||
| padding: const EdgeInsets.all(32.0), | ||
| child: Center( | ||
| child: GestureDetector( | ||
| onTap: () { | ||
| memoryProvider.syncWals(); | ||
| }, | ||
| child: Text( | ||
| "You have ${memoryProvider.missingWals.map((val) => val.seconds).reduce((a, b) => a + b)}s stereo localy, sync now?"), | ||
| ), | ||
| ), | ||
| ) | ||
| : SizedBox.shrink()), |
There was a problem hiding this comment.
The code block here is responsible for displaying a prompt to the user when there are missing audio frames that need to be synced. However, it's not handling the case where memoryProvider.missingWals is null, which could lead to a null pointer exception if missingWals is not initialized. You should add a null check before accessing its isNotEmpty property.
Also, the calculation of total seconds of missing audio frames is done directly in the UI layer. This logic should be moved to the provider or model layer to keep the UI layer clean and focused on rendering.
- child: memoryProvider.missingWals.isNotEmpty
+ child: (memoryProvider.missingWals?.isNotEmpty ?? false)
// Move this logic to the provider or model layer
- "You have ${memoryProvider.missingWals.map((val) => val.seconds).reduce((a, b) => a + b)}s stereo localy, sync now?"
+ "You have ${memoryProvider.totalMissingSeconds}s stereo localy, sync now?"Please consider moving the calculation logic to a method in the MemoryProvider class:
class MemoryProvider {
// Other properties and methods...
int get totalMissingSeconds {
return missingWals?.map((val) => val.seconds)?.reduce((a, b) => a + b) ?? 0;
}
}
app/lib/services/wals.dart
Outdated
|
|
||
| _wals[i] = wal; | ||
| } | ||
| } | ||
|
|
||
| SharedPreferencesUtil().wals = _wals; | ||
| } | ||
|
|
||
| @override | ||
| Future stop() async { | ||
| debugPrint("wal service stop"); | ||
| _chunkingTimer?.cancel(); | ||
| _flushingTimer?.cancel(); | ||
|
|
||
| await _chunk(); | ||
| await _flush(); | ||
|
|
||
| _frames = []; | ||
| _syncFrameSeq.clear(); | ||
| _wals = []; | ||
|
|
||
| _status = WalServiceStatus.stop; | ||
| _onStatusChanged(_status); | ||
| _subscriptions.clear(); | ||
| } | ||
|
|
||
| void _onStatusChanged(WalServiceStatus status) { | ||
| for (var s in _subscriptions.values) { | ||
| s.onStatusChanged(status); | ||
| } | ||
| } | ||
|
|
||
| @override | ||
| Future<bool> deleteWal(Wal wal) async { | ||
| // Delete file | ||
| if (wal.filePath != null) { | ||
| try { | ||
| final file = File(wal.filePath!); | ||
| await file.delete(); | ||
| } catch (e) { | ||
| debugPrint(e.toString()); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| _wals.removeWhere((w) => w.id == wal.id); | ||
| return true; | ||
| } | ||
|
|
||
| @override | ||
| Future<List<Wal>> getMissingWals() async { | ||
| return _wals.where((w) => w.status == WalStatus.miss).toList(); | ||
| } | ||
|
|
||
| @override | ||
| void onByteStream(List<int> value) async { | ||
| _frames.add(value); | ||
| } | ||
|
|
||
| @override | ||
| void onBytesSync(List<int> value) { | ||
| var head = value.sublist(0, 3); | ||
| var seq = Uint8List.fromList(head..add(0)).buffer.asByteData().getInt32(0); | ||
| _syncFrameSeq.add(seq); | ||
| } | ||
|
|
||
| @override | ||
| Future<bool> syncWal(Wal wal) async { | ||
| // TODO: implement syncWal | ||
| return true; | ||
| } | ||
|
|
||
| @override | ||
| Future syncAll({IWalSyncProgressListener? progress}) async { | ||
| _wals.removeWhere((wal) => wal.status == WalStatus.synced); | ||
|
|
||
| await _flush(); | ||
|
|
||
| var wals = _wals.where((w) => w.status == WalStatus.miss && w.storage == WalStorage.disk); | ||
| for (var wal in wals) { | ||
| debugPrint("sync id ${wal.id}"); | ||
| if (wal.filePath == null) { | ||
| debugPrint("sync error: file path is not found. wal id ${wal.id}"); | ||
| continue; | ||
| } | ||
|
|
||
| try { | ||
| File file = File(wal.filePath!); | ||
| var bytes = await file.readAsBytes(); | ||
|
|
||
| // TODO: sync to socket | ||
| debugPrint("sync wal ${wal.id} file ${wal.filePath} length ${bytes.length}"); | ||
| debugPrint("[${bytes.join(", ")}]"); | ||
| } catch (e) { | ||
| debugPrint(e.toString()); | ||
| continue; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The WalService class seems to be doing a lot of work. It's handling the chunking and flushing of data, managing subscriptions, and also dealing with file I/O operations. This is a violation of the Single Responsibility Principle (SRP) which states that a class should have only one reason to change.
Consider breaking down this class into smaller classes each having its own responsibility. For example, you could create a separate WalFileManager class that handles all the file related operations like reading from and writing to files.
Here's an example of how you might refactor the file-related operations:
class WalFileManager {
Future<String> writeToFile(Wal wal, List<int> data) async {
final directory = await getTemporaryDirectory();
String filePath = '${directory.path}/${wal.getFileName()}';
final file = File(filePath);
await file.writeAsBytes(data);
return filePath;
}
Future<List<int>> readFromFile(String filePath) async {
File file = File(filePath);
var bytes = await file.readAsBytes();
return bytes;
}
Future<bool> deleteFile(String filePath) async {
try {
final file = File(filePath);
await file.delete();
return true;
} catch (e) {
debugPrint(e.toString());
return false;
}
}
}You can then use this WalFileManager in your WalService class. This will make your code more modular, easier to maintain, and test.
|
|
||
| set wals(List<Wal> wals) { | ||
| final List<String> value = wals.map((e) => jsonEncode(e.toJson())).toList(); | ||
| saveStringList('wals', value); | ||
| } | ||
|
|
||
| List<Wal> get wals { | ||
| final List<String> value = getStringList('wals') ?? []; | ||
| return Wal.fromJsonList(value.map((e) => jsonDecode(e)).toList()); | ||
| } |
There was a problem hiding this comment.
The new methods set wals and get wals are handling JSON encoding/decoding directly. This could lead to potential issues if the Wal object's structure changes in the future, as it would require updates in multiple places. It would be better to encapsulate this logic within the Wal class itself, providing a method to convert a list of Wal objects to a list of strings and vice versa.
set wals(List<Wal> wals) {
- final List<String> value = wals.map((e) => jsonEncode(e.toJson())).toList();
+ final List<String> value = Wal.encodeList(wals);
saveStringList('wals', value);
}
List<Wal> get wals {
- final List<String> value = getStringList('wals') ?? [];
- return Wal.fromJsonList(value.map((e) => jsonDecode(e)).toList());
+ return Wal.decodeList(getStringList('wals') ?? []);
}In the Wal class, you can add these static methods:
class Wal {
// Other existing code...
static List<String> encodeList(List<Wal> wals) {
return wals.map((wal) => jsonEncode(wal.toJson())).toList();
}
static List<Wal> decodeList(List<String> walStrings) {
return walStrings.map((walString) => Wal.fromJson(jsonDecode(walString))).toList();
}
}This way, the responsibility of encoding/decoding Wal objects is kept within the Wal class, improving maintainability and reducing the risk of errors.
| IWalService get _wal => ServiceManager.instance().wal; | ||
| List<Wal> _missingWals = []; | ||
| List<Wal> get missingWals => _missingWals; |
There was a problem hiding this comment.
The _missingWals field is directly exposed via a getter. This could potentially allow external modification of the list, which might lead to unexpected behavior. Consider returning a copy of the list instead to ensure encapsulation.
- List<Wal> get missingWals => _missingWals;
+ List<Wal> get missingWals => List.from(_missingWals);| MemoryProvider() { | ||
| _wal.subscribe(this, this); | ||
| _preload(); | ||
| } |
There was a problem hiding this comment.
In the constructor, you are subscribing to the _wal service before calling _preload(). If the _wal service emits events between these two calls, those events will be missed. To avoid this race condition, consider preloading the data before subscribing to the service.
- MemoryProvider() {
- _wal.subscribe(this, this);
- _preload();
- }
+ MemoryProvider() {
+ _preload().then((_) {
+ _wal.subscribe(this, this);
+ });
+ }| @override | ||
| void onNewMissingWal(Wal wal) async { | ||
| _missingWals = await _wal.getMissingWals(); | ||
| notifyListeners(); | ||
| } | ||
|
|
||
| @override | ||
| void onWalSynced(Wal wal, ServerMemory memory) async { | ||
| _missingWals = await _wal.getMissingWals(); | ||
| notifyListeners(); | ||
| } | ||
|
|
||
| @override | ||
| void onStatusChanged(WalServiceStatus status) {} | ||
|
|
||
| void syncWals() { | ||
| _wal.syncAll(); | ||
| } |
There was a problem hiding this comment.
There's a lot of repetition in onNewMissingWal, onWalSynced, and syncWals methods where you're fetching missing Wals and notifying listeners. This violates the DRY (Don't Repeat Yourself) principle. Consider creating a separate method for this repeated code and call it in these methods.
- @override
- void onNewMissingWal(Wal wal) async {
- _missingWals = await _wal.getMissingWals();
- notifyListeners();
- }
-
- @override
- void onWalSynced(Wal wal, ServerMemory memory) async {
- _missingWals = await _wal.getMissingWals();
- notifyListeners();
- }
+ void _updateMissingWals() async {
+ _missingWals = await _wal.getMissingWals();
+ notifyListeners();
+ }
+ @override
+ void onNewMissingWal(Wal wal) async {
+ _updateMissingWals();
+ }
+ @override
+ void onWalSynced(Wal wal, ServerMemory memory) async {
+ _updateMissingWals();
+ }Also, the onStatusChanged method is empty. If you don't plan to implement it, consider throwing a UnimplementedError to avoid silent failures.
- void onStatusChanged(WalServiceStatus status) {}
+ void onStatusChanged(WalServiceStatus status) {
+ throw UnimplementedError();
+ }
There was a problem hiding this comment.
Files selected (6)
- app/lib/backend/schema/message_event.dart (4)
- app/lib/providers/memory_provider.dart (3)
- app/lib/services/sockets/wal_connection.dart (1)
- app/lib/services/wals.dart (1)
- backend/models/message_event.py (1)
- backend/routers/transcribe_v2.py (2)
Files not summarized due to errors (1)
- app/lib/services/wals.dart (diff tokens exceeds limit)
Files skipped from review due to trivial changes (1)
- app/lib/backend/schema/message_event.dart
Review comments generated (14)
- Review: 13
- 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.
|
|
||
| class MemoryBackwardSycnedEvent(MessageEvent): | ||
| name: Optional[str] = None | ||
|
|
||
| def to_json(self): | ||
| j = self.model_dump(mode="json") | ||
| j["type"] = self.event_type | ||
| return j |
There was a problem hiding this comment.
The new class MemoryBackwardSycnedEvent seems to have a typo in its name. It should be MemoryBackwardSyncedEvent. Also, the to_json method is identical to the one in the parent class MessageEvent. If there's no additional logic needed for this subclass, you can remove this overridden method.
- class MemoryBackwardSycnedEvent(MessageEvent):
+ class MemoryBackwardSyncedEvent(MessageEvent):
name: Optional[str] = None
- def to_json(self):
- j = self.model_dump(mode="json")
- j["type"] = self.event_type
- return j
app/lib/services/wals.dart
Outdated
| _syncFrameSeq.add(seq); | ||
| } | ||
|
|
||
| @override | ||
| Future<bool> syncWal(Wal wal) async { | ||
| // TODO: implement syncWal | ||
| return true; | ||
| } | ||
|
|
||
| @override | ||
| Future syncAll({IWalSyncProgressListener? progress}) async { | ||
| _wals.removeWhere((wal) => wal.status == WalStatus.synced); | ||
| await _flush(); | ||
| var wals = _wals.where((w) => w.status == WalStatus.miss && w.storage == WalStorage.disk).toList(); | ||
| if (wals.isEmpty) { | ||
| debugPrint("All synced!"); | ||
| return; | ||
| } | ||
|
|
||
| // Establish connection | ||
| _socket?.stop(); | ||
| _socket = WalSocketService.create(wals.map<String>((wal) => wal.getFileName()).toList()); | ||
| await _socket?.start(); | ||
| if (_socket?.state != SocketServiceState.connected) { | ||
| _socket?.stop(); | ||
| debugPrint("Cant not connect to socket!"); | ||
| return; | ||
| } | ||
| _socket?.subscribe(this, this); | ||
|
|
||
| for (var i = 0; i < wals.length; i++) { | ||
| var wal = wals[i]; | ||
| debugPrint("sync id ${wal.id}"); | ||
| if (wal.filePath == null) { | ||
| debugPrint("sync error: file path is not found. wal id ${wal.id}"); | ||
| continue; | ||
| } | ||
|
|
||
| try { | ||
| File file = File(wal.filePath!); | ||
| var bytes = await file.readAsBytes(); | ||
|
|
||
| final byteFrame = ByteData(12 + bytes.length); | ||
| byteFrame.setUint32(0, 1, Endian.big); // 0001, start new file | ||
| byteFrame.setUint32(4, i, Endian.big); // index | ||
| byteFrame.setUint32(8, bytes.length, Endian.big); // length | ||
| for (int i = 0; i < bytes.length; i++) { | ||
| byteFrame.setUint8(i + 12, bytes[i]); | ||
| } | ||
| if (_socket?.state != SocketServiceState.connected) { | ||
| debugPrint("sync error: socket is closed. wal id ${wal.id}"); | ||
| break; | ||
| } | ||
| _socket?.send(byteFrame.buffer.asUint8List()); | ||
|
|
||
| debugPrint("sync wal ${wal.id} file ${wal.filePath} length ${bytes.length}"); | ||
| debugPrint("[${bytes.sublist(0, 100).join(", ")}]"); | ||
| } catch (e) { | ||
| debugPrint(e.toString()); | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| // End | ||
| final byteFrame = ByteData(4); | ||
| byteFrame.setUint32(0, 0, Endian.big); // 0000, end | ||
| _socket?.send(byteFrame.buffer.asUint8List()); | ||
| } | ||
|
|
||
| @override | ||
| void onClosed() {} | ||
|
|
||
| @override | ||
| void onConnected() {} | ||
|
|
||
| @override | ||
| void onError(Object err) {} | ||
|
|
||
| @override | ||
| void onMessageEventReceived(ServerMessageEvent event) async { | ||
| if (event.type == MessageEventType.memoyBackwardSynced) { | ||
| debugPrint("onMessageEventReceived > memoyBackwardSynced"); | ||
| int? timestamp = int.tryParse(event.name?.split("_")[1] ?? ""); | ||
| final idx = _wals.indexWhere((w) => w.timestamp == timestamp); | ||
| if (idx <= 0) { | ||
| return; | ||
| } | ||
| var wal = _wals[idx]; | ||
|
|
||
| // send | ||
| for (var sub in _subscriptions.values) { | ||
| sub.onWalSynced(wal); | ||
| } | ||
|
|
||
| // update | ||
| await _deleteWal(wal); | ||
| SharedPreferencesUtil().wals = _wals; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The WalService class seems to be handling a lot of responsibilities. It's managing the Write-Ahead Logging (WAL), chunking frames, flushing data, handling subscriptions, and also dealing with WebSocket communication. This is a violation of the Single Responsibility Principle (SRP) of SOLID principles.
Consider breaking down this class into smaller classes each handling a single responsibility. For instance, you could have a separate class for managing WebSocket communication, another for managing WALs, and so on. This will make your code more maintainable and easier to test.
Also, there are no error handling mechanisms in place for the async operations. For example, in the _chunk method (lines 159-221), if an error occurs during the execution of the method, it will not be caught and handled. This can lead to unexpected behavior or crashes. Consider adding try-catch blocks around these operations to handle potential errors gracefully.
Here's an example of how you might refactor the _chunk method to include error handling:
+ Future _chunk() async {
+ try {
+ debugPrint("_chunk");
+ if (_frames.isEmpty) {
+ debugPrint("Frames are empty");
+ return;
+ }
+ // rest of the method...
+ } catch (e) {
+ debugPrint("Error occurred while chunking: $e");
+ }
+ }Lastly, I noticed that the syncAll method (lines 333-390) sends data over the socket without checking if the socket connection is still open. This could potentially lead to unhandled exceptions if the connection is closed unexpectedly. Consider adding checks before sending data to ensure the connection is still open.
| final recordingsLanguage = SharedPreferencesUtil().recordingsLanguage; | ||
| var params = '?language=$recordingsLanguage&uid=${SharedPreferencesUtil().uid}&file_names=${fileNames.join(",")}'; | ||
| String url = '${Env.apiBaseUrl!.replaceAll('https', 'wss')}v2/listen/backward$params'; |
There was a problem hiding this comment.
The URL parameters are being constructed manually which can lead to errors if not done carefully. It's better to use Uri class to construct the URL with query parameters. This will ensure proper encoding of the parameters and make the code more robust.
- var params = '?language=$recordingsLanguage&uid=${SharedPreferencesUtil().uid}&file_names=${fileNames.join(",")}';
- String url = '${Env.apiBaseUrl!.replaceAll('https', 'wss')}v2/listen/backward$params';
+ final uri = Uri(
+ scheme: 'wss',
+ host: Env.apiBaseUrl!.host,
+ path: 'v2/listen/backward',
+ queryParameters: {
+ 'language': recordingsLanguage,
+ 'uid': SharedPreferencesUtil().uid,
+ 'file_names': fileNames.join(","),
+ },
+ );
+ String url = uri.toString();| bool ok = await _socket.connect(); | ||
| if (!ok) { | ||
| debugPrint("Can not connect to websocket"); | ||
| } |
There was a problem hiding this comment.
There is no error handling for when the connection to the WebSocket fails. Consider adding a mechanism to handle this scenario, such as retrying the connection or notifying the user about the issue.
| _socket.send(message); | ||
| return; |
There was a problem hiding this comment.
The send method is marked as async but it doesn't seem to be doing any asynchronous operation. If _socket.send(message) is not an async function, you can remove the async keyword from the method signature.
- Future send(dynamic message) async {
+ void send(dynamic message) {
_socket.send(message);
- return;
}| import 'package:friend_private/services/services.dart'; | ||
| import 'package:friend_private/services/wals.dart'; |
There was a problem hiding this comment.
The new imports for services.dart and wals.dart are added correctly. No issues found.
+ import 'package:friend_private/services/services.dart';
+ import 'package:friend_private/services/wals.dart';| IWalService get _wal => ServiceManager.instance().wal; | ||
| List<Wal> _missingWals = []; | ||
| List<Wal> get missingWals => _missingWals; | ||
|
|
||
| MemoryProvider() { | ||
| _wal.subscribe(this, this); | ||
| _preload(); | ||
| } |
There was a problem hiding this comment.
The constructor of MemoryProvider now subscribes to the IWalService and preloads missing Wals. This is a good practice as it initializes necessary data at object creation. However, error handling is missing in _preload() method. If there's an error while getting missing Wals, it should be handled properly.
+ IWalService get _wal => ServiceManager.instance().wal;
+ List<Wal> _missingWals = [];
+ List<Wal> get missingWals => _missingWals;
+
+ MemoryProvider() {
+ _wal.subscribe(this, this);
+ _preload();
+ }
+
+ _preload() async {
+ try {
+ _missingWals = await _wal.getMissingWals();
+ } catch (e) {
+ // Handle or log the error here
+ }
+ notifyListeners();
+ }| _wal.unsubscribe(this); | ||
| super.dispose(); |
There was a problem hiding this comment.
Unsubscribing from the IWalService in the dispose method is a good practice to prevent memory leaks. No issues found.
+ _wal.unsubscribe(this);
+ super.dispose();| void onNewMissingWal(Wal wal) async { | ||
| _missingWals = await _wal.getMissingWals(); | ||
| notifyListeners(); | ||
| } | ||
|
|
||
| @override | ||
| void onWalSynced(Wal wal, {ServerMemory? memory}) async { | ||
| _missingWals = await _wal.getMissingWals(); | ||
| notifyListeners(); | ||
| } |
There was a problem hiding this comment.
The methods onNewMissingWal and onWalSynced are updating _missingWals and notifying listeners. However, they are doing the same thing. Consider refactoring this to avoid code duplication. Also, add error handling when getting missing Wals.
+ @override
+ void onNewMissingWal(Wal wal) async {
+ await updateMissingWals();
+ }
+
+ @override
+ void onWalSynced(Wal wal, {ServerMemory? memory}) async {
+ await updateMissingWals();
+ }
+
+ Future<void> updateMissingWals() async {
+ try {
+ _missingWals = await _wal.getMissingWals();
+ } catch (e) {
+ // Handle or log the error here
+ }
+ notifyListeners();
+ }| void onStatusChanged(WalServiceStatus status) {} | ||
|
|
||
| void syncWals() { | ||
| _wal.syncAll(); | ||
| } |
There was a problem hiding this comment.
The onStatusChanged method is empty. If it's not used, consider removing it to avoid confusion. The syncWals method correctly triggers syncing of all Wals.
- @override
- void onStatusChanged(WalServiceStatus status) {}
+
+ void syncWals() {
+ _wal.syncAll();
+ }
There was a problem hiding this comment.
Files selected (1)
- app/lib/services/wals.dart (1)
Files not summarized due to errors (1)
- app/lib/services/wals.dart (diff tokens exceeds limit)
Review comments generated (1)
- Review: 1
- LGTM: 0
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.
| _syncFrameSeq.add(seq); | ||
| } | ||
|
|
||
| @override | ||
| Future<bool> syncWal(Wal wal) async { | ||
| // TODO: implement syncWal | ||
| return true; | ||
| } | ||
|
|
||
| @override | ||
| Future syncAll({IWalSyncProgressListener? progress}) async { | ||
| _wals.removeWhere((wal) => wal.status == WalStatus.synced); | ||
| await _flush(); | ||
| var wals = _wals.where((w) => w.status == WalStatus.miss && w.storage == WalStorage.disk).toList(); | ||
| if (wals.isEmpty) { | ||
| debugPrint("All synced!"); | ||
| return; | ||
| } | ||
|
|
||
| // Establish connection | ||
| _socket?.stop(); | ||
| _socket = WalSocketService.create(wals.map<String>((wal) => wal.getFileName()).toList()); | ||
| await _socket?.start(); | ||
| if (_socket?.state != SocketServiceState.connected) { | ||
| _socket?.stop(); | ||
| debugPrint("Cant not connect to socket!"); | ||
| return; | ||
| } | ||
| _socket?.subscribe(this, this); | ||
|
|
||
| for (var i = 0; i < wals.length; i++) { | ||
| var wal = wals[i]; | ||
| debugPrint("sync id ${wal.id}"); | ||
| if (wal.filePath == null) { | ||
| debugPrint("sync error: file path is not found. wal id ${wal.id}"); | ||
| continue; | ||
| } | ||
|
|
||
| try { | ||
| File file = File(wal.filePath!); | ||
| var bytes = await file.readAsBytes(); | ||
|
|
||
| final byteFrame = ByteData(12 + bytes.length); | ||
| byteFrame.setUint32(0, 1, Endian.big); // 0001, start new file | ||
| byteFrame.setUint32(4, i, Endian.big); // index | ||
| byteFrame.setUint32(8, bytes.length, Endian.big); // length | ||
| for (int i = 0; i < bytes.length; i++) { | ||
| byteFrame.setUint8(i + 12, bytes[i]); | ||
| } | ||
| if (_socket?.state != SocketServiceState.connected) { | ||
| debugPrint("sync error: socket is closed. wal id ${wal.id}"); | ||
| break; | ||
| } | ||
| _socket?.send(byteFrame.buffer.asUint8List()); | ||
|
|
||
| debugPrint("sync wal ${wal.id} file ${wal.filePath} length ${bytes.length}"); | ||
| //debugPrint("[${bytes.sublist(0, 100).join(", ")}]"); | ||
| } catch (e) { | ||
| debugPrint(e.toString()); | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| // End | ||
| final byteFrame = ByteData(4); | ||
| byteFrame.setUint32(0, 0, Endian.big); // 0000, end | ||
| _socket?.send(byteFrame.buffer.asUint8List()); | ||
| } | ||
|
|
||
| @override | ||
| void onClosed() {} | ||
|
|
||
| @override | ||
| void onConnected() {} | ||
|
|
||
| @override | ||
| void onError(Object err) {} | ||
|
|
||
| @override | ||
| void onMessageEventReceived(ServerMessageEvent event) async { | ||
| if (event.type == MessageEventType.memoyBackwardSynced) { | ||
| int? timerStart = int.tryParse(event.name?.split("_")[1] ?? ""); | ||
| final idx = _wals.indexWhere((w) => w.timerStart == timerStart); | ||
| if (idx < 0) { | ||
| debugPrint("Wal is not found $timerStart"); | ||
| return; | ||
| } | ||
| var wal = _wals[idx]; | ||
|
|
||
| // update | ||
| await _deleteWal(wal); | ||
| SharedPreferencesUtil().wals = _wals; | ||
|
|
||
| // send | ||
| for (var sub in _subscriptions.values) { | ||
| sub.onWalSynced(wal); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The WalService class seems to be handling a lot of responsibilities. It's managing the Write-Ahead Logging (WAL), chunking frames, flushing data, handling subscriptions, and also interacting with the WebSocket service. This violates the Single Responsibility Principle (SRP) of SOLID principles, which states that a class should have only one reason to change.
To improve maintainability and modularity, consider breaking down this class into smaller classes each having its own responsibility. For instance, you could create separate classes for managing WALs, handling WebSocket communication, and managing subscriptions.
Also, there are no error handling mechanisms in place for many of the async operations like file read/write operations, socket connections, etc. This can lead to unhandled exceptions causing the application to crash or behave unexpectedly. Consider adding appropriate error handling mechanisms to improve the robustness of your code.
Here is an example of how you might refactor the file writing operation with error handling:
- final file = File(filePath);
- await file.writeAsBytes(data);
+ try {
+ final file = File(filePath);
+ await file.writeAsBytes(data);
+ } catch (e) {
+ debugPrint("Error writing to file: $e");
+ // Handle or throw the error as needed
+ }Please apply similar changes to other parts of the code where necessary.
Issue: #958
Summary by Entelligence.AI
main.dartfor better code organization.memory_backward_synced, allowing more granular tracking of synchronization events.