-
Notifications
You must be signed in to change notification settings - Fork 923
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
task v2 refactor part 7: observer_task -> task_v2 frontend code #1821
Conversation
…src/' <!-- ELLIPSIS_HIDDEN --> > [!IMPORTANT] > Rename `observer_task` to `task_v2` across workflow components and API types in `skyvern-frontend`. > > - **Renaming**: > - Rename `observer_task` to `task_v2` in `WorkflowPostRunParameters.tsx`, `WorkflowRun.tsx`, and `WorkflowRunOutput.tsx`. > - Update `WorkflowRunStatusApiResponse` type in `types.ts` to use `task_v2`. > - **Behavior**: > - Update logic in `WorkflowPostRunParameters.tsx` to check `task_v2` for webhook and proxy location. > - Modify `WorkflowRun.tsx` to use `task_v2` for task prompt display. > - Adjust `WorkflowRunOutput.tsx` to display `task_v2` output. > > <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 6bda30496e71b955144eb07e25d13f0fd3955943. 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.
👍 Looks good to me! Reviewed everything up to a5c75eb in 1 minute and 27 seconds
More details
- Looked at
79
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
8
drafted comments based on config settings.
1. skyvern-frontend/src/routes/workflows/workflowRun/WorkflowPostRunParameters.tsx:47
- Draft comment:
Consider renaming 'isObserverTask' to 'isTaskV2' to match the property name 'task_v2'. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. skyvern-frontend/src/routes/workflows/WorkflowRunOutput.tsx:80
- Draft comment:
Consider renaming 'observerOutput' to 'taskV2Output' for clarity since it fetches from 'task_v2'. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. skyvern-frontend/src/api/types.ts:244
- Draft comment:
Renamed property 'observer_task' to 'task_v2' is consistent. Consider renaming the associated type if it makes sense. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. skyvern-frontend/src/routes/workflows/WorkflowRun.tsx:129
- Draft comment:
isTaskv2Run now correctly checks 'task_v2'. This update is consistent with the refactor. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
5. skyvern-frontend/src/routes/workflows/workflowRun/WorkflowPostRunParameters.tsx:47
- Draft comment:
Variable name 'isObserverTask' is now based on 'task_v2'; consider renaming it to 'isTaskv2' for consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
While the suggestion technically makes sense since the property name changed, this seems like a minor stylistic preference. The current name 'isObserverTask' might actually be more semantically meaningful as it describes what the task IS rather than just matching the property name. Without more context about what an "observer task" means in the codebase, we can't be sure the rename would be an improvement.
The comment assumes that matching the property name is better than using a semantic name, which may not be true. We also don't know if "observer task" has special meaning in the codebase that would make the current name more appropriate.
Given that variable naming is often about domain meaning rather than matching property names, and that we lack context about the domain meaning, this comment may not be adding value.
The comment should be removed as it's making assumptions about naming without sufficient context, and the current name may actually be more semantically appropriate.
6. skyvern-frontend/src/routes/workflows/workflowRun/WorkflowPostRunParameters.tsx:50
- Draft comment:
Updated properties (webhook_callback_url and proxy_location) accessed via task_v2 look correct with optional chaining. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
7. skyvern-frontend/src/routes/workflows/workflowRun/WorkflowPostRunParameters.tsx:149
- Draft comment:
Rendering of Task 2.0 parameters now uses 'task_v2' check, which is consistent and correct. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
8. skyvern-frontend/src/routes/workflows/workflowRun/WorkflowRunOutput.tsx:80
- Draft comment:
Accessing task_v2 output (observerOutput) is updated correctly. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_GJGUQaLKglNkNcX8
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 a5c75eb in 1 minute and 30 seconds
More details
- Looked at
79
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
9
drafted comments based on config settings.
1. skyvern-frontend/src/api/types.ts:244
- Draft comment:
Ensure backend and frontend agree on renaming 'observer_task' to 'task_v2'. Verify that the ObserverTask type now semantically matches Task 2.0. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to ensure and verify that the backend and frontend agree on a renaming and that the types semantically match. This falls under asking the author to confirm or verify intentions, which is against the rules.
2. skyvern-frontend/src/routes/workflows/WorkflowRun.tsx:129
- Draft comment:
The check 'workflowRun.task_v2 !== null' is correct; consider consistent naming (e.g., TaskV2) for clarity. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. skyvern-frontend/src/routes/workflows/workflowRun/WorkflowPostRunParameters.tsx:47
- Draft comment:
Rename variable 'isObserverTask' to 'isTaskV2' for clarity, since it now checks task_v2. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The variable name is somewhat misleading now that the implementation changed. However, this seems like a relatively minor naming issue. The variable is only used locally within this component, and its purpose is still clear from the context. The behavior hasn't changed, just the property name being checked.
The variable name could be confusing to future developers who see "observer" in the name but "task_v2" in the code. However, there may be a broader codebase context where "observer task" is still the conceptual term used.
While the name mismatch is real, this is a relatively minor local variable naming issue that doesn't impact functionality or maintainability significantly.
Delete this comment as it suggests a minor variable rename that doesn't significantly impact code quality or clarity.
4. skyvern-frontend/src/routes/workflows/workflowRun/WorkflowRunOutput.tsx:80
- Draft comment:
Update for task_v2 output is correct; ensure the UI handles output structure as expected. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
5. skyvern-frontend/src/api/types.ts:244
- Draft comment:
Renamed 'observer_task' to 'task_v2' as part of the refactor. Consider if the type 'ObserverTask' should also be renamed for clarity, to avoid confusion with older implementations. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
6. skyvern-frontend/src/routes/workflows/WorkflowRun.tsx:129
- Draft comment:
Updated check to use 'task_v2' instead of 'observer_task'. Looks good. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
7. skyvern-frontend/src/routes/workflows/workflowRun/WorkflowPostRunParameters.tsx:47
- Draft comment:
Updated usage of 'observer_task' to 'task_v2'. Consider renaming the local variable 'isObserverTask' to 'isTaskv2Run' for consistency with other files. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
8. skyvern-frontend/src/routes/workflows/workflowRun/WorkflowRunOutput.tsx:80
- Draft comment:
Updated observerOutput to use 'task_v2'. This change is consistent with the refactor. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
9. skyvern-frontend/src/routes/workflows/workflowRun/WorkflowRunOutput.tsx:140
- Draft comment:
Consider updating the header on line 140 from 'Task 2.0 Output' to 'Task v2 Output' for consistency with the updated property name (workflowRun?.task_v2?.output). - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_u4qIRxQTy1BdWcei
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Important
Rename
observer_task
totask_v2
in frontend components to update task-related data handling and display.observer_task
totask_v2
inWorkflowRunStatusApiResponse
intypes.ts
.isTaskv2Run
andisObserverTask
checks inWorkflowRun.tsx
andWorkflowPostRunParameters.tsx
.observerOutput
to usetask_v2.output
inWorkflowRunOutput.tsx
.task_v2
presence instead ofobserver_task
inWorkflowRun.tsx
,WorkflowPostRunParameters.tsx
, andWorkflowRunOutput.tsx
.task_v2
related data, such aswebhook_callback_url
,proxy_location
, andprompt
.This description was created by
for a5c75eb. It will automatically update as commits are pushed.