Fix the order of submit field in some cases#2543
Conversation
WalkthroughCentralizes last-row field ordering: server-side ordering is delegated to FrmSubmitHelper which updates DB orders and emits a JSON hidden input. The add-field view prints that input. Admin JS includes last_row_field_ids for non-repeaters and consumes the hidden JSON to sync per-field Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant AdminJS as Admin JS (builder)
participant Server as PHP (FrmFieldsHelper / FrmSubmitHelper)
participant View as add_field.php
participant DOM as Builder DOM
User->>AdminJS: Add field (drag/drop or insert)
AdminJS->>AdminJS: getInsertNewFieldArgs()
alt Non-repeater
AdminJS->>AdminJS: include last_row_field_ids
else Repeater
AdminJS->>AdminJS: omit last_row_field_ids
end
AdminJS->>Server: Request add field (args)
Server->>Server: FrmSubmitHelper::update_last_row_fields_order_when_adding_field()
note right of Server: reads POST.last_row_field_ids, updates DB field_order, stores orders
Server->>View: Render add_field.php
View->>View: FrmSubmitHelper::print_last_row_fields_order_input()
View-->>AdminJS: Response (markup + hidden input frm-last-row-fields-order)
AdminJS->>DOM: Insert new field markup
AdminJS->>AdminJS: parse frm-last-row-fields-order JSON
AdminJS->>DOM: updateLastRowFieldsOrder -> set field_order_<id> inputs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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.
Actionable comments posted: 2
🧹 Nitpick comments (3)
js/src/admin/admin.js (3)
1618-1634: Robustness: coerce IDs to strings and skip sending empty arraysLogic is sound. Minor hardening:
- Coerce DOM/form IDs to strings to avoid false negatives on numeric vs string comparisons.
- Don’t send last_row_field_ids when empty.
- // Only send last row field IDs to update their order if this field isn't added to a repeater. - const isInRepeater = sectionId > 0 && document.getElementById( 'form_id' ).value !== formId; - if ( ! isInRepeater ) { - fieldArgs.last_row_field_ids = getFieldIdsInSubmitRow(); - } + // Only send last row field IDs to update their order if this field isn't added to a nested form (e.g., repeater). + const rootFormEl = document.getElementById( 'form_id' ); + const isNestedForm = rootFormEl && String( rootFormEl.value ) !== String( formId ); + if ( ! isNestedForm ) { + const lastRowIds = getFieldIdsInSubmitRow(); + if ( lastRowIds.length ) { + fieldArgs.last_row_field_ids = lastRowIds; + } + }
1763-1776: Make submit-row ID collection safer and more conciseFilter out unexpected nodes and undefined IDs to avoid leaking empty values.
- function getFieldIdsInSubmitRow() { - const submitField = document.querySelector( '.edit_field_type_submit' ); - if ( ! submitField ) { - return []; - } - - const lastRowFields = submitField.parentNode.children; - const ids = []; - for ( let i = 0; i < lastRowFields.length; i++ ) { - ids.push( lastRowFields[ i ].dataset.fid ); - } - - return ids; - } + function getFieldIdsInSubmitRow() { + const submitField = document.querySelector( '.edit_field_type_submit' ); + if ( ! submitField ) { + return []; + } + return Array.from( submitField.parentNode.children ) + .map( el => el?.dataset?.fid ) + .filter( Boolean ); + }
2733-2737: Guard JSON parsing and input type to avoid runtime errors
- JSON.parse can throw if the value is malformed; add a try/catch.
- Ensure fieldsOrder is a plain object (not array/null) before iterating.
- const lastRowOrderInput = field.querySelector( '#frm-last-row-fields-order' ); - if ( lastRowOrderInput ) { - updateLastRowFieldsOrder( JSON.parse( lastRowOrderInput.value ) ); - } + const lastRowOrderInput = field.querySelector( '#frm-last-row-fields-order' ); + if ( lastRowOrderInput ) { + try { + const fieldsOrder = JSON.parse( lastRowOrderInput.value ); + updateLastRowFieldsOrder( fieldsOrder ); + } catch ( e ) { + // ignore invalid JSON; no-op + } + }- function updateLastRowFieldsOrder( fieldsOrder ) { - if ( ! fieldsOrder || 'object' !== typeof fieldsOrder ) { - return; - } + function updateLastRowFieldsOrder( fieldsOrder ) { + if ( ! fieldsOrder || 'object' !== typeof fieldsOrder || Array.isArray( fieldsOrder ) ) { + return; + } Object.keys( fieldsOrder ).forEach( fieldId => { const orderInput = document.querySelector( 'input[name="field_options[field_order_' + fieldId + ']"]' ); if ( orderInput ) { orderInput.value = fieldsOrder[ fieldId ]; } - }); + } ); }Also applies to: 2773-2784
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
classes/helpers/FrmFieldsHelper.php(1 hunks)classes/helpers/FrmSubmitHelper.php(2 hunks)classes/views/frm-forms/add_field.php(1 hunks)css/admin/frm-settings-components.css(1 hunks)css/frm_testing_mode.css(1 hunks)js/addons-page.js(1 hunks)js/formidable-settings-components.js(1 hunks)js/formidable_blocks.js(1 hunks)js/formidable_dashboard.js(1 hunks)js/formidable_overlay.js(1 hunks)js/formidable_styles.js(1 hunks)js/frm_testing_mode.js(1 hunks)js/onboarding-wizard.js(1 hunks)js/src/admin/admin.js(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
js/onboarding-wizard.js (2)
js/src/admin/admin.js (44)
t(692-692)t(717-717)t(8957-8957)t(10143-10143)e(8176-8176)i(438-438)i(1771-1771)i(2249-2249)i(2993-2993)i(3008-3008)i(3080-3080)i(3098-3098)i(3112-3115)i(3138-3146)i(3176-3176)i(3185-3185)i(3446-3452)i(3567-3570)i(5392-5392)i(5461-5462)i(5536-5538)a(3056-3056)c(697-697)c(722-722)c(7214-7214)c(9033-9033)s(274-274)s(8182-8182)s(8933-8933)s(9010-9010)f(8972-8972)p(8118-8118)p(8133-8133)v(3352-3352)v(3372-3372)v(6851-6851)v(7218-7218)v(7769-7769)v(7785-7785)v(7801-7801)w(9506-9506)frmDom(239-239)frmDom(240-240)frmDom(241-241)js/src/admin/addon-state.js (3)
a(200-200)frmDom(3-3)ajaxurl(12-12)
classes/views/frm-forms/add_field.php (1)
classes/helpers/FrmSubmitHelper.php (2)
FrmSubmitHelper(16-246)print_last_row_fields_order_input(234-245)
js/frm_testing_mode.js (1)
js/src/frm_testing_mode.js (1)
frmDom(34-34)
js/formidable_blocks.js (1)
js/src/admin/addon-state.js (2)
a(200-200)ajaxurl(12-12)
classes/helpers/FrmSubmitHelper.php (2)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-4617)get_post_param(682-692)classes/models/FrmField.php (2)
FrmField(6-1501)update(621-693)
js/formidable-settings-components.js (1)
js/src/admin/admin.js (45)
e(8176-8176)t(692-692)t(717-717)t(8957-8957)t(10143-10143)i(438-438)i(1771-1771)i(2249-2249)i(2993-2993)i(3008-3008)i(3080-3080)i(3098-3098)i(3112-3115)i(3138-3146)i(3176-3176)i(3185-3185)i(3446-3452)i(3567-3570)i(5392-5392)i(5461-5462)i(5536-5538)a(3056-3056)s(274-274)s(8182-8182)s(8933-8933)s(9010-9010)c(697-697)c(722-722)c(7214-7214)c(9033-9033)f(8972-8972)v(3352-3352)v(3372-3372)v(6851-6851)v(7218-7218)v(7769-7769)v(7785-7785)v(7801-7801)p(8118-8118)p(8133-8133)frmDom(239-239)frmDom(240-240)frmDom(241-241)wp(258-258)w(9506-9506)
classes/helpers/FrmFieldsHelper.php (1)
classes/helpers/FrmSubmitHelper.php (2)
FrmSubmitHelper(16-246)update_last_row_fields_order_when_adding_field(210-227)
🪛 Biome (2.1.2)
js/onboarding-wizard.js
[error] 2-2: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
[error] 2-2: Unsafe usage of 'throw'.
'throw' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.
(lint/correctness/noUnsafeFinally)
[error] 2-2: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
[error] 2-2: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
[error] 2-2: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
[error] 2-2: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
[error] 2-2: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
[error] 2-2: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 2-2: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 2-2: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 2-2: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 2-2: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 2-2: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 2-2: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 2-2: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 2-2: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 2-2: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 2-2: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 2-2: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 2-2: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 2-2: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 2-2: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 2-2: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 2-2: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 2-2: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
js/frm_testing_mode.js
[error] 2-2: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
[error] 2-2: Do not use the t variable name as a label
The variable is declared here
Creating a label with the same name as an in-scope variable leads to confusion.
(lint/suspicious/noLabelVar)
[error] 2-2: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 2-2: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 2-2: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 2-2: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 2-2: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
js/formidable_blocks.js
[error] 1-1: Unsafe usage of 'return'.
'return' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.
(lint/correctness/noUnsafeFinally)
[error] 1-1: Unsafe usage of 'throw'.
'throw' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.
(lint/correctness/noUnsafeFinally)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
js/addons-page.js
[error] 1-1: Unsafe usage of 'throw'.
'throw' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.
(lint/correctness/noUnsafeFinally)
[error] 1-1: Unsafe usage of 'return'.
'return' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.
(lint/correctness/noUnsafeFinally)
[error] 1-1: Unsafe usage of 'throw'.
'throw' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.
(lint/correctness/noUnsafeFinally)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: Do not use the t variable name as a label
The variable is declared here
Creating a label with the same name as an in-scope variable leads to confusion.
(lint/suspicious/noLabelVar)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
js/formidable_dashboard.js
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
js/formidable_overlay.js
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
css/frm_testing_mode.css
[error] 3-3: Unexpected shorthand property background after background-color
(lint/suspicious/noShorthandPropertyOverrides)
[error] 3-3: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
text-align is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
[error] 3-3: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
border-radius is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
js/formidable-settings-components.js
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
js/formidable_styles.js
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: Do not use the t variable name as a label
The variable is declared here
Creating a label with the same name as an in-scope variable leads to confusion.
(lint/suspicious/noLabelVar)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
🔇 Additional comments (12)
css/admin/frm-settings-components.css (1)
1-1: LGTM! Build artifact with no functional changes.This appears to be a compiled/minified CSS file. The AI summary confirms no observable functional changes in this file.
js/formidable_overlay.js (1)
1-1: LGTM! Formatting-only changes in build artifact.This minified JavaScript file contains only stylistic formatting adjustments with no behavioral changes, as confirmed by the AI summary.
js/addons-page.js (1)
1-1: LGTM! Internal module ID updates.The changes update internal module references (616→8616, 604→7604) as part of a coordinated bundler update. No functional changes to the code logic.
js/formidable_styles.js (1)
1-1: LGTM! Internal module ID updates.Consistent with js/addons-page.js, this file updates internal module IDs (616→8616, 604→7604) with no functional changes.
js/onboarding-wizard.js (1)
2-2: LGTM! Internal refactoring with no behavioral changes.This minified file contains internal helper name refactoring and formatting adjustments. The AI summary confirms no observable behavior, initialization, or event wiring changes.
js/formidable-settings-components.js (1)
1-1: LGTM! Syntax formatting changes only.This minified file contains minor formatting and wrapper syntax changes with no functional behavior or public API modifications.
js/formidable_dashboard.js (1)
1-1: LGTM! Minor callback wrapping refactor.This minified file shows minor refactoring with extra parentheses wrapping around callbacks. The AI summary confirms no changes to control flow or behavior.
css/frm_testing_mode.css (1)
3-3: LGTM! Restores proper CSS comment boundary.The change correctly reintroduces the comment terminator
*/before the tooltip CSS block, restoring proper syntax. The static analysis hints about duplicate properties and shorthand overrides can be safely ignored as this appears to be minified/compiled CSS where such patterns are acceptable.js/formidable_blocks.js (1)
1-1: Build artifact with automated identifier regeneration—no source changes to review.This file is a minified JavaScript bundle where only module IDs (532→2532) and CSS class/keyframe hashes have changed. These are automatic outputs from the build toolchain, not manual code changes. The PR's actual logic for fixing submit field order resides in other files (PHP and admin JS source).
Static analysis errors are false positives from parsing minified code and can be safely ignored.
classes/views/frm-forms/add_field.php (1)
15-19: LGTM!The call to
FrmSubmitHelper::print_last_row_fields_order_input()is appropriately placed before thefrm_extra_field_actionshook, allowing the hidden input to be output before custom actions are invoked. The centralized helper method ensures proper encapsulation of the last-row field ordering logic.classes/helpers/FrmFieldsHelper.php (1)
45-46: LGTM!The refactoring correctly centralizes last-row field ordering logic into
FrmSubmitHelper::update_last_row_fields_order_when_adding_field(). This improves separation of concerns and makes the code more maintainable.classes/helpers/FrmSubmitHelper.php (1)
32-37: LGTM!The private static property
$last_row_fields_orderis appropriately used to track last-row field orders during the request lifecycle. The PHPDoc clearly describes its purpose and structure.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2543 +/- ##
============================================
- Coverage 27.35% 27.20% -0.15%
- Complexity 8668 8728 +60
============================================
Files 142 143 +1
Lines 29228 29417 +189
============================================
+ Hits 7995 8004 +9
- Misses 21233 21413 +180 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
js/src/admin/admin.js (3)
1618-1634: Gate last_row_field_ids for section (divider) too, or confirm server ignores itGood call excluding repeaters. The PR objective also mentions “section”; unless the server already ignores last_row_field_ids when field_type === 'divider', we should gate it client‑side too to avoid unintended last‑row updates.
Suggested tweak:
- const isInRepeater = sectionId > 0 && document.getElementById( 'form_id' ).value !== formId; - if ( ! isInRepeater ) { + const currentFormId = String( document.getElementById( 'form_id' ).value ); + const isInRepeater = sectionId > 0 && String( formId ) !== currentFormId; + const isSectionField = fieldType === 'divider'; + if ( ! isInRepeater && ! isSectionField ) { fieldArgs.last_row_field_ids = getFieldIdsInSubmitRow(); }Please confirm server-side FrmSubmitHelper ignores last_row_field_ids for divider fields; if not, apply the above.
2733-2737: Harden JSON parsing and clean up the hidden inputParsing without a guard can break the builder on malformed input. Also remove the consumed input to avoid stale DOM.
- const lastRowOrderInput = field.querySelector( '#frm-last-row-fields-order' ); - if ( lastRowOrderInput ) { - updateLastRowFieldsOrder( JSON.parse( lastRowOrderInput.value ) ); - } + const lastRowOrderInput = field.querySelector( '#frm-last-row-fields-order' ); + if ( lastRowOrderInput ) { + try { + const order = JSON.parse( lastRowOrderInput.value ); + updateLastRowFieldsOrder( order ); + } catch ( e ) { + // Ignore malformed JSON + } finally { + lastRowOrderInput.remove(); + } + }
2773-2784: Optionally trigger change events after updating valuesIf any listeners depend on change events for field_order inputs, consider firing them after assignment.
if ( orderInput ) { orderInput.value = fieldsOrder[ fieldId ]; + if ( window.jQuery ) { + jQuery( orderInput ).trigger( 'change' ); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/src/admin/admin.js(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
- GitHub Check: Run PHP Syntax inspection (8.3)
lauramekaj1
left a comment
lauramekaj1
left a comment
There was a problem hiding this comment.
@truongwp @Crabcyborg I tested it and it's working as expected. Thank you!
|
Thank you Laura! 🙌 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
js/src/admin/admin.js (1)
2773-2784: Minor: consider triggering change/events after updating inputs and/or exposing API.Setting field_options[field_order_*] silently may bypass listeners. Optionally trigger a change to keep any watchers in sync, and expose updateLastRowFieldsOrder via the returned module if external callers/tests need it.
Example:
- if ( orderInput ) { - orderInput.value = fieldsOrder[ fieldId ]; - } + if ( orderInput ) { + orderInput.value = fieldsOrder[ fieldId ]; + jQuery( orderInput ).trigger( 'change' ); + }If public exposure is intended, add updateLastRowFieldsOrder to the returned object at the bottom.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/src/admin/admin.js(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
3 participants
Fixes https://github.com/Strategy11/formidable-pro/issues/6027
Besides the first case in the issue, I found the second case that causes this issue (screencast below):
Screen.Recording.2025-10-15.at.16.44.23.mov
For the first case, when adding a field to a repeater or section, it has a different field order, we should not update the submit row fields order.
For the second case, when showing the submit settings, it has the old field_order input value, which is used when updating the form.