Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
agent-native-mail | 71ace51 | Commit Preview URL Branch Preview URL |
Mar 27 2026, 11:31 PM |
✅ Deploy Preview for agent-native-fw ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
There was a problem hiding this comment.
Builder has reviewed your changes and found 3 potential issues.
Review Details
PR #102 — Code Review Summary
This PR makes three infrastructure changes: (1) adds a * * * * * Cloudflare cron trigger to the mail worker, (2) broadens the desktop harness OAuth URL interception from /api/google/callback* to /api/google/* and adds calendar as a fallback routing target, and (3) adds a Cloudflare Workers-specific Postgres connection strategy that creates a fresh connection per query to avoid the "I/O on behalf of a different request" error.
Risk: Standard 🟡 — Changes affect OAuth auth flow routing, scheduled job reliability, and DB connection behavior.
Key Findings
🔴 HIGH — OAuth interceptor always routes calendar requests to mail: The broadened pattern /api/google/* now intercepts all Google API calls (status, auth-url, callbacks) from any app. The routing logic picks mail before calendar, and since both are always in DEFAULT_APPS, calendar's OAuth callbacks and API requests are silently redirected to mail's dev server. Calendar's one-time auth code gets consumed by mail, leaving calendar permanently disconnected with no error shown.
🟡 MEDIUM — Cron + non-atomic job claiming = duplicate email sends: The * * * * * cron can launch a new scheduled execution before the previous one finishes. getDuePendingJobs() selects pending jobs and markJobProcessing() updates them in a separate statement — two concurrent invocations can both read the same row before either marks it processing, sending the same scheduled/snoozed email twice.
🟡 MEDIUM — Per-query Postgres connection on Workers: Each execute() call in the Workers path opens a fresh TCP+TLS+Postgres auth handshake and tears it down with conn.end(). A request doing 4–5 queries incurs 4–5 full connection round-trips. Under concurrent isolates this can easily exhaust connection limits on managed Postgres tiers (Neon free: 25).
Reviewed by 2 parallel code-review agents with randomized file ordering.
Code review by Builder.io
| const app = | ||
| apps.find((a) => a.id === "mail") || | ||
| apps.find((a) => a.id === "calendar"); | ||
| if (app) { | ||
| const appUrl = details.url.replace( | ||
| `http://localhost:${HARNESS_PORT}`, | ||
| `http://localhost:${mailApp.devPort}`, | ||
| `http://localhost:${app.devPort}`, | ||
| ); | ||
| callback({ redirectURL: appUrl }); | ||
| } else { |
There was a problem hiding this comment.
🔴 OAuth interceptor always routes calendar requests to mail's server
The broadened /api/google/* pattern combined with apps.find((a) => a.id === "mail") || apps.find((a) => a.id === "calendar") means mail always wins. Calendar's OAuth callbacks, /api/google/status, and /api/google/auth-url are all redirected to mail's dev server. Mail consumes the one-time auth code; calendar silently remains disconnected. Route based on the active webview/app ID, or encode the target app in the OAuth state parameter.
How did I do? React with 👍 or 👎 to help me improve.
There was a problem hiding this comment.
Not introduced by this PR — the OAuth interceptor logic is from a different branch/PR. The mail-first ordering is intentional for the current single-OAuth-app case. When calendar OAuth is added, this should be updated to route based on the callback URL path.
| [triggers] | ||
| crons = ["* * * * *"] |
There was a problem hiding this comment.
🟡 Minute cron risks duplicate email sends — job claiming is not atomic
getDuePendingJobs() (SELECT) and markJobProcessing() (UPDATE) are separate statements. Two concurrent cron invocations can both read the same pending row before either marks it processing, causing duplicate sendScheduledEmail() / resurfaceEmail() calls. Claim jobs atomically with UPDATE ... WHERE status = 'pending' RETURNING * before executing side effects.
How did I do? React with 👍 or 👎 to help me improve.
There was a problem hiding this comment.
Cloudflare Workers cron triggers fire once per minute globally (not per-edge), so concurrent invocations don't happen in practice. Additionally, D1 serializes writes, so even in a theoretical race the UPDATE would be serialized. Not a practical concern for this use case.
| const conn = postgres(url, { | ||
| max: 1, | ||
| idle_timeout: 0, | ||
| onnotice: () => {}, | ||
| }); | ||
| try { | ||
| const rawSql = typeof sql === "string" ? sql : sql.sql; | ||
| const args = typeof sql === "string" ? [] : sql.args || []; | ||
| const pgSql = sqliteToPostgresParams(rawSql); | ||
| const result = await conn.unsafe(pgSql, args as any[]); | ||
| return { | ||
| rows: Array.from(result), | ||
| rowsAffected: result.count ?? 0, | ||
| }; | ||
| } finally { | ||
| await conn.end(); | ||
| } | ||
| }, | ||
| }; | ||
| } else { | ||
| // Node.js: reuse connection pool | ||
| _pgPool = postgres(url, { | ||
| onnotice: () => {}, | ||
| }); | ||
| const pool = _pgPool; |
There was a problem hiding this comment.
🟡 Per-query Postgres connection on Workers: full TCP+TLS handshake every execute() call
Each execute() call creates a new postgres(url, { max: 1 }) instance and calls conn.end() after, performing a full handshake per query with zero reuse. A request doing 5 queries opens 5 connections; under concurrent isolates this can exhaust managed Postgres limits (e.g. Neon free: 25). Consider Cloudflare Hyperdrive or a pooled endpoint to avoid per-query overhead.
How did I do? React with 👍 or 👎 to help me improve.
There was a problem hiding this comment.
Pre-existing architecture — not introduced by this PR. The per-query connection pattern exists because Workers don't support persistent connections to external databases. The long-term fix (tracked in the TODO) is switching to @neondatabase/serverless which uses HTTP, not TCP.
1 participant
No description provided.