fix: DATABASE_URL takes priority over D1 binding#100
Conversation
When both DATABASE_URL and a Cloudflare D1 binding exist, DATABASE_URL now wins. This ensures Neon/Supabase/Turso is used when configured, even if a D1 database is still attached to the Worker. Applied consistently across getDialect(), createGetDb(), and runMigrations().
✅ Deploy Preview for agent-native-fw ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
agent-native-mail | c63189d | Commit Preview URL Branch Preview URL |
Mar 27 2026, 08:33 PM |
![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 2 potential issues.
Review Details
PR #100 — DATABASE_URL takes priority over D1 binding
This PR correctly fixes the dialect priority in getDialect(), createGetDb(), and runMigrations() so that DATABASE_URL (Neon/Supabase/Turso) wins over a Cloudflare D1 binding when both exist. The reordering logic is sound and URL format coverage is complete (postgres://, postgresql://, libsql://, https:// all handled correctly). The migration consistency is also correct — runMigrations() now delegates to getDialect() rather than duplicating detection logic.
Risk: High — the PR also includes an auth fix (auth.ts) that affects session creation.
Key Findings
🟡 MEDIUM — Auth sentinel email may affect OAuth token ownership downstream
The fix passes "user" as the email argument to addSession(), resolving the login regression from PR #99 (where getSessionEmail() returning null caused an infinite redirect). However, "user" is stored as the literal session.email, which propagates to OAuth token ownership in the mail and calendar templates. This is self-consistent for fresh installs but diverges from Google-OAuth-authenticated users (whose tokens are stored under their real email).
🟡 MEDIUM — D1 binding absence after dialect detection has no fast-fail
If getDialect() returns "d1" but __cf_env?.DB is somehow null in createGetDb(), the code falls through silently to the LibSQL path — which will fail confusingly in a Cloudflare environment (no filesystem). A throw would make this fail fast with a clear message.
Found via 2 parallel code-review agent calls with randomized file ordering.
Code review by Builder.io
| } | ||
| const sessionToken = crypto.randomBytes(32).toString("hex"); | ||
| await addSession(sessionToken); | ||
| await addSession(sessionToken, "user"); |
There was a problem hiding this comment.
🟡 "user" stored as session email — affects OAuth token ownership
The second parameter of addSession is email, not a role. Storing "user" fixes the login regression (null email → infinite redirect) but means session.email === "user" for all token-auth sessions. In the mail/calendar templates, OAuth tokens are stored with owner = session.email, so Google accounts added via this flow are keyed to "user" instead of a real address — breaking lookups for users who previously authenticated via Google OAuth login. Consider a more stable sentinel like "token-auth@localhost", or fix getSession() to use the existing hasSession() fallback instead.
How did I do? React with 👍 or 👎 to help me improve.
There was a problem hiding this comment.
ACCESS_TOKEN login is only used for simple token-based access without Google. When Google OAuth is later connected, the callback creates a new session with the real email — so token ownership is correct for the Google flow. The "user" sentinel only applies to the initial non-OAuth login. Not a real issue in practice.
| if (d1) { | ||
| _db = drizzleD1(d1, { schema }) as unknown as LibSQLDatabase<T>; | ||
| _dbReady = Promise.resolve(_db); | ||
| return _dbReady; |
There was a problem hiding this comment.
🟡 No fail-fast if D1 dialect detected but binding is absent
If getDialect() returns "d1" but __cf_env?.DB is null here, the code silently falls through to the LibSQL/SQLite path — which will crash in a Cloudflare Workers environment (no filesystem). Adding else { throw new Error('D1 dialect selected but __cf_env.DB binding is missing') } would give a clear, actionable error instead of a confusing downstream failure.
How did I do? React with 👍 or 👎 to help me improve.
There was a problem hiding this comment.
getDialect() only returns d1 when __cf_env.DB exists, so this cant happen in practice. Not worth the added code.
1 participant
Summary
When both
DATABASE_URLand a Cloudflare D1 binding exist,DATABASE_URLnow wins. This ensures Neon/Supabase/Turso is used when configured, even if a legacy D1 database is still attached to the Worker.Applied consistently across
getDialect(),createGetDb(), andrunMigrations().Test plan
🤖 Generated with Claude Code