Add new FF ESLint rules#2981
Merged
Merged
Conversation
…ectorAll and similar DOM methods
…g DocumentFragment
… formatting improvements
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| PHP |  |
Mar 4, 2026 7:39p.m. | Review ↗ |
| JavaScript | |
Mar 4, 2026 7:39p.m. | Review ↗ |
Contributor
📝 WalkthroughWalkthroughAdds three new custom ESLint rules and registers them; updates DeepSource and ESLint configurations and package.json; modernizes many JS files (template literals, destructuring, let/const); adds a workflow principle doc entry and updates ESLint rules documentation. Changes
Sequence Diagram(s)(Skipped — changes are a set of linting rules, config updates, and widespread stylistic edits that do not introduce a new multi-component runtime control flow.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![deepsource-io[bot]](https://avatars.githubusercontent.com/in/16372?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2981 +/- ##
============================================
+ Coverage 26.60% 26.66% +0.05%
+ Complexity 8882 8880 -2
============================================
Files 145 145
Lines 29932 29918 -14
============================================
+ Hits 7964 7977 +13
+ Misses 21968 21941 -27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…mprove code clarity
…itch statement patterns for better code quality
…or better code clarity
…ral into single line
Revert cf8960a since JS-0119 is now suppressed via init-declarations: off in eslint.config.mjs, making the manual variable initialization changes unnecessary.
- Add globals.jquery to ESLint languageOptions so jQuery is recognized as a JSDoc type without per-file comments - Add import/no-unresolved:off to suppress webpack resolver errors (JS-W1029, JS-R1001, JS-E1007, JS-W1028, JS-C1001) - Add node resolver fallback in import/resolver settings - Add dependency_file_paths and react plugin to .deepsource.toml - Remove redundant global comments from style.js, formidable_admin_global.js, formidable.js, admin.js - Fix JS-0066: !! to Boolean() in checkForMultiselectKeys - Fix JS-W1044: optional chaining for newRowId, button.dataset - Fix JS-0021: Object.prototype.hasOwnProperty.call in admin.js
…in mouseup handler
…ptions.length to prevent prototype pollution and null reference errors
…tructuring for forms.length
…ions for loop counters and common abbreviations
… import/named and import/default suppressions
…line to package.json
…mprove variable naming in admin.js
…ingle-letter variables m and b with descriptive docMarginTop and bodyMarginTop
…reformatting template literal for better readability in admin.js
…et in formidable.js
Contributor
Author
|
@Crabcyborg I’ve also enabled some warnings for best practices that we should address carefully. They don’t affect whether ESLint fails or not, and they’re mostly about performance, new patterns, and general best practices. I’ll handle them in a separate PR since they need more QA. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
eslint-rules/rules/prefer-document-fragment.js (2)
78-88: Same guard needed inCallExpression:exit.The exit handler also needs to guard against computed properties to stay in sync with the entry logic.
🛡️ Suggested fix
'CallExpression:exit'( node ) { const { callee } = node; if ( callee.type === 'MemberExpression' ) { - const methodName = callee.property.name; + const methodName = callee.property.type === 'Identifier' ? callee.property.name : null; - if ( methodName === 'forEach' || methodName === 'map' ) { + if ( methodName && ( methodName === 'forEach' || methodName === 'map' ) ) { exitLoop(); } } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eslint-rules/rules/prefer-document-fragment.js` around lines 78 - 88, The exit handler for "CallExpression:exit" currently assumes callee.property is an identifier and reads callee.property.name; add the same computed-property guard used in the entry logic: first check that callee.type === 'MemberExpression' and that callee.computed is false (or that callee.property.type === 'Identifier') before accessing callee.property.name and deciding if methodName is 'forEach' or 'map', then call exitLoop(); update the condition around the methodName extraction in the CallExpression:exit block to mirror the entry guard.
55-75: Guard against computed property names.Similar to
no-repeated-selector, accessingcallee.property.namewithout checking if it's anIdentifiercould returnundefinedfor computed member expressions.🛡️ Suggested defensive check
CallExpression( node ) { const { callee } = node; // Check for .forEach/.map if ( callee.type === 'MemberExpression' ) { - const methodName = callee.property.name; + const methodName = callee.property.type === 'Identifier' ? callee.property.name : null; + + if ( ! methodName ) { + return; + } if ( methodName === 'forEach' || methodName === 'map' ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eslint-rules/rules/prefer-document-fragment.js` around lines 55 - 75, The code assumes callee.property is an Identifier by reading callee.property.name; guard against computed member expressions by checking callee.property exists and callee.property.type === 'Identifier' (or that callee.computed is false) before reading .name, then proceed to set methodName and run the forEach/map and appendChild/append/prepend checks (symbols: callee.property.name, MemberExpression, methodName, isInLoop, context.report, node). If the property is not an Identifier, skip the methodName logic so computed properties don't cause undefined accesses or false positives.eslint-rules/rules/no-optional-chaining-queryselectorall.js (1)
49-53: Guard against computed property names.Same pattern as the other rules -
callee.property.namecould be undefined for computed member expressions likeobj['querySelectorAll']().🛡️ Suggested defensive check for line 49
- const methodName = callee.property.name; + const methodName = callee.property.type === 'Identifier' ? callee.property.name : null; - if ( ! alwaysReturns.has( methodName ) ) { + if ( ! methodName || ! alwaysReturns.has( methodName ) ) { return; }Also applies to: 91-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eslint-rules/rules/no-optional-chaining-queryselectorall.js` around lines 49 - 53, The rule currently assumes callee.property.name exists (used alongside the alwaysReturns set) which breaks for computed member expressions; update both places that access callee.property.name to first check that callee.property exists and is an Identifier (e.g., callee.property && callee.property.type === 'Identifier') before reading .name, and only then compare against the alwaysReturns set (apply the same defensive check for the other occurrence around lines referencing callee.property.name / alwaysReturns).eslint-rules/rules/no-repeated-selector.js (1)
96-100: Guard against computed property names.If
callee.propertyis a computed expression (e.g.,obj[methodName]()), accessing.namewill beundefined. This is minor since such patterns are rare for DOM methods, but adding a guard improves robustness.🛡️ Suggested defensive check
- const methodName = callee.property.name; + const methodName = callee.property.type === 'Identifier' ? callee.property.name : null; - if ( methodName === 'querySelector' || methodName === 'querySelectorAll' ) { + if ( methodName && ( methodName === 'querySelector' || methodName === 'querySelectorAll' ) ) { trackSelector( node, methodName ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eslint-rules/rules/no-repeated-selector.js` around lines 96 - 100, The code reads callee.property.name unguarded which fails for computed properties (e.g. obj[methodName]()); update the check in the block that sets methodName so you only read .name when callee.property exists and is an Identifier (or when the MemberExpression is not computed), e.g. guard callee.property && callee.property.type === 'Identifier' before assigning methodName and only call trackSelector(node, methodName) when methodName is a string and equals 'querySelector' or 'querySelectorAll'; this prevents undefined names and avoids calling trackSelector with invalid methodName.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@eslint-rules/rules/no-optional-chaining-queryselectorall.js`:
- Around line 101-108: The current fixer in fix(fixer) uses
sourceCode.getText(object) and sourceCode.getText(node.property) and always
emits `${objectText}.${propertyText}`, which breaks computed accesses
(node.computed true) by turning result?.[0] into result.0; update the fixer to
check node.computed and, when true, emit bracket notation preserving the
property expression (e.g., `${objectText}[${propertyText}]`), otherwise keep the
dot form; continue to use sourceCode.getText(object) and
sourceCode.getText(node.property) and call fixer.replaceText(node, fixedText) as
before.
In `@js/formidable.js`:
- Line 1085: The forEach callback uses an expression-bodied arrow which creates
an implicit return and triggers the lint rule; change the callback passed to
endReplace.forEach to use a block body (i.e., replace the expression-bodied
arrow with a brace-enclosed function body that calls el.remove(); ) so the
callback has an explicit statement rather than an implicit return, keeping the
call site in js/formidable.js (endReplace.forEach) otherwise unchanged.
In `@stripe/js/frmstrp.js`:
- Around line 317-322: The code unconditionally appends a first/last name
fragment onto cardObject.name which can result in the string "undefined" being
added; fix by only appending when the retrieved value is non-empty: call
getNameFieldValue(lastFieldContainer, 'last') and only append if it returns a
truthy/non-empty string (trimmed), and likewise only append lastField.value if
lastField && lastField.value is truthy; implement using conditional
concatenation like: if (value) cardObject.name = `${cardObject.name} ${value}`
(ensuring cardObject.name is initialized), so neither getNameFieldValue nor
lastField.value can leak "undefined" into cardObject.name.
---
Nitpick comments:
In `@eslint-rules/rules/no-optional-chaining-queryselectorall.js`:
- Around line 49-53: The rule currently assumes callee.property.name exists
(used alongside the alwaysReturns set) which breaks for computed member
expressions; update both places that access callee.property.name to first check
that callee.property exists and is an Identifier (e.g., callee.property &&
callee.property.type === 'Identifier') before reading .name, and only then
compare against the alwaysReturns set (apply the same defensive check for the
other occurrence around lines referencing callee.property.name / alwaysReturns).
In `@eslint-rules/rules/no-repeated-selector.js`:
- Around line 96-100: The code reads callee.property.name unguarded which fails
for computed properties (e.g. obj[methodName]()); update the check in the block
that sets methodName so you only read .name when callee.property exists and is
an Identifier (or when the MemberExpression is not computed), e.g. guard
callee.property && callee.property.type === 'Identifier' before assigning
methodName and only call trackSelector(node, methodName) when methodName is a
string and equals 'querySelector' or 'querySelectorAll'; this prevents undefined
names and avoids calling trackSelector with invalid methodName.
In `@eslint-rules/rules/prefer-document-fragment.js`:
- Around line 78-88: The exit handler for "CallExpression:exit" currently
assumes callee.property is an identifier and reads callee.property.name; add the
same computed-property guard used in the entry logic: first check that
callee.type === 'MemberExpression' and that callee.computed is false (or that
callee.property.type === 'Identifier') before accessing callee.property.name and
deciding if methodName is 'forEach' or 'map', then call exitLoop(); update the
condition around the methodName extraction in the CallExpression:exit block to
mirror the entry guard.
- Around line 55-75: The code assumes callee.property is an Identifier by
reading callee.property.name; guard against computed member expressions by
checking callee.property exists and callee.property.type === 'Identifier' (or
that callee.computed is false) before reading .name, then proceed to set
methodName and run the forEach/map and appendChild/append/prepend checks
(symbols: callee.property.name, MemberExpression, methodName, isInLoop,
context.report, node). If the property is not an Identifier, skip the methodName
logic so computed properties don't cause undefined accesses or false positives.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 52552978-daa0-4134-9ec7-cb0d3bdc82e4
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (56)
.deepsource.toml.windsurf/rules/enterprise/code-change-principles.md.windsurf/skills/eslint-rules/SKILL.mdeslint-rules/index.jseslint-rules/rules/no-optional-chaining-queryselectorall.jseslint-rules/rules/no-repeated-selector.jseslint-rules/rules/prefer-document-fragment.jseslint.config.mjsjs/addons-page.jsjs/admin/applications.jsjs/admin/deactivation-feedback.jsjs/admin/dom.jsjs/admin/embed.jsjs/admin/legacy-views.jsjs/admin/settings.jsjs/admin/style.jsjs/form-templates.jsjs/formidable-settings-components.jsjs/formidable-web-components.jsjs/formidable.jsjs/formidable_admin.jsjs/formidable_admin_global.jsjs/formidable_blocks.jsjs/formidable_dashboard.jsjs/formidable_overlay.jsjs/frm_testing_mode.jsjs/onboarding-wizard.jsjs/packages/floating-links/s11-floating-links.jsjs/plugin-search.jsjs/src/addons-page/elements/elements.jsjs/src/admin/addon-state.jsjs/src/admin/admin.jsjs/src/admin/upgrade-popup.jsjs/src/common/components/icon.jsjs/src/common/components/radio.jsjs/src/components/class-overlay.jsjs/src/components/class-tabs-navigator.jsjs/src/core/utils/animation.jsjs/src/form-templates/events/favoriteButtonListener.jsjs/src/form/block.jsjs/src/form/calculator.jsjs/src/form/graph.jsjs/src/form/inspector.jsjs/src/form/modal.jsjs/src/form/views.jsjs/src/frm_testing_mode.jsjs/src/settings-components/components/slider-component.jspackage.jsonsquare/js/frontend.jssquare/js/settings.jsstripe/js/connect_settings.jsstripe/js/frmstrp.jsstripe/js/frmtrans_admin.jstests/cypress/e2e/Forms/formPageDataValidation.cy.jstests/cypress/e2e/Forms/formsSettings.cy.jstests/cypress/e2e/admin.cy.js
Comment on lines
+101
to
+108
| fix( fixer ) { | ||
| // Replace ?. with . | ||
| const objectText = sourceCode.getText( object ); | ||
| const propertyText = sourceCode.getText( node.property ); | ||
| const fixedText = `${ objectText }.${ propertyText }`; | ||
|
|
||
| return fixer.replaceText( node, fixedText ); | ||
| }, |
Contributor
There was a problem hiding this comment.
Fix may produce invalid code for computed properties.
If node.computed is true (e.g., result?.[0]), the fix outputs result.0 instead of result[0]. Consider preserving bracket notation for computed accesses.
🐛 Proposed fix for computed property handling
fix( fixer ) {
// Replace ?. with .
const objectText = sourceCode.getText( object );
const propertyText = sourceCode.getText( node.property );
- const fixedText = `${ objectText }.${ propertyText }`;
+ const fixedText = node.computed
+ ? `${ objectText }[${ propertyText }]`
+ : `${ objectText }.${ propertyText }`;
return fixer.replaceText( node, fixedText );
},📝 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
| fix( fixer ) { | |
| // Replace ?. with . | |
| const objectText = sourceCode.getText( object ); | |
| const propertyText = sourceCode.getText( node.property ); | |
| const fixedText = `${ objectText }.${ propertyText }`; | |
| return fixer.replaceText( node, fixedText ); | |
| }, | |
| fix( fixer ) { | |
| // Replace ?. with . | |
| const objectText = sourceCode.getText( object ); | |
| const propertyText = sourceCode.getText( node.property ); | |
| const fixedText = node.computed | |
| ? `${ objectText }[${ propertyText }]` | |
| : `${ objectText }.${ propertyText }`; | |
| return fixer.replaceText( node, fixedText ); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@eslint-rules/rules/no-optional-chaining-queryselectorall.js` around lines 101
- 108, The current fixer in fix(fixer) uses sourceCode.getText(object) and
sourceCode.getText(node.property) and always emits
`${objectText}.${propertyText}`, which breaks computed accesses (node.computed
true) by turning result?.[0] into result.0; update the fixer to check
node.computed and, when true, emit bracket notation preserving the property
expression (e.g., `${objectText}[${propertyText}]`), otherwise keep the dot
form; continue to use sourceCode.getText(object) and
sourceCode.getText(node.property) and call fixer.replaceText(node, fixedText) as
before.
Crabcyborg
reviewed
Mar 4, 2026
| const basedUrlParams = '?action=frm_' + type + '_search&nonce=' + frmGlobal.nonce; | ||
| const elements = ! container ? jQuery( '.frm-' + type + '-search' ) : jQuery( container ).find( '.frm-' + type + '-search' ); | ||
| const basedUrlParams = `?action=frm_${ type }_search&nonce=${ frmGlobal.nonce }`; | ||
| const elements = ! container ? jQuery( `.frm-${ type }-search` ) : jQuery( container ).find( `.frm-${ type }-search` ); |
Contributor
There was a problem hiding this comment.
@shervElmi I have a PHP sniff for a pattern like this.
If a ternary (or an if/else pair) starts with a negative expression, the condition makes more sense flipped in most cases (unless it's something like !== undefined. A "double false" sort of check can be left alone).
I'd prefer:
const elements = container ? jQuery( container ).find( `.frm-${ type }-search` ) : jQuery( `.frm-${ type }-search` );
Crabcyborg
reviewed
Mar 4, 2026
|
|
||
| // Enforce frm-javascript.md patterns | ||
| 'no-var': 'warn', | ||
| 'prefer-const': 'warn', |
Contributor
There was a problem hiding this comment.
@shervElmi I tried removing this rule exception for Oxlint.
It catches another case though.
× eslint(prefer-const): `notEmptySearchText` is never reassigned.
╭─[js/src/core/page-skeleton/events/categoryListener.js:38:46]
37 │ const newSelectedCategory = clickedCategory.getAttribute( 'data-category' );
38 │ let { selectedCategory, selectedCategoryEl, notEmptySearchText } =
· ──────────────────
39 │ getState();
╰────
help: Use `const` instead.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/formidable.js (1)
1410-1413:⚠️ Potential issue | 🔴 CriticalGuard
previousSiblingbefore dereferencingnodeName.Line 1412 assigns
element.previousSibling, which can benull. The loop body immediately accesseselement.nodeNameon line 1413 without a null check, causing a runtime TypeError when traversing past the first sibling.Suggested fix
do { element = element.previousSibling; + if ( ! element ) { + break; + } if ( [ 'input', 'select', 'textarea' ].includes( element.nodeName.toLowerCase() ) ) { focusInput( element ); break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/formidable.js` around lines 1410 - 1413, The loop that walks siblings assigns element = element.previousSibling and immediately dereferences element.nodeName; guard against element being null before accessing nodeName by checking if element is truthy (or breaking when null) inside the do/while so you don't call element.nodeName when previousSibling is null; update the block around element.previousSibling and the call to focusInput(element) (referencing element.previousSibling, element.nodeName, and focusInput) to only evaluate nodeName and call focusInput when element is non-null.
♻️ Duplicate comments (1)
js/formidable.js (1)
1083-1084:⚠️ Potential issue | 🟡 MinorUse a block body in this
forEachcallback to clear the Biome error.Line 1084 still uses an expression-bodied callback, which keeps triggering
lint/suspicious/useIterableCallbackReturn.Suggested fix
- endReplace.forEach( el => el.remove() ); + endReplace.forEach( el => { + el.remove(); + } );#!/bin/bash # Verify callback style at the reported site. rg -n -C2 'endReplace\.forEach\(' js/formidable.js🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/formidable.js` around lines 1083 - 1084, The arrow callback passed to endReplace.forEach currently uses an expression body (endReplace.forEach(el => el.remove())), which triggers the lint rule; change it to a block-body callback (endReplace.forEach(el => { el.remove(); })) so the callback has an explicit statement body—update the callback in the code that invokes endReplace.forEach to use a block with el.remove() inside.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@js/formidable.js`:
- Around line 1410-1413: The loop that walks siblings assigns element =
element.previousSibling and immediately dereferences element.nodeName; guard
against element being null before accessing nodeName by checking if element is
truthy (or breaking when null) inside the do/while so you don't call
element.nodeName when previousSibling is null; update the block around
element.previousSibling and the call to focusInput(element) (referencing
element.previousSibling, element.nodeName, and focusInput) to only evaluate
nodeName and call focusInput when element is non-null.
---
Duplicate comments:
In `@js/formidable.js`:
- Around line 1083-1084: The arrow callback passed to endReplace.forEach
currently uses an expression body (endReplace.forEach(el => el.remove())), which
triggers the lint rule; change it to a block-body callback
(endReplace.forEach(el => { el.remove(); })) so the callback has an explicit
statement body—update the callback in the code that invokes endReplace.forEach
to use a block with el.remove() inside.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: adf8e94a-fcd8-4737-9ece-75b3e0cb3a99
📒 Files selected for processing (1)
js/formidable.js
Crabcyborg
approved these changes
Mar 4, 2026
Contributor
Crabcyborg
left a comment
Crabcyborg
left a comment
There was a problem hiding this comment.
Thanks @shervElmi!
I left a couple comments but I think we can make more changes in future PRs.
If you can keep trying to come up with new rules, that would be great as well.
I'm not 100% sure the template strings are all easier to read, and I'm not totally sure about the one-var rule either, but I'm fine with adding them.
🚀
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds several new FF ESLint rules to improve code quality and consistency.
Summary by CodeRabbit
Chores
Tests
Documentation