Merge pull request #33141 from overleaf/mj-tutorials-show-one

[web] Ensure only one tutorial shows at once

GitOrigin-RevId: 797c677a3d45635451485d79ed1c0705819ed5ad

services/web/frontend/js/shared/context/tutorial-context.tsx

@@ -1,12 +1,12 @@
 import getMeta from '@/utils/meta'
 import {
   createContext,
-  Dispatch,
   FC,
-  SetStateAction,
+  MutableRefObject,
   useCallback,
   useContext,
   useMemo,
+  useRef,
   useState,
 } from 'react'
 
@@ -15,7 +15,8 @@ export const TutorialContext = createContext<
       deactivateTutorial: (tutorial: string) => void
       inactiveTutorials: string[]
       currentPopup: string | null
-      setCurrentPopup: Dispatch<SetStateAction<string | null>>
+      currentPopupRef: MutableRefObject<string | null>
+      setCurrentPopup: (value: string | null) => void
     }
   | undefined
 >(undefined)
@@ -25,7 +26,13 @@ export const TutorialProvider: FC<React.PropsWithChildren> = ({ children }) => {
     () => getMeta('ol-inactiveTutorials') || []
   )
 
-  const [currentPopup, setCurrentPopup] = useState<string | null>(null)
+  const [currentPopup, setCurrentPopupState] = useState<string | null>(null)
+  const currentPopupRef = useRef<string | null>(null)
+
+  const setCurrentPopup = useCallback((value: string | null) => {
+    currentPopupRef.current = value
+    setCurrentPopupState(value)
+  }, [])
 
   const deactivateTutorial = useCallback(
     (tutorialKey: string) => {
@@ -39,6 +46,7 @@ export const TutorialProvider: FC<React.PropsWithChildren> = ({ children }) => {
       deactivateTutorial,
       inactiveTutorials,
       currentPopup,
+      currentPopupRef,
       setCurrentPopup,
     }),
     [deactivateTutorial, inactiveTutorials, currentPopup, setCurrentPopup]

services/web/frontend/js/shared/hooks/promotions/use-tutorial.tsx

@@ -24,15 +24,28 @@ const useTutorial = (
   const {
     deactivateTutorial,
     currentPopup,
+    currentPopupRef,
     setCurrentPopup,
     inactiveTutorials,
   } = useTutorialContext()
 
   const checkCompletion = useCallback(
     () => inactiveTutorials.includes(tutorialKey),
-    [inactiveTutorials, tutorialKey]
+    // currentPopup is a dependency so consumers re-run their effect when the
+    // popup slot frees up and can retry tryShowingPopup.
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+    [inactiveTutorials, tutorialKey, currentPopup]
   )
 
+  const clearPopup = useCallback(() => {
+    // popups should only clear themselves, in cases they need to cleanup or shouldnt show anymore
+    // allow forcing the clear if needed, eg: higher prio alert needs to show
+    if (currentPopupRef.current === tutorialKey) {
+      setCurrentPopup(null)
+      setShowPopup(false)
+    }
+  }, [currentPopupRef, setCurrentPopup, setShowPopup, tutorialKey])
+
   const completeTutorial = useCallback(
     async (
       {
@@ -52,10 +65,10 @@ const useTutorial = (
         }
         debugConsole.error(err)
       }
-      setShowPopup(false)
       deactivateTutorial(tutorialKey)
+      clearPopup()
     },
-    [deactivateTutorial, eventData, tutorialKey]
+    [deactivateTutorial, eventData, tutorialKey, clearPopup]
   )
 
   const dismissTutorial = useCallback(
@@ -79,26 +92,19 @@ const useTutorial = (
   // try to show the popup if we don't already have one showing, returns true if it can show, false if it can't
   const tryShowingPopup = useCallback(
     (eventName: string = 'promo-prompt') => {
-      if (currentPopup === null) {
-        setCurrentPopup(tutorialKey)
-        setShowPopup(true)
-        eventTracking.sendMB(eventName, eventData)
-        return true
+      // Check the lock via ref so concurrent callers in the same render see
+      // each other's claim; useState would batch and both would read stale null.
+      if (currentPopupRef.current !== null) {
+        return false
       }
-      return false
+      setCurrentPopup(tutorialKey)
+      setShowPopup(true)
+      eventTracking.sendMB(eventName, eventData)
+      return true
     },
-    [currentPopup, setCurrentPopup, tutorialKey, eventData]
+    [currentPopupRef, setCurrentPopup, tutorialKey, eventData]
   )
 
-  const clearPopup = useCallback(() => {
-    // popups should only clear themselves, in cases they need to cleanup or shouldnt show anymore
-    // allow forcing the clear if needed, eg: higher prio alert needs to show
-    if (currentPopup === tutorialKey) {
-      setCurrentPopup(null)
-      setShowPopup(false)
-    }
-  }, [setCurrentPopup, setShowPopup, currentPopup, tutorialKey])
-
   const clearAndShow = useCallback(
     (eventName: string = 'promo-prompt') => {
       setCurrentPopup(tutorialKey)

services/web/test/frontend/helpers/make-tutorial-provider.tsx

@@ -2,6 +2,7 @@ import React, {
   type FC,
   type PropsWithChildren,
   useCallback,
+  useRef,
   useState,
 } from 'react'
 import { TutorialContext } from '@/shared/context/tutorial-context'
@@ -13,6 +14,7 @@ export const makeTutorialProvider = (opts?: {
     const [inactiveTutorials, setInactiveTutorials] = useState<string[]>(
       opts?.inactiveTutorials ?? []
     )
+    const currentPopupRef = useRef<string | null>(null)
     const deactivateTutorial = useCallback((key: string) => {
       setInactiveTutorials(prev => (prev.includes(key) ? prev : [...prev, key]))
     }, [])
@@ -20,6 +22,7 @@ export const makeTutorialProvider = (opts?: {
       deactivateTutorial,
       inactiveTutorials,
       currentPopup: null,
+      currentPopupRef,
       setCurrentPopup: () => {},
     }
     return (

services/web/test/frontend/shared/hooks/use-tutorial.spec.tsx

@@ -145,9 +145,16 @@ describe('useTutorial', function () {
   })
 
   describe('for two tutorials at the same time', function () {
-    // FIXME: This should work, but doesn't.
-    // eslint-disable-next-line mocha/no-skipped-tests
-    it.skip('only shows one popup at a time', function () {
+    beforeEach(function () {
+      cy.intercept('POST', '/tutorial/test-tutorial-1/complete', {
+        statusCode: 200,
+      }).as('completeTutorial1')
+      cy.intercept('POST', '/tutorial/test-tutorial-2/complete', {
+        statusCode: 200,
+      }).as('completeTutorial2')
+    })
+
+    it('only shows one popup at a time', function () {
       cy.mount(
         <EditorProviders>
           <TutorialTester tutorial="test-tutorial-1" />
@@ -157,5 +164,23 @@ describe('useTutorial', function () {
 
       cy.findAllByText(/active/).should('have.length', 1)
     })
+
+    it('shows the second popup after the first is completed', function () {
+      cy.mount(
+        <EditorProviders>
+          <TutorialTester tutorial="test-tutorial-1" />
+          <TutorialTester tutorial="test-tutorial-2" />
+        </EditorProviders>
+      )
+
+      cy.findByText('test-tutorial-1 active').should('be.visible')
+      cy.findByText('test-tutorial-2 active').should('not.exist')
+
+      cy.findByRole('button', { name: 'Complete' }).click()
+      cy.wait('@completeTutorial1')
+
+      cy.findByText('test-tutorial-1 active').should('not.exist')
+      cy.findByText('test-tutorial-2 active').should('be.visible')
+    })
   })
 })