-
Notifications
You must be signed in to change notification settings - Fork 0
Enh/nxt 4424 workflow syncer #32
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
base: master
Are you sure you want to change the base?
Conversation
NXT-4224 (Automatically / Manually sync when editing workflows in the browser)
NXT-4224 (Automatically / Manually sync when editing workflows in the browser)
NXT-4224 (Automatically / Manually sync when editing workflows in the browser)
NXT-4224 (Automatically / Manually sync when editing workflows in the browser)
NXT-4224 (Automatically / Manually sync when editing workflows in the browser)
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.
Pull request overview
This PR implements project synchronization functionality for workflows, adding UI indicators and error handling for sync state changes. The changes introduce a new sync button in the toolbar that displays the current sync state and allows manual triggering of project saves when auto-sync is disabled or encounters size limits.
- Adds
ProjectSyncStatetracking to the application store with sync state management - Implements a sync button in the workflow toolbar with visual state indicators
- Introduces toast notifications for sync errors and size limit warnings
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| org.knime.ui.js/src/toastPresets/application.ts | Adds toast presets for sync failure and size limit errors |
| org.knime.ui.js/src/store/application/application.ts | Implements sync state tracking and error handling logic in the application store |
| org.knime.ui.js/src/components/toolbar/WorkflowToolbar.vue | Adds sync button UI component with state-dependent labels and click handler |
| org.knime.ui.js/src/components/toolbar/tests/WorkflowToolbar.test.ts | Updates mock to properly preserve original kds-components exports |
| org.knime.ui.js/src/api/gateway-api/generated-api.ts | Adds TypeScript definitions for ProjectSyncState and SyncStateDetails |
| org.knime.ui.java/src/eclipse/org/knime/ui/java/util/CreateProject.java | Changes method visibility from public to private |
| org.knime.ui.java/src/eclipse/org/knime/ui/java/browser/lifecycle/Init.java | Adds null WorkflowSyncerProvider parameter for desktop UI initialization |
| org.knime.ui.java/src/eclipse/org/knime/ui/java/api/SaveProject.java | Adds documentation for private helper methods |
| org.knime.ui.java/src/eclipse/org/knime/ui/java/api/OpenProject.java | Extracts progressReporter to a separate variable for clarity |
| org.knime.ui.java/src/eclipse/org/knime/ui/java/api/DesktopAPI.java | Removes unused javadoc suppression warning |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // TODO: Is this cool? | ||
| state === ProjectSyncState.StateEnum.WRITING && | ||
| this.projectSyncState.state !== ProjectSyncState.StateEnum.WRITING | ||
| ) { | ||
| useLifecycleStore().setIsLoadingWorkflow(true); | ||
| } else if ( | ||
| // TODO: Is this cool? |
Copilot
AI
Dec 18, 2025
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.
The TODO comment 'Is this cool?' is unclear about what needs validation. Replace with a specific question or concern about the logic, such as 'TODO: Verify if showing loading indicator during WRITING state is the correct UX behavior.'
| // TODO: Is this cool? | |
| state === ProjectSyncState.StateEnum.WRITING && | |
| this.projectSyncState.state !== ProjectSyncState.StateEnum.WRITING | |
| ) { | |
| useLifecycleStore().setIsLoadingWorkflow(true); | |
| } else if ( | |
| // TODO: Is this cool? | |
| // TODO: Verify if showing the workflow loading indicator during the WRITING project sync state is the correct UX behavior. | |
| state === ProjectSyncState.StateEnum.WRITING && | |
| this.projectSyncState.state !== ProjectSyncState.StateEnum.WRITING | |
| ) { | |
| useLifecycleStore().setIsLoadingWorkflow(true); | |
| } else if ( | |
| // TODO: Verify if hiding the workflow loading indicator after leaving the WRITING project sync state is the correct UX behavior. |
| // TODO: Is this cool? | ||
| state === ProjectSyncState.StateEnum.WRITING && | ||
| this.projectSyncState.state !== ProjectSyncState.StateEnum.WRITING | ||
| ) { | ||
| useLifecycleStore().setIsLoadingWorkflow(true); | ||
| } else if ( | ||
| // TODO: Is this cool? |
Copilot
AI
Dec 18, 2025
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.
The TODO comment 'Is this cool?' is unclear about what needs validation. Replace with a specific question or concern about the logic, such as 'TODO: Confirm if clearing loading indicator when exiting WRITING state is the desired behavior.'
| // TODO: Is this cool? | |
| state === ProjectSyncState.StateEnum.WRITING && | |
| this.projectSyncState.state !== ProjectSyncState.StateEnum.WRITING | |
| ) { | |
| useLifecycleStore().setIsLoadingWorkflow(true); | |
| } else if ( | |
| // TODO: Is this cool? | |
| // TODO: Confirm if setting loading indicator when entering WRITING state is the desired behavior. | |
| state === ProjectSyncState.StateEnum.WRITING && | |
| this.projectSyncState.state !== ProjectSyncState.StateEnum.WRITING | |
| ) { | |
| useLifecycleStore().setIsLoadingWorkflow(true); | |
| } else if ( | |
| // TODO: Confirm if clearing loading indicator when exiting WRITING state is the desired behavior. |
| case ProjectSyncState.StateEnum.ERROR: | ||
| return "Sync error"; | ||
| default: | ||
| return "Unexpected state"; // This should never happen |
Copilot
AI
Dec 18, 2025
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.
The error message 'Unexpected state' is not actionable for users or developers. Consider returning a more descriptive message that includes the actual state value, such as Unknown sync state: ${state}, to aid debugging.
| return "Unexpected state"; // This should never happen | |
| return `Unknown sync state: ${String(state)}`; // This should never happen |
| * @throws IllegalStateException if the dependencies have been already injected | ||
| */ | ||
| @SuppressWarnings({"java:S107", "JavadocDeclaration", "javadoc"}) // Parameter count | ||
| @SuppressWarnings({"java:S107", "JavadocDeclaration"}) // Parameter count |
Copilot
AI
Dec 18, 2025
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.
The removal of 'javadoc' from the suppressions list should be accompanied by ensuring that all Javadoc issues are resolved. Verify that no Javadoc warnings remain for this method.
No description provided.