✨(jmap_email) clean text preview#758
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughJMAP email parsing now uses immutable ChangesJMAP email parsing update
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant parse_email
participant EmailBodyPart
participant preview_text
parse_email->>EmailBodyPart: select first textual HTML or plain-text content
parse_email->>preview_text: pass content and configured preview bounds
preview_text-->>parse_email: return cleaned one-line preview
parse_email-->>parse_email: store RFC 8621 preview
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jmap-email/jmap_email/parser.py (1)
1677-1680: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale cost description for
previewparam.This docstring still says the cost is "one HTML strip + a unicode-space normalise per message," but
_compute_previewnow delegates topreview_text, which additionally runs markdown-stripping (code fences, links/images, headers, hrules, blockquotes, lists, hard breaks, emphasis) — several more full-text regex passes than described here.📝 Suggested doc update
preview : bool, default True Compute ``preview``: a single-line ≤ 256-char plain-text excerpt. - Set to ``False`` when you don't need it; the cost is one HTML - strip + a unicode-space normalise per message. + Set to ``False`` when you don't need it; the cost is one HTML + strip, a markdown-syntax strip, and a whitespace normalise per + message, over the full body before truncation.🤖 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 `@src/jmap-email/jmap_email/parser.py` around lines 1677 - 1680, Update the `preview` parameter docstring near `_compute_preview` to accurately describe the current per-message cost, including the additional markdown-stripping work performed through `preview_text` alongside HTML stripping and Unicode-space normalization. Keep the description concise and consistent with the actual implementation.
🤖 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 `@src/jmap-email/jmap_email/helpers.py`:
- Around line 202-322: Update preview_text and the surrounding
_HTMLTextExtractor flow to avoid fully parsing and regex-normalizing the first
body part when only a 256-character preview is needed. Implement an early-exit
or streaming cleanup path that stops once max_length cleaned characters are
available, while preserving the current HTML/Markdown stripping, whitespace
normalization, and cleaned-output semantics rather than truncating a raw prefix.
---
Outside diff comments:
In `@src/jmap-email/jmap_email/parser.py`:
- Around line 1677-1680: Update the `preview` parameter docstring near
`_compute_preview` to accurately describe the current per-message cost,
including the additional markdown-stripping work performed through
`preview_text` alongside HTML stripping and Unicode-space normalization. Keep
the description concise and consistent with the actual implementation.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b130e8e3-4c77-44b5-a4d1-0c2a2779211d
📒 Files selected for processing (8)
src/jmap-email/CHANGELOG.mdsrc/jmap-email/README.mdsrc/jmap-email/jmap_email/__init__.pysrc/jmap-email/jmap_email/helpers.pysrc/jmap-email/jmap_email/parser.pysrc/jmap-email/tests/test_helpers.pysrc/jmap-email/tests/test_parser.pysrc/jmap-email/tests/test_preview_fuzz.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/jmap-email/jmap_email/helpers.py`:
- Around line 353-359: Validate max_length at the start of the preview-cleaning
function, before the early return or slicing logic, and raise ValueError
whenever max_length is negative. Preserve the existing behavior for zero and
positive values.
- Around line 361-368: Update the early return in the preview-generation loop
around _clean_preview so it does not return when the cut falls inside an
incomplete recognized Markdown construct, including multiline link or image
labels. Detect or defer such boundaries and continue widening the window or use
the existing fallback until the cleaned preview is syntax-safe; add a regression
test covering a label that spans the cut.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 84319083-c1eb-4927-ba8a-09210f6ec35f
📒 Files selected for processing (5)
src/jmap-email/CHANGELOG.mdsrc/jmap-email/jmap_email/helpers.pysrc/jmap-email/jmap_email/parser.pysrc/jmap-email/tests/test_helpers.pysrc/jmap-email/tests/test_parser.py
| if not text: | ||
| return "" | ||
|
|
||
| # Small bodies: nothing to save, clean in one shot. | ||
| window = max(max_length * 8, 64) | ||
| if len(text) <= window: | ||
| return _clean_preview(text)[:max_length].rstrip() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject negative max_length values.
max_length=-1 reaches a negative slice ([:-1]) and returns nearly the entire cleaned body, contradicting the advertised length cap. Raise ValueError when max_length < 0.
🤖 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 `@src/jmap-email/jmap_email/helpers.py` around lines 353 - 359, Validate
max_length at the start of the preview-cleaning function, before the early
return or slicing logic, and raise ValueError whenever max_length is negative.
Preserve the existing behavior for zero and positive values.
| need = max_length + _PREVIEW_EARLY_EXIT_MARGIN | ||
| while window < len(text): | ||
| cut = text.rfind("\n", 0, window) | ||
| # No line boundary in the window — cannot cut safely, widen. | ||
| if cut > 0: | ||
| cleaned = _clean_preview(text[:cut]) | ||
| if len(cleaned) >= need: | ||
| return cleaned[:max_length].rstrip() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not early-exit inside an incomplete Markdown construct.
The documented margin does not preserve the clean-before-truncate contract: a multiline link label beginning near the start and closing after cut leaves its leading [ in cleaned, which can fall within the first 256 characters; a full clean removes it. Only return after a syntax-safe boundary, or continue/fall back when a recognized construct is incomplete. Add a regression covering a link/image label spanning the cut.
🤖 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 `@src/jmap-email/jmap_email/helpers.py` around lines 361 - 368, Update the
early return in the preview-generation loop around _clean_preview so it does not
return when the cut falls inside an incomplete recognized Markdown construct,
including multiline link or image labels. Detect or defer such boundaries and
continue widening the window or use the existing fallback until the cleaned
preview is syntax-safe; add a regression test covering a label that spans the
cut.
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 `@src/jmap-email/pyproject.toml`:
- Line 3: Update the release version consistently in pyproject.toml and
jmap_email.__init__ so it remains monotonic and does not reuse the already
published 0.2.0; use the next appropriate version for this release and keep both
version declarations synchronized.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f7ccafed-c32f-486f-8499-05932e62a448
⛔ Files ignored due to path filters (1)
src/jmap-email/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
bin/release-jmap-email.shbin/release.pysrc/backend/core/mda/autoreply.pysrc/backend/core/mda/dispatch_webhooks.pysrc/backend/core/mda/inbound.pysrc/backend/core/mda/inbound_auth.pysrc/backend/core/mda/inbound_create.pysrc/backend/core/mda/inbound_pipeline.pysrc/backend/core/mda/inbound_tasks.pysrc/backend/core/mda/replies.pysrc/backend/core/mda/spam.pysrc/backend/core/mda/utils.pysrc/backend/core/mda/webhook_payload.pysrc/backend/core/models.pysrc/backend/core/services/exporter/tasks.pysrc/backend/core/services/importer/labels.pysrc/backend/core/services/importer/mbox_tasks.pysrc/backend/core/services/importer/pst.pysrc/backend/pyproject.tomlsrc/jmap-email/CHANGELOG.mdsrc/jmap-email/README.mdsrc/jmap-email/jmap_email/__init__.pysrc/jmap-email/jmap_email/helpers.pysrc/jmap-email/jmap_email/limits.pysrc/jmap-email/jmap_email/options.pysrc/jmap-email/jmap_email/parser.pysrc/jmap-email/jmap_email/preview.pysrc/jmap-email/pyproject.tomlsrc/jmap-email/tests/test_options.pysrc/jmap-email/tests/test_parser.pysrc/jmap-email/tests/test_preview.pysrc/jmap-email/tests/test_preview_fuzz.py
💤 Files with no reviewable changes (2)
- src/jmap-email/jmap_email/limits.py
- src/jmap-email/jmap_email/helpers.py
5211227 to
676d645
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/backend/core/models.py (1)
2578-2620: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not catch database insert failures as upload failures.
The
tryblock includesself.create()at Lines 2602-2612. If that insert raises a database exception, Django marks the surrounding transaction as rollback-only; the fallbackself.create()at Lines 2620-2628 then executes inside the broken transaction and raisesTransactionManagementErrorinstead of falling back to PostgreSQL storage. Restrict thetryblock toupload_blob()and let database errors roll back normally.🤖 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 `@src/backend/core/models.py` around lines 2578 - 2620, Restrict the try/except in the prefer_offloaded branch to staged model construction and service.upload_blob(), leaving the object-storage self.create() call outside the exception handler. Preserve fallback handling only for upload failures, while allowing database insert exceptions to propagate and roll back normally.src/backend/core/services/importer/mbox.py (2)
128-145: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the non-seekable stream contract.
The docstring says
file.seek()is optional, but the final-message path unconditionally callsfile.tell()andfile.seek(). A read-only stream therefore crashes after indexing otherwise valid input. Either require a seekable file explicitly or derive the final offset from the tracked buffer.🤖 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 `@src/backend/core/services/importer/mbox.py` around lines 128 - 145, Fix the final-message handling in the importer so it honors the documented optional-seek contract instead of unconditionally calling file.tell() and file.seek(). Prefer deriving the final offset from the already tracked scan state and buffer, or explicitly enforce seekability at the API boundary; preserve indexing for valid non-seekable streams and update the relevant docstring contract if seekability becomes required.
154-175: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not truncate header scanning before the Date header.
Only the first 2048 bytes are inspected. If a valid message has a larger header block with
Date:after that cutoff,_mbox_plan()treats it as undated and places it after all dated messages, violating the documented oldest-first ordering. Scan through the header terminator, or document and handle this ordering limitation.🤖 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 `@src/backend/core/services/importer/mbox.py` around lines 154 - 175, The header parsing in _mbox_plan() must not stop at the fixed 2048-byte cutoff before finding Date. Update the logic around header_size and extract_date_from_headers to read through the message’s header terminator (or otherwise continue until Date is found), while preserving buffer reuse and file-position restoration. Ensure valid messages with Date headers beyond 2048 bytes receive their date and remain correctly ordered.src/backend/core/services/importer/pst.py (1)
1186-1207: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftBudget the final MIME representation, not only raw attachment bytes.
MAX_INCOMING_EMAIL_SIZEis enforced later against the composed EML insrc/backend/core/services/importer/utils.py(thedelivercheck), but this loop permits raw attachments to consume the entire limit before base64 encoding, message bodies, and MIME headers are added. Large inputs can therefore exceed the configured cap and allocate substantial extra memory before rejection.Reserve conservative body/MIME/base64 overhead before
read_buffer(), or enforce the bound while constructing the final EML.🤖 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 `@src/backend/core/services/importer/pst.py` around lines 1186 - 1207, Update the attachment budgeting in the message reconstruction loop around `attachment_budget` and `read_buffer()` to reserve conservative space for the message body, MIME headers, and base64 encoding, so raw attachment data cannot consume the entire `MAX_INCOMING_EMAIL_SIZE`. Ensure the reserved budget is applied before reading each attachment while preserving `OversizedAttachmentError` for attachments that exceed the remaining allowable capacity.
🤖 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 `@src/backend/pyproject.toml`:
- Line 147: Remove the "TID251" entry from the lint.ignore configuration so
flake8-tidy-imports banned-API checks remain enabled for the configured
jmap_email modules; leave the other ignored codes unchanged.
---
Outside diff comments:
In `@src/backend/core/models.py`:
- Around line 2578-2620: Restrict the try/except in the prefer_offloaded branch
to staged model construction and service.upload_blob(), leaving the
object-storage self.create() call outside the exception handler. Preserve
fallback handling only for upload failures, while allowing database insert
exceptions to propagate and roll back normally.
In `@src/backend/core/services/importer/mbox.py`:
- Around line 128-145: Fix the final-message handling in the importer so it
honors the documented optional-seek contract instead of unconditionally calling
file.tell() and file.seek(). Prefer deriving the final offset from the already
tracked scan state and buffer, or explicitly enforce seekability at the API
boundary; preserve indexing for valid non-seekable streams and update the
relevant docstring contract if seekability becomes required.
- Around line 154-175: The header parsing in _mbox_plan() must not stop at the
fixed 2048-byte cutoff before finding Date. Update the logic around header_size
and extract_date_from_headers to read through the message’s header terminator
(or otherwise continue until Date is found), while preserving buffer reuse and
file-position restoration. Ensure valid messages with Date headers beyond 2048
bytes receive their date and remain correctly ordered.
In `@src/backend/core/services/importer/pst.py`:
- Around line 1186-1207: Update the attachment budgeting in the message
reconstruction loop around `attachment_budget` and `read_buffer()` to reserve
conservative space for the message body, MIME headers, and base64 encoding, so
raw attachment data cannot consume the entire `MAX_INCOMING_EMAIL_SIZE`. Ensure
the reserved budget is applied before reading each attachment while preserving
`OversizedAttachmentError` for attachments that exceed the remaining allowable
capacity.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6f3d72eb-c125-4b97-afd6-9a0b46b897e0
📒 Files selected for processing (18)
bin/release-jmap-email.shsrc/backend/core/mda/autoreply.pysrc/backend/core/mda/dispatch_webhooks.pysrc/backend/core/mda/inbound.pysrc/backend/core/mda/inbound_auth.pysrc/backend/core/mda/inbound_create.pysrc/backend/core/mda/inbound_pipeline.pysrc/backend/core/mda/inbound_tasks.pysrc/backend/core/mda/replies.pysrc/backend/core/mda/spam.pysrc/backend/core/mda/utils.pysrc/backend/core/mda/webhook_payload.pysrc/backend/core/models.pysrc/backend/core/services/exporter/tasks.pysrc/backend/core/services/importer/labels.pysrc/backend/core/services/importer/mbox.pysrc/backend/core/services/importer/pst.pysrc/backend/pyproject.toml
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jmap-email/jmap_email/parser.py (1)
1921-1927: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
optionsnot propagated to internal address parsing.
_addrs()callsparse_addresses(...)withoutoptions=options, so every address field (from/sender/replyTo/to/cc/bcc, and allResent-*variants at lines 2009-2018) is parsed againstDEFAULT_PARSE_OPTIONS.max_address_list_bytesregardless of theoptionsthe caller passed toparse_email. This defeats the stated purpose of this change — propagating configured caps through parser resource-limit paths — specifically for the address-list byte cap.🐛 Proposed fix
def _addrs(name: str) -> list[EmailAddress] | None: """Header-present → list (possibly empty after validation); header-absent → None. Spec mandates the null-vs-empty distinction (RFC 8621 §4.1.2.2).""" if name not in decoded_by_name: return None - return _jmap_addresses(parse_addresses(raw_addr_headers.get(name, ""))) + return _jmap_addresses( + parse_addresses(raw_addr_headers.get(name, ""), options=options) + )Also applies to: 2009-2018
🤖 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 `@src/jmap-email/jmap_email/parser.py` around lines 1921 - 1927, Update the internal _addrs function to pass the caller’s options into parse_addresses, ensuring all address fields and Resent-* variants honor the configured max_address_list_bytes cap. Preserve the existing null-versus-empty behavior and continue routing parsed results through _jmap_addresses.
🤖 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 `@src/jmap-email/tests/test_preview_fuzz.py`:
- Around line 104-110: Extend test_idempotent_on_plain_prose to parameterize
over the existing content_types strategy, passing the same generated
content_type to both cleaner calls. Preserve the current idempotence assertion
while covering text/plain, HTML, markdown, empty, and unknown content types,
including the HTML path that skips cleaning stages.
---
Outside diff comments:
In `@src/jmap-email/jmap_email/parser.py`:
- Around line 1921-1927: Update the internal _addrs function to pass the
caller’s options into parse_addresses, ensuring all address fields and Resent-*
variants honor the configured max_address_list_bytes cap. Preserve the existing
null-versus-empty behavior and continue routing parsed results through
_jmap_addresses.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5d6116f2-1863-43fb-b52a-eb0c9f01a886
⛔ Files ignored due to path filters (1)
src/jmap-email/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
src/jmap-email/CHANGELOG.mdsrc/jmap-email/README.mdsrc/jmap-email/jmap_email/parser.pysrc/jmap-email/jmap_email/preview.pysrc/jmap-email/tests/test_options.pysrc/jmap-email/tests/test_parser.pysrc/jmap-email/tests/test_preview.pysrc/jmap-email/tests/test_preview_fuzz.py
💤 Files with no reviewable changes (1)
- src/jmap-email/tests/test_options.py
Add a `preview_text` helper that is exposed by the lib. It stripped html tags through a HTMLParser and also strip markdownish syntax that can be found into text body. Previously preview attribute could contains html/md noise, now it is a clean display ready text string.
eee0696 to
4611521
Compare
2 participants
Purpose
Add a
preview_texthelper that is exposed by the lib. It stripped html tags through a HTMLParser and also strip markdownish syntax that can be found into text body. Previously preview attribute could contains html/md noise, now it is a clean display ready text string.Summary by CodeRabbit
New Features
preview_textand configurable parsing/preview caps viaParseOptions+options=.Bug Fixes
Breaking Changes
ParseLimits→ParseOptions;limits=is removed in favor ofoptions=(no aliasing).jmap_email.typesand reduced top-level re-exports.Documentation
Tests