Outgoing email: pick a provider instead of typing seven fields - #428
Open
onel wants to merge 12 commits into
Open
Outgoing email: pick a provider instead of typing seven fields#428onel wants to merge 12 commits into
onel wants to merge 12 commits into
Conversation
Adding an outgoing-email account meant typing host, port, encryption,
username, password, from address and label — each looked up in the
provider's own docs. For any provider worth presetting, four of those
are constants and often the username is too.
Settings → Outgoing email is now a two-step add: pick the provider,
then supply the credential, the from address, a username where the
provider does not fix one, and a region for the two providers whose
region changes the host. Host, port and encryption are prefilled behind
an Advanced settings disclosure so a non-standard endpoint is never
trapped; custom renders as that disclosure open with nothing prefilled,
which is the old form.
Nine presets live in a new leaf package internal/mailpreset and are
served admin-only at GET /api/v1/mail-presets. Every one is STARTTLS on
a port a hosted box can reach — 587, except SMTP2GO's 2525. Hosted
blocks 25 and 465, so no preset may default to implicit TLS.
Two of the planned constants were wrong. Mailgun's EU host is
smtp.eu.mailgun.org, a prefix rather than a region code, so one
{region} host template could not serve both it and SES; each region
option now names the host it resolves to. Brevo's username is the SMTP
login xxx@smtp-brevo.com, not the account email.
mail_providers.provider_type persists which preset was picked, via the
idempotent ALTER path with DEFAULT 'custom'. No CHECK — one cannot ride
an ALTER — so it is validated in Go like scope and exposure. Rows that
predate this load as custom, which is right: they were typed by hand.
The column is kept for the credential broker, not for the form.
The server stores what the client sends and never re-derives host, port
or encryption from the preset; re-deriving would silently undo the
advanced override. The test-send now names the blocked port when a
hosted box fails to connect on 25 or 465, instead of a bare timeout.
internal/lifecycle/mail.go is untouched on purpose.
Not done: no live test-send against any provider. The constants are
verified against each vendor's current docs and nothing more.
Closes #426.
Claude-Session: https://claude.ai/code/session_01XHrpFqkLosCXFbKeCHoEdo
Confidence Score: 3/5The PR does not appear safe to merge because preset failures still block Custom SMTP setup and can let a Postmark username diverge from its retained token. The current add-flow error state offers only a retry even though Custom SMTP is part of the unavailable server response. The edit flow also loses Postmark’s username lock when that response is unavailable, while the API accepts the changed username and keeps the old password. Files Needing Attention: web-ui/src/views/settings/OutgoingEmailAddSection.vue, web-ui/src/views/settings/OutgoingEmailSection.vue
|
| Filename | Overview |
|---|---|
| web-ui/src/views/settings/OutgoingEmailSection.vue | Adds preset-aware editing, but the previously reported Postmark credential mismatch remains reachable when preset loading fails. |
| web-ui/src/views/settings/OutgoingEmailAddSection.vue | Adds the two-step account flow and retry feedback, but a persistent preset failure still blocks the server-provided Custom SMTP path. |
| web-ui/src/mailProviderForm.ts | Centralizes preset form initialization, validation, warnings, and same-as-password synchronization. |
| internal/api/mail.go | Adds preset and pre-save verification endpoints, provider-type handling, and clearer hosted-port connection errors. |
| internal/mailpreset/mailpreset.go | Defines immutable SMTP provider presets, including region mappings and the Custom SMTP option. |
| internal/store/mail.go | Persists provider type while retaining the existing write-only password during blank-password updates. |
Reviews (7): Last reviewed commit: "small UI change" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Self-review found one Block and two Notes. Block: editing a Postmark account wiped its username. startEdit blanks the password field, because an empty password means "keep the stored one". The update path then copied that empty value across to the username, which same_as_password keeps in step, and the server overwrites username unconditionally. Editing a Postmark account's label was enough to leave it with a stored password and no username, failing AUTH on its next send with nothing in the UI to say why. A blank password is no longer copied across. blockedPortHint matched on the string prefix "connect: ", a silent coupling to sendTestMail's error wording. Dial failures are now a typed *dialError matched with errors.As, and a new test drives the real sendTestMail against a closed port to assert the live path still produces one — reverting the type makes that test fail. The :open binding on the two details disclosures is correct only because both blocks remount on change. That assumption is now written next to the binding. Also records in the progress entry that every preset host was probed live with net/smtp: all sixteen hostnames, including all eight SES regions, connect on the preset's port, complete STARTTLS with a valid certificate for that name, and offer AUTH PLAIN. Claude-Session: https://claude.ai/code/session_01XHrpFqkLosCXFbKeCHoEdo
The rule said to always implement in a git worktree. In practice that leaves a second copy of the repo to find, test in and clean up, and the finished branch has to be pulled back into the main folder anyway. A worktree now covers only the case it exists for: uncommitted changes a checkout would disturb. Otherwise branch off dev and work in place. Keeps the guidance that a worktree, when needed, goes outside the repo folder and is removed once the branch is pushed. Claude-Session: https://claude.ai/code/session_01XHrpFqkLosCXFbKeCHoEdo
The first pass at the outgoing-email form had three problems. The provider cards were bordered text boxes that did not read as clickable. They are now real buttons carrying the provider's logo above its name, with hover, focus-visible and active states. The first field said only "Name" and nothing said whose name. It is now "Account name" with a hint saying it is malmo's own label for the account and only the admin sees it. Every other input gained a real label; no field relies on a placeholder to say what it is. Two columns made the field pairing look meaningful when it was not, so fields are one per line. Logos are bundled rather than fetched: a box may have no internet, and the dashboard must not call a CDN on render. Single-path monochrome marks in currentColor, so they inherit the olive tokens. Postmark, SMTP2GO and Google Workspace have no mark available and fall back to a lettermark tile; custom is not a brand and gets the Lucide server glyph. Paths come from simple-icons, whose icon data is CC0. Styling reuses the repo's idioms instead of new ad-hoc classes: the shared Button component for every action, and the labelled-stack field pattern from CustomInstallView. "Advanced settings" is now "Server settings", which says what is inside it. Claude-Session: https://claude.ai/code/session_01XHrpFqkLosCXFbKeCHoEdo
Adding an account was two states on one page, so the browser Back button did nothing where it looked like it should. The two steps are now two routes: /settings/mail/add picks a provider, /settings/mail/add/:preset fills in the rest. The picked preset is read from the route rather than held in a ref, which is what makes Back work. Field values stay out of the URL — a credential must never land in browser history — so a reload re-seeds the form from the preset. A :preset naming nothing falls back to the picker. The split moved the form rules into src/mailProviderForm.ts. The add form and the inline edit form have to agree field for field, and they now do so by construction rather than by two copies staying in step. New POST /api/v1/mail-providers/verify checks a config the admin has not saved yet: it takes the same body as create rather than an id, so a provider that cannot connect never becomes an account someone has to find and delete. It connects, does STARTTLS, authenticates and hangs up, sending nothing. The add form runs it by default behind a "Test configuration when adding" checkbox, before the elevation prompt. It shares the dial-and-auth path with the test-send (connectMail) rather than approximating it, so the check cannot drift from a real send; it stops before MAIL FROM. That is also its limit: it cannot prove the provider accepts the from address, which is why the per-row test-send stays. Sending the check mail to a malmo-owned address was rejected — a sandboxed SES or Mailgun account may only send to verified recipients, so a fixed address would fail for the admins whose config is fine, and bounces would spend the user's sender reputation. dialError no longer prefixes "connect:", which was rendering as "could not connect: connect: ... connect: connection refused". The type carries that meaning now that errors.As matches it, not the string. Logos are real colour marks in web-ui/src/assets/mail-providers/, matched by preset id, so adding one is adding a file. The slot is a fixed-height box with object-contain, never a fixed square, because the folder holds both square icons and wide wordmarks. Missing marks fall back to a lettermark tile. An empty account list now shows an empty state with the paper airplane and the Add account button, instead of a bare line of text. Claude-Session: https://claude.ai/code/session_01XHrpFqkLosCXFbKeCHoEdo
Account, Activity, Installed app detail and Users each hand-rolled the same bordered pill with their own utility classes. They now use the shared <Button> component, so the settings surfaces pick up one set of hover, focus-visible and disabled states instead of four near-copies. No behaviour change. Claude-Session: https://claude.ai/code/session_01XHrpFqkLosCXFbKeCHoEdo
architecture.md's per-package table had no internal/mailpreset row. That table is the as-built map the rest of the docs tell you to trust over any other package list, so a new package missing from it is the bug the rule exists to prevent. web-ui.md's folder tree was missing mailProviderForm.ts, MailProviderLogo.vue and both outgoing-email views, with a pointer to the assets/mail-providers README so adding a logo is findable from the code map. OutgoingEmailSection.vue had been missing there since #122, not just since this branch. CLAUDE.md's internal/ list gains mailpreset. That list is explicitly non-authoritative, but it costs one word to keep it right. Claude-Session: https://claude.ai/code/session_01XHrpFqkLosCXFbKeCHoEdo
Em dashes read as a pause most people have to re-parse. Every user-visible string on the outgoing-email pages and in the install dialog now uses a full stop or a colon instead. Two of the strings live on the Go side: the blocked-port hint the brain appends to a failed connection, and the body of the test email itself. No behaviour changes, and no test asserts on any of the reworded strings. Claude-Session: https://claude.ai/code/session_01XNeparFYrzXctmyE51aqAE
The install dialog listed accounts as bare radio buttons with the account
name next to them. An account name is whatever the admin typed ("Work
mail"), so the list never said which provider actually sends the mail. It
is now a stack of cards: the provider logo, the account name, the whole
card as the hit target, and an outline on the selected one. "None" leads
and stays a normal choice.
This needed one field on the wire. MailProviderOption, the id+label shape
that the install plan and GET /api/v1/mail-providers/options both return,
gains provider_type. It names the preset a logo is looked up by, carries
no host and no credential, and so is safe on both non-admin surfaces.
Claude-Session: https://claude.ai/code/session_01XNeparFYrzXctmyE51aqAE
…rname Two holes an automated review found. Both fail quietly. The provider list comes from the server, and "Custom SMTP server" is one of its entries, so a failed GET /api/v1/mail-presets left an empty grid with no message and nothing to click. A deep link into /settings/mail/add/<preset> was worse: it sat on "Loading…" forever, because the unknown-preset redirect only fires once the list has arrived. Both paths now say the list could not be loaded and offer a retry. For a same_as_password preset (Postmark) the username is the server token. The Advanced username field let an admin edit it on its own: harmless on the add form, where it is overwritten, but on the edit form a blank password means "keep the stored one", so the account was saved with a username the credential no longer matches and started failing AUTH with nothing in the UI to explain it. The field is now shown but disabled for that mode. A fixed preset (SendGrid's literal apikey) stays editable, because there the value is a real one an admin might need to override. Claude-Session: https://claude.ai/code/session_01XNeparFYrzXctmyE51aqAE
Vite watched the UI; nothing watched the Go side. A brain edit only reached the running process on the next make dev, and the symptom was never obvious. A route added minutes ago answers 405. A new response field arrives undefined. It cost more time to diagnose than the rebuild costs to run. dev/dev-go.sh now runs the fake host-agent and the brain and rebuilds both when a .go file changes. Three choices worth knowing: - The debounce is 10 seconds, not the usual fraction of one. Most edits here arrive from a coding agent, which writes a burst of files over several seconds; a short debounce would restart the brain in the middle of one, again and again. MALMO_DEV_DEBOUNCE=2 for hand editing. - The build goes to .dev/next/ first and only a build that succeeded stops anything, so a typo costs a "[watch] build failed" line, not the stack. Staging also avoids "text file busy": Linux will not write over a running executable. - Shutdown sends TERM, waits up to 5 seconds, then KILLs, so a wedged child cannot hang the Ctrl-C that got you there. Needs GNU find for -newermt, so Linux and WSL. Elsewhere it prints one line and runs exactly as before. Claude-Session: https://claude.ai/code/session_01XNeparFYrzXctmyE51aqAE
| /> | ||
| </div> | ||
|
|
||
| <div v-if="!editPreset || editPreset.username_mode === 'user'" class="space-y-1.5"> |
There was a problem hiding this comment.
When /mail-presets is unavailable while an admin edits a Postmark account, editPreset is undefined, so this branch exposes the username and syncSameAsPassword does nothing. Saving a changed username with a blank password replaces the username while preserving the old token, causing later SMTP authentication to fail.
Knowledge Base Used: Access, setup, and system management API
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.
Closes #426.
What
Adding an email account meant typing seven fields the admin had to look up in their provider's docs. For any provider worth presetting, four of them are constants. Settings → Outgoing email is now a two-step add: pick the provider, then supply only the credential, the from address, a username where the provider does not fix one, and a region for the two providers whose region changes the host.
Host, port and encryption are prefilled behind an Advanced settings disclosure — editable, so a non-standard endpoint is never trapped.
customrenders as that disclosure open with nothing prefilled: today's form.Nine presets in a new leaf package
internal/mailpreset, served admin-only atGET /api/v1/mail-presets. Every one is STARTTLS on a port a hosted box can reach — 587, except SMTP2GO's 2525. Hosted blocks 25 and 465 (SERVICE_PROVISIONING.md, measured in #425), so no preset may default to implicit TLS.Two of the issue's constants were wrong
Each host, port and username rule was checked against that provider's own docs:
smtp.eu.mailgun.org— a prefix, not a region code — so the issue's single{region}host template could not serve both it and SES. Each region option now names the host it resolves to. One mechanism, no substitution.xxx@smtp-brevo.com, not the account email. Help text, not structure, but exactly the kind of thing that turns into a failed test-send.The rest
mail_providers.provider_typevia the idempotent ALTER path,DEFAULT 'custom'. NoCHECK(one cannot ride an ALTER), validated in Go likescopeandexposure. Old rows load ascustom, which is right — they were typed by hand. Kept for the credential broker, not for the form.internal/lifecycle/mail.go, the install plan and the binding endpoints are untouched on purpose.DECISIONS.md2026-08-27 (D1–D4),SERVICE_PROVISIONING.md,SETTINGS.md, progress entry + index, OpenAPI + TS types regenerated.Verification
make test-nopam,gofmt,go vet,make openapi-checkandmake check-webare green.make checkdid not run end to end: this machine cannot buildmsteinert/pam(C.RTLD_NEXT), which fails identically on unmodifieddev.No live test-send was performed against any provider. That is the issue's real acceptance gate — a hostname or username rule can be wrong in a way no unit test sees — and it needs a provisioned hosted box plus a real account at each of the eight providers. Remaining gaps are in the progress entry.
https://claude.ai/code/session_01XHrpFqkLosCXFbKeCHoEdo