Skip to content

Conversation

@AbdiTolesa
Copy link
Contributor

@AbdiTolesa AbdiTolesa commented Dec 15, 2025

Fix https://github.com/Strategy11/formidable-pro/issues/5228

CleanShot.2025-12-25.at.21.45.52.mp4

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 15, 2025

Walkthrough

Adds an optional field-group-splitting flow when a field gains the frm_first class and applies multiple cosmetic/minified formatting edits across JS files; also appends duplicated CSS blocks in two stylesheet files.

Changes

Cohort / File(s) Summary
Field-group splitting logic
js/src/admin/admin.js
Adds shouldTryBreakFieldGroup parameter to liveChanges() and changeFieldClass() and introduces maybeBreakFieldGroup() which can clone and split a field group before applying class changes when frm_first is set.
CSS duplication
css/admin/frm-settings-components.css, css/frm_testing_mode.css
Identical CSS blocks appended/duplicated (Formidable settings block and Bootstrap tooltip rules), resulting in redundant style rules at file ends.
Formatting / minified JS edits
js/formidable_dashboard.js, js/formidable_overlay.js, js/formidable-settings-components.js, js/frm_testing_mode.js, js/onboarding-wizard.js, js/welcome-tour.js, js/addons-page.js, js/formidable_styles.js, js/frm_testing_mode.js
Whitespace, wrapper/arrow-style replacements, identifier renames and reformatting in minified bundles; no functional behavior changes.
Export binding rename
js/formidable_blocks.js
Export symbol binding changed (export name updated from A to Ay, internal binding switch from i to l).

Sequence Diagram(s)

sequenceDiagram
    participant AdminUI as Admin UI
    participant Handler as liveChanges / changeFieldClass
    participant Splitter as maybeBreakFieldGroup
    participant DOM as DOM

    AdminUI->>Handler: field class update event
    Handler->>Handler: read shouldTryBreakFieldGroup (default: true)
    Handler->>Splitter: maybeBreakFieldGroup(flag, field, classes, replaceWith)

    alt flag enabled and conditions met
        Splitter->>DOM: clone current field-group
        Splitter->>DOM: move target field into new group
        Splitter->>DOM: update DOM structure
        Splitter->>Handler: split complete
    else no split
        Splitter->>Handler: no action
    end

    Handler->>DOM: apply class replacement to field
    Handler->>AdminUI: emit update complete
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested labels

run tests

Suggested reviewers

  • Crabcyborg
  • shervElmi
  • lauramekaj1

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 1.20% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: adding logic to break a field group when the frm_first class is added to a field, which aligns with the primary functional change in js/src/admin/admin.js.
Description check ✅ Passed The description references issue #5228 and provides a video link demonstrating the fix, which relates to the field group breaking functionality introduced in this PR.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch issue-5228

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
js/src/admin/admin.js (1)

6763-6816: Field-group splitting logic has multiple critical correctness and robustness issues that will cause incorrect behavior and runtime errors.

The changeFieldClass enhancement for splitting a row when frm_first is added has good intent but the implementation is flawed:

  1. Updates the wrong DOM node (detached field)
    The code removes fields from the original row via fieldsInFieldBox[i].remove() for i >= indexOfTargetField. This detaches the original field element from the DOM. However, the final mutation field.className = field.className.replace( replace, replaceWith ) happens on this detached node, not on its clone in the new visible row. The user will not see the updated classes.

  2. Missing null guard on fieldBox
    const fieldBox = field.parentElement.closest( 'li.frm_field_box.ui-draggable' ); may return null if the element is not inside a draggable field box. The immediate call to fieldBox.cloneNode( true ) will throw a runtime error. Since changeFieldClass is called generically for any class change, this is a real risk outside the builder context.

  3. Splits incorrectly when target is the first field
    When indexOfTargetField === 0, the loop removes all fields from the original row, leaving it empty. The new cloned box contains all fields. The empty original row persists in the DOM, causing stray empty groups and broken builder state.

  4. Cloning breaks drag-and-drop and event listeners
    cloneNode(true) does not copy jQuery UI data, event listeners, or draggable/droppable state. The new row is inserted without calls to makeDroppable or makeDraggable, so drag-and-drop does not work on it. Other JS listeners attached to child elements are also lost.

  5. No follow-up layout and builder sync
    After restructuring, syncLayoutClasses, updateFieldOrder, and fieldUpdated() are not called, leaving internal state out of sync with the new DOM.

Rework this block to move (not clone) existing field nodes, guard against null/non-builder contexts, skip splitting when the target is already first, and reinitialize drag-and-drop and layout after the split.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e226d8f and 2a58365.

⛔ Files ignored due to path filters (9)
  • js/addons-page.js.map is excluded by !**/*.map, !**/*.map
  • js/form-templates.js.map is excluded by !**/*.map, !**/*.map
  • js/formidable-settings-components.js.map is excluded by !**/*.map, !**/*.map
  • js/formidable_admin.js.map is excluded by !**/*.map, !**/*.map
  • js/formidable_blocks.js.map is excluded by !**/*.map, !**/*.map
  • js/formidable_dashboard.js.map is excluded by !**/*.map, !**/*.map
  • js/formidable_overlay.js.map is excluded by !**/*.map, !**/*.map
  • js/formidable_styles.js.map is excluded by !**/*.map, !**/*.map
  • js/onboarding-wizard.js.map is excluded by !**/*.map, !**/*.map
📒 Files selected for processing (6)
  • css/admin/frm-settings-components.css (1 hunks)
  • css/admin/welcome-tour.css (1 hunks)
  • css/font_icons.css (1 hunks)
  • js/formidable_dashboard.js (1 hunks)
  • js/formidable_overlay.js (1 hunks)
  • js/src/admin/admin.js (5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
js/formidable_overlay.js (3)
js/src/components/class-overlay.js (1)
  • frmOverlay (6-133)
js/src/core/utils/animation.js (1)
  • frmAnimate (1-76)
js/src/core/utils/error.js (3)
  • inputElement (16-16)
  • errorElement (17-17)
  • span (26-26)
🪛 Biome (2.1.2)
js/formidable_overlay.js

[error] 16-16: Do not reassign a function declaration.

Reassigned here.

Use a local variable instead.

(lint/suspicious/noFunctionAssign)


[error] 224-224: Do not reassign a function declaration.

Reassigned here.

Use a local variable instead.

(lint/suspicious/noFunctionAssign)


[error] 527-527: Do not reassign a function declaration.

Reassigned here.

Use a local variable instead.

(lint/suspicious/noFunctionAssign)

js/formidable_dashboard.js

[error] 15-15: Do not reassign a function declaration.

Reassigned here.

Use a local variable instead.

(lint/suspicious/noFunctionAssign)


[error] 83-83: Do not reassign a function declaration.

Reassigned here.

Use a local variable instead.

(lint/suspicious/noFunctionAssign)


[error] 285-285: Do not reassign a function declaration.

Reassigned here.

Use a local variable instead.

(lint/suspicious/noFunctionAssign)


[error] 588-588: Do not reassign a function declaration.

Reassigned here.

Use a local variable instead.

(lint/suspicious/noFunctionAssign)


[error] 848-848: Do not reassign a function declaration.

Reassigned here.

Use a local variable instead.

(lint/suspicious/noFunctionAssign)

🔇 Additional comments (5)
css/admin/welcome-tour.css (1)

1-152: Safe formatting refactor with improved maintainability.

The CSS has been expanded from a minified form to well-formatted, readable code with proper indentation and comments. All selectors (.frm-checklist, .frm-spotlight, etc.) and property values are preserved; this is purely a readability and source-mapping enhancement with no functional styling changes.

css/font_icons.css (1)

1-853: Comprehensive CSS formatting refactor—icon library fully preserved.

This large icon-library stylesheet has been reformatted from minified to expanded form with proper indentation and structure. All ~250+ icon class definitions (.frm_*_icon::before rules) and their content values are preserved. The @font-face rule and all property declarations remain intact. This is a pure readability enhancement with no functional impact.

css/admin/frm-settings-components.css (1)

1-627: Well-organized formatting refactor for admin component styles.

This stylesheet has been expanded from minified form and reorganized with clear section comments (accordion, color picker, background image, radio buttons, sliders, tabs, miscellaneous). All selector chains, property values, custom variables (--grey-*, --gap-*, etc.), and complex CSS (animations, transforms, flex layouts) are fully preserved. This is a pure structural and readability enhancement with zero functional impact on component styling.

js/src/admin/admin.js (2)

1466-1518: Using a flag to distinguish programmatic vs user‑initiated class changes looks good

Passing false as a second argument to the change event (jQuery( layoutClassesInput ).trigger( 'change', [ false ] );) cleanly threads context into liveChanges without altering existing handler contracts. This is a reasonable way to avoid unintended field‑group breaking during automated layout sync.


3257-3279: liveChanges parameter signature is safe and properly integrated

The new signature function liveChanges( event, shouldTryBreakFieldGroup = true ) is consistent with jQuery's trigger semantics and safe for use as an event handler. The parameter is correctly forwarded to changeFieldClass(), which uses it to conditionally break field groups when the frm_first class is added. No external callers invoking liveChanges directly exist outside this file—both usages follow the jQuery event handler pattern via .on('change click', '[data-changeme]', liveChanges), so the default parameter value handles missing arguments correctly.

Comment on lines 122 to 124
// Cleanup observers when page unloads to prevent memory leaks
window.addEventListener('beforeunload', this.cleanupObservers);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

cleanupObservers loses this context when passed as event handler.

The cleanupObservers method uses this.resizeObserver but is passed directly to addEventListener without binding. When called as an event handler, this will be the window object, not the frmTabsNavigator instance, causing the cleanup to silently fail.

       // Cleanup observers when page unloads to prevent memory leaks
-      window.addEventListener('beforeunload', this.cleanupObservers);
+      window.addEventListener('beforeunload', this.cleanupObservers.bind(this));
📝 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
// Cleanup observers when page unloads to prevent memory leaks
window.addEventListener('beforeunload', this.cleanupObservers);
}
// Cleanup observers when page unloads to prevent memory leaks
window.addEventListener('beforeunload', this.cleanupObservers.bind(this));
}
🤖 Prompt for AI Agents
In js/formidable_dashboard.js around lines 122 to 124, cleanupObservers is
passed directly to window.addEventListener so it loses the frmTabsNavigator
instance `this` context; update the registration to preserve context (e.g., bind
the method to the instance or wrap it in an arrow/inline function that calls
this.cleanupObservers()) and ensure any removal uses the same bound reference so
observers are properly disconnected on unload.

Comment on lines 884 to 893
key: "initInbox",
value: function initInbox() {
var _this = this;
new _components_class_tabs_navigator__WEBPACK_IMPORTED_MODULE_1__.frmTabsNavigator('.frm-inbox-wrapper');
var userEmailInput = document.querySelector('#frm_leave_email');
var subscribeButton = document.querySelector('#frm-add-my-email-address');
subscribeButton.addEventListener('click', function () {
_this.saveSubscribedEmail(userEmailInput.value).then();
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Missing null checks before accessing DOM elements.

If #frm_leave_email or #frm-add-my-email-address elements don't exist in the DOM, this code will throw a runtime error when attempting to call addEventListener on null or access .value of null.

     key: "initInbox",
     value: function initInbox() {
       var _this = this;
       new _components_class_tabs_navigator__WEBPACK_IMPORTED_MODULE_1__.frmTabsNavigator('.frm-inbox-wrapper');
       var userEmailInput = document.querySelector('#frm_leave_email');
       var subscribeButton = document.querySelector('#frm-add-my-email-address');
+      if (!subscribeButton || !userEmailInput) {
+        return;
+      }
       subscribeButton.addEventListener('click', function () {
         _this.saveSubscribedEmail(userEmailInput.value).then();
       });
     }
📝 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
key: "initInbox",
value: function initInbox() {
var _this = this;
new _components_class_tabs_navigator__WEBPACK_IMPORTED_MODULE_1__.frmTabsNavigator('.frm-inbox-wrapper');
var userEmailInput = document.querySelector('#frm_leave_email');
var subscribeButton = document.querySelector('#frm-add-my-email-address');
subscribeButton.addEventListener('click', function () {
_this.saveSubscribedEmail(userEmailInput.value).then();
});
}
key: "initInbox",
value: function initInbox() {
var _this = this;
new _components_class_tabs_navigator__WEBPACK_IMPORTED_MODULE_1__.frmTabsNavigator('.frm-inbox-wrapper');
var userEmailInput = document.querySelector('#frm_leave_email');
var subscribeButton = document.querySelector('#frm-add-my-email-address');
if (!subscribeButton || !userEmailInput) {
return;
}
subscribeButton.addEventListener('click', function () {
_this.saveSubscribedEmail(userEmailInput.value).then();
});
}
🤖 Prompt for AI Agents
In js/formidable_dashboard.js around lines 884 to 893, the code assumes
document.querySelector('#frm_leave_email') and
document.querySelector('#frm-add-my-email-address') always return elements; add
null checks so you only call addEventListener when subscribeButton is non-null
and only read userEmailInput.value when userEmailInput is non-null (e.g., wrap
the listener registration in an if (subscribeButton) {
subscribeButton.addEventListener(...)} and inside the handler guard that
userEmailInput exists before calling saveSubscribedEmail with its value or
early-return), ensuring no runtime errors when those DOM elements are absent.

Comment on lines 919 to 952
}, {
key: "saveSubscribedEmail",
value: function saveSubscribedEmail(email) {
return fetch(window.ajaxurl, {
method: 'POST',
headers: {
'Content-Type': 'application/x-www-form-urlencoded'
},
body: new URLSearchParams({
action: this.options.ajax.action,
dashboard_action: this.options.ajax.dashboardActions.saveSubscribedEmail,
email: email
})
}).then(function (result) {
return result.json();
});
}
}, {
key: "closeWelcomeBannerSaveCookieRequest",
value: function closeWelcomeBannerSaveCookieRequest() {
return fetch(window.ajaxurl, {
method: 'POST',
headers: {
'Content-Type': 'application/x-www-form-urlencoded'
},
body: new URLSearchParams({
action: this.options.ajax.action,
dashboard_action: this.options.ajax.dashboardActions.welcomeBanner,
banner_has_closed: 1
})
}).then(function (result) {
return result.json();
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's examine the js/formidable_dashboard.js file around the mentioned lines
wc -l js/formidable_dashboard.js

Repository: Strategy11/formidable-forms

Length of output: 101


🏁 Script executed:

# Get the imports and the specific methods mentioned
sed -n '1,50p' js/formidable_dashboard.js

Repository: Strategy11/formidable-forms

Length of output: 3068


🏁 Script executed:

# Check the specific lines mentioned (919-952)
sed -n '919,952p' js/formidable_dashboard.js

Repository: Strategy11/formidable-forms

Length of output: 1137


🏁 Script executed:

# Search for frmGlobal or nonce references in the file
rg -n "frmGlobal|nonce" js/formidable_dashboard.js | head -30

Repository: Strategy11/formidable-forms

Length of output: 259


🏁 Script executed:

# Check the beginning of the source (after webpack bootstrap) to find imports/globals
sed -n '100,200p' js/formidable_dashboard.js | head -50

Repository: Strategy11/formidable-forms

Length of output: 2350


🏁 Script executed:

# Search for server-side nonce validation in PHP files
rg -n -B2 -A5 "saveSubscribedEmail|welcomeBanner" --type=php | head -60

Repository: Strategy11/formidable-forms

Length of output: 53


🏁 Script executed:

# Broader search for dashboard action handlers in PHP
rg -n "dashboard_action" --type=php | head -20

Repository: Strategy11/formidable-forms

Length of output: 306


🏁 Script executed:

# Search for AJAX action handlers
rg -n -B2 -A10 "wp_ajax.*dashboard|dashboard.*wp_ajax" --type=php

Repository: Strategy11/formidable-forms

Length of output: 1179


🏁 Script executed:

# Look for any nonce validation in dashboard-related code
rg -n "wp_verify_nonce" --type=php | grep -i dashboard

Repository: Strategy11/formidable-forms

Length of output: 53


🏁 Script executed:

# Search more broadly for where these actions are handled
fd -e php | xargs rg -l "dashboard_action" | head -10

Repository: Strategy11/formidable-forms

Length of output: 117


🏁 Script executed:

# Examine the FrmDashboardController.php file around the AJAX handler
sed -n '320,380p' classes/controllers/FrmDashboardController.php

Repository: Strategy11/formidable-forms

Length of output: 1813


🏁 Script executed:

# Get more context around nonce validation
sed -n '1,360p' classes/controllers/FrmDashboardController.php | tail -100

Repository: Strategy11/formidable-forms

Length of output: 3388


AJAX requests missing CSRF nonce protection.

The saveSubscribedEmail and closeWelcomeBannerSaveCookieRequest methods make POST requests to ajaxurl without including a nonce for CSRF verification. The nonce is available from window.frmGlobal.nonce but is not sent. Additionally, the server-side handler in FrmDashboardController::ajax_requests() does not validate a nonce before processing these actions.

Add the nonce parameter to both client-side requests:

       body: new URLSearchParams({
         action: this.options.ajax.action,
         dashboard_action: this.options.ajax.dashboardActions.saveSubscribedEmail,
-        email: email
+        email: email,
+        nonce: frmGlobal.nonce
       })

Apply the same fix to closeWelcomeBannerSaveCookieRequest. Then add server-side nonce verification in FrmDashboardController::ajax_requests() using wp_verify_nonce() before processing the dashboard actions.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In js/formidable_dashboard.js around lines 919 to 952, the two POST AJAX calls
(saveSubscribedEmail and closeWelcomeBannerSaveCookieRequest) do not include the
CSRF nonce; add the nonce from window.frmGlobal.nonce to the URLSearchParams
body for both requests (key name should match server expectation, e.g. 'nonce').
Also update the server-side FrmDashboardController::ajax_requests() to verify
the nonce with wp_verify_nonce($_REQUEST['nonce'], '<expected-action>') before
processing the dashboard_action, and return an appropriate WP_Error/JSON
response if verification fails. Ensure the nonce action string used in
wp_verify_nonce matches the one you use when generating the nonce in PHP.

Comment on lines 119 to 127
if (buttons) {
var buttonsWrapperElementOptions = {
className: 'frm-overlay--cta frm-flex-box',
children: buttons
};
return frmDom.div(buttonsWrapperElementOptions);
}
return '';
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Array truthiness check is always true.

The buttons variable is the result of .map(), which always returns an array. This condition is always truthy, even when the array is empty, causing an empty wrapper div to be rendered.

-      if (buttons) {
+      if (buttons.length) {
📝 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
if (buttons) {
var buttonsWrapperElementOptions = {
className: 'frm-overlay--cta frm-flex-box',
children: buttons
};
return frmDom.div(buttonsWrapperElementOptions);
}
return '';
}
if (buttons.length) {
var buttonsWrapperElementOptions = {
className: 'frm-overlay--cta frm-flex-box',
children: buttons
};
return frmDom.div(buttonsWrapperElementOptions);
}
return '';
}
🤖 Prompt for AI Agents
In js/formidable_overlay.js around lines 119 to 127, the condition `if
(buttons)` is always truthy because `buttons` is the result of `.map()` (an
array) and will render an empty wrapper even when no buttons exist; change the
guard to check for non-empty array (e.g., `if (buttons && buttons.length > 0)`)
so the wrapper div is only created when there are actual button elements,
otherwise return the empty string as before.

Comment on lines 346 to 348
var addToRequestQueue = function addToRequestQueue(task) {
return lastPromise = lastPromise.then(task).catch(task);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Unusual error handling in request queue may cause unexpected behavior.

When a promise rejects, .catch(task) calls the task function with the rejection reason as its argument. Most task functions likely expect no arguments or specific arguments, not error objects. This could lead to silent failures or unexpected behavior.

Consider using a proper error handler:

 var addToRequestQueue = function addToRequestQueue(task) {
-  return lastPromise = lastPromise.then(task).catch(task);
+  return lastPromise = lastPromise.then(task).catch(function(error) {
+    console.error('Request queue error:', error);
+    return task(); // Retry or skip based on requirements
+  });
 };
📝 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
var addToRequestQueue = function addToRequestQueue(task) {
return lastPromise = lastPromise.then(task).catch(task);
};
var addToRequestQueue = function addToRequestQueue(task) {
return lastPromise = lastPromise.then(task).catch(function(error) {
console.error('Request queue error:', error);
return task(); // Retry or skip based on requirements
});
};
🤖 Prompt for AI Agents
In js/formidable_overlay.js around lines 346-348, the request-queue catch
currently forwards the rejection value into task by using .catch(task), which
passes an error object as an argument to task; change the error handling so that
task is invoked without the rejection reason (e.g., .catch(() => task()) or
.catch(() => Promise.resolve().then(task))) and optionally log the error before
calling task, ensuring lastPromise is assigned to the recovered promise chain
and that rejected values are not passed as unintended arguments to task.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
js/src/admin/admin.js (1)

6763-6801: maybeBreakFieldGroup has critical bugs that silently fail to apply class changes and break drag/drop behavior

The field-group splitting logic has several correctness issues that prevent intended changes from being applied to the visible DOM:

  1. Class updates applied to detached, invisible node (Line 6798 + 6801):

    • maybeBreakFieldGroup removes the original field via fieldsInFieldGroup.slice( indexOfTargetField ).map( field => field.remove() ).
    • changeFieldClass then calls field.className = field.className.replace( ... ) on the now-detached original field.
    • The visible field (inside the cloned newFieldGroup) never receives the class update, so frm_first silently fails to apply.
  2. New field group loses drag/drop behavior (Line 6816):

    • fieldGroup.cloneNode( true ) copies DOM structure but not jQuery UI event handlers or data.
    • maybeBreakFieldGroup never calls makeDroppable / makeDraggable on newFieldGroup, leaving the new group non-interactive in the builder.
  3. Unprotected null dereference (Line 6814):

    • .closest( 'li.frm_field_box.ui-draggable' ) can return null if the group lacks the ui-draggable class, causing fieldGroup.cloneNode( true ) to crash at runtime.
  4. Unnecessary empty groups when target is first/only field (Line 6813):

    • When indexOfTargetField <= 0 or the group has one field, the code still clones and splits, wasting DOM nodes.

The fix requires returning the (possibly moved) field node from maybeBreakFieldGroup so changeFieldClass updates the visible element, moving existing nodes instead of cloning to preserve event bindings, and re-applying makeDroppable / makeDraggable to the new group. Edge cases (null fieldGroup, first field, single-field groups) should short-circuit to avoid splits and crashes.

🧹 Nitpick comments (1)
js/src/admin/admin.js (1)

3257-3279: liveChanges signature change is backward‑compatible but semantics should be documented

Extending liveChanges to liveChanges( event, shouldTryBreakFieldGroup = true ) and threading that flag into changeFieldClass is backward‑compatible for existing callers, and the default true keeps prior behavior unless explicitly overridden (as from getSyncLayoutClass). Consider adding a short JSDoc on liveChanges noting the meaning of shouldTryBreakFieldGroup so future internal callers don’t accidentally opt into group‑breaking when they mean to do a pure visual sync.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2a58365 and 98552c9.

⛔ Files ignored due to path filters (1)
  • js/formidable_admin.js.map is excluded by !**/*.map, !**/*.map
📒 Files selected for processing (15)
  • css/admin/frm-settings-components.css
  • css/frm_admin.css
  • css/frm_testing_mode.css
  • js/addons-page.js
  • js/form-templates.js
  • js/formidable-settings-components.js
  • js/formidable_admin.js
  • js/formidable_blocks.js
  • js/formidable_dashboard.js
  • js/formidable_overlay.js
  • js/formidable_styles.js
  • js/frm_testing_mode.js
  • js/onboarding-wizard.js
  • js/src/admin/admin.js
  • js/welcome-tour.js
🚧 Files skipped from review as they are similar to previous changes (1)
  • css/admin/frm-settings-components.css
🧰 Additional context used
🧬 Code graph analysis (3)
js/formidable_dashboard.js (1)
js/src/admin/addon-state.js (2)
  • frmDom (3-3)
  • a (198-198)
js/formidable_overlay.js (3)
js/src/admin/admin.js (41)
  • e (8305-8305)
  • t (718-718)
  • t (743-743)
  • t (9092-9092)
  • t (10283-10283)
  • frmDom (241-241)
  • frmDom (242-242)
  • frmDom (243-243)
  • a (3098-3098)
  • i (440-440)
  • i (1798-1798)
  • i (2283-2283)
  • i (3035-3035)
  • i (3050-3050)
  • i (3122-3122)
  • i (3140-3140)
  • i (3154-3157)
  • i (3180-3188)
  • i (3218-3218)
  • i (3227-3227)
  • i (3488-3494)
  • i (3630-3633)
  • i (5480-5480)
  • i (5549-5550)
  • i (5624-5626)
  • c (723-723)
  • c (748-748)
  • c (7343-7343)
  • c (9168-9168)
  • s (276-276)
  • s (8311-8311)
  • s (9068-9068)
  • s (9145-9145)
  • f (9107-9107)
  • v (3394-3394)
  • v (3414-3414)
  • v (6975-6975)
  • v (7347-7347)
  • v (7898-7898)
  • v (7914-7914)
  • v (7930-7930)
js/src/admin/upgrade-popup.js (1)
  • frmDom (3-3)
js/src/admin/addon-state.js (2)
  • frmDom (3-3)
  • a (198-198)
js/frm_testing_mode.js (1)
js/admin/style.js (5)
  • frmDom (23-23)
  • frmDom (24-24)
  • frmDom (25-25)
  • frmDom (26-26)
  • frmDom (1164-1164)
🪛 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)

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.

min-width 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_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-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_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)

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)

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/welcome-tour.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: 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)


[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)

🔇 Additional comments (11)
css/frm_testing_mode.css (1)

2-3: No CSS duplication found in this file.

The Bootstrap tooltip CSS block appears only once in the stylesheet. The comment closure (*/) at line 3 correctly terminates the Bootstrap v4.6.1 comment block, and the minified CSS rules that follow contain no duplicate class definitions. Properties like text-align, min-width, and border-radius appear multiple times only because they belong to different CSS selectors—this is normal CSS structure, not duplication.

js/welcome-tour.js (1)

1-2: Minified bundle generated by webpack; Biome errors are false positives.

This file is the minified output from webpack (built from js/src/welcome-tour/). Unlike the original comment suggested, this project intentionally commits built bundles to git—the .gitignore only excludes js/*.js.LICENSE.txt files, not the compiled .js files themselves.

The Biome static analysis errors (switch fallthrough, function reassignments, label conflicts) are expected false positives inherent to minified code and should not drive code review decisions. No action needed.

js/formidable_styles.js (1)

1-1: Minified build artifact with no functional changes.

This file is a minified production build. The AI summary confirms no functional changes were detected—only formatting/whitespace adjustments. The static analysis warnings about variable hoisting and function reassignment are expected patterns in minified code and are false positives.

js/addons-page.js (1)

1-1: Minified build artifact with no functional changes.

This minified production build shows no functional changes according to the summary. The static analysis warnings are expected minified code patterns.

js/onboarding-wizard.js (1)

2-2: Internal identifier renaming in minified build.

The summary indicates only internal parameter identifier changes (e.g., n → r) with no changes to control flow or behavior. The static analysis warnings are false positives for minified code.

js/formidable-settings-components.js (1)

1-1: Structural reflow in minified build with no functional impact.

This is a formatting/structural reflow with no functional changes. Static analysis warnings are expected for minified production code.

js/formidable_dashboard.js (1)

1-1: Formatting changes in minified build.

The summary confirms only minor formatting/alignment changes with no alterations to runtime behavior, control flow, or logic. The changes appear to be from a rebuild process.

js/frm_testing_mode.js (1)

2-2: Internal parameter renaming in minified build.

This minified build shows only internal parameter name changes and minor operand reordering with no behavioral changes.

js/formidable_overlay.js (1)

1-1: Minified variant replacement with preserved behavior.

The summary indicates this is a minified variant that preserves the same public surface and behavior. No functional changes to public APIs or event handling.

js/formidable_blocks.js (1)

1-1: Export binding change in minified build.

The export name remains Ay but the internal binding changed from i to l. According to the summary, this is an internal-only change with no impact on external consumers. The functional CSS/class content remains unchanged.

js/src/admin/admin.js (1)

1466-1518: Change-event gating for layout class sync is sound

Passing [ false ] into the change trigger on layoutClassesInput cleanly distinguishes programmatic sync from user-initiated edits, so the new shouldTryBreakFieldGroup flag can short‑circuit group‑breaking logic without altering existing handlers. No functional issues spotted here.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
js/src/admin/admin.js (1)

6763-6801: Field group splitting currently updates the wrong DOM node (classes applied to detached element)

Right now maybeBreakFieldGroup removes the original field element from the DOM and keeps a cloned copy in the new group:

  • fieldGroup.cloneNode( true ) creates a full clone (including the target .form-field and its siblings).
  • fieldsInFieldGroup.slice( indexOfTargetField ).map( field => field.remove() ); removes the original target field and all following siblings from the original UL.
  • After inserting the cloned newFieldGroup, you only remove the cloned predecessors, so the live DOM for the target field is the cloned node, not the original one referenced by field.

However, changeFieldClass then does:

maybeBreakFieldGroup( !! shouldTryBreakFieldGroup, field, classes, replaceWith );
field.className = field.className.replace( replace, replaceWith );

so the class replacement runs on the original (now detached) node, while the visible cloned node in the form builder keeps the old classes (and will not gain the new frm_first). That makes the new “break group when frm_first is added” behavior unreliable and hard to debug.

Two concrete improvements are needed here:

  1. Fix correctness: ensure the live DOM node gets updated

    Minimal change inside changeFieldClass:

  • maybeBreakFieldGroup( !! shouldTryBreakFieldGroup, field, classes, replaceWith );
    
  • field.className = field.className.replace( replace, replaceWith );
    
  • maybeBreakFieldGroup( !! shouldTryBreakFieldGroup, field, classes, replaceWith );
    
  • // After maybeBreakFieldGroup runs, the original element may have been
    
  • // replaced by a clone in the DOM. Always update the live element.
    
  • if ( field.id ) {
    
  • 	const liveField = document.getElementById( field.id );
    
  • 	if ( liveField ) {
    
  • 		field = liveField;
    
  • 	}
    
  • }
    
  • field.className = field.className.replace( replace, replaceWith );
    
    
    
  1. Prefer moving existing children instead of cloning the entire group

    Cloning the group (cloneNode( true )) duplicates all descendant nodes and their runtime state; you then partially remove originals and clones. Even with the above fix, this risks losing non‑delegated listeners and any state attached to the original nodes. A more robust approach is to:

    • Clone only the wrapper structure (e.g., fieldGroup.cloneNode( false ) and field.parentElement.cloneNode( false )).
    • Move fieldsInFieldGroup.slice( indexOfTargetField ) into the new UL with appendChild instead of cloning.
    • Then initialize droppable/dragging on the new UL.

    That keeps field identity intact and avoids subtle regressions in other builder behaviors.

Given this touches the core builder layout DOM, I’d treat fixing this before release as high priority.

🧹 Nitpick comments (1)
js/src/admin/admin.js (1)

6803-6828: Guardrails and lint for maybeBreakFieldGroup

Functionally, maybeBreakFieldGroup should also defensively bail out when the structure is not what you expect:

  • If field.parentElement or field.parentElement.closest( 'li.frm_field_box.ui-draggable' ) is null, this will currently throw. A simple early‑return guard keeps things safe when frm_first is applied outside of a standard field group.
  • If the group has only one child, splitting does nothing useful; short‑circuiting on fieldsInFieldGroup.length <= 1 avoids creating empty wrappers.

On the style side, ESLint is flagging the JSDoc alignment here (Expected JSDoc block lines to be aligned). Re‑aligning the * and fixing the param types (e.g., @param {boolean} shouldTryBreakFieldGroup) will clear the warning without behavioral change.

You may also want to replace .map( field => field.remove() ) with .forEach( field => field.remove() ) to better express the side‑effect intent.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 98552c9 and 2624df6.

⛔ Files ignored due to path filters (1)
  • js/formidable_admin.js.map is excluded by !**/*.map, !**/*.map
📒 Files selected for processing (2)
  • js/formidable_admin.js
  • js/src/admin/admin.js
🧰 Additional context used
🪛 GitHub Check: Run ESLint
js/src/admin/admin.js

[warning] 6803-6803:
Expected JSDoc block lines to be aligned

🔇 Additional comments (2)
js/src/admin/admin.js (2)

1466-1518: Good use of an extra argument to gate side‑effects from auto layout sync

Passing false as the second arg in jQuery( layoutClassesInput ).trigger( 'change', [ false ] ); cleanly distinguishes programmatic layout sync from user‑initiated class edits, so liveChanges can avoid attempting to break field groups in the former case. This looks correct and backwards‑compatible with other change listeners that ignore extra args.


3257-3279: liveChanges new parameter is fine but relies on all internal trigger sites being updated

The new shouldTryBreakFieldGroup = true parameter and changeFieldClass( changes, this, shouldTryBreakFieldGroup ); wiring are consistent, and the only explicit internal trigger (getSyncLayoutClass) now passes [ false ], so behavior should be gated as intended. Please double‑check that no external/custom code calls liveChanges directly with a different second argument that might now inadvertently control the group‑breaking behavior.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants