feat: email composer overhaul + reliability fix#197
Merged
Systemsaholic merged 26 commits intomainfrom Apr 14, 2026
Merged
Conversation
Shared EmailComposer with TipTap editor, CRM contact search tokens, template picker with variable resolution, file attachments + trip documents, and signature preview. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Addresses 6 Codex findings: 1. SSRF fix: attachments use storagePath, not URLs 2. Template render uses existing getTemplateBySlug/renderTemplate 3. Template picker adds preview-before-insert + trip picker fallback 4. Attachment upload endpoint added to file structure 5. EmailComposer uses TipTapEditor wrapper (not inline reimplementation) 6. Trip page wiring uses primaryContact (not trip.contacts) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nsions Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ring, and render endpoint Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… via StorageService Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…Trip/Contact views Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Trip page: Email button before Preview, pre-fills primaryContact + trip name - Contact page: Mail icon next to email, pre-fills contact email - Both render ComposePanel (slide-out Sheet) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1. Fix template service type error (match[1] undefined check) 2. Add POST /email-accounts/:id/attachments upload endpoint 3. Validate storagePath is agency-scoped before download (prevent cross-tenant access) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Search was only matching firstName/lastName/email/phone. Added preferredName and legalFirstName since displayName is computed from these — contacts were invisible to search when their display name came from preferredName. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…stead of importing TripsModule The forwardRef(() => TripsModule) created a circular dep chain: contacts.module → email.module → email-accounts.module → trips.module → contacts.module Fix: provide StorageService + StorageProviderFactory directly as providers. Also adds preferredName/legalFirstName to contact search. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…a circular dep StorageProviderFactory → CredentialResolverService creates a deep dependency chain that can't be provided in EmailAccountsModule without importing TripsModule (which creates a circular dep). Attachment upload/download stubbed for Phase 2 when a dedicated StorageModule is extracted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ImapFlow download() can return undefined/empty when UID doesn't exist or mailbox is empty. Without guard, for-await throws "Cannot read properties of undefined (reading Symbol.asyncIterator)". Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…le flood Browser can't resolve cid: URIs, causing ERR_UNKNOWN_URL_SCHEME errors and triggering massive React re-render cascades from image error events. Replace with display:none placeholder until CID image resolution is implemented. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ing them Users need to know embedded images exist even if we can't display them yet. Shows a small dashed-border box with "Embedded Image" text so nothing is silently lost. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Eager body parsing during sync — fetch source with metadata, parse with postal-mime, store complete email in DB. No more lazy loading failures. Batch size reduced to 25. CID images deferred to follow-up. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…d lazy fetch Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…al-mime, store in DB Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…Loading... Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Existing emails with null bodyHtml get their body filled when re-synced. COALESCE preserves existing body content — only fills if currently null. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR establishes a comprehensive email composer overhaul by introducing Tiptap-based rich-text editing, adding new shared composer components (EmailComposer, RecipientTokenInput, TemplatePicker, AttachmentBar), integrating the composer across Contact and Trip pages via ComposePanel, extending backend DTOs and endpoints for attachments and template rendering, and refining IMAP sync behavior for improved email body handling. Changes
Sequence DiagramsequenceDiagram
actor User
participant ComposePanel as ComposePanel/<br>ComposeDialog
participant EmailComposer as EmailComposer
participant TipTapEditor as TipTap Editor<br>& Toolbar
participant TemplatePicker as Template<br>Picker
participant AttachmentBar as Attachment<br>Bar
participant Backend as Backend API
participant Storage as R2 Storage
User->>ComposePanel: Opens composer (via Trip/Contact/Inbox)
ComposePanel->>EmailComposer: Render with compose state
EmailComposer->>TipTapEditor: Initialize editor + render toolbar
alt User selects template
User->>TemplatePicker: Click template button
TemplatePicker->>Backend: POST /email-templates/:slug/render
Backend-->>TemplatePicker: subject, bodyHtml, unresolvedVariables
TemplatePicker->>EmailComposer: onInsert callback (subject + body)
EmailComposer->>TipTapEditor: Update editor content
end
alt User uploads attachment
User->>AttachmentBar: Select file from computer
AttachmentBar->>Backend: POST /email-accounts/:id/attachments (multipart)
Backend->>Storage: Store file in R2
Storage-->>Backend: storagePath
Backend-->>AttachmentBar: { storagePath, filename, size }
AttachmentBar->>EmailComposer: Update attachments list
end
User->>EmailComposer: Click Send
EmailComposer->>Backend: useSendEmail().mutate({ to, cc, bcc, subject, bodyHtml, attachments })
Backend->>Storage: Fetch attachment content from R2 (storagePath)
Storage-->>Backend: Binary content
Backend->>Backend: Attach to Nodemailer envelope
Backend->>Backend: Send SMTP
Backend-->>EmailComposer: onSuccess
EmailComposer->>ComposePanel: closeCompose()
ComposePanel->>User: Dismiss panel/dialog
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related Issues
Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/admin/src/lib/sanitize-email-html.ts (1)
47-64:⚠️ Potential issue | 🟠 MajorMove CID rewriting before the trusted/all-images early return.
Line 47 returns before Line 51 runs. When
allowAllImagesorsenderTrustedis true,cid:URLs are left untouched, so embedded images still render as broken/missing instead of placeholders.Suggested fix
- if (options?.allowAllImages || senderTrusted) { - return { html: doc.body.innerHTML, hasBlockedImages: false } - } - - // Replace cid: inline images with visible placeholder (browser can't resolve cid: URIs) - doc.querySelectorAll('img[src^="cid:"]').forEach((img) => { - img.setAttribute('src', 'data:image/svg+xml,' + encodeURIComponent( + // Replace cid: inline images with visible placeholder (browser can't resolve cid: URIs) + doc.querySelectorAll('img[src]').forEach((img) => { + const src = img.getAttribute('src') || '' + if (!/^cid:/i.test(src)) return + img.setAttribute('src', 'data:image/svg+xml,' + encodeURIComponent( '<svg xmlns="http://www.w3.org/2000/svg" width="120" height="80" viewBox="0 0 120 80">' + '<rect width="120" height="80" fill="#f3f4f6" rx="4"/>' + '<text x="60" y="36" text-anchor="middle" font-family="sans-serif" font-size="10" fill="#9ca3af">Embedded</text>' + '<text x="60" y="50" text-anchor="middle" font-family="sans-serif" font-size="10" fill="#9ca3af">Image</text>' + '</svg>' )) img.setAttribute('alt', '[Embedded image — not yet supported]') img.removeAttribute('width') img.removeAttribute('height') img.setAttribute('style', 'max-width:120px;max-height:80px;border:1px dashed `#d1d5db`;border-radius:4px;') }) + + if (options?.allowAllImages || senderTrusted) { + return { html: doc.body.innerHTML, hasBlockedImages: false } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/lib/sanitize-email-html.ts` around lines 47 - 64, The early return triggered by options?.allowAllImages or senderTrusted skips the CID rewriting, leaving cid: images broken; move the doc.querySelectorAll('img[src^="cid:"]').forEach(...) block so it runs before the early return (i.e., always rewrite cid: images to the SVG placeholder), or run that snippet unconditionally regardless of the return condition, then return { html: doc.body.innerHTML, hasBlockedImages: false } as before; update references in the sanitize function where options?.allowAllImages, senderTrusted, and the cid rewriting appear.apps/api/src/email-accounts/imap-sync.service.ts (1)
678-686:⚠️ Potential issue | 🟠 MajorUnconditional source download defeats the 1 MB size threshold.
source: truein the fetch call (line 684) downloads the full RFC822 payload for every message in the range. The size check at lines 813-818 only guards the parsing step—the network transfer and memory cost already occurred. To enforce the threshold effectively, fetch metadata (size) first withoutsource: true, then conditionally download source only for messages under 1 MB.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/email-accounts/imap-sync.service.ts` around lines 678 - 686, The fetch currently includes source: true in the initial client.fetch(fetchRange, ...) which unconditionally downloads full RFC822 payloads; change the flow to first perform a metadata-only fetch (remove source: true) to retrieve sizes, then for each message whose msg.size is below the 1 MB threshold call a second fetch for that individual UID with source: true to download the payload; use the same envelope/bodyStructure/flags/uid/internalDate/size options and keep the { uid: true } fetch option when doing the conditional per-UID fetch, and preserve the existing size-check/parse logic (the code that inspects msg.size before parsing) so network transfer is avoided for large messages.
🧹 Nitpick comments (7)
apps/api/src/email-accounts/dto/email-attachment.dto.ts (2)
4-6: Consider adding@IsNotEmpty()forfilename.
@MaxLength(255)doesn't prevent empty strings. An empty filename would pass validation but is semantically invalid for an attachment.🛡️ Proposed fix
+import { IsString, IsOptional, IsNumber, MaxLength, IsNotEmpty } from 'class-validator' -import { IsString, IsOptional, IsNumber, MaxLength } from 'class-validator' export class EmailAttachmentDto { `@IsString`() + `@IsNotEmpty`() `@MaxLength`(255) filename!: string🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/email-accounts/dto/email-attachment.dto.ts` around lines 4 - 6, Add a non-empty check to the EmailAttachment DTO by decorating the filename property with `@IsNotEmpty`() so empty strings are rejected; update the imports to include IsNotEmpty from class-validator and apply it alongside the existing `@IsString`() and `@MaxLength`(255) on the filename property in the email attachment DTO (filename) to ensure it cannot be empty.
15-17: Consider adding@Min(0)forsize.A negative file size would pass validation but is semantically invalid. Adding a minimum constraint ensures data integrity.
🛡️ Proposed fix
+import { IsString, IsOptional, IsNumber, MaxLength, Min } from 'class-validator' `@IsOptional`() `@IsNumber`() + `@Min`(0) size?: number🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/email-accounts/dto/email-attachment.dto.ts` around lines 15 - 17, The size property currently has `@IsOptional`() and `@IsNumber`() but allows negative values; add the `@Min`(0) decorator to the size field (in email-attachment.dto.ts on the size property) so validation enforces non-negative file sizes—keep the existing `@IsOptional`() and `@IsNumber`() decorators and import Min from class-validator if not already imported.apps/admin/src/components/email-composer/signature-preview.tsx (1)
7-7: Avoidas anycast—add proper typing.Casting
emailSignatureConfigasanybypasses type safety. Consider defining an interface for the signature config shape.♻️ Proposed fix
+interface EmailSignatureConfig { + enabled?: boolean + signatureHtml?: string +} export function SignaturePreview() { const { data: profile } = useMyProfile() - const signatureConfig = profile?.emailSignatureConfig as any + const signatureConfig = profile?.emailSignatureConfig as EmailSignatureConfig | undefined🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/components/email-composer/signature-preview.tsx` at line 7, signatureConfig is currently cast with `as any`, which loses type safety; define a proper interface (e.g., EmailSignatureConfig) describing the expected shape (fields like name, title, phone, avatarUrl, etc.) and replace the `as any` cast by typing signatureConfig as EmailSignatureConfig | undefined (or narrow profile to include emailSignatureConfig: EmailSignatureConfig). Update the component/prop types (e.g., SignaturePreview props or the profile type) to use EmailSignatureConfig so callers and usage sites (signatureConfig) are type-checked and adjust any downstream accesses to match the new interface.apps/admin/src/components/email-composer/recipient-token-input.tsx (2)
177-185: ThesetTimeoutblur delay is fragile.The 200ms delay allows dropdown clicks to register before blur commits the input. On slower devices or under load, this timing may be insufficient. The
onMouseDown={(e) => e.preventDefault()}at line 252 is the correct fix for preventing blur—consider removing the timeout entirely and relying solely onpreventDefault.♻️ Simplified blur handler
function handleBlur() { - // Delay to allow dropdown click to register first - setTimeout(() => { - setShowDropdown(false) - if (inputValue.trim()) { - commitInputValue(inputValue) - } - }, 200) + setShowDropdown(false) + if (inputValue.trim()) { + commitInputValue(inputValue) + } }Since
onMouseDown={(e) => e.preventDefault()}on dropdown options already prevents the blur from firing when clicking an option, the timeout is redundant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/components/email-composer/recipient-token-input.tsx` around lines 177 - 185, The blur handler handleBlur currently uses a fragile setTimeout(200) to delay closing the dropdown and committing input; remove the timeout and make handleBlur run synchronously (call setShowDropdown(false) and, if inputValue.trim(), commitInputValue(inputValue) immediately). Rely on the existing onMouseDown={(e) => e.preventDefault()} on the dropdown option elements to prevent blur when clicking options; ensure those option elements keep the onMouseDown preventDefault behavior so clicks on options don't trigger blur and no timing delay is needed.
262-264: Guard against null/undefined when computing avatar initial.If both
contact.displayNameandcontact.emailare null/undefined,charAt(0)on'?'works, but the nullish coalescing could be clearer. However, the bigger issue is thatcontact.emailcould be null (the check exists at line 115), yet contacts without email are still shown in the dropdown—consider filtering them out.♻️ Filter contacts without email from dropdown
const contacts: ContactListItemDto[] = - debouncedSearch && contactsData?.data ? contactsData.data : [] + debouncedSearch && contactsData?.data + ? contactsData.data.filter((c) => c.email) + : []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/components/email-composer/recipient-token-input.tsx` around lines 262 - 264, The avatar-initial computation using (contact.displayName ?? contact.email ?? '?').charAt(0).toUpperCase() should be made defensive and the dropdown should exclude contacts without an email: first, compute the initial by coalescing to a non-null string (e.g., const label = contact.displayName || contact.email || '?') and then call label.charAt(0).toUpperCase() so charAt is never called on null/undefined (referencing contact.displayName, contact.email and the current charAt usage). Second, filter the contacts source before rendering the dropdown items to remove entries where contact.email is falsy (where the list is mapped to render tokens/dropdown items), ensuring contacts without email are not shown in the dropdown. Ensure these changes are applied in the recipient-token-input component where contacts are mapped/rendered.apps/api/src/email/email-templates.controller.ts (1)
249-253: Add a validated DTO for the request body.The inline body type bypasses
class-validator. ThetripIdandcontactIdshould be validated as UUIDs, andvariableskeys should be constrained to prevent the ReDoS issue flagged inEmailTemplatesService.renderWithContext.♻️ Proposed DTO
Create a new DTO (e.g.,
RenderTemplateDto):import { IsOptional, IsUUID, IsObject } from 'class-validator' export class RenderTemplateDto { `@IsOptional`() `@IsUUID`() tripId?: string `@IsOptional`() `@IsUUID`() contactId?: string `@IsOptional`() `@IsObject`() variables?: Record<string, string> }Then update the controller:
- `@Body`() body: { tripId?: string; contactId?: string; variables?: Record<string, string> }, + `@Body`() body: RenderTemplateDto,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/email/email-templates.controller.ts` around lines 249 - 253, Create a new DTO class RenderTemplateDto and use it in the controller signature (replace the inline body type in renderWithContext with `@Body`() body: RenderTemplateDto) so class-validator runs; validate tripId and contactId with `@IsOptional`() and `@IsUUID`(), and validate variables as an optional object of string values with a safe key constraint (e.g., enforce keys match a conservative regex and limit key/value lengths) to prevent the ReDoS issue referenced in EmailTemplatesService.renderWithContext; update any callers or the EmailTemplatesService.renderWithContext parameter typing to accept the new DTO shape.apps/admin/src/components/email-composer/compose-dialog.tsx (1)
18-21: This still renders as a bounded modal, not the inbox full-screen composer.
max-w-3xl h-[80vh]keeps the editor in a centered modal-sized surface. If Inbox is supposed to use the full-screen variant while Trip/Contact use the sheet, this class set should be updated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/components/email-composer/compose-dialog.tsx` around lines 18 - 21, The DialogContent currently forces a centered modal via "max-w-3xl h-[80vh]" causing Inbox to render as a modal; update the ComposeDialog to choose classes based on compose?.source (or similar) so Inbox uses full-screen classes (e.g., full width/height: w-screen h-screen or w-full h-full and remove max-w-3xl/h-[80vh]) while Trip/Contact keep the sheet/modal classes ("max-w-3xl h-[80vh]"). Change the className on DialogContent accordingly and keep EmailComposer className="h-full" unchanged so the editor fills the selected container.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/admin/src/app/contacts/`[id]/page.tsx:
- Around line 546-559: The icon-only compose Button (the Button JSX wrapping the
Mail icon that calls useEmailStore.getState().openCompose) lacks an accessible
name; add an aria-label (e.g., aria-label="Compose email to {contact.displayName
|| contact.email}") or include visually-hidden text inside the Button so screen
readers announce the action, ensuring the label references the contact when
possible to provide clear context.
In `@apps/admin/src/components/email-composer/compose-panel.tsx`:
- Around line 3-15: ComposePanel currently returns null when no account is
resolved, leaving compose latched; update ComposePanel to handle the no-account
state by detecting when compose is true but accountId is null and then either
(a) render a simple user-visible fallback UI (e.g., a small panel or message
with a "Close" action) that explains "No email account configured" and calls
closeCompose on dismiss, or (b) programmatically call closeCompose and surface
an error/toast. Use the existing symbols: ComposePanel component, compose and
closeCompose from useEmailStore, and accountId derived from useEmailAccounts to
implement this behavior so openCompose no longer leaves compose stuck when
accounts are absent.
In `@apps/admin/src/components/email-composer/composer-toolbar.tsx`:
- Around line 163-192: The toolbar renders both utility buttons unconditionally,
which leads to dead controls when callbacks like onAttachClick or
onTemplateClick are not provided; update the ComposerToolbar
(composer-toolbar.tsx) to conditionally render each Tooltip/Button block only if
its handler prop (onAttachClick, onTemplateClick) is defined (or alternatively
make those props required in the component's props type), e.g., wrap the
Paperclip/Button block in a guard checking onAttachClick and the FileText/Button
block in a guard checking onTemplateClick so buttons only appear when their
handlers are wired.
In `@apps/admin/src/components/email-composer/email-composer.tsx`:
- Around line 114-123: handleTemplateInsert currently overwrites any existing
subject (e.g., compose.prefillSubject) when a template provides one; change the
logic to only setSubject(result.subject) if there is no existing subject (check
that subject is falsy) and include subject in the hook dependency array so the
closure sees current state. Keep the editor.content behavior unchanged
(editor.commands.setContent(result.bodyHtml)).
- Around line 313-345: The loop checks the 10 MB cap against the closed-over
attachments state, causing multiple files selected in one picker to bypass the
limit; fix it by computing a running total before iterating (e.g., let
runningTotal = attachments.reduce(...)) and then inside the for (const file of
Array.from(files)) loop check runningTotal + file.size > 10 * 1024 * 1024, alert
and break if true, otherwise add file.size to runningTotal and proceed with the
existing upload flow that calls api.postFormData and setAttachments so the
per-session accepted files are accounted for correctly.
- Around line 346-348: The catch block in the attachment upload handler
currently only does console.error; import and use useToast and Sentry (as done
in use-emails.ts) and replace the console.error with a toast and Sentry capture:
call toast({ title: 'Upload failed', description: (err as Error).message,
variant: 'destructive' }) and call Sentry.captureException(err) so the error is
reported (the Sentry environment is already set via
NEXT_PUBLIC_SENTRY_ENVIRONMENT). Update the catch in the upload handler to use
these symbols (useToast, toast, Sentry.captureException) and ensure the import
statements for useToast and Sentry are added at the top of the file.
In `@apps/admin/src/components/email-composer/signature-preview.tsx`:
- Around line 16-19: The rendered signatureHtml is unsafe and must be sanitized
before use; import and call the existing sanitizeEmailHtml from
apps/admin/src/lib/sanitize-email-html.ts and replace the value passed to
dangerouslySetInnerHTML with the sanitized output (e.g., const sanitized =
sanitizeEmailHtml(signatureHtml)) in the SignaturePreview component, keeping the
original buildSignatureHtml usage but ensuring names/tagline/microSiteUrl are
cleaned before rendering and handling empty/null safely.
In `@apps/admin/src/components/email-composer/template-picker.tsx`:
- Around line 115-119: The preview is injecting previewResult.bodyHtml via
dangerouslySetInnerHTML in the template-picker component without sanitization;
update the render to sanitize the HTML with DOMPurify (imported from
'dompurify') before passing it to dangerouslySetInnerHTML (follow the same
sanitization pattern used in email-reader.tsx and email-preview-dialog.tsx),
i.e., call DOMPurify.sanitize(previewResult.bodyHtml) and use that sanitized
string for the __html value so user-editable template HTML cannot introduce XSS.
In `@apps/api/src/email-accounts/email-accounts.controller.ts`:
- Around line 322-339: The uploadAttachment route currently always throws
BadRequestException which breaks the client contract; replace the placeholder by
wiring in the StorageService: in uploadAttachment (and the controller
constructor) remove the throw, call this.emailAccountsService.findOne(id,
auth.userId) as already done, then use an injected StorageService (e.g.,
storageService.upload or storeFile) to persist the provided
file.buffer/file.stream, returning { storagePath, filename: file.originalname,
size: file.size }; ensure errors from StorageService are caught and rethrown as
appropriate HTTP errors and keep the existing ParseFilePipe validators; if
StorageService isn’t available yet, instead remove/expose this route behind a
feature flag or return 404/501 to avoid a permanent 400.
In `@apps/api/src/email-accounts/imap-sync.service.ts`:
- Around line 188-190: The graceful fallback branches that currently only log a
warning (the check on downloadResult?.content that calls this.logger.warn for
UID ${email.imapUid} and the caught-but-swallowed message parsing error) should
be reported to Sentry before returning; update the ImapSyncService logic to call
captureException() with the enriched error/context (include email.imapUid,
email.folder, and any parsing details) or re-throw an enriched error instead of
silently returning nulls, following the same pattern used in the error handling
at the enrichment/rethrow site around the existing pattern (see the enrichment +
re-throw at line ~527) so these runtime failures are captured by Sentry.
In `@apps/api/src/email-accounts/smtp-send.service.ts`:
- Around line 122-128: The current attachments check in smtp-send.service.ts
silently drops attachments (see dto.attachments and this.logger.warn) — change
this to either (preferred) reject the request by throwing a proper HTTP error
(e.g., BadRequestException or a domain-specific error) that includes a clear
message and the dto.attachments.length, so the caller/UI knows sending failed
due to unimplemented attachments; or (alternate) keep sending but change the
method's response contract to include explicit metadata (e.g.,
attachmentsSkipped: true and skippedCount) so the UI can surface a warning.
Update the logic in the same attachments check block (replace this.logger.warn)
and ensure the service method signature and API response type are adjusted
accordingly if you choose the metadata option.
In `@apps/api/src/email/email-templates.service.ts`:
- Around line 292-299: The RegExp construction using unescaped user-controlled
keys in the manual override loop (context.variables) can cause ReDoS; update the
code that builds pattern (used to replace in subject and bodyHtml) to escape
regex metacharacters in key before creating new RegExp (e.g., add an
escapeRegExp helper or import a safe utility and call it on key), then build the
RegExp from the escaped string and proceed with subject.replace and
bodyHtml.replace; ensure the helper name (escapeRegExp) and the variables
referenced (context.variables, subject, bodyHtml, pattern) are used so the
change is easy to locate.
In `@docs/superpowers/plans/2026-04-13-email-composer-overhaul.md`:
- Around line 842-843: The verification step uses the piped command "pnpm
--filter `@tailfire/api` exec tsc --noEmit 2>&1 | grep template-picker", which
masks tsc's exit code because grep returns 1 when no match is found; change the
step to either run "pnpm --filter `@tailfire/api` exec tsc --noEmit" directly or
preserve the compiler's exit status before filtering (e.g., capture tsc's exit
code and use it for the task result, or use a pipe approach that preserves
PIPESTATUS/returns the original exit code) so a clean compile does not appear as
a failure.
In `@docs/superpowers/plans/2026-04-13-email-reliability.md`:
- Around line 102-105: The plan's "Step 3: Verify compile" only runs TypeScript
for `@tailfire/admin`, but the change touches apps/api (notably
apps/api/src/email-accounts/imap-sync.service.ts), so update the verification
step to include the API workspace or run a repo-wide typecheck; for example,
replace or extend the command `pnpm --filter `@tailfire/admin` exec tsc --noEmit`
to also run `tsc --noEmit` in the API package or run a top-level `pnpm -w exec
tsc --noEmit` so the API's compile errors are caught during verification.
In `@docs/superpowers/specs/2026-04-13-email-composer-overhaul-design.md`:
- Around line 168-171: Update the backend spec to reflect the storagePath-based
attachment contract: change SendEmailDto's attachments to { filename: string;
storagePath: string; contentType: string }[] (remove URL-based semantics),
update SmtpSendService description to state it reads attachment bytes from the
storagePath (not from publicly fetchable R2 URLs) and attaches them to
Nodemailer via the attachments option, and ensure the documentation for the trip
documents endpoint (GET /trips/:id/documents) specifies it returns R2 file
metadata including storagePath entries so callers can pass storagePath into
SendEmailDto.
---
Outside diff comments:
In `@apps/admin/src/lib/sanitize-email-html.ts`:
- Around line 47-64: The early return triggered by options?.allowAllImages or
senderTrusted skips the CID rewriting, leaving cid: images broken; move the
doc.querySelectorAll('img[src^="cid:"]').forEach(...) block so it runs before
the early return (i.e., always rewrite cid: images to the SVG placeholder), or
run that snippet unconditionally regardless of the return condition, then return
{ html: doc.body.innerHTML, hasBlockedImages: false } as before; update
references in the sanitize function where options?.allowAllImages,
senderTrusted, and the cid rewriting appear.
In `@apps/api/src/email-accounts/imap-sync.service.ts`:
- Around line 678-686: The fetch currently includes source: true in the initial
client.fetch(fetchRange, ...) which unconditionally downloads full RFC822
payloads; change the flow to first perform a metadata-only fetch (remove source:
true) to retrieve sizes, then for each message whose msg.size is below the 1 MB
threshold call a second fetch for that individual UID with source: true to
download the payload; use the same
envelope/bodyStructure/flags/uid/internalDate/size options and keep the { uid:
true } fetch option when doing the conditional per-UID fetch, and preserve the
existing size-check/parse logic (the code that inspects msg.size before parsing)
so network transfer is avoided for large messages.
---
Nitpick comments:
In `@apps/admin/src/components/email-composer/compose-dialog.tsx`:
- Around line 18-21: The DialogContent currently forces a centered modal via
"max-w-3xl h-[80vh]" causing Inbox to render as a modal; update the
ComposeDialog to choose classes based on compose?.source (or similar) so Inbox
uses full-screen classes (e.g., full width/height: w-screen h-screen or w-full
h-full and remove max-w-3xl/h-[80vh]) while Trip/Contact keep the sheet/modal
classes ("max-w-3xl h-[80vh]"). Change the className on DialogContent
accordingly and keep EmailComposer className="h-full" unchanged so the editor
fills the selected container.
In `@apps/admin/src/components/email-composer/recipient-token-input.tsx`:
- Around line 177-185: The blur handler handleBlur currently uses a fragile
setTimeout(200) to delay closing the dropdown and committing input; remove the
timeout and make handleBlur run synchronously (call setShowDropdown(false) and,
if inputValue.trim(), commitInputValue(inputValue) immediately). Rely on the
existing onMouseDown={(e) => e.preventDefault()} on the dropdown option elements
to prevent blur when clicking options; ensure those option elements keep the
onMouseDown preventDefault behavior so clicks on options don't trigger blur and
no timing delay is needed.
- Around line 262-264: The avatar-initial computation using (contact.displayName
?? contact.email ?? '?').charAt(0).toUpperCase() should be made defensive and
the dropdown should exclude contacts without an email: first, compute the
initial by coalescing to a non-null string (e.g., const label =
contact.displayName || contact.email || '?') and then call
label.charAt(0).toUpperCase() so charAt is never called on null/undefined
(referencing contact.displayName, contact.email and the current charAt usage).
Second, filter the contacts source before rendering the dropdown items to remove
entries where contact.email is falsy (where the list is mapped to render
tokens/dropdown items), ensuring contacts without email are not shown in the
dropdown. Ensure these changes are applied in the recipient-token-input
component where contacts are mapped/rendered.
In `@apps/admin/src/components/email-composer/signature-preview.tsx`:
- Line 7: signatureConfig is currently cast with `as any`, which loses type
safety; define a proper interface (e.g., EmailSignatureConfig) describing the
expected shape (fields like name, title, phone, avatarUrl, etc.) and replace the
`as any` cast by typing signatureConfig as EmailSignatureConfig | undefined (or
narrow profile to include emailSignatureConfig: EmailSignatureConfig). Update
the component/prop types (e.g., SignaturePreview props or the profile type) to
use EmailSignatureConfig so callers and usage sites (signatureConfig) are
type-checked and adjust any downstream accesses to match the new interface.
In `@apps/api/src/email-accounts/dto/email-attachment.dto.ts`:
- Around line 4-6: Add a non-empty check to the EmailAttachment DTO by
decorating the filename property with `@IsNotEmpty`() so empty strings are
rejected; update the imports to include IsNotEmpty from class-validator and
apply it alongside the existing `@IsString`() and `@MaxLength`(255) on the filename
property in the email attachment DTO (filename) to ensure it cannot be empty.
- Around line 15-17: The size property currently has `@IsOptional`() and
`@IsNumber`() but allows negative values; add the `@Min`(0) decorator to the size
field (in email-attachment.dto.ts on the size property) so validation enforces
non-negative file sizes—keep the existing `@IsOptional`() and `@IsNumber`()
decorators and import Min from class-validator if not already imported.
In `@apps/api/src/email/email-templates.controller.ts`:
- Around line 249-253: Create a new DTO class RenderTemplateDto and use it in
the controller signature (replace the inline body type in renderWithContext with
`@Body`() body: RenderTemplateDto) so class-validator runs; validate tripId and
contactId with `@IsOptional`() and `@IsUUID`(), and validate variables as an
optional object of string values with a safe key constraint (e.g., enforce keys
match a conservative regex and limit key/value lengths) to prevent the ReDoS
issue referenced in EmailTemplatesService.renderWithContext; update any callers
or the EmailTemplatesService.renderWithContext parameter typing to accept the
new DTO shape.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ec24de6-1a59-4f15-a11b-22de3d17c0a4
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (31)
apps/admin/package.jsonapps/admin/src/app/contacts/[id]/page.tsxapps/admin/src/app/emails/inbox/_components/compose-email-dialog.tsxapps/admin/src/app/emails/inbox/_components/email-reader.tsxapps/admin/src/app/emails/inbox/page.tsxapps/admin/src/app/trips/[id]/page.tsxapps/admin/src/components/email-composer/attachment-bar.tsxapps/admin/src/components/email-composer/compose-dialog.tsxapps/admin/src/components/email-composer/compose-panel.tsxapps/admin/src/components/email-composer/composer-toolbar.tsxapps/admin/src/components/email-composer/email-composer.tsxapps/admin/src/components/email-composer/recipient-token-input.tsxapps/admin/src/components/email-composer/signature-preview.tsxapps/admin/src/components/email-composer/template-picker.tsxapps/admin/src/components/email-composer/tiptap-editor.tsxapps/admin/src/hooks/use-email-templates.tsapps/admin/src/hooks/use-emails.tsapps/admin/src/lib/sanitize-email-html.tsapps/admin/src/stores/email.store.tsapps/api/src/contacts/contacts.service.tsapps/api/src/email-accounts/dto/email-attachment.dto.tsapps/api/src/email-accounts/dto/send-email.dto.tsapps/api/src/email-accounts/email-accounts.controller.tsapps/api/src/email-accounts/imap-sync.service.tsapps/api/src/email-accounts/smtp-send.service.tsapps/api/src/email/email-templates.controller.tsapps/api/src/email/email-templates.service.tsdocs/superpowers/plans/2026-04-13-email-composer-overhaul.mddocs/superpowers/plans/2026-04-13-email-reliability.mddocs/superpowers/specs/2026-04-13-email-composer-overhaul-design.mddocs/superpowers/specs/2026-04-13-email-reliability-design.md
💤 Files with no reviewable changes (1)
- apps/admin/src/app/emails/inbox/_components/compose-email-dialog.tsx
Comment on lines
+546
to
+559
| <Button | ||
| variant="ghost" | ||
| size="sm" | ||
| className="h-6 w-6 p-0" | ||
| onClick={() => { | ||
| useEmailStore.getState().openCompose({ | ||
| mode: 'new', | ||
| contactId: contact.id, | ||
| prefillTo: [{ address: contact.email!, name: contact.displayName }], | ||
| }) | ||
| }} | ||
| > | ||
| <Mail className="h-3.5 w-3.5" /> | ||
| </Button> |
There was a problem hiding this comment.
Add an accessible name to this icon-only compose button.
The button has no text alternative, so screen readers won't announce what action it performs.
♿ Suggested fix
<Button
variant="ghost"
size="sm"
className="h-6 w-6 p-0"
+ aria-label={`Compose email to ${contact.displayName || contact.email}`}
+ title="Compose email"
onClick={() => {
useEmailStore.getState().openCompose({
mode: 'new',
contactId: contact.id,
prefillTo: [{ address: contact.email!, name: contact.displayName }],
})
}}
>
- <Mail className="h-3.5 w-3.5" />
+ <Mail className="h-3.5 w-3.5" aria-hidden="true" />
</Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin/src/app/contacts/`[id]/page.tsx around lines 546 - 559, The
icon-only compose Button (the Button JSX wrapping the Mail icon that calls
useEmailStore.getState().openCompose) lacks an accessible name; add an
aria-label (e.g., aria-label="Compose email to {contact.displayName ||
contact.email}") or include visually-hidden text inside the Button so screen
readers announce the action, ensuring the label references the contact when
possible to provide clear context.
Comment on lines
+3
to
+15
| import { Sheet, SheetContent, SheetTitle } from '@/components/ui/sheet' | ||
| import { useEmailStore } from '@/stores/email.store' | ||
| import { useEmailAccounts } from '@/hooks/use-email-accounts' | ||
| import { EmailComposer } from './email-composer' | ||
|
|
||
| export function ComposePanel() { | ||
| const compose = useEmailStore((s) => s.compose) | ||
| const closeCompose = useEmailStore((s) => s.closeCompose) | ||
| const { data: accounts } = useEmailAccounts() | ||
| const accountId = accounts?.[0]?.id ?? null | ||
|
|
||
| if (!compose || !accountId) return null | ||
|
|
There was a problem hiding this comment.
Handle the no-account state instead of silently returning null.
Trip and Contact pages now call openCompose(...) unconditionally. When account resolution yields no account, this branch renders nothing, gives the user no feedback, and leaves compose latched in the store until something else clears it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin/src/components/email-composer/compose-panel.tsx` around lines 3 -
15, ComposePanel currently returns null when no account is resolved, leaving
compose latched; update ComposePanel to handle the no-account state by detecting
when compose is true but accountId is null and then either (a) render a simple
user-visible fallback UI (e.g., a small panel or message with a "Close" action)
that explains "No email account configured" and calls closeCompose on dismiss,
or (b) programmatically call closeCompose and surface an error/toast. Use the
existing symbols: ComposePanel component, compose and closeCompose from
useEmailStore, and accountId derived from useEmailAccounts to implement this
behavior so openCompose no longer leaves compose stuck when accounts are absent.
Comment on lines
+163
to
+192
| {/* Utility buttons */} | ||
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <Button | ||
| variant="ghost" | ||
| size="sm" | ||
| className="h-9 w-9 p-0" | ||
| onClick={onAttachClick} | ||
| aria-label="Attach file" | ||
| > | ||
| <Paperclip className="h-4 w-4" /> | ||
| </Button> | ||
| </TooltipTrigger> | ||
| <TooltipContent>Attach file</TooltipContent> | ||
| </Tooltip> | ||
|
|
||
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <Button | ||
| variant="ghost" | ||
| size="sm" | ||
| className="h-9 w-9 p-0" | ||
| onClick={onTemplateClick} | ||
| aria-label="Use template" | ||
| > | ||
| <FileText className="h-4 w-4" /> | ||
| </Button> | ||
| </TooltipTrigger> | ||
| <TooltipContent>Use template</TooltipContent> | ||
| </Tooltip> |
There was a problem hiding this comment.
Hide optional actions when no handler is wired.
Both utility buttons always render even though their callbacks are optional. In the current composer integration the template button has no handler, so users get a dead control. Render each button conditionally or make the props required.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin/src/components/email-composer/composer-toolbar.tsx` around lines
163 - 192, The toolbar renders both utility buttons unconditionally, which leads
to dead controls when callbacks like onAttachClick or onTemplateClick are not
provided; update the ComposerToolbar (composer-toolbar.tsx) to conditionally
render each Tooltip/Button block only if its handler prop (onAttachClick,
onTemplateClick) is defined (or alternatively make those props required in the
component's props type), e.g., wrap the Paperclip/Button block in a guard
checking onAttachClick and the FileText/Button block in a guard checking
onTemplateClick so buttons only appear when their handlers are wired.
Comment on lines
+114
to
+123
| const handleTemplateInsert = React.useCallback( | ||
| (result: { subject: string; bodyHtml: string }) => { | ||
| if (result.subject) { | ||
| setSubject(result.subject) | ||
| } | ||
| if (result.bodyHtml && editor) { | ||
| editor.commands.setContent(result.bodyHtml) | ||
| } | ||
| }, | ||
| [editor], |
There was a problem hiding this comment.
Don't overwrite an existing subject when inserting a template.
subject is seeded from compose.prefillSubject, but this handler always replaces it whenever the template has a subject. That will clobber reply/forward and trip-prefilled subjects instead of only filling blanks.
♻️ Suggested change
const handleTemplateInsert = React.useCallback(
(result: { subject: string; bodyHtml: string }) => {
- if (result.subject) {
+ if (!subject.trim() && result.subject) {
setSubject(result.subject)
}
if (result.bodyHtml && editor) {
editor.commands.setContent(result.bodyHtml)
}
},
- [editor],
+ [editor, subject],
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleTemplateInsert = React.useCallback( | |
| (result: { subject: string; bodyHtml: string }) => { | |
| if (result.subject) { | |
| setSubject(result.subject) | |
| } | |
| if (result.bodyHtml && editor) { | |
| editor.commands.setContent(result.bodyHtml) | |
| } | |
| }, | |
| [editor], | |
| const handleTemplateInsert = React.useCallback( | |
| (result: { subject: string; bodyHtml: string }) => { | |
| if (!subject.trim() && result.subject) { | |
| setSubject(result.subject) | |
| } | |
| if (result.bodyHtml && editor) { | |
| editor.commands.setContent(result.bodyHtml) | |
| } | |
| }, | |
| [editor, subject], |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin/src/components/email-composer/email-composer.tsx` around lines 114
- 123, handleTemplateInsert currently overwrites any existing subject (e.g.,
compose.prefillSubject) when a template provides one; change the logic to only
setSubject(result.subject) if there is no existing subject (check that subject
is falsy) and include subject in the hook dependency array so the closure sees
current state. Keep the editor.content behavior unchanged
(editor.commands.setContent(result.bodyHtml)).
Comment on lines
+313
to
+345
| const { api } = await import('@/lib/api') | ||
| for (const file of Array.from(files)) { | ||
| const currentTotal = attachments.reduce( | ||
| (sum, a) => sum + (a.size || 0), | ||
| 0, | ||
| ) | ||
| if (currentTotal + file.size > 10 * 1024 * 1024) { | ||
| alert('Total attachment size cannot exceed 10 MB') | ||
| break | ||
| } | ||
|
|
||
| const formData = new FormData() | ||
| formData.append('file', file) | ||
|
|
||
| try { | ||
| const result = await api.postFormData<{ | ||
| storagePath: string | ||
| filename: string | ||
| size: number | ||
| }>( | ||
| `/email-accounts/${accountId}/attachments`, | ||
| formData, | ||
| ) | ||
| setAttachments((prev) => [ | ||
| ...prev, | ||
| { | ||
| filename: result.filename || file.name, | ||
| storagePath: result.storagePath, | ||
| contentType: file.type, | ||
| size: file.size, | ||
| source: 'upload' as const, | ||
| }, | ||
| ]) |
There was a problem hiding this comment.
The 10 MB cap is checked against stale attachment state.
Inside this loop the total is recalculated from the closed-over attachments, so selecting multiple files in one picker session ignores files that were accepted earlier in the same batch. Two 6 MB files will both pass here and push the composer over the advertised limit.
♻️ Suggested change
const { api } = await import('@/lib/api')
+ let runningTotal = attachments.reduce(
+ (sum, a) => sum + (a.size || 0),
+ 0,
+ )
for (const file of Array.from(files)) {
- const currentTotal = attachments.reduce(
- (sum, a) => sum + (a.size || 0),
- 0,
- )
- if (currentTotal + file.size > 10 * 1024 * 1024) {
+ if (runningTotal + file.size > 10 * 1024 * 1024) {
alert('Total attachment size cannot exceed 10 MB')
break
}
const formData = new FormData()
formData.append('file', file)
try {
const result = await api.postFormData<{
storagePath: string
filename: string
size: number
}>(
`/email-accounts/${accountId}/attachments`,
formData,
)
setAttachments((prev) => [
...prev,
{
filename: result.filename || file.name,
storagePath: result.storagePath,
contentType: file.type,
size: file.size,
source: 'upload' as const,
},
])
+ runningTotal += file.size
} catch (err) {
console.error('Upload failed:', err)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const { api } = await import('@/lib/api') | |
| for (const file of Array.from(files)) { | |
| const currentTotal = attachments.reduce( | |
| (sum, a) => sum + (a.size || 0), | |
| 0, | |
| ) | |
| if (currentTotal + file.size > 10 * 1024 * 1024) { | |
| alert('Total attachment size cannot exceed 10 MB') | |
| break | |
| } | |
| const formData = new FormData() | |
| formData.append('file', file) | |
| try { | |
| const result = await api.postFormData<{ | |
| storagePath: string | |
| filename: string | |
| size: number | |
| }>( | |
| `/email-accounts/${accountId}/attachments`, | |
| formData, | |
| ) | |
| setAttachments((prev) => [ | |
| ...prev, | |
| { | |
| filename: result.filename || file.name, | |
| storagePath: result.storagePath, | |
| contentType: file.type, | |
| size: file.size, | |
| source: 'upload' as const, | |
| }, | |
| ]) | |
| const { api } = await import('@/lib/api') | |
| let runningTotal = attachments.reduce( | |
| (sum, a) => sum + (a.size || 0), | |
| 0, | |
| ) | |
| for (const file of Array.from(files)) { | |
| if (runningTotal + file.size > 10 * 1024 * 1024) { | |
| alert('Total attachment size cannot exceed 10 MB') | |
| break | |
| } | |
| const formData = new FormData() | |
| formData.append('file', file) | |
| try { | |
| const result = await api.postFormData<{ | |
| storagePath: string | |
| filename: string | |
| size: number | |
| }>( | |
| `/email-accounts/${accountId}/attachments`, | |
| formData, | |
| ) | |
| setAttachments((prev) => [ | |
| ...prev, | |
| { | |
| filename: result.filename || file.name, | |
| storagePath: result.storagePath, | |
| contentType: file.type, | |
| size: file.size, | |
| source: 'upload' as const, | |
| }, | |
| ]) | |
| runningTotal += file.size | |
| } catch (err) { | |
| console.error('Upload failed:', err) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin/src/components/email-composer/email-composer.tsx` around lines 313
- 345, The loop checks the 10 MB cap against the closed-over attachments state,
causing multiple files selected in one picker to bypass the limit; fix it by
computing a running total before iterating (e.g., let runningTotal =
attachments.reduce(...)) and then inside the for (const file of
Array.from(files)) loop check runningTotal + file.size > 10 * 1024 * 1024, alert
and break if true, otherwise add file.size to runningTotal and proceed with the
existing upload flow that calls api.postFormData and setAttachments so the
per-session accepted files are accounted for correctly.
Comment on lines
+122
to
+128
| if (dto.attachments && dto.attachments.length > 0) { | ||
| // Attachment download requires StorageService which has a deep dependency chain | ||
| // (StorageProviderFactory → CredentialResolverService) that creates circular deps | ||
| // when imported directly into EmailAccountsModule. Attachment sending will be | ||
| // implemented via a dedicated service that can access the storage layer. | ||
| this.logger.warn(`${dto.attachments.length} attachment(s) specified but attachment sending not yet implemented`) | ||
| } |
There was a problem hiding this comment.
Silently dropping attachments may confuse users.
The email is sent without attachments while logging a warning server-side. The sender won't know their attachments weren't included. Consider either:
- Throwing an error to prevent sending until attachments are implemented, or
- Returning metadata indicating attachments were skipped so the UI can warn the user.
💡 Option 1: Prevent send with attachments
if (dto.attachments && dto.attachments.length > 0) {
- this.logger.warn(`${dto.attachments.length} attachment(s) specified but attachment sending not yet implemented`)
+ throw new BadRequestException(
+ 'Attachment sending is not yet implemented. Please send without attachments or wait for the feature to be released.',
+ )
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/email-accounts/smtp-send.service.ts` around lines 122 - 128, The
current attachments check in smtp-send.service.ts silently drops attachments
(see dto.attachments and this.logger.warn) — change this to either (preferred)
reject the request by throwing a proper HTTP error (e.g., BadRequestException or
a domain-specific error) that includes a clear message and the
dto.attachments.length, so the caller/UI knows sending failed due to
unimplemented attachments; or (alternate) keep sending but change the method's
response contract to include explicit metadata (e.g., attachmentsSkipped: true
and skippedCount) so the UI can surface a warning. Update the logic in the same
attachments check block (replace this.logger.warn) and ensure the service method
signature and API response type are adjusted accordingly if you choose the
metadata option.
Comment on lines
+292
to
+299
| // Apply manual variable overrides | ||
| if (context.variables) { | ||
| for (const [key, value] of Object.entries(context.variables)) { | ||
| const pattern = new RegExp(`\\{\\{${key}(?:::.*?)?\\}\\}`, 'g') | ||
| subject = subject.replace(pattern, value) | ||
| bodyHtml = bodyHtml.replace(pattern, value) | ||
| } | ||
| } |
There was a problem hiding this comment.
ReDoS vulnerability: escape key before using in RegExp.
The key variable comes from context.variables (user-controlled input from the request body). A malicious key containing regex metacharacters (e.g., (a+)+$) could cause catastrophic backtracking.
🐛 Proposed fix
+ /**
+ * Escape special regex characters in a string.
+ */
+ private escapeRegex(str: string): string {
+ return str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')
+ }
async renderWithContext(
slug: string,
agencyId: string,
context: { tripId?: string; contactId?: string; agentId?: string; variables?: Record<string, string> },
): Promise<{ subject: string; bodyHtml: string; unresolvedVariables: string[] }> {
// ... existing code ...
// Apply manual variable overrides
if (context.variables) {
for (const [key, value] of Object.entries(context.variables)) {
- const pattern = new RegExp(`\\{\\{${key}(?:::.*?)?\\}\\}`, 'g')
+ const escapedKey = this.escapeRegex(key)
+ const pattern = new RegExp(`\\{\\{${escapedKey}(?:::.*?)?\\}\\}`, 'g')
subject = subject.replace(pattern, value)
bodyHtml = bodyHtml.replace(pattern, value)
}
}🧰 Tools
🪛 ast-grep (0.42.1)
[warning] 294-294: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\{\\{${key}(?:::.*?)?\\}\\}, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/email/email-templates.service.ts` around lines 292 - 299, The
RegExp construction using unescaped user-controlled keys in the manual override
loop (context.variables) can cause ReDoS; update the code that builds pattern
(used to replace in subject and bodyHtml) to escape regex metacharacters in key
before creating new RegExp (e.g., add an escapeRegExp helper or import a safe
utility and call it on key), then build the RegExp from the escaped string and
proceed with subject.replace and bodyHtml.replace; ensure the helper name
(escapeRegExp) and the variables referenced (context.variables, subject,
bodyHtml, pattern) are used so the change is easy to locate.
Comment on lines
+842
to
+843
| Run: `pnpm --filter @tailfire/admin exec tsc --noEmit` | ||
| Run: `pnpm --filter @tailfire/api exec tsc --noEmit 2>&1 | grep template-picker` (verify no new errors from our changes) |
There was a problem hiding this comment.
This verification step reports success as failure.
grep template-picker exits with status 1 when tsc --noEmit is clean, so an agent following this step will fail the task on a successful compile. Run tsc --noEmit directly, or preserve the compiler exit code before filtering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-04-13-email-composer-overhaul.md` around lines
842 - 843, The verification step uses the piped command "pnpm --filter
`@tailfire/api` exec tsc --noEmit 2>&1 | grep template-picker", which masks tsc's
exit code because grep returns 1 when no match is found; change the step to
either run "pnpm --filter `@tailfire/api` exec tsc --noEmit" directly or preserve
the compiler's exit status before filtering (e.g., capture tsc's exit code and
use it for the task result, or use a pipe approach that preserves
PIPESTATUS/returns the original exit code) so a clean compile does not appear as
a failure.
Comment on lines
+102
to
+105
| - [ ] **Step 3: Verify compile** | ||
|
|
||
| Run: `pnpm --filter @tailfire/admin exec tsc --noEmit` | ||
|
|
There was a problem hiding this comment.
Type-check the API workspace in this plan too.
These verification steps only run tsc for @tailfire/admin, but the plan also changes apps/api/src/email-accounts/imap-sync.service.ts. As written, this can still mark the work as “verified” while the API has a compile error. Please add the API package or a repo-wide typecheck here.
Also applies to: 330-334
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-04-13-email-reliability.md` around lines 102 -
105, The plan's "Step 3: Verify compile" only runs TypeScript for
`@tailfire/admin`, but the change touches apps/api (notably
apps/api/src/email-accounts/imap-sync.service.ts), so update the verification
step to include the API workspace or run a repo-wide typecheck; for example,
replace or extend the command `pnpm --filter `@tailfire/admin` exec tsc --noEmit`
to also run `tsc --noEmit` in the API package or run a top-level `pnpm -w exec
tsc --noEmit` so the API's compile errors are caught during verification.
Comment on lines
+168
to
+171
| ### Backend Changes | ||
| - `SendEmailDto` needs an `attachments` field: `{ filename: string; url: string; contentType: string }[]` | ||
| - `SmtpSendService` fetches attachment content from R2 URLs and attaches via Nodemailer's `attachments` option | ||
| - Need a `GET /trips/:id/documents` endpoint if one doesn't exist (list R2 files for a trip) |
There was a problem hiding this comment.
Keep the spec aligned with the storagePath attachment contract.
This section still describes passing attachment URLs through SendEmailDto and downloading from R2 URLs, but the implementation in this PR moved to storagePath specifically to avoid URL-based fetches. Leaving the old contract here will mislead the next round of work.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/specs/2026-04-13-email-composer-overhaul-design.md` around
lines 168 - 171, Update the backend spec to reflect the storagePath-based
attachment contract: change SendEmailDto's attachments to { filename: string;
storagePath: string; contentType: string }[] (remove URL-based semantics),
update SmtpSendService description to state it reads attachment bytes from the
storagePath (not from publicly fetchable R2 URLs) and attaches them to
Nodemailer via the attachments option, and ensure the documentation for the trip
documents endpoint (GET /trips/:id/documents) specifies it returns R2 file
metadata including storagePath entries so callers can pass storagePath into
SendEmailDto.
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.
Summary
Complete email composer overhaul with TipTap rich text editor, CRM contact search, template integration, and email body reliability fix.
Email Composer (13 new files)
Email Reliability Fix
source: trueadded to ImapFlow fetch, parsed with postal-mimeBug Fixes
fetchEmailBodyguard against undefineddownloadResult.contentpreferredNameandlegalFirstNamematch[1]null check)Changes
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation