Conversation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/backend/src/auto-migrations/index.tsx (1)
104-196: Avoid sleeping while holding the advisory xact lock (and a DB connection)
await wait(500)runs inside the Prisma transaction, so it holdspg_advisory_xact_lock(...)and the transaction connection for the extra 500ms, which can amplify contention/timeouts when multiple workers race migrations. Prefer committing first, then sleeping before the next retry.await options.prismaClient.$transaction(async (tx) => { + let requestedRepeatDelayMs: number | null = null; log(` |> Preparing...`); await tx.$executeRaw` SELECT pg_advisory_xact_lock(${MIGRATION_LOCK_ID}); `; @@ if (res[0].should_repeat_migration) { log(` |> Migration ${migration.migrationName} requested to be repeated. This is normal and *not* indicative of a problem.`); - await wait(500); // give the database a chance to catch up with everything else that's happening + requestedRepeatDelayMs = 500; // delay after commit to avoid holding locks/connections // Commit the transaction and continue re-running the migration return; } @@ newlyAppliedMigrationNames.push(migration.migrationName); shouldRepeat = false; - }, { + if (requestedRepeatDelayMs != null) { + // keep shouldRepeat=true, but sleep outside of the transaction + } + }, { timeout: 80_000, maxWait: 30_000, }); + // If we returned early to repeat, wait here after commit/connection release. + if (shouldRepeat) { + await wait(500); + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/backend/prisma/migrations/20251020183000_migrate_sent_email/migration.sql(1 hunks)apps/backend/src/auto-migrations/index.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/backend/prisma/migrations/20251020183000_migrate_sent_email/migration.sql
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Always add new E2E tests when changing the API or SDK interface
For blocking alerts and errors, never use toast; use alerts instead as they are less easily missed by the user
NEVER try-catch-all, NEVER void a promise, and NEVER .catch(console.error); use loading indicators and async callbacks instead, or use runAsynchronously/runAsynchronouslyWithAlert for error handling
Use ES6 maps instead of records wherever you can
Files:
apps/backend/src/auto-migrations/index.tsx
**/*.{ts,tsx,css}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,css}: Keep hover/click transitions snappy and fast; avoid fade-in delays on hover. Apply transitions after action completion instead, like smooth fade-out when hover ends
Use hover-exit transitions instead of hover-enter transitions; for example, use 'transition-colors hover:transition-none' instead of fade-in on hover
Files:
apps/backend/src/auto-migrations/index.tsx
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
NEVER use Next.js dynamic functions if you can avoid them; prefer using client components and hooks like usePathname instead of await params to keep pages static
Files:
apps/backend/src/auto-migrations/index.tsx
{.env*,**/*.{ts,tsx,js}}
📄 CodeRabbit inference engine (AGENTS.md)
Prefix environment variables with STACK_ (or NEXT_PUBLIC_STACK_ if public) so changes are picked up by Turborepo and improves readability
Files:
apps/backend/src/auto-migrations/index.tsx
apps/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Check existing apps for inspiration when implementing new apps or pages; update apps-frontend.tsx and apps-config.ts to add new apps
Files:
apps/backend/src/auto-migrations/index.tsx
🧠 Learnings (1)
📚 Learning: 2025-12-04T18:03:49.984Z
Learnt from: CR
Repo: stack-auth/stack-auth PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T18:03:49.984Z
Learning: Applies to packages/stack-shared/src/config/schema.ts : Whenever making backwards-incompatible changes to the config schema, update the migration functions in packages/stack-shared/src/config/schema.ts
Applied to files:
apps/backend/src/auto-migrations/index.tsx
🧬 Code graph analysis (1)
apps/backend/src/auto-migrations/index.tsx (1)
packages/stack-shared/src/utils/promises.tsx (1)
wait(260-268)
⏰ 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). (12)
- GitHub Check: Vercel Agent Review
- GitHub Check: Cursor Bugbot
- GitHub Check: check_prisma_migrations (22.x)
- GitHub Check: docker
- GitHub Check: build (22.x)
- GitHub Check: restart-dev-and-test-with-custom-base-port
- GitHub Check: build (22.x)
- GitHub Check: restart-dev-and-test
- GitHub Check: lint_and_build (latest)
- GitHub Check: setup-tests
- GitHub Check: build (22.x)
- GitHub Check: all-good
🔇 Additional comments (1)
apps/backend/src/auto-migrations/index.tsx (1)
4-4:waitimport path/version stabilityImporting from
@stackframe/stack-shared/dist/...is fine if that path is treated as public API; otherwise it’s easy to break on package/build changes. Consider importing from a stable/public entrypoint if one exists.
Older cmux preview screenshots (latest comment is below)Preview Screenshots⏳ Preview screenshots are being captured... Workspace and dev browser links will appear here once the preview environment is ready. Generated by cmux preview system |
Older cmux preview screenshots (latest comment is below)Preview Screenshots⏳ Preview screenshots are being captured... Workspace and dev browser links will appear here once the preview environment is ready. Generated by cmux preview system |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/backend/prisma/migrations/20251212183000_migrate_sent_email/migration.sql (1)
4-15: Optional: make batching deterministic to reduce long-tail/dup work
Consider adding anORDER BY se."id"(or createdAt/id) into_migrateto avoid non-deterministic LIMIT selection across runs.apps/backend/prisma/migrations/20251212180000_email_outbox/migration.sql (2)
92-95: Priority ordering vs indexes:priority ASClikely fights “high priority first”
priorityassigns 100 for high-priority (Line 93), butEmailOutbox_sending_idxis(tenancyId, priority, scheduledAt)(Line 248) andEmailOutbox_ordering_idxstorespriority ASC(Line 256). If the processor selects “highest priority first”, these indexes won’t support that ordering well (and may encourage lowest-priority-first scans).Also applies to: 248-258
251-257: Double-checkscheduledAtIfNotYetQueued DESCaligns with “send oldest first”
scheduledAtIfNotYetQueuedisscheduledAtwhen not queued (Line 109-111), butordering_idxusesscheduledAtIfNotYetQueued DESC(Line 255). If you intend FIFO by scheduled time, this likely wantsASC(or you need matching query ORDER BY).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/backend/prisma/migrations/20251212180000_email_outbox/migration.sql(1 hunks)apps/backend/prisma/migrations/20251212183000_migrate_sent_email/migration.sql(1 hunks)apps/backend/prisma/migrations/20251212185000_add_no_email_provided_skip_reason/migration.sql(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/backend/prisma/migrations/20251212185000_add_no_email_provided_skip_reason/migration.sql
⏰ 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). (12)
- GitHub Check: build (22.x)
- GitHub Check: lint_and_build (latest)
- GitHub Check: build (22.x)
- GitHub Check: build (22.x)
- GitHub Check: restart-dev-and-test-with-custom-base-port
- GitHub Check: restart-dev-and-test
- GitHub Check: check_prisma_migrations (22.x)
- GitHub Check: docker
- GitHub Check: setup-tests
- GitHub Check: all-good
- GitHub Check: Vercel Agent Review
- GitHub Check: Cursor Bugbot
🔇 Additional comments (2)
apps/backend/prisma/migrations/20251212183000_migrate_sent_email/migration.sql (2)
1-3: The migration runner properly gates statements 2/3 (INSERT metadata and DROP TABLE) on statement 1 completion. When statement 1 returnsshould_repeat_migration = true, line 155 ofindex.tsxreturns from the transaction callback, preventing execution of remaining statements in that iteration. Statements 2/3 only execute aftershould_repeat_migration = false. TheSPLIT_STATEMENT_SENTINELandCONDITIONALLY_REPEAT_MIGRATION_SENTINELare control-flow markers (not just SQL comments) that the runner uses to gate conditional logic—the outer loop (for (let repeat = 0; shouldRepeat; repeat++)at line 105) re-runs the migration until completion. This design eliminates the risk of prematureDROP TABLEexecution.Likely an incorrect or invalid review comment.
67-76: The legacy recipient mapping is correct and safe.SentEmail.tois defined asTEXT[](PostgreSQL text array of email address strings), andto_jsonb(se."to")correctly converts it to a JSONB array of strings, which matches the downstreamEmailOutbox.tocontract expecting{ type: "custom-emails", emails: string[] }. The concern about potential {email,name} object mismatches is invalid since the source data structure is always a string array, not objects with multiple properties.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Bug: Unused fetch could throw if email env vars missing
The variables internalTenancy and emailConfig are fetched unconditionally but never used because the email sending functionality is disabled (line 72 throws immediately). The getSharedEmailConfig call invokes getEnvVariable for email configuration variables (STACK_EMAIL_HOST, etc.) without defaults, which will throw an error if those environment variables aren't set. This causes the endpoint to fail with a confusing error about missing email config even though email sending is disabled. These fetches should be removed or moved inside the conditional block.
apps/backend/src/app/api/latest/internal/failed-emails-digest/route.ts#L46-L48
Older cmux preview screenshots (latest comment is below)Preview Screenshots⏳ Preview screenshots are being captured... Workspace and dev browser links will appear here once the preview environment is ready. Generated by cmux preview system |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/backend/prisma/migrations/20251212183000_migrate_sent_email/migration.sql (2)
7-9: DroppingSentEmailwill discard “orphaned tenancy” rows (if any) without migrating them.
TheWHERE EXISTS (Tenancy…)filter intentionally skips anySentEmailwhosetenancyIdno longer exists, butDROP TABLE … "SentEmail"will still remove them. If that’s intended cleanup, consider making it explicit (e.g., pre-delete with a comment / sanity-count), or gate the DROP on “no remaining rows”.Also applies to: 140-140
14-15: Add a deterministic ORDER BY to the 10k batching selection.
LIMIT 10000withoutORDER BYmakes each batch nondeterministic, which can complicate debugging/rollbacks. Deterministic ordering is cheap here and keeps re-runs predictable.WITH to_migrate AS ( SELECT se."tenancyId", se."id" FROM "SentEmail" se @@ AND NOT EXISTS ( SELECT 1 FROM "EmailOutbox" eo WHERE eo."tenancyId" = se."tenancyId" AND eo."id" = se."id" ) + ORDER BY se."createdAt" ASC, se."tenancyId" ASC, se."id" ASC LIMIT 10000 ),Also applies to: 128-133
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/backend/prisma/migrations/20251212183000_migrate_sent_email/migration.sql(1 hunks)
⏰ 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). (12)
- GitHub Check: Vercel Agent Review
- GitHub Check: Cursor Bugbot
- GitHub Check: build (22.x)
- GitHub Check: lint_and_build (latest)
- GitHub Check: all-good
- GitHub Check: build (22.x)
- GitHub Check: restart-dev-and-test
- GitHub Check: restart-dev-and-test-with-custom-base-port
- GitHub Check: docker
- GitHub Check: setup-tests
- GitHub Check: check_prisma_migrations (22.x)
- GitHub Check: build (22.x)
🔇 Additional comments (2)
apps/backend/prisma/migrations/20251212183000_migrate_sent_email/migration.sql (2)
1-3: Sentinel placement is correct — the runner splits bySPLIT_STATEMENT_SENTINEL, so the first statement (lines 1–133) withCONDITIONALLY_REPEAT_MIGRATION_SENTINELrepeats in full, and the second statement (lines 136–140) without the sentinel executes once. The DROP at line 140 is safely outside the repeat block and will not execute after the first 10k batch.
88-88: No action needed. PostgreSQL 16.1 includesgen_random_uuid()as a built-in function (available since PostgreSQL 13), so pgcrypto extension is not required. This usage is consistent with the project's Prisma schema, which already uses the same function without extension setup.Likely an incorrect or invalid review comment.
Preview Screenshots⏳ Preview screenshots are being captured... Workspace and dev browser links will appear here once the preview environment is ready. Generated by cmux preview system |
I'm sorry for whoever reviews this. I tried to split it up as much as possible so this is only the existing functionality migrated onto the new email outbox table; the new endpoints or UI changes are not here yet.
Tests not passing yet but already ready for reviews.
How to review:
The prompt that generated the first version (but note that the final version is somewhat different from what was generated, but it might help get some overview):
Note
Migrates email to an async outbox pipeline with queueing/rendering, delivery stats, updated APIs, and extensive tests.
EmailOutboxschema with generated columns, constraints, and indices; migrate data fromSentEmailand drop it.sendEmailToMany, batch render (Freestyle), queue, and send (low-level SMTP/provider) with robust error handling and skip reasons.runEmailQueueStepand internal endpoint/api/latest/internal/email-queue-stepwith locking, delta-based capacity, and stochastic quotas.POST /api/latest/emails/send-email(per-user enqueue, variables JSON, priority/scheduling, notification category validation).GET /api/latest/emails/delivery-info(hour/day/week/month stats and capacity).getEmailDeliveryStatsanduseEmailDeliveryStats; server interface method added.Written by Cursor Bugbot for commit 313c229. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.