Implement 'you have unsaved changes'

[ykeremy]

🚀	This description was created by Ellipsis for commit c5a8746

feat: add unsaved changes tracking to workflow editor

Summary:

Add unsaved changes tracking to workflow editor with state management, navigation prompt, and UI components.

Key points:

• Implement unsaved changes tracking in workflow editor.
• Use useWorkflowHasChangesStore in FlowRenderer.tsx, WorkflowEditor.tsx, and WorkflowParametersPanel.tsx.
• Display dialog in WorkflowEditor.tsx and FlowRenderer.tsx when navigating away with unsaved changes.
• Save changes using saveWorkflowMutation in FlowRenderer.tsx.
• Introduce WorkflowHasChangesStore.ts for hasChanges state management.
• Update FlowRenderer.tsx for node and edge modifications.
• Update WorkflowParametersPanel.tsx for parameter add, edit, and delete.
• Add dialog and buttons in FlowRenderer.tsx for handling unsaved changes.

---

Generated with ❤️ by ellipsis.dev

[ellipsis-dev]

👍 Looks good to me! Incremental review on c5a8746 in 28 seconds

More details

• Looked at 681 lines of code in 4 files
• Skipped 0 files when reviewing.
• Skipped posting 3 drafted comments based on config settings.

1. skyvern-frontend/src/routes/workflows/editor/FlowRenderer.tsx:430

• Draft comment:
  Consider checking if blocker.proceed is defined before calling it to prevent potential runtime errors.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The useBlocker hook is used to prevent navigation when there are unsaved changes. However, the blocker.proceed?.() function is called without checking if it exists, which could lead to potential runtime errors if blocker.proceed is undefined.

2. skyvern-frontend/src/routes/workflows/editor/FlowRenderer.tsx:437

• Draft comment:
  Consider using await with handleSave() to ensure the save operation completes before proceeding with navigation.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The handleSave function is called without awaiting its promise resolution. This could lead to unexpected behavior if the save operation is not completed before proceeding with navigation.

3. skyvern-frontend/src/routes/workflows/editor/FlowRenderer.tsx:438

• Draft comment:
  Consider checking if blocker.proceed is defined before calling it to prevent potential runtime errors.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The useBlocker hook is used to prevent navigation when there are unsaved changes. However, the blocker.proceed?.() function is called without checking if it exists, which could lead to potential runtime errors if blocker.proceed is undefined.

Workflow ID: wflow_TkuwH2KRoZ8auBpo

---

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

[ellipsis-dev]

👍 Looks good to me! Reviewed everything up to c5a8746 in 33 seconds

More details

• Looked at 681 lines of code in 4 files
• Skipped 0 files when reviewing.
• Skipped posting 5 drafted comments based on config settings.

1. skyvern-frontend/src/routes/workflows/editor/FlowRenderer.tsx:414

• Draft comment:
  Consider checking if blocker is defined before calling blocker.reset?.(). This will prevent potential runtime errors if blocker is undefined.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The use of optional chaining (?.) in the code already provides a safeguard against blocker being undefined. This makes the comment unnecessary as the code is already handling the potential issue. The comment does not suggest a change that is needed, as the code is correct as is.
  I might be overlooking a scenario where blocker could be undefined before the optional chaining is applied, but the use of useBlocker should ensure blocker is defined.
  The use of useBlocker should ensure that blocker is defined, and the optional chaining is a standard way to handle potential undefined values, making the comment unnecessary.
  The comment is unnecessary because the code already handles the potential issue with optional chaining. The comment should be deleted.

2. skyvern-frontend/src/routes/workflows/editor/FlowRenderer.tsx:436

• Draft comment:
  Consider adding error handling for the handleSave function call to manage potential promise rejections.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The handleSave function is already wrapped in a mutation that has an onError callback, which handles errors by showing a toast message. This indicates that error handling is already implemented. The comment may be redundant as the error handling seems to be adequately addressed.
  I might be overlooking a specific scenario where additional error handling could be beneficial, such as handling errors at the point of the handleSave call rather than within the mutation itself.
  The existing error handling within the mutation's onError callback should suffice for managing promise rejections, as it provides user feedback through a toast message.
  The comment is unnecessary because error handling is already implemented within the mutation's onError callback. The comment should be deleted.

3. skyvern-frontend/src/routes/workflows/editor/FlowRenderer.tsx:215

• Draft comment:
  Consider adding nodes and edges to the dependency array of this useEffect to ensure it runs when these values change.
• Reason this comment was not posted:
  Comment was on unchanged code.

4. skyvern-frontend/src/routes/workflows/editor/FlowRenderer.tsx:349

• Draft comment:
  Consider consolidating multiple setHasChanges(true) calls to reduce unnecessary re-renders. This is applicable in other parts of the code as well.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The setHasChanges(true) is called multiple times in the code, which could lead to unnecessary re-renders. It might be beneficial to consolidate these calls.

5. skyvern-frontend/src/store/WorkflowHasChangesStore.ts:10

• Draft comment:
  Consider implementing a mechanism to persist the hasChanges state across sessions to prevent loss of unsaved changes on page refresh.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment suggests a potential improvement to the code by adding persistence for the hasChanges state. This is a valid suggestion as it addresses a potential issue with losing state on page refresh. However, the comment is speculative and not directly related to a bug or error in the current implementation. It is more of a feature suggestion than a necessary code change.
  The comment could be seen as a useful suggestion for improving user experience, but it does not point out a flaw or error in the current code. It might not be necessary to address this in the current scope of changes.
  While the comment is speculative, it does highlight a potential user experience issue that could be important depending on the application's requirements. However, without knowing the specific requirements, it's hard to justify keeping the comment.
  The comment is speculative and suggests a feature rather than addressing a clear issue with the current code. It should be deleted as it does not indicate a necessary code change.

Workflow ID: wflow_LkgC3kRMNFWq7zFp

---

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