feat: onboarding tour with name changes#3
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a guided modal tour system (components, modal runtime, tours, styles), PHP persistence/localization for tour state, integrates GuidedModalTour into editor surfaces (editor root, sidebar, interaction library), defers configure-modal apply work into an async callback, and applies small UI classname tweaks. ChangesGuided Modal Tour
Sequence Diagram(s)sequenceDiagram
participant Editor as Editor UI
participant GMT as GuidedModalTour
participant Util as Tour State Manager
participant MT as ModalTour
participant PHP as Interact_Getting_Started_Screen
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->>PHP: read localized 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, nextEvent
MT->>MT: update step, reposition, show glow
end
MT->>GMT: onClose
GMT->>Util: clearActiveTour()
GMT->>PHP: 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 | ✅ 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 |
![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.
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.
…al tour to store finished tour
🤖 Pull request artifacts
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/editor/interaction-library/configure-modal.js (1)
236-245: ⚡ Quick winConsider adding a loading state to the apply button during async processing.
Since
handleApplynow defers all work insidesetTimeout, the button click returns immediately while the actual insertion/application happens asynchronously. Without a loading indicator or disabled state, users receive no visual feedback that work is in progress and might click the button multiple times.💡 Suggested improvement
Add a
isApplyingstate and disable the button during processing:const [ optionValues, setOptionValues ] = useState( {} ) const [ selectedTarget, setSelectedTarget ] = useState( interactionTarget ) const [ sideBarEl, setSideBarEl ] = useState( null ) + const [ isApplying, setIsApplying ] = useState( false )const handleApply = () => { + setIsApplying( true ) setTimeout( () => { try { const config = getConfig( selectedPreset.config ) // ... rest of logic ... closeModal() openInteractionsSidebar() } catch ( error ) { console.error( 'Interactions Library apply error:', error ) // eslint-disable-line no-console + setIsApplying( false ) } } ) }<div className="interact-interaction-library__configure__apply-button"> <Button variant="primary" className="interact-interaction-library__configure__apply-button__button" onClick={ handleApply } + isBusy={ isApplying } + disabled={ isApplying } > { mode === 'insert' ? __( 'Insert Interaction', 'interactions' ) : __( 'Apply Interaction', 'interactions' ) } </Button> </div>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/editor/interaction-library/configure-modal.js` around lines 236 - 245, The apply button currently returns immediately because handleApply defers work with setTimeout and provides no feedback; add a local boolean state (e.g., isApplying) in the ConfigureModal component, set isApplying = true at the start of handleApply before scheduling the async work and set isApplying = false in the final callback/cleanup (or in both success/error branches), then pass isApplying to the Button to disable it and optionally change its label or show a spinner (use mode and existing className to keep styling consistent); this prevents duplicate clicks while the deferred insertion/application runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/editor/interaction-library/configure-modal.js`:
- Around line 60-156: The deferred apply logic inside the setTimeout callback
may throw after the small try/catch around config(); wrap the entire setTimeout
callback body in a try/catch that catches any error from parse(),
dispatch(...).insertBlocks, applyTargetMappings, createNewAction,
createNewInteraction, updateInteraction, etc., log the error and surface a
user-visible failure (e.g., dispatch a notice) and abort closing the
modal/opening the sidebar and abort saving the post to avoid leaving the editor
inconsistent; ensure dispatch('interact/interactions').setDidModifyPostContent
remains correct on error and only proceed to closeModal() /
openInteractionsSidebar() / savePost() on successful completion.
---
Nitpick comments:
In `@src/editor/interaction-library/configure-modal.js`:
- Around line 236-245: The apply button currently returns immediately because
handleApply defers work with setTimeout and provides no feedback; add a local
boolean state (e.g., isApplying) in the ConfigureModal component, set isApplying
= true at the start of handleApply before scheduling the async work and set
isApplying = false in the final callback/cleanup (or in both success/error
branches), then pass isApplying to the Button to disable it and optionally
change its label or show a spinner (use mode and existing className to keep
styling consistent); this prevents duplicate clicks while the deferred
insertion/application runs.
🪄 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: 02ca3ba6-5a54-43c2-8a04-9cc3aef58d3d
📒 Files selected for processing (5)
interactions.phpsrc/editor/components/interaction-panel/index.jssrc/editor/components/timeline/index.jssrc/editor/interaction-library/configure-modal.jssrc/editor/interaction-library/index.js
✅ Files skipped from review due to trivial changes (1)
- src/editor/components/timeline/index.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/editor/components/interaction-panel/index.js
- interactions.php
|
@coderabbitai review this PR, update your comments |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/editor/plugins/top-toolbar-button/add-interaction-button.js (1)
26-26:⚠️ Potential issue | 🟠 MajorSelector mismatch issue remains unresolved.
A previous review identified that code in
src/editor/plugins/top-toolbar-button/index.jssearches for.interact-insert-library-button__wrapper, but this selector does not match any element in the codebase. The button usesinteract-insert-library-button(without__wrapper), and the wrapper div usesinteract-add-interaction-button-wrapper. This className change does not address the underlying selector issue.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/editor/plugins/top-toolbar-button/add-interaction-button.js` at line 26, The selector used in src/editor/plugins/top-toolbar-button/index.js ('.interact-insert-library-button__wrapper') doesn't match any DOM class; update the code so the selector targets the actual element class or rename the element to match the selector — specifically either change the selector to '.interact-add-interaction-button-wrapper' (to match the wrapper div) or add/rename the wrapper element to have the class 'interact-insert-library-button__wrapper'; locate references in index.js and the button component that set className 'interact-insert-library-button' and 'interact-add-interaction-button-wrapper' and make them consistent.
🧹 Nitpick comments (1)
src/editor/interaction-library/configure-modal.js (1)
151-153: ⚡ Quick winConsider adding user-facing error notification.
While the error-handling structure is now correct (errors are caught, modal doesn't close, sidebar doesn't open), errors are only logged to the console. Users receive no feedback when the interaction application fails, which creates a poor experience.
Consider dispatching a WordPress notice to inform the user:
} catch ( error ) { console.error( 'Interactions Library configuration error:', error.message ) // eslint-disable-line no-console dispatch( 'core/notices' ).createNotice( 'error', __( 'Failed to apply interaction. Please check the console for details.', 'interactions' ), { type: 'snackbar', isDismissible: true, } ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/editor/interaction-library/configure-modal.js` around lines 151 - 153, The catch block in configure-modal.js currently only logs errors to the console; update it to also dispatch a WordPress admin notice so users receive feedback: keep the existing console.error line, then call dispatch('core/notices').createNotice to create an 'error' notice with a user-friendly translated message (use __ with the 'interactions' text domain), and pass options { type: 'snackbar', isDismissible: true } so it appears as a dismissible snackbar; ensure dispatch and __ are imported/available in the module before using them.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/editor/components/guided-modal-tour/editor.scss`:
- Line 14: Adjust blank lines to satisfy stylelint: add an empty line before the
declaration "left: var(--left);" and before the comments flagged by
scss/double-slash-comment-empty-line-before, and remove the extra empty line
immediately before the "display: none;" declaration so there is no blank line
preceding it; locate these by finding the "left: var(--left);" and "display:
none;" declarations and the nearby // comments in the guided-modal-tour SCSS and
add/remove blank lines accordingly to meet `declaration-empty-line-before` and
`scss/double-slash-comment-empty-line-before` rules.
In `@src/editor/components/modal-tour/index.js`:
- Around line 349-351: The ternary computing glowTargetSize contains an
unreachable check (the second branch rechecks targetRect.width > 300), so update
the middle condition to use a lower width threshold (e.g., targetRect.width >
100) so medium catches wide-but-short elements; locate the glowTargetSize
expression in src/editor/components/modal-tour/index.js and change the middle
conditional from targetRect.width > 300 || targetRect.height > 100 to
targetRect.width > 100 || targetRect.height > 100 (keeping the existing 'large'
and 'small' branches unchanged).
In `@src/editor/getting-started.php`:
- Around line 27-44: The registered setting interact_guided_tour_states is
exposed via show_in_rest which causes Editor/Author users to get 403 when
frontend JS (models.Settings.save in
src/editor/components/guided-modal-tour/index.js) attempts to persist to
/wp/v2/settings; update the registration in getting-started.php so that
show_in_rest and/or registration only happens for users with
current_user_can('manage_options') (or skip exposing it and gate the localized
guidedTourStates with current_user_can), or alternatively switch persistence to
a per-user mechanism (store in user meta via update_user_meta and a
capability-appropriate REST callback or custom route) and adjust
sanitize_array_setting usage accordingly. Ensure references:
interact_guided_tour_states, sanitize_array_setting, and the guided-modal-tour
component are updated to match the chosen approach.
In `@src/editor/interaction-library/configure-modal.js`:
- Around line 86-88: The early return in the setTimeout callback after calling
parse() (when block is falsy) leaves the modal in a broken state; instead of
returning from the callback, throw a descriptive Error so the surrounding
try-catch in the parent function will catch it and run the existing error
handling/cleanup. Locate the setTimeout callback that calls parse() and
currently does "if (!block) { return }" and replace that return with throwing an
Error (e.g., throw new Error('Failed to parse block in configure modal')), so
the try-catch surrounding the operation can close the modal, open the sidebar,
and show the error to the user.
---
Duplicate comments:
In `@src/editor/plugins/top-toolbar-button/add-interaction-button.js`:
- Line 26: The selector used in src/editor/plugins/top-toolbar-button/index.js
('.interact-insert-library-button__wrapper') doesn't match any DOM class; update
the code so the selector targets the actual element class or rename the element
to match the selector — specifically either change the selector to
'.interact-add-interaction-button-wrapper' (to match the wrapper div) or
add/rename the wrapper element to have the class
'interact-insert-library-button__wrapper'; locate references in index.js and the
button component that set className 'interact-insert-library-button' and
'interact-add-interaction-button-wrapper' and make them consistent.
---
Nitpick comments:
In `@src/editor/interaction-library/configure-modal.js`:
- Around line 151-153: The catch block in configure-modal.js currently only logs
errors to the console; update it to also dispatch a WordPress admin notice so
users receive feedback: keep the existing console.error line, then call
dispatch('core/notices').createNotice to create an 'error' notice with a
user-friendly translated message (use __ with the 'interactions' text domain),
and pass options { type: 'snackbar', isDismissible: true } so it appears as a
dismissible snackbar; ensure dispatch and __ are imported/available in the
module before using them.
🪄 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: 9d64d09c-460b-4688-8a69-27aef5267722
📒 Files selected for processing (24)
interactions.phpsrc/editor/components/guided-modal-tour/README.MDsrc/editor/components/guided-modal-tour/editor.scsssrc/editor/components/guided-modal-tour/index.jssrc/editor/components/guided-modal-tour/tour-conditions.jssrc/editor/components/guided-modal-tour/util.jssrc/editor/components/index.jssrc/editor/components/interaction-panel/index.jssrc/editor/components/modal-tour/index.jssrc/editor/components/modal-tour/tour-steps.jssrc/editor/components/modal-tour/tours/README.mdsrc/editor/components/modal-tour/tours/editor.jssrc/editor/components/modal-tour/tours/index.jssrc/editor/components/modal-tour/tours/interaction-library.jssrc/editor/components/modal-tour/tours/sidebar.jssrc/editor/components/timeline/index.jssrc/editor/editor.jssrc/editor/getting-started.phpsrc/editor/interaction-library/configure-modal.jssrc/editor/interaction-library/index.jssrc/editor/interaction-library/select-modal.jssrc/editor/plugins/block-toolbar-button/index.jssrc/editor/plugins/top-toolbar-button/add-interaction-button.jssrc/editor/plugins/top-toolbar-button/index.js
2 participants
Summary by CodeRabbit
New Features
Documentation
UX
Style