Closed
Conversation
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 14 minutes and 2 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 (2)
📝 WalkthroughWalkthroughAdded a new dynamic API endpoint (/api/accounts/emails) with CORS preflight handling, request validation and auth checks, parallelized access verification, and email selection from the Supabase layer; responses include CORS headers and explicit error handling. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Route as "Route /api/accounts/emails"
participant Validator as "validateGetAccountEmailsQuery"
participant AuthCheck as "checkAccountAccess (parallel)"
participant DB as "selectAccountEmails"
participant Responder as "Response Builder"
Client->>Route: GET /api/accounts/emails?account_id=...
Route->>Validator: validateGetAccountEmailsQuery(request)
Validator->>Validator: validateAuthContext + extract accountIds
alt Validation returns NextResponse
Validator-->>Route: NextResponse (auth/400)
Route-->>Client: NextResponse + CORS headers
else Validated query
Validator-->>Route: ValidatedGetAccountEmailsQuery
Route->>AuthCheck: run checkAccountAccess for each accountId (Promise.all)
AuthCheck-->>Route: all access results
alt any access denied
Route->>Responder: build 403 JSON + CORS
Responder-->>Client: 403 Unauthorized
else all access granted
Route->>DB: selectAccountEmails({ accountIds })
DB-->>Route: email records
Route->>Responder: build 200 JSON + CORS
Responder-->>Client: 200 OK with emails
end
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/api/accounts/emails/route.ts (2)
16-28: Add@paramand@returnsto GET handler JSDoc.The static analysis correctly identifies missing JSDoc declarations. Completing these improves API documentation consistency.
📝 Proposed JSDoc fix
/** * GET /api/accounts/emails * * Retrieves account email rows for the requested account IDs after verifying * that the authenticated caller has access to the provided artist account. * * Query parameters: * - artist_account_id (required): Artist account used for access checks * - account_id (optional, repeatable): Account IDs to look up + * + * `@param` request - The incoming Next.js request object + * `@returns` NextResponse with account emails array or error */ export async function GET(request: NextRequest) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/accounts/emails/route.ts` around lines 16 - 28, The JSDoc block above the exported GET handler is missing `@param` and `@returns` tags; update the comment for the GET function (which delegates to getAccountEmailsHandler) to include a `@param` {NextRequest} request describing the request and a `@returns` {Promise<Response>} describing the returned response so static analysis and generated docs include the parameter and return types/semantics.
6-14: Complete JSDoc with@returnsdeclaration.Static analysis flags the missing
@returnsdocumentation. For API routes, documenting the response helps maintain consistency.📝 Proposed JSDoc fix
/** * OPTIONS handler for CORS preflight requests. + * `@returns` NextResponse with 200 status and CORS headers */ export async function OPTIONS() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/accounts/emails/route.ts` around lines 6 - 14, The JSDoc for the OPTIONS handler (exported function OPTIONS) is missing a `@returns` tag; update its block comment to include a `@returns` that describes the NextResponse returned (status 200 with CORS headers) so static analysis is satisfied. Ensure the `@returns` mentions the NextResponse object and the fact it returns a 200 response for CORS preflight with headers from getCorsHeaders(), and keep the description concise and consistent with the existing comment.lib/accounts/validateGetAccountEmailsQuery.ts (1)
6-10: Consider using Zod for query parameter validation.Per coding guidelines, validate functions should use Zod for schema validation and export both the schema and inferred TypeScript type. The current implementation uses manual validation which works correctly, but adopting Zod would:
- Provide declarative, self-documenting validation rules
- Enable automatic type inference from the schema
- Maintain consistency with other validate functions in the codebase
♻️ Proposed Zod-based implementation
import type { NextRequest } from "next/server"; import { NextResponse } from "next/server"; import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; import { validateAuthContext } from "@/lib/auth/validateAuthContext"; +import { z } from "zod"; -export interface ValidatedGetAccountEmailsQuery { - authenticatedAccountId: string; - artistAccountId: string; - accountIds: string[]; -} +export const getAccountEmailsQuerySchema = z.object({ + artist_account_id: z.string().min(1, "artist_account_id parameter is required"), + account_id: z.array(z.string()).optional().default([]), +}); + +export type ValidatedGetAccountEmailsQuery = z.infer<typeof getAccountEmailsQuerySchema> & { + authenticatedAccountId: string; +}; /** * Validates auth and query params for GET /api/accounts/emails. */ export async function validateGetAccountEmailsQuery( request: NextRequest, ): Promise<NextResponse | ValidatedGetAccountEmailsQuery> { const authResult = await validateAuthContext(request); if (authResult instanceof NextResponse) { return authResult; } const { searchParams } = new URL(request.url); - const artistAccountId = searchParams.get("artist_account_id"); - - if (!artistAccountId) { - return NextResponse.json( - { error: "artist_account_id parameter is required" }, - { - status: 400, - headers: getCorsHeaders(), - }, - ); - } - - return { - authenticatedAccountId: authResult.accountId, - artistAccountId, - accountIds: searchParams.getAll("account_id"), - }; + const parseResult = getAccountEmailsQuerySchema.safeParse({ + artist_account_id: searchParams.get("artist_account_id"), + account_id: searchParams.getAll("account_id"), + }); + + if (!parseResult.success) { + return NextResponse.json( + { error: parseResult.error.issues[0]?.message ?? "Invalid query parameters" }, + { status: 400, headers: getCorsHeaders() }, + ); + } + + return { + authenticatedAccountId: authResult.accountId, + artistAccountId: parseResult.data.artist_account_id, + accountIds: parseResult.data.account_id, + }; }As per coding guidelines: "For validation functions, ensure: Use Zod for schema validation, Return NextResponse on error or validated data on success, Export inferred types for validated data."
Also applies to: 23-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/accounts/validateGetAccountEmailsQuery.ts` around lines 6 - 10, The current manual validation that yields the ValidatedGetAccountEmailsQuery shape should be replaced with a Zod schema: define and export a zod schema (e.g., getAccountEmailsQuerySchema) that enforces authenticatedAccountId and artistAccountId as strings and accountIds as string array, export the inferred TypeScript type (e.g., type ValidatedGetAccountEmailsQuery = z.infer<typeof getAccountEmailsQuerySchema>), and update the validate function to parse the incoming query with schema.safeParse or try/catch and return a NextResponse on validation error or the validated data on success; update any references to use the new schema and exported inferred type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/api/accounts/emails/route.ts`:
- Around line 16-28: The JSDoc block above the exported GET handler is missing
`@param` and `@returns` tags; update the comment for the GET function (which
delegates to getAccountEmailsHandler) to include a `@param` {NextRequest} request
describing the request and a `@returns` {Promise<Response>} describing the
returned response so static analysis and generated docs include the parameter
and return types/semantics.
- Around line 6-14: The JSDoc for the OPTIONS handler (exported function
OPTIONS) is missing a `@returns` tag; update its block comment to include a
`@returns` that describes the NextResponse returned (status 200 with CORS headers)
so static analysis is satisfied. Ensure the `@returns` mentions the NextResponse
object and the fact it returns a 200 response for CORS preflight with headers
from getCorsHeaders(), and keep the description concise and consistent with the
existing comment.
In `@lib/accounts/validateGetAccountEmailsQuery.ts`:
- Around line 6-10: The current manual validation that yields the
ValidatedGetAccountEmailsQuery shape should be replaced with a Zod schema:
define and export a zod schema (e.g., getAccountEmailsQuerySchema) that enforces
authenticatedAccountId and artistAccountId as strings and accountIds as string
array, export the inferred TypeScript type (e.g., type
ValidatedGetAccountEmailsQuery = z.infer<typeof getAccountEmailsQuerySchema>),
and update the validate function to parse the incoming query with
schema.safeParse or try/catch and return a NextResponse on validation error or
the validated data on success; update any references to use the new schema and
exported inferred type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9043f468-02ba-41b7-837f-be4b2804b354
⛔ Files ignored due to path filters (2)
lib/accounts/__tests__/getAccountEmailsHandler.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/accounts/__tests__/validateGetAccountEmailsQuery.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (4)
app/api/accounts/emails/route.tslib/accounts/getAccountEmailsHandler.tslib/accounts/validateGetAccountEmailsQuery.tslib/supabase/account_emails/selectAccountEmails.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 6 files
Confidence score: 3/5
- There is a concrete regression risk in
lib/accounts/getAccountEmailsHandler.ts: the emptyaccountIdsearly return bypassescheckAccountArtistAccess, so unauthorized callers can receive200 []instead of the expected403. - The issue in
lib/accounts/validateGetAccountEmailsQuery.tsis medium severity: validation errors omitstatusandmissing_fields, which can break clients expecting the project’s standard error contract. - Given a high-confidence authorization-path issue (6/10) plus an API-shape inconsistency (5/10), this carries some user-facing risk and is better fixed before merge.
- Pay close attention to
lib/accounts/getAccountEmailsHandler.tsandlib/accounts/validateGetAccountEmailsQuery.ts- restore access checks before early returns and align validation error payload shape.
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/accounts/validateGetAccountEmailsQuery.ts">
<violation number="1" location="lib/accounts/validateGetAccountEmailsQuery.ts:28">
P2: Error response is missing `status` and `missing_fields` fields, deviating from the standard error shape used by all other validation functions. Consider using a Zod schema with `safeParse` to match the project convention (see `validateGetTasksQuery.ts` or `validateGetCatalogsQuery.ts` for the pattern).</violation>
</file>
<file name="lib/accounts/getAccountEmailsHandler.ts">
<violation number="1" location="lib/accounts/getAccountEmailsHandler.ts:17">
P2: Authorization bypass: the empty-`accountIds` early return skips the `checkAccountArtistAccess` check. An authenticated caller without artist access gets `200 []` instead of `403`. Move the empty-array shortcut after the access check to enforce consistent authorization.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client
participant Route as API Route (Next.js)
participant Handler as Account Emails Handler
participant Auth as Auth Context
participant Access as Artist Access Service
participant DB as Supabase (account_emails)
Note over Client,DB: NEW: Account Email Retrieval Flow
Client->>Route: GET /api/accounts/emails?artist_account_id=X&account_id=Y
Route->>Handler: getAccountEmailsHandler(request)
Handler->>Auth: NEW: validateAuthContext(request)
alt Invalid Auth
Auth-->>Handler: 401 Unauthorized
Handler-->>Client: 401 Unauthorized
else Valid Auth
Auth-->>Handler: { accountId }
end
Handler->>Handler: NEW: validateGetAccountEmailsQuery()
opt artist_account_id missing
Handler-->>Client: 400 Bad Request
end
alt No account_ids provided
Handler-->>Client: 200 [] (Empty set)
else account_ids present
Handler->>Access: NEW: checkAccountArtistAccess(authId, artistId)
alt Access Denied
Access-->>Handler: false
Handler-->>Client: 403 Forbidden
else Access Granted
Access-->>Handler: true
Handler->>DB: CHANGED: selectAccountEmails({ accountIds })
DB-->>Handler: List of account_email rows
Note right of Handler: Includes CORS headers
Handler-->>Client: 200 OK (JSON rows)
end
end
Note over Handler,Client: All responses include getCorsHeaders()
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
There was a problem hiding this comment.
1 issue found across 5 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/accounts/getAccountEmailsHandler.ts">
<violation number="1" location="lib/accounts/getAccountEmailsHandler.ts:25">
P1: Unbounded `Promise.all` over user-supplied `accountIds` can trigger excessive concurrent DB queries. Each `checkAccountAccess` call runs up to 4 DB queries, so N account IDs → up to 4N concurrent queries with no cap. Consider adding a reasonable upper limit on `accountIds.length` (e.g., in the validation step) or batching the access checks.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
Collaborator
Author
|
Latest preview smoke test on the current account-based contract: Preview URL: Verified responses:
This matches the current implementation:
|
Collaborator
Author
|
Closing this migration PR. We are switching to a new plan for the account emails change. |
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
Testing
Summary by cubic
Adds GET
/api/accounts/emailsto fetch account email rows by account ID with per-account authorization. Requires at least oneaccount_idand validates access up front; responses are CORS-enabled and not cached.New Features
GET /api/accounts/emailswith repeatableaccount_id; returns rawaccount_emailsrows (empty array if no matches).account_id; returns 400 with an error when missing.OPTIONSfor CORS;dynamic = "force-dynamic",fetchCache = "force-no-store",revalidate = 0.Refactors
checkAccountAccess; returns 403 if any requested account is unauthorized; auth derives fromvalidateAuthContext.account_id; validation now enforces at least one ID.selectAccountEmailsparameters.Written for commit 1184ae9. Summary will update on new commits.
Summary by CodeRabbit