Fix tab active underline positioning on initial load and resize#2798
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR removes initial-width/data attributes and frm-first markers from tab underline spans, hides the underline by default via CSS, updates underline init timing and control flow (initSlideTrackUnderline signature changed), removes helper methods, broadens resize observation targets, and normalizes minor HTML whitespace/newline issues. Changes
Sequence Diagram(s)sequenceDiagram
participant Component as FrmTabNavigatorComponent
participant Navigator as FrmTabsNavigator
participant DOM as UnderlineElement
participant Observer as ResizeObserver
rect rgba(200,220,255,0.5)
Note over Component,Navigator: Old Flow
Component->>Component: initView()
Component->>Component: setInitialUnderlineWidth()
Component->>Navigator: initSlideTrackUnderline(nav, index)
Navigator->>DOM: create delimiter + span (data-initial-width, frm-first)
DOM->>Navigator: positioned/sized using index
Observer->>DOM: observed specific scrollbar wrapper
end
rect rgba(200,255,220,0.5)
Note over Component,Navigator: New Flow
Component->>Component: afterViewInit()
Component->>Navigator: initSlideTrackUnderline(nav)
Navigator->>DOM: create delimiter + span (no data attrs / no frm-first)
Navigator->>DOM: auto-detect active nav, set slideTrackLine display:block
Observer->>DOM: observe broader target ('.frm-scrollbar-wrapper, .styling_settings' or body)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
…ideTrackUnderline signature
…al underline width setup
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/components/class-tabs-navigator.js (1)
38-39: Bug:thiscontext lost in event listener callback.
this.cleanupObserversis passed directly as the event listener callback. Whenbeforeunloadfires,thisinsidecleanupObserverswill referencewindow(orundefinedin strict mode), not the class instance. This causesthis.resizeObserverto beundefined, making the cleanup silently fail.Proposed fix - bind the method or use an arrow function
this.setupScrollbarObserver(); // Cleanup observers when page unloads to prevent memory leaks - window.addEventListener( 'beforeunload', this.cleanupObservers ); + window.addEventListener( 'beforeunload', () => this.cleanupObservers() );Alternatively, bind in the constructor:
constructor( wrapper ) { // ... existing code ... this.cleanupObservers = this.cleanupObservers.bind( this ); this.init(); }
🤖 Fix all issues with AI agents
In `@js/src/components/class-tabs-navigator.js`:
- Around line 59-62: initSlideTrackUnderline currently treats this.navs (a
NodeList) like an array and calls .filter, which will throw if nav argument is
omitted; update the method to convert this.navs to an array before filtering
(e.g., Array.from(this.navs).filter(...)) or simply remove the unused fallback
and always require the nav parameter; ensure you then pass the resulting
element(s) into positionUnderlineIndicator so references to
initSlideTrackUnderline, this.navs, and positionUnderlineIndicator are
consistent.
🧹 Nitpick comments (2)
classes/views/styles/_buttons.php (1)
364-366: Minor indentation inconsistency.Line 364 has an extra leading tab compared to the other
<labelelements in this file.Suggested fix
<div class="frm5 frm_form_field"> - <label + <label for="frm_submit_active_border_color" class="frm-style-item-heading"><?php esc_html_e( 'Border Color', 'formidable' ); ?></label>js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.js (1)
125-132: Consider removing unused method.The
getTabUnderline()method is not called anywhere in the codebase and can be safely removed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
classes/views/frm-forms/add_field_links.phpclasses/views/styles/_buttons.phpclasses/views/styles/_field-colors.phpclasses/views/styles/_field-labels.phpclasses/views/styles/_form-messages.phpcss/admin/frm-settings-components.csscss/frm_admin.cssjs/formidable-settings-components.jsjs/formidable-web-components.jsjs/formidable_dashboard.jsjs/src/components/class-tabs-navigator.jsjs/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.cssjs/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.jsjs/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.scssresources/scss/admin/components/settings/_tabs.scssresources/scss/admin/rtl/_settings-components.scss
💤 Files with no reviewable changes (1)
- resources/scss/admin/rtl/_settings-components.scss
🧰 Additional context used
📓 Path-based instructions (2)
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: - Do not make suggestions when we use x.x for a version placeholder. This is used like "@SInCE x.x" in a PHP doc comment, but also like 'x.x' when functions like _deprecated_function() are used.
- Treat these placeholders as acceptable in this codebase.
Files:
classes/views/styles/_field-colors.phpclasses/views/frm-forms/add_field_links.phpclasses/views/styles/_buttons.phpclasses/views/styles/_field-labels.phpclasses/views/styles/_form-messages.php
**/*.js
⚙️ CodeRabbit configuration file
**/*.js: - Do not make suggestions when we use x.x for a version placeholder. This is used like "@SInCE x.x" in a JavaScript doc comment.
- Treat these placeholders as acceptable in this codebase.
Files:
js/formidable-settings-components.jsjs/formidable_dashboard.jsjs/src/components/class-tabs-navigator.jsjs/formidable-web-components.jsjs/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.js
🧬 Code graph analysis (1)
js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.js (1)
js/src/components/class-tabs-navigator.js (1)
frmTabsNavigator(1-129)
🪛 Biome (2.1.2)
js/formidable-settings-components.js
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
js/formidable_dashboard.js
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
js/formidable-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)
🔇 Additional comments (18)
classes/views/frm-forms/add_field_links.php (1)
10-10: LGTM!Removing the hardcoded
data-initial-widthattribute is correct. The underline width is now dynamically calculated by the JavaScript usingelement.clientWidthinpositionUnderlineIndicator(), which properly handles initial load and resize events.js/formidable_dashboard.js (1)
1-1: Auto-generated minified file — static analysis warnings are false positives.The Biome errors (e.g.,
noInvalidUseBeforeDeclaration,noFunctionAssign) are artifacts of the minification/bundling process. Patterns like IIFE variable hoisting and function expression assignments in minified output commonly trigger these rules but are valid JavaScript at runtime.No action needed on this generated file.
classes/views/styles/_field-colors.php (1)
8-8: LGTM!Removal of
data-initial-widthandfrm-firstclass aligns with the refactored initialization flow. The underline positioning is now handled dynamically in JavaScript, making these static attributes unnecessary.js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.scss (1)
95-95: LGTM!Hiding the underline by default with
display: noneis the correct approach to prevent visual glitches during initial load. The JavaScript setsdisplay: blockafter calculating the correct position ininit(), ensuring the underline appears only when properly positioned.js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.css (1)
1-1: Compiled CSS matches source SCSS.This is the auto-generated minified output. The
display:nonerule for.frm-tabs-delimiter .frm-tabs-active-underlinecorrectly reflects the SCSS source changes.css/admin/frm-settings-components.css (1)
1-1: Auto-generated minified CSS file looks correct.This compiled CSS includes the
display: nonerule for.frm-tabs-active-underline, consistent with the SCSS source changes. The AI summary mentioned duplication, but examining the file shows a single minified line—likely an artifact of diff comparison rather than actual duplication.classes/views/styles/_form-messages.php (2)
8-8: Underline element simplified correctly.Removing
data-initial-widthandfrm-firstclass aligns with the refactored approach where the JS (class-tabs-navigator.js) now dynamically calculates and sets the underline width based on the active tab element, rather than relying on hardcoded initial values.
21-23: Whitespace normalization is acceptable.These are purely formatting changes (multi-line label tag attributes) with no functional impact.
resources/scss/admin/components/settings/_tabs.scss (1)
97-107: Good approach: hide underline by default, show after JS initialization.Setting
display: noneprevents the underline from appearing incorrectly positioned before JavaScript calculates and sets the correct position and width. The JS (class-tabs-navigator.js) setsthis.slideTrackLine.style.display = "block"after callinginitSlideTrackUnderline(), ensuring the underline only appears once properly positioned.js/formidable-settings-components.js (1)
1-1: Static analysis warnings are false positives for minified code.The Biome warnings about "variable used before declaration" and "function reassignment" are expected false positives when analyzing minified/bundled JavaScript. Minifiers reorganize code, hoist variables, and reuse single-letter identifiers across scopes in ways that confuse static analyzers. The source files (e.g.,
js/src/components/class-tabs-navigator.js) should be the targets for static analysis, not the compiled output.This auto-generated bundle contains the updated
frmTabsNavigatorclass with the corrected underline initialization logic.classes/views/styles/_field-labels.php (2)
8-8: Consistent with other template files.The underline element simplification matches the pattern in
_form-messages.phpand other style templates, ensuring uniform behavior across all tab components.
21-24: Whitespace normalization looks fine.Consistent formatting across label elements with no functional changes.
classes/views/styles/_buttons.php (1)
8-8: LGTM!The underline element simplification aligns with the PR objectives - removing
data-initial-widthandfrm-firstclass allows the JS initialization logic to handle positioning consistently.js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.js (2)
31-33: LGTM!Moving
frmTabsNavigatorinitialization toafterViewInitensures the DOM structure is fully rendered before the tab navigator sets up event listeners and positions the underline. This is the correct approach for fixing the initial load positioning issue.
40-49: LGTM!The simplified underline element creation aligns with the refactored initialization flow. The
frmTabsNavigatorclass now handles all positioning logic, so thedata-initial-widthattribute andfrm-firstclass are no longer needed.js/formidable-web-components.js (1)
1-1: Auto-generated bundle file - static analysis warnings are false positives.This is a minified bundle output. The Biome static analysis warnings about "variable used before declaration" and "function reassignment" are false positives caused by:
- Minification transformations that reorganize code
- IIFE patterns and variable hoisting in bundled output
- Class transpilation patterns
The actual source code should be reviewed in
js/src/components/class-tabs-navigator.jsandjs/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.jsinstead.js/src/components/class-tabs-navigator.js (2)
31-35: LGTM!Good improvement - initializing the underline for the active tab during setup and explicitly showing it with
display: 'block'ensures correct positioning on initial load. This addresses the core issue described in the PR.
68-80: LGTM!Expanding the
ResizeObservertarget to include.styling_settingsas a fallback ensures the underline repositions correctly on the Style page. The fallback todocument.bodyprovides resilience if neither container is found.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
shervElmi
requested review from
Crabcyborg
and removed request for
Crabcyborg and
Liviu-p
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@js/formidable-settings-components.js`:
- Line 1: The fallback in w.initSlideTrackUnderline calls .filter() on a
NodeList (this.navs.filter(...)) which will throw—change initSlideTrackUnderline
(and its call sites) to convert this.navs to an Array before using .filter (e.g.
Array.from(this.navs).filter(...)) or use Array.prototype.find to get the active
nav element, then pass that element to positionUnderlineIndicator; also ensure
cleanupObservers retains the proper this by binding it when adding the
beforeunload listener in w.init (use this.cleanupObservers.bind(this)).
🧹 Nitpick comments (1)
js/formidable-settings-components.js (1)
1-1: BindcleanupObserversto the instance.
beforeunloadcallbacks run withthis === window, socleanupObserverswon’t seethis.resizeObserver. Binding avoids silent no‑ops.♻️ Proposed fix
- window.addEventListener("beforeunload",this.cleanupObservers) + window.addEventListener("beforeunload",this.cleanupObservers.bind(this))
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| PHP |  |
Mar 5, 2026 1:16p.m. | Review ↗ |
| JavaScript | |
Mar 5, 2026 1:16p.m. | Review ↗ |
![deepsource-io[bot]](https://avatars.githubusercontent.com/in/16372?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (2)
js/src/components/class-tabs-navigator.js (2)
39-39:⚠️ Potential issue | 🟡 MinorBind
cleanupObserverswhen attachingbeforeunload.Line 39 passes an unbound method reference, so
cleanupObserverscan execute with the wrongthiscontext.💡 Proposed fix
init() { + this.handleBeforeUnload = this.cleanupObservers.bind( this ); // ... - window.addEventListener( 'beforeunload', this.cleanupObservers ); + window.addEventListener( 'beforeunload', this.handleBeforeUnload ); }#!/bin/bash # Verify unbound beforeunload listener usage. rg -nP --type=js "addEventListener\\(\\s*'beforeunload'\\s*,\\s*this\\.cleanupObservers\\s*\\)" js/src/components/class-tabs-navigator.js js/formidable_dashboard.js js/formidable-settings-components.js🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/src/components/class-tabs-navigator.js` at line 39, The unbound method reference passed to window.addEventListener('beforeunload', this.cleanupObservers) can run with the wrong this; bind the handler when attaching and ensure the same bound reference is used when removing it. Replace the raw method reference with a bound version (e.g., this.cleanupObservers = this.cleanupObservers.bind(this) in the constructor or store a boundHandler property and pass that to window.addEventListener) so cleanupObservers runs with the correct instance context and can be removed reliably later.
59-61:⚠️ Potential issue | 🟠 MajorFix fallback in
initSlideTrackUnderline(Line 60).The fallback uses
.filter()on aNodeList, and also passes a collection intopositionUnderlineIndicator(expects one element). This is a latent runtime break if the method is called withoutnav.💡 Proposed fix
initSlideTrackUnderline( nav ) { - const activeNav = nav !== undefined ? nav : this.navs.filter( nav => nav.classList.contains( 'frm-active' ) ); - this.positionUnderlineIndicator( activeNav ); + const activeNav = nav ?? Array.from( this.navs ).find( navItem => navItem.classList.contains( 'frm-active' ) ); + if ( ! activeNav ) { + return; + } + this.positionUnderlineIndicator( activeNav ); }#!/bin/bash # Verify fallback implementation and callsites for initSlideTrackUnderline. rg -nP --type=js 'initSlideTrackUnderline\s*\(' js/src/components/class-tabs-navigator.js js/src/dashboard.js js/src/settings-components/components/tabs-component.js js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.js rg -nP --type=js 'this\.navs\.filter\s*\(' js/src/components/class-tabs-navigator.js🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/src/components/class-tabs-navigator.js` around lines 59 - 61, initSlideTrackUnderline currently falls back to this.navs.filter(...), returning a collection and passing it into positionUnderlineIndicator which expects a single element; change the fallback to pick a single element from this.navs (e.g., Array.from(this.navs).find(el => el.classList.contains('frm-active')) or this.navs.find if already an array) and pass that element to positionUnderlineIndicator (or bail out if none found). Update initSlideTrackUnderline to compute const activeNav = nav ?? singleActiveElement and then call this.positionUnderlineIndicator(activeNav) only when activeNav is a single element.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@js/src/components/class-tabs-navigator.js`:
- Line 39: The unbound method reference passed to
window.addEventListener('beforeunload', this.cleanupObservers) can run with the
wrong this; bind the handler when attaching and ensure the same bound reference
is used when removing it. Replace the raw method reference with a bound version
(e.g., this.cleanupObservers = this.cleanupObservers.bind(this) in the
constructor or store a boundHandler property and pass that to
window.addEventListener) so cleanupObservers runs with the correct instance
context and can be removed reliably later.
- Around line 59-61: initSlideTrackUnderline currently falls back to
this.navs.filter(...), returning a collection and passing it into
positionUnderlineIndicator which expects a single element; change the fallback
to pick a single element from this.navs (e.g., Array.from(this.navs).find(el =>
el.classList.contains('frm-active')) or this.navs.find if already an array) and
pass that element to positionUnderlineIndicator (or bail out if none found).
Update initSlideTrackUnderline to compute const activeNav = nav ??
singleActiveElement and then call this.positionUnderlineIndicator(activeNav)
only when activeNav is a single element.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cb13b86c-af82-4643-aa6d-39ba27d37865
📒 Files selected for processing (9)
css/admin/frm-settings-components.csscss/frm_admin.cssjs/formidable-settings-components.jsjs/formidable-web-components.jsjs/formidable_dashboard.jsjs/src/components/class-tabs-navigator.jsjs/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.cssjs/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.jsjs/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.scss
🚧 Files skipped from review as they are similar to previous changes (4)
- js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.js
- js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.scss
- css/admin/frm-settings-components.css
- js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.css
Crabcyborg
left a comment
Crabcyborg
left a comment
There was a problem hiding this comment.
Sorry Sherv! This PR must have slipped by me.
This looks good to me.
🚀
…itioning Fix tab active underline positioning on initial load and resize
2 participants
Fixes tab active underline positioning issues across all pages (Views, Builder, Style pages) for both initial load and window/container resize events.
Issue URL
https://strategy11.slack.com/archives/C799A2R61/p1768337654554679?thread_ts=1768327743.851509&cid=C799A2R61
Pro PR
Screen recording demo
Before
https://share.cleanshot.com/t28NMS91
After
https://share.cleanshot.com/GqBMfCll
Summary by CodeRabbit
Refactor
Style