-
Notifications
You must be signed in to change notification settings - Fork 532
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 html elements tree in prompt #731
Conversation
…src/' <!-- ELLIPSIS_HIDDEN --> | 🚀 | This description was created by [Ellipsis](https://www.ellipsis.dev) for commit e3ddc5bda7d296d2e5e8b9eb02f3da105be0e2e1 | |--------|--------| ### Summary: This PR introduces a new artifact type `VisibleElementsTreeInPrompt` and a corresponding `HTMLArtifact` component to fetch and display formatted HTML data in the `StepArtifacts` component. **Key points**: - Added `VisibleElementsTreeInPrompt` to `ArtifactType` in `skyvern-frontend/src/api/types.ts`. - Created `HTMLArtifact` component in `skyvern-frontend/src/routes/tasks/detail/HTMLArtifact.tsx` to fetch and format HTML data. - Updated `StepArtifacts` in `skyvern-frontend/src/routes/tasks/detail/StepArtifacts.tsx` to use `HTMLArtifact` for `VisibleElementsTreeInPrompt` artifacts. ---- Generated with ❤️ by [ellipsis.dev](https://www.ellipsis.dev) <!-- 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 4b82233 in 21 seconds
More details
- Looked at
122
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. skyvern-frontend/src/routes/tasks/detail/HTMLArtifact.tsx:49
- Draft comment:
ThequeryFn
does not handle cases whereartifact.uri
is neither afile://
nor ans3://
URL. Consider adding error handling for unsupported URI schemes. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is relevant to the changes made in the diff, as it addresses a potential issue in the newly addedqueryFn
function. The comment is actionable, suggesting the addition of error handling for unsupported URI schemes, which is a clear and useful suggestion for improving the code's robustness.
The comment could be seen as speculative since it assumes that other URI schemes might be used, but it does not confirm if this is a requirement or a likely scenario.
Even if speculative, the comment highlights a potential gap in the code's error handling, which is a valid concern for ensuring robustness.
The comment should be kept as it provides a useful suggestion for improving error handling in the new code.
Workflow ID: wflow_t0r7EjDcT1YK8UGc
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.
|
||
result += indent + "<" + element + ">\r\n"; | ||
|
||
if (element.match(/^<?\w[^>]*[^/]$/) && !element.startsWith("input")) { |
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 format
function does not correctly handle self-closing tags other than input
. Consider updating the logic to handle all self-closing tags properly.
@@ -164,8 +164,8 @@ function StepArtifacts({ id, stepProps }: Props) { | |||
)} | |||
</TabsContent> | |||
<TabsContent value="element_tree_trimmed"> |
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 tab labeled 'Element Tree' is misleading as it now displays 'VisibleElementsTreeInPrompt'. Consider renaming the tab to accurately reflect its content.
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. Reviewed everything up to 4b82233 in 21 seconds
More details
- Looked at
123
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_4gzeOtBhQTV5HiZt
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.
} | ||
}); | ||
|
||
return result.substring(1, result.length - 3); |
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 format
function assumes the result string will always have a length greater than 3, which might not be the case. Consider adding a check to ensure the string length is sufficient before using substring
.
if (artifact.uri.startsWith("s3://") && artifact.signed_url) { | ||
return axios.get(artifact.signed_url).then((response) => response.data); | ||
} | ||
}, |
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 useQuery
function does not handle cases where the artifact URI does not start with 'file://' or 's3://'. Consider adding an error or default case to handle unexpected URI formats.
Summary:
This PR adds a new artifact type
VisibleElementsTreeInPrompt
and integrates it into the frontend with a newHTMLArtifact
component for displaying formatted HTML data.Key points:
VisibleElementsTreeInPrompt
toArtifactType
inskyvern-frontend/src/api/types.ts
.HTMLArtifact
component inskyvern-frontend/src/routes/tasks/detail/HTMLArtifact.tsx
to fetch and format HTML data.StepArtifacts
inskyvern-frontend/src/routes/tasks/detail/StepArtifacts.tsx
to useHTMLArtifact
forVisibleElementsTreeInPrompt
artifacts.Generated with ❤️ by ellipsis.dev