Feat: More Guided Tours #3639
Feat: More Guided Tours #3639
Conversation
WalkthroughAdds three guided tours ("block-backgrounds", "responsive-controls", "hover-states"), URL-query-based tour condition keys, dynamic selection of tour via the editor Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EditorTour as Editor Tour
participant TourCond as TOUR_CONDITIONS
participant BlockBG as Block Backgrounds Tour
participant RespCtrl as Responsive Controls Tour
participant HoverSt as Hover States Tour
participant Editor as WP Editor
User->>EditorTour: Open editor (maybe ?tour=...)
EditorTour->>TourCond: Read URL param "tour"
alt tour = block-backgrounds
EditorTour->>BlockBG: Render GuidedModalTour(tourId)
BlockBG->>Editor: initialize() -> insertBlocks() / selectBlock()
else tour = responsive-controls
EditorTour->>RespCtrl: Render GuidedModalTour(tourId)
RespCtrl->>Editor: initialize() -> insertBlocks() / selectBlock()
else tour = hover-states
EditorTour->>HoverSt: Render GuidedModalTour(tourId)
HoverSt->>Editor: initialize() -> insertBlocks() / selectBlock()
else no param or other
EditorTour->>EditorTour: Render default "blocks" tour
end
loop Tour steps
Note over Editor,EditorTour: preStep opens panels / selects targets
Tour->>Editor: preStep/postStep (simulate clicks, update attrs)
Note right of Editor: async UI actions (setTimeouts, DOM clicks)
User->>Tour: Progress / interact
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
🤖 Pull request artifacts
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/welcome/getting-started.js (1)
200-203: Consider making the "Quick Start" label more explicit.The condition
i === 0assumes the first item should show the label. If the array order changes or items are filtered in the future, this could break.Consider adding a
showLabelproperty to QUICK_BUTTONS entries:const QUICK_BUTTONS = [ { id: 'design-library', + showLabel: true, icon: addTemplateIcon,Then use:
-{ i === 0 && <div className="s-quick-buttons-arrow"> +{ item.showLabel && <div className="s-quick-buttons-arrow">src/lazy-components/modal-tour/tours/block-backgrounds.js (1)
83-97: Improve resilience of DOM manipulation in tour steps.The direct
querySelectorcalls and DOM clicks are fragile and may fail if:
- Selectors change due to WordPress/plugin updates
- Elements are not yet rendered
- Class names are dynamically generated differently
Consider:
- Adding error handling and logging when elements aren't found
- Using data attributes instead of complex class selectors for tour targets
- Wrapping DOM operations in try-catch blocks to prevent tour crashes
Example:
preStep: () => { try { dispatch('core/edit-post').openGeneralSidebar('edit-post/block') const allBlocks = select('core/block-editor').getBlocks() const columnsBlock = allBlocks.find(block => block.name === 'stackable/columns') if (columnsBlock) { dispatch('core/block-editor').selectBlock(columnsBlock.clientId) } else { console.warn('Tour: columns block not found') } } catch (error) { console.error('Tour step failed:', error) } },Also applies to: 110-125, 138-146, 175-188, 193-197
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/guided-modal-tour/tour-conditions.js(1 hunks)src/lazy-components/modal-tour/tours/block-backgrounds.js(1 hunks)src/plugins/guided-modal-tour/editor-block-tour.js(1 hunks)src/welcome/getting-started.js(1 hunks)src/welcome/getting-started.scss(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/plugins/guided-modal-tour/editor-block-tour.js (1)
src/components/guided-modal-tour/index.js (1)
GuidedModalTour(27-104)
⏰ 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). (7)
- GitHub Check: PHP 7.3 and WP latest
- GitHub Check: PHP 8.2 and WP 6.7.2
- GitHub Check: PHP 7.3 and WP 6.5.5
- GitHub Check: PHP 8.2 and WP 6.5.5
- GitHub Check: PHP 8.2 and WP 6.6.2
- GitHub Check: PHP 8.2 and WP latest
- GitHub Check: build
🔇 Additional comments (7)
src/components/guided-modal-tour/tour-conditions.js (1)
30-33: LGTM! Consistent tour condition implementation.The new
block-backgroundstour condition follows the established pattern and correctly checks for the URL parameter to activate the tour.src/welcome/getting-started.scss (2)
197-200: Good refinement of the SVG sizing rule.Excluding
.s-quick-buttons-arrowfrom the generic SVG sizing prevents unintended styling conflicts with the decorative arrow element.
239-268: Layout refactor supports the new data-driven cards.The grid gap adjustment, video positioning, and flex-direction changes align well with the QUICK_BUTTONS rendering strategy in
getting-started.js.src/welcome/getting-started.js (1)
140-156: Verify the purpose of hidden quick-start items.Two entries (
site-kitanddesign-system-picker) havestyle: { display: 'none' }. Are these feature-flagged for future release, or should they be removed from the configuration?Consider one of these approaches:
- If they're coming soon, add a comment explaining they're feature-flagged
- If they're experimental, move them to a separate
EXPERIMENTAL_BUTTONSarray- If they're not needed, remove them to reduce maintenance overhead
src/plugins/guided-modal-tour/editor-block-tour.js (1)
5-10: LGTM! Clean integration of the new tour.The dual-tour rendering is handled correctly. The
GuidedModalTourcomponent's internal logic ensures only one tour shows at a time based on URL parameters and conditions.src/lazy-components/modal-tour/tours/block-backgrounds.js (2)
7-71: Clean block structure initialization.The nested block creation logic is well-structured and creates appropriate default content for the tour demonstration.
72-210: Well-crafted tour flow and copy.The step-by-step progression is logical, and the i18n-ready help text provides clear guidance for users learning about backgrounds and containers.
| const blockObject = wp.blocks.createBlock( | ||
| 'stackable/columns', | ||
| { | ||
| uniqueId: '1dbe04e', | ||
| blockMargin: { bottom: '' }, | ||
| align: 'full', | ||
| containerWidth: 50, | ||
| containerHorizontalAlign: 'flex-start', | ||
| containerWidthUnit: '%', | ||
| }, | ||
| [ | ||
| wp.blocks.createBlock( | ||
| 'stackable/column', | ||
| { | ||
| uniqueId: 'f957abc', | ||
| columnSpacing: { | ||
| top: '', right: '', bottom: '', left: '', | ||
| }, | ||
| }, | ||
| [ | ||
| wp.blocks.createBlock( | ||
| 'stackable/heading', | ||
| { | ||
| uniqueId: 'a8ebea7', | ||
| // Retain our text | ||
| text: 'Explore the World with Us', | ||
| textTag: 'h2', | ||
| } | ||
| ), | ||
| wp.blocks.createBlock( | ||
| 'stackable/text', | ||
| { | ||
| uniqueId: '57e76a1', | ||
| // Retain our text | ||
| text: 'Discover breathtaking destinations, plan your next adventure, and make unforgettable memories with our travel guides and tips.', | ||
| } | ||
| ), | ||
| wp.blocks.createBlock( | ||
| 'stackable/button-group', | ||
| { uniqueId: 'e063798' }, | ||
| [ | ||
| wp.blocks.createBlock( | ||
| 'stackable/button', | ||
| { | ||
| uniqueId: '5d04ca8', | ||
| // Retain our text | ||
| text: 'Start your journey', | ||
| url: '', | ||
| } | ||
| ), | ||
| ] | ||
| ), | ||
| ] | ||
| ), | ||
| ] | ||
| ) |
There was a problem hiding this comment.
Potential block ID conflicts with hardcoded uniqueIds.
The hardcoded uniqueId values (e.g., '1dbe04e', 'f957abc') could conflict if blocks with these IDs already exist on the page, potentially causing unexpected behavior or errors.
Consider generating unique IDs dynamically:
const blockObject = wp.blocks.createBlock(
'stackable/columns',
{
- uniqueId: '1dbe04e',
+ uniqueId: `tour-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`,Or use a utility function if one exists in the codebase for generating unique block IDs.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/lazy-components/modal-tour/tours/block-backgrounds.js around lines 10 to
65, the blocks are created with hardcoded uniqueId values which can collide with
existing blocks; update the code to stop using fixed IDs by either removing the
uniqueId property (let Gutenberg generate IDs) or replacing each hardcoded
string with a call to a shared ID generator (e.g., const id =
generateUniqueId(); then use uniqueId: id) — ensure you import or use the
project utility if one exists, or add a small helper that produces RFC4122-like
or sufficiently random IDs and call it for every block creation.
| setTimeout( () => { | ||
| // Click the tab | ||
| document.querySelector( '.edit-post-sidebar__panel-tab.ugb-tab--style:not(.is-active)' )?.click() | ||
| }, 100 ) |
There was a problem hiding this comment.
🧩 Analysis chain
Race conditions from hardcoded setTimeout delays.
The 100ms delays are arbitrary and may fail on slower systems, during high CPU load, or in different browser environments, causing the tour to break or behave unpredictably.
Consider using requestAnimationFrame or polling with a timeout for more reliable synchronization:
const waitForElement = (selector, timeout = 2000) => {
return new Promise((resolve, reject) => {
const startTime = Date.now()
const check = () => {
const element = document.querySelector(selector)
if (element) {
resolve(element)
} else if (Date.now() - startTime > timeout) {
reject(new Error(`Element ${selector} not found`))
} else {
requestAnimationFrame(check)
}
}
check()
})
}
// Usage in preStep:
await waitForElement('.edit-post-sidebar__panel-tab.ugb-tab--style')
document.querySelector('.edit-post-sidebar__panel-tab.ugb-tab--style:not(.is-active)')?.click()Also applies to: 184-186
I'll verify the review comment by examining the codebase and understanding the context around those hardcoded delays.
🏁 Script executed:
#!/bin/bash
# Check if file exists and view the code around the mentioned lines
if [ -f "src/lazy-components/modal-tour/tours/block-backgrounds.js" ]; then
echo "=== File found. Showing lines 115-130 ==="
sed -n '115,130p' "src/lazy-components/modal-tour/tours/block-backgrounds.js"
echo ""
echo "=== Showing lines 180-190 ==="
sed -n '180,190p' "src/lazy-components/modal-tour/tours/block-backgrounds.js"
else
echo "File not found at expected path"
# Try to find it
find . -name "block-backgrounds.js" -type f 2>/dev/null
fiLength of output: 1368
🏁 Script executed:
#!/bin/bash
# Search for all setTimeout calls in the file to understand usage patterns
echo "=== All setTimeout calls in block-backgrounds.js ==="
rg -n "setTimeout" "src/lazy-components/modal-tour/tours/block-backgrounds.js" -B2 -A4Length of output: 641
🏁 Script executed:
#!/bin/bash
# Check if there are utility functions for DOM element waiting in the codebase
echo "=== Searching for existing waitFor, waitForElement patterns ==="
rg -n "waitFor|requestAnimationFrame" --type=js -g "!node_modules" | head -30Length of output: 299062
🏁 Script executed:
#!/bin/bash
# Understand the structure and purpose of this file
echo "=== File context: imports, function definitions ==="
head -50 "src/lazy-components/modal-tour/tours/block-backgrounds.js"Length of output: 1372
Address race condition risk from hardcoded setTimeout delays in tour component.
Lines 121-124 and 184-186 use arbitrary 100ms delays before querying and clicking DOM elements. On slower systems or under CPU load, the target elements may not exist in the DOM yet, causing the selector to return null and the tour to fail silently. Replace with polling or requestAnimationFrame to ensure elements are present before interaction.
const waitForElement = (selector, timeout = 2000) => {
return new Promise((resolve, reject) => {
const startTime = Date.now()
const check = () => {
const element = document.querySelector(selector)
if (element) {
resolve(element)
} else if (Date.now() - startTime > timeout) {
reject(new Error(`Element ${selector} not found`))
} else {
requestAnimationFrame(check)
}
}
check()
})
}
// Usage in preStep:
await waitForElement('.edit-post-sidebar__panel-tab.ugb-tab--style:not(.is-active)')
document.querySelector('.edit-post-sidebar__panel-tab.ugb-tab--style:not(.is-active)')?.click()Also applies to: 184-186
🤖 Prompt for AI Agents
In src/lazy-components/modal-tour/tours/block-backgrounds.js around lines
121-124 (and similarly at 184-186), the hardcoded setTimeout(100) introduces a
race on DOM availability; replace these timeouts with a small utility that waits
for the element (polling via requestAnimationFrame or a MutationObserver) with a
reasonable timeout, await that promise before calling .click(), and handle
rejection/logging if the element never appears so the tour fails predictably
instead of silently; add the waitForElement helper once and reuse it for both
places.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/lazy-components/modal-tour/tours/responsive-controls.js (2)
65-67: Remove commented-out code.Dead code should be removed to improve maintainability.
- // Delete all blocks - // const allBlocks = select( 'core/block-editor' ).getBlocks() - // dispatch( 'core/block-editor' ).removeBlocks( allBlocks.map( block => block.clientId ) ) -
117-122: Code duplication in preStep and postStep.The
postStepat line 121 duplicates similar logic from thepreStepat line 118, and this pattern repeats in subsequent steps (e.g., line 134). This duplication makes the code harder to maintain.Consider extracting common DOM manipulation patterns into helper functions:
// At the top of the file const helpers = { openResponsiveToggle: () => { document.querySelector( '.ugb-sort-control .stk-control-responsive-toggle:not(.is-open) button' )?.click() }, selectDevice: ( device ) => { document.querySelector( `.ugb-sort-control .stk-control-responsive-toggle button:not(.is-active)[data-value="${device}"]` )?.click() }, } // Then in steps: preStep: () => { helpers.openResponsiveToggle() }, postStep: () => { helpers.selectDevice( 'mobile' ) },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/guided-modal-tour/tour-conditions.js(1 hunks)src/lazy-components/modal-tour/tours/responsive-controls.js(1 hunks)src/plugins/guided-modal-tour/editor-block-tour.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/guided-modal-tour/tour-conditions.js
- src/plugins/guided-modal-tour/editor-block-tour.js
⏰ 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). (7)
- GitHub Check: PHP 8.2 and WP 6.6.2
- GitHub Check: build
- GitHub Check: PHP 7.3 and WP 6.5.5
- GitHub Check: PHP 8.2 and WP latest
- GitHub Check: PHP 8.2 and WP 6.7.2
- GitHub Check: PHP 7.3 and WP latest
- GitHub Check: PHP 8.2 and WP 6.5.5
🔇 Additional comments (2)
src/lazy-components/modal-tour/tours/responsive-controls.js (2)
75-199: Well-structured tour flow with good UX guidance.The tour steps provide clear, progressive guidance through the responsive controls feature with helpful descriptions using interpolated elements for emphasis. The flow logically guides users through enabling, using, and understanding responsive controls.
4-4: No issues found with @wordpress/element dependency.The codebase widely uses
@wordpress/elementacross 100+ files with no issues. The import ofcreateInterpolateElementfrom@wordpress/elementin the responsive-controls.js file is consistent with this established pattern throughout the codebase, and the package is properly managed in the project's dependency lock file. The original concern is not applicable here.
| const blockObject = wp.blocks.createBlock( | ||
| 'stackable/columns', | ||
| { | ||
| uniqueId: '1dbe04e', | ||
| blockMargin: { bottom: '' }, | ||
| align: 'full', | ||
| }, | ||
| [ | ||
| wp.blocks.createBlock( | ||
| 'stackable/column', | ||
| { | ||
| uniqueId: 'f957abc', | ||
| hasContainer: true, | ||
| columnSpacing: { | ||
| top: '', right: '', bottom: '', left: '', | ||
| }, | ||
| }, | ||
| [ | ||
| wp.blocks.createBlock( | ||
| 'stackable/heading', | ||
| { | ||
| uniqueId: 'a8ebea7', | ||
| // Retain our text | ||
| text: 'Explore the World with Us', | ||
| textTag: 'h2', | ||
| } | ||
| ), | ||
| wp.blocks.createBlock( | ||
| 'stackable/text', | ||
| { | ||
| uniqueId: '57e76a1', | ||
| // Retain our text | ||
| text: 'Discover breathtaking destinations, plan your next adventure, and make unforgettable memories with our travel guides and tips.', | ||
| } | ||
| ), | ||
| ] | ||
| ), | ||
| wp.blocks.createBlock( | ||
| 'stackable/column', | ||
| { | ||
| uniqueId: '3dcffca', | ||
| }, | ||
| [ | ||
| wp.blocks.createBlock( | ||
| 'stackable/image', | ||
| { | ||
| uniqueId: 'e063798', | ||
| imageExternalUrl: 'https://picsum.photos/id/177/500/700.jpg', | ||
| } | ||
| ), | ||
| ] | ||
| ), | ||
| ] | ||
| ) |
There was a problem hiding this comment.
Generate unique IDs dynamically to prevent conflicts.
The block creation uses hardcoded uniqueId values ('1dbe04e', 'f957abc', 'a8ebea7', '57e76a1', '3dcffca', 'e063798'). If blocks with these IDs already exist in the editor, this could cause conflicts or unexpected behavior.
Consider using a dynamic ID generation utility if available in Stackable, or generate random IDs at runtime:
+// Helper to generate unique IDs
+const generateUniqueId = () => Math.random().toString(36).substring(2, 9)
+
const blockObject = wp.blocks.createBlock(
'stackable/columns',
{
- uniqueId: '1dbe04e',
+ uniqueId: generateUniqueId(),
blockMargin: { bottom: '' },
align: 'full',
},Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/lazy-components/modal-tour/tours/responsive-controls.js around lines 10
to 63, the blocks are created with hardcoded uniqueId values which can conflict
with existing blocks; replace each hardcoded uniqueId with a dynamically
generated ID at runtime (use the Stackable ID utility if available, e.g.
stackable.utils.generateUniqueId or similar), or fall back to a safe random/UUID
generator to produce unique strings for each block before calling
wp.blocks.createBlock; ensure you generate a fresh ID for every block instance
(parent and all children) and pass those generated IDs into the uniqueId
properties.
| 'stackable/image', | ||
| { | ||
| uniqueId: 'e063798', | ||
| imageExternalUrl: 'https://picsum.photos/id/177/500/700.jpg', |
There was a problem hiding this comment.
External image dependency may be unreliable.
The tour relies on an external image URL (https://picsum.photos/id/177/500/700.jpg) which could fail if the service is unavailable or slow, potentially disrupting the tour experience.
Consider using a local placeholder image or adding fallback handling for image load failures.
🤖 Prompt for AI Agents
In src/lazy-components/modal-tour/tours/responsive-controls.js around line 57,
the hard-coded external image URL is unreliable; replace it with a local
placeholder asset (import or require a bundled image) or implement fallback
handling by adding an onError handler that swaps the src to a local placeholder
and optionally sets a timeout/placeholder while loading; ensure the local image
is added to the project assets and referenced via the module import so builds
include it and update the tour data to use the local reference or wrapped image
component with the onError fallback.
| preStep: () => { | ||
| dispatch( 'core/edit-post' ).openGeneralSidebar( 'edit-post/block' ) | ||
| // Make sure the Columns block is selected | ||
| const allBlocks = select( 'core/block-editor' ).getBlocks() | ||
| const columnsBlock = allBlocks.find( block => block.name === 'stackable/columns' ) | ||
| if ( columnsBlock ) { | ||
| dispatch( 'core/block-editor' ).selectBlock( columnsBlock.clientId ) | ||
| } | ||
| setTimeout( () => { | ||
| // Open Layout tab if it's not open | ||
| document.querySelector( '.edit-post-sidebar__panel-tab.ugb-tab--layout:not(.is-active)' )?.click() | ||
| }, 100 ) | ||
| setTimeout( () => { | ||
| document.querySelector( '.ugb-panel--layout:not(.is-opened)' )?.click() | ||
| }, 200 ) | ||
| }, |
There was a problem hiding this comment.
Race conditions with hardcoded timeouts.
The preStep uses setTimeout with hardcoded delays (100ms, 200ms) to orchestrate DOM interactions. These delays are brittle and environment-dependent—they may be too short on slower systems or unnecessarily long on faster ones, potentially causing the tour to fail or feel sluggish.
Consider using more reliable synchronization methods:
preStep: async () => {
dispatch( 'core/edit-post' ).openGeneralSidebar( 'edit-post/block' )
const allBlocks = select( 'core/block-editor' ).getBlocks()
const columnsBlock = allBlocks.find( block => block.name === 'stackable/columns' )
if ( columnsBlock ) {
dispatch( 'core/block-editor' ).selectBlock( columnsBlock.clientId )
}
// Wait for DOM to update using requestAnimationFrame or MutationObserver
await new Promise( resolve => requestAnimationFrame( resolve ) )
document.querySelector( '.edit-post-sidebar__panel-tab.ugb-tab--layout:not(.is-active)' )?.click()
await new Promise( resolve => requestAnimationFrame( resolve ) )
document.querySelector( '.ugb-panel--layout:not(.is-opened)' )?.click()
},Alternatively, poll for element readiness or use MutationObserver to detect when elements are available.
| postStep: () => { | ||
| // Update the order of the columns for mobile by dispatching an attribute update. | ||
| const allBlocks = wp.data.select( 'core/block-editor' ).getBlocks() | ||
| const columnsBlock = allBlocks.find( block => block.name === 'stackable/columns' ) | ||
| if ( columnsBlock ) { | ||
| // Reverse the order of columns for mobile by updating the mobileOrder attribute. | ||
| // The default order is [0, 1]. Swapping makes it [1, 0]. | ||
| wp.data.dispatch( 'core/block-editor' ).updateBlockAttributes( columnsBlock.clientId, { | ||
| columnArrangementMobile: '2,1', | ||
| } ) | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Inconsistent import usage and missing error handling.
Lines 148-149 use wp.data.select and line 153 uses wp.data.dispatch, but select and dispatch were already imported from @wordpress/data at line 3. This inconsistency makes the code harder to maintain.
Additionally, there's no validation that the attribute update succeeded or feedback if the columnsBlock is not found.
postStep: () => {
- const allBlocks = wp.data.select( 'core/block-editor' ).getBlocks()
+ const allBlocks = select( 'core/block-editor' ).getBlocks()
const columnsBlock = allBlocks.find( block => block.name === 'stackable/columns' )
if ( columnsBlock ) {
- wp.data.dispatch( 'core/block-editor' ).updateBlockAttributes( columnsBlock.clientId, {
+ dispatch( 'core/block-editor' ).updateBlockAttributes( columnsBlock.clientId, {
columnArrangementMobile: '2,1',
} )
+ } else {
+ console.warn( 'Columns block not found for mobile arrangement update' )
}
},🤖 Prompt for AI Agents
In src/lazy-components/modal-tour/tours/responsive-controls.js around lines 146
to 157, replace the inconsistent globals wp.data.select/wp.data.dispatch calls
with the already imported select and dispatch from @wordpress/data, check that
columnsBlock exists (and has a clientId) and handle the not-found case (e.g.
early return or log a warning), and wrap the updateBlockAttributes call in a
try/catch or check its result to log an error or provide feedback if the
attribute update fails so the code reliably reports success or failure.
| postStep: () => { | ||
| document.querySelector( '.ugb-sort-control .stk-control-responsive-toggle:not(.is-open) button' )?.click() | ||
| setTimeout( () => { | ||
| document.querySelector( '.ugb-sort-control .stk-control-responsive-toggle button:not(.is-active)[data-value="desktop"]' )?.click() | ||
| }, 100 ) | ||
| }, |
There was a problem hiding this comment.
Additional race condition with nested setTimeout.
Similar to the timing issues in Step 1, this postStep uses a nested setTimeout with a 100ms delay that could cause race conditions.
This compounds the timing concerns already raised. Consider implementing the async/await pattern suggested for Step 1 throughout all steps for consistency.
🤖 Prompt for AI Agents
In src/lazy-components/modal-tour/tours/responsive-controls.js around lines 173
to 178, the postStep uses a nested setTimeout(100) which risks race conditions;
replace the nested timeout with an async postStep that awaits DOM-ready/selector
checks and performs clicks sequentially (first click the closed
responsive-toggle button if present, then await the appearance/state change of
the responsive options, then click the desktop button), using a small
polling/wait-for-selector helper or Promise with requestAnimationFrame instead
of setTimeout to ensure deterministic ordering and remove the nested time-based
delay.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/lazy-components/modal-tour/tours/responsive-controls.js (3)
148-155: Call the importedselect/dispatchhelpers directly.We already import
selectanddispatchat the top, but this block still reaches forwp.data.*. Sticking to the imports keeps the bundle self-contained and avoids depending on the globalwpnamespace (which may not exist in automated contexts).- const allBlocks = wp.data.select( 'core/block-editor' ).getBlocks() + const allBlocks = select( 'core/block-editor' ).getBlocks() @@ - wp.data.dispatch( 'core/block-editor' ).updateBlockAttributes( columnsBlock.clientId, { + dispatch( 'core/block-editor' ).updateBlockAttributes( columnsBlock.clientId, {
10-60: Generate blockuniqueIds dynamically.All six blocks are still created with fixed IDs (
'1dbe04e','f957abc', …'e063798'). If a user runs this tour more than once—or runs it after the hover-states tour that reuses'e063798'—Gutenberg encounters duplicateuniqueIdattributes and throws invalid-block warnings. This was raised earlier and remains unresolved; we need runtime-generated IDs to keep editor state consistent.One approach:
initialize: () => { - // Add some default content that we will select - - const blockObject = wp.blocks.createBlock( + const makeUniqueId = ( prefix = 'tour-' ) => `${ prefix }${ Math.random().toString(36).slice(2, 11) }` + const blockObject = wp.blocks.createBlock( 'stackable/columns', { - uniqueId: '1dbe04e', + uniqueId: makeUniqueId( 'columns-' ), @@ wp.blocks.createBlock( 'stackable/column', { - uniqueId: 'f957abc', + uniqueId: makeUniqueId( 'column-' ), @@ wp.blocks.createBlock( 'stackable/heading', { - uniqueId: 'a8ebea7', + uniqueId: makeUniqueId( 'heading-' ), @@ wp.blocks.createBlock( 'stackable/text', { - uniqueId: '57e76a1', + uniqueId: makeUniqueId( 'text-' ), @@ wp.blocks.createBlock( 'stackable/column', { - uniqueId: '3dcffca', + uniqueId: makeUniqueId( 'column-' ), @@ wp.blocks.createBlock( 'stackable/image', { - uniqueId: 'e063798', + uniqueId: makeUniqueId( 'image-' ),
86-178: Eliminate hard-coded timeouts from tour steps.The tour flow is stitched together with
setTimeout(100/200)(Lines 94-100, 118-121, 175-177). Those arbitrary sleeps are fragile—on slower editors the selectors aren’t ready yet (steps stall), while on faster environments they just add lag. Please replace them with deterministic waits (e.g., an asyncwaitForElementhelper or MutationObserver) so each click happens only after the UI element exists and is in the expected state.Here’s a sketch of how you can restructure the first occurrence; the same helper can be reused for the other steps:
- setTimeout( () => { - // Open Layout tab if it's not open - document.querySelector( '.edit-post-sidebar__panel-tab.ugb-tab--layout:not(.is-active)' )?.click() - }, 100 ) - setTimeout( () => { - document.querySelector( '.ugb-panel--layout:not(.is-opened)' )?.click() - }, 200 ) + const waitForElement = selector => new Promise( resolve => { + const tick = () => { + const node = document.querySelector( selector ) + return node ? resolve( node ) : requestAnimationFrame( tick ) + } + tick() + } ) + + waitForElement( '.edit-post-sidebar__panel-tab.ugb-tab--layout' ) + .then( tab => { + if ( ! tab.classList.contains( 'is-active' ) ) { + tab.click() + } + return waitForElement( '.ugb-panel--layout' ) + } ) + .then( panel => { + if ( ! panel.classList.contains( 'is-opened' ) ) { + panel.click() + } + } )Apply the same waiting strategy for the mobile/desktop toggles later in the tour.
🧹 Nitpick comments (2)
src/components/base-control2/index.js (1)
112-112: LGTM! Data attribute added for guided tour targeting.The addition of the
data-attributealigns well with the PR's guided tour functionality, enabling easier targeting of specific controls.Consider using a more specific attribute name like
data-stk-attributeordata-control-attributeto improve clarity in DevTools and avoid potential naming conflicts with other libraries or user code.- <div className="stk-control-content" data-attribute={ props.attribute }> + <div className="stk-control-content" data-stk-attribute={ props.attribute }>src/lazy-components/modal-tour/tours/hover-states.js (1)
76-83: Use the imported data helpers instead ofwp.data.*.Inside this module we already depend on the bundled build. Leaning on the global
wp.datamixes patterns and can break when the globals are not exposed (e.g., automated tests). Please importselectalongsidedispatchand call those helpers directly for consistency.You can streamline it as follows:
-import { dispatch } from '@wordpress/data' +import { dispatch, select } from '@wordpress/data' @@ - const allBlocks = wp.data.select( 'core/block-editor' ).getBlocks() + const allBlocks = select( 'core/block-editor' ).getBlocks() @@ - wp.data.dispatch( 'core/block-editor' ).updateBlockAttributes( imageBlock.clientId, { + dispatch( 'core/block-editor' ).updateBlockAttributes( imageBlock.clientId, {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/base-control2/index.js(1 hunks)src/components/guided-modal-tour/tour-conditions.js(1 hunks)src/lazy-components/modal-tour/tours/hover-states.js(1 hunks)src/lazy-components/modal-tour/tours/responsive-controls.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/guided-modal-tour/tour-conditions.js
🧰 Additional context used
🧬 Code graph analysis (2)
src/lazy-components/modal-tour/tours/hover-states.js (1)
src/lazy-components/modal-tour/tours/responsive-controls.js (3)
blockObject(10-63)allBlocks(89-89)allBlocks(148-148)
src/lazy-components/modal-tour/tours/responsive-controls.js (1)
src/lazy-components/modal-tour/tours/hover-states.js (2)
blockObject(10-16)allBlocks(77-77)
⏰ 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). (7)
- GitHub Check: build
- GitHub Check: PHP 7.3 and WP 6.5.5
- GitHub Check: PHP 8.2 and WP latest
- GitHub Check: PHP 8.2 and WP 6.7.2
- GitHub Check: PHP 8.2 and WP 6.5.5
- GitHub Check: PHP 7.3 and WP latest
- GitHub Check: PHP 8.2 and WP 6.6.2
| const blockObject = wp.blocks.createBlock( | ||
| 'stackable/image', | ||
| { | ||
| uniqueId: 'e063798', | ||
| imageExternalUrl: 'https://picsum.photos/id/177/500/700.jpg', | ||
| } | ||
| ) |
There was a problem hiding this comment.
Do not reuse hard-coded uniqueId values.
'e063798' is already used by the responsive-controls tour, so running both tours in one editor session produces duplicate block uniqueIds. Gutenberg treats those IDs as primary keys—duplicates trigger invalid-block warnings and can break attribute syncing. Please generate a fresh ID each time you insert the block instead of reusing a constant.
Here is one way to do it:
export const hoverStates = {
initialize: () => {
- // Add some default content that we will select
-
- const blockObject = wp.blocks.createBlock(
+ const makeUniqueId = ( prefix = 'tour-' ) => `${ prefix }${ Math.random().toString(36).slice(2, 11) }`
+ const blockObject = wp.blocks.createBlock(
'stackable/image',
{
- uniqueId: 'e063798',
+ uniqueId: makeUniqueId( 'hover-image-' ),📝 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.
| const blockObject = wp.blocks.createBlock( | |
| 'stackable/image', | |
| { | |
| uniqueId: 'e063798', | |
| imageExternalUrl: 'https://picsum.photos/id/177/500/700.jpg', | |
| } | |
| ) | |
| const makeUniqueId = ( prefix = 'tour-' ) => `${ prefix }${ Math.random().toString(36).slice(2, 11) }` | |
| const blockObject = wp.blocks.createBlock( | |
| 'stackable/image', | |
| { | |
| uniqueId: makeUniqueId( 'hover-image-' ), | |
| imageExternalUrl: 'https://picsum.photos/id/177/500/700.jpg', | |
| } | |
| ) |
🤖 Prompt for AI Agents
In src/lazy-components/modal-tour/tours/hover-states.js around lines 10-16, the
block is created with a hard-coded uniqueId ('e063798') which collides with
other tours; replace the constant with a freshly generated ID each time you
insert the block. Generate a new uniqueId at runtime (for example using a
UUID/timestamp/random generator or the appropriate WP utility) and pass that
generated value into the block attributes instead of the hard-coded string so
each created block has a distinct uniqueId.
| setTimeout( () => { | ||
| document.querySelector( '.edit-post-sidebar__panel-tab.ugb-tab--style:not(.is-active)' )?.click() | ||
| setTimeout( () => { | ||
| document.querySelector( '.ugb-panel--image:not(.is-opened)' )?.click() | ||
| const target = document.querySelector( '.stk-control:has([data-attribute="imageZoom"])' ) | ||
| target?.scrollIntoView( { behavior: 'auto', block: 'center' } ) | ||
| }, 100 ) | ||
| }, 200 ) |
There was a problem hiding this comment.
Replace brittle timeouts with deterministic waits.
The nested setTimeout(200) → setTimeout(100) chain is inherently racey: on slower hosts the elements may not exist yet (tour stalls), while on faster hosts the extra delays make the tour feel unresponsive. Please switch to an explicit wait-for-selector helper (e.g., polling with requestAnimationFrame or a small promise that resolves once document.querySelector finds the panel) so the clicks fire exactly when the UI is ready rather than after arbitrary sleeps.
A safer pattern could look like:
- setTimeout( () => {
- document.querySelector( '.edit-post-sidebar__panel-tab.ugb-tab--style:not(.is-active)' )?.click()
- setTimeout( () => {
- document.querySelector( '.ugb-panel--image:not(.is-opened)' )?.click()
- const target = document.querySelector( '.stk-control:has([data-attribute="imageZoom"])' )
- target?.scrollIntoView( { behavior: 'auto', block: 'center' } )
- }, 100 )
- }, 200 )
+ const waitForElement = selector => new Promise( resolve => {
+ const tick = () => {
+ const node = document.querySelector( selector )
+ return node ? resolve( node ) : requestAnimationFrame( tick )
+ }
+ tick()
+ } )
+
+ waitForElement( '.edit-post-sidebar__panel-tab.ugb-tab--style' )
+ .then( tab => {
+ if ( ! tab.classList.contains( 'is-active' ) ) {
+ tab.click()
+ }
+ return waitForElement( '.ugb-panel--image' )
+ } )
+ .then( panel => {
+ if ( ! panel.classList.contains( 'is-opened' ) ) {
+ panel.click()
+ }
+ document
+ .querySelector( '.stk-control:has([data-attribute="imageZoom"])' )
+ ?.scrollIntoView( { behavior: 'auto', block: 'center' } )
+ } )📝 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.
| setTimeout( () => { | |
| document.querySelector( '.edit-post-sidebar__panel-tab.ugb-tab--style:not(.is-active)' )?.click() | |
| setTimeout( () => { | |
| document.querySelector( '.ugb-panel--image:not(.is-opened)' )?.click() | |
| const target = document.querySelector( '.stk-control:has([data-attribute="imageZoom"])' ) | |
| target?.scrollIntoView( { behavior: 'auto', block: 'center' } ) | |
| }, 100 ) | |
| }, 200 ) | |
| const waitForElement = selector => new Promise( resolve => { | |
| const tick = () => { | |
| const node = document.querySelector( selector ) | |
| return node ? resolve( node ) : requestAnimationFrame( tick ) | |
| } | |
| tick() | |
| } ) | |
| waitForElement( '.edit-post-sidebar__panel-tab.ugb-tab--style' ) | |
| .then( tab => { | |
| if ( ! tab.classList.contains( 'is-active' ) ) { | |
| tab.click() | |
| } | |
| return waitForElement( '.ugb-panel--image' ) | |
| } ) | |
| .then( panel => { | |
| if ( ! panel.classList.contains( 'is-opened' ) ) { | |
| panel.click() | |
| } | |
| document | |
| .querySelector( '.stk-control:has([data-attribute="imageZoom"])' ) | |
| ?.scrollIntoView( { behavior: 'auto', block: 'center' } ) | |
| } ) |
🤖 Prompt for AI Agents
In src/lazy-components/modal-tour/tours/hover-states.js around lines 24 to 31,
replace the nested setTimeout calls with deterministic waits: implement a small
waitForSelector helper that polls (via requestAnimationFrame or setInterval)
until document.querySelector(selector) returns an element or a timeout elapses,
then use it to await the first selector
'.edit-post-sidebar__panel-tab.ugb-tab--style:not(.is-active)' and click it,
then await the second selector '.ugb-panel--image:not(.is-opened)' and click it,
then query '.stk-control:has([data-attribute="imageZoom"])' and scrollIntoView;
remove the arbitrary 200/100ms sleeps so the actions run only once the UI
elements are present.
Summary by CodeRabbit
New Features
Style
Chores