NXT-4224: WIP: New event type 'WORKFLOW_CHANGED' #25
+151
−4
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
NXT-4224 (Automatically / Manually sync when editing workflows in the browser)
There was a problem hiding this comment.
Pull request overview
This PR introduces a new workflow event type WORKFLOW_CHANGED to enable synchronization when workflows are edited in the browser. The change adds a new event constant and ensures it's fired whenever a workflow is marked as dirty.
Key changes:
- Added
WORKFLOW_CHANGEDenum constant toWorkflowEvent.Type - Modified
setDirty()to always fireWORKFLOW_CHANGEDevent in addition to conditionalWORKFLOW_DIRTYevent
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| WorkflowEvent.java | Adds new WORKFLOW_CHANGED event type constant with documentation |
| WorkflowManager.java | Updates setDirty() to fire the new WORKFLOW_CHANGED event and renames local variable for clarity |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
9965
to
+9966
| } | ||
| notifyWorkflowListeners(new WorkflowEvent(WorkflowEvent.Type.WORKFLOW_CHANGED, getID(), null, null)); |
There was a problem hiding this comment.
The WORKFLOW_CHANGED event is fired unconditionally on every setDirty() call, even when the workflow is already dirty. This could result in redundant event notifications for listeners. Consider firing this event only when sendWorkflowDirtyEvent is true, or document why unconditional firing is necessary for the browser sync functionality.
Suggested change
| } | |
| notifyWorkflowListeners(new WorkflowEvent(WorkflowEvent.Type.WORKFLOW_CHANGED, getID(), null, null)); | |
| notifyWorkflowListeners(new WorkflowEvent(WorkflowEvent.Type.WORKFLOW_CHANGED, getID(), null, null)); | |
| } |
|
A syncer ultimately is specific to a WorkflowManager instance because it listens to events emitted by a wfm. Indeed, currently, a Session corresponds to a Project corresponds to a WorkflowManager. However, the WorkflowManager instance is not guaranteed to be present for the entire session, see e.g. WorkflowService#disposeVersion. So we need to react to the wfm being disposed. This could be implemented by a Project#onWfmDispose callback (as in previous commits). But then in the callback implementation one would have to identify the syncer instance to call `dispose` on, i.e. have to know the project/session ID and then lookup the syncer in WorkflowSyncerManager. Attaching the syncer directly to the wfm via WorkflowResourceCache means (a) We do not need to add listener support in Project/ProjectWfmCache, (b) we do not need to register the on-dispose procedure of the syncer, complicated by a syncer only being optionally present, (c) we do not have to implement an access facility (previously the WorkflowSyncerManager/WorkflowSynerProvider service dependency) but can use existing API. Note that two things have to be considered: (1) the syncer has to be attached to the wfm instance already loaded in GatewayJobPool (2) the syncer has to be attached to any wfm dynamically loaded in the future. For separation of concerns, (2) should not affect the WorkflowManagerLoader implementation (as in previous commits). (1) has to be done from the session initialisation. For (2) I can currently think of no better way than to expose a Project#onWfmLoad callback (as in previous commits). Disposing a syncer instance is necessary because the sync job has to be canceled. For instance, a debounce timer might still be running. Avoid having to pass projectId to calls like LocalSaver#saveProject -- this is needed only so that the implementation can access the wfm instance. This is counterintuitive since the syncer is already created specific to a wfm instance. Further, the wfm access would have to happen via static/singleton access (as in previous commits) or injected.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NXT-4224 (Automatically / Manually sync when editing workflows in the browser)