feat: IMAP write-back + remote image safety (#187, #188)#189
Merged
Systemsaholic merged 15 commits intomainfrom Apr 12, 2026
Merged
feat: IMAP write-back + remote image safety (#187, #188)#189Systemsaholic merged 15 commits intomainfrom
Systemsaholic merged 15 commits intomainfrom
Conversation
Key revisions from Codex: - Flags: DB-optimistic + BullMQ queued write-back (not fire-and-forget) - Move/Delete: IMAP-first (not DB-optimistic) to prevent stale UIDs - New ImapWriteService to avoid circular DI - Image blocking: also strip background-image:url(), preserve cid: URIs - Thread fix: also backfill threadId on sent-message dedup path - Separate writeback error tracking from sync/auth errors Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
) 10 tasks in 2 parts: Part A (backend): ImapWriteService, BullMQ processor, wire into EmailAccountsService, sync state fix, thread grouping fix Part B (frontend): sanitizer image blocking, trusted domains type, email reader banner with Load/Trust controls Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Standalone service that connects directly to IMAP servers for write operations (flags, move, delete) without depending on EmailAccountsService, avoiding circular DI. Also adds EMAIL_WRITEBACK queue and EMAIL_WRITEBACK_FLAGS job type constants. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
BullMQ processor that handles 'flags' jobs by delegating to ImapWriteService, allowing the API to respond instantly while IMAP write operations happen asynchronously. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Register the EMAIL_WRITEBACK BullMQ queue with 3 retry attempts and exponential backoff (5s base). Add ImapWriteService and EmailWritebackProcessor to providers, export ImapWriteService for use by other modules. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Track `highestPersistedUid` separately from `uidNext`. Only advance it after each successful `upsertEmailFromImap` call so that any message whose upsert throws an error is re-fetched on the next sync cycle instead of being silently skipped because `lastUid` jumped past it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…essage dedup Rewrote `computeThreadId` to: - Check both `inReplyTo` and all message IDs in the `References` header - Return a non-null UUID for every message (root messages now always get a threadId) - If a matched parent email has threadId=null, generate a new UUID and backfill the parent row before returning it so the full conversation is linked Also fixed the sent-message dedup path: when IMAP sync merges an incoming message with an existing outbound row (same messageId, imapUid=null), the `threadId` is now preserved — existing non-null threadId wins, otherwise the freshly-computed threadId from the current message is written through. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Wire ImapWriteService and EMAIL_WRITEBACK queue into EmailAccountsService: - updateEmailFlags: after DB update, enqueue fire-and-forget flag writeback for inbound emails with IMAP UID - deleteEmail: IMAP delete (move to Trash) first, then DB delete; outbound-only emails skip IMAP - moveEmail: new IMAP-first method that moves on server, captures new UID, then updates DB - Controller updated to route move through EmailAccountsService instead of ImapSyncService Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… URIs
Sanitizer now replaces remote http(s) img src with 1px transparent GIF and
strips background-image url() from inline styles. cid: and data: URIs pass
through untouched. Return type changes to { html, hasBlockedImages } to
support an opt-in banner. Accepts trustedDomains and allowAllImages options.
Updates email-preview-dialog.tsx caller to destructure new return shape.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Persisted per-user list of sender domains whose remote images are allowed through the email sanitizer without prompting. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When the sanitizer blocks remote images, an amber banner appears above the email body with two actions: "Load images" (one-time override for the current email) and "Always load from @Domain" (persists the sender domain to the user's trustedImageDomains preference). The banner and forceShowImages state reset when navigating to a different email. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughImplements IMAP write-back (flags/move/delete) with a BullMQ queue and processor, changes sync to advance only persisted UIDs and backfill thread IDs, and adds remote-image blocking with a trusted-domain allowlist plus UI controls to load or trust images. Changes
Sequence DiagramssequenceDiagram
actor User as User
participant UI as Email Reader
participant DB as Database
participant Service as EmailAccountsService
participant Queue as BullMQ Queue
participant Processor as EmailWritebackProcessor
participant IMAP as IMAP Server
User->>UI: Toggle read/star on message
UI->>DB: PATCH update isSeen/isFlagged
DB-->>UI: Success
UI->>Service: call updateEmailFlags
Service->>Queue: Enqueue `email.writeback.flags` job
Queue-->>Processor: Job delivered
Processor->>IMAP: connect & messageFlagsAdd/Remove(uid, folder, flags)
IMAP-->>Processor: Result
Processor->>Processor: Log success or schedule retry
sequenceDiagram
actor User as User
participant UI as Email Reader
participant Sanitizer as sanitizeEmailHtml
participant Profile as User Profile Service
participant DB as Database
User->>UI: Open email with remote images
UI->>Sanitizer: sanitizeEmailHtml(bodyHtml, { trustedDomains, allowAllImages })
Sanitizer->>Sanitizer: Parse & replace remote images not in allowed domains
Sanitizer-->>UI: { html, hasBlockedImages }
alt hasBlockedImages && !forceShowImages
UI->>UI: Show "Images hidden" banner
User->>UI: Click "Always load from domain"
UI->>Profile: updateMyProfile(add domain to trustedImageDomains)
Profile->>DB: Persist preference
DB-->>Profile: OK
UI->>Sanitizer: Re-run with updated trustedDomains
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 10
🤖 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/emails/inbox/_components/email-reader.tsx`:
- Around line 70-88: The current "Always load from `@domain`" flow stores
senderDomain into trustedImageDomains so sanitizeEmailHtml only allows image
hostnames in trustedDomains, which fails when senders host images on other CDNs;
fix by persisting sender trust separately or teaching the sanitizer to treat the
sender as trusted: either (A) add a new preference key (e.g., trustedSenders)
and update handleTrustDomain/updateProfile to append senderDomain to
profile.platformPreferences.trustedSenders instead of trustedImageDomains, and
pass trustedSenders into sanitizeEmailHtml, or (B) extend sanitizeEmailHtml to
accept a trustedSenders list and, when senderDomain (from senderDomain variable)
is trusted, bypass remote-image blocking even if the image hostname is not in
trustedImageDomains; update the call-site where sanitizeEmailHtml is invoked and
handleTrustDomain accordingly.
In `@apps/admin/src/lib/sanitize-email-html.ts`:
- Around line 3-7: The URL pattern only matches http/https and misses
protocol-relative URLs (e.g. //tracker.example); update the remote URL checks
(the REMOTE_URL_PATTERN and any similar regexes used in the sanitization logic)
to also match protocol-relative URLs by allowing an optional scheme before the
leading // (e.g. use a pattern that accepts either "http(s)://" or "//"); update
all occurrences referenced in the file (including the checks around lines 42-55
and 63-67) so images and CSS background-image rules that start with // are
treated as remote and blocked unless explicitly allowed by
trustedDomains/allowAllImages.
In `@apps/api/src/email-accounts/email-accounts.service.ts`:
- Around line 466-490: The current fire-and-forget enqueue in
writebackQueue.add('email.writeback.flags', ...) can drop or apply stale delta
writes; instead make the job authoritative by (1) reading the current
authoritative flag state (and a version/timestamp such as a flagsVersion or
updatedAt from syncedEmails) and include that full state and version in the job
payload (include symbols: emailId, accountId, uid, folder, flags, syncedEmails,
flagsVersion/updatedAt) and (2) stop swallowing enqueue failures—await
writebackQueue.add and remove the .catch so enqueue errors surface (or implement
retry logic on enqueue) so the write is durable; finally ensure the writeback
worker compares the job's version to the current DB version and only applies
changes when the job's version is newer (or re-reads DB and applies
authoritative flags) to prevent older jobs from overwriting newer state.
- Around line 522-529: The catch blocks around IMAP operations (specifically the
catch for this.imapWriteService.deleteMessage(accountId, email.imapUid,
email.folder) and the similar catch handling the move/delete block later)
currently only log and rethrow as HttpException; modify them to first call
Sentry.captureException(err, { tags: { SENTRY_ENVIRONMENT:
process.env.SENTRY_ENVIRONMENT ?? 'production' } }) (or use your app's Sentry
instance) to ensure errors are sent to Sentry with the SENTRY_ENVIRONMENT tag,
then continue to this.logger.error(...) and throw the HttpException as before;
apply the same change to the other catch block referenced (around lines 576-579)
so all IMAP write failures are captured by Sentry before being converted to 502.
In `@apps/api/src/email-accounts/email-writeback.processor.ts`:
- Around line 29-46: The switch in process(job: Job) is matching 'flags' but
jobs are enqueued as 'email.writeback.flags', so flag write-back jobs hit the
default branch; update the switch to match the actual job name (e.g., replace
case 'flags' with case 'email.writeback.flags') or add an additional case for
'email.writeback.flags' (or normalize job.name and compare suffix) so that the
code path invoking imapWriteService.writeFlags(accountId, uid, folder, { isSeen,
isFlagged }) is executed for enqueued flag jobs.
In `@apps/api/src/email-accounts/imap-sync.service.ts`:
- Around line 89-90: The code sets highestPersistedUid using Math.max(lastUid,
uid) which can leap over failed UIDs and never retry gaps; modify the logic in
imap-sync.service.ts (where highestPersistedUid and lastUid are used) to only
advance the contiguous pointer when the next expected UID is successfully
persisted (e.g., if uid === highestPersistedUid + 1 then set highestPersistedUid
= uid), and for out-of-order higher UIDs buffer or persist them without moving
highestPersistedUid until missing lower UIDs succeed (or maintain a processedUid
set and advance highestPersistedUid while processedUid contains
highestPersistedUid+1); update all places that currently use Math.max(lastUid,
uid) (including the blocks around highestPersistedUid assignment) to this
contiguous-advance approach so failed UIDs will be retried.
- Around line 599-603: The fetch used during sync currently only requests {
envelope: true, bodyStructure: true, flags: true, uid: true } so
envelope.references is never populated; update the IMAP fetch options to include
headers: ['References'] so envelope.references is returned and
computeThreadId(accountId, envelope?.messageId, envelope?.inReplyTo,
referencesRaw) can use the References header; locate the fetch call in
imap-sync.service.ts (the call that returns envelope/bodyStructure/flags/uid)
and add headers: ['References'] to its options.
In `@apps/api/src/email-accounts/imap-write.service.ts`:
- Around line 77-79: The code assumes client.messageMove(...) returns an object
with destination.uidMap but ImapFlow's messageMove returns Promise<void>, so
newUid is always null; fix by replacing the move-without-UID approach: use
client.messageCopy({ uid }, toFolder, { uid: true }) to get a uidMap (inspect
result.destination.uidMap to compute newUid from Object.values(...)[0]), then
delete the original message via client.messageDelete/expunge (or use
client.messageMove only if you then fetch the destination folder to resolve the
new UID by headers/date). Update the logic in imap-write.service.ts where
messageMove, uidMap, and newUid are used so callers receive the real destination
UID (or document the fallback if you choose the fetch-by-headers approach).
In `@docs/superpowers/plans/2026-04-12-imap-writeback-image-safety.md`:
- Around line 308-315: The plan's Step 4 describes performing a soft-delete but
the service implementation actually hard-deletes the syncedEmails row; reconcile
this mismatch by either (A) updating the plan text to state that the delete
method performs a hard-delete of the syncedEmails row after a successful
this.imapWriteService.deleteMessage(accountId, email.imapUid, email.folder) (and
leaving outbound-only behavior unchanged), or (B) changing the service delete
method so that on IMAP success it performs a soft-delete (marking the
syncedEmails row as deleted rather than removing it) so the plan remains
accurate; locate the delete method and the code that removes rows from the
syncedEmails table and apply the chosen fix.
In `@docs/superpowers/specs/2026-04-12-imap-writeback-image-safety-design.md`:
- Around line 47-50: Update the design document section to replace references
that say write-back connections go through ImapSyncService: state that
EmailWritebackProcessor delegates write operations to the new standalone
ImapWriteService, and that EmailAccountsService enqueues jobs to
EmailWritebackProcessor (only depends on BullMQ) while ImapWriteService handles
IMAP connections (and does NOT depend on EmailAccountsService), removing the
circular-DI wording that references ImapSyncService.
🪄 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: 7cf8319b-f914-4cf2-a1d5-393dc44edc68
📒 Files selected for processing (14)
apps/admin/src/app/contacts/[id]/_components/email-preview-dialog.tsxapps/admin/src/app/emails/inbox/_components/email-reader.tsxapps/admin/src/lib/sanitize-email-html.tsapps/api/src/automation/automation.types.tsapps/api/src/email-accounts/email-accounts.controller.tsapps/api/src/email-accounts/email-accounts.module.tsapps/api/src/email-accounts/email-accounts.service.tsapps/api/src/email-accounts/email-writeback.processor.tsapps/api/src/email-accounts/imap-sync.service.tsapps/api/src/email-accounts/imap-write.service.tsdocs/superpowers/plans/2026-04-12-imap-writeback-image-safety.mddocs/superpowers/specs/2026-04-12-imap-writeback-image-safety-design.mdpackages/shared-types/src/api/user-profiles.types.tspackages/shared-types/tsconfig.tsbuildinfo
Comment on lines
+70
to
+88
| const trustedDomains = (profile?.platformPreferences as any)?.trustedImageDomains ?? [] | ||
| const senderDomain = email?.fromAddress?.split('@')[1]?.toLowerCase() | ||
|
|
||
| const { html: sanitizedBody, hasBlockedImages } = useMemo(() => { | ||
| if (!email?.bodyHtml) return { html: '', hasBlockedImages: false } | ||
| return sanitizeEmailHtml(email.bodyHtml, { | ||
| trustedDomains, | ||
| allowAllImages: forceShowImages, | ||
| }) | ||
| }, [email?.bodyHtml, trustedDomains, forceShowImages]) // eslint-disable-line react-hooks/exhaustive-deps | ||
|
|
||
| function handleTrustDomain(domain: string) { | ||
| const current = (profile?.platformPreferences as any)?.trustedImageDomains ?? [] | ||
| if (!current.includes(domain)) { | ||
| updateProfile.mutate({ | ||
| platformPreferences: { | ||
| ...(profile?.platformPreferences as object), | ||
| trustedImageDomains: [...current, domain], | ||
| }, |
There was a problem hiding this comment.
Sender trust is being stored as an image-host allowlist.
senderDomain comes from email.fromAddress, but sanitizeEmailHtml() only allows remote URLs whose image hostname matches trustedDomains (apps/admin/src/lib/sanitize-email-html.ts:63-71). For senders that serve assets from a different CDN, “Always load from @domain” will persist and still keep future images blocked. Persist sender trust separately, or have the sanitizer bypass remote-image blocking when the sender domain is trusted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin/src/app/emails/inbox/_components/email-reader.tsx` around lines 70
- 88, The current "Always load from `@domain`" flow stores senderDomain into
trustedImageDomains so sanitizeEmailHtml only allows image hostnames in
trustedDomains, which fails when senders host images on other CDNs; fix by
persisting sender trust separately or teaching the sanitizer to treat the sender
as trusted: either (A) add a new preference key (e.g., trustedSenders) and
update handleTrustDomain/updateProfile to append senderDomain to
profile.platformPreferences.trustedSenders instead of trustedImageDomains, and
pass trustedSenders into sanitizeEmailHtml, or (B) extend sanitizeEmailHtml to
accept a trustedSenders list and, when senderDomain (from senderDomain variable)
is trusted, bypass remote-image blocking even if the image hostname is not in
trustedImageDomains; update the call-site where sanitizeEmailHtml is invoked and
handleTrustDomain accordingly.
Comment on lines
+3
to
+7
| const REMOTE_URL_PATTERN = /^https?:\/\//i | ||
|
|
||
| interface SanitizeOptions { | ||
| trustedDomains?: string[] | ||
| allowAllImages?: boolean |
There was a problem hiding this comment.
Block protocol-relative image URLs too.
The current checks only catch http:// and https://. URLs like //tracker.example/pixel.gif will still load in both <img src> and background-image:url(...), which leaves an easy bypass for the tracking protection.
Suggested fix
-const REMOTE_URL_PATTERN = /^https?:\/\//i
+const REMOTE_URL_PATTERN = /^(https?:)?\/\//i
...
- if (/url\s*\(\s*['"]?https?:\/\//i.test(style)) {
+ if (/url\s*\(\s*['"]?(https?:)?\/\//i.test(style)) {
const cleaned = style.replace(/background(-image)?\s*:\s*[^;]*url\s*\([^)]*\)[^;]*/gi, '')
el.setAttribute('style', cleaned)
hasBlockedImages = true
}
...
function isAllowedDomain(url: string, trustedDomains?: string[]): boolean {
if (!trustedDomains?.length) return false
try {
- const hostname = new URL(url).hostname.toLowerCase()
- return trustedDomains.some(d => hostname === d || hostname.endsWith(`.${d}`))
+ const normalizedUrl = url.startsWith('//') ? `https:${url}` : url
+ const hostname = new URL(normalizedUrl).hostname.toLowerCase()
+ return trustedDomains.some((d) => {
+ const domain = d.toLowerCase()
+ return hostname === domain || hostname.endsWith(`.${domain}`)
+ })
} catch {
return false
}
}Also applies to: 42-55, 63-67
🤖 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 3 - 7, The URL
pattern only matches http/https and misses protocol-relative URLs (e.g.
//tracker.example); update the remote URL checks (the REMOTE_URL_PATTERN and any
similar regexes used in the sanitization logic) to also match protocol-relative
URLs by allowing an optional scheme before the leading // (e.g. use a pattern
that accepts either "http(s)://" or "//"); update all occurrences referenced in
the file (including the checks around lines 42-55 and 63-67) so images and CSS
background-image rules that start with // are treated as remote and blocked
unless explicitly allowed by trustedDomains/allowAllImages.
Comment on lines
+466
to
+490
| // Queue IMAP flag write-back (fire-and-forget) | ||
| // Only for inbound emails with a valid IMAP UID | ||
| const emailRow = await this.db.client | ||
| .select({ | ||
| imapUid: this.db.schema.syncedEmails.imapUid, | ||
| folder: this.db.schema.syncedEmails.folder, | ||
| isOutbound: this.db.schema.syncedEmails.isOutbound, | ||
| }) | ||
| .from(this.db.schema.syncedEmails) | ||
| .where(eq(this.db.schema.syncedEmails.id, emailId)) | ||
| .limit(1) | ||
|
|
||
| const email = emailRow[0] | ||
| if (email?.imapUid && !email.isOutbound) { | ||
| this.writebackQueue | ||
| .add('email.writeback.flags', { | ||
| type: 'flags', | ||
| accountId, | ||
| uid: email.imapUid, | ||
| folder: email.folder, | ||
| flags, | ||
| }) | ||
| .catch((err) => | ||
| this.logger.warn(`Failed to queue flag writeback: ${err.message}`), | ||
| ) |
There was a problem hiding this comment.
Make flag write-back jobs authoritative, not fire-and-forget deltas.
This path has two desync modes today: .add(...).catch(...) drops the only IMAP write when BullMQ/Redis is unavailable, and an older retried job can run after a newer toggle and flip IMAP back to stale state because the payload only contains the delta. Please make write-back durable and version-aware, or have the worker re-read the current DB flags before applying anything.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/email-accounts/email-accounts.service.ts` around lines 466 -
490, The current fire-and-forget enqueue in
writebackQueue.add('email.writeback.flags', ...) can drop or apply stale delta
writes; instead make the job authoritative by (1) reading the current
authoritative flag state (and a version/timestamp such as a flagsVersion or
updatedAt from syncedEmails) and include that full state and version in the job
payload (include symbols: emailId, accountId, uid, folder, flags, syncedEmails,
flagsVersion/updatedAt) and (2) stop swallowing enqueue failures—await
writebackQueue.add and remove the .catch so enqueue errors surface (or implement
retry logic on enqueue) so the write is durable; finally ensure the writeback
worker compares the job's version to the current DB version and only applies
changes when the job's version is newer (or re-reads DB and applies
authoritative flags) to prevent older jobs from overwriting newer state.
Comment on lines
+522
to
+529
| // 2. IMAP delete first (move to Trash) if has UID and not outbound | ||
| if (email.imapUid && !email.isOutbound) { | ||
| try { | ||
| await this.imapWriteService.deleteMessage(accountId, email.imapUid, email.folder) | ||
| } catch (err: any) { | ||
| this.logger.error(`IMAP delete failed: ${err.message}`) | ||
| throw new HttpException('Failed to delete email on mail server', HttpStatus.BAD_GATEWAY) | ||
| } |
There was a problem hiding this comment.
Capture IMAP write failures before translating them to 502.
These catch blocks convert provider/runtime errors into HttpException, but they only log them. Please emit them to Sentry first so delete/move failures still page correctly and preserve provider context for debugging.
As per coding guidelines, "All runtime errors in the API must be captured by Sentry. Errors must be tagged with SENTRY_ENVIRONMENT as development, preview, or production."
Also applies to: 576-579
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/email-accounts/email-accounts.service.ts` around lines 522 -
529, The catch blocks around IMAP operations (specifically the catch for
this.imapWriteService.deleteMessage(accountId, email.imapUid, email.folder) and
the similar catch handling the move/delete block later) currently only log and
rethrow as HttpException; modify them to first call Sentry.captureException(err,
{ tags: { SENTRY_ENVIRONMENT: process.env.SENTRY_ENVIRONMENT ?? 'production' }
}) (or use your app's Sentry instance) to ensure errors are sent to Sentry with
the SENTRY_ENVIRONMENT tag, then continue to this.logger.error(...) and throw
the HttpException as before; apply the same change to the other catch block
referenced (around lines 576-579) so all IMAP write failures are captured by
Sentry before being converted to 502.
Comment on lines
+29
to
+46
| async process(job: Job): Promise<void> { | ||
| switch (job.name) { | ||
| case 'flags': { | ||
| const data = job.data as WritebackFlagsPayload | ||
| this.logger.debug( | ||
| `Writing flags for UID ${data.uid} in ${data.folder} (account ${data.accountId})`, | ||
| ) | ||
| await this.imapWriteService.writeFlags( | ||
| data.accountId, | ||
| data.uid, | ||
| data.folder, | ||
| { isSeen: data.isSeen, isFlagged: data.isFlagged }, | ||
| ) | ||
| break | ||
| } | ||
| default: | ||
| this.logger.warn(`Unknown job name: ${job.name}`) | ||
| } |
There was a problem hiding this comment.
Flag write-back jobs never execute with the current switch.
apps/api/src/email-accounts/email-accounts.service.ts:478-490 enqueues 'email.writeback.flags', but this worker only handles 'flags'. Every queued flag update will hit default, log a warning, and finish without writing anything to IMAP.
Suggested fix
-import { QUEUES } from '../automation/automation.types'
+import { JOB_TYPES, QUEUES } from '../automation/automation.types'
...
async process(job: Job): Promise<void> {
switch (job.name) {
- case 'flags': {
+ case JOB_TYPES.EMAIL_WRITEBACK_FLAGS: {
const data = job.data as WritebackFlagsPayload
this.logger.debug(
`Writing flags for UID ${data.uid} in ${data.folder} (account ${data.accountId})`,
)
await this.imapWriteService.writeFlags(
data.accountId,
data.uid,
data.folder,
{ isSeen: data.isSeen, isFlagged: data.isFlagged },
)
break
}
default:
- this.logger.warn(`Unknown job name: ${job.name}`)
+ throw new Error(`Unknown job name: ${job.name}`)
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/email-accounts/email-writeback.processor.ts` around lines 29 -
46, The switch in process(job: Job) is matching 'flags' but jobs are enqueued as
'email.writeback.flags', so flag write-back jobs hit the default branch; update
the switch to match the actual job name (e.g., replace case 'flags' with case
'email.writeback.flags') or add an additional case for 'email.writeback.flags'
(or normalize job.name and compare suffix) so that the code path invoking
imapWriteService.writeFlags(accountId, uid, folder, { isSeen, isFlagged }) is
executed for enqueued flag jobs.
Comment on lines
+89
to
+90
| let highestPersistedUid = lastUid | ||
|
|
There was a problem hiding this comment.
lastUid can still jump past failed messages.
highestPersistedUid = Math.max(...) tracks the largest successful UID, not the highest contiguous successful UID. If UID 101 fails but 102 and 103 succeed, Line 131 still stores 103, so UID 101 is never retried.
Suggested fix
- let highestPersistedUid = lastUid
+ let highestPersistedUid = lastUid
...
- await this.upsertEmailFromImap(accountId, account.agencyId, 'INBOX', msg)
- highestPersistedUid = Math.max(highestPersistedUid, Number(msg.uid))
+ await this.upsertEmailFromImap(accountId, account.agencyId, 'INBOX', msg)
+ const uid = Number(msg.uid)
+ if (uid === highestPersistedUid + 1) {
+ highestPersistedUid = uid
+ }Also applies to: 105-107, 120-132
🤖 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 89 - 90, The
code sets highestPersistedUid using Math.max(lastUid, uid) which can leap over
failed UIDs and never retry gaps; modify the logic in imap-sync.service.ts
(where highestPersistedUid and lastUid are used) to only advance the contiguous
pointer when the next expected UID is successfully persisted (e.g., if uid ===
highestPersistedUid + 1 then set highestPersistedUid = uid), and for
out-of-order higher UIDs buffer or persist them without moving
highestPersistedUid until missing lower UIDs succeed (or maintain a processedUid
set and advance highestPersistedUid while processedUid contains
highestPersistedUid+1); update all places that currently use Math.max(lastUid,
uid) (including the blocks around highestPersistedUid assignment) to this
contiguous-advance approach so failed UIDs will be retried.
Comment on lines
+599
to
+603
| // Compute thread ID — considers inReplyTo and References header | ||
| const referencesRaw = Array.isArray(envelope?.references) | ||
| ? envelope.references.join(' ') | ||
| : envelope?.references | ||
| const threadId = await this.computeThreadId(accountId, envelope?.messageId, envelope?.inReplyTo, referencesRaw) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does ImapFlow's fetch(..., { envelope: true })include theReferencesheader onmsg.envelope, or must References be fetched separately?
💡 Result:
No, ImapFlow's fetch(..., { envelope: true }) does not include the References header in msg.envelope. The References header must be fetched separately using the headers: ['References'] option or similar.
Citations:
- 1: https://imapflow.com/docs/guides/fetching-messages/
- 2: https://imapflow.com/docs/api/imapflow-client/
- 3: Add email references in enveloppe postalsys/imapflow#180
🏁 Script executed:
# Check the file size and read the target lines and surrounding context
wc -l apps/api/src/email-accounts/imap-sync.service.tsRepository: Systemsaholic/tailfire
Length of output: 118
🏁 Script executed:
# Read the lines around 599-603 to see the actual code
sed -n '595,610p' apps/api/src/email-accounts/imap-sync.service.tsRepository: Systemsaholic/tailfire
Length of output: 845
🏁 Script executed:
# Search for the fetch call that retrieves the envelope to see what options are used
rg -n "fetch.*envelope" apps/api/src/email-accounts/imap-sync.service.ts -A 5 -B 2Repository: Systemsaholic/tailfire
Length of output: 48
🏁 Script executed:
# Search for "fetch" calls more broadly in the service
rg -n "\.fetch\(" apps/api/src/email-accounts/imap-sync.service.ts -B 3 -A 10Repository: Systemsaholic/tailfire
Length of output: 783
🏁 Script executed:
# Also search for ImapFlow client usage and message retrieval
rg -n "for await.*imap" apps/api/src/email-accounts/imap-sync.service.ts -B 2 -A 15Repository: Systemsaholic/tailfire
Length of output: 48
🏁 Script executed:
# Search for any fetch calls with headers option to see if References is fetched elsewhere
rg -n "headers\s*:" apps/api/src/email-accounts/imap-sync.service.ts -B 5 -A 5Repository: Systemsaholic/tailfire
Length of output: 48
🏁 Script executed:
# Search for any other references to "references" in the file
rg -n "references" apps/api/src/email-accounts/imap-sync.service.ts -iRepository: Systemsaholic/tailfire
Length of output: 805
🏁 Script executed:
# Check the computeThreadId implementation to understand how References should be used
rg -n "computeThreadId" apps/api/src/email-accounts/imap-sync.service.ts -A 30 | head -50Repository: Systemsaholic/tailfire
Length of output: 2205
🏁 Script executed:
# Also look for where referencesHeader is populated to understand the intended flow
rg -n "referencesHeader" apps/api/src/email-accounts/imap-sync.service.ts -B 3 -A 3Repository: Systemsaholic/tailfire
Length of output: 933
References header is not fetched; add it to the fetch call.
The sync fetch at line 97 only requests { envelope: true, bodyStructure: true, flags: true, uid: true }. ImapFlow's envelope: true does not include the References header—it must be explicitly requested via headers: ['References']. As a result, envelope.references is always undefined, and computeThreadId() receives an empty referencesRaw, falling back to inReplyTo only and losing References-based threading logic.
Update the fetch call to include headers: ['References'] so the References header is populated and threading considers all message-ID references.
🤖 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 599 - 603, The
fetch used during sync currently only requests { envelope: true, bodyStructure:
true, flags: true, uid: true } so envelope.references is never populated; update
the IMAP fetch options to include headers: ['References'] so envelope.references
is returned and computeThreadId(accountId, envelope?.messageId,
envelope?.inReplyTo, referencesRaw) can use the References header; locate the
fetch call in imap-sync.service.ts (the call that returns
envelope/bodyStructure/flags/uid) and add headers: ['References'] to its
options.
Comment on lines
+77
to
+79
| const result = await client.messageMove({ uid }, toFolder, { uid: true }) | ||
| const uidMap = result?.destination?.uidMap | ||
| const newUid = uidMap ? Number(Object.values(uidMap)[0]) || null : null |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the official ImapFlow API, what is the type of messageMove(...).destination.uidMap, and how should the new UID for a moved message be read?
💡 Result:
In the official ImapFlow API documentation, messageMove(range, destination, options) returns a Promise (shows await client.messageMove(...) with no result assigned). There is no destination.uidMap property mentioned or returned by messageMove. However, the closely related messageCopy(range, destination, options) does return a Promise with properties including uidMap (Map from source UIDs to destination UIDs), path (destination path), and uidValidity (BigInt). Example: let result = await client.messageCopy('1:10', 'Archive', { uid: true }); console.log('Copied to UIDs:', result.uidMap); The query may confuse messageMove with messageCopy. For messageMove, since it uses the IMAP MOVE extension (RFC 6851), which is atomic and implemented on top of COPY+DELETE, it likely does not return UID mapping in the same way, or the docs simply omit showing it. To read new UIDs after move, either: 1. Use messageCopy instead if copy semantics are acceptable, then delete originals manually. 2. After messageMove, lock the destination mailbox and fetch recent messages or use search to identify moved ones (e.g., by date, flags, or size). 3. Check TypeScript types in lib/imap-flow.d.ts from the package for exact return type (docs reference this for details). No explicit type or example for messageMove UID mapping found in official docs at imapflow.com.
Citations:
🏁 Script executed:
cat -n apps/api/src/email-accounts/imap-write.service.ts | head -100Repository: Systemsaholic/tailfire
Length of output: 3993
🏁 Script executed:
rg "messageMove|messageCopy" apps/api/src --context 3Repository: Systemsaholic/tailfire
Length of output: 1374
🏁 Script executed:
sed -n '1,50p' apps/api/src/email-accounts/imap-sync.service.tsRepository: Systemsaholic/tailfire
Length of output: 1662
🏁 Script executed:
rg -B 5 -A 10 "messageMove" apps/api/src/email-accounts/imap-sync.service.tsRepository: Systemsaholic/tailfire
Length of output: 567
🏁 Script executed:
rg "moveMessage" apps/api/src --context 2Repository: Systemsaholic/tailfire
Length of output: 1268
🏁 Script executed:
rg -B 3 -A 8 "this.imapWriteService.moveMessage" apps/api/src/email-accounts/email-accounts.service.tsRepository: Systemsaholic/tailfire
Length of output: 459
messageMove() returns Promise<void> with no properties — newUid will always be null.
According to ImapFlow's official API, messageMove() returns Promise<void> and does not include a destination or uidMap property. The code at line 77–79 attempts to read result?.destination?.uidMap, but since result is undefined, newUid will always be null after every successful move. This breaks the function's documented contract ("The new UID in the destination folder") and causes callers in email-accounts.service.ts to fall back to the stale UID.
Consider using messageCopy() (which does return uidMap) followed by deletion, or implement an alternative strategy (e.g., fetch the destination folder post-move to identify the message by headers/date).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/email-accounts/imap-write.service.ts` around lines 77 - 79, The
code assumes client.messageMove(...) returns an object with destination.uidMap
but ImapFlow's messageMove returns Promise<void>, so newUid is always null; fix
by replacing the move-without-UID approach: use client.messageCopy({ uid },
toFolder, { uid: true }) to get a uidMap (inspect result.destination.uidMap to
compute newUid from Object.values(...)[0]), then delete the original message via
client.messageDelete/expunge (or use client.messageMove only if you then fetch
the destination folder to resolve the new UID by headers/date). Update the logic
in imap-write.service.ts where messageMove, uidMap, and newUid are used so
callers receive the real destination UID (or document the fallback if you choose
the fetch-by-headers approach).
Comment on lines
+308
to
+315
| - [ ] **Step 4: Make delete IMAP-first** | ||
|
|
||
| Find the delete method. Change it to: | ||
| 1. Call `this.imapWriteService.deleteMessage(accountId, email.imapUid, email.folder)` first | ||
| 2. On success: soft-delete from DB (existing behavior) | ||
| 3. On IMAP failure: throw error (don't delete from DB) | ||
|
|
||
| For outbound-only emails (no imapUid), skip IMAP and just delete from DB. |
There was a problem hiding this comment.
The plan still describes delete as a soft-delete.
The current service implementation hard-deletes the syncedEmails row, so this step is now misleading for anyone validating behavior or using the plan for follow-up work.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-04-12-imap-writeback-image-safety.md` around
lines 308 - 315, The plan's Step 4 describes performing a soft-delete but the
service implementation actually hard-deletes the syncedEmails row; reconcile
this mismatch by either (A) updating the plan text to state that the delete
method performs a hard-delete of the syncedEmails row after a successful
this.imapWriteService.deleteMessage(accountId, email.imapUid, email.folder) (and
leaving outbound-only behavior unchanged), or (B) changing the service delete
method so that on IMAP success it performs a soft-delete (marking the
syncedEmails row as deleted rather than removing it) so the plan remains
accurate; locate the delete method and the code that removes rows from the
syncedEmails table and apply the chosen fix.
Comment on lines
+47
to
+50
| - **New `EmailWritebackProcessor`** — a BullMQ processor in the email-accounts module | ||
| - `EmailAccountsService` enqueues jobs (only depends on BullMQ, not ImapSync) | ||
| - `EmailWritebackProcessor` depends on `ImapSyncService` for IMAP connections | ||
| - For IMAP-first operations (move/delete): add new methods to a standalone `ImapWriteService` that `EmailAccountsService` can call directly (no cycle because ImapWriteService is new and doesn't depend on EmailAccountsService) |
There was a problem hiding this comment.
Update the design doc to reference ImapWriteService.
The implementation no longer routes write-back connections through ImapSyncService; EmailWritebackProcessor now delegates to the standalone ImapWriteService. Leaving this section unchanged makes the circular-DI explanation misleading.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/specs/2026-04-12-imap-writeback-image-safety-design.md`
around lines 47 - 50, Update the design document section to replace references
that say write-back connections go through ImapSyncService: state that
EmailWritebackProcessor delegates write operations to the new standalone
ImapWriteService, and that EmailAccountsService enqueues jobs to
EmailWritebackProcessor (only depends on BullMQ) while ImapWriteService handles
IMAP connections (and does NOT depend on EmailAccountsService), removing the
circular-DI wording that references ImapSyncService.
The onLayoutChange useEffect was firing on initial render, which triggered a profile save → profile update → layout re-read → state change → re-render → loop. Fix: skip initial render via ref, use stable ref for callback to remove it from dependency array. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add internalDate to IMAP fetch and use for date field - Backfill NULL dates from synced_at - Fix DropdownMenuContent positioning with side/sideOffset - Add "Add Sub-Folder" menu item - Extend sync to also fetch from Sent folder (not just INBOX) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
All three panes (left, center, right) now have flex flex-col so child components with flex-1 overflow-y-auto can scroll properly. The email reader body was unscrollable because the parent pane didn't pass through flex layout. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Apr 12, 2026
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
Part A: IMAP Write-Back
Email flag changes now sync back to the IMAP server:
Architecture:
ImapWriteService— standalone IMAP write methods (no circular DI)EmailWritebackProcessor— BullMQ processor for flag jobs (3 retries, exponential backoff)EmailAccountsServiceenqueues flag jobs + calls ImapWriteService directly for move/deleteSync Reliability Fixes
lastUidonly advances to highest successfully persisted UID (notuidNext - 1)Part B: Remote Image Safety
img[src]ANDbackground-image: url(...)from inline stylescid:anddata:URIs (inline attachments)Test plan
fixes #187, fixes #188
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes