-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Maryhipp/canvas workflows #8596
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: main
Are you sure you want to change the base?
Conversation
…d outputs of the canvas, build UI where use can select a workflow with these nodes to run against canvas
Add support for displaying workflow exposed fields in the canvas parameters panel. Uses a shadow slice pattern to maintain complete state isolation between canvas and workflow tabs. Key features: - Shadow nodes slice mirrors workflow nodes structure for canvas workflows - Context providers redirect field component selectors to canvas workflow data - Middleware intercepts field mutations and routes to appropriate slice - Filters out canvas input nodes from exposed fields - Always displays fields in view mode - Each field wrapped with correct node context for proper data access 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
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.
Overall I think this will work just fine. Some design suggestions below
slice, | ||
schema: zNodesState, | ||
getInitialState, | ||
persistConfig: { |
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.
Can omit persistConfig
instead of adding denylist to skip persistence for the whole slice
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.
But that said, I think we do want persistence here... If I load a workflow into canvas and change some of the fields, I'd expect them to persist.
); | ||
} | ||
|
||
// Check if field has a value (not null/undefined/empty) |
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.
This requires the user to select an image for the composite raster layer node before saving the workflow, but that seems unnecessary
const INPUT_TAG = 'canvas-workflow-input'; | ||
const OUTPUT_TAG = 'canvas-workflow-output'; | ||
|
||
const validateCanvasWorkflow = (workflow: WorkflowV3, templates: Templates): ValidateResult => { |
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.
There is already a robust workflow validation utility validateWorkflow()
we use when loading workflows: invokeai/frontend/web/src/features/nodes/util/workflow/validateWorkflow.ts
Before invoking, we use this getInvocationNodeErrors()
utility to check each node to make sure it has the right inputs: invokeai/frontend/web/src/features/nodes/store/util/fieldValidators.ts
We could:
- Validate workflow using
validateWorkflow()
- Get all node errors using
getInvocationNodeErrors()
- Make an exception for "no image input" errors for
canvas-workflow-input
nodes - Do the extra "only one input node and one output node" checks at the end
const seed = g.addNode({ | ||
id: getPrefixedId('seed'), | ||
type: 'integer', | ||
}); | ||
|
||
const positivePrompt = g.addNode({ | ||
id: getPrefixedId('positive_prompt'), | ||
type: 'string', | ||
value: prompts.positive, | ||
}); |
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.
Think we should skip these and also not add metadata - up to the workflow author to build metadata if they want it.
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.
This isn't working quite right. The nodes slice always handles the actions even if this listener triggers and dispatches the canvas-specific action also.
For example, if I change the field in the workflow editor, it also changes it in the canvas workflow.
There's an "action router" pattern I've been exploring in #8553 that I think could solve this. The idea is to inject metadata into actions and use it to decide who handles the action. In that PR, the metadata is the canvas ID. For this PR, the metadata would be a flag that says "Is this action for the node editor workflow, or canvas workflow?". Roughed out:
// flag injection/matching utilities, use symbol to prevent collisions
const CANVAS_WORKFLOW_KEY = Symbol('CANVAS_WORKFLOW_KEY');
const NODES_WORKFLOW_KEY = Symbol('NODES_WORKFLOW_KEY');
// call this on the action before dispatching if we are in the canvas workflow, this will go in the useDispatch wrapper below
const injectCanvasWorkflowKey = (action: UnknownAction) => {
Object.assign(action, { meta: { [CANVAS_WORKFLOW_KEY]: true } }); // could be a canvas id instead of boolean
};
// call this on the action before dispatching if we are in node editor
const injectNodesWorkflowKey = (action: UnknownAction) => {
Object.assign(action, { meta: { [NODES_WORKFLOW_KEY]: true } });
};
// type guards
const isCanvasWorkflowAction = (action: UnknownAction) => {
return isPlainObject(action?.meta) && action?.meta?.[CANVAS_WORKFLOW_KEY] === true;
};
const isNodesWorkflowAction = (action: UnknownAction) => {
return isPlainObject(action?.meta) && action?.meta?.[NODES_WORKFLOW_KEY] === true;
};
// cut and paste all the feild reducers from nodesSlice to this new slice
// but this slice doesn't get added to the store, we just use it as a convenient way to define the reducers
const fieldSlice__DO_NOT_ADD_TO_STORE = createSlice({
name: 'fields',
initialState: getInitialState(),
reducers: {
fieldValueReset: (state, action: FieldValueAction<StatefulFieldValue>) => {
fieldValueReducer(state, action, zStatefulFieldValue);
},
fieldStringValueChanged: (state, action: FieldValueAction<StringFieldValue>) => {
fieldValueReducer(state, action, zStringFieldValue);
},
//... rest of field reducers
},
});
// this will match any field action, regardless of its flag
const isFieldAction = isAnyOf(...Object.values(fieldSlice__DO_NOT_ADD_TO_STORE.actions).map((a) => a.match))
Then in nodesSlice and canvasWorkflowSlice we can do this:
const nodesSlice = createSlice({
// ...
extraReducers: (builder) => {
builder.addMatcher(
isFieldAction,
(state, action) => {
if (!isNodesWorkflowAction(action)) {
return;
}
fieldSlice__DO_NOT_ADD_TO_STORE.reducer(state, action);
}
);
},
})
const canvasWorkflowSlice = createSlice({
// ...
extraReducers: (builder) => {
builder.addMatcher(
isFieldAction,
(state, action) => {
if (!isCanvasWorkflowAction(action)) {
return;
}
fieldSlice__DO_NOT_ADD_TO_STORE.reducer(state, action);
}
);
},
})
Wrap useAppDispatch
to automatically inject the flags:
export const useAppDispatch = () => {
const isCanvasWorkflow = useCanvasWorkflowContext() // bool | null
const isNodesWorkflow = useNodesWorkflowContext() // bool | null
const dispatch = useDispatch();
return useCallback((action) => {
if (isCanvasWorkflow && isNodesWorkflow) {
throw new Error('A component cannot be in both a canvas workflow and a nodes workflow');
}
if (isCanvasWorkflow) {
injectCanvasWorkflowKey(action);
} else if (isNodesWorkflow) {
injectNodesWorkflowKey(action);
}
return dispatch(action);
}, [dispatch, isCanvasWorkflow, isNodesWorkflow])
}
This pattern is similar to what I'm proposing in #8553 . Plays nicely with it and a future where we have multiple workflow editor instances
Summary
Related Issues / Discussions
QA Instructions
Merge Plan
Checklist
What's New
copy (if doing a release after this PR)