fix(nav): preserve detail payloads in history + don't toggle off shuffle#24
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughNavigation state moved to a typed HistoryEntry stack in AppLayout; active view and payload ids are derived from the current history entry. Navigation callbacks push/replace history entries. Sidebar now navigates via a ChangesNavigation History Refactor and Shuffle State Awareness
Sequence Diagram(s)sequenceDiagram
participant Sidebar
participant AppLayout
participant DetailView
participant Player
Sidebar->>AppLayout: navigateToPlaylist(playlistId)
AppLayout->>AppLayout: pushEntry({ id: "playlist", playlistId })
AppLayout->>DetailView: derive activePlaylistId from currentEntry
DetailView->>Player: playTracks(...) / check isShuffled
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Comment |
github-actions
Bot
added
scope: frontend
… shuffle
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.
InstaZDLL
force-pushed
the
fix/detail-views-nav-and-shuffle
branch
from
03024c0 to
445cd4e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/layout/AppLayout.tsx`:
- Around line 108-122: History entries for the Library view don't include the
selected tab, so HistoryEntry currently has only { id: "library" } while
libraryTab is stored separately; update HistoryEntry to include the selected tab
(e.g., { id: "library"; tab?: "albums" | "artists" | "tracks" | ... }) and
change all places that push { id: "library" } to instead push the entry with the
current tab, then implement a dedicated library-navigation callback (e.g.,
deriveLibraryTabFromEntry or onNavigateLibrary) that reads the tab from the
current HistoryEntry and sets libraryTab accordingly when
navigating/back/forward; update usages of HistoryEntry, navigation push calls,
and the component state updater that currently sets libraryTab externally to
always derive it from the active HistoryEntry.
- Around line 362-365: The onAfterDelete handler passed into PlaylistView
currently appends a new "home" history entry via setActiveView("home"), leaving
the deleted playlist as a back-target; instead replace/prune the current history
entry so Back doesn't return to the deleted playlist. Change the onAfterDelete
implementation to perform a history replace (e.g. use your router's
navigate(..., { replace: true }) or window.history.replaceState(...) to set the
view to home) and then update local state (setActiveView) as needed; update the
PlaylistView onAfterDelete usage to call the replace-based navigation rather
than pushing a new entry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7bfd9e52-076a-41a4-a02f-dd803a418945
📒 Files selected for processing (5)
src/components/layout/AppLayout.tsxsrc/components/layout/Sidebar.tsxsrc/components/views/AlbumDetailView.tsxsrc/components/views/ArtistDetailView.tsxsrc/components/views/GenreDetailView.tsx
… 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.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/layout/AppLayout.tsx`:
- Around line 235-255: pushEntry and replaceEntry capture stale historyIndex
causing lost entries or out-of-bounds reads; change the state shape to a single
object (e.g. { viewHistory, historyIndex }) and update both values atomically in
one functional setter so the updater has current index and history together;
replace setViewHistory/setHistoryIndex uses in pushEntry and replaceEntry with a
single setHistoryState(prev => { compute newHistory =
[...prev.viewHistory.slice(0, prev.historyIndex + 1), entry] (or for replace,
copy and set newHistory[prev.historyIndex]=entry); return { viewHistory:
newHistory, historyIndex: prev.historyIndex + (push? 1 : 0) } }); update all
references to viewHistory and historyIndex to read from the combined state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 35298a9a-10b0-4bc1-b5aa-16b166517786
📒 Files selected for processing (1)
src/components/layout/AppLayout.tsx
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses two pre-existing UX bugs surfaced by CodeRabbit on #23 — both apply to album / artist / genre detail views since they share the same pattern. Targets
feat/genre-detail-viewso it merges in lockstep with #23.1. Nav back/forward lost the target id
viewHistorystored onlyViewIdstrings whileactiveAlbumId/activeArtistId/activeGenreId/activePlaylistId/activeWrappedYearlived in paralleluseState. Sequence:activeAlbumId = AactiveAlbumId = Balbum-detailbutactiveAlbumIdis still B, not AFix: store
HistoryEntry { id, ...payload }inviewHistory, derive everyactive*Idfrom the current entry. Drops the parallel state and lets back/forward restore the exact target.2. Shuffle button toggled instead of enabling
The detail-view
handleShufflePlaycalledtoggleShuffle()unconditionally. If shuffle was already on, clicking "Shuffle" disabled it and played the queue in order. Fix: gate withif (!isShuffled)in all three detail views.Collateral
Sidebar'ssetActivePlaylistIdprop becomesnavigateToPlaylist, removing the call sites that paired two setters in sequence.Test plan
bun run typecheck✅Summary by CodeRabbit
Refactor
Bug Fixes