Conversation
WalkthroughAdds Redis-backed distributed locking and DynamoDB-backed persistence to support a new POST /createInvoice flow. Introduces 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) | ⏱️ ~25 minutes 🚥 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
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.
|
@AdityaK2905 Lets follow up on this after break - I forgot but are we good to merge after fixing that silent catch? |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@src/api/functions/stripe.ts`:
- Around line 508-518: The InvoiceAddParams type includes an unused emailDomain
field which is never used in addInvoice (the function derives normalizedDomain
from contactEmail in addInvoice and similar to checkCustomerParams); remove
emailDomain from the InvoiceAddParams definition or update addInvoice to use the
provided emailDomain (choose one approach consistently). Locate the
InvoiceAddParams type and the addInvoice function and either delete the
emailDomain property from InvoiceAddParams and any callers that pass it, or
modify addInvoice to prefer the passed emailDomain over deriving from
contactEmail (ensuring normalizedDomain is computed from the chosen source).
- Around line 447-458: The code assumes
stripeClient.customers.retrieve(existingCustomerId) returns an active customer;
add a guard for the Stripe.DeletedCustomer case by checking
stripeCustomer.deleted === true before reading name/email. In the block around
stripeClient, set liveName and liveEmail to null when stripeCustomer.deleted is
true (or otherwise skip property access) so the later needsConfirmation logic
uses nulls instead of accessing missing fields; reference the stripeClient,
stripeCustomer, existingCustomerId, liveName, liveEmail, and needsConfirmation
identifiers when applying the fix.
- Around line 555-590: Wrap the await dynamoClient.send(dynamoCommand) call in a
try/catch and handle idempotent retries: if the catch error is a
TransactionCanceledException and its CancellationReasons array (or error
payload) indicates a ConditionalCheckFailedException for the CHARGE Put (i.e.,
the conditional idempotency failed because the invoice already exists), swallow
the error and return the same success shape (return { customerId:
result.customerId }); otherwise rethrow the error. Use the existing
dynamoCommand / TransactWriteItemsCommand and dynamoClient.send identifiers to
locate where to add the try/catch and inspect e.name ===
'TransactionCanceledException' and e.CancellationReasons.some(r => r.Code ===
'ConditionalCheckFailed') (or equivalent fields) to detect the specific
condition.
- Around line 396-441: Wrap the TransactWriteItemsCommand/dynamoClient.send call
in a try/catch so that if the DynamoDB transaction fails after
createStripeCustomer returns (customer variable), you either (a) attempt to
delete the newly created Stripe customer via the Stripe SDK (call
stripe.customers.del(customer)) or (b) record the failure to a dedicated
cleanup/compensating table (in genericConfig.StripePaymentsDynamoTableName or a
new cleanup table) and log full context; specifically catch
ConditionalCheckFailedException and network errors from dynamoClient.send,
include the customer id and normalizedEmail in logs, and ensure the function
returns an error or retryable response only after the compensating delete or
cleanup entry is created to avoid orphaned Stripe customers.
- Around line 339-347: The checkCustomerParams type includes an unused
emailDomain; either remove it from the type and all callers of
checkOrCreateCustomer (adjusting call sites to stop passing emailDomain) or
change checkOrCreateCustomer to use the provided emailDomain instead of
re-deriving it: accept emailDomain, normalize/validate it against the domain
parsed from customerEmail (e.g., compare normalizedDomain derived from
customerEmail with the passed emailDomain and throw or log if they differ), and
then use the passed value for downstream logic; update all references to
checkCustomerParams, checkOrCreateCustomer, and any callers accordingly to keep
the signature consistent.
- Around line 444-445: The code extracts existingCustomerId by indexing into the
raw DynamoDB item with (customerResponse.Items![0] as any).stripeCustomerId.S
which is brittle and uses unsafe casting; instead import and use unmarshall from
`@aws-sdk/util-dynamodb` to convert the DynamoDB item to a plain JS object and
then read stripeCustomerId (e.g.,
unmarshall(customerResponse.Items![0]).stripeCustomerId) so existingCustomerId
is obtained in a type-safe, cleaner way; update the code around
customerResponse.Items and the declaration of existingCustomerId to use
unmarshall and appropriate typing.
| export type checkCustomerParams = { | ||
| acmOrg: string; | ||
| emailDomain: string; | ||
| redisClient: Redis; | ||
| dynamoClient: DynamoDBClient; | ||
| customerEmail: string; | ||
| customerName: string; | ||
| stripeApiKey: string; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unused emailDomain parameter in checkCustomerParams.
The emailDomain parameter is defined but never used within checkOrCreateCustomer. The function derives normalizedDomain directly from customerEmail (lines 366-372), making emailDomain redundant.
Either remove the unused parameter or use it instead of re-deriving from the email:
♻️ Option 1: Remove unused parameter
export type checkCustomerParams = {
acmOrg: string;
- emailDomain: string;
redisClient: Redis;
dynamoClient: DynamoDBClient;
customerEmail: string;
customerName: string;
stripeApiKey: string;
};Then update callers accordingly.
♻️ Option 2: Use the parameter and validate consistency
export const checkOrCreateCustomer = async ({
acmOrg,
emailDomain,
redisClient,
dynamoClient,
customerEmail,
customerName,
stripeApiKey,
}: checkCustomerParams): Promise<CheckOrCreateResult> => {
const normalizedEmail = customerEmail.trim().toLowerCase();
const [, domainPart] = normalizedEmail.split("@");
if (!domainPart) {
throw new Error(`Could not derive email domain for "${customerEmail}".`);
}
- const normalizedDomain = domainPart.toLowerCase();
+ const normalizedDomain = emailDomain.trim().toLowerCase();
+ if (normalizedDomain !== domainPart) {
+ throw new ValidationError({
+ message: `Email domain mismatch: expected "${normalizedDomain}", got "${domainPart}".`,
+ });
+ }🤖 Prompt for AI Agents
In `@src/api/functions/stripe.ts` around lines 339 - 347, The checkCustomerParams
type includes an unused emailDomain; either remove it from the type and all
callers of checkOrCreateCustomer (adjusting call sites to stop passing
emailDomain) or change checkOrCreateCustomer to use the provided emailDomain
instead of re-deriving it: accept emailDomain, normalize/validate it against the
domain parsed from customerEmail (e.g., compare normalizedDomain derived from
customerEmail with the passed emailDomain and throw or log if they differ), and
then use the passed value for downstream logic; update all references to
checkCustomerParams, checkOrCreateCustomer, and any callers accordingly to keep
the signature consistent.
| if (customerResponse.Count === 0) { | ||
| const customer = await createStripeCustomer({ | ||
| email: normalizedEmail, | ||
| name: customerName, | ||
| stripeApiKey, | ||
| }); | ||
|
|
||
| const createCustomer = new TransactWriteItemsCommand({ | ||
| TransactItems: [ | ||
| { | ||
| Put: { | ||
| TableName: genericConfig.StripePaymentsDynamoTableName, | ||
| Item: marshall( | ||
| { | ||
| primaryKey: pk, | ||
| sortKey: "CUSTOMER", | ||
| stripeCustomerId: customer, | ||
| totalAmount: 0, | ||
| createdAt: new Date().toISOString(), | ||
| }, | ||
| { removeUndefinedValues: true }, | ||
| ), | ||
| ConditionExpression: | ||
| "attribute_not_exists(primaryKey) AND attribute_not_exists(sortKey)", | ||
| }, | ||
| }, | ||
| { | ||
| Put: { | ||
| TableName: genericConfig.StripePaymentsDynamoTableName, | ||
| Item: marshall( | ||
| { | ||
| primaryKey: pk, | ||
| sortKey: `EMAIL#${normalizedEmail}`, | ||
| stripeCustomerId: customer, | ||
| createdAt: new Date().toISOString(), | ||
| }, | ||
| { removeUndefinedValues: true }, | ||
| ), | ||
| ConditionExpression: | ||
| "attribute_not_exists(primaryKey) AND attribute_not_exists(sortKey)", | ||
| }, | ||
| }, | ||
| ], | ||
| }); | ||
| await dynamoClient.send(createCustomer); | ||
| return { customerId: customer }; |
There was a problem hiding this comment.
Potential orphaned Stripe customer on DynamoDB transaction failure.
If createStripeCustomer succeeds (line 397-401) but the subsequent TransactWriteItemsCommand fails (e.g., ConditionalCheckFailedException due to lock contention edge cases or network partition), a Stripe customer is created but never recorded in DynamoDB. This orphaned customer cannot be reconciled on retry since a new Stripe customer would be created.
Consider wrapping this in error handling to log or attempt cleanup:
🛡️ Suggested defensive handling
if (customerResponse.Count === 0) {
const customer = await createStripeCustomer({
email: normalizedEmail,
name: customerName,
stripeApiKey,
});
const createCustomer = new TransactWriteItemsCommand({
// ... existing code ...
});
- await dynamoClient.send(createCustomer);
- return { customerId: customer };
+ try {
+ await dynamoClient.send(createCustomer);
+ return { customerId: customer };
+ } catch (e) {
+ // Log orphaned Stripe customer for manual reconciliation
+ console.error(
+ `DynamoDB write failed after Stripe customer creation. Orphaned Stripe customer ID: ${customer}, PK: ${pk}`,
+ e,
+ );
+ throw e;
+ }
}🤖 Prompt for AI Agents
In `@src/api/functions/stripe.ts` around lines 396 - 441, Wrap the
TransactWriteItemsCommand/dynamoClient.send call in a try/catch so that if the
DynamoDB transaction fails after createStripeCustomer returns (customer
variable), you either (a) attempt to delete the newly created Stripe customer
via the Stripe SDK (call stripe.customers.del(customer)) or (b) record the
failure to a dedicated cleanup/compensating table (in
genericConfig.StripePaymentsDynamoTableName or a new cleanup table) and log full
context; specifically catch ConditionalCheckFailedException and network errors
from dynamoClient.send, include the customer id and normalizedEmail in logs, and
ensure the function returns an error or retryable response only after the
compensating delete or cleanup entry is created to avoid orphaned Stripe
customers.
| const existingCustomerId = (customerResponse.Items![0] as any) | ||
| .stripeCustomerId.S as string; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using unmarshall for type-safe DynamoDB item access.
The current code uses as any to access the DynamoDB item structure. Using unmarshall from @aws-sdk/util-dynamodb (already imported via marshall) would provide cleaner access.
♻️ Suggested refactor
+import { marshall, unmarshall } from "@aws-sdk/util-dynamodb";
// ...
- const existingCustomerId = (customerResponse.Items![0] as any)
- .stripeCustomerId.S as string;
+ const existingItem = unmarshall(customerResponse.Items![0]);
+ const existingCustomerId = existingItem.stripeCustomerId as string;📝 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 existingCustomerId = (customerResponse.Items![0] as any) | |
| .stripeCustomerId.S as string; | |
| const existingItem = unmarshall(customerResponse.Items![0]); | |
| const existingCustomerId = existingItem.stripeCustomerId as string; |
🤖 Prompt for AI Agents
In `@src/api/functions/stripe.ts` around lines 444 - 445, The code extracts
existingCustomerId by indexing into the raw DynamoDB item with
(customerResponse.Items![0] as any).stripeCustomerId.S which is brittle and uses
unsafe casting; instead import and use unmarshall from `@aws-sdk/util-dynamodb` to
convert the DynamoDB item to a plain JS object and then read stripeCustomerId
(e.g., unmarshall(customerResponse.Items![0]).stripeCustomerId) so
existingCustomerId is obtained in a type-safe, cleaner way; update the code
around customerResponse.Items and the declaration of existingCustomerId to use
unmarshall and appropriate typing.
| const stripeClient = new Stripe(stripeApiKey); | ||
| const stripeCustomer = | ||
| await stripeClient.customers.retrieve(existingCustomerId); | ||
|
|
||
| const liveName = | ||
| "name" in stripeCustomer ? (stripeCustomer as any).name : null; | ||
| const liveEmail = | ||
| "email" in stripeCustomer ? (stripeCustomer as any).email : null; | ||
|
|
||
| const needsConfirmation = | ||
| (!!liveName && liveName !== customerName) || | ||
| (!!liveEmail && liveEmail.toLowerCase() !== normalizedEmail); |
There was a problem hiding this comment.
Handle deleted Stripe customer case.
stripeClient.customers.retrieve(existingCustomerId) can return a Stripe.DeletedCustomer object if the customer was deleted in Stripe. In that case, stripeCustomer.deleted === true and accessing name/email properties won't work as expected.
🛡️ Suggested fix
const stripeCustomer =
await stripeClient.customers.retrieve(existingCustomerId);
+ if (stripeCustomer.deleted) {
+ // Customer was deleted in Stripe; treat as needing re-creation or log warning
+ console.warn(
+ `Stripe customer ${existingCustomerId} was deleted. Consider re-creating.`,
+ );
+ // Option: return needsConfirmation or throw an error
+ }
+
const liveName =
- "name" in stripeCustomer ? (stripeCustomer as any).name : null;
+ "name" in stripeCustomer && !stripeCustomer.deleted
+ ? stripeCustomer.name
+ : null;
const liveEmail =
- "email" in stripeCustomer ? (stripeCustomer as any).email : null;
+ "email" in stripeCustomer && !stripeCustomer.deleted
+ ? stripeCustomer.email
+ : null;📝 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 stripeClient = new Stripe(stripeApiKey); | |
| const stripeCustomer = | |
| await stripeClient.customers.retrieve(existingCustomerId); | |
| const liveName = | |
| "name" in stripeCustomer ? (stripeCustomer as any).name : null; | |
| const liveEmail = | |
| "email" in stripeCustomer ? (stripeCustomer as any).email : null; | |
| const needsConfirmation = | |
| (!!liveName && liveName !== customerName) || | |
| (!!liveEmail && liveEmail.toLowerCase() !== normalizedEmail); | |
| const stripeClient = new Stripe(stripeApiKey); | |
| const stripeCustomer = | |
| await stripeClient.customers.retrieve(existingCustomerId); | |
| if (stripeCustomer.deleted) { | |
| // Customer was deleted in Stripe; treat as needing re-creation or log warning | |
| console.warn( | |
| `Stripe customer ${existingCustomerId} was deleted. Consider re-creating.`, | |
| ); | |
| // Option: return needsConfirmation or throw an error | |
| } | |
| const liveName = | |
| "name" in stripeCustomer && !stripeCustomer.deleted | |
| ? stripeCustomer.name | |
| : null; | |
| const liveEmail = | |
| "email" in stripeCustomer && !stripeCustomer.deleted | |
| ? stripeCustomer.email | |
| : null; | |
| const needsConfirmation = | |
| (!!liveName && liveName !== customerName) || | |
| (!!liveEmail && liveEmail.toLowerCase() !== normalizedEmail); |
🤖 Prompt for AI Agents
In `@src/api/functions/stripe.ts` around lines 447 - 458, The code assumes
stripeClient.customers.retrieve(existingCustomerId) returns an active customer;
add a guard for the Stripe.DeletedCustomer case by checking
stripeCustomer.deleted === true before reading name/email. In the block around
stripeClient, set liveName and liveEmail to null when stripeCustomer.deleted is
true (or otherwise skip property access) so the later needsConfirmation logic
uses nulls instead of accessing missing fields; reference the stripeClient,
stripeCustomer, existingCustomerId, liveName, liveEmail, and needsConfirmation
identifiers when applying the fix.
| export type InvoiceAddParams = { | ||
| acmOrg: string; | ||
| emailDomain: string; | ||
| invoiceId: string; | ||
| invoiceAmountUsd: number; | ||
| redisClient: Redis; | ||
| dynamoClient: DynamoDBClient; | ||
| contactEmail: string; | ||
| contactName: string; | ||
| stripeApiKey: string; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Same unused emailDomain parameter issue.
Similar to checkCustomerParams, the emailDomain parameter is unused in addInvoice—the function derives normalizedDomain from contactEmail (lines 531-538).
🤖 Prompt for AI Agents
In `@src/api/functions/stripe.ts` around lines 508 - 518, The InvoiceAddParams
type includes an unused emailDomain field which is never used in addInvoice (the
function derives normalizedDomain from contactEmail in addInvoice and similar to
checkCustomerParams); remove emailDomain from the InvoiceAddParams definition or
update addInvoice to use the provided emailDomain (choose one approach
consistently). Locate the InvoiceAddParams type and the addInvoice function and
either delete the emailDomain property from InvoiceAddParams and any callers
that pass it, or modify addInvoice to prefer the passed emailDomain over
deriving from contactEmail (ensuring normalizedDomain is computed from the
chosen source).
| const dynamoCommand = new TransactWriteItemsCommand({ | ||
| TransactItems: [ | ||
| { | ||
| Put: { | ||
| TableName: genericConfig.StripePaymentsDynamoTableName, | ||
| Item: marshall( | ||
| { | ||
| primaryKey: pk, | ||
| sortKey: `CHARGE#${invoiceId}`, | ||
| invoiceAmtUsd: invoiceAmountUsd, | ||
| createdAt: new Date().toISOString(), | ||
| }, | ||
| { removeUndefinedValues: true }, | ||
| ), | ||
| ConditionExpression: | ||
| "attribute_not_exists(primaryKey) AND attribute_not_exists(sortKey)", | ||
| }, | ||
| }, | ||
| { | ||
| Update: { | ||
| TableName: genericConfig.StripePaymentsDynamoTableName, | ||
| Key: { | ||
| primaryKey: { S: pk }, | ||
| sortKey: { S: "CUSTOMER" }, | ||
| }, | ||
| UpdateExpression: "SET totalAmount = totalAmount + :inc", | ||
| ExpressionAttributeValues: { | ||
| ":inc": { N: invoiceAmountUsd.toString() }, | ||
| }, | ||
| }, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| await dynamoClient.send(dynamoCommand); | ||
| return { customerId: result.customerId }; |
There was a problem hiding this comment.
Handle ConditionalCheckFailedException for idempotent retry behavior.
The ConditionExpression on the CHARGE Put (lines 569-570) ensures idempotency by rejecting duplicate invoice writes. However, if a retry occurs with the same invoiceId, the transaction fails with TransactionCanceledException containing ConditionalCheckFailedException, which will propagate as a 500 error to the client.
For true idempotency, catch this specific failure and return success (since the CHARGE was already recorded):
🛡️ Suggested fix for idempotent retry handling
- await dynamoClient.send(dynamoCommand);
- return { customerId: result.customerId };
+ try {
+ await dynamoClient.send(dynamoCommand);
+ } catch (e) {
+ // Handle idempotent retry: if CHARGE already exists, treat as success
+ if (
+ e instanceof Error &&
+ e.name === "TransactionCanceledException" &&
+ (e as any).CancellationReasons?.some(
+ (r: any) => r.Code === "ConditionalCheckFailed",
+ )
+ ) {
+ console.info(
+ `Invoice ${invoiceId} already exists for ${pk}, returning existing customer`,
+ );
+ return { customerId: result.customerId };
+ }
+ throw e;
+ }
+ return { customerId: result.customerId };📝 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 dynamoCommand = new TransactWriteItemsCommand({ | |
| TransactItems: [ | |
| { | |
| Put: { | |
| TableName: genericConfig.StripePaymentsDynamoTableName, | |
| Item: marshall( | |
| { | |
| primaryKey: pk, | |
| sortKey: `CHARGE#${invoiceId}`, | |
| invoiceAmtUsd: invoiceAmountUsd, | |
| createdAt: new Date().toISOString(), | |
| }, | |
| { removeUndefinedValues: true }, | |
| ), | |
| ConditionExpression: | |
| "attribute_not_exists(primaryKey) AND attribute_not_exists(sortKey)", | |
| }, | |
| }, | |
| { | |
| Update: { | |
| TableName: genericConfig.StripePaymentsDynamoTableName, | |
| Key: { | |
| primaryKey: { S: pk }, | |
| sortKey: { S: "CUSTOMER" }, | |
| }, | |
| UpdateExpression: "SET totalAmount = totalAmount + :inc", | |
| ExpressionAttributeValues: { | |
| ":inc": { N: invoiceAmountUsd.toString() }, | |
| }, | |
| }, | |
| }, | |
| ], | |
| }); | |
| await dynamoClient.send(dynamoCommand); | |
| return { customerId: result.customerId }; | |
| const dynamoCommand = new TransactWriteItemsCommand({ | |
| TransactItems: [ | |
| { | |
| Put: { | |
| TableName: genericConfig.StripePaymentsDynamoTableName, | |
| Item: marshall( | |
| { | |
| primaryKey: pk, | |
| sortKey: `CHARGE#${invoiceId}`, | |
| invoiceAmtUsd: invoiceAmountUsd, | |
| createdAt: new Date().toISOString(), | |
| }, | |
| { removeUndefinedValues: true }, | |
| ), | |
| ConditionExpression: | |
| "attribute_not_exists(primaryKey) AND attribute_not_exists(sortKey)", | |
| }, | |
| }, | |
| { | |
| Update: { | |
| TableName: genericConfig.StripePaymentsDynamoTableName, | |
| Key: { | |
| primaryKey: { S: pk }, | |
| sortKey: { S: "CUSTOMER" }, | |
| }, | |
| UpdateExpression: "SET totalAmount = totalAmount + :inc", | |
| ExpressionAttributeValues: { | |
| ":inc": { N: invoiceAmountUsd.toString() }, | |
| }, | |
| }, | |
| }, | |
| ], | |
| }); | |
| try { | |
| await dynamoClient.send(dynamoCommand); | |
| } catch (e) { | |
| // Handle idempotent retry: if CHARGE already exists, treat as success | |
| if ( | |
| e instanceof Error && | |
| e.name === "TransactionCanceledException" && | |
| (e as any).CancellationReasons?.some( | |
| (r: any) => r.Code === "ConditionalCheckFailed", | |
| ) | |
| ) { | |
| console.info( | |
| `Invoice ${invoiceId} already exists for ${pk}, returning existing customer`, | |
| ); | |
| return { customerId: result.customerId }; | |
| } | |
| throw e; | |
| } | |
| return { customerId: result.customerId }; |
🤖 Prompt for AI Agents
In `@src/api/functions/stripe.ts` around lines 555 - 590, Wrap the await
dynamoClient.send(dynamoCommand) call in a try/catch and handle idempotent
retries: if the catch error is a TransactionCanceledException and its
CancellationReasons array (or error payload) indicates a
ConditionalCheckFailedException for the CHARGE Put (i.e., the conditional
idempotency failed because the invoice already exists), swallow the error and
return the same success shape (return { customerId: result.customerId });
otherwise rethrow the error. Use the existing dynamoCommand /
TransactWriteItemsCommand and dynamoClient.send identifiers to locate where to
add the try/catch and inspect e.name === 'TransactionCanceledException' and
e.CancellationReasons.some(r => r.Code === 'ConditionalCheckFailed') (or
equivalent fields) to detect the specific condition.
3 participants
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.