feat: add contextVar regex validation #921
Conversation
WalkthroughThe changes introduce a new optional property, Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CS as ConversationService
participant DB as ContextVar Repository
participant L as Logger
U->>CS: Submit user input with value
CS->>DB: Retrieve context variable configuration
CS->>CS: Validate input against `pattern`
alt Value matches pattern
CS->>CS: Update profile and conversation contexts
else Value does not match pattern
CS->>L: Log warning and skip update
end
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
api/src/chat/services/conversation.service.ts (2)
119-120: Use template literal conversion with cautionConverting
contextValueto a string using template literals (${contextValue}) works for simple values but may produce unexpected results with complex objects or arrays.Consider adding a more robust type check or conversion for non-string values:
- const isValidContextValue = new RegExp(pattern.slice(1, -1)).test( - `${contextValue}`, - ); + const stringValue = typeof contextValue === 'object' ? + JSON.stringify(contextValue) : `${contextValue}`; + const isValidContextValue = new RegExp(pattern.slice(1, -1)).test(stringValue);
118-119: Add a default pattern handlingThe current code uses an empty string as the default pattern, which will match anything when converted to a RegExp. This might lead to unexpected behavior if a pattern is missing.
- const { permanent, pattern = '' } = context_var || {}; + const { permanent, pattern = '/.+/' } = context_var || {};frontend/src/components/context-vars/ContextVarForm.tsx (2)
177-181: Add informative helper text for regex patternsThe regex input would benefit from guidance on the expected format and purpose of the regex pattern.
helperText={errors.pattern ? errors.pattern.message - : null} + : t("help.regex_pattern_for_context_vars")}Consider adding a translation key with helpful information about using regex patterns for context variables.
158-160: Consider more explicit empty pattern handlingThe current code treats an empty pattern as valid, which will match any string. This might be unintended behavior.
- if (pattern.slice(1, -1) === "") { - return true; - } + if (pattern.slice(1, -1) === "") { + return t("message.empty_regex_will_match_anything"); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
api/src/chat/controllers/context-var.controller.spec.ts(1 hunks)api/src/chat/dto/context-var.dto.ts(2 hunks)api/src/chat/schemas/context-var.schema.ts(3 hunks)api/src/chat/services/context-var.service.ts(2 hunks)api/src/chat/services/conversation.service.ts(2 hunks)api/src/utils/test/fixtures/contextvar.ts(1 hunks)frontend/src/components/context-vars/ContextVarForm.tsx(5 hunks)frontend/src/components/context-vars/index.tsx(2 hunks)frontend/src/components/visual-editor/form/inputs/triggers/PatternInput.tsx(1 hunks)frontend/src/types/context-var.types.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
frontend/src/components/visual-editor/form/inputs/triggers/PatternInput.tsx (1)
frontend/src/types/block.types.ts (1)
Pattern(77-77)
api/src/chat/schemas/context-var.schema.ts (2)
api/src/utils/helpers/zod-validation.ts (1)
buildZodSchemaValidator(11-15)api/src/chat/schemas/types/pattern.ts (1)
stringRegexPatternSchema(35-51)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: API-Tests
- GitHub Check: Frontend Tests
🔇 Additional comments (16)
api/src/utils/test/fixtures/contextvar.ts (2)
21-21: Pattern format seems consistent with implementationThe addition of the
pattern: '/.+/'property to thecontentVarDefaultValuesobject aligns with the PR objectives for regex validation of context variables. The default pattern/.+/will match one or more of any character.
2-2: Copyright year updated appropriatelyThe copyright year has been updated to 2025, keeping the license information current.
api/src/chat/controllers/context-var.controller.spec.ts (1)
106-106: Test case properly includes new pattern propertyThe test case for context variable creation now includes the
patternproperty with the default value/.+/, ensuring the new functionality is properly tested.frontend/src/types/context-var.types.ts (2)
17-17: Interface properly updated with optional pattern propertyThe
IContextVarAttributesinterface has been correctly updated to include the optionalpatternproperty of type string, ensuring type safety when working with context variables in the frontend.
2-2: Copyright year updatedThe copyright year has been updated to 2025 in the license header.
api/src/chat/dto/context-var.dto.ts (3)
30-36: Well-implemented pattern property with appropriate validationThe
patternproperty has been properly added to theContextVarCreateDtoclass with appropriate decorators:
@ApiPropertyOptionalfor API documentation@IsOptional()to mark it as optional@IsString()for type validationThis implementation supports the PR objective of adding regex validation for context variables.
9-9: Import updated to include required decoratorThe import statement has been properly updated to include
ApiPropertyOptionalwhich is used for the new pattern property.
2-2: Copyright year updatedThe copyright year has been updated to 2025 in the license header.
api/src/chat/schemas/context-var.schema.ts (3)
2-2: Updated copyright year from 2024 to 2025The copyright year has been updated to reflect the current year.
13-16: Added imports for validation functionsThe imports for
buildZodSchemaValidatorandstringRegexPatternSchemahave been added to support the new validation functionality for thepatternproperty.
46-51: Added new optional 'pattern' property with validationThe implementation adds a new optional property
patternto theContextVarclass, which allows for regex pattern validation of context variables. The default value of/.+/will match any non-empty string.The property:
- Uses the Zod validation schema for regex patterns
- Has a sensible default that matches any non-empty string
- Is correctly marked as optional with the
?operatorThis addition supports the PR objective of allowing multiple contextVars within the same block without value interference.
frontend/src/components/visual-editor/form/inputs/triggers/PatternInput.tsx (1)
29-29: Exported isRegex function for reuseThe
isRegexfunction has been changed from a private constant to an exported function, allowing it to be used in other components. This change supports code reusability and consistency in regex pattern validation across the application.api/src/chat/services/context-var.service.ts (1)
2-2: Updated copyright year from 2024 to 2025The copyright year has been updated to reflect the current year.
frontend/src/components/context-vars/index.tsx (2)
12-12: Added Chip component importThe Chip component has been imported from @mui/material to support displaying patterns in the data grid.
113-121: Added pattern column to display regex in UIA new column has been added to display the regex pattern associated with each context variable. The implementation uses a Chip component to visually represent the pattern, which enhances the user interface by making patterns easily visible and distinguishable in the data grid.
The column has appropriate configuration including flex sizing, header alignment, and disabled column menu for consistency with other columns in the grid.
frontend/src/components/context-vars/ContextVarForm.tsx (1)
174-175:❓ Verification inconclusive
Ensure consistent format for all regex patterns
The
setValueAsfunction is correctly ensuring that all patterns have delimiters, but there's no validation for the format of patterns coming from the backend.Make sure all existing patterns in the database follow the format
/pattern/. Run this script to verify:
🏁 Script executed:
#!/bin/bash # Check for contextVars with invalid regex patterns # Look for collections/models where contextVars are stored echo "Searching for contextVars models..." # Search for contextVar schema and pattern field rg -A 10 "class ContextVar" --glob="*.ts" rg "pattern.*Prop" --glob="*.ts" # Search for any existing context variable creation/update in MongoDB queries rg "contextVar.*pattern" --glob="*.ts"Length of output: 5020
Regex Patterns: Validate Consistency Across Frontend and Backend
The
setValueAsfunction infrontend/src/components/context-vars/ContextVarForm.tsx(lines 174–175) correctly adds delimiters for user input. However, our initial search of the ContextVar-related files (including schemas and seed files) did not yield a definitive confirmation that all patterns coming from the backend already adhere to the/pattern/format.Action Required:
- Manually verify that all regex patterns stored in the database (e.g., in files like
api/src/chat/schemas/context-var.schema.tsandapi/src/chat/seeds/context-var.seed.ts) follow the/pattern/format.- If any non-conforming patterns are discovered, please consider introducing backend validations or corrective transformations similar to the frontend
setValueAslogic to ensure consistency.
| ) { | ||
| Logger.debug( | ||
| `Adding context var to subscriber: ${capture.context_var} = ${contextValue}`, | ||
| const context_var = contextVars[capture.context_var]; |
There was a problem hiding this comment.
Lines 98-100.
There's obsolete commented out code. Can we get rid of it ?
There was a problem hiding this comment.
Thank you for the feedback, your are right but.
Let's keep the update as short as possible to facilitate the review process.
I propose to create a dedicated issue for the recommended update.
There was a problem hiding this comment.
That's a duplicate issue. I already created it previously. See the original one
|
LGTM! |
| @@ -26,7 +26,7 @@ import { | |||
| import { OutcomeInput } from "./OutcomeInput"; | |||
| import { PostbackInput } from "./PostbackInput"; | |||
|
|
|||
| const isRegex = (str: Pattern) => { | |||
| export const isRegex = (str: Pattern) => { | |||
There was a problem hiding this comment.
We can do this instead to check for regex validation:
try {
new RegExp(str.slice(1, -1));
return true;
} catch (e) {
return false;
}
There was a problem hiding this comment.
The isRegex function is used to detect from an input the corresponding form type to show.
If we use this function the text case will be impacted directly.
Motivation:
The main motivation is to add an extra regex validation before updating contextVars (permanent, and not permanent).
This added feature will give us the possibility to use multiple contextVars in the same block without value interference.
(NB: this update suppose that the regex for all the contextVars used in the same block are unique)
Fixes #917
Screenshots:
Checklist:
Summary by CodeRabbit
New Features
Refactor