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

Format observer outputs #1560

Merged
merged 1 commit into from
Jan 15, 2025
Merged

Format observer outputs #1560

merged 1 commit into from
Jan 15, 2025

Conversation

wintonzheng
Copy link
Contributor

@wintonzheng wintonzheng commented Jan 15, 2025

Important

Enhance observer output formatting and display in WorkflowRunOutput.tsx and update ObserverCruise type in types.ts.

  • Types:
    • Add output and summary fields to ObserverCruise in types.ts.
  • UI Enhancements:
    • In WorkflowRunOutput.tsx, add getAggregatedExtractedInformation() and formatExtractedInformation() to format outputs.
    • Display observerOutput in a new section with CodeEditor in WorkflowRunOutput().
    • Use formattedOutputs for displaying workflow run outputs.
  • Misc:
    • Add statusIsFinalized import and usage in WorkflowRunTimeline.tsx for better readability and handling of empty timelines.

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

…src/'

<!-- ELLIPSIS_HIDDEN -->

> [!IMPORTANT]
> Enhance observer output formatting and display in workflow components, and update type definitions and imports for improved readability.
>
>   - **Behavior**:
>     - Add `getAggregatedExtractedInformation()` and `formatExtractedInformation()` functions in `WorkflowRunOutput.tsx` to format outputs by aggregating extracted information.
>     - Display observer outputs in `WorkflowRunOutput.tsx` using a `CodeEditor` component.
>     - Update `WorkflowRunTimeline.tsx` to show a message when the timeline is empty and the workflow is finalized.
>   - **Types**:
>     - Add `output` and `summary` fields to `ObserverCruise` type in `types.ts`.
>   - **Misc**:
>     - Import `statusIsFinalized` in `WorkflowRunTimeline.tsx` and `WorkflowRunTimeline.tsx` for better readability.
>     - Update `tsconfig.node.json` to include `vite.config.eval.ts`.
>
> <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 701014765c5d0253aca2e0695c3251ce6540c385. 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! Incremental review on c373b74 in 34 seconds

More details
  • Looked at 120 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. skyvern-frontend/src/routes/workflows/workflowRun/WorkflowRunOutput.tsx:16
  • Draft comment:
    Consider adding a check to ensure output.extracted_information is an object before assigning it to extractedInformation[id]. This will prevent potential issues if the data structure changes.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The function getAggregatedExtractedInformation iterates over the outputs object to extract extracted_information from each entry. However, it does not handle the case where extracted_information might not be an object, which could lead to unexpected behavior if the data structure changes.
2. skyvern-frontend/src/routes/workflows/workflowRun/WorkflowRunOutput.tsx:30
  • Draft comment:
    Ensure that merging extracted_information into outputs does not unintentionally overwrite existing data. Consider renaming the key or handling conflicts appropriately.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The formatExtractedInformation function merges extracted_information with outputs. However, if outputs already contains an extracted_information key, it will be overwritten, which might not be the intended behavior.
3. skyvern-frontend/src/routes/workflows/workflowRun/WorkflowRunOutput.tsx:156
  • Draft comment:
    Consider implementing a mechanism to truncate or paginate large JSON data to improve performance and readability in the CodeEditor.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The WorkflowRunOutput component uses JSON.stringify to format JSON data for display. However, if the data is large, this could impact performance and readability.
4. skyvern-frontend/src/routes/workflows/workflowRun/WorkflowRunTimeline.tsx:104
  • Draft comment:
    Ensure workflowRunTimeline is an array before calling .map to prevent potential runtime errors.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The WorkflowRunTimeline component uses workflowRunTimeline?.map without checking if workflowRunTimeline is an array, which could lead to runtime errors if it's undefined or not an array.

Workflow ID: wflow_lUIgE03YNlfSVmkQ


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! Reviewed everything up to c373b74 in 1 minute and 59 seconds

More details
  • Looked at 120 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. skyvern-frontend/src/routes/workflows/workflowRun/WorkflowRunOutput.tsx:16
  • Draft comment:
    Consider using reduce for conciseness:
const extractedInformation = Object.entries(outputs).reduce((acc, [id, output]) => {
  if (typeof output === "object" && output !== null && "extracted_information" in output) {
    acc[id] = output.extracted_information;
  }
  return acc;
}, {});
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The function getAggregatedExtractedInformation iterates over the outputs object and checks if each output has an extracted_information property. If it does, it adds it to the extractedInformation object. This is a straightforward implementation, but it could be more concise using reduce. This suggestion applies to similar patterns in the codebase.
2. skyvern-frontend/src/routes/workflows/workflowRun/WorkflowRunOutput.tsx:30
  • Draft comment:
    Consider returning directly for simplicity:
return {
  extracted_information: getAggregatedExtractedInformation(outputs),
  ...outputs,
};
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The function formatExtractedInformation is straightforward, but it could be optimized by directly returning the result of getAggregatedExtractedInformation without storing it in a variable first. This is a minor optimization but can improve readability.
3. skyvern-frontend/src/routes/workflows/workflowRun/WorkflowRunTimeline.tsx:17
  • Draft comment:
    Remove unused import statusIsFinalized to clean up the code.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The statusIsFinalized function is imported but not used in the WorkflowRunTimeline component. This is unnecessary and should be removed to clean up the code.
4. skyvern-frontend/src/routes/workflows/workflowRun/WorkflowRunTimeline.tsx:59
  • Draft comment:
    Remove unused variable workflowRunIsFinalized to clean up the code.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The workflowRunIsFinalized variable is defined but not used in the WorkflowRunTimeline component. This is unnecessary and should be removed to clean up the code.

Workflow ID: wflow_VGDqMllyknPRGP6V


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

@msalihaltun msalihaltun merged commit b49b4b1 into main Jan 15, 2025
6 checks passed
@msalihaltun msalihaltun deleted the salih/format-observer-outputs branch January 15, 2025 11:41
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