-
Notifications
You must be signed in to change notification settings - Fork 14
fix: avoid triggering llmo-customer-analysis if llmo config was updat… #1634
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
fix: avoid triggering llmo-customer-analysis if llmo config was updat… #1634
Conversation
…ed due to categorization
|
This PR will trigger a patch release when merged. |
davidaurelio
left a comment
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.
Thanks for putting in the work, the ideas here are sound.
However, there's already logic in place that you can adapt and extend, and I'd like to see that.
Please check my inline comments for details.
davidaurelio
left a comment
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.
looking pretty good.
couple of comments and questions.
lmk when you pushed prompt comparison updates so that I can make this green
src/llmo-customer-analysis/utils.js
Outdated
|
|
||
| for (const topic of topicsForNewCategories) { | ||
| const prompts = topic.prompts || []; | ||
| if (prompts.length === 0) return false; |
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.
is this actually correct? if yes, can you add a comment why?
Asking because it seems that if there are zero prompts and all other topics have only AI prompts, then it could still be only ai changes
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.
Can u be a little bit more specific? I think it is correct bc the function returns false if the config was updated by anything except for aiTopics and categories with AI origin.
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.
sure, the logic here to me reads:
topicsForNewCategoriesis all human or ai topics that have a category that is exclusively in the old or new configuration. I.e. all topics that have been added or deleted in the new config.- if any of those topics have 0 prompts, that means that the changes are not ai only.
Maybe I am misreading it. If this indeed is the logic, I am not following.
…ve AI categorization check
davidaurelio
left a comment
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.
lgtm, just the last comment
Co-authored-by: David Aurelio <[email protected]>
… categorization changes detection logic
## [1.255.8](v1.255.7...v1.255.8) (2025-11-26) ### Bug Fixes * avoid triggering llmo-customer-analysis if llmo config was updat… ([#1634](#1634)) ([52d3130](52d3130))
|
🎉 This PR is included in version 1.255.8 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
…ed due to categorization
Please ensure your pull request adheres to the following guidelines:
Related Issues
Thanks for contributing!