feat(api): migrate GET /api/credits/get + GET /api/subscription/status#500
Open
arpitgupta1214 wants to merge 3 commits intotestfrom
Open
feat(api): migrate GET /api/credits/get + GET /api/subscription/status#500arpitgupta1214 wants to merge 3 commits intotestfrom
arpitgupta1214 wants to merge 3 commits intotestfrom
Conversation
Group 3 (Billing read) of the chat→api migration plan. Ports chat's
account-scoped read endpoints into recoup-api with 1:1 response shapes
so chat-side cutover is a base-URL swap.
- New routes:
- GET /api/credits/get?accountId=… → { data: CreditsUsage | null }
- GET /api/subscription/status?accountId=… → { isPro: boolean }
- Handlers + Zod query validators in lib/credits and lib/subscription.
- Ports stripe helpers (getActiveSubscriptions, getActiveSubscriptionDetails,
isActiveSubscription, getOrgSubscription) into lib/stripe and the
monthly-refill checkAndResetCredits into lib/credits, reusing existing
lib/supabase/credits_usage and lib/supabase/account_organization_ids.
- Endpoints unauthenticated to mirror chat parity. STRIPE_SK already wired.
- 9 new vitest cases. Full suite 2379/2379 green; lint + format clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 (9)
✨ 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. Review rate limit: 0/1 reviews remaining, refill in 16 minutes and 12 seconds.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 13 files
Confidence score: 2/5
- There are concrete, high-impact correctness issues:
lib/stripe/isActiveSubscription.tscan treatstatus === "canceled"as active, andlib/stripe/getActiveSubscriptions.tsonly checks the first Stripe page, which can falsely report subscribed/unsubscribed state for real users. - Two handlers (
lib/subscription/getSubscriptionStatusHandler.tsandlib/credits/validateCreditsGetQuery.ts) appear to trust caller-providedaccountIdquery params, creating a likely account-scoping/access-control risk unless ownership is enforced elsewhere. - Given multiple 7–8/10 issues with high confidence and user-facing/security impact, this looks high risk to merge before fixes.
- Pay close attention to
lib/subscription/getSubscriptionStatusHandler.ts,lib/credits/validateCreditsGetQuery.ts,lib/stripe/isActiveSubscription.ts,lib/stripe/getActiveSubscriptions.ts- account authorization and subscription-state logic can produce unauthorized access or incorrect billing status.
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/isActiveSubscription.ts">
<violation number="1" location="lib/stripe/isActiveSubscription.ts:10">
P1: `isActiveSubscription` can return `true` for subscriptions with `status === "canceled"`, which misclassifies canceled subscriptions as active.</violation>
</file>
<file name="lib/stripe/getActiveSubscriptionDetails.ts">
<violation number="1" location="lib/stripe/getActiveSubscriptionDetails.ts:11">
P3: This `try/catch` is redundant because `getActiveSubscriptions` already catches and suppresses errors, so the local `catch` branch is effectively unreachable.</violation>
</file>
<file name="lib/subscription/getSubscriptionStatusHandler.ts">
<violation number="1" location="lib/subscription/getSubscriptionStatusHandler.ts:17">
P1: Do not trust `accountId` from query params for this handler. Derive account identity from authenticated context (or verify the caller owns the requested account) before fetching subscription data.</violation>
</file>
<file name="lib/credits/validateCreditsGetQuery.ts">
<violation number="1" location="lib/credits/validateCreditsGetQuery.ts:14">
P1: Do not accept `accountId` from query parameters for account scoping; derive it from authenticated context to avoid caller-controlled account access.</violation>
</file>
<file name="lib/stripe/getActiveSubscriptions.ts">
<violation number="1" location="lib/stripe/getActiveSubscriptions.ts:12">
P1: This only reads the first Stripe page (`limit: 100`) before filtering by account, so accounts can be reported as unsubscribed when their subscription is on a later page.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client
participant API as Next.js API Route
participant Handler as Credits/Sub Handlers
participant DB as Supabase
participant Stripe as Stripe API
Note over Client, Stripe: NEW: Billing Read Migration Flow (Account Scoped)
Client->>API: GET /api/credits/get?accountId=...
API->>Handler: NEW: getCreditsHandler()
Handler->>Handler: Validate accountId (Zod)
alt Invalid accountId
Handler-->>Client: 400 Bad Request
end
Handler->>DB: selectCreditsUsage(accountId)
DB-->>Handler: Current credits + last timestamp
par Subscription Check
Handler->>Stripe: NEW: getActiveSubscriptionDetails(accountId)
Stripe-->>Handler: User's Subscription
and Organization Check
Handler->>DB: getAccountOrganizations(accountId)
DB-->>Handler: List of Org IDs
loop For each Org
Handler->>Stripe: getActiveSubscriptionDetails(orgId)
Stripe-->>Handler: Org's Subscription
end
end
Handler->>Handler: NEW: checkAndResetCredits Logic
Note over Handler: Determine if isPro (User OR Org sub)<br/>Check if monthly cycle or new sub started
opt Refill Needed (Monthly or New Pro Sub)
Handler->>DB: NEW: updateCreditsUsage(remaining_credits, timestamp)
DB-->>Handler: Updated Row
end
Handler-->>Client: 200 OK + Data + CORS Headers
Note over Client, Stripe: NEW: Subscription Status Flow
Client->>API: GET /api/subscription/status?accountId=...
API->>Handler: NEW: getSubscriptionStatusHandler()
par Parallel Sub Check
Handler->>Stripe: getActiveSubscriptionDetails(accountId)
Handler->>Handler: getOrgSubscription(accountId)
end
Handler->>Handler: isActiveSubscription() check
Handler-->>Client: 200 OK { isPro: boolean }
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (!subscription) return false; | ||
| const isTrial = subscription?.status === "trialing"; | ||
| const isCanceledTrial = isTrial && subscription?.canceled_at; | ||
| const subscriptionActive = !isCanceledTrial; |
There was a problem hiding this comment.
P1: isActiveSubscription can return true for subscriptions with status === "canceled", which misclassifies canceled subscriptions as active.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/stripe/isActiveSubscription.ts, line 10:
<comment>`isActiveSubscription` can return `true` for subscriptions with `status === "canceled"`, which misclassifies canceled subscriptions as active.</comment>
<file context>
@@ -0,0 +1,14 @@
+ if (!subscription) return false;
+ const isTrial = subscription?.status === "trialing";
+ const isCanceledTrial = isTrial && subscription?.canceled_at;
+ const subscriptionActive = !isCanceledTrial;
+ return subscriptionActive;
+};
</file context>
Comment on lines
+12
to
+21
| limit: 100, | ||
| current_period_end: { | ||
| gt: parseInt(Number(Date.now() / 1000).toFixed(0), 10), | ||
| }, | ||
| }); | ||
|
|
||
| const activeSubscriptions = subscriptions?.data?.filter( | ||
| (subscription: Stripe.Subscription) => subscription.metadata?.accountId === accountId, | ||
| ); | ||
|
|
There was a problem hiding this comment.
P1: This only reads the first Stripe page (limit: 100) before filtering by account, so accounts can be reported as unsubscribed when their subscription is on a later page.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/stripe/getActiveSubscriptions.ts, line 12:
<comment>This only reads the first Stripe page (`limit: 100`) before filtering by account, so accounts can be reported as unsubscribed when their subscription is on a later page.</comment>
<file context>
@@ -0,0 +1,27 @@
+export const getActiveSubscriptions = async (accountId: string): Promise<Stripe.Subscription[]> => {
+ try {
+ const subscriptions = await stripeClient.subscriptions.list({
+ limit: 100,
+ current_period_end: {
+ gt: parseInt(Number(Date.now() / 1000).toFixed(0), 10),
</file context>
Suggested change
| limit: 100, | |
| current_period_end: { | |
| gt: parseInt(Number(Date.now() / 1000).toFixed(0), 10), | |
| }, | |
| }); | |
| const activeSubscriptions = subscriptions?.data?.filter( | |
| (subscription: Stripe.Subscription) => subscription.metadata?.accountId === accountId, | |
| ); | |
| let subscriptions: Stripe.Subscription[] = []; | |
| let startingAfter: string | undefined; | |
| do { | |
| const page = await stripeClient.subscriptions.list({ | |
| limit: 100, | |
| current_period_end: { | |
| gt: Math.floor(Date.now() / 1000), | |
| }, | |
| ...(startingAfter ? { starting_after: startingAfter } : {}), | |
| }); | |
| subscriptions = subscriptions.concat(page.data); | |
| startingAfter = page.has_more ? page.data[page.data.length - 1]?.id : undefined; | |
| } while (startingAfter); | |
| const activeSubscriptions = subscriptions.filter( | |
| (subscription: Stripe.Subscription) => subscription.metadata?.accountId === accountId, | |
| ); |
| export const getActiveSubscriptionDetails = async ( | ||
| accountId: string, | ||
| ): Promise<Stripe.Subscription | null> => { | ||
| try { |
There was a problem hiding this comment.
P3: This try/catch is redundant because getActiveSubscriptions already catches and suppresses errors, so the local catch branch is effectively unreachable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/stripe/getActiveSubscriptionDetails.ts, line 11:
<comment>This `try/catch` is redundant because `getActiveSubscriptions` already catches and suppresses errors, so the local `catch` branch is effectively unreachable.</comment>
<file context>
@@ -0,0 +1,18 @@
+export const getActiveSubscriptionDetails = async (
+ accountId: string,
+): Promise<Stripe.Subscription | null> => {
+ try {
+ const activeSubscriptions = await getActiveSubscriptions(accountId);
+ return activeSubscriptions.length > 0 ? activeSubscriptions[0] : null;
</file context>
Renames the two new endpoints to follow api repo conventions (plural collections, no verb suffixes) and derives accountId from validateAuthContext instead of an accountId query param — matching the group 4 stripe outbound migration. - /api/credits/get → /api/credits - /api/subscription/status → /api/subscriptions/status - Drops validate*Query.ts; handlers call validateAuthContext directly. - Tests updated; full suite 2379/2379 green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The endpoint is the singleton "current subscription status for the authenticated account" — no resource collection makes sense here, so a singular path without a sub-resource verb reads better than /api/subscriptions/status. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Group 3 (Billing read) of the chat→api migration plan. Adds parity for chat's account-scoped reads behind
usePayment, so chat-side cutover is a base-URL swap.Test plan
pnpm test— 2379/2379 green locallypnpm lint+pnpm format:checkclean/api/credits/get?accountId=…and/api/subscription/status?accountId=…