Feature/customer checkout #423
Conversation
WalkthroughAdds Redis-backed distributed locking and DynamoDB persistence to support a new POST /createInvoice flow that checks/creates Stripe customers (with confirmation for name/email mismatches), records invoices, and returns either a checkout link or a 409 conflict with details. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Route as POST /createInvoice
participant AddInvoice as addInvoice()
participant CheckCustomer as checkOrCreateCustomer()
participant Redis as Redis Lock
participant DynamoDB as DynamoDB
participant Stripe as Stripe API
Client->>Route: POST /createInvoice
Route->>AddInvoice: invoke with invoice params
AddInvoice->>CheckCustomer: ensure customer exists
CheckCustomer->>Redis: acquire lock (acmOrg / emailDomain)
alt lock acquired
Redis-->>CheckCustomer: lock granted
CheckCustomer->>DynamoDB: query CUSTOMER / EMAIL#
alt customer exists
DynamoDB-->>CheckCustomer: CUSTOMER record
CheckCustomer->>Stripe: fetch customer data
Stripe-->>CheckCustomer: current name/email
alt matches incoming
CheckCustomer-->>AddInvoice: return customerId
else mismatch
CheckCustomer-->>AddInvoice: return needsConfirmation + current/incoming
end
else not found
CheckCustomer->>Stripe: create customer
Stripe-->>CheckCustomer: customerId
CheckCustomer->>DynamoDB: put CUSTOMER & EMAIL# (atomic/conditional)
CheckCustomer-->>AddInvoice: return customerId
end
CheckCustomer->>Redis: release lock
else lock failed / timed out
Redis-->>CheckCustomer: lock failed
CheckCustomer->>DynamoDB: fallback query for CUSTOMER
end
alt needsConfirmation
AddInvoice-->>Route: propagate needsConfirmation
Route-->>Client: 409 Conflict (details)
else success
AddInvoice->>DynamoDB: write CHARGE#<invoiceId> and increment CUSTOMER totalAmount
AddInvoice-->>Route: customerId
Route->>Stripe: createCheckoutSessionWithCustomer(customerId)
Stripe-->>Route: checkout link
Route-->>Client: 201 Created { id, link }
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 |
💰 Infracost reportMonthly estimate generatedThis comment will be updated when code changes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/api/routes/stripe.ts (1)
785-866: New payment_intent.succeeded handler is robust; consider capturing more metadataThe handler defensively validates
customerId, email, andacm_org, normalizes the email domain, and writes a concise record intoStripePaymentsDynamoTablekeyed byacmOrg#domain. Logging and error handling also look solid.You may optionally want to include
customerIdin the stored item for easier debugging/analytics later, but the current implementation is sound.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
src/api/functions/stripe.ts(2 hunks)src/api/routes/stripe.ts(3 hunks)src/common/types/stripe.ts(2 hunks)
🧰 Additional context used
🪛 ESLint
src/api/functions/stripe.ts
[error] 6-6: Unexpected use of file extension "js" for "api/types.js"
(import/extensions)
[error] 13-13: Unexpected use of file extension "js" for "common/config.js"
(import/extensions)
src/common/types/stripe.ts
[error] 24-24: Insert ,
(prettier/prettier)
[error] 42-42: Delete ····
(prettier/prettier)
[error] 55-55: Insert ,
(prettier/prettier)
[error] 59-60: Replace >;⏎ with ⏎>;
(prettier/prettier)
[error] 63-63: Insert ⏎
(prettier/prettier)
[error] 78-78: Insert ⏎
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Unit Tests
- GitHub Check: Build Application
🔇 Additional comments (2)
src/api/routes/stripe.ts (2)
12-22: New Stripe helper imports and request schema wiring look correct
addInvoice,createCheckoutSessionWithCustomer,InvoiceAddParams, andcreateInvoicePostRequestSchemaare all used consistently in the new route below; the import surface looks clean.Also applies to: 40-43
117-151: 409 confirmation flow response shape is consistent with typesThe
/createInvoicehandler’s 409 path (needsConfirmation,customerId,current,incoming,message) matches theCheckOrCreateResultstructure andcreateInvoiceConflictResponseSchemayou defined, so the conflict semantics and payload look coherent.
| import { createLock, IoredisAdapter, type SimpleLock } from "redlock-universal"; | ||
| import { Redis } from "api/types.js"; | ||
| import { | ||
| TransactWriteItemsCommand, | ||
| QueryCommand, | ||
| UpdateItemCommand, | ||
| DynamoDBClient, | ||
| } from "@aws-sdk/client-dynamodb"; | ||
| import { genericConfig } from "common/config.js"; | ||
| import { marshall } from "@aws-sdk/util-dynamodb"; |
There was a problem hiding this comment.
🧩 Analysis chain
Address ESLint import/extensions complaints for new imports
ESLint is flagging the .js extensions on the new imports from "api/types.js" and "common/config.js".
Depending on the project’s convention, either:
- Drop the extension to satisfy
import/extensions(e.g.import { Redis } from "api/types";,import { genericConfig } from "common/config";), or - Adjust the ESLint config to allow
.jsextensions for these path-mapped imports, in line with how other modules in this repo are handled.
Please align this with whatever pattern the rest of the codebase is using so lint passes.
🏁 Script executed:
#!/bin/bash
# Find ESLint configuration files
fd -e "eslintrc*" -e ".eslintignore" -e "eslint.config.*"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check package.json for eslint config or import/extensions rule
cat package.json | grep -A 20 '"eslintConfig"' 2>/dev/null || echo "No eslintConfig in package.json"Length of output: 31
🏁 Script executed:
#!/bin/bash
# Sample imports from TypeScript files to understand convention
echo "=== Sampling imports across codebase ==="
rg -n "^import\s+" --type ts --max-count 5 -g 'src/**' | head -40Length of output: 2800
🏁 Script executed:
#!/bin/bash
# Specifically check if api/types and common/config imports are used elsewhere
echo "=== Imports from api/types ==="
rg -n 'from\s+["\']api/types' --type ts
echo ""
echo "=== Imports from common/config ==="
rg -n 'from\s+["\']common/config' --type tsLength of output: 99
🏁 Script executed:
#!/bin/bash
# Check the actual tsconfig or module resolution settings
cat tsconfig.json 2>/dev/null | head -40Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find and display actual imports in stripe.ts
echo "=== Current imports in stripe.ts (lines 1-14) ==="
head -14 src/api/functions/stripe.tsLength of output: 613
🏁 Script executed:
#!/bin/bash
# Search for imports of api/types using simpler pattern
echo "=== Files importing api/types (any variant) ==="
rg -n 'api/types' --type ts -C 2Length of output: 4665
🏁 Script executed:
#!/bin/bash
# Search for imports of common/config using simpler pattern
echo "=== Files importing common/config (any variant) ==="
rg -n 'common/config' --type ts -C 2Length of output: 22578
🏁 Script executed:
#!/bin/bash
# Check if these modules exist and have .js extensions
echo "=== Checking if api/types.ts or api/types.js exists ==="
fd -e "types" --path "*/api/*" 2>/dev/null | head -20
echo ""
echo "=== Checking if common/config.ts or common/config.js exists ==="
fd -e "config" --path "*/common/*" 2>/dev/null | head -20Length of output: 120
🏁 Script executed:
#!/bin/bash
# Look for ESLint config in different formats
echo "=== Searching for eslint mentions in config files ==="
rg -l 'eslint|import/extensions' --type json --type js --type yml --type yaml 2>/dev/null | head -20Length of output: 53
🏁 Script executed:
#!/bin/bash
# Check how other files in src/api import from api/types and common/config
echo "=== Imports in other api files ==="
rg -n 'from.*["\']' src/api/ --type ts | grep -E '(api/|common/)' | head -20Length of output: 198
🏁 Script executed:
#!/bin/bash
# Search for any hidden eslint configs, including dotfiles
echo "=== All files with 'eslint' in name (including hidden) ==="
find . -name "*eslint*" -type f 2>/dev/null | head -20Length of output: 1529
🏁 Script executed:
#!/bin/bash
# Check for pre-commit or husky configs
echo "=== Checking for pre-commit/husky/git hooks ==="
ls -la .husky/ 2>/dev/null || echo "No .husky directory"
ls -la .git/hooks/ 2>/dev/null | head -20 || echo "No git hooks visible"
cat .pre-commit-config.yaml 2>/dev/null || echo "No .pre-commit-config.yaml"Length of output: 306
🏁 Script executed:
#!/bin/bash
# Verify pattern: count imports WITH .js vs WITHOUT .js for api/* and common/*
echo "=== Imports from api/* WITH .js extension ==="
rg 'from\s+["\x27]api/[^"'\'']*\.js["\x27]' --type ts | wc -l
echo ""
echo "=== Imports from api/* WITHOUT .js extension ==="
rg 'from\s+["\x27]api/[^"'\'']*["\x27](?!\.js)' --type ts | wc -l
echo ""
echo "=== Imports from common/* WITH .js extension ==="
rg 'from\s+["\x27]common/[^"'\'']*\.js["\x27]' --type ts | wc -l
echo ""
echo "=== Imports from common/* WITHOUT .js extension ==="
rg 'from\s+["\x27]common/[^"'\'']*["\x27](?!\.js)' --type ts | wc -lLength of output: 764
🏁 Script executed:
#!/bin/bash
# Check specific file to verify pattern in one location
echo "=== All imports in src/api/functions/apiKey.ts ==="
head -20 src/api/functions/apiKey.ts | grep -E '^import'Length of output: 529
🏁 Script executed:
#!/bin/bash
# Inspect the ESLint configuration
echo "=== ESLint config (eslint.config.mjs) ==="
cat eslint.config.mjsLength of output: 2607
🏁 Script executed:
#!/bin/bash
# Inspect the pre-commit hook
echo "=== Pre-commit hook script ==="
cat .husky/pre-commitLength of output: 550
🏁 Script executed:
#!/bin/bash
# Check .eslintignore to see what's ignored
echo "=== ESLint ignore file ==="
cat .eslintignoreLength of output: 52
Remove .js extensions from path-mapped imports to comply with ESLint import/extensions rule
The ESLint configuration (eslint.config.mjs) explicitly requires that .js and .ts extensions be omitted from imports: "import/extensions": ["error", "ignorePackages", { js: "never", jsx: "never", ts: "never", tsx: "never" }].
The imports in stripe.ts at lines 6 and 13 violate this rule:
- Change
import { Redis } from "api/types.js";toimport { Redis } from "api/types"; - Change
import { genericConfig } from "common/config.js";toimport { genericConfig } from "common/config";
The pre-commit hook runs yarn lint --fix, which will catch this during commit. Note: the entire codebase currently uses .js extensions for these paths (127+ instances for api/*, 132+ for common/*), so a widespread fix may be needed beyond this file.
🧰 Tools
🪛 ESLint
[error] 6-6: Unexpected use of file extension "js" for "api/types.js"
(import/extensions)
[error] 13-13: Unexpected use of file extension "js" for "common/config.js"
(import/extensions)
🤖 Prompt for AI Agents
In src/api/functions/stripe.ts around lines 5 to 14, two path-mapped imports
include explicit .js extensions which violates the ESLint import/extensions
rule; remove the .js extensions from the imports so they read import { Redis }
from "api/types" and import { genericConfig } from "common/config", then run the
linter/fix (yarn lint --fix) to verify and apply consistent fixes across the
codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/api/functions/stripe.ts (1)
5-14: Past review comments already address the import extension issues.The
.jsextensions on lines 6 and 13 were flagged in previous reviews, and the ESLint configuration update ineslint.config.mjsis intended to resolve import resolution. Please refer to the existing review comments for remediation guidance.
🧹 Nitpick comments (4)
src/api/functions/stripe.ts (4)
368-370: UseValidationErrorfor consistency.Line 369 throws a generic
Error, but the codebase imports and usesValidationErrorfromcommon/errors/index.jsfor validation failures (see line 2). For consistency with other validation errors in this module and improved error handling upstream, useValidationErrorhere.Apply this diff:
if (!domainPart) { - throw new Error(`Could not derive email domain for "${customerEmail}".`); + throw new ValidationError({ + message: `Could not derive email domain for "${customerEmail}".`, + }); }
444-445: Avoid unsafe type assertions when accessing DynamoDB response.Lines 444-445 use
(customerResponse.Items![0] as any)to access the Stripe customer ID. This bypasses type safety and can lead to runtime errors if the response structure changes.Consider using the
unmarshallutility from@aws-sdk/util-dynamodbto properly type the response:+ import { marshall, unmarshall } from "@aws-sdk/util-dynamodb"; // ... later in the function ... - const existingCustomerId = (customerResponse.Items![0] as any) - .stripeCustomerId.S as string; + const item = unmarshall(customerResponse.Items![0]); + const existingCustomerId = item.stripeCustomerId as string;Note: You already import
marshallon line 14, so just addunmarshallto that import.
451-454: Use proper Stripe types instead ofas anycasts.Lines 451-454 cast
stripeCustomertoanyto accessnameandThe Stripe SDK exports a
Customertype that you can use for type narrowing:const stripeClient = new Stripe(stripeApiKey); const stripeCustomer = await stripeClient.customers.retrieve(existingCustomerId); + if (stripeCustomer.deleted) { + throw new InternalServerError({ + message: `Customer ${existingCustomerId} has been deleted from Stripe.`, + }); + } + - const liveName = - "name" in stripeCustomer ? (stripeCustomer as any).name : null; - const liveEmail = - "email" in stripeCustomer ? (stripeCustomer as any).email : null; + const liveName = stripeCustomer.name ?? null; + const liveEmail = stripeCustomer.email ?? null;This also adds a check for deleted customers, which would otherwise cause issues downstream.
525-527: UseValidationErrorfor consistency.Line 526 throws a generic
Error, but should useValidationErrorfor consistency with other validation errors in this module (see line 2 for the import).Apply this diff:
if (!domainPart) { - throw new Error(`Could not derive email domain for "${contactEmail}".`); + throw new ValidationError({ + message: `Could not derive email domain for "${contactEmail}".`, + }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
eslint.config.mjs(1 hunks)src/api/functions/stripe.ts(2 hunks)src/api/package.json(2 hunks)src/api/routes/stripe.ts(3 hunks)src/common/types/stripe.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/api/routes/stripe.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/api/functions/stripe.ts (2)
src/api/types.d.ts (1)
Redis(13-13)src/common/config.ts (1)
genericConfig(244-244)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Application
- GitHub Check: Run Unit Tests
🔇 Additional comments (6)
eslint.config.mjs (1)
42-45: LGTM! Node resolver configuration addresses import resolution.The addition of the Node resolver with explicit paths for
src/apiandsrc/uienables ESLint to properly resolve path-mapped imports. This complements the existing TypeScript resolver and should help address import/extensions issues.However, note that past reviews identified 127+ instances of
.jsextensions onapi/*imports and 132+ oncommon/*imports throughout the codebase. While this ESLint config change is correct, those explicit extensions still need to be removed to comply with theimport/extensionsrule.src/api/functions/stripe.ts (2)
339-354: LGTM! Type definitions are clear and well-structured.The
checkCustomerParamsandCheckOrCreateResulttypes properly capture the function signatures and return shapes for the customer check/create flow.
546-578: LGTM! Transaction structure is correct and idempotent.The
TransactWriteItemsCommandnow properly separates thePutandUpdateoperations into two distinctTransactItemselements (lines 548-563 and 564-576), which conforms to DynamoDB's API requirements. TheConditionExpressionon thePut(lines 560-561) ensures idempotency by preventing duplicate invoice records on retries.This addresses the critical issue raised in previous reviews.
src/common/types/stripe.ts (2)
23-66: LGTM! New invoice schemas are well-structured and properly formatted.The new schemas for invoice creation (
createInvoicePostResponseSchema,createInvoiceConflictResponseSchema, and their union types) are correctly defined and follow Zod best practices. The formatting issues flagged in previous reviews have been resolved—all trailing commas and line breaks are now in place.The schema structure properly supports the dual response pattern (success with checkout link vs. conflict requiring confirmation), which aligns with the implementation in
src/api/functions/stripe.ts.
76-76: LGTM! Formatting is correct.The
createdAtfield union andGetInvoiceLinksResponsetype formatting are correct. Previous formatting issues have been addressed.Also applies to: 81-82
src/api/package.json (1)
20-20: No action required—AWS SDK v3.922.0 is a stable release with no known security vulnerabilities.Version 3.922.0 exists in the npm registry for both @aws-sdk/client-s3 and @aws-sdk/s3-request-presigner, and Snyk reports no direct vulnerabilities for these releases. No CVEs are associated with this version.
| try { | ||
| await dynamoClient.send(ensureEmailMap); | ||
| } catch (e) { | ||
| // ignore | ||
| } |
There was a problem hiding this comment.
Silent error suppression may hide legitimate issues.
Lines 480-484 attempt to create an EMAIL# mapping entry but silently ignore all errors with a bare catch (e) { // ignore }. While this might be intended to handle duplicate writes gracefully, it also suppresses legitimate errors like permission issues, network failures, or DynamoDB service errors that should be logged or handled.
At minimum, log the error for debugging and only suppress the specific ConditionalCheckFailedException:
try {
await dynamoClient.send(ensureEmailMap);
} catch (e) {
- // ignore
+ // Ignore if the EMAIL# entry already exists (ConditionalCheckFailedException)
+ if (
+ !(e instanceof Error) ||
+ !e.name.includes("ConditionalCheckFailedException")
+ ) {
+ // Log unexpected errors for debugging
+ console.warn(
+ `Failed to create EMAIL# mapping for ${normalizedEmail}:`,
+ e
+ );
+ }
}📝 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.
| try { | |
| await dynamoClient.send(ensureEmailMap); | |
| } catch (e) { | |
| // ignore | |
| } | |
| try { | |
| await dynamoClient.send(ensureEmailMap); | |
| } catch (e) { | |
| // Ignore if the EMAIL# entry already exists (ConditionalCheckFailedException) | |
| if ( | |
| !(e instanceof Error) || | |
| !e.name.includes("ConditionalCheckFailedException") | |
| ) { | |
| // Log unexpected errors for debugging | |
| console.warn( | |
| `Failed to create EMAIL# mapping for ${normalizedEmail}:`, | |
| e | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/api/functions/stripe.ts around lines 480-484, the try/catch silently
swallows all errors when creating the EMAIL# mapping; change it to catch the
error, log it (including error message/stack and context), and only suppress the
specific conditional-failure that indicates a duplicate (e.g., check error.name
or error.code for ConditionalCheckFailedException or ConditionalCheckFailed);
for any other errors rethrow or return an error result so permission, network,
or service errors are visible and handled appropriately.
Branched off of Vishnu's work to
Summary by CodeRabbit
New Features
Documentation / Validation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.