Created solution Filter Inferred Intent Confidence Scores #1409
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughAdds runtime filtering in NlpSampleController.message to return only user-defined entities and validated trait values, introducing NlpValueService dependency. Updates constructor accordingly. Adds tests that mock predictions to verify inferred intents are excluded and user-defined entities/values are preserved. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant Cn as NlpSampleController
participant H as NlpHelper
participant M as NlpMappingStore
participant V as NlpValueService
C->>Cn: POST /nlp/sample/message (text)
Cn->>H: predict(text)
H-->>Cn: entities (intent, traits, others)
Cn->>M: fetch user-defined NLP mappings
M-->>Cn: mappings (intents, traits, values)
rect rgba(200,230,255,0.3)
note right of Cn: Filter phase
loop for each entity
alt trait-like entity
Cn->>V: get allowed values for trait
V-->>Cn: allowed values
Cn-->>Cn: keep only if value in allowed set
else other entity (e.g., intent)
Cn-->>Cn: keep only if entity/value defined by user
end
end
end
Cn-->>C: filtered entities (non-null only)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
api/src/nlp/controllers/nlp-sample.controller.spec.ts (2)
458-518: Consider cleanup after test data creation.The test creates a new NLP value (
greeting) in the database. If this data persists across tests, it could cause test pollution or flakiness. Consider:
- Moving this setup to a dedicated
beforeEachorbeforeAllhook with corresponding cleanup- Or ensuring the test suite cleans up created entities in
afterEachExample cleanup pattern:
+ let createdValueId: string; + it('should filter out intent values that are not user-defined', async () => { const intentEntity = await nlpEntityService.findOne({ name: 'intent' }); - await nlpValueService.create({ + const createdValue = await nlpValueService.create({ entity: intentEntity!.id, value: 'greeting', expressions: [], }); + createdValueId = createdValue.id; // ... rest of test }); + + afterEach(async () => { + if (createdValueId) { + await nlpValueService.deleteOne(createdValueId); + createdValueId = null; + } + });
520-545: Strengthen test assertions with content verification.The test only verifies
result.entities.length === 2but doesn't validate the actual entity content. This makes the test less robust and harder to debug if it fails.Apply this diff to add content assertions:
const result = await nlpSampleController.message('Hello'); expect(result.entities).toHaveLength(2); + expect(result.entities).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + entity: 'intent', + value: 'greeting', + }), + expect.objectContaining({ + entity: 'language', + value: 'en', + }), + ]), + ); });api/src/nlp/controllers/nlp-sample.controller.ts (2)
256-258: Return type could be strengthened with explicit typing.The return statement filters out
nullvalues but TypeScript may not infer the narrowed type correctly. Consider adding an explicit type assertion or type guard for clarity.return { - entities: filteredEntities.filter((e) => e !== null), + entities: filteredEntities.filter((e): e is NonNullable<typeof e> => e !== null), };This ensures the returned
entitiesarray has the correct non-nullable type.
216-259: Consider documenting the filtering behavior.The filtering logic has several implicit behaviors that might not be obvious to future maintainers:
- Entities not in
nlpMapare silently excluded- Trait entities with invalid values are silently excluded
- Non-trait entities pass through if they exist in
nlpMapConsider adding inline comments to clarify the filtering rules:
const filteredEntities = await Promise.all( prediction.entities.map(async (entity) => { const nlpEntity = nlpMap.get(entity.entity); + // Filter out entities that don't exist in user's configuration if (!nlpEntity) { return null; } + // For trait entities (e.g., intent), validate that the value is user-defined if (nlpEntity.lookups?.includes('trait')) { const entityValues = await this.nlpValueService.find({ entity: nlpEntity.id, }); const valueExists = entityValues.some( (v) => v.value === entity.value, ); + // Filter out trait values that user hasn't defined if (!valueExists) { return null; } } + // Keep entities that exist in config and have valid values return entity; }), );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/src/nlp/controllers/nlp-sample.controller.spec.ts(1 hunks)api/src/nlp/controllers/nlp-sample.controller.ts(4 hunks)
Comment on lines
+224
to
+253
| const filteredEntities = await Promise.all( | ||
| prediction.entities.map(async (entity) => { | ||
| const nlpEntity = nlpMap.get(entity.entity); | ||
|
|
||
| // If entity doesn't exist in user's configuration, exclude it | ||
| if (!nlpEntity) { | ||
| return null; | ||
| } | ||
|
|
||
| // For trait entities (like intent), check if the value exists | ||
| if (nlpEntity.lookups?.includes('trait')) { | ||
| // Get all values for this entity | ||
| const entityValues = await this.nlpValueService.find({ | ||
| entity: nlpEntity.id, | ||
| }); | ||
|
|
||
| // Check if the predicted value exists in user-defined values | ||
| const valueExists = entityValues.some( | ||
| (v) => v.value === entity.value, | ||
| ); | ||
|
|
||
| // If value doesn't exist, exclude this entity | ||
| if (!valueExists) { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| return entity; | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Potential N+1 query performance issue in entity filtering.
The filtering logic calls this.nlpValueService.find({ entity: nlpEntity.id }) inside a Promise.all(prediction.entities.map(...)), which means for each trait entity in the prediction, a separate database query is executed. If predictions contain many trait entities, this could cause performance degradation.
Consider optimizing by:
- Pre-fetching all entity values in a single query before the loop
- Building a Map<entityId, Set> for O(1) lookups
Apply this optimization:
const nlpMap = await this.nlpEntityService.getNlpMap();
+
+ // Pre-fetch all values for trait entities in one query
+ const traitEntityIds = Array.from(nlpMap.values())
+ .filter((entity) => entity.lookups?.includes('trait'))
+ .map((entity) => entity.id);
+
+ const allEntityValues = traitEntityIds.length > 0
+ ? await this.nlpValueService.find({
+ entity: { $in: traitEntityIds },
+ })
+ : [];
+
+ // Build a lookup map: entityId -> Set of valid values
+ const entityValueMap = new Map<string, Set<string>>();
+ for (const value of allEntityValues) {
+ if (!entityValueMap.has(value.entity)) {
+ entityValueMap.set(value.entity, new Set());
+ }
+ entityValueMap.get(value.entity)!.add(value.value);
+ }
const filteredEntities = await Promise.all(
prediction.entities.map(async (entity) => {
const nlpEntity = nlpMap.get(entity.entity);
if (!nlpEntity) {
return null;
}
if (nlpEntity.lookups?.includes('trait')) {
- const entityValues = await this.nlpValueService.find({
- entity: nlpEntity.id,
- });
-
- const valueExists = entityValues.some(
- (v) => v.value === entity.value,
- );
+ const validValues = entityValueMap.get(nlpEntity.id);
+ const valueExists = validValues?.has(entity.value) ?? false;
if (!valueExists) {
return null;
}
}
return entity;
}),
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const filteredEntities = await Promise.all( | |
| prediction.entities.map(async (entity) => { | |
| const nlpEntity = nlpMap.get(entity.entity); | |
| // If entity doesn't exist in user's configuration, exclude it | |
| if (!nlpEntity) { | |
| return null; | |
| } | |
| // For trait entities (like intent), check if the value exists | |
| if (nlpEntity.lookups?.includes('trait')) { | |
| // Get all values for this entity | |
| const entityValues = await this.nlpValueService.find({ | |
| entity: nlpEntity.id, | |
| }); | |
| // Check if the predicted value exists in user-defined values | |
| const valueExists = entityValues.some( | |
| (v) => v.value === entity.value, | |
| ); | |
| // If value doesn't exist, exclude this entity | |
| if (!valueExists) { | |
| return null; | |
| } | |
| } | |
| return entity; | |
| }), | |
| ); | |
| const nlpMap = await this.nlpEntityService.getNlpMap(); | |
| // Pre-fetch all values for trait entities in one query | |
| const traitEntityIds = Array.from(nlpMap.values()) | |
| .filter((entity) => entity.lookups?.includes('trait')) | |
| .map((entity) => entity.id); | |
| const allEntityValues = traitEntityIds.length > 0 | |
| ? await this.nlpValueService.find({ | |
| entity: { $in: traitEntityIds }, | |
| }) | |
| : []; | |
| // Build a lookup map: entityId -> Set of valid values | |
| const entityValueMap = new Map<string, Set<string>>(); | |
| for (const value of allEntityValues) { | |
| if (!entityValueMap.has(value.entity)) { | |
| entityValueMap.set(value.entity, new Set()); | |
| } | |
| entityValueMap.get(value.entity)!.add(value.value); | |
| } | |
| const filteredEntities = await Promise.all( | |
| prediction.entities.map(async (entity) => { | |
| const nlpEntity = nlpMap.get(entity.entity); | |
| // If entity doesn't exist in user's configuration, exclude it | |
| if (!nlpEntity) { | |
| return null; | |
| } | |
| // For trait entities (like intent), check if the value exists | |
| if (nlpEntity.lookups?.includes('trait')) { | |
| const validValues = entityValueMap.get(nlpEntity.id); | |
| const valueExists = validValues?.has(entity.value) ?? false; | |
| // If value doesn't exist, exclude this entity | |
| if (!valueExists) { | |
| return null; | |
| } | |
| } | |
| return entity; | |
| }), | |
| ); |
🤖 Prompt for AI Agents
In api/src/nlp/controllers/nlp-sample.controller.ts around lines 224 to 253, the
current loop issues a DB query per trait entity causing an N+1 problem; instead,
before the Promise.all loop collect all nlpEntity ids that have lookups
including 'trait', fetch all values for those entities in a single call (e.g.,
this.nlpValueService.find({ entity: { $in: [...] } })), build a Map<entityId,
Set<value>> for O(1) membership checks, and then inside the mapping use the
precomputed Map to decide whether to return the entity or null without any
additional DB calls.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #738
Solution: Filter Inferred Intent Confidence Scores
Problem
The frontend displayed intent confidence scores for intents that users never defined. Ludwig NLU infers intents from its training data, causing confusion when high confidence scores appear for non-existent user intents.
Example: User enters "Bonsoir" → Ludwig returns
greetings_goodeveningwith 99.68% confidence → User never created this intent → Confusion!Solution
Implemented server-side filtering in the
/nlpsample/messageendpoint to return only user-defined entities and values.What Changed
Modified:
api/src/nlp/controllers/nlp-sample.controller.tsNlpValueServicedependency injectionmessage()endpoint that:Added Tests:
api/src/nlp/controllers/nlp-sample.controller.spec.tsHow It Works
Filtering Process:
Impact
Before ❌
After ✅
Testing
Run tests:
Manual test:
greetinggreetingintent shown with confidence ✅Files Modified
api/src/nlp/controllers/nlp-sample.controller.ts(~45 lines)api/src/nlp/controllers/nlp-sample.controller.spec.ts(~90 lines)Benefits
✅ Eliminates user confusion
✅ Accurate confidence scores
✅ Backward compatible
✅ Well-tested
✅ Performance optimized (uses caching)
Summary
This fix filters out Ludwig NLU's inferred intents that don't exist in the user's configuration. Users now see confidence scores only for intents they've explicitly defined, providing an accurate representation of their chatbot's NLP capabilities.
Summary by CodeRabbit
Bug Fixes
Tests