You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.

To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

.vscode/settings.json (1)

19-19: Consider alphabetical ordering in the cSpell.words list.

The word "dbgenerated" was inserted between "clsx" and "cmdk", but alphabetically it should appear after "cmdk" (since 'c' < 'd'). While this doesn't affect functionality, keeping the list sorted helps with maintainability.

If you'd like to reorder, move "dbgenerated" to after "cmdk" at line 20.

apps/e2e/tests/backend/endpoints/api/v1/auth/email-normalization.test.ts (1)

176-180: LGTM! Improved test reliability with subject-based message filtering.

The change to use waitForMessagesWithSubject makes the test more specific and reliable by directly filtering for the password reset email. The validation of message count and direct access to messages[0] are both appropriate.

Optional: Consider extracting the subject string to a constant.

The subject "Reset your password at New Project" is duplicated on lines 176 and 251. Extracting it to a constant would improve maintainability if the subject format changes.

Apply this refactor if desired:

+const PASSWORD_RESET_EMAIL_SUBJECT = "Reset your password at New Project";
+
 it("password reset should work with normalized email when account was created with unnormalized email", async ({ expect }) => {
   // ... setup code ...
   
   // Get the reset code from the mailbox
-  const messages = await backendContext.value.mailbox.waitForMessagesWithSubject("Reset your password at New Project");
+  const messages = await backendContext.value.mailbox.waitForMessagesWithSubject(PASSWORD_RESET_EMAIL_SUBJECT);

And apply the same change on line 251.

packages/stack-shared/src/utils/results.tsx (1)

367-385: Typed retry<T, E> correctly propagates E into RetryError<E>

The new signature (fn returning Result<T, E>/Promise<Result<T, E>>, collecting const errors: E[], and returning Result<T, RetryError<E>> & { attempts: number }) matches the implementation: success path returns a typed ok result with attempts, and the failure path wraps the collected E[] in new RetryError(errors). Existing semantics (delay schedule, attempts counting) are preserved and tests cover first-try success, eventual success, and all-fail scenarios.

If you ever expect totalAttempts to be 0 or negative, you might optionally guard that case (e.g., throw or early-return) to avoid constructing a RetryError with an empty errors array and cause: undefined, but that’s preexisting behavior and not introduced by this change.

apps/e2e/tests/backend/endpoints/api/v1/internal/failed-emails-digest.test.ts (1)

203-428: TODO block and /api/v1/send-email usages to revisit when re-enabling digest

The remaining scenarios are correctly marked as it.todo with clear TODO comments. They still hit /api/v1/send-email (rather than /api/v1/emails/send-email) and assume particular response shapes (e.g. { "results": [] }), which may diverge from the outbox-backed flow over time.

When you re-enable failed-emails digest, it would be good to:

• Double-check that /api/v1/send-email is still the right endpoint for these failure setups, or align them with /api/v1/emails/send-email if that’s now canonical.
• Reuse the testFailedEmails helper (once it wires query through) where possible to avoid duplicating “send failing email” setup across tests.

No immediate breakage since these are todos, but worth keeping on the radar before switching them back to active tests.

apps/backend/src/prisma-client.tsx (3)

74-90: Keep Postgres client caches in sync with the legacy global store

The new postgresPrismaClientsStore works as a cache, but prismaClientsStore.postgres (the legacy map stored on globalVar.__stack_prisma_clients) is no longer updated. If anything else in the codebase still relies on that legacy map, it will now see an empty cache.

To preserve existing behavior and align with the Neon path, consider:

-const postgresPrismaClientsStore: Map<string, {
-  client: PrismaClient,
-  schema: string | null,
-}> = globalVar.__stack_postgres_prisma_clients ??= new Map();
-function getPostgresPrismaClient(connectionString: string) {
-  let postgresPrismaClient = postgresPrismaClientsStore.get(connectionString);
+const postgresPrismaClientsStore: Map<string, {
+  client: PrismaClient,
+  schema: string | null,
+}> = globalVar.__stack_postgres_prisma_clients ??= new Map();
+function getPostgresPrismaClient(connectionString: string) {
+  let postgresPrismaClient =
+    postgresPrismaClientsStore.get(connectionString) ??
+    prismaClientsStore.postgres.get(connectionString);
   if (!postgresPrismaClient) {
     const schema = getSchemaFromConnectionString(connectionString);
     const adapter = new PrismaPg({ connectionString }, schema ? { schema } : undefined);
     postgresPrismaClient = {
       client: new PrismaClient({ adapter }),
       schema,
     };
-    postgresPrismaClientsStore.set(connectionString, postgresPrismaClient);
+    postgresPrismaClientsStore.set(connectionString, postgresPrismaClient);
+    prismaClientsStore.postgres.set(connectionString, postgresPrismaClient);
   }
   return postgresPrismaClient;
 }

---

92-127: Harden global connection resolution and verify top‑level await support

The tcpPing + OrbStack probe is a nice dev-only optimization, but there are a couple of edge cases to tighten up:

• originalGlobalConnectionString is obtained via getEnvVariable("STACK_DATABASE_CONNECTION_STRING", ""). If that env var is unset, you end up with "", which flows into getPostgresPrismaClient(actualGlobalConnectionString) → getSchemaFromConnectionString("") → new URL(""), causing an opaque Invalid URL at import time. It would be clearer to fail fast when the env var is missing instead of relying on new URL to blow up. For example:

-const originalGlobalConnectionString = getEnvVariable("STACK_DATABASE_CONNECTION_STRING", "");
+const originalGlobalConnectionString = getEnvVariable("STACK_DATABASE_CONNECTION_STRING");

or explicitly throw if `originalGlobalConnectionString.trim().length === 0` before using it.

- `actualGlobalConnectionString` is computed with a top‑level `await`. That requires this module to be compiled as an ES module / NodeNext and run in a runtime that supports top‑level `await`. Please double‑check the backend tsconfig and deployment target so this doesn’t introduce a subtle startup error in environments that still expect CJS-only modules.

The rest of the OrbStack detection logic (short `tcpPing` timeout, darwin+development guard) looks reasonable.

---

`26-36`: **Avoid keeping an extra unused global PrismaClient alive**

With `globalPrismaClient` now sourced from `getPostgresPrismaClient(actualGlobalConnectionString)`, the `prismaClientsStore.global = new PrismaClient()` created at module init appears to be unused (at least within this file), yet still opens a DB connection and is stored on `globalVar.__stack_prisma_clients` in development.

If nothing else in the repo relies on `globalVar.__stack_prisma_clients.global`, consider simplifying:

- Remove the `global` field from `prismaClientsStore` entirely, or
- Make `prismaClientsStore.global` a reference/alias to `globalPrismaClient` instead of a separate client.

This avoids an unnecessary connection and clarifies which client is the canonical “global” one.




Also applies to: 130-130

</blockquote></details>
<details>
<summary>apps/backend/src/route-handlers/smart-route-handler.tsx (1)</summary><blockquote>

`105-113`: **Correct exclusion for long-running email queue endpoint.**

The conditional correctly excludes the email queue step endpoint from the timeout watcher, which is appropriate for long-running background processing.




Consider extracting the path to a constant to avoid typos and improve maintainability:

```diff
+const EMAIL_QUEUE_STEP_PATH = "/api/latest/internal/email-queue-step";
+
// request duration warning
-if (req.nextUrl.pathname !== "/api/latest/internal/email-queue-step") {
+if (req.nextUrl.pathname !== EMAIL_QUEUE_STEP_PATH) {
 const warnAfterSeconds = 12;

apps/e2e/tests/js/email.test.ts (1)

174-225: Consider polling pattern when enabling this test.

The test uses a fixed 5-second wait, which could be flaky. Consider using a polling pattern with a timeout instead:

// Wait for email delivery with polling
let info;
const maxAttempts = 20;
for (let i = 0; i < maxAttempts; i++) {
  await wait(500);
  info = await serverApp.getEmailDeliveryStats();
  if (info.stats.hour.sent > 0) {
    break;
  }
}
expect(info.stats.hour.sent).toBeGreaterThan(0);

Also, the hardcoded snapshot values (like ratePerSecond: 1.561904761904762) may be non-deterministic depending on timing. Consider asserting on structure and ranges rather than exact values when you enable this test.

apps/backend/src/lib/notification-categories.ts (1)

5-5: Unsubscribe verification wiring looks correct; consider centralizing category lookup & validation

The switch to unsubscribeLinkVerificationCodeHandler.createCode with method: {} and a data payload of { user_id, notification_category_id } matches the handler’s schema and looks good.

You now have both getNotificationCategoryByName and getNotificationCategoryById; to keep behavior consistent with hasNotificationEnabled (which throws on invalid ids), consider:

• Reusing getNotificationCategoryById inside both hasNotificationEnabled and generateUnsubscribeLink.
• Throwing a StackAssertionError in generateUnsubscribeLink when the id is unknown, so bad ids fail fast instead of producing unusable links.

This removes the ad‑hoc find call and aligns all call sites on one source of truth for valid category ids.

Also applies to: 29-31, 54-61

apps/e2e/tests/setup.ts (1)

1-2: Ensure callbacks array is cleared even if one cleanup callback throws

The afterEach hook correctly runs all registered afterTestFinishesCallbacks, but if any callback rejects, the function returns early and afterTestFinishesCallbacks.length = 0 is never executed. That can leak callbacks into later tests.

You can preserve current behavior (fail fast on first error) while guaranteeing cleanup via finally:

-afterEach(async () => {
-  for (const callback of afterTestFinishesCallbacks) {
-    await callback();
-  }
-  afterTestFinishesCallbacks.length = 0;
-});
+afterEach(async () => {
+  try {
+    for (const callback of afterTestFinishesCallbacks) {
+      await callback();
+    }
+  } finally {
+    afterTestFinishesCallbacks.length = 0;
+  }
+});

This keeps the lifecycle hook simple and avoids cross‑test contamination when a teardown step fails.

Also applies to: 13-18

apps/backend/src/app/api/latest/internal/failed-emails-digest/route.ts (1)

1-1: Failed‑emails digest sending is now deliberately disabled

In non‑dry_run mode you now throw StackAssertionError("Failed emails digest is currently disabled!"), mark anyDigestsFailedToSend = true, and capture the error. This cleanly disables actual digest sending while still surfacing failures via status 500 / success: false, which seems intentional for the outbox migration.

Given this, you might consider short‑circuiting earlier (before building emailHtml / fetching email config) or guarding this whole block behind a feature flag to avoid doing unused work until the digest path is re‑enabled.

Also applies to: 46-76

apps/backend/scripts/run-email-queue.ts (1)

12-21: Consider the operational implications of setInterval for a long-running process.

This script uses setInterval to create a continuously-running process that calls the endpoint every 60 seconds. This approach requires:

• The script to run indefinitely without crashes
• Proper process management/restart on failures
• Resource allocation for a persistent process

Verify that your deployment environment supports long-running scripts and has appropriate monitoring/restart mechanisms in place.

apps/e2e/tests/backend/endpoints/api/v1/emails/delivery-info.test.ts (1)

69-74: The fixed 5-second wait makes the test timing-dependent and potentially flaky.

The test uses a fixed 5-second wait assuming the background email queue processor will complete within that time. This approach can lead to:

• Flaky test failures if processing takes longer (e.g., under load, CI slowness)
• Unnecessarily slow tests if processing completes faster

Consider implementing a polling mechanism with timeout:

// Poll for updated stats with timeout
const maxWaitMs = 10000;
const pollIntervalMs = 500;
let elapsed = 0;

while (elapsed < maxWaitMs) {
  const response = await niceBackendFetch("/api/v1/emails/delivery-info", {
    method: "GET",
    accessType: "server",
  });
  
  if (response.body.stats.hour.sent === 1) {
    break;
  }
  
  await wait(pollIntervalMs);
  elapsed += pollIntervalMs;
}

packages/template/src/lib/stack-app/email/index.ts (1)

33-54: Clarify naming convention and alignment with backend delivery‑stats types

These new public template types use snake_case fields (marked_as_spam, rate_per_second, penalty_factor), while other template‑level types here (e.g. AdminSentEmail) are camelCase, and the backend’s internal EmailDeliveryWindowStats uses markedAsSpam (camelCase) in apps/backend/src/lib/email-delivery-stats.tsx. It would be good to:

• Decide whether template‑level types should consistently be camelCase (with the interface layer mapping to/from snake_case JSON), and
• Ensure these definitions stay structurally aligned with what StackServerInterface.getEmailDeliveryInfo() and the /emails/delivery-info route actually return.

If camelCase is preferred for consumer ergonomics, renaming these fields now is lower risk than changing them later once external usage grows.

apps/backend/prisma/migrations/20251020183000_migrate_sent_email/migration.sql (1)

1-112: Migration mapping looks consistent but relies on a few important behavioral assumptions

The data migration from "SentEmail" into "EmailOutbox" is generally well thought out (preserving HTML/text/subject, mapping error JSON into the new error fields, and marking legacy rows as already rendered/sent), but a few details are worth explicitly validating against the new outbox/queue semantics and schema:

• Queue step behavior for migrated rows

  • All migrated rows are marked isQueued = TRUE and have startedSendingAt / finishedSendingAt populated, with skippedReason = NULL. This should guarantee that runEmailQueueStep never tries to re‑queue or re‑send these historical emails (especially ones with se."error" != NULL). Please confirm the queue selection query indeed excludes rows with isQueued = TRUE or non‑null finishedSendingAt so the LegacyEmail tsxSource is never evaluated.

• createdWith representation

  • createdWith is hard‑coded to 'PROGRAMMATIC_CALL'. In the new pipeline, fresh rows are written with { type: "programmatic-call", templateId: ... } objects at the TypeScript level. Make sure the underlying Prisma/DB type for "createdWith" accepts this legacy string variant and that any consumers handling it treat unknown/legacy shapes defensively so historical entries don’t break UIs or stats.

• to field JSON shape

  • to is populated as jsonb_build_object('type', 'custom-emails', 'emails', COALESCE(to_jsonb(se."to"), '[]'::jsonb)). This is correct if se."to" is already an array‑like value; if it’s a scalar address, this will yield a JSON string instead of a one‑element array. It’s worth confirming the old SentEmail."to" column type and, if necessary, normalizing to always store an array for "emails".

• Legacy delivery stats interpretation

  • renderedIsTransactional is set to TRUE and canHaveDeliveryInfo to FALSE for all migrated rows, with all delivery event timestamps left NULL. Make sure this matches how the new delivery‑stats code aggregates data so that legacy sends are either intentionally excluded or included in a controlled way.

None of these are blockers if the assumptions match the rest of the PR, but they’re subtle enough that I’d verify them against the queue‑step SQL and the Prisma schema before running this migration in production.

packages/template/src/lib/stack-app/apps/implementations/server-app-impl.ts (1)

30-30: Email delivery stats integration matches existing cache/hook patterns; consider refresh semantics

The new EmailDeliveryInfo surface is wired consistently with the rest of this class:

• _emailDeliveryInfoCache uses createCache + getEmailDeliveryInfo() and stores Result<EmailDeliveryInfo>.
• getEmailDeliveryStats() correctly unwraps with Result.orThrow(...).
• useEmailDeliveryStats() uses useAsyncCache on the same cache with an empty dependency tuple, matching other “global” hooks.

The only behavioral choice to be aware of is that this cache has no explicit invalidation or TTL; for a given _StackServerAppImplIncomplete instance, delivery stats will remain whatever they were on first fetch until you explicitly refresh or construct a new instance. If these stats are meant to update while a dashboard is open, you may eventually want:

• either a refresh path (e.g. method that calls _emailDeliveryInfoCache.refreshWhere(() => true)), or
• to avoid caching entirely here and rely on the underlying interface for any desired memoization.

For now the implementation is correct and consistent; this is mainly about expectations for how “live” the stats should be.

Also applies to: 138-141, 1302-1310

apps/backend/src/app/api/latest/emails/delivery-info/route.tsx (1)

1-65: Delivery-info route looks correct; consider DRY-ing the stats schema

The route wiring, auth requirements, and mapping from EmailDeliveryStats to the public stats/capacity shape all look consistent and type-safe. The windows array also keeps the handler logic maintainable.

If you end up adding/removing windows in the future, consider generating the stats schema from the same windows list (or a shared type) to avoid the small duplication between windows and the hard-coded hour/day/week/month shape in the yup response schema.

apps/backend/src/app/api/latest/internal/emails/crud.tsx (2)

8-27: Recipient mapping and error derivation are reasonable; consider future UX for user-primary-email

The new prismaModelToCrud looks consistent with the EmailOutbox shape: you correctly handle the three to variants and derive error from render vs send error messages, while keeping sent_at_millis aligned with finishedSendingAt ?? createdAt.

One thing to keep in mind: for type === "user-primary-email" you now surface "User ID: ${recipient.userId}" rather than an actual email address. If the existing admin UI or any tooling expects a literal email in to, this could be a slight UX regression.

If that becomes an issue, a possible follow-up would be to:

• Join to ContactChannel for the primary email in this case, or
• Extend the InternalEmailsCrud read type with an explicit recipient object, keeping to as a best-effort string list.

For now, this mapping is acceptable as long as consumers only treat to as display text rather than a strict email list.

---

38-46: Ordering by createdAt is simple; verify if you want sent-centric ordering instead

onList currently orders by createdAt descending, which is straightforward and likely mirrors the old SentEmail-based behavior.

Given the detailed ordering rules documented on EmailOutbox (finishedSendingAt, scheduledAtIfNotYetQueued, priority, id), consider whether this internal list should eventually use that same ordering (or at least finishedSendingAt when present) so that “recently sent” emails always float to the top, regardless of when they were created.

Not urgent, but worth confirming with how the admin UI consumes this list.

apps/backend/src/lib/email-rendering.tsx (1)

236-346: Consider de-duplicating batched render logic across helpers

renderEmailsForTenancyBatched is well-structured and correctly wires per-request template/theme modules and inputs, but it largely duplicates the bundling and Freestyle-execution logic from renderEmailsWithTemplateBatched (including nodeModules and error handling). Extracting a shared internal helper (e.g. executeFreestyleBundle(bundleText: string)) would reduce duplication and help keep behavior in sync if you need to tweak versions or error handling later.

apps/backend/src/app/api/latest/emails/send-email/route.tsx (2)

63-65: Redundant env var check – getEnvVariable already throws

getEnvVariable("STACK_FREESTYLE_API_KEY") will throw a StackAssertionError if the variable is missing, so the if (!getEnvVariable(...)) condition is effectively unreachable for the “not set” case. If you want a clean StatusError(500, ...) instead of a generic assertion error, consider explicitly catching that and re‑throwing as a StatusError; otherwise you can drop the if and just rely on getEnvVariable’s failure mode.

-    if (!getEnvVariable("STACK_FREESTYLE_API_KEY")) {
-      throw new StatusError(500, "STACK_FREESTYLE_API_KEY is not set");
-    }
+    // Will throw if STACK_FREESTYLE_API_KEY is missing or empty
+    getEnvVariable("STACK_FREESTYLE_API_KEY");

---

19-26: Subject and notification category fields are currently ignored

The new subject and notification_category_name fields are accepted in the body schema but not yet forwarded into the outbox or rendering. The TODO comment notes this, but just to highlight: clients may assume they have an effect already.

If you don’t plan to wire them in this PR, consider explicitly documenting them as “reserved/ignored for now” in the OpenAPI metadata to avoid confusion.

apps/backend/src/lib/email-delivery-stats.tsx (1)

38-96: Double‑check what counts as “sent” for penalty calculations

deliveryStatsQuery treats an outbox row as “sent” when:

• finishedSendingAt is within the window, and
• sendServerErrorInternalMessage IS NULL and skippedReason IS NULL.

However, rows with only external send errors (sendServerErrorExternalMessage / ...Details) will still be counted as “sent_last_*, even though they are also classified as SERVER_ERRORin the generatedstatus`.

If the capacity penalty is meant to react to actual delivery failures, you might want to exclude rows with any send server error from the sent counts (internal or external), or at least consider external errors too:

-- Example tweak inside each sent_last_* CASE
AND "sendServerErrorInternalMessage" IS NULL
AND "sendServerErrorExternalMessage" IS NULL
AND "skippedReason" IS NULL

Not necessarily a bug, but worth aligning with whatever semantics the UI and deliverability model are expecting from “sent”.

apps/backend/src/lib/email-queue-step.tsx (1)

475-479: Semantically incorrect skipped reason.

EmailOutboxSkippedReason.USER_DELETED_ACCOUNT is used in multiple scenarios:

• Line 477: when custom-emails recipient has an empty email list
• Line 484: when user is not found in the database
• Line 496: when resolved emails array is empty (user exists but has no email)

Only line 484 correctly represents an actual deleted account. The other cases could be:

• Empty custom email list: misconfiguration or API misuse
• User has no email: user data incomplete

Consider introducing more specific reasons like NO_EMAIL_ADDRESS or INVALID_RECIPIENT_CONFIG for better observability and debugging.

Also applies to: 483-485, 495-497

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: N2D4
Repo: stack-auth/stack-auth PR: 943
File: examples/convex/app/action/page.tsx:23-28
Timestamp: 2025-10-11T04:13:19.308Z
Learning: In the stack-auth codebase, use `runAsynchronouslyWithAlert` from `stackframe/stack-shared/dist/utils/promises` for async button click handlers and form submissions instead of manual try/catch blocks. This utility automatically handles errors and shows alerts to users.

apps/e2e/tests/backend/backend-helpers.ts (1)

397-407: Bound the polling loop to satisfy linter.

The for (let i = 0; true; i++) loop has an infinite condition that triggers the Biome linter error, even though it breaks/throws inside. Additionally, the HTML body check needs a type guard to prevent .includes() being called on undefined.

Apply this diff to bound the loop and add defensive checks:

-      for (let i = 0; true; i++) {
-        const messages = await mailbox.fetchMessages();
-        const containsSubstring = messages.some(message => message.subject.includes("Sign in to") && message.body?.html.includes(response.body.nonce));
-        if (containsSubstring) {
-          break;
-        }
-        await wait(200);
-        if (i >= 30) {
-          throw new StackAssertionError(`Sign-in code message not found after ${i} attempts`, { messages });
-        }
-      }
+      let lastMessages: Awaited<ReturnType<Mailbox["fetchMessages"]>> = [];
+      for (let i = 0; i < 30; i++) {
+        lastMessages = await mailbox.fetchMessages();
+        const containsSubstring = lastMessages.some(
+          (message) =>
+            message.subject.includes("Sign in to") &&
+            typeof message.body?.html === "string" &&
+            message.body.html.includes(response.body.nonce),
+        );
+        if (containsSubstring) {
+          break;
+        }
+        await wait(200);
+      }
+      if (!lastMessages.some(
+        (message) =>
+          message.subject.includes("Sign in to") &&
+          typeof message.body?.html === "string" &&
+          message.body.html.includes(response.body.nonce),
+      )) {
+        throw new StackAssertionError("Sign-in code message not found after 30 attempts", { messages: lastMessages });
+      }

apps/e2e/tests/backend/backend-helpers.ts (2)

422-428: Add defensive type guard for HTML body filtering.

The getSignInCodeFromMailbox function filters messages by nonce but doesn't guard against message.body?.html being undefined or non-string before calling .includes().

Apply this diff to add a type guard:

 export async function getSignInCodeFromMailbox(nonce?: string) {
   const mailbox = backendContext.value.mailbox;
   const messages = await mailbox.fetchMessages();
-  const message = messages.filter(message => nonce === undefined || message.body?.html.includes(nonce)).findLast((message) => message.subject.includes("Sign in to")) ?? throwErr("Sign-in code message not found");
+  const message = messages
+    .filter(message =>
+      nonce === undefined ||
+      (typeof message.body?.html === "string" && message.body.html.includes(nonce))
+    )
+    .findLast((message) => message.subject.includes("Sign in to")) ?? throwErr("Sign-in code message not found");
   const signInCode = message.body?.text.match(/http:\/\/localhost:12345\/some-callback-url\?code=([a-zA-Z0-9]+)/)?.[1] ?? throwErr("Sign-in URL not found");
   return signInCode;
 }

---

996-1008: Inconsistent assertion pattern.

The ProjectApiKey.User.create function changed from using toMatchInlineSnapshot to toMatchObject. While both work, this creates inconsistency with other helper functions in this file that use inline snapshots (e.g., Team.create at line 1233).

For consistency, consider either:

1. Keeping the inline snapshot pattern used elsewhere in this file, or
2. Documenting why toMatchObject is preferred here (e.g., if the response varies in ways that break snapshots)

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

apps/backend/prisma/schema.prisma (1)

758-759: Fix isPaused comment to match actual field semantics.

The comment claims this column is auto-generated and cannot be set manually, but the field definition (Boolean @default(false)) shows it's a plain mutable boolean flag. This contradicts other computed fields in the model (e.g., status, simpleStatus, priority) which explicitly use @default(dbgenerated("<manually set>")).

Since isPaused should be application-managed (e.g., to pause outbound email sending), the comment is incorrect and misleading. Update it to reflect that isPaused is a mutable, application-managed flag.

- // This column is auto-generated as defined in the SQL migration. It can not be set manually.
+ // Mutable flag managed by application code to pause/resume email sending. Can be set at any time.
  isPaused Boolean @default(false)

docker/dependencies/docker.compose.yaml (1)

241-245: [Duplicate] Document tmpfs volume implications.

The postgres-data volume uses tmpfs, which means all database state is lost when the container restarts. This is appropriate for development but should be clearly documented to prevent developer confusion when restarting Docker and losing test data.

apps/e2e/tests/backend/backend-helpers.ts (2)

423-429: Add type guard to prevent runtime error.

Line 426 calls .includes() on message.body?.html without verifying it's a string. This can throw a runtime error if html is undefined or not a string.

Apply this diff:

-      const message = messages.filter(message => nonce === undefined || message.body?.html.includes(nonce)).findLast((message) => message.subject.includes("Sign in to")) ?? throwErr("Sign-in code message not found");
+      const message = messages
+        .filter(message =>
+          nonce === undefined ||
+          (typeof message.body?.html === "string" && message.body.html.includes(nonce))
+        )
+        .findLast((message) => message.subject.includes("Sign in to")) ?? throwErr("Sign-in code message not found");

Based on past review comments.

---

398-408: Address the linter error and add defensive type guards.

This code still has the issues flagged in the previous review:

1. Infinite loop condition: Line 398 uses for (let i = 0; true; i++) which Biome flags as a constant condition. Bound the loop explicitly with for (let i = 0; i < 31; i++) to satisfy the linter.

2. Missing type guard: Line 400 calls .includes() on message.body?.html without verifying it's a string. If html is undefined or not a string, this will throw a runtime error.

Apply this diff to fix both issues:

-      for (let i = 0; true; i++) {
+      let lastMessages: Awaited<ReturnType<typeof mailbox.fetchMessages>> = [];
+      for (let i = 0; i < 31; i++) {
-        const messages = await mailbox.fetchMessages();
+        lastMessages = await mailbox.fetchMessages();
-        const containsSubstring = messages.some(message => message.subject.includes("Sign in to") && message.body?.html.includes(response.body.nonce));
+        const containsSubstring = lastMessages.some(
+          (message) =>
+            message.subject.includes("Sign in to") &&
+            typeof message.body?.html === "string" &&
+            message.body.html.includes(response.body.nonce)
+        );
         if (containsSubstring) {
           break;
         }
         await wait(200);
-        if (i >= 30) {
-          throw new StackAssertionError(`Sign-in code message not found after ${i} attempts`, { messages });
-        }
       }
+      if (!lastMessages.some(
+        (message) =>
+          message.subject.includes("Sign in to") &&
+          typeof message.body?.html === "string" &&
+          message.body.html.includes(response.body.nonce)
+      )) {
+        throw new StackAssertionError("Sign-in code message not found after 30 attempts", { messages: lastMessages });
+      }

Based on past review comments.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

apps/backend/scripts/generate-openapi-fumadocs.ts (1)

82-86: Consider simplifying the error handler parameter.

The error handler correctly ensures the script exits with a failure code. However, .catch() receives a single error argument, so the ...args spread pattern is unconventional. Consider simplifying for clarity:

-main().catch((...args) => {
-  console.error(`ERROR! Could not update Fumadocs OpenAPI schema`, ...args);
+main().catch((error) => {
+  console.error(`ERROR! Could not update Fumadocs OpenAPI schema`, error);
   process.exit(1);
 });

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

apps/backend/src/app/api/latest/internal/email-queue-step/route.tsx (1)

35-40: Long‑running 2‑minute loop is risky for serverless/cron execution

This loop can keep the route handler alive for up to 2 minutes, calling runEmailQueueStep every second. On typical serverless platforms (including Vercel route handlers/cron), this is close to or beyond common max execution times and can lead to the function being killed mid‑processing and overlapping invocations from the cron schedule.

Consider instead:

• Having this endpoint invoke runEmailQueueStep() once per request and relying on your scheduler/queue (cron/QStash) for repeated triggering, or
• At least reducing the loop window substantially and/or enforcing a hard cap on invocations per request.

Example local change if you opt for single‑shot execution:

-    const startTime = performance.now();
-
-    while (performance.now() - startTime < 2 * 60 * 1000) {
-      await runEmailQueueStep();
-      await wait(1000);
-    }
+    await runEmailQueueStep();

apps/backend/src/app/api/latest/internal/email-queue-step/route.tsx (1)

15-21: Request schema includes unused auth field

You’re not using auth in the handler anymore, and the endpoint is authenticated purely via the Authorization header. Keeping auth: yupObject({}).nullable().optional() here is slightly confusing; consider dropping it from the schema to better reflect how this endpoint is actually secured.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

apps/backend/src/prisma-client.tsx (1)

119-119: Remove unused variable.

The now variable is assigned but never used.

Apply this diff:

       const [, password, portPrefix, schema] = match;
       const orbStackDomain = `db.stack-dependencies-${portPrefix}.orb.local`;
-      const now = performance.now();
       const ok = await tcpPing(orbStackDomain, 5432, 50);  // extremely short timeout; OrbStack should be fast to respond, otherwise why are we doing this?

apps/backend/src/lib/email-queue-step.tsx (3)

427-445: Set canHaveDeliveryInfo: true for successful sends.

Line 438 sets canHaveDeliveryInfo: false even when the email sends successfully. If your email provider supports delivery tracking (bounces, opens, etc.), this field should be true for successful sends to enable downstream delivery status updates. The error case at line 420 correctly uses false.

Apply this diff:

     } else {
       await globalPrismaClient.emailOutbox.update({
         where: {
           tenancyId_id: {
             tenancyId: row.tenancyId,
             id: row.id,
           },
           finishedSendingAt: null,
         },
         data: {
           finishedSendingAt: new Date(),
-          canHaveDeliveryInfo: false,
+          canHaveDeliveryInfo: true,
           sendServerErrorExternalMessage: null,
           sendServerErrorExternalDetails: Prisma.DbNull,
           sendServerErrorInternalMessage: null,
           sendServerErrorInternalDetails: Prisma.DbNull,
         },
       });
     }

---

374-379: Fix the isPrimary type check.

Line 377 compares isPrimary === "TRUE" (string comparison), but based on the Prisma schema and usage elsewhere in the codebase, isPrimary is a boolean field. This bug causes getPrimaryEmail() to always return undefined because the condition never matches. The eslint-disable comment on line 376 hints at this type mismatch.

Apply this diff:

 function getPrimaryEmail(user: ProjectUserWithContacts | undefined): string | undefined {
   if (!user) return undefined;
-  // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
-  const primaryChannel = user.contactChannels.find((channel) => channel.type === "EMAIL" && channel.isPrimary === "TRUE");
+  const primaryChannel = user.contactChannels.find((channel) => channel.type === "EMAIL" && channel.isPrimary === true);
   return primaryChannel?.value ?? undefined;
 }

---

237-241: Fix the case sensitivity bug and implement proper logic for determining if notification is transactional.

Line 241 has a critical bug: it compares output.notificationCategory (which is a UUID ID) against the lowercase string "transactional", which will always be false. The actual category name is "Transactional" (capitalized) and must be retrieved via getNotificationCategoryById().

Replace:

-        renderedIsTransactional: output.notificationCategory === "transactional",  // TODO this should use smarter logic for notification category handling
+        renderedIsTransactional: getNotificationCategoryById(output.notificationCategory)?.name === "Transactional" ?? false,

apps/backend/src/app/api/latest/internal/email-queue-step/route.tsx (1)

40-46: Revisit the 2-minute execution loop.

Multiple reviewers flagged this loop as problematic for serverless environments (lines 40-46 in past comments). If Vercel Cron is invoking this endpoint regularly, the internal loop may be unnecessary—simply execute runEmailQueueStep() once and exit. The only_one_step parameter provides a testing escape hatch but doesn't address production concerns about exceeding typical serverless timeout limits.

Consider removing the loop and relying on more frequent cron triggers instead:

-    const startTime = performance.now();
-
-    while (performance.now() - startTime < 2 * 60 * 1000) {
-      await runEmailQueueStep();
-      await wait(1000);
-      if (query.only_one_step === "true") {
-        break;
-      }
-    }
+    await runEmailQueueStep();

Alternatively, reduce the loop duration to stay well within timeout limits (e.g., 30-50 seconds) if extended processing per invocation is essential.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

apps/backend/src/app/api/latest/emails/send-email/route.tsx (1)

19-20: Unused parameters flagged in previous review.

The subject and notification_category_name parameters remain unused, as noted in the previous review comments. This continues to be confusing for API consumers who may expect these to work.

apps/backend/src/lib/email-queue-step.tsx (7)

236-248: Verify case sensitivity in notification category matching.

Line 236 matches category.name === output.notificationCategory, where categories are defined with name "Transactional" (capital T). If templates return notificationCategory: "transactional" (lowercase), the lookup will fail and renderedIsTransactional will always be false, breaking priority calculations.

Run this script to check how templates define notification categories:

#!/bin/bash
# Check notification category casing in templates
rg -n "notificationCategory" apps/backend/src --type tsx -C2

---

365-370: Verify isPrimary type handling.

The code compares channel.isPrimary === "TRUE" (line 368), treating it as an enum string. However, the Prisma schema defines isPrimary BooleanTrue? where BooleanTrue is an enum with a single value TRUE. At runtime, Prisma enums are represented as strings, so the comparison is technically correct. However, verify this is consistent with how isPrimary is handled elsewhere in the codebase.

Run this script to check other uses of isPrimary:

#!/bin/bash
# Check how isPrimary is used throughout the codebase
rg -n "isPrimary" apps/backend/src --type ts --type tsx -C2

---

538-563: Document or remove unused serializeRecipient export.

The serializeRecipient function is exported but has no visible callers in this file. If it's part of a public API for external use, add a JSDoc comment explaining its purpose. Otherwise, consider removing the export.

---

72-72: Critical: metadata key mismatch breaks rate limiting.

The key "EMAIL_QUEUE_METADATA_KEY" doesn't match the migration, which inserts a row with key 'email-queue-step'. The query will never find the existing metadata, causing delta to always be 0 and breaking send-rate calculations.

Apply this fix:

-  const key = "EMAIL_QUEUE_METADATA_KEY";
+  const key = "email-queue-step";

---

181-191: Major: unsubscribe links not generated during initial rendering.

During the rendering phase, row.renderedNotificationCategoryId is NULL (it's only set at line 247 after rendering completes). This means unsubscribe links will never be generated during the initial render. The notification category is determined from the template output, creating a circular dependency.

Consider generating unsubscribe links in a second pass after rendering completes, or require templates to declare their notification category upfront.

---

402-417: Add race condition check to error update.

The error case update (lines 402-417) lacks a finishedSendingAt: null check in the WHERE clause, unlike the success case (line 425) and exception handler (line 445). This inconsistency could allow overwriting a row that was already marked as finished by another process.

Apply this fix:

     if (result.status === "error") {
       await globalPrismaClient.emailOutbox.update({
         where: {
           tenancyId_id: {
             tenancyId: row.tenancyId,
             id: row.id,
           },
+          finishedSendingAt: null,
         },

---

402-456: canHaveDeliveryInfo is hardcoded to false.

Lines 411, 429, and 449 all set canHaveDeliveryInfo: false regardless of the email provider. According to the schema comments, this should be true for providers like Resend that support delivery tracking. Consider determining this value based on context.emailConfig.type or a provider capability flag.

apps/backend/src/app/api/latest/emails/send-email/route.tsx (1)

11-14: Remove unused user_email field from type definition.

The user_email field is defined in the UserResult type but is never populated or returned in the response (lines 149-151 only return user_id). The response schema (lines 57-59) also only declares user_id.

Apply this diff to simplify the type:

 type UserResult = {
   user_id: string,
-  user_email?: string,
 };

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro