From c1e4f1c9ae3d340157ec66f3f6382c899b1147bd Mon Sep 17 00:00:00 2001 From: Deluan Date: Tue, 4 Aug 2026 09:39:00 -0400 Subject: [PATCH] revert(ui): move detail-page scroll handling to its own branch Reverts 40153bd43. Scroll position carrying over between pages is stock React Router behaviour rather than something this branch introduced, so the fix does not belong in an artwork PR that is otherwise ready for review. The work continues on fix/scroll-restoration, off master, where it grew into per-route restoration: a new page starts at the top and going back returns to where you were, which the scroll-to-top could not do. --- 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 | 44 --------------------------- 5 files changed, 2 insertions(+), 65 deletions(-) delete mode 100644 ui/src/common/useScrollToTop.jsx delete mode 100644 ui/src/common/useScrollToTop.test.jsx diff --git a/ui/src/album/AlbumShow.jsx b/ui/src/album/AlbumShow.jsx index c9a3defda..c9e944999 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, useScrollToTop, Title } from '../common' +import { useResourceRefresh, Title } from '../common' const useStyles = makeStyles( (theme) => ({ @@ -28,7 +28,6 @@ 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 99a588a05..955a565d6 100644 --- a/ui/src/artist/ArtistShow.jsx +++ b/ui/src/artist/ArtistShow.jsx @@ -13,12 +13,7 @@ import subsonic from '../subsonic' import AlbumGridView from '../album/AlbumGridView' import MobileArtistDetails from './MobileArtistDetails' import DesktopArtistDetails from './DesktopArtistDetails' -import { - useAlbumsPerPage, - useResourceRefresh, - useScrollToTop, - Title, -} from '../common/index.js' +import { useAlbumsPerPage, useResourceRefresh, Title } from '../common/index.js' import ArtistActions from './ArtistActions' import { makeStyles } from '@material-ui/core' @@ -90,7 +85,6 @@ 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 9dd966537..5d0276cc1 100644 --- a/ui/src/common/index.js +++ b/ui/src/common/index.js @@ -28,7 +28,6 @@ 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 deleted file mode 100644 index b00e4c650..000000000 --- a/ui/src/common/useScrollToTop.jsx +++ /dev/null @@ -1,11 +0,0 @@ -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 deleted file mode 100644 index 89bde3d37..000000000 --- a/ui/src/common/useScrollToTop.test.jsx +++ /dev/null @@ -1,44 +0,0 @@ -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 }) - }) - - // Detail-to-detail navigation reuses the component, so only the key change signals a new page. - 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. - 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) - }) -})