-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat(TagImprovement): Improved tag dropdown and outputselect #904
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
base: main
Are you sure you want to change the base?
Conversation
@mhbdev is attempting to deploy a commit to the Sim Team on Vercel. A member of the Team first needs to authorize it. |
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.
Greptile Summary
This PR introduces conditional output filtering functionality to enable dynamic display of tool outputs based on block configuration values. The implementation adds a condition
property to the OutputFieldDefinition
type that supports field-based conditions with array values, negation (not
), and AND logic.
The core changes span three files:
-
Type Definition (
blocks/types.ts
): ExtendedOutputFieldDefinition
with an optionalcondition
property that follows the same pattern as existing input conditions in the codebase -
Output Select Component (
output-select.tsx
): Added three helper functions (getBlockCurrentValues
,evaluateOutputCondition
,filterOutputsByConditions
) that retrieve current block values and filter outputs based on conditions. The filtering is applied when processing outputs (lines 182-185) -
Tag Dropdown (
tag-dropdown.tsx
): Integrated the same conditional filtering logic throughout the tag generation process, ensuring output tags are dynamically shown/hidden based on current block state
This addresses the problem where tools like arxiv
had to use comments to separate different output types for different operations (search, get paper details, etc.). Now developers can define conditions like {field: 'operation', value: 'arxiv_search'}
to show specific outputs only when relevant, creating a cleaner and more contextual user interface.
The implementation maintains consistency with existing condition evaluation patterns used elsewhere in the codebase, particularly mirroring the logic found in SubBlockConfig
for input conditions.
Confidence score: 3/5
- This PR introduces moderate complexity with potential type safety concerns and needs careful testing
- Score reflects the addition of conditional logic that could have edge cases with undefined values and type mismatches
- Pay close attention to the condition evaluation logic and type definitions in
blocks/types.ts
3 files reviewed, 5 comments
const isAndValueMatch = | ||
!condition.and || | ||
(() => { | ||
const andFieldValue = currentValues[condition.and!.field] |
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.
logic: Non-null assertion operator used without null check. Consider adding explicit null check for condition.and
before accessing nested properties.
const isAndValueMatch = | ||
!condition.and || | ||
(() => { | ||
const andFieldValue = currentValues[condition.and!.field] |
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.
logic: Using non-null assertion operator (!
) on condition.and!.field
could throw if condition.and
is undefined despite the guard check. Consider using optional chaining: condition.and?.field
const andFieldValue = currentValues[condition.and!.field] | |
const andFieldValue = currentValues[condition.and?.field] |
not?: boolean | ||
and?: { | ||
field: string | ||
value: string | number | boolean | Array<string | number | boolean> | undefined |
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.
logic: The undefined
type for value
in the and
condition seems inconsistent with the main condition which doesn't allow undefined. This could lead to unexpected behavior.
value: string | number | boolean | Array<string | number | boolean> | undefined | |
value: string | number | boolean | Array<string | number | boolean> |
condition?: { | ||
field: string | ||
value: string | number | boolean | Array<string | number | boolean> | ||
not?: boolean | ||
and?: { | ||
field: string | ||
value: string | number | boolean | Array<string | number | boolean> | undefined | ||
not?: boolean | ||
} | ||
} |
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.
style: Consider extracting the condition type into a separate interface since it's duplicated in SubBlockConfig (lines 134-154). This would improve maintainability and ensure consistency.
Context Used: Context - When defining properties for components, use a dedicated config file (.ts) for configuration and keep rendered components in their respective component files. (link)
description?: string; | ||
condition?: { | ||
field: string | ||
value: string | number | boolean | Array<string | number | boolean> |
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.
style: The union type for value
is quite complex. Consider creating a separate type alias for ConditionValue
to improve readability.
Summary
Previously we could not filter outputs based on user inputs. From now on outputs of a tool will also support a condition field.
Example:
arxiv
Instead of using comments to separate different types of responses like this:
we can say:
Fixes #(issue)
Type of Change
Testing
It is not a breaking change but you can focus on checking the functionality in the app.
Checklist
Screenshots/Videos
Solves #905