From 445cd4eefaaf84f0caea80695c1e9978fc3e16ac Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Fri, 15 May 2026 22:57:55 +0200 Subject: [PATCH 1/3] fix(nav): preserve detail payloads in view history + don't toggle off shuffle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit two pre-existing UX bugs in the detail views, surfaced by CodeRabbit on the genre-detail PR but identical for album / artist / genre: 1. nav back/forward lost the target id. viewHistory only stored ViewId strings, with activeAlbumId / activeArtistId / activeGenreId / activePlaylistId / activeWrappedYear held in parallel useState. visiting album A → home → album B → back → back landed on "album-detail" with activeAlbumId still pointing at B. fix: store HistoryEntry { id, payload } in viewHistory and derive every active*Id from the current entry. drops the parallel state and lets back/forward restore the exact target. 2. shuffle button toggled instead of enabling. handleShufflePlay called toggleShuffle() unconditionally, so clicking "Shuffle" while shuffle was already on disabled it and played the queue in order. fix: gate the toggle with `if (!isShuffled)` in all three detail views. Sidebar's setActivePlaylistId prop becomes navigateToPlaylist, removing the call sites that used to call two setters in sequence. --- src/components/layout/AppLayout.tsx | 107 ++++++++++++++-------- src/components/layout/Sidebar.tsx | 15 ++- src/components/views/AlbumDetailView.tsx | 7 +- src/components/views/ArtistDetailView.tsx | 7 +- src/components/views/GenreDetailView.tsx | 7 +- 5 files changed, 91 insertions(+), 52 deletions(-) diff --git a/src/components/layout/AppLayout.tsx b/src/components/layout/AppLayout.tsx index 26b7782..c46568f 100644 --- a/src/components/layout/AppLayout.tsx +++ b/src/components/layout/AppLayout.tsx @@ -100,6 +100,27 @@ const GenreDetailView = lazy(() => })), ); +// Each entry in the navigation history pairs a view id with its payload +// (when relevant) so back/forward can restore the exact target the user +// visited. Payload fields are optional so callers without a target (e.g. +// the initial "home" entry, or navigating to "wrapped" without a year) +// stay valid. +type HistoryEntry = + | { id: "home" } + | { id: "library" } + | { id: "settings" } + | { id: "spotify" } + | { id: "about" } + | { id: "feedback" } + | { id: "statistics" } + | { id: "liked" } + | { id: "recent" } + | { id: "wrapped"; year?: number | null } + | { id: "playlist"; playlistId?: number | null } + | { id: "album-detail"; albumId?: number | null } + | { id: "artist-detail"; artistId?: number | null } + | { id: "genre-detail"; genreId?: number | null }; + export function AppLayout() { const { t } = useTranslation(); const { isDark } = useTheme(); @@ -109,22 +130,17 @@ export function AppLayout() { // listener and re-reads bindings whenever Settings emits the // shortcuts-changed event. useGlobalShortcuts(); - const [viewHistory, setViewHistory] = useState(["home"]); + // History entries carry their payload (album/artist/genre/playlist id, + // wrapped year) directly so back/forward restore the exact target the + // user visited — not whatever target was set most recently. Without + // this, navigating album A → home → album B → back → back lands on + // "album-detail" with activeAlbumId still pointing at B. + const [viewHistory, setViewHistory] = useState([ + { id: "home" }, + ]); const [historyIndex, setHistoryIndex] = useState(0); const [isProfileModalOpen, setIsProfileModalOpen] = useState(false); const [libraryTab, setLibraryTab] = useState("morceaux"); - // Currently focused playlist for the "playlist" view. The view itself - // re-fetches when this id changes; the sidebar uses it to highlight - // the active row. - const [activePlaylistId, setActivePlaylistId] = useState(null); - const [activeAlbumId, setActiveAlbumId] = useState(null); - const [activeArtistId, setActiveArtistId] = useState(null); - const [activeGenreId, setActiveGenreId] = useState(null); - // Year requested when navigating into the Wrapped overlay. `null` - // tells WrappedView to pick the most recent year with plays. - const [activeWrappedYear, setActiveWrappedYear] = useState( - null, - ); // First-run onboarding: prompt the user to point WaveFlow at a // music folder when no library has been populated yet. @@ -199,16 +215,41 @@ export function AppLayout() { ); }, []); - const activeView = viewHistory[historyIndex]; + const currentEntry = viewHistory[historyIndex]; + const activeView: ViewId = currentEntry.id; + // Derived from the current history entry so back/forward restore the + // correct payload. `null` for views without a payload. + const activeAlbumId = + currentEntry.id === "album-detail" ? (currentEntry.albumId ?? null) : null; + const activeArtistId = + currentEntry.id === "artist-detail" + ? (currentEntry.artistId ?? null) + : null; + const activeGenreId = + currentEntry.id === "genre-detail" ? (currentEntry.genreId ?? null) : null; + const activePlaylistId = + currentEntry.id === "playlist" ? (currentEntry.playlistId ?? null) : null; + const activeWrappedYear = + currentEntry.id === "wrapped" ? (currentEntry.year ?? null) : null; - const setActiveView = useCallback( - (view: ViewId) => { - setViewHistory((prev) => [...prev.slice(0, historyIndex + 1), view]); + const pushEntry = useCallback( + (entry: HistoryEntry) => { + setViewHistory((prev) => [...prev.slice(0, historyIndex + 1), entry]); setHistoryIndex((prev) => prev + 1); }, [historyIndex], ); + // Wrapper used by views that only need a plain id (Home, Settings, …). + // The cast is safe because every `ViewId` matches a HistoryEntry whose + // payload fields are optional. + const setActiveView = useCallback( + (view: ViewId) => { + pushEntry({ id: view } as HistoryEntry); + }, + [pushEntry], + ); + const canGoBack = historyIndex > 0; const canGoForward = historyIndex < viewHistory.length - 1; @@ -222,42 +263,37 @@ export function AppLayout() { const navigateToAlbum = useCallback( (albumId: number) => { - setActiveAlbumId(albumId); - setActiveView("album-detail"); + pushEntry({ id: "album-detail", albumId }); }, - [setActiveView], + [pushEntry], ); const navigateToArtist = useCallback( (artistId: number) => { - setActiveArtistId(artistId); - setActiveView("artist-detail"); + pushEntry({ id: "artist-detail", artistId }); }, - [setActiveView], + [pushEntry], ); const navigateToGenre = useCallback( (genreId: number) => { - setActiveGenreId(genreId); - setActiveView("genre-detail"); + pushEntry({ id: "genre-detail", genreId }); }, - [setActiveView], + [pushEntry], ); const navigateToPlaylist = useCallback( (playlistId: number) => { - setActivePlaylistId(playlistId); - setActiveView("playlist"); + pushEntry({ id: "playlist", playlistId }); }, - [setActiveView], + [pushEntry], ); const navigateToWrapped = useCallback( (year: number | null) => { - setActiveWrappedYear(year); - setActiveView("wrapped"); + pushEntry({ id: "wrapped", year }); }, - [setActiveView], + [pushEntry], ); function renderView() { @@ -325,10 +361,7 @@ export function AppLayout() { return ( { - setActivePlaylistId(null); - setActiveView("home"); - }} + onAfterDelete={() => setActiveView("home")} onNavigateToAlbum={navigateToAlbum} onNavigateToArtist={navigateToArtist} /> @@ -396,7 +429,7 @@ export function AppLayout() { libraryTab={libraryTab} setLibraryTab={setLibraryTab} activePlaylistId={activePlaylistId} - setActivePlaylistId={setActivePlaylistId} + navigateToPlaylist={navigateToPlaylist} /> {/* Center Content. `min-w-0` is required so a long playlist diff --git a/src/components/layout/Sidebar.tsx b/src/components/layout/Sidebar.tsx index 2989249..c50a8ca 100644 --- a/src/components/layout/Sidebar.tsx +++ b/src/components/layout/Sidebar.tsx @@ -39,7 +39,7 @@ interface SidebarProps { libraryTab: LibraryTab; setLibraryTab: (tab: LibraryTab) => void; activePlaylistId: number | null; - setActivePlaylistId: (id: number | null) => void; + navigateToPlaylist: (id: number) => void; } export function Sidebar({ @@ -48,7 +48,7 @@ export function Sidebar({ libraryTab, setLibraryTab, activePlaylistId, - setActivePlaylistId, + navigateToPlaylist, }: SidebarProps) { const { t } = useTranslation(); const { activeProfile } = useProfile(); @@ -162,16 +162,14 @@ export function Sidebar({ color_id: data.colorId, icon_id: data.iconId, }); - setActivePlaylistId(created.id); - setActiveView("playlist"); + navigateToPlaylist(created.id); } catch (err) { console.error("[Sidebar] failed to create playlist", err); } }; const handleSelectPlaylist = (playlistId: number) => { - setActivePlaylistId(playlistId); - setActiveView("playlist"); + navigateToPlaylist(playlistId); }; const handleImportM3u = useCallback(async () => { @@ -183,8 +181,7 @@ export function Sidebar({ try { const result = await importPlaylistM3u(path); await refreshPlaylists(); - setActivePlaylistId(result.playlist_id); - setActiveView("playlist"); + navigateToPlaylist(result.playlist_id); if (result.missing > 0) { console.warn( `[Sidebar] m3u import: ${result.missing} entries not found in library`, @@ -194,7 +191,7 @@ export function Sidebar({ } catch (err) { console.error("[Sidebar] import m3u failed", err); } - }, [t, refreshPlaylists, setActivePlaylistId, setActiveView]); + }, [t, refreshPlaylists, navigateToPlaylist]); const isPlaylistRowActive = (id: number) => activeView === "playlist" && activePlaylistId === id; diff --git a/src/components/views/AlbumDetailView.tsx b/src/components/views/AlbumDetailView.tsx index 3bd73fd..cdc2413 100644 --- a/src/components/views/AlbumDetailView.tsx +++ b/src/components/views/AlbumDetailView.tsx @@ -39,7 +39,8 @@ export function AlbumDetailView({ onNavigateToArtist, }: AlbumDetailViewProps) { const { t } = useTranslation(); - const { playTracks, currentTrack, toggleShuffle, isPlaying } = usePlayer(); + const { playTracks, currentTrack, toggleShuffle, isShuffled, isPlaying } = + usePlayer(); const { createPlaylist } = usePlaylist(); const [album, setAlbum] = useState(null); @@ -191,7 +192,9 @@ export function AlbumDetailView({ const handleShufflePlay = async () => { if (playableTracks.length === 0) return; await playTracks(playableTracks, 0, { type: "library", id: null }); - await toggleShuffle(); + // Gate the toggle so we never *disable* shuffle when the user + // explicitly clicks the Shuffle button. + if (!isShuffled) await toggleShuffle(); }; const label = enrichedLabel ?? album.label; diff --git a/src/components/views/ArtistDetailView.tsx b/src/components/views/ArtistDetailView.tsx index 43463fe..0d13f68 100644 --- a/src/components/views/ArtistDetailView.tsx +++ b/src/components/views/ArtistDetailView.tsx @@ -41,7 +41,8 @@ export function ArtistDetailView({ onNavigateToArtist, }: ArtistDetailViewProps) { const { t } = useTranslation(); - const { playTracks, currentTrack, toggleShuffle, isPlaying } = usePlayer(); + const { playTracks, currentTrack, toggleShuffle, isShuffled, isPlaying } = + usePlayer(); const { createPlaylist } = usePlaylist(); const [artist, setArtist] = useState(null); @@ -227,7 +228,9 @@ export function ArtistDetailView({ const handleShufflePlay = async () => { if (tracks.length === 0) return; await playTracks(tracks, 0, { type: "library", id: null }); - await toggleShuffle(); + // Gate the toggle so we never *disable* shuffle when the user + // explicitly clicks the Shuffle button. + if (!isShuffled) await toggleShuffle(); }; const fansLabel = diff --git a/src/components/views/GenreDetailView.tsx b/src/components/views/GenreDetailView.tsx index 87ad9a9..75915a1 100644 --- a/src/components/views/GenreDetailView.tsx +++ b/src/components/views/GenreDetailView.tsx @@ -31,7 +31,8 @@ export function GenreDetailView({ onNavigateToArtist, }: GenreDetailViewProps) { const { t } = useTranslation(); - const { playTracks, currentTrack, toggleShuffle, isPlaying } = usePlayer(); + const { playTracks, currentTrack, toggleShuffle, isShuffled, isPlaying } = + usePlayer(); const { createPlaylist } = usePlaylist(); const [genre, setGenre] = useState(null); @@ -120,7 +121,9 @@ export function GenreDetailView({ const handleShufflePlay = async () => { if (tracks.length === 0) return; await playTracks(tracks, 0, { type: "library", id: null }); - await toggleShuffle(); + // Gate the toggle so we never *disable* shuffle when the user + // explicitly clicks the Shuffle button. + if (!isShuffled) await toggleShuffle(); }; return ( From afd356c8a38d178911d0db5e4fc1818177346ea9 Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Fri, 15 May 2026 23:34:02 +0200 Subject: [PATCH 2/3] fix(nav): replace history entry on playlist delete instead of pushing home deleting a playlist while viewing it called setActiveView("home") which pushed a new entry on top of the deleted playlist. back navigated to the ghost. add replaceEntry() that swaps the current entry in place and use it from onAfterDelete so the deleted playlist falls out of history. --- src/components/layout/AppLayout.tsx | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/components/layout/AppLayout.tsx b/src/components/layout/AppLayout.tsx index c46568f..2db20b0 100644 --- a/src/components/layout/AppLayout.tsx +++ b/src/components/layout/AppLayout.tsx @@ -240,6 +240,20 @@ export function AppLayout() { [historyIndex], ); + // Replace the current entry in place (no index bump). Used when the + // current target no longer exists — e.g. a playlist that was just + // deleted — so Back doesn't return to a ghost page. + const replaceEntry = useCallback( + (entry: HistoryEntry) => { + setViewHistory((prev) => { + const next = [...prev]; + next[historyIndex] = entry; + return next; + }); + }, + [historyIndex], + ); + // Wrapper used by views that only need a plain id (Home, Settings, …). // The cast is safe because every `ViewId` matches a HistoryEntry whose // payload fields are optional. @@ -361,7 +375,7 @@ export function AppLayout() { return ( setActiveView("home")} + onAfterDelete={() => replaceEntry({ id: "home" })} onNavigateToAlbum={navigateToAlbum} onNavigateToArtist={navigateToArtist} /> From ad447063020a574842cab85a71850a47f671ab26 Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Fri, 15 May 2026 23:39:58 +0200 Subject: [PATCH 3/3] fix(nav): merge viewHistory + index into one atomic state object MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit splitting the history array and its index across two useState calls let rapid navigations queue setters that all read the same stale index from their closure, so the second push slices to the same position as the first (losing the first entry) while setHistoryIndex still advances by one — leaving the index past history.length - 1. collapse both into a single { history, index } state and update them in one functional setter so push / replace / goBack / goForward always see the current pair. --- src/components/layout/AppLayout.tsx | 60 +++++++++++++++++------------ 1 file changed, 35 insertions(+), 25 deletions(-) diff --git a/src/components/layout/AppLayout.tsx b/src/components/layout/AppLayout.tsx index 2db20b0..84cba87 100644 --- a/src/components/layout/AppLayout.tsx +++ b/src/components/layout/AppLayout.tsx @@ -135,10 +135,18 @@ export function AppLayout() { // user visited — not whatever target was set most recently. Without // this, navigating album A → home → album B → back → back lands on // "album-detail" with activeAlbumId still pointing at B. - const [viewHistory, setViewHistory] = useState([ - { id: "home" }, - ]); - const [historyIndex, setHistoryIndex] = useState(0); + // + // History + index live in a single state object so push/replace can + // update both atomically inside one functional setter. Splitting them + // would let rapid back-to-back navigations queue setters that all read + // the same stale index, losing entries and leaving `index` past + // `history.length - 1`. + const [navState, setNavState] = useState<{ + history: HistoryEntry[]; + index: number; + }>({ history: [{ id: "home" }], index: 0 }); + const viewHistory = navState.history; + const historyIndex = navState.index; const [isProfileModalOpen, setIsProfileModalOpen] = useState(false); const [libraryTab, setLibraryTab] = useState("morceaux"); @@ -232,27 +240,23 @@ export function AppLayout() { const activeWrappedYear = currentEntry.id === "wrapped" ? (currentEntry.year ?? null) : null; - const pushEntry = useCallback( - (entry: HistoryEntry) => { - setViewHistory((prev) => [...prev.slice(0, historyIndex + 1), entry]); - setHistoryIndex((prev) => prev + 1); - }, - [historyIndex], - ); + const pushEntry = useCallback((entry: HistoryEntry) => { + setNavState(({ history, index }) => ({ + history: [...history.slice(0, index + 1), entry], + index: index + 1, + })); + }, []); // Replace the current entry in place (no index bump). Used when the // current target no longer exists — e.g. a playlist that was just // deleted — so Back doesn't return to a ghost page. - const replaceEntry = useCallback( - (entry: HistoryEntry) => { - setViewHistory((prev) => { - const next = [...prev]; - next[historyIndex] = entry; - return next; - }); - }, - [historyIndex], - ); + const replaceEntry = useCallback((entry: HistoryEntry) => { + setNavState(({ history, index }) => { + const next = [...history]; + next[index] = entry; + return { history: next, index }; + }); + }, []); // Wrapper used by views that only need a plain id (Home, Settings, …). // The cast is safe because every `ViewId` matches a HistoryEntry whose @@ -268,12 +272,18 @@ export function AppLayout() { const canGoForward = historyIndex < viewHistory.length - 1; const goBack = useCallback(() => { - if (canGoBack) setHistoryIndex((i) => i - 1); - }, [canGoBack]); + setNavState(({ history, index }) => + index > 0 ? { history, index: index - 1 } : { history, index }, + ); + }, []); const goForward = useCallback(() => { - if (canGoForward) setHistoryIndex((i) => i + 1); - }, [canGoForward]); + setNavState(({ history, index }) => + index < history.length - 1 + ? { history, index: index + 1 } + : { history, index }, + ); + }, []); const navigateToAlbum = useCallback( (albumId: number) => {