CLI email template commands (2/2)#2633
Draft
drew-harris wants to merge 1 commit into
Draft
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
📝 WalkthroughWalkthroughAdds CLI commands ChangesEmail Template Push/Pull Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 |
Contributor
|
View Vercel preview at instant-www-js-drewh-cli-email-jsv.vercel.app. |
![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: 3
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/dash/routes.clj (1)
1357-1387:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
sender-user-idwill benilfor org-owned apps, causing a NOT NULL constraint violation insync-sender!
req->app-accepting-superadmin-or-ref-token!explicitly usesselect-keysto return only{:app ...}, souseris alwaysnil. The fallback(or (:id user) (:creator_id app))therefore reduces to(:creator_id app), which isnilfor org-owned apps (they useorg-idbut have nocreator-id).When a user provides
sender-emailfor an org-owned app,sync-sender!callsput!with{:user-id nil ...}, which violates the NOT NULL constraint on theuser_idcolumn inapp_email_senders(set by migration 21).The old
req->app-and-user!always produced a non-nil:user– this is a regression from switching to the new auth function.🔧 One fix option: look up the authenticated user separately for the sender-user-id
-(let [{app :app user :user} (req->app-accepting-superadmin-or-ref-token! :admin :apps/write req) +(let [{app :app} (req->app-accepting-superadmin-or-ref-token! :admin :apps/write req) + user (try (req->auth-user! req) (catch Exception _ nil))🤖 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/dash/routes.clj` around lines 1357 - 1387, The sender-user-id can be nil because req->app-accepting-superadmin-or-ref-token! only returns :app; update email-template-post to explicitly look up the authenticated user for sender-user-id (e.g., call the existing request user helper such as req->user! or the equivalent auth helper) and set sender-user-id to (:id authenticated-user) or fallback to (:creator_id app); ensure you do not pass nil into app-email-sender-model/sync-sender! (or else validate/raise if no valid user-id can be determined) so sync-sender! is never invoked with a nil :user-id.
🧹 Nitpick comments (3)
client/packages/cli/src/commands/auth/email/pull.ts (1)
7-19: 💤 Low valueSchema constants used before declaration — move them above
authEmailPullCmd
PullEmailResponse,EmailTemplate, andNullableString(lines 21–40) are referenced inside theauthEmailPullCmdgenerator body (line 13), but defined after it. This works at runtime because Effect's lazy evaluation means the generator body only executes after module initialization completes, but it hurts readability. Moving the schema definitions to the top of the file makes the data model visible before the function that consumes it.🤖 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/pull.ts` around lines 7 - 19, The schema constants PullEmailResponse, EmailTemplate, and NullableString are declared after authEmailPullCmd but referenced inside its generator; move the declarations for NullableString, EmailTemplate, and PullEmailResponse to appear above the authEmailPullCmd definition so the data model is visible before use, and leave the rest of the function (including CurrentApp and InstantHttpAuthed usage) unchanged.server/src/instant/dash/routes.clj (1)
1389-1393: 💤 Low value
email-template-gethardcodes"magic-code"— consider accepting email-type as a query param for future extensibilityThe POST endpoint accepts any
:email-typefrom the body, but this GET endpoint is hardcoded. If a second email type (e.g."invite") is ever added, a new endpoint or a query parameter would be needed. Making it a?email-type=query param now would mirror the POST design and avoid breaking the API later.🤖 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/dash/routes.clj` around lines 1389 - 1393, The GET handler email-template-get currently hardcodes the email-type to "magic-code"; change it to read an optional ?email-type= query parameter from the request (e.g. from req's query-params) and pass that value into app-email-template-model/get-by-app-id-and-email-type, defaulting to "magic-code" when the query param is absent or empty so behavior stays backward compatible; update email-template-get to extract the query param, validate/normalize it if necessary, and use that variable instead of the string literal.client/packages/cli/src/commands/auth/email/push.ts (1)
4-6: 💤 Low value
authEmailPushCmdis a no-op wrapper — consider a direct re-exportThe function body is just
yield* pushEmail, adding no additional logic, error mapping, or context. A re-export would be simpler and consistent with howauthEmailPullCmdexposes its logic directly:-export const authEmailPushCmd = Effect.fn(function* () { - yield* pushEmail; -}); +export { pushEmail as authEmailPushCmd } from '../../../lib/email.ts';🤖 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 4 - 6, authEmailPushCmd is just a no-op wrapper around pushEmail (yield* pushEmail) and should be exposed directly like authEmailPullCmd; remove the Effect.fn wrapper and re-export pushEmail as authEmailPushCmd (or export { pushEmail as authEmailPushCmd }) so the original Effect value and types are preserved and no extra indirection remains.
🤖 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/cli/src/commands/auth/email/pull.ts`:
- Around line 15-18: The current extraction uses a redundant union guard
('template' in emailState) and logs JSON.stringify(template) which prints "null"
with no user-facing message; simplify PullEmailResponse to always be { template:
EmailTemplate | null }, remove the union variants and the `'template' in
emailState` guard, access the template directly via emailState.template (or the
corresponding response field) in pull.ts, and replace the raw JSON.stringify log
with a friendly message when template is null (e.g., "No template configured")
otherwise pretty-print the template via Effect.log.
In `@client/packages/cli/src/commands/info.ts`:
- Around line 36-41: The block using Effect.serviceOption(CurrentApp) is
unreachable and wrong: either remove it or wire the info command to provide
CurrentApp (add WithAppLayer in index.ts), and if you keep it fix the HTTP call
to interpolate appId into the correct route (use GET /dash/apps/:app_id via
authedHttp.get with appId), ensure authedHttp exists before calling it, and
actually use the returned appInfo (log or return it) instead of leaving it
unused.
In `@client/packages/cli/src/lib/email.ts`:
- Around line 59-73: The request currently sets 'sender-email' to an empty
string via authEmail.fromAddress ?? '' which sends "" in JSON; change the body
construction passed to HttpClientRequest.bodyJson so it omits the 'sender-email'
property entirely when authEmail.fromAddress is null/undefined/empty (or
alternatively set it to null), e.g. build the payload from authEmail.subject,
authEmail.body, authEmail.from, and conditionally include 'sender-email' only
when authEmail.fromAddress has a value before calling HttpClientRequest.bodyJson
in the pipeline that uses withCommand('auth email push') and
.post(`/dash/apps/${appId}/email_templates`).
---
Outside diff comments:
In `@server/src/instant/dash/routes.clj`:
- Around line 1357-1387: The sender-user-id can be nil because
req->app-accepting-superadmin-or-ref-token! only returns :app; update
email-template-post to explicitly look up the authenticated user for
sender-user-id (e.g., call the existing request user helper such as req->user!
or the equivalent auth helper) and set sender-user-id to (:id
authenticated-user) or fallback to (:creator_id app); ensure you do not pass nil
into app-email-sender-model/sync-sender! (or else validate/raise if no valid
user-id can be determined) so sync-sender! is never invoked with a nil :user-id.
---
Nitpick comments:
In `@client/packages/cli/src/commands/auth/email/pull.ts`:
- Around line 7-19: The schema constants PullEmailResponse, EmailTemplate, and
NullableString are declared after authEmailPullCmd but referenced inside its
generator; move the declarations for NullableString, EmailTemplate, and
PullEmailResponse to appear above the authEmailPullCmd definition so the data
model is visible before use, and leave the rest of the function (including
CurrentApp and InstantHttpAuthed usage) unchanged.
In `@client/packages/cli/src/commands/auth/email/push.ts`:
- Around line 4-6: authEmailPushCmd is just a no-op wrapper around pushEmail
(yield* pushEmail) and should be exposed directly like authEmailPullCmd; remove
the Effect.fn wrapper and re-export pushEmail as authEmailPushCmd (or export {
pushEmail as authEmailPushCmd }) so the original Effect value and types are
preserved and no extra indirection remains.
In `@server/src/instant/dash/routes.clj`:
- Around line 1389-1393: The GET handler email-template-get currently hardcodes
the email-type to "magic-code"; change it to read an optional ?email-type= query
parameter from the request (e.g. from req's query-params) and pass that value
into app-email-template-model/get-by-app-id-and-email-type, defaulting to
"magic-code" when the query param is absent or empty so behavior stays backward
compatible; update email-template-get to extract the query param,
validate/normalize it if necessary, and use that variable instead of the string
literal.
🪄 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: 873d1a3d-d509-4019-8b76-dd669e31148a
📒 Files selected for processing (8)
client/packages/cli/src/commands/auth/email/pull.tsclient/packages/cli/src/commands/auth/email/push.tsclient/packages/cli/src/commands/info.tsclient/packages/cli/src/index.tsclient/packages/cli/src/lib/email.tsclient/packages/cli/src/old.jsclient/packages/cli/src/util/findConfigCandidates.tsserver/src/instant/dash/routes.clj
drew-harris
force-pushed
the
drewh/cli-email
branch
from
faabd4c to
d36040e
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
client/packages/cli/src/commands/info.ts (1)
36-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIncomplete implementation: wrong URL, unused variable, and dead code —
CurrentAppis never provided toinfoThese issues were flagged in a previous review and remain unresolved:
- Wrong URL — Line 40 uses the hardcoded
/dash/app/path. TheappIdfrom line 39 is never interpolated.- Unused variable —
appInfois assigned but never consumed (logged or returned).- Dead code — The
infocommand is wired withAuthLayerLive, notWithAppLayer, soCurrentAppis never provided to the service context.Effect.serviceOption(CurrentApp)will always yieldOption.none(), making this entire block unreachable.🤖 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/info.ts` around lines 36 - 41, The block using Effect.serviceOption(CurrentApp) is dead because the info command is wired with AuthLayerLive (not WithAppLayer), and it also misuses appId and appInfo; either remove this unreachable branch or properly provide CurrentApp via WithAppLayer to info; if you choose to keep it, change the HTTP call in the authedHttp.get to interpolate the appId (e.g., `/dash/app/${appId}`), and actually use appInfo (log or return it) instead of leaving it unused; ensure consistency between the wiring (AuthLayerLive vs WithAppLayer), the service lookup (CurrentApp), and the HTTP request/consumption (appId and appInfo).
🧹 Nitpick comments (1)
client/packages/cli/src/util/findConfigCandidates.ts (1)
87-95: ⚡ Quick winConsider collapsing the repeated env-path / read-candidates / write-path boilerplate into parameterised helpers.
getEnvSchemaPathWithLogging,getEnvPermsPathWithLogging, andgetEnvEmailPathWithLoggingare structurally identical, differing only by env-var name. The same triplication applies toget*ReadCandidates(root candidate +src/libscan +appprefix) andget*PathToWrite. Each time a new config type is introduced, three more copy-paste functions are needed. Extracting factory helpers now (while there are only three types) would keep future additions a one-liner.♻️ Sketch of a factory approach
+function makeEnvPathLogger(envVar: string): () => string | undefined { + return function () { + const p = process.env[envVar]; + if (p) console.log(`Using ${envVar}=${chalk.green(p)}`); + return p; + }; +} + +function makeReadCandidates(stem: string, envVar: string): () => ConfigCandidate[] { + return function () { + const existing = process.env[envVar]; + if (existing) { + if (existing) console.log(`Using ${envVar}=${chalk.green(existing)}`); + return [{ files: existing, transform: transformImports }]; + } + const extensions = ['ts', 'mts', 'cts', 'js', 'mjs', 'cjs']; + const candidates: ConfigCandidate[] = [ + { files: stem, extensions, transform: transformImports }, + ]; + for (const p of findPathsRecursive('src', 3, stem)) + candidates.push({ files: p, extensions, transform: transformImports }); + for (const p of findPathsRecursive('lib', 2, stem)) + candidates.push({ files: p, extensions, transform: transformImports }); + candidates.push({ files: `app/${stem}`, extensions, transform: transformImports }); + return candidates; + }; +} + +function makePathToWrite(envVar: string, defaultStem: string): () => string { + return function () { + if (process.env[envVar]) return process.env[envVar]!; + if (existsSync(join(process.cwd(), 'src'))) return join('src', `${defaultStem}.ts`); + return `${defaultStem}.ts`; + }; +} + -function getEnvEmailPathWithLogging() { ... } -export function getEmailReadCandidates() { ... } -export function getEmailPathToWrite() { ... } +export const getEmailReadCandidates = makeReadCandidates('instant.email', 'INSTANT_EMAIL_FILE_PATH'); +export const getEmailPathToWrite = makePathToWrite('INSTANT_EMAIL_FILE_PATH', 'instant.email');Also applies to: 188-228, 252-260
🤖 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/util/findConfigCandidates.ts` around lines 87 - 95, Several functions are duplicated (getEnvSchemaPathWithLogging, getEnvPermsPathWithLogging, getEnvEmailPathWithLogging and the get*ReadCandidates / get*PathToWrite families) — extract parameterized factory helpers to eliminate copy/paste: create a single makeEnvPathWithLogging(envVarName) that returns the env-path getter used for schema/perms/email; create makeReadCandidates(baseNames, options?) to produce the root/src/lib/app-prefixed candidate arrays and makePathToWrite(baseName) for write-path logic, then replace the specific functions (getEnvSchemaPathWithLogging, getEnvPermsPathWithLogging, getEnvEmailPathWithLogging, getSchemaReadCandidates/getPermsReadCandidates/getEmailReadCandidates, getSchemaPathToWrite/getPermsPathToWrite/getEmailPathToWrite) with one-line invocations of those factories so all behavior and logging remains the same but future config types are a one-liner.
🤖 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.
Duplicate comments:
In `@client/packages/cli/src/commands/info.ts`:
- Around line 36-41: The block using Effect.serviceOption(CurrentApp) is dead
because the info command is wired with AuthLayerLive (not WithAppLayer), and it
also misuses appId and appInfo; either remove this unreachable branch or
properly provide CurrentApp via WithAppLayer to info; if you choose to keep it,
change the HTTP call in the authedHttp.get to interpolate the appId (e.g.,
`/dash/app/${appId}`), and actually use appInfo (log or return it) instead of
leaving it unused; ensure consistency between the wiring (AuthLayerLive vs
WithAppLayer), the service lookup (CurrentApp), and the HTTP request/consumption
(appId and appInfo).
---
Nitpick comments:
In `@client/packages/cli/src/util/findConfigCandidates.ts`:
- Around line 87-95: Several functions are duplicated
(getEnvSchemaPathWithLogging, getEnvPermsPathWithLogging,
getEnvEmailPathWithLogging and the get*ReadCandidates / get*PathToWrite
families) — extract parameterized factory helpers to eliminate copy/paste:
create a single makeEnvPathWithLogging(envVarName) that returns the env-path
getter used for schema/perms/email; create makeReadCandidates(baseNames,
options?) to produce the root/src/lib/app-prefixed candidate arrays and
makePathToWrite(baseName) for write-path logic, then replace the specific
functions (getEnvSchemaPathWithLogging, getEnvPermsPathWithLogging,
getEnvEmailPathWithLogging,
getSchemaReadCandidates/getPermsReadCandidates/getEmailReadCandidates,
getSchemaPathToWrite/getPermsPathToWrite/getEmailPathToWrite) with one-line
invocations of those factories so all behavior and logging remains the same but
future config types are a one-liner.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 790fcbae-a349-4dcf-89cb-dffdacb61dba
📒 Files selected for processing (8)
client/packages/cli/src/commands/auth/email/pull.tsclient/packages/cli/src/commands/auth/email/push.tsclient/packages/cli/src/commands/info.tsclient/packages/cli/src/index.tsclient/packages/cli/src/lib/email.tsclient/packages/cli/src/old.jsclient/packages/cli/src/util/findConfigCandidates.tsserver/src/instant/dash/routes.clj
✅ Files skipped from review due to trivial changes (1)
- client/packages/cli/src/commands/auth/email/push.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- client/packages/cli/src/index.ts
- client/packages/cli/src/lib/email.ts
- client/packages/cli/src/commands/auth/email/pull.ts
- server/src/instant/dash/routes.clj
drew-harris
force-pushed
the
drewh/cli-email
branch
from
d36040e to
211b5ab
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
client/packages/cli/src/commands/auth/email/pull.ts (2)
34-40: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winCollapse
PullEmailResponseto match the actual server contract.Since the server always returns
{ template: EmailTemplate | null }, the two extra union arms (EmailTemplateandSchema.Nullat the top level) are dead code and add noise. Simplifying also removes the need for the guard in the command body above.✨ Suggested simplification
-const PullEmailResponse = Schema.Union( - Schema.Struct({ - template: Schema.Union(EmailTemplate, Schema.Null), - }), - EmailTemplate, - Schema.Null, -); +const PullEmailResponse = Schema.Struct({ + template: Schema.Union(EmailTemplate, Schema.Null), +});🤖 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/pull.ts` around lines 34 - 40, PullEmailResponse is an overly broad union; simplify it to match the server contract by replacing the top-level Union with a single Schema.Struct that has a "template" field whose type is Schema.Union(EmailTemplate, Schema.Null) (or Schema.Optional/Nullable equivalent used elsewhere) so the response shape is exactly { template: EmailTemplate | null }, removing the extra EmailTemplate and Schema.Null arms and the runtime guard in the command.
15-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
nulltemplate logs as"null"with no user-facing message; response extraction can be simplified.The existing comment on this segment remains valid against the current code. The server-side
email-template-getendpoint always returns{ template: ... }, making theEmailTemplateandSchema.Nulltop-level union variants inPullEmailResponseunreachable, so the'template' in emailStateguard is alwaystrue. Additionally, whentemplateisnull,JSON.stringify(null, null, 2)prints the string"null"with no user-friendly explanation.✨ Suggested simplification
- const template = - emailState && 'template' in emailState ? emailState.template : emailState; - - yield* Effect.log(JSON.stringify(template, null, 2)); + const template = emailState.template; + + if (!template) { + yield* Effect.log('No email template configured for this app.'); + } else { + yield* Effect.log(JSON.stringify(template, null, 2)); + }🤖 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/pull.ts` around lines 15 - 18, The guard and JSON.stringify call produce confusing output when template is null and the union branch is unreachable; simplify by directly reading emailState.template (since the server always returns { template: ... }) and replace the raw JSON.stringify with a user-friendly log when template is null (e.g., log "No template available" or similar) otherwise log the template content; update the code paths referencing template and emailState in pull.ts (the template variable extraction and the Effect.log call) 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.
Duplicate comments:
In `@client/packages/cli/src/commands/auth/email/pull.ts`:
- Around line 34-40: PullEmailResponse is an overly broad union; simplify it to
match the server contract by replacing the top-level Union with a single
Schema.Struct that has a "template" field whose type is
Schema.Union(EmailTemplate, Schema.Null) (or Schema.Optional/Nullable equivalent
used elsewhere) so the response shape is exactly { template: EmailTemplate |
null }, removing the extra EmailTemplate and Schema.Null arms and the runtime
guard in the command.
- Around line 15-18: The guard and JSON.stringify call produce confusing output
when template is null and the union branch is unreachable; simplify by directly
reading emailState.template (since the server always returns { template: ... })
and replace the raw JSON.stringify with a user-friendly log when template is
null (e.g., log "No template available" or similar) otherwise log the template
content; update the code paths referencing template and emailState in pull.ts
(the template variable extraction and the Effect.log call) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ffb8ec8b-fabd-4ab7-9c41-d62a33ad52e8
📒 Files selected for processing (3)
client/packages/cli/src/commands/auth/email/pull.tsclient/packages/cli/src/index.tsserver/src/instant/dash/routes.clj
🚧 Files skipped from review as they are similar to previous changes (2)
- client/packages/cli/src/index.ts
- server/src/instant/dash/routes.clj
drew-harris
force-pushed
the
drewh/cli-email
branch
from
211b5ab to
5d4cda3
Compare
drew-harris
force-pushed
the
drewh/cli-email
branch
from
8e50141 to
118e77d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/packages/cli/src/commands/auth/email/pull.ts (1)
8-9: ⚡ Quick winGuard
optsbefore readingfileLine 9 assumes an options object is always present. If a caller invokes this command with
undefined, this will throw beforepullEmailruns.Suggested patch
-export const authEmailPullCmd = Effect.fn(function* (opts: AuthEmailPullOpts) { - yield* pullEmail(opts.file); +export const authEmailPullCmd = Effect.fn(function* ( + opts: AuthEmailPullOpts = {}, +) { + yield* pullEmail(opts.file); });🤖 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/pull.ts` around lines 8 - 9, authEmailPullCmd currently dereferences opts.file without validating opts; if callers pass undefined this will throw. Update authEmailPullCmd to guard the parameter (opts) before accessing file: check opts is truthy (or provide a default object) and validate that opts.file exists (or handle missing file) before calling pullEmail(opts.file), and if invalid, return or throw a clear error. Reference the function authEmailPullCmd, the type AuthEmailPullOpts, and the call to pullEmail to locate the change.
🤖 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/cli/src/commands/pull.ts`:
- Around line 111-113: The code sets fromAddress by treating any falsy value the
same as undefined, which turns a legitimate empty-string sender ("") into
'undefined'; change the check in pull.ts so it only treats null/undefined as
missing: use a test against null/undefined (e.g., template.email !== null &&
template.email !== undefined or typeof template.email === 'string') and only
call JSON.stringify(template.email) in that case, otherwise set
'undefined'—update the assignment for fromAddress accordingly so empty-string
values are preserved.
---
Nitpick comments:
In `@client/packages/cli/src/commands/auth/email/pull.ts`:
- Around line 8-9: authEmailPullCmd currently dereferences opts.file without
validating opts; if callers pass undefined this will throw. Update
authEmailPullCmd to guard the parameter (opts) before accessing file: check opts
is truthy (or provide a default object) and validate that opts.file exists (or
handle missing file) before calling pullEmail(opts.file), and if invalid, return
or throw a clear error. Reference the function authEmailPullCmd, the type
AuthEmailPullOpts, and the call to pullEmail to locate the change.
🪄 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: be550d82-1a98-41a2-aef1-c7025b73f12d
📒 Files selected for processing (8)
client/packages/cli/__tests__/authEmail.test.tsclient/packages/cli/src/commands/auth/email/pull.tsclient/packages/cli/src/commands/auth/email/push.tsclient/packages/cli/src/commands/pull.tsclient/packages/cli/src/index.tsclient/packages/cli/src/lib/email.tsclient/packages/cli/src/old.jsclient/packages/cli/src/util/findConfigCandidates.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- client/packages/cli/src/commands/auth/email/push.ts
- client/packages/cli/src/index.ts
- client/packages/cli/src/util/findConfigCandidates.ts
- client/packages/cli/src/lib/email.ts
drew-harris
force-pushed
the
drewh/cli-email
branch
from
da881de to
f76b6a3
Compare
drew-harris
changed the title
drew-harris
force-pushed
the
drewh/validate-senders-app-level
branch
from
253b606 to
19eff43
Compare
drew-harris
force-pushed
the
drewh/cli-email
branch
from
b0f64ee to
4d10e42
Compare
drew-harris
force-pushed
the
drewh/validate-senders-app-level
branch
from
19eff43 to
68c7afb
Compare
drew-harris
force-pushed
the
drewh/cli-email
branch
from
4d10e42 to
4ffcfa7
Compare
drew-harris
force-pushed
the
drewh/validate-senders-app-level
branch
from
68c7afb to
8e8d726
Compare
drew-harris
force-pushed
the
drewh/cli-email
branch
from
4ffcfa7 to
31d0019
Compare
drew-harris
force-pushed
the
drewh/cli-email
branch
from
31d0019 to
75bd174
Compare
drew-harris
force-pushed
the
drewh/validate-senders-app-level
branch
2 times, most recently
from
ad0aafd to
a98e807
Compare
drew-harris
force-pushed
the
drewh/cli-email
branch
from
75bd174 to
683cb29
Compare
drew-harris
force-pushed
the
drewh/validate-senders-app-level
branch
from
a98e807 to
1d01c82
Compare
drew-harris
force-pushed
the
drewh/cli-email
branch
from
683cb29 to
270f634
Compare
drew-harris
force-pushed
the
drewh/cli-email
branch
from
270f634 to
1a2f6c3
Compare
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
1 participant
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.
Adds
instant-cli auth emailcommands to push and pull the email template to/from an instant.email.ts file.