refactor(email): registry-pattern provider architecture + code-review fixes#573
Merged
zbigniewsobiecki merged 2 commits intodevfrom Feb 27, 2026
Merged
Conversation
…ider architecture Decomposes EmailClient into clean, extensible layers: - EmailProvider interface — runtime ops (search/read/send/reply/mark) - EmailIntegration interface — credential resolution + AsyncLocalStorage scoping - EmailIntegrationRegistry — singleton, populated at import time (mirrors pm/index.ts) - ImapEmailProvider — password-auth via imapflow + nodemailer SMTP - GmailEmailProvider — OAuth-auth via imapflow IMAP + Gmail REST API send - ImapIntegration / GmailIntegration — per-provider DB credential resolution Code-review fixes applied to gadget core files: - Hoist const message before logger.error (was extracted twice in all 5 files) - Add (no body) fallback in readEmail for attachment-only emails - Guard empty accepted list in sendEmail + replyToEmail (misleading success message) - Remove embedded \n from searchEmails header (caused double blank line in output) - Log logger.warn in ImapIntegration when IMAP/SMTP port parses as NaN New and expanded test coverage: - context.test.ts: AsyncLocalStorage scoping + getEmailProvider throws outside scope - imap/adapter.test.ts: readEmail (success + not-found) + replyToEmail (threading, cleanup) - gmail/adapter.test.ts: searchEmails + readEmail tests; mocks migrated to vi.hoisted() - integration.test.ts: registry delegation, credential fallback, warn path Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Production callers of withEmailIntegration / hasEmailIntegration were importing directly from email/integration.js, which bypasses the side-effect registration in email/index.ts that populates the emailRegistry with ImapIntegration and GmailIntegration. Without the registry populated, emailRegistry.getOrNull() always returns null, causing withEmailIntegration to run without scoping a provider (email gadgets fail) and hasEmailIntegration to always return false. Fix: update the four production callers and the integration test to import from email/index.js (same pattern as PM callers importing from pm/index.js). Files changed: - src/triggers/shared/integration-validation.ts - src/triggers/shared/manual-runner.ts - src/triggers/github/webhook-handler.ts - src/pm/webhook-handler.ts - tests/integration/integration-validation.test.ts Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
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.
Summary
Replaces the monolithic
EmailClientclass with a clean, extensible registry-based architecture modelled after the existing PM integration pattern.Architecture changes
EmailProviderinterface — runtime operations (searchEmails / readEmail / sendEmail / replyToEmail / markEmailAsSeen)EmailIntegrationinterface — DB credential resolution + AsyncLocalStorage provider scoping per projectEmailIntegrationRegistry— singleton populated at import time viasrc/email/index.ts(same pattern aspm/index.ts)ImapEmailProvider— password-auth via imapflow (IMAP) + nodemailer (SMTP)GmailEmailProvider— OAuth-auth via imapflow (IMAP) + Gmail REST API (send) — avoids SMTP 465 blocking in containersImapIntegration/GmailIntegration— resolve credentials from DB, scope provider viawithEmailProviderAdding a new email provider now requires only: a new
EmailProviderclass + oneemailRegistry.register()call.Code-review fixes (gadget core + integration)
src/gadgets/email/core/*.tsconst messagebeforelogger.error— was extracted twicereadEmail.tselse { lines.push('', '(no body)') }fallback for attachment-only emailssendEmail.ts+replyToEmail.tsacceptedlist — was emitting"Email sent successfully to "searchEmails.ts\nfrom header line — caused double blank line in outputimap/integration.tslogger.warnwhen IMAP/SMTP port is non-numeric — was silently returningnullTest coverage
context.test.ts— AsyncLocalStorage scoping,getEmailProviderthrows outside scopeimap/adapter.test.ts— addsreadEmail(success + not-found) andreplyToEmail(threading headers,finallycleanup)gmail/adapter.test.ts— addssearchEmails+readEmailtests; migrates mocks tovi.hoisted()for correctnessintegration.test.ts— registry delegation, credential fallback, warn pathTest plan
npm test— 3430 tests passnpm run typecheck— zero errorsnpm run lint— zero warnings🤖 Generated with Claude Code