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

Bugfix/output schema #146

Merged
merged 3 commits into from
Feb 7, 2025
Merged

Bugfix/output schema #146

merged 3 commits into from
Feb 7, 2025

Conversation

preet-bhadra
Copy link
Collaborator

@preet-bhadra preet-bhadra commented Feb 7, 2025

Important

Adds JSON schema generation for workflow input variables and integrates it into existing functions in flowSlice.ts.

  • Behavior:
    • Adds generateJsonSchema function to create JSON schema from a given schema.
    • Updates setWorkflowInputVariable, deleteWorkflowInputVariable, and updateWorkflowInputVariableKey to use generateJsonSchema for output_json_schema.
  • Refactoring:
    • Refactors setWorkflowInputVariable, deleteWorkflowInputVariable, and updateWorkflowInputVariableKey for better readability in flowSlice.ts.

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

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 f215ec5 in 1 minute and 29 seconds

More details
  • Looked at 142 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. frontend/src/store/flowSlice.ts:465
  • Draft comment:
    The JSON schema generation (lines 466-472) is duplicated from other reducers. Refactoring into a helper would improve maintainability.
  • Reason this comment was not posted:
    Marked as duplicate.
2. frontend/src/store/flowSlice.ts:535
  • Draft comment:
    Duplicated JSON schema generation logic appears here (lines 536-542). Consider refactoring this into a common utility to improve consistency.
  • Reason this comment was not posted:
    Marked as duplicate.
3. frontend/src/store/flowSlice.ts:411
  • Draft comment:
    Refactor duplicate JSON schema generation logic into a helper to reduce code duplication.
  • Reason this comment was not posted:
    Marked as duplicate.
4. frontend/src/store/flowSlice.ts:553
  • Draft comment:
    Edge update only renames sourceHandle; confirm if targetHandle should also be updated.
  • 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.
5. frontend/src/store/flowSlice.ts:522
  • Draft comment:
    Consider checking for a newKey collision in workflowInputVariables to avoid overwriting an existing entry.
  • 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_cpoamuyY5yfgFd2m


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.

}

// Generate JSON schema
const jsonSchema = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider extracting the JSON schema generation (lines 411-418) into a utility function to reduce duplication.

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 8ca19af in 34 seconds

More details
  • Looked at 85 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. frontend/src/store/flowSlice.ts:104
  • Draft comment:
    Good refactoring! Extracting the duplicate JSON schema generation into generateJsonSchema improves maintainability. Consider adding a brief JSDoc comment explaining the expected structure of the input schema and that all keys will be marked as required.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
2. frontend/src/store/flowSlice.ts:422
  • Draft comment:
    Refactoring looks good! Double-check that marking all keys as required (via Object.keys(schema)) is the intended behavior; in some cases, optional properties might be needed.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
3. frontend/src/store/flowSlice.ts:107
  • Draft comment:
    Good DRY refactor: The new generateJsonSchema helper removes duplicate inline JSON schema generation. Consider conditionally adding the 'required' field only if the schema has keys, to better conform to JSON Schema best practices.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
4. frontend/src/store/flowSlice.ts:424
  • Draft comment:
    Nice replacement of inline JSON schema generation with generateJsonSchema, improving maintainability and readability across multiple schema update blocks.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None

Workflow ID: wflow_zjiyc3H155lrkA1R


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

@preet-bhadra preet-bhadra merged commit a52bcab into main Feb 7, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant