cli auth email fixes#2760
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughSchema updates make email sender and verification fields nullish-capable; the CLI push command now fetches the server default email template and omits ChangesEmail configuration and default template integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/packages/cli/src/commands/auth/email/push.ts (1)
70-81:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
sendSenderVerificationwith the same default-sender condition.The new default-sender guard is only applied to DNS verification (Lines 83-86), but the email send (Lines 70-81) can still fire based only on
verification_verified === false. That can trigger unnecessary verification emails for default/omitted sender scenarios.Suggested fix
+ const shouldVerifyCustomSender = + !!authEmail.senderEmail && authEmail.senderEmail !== defTemplate.senderEmail; + - if (info?.verification_verified === false) { + if (shouldVerifyCustomSender && info?.verification_verified === false) { yield* sendSenderVerification; yield* Effect.log( boxen( "We've sent a confirmation email containing a six digit code to verify the sender email address.\nUse instant-cli auth email verify <code> to complete verification.", { borderColor: 'yellow', padding: { right: 1, left: 1 }, }, ), ); } - if ( - emailConfig.authEmail.senderEmail && - emailConfig.authEmail.senderEmail !== defTemplate.senderEmail - ) { + if (shouldVerifyCustomSender) { const verification = yield* getVerification; if (verification.verification) { yield* Effect.log( formatSenderVerificationDnsRecords(verification.verification), ); } }Also applies to: 83-86
🤖 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 `@client/packages/cli/src/commands/auth/email/push.ts` around lines 70 - 81, The sendSenderVerification branch is triggered solely by info?.verification_verified === false which can send unnecessary verification emails for the default/omitted sender; wrap the yield* sendSenderVerification and the associated Effect.log boxen message with the same default-sender guard used for the DNS verification branch so the sender verification only runs when the sender is not the default/omitted address (i.e., apply the same condition used in the DNS verification block to the sendSenderVerification path).
🤖 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.
Outside diff comments:
In `@client/packages/cli/src/commands/auth/email/push.ts`:
- Around line 70-81: The sendSenderVerification branch is triggered solely by
info?.verification_verified === false which can send unnecessary verification
emails for the default/omitted sender; wrap the yield* sendSenderVerification
and the associated Effect.log boxen message with the same default-sender guard
used for the DNS verification branch so the sender verification only runs when
the sender is not the default/omitted address (i.e., apply the same condition
used in the DNS verification block to the sendSenderVerification path).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 16ad5a85-65c7-40ca-bf62-9da5973576dd
📒 Files selected for processing (3)
client/packages/cli/src/commands/auth/email/push.tsclient/packages/cli/src/commands/auth/email/status.tsclient/packages/cli/src/lib/email.ts
Contributor
|
View Vercel preview at instant-www-js-drewh-auth-cli-email-fixes-jsv.vercel.app. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Handles edge cases where the user pulls a config with sender email == verify@auth-pm.instantdb.com