Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request adds comprehensive Stackable plugin integration to the Interactions system. It introduces six new action types (accordion toggle, carousel slide change, count-up reset, horizontal scroller scroll, progress bar/circle value change, tabs change) and five interaction types with corresponding frontend implementations. The system is enhanced to support multiple target selectors per block and conditionally load Stackable components when the plugin is present. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant FrontendUI as Frontend UI
participant InteractRunner as InteractRunner
participant Action as Action Handler
participant StackableBlock as Stackable Block
User->>FrontendUI: Trigger interaction event<br/>(e.g., click accordion)
FrontendUI->>InteractRunner: Event fires<br/>(stackable-accordion-toggle)
InteractRunner->>InteractRunner: Read stateAction option<br/>(toggle/open/close)
InteractRunner->>StackableBlock: Check element class<br/>(wp-block-stackable-accordion)
StackableBlock-->>InteractRunner: Element verified
InteractRunner->>InteractRunner: Create timeline instance<br/>(if state matches)
InteractRunner->>Action: Execute queued actions
Action->>Action: Get all target selectors<br/>(blockElementSelectors)
Action->>StackableBlock: Apply action to each target<br/>(setState, updateValue, etc.)
StackableBlock-->>User: Block updates visually
InteractRunner->>InteractRunner: Play timeline
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
🤖 Pull request artifacts
|
![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: 16
🧹 Nitpick comments (2)
src/integrations/stackable/interaction-types/frontend/stackableAccordionToggle.js (2)
9-23: Variableelreferenced before definition in handler closure.The
handlerfunction (line 11) referencesel, butelis not defined until line 23. While JavaScript hoisting allows this to work at runtime becauseelis assigned before the handler is ever called, the code structure is confusing and error-prone. Consider reordering for clarity.Proposed refactor for clarity
stackableAccordionToggle: { initTimeline: interaction => { const stateAction = interaction.getOption( 'stateAction', 'toggle' ) + const el = interaction.getCurrentTrigger() + + // Do not proceed if the target is not a Stackable Accordion + if ( ! el?.classList?.contains( 'wp-block-stackable-accordion' ) ) { + return + } let timeline = null const handler = event => { const isOpen = event.newState ? event.newState === 'open' : el.open if ( stateAction === 'toggle' || ( stateAction === 'open' && isOpen ) || ( stateAction === 'close' && ! isOpen ) ) { timeline?.destroy( false ) timeline = interaction.createTimelineInstance( 0 ) timeline?.play() } } - const el = interaction.getCurrentTrigger() - - // Do not proceed if the target is not a Stackable Accordion - if ( el.classList?.contains( 'wp-block-stackable-accordion' ) ) { - el.addEventListener( 'toggle', handler ) + el.addEventListener( 'toggle', handler ) - return () => { - timeline?.destroy() - el.removeEventListener( 'toggle', handler ) - } + return () => { + timeline?.destroy() + el.removeEventListener( 'toggle', handler ) } }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/integrations/stackable/interaction-types/frontend/stackableAccordionToggle.js` around lines 9 - 23, The handler closure references el before it's declared which is confusing; move the declaration of const el = interaction.getCurrentTrigger() above the handler (or alternatively have the handler call interaction.getCurrentTrigger() when invoked) so that handler, timeline, and el are clearly defined in that order; update references to el inside handler accordingly and keep timeline management (timeline?.destroy and timeline = interaction.createTimelineInstance) unchanged.
26-33: Missing cleanup return when accordion check fails.When
eldoesn't have thewp-block-stackable-accordionclass, the function returnsundefinedimplicitly. While this may be acceptable, explicitly returning a no-op cleanup function would be more consistent with the pattern and prevent potential issues if the caller expects a function.Proposed fix
// Do not proceed if the target is not a Stackable Accordion - if ( el.classList?.contains( 'wp-block-stackable-accordion' ) ) { + if ( ! el?.classList?.contains( 'wp-block-stackable-accordion' ) ) { + return () => {} + } + - el.addEventListener( 'toggle', handler ) + el.addEventListener( 'toggle', handler ) - return () => { - timeline?.destroy() - el.removeEventListener( 'toggle', handler ) - } + return () => { + timeline?.destroy() + el.removeEventListener( 'toggle', handler ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/integrations/stackable/interaction-types/frontend/stackableAccordionToggle.js` around lines 26 - 33, The cleanup return is only provided inside the if branch checking el.classList.contains('wp-block-stackable-accordion'), so when that check fails the function returns undefined; change the function to always return a cleanup function by adding an explicit no-op return (e.g., return () => {}) when the accordion class check fails, ensuring handlers like timeline?.destroy() and el.removeEventListener('toggle', handler) are only executed in the true branch while callers always receive a function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/generate-frontend-php-scripts.mjs`:
- Around line 142-186: The Stackable integration scan currently runs inside
processSourceDir (triggered by sourceDirs.forEach(processSourceDir)), causing
duplicate processing and races; move the two fs.readdirSync blocks that iterate
the Stackable frontend interaction-types and action-types out of
processSourceDir so they execute once during the top-level build flow, and
before each readdirSync add an existsSync guard (use fs.existsSync) to mirror
the earlier pattern; ensure you keep the same variables (type, scriptPath,
scriptPathRelative, outputFile, content) and continue calling writeFile/minify
as before so behavior is unchanged except for single-run scanning.
In
`@src/integrations/stackable/action-types/class-action-type-stackable-progress-bar-change-value.php`:
- Around line 20-27: The frontend handler incorrectly coerces value 0 to 100 by
using the falsy fallback operator; in
src/integrations/stackable/action-types/frontend/stackableProgressBarChangeValue.js
locate where the action payload's value is assigned using "|| 100" (the variable
named value) and replace that fallback with nullish coalescing so only undefined
or null fall back to 100 (i.e., use "?? 100" for the default). Ensure any
downstream uses still treat 0 as a legitimate value.
In `@src/integrations/stackable/action-types/frontend/stackableCountUpReset.js`:
- Around line 10-15: The code reads text.countUp without guarding that text
exists: change the logic in the handler that uses
el.querySelector('.stk-block-count-up__text') so you first check the result
(variable text) for null/undefined before accessing text.countUp; specifically,
in the block around el.querySelector('.stk-block-count-up__text') and the
countUp variable, make the guard check the existence of text (and then
text.countUp) and return early if text is falsy to avoid a runtime TypeError
when the markup is missing or changed.
In
`@src/integrations/stackable/action-types/frontend/stackableHorizontalScrollerScroll.js`:
- Around line 19-21: Fix the typo and add a defensive null check: replace the
incorrect property access child.clienWidth with child.clientWidth and ensure you
handle a missing child (from blockContent?.children?.[0]) before accessing
properties or calling window.getComputedStyle; update the logic that computes
columnWidth and scrollSnapAlign in the same block (references: child,
columnWidth, scrollSnapAlign, blockContent) so columnWidth falls back to 300 if
child is undefined or clientWidth is falsy, and only call
window.getComputedStyle(child) when child exists.
In
`@src/integrations/stackable/action-types/frontend/stackableProgressBarChangeValue.js`:
- Line 12: The inline comment "// Do not proceed if the target is not a
horizontal scroller" in the stackableProgressBarChangeValue action is a
copy-paste error; update that comment to accurately describe this action's
intent (e.g., reference progress bar elements or progress value updates) so it
reflects that the guard prevents continuing when the event target is not a
progress bar control handled by stackableProgressBarChangeValue; locate the
comment inside
src/integrations/stackable/action-types/frontend/stackableProgressBarChangeValue.js
and replace the incorrect text with a concise, correct description referring to
progress bar handling.
- Around line 17-21: The code assumes elements exist and directly accesses
bar.style and text.textContent; add null checks after querying
'.stk-progress-bar__bar' and '.stk-progress-bar__progress-value-text' in the
Stackable progress update logic (the variables named bar and text) and bail out
or skip updating if either is missing to avoid runtime errors — e.g., after
const bar = el.querySelector(... ) and const text = el.querySelector(... ),
check if (!bar || !text) return (or continue) before using bar.style.width or
text.textContent.
In
`@src/integrations/stackable/action-types/frontend/stackableProgressCircleChangeValue.js`:
- Line 12: The inline comment in stackableProgressCircleChangeValue.js is a
copy-paste error referencing a "horizontal scroller"; update the comment in the
function/handler (stackableProgressCircleChangeValue) to correctly describe that
it skips processing when the event target is not a progress circle (e.g., "Do
not proceed if the target is not a progress circle" or similar), ensuring any
surrounding comments accurately reflect progress circle handling.
- Around line 17-21: Add guards to avoid runtime errors when elements are
missing: after querying const circle = el.querySelector('.stk-progress-circle')
and const text = el.querySelector('.stk-progress-circle__inner-text') check that
each is non-null before accessing circle.style or text.textContent (e.g., if
(!circle) return; or if (!text) return; or use conditional early
returns/logging). Update the function that performs this DOM update
(stackableProgressCircleChangeValue or the surrounding handler) to perform these
null checks and bail out gracefully when elements are absent.
In
`@src/integrations/stackable/interaction-types/class-interaction-type-stackable-carousel-slide-change.php`:
- Around line 2-4: The file header docblock is stale saying "Stackable Accordion
Toggle" but the implementation is for carousel slide change; update the top
docblock title and brief description to "Interaction Type: Stackable Carousel
Slide Change" (or match the class name in this file, e.g., the
Stackable_Carousel_Slide_Change class) so the header accurately reflects the
class purpose and intent.
In
`@src/integrations/stackable/interaction-types/class-interaction-type-stackable-horizontal-scroller-scroll.php`:
- Around line 2-4: Update the stale docblock header that reads "Interaction
Type: Stackable Accordion Toggle" to accurately describe this file's
functionality; replace it with something like "Interaction Type: Stackable
Horizontal Scroller Scroll" so the top-of-file comment matches the horizontal
scroller interaction implemented in this file.
In
`@src/integrations/stackable/interaction-types/class-interaction-type-stackable-tabs-change.php`:
- Around line 2-4: The top PHPDoc header is stale: replace the incorrect summary
text "Stackable Accordion Toggle" with a correct description such as "Stackable
Tabs Change" (or "Interaction Type: Stackable Tabs Change") in the file's header
comment so the comment matches the implementation (the header comment near the
top of class-interaction-type-stackable-tabs-change.php); ensure the short
description line reflects Tabs Change and remove any leftover references to
Accordion Toggle.
In
`@src/integrations/stackable/interaction-types/frontend/stackableCarouselSlideChange.js`:
- Around line 22-26: The null/undefined trigger from
interaction.getCurrentTrigger() isn't guarded before accessing classList in the
block that registers the handler; change the conditional to check the element
itself and its classList (e.g., if
(el?.classList?.contains('wp-block-stackable-carousel')) ) so you don't call
.classList on a nullish el, and ensure the addEventListener call on handler
remains inside that guarded branch.
In
`@src/integrations/stackable/interaction-types/frontend/stackableHorizontalScrollerScroll.js`:
- Line 46: Update the inline comment in stackableHorizontalScrollerScroll.js
that currently reads "Stackable Tabs" to the correct component name "Stackable
Horizontal Scroller" so the comment matches the interaction implemented in
stackableHorizontalScrollerScroll (and any related functions like handleScroll
or isStackableHorizontalScroller checks).
- Around line 15-20: The code accesses DOM elements without guarding for missing
scroller or children causing runtime errors; update the logic around target,
child and children length in the function that computes
columnWidth/scrollSnapAlign/maxScrollLeft so it first checks that target and
target.children exist and that length > 0, bail out (or return a safe default)
if not, and only bind event listeners when a valid child exists; also replace
direct uses of child.clientWidth and window.getComputedStyle(child) with guarded
reads that fall back to defaults (e.g., 300 and 'none') and ensure maxScrollLeft
is computed only when target.clientWidth/scrollWidth are present.
- Around line 53-56: The cleanup currently destroys timeline and removes the
scroll listener but does not clear a pending debounce timer; declare the timeout
id in the same scope where the scroll handler is created (e.g., let
debounceTimeout / scrollTimeout), assign it when you call setTimeout inside the
handler, and in the returned cleanup call clearTimeout(debounceTimeout) (or
clearTimeout(scrollTimeout)) before calling timeline?.destroy() and
blockContent.removeEventListener('scroll', handler) so no queued timeout can run
after teardown.
In
`@src/integrations/stackable/interaction-types/frontend/stackableTabsChange.js`:
- Around line 22-26: getCurrentTrigger() can return undefined so accessing
el.classList may throw; update the guard in the event registration to null-safe
check the element before using classList (use
el?.classList?.contains('wp-block-stackable-tabs')) and only call
el.addEventListener('stackable-tabs-change', handler) when that check passes;
locate this logic around the getCurrentTrigger() call and the handler
registration to apply the change.
---
Nitpick comments:
In
`@src/integrations/stackable/interaction-types/frontend/stackableAccordionToggle.js`:
- Around line 9-23: The handler closure references el before it's declared which
is confusing; move the declaration of const el = interaction.getCurrentTrigger()
above the handler (or alternatively have the handler call
interaction.getCurrentTrigger() when invoked) so that handler, timeline, and el
are clearly defined in that order; update references to el inside handler
accordingly and keep timeline management (timeline?.destroy and timeline =
interaction.createTimelineInstance) unchanged.
- Around line 26-33: The cleanup return is only provided inside the if branch
checking el.classList.contains('wp-block-stackable-accordion'), so when that
check fails the function returns undefined; change the function to always return
a cleanup function by adding an explicit no-op return (e.g., return () => {})
when the accordion class check fails, ensuring handlers like timeline?.destroy()
and el.removeEventListener('toggle', handler) are only executed in the true
branch while callers always receive a function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a395012f-15d0-4d88-8696-a4fd6dab925b
📒 Files selected for processing (28)
interactions.phpscripts/generate-frontend-php-scripts.mjssrc/action-types/frontend/backgroundColor.jssrc/action-types/frontend/textColor.jssrc/editor/editor.phpsrc/frontend/scripts/class-interaction.jssrc/integrations/stackable/action-types/class-action-type-stackable-accordion-toggle.phpsrc/integrations/stackable/action-types/class-action-type-stackable-carousel-change-slide.phpsrc/integrations/stackable/action-types/class-action-type-stackable-count-up-reset.phpsrc/integrations/stackable/action-types/class-action-type-stackable-horizontal-scroller-scroll.phpsrc/integrations/stackable/action-types/class-action-type-stackable-progress-bar-change-value.phpsrc/integrations/stackable/action-types/class-action-type-stackable-progress-circle-change-value.phpsrc/integrations/stackable/action-types/class-action-type-stackable-tabs-change-tab.phpsrc/integrations/stackable/action-types/frontend/stackableAccordionToggle.jssrc/integrations/stackable/action-types/frontend/stackableCarouselChangeSlide.jssrc/integrations/stackable/action-types/frontend/stackableCountUpReset.jssrc/integrations/stackable/action-types/frontend/stackableHorizontalScrollerScroll.jssrc/integrations/stackable/action-types/frontend/stackableProgressBarChangeValue.jssrc/integrations/stackable/action-types/frontend/stackableProgressCircleChangeValue.jssrc/integrations/stackable/action-types/frontend/stackableTabsChangeTab.jssrc/integrations/stackable/interaction-types/class-interaction-type-stackable-accordion-toggle.phpsrc/integrations/stackable/interaction-types/class-interaction-type-stackable-carousel-slide-change.phpsrc/integrations/stackable/interaction-types/class-interaction-type-stackable-horizontal-scroller-scroll.phpsrc/integrations/stackable/interaction-types/class-interaction-type-stackable-tabs-change.phpsrc/integrations/stackable/interaction-types/frontend/stackableAccordionToggle.jssrc/integrations/stackable/interaction-types/frontend/stackableCarouselSlideChange.jssrc/integrations/stackable/interaction-types/frontend/stackableHorizontalScrollerScroll.jssrc/integrations/stackable/interaction-types/frontend/stackableTabsChange.js
Comment on lines
+142
to
+186
| // Stackable intergration interaction types | ||
| fs.readdirSync( path.resolve( __dirname, '../src/integrations/stackable/interaction-types/frontend' ) ) | ||
| .filter( file => file.endsWith( '.js' ) ) | ||
| .forEach( async file => { | ||
| const type = file.replace( '.js', '' ) | ||
| const scriptPath = path.resolve( __dirname, `../src/integrations/stackable//interaction-types/frontend/${ file }` ) | ||
| const scriptPathRelative = path.relative( path.resolve( __dirname, '../' ), scriptPath ) | ||
| const outputFile = path.resolve( __dirname, `../dist/frontend/interactions/${ type }.php` ) | ||
| const content = fs.readFileSync( scriptPath, 'utf8' ) | ||
|
|
||
| if ( buildType === 'development' ) { | ||
| writeFile( content, outputFile, scriptPathRelative ) | ||
| } else { | ||
| await minify( { | ||
| compressor: uglifyJS, | ||
| content, | ||
| output: outputFile, | ||
| } ).then( min => { | ||
| writeFile( min, outputFile, scriptPathRelative ) | ||
| } ) | ||
| } | ||
| } ) | ||
|
|
||
| // Stackable intergration action types | ||
| fs.readdirSync( path.resolve( __dirname, '../src/integrations/stackable/action-types/frontend' ) ) | ||
| .filter( file => file.endsWith( '.js' ) ) | ||
| .forEach( async file => { | ||
| const type = file.replace( '.js', '' ) | ||
| const scriptPath = path.resolve( __dirname, `../src/integrations/stackable/action-types/frontend/${ file }` ) | ||
| const scriptPathRelative = path.relative( path.resolve( __dirname, '../' ), scriptPath ) | ||
| const outputFile = path.resolve( __dirname, `../dist/frontend/actions/${ type }.php` ) | ||
| const content = fs.readFileSync( scriptPath, 'utf8' ) | ||
|
|
||
| if ( buildType === 'development' ) { | ||
| writeFile( content, outputFile, scriptPathRelative ) | ||
| } else { | ||
| await minify( { | ||
| compressor: uglifyJS, | ||
| content, | ||
| output: outputFile, | ||
| } ).then( min => { | ||
| writeFile( min, outputFile, scriptPathRelative ) | ||
| } ) | ||
| } | ||
| } ) |
Contributor
There was a problem hiding this comment.
Move the Stackable scan out of processSourceDir().
sourceDirs.forEach( processSourceDir ) invokes this block once per source root, so premium builds process the same Stackable files twice and race to overwrite the same dist/frontend/{interactions,actions} outputs. Please run this scan once, and mirror the earlier existsSync guard before each readdirSync.
💡 Suggested refactor
const processSourceDir = sourceDir => {
console.log( `📁 Processing frontend scripts from ${ sourceDir }` )
@@
- // Stackable intergration interaction types
- fs.readdirSync( path.resolve( __dirname, '../src/integrations/stackable/interaction-types/frontend' ) )
- .filter( file => file.endsWith( '.js' ) )
- .forEach( async file => {
- const type = file.replace( '.js', '' )
- const scriptPath = path.resolve( __dirname, `../src/integrations/stackable//interaction-types/frontend/${ file }` )
- const scriptPathRelative = path.relative( path.resolve( __dirname, '../' ), scriptPath )
- const outputFile = path.resolve( __dirname, `../dist/frontend/interactions/${ type }.php` )
- const content = fs.readFileSync( scriptPath, 'utf8' )
-
- if ( buildType === 'development' ) {
- writeFile( content, outputFile, scriptPathRelative )
- } else {
- await minify( {
- compressor: uglifyJS,
- content,
- output: outputFile,
- } ).then( min => {
- writeFile( min, outputFile, scriptPathRelative )
- } )
- }
- } )
-
- // Stackable intergration action types
- fs.readdirSync( path.resolve( __dirname, '../src/integrations/stackable/action-types/frontend' ) )
- .filter( file => file.endsWith( '.js' ) )
- .forEach( async file => {
- const type = file.replace( '.js', '' )
- const scriptPath = path.resolve( __dirname, `../src/integrations/stackable/action-types/frontend/${ file }` )
- const scriptPathRelative = path.relative( path.resolve( __dirname, '../' ), scriptPath )
- const outputFile = path.resolve( __dirname, `../dist/frontend/actions/${ type }.php` )
- const content = fs.readFileSync( scriptPath, 'utf8' )
-
- if ( buildType === 'development' ) {
- writeFile( content, outputFile, scriptPathRelative )
- } else {
- await minify( {
- compressor: uglifyJS,
- content,
- output: outputFile,
- } ).then( min => {
- writeFile( min, outputFile, scriptPathRelative )
- } )
- }
- } )
}
+
+const processStackableDirs = () => {
+ const stackableInteractionDir = path.resolve( __dirname, '../src/integrations/stackable/interaction-types/frontend' )
+ if ( fs.existsSync( stackableInteractionDir ) ) {
+ fs.readdirSync( stackableInteractionDir )
+ .filter( file => file.endsWith( '.js' ) )
+ .forEach( async file => {
+ const type = file.replace( '.js', '' )
+ const scriptPath = path.resolve( stackableInteractionDir, file )
+ const scriptPathRelative = path.relative( path.resolve( __dirname, '../' ), scriptPath )
+ const outputFile = path.resolve( __dirname, `../dist/frontend/interactions/${ type }.php` )
+ const content = fs.readFileSync( scriptPath, 'utf8' )
+ if ( buildType === 'development' ) {
+ writeFile( content, outputFile, scriptPathRelative )
+ } else {
+ await minify( { compressor: uglifyJS, content, output: outputFile } ).then( min => {
+ writeFile( min, outputFile, scriptPathRelative )
+ } )
+ }
+ } )
+ }
+
+ const stackableActionDir = path.resolve( __dirname, '../src/integrations/stackable/action-types/frontend' )
+ if ( fs.existsSync( stackableActionDir ) ) {
+ fs.readdirSync( stackableActionDir )
+ .filter( file => file.endsWith( '.js' ) )
+ .forEach( async file => {
+ const type = file.replace( '.js', '' )
+ const scriptPath = path.resolve( stackableActionDir, file )
+ const scriptPathRelative = path.relative( path.resolve( __dirname, '../' ), scriptPath )
+ const outputFile = path.resolve( __dirname, `../dist/frontend/actions/${ type }.php` )
+ const content = fs.readFileSync( scriptPath, 'utf8' )
+ if ( buildType === 'development' ) {
+ writeFile( content, outputFile, scriptPathRelative )
+ } else {
+ await minify( { compressor: uglifyJS, content, output: outputFile } ).then( min => {
+ writeFile( min, outputFile, scriptPathRelative )
+ } )
+ }
+ } )
+ }
+}
// Process all source directories
sourceDirs.forEach( processSourceDir )
+processStackableDirs()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/generate-frontend-php-scripts.mjs` around lines 142 - 186, The
Stackable integration scan currently runs inside processSourceDir (triggered by
sourceDirs.forEach(processSourceDir)), causing duplicate processing and races;
move the two fs.readdirSync blocks that iterate the Stackable frontend
interaction-types and action-types out of processSourceDir so they execute once
during the top-level build flow, and before each readdirSync add an existsSync
guard (use fs.existsSync) to mirror the earlier pattern; ensure you keep the
same variables (type, scriptPath, scriptPathRelative, outputFile, content) and
continue calling writeFile/minify as before so behavior is unchanged except for
single-run scanning.
Comment on lines
+20
to
+27
| 'value' => [ | ||
| 'name' => __( 'Value', 'interactions' ), | ||
| 'type' => 'number', | ||
| 'default' => 100, | ||
| 'min' => 0, | ||
| 'max' => 100, | ||
| 'step' => 0.1, | ||
| 'help' => __( 'The value to change into.', 'interactions' ), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd -i 'stackableProgressBarChangeValue.js' | head -n 1)"
echo "Inspecting $file"
nl -ba "$file" | sed -n '1,80p'
rg -n "getValue\\(\\s*'value'\\s*\\)\\s*\\|\\|\\s*100" "$file"Repository: gambitph/Interactions
Length of output: 221
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find and read the frontend file
file="src/integrations/stackable/action-types/frontend/stackableProgressBarChangeValue.js"
if [ -f "$file" ]; then
echo "=== File contents ==="
cat -n "$file"
echo ""
echo "=== Searching for getValue('value') pattern ==="
rg -n "getValue" "$file" -A 2 -B 2
else
echo "File not found: $file"
fiRepository: gambitph/Interactions
Length of output: 1222
Fix falsy coercion of value: 0 in frontend handler.
The backend schema allows value to be 0 (min is 0), but line 9 of src/integrations/stackable/action-types/frontend/stackableProgressBarChangeValue.js uses || 100 which treats 0 as falsy and incorrectly coerces it to 100. Use nullish coalescing instead:
Proposed fix
-const value = String( action.getValue( 'value' ) || 100 )
+const rawValue = action.getValue( 'value' )
+const value = String( rawValue ?? 100 )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/integrations/stackable/action-types/class-action-type-stackable-progress-bar-change-value.php`
around lines 20 - 27, The frontend handler incorrectly coerces value 0 to 100 by
using the falsy fallback operator; in
src/integrations/stackable/action-types/frontend/stackableProgressBarChangeValue.js
locate where the action payload's value is assigned using "|| 100" (the variable
named value) and replace that fallback with nullish coalescing so only undefined
or null fall back to 100 (i.e., use "?? 100" for the default). Ensure any
downstream uses still treat 0 as a legitimate value.
Comment on lines
+10
to
+15
| const text = el.querySelector( '.stk-block-count-up__text' ) | ||
| const countUp = text.countUp | ||
|
|
||
| // Do not proceed if the target is not a Stackable Count Up | ||
| if ( ! countUp ) { | ||
| return |
Contributor
There was a problem hiding this comment.
Guard the missing Count Up element before reading countUp.
el.querySelector( '.stk-block-count-up__text' ) can return null, so text.countUp throws before the fallback check runs. That breaks the action on any target whose markup is incomplete or has changed.
💡 Suggested fix
action.getTargets().forEach( el => {
const text = el.querySelector( '.stk-block-count-up__text' )
- const countUp = text.countUp
+ const countUp = text?.countUp
// Do not proceed if the target is not a Stackable Count Up
if ( ! countUp ) {
return
}📝 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
| const text = el.querySelector( '.stk-block-count-up__text' ) | |
| const countUp = text.countUp | |
| // Do not proceed if the target is not a Stackable Count Up | |
| if ( ! countUp ) { | |
| return | |
| const text = el.querySelector( '.stk-block-count-up__text' ) | |
| const countUp = text?.countUp | |
| // Do not proceed if the target is not a Stackable Count Up | |
| if ( ! countUp ) { | |
| return |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/integrations/stackable/action-types/frontend/stackableCountUpReset.js`
around lines 10 - 15, The code reads text.countUp without guarding that text
exists: change the logic in the handler that uses
el.querySelector('.stk-block-count-up__text') so you first check the result
(variable text) for null/undefined before accessing text.countUp; specifically,
in the block around el.querySelector('.stk-block-count-up__text') and the
countUp variable, make the guard check the existence of text (and then
text.countUp) and return early if text is falsy to avoid a runtime TypeError
when the markup is missing or changed.
Comment on lines
+19
to
+21
| const child = blockContent?.children?.[ 0 ] | ||
| const columnWidth = child.clienWidth ?? 300 | ||
| const scrollSnapAlign = window.getComputedStyle( child )?.scrollSnapAlign ?? 'none' |
Contributor
There was a problem hiding this comment.
Critical typo: clienWidth should be clientWidth.
This typo causes child.clienWidth to always be undefined, making the code always use the fallback value of 300. Additionally, child could be undefined if blockContent has no children.
Proposed fix
const blockContent = el.querySelector( '.stk-block-content' )
const child = blockContent?.children?.[ 0 ]
- const columnWidth = child.clienWidth ?? 300
+ if ( ! child ) {
+ return
+ }
+ const columnWidth = child.clientWidth || 300
const scrollSnapAlign = window.getComputedStyle( child )?.scrollSnapAlign ?? 'none'📝 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
| const child = blockContent?.children?.[ 0 ] | |
| const columnWidth = child.clienWidth ?? 300 | |
| const scrollSnapAlign = window.getComputedStyle( child )?.scrollSnapAlign ?? 'none' | |
| const child = blockContent?.children?.[ 0 ] | |
| if ( ! child ) { | |
| return | |
| } | |
| const columnWidth = child.clientWidth || 300 | |
| const scrollSnapAlign = window.getComputedStyle( child )?.scrollSnapAlign ?? 'none' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/integrations/stackable/action-types/frontend/stackableHorizontalScrollerScroll.js`
around lines 19 - 21, Fix the typo and add a defensive null check: replace the
incorrect property access child.clienWidth with child.clientWidth and ensure you
handle a missing child (from blockContent?.children?.[0]) before accessing
properties or calling window.getComputedStyle; update the logic that computes
columnWidth and scrollSnapAlign in the same block (references: child,
columnWidth, scrollSnapAlign, blockContent) so columnWidth falls back to 300 if
child is undefined or clientWidth is falsy, and only call
window.getComputedStyle(child) when child exists.
| const value = String( action.getValue( 'value' ) || 100 ) | ||
|
|
||
| action.getTargets().forEach( el => { | ||
| // Do not proceed if the target is not a horizontal scroller |
Contributor
There was a problem hiding this comment.
Incorrect comment: copy-paste error.
The comment mentions "horizontal scroller" but this action handles progress bars.
Proposed fix
- // Do not proceed if the target is not a horizontal scroller
+ // Do not proceed if the target is not a Stackable Progress Bar📝 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
| // Do not proceed if the target is not a horizontal scroller | |
| // Do not proceed if the target is not a Stackable Progress Bar |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/integrations/stackable/action-types/frontend/stackableProgressBarChangeValue.js`
at line 12, The inline comment "// Do not proceed if the target is not a
horizontal scroller" in the stackableProgressBarChangeValue action is a
copy-paste error; update that comment to accurately describe this action's
intent (e.g., reference progress bar elements or progress value updates) so it
reflects that the guard prevents continuing when the event target is not a
progress bar control handled by stackableProgressBarChangeValue; locate the
comment inside
src/integrations/stackable/action-types/frontend/stackableProgressBarChangeValue.js
and replace the incorrect text with a concise, correct description referring to
progress bar handling.
Comment on lines
+22
to
+26
| const el = interaction.getCurrentTrigger() | ||
|
|
||
| // Do not proceed if the target is not a Stackable Carousel | ||
| if ( el.classList?.contains( 'wp-block-stackable-carousel' ) ) { | ||
| el.addEventListener( 'stackable-carousel-slide-change', handler ) |
Contributor
There was a problem hiding this comment.
Guard el itself before reading classList.
If interaction.getCurrentTrigger() is nullish, el.classList?.contains(...) still throws. Use el?.classList?.contains(...) to avoid runtime failures.
Proposed fix
- if ( el.classList?.contains( 'wp-block-stackable-carousel' ) ) {
+ if ( el?.classList?.contains( 'wp-block-stackable-carousel' ) ) {#!/bin/bash
set -euo pipefail
# Verify getCurrentTrigger usage and null-guard patterns.
rg -n -C3 'getCurrentTrigger\s*\(' src/frontend/scripts/class-interaction.js
rg -n -C2 'const el = interaction.getCurrentTrigger\(\)' src/integrations/stackable/interaction-types/frontend/stackableCarouselSlideChange.js src/integrations/stackable/interaction-types/frontend/stackableTabsChange.js
rg -n -C2 'el\.classList\?\.' src/integrations/stackable/interaction-types/frontend/stackableCarouselSlideChange.js src/integrations/stackable/interaction-types/frontend/stackableTabsChange.js🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/integrations/stackable/interaction-types/frontend/stackableCarouselSlideChange.js`
around lines 22 - 26, The null/undefined trigger from
interaction.getCurrentTrigger() isn't guarded before accessing classList in the
block that registers the handler; change the conditional to check the element
itself and its classList (e.g., if
(el?.classList?.contains('wp-block-stackable-carousel')) ) so you don't call
.classList on a nullish el, and ensure the addEventListener call on handler
remains inside that guarded branch.
Comment on lines
+15
to
+20
| const child = target.children[ 0 ] | ||
| const length = target.children.length | ||
|
|
||
| const columnWidth = child.clientWidth ?? 300 | ||
| const scrollSnapAlign = window.getComputedStyle( child )?.scrollSnapAlign ?? 'none' | ||
| const maxScrollLeft = target.scrollWidth - target.clientWidth |
Contributor
There was a problem hiding this comment.
Add null/empty guards before DOM access and listener binding.
This can throw when .stk-block-content is absent or when the scroller has no children (child undefined).
Proposed guard fixes
const handler = event => {
const target = event.target
- const child = target.children[ 0 ]
+ const child = target.children?.[ 0 ]
+ if ( ! child ) {
+ return
+ }
const length = target.children.length
const columnWidth = child.clientWidth ?? 300 const blockContent = el.querySelector( '.stk-block-content' )
+if ( ! blockContent ) {
+ return
+}
blockContent.addEventListener( 'scroll', handler )Also applies to: 49-51
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/integrations/stackable/interaction-types/frontend/stackableHorizontalScrollerScroll.js`
around lines 15 - 20, The code accesses DOM elements without guarding for
missing scroller or children causing runtime errors; update the logic around
target, child and children length in the function that computes
columnWidth/scrollSnapAlign/maxScrollLeft so it first checks that target and
target.children exist and that length > 0, bail out (or return a safe default)
if not, and only bind event listeners when a valid child exists; also replace
direct uses of child.clientWidth and window.getComputedStyle(child) with guarded
reads that fall back to defaults (e.g., 300 and 'none') and ensure maxScrollLeft
is computed only when target.clientWidth/scrollWidth are present.
|
|
||
| const el = interaction.getCurrentTrigger() | ||
|
|
||
| // Do not proceed if the target is not a Stackable Tabs |
Contributor
There was a problem hiding this comment.
Correct the inline comment target name.
“Stackable Tabs” should be “Stackable Horizontal Scroller” to match this interaction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/integrations/stackable/interaction-types/frontend/stackableHorizontalScrollerScroll.js`
at line 46, Update the inline comment in stackableHorizontalScrollerScroll.js
that currently reads "Stackable Tabs" to the correct component name "Stackable
Horizontal Scroller" so the comment matches the interaction implemented in
stackableHorizontalScrollerScroll (and any related functions like handleScroll
or isStackableHorizontalScroller checks).
Comment on lines
+53
to
+56
| return () => { | ||
| timeline?.destroy() | ||
| blockContent.removeEventListener( 'scroll', handler ) | ||
| } |
Contributor
There was a problem hiding this comment.
Clear the pending debounce timer during cleanup.
A queued timeout can still run after teardown; clear it to avoid post-unmount timeline creation.
Proposed cleanup update
return () => {
+ clearTimeout( isScrolling )
timeline?.destroy()
blockContent.removeEventListener( 'scroll', handler )
}📝 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
| return () => { | |
| timeline?.destroy() | |
| blockContent.removeEventListener( 'scroll', handler ) | |
| } | |
| return () => { | |
| clearTimeout( isScrolling ) | |
| timeline?.destroy() | |
| blockContent.removeEventListener( 'scroll', handler ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/integrations/stackable/interaction-types/frontend/stackableHorizontalScrollerScroll.js`
around lines 53 - 56, The cleanup currently destroys timeline and removes the
scroll listener but does not clear a pending debounce timer; declare the timeout
id in the same scope where the scroll handler is created (e.g., let
debounceTimeout / scrollTimeout), assign it when you call setTimeout inside the
handler, and in the returned cleanup call clearTimeout(debounceTimeout) (or
clearTimeout(scrollTimeout)) before calling timeline?.destroy() and
blockContent.removeEventListener('scroll', handler) so no queued timeout can run
after teardown.
Comment on lines
+22
to
+26
| const el = interaction.getCurrentTrigger() | ||
|
|
||
| // Do not proceed if the target is not a Stackable Tabs | ||
| if ( el.classList?.contains( 'wp-block-stackable-tabs' ) ) { | ||
| el.addEventListener( 'stackable-tabs-change', handler ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/integrations/stackable/interaction-types/frontend/stackableTabsChange.jsRepository: gambitph/Interactions
Length of output: 1321
🏁 Script executed:
# Find the carousel file mentioned for comparison
find . -type f -name '*carousel*' -o -name '*Carousel*' | grep -i interactionRepository: gambitph/Interactions
Length of output: 377
🏁 Script executed:
# Search for getCurrentTrigger implementation and usage
rg -A 5 "getCurrentTrigger" --type jsRepository: gambitph/Interactions
Length of output: 10240
Add null-safe access on el before accessing classList.
The getCurrentTrigger() method can return undefined if no trigger element exists. The current code will throw "Cannot read property 'classList' of undefined" before the optional chaining takes effect. Use el?.classList?.contains(...) instead.
Proposed fix
- if ( el.classList?.contains( 'wp-block-stackable-tabs' ) ) {
+ if ( el?.classList?.contains( 'wp-block-stackable-tabs' ) ) {📝 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
| const el = interaction.getCurrentTrigger() | |
| // Do not proceed if the target is not a Stackable Tabs | |
| if ( el.classList?.contains( 'wp-block-stackable-tabs' ) ) { | |
| el.addEventListener( 'stackable-tabs-change', handler ) | |
| const el = interaction.getCurrentTrigger() | |
| // Do not proceed if the target is not a Stackable Tabs | |
| if ( el?.classList?.contains( 'wp-block-stackable-tabs' ) ) { | |
| el.addEventListener( 'stackable-tabs-change', handler ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/integrations/stackable/interaction-types/frontend/stackableTabsChange.js`
around lines 22 - 26, getCurrentTrigger() can return undefined so accessing
el.classList may throw; update the guard in the event registration to null-safe
check the element before using classList (use
el?.classList?.contains('wp-block-stackable-tabs')) and only call
el.addEventListener('stackable-tabs-change', handler) when that check passes;
locate this logic around the getCurrentTrigger() call and the handler
registration to apply the change.
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
1 participant
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.
fixes #21
Summary by CodeRabbit
Release Notes