Extract workflow-builder store/API modules - #19
Conversation
Single owner of nodes/connections/nodeIdCounter, same shape as form-builder-store.js's BuilderStore — factory, not a singleton (admin inlines can put two builders on one page), extends EventTarget so future extracted modules can react to changes instead of closing over the whole WorkflowBuilder instance. WorkflowBuilder now holds this.store and proxies this.nodes/this.connections/this.nodeIdCounter through it via getters/setters, so every existing call site keeps working unchanged. nextNodeId()/seedNodeIdCounterFromNodes() aren't wired into any call sites yet — available for the next extraction to use, same sequencing form-builder.js's saga followed (store landed before fieldIdCounter++ call sites were migrated to store.nextFieldId()).
Pure move: loadWorkflow, saveWorkflow, and the save-status/dirty-tracking helpers they depend on (setSaveStatus, getWorkflowSnapshot, syncSavedWorkflowSnapshot, updateDirtyState, updateDirtyIndicator) into their own module, mixed onto WorkflowBuilder.prototype via Object.assign — same shape as form-builder-api.js. No logic changed.
createStartNode/createNode had zero test coverage before or after being wired to store.nextNodeId() -- added tests confirming ids come from the store, the counter is shared/sequential across both methods, and a newly-created node doesn't collide with ids already seeded from a loaded workflow. Also strengthened getWorkflowSnapshot/loadWorkflow's existing tests with spies on store.snapshot()/store.seedNodeIdCounterFromNodes() -- the prior tests only checked output equivalence, which would pass whether or not those methods actually delegate to the store, so they wouldn't catch a regression back to the duplicated inline logic.
Newly created workflow builder steps would return a 400 error on attempted save with log message: "Workflow validation failed: ['“on” value must be either True or False.']". Adjusting the step's name would allow the step to be saved. After investigation, determined that showNodeProperties() attaches a generic change listener to every input in the node properties panel. Listener read e.target.value unconditionally and passed it to updateNodeProperty(), but checkbox .value = static HTML value attribute, which defaults to literal string "on" and not bool, which was then rejected by Django's BooleanField
|
@matteius I did my best to QA this one, but as I have way less experience with the workflow-builder (I've spent most of my time in your application working with form-builder) may want to be extra careful on this one just to be on the safe side |
There was a problem hiding this comment.
Pull request overview
Refactors the Workflow Builder frontend by extracting shared state into a dedicated store module and separating the load/save (API + dirty-tracking) logic into its own mixin, while adding JS regression coverage—especially around checkbox handling in the node properties panel.
Changes:
- Introduce
createWorkflowBuilderStore()/WorkflowBuilderStoreas a per-instance state owner for nodes, connections, and node id generation. - Extract load/save + dirty tracking into
workflow-builder-api.jsand mix it intoWorkflowBuilder.prototype. - Add Vitest regression tests for store wiring, node creation id sequencing, API methods, and checkbox change handling.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests_js/workflow-builder/showNodeProperties.test.js | Adds regression coverage for checkbox change forwarding (checked vs "on"). |
| tests_js/workflow-builder/nodeCreation.test.js | Verifies node creation uses the store’s shared node id counter and renders/stacking behavior. |
| tests_js/workflow-builder-store/createWorkflowBuilderStore.test.js | Covers store factory semantics, events, id generation, snapshot/restore. |
| tests_js/workflow-builder-api/apiMethods.test.js | Adds coverage for extracted API methods (load/save, dirty tracking, status). |
| django_forms_workflows/static/django_forms_workflows/js/workflow-builder.js | Wires in the new store + API mixin; fixes checkbox value forwarding in properties listener. |
| django_forms_workflows/static/django_forms_workflows/js/workflow-builder-store.js | New EventTarget-based store for nodes/connections/counter + snapshot/restore. |
| django_forms_workflows/static/django_forms_workflows/js/workflow-builder-api.js | New module containing extracted load/save/status/dirty-tracking methods. |
Suppressed comments (2)
django_forms_workflows/static/django_forms_workflows/js/workflow-builder.js:431
nodesis now proxied throughthis.storeandsetNodes()emitsnodes-changed, but mutating the array in-place withpush()bypasses the setter/event. This makes it impossible for extracted modules listening to the store to observe node creation reliably. Prefer updating via the setter with a new array (or a dedicated store method) so the change is observable.
This issue also appears on line 435 of the same file.
createStartNode() {
const node = {
id: this.store.nextNodeId(),
type: 'start',
x: 100,
y: 100,
data: {}
};
this.nodes.push(node);
this.bringNodeToFront(node.id);
django_forms_workflows/static/django_forms_workflows/js/workflow-builder.js:444
nodesupdates are intended to go through the store proxy sosetNodes()can emitnodes-changed. Usingpush()mutates in place and bypasses that mechanism, so any store listeners would miss node creation. Update via the setter (copy-on-write) or add a store helper that pushes + dispatches.
createNode(type, x, y) {
const node = {
id: this.store.nextNodeId(),
type: type,
x: x,
y: y,
data: this.getDefaultNodeData(type)
};
this.nodes.push(node);
this.bringNodeToFront(node.id);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
push() mutated this.ndoes in place, skipping setNodes() and the nodes-changed event it dispatches. Use copy-on-write via the setter, matching the existing pattern used for node deletion
Same class of bug as createNode/createStartNode fix: push() mutated this.connections in place.
|
Thanks for all your work helping to improve this package! |
Uh oh!
There was an error while loading. Please reload this page.