Skip to content
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 new diagnostic tabs #538

Merged
merged 1 commit into from
Jul 2, 2024
Merged

Add new diagnostic tabs #538

merged 1 commit into from
Jul 2, 2024

Conversation

ykeremy
Copy link
Contributor

@ykeremy ykeremy commented Jul 2, 2024

🚀 This description was created by Ellipsis for commit 39ae778

Summary:

Adds new diagnostic tabs for 'HTML Element Tree' and 'LLM Request (Raw)' in StepArtifacts component, handling new artifact types and updating UI accordingly.

Key points:

  • File Modified: skyvern-frontend/src/routes/tasks/detail/StepArtifacts.tsx
  • Component Modified: StepArtifacts
  • New Tabs Added: 'HTML Element Tree' and 'LLM Request (Raw)'
  • New Artifact Types Handled: ArtifactType.VisibleElementsTree and ArtifactType.LLMRequest
  • UI Changes: Updated TabsList to include new tabs and TabsContent to display corresponding content if artifacts are present.

Generated with ❤️ by ellipsis.dev

…src/'

<!-- ELLIPSIS_HIDDEN -->

| 🚀 | This description was created by [Ellipsis](https://www.ellipsis.dev) for commit 2e6fb7a31a98c249461b08048404df3b16f63806  |
|--------|--------|

### Summary:
Adds new diagnostic tabs for 'HTML Element Tree' and 'LLM Request (Raw)' in `StepArtifacts` component, handling new artifact types and updating UI accordingly.

**Key points**:
- **File Modified**: `skyvern-frontend/src/routes/tasks/detail/StepArtifacts.tsx`
- **Component Modified**: `StepArtifacts`
- **New Tabs Added**: 'HTML Element Tree' and 'LLM Request (Raw)'
- **New Artifact Types Handled**: `ArtifactType.VisibleElementsTree` and `ArtifactType.LLMRequest`
- **UI Changes**: Updated `TabsList` to include new tabs and `TabsContent` to display corresponding content if artifacts are present.

----
Generated with ❤️ by [ellipsis.dev](https://www.ellipsis.dev)

<!-- ELLIPSIS_HIDDEN -->
@ykeremy ykeremy added the sync label Jul 2, 2024
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Reviewed everything up to 39ae778 in 47 seconds

More details
  • Looked at 60 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_0zlpgkSzvjCSQ5Uo


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.

@@ -72,14 +80,16 @@ function StepArtifacts({ id, stepProps }: Props) {

return (
<Tabs defaultValue="info" className="w-full">
<TabsList className="grid w-full h-16 grid-cols-4">
<TabsList className="grid w-full h-16 grid-cols-5">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The grid layout specified as grid-cols-5 does not match the number of TabsTrigger components (8). This will cause layout issues. Consider adjusting the grid columns to match the number of tabs or using a more responsive grid setup.

Suggested change
<TabsList className="grid w-full h-16 grid-cols-5">
<TabsList className="grid w-full h-16 grid-cols-8">

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well this is the worst suggestion ellipsis has given lol

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops - sorry about that, thanks for leaving a 👎

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 39ae778 in 55 seconds

More details
  • Looked at 59 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_WgM7MxQ953iFa6cM


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.

@@ -72,14 +80,16 @@ function StepArtifacts({ id, stepProps }: Props) {

return (
<Tabs defaultValue="info" className="w-full">
<TabsList className="grid w-full h-16 grid-cols-4">
<TabsList className="grid w-full h-16 grid-cols-5">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The grid layout for TabsList is set to 5 columns, but there are 9 tabs defined. This will cause layout issues. Consider adjusting the grid layout to accommodate all tabs properly.

Suggested change
<TabsList className="grid w-full h-16 grid-cols-5">
<TabsList className="grid w-full h-16 grid-cols-9">

@msalihaltun msalihaltun merged commit 09af0b8 into main Jul 2, 2024
2 checks passed
@msalihaltun msalihaltun deleted the salih/add-new-diagnostic-tabs branch July 2, 2024 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants