server changes for email cli (2/3)#2719
Open
drew-harris wants to merge 4 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoves user-id from app email sender storage and sync paths, permits NULL in app_email_senders.user_id via migration, extracts default magic-code email/template helpers, and updates dashboard routes to use app-level (superadmin/ref-token) authorization and new endpoints for default template and email status. ChangesEmail Sender Refactoring and Default Template Extraction
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
drew-harris
changed the title
drew-harris
changed the title
drew-harris
force-pushed
the
drewh/validate-senders-app-level
branch
from
ebc9b81 to
fdf06ca
Compare
drew-harris
force-pushed
the
drewh/server-email-cli
branch
from
d552476 to
d645fb6
Compare
drew-harris
force-pushed
the
drewh/server-email-cli
branch
from
d645fb6 to
16eb40b
Compare
Contributor
|
View Vercel preview at instant-www-js-drewh-server-email-cli-jsv.vercel.app. |
stopachka
reviewed
Jun 1, 2026
drew-harris
force-pushed
the
drewh/server-email-cli
branch
from
16eb40b to
2df5beb
Compare
stopachka
reviewed
Jun 1, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/instant/model/app_email_sender.clj (1)
85-88:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unexpected
:app-idparameter fromput!call.The
put!function signature at line 21 only accepts{:keys [email name postmark-id]}, but this call is passing:app-id app-id. Theput!function will silently ignore the extra key, but this is misleading and could cause confusion.🔧 Proposed fix
- sender (put! {:email email - :name name - :app-id app-id - :postmark-id postmark-id}) + sender (put! {:email email + :name name + :postmark-id postmark-id})🤖 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 - 88, The map passed to put! in the sender binding includes an unexpected :app-id key which is not accepted by put! (its signature only uses {:keys [email name postmark-id]}). Remove the :app-id app-id entry from the map passed to put! (in the sender binding) so the call only sends :email, :name and :postmark-id to put!.
🤖 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 `@client/packages/platform/src/email.ts`:
- Around line 1-23: The HTML template defaultMagicCodeEmailHtml contains
duplicated nested container divs with identical styles; remove the redundant
inner wrapper so there is a single outer container (retain one set of the style
attributes: background, font-family, line-height, font-size and the
max-width/white background/padding block) and ensure all inner content (the
welcome paragraph, {app_title}, {code}, and notes) remains inside that single
container with matching closing tags; update the string literal accordingly so
the markup is not double-wrapped.
In `@server/resources/migrations/112_allow_null_sender_user_id.down.sql`:
- Line 1: The down migration will fail if any app_email_senders.user_id rows are
NULL; modify 112_allow_null_sender_user_id.down.sql to handle NULLs before
running ALTER TABLE by either (A) adding a data migration step that updates rows
WHERE user_id IS NULL to a safe sentinel/owner user id (e.g., a known system
user) and then performing ALTER TABLE, or (B) adding a conditional check that
aborts with a clear error instructing manual cleanup if NULLs exist; reference
the table app_email_senders and column user_id when implementing this safeguard.
---
Outside diff comments:
In `@server/src/instant/model/app_email_sender.clj`:
- Around line 85-88: The map passed to put! in the sender binding includes an
unexpected :app-id key which is not accepted by put! (its signature only uses
{:keys [email name postmark-id]}). Remove the :app-id app-id entry from the map
passed to put! (in the sender binding) so the call only sends :email, :name and
:postmark-id to put!.
🪄 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: 58ff8ea7-d985-453a-9161-e3e375e48907
📒 Files selected for processing (8)
client/packages/platform/src/email.tsclient/packages/platform/src/index.tsclient/www/components/dash/auth/Email.tsxserver/resources/migrations/112_allow_null_sender_user_id.down.sqlserver/resources/migrations/112_allow_null_sender_user_id.up.sqlserver/src/instant/dash/routes.cljserver/src/instant/model/app_email_sender.cljserver/src/instant/runtime/magic_code_auth.clj
drew-harris
force-pushed
the
drewh/server-email-cli
branch
2 times, most recently
from
2d84eec to
9a8de03
Compare
drew-harris
force-pushed
the
drewh/server-email-cli
branch
from
795d026 to
e951a8b
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/src/instant/runtime/magic_code_auth.clj (1)
126-134: ⚡ Quick winCompute
expirationonce per request.
friendly-expirationis now evaluated twice on the no-template path: once while buildingtemplate-paramsand again insidedefault-email-params. Sincesend!is on the auth path, keep the derived expiration lazy or pass it into the helper so the default flow only does the lookup once.♻️ Proposed fix
-(defn default-email-params [app code] +(defn default-email-params [app code expiration] (let [{default-sender-email :email} (config/app-email-sender) template-params {:code code :app_title (:title app) - :expiration (friendly-expiration app)}] + :expiration expiration}] {:sender-name (:title app) :sender-email default-sender-email :subject (str code " is your verification code for " (:title app)) :body (default-body template-params)}))- template-params {:user_email email - :code code - :app_title (:title app) - :expiration (friendly-expiration app)} + expiration (friendly-expiration app) {default-sender-email :email} (config/app-email-sender) @@ - email-params (if template - {:sender-email sender-email - :sender-name (or (:name template) (:title app)) - :subject (template-replace (:subject template) template-params false) - :body (template-replace (:body template) template-params true)} - (default-email-params app code)) + email-params (if template + (let [template-params {:user_email email + :code code + :app_title (:title app) + :expiration expiration}] + {:sender-email sender-email + :sender-name (or (:name template) (:title app)) + :subject (template-replace (:subject template) template-params false) + :body (template-replace (:body template) template-params true)}) + (default-email-params app code expiration))Also applies to: 151-172
🤖 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/runtime/magic_code_auth.clj` around lines 126 - 134, The function default-email-params is calling friendly-expiration twice via template-params and again inside default-body; compute the expiration once at the top of the request and reuse it: bind a local expiration value (e.g., let [expiration (friendly-expiration app)]) and use that in template-params and when calling default-body (or change default-body to accept an expiration argument), so default-email-params (and related call sites like send!) only perform the lookup a single time; update references to template-params/default-body/friendly-expiration accordingly.
🤖 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.
Nitpick comments:
In `@server/src/instant/runtime/magic_code_auth.clj`:
- Around line 126-134: The function default-email-params is calling
friendly-expiration twice via template-params and again inside default-body;
compute the expiration once at the top of the request and reuse it: bind a local
expiration value (e.g., let [expiration (friendly-expiration app)]) and use that
in template-params and when calling default-body (or change default-body to
accept an expiration argument), so default-email-params (and related call sites
like send!) only perform the lookup a single time; update references to
template-params/default-body/friendly-expiration accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 755cd079-22bc-4c79-99b8-d087e6b7744f
📒 Files selected for processing (5)
server/resources/migrations/112_allow_null_sender_user_id.down.sqlserver/resources/migrations/112_allow_null_sender_user_id.up.sqlserver/src/instant/dash/routes.cljserver/src/instant/model/app_email_sender.cljserver/src/instant/runtime/magic_code_auth.clj
🚧 Files skipped from review as they are similar to previous changes (4)
- server/resources/migrations/112_allow_null_sender_user_id.down.sql
- server/resources/migrations/112_allow_null_sender_user_id.up.sql
- server/src/instant/model/app_email_sender.clj
- server/src/instant/dash/routes.clj
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.
Includes migration to drop user_id field from custom senders table.
Need to run migration before deploying.
Purpose of this pr is to remove the restriction against superuser tokens for auth email routes.