feat: Add Worker to IngredientsController #649
Conversation
|
|
Reviewer's Guide by SourceryThis PR improves the performance of the v1 Ingredients endpoint by moving the ingredient matching logic to a Worker thread and switches the runtime environment from Node.js to Bun. The sophisticated matching logic is moved from the controller to a dedicated worker file, allowing for parallel processing of ingredients categorization. The Docker image is updated to use Bun instead of Node.js for better startup times and performance. Sequence diagram for ingredient categorization using WorkersequenceDiagram
participant Controller as IngredientsV1Controller
participant Worker as IngredientWorker
Controller->>Worker: Start Worker with ingredients
Worker-->>Controller: Categorized results
Controller->>Controller: Process results
Controller->>Controller: Translate results if needed
Controller->>Controller: Send response
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @philipbrembeck - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider restoring the comment about priority ordering (not_vegan > maybe_not_vegan > vegan) in the tests as it documents important implementation behavior.
- It would be helpful to document the specific performance improvements seen with Bun vs Node.js in a comment or the PR description.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return false; | ||
| worker.on("message", resolve); | ||
| worker.on("error", reject); | ||
| worker.on("exit", (code) => { |
There was a problem hiding this comment.
issue (bug_risk): The promise is not resolved when worker exits normally with code 0
This could cause requests to hang indefinitely. Consider resolving the promise on normal exit.
| isVegan: this.isVegan, | ||
| }, | ||
| } | ||
| ); |
There was a problem hiding this comment.
suggestion (bug_risk): Array slicing for translation results is fragile and error-prone
Consider using a more robust approach, such as maintaining the category information alongside each ingredient through the translation process.
Suggested implementation:
// Prepare ingredients with their categories
const ingredientsWithCategories = [
...notVeganResult.map(item => ({ text: item, category: 'notVegan' })),
...maybeNotVeganResult.map(item => ({ text: item, category: 'maybeNotVegan' })),
...veganResult.map(item => ({ text: item, category: 'vegan' }))
];
// Get translations while preserving category information
const translatedIngredients = backTranslated.map((translation, index) => ({
text: translation,
category: ingredientsWithCategories[index].category
}));
// Organize translations by category
const translatedNotVegan = translatedIngredients
.filter(item => item.category === 'notVegan')
.map(item => item.text);
const translatedMaybeNotVegan = translatedIngredients
.filter(item => item.category === 'maybeNotVegan')
.map(item => item.text);
const translatedVegan = translatedIngredients
.filter(item => item.category === 'vegan')
.map(item => item.text);Make sure that:
- The
backTranslatedarray contains translations in the same order as the original ingredients - All ingredient arrays (notVeganResult, maybeNotVeganResult, veganResult) contain string values
- The translation service is properly handling the combined array of all ingredients
| veganResult: string[]; | ||
| unknownResult: string[]; | ||
| }> { | ||
| return new Promise((resolve, reject) => { |
There was a problem hiding this comment.
suggestion (performance): Worker thread is not explicitly terminated after use
Consider adding worker.terminate() after the message is received to clean up resources promptly.
Suggested implementation:
return new Promise((resolve, reject) => {
const worker = new Worker(
path.resolve(__dirname, "ingredient.worker.js"),
{
workerData: {
ingredients,
isNotVegan: this.isNotVegan,
isMaybeNotVegan: this.isMaybeNotVegan,
isVegan: this.isVegan,
},
}
);
worker.on('message', (result) => {
worker.terminate();
resolve(result);
});
worker.on('error', (error) => {
worker.terminate();
reject(error);
});
Since I can only see part of the code, you'll need to:
- Make sure any existing message and error handlers are merged with this change
- Keep any additional error handling or message processing logic that may exist
| maybeNotVeganResult, | ||
| veganResult, | ||
| unknownResult, | ||
| }); |
There was a problem hiding this comment.
issue (testing): Missing worker-specific test cases
The implementation has been moved to a worker, but there are no tests verifying the worker's behavior, error handling, or edge cases. Consider adding tests for worker initialization failures, worker communication errors, and proper cleanup.
| const wordBoundaryRegex = new RegExp(`\\b${normalizedIngredient}\\b`); | ||
| if (list.some((item) => wordBoundaryRegex.test(item.replace(/\s+/g, "")))) | ||
| return true; | ||
| private runWorker(ingredients: string[]): Promise<{ |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the worker thread implementation into a simpler Promise-based service class
The worker thread implementation adds unnecessary complexity for string matching operations. Consider simplifying to a Promise-based service class:
// ingredients.service.ts
export class IngredientsService {
async categorizeIngredients(ingredients: string[]) {
return {
notVeganResult: ingredients.filter(item =>
this.sophisticatedMatch(item, this.isNotVegan)),
maybeNotVeganResult: ingredients.filter(item =>
!this.sophisticatedMatch(item, this.isNotVegan) &&
this.sophisticatedMatch(item, this.isMaybeNotVegan)),
veganResult: ingredients.filter(item =>
this.sophisticatedMatch(item, this.isVegan)),
unknownResult: ingredients.filter(item =>
!this.sophisticatedMatch(item, this.isNotVegan) &&
!this.sophisticatedMatch(item, this.isMaybeNotVegan) &&
!this.sophisticatedMatch(item, this.isVegan))
};
}
}
// controller
const results = await this.ingredientsService.categorizeIngredients(response);This achieves non-blocking behavior while eliminating worker thread complexity. The translation section can also be simplified by using destructuring:
const translatedResults = backTranslated.reduce((acc, _, i) => {
if (i < notVeganResult.length) acc.notVegan.push(backTranslated[i]);
else if (i < notVeganResult.length + maybeNotVeganResult.length)
acc.maybeNotVegan.push(backTranslated[i]);
// ... etc
return acc;
}, { notVegan: [], maybeNotVegan: [], vegan: [], unknown: [] });
The v1 Ingredients Endpoint receives a worker in this PR for improved performance.
In the same go, I opted to use Bun at Runtime in the Dockerfile, for mainly faster start up times but also better overall performance.
Summary by Sourcery
Add a worker thread to the IngredientsV1Controller for asynchronous ingredient categorization and switch to Bun in the Dockerfile for improved performance.
New Features:
Enhancements: