Skip to content

Extract workflow-builder store/API modules - #19

Merged
matteius merged 6 commits into
opensensor:mainfrom
ViolanteCodes:add_create_workflow_builder_store
Aug 10, 2026
Merged

Extract workflow-builder store/API modules#19
matteius merged 6 commits into
opensensor:mainfrom
ViolanteCodes:add_create_workflow_builder_store

Conversation

@ViolanteCodes

@ViolanteCodes ViolanteCodes commented Aug 10, 2026

Copy link
Copy Markdown
Contributor
  • f0c45d7 - Add createWorkflowBuilderStore() factory to hold node/connection/counter state
  • bb0cfe0 - Extract workflow-builder-api.js out of workflow-builder.js
  • ddaa4d0 - Add regression coverage for store-wiring
  • 4269032 - Bugfix: fix showNodeProperties() generic change listener to correctly read e.target.checked for checkboxes (vs e.target.value)

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
@ViolanteCodes

Copy link
Copy Markdown
Contributor Author

@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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() / WorkflowBuilderStore as a per-instance state owner for nodes, connections, and node id generation.
  • Extract load/save + dirty tracking into workflow-builder-api.js and mix it into WorkflowBuilder.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

  • nodes is now proxied through this.store and setNodes() emits nodes-changed, but mutating the array in-place with push() 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

  • nodes updates are intended to go through the store proxy so setNodes() can emit nodes-changed. Using push() 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.
@matteius
matteius merged commit 085ac30 into opensensor:main Aug 10, 2026
5 checks passed
@matteius

Copy link
Copy Markdown
Contributor

Thanks for all your work helping to improve this package!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants