Various IMAP improvements - #357
Conversation
Close abhinavxd#354 by allowing Fetch to update the seen flag. Close abhinavxd#352 by implementing an Expunge Inbox option on the IMAP channel configuration.
📝 WalkthroughWalkthroughAdds an optional IMAP setting to delete imported messages from the server, filters previously deleted messages, and performs batch expunge. OAuth defaults and admin UI support the setting. Inbox route loading now soft-refreshes an already matching conversation list. ChangesIMAP Expunge Feature
Conversation List Refresh
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant IMAP as IMAP Server
participant processMailbox
participant fetchAndProcessMessages
participant EnvelopeHandler
processMailbox->>IMAP: Select mailbox read-write
processMailbox->>fetchAndProcessMessages: Pass ExpungeInbox
fetchAndProcessMessages->>IMAP: Search excluding Deleted messages
IMAP-->>fetchAndProcessMessages: Matching messages
fetchAndProcessMessages->>IMAP: Fetch UID and Flags
fetchAndProcessMessages->>EnvelopeHandler: Process message
fetchAndProcessMessages->>IMAP: STORE processed UIDs as Deleted
IMAP-->>fetchAndProcessMessages: Deleted flags stored
fetchAndProcessMessages->>IMAP: EXPUNGE
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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 |
| } | ||
|
|
||
| if _, err := client.Select(cfg.Mailbox, &imap.SelectOptions{ReadOnly: true}).Wait(); err != nil { | ||
| if _, err := client.Select(cfg.Mailbox, &imap.SelectOptions{ReadOnly: false}).Wait(); err != nil { |
There was a problem hiding this comment.
Fetch will automatically mark the email as read, but needs the ReadOnly: true setting to do that.
| } | ||
|
|
||
| return e.fetchAndProcessMessages(ctx, client, searchResults, e.Identifier()) | ||
| return e.fetchAndProcessMessages(ctx, client, searchResults, e.Identifier(), cfg.ExpungeInbox) |
There was a problem hiding this comment.
Passing in the user option for the expunge inbox setting.
| func (e *Email) searchMessages(client *imapclient.Client, since time.Time) (*imap.SearchData, error) { | ||
| criteria := &imap.SearchCriteria{ | ||
| Since: since, | ||
| NotFlag: []imap.Flag{imap.FlagDeleted}, |
There was a problem hiding this comment.
#355 don't pull deleted messages that have not been expunged. This should fix some issues with email reappearing after being deleted by external clients also.
| // Fetch envelope and headers needed for auto-reply detection. | ||
| fetchOptions := &imap.FetchOptions{ | ||
| Envelope: true, | ||
| UID: true, |
There was a problem hiding this comment.
Getting the UID seemed very tricky. It works, but maybe an imap expert can review this?
| } | ||
|
|
||
| } | ||
| fmt.Printf("UIDs to delete: %s\n", uidsToDelete.String()) |
There was a problem hiding this comment.
I forgot to remove this debugging statement.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
frontend/shared-ui/assets/styles/main.scss (1)
382-388: ⚡ Quick winUse CSS variables instead of hardcoded colors.
The
.message-incomingstyles use hardcoded hex colors (#a0b8c8,#323a48) instead of the CSS variable pattern used consistently throughout this file (e.g.,hsl(var(--border)),hsl(var(--background))). This makes the colors harder to theme and maintain.Additionally, the
4pxborder is noticeably thicker than the standard1pxborders used elsewhere—verify this is intentional.♻️ Refactor to use CSS variables
.message-incoming{ - border: 4px solid `#a0b8c8`; + border: 2px solid hsl(var(--border)); } .dark .message-incoming{ - background: `#323a48`; + background: hsl(var(--muted)); }Adjust the specific CSS variables (
--border,--muted, etc.) and border width as needed to match your design intent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/shared-ui/assets/styles/main.scss` around lines 382 - 388, Replace the hardcoded colors and nonstandard border width in .message-incoming and .dark .message-incoming with the project's CSS variable pattern: use hsl(var(--border)) (or hsl(var(--muted)) if more appropriate) for the border color and hsl(var(--background)) for the dark background; change the border from 4px to the standard 1px unless the 4px thickness is intentional, and ensure both .message-incoming and .dark .message-incoming reference the same variables for theming consistency..zed/tasks.json (1)
50-50: 💤 Low valueConsider
save: "all"for the Make task.With
save: "none", unsaved buffer changes won't be included in the build, which might surprise developers who expect their edits to be compiled. Settingsave: "all"would ensure all changes are persisted before building.📝 Optional improvement
- "save": "none", + "save": "all",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.zed/tasks.json at line 50, Change the Make task's "save" setting from "none" to "all" so unsaved buffers are persisted before running the build; locate the Make task entry in .zed/tasks.json and update the "save" property to "all" to ensure all open files are written prior to executing the make command.internal/inbox/channel/email/imap.go (3)
337-337: ⚡ Quick winImprove log message clarity.
The log message could be more informative by including the count of messages being expunged.
📝 Proposed improvement
- e.lo.Info("EXPUNGE", "email", "Expunging email") + e.lo.Info("expunging messages from server", "count", len(uidsToDelete.String()), "inbox_id", e.Identifier())Note: If
len(uidsToDelete)doesn't work correctly (per the verification request above), consider tracking the count separately or using a different method to report it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/inbox/channel/email/imap.go` at line 337, The log call e.lo.Info("EXPUNGE", "email", "Expunging email") should include the number of messages being expunged for clarity—update the message to append or include the count (e.g., using len(uidsToDelete) or a tracked counter if len(uidsToDelete) is not reliable) and reference the same context keys; modify the code around the EXPUNGE log in imap.go (where uidsToDelete is built/used) to compute the count and pass it into e.lo.Info so logs show "Expunging X emails" or similar.
254-258: 💤 Low valueSimplify comments.
The comments are overly verbose for straightforward type assertion and assignment.
♻️ Proposed simplification
- // getting the UID for Expunge operation if uidData, ok := item.(imapclient.FetchItemDataUID); ok { - // uidData.UID holds the exact ID you need! - // Save it to a local variable to append to your messages slice later - currentUID = uidData.UID + currentUID = uidData.UID }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/inbox/channel/email/imap.go` around lines 254 - 258, The block performing the type assertion for imapclient.FetchItemDataUID and assignment to currentUID is over-commented; replace the verbose comments inside that if-statement with a single concise line like "Assign UID from fetched item" (keep the if uidData, ok := item.(imapclient.FetchItemDataUID); ok { currentUID = uidData.UID } structure) so the intent remains clear but comments are minimal.
330-331: UIDSet delete guard and NumSet conversion in imap.go
len(uidsToDelete) > 0is valid:imap.UIDSetis a[]UIDRange, so the zero/nil set haslen == 0(len counts ranges, but that’s enough to detect “non-empty” before calling STORE/EXPUNGE).imap.NumSet(uidsToDelete)is redundant:imap.UIDSetalready satisfies theimap.NumSetinterface, so you can passuidsToDeletedirectly (or assignnumSet := uidsToDelete) without an explicit conversion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/inbox/channel/email/imap.go` around lines 330 - 331, The current code redundantly converts uidsToDelete with imap.NumSet and can be simplified: keep the existing guard (if len(uidsToDelete) > 0 && expungeOption) but remove the unnecessary imap.NumSet conversion since imap.UIDSet already implements imap.NumSet; use uidsToDelete directly (or assign numSet := uidsToDelete) when calling STORE/EXPUNGE so you pass the UID set without the extra conversion.i18n/en-US.json (1)
273-274: 💤 Low valueConsider more user-friendly wording for the title.
The current title "Expunge Server after Importing" uses technical IMAP terminology ("expunge") that may not be immediately clear to all users. Consider a more descriptive alternative.
Optional alternative:
"admin.inbox.expungeInbox": "Delete emails from server after import",However, the current wording is acceptable and the description clarifies the behavior, so this is just a suggestion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@i18n/en-US.json` around lines 273 - 274, The string value for the i18n key "admin.inbox.expungeInbox" uses the technical term "Expunge" which may confuse users; update the value in i18n/en-US.json to a more user-friendly phrase such as "Delete emails from server after import" (or "Remove emails from server after importing") while keeping "admin.inbox.expungeInbox.description" unchanged so the description still clarifies the behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/apps/main/src/features/admin/inbox/EmailInboxForm.vue`:
- Around line 490-500: Add the missing expunge_inbox toggle to the OAuth IMAP
configuration block by inserting a FormField for name "imap.expunge_inbox"
inside the v-show="isOAuthInbox" (OAuth IMAP Configuration) section; use the
same SwitchField/FormItem pattern as the manual setup (refer to the existing
FormField usage around "imap.expunge_inbox" and the SwitchField component) so
the switch binds to componentField.modelValue and calls handleChange on
`@update`:checked.
In `@frontend/shared-ui/assets/styles/main.scss`:
- Around line 379-381: The .message-outgoing rule is overriding the Tailwind
bg-private utility (because it appears after `@tailwind` utilities with equal
specificity) and removing the private message visual indicator; remove the
background: inherit declaration from the .message-outgoing selector (or restrict
it to non-private messages only, e.g., target .message-outgoing elements that do
NOT also have the private class) so that the bg-private utility can apply;
update the styling around .message-outgoing (and any private marker class like
bg-private or .message--private) to preserve private message backgrounds.
In `@internal/inbox/channel/email/imap.go`:
- Line 322: Remove the debug fmt.Printf calls that print msgData.uid and replace
them with structured debug logging using the existing logger (e.lo.Debug), e.g.
log the UID via e.lo.Debug with a clear message and include msgData.uid as a
field or formatted value; update both occurrences that print the UID so no
fmt.Printf remains in the IMAP handling code (references: msgData.uid and logger
e.lo.Debug).
- Line 235: The declaration for the variable currentUID (currentUID imap.UID)
contains mixed tabs and spaces; fix it by normalizing the indentation to use
tabs only (align with surrounding var/const blocks and other variable
declarations in imap.go), replacing the spaces before "currentUID" with a single
tab so the variable aligns consistently with other entries in that declaration
block.
---
Nitpick comments:
In @.zed/tasks.json:
- Line 50: Change the Make task's "save" setting from "none" to "all" so unsaved
buffers are persisted before running the build; locate the Make task entry in
.zed/tasks.json and update the "save" property to "all" to ensure all open files
are written prior to executing the make command.
In `@frontend/shared-ui/assets/styles/main.scss`:
- Around line 382-388: Replace the hardcoded colors and nonstandard border width
in .message-incoming and .dark .message-incoming with the project's CSS variable
pattern: use hsl(var(--border)) (or hsl(var(--muted)) if more appropriate) for
the border color and hsl(var(--background)) for the dark background; change the
border from 4px to the standard 1px unless the 4px thickness is intentional, and
ensure both .message-incoming and .dark .message-incoming reference the same
variables for theming consistency.
In `@i18n/en-US.json`:
- Around line 273-274: The string value for the i18n key
"admin.inbox.expungeInbox" uses the technical term "Expunge" which may confuse
users; update the value in i18n/en-US.json to a more user-friendly phrase such
as "Delete emails from server after import" (or "Remove emails from server after
importing") while keeping "admin.inbox.expungeInbox.description" unchanged so
the description still clarifies the behavior.
In `@internal/inbox/channel/email/imap.go`:
- Line 337: The log call e.lo.Info("EXPUNGE", "email", "Expunging email") should
include the number of messages being expunged for clarity—update the message to
append or include the count (e.g., using len(uidsToDelete) or a tracked counter
if len(uidsToDelete) is not reliable) and reference the same context keys;
modify the code around the EXPUNGE log in imap.go (where uidsToDelete is
built/used) to compute the count and pass it into e.lo.Info so logs show
"Expunging X emails" or similar.
- Around line 254-258: The block performing the type assertion for
imapclient.FetchItemDataUID and assignment to currentUID is over-commented;
replace the verbose comments inside that if-statement with a single concise line
like "Assign UID from fetched item" (keep the if uidData, ok :=
item.(imapclient.FetchItemDataUID); ok { currentUID = uidData.UID } structure)
so the intent remains clear but comments are minimal.
- Around line 330-331: The current code redundantly converts uidsToDelete with
imap.NumSet and can be simplified: keep the existing guard (if len(uidsToDelete)
> 0 && expungeOption) but remove the unnecessary imap.NumSet conversion since
imap.UIDSet already implements imap.NumSet; use uidsToDelete directly (or assign
numSet := uidsToDelete) when calling STORE/EXPUNGE so you pass the UID set
without the extra conversion.
🪄 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 Plus
Run ID: ca023e82-934c-4f45-b4ec-d898f07d985e
📒 Files selected for processing (10)
.zed/tasks.jsoncmd/oauth.gofrontend/apps/main/src/features/admin/inbox/EmailInboxForm.vuefrontend/apps/main/src/features/admin/inbox/formSchema.jsfrontend/apps/main/src/features/conversation/message/MessageBubble.vuefrontend/shared-ui/assets/styles/main.scssi18n/en-US.jsoninternal/inbox/channel/email/imap.gointernal/inbox/models/models.gostatic/widget.js
| <FormField v-slot="{ componentField, handleChange }" name="imap.expunge_inbox"> | ||
| <FormItem> | ||
| <SwitchField | ||
| :title="$t('admin.inbox.expungeInbox')" | ||
| :description="$t('admin.inbox.expungeInbox.description')" | ||
| :checked="componentField.modelValue" | ||
| @update:checked="handleChange" | ||
| /> | ||
| </FormItem> | ||
| </FormField> | ||
|
|
There was a problem hiding this comment.
Add expunge_inbox field to OAuth IMAP configuration section.
The expunge inbox toggle is only added to the manual setup section (lines 490-500), but OAuth-connected inboxes also support this feature. Users with OAuth-configured inboxes should be able to enable/disable expunge from the UI.
Add the same field to the OAuth IMAP configuration section (after line 278, within the v-show="isOAuthInbox" block).
➕ Proposed addition
After line 278 (inside the OAuth IMAP Configuration section), add:
<FormField v-slot="{ componentField, handleChange }" name="imap.expunge_inbox">
<FormItem>
<SwitchField
:title="$t('admin.inbox.expungeInbox')"
:description="$t('admin.inbox.expungeInbox.description')"
:checked="componentField.modelValue"
`@update`:checked="handleChange"
/>
</FormItem>
</FormField>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/apps/main/src/features/admin/inbox/EmailInboxForm.vue` around lines
490 - 500, Add the missing expunge_inbox toggle to the OAuth IMAP configuration
block by inserting a FormField for name "imap.expunge_inbox" inside the
v-show="isOAuthInbox" (OAuth IMAP Configuration) section; use the same
SwitchField/FormItem pattern as the manual setup (refer to the existing
FormField usage around "imap.expunge_inbox" and the SwitchField component) so
the switch binds to componentField.modelValue and calls handleChange on
`@update`:checked.
| autoReply bool | ||
| isLoop bool | ||
| extractedMessageID string | ||
| currentUID imap.UID |
There was a problem hiding this comment.
Fix inconsistent whitespace.
The variable declaration uses mixed tabs and spaces. Use consistent indentation (tabs).
🧹 Proposed fix
- currentUID imap.UID
+ currentUID imap.UID📝 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.
| currentUID imap.UID | |
| currentUID imap.UID |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/inbox/channel/email/imap.go` at line 235, The declaration for the
variable currentUID (currentUID imap.UID) contains mixed tabs and spaces; fix it
by normalizing the indentation to use tabs only (align with surrounding
var/const blocks and other variable declarations in imap.go), replacing the
spaces before "currentUID" with a single tab so the variable aligns consistently
with other entries in that declaration block.
| } else { | ||
| if expungeOption { | ||
| // If deletion is enabled, add the message to the delete list | ||
| fmt.Printf("Add UID to Expunge %d\n", msgData.uid) |
There was a problem hiding this comment.
Remove debug print statements.
These fmt.Printf statements should be removed before merging to production. Use the structured logger (e.lo.Debug) instead if debugging output is needed.
🔧 Proposed fix
if expungeOption {
// If deletion is enabled, add the message to the delete list
- fmt.Printf("Add UID to Expunge %d\n", msgData.uid)
+ e.lo.Debug("adding message UID to expunge list", "uid", msgData.uid)
uidsToDelete.AddNum(msgData.uid)
}
}
}
- fmt.Printf("UIDs to delete: %s\n", uidsToDelete.String())
// Batch delete messages from serverAlso applies to: 328-328
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/inbox/channel/email/imap.go` at line 322, Remove the debug
fmt.Printf calls that print msgData.uid and replace them with structured debug
logging using the existing logger (e.lo.Debug), e.g. log the UID via e.lo.Debug
with a clear message and include msgData.uid as a field or formatted value;
update both occurrences that print the UID so no fmt.Printf remains in the IMAP
handling code (references: msgData.uid and logger e.lo.Debug).
|
Hey thanks, I'll check this over this weekend. |
@vanboom Don’t touch localizations files, just update the source file |
|
This PR mixes various things and has unrelated changes in this. |
|
I think you commited your zed file by mistake. |
|
Hi @abhinavxd , yes sorry! I'll try to clean this up. Thanks for your patience. I have been using it for a couple of months now and it is working GREAT with my IMAP server...just like Zammad does. |
|
@abhinavxd I think I have this cleaned up now. Thank you! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/apps/main/src/views/inbox/InboxView.vue`:
- Around line 23-30: Update storeHasCurrentList in InboxView.vue so the viewID
path preserves the existing conversation-status reset before returning whether
the current list matches. Ensure soft refresh through refreshConversationList()
cannot reuse a stale conversations.status filter, while retaining the current
matching logic for type and team list paths.
🪄 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 Plus
Run ID: 1c75dbd3-da7c-44e9-bddd-3c25dd604378
📒 Files selected for processing (1)
frontend/apps/main/src/views/inbox/InboxView.vue
| const storeHasCurrentList = () => { | ||
| const c = conversationStore.conversations | ||
| if (!c.initialized) return false | ||
| if (viewID.value) return c.listType === CONVERSATION_LIST_TYPE.VIEW && String(c.viewID) === String(viewID.value) | ||
| if (type.value) return c.listType === type.value | ||
| if (teamID.value) return c.listType === CONVERSATION_LIST_TYPE.TEAM_UNASSIGNED && String(c.teamID) === String(teamID.value) | ||
| return false | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the view status reset during soft refresh.
This early return bypasses the existing viewID branch that clears the conversation status. If conversations.status is stale, refreshConversationList() reapplies it as a filter and can omit conversations from the view.
Proposed fix
if (storeHasCurrentList()) {
+ if (viewID.value) {
+ conversationStore.setListStatus('', false)
+ }
conversationStore.refreshConversationList()
return
}This relies on the existing view reset in Lines 45-48 and status-filter handling in frontend/apps/main/src/stores/conversation.js Lines 521-565.
Also applies to: 39-42
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/apps/main/src/views/inbox/InboxView.vue` around lines 23 - 30,
Update storeHasCurrentList in InboxView.vue so the viewID path preserves the
existing conversation-status reset before returning whether the current list
matches. Ensure soft refresh through refreshConversationList() cannot reuse a
stale conversations.status filter, while retaining the current matching logic
for type and team list paths.
3 participants
I am not sure I have the capability to update all of the language translations. How do you typically do that? AI?
Summary by CodeRabbit
New Features
Bug Fixes