feat: onboarding tour with name changes #3
Conversation
WalkthroughAdds a guided modal tour system: React components and utilities for guided tours, a modal tour renderer with step/position logic, tour definitions and discovery, SCSS styling, PHP settings persistence, and multiple editor integration points via component renders and a require_once in interactions.php. Changes
Sequence Diagram(s)sequenceDiagram
participant Editor as Editor UI
participant GMT as GuidedModalTour
participant Util as Tour State Manager
participant MT as ModalTour
participant WP as WordPress Settings
Editor->>GMT: Render GuidedModalTour(tourId)
GMT->>Util: isTourActive() / getActiveTourId()
alt Active tour exists
GMT-->>Editor: Do not mount ModalTour
else No active tour
GMT->>Util: addTourStateListener(...)
GMT->>WP: read guidedTourStates
alt Tour should show
GMT->>MT: Mount ModalTour (lazy)
GMT->>Util: setActiveTour(tourId)
MT->>MT: initialize, compute position, show step
loop User interaction
MT->>MT: preStep/postStep, handle next/back, maybe nextEvent
MT->>MT: update step, reposition, show glow
end
MT->>GMT: onClose
GMT->>Util: clearActiveTour()
GMT->>WP: update guidedTourStates (append tourId)
GMT->>Editor: remove tour query param
else Skip render
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (13)
src/editor/getting-started.php (2)
14-16: Fix indentation consistency.The indentation appears to use spaces instead of tabs, which is inconsistent with WordPress PHP coding standards. Ensure consistent use of tabs for indentation throughout the file.
Apply this diff to fix the indentation:
- // Register settings. - add_action( 'admin_init', array( $this, 'register_settings' ) ); - add_action( 'rest_api_init', array( $this, 'register_settings' ) ); + // Register settings. + add_action( 'admin_init', array( $this, 'register_settings' ) ); + add_action( 'rest_api_init', array( $this, 'register_settings' ) );
32-32: Clarify description text.The description mentions "block tours" but this setting is for guided interaction tours. Consider updating the text to accurately reflect its purpose.
Apply this diff:
-'description' => __( 'An array of strings representing completed block tours.', 'interactions' ), +'description' => __( 'An array of strings representing completed guided tours.', 'interactions' ),src/editor/components/modal-tour/tours/index.js (2)
22-30: Consider making the export pattern more explicit.The fallback logic that takes the first named export (line 28) could be unpredictable if a tour file accidentally exports multiple named items. Consider either:
- Documenting that tour files should always use default exports OR a single named export matching the filename
- Adding a warning when multiple named exports are found
- Being more specific about which named export to use (e.g., matching the filename)
Consider adding validation:
// Prefer default export, fallback to first named export if available if ( tourModule.default ) { tours[ tourName ] = tourModule.default } else { // If no default export, use the first named export (if any) const namedExports = Object.keys( tourModule ).filter( name => name !== 'default' ) if ( namedExports.length > 0 ) { + if ( namedExports.length > 1 ) { + console.warn( `Tour file ${ fileName } has multiple named exports. Using the first one: ${ namedExports[ 0 ] }` ) + } tours[ tourName ] = tourModule[ namedExports[ 0 ] ] } }
5-5: Consider adding file extension filter for clarity.The regex
/\.js$/will match any.jsfile, but for better maintainability and to avoid accidentally picking up other JavaScript files (like test files if added later), consider being more explicit or adding a naming convention check.You could document that tour files should follow a specific naming pattern, or update the pattern to be more restrictive:
// Only match files that look like tour definitions (no .test.js, .spec.js, etc.) const tourContext = require.context( './', false, /^(?!.*\.(test|spec)).*\.js$/ )src/editor/components/guided-modal-tour/editor.scss (2)
64-64: Remove or uncomment the commented border style.The commented-out border style should either be removed if not needed or uncommented if it serves a purpose during development.
border-radius: 16px; - // border: 1px solid #f00069ad; }
178-190: Clean up commented styles or document their purpose.The step indicator styling includes commented-out cursor and hover styles. If these are intentionally disabled, consider adding a comment explaining why. Otherwise, remove them to keep the code clean.
height: 8px; border-radius: 20px; background-color: #e1e1e1; - // cursor: pointer; padding: 0 !important; margin: 0 !important; &--active { background: #05f; width: 24px; border-radius: 20px; } - - // &:hover { - // background-color: #aaa; - // } }src/editor/components/guided-modal-tour/index.js (1)
62-63: Add defensive error handling for condition evaluation.The condition function is called without error handling. If a tour condition throws an exception, it will break the entire component.
Add a try-catch wrapper:
const condition = TOUR_CONDITIONS[ tourId ] - const conditionResult = condition ? condition() : null + let conditionResult = null + if ( condition ) { + try { + conditionResult = condition() + } catch ( error ) { + console.error( `Error evaluating condition for tour "${ tourId }":`, error ) + conditionResult = null // Fallback to default behavior + } + }src/editor/components/modal-tour/tours/editor.js (1)
13-17: Stabilize and de‑duplicate the button selectorThe same selector is repeated 3x. Centralize it to avoid drift and consider moving to a stable data-* hook to survive class renames.
Apply this diff:
import { __ } from '@wordpress/i18n' import { createInterpolateElement } from '@wordpress/element' +// Prefer a stable data-* selector in markup; fallback to class until available. +const INSERT_BTN_SEL = '.interact-insert-library-button' + export const editor = { hasConfetti: false, steps: [ { title: '👋 ' + __( 'Welcome to Interactions', 'interactions' ), description: __( 'Transform your WordPress site with animations and dynamic interactions that bring your content to life. Let’s get started by exploring the Interaction Library.', 'interactions' ), help: createInterpolateElement( __( 'Click the <strong>Interactions</strong> button to continue.', 'interactions' ), { strong: <strong />, } ), - anchor: '.interact-insert-library-button', + anchor: INSERT_BTN_SEL, position: 'bottom', - nextEventTarget: '.interact-insert-library-button', - glowTarget: '.interact-insert-library-button', + nextEventTarget: INSERT_BTN_SEL, + glowTarget: INSERT_BTN_SEL, showNext: false, }, ], }src/editor/components/guided-modal-tour/util.js (1)
17-20: Avoid redundant notifications and validate listenersMinor polish: skip notifying when the value is unchanged, and guard listener type.
Apply this diff:
export const setActiveTour = tourId => { - activeTourId = tourId - tourStateListeners.forEach( listener => listener( tourId ) ) + if ( activeTourId === tourId ) return + activeTourId = tourId + tourStateListeners.forEach( listener => listener( tourId ) ) } export const clearActiveTour = () => { - activeTourId = null - tourStateListeners.forEach( listener => listener( null ) ) + if ( activeTourId === null ) return + activeTourId = null + tourStateListeners.forEach( listener => listener( null ) ) } export const addTourStateListener = listener => { - tourStateListeners.add( listener ) + if ( typeof listener !== 'function' ) { + // No-op remove function for invalid input + return () => {} + } + tourStateListeners.add( listener ) return () => tourStateListeners.delete( listener ) }Also applies to: 25-28, 50-53
src/editor/components/modal-tour/tours/sidebar.js (2)
26-33: Honor reduced‑motion preference when scrollingRespect users with motion sensitivity by disabling smooth scroll when prefers-reduced-motion is set.
Apply this diff:
preStep: () => { - // Scroll to the preview button before moving to the next step. - document.querySelector( '.interact-timeline__preview-button' )?.scrollIntoView( { - behavior: 'smooth', - block: 'center', - } ) + // Scroll to the preview button before moving to the next step. + const btn = document.querySelector( '.interact-timeline__preview-button' ) + if ( ! btn ) return + const prefersReduced = + typeof window !== 'undefined' && + window.matchMedia?.('(prefers-reduced-motion: reduce)')?.matches + btn.scrollIntoView( { + behavior: prefersReduced ? 'auto' : 'smooth', + block: 'center', + } ) },
47-61: Add guards around data store accessThe block editor store should exist here, but add safe checks to avoid rare runtime errors in atypical contexts.
Apply this diff:
- const blocks = select( 'core/block-editor' ).getBlocks() + const editorStore = select( 'core/block-editor' ) + const blocks = editorStore?.getBlocks?.() || [] @@ - dispatch( 'core/block-editor' ).selectBlock( innermostBlock.clientId ) + dispatch( 'core/block-editor' )?.selectBlock?.( innermostBlock.clientId )src/editor/components/modal-tour/index.js (2)
193-202: Include skipIf in deps and guard against exceptionsEnsure the latest skipIf is used and defensive against errors.
Apply this diff:
-useEffect( () => { - if ( skipIf() ) { +useEffect( () => { + let shouldSkip = false + try { + shouldSkip = !!skipIf() + } catch (e) { + shouldSkip = false + } + if ( shouldSkip ) { if ( direction === 'forward' ) { handleNextEvent() } else { handleBackEvent() } } -}, [ currentStep, direction ] ) +}, [ currentStep, direction, skipIf, handleNextEvent, handleBackEvent ] )
342-345: Correct glow size thresholds (medium branch unreachable for width)The second condition repeats width > 300, making the width path for medium unreachable. Use a lower width threshold.
Apply this diff:
- const glowTargetSize = targetRect.width > 300 || targetRect.height > 200 ? 'large' - : targetRect.width > 300 || targetRect.height > 100 ? 'medium' + const glowTargetSize = targetRect.width > 300 || targetRect.height > 200 ? 'large' + : targetRect.width > 200 || targetRect.height > 100 ? 'medium' : 'small'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
interactions.php(1 hunks)src/editor/components/guided-modal-tour/README.MD(1 hunks)src/editor/components/guided-modal-tour/editor.scss(1 hunks)src/editor/components/guided-modal-tour/index.js(1 hunks)src/editor/components/guided-modal-tour/tour-conditions.js(1 hunks)src/editor/components/guided-modal-tour/util.js(1 hunks)src/editor/components/index.js(1 hunks)src/editor/components/interaction-panel/index.js(2 hunks)src/editor/components/modal-tour/index.js(1 hunks)src/editor/components/modal-tour/tour-steps.js(1 hunks)src/editor/components/modal-tour/tours/README.md(1 hunks)src/editor/components/modal-tour/tours/editor.js(1 hunks)src/editor/components/modal-tour/tours/index.js(1 hunks)src/editor/components/modal-tour/tours/interaction-library.js(1 hunks)src/editor/components/modal-tour/tours/sidebar.js(1 hunks)src/editor/components/timeline/index.js(1 hunks)src/editor/editor.js(2 hunks)src/editor/getting-started.php(1 hunks)src/editor/interaction-library/index.js(3 hunks)src/editor/interaction-library/select-modal.js(1 hunks)src/editor/plugins/block-toolbar-button/index.js(1 hunks)src/editor/plugins/top-toolbar-button/add-interaction-button.js(1 hunks)src/editor/plugins/top-toolbar-button/index.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/editor/components/modal-tour/tour-steps.js (1)
src/editor/components/modal-tour/tours/index.js (1)
tours(6-6)
src/editor/editor.js (1)
src/editor/components/guided-modal-tour/index.js (1)
GuidedModalTour(27-104)
src/editor/components/interaction-panel/index.js (1)
src/editor/components/guided-modal-tour/index.js (1)
GuidedModalTour(27-104)
src/editor/interaction-library/index.js (1)
src/editor/components/guided-modal-tour/index.js (1)
GuidedModalTour(27-104)
src/editor/components/guided-modal-tour/index.js (2)
src/editor/components/guided-modal-tour/util.js (8)
isTourActive(35-35)isTourActive(35-35)getActiveTourId(42-42)getActiveTourId(42-42)addTourStateListener(50-53)addTourStateListener(50-53)clearActiveTour(25-28)clearActiveTour(25-28)src/editor/components/guided-modal-tour/tour-conditions.js (2)
TOUR_CONDITIONS(2-2)TOUR_CONDITIONS(2-2)
src/editor/components/modal-tour/index.js (2)
src/editor/components/modal-tour/tour-steps.js (2)
TOUR_STEPS(4-6)TOUR_STEPS(4-6)src/editor/components/guided-modal-tour/util.js (6)
setActiveTour(17-20)setActiveTour(17-20)getActiveTourId(42-42)getActiveTourId(42-42)clearActiveTour(25-28)clearActiveTour(25-28)
🪛 LanguageTool
src/editor/components/modal-tour/tours/README.md
[style] ~65-~65: To form a complete sentence, be sure to include a subject.
Context: ...tring, optional) The size of the modal. Can be: - 'small' (default) - 'medium' ...
(MISSING_IT_THERE)
[style] ~74-~74: To form a complete sentence, be sure to include a subject.
Context: ...on of the modal relative to the anchor. Can be: - 'left' - 'right' - 'top' - ...
(MISSING_IT_THERE)
🔇 Additional comments (22)
src/editor/plugins/block-toolbar-button/index.js (1)
130-130: LGTM! Clean styling hook addition.The className addition follows the project's naming convention and enables targeted styling or selection for the guided tour feature without affecting existing functionality.
src/editor/components/guided-modal-tour/README.MD (1)
1-36: LGTM! Clear and helpful documentation.The documentation provides a well-structured guide for creating new tours with clear steps and examples. This will help developers implement tours consistently across the codebase.
src/editor/getting-started.php (1)
23-45: LGTM! Setting registration is well-structured.The setting registration properly exposes the array to the REST API with appropriate schema and sanitization. The default empty array is sensible.
src/editor/components/index.js (1)
11-11: LGTM! Export follows existing patterns.The GuidedModalTour export is correctly added and follows the same pattern as other component exports in this file.
src/editor/components/interaction-panel/index.js (2)
2-2: LGTM! Import is correctly added.The GuidedModalTour import is properly added to the existing import statement.
636-636: LGTM! Tour integration is well-placed.The GuidedModalTour component with
tourId="sidebar"is appropriately placed at the end of the InteractionPanel render, allowing it to overlay the panel content when active.src/editor/components/modal-tour/tour-steps.js (1)
1-6: LGTM! Clean aggregation pattern.The file provides a clean centralized export for tour steps by spreading the tours object. This pattern allows for easy extension and maintains a single source of truth.
src/editor/interaction-library/index.js (3)
12-12: LGTM! Import is correctly added.The GuidedModalTour import is properly added and follows the existing import pattern.
197-200: Verify setTimeout doesn't cause issues on unmount.Wrapping the state resets in a 100ms
setTimeoutmay cause issues if the component unmounts before the timeout fires, potentially leading to React warnings about setting state on an unmounted component. Consider cleanup in auseEffectreturn or using a ref to track mount status.If this delay is intentional (e.g., to allow the tour to render before state cleanup), consider adding a comment explaining the rationale and verifying that cleanup is properly handled:
useEffect(() => { let timeoutId; // Cleanup function return () => { if (timeoutId) { clearTimeout(timeoutId); } }; }, []);
276-276: LGTM! Tour placement is appropriate.The GuidedModalTour component is correctly placed inside the Modal, allowing it to display the tour overlay for the interaction library.
interactions.php (1)
138-138: LGTM! File inclusion is properly placed.The
require_oncefor getting-started.php is correctly added with the proper path and follows the existing pattern in the file.src/editor/components/timeline/index.js (1)
1113-1113: LGTM! CSS class added for styling/targeting.The
className="interact-timeline__preview-button"attribute provides a styling hook and likely helps the tour system target this button. The BEM naming convention is consistent with the codebase.src/editor/components/guided-modal-tour/tour-conditions.js (1)
1-2: LGTM - Placeholder for future tour conditions.The empty
TOUR_CONDITIONSobject with clear documentation is ready for future per-tour condition overrides. Note that individual tour configurations can also define their ownconditionfunctions (as documented in the README), so this serves as a centralized override mechanism when needed.src/editor/components/modal-tour/tours/README.md (2)
1-164: Excellent documentation for the tour system.The README provides comprehensive guidance on creating tours, including:
- Tour structure and properties
- Step-level configuration options
- Complete working examples
- Clear explanations of the discovery mechanism
This will significantly help future contributors create new tours.
3-3: No issues found—the referenced file path is correct.The file
src/editor/components/guided-modal-tour/README.MDexists exactly as referenced in the documentation with the uppercase.MDextension. The path and case are correct and require no changes.src/editor/interaction-library/select-modal.js (1)
96-96: LGTM - Enables category-specific targeting for guided tours.Adding per-category CSS classes allows the guided tour system to precisely target and highlight specific categories. This is a clean implementation that aligns with the tour's
glowTargetfunctionality seen insrc/editor/components/modal-tour/tours/interaction-library.js.src/editor/components/modal-tour/tours/interaction-library.js (1)
1-51: LGTM - Well-structured interactive tour.The interaction library tour provides a clear, step-by-step onboarding experience:
- Proper i18n for all user-facing text
- Interactive steps that wait for user actions
- Appropriate use of glow targets and anchoring
The offset values use string format (e.g.,
'-400px'), which should work with CSS custom properties but consider verifying this matches the expected format in the ModalTour component.src/editor/components/guided-modal-tour/editor.scss (1)
1-236: Comprehensive and well-structured styling.The SCSS provides excellent visual polish for the guided tour system:
- Proper z-index layering for overlay and glow effects
- Smooth transitions and animations
- Flexible positioning system with CSS custom properties
- Multiple position variants for different anchoring scenarios
src/editor/components/guided-modal-tour/index.js (2)
42-47: LGTM - Proper cleanup of event listener.The effect hook correctly subscribes to tour state changes and returns the cleanup function to remove the listener on unmount or when
tourIdchanges.
27-30: LGTM - Using memo appropriately.The component is wrapped in
memowhich is appropriate given it only depends on thetourIdprop and includes internal state management.src/editor/editor.js (2)
9-9: GuidedModalTour import is correctly resolved.Verification confirms that
GuidedModalTouris properly exported through the import chain:
src/editor/components/index.jsline 11 re-exports it from./guided-modal-toursrc/editor/components/guided-modal-tour/index.jsline 106 exports it as defaultThe import in
src/editor/editor.jsresolves correctly. No issues found.
449-449: No issues found — editor tour is properly defined and integrated.The editor tour exists at
src/editor/components/modal-tour/tours/editor.jswith a well-formed export structure. The dynamic discovery pattern inindex.jscorrectly handles the named export, and the integration throughtour-steps.jsto theModalTourcomponent works as intended. ThetourId="editor"prop will correctly resolve to the tour definition.
| // Update the interact_guided_tour_states setting | ||
| if ( ! guidedTourStates.includes( tourId ) ) { | ||
| // eslint-disable-next-line camelcase | ||
| const settings = new models.Settings( { interact_guided_tour_states: [ ...guidedTourStates, tourId ] } ) | ||
| settings.save() | ||
| } |
There was a problem hiding this comment.
Add error handling for settings persistence.
The settings.save() call lacks error handling. If the API request fails, the tour will be marked as complete locally but not persisted, leading to the tour reappearing on page reload.
Add error handling:
// Update the interact_guided_tour_states setting
if ( ! guidedTourStates.includes( tourId ) ) {
// eslint-disable-next-line camelcase
const settings = new models.Settings( { interact_guided_tour_states: [ ...guidedTourStates, tourId ] } )
- settings.save()
+ settings.save().catch( error => {
+ console.error( 'Failed to save tour state:', error )
+ // Optionally: notify the user or retry
+ } )
}📝 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.
| // Update the interact_guided_tour_states setting | |
| if ( ! guidedTourStates.includes( tourId ) ) { | |
| // eslint-disable-next-line camelcase | |
| const settings = new models.Settings( { interact_guided_tour_states: [ ...guidedTourStates, tourId ] } ) | |
| settings.save() | |
| } | |
| // Update the interact_guided_tour_states setting | |
| if ( ! guidedTourStates.includes( tourId ) ) { | |
| // eslint-disable-next-line camelcase | |
| const settings = new models.Settings( { interact_guided_tour_states: [ ...guidedTourStates, tourId ] } ) | |
| settings.save().catch( error => { | |
| console.error( 'Failed to save tour state:', error ) | |
| // Optionally: notify the user or retry | |
| } ) | |
| } |
🤖 Prompt for AI Agents
In src/editor/components/guided-modal-tour/index.js around lines 86–91, the
settings.save() call is invoked without error handling which lets the tour be
marked complete locally even if the API save fails; change the flow to persist
first and only update local guidedTourStates on successful save (or, if you must
update optimistically, add a catch to revert the local state), and in the catch
log the error and surface a user-facing notification (or retry) so failures are
visible and data remains consistent.
| } | ||
|
|
||
| // Soft update the global variable to prevent the tour from being shown again. | ||
| guidedTourStates.push( tourId ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid direct mutation of the imported array.
Directly mutating guidedTourStates with .push() is a side effect that could lead to unexpected behavior in React. While this appears to be a cache optimization after the API call, consider using immutable updates or documenting this as an intentional cache sync.
If guidedTourStates is meant to be mutated as a global cache, add a comment explaining this. Otherwise, consider if this mutation is necessary since the component already handles state with setIsDone(true):
// Soft update the global variable to prevent the tour from being shown again.
+ // Note: This mutates the global cache to prevent re-renders in other components
guidedTourStates.push( tourId )Based on learnings: In React, direct mutations of imported data structures should be avoided or clearly documented as intentional cache synchronization.
📝 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.
| guidedTourStates.push( tourId ) | |
| // Soft update the global variable to prevent the tour from being shown again. | |
| // Note: This mutates the global cache to prevent re-renders in other components | |
| guidedTourStates.push( tourId ) |
| const modalRect = modalRef.current.querySelector( '.interact-tour-modal' ).getBoundingClientRect() | ||
| const defaultOffset = [ `${ ( window.innerWidth / 2 ) - ( modalRect.width / 2 ) }px`, `${ ( window.innerHeight / 2 ) - ( modalRect.height / 2 ) }px` ] | ||
|
|
||
| if ( ! anchor ) { | ||
| return defaultOffset // This is for the entire screen. | ||
| } | ||
|
|
||
| // Based on the anchor and position, calculate the X and Y offsets of the modal relative to the anchor. | ||
| // We have the modalRef.current which we can use to get the modal's bounding client rect. | ||
| const anchorRect = document.querySelector( anchor )?.getBoundingClientRect() | ||
|
|
||
| if ( ! anchorRect ) { | ||
| return defaultOffset | ||
| } |
There was a problem hiding this comment.
Guard modal/anchor queries to avoid runtime errors
querySelector may be unavailable or return null; bail out safely.
Apply this diff:
- const modalRect = modalRef.current.querySelector( '.interact-tour-modal' ).getBoundingClientRect()
+ const modalEl = modalRef.current?.querySelector?.( '.interact-tour-modal' )
+ if ( ! modalEl ) {
+ return [ '', '' ]
+ }
+ const modalRect = modalEl.getBoundingClientRect()
@@
- const anchorRect = document.querySelector( anchor )?.getBoundingClientRect()
+ const anchorRect = document.querySelector( anchor )?.getBoundingClientRect()📝 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 modalRect = modalRef.current.querySelector( '.interact-tour-modal' ).getBoundingClientRect() | |
| const defaultOffset = [ `${ ( window.innerWidth / 2 ) - ( modalRect.width / 2 ) }px`, `${ ( window.innerHeight / 2 ) - ( modalRect.height / 2 ) }px` ] | |
| if ( ! anchor ) { | |
| return defaultOffset // This is for the entire screen. | |
| } | |
| // Based on the anchor and position, calculate the X and Y offsets of the modal relative to the anchor. | |
| // We have the modalRef.current which we can use to get the modal's bounding client rect. | |
| const anchorRect = document.querySelector( anchor )?.getBoundingClientRect() | |
| if ( ! anchorRect ) { | |
| return defaultOffset | |
| } | |
| const modalEl = modalRef.current?.querySelector?.( '.interact-tour-modal' ) | |
| if ( ! modalEl ) { | |
| return [ '', '' ] | |
| } | |
| const modalRect = modalEl.getBoundingClientRect() | |
| const defaultOffset = [ `${ ( window.innerWidth / 2 ) - ( modalRect.width / 2 ) }px`, `${ ( window.innerHeight / 2 ) - ( modalRect.height / 2 ) }px` ] | |
| if ( ! anchor ) { | |
| return defaultOffset // This is for the entire screen. | |
| } | |
| // Based on the anchor and position, calculate the X and Y offsets of the modal relative to the anchor. | |
| // We have the modalRef.current which we can use to get the modal's bounding client rect. | |
| const anchorRect = document.querySelector( anchor )?.getBoundingClientRect() | |
| if ( ! anchorRect ) { | |
| return defaultOffset | |
| } |
🤖 Prompt for AI Agents
In src/editor/components/modal-tour/index.js around lines 278 to 291, the code
calls modalRef.current.querySelector(...) and
document.querySelector(anchor).getBoundingClientRect() without guarding for
null/undefined which can throw; update the logic to first verify modalRef and
modalRef.current exist and that querySelector returns an element before calling
getBoundingClientRect, and likewise check document.querySelector(anchor) exists
before using its rect; if any of these are missing (or window is undefined),
immediately return the defaultOffset to bail out safely.
| useEffect( () => { | ||
| const handleHeaderClick = () => { | ||
| onClose() | ||
| } | ||
| if ( modalRef.current ) { | ||
| modalRef.current.querySelector( '.components-modal__header > .components-button' ).addEventListener( 'click', handleHeaderClick ) | ||
| } | ||
| return () => { | ||
| if ( modalRef.current ) { | ||
| modalRef.current.querySelector( '.components-modal__header > .components-button' ).removeEventListener( 'click', handleHeaderClick ) | ||
| } | ||
| } | ||
| }, [ modalRef.current, onClose ] ) |
There was a problem hiding this comment.
Null‑safe close‑button hook and stable dependencies
Query can return null; also avoid using ref.current in deps. Hook the close button safely and re-run when the modal shows.
Apply this diff:
-useEffect( () => {
- const handleHeaderClick = () => {
- onClose()
- }
- if ( modalRef.current ) {
- modalRef.current.querySelector( '.components-modal__header > .components-button' ).addEventListener( 'click', handleHeaderClick )
- }
- return () => {
- if ( modalRef.current ) {
- modalRef.current.querySelector( '.components-modal__header > .components-button' ).removeEventListener( 'click', handleHeaderClick )
- }
- }
-}, [ modalRef.current, onClose ] )
+useEffect( () => {
+ const handleHeaderClick = () => onClose()
+ const closeBtn = modalRef.current?.querySelector?.( '.components-modal__header > .components-button' )
+ closeBtn?.addEventListener( 'click', handleHeaderClick )
+ return () => closeBtn?.removeEventListener( 'click', handleHeaderClick )
+}, [ isVisible, onClose ] )Optional: for accessibility, consider wiring onRequestClose={ onClose } to support ESC key closing while still keeping shouldCloseOnClickOutside={ false }.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/editor/components/modal-tour/index.js around lines 361 to 373, the effect
assumes querySelector returns an element and uses modalRef.current in the
dependency array; update the effect to guard against null from querySelector
before adding/removing the listener, reference modalRef (not modalRef.current)
in the deps so React tracks the ref object, and include the modal visibility
prop (e.g. isOpen or similar) so the hook re-runs when the modal is shown;
attach the click listener only if the button element exists and remove it in the
cleanup safely.
| } | ||
| } } | ||
| className="ugb-insert-library-button" | ||
| className="interact-insert-library-button" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify selector compatibility with the wrapper element.
The className was updated to interact-insert-library-button, but in src/editor/plugins/top-toolbar-button/index.js (line 25), the code searches for .interact-insert-library-button__wrapper. This selector won't match the button's className and also won't match the wrapper div's className (interact-add-interaction-button-wrapper from line 13 of index.js).
Please confirm whether:
- There's another element with the
interact-insert-library-button__wrapperclass not shown in this diff, or - The selector in index.js should be updated to match the actual wrapper className
🏁 Script executed:
#!/bin/bash
# Search for any elements with the class "interact-insert-library-button__wrapper"
# to verify if this selector will match any elements
rg -n "interact-insert-library-button__wrapper" --type js --type jsx --type ts --type tsx -C3Length of output: 27
Let me now search the codebase directly for the files and examine them:
Verify selector compatibility with the wrapper element.
The className was updated to interact-insert-library-button, but code in src/editor/plugins/top-toolbar-button/index.js searches for .interact-insert-library-button__wrapper. Confirm whether this selector exists in the codebase or if it needs updating to match the actual wrapper classNames.
🏁 Script executed:
#!/bin/bash
# Search for the selector using correct ripgrep syntax
# The "js" type includes .js, .jsx, and .vue files
rg -n "interact-insert-library-button__wrapper" -tjs -C3Length of output: 844
🏁 Script executed:
#!/bin/bash
# Cat the two files to examine them directly
echo "=== add-interaction-button.js (lines 20-35) ==="
cat src/editor/plugins/top-toolbar-button/add-interaction-button.js | sed -n '20,35p'
echo -e "\n=== index.js (lines 1-30) ==="
cat src/editor/plugins/top-toolbar-button/index.js | sed -n '1,30p'
echo -e "\n=== Search for the selector anywhere in the codebase ==="
rg "interact-insert-library-button__wrapper" -tjs -C2Length of output: 2414
Update selector in index.js to match the actual button className.
The verification confirms the original review comment's concern: the selector .interact-insert-library-button__wrapper does not exist anywhere in the codebase. The button element has class interact-insert-library-button (without the __wrapper suffix), and the wrapper div has class interact-add-interaction-button-wrapper.
Line 25 of index.js must be updated to query for an element that actually exists. Either update the selector to .interact-insert-library-button, or reconsider if this fallback positioning logic is still needed.
🤖 Prompt for AI Agents
In src/editor/plugins/top-toolbar-button/add-interaction-button.js around line
26, the selector `.interact-insert-library-button__wrapper` does not match any
element; update the query to use the actual class
`.interact-insert-library-button` (or, if you intended to target the wrapper,
use `.interact-add-interaction-button-wrapper`) so the fallback positioning
logic finds the element; after updating the selector, run the fallback path to
confirm positioning still behaves as expected and remove the fallback logic if
it is no longer necessary.
| // If .interact-insert-library-button__wrapper button is present, add after this button. | ||
| const insertLibraryButton = toolbar.querySelector( '.interact-insert-library-button__wrapper' ) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the selector targets an existing element.
The selector was updated to .interact-insert-library-button__wrapper, aligning with the className refactoring. However, the button created in add-interaction-button.js has className interact-insert-library-button (without __wrapper suffix), and the wrapper div created in line 13 has className interact-add-interaction-button-wrapper.
The fallback to appendChild (line 29) ensures the button will still be added even if the selector doesn't match, but please verify that .interact-insert-library-button__wrapper matches an actual DOM element for proper insertion positioning.
🏁 Script executed:
#!/bin/bash
# Search for where "interact-insert-library-button__wrapper" class is defined
# to verify the selector will match an actual element
rg -n "interact-insert-library-button__wrapper" --type js --type jsx --type ts --type tsx --type css --type scss -C3Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for the selector string across all files (fixing the file type error)
echo "=== Searching for 'interact-insert-library-button__wrapper' ==="
rg -n "interact-insert-library-button__wrapper" -C3
echo ""
echo "=== Searching for 'interact-insert-library-button' (broader search) ==="
rg -n "interact-insert-library-button" -C3
echo ""
echo "=== Checking add-interaction-button.js for actual classNames ==="
fd -n "add-interaction-button.js" --exec cat {}Length of output: 3661
🏁 Script executed:
#!/bin/bash
# View the add-interaction-button.js file to understand the component structure
echo "=== Content of add-interaction-button.js ==="
cat src/editor/plugins/top-toolbar-button/add-interaction-button.js
echo ""
echo "=== Check what wraps the button (look for parent elements) ==="
rg -n "interact-insert-library-button" -B5 -A5 src/editor/plugins/top-toolbar-button/add-interaction-button.jsLength of output: 1368
Update selector to match the button's actual className.
The selector on line 25 searches for .interact-insert-library-button__wrapper, but the button in add-interaction-button.js has className interact-insert-library-button (without the __wrapper suffix). This mismatch means the selector will never find the element, causing the code to always fall back to appendChild, which breaks the intended positioning.
Update line 25 to:
const insertLibraryButton = toolbar.querySelector( '.interact-insert-library-button' )🤖 Prompt for AI Agents
In src/editor/plugins/top-toolbar-button/index.js around lines 24 to 25, the
querySelector uses the wrong class name
('.interact-insert-library-button__wrapper') which does not match the actual
button class; change the selector to '.interact-insert-library-button' so the
code finds the existing button and inserts in the correct position instead of
always falling back to appendChild.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/editor/components/modal-tour/index.js (1)
367-380: Duplicate: Close button handler has unstable dependencies.This effect still includes
modalRef.currentin the dependency array, which is problematic because ref mutations don't trigger re-renders. The previous review comment suggested usingisVisiblein the dependencies instead and adding proper guards forquerySelector.For reference, the suggested fix from the previous review:
-useEffect( () => { - const handleHeaderClick = () => { - onClose() - } - if ( modalRef.current ) { - modalRef.current.querySelector( '.components-modal__header > .components-button' ).addEventListener( 'click', handleHeaderClick ) - } - return () => { - if ( modalRef.current ) { - modalRef.current.querySelector( '.components-modal__header > .components-button' ).removeEventListener( 'click', handleHeaderClick ) - } - } -}, [ modalRef.current, onClose ] ) +useEffect( () => { + const handleHeaderClick = () => onClose() + const closeBtn = modalRef.current?.querySelector?.( '.components-modal__header > .components-button' ) + closeBtn?.addEventListener( 'click', handleHeaderClick ) + return () => closeBtn?.removeEventListener( 'click', handleHeaderClick ) +}, [ isVisible, onClose ] )
🧹 Nitpick comments (2)
src/editor/components/modal-tour/index.js (2)
194-203: Include callback dependencies in effect array.The effect calls
handleNextEventandhandleBackEventbut doesn't list them in the dependency array. While they're stable due touseCallback, including them follows React best practices and satisfies the exhaustive-deps rule.Apply this diff:
// If we just moved to this step, even before showing it check if we should skip it, if so, move to the next/prev step. useEffect( () => { if ( skipIf() ) { if ( direction === 'forward' ) { handleNextEvent() } else { handleBackEvent() } } - }, [ currentStep, direction ] ) + }, [ currentStep, direction, handleNextEvent, handleBackEvent, skipIf ] )
274-338: RemovemodalRef.currentfrom dependencies.Including
modalRef.currentin theuseMemodependency array is an anti-pattern because ref mutations don't trigger re-renders and readingref.currentduring render is unreliable. TheforceRefreshstate already handles re-computation when needed.Apply this diff:
- }, [ anchor, position, modalRef.current, isVisible, isVisibleDelayed, isTransitioning, forceRefresh ] ) + }, [ anchor, position, isVisible, isVisibleDelayed, isTransitioning, forceRefresh ] )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/editor/components/guided-modal-tour/index.js(1 hunks)src/editor/components/modal-tour/index.js(1 hunks)src/editor/components/modal-tour/tours/sidebar.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/editor/components/modal-tour/tours/sidebar.js
🧰 Additional context used
🧬 Code graph analysis (2)
src/editor/components/guided-modal-tour/index.js (3)
src/editor/components/modal-tour/index.js (2)
props(496-499)ModalTour(33-464)src/editor/components/guided-modal-tour/util.js (8)
isTourActive(35-35)isTourActive(35-35)getActiveTourId(42-42)getActiveTourId(42-42)addTourStateListener(50-53)addTourStateListener(50-53)clearActiveTour(25-28)clearActiveTour(25-28)src/editor/components/guided-modal-tour/tour-conditions.js (2)
TOUR_CONDITIONS(2-2)TOUR_CONDITIONS(2-2)
src/editor/components/modal-tour/index.js (3)
src/editor/components/guided-modal-tour/index.js (2)
ModalTour(73-73)props(28-30)src/editor/components/modal-tour/tour-steps.js (2)
TOUR_STEPS(4-6)TOUR_STEPS(4-6)src/editor/components/guided-modal-tour/util.js (6)
setActiveTour(17-20)setActiveTour(17-20)getActiveTourId(42-42)getActiveTourId(42-42)clearActiveTour(25-28)clearActiveTour(25-28)
🔇 Additional comments (11)
src/editor/components/guided-modal-tour/index.js (4)
1-24: LGTM!The imports are well-organized and include all necessary dependencies for tour state management, conditions, and React hooks.
27-56: LGTM!The state management and tour conflict prevention logic is well-implemented. The listener cleanup is properly handled in the effect's return function.
58-70: LGTM!The condition logic correctly handles the three-state system (true/false/null) and appropriately gates tour visibility based on completion state.
79-102: LGTM!The
onClosehandler correctly manages tour completion: clears active tour state, persists to WordPress settings with error handling, updates the global cache, and cleans up URL parameters. Previous review concerns about error handling have been addressed.src/editor/components/modal-tour/index.js (7)
1-46: LGTM!The imports, constant definitions, and initial component setup are well-structured. State management and ref usage are appropriate for the tour functionality.
48-78: LGTM!The tour configuration and step access are properly guarded with fallbacks (
|| {}), preventing crashes whentourIdis invalid or steps are empty. Previous review concerns have been addressed.
80-111: LGTM!The initialization and active tour management effects are well-implemented with proper cleanup. The force refresh interval ensures modal positioning stays accurate during visibility.
113-192: LGTM!Both navigation handlers (
handleNextEventandhandleBackEvent) properly manage transitions, visibility states, and step lifecycle hooks with appropriate optional chaining. The callback dependencies are correctly specified.
205-272: LGTM!The initial visibility delay, event listener setup (with proper delegation for click events), and glow element lifecycle are all correctly implemented with appropriate cleanup functions.
340-365: LGTM!The glow target positioning logic correctly calculates size classes and updates the glow element's position dynamically. Proper guards ensure safe DOM access.
382-522: LGTM!The render logic, confetti animation, and Steps indicator component are all well-implemented. The dynamic styling via inline
<style>and conditional rendering are appropriate for this use case.
| // Only lazy-load ModalTour when we're actually going to render it | ||
| const ModalTour = lazy( () => import( /* webpackChunkName: "modal-tour" */ '../modal-tour' ) ) |
There was a problem hiding this comment.
Move lazy() call to module scope.
The lazy() call is currently inside the component's render path, which means it's re-invoked on every render. React's lazy() must be called once at the module level to properly enable code-splitting and avoid remounting issues.
Apply this diff:
+// Only lazy-load ModalTour when we're actually going to render it
+const ModalTour = lazy( () => import( /* webpackChunkName: "modal-tour" */ '../modal-tour' ) )
+
// The main tour component.
const GuidedModalTour = memo( props => {
const {
@@ -69,9 +72,6 @@
}
}
- // Only lazy-load ModalTour when we're actually going to render it
- const ModalTour = lazy( () => import( /* webpackChunkName: "modal-tour" */ '../modal-tour' ) )
-
return (
<Suspense fallback={ null }>
<ModalTour📝 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.
| // Only lazy-load ModalTour when we're actually going to render it | |
| const ModalTour = lazy( () => import( /* webpackChunkName: "modal-tour" */ '../modal-tour' ) ) | |
| // Only lazy-load ModalTour when we're actually going to render it | |
| const ModalTour = lazy( () => import( /* webpackChunkName: "modal-tour" */ '../modal-tour' ) ) | |
| // The main tour component. | |
| const GuidedModalTour = memo( props => { | |
| const { | |
| tourId, | |
| isDone, | |
| setIsDone, | |
| } = props | |
| const markTourAsComplete = useCallback( async () => { | |
| setIsDone( true ) | |
| const guidedTourStates = window.interactEvents?.guidedTourStates || [] | |
| if ( guidedTourStates.includes( tourId ) ) { | |
| return | |
| } | |
| // Update the interact_guided_tour_states setting | |
| if ( ! guidedTourStates.includes( tourId ) ) { | |
| // eslint-disable-next-line camelcase | |
| const settings = new models.Settings( { interact_guided_tour_states: [ ...guidedTourStates, tourId ] } ) | |
| settings.save().catch( error => { | |
| console.error( 'Failed to save tour state:', error ) | |
| // Optionally: notify the user or retry | |
| } ) | |
| } | |
| // Soft update the global variable to prevent the tour from being shown again. | |
| // Note: This mutates the global cache to prevent re-renders in other components | |
| guidedTourStates.push( tourId ) | |
| }, [ tourId, setIsDone ] ) | |
| return ( | |
| <Suspense fallback={ null }> | |
| <ModalTour | |
| onClose={ markTourAsComplete } | |
| /> | |
| </Suspense> | |
| ) | |
| } ) |
🤖 Prompt for AI Agents
In src/editor/components/guided-modal-tour/index.js around lines 72-73, the
lazy() call for ModalTour is inside the component render path causing it to be
re-invoked on every render; move the const ModalTour = lazy(() => import(/*
webpackChunkName: "modal-tour" */ '../modal-tour')) to module scope (top of the
file) so it runs once at load time, remove the in-render declaration and
reference the top-level ModalTour variable instead; also ensure React.lazy (or
lazy) is imported at the top of the file if not already.
…al tour to store finished tour
Summary by CodeRabbit
Release Notes
New Features
Documentation
Style