feat: added recently songs section and minor fix: no multiple votes per song#10
feat: added recently songs section and minor fix: no multiple votes per song#10ShauryaGarg17 wants to merge 2 commits intoACM-VIT:mainfrom
Conversation
|
@ShauryaGarg17 is attempting to deploy a commit to the ACM Deployments Team on Vercel. A member of the Team first needs to authorize it. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d035b70728
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
convex/playedSongs.ts
Outdated
| await ctx.db.insert("songs", { | ||
| roomId: args.roomId, | ||
| provider: playedSong.provider, | ||
| providerId: playedSong.providerId, | ||
| title: playedSong.title, |
There was a problem hiding this comment.
The new readdSong mutation inserts a song into songs but never updates rooms.currentSongId when the queue was previously empty. In the common case where playback has finished and currentSongId is unset, re-adding a track leaves the room stuck with no active song; the player does not resume, and later addSong calls also won’t auto-start because the queue length is already greater than 1. This should mirror songs.addSong by assigning currentSongId when this insert creates the first queued track.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR adds a "Recently Played" songs feature to display the last 20 played songs per room, along with the ability to re-add them to the queue. It also implements frontend double-click prevention for voting operations. The PR includes significant code formatting changes that don't affect functionality.
Changes:
- Added
playedSongstable schema with indexed queries by room and played timestamp - Modified
advanceSongmutation to save finished songs to history before deletion - Created new
listRecentlyPlayedquery andreaddSongmutation for the recently played feature - Added collapsible "Recently Played" UI section with re-add functionality and voting double-click prevention
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| convex/schema.ts | Adds new playedSongs table definition with appropriate indexes |
| convex/rooms.ts | Updates advanceSong to save songs to history; formatting improvements |
| convex/playedSongs.ts | New file implementing recently played queries and re-add song mutation |
| app/room/[code]/page.tsx | Adds recently played UI section, voting double-click prevention, and extensive formatting changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| addedBy: v.string(), | ||
| addedByName: v.optional(v.string()), | ||
| playedAt: v.number(), | ||
| }).index("by_room_played", ["roomId", "playedAt"]), |
There was a problem hiding this comment.
The playedSongs table will grow indefinitely without any cleanup mechanism. Consider implementing a retention policy or cleanup job similar to the reactions table cleanup pattern (see convex/reactions.ts:35-36). For example, you could periodically delete played songs older than 30 days, or limit the history to a maximum number of entries per room to prevent unbounded growth.
| }).index("by_room_played", ["roomId", "playedAt"]), | |
| }) | |
| .index("by_room_played", ["roomId", "playedAt"]) | |
| .index("by_playedAt", ["playedAt"]), |
| // Check if song is already in the queue | ||
| const existing = await ctx.db | ||
| .query("songs") | ||
| .withIndex("by_room_provider", (q) => | ||
| q | ||
| .eq("roomId", args.roomId) | ||
| .eq("provider", playedSong.provider) | ||
| .eq("providerId", playedSong.providerId), | ||
| ) | ||
| .unique(); | ||
|
|
||
| if (existing) { | ||
| throw new Error("This song is already in the queue."); | ||
| } |
There was a problem hiding this comment.
The duplicate check only verifies if the song exists in the queue, but doesn't check if it's currently playing. A user could re-add a song that is currently playing (stored in room.currentSongId), which would result in a duplicate. Consider checking if the song matches the currently playing song as well.
| // Add the song back to the queue | ||
| await ctx.db.insert("songs", { | ||
| roomId: args.roomId, | ||
| provider: playedSong.provider, | ||
| providerId: playedSong.providerId, | ||
| title: playedSong.title, | ||
| artist: playedSong.artist, | ||
| albumArtUrl: playedSong.albumArtUrl, | ||
| addedBy: args.userId, | ||
| addedByName: args.userName, | ||
| addedAt: now, | ||
| score: 0, | ||
| lastScoreUpdatedAt: now, | ||
| }); |
There was a problem hiding this comment.
The readdSong mutation doesn't check if the queue is empty and set the added song as currentSongId if needed. In convex/songs.ts:72-79, the addSong mutation checks if the queue has only one song after insertion and sets it as currentSongId. The readdSong mutation should have similar logic to ensure the first song starts playing automatically.
| artist: playedSong.artist, | ||
| albumArtUrl: playedSong.albumArtUrl, | ||
| addedBy: args.userId, | ||
| addedByName: args.userName, |
There was a problem hiding this comment.
The addedByName field is set to args.userName which can be undefined, whereas in convex/songs.ts:67, addSong sets it to "Anonymous" as a default. This inconsistency could lead to undefined values in the database. Consider applying the same default value pattern for consistency.
| addedByName: args.userName, | |
| addedByName: args.userName ?? "Anonymous", |
| {recentlyPlayed.slice(0, 10).map((song) => ( | ||
| <div | ||
| key={song._id} | ||
| className="group flex items-center gap-3 rounded-xl p-2.5 hover:bg-white/[0.04] border border-transparent hover:border-white/[0.06] transition-all duration-200" | ||
| > | ||
| <div className="h-10 w-14 overflow-hidden rounded-lg bg-white/[0.04] shrink-0 ring-1 ring-white/[0.06]"> | ||
| {song.providerId && ( | ||
| <img | ||
| src={`https://img.youtube.com/vi/${song.providerId}/default.jpg`} | ||
| alt="" | ||
| className="h-full w-full object-cover" | ||
| /> | ||
| )} | ||
| </div> | ||
| <div className="flex-1 min-w-0"> | ||
| <p className="text-sm font-medium text-white/70 truncate"> | ||
| {song.title} | ||
| </p> | ||
| <p className="text-[11px] text-white/25 truncate"> | ||
| {song.artist || "Unknown artist"} | ||
| </p> | ||
| </div> | ||
| <button | ||
| type="button" | ||
| onClick={() => handleReaddSong(song._id)} | ||
| disabled={ | ||
| !canAdd || | ||
| !userId || | ||
| readdingInProgress.has(song._id) | ||
| } | ||
| className="shrink-0 h-7 px-3 rounded-lg text-[11px] font-bold bg-accent/80 hover:bg-accent text-black border border-accent/50 disabled:opacity-30 disabled:cursor-not-allowed transition-all duration-200" | ||
| > | ||
| {readdingInProgress.has(song._id) | ||
| ? "..." | ||
| : atSongLimit | ||
| ? "Limit" | ||
| : "+ Re-add"} | ||
| </button> | ||
| </div> | ||
| ))} |
There was a problem hiding this comment.
This code assumes all played songs are YouTube videos by hardcoding the YouTube thumbnail URL. However, the schema supports custom providers (provider can be "youtube" or "custom"). This will break thumbnail display for custom songs or lead to 404 errors. Consider checking the provider field and only showing YouTube thumbnails for YouTube songs, or using the albumArtUrl field if available.
| {recentlyPlayed.slice(0, 10).map((song) => ( | |
| <div | |
| key={song._id} | |
| className="group flex items-center gap-3 rounded-xl p-2.5 hover:bg-white/[0.04] border border-transparent hover:border-white/[0.06] transition-all duration-200" | |
| > | |
| <div className="h-10 w-14 overflow-hidden rounded-lg bg-white/[0.04] shrink-0 ring-1 ring-white/[0.06]"> | |
| {song.providerId && ( | |
| <img | |
| src={`https://img.youtube.com/vi/${song.providerId}/default.jpg`} | |
| alt="" | |
| className="h-full w-full object-cover" | |
| /> | |
| )} | |
| </div> | |
| <div className="flex-1 min-w-0"> | |
| <p className="text-sm font-medium text-white/70 truncate"> | |
| {song.title} | |
| </p> | |
| <p className="text-[11px] text-white/25 truncate"> | |
| {song.artist || "Unknown artist"} | |
| </p> | |
| </div> | |
| <button | |
| type="button" | |
| onClick={() => handleReaddSong(song._id)} | |
| disabled={ | |
| !canAdd || | |
| !userId || | |
| readdingInProgress.has(song._id) | |
| } | |
| className="shrink-0 h-7 px-3 rounded-lg text-[11px] font-bold bg-accent/80 hover:bg-accent text-black border border-accent/50 disabled:opacity-30 disabled:cursor-not-allowed transition-all duration-200" | |
| > | |
| {readdingInProgress.has(song._id) | |
| ? "..." | |
| : atSongLimit | |
| ? "Limit" | |
| : "+ Re-add"} | |
| </button> | |
| </div> | |
| ))} | |
| {recentlyPlayed.slice(0, 10).map((song) => { | |
| const thumbnailUrl = | |
| song.albumArtUrl || | |
| (song.provider === "youtube" && song.providerId | |
| ? `https://img.youtube.com/vi/${song.providerId}/default.jpg` | |
| : undefined); | |
| return ( | |
| <div | |
| key={song._id} | |
| className="group flex items-center gap-3 rounded-xl p-2.5 hover:bg-white/[0.04] border border-transparent hover:border-white/[0.06] transition-all duration-200" | |
| > | |
| <div className="h-10 w-14 overflow-hidden rounded-lg bg-white/[0.04] shrink-0 ring-1 ring-white/[0.06]"> | |
| {thumbnailUrl && ( | |
| <img | |
| src={thumbnailUrl} | |
| alt="" | |
| className="h-full w-full object-cover" | |
| /> | |
| )} | |
| </div> | |
| <div className="flex-1 min-w-0"> | |
| <p className="text-sm font-medium text-white/70 truncate"> | |
| {song.title} | |
| </p> | |
| <p className="text-[11px] text-white/25 truncate"> | |
| {song.artist || "Unknown artist"} | |
| </p> | |
| </div> | |
| <button | |
| type="button" | |
| onClick={() => handleReaddSong(song._id)} | |
| disabled={ | |
| !canAdd || | |
| !userId || | |
| readdingInProgress.has(song._id) | |
| } | |
| className="shrink-0 h-7 px-3 rounded-lg text-[11px] font-bold bg-accent/80 hover:bg-accent text-black border border-accent/50 disabled:opacity-30 disabled:cursor-not-allowed transition-all duration-200" | |
| > | |
| {readdingInProgress.has(song._id) | |
| ? "..." | |
| : atSongLimit | |
| ? "Limit" | |
| : "+ Re-add"} | |
| </button> | |
| </div> | |
| ); | |
| })} |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Cleanup: remove old played songs beyond the retention limit | ||
| const playedSongs = await ctx.db | ||
| .query("playedSongs") | ||
| .withIndex("by_room_played", (q) => q.eq("roomId", args.roomId)) | ||
| .order("desc") | ||
| .collect(); | ||
| if (playedSongs.length > MAX_PLAYED_SONGS_PER_ROOM) { | ||
| const toDelete = playedSongs.slice(MAX_PLAYED_SONGS_PER_ROOM); | ||
| for (const old of toDelete) { | ||
| await ctx.db.delete(old._id); | ||
| } | ||
| } |
There was a problem hiding this comment.
The cleanup logic has a potential performance issue. The query fetches all played songs and then slices them in memory. For rooms that have had many songs played (up to the limit of 100), this means fetching more data than necessary. Consider using a more efficient approach: query the played songs in ascending order by playedAt, skip the first (length - MAX_PLAYED_SONGS_PER_ROOM) records, and delete only those that need to be removed. Alternatively, calculate the count first, then fetch only the records to delete.
| {song.providerId && ( | ||
| <img | ||
| src={`https://img.youtube.com/vi/${song.providerId}/default.jpg`} | ||
| alt="" | ||
| className="h-full w-full object-cover" | ||
| /> |
There was a problem hiding this comment.
The thumbnail URL construction assumes all played songs are from YouTube. However, the playedSongs table supports both "youtube" and "custom" providers. For custom songs, this will display a broken image. Consider checking the provider field and either using the albumArtUrl field for custom songs or showing a placeholder icon when provider is "custom".
| {song.providerId && ( | |
| <img | |
| src={`https://img.youtube.com/vi/${song.providerId}/default.jpg`} | |
| alt="" | |
| className="h-full w-full object-cover" | |
| /> | |
| {song.provider === "youtube" && song.providerId ? ( | |
| <img | |
| src={`https://img.youtube.com/vi/${song.providerId}/default.jpg`} | |
| alt="" | |
| className="h-full w-full object-cover" | |
| /> | |
| ) : song.provider === "custom" && song.albumArtUrl ? ( | |
| <img | |
| src={song.albumArtUrl} | |
| alt="" | |
| className="h-full w-full object-cover" | |
| /> | |
| ) : ( | |
| <div className="h-full w-full flex items-center justify-center text-[10px] text-white/30 bg-white/[0.02]"> | |
| No art | |
| </div> |
| {readdingInProgress.has(song._id) | ||
| ? "..." | ||
| : atSongLimit | ||
| ? "Limit" | ||
| : "+ Re-add"} |
There was a problem hiding this comment.
The button text logic could be clearer. When a user is at the song limit, the button shows "Limit" even while a re-add operation is in progress. Consider changing the order of conditions to check readdingInProgress.has(song._id) first, so users see the loading indicator ("...") during the operation, regardless of whether they're at the limit.
| const played = await ctx.db | ||
| .query("playedSongs") | ||
| .withIndex("by_room_played", (q) => q.eq("roomId", args.roomId)) | ||
| .order("desc") | ||
| .take(20); |
There was a problem hiding this comment.
There's an inconsistency between the query limit and the display limit. The listRecentlyPlayed query returns up to 20 songs, but the UI only displays the first 10. Consider either adjusting the query to take(10) to avoid fetching unused data, or displaying all 20 songs that are fetched. This would improve efficiency and reduce unnecessary data transfer.
2 participants
No description provided.