Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR adds email idempotency functionality across the codebase. An optional Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const pendingRecord: EmailIdempotencyRecord = { | ||
| status: "PENDING", | ||
| createdAt: existingRecord?.createdAt ?? nowIso, | ||
| lastUpdatedAt: nowIso, | ||
| }; |
There was a problem hiding this comment.
The code writes a PENDING idempotency record in Redis as soon as an idempotencyKey is supplied, but there is no matching cleanup when the function exits early before the queueing try block (e.g., invalid API key, suppressed recipients, or any other validation error thrown later in sendEmail). In those cases the Redis key remains stuck with status PENDING for the full three‑day TTL, so subsequent requests with the same idempotency key will always hit the NOT_UNIQUE branch even though no email was actually processed. This blocks legitimate retries and leaks keys until the TTL expires. Consider wrapping the validation and early returns in a try/finally that deletes or marks the key as FAILED so clients can retry safely.
Useful? React with 👍 / 👎.
Deploying usesend with
|
| Latest commit: |
65a0904
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d9e2f633.usesend.pages.dev |
| Branch Preview URL: | https://codex-implement-idempotency.usesend.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/web/src/server/redis.ts (1)
82-94: Consider logging JSON parse errors for observability.The
getJsonValuefunction silently returnsnullwhen JSON parsing fails (lines 89-93). While acceptable for cache misses, this could hide data corruption issues in idempotency records. Consider logging parse errors to aid debugging:try { return JSON.parse(cached) as T; } catch { + logger.warn({ key }, "Failed to parse JSON from Redis"); return null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/docs/docs.json(1 hunks)apps/docs/guides/email-idempotency.mdx(1 hunks)apps/web/src/server/public-api/api/emails/send-email.ts(1 hunks)apps/web/src/server/public-api/schemas/email-schema.ts(1 hunks)apps/web/src/server/redis.ts(1 hunks)apps/web/src/server/service/email-service.ts(5 hunks)apps/web/src/types/index.ts(1 hunks)packages/python-sdk/usesend/emails.py(1 hunks)packages/python-sdk/usesend/types.py(1 hunks)packages/sdk/src/email.ts(1 hunks)packages/sdk/types/schema.d.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Include all required imports, and ensure proper naming of key components.
Files:
apps/web/src/server/redis.tspackages/sdk/types/schema.d.tsapps/web/src/server/public-api/schemas/email-schema.tspackages/sdk/src/email.tsapps/web/src/types/index.tsapps/web/src/server/service/email-service.tsapps/web/src/server/public-api/api/emails/send-email.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: TypeScript-first: use .ts/.tsx for source code (avoid JavaScript source files)
Use 2-space indentation and semicolons (Prettier 3 enforces these)
Adhere to @usesend/eslint-config; fix all ESLint warnings (CI fails on warnings)
Do not use dynamic imports; always place imports at the top of the module
Files:
apps/web/src/server/redis.tspackages/sdk/types/schema.d.tsapps/web/src/server/public-api/schemas/email-schema.tspackages/sdk/src/email.tsapps/web/src/types/index.tsapps/web/src/server/service/email-service.tsapps/web/src/server/public-api/api/emails/send-email.ts
**/*.{ts,tsx,md}
📄 CodeRabbit inference engine (AGENTS.md)
Format code with Prettier 3 (run pnpm format)
Files:
apps/web/src/server/redis.tspackages/sdk/types/schema.d.tsapps/web/src/server/public-api/schemas/email-schema.tspackages/sdk/src/email.tsapps/web/src/types/index.tsapps/web/src/server/service/email-service.tsapps/web/src/server/public-api/api/emails/send-email.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/**/*.{ts,tsx}: In apps/web, use the/ alias for src imports (e.g., import { x } from "/utils/x")
Prefer using tRPC in apps/web unless explicitly asked otherwise
Files:
apps/web/src/server/redis.tsapps/web/src/server/public-api/schemas/email-schema.tsapps/web/src/types/index.tsapps/web/src/server/service/email-service.tsapps/web/src/server/public-api/api/emails/send-email.ts
🧬 Code graph analysis (3)
apps/web/src/server/service/email-service.ts (4)
apps/web/src/server/redis.ts (3)
getJsonValue(82-94)deleteKey(96-99)setJsonValue(57-80)apps/web/src/server/db.ts (1)
db(20-20)apps/web/src/server/logger/log.ts (1)
logger(31-63)apps/web/src/server/public-api/api-error.ts (1)
UnsendApiError(62-75)
apps/web/src/server/public-api/api/emails/send-email.ts (1)
apps/web/src/server/service/email-service.ts (1)
sendEmail(114-498)
packages/python-sdk/usesend/emails.py (2)
packages/python-sdk/usesend/usesend.py (1)
get(110-111)packages/python-sdk/usesend/contacts.py (1)
get(34-40)
🔇 Additional comments (15)
packages/python-sdk/usesend/types.py (1)
213-213: LGTM! Idempotency key field added correctly.The optional
idempotencyKeyfield is properly typed and integrated into theEmailCreateTypedDict.packages/python-sdk/usesend/emails.py (1)
45-50: LGTM! Idempotency key normalization is correctly implemented.The normalization logic properly trims whitespace and removes empty keys before sending the request. This ensures clean, consistent idempotency keys reach the API.
apps/web/src/server/public-api/schemas/email-schema.ts (1)
10-19: LGTM! Schema validation is properly configured.The idempotency key validation includes trimming, length constraints (1-255 chars), and clear API documentation. This provides robust server-side validation.
apps/docs/docs.json (1)
41-44: LGTM! Documentation navigation updated correctly.The new idempotency guide is properly integrated into the Guides section.
apps/docs/guides/email-idempotency.mdx (1)
1-49: LGTM! Clear and comprehensive documentation.The idempotency guide effectively explains the feature with practical examples for both JavaScript and Python SDKs. The note about cached responses and automatic failure cleanup provides important usage context.
packages/sdk/src/email.ts (1)
85-93: LGTM! Idempotency key normalization is correctly implemented.The normalization logic properly trims whitespace and removes empty keys, consistent with the Python SDK implementation. This ensures clean, consistent idempotency keys reach the API.
apps/web/src/types/index.ts (1)
4-4: LGTM! Type definition added correctly.The optional
idempotencyKeyfield is properly integrated into theEmailContenttype.packages/sdk/types/schema.d.ts (1)
573-574: LGTM! Schema definition correctly includes idempotency key.The auto-generated schema properly reflects the idempotencyKey field with appropriate documentation. The source OpenAPI specification is correctly configured.
apps/web/src/server/public-api/api/emails/send-email.ts (1)
35-50: LGTM! Clean refactor improves readability.Consolidating the validated JSON payload into a single
bodyvariable eliminates redundantc.req.valid("json")calls and makes the code more maintainable. The optional chaining on line 39 (body?.html) is appropriate defensive coding for an optional schema field.apps/web/src/server/redis.ts (1)
96-99: LGTM!Simple, correct implementation of the Redis DEL wrapper.
apps/web/src/server/service/email-service.ts (5)
16-18: LGTM! Well-defined idempotency constants.The 3-day TTL aligns with the PR objectives and provides a reasonable window for duplicate detection. The 500ms total wait time (5 × 100ms) appropriately handles concurrent request resolution without excessive blocking.
31-43: LGTM! Correct concurrent request handling.The polling logic correctly exits early when the idempotent operation completes (emailId present), fails, or the record is deleted. The 500ms total wait time balances responsiveness with handling legitimate concurrent requests.
45-72: LGTM! Robust cached result resolution.Good defensive programming: handles stale Redis records (emailId present but email deleted from DB) by cleaning up the key. The informative logging aids debugging and makes the 3-day expiry explicit.
421-462: LGTM! Correct idempotency record lifecycle updates.The CREATED and QUEUED state transitions correctly preserve the original
createdAttimestamp while updatinglastUpdatedAtand adding status-specific metadata (emailId,queuedAt). These updates will work correctly once the critical error-handling issue (lines 139-224) is addressed.
479-493: LGTM! Correct failure state recording for queueing errors.The FAILED state update correctly captures queueing errors with the error message and timestamp. This handles the post-creation failure path properly. The pre-creation failure path (validation errors, etc.) requires the try-catch wrapper suggested in the earlier comment for lines 139-224.
| export async function setJsonValue( | ||
| key: string, | ||
| value: unknown, | ||
| options?: RedisSetJsonOptions | ||
| ): Promise<"OK" | null> { | ||
| const redis = getRedis(); | ||
| const payload = JSON.stringify(value); | ||
| const mode = options?.mode; | ||
| const ttlSeconds = options?.ttlSeconds; | ||
|
|
||
| if (mode && ttlSeconds) { | ||
| return redis.set(key, payload, mode, "EX", ttlSeconds); | ||
| } | ||
|
|
||
| if (mode) { | ||
| return redis.set(key, payload, mode); | ||
| } | ||
|
|
||
| if (ttlSeconds) { | ||
| return redis.set(key, payload, "EX", ttlSeconds); | ||
| } | ||
|
|
||
| return redis.set(key, payload); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify ioredis SET command argument order.
The setJsonValue implementation on line 68 passes arguments as redis.set(key, payload, mode, "EX", ttlSeconds) where mode is "NX" or "XX". While Redis accepts these flags in any order, ensure that ioredis v5.6.1 TypeScript overloads support this exact signature to avoid type errors or runtime issues.
Alternatively, test this syntax in the integration tests or verify against ioredis documentation:
🏁 Script executed:
#!/bin/bash
# Verify ioredis SET command syntax supports NX/XX with EX in this argument order
# Search for ioredis SET usage patterns in the codebase
rg -n -C3 'redis\.set\(' --type=ts
# Check ioredis type definitions
fd -e d.ts -x cat {} \; | rg -A5 'interface Redis' | head -50Length of output: 960
🌐 Web query:
ioredis v5 SET command with NX and EX options syntax
💡 Result:
Use positional arguments (they're passed straight to Redis). Example:
- redis.set("key", "value", "EX", 60, "NX"); // set if not exists, expires in 60s. [1][2]
Notes:
- EX takes seconds (or use "PX" and milliseconds). NX and XX are mutually exclusive. [2]
Sources:
[1] ioredis README examples.
[2] Redis SET command documentation.
Fix argument order for redis.set() with NX/XX and EX options on line 68.
The call on line 68 passes arguments in incorrect order: redis.set(key, payload, mode, "EX", ttlSeconds). According to Redis protocol, expiration options (EX, seconds) must come before NX/XX flags. Correct the order to: redis.set(key, payload, "EX", ttlSeconds, mode).
🤖 Prompt for AI Agents
In apps/web/src/server/redis.ts around lines 57 to 80, the redis.set call that
combines mode (NX/XX) and expiration currently passes arguments in the wrong
order; change the call from redis.set(key, payload, mode, "EX", ttlSeconds) to
pass the expiration before the mode, i.e. redis.set(key, payload, "EX",
ttlSeconds, mode), ensuring all other branches remain consistent and return the
redis.set result.
| const normalizedIdempotencyKey = idempotencyKey?.trim(); | ||
| const idempotencyRedisKey = normalizedIdempotencyKey | ||
| ? `email-idempotency:${teamId}:${normalizedIdempotencyKey}` | ||
| : null; | ||
| let idempotencyRecordSnapshot: EmailIdempotencyRecord | null = null; | ||
| let idempotencyCreatedAt = new Date().toISOString(); | ||
|
|
||
| if (idempotencyRedisKey) { | ||
| let existingRecord = await getJsonValue<EmailIdempotencyRecord>( | ||
| idempotencyRedisKey | ||
| ); | ||
|
|
||
| if (existingRecord?.status === "FAILED") { | ||
| await deleteKey(idempotencyRedisKey); | ||
| existingRecord = null; | ||
| } else { | ||
| const cachedEmail = await resolveEmailFromRecord( | ||
| existingRecord, | ||
| idempotencyRedisKey, | ||
| teamId | ||
| ); | ||
|
|
||
| if (cachedEmail) { | ||
| return cachedEmail; | ||
| } | ||
|
|
||
| if (existingRecord?.status === "PENDING") { | ||
| const awaitedRecord = await waitForIdempotentRecord( | ||
| idempotencyRedisKey | ||
| ); | ||
| const awaitedEmail = await resolveEmailFromRecord( | ||
| awaitedRecord, | ||
| idempotencyRedisKey, | ||
| teamId | ||
| ); | ||
|
|
||
| if (awaitedEmail) { | ||
| return awaitedEmail; | ||
| } | ||
|
|
||
| if (awaitedRecord?.status === "FAILED") { | ||
| await deleteKey(idempotencyRedisKey); | ||
| existingRecord = null; | ||
| } else { | ||
| existingRecord = awaitedRecord; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const nowIso = new Date().toISOString(); | ||
| const pendingRecord: EmailIdempotencyRecord = { | ||
| status: "PENDING", | ||
| createdAt: existingRecord?.createdAt ?? nowIso, | ||
| lastUpdatedAt: nowIso, | ||
| }; | ||
|
|
||
| const setResult = await setJsonValue(idempotencyRedisKey, pendingRecord, { | ||
| ttlSeconds: IDEMPOTENCY_TTL_SECONDS, | ||
| mode: "NX", | ||
| }); | ||
|
|
||
| if (setResult !== "OK") { | ||
| const latestRecord = await waitForIdempotentRecord(idempotencyRedisKey); | ||
| const latestEmail = await resolveEmailFromRecord( | ||
| latestRecord, | ||
| idempotencyRedisKey, | ||
| teamId | ||
| ); | ||
|
|
||
| if (latestEmail) { | ||
| return latestEmail; | ||
| } | ||
|
|
||
| if (latestRecord?.status === "FAILED") { | ||
| await deleteKey(idempotencyRedisKey); | ||
| } | ||
|
|
||
| throw new UnsendApiError({ | ||
| code: "NOT_UNIQUE", | ||
| message: "A request with this idempotency key is already in progress.", | ||
| }); | ||
| } | ||
|
|
||
| idempotencyCreatedAt = pendingRecord.createdAt; | ||
| idempotencyRecordSnapshot = pendingRecord; | ||
| } |
There was a problem hiding this comment.
Critical: Wrap idempotency flow in try-catch to clean up PENDING records.
If an error occurs after creating the PENDING record (line 195) but before updating to CREATED (line 430)—such as validation failures on lines 236-246 or lines 377-389—the PENDING record remains stuck in Redis for 3 days. Subsequent requests with the same idempotencyKey will wait, then throw NOT_UNIQUE errors until the TTL expires.
Wrap the entire flow from line 224 onward in a try-catch block to update the record to FAILED on any error:
idempotencyCreatedAt = pendingRecord.createdAt;
idempotencyRecordSnapshot = pendingRecord;
}
+
+ try {
let domain: Awaited<ReturnType<typeof validateDomainFromEmail>>;
// ... (rest of the email creation flow)
return email;
+
+ } catch (error: any) {
+ if (idempotencyRedisKey) {
+ const failedAt = new Date().toISOString();
+ const failureRecord: EmailIdempotencyRecord = {
+ status: "FAILED",
+ createdAt: idempotencyCreatedAt,
+ lastUpdatedAt: failedAt,
+ error: error instanceof Error ? error.message : String(error),
+ };
+ await setJsonValue(idempotencyRedisKey, failureRecord, {
+ ttlSeconds: IDEMPOTENCY_TTL_SECONDS,
+ });
+ }
+ throw error;
+ }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/web/src/server/service/email-service.ts around lines 139 to 224, the
idempotency PENDING record can be left in Redis if an error occurs after it is
created; wrap the entire processing flow that follows creation of the PENDING
record (starting just after the setJsonValue that writes PENDING) in a try-catch
so that any thrown error results in updating the idempotency Redis key to a
FAILED EmailIdempotencyRecord (set status: "FAILED" and lastUpdatedAt to nowIso,
keep createdAt from the snapshot) before rethrowing; ensure the update is
written atomically (overwrite) and handle cases where the key was removed by
other flows (no-op if delete occurs), and rethrow the original error so upstream
error handling remains unchanged.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 11 files
Prompt for AI agents (all 3 issues)
Understand the root cause of the following 3 issues and fix them.
<file name="apps/docs/guides/email-idempotency.mdx">
<violation number="1" location="apps/docs/guides/email-idempotency.mdx:9">
This line claims all duplicate requests return the first queued email, but the API sends an "already in progress" error when the original request hasn't finished. Please clarify the pending-request behavior.</violation>
</file>
<file name="apps/web/src/server/service/email-service.ts">
<violation number="1" location="apps/web/src/server/service/email-service.ts:195">
Errors thrown after creating the PENDING idempotency record can leave the key stuck for up to 3 days, causing subsequent requests with the same idempotencyKey to fail with NOT_UNIQUE. Wrap the processing that follows creation of the PENDING record in a try-catch and, on any error, update the Redis record to status "FAILED" (preserving createdAt and setting lastUpdatedAt) before rethrowing to avoid blocking retries.</violation>
<violation number="2" location="apps/web/src/server/service/email-service.ts:457">
If the Redis cache write fails after queueEmail succeeds, this line throws and we immediately mark the send as FAILED even though the job is already queued, so callers see an error and may retry, generating duplicate sends. Guard the idempotency cache update so Redis failures don't flip the request into the failure path.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| ## Overview | ||
|
|
||
| When you provide an `idempotencyKey` with the email send endpoint, useSend will | ||
| short-circuit duplicate requests and return the first queued email. The key is |
There was a problem hiding this comment.
This line claims all duplicate requests return the first queued email, but the API sends an "already in progress" error when the original request hasn't finished. Please clarify the pending-request behavior.
Prompt for AI agents
Address the following comment on apps/docs/guides/email-idempotency.mdx at line 9:
<comment>This line claims all duplicate requests return the first queued email, but the API sends an "already in progress" error when the original request hasn't finished. Please clarify the pending-request behavior.</comment>
<file context>
@@ -0,0 +1,49 @@
+## Overview
+
+When you provide an `idempotencyKey` with the email send endpoint, useSend will
+short-circuit duplicate requests and return the first queued email. The key is
+scoped to the team and automatically expires after three days so you do not need
+any manual cleanup.
</file context>
| lastUpdatedAt: queuedAt, | ||
| }; | ||
|
|
||
| await setJsonValue(idempotencyRedisKey, queuedRecord, { |
There was a problem hiding this comment.
If the Redis cache write fails after queueEmail succeeds, this line throws and we immediately mark the send as FAILED even though the job is already queued, so callers see an error and may retry, generating duplicate sends. Guard the idempotency cache update so Redis failures don't flip the request into the failure path.
Prompt for AI agents
Address the following comment on apps/web/src/server/service/email-service.ts at line 457:
<comment>If the Redis cache write fails after queueEmail succeeds, this line throws and we immediately mark the send as FAILED even though the job is already queued, so callers see an error and may retry, generating duplicate sends. Guard the idempotency cache update so Redis failures don't flip the request into the failure path.</comment>
<file context>
@@ -280,6 +443,23 @@ export async function sendEmail(
+ lastUpdatedAt: queuedAt,
+ };
+
+ await setJsonValue(idempotencyRedisKey, queuedRecord, {
+ ttlSeconds: IDEMPOTENCY_TTL_SECONDS,
+ });
</file context>
| lastUpdatedAt: nowIso, | ||
| }; | ||
|
|
||
| const setResult = await setJsonValue(idempotencyRedisKey, pendingRecord, { |
There was a problem hiding this comment.
Errors thrown after creating the PENDING idempotency record can leave the key stuck for up to 3 days, causing subsequent requests with the same idempotencyKey to fail with NOT_UNIQUE. Wrap the processing that follows creation of the PENDING record in a try-catch and, on any error, update the Redis record to status "FAILED" (preserving createdAt and setting lastUpdatedAt) before rethrowing to avoid blocking retries.
Prompt for AI agents
Address the following comment on apps/web/src/server/service/email-service.ts at line 195:
<comment>Errors thrown after creating the PENDING idempotency record can leave the key stuck for up to 3 days, causing subsequent requests with the same idempotencyKey to fail with NOT_UNIQUE. Wrap the processing that follows creation of the PENDING record in a try-catch and, on any error, update the Redis record to status "FAILED" (preserving createdAt and setting lastUpdatedAt) before rethrowing to avoid blocking retries.</comment>
<file context>
@@ -72,10 +131,98 @@ export async function sendEmail(
+ lastUpdatedAt: nowIso,
+ };
+
+ const setResult = await setJsonValue(idempotencyRedisKey, pendingRecord, {
+ ttlSeconds: IDEMPOTENCY_TTL_SECONDS,
+ mode: "NX",
</file context>
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68faf7f22c5c8329a7f92d920f50693a
Summary by cubic
Adds optional idempotencyKey to the email send API to deduplicate duplicate requests, using Redis reservations and cached results with a 3-day TTL. SDKs normalize the key, and a new guide documents usage.
Summary by CodeRabbit
Release Notes
New Features
idempotencyKeyparameter when sending emails to deduplicate requests and reuse cached responses for up to 3 days.Documentation