Conversation
📝 WalkthroughWalkthroughThe PR refactors the tour initialization and event handling in the JavaScript tours module while adding conditional settings persistence in the PHP tours class. The changes improve code organization through named functions and localized logic without altering external behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
assets/js/tours.js (3)
64-72: Consider hoistingaction_urloutside the step loop.The
action_urlfunction is redefined on every step iteration. Since it doesn't depend on step-specific closures, it can be defined once outside the loop for minor efficiency gains.♻️ Suggested refactor
+ const action_url = function(url, target = '_blank') { + return () => { + window.open(url, target); + }; + }; + _.each(tour, function(step, step_index) { const last_step = (step_index + 1) === tour.length; - const action_url = function(url, target = '_blank') { - - return () => { - - window.open(url, target); - - }; - - }; - step.buttons = _.isArray(step.buttons) ? step.buttons : [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/js/tours.js` around lines 64 - 72, The helper function action_url is being redefined inside the step loop on every iteration; hoist it out of the loop by declaring a single top-level function (e.g., function action_url(url, target = '_blank') { return () => window.open(url, target); }) outside the loop so all steps reuse the same function, keeping the same signature and behavior and ensuring no step-specific closures are referenced.
44-58: Marking tour finished on cancel - verify this is intentional UX.Wiring
markTourFinishedto bothcompleteandcancelevents means users who accidentally dismiss a tour will never see it again. This aligns with the PR title "Ensure tours only show once," but consider if this is the desired UX.Also, the AJAX call lacks error handling. If the request fails, the tour dismisses client-side but isn't persisted, causing the tour to reappear on next load.
♻️ Optional: Add error handling for failed requests
const markTourFinished = function() { $.ajax({ url: ajaxurl, data: { action: 'wu_mark_tour_as_finished', tour_id, nonce: wu_tours_vars.nonce, }, + error: function() { + console.warn('Failed to mark tour as finished:', tour_id); + }, }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/js/tours.js` around lines 44 - 58, The current code binds markTourFinished to both window[tour_id].on('complete') and .on('cancel') which will persist-dismiss tours on cancel; decide whether cancel should not mark finished and if so remove the cancel binding (remove or change window[tour_id].on('cancel', markTourFinished')). Also add robust AJAX callbacks inside markTourFinished (use $.ajax success/error or .done/.fail) targeting action 'wu_mark_tour_as_finished' so failures are handled: on success keep current behavior, on error surface an error (console/log/toast) and restore or reopen the tour or retry the request so a failed persistence call does not incorrectly hide the tour across page loads. Ensure you reference markTourFinished, window[tour_id] and the AJAX action 'wu_mark_tour_as_finished' when making edits.
23-39: Fragile DOM traversal pattern - verify Tippy.js API compatibility.The deep nesting
instance.popperChildren.content.children[0].children[0].childrenis brittle and may break with Shepherd.js or Tippy.js updates. In Tippy.js v6+, thepopperChildrenproperty was removed in favor of accessing elements directly viainstance.popper.Consider using more robust selectors:
const content = instance.popper.querySelector('.shepherd-content'); const header = content?.querySelector('.shepherd-header'); const text = content?.querySelector('.shepherd-text'); const footer = content?.querySelector('.shepherd-footer');Shepherd.js 14.5 popperChildren API Tippy.js🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/js/tours.js` around lines 23 - 39, The onCreate handler uses a fragile deep traversal (instance.popperChildren.content.children[0].children[0].children) that can break with Tippy/Shepherd changes; update onCreate to reference instance.popper and use robust querySelector calls (e.g., find elements by '.shepherd-content', '.shepherd-header', '.shepherd-text', '.shepherd-footer' or other stable class names), add null/exists guards before calling classList.add, and keep the same class additions ('wu-p-2', 'wu-pb-0', etc.) so styling behavior is preserved while avoiding reliance on popperChildren.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@assets/js/tours.js`:
- Around line 64-72: The helper function action_url is being redefined inside
the step loop on every iteration; hoist it out of the loop by declaring a single
top-level function (e.g., function action_url(url, target = '_blank') { return
() => window.open(url, target); }) outside the loop so all steps reuse the same
function, keeping the same signature and behavior and ensuring no step-specific
closures are referenced.
- Around line 44-58: The current code binds markTourFinished to both
window[tour_id].on('complete') and .on('cancel') which will persist-dismiss
tours on cancel; decide whether cancel should not mark finished and if so remove
the cancel binding (remove or change window[tour_id].on('cancel',
markTourFinished')). Also add robust AJAX callbacks inside markTourFinished (use
$.ajax success/error or .done/.fail) targeting action 'wu_mark_tour_as_finished'
so failures are handled: on success keep current behavior, on error surface an
error (console/log/toast) and restore or reopen the tour or retry the request so
a failed persistence call does not incorrectly hide the tour across page loads.
Ensure you reference markTourFinished, window[tour_id] and the AJAX action
'wu_mark_tour_as_finished' when making edits.
- Around line 23-39: The onCreate handler uses a fragile deep traversal
(instance.popperChildren.content.children[0].children[0].children) that can
break with Tippy/Shepherd changes; update onCreate to reference instance.popper
and use robust querySelector calls (e.g., find elements by '.shepherd-content',
'.shepherd-header', '.shepherd-text', '.shepherd-footer' or other stable class
names), add null/exists guards before calling classList.add, and keep the same
class additions ('wu-p-2', 'wu-pb-0', etc.) so styling behavior is preserved
while avoiding reliance on popperChildren.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 49734e21-1003-4112-99f7-6ff264d6ced1
📒 Files selected for processing (2)
assets/js/tours.jsinc/ui/class-tours.php
1 participant
Summary by CodeRabbit