dual write to app_email_verifications#2647
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis pull request introduces email sender verification state tracking. It creates a new ChangesEmail Sender Verification State
🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
drew-harris
force-pushed
the
drewh/email-migration-dual-write
branch
from
ed9707e to
460530a
Compare
drew-harris
force-pushed
the
drewh/email-migration-dual-write
branch
from
460530a to
5b5c4cd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/src/instant/model/app_email_sender.clj (1)
85-94: 🏗️ Heavy liftConsider wrapping dual writes in a transaction.
The sender record write (lines 86-90) and verification write (lines 91-93) each acquire separate connections from the pool, meaning they execute in separate transactions. If the verification write fails after the sender write succeeds, you'll have a sender record without a corresponding verification entry.
For stricter consistency, consider wrapping both writes in a single transaction:
♻️ Possible transaction-based approach
(jdbc/with-transaction [tx (aurora/conn-pool :write)] (let [sender (put! tx {:email email :name name :user-id user-id :postmark-id postmark-id})] (verification/put! tx {:app-id app-id :sender-id (get sender :id) :verified true}) sender))This ensures both writes commit or roll back together.
Note: Given this is Step 1 of a migration, temporary inconsistency might be acceptable—just be aware of the dual-write risk.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/instant/model/app_email_sender.clj` around lines 85 - 94, The sender write (put!) and the verification write (verification/put!) run as separate DB connections/transactions, risking partial commits; wrap both operations in a single transaction using jdbc/with-transaction (using aurora/conn-pool :write) and pass the transaction binding (tx) into both put! calls so the creation of sender (store postmark-id, email, name, app-id, user-id) and the verification entry (app-id, :sender-id from sender, :verified true) commit or roll back together.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/src/instant/model/app_email_verification.clj`:
- Around line 11-14: The INSERT into app_email_verifications using
sql/execute-one! currently uses "ON CONFLICT DO NOTHING" which prevents updating
an existing row for the same app_id and sender_id; change the conflict clause to
"ON CONFLICT (app_id, sender_id) DO UPDATE SET verified = EXCLUDED.verified,
updated_at = now()" so existing verifications can be updated (keep the same
INSERT values and UUID generation but replace the DO NOTHING clause with the DO
UPDATE clause referencing EXCLUDED.verified and updated_at = now()).
---
Nitpick comments:
In `@server/src/instant/model/app_email_sender.clj`:
- Around line 85-94: The sender write (put!) and the verification write
(verification/put!) run as separate DB connections/transactions, risking partial
commits; wrap both operations in a single transaction using
jdbc/with-transaction (using aurora/conn-pool :write) and pass the transaction
binding (tx) into both put! calls so the creation of sender (store postmark-id,
email, name, app-id, user-id) and the verification entry (app-id, :sender-id
from sender, :verified true) commit or roll back together.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4395baa8-97f6-4a6d-a566-8681e411132c
📒 Files selected for processing (4)
server/resources/migrations/107_add_email_verifications.down.sqlserver/resources/migrations/107_add_email_verifications.up.sqlserver/src/instant/model/app_email_sender.cljserver/src/instant/model/app_email_verification.clj
| (sql/execute-one! conn ["INSERT INTO | ||
| app_email_verifications | ||
| (id, app_id, sender_id, verified) | ||
| VALUES (?::uuid, ?, ?, ?) ON CONFLICT DO NOTHING" (UUID/randomUUID) app-id sender-id verified]))) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for calls to verification/put! or app-email-verification/put! with verified parameter
rg -nP --type=clojure -C3 '(verification/put!|app-email-verification/put!)'Repository: instantdb/instant
Length of output: 671
🏁 Script executed:
# First, let's look at the file being reviewed to understand its context
cat -n server/src/instant/model/app_email_verification.cljRepository: instantdb/instant
Length of output: 634
🏁 Script executed:
# Search for the verification/put! function definition
rg -n "defn\s+put!" --type=clojure -A 10 | head -50Repository: instantdb/instant
Length of output: 3553
🏁 Script executed:
# Check for any migrations or schema files related to app_email_verifications
fd -type f -name "*.sql" -o -name "*migration*" | head -20Repository: instantdb/instant
Length of output: 232
🏁 Script executed:
# Search for all calls to app_email_verifications table or verification functions
rg -n "app_email_verifications|app-email-verification" --type=clojure | head -30Repository: instantdb/instant
Length of output: 356
🏁 Script executed:
# Look for any calls that might create verified=false
rg -n ":verified\s*false" --type=clojureRepository: instantdb/instant
Length of output: 43
🏁 Script executed:
# Search for migrations or schema files
find . -type f \( -name "*.sql" -o -name "*migration*" -o -name "*schema*" \) | grep -i -E "(migration|schema)" | head -20Repository: instantdb/instant
Length of output: 1102
🏁 Script executed:
# Look in db/migrations or similar directories
find . -type d \( -name migrations -o -name sql -o -name db \) 2>/dev/nullRepository: instantdb/instant
Length of output: 204
🏁 Script executed:
# Search for CREATE TABLE or ALTER TABLE statements related to app_email_verifications
rg -n "app_email_verifications" --type=text -B2 -A5Repository: instantdb/instant
Length of output: 90
🏁 Script executed:
# Get more context around the verification/put! call in app_email_sender.clj
sed -n '80,100p' server/src/instant/model/app_email_sender.cljRepository: instantdb/instant
Length of output: 914
🏁 Script executed:
# Look for migrations in the server resources
ls -la server/resources/migrations/Repository: instantdb/instant
Length of output: 17450
🏁 Script executed:
# Search for any SQL files that mention app_email_verifications
rg -n "app_email_verifications" server/resources/migrations/ -A5 -B2Repository: instantdb/instant
Length of output: 1338
🏁 Script executed:
# Search for all instances where verification/put! is called
rg -n "verification/put!" --type=clojure -B2 -A2Repository: instantdb/instant
Length of output: 523
🏁 Script executed:
# Check if there's any code that might create or update the verified field to false
rg -n "verified" server/src/instant/model/app_email_verification.clj server/src/instant/model/app_email_sender.cljRepository: instantdb/instant
Length of output: 504
Change to ON CONFLICT DO UPDATE to allow future verification updates.
The ON CONFLICT DO NOTHING clause prevents any updates to records with the same (app_id, sender_id). While current code always passes verified=true on insert, if future code needs to update an existing verification's status (e.g., from false to true), this approach will silently fail.
This pattern is also inconsistent with other similar models in the codebase (rule.clj, instant_profile.clj, app_email_sender.clj all use DO UPDATE SET). Use ON CONFLICT (app_id, sender_id) DO UPDATE SET verified = EXCLUDED.verified, updated_at = now() instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/instant/model/app_email_verification.clj` around lines 11 - 14,
The INSERT into app_email_verifications using sql/execute-one! currently uses
"ON CONFLICT DO NOTHING" which prevents updating an existing row for the same
app_id and sender_id; change the conflict clause to "ON CONFLICT (app_id,
sender_id) DO UPDATE SET verified = EXCLUDED.verified, updated_at = now()" so
existing verifications can be updated (keep the same INSERT values and UUID
generation but replace the DO NOTHING clause with the DO UPDATE clause
referencing EXCLUDED.verified and updated_at = now()).
|
View Vercel preview at instant-www-js-drewh-email-migration-dual-write-jsv.vercel.app. |
2 participants
Step 1 of migrating the user based custom email sender verification to app based.
For each update / creation of a custom email template, if there is a custom sender id, create a row in a
app_id <-> postmark idverifications table with "verified = true"