From 40153bd434b35f84cff0a7b187d920c28c3be1e4 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sun, 26 Jul 2026 12:16:01 -0400 Subject: [PATCH] fix(ui): anchor detail pages to the top when opened from a list React Router keeps the previous page's scroll offset, so opening an album from a scrolled list started the detail page mid-song-list. Artist pages had the same bug; it just shows less because the artist list is rarely long enough to scroll far. Keyed on the record id rather than mount, so detail-to-detail navigation (an album's artist link) resets too, and so the scroll waits for the record instead of firing against an empty page. --- ui/src/album/AlbumShow.jsx | 3 +- ui/src/artist/ArtistShow.jsx | 8 ++++- ui/src/common/index.js | 1 + ui/src/common/useScrollToTop.jsx | 11 +++++++ ui/src/common/useScrollToTop.test.jsx | 46 +++++++++++++++++++++++++++ 5 files changed, 67 insertions(+), 2 deletions(-) create mode 100644 ui/src/common/useScrollToTop.jsx create mode 100644 ui/src/common/useScrollToTop.test.jsx diff --git a/ui/src/album/AlbumShow.jsx b/ui/src/album/AlbumShow.jsx index c9e944999..c9a3defda 100644 --- a/ui/src/album/AlbumShow.jsx +++ b/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, useScrollToTop, Title } from '../common' const useStyles = makeStyles( (theme) => ({ @@ -28,6 +28,7 @@ const AlbumShowLayout = (props) => { const { record } = context const classes = useStyles() useResourceRefresh('album', 'song') + useScrollToTop(record?.id) return ( <> diff --git a/ui/src/artist/ArtistShow.jsx b/ui/src/artist/ArtistShow.jsx index 955a565d6..99a588a05 100644 --- a/ui/src/artist/ArtistShow.jsx +++ b/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, + useScrollToTop, + 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') + useScrollToTop(record?.id) const maxPerPage = 90 let perPage = 0 diff --git a/ui/src/common/index.js b/ui/src/common/index.js index 5d0276cc1..9dd966537 100644 --- a/ui/src/common/index.js +++ b/ui/src/common/index.js @@ -28,6 +28,7 @@ export * from './useAlbumsPerPage' export * from './useGetHandleArtistClick' export * from './useInterval' export * from './useResourceRefresh' +export * from './useScrollToTop' export * from './useRefreshOnEvents' export * from './useToggleLove' export * from './useTraceUpdate' diff --git a/ui/src/common/useScrollToTop.jsx b/ui/src/common/useScrollToTop.jsx new file mode 100644 index 000000000..b00e4c650 --- /dev/null +++ b/ui/src/common/useScrollToTop.jsx @@ -0,0 +1,11 @@ +import { useEffect } from 'react' + +// React Router keeps the previous page's scroll offset, so a detail page opened from a scrolled +// list starts mid-page. Keyed on the record id so detail-to-detail navigation resets too. +export const useScrollToTop = (key) => { + useEffect(() => { + if (key) { + window.scrollTo({ top: 0 }) + } + }, [key]) +} diff --git a/ui/src/common/useScrollToTop.test.jsx b/ui/src/common/useScrollToTop.test.jsx new file mode 100644 index 000000000..631d1362e --- /dev/null +++ b/ui/src/common/useScrollToTop.test.jsx @@ -0,0 +1,46 @@ +import { renderHook } from '@testing-library/react-hooks' +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { useScrollToTop } from './useScrollToTop' + +describe('useScrollToTop', () => { + beforeEach(() => { + window.scrollTo = vi.fn() + }) + + it('scrolls to the top on mount', () => { + renderHook(() => useScrollToTop('al-1')) + expect(window.scrollTo).toHaveBeenCalledWith({ top: 0 }) + }) + + // Navigating straight from one detail page to another (an album's artist link, say) reuses the + // component, so only the key change tells us we are looking at something new. + it('scrolls again when the key changes', () => { + const { rerender } = renderHook(({ id }) => useScrollToTop(id), { + initialProps: { id: 'al-1' }, + }) + expect(window.scrollTo).toHaveBeenCalledTimes(1) + + rerender({ id: 'al-2' }) + expect(window.scrollTo).toHaveBeenCalledTimes(2) + }) + + it('does not scroll again on a re-render with the same key', () => { + const { rerender } = renderHook(({ id }) => useScrollToTop(id), { + initialProps: { id: 'al-1' }, + }) + rerender({ id: 'al-1' }) + expect(window.scrollTo).toHaveBeenCalledTimes(1) + }) + + // The record arrives after the first render, so the key starts undefined; scrolling then would + // fire before the page has its content and read as a no-op. + it('waits for a key rather than scrolling on an empty record', () => { + const { rerender } = renderHook(({ id }) => useScrollToTop(id), { + initialProps: { id: undefined }, + }) + expect(window.scrollTo).not.toHaveBeenCalled() + + rerender({ id: 'al-1' }) + expect(window.scrollTo).toHaveBeenCalledTimes(1) + }) +})