Add undo delete memory option #858
Conversation
|
WalkthroughThis update refactors the memory deletion process in the application. The changes prioritize local deletion before server deletion and introduce an undo option for deleted memories. This enhances user experience by providing immediate feedback and a chance to recover from accidental deletions. 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 (4)
- app/lib/pages/memories/page.dart (1)
- app/lib/pages/memories/widgets/memory_list_item.dart (2)
- app/lib/pages/onboarding/memory_created_widget.dart (1)
- app/lib/providers/memory_provider.dart (1)
Review comments generated (5)
- Review: 3
- 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.
| ///////////////////////////////////////////////////////////////// | ||
| ////////// Delete Memory With Undo Functionality /////////////// | ||
|
|
||
| Map<String, ServerMemory> memoriesToDelete = {}; | ||
|
|
||
| void deleteMemoryLocally(ServerMemory memory, int index) { | ||
| memoriesToDelete[memory.id] = memory; | ||
| memories.removeWhere((element) => element.id == memory.id); | ||
| filterMemories(''); | ||
| notifyListeners(); | ||
| } | ||
|
|
||
| void deleteMemoryOnServer(String memoryId) { | ||
| deleteMemoryServer(memoryId); | ||
| memoriesToDelete.remove(memoryId); | ||
| } | ||
|
|
||
| void undoDeleteMemory(String memoryId, int index) { | ||
| if (memoriesToDelete.containsKey(memoryId)) { | ||
| ServerMemory memory = memoriesToDelete.remove(memoryId)!; | ||
| memories.insert(index, memory); | ||
| filterMemories(''); | ||
| } | ||
| notifyListeners(); | ||
| } | ||
| ///////////////////////////////////////////////////////////////// | ||
|
|
There was a problem hiding this comment.
The new implementation of the delete functionality with undo option seems to be logically sound. However, there is a potential issue with race conditions. If deleteMemoryOnServer is called before undoDeleteMemory, and the server deletion operation takes longer than expected, the user might be able to undo the deletion while the memory is still being deleted on the server. This could lead to inconsistencies between the local and server data.
To mitigate this, you could introduce a flag that indicates whether a server deletion operation is in progress for a particular memory. You would then check this flag in undoDeleteMemory and prevent the undo operation if the deletion is still ongoing.
Here's a rough example of how you could implement this:
Map<String, ServerMemory> memoriesToDelete = {};
+ Map<String, bool> isDeletingOnServer = {};
void deleteMemoryLocally(ServerMemory memory, int index) {
memoriesToDelete[memory.id] = memory;
+ isDeletingOnServer[memory.id] = false;
// ...
}
void deleteMemoryOnServer(String memoryId) {
+ isDeletingOnServer[memoryId] = true;
deleteMemoryServer(memoryId);
// ...
}
void undoDeleteMemory(String memoryId, int index) {
+ if (isDeletingOnServer[memoryId]) {
+ // Show some error message to the user
+ return;
+ }
// ...
}Please note that you also need to handle the case where the server deletion fails. In such a case, you should reset the isDeletingOnServer flag for the corresponding memory.
| final bool isFromOnboarding; | ||
| final int memoryIdx; | ||
| final ServerMemory memory; | ||
| final Function(ServerMemory, int) updateMemory; | ||
| final Function(ServerMemory, int) deleteMemory; | ||
|
|
||
| const MemoryListItem({ | ||
| super.key, | ||
| required this.memory, | ||
| required this.updateMemory, | ||
| required this.memoryIdx, | ||
| required this.deleteMemory, | ||
| this.isFromOnboarding = false, | ||
| }); |
There was a problem hiding this comment.
The updateMemory and deleteMemory functions have been removed from the MemoryListItem constructor. This change seems to be in line with the refactor towards using a Consumer widget for handling memory deletion, which should improve modularity and maintainability by delegating the responsibility of memory management to the MemoryProvider. However, it's important to ensure that all instances of MemoryListItem are updated accordingly to avoid any runtime errors due to missing arguments.
- final Function(ServerMemory, int) updateMemory;
- final Function(ServerMemory, int) deleteMemory;
const MemoryListItem({
super.key,
required this.memory,
- required this.updateMemory,
required this.memoryIdx,
- required this.deleteMemory,
this.isFromOnboarding = false,
});| 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), | ||
| ), | ||
| child: ClipRRect( | ||
| borderRadius: BorderRadius.circular(16.0), | ||
| child: Dismissible( | ||
| key: Key(widget.memory.id), | ||
| direction: DismissDirection.endToStart, | ||
| background: Container( | ||
| alignment: Alignment.centerRight, | ||
| padding: const EdgeInsets.only(right: 20.0), | ||
| color: Colors.red, | ||
| child: const Icon(Icons.delete, color: Colors.white), | ||
| ), | ||
| onDismissed: (direction) { | ||
| var memory = widget.memory; | ||
| var memoryIdx = widget.memoryIdx; | ||
| provider.deleteMemoryLocally(memory, memoryIdx); | ||
| ScaffoldMessenger.of(context) | ||
| .showSnackBar( | ||
| SnackBar( | ||
| content: const Text('Memory deleted successfully'), | ||
| padding: const EdgeInsets.symmetric(horizontal: 16, vertical: 16), | ||
| action: SnackBarAction( | ||
| label: 'Undo', | ||
| textColor: Colors.white, | ||
| onPressed: () { | ||
| provider.undoDeleteMemory(memory.id, memoryIdx); | ||
| }, | ||
| ), | ||
| widget.memory.discarded | ||
| ? Text( | ||
| widget.memory.getTranscript(maxCount: 100), | ||
| style: Theme.of(context) | ||
| .textTheme | ||
| .bodyMedium! | ||
| .copyWith(color: Colors.grey.shade300, height: 1.3), | ||
| ) | ||
| : const SizedBox(height: 8), | ||
| ], | ||
| ), | ||
| ) | ||
| .closed | ||
| .then((reason) { | ||
| if (reason != SnackBarClosedReason.action) { | ||
| if (provider.memoriesToDelete.containsKey(memory.id)) { | ||
| provider.deleteMemoryOnServer(memory.id); | ||
| } | ||
| } | ||
| }); | ||
| }, | ||
| child: Padding( | ||
| padding: const EdgeInsetsDirectional.all(16), | ||
| child: Column( | ||
| mainAxisSize: MainAxisSize.max, | ||
| crossAxisAlignment: CrossAxisAlignment.start, | ||
| children: [ | ||
| _getMemoryHeader(), | ||
| const SizedBox(height: 16), | ||
| widget.memory.discarded | ||
| ? const SizedBox.shrink() | ||
| : Text( | ||
| structured.title, | ||
| style: Theme.of(context).textTheme.titleLarge, | ||
| maxLines: 1, | ||
| ), | ||
| widget.memory.discarded ? const SizedBox.shrink() : const SizedBox(height: 8), | ||
| widget.memory.discarded | ||
| ? const SizedBox.shrink() | ||
| : Text( | ||
| structured.overview, | ||
| style: Theme.of(context) | ||
| .textTheme | ||
| .bodyMedium! | ||
| .copyWith(color: Colors.grey.shade300, height: 1.3), | ||
| maxLines: 2, | ||
| ), | ||
| widget.memory.discarded | ||
| ? Text( | ||
| widget.memory.getTranscript(maxCount: 100), | ||
| style: Theme.of(context) | ||
| .textTheme | ||
| .bodyMedium! | ||
| .copyWith(color: Colors.grey.shade300, height: 1.3), | ||
| ) | ||
| : const SizedBox(height: 8), | ||
| ], | ||
| ), | ||
| ), | ||
| ), | ||
| ), | ||
| ), | ||
| ), | ||
| ), | ||
| ); | ||
| }), |
There was a problem hiding this comment.
The new code introduces an undo option for memory deletion via a SnackBarAction, which is a good UX improvement. The deletion process has been refactored to prioritize local deletion before server deletion. This could potentially improve performance by reducing immediate server calls, but it also introduces the risk of data inconsistency if the app crashes or is closed before the server deletion can occur. It would be beneficial to implement some form of persistent queue or job system to ensure server deletions are eventually carried out even if the app is closed.
Also, there's no error handling when calling provider.deleteMemoryOnServer(memory.id). If the server deletion fails (due to network issues, server errors, etc.), the user won't be notified and the local and server data will be inconsistent. Consider adding error handling here to improve robustness.
+ .closed
+ .then((reason) {
+ if (reason != SnackBarClosedReason.action) {
+ if (provider.memoriesToDelete.containsKey(memory.id)) {
+ provider.deleteMemoryOnServer(memory.id);
+ }
+ }
+ });
screen-20240916-222013.mp4
Summary by Entelligence.AI