refactor(ui): workflow unsaved changes/published state tracking #7891
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
psychedelicious
requested review from
blessedcoolant,
maryhipp and
hipsterusername
as code owners
4 tasks
hipsterusername
approved these changes
Apr 7, 2025
hipsterusername
approved these changes
Apr 7, 2025
Previously, the workflow form's root element id was random. Every time we reset the workflow editor, the root id changed. This makes it difficult to check if the workflow editor is untouched (in its default state). Now that root element's id is simply "root". I can't imagine any way that this would break anything.
Accidentally left in from prev change
Previously, we maintained an `isTouched` flag in redux state to indicate if a workflow had unsaved changes. We manually updated this whenever we changed something on the workflow. This was tedious and error-prone. It also didn't handle undo/redo, so if you made a change to a node and undid it, we'd still think the workflow had unsaved changes. Moving forward, we use a simpler and more robust strategy by hashing the server's version of the workflow and comparing it to the client's version of the workflow. The hashing uses `stable-hash`, which is both fast and, well, stable. Most importantly, the ordering of keys in hashed objects does not change the resultant hash. - Remove `isTouched` state entirely. - Extract the logic that builds the "preview" workflow object from redux state into its own hook. This "preview" workflow is what we send to the server when saving a workflow. This "preview" workflow is effectively the client version of the workflow. - Add `useDoesWorkflowHaveUnsavedChanges()` hook, which compares the hash of the client workflow and server workflow (if it exists). - Add `useIsWorkflowUntouched()` hook, which compares the hash of the client workflow and the initial workflow that you get when you click new workflow. - Remove `reactflow` workaround in the nodes slice undo/redo filter. When we set the nodes state while loading a workflow, `reactflow` emits a nodes size/placement change event. This triggered up our `isTouched` flag logic and marked the workflow as unsaved right from the get-go. With the new strategy to track touched status, this workaround can be removed. - Update all logic that tracked the old `isTouched` flag to use the new hooks.
Whether a workflow is published or not shouldn't be something stored on the client. It's properly server-side state. This change removes the `is_published` flag from redux and updates all references to the flag to use the getWorkflow query. It also updates the socket event listener that handles session complete events. When a validation run completes, we invalidate the tags for the getWorkflow query. We need to do a bit of juggling to avoid a race condition (documented in the code). Works well though.
psychedelicious
force-pushed
the
psyche/feat/ui/workflow-touched
branch
from
3ff40b0 to
689bd2f
Compare
There was a problem hiding this comment.
Copilot reviewed 29 out of 29 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (3)
invokeai/frontend/web/src/features/nodes/components/sidePanel/workflow/PublishWorkflowPanelContent.tsx:251
- [nitpick] Consider renaming the 'isWorkflowSaved' prop to a name that directly reflects its meaning (such as 'hasUnsavedChanges') to avoid ambiguity from the negation logic.
isWorkflowSaved={!doesWorkflowHaveUnsavedChanges}
invokeai/frontend/web/src/features/nodes/components/sidePanel/workflow/IsolatedWorkflowBuilderWatcher.tsx:58
- Consider adding test cases to verify the behavior of the new unsaved changes tracking logic implemented in this hook.
export const useDoesWorkflowHaveUnsavedChanges = () => {
invokeai/frontend/web/src/features/nodes/store/workflowSlice.ts:89
- The removal of 'state.isTouched' updates from the action handlers requires comprehensive integration tests to ensure unsaved changes tracking still works as expected.
state.name = action.payload;
4 tasks
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.
Summary
Related Issues / Discussions
n/a
QA Instructions
Try opening and editing a workflow. It should track unsaved changes correctly. The workflow empty state should be detected correctly (i.e. when the workflow is identical to what you get when you make a new workflow, you should see empty state prompting you to load or create a new workflow).
Merge Plan
n/a
Checklist
What's Newcopy (if doing a release after this PR)