root web component, make tab navigator accessible as web component#2502
Conversation
…sible in viewport
Liviu-p
changed the title
|
Warning Rate limit exceeded@Crabcyborg has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 9 minutes and 15 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis PR introduces a new web components module for tab navigation and refactors admin script dependency management. It adds a private helper function in FrmAppController for centralizing script dependencies, extends FrmFormsController with an optional template path parameter, establishes a web components base class infrastructure with a tab navigator component, and updates build configuration to support web components bundling. Changes
Sequence Diagram(s)sequenceDiagram
participant DOM as DOM Tree
participant Component as frmTabNavigatorComponent
participant WebComp as frmWebComponent
participant Observer as IntersectionObserver
participant Renderer as DOM Renderer
DOM->>Component: connectedCallback()
Component->>WebComp: render()
WebComp->>Component: initView()
Component->>Renderer: Build wrapper, headings, containers
Renderer->>WebComp: Append style + view to shadowRoot
WebComp->>Observer: whenElementBecomesVisible()
Observer->>Observer: Wait for visibility
Observer->>WebComp: Element visible
WebComp->>Component: afterViewInit(wrapper)
Component->>Component: setInitialUnderlineWidth(wrapper)
Component->>DOM: Set underline width on active tab
Renderer->>DOM: Complete render cycle
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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: 9
🧹 Nitpick comments (16)
js/src/web-components/index.js (1)
1-3: Guard against double registration of custom element.customElements.define throws if the tag is already defined (can happen with duplicated enqueues or if Pro also registers). Add a defensive check.
Apply:
import { frmTabNavigatorComponent } from './frm-tab-navigator-component/frm-tab-navigator-component'; -customElements.define( 'frm-tab-navigator-component', frmTabNavigatorComponent ); +const TAG = 'frm-tab-navigator-component'; +if ( window.customElements && ! customElements.get( TAG ) ) { + customElements.define( TAG, frmTabNavigatorComponent ); +}css/admin/frm-settings-components.js (1)
1-6: Remove empty webpack JS stub css/admin/frm-settings-components.jsNo-op bootstrap emitted for the SCSS-only webpack entry and unused. Evidence: webpack.config.js defines the SCSS entry 'admin/frm-settings-components' -> resources/scss/admin/frm-settings-components.scss (webpack.config.js:42); the repo tracks css/admin/frm-settings-components.js but ripgrep finds no references; FrmAppController only registers the CSS (css/admin/frm-settings-components.css) at classes/controllers/FrmAppController.php:803. Delete css/admin/frm-settings-components.js and configure webpack/MiniCssExtractPlugin to avoid emitting JS stubs for CSS-only entries; verify nothing enqueues that JS.
css/font_icons.js (1)
1-6: Remove css/font_icons.js (empty webpack bootstrap)css/font_icons.js is an empty webpack bootstrap (104 bytes) and unreferenced; the SCSS entry is resources/scss/font_icons.scss → css/font_icons.css (enqueued in classes/controllers/FrmElementorController.php). Delete css/font_icons.js and adjust webpack.config.js (around lines ~138–142 — RemoveEmptyScriptsPlugin / MiniCssExtractPlugin) to prevent emitting JS for SCSS-only entries.
css/frm_admin.js (1)
1-6: Delete stray empty webpack bootstrap JS emitted next to CSS.
css/frm_admin.js contains only an empty webpack bootstrap wrapper (small autogenerated) — delete the file and update the build config so SCSS entries don’t emit JS.resources/scss/admin/components/panel/_options-panel.scss (1)
60-65: Respect reduced‑motion users.Disable transitions when prefers-reduced-motion is set.
Apply this diff:
&:hover { color: var(--grey-700); background-color: #fff; box-shadow: var(--box-shadow-sm); } } + +@media (prefers-reduced-motion: reduce) { + #frm-options-panel h3, + .frm-collapsible { + transition: none; + } + #frm-options-panel h3 .frmsvg, + .frm-collapsible .frmsvg { + transition: none; + } +}classes/controllers/FrmFormsController.php (1)
1608-1608: Document and suppress the “unused” $class param (used by included views).Prevents PHPMD noise while clarifying intent.
Apply this diff:
- public static function mb_tags_box( $form_id, $class = '', $template = 'default' ) { + /** + * Render the advanced info box. + * + * @param int|string $form_id + * @param string $class Used by included templates for additional CSS classes. + * @param string $template 'default'|'new-tab-navigator' + * + * @SuppressWarnings(PHPMD.UnusedFormalParameter) + */ + public static function mb_tags_box( $form_id, $class = '', $template = 'default' ) {js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.css (1)
103-130: Honor prefers‑reduced‑motion and add focus visibility.Small a11y win; avoids unnecessary animation.
Apply this diff:
.frm-tabs-slide-track > div.frm-active { opacity: 1; transition: 0.35s opacity linear; } + +@media (prefers-reduced-motion: reduce) { + .frm-tabs-slide-track { transition: none; } + .frm-tabs-delimiter .frm-tabs-active-underline { transition: none; } +} + +.frm-tabs-navs ul li:focus-visible { + outline: 2px solid var(--primary-500); + outline-offset: 2px; +}webpack.config.js (2)
104-111: Prefer webpack 5 asset modules over raw-loader.Removes an extra dependency and aligns with modern webpack.
Apply this diff:
- { - test: /\.css$/i, - use: ['style-loader', 'css-loader'], - exclude: /-component\.css$/ - }, - { - test: /-component\.css$/i, - use: ['raw-loader'] - }, + { + test: /\.css$/i, + use: ['style-loader', 'css-loader'], + exclude: /-component\.css$/i + }, + { + test: /-component\.css$/i, + type: 'asset/source' + },
106-106: Case-insensitive exclude.Match the -component.css pattern regardless of case.
Apply this diff:
- exclude: /-component\.css$/ + exclude: /-component\.css$/ijs/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.js (2)
96-99: Add basic ARIA/keyboard affordances to tabs.Enables tab focus and assists screen readers.
Apply this diff:
- return `<li class="${className}">${tab.getAttribute( 'data-tab-title' )}</li>`; + return `<li class="${className}" role="tab" aria-selected="${index === 0}" tabindex="0">${tab.getAttribute( 'data-tab-title' )}</li>`;
118-120: Handle non‑shadow DOM in getTabUnderline().Prevents null dereference when data-shadow-dom="false".
Apply this diff:
- return this.shadowRoot.querySelector( '.frm-tabs-active-underline' ); + const root = this.useShadowDom ? this.shadowRoot : this; + return root ? root.querySelector( '.frm-tabs-active-underline' ) : null;js/src/web-components/frm-web-component.js (2)
24-26: Guard against undefined componentStyle.Avoid inserting the literal string "undefined" into the DOM.
- style.textContent = this.componentStyle; + style.textContent = this.componentStyle || '';
20-22: Correct JSDoc return type.The method returns a style element, not a string.
- * @return string + * @return {HTMLStyleElement}js/formidable-web-components.js (3)
206-207: Remove hard-coded data-initial-width="123".This causes a visible jump and magic number. Compute from the active tab or omit and let afterViewInit set it.
- return "<div class=\"frm-tabs-delimiter\">\n\t\t\t<span data-initial-width=\"123\" class=\"frm-tabs-active-underline frm-first\"></span>\n\t\t</div>"; + return "<div class=\"frm-tabs-delimiter\">\n\t\t\t<span class=\"frm-tabs-active-underline frm-first\"></span>\n\t\t</div>";
268-271: getTabUnderline breaks without Shadow DOM.Query the correct root based on data-shadow-dom.
- return this.shadowRoot.querySelector('.frm-tabs-active-underline'); + var root = this.useShadowDom ? this.shadowRoot : this; + return root.querySelector('.frm-tabs-active-underline');
57-58: Unnecessary preventDefault onclicks. No default action to prevent; safe to remove.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
js/formidable-settings-components.js.mapis excluded by!**/*.map,!**/*.mappackage.jsonis excluded by!**/*.json
📒 Files selected for processing (14)
classes/controllers/FrmAppController.php(1 hunks)classes/controllers/FrmFormsController.php(2 hunks)classes/views/shared/mb_adv_info_new.php(1 hunks)css/admin/frm-settings-components.js(1 hunks)css/font_icons.js(1 hunks)css/frm_admin.js(1 hunks)js/formidable-web-components.js(1 hunks)js/formidable_admin.js(1 hunks)js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.css(1 hunks)js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.js(1 hunks)js/src/web-components/frm-web-component.js(1 hunks)js/src/web-components/index.js(1 hunks)resources/scss/admin/components/panel/_options-panel.scss(1 hunks)webpack.config.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
classes/controllers/FrmFormsController.php (2)
js/admin/applications.js (1)
template(54-54)classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-4622)plugin_path(58-60)
js/src/web-components/frm-web-component.js (1)
webpack.config.js (1)
entries(26-45)
js/src/web-components/index.js (1)
js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.js (1)
frmTabNavigatorComponent(5-122)
classes/controllers/FrmAppController.php (1)
classes/helpers/FrmAppHelper.php (1)
plugin_url(65-68)
js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.js (2)
js/src/web-components/frm-web-component.js (1)
frmWebComponent(1-107)js/src/components/class-tabs-navigator.js (1)
frmTabsNavigator(1-89)
classes/views/shared/mb_adv_info_new.php (6)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-4622)show_search_box(1536-1591)classes/models/FrmField.php (2)
FrmField(6-1501)is_no_save_field(1168-1170)classes/helpers/FrmFormsHelper.php (3)
FrmFormsHelper(6-1999)insert_opt_html(615-665)insert_code_html(686-709)classes/helpers/FrmHtmlHelper.php (2)
FrmHtmlHelper(9-124)echo_dropdown_option(50-57)classes/helpers/FrmShortcodeHelper.php (2)
FrmShortcodeHelper(9-141)get_contextual_codes(73-80)classes/controllers/FrmFormsController.php (1)
advanced_helpers(1640-1670)
js/formidable-web-components.js (1)
webpack.config.js (1)
entries(26-45)
🪛 PHPMD (2.15.0)
classes/controllers/FrmFormsController.php
1608-1608: Avoid unused parameters such as '$class'. (undefined)
(UnusedFormalParameter)
🪛 Biome (2.1.2)
js/formidable-web-components.js
[error] 15-15: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 135-135: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 144-144: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 145-145: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 147-147: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 287-287: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 297-297: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 299-299: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 301-301: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 302-302: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
🔇 Additional comments (9)
js/formidable_admin.js (1)
11333-11333: Public export looks good; mind thethisbinding.
maybeCollapseSettingsrelies onthisbeing the collapsible header element. When exposing it publicly, ensure external code calls it as an event handler (e.g.,el.addEventListener('click', frmAdminBuild.maybeCollapseSettings)) or with.call(el, event)to maintain the expected context.Optionally harden the export to accept an explicit context:
- maybeCollapseSettings, + // Preserve expected context whether used as a listener or called directly. + maybeCollapseSettings: function(ctx) { + return ctx ? maybeCollapseSettings.call(ctx) : maybeCollapseSettings.call(this); + },Please confirm the new web component uses it with the correct context (listener or
.call), sothis.classListandthis.nextElementSiblingremain valid.resources/scss/admin/components/panel/_options-panel.scss (1)
12-13: Selector expansion LGTM.Including .frm-collapsible shares styling consistently with panel headings.
js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.css (1)
1-130: Solid base styles for the tab navigator.Structure and state styles read well.
webpack.config.js (2)
37-38: New entry looks correct and matches the admin registration.
1-175: Verify raw-loader availability
- webpack.config.js (root) contains a rule matching /-component.css$/i that uses "raw-loader".
- No package.json found in the repo root to confirm the dependency — either add "raw-loader" to devDependencies or remove/replace its usage (e.g., use webpack asset/source) and update the config accordingly.
classes/controllers/FrmFormsController.php (1)
1608-1635: Template switch is safe and backward compatibleSearch found one call site: classes/views/frm-forms/mb_insert_fields.php (line 9) — it calls mb_tags_box($id) so the default template remains in use. No change required; to opt this view into the new template, call mb_tags_box($id, '', 'new-tab-navigator').
classes/views/shared/mb_adv_info_new.php (1)
6-6: Verify ID uniqueness for taxonomy-linkcategory.This ID is common in WP admin. Ensure no duplicate IDs exist on the page to avoid CSS/JS collisions.
js/formidable-web-components.js (2)
354-356: afterViewInit receives the host, not the wrapper (mirrors src bug).This is the compiled artifact of the base-class issue; fix in src and rebuild.
391-393: IntersectionObserver observing ShadowRoot (compiled).Same as src: observe the host element; fix in src and rebuild.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
js/src/admin/admin.js (1)
11429-11429: Public API relies onthisbinding; make it safe to call imperatively.
maybeCollapseSettingsexpectsthisto be the header element. As a public export, calling it directly (e.g.,frmAdminBuild.maybeCollapseSettings()) or passing it around can throw (noclassListon the wrongthis). Either document usage (“use as an event handler or call with .call(el)”) or make it accept an argument and fall back tothis.Apply this refactor to make it robust for both event handler and direct calls:
- function maybeCollapseSettings() { - /*jshint validthis:true */ - this.classList.toggle( 'frm-collapsed' ); - - // Toggles the "aria-expanded" attribute - const expanded = this.getAttribute( 'aria-expanded' ) === 'true' || false; - this.setAttribute( 'aria-expanded', ! expanded ); - - addSlideAnimationCssVars( this.nextElementSibling ); - } + /** + * Toggle settings section collapse (safe for event handlers and direct calls). + * @param {Event|HTMLElement} targetOrEvent + */ + function maybeCollapseSettings( targetOrEvent ) { + /*jshint validthis:true */ + const el = (this && this.nodeType === 1) + ? this + : (targetOrEvent && (targetOrEvent.currentTarget || targetOrEvent.target || targetOrEvent)); + if ( ! el || ! el.classList ) { + return; + } + el.classList.toggle( 'frm-collapsed' ); + const expanded = el.getAttribute( 'aria-expanded' ) === 'true'; + el.setAttribute( 'aria-expanded', String( ! expanded ) ); + addSlideAnimationCssVars( el.nextElementSibling ); + }Please confirm new call sites (e.g., in web components) either:
- add listeners:
header.addEventListener('click', frmAdminBuild.maybeCollapseSettings), or- call imperatively:
frmAdminBuild.maybeCollapseSettings(headerEl).classes/controllers/FrmFormsController.php (1)
1608-1608: Update docblock for new param and suppress PHPMD for $classDocument $template_path and suppress the UnusedFormalParameter warning since $class may be consumed in the included template.
- /** - * @param int|string $form_id - * @param string $class - * @return void - */ + /** + * @param int|string $form_id + * @param string $class CSS class passed through to the included template. + * @param string $template_path Absolute path to a PHP template to include; pass 'default' (or empty) to use core template. + * @return void + * @SuppressWarnings(PHPMD.UnusedFormalParameter) + */ public static function mb_tags_box( $form_id, $class = '', $template_path = 'default' ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
js/formidable-settings-components.js.mapis excluded by!**/*.map,!**/*.map
📒 Files selected for processing (2)
classes/controllers/FrmFormsController.php(2 hunks)js/src/admin/admin.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
classes/controllers/FrmFormsController.php (1)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-4623)plugin_path(58-60)
🪛 PHPMD (2.15.0)
classes/controllers/FrmFormsController.php
1608-1608: Avoid unused parameters such as '$class'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (1)
classes/controllers/FrmFormsController.php (1)
1629-1635: Harden template_path handling to avoid LFI and brittle includesFinding: only call site in this repo is classes/views/frm-forms/mb_insert_fields.php:9 — FrmFormsController::mb_tags_box($id) — no third arg passed, so current risk is low.
Action: apply the proposed hardening (use realpath, restrict to allowed dirs, require .php) and add the path_is_in_allowed_dirs helper as suggested.
Verify: confirm Views add‑on or any other add‑ons do not call mb_tags_box with user-controlled paths; if they must, require absolute plugin paths or extend frm_tags_box_allowed_template_dirs via the provided filter.
…webpack to compile the scss to css files that may be available in web-components folder
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
js/formidable-web-components.js (2)
1-1: Avoid magic width "123" for underline.Remove hardcoded data-initial-width; fallback to first tab width.
Apply in src and rebuild:
- getTabDelimiter() { - var wrap = document.createElement('div'), underline = document.createElement('span'); - underline.setAttribute('data-initial-width', '123'); + getTabDelimiter() { + var wrap = document.createElement('div'), underline = document.createElement('span'); underline.classList.add('frm-tabs-active-underline', 'frm-first'); wrap.className = 'frm-tabs-delimiter'; wrap.appendChild(underline); return wrap; }- initDefaultSlideTrackerWidth() { - if (this.slideTrackLine.dataset.initialWidth) { - this.slideTrackLine.style.width = `${this.slideTrackLine.dataset.initialWidth}px`; - } - } + initDefaultSlideTrackerWidth() { + if (this.slideTrackLine.dataset.initialWidth) { + this.slideTrackLine.style.width = `${this.slideTrackLine.dataset.initialWidth}px`; + } else if (this.navs && this.navs.length) { + this.slideTrackLine.style.width = this.navs[0].clientWidth + 'px'; + } + }
1-1: Resilience: Recalculate underline on resize.Optional but improves UX when admin resizes the window or nav font changes.
Apply in src and rebuild:
- initView() { + initView() { this.tabs = this.querySelectorAll('.frm-tab'); if (this.tabs.length === 0) return null; var wrap = document.createElement('div'); wrap.classList.add('frm-tabs-wrapper'); wrap.appendChild(this.getTabDelimiter()); wrap.appendChild(this.getTabs()); wrap.appendChild(this.getTabContainer()); - new TabsNavigator(wrap); + this._tabsNavigator = new TabsNavigator(wrap); return wrap; }- afterViewInit(wrap) { - this.setInitialUnderlineWidth(wrap); - } + afterViewInit(wrap) { + this.setInitialUnderlineWidth(wrap); + var self = this; + this._onResize = function () { + self.setInitialUnderlineWidth(wrap); + var active = wrap.querySelector('li.frm-active'); + if (active && self._tabsNavigator) { + var index = Array.from(wrap.querySelectorAll('.frm-tabs-navs li')).indexOf(active); + self._tabsNavigator.initSlideTrackUnderline(active, index); + } + }; + window.addEventListener('resize', this._onResize, { passive: true }); + }- disconnectedCallback() {} + disconnectedCallback() { + if (this._onResize) { + window.removeEventListener('resize', this._onResize); + this._onResize = null; + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.json,!**/*.jsonpackage.jsonis excluded by!**/*.json
📒 Files selected for processing (6)
js/formidable-web-components.js(1 hunks)js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.css(1 hunks)js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.js(1 hunks)js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.scss(1 hunks)js/src/web-components/frm-web-component.js(1 hunks)webpack.config.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.css
- js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.js
- webpack.config.js
🧰 Additional context used
🧬 Code graph analysis (1)
js/src/web-components/frm-web-component.js (1)
webpack.config.js (1)
entries(37-59)
🪛 Biome (2.1.2)
js/formidable-web-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: 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)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Run PHP Syntax inspection (8.3)
- GitHub Check: Cypress
🔇 Additional comments (6)
js/src/web-components/frm-web-component.js (2)
38-45: Fix afterViewInit root parameterWhen Shadow DOM is active,
afterViewInitreceives the host element instead of the shadow root, so any DOM queries in subclasses fail. Pass the wrapper you already computed so components can find their content.- this.whenElementBecomesVisible().then( () => { - this.afterViewInit( this ); - }); + this.whenElementBecomesVisible().then( () => { + this.afterViewInit( wrapper ); + });
72-75: Observe the host element with IntersectionObserver
IntersectionObserver.observethrows when given aShadowRoot(it expects an Element/Document), so Shadow DOM components crash here. Just observe the custom element itself; visibility works for both shadow and light DOM cases.- const element = this.useShadowDom ? this.shadowRoot : this; - - observer.observe( element ); + observer.observe( this );js/formidable-web-components.js (4)
1-1: Fix: NodeList.filter misuse in initSlideTrackUnderline (runtime break)..filter() on NodeList returns an array; later treated as an Element. Use Array.from(...).find(...). Add a guard.
Apply in src and rebuild:
- initSlideTrackUnderline(nav, index) { - this.slideTrackLine.classList.remove('frm-first', 'frm-last'); - var activeNav = void 0 !== nav ? nav : this.navs.filter(function (nav) { - return nav.classList.contains('frm-active'); - }); - var x = this.isRTL - ? -(activeNav.parentElement.offsetWidth - activeNav.offsetLeft - activeNav.offsetWidth) - : activeNav.offsetLeft; - this.slideTrackLine.style.transform = `translateX(${x}px)`; - this.slideTrackLine.style.width = activeNav.clientWidth + 'px'; - } + initSlideTrackUnderline(nav, index) { + this.slideTrackLine.classList.remove('frm-first', 'frm-last'); + var activeNav = typeof nav !== 'undefined' + ? nav + : Array.from(this.navs).find(function (li) { return li.classList.contains('frm-active'); }); + if (!activeNav) return; + var x = this.isRTL + ? -(activeNav.parentElement.offsetWidth - activeNav.offsetLeft - activeNav.offsetWidth) + : activeNav.offsetLeft; + this.slideTrackLine.style.transform = `translateX(${x}px)`; + this.slideTrackLine.style.width = activeNav.clientWidth + 'px'; + }
1-1: A11y: Add role="tablist" to the UL.Required for screen readers.
Apply in src and rebuild:
- getTabs() { + getTabs() { var wrapper = document.createElement('div'), ul = document.createElement('ul'); wrapper.className = 'frm-tabs-navs'; + ul.setAttribute('role', 'tablist'); wrapper.appendChild(ul); Array.from(this.tabs).forEach((tab, index) => { ul.appendChild(this.createTabHeading(tab, index)); }); return wrapper; }
1-1: A11y: Make tabs focusable and identify them as tabs; add Enter/Space keyboard support.Add role, tabindex, aria-selected; update on activation; support Enter/Space.
Apply in src and rebuild:
- createTabHeading(tab, index) { - var className = index === 0 ? 'frm-active' : '', li = document.createElement('li'); - li.className = className; - li.innerText = tab.getAttribute('data-tab-title'); - return li; - } + createTabHeading(tab, index) { + var isActive = index === 0; + var li = document.createElement('li'); + li.className = isActive ? 'frm-active' : ''; + li.setAttribute('role', 'tab'); + li.setAttribute('tabindex', isActive ? '0' : '-1'); + li.setAttribute('aria-selected', isActive ? 'true' : 'false'); + li.id = (this.id ? this.id + '-tab-' + index : 'frm-tab-' + index); + li.innerText = tab.getAttribute('data-tab-title'); + return li; + }- init() { + init() { var _this = this; if (this.wrapper !== null && this.navs.length && this.slideTrackLine !== null && this.slideTrack !== null && this.slides.length) { this.initDefaultSlideTrackerWidth(); this.navs.forEach(function (nav, index) { nav.addEventListener('click', function (event) { return _this.onNavClick(event, index); }); + nav.addEventListener('keydown', function (e) { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + _this.onNavClick({ currentTarget: nav, preventDefault: function () {} }, index); + } + }); }); } }- onNavClick(event, index) { + onNavClick(event, index) { var nav = event.currentTarget; event.preventDefault(); this.removeActiveClassnameFromNavs(); nav.classList.add('frm-active'); + // A11y: update aria-selected and tabindex + this.navs.forEach(function (li, i) { + li.setAttribute('aria-selected', i === index ? 'true' : 'false'); + li.setAttribute('tabindex', i === index ? '0' : '-1'); + }); + if (nav.focus) { nav.focus(); } this.initSlideTrackUnderline(nav, index); this.changeSlide(index); var anchor = nav.querySelector('a'); if (anchor && anchor.id === 'frm_insert_fields_tab' && !anchor.closest('#frm_adv_info')) { window.frmAdminBuild?.clearSettingsBox?.(); } }
1-1: Verification: Ensure fixes are applied in src (not only the bundle) and lint excludes built files.Search for NodeList.filter on NodeList, missing ARIA roles, and keyboard handlers in src. Also make sure the linter ignores compiled bundles.
Run:
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2502 +/- ##
=========================================
Coverage 27.35% 27.35%
- Complexity 8668 8674 +6
=========================================
Files 142 142
Lines 29228 29243 +15
=========================================
+ Hits 7995 8000 +5
- Misses 21233 21243 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
classes/controllers/FrmAppController.php (1)
686-687: Web components are registered but not loaded; add as a dep to settings bundle
formidable-web-componentsis only registered and never enqueued.formidable-settings-componentsshould depend on it so the custom element is defined when settings UI runs. This was flagged earlier and is still pending.Apply this diff:
- wp_register_script( 'formidable-settings-components', $plugin_url . '/js/formidable-settings-components.js', array( 'formidable_admin' ), $version, true ); + wp_register_script( + 'formidable-settings-components', + $plugin_url . '/js/formidable-settings-components.js', + array( 'formidable_admin', 'formidable-web-components' ), + $version, + true + );Optional: For clarity, consider registering
formidable_adminbeforeformidable-web-componentsin admin_js().Also applies to: 815-816
🧹 Nitpick comments (4)
classes/controllers/FrmFormsController.php (2)
1628-1634: Improve error handling for missing template files.The current implementation silently falls back to the default template when a custom template file doesn't exist. While this provides robustness, it could mask configuration errors or deployment issues.
Consider logging a notice when falling back to the default:
if ( 'default' === $template_path || ! file_exists( $template_path ) ) { + if ( 'default' !== $template_path ) { + // Log when falling back due to missing file + error_log( 'Formidable: Custom template not found: ' . $template_path . '. Using default.' ); + } include FrmAppHelper::plugin_path() . '/classes/views/shared/mb_adv_info.php'; return; } +if ( ! is_readable( $template_path ) ) { + error_log( 'Formidable: Custom template not readable: ' . $template_path . '. Using default.' ); + include FrmAppHelper::plugin_path() . '/classes/views/shared/mb_adv_info.php'; + return; +} + include $template_path;
1608-1608: Sanitize the$template_pathto prevent arbitrary file inclusion.Currently
mb_tags_box()is only invoked with the default value, so there’s no immediate risk. To future-proof, resolve and validate the path before including:$base = realpath( FrmAppHelper::plugin_path() ); $real_path = realpath( $template_path ); if ( ! $real_path || strpos( $real_path, $base ) !== 0 || ! is_readable( $real_path ) ) { include $base . '/classes/views/shared/mb_adv_info.php'; return; } include $real_path;css/admin/frm-settings-components.css (1)
1-1: Please avoid duplicating the entire minified rulesetThis addition repeats the whole existing stylesheet (just to tweak the first selector), so we now ship two identical copies of the same rules. That bloats the asset and doubles future maintenance overhead. Instead, adjust only the selector that needs the
.jsvariant—either by editing the original SCSS source or appending a single rule for the new case—so the shared declarations stay defined once.classes/controllers/FrmAppController.php (1)
775-803: Replace placeholder @SInCEUpdate
@since x.xwith the correct plugin version for this release to keep docs accurate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
js/formidable-settings-components.js.mapis excluded by!**/*.map,!**/*.mappackage-lock.jsonis excluded by!**/package-lock.json,!**/*.jsonpackage.jsonis excluded by!**/*.json
📒 Files selected for processing (4)
classes/controllers/FrmAppController.php(2 hunks)classes/controllers/FrmFormsController.php(2 hunks)css/admin/frm-settings-components.css(1 hunks)js/src/admin/admin.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
classes/controllers/FrmFormsController.php (1)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-4617)plugin_path(58-60)
classes/controllers/FrmAppController.php (1)
classes/helpers/FrmAppHelper.php (3)
plugin_url(65-68)FrmAppHelper(6-4617)is_style_editor_page(1940-1959)
🪛 PHPMD (2.15.0)
classes/controllers/FrmFormsController.php
1608-1608: Avoid unused parameters such as '$class'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Run PHP Syntax inspection (8.3)
🔇 Additional comments (2)
js/src/admin/admin.js (1)
11436-11436: API surface update looks goodExporting the existing
maybeCollapseSettingshelper makes sense and keeps the original behavior intact.classes/controllers/FrmAppController.php (1)
692-692: Good refactor: centralized admin JS dependenciesMoving deps into
get_admin_js_dependencies()improves maintainability. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @classes/controllers/FrmFormsController.php:
- Line 1719: The docblock for mb_tags_box is missing a @param for the new
$template_path parameter; update the phpdoc above the mb_tags_box method to
include a @param string $template_path description, and if possible add a scalar
type hint to the method signature (e.g., string $template_path) to improve type
safety while ensuring backward compatibility.
♻️ Duplicate comments (1)
classes/controllers/FrmAppController.php (1)
719-720: Verify if 'formidable-settings-components' needs 'formidable-web-components' as a dependency.Based on the past review comment, if
formidable-settings-componentsuses the custom elements registered byformidable-web-components(e.g.,frm-tab-navigator-component), it should declare it as a dependency to ensure the custom element is defined before the settings code runs.Run the following script to check if the settings components bundle references the web components:
#!/bin/bash # Description: Check if formidable-settings-components uses web components # Check if the built settings components file references the custom element if [ -f js/formidable-settings-components.js ]; then echo "Checking formidable-settings-components.js for web component usage:" rg -n "frm-tab-navigator-component|frmTabNavigatorComponent|customElements" js/formidable-settings-components.js || echo "No web component references found" else echo "Built file js/formidable-settings-components.js not found" fi # Check source files that might be bundled into settings components echo "" echo "Checking source files for web component usage:" fd -e js -e jsx . js/src --exec rg -l "frm-tab-navigator-component|frmTabNavigatorComponent" {} \;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.eslintignoreclasses/controllers/FrmAppController.phpclasses/controllers/FrmFormsController.phpcss/frm_admin.css
🧰 Additional context used
🧬 Code graph analysis (2)
classes/controllers/FrmAppController.php (1)
classes/helpers/FrmAppHelper.php (3)
plugin_url(65-68)FrmAppHelper(6-5054)is_style_editor_page(2128-2147)
classes/controllers/FrmFormsController.php (3)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-5054)plugin_path(58-60)stripe/helpers/FrmTransLiteAppHelper.php (1)
plugin_path(11-13)stripe/helpers/FrmStrpLiteAppHelper.php (1)
plugin_path(16-18)
🪛 GitHub Actions: Inspections
classes/controllers/FrmFormsController.php
[error] 1-1: PHP CS Fixer: Found 1 file with issues (phpdoc_add_missing_param_annotation) in 494 files; missing @param annotation in FrmFormsController.php. The command 'vendor/bin/php-cs-fixer fix --dry-run --verbose' failed with exit code 8.
🪛 GitHub Check: PHPStan
classes/controllers/FrmFormsController.php
[failure] 1719-1719:
Method FrmFormsController::mb_tags_box() has parameter $template_path with no type specified.
🪛 PHPMD (2.15.0)
classes/controllers/FrmFormsController.php
1719-1719: Avoid unused parameters such as '$class'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: Cypress
🔇 Additional comments (4)
.eslintignore (1)
14-14: LGTM!The addition of
js/formidable-web-components.jsto the ESLint ignore list is appropriate, as this is a generated/built file consistent with other ignored assets in the project.classes/controllers/FrmFormsController.php (1)
1740-1746: LGTM! Template path logic is correct.The conditional logic correctly handles both the default template path and custom template paths, with proper fallback behavior when the custom path doesn't exist.
classes/controllers/FrmAppController.php (2)
812-839: LGTM! Well-structured dependency management helper.The new
get_admin_js_dependencies()method effectively centralizes dependency management and conditionally includeswp-color-pickeronly when editing styles, which is a good optimization.
725-725: Good refactoring to use the dependency helper.Replacing the hard-coded dependency array with the new helper method improves maintainability and enables conditional dependency loading.
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @classes/controllers/FrmFormsController.php:
- Around line 1716-1720: The mb_tags_box method signature and docblock should
declare $template_path as a string: update the PHPDoc to replace "@param mixed
$template_path" with "@param string $template_path" and add a string type hint
to the method signature (public static function mb_tags_box(int $form_id, string
$class = '', string $template_path = 'default')) while keeping existing defaults
and other param types intact; locate the mb_tags_box function to apply these
changes and run a quick static check to ensure no signature conflicts.
🧹 Nitpick comments (1)
js/src/admin/admin.js (1)
5755-5765: ExportingmaybeCollapseSettingslooks correct; be mindful ofthisbindingExposing
maybeCollapseSettingsonfrmAdminBuildJSis reasonable and should be backward compatible. The function expectsthisto be the header element (h3/h4) and usesthis.nextElementSibling, so external callers should either:
- use it directly as an event handler (
el.addEventListener('click', frmAdminBuild.maybeCollapseSettings)), or- call it with
Function.prototype.call/apply(frmAdminBuild.maybeCollapseSettings.call(el)),to avoid
thisbeingwindow/undefinedin strict contexts.If you expect wider reuse, consider a thin wrapper that accepts the header element as an explicit parameter (e.g.,
collapseSettings(headerEl)), and internally delegates to the existing function.Also applies to: 11252-11273
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
classes/controllers/FrmFormsController.phpjs/src/admin/admin.jsjs/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.jsjs/src/web-components/frm-web-component.jsjs/src/web-components/index.js
🚧 Files skipped from review as they are similar to previous changes (3)
- js/src/web-components/index.js
- js/src/web-components/frm-web-component.js
- js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.js
🧰 Additional context used
🪛 PHPMD (2.15.0)
classes/controllers/FrmFormsController.php
1720-1720: Avoid unused parameters such as '$class'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: PHP 8 tests in WP 6.9
🔇 Additional comments (2)
js/src/admin/admin.js (1)
1924-1931: JSDoc forfrm_deny_drop_in_repeaterfilter now matches runtime signatureThe updated
@param {boolean} denyDropInRepeatercorrectly documents the first argument passed towp.hooks.applyFilters( 'frm_deny_drop_in_repeater', false, draggable ). No runtime impact; doc improvement looks good.classes/controllers/FrmFormsController.php (1)
1741-1746: No security vulnerability exists. The methodmb_tags_box()is only called from protected admin pages with the default$template_pathvalue ('default'), and there is no mechanism for user input to reach this parameter. The code is already secured by capability checks that terminate execution withwp_die()if the user lacks the required 'frm_edit_forms' permission, and is not exposed to untrusted input.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
classes/controllers/FrmFormsController.php (1)
1716-1722: Add string type hint to$template_pathparameter.The method signature lacks a string type hint for
$template_path, though the docblock documents it as a string and the parameter has a string default value. Adding the type hint aligns with PHP best practices and improves type safety.Additionally, replace the
@since x.xplaceholder on line 1718 with the actual version number (likely 6.27 based on the current release being 6.26.1).Proposed fix
/** * @param int|string $form_id * @param string $class * @param string $template_path The path to a template file to use instead of the default. * - * @since x.x Added $template_path parameter. + * @since 6.27 Added $template_path parameter. * * @return void */ - public static function mb_tags_box( $form_id, $class = '', $template_path = 'default' ) { + public static function mb_tags_box( $form_id, $class = '', string $template_path = 'default' ) {The
$classparameter is used within the included template file (mb_adv_info.php), so it should not be removed.
🧹 Nitpick comments (1)
classes/controllers/FrmFormsController.php (1)
1743-1748: Consider adding path validation if custom template paths are used.The code currently lacks validation beyond
file_exists()when including custom template paths, which could theoretically allow path traversal if a caller passes user-controlled input to$template_path. However, in the current implementation, the only call site uses the default parameter and no custom paths are passed.If this function is intended to support custom template paths in the future, add validation:
- Use
realpath()to verify the path resolves within an allowed directory (e.g., plugin directory)- Validate file extension to ensure only PHP files are included
- Or restrict to a whitelist of allowed template paths
For now, this is a minor defensive hardening suggestion rather than a critical issue.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/controllers/FrmFormsController.php
🧰 Additional context used
🪛 PHPMD (2.15.0)
classes/controllers/FrmFormsController.php
1722-1722: Avoid unused parameters such as '$class'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Cypress
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: PHP 7.4 tests in WP 6.9
Crabcyborg
left a comment
Crabcyborg
left a comment
There was a problem hiding this comment.
Thanks @Liviu-p!
I got this up to date with master, and made some small changes to get it passing the workflows.
I think this is good to merge now.
🚀
Crabcyborg
deleted the
feature/make-style-elements-accessbile-as-web-components
branch
…accessbile-as-web-components root web component, make tab navigator accessible as web component
2 participants
mb_adv_info- related PR: https://github.com/Strategy11/formidable-views/pull/669