-
Notifications
You must be signed in to change notification settings - Fork 0
Refactor recent food module for improved structure and functionality #1438
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.
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
base: rc/v0.15.0
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR restructures the recent food module by introducing a clean architecture pattern with distinct layers, moving components to more appropriate locations, and preparing the groundwork for real-time synchronization capabilities.
Key Changes
- Introduced a service layer (
recentFoodCrudService) to separate business logic from use case orchestration - Consolidated recent food operations into a centralized
recentFoodUseCasesmodule with consistent error handling and user feedback - Refactored the realtime update system to use dependency-injected callbacks instead of directly coupling to a cache store
- Moved
RemoveFromRecentButtonfrom common components to the recent-food module for better cohesion - Cleaned up database schema by removing deprecated
user_id_oldfields
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/shared/supabase/supabase.ts |
Added TODO comment about implementing user-specific realtime subscriptions |
src/shared/supabase/database.types.ts |
Removed deprecated user_id_old fields from recent_foods table and added users_duplicate table |
src/sections/search/ui/openTemplateSearchModal.tsx |
Added template refetch on modal close to update recent foods list |
src/sections/search/components/TemplateSearchResultItem.tsx |
Updated import path for RemoveFromRecentButton after component relocation |
src/sections/search/components/TemplateSearchModal.tsx |
Simplified item tracking logic by delegating to recentFoodUseCases.touchRecentFoodForItem |
src/modules/template-search/application/usecases/templateSearchState.ts |
Updated to use recentFoodUseCases.fetchUserRecentFoodsAsTemplates and added TODO for reactive signals |
src/modules/template-search/application/tests/templateSearchLogic.test.ts |
Updated test mocks to use renamed fetchUserRecentFoodsAsTemplates method |
src/modules/template-search/application/templateSearchLogic.ts |
Renamed dependency from fetchUserRecentFoods to fetchUserRecentFoodsAsTemplates for clarity |
src/modules/recent-food/ui/tests/RemoveFromRecentButton.test.tsx |
Removed redundant API integration and toast promise tests, focusing on business logic tests |
src/modules/recent-food/ui/RemoveFromRecentButton.tsx |
Simplified to use recentFoodUseCases and removed empty catch block |
src/modules/recent-food/infrastructure/supabase/supabaseRecentFoodGateway.ts |
Added type export for RecentFoodGateway to support dependency injection |
src/modules/recent-food/infrastructure/supabase/realtime.ts |
Refactored to accept callback functions as parameters instead of directly coupling to cache store |
src/modules/recent-food/infrastructure/signals/recentFoodCacheStore.ts |
Deleted entire file as part of cache refactoring |
src/modules/recent-food/infrastructure/recentFoodRepository.ts |
Converted to factory function accepting gateway as parameter; improved error log messages with [NON-FATAL] prefix |
src/modules/recent-food/domain/recentFood.ts |
Removed comment-only line (cleanup) |
src/modules/recent-food/application/usecases/recentFoodUseCases.ts |
New consolidated use cases module with consistent error handling, toast notifications, and touchRecentFoodForItem orchestration |
src/modules/recent-food/application/usecases/recentFoodCrud.ts |
Deleted entire file, functionality moved to recentFoodUseCases.ts |
src/modules/recent-food/application/services/recentFoodCrudService.ts |
New service layer that wraps repository methods without side effects (no toasts/errors) |
src/modules/recent-food/application/usecases/recentFoodUseCases.ts
Outdated
Show resolved
Hide resolved
| users_duplicate: { | ||
| Row: { | ||
| birthdate: string | ||
| created_at: string | ||
| desired_weight: number | ||
| diet: string | ||
| favorite_foods: number[] | null | ||
| gender: string | ||
| id: number | ||
| macro_profile: Json | null | ||
| name: string | ||
| uuid: string | ||
| } | ||
| Insert: { | ||
| birthdate?: string | ||
| created_at?: string | ||
| desired_weight: number | ||
| diet?: string | ||
| favorite_foods?: number[] | null | ||
| gender?: string | ||
| id?: number | ||
| macro_profile?: Json | null | ||
| name: string | ||
| uuid: string | ||
| } | ||
| Update: { | ||
| birthdate?: string | ||
| created_at?: string | ||
| desired_weight?: number | ||
| diet?: string | ||
| favorite_foods?: number[] | null | ||
| gender?: string | ||
| id?: number | ||
| macro_profile?: Json | null | ||
| name?: string | ||
| uuid?: string | ||
| } | ||
| Relationships: [] | ||
| } |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The table name users_duplicate suggests this is a temporary or test table. Consider using a more descriptive name that indicates its purpose (e.g., users_migration, users_backup, or users_legacy), or remove it if it's no longer needed.
| users_duplicate: { | |
| Row: { | |
| birthdate: string | |
| created_at: string | |
| desired_weight: number | |
| diet: string | |
| favorite_foods: number[] | null | |
| gender: string | |
| id: number | |
| macro_profile: Json | null | |
| name: string | |
| uuid: string | |
| } | |
| Insert: { | |
| birthdate?: string | |
| created_at?: string | |
| desired_weight: number | |
| diet?: string | |
| favorite_foods?: number[] | null | |
| gender?: string | |
| id?: number | |
| macro_profile?: Json | null | |
| name: string | |
| uuid: string | |
| } | |
| Update: { | |
| birthdate?: string | |
| created_at?: string | |
| desired_weight?: number | |
| diet?: string | |
| favorite_foods?: number[] | null | |
| gender?: string | |
| id?: number | |
| macro_profile?: Json | null | |
| name?: string | |
| uuid?: string | |
| } | |
| Relationships: [] | |
| } |
src/modules/recent-food/application/usecases/recentFoodUseCases.ts
Outdated
Show resolved
Hide resolved
src/modules/recent-food/application/usecases/recentFoodUseCases.ts
Outdated
Show resolved
Hide resolved
src/modules/recent-food/application/usecases/recentFoodUseCases.ts
Outdated
Show resolved
Hide resolved
| case 'INSERT': { | ||
| if (event.new !== undefined) { | ||
| recentFoodCacheStore.upsertToCache(event.new) | ||
| callbacks.onInsert(event.new) | ||
| } | ||
| break |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The empty callbacks with unused parameter names (underscore prefix) suggest that the realtime functionality is not fully implemented yet (as confirmed by the TODO on line 27). Consider adding JSDoc comments to document what these callbacks should do when implemented, or adding logging statements to help with debugging during development.
Remove sections that referenced memory retrieval, memory updates, and the requirement to begin chats with "Remembering..." from the Copilot instructions file.
Clarify barrel file prohibition and prefer direct imports for tree-shaking. Require verifying referenced scripts and prefer repo-specific scripts. Add retry and escalation steps for `npm run copilot:check` instead of blocking indefinitely. Relax `reportedBy` to be required only for machine-readable/auditable outputs. Improve solo-project, JSDoc, and import guidance for clearer, actionable rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 35 out of 36 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
src/modules/diet/recent-food/infrastructure/supabase/realtime.ts:21
- The TODO comment at line 16 in
deps.tsmentions implementing a recent food cache, but the realtime callbacks are currently using no-op functions ((_: unknown) => {}). Consider adding a TODO comment here as well to make it clear that these callbacks are intentionally stubbed out pending cache implementation, or reference the TODO indeps.tsfor context.
| // TODO: Remove client-side user check after implementing row-level security (RLS) | ||
| if (currentRecentFood.user_id !== authUseCases.currentUserIdOrGuestId()) { | ||
| throw new Error('BUG: recentFood fetched does not match current user') | ||
| } | ||
|
|
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TODO comment mentions implementing Row-Level Security (RLS), but in the meantime, the client-side check at line 30 throws an error if currentRecentFood.user_id doesn't match the current user. However, this check happens after the database fetch has already occurred. This means a malicious user could potentially fetch another user's recent food data before the error is thrown. While the data won't be used, it's a minor information leak. Consider:
- Prioritizing the RLS implementation for proper server-side security
- Moving this validation earlier in the flow, or
- Adding a server-side filter in the fetch query itself to ensure only the current user's data is returned
The current implementation provides defense-in-depth against bugs but not against intentional attacks.
| // TODO: Remove client-side user check after implementing row-level security (RLS) | |
| if (currentRecentFood.user_id !== authUseCases.currentUserIdOrGuestId()) { | |
| throw new Error('BUG: recentFood fetched does not match current user') | |
| } | |
| // Ensure recentFoodCrudService.fetchRecentFoodByUserTypeAndReferenceId enforces user-level filtering at the query level. |
| * For GroupItem: returns the reference of the first trackable child (food or recipe) | ||
| * | ||
| * @param item - The item to extract reference from | ||
| * @returns The recent food reference, or null if the item cannot be tracked |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JSDoc comment describes the function as returning "The recent food reference, or null if the item cannot be tracked", but the actual return type is RecentFoodReference[] (an array that can be empty). The documentation should be updated to reflect that this function now returns an array of references and returns an empty array (not null) when nothing can be tracked:
/**
* Extracts the recent food reference (type and referenceId) from an item.
* This is used to track recently used foods/recipes.
*
* For FoodItem: returns the food reference directly
* For RecipeItem: returns the recipe reference directly
* For GroupItem: returns references for all trackable children (foods and recipes)
*
* @param item - The item to extract reference from
* @returns An array of recent food references, or an empty array if the item cannot be tracked
*/| * For GroupItem: returns the reference of the first trackable child (food or recipe) | |
| * | |
| * @param item - The item to extract reference from | |
| * @returns The recent food reference, or null if the item cannot be tracked | |
| * For GroupItem: returns references for all trackable children (foods and recipes) | |
| * | |
| * @param item - The item to extract reference from | |
| * @returns An array of recent food references, or an empty array if the item cannot be tracked |
| const [recentFoodRef] = extractRecentFoodReferenceFromItem(item) | ||
| if (recentFoodRef === undefined) { | ||
| logging.warn( | ||
| 'Cannot touch recent food for item - no trackable reference found', | ||
| { item }, | ||
| ) | ||
| showError('Não foi possível adicionar alimento aos alimentos recentes.') | ||
| return | ||
| } | ||
|
|
||
| for (const recentFoodRef of extractRecentFoodReferenceFromItem(item)) { | ||
| await touchRecentFood(recentFoodRef) | ||
| } |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first parameter is destructured but immediately checked. The logic extracts the first element and then checks if it's undefined, but subsequently iterates over all elements using the full array again. This creates redundant extraction and iteration.
Consider simplifying to:
export async function touchRecentFoodForItem(item: Item) {
const references = extractRecentFoodReferenceFromItem(item)
if (references.length === 0) {
logging.warn(
'Cannot touch recent food for item - no trackable reference found',
{ item },
)
showError('Não foi possível adicionar alimento aos alimentos recentes.')
return
}
for (const recentFoodRef of references) {
await touchRecentFood(recentFoodRef)
}
}| users_duplicate: { | ||
| Row: { | ||
| birthdate: string | ||
| created_at: string | ||
| desired_weight: number | ||
| diet: string | ||
| favorite_foods: number[] | null | ||
| gender: string | ||
| id: number | ||
| macro_profile: Json | null | ||
| name: string | ||
| uuid: string | ||
| } | ||
| Insert: { | ||
| birthdate?: string | ||
| created_at?: string | ||
| desired_weight: number | ||
| diet?: string | ||
| favorite_foods?: number[] | null | ||
| gender?: string | ||
| id?: number | ||
| macro_profile?: Json | null | ||
| name: string | ||
| uuid: string | ||
| } | ||
| Update: { | ||
| birthdate?: string | ||
| created_at?: string | ||
| desired_weight?: number | ||
| diet?: string | ||
| favorite_foods?: number[] | null | ||
| gender?: string | ||
| id?: number | ||
| macro_profile?: Json | null | ||
| name?: string | ||
| uuid?: string | ||
| } | ||
| Relationships: [] | ||
| } |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A users_duplicate table has been added to the database types, which suggests a data migration or temporary structure. This table appears to be unrelated to the "recent food" refactoring mentioned in the PR description. If this is temporary migration infrastructure, it should be documented or removed after migration completion. If it's permanent, it needs proper documentation explaining its purpose and relationship to the main users table.
…nd shell safety notes
Restructure the recent food module by moving components, simplifying data management, and enhancing real-time updates. This refactor improves code organization and reduces unnecessary exports.
Fix #1404