Personal Songs (Plan 3)#2
Merged
Merged
Conversation
Replace single composite unique indexes (broken for NULLable band_id/user_id due to SQLite NULL semantics) with two partial unique indexes per table: idx_annotation_user/idx_annotation_band on SongAnnotation and idx_practice_user_day/idx_practice_band_day on PracticeEvent, each guarded by a WHERE IS NOT NULL clause so duplicates are correctly rejected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reorder imports to match goimports canonical grouping (produced by just fmt). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Restructure UpdateSong to stage-validate all fields (title, artist, status, notes) before writing any, preventing partial saves on mixed valid/invalid payloads. Split CreateSong validation messages for blank title vs oversized artist. Add regression test asserting no partial identity write when status is invalid. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Enforce that UpdateResource and DeleteResource verify the resource belongs to the song in the URL path parameter. Updated repository signatures to accept songID and filter by it, preventing access to resources from different songs. Added regression test to verify mismatched song URLs return 404. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Map genuine DB failures (ErrRecordNotFound) through error wrapping in ReorderFolders and SetFolderEntries repository methods, so handlers can distinguish not-found errors (400) from DB errors (500). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 40 minutes and 9 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR implements a complete personal song library feature for BandWidth, adding songs with practice tracking, folder-based organization, linked resources, and full-stack support including drag-and-drop folder reordering. ChangesPersonal Song Library Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 `@frontend/src/components/folders/FolderSidebar.tsx`:
- Around line 123-130: The create handler can still trigger duplicate mutations
on very fast clicks; add the same double-submit guard pattern used by the rename
form: create a ref (e.g., createInFlightRef) in the FolderSidebar component,
check if createInFlightRef.current is true at the top of the create(e:
FormEvent) function and return early if so, set createInFlightRef.current = true
immediately before calling createFolder.mutate, and then clear it (set to false)
in the mutation callbacks (onSuccess and onError) so the guard resets after the
request completes; keep the existing createFolder.isPending disabled prop as
well for UX feedback.
- Around line 40-50: submitRename currently calls onRename synchronously and
hides the editor and sets submitted.current = true regardless of errors; change
it to handle async errors: make submitRename async, await onRename(name.trim()),
catch any error to display a short UI message/toast (or call a passed error
handler), and in the catch reset submitted.current = false and keep
setEditing(true) so the user can retry; reference submitRename, onRename,
submitted, setEditing, and name to locate and update the logic.
In `@frontend/src/components/songs/ResourceList.tsx`:
- Around line 41-46: The anchor in ResourceList (the <a> element rendering
r.url) uses rel="noreferrer" with target="_blank"; update the rel to include
"noopener" (e.g., "noreferrer noopener") to explicitly prevent the new page from
accessing window.opener. Locate the anchor in ResourceList.tsx (the element with
className "link min-w-0 flex-1 truncate" and href={r.url}) and add noopener to
the rel attribute.
In `@frontend/src/hooks/folders.ts`:
- Around line 57-71: The onSettled invalidation in useSetFolderEntries causes a
refetch even after successful optimistic updates; to align with
useReorderFolders, remove the onSettled handler from useSetFolderEntries so it
only invalidates onError (keep the onError invalidateQueries) and rely on the
optimistic update in onMutate; locate useSetFolderEntries and delete the
onSettled property (and its invalidateQueries call) so behavior matches
useReorderFolders.
In `@frontend/src/pages/HomePage.tsx`:
- Around line 53-59: The practiced flow and undoPractice mutation lack error
handling: update the logPractice.mutate call in practiced and the undoPractice
mutation to include onError handlers that surface a brief error toast to the
user (and optionally log the error) and clear any optimistic undo state via
setUndo as needed; specifically add onError to logPractice.mutate (alongside
existing onSuccess) to show a user-facing message like "Failed to log practice"
and to undoPractice's mutate to show "Failed to undo practice" and keep internal
state consistent so the UI doesn't silently fail.
In `@frontend/src/pages/SongPage.tsx`:
- Around line 95-106: The select's onChange passes e.target.value (a string)
into updateSong.mutate where the mutation expects a SongStatus; cast the value
to SongStatus to satisfy types and document the constraint—update the onChange
handler in SongPage (the select with id="status", value={song.status}, options
from statusOptions) to call updateSong.mutate({ status: e.target.value as
SongStatus }) so the type system knows the value matches the SongStatus union.
- Around line 47-53: The component currently only checks the `song` value and
shows a spinner, so update the `useSong(id)` usage in the SongPage component to
also destructure and handle `isLoading`, `isError`, `error`, and `refetch` (or
equivalent) from the hook; replace the current `if (!song)` branch with logic
that shows the loading spinner when `isLoading`, and when `isError` render a
concise error state (user-facing message using `error.message` if available and
a retry button that calls `refetch`) so network/404/authorization failures don’t
leave users stuck on an infinite spinner.
In `@internal/model/songs.go`:
- Around line 28-36: The Song model currently allows OwnerUserID and OwnerBandID
to be both null or both set; add enforcement so exactly one is set: preferred
fix is a DB-level CHECK constraint (ALTER TABLE / CREATE TABLE migration adding
CHECK ((OwnerUserID IS NULL) != (OwnerBandID IS NULL))) tied to the Song schema;
if DB engine doesn't support CHECK or for safety also add repository-layer
validation in repository.CreateSong (and any update method) to reject
creates/updates where both are null or both non-null, returning a validation
error; update any migrations and tests accordingly and reference the Song struct
and OwnerUserID/OwnerBandID fields and repository.CreateSong in the changes.
In `@internal/repository/folders.go`:
- Around line 47-72: FoldersForUser performs an N+1 by querying FolderEntry per
folder; modify it to preload entries in one query by using GORM Preload on
model.Folder (ensure model.Folder has Entries []FolderEntry with
gorm:"foreignKey:FolderID"), then iterate folders' Entries to build
FolderWithSongs (keep ID/Name/Position and collect SongID from each Entry);
update the function's use of r.db to use r.db.Where("owner_user_id = ?",
userID).Preload("Entries", func(db *gorm.DB) *gorm.DB { return
db.Order("position, id") }).Order("position, id").Find(&folders).Error and
remove the per-folder query loop.
In `@internal/repository/resources.go`:
- Around line 19-36: The CreateResource method currently reads MAX(position)
then inserts maxPos+1 separately, causing race conditions; fix by performing the
read-and-insert inside a DB transaction and lock the relevant rows when
computing MAX(position) (use a SELECT FOR UPDATE / row lock via r.db
transaction/locking clauses on model.Resource filtered by songID and userID) so
concurrent creators serialize and receive unique positions; alternatively add a
unique constraint on (song_id, user_id, position) and implement
retry-on-conflict logic when r.db.Create(res) fails due to constraint
violation—update Repo.CreateResource and model.Resource accordingly.
🪄 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: 77d0c875-5e03-49a2-a013-1d4fd9d1ca82
⛔ Files ignored due to path filters (1)
frontend/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (46)
AGENTS.mdREADME.mdcmd/bandwidth/server.gocmd/bandwidth/server_test.godocs/superpowers/plans/2026-06-11-personal-songs.mdfrontend/package.jsonfrontend/src/App.tsxfrontend/src/components/folders/FolderPicker.test.tsxfrontend/src/components/folders/FolderPicker.tsxfrontend/src/components/folders/FolderSidebar.test.tsxfrontend/src/components/folders/FolderSidebar.tsxfrontend/src/components/folders/SortableSongList.tsxfrontend/src/components/songs/AddSongModal.tsxfrontend/src/components/songs/ConfirmModal.tsxfrontend/src/components/songs/ResourceList.tsxfrontend/src/components/songs/SongRow.tsxfrontend/src/components/songs/StatusBadge.tsxfrontend/src/hooks/folders.tsfrontend/src/hooks/songs.tsfrontend/src/lib/api.tsfrontend/src/lib/dates.test.tsfrontend/src/lib/dates.tsfrontend/src/lib/types.tsfrontend/src/pages/HomePage.test.tsxfrontend/src/pages/HomePage.tsxfrontend/src/pages/SongPage.test.tsxfrontend/src/pages/SongPage.tsxinternal/handlers/folders.gointernal/handlers/folders_test.gointernal/handlers/practice.gointernal/handlers/practice_test.gointernal/handlers/resources.gointernal/handlers/songs.gointernal/handlers/songs_test.gointernal/model/songs.gointernal/repository/folders.gointernal/repository/folders_test.gointernal/repository/passwordresets.gointernal/repository/practice.gointernal/repository/practice_test.gointernal/repository/repository.gointernal/repository/repository_test.gointernal/repository/resources.gointernal/repository/resources_test.gointernal/repository/songs.gointernal/repository/songs_test.go
Comment on lines
+40
to
+50
| const submitRename = (e: FormEvent) => { | ||
| e.preventDefault(); | ||
| if (submitted.current) { | ||
| return; | ||
| } | ||
| submitted.current = true; | ||
| if (name.trim()) { | ||
| onRename(name.trim()); | ||
| } | ||
| setEditing(false); | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Rename errors are silently ignored.
When submitRename calls onRename, any mutation error is not surfaced to the user. The form exits edit mode (line 49) regardless of success or failure, and the submitted flag remains true, preventing immediate retry. Users who experience a failed rename won't receive feedback and must click the rename button again to retry.
This is acceptable for a low-stakes operation like folder renaming, but consider showing a brief error toast or message if you want to improve the UX for network/server errors.
🤖 Prompt for 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.
In `@frontend/src/components/folders/FolderSidebar.tsx` around lines 40 - 50,
submitRename currently calls onRename synchronously and hides the editor and
sets submitted.current = true regardless of errors; change it to handle async
errors: make submitRename async, await onRename(name.trim()), catch any error to
display a short UI message/toast (or call a passed error handler), and in the
catch reset submitted.current = false and keep setEditing(true) so the user can
retry; reference submitRename, onRename, submitted, setEditing, and name to
locate and update the logic.
Comment on lines
+123
to
+130
| const create = (e: FormEvent) => { | ||
| e.preventDefault(); | ||
| if (!newName.trim()) return; | ||
| createFolder.mutate( | ||
| {name: newName.trim()}, | ||
| {onSuccess: () => setNewName('')}, | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider adding double-submit guard to folder creation.
The rename form uses a useRef double-submit guard (line 38-45), but the create folder form relies only on disabled={createFolder.isPending} (line 180). While the button disable provides some protection, two rapid clicks before isPending becomes true could theoretically trigger duplicate mutations. For consistency with the rename pattern, consider adding a useRef guard to the create handler.
🤖 Prompt for 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.
In `@frontend/src/components/folders/FolderSidebar.tsx` around lines 123 - 130,
The create handler can still trigger duplicate mutations on very fast clicks;
add the same double-submit guard pattern used by the rename form: create a ref
(e.g., createInFlightRef) in the FolderSidebar component, check if
createInFlightRef.current is true at the top of the create(e: FormEvent)
function and return early if so, set createInFlightRef.current = true
immediately before calling createFolder.mutate, and then clear it (set to false)
in the mutation callbacks (onSuccess and onError) so the guard resets after the
request completes; keep the existing createFolder.isPending disabled prop as
well for UX feedback.
Comment on lines
+57
to
+71
| export function useSetFolderEntries() { | ||
| const queryClient = useQueryClient(); | ||
| return useMutation<void, ApiError, {id: number; songIds: number[]}>({ | ||
| mutationFn: ({id, songIds}) => | ||
| api.put<void>(`/api/folders/${id}/entries`, {songIds}), | ||
| onMutate: ({id, songIds}) => { | ||
| queryClient.setQueryData<Folder[] | undefined>(['folders'], folders => | ||
| folders?.map(f => (f.id === id ? {...f, songIds} : f)), | ||
| ); | ||
| }, | ||
| onError: () => void queryClient.invalidateQueries({queryKey: ['folders']}), | ||
| onSettled: () => | ||
| void queryClient.invalidateQueries({queryKey: ['folders']}), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider aligning invalidation strategy with useReorderFolders for consistency.
useSetFolderEntries invalidates on both onError and onSettled (line 67-69), causing a refetch even on successful mutations. In contrast, useReorderFolders only invalidates on error (line 53), trusting the optimistic update as the final state on success. Since both mutations return void and use optimistic updates, the approaches could be aligned for consistency.
The current approach is conservative (ensures consistency) but performs an extra fetch on success. If you prefer optimistic-only (like reorder), remove the onSettled handler. If you prefer always-refetch (safer), add onSettled to useReorderFolders as well.
🤖 Prompt for 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.
In `@frontend/src/hooks/folders.ts` around lines 57 - 71, The onSettled
invalidation in useSetFolderEntries causes a refetch even after successful
optimistic updates; to align with useReorderFolders, remove the onSettled
handler from useSetFolderEntries so it only invalidates onError (keep the
onError invalidateQueries) and rely on the optimistic update in onMutate; locate
useSetFolderEntries and delete the onSettled property (and its invalidateQueries
call) so behavior matches useReorderFolders.
Comment on lines
+28
to
+36
| type Song struct { | ||
| ID uint `gorm:"primarykey"` | ||
| Title string `gorm:"not null"` | ||
| Artist string | ||
| OwnerUserID *uint `gorm:"index"` | ||
| OwnerBandID *uint `gorm:"index"` | ||
| CreatedAt time.Time | ||
| UpdatedAt time.Time | ||
| } |
There was a problem hiding this comment.
Missing XOR constraint on Song ownership.
The Song model allows both OwnerUserID and OwnerBandID to be nil (orphaned song) or both to be non-null (ambiguous ownership). The schema does not enforce that exactly one ownership field must be set.
This can corrupt data integrity: orphaned songs could leak across user boundaries, and dual-ownership breaks the identity model described in the PR objectives ("a user XOR a band").
🛡️ Add a CHECK constraint or repository-layer enforcement
Option 1: SQLite CHECK constraint (preferred for DB-level guarantee):
type Song struct {
ID uint `gorm:"primarykey"`
Title string `gorm:"not null"`
Artist string
- OwnerUserID *uint `gorm:"index"`
- OwnerBandID *uint `gorm:"index"`
+ OwnerUserID *uint `gorm:"index;check:(owner_user_id IS NOT NULL AND owner_band_id IS NULL) OR (owner_user_id IS NULL AND owner_band_id IS NOT NULL)"`
+ OwnerBandID *uint `gorm:"index"`
CreatedAt time.Time
UpdatedAt time.Time
}Option 2: Repository validation (if CHECK is not supported):
Add a validation hook in repository.CreateSong to reject songs that violate XOR before insertion.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| type Song struct { | |
| ID uint `gorm:"primarykey"` | |
| Title string `gorm:"not null"` | |
| Artist string | |
| OwnerUserID *uint `gorm:"index"` | |
| OwnerBandID *uint `gorm:"index"` | |
| CreatedAt time.Time | |
| UpdatedAt time.Time | |
| } | |
| type Song struct { | |
| ID uint `gorm:"primarykey"` | |
| Title string `gorm:"not null"` | |
| Artist string | |
| OwnerUserID *uint `gorm:"index;check:(owner_user_id IS NOT NULL AND owner_band_id IS NULL) OR (owner_user_id IS NULL AND owner_band_id IS NOT NULL)"` | |
| OwnerBandID *uint `gorm:"index"` | |
| CreatedAt time.Time | |
| UpdatedAt time.Time | |
| } |
🤖 Prompt for 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.
In `@internal/model/songs.go` around lines 28 - 36, The Song model currently
allows OwnerUserID and OwnerBandID to be both null or both set; add enforcement
so exactly one is set: preferred fix is a DB-level CHECK constraint (ALTER TABLE
/ CREATE TABLE migration adding CHECK ((OwnerUserID IS NULL) != (OwnerBandID IS
NULL))) tied to the Song schema; if DB engine doesn't support CHECK or for
safety also add repository-layer validation in repository.CreateSong (and any
update method) to reject creates/updates where both are null or both non-null,
returning a validation error; update any migrations and tests accordingly and
reference the Song struct and OwnerUserID/OwnerBandID fields and
repository.CreateSong in the changes.
Comment on lines
+47
to
+72
| // FoldersForUser returns the user's folders in position order, each with | ||
| // its ordered song IDs. | ||
| func (r *Repo) FoldersForUser(userID uint) ([]FolderWithSongs, error) { | ||
| var folders []model.Folder | ||
| err := r.db.Where("owner_user_id = ?", userID). | ||
| Order("position, id").Find(&folders).Error | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| result := make([]FolderWithSongs, 0, len(folders)) | ||
| for _, f := range folders { | ||
| var entries []model.FolderEntry | ||
| if err := r.db.Where("folder_id = ?", f.ID). | ||
| Order("position, id").Find(&entries).Error; err != nil { | ||
| return nil, err | ||
| } | ||
| songIDs := make([]uint, 0, len(entries)) | ||
| for _, e := range entries { | ||
| songIDs = append(songIDs, e.SongID) | ||
| } | ||
| result = append(result, FolderWithSongs{ | ||
| ID: f.ID, Name: f.Name, Position: f.Position, SongIDs: songIDs, | ||
| }) | ||
| } | ||
| return result, nil | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy lift
N+1 query: load folder entries in a single query.
FoldersForUser loads entries for each folder inside a loop (lines 59-61), issuing one query per folder. For a user with many folders, this creates a performance bottleneck.
Use GORM's Preload to load all entries in a single query:
⚡ Proposed fix using Preload
Replace the current implementation with:
func (r *Repo) FoldersForUser(userID uint) ([]FolderWithSongs, error) {
var folders []model.Folder
err := r.db.Where("owner_user_id = ?", userID).
+ Preload("Entries", func(db *gorm.DB) *gorm.DB {
+ return db.Order("position, id")
+ }).
Order("position, id").Find(&folders).Error
if err != nil {
return nil, err
}
result := make([]FolderWithSongs, 0, len(folders))
for _, f := range folders {
- var entries []model.FolderEntry
- if err := r.db.Where("folder_id = ?", f.ID).
- Order("position, id").Find(&entries).Error; err != nil {
- return nil, err
- }
- songIDs := make([]uint, 0, len(entries))
- for _, e := range entries {
+ songIDs := make([]uint, 0, len(f.Entries))
+ for _, e := range f.Entries {
songIDs = append(songIDs, e.SongID)
}
result = append(result, FolderWithSongs{
ID: f.ID, Name: f.Name, Position: f.Position, SongIDs: songIDs,
})
}
return result, nil
}This requires model.Folder to have an Entries []FolderEntry field with the appropriate GORM association tag (e.g., gorm:"foreignKey:FolderID").
🤖 Prompt for 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.
In `@internal/repository/folders.go` around lines 47 - 72, FoldersForUser performs
an N+1 by querying FolderEntry per folder; modify it to preload entries in one
query by using GORM Preload on model.Folder (ensure model.Folder has Entries
[]FolderEntry with gorm:"foreignKey:FolderID"), then iterate folders' Entries to
build FolderWithSongs (keep ID/Name/Position and collect SongID from each
Entry); update the function's use of r.db to use r.db.Where("owner_user_id = ?",
userID).Preload("Entries", func(db *gorm.DB) *gorm.DB { return
db.Order("position, id") }).Order("position, id").Find(&folders).Error and
remove the per-folder query loop.
Comment on lines
+19
to
+36
| func (r *Repo) CreateResource(songID, userID uint, url, label string) (*model.Resource, error) { | ||
| var maxPos int | ||
| err := r.db.Model(&model.Resource{}). | ||
| Select("COALESCE(MAX(position), 0)"). | ||
| Where("song_id = ? AND user_id = ?", songID, userID). | ||
| Scan(&maxPos).Error | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| res := &model.Resource{ | ||
| SongID: songID, UserID: &userID, | ||
| URL: url, Label: label, Position: maxPos + 1, | ||
| } | ||
| if err := r.db.Create(res).Error; err != nil { | ||
| return nil, err | ||
| } | ||
| return res, nil | ||
| } |
There was a problem hiding this comment.
Race condition in position assignment.
CreateResource reads MAX(position) then writes maxPos + 1 in two separate operations. Concurrent creates for the same (songID, userID) can read the same maxPos, then both write the same new position, violating the expected unique ordering for drag-and-drop reordering.
The PR objectives describe "drag-reorder" behavior, which requires stable, unique positions per scope.
🔒 Proposed fix using a transaction or advisory lock
Option 1: Transaction with SELECT FOR UPDATE (SQLite supports this):
func (r *Repo) CreateResource(songID, userID uint, url, label string) (*model.Resource, error) {
+ return r.createResourceTx(songID, userID, url, label)
+}
+
+func (r *Repo) createResourceTx(songID, userID uint, url, label string) (*model.Resource, error) {
+ tx := r.db.Begin()
+ defer func() {
+ if r := recover(); r != nil {
+ tx.Rollback()
+ }
+ }()
+
var maxPos int
- err := r.db.Model(&model.Resource{}).
+ err := tx.Model(&model.Resource{}).
Select("COALESCE(MAX(position), 0)").
Where("song_id = ? AND user_id = ?", songID, userID).
+ Clauses(clause.Locking{Strength: "UPDATE"}).
Scan(&maxPos).Error
if err != nil {
+ tx.Rollback()
return nil, err
}
res := &model.Resource{
SongID: songID, UserID: &userID,
URL: url, Label: label, Position: maxPos + 1,
}
- if err := r.db.Create(res).Error; err != nil {
+ if err := tx.Create(res).Error; err != nil {
+ tx.Rollback()
return nil, err
}
+ if err := tx.Commit().Error; err != nil {
+ return nil, err
+ }
return res, nil
}Option 2: Add a unique constraint on (song_id, user_id, position) and retry on conflict (more complex, requires conflict handling).
🤖 Prompt for 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.
In `@internal/repository/resources.go` around lines 19 - 36, The CreateResource
method currently reads MAX(position) then inserts maxPos+1 separately, causing
race conditions; fix by performing the read-and-insert inside a DB transaction
and lock the relevant rows when computing MAX(position) (use a SELECT FOR UPDATE
/ row lock via r.db transaction/locking clauses on model.Resource filtered by
songID and userID) so concurrent creators serialize and receive unique
positions; alternatively add a unique constraint on (song_id, user_id, position)
and implement retry-on-conflict logic when r.db.Create(res) fails due to
constraint violation—update Repo.CreateResource and model.Resource accordingly.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jun 15, 2026
Merged
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
Test Plan
just checkgreen (all six gates)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Chores