diff --git a/contrib/navidrome b/contrib/navidrome index bb6908797..8ab5afe2e 100644 --- a/contrib/navidrome +++ b/contrib/navidrome @@ -2,7 +2,7 @@ name=$RC_SVCNAME command="/opt/navidrome/${RC_SVCNAME}" -command_args="-datafolder /opt/navidrome" +command_args="--datafolder /opt/navidrome" command_user="${RC_SVCNAME}" pidfile="/var/run/${RC_SVCNAME}.pid" output_log="/opt/navidrome/${RC_SVCNAME}.log" diff --git a/model/criteria/fields.go b/model/criteria/fields.go index 5c9ec898d..a9a634c01 100644 --- a/model/criteria/fields.go +++ b/model/criteria/fields.go @@ -71,6 +71,11 @@ var fieldMap = map[string]FieldInfo{ "albumlastplayed": {}, "albumdateloved": {}, "albumdaterated": {}, + "albumdateadded": {}, + "albumdatemodified": {}, + "albumduration": {}, + "albumsongcount": {}, + "albumsize": {}, "artistrating": {}, "artistloved": {Boolean: true}, "artistplaycount": {}, diff --git a/persistence/criteria_sql.go b/persistence/criteria_sql.go index 43cab4fdd..70f9a1fb5 100644 --- a/persistence/criteria_sql.go +++ b/persistence/criteria_sql.go @@ -21,6 +21,7 @@ const ( smartPlaylistJoinNone smartPlaylistJoinType = 0 smartPlaylistJoinAlbumAnnotation smartPlaylistJoinType = 1 << iota smartPlaylistJoinArtistAnnotation + smartPlaylistJoinAlbum ) func (j smartPlaylistJoinType) has(other smartPlaylistJoinType) bool { @@ -103,6 +104,11 @@ var smartPlaylistFields = map[string]smartPlaylistField{ "albumlastplayed": {expr: "album_annotation.play_date", joinType: smartPlaylistJoinAlbumAnnotation}, "albumdateloved": {expr: "album_annotation.starred_at", joinType: smartPlaylistJoinAlbumAnnotation}, "albumdaterated": {expr: "album_annotation.rated_at", joinType: smartPlaylistJoinAlbumAnnotation}, + "albumdateadded": {expr: "album.created_at", joinType: smartPlaylistJoinAlbum}, + "albumdatemodified": {expr: "album.updated_at", joinType: smartPlaylistJoinAlbum}, + "albumduration": {expr: "album.duration", joinType: smartPlaylistJoinAlbum}, + "albumsongcount": {expr: "album.song_count", joinType: smartPlaylistJoinAlbum}, + "albumsize": {expr: "album.size", joinType: smartPlaylistJoinAlbum}, "artistrating": {expr: "artist_annotation.rating", coalesceDefault: 0, joinType: smartPlaylistJoinArtistAnnotation}, "artistloved": {expr: "artist_annotation.starred", coalesceDefault: false, joinType: smartPlaylistJoinArtistAnnotation}, "artistplaycount": {expr: "artist_annotation.play_count", coalesceDefault: 0, joinType: smartPlaylistJoinArtistAnnotation}, diff --git a/persistence/criteria_sql_test.go b/persistence/criteria_sql_test.go index 59bdc4452..0cd7187c5 100644 --- a/persistence/criteria_sql_test.go +++ b/persistence/criteria_sql_test.go @@ -51,6 +51,11 @@ var _ = Describe("Smart playlist criteria SQL", func() { Entry("not in playlist", criteria.NotInPlaylist{"id": "deadbeef-dead-beef"}, "media_file.id NOT IN (SELECT media_file_id FROM playlist_tracks pl LEFT JOIN playlist on pl.playlist_id = playlist.id WHERE (pl.playlist_id = ? AND playlist.public = ?))", "deadbeef-dead-beef", 1), Entry("album annotation", criteria.Gt{"albumRating": 3}, "album_annotation.rating > ?", 3), Entry("artist annotation", criteria.Is{"artistLoved": true}, "artist_annotation.starred = ?", true), + Entry("album column", criteria.Gt{"albumSongCount": 5}, "album.song_count > ?", 5), + Entry("album duration column", criteria.Lt{"albumDuration": 600}, "album.duration < ?", 600), + Entry("album size column", criteria.Gt{"albumSize": 1000}, "album.size > ?", 1000), + Entry("album date column", criteria.After{"albumDateAdded": time.Date(2021, 10, 1, 0, 0, 0, 0, time.Local)}, "album.created_at > ?", time.Date(2021, 10, 1, 0, 0, 0, 0, time.Local)), + Entry("album modified column", criteria.Before{"albumDateModified": time.Date(2021, 10, 1, 0, 0, 0, 0, time.Local)}, "album.updated_at < ?", time.Date(2021, 10, 1, 0, 0, 0, 0, time.Local)), // Annotation fields use a COALESCE default (0 for numeric, false for bool) so that tracks // with no annotation row behave as that default. To keep the annotation index usable, the // COALESCE is dropped when the compared value cannot match the default (the missing-row @@ -298,6 +303,11 @@ var _ = Describe("Smart playlist criteria SQL", func() { Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "random"}).OrderBy()).To(Equal("random() asc")) }) + It("sorts by album columns bare, with no COALESCE default", func() { + Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "-albumDateAdded,trackNumber"}).OrderBy()). + To(Equal("album.created_at desc, media_file.track_number asc")) + }) + It("sorts by multiple fields", func() { Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "title,-rating"}).OrderBy()).To(Equal("media_file.title asc, COALESCE(annotation.rating, 0) desc")) }) @@ -323,6 +333,28 @@ var _ = Describe("Smart playlist criteria SQL", func() { } }) + It("declares a joinType matching the table each field selects from", func() { + // Omitting the joinType still compiles, so without this the field would only fail at + // refresh time with "no such column". + joinByTable := map[string]smartPlaylistJoinType{ + "media_file": smartPlaylistJoinNone, + "annotation": smartPlaylistJoinNone, + "album": smartPlaylistJoinAlbum, + "album_annotation": smartPlaylistJoinAlbumAnnotation, + "artist_annotation": smartPlaylistJoinArtistAnnotation, + } + for name, field := range smartPlaylistFields { + if field.expr == "" { + continue + } + table, _, ok := strings.Cut(field.expr, ".") + Expect(ok).To(BeTrue(), "field %q has expr %q with no table prefix", name, field.expr) + want, known := joinByTable[table] + Expect(known).To(BeTrue(), "field %q selects from unknown table %q", name, table) + Expect(field.joinType).To(Equal(want), "field %q selects from %q but declares the wrong joinType", name, table) + } + }) + Describe("JSON condition merging", func() { It("merges multiple role conditions in an OR group into a single EXISTS", func() { expr := criteria.Any{ @@ -585,5 +617,21 @@ var _ = Describe("Smart playlist criteria SQL", func() { Expect(newSmartPlaylistCriteria(c).RequiredJoins().has(smartPlaylistJoinArtistAnnotation)).To(BeTrue()) }) + + It("keeps a sort-only album join out of the expression joins", func() { + c := criteria.Criteria{Expression: criteria.All{criteria.Contains{"title": "love"}}, Sort: "-albumDateAdded"} + cSQL := newSmartPlaylistCriteria(c) + + Expect(cSQL.ExpressionJoins()).To(Equal(smartPlaylistJoinNone)) + Expect(cSQL.RequiredJoins().has(smartPlaylistJoinAlbum)).To(BeTrue()) + }) + + It("distinguishes the album join from the album annotation join", func() { + c := criteria.Criteria{Expression: criteria.All{criteria.Gt{"albumRating": 3}}} + joins := newSmartPlaylistCriteria(c).RequiredJoins() + + Expect(joins.has(smartPlaylistJoinAlbumAnnotation)).To(BeTrue()) + Expect(joins.has(smartPlaylistJoinAlbum)).To(BeFalse()) + }) }) }) diff --git a/persistence/e2e/smartplaylist_test.go b/persistence/e2e/smartplaylist_test.go index 4966d3b8a..b96605d09 100644 --- a/persistence/e2e/smartplaylist_test.go +++ b/persistence/e2e/smartplaylist_test.go @@ -204,6 +204,43 @@ var _ = Describe("Smart Playlists", func() { }) }) + Describe("Album aggregate fields", func() { + // Abbey Road and IV have two tracks each; the other four albums have one. + It("matches albums with more than one track", func() { + results := evaluateRule(`{"all":[{"gt":{"albumsongcount":1}}]}`) + Expect(results).To(ConsistOf("Come Together", "Something", "Stairway To Heaven", "Black Dog")) + }) + + It("matches single-track albums", func() { + results := evaluateRule(`{"all":[{"is":{"albumsongcount":1}}]}`) + Expect(results).To(ConsistOf("So What", "Bohemian Rhapsody", "All Along the Watchtower", + "We Are the Champions")) + }) + + It("matches albumDateAdded inTheLast 1 day", func() { + results := evaluateRule(`{"all":[{"inTheLast":{"albumdateadded":1}}]}`) + Expect(results).To(ConsistOf("Come Together", "Something", "Stairway To Heaven", "Black Dog", + "So What", "Bohemian Rhapsody", "All Along the Watchtower", "We Are the Champions")) + }) + + It("matches albumDateModified before a far-future date", func() { + results := evaluateRule(`{"all":[{"before":{"albumdatemodified":"2099-01-01"}}]}`) + Expect(results).To(HaveLen(8)) + }) + + // Fixture durations are randomized, so only the aggregate being populated can be asserted. + It("resolves albumDuration and albumSize to non-zero aggregates", func() { + Expect(evaluateRule(`{"all":[{"gt":{"albumduration":0}}]}`)).To(HaveLen(8)) + Expect(evaluateRule(`{"all":[{"gt":{"albumsize":0}}]}`)).To(HaveLen(8)) + }) + + It("groups by album date and orders within the album (issue #5347)", func() { + results := evaluateRuleOrdered( + `{"all":[{"is":{"album":"Abbey Road"}}],"sort":"-albumdateadded,tracknumber"}`) + Expect(results).To(Equal([]string{"Come Together", "Something"})) + }) + }) + Describe("Logic operators", func() { It("matches with ALL (AND)", func() { results := evaluateRule(`{"all":[{"is":{"genre":"Blues"}},{"gt":{"bpm":130}}]}`) diff --git a/persistence/smart_playlist_repository.go b/persistence/smart_playlist_repository.go index f7c41597d..5f84981b3 100644 --- a/persistence/smart_playlist_repository.go +++ b/persistence/smart_playlist_repository.go @@ -122,7 +122,7 @@ func (r *playlistRepository) resolvePercentageLimit(pls *model.Playlist, rulesSQ exprJoins := rulesSQL.ExpressionJoins() countSq := Select("count(*) as count").From("media_file") countSq = r.addMediaFileAnnotationJoin(countSq, userID) - countSq = r.addSmartPlaylistAnnotationJoins(countSq, exprJoins, userID) + countSq = r.addSmartPlaylistJoins(countSq, exprJoins, userID) countSq = r.applyLibraryFilter(countSq, "media_file") cond, err := rulesSQL.Where() @@ -144,7 +144,7 @@ func (r *playlistRepository) resolvePercentageLimit(pls *model.Playlist, rulesSQ } // buildSmartPlaylistQuery constructs the SQL query to select media files matching the smart playlist criteria, -// including necessary joins for annotations and library filtering. +// including the joins its fields require and library filtering. func (r *playlistRepository) buildSmartPlaylistQuery(pls *model.Playlist, rulesSQL smartPlaylistCriteria, userID string) SelectBuilder { orderBy := rulesSQL.OrderBy() sq := Select("row_number() over (order by "+orderBy+") as id", "'"+pls.ID+"' as playlist_id", "media_file.id as media_file_id"). @@ -152,7 +152,7 @@ func (r *playlistRepository) buildSmartPlaylistQuery(pls *model.Playlist, rulesS sq = r.addMediaFileAnnotationJoin(sq, userID) requiredJoins := rulesSQL.RequiredJoins() - sq = r.addSmartPlaylistAnnotationJoins(sq, requiredJoins, userID) + sq = r.addSmartPlaylistJoins(sq, requiredJoins, userID) sq = r.applyLibraryFilter(sq, "media_file") return sq } @@ -166,9 +166,8 @@ func (r *playlistRepository) addMediaFileAnnotationJoin(sq SelectBuilder, userID " AND annotation.user_id = ?)", userID) } -// addSmartPlaylistAnnotationJoins adds left joins to the annotation table for albums and artists as needed based on -// the smart playlist criteria, filtering by user ID to include user-specific annotations in the evaluation. -func (r *playlistRepository) addSmartPlaylistAnnotationJoins(sq SelectBuilder, joins smartPlaylistJoinType, userID string) SelectBuilder { +// addSmartPlaylistJoins adds the left joins required by the criteria's fields. +func (r *playlistRepository) addSmartPlaylistJoins(sq SelectBuilder, joins smartPlaylistJoinType, userID string) SelectBuilder { if joins.has(smartPlaylistJoinAlbumAnnotation) { sq = sq.LeftJoin("annotation AS album_annotation ON ("+ "album_annotation.item_id = media_file.album_id"+ @@ -181,6 +180,9 @@ func (r *playlistRepository) addSmartPlaylistAnnotationJoins(sq SelectBuilder, j " AND artist_annotation.item_type = 'artist'"+ " AND artist_annotation.user_id = ?)", userID) } + if joins.has(smartPlaylistJoinAlbum) { + sq = sq.LeftJoin("album ON album.id = media_file.album_id") + } return sq } diff --git a/persistence/smart_playlist_repository_test.go b/persistence/smart_playlist_repository_test.go index 8e3ae488f..e62436890 100644 --- a/persistence/smart_playlist_repository_test.go +++ b/persistence/smart_playlist_repository_test.go @@ -9,6 +9,7 @@ import ( "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/criteria" "github.com/navidrome/navidrome/model/request" + "github.com/navidrome/navidrome/utils/slice" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/pocketbase/dbx" @@ -392,6 +393,40 @@ var _ = Describe("PlaylistRepository - Smart Playlists", func() { }) }) + Describe("Smart Playlists with Album Aggregate Criteria", func() { + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.SmartPlaylistRefreshDelay = -1 * time.Second + }) + + trackIDsOf := func(rules *criteria.Criteria) []string { + newPls := model.Playlist{Name: "Album Aggregates", OwnerID: "userid", Rules: rules} + Expect(repo.Put(&newPls)).To(Succeed()) + DeferCleanup(func() { _ = repo.Delete(newPls.ID) }) + + pls, err := repo.GetWithTracks(newPls.ID, true, false) + Expect(err).ToNot(HaveOccurred()) + return slice.Map(pls.Tracks, func(t model.PlaylistTrack) string { return t.MediaFileID }) + } + + It("filters on albumSongCount", func() { + // albumMultiDisc (ID "104") is the only fixture album with SongCount > 3 + rules := &criteria.Criteria{Expression: criteria.All{criteria.Gt{"albumSongCount": 3}}} + + Expect(trackIDsOf(rules)).To(ConsistOf("2001", "2002", "2003", "2004")) + }) + + It("sorts by an album field not referenced in the expression (issue #5347)", func() { + // All four tracks share album 104, so the album date ties and disc/track number decide. + rules := &criteria.Criteria{ + Expression: criteria.All{criteria.Is{"album": "Multi Disc Album"}}, + Sort: "-albumDateAdded,discNumber,trackNumber", + } + + Expect(trackIDsOf(rules)).To(HaveExactElements("2002", "2004", "2003", "2001")) + }) + }) + Describe("Smart Playlists with Tag Criteria", func() { var mfRepo model.MediaFileRepository var testPlaylistID string diff --git a/scheduler/log_adapter.go b/scheduler/log_adapter.go index 057818785..f7089f97c 100644 --- a/scheduler/log_adapter.go +++ b/scheduler/log_adapter.go @@ -11,7 +11,7 @@ func (l *logger) Info(msg string, keysAndValues ...any) { "Scheduler: " + msg, } args = append(args, keysAndValues...) - log.Debug(args...) + log.Trace(args...) } func (l *logger) Error(err error, msg string, keysAndValues ...any) { diff --git a/ui/src/album/AlbumList.jsx b/ui/src/album/AlbumList.jsx index 5108bfaa1..cd73fa0c3 100644 --- a/ui/src/album/AlbumList.jsx +++ b/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={} > - {albumView.grid ? ( - - ) : ( - - )} + + {albumView.grid ? ( + + ) : ( + + )} + } /> diff --git a/ui/src/album/AlbumShow.jsx b/ui/src/album/AlbumShow.jsx index c9e944999..86b5ab1bc 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, 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 ( <> diff --git a/ui/src/artist/ArtistShow.jsx b/ui/src/artist/ArtistShow.jsx index 955a565d6..eef2989d5 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, + 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 diff --git a/ui/src/common/ArtistLinkField.jsx b/ui/src/common/ArtistLinkField.jsx index d41b47b06..2a70ded7c 100644 --- a/ui/src/common/ArtistLinkField.jsx +++ b/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() diff --git a/ui/src/common/SongDatagrid.jsx b/ui/src/common/SongDatagrid.jsx index d2c98bbe7..4a813f84d 100644 --- a/ui/src/common/SongDatagrid.jsx +++ b/ui/src/common/SongDatagrid.jsx @@ -78,7 +78,7 @@ const useStyles = makeStyles({ }, }) -const DiscSubtitleRow = forwardRef( +export const DiscSubtitleRow = forwardRef( ({ record, onClick, colSpan, contextAlwaysVisible }, ref) => { const translate = useTranslate() const isDesktop = useMediaQuery((theme) => theme.breakpoints.up('md')) @@ -153,13 +153,17 @@ const DiscSubtitleRow = forwardRef( {subtitle} {isLightboxOpen && !imageError && ( - + // Lightbox portals out of the row, but React still bubbles its + // events up this tree, where the row's onClick would play the disc. + e.stopPropagation()}> + + )} diff --git a/ui/src/common/SongDatagrid.test.jsx b/ui/src/common/SongDatagrid.test.jsx new file mode 100644 index 000000000..f8459d8eb --- /dev/null +++ b/ui/src/common/SongDatagrid.test.jsx @@ -0,0 +1,74 @@ +import React from 'react' +import { render, fireEvent, screen } from '@testing-library/react' +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { createTheme, ThemeProvider } from '@material-ui/core/styles' +import { DiscSubtitleRow } from './SongDatagrid' + +vi.mock('../subsonic', () => ({ + default: { getDiscCoverArtUrl: () => 'http://localhost/cover.jpg' }, +})) + +vi.mock('react-redux', () => ({ useDispatch: () => vi.fn() })) + +vi.mock('../common', () => ({ AlbumContextMenu: () => null })) + +vi.mock('react-dnd', () => ({ useDrag: () => [{}, vi.fn()] })) + +const record = { + id: 'song-1', + albumId: 'album-1', + album: 'The Album', + discNumber: 2, + discSubtitle: 'Bonus Disc', + updatedAt: '2024-01-01', +} + +const renderRow = (onClick) => + render( + + + + + +
+ , + ) + +const openLightbox = () => { + fireEvent.click(document.querySelector('img')) + expect(document.querySelector('.ril__closeButton')).toBeTruthy() +} + +describe('DiscSubtitleRow', () => { + beforeEach(() => vi.clearAllMocks()) + + it('plays the disc when the row is clicked', () => { + const onClick = vi.fn() + renderRow(onClick) + fireEvent.click(screen.getByText('Bonus Disc')) + expect(onClick).toHaveBeenCalledWith(2) + }) + + it('does not play the disc when opening the lightbox', () => { + const onClick = vi.fn() + renderRow(onClick) + openLightbox() + expect(onClick).not.toHaveBeenCalled() + }) + + it('does not play the disc when closing the lightbox', () => { + const onClick = vi.fn() + renderRow(onClick) + openLightbox() + fireEvent.click(document.querySelector('.ril__closeButton')) + expect(onClick).not.toHaveBeenCalled() + }) + + it('does not play the disc when clicking the lightbox backdrop', () => { + const onClick = vi.fn() + renderRow(onClick) + openLightbox() + fireEvent.click(document.querySelector('.ril__inner')) + expect(onClick).not.toHaveBeenCalled() + }) +}) diff --git a/ui/src/common/index.js b/ui/src/common/index.js index ac8d7f62c..c88ea3e9c 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 './useScrollRestoration' export * from './useRefreshOnEvents' export * from './useToggleLove' export * from './useTraceUpdate' diff --git a/ui/src/common/useScrollRestoration.jsx b/ui/src/common/useScrollRestoration.jsx new file mode 100644 index 000000000..94e5cb2fd --- /dev/null +++ b/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]) +} diff --git a/ui/src/common/useScrollRestoration.test.jsx b/ui/src/common/useScrollRestoration.test.jsx new file mode 100644 index 000000000..7ca67a724 --- /dev/null +++ b/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() + scrollTo(1400) + + mockLocation.pathname = '/album/al-1/show' + rerender() + + 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 }) + }) +}) diff --git a/ui/src/themes/useCurrentTheme.js b/ui/src/themes/useCurrentTheme.js index 0d986033d..2b5d13d13 100644 --- a/ui/src/themes/useCurrentTheme.js +++ b/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