✨(global) normalize thread/message snippet#755
Open
jbpenrath wants to merge 4 commits into
Open
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:
📝 WalkthroughWalkthroughThe change centralizes cleaned plain-text preview generation, derives persisted thread snippets through ChangesSnippet pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Message
participant JMAPParser
participant preview_text
participant Thread
participant API
participant UI
Message->>JMAPParser: parse MIME content
JMAPParser->>preview_text: clean preview text
preview_text-->>Thread: return truncated snippet
Thread->>Thread: persist derived snippet
Thread->>API: expose snippet fields
API-->>UI: return snippet data
UI->>UI: render thread or folded-message snippet
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/core/management/commands/backfill_thread_snippets.py`:
- Around line 72-85: Update the --before parsing flow in the backfill command so
any datetime returned by parse_datetime is converted to an aware UTC datetime
when it is naive, while preserving existing timezone information for
already-aware values. Ensure the subsequent created_at__lt filter always
receives a timezone-aware cutoff.
In `@src/backend/core/migrations/0033_purge_thread_snippets.py`:
- Around line 14-17: Update purge_snippets to clear non-empty Thread.snippet
values in bounded, keyset-paginated batches rather than one full-table update,
reusing the batching approach and ordering used by backfill_thread_snippets
where available. Disable migration-wide atomicity so each batch commits
independently while preserving the migration’s complete purge behavior.
In `@src/backend/core/models.py`:
- Around line 1036-1038: Update the Thread model’s snippet field definition to
add an index suitable for the exact-match filters used by the backfill command
and purge migration. Keep the existing field behavior and update the
corresponding schema migration so the database index is created.
- Around line 1216-1233: Guard the Message.objects.get refetch in update_stats
when resolving last_visible, catching Message.DoesNotExist from the
snapshot/refetch race. If the message was deleted, avoid propagating the
exception and leave the existing snippet unchanged while still updating
messaged_at.
In `@src/jmap-email/jmap_email/helpers.py`:
- Line 238: Update the _HTML_TAG_RE pattern in helpers.py to match only
plausible HTML tags whose contents begin with a letter, slash, exclamation mark,
or question mark, while preserving markup stripping for valid HTML and avoiding
removal of plain-text comparisons or code expressions.
In `@src/jmap-email/jmap_email/parser.py`:
- Around line 1555-1560: Update the preview source selection in the parser
around text_body and html_body so content values are accepted only when they are
strings; treat None and all other non-string values as empty strings instead of
converting them with str(). Preserve the existing text-before-HTML fallback and
pass the resulting safe source to preview_text.
🪄 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: fad56051-c623-4a31-ae29-75b72e511cf4
⛔ Files ignored due to path filters (1)
src/frontend/src/features/api/gen/models/message.tsis excluded by!**/gen/**
📒 Files selected for processing (31)
src/backend/core/admin.pysrc/backend/core/api/openapi.jsonsrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/thread.pysrc/backend/core/factories.pysrc/backend/core/management/commands/backfill_thread_snippets.pysrc/backend/core/mda/inbound_create.pysrc/backend/core/mda/outbound.pysrc/backend/core/mda/utils.pysrc/backend/core/migrations/0033_purge_thread_snippets.pysrc/backend/core/models.pysrc/backend/core/tests/api/test_messages_create.pysrc/backend/core/tests/api/test_thread_split.pysrc/backend/core/tests/commands/test_backfill_thread_snippets.pysrc/backend/core/tests/mda/test_inbound.pysrc/backend/core/tests/models/test_thread_snippet.pysrc/backend/core/tests/test_utils.pysrc/frontend/src/features/blocknote/hooks/use-base64-composer.tsxsrc/frontend/src/features/blocknote/markdown-exporter.test.tssrc/frontend/src/features/blocknote/markdown-exporter.tssrc/frontend/src/features/forms/components/message-composer/index.tsxsrc/frontend/src/features/layouts/components/thread-panel/components/thread-item/_index.scsssrc/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-header.tsxsrc/jmap-email/CHANGELOG.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.py
💤 Files with no reviewable changes (2)
- src/backend/core/mda/outbound.py
- src/backend/core/factories.py
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/core/migrations/0033_purge_thread_snippets.py (1)
37-46: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winDisable migration-wide atomicity to ensure batches actually commit independently.
Passing
atomic=FalsetoRunPythononly prevents Django from creating an inner savepoint. Because theMigrationclass itself does not explicitly declareatomic = False, Django'sMigrationExecutorstill wraps the entire migration in an outer transaction by default.Because they share the same active outer transaction, all keyset-paginated batches will still accumulate row locks and WAL, committing simultaneously only when the script finishes. Setting
atomic = Falseon the class itself is required to truly release locks and commit independently per batch.⚡ Proposed fix
class Migration(migrations.Migration): """Data-only migration: no schema change.""" + atomic = False + dependencies = [ ("core", "0032_inboundmessage_blob_envelope"), ]🤖 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/migrations/0033_purge_thread_snippets.py` around lines 37 - 46, Set the Migration class’s atomic attribute to False so the migration executor does not wrap the entire purge in one transaction, allowing purge_snippets batches to commit independently while preserving the existing RunPython configuration.
🤖 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/core/models.py`:
- Around line 1229-1242: Extend the exception handling around snippet derivation
in the thread update logic to catch any Exception raised by
last_message.get_parsed_data() or message_snippet(). Log the failure with
logger.exception() for reporting, then set self.snippet to an empty string so
update_stats() continues without aborting; preserve the existing
Message.DoesNotExist handling.
In `@src/frontend/src/features/blocknote/markdown-exporter.ts`:
- Around line 55-62: Update the HTML_TAG_RE definition used by stripHtmlTags to
include the global g flag, ensuring all matching tags are removed in one
replacement pass. Preserve the existing stripping behavior, and add a narrowly
scoped CodeQL suppression only if the output is confirmed never to reach
dangerouslySetInnerHTML or another HTML sink.
- Around line 78-85: Update the markdown processing around CODE_FENCE_RE and
stripHtmlTags so HTML-like text within fenced code blocks is preserved while
ordinary markdown content still has tags stripped. Preserve fenced code segments
separately or otherwise exclude them from stripHtmlTags, then apply the existing
link, image, autolink, and line-break replacements to the remaining content.
---
Outside diff comments:
In `@src/backend/core/migrations/0033_purge_thread_snippets.py`:
- Around line 37-46: Set the Migration class’s atomic attribute to False so the
migration executor does not wrap the entire purge in one transaction, allowing
purge_snippets batches to commit independently while preserving the existing
RunPython configuration.
🪄 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: 81961da1-c0b5-4753-ad24-28b70154f530
📒 Files selected for processing (5)
src/backend/core/migrations/0033_purge_thread_snippets.pysrc/backend/core/models.pysrc/frontend/src/features/blocknote/markdown-exporter.test.tssrc/frontend/src/features/blocknote/markdown-exporter.tssrc/jmap-email/jmap_email/parser.py
jbpenrath
force-pushed
the
feat/message-preview
branch
from
July 21, 2026 09:48
4fa629a to
5a97599
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/backend/core/models.py (1)
1036-1038: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueSnippet field still lacks an index for exact-match filtering.
snippetis filtered by exact match (snippet="") in bothbackfill_thread_snippets.pyand migration0033_purge_thread_snippets.py, but the field still has nodb_index/index. This was raised in a prior review round and wasn't marked as addressed.As per coding guidelines,
src/backend/**/{models.py,migrations/**/*.py}should "Implement database indexing and query optimization (Model Meta indexes, constraints)."🤖 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 1036 - 1038, Update the snippet field declaration in the model containing snippet to add a database index for exact-match filtering, using the project’s established indexing convention. Ensure the generated migration updates the database schema consistently, without changing the field’s existing text, blank, or update behavior.Source: Coding guidelines
🤖 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.
Duplicate comments:
In `@src/backend/core/models.py`:
- Around line 1036-1038: Update the snippet field declaration in the model
containing snippet to add a database index for exact-match filtering, using the
project’s established indexing convention. Ensure the generated migration
updates the database schema consistently, without changing the field’s existing
text, blank, or update behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 330d1a5c-09e9-43b3-9a0a-6b76061fa3bc
⛔ Files ignored due to path filters (1)
src/frontend/src/features/api/gen/models/message.tsis excluded by!**/gen/**
📒 Files selected for processing (35)
compose.yamlsrc/backend/core/admin.pysrc/backend/core/api/openapi.jsonsrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/thread.pysrc/backend/core/factories.pysrc/backend/core/management/commands/backfill_thread_snippets.pysrc/backend/core/mda/inbound_create.pysrc/backend/core/mda/outbound.pysrc/backend/core/mda/utils.pysrc/backend/core/migrations/0033_purge_thread_snippets.pysrc/backend/core/models.pysrc/backend/core/tests/api/test_config.pysrc/backend/core/tests/api/test_messages_create.pysrc/backend/core/tests/api/test_thread_split.pysrc/backend/core/tests/commands/test_backfill_thread_snippets.pysrc/backend/core/tests/mda/test_inbound.pysrc/backend/core/tests/models/test_thread_snippet.pysrc/backend/core/tests/test_utils.pysrc/frontend/src/features/blocknote/hooks/use-base64-composer.tsxsrc/frontend/src/features/blocknote/markdown-exporter.test.tssrc/frontend/src/features/blocknote/markdown-exporter.tssrc/frontend/src/features/forms/components/message-composer/index.tsxsrc/frontend/src/features/layouts/components/thread-panel/components/thread-item/_index.scsssrc/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-header.tsxsrc/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
💤 Files with no reviewable changes (2)
- src/backend/core/factories.py
- src/backend/core/mda/outbound.py
Use the new preview_text method of jmap_email to generate clean snippet for thread (denormalized at update_stats) and message (at serialization). Now when a mesage is folded, we display this snippet. In the thread list we also display this snippet above the subject.
Upgrade to version 0.2.0
jbpenrath
force-pushed
the
feat/message-preview
branch
from
July 23, 2026 14:02
5096075 to
6414310
Compare
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
2 participants
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.
Purpose
Currently we compute thread snippet naively. So if html or text body contains html or markdown tags it was not stripped and the snippet was unreadable by a human so we did not use it. Furthermore, we did not compute snippet for message but it could be useful to display it on frontend when a message is folded.
Summary by CodeRabbit
snippetpreviews to API message responses, and surfaced them across thread UI (thread list items and folded message headers).backfill_thread_snippetsto rebuild existing thread snippets (dry-run, batching, limits, date cutoff).textBodyas plain text for cleaner previews.