feat(api): migrate GET /api/artists/pro#477
Conversation
Admin-scoped flat route returning the deduped list of artist IDs owned by "pro" accounts (enterprise email domain or active Stripe subscription). Ports Stripe client + getActiveSubscriptions inline until the subscription PR lands; reuses validateAdminAuth for scope.
|
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 7 minutes and 40 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 (3)
📝 WalkthroughWalkthroughThis PR introduces a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant Route as API Route
participant Handler as getArtistsProHandler
participant Validator as validateGetArtistsProRequest
participant Auth as validateAdminAuth
participant ProLogic as getProArtists
participant EntDB as Enterprise DB
participant SubDB as Subscriptions DB
participant AccountDB as Account Mapping DB
Client->>Route: GET /api/artists/pro
Route->>Handler: forward request
Handler->>Validator: validate request
Validator->>Auth: enforce admin auth
Auth-->>Validator: success or error response
alt validation fails
Validator-->>Handler: NextResponse (error)
Handler-->>Route: NextResponse
Route-->>Client: 401/403 error
else validation succeeds
Validator-->>Handler: empty object (valid)
Handler->>ProLogic: getProArtists()
par fetch concurrently
ProLogic->>EntDB: getEnterpriseAccountIds()
EntDB-->>ProLogic: enterprise account IDs
and
ProLogic->>SubDB: selectActiveSubscriptionAccountIds()
SubDB-->>ProLogic: subscription account IDs
end
ProLogic->>ProLogic: merge & deduplicate
ProLogic->>AccountDB: selectAccountArtistIds(merged IDs)
AccountDB-->>ProLogic: artist IDs
ProLogic-->>Handler: artist_id[]
Handler-->>Route: 200 + artists JSON
Route-->>Client: { status: 200, artists: [...] }
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 16 files
Confidence score: 2/5
- Several high-confidence issues in
lib/stripe/getActiveSubscriptions.tsand Supabase selectors convert real backend failures into[], which can silently change error cases into “no data” responses. - The most severe risk is user-facing correctness:
/api/artists/promay return 200 with incomplete or missing pro artist data when queries fail, rather than triggering proper error handling. - Given multiple 7–8/10 severity findings with 8–9/10 confidence, this is a clear regression risk and not just housekeeping.
- Pay close attention to
lib/stripe/getActiveSubscriptions.ts,lib/supabase/account_emails/selectAccountEmailsByDomain.ts,lib/supabase/account_artist_ids/selectAccountArtistIds.ts,lib/supabase/account_emails/selectAccountEmailsByAccountIds.ts,app/api/artists/pro/route.ts- error swallowing and API correctness need to be fixed before merge.
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/account_emails/selectAccountEmailsByDomain.ts">
<violation number="1" location="lib/supabase/account_emails/selectAccountEmailsByDomain.ts:17">
P1: Do not swallow Supabase query failures as `[]`; this hides backend errors and can return incorrect 200 responses with missing pro artists.</violation>
</file>
<file name="lib/supabase/account_artist_ids/selectAccountArtistIds.ts">
<violation number="1" location="lib/supabase/account_artist_ids/selectAccountArtistIds.ts:15">
P1: Database errors are being swallowed and converted to an empty result, which can silently produce incorrect responses instead of triggering proper error handling.</violation>
</file>
<file name="lib/stripe/getActiveSubscriptions.ts">
<violation number="1" location="lib/stripe/getActiveSubscriptions.ts:27">
P1: Do not return `[]` on Stripe errors here; it masks real failures as empty data and can produce incorrect results.</violation>
</file>
<file name="lib/supabase/account_emails/selectAccountEmailsByAccountIds.ts">
<violation number="1" location="lib/supabase/account_emails/selectAccountEmailsByAccountIds.ts:16">
P1: Do not return an empty array on Supabase query failure here; it masks backend errors and can make `/api/artists/pro` return a successful but incomplete result.
(Based on your team's feedback about returning generic 500s while logging full internal errors.) [FEEDBACK_USED]</violation>
</file>
<file name="app/api/artists/pro/route.ts">
<violation number="1" location="app/api/artists/pro/route.ts:27">
P2: Custom agent: **Module should export a single primary function whose name matches the filename**
Module exports multiple top-level functions instead of a single primary export, violating the filename-to-export convention.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client as Admin/Task Client
participant Route as GET /api/artists/pro
participant Auth as validateAdminAuth
participant Service as getEnterpriseArtists
participant Stripe as Stripe API
participant DB as Supabase
Note over Client,DB: NEW: Admin-scoped Pro Artist Migration Flow
Client->>Route: Request with Admin Token
Route->>Auth: NEW: validateAdminAuth(request)
alt Unauthorized or Non-Admin
Auth-->>Client: 401 Unauthorized / 403 Forbidden
else Admin Authorized
Auth-->>Route: Auth context (accountId, token)
Route->>Service: NEW: getEnterpriseArtists()
par Parallel Data Fetch
Service->>DB: NEW: selectAccountEmailsByDomain(enterprise_domains)
DB-->>Service: enterprise account list
and
Service->>Stripe: NEW: getActiveSubscriptions()
Note right of Stripe: Capped at 100 active subs
Stripe-->>Service: Stripe metadata (accountId)
Service->>DB: NEW: selectAccountEmailsByAccountIds(stripe_account_ids)
DB-->>Service: subscriber account list
end
Service->>Service: Deduplicate unique account_ids
opt If pro account_ids exists
Service->>DB: NEW: selectAccountArtistIds(account_ids)
DB-->>Service: artist_id rows
end
Service-->>Route: Deduplicated string[] (artist_ids)
alt Request Success
Route-->>Client: 200 OK + { status: "success", artists }
else Internal Exception
Note over Route: NEW: Error masked to prevent DB leak
Route-->>Client: 500 Internal Server Error
end
end
Note over Route: NEW: Includes CORS headers via getCorsHeaders()
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| .select("artist_id") | ||
| .in("account_id", accountIds); | ||
|
|
||
| if (error) { |
There was a problem hiding this comment.
P1: Database errors are being swallowed and converted to an empty result, which can silently produce incorrect responses instead of triggering proper error handling.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/supabase/account_artist_ids/selectAccountArtistIds.ts, line 15:
<comment>Database errors are being swallowed and converted to an empty result, which can silently produce incorrect responses instead of triggering proper error handling.</comment>
<file context>
@@ -0,0 +1,21 @@
+ .select("artist_id")
+ .in("account_id", accountIds);
+
+ if (error) {
+ console.error("Error fetching account_artist_ids:", error);
+ return [];
</file context>
| @@ -0,0 +1,29 @@ | |||
| import { NextRequest, NextResponse } from "next/server"; | |||
There was a problem hiding this comment.
P2: Custom agent: Module should export a single primary function whose name matches the filename
Module exports multiple top-level functions instead of a single primary export, violating the filename-to-export convention.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/api/artists/pro/route.ts, line 27:
<comment>Module exports multiple top-level functions instead of a single primary export, violating the filename-to-export convention.</comment>
<file context>
@@ -0,0 +1,29 @@
+ * @param request - The incoming request.
+ * @returns A NextResponse with `{ status, artists, [error] }`.
+ */
+export async function GET(request: NextRequest) {
+ return getArtistsProHandler(request);
+}
</file context>
Next build on Vercel evaluates lib/stripe/client.ts while collecting page data; throwing at module-load when STRIPE_SECRET_KEY is absent from the preview env killed the build. Move the guard into a lazy factory so build-time import is always safe — the first request still surfaces a 500 via the handler's catch when the secret is genuinely missing. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Preview smoke — HEAD
|
| Probe | Expected | Actual | Body |
|---|---|---|---|
| No auth | 401 | 401 | {"status":"error","error":"Exactly one of x-api-key or Authorization must be provided"} |
Non-admin x-api-key |
403 | 403 | {"status":"error","message":"Forbidden"} |
Admin x-api-key |
200 with {status,artists} |
not smoked here — admin key not in CI env | — |
Admin-scope note (intentional behavior change vs legacy)
Legacy https://api.recoupable.com/api/artists/pro is currently unauthenticated — it returns the full paying-customer list (386 artists right now) to any caller. The migration closes that leak by gating on validateAdminAuth (Recoup-org admin membership). Smoke output above confirms the gate rejects unauthenticated and non-admin credentials.
Legacy parity (200 set)
Legacy returns 386 deduped artist IDs. Handler parity is unit-covered (lib/artists/__tests__/getArtistsProHandler.test.ts — it.each matrix of enterprise-only / subscription-only / both-deduped / neither paths, plus the 500-leak regression). Full 200 cross-check against legacy will be verified by whoever runs the admin-key smoke after merge to test.
Build fix
Initial deploy failed because lib/stripe/client.ts threw at module-load when STRIPE_SECRET_KEY was absent; Next evaluates that module during next build on Vercel. Moved the guard into a lazy factory (33b2c3a) — build-time import is now safe, missing secret surfaces as a 500 on first request.
There was a problem hiding this comment.
2 issues found across 2 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/stripe/client.ts">
<violation number="1" location="lib/stripe/client.ts:22">
P3: Custom agent: **Module should export a single primary function whose name matches the filename**
Default-exported primary function name does not match the file basename.</violation>
<violation number="2" location="lib/stripe/client.ts:27">
P1: A missing Stripe secret now gets swallowed as an empty subscription set, causing `/api/artists/pro` to return incomplete "success" data instead of failing.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Add optional domain filter (ilike %@<domain>%) to the existing selectAccountEmails helper and drop the per-filter wrappers. One helper, three optional filters (emails, accountIds, domain).
Use Stripe SDK's async iterator so we're not capped at 100 rows — the cursor paginates the full active set. The accountId filter param was dead (no caller passed it); drop rather than carry; reintroduce via Stripe Search API if a single-account lookup is ever needed.
Swap getActiveSubscriptions for an iterateActiveSubscriptions async generator. Two wins: 1. status: 'active' is Stripe's native filter — no current_period_end vs server-clock comparison (timezone / skew hazards). 2. Yielded incrementally so callers fold results into a Set as they arrive; no hard in-memory cap on the active subscription count. Caller now holds only the deduped accountId strings, not full Subscription objects.
There was a problem hiding this comment.
1 issue found across 1 file (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/stripe/getActiveSubscriptions.ts">
<violation number="1" location="lib/stripe/getActiveSubscriptions.ts:13">
P1: `autoPagingToArray({ limit: 10000 })` introduces a hard truncation at 10k subscriptions, which can silently omit active subscribers.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- lib/stripe/ now owns getActiveSubscriptions + getSubscriberAccountEmails (they query Stripe, not enterprise domains). - lib/enterprise/ keeps only ENTERPRISE_DOMAINS and the domain email fan-out. - The merge orchestrator moves to lib/artists/getProArtists.ts (enterprise ∪ subscribers); renamed from getEnterpriseArtists since 'enterprise artists' understates what it returns. - getActiveSubscriptions uses a manual starting_after cursor loop so there is no iterator and no hard row cap.
There was a problem hiding this comment.
1 issue found across 8 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/stripe/getActiveSubscriptions.ts">
<violation number="1" location="lib/stripe/getActiveSubscriptions.ts:26">
P1: Do not swallow Stripe fetch errors here; rethrow after logging so the handler can return a proper 500 instead of silently returning incomplete results.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…ound-trip The subscriber path fetched Stripe subs, extracted metadata.accountId, round-tripped Supabase to get account_emails rows, then unpacked account_id again — the email lookup was inert. - lib/stripe/getSubscriberAccountIds reads metadata.accountId from Stripe directly; no Supabase call. - lib/enterprise/getEnterpriseAccountIds projects the ilike result to account_ids at the source. - getProArtists now merges two string arrays, dedupes once, then hits account_artist_ids.
getSubscriberAccountIds was a two-line .map/.filter wrapper. Drop it and inline against the Stripe result — no useful indirection.
getActiveSubscriptions returned full Subscription objects only to have every caller project down to metadata.accountId. Collapse that into getActiveSubscriptionAccountIds — strings in, strings out.
There was a problem hiding this comment.
1 issue found across 6 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/stripe/getActiveSubscriptionAccountIds.ts">
<violation number="1" location="lib/stripe/getActiveSubscriptionAccountIds.ts:29">
P2: On Stripe API errors, the function can return a partial ID list instead of the documented fail-safe empty array.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Supabase has a subscriptions table mirroring billing-provider state (stripe/lemon-squeezy/paddle) with account_id + active flag. Read from that and skip Stripe entirely — no pagination, no secret-key env var, no provider round-trip, and cross-provider correct. - lib/supabase/subscriptions/selectActiveSubscriptionAccountIds.ts - lib/stripe/ removed - stripe package uninstalled getProArtists now merges two Supabase queries.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
lib/supabase/account_emails/selectAccountEmails.ts (1)
37-39: Escape ILIKE wildcards indomainfor defense-in-depth.Today
ENTERPRISE_DOMAINSis a hard-coded set, so this is safe in practice. ButselectAccountEmailsis a generic helper — the first time a caller passes an unsanitizeddomainstring,%or_characters would silently widen the match (e.g.,domain = "%"returns every email). Cheap to guard at the boundary:🛡️ Proposed hardening
if (hasDomain) { - query = query.ilike("email", `%@${domain}%`); + const escapedDomain = domain!.replace(/([\\%_])/g, "\\$1"); + query = query.ilike("email", `%@${escapedDomain}%`); }Also worth a one-line comment noting that the trailing
%intentionally preserves the legacyuser+tag@<domain>.anythingmatch (so future readers don't "tighten" it to%@${domain}and break parity).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/supabase/account_emails/selectAccountEmails.ts` around lines 37 - 39, selectAccountEmails currently passes the raw domain into query.ilike when hasDomain is true, which lets '%' and '_' in the domain widen the match; before calling query.ilike in the hasDomain branch, escape SQL ILIKE wildcard characters in the domain (escape '%' -> '\%' and '_' -> '\_') and use the escaped value in the pattern; keep the trailing '%' in the pattern intentionally to preserve legacy matching of user+tag@<domain>.anything and add a one-line comment by the query.ilike call explaining that the trailing '%' is deliberate to maintain parity.lib/artists/validateGetArtistsProRequest.ts (1)
10-18: Consider returning the auth context instead of{}for future-proofing.Returning
Record<string, never>discards theaccountId/orgId/authTokenthatvalidateAdminAuthalready resolved. If the handler ever needs to log which admin triggered the call (audit trail) or scope the pro-artist query, you'd have to re-authenticate. Mirroring the pattern used invalidateGetArtistsRequest(which returnsparamson success) would keep the contract uniform across validators.♻️ Proposed refactor
+import type { AuthContext } from "@/lib/auth/validateAuthContext"; + export async function validateGetArtistsProRequest( request: NextRequest, -): Promise<NextResponse | Record<string, never>> { +): Promise<NextResponse | AuthContext> { const auth = await validateAdminAuth(request); if (auth instanceof NextResponse) { return auth; } - return {}; + return auth; }(Adjust the imported type name to whatever
validateAdminAuthactually returns.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/artists/validateGetArtistsProRequest.ts` around lines 10 - 18, The validator validateGetArtistsProRequest currently discards the resolved auth context and returns an empty object; change it to return the auth object from validateAdminAuth on success so callers can access accountId/orgId/authToken for logging or scoping. Update the function signature return type from Promise<NextResponse | Record<string, never>> to Promise<NextResponse | <AuthType>> (use the actual type returned by validateAdminAuth), and on success return auth rather than {}—mirroring the pattern used by validateGetArtistsRequest; keep the existing NextResponse short-circuit when auth is an instance of NextResponse.lib/supabase/account_artist_ids/selectAccountArtistIds.ts (1)
7-21: Add an explicit return type for the contract.The signature currently leaks the inferred Supabase select shape. Per the repo guideline on typed Supabase returns, an explicit
Promise<Pick<Tables<"account_artist_ids">, "artist_id">[]>(or a dedicated projection type) makes the contract obvious to callers likegetProArtistsand prevents a futureselect("*")change from silently broadening the return type.♻️ Proposed refactor
+import type { Tables } from "@/types/database.types"; import supabase from "@/lib/supabase/serverClient"; -export async function selectAccountArtistIds(accountIds: string[]) { +export async function selectAccountArtistIds( + accountIds: string[], +): Promise<Pick<Tables<"account_artist_ids">, "artist_id">[]> { if (accountIds.length === 0) return [];As per coding guidelines: "Return typed results using Tables<'table_name'>" for Supabase operations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/supabase/account_artist_ids/selectAccountArtistIds.ts` around lines 7 - 21, Add an explicit return type to selectAccountArtistIds so it returns Promise<Pick<Tables<"account_artist_ids">, "artist_id">[]> instead of an inferred Supabase shape; update the function signature (selectAccountArtistIds) to use that type, ensure the supabase .select("artist_id") call is kept, and cast or map the received data to that projection before returning (handle null/error paths by returning an empty array typed to the same projection) so callers like getProArtists receive a stable, narrow contract.lib/artists/getArtistsProHandler.ts (2)
19-26: DRY: PrefererrorResponse()unless theartists: []field is load-bearing.Every other handler in the codebase (e.g.,
getSongsHandler) funnels 500s through@/lib/networking/errorResponseto keep the error envelope ({ status: "error", error }) and CORS headers consistent. This handler inlines the response and additionally addsartists: []. If clients rely onartistsalways being present on the response body, keep the inline form but call it out in a comment so the next refactor doesn't "normalize" it away. Otherwise, switching toerrorResponse("Internal server error", 500)removes the duplication.♻️ Proposed refactor (if
artists: []isn't required on error)-import { NextRequest, NextResponse } from "next/server"; -import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; +import { NextRequest, NextResponse } from "next/server"; +import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; +import { errorResponse } from "@/lib/networking/errorResponse"; import { getProArtists } from "@/lib/artists/getProArtists"; import { validateGetArtistsProRequest } from "@/lib/artists/validateGetArtistsProRequest"; @@ } catch (error) { - // Never leak error.message — it can surface DB hosts / stack hints. console.error("[ERROR] getArtistsProHandler:", error); - return NextResponse.json( - { status: "error", artists: [], error: "Internal server error" }, - { status: 500, headers: getCorsHeaders() }, - ); + // Never leak error.message — it can surface DB hosts / stack hints. + return errorResponse("Internal server error", 500); }As per coding guidelines: "Extract shared logic into reusable utilities following Don't Repeat Yourself (DRY) principle".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/artists/getArtistsProHandler.ts` around lines 19 - 26, In getArtistsProHandler, avoid inlining the 500 response; if the artists: [] field is not required by clients replace the inline NextResponse.json call with a call to the shared errorResponse("Internal server error", 500) to preserve consistent error envelope and CORS headers (use the existing errorResponse function from `@/lib/networking/errorResponse`); if clients do need artists: [] to be present keep the inline response but add a clear comment above the return indicating the field is load-bearing so it isn't later removed during DRY refactors. Also ensure you don't leak sensitive error details when logging.
6-13: Minor: consider a short-circuit for OPTIONS if the route ever shares this handler.Not strictly needed today (the route file defines a dedicated
OPTIONShandler per the PR description), but worth a one-line guard in case this handler is ever wired to multiple verbs. Purely defensive — feel free to ignore if the route contract is locked.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/artists/getArtistsProHandler.ts` around lines 6 - 13, Add a one-line defensive short-circuit at the start of getArtistsProHandler to immediately handle HTTP OPTIONS requests: check request.method === 'OPTIONS' and return an empty 204 response before calling validateGetArtistsProRequest or getProArtists; this keeps validateGetArtistsProRequest and the rest of the handler from running for preflight checks if this handler ever gets wired to multiple verbs.lib/enterprise/consts.ts (1)
5-12: Consider movingENTERPRISE_DOMAINStolib/const.ts.Per a prior learning, shared constants in this repo live in
lib/const.ts(alongsideINBOUND_EMAIL_DOMAIN,OUTBOUND_EMAIL_DOMAIN, etc.).ENTERPRISE_DOMAINSis feature-scoped today, but it's likely to be referenced from billing, onboarding, or analytics paths later — centralizing now avoids a future "which file owns this list" split. If you'd rather keep it colocated with the enterprise feature, that's defensible too; just flagging the convention.Also: small readability win — a trailing comment noting that
rostrum.comvsrostrumrecords.comare distinct (not a duplicate/typo) will save the next reader a double-take.Based on learnings: "All shared constants should be defined in
lib/const.ts, includingINBOUND_EMAIL_DOMAIN,OUTBOUND_EMAIL_DOMAIN,SUPABASE_STORAGE_BUCKET, wallet addresses, model names, and API keys".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/enterprise/consts.ts` around lines 5 - 12, Move the shared constant ENTERPRISE_DOMAINS out of lib/enterprise/consts.ts into the central lib/const.ts to follow the repo convention for shared constants; add an exported ReadonlySet<string> named ENTERPRISE_DOMAINS in lib/const.ts, remove or re-export the duplicate from lib/enterprise/consts.ts, and update any imports that reference ENTERPRISE_DOMAINS to import from lib/const.ts instead; while moving, append a trailing comment explaining that "rostrum.com" and "rostrumrecords.com" are distinct entries (not a duplicate/typo) so future readers aren't confused.app/api/artists/pro/route.ts (1)
27-29: Considerawait-ing the handler so this route owns its error boundary.Returning the handler's promise directly is perfectly valid, but since
getArtistsProHandleris the sole point of failure visible from this route, anawait+try/catchhere would let the route layer guarantee a JSON 500 with CORS headers even if the handler ever throws synchronously or rejects unexpectedly (e.g., a future refactor slips past its internal try/catch). Totally optional — the handler appears to already wrap its own body in try/catch per the PR summary, so this is belt-and-suspenders territory.🧯 Optional defensive shape
-export async function GET(request: NextRequest) { - return getArtistsProHandler(request); -} +export async function GET(request: NextRequest) { + try { + return await getArtistsProHandler(request); + } catch (error) { + console.error("/api/artists/pro unhandled error:", error); + return NextResponse.json( + { status: "error", error: "Internal Server Error" }, + { status: 500, headers: getCorsHeaders() }, + ); + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/artists/pro/route.ts` around lines 27 - 29, The GET route currently returns the promise from getArtistsProHandler directly; change GET to await getArtistsProHandler inside a try/catch so the route owns its error boundary—call await getArtistsProHandler(request) and in catch return a JSON 500 response with appropriate CORS headers (same headers used elsewhere) so any synchronous throw or rejection is handled at the route level; reference the GET function and getArtistsProHandler to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/api/artists/pro/route.ts`:
- Around line 17-26: The JSDoc for the GET handler in
app/api/artists/pro/route.ts is stale (mentions "active Stripe subscriptions");
update the comment above the handler to remove Stripe references and instead
state that "pro" artists are determined from enterprise-domain emails or the
local Supabase subscriptions mirror (the code reading from the subscriptions
mirror determines active status). Keep the same shape of the comment (purpose,
params, returns) but correct the implementation source so future readers know to
look at the Supabase mirror logic used by this route.
---
Nitpick comments:
In `@app/api/artists/pro/route.ts`:
- Around line 27-29: The GET route currently returns the promise from
getArtistsProHandler directly; change GET to await getArtistsProHandler inside a
try/catch so the route owns its error boundary—call await
getArtistsProHandler(request) and in catch return a JSON 500 response with
appropriate CORS headers (same headers used elsewhere) so any synchronous throw
or rejection is handled at the route level; reference the GET function and
getArtistsProHandler to locate the change.
In `@lib/artists/getArtistsProHandler.ts`:
- Around line 19-26: In getArtistsProHandler, avoid inlining the 500 response;
if the artists: [] field is not required by clients replace the inline
NextResponse.json call with a call to the shared errorResponse("Internal server
error", 500) to preserve consistent error envelope and CORS headers (use the
existing errorResponse function from `@/lib/networking/errorResponse`); if clients
do need artists: [] to be present keep the inline response but add a clear
comment above the return indicating the field is load-bearing so it isn't later
removed during DRY refactors. Also ensure you don't leak sensitive error details
when logging.
- Around line 6-13: Add a one-line defensive short-circuit at the start of
getArtistsProHandler to immediately handle HTTP OPTIONS requests: check
request.method === 'OPTIONS' and return an empty 204 response before calling
validateGetArtistsProRequest or getProArtists; this keeps
validateGetArtistsProRequest and the rest of the handler from running for
preflight checks if this handler ever gets wired to multiple verbs.
In `@lib/artists/validateGetArtistsProRequest.ts`:
- Around line 10-18: The validator validateGetArtistsProRequest currently
discards the resolved auth context and returns an empty object; change it to
return the auth object from validateAdminAuth on success so callers can access
accountId/orgId/authToken for logging or scoping. Update the function signature
return type from Promise<NextResponse | Record<string, never>> to
Promise<NextResponse | <AuthType>> (use the actual type returned by
validateAdminAuth), and on success return auth rather than {}—mirroring the
pattern used by validateGetArtistsRequest; keep the existing NextResponse
short-circuit when auth is an instance of NextResponse.
In `@lib/enterprise/consts.ts`:
- Around line 5-12: Move the shared constant ENTERPRISE_DOMAINS out of
lib/enterprise/consts.ts into the central lib/const.ts to follow the repo
convention for shared constants; add an exported ReadonlySet<string> named
ENTERPRISE_DOMAINS in lib/const.ts, remove or re-export the duplicate from
lib/enterprise/consts.ts, and update any imports that reference
ENTERPRISE_DOMAINS to import from lib/const.ts instead; while moving, append a
trailing comment explaining that "rostrum.com" and "rostrumrecords.com" are
distinct entries (not a duplicate/typo) so future readers aren't confused.
In `@lib/supabase/account_artist_ids/selectAccountArtistIds.ts`:
- Around line 7-21: Add an explicit return type to selectAccountArtistIds so it
returns Promise<Pick<Tables<"account_artist_ids">, "artist_id">[]> instead of an
inferred Supabase shape; update the function signature (selectAccountArtistIds)
to use that type, ensure the supabase .select("artist_id") call is kept, and
cast or map the received data to that projection before returning (handle
null/error paths by returning an empty array typed to the same projection) so
callers like getProArtists receive a stable, narrow contract.
In `@lib/supabase/account_emails/selectAccountEmails.ts`:
- Around line 37-39: selectAccountEmails currently passes the raw domain into
query.ilike when hasDomain is true, which lets '%' and '_' in the domain widen
the match; before calling query.ilike in the hasDomain branch, escape SQL ILIKE
wildcard characters in the domain (escape '%' -> '\%' and '_' -> '\_') and use
the escaped value in the pattern; keep the trailing '%' in the pattern
intentionally to preserve legacy matching of user+tag@<domain>.anything and add
a one-line comment by the query.ilike call explaining that the trailing '%' is
deliberate to maintain parity.
🪄 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: 3210d8af-e73f-48ee-b0e0-eec60d81b72c
⛔ Files ignored due to path filters (2)
lib/artists/__tests__/getArtistsProHandler.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/artists/__tests__/validateGetArtistsProRequest.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (9)
app/api/artists/pro/route.tslib/artists/getArtistsProHandler.tslib/artists/getProArtists.tslib/artists/validateGetArtistsProRequest.tslib/enterprise/consts.tslib/enterprise/getEnterpriseAccountIds.tslib/supabase/account_artist_ids/selectAccountArtistIds.tslib/supabase/account_emails/selectAccountEmails.tslib/supabase/subscriptions/selectActiveSubscriptionAccountIds.ts
| /** | ||
| * GET /api/artists/pro | ||
| * | ||
| * Returns a deduplicated list of artist IDs owned by "pro" accounts | ||
| * (enterprise-domain emails or active Stripe subscriptions). Admin-scoped — | ||
| * the response is the paying-customer list. | ||
| * | ||
| * @param request - The incoming request. | ||
| * @returns A NextResponse with `{ status, artists, [error] }`. | ||
| */ |
There was a problem hiding this comment.
Stale JSDoc: "active Stripe subscriptions" no longer reflects the implementation.
Per the PR's commit history, later commits replaced the Stripe API calls with reads from a local Supabase subscriptions mirror. Keeping the Stripe reference in this doc comment will mislead future readers hunting down the source of truth for "active subscription" semantics — a small clean-code nit, but docs that drift from reality erode trust faster than bad code does.
📝 Suggested wording tweak
/**
* GET /api/artists/pro
*
* Returns a deduplicated list of artist IDs owned by "pro" accounts
- * (enterprise-domain emails or active Stripe subscriptions). Admin-scoped —
- * the response is the paying-customer list.
+ * (enterprise-domain emails or accounts with an active subscription in the
+ * local subscriptions mirror). Admin-scoped — the response is the
+ * paying-customer list.
*
* `@param` request - The incoming request.
* `@returns` A NextResponse with `{ status, artists, [error] }`.
*/📝 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.
| /** | |
| * GET /api/artists/pro | |
| * | |
| * Returns a deduplicated list of artist IDs owned by "pro" accounts | |
| * (enterprise-domain emails or active Stripe subscriptions). Admin-scoped — | |
| * the response is the paying-customer list. | |
| * | |
| * @param request - The incoming request. | |
| * @returns A NextResponse with `{ status, artists, [error] }`. | |
| */ | |
| /** | |
| * GET /api/artists/pro | |
| * | |
| * Returns a deduplicated list of artist IDs owned by "pro" accounts | |
| * (enterprise-domain emails or accounts with an active subscription in the | |
| * local subscriptions mirror). Admin-scoped — the response is the | |
| * paying-customer list. | |
| * | |
| * `@param` request - The incoming request. | |
| * `@returns` A NextResponse with `{ status, artists, [error] }`. | |
| */ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/artists/pro/route.ts` around lines 17 - 26, The JSDoc for the GET
handler in app/api/artists/pro/route.ts is stale (mentions "active Stripe
subscriptions"); update the comment above the handler to remove Stripe
references and instead state that "pro" artists are determined from
enterprise-domain emails or the local Supabase subscriptions mirror (the code
reading from the subscriptions mirror determines active status). Keep the same
shape of the comment (purpose, params, returns) but correct the implementation
source so future readers know to look at the Supabase mirror logic used by this
route.
There was a problem hiding this comment.
2 issues found across 6 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/subscriptions/selectActiveSubscriptionAccountIds.ts">
<violation number="1" location="lib/supabase/subscriptions/selectActiveSubscriptionAccountIds.ts:12">
P1: This unpaginated `select("account_id")` can silently truncate active subscriptions when row count exceeds the Supabase API max rows, leading to incomplete pro-artist results.
(Based on your team's feedback about preferring DB-side pagination over raising query limits.) [FEEDBACK_USED]</violation>
</file>
<file name="lib/artists/getProArtists.ts">
<violation number="1" location="lib/artists/getProArtists.ts:15">
P2: This now includes non-Stripe active subscriptions because the new selector is provider-agnostic. That changes the endpoint’s Stripe-scoped pro-account logic and can return extra artist IDs.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| export async function getProArtists(): Promise<string[]> { | ||
| const [enterpriseIds, subscriberIds] = await Promise.all([ | ||
| getEnterpriseAccountIds(), | ||
| selectActiveSubscriptionAccountIds(), |
There was a problem hiding this comment.
P2: This now includes non-Stripe active subscriptions because the new selector is provider-agnostic. That changes the endpoint’s Stripe-scoped pro-account logic and can return extra artist IDs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/artists/getProArtists.ts, line 15:
<comment>This now includes non-Stripe active subscriptions because the new selector is provider-agnostic. That changes the endpoint’s Stripe-scoped pro-account logic and can return extra artist IDs.</comment>
<file context>
@@ -12,7 +12,7 @@ import { selectAccountArtistIds } from "@/lib/supabase/account_artist_ids/select
const [enterpriseIds, subscriberIds] = await Promise.all([
getEnterpriseAccountIds(),
- getActiveSubscriptionAccountIds(),
+ selectActiveSubscriptionAccountIds(),
]);
</file context>
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
There was a problem hiding this comment.
KISS - Make this more general
- actual: lib/supabase/subscriptions/selectActiveSubscriptionAccountIds.ts
- required: lib/supabase/subscriptions/selectSubscriptions.ts
There was a problem hiding this comment.
Done in 6b5057f — replaced with generic selectSubscriptions({ accountIds?, active? }). Caller in getProArtists projects to account_ids.
Per review — replace selectActiveSubscriptionAccountIds with a generic
selectSubscriptions({ accountIds?, active? }) helper. Caller projects to
account_ids.
Preview smoke — HEAD
|
| # | Probe | Expected | Actual | Body |
|---|---|---|---|---|
| 1 | No auth | 401 | ✅ 401 | {"status":"error","error":"Exactly one of x-api-key or Authorization must be provided"} |
| 2 | x-api-key: recoup_sk_… |
200 | ✅ 200 | {"status":"success","artists":[…284 UUIDs…]} (1.1s, 11108 B) |
| 3 | Authorization: Bearer recoup_sk_… |
401 | ✅ 401 | {"status":"error","message":"Failed to verify authentication token"} — Bearer path treats value as a Privy JWT per convention; API keys go via x-api-key |
| 4 | Both headers present | 401 | ✅ 401 | {"status":"error","error":"Exactly one of x-api-key or Authorization must be provided"} |
| 5 | Bogus API key | 401 | ✅ 401 | {"status":"error","message":"Unauthorized"} |
| 6 | POST instead of GET | 405 | ✅ 405 | (empty — Next.js default) |
| 7 | OPTIONS preflight | 200 + CORS | ✅ 200 | Access-Control-Allow-Methods: GET, POST, PUT, DELETE, OPTIONS, PATCH, Origin: *, Headers: Content-Type, Authorization, X-Requested-With, x-api-key |
| 8 | Dedup sanity on the 284-row response | total == unique |
✅ 284 = 284 | no duplicates present |
Observations
- All 284 entries are well-formed UUIDs (spot-checked first 5 — standard v4 layout). Sample:
e48d0737-dfb0-407e-85c5-a13ecd424af6. - No stale Stripe behavior leaked — the endpoint reads from the local
subscriptionsmirror (per the final commits) and returns a reasonable-size merged set (enterprise-domain accounts + active subscriptions). - Status line
{ status, artists, [error] }matches the JSDoc contract. Noerrorfield present on success. - Response time was ~1.1s for the full payload on the preview. Not a regression flag on its own, but worth noting it's not a sub-100ms lookup — if you expect this to be on a hot path, a quick trace would confirm whether the two-stage Supabase join (account_emails by domain + subscriptions → account_artist_ids) is cache-friendly or needs tightening.
Open-issue cross-check
All five cubic P1 findings (swallowed Supabase/Stripe errors as [], hard 10k cap, missing secret swallowed, partial Stripe IDs on error) have been addressed in the commit series I can see on the branch — and the smoke test doesn't surface any of the residual failure modes.
The two open semantic flags from review (provider-agnostic selector now potentially including non-Stripe active subs; stale JSDoc still mentioning "active Stripe subscriptions") aren't testable from the preview — they're worth a note from @arpitgupta1214 on what populates the subscriptions table today.
No blocking issues from my side.
2 participants
Admin-scoped migration of legacy
GET /api/artists/prointomono/api— returns the deduped list of artist IDs owned by pro-tier accounts (enterprise email domain or active Stripe subscription).Test plan
api.recoupable.com/api/artists/pro(requires tasks service account to be flagged admin)Summary by cubic
Migrated GET /api/admins/artists/pro into
mono/apias an admin-only endpoint that returns a deduped list of pro artist IDs from enterprise-domain emails and active subscriptions in the Supabase mirror. Sources resolve to account IDs, then map to unique artist IDs.New Features
account_idthenartist_id; short-circuits on empty.Refactors
subscriptionsmirror and introducedselectSubscriptions({ accountIds?, active? }).account_ids; merged/deduped once ingetProArtists, then a singleaccount_artist_idsquery;selectAccountEmailsadds an optional domain filter (ilike %@<domain>%).Written for commit 6b5057f. Summary will update on new commits.
Summary by CodeRabbit