-
Notifications
You must be signed in to change notification settings - Fork 789
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
Add collapsible for workflow run tasks table #1219
Conversation
…src/' <!-- ELLIPSIS_HIDDEN --> > [!IMPORTANT] > Add collapsible functionality to workflow run tasks table using Radix UI components, updating UI components and API types. > > - **UI Components**: > - Add `Collapsible`, `CollapsibleTrigger`, and `CollapsibleContent` components in `collapsible.tsx` using `@radix-ui/react-collapsible`. > - Create `WorkflowBlockCollapsibleContent` in `WorkflowBlockCollapsibleContent.tsx` to display task details with collapsible sections. > - **Workflow Run Page**: > - Update `WorkflowRun.tsx` to use `WorkflowBlockCollapsibleContent` for displaying tasks. > - Modify task table headers and structure to accommodate collapsible functionality. > - **API Types**: > - Add `failure_reason` to `WorkflowRunStatusApiResponse` in `types.ts`. > > <sup>This description was created by </sup>[<img alt="Ellipsis" src="https://img.shields.io/badge/Ellipsis-blue?color=175173">](https://www.ellipsis.dev?ref=Skyvern-AI%2Fskyvern-cloud&utm_source=github&utm_medium=referral)<sup> for de7eb79572cfb72360ec18da122036b50a4c69b8. It will automatically update as commits are pushed.</sup> <!-- ELLIPSIS_HIDDEN -->
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.
❌ Changes requested. Incremental review on 64b32c3 in 56 seconds
More details
- Looked at
455
lines of code in5
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. skyvern-frontend/src/components/ui/collapsible.tsx:1
- Draft comment:
Consider adding a comment at the top of the file to explain the purpose and usage of theCollapsible
component using Radix UI. - Reason this comment was not posted:
Confidence changes required:30%
TheCollapsible
component is correctly implemented using Radix UI components. However, the file could benefit from a brief comment explaining its purpose and usage.
2. skyvern-frontend/src/routes/workflows/WorkflowRun.tsx:551
- Draft comment:
Thekey
prop onWorkflowBlockCollapsibleContent
is redundant since it is already used on the parent component. Consider removing it. - Reason this comment was not posted:
Confidence changes required:50%
TheWorkflowBlockCollapsibleContent
component is used to display task details in a collapsible format. However, thekey
prop is not needed on theCollapsible
component since it is already being used on theWorkflowBlockCollapsibleContent
component. This is redundant and can be removed.
3. skyvern-frontend/src/routes/workflows/editor/workflowEditorUtils.ts:558
- Draft comment:
Thetitle
field is added ingetWorkflowBlock
but not used inconvertBlocks
. Ensure consistency in handling thetitle
field across functions. - Reason this comment was not posted:
Confidence changes required:50%
Thetitle
field is added to thegetWorkflowBlock
function but is not used in theconvertBlocks
function. This inconsistency might lead to issues if the title is expected in the YAML conversion.
Workflow ID: wflow_HUxuDlMPKM6uSQL6
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
) : null; | ||
|
||
return ( | ||
<Collapsible key={task.task_id} asChild open={open} onOpenChange={setOpen}> |
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.
The asChild
prop in Collapsible
is used incorrectly. It should not be used with a fragment. Consider removing asChild
or wrapping the fragment in a valid component.
<Collapsible key={task.task_id} asChild open={open} onOpenChange={setOpen}> | |
<Collapsible key={task.task_id} open={open} onOpenChange={setOpen}> |
@@ -277,6 +277,7 @@ export type WorkflowRunStatusApiResponse = { | |||
screenshot_urls: Array<string> | null; | |||
recording_url: string | null; | |||
outputs: Record<string, unknown> | null; | |||
failure_reason: string | null; |
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.
The failure_reason
field is added to WorkflowRunStatusApiResponse
but is not utilized in the UI. Consider displaying it in the WorkflowRun
component to provide more context on workflow failures.
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.
👍 Looks good to me! Reviewed everything up to 64b32c3 in 1 minute and 7 seconds
More details
- Looked at
455
lines of code in5
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. skyvern-frontend/src/routes/workflows/WorkflowBlockCollapsibleContent.tsx:53
- Draft comment:
TheasChild
prop is not valid for theCollapsible
component. Please remove it. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment directly addresses a change in the diff, specifically the use of the 'asChild' prop with the 'Collapsible' component. However, without additional context on the 'Collapsible' component's implementation, it's difficult to verify the validity of the comment. The comment could be correct if 'asChild' is indeed not a valid prop for 'Collapsible', but this requires confirmation from the component's definition.
I might be missing the actual implementation details of the 'Collapsible' component, which could confirm or refute the validity of the 'asChild' prop. Without this information, it's speculative to determine the correctness of the comment.
The comment is about a specific change in the diff, and if 'asChild' is not a valid prop, it would require a code change. However, without the component's implementation details, it's challenging to confirm this.
The comment should be kept if 'asChild' is indeed not a valid prop for 'Collapsible'. However, without confirmation from the component's implementation, it's speculative. More context is needed to make a definitive decision.
Workflow ID: wflow_KX4FKLv7OHVpNIDp
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and 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.
👍 Looks good to me! Incremental review on c8e2024 in 10 seconds
More details
- Looked at
12
lines of code in1
files - Skipped
1
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. skyvern-frontend/package.json:27
- Draft comment:
Added@radix-ui/react-collapsible
dependency correctly for collapsible functionality. - Reason this comment was not posted:
Confidence changes required:0%
The addition of the new dependency is correct and aligns with the PR description.
Workflow ID: wflow_awTef5E9RbvI2HEu
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Important
Add collapsible functionality to workflow run tasks table using Radix UI components and update API types.
Collapsible
,CollapsibleTrigger
, andCollapsibleContent
components incollapsible.tsx
using@radix-ui/react-collapsible
.WorkflowBlockCollapsibleContent
inWorkflowBlockCollapsibleContent.tsx
to display task details with collapsible sections.WorkflowRun.tsx
to useWorkflowBlockCollapsibleContent
for displaying tasks.failure_reason
toWorkflowRunStatusApiResponse
intypes.ts
.This description was created by for c8e2024. It will automatically update as commits are pushed.