Conversation
📝 WalkthroughWalkthroughThe change adds configurable trashbin retention and permanent deletion, mailbox storage usage and entitlement APIs, scheduled cleanup, storage caching, and frontend quota, storage-settings, retention-notice, and empty-trash controls. ChangesTrashbin and mailbox storage
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ThreadPanel
participant EmptyTrashHook
participant MailboxAPI
participant TrashbinService
User->>ThreadPanel: Selects Empty trash or Empty spam
ThreadPanel->>EmptyTrashHook: Request confirmation
EmptyTrashHook->>MailboxAPI: POST scoped deletion request
MailboxAPI->>TrashbinService: Delete authorized messages
TrashbinService-->>MailboxAPI: Return deleted_count
MailboxAPI-->>EmptyTrashHook: Return deletion result
EmptyTrashHook-->>ThreadPanel: Invalidate stats and show toast
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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
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/mda/inbound_create.py (1)
519-541: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUnguarded
update_fieldsconstruction frommessage_flags.keys().
update_fieldsis built from all remainingmessage_flagskeys without the samehasattr(message, flag)guard used in thesetattrloop just above. Ifcompute_labels_and_flagsever returns a key that isn't a genuineMessagefield,message.save(update_fields=update_fields)will raise, since Django validates everyupdate_fieldsentry against the model's concrete fields.Today this is probably safe (only
is_trashed/is_spam-style keys survive after poppingis_unread/_starred), but the asymmetry between the guardedsetattrand the unguardedupdate_fieldslist is a latent trap for future flag additions.#!/bin/bash # Inspect compute_labels_and_flags to confirm every returned key (besides # is_unread/_starred) maps to a real Message model field. rg -n -A 40 'def compute_labels_and_flags' --type=py🤖 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/mda/inbound_create.py` around lines 519 - 541, Build update_fields in the import flow using only message_flags keys that pass the existing hasattr(message, flag) check in the setattr loop. Keep assigning recognized flags as before, preserve created_at and trashed_at updates, and exclude unknown keys so message.save(update_fields=...) only receives valid Message fields.
🤖 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/api/openapi.json`:
- Line 4271: Update the mailbox storage calculation in the viewset method that
builds LargestThread results so each thread’s total_size includes per-message
overhead in addition to blob and draft-blob bytes, matching the OpenAPI
description. Apply the same corrected storage basis to the related lines
8588-8590 and preserve the existing totals and ordering behavior.
In `@src/backend/core/api/viewsets/mailbox.py`:
- Around line 156-208: Reduce the query cost in the storage action by reusing
the cached mailbox storage accessor, such as get_mailbox_storage_used, instead
of recomputing totals with compute_mailbox_storage_used, and cache the remaining
storage payload or aggregates with a short TTL. Preserve the existing response
values and top-100 thread behavior while ensuring repeated Storage-tab requests
do not rescan the mailbox’s full message/blob set.
- Around line 241-251: Update the `largest_threads` annotation in the mailbox
storage calculation to include per-message overhead using the existing
`msg_count` annotation and overhead value, matching `_storage_for_messages` and
the documented per-thread sizing. Keep the existing blob and draft size
components and ordering intact.
In `@src/backend/core/entitlements/backends/local.py`:
- Around line 48-66: Update get_mailbox_entitlements in
src/backend/core/entitlements/backends/local.py (lines 48-66) to check org_value
with an explicit is not None comparison, matching the existing convention in
src/backend/core/entitlements/backends/deploycenter.py (lines 220-248). No
change is needed in deploycenter.py.
In `@src/backend/core/models.py`:
- Around line 2203-2207: Update msg_trashbin_cutoff_idx to use an expression
index on Coalesce("trashed_at", "created_at") while preserving the existing
is_trashed/is_spam partial condition, and add the corresponding database
migration.
In `@src/backend/core/services/trashbin.py`:
- Around line 160-175: Update the batch sweep around permanently_delete_messages
so mailbox storage cache invalidation occurs for each successfully processed
batch, or is guaranteed via try/finally when the loop aborts. Preserve
touched_mailbox_ids for all completed deletions so a later failure cannot skip
invalidation of earlier batches.
In `@src/backend/messages/settings.py`:
- Around line 1082-1088: The TRASHBIN_ALLOW_EMPTY setting currently defers
invalid-value detection until runtime; validate it against TrashbinAllowEmpty
during startup configuration while retaining runtime validation if
override_settings requires it. Update src/backend/messages/settings.py lines
1082-1088 at TRASHBIN_ALLOW_EMPTY, and update docs/env.md line 413 to retain the
startup-rejection claim only after startup validation is implemented.
In `@src/frontend/public/locales/common/fr-FR.json`:
- Line 1043: Update the French translation for “You are about to permanently
delete every message in the spam folder. This cannot be undone.” to use “dossier
des indésirables” instead of “dossier indésirables,” preserving the rest of the
translation.
In
`@src/frontend/src/features/layouts/components/mailbox-settings/modal-mailbox-settings/storage-tab/index.tsx`:
- Around line 198-201: Update the status indicator span in the mailbox settings
storage item to expose its existing read/unread aria-label to assistive
technology by adding role="img" (or an equivalent visually hidden text child),
while preserving the current label selection.
In `@src/frontend/src/features/message/use-empty-trash.tsx`:
- Around line 81-83: Update the onSuccess handler in use-empty-trash.tsx to use
the generated response type from useMailboxesEmptyTrashCreate directly, removing
the manual `{ deleted_count?: number }` cast. Access deleted_count through
response.data while preserving the existing nullish fallback to 0.
---
Outside diff comments:
In `@src/backend/core/mda/inbound_create.py`:
- Around line 519-541: Build update_fields in the import flow using only
message_flags keys that pass the existing hasattr(message, flag) check in the
setattr loop. Keep assigning recognized flags as before, preserve created_at and
trashed_at updates, and exclude unknown keys so message.save(update_fields=...)
only receives valid Message fields.
🪄 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: 77ac0eb8-0c4f-4b96-826c-e15b5602fdcf
⛔ Files ignored due to path filters (14)
src/frontend/src/features/api/gen/mailboxes/mailboxes.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/config_retrieve200.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/largest_thread.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_abilities.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_empty_trash_request_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_empty_trash_request_scope_enum.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_empty_trash_response.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_entitlements.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_entitlements_organization.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_storage_stats.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/storage_entitlement.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_bulk_delete_request_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_bulk_delete_request_scope_enum.tsis excluded by!**/gen/**
📒 Files selected for processing (47)
docs/env.mdsrc/backend/core/api/openapi.jsonsrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/config.pysrc/backend/core/api/viewsets/flag.pysrc/backend/core/api/viewsets/mailbox.pysrc/backend/core/api/viewsets/metrics.pysrc/backend/core/api/viewsets/thread.pysrc/backend/core/entitlements/__init__.pysrc/backend/core/entitlements/backends/base.pysrc/backend/core/entitlements/backends/deploycenter.pysrc/backend/core/entitlements/backends/local.pysrc/backend/core/enums.pysrc/backend/core/mda/inbound_create.pysrc/backend/core/migrations/0035_message_msg_trashbin_cutoff_idx.pysrc/backend/core/models.pysrc/backend/core/services/storage.pysrc/backend/core/services/trashbin.pysrc/backend/core/tasks.pysrc/backend/core/tests/api/test_config.pysrc/backend/core/tests/api/test_mailbox_empty_trash.pysrc/backend/core/tests/api/test_mailbox_entitlements.pysrc/backend/core/tests/api/test_mailbox_storage.pysrc/backend/core/tests/api/test_messages_flag.pysrc/backend/core/tests/entitlements/test_mailbox_backends.pysrc/backend/core/tests/importer/test_import_channel.pysrc/backend/core/tests/services/test_storage_cache.pysrc/backend/core/tests/tasks/test_cleanup_trashbin.pysrc/backend/messages/celery_app.pysrc/backend/messages/settings.pysrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/public/locales/common/nl-NL.jsonsrc/frontend/src/features/config/resolve.tssrc/frontend/src/features/layouts/components/mailbox-panel/index.tsxsrc/frontend/src/features/layouts/components/mailbox-settings/modal-mailbox-settings/_index.scsssrc/frontend/src/features/layouts/components/mailbox-settings/modal-mailbox-settings/index.tsxsrc/frontend/src/features/layouts/components/mailbox-settings/modal-mailbox-settings/storage-tab/index.tsxsrc/frontend/src/features/layouts/components/thread-panel/_index.scsssrc/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsxsrc/frontend/src/features/layouts/components/thread-panel/index.tsxsrc/frontend/src/features/message/use-delete.tsxsrc/frontend/src/features/message/use-empty-trash.tsxsrc/frontend/src/features/quota/api/use-mailbox-entitlements.tssrc/frontend/src/features/quota/components/quota-widget.scsssrc/frontend/src/features/quota/components/quota-widget.tsxsrc/frontend/src/hooks/use-ability.ts
| "/api/v1.0/mailboxes/{id}/storage/": { | ||
| "get": { | ||
| "operationId": "mailboxes_storage_retrieve", | ||
| "description": "Return total storage and the top-100 largest threads for the mailbox.\n\nThe total is computed with the shared storage service (the same formula\nthe metrics endpoints and the quota gauge use), so the \"Total storage\nused\" here always matches the sidebar gauge. Per-thread sizes and the\ntrash/spam subtotals cover message overhead plus MIME and draft blobs —\nattachments and templates are not thread-scoped.\n\nBacks the Storage settings tab; mailbox admins only.", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align LargestThread.size with the advertised storage basis.
Line 4271 promises per-thread sizes include message overhead, but src/backend/core/api/viewsets/mailbox.py currently returns only blob and draft-blob bytes. Add the per-message overhead to total_size (and update Lines 8588-8590), or remove the promise; the current contract is contradictory.
Also applies to: 8588-8590
🤖 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/api/openapi.json` at line 4271, Update the mailbox storage
calculation in the viewset method that builds LargestThread results so each
thread’s total_size includes per-message overhead in addition to blob and
draft-blob bytes, matching the OpenAPI description. Apply the same corrected
storage basis to the related lines 8588-8590 and preserve the existing totals
and ordering behavior.
| @action(detail=True, methods=["get"], url_path="storage") | ||
| def storage(self, request, **kwargs): | ||
| """Return total storage and the top-100 largest threads for the mailbox. | ||
|
|
||
| The total is computed with the shared storage service (the same formula | ||
| the metrics endpoints and the quota gauge use), so the "Total storage | ||
| used" here always matches the sidebar gauge. Per-thread sizes and the | ||
| trash/spam subtotals cover message overhead plus MIME and draft blobs — | ||
| attachments and templates are not thread-scoped. | ||
|
|
||
| Backs the Storage settings tab; mailbox admins only. | ||
| """ | ||
| mailbox = self.get_object() | ||
| overhead = settings.METRICS_STORAGE_USED_OVERHEAD_BY_MESSAGE | ||
|
|
||
| thread_ids = models.ThreadAccess.objects.filter(mailbox=mailbox).values_list( | ||
| "thread_id", flat=True | ||
| ) | ||
|
|
||
| # Total mirrors the gauge exactly (message overhead + every blob the | ||
| # mailbox reaches), so the two never disagree. | ||
| total_storage = compute_mailbox_storage_used(mailbox) | ||
| message_count = models.Message.objects.filter(thread__id__in=thread_ids).count() | ||
| thread_count = models.Thread.objects.filter(accesses__mailbox=mailbox).count() | ||
|
|
||
| # Storage held by trashed / spam mail: message overhead plus MIME and | ||
| # draft blobs — the same basis as the per-thread sizes below | ||
| # (attachments/templates are not thread-scoped). | ||
| # | ||
| # Scoped per MESSAGE, not per thread. The thread-level flags are | ||
| # denormalizations that answer a different question: Thread.is_trashed | ||
| # is true only when *every* message is trashed, and Thread.is_spam | ||
| # mirrors the *first* message alone (see Thread.update_stats). Summing | ||
| # whole threads through them therefore both under-counted (a partly | ||
| # trashed thread contributed nothing) and over-counted (a spam-first | ||
| # thread contributed its non-spam messages too). Message-level scoping | ||
| # matches what ``empty_trashbin`` actually deletes. | ||
| # | ||
| # ``blob`` and ``draft_blob`` are both forward FKs, so joining them adds | ||
| # no row fan-out and Count() stays one-per-message. Blobs are summed per | ||
| # referencing message rather than deduplicated, which is the same | ||
| # "storage felt" basis as the total above (see core.services.storage). | ||
| def _storage_for_messages(condition): | ||
| return models.Message.objects.filter( | ||
| condition, thread_id__in=thread_ids | ||
| ).aggregate( | ||
| total=Coalesce(Sum("blob__size_compressed"), Value(0)) | ||
| + Coalesce(Sum("draft_blob__size_compressed"), Value(0)) | ||
| + Count("id") * overhead | ||
| )["total"] | ||
|
|
||
| trashed_storage = _storage_for_messages(Q(is_trashed=True)) | ||
| spam_storage = _storage_for_messages(Q(is_spam=True)) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff
storage runs several unbounded aggregate scans per request.
compute_mailbox_storage_used, two message-level aggregates, a message count, a thread count and the top-100 annotation each traverse the mailbox's whole message/blob set on every open of the Storage tab. Consider serving the totals from the cached storage accessors (get_mailbox_storage_used, as the entitlements backend does) and/or caching this payload with a short TTL, so a large mailbox cannot make the settings modal an expensive query burst.
🤖 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/api/viewsets/mailbox.py` around lines 156 - 208, Reduce the
query cost in the storage action by reusing the cached mailbox storage accessor,
such as get_mailbox_storage_used, instead of recomputing totals with
compute_mailbox_storage_used, and cache the remaining storage payload or
aggregates with a short TTL. Preserve the existing response values and top-100
thread behavior while ensuring repeated Storage-tab requests do not rescan the
mailbox’s full message/blob set.
| largest_threads = ( | ||
| models.Thread.objects.filter(accesses__mailbox=mailbox) | ||
| .annotate( | ||
| blob_size=Coalesce(thread_size_subquery, Value(0)), | ||
| draft_size=Coalesce(thread_draft_size_subquery, Value(0)), | ||
| msg_count=Coalesce(thread_msg_count_subquery, Value(0)), | ||
| access_read_at=read_at_subquery, | ||
| ) | ||
| .annotate(total_size=F("blob_size") + F("draft_size")) | ||
| .order_by("-total_size")[:100] | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Per-thread total_size omits the per-message overhead the docstring and the trash/spam subtotals include.
Lines 162-164 state per-thread sizes "cover message overhead plus MIME and draft blobs", and _storage_for_messages adds Count("id") * overhead, but total_size is only blob_size + draft_size. msg_count is annotated yet unused in the sum, so the "Largest conversations" list under-reports (and can mis-order) relative to the trash/spam figures and the total. Either include the overhead or correct the docstring.
🐛 Proposed fix
- .annotate(total_size=F("blob_size") + F("draft_size"))
+ .annotate(
+ total_size=F("blob_size") + F("draft_size") + F("msg_count") * overhead
+ )📝 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.
| largest_threads = ( | |
| models.Thread.objects.filter(accesses__mailbox=mailbox) | |
| .annotate( | |
| blob_size=Coalesce(thread_size_subquery, Value(0)), | |
| draft_size=Coalesce(thread_draft_size_subquery, Value(0)), | |
| msg_count=Coalesce(thread_msg_count_subquery, Value(0)), | |
| access_read_at=read_at_subquery, | |
| ) | |
| .annotate(total_size=F("blob_size") + F("draft_size")) | |
| .order_by("-total_size")[:100] | |
| ) | |
| largest_threads = ( | |
| models.Thread.objects.filter(accesses__mailbox=mailbox) | |
| .annotate( | |
| blob_size=Coalesce(thread_size_subquery, Value(0)), | |
| draft_size=Coalesce(thread_draft_size_subquery, Value(0)), | |
| msg_count=Coalesce(thread_msg_count_subquery, Value(0)), | |
| access_read_at=read_at_subquery, | |
| ) | |
| .annotate( | |
| total_size=F("blob_size") + F("draft_size") + F("msg_count") * overhead | |
| ) | |
| .order_by("-total_size")[:100] | |
| ) |
🤖 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/api/viewsets/mailbox.py` around lines 241 - 251, Update the
`largest_threads` annotation in the mailbox storage calculation to include
per-message overhead using the existing `msg_count` annotation and overhead
value, matching `_storage_for_messages` and the documented per-thread sizing.
Keep the existing blob and draft size components and ordering intact.
| def get_mailbox_entitlements(self, mailbox, force_refresh=False): | ||
| account = { | ||
| "storage_used": get_mailbox_storage_used(mailbox), | ||
| "max_storage": self.mailbox_storage_limit, | ||
| } | ||
|
|
||
| organization = None | ||
| org_value = (mailbox.domain.custom_attributes or {}).get( | ||
| self.organization_claim | ||
| ) | ||
| if org_value: | ||
| organization = { | ||
| "storage_used": get_organization_storage_used( | ||
| self.organization_claim, org_value | ||
| ), | ||
| "max_storage": self.organization_storage_limit, | ||
| } | ||
|
|
||
| return {"account": account, "organization": organization} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align org_value truthiness check between the two entitlements backends. Both backends derive org_value the same way, but local.py uses a truthy check while deploycenter.py uses is not None; an empty-string organization claim would be handled differently depending on which backend is configured, breaking the interchangeability the EntitlementsBackend abstraction relies on.
src/backend/core/entitlements/backends/local.py#L48-L66: changeif org_value:toif org_value is not None:to matchdeploycenter.py.src/backend/core/entitlements/backends/deploycenter.py#L220-L248: no change needed; kept as the reference convention (already consistent with_build_usage_metrics'sorg_value is Nonecheck).
🐛 Proposed fix
--- a/src/backend/core/entitlements/backends/local.py
+++ b/src/backend/core/entitlements/backends/local.py
@@
- if org_value:
+ if org_value is not None:
organization = {📝 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.
| def get_mailbox_entitlements(self, mailbox, force_refresh=False): | |
| account = { | |
| "storage_used": get_mailbox_storage_used(mailbox), | |
| "max_storage": self.mailbox_storage_limit, | |
| } | |
| organization = None | |
| org_value = (mailbox.domain.custom_attributes or {}).get( | |
| self.organization_claim | |
| ) | |
| if org_value: | |
| organization = { | |
| "storage_used": get_organization_storage_used( | |
| self.organization_claim, org_value | |
| ), | |
| "max_storage": self.organization_storage_limit, | |
| } | |
| return {"account": account, "organization": organization} | |
| def get_mailbox_entitlements(self, mailbox, force_refresh=False): | |
| account = { | |
| "storage_used": get_mailbox_storage_used(mailbox), | |
| "max_storage": self.mailbox_storage_limit, | |
| } | |
| organization = None | |
| org_value = (mailbox.domain.custom_attributes or {}).get( | |
| self.organization_claim | |
| ) | |
| if org_value is not None: | |
| organization = { | |
| "storage_used": get_organization_storage_used( | |
| self.organization_claim, org_value | |
| ), | |
| "max_storage": self.organization_storage_limit, | |
| } | |
| return {"account": account, "organization": organization} |
📍 Affects 2 files
src/backend/core/entitlements/backends/local.py#L48-L66(this comment)src/backend/core/entitlements/backends/deploycenter.py#L220-L248
🤖 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/entitlements/backends/local.py` around lines 48 - 66, Update
get_mailbox_entitlements in src/backend/core/entitlements/backends/local.py
(lines 48-66) to check org_value with an explicit is not None comparison,
matching the existing convention in
src/backend/core/entitlements/backends/deploycenter.py (lines 220-248). No
change is needed in deploycenter.py.
| models.Index( | ||
| fields=["trashed_at", "created_at"], | ||
| name="msg_trashbin_cutoff_idx", | ||
| condition=models.Q(is_trashed=True) | models.Q(is_spam=True), | ||
| ), |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate models.py =="
fd -a 'models.py$' . | sed 's#^\./##' | head -50
echo "== inspect relevant lines in src/backend/core/models.py =="
wc -l src/backend/core/models.py
sed -n '2170,2225p' src/backend/core/models.py
echo "== search cleanup_trashbin_task =="
rg -n "cleanup_trashbin_task|trashed_at|is_trashed|is_spam|Coalesce|coalesce" src/backend -S
echo "== migrations mentioning msg_trashbin_cutoff_idx or Message indexing =="
rg -n "msg_trashbin_cutoff_idx|trashbin_cutoff_idx|is_trashed|is_spam|Model indexes" src/backend src/backend/migrations -S || true
echo "== git diff summary/name-only =="
git diff --stat || true
git diff --name-only || trueRepository: suitenumerique/messages
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== trashbin task implementation =="
sed -n '1,190p' src/backend/core/services/trashbin.py
echo "== migration creating the index =="
sed -n '1,140p' src/backend/core/migrations/0035_message_msg_trashbin_cutoff_idx.py
echo "== model imports relevant to Coalesce for expression indexes =="
sed -n '1,80p' src/backend/core/models.py
echo "== behavioral planner proxy: compare SQL generated for current fields index vs expression index with Django if available locally, otherwise use read-only AST/text parser =="
python3 - <<'PY'
import ast, pathlib, re
models = pathlib.Path('src/backend/core/models.py').read_text(encoding='utf-8')
tree = ast.parse(models)
for cls in tree.body:
if isinstance(cls, ast.ClassDef) and cls.name == 'Message':
indexes = []
for item in cls.body:
if isinstance(item, ast.Assign) and any(getattr(a,'id','')=='indexes' for a in item.targets):
for val in item.value.elements:
idx = {}
if isinstance(val, ast.Call) and getattr(val.func,'id','') == 'models.Index':
for kw in val.keywords:
if kw.arg == 'fields':
idx['fields'] = [elt.s if isinstance(elt, ast.Constant) else ast.unparse(elt) for elt in kw.value.elts]
elif kw.arg == 'name':
idx['name'] = kw.value.s
elif kw.arg == 'expression':
idx['expression'] = ast.unparse(kw.value)
indexes.append(idx)
print(indexes)
PY
echo "== Django version if installed =="
python3 - <<'PY'
try:
import django
print(django.VERSION)
except Exception as exc:
print('django_not_importable:', type(exc).__name__, str(exc))
PYRepository: suitenumerique/messages
Length of output: 11070
🌐 Web query:
Django expression Index Coalesce function fields vs expression documentation PostgreSQL functional index
💡 Result:
In Django, you can create functional indexes using the Index class by passing expressions (including database functions like Coalesce) as positional arguments, rather than using the fields argument [1][2]. Since Django 3.2, native support for functional indexes exists, rendering external packages like django-expression-index obsolete [3][4]. Key technical distinctions: 1. Fields vs. Expressions: The Index class accepts either fields (a list of field names) or expressions (positional arguments) [1][2]. When using fields, Django creates a standard index on those columns [2]. When using expressions, you enable functional indexes, which index the result of a database function (e.g., Coalesce, Lower) applied to your model fields [1][5]. 2. Coalesce in Functional Indexes: You can use Coalesce within an Index by passing it as a positional argument [6]. A common use case is to provide a non-null default value for an index on nullable fields [6]. Example usage: Index(Coalesce("field_name", Value("default_value")), name="coalesce_idx") [6]. 3. PostgreSQL Considerations: PostgreSQL requires that functions used in functional indexes be marked as IMMUTABLE [7]. While Coalesce is generally supported, be aware that complex expressions or those involving non-immutable data may fail at the database level [7]. Additionally, using Coalesce in certain contexts (like full-text search) may inadvertently block the usage of indexes if the optimizer cannot determine that the result is equivalent to the underlying column or if nullability handling differs [8]. 4. Documentation: The official Django Model index reference documentation confirms that the *expressions positional argument is the correct way to implement indexes on database functions [1][5]. For the behavior of Coalesce itself, refer to the Database Functions documentation [9][10][11]. Note: When using complex expressions in indexes, ensure your migrations are stable. Some versions of Django have had issues with redundant migration generation when using Coalesce within indexes, particularly when the output_field type is implicitly or explicitly defined [12].
Citations:
- 1: https://docs.djangoproject.com/en/5.2/ref/models/indexes/
- 2: https://docs.djangoproject.com/en/6.0/ref/models/indexes/
- 3: https://github.com/kmierzeje/django-expression-index
- 4: https://pypi.org/project/django-expression-index/
- 5: https://github.com/django/django/blob/master/docs/ref/models/indexes.txt
- 6: https://stackoverflow.com/questions/69713468/django-add-a-unique-index-on-expressions-in-postgres
- 7: https://code.djangoproject.com/ticket/26167?cnum_hist=7&cversion=0
- 8: https://code.djangoproject.com/ticket/31304
- 9: https://docs.djangoproject.com/en/6.1/ref/models/database-functions/
- 10: https://docs.djangoproject.com/en/5.2/ref/models/database-functions/
- 11: https://docs.djangoproject.com/en/6.0/ref/models/database-functions/
- 12: https://code.djangoproject.com/ticket/36199
Index the coalesced trashbin timestamp.
cleanup_trashbin_task filters Coalesce("trashed_at", "created_at") < cutoff, but msg_trashbin_cutoff_idx indexes the raw columns. A b-tree on trashed_at, created_at does not satisfy the coalesced range predicate, so the daily sweep may still scan broader index matching rows. Use an expression index for Coalesce("trashed_at", "created_at") with the same partial condition and add/update the migration.
🤖 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 2203 - 2207, Update
msg_trashbin_cutoff_idx to use an expression index on Coalesce("trashed_at",
"created_at") while preserving the existing is_trashed/is_spam partial
condition, and add the corresponding database migration.
Source: Coding guidelines
| while True: | ||
| batch_ids = list(base_qs.values_list("pk", flat=True)[:batch_size]) | ||
| if not batch_ids: | ||
| break | ||
| # Collected before the delete: the ThreadAccess rows are unreachable | ||
| # through the messages once they are gone. | ||
| touched_mailbox_ids.update( | ||
| models.ThreadAccess.objects.filter( | ||
| thread__messages__pk__in=batch_ids | ||
| ).values_list("mailbox_id", flat=True) | ||
| ) | ||
| deleted_count += permanently_delete_messages( | ||
| models.Message.objects.filter(pk__in=batch_ids) | ||
| ) | ||
|
|
||
| invalidate_mailbox_storage_ids(touched_mailbox_ids) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
Cache invalidation is skipped when a batch fails mid-sweep.
invalidate_mailbox_storage_ids(touched_mailbox_ids) runs only after the loop completes. If one batch raises (e.g. ProtectedError/DB error), already-deleted batches leave stale cached usage until the TTL expires, and the retry won't know the earlier mailbox ids. Invalidating per batch (or in a try/finally) keeps the gauge consistent with what was actually deleted.
♻️ Proposed change
- while True:
- batch_ids = list(base_qs.values_list("pk", flat=True)[:batch_size])
- if not batch_ids:
- break
- # Collected before the delete: the ThreadAccess rows are unreachable
- # through the messages once they are gone.
- touched_mailbox_ids.update(
- models.ThreadAccess.objects.filter(
- thread__messages__pk__in=batch_ids
- ).values_list("mailbox_id", flat=True)
- )
- deleted_count += permanently_delete_messages(
- models.Message.objects.filter(pk__in=batch_ids)
- )
-
- invalidate_mailbox_storage_ids(touched_mailbox_ids)
+ try:
+ while True:
+ batch_ids = list(base_qs.values_list("pk", flat=True)[:batch_size])
+ if not batch_ids:
+ break
+ # Collected before the delete: the ThreadAccess rows are unreachable
+ # through the messages once they are gone.
+ batch_mailbox_ids = set(
+ models.ThreadAccess.objects.filter(
+ thread__messages__pk__in=batch_ids
+ ).values_list("mailbox_id", flat=True)
+ )
+ touched_mailbox_ids.update(batch_mailbox_ids)
+ deleted_count += permanently_delete_messages(
+ models.Message.objects.filter(pk__in=batch_ids)
+ )
+ invalidate_mailbox_storage_ids(batch_mailbox_ids)
+ finally:
+ invalidate_mailbox_storage_ids(touched_mailbox_ids)📝 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.
| while True: | |
| batch_ids = list(base_qs.values_list("pk", flat=True)[:batch_size]) | |
| if not batch_ids: | |
| break | |
| # Collected before the delete: the ThreadAccess rows are unreachable | |
| # through the messages once they are gone. | |
| touched_mailbox_ids.update( | |
| models.ThreadAccess.objects.filter( | |
| thread__messages__pk__in=batch_ids | |
| ).values_list("mailbox_id", flat=True) | |
| ) | |
| deleted_count += permanently_delete_messages( | |
| models.Message.objects.filter(pk__in=batch_ids) | |
| ) | |
| invalidate_mailbox_storage_ids(touched_mailbox_ids) | |
| try: | |
| while True: | |
| batch_ids = list(base_qs.values_list("pk", flat=True)[:batch_size]) | |
| if not batch_ids: | |
| break | |
| # Collected before the delete: the ThreadAccess rows are unreachable | |
| # through the messages once they are gone. | |
| batch_mailbox_ids = set( | |
| models.ThreadAccess.objects.filter( | |
| thread__messages__pk__in=batch_ids | |
| ).values_list("mailbox_id", flat=True) | |
| ) | |
| touched_mailbox_ids.update(batch_mailbox_ids) | |
| deleted_count += permanently_delete_messages( | |
| models.Message.objects.filter(pk__in=batch_ids) | |
| ) | |
| invalidate_mailbox_storage_ids(batch_mailbox_ids) | |
| finally: | |
| invalidate_mailbox_storage_ids(touched_mailbox_ids) |
🤖 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/trashbin.py` around lines 160 - 175, Update the
batch sweep around permanently_delete_messages so mailbox storage cache
invalidation occurs for each successfully processed batch, or is guaranteed via
try/finally when the loop aborts. Preserve touched_mailbox_ids for all completed
deletions so a later failure cannot skip invalidation of earlier batches.
| # Who may manually empty the trashbin: "never" (only the cutoff sweep | ||
| # deletes), "admins" (mailbox role ADMIN), or "editors" (role >= EDITOR). | ||
| # Validated on read, against ``core.enums.TrashbinAllowEmpty`` — an | ||
| # unrecognised value raises rather than silently meaning "nobody". | ||
| TRASHBIN_ALLOW_EMPTY = values.Value( | ||
| "admins", environ_name="TRASHBIN_ALLOW_EMPTY", environ_prefix=None | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate TRASHBIN_ALLOW_EMPTY during startup. The application currently boots with an invalid policy and raises only when Mailbox.get_abilities() is later evaluated; that can turn a configuration error into request-time failures.
src/backend/messages/settings.py#L1082-L1088: validate againstTrashbinAllowEmptyin startup configuration (while retaining runtime validation ifoverride_settingssupport is required).docs/env.md#L413-L413: keep the startup-rejection claim only once the setting is validated during startup.
📍 Affects 2 files
src/backend/messages/settings.py#L1082-L1088(this comment)docs/env.md#L413-L413
🤖 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/messages/settings.py` around lines 1082 - 1088, The
TRASHBIN_ALLOW_EMPTY setting currently defers invalid-value detection until
runtime; validate it against TrashbinAllowEmpty during startup configuration
while retaining runtime validation if override_settings requires it. Update
src/backend/messages/settings.py lines 1082-1088 at TRASHBIN_ALLOW_EMPTY, and
update docs/env.md line 413 to retain the startup-rejection claim only after
startup validation is implemented.
| "You and {{assignees}} were unassigned_other": "Vous et {{assignees}} avez été désassigné·e·s", | ||
| "You and all users with access to the mailbox \"{{mailboxName}}\" will no longer see this thread.": "Vous et tous les utilisateurs avec un accès à la boîte « {{mailboxName}} » ne pourront plus voir cette conversation.", | ||
| "You are about to leave this page and be redirected to:": "Vous êtes sur le point de quitter cette page et d'être redirigé vers :", | ||
| "You are about to permanently delete every message in the spam folder. This cannot be undone.": "Vous êtes sur le point de supprimer définitivement tous les messages du dossier indésirables. Cette action est irréversible.", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the French spam-folder label.
Use “dossier des indésirables” rather than “dossier indésirables.”
Proposed fix
- "You are about to permanently delete every message in the spam folder. This cannot be undone.": "Vous êtes sur le point de supprimer définitivement tous les messages du dossier indésirables. Cette action est irréversible.",
+ "You are about to permanently delete every message in the spam folder. This cannot be undone.": "Vous êtes sur le point de supprimer définitivement tous les messages du dossier des indésirables. Cette action est irréversible.",📝 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.
| "You are about to permanently delete every message in the spam folder. This cannot be undone.": "Vous êtes sur le point de supprimer définitivement tous les messages du dossier indésirables. Cette action est irréversible.", | |
| "You are about to permanently delete every message in the spam folder. This cannot be undone.": "Vous êtes sur le point de supprimer définitivement tous les messages du dossier des indésirables. Cette action est irréversible.", |
🤖 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/frontend/public/locales/common/fr-FR.json` at line 1043, Update the
French translation for “You are about to permanently delete every message in the
spam folder. This cannot be undone.” to use “dossier des indésirables” instead
of “dossier indésirables,” preserving the rest of the translation.
| <span | ||
| className="mailbox-settings__storage-item-status" | ||
| aria-label={thread.is_unread ? t("Unread") : t("Read")} | ||
| /> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
aria-label on a plain <span> is not exposed to assistive tech.
The status dot has no role, so the read/unread label is dropped by most screen readers. Give the element a role (role="img") or move the text into a visually hidden child.
♿ Proposed fix
<span
className="mailbox-settings__storage-item-status"
+ role="img"
aria-label={thread.is_unread ? t("Unread") : t("Read")}
/>📝 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.
| <span | |
| className="mailbox-settings__storage-item-status" | |
| aria-label={thread.is_unread ? t("Unread") : t("Read")} | |
| /> | |
| <span | |
| className="mailbox-settings__storage-item-status" | |
| role="img" | |
| aria-label={thread.is_unread ? t("Unread") : t("Read")} | |
| /> |
🤖 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/frontend/src/features/layouts/components/mailbox-settings/modal-mailbox-settings/storage-tab/index.tsx`
around lines 198 - 201, Update the status indicator span in the mailbox settings
storage item to expose its existing read/unread aria-label to assistive
technology by adding role="img" (or an equivalent visually hidden text child),
while preserving the current label selection.
| onSuccess: (response) => { | ||
| const data = response.data as { deleted_count?: number }; | ||
| const deletedCount = data?.deleted_count ?? 0; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Prefer the generated response type over a manual cast.
response.data as { deleted_count?: number } bypasses whatever type useMailboxesEmptyTrashCreate actually generates from the OpenAPI schema. If that schema already types the 200 response with deleted_count (matching the backend's Response({"success": True, "deleted_count": deleted_count})), using it directly avoids the cast silently masking any future contract drift.
#!/bin/bash
# Check the generated response type for the empty-trash mutation.
rg -n -A 15 'useMailboxesEmptyTrashCreate' --type=ts src/frontend/src/features/api/gen 2>/dev/null || \
fd -e ts . src/frontend/src/features/api/gen --exec rg -l 'EmptyTrash'🤖 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/frontend/src/features/message/use-empty-trash.tsx` around lines 81 - 83,
Update the onSuccess handler in use-empty-trash.tsx to use the generated
response type from useMailboxesEmptyTrashCreate directly, removing the manual `{
deleted_count?: number }` cast. Access deleted_count through response.data while
preserving the existing nullish fallback to 0.
1 participant
Summary by CodeRabbit