Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Processing memory with status, capturing to, get list and more. #930

Merged
merged 11 commits into from
Sep 28, 2024
Merged

Processing memory with status, capturing to, get list and more. #930

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d013348
Processing memory with status, capturing to, get list and more.
beastoin Sep 27, 2024
280adb7
Keep capturing memory when disconnects happened
beastoin Sep 27, 2024
e02d7da
Set pre-segments for capturing memory
beastoin Sep 27, 2024
15b0580
Processing memory with loading indicator
beastoin Sep 27, 2024
e6c8d3e
Notify memories with new done processing memory
beastoin Sep 28, 2024
6076bc4
Processing memory status changed w/ socket
beastoin Sep 28, 2024
7fcacfd
Notify combining memory to memories page
beastoin Sep 28, 2024
8f761c8
Rebased #202409281519
beastoin Sep 28, 2024
2b96942
Minor fixes on event new processing memory created, duplicated new me…
beastoin Sep 28, 2024
75bd737
Fix capturing_to on transcript updates
beastoin Sep 28, 2024
d0a8e69
Simplify combine proccessing memory conditional
beastoin Sep 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions app/lib/backend/http/api/memories.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,24 @@ Future<List<ServerMemory>> getMemories({int limit = 50, int offset = 0}) async {
return [];
}

Future<List<ServerProcessingMemory>> getProcessingMemories(
{List<String> filterIds = const [], int limit = 5, int offset = 0}) async {
var url = '${Env.apiBaseUrl}v1/processing-memories?filter_ids=${filterIds.join(",")}&limit=$limit&offset=$offset';
var response = await makeApiCall(url: url, headers: {}, method: 'GET', body: '');
if (response == null) return [];
if (response.statusCode == 200) {
// decode body bytes to utf8 string and then parse json so as to avoid utf8 char issues
var body = utf8.decode(response.bodyBytes);
var memories =
(jsonDecode(body)["result"] as List<dynamic>).map((memory) => ServerProcessingMemory.fromJson(memory)).toList();
debugPrint('getProcessingMemories length: ${memories.length}');
return memories;
} else {
debugPrint("[API-Error] $url - ${response.statusCode}");
}
return [];
}
Comment on lines +89 to +105
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The getProcessingMemories function is making an API call but it doesn't handle all possible HTTP status codes. It only checks for a 200 status code, and if the response is not null. However, there could be other status codes returned by the server such as 400 (Bad Request), 401 (Unauthorized), 403 (Forbidden), 404 (Not Found), 500 (Internal Server Error), etc. These should be handled appropriately to provide meaningful feedback to the user or for debugging purposes.

Also, the headers for the API call are empty. If your API requires any form of authentication, you might need to include appropriate headers.

Lastly, the debug print statements on lines 99 and 102 are not ideal for production code. Consider using a logging library that can be toggled for different environments (development, staging, production).

Here's a suggestion on how to improve this:

Future<List<ServerProcessingMemory>> getProcessingMemories(
    {List<String> filterIds = const [], int limit = 5, int offset = 0}) async {
  var url = '${Env.apiBaseUrl}v1/processing-memories?filter_ids=${filterIds.join(",")}&limit=$limit&offset=$offset';
  var response = await makeApiCall(url: url, headers: /* Add necessary headers here */, method: 'GET', body: '');
  if (response == null) return [];
  switch(response.statusCode) {
    case 200:
      // decode body bytes to utf8 string and then parse json so as to avoid utf8 char issues
      var body = utf8.decode(response.bodyBytes);
      var memories =
          (jsonDecode(body)["result"] as List<dynamic>).map((memory) => ServerProcessingMemory.fromJson(memory)).toList();
      // Use a logging library instead of debugPrint
      log('getProcessingMemories length: ${memories.length}');
      return memories;
    case 400:
      // Handle bad request
      break;
    case 401:
      // Handle unauthorized
      break;
    // Add more cases as necessary
    default:
      // Use a logging library instead of debugPrint
      log("[API-Error] $url - ${response.statusCode}");
  }
  return [];
}

Please replace /* Add necessary headers here */ with the actual headers required for your API call and replace log with your preferred logging method.


Future<ServerMemory?> reProcessMemoryServer(String memoryId) async {
var response = await makeApiCall(
url: '${Env.apiBaseUrl}v1/memories/$memoryId/reprocess',
Expand Down
29 changes: 26 additions & 3 deletions app/lib/backend/http/api/processing_memories.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,13 @@ Future<UpdateProcessingMemoryResponse?> updateProcessingMemoryServer({
bodyData.addAll({"geolocation": geolocation.toJson()});
}

debugPrint(jsonEncode(bodyData));

var response = await makeApiCall(
url: '${Env.apiBaseUrl}v1/processing-memories/$id',
headers: {},
method: 'PATCH',
body: jsonEncode(bodyData),
);
if (response == null) return null;
debugPrint('updateProcessingMemoryServer: ${response.body}');
if (response.statusCode == 200) {
return UpdateProcessingMemoryResponse.fromJson(jsonDecode(response.body));
} else {
Comment on lines 24 to 33
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The makeApiCall function is called with an empty headers object. If this API requires authentication, you should include the necessary authorization headers. Also, error handling seems to be missing for non-200 status codes. It would be beneficial to add some form of error logging or reporting in case the API call fails.

-      headers: {},
+      headers: {
+        'Authorization': 'Bearer $token',
+      },
-    } else {
+    } else {
+      // Log or report error
+      print('Error updating processing memory: ${response.statusCode}');
+    }

Expand All @@ -46,3 +43,29 @@ Future<UpdateProcessingMemoryResponse?> updateProcessingMemoryServer({
}
return null;
}

Future<ProcessingMemoryResponse?> fetchProcessingMemoryServer({
required String id,
}) async {
var response = await makeApiCall(
url: '${Env.apiBaseUrl}v1/processing-memories/$id',
headers: {},
method: 'GET',
body: "",
);
if (response == null) return null;
if (response.statusCode == 200) {
return ProcessingMemoryResponse.fromJson(jsonDecode(response.body));
} else {
// TODO: Server returns 304 doesn't recover
CrashReporting.reportHandledCrash(
Exception('Failed to create memory'),
StackTrace.current,
level: NonFatalExceptionLevel.info,
userAttributes: {
'response': response.body,
},
);
}
return null;
Comment on lines +50 to +70
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

Similar to the previous comment, the makeApiCall function is called with an empty headers object. If this API requires authentication, you should include the necessary authorization headers. Additionally, the body parameter for a GET request is typically not used and can be omitted. Lastly, the exception message at line 62 says 'Failed to create memory', which seems incorrect as this function is fetching a memory, not creating one.

-     headers: {},
+     headers: {
+       'Authorization': 'Bearer $token',
+     },
-     body: "",
-       Exception('Failed to create memory'),
+       Exception('Failed to fetch memory'),

}
50 changes: 50 additions & 0 deletions app/lib/backend/schema/memory.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import 'dart:convert';
import 'dart:math';

import 'package:collection/collection.dart';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The import of package:collection/collection.dart is new. Ensure that this package is actually used in the file, otherwise, it's an unnecessary import which can be removed to keep the code clean and efficient.

- import 'package:collection/collection.dart';

import 'package:flutter/material.dart';
import 'package:friend_private/backend/schema/geolocation.dart';
import 'package:friend_private/backend/schema/message.dart';
Expand Down Expand Up @@ -57,22 +58,52 @@ class MemoryPostProcessing {
toJson() => {'status': status.toString().split('.').last, 'model': model.toString().split('.').last};
}

enum ServerProcessingMemoryStatus {
capturing('capturing'),
processing('processing'),
done('done'),
unknown('unknown'),
;

final String value;
const ServerProcessingMemoryStatus(this.value);

static ServerProcessingMemoryStatus valuesFromString(String value) {
return ServerProcessingMemoryStatus.values.firstWhereOrNull((e) => e.value == value) ??
ServerProcessingMemoryStatus.unknown;
}
}
Comment on lines +61 to +75
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The ServerProcessingMemoryStatus enum has been introduced with a method valuesFromString. This method returns unknown if the provided string doesn't match any of the enum values. This could potentially hide errors if an unexpected status value is received from the server. It might be better to throw an error or at least log a warning in such cases.

static ServerProcessingMemoryStatus valuesFromString(String value) {
  return ServerProcessingMemoryStatus.values.firstWhereOrNull((e) => e.value == value) ??
- 73:       ServerProcessingMemoryStatus.unknown;
+ 73:       (throw ArgumentError('Unknown ServerProcessingMemoryStatus: $value'));
}


class ServerProcessingMemory {
final String id;
final DateTime createdAt;
final DateTime? startedAt;
final DateTime? capturingTo;
final ServerProcessingMemoryStatus? status;
final List<TranscriptSegment> transcriptSegments;
final String? memoryId;

ServerProcessingMemory({
required this.id,
required this.createdAt,
this.startedAt,
this.capturingTo,
this.status,
this.transcriptSegments = const [],
this.memoryId,
});

factory ServerProcessingMemory.fromJson(Map<String, dynamic> json) {
return ServerProcessingMemory(
id: json['id'],
createdAt: DateTime.parse(json['created_at']).toLocal(),
startedAt: json['started_at'] != null ? DateTime.parse(json['started_at']).toLocal() : null,
capturingTo: json['capturing_to'] != null ? DateTime.parse(json['capturing_to']).toLocal() : null,
status: json['status'] != null ? ServerProcessingMemoryStatus.valuesFromString(json['status']) : null,
transcriptSegments: ((json['transcript_segments'] ?? []) as List<dynamic>)
.map((segment) => TranscriptSegment.fromJson(segment))
.toList(),
memoryId: json['memory_id'],
Comment on lines 77 to +106
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The ServerProcessingMemory class has been updated with new fields capturingTo, status, and memoryId. The fromJson factory method has also been updated accordingly. However, there is no null safety check for json['id'] and json['created_at']. If these fields are not present in the JSON object, the application will crash. Consider adding null checks for these fields.

factory ServerProcessingMemory.fromJson(Map<String, dynamic> json) {
- 97:   return ServerProcessingMemory(
- 98:     id: json['id'],
- 99:     createdAt: DateTime.parse(json['created_at']).toLocal(),
+ 97:   if (json['id'] != null && json['created_at'] != null) {
+ 98:     return ServerProcessingMemory(
+ 99:       id: json['id'],
+ 100:      createdAt: DateTime.parse(json['created_at']).toLocal(),
     startedAt: json['started_at'] != null ? DateTime.parse(json['started_at']).toLocal() : null,
    capturingTo: json['capturing_to'] != null ? DateTime.parse(json['capturing_to']).toLocal() : null,
    status: json['status'] != null ? ServerProcessingMemoryStatus.valuesFromString(json['status']) : null,
    transcriptSegments: ((json['transcript_segments'] ?? []) as List<dynamic>)
        .map((segment) => TranscriptSegment.fromJson(segment))
        .toList(),
    memoryId: json['memory_id'],
+ 107:    );
+ 108:  } else {
+ 109:    throw ArgumentError('Missing required fields in JSON');
+ 110:  }
111:}

);
}

Expand All @@ -81,6 +112,10 @@ class ServerProcessingMemory {
'id': id,
'created_at': createdAt.toUtc().toIso8601String(),
'started_at': startedAt?.toUtc().toIso8601String(),
'capturing_to': capturingTo?.toUtc().toIso8601String(),
'status': status.toString(),
Comment on lines +115 to +116
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The toJson method of ServerProcessingMemory has been updated to include capturingTo and status. This change is consistent with the updates made to the ServerProcessingMemory class.

'transcript_segments': transcriptSegments.map((segment) => segment.toJson()).toList(),
Comment on lines +115 to +117
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The toJson method of ServerProcessingMemory has been updated to include the new fields capturingTo and status. This is a necessary change for serialization of the updated class.

'memory_id': memoryId,
Comment on lines +115 to +118
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The toJson method has been updated to include the new fields. However, for the status field, it's directly calling toString() which will return the full name of the enum including the class name (e.g., ServerProcessingMemoryStatus.capturing). Consider using split('.').last to get only the value part of the enum.

- 'status': status.toString(),
+ 'status': status.toString().split('.').last,

};
}

Expand All @@ -97,6 +132,18 @@ class ServerProcessingMemory {
}
}

class ProcessingMemoryResponse {
final ServerProcessingMemory? result;

ProcessingMemoryResponse({required this.result});

factory ProcessingMemoryResponse.fromJson(Map<String, dynamic> json) {
return ProcessingMemoryResponse(
result: json['result'] != null ? ServerProcessingMemory.fromJson(json['result']) : null,
);
}
}
Comment on lines +135 to +145
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

A new class ProcessingMemoryResponse has been introduced with a factory method fromJson. This method assumes that if json['result'] is not null, it can be parsed into a ServerProcessingMemory object. If the structure of json['result'] is not as expected, this could lead to runtime errors. Consider adding error handling or validation here.

factory ProcessingMemoryResponse.fromJson(Map<String, dynamic> json) {
- 141:   return ProcessingMemoryResponse(
- 142:     result: json['result'] != null ? ServerProcessingMemory.fromJson(json['result']) : null,
+ 141:   if (json['result'] != null) {
+ 142:     try {
+ 143:       return ProcessingMemoryResponse(
+ 144:         result: ServerProcessingMemory.fromJson(json['result']),
+ 145:       );
+ 146:     } catch (e) {
+ 147:       throw FormatException('Invalid format for ServerProcessingMemory');
+ 148:     }
+ 149:   } else {
+ 150:     return ProcessingMemoryResponse(result: null);
+ 151:   }
}


class UpdateProcessingMemoryResponse {
final ServerProcessingMemory? result;

Expand Down Expand Up @@ -135,6 +182,9 @@ class ServerMemory {
final bool failed;
int retries;

// local label
bool isNew = false;

Comment on lines +185 to +187
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

A new local label isNew has been added to the ServerMemory class. Ensure that this field is used somewhere in the codebase, otherwise, it's an unnecessary addition which can be removed to keep the code clean and efficient.

- bool isNew = false;

ServerMemory({
required this.id,
required this.createdAt,
Expand Down
6 changes: 6 additions & 0 deletions app/lib/backend/schema/message_event.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ enum MessageEventType {
memoryPostProcessingSuccess('memory_post_processing_success'),
memoryPostProcessingFailed('memory_post_processing_failed'),
newProcessingMemoryCreated('new_processing_memory_created'),
processingMemoryStatusChanged('processing_memory_status_changed'),
ping('ping'),
unknown('unknown'),
;
Expand All @@ -26,13 +27,15 @@ class ServerMessageEvent {
String? processingMemoryId;
ServerMemory? memory;
List<ServerMessage>? messages;
ServerProcessingMemoryStatus? processingMemoryStatus;

ServerMessageEvent(
this.type,
this.memoryId,
this.processingMemoryId,
this.memory,
this.messages,
this.processingMemoryStatus,
);

static ServerMessageEvent fromJson(Map<String, dynamic> json) {
Expand All @@ -42,6 +45,9 @@ class ServerMessageEvent {
json['processing_memory_id'],
json['memory'] != null ? ServerMemory.fromJson(json['memory']) : null,
((json['messages'] ?? []) as List<dynamic>).map((message) => ServerMessage.fromJson(message)).toList(),
json['processing_memory_status'] != null
? ServerProcessingMemoryStatus.valuesFromString(json['processing_memory_status'])
: null,
);
}
}
1 change: 0 additions & 1 deletion app/lib/main.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,6 @@ class _MyAppState extends State<MyApp> with WidgetsBindingObserver {
@override
void didChangeAppLifecycleState(AppLifecycleState state) {
super.didChangeAppLifecycleState(state);
debugPrint("App > lifecycle changed $state");
if (state == AppLifecycleState.detached) {
_deinit();
}
Expand Down
1 change: 1 addition & 0 deletions app/lib/pages/memories/page.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ class _MemoriesPageState extends State<MemoriesPage> with AutomaticKeepAliveClie
const SliverToBoxAdapter(child: SizedBox(height: 32)),
const SliverToBoxAdapter(child: SpeechProfileCardWidget()),
SliverToBoxAdapter(child: getMemoryCaptureWidget()),
getProcessingMemoriesWidget(memoryProvider.processingMemories),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The getProcessingMemoriesWidget function is being called with an argument, but it's not clear if this function can handle null values. If the processingMemories list in memoryProvider is null, this could potentially lead to a runtime error. Consider adding null safety checks either within the getProcessingMemoriesWidget function or before calling it.

- getProcessingMemoriesWidget(memoryProvider.processingMemories),
+ getProcessingMemoriesWidget(memoryProvider.processingMemories ?? []),

This will ensure that an empty list is passed to the function if processingMemories is null, preventing potential null dereference errors.

if (memoryProvider.groupedMemories.isEmpty && !memoryProvider.isLoadingMemories)
const SliverToBoxAdapter(
child: Center(
Expand Down
37 changes: 0 additions & 37 deletions app/lib/pages/memories/widgets/capture.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,50 +77,13 @@ class LiteCaptureWidgetState extends State<LiteCaptureWidget>
super.dispose();
}

// TODO: use connection directly
Future<BleAudioCodec> _getAudioCodec(String deviceId) async {
var connection = await ServiceManager.instance().device.ensureConnection(deviceId);
if (connection == null) {
return BleAudioCodec.pcm8;
}
return connection.getAudioCodec();
}
// Future requestLocationPermission() async {
// LocationService locationService = LocationService();
// bool serviceEnabled = await locationService.enableService();
// if (!serviceEnabled) {
// debugPrint('Location service not enabled');
// if (mounted) {
// ScaffoldMessenger.of(context).showSnackBar(
// const SnackBar(
// content: Text(
// 'Location services are disabled. Enable them for a better experience.',
// style: TextStyle(color: Colors.white, fontSize: 14),
// ),
// ),
// );
// }
// } else {
// PermissionStatus permissionGranted = await locationService.requestPermission();
// SharedPreferencesUtil().locationEnabled = permissionGranted == PermissionStatus.granted;
// MixpanelManager().setUserProperty('Location Enabled', SharedPreferencesUtil().locationEnabled);
// if (permissionGranted == PermissionStatus.denied) {
// debugPrint('Location permission not granted');
// } else if (permissionGranted == PermissionStatus.deniedForever) {
// debugPrint('Location permission denied forever');
// if (mounted) {
// ScaffoldMessenger.of(context).showSnackBar(
// const SnackBar(
// content: Text(
// 'If you change your mind, you can enable location services in your device settings.',
// style: TextStyle(color: Colors.white, fontSize: 14),
// ),
// ),
// );
// }
// }
// }
// }

@override
Widget build(BuildContext context) {
Expand Down
17 changes: 13 additions & 4 deletions app/lib/pages/memories/widgets/memory_list_item.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ class _MemoryListItemState extends State<MemoryListItem> {
onTap: () async {
MixpanelManager().memoryListItemClicked(widget.memory, widget.memoryIdx);
context.read<MemoryDetailProvider>().updateMemory(widget.memoryIdx, widget.date);
Provider.of<MemoryProvider>(context, listen: false).onMemoryTap(widget.memoryIdx);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The Provider.of<MemoryProvider>(context, listen: false).onMemoryTap(widget.memoryIdx); line is added to handle the memory tap event. However, it's not clear if this method has any side effects or if it returns a Future. If it does, you should consider awaiting it to ensure that any potential exceptions are properly handled and that the operation completes before moving on.

- Provider.of<MemoryProvider>(context, listen: false).onMemoryTap(widget.memoryIdx);
+ await Provider.of<MemoryProvider>(context, listen: false).onMemoryTap(widget.memoryIdx);

routeToPage(
context,
MemoryDetailPage(memory: widget.memory, isFromOnboarding: widget.isFromOnboarding),
Expand All @@ -46,10 +47,18 @@ class _MemoryListItemState extends State<MemoryListItem> {
EdgeInsets.only(top: 12, left: widget.isFromOnboarding ? 0 : 16, right: widget.isFromOnboarding ? 0 : 16),
child: Container(
width: double.maxFinite,
decoration: BoxDecoration(
color: Colors.grey.shade900,
borderRadius: BorderRadius.circular(16.0),
),
decoration: widget.memory.isNew
? BoxDecoration(
color: Colors.grey.shade900,
borderRadius: BorderRadius.circular(16.0),
border: Border.all(
color: Colors.lightBlue,
width: 1,
))
: BoxDecoration(
color: Colors.grey.shade900,
borderRadius: BorderRadius.circular(16.0),
),
Comment on lines +50 to +61
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

This change introduces a conditional decoration for the widget based on whether the memory is new or not. It seems fine from a logic perspective, but make sure that the UI changes have been thoroughly tested across different screen sizes and resolutions to ensure that they look as expected.

child: ClipRRect(
borderRadius: BorderRadius.circular(16.0),
child: Dismissible(
Expand Down
Loading
Loading