Stop using jQuery in captcha render code#2189
Conversation
WalkthroughThe pull request introduces a new method Changes
Sequence DiagramsequenceDiagram
participant Form as Form Component
participant Captcha as Captcha Renderer
participant Validator as Captcha Validator
Form->>Captcha: renderCaptcha(captcha, selector)
Captcha-->>Captcha: Check existing render
Captcha->>Validator: Validate Captcha
Validator-->>Captcha: Validation Result
Captcha->>Form: Trigger afterRecaptcha callback
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
js/formidable.js (1)
1677-1683: Fix JSDoc block alignmentESLint flagged these lines for alignment issues within the JSDoc comment block. Correcting the indentation will ensure the code adheres to style guidelines.
Apply the following diff to align the comment lines properly:
- /** - * Render a captcha field. - * - * @param {HTMLElement} captcha - * @param {string} captchaSelector - * @return {void} - */ + /** + * Render a captcha field. + * + * @param {HTMLElement} captcha + * @param {string} captchaSelector + * @return {void} + */🧰 Tools
🪛 eslint
[error] 1677-1683: Expected JSDoc block lines to be aligned.
(jsdoc/check-line-alignment)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/formidable.js(1 hunks)
🧰 Additional context used
🪛 eslint
js/formidable.js
[error] 1677-1683: Expected JSDoc block lines to be aligned.
(jsdoc/check-line-alignment)
🔇 Additional comments (4)
js/formidable.js (4)
1685-1685: Good use of 'data-rid' attribute check
Using getAttribute('data-rid') to determine if the captcha is already rendered is a clean approach to prevent duplicate rendering. No issues detected.
1690-1696: Parameter assignments are clear and maintainable
Declaring captcha parameters with const and grouping them in a single object is neat. This approach enhances readability by keeping all config options together.
1698-1704: Optional chaining for DOM lookups is safe
Leveraging optional chaining when searching for the form and the label is a good defensive coding strategy to prevent runtime errors. Hiding the captcha label when size equals "invisible" is logically sound.
1710-1712: Robust fallback with dynamic captcha references
Conditionally using turnstile or a different captcha library is a flexible pattern. Using the captcha ID as the container reference is a neat trick for dynamic usage.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
js/formidable.js (1)
Line range hint
1748-1758: Fix inconsistent turnstile check.The turnstile check should use the provided selector parameter for consistency.
function getSelectedCaptcha( captchaSelector ) { if ( captchaSelector === '.frm-g-recaptcha' ) { return grecaptcha; } - if ( document.querySelector( '.cf-turnstile' ) ) { + if ( captchaSelector === '.cf-turnstile' ) { return turnstile; } return hcaptcha; }
🧹 Nitpick comments (3)
js/formidable.js (3)
1690-1695: Add error handling for missing required attributes.The code assumes that required attributes like
data-sitekeywill always be present. Consider adding validation.const size = captcha.getAttribute( 'data-size' ); +if ( ! captcha.getAttribute( 'data-sitekey' ) ) { + console.error( 'Captcha is missing required data-sitekey attribute' ); + return; +} const params = { sitekey: captcha.getAttribute( 'data-sitekey' ), size: size, theme: captcha.getAttribute( 'data-theme' ) };
1697-1709: Extract callback function for better readability.Consider extracting the callback function to improve code organization.
+function createInvisibleCaptchaCallback( formID ) { + return function( token ) { + frmFrontForm.afterRecaptcha( token, formID ); + }; +} if ( size === 'invisible' ) { const formID = captcha.closest( 'form' )?.querySelector( 'input[name="form_id"]' )?.value; const captchaLabel = captcha.closest( '.frm_form_field' )?.querySelector( '.frm_primary_label' ); if ( captchaLabel ) { captchaLabel.style.display = 'none'; } - params.callback = function( token ) { - frmFrontForm.afterRecaptcha( token, formID ); - }; + params.callback = createInvisibleCaptchaCallback( formID ); }
1710-1712: Simplify turnstile condition.The turnstile condition could be simplified for better readability.
-const activeCaptcha = getSelectedCaptcha( captchaSelector ); -const captchaContainer = typeof turnstile !== 'undefined' && turnstile === activeCaptcha ? '#' + captcha.id : captcha.id; -const captchaID = activeCaptcha.render( captchaContainer, params ); +const activeCaptcha = getSelectedCaptcha( captchaSelector ); +const isTurnstile = typeof turnstile !== 'undefined' && turnstile === activeCaptcha; +const containerId = isTurnstile ? '#' + captcha.id : captcha.id; +const captchaID = activeCaptcha.render( containerId, params );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/formidable.js(1 hunks)
🔇 Additional comments (2)
js/formidable.js (2)
1677-1683: LGTM! Clear function documentation.
The JSDoc comments clearly document the function parameters and purpose.
1685-1688: LGTM! Early return pattern.
Good use of early return pattern to avoid unnecessary processing if the captcha is already rendered.
1 participant
Related issue https://github.com/Strategy11/formidable-pro/issues/5002
This update removes a use of the jQuery
.hide()function, as well as.find(), and 2 uses of.closest().This also updates a lot of uses of
lettoconst, removes some quotes around object keys, and I added some type comments. I also optimized the rendered check, so it happens before we do everything else.And it fixes a bug. The code was intended to hide the invisible captcha field's label, but the
.closest()call was incorrect and it would never select anything.