Use react lifecycle to manage scroll

This fixes a issue where scroll restoration was not working as it was called before
react renders the restored page. In this change we move the restoration to
`useLayoutEffect`.

In reviewing the fix, we noticed that `setActivePage` was not being dispatched
in `navigateTo`, and relied on `onHistoryChange` to make the correct changes.
We refactor to move the explicit nav logic from `onHistoryChange` and back to
`navigateTo`.

Author: jho406

superglue/lib/components/Navigation.tsx

@@ -1,12 +1,13 @@
 import React, {
   createContext,
   useEffect,
+  useLayoutEffect,
   forwardRef,
   useImperativeHandle,
   ForwardedRef,
 } from 'react'
 import { urlToPageKey, pathWithoutBZParams } from '../utils'
-import { removePage, historyChange, setActivePage } from '../actions'
+import { removePage, setActivePage } from '../actions'
 import {
   HistoryState,
   RootState,
@@ -15,7 +16,6 @@ import {
   NavigationProviderProps,
   AllPages,
   SuperglueState,
-  PageKey,
 } from '../types'
 import { Update } from 'history'
 import { useDispatch, useSelector, useStore } from 'react-redux'
@@ -53,12 +53,23 @@ const NavigationProvider = forwardRef(function NavigationProvider(
   const superglue = useSelector<RootState, SuperglueState>(
     (state) => state.superglue
   )
+  const currentPageKey = useSelector<RootState, string>(
+    (state) => state.superglue.currentPageKey
+  )
   const store = useStore()
 
   useEffect(() => {
     return history.listen(onHistoryChange)
   }, [])
 
+  useLayoutEffect(() => {
+    const state = history.location.state as HistoryState
+    if (state && 'superglue' in state) {
+      const { posX, posY } = state
+      setWindowScroll(posX, posY)
+    }
+  }, [currentPageKey])
+
   useImperativeHandle(
     ref,
     () => {
@@ -69,36 +80,14 @@ const NavigationProvider = forwardRef(function NavigationProvider(
     []
   )
 
-  const visitAndRestore = (pageKey: PageKey, posX: number, posY: number) => {
-    // When the application visit gets called with revisit: true
-    // -  In cases where the response was not redirected, the calculated
-    //    navigationAction is set to 'none' (meaning `navigateTo` immediately returned `false`)
-    //    and so we have restore scroll and the set the active page
-    // -  In cases where the response was redirected, the calculated
-    //    navigationAction is set to 'replace', and is handled gracefully by navigateTo,
-    //    before this method gets called.
-    // That's why we're only concerned with the first case, but we gracefully warn
-    // if the application visit did not return the meta object like the dev was supposed to.
-    return visit(pageKey, { revisit: true }).then((meta) => {
-      if (meta) {
-        if (meta.navigationAction === 'none') {
-          dispatch(setActivePage({ pageKey }))
-          setWindowScroll(posX, posY)
-        }
-      } else {
-        console.warn(
-          `scoll restoration was skipped. Your visit's then funtion
-          should return the meta object it recieved if you want your
-          application to restore the page's previous scroll.`
-        )
-      }
-    })
-  }
-
   const onHistoryChange = ({ location, action }: Update): void => {
     const state = location.state as HistoryState
 
-    if (!state && location.hash !== '' && action === 'POP') {
+    if (action !== 'POP') {
+      return
+    }
+
+    if (!state && location.hash !== '') {
       const nextPageKey = urlToPageKey(location.pathname + location.search)
       const containsKey = !!pages[nextPageKey]
       if (containsKey) {
@@ -119,17 +108,8 @@ const NavigationProvider = forwardRef(function NavigationProvider(
     }
 
     if (state && 'superglue' in state) {
-      dispatch(
-        historyChange({
-          pageKey: state.pageKey,
-        })
-      )
-
-      if (action !== 'POP') {
-        return
-      }
-
-      const { pageKey, posX, posY } = state
+      const { pageKey } = state
+      const prevPageKey = store.getState().superglue.currentPageKey
       const containsKey = !!pages[pageKey]
 
       if (containsKey) {
@@ -138,19 +118,38 @@ const NavigationProvider = forwardRef(function NavigationProvider(
         switch (restoreStrategy) {
           case 'fromCacheOnly':
             dispatch(setActivePage({ pageKey }))
-            setWindowScroll(posX, posY)
             break
           case 'fromCacheAndRevisitInBackground':
             dispatch(setActivePage({ pageKey }))
-            setWindowScroll(posX, posY)
             visit(pageKey, { revisit: true })
             break
           case 'revisitOnly':
           default:
-            visitAndRestore(pageKey, posX, posY)
+            visit(pageKey, { revisit: true }).then(() => {
+              const noNav =
+                prevPageKey === store.getState().superglue.currentPageKey
+              if (noNav) {
+                // When "POP'ed", revisiting (using revisit: true) a page can result in
+                // a redirect, or a render of the same page.
+                //
+                // When its a redirect, calculateNavAction  will correctly set the
+                // navigationAction to `replace` this is the noop scenario.
+                //
+                // When its the same page, navigationAction is set to `none` and
+                // no navigation took place. In that case, we have to set the
+                // activePage otherwise the user is stuck on the original page.
+                dispatch(setActivePage({ pageKey }))
+              }
+            })
         }
       } else {
-        visitAndRestore(pageKey, posX, posY)
+        visit(pageKey, { revisit: true }).then(() => {
+          const noNav =
+            prevPageKey === store.getState().superglue.currentPageKey
+          if (noNav) {
+            dispatch(setActivePage({ pageKey }))
+          }
+        })
       }
     }
   }
@@ -175,7 +174,6 @@ const NavigationProvider = forwardRef(function NavigationProvider(
     if (hasPage) {
       const location = history.location
       const state = location.state as HistoryState
-      const prevPageKey = state.pageKey
       const historyArgs = [
         path,
         {
@@ -196,26 +194,24 @@ const NavigationProvider = forwardRef(function NavigationProvider(
             },
             {
               ...state,
-              posY: window.pageYOffset,
-              posX: window.pageXOffset,
+              posY: window.scrollY,
+              posX: window.scrollX,
             }
           )
         }
 
         history.push(...historyArgs)
+        dispatch(setActivePage({ pageKey: nextPageKey }))
       }
 
       if (action === 'replace') {
         history.replace(...historyArgs)
-      }
 
-      setActivePage({ pageKey: nextPageKey })
-      setWindowScroll(0, 0)
-
-      if (action === 'replace' && prevPageKey && prevPageKey !== nextPageKey) {
-        dispatch(removePage({ pageKey: prevPageKey }))
+        if (currentPageKey !== nextPageKey) {
+          dispatch(setActivePage({ pageKey: nextPageKey }))
+          dispatch(removePage({ pageKey: currentPageKey }))
+        }
       }
-
       return true
     } else {
       console.warn(
@@ -228,7 +224,7 @@ const NavigationProvider = forwardRef(function NavigationProvider(
     }
   }
 
-  const { currentPageKey, search } = superglue
+  const { search } = superglue
   const { componentIdentifier } = pages[currentPageKey]
   const Component = mapping[componentIdentifier]
 

superglue/spec/lib/NavComponent.spec.jsx

@@ -8,6 +8,7 @@ import userEvent from '@testing-library/user-event'
 import { NavigationContext } from '../../lib/components/Navigation'
 import { configureStore } from '@reduxjs/toolkit'
 import { rootReducer } from '../../lib'
+import { setActivePage } from '../../lib/actions'
 
 const buildStore = (preloadedState) => {
   let resultsReducer = (state = [], action) => {
@@ -175,13 +176,7 @@ describe('Nav', () => {
 
       const expectedActions = [
         {
-          type: '@@superglue/HISTORY_CHANGE',
-          payload: {
-            pageKey: '/home',
-          },
-        },
-        {
-          type: '@@superglue/HISTORY_CHANGE',
+          type: '@@superglue/SET_ACTIVE_PAGE',
           payload: {
             pageKey: '/about',
           },
@@ -362,7 +357,7 @@ describe('Nav', () => {
 
   describe('history pop', () => {
     describe('when the previous page was set to "revisitOnly"', () => {
-      it('revisits the page and scrolls when finished', () => {
+      it('revisits the page and scrolls when finished', async () => {
         const history = createMemoryHistory({})
         history.replace('/home', {
           superglue: true,
@@ -402,9 +397,7 @@ describe('Nav', () => {
         const fakeVisit = vi.fn((...args) => {
           return {
             then: vi.fn((fn) => {
-              expect(scrollTo).not.toHaveBeenCalled()
               fn({ navigationAction })
-              expect(scrollTo).toHaveBeenCalledWith(5, 5)
             }),
           }
         })
@@ -421,11 +414,19 @@ describe('Nav', () => {
           </Provider>
         )
 
+        expect(scrollTo).toHaveBeenCalledWith(10, 10)
         history.back()
+        expect(scrollTo).not.toHaveBeenCalledWith(5, 5)
         expect(fakeVisit).toHaveBeenCalledWith('/home', { revisit: true })
+
+        await expect.poll(() => scrollTo.toHaveBeenCalledWith(5, 5))
       })
 
-      it('revisits the page and skips scroll when redirected (navigationAction is not "none")', () => {
+      it('revisits the page and when redirected replaces with the new page', async () => {
+        const Login = () => {
+          return <h1>Login Page</h1>
+        }
+
         const history = createMemoryHistory({})
         history.replace('/home', {
           superglue: true,
@@ -451,6 +452,10 @@ describe('Nav', () => {
               componentIdentifier: 'about',
               restoreStrategy: 'revisitOnly',
             },
+            '/login': {
+              componentIdentifier: 'login',
+              restoreStrategy: 'fromCacheOnly',
+            },
           },
           superglue: {
             csrfToken: 'abc',
@@ -460,39 +465,45 @@ describe('Nav', () => {
         const scrollTo = vi
           .spyOn(window, 'scrollTo')
           .mockImplementation(() => {})
-        const navigationAction = 'push'
+        const navigationAction = 'replace'
 
         const fakeVisit = vi.fn((...args) => {
           return {
             then: vi.fn((fn) => {
-              // expect(scrollTo).not.toHaveBeenCalled()
+              store.dispatch(setActivePage({ pageKey: '/login' }))
               fn({ navigationAction })
-              expect(scrollTo).not.toHaveBeenCalled()
             }),
           }
         })
 
-        // scroll to 0 0
-
         render(
           <Provider store={store}>
             <NavigationProvider
               store={store}
               visit={fakeVisit}
-              mapping={{ home: Home, about: About }}
+              mapping={{ home: Home, about: About, login: Login }}
               initialPageKey={'/home'}
               history={history}
             />
           </Provider>
         )
 
+        expect(screen.getByRole('heading')).toHaveTextContent('About Page')
+        expect(screen.getByRole('heading')).not.toHaveTextContent('Login Page')
+
         history.back()
-        expect(fakeVisit).toHaveBeenCalledWith('/home', { revisit: true })
+
+        await expect.poll(() =>
+          screen.getByRole('heading').not.toHaveTextContent('About Page')
+        )
+        await expect.poll(() =>
+          screen.getByRole('heading').toHaveTextContent('Login Page')
+        )
       })
     })
 
     describe('when the previous page was set to "fromCacheOnly"', () => {
-      it('restores without visiting and scrolls', () => {
+      it('restores without visiting and scrolls', async () => {
         const history = createMemoryHistory({})
         history.replace('/home', {
           superglue: true,
@@ -541,13 +552,14 @@ describe('Nav', () => {
         )
 
         history.back()
-        expect(scrollTo).toHaveBeenCalledWith(5, 5)
         expect(fakeVisit).not.toHaveBeenCalled()
+        expect(scrollTo).not.toHaveBeenCalledWith(5, 5)
+        await expect.poll(() => scrollTo.toHaveBeenCalledWith(5, 5))
       })
     })
 
     describe('and the previous page was set to "fromCacheAndRevisitInBackground"', () => {
-      it('restores, scrolls, and revisits the page in the background', () => {
+      it('restores, scrolls, and revisits the page in the background', async () => {
         const history = createMemoryHistory({})
         history.replace('/home', {
           superglue: true,
@@ -582,9 +594,7 @@ describe('Nav', () => {
           .spyOn(window, 'scrollTo')
           .mockImplementation(() => {})
 
-        const fakeVisit = vi.fn((...args) => {
-          expect(scrollTo).toHaveBeenCalledWith(5, 5)
-        })
+        const fakeVisit = vi.fn((...args) => {})
 
         render(
           <Provider store={store}>
@@ -600,6 +610,8 @@ describe('Nav', () => {
 
         history.back()
         expect(fakeVisit).toHaveBeenCalledWith('/home', { revisit: true })
+        expect(scrollTo).not.toHaveBeenCalledWith(5, 5)
+        await expect.poll(() => scrollTo.toHaveBeenCalledWith(5, 5))
       })
     })
   })