Merge branch 'navidrome:master' into master

ui/src/album/AlbumList.jsx

@@ -1,3 +1,4 @@
+import { cloneElement } from 'react'
 import { useSelector } from 'react-redux'
 import { Redirect, useLocation } from 'react-router-dom'
 import {
@@ -23,6 +24,7 @@ import {
   Title,
   useAlbumsPerPage,
   useResourceRefresh,
+  useScrollRestoration,
   useSetToggleableFields,
 } from '../common'
 import AlbumListActions from './AlbumListActions'
@@ -39,6 +41,13 @@ import ExpandInfoDialog from '../dialogs/ExpandInfoDialog'
 import { humanize } from 'inflection'
 import { makeStyles } from '@material-ui/core/styles'
 
+// Waits for rows: restoring into an unrendered list leaves the page too short to hold the offset.
+const ScrollRestorer = ({ children, ...rest }) => {
+  const { loaded, total } = useListContext()
+  useScrollRestoration(loaded && total > 0)
+  return cloneElement(children, rest)
+}
+
 const useStyles = makeStyles({
   chip: {
     margin: 0,
@@ -255,11 +264,13 @@ const AlbumList = (props) => {
         }
         title={<AlbumListTitle albumListType={albumListType} />}
       >
-        {albumView.grid ? (
-          <AlbumGridView albumListType={albumListType} {...props} />
-        ) : (
-          <AlbumTableView {...props} />
-        )}
+        <ScrollRestorer>
+          {albumView.grid ? (
+            <AlbumGridView albumListType={albumListType} {...props} />
+          ) : (
+            <AlbumTableView {...props} />
+          )}
+        </ScrollRestorer>
       </List>
       <ExpandInfoDialog content={<AlbumInfo />} />
     </>

ui/src/album/AlbumShow.jsx

@@ -10,7 +10,7 @@ import { makeStyles } from '@material-ui/core/styles'
 import AlbumSongs from './AlbumSongs'
 import AlbumDetails from './AlbumDetails'
 import AlbumActions from './AlbumActions'
-import { useResourceRefresh, Title } from '../common'
+import { useResourceRefresh, useScrollRestoration, Title } from '../common'
 
 const useStyles = makeStyles(
   (theme) => ({
@@ -28,6 +28,7 @@ const AlbumShowLayout = (props) => {
   const { record } = context
   const classes = useStyles()
   useResourceRefresh('album', 'song')
+  useScrollRestoration(!!record?.id)
 
   return (
     <>

ui/src/artist/ArtistShow.jsx

@@ -13,7 +13,12 @@ import subsonic from '../subsonic'
 import AlbumGridView from '../album/AlbumGridView'
 import MobileArtistDetails from './MobileArtistDetails'
 import DesktopArtistDetails from './DesktopArtistDetails'
-import { useAlbumsPerPage, useResourceRefresh, Title } from '../common/index.js'
+import {
+  useAlbumsPerPage,
+  useResourceRefresh,
+  useScrollRestoration,
+  Title,
+} from '../common/index.js'
 import ArtistActions from './ArtistActions'
 import { makeStyles } from '@material-ui/core'
 
@@ -85,6 +90,7 @@ const ArtistShowLayout = (props) => {
   const [, perPageOptions] = useAlbumsPerPage(width)
   const classes = useStyles()
   useResourceRefresh('artist', 'album')
+  useScrollRestoration(!!record?.id)
 
   const maxPerPage = 90
   let perPage = 0

ui/src/common/ArtistLinkField.jsx

@@ -7,7 +7,8 @@ import { intersperse } from '../utils/index.js'
 import { useDispatch } from 'react-redux'
 import { closeExtendedInfoDialog } from '../actions/dialogs.js'
 
-const ALink = withWidth()((props) => {
+// noSSR: withWidth otherwise renders null until mounted, so the artist line pops in and grows the row.
+const ALink = withWidth({ noSSR: true })((props) => {
   const { artist, width, ...rest } = props
   const artistLink = useGetHandleArtistClick(width)
   const dispatch = useDispatch()

ui/src/common/index.js

@@ -28,6 +28,7 @@ export * from './useAlbumsPerPage'
 export * from './useGetHandleArtistClick'
 export * from './useInterval'
 export * from './useResourceRefresh'
+export * from './useScrollRestoration'
 export * from './useRefreshOnEvents'
 export * from './useToggleLove'
 export * from './useTraceUpdate'

ui/src/common/useScrollRestoration.jsx

@@ -0,0 +1,64 @@
+import { useEffect, useLayoutEffect, useRef } from 'react'
+import { useHistory, useLocation } from 'react-router-dom'
+
+// Keyed on the route because hash history assigns no location.key, so every page would
+// otherwise share one slot and overwrite the offset we came back for.
+const positions = new Map()
+const MAX_ENTRIES = 50
+const MAX_FRAMES = 60
+
+export const useScrollRestoration = (ready = true) => {
+  const { pathname, search } = useLocation()
+  const history = useHistory()
+  const key = pathname + search
+  const handled = useRef(null)
+  const latest = useRef(0)
+
+  useEffect(() => {
+    latest.current = window.scrollY
+    const track = () => {
+      latest.current = window.scrollY
+    }
+    window.addEventListener('scroll', track, { passive: true })
+    return () => window.removeEventListener('scroll', track)
+  }, [key])
+
+  // Layout cleanup, so the offset is banked while this page is torn down. A passive one runs
+  // after the incoming page tops itself, and this page's listener records that 0 first.
+  useLayoutEffect(
+    () => () => {
+      positions.delete(key)
+      positions.set(key, latest.current)
+      if (positions.size > MAX_ENTRIES) {
+        positions.delete(positions.keys().next().value)
+      }
+    },
+    [key],
+  )
+
+  // Layout effect: a passive one runs after paint, so the list would be painted at the old
+  // offset first and visibly jump.
+  useLayoutEffect(() => {
+    if (!ready || handled.current === key) return
+    handled.current = key
+    const saved = positions.get(key)
+    const top = history.action === 'POP' && saved !== undefined ? saved : 0
+    window.scrollTo({ top })
+    if (!top || Math.round(window.scrollY) === top) return
+
+    // The page can still be filling in, and until it is tall enough the browser clamps us to
+    // the top. Keep asking until the offset sticks, yielding if anything else moves us.
+    let frame
+    let frames = 0
+    let landed = Math.round(window.scrollY)
+    const retry = () => {
+      const y = Math.round(window.scrollY)
+      if (y === top || y !== landed || ++frames > MAX_FRAMES) return
+      window.scrollTo({ top })
+      landed = Math.round(window.scrollY)
+      frame = requestAnimationFrame(retry)
+    }
+    frame = requestAnimationFrame(retry)
+    return () => cancelAnimationFrame(frame)
+  }, [ready, key, history.action])
+}

ui/src/common/useScrollRestoration.test.jsx

@@ -0,0 +1,222 @@
+import { renderHook, act } from '@testing-library/react-hooks'
+import { render } from '@testing-library/react'
+import { useLayoutEffect } from 'react'
+import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'
+
+const mockLocation = { pathname: '/album', search: '' }
+const mockHistory = { action: 'PUSH' }
+vi.mock('react-router-dom', () => ({
+  useLocation: () => mockLocation,
+  useHistory: () => mockHistory,
+}))
+
+import { useScrollRestoration } from './useScrollRestoration'
+
+describe('useScrollRestoration', () => {
+  beforeEach(() => {
+    window.scrollTo = vi.fn()
+    window.scrollY = 0
+    mockLocation.pathname = '/album'
+    mockLocation.search = ''
+    mockHistory.action = 'PUSH'
+  })
+  afterEach(() => {
+    vi.useRealTimers()
+    vi.clearAllMocks()
+  })
+
+  const scrollTo = (y) => {
+    window.scrollY = y
+    act(() => {
+      window.dispatchEvent(new Event('scroll'))
+    })
+  }
+
+  // Returns to a page whose rows have not arrived, so scrollTo still clamps. The returned setter
+  // grows the page to the height it reaches once they do.
+  const returnToUnfilledPage = (saved) => {
+    vi.useFakeTimers()
+    let maxScroll = saved
+    window.scrollTo = vi.fn(({ top }) => {
+      window.scrollY = Math.min(top, maxScroll)
+    })
+
+    const first = renderHook(() => useScrollRestoration())
+    scrollTo(saved)
+    first.unmount()
+
+    maxScroll = 0
+    window.scrollY = 0
+    mockHistory.action = 'POP'
+    renderHook(() => useScrollRestoration())
+    return (height) => (maxScroll = height)
+  }
+
+  const advanceFrames = () => act(() => vi.advanceTimersByTime(100))
+
+  it('starts a pushed route at the top', () => {
+    renderHook(() => useScrollRestoration())
+    expect(window.scrollTo).toHaveBeenCalledWith({ top: 0 })
+  })
+
+  it('restores the saved offset when returning to an entry', () => {
+    const first = renderHook(() => useScrollRestoration())
+    scrollTo(640)
+    first.unmount()
+
+    mockLocation.pathname = '/album/al-1/show'
+    const second = renderHook(() => useScrollRestoration())
+    expect(window.scrollTo).toHaveBeenLastCalledWith({ top: 0 })
+    second.unmount()
+
+    mockLocation.pathname = '/album'
+    mockHistory.action = 'POP'
+    renderHook(() => useScrollRestoration())
+    expect(window.scrollTo).toHaveBeenLastCalledWith({ top: 640 })
+  })
+
+  it('tops a pushed route even if that key was seen before', () => {
+    const first = renderHook(() => useScrollRestoration())
+    scrollTo(500)
+    first.unmount()
+
+    mockHistory.action = 'PUSH'
+    renderHook(() => useScrollRestoration())
+    expect(window.scrollTo).toHaveBeenLastCalledWith({ top: 0 })
+  })
+
+  // Restoring before the rows exist leaves the document too short: the browser clamps to the
+  // top and the offset is lost with no error.
+  it('waits for readiness before touching the scroll position', () => {
+    const { rerender } = renderHook(
+      ({ ready }) => useScrollRestoration(ready),
+      {
+        initialProps: { ready: false },
+      },
+    )
+    expect(window.scrollTo).not.toHaveBeenCalled()
+
+    rerender({ ready: true })
+    expect(window.scrollTo).toHaveBeenCalledWith({ top: 0 })
+  })
+
+  // Hash history assigns no location.key, so an implementation keyed on it would collapse every
+  // route into one slot and the detail page would erase the list's offset before we returned.
+  it('keeps a separate offset per route', () => {
+    const list = renderHook(() => useScrollRestoration())
+    scrollTo(900)
+    list.unmount()
+
+    mockLocation.pathname = '/album/al-1/show'
+    const detail = renderHook(() => useScrollRestoration())
+    scrollTo(0)
+    detail.unmount()
+
+    mockLocation.pathname = '/album'
+    mockHistory.action = 'POP'
+    renderHook(() => useScrollRestoration())
+    expect(window.scrollTo).toHaveBeenLastCalledWith({ top: 900 })
+  })
+
+  // Without the per-instance guard this fires again when readiness flickers, fighting a user who
+  // has already started scrolling. Nothing else in the suite pins it.
+  it('does not scroll again when readiness flickers on the same route', () => {
+    const { rerender } = renderHook(
+      ({ ready }) => useScrollRestoration(ready),
+      {
+        initialProps: { ready: true },
+      },
+    )
+    expect(window.scrollTo).toHaveBeenCalledTimes(1)
+
+    rerender({ ready: false })
+    rerender({ ready: true })
+    expect(window.scrollTo).toHaveBeenCalledTimes(1)
+  })
+
+  it('scrolls once per entry, not on every re-render', () => {
+    const { rerender } = renderHook(() => useScrollRestoration())
+    rerender()
+    rerender()
+    expect(window.scrollTo).toHaveBeenCalledTimes(1)
+  })
+
+  // The artist page reports ready as soon as its header record is cached, while the albums below
+  // are still loading. A single scrollTo lands on a viewport-tall document and is clamped to 0.
+  it('keeps trying until the page is tall enough to hold the offset', () => {
+    const fillPageTo = returnToUnfilledPage(800)
+    expect(window.scrollY).toBe(0)
+
+    fillPageTo(1200)
+    advanceFrames()
+    expect(window.scrollY).toBe(800)
+  })
+
+  it('stops retrying once the user scrolls somewhere else', () => {
+    const fillPageTo = returnToUnfilledPage(800)
+
+    fillPageTo(1200)
+    window.scrollY = 300
+    advanceFrames()
+    expect(window.scrollY).toBe(300)
+  })
+
+  // A trackpad back-swipe keeps firing wheel momentum through the restore, so treating input as
+  // the yield signal cancels the gesture's own restore.
+  it('keeps restoring while wheel momentum fires, as a back-swipe does', () => {
+    const fillPageTo = returnToUnfilledPage(800)
+
+    act(() => {
+      for (let i = 0; i < 10; i++) {
+        window.dispatchEvent(
+          new WheelEvent('wheel', { deltaX: -30, deltaY: 0 }),
+        )
+      }
+    })
+    fillPageTo(1200)
+    advanceFrames()
+    expect(window.scrollY).toBe(800)
+  })
+
+  // The incoming page tops itself in its own layout effect, while this page's scroll listener is
+  // still attached. Banking the offset any later than the teardown records that 0 instead.
+  it('records the offset before the next page scrolls to the top', () => {
+    const Outgoing = () => {
+      useScrollRestoration()
+      return null
+    }
+    const Incoming = () => {
+      useLayoutEffect(() => {
+        window.scrollY = 0
+        window.dispatchEvent(new Event('scroll'))
+      }, [])
+      return null
+    }
+
+    const { rerender } = render(<Outgoing />)
+    scrollTo(1400)
+
+    mockLocation.pathname = '/album/al-1/show'
+    rerender(<Incoming />)
+
+    mockLocation.pathname = '/album'
+    mockHistory.action = 'POP'
+    renderHook(() => useScrollRestoration())
+    expect(window.scrollTo).toHaveBeenLastCalledWith({ top: 1400 })
+  })
+
+  it('keeps saving after the restore, so leaving again remembers the new offset', () => {
+    const first = renderHook(() => useScrollRestoration())
+    scrollTo(200)
+    first.unmount()
+
+    mockHistory.action = 'POP'
+    const second = renderHook(() => useScrollRestoration())
+    expect(window.scrollTo).toHaveBeenLastCalledWith({ top: 200 })
+    scrollTo(900)
+    second.unmount()
+
+    renderHook(() => useScrollRestoration())
+    expect(window.scrollTo).toHaveBeenLastCalledWith({ top: 900 })
+  })
+})

ui/src/themes/useCurrentTheme.js

@@ -3,10 +3,14 @@ import useMediaQuery from '@material-ui/core/useMediaQuery'
 import themes from './index'
 import { AUTO_THEME_ID } from '../consts'
 import config from '../config'
-import { useEffect } from 'react'
+import { useEffect, useMemo } from 'react'
 
 const useCurrentTheme = () => {
-  const prefersLightMode = useMediaQuery('(prefers-color-scheme: light)')
+  // Runs above the ThemeProvider carrying the prop below, so it needs its own noSsr or the
+  // auto theme renders dark first and flips.
+  const prefersLightMode = useMediaQuery('(prefers-color-scheme: light)', {
+    noSsr: true,
+  })
   const theme = useSelector((state) => {
     if (state.theme === AUTO_THEME_ID) {
       return prefersLightMode ? themes.LightTheme : themes.DarkTheme
@@ -50,7 +54,15 @@ const useCurrentTheme = () => {
     document.body.style.backgroundColor = bgColor
   }, [theme])
 
-  return theme
+  // We never server-render, so let media queries resolve on the first render: the default
+  // defers them to an effect, which makes every mount paint the wrong breakpoint and reflow.
+  return useMemo(
+    () => ({
+      ...theme,
+      props: { ...theme.props, MuiUseMediaQuery: { noSsr: true } },
+    }),
+    [theme],
+  )
 }
 
 export default useCurrentTheme