React ContextualSaveBar does not hide on component unmount

[mbaumbach]

Describe the bug

The ContextualSaveBar component in the @shopify/app-bridge-react library does not hide when it's unmounted. This can come up if you have a navigation event occur (e.g. React Router) and don't change the visible prop on the ContextualSaveBar to false. Ideally, I think a useEffect should exist on the ContextualSaveBar that will hide it if the component is unmounted.

To Reproduce

Steps to reproduce the behaviour:

1. Create a ContextualSaveBar like this:

const [mounted, setMounted] = useState(true);
{mounted && (
    <ContextualSaveBar
        saveAction={{ onAction: () => alert('Save') }}
        discardAction={{ onAction: () => setMounted(false) }}
        visible
    />
)}

2. Click on the Discard button.
3. The ContextualSaveBar will no longer be mounted, but still be visible.

While this is a contrived example and obviously you would use the visible prop, it will help exemplify the problem if you have a navigation event occur that doesn't set visible to false before occurring. I noticed this when disabling the leave confirmation modal and clicked on the app name which tried to navigate back to the root of the embedded app. This left the contextual save bar visible/stuck, yet the component was no longer mounted.

Expected behaviour

Any time a ContextualSaveBar component is unmounted, it should also hide it if it's visible. Perhaps this requires an effect like this in the component:

useEffect(() => {
    return () => contextualSaveBar.dispatch(Action.HIDE);
}, [contextualSaveBar]);

The only trickiness here is that contextualSaveBar is assigned via useMemo so there's no guarantee it won't get recomputed by React. With the current useEffects in place, I think in the unlikely event that the memoization re-runs, you'd see a flicker of the contextual save bar.

So there may need to be some useRef magic to make sure a true unmount effect like this is possible:

useEffect(() => {
    // The contextualSaveBarRef.current could be assigned in the useMemo perhaps
    return () => contextualSaveBarRef.current?.dispatch(Action.HIDE);
}, []);

There may also be cleaner solutions, but that's what I came up with via a cursory look at the code. :)

Contextual information

Packages and versions

List the relevant packages you’re using, and their versions. For example:

• @shopify/app-bridge @ 3.1.1
• @shopify/app-bridge-react @ 3.1.1
• @shopify/polaris @ 9.19.0

Platform

• OS: Any
• OS Version: Any
• App: Chrome

[henrytao-me]

Thanks for reporting this @mbaumbach. I pushed a fix for this and it should be available in the next client release 🙇