Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes issues with categorization and caching in the trending hashtags system. The changes focus on improving cache reliability, clarifying category determination logic, and removing unnecessary fields from API responses.
Key changes:
- Added delayed execution and error handling for post.created event emissions to ensure interest data is available
- Improved category determination logic to explicitly exclude GENERAL and PERSONALIZED from mapping
- Enhanced caching by storing DB fallback results and adding debug logging
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/post/services/post.service.ts | Wraps post.created event emission in setTimeout with error handling and fetches updated post data to ensure interest_id is available |
| src/post/services/personalized-trends.service.ts | Removes score and categories fields from the return type and response object |
| src/post/services/hashtag-trends.service.ts | Caches DB fallback results, removes score field, and adds explicit category filtering with debug logging |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| interestSlug, | ||
| timestamp: post.created_at.getTime(), | ||
| }); | ||
| setTimeout(async () => { |
There was a problem hiding this comment.
Using setTimeout with a hardcoded delay of 1500ms is a fragile solution that creates a race condition. If the database update takes longer than 1.5 seconds, the interest_id may still be null. Consider using a more reliable approach such as refetching the post within the same transaction, using database triggers, or implementing a retry mechanism with exponential backoff.
| timestamp: post.created_at.getTime(), | ||
| }); | ||
| } catch (error) { | ||
| console.error('Failed to emit post.created event:', error); |
There was a problem hiding this comment.
Direct console.error usage is inconsistent with the logging pattern typically used in services. Consider using a logger instance (e.g., this.logger.error) for consistent log formatting and level management.
| if (category === TrendCategory.GENERAL || category === TrendCategory.PERSONALIZED) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The logic for excluding GENERAL and PERSONALIZED is redundant since GENERAL is explicitly added at line 415, and PERSONALIZED should not be in CATEGORY_TO_INTERESTS. This filtering suggests a potential misunderstanding of the data structure. Consider validating that CATEGORY_TO_INTERESTS doesn't contain these categories, or document why this defensive check is necessary.
| if (category === TrendCategory.GENERAL || category === TrendCategory.PERSONALIZED) { | |
| continue; | |
| } | |
| // Assumption: CATEGORY_TO_INTERESTS does not contain GENERAL or PERSONALIZED |
No description provided.