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

feat(frontend): Catch flow errors in the UI #4429

Merged
merged 3 commits into from
Sep 25, 2024
Merged

Conversation

fatonramadani
Copy link
Contributor

@fatonramadani fatonramadani commented Sep 24, 2024

Screen.Recording.2024-09-24.at.14.33.22.mov

Important

Adds error handling for flow parsing in the frontend, detecting cycles and duplicates, with UI alerts and YAML editor access for corrections.

  • Error Handling:
    • Adds cycle detection in graphBuilder.ts using detectCycle().
    • Handles duplicate nodes in addNode() in graphBuilder.ts.
    • Updates graphBuilder.ts to return an error message if exceptions occur during graph building.
  • UI Enhancements:
    • Displays error alerts in FlowGraphV2.svelte if graph parsing fails.
    • Adds Alert and Button components in FlowGraphV2.svelte to notify users of errors and provide a way to open the YAML editor.
    • Modifies FlowConstants.svelte to handle flow steps with indices to prevent key collisions.

This description was created by Ellipsis for 3e0d983. 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.

👍 Looks good to me! Reviewed everything up to 2148a24 in 27 seconds

More details
  • Looked at 1064 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. frontend/src/lib/components/graph/graphBuilder.ts:105
  • Draft comment:
    The cycle detection logic only checks for cycles starting from the source node. Consider checking for cycles in the entire graph after all edges are added to ensure no cycles exist.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment points out a potential flaw in the cycle detection logic, which is relevant to the changes made in the diff. The current implementation checks for cycles only from the source node when adding an edge, which might not catch all cycles. This is a valid concern and suggests a potential improvement to the code.
    The comment might be speculative if the current cycle detection is sufficient for the intended use case. Without more context on how the graph is used, it's hard to determine if the suggested change is necessary.
    The comment highlights a potential issue with the current implementation that could lead to undetected cycles, which is a valid concern for graph algorithms.
    The comment is valid as it points out a potential issue with the cycle detection logic that could lead to undetected cycles. It should be kept as it suggests a meaningful improvement to the code.
2. frontend/src/lib/components/graph/graphBuilder.ts:530
  • Draft comment:
    The error handling in the graphBuilder function is too generic. Consider logging the error or providing more context to help with debugging.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is about a change made in the diff, specifically the addition of a try-catch block. The suggestion to log errors or provide more context is a valid code quality improvement. It is actionable and clear, as it suggests a specific improvement to the error handling mechanism.
    The comment does not specify how to log the error or what additional context should be provided, which might make it less actionable. However, the suggestion itself is still valid and can be considered a good practice.
    While the comment could be more specific, the general suggestion to improve error handling is still valuable and aligns with best practices.
    The comment is valid and suggests a clear improvement to the error handling mechanism. It should be kept.
3. frontend/src/lib/components/graph/graphBuilder.ts:44
  • Draft comment:
    The addNode function throws an error for duplicate nodes. Consider handling this error more gracefully to avoid disrupting the entire graph building process.
  • 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, specifically the addition of error throwing in the addNode function. The suggestion to handle the error more gracefully is a valid code quality improvement, as it could prevent the entire graph building process from failing due to a single duplicate node. The current implementation does catch the error, but the suggestion to handle it more gracefully could lead to a more robust solution.
    The current implementation already catches the error and returns an error message, which might be considered sufficient error handling. The comment does not provide specific guidance on what 'more gracefully' means, which could make it less actionable.
    While the error is caught, the suggestion to handle it more gracefully could imply logging the error, skipping the duplicate node, or providing more detailed feedback, which could be beneficial.
    The comment is relevant and suggests a potential improvement in error handling. It should be kept as it addresses a change made in the diff and proposes a code quality enhancement.

Workflow ID: wflow_h031zyU7uXiUSrji


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

Copy link

cloudflare-workers-and-pages bot commented Sep 24, 2024

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: 3e0d983
Status: ✅  Deploy successful!
Preview URL: https://7e6d6e75.windmill.pages.dev
Branch Preview URL: https://fr-flow-parsing-errors.windmill.pages.dev

View logs

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 3e0d983 in 11 seconds

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

Workflow ID: wflow_rSeovaGS4YxkLF4b


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

@rubenfiszel rubenfiszel merged commit 84eefad into main Sep 25, 2024
3 checks passed
@rubenfiszel rubenfiszel deleted the fr/flow-parsing-errors branch September 25, 2024 14:22
@github-actions github-actions bot locked and limited conversation to collaborators Sep 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants