Conversation
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Code Review
This pull request introduces a new stream condition for model associations, allowing users to filter associations based on whether a request is streaming or not. The changes span the frontend association dialog, including new validation logic and localized strings, and the backend validation and evaluation engines. Feedback focuses on improving translation consistency for the new field, handling operator aliases in the UI to prevent data loss, and ensuring the backend validation logic includes all supported operators for consistency.
| ], | ||
| }, | ||
| { | ||
| value: 'stream', |
There was a problem hiding this comment.
The stream field label is hardcoded as 'Stream' in the whenFilterFields definition. However, the translation logic in AssociationRow (around line 1260) currently only explicitly handles the prompt_tokens field. This will prevent the stream label from being translated in other languages (e.g., Chinese). Consider refactoring the logic in AssociationRow to use a generic translation key based on the field value.
| } | ||
|
|
||
| if (!promptTokensOperators.includes(condition.operator as PromptTokensOperator)) { | ||
| if (!isValidConditionOperator(condition.field, condition.operator)) { |
There was a problem hiding this comment.
The isValidConditionOperator check is strict and only allows operators defined in whenFilterFields (like eq, ne). However, the backend supports various aliases (e.g., ==, !=, <). If a condition is saved with one of these aliases via the API, it will be filtered out by normalizeWhenCondition and disappear from the UI. Consider mapping these aliases to their canonical names before validation.
|
|
||
| func validateStreamLeaf(condition objects.Condition) error { | ||
| switch condition.Operator { | ||
| case "eq", "ne", "=", "==", "!=": |
There was a problem hiding this comment.
The validateStreamLeaf function is missing the <> operator, which is supported by the condition evaluator in internal/objects/condition.go. Adding it ensures consistency between validation and evaluation logic.
| case "eq", "ne", "=", "==", "!=": | |
| case "eq", "ne", "=", "==", "!=", "<>": |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Uh oh!
There was an error while loading. Please reload this page.