-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
9024 workflow test serverless function follow up #9066
9024 workflow test serverless function follow up #9066
Conversation
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.
PR Summary
This PR enhances workflow variable handling and UI components, focusing on tab and dropdown styling improvements.
- Added horizontal scrolling with hidden scrollbar in
TabList.tsx
to handle tab overflow gracefully - Introduced
LinkOutputSchema
type and handling infilterOutputSchema.ts
for workflow variable selection - Improved node selection in workflow diagrams by updating all nodes' selection state via
reactflow.setNodes()
- Added
EllipsisDisplay
toTab
component for better text overflow handling - Standardized tab list styling by removing custom containers and adding consistent background/padding styles
16 file(s) reviewed, 8 comment(s)
Edit PR Review Bot Settings | Greptile
...node, | ||
selected: workflowDiagramTriggerNodeSelection === node.id, | ||
})), | ||
); | ||
|
||
setWorkflowDiagramTriggerNodeSelection(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: clearing selection immediately after setting it could cause flickering or race conditions if there are other effects watching node selection
@@ -93,7 +94,7 @@ export const Tab = ({ | |||
<StyledHover> | |||
{logo && <StyledLogo src={logo} alt={`${title} logo`} />} | |||
{Icon && <Icon size={theme.icon.size.md} />} | |||
{title} | |||
<EllipsisDisplay>{title}</EllipsisDisplay> |
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: EllipsisDisplay may need a max-width prop to ensure proper truncation within the tab's available space
if (isObject(value) && !Array.isArray(value)) { | ||
acc[key] = { | ||
isLeaf: false, | ||
icon: 'IconVariable', | ||
value: getFunctionOutputSchema(value), | ||
}; | ||
} else { |
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: this recursively processes nested objects but doesn't handle arrays. Arrays are treated as leaf nodes which may not be the desired behavior for complex data structures
isLeaf: false, | ||
icon: 'IconVariable', | ||
value: getFunctionOutputSchema(value), |
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: hardcoding 'IconVariable' here may be better defined as a constant at the module level for consistency and maintainability
@@ -60,7 +62,7 @@ export const SearchVariablesDropdownWorkflowStepItems = ({ | |||
hovered={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.
style: hovered={false} is redundant since it's the default state and doesn't affect the component's behavior
@@ -1,20 +1,27 @@ | |||
import { InputSchemaPropertyType } from '@/workflow/types/InputSchema'; | |||
|
|||
export type Leaf = { | |||
type Leaf = { | |||
isLeaf: true; | |||
type?: InputSchemaPropertyType; | |||
icon?: string; | |||
label?: string; | |||
value: any; |
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: value is typed as 'any' - consider using a more specific type or adding a generic type parameter
export const WORKFLOW_SERVERLESS_FUNCTION_TAB_LIST_COMPONENT_ID = | ||
'workflow-serverless-function-tab-list-component-id'; |
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 using a more specific ID format like twenty-workflow-serverless-function-tab-list
to better align with common component ID patterns and avoid potential conflicts
type Link = { | ||
isLeaf: true; | ||
tab?: string; | ||
icon?: string; | ||
label?: string; | ||
}; |
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: Link type has isLeaf: true but no value property, unlike the other Leaf type. This inconsistency could cause type checking issues.
* | ||
* Selecting the node will cause a right drawer to open in order to edit the step. | ||
*/ | ||
setWorkflowDiagramTriggerNodeSelection(createdStep.id); |
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.
why don't we need this anymore?
...node, | ||
selected: workflowDiagramTriggerNodeSelection === node.id, | ||
})), | ||
); |
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.
interesting greptile commnet on this one. We are calling setWorkflowDiagramTriggerNodeSelection. How does that work?
@@ -51,7 +66,9 @@ export const SearchVariablesDropdownFieldItems = ({ | |||
const getDisplayedSubStepFields = () => { | |||
const currentSubStep = getCurrentSubStep(); | |||
|
|||
if (isRecordOutputSchema(currentSubStep)) { | |||
if (isLinkOutputSchema(currentSubStep)) { |
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.
👍
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.
57daaff
to
f3c6d35
Compare
Enregistrement.de.l.ecran.2024-12-13.a.18.55.21.mov