feat(library): toggle playlist membership from track + popover#76
Merged
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (21)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review by Qodo
1. Stale fetch clobbers toggles
|
github-actions
Bot
added
scope: frontend
Open the `+` popover on a track row and rows for playlists that already contain the track now render a green checkmark; clicking such a row removes the track from that playlist instead of re-adding it. Spotify- style toggle, no confirmation modal (action is trivially reversible). Backend - New `list_playlists_containing_track(track_id) -> Vec<i64>` command, excludes smart playlists (rule-driven, not a toggle target). Frontend - TrackTable lazy-fetches membership the first time a track's popover opens, caches it for the table's lifetime, and flips the cached set optimistically on click. - AddToPlaylistPopover gains an optional `memberPlaylistIds` prop to render the checkmark + adjust each row's aria-label. Album / artist / folder popovers omit the prop — they bulk-add and have no symmetric remove semantic. - New i18n keys `trackActions.addToPlaylistNamed` / `removeFromPlaylistNamed` propagated to all 17 locales.
InstaZDLL
force-pushed
the
feat/playlist-popover-toggle-membership
branch
from
ee007e9 to
bcf044c
Compare
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 10 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+1230
to
+1242
| const opening = !isMenuOpen; | ||
| setOpenMenuTrackId(opening ? track.id : null); | ||
| // Lazy-fetch membership the first time this track's | ||
| // popover is opened. Subsequent opens reuse the cached | ||
| // set (kept in sync via optimistic updates on toggle). | ||
| if (opening && !trackMembership.has(track.id)) { | ||
| listPlaylistsContainingTrack(track.id) | ||
| .then((ids) => { | ||
| setTrackMembership((prev) => { | ||
| const next = new Map(prev); | ||
| next.set(track.id, new Set(ids)); | ||
| return next; | ||
| }); |
There was a problem hiding this comment.
1. Stale fetch clobbers toggles 🐞 Bug ≡ Correctness
In TrackTable, listPlaylistsContainingTrack() resolves asynchronously and unconditionally overwrites trackMembership for the track, so a late response can erase optimistic add/remove flips (or race with multiple in-flight fetches) and render incorrect checkmarks/toggle behavior.
Agent Prompt
## Issue description
`listPlaylistsContainingTrack(track.id)` writes `next.set(track.id, new Set(ids))` when it resolves, regardless of whether the user already toggled membership for that track while the request was in flight. This can clobber optimistic updates and/or produce an incorrect cached membership set.
## Issue Context
The race is possible whenever the user opens the `+` popover and clicks a playlist before the membership fetch returns, or when the popover is opened/closed/reopened quickly (triggering multiple concurrent fetches).
## Fix Focus Areas
- src/components/views/LibraryView.tsx[1224-1290]
## Implementation direction
- Track per-track in-flight requests (e.g., `membershipFetchVersion: Map<trackId, number>` or `inFlight: Set<trackId>`).
- When a fetch resolves, update state only if it’s still the latest request for that track.
- Merge fetched IDs with any optimistic mutations that occurred during the request (e.g., record pending ops per track while loading, then apply them to the fetched `Set` before committing).
- Prevent duplicate fetches for the same track while one is already in flight.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1269
to
+1288
| const members = trackMembership.get(track.id); | ||
| const isMember = members?.has(playlistId) ?? false; | ||
| // Optimistic membership flip — the underlying mutations | ||
| // are idempotent on the backend, so a failed RPC just | ||
| // means the visual state will drift until the next | ||
| // popover open, which is the worst-case loss for a | ||
| // single click. | ||
| setTrackMembership((prev) => { | ||
| const next = new Map(prev); | ||
| const set = new Set(next.get(track.id) ?? []); | ||
| if (isMember) set.delete(playlistId); | ||
| else set.add(playlistId); | ||
| next.set(track.id, set); | ||
| return next; | ||
| }); | ||
| if (isMember) { | ||
| void onRemoveFromPlaylist(playlistId, track.id); | ||
| } else { | ||
| void onAddToPlaylist(playlistId, track.id); | ||
| } |
There was a problem hiding this comment.
2. Smart playlists become toggleable 🐞 Bug ≡ Correctness
Although the backend membership API excludes smart playlists, the frontend optimistic membership set can still add smart playlist IDs, causing smart playlists to show checkmarks and later route clicks to removeTrackFromPlaylist, contradicting the intended “non-toggle target” behavior.
Agent Prompt
## Issue description
Smart playlists are excluded server-side from `list_playlists_containing_track`, but the client-side optimistic update adds/removes *any* `playlistId` into `trackMembership`. This allows smart playlists to gain checkmarks and be treated as toggle targets (including calling remove), which contradicts the feature intent.
## Issue Context
- Backend explicitly filters `p.is_smart = 0` for the membership list.
- Frontend uses `memberPlaylistIds` both to render checkmarks and to decide add vs remove.
- Smart playlists are re-materialized by deleting and recreating `playlist_track` rows, so manual toggles are not durable.
## Fix Focus Areas
- src/components/views/LibraryView.tsx[1264-1387]
- src-tauri/src/commands/playlist.rs[419-442]
- src/lib/tauri/playlist.ts[9-34]
## Implementation direction
- Ensure smart playlists never render a membership checkmark: in `AddToPlaylistPopover`, compute `isMember` as `!pl.is_smart && (memberPlaylistIds?.has(pl.id) ?? false)`.
- In `TrackTable.onPick`, look up the playlist by id (from `playlists`) and:
- If `pl.is_smart`, always call the add handler and **do not** mutate `trackMembership` (so smart playlists can’t become “members” in the local Set).
- Otherwise keep current toggle/optimistic behavior.
- Optionally: consider filtering smart playlists out of the list entirely when `memberPlaylistIds` is provided, if the product intent is “not a target at all.”
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
+popover on a track row in the Songs view: playlists that already contain the track render a green checkmark on the right, and clicking such a row removes the track instead of re-adding it (Spotify-style toggle).Backend
list_playlists_containing_track(track_id) -> Vec<i64>command incommands/playlist.rs. Smart playlists are excluded (their membership is rule-driven, not a toggle target).Frontend
TrackTablelazy-fetches membership on first popover open per track and caches it for the table's lifetime. Toggling flips the cached set + dispatchesaddTracksToPlaylistorremoveTrackFromPlaylistaccordingly.AddToPlaylistPopovergains an optionalmemberPlaylistIds: ReadonlySet<number>prop. When provided, matching rows render<Check>and aria-labels switch betweenaddToPlaylistNamed/removeFromPlaylistNamed.Test plan
+popover on a track that's already in one or more playlists → those rows show a green checkmark, others don't.+popover still works as a plain add (no checkmarks).