✨(threadevent) add ThreadEvents and Webhooks #383
Conversation
WalkthroughAdds ThreadEvent model, REST endpoints, serializers, and admin inline; implements webhook inbound endpoints with API-key authentication; changes deliver_inbound_message to return Message|None and updates callers/tests; integrates thread events into frontend timeline and adds ThreadEvent UI component and styles. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WebhookAuth as WebhookAuthentication
participant InboundWebhook as InboundWebhookViewSet
participant Delivery as deliver_inbound_message
participant DB as Database
Client->>WebhookAuth: POST /inbound/webhook/message (X-Channel-ID, X-API-Key)
WebhookAuth->>DB: Fetch Channel by ID
WebhookAuth->>WebhookAuth: _authenticate_api_key
alt API Key valid
WebhookAuth->>InboundWebhook: succeed (user-like payload)
else Invalid/Missing
WebhookAuth-->>Client: 401/403
end
InboundWebhook->>InboundWebhook: Validate payload (from, message, subject)
InboundWebhook->>Delivery: deliver_inbound_message(parsed payload, channel)
Delivery->>DB: Create or retrieve Message/Thread
alt Success
Delivery-->>InboundWebhook: Message object
InboundWebhook-->>Client: 200/201 with ids
else Failure
Delivery-->>InboundWebhook: None
InboundWebhook-->>Client: 500
end
sequenceDiagram
participant Client
participant ThreadEventVS as ThreadEventViewSet
participant Permission as IsAllowedToAccess
participant DB as Database
Client->>ThreadEventVS: POST /threads/{thread_id}/events (auth)
ThreadEventVS->>Permission: has_permission(thread_id from URL)
alt Allowed
Permission-->>ThreadEventVS: True
ThreadEventVS->>DB: Create ThreadEvent (thread, type, data, channel?)
ThreadEventVS-->>Client: 201 {id, thread, type, ...}
else Denied
Permission-->>Client: 403
end
sequenceDiagram
participant UI
participant Provider as MailboxProvider
participant API as Thread Events API
participant ThreadView as ThreadView Component
UI->>Provider: select thread
Provider->>API: fetch thread events (useThreadsEventsList)
API-->>Provider: [ThreadEvent...]
Provider->>Provider: merge messages + events => threadItems (sorted by created_at)
Provider->>ThreadView: pass threadItems
ThreadView->>ThreadView: render timeline: message OR event (iframe/JSON)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Pylint (4.0.3)src/backend/core/admin.pysrc/backend/core/api/viewsets/thread_event.pysrc/backend/core/api/serializers.py
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
compose.yaml(1 hunks)src/backend/core/api/viewsets/inbound/webhook.py(1 hunks)src/backend/core/tests/api/test_inbound_webhook.py(1 hunks)src/backend/core/urls.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/backend/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/*.py: Follow Django/PEP 8 style with a 100-character line limit
Use descriptive, snake_case names for variables and functions
Use Django ORM for database access; avoid raw SQL unless necessary for performance
Use Django’s built-in user model and authentication framework
Prefer try-except blocks to handle exceptions in business logic and views
Log expected and unexpected actions with appropriate log levels
Capture and report exceptions to Sentry; use capture_exception() for custom errors
Do not log sensitive information (tokens, passwords, financial/health data, PII)
Files:
src/backend/core/urls.pysrc/backend/core/tests/api/test_inbound_webhook.pysrc/backend/core/api/viewsets/inbound/webhook.py
src/backend/**/urls.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Define clear, RESTful URL patterns using Django’s URL dispatcher
Files:
src/backend/core/urls.py
src/backend/**/{tests.py,tests/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/{tests.py,tests/**/*.py}: Use Django’s testing tools (pytest-django) to ensure code quality and reliability
Unit tests should focus on a single use case, keep assertions minimal, and cover all possible cases
Files:
src/backend/core/tests/api/test_inbound_webhook.py
🧠 Learnings (1)
📚 Learning: 2025-09-02T10:12:12.835Z
Learnt from: CR
PR: suitenumerique/messages#0
File: .cursor/rules/django-python.mdc:0-0
Timestamp: 2025-09-02T10:12:12.835Z
Learning: Applies to src/backend/**/views.py : Use Django REST Framework viewsets for API endpoints
Applied to files:
src/backend/core/urls.py
🧬 Code graph analysis (3)
src/backend/core/urls.py (1)
src/backend/core/api/viewsets/inbound/webhook.py (1)
InboundWebhookViewSet(73-210)
src/backend/core/tests/api/test_inbound_webhook.py (2)
src/backend/core/api/viewsets/inbound/webhook.py (3)
WebhookAuthentication(26-70)authenticate(33-50)authenticate_header(68-70)src/backend/core/factories.py (2)
MailboxFactory(60-96)ChannelFactory(249-258)
src/backend/core/api/viewsets/inbound/webhook.py (4)
src/backend/core/api/permissions.py (1)
IsAuthenticated(18-25)src/backend/core/mda/inbound.py (1)
deliver_inbound_message(332-736)src/backend/core/mda/rfc5322/composer.py (1)
compose_email(481-556)src/backend/core/models.py (1)
Channel(418-469)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: docker-publish-socks-proxy / docker-build-push
- GitHub Check: docker-publish-mta-in / docker-build-push
- GitHub Check: build-front
- GitHub Check: check-api-state
- GitHub Check: test-back
- GitHub Check: lint-back
| try: | ||
| channel = models.Channel.objects.get(id=channel_id) | ||
| except models.Channel.DoesNotExist as e: | ||
| raise AuthenticationFailed("Invalid channel ID") from e | ||
|
|
||
| # Get authentication method from channel settings | ||
| auth_method = (channel.settings or {}).get("auth_method", "api_key") | ||
|
|
||
| if auth_method == "api_key": | ||
| return self._authenticate_api_key(request, channel) | ||
|
|
||
| raise AuthenticationFailed(f"Unsupported authentication method: {auth_method}") | ||
|
|
||
| def _authenticate_api_key(self, request, channel): |
There was a problem hiding this comment.
Handle invalid channel IDs without 500s
Line 40 raises ValueError when X-Channel-ID isn’t a valid UUID, causing a 500 instead of an auth failure. Wrap the lookup to catch ValueError alongside DoesNotExist and re-raise AuthenticationFailed.
Apply this diff:
- try:
- channel = models.Channel.objects.get(id=channel_id)
- except models.Channel.DoesNotExist as e:
- raise AuthenticationFailed("Invalid channel ID") from e
+ try:
+ channel = models.Channel.objects.get(id=channel_id)
+ except (ValueError, models.Channel.DoesNotExist) as exc:
+ raise AuthenticationFailed("Invalid channel ID") from exc🤖 Prompt for AI Agents
In src/backend/core/api/viewsets/inbound/webhook.py around lines 39 to 52, the
Channel lookup currently only catches models.Channel.DoesNotExist, but when
X-Channel-ID is not a valid UUID the lookup raises ValueError and returns a 500;
update the exception handling to catch both models.Channel.DoesNotExist and
ValueError and re-raise AuthenticationFailed (preserving the original exception
via "from e") so invalid or malformed channel IDs result in an auth failure
instead of an internal server error.
| def sanitize_header(header: str) -> str: | ||
| return header.replace("\r", "").replace("\n", "")[0:1000] | ||
|
|
||
| # Add webhook-specific headers | ||
| prepend_headers = [("X-StMsg-Sender-Auth", "webhook")] | ||
|
|
||
| # Add source information | ||
| if request.META.get("HTTP_USER_AGENT"): | ||
| prepend_headers.append( | ||
| ( | ||
| "X-StMsg-Webhook-User-Agent", | ||
| sanitize_header(request.META.get("HTTP_USER_AGENT")), | ||
| ) | ||
| ) | ||
|
|
||
| if request.META.get("HTTP_REFERER"): | ||
| prepend_headers.append( | ||
| ( | ||
| "X-StMsg-Webhook-Referer", | ||
| sanitize_header(request.META.get("HTTP_REFERER")), | ||
| ) | ||
| ) | ||
|
|
||
| prepend_headers.append( | ||
| ( | ||
| "Received", | ||
| f"from webhook ({sanitize_header(request.META.get('REMOTE_ADDR'))})", | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Guard sanitize_header against missing values
sanitize_header(request.META.get("REMOTE_ADDR")) blows up when the header is absent (None has no replace), leading to a 500. Default to an empty string (or sentinel) before sanitizing.
Recommend:
- def sanitize_header(header: str) -> str:
- return header.replace("\r", "").replace("\n", "")[0:1000]
+ def sanitize_header(header: str | None) -> str:
+ sanitized = (header or "").replace("\r", "").replace("\n", "")
+ return sanitized[:1000]📝 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 sanitize_header(header: str) -> str: | |
| return header.replace("\r", "").replace("\n", "")[0:1000] | |
| # Add webhook-specific headers | |
| prepend_headers = [("X-StMsg-Sender-Auth", "webhook")] | |
| # Add source information | |
| if request.META.get("HTTP_USER_AGENT"): | |
| prepend_headers.append( | |
| ( | |
| "X-StMsg-Webhook-User-Agent", | |
| sanitize_header(request.META.get("HTTP_USER_AGENT")), | |
| ) | |
| ) | |
| if request.META.get("HTTP_REFERER"): | |
| prepend_headers.append( | |
| ( | |
| "X-StMsg-Webhook-Referer", | |
| sanitize_header(request.META.get("HTTP_REFERER")), | |
| ) | |
| ) | |
| prepend_headers.append( | |
| ( | |
| "Received", | |
| f"from webhook ({sanitize_header(request.META.get('REMOTE_ADDR'))})", | |
| ) | |
| ) | |
| def sanitize_header(header: str | None) -> str: | |
| sanitized = (header or "").replace("\r", "").replace("\n", "") | |
| return sanitized[:1000] | |
| # Add webhook-specific headers | |
| prepend_headers = [("X-StMsg-Sender-Auth", "webhook")] | |
| # Add source information | |
| if request.META.get("HTTP_USER_AGENT"): | |
| prepend_headers.append( | |
| ( | |
| "X-StMsg-Webhook-User-Agent", | |
| sanitize_header(request.META.get("HTTP_USER_AGENT")), | |
| ) | |
| ) | |
| if request.META.get("HTTP_REFERER"): | |
| prepend_headers.append( | |
| ( | |
| "X-StMsg-Webhook-Referer", | |
| sanitize_header(request.META.get("HTTP_REFERER")), | |
| ) | |
| ) | |
| prepend_headers.append( | |
| ( | |
| "Received", | |
| f"from webhook ({sanitize_header(request.META.get('REMOTE_ADDR'))})", | |
| ) | |
| ) |
🤖 Prompt for AI Agents
In src/backend/core/api/viewsets/inbound/webhook.py around lines 145 to 173,
sanitize_header is called with request.META.get(...) values that may be None,
causing AttributeError; change the code to ensure sanitize_header always
receives a string (e.g., pass request.META.get('REMOTE_ADDR') or '' via or ""),
or update sanitize_header to accept Optional[str] and coerce None to '' before
calling replace and slicing; apply the same guarding to all places
sanitize_header is called (USER_AGENT, HTTP_REFERER, REMOTE_ADDR) so missing
headers no longer raise a 500.
sylvinus
changed the title
sylvinus
changed the title
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
src/backend/core/api/viewsets/inbound/webhook.py (2)
39-44: Catch ValueError for malformed channel IDs
models.Channel.objects.get(id=channel_id)raisesValueErrorwhenX-Channel-IDisn’t a valid UUID. That currently bubbles up as a 500 instead of an auth failure. Please treatValueErrorthe same asDoesNotExistso malformed headers are rejected cleanly.- try: - channel = models.Channel.objects.get(id=channel_id) - except models.Channel.DoesNotExist as e: - raise AuthenticationFailed("Invalid channel ID") from e + try: + channel = models.Channel.objects.get(id=channel_id) + except (ValueError, models.Channel.DoesNotExist) as exc: + raise AuthenticationFailed("Invalid channel ID") from exc
145-156: Harden sanitize_header against missing META entries
request.META.get("REMOTE_ADDR")can beNone, andNone.replace(...)raisesAttributeError, turning unauthenticated hits into 500s. CoerceNoneto an empty string before sanitizing.- def sanitize_header(header: str) -> str: - return header.replace("\r", "").replace("\n", "")[0:1000] + def sanitize_header(header: str | None) -> str: + sanitized = (header or "").replace("\r", "").replace("\n", "") + return sanitized[:1000]
🧹 Nitpick comments (7)
src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx (1)
66-69: Add error handling for JSON serialization.
JSON.stringifycan throw on circular references or non-serializable values. While unlikely with API data, defensive error handling improves robustness.Apply this diff:
<div className="thread-event__content"> <pre className="thread-event__json"> - {JSON.stringify(data, null, 2)} + {(() => { + try { + return JSON.stringify(data, null, 2); + } catch (error) { + return t("Unable to display event data"); + } + })()} </pre> </div>src/frontend/src/features/layouts/components/thread-view/index.tsx (1)
24-28: Consider consolidating the TimelineItem type definition.The
TimelineItemtype is defined both here and insrc/frontend/src/features/providers/mailbox.tsx. While the definitions are currently identical, maintaining them separately risks inconsistency as the codebase evolves.Consider extracting this type to a shared location (e.g.,
src/frontend/src/features/api/gen/modelsor a dedicated types file) and importing it in both locations.src/backend/core/api/openapi.json (5)
4760-4855: New list/create endpoints look coherent; minor polish suggested.
- Consider pluralizing the tag to "thread-events" for UI grouping consistency.
- Optionally add filters (e.g., type=..., channel=...) and ordering params.
4805-4855: Define shape for request.data and document allowed type values.
- ThreadEventCreateRequest.data is untyped (any). Prefer
"type": "object", "additionalProperties": trueand add an example.- If event types are finite, expose an enum or reference a Type schema to help clients validate.
7597-7643: Make thread/channel readOnly; keep type readOnly but required is fine (minor codegen nit).
- Suggest marking thread and channel as readOnly to reflect immutability post-create.
- Optional: consider adding
"description"for type and a small enum or naming convention.
7644-7690: Create response is fine; small doc tweak.
- Required includes channel while property is readOnly; acceptable for responses, but you may drop it from "required" to avoid client confusion.
7691-7705: Create request is minimal and clear.
- Consider typing data as object and adding an example payload for generators.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
src/frontend/src/features/api/gen/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/paginated_thread_event_list.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/patched_thread_event_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event_create.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event_create_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/threads_events_list_params.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/thread-event/thread-event.tsis excluded by!**/gen/**
📒 Files selected for processing (27)
src/backend/core/admin.py(1 hunks)src/backend/core/api/openapi.json(4 hunks)src/backend/core/api/permissions.py(2 hunks)src/backend/core/api/serializers.py(1 hunks)src/backend/core/api/viewsets/inbound/mta.py(1 hunks)src/backend/core/api/viewsets/inbound/webhook.py(1 hunks)src/backend/core/api/viewsets/inbound/widget.py(1 hunks)src/backend/core/api/viewsets/thread_event.py(1 hunks)src/backend/core/factories.py(2 hunks)src/backend/core/mda/inbound.py(7 hunks)src/backend/core/mda/outbound.py(1 hunks)src/backend/core/migrations/0012_threadevent.py(1 hunks)src/backend/core/models.py(1 hunks)src/backend/core/tests/api/test_inbound_mta.py(3 hunks)src/backend/core/tests/api/test_inbound_webhook.py(1 hunks)src/backend/core/tests/api/test_inbound_widget.py(3 hunks)src/backend/core/tests/api/test_thread_event.py(1 hunks)src/backend/core/tests/importer/test_import_service.py(2 hunks)src/backend/core/tests/mda/test_inbound.py(17 hunks)src/backend/core/tests/tasks/test_task_importer.py(2 hunks)src/backend/core/urls.py(5 hunks)src/backend/messages/settings.py(1 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-event/_index.scss(1 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx(1 hunks)src/frontend/src/features/layouts/components/thread-view/index.tsx(7 hunks)src/frontend/src/features/providers/mailbox.tsx(8 hunks)src/frontend/src/styles/main.scss(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/backend/core/tests/api/test_inbound_webhook.py
🧰 Additional context used
📓 Path-based instructions (8)
src/backend/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/*.py: Follow Django/PEP 8 style with a 100-character line limit
Use descriptive, snake_case names for variables and functions
Use Django ORM for database access; avoid raw SQL unless necessary for performance
Use Django’s built-in user model and authentication framework
Prefer try-except blocks to handle exceptions in business logic and views
Log expected and unexpected actions with appropriate log levels
Capture and report exceptions to Sentry; use capture_exception() for custom errors
Do not log sensitive information (tokens, passwords, financial/health data, PII)
Files:
src/backend/core/tests/importer/test_import_service.pysrc/backend/messages/settings.pysrc/backend/core/mda/outbound.pysrc/backend/core/migrations/0012_threadevent.pysrc/backend/core/admin.pysrc/backend/core/api/viewsets/inbound/widget.pysrc/backend/core/tests/tasks/test_task_importer.pysrc/backend/core/urls.pysrc/backend/core/tests/api/test_inbound_mta.pysrc/backend/core/api/viewsets/thread_event.pysrc/backend/core/tests/api/test_thread_event.pysrc/backend/core/api/viewsets/inbound/mta.pysrc/backend/core/mda/inbound.pysrc/backend/core/api/permissions.pysrc/backend/core/tests/api/test_inbound_widget.pysrc/backend/core/api/serializers.pysrc/backend/core/tests/mda/test_inbound.pysrc/backend/core/api/viewsets/inbound/webhook.pysrc/backend/core/factories.pysrc/backend/core/models.py
src/backend/**/{tests.py,tests/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/{tests.py,tests/**/*.py}: Use Django’s testing tools (pytest-django) to ensure code quality and reliability
Unit tests should focus on a single use case, keep assertions minimal, and cover all possible cases
Files:
src/backend/core/tests/importer/test_import_service.pysrc/backend/core/tests/tasks/test_task_importer.pysrc/backend/core/tests/api/test_inbound_mta.pysrc/backend/core/tests/api/test_thread_event.pysrc/backend/core/tests/api/test_inbound_widget.pysrc/backend/core/tests/mda/test_inbound.py
src/backend/**/{settings.py,middleware.py}
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Use middleware judiciously for cross-cutting concerns (authentication, logging, caching)
Files:
src/backend/messages/settings.py
src/backend/**/settings.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/settings.py: Leverage Django’s caching framework (e.g., Redis/Memcached) where appropriate
Use Django’s cache framework with a backend like Redis or Memcached to reduce DB load
Optimize static file handling using Django’s staticfiles pipeline (e.g., WhiteNoise)
Files:
src/backend/messages/settings.py
src/backend/**/{models.py,migrations/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Implement database indexing and query optimization (Model Meta indexes, constraints)
Files:
src/backend/core/migrations/0012_threadevent.pysrc/backend/core/models.py
src/backend/**/urls.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Define clear, RESTful URL patterns using Django’s URL dispatcher
Files:
src/backend/core/urls.py
src/backend/{templates/**/*.html,**/serializers.py}
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Use Django templates for HTML and DRF serializers for JSON responses
Files:
src/backend/core/api/serializers.py
src/backend/**/{models.py,forms.py,views.py}
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Keep business logic in models and forms; keep views thin and focused on request handling
Files:
src/backend/core/models.py
🧠 Learnings (4)
📚 Learning: 2025-09-02T10:12:12.835Z
Learnt from: CR
Repo: suitenumerique/messages PR: 0
File: .cursor/rules/django-python.mdc:0-0
Timestamp: 2025-09-02T10:12:12.835Z
Learning: Apply Django security best practices (CSRF, SQL injection protection, XSS prevention)
Applied to files:
src/backend/messages/settings.py
📚 Learning: 2025-09-02T10:12:12.835Z
Learnt from: CR
Repo: suitenumerique/messages PR: 0
File: .cursor/rules/django-python.mdc:0-0
Timestamp: 2025-09-02T10:12:12.835Z
Learning: Applies to src/backend/**/urls.py : Define clear, RESTful URL patterns using Django’s URL dispatcher
Applied to files:
src/backend/core/urls.py
📚 Learning: 2025-09-02T10:12:12.835Z
Learnt from: CR
Repo: suitenumerique/messages PR: 0
File: .cursor/rules/django-python.mdc:0-0
Timestamp: 2025-09-02T10:12:12.835Z
Learning: Applies to src/backend/**/views.py : Use Django REST Framework viewsets for API endpoints
Applied to files:
src/backend/core/urls.pysrc/backend/core/api/viewsets/thread_event.py
📚 Learning: 2025-09-02T10:12:12.835Z
Learnt from: CR
Repo: suitenumerique/messages PR: 0
File: .cursor/rules/django-python.mdc:0-0
Timestamp: 2025-09-02T10:12:12.835Z
Learning: Applies to src/backend/**/{tests.py,tests/**/*.py} : Use Django’s testing tools (pytest-django) to ensure code quality and reliability
Applied to files:
src/backend/core/tests/api/test_thread_event.py
🧬 Code graph analysis (20)
src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx (2)
src/backend/core/models.py (1)
ThreadEvent(1126-1150)src/frontend/src/features/api/gen/models/thread_event.ts (1)
ThreadEvent(12-23)
src/backend/core/mda/outbound.py (1)
src/backend/core/mda/inbound.py (1)
deliver_inbound_message(332-740)
src/backend/core/migrations/0012_threadevent.py (1)
src/backend/core/api/fields.py (1)
JSONField(8-25)
src/backend/core/admin.py (1)
src/backend/core/models.py (2)
ThreadEvent(1126-1150)Thread(742-895)
src/backend/core/api/viewsets/inbound/widget.py (2)
src/backend/core/mda/inbound.py (1)
deliver_inbound_message(332-740)src/backend/core/mda/rfc5322/composer.py (1)
compose_email(481-556)
src/backend/core/urls.py (3)
src/backend/core/api/viewsets/inbound/webhook.py (1)
InboundWebhookViewSet(73-285)src/backend/core/api/viewsets/thread_event.py (1)
ThreadEventViewSet(12-57)src/backend/core/api/viewsets/thread_access.py (1)
ThreadAccessViewSet(27-64)
src/backend/core/tests/api/test_inbound_mta.py (2)
src/backend/core/tests/importer/test_import_service.py (2)
mock_deliver(159-161)mock_deliver(261-263)src/backend/core/tests/tasks/test_task_importer.py (1)
mock_deliver(203-212)
src/backend/core/api/viewsets/thread_event.py (3)
src/backend/core/api/serializers.py (2)
ThreadEventSerializer(815-855)ThreadEventCreateSerializer(795-812)src/backend/core/api/permissions.py (2)
IsAuthenticated(18-25)IsAllowedToAccess(92-186)src/backend/core/models.py (8)
ThreadEvent(1126-1150)Thread(742-895)save(92-95)save(207-210)save(291-294)save(954-1006)save(1528-1531)save(1784-1800)
src/backend/core/tests/api/test_thread_event.py (2)
src/backend/core/factories.py (7)
UserFactory(20-31)MailboxFactory(60-96)MailboxAccessFactory(99-109)ThreadFactory(137-144)ThreadAccessFactory(147-157)ThreadEventFactory(160-171)ChannelFactory(125-134)src/backend/core/models.py (2)
ThreadEvent(1126-1150)delete(1078-1098)
src/frontend/src/features/layouts/components/thread-view/index.tsx (2)
src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx (1)
ThreadEvent(20-73)src/frontend/src/features/api/gen/models/thread_event.ts (1)
ThreadEvent(12-23)
src/backend/core/api/viewsets/inbound/mta.py (2)
src/backend/core/api/viewsets/inbound/webhook.py (1)
message(90-197)src/backend/core/mda/inbound.py (1)
deliver_inbound_message(332-740)
src/backend/core/mda/inbound.py (2)
src/backend/core/models.py (1)
Message(1215-1379)src/backend/core/api/viewsets/inbound/webhook.py (1)
message(90-197)
src/backend/core/api/permissions.py (1)
src/backend/core/models.py (6)
Mailbox(472-696)ThreadAccess(1101-1123)MailboxAccess(699-739)Message(1215-1379)Thread(742-895)ThreadEvent(1126-1150)
src/backend/core/tests/api/test_inbound_widget.py (2)
src/backend/core/tests/importer/test_import_service.py (2)
mock_deliver(159-161)mock_deliver(261-263)src/backend/core/tests/tasks/test_task_importer.py (1)
mock_deliver(203-212)
src/backend/core/api/serializers.py (1)
src/backend/core/models.py (17)
Meta(89-90)Meta(199-202)Meta(283-286)Meta(454-466)Meta(501-506)Meta(718-722)Meta(761-764)Meta(944-949)Meta(1116-1120)Meta(1143-1147)Meta(1164-1168)Meta(1205-1209)Meta(1280-1284)Meta(1511-1523)Meta(1579-1583)Meta(1619-1623)ThreadEvent(1126-1150)
src/backend/core/tests/mda/test_inbound.py (2)
src/backend/core/api/viewsets/inbound/webhook.py (1)
message(90-197)src/backend/core/mda/inbound.py (1)
deliver_inbound_message(332-740)
src/frontend/src/features/providers/mailbox.tsx (3)
src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx (1)
ThreadEvent(20-73)src/frontend/src/features/api/gen/models/thread_event.ts (1)
ThreadEvent(12-23)src/frontend/src/features/api/gen/thread-event/thread-event.ts (2)
useThreadsEventsList(216-250)getThreadsEventsListQueryKey(85-93)
src/backend/core/api/viewsets/inbound/webhook.py (5)
src/backend/core/api/permissions.py (1)
IsAuthenticated(18-25)src/backend/core/mda/inbound.py (1)
deliver_inbound_message(332-740)src/backend/core/mda/rfc5322/composer.py (1)
compose_email(481-556)src/backend/core/api/viewsets/inbound/widget.py (1)
authenticate(31-43)src/backend/core/models.py (4)
Channel(418-469)Thread(742-895)ThreadAccess(1101-1123)ThreadEvent(1126-1150)
src/backend/core/factories.py (1)
src/backend/core/models.py (18)
Meta(89-90)Meta(199-202)Meta(283-286)Meta(454-466)Meta(501-506)Meta(718-722)Meta(761-764)Meta(944-949)Meta(1116-1120)Meta(1143-1147)Meta(1164-1168)Meta(1205-1209)Meta(1280-1284)Meta(1511-1523)Meta(1579-1583)Meta(1619-1623)Channel(418-469)ThreadEvent(1126-1150)
src/backend/core/models.py (3)
src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx (1)
ThreadEvent(20-73)src/frontend/src/features/api/gen/models/thread_event.ts (1)
ThreadEvent(12-23)src/backend/core/api/fields.py (1)
JSONField(8-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: docker-publish-socks-proxy / docker-build-push
- GitHub Check: check-api-state
- GitHub Check: build-front
- GitHub Check: test-back
- GitHub Check: lint-back
🔇 Additional comments (24)
src/frontend/src/styles/main.scss (1)
44-44: LGTM!The thread-event styles import is correctly placed alongside other thread-view component styles.
src/frontend/src/features/providers/mailbox.tsx (5)
28-32: LGTM! Well-structured timeline type.The
TimelineItemtype provides a clear discriminated union for messages and events with a commoncreated_atfield for sorting.
192-200: LGTM! Proper query setup for thread events.The
threadEventsQueryis correctly configured with the same enabled condition as messages, ensuring events are only fetched when a thread is selected.
203-234: LGTM! Timeline merging logic is sound.The implementation correctly:
- Creates timeline items only when both messages and a selected thread exist
- Merges messages and events into a unified timeline
- Sorts items chronologically by
created_at- Handles optional event data gracefully
289-291: LGTM! Proper invalidation of thread events.The invalidation logic correctly refreshes thread events alongside messages when thread data changes.
339-339: LGTM! Complete loading state tracking.The
isPendingcalculation correctly includesthreadEventsQueryto ensure the UI waits for all data before rendering.src/frontend/src/features/layouts/components/thread-view/components/thread-event/_index.scss (1)
1-74: LGTM! Well-structured component styles.The SCSS follows BEM naming conventions and properly uses design tokens for theming. The styles provide clear visual structure for thread events including error states and iframe containers.
src/frontend/src/features/layouts/components/thread-view/index.tsx (3)
152-179: LGTM! Timeline rendering correctly handles messages and events.The rendering logic properly:
- Iterates over timeline items in chronological order
- Discriminates between message and event types
- Passes appropriate props to each component type
- Uses unique keys for React reconciliation
- Preserves unread refs for intersection observer
240-263: LGTM! Timeline filtering logic is well-designed.The implementation correctly:
- Filters messages based on trash view state
- Unconditionally includes all events in the timeline
- Updates message data with draft children from the filtered set
- Maintains timeline integrity across view state changes
271-277: LGTM! Proper loading state management.The loading check correctly waits for both the messages query and timeline items to be available before rendering content.
src/backend/core/urls.py (1)
15-15: LGTM! Clean integration of new API endpoints.The routing changes correctly register the new webhook and thread event endpoints using Django REST Framework's nested routers. The imports and route configurations follow established patterns in the codebase.
Also applies to: 39-39, 64-66, 90-92
src/backend/core/tests/api/test_inbound_widget.py (1)
179-181: LGTM! Mock updated to reflect new return type.The mock changes correctly simulate
deliver_inbound_messagereturning a Message object with anidattribute instead of a boolean, aligning with the updated function signature.Also applies to: 272-274
src/backend/core/mda/outbound.py (1)
345-350: LGTM! Correctly adapted to new return type.The change properly handles the updated
deliver_inbound_messagereturn type by checking if the returned message is not None instead of checking a boolean value.src/backend/core/tests/importer/test_import_service.py (1)
160-161: LGTM! Mock correctly propagates Message object.The mock changes ensure that the actual Message object returned by
deliver_inbound_messageis propagated through the test, aligning with the updated return type.Also applies to: 262-263
src/backend/core/tests/tasks/test_task_importer.py (1)
92-96: LGTM! Test mocks updated for new return type.The mock changes correctly simulate both success (Message object with id) and failure (None) cases, aligning with the updated
deliver_inbound_messagereturn type ofOptional[Message].Also applies to: 207-209
src/backend/core/tests/mda/test_inbound.py (1)
263-267: LGTM! Comprehensive test updates for new return type.All test assertions have been consistently updated to check for Message objects (
assert message is not None) instead of boolean success flags. The changes properly test both success and failure cases throughout the test suite.Also applies to: 327-331, 349-353, 369-373, 398-402, 424-428, 443-447, 470-474, 505-506, 528-529, 553-554, 584-588, 618-622, 654-658, 692-695, 727-730, 743-746
src/backend/core/admin.py (1)
230-238: LGTM! Clean admin integration for ThreadEvent.The
ThreadEventInlineconfiguration follows Django admin best practices with appropriate field selection, read-only fields forcreated_at, and autocomplete for thechannelrelationship. The integration intoThreadAdminis straightforward.Also applies to: 244-244
src/backend/core/factories.py (1)
125-135: LGTM! Well-structured test factories for new models.Both
ChannelFactoryandThreadEventFactoryfollow factory_boy best practices:
- Appropriate use of
SubFactoryfor relationshipsFuzzyChoicefor enum-like fieldsFakerfor generating test data with custom word lists forThreadEventFactory.typeThese factories will provide good test data coverage for the new webhook and thread event features.
Also applies to: 160-172
src/backend/core/models.py (1)
1126-1150: ThreadEvent model aligns with existing conventionsThe FK wiring, ordering, and JSON default mirror the rest of the app nicely. Looks good to me.
src/backend/core/migrations/0012_threadevent.py (1)
15-33: Migration mirrors the model changesSchema matches the model (including ordering and help texts), so the migration looks correct.
src/backend/core/mda/inbound.py (1)
333-347: Return type change improves downstream contextReturning the
Messageinstance (instead of a boolean) gives callers richer context while preserving truthiness. Nicely handled.src/backend/core/api/serializers.py (1)
795-855: Serializer split for create vs read looks goodKeeping a create-specific serializer with method fields and a read serializer enforcing immutability matches our patterns and keeps write rules tight.
src/backend/core/api/openapi.json (2)
6968-6998: Pagination schema is consistent with existing patterns. LGTM.
4856-5059: ThreadEvent serializer already validates immutability—but OpenAPI schema needs alignment.The codebase already prevents thread/channel/type reassignment via defensive validation in
ThreadEventSerializer.validate()(lines 821–835), which explicitly rejects update attempts with "This field cannot be updated." errors. Tests confirm this works (test_update_thread_event_readonly_fields).However, the review comment remains valid on two points:
Schema/spec mismatch: The OpenAPI spec (
openapi.json) still allows thread and channel in PUT/PATCH request bodies (shows them inThreadEventRequestandPatchedThreadEventRequest), even though the serializer rejects them. This misleads API consumers.Declarative approach preferred: The validation is reactive (checking
request.datainvalidate()) rather than declarative (read_only=Truein field definitions). This makes the intent less clear and harder to discover in the schema.Recommended action:
- Update
ThreadEventSerializerto useread_only=Trueon thread, channel, and type fields instead of (or in addition to) runtime validation.- Regenerate the OpenAPI spec to reflect these as read-only in the schema components.
- Optionally add explicit path/body thread_id equality check in
perform_update/perform_partial_updatefor defense-in-depth.
| "PatchedThreadEventRequest": { | ||
| "type": "object", | ||
| "description": "Serialize thread events.", | ||
| "properties": { | ||
| "thread": { | ||
| "type": "string", | ||
| "format": "uuid" | ||
| }, | ||
| "channel": { | ||
| "type": "string", | ||
| "format": "uuid" | ||
| }, | ||
| "data": {} | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Limit PATCH surface to data only.
Allowing thread/channel in PatchedThreadEventRequest invites unintended reassignment. Keep only data patchable. See proposed schema change in earlier comment.
🤖 Prompt for AI Agents
In src/backend/core/api/openapi.json around lines 7148 to 7162, the
PatchedThreadEventRequest schema currently allows "thread" and "channel" fields
which could enable unintended reassignment; update the schema to remove the
"thread" and "channel" properties so only "data" remains patchable, keep the
object type/description and "data" property as-is (or explicitly allow an
arbitrary object) and adjust any references/examples/comments to reflect that
only the event data may be patched.
| "ThreadEventRequest": { | ||
| "type": "object", | ||
| "description": "Serialize thread events.", | ||
| "properties": { | ||
| "thread": { | ||
| "type": "string", | ||
| "format": "uuid" | ||
| }, | ||
| "channel": { | ||
| "type": "string", | ||
| "format": "uuid" | ||
| }, | ||
| "data": {} | ||
| }, | ||
| "required": [ | ||
| "channel", | ||
| "thread" | ||
| ] | ||
| }, |
There was a problem hiding this comment.
Remove channel/thread from update request required; prevent reassignment.
See consolidated schema diff in the earlier comment to align with path thread_id and preserve provenance.
🤖 Prompt for AI Agents
In src/backend/core/api/openapi.json around lines 7706 to 7724, the
ThreadEventRequest schema currently requires "channel" and "thread", which
allows/forces reassignment via request body; remove "channel" and "thread" from
the required list and either remove those properties from the request schema or
mark them as readOnly (preferred) so callers cannot change provenance; ensure
the API uses the path parameter (thread_id) for identifying the thread and that
server-side validation ignores any provided channel/thread values in the body to
prevent reassignment.
| thread_id = data.get("thread_id") | ||
| event_type = data.get("type") | ||
| event_data = data.get("data", {}) | ||
|
|
||
| # Validate required fields | ||
| if not thread_id: | ||
| return Response( | ||
| {"detail": "Missing thread_id"}, status=status.HTTP_400_BAD_REQUEST | ||
| ) | ||
|
|
||
| if not event_type: | ||
| return Response( | ||
| {"detail": "Missing type"}, status=status.HTTP_400_BAD_REQUEST | ||
| ) | ||
|
|
||
| # Validate event type length | ||
| if len(event_type) > 36: | ||
| return Response( | ||
| {"detail": "Type exceeds maximum length of 36 characters"}, | ||
| status=status.HTTP_400_BAD_REQUEST, | ||
| ) |
There was a problem hiding this comment.
Validate type is a string before using len
If webhook callers send a non-string type (e.g., number), len(event_type) raises TypeError, yielding a 500. Guard the type first and return 400 for invalid payloads.
if not event_type:
return Response(
{"detail": "Missing type"}, status=status.HTTP_400_BAD_REQUEST
)
- # Validate event type length
- if len(event_type) > 36:
+ if not isinstance(event_type, str):
+ return Response(
+ {"detail": "Type must be a string"},
+ status=status.HTTP_400_BAD_REQUEST,
+ )
+
+ # Validate event type length
+ if len(event_type) > 36:
return Response(
{"detail": "Type exceeds maximum length of 36 characters"},
status=status.HTTP_400_BAD_REQUEST,
)🤖 Prompt for AI Agents
In src/backend/core/api/viewsets/inbound/webhook.py around lines 215-235, the
code calls len(event_type) without ensuring event_type is a string, which will
raise TypeError for non-string inputs; update the validation to first check that
event_type is an instance of str and if not return a 400 Response with a clear
message like "Invalid type: expected string", then proceed to the existing
length check (len(event_type) > 36) only after confirming it's a string.
| thread = models.Thread.objects.get(id=thread_id) | ||
| except models.Thread.DoesNotExist: | ||
| return Response( | ||
| {"detail": "Thread not found"}, status=status.HTTP_404_NOT_FOUND | ||
| ) |
There was a problem hiding this comment.
Handle invalid thread UUIDs without 500s
Passing a malformed thread_id into Thread.objects.get raises ValueError, producing a 500. Treat that as bad input and respond with 400 before the DoesNotExist branch.
- try:
- thread = models.Thread.objects.get(id=thread_id)
- except models.Thread.DoesNotExist:
+ try:
+ thread = models.Thread.objects.get(id=thread_id)
+ except ValueError:
+ return Response(
+ {"detail": "Invalid thread_id"}, status=status.HTTP_400_BAD_REQUEST
+ )
+ except models.Thread.DoesNotExist:
return Response(
{"detail": "Thread not found"}, status=status.HTTP_404_NOT_FOUND
)🤖 Prompt for AI Agents
In src/backend/core/api/viewsets/inbound/webhook.py around lines 247 to 251, the
code currently only handles models.Thread.DoesNotExist but a malformed thread_id
can raise ValueError and cause a 500; update the lookup to catch ValueError
before the DoesNotExist branch and return a 400 Bad Request with a helpful
message (e.g. "Invalid thread id"), or validate/parse the UUID first and return
400 if invalid, then proceed to handle DoesNotExist as now.
| from drf_spectacular.utils import extend_schema | ||
| from rest_framework import mixins, viewsets | ||
|
|
||
| from core import models | ||
|
|
||
| from .. import permissions, serializers | ||
|
|
||
|
|
||
| @extend_schema(tags=["thread-event"]) | ||
| class ThreadEventViewSet( | ||
| viewsets.GenericViewSet, | ||
| mixins.ListModelMixin, | ||
| mixins.CreateModelMixin, | ||
| mixins.UpdateModelMixin, | ||
| mixins.DestroyModelMixin, | ||
| mixins.RetrieveModelMixin, | ||
| ): | ||
| """ViewSet for ThreadEvent model.""" | ||
|
|
||
| serializer_class = serializers.ThreadEventSerializer | ||
| permission_classes = [ | ||
| permissions.IsAuthenticated, | ||
| permissions.IsAllowedToAccess, | ||
| ] | ||
| lookup_field = "id" | ||
| lookup_url_kwarg = "id" | ||
| queryset = ( | ||
| models.ThreadEvent.objects.select_related("thread") | ||
| .select_related("channel") | ||
| .all() | ||
| ) | ||
|
|
||
| def get_serializer_class(self): | ||
| """Use create serializer for CREATE, default for all other operations.""" | ||
| if self.action == "create": | ||
| return serializers.ThreadEventCreateSerializer | ||
| return serializers.ThreadEventSerializer | ||
|
|
||
| def get_queryset(self): | ||
| """Restrict results to thread events for the specified thread. | ||
| ThreadAccess is checked by IsAllowedToAccess permission class. | ||
| """ | ||
| # Get thread_id from URL kwargs (provided by nested router) | ||
| thread_id = self.kwargs.get("thread_id") | ||
| if not thread_id: | ||
| return models.ThreadEvent.objects.none() | ||
|
|
||
| # Filter by thread_id only - access control handled by permission class | ||
| return self.queryset.filter(thread_id=thread_id).order_by("created_at") | ||
|
|
||
| def perform_create(self, serializer): | ||
| """Set the thread from URL kwargs when creating a ThreadEvent.""" | ||
| thread_id = self.kwargs.get("thread_id") | ||
| thread = models.Thread.objects.get(id=thread_id) | ||
| serializer.save(thread=thread) |
There was a problem hiding this comment.
Handle missing threads with a 404
Thread.objects.get(id=thread_id) will raise Thread.DoesNotExist, which DRF turns into a 500 unless we translate it ourselves. That means a typo in the nested URL gives clients a server error instead of a 404. Let’s switch to get_object_or_404 so the API responds with the proper status.
-from drf_spectacular.utils import extend_schema
-from rest_framework import mixins, viewsets
+from django.shortcuts import get_object_or_404
+from drf_spectacular.utils import extend_schema
+from rest_framework import mixins, viewsets
@@
- thread = models.Thread.objects.get(id=thread_id)
+ thread = get_object_or_404(models.Thread, id=thread_id)🤖 Prompt for AI Agents
In src/backend/core/api/viewsets/thread_event.py around lines 3 to 57, the
perform_create uses Thread.objects.get(id=thread_id) which raises
Thread.DoesNotExist and can surface as a 500; replace that call with Django’s
get_object_or_404 to return a 404 for missing threads: add "from
django.shortcuts import get_object_or_404" to the imports and change the lookup
to use get_object_or_404(models.Thread, id=thread_id) before calling
serializer.save(thread=thread).
| if (type === "iframe") { | ||
| const iframeData = data as IframeData; | ||
| const src = iframeData.src; | ||
|
|
||
| if (!src || typeof src !== "string") { | ||
| // Error message if src is missing or invalid - fallback to default display | ||
| return ( | ||
| <div className="thread-event"> | ||
| <p className="thread-event__error"> | ||
| {t("This event contains an invalid iframe.")} | ||
| </p> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <div className="thread-event__iframe-container"> | ||
| <iframe | ||
| className="thread-event__iframe" | ||
| src={src} | ||
| width={iframeData.width || "100%"} | ||
| height={iframeData.height || "400px"} | ||
| title={iframeData.title || "Embedded content"} | ||
| sandbox={iframeData.sandbox} | ||
| allow={iframeData.allow} | ||
| /> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
CRITICAL: Validate and sanitize iframe src to prevent XSS.
The src attribute is rendered directly without URL validation or sanitization. This creates multiple security risks:
- Malicious actors could inject
javascript:protocol URLs - Data exfiltration via unsafe URLs
- No origin restrictions
Additionally, sandbox and allow attributes are passed without validation, potentially enabling dangerous permissions.
Apply this diff to add basic security controls:
if (type === "iframe") {
const iframeData = data as IframeData;
const src = iframeData.src;
- if (!src || typeof src !== "string") {
+ // Validate URL and block dangerous protocols
+ if (!src || typeof src !== "string") {
// Error message if src is missing or invalid - fallback to default display
return (
<div className="thread-event">
<p className="thread-event__error">
{t("This event contains an invalid iframe.")}
</p>
</div>
);
}
+
+ // Block javascript: and data: protocols
+ try {
+ const url = new URL(src, window.location.origin);
+ if (url.protocol === 'javascript:' || url.protocol === 'data:') {
+ return (
+ <div className="thread-event">
+ <p className="thread-event__error">
+ {t("This event contains an invalid iframe.")}
+ </p>
+ </div>
+ );
+ }
+ } catch {
+ return (
+ <div className="thread-event">
+ <p className="thread-event__error">
+ {t("This event contains an invalid iframe.")}
+ </p>
+ </div>
+ );
+ }
return (
<div className="thread-event__iframe-container">
<iframe
className="thread-event__iframe"
src={src}
width={iframeData.width || "100%"}
height={iframeData.height || "400px"}
title={iframeData.title || "Embedded content"}
- sandbox={iframeData.sandbox}
+ sandbox={iframeData.sandbox || "allow-scripts allow-same-origin"}
allow={iframeData.allow}
/>
</div>
);
}Note: Consider implementing a server-side allowlist of permitted iframe domains for stronger security.
📝 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.
| if (type === "iframe") { | |
| const iframeData = data as IframeData; | |
| const src = iframeData.src; | |
| if (!src || typeof src !== "string") { | |
| // Error message if src is missing or invalid - fallback to default display | |
| return ( | |
| <div className="thread-event"> | |
| <p className="thread-event__error"> | |
| {t("This event contains an invalid iframe.")} | |
| </p> | |
| </div> | |
| ); | |
| } | |
| return ( | |
| <div className="thread-event__iframe-container"> | |
| <iframe | |
| className="thread-event__iframe" | |
| src={src} | |
| width={iframeData.width || "100%"} | |
| height={iframeData.height || "400px"} | |
| title={iframeData.title || "Embedded content"} | |
| sandbox={iframeData.sandbox} | |
| allow={iframeData.allow} | |
| /> | |
| </div> | |
| ); | |
| if (type === "iframe") { | |
| const iframeData = data as IframeData; | |
| const src = iframeData.src; | |
| // Validate URL and block dangerous protocols | |
| if (!src || typeof src !== "string") { | |
| // Error message if src is missing or invalid - fallback to default display | |
| return ( | |
| <div className="thread-event"> | |
| <p className="thread-event__error"> | |
| {t("This event contains an invalid iframe.")} | |
| </p> | |
| </div> | |
| ); | |
| } | |
| // Block javascript: and data: protocols | |
| try { | |
| const url = new URL(src, window.location.origin); | |
| if (url.protocol === 'javascript:' || url.protocol === 'data:') { | |
| return ( | |
| <div className="thread-event"> | |
| <p className="thread-event__error"> | |
| {t("This event contains an invalid iframe.")} | |
| </p> | |
| </div> | |
| ); | |
| } | |
| } catch { | |
| return ( | |
| <div className="thread-event"> | |
| <p className="thread-event__error"> | |
| {t("This event contains an invalid iframe.")} | |
| </p> | |
| </div> | |
| ); | |
| } | |
| return ( | |
| <div className="thread-event__iframe-container"> | |
| <iframe | |
| className="thread-event__iframe" | |
| src={src} | |
| width={iframeData.width || "100%"} | |
| height={iframeData.height || "400px"} | |
| title={iframeData.title || "Embedded content"} | |
| sandbox={iframeData.sandbox || "allow-scripts allow-same-origin"} | |
| allow={iframeData.allow} | |
| /> | |
| </div> | |
| ); | |
| } |
🤖 Prompt for AI Agents
In
src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx
around lines 24 to 51, the iframe src, sandbox and allow attributes are used
without validation which risks XSS and dangerous permissions; validate and
sanitize by: 1) parse the src with the URL constructor and accept only http(s)
origins matching a client or server-side allowlist (reject javascript:, data:,
about: etc.); 2) fallback to the error UI when src is invalid or not in the
allowlist; 3) enforce a safe sandbox default (e.g. use a minimal token set or
add "allow-same-origin" only when explicitly allowed by a validated source) and
validate incoming sandbox tokens against a small allowed set; 4) validate the
allow attribute against an explicit whitelist of allowed feature tokens (or drop
it) to prevent granting dangerous permissions; and 5) log or surface validation
failures and consider moving allowlist checks to the server for stronger
protection.
| class ThreadEvent(BaseModel): | ||
| """Thread event model to store different types of events in a thread.""" | ||
|
|
||
| thread = models.ForeignKey( | ||
| "Thread", on_delete=models.CASCADE, related_name="events" | ||
| ) | ||
| type = models.CharField(_("type"), max_length=36) | ||
| channel = models.ForeignKey( | ||
| "Channel", | ||
| on_delete=models.SET_NULL, | ||
| null=True, | ||
| blank=True, | ||
| related_name="thread_events", | ||
| help_text=_("Channel that created this event"), | ||
| ) | ||
| data = models.JSONField(_("data"), default=dict, blank=True) | ||
|
|
||
| class Meta: | ||
| db_table = "messages_threadevent" | ||
| verbose_name = _("thread event") | ||
| verbose_name_plural = _("thread events") | ||
| ordering = ["created_at"] | ||
|
|
||
| def __str__(self): | ||
| return f"{self.thread} - {self.type}" | ||
|
|
||
|
|
There was a problem hiding this comment.
In case I would like to create an event which relies on a specific message ? (e.g: Ask a review), the message_id will be stored into the data payload ?
I feel this model will have lot of entities so if we need to lookup on some fields those ones should be indexed and easy to access ?
| const threadEventsQuery = useThreadsEventsList( | ||
| selectedThread?.id ?? '', | ||
| undefined, | ||
| { | ||
| query: { | ||
| enabled: !!selectedThread, | ||
| } | ||
| } | ||
| ); | ||
|
|
||
| // Merge messages and events into a single timeline sorted by created_at | ||
| const timelineItems = useMemo<TimelineItem[] | null>(() => { | ||
| if (!messagesQuery.data?.data?.results || !selectedThread) return null; | ||
|
|
||
| const items: TimelineItem[] = []; | ||
|
|
||
| // Add messages | ||
| messagesQuery.data.data.results.forEach((message) => { | ||
| items.push({ | ||
| type: 'message', | ||
| data: message, | ||
| created_at: message.created_at, | ||
| }); | ||
| }); | ||
|
|
||
| // Add events | ||
| if (threadEventsQuery.data?.data?.results) { | ||
| threadEventsQuery.data.data.results.forEach((event) => { | ||
| items.push({ | ||
| type: 'event', | ||
| data: event, | ||
| created_at: event.created_at, | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| // Sort by created_at | ||
| return items.sort((a, b) => { | ||
| const dateA = new Date(a.created_at).getTime(); | ||
| const dateB = new Date(b.created_at).getTime(); | ||
| return dateA - dateB; | ||
| }); | ||
| }, [messagesQuery.data?.data?.results, threadEventsQuery.data?.data?.results, selectedThread]); | ||
|
|
There was a problem hiding this comment.
Oh... by reading that I realize both message and thread event endpoints are paginated but we don't use an InfiniteQuery to fetch those resources so currently we always fetch only the first page...
Can we imagine to remove pagination from those resources (Is it acceptable from a performance point of view ?) ? It will ease the logic to merge data and ease its display.
There was a problem hiding this comment.
up to you. It would make sense once it's more mature to merge all that in a single api call for a thread. I guess there will be huge threads so paginating would be a plus even if we put the default at ~100 messages.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/backend/core/api/viewsets/thread_event.py (1)
54-58: Useget_object_or_404when resolving the parent thread inperform_create
models.Thread.objects.get(id=thread_id)will raiseThread.DoesNotExistif the URL contains an invalid thread id. Even though current permissions likely prevent that in practice, it’s safer and more explicit to surface a 404 instead of risking a 500 if permission behavior changes or this view is reused.You can rely on Django’s shortcut:
-from drf_spectacular.utils import extend_schema -from rest_framework import mixins, viewsets +from django.shortcuts import get_object_or_404 +from drf_spectacular.utils import extend_schema +from rest_framework import mixins, viewsets @@ def perform_create(self, serializer): """Set the thread from URL kwargs when creating a ThreadEvent.""" thread_id = self.kwargs.get("thread_id") - thread = models.Thread.objects.get(id=thread_id) + thread = get_object_or_404(models.Thread, id=thread_id) serializer.save(thread=thread)This also addresses the earlier automated review’s concern about missing 404 handling.
🧹 Nitpick comments (5)
src/backend/core/factories.py (1)
125-135: Consider simplifying the settings field.The factory implementation is correct and satisfies the Channel model's constraint. However, for the
settingsfield on line 133, usingfactory.Dict()with a completely static nested dictionary is unconventional. Since the value doesn't contain any dynamic factory declarations, a plain dictionary would be simpler and more readable:- settings = factory.Dict({"config": {"enabled": True}}) + settings = {"config": {"enabled": True}}src/frontend/src/features/providers/mailbox.tsx (2)
202-235: Merged timeline construction is sound; consider returning[]instead ofnullThe merge logic correctly:
- Builds separate entries for messages and events,
- Handles the absence of events gracefully,
- Sorts by
created_atso the timeline is chronologically ordered.You could simplify consumers by returning an empty array instead of
nullwhen there is a selected thread but no messages yet (or on transient states), e.g.:- const threadItems = useMemo<TimelineItem[] | null>(() => { - if (!messagesQuery.data?.data?.results || !selectedThread) return null; + const threadItems = useMemo<TimelineItem[] | null>(() => { + if (!selectedThread) return null; + + const messageResults = messagesQuery.data?.data?.results ?? []; + if (!messageResults.length && !threadEventsQuery.data?.data?.results?.length) { + return []; + } @@ - // Add messages - messagesQuery.data.data.results.forEach((message) => { + // Add messages + messageResults.forEach((message) => {Not mandatory, but it avoids propagating
nullchecks into every consumer that can reasonably treat “no items” as an empty list.
326-375: Context additions forthreadItemsandisPendingare coherent; consider exposing event errorsAdding
threadItemsto the context and includingthreadEventsQuery.isPendinginisPendingkeeps the provider’s surface aligned with the new timeline behavior.If you expect callers to react specifically to thread event failures (as opposed to message failures), consider extending
queryStates/errorwith athreadEventsentry so downstream components can distinguish and display those errors independently.src/frontend/src/features/layouts/components/thread-view/index.tsx (1)
4-5: Timeline types/props are wired correctly; consider centralizingTimelineItemThe new
TimelineItemtype andthreadItemsprop onThreadViewComponentmatch the provider’s intent and keep the component decoupled from raw API types. However, you now have two structurally similarTimelineItemdefinitions (one in the mailbox provider, one here). To avoid future drift, consider exporting a sharedTimelineItem(or a generic timeline item type) from a common module and reusing it here.Also applies to: 9-9, 20-28, 30-38, 40-40
src/backend/core/api/serializers.py (1)
820-865: Update serializer enforces immutability on core relations; check client schema formessage
ThreadEventSerializer:
- Uses
UUIDField(source="*.id")forthread,channel, andmessage, aligned with how other serializers expose UUID FKs,- Marks these fields (plus
type) as read-only and reinforces that invalidate()by rejecting attempts to update them on an existing instance,- Provides a full read model for the ThreadEvent, suitable for list/retrieve/update responses.
One thing to verify: the frontend
ThreadEventmodel (e.g.,src/frontend/src/features/api/gen/models/thread_event.ts) currently appears to omit themessagefield in the snippet provided. Ifmessageis part of the API contract here, it would be good to regenerate or adjust the client types so they includemessage?: string | nullto stay in sync with this serializer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/backend/core/admin.py(1 hunks)src/backend/core/api/serializers.py(1 hunks)src/backend/core/api/viewsets/thread_event.py(1 hunks)src/backend/core/factories.py(2 hunks)src/backend/core/models.py(1 hunks)src/frontend/src/features/layouts/components/thread-view/index.tsx(7 hunks)src/frontend/src/features/providers/mailbox.tsx(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/backend/core/admin.py
- src/backend/core/models.py
🧰 Additional context used
📓 Path-based instructions (2)
src/backend/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/*.py: Follow Django/PEP 8 style with a 100-character line limit
Use descriptive, snake_case names for variables and functions
Use Django ORM for database access; avoid raw SQL unless necessary for performance
Use Django’s built-in user model and authentication framework
Prefer try-except blocks to handle exceptions in business logic and views
Log expected and unexpected actions with appropriate log levels
Capture and report exceptions to Sentry; use capture_exception() for custom errors
Do not log sensitive information (tokens, passwords, financial/health data, PII)
Files:
src/backend/core/factories.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/thread_event.py
src/backend/{templates/**/*.html,**/serializers.py}
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Use Django templates for HTML and DRF serializers for JSON responses
Files:
src/backend/core/api/serializers.py
🧠 Learnings (1)
📚 Learning: 2025-09-02T10:12:12.835Z
Learnt from: CR
Repo: suitenumerique/messages PR: 0
File: .cursor/rules/django-python.mdc:0-0
Timestamp: 2025-09-02T10:12:12.835Z
Learning: Applies to src/backend/**/views.py : Use Django REST Framework viewsets for API endpoints
Applied to files:
src/backend/core/api/viewsets/thread_event.py
🧬 Code graph analysis (5)
src/frontend/src/features/providers/mailbox.tsx (3)
src/frontend/src/features/api/gen/models/thread_event.ts (1)
ThreadEvent(12-23)src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx (1)
ThreadEvent(20-73)src/frontend/src/features/api/gen/thread-event/thread-event.ts (2)
useThreadsEventsList(216-250)getThreadsEventsListQueryKey(85-93)
src/frontend/src/features/layouts/components/thread-view/index.tsx (2)
src/frontend/src/features/api/gen/models/thread_event.ts (1)
ThreadEvent(12-23)src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx (1)
ThreadEvent(20-73)
src/backend/core/factories.py (1)
src/backend/core/models.py (18)
Meta(89-90)Meta(199-202)Meta(283-286)Meta(454-466)Meta(501-506)Meta(718-722)Meta(761-764)Meta(944-949)Meta(1116-1120)Meta(1151-1155)Meta(1172-1176)Meta(1213-1217)Meta(1288-1292)Meta(1519-1531)Meta(1587-1591)Meta(1627-1631)Channel(418-469)ThreadEvent(1126-1158)
src/backend/core/api/serializers.py (2)
src/backend/core/models.py (17)
Meta(89-90)Meta(199-202)Meta(283-286)Meta(454-466)Meta(501-506)Meta(718-722)Meta(761-764)Meta(944-949)Meta(1116-1120)Meta(1151-1155)Meta(1172-1176)Meta(1213-1217)Meta(1288-1292)Meta(1519-1531)Meta(1587-1591)Meta(1627-1631)ThreadEvent(1126-1158)src/frontend/src/features/api/gen/models/thread_event.ts (1)
ThreadEvent(12-23)
src/backend/core/api/viewsets/thread_event.py (3)
src/backend/core/api/serializers.py (2)
ThreadEventSerializer(820-865)ThreadEventCreateSerializer(795-817)src/backend/core/api/permissions.py (2)
IsAuthenticated(18-25)IsAllowedToAccess(92-186)src/backend/core/models.py (2)
ThreadEvent(1126-1158)Thread(742-895)
🔇 Additional comments (10)
src/backend/core/factories.py (1)
160-173: LGTM!The ThreadEventFactory is well-implemented. The explicit
Noneassignment for themessagefield with the accompanying comment is good documentation practice, making it clear that this field is optional and can be set explicitly in tests when needed. The custom word list for thetypefield appropriately constrains the generated values to meaningful event types.src/frontend/src/features/providers/mailbox.tsx (3)
27-32: TimelineItem andthreadItemswiring looks consistentDefining
TimelineItemwith a discriminatedtypeand exposingthreadItems: TimelineItem[] | nullon the context (including the default value) cleanly prepares the mailbox provider for mixed message/event timelines. No functional issues spotted here.Also applies to: 38-38, 60-65
192-201: Event query and invalidation are correctly scoped to the selected threadUsing
useThreadsEventsListwithenabled: !!selectedThreadensures events are fetched only when a thread is selected, and invalidating viagetThreadsEventsListQueryKey(selectedThread.id)keeps cache invalidation aligned with the query key. If you later introduce query params to the events list, remember to pass the same params into both the hook andgetThreadsEventsListQueryKeyto avoid stale cache entries.Also applies to: 289-291
2-2: No duplicate import found—review comment is incorrectThe actual import statement at line 2 contains only a single
Threadimport. The second symbol isThreadEvent, which is a distinct and necessary import. No action is required; the code is already correct.Likely an incorrect or invalid review comment.
src/frontend/src/features/layouts/components/thread-view/index.tsx (4)
152-179: Mixed rendering of messages and events is correct and preserves read/scroll behaviorThe
threadItems.maplogic:
- Correctly distinguishes
messagevseventitems viaitem.type,- Maintains existing read-tracking by attaching refs and
data-message-idonly toThreadMessageentries,- Uses stable keys (
message-{id},event-{id}),- Safely passes
event.channel ?? undefinedand typeddatatoThreadEventComponent.This looks functionally solid.
213-228: Filtering timeline items in sync with filtered messages is reasonable
filteredThreadItems:
- Filters out message items whose ids are not in
filteredMessages,- Always keeps event items, which matches an “event log” mental model,
- Replaces message payloads with the enriched
MessageWithDraftChildinstances so draft children are preserved.The logic and dependencies (
[threadItems, filteredMessages]) look consistent. No functional issues spotted.Also applies to: 240-263
280-286: PassingfilteredThreadItemsintoThreadViewComponentkeeps UI aligned with filtersWiring
threadItems={filteredThreadItems}while separately passingmessages={filteredMessages}ensures the rendered timeline stays in sync with trash filters and draft nesting, and avoids duplicating filtering logic insideThreadViewComponent. This looks clean.
271-271: Verification confirms the review comment is accurateThe analysis is correct:
threadItemsremainsnullwhenmessagesQueryerrors (sincemessagesQuery.data?.data?.resultsis falsy), whilequeryStates.messages.isLoadingbecomesfalse. This causes the loading condition to remain permanently true, showing an infinite spinner.The fix is safe because
filteredThreadItems(which becomes thethreadItemsprop passed toThreadViewComponent) is guaranteed to return an empty array when the upstreamthreadItemsis null, never null itself. Removing the!threadItemscheck allows the component to render an empty view instead of blocking on a spinner when the query errors.The proposed diff is correct and the severity is appropriate for a UX-blocking bug.
src/backend/core/api/viewsets/thread_event.py (1)
11-35: ThreadEventViewSet structure aligns with DRF patternsUsing a
GenericViewSetplus standard mixins, a thread-scoped queryset (filtered bythread_id), and dynamicget_serializer_classto switch toThreadEventCreateSerializeron create is consistent with DRF best practices and the project’s existing API style.select_relatedonthread,channel, andmessageis also a good default for avoiding N+1s on list/detail.Also applies to: 36-53
src/backend/core/api/serializers.py (1)
795-818: Create serializer correctly constrains writable fields and returns related IDs
ThreadEventCreateSerializer:
- Makes
thread,channel, andmessageread-only, exposing them as string IDs viaSerializerMethodField,- Leaves
typeanddataas the only writable business fields,- Matches the viewset’s behavior of injecting
threadfrom the URL (serializer.save(thread=thread)).This is a good pattern for nested-create endpoints where relationships are controlled by the server rather than client input.
Webhook is a more generic version of the widget endpoint
ThreadEvents are blocks that can be inserted into Threads (fixes #188).
Frontend is still prototype, we will iterate on a few event types before merging
Summary by CodeRabbit
New Features
Tests
Documentation
Style
✏️ Tip: You can customize this high-level summary in your review settings.