Dashboard Sharing Feature Tour is buggy — "Crashes" if first step is accessed after going to the end of the feature tour

[tofumatt]

trafficstars

Bug Description

Originally posted by @aaemnnosttv in https://github.com/google/site-kit-wp/issues/6478#issuecomment-1460935747

The Dashboard Sharing feature tour opens the Dashboard Settings modal when the second stage of the tour is accessed, eg.:

(But you can notice that going "Back" then "Next" again the modal does not re-appear.

There's also a no-op from operating on an unmounted component when going from the main dashboard (first step) to the modal (second step):

Steps to reproduce

1. Set up the Site Kit without Dashboard Sharing enabled in the tester plugin
2. Once set up, go to the tester plugin and enable Dashboard sharing
3. Go to the dashboard and you’ll see the Dashboard Sharing Feature Tour
4. Advance from the first step to the second to observer the console error
5. Go "Back" from the second step to the first
6. Click "Next" from the first step and notice the tour closes/crashes instead of opening the modal again

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation Brief

Test Coverage

QA Brief

Changelog entry

[mxbclang]

@aaemnnosttv Is this a P0? It's been AC for three months so feels like it should be a P1 or P2?

[ivonac4]

@tofumatt Are you still planning to work on this soon? If not, could you please unassign yourself so someone else can pick it up? Thank you!

[eugene-manuilov]

AC ✔

[zutigrm]

It seems as there is an internal conflict between our steps change callback and internal navigation of the Joyride, first time tour is switched to the next step (2nd step) the unmounted target notice is shown

Details

Target not mounted {event: 'click', placement: 'auto', offset: 10, disableBeacon: true, disableCloseOnEsc: false, …} content : "Grant access to the view-only dashboard for each service for the specific roles you want. Users will see the Site Kit dashboard with only the services that have been shared with them without needing to sign-in with Google." disableBeacon : true disableCloseOnEsc : false disableOverlay : false disableOverlayClose : true disableScrollParentFix : false disableScrolling : true event : "click" floaterProps : {options: {…}, wrapperOptions: {…}, disableAnimation: true, styles: {…}, offset: 20} hideBackButton : false hideCloseButton : false hideFooter : false isFixed : true locale : {back: 'Back', close: 'Close', last: 'Got it', next: 'Next', nextLabelWithProgress: 'Next (Step {step} of {steps})', …} offset : 10 placement : "auto" showProgress : true showSkipButton : false spotlightClicks : false spotlightPadding : 10 styles : {beacon: {…}, beaconInner: {…}, beaconOuter: {…}, tooltip: {…}, tooltipContainer: {…}, …} target : ".googlesitekit-dashboard-sharing-settings__main .googlesitekit-dashboard-sharing-settings__column--view" title : "Manage view access for other roles" tooltipComponent : ƒ TourTooltip(_ref) [[Prototype]] : Object

Since we have separate callback that handles opening of the dashboard sharing modal https://github.com/google/site-kit-wp/blob/f31575ce354ebf382e6f075848d7b7392b6e9c04/assets/js/feature-tours/dashboard-sharing.js#L53, I tried moving it before the step navigation happens, and also delaying the steps navigation to ensure target element is mounted https://github.com/google/site-kit-wp/blob/f31575ce354ebf382e6f075848d7b7392b6e9c04/assets/js/components/TourTooltips.js#L221-L229

But even in that case, and ensuring target is accessible for shouldChangeStep:

let el = step.target;
if ( 'string' === typeof step.target ) {
	el = global.document.querySelector( step.target );
	if ( el ) {
		changeStep( index, action );
	}
} else {
	changeStep( index, action );
}

There seems to be internal step navigation/or internal issue in general accessing the target that is in the modal. Second time it happens, going to the prev step and then next step - when the modal flashes and get's closed is when error is thrown:

Details

react-dom.development.js:88 Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method. in ReactFloater (created by JoyrideStep) in div (created by JoyrideStep) in JoyrideStep (created by Joyride)

The error is originating from within the Joyride, on unmount lifecycle. Couldn't inspect what is happening on deeper level, due to the logic being handled in 3rd party component, but the "catch" in the approach is that we need to handle the modal opening before the target for the next step is accessed, which seems to be handled internally in the component even we use our step callback to delay the steps switching until modal is opened 🤔

Haven't found any feasible change/workaround for this issue so far, as the source of error is originating from the third party component

[benbowler]

I spent some time digging into the Joyride code and how our callbacks are interacting with this. As @zutigrm discusses the error is within the Joyride component. I found that it triggers a tour:end event after hitting the next button from the first step the second time, this is because Joyride is still handling it's own step navigation, which it shouldn't be because we are managing the step state within TourTooltips component.

Debugging this behaviour I saw that internally Joyride was not in the "controlled" state which signifies that the step state is handled outside of the plugin. Further, I found that Joyride sets the controlled state once at mount of the component and sets it based on the value of stepIndex on mount which is unclear from the documentation. Because we are getting the value of stepIndex from the store it is undefined which leads to Joyride being in uncontrolled state meaning that it was internally handling the steps and next and back behaviour while still being passed indexes leading to unexpected results such as this bug and the Target not mounted warning even though the target components were in the DOM.

This means there is quite a simple fix in the IB.

I've added some extra time to the estimate for QA.

The other use of Joyride ( for the Joyride tooltip ) only has one step so this bug does not occur.

[eugene-manuilov]

Thanks, @benbowler, but we no longer need that feature tour. I created another task to delete all feature tours that we have in our codebase: #10574. Closing this task.

[benbowler]

Hey @eugene-manuilov as we're keeping TourTooltip around for future tours I suggest we reopen this ticket and complete this fix, otherwise when we add new tours in future this bug we resurface.