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 bad error handling in task recording #737

Merged
merged 1 commit into from
Aug 26, 2024

Conversation

ykeremy
Copy link
Contributor

@ykeremy ykeremy commented Aug 26, 2024

🚀 This description was created by Ellipsis for commit 04a1b19

Summary:

This PR improves error handling in TaskRecording by updating useQuery and getRecordingURL to handle missing recording URLs.

Key points:

  • Improve error handling in TaskRecording.
  • Update useQuery in TaskRecording.tsx to return string | null.
  • Modify getRecordingURL in artifactUtils.ts to return null if task.recording_url is missing.
  • Display "No recording available for this task" when recordingURL is null.

Generated with ❤️ by ellipsis.dev

…src/'

<!-- ELLIPSIS_HIDDEN -->

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

### Summary:
This PR improves error handling in `TaskRecording` by changing `useQuery` to return `string | null` and updating `getRecordingURL` to return `null` when no recording URL is available.

**Key points**:
- Update `useQuery` in `skyvern-frontend/src/routes/tasks/detail/TaskRecording.tsx` to return `string | null` instead of `string | undefined`.
- Modify `getRecordingURL` in `skyvern-frontend/src/routes/tasks/detail/artifactUtils.ts` to return `null` when `task.recording_url` is absent.
- Adjust error handling in `TaskRecording` to display "No recording available for this task" when `recordingURL` is `null`.

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

<!-- ELLIPSIS_HIDDEN -->
@ykeremy ykeremy added the sync label Aug 26, 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 04a1b19 in 10 seconds

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

Workflow ID: wflow_i2iVZw7z2LV5TWS3


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.

@@ -22,7 +22,7 @@ export function getScreenshotURL(task: TaskApiResponse) {

export function getRecordingURL(task: TaskApiResponse) {
if (!task.recording_url) {
return;
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider returning null instead of undefined for consistency with getRecordingURL.

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 04a1b19 in 17 seconds

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

Workflow ID: wflow_fuVRpTUQGeoledUX


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.

@@ -13,11 +14,11 @@ function TaskRecording() {
data: recordingURL,
isLoading: taskIsLoading,
isError: taskIsError,
} = useQuery<string | undefined>({
} = useQuery<string | null>({
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding retry: false to the useQuery options to prevent unnecessary retries when the recording URL is missing.

@msalihaltun msalihaltun merged commit 106f779 into main Aug 26, 2024
2 checks passed
@msalihaltun msalihaltun deleted the salih/fix-recording-message branch August 26, 2024 19:51
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