Conversation
|
Warning Rate limit exceeded
⌛ 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. 📝 WalkthroughWalkthroughThis PR adds a new "lifetime" plan tier with expandable credit balances, touching DB migrations, plan config, Polar integration (checkout/webhook), AI access logic (credit checks and deductions), and subscription service APIs. Changes
Sequence DiagramsequenceDiagram
actor User
participant Checkout as Polar Checkout
participant Webhook as Polar Webhook
participant SubSvc as Subscription Service
participant DB as Database
participant AccessSvc as AI Access Service
User->>Checkout: Purchase lifetime product
Checkout->>Webhook: onOrderPaid (lifetime product)
Webhook->>SubSvc: create/upsert lifetime subscription (customer externalId)
SubSvc->>DB: insert/upsert subscription (tier=lifetime, creditBalance=initial)
DB-->>SubSvc: subscription persisted
User->>AccessSvc: Request AI access
AccessSvc->>DB: fetch subscription + creditBalance
DB-->>AccessSvc: tier=lifetime, creditBalance=X
alt creditBalance > 0
AccessSvc-->>User: allowed=true, creditBalance=X
User->>AccessSvc: send usage (cost)
AccessSvc->>SubSvc: updateCreditBalance(userId, -cost)
SubSvc->>DB: decrement credit_balance
else creditBalance ≤ 0
AccessSvc-->>User: allowed=false, reason="Insufficient credits"
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/server/services/subscription.ts (1)
67-73:⚠️ Potential issue | 🟠 MajorPotential data loss:
onConflictDoUpdatemay overwritecreditBalance.When
upsertis called withdatacontainingcreditBalance: 0(or any value), theonConflictDoUpdatespreads...datadirectly, which could unintentionally reset an existing user's credit balance to 0.For lifetime plan top-ups or subsequent webhook calls, this could erase accumulated credits.
🐛 Proposed fix to exclude creditBalance from conflict update
.onConflictDoUpdate({ target: userSubscription.userId, set: { - ...data, + ...(({ creditBalance: _ignored, ...rest }) => rest)(data), updatedAt: new Date() } });Alternatively, be more explicit about which fields should be updated:
.onConflictDoUpdate({ target: userSubscription.userId, set: { tier: data.tier, polarSubscriptionId: data.polarSubscriptionId, stripeSubscriptionId: data.stripeSubscriptionId, currentPeriodStart: data.currentPeriodStart, currentPeriodEnd: data.currentPeriodEnd, cancelAtPeriodEnd: data.cancelAtPeriodEnd, enabled: data.enabled, updatedAt: new Date() // Note: creditBalance intentionally NOT updated here } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/services/subscription.ts` around lines 67 - 73, The upsert uses onConflictDoUpdate with a spread of ...data which can overwrite existing creditBalance (e.g., set to 0); update the conflict resolution in the userSubscription upsert to exclude creditBalance from the set map (or explicitly list allowed fields to update) so creditBalance is preserved on conflict—locate the call to userSubscription.upsert and its onConflictDoUpdate and remove creditBalance from the object merged into set (or replace spreading ...data with an explicit set of fields that does not include creditBalance).
🧹 Nitpick comments (4)
.env.example (1)
41-41: Clarify and align handling of optional PUBLIC_POLAR_LIFETIME_PRODUCT_ID across files.The
.env.exampleentry is correct, but there's inconsistent handling between files:
src/lib/config/plans.ts:9-10treats this as optional with an empty string fallbacksrc/lib/server/auth.ts:11imports from$env/static/public, expecting it to be defined at build time- Checkout logic (
src/lib/server/auth.ts:100-115) includes a validation guard (&&), filtering out empty values before sending to Polar- Webhook handler (
src/lib/server/auth.ts:126) will simply not match an empty product ID against real Polar dataThis inconsistency creates confusion about whether the lifetime feature is required or truly optional. While the code won't silently assign incorrect tiers (the checkout guard and Polar's non-empty productIds prevent that), the mixed approach is error-prone for future maintenance.
Consider:
- If lifetime is optional, remove the static import from auth.ts and use consistent runtime fallbacks across both files
- If lifetime is required, remove the empty string fallback in plans.ts and add explicit validation/error handling when the env var is missing
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.example at line 41, The env var PUBLIC_POLAR_LIFETIME_PRODUCT_ID is handled inconsistently; pick one behavior and make code consistent — I recommend treating it as optional: replace the static import in auth.ts with a runtime read (same approach used in src/lib/config/plans.ts) so both files use the same fallback logic, ensure the checkout guard (the existing && filter) continues to filter out empty strings, and update any code that compares product IDs (e.g., the webhook matching logic) to tolerate an undefined/empty PUBLIC_POLAR_LIFETIME_PRODUCT_ID; alternatively, if you choose to make it required, remove the empty-string fallback in plans.ts and add an explicit startup validation that throws/logs a clear error when PUBLIC_POLAR_LIFETIME_PRODUCT_ID is missing (affecting the code paths in plans.ts and auth.ts).src/lib/components/marketing/pricing-section.svelte (1)
2-7: Reorganize imports per coding guidelines.The import statements should be ordered: External packages → SvelteKit → Internal lib → Relative imports. Currently, they are mixed.
📦 Proposed fix for import order
- import LoginPromptDialog from '$lib/components/editor/login-prompt-dialog.svelte'; - import PricingCard from './pricing-card.svelte'; - import { compareTiers, getAllPlans, type PlanConfig, type PlanTier } from '$lib/config/plans'; import { resolve } from '$app/paths'; import { goto } from '$app/navigation'; + import { compareTiers, getAllPlans, type PlanConfig, type PlanTier } from '$lib/config/plans'; + import LoginPromptDialog from '$lib/components/editor/login-prompt-dialog.svelte'; import { getUser } from '$lib/functions/auth.remote'; + import PricingCard from './pricing-card.svelte';As per coding guidelines: "Organize imports in order: External packages → SvelteKit → Internal lib → Relative imports"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/components/marketing/pricing-section.svelte` around lines 2 - 7, Reorder the import statements to follow External packages → SvelteKit → Internal lib → Relative imports; specifically move the $app imports (resolve, goto) first among app-level imports, then the internal lib imports (compareTiers, getAllPlans, PlanConfig, PlanTier from '$lib/config/plans' and getUser from '$lib/functions/auth.remote'), and keep relative imports (PricingCard and LoginPromptDialog) last so that the symbols PricingCard, LoginPromptDialog, resolve, goto, compareTiers, getAllPlans, PlanConfig, PlanTier, and getUser are grouped according to the guideline.src/lib/server/auth.ts (1)
119-139: Consider adding error handling for the customer lookup.The webhook silently proceeds if
customer.externalIdis missing, but ifpolarClient.customers.get()throws, the error will propagate and may cause webhook retries. Consider wrapping in try-catch for resilience.Additionally, the
currentPeriodEnd: new Date('2099-12-31')approach is pragmatic for lifetime plans, though you may want to document this convention.🛡️ Optional: Add error handling for customer lookup
onOrderPaid: async (payload) => { const customerId = payload.data.customerId; const productId = payload.data.productId; // Check if this is a lifetime plan purchase if (productId === PUBLIC_POLAR_LIFETIME_PRODUCT_ID) { + try { const customer = await polarClient.customers.get({ id: customerId }); if (customer.externalId) { // Create lifetime subscription (credit balance auto-initialized by upsert) await subscriptionService.upsert(customer.externalId, { tier: 'lifetime', enabled: true, currentPeriodStart: new Date(), currentPeriodEnd: new Date('2099-12-31'), // Far future date for lifetime cancelAtPeriodEnd: false }); } + } catch (err) { + console.error('[Polar] Failed to process lifetime order:', err); + throw err; // Re-throw to trigger webhook retry + } } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/auth.ts` around lines 119 - 139, Wrap the customer lookup in onOrderPaid with a try-catch: call polarClient.customers.get({ id: customerId }) inside the try, and if it throws catch the error, log it (including error details and context: customerId, productId, PUBLIC_POLAR_LIFETIME_PRODUCT_ID) and return/exit the handler early so the webhook doesn't propagate the exception; additionally, ensure you still check customer.externalId before calling subscriptionService.upsert and log a warning if externalId is missing instead of silently proceeding.src/lib/config/plans.ts (1)
36-37: Replace the-2magic value with an explicit AI billing mode.
maxCostPerMonthnow carries three meanings (>= 0,-1,-2). That is easy to misread anywhere this field is treated as a real dollar amount. A small discriminated union, or a separateaiBillingModelfield, would make downstream handling exhaustive instead of sentinel-based.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/config/plans.ts` around lines 36 - 37, The maxCostPerMonth field is overloaded (>=0, -1, -2) and should be replaced by an explicit discriminant: introduce an aiBillingModel enum/type (e.g., "MonthlyLimit" | "Unlimited" | "ExpandableCredit") and update the plan type to be a discriminated union that pairs MonthlyLimit with a numeric maxCostPerMonth and ExpandableCredit with expandableCreditBalance; update any consumers that check maxCostPerMonth for -1 or -2 to switch on aiBillingModel instead and read the appropriate field (maxCostPerMonth or expandableCreditBalance) so handling is exhaustive and no sentinel values remain (refer to maxCostPerMonth, expandableCreditBalance and the plan type in plans.ts).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/config/plans.ts`:
- Around line 189-194: The getPaidPlans function currently defaults
includeExperimental to true so PLANS.lifetime is surfaced; change its behavior
to keep experimental plans opt-in by default (set includeExperimental = false)
or add an explicit gate that only pushes PLANS.lifetime when includeExperimental
is true AND a configured product ID/server support flag is present; update the
function (getPaidPlans) to check PLANS.lifetime existence plus the new gate (or
default false) so the Lifetime plan is not shown unless the server and config
explicitly support it.
- Around line 8-10: The current PUBLIC_POLAR_LIFETIME_PRODUCT_ID coerces a
missing env var to '' which hides absent config; replace the raw cast with a Zod
parse that returns string | undefined (not an empty string) so downstream logic
can detect absence. Use
z.string().nonempty().optional().parse(import.meta.env?.PUBLIC_POLAR_LIFETIME_PRODUCT_ID)
(or an equivalent Zod schema) to validate the value once in
src/lib/config/plans.ts and assign the parsed result to
PUBLIC_POLAR_LIFETIME_PRODUCT_ID, leaving it undefined when not provided.
In `@src/lib/server/services/ai-access.ts`:
- Around line 183-188: The credit check in canUserAccessAI() and the deduction
in logAIUsage() are non-atomic and can race; make the balance check-and-deduct a
single transactional operation to prevent over-consumption. Change the flow so
logAIUsage() (or a new subscriptionService method) opens a DB transaction, locks
the user's subscription row (e.g., SELECT ... FOR UPDATE) or uses optimistic
locking (add/version column and conditional UPDATE), verifies the balance >=
estimatedCost inside the transaction, performs the balance decrement and version
increment (if used), and commits; ensure canUserAccessAI() either delegates to
this atomic method or only performs a non-authoritative pre-check for UX while
enforcement is done by the transactional update in
subscriptionService.updateCreditBalance.
---
Outside diff comments:
In `@src/lib/server/services/subscription.ts`:
- Around line 67-73: The upsert uses onConflictDoUpdate with a spread of ...data
which can overwrite existing creditBalance (e.g., set to 0); update the conflict
resolution in the userSubscription upsert to exclude creditBalance from the set
map (or explicitly list allowed fields to update) so creditBalance is preserved
on conflict—locate the call to userSubscription.upsert and its
onConflictDoUpdate and remove creditBalance from the object merged into set (or
replace spreading ...data with an explicit set of fields that does not include
creditBalance).
---
Nitpick comments:
In @.env.example:
- Line 41: The env var PUBLIC_POLAR_LIFETIME_PRODUCT_ID is handled
inconsistently; pick one behavior and make code consistent — I recommend
treating it as optional: replace the static import in auth.ts with a runtime
read (same approach used in src/lib/config/plans.ts) so both files use the same
fallback logic, ensure the checkout guard (the existing && filter) continues to
filter out empty strings, and update any code that compares product IDs (e.g.,
the webhook matching logic) to tolerate an undefined/empty
PUBLIC_POLAR_LIFETIME_PRODUCT_ID; alternatively, if you choose to make it
required, remove the empty-string fallback in plans.ts and add an explicit
startup validation that throws/logs a clear error when
PUBLIC_POLAR_LIFETIME_PRODUCT_ID is missing (affecting the code paths in
plans.ts and auth.ts).
In `@src/lib/components/marketing/pricing-section.svelte`:
- Around line 2-7: Reorder the import statements to follow External packages →
SvelteKit → Internal lib → Relative imports; specifically move the $app imports
(resolve, goto) first among app-level imports, then the internal lib imports
(compareTiers, getAllPlans, PlanConfig, PlanTier from '$lib/config/plans' and
getUser from '$lib/functions/auth.remote'), and keep relative imports
(PricingCard and LoginPromptDialog) last so that the symbols PricingCard,
LoginPromptDialog, resolve, goto, compareTiers, getAllPlans, PlanConfig,
PlanTier, and getUser are grouped according to the guideline.
In `@src/lib/config/plans.ts`:
- Around line 36-37: The maxCostPerMonth field is overloaded (>=0, -1, -2) and
should be replaced by an explicit discriminant: introduce an aiBillingModel
enum/type (e.g., "MonthlyLimit" | "Unlimited" | "ExpandableCredit") and update
the plan type to be a discriminated union that pairs MonthlyLimit with a numeric
maxCostPerMonth and ExpandableCredit with expandableCreditBalance; update any
consumers that check maxCostPerMonth for -1 or -2 to switch on aiBillingModel
instead and read the appropriate field (maxCostPerMonth or
expandableCreditBalance) so handling is exhaustive and no sentinel values remain
(refer to maxCostPerMonth, expandableCreditBalance and the plan type in
plans.ts).
In `@src/lib/server/auth.ts`:
- Around line 119-139: Wrap the customer lookup in onOrderPaid with a try-catch:
call polarClient.customers.get({ id: customerId }) inside the try, and if it
throws catch the error, log it (including error details and context: customerId,
productId, PUBLIC_POLAR_LIFETIME_PRODUCT_ID) and return/exit the handler early
so the webhook doesn't propagate the exception; additionally, ensure you still
check customer.externalId before calling subscriptionService.upsert and log a
warning if externalId is missing instead of silently proceeding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b3091340-f03a-42ee-87e2-b2bfac40f2a3
📒 Files selected for processing (11)
.env.exampledrizzle/0015_broken_barracuda.sqldrizzle/meta/0015_snapshot.jsondrizzle/meta/_journal.jsonsrc/lib/components/marketing/pricing-section.sveltesrc/lib/config/plans.tssrc/lib/schemas/subscription.tssrc/lib/server/auth.tssrc/lib/server/db/schema/ai.tssrc/lib/server/services/ai-access.tssrc/lib/server/services/subscription.ts
src/lib/config/plans.ts
Outdated
| // Optional: Lifetime plan product ID (experimental) | ||
| const PUBLIC_POLAR_LIFETIME_PRODUCT_ID = | ||
| (import.meta.env?.PUBLIC_POLAR_LIFETIME_PRODUCT_ID as string | undefined) || ''; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Validate the lifetime product ID instead of coercing it to ''.
This is external config, but the cast plus empty-string fallback silently accepts a missing value and makes the plan look configured when it is not. Parse it once with Zod and keep it undefined when absent so downstream code can disable the plan cleanly.
🛠️ Suggested change
+import { z } from 'zod';
import { PUBLIC_POLAR_CREATOR_PRODUCT_ID, PUBLIC_POLAR_PRO_PRODUCT_ID } from '$env/static/public';
-const PUBLIC_POLAR_LIFETIME_PRODUCT_ID =
- (import.meta.env?.PUBLIC_POLAR_LIFETIME_PRODUCT_ID as string | undefined) || '';
+const publicPlanEnvSchema = z.object({
+ PUBLIC_POLAR_LIFETIME_PRODUCT_ID: z.string().trim().min(1).optional()
+});
+
+const { PUBLIC_POLAR_LIFETIME_PRODUCT_ID } = publicPlanEnvSchema.parse({
+ PUBLIC_POLAR_LIFETIME_PRODUCT_ID: import.meta.env.PUBLIC_POLAR_LIFETIME_PRODUCT_ID
+});As per coding guidelines, src/lib/**/*.ts: Validate all external data with Zod schemas before using in the application.
📝 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.
| // Optional: Lifetime plan product ID (experimental) | |
| const PUBLIC_POLAR_LIFETIME_PRODUCT_ID = | |
| (import.meta.env?.PUBLIC_POLAR_LIFETIME_PRODUCT_ID as string | undefined) || ''; | |
| import { z } from 'zod'; | |
| import { PUBLIC_POLAR_CREATOR_PRODUCT_ID, PUBLIC_POLAR_PRO_PRODUCT_ID } from '$env/static/public'; | |
| // Optional: Lifetime plan product ID (experimental) | |
| const publicPlanEnvSchema = z.object({ | |
| PUBLIC_POLAR_LIFETIME_PRODUCT_ID: z.string().trim().min(1).optional() | |
| }); | |
| const { PUBLIC_POLAR_LIFETIME_PRODUCT_ID } = publicPlanEnvSchema.parse({ | |
| PUBLIC_POLAR_LIFETIME_PRODUCT_ID: import.meta.env.PUBLIC_POLAR_LIFETIME_PRODUCT_ID | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/config/plans.ts` around lines 8 - 10, The current
PUBLIC_POLAR_LIFETIME_PRODUCT_ID coerces a missing env var to '' which hides
absent config; replace the raw cast with a Zod parse that returns string |
undefined (not an empty string) so downstream logic can detect absence. Use
z.string().nonempty().optional().parse(import.meta.env?.PUBLIC_POLAR_LIFETIME_PRODUCT_ID)
(or an equivalent Zod schema) to validate the value once in
src/lib/config/plans.ts and assign the parsed result to
PUBLIC_POLAR_LIFETIME_PRODUCT_ID, leaving it undefined when not provided.
src/lib/config/plans.ts
Outdated
| export function getPaidPlans(includeExperimental = true): PlanConfig[] { | ||
| const plans = [PLANS.creator, PLANS.pro]; | ||
| if (includeExperimental && PLANS.lifetime) { | ||
| plans.push(PLANS.lifetime); | ||
| } | ||
| return plans; |
There was a problem hiding this comment.
Don't surface Lifetime by default until checkout accepts it.
With this default, Line 23 in src/lib/components/editor/pricing-dialog.svelte will render the Lifetime plan, but Lines 4-8 in src/routes/(app)/subscribe/[slug]/+server.ts still reject anything except creator and pro. That makes the new CTA fail with 400 in the current state. Keep experimental plans opt-in here, or gate them on both server support and a configured product ID.
🛠️ Suggested change
-export function getPaidPlans(includeExperimental = true): PlanConfig[] {
+export function getPaidPlans(includeExperimental = false): PlanConfig[] {
const plans = [PLANS.creator, PLANS.pro];
- if (includeExperimental && PLANS.lifetime) {
+ if (includeExperimental && PLANS.lifetime.polarProductId) {
plans.push(PLANS.lifetime);
}
return plans;
}📝 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.
| export function getPaidPlans(includeExperimental = true): PlanConfig[] { | |
| const plans = [PLANS.creator, PLANS.pro]; | |
| if (includeExperimental && PLANS.lifetime) { | |
| plans.push(PLANS.lifetime); | |
| } | |
| return plans; | |
| export function getPaidPlans(includeExperimental = false): PlanConfig[] { | |
| const plans = [PLANS.creator, PLANS.pro]; | |
| if (includeExperimental && PLANS.lifetime.polarProductId) { | |
| plans.push(PLANS.lifetime); | |
| } | |
| return plans; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/config/plans.ts` around lines 189 - 194, The getPaidPlans function
currently defaults includeExperimental to true so PLANS.lifetime is surfaced;
change its behavior to keep experimental plans opt-in by default (set
includeExperimental = false) or add an explicit gate that only pushes
PLANS.lifetime when includeExperimental is true AND a configured product
ID/server support flag is present; update the function (getPaidPlans) to check
PLANS.lifetime existence plus the new gate (or default false) so the Lifetime
plan is not shown unless the server and config explicitly support it.
|
|
||
| // For lifetime plans, deduct from credit balance | ||
| const subscription = await subscriptionService.getByUserId(userId); | ||
| if (subscription?.tier === 'lifetime') { | ||
| await subscriptionService.updateCreditBalance(userId, -estimatedCost); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
How to implement optimistic locking for credit balance deduction in PostgreSQL with Drizzle ORM?
💡 Result:
Pattern (single atomic UPDATE ... WHERE ... RETURNING)
Use a version (lock) column and make the deduction succeed only if (a) the row is still at the expected version and (b) there’s enough balance; then increment the version in the same statement. This is the core of optimistic locking in PostgreSQL. (reintech.io)
1) Table schema (Drizzle)
import { pgTable, uuid, integer, timestamp } from "drizzle-orm/pg-core";
export const accounts = pgTable("accounts", {
userId: uuid("user_id").primaryKey(),
balance: integer("balance").notNull(),
version: integer("version").notNull().default(0), // optimistic lock
updatedAt: timestamp("updated_at").notNull(),
});2) Deduct credits with optimistic locking (Drizzle ORM)
import { and, eq, gte, sql } from "drizzle-orm";
import { accounts } from "./schema";
export async function deductCredits(
db: any,
userId: string,
amount: number,
expectedVersion: number,
) {
// Optional but common: wrap in a transaction if you also write a ledger row, etc.
return await db.transaction(async (tx: any) => {
const [row] = await tx
.update(accounts)
.set({
balance: sql`${accounts.balance} - ${amount}`,
version: sql`${accounts.version} + 1`,
updatedAt: sql`now()`,
})
.where(
and(
eq(accounts.userId, userId),
eq(accounts.version, expectedVersion), // optimistic lock check
gte(accounts.balance, amount), // prevent negative balance
),
)
.returning({ balance: accounts.balance, version: accounts.version });
// If no row returned, either:
// - version mismatch (concurrent update) OR
// - insufficient funds OR
// - userId not found
if (!row) throw new Error("ConflictOrInsufficientFunds");
return row;
});
}Notes:
- Drizzle supports transactions via
db.transaction(async (tx) => ...). (orm.drizzle.team) - Drizzle supports “computed” update values via
sql\${col} - ${x}`` (their docs show this exact subtraction pattern). (orm.drizzle.team) - The optimistic-lock behavior relies on PostgreSQL row locking/MVCC: concurrent updates may wait, and the loser ends up updating 0 rows because the version no longer matches. (stackoverflow.com)
Practical retry loop (optional)
If you don’t have expectedVersion from the caller, you can implement: SELECT version,balance → attempt the update above → if it fails, re-read and retry a small number of times (the PostgreSQL docs describe this “retry-loop” approach as optimistic locking). (postgresql.org)
Citations:
- 1: https://reintech.io/blog/implementing-optimistic-locking-postgresql?utm_source=openai
- 2: https://orm.drizzle.team/docs/transactions?utm_source=openai
- 3: https://orm.drizzle.team/docs/transactions?utm_source=openai
- 4: https://stackoverflow.com/questions/43247757/postgres-strange-behavior-with-optimistic-locking?utm_source=openai
- 5: https://www.postgresql.org/files/developer/concurrency.pdf?utm_source=openai
🏁 Script executed:
wc -l src/lib/server/services/ai-access.tsRepository: epavanello/devmotion
Length of output: 104
🏁 Script executed:
# Read the target file to see lines 183-188 and surrounding context
cat -n src/lib/server/services/ai-access.ts | head -220 | tail -80Repository: epavanello/devmotion
Length of output: 2578
🏁 Script executed:
# Search for the canUserAccessAI and logAIUsage functions
rg -A 15 "canUserAccessAI|logAIUsage" src/lib/server/services/ai-access.tsRepository: epavanello/devmotion
Length of output: 977
🏁 Script executed:
# Search for updateCreditBalance to verify the GREATEST implementation
rg -A 10 "updateCreditBalance" src/lib/server/services/ai-access.tsRepository: epavanello/devmotion
Length of output: 338
🏁 Script executed:
# Look for subscription service to understand the tier checking
rg -A 5 "tier.*lifetime" src/lib/server/services/Repository: epavanello/devmotion
Length of output: 829
🏁 Script executed:
# Get the full canUserAccessAI function
rg -B 5 -A 50 "export async function canUserAccessAI" src/lib/server/services/ai-access.tsRepository: epavanello/devmotion
Length of output: 1363
🏁 Script executed:
# Find the subscription service and updateCreditBalance implementation
cat -n src/lib/server/services/subscription.ts | grep -A 20 "updateCreditBalance"Repository: epavanello/devmotion
Length of output: 1155
🏁 Script executed:
# Search for the complete updateCreditBalance method implementation
rg -A 15 "updateCreditBalance" src/lib/server/services/subscription.tsRepository: epavanello/devmotion
Length of output: 862
Race condition: credit balance check and deduction are not atomic.
The access check in canUserAccessAI() and the deduction in logAIUsage() are separate operations with no transactional guarantee. Concurrent requests can both pass the access check before either deducts, allowing users to consume more credits than available. While GREATEST(0, ...) in updateCreditBalance prevents negative balances, it doesn't prevent over-consumption—users could get "free" AI usage when concurrent requests race past the balance check.
For an experimental/MVP lifetime feature, this may be acceptable. If the feature graduates, consider one of these mitigations:
- Optimistic locking: Add a
versioncolumn; update succeeds only if the version matches and balance is sufficient, then increment version atomically in the same statement - Row-level locking: Use
SELECT ... FOR UPDATEto serialize access - Accept the risk: For small credit values, the business impact may be negligible
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/server/services/ai-access.ts` around lines 183 - 188, The credit
check in canUserAccessAI() and the deduction in logAIUsage() are non-atomic and
can race; make the balance check-and-deduct a single transactional operation to
prevent over-consumption. Change the flow so logAIUsage() (or a new
subscriptionService method) opens a DB transaction, locks the user's
subscription row (e.g., SELECT ... FOR UPDATE) or uses optimistic locking
(add/version column and conditional UPDATE), verifies the balance >=
estimatedCost inside the transaction, performs the balance decrement and version
increment (if used), and commits; ensure canUserAccessAI() either delegates to
this atomic method or only performs a non-authoritative pre-check for UX while
enforcement is done by the transactional update in
subscriptionService.updateCreditBalance.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/lib/config/plans.ts (2)
6-10:⚠️ Potential issue | 🟠 MajorValidate
PUBLIC_POLAR_LIFETIME_PRODUCT_IDbefore storing it inPLANS.This is external config, but it's assigned verbatim into
polarProductId. An empty or malformed value still makes the lifetime plan look configured, which then combines badly withgetPaidPlans()below. Parse it once with Zod and leave the fieldundefinedwhen absent. As per coding guidelines,src/lib/**/*.ts: Validate all external data with Zod schemas before using in the application.Suggested change
+import { z } from 'zod'; import { PUBLIC_POLAR_CREATOR_PRODUCT_ID, PUBLIC_POLAR_PRO_PRODUCT_ID, - PUBLIC_POLAR_LIFETIME_PRODUCT_ID + PUBLIC_POLAR_LIFETIME_PRODUCT_ID as rawLifetimeProductId } from '$env/static/public'; + +const publicPlanEnvSchema = z.object({ + PUBLIC_POLAR_LIFETIME_PRODUCT_ID: z.string().trim().min(1).optional() +}); + +const { PUBLIC_POLAR_LIFETIME_PRODUCT_ID } = publicPlanEnvSchema.parse({ + PUBLIC_POLAR_LIFETIME_PRODUCT_ID: rawLifetimeProductId || undefined +});Also applies to: 158-158
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/config/plans.ts` around lines 6 - 10, The PLANS config currently assigns PUBLIC_POLAR_LIFETIME_PRODUCT_ID verbatim into polarProductId; validate this external env with Zod (e.g., create a schema that accepts a non-empty string or undefined), parse PUBLIC_POLAR_LIFETIME_PRODUCT_ID once and if validation fails or value is empty set polarProductId to undefined instead of the raw value, and update the PLANS export so the lifetime plan only appears configured when the parsed value is valid; adjust any logic in getPaidPlans() that relies on PLANS to handle an undefined polarProductId.
189-194:⚠️ Potential issue | 🟠 MajorDon't surface the experimental Lifetime plan by default.
src/lib/components/editor/pricing-dialog.sveltecallsgetPaidPlans()with no arguments, so this default now renders the Lifetime CTA for everyone. KeepincludeExperimentalopt-in here, or additionally gatePLANS.lifetimeon a configured product/server support before pushing it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/config/plans.ts` around lines 189 - 194, The getPaidPlans function currently surfaces experimental PLANS.lifetime by default; change its behavior so experimental plans are opt-in by setting includeExperimental to false (i.e., export function getPaidPlans(includeExperimental = false): PlanConfig[]) or alternatively add a server/product feature check before pushing PLANS.lifetime (e.g., call a feature flag or config check inside getPaidPlans) so PLANS.lifetime is only returned when explicitly enabled; update callers that should show Lifetime to pass includeExperimental=true or rely on the feature flag.
🧹 Nitpick comments (1)
src/lib/schemas/subscription.ts (1)
20-22: Make the schema ownPlanTier.
PlanTierSchemais declared here, butPlanTierstill comes from$lib/config/plans. That leaves two independent tier definitions that can drift the next time a plan is added. Exporttype PlanTier = z.infer<typeof PlanTierSchema>from this module and have plan config consume it instead. As per coding guidelines,src/lib/schemas/**/*.ts: Define Zod schemas first as single source of truth, infer TypeScript types usingz.inferinstead of duplicating type definitions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/schemas/subscription.ts` around lines 20 - 22, Replace the duplicated PlanTier type with an inferred type from the Zod schema so the schema is the single source of truth: export type PlanTier = z.infer<typeof PlanTierSchema> in this module (where PlanTierSchema is declared) and remove any separate PlanTier type imports from the plan config; then update the plan configuration to import the inferred PlanTier from this schema module. Ensure you keep the existing export of PlanTierSchema and only change the type export to use z.infer<typeof PlanTierSchema>.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/config/plans.ts`:
- Around line 231-232: The priority map gives 'lifetime' the same numeric value
as 'creator', causing compareTiers to treat them as equal; update the priority
object (the const named priority used by compareTiers) so 'lifetime' has a
distinct numeric rank (choose a value that reflects its intended ordering
relative to free/creator/pro) to ensure compareTiers('creator','lifetime')
returns a non-zero result.
In `@src/lib/schemas/subscription.ts`:
- Around line 35-39: The hasReachedLimit helper currently only treats
maxCostPerMonth === -1 as lifetime, causing plans with maxCostPerMonth === -2 to
be considered limited; update hasReachedLimit to detect lifetime plans by
checking if plan.maxCostPerMonth < 0 (or otherwise treat any negative value as
the lifetime sentinel) and return false (not reached) early, and ensure this
logic is applied before any logic that exposes or uses creditBalance; reference
the hasReachedLimit function and the creditBalance field and the maxCostPerMonth
values defined in src/lib/config/plans.ts when making the change.
---
Duplicate comments:
In `@src/lib/config/plans.ts`:
- Around line 6-10: The PLANS config currently assigns
PUBLIC_POLAR_LIFETIME_PRODUCT_ID verbatim into polarProductId; validate this
external env with Zod (e.g., create a schema that accepts a non-empty string or
undefined), parse PUBLIC_POLAR_LIFETIME_PRODUCT_ID once and if validation fails
or value is empty set polarProductId to undefined instead of the raw value, and
update the PLANS export so the lifetime plan only appears configured when the
parsed value is valid; adjust any logic in getPaidPlans() that relies on PLANS
to handle an undefined polarProductId.
- Around line 189-194: The getPaidPlans function currently surfaces experimental
PLANS.lifetime by default; change its behavior so experimental plans are opt-in
by setting includeExperimental to false (i.e., export function
getPaidPlans(includeExperimental = false): PlanConfig[]) or alternatively add a
server/product feature check before pushing PLANS.lifetime (e.g., call a feature
flag or config check inside getPaidPlans) so PLANS.lifetime is only returned
when explicitly enabled; update callers that should show Lifetime to pass
includeExperimental=true or rely on the feature flag.
---
Nitpick comments:
In `@src/lib/schemas/subscription.ts`:
- Around line 20-22: Replace the duplicated PlanTier type with an inferred type
from the Zod schema so the schema is the single source of truth: export type
PlanTier = z.infer<typeof PlanTierSchema> in this module (where PlanTierSchema
is declared) and remove any separate PlanTier type imports from the plan config;
then update the plan configuration to import the inferred PlanTier from this
schema module. Ensure you keep the existing export of PlanTierSchema and only
change the type export to use z.infer<typeof PlanTierSchema>.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3080601a-8ef0-4074-891f-7626ed34b4db
📒 Files selected for processing (6)
src/lib/ai/models.tssrc/lib/components/marketing/pricing-section.sveltesrc/lib/config/plans.tssrc/lib/layers/LayerWrapper.sveltesrc/lib/layers/components/LaptopLayer.sveltesrc/lib/schemas/subscription.ts
💤 Files with no reviewable changes (1)
- src/lib/ai/models.ts
✅ Files skipped from review due to trivial changes (2)
- src/lib/layers/LayerWrapper.svelte
- src/lib/layers/components/LaptopLayer.svelte
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/components/marketing/pricing-section.svelte
| const priority = { free: 0, creator: 1, lifetime: 1, pro: 2 }; | ||
| return priority[tier]; |
There was a problem hiding this comment.
Give lifetime a unique priority.
Right now compareTiers('creator', 'lifetime') returns 0, because both tiers map to 1. That makes two distinct plans compare as equal, which can break upgrade/downgrade checks and any UI logic built on compareTiers().
Suggested change
export function getTierPriority(tier: PlanTier): number {
- const priority = { free: 0, creator: 1, lifetime: 1, pro: 2 };
+ const priority = { free: 0, creator: 1, lifetime: 2, pro: 3 } as const;
return priority[tier];
}📝 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.
| const priority = { free: 0, creator: 1, lifetime: 1, pro: 2 }; | |
| return priority[tier]; | |
| export function getTierPriority(tier: PlanTier): number { | |
| const priority = { free: 0, creator: 1, lifetime: 2, pro: 3 } as const; | |
| return priority[tier]; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/config/plans.ts` around lines 231 - 232, The priority map gives
'lifetime' the same numeric value as 'creator', causing compareTiers to treat
them as equal; update the priority object (the const named priority used by
compareTiers) so 'lifetime' has a distinct numeric rank (choose a value that
reflects its intended ordering relative to free/creator/pro) to ensure
compareTiers('creator','lifetime') returns a non-zero result.
| creditBalance: z | ||
| .number() | ||
| .min(0) | ||
| .default(0) | ||
| .describe('Expandable credit balance in USD (for lifetime plans)'), |
There was a problem hiding this comment.
Handle lifetime credits in hasReachedLimit() before exposing creditBalance.
This field enables the lifetime path, but hasReachedLimit() below still only special-cases -1. With src/lib/config/plans.ts setting lifetime maxCostPerMonth to -2, the current helper will evaluate usage.aiCostUsed >= -2 and report the limit as reached immediately for every lifetime user.
Possible fix outside this hunk
export function hasReachedLimit(
usage: UsageStats,
tier: PlanTier,
limitType: 'maxCostPerMonth' | 'cloudProjects'
): boolean {
const plan = getPlan(tier);
const limit = plan.limits[limitType];
+ if (limitType === 'maxCostPerMonth' && limit === -2) {
+ return usage.creditBalance <= 0;
+ }
+
// Unlimited
if (limit === -1) return false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/schemas/subscription.ts` around lines 35 - 39, The hasReachedLimit
helper currently only treats maxCostPerMonth === -1 as lifetime, causing plans
with maxCostPerMonth === -2 to be considered limited; update hasReachedLimit to
detect lifetime plans by checking if plan.maxCostPerMonth < 0 (or otherwise
treat any negative value as the lifetime sentinel) and return false (not
reached) early, and ensure this logic is applied before any logic that exposes
or uses creditBalance; reference the hasReachedLimit function and the
creditBalance field and the maxCostPerMonth values defined in
src/lib/config/plans.ts when making the change.
1 participant
Summary by CodeRabbit
New Features
Chores