Optional original-sender header for IMAP inboxes (#348) - #439
Conversation
Adds an optional per-inbox IMAP setting, `original_sender_header`. When set, the address in that header (e.g. X-Original-Sender for Google Groups) is used as the message sender instead of the From header, falling back to From when the header is absent or unparseable. Empty by default, so existing inboxes are unaffected. - models: add OriginalSenderHeader to IMAPConfig (stored in inbox JSON config, no migration). - imap: fetch the configured header, parse it via net/mail, and remap the sender address and contact name in processEnvelope. - frontend: optional field in the email inbox form (both auth modes), zod schema, defaults, and en-US strings. - test: unit coverage for extractSenderFromHeader. Implements abhinavxd#348 (scoped to From remapping, per maintainer's design).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds an optional IMAP original-sender header setting, exposes it in inbox configuration forms, parses the configured header, and uses its sender data during message and contact processing with fallback to the envelope sender. ChangesIMAP original sender support
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant IMAPServer
participant fetchAndProcessMessages
participant extractSenderFromHeader
participant processEnvelope
participant ContactCreation
IMAPServer->>fetchAndProcessMessages: fetch configured sender header
fetchAndProcessMessages->>extractSenderFromHeader: parse header value
extractSenderFromHeader-->>fetchAndProcessMessages: sender address and display name
fetchAndProcessMessages->>processEnvelope: pass sender override
processEnvelope->>ContactCreation: create or update contact identity
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/formSchema.js`:
- Around line 49-50: Update the original_sender_header schema in the frontend
form validation to trim the value and accept only valid IMAP header field names,
rejecting whitespace-only, colon-containing, control-character, and other
invalid values so invalid input falls back to From. Apply the same validation
and normalization at the backend boundary before constructing the IMAP
HEADER.FIELDS request.
In `@internal/inbox/channel/email/imap.go`:
- Around line 342-344: Update the message metadata construction in the
surrounding IMAP handling flow to populate Meta["from"] from the effective
fromAddress after the override, rather than env.From. Preserve the override
consistently for contact creation, blocking checks, and metadata, while
retaining raw sender data only if an existing separate field supports it.
- Around line 181-185: Update the FETCH command lifecycle in the surrounding
IMAP message-fetch function: ensure fetchCmd.Close() runs on every exit path,
including early returns, and propagate any error it returns instead of returning
success after fetchCmd.Next() reaches normal completion. Preserve the existing
message ingestion behavior while reporting deferred FETCH failures, including
those caused by requesting cfg.OriginalSenderHeader.
🪄 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: c220e66e-c887-4ef9-acff-bd95e2dc223e
📒 Files selected for processing (6)
frontend/apps/main/src/features/admin/inbox/EmailInboxForm.vuefrontend/apps/main/src/features/admin/inbox/formSchema.jsi18n/en-US.jsoninternal/inbox/channel/email/imap.gointernal/inbox/channel/email/imap_original_sender_test.gointernal/inbox/models/models.go
| // Also fetch the configured original-sender header (e.g. X-Original-Sender) | ||
| // so the sender can be remapped below. | ||
| if cfg.OriginalSenderHeader != "" { | ||
| headerFields = append(headerFields, cfg.OriginalSenderHeader) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Tracked files matching imap.go:"
fd -a 'imap.go$' . | sed 's#^\./##'
echo
echo "File outline:"
ast-grep outline internal/inbox/channel/email/imap.go --view compact || true
echo
echo "Relevant source lines 140-220:"
sed -n '140,220p' internal/inbox/channel/email/imap.go | nl -ba -v140
echo
echo "Search for fetchCmd.Close and fetchCmd.Next in repository:"
rg -n "fetchCmd\.(Next|Close)|\.Next\(|\.Close\(\)" internal/inbox/channel/email/imap.go . --glob '!vendor/**' --glob '!node_modules/**' | head -n 120Repository: abhinavxd/libredesk
Length of output: 484
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate go module and go-imap usage:"
rg -n "go-emersion/go-imap|github.com/emersion/go-imap|FetchCommand\." go.mod go.sum internal --glob '!vendor/**' --glob '!node_modules/**' | head -n 80
echo
echo "Module version if present:"
sed -n '/github.com\/emersion\/go-imap/v2/p' go.mod go.sum 2>/dev/null | head -n 20Repository: abhinavxd/libredesk
Length of output: 648
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import urllib.request
url = "https://raw.githubusercontent.com/emersion/go-imap/v2.0.0-beta.3/imapclient/fetch.go"
try:
data = urllib.request.urlopen(url, timeout=20).read().decode()
except Exception as e:
print(f"UNABLE_TO_READ: {e}")
raise SystemExit(0)
for marker in ["func (c *FetchCommand) Next()", "func (c *FetchCommand) Close()"]:
idx = data.find(marker)
print(f"\n--- {marker.split()[1] if 'Next()' in marker else marker} ---")
if idx != -1:
text = data[idx:idx+1200]
else:
text = "NOT FOUND"
print(text)
PYRepository: abhinavxd/libredesk
Length of output: 308
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant source lines 130-230:"
sed -n '130,230p' internal/inbox/channel/email/imap.go | cat -n | sed 's/^/ /;s/ *$/ /;s/^/ /' | awk '{print substr($0,1)'
echo
echo "Search for fetch command handling:"
rg -n "fetchCmd|\.Next\(|\.Close\(\)|FETCH" internal/inbox/channel/email/imap.go internal --glob '!vendor/**' --glob '!node_modules/**' | head -n 200
echo
echo "Read full relevant function if file is not too large:"
wc -l internal/inbox/channel/email/imap.go
sed -n '1,260p' internal/inbox/channel/email/imap.go | cat -nRepository: abhinavxd/libredesk
Length of output: 301
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Fetch go-imap FetchCommand Close/Next source without TLS validation using HTTP where available,
# or the GitHub raw HTTPS endpoint with env set to skip verification.
python3 - <<'PY'
import urllib.request, urllib.error
url = "https://raw.githubusercontent.com/emersion/go-imamp/v2.0.0-beta.3/imapclient/fetch.go"
for u in [
"https://raw.githubusercontent.com/emersion/go-imap/v2.0.0-beta.3/imapclient/fetch.go",
"http://raw.githubusercontent.com/emersion/go-imap/v2.0.0-beta.3/imapclient/fetch.go",
]:
print(f"Trying {u}")
try:
req = urllib.request.Request(u, headers={"User-Agent": "code-rabbit-verifier"})
ctx = urllib.request.SSLContext()
ctx.check_hostname = False
ctx.verify_mode = 0
data = urllib.request.urlopen(req, context=ctx, timeout=20).read().decode()
print("OK")
print("\n--- Candidate definitions ---")
for term in ["func (c *FetchCommand) Next()", "func (c *FetchCommand) Close()", "func (c *FetchCommand) Error()"]:
i = data.find(term)
if i == -1:
continue
print(term)
print(data[i:i+1400])
print()
raise SystemExit(0)
except Exception as e:
print(f"ERR: {type(e).__name__}: {e}")
PYRepository: abhinavxd/libredesk
Length of output: 490
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "sed line count then lines 130-240:"
wc -l internal/inbox/channel/email/imap.go
sed -n '130,240p' internal/inbox/channel/email/imap.go
echo
echo "Command handling occurrences:"
rg -n "fetchCmd|Next\(|Close\(|Error|FETCH" internal/inbox/channel/email/imap.go
echo
echo "Fetch command source snippets via httpx/curl/python if available:"
command -v curl || true
command -v wget || true
python3 - <<'PY'
import urllib.request
url = "http://raw.githubusercontent.com/emersion/go-imap/v2.0.0-beta.3/imapclient/fetch.go"
data = urllib.request.urlopen(url, timeout=20).read().decode()
for term in ["func (c *FetchCommand) Next()", "func (c *FetchCommand) Close()", "func (c *FetchCommand) Error()"]:
i = data.find(term)
print(f"--- {term} ---")
print(data[i:i+1500])
PYRepository: abhinavxd/libredesk
Length of output: 6138
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "First 170 lines of repository file before fetchCmd appears:"
sed -n '1,170p' internal/inbox/channel/email/imap.go
echo
echo "Fetch loop and command result handling:"
sed -n '200,340p' internal/inbox/channel/email/imap.go
echo
echo "Fetch command source snippets with curl:"
url="https://raw.githubusercontent.com/emersion/go-imap/v2.0.0-beta.3/imapclient/fetch.go"
curl -fsSL "$url" | sed -n '/func (c \*FetchCommand) Next()/,/func (c \*FetchCommand) [ACDEHKRSU]/p' | sed '/func (c \*FetchCommand) /{N;N;N;N;N;N;N;N;N;N}'
curl -fsSL "$url" | sed -n '/func (c \*FetchCommand) Close()/,/func (c \*FetchCommand) /p' | sed '/func (c \*FetchCommand) /{N;N;N;N;N;N;N;N;N;N}'
curl -fsSL "$url" | sed -n '/func (c \*FetchCommand) Error()/,/func (c \*FetchCommand) /p' | sed '/func (c \*FetchCommand) /{N;N;N;N;N;N;N;N;N;N}'Repository: abhinavxd/libredesk
Length of output: 10581
🌐 Web query:
github.com/emersion/go-imap/v2 imapclient FetchCommand Close Error documentation
💡 Result:
In the go-imap/v2 library, the Close method on an imapclient.FetchCommand is used to finalize the command and release associated resources [1][2][3]. If an error occurred during the processing of the FETCH response (such as an IMAP protocol error, network timeout, or issues with a misbehaving server), this error is returned by the Close method [2][4]. Common considerations regarding FetchCommand.Close include: 1. Error Handling: It is standard practice to check the error returned by Close to ensure the command completed successfully [5][2]. Some developers use a deferred function to log or handle these errors, as shown in official examples and community issue reports [5][2]. 2. Blocking Behavior: The Close method may block if the underlying command has not finished reading all data, or it might hang indefinitely if a network timeout occurs or if the server does not send the expected data [1][3]. 3. Resource Management: Calling Close is essential for cleaning up the command's state and internal channels [3][6]. Failure to call it or improper handling can lead to resource leaks or deadlocks [3]. 4. Known Issues: Community reports have identified potential edge cases, such as panics if certain data fields (like FetchItemDataBodySection.Literal) are nil [5] or errors when interacting with servers that produce non-compliant IMAP responses [2][4]. For the most accurate technical implementation details, refer to the official package documentation at https://pkg.go.dev/github.com/emersion/go-imap/v2/imapclient [7][8].
Citations:
- 1: [v2] fetchCmd.Close() never finish if timeout emersion/go-imap#650
- 2: v2: FETCH can produce empty/imcomplete message (imapclient.FetchMessageBuffer) emersion/go-imap#557
- 3: FetchCommand.Collect/Close/Next stuck when no result is returned emersion/go-imap#562
- 4: Fetch and misbehaving IMAP server emersion/go-imap#630
- 5: FETCH command Close() panics: FetchItemDataBodySection.Literal is nil in discard() emersion/go-imap#644
- 6: https://github.com/emersion/go-imap/blob/v2/imapclient/client.go
- 7: https://pkg.go.dev/github.com/emersion/go-imap/v2/imapclient
- 8: https://pkg.go.dev/github.com/emersion/go-imap/v2
🌐 Web query:
github.com/emersion/go-imap/v2 FetchCommands Error Close API
💡 Result:
In the github.com/emersion/go-imap/v2 library, the FetchCommand returned by Client.Fetch provides a streaming API for retrieving message data [1][2]. The Close() method is used to terminate the command and release associated resources [1][2]. Key considerations regarding FetchCommand.Close() and error handling include: 1. Functionality: Close() cleans up the command state and returns an error if the command encountered a failure, such as issues during the parsing of the IMAP response stream [1][3][2]. It is considered good practice to call Close() (often using defer) after you have finished processing results to ensure proper cleanup [1][4][2]. 2. Error Propagation: Because go-imap/v2 is designed for streaming, errors might not appear immediately when calling Next() or Collect() [3]. If the server sends malformed data or violates the IMAP protocol, an error may only surface when the library attempts to fully parse the response stream, which can happen during or after Close() [3]. 3. Known Issues and Stability: - Blocking/Deadlocks: In some earlier versions, Close() could block indefinitely if no results were returned by the fetch operation [5]. - Panics: Edge cases have been reported where Close() might panic if it attempts to access fields (such as FetchItemDataBodySection.Literal) that were not properly initialized by the server's response [6]. When working with go-imap/v2, ensure you handle the error returned by Close() to detect potential issues that occurred during the streaming process [1][3]. If you encounter persistent issues, verify that your implementation follows the latest practices documented in the imapclient package [7].
Citations:
- 1: https://context7.com/emersion/go-imap/llms.txt?tokens=10000
- 2: https://context7.com/emersion/go-imap/llms.txt
- 3: Fetch and misbehaving IMAP server emersion/go-imap#630
- 4: Reading emails emersion/go-imap#473
- 5: FetchCommand.Collect/Close/Next stuck when no result is returned emersion/go-imap#562
- 6: FETCH command Close() panics: FetchItemDataBodySection.Literal is nil in discard() emersion/go-imap#644
- 7: https://pkg.go.dev/github.com/emersion/go-imap/v2/imapclient
🌐 Web query:
github.com/emersion/go-imap v2 FetchCommand Next returns nil on error
💡 Result:
In github.com/emersion/go-imap/v2, the FetchCommand.Next() method returning nil is the intended behavior when there are no more messages to retrieve from the server for the given FETCH or UID FETCH command [1]. It signifies that the stream of messages for that command has concluded [1]. If you are encountering nil and believe there should be data, or if you are observing unexpected blocking, consider the following technical context: 1. Streaming Nature: Next() is designed to stream messages. When it returns nil, it indicates the channel is empty or the command has finished processing [1]. 2. Command Completion and Blocking: If Next() blocks indefinitely, it is typically because the command has not been properly closed or the server has not sent a completion response [2][3]. Always ensure you call FetchCommand.Close() to release resources and allow the underlying connection to process subsequent commands [3][1]. 3. Server-Side Behavior: Often, Next() returns nil because the IMAP server returned zero messages for the requested sequence or UID set [3]. This can occur if messages were expunged or moved since the sequence numbers were last verified [3]. 4. Error Handling: While Next() returns nil upon command completion, errors (such as connection issues or parsing failures) are generally handled through other mechanisms, such as checking the return value of FetchCommand.Close() or observing the library's logging/error handling patterns during the command lifecycle [3][4][5]. If you suspect a bug (such as an incorrect nil return due to a parsing error or protocol mismatch), verify the server's response logs [5] and check the official repository for known issues related to specific server implementations, as some issues have been reported regarding non-standard server responses [4][5].
Citations:
- 1: https://context7.com/emersion/go-imap/llms.txt?tokens=10000
- 2: FetchCommand.Collect/Close/Next stuck when no result is returned emersion/go-imap#562
- 3: FETCH command did not return any message emersion/go-imap#617
- 4: FETCH command Close() panics: FetchItemDataBodySection.Literal is nil in discard() emersion/go-imap#644
- 5: v2：imapclient fetch message, parse FetchMessageBuffers error emersion/go-imap#659
Propagate FETCH command errors from Close() before returning.
fetchCmd.Next() returns nil for normal completion, while FETCH command errors are reported when the command finishes; this loop has no defer fetchCmd.Close() and returns nil immediately after the stream ends, so a failed FETCH due to the configured header request can be treated as success with no messages ingested. Close the command on all exit paths and propagate any returned error.
🤖 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 181 - 185, Update the
FETCH command lifecycle in the surrounding IMAP message-fetch function: ensure
fetchCmd.Close() runs on every exit path, including early returns, and propagate
any error it returns instead of returning success after fetchCmd.Next() reaches
normal completion. Preserve the existing message ingestion behavior while
reporting deferred FETCH failures, including those caused by requesting
cfg.OriginalSenderHeader.
Source: MCP tools
- Record the remapped address in message meta "from" so it stays consistent with the contact (and isn't left empty when the envelope has no From). - Trim and validate the configured header name before using it in the IMAP HEADER.FIELDS request; an invalid value is ignored (falls back to From) instead of risking a malformed fetch. Mirror the trim/validation in the frontend schema. Addresses review feedback on abhinavxd#439.
|
Thanks for the review — pushed a follow-up for two of the three:
On the FETCH
|
|
Thanks for merging #454 🙏 Whenever you get a chance, this one (the optional original-sender header) is ready too — I pushed fixes for the CodeRabbit feedback a while back (effective sender now consistent in message meta, plus header-name trim/validation), and |
|
Hey, I'll check this over weekend. |
|
I'll merge this closer to next release, as HelpCenter is releasing next. |
3 participants
Implements #348.
Went with the generic approach you suggested rather than anything Google-specific: there's a new optional
original_sender_headerfield on the IMAP inbox config. When it's set, we take the address from that header (e.g.X-Original-Senderfor Google Groups) as the sender, and fall back to the normalFromwhenever the header isn't there. Left empty it changes nothing, so existing inboxes behave exactly as they do today.I kept this to the
From/sender side only since that's what actually fixes the Google Groups case. If atoremap turns out to be needed we can do it as a small follow-up with its own header field.A few notes on how it works:
net/mail— handles both a bareuser@example.comandName <user@example.com>. Anything that doesn't parse just falls back toFrominstead of dropping the message.X-Original-Sender, plus the en-US strings.For testing I added a unit test around the header parsing (
extractSenderFromHeader) covering the display-name, bare-address, missing-header and unparseable cases.go build ./...andgo vet ./internal/inbox/...are clean and the email package tests pass. I didn't spin up the frontend locally, but the new field just mirrors the existing IMAP inputs.Happy to move the field or adjust anything if you'd rather it sit elsewhere.
Summary by CodeRabbit
New Features
X-Original-Senderexample for Google Groups.Bug Fixes
Tests