feat(api): migrate GET /api/subscriptions to /api/accounts/{id}/subscription#476
Closed
arpitgupta1214 wants to merge 4 commits into
Closed
feat(api): migrate GET /api/subscriptions to /api/accounts/{id}/subscription#476arpitgupta1214 wants to merge 4 commits into
arpitgupta1214 wants to merge 4 commits into
Conversation
…ription
Ports the Express /api/subscriptions?accountId= handler to a nested
/api/accounts/{id}/subscription route with validateAuthContext +
canAccessAccount access check. Seeds lib/stripe/* and lib/enterprise/*,
adds the subscriptions domain, and pins the Stripe SDK apiVersion to
match chat. Response body is byte-identical to the legacy endpoint.
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 17 minutes and 56 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 (4)
📒 Files selected for processing (7)
✨ 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.
8 issues found across 13 files
Confidence score: 3/5
- There is a moderate merge risk: several high-confidence issues (6–7/10) are user-impacting, especially around subscription state resolution and error handling.
- In
lib/stripe/getActiveSubscriptions.ts, missing explicit status filtering plus returning[]on Stripe failures can misreport inactive/error states as "no active subscription," which can drive incorrect 404/empty responses. - In
lib/subscriptions/getSubscriptionsHandler.tsandlib/stripe/getActiveSubscriptionDetails.ts, treating failures as "not found" (404ornull) can mask operational/query problems and make debugging and recovery harder. - Pay close attention to
app/api/accounts/[id]/subscription/route.ts,lib/stripe/getActiveSubscriptions.ts,lib/subscriptions/getSubscriptionsHandler.ts, andlib/stripe/getActiveSubscriptionDetails.ts- routing/export contract and failure-path handling may misclassify real errors as missing subscriptions.
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:25">
P2: Custom agent: **Module should export a single primary function whose name matches the filename**
`client.ts` exports a default object instance instead of a primary function named to match the filename (`client`).</violation>
</file>
<file name="lib/subscriptions/__tests__/getSubscriptionsHandler.test.ts">
<violation number="1" location="lib/subscriptions/__tests__/getSubscriptionsHandler.test.ts:1">
P3: Custom agent: **Enforce Clear Code Style and Maintainability Practices**
New file exceeds the 100-line file-size limit required by Rule 3.</violation>
</file>
<file name="lib/subscriptions/__tests__/validateGetSubscriptionRequest.test.ts">
<violation number="1" location="lib/subscriptions/__tests__/validateGetSubscriptionRequest.test.ts:1">
P3: Custom agent: **Enforce Clear Code Style and Maintainability Practices**
File exceeds the repository’s 100-line maintainability cap.</violation>
</file>
<file name="app/api/accounts/[id]/subscription/route.ts">
<violation number="1" location="app/api/accounts/[id]/subscription/route.ts:10">
P1: Custom agent: **Module should export a single primary function whose name matches the filename**
Module exports multiple top-level functions and none matches the `route.ts` basename, violating the single-primary-export naming rule.</violation>
</file>
<file name="lib/subscriptions/getSubscriptionsHandler.ts">
<violation number="1" location="lib/subscriptions/getSubscriptionsHandler.ts:34">
P2: Do not return `404 Account not found` from an empty `account_emails` result; it can mask query failures and mislabel existing accounts.</violation>
</file>
<file name="lib/stripe/getActiveSubscriptionDetails.ts">
<violation number="1" location="lib/stripe/getActiveSubscriptionDetails.ts:17">
P2: Do not convert exceptions to `null` here; it makes operational failures indistinguishable from "no active subscription" and can return an incorrect 404.</violation>
</file>
<file name="lib/stripe/getActiveSubscriptions.ts">
<violation number="1" location="lib/stripe/getActiveSubscriptions.ts:21">
P1: Add an explicit status filter; this query can return non-active subscriptions as active.</violation>
<violation number="2" location="lib/stripe/getActiveSubscriptions.ts:30">
P2: Do not return an empty array on Stripe errors; it masks failures as "no subscription".</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client
participant API as Next.js Route Handler
participant Auth as Auth & Permissions
participant DB as Supabase DB
participant Stripe as Stripe API
Note over Client,Stripe: GET /api/accounts/{id}/subscription
Client->>API: NEW: Request with Account ID and Auth Header
API->>API: NEW: Validate ID is UUID
API->>Auth: NEW: validateAuthContext(request)
alt Unauthorized (401)
Auth-->>Client: Return 401 Unauthorized
else Authorized
Auth-->>API: Return Requester accountId
end
API->>DB: NEW: selectAccounts(targetId)
alt Not Found (404)
DB-->>Client: Return 404 Account not found
end
DB-->>API: Account Record
API->>Auth: NEW: canAccessAccount(requesterId, targetId)
alt Access Denied (403)
Auth-->>Client: Return 403 Forbidden
end
Auth-->>API: Access Granted
API->>DB: NEW: selectAccountEmails(targetId)
DB-->>API: List of emails
API->>API: NEW: isEnterprise(emails) check via domain allow-list
alt NEW: Enterprise Domain Found
API-->>Client: 200 OK { isEnterprise: true }
else NEW: Standard Account
API->>Stripe: NEW: subscriptions.list({ limit: 100 })
Stripe-->>API: Active subscriptions
API->>API: NEW: Filter by metadata.accountId
alt Subscription Found
API-->>Client: 200 OK { subscription } (Stripe raw payload)
else No Active Subscription
API-->>Client: 404 Not Found { error: "No active subscription found" }
end
end
Note over API,Stripe: Safe Error Handling: Stripe/DB errors return 500 without leaking details
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| * | ||
| * @returns A NextResponse with CORS headers. | ||
| */ | ||
| export async function OPTIONS() { |
There was a problem hiding this comment.
P1: Custom agent: Module should export a single primary function whose name matches the filename
Module exports multiple top-level functions and none matches the route.ts basename, violating the single-primary-export naming rule.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/api/accounts/[id]/subscription/route.ts, line 10:
<comment>Module exports multiple top-level functions and none matches the `route.ts` basename, violating the single-primary-export naming rule.</comment>
<file context>
@@ -0,0 +1,34 @@
+ *
+ * @returns A NextResponse with CORS headers.
+ */
+export async function OPTIONS() {
+ return new NextResponse(null, {
+ status: 200,
</file context>
| apiVersion: STRIPE_API_VERSION as Stripe.LatestApiVersion, | ||
| }); | ||
|
|
||
| export default stripeClient; |
There was a problem hiding this comment.
P2: Custom agent: Module should export a single primary function whose name matches the filename
client.ts exports a default object instance instead of a primary function named to match the filename (client).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/stripe/client.ts, line 25:
<comment>`client.ts` exports a default object instance instead of a primary function named to match the filename (`client`).</comment>
<file context>
@@ -0,0 +1,25 @@
+ apiVersion: STRIPE_API_VERSION as Stripe.LatestApiVersion,
+});
+
+export default stripeClient;
</file context>
| @@ -0,0 +1,120 @@ | |||
| import { describe, it, expect, vi, beforeEach } from "vitest"; | |||
There was a problem hiding this comment.
P3: Custom agent: Enforce Clear Code Style and Maintainability Practices
New file exceeds the 100-line file-size limit required by Rule 3.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/subscriptions/__tests__/getSubscriptionsHandler.test.ts, line 1:
<comment>New file exceeds the 100-line file-size limit required by Rule 3.</comment>
<file context>
@@ -0,0 +1,120 @@
+import { describe, it, expect, vi, beforeEach } from "vitest";
+import { NextRequest, NextResponse } from "next/server";
+
</file context>
| @@ -0,0 +1,125 @@ | |||
| import { describe, it, expect, vi, beforeEach } from "vitest"; | |||
There was a problem hiding this comment.
P3: Custom agent: Enforce Clear Code Style and Maintainability Practices
File exceeds the repository’s 100-line maintainability cap.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/subscriptions/__tests__/validateGetSubscriptionRequest.test.ts, line 1:
<comment>File exceeds the repository’s 100-line maintainability cap.</comment>
<file context>
@@ -0,0 +1,125 @@
+import { describe, it, expect, vi, beforeEach } from "vitest";
+import { NextRequest, NextResponse } from "next/server";
+
</file context>
…validator type - getActiveSubscriptions (list 100 + client-side filter) -> getActiveSubscription using stripe.subscriptions.search with metadata+status filter (scales past 100) - Drop getActiveSubscriptionDetails wrapper, inline nullable handling - Validator returns derived GetSubscriptionParams directly (drop extra interface)
…sserted in validator)
There was a problem hiding this comment.
1 issue found across 9 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/getActiveSubscription.ts">
<violation number="1" location="lib/stripe/getActiveSubscription.ts:22">
P1: Do not swallow Stripe errors as `null`; this turns upstream failures into false 404 responses.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…lper Discriminated return (enterprise | subscription | null) collapses orchestration out of the handler. Owns emails lookup + enterprise check + Stripe search. Tests kept at handler + validator level only.
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/subscriptions/getAccountSubscription.ts">
<violation number="1" location="lib/subscriptions/getAccountSubscription.ts:35">
P1: Do not return `null` from the catch block here; it causes backend failures to be reported as 404 "No active subscription found" instead of a 500 error.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| return subscription ? { subscription } : null; | ||
| } catch (error) { | ||
| console.error("[ERROR] getAccountSubscription:", error); | ||
| return null; |
There was a problem hiding this comment.
P1: Do not return null from the catch block here; it causes backend failures to be reported as 404 "No active subscription found" instead of a 500 error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/subscriptions/getAccountSubscription.ts, line 35:
<comment>Do not return `null` from the catch block here; it causes backend failures to be reported as 404 "No active subscription found" instead of a 500 error.</comment>
<file context>
@@ -0,0 +1,37 @@
+ return subscription ? { subscription } : null;
+ } catch (error) {
+ console.error("[ERROR] getAccountSubscription:", error);
+ return null;
+ }
+}
</file context>
3 tasks
Collaborator
Author
|
Closing — no monorepo caller hits this endpoint (chat's helper was dead code) and we're removing the endpoint from public docs (recoupable/docs#159) instead of migrating it. |
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.
Ports the Express
/api/subscriptions?accountId=handler to a nestedGET /api/accounts/{id}/subscriptionwithvalidateAuthContext+canAccessAccountaccess check; seedslib/stripe/*,lib/enterprise/*, and thesubscriptionsdomain. Response body is byte-identical to the legacy endpoint (thesubscriptionpayload keeps camelCase since it's a rawStripe.Subscription).Test plan
pnpm test lib/subscriptions lib/stripepasses (17 tests)pnpm testpasses (2238 tests)curl -H "x-api-key: $RECOUP_TEST_API_KEY" <preview>/api/accounts/<uuid>/subscriptionreturns 200 with{ isEnterprise: true }for an enterprise account and{ subscription }for a paid accountSummary by cubic
Migrated the legacy
GET /api/subscriptions?accountId=toGET /api/accounts/{id}/subscriptionwith proper auth and access checks. The response stays byte-identical, including camelCase on the Stripesubscription, and supports enterprise short-circuiting.New Features
OPTIONSpreflight.{ isEnterprise: true }without Stripe.stripeAPI version; tests added for handler and validator.Refactors
subscriptions.searchto scale past 100 active subs.getAccountSubscription(returnsenterprise | subscription | null); handler now just validates and formats the response.Written for commit 3f922d4. Summary will update on new commits.