feat: add Page Break field type for multistep forms#150
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 21 minutes and 7 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughAdds a Page Break field type end-to-end: backend mapping and validation updates; frontend PageBreak component, field-type registry, builder and submission multi-step UI; utilities/composables/stores for steps and layout compaction; e2e helpers/specs; and unit/integration tests for validation and DocType sync. ChangesPage Break Field Type Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
forms_pro/tests/test_page_break.py (1)
8-50: ⚡ Quick winConsider strengthening assertions to verify DocType sync.
The test currently only asserts that
"Page Break"appears inform.fieldsafter reload (line 50). While this confirms form-level persistence, it doesn't verify that the Page Break field synced correctly to the linked DocType with the expectedHTMLfieldtype and<hr>options—unliketest_page_break_syncs_as_html_to_linked_doctypewhich does verify the sync contract.Consider adding assertions similar to lines 68-72 in the second test to confirm the full persistence chain in this multi-field scenario as well.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forms_pro/tests/test_page_break.py` around lines 8 - 50, The test test_form_with_page_break_saves only checks that "Page Break" exists in form.fields after reload; enhance it to also verify the linked DocType sync by locating the generated DocType for the form (after form.save() / form.reload()) and asserting that the corresponding field was created there with fieldtype "HTML" and options "<hr>" (mirror the assertions in test_page_break_syncs_as_html_to_linked_doctype). Update the test to fetch the linked DocType (using the same lookup used in the other test), find the field by fieldname (e.g., "step_2"), and assert its fieldtype and options match the expected synced values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@forms_pro/tests/test_page_break.py`:
- Around line 8-50: The test test_form_with_page_break_saves only checks that
"Page Break" exists in form.fields after reload; enhance it to also verify the
linked DocType sync by locating the generated DocType for the form (after
form.save() / form.reload()) and asserting that the corresponding field was
created there with fieldtype "HTML" and options "<hr>" (mirror the assertions in
test_page_break_syncs_as_html_to_linked_doctype). Update the test to fetch the
linked DocType (using the same lookup used in the other test), find the field by
fieldname (e.g., "step_2"), and assert its fieldtype and options match the
expected synced values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 77184cf1-2c0f-4988-92fa-a547e859caeb
📒 Files selected for processing (5)
forms_pro/api/submission/test_submission_validation.pyforms_pro/forms_pro/doctype/form_field/form_field.jsonforms_pro/forms_pro/doctype/form_field/form_field.pyforms_pro/tests/test_page_break.pyfrontend/src/types/FormsPro/form_field.types.ts
Tab Break is a native Frappe layout field that renders proper tabs in Desk view, replacing the raw HTML <hr> workaround.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
forms_pro/forms_pro/doctype/form_field/form_field.py (1)
59-82:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate the auto-generated
DF.Literalunion to includePage Break.The runtime map supports
"Page Break", but the TYPE_CHECKINGfieldtypeliteral list does not. This creates a cross-layer type contract mismatch (editor/type-check errors despite valid runtime value). Please regenerate this block so"Page Break"is present in the union.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forms_pro/forms_pro/doctype/form_field/form_field.py` around lines 59 - 82, The DF.Literal type list for the fieldtype attribute is missing "Page Break", causing a type-check mismatch; update/regenerate the DF.Literal union used by the fieldtype variable (the literal block assigned to fieldtype in form_field.py) to include "Page Break" alongside the other entries so the TYPE_CHECKING literal matches the runtime map.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@forms_pro/forms_pro/doctype/form_field/form_field.py`:
- Around line 59-82: The DF.Literal type list for the fieldtype attribute is
missing "Page Break", causing a type-check mismatch; update/regenerate the
DF.Literal union used by the fieldtype variable (the literal block assigned to
fieldtype in form_field.py) to include "Page Break" alongside the other entries
so the TYPE_CHECKING literal matches the runtime map.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 07154101-4eaa-42aa-be7f-dd4dd3ec8d6b
📒 Files selected for processing (3)
forms_pro/api/submission/test_submission_validation.pyforms_pro/forms_pro/doctype/form_field/form_field.pyforms_pro/tests/test_page_break.py
🚧 Files skipped from review as they are similar to previous changes (1)
- forms_pro/tests/test_page_break.py
This reverts commit b34cec1.
Add Page Break to fieldTypes registry, FieldRenderer, form_fields utils, and hide it from the sidebar. Filter it out in submission field display.
Add section navigation bar for multi-page forms. Sections are derived from Page Break fields. Supports renaming (double-click), adding, and removing sections with field merge or deletion options.
Add step indicator and Next/Back navigation for multi-page form submissions. Validates current step fields before advancing.
- Move step state (useFormSteps) into submissionForm store; FormRenderer reads steps/navigation off the store instead of the composable - Add stepDirection (forward/backward) to drive slide animations - Redesign StepIndicator as numbered-circle stepper with animated check and progress-fill connectors - Compact step header on step 2+, full header on step 1 - Footer progress pills + Back/Next/Submit/Draft buttons; success fade-up - Rename builder terminology page -> Step (addSection -> addStep)
Replace hardcoded Tailwind grays with surface/ink/outline tokens so the multi-step submission UI respects theming. Kept the translucent halo ring on the active step (no opaque token equivalent).
Drop the fixed percentage stepper width that starved nodes of space and collapsed connectors. Use a scrollable w-max/mx-auto row with fixed-width connectors, bounded nodes, and truncated labels so the stepper centers when it fits and scrolls cleanly when many steps overflow narrow screens.
User-facing labels and dialog text only. Default page-break labels now read "Step N", and the remove-step dialog and nav aria-labels follow. Internal symbols and section_* fieldnames unchanged.
Step grouping was duplicated between editForm store and useFormSteps composable and had drifted (label fallback, empty-form behavior). Both now consume one parameterized groupFieldsIntoSteps core. - form_steps.ts: grouping, page-break index helpers, step mutations - form_fields.ts: compact/lastRowIndex/scrubFieldname (now take fields param instead of reading the store resource) - editForm store keeps state, persistence, and thin action wrappers; step actions renamed removeSection* -> removeStep*, renameSection -> renameStep to finish the Section->Step migration
Add Playwright specs for step navigation in the builder (add/rename/ remove/persist) and the public multi-step submission flow (next/back, value retention, done/current/todo stepper, conditional across steps, guest path). Supporting changes to make flows testable + correct: - SectionNavBar: fix Escape during rename leaking into the next save; add aria-label "Remove step" and "Add Step" labels - StepIndicator: emit data-step-* hooks for stable step assertions - submission helper: resolve field inputs via label sibling Two known limitations pinned as test.fixme: getActiveStepEndIndex leading-PageBreak offset, and REST seeding of empty-label Page Breaks.
Address review findings on the multi-step builder: - SectionNavBar: replace the nested <X role="button"> (mouse-only, interactive element inside <Button>) with a real frappe-ui ghost icon Button rendered as a sibling of the step tab. Now focusable and Enter/Space activatable, and no longer invalid nested-interactive HTML. - SectionNavBar: reset cancelRename in startRename so an Escape whose @blur never fires (DOM removal doesn't reliably blur) can't leak into and silently drop the next rename. - e2e: seed helper now asserts the Form PUT succeeded, failing loudly instead of as a downstream locator timeout. - e2e: scope the remove control via the new [data-step-tab] group since it is no longer a child of the tab button.
Keep the remove "X" inside the step tab's suffix slot so it shares the tab's pill background instead of floating beside it. Keyboard access is handled on the icon itself: tabindex, Enter/Space activation, and a focus-visible ring. Nesting changes the tab's accessible name to "<label> Remove step", so the e2e stepTab locator now matches the label with that suffix optional.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/stores/editForm.ts (1)
248-266:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInconsistent behavior:
addFieldFromDoctypeignores active section.
addFieldFromDoctypealways appends to the end of the form usinglastRowIndex(fs) + 1andfs.push(), regardless ofactiveSectionIndex. This differs fromaddField, which inserts into the active section.User impact: When editing Section 2 in a multi-step form, clicking "Add Field" inserts into Section 2, but importing a field from the linked DocType appends it to the very end of the form (potentially in a different section). This breaks user expectations.
♻️ Align with addField behavior
function addFieldFromDoctype(field: any) { if (!formResource.value?.doc) return; const fs: FormField[] = formResource.value.doc.fields; + const insertAt = getActiveStepEndIndex( + fs, + activeSectionIndex.value, + isMultiSection.value + ); + + const fieldsBeforeInsert = fs.slice(0, insertAt); + const newRowIndex = + fieldsBeforeInsert.length > 0 + ? Math.max(...fieldsBeforeInsert.map((f) => f.row_index ?? 0)) + 1 + : 0; const _newField: FormField = { idx: fs.length + 1, fieldtype: field.fieldtype, label: field.label, fieldname: field.fieldname, options: field.options, default: field.default, description: field.description, - row_index: lastRowIndex(fs) + 1, + row_index: newRowIndex, column_index: 0, cell_index: 0, }; - fs.push(_newField); + fs.splice(insertAt, 0, _newField); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/stores/editForm.ts` around lines 248 - 266, The addFieldFromDoctype function appends new fields to the very end of fields (using lastRowIndex(fs)+1 and fs.push) ignoring the current activeSectionIndex; change it to mirror addField: compute the insertion row based on activeSectionIndex (e.g., determine the starting/ending row range for that section or use a helper that addField uses), set row_index to a row inside the active section, set column_index and cell_index consistently with addField, and insert the new FormField into formResource.value.doc.fields using splice at the correct position instead of fs.push; update any indexes or re-normalization logic the addField path uses so the new field appears in the active section.
🧹 Nitpick comments (2)
frontend/src/components/builder/SectionNavBar.vue (1)
87-159: ⚡ Quick winIcon prop inconsistency between "Add Step" buttons.
Line 94 uses
:icon-left="Plus"while line 147 uses:icon="Plus", but both buttons havelabel="Add Step". For visual consistency, both should use the same icon prop. Theicon-leftprop is typically preferred when a label is present to ensure consistent left-aligned icon placement.🎨 Proposed fix for consistency
<Button variant="ghost" size="sm" label="Add Step" - :icon="Plus" + :icon-left="Plus" aria-label="Add Step" tooltip="Click to add a new step to the form" `@click`="store.addStep()"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/builder/SectionNavBar.vue` around lines 87 - 159, Two "Add Step" Button usages are inconsistent: one uses :icon-left="Plus" (first Button) while the other uses :icon="Plus" (second Button). Update the second Button (the one with label="Add Step" near the end of the template) to use :icon-left="Plus" so both buttons use the same icon prop and preserve consistent left-aligned icon placement; verify the Button component still receives tooltip, aria-label and `@click` handlers unchanged.frontend/src/components/submission/FormRenderer.vue (1)
49-49: 💤 Low valueMinor: Form animates on initial page load.
The
:keyand:class="animClass"trigger a slide-in animation even on the first render, becausestepDirectioninitializes to"forward"inuseFormSteps. Users will see the form slide in from the right when the page first loads, which may feel odd—animations should ideally apply only during navigation between steps.🎨 Possible fix: skip animation on initial render
Option 1: Track whether this is the first render in the component:
const animClass = computed(() => { + if (!hasNavigated.value) return ""; return store.stepDirection === "forward" ? "anim-forward" : "anim-back"; }); + +const hasNavigated = ref(false); +watch(() => store.currentStepIndex, () => { + hasNavigated.value = true; +});Option 2: Update
useFormStepsto initializestepDirectionasref<"forward" | "backward" | null>(null)and set it only during navigation, then adjustanimClassto return""when direction is null.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/submission/FormRenderer.vue` at line 49, The slide-in animation runs on initial load because stepDirection in useFormSteps is initialized to "forward" and animClass (used on the div with :key="store.currentStepIndex" and :class="animClass") applies the transition; fix by preventing animations on first render either by changing useFormSteps.stepDirection to ref<"forward"|"backward"|null>(null) and updating the animClass computed to return an empty string when stepDirection is null, or by adding an isInitialRender flag in FormRenderer.vue (set true then flipped after mount) and have animClass return "" while isInitialRender is true so the animation only occurs on subsequent step navigations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/stores/editForm.ts`:
- Around line 217-246: The addField function computes newRowIndex from
activeSectionFields which yields 0 for empty non-first sections and causes
row_index collisions; fix by deriving newRowIndex from the full fields array
insertion point (use getActiveStepEndIndex(fs, activeSectionIndex.value,
isMultiSection.value) to determine where the new field lands and compute
newRowIndex as the previous row_index at that insertion position + 1 or the max
row_index within the preceding rows), or simply call compact(fs) immediately
after fs.splice(insertAt, 0, newField) to renumber
row_index/column_index/cell_index consistently; update addField (and reference
activeSectionFields, newRowIndex, getActiveStepEndIndex, compact) accordingly.
In `@frontend/src/utils/form_steps.ts`:
- Around line 202-228: renameStep currently inserts a leading Page Break for
index 0 with row_index: 0 without shifting existing fields, which creates
duplicate (row_index, column_index, cell_index) tuples; update renameStep to
first offset all existing fields' row_index (and if applicable their row-related
indices) by +1 (e.g., loop over fields and increment row_index) before
performing fields.unshift, or alternatively insert then call the existing
compact() routine to recompute/normalize grid coordinates; ensure the new Page
Break carries newLabel and that you update/retain any references used by
findStepPageBreakIndex and downstream layout logic.
---
Outside diff comments:
In `@frontend/src/stores/editForm.ts`:
- Around line 248-266: The addFieldFromDoctype function appends new fields to
the very end of fields (using lastRowIndex(fs)+1 and fs.push) ignoring the
current activeSectionIndex; change it to mirror addField: compute the insertion
row based on activeSectionIndex (e.g., determine the starting/ending row range
for that section or use a helper that addField uses), set row_index to a row
inside the active section, set column_index and cell_index consistently with
addField, and insert the new FormField into formResource.value.doc.fields using
splice at the correct position instead of fs.push; update any indexes or
re-normalization logic the addField path uses so the new field appears in the
active section.
---
Nitpick comments:
In `@frontend/src/components/builder/SectionNavBar.vue`:
- Around line 87-159: Two "Add Step" Button usages are inconsistent: one uses
:icon-left="Plus" (first Button) while the other uses :icon="Plus" (second
Button). Update the second Button (the one with label="Add Step" near the end of
the template) to use :icon-left="Plus" so both buttons use the same icon prop
and preserve consistent left-aligned icon placement; verify the Button component
still receives tooltip, aria-label and `@click` handlers unchanged.
In `@frontend/src/components/submission/FormRenderer.vue`:
- Line 49: The slide-in animation runs on initial load because stepDirection in
useFormSteps is initialized to "forward" and animClass (used on the div with
:key="store.currentStepIndex" and :class="animClass") applies the transition;
fix by preventing animations on first render either by changing
useFormSteps.stepDirection to ref<"forward"|"backward"|null>(null) and updating
the animClass computed to return an empty string when stepDirection is null, or
by adding an isInitialRender flag in FormRenderer.vue (set true then flipped
after mount) and have animClass return "" while isInitialRender is true so the
animation only occurs on subsequent step navigations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36c0582e-499d-4d20-9c09-fbcca86b815d
📒 Files selected for processing (23)
.gitignorefrontend/e2e/helpers/form-builder.tsfrontend/e2e/helpers/form-fields.tsfrontend/e2e/helpers/submission.tsfrontend/e2e/specs/builder-steps.spec.tsfrontend/e2e/specs/submission-steps.spec.tsfrontend/src/components/FormBuilderContent.vuefrontend/src/components/builder/FieldRenderer.vuefrontend/src/components/builder/SectionNavBar.vuefrontend/src/components/builder/sidebar/AddFieldsSection.vuefrontend/src/components/fields/PageBreak.vuefrontend/src/components/form/submissions/SubmissionFieldValue.vuefrontend/src/components/submission/FormHeader.vuefrontend/src/components/submission/FormRenderer.vuefrontend/src/components/submission/StepIndicator.vuefrontend/src/components/submission/SuccessSection.vuefrontend/src/composables/useFormSteps.tsfrontend/src/config/fieldTypes.tsfrontend/src/pages/SubmissionPage.vuefrontend/src/stores/editForm.tsfrontend/src/stores/submissionForm.tsfrontend/src/utils/form_fields.tsfrontend/src/utils/form_steps.ts
✅ Files skipped from review due to trivial changes (1)
- .gitignore
Comment on lines
+217
to
246
| function addField(fieldtype: Fieldtype) { | ||
| if (!formResource.value?.doc) return; | ||
| const fs: FormField[] = formResource.value.doc.fields; | ||
|
|
||
| // Renumber cell_index within each (row, column) to 0..K-1 | ||
| const cellMap = new Map<string, FormField[]>(); | ||
| for (const f of fs) { | ||
| const key = `${f.row_index}-${f.column_index}`; | ||
| if (!cellMap.has(key)) cellMap.set(key, []); | ||
| cellMap.get(key)!.push(f); | ||
| } | ||
| for (const cells of cellMap.values()) { | ||
| cells | ||
| .sort((a, b) => (a.cell_index ?? 0) - (b.cell_index ?? 0)) | ||
| .forEach((f, i) => { | ||
| f.cell_index = i; | ||
| }); | ||
| } | ||
| } | ||
| const sectionFields = activeSectionFields.value; | ||
| const newRowIndex = | ||
| sectionFields.length > 0 | ||
| ? Math.max(...sectionFields.map((f) => f.row_index ?? 0)) + 1 | ||
| : 0; | ||
|
|
||
| function lastRowIndex(fs: FormField[]): number { | ||
| return fs.reduce((m, f) => Math.max(m, f.row_index ?? 0), -1); | ||
| } | ||
| const newField: FormField = { | ||
| idx: fs.length + 1, | ||
| fieldtype, | ||
| label: "", | ||
| fieldname: "", | ||
| options: "", | ||
| default: "", | ||
| description: "", | ||
| row_index: newRowIndex, | ||
| column_index: 0, | ||
| cell_index: 0, | ||
| }; | ||
|
|
||
| function addField(fieldtype: Fieldtype) { | ||
| if (formResource.value?.doc) { | ||
| const fs: FormField[] = formResource.value.doc.fields; | ||
|
|
||
| const newField: FormField = { | ||
| idx: fs.length + 1, | ||
| fieldtype, | ||
| label: "", | ||
| fieldname: "", | ||
| options: "", | ||
| default: "", | ||
| description: "", | ||
| row_index: lastRowIndex(fs) + 1, | ||
| column_index: 0, | ||
| cell_index: 0, | ||
| }; | ||
|
|
||
| fs.push(newField); | ||
| } | ||
| const insertAt = getActiveStepEndIndex( | ||
| fs, | ||
| activeSectionIndex.value, | ||
| isMultiSection.value | ||
| ); | ||
| fs.splice(insertAt, 0, newField); | ||
| } |
There was a problem hiding this comment.
Critical: row_index computation breaks for empty sections.
When adding a field to an empty non-first section, newRowIndex is set to 0 (line 225) because sectionFields is empty. This creates a row index collision with earlier sections.
Example scenario:
- Section 1: fields at row_index 0, 1, 2
- Section 2: empty (just a PAGE_BREAK with no fields after it)
- User adds field to Section 2 → newRowIndex = 0 → collision!
The field is inserted at the correct array position (via getActiveStepEndIndex), but its row_index property is wrong. Since addField doesn't call compact(fs), the grid layout will be broken until the user performs another operation that triggers compacting.
🔧 Recommended fix
Compute newRowIndex based on the insertion point in the full fields array, not just the active section:
function addField(fieldtype: Fieldtype) {
if (!formResource.value?.doc) return;
const fs: FormField[] = formResource.value.doc.fields;
- const sectionFields = activeSectionFields.value;
- const newRowIndex =
- sectionFields.length > 0
- ? Math.max(...sectionFields.map((f) => f.row_index ?? 0)) + 1
- : 0;
+ const insertAt = getActiveStepEndIndex(
+ fs,
+ activeSectionIndex.value,
+ isMultiSection.value
+ );
+
+ // Find the last row_index before the insertion point
+ const fieldsBeforeInsert = fs.slice(0, insertAt);
+ const newRowIndex =
+ fieldsBeforeInsert.length > 0
+ ? Math.max(...fieldsBeforeInsert.map((f) => f.row_index ?? 0)) + 1
+ : 0;
const newField: FormField = {
idx: fs.length + 1,
fieldtype,
label: "",
fieldname: "",
options: "",
default: "",
description: "",
row_index: newRowIndex,
column_index: 0,
cell_index: 0,
};
- const insertAt = getActiveStepEndIndex(
- fs,
- activeSectionIndex.value,
- isMultiSection.value
- );
fs.splice(insertAt, 0, newField);
}Alternatively, call compact(fs) after insertion to renumber all indices consistently.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function addField(fieldtype: Fieldtype) { | |
| if (!formResource.value?.doc) return; | |
| const fs: FormField[] = formResource.value.doc.fields; | |
| // Renumber cell_index within each (row, column) to 0..K-1 | |
| const cellMap = new Map<string, FormField[]>(); | |
| for (const f of fs) { | |
| const key = `${f.row_index}-${f.column_index}`; | |
| if (!cellMap.has(key)) cellMap.set(key, []); | |
| cellMap.get(key)!.push(f); | |
| } | |
| for (const cells of cellMap.values()) { | |
| cells | |
| .sort((a, b) => (a.cell_index ?? 0) - (b.cell_index ?? 0)) | |
| .forEach((f, i) => { | |
| f.cell_index = i; | |
| }); | |
| } | |
| } | |
| const sectionFields = activeSectionFields.value; | |
| const newRowIndex = | |
| sectionFields.length > 0 | |
| ? Math.max(...sectionFields.map((f) => f.row_index ?? 0)) + 1 | |
| : 0; | |
| function lastRowIndex(fs: FormField[]): number { | |
| return fs.reduce((m, f) => Math.max(m, f.row_index ?? 0), -1); | |
| } | |
| const newField: FormField = { | |
| idx: fs.length + 1, | |
| fieldtype, | |
| label: "", | |
| fieldname: "", | |
| options: "", | |
| default: "", | |
| description: "", | |
| row_index: newRowIndex, | |
| column_index: 0, | |
| cell_index: 0, | |
| }; | |
| function addField(fieldtype: Fieldtype) { | |
| if (formResource.value?.doc) { | |
| const fs: FormField[] = formResource.value.doc.fields; | |
| const newField: FormField = { | |
| idx: fs.length + 1, | |
| fieldtype, | |
| label: "", | |
| fieldname: "", | |
| options: "", | |
| default: "", | |
| description: "", | |
| row_index: lastRowIndex(fs) + 1, | |
| column_index: 0, | |
| cell_index: 0, | |
| }; | |
| fs.push(newField); | |
| } | |
| const insertAt = getActiveStepEndIndex( | |
| fs, | |
| activeSectionIndex.value, | |
| isMultiSection.value | |
| ); | |
| fs.splice(insertAt, 0, newField); | |
| } | |
| function addField(fieldtype: Fieldtype) { | |
| if (!formResource.value?.doc) return; | |
| const fs: FormField[] = formResource.value.doc.fields; | |
| const insertAt = getActiveStepEndIndex( | |
| fs, | |
| activeSectionIndex.value, | |
| isMultiSection.value | |
| ); | |
| // Find the last row_index before the insertion point | |
| const fieldsBeforeInsert = fs.slice(0, insertAt); | |
| const newRowIndex = | |
| fieldsBeforeInsert.length > 0 | |
| ? Math.max(...fieldsBeforeInsert.map((f) => f.row_index ?? 0)) + 1 | |
| : 0; | |
| const newField: FormField = { | |
| idx: fs.length + 1, | |
| fieldtype, | |
| label: "", | |
| fieldname: "", | |
| options: "", | |
| default: "", | |
| description: "", | |
| row_index: newRowIndex, | |
| column_index: 0, | |
| cell_index: 0, | |
| }; | |
| fs.splice(insertAt, 0, newField); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/stores/editForm.ts` around lines 217 - 246, The addField
function computes newRowIndex from activeSectionFields which yields 0 for empty
non-first sections and causes row_index collisions; fix by deriving newRowIndex
from the full fields array insertion point (use getActiveStepEndIndex(fs,
activeSectionIndex.value, isMultiSection.value) to determine where the new field
lands and compute newRowIndex as the previous row_index at that insertion
position + 1 or the max row_index within the preceding rows), or simply call
compact(fs) immediately after fs.splice(insertAt, 0, newField) to renumber
row_index/column_index/cell_index consistently; update addField (and reference
activeSectionFields, newRowIndex, getActiveStepEndIndex, compact) accordingly.
groupFieldsIntoSteps returns no steps for an empty field list, so isLastStep was false on step 0 and FormRenderer showed Next instead of Submit. Pass alwaysIncludeTrailing so there is always >= 1 step.
Runtime map already supported it; the TYPE_CHECKING union lagged behind the doctype JSON options.
Field insertion computed row_index from the active section only, so an empty or middle section produced rows colliding with other sections — moveField and compact() treat row_index as global. Centralize insertion in insertFieldAtStepEnd, which numbers the new row from everything before the insertion point and shifts later rows. Also fixed along the way: - addFieldFromDoctype appended to the form end instead of the active section; it now uses the same helper - getActiveStepEndIndex was off by one step when the form has a leading Page Break (un-fixmes the palette e2e test) - renameStep step 0 inserted a leading Page Break at row 0 without shifting existing fields off that row Pure grid helpers moved to form_layout.ts so vitest can import them without the component registry; form_fields.ts re-exports them.
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
1 participant
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
Page Breakas a new display-only field type (like Heading 1/2/3)HTMLfieldtype, generates<hr>on linked DocTypeChanges
form_field.json— addedPage Breakto fieldtype optionsform_field.py— mapping, display-only set,get_options()returns<hr>test_submission_validation.py— 6 unit teststest_page_break.py— 2 integration testsform_field.types.ts— auto-generated enum memberTest plan
bench run-tests --app forms_pro)🤖 Generated with Claude Code
Summary by CodeRabbit