Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a 'manual' category for memories, allowing users to create memories that are not automatically categorized. The changes span both the frontend and backend, adding the new category to enums, UI components for filtering and creation, and updating the API to handle this new category.
My review has identified a critical issue in the backend logic that could lead to user-selected categories being incorrectly overridden. I've also pointed out an opportunity to refactor duplicated UI code in the Flutter app to improve maintainability. Please see the detailed comments for suggestions.
| if memory.category != MemoryCategory.manual: | ||
| # Only use the two primary categories for auto-categorization | ||
| categories = [MemoryCategory.interesting.value, MemoryCategory.system.value] | ||
| memory.category = identify_category_for_memory(memory.content, categories) |
There was a problem hiding this comment.
The current logic for auto-categorization is flawed. If a user manually creates a memory and selects the 'interesting' or 'system' category in the UI, this logic will override their choice by re-running the categorization. Since the client now sends a specific category ('manual', 'interesting', or 'system') when a user creates a memory, this auto-categorization logic is not only redundant but also introduces a bug. The category provided by the client should be trusted. I suggest removing this block.
| PopupMenuItem<FilterOption>( | ||
| value: FilterOption.manual, | ||
| child: Row( | ||
| children: [ | ||
| const Text( | ||
| 'Manual', | ||
| style: TextStyle(color: Colors.white), | ||
| ), | ||
| const Spacer(), | ||
| if (_currentFilter == FilterOption.manual) | ||
| const Icon(Icons.check, size: 16, color: Colors.white), | ||
| ], | ||
| ), | ||
| ), |
There was a problem hiding this comment.
This new PopupMenuItem for the 'Manual' filter option adds to the existing code duplication for creating these menu items. To improve maintainability and reduce redundancy, consider creating a helper method that generates these PopupMenuItem widgets. This would make the code cleaner and easier to update in the future.
For example, you could create a method like this:
PopupMenuItem<FilterOption> _buildFilterMenuItem(FilterOption option, String title) {
return PopupMenuItem<FilterOption>(
value: option,
child: Row(
children: [
Text(
title,
style: const TextStyle(color: Colors.white),
),
const Spacer(),
if (_currentFilter == option)
const Icon(Icons.check, size: 16, color: Colors.white),
],
),
);
}And then use it like this:
itemBuilder: (BuildContext context) => <PopupMenuEntry<FilterOption>>[
_buildFilterMenuItem(FilterOption.all, 'All'),
_buildFilterMenuItem(FilterOption.interesting, 'Interesting'),
_buildFilterMenuItem(FilterOption.system, 'System'),
_buildFilterMenuItem(FilterOption.manual, 'Manual'),
],
#3460