Extract Property Editor into separate module - #12
Merged
matteius merged 6 commits intoAug 3, 2026
Conversation
…is.escapeHtml(...) before landing in the disabled Field Type input, matching every other field value on that tab.
…view matches The live preview always sends fields: this.fields (flat array order), but handleFieldDroppedToStep/handleFieldMovedToStep/updateFieldOrderInStep only kept formSteps in sync, never this.fields itself - so a field added or moved within a step could render correctly on the canvas but show up in the wrong position (or last) in the preview. Wires in the existing but previously-uncalled updateFieldOrderFromSteps() after each mutation. Since data-field-index on every rendered field-item is derived from this.fields' current array index, the reorder has to happen before re-rendering, and every step's canvas has to be re-rendered (not just the touched one) - a flattened-array reorder can shift indices for fields in other steps too. Rendering first (or only re-rendering the touched step) left stale indices behind, which a subsequent drag-reorder would read and silently corrupt this.fields/formSteps - the actual cause of a 500 from the preview endpoint found via manual testing.
Extends handleFieldDroppedToStep.test.js and updateFieldOrderInStep.test.js to assert on this.fields, not just formSteps. Adds handleFieldMovedToStep.test.js (previously zero coverage) and multiStepFieldIndexSync.test.js, which specifically reproduces the cross-step index corruption reported after the initial fix: a field added to one step shifting data-field-index for fields in a different, unrendered step, then a follow-up reorder reading those stale indices and silently dropping a field. Verified each new/extended assertion actually fails against both the pre-fix code and the intermediate (render-before-reorder) attempt before landing the real fix in the previous commit.
…ep mode handleFieldMovedToStep (dragging an existing field between steps) and updateFieldOrderInStep (reordering within a step) never called pushUndo(), unlike every other field-mutating action (add, duplicate, delete, drop from palette). Ctrl+Z had nothing on the undo stack to restore to after either action - pre-existing gap, not introduced by the field-order sync fix, just surfaced by testing that fix. Both call sites (Sortable's onAdd/onUpdate) only fire when something actually changed, so this doesn't spam the undo stack on no-op drags.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extracts the form builder’s field property-editor (modal + tab builders + save logic) from form-builder.js into a dedicated module, and fixes multi-step field ordering/undo behaviors so the live preview and DOM data-field-index attributes stay consistent after add/move/reorder operations.
Changes:
- Extract property-editor methods into
form-builder-property-editor.jsand mix them intoFormBuilder.prototype. - Fix multi-step ordering by re-syncing
this.fieldstoformStepsand re-rendering all steps after add/move/reorder; add undo snapshots for these operations. - Add/adjust Vitest coverage for the extracted property-editor and for the multi-step ordering/index-sync regressions.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests_js/form-builder/updateFieldOrderInStep.test.js | Adds regression coverage for syncing this.fields order and pushing undo snapshots when reordering within a step. |
| tests_js/form-builder/multiStepFieldIndexSync.test.js | New regression tests to ensure cross-step operations don’t leave stale data-field-index mappings or corrupt state. |
| tests_js/form-builder/initializePropertyFormTabs.test.js | Removes tests that were moved to the new property-editor module test suite. |
| tests_js/form-builder/handleFieldMovedToStep.test.js | New tests for cross-step moves, this.fields reorder sync, and undo snapshot behavior. |
| tests_js/form-builder/handleFieldDroppedToStep.test.js | Adds tests for correct insertion ordering + property editor opening after this.fields reorder. |
| tests_js/form-builder-property-editor/propertyEditorMethods.test.js | New comprehensive tests for extracted property-editor methods, including XSS escaping for field_type. |
| django_forms_workflows/static/django_forms_workflows/js/form-builder.js | Imports/mixes in the extracted property-editor, and updates multi-step handlers to reorder + re-render correctly and push undo snapshots. |
| django_forms_workflows/static/django_forms_workflows/js/form-builder-property-editor.js | New module containing the extracted property-editor implementation. |
Suppressed comments (3)
django_forms_workflows/static/django_forms_workflows/js/form-builder-property-editor.js:637
- saveFieldProperties pairs operators/values to
.condition-fieldrows using the NodeList iteration index, but the DOM rows are keyed bydata-index. If a row is deleted or indices are not contiguous, this can read the wrong operator/value (or throw). Useel.dataset.indexfor the lookup key.
document.querySelectorAll('.condition-field').forEach((el, index) => {
const fieldName = el.value;
const operator = document.querySelector(`.condition-operator[data-index="${index}"]`).value;
const value = document.querySelector(`.condition-value[data-index="${index}"]`).value;
django_forms_workflows/static/django_forms_workflows/js/form-builder-property-editor.js:672
- saveFieldProperties uses the NodeList iteration index to look up
.validation-value/.validation-messagebydata-index. Deleting/reordering rows can desync these, causing wrong values to be saved. Useel.dataset.indexas the lookup key.
document.querySelectorAll('.validation-type').forEach((el, index) => {
const type = el.value;
const value = document.querySelector(`.validation-value[data-index="${index}"]`)?.value;
const message = document.querySelector(`.validation-message[data-index="${index}"]`)?.value;
django_forms_workflows/static/django_forms_workflows/js/form-builder-property-editor.js:697
- saveFieldProperties pairs
.dependency-sourcerows to.dependency-endpointinputs using the NodeList iteration index, but the UI keys rows bydata-index. If a dependency row is removed, this can read the wrong endpoint. Useel.dataset.indexfor the lookup key.
document.querySelectorAll('.dependency-source').forEach((el, index) => {
const sourceField = el.value;
const endpoint = document.querySelector(`.dependency-endpoint[data-index="${index}"]`)?.value;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+472
to
+476
| // Add event listener for add button | ||
| const btnAdd = document.getElementById('btnAddValidation'); | ||
| if (btnAdd) { | ||
| btnAdd.addEventListener('click', () => this.addValidationRuleRow({}, rules.length)); | ||
| } |
Comment on lines
+528
to
+532
| // Add event listener for add button | ||
| const btnAdd = document.getElementById('btnAddDependency'); | ||
| if (btnAdd) { | ||
| btnAdd.addEventListener('click', () => this.addDependencyRow({}, dependencies.length)); | ||
| } |
Comment on lines
+409
to
+413
| // Add event listener for add button | ||
| const btnAdd = document.getElementById('btnAddCondition'); | ||
| if (btnAdd) { | ||
| btnAdd.addEventListener('click', () => this.addConditionRow({}, conditions.length)); | ||
| } |
Contributor
|
I am trying out this co-pilot feature where it fixes its recommendations in one batch -- stand by. |
matteius
approved these changes
Aug 3, 2026
matteius
pushed a commit
that referenced
this pull request
Aug 5, 2026
…single-step canvas setupCanvas's Sortable onAdd (existing-field-moved branch) and onUpdate handlers mutated this.fields directly without calling pushUndo() first, so Ctrl+Z couldn't undo a single-step canvas drag-reorder. PR #12 fixed the equivalent gap for cross-step move/reorder (handleFieldMovedToStep/updateFieldOrderInStep) but never touched setupCanvas - this was flagged as still-open in the module-split plan. (addFieldAtPosition already pushed its own undo snapshot, so the palette-drop branch of onAdd was unaffected.)
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
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.
Extracts the property-editor code out of form-builder.js and fixes a few small bugs:
field_typebefore rendering it into the property editor