Uh oh!
There was an error while loading. Please reload this page.
feat(api): migrate GET /api/songs - #466
Merged
Merged
Conversation
Port the legacy Recoup-Agent-APIs /songs endpoint into the mono api as
GET /api/songs (flat). Accepts optional isrc and artist_account_id filters,
orders by updated_at DESC, and returns { status, songs } with each song's
song_artists flattened into a top-level artists[] to preserve the wire shape
consumed by chat's getSongsByIsrc caller.
Auth policy: plain validateAuthContext (no checkAccountArtistAccess). Song
metadata is effectively DSP-public, so per-artist song lists are unions of
DSP-public data — scoping them wouldn't reduce exposure. Rationale
documented in the validator JSDoc so a future maintainer doesn't treat the
missing scope check as an oversight.Contributor
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 9 minutes and 25 seconds. ⌛ 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: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request introduces a new GET Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Route as API Route
participant Handler as getSongsHandler
participant Validator as validateGetSongsRequest
participant Auth as validateAuthContext
participant DB as selectSongsWithArtists
participant Supabase
Client->>Route: GET /api/songs?isrc=...
Route->>Handler: delegate request
Handler->>Validator: validate request
Validator->>Auth: check auth context
alt Auth fails
Auth-->>Validator: NextResponse (error)
Validator-->>Handler: NextResponse (error)
Handler-->>Route: NextResponse (error)
Route-->>Client: error response
else Auth succeeds
Auth-->>Validator: validated auth
Validator->>Validator: parse & validate query params
alt Params invalid
Validator-->>Handler: validationErrorResponse
Handler-->>Route: NextResponse (400)
Route-->>Client: validation error
else Params valid
Validator-->>Handler: GetSongsParams
Handler->>DB: selectSongsWithArtists(params)
DB->>Supabase: query songs with artists
Supabase-->>DB: song data + nested artists
DB->>DB: transform artist structure
DB-->>Handler: songs array
Handler->>Handler: format success response
Handler-->>Route: NextResponse (200)
Route-->>Client: { status: "success", songs }
end
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
arpitgupta1214
commented
Apr 21, 2026
CollaboratorAuthor
Preview smoke — |
| Case | Status | Body |
|---|---|---|
| No auth | 401 | {"status":"error","error":"Exactly one of x-api-key or Authorization must be provided"} |
artist_account_id=not-a-uuid (authed) | 400 | {"status":"error","missing_fields":["artist_account_id"],"error":"artist_account_id must be a valid UUID"} |
isrc=USRC17607839 (authed, x-api-key) | 200 | {"status":"success","songs":[{"isrc":"USRC17607839","name":"Crazy Eyes","album":"Bigger Than Both Of Us",...,"artists":[…]}]} |
recoup-api-git-migration-songs-by-isrc-recoupable-ad724970.vercel.app alias not registered for this preview (DEPLOYMENT_NOT_FOUND) — same pattern as other migration PRs; re-verify on that host after promotion to test.
There was a problem hiding this comment.
2 issues found across 8 files
Confidence score: 3/5
- There is a concrete regression risk in
lib/supabase/songs/selectSongsWithArtists.ts: filteringartist_account_idon an embedded relation without!innercan return songs that should be excluded, which may surface incorrect results to users. - The test-file size issue in
lib/songs/__tests__/getSongsHandler.test.tsis low severity and mainly maintainability-focused, so it does not materially block merging on its own. - Given the medium-severity, high-confidence query behavior issue (6/10, confidence 9/10), this PR carries some user-impacting risk until the join/filter logic is tightened.
- Pay close attention to
lib/supabase/songs/selectSongsWithArtists.tsandlib/songs/__tests__/getSongsHandler.test.ts- fix relation filtering correctness first, then address test-file maintainability limits.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/supabase/songs/selectSongsWithArtists.ts">
<violation number="1" location="lib/supabase/songs/selectSongsWithArtists.ts:34">
P2: `artist_account_id` filtering is applied on an embedded relation without `!inner`, so non-matching songs can still be returned.</violation>
</file>
<file name="lib/songs/__tests__/getSongsHandler.test.ts">
<violation number="1" location="lib/songs/__tests__/getSongsHandler.test.ts:1">
P3: Custom agent: **Enforce Clear Code Style and Maintainability Practices**
Test file exceeds the 100-line file-size limit.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client
participant API as Route Handler (Next.js)
participant Auth as validateAuthContext
participant Val as validateGetSongsRequest (Zod)
participant DB as selectSongsWithArtists (Supabase)
participant Supa as Supabase Database
Note over Client,Supa: NEW: GET /api/songs Flow
Client->>API: GET /api/songs?isrc=...&artist_account_id=...
API->>Val: validateGetSongsRequest(request)
Val->>Auth: NEW: validateAuthContext(request)
Note right of Auth: Checks x-api-key or Bearer token
alt Auth Failed
Auth-->>Val: 401 Unauthorized
Val-->>API: 401 Unauthorized
API-->>Client: 401 Unauthorized
else Auth Success
Auth-->>Val: AuthContext
end
Val->>Val: NEW: Parse & trim query params (Zod)
alt Invalid Parameters (e.g. malformed UUID)
Val-->>API: 400 Bad Request
API-->>Client: 400 Bad Request (missing_fields)
else Valid Parameters
Val-->>API: GetSongsParams
end
API->>DB: NEW: getSongsWithArtists(params)
DB->>Supa: select songs + song_artists + accounts!inner
alt Database Error
Supa-->>DB: DB Error
DB-->>API: throw Error
API->>API: internal logging
API-->>Client: 500 Internal Server Error (Obfuscated)
else Success
Supa-->>DB: Raw nested JSON
DB->>DB: NEW: Flatten song_artists.accounts -> artists[]
DB-->>API: { status: "success", songs }
end
API->>API: CHANGED: getCorsHeaders()
API-->>Client: 200 OK + JSON Body + CORS Headers
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…JSDoc - Delete lib/songs/getSongsWithArtists.ts — envelope is a one-liner built inline in the handler; YAGNI over a thin wrapper - Switch .in(col, [val]) → .eq(col, val) in selectSongsWithArtists — both filters are singular, so .in was dead plurality - Trim JSDoc on handler/validator/supabase helper to minimal 'why' comments only
Recent migration supabase helpers (account_catalogs, account_socials) ship without __tests__ — the coverage lives at the handler/validator level. Follow that convention.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
lib/songs/validateGetSongsRequest.ts (2)
26-38: Minor: only the first validation issue is surfaced.If both
isrcandartist_account_idare invalid, the caller only ever sees one. That matches the preview behavior described in the PR, so this is intentional — just flagging in case you'd prefer to return allissues(e.g.,missing_fields: error.issues.map(i => i.path).flat()). Feel free to ignore if parity with the legacy endpoint is the goal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/songs/validateGetSongsRequest.ts` around lines 26 - 38, The current validation in validateGetSongsRequest.ts only returns the first Zod issue (using error.issues[0]/firstError); change it to surface all validation issues from getSongsParamsSchema.safeParse by mapping error.issues into a consolidated payload (e.g., aggregate messages and paths or a missing_fields array) and pass that full array to validationErrorResponse instead of a single issue so callers see every validation problem rather than just the first.
6-9: Adopt top-levelz.uuid()for improved API and tree-shakability. Per Zod 4.1.x migration,z.string().uuid()is deprecated in favor of the top-level form. Note thatz.uuid()enforces strict RFC 4122 compliance; if this endpoint accepts non-standard UUID formats, usez.guid()instead.♻️ Proposed refactor
export const getSongsParamsSchema = z.object({ isrc: z.string().trim().min(1, "isrc cannot be empty").optional(), - artist_account_id: z.string().uuid("artist_account_id must be a valid UUID").optional(),+ artist_account_id: z.uuid("artist_account_id must be a valid UUID").optional(), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/songs/validateGetSongsRequest.ts` around lines 6 - 9, Replace the deprecated z.string().uuid(...) usage in getSongsParamsSchema with the top-level z.uuid() (or z.guid() if non-RFC-4122 UUIDs are expected) for artist_account_id; update the artist_account_id schema entry to use z.uuid("artist_account_id must be a valid UUID").optional() (or z.guid(...).optional()) so the schema uses the new top-level validator and preserves the existing error message and optionality.lib/supabase/songs/selectSongsWithArtists.ts (1)
8-8: Optional: add an explicit return type.A
Promise<Array<{ isrc: string; name: string | null; album: string | null; notes: string | null; updated_at: string; artists: Array<{ id: string; name: string; timestamp: string } | null> }>>(or a named type) helps callers and keeps the public API of this module honest — right now the return type is fully inferred from a large select string.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/supabase/songs/selectSongsWithArtists.ts` at line 8, The function selectSongsWithArtists currently relies on TypeScript to infer a complex return type from the select string; explicitly declare its return type to improve caller ergonomics and API stability — either add a named interface (e.g., SongsWithArtists or SelectSongsWithArtistsResult) describing the array item shape (isrc, name, album, notes, updated_at, artists: Array<{ id, name, timestamp } | null>) and change the signature of selectSongsWithArtists(params: SelectSongsWithArtistsParams = {}): Promise<SongsWithArtists[]> or directly annotate it as Promise<Array<{ isrc: string; name: string | null; album: string | null; notes: string | null; updated_at: string; artists: Array<{ id: string; name: string; timestamp } | null> }>> so callers see the exact contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/songs/validateGetSongsRequest.ts`:
- Around line 1-41: Rename the file to validateGetSongsQuery.ts and rename the
exported function validateGetSongsRequest to validateGetSongsQuery (keeping and
exporting getSongsParamsSchema and GetSongsParams), update any imports that pull
validateGetSongsRequest to import validateGetSongsQuery instead, and ensure the
signature and return type remain Promise<NextResponse | GetSongsParams> and the
schema export name getSongsParamsSchema is preserved so callers and types
continue to work.
---
Nitpick comments:
In `@lib/songs/validateGetSongsRequest.ts`:
- Around line 26-38: The current validation in validateGetSongsRequest.ts only
returns the first Zod issue (using error.issues[0]/firstError); change it to
surface all validation issues from getSongsParamsSchema.safeParse by mapping
error.issues into a consolidated payload (e.g., aggregate messages and paths or
a missing_fields array) and pass that full array to validationErrorResponse
instead of a single issue so callers see every validation problem rather than
just the first.
- Around line 6-9: Replace the deprecated z.string().uuid(...) usage in
getSongsParamsSchema with the top-level z.uuid() (or z.guid() if non-RFC-4122
UUIDs are expected) for artist_account_id; update the artist_account_id schema
entry to use z.uuid("artist_account_id must be a valid UUID").optional() (or
z.guid(...).optional()) so the schema uses the new top-level validator and
preserves the existing error message and optionality.
In `@lib/supabase/songs/selectSongsWithArtists.ts`:
- Line 8: The function selectSongsWithArtists currently relies on TypeScript to
infer a complex return type from the select string; explicitly declare its
return type to improve caller ergonomics and API stability — either add a named
interface (e.g., SongsWithArtists or SelectSongsWithArtistsResult) describing
the array item shape (isrc, name, album, notes, updated_at, artists: Array<{ id,
name, timestamp } | null>) and change the signature of
selectSongsWithArtists(params: SelectSongsWithArtistsParams = {}):
Promise<SongsWithArtists[]> or directly annotate it as Promise<Array<{ isrc:
string; name: string | null; album: string | null; notes: string | null;
updated_at: string; artists: Array<{ id: string; name: string; timestamp } |
null> }>> so callers see the exact contract.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 67ddb040-2782-4bc5-b174-a7875d5fde77
⛔ Files ignored due to path filters (2)
lib/songs/__tests__/getSongsHandler.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/songs/__tests__/validateGetSongsRequest.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (4)
app/api/songs/route.tslib/songs/getSongsHandler.tslib/songs/validateGetSongsRequest.tslib/supabase/songs/selectSongsWithArtists.ts
Comment on lines
+1
to
+41
| import { NextRequest, NextResponse } from "next/server"; | ||
| import { z } from "zod"; | ||
| import { validateAuthContext } from "@/lib/auth/validateAuthContext"; | ||
| import { validationErrorResponse } from "@/lib/zod/validationErrorResponse"; | ||
| export const getSongsParamsSchema = z.object({ | ||
| isrc: z.string().trim().min(1, "isrc cannot be empty").optional(), | ||
| artist_account_id: z.string().uuid("artist_account_id must be a valid UUID").optional(), | ||
| }); | ||
| export type GetSongsParams = z.infer<typeof getSongsParamsSchema>; | ||
| /** | ||
| * Auth-only; no `checkAccountArtistAccess` — song metadata is DSP-public | ||
| * (same data DSPs expose via ISRC lookup), so per-artist scoping would not | ||
| * meaningfully reduce exposure. | ||
| */ | ||
| export async function validateGetSongsRequest( | ||
| request: NextRequest, | ||
| ): Promise<NextResponse | GetSongsParams> { | ||
| const authResult = await validateAuthContext(request); | ||
| if (authResult instanceof NextResponse) { | ||
| return authResult; | ||
| } | ||
| const { searchParams } = new URL(request.url); | ||
| const rawParams: Record<string, string> = {}; | ||
| const isrc = searchParams.get("isrc"); | ||
| const artistAccountId = searchParams.get("artist_account_id"); | ||
| if (isrc !== null) rawParams.isrc = isrc; | ||
| if (artistAccountId !== null) rawParams.artist_account_id = artistAccountId; | ||
| const { data, error } = getSongsParamsSchema.safeParse(rawParams); | ||
| if (error) { | ||
| const firstError = error.issues[0]; | ||
| return validationErrorResponse(firstError.message, firstError.path); | ||
| } | ||
| return data; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Rename to validateGetSongsQuery.ts to match the repo's validator naming convention.
This endpoint validates query parameters (isrc, artist_account_id), not a request body, so per the lib/**/validate*.ts guideline the file should be validateGetSongsQuery.ts with the exported function renamed accordingly. The route/handler imports will need to follow.
As per coding guidelines: "Create validate functions in validate<EndpointName>Body.ts or validate<EndpointName>Query.ts files that export both the schema and inferred TypeScript type."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/songs/validateGetSongsRequest.ts` around lines 1 - 41, Rename the file to
validateGetSongsQuery.ts and rename the exported function
validateGetSongsRequest to validateGetSongsQuery (keeping and exporting
getSongsParamsSchema and GetSongsParams), update any imports that pull
validateGetSongsRequest to import validateGetSongsQuery instead, and ensure the
signature and return type remain Promise<NextResponse | GetSongsParams> and the
schema export name getSongsParamsSchema is preserved so callers and types
continue to work.
- Add !inner on song_artists embed — .eq("song_artists.artist", …)
on a left-joined embed filtered the array but still returned
songs with empty song_artists. !inner excludes the parent row.
- Trim handler + validator tests under the 100-line repo limit
using it.each for the parametrized 200/400 cases.There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/supabase/songs/selectSongsWithArtists.ts">
<violation number="1" location="lib/supabase/songs/selectSongsWithArtists.ts:18">
P2: Using `song_artists!inner` unconditionally changes the endpoint to exclude songs that have no artist join rows, which can silently drop valid songs from unfiltered results.</violation>
</file>
<file name="lib/songs/__tests__/getSongsHandler.test.ts">
<violation number="1" location="lib/songs/__tests__/getSongsHandler.test.ts:33">
P2: The success-path test was weakened: it no longer asserts the response envelope or CORS headers, so API contract regressions can pass unnoticed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| album, | ||
| notes, | ||
| updated_at, | ||
| song_artists!inner ( |
There was a problem hiding this comment.
P2: Using song_artists!inner unconditionally changes the endpoint to exclude songs that have no artist join rows, which can silently drop valid songs from unfiltered results.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/supabase/songs/selectSongsWithArtists.ts, line 18:
<comment>Using `song_artists!inner` unconditionally changes the endpoint to exclude songs that have no artist join rows, which can silently drop valid songs from unfiltered results.</comment>
<file context>
@@ -15,7 +15,7 @@ export async function selectSongsWithArtists(params: SelectSongsWithArtistsParam
notes,
updated_at,
- song_artists (
+ song_artists!inner (
artist,
accounts!inner (
</file context>
Suggested change
| song_artists!inner( | |
| song_artists( |
Comment on lines
+33
to
+37
| ])("200 with query %s", async (qs, expected) => { | ||
| const res = await getSongsHandler(makeReq(`https://x/api/songs?${qs}`)); | ||
| expect(res.status).toBe(200); | ||
| expect(mockSelectSongsWithArtists).toHaveBeenCalledWith(expected); | ||
| }); |
There was a problem hiding this comment.
P2: The success-path test was weakened: it no longer asserts the response envelope or CORS headers, so API contract regressions can pass unnoticed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/songs/__tests__/getSongsHandler.test.ts, line 33:
<comment>The success-path test was weakened: it no longer asserts the response envelope or CORS headers, so API contract regressions can pass unnoticed.</comment>
<file context>
@@ -9,106 +9,54 @@ const mockSelectSongsWithArtists = vi.fn();
+ ["isrc=USRC17607839", { isrc: "USRC17607839" }],
+ [`artist_account_id=${UUID}`, { artist_account_id: UUID }],
+ ["", {}],
+ ])("200 with query %s", async (qs, expected) => {
+ const res = await getSongsHandler(makeReq(`https://x/api/songs?${qs}`));
+ expect(res.status).toBe(200);
</file context>
Suggested change
| ])("200 with query %s",async(qs,expected)=>{ | |
| constres=awaitgetSongsHandler(makeReq(`https://x/api/songs?${qs}`)); | |
| expect(res.status).toBe(200); | |
| expect(mockSelectSongsWithArtists).toHaveBeenCalledWith(expected); | |
| }); | |
| ])("200 with query %s",async(qs,expected)=>{ | |
| constres=awaitgetSongsHandler(makeReq(`https://x/api/songs?${qs}`)); | |
| expect(res.status).toBe(200); | |
| expect(res.headers.get("Access-Control-Allow-Origin")).toBe("*"); | |
| constbody=awaitres.json(); | |
| expect(body.status).toBe("success"); | |
| expect(Array.isArray(body.songs)).toBe(true); | |
| expect(mockSelectSongsWithArtists).toHaveBeenCalledWith(expected); | |
| }); |
3 tasks
sweetmantech
approved these changes
Apr 23, 2026
Contributor
Preview smoke testAgainst preview
Populated row shape (from case 7){
"isrc": "USRC17607839",
"name": "Crazy Eyes",
"album": "Bigger Than Both Of Us",
"notes": "Track: Crazy Eyes\nArtist: Daryl Hall & John Oates\n...",
"updated_at": "2026-04-02T22:44:31.916527+00:00",
"artists": [
{
"id": "ac214945-993e-4f72-a6d8-27d544d054ef",
"name": "Daryl Hall & John Oates",
"timestamp": null
}
]
}Findings
🤖 Tested with Claude Code |
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Apr 23, 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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Ports the legacy
GET /api/songsExpress route into monoapiwithvalidateAuthContext+ Zod filters (isrc,artist_account_id); response shape is byte-identical to the legacy endpoint so existing callers need no adaptation.Test plan
pnpm testgreen (2184 passed)pnpm lint:checkcleanisrc=<known>withx-api-key)Summary by cubic
Adds a new
GET /api/songsendpoint that returns songs with flattened artist accounts. Supports optional filters and auth, matching the legacy Express/songsresponse so existing callers (e.g., chat’sgetSongsByIsrc) need no changes.New Features
isrcandartist_account_id(UUID). Ordered byupdated_atDESC.x-api-keyorAuthorization: Bearer. No per-artist scope check.{ status: "success", songs }withartists[]flattened fromsong_artists.accounts. Includes CORS preflight and headers.Bug Fixes
artist_account_idfilter now excludes non-matching songs viasong_artists!inner(prevents emptysong_artistsrows).Written for commit 3bc4656. Summary will update on new commits.
Summary by CodeRabbit