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

Fix wrongly ordered action screenshots #1464

Merged
merged 1 commit into from
Jan 2, 2025

Conversation

wintonzheng
Copy link
Contributor

@wintonzheng wintonzheng commented Jan 2, 2025

Important

Fixes action screenshot order and ensures unique Tabs keys in WorkflowRunTimelineItemInfoSection.tsx.

  • Behavior:
    • Fixes ordering of action screenshots in ActionScreenshot.tsx by reversing the index calculation.
    • Changes Tabs key in WorkflowRunTimelineItemInfoSection.tsx to use item.task_id if available, ensuring unique keys.
  • Misc:
    • Adds a comment in ActionScreenshot.tsx explaining the reverse order of screenshots.

This description was created by Ellipsis for 02c14b3. It will automatically update as commits are pushed.

…src/'

<!-- ELLIPSIS_HIDDEN -->

> [!IMPORTANT]
> Fixes action screenshot ordering and updates `Tabs` key in `WorkflowRunTimelineItemInfoSection.tsx`.
>
>   - **Behavior**:
>     - Fixes ordering of action screenshots in `ActionScreenshot.tsx` by reversing the index calculation.
>     - Changes `Tabs` key in `WorkflowRunTimelineItemInfoSection.tsx` to use `item.task_id` if available, ensuring unique keys.
>   - **Misc**:
>     - Adds a comment in `ActionScreenshot.tsx` explaining the reverse order of screenshots.
>
> <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 93820b19957779d13523a98e2c4e46137d770eee. It will automatically update as commits are pushed.</sup>

<!-- ELLIPSIS_HIDDEN -->
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.

👍 Looks good to me! Reviewed everything up to 02c14b3 in 13 seconds

More details
  • Looked at 28 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. skyvern-frontend/src/routes/workflows/workflowRun/WorkflowRunTimelineItemInfoSection.tsx:75
  • Draft comment:
    Ensure that the Tabs key is unique by using item.task_id ?? item.block_type consistently across similar components.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The change in the key for Tabs is correct, but it should be consistent across all similar components.

Workflow ID: wflow_bD3X7nSsxxg5voQ2


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

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.

👍 Looks good to me! Incremental review on 02c14b3 in 35 seconds

More details
  • Looked at 28 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. skyvern-frontend/src/routes/workflows/workflowRun/WorkflowRunTimelineItemInfoSection.tsx:75
  • Draft comment:
    Using item.task_id ?? item.block_type as a key might not ensure uniqueness if task_id is undefined. Consider ensuring task_id is always defined or use a more unique identifier.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The key uniqueness concern is likely not valid because: 1) Each Tabs instance is in a mutually exclusive conditional block 2) The fallback to block_type is fine since block_type is an enum 3) React keys only need to be unique among siblings 4) This is just for tab state management, not for list rendering where key uniqueness is more critical.
    I might be underestimating the importance of unique keys in React components. There could be edge cases where task_id being undefined causes issues.
    Even if task_id is undefined, block_type is an enum value that would provide sufficient uniqueness within this component's scope. The keys only need to be unique among siblings, which is guaranteed here.
    The comment should be deleted as it raises a theoretical concern that isn't actually an issue in this context, given the component structure and React's key requirements.

Workflow ID: wflow_2nB0xqHlbWIhlnzu


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@msalihaltun msalihaltun merged commit 22f72c1 into main Jan 2, 2025
2 checks passed
@msalihaltun msalihaltun deleted the salih/fix-action-screenshot-order branch January 2, 2025 13:37
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.

2 participants