From 50633f839dabe49571099977753036d166734982 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Tue, 4 Aug 2026 22:07:16 -0400 Subject: [PATCH 1/6] fix(ui): restore each page's scroll position when navigating back (#5892) * 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. * feat(ui): remember each route's scroll offset A hook that saves window.scrollY per history entry and, on navigation, tops a pushed route or returns a popped one to where it was. Keyed on location.key so back and forward each restore their own offset, and bounded because history entries are not. Callers pass their own readiness: restoring before the rows render leaves the document too short, so the browser clamps to the top and the offset is lost with no error to show for it. * refactor(ui): fold scroll-to-top into route scroll restoration useScrollToTop only ever forced the top, so it fixed opening a detail page from a scrolled list and did nothing for the return trip: coming back to the list landed wherever it happened to be. The new hook covers both, and its readiness argument carries over the reason the old one keyed on the record id - waiting for the record rather than firing against an empty page. * feat(ui): restore the album list's position when returning to it The list is the page worth remembering: scrolling a long way to find an album and landing back at the top is the annoyance the detail-page scroll fix could not reach. Mounted as a pass-through around the List's child rather than beside it, because React Admin calls Children.only on List children and clones that one child with the list props - which is also how this reads loaded/total to wait for rows. * fix(ui): key scroll restoration on the route, not location.key Hash history never assigns location.key - history.js calls createLocation with an undefined key and warns that it cannot carry state - so every route collapsed into one slot. The detail page's own scroll listener then overwrote the list's saved offset with 0, and going back restored that 0. Only the scroll-to-top half worked, which is what made it look fine. Keyed on pathname+search instead. The spec that would have caught this asserts two routes keep separate offsets; the previous ones mocked the router and so only ever confirmed the assumption. * refactor(ui): tidy up scroll restoration after review Four cleanups, none changing behaviour: ScrollRestorer read loaded/total off whatever props React Admin happened to clone in. useListContext is the contract for those values and is already used three times elsewhere, including further down this same file; the clone payload is an implementation detail that would fail silently if it ever changed. The restore runs in a layout effect now. A passive effect is flushed after paint, so an image-heavy grid was painted at the old offset and then jumped. MAX_ENTRIES matches the naming every other module-level cap in common/ uses. The per-instance guard against re-restoring was deletable with a green suite, so it now has a spec: readiness flickering on one route must not scroll twice. * fix(ui): resolve media queries on the first render MUI defaults useMediaQuery and withWidth to SSR-safe two-pass rendering: the first render reports the wrong breakpoint and a layout effect corrects it. Navidrome is client-only, so every mount paints a layout it immediately reflows. Album tiles render without their artist line and grow 14px when it arrives, and react-admin's own buttons paint labelled before collapsing to icons, which together grew the album list 79px right after a scroll restore and dragged the position with it. Setting noSsr on the theme reaches react-admin's internals too. withWidth needs its own option because it gates on a mount flag rather than on the media query. * fix(ui): keep restoring the scroll offset until it sticks A single scrollTo assumes the page is already at its final height. It often is not: the artist page reports ready as soon as its cached header record renders, while the albums below are still loading, so the browser clamps the offset to the top of a viewport-tall document and it is lost with no error. The restore now retries until the offset lands, yielding as soon as the position moves somewhere we did not put it. Keying that on input events instead breaks the trackpad back-swipe, whose wheel momentum keeps firing through the restore window. Offsets are also committed when leaving the page rather than on every scroll. Tearing the page down collapses the document and snaps to the top, and a live listener recorded that clamp over the offset being left behind. * refactor(ui): tidy up scroll restoration after review Folds the offset commit into the scroll tracker's own cleanup now that the tracker ignores the collapse snap, so one effect does what two did. Skips scheduling the retry loop when the first scrollTo already landed, and only pays for the scrollHeight read on scroll events that reach the top, since that is the only case the guard can fire. The theme hook's own prefers-color-scheme query runs above the ThemeProvider carrying the new prop, so it needs noSsr passed directly or the auto theme still renders dark first and flips. Also lifts the repeated fake-timer fixture in the specs into one helper and restores real timers from afterEach, so a failing assertion no longer leaves them installed for every later spec. * fix(ui): bank the scroll offset before the next page tops itself Committing the offset from the scroll listener's passive cleanup ran too late. The incoming page scrolls to the top in its own layout effect, and the outgoing page's listener is still attached at that moment, so it recorded that 0 over the offset being left behind. Whether it did depended on when the browser dispatched the scroll event relative to React's passive flush, which made returning to a detail page lose its position intermittently. Moving the commit back into a layout cleanup makes it deterministic: React runs those in the mutation phase, before the incoming page's layout effects. The scrollHeight guard in the listener went with it, since it only covered the case where the incoming page was shorter than the viewport and the ordering covers all of them. --- ui/src/album/AlbumList.jsx | 21 +- ui/src/album/AlbumShow.jsx | 3 +- ui/src/artist/ArtistShow.jsx | 8 +- ui/src/common/ArtistLinkField.jsx | 3 +- ui/src/common/index.js | 1 + ui/src/common/useScrollRestoration.jsx | 64 ++++++ ui/src/common/useScrollRestoration.test.jsx | 222 ++++++++++++++++++++ ui/src/themes/useCurrentTheme.js | 18 +- 8 files changed, 329 insertions(+), 11 deletions(-) create mode 100644 ui/src/common/useScrollRestoration.jsx create mode 100644 ui/src/common/useScrollRestoration.test.jsx 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/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 From d764a1e9d77d255a79a13eac1ef039957051252c Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 6 Aug 2026 00:40:58 -0400 Subject: [PATCH 2/6] fix(log): change debug log level to trace to reduce log noise from cron Signed-off-by: Deluan --- scheduler/log_adapter.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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) { From dce48ff650f0f7ba30d7f4113fc443751d4c93ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Thu, 6 Aug 2026 10:59:50 -0400 Subject: [PATCH 3/6] feat(smartplaylist): add album-level fields for sorting and filtering (#5899) Smart playlists could only sort by the track-level `dateadded`, which scatters an album's tracks because every track has its own timestamp. There was no way to express "newest albums first, tracks in album order". Adds five fields backed by the album table: albumdateadded, albumdatemodified, albumduration, albumsongcount and albumsize, so `"sort": "-albumdateadded, tracknumber"` now works. They filter as well as sort, which also enables rules like "everything from albums added this month" or "skip singles and EPs". These need the album table rather than the album_annotation table the existing album* fields join, so they get their own bit in the join mask. The bitmask already unions joins from both the expression and the sort fields, so a sort-only reference pulls in the join for the main query while correctly staying out of the percentage-limit count query. No COALESCE default is used. That mechanism exists because an album_annotation row is genuinely often absent; the album row always exists and song_count, duration and size are NOT NULL, so leaving the columns bare keeps filters index-friendly. albumdatemodified follows the existing track-level `datemodified` naming rather than the `albumdateupdated` spelling used in the request. albumreleasedate was considered and rejected: album.release_date is allOrNothing() over the tracks, so it equals the track-level releasedate when they agree and collapses to empty when they disagree. Closes https://github.com/navidrome/navidrome/discussions/5347 --- model/criteria/fields.go | 5 ++ persistence/criteria_sql.go | 6 +++ persistence/criteria_sql_test.go | 48 +++++++++++++++++++ persistence/e2e/smartplaylist_test.go | 37 ++++++++++++++ persistence/smart_playlist_repository.go | 14 +++--- persistence/smart_playlist_repository_test.go | 35 ++++++++++++++ 6 files changed, 139 insertions(+), 6 deletions(-) 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 From 0f4c9b82121f870e5c925657458a9b87912f59ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Fri, 7 Aug 2026 09:36:52 -0400 Subject: [PATCH 4/6] fix(ui): don't start playback when closing the disc cover lightbox (#5901) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Lightbox renders through a React portal (react-modal), and React propagates synthetic events up the React tree rather than the DOM tree, so clicking its close button or backdrop bubbled into the ancestor TableRow and triggered playSubset. The existing timestamp guard could not catch this: react-image-lightbox defers onCloseRequest by animationDuration via setTimeout, so lightboxClosedAt was only stamped 200ms after the click had already propagated. The guard is kept because it still covers the separate mobile ghost-click path, where the synthesized click lands on the row after the overlay unmounts. Stopping propagation in onCloseRequest — the approach used by ContextMenus, whose MUI onClose fires synchronously — does not work for the same reason. Instead, wrap the Lightbox in an element that stops propagation at the React tree boundary the portal bubbles through, which also covers the lightbox controls we don't own (zoom buttons, caption, image drag). --- ui/src/common/SongDatagrid.jsx | 20 ++++---- ui/src/common/SongDatagrid.test.jsx | 74 +++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 8 deletions(-) create mode 100644 ui/src/common/SongDatagrid.test.jsx 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() + }) +}) From a0bf78cdeab4d0f316edcd37094f03d09f0ede8e Mon Sep 17 00:00:00 2001 From: ant Date: Sat, 8 Aug 2026 12:40:05 -0700 Subject: [PATCH 5/6] fix(contrib): added missing hyphen in OpenRC script that caused crashes on startup (#5906) Signed-off-by: Ant --- contrib/navidrome | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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" From 5a5311a9c0ac0147fb49ed990f4cbb4f6c7ac0a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Sat, 8 Aug 2026 21:57:44 -0400 Subject: [PATCH 6/6] fix(plugins): make generated mock stubs nil-safe for nilable returns (#5909) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ndpgen mock accessors asserted return types unconditionally, so a plugin test using Return(nil, ...) — the natural way to model an empty result or a pagination terminal page — panicked on the untyped-nil type assertion instead of returning the zero value. Guard pointer, slice, map and any returns in both the host-service client stubs and the PDK stub, and regenerate. --- plugins/cmd/ndpgen/internal/generator.go | 62 ++++++++---- plugins/cmd/ndpgen/internal/generator_test.go | 96 ++++++++++++++++++- .../internal/templates/client_stub.go.tmpl | 2 +- .../internal/templates/pdk_stub.go.tmpl | 2 +- plugins/pdk/go/host/mock_nil_returns_test.go | 27 ++++++ plugins/pdk/go/host/nd_host_cache_stub.go | 6 +- plugins/pdk/go/host/nd_host_config_stub.go | 6 +- plugins/pdk/go/host/nd_host_http_stub.go | 6 +- plugins/pdk/go/host/nd_host_kvstore_stub.go | 18 +++- plugins/pdk/go/host/nd_host_library_stub.go | 12 ++- plugins/pdk/go/host/nd_host_matcher_stub.go | 6 +- .../pdk/go/host/nd_host_subsonicapi_stub.go | 6 +- plugins/pdk/go/host/nd_host_task_stub.go | 6 +- plugins/pdk/go/host/nd_host_users_stub.go | 12 ++- plugins/pdk/go/pdk/pdk_stub.go | 24 ++++- 15 files changed, 250 insertions(+), 41 deletions(-) create mode 100644 plugins/pdk/go/host/mock_nil_returns_test.go diff --git a/plugins/cmd/ndpgen/internal/generator.go b/plugins/cmd/ndpgen/internal/generator.go index 50f53cd37..bfc1b7c4e 100644 --- a/plugins/cmd/ndpgen/internal/generator.go +++ b/plugins/cmd/ndpgen/internal/generator.go @@ -27,32 +27,46 @@ func hostFuncMap(svc Service) template.FuncMap { // Uses private (lowercase) type names for request/response structs. func clientFuncMap(svc Service) template.FuncMap { return template.FuncMap{ - "lower": strings.ToLower, - "title": strings.Title, - "exportName": func(m Method) string { return m.FunctionName(svc.ExportPrefix()) }, - "requestType": func(m Method) string { return m.ClientRequestTypeName(svc.Name) }, - "responseType": func(m Method) string { return m.ClientResponseTypeName(svc.Name) }, - "formatDoc": formatDoc, - "mockReturnValues": mockReturnValues, + "lower": strings.ToLower, + "title": strings.Title, + "exportName": func(m Method) string { return m.FunctionName(svc.ExportPrefix()) }, + "requestType": func(m Method) string { return m.ClientRequestTypeName(svc.Name) }, + "responseType": func(m Method) string { return m.ClientResponseTypeName(svc.Name) }, + "formatDoc": formatDoc, + "mockReturnBody": mockReturnBody, } } -// mockReturnValues generates the testify mock return value accessors for a method. -// For example: args.String(0), args.Bool(1), args.Error(2) -func mockReturnValues(m Method) string { +// mockReturnBody generates the testify mock body lines that extract return values. +// Nil-able returns are guarded so tests can use Return(nil, ...) without the +// type-assertion panic testify produces on an untyped nil. +func mockReturnBody(m Method) string { + var b strings.Builder var parts []string - idx := 0 - for _, r := range m.Returns { - parts = append(parts, mockAccessor(r.Type, idx)) - idx++ + for idx, r := range m.Returns { + if isNilableType(r.Type) { + name := fmt.Sprintf("r%d", idx) + fmt.Fprintf(&b, "\tvar %s %s\n\tif v := args.Get(%d); v != nil {\n\t\t%s = v.(%s)\n\t}\n", name, r.Type, idx, name, r.Type) + parts = append(parts, name) + } else { + parts = append(parts, mockAccessor(r.Type, idx)) + } } if m.HasError { - parts = append(parts, fmt.Sprintf("args.Error(%d)", idx)) + parts = append(parts, fmt.Sprintf("args.Error(%d)", len(m.Returns))) } - return strings.Join(parts, ", ") + b.WriteString("\treturn " + strings.Join(parts, ", ")) + return b.String() +} + +// isNilableType reports whether a nil mock return value is a valid intent for the +// type, rather than a malformed test expectation. +func isNilableType(typ string) bool { + return strings.HasPrefix(typ, "*") || strings.HasPrefix(typ, "[]") || + strings.HasPrefix(typ, "map[") || typ == "any" || typ == "interface{}" } // mockAccessor returns the testify mock accessor call for a given type and index. @@ -725,7 +739,7 @@ func pdkFuncMap() template.FuncMap { "returnList": pdkReturnList, "argList": pdkArgList, "argListWithReceiver": pdkArgListWithReceiver, - "mockReturns": pdkMockReturns, + "mockReturnBody": pdkMockReturnBody, "constValue": pdkConstValue, "stubTypeUnderlying": stubTypeUnderlying, "methodReceiver": pdkMethodReceiver, @@ -834,12 +848,20 @@ func pdkMethodReceiver(receiver, typeName string) string { } // pdkMockReturns generates the mock return accessors for a function. -func pdkMockReturns(returns []PDKReturn) string { +func pdkMockReturnBody(returns []PDKReturn) string { + var b strings.Builder var parts []string for i, r := range returns { - parts = append(parts, mockAccessorForType(r.Type, i)) + if isNilableType(r.Type) { + name := fmt.Sprintf("r%d", i) + fmt.Fprintf(&b, "\tvar %s %s\n\tif v := args.Get(%d); v != nil {\n\t\t%s = v.(%s)\n\t}\n", name, r.Type, i, name, r.Type) + parts = append(parts, name) + } else { + parts = append(parts, mockAccessorForType(r.Type, i)) + } } - return strings.Join(parts, ", ") + b.WriteString("\treturn " + strings.Join(parts, ", ")) + return b.String() } // mockAccessorForType returns the testify mock accessor for a type. diff --git a/plugins/cmd/ndpgen/internal/generator_test.go b/plugins/cmd/ndpgen/internal/generator_test.go index 15b97e1ae..ea94c7fc6 100644 --- a/plugins/cmd/ndpgen/internal/generator_test.go +++ b/plugins/cmd/ndpgen/internal/generator_test.go @@ -748,8 +748,9 @@ var _ = Describe("Generator", func() { // Check bool return uses args.Bool(1) Expect(codeStr).To(ContainSubstring("args.Bool(1)")) - // Check []byte return uses args.Get(0).([]byte) - Expect(codeStr).To(ContainSubstring("args.Get(0).([]byte)")) + // []byte is nil-able, so it gets a guarded extraction + Expect(codeStr).To(ContainSubstring("var r0 []byte")) + Expect(codeStr).To(ContainSubstring("r0 = v.([]byte)")) // Check error returns use args.Error(N) Expect(codeStr).To(ContainSubstring("args.Error(")) @@ -1699,6 +1700,97 @@ var _ = Describe("Rust Generation", func() { Expect(codeStr).NotTo(ContainSubstring("use base64")) }) }) + + Describe("nil-safe mock accessors", func() { + Describe("GenerateClientGoStub", func() { + It("guards pointer, slice and map returns so Return(nil, ...) does not panic", func() { + svc := Service{ + Name: "Paged", + Interface: "PagedService", + Methods: []Method{ + { + Name: "GetPage", + HasError: true, + Params: []Param{NewParam("query", "string")}, + Returns: []Param{NewParam("items", "[]Item"), NewParam("next", "*PageOptions")}, + }, + { + Name: "GetLabels", + HasError: true, + Params: []Param{NewParam("id", "string")}, + Returns: []Param{NewParam("labels", "map[string]string")}, + }, + }, + } + + code, err := GenerateClientGoStub(svc, "host") + Expect(err).NotTo(HaveOccurred()) + _, err = format.Source(code) + Expect(err).NotTo(HaveOccurred()) + + codeStr := string(code) + Expect(codeStr).To(ContainSubstring("var r0 []Item")) + Expect(codeStr).To(ContainSubstring("var r1 *PageOptions")) + Expect(codeStr).To(ContainSubstring("if v := args.Get(1); v != nil {")) + Expect(codeStr).To(ContainSubstring("r1 = v.(*PageOptions)")) + Expect(codeStr).To(ContainSubstring("return r0, r1, args.Error(2)")) + Expect(codeStr).To(ContainSubstring("var r0 map[string]string")) + Expect(codeStr).NotTo(ContainSubstring("args.Get(0).([]Item)")) + Expect(codeStr).NotTo(ContainSubstring("args.Get(1).(*PageOptions)")) + }) + + It("keeps non-nilable returns as inline accessors", func() { + svc := Service{ + Name: "Counter", + Interface: "CounterService", + Methods: []Method{ + { + Name: "Count", + HasError: true, + Params: []Param{NewParam("id", "string")}, + Returns: []Param{NewParam("count", "int64"), NewParam("name", "string")}, + }, + }, + } + + code, err := GenerateClientGoStub(svc, "host") + Expect(err).NotTo(HaveOccurred()) + + codeStr := string(code) + Expect(codeStr).To(ContainSubstring("return args.Get(0).(int64), args.String(1), args.Error(2)")) + Expect(codeStr).NotTo(ContainSubstring("var r0")) + }) + }) + + Describe("GeneratePDKGoStub", func() { + It("guards pointer and slice returns", func() { + symbols := &PDKSymbols{ + Functions: []PDKFunc{ + { + Name: "NewHTTPRequest", + Params: []PDKParam{{Name: "method", Type: "HTTPMethod"}, {Name: "url", Type: "string"}}, + Returns: []PDKReturn{{Type: "*HTTPRequest"}}, + }, + { + Name: "Input", + Returns: []PDKReturn{{Type: "[]byte"}}, + }, + }, + } + + code, err := GeneratePDKGoStub(symbols) + Expect(err).NotTo(HaveOccurred()) + _, err = format.Source(code) + Expect(err).NotTo(HaveOccurred()) + + codeStr := string(code) + Expect(codeStr).To(ContainSubstring("var r0 *HTTPRequest")) + Expect(codeStr).To(ContainSubstring("r0 = v.(*HTTPRequest)")) + Expect(codeStr).To(ContainSubstring("var r0 []byte")) + Expect(codeStr).NotTo(ContainSubstring("return args.Get(0).(*HTTPRequest)")) + }) + }) + }) }) func writeFile(path, content string) error { diff --git a/plugins/cmd/ndpgen/internal/templates/client_stub.go.tmpl b/plugins/cmd/ndpgen/internal/templates/client_stub.go.tmpl index d2a8f3f27..eb5e9f844 100644 --- a/plugins/cmd/ndpgen/internal/templates/client_stub.go.tmpl +++ b/plugins/cmd/ndpgen/internal/templates/client_stub.go.tmpl @@ -51,7 +51,7 @@ var {{.Service.Name}}Mock = &mock{{.Service.Name}}Service{} // {{.Name}} is the mock method for {{$.Service.Name}}{{.Name}}. func (m *mock{{$.Service.Name}}Service) {{.Name}}({{range $i, $p := .Params}}{{if $i}}, {{end}}{{$p.Name}} {{$p.Type}}{{end}}) {{.ReturnSignature}} { args := m.Called({{range $i, $p := .Params}}{{if $i}}, {{end}}{{$p.Name}}{{end}}) - return {{mockReturnValues .}} +{{mockReturnBody .}} } // {{$.Service.Name}}{{.Name}} delegates to the mock instance. diff --git a/plugins/cmd/ndpgen/internal/templates/pdk_stub.go.tmpl b/plugins/cmd/ndpgen/internal/templates/pdk_stub.go.tmpl index 6a57a6469..2bd73f107 100644 --- a/plugins/cmd/ndpgen/internal/templates/pdk_stub.go.tmpl +++ b/plugins/cmd/ndpgen/internal/templates/pdk_stub.go.tmpl @@ -34,7 +34,7 @@ func ResetMock() { func {{.Name}}({{paramList .Params}}){{returnList .Returns}} { {{- if .Returns}} args := PDKMock.Called({{argList .Params}}) - return {{mockReturns .Returns}} +{{mockReturnBody .Returns}} {{- else}} PDKMock.Called({{argList .Params}}) {{- end}} diff --git a/plugins/pdk/go/host/mock_nil_returns_test.go b/plugins/pdk/go/host/mock_nil_returns_test.go new file mode 100644 index 000000000..ea5f334f1 --- /dev/null +++ b/plugins/pdk/go/host/mock_nil_returns_test.go @@ -0,0 +1,27 @@ +//go:build !wasip1 + +package host + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +// Return(nil, ...) models "no result"; the generated accessors must yield the +// zero value instead of panicking on the type assertion. +func TestMockNilPointerReturn(t *testing.T) { + HTTPMock.On("Send", HTTPRequest{URL: "http://nil.example"}).Return(nil, nil) + + resp, err := HTTPSend(HTTPRequest{URL: "http://nil.example"}) + require.NoError(t, err) + require.Nil(t, resp) +} + +func TestMockNilSliceReturn(t *testing.T) { + LibraryMock.On("GetAllLibraries").Return(nil, nil) + + libs, err := LibraryGetAllLibraries() + require.NoError(t, err) + require.Nil(t, libs) +} diff --git a/plugins/pdk/go/host/nd_host_cache_stub.go b/plugins/pdk/go/host/nd_host_cache_stub.go index 46bb44bcf..faf4e65d2 100644 --- a/plugins/pdk/go/host/nd_host_cache_stub.go +++ b/plugins/pdk/go/host/nd_host_cache_stub.go @@ -154,7 +154,11 @@ func CacheSetBytes(key string, value []byte, ttlSeconds int64) error { // GetBytes is the mock method for CacheGetBytes. func (m *mockCacheService) GetBytes(key string) ([]byte, bool, error) { args := m.Called(key) - return args.Get(0).([]byte), args.Bool(1), args.Error(2) + var r0 []byte + if v := args.Get(0); v != nil { + r0 = v.([]byte) + } + return r0, args.Bool(1), args.Error(2) } // CacheGetBytes delegates to the mock instance. diff --git a/plugins/pdk/go/host/nd_host_config_stub.go b/plugins/pdk/go/host/nd_host_config_stub.go index 463c29b76..0327346a3 100644 --- a/plugins/pdk/go/host/nd_host_config_stub.go +++ b/plugins/pdk/go/host/nd_host_config_stub.go @@ -59,7 +59,11 @@ func ConfigGetInt(key string) (int64, bool) { // Keys is the mock method for ConfigKeys. func (m *mockConfigService) Keys(prefix string) []string { args := m.Called(prefix) - return args.Get(0).([]string) + var r0 []string + if v := args.Get(0); v != nil { + r0 = v.([]string) + } + return r0 } // ConfigKeys delegates to the mock instance. diff --git a/plugins/pdk/go/host/nd_host_http_stub.go b/plugins/pdk/go/host/nd_host_http_stub.go index 09c4d0fcf..8706ac9e5 100644 --- a/plugins/pdk/go/host/nd_host_http_stub.go +++ b/plugins/pdk/go/host/nd_host_http_stub.go @@ -43,7 +43,11 @@ var HTTPMock = &mockHTTPService{} // Send is the mock method for HTTPSend. func (m *mockHTTPService) Send(request HTTPRequest) (*HTTPResponse, error) { args := m.Called(request) - return args.Get(0).(*HTTPResponse), args.Error(1) + var r0 *HTTPResponse + if v := args.Get(0); v != nil { + r0 = v.(*HTTPResponse) + } + return r0, args.Error(1) } // HTTPSend delegates to the mock instance. diff --git a/plugins/pdk/go/host/nd_host_kvstore_stub.go b/plugins/pdk/go/host/nd_host_kvstore_stub.go index fce038aa1..e5243860a 100644 --- a/plugins/pdk/go/host/nd_host_kvstore_stub.go +++ b/plugins/pdk/go/host/nd_host_kvstore_stub.go @@ -64,7 +64,11 @@ func KVStoreSetWithTTL(key string, value []byte, ttlSeconds int64) error { // Get is the mock method for KVStoreGet. func (m *mockKVStoreService) Get(key string) ([]byte, bool, error) { args := m.Called(key) - return args.Get(0).([]byte), args.Bool(1), args.Error(2) + var r0 []byte + if v := args.Get(0); v != nil { + r0 = v.([]byte) + } + return r0, args.Bool(1), args.Error(2) } // KVStoreGet delegates to the mock instance. @@ -81,7 +85,11 @@ func KVStoreGet(key string) ([]byte, bool, error) { // GetMany is the mock method for KVStoreGetMany. func (m *mockKVStoreService) GetMany(keys []string) (map[string][]byte, error) { args := m.Called(keys) - return args.Get(0).(map[string][]byte), args.Error(1) + var r0 map[string][]byte + if v := args.Get(0); v != nil { + r0 = v.(map[string][]byte) + } + return r0, args.Error(1) } // KVStoreGetMany delegates to the mock instance. @@ -116,7 +124,11 @@ func KVStoreHas(key string) (bool, error) { // List is the mock method for KVStoreList. func (m *mockKVStoreService) List(prefix string) ([]string, error) { args := m.Called(prefix) - return args.Get(0).([]string), args.Error(1) + var r0 []string + if v := args.Get(0); v != nil { + r0 = v.([]string) + } + return r0, args.Error(1) } // KVStoreList delegates to the mock instance. diff --git a/plugins/pdk/go/host/nd_host_library_stub.go b/plugins/pdk/go/host/nd_host_library_stub.go index 1e4c230c3..0232c49d9 100644 --- a/plugins/pdk/go/host/nd_host_library_stub.go +++ b/plugins/pdk/go/host/nd_host_library_stub.go @@ -39,7 +39,11 @@ var LibraryMock = &mockLibraryService{} // GetLibrary is the mock method for LibraryGetLibrary. func (m *mockLibraryService) GetLibrary(id int32) (*Library, error) { args := m.Called(id) - return args.Get(0).(*Library), args.Error(1) + var r0 *Library + if v := args.Get(0); v != nil { + r0 = v.(*Library) + } + return r0, args.Error(1) } // LibraryGetLibrary delegates to the mock instance. @@ -56,7 +60,11 @@ func LibraryGetLibrary(id int32) (*Library, error) { // GetAllLibraries is the mock method for LibraryGetAllLibraries. func (m *mockLibraryService) GetAllLibraries() ([]Library, error) { args := m.Called() - return args.Get(0).([]Library), args.Error(1) + var r0 []Library + if v := args.Get(0); v != nil { + r0 = v.([]Library) + } + return r0, args.Error(1) } // LibraryGetAllLibraries delegates to the mock instance. diff --git a/plugins/pdk/go/host/nd_host_matcher_stub.go b/plugins/pdk/go/host/nd_host_matcher_stub.go index 07b34bb82..ade368c06 100644 --- a/plugins/pdk/go/host/nd_host_matcher_stub.go +++ b/plugins/pdk/go/host/nd_host_matcher_stub.go @@ -31,7 +31,11 @@ var MatcherMock = &mockMatcherService{} // MatchSongs is the mock method for MatcherMatchSongs. func (m *mockMatcherService) MatchSongs(songs []types.SongRef, opts MatchOptions) ([]*types.Track, error) { args := m.Called(songs, opts) - return args.Get(0).([]*types.Track), args.Error(1) + var r0 []*types.Track + if v := args.Get(0); v != nil { + r0 = v.([]*types.Track) + } + return r0, args.Error(1) } // MatcherMatchSongs delegates to the mock instance. diff --git a/plugins/pdk/go/host/nd_host_subsonicapi_stub.go b/plugins/pdk/go/host/nd_host_subsonicapi_stub.go index 6d3a56b35..ef3d1b735 100644 --- a/plugins/pdk/go/host/nd_host_subsonicapi_stub.go +++ b/plugins/pdk/go/host/nd_host_subsonicapi_stub.go @@ -39,7 +39,11 @@ func SubsonicAPICall(uri string) (string, error) { // CallRaw is the mock method for SubsonicAPICallRaw. func (m *mockSubsonicAPIService) CallRaw(uri string) (string, []byte, error) { args := m.Called(uri) - return args.String(0), args.Get(1).([]byte), args.Error(2) + var r1 []byte + if v := args.Get(1); v != nil { + r1 = v.([]byte) + } + return args.String(0), r1, args.Error(2) } // SubsonicAPICallRaw delegates to the mock instance. diff --git a/plugins/pdk/go/host/nd_host_task_stub.go b/plugins/pdk/go/host/nd_host_task_stub.go index 286d14035..f2c45a2b5 100644 --- a/plugins/pdk/go/host/nd_host_task_stub.go +++ b/plugins/pdk/go/host/nd_host_task_stub.go @@ -70,7 +70,11 @@ func TaskEnqueue(queueName string, payload []byte) (string, error) { // Get is the mock method for TaskGet. func (m *mockTaskService) Get(taskID string) (*TaskInfo, error) { args := m.Called(taskID) - return args.Get(0).(*TaskInfo), args.Error(1) + var r0 *TaskInfo + if v := args.Get(0); v != nil { + r0 = v.(*TaskInfo) + } + return r0, args.Error(1) } // TaskGet delegates to the mock instance. diff --git a/plugins/pdk/go/host/nd_host_users_stub.go b/plugins/pdk/go/host/nd_host_users_stub.go index 8858b2109..e0f8c0939 100644 --- a/plugins/pdk/go/host/nd_host_users_stub.go +++ b/plugins/pdk/go/host/nd_host_users_stub.go @@ -33,7 +33,11 @@ var UsersMock = &mockUsersService{} // GetUsers is the mock method for UsersGetUsers. func (m *mockUsersService) GetUsers() ([]User, error) { args := m.Called() - return args.Get(0).([]User), args.Error(1) + var r0 []User + if v := args.Get(0); v != nil { + r0 = v.([]User) + } + return r0, args.Error(1) } // UsersGetUsers delegates to the mock instance. @@ -49,7 +53,11 @@ func UsersGetUsers() ([]User, error) { // GetAdmins is the mock method for UsersGetAdmins. func (m *mockUsersService) GetAdmins() ([]User, error) { args := m.Called() - return args.Get(0).([]User), args.Error(1) + var r0 []User + if v := args.Get(0); v != nil { + r0 = v.([]User) + } + return r0, args.Error(1) } // UsersGetAdmins delegates to the mock instance. diff --git a/plugins/pdk/go/pdk/pdk_stub.go b/plugins/pdk/go/pdk/pdk_stub.go index 3bdbb1cb7..8f2707d56 100644 --- a/plugins/pdk/go/pdk/pdk_stub.go +++ b/plugins/pdk/go/pdk/pdk_stub.go @@ -62,7 +62,11 @@ func GetConfig(key string) (string, bool) { // GetVar GetVar returns the byte slice (if any) associated with `key`. func GetVar(key string) []byte { args := PDKMock.Called(key) - return args.Get(0).([]byte) + var r0 []byte + if v := args.Get(0); v != nil { + r0 = v.([]byte) + } + return r0 } // GetVarInt GetVarInt returns the int associated with `key` (or 0 if none). @@ -74,7 +78,11 @@ func GetVarInt(key string) int { // Input Input returns a slice of bytes from the host. func Input() []byte { args := PDKMock.Called() - return args.Get(0).([]byte) + var r0 []byte + if v := args.Get(0); v != nil { + r0 = v.([]byte) + } + return r0 } // InputJSON InputJSON returns unmartialed JSON data from the host "input". @@ -108,7 +116,11 @@ func LogMemory(level LogLevel, m Memory) { // NewHTTPRequest NewHTTPRequest returns a new `HTTPRequest`. func NewHTTPRequest(method HTTPMethod, url string) *HTTPRequest { args := PDKMock.Called(method, url) - return args.Get(0).(*HTTPRequest) + var r0 *HTTPRequest + if v := args.Get(0); v != nil { + r0 = v.(*HTTPRequest) + } + return r0 } func NewMemory(offset uint64, length uint64) Memory { args := PDKMock.Called(offset, length) @@ -139,7 +151,11 @@ func OutputString(s string) { // ParamBytes ParamBytes returns bytes from Extism host memory given an offset. func ParamBytes(offset uint64) []byte { args := PDKMock.Called(offset) - return args.Get(0).([]byte) + var r0 []byte + if v := args.Get(0); v != nil { + r0 = v.([]byte) + } + return r0 } // ParamString ParamString returns UTF-8 string data from Extism host memory given an offset.