Fix range selection that includes fields in section #2527
Conversation
WalkthroughExtracts Shift-multi-select range computation into a new helper getSelectedRange($firstGroup, hoverTarget) in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AdminUI as Admin UI
participant RangeFn as getSelectedRange
User->>AdminUI: Shift + hover/click a field
AdminUI->>RangeFn: getSelectedRange($firstGroup, hoverTarget)
Note right of RangeFn #E6F4EA: compute ordered, inclusive range\nhandle Section-contained fields
RangeFn-->>AdminUI: jQuery collection (field range)
AdminUI->>AdminUI: iterate range and toggle selection
AdminUI-->>User: updated multi-selection UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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: 8
🧹 Nitpick comments (2)
css/admin/frm-settings-components.css (2)
245-250: Add keyboard focus styles (a11y).Hover-only styling misses keyboard users. Mirror the hover state on
:focus-visible..frm-style-tabs-wrapper .frm-tabs-navs ul > li:not(.frm-active):hover, .frm-style-component .frm-radio-container input:not(:checked) + label:hover { background: rgb(234, 236, 240); border-radius: var(--small-radius); box-shadow: var(--button-shadow); } +/* Keyboard focus parity */ +.frm-style-tabs-wrapper .frm-tabs-navs ul > li:not(.frm-active):focus-visible, +.frm-style-component .frm-radio-container input:not(:checked) + label:focus-visible { + background: rgb(234, 236, 240); + border-radius: var(--small-radius); + box-shadow: var(--button-shadow); + outline: none; +}
127-135: Avoidtext-indent: -9999pxfor hiding text.Use a visually-hidden utility to keep content accessible without layout shifts.
Example replacement:
-.frm_remove_image_option { +/* Replace text hiding with visually-hidden utility for child text node */ +.frm_remove_image_option { width: 100%; height: 100%; border: 1px solid var(--red-500); - text-indent: -9999px; position: absolute; top: 0; left: 0; } +/* Add a shared utility in base CSS and apply to the text node instead */ +.sr-only { + position: absolute !important; + width: 1px !important; + height: 1px !important; + padding: 0 !important; + margin: -1px !important; + overflow: hidden !important; + clip: rect(0, 0, 0, 0) !important; + white-space: nowrap !important; + border: 0 !important; +}Then wrap the textual label in an element and add
class="sr-only".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
js/form-templates.js.mapis excluded by!**/*.map,!**/*.mapjs/formidable-settings-components.js.mapis excluded by!**/*.map,!**/*.mapjs/formidable_dashboard.js.mapis excluded by!**/*.map,!**/*.mapjs/formidable_styles.js.mapis excluded by!**/*.map,!**/*.map
📒 Files selected for processing (5)
css/admin/frm-settings-components.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(1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
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] 542-542: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 792-792: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
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] 481-481: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
🔇 Additional comments (2)
js/src/admin/admin.js (1)
4574-4595: Verify shift-range selection logic in sections
Ensure that hoverTarget.closest('.edit_field_type_divider').closest('ul').closest('.frm_field_box.ui-draggable') correctly identifies the section container, that fieldsInSection.querySelectorAll('.frm_field_box.ui-draggable') returns the expected list items, and that comparing $firstGroup.parent().index() with jQuery(hoverTarget.parentNode).index() yields accurate forward/backward ranges. Add tests for shift-select both within a section and across sections.js/formidable_dashboard.js (1)
1-10: Lint false positives on compiled helpers.The “noFunctionAssign” errors come from Babel helpers in compiled bundles. Exclude built files from linting or set an override for webpack outputs.
Suggested config update (example):
- Add ignore patterns like
css/**,js/*.js, or**/formidable_*.jsto your linter config, or use per-file overrides to disablesuspicious/noFunctionAssignfor bundles.Also applies to: 15-21, 83-89, 285-291, 542-546, 792-798
| .formidable_page_formidable-styles.js .control-section .accordion-section-title:focus { | ||
| background: none; | ||
| } |
There was a problem hiding this comment.
Selector likely never matches (combined .formidable_page_formidable-styles.js).
WordPress typically applies js to <html>, not the same element as formidable_page_formidable-styles. Split selectors or scope js to html.
Suggested fix:
-.formidable_page_formidable-styles.js .control-section .accordion-section-title:focus {
+html.js .formidable_page_formidable-styles .control-section .accordion-section-title:focus {
background: none;
}🤖 Prompt for AI Agents
In css/admin/frm-settings-components.css around lines 10-12, the selector
".formidable_page_formidable-styles.js .control-section
.accordion-section-title:focus" incorrectly assumes the "js" class sits on the
same element as "formidable_page_formidable-styles"; change the selector so it
matches real DOM placement by scoping the "js" class to the html element and/or
providing the non-js fallback. Replace the single combined selector with two
selectors separated by a comma: "html.js .formidable_page_formidable-styles
.control-section .accordion-section-title:focus" and
".formidable_page_formidable-styles .control-section
.accordion-section-title:focus" so the rule applies when the js class is on html
and still works without it.
css/font_icons.css
Outdated
| .frm_arrowup5_solid_icon::before { | ||
| content: "\e9d"; | ||
| } |
There was a problem hiding this comment.
Fix the Arrow Up solid glyph codepoint
content: "\e9d"; escapes to U+0E9D (Thai character), so our icon font won’t render and the arrow shows a fallback glyph. Nearby mappings stay in the private-use block (\e90*), so this looks like a truncated code. Please restore the intended codepoint (likely \e90d) to keep the icon visible.
-.frm_arrowup5_solid_icon::before {
- content: "\e9d";
+.frm_arrowup5_solid_icon::before {
+ content: "\e90d";
}📝 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.
| .frm_arrowup5_solid_icon::before { | |
| content: "\e9d"; | |
| } | |
| .frm_arrowup5_solid_icon::before { | |
| content: "\e90d"; | |
| } |
🤖 Prompt for AI Agents
In css/font_icons.css around lines 225 to 227, the glyph mapping uses the
incorrect escape value e9d which maps to the Thai block; replace that escape
with the intended private-use block codepoint e90d so the arrow-up solid icon
renders consistently with nearby mappings, and verify the surrounding icon
codepoints follow the same e90* sequence.
js/formidable_dashboard.js
Outdated
| window.addEventListener('beforeunload', this.cleanupObservers); | ||
| } |
There was a problem hiding this comment.
Unbound event handler loses this (cleanup not executed).
this.cleanupObservers runs with wrong context on beforeunload. Bind or wrap in an arrow function.
- window.addEventListener('beforeunload', this.cleanupObservers);
+ window.addEventListener('beforeunload', () => this.cleanupObservers());📝 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.
| window.addEventListener('beforeunload', this.cleanupObservers); | |
| } | |
| window.addEventListener('beforeunload', () => this.cleanupObservers()); | |
| } |
🤖 Prompt for AI Agents
In js/formidable_dashboard.js around lines 123 to 124, the beforeunload handler
is passed as this.cleanupObservers which loses the class instance context so
cleanup won't run correctly; fix by passing a bound function or an arrow wrapper
that preserves `this` (e.g., register the listener with
this.cleanupObservers.bind(this) or () => this.cleanupObservers()) and ensure
the same bound/wrapped reference is used when removing the listener so
cleanupObservers can be removed correctly.
js/formidable_dashboard.js
Outdated
| key: "initSlideTrackUnderline", | ||
| value: function initSlideTrackUnderline(nav, index) { | ||
| this.slideTrackLine.classList.remove('frm-first', 'frm-last'); | ||
| var activeNav = 'undefined' !== typeof nav ? nav : this.navs.filter(function (nav) { | ||
| return nav.classList.contains('frm-active'); | ||
| }); | ||
| this.positionUnderlineIndicator(activeNav); | ||
| } |
There was a problem hiding this comment.
Bug: uses NodeList.filter and passes array to a method expecting an element.
this.navs is a NodeList; .filter is unavailable, and positionUnderlineIndicator expects an HTMLElement, not an array.
initSlideTrackUnderline(nav, index) {
this.slideTrackLine.classList.remove('frm-first', 'frm-last');
- var activeNav = 'undefined' !== typeof nav ? nav : this.navs.filter(function (nav) {
- return nav.classList.contains('frm-active');
- });
- this.positionUnderlineIndicator(activeNav);
+ var activeNav = 'undefined' !== typeof nav
+ ? nav
+ : this.wrapper.querySelector('.frm-tabs-navs ul > li.frm-active');
+ if (activeNav) {
+ this.positionUnderlineIndicator(activeNav);
+ }
}📝 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.
| key: "initSlideTrackUnderline", | |
| value: function initSlideTrackUnderline(nav, index) { | |
| this.slideTrackLine.classList.remove('frm-first', 'frm-last'); | |
| var activeNav = 'undefined' !== typeof nav ? nav : this.navs.filter(function (nav) { | |
| return nav.classList.contains('frm-active'); | |
| }); | |
| this.positionUnderlineIndicator(activeNav); | |
| } | |
| key: "initSlideTrackUnderline", | |
| value: function initSlideTrackUnderline(nav, index) { | |
| this.slideTrackLine.classList.remove('frm-first', 'frm-last'); | |
| var activeNav = 'undefined' !== typeof nav | |
| ? nav | |
| : this.wrapper.querySelector('.frm-tabs-navs ul > li.frm-active'); | |
| if (activeNav) { | |
| this.positionUnderlineIndicator(activeNav); | |
| } | |
| } |
js/formidable_dashboard.js
Outdated
| /* harmony import */ var core_utils__WEBPACK_IMPORTED_MODULE_0__ = __webpack_require__(/*! core/utils */ "./js/src/core/utils/index.js"); | ||
| /* harmony import */ var _components_class_tabs_navigator__WEBPACK_IMPORTED_MODULE_1__ = __webpack_require__(/*! ./components/class-tabs-navigator */ "./js/src/components/class-tabs-navigator.js"); | ||
| /* harmony import */ var _components_class_counter__WEBPACK_IMPORTED_MODULE_2__ = __webpack_require__(/*! ./components/class-counter */ "./js/src/components/class-counter.js"); | ||
| function _typeof(o) { "@babel/helpers - typeof"; return _typeof = "function" == typeof Symbol && "symbol" == typeof Symbol.iterator ? function (o) { return typeof o; } : function (o) { return o && "function" == typeof Symbol && o.constructor === Symbol && o !== Symbol.prototype ? "symbol" : typeof o; }, _typeof(o); } |
There was a problem hiding this comment.
Add nonce to admin-ajax requests (CSRF protection).
A nonce is exported in core/constants. Include it in the POST body to harden requests.
/* harmony import */ var core_utils__WEBPACK_IMPORTED_MODULE_0__ = __webpack_require__(/*! core/utils */ "./js/src/core/utils/index.js");
+/* harmony import */ var core_constants__WEBPACK_IMPORTED_MODULE_3__ = __webpack_require__(/*! core/constants */ "./js/src/core/constants.js");
@@
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
+ email: email,
+ nonce: core_constants__WEBPACK_IMPORTED_MODULE_3__.nonce
})
}).then(function (result) {
return result.json();
});
}
@@
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
+ banner_has_closed: 1,
+ nonce: core_constants__WEBPACK_IMPORTED_MODULE_3__.nonce
})
}).then(function (result) {
return result.json();
});
}Also applies to: 866-876, 883-893
🧰 Tools
🪛 Biome (2.1.2)
[error] 792-792: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
🤖 Prompt for AI Agents
In js/formidable_dashboard.js around lines 789-792 (and also update occurrences
at 866-876 and 883-893), admin-ajax POST requests currently lack the CSRF nonce;
import the nonce exported from core/constants (add the import near other
imports) and include it as a field in the POST body (e.g., data.nonce =
FORMI_NONCE) for every fetch/XHR that posts to admin-ajax; ensure the key
matches the server-side expectation and update all three code blocks to attach
the nonce before sending.
js/formidable_dashboard.js
Outdated
| 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(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Null checks and default prevention for subscribe button.
Avoid NPEs if elements are missing and prevent form submission/page reload on click.
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();
- });
+ if (subscribeButton && userEmailInput) {
+ subscribeButton.addEventListener('click', function (e) {
+ e.preventDefault();
+ _this.saveSubscribedEmail(userEmailInput.value).catch(function(err){ console.error(err); });
+ });
+ }📝 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.
| 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(); | |
| }); | |
| } | |
| var userEmailInput = document.querySelector('#frm_leave_email'); | |
| var subscribeButton = document.querySelector('#frm-add-my-email-address'); | |
| if (subscribeButton && userEmailInput) { | |
| subscribeButton.addEventListener('click', function (e) { | |
| e.preventDefault(); | |
| _this.saveSubscribedEmail(userEmailInput.value).catch(function (err) { | |
| console.error(err); | |
| }); | |
| }); | |
| } | |
| } |
🤖 Prompt for AI Agents
In js/formidable_dashboard.js around lines 832–837, the click handler assumes
DOM elements exist and doesn't prevent default form submission; update to first
verify subscribeButton and userEmailInput are non-null before attaching the
listener, inside the handler call event.preventDefault(), read the input value
only after checking userEmailInput exists (use a safe fallback like empty string
or trim), and invoke saveSubscribedEmail with that value while at least
attaching a .catch() to handle errors (do not leave a bare .then()). This
prevents NPEs and stops the form/page reload on click.
js/formidable_overlay.js
Outdated
| var lastPromise = Promise.resolve(); | ||
|
|
||
| /** | ||
| * Adds a task to the request queue. | ||
| * | ||
| * @param {function(): Promise<any>} task A function that returns a promise. | ||
| * @return {Promise<any>} The new last promise in the queue. | ||
| */ | ||
| var addToRequestQueue = function addToRequestQueue(task) { | ||
| return lastPromise = lastPromise.then(task).catch(task); | ||
| }; |
There was a problem hiding this comment.
Fix the request queue retry loop
addToRequestQueue reuses the same task as both the success handler and the rejection handler. When a queued task rejects, the .catch(task) branch invokes that task again with the rejection value, so the same failing task is executed twice (and will keep recursing if it keeps rejecting). This traps the queue on the failed task and prevents every subsequent request from running. Please keep the queue moving by swallowing the rejection (or logging it) and only chaining the next task after the prior promise settles.
A simple fix at the source (js/src/core/utils/async.js) would be:
-const addToRequestQueue = ( task ) => {
- return lastPromise = lastPromise.then( task ).catch( task );
-};
+const addToRequestQueue = ( task ) => {
+ lastPromise = lastPromise.catch( () => undefined );
+ lastPromise = lastPromise.then( () => task() );
+ return lastPromise.catch( ( error ) => {
+ console.error( error );
+ return Promise.reject( error );
+ } );
+};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In js/formidable_overlay.js around lines 338 to 348, addToRequestQueue currently
chains .then(task).catch(task) which re-invokes the same failing task on
rejection and stalls the queue; change the chaining so the queued task is
invoked once and the queue continues regardless of its outcome by calling
lastPromise = lastPromise.then(() => task()).then(() => {}, (err) => { /*
swallow or log err */ }); in other words, invoke task in the success handler,
and on rejection swallow or log the error instead of calling task again so the
next queued task runs after the prior promise settles.
|
Hi @AbdiTolesa, I tested this and noticed this issue: When selecting from top to bottom (e.g., starting from a field above the Section and clicking on the last field inside the Section), some fields inside the Section are not included in the selection. But wanted to mention also that when selecting from bottom to top (e.g., starting from a field outside the Section and Shift-clicking the first field inside the Section), the selection works correctly, and all expected fields are selected, including those in the Section. Please see screen recording for reference: https://www.loom.com/share/a69369690a0f4e92bcdb317d14c203f5 |
|
Thank you for catching that @lauramekaj1! I have fixed that issue with my latest commit too. |
@AbdiTolesa I retested it and there are some issues I noticed:
Please see screen recording as a reference for the three issues: https://www.loom.com/share/c03cc1dcfbcd456581d51cbc2adac328 |
@lauramekaj1 I think this sounds like https://github.com/Strategy11/formidable-pro/issues/6017 but can you confirm that? |
@lauramekaj1 I have fixed those two too. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/src/admin/admin.js(2 hunks)
🔇 Additional comments (1)
js/src/admin/admin.js (1)
4617-4626: Verify selected count accuracy when adding range results.Range now includes the hover target and possibly the first group. Since numberOfSelectedGroups is pre-incremented for the current target, ensure the loop doesn’t double-count it when it isn’t already selected. If needed, filter out the hoverTarget wrapper before incrementing, or compute the count from the final DOM selection to avoid off‑by‑one.
@AbdiTolesaI can confirm that Issue 1 is quite similar to the linked issue you provided. I’ve retested the fixes for Issues 2 and 3, and I was still able to reproduce the same unexpected behavior. Could you please check on your side as well? |
AbdiTolesa
added
deploy
There was a problem hiding this comment.
Actionable comments posted: 1
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)
4614-4629: Fix double‑counting selected groups during Shift‑select
numberOfSelectedGroupsis incremented before the loop and again when the hovered group is added inside the loop, inflating the count. Remove the pre‑increment and compute the count from the DOM after selection.- } else if ( shiftKeyIsDown && ! groupIsActive ) { - ++numberOfSelectedGroups; // include the one we're selecting right now. + } else if ( shiftKeyIsDown && ! groupIsActive ) { const $firstGroup = $selectedFieldGroups.first(); getSelectedRange( $firstGroup, hoverTarget ).each( function() { const $fieldGroup = jQuery( this ).closest( 'li' ).find( 'ul.frm_sorting' ); if ( ! $fieldGroup.hasClass( 'frm-selected-field-group' ) ) { - ++numberOfSelectedGroups; $fieldGroup.addClass( 'frm-selected-field-group' ); } } ); + // Recalculate after applying range selection. + numberOfSelectedGroups = jQuery( '.frm-selected-field-group' ).length; }
♻️ Duplicate comments (1)
js/src/admin/admin.js (1)
4536-4573: Shift+range selection still breaks across Sections/pages; compute range via a flattened DOM sliceProblems:
- Compares indexes from different parents → wrong direction/ranges.
- nextUntil/prevUntil don’t cross Section/page boundaries.
- Brittle filter
classList.length === 2.- Backward slice math is incorrect.
Fix by flattening all selectable row wrappers in DOM order and slicing between first and target (inclusive). This resolves over‑selection in Sections, bottom‑to‑top spans, and second‑page selection.
-function getSelectedRange( $firstGroup, hoverTarget ) { - const hoverTargetSection = hoverTarget.closest( '.edit_field_type_divider' ); - let targetSection, $range, fieldsInSection; - if ( hoverTargetSection ) { - targetSection = hoverTargetSection.closest( 'ul' ).closest( '.frm_field_box.ui-draggable' ); - fieldsInSection = [ ...hoverTargetSection.querySelectorAll( '.frm_field_box.ui-draggable' )].filter( el => el.classList.length === 2 ); - } - const hoverTargetBox = hoverTarget.closest( 'li' ); - if ( $firstGroup.parent().index() < jQuery( hoverTarget.parentNode ).index() ) { - // If field target field is in a section. - $range = $firstGroup.parent().nextUntil( targetSection || hoverTarget.parentNode ); - - if ( ! fieldsInSection ) { - return $range; - } - $range = $range.add( fieldsInSection.slice( 0, fieldsInSection.indexOf( hoverTargetBox ) + 1 ) ); - return $range; - } - - // Select fields back to the first group. - $range = $firstGroup.parent().prevUntil( targetSection || hoverTarget.parentNode ); - if ( ! fieldsInSection ) { - return $range; - } - $range = $range.add( fieldsInSection.slice( -fieldsInSection.indexOf( hoverTargetBox ) + 1 ) ); - - return $range; -} +function getSelectedRange( $firstGroup, hoverTarget ) { + // Flatten all selectable row wrappers (inside/outside Sections) in DOM order. + // Exclude Section title rows themselves. + const allRows = Array.from( + document.querySelectorAll( '#frm-show-fields li.frm_field_box' ) + ).filter( li => ! li.classList.contains( 'edit_field_type_divider' ) ); + + const firstWrapper = $firstGroup.parent().get( 0 ); // li.frm_field_box + const targetWrapper = hoverTarget.closest( 'li.frm_field_box' ); + if ( ! firstWrapper || ! targetWrapper ) { + return jQuery(); + } + + const start = allRows.indexOf( firstWrapper ); + const end = allRows.indexOf( targetWrapper ); + if ( start === -1 || end === -1 ) { + return jQuery(); + } + + const from = Math.min( start, end ); + const to = Math.max( start, end ); + return jQuery( allRows.slice( from, to + 1 ) ); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
css/frm_testing_mode.css(1 hunks)js/addons-page.js(1 hunks)js/formidable_blocks.js(1 hunks)js/formidable_styles.js(1 hunks)js/frm_testing_mode.js(1 hunks)js/src/admin/admin.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
js/frm_testing_mode.js (1)
js/src/frm_testing_mode.js (1)
frmDom(34-34)
🪛 Biome (2.1.2)
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)
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/addons-page.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: 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_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: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[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)
🔇 Additional comments (5)
js/formidable_styles.js (1)
1-1: This file appears to be minified/bundled output; line-by-line review not feasible.These are build artifacts (bundled modules with internal memoization and i18n utilities). Module ID remapping (616→8616, 604→7604) aligns with similar changes across the PR. Static analysis errors are false positives typical of minified code.
js/frm_testing_mode.js (1)
2-2: This file appears to be minified/bundled output; line-by-line review not feasible.Build artifact with internal module ID remapping (793→8793, 323→1323). Static analysis flags are false positives expected in minified code.
js/addons-page.js (1)
1-1: This file appears to be minified/bundled output; line-by-line review not feasible.Build artifact with internal module ID remapping (616→8616). Wiring change is consistent across the PR. Static analysis flags are false positives expected in minified code.
css/frm_testing_mode.css (1)
1-1: Review comment appears to be based on incomplete information. The file exists and is provided for review.The script execution reveals:
- ✅
js/src/admin/admin.jsexists in the codebase- ✅
getSelectedRange($firstGroup, hoverTarget)is implemented (lines 4546-4619)- ✅ The function is actively used at line 4619 with
.each()iteration- ❌ No dedicated test files exist for the multi-select fix
The review comment claims the file is "not provided" but it is present in the repository. However, the absence of test cases for the three reported issues is a legitimate concern. The review should be updated to focus on verifying test coverage rather than claiming the file is missing.
js/formidable_blocks.js (1)
1-1: This is generated build output; actual source changes not provided for review.This file is minified/bundled JavaScript produced by a build tool (likely Webpack). The changes consist of module ID rotation (532→2532, 314→6314), export renaming (A→Ay), and CSS class hash updates — all typical of rebuilding a bundle.
The PR's actual fix (shift-multi-select range logic for fields in/out of sections) is implemented in
js/src/admin/admin.js(per the AI summary: "extracting range construction into a new helpergetSelectedRange"), but that source file was not included in this review.The static analysis hints flagging 85+ errors (hoisting, function reassignment, unsafe finally) are false positives; these patterns are normal in transpiled/minified output and should be ignored.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/src/admin/admin.js(2 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). (1)
- GitHub Check: 🎉 Deploy
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
js/formidable_admin.js.mapis excluded by!**/*.map,!**/*.map
📒 Files selected for processing (1)
js/src/admin/admin.js(2 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). (1)
- GitHub Check: Run PHP Syntax inspection (8.3)
Hi @lauramekaj1 Yes I think I had an illusion it was working as I haven't had a good number of fields inside my section. Those two issues are fixed for me. Could you verify that please? |
@AbdiTolesa I tested it and validated that the issues are fixed. Thank you! |
|
Thanks Abdi and Laura. I want to wait for #2521 to be merged first. |
Fix https://github.com/Strategy11/formidable-pro/issues/6017
Test procedure
CleanShot.2025-10-07.at.17.06.54.mp4