mirror of
https://github.com/navidrome/navidrome.git
synced 2026-08-01 07:21:17 +00:00
feat(ui): remember 'items per page' selection across sessions (#5819)
* feat(ui): add perPageStore helper for items-per-page persistence * feat(ui): persist items-per-page selection in localStorage * feat(ui): enable per-page persistence on album grid, missing files and playlist tracks * feat(ui): restore saved album grid page size on fresh load * feat(ui): restore saved items-per-page on list load * fix(ui): move defaultRowsPerPageOptions to perPageStore to satisfy react-refresh lint * fix(ui): correct defaultRowsPerPageOptions import in List and add render test * refactor(ui): default getStoredPerPage fallback to the first option The fallback equalled options[0] at three of four call sites; default it so those sites stop restating it. SongList and useAlbumsPerPage still pass an explicit fallback where it legitimately differs from the first option. * fix(ui): persist only user-selected page sizes, validate album size against width Persisting on every pagination context value let a URL-injected perPage (e.g. the perPage=15 album link in NowPlayingPanel) or a forced single-option mobile grid overwrite the saved preference. Persist only a value that is an actual option in a multi-option selector. Also validate the album grid's redux session value against the current width so a size chosen at a wider breakpoint can't leave an out-of-range selector. * fix(ui): restore saved items-per-page on the radio list RadioList passed a hard-coded perPage that overrode List's stored seed via the props spread, so a radio-list page size was persisted but never restored. Seed it from storage like the other list views. * fix(ui): persist page size only on an actual selector change Watching the pagination context value meant any valid value persisted itself: loading a list at a breakpoint where the saved size is invalid stored the responsive fallback, and opening a URL with a valid ?perPage= stored that too, either way discarding the user's real preference. Inject a wrapped setPerPage instead, so only the rows-per-page selector writes. This also drops the option-validation heuristics, which the new trigger makes unnecessary.
This commit is contained in:
parent
bf79d2f3a2
commit
6c2644d208
@ -6,7 +6,6 @@ import {
|
||||
Filter,
|
||||
NullableBooleanInput,
|
||||
NumberInput,
|
||||
Pagination,
|
||||
ReferenceArrayInput,
|
||||
ReferenceInput,
|
||||
SearchInput,
|
||||
@ -20,6 +19,7 @@ import FavoriteIcon from '@material-ui/icons/Favorite'
|
||||
import { withWidth } from '@material-ui/core'
|
||||
import {
|
||||
List,
|
||||
Pagination,
|
||||
Title,
|
||||
useAlbumsPerPage,
|
||||
useResourceRefresh,
|
||||
|
||||
@ -100,6 +100,7 @@ const ArtistShowLayout = (props) => {
|
||||
const rowsPerPageOptions = [1, 2, 3].map((option) =>
|
||||
Math.trunc(option * (perPage / 3)),
|
||||
)
|
||||
// react-admin's Pagination on purpose: the common one would persist 30/60/90 under the album grid's key
|
||||
pagination = <Pagination rowsPerPageOptions={rowsPerPageOptions} />
|
||||
}
|
||||
|
||||
|
||||
@ -2,6 +2,7 @@ import React from 'react'
|
||||
import { List as RAList } from 'react-admin'
|
||||
import config from '../config'
|
||||
import { Pagination } from './Pagination'
|
||||
import { defaultRowsPerPageOptions, getStoredPerPage } from './perPageStore'
|
||||
import { Title } from './index'
|
||||
|
||||
export const List = (props) => {
|
||||
@ -15,7 +16,7 @@ export const List = (props) => {
|
||||
/>
|
||||
}
|
||||
debounce={config.uiSearchDebounceMs}
|
||||
perPage={15}
|
||||
perPage={getStoredPerPage(resource, defaultRowsPerPageOptions)}
|
||||
pagination={<Pagination />}
|
||||
{...props}
|
||||
/>
|
||||
|
||||
27
ui/src/common/List.test.jsx
Normal file
27
ui/src/common/List.test.jsx
Normal file
@ -0,0 +1,27 @@
|
||||
import React from 'react'
|
||||
import { render, screen } from '@testing-library/react'
|
||||
import { describe, it, expect, vi } from 'vitest'
|
||||
import { List } from './List'
|
||||
|
||||
// Only stub the heavy react-admin List controller (data fetching, router sync);
|
||||
// everything else, including our own Pagination/perPageStore wiring, stays real
|
||||
// so a bad import (the bug this test guards against) throws on render.
|
||||
vi.mock('react-admin', async (importOriginal) => {
|
||||
const actual = await importOriginal()
|
||||
return {
|
||||
...actual,
|
||||
List: ({ children }) => <div data-testid="ra-list">{children}</div>,
|
||||
}
|
||||
})
|
||||
|
||||
describe('List', () => {
|
||||
it('renders without throwing and shows its children', () => {
|
||||
render(
|
||||
<List resource="song">
|
||||
<div>list content</div>
|
||||
</List>,
|
||||
)
|
||||
expect(screen.getByTestId('ra-list')).toBeInTheDocument()
|
||||
expect(screen.getByText('list content')).toBeInTheDocument()
|
||||
})
|
||||
})
|
||||
@ -1,6 +1,29 @@
|
||||
import React from 'react'
|
||||
import { Pagination as RAPagination } from 'react-admin'
|
||||
import React, { useCallback } from 'react'
|
||||
import {
|
||||
Pagination as RAPagination,
|
||||
useListPaginationContext,
|
||||
} from 'react-admin'
|
||||
import { setStoredPerPage, defaultRowsPerPageOptions } from './perPageStore'
|
||||
|
||||
export const Pagination = (props) => (
|
||||
<RAPagination rowsPerPageOptions={[15, 25, 50]} {...props} />
|
||||
)
|
||||
export const Pagination = ({
|
||||
rowsPerPageOptions = defaultRowsPerPageOptions,
|
||||
...props
|
||||
}) => {
|
||||
const { resource, setPerPage } = useListPaginationContext()
|
||||
// Persist only a selector-driven change: mount, URL params and responsive
|
||||
// fallbacks never call setPerPage, so they can't overwrite the preference.
|
||||
const handleSetPerPage = useCallback(
|
||||
(value) => {
|
||||
if (resource) setStoredPerPage(resource, value)
|
||||
setPerPage(value)
|
||||
},
|
||||
[resource, setPerPage],
|
||||
)
|
||||
return (
|
||||
<RAPagination
|
||||
rowsPerPageOptions={rowsPerPageOptions}
|
||||
{...props}
|
||||
setPerPage={handleSetPerPage}
|
||||
/>
|
||||
)
|
||||
}
|
||||
|
||||
62
ui/src/common/Pagination.test.jsx
Normal file
62
ui/src/common/Pagination.test.jsx
Normal file
@ -0,0 +1,62 @@
|
||||
import React from 'react'
|
||||
import { render, screen, fireEvent } from '@testing-library/react'
|
||||
import { describe, it, expect, beforeEach, vi } from 'vitest'
|
||||
import { Pagination } from './Pagination'
|
||||
|
||||
// stub RA's Pagination so a test can invoke the injected setPerPage, i.e.
|
||||
// simulate an actual rows-per-page selection
|
||||
vi.mock('react-admin', async () => {
|
||||
const React = await vi.importActual('react')
|
||||
return {
|
||||
Pagination: ({ setPerPage }) =>
|
||||
React.createElement(
|
||||
'button',
|
||||
{ onClick: () => setPerPage(50) },
|
||||
'select 50',
|
||||
),
|
||||
useListPaginationContext: vi.fn(),
|
||||
}
|
||||
})
|
||||
|
||||
describe('Pagination', () => {
|
||||
let mockContext
|
||||
let setPerPage
|
||||
|
||||
beforeEach(async () => {
|
||||
vi.clearAllMocks()
|
||||
localStorage.clear()
|
||||
setPerPage = vi.fn()
|
||||
const { useListPaginationContext } = await import('react-admin')
|
||||
mockContext = vi.mocked(useListPaginationContext)
|
||||
})
|
||||
|
||||
const selectPerPage = () => fireEvent.click(screen.getByText('select 50'))
|
||||
|
||||
it('persists the page size chosen in the selector', () => {
|
||||
mockContext.mockReturnValue({ resource: 'song', perPage: 15, setPerPage })
|
||||
render(<Pagination />)
|
||||
selectPerPage()
|
||||
expect(localStorage.getItem('perPage.song')).toEqual('50')
|
||||
})
|
||||
|
||||
it('still applies the change to the list', () => {
|
||||
mockContext.mockReturnValue({ resource: 'song', perPage: 15, setPerPage })
|
||||
render(<Pagination />)
|
||||
selectPerPage()
|
||||
expect(setPerPage).toHaveBeenCalledWith(50)
|
||||
})
|
||||
|
||||
it('does not persist a page size the user did not select', () => {
|
||||
mockContext.mockReturnValue({ resource: 'song', perPage: 15, setPerPage })
|
||||
render(<Pagination />)
|
||||
expect(localStorage.getItem('perPage.song')).toBeNull()
|
||||
})
|
||||
|
||||
it('does not persist without a resource in context', () => {
|
||||
mockContext.mockReturnValue({ perPage: 15, setPerPage })
|
||||
render(<Pagination />)
|
||||
selectPerPage()
|
||||
expect(localStorage.getItem('perPage.undefined')).toBeNull()
|
||||
expect(setPerPage).toHaveBeenCalledWith(50)
|
||||
})
|
||||
})
|
||||
@ -10,6 +10,7 @@ export * from './DurationField'
|
||||
export * from './List'
|
||||
export * from './MultiLineTextField'
|
||||
export * from './Pagination'
|
||||
export * from './perPageStore'
|
||||
export * from './PlayButton'
|
||||
export * from './QuickFilter'
|
||||
export * from './RangeField'
|
||||
|
||||
11
ui/src/common/perPageStore.js
Normal file
11
ui/src/common/perPageStore.js
Normal file
@ -0,0 +1,11 @@
|
||||
export const defaultRowsPerPageOptions = [15, 25, 50]
|
||||
|
||||
const key = (resource) => `perPage.${resource}`
|
||||
|
||||
export const getStoredPerPage = (resource, options, fallback = options[0]) => {
|
||||
const stored = parseInt(localStorage.getItem(key(resource)), 10)
|
||||
return options.includes(stored) ? stored : fallback
|
||||
}
|
||||
|
||||
export const setStoredPerPage = (resource, perPage) =>
|
||||
localStorage.setItem(key(resource), String(perPage))
|
||||
40
ui/src/common/perPageStore.test.js
Normal file
40
ui/src/common/perPageStore.test.js
Normal file
@ -0,0 +1,40 @@
|
||||
import { describe, it, expect, beforeEach } from 'vitest'
|
||||
import { getStoredPerPage, setStoredPerPage } from './perPageStore'
|
||||
|
||||
const options = [15, 25, 50]
|
||||
|
||||
describe('perPageStore', () => {
|
||||
beforeEach(() => {
|
||||
localStorage.clear()
|
||||
})
|
||||
|
||||
it('round-trips a stored value', () => {
|
||||
setStoredPerPage('song', 25)
|
||||
expect(getStoredPerPage('song', options, 15)).toEqual(25)
|
||||
})
|
||||
|
||||
it('keys values per resource', () => {
|
||||
setStoredPerPage('song', 25)
|
||||
setStoredPerPage('playlist', 50)
|
||||
expect(getStoredPerPage('song', options, 15)).toEqual(25)
|
||||
expect(getStoredPerPage('playlist', options, 15)).toEqual(50)
|
||||
})
|
||||
|
||||
it('returns the fallback when nothing is stored', () => {
|
||||
expect(getStoredPerPage('song', options, 15)).toEqual(15)
|
||||
})
|
||||
|
||||
it('returns the fallback for garbage values', () => {
|
||||
localStorage.setItem('perPage.song', 'bogus')
|
||||
expect(getStoredPerPage('song', options, 15)).toEqual(15)
|
||||
})
|
||||
|
||||
it('returns the fallback when the stored value is not a valid option', () => {
|
||||
setStoredPerPage('album', 90)
|
||||
expect(getStoredPerPage('album', [18, 36, 72], 18)).toEqual(18)
|
||||
})
|
||||
|
||||
it('defaults the fallback to the first option', () => {
|
||||
expect(getStoredPerPage('song', options)).toEqual(15)
|
||||
})
|
||||
})
|
||||
@ -1,4 +1,5 @@
|
||||
import { useSelector } from 'react-redux'
|
||||
import { getStoredPerPage } from './perPageStore'
|
||||
|
||||
const getPerPage = (width) => {
|
||||
if (width === 'xs') return 12
|
||||
@ -17,10 +18,15 @@ const getPerPageOptions = (width) => {
|
||||
}
|
||||
|
||||
export const useAlbumsPerPage = (width) => {
|
||||
const perPage =
|
||||
useSelector(
|
||||
(state) => state?.admin.resources?.album?.list?.params?.perPage,
|
||||
) || getPerPage(width)
|
||||
const options = getPerPageOptions(width)
|
||||
const sessionPerPage = useSelector(
|
||||
(state) => state?.admin.resources?.album?.list?.params?.perPage,
|
||||
)
|
||||
// Use the session value only when it's valid for the current width, so a
|
||||
// size picked at a wider breakpoint can't leave an out-of-range selector.
|
||||
const perPage = options.includes(sessionPerPage)
|
||||
? sessionPerPage
|
||||
: getStoredPerPage('album', options, getPerPage(width))
|
||||
|
||||
return [perPage, getPerPageOptions(width)]
|
||||
return [perPage, options]
|
||||
}
|
||||
|
||||
61
ui/src/common/useAlbumsPerPage.test.jsx
Normal file
61
ui/src/common/useAlbumsPerPage.test.jsx
Normal file
@ -0,0 +1,61 @@
|
||||
import { renderHook } from '@testing-library/react-hooks'
|
||||
import { describe, it, expect, beforeEach, vi } from 'vitest'
|
||||
import { useAlbumsPerPage } from './useAlbumsPerPage'
|
||||
import { setStoredPerPage } from './perPageStore'
|
||||
|
||||
vi.mock('react-redux', () => ({
|
||||
useSelector: vi.fn(),
|
||||
}))
|
||||
|
||||
describe('useAlbumsPerPage', () => {
|
||||
let mockUseSelector
|
||||
|
||||
beforeEach(async () => {
|
||||
vi.clearAllMocks()
|
||||
localStorage.clear()
|
||||
const { useSelector } = await import('react-redux')
|
||||
mockUseSelector = vi.mocked(useSelector)
|
||||
})
|
||||
|
||||
const setReduxPerPage = (value) =>
|
||||
mockUseSelector.mockImplementation((selector) =>
|
||||
selector({
|
||||
admin: {
|
||||
resources: { album: { list: { params: { perPage: value } } } },
|
||||
},
|
||||
}),
|
||||
)
|
||||
|
||||
it('prefers the redux session value over the stored one', () => {
|
||||
setReduxPerPage(36)
|
||||
setStoredPerPage('album', 72)
|
||||
const { result } = renderHook(() => useAlbumsPerPage('lg'))
|
||||
expect(result.current[0]).toEqual(36)
|
||||
})
|
||||
|
||||
it('falls back to the stored value on fresh load', () => {
|
||||
setReduxPerPage(undefined)
|
||||
setStoredPerPage('album', 72)
|
||||
const { result } = renderHook(() => useAlbumsPerPage('lg'))
|
||||
expect(result.current[0]).toEqual(72)
|
||||
})
|
||||
|
||||
it('ignores stored values invalid for the current width', () => {
|
||||
setReduxPerPage(undefined)
|
||||
setStoredPerPage('album', 72) // valid for lg, not for md
|
||||
const { result } = renderHook(() => useAlbumsPerPage('md'))
|
||||
expect(result.current[0]).toEqual(12)
|
||||
})
|
||||
|
||||
it('returns the responsive default when nothing is stored', () => {
|
||||
setReduxPerPage(undefined)
|
||||
const { result } = renderHook(() => useAlbumsPerPage('xl'))
|
||||
expect(result.current).toEqual([36, [18, 36, 72]])
|
||||
})
|
||||
|
||||
it('ignores a redux value invalid for the current width', () => {
|
||||
setReduxPerPage(72) // valid for lg, not for md
|
||||
const { result } = renderHook(() => useAlbumsPerPage('md'))
|
||||
expect(result.current[0]).toEqual(12)
|
||||
})
|
||||
})
|
||||
@ -1,10 +1,15 @@
|
||||
import { List, SizeField, useResourceRefresh } from '../common/index'
|
||||
import {
|
||||
List,
|
||||
Pagination,
|
||||
SizeField,
|
||||
getStoredPerPage,
|
||||
useResourceRefresh,
|
||||
} from '../common/index'
|
||||
import {
|
||||
Datagrid,
|
||||
DateField,
|
||||
TextField,
|
||||
downloadCSV,
|
||||
Pagination,
|
||||
Filter,
|
||||
ReferenceInput,
|
||||
useTranslate,
|
||||
@ -49,8 +54,10 @@ const BulkActionButtons = (props) => (
|
||||
</>
|
||||
)
|
||||
|
||||
const missingPerPageOptions = [50, 100, 200]
|
||||
|
||||
const MissingPagination = (props) => (
|
||||
<Pagination rowsPerPageOptions={[50, 100, 200]} {...props} />
|
||||
<Pagination rowsPerPageOptions={missingPerPageOptions} {...props} />
|
||||
)
|
||||
|
||||
const MissingFilesList = (props) => {
|
||||
@ -63,7 +70,7 @@ const MissingFilesList = (props) => {
|
||||
actions={<MissingListActions />}
|
||||
filters={<MissingFilesFilter />}
|
||||
bulkActionButtons={<BulkActionButtons />}
|
||||
perPage={50}
|
||||
perPage={getStoredPerPage('missing', missingPerPageOptions)}
|
||||
pagination={<MissingPagination />}
|
||||
>
|
||||
<Datagrid>
|
||||
|
||||
@ -4,14 +4,21 @@ import {
|
||||
ShowContextProvider,
|
||||
useShowContext,
|
||||
useShowController,
|
||||
Pagination,
|
||||
Title as RaTitle,
|
||||
} from 'react-admin'
|
||||
import { makeStyles } from '@material-ui/core/styles'
|
||||
import PlaylistDetails from './PlaylistDetails'
|
||||
import PlaylistSongs from './PlaylistSongs'
|
||||
import PlaylistActions from './PlaylistActions'
|
||||
import { Title, canChangeTracks, useResourceRefresh } from '../common'
|
||||
import {
|
||||
Pagination,
|
||||
Title,
|
||||
canChangeTracks,
|
||||
getStoredPerPage,
|
||||
useResourceRefresh,
|
||||
} from '../common'
|
||||
|
||||
const playlistTrackPerPageOptions = [100, 250, 500]
|
||||
|
||||
const useStyles = makeStyles(
|
||||
(theme) => ({
|
||||
@ -41,7 +48,10 @@ const PlaylistShowLayout = (props) => {
|
||||
reference="playlistTrack"
|
||||
target="playlist_id"
|
||||
sort={{ field: 'id', order: 'ASC' }}
|
||||
perPage={100}
|
||||
perPage={getStoredPerPage(
|
||||
'playlistTrack',
|
||||
playlistTrackPerPageOptions,
|
||||
)}
|
||||
filter={{ playlist_id: props.id }}
|
||||
>
|
||||
<PlaylistSongs
|
||||
@ -56,7 +66,9 @@ const PlaylistShowLayout = (props) => {
|
||||
}
|
||||
resource={'playlistTrack'}
|
||||
exporter={false}
|
||||
pagination={<Pagination rowsPerPageOptions={[100, 250, 500]} />}
|
||||
pagination={
|
||||
<Pagination rowsPerPageOptions={playlistTrackPerPageOptions} />
|
||||
}
|
||||
/>
|
||||
</ReferenceManyField>
|
||||
)}
|
||||
|
||||
@ -16,6 +16,8 @@ import {
|
||||
} from 'react-admin'
|
||||
import {
|
||||
List,
|
||||
defaultRowsPerPageOptions,
|
||||
getStoredPerPage,
|
||||
useImageUrl,
|
||||
ToggleFieldsMenu,
|
||||
useSelectedFields,
|
||||
@ -135,7 +137,11 @@ const RadioList = ({ permissions, ...props }) => {
|
||||
hasCreate={isAdmin}
|
||||
actions={<RadioListActions isAdmin={isAdmin} />}
|
||||
filters={<RadioFilter />}
|
||||
perPage={isXsmall ? 25 : 10}
|
||||
perPage={getStoredPerPage(
|
||||
'radio',
|
||||
defaultRowsPerPageOptions,
|
||||
isXsmall ? 25 : 10,
|
||||
)}
|
||||
>
|
||||
{isXsmall ? (
|
||||
<SimpleList
|
||||
|
||||
@ -26,6 +26,8 @@ import {
|
||||
useResourceRefresh,
|
||||
ArtistLinkField,
|
||||
PathField,
|
||||
defaultRowsPerPageOptions,
|
||||
getStoredPerPage,
|
||||
} from '../common'
|
||||
import { useDispatch } from 'react-redux'
|
||||
import { makeStyles } from '@material-ui/core/styles'
|
||||
@ -215,7 +217,11 @@ const SongList = (props) => {
|
||||
bulkActionButtons={<SongBulkActions />}
|
||||
actions={<SongListActions />}
|
||||
filters={<SongFilter />}
|
||||
perPage={isXsmall ? 50 : 15}
|
||||
perPage={getStoredPerPage(
|
||||
'song',
|
||||
defaultRowsPerPageOptions,
|
||||
isXsmall ? 50 : 15,
|
||||
)}
|
||||
>
|
||||
{isXsmall ? (
|
||||
<SongSimpleList />
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user