Undo send #450
Undo send #450
Conversation
Add new API endpoint to cancel queued send tasks and revert messages to draft state. This enables the undo send feature.
- Add UndoSendToast component with countdown timer - Integrate undo functionality in message send flow - Use refetchQueries to ensure fresh data before navigation - Clean up draft blob when canceling send
WalkthroughAdds a delayed-send + undo workflow: new POST /api/v1.0/send/cancel/ endpoint and CancelSendRequest schema, a delay field (0–30) on send requests, backend changes to schedule/revoke Celery send tasks and preserve drafts, frontend undo-toast and cancel hook wired into the send flow. Changes
Sequence DiagramsequenceDiagram
participant User
participant Frontend
participant Backend
participant Queue as Celery
participant DB as Database
User->>Frontend: Submit send (delay=5)
Frontend->>Backend: POST /api/v1.0/send (payload includes delay)
Backend->>DB: Save message (is_draft=true)
Backend->>Queue: Enqueue send task with countdown(5)
Backend-->>Frontend: 200 { task_id, message }
Frontend->>Frontend: Show UndoSendToast (countdown)
alt User clicks Undo before timeout
User->>Frontend: Click Undo
Frontend->>Backend: POST /api/v1.0/send/cancel/ (taskId, messageId)
Backend->>Queue: Revoke task (terminate=true)
Backend->>DB: Keep message as draft, remove draft_blob (if present)
Backend-->>Frontend: 200 OK
Frontend->>Frontend: Navigate to draft / update UI
else Countdown completes
Queue->>Backend: Execute scheduled send task
Backend->>DB: Finalize draft -> is_draft=false, cleanup draft_blob
Backend->>DB: Update thread stats
Backend-->>Frontend: (after fetch) reflect sent state
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (11)
src/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx (1)
23-23: Verify the intent of the unusedsearchParamsvariable.The
useSearchParamshook is imported and initialized but thesearchParamsvariable is never used in the component. Given this PR adds an undo-send feature that redirects users to the Drafts page and opens the draft editor, it's possible this was intended to handle query-driven navigation (e.g., opening a specific draft based on URL parameters) but the implementation was not completed.Please clarify:
- Is this preparatory code for upcoming functionality in this draft PR?
- Should this be removed as unused code?
- Was there intended usage that was forgotten?
If this is incomplete work, consider either completing the implementation or removing the unused import and variable to keep the code clean.
Also applies to: 34-34
src/backend/core/mda/outbound.py (1)
304-304: Use lazy string formatting for logging.Per coding guidelines, prefer
%-style formatting in logging calls for lazy evaluation:- logger.warning(f"Failed to cleanup draft_blob for message {message.id}: {e}") + logger.warning("Failed to cleanup draft_blob for message %s: %s", message.id, e)src/frontend/src/features/message/use-cancel-send.tsx (2)
19-29: Consider adding error handling to the mutation.The mutation lacks an
onErrorcallback. While errors will propagate when usingmutateAsync, consumers usingmutatewon't have error feedback. Consider adding error handling:return useMutation({ mutationFn: cancelSend, onSuccess: async () => { await Promise.all([ queryClient.refetchQueries({ queryKey: ["messages"] }), queryClient.refetchQueries({ queryKey: ["threads"] }), ]); }, + onError: (error) => { + console.error('Failed to cancel send:', error); + }, });
9-14: Return type should match API response.The
cancelSendfunction returnsPromise<void>, butfetchAPIreturns a response object. Consider typing the return value to ensure type safety:-const cancelSend = async (params: CancelSendParams): Promise<void> => { - await fetchAPI("/api/v1.0/send/cancel/", { +const cancelSend = async (params: CancelSendParams) => { + return fetchAPI<{ status: number; data: { detail: string } }>("/api/v1.0/send/cancel/", { method: "POST", body: JSON.stringify(params), }); };src/frontend/src/features/message/components/undo-send-toast.tsx (1)
16-33: useEffect recreates interval on every countdown change.Including
countdownin the dependency array causes the effect to re-run every second, unnecessarily recreating the interval. Also, callingonComplete()insidesetCountdownis a side effect in a state updater.Consider using a ref to track the interval and only run the effect once:
+ const timerRef = useRef<NodeJS.Timeout | null>(null); + useEffect(() => { - if (countdown <= 0 || isCancelled) { + if (isCancelled) { return; } - const timer = setInterval(() => { + timerRef.current = setInterval(() => { setCountdown((prev) => { - const next = prev - 1; - if (next <= 0) { - clearInterval(timer); - onComplete(); - } - return next; + return prev - 1; }); }, 1000); - return () => clearInterval(timer); - }, [countdown, onComplete, isCancelled]); + return () => { + if (timerRef.current) clearInterval(timerRef.current); + }; + }, [isCancelled]); + + useEffect(() => { + if (countdown <= 0 && !isCancelled) { + if (timerRef.current) clearInterval(timerRef.current); + onComplete(); + } + }, [countdown, isCancelled, onComplete]);src/frontend/src/features/forms/components/message-form/index.tsx (2)
123-124: Move UNDO_SEND_DELAY to a config constant.The comment indicates this will become a user setting in Phase 2. Consider moving this to a centralized config file now to make the future change easier:
- // Hardcoded delay for Phase 1 testing (will be user setting in Phase 2) - const UNDO_SEND_DELAY = 5; + // TODO: Make this a user setting in Phase 2And in a config file (e.g.,
src/features/config/constants.ts):export const UNDO_SEND_DELAY = 5;
270-274: Document the router.push workaround or create a follow-up issue.Using
window.location.hrefcauses a full page reload, which degrades UX. The comment mentions "router.push has issues" - consider documenting the specific issue or creating a follow-up task to fix it properly.src/backend/core/api/viewsets/cancel_send.py (1)
20-25: Consider validating taskId format.While Celery will ignore invalid task IDs, you might want to use
UUIDFieldfortaskIdsince Celery task IDs are typically UUIDs. This would provide earlier validation and clearer API documentation.- taskId = drf_serializers.CharField(required=True, help_text="Celery task ID to cancel") + taskId = drf_serializers.UUIDField(required=True, help_text="Celery task ID to cancel")src/backend/core/api/openapi.json (3)
4136-4220: Define a concrete response and cover “already started” case.Return a typed payload instead of an unspecified object, and add 409 when the task can’t be revoked (already STARTED/SUCCESS). Also declare idempotency (repeated cancels should be OK and return the same shape).
Apply this diff in the cancel path to use a proper schema and add 409:
"responses": { "200": { "content": { "application/json": { - "schema": { - "type": "object", - "additionalProperties": {}, - "description": "Unspecified response body" - }, + "schema": { "$ref": "#/components/schemas/CancelSendResponse" }, "examples": { "CancelSend": { "value": { "taskId": "a1b2c3d4-e5f6-7890-1234-567890abcdef", "messageId": "123e4567-e89b-12d3-a456-426614174000" }, "summary": "Cancel Send" } } } }, "description": "" }, + "409": { + "content": { + "application/json": { + "schema": { "type": "object", "properties": { "detail": { "type": "string" } } } + } + }, + "description": "Task already started or finished; cannot cancel" + }, "400": {And add this schema under components.schemas:
+ "CancelSendResponse": { + "type": "object", + "properties": { + "taskId": { "type": "string", "description": "Celery task ID revoked" }, + "messageId": { "type": "string", "format": "uuid", "description": "Draft message ID" }, + "revoked": { "type": "boolean", "description": "True if revoke issued (idempotent)" } + }, + "required": ["taskId", "messageId", "revoked"] + },
5207-5226: TightentaskIdtyping and align naming.If your Celery task IDs are UUIDs, declare
format: uuid; otherwise document the expected format. Also consider harmonizingtaskIdvstask_idused elsewhere for consistency."CancelSendRequest": { "type": "object", "description": "Serializer for canceling send tasks.", "properties": { "taskId": { - "type": "string", + "type": "string", + "format": "uuid", "minLength": 1, "description": "Celery task ID to cancel" },Please confirm whether Celery task IDs can be non‑UUID in your deployment; if yes, keep
type: stringand add a brief note like “UUID by default; may be broker-specific”.
7218-7223: Documentdelayclearly; verify default matches product behavior.Add a short description clarifying units and undo semantics. If the intended default undo window is 5s, consider updating the backend serializer default and then this schema.
"delay": { "type": "integer", "maximum": 30, "minimum": 0, - "default": 0 + "default": 0, + "description": "Delay in seconds before dispatch (enables undo-cancel within this window)" }Confirm the server-side default for
delayin SendMessageSerializer; if you change it to 5s to match the UX, update the OpenAPI default here too.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/backend/core/api/openapi.json(3 hunks)src/backend/core/api/serializers.py(1 hunks)src/backend/core/api/viewsets/cancel_send.py(1 hunks)src/backend/core/api/viewsets/send.py(2 hunks)src/backend/core/mda/outbound.py(3 hunks)src/backend/core/urls.py(2 hunks)src/frontend/src/features/forms/components/message-form/index.tsx(4 hunks)src/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx(2 hunks)src/frontend/src/features/message/components/undo-send-toast.tsx(1 hunks)src/frontend/src/features/message/use-cancel-send.tsx(1 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/api/serializers.pysrc/backend/core/api/viewsets/send.pysrc/backend/core/api/viewsets/cancel_send.pysrc/backend/core/mda/outbound.pysrc/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/**/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
🧠 Learnings (3)
📚 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,tasks.py} : Use asynchronous views and Celery tasks for I/O-bound or long-running operations
Applied to files:
src/backend/core/api/viewsets/cancel_send.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/api/viewsets/cancel_send.pysrc/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/**/urls.py : Define clear, RESTful URL patterns using Django’s URL dispatcher
Applied to files:
src/backend/core/urls.py
🧬 Code graph analysis (6)
src/backend/core/api/viewsets/send.py (2)
src/backend/core/mda/tasks.py (1)
send_message_task(21-78)src/backend/core/api/serializers.py (1)
MessageSerializer(619-780)
src/frontend/src/features/message/use-cancel-send.tsx (1)
src/frontend/src/features/api/fetch-api.ts (1)
fetchAPI(12-40)
src/backend/core/api/viewsets/cancel_send.py (3)
src/backend/core/api/permissions.py (1)
IsAllowedToAccess(92-167)src/backend/core/api/viewsets/send.py (1)
post(77-151)src/backend/core/models.py (1)
update_stats(769-895)
src/frontend/src/features/message/components/undo-send-toast.tsx (1)
src/frontend/src/features/ui/components/toaster/index.tsx (1)
ToasterItem(16-61)
src/backend/core/mda/outbound.py (1)
src/backend/core/models.py (7)
save(92-95)save(207-210)save(291-294)save(954-1006)save(1501-1504)save(1757-1773)update_stats(769-895)
src/backend/core/urls.py (1)
src/backend/core/api/viewsets/cancel_send.py (1)
CancelSendView(60-120)
⏰ 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). (8)
- GitHub Check: docker-publish-socks-proxy / docker-build-push
- GitHub Check: lint-back
- GitHub Check: lint-front
- GitHub Check: test-front
- GitHub Check: test-e2e
- GitHub Check: build-front
- GitHub Check: test-back
- GitHub Check: check-api-state
🔇 Additional comments (9)
src/backend/core/mda/outbound.py (2)
475-477: Good placement of thread stats update in finally block.Moving
update_stats()to the finally block ensures thread statistics remain consistent regardless of delivery success or failure.
294-304: [rewritten comment]
[classification tag]src/backend/core/urls.py (1)
36-36: LGTM!The new cancel-send endpoint follows established URL patterns and naming conventions. The route is logically placed alongside the send endpoint. Based on learnings, this follows the guideline for clear, RESTful URL patterns.
Also applies to: 178-182
src/backend/core/api/viewsets/send.py (2)
124-135: LGTM - Delayed send branching is correctly implemented.The logic properly distinguishes between delayed and immediate sends:
- Delayed sends (
delay > 0): Keep message as draft, schedule task with countdown- Immediate sends (
delay == 0): Mark as non-draft synchronously, then queue taskThis ensures the message state is appropriate for each scenario and supports the undo workflow.
141-143: Correct conditional stats update.Thread stats are only updated for immediate sends here. For delayed sends,
update_stats()is appropriately called insend_message()(outbound.py) within the finally block after task execution.src/frontend/src/features/forms/components/message-form/index.tsx (1)
253-317: Well-structured delayed send flow with proper cleanup.The undo flow correctly:
- Dismisses the toast immediately on undo action
- Waits for cancel mutation and query refetch before navigation
- Shows success toast on completion
- Invalidates relevant queries to refresh UI state
Good implementation of the feature requirements.
src/backend/core/api/serializers.py (1)
1463-1466: LGTM! The delay field is well-defined.The field correctly supports the delayed-send feature with appropriate validation (0-30 seconds) and a backwards-compatible default of 0 for immediate sends.
src/backend/core/api/viewsets/cancel_send.py (2)
1-18: LGTM! Imports are appropriate.The imports are well-organized and include all necessary dependencies for the cancel-send functionality.
27-59: LGTM! OpenAPI documentation is comprehensive.The schema decoration provides clear examples and response formats that will help API consumers understand the endpoint.
| def post(self, request): | ||
| """Cancel a queued send task.""" | ||
| serializer = CancelSendSerializer(data=request.data) | ||
| serializer.is_valid(raise_exception=True) | ||
|
|
||
| task_id = serializer.validated_data.get("taskId") | ||
| message_id = serializer.validated_data.get("messageId") | ||
|
|
||
| # Ensure message stays as draft | ||
| try: | ||
| message = models.Message.objects.get(id=message_id) | ||
|
|
||
| # Check if already sent - if draft_blob is gone, it's too late | ||
| if not message.draft_blob: | ||
| raise drf_exceptions.ValidationError( | ||
| "Cannot cancel: message has already been sent." | ||
| ) | ||
|
|
There was a problem hiding this comment.
Missing object-level permission check.
The view fetches the message but never validates that the requesting user has permission to cancel it. Looking at the send view (src/backend/core/api/viewsets/send.py line 103), it calls self.check_object_permissions(request, message) after fetching.
Add permission check after fetching the message:
try:
message = models.Message.objects.get(id=message_id)
+ self.check_object_permissions(request, message)
# Check if already sent - if draft_blob is gone, it's too late
if not message.draft_blob:📝 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 post(self, request): | |
| """Cancel a queued send task.""" | |
| serializer = CancelSendSerializer(data=request.data) | |
| serializer.is_valid(raise_exception=True) | |
| task_id = serializer.validated_data.get("taskId") | |
| message_id = serializer.validated_data.get("messageId") | |
| # Ensure message stays as draft | |
| try: | |
| message = models.Message.objects.get(id=message_id) | |
| # Check if already sent - if draft_blob is gone, it's too late | |
| if not message.draft_blob: | |
| raise drf_exceptions.ValidationError( | |
| "Cannot cancel: message has already been sent." | |
| ) | |
| def post(self, request): | |
| """Cancel a queued send task.""" | |
| serializer = CancelSendSerializer(data=request.data) | |
| serializer.is_valid(raise_exception=True) | |
| task_id = serializer.validated_data.get("taskId") | |
| message_id = serializer.validated_data.get("messageId") | |
| # Ensure message stays as draft | |
| try: | |
| message = models.Message.objects.get(id=message_id) | |
| self.check_object_permissions(request, message) | |
| # Check if already sent - if draft_blob is gone, it's too late | |
| if not message.draft_blob: | |
| raise drf_exceptions.ValidationError( | |
| "Cannot cancel: message has already been sent." | |
| ) |
🤖 Prompt for AI Agents
In src/backend/core/api/viewsets/cancel_send.py around lines 65 to 82, after
fetching the message object you must perform an object-level permission check;
add a call to self.check_object_permissions(request, message) immediately after
message = models.Message.objects.get(id=message_id) so the view enforces the
same permissions as the send view before proceeding to validate draft_blob and
cancel.
| # Ensure message stays as draft | ||
| try: | ||
| message = models.Message.objects.get(id=message_id) | ||
|
|
||
| # Check if already sent - if draft_blob is gone, it's too late | ||
| if not message.draft_blob: | ||
| raise drf_exceptions.ValidationError( | ||
| "Cannot cancel: message has already been sent." | ||
| ) | ||
|
|
||
| # Revoke the Celery task | ||
| celery_app.control.revoke(task_id, terminate=True) | ||
| logger.info(f"Revoked Celery task {task_id} for message {message_id}") |
There was a problem hiding this comment.
Race condition: message could be sent between check and revoke.
There's a time-of-check-to-time-of-use (TOCTOU) issue between verifying draft_blob exists (line 78) and revoking the task (line 84). If the Celery task executes in this window, the message will be sent, draft_blob removed, and then this code will attempt to revert it to draft state, leading to inconsistent data.
Consider implementing a distributed lock (e.g., using Redis) around the send and cancel operations to prevent concurrent execution:
from django.core.cache import cache
# In cancel view, before line 75
lock_key = f"message_send_lock_{message_id}"
if not cache.add(lock_key, "locked", timeout=60):
raise drf_exceptions.ValidationError("Operation in progress")
try:
# ... existing cancel logic ...
finally:
cache.delete(lock_key)And similarly in the send view's task execution. Based on learnings, this aligns with handling TOCTOU issues in concurrent operations.
🤖 Prompt for AI Agents
In src/backend/core/api/viewsets/cancel_send.py around lines 73 to 85, there is
a TOCTOU race between checking message.draft_blob and revoking the Celery task;
wrap the check+revoke in a distributed lock (e.g., Redis via Django cache) keyed
by the message id so cancel and the send task cannot run concurrently, acquire
the lock with a non-blocking add and return a ValidationError if unable to
acquire, perform the get/check/revoke logic while holding the lock, and always
release the lock in a finally block; ensure the send task also obtains/releases
the same lock so both sides coordinate and set a sensible lock timeout.
| ) | ||
|
|
||
| # Revoke the Celery task | ||
| celery_app.control.revoke(task_id, terminate=True) |
There was a problem hiding this comment.
Task revocation with terminate=True could disrupt email sending.
Using terminate=True sends SIGTERM to the worker, which forcefully kills the task even if it's mid-execution. If the task has already started sending the email (SMTP connection open, partial transmission), this could leave the system in an inconsistent state or cause email corruption.
Consider:
- First try revoking without terminate:
celery_app.control.revoke(task_id, terminate=False) - Check if the message state has changed (refresh from DB and re-check draft_blob)
- Only use terminate=True as a last resort if the message is still in a safe state
Alternatively, design the Celery task to check a cancellation flag at the start of execution and abort if set, avoiding the need for terminate=True.
🤖 Prompt for AI Agents
In src/backend/core/api/viewsets/cancel_send.py around line 84, the code
currently calls celery_app.control.revoke(task_id, terminate=True) which
forcefully kills tasks; update the flow to first call revoke with
terminate=False, then reload the message record from the database and re-check
draft_blob/state to determine whether the send has begun; only if the refreshed
state indicates it is still safe to forcibly stop (or if a
timeout/failed-soft-revoke occurs) call revoke with terminate=True as a last
resort; additionally, modify the task contract to respect a cancellable flag
(check a DB cancellation field at start and abort cleanly) so terminate=True is
rarely needed.
There was a problem hiding this comment.
Agree with coderabbit here, we need to cancel from the task, not from the outside
There was a problem hiding this comment.
@sylvinus Understood! You're right — canceling from within the task is much safer and more predictable than external termination.
Here's the recommended approach:
Implementation Strategy
1. Add a cancellation flag to the Message model
Add a field to track cancellation intent:
# In models.py
is_send_cancelled = models.BooleanField(default=False)2. Modify the cancel_send view
Instead of revoking the task, set the cancellation flag:
# In cancel_send.py
def post(self, request):
# ... validation ...
message = models.Message.objects.get(id=message_id)
if not message.draft_blob:
raise drf_exceptions.ValidationError(
"Cannot cancel: message has already been sent."
)
# Set cancellation flag instead of revoking
message.is_send_cancelled = True
message.save(update_fields=["is_send_cancelled"])
# Optional: still call revoke without terminate for cleanup
celery_app.control.revoke(task_id, terminate=False)
return Response({"detail": "Send cancelled successfully."})3. Check the flag at the start of the Celery task
In send_message task (or wrapper), check before any SMTP operations:
# In tasks.py or outbound.py
@celery_app.task
def send_message_task(message_id, ...):
message = Message.objects.get(id=message_id)
# Check cancellation flag BEFORE any send operations
if message.is_send_cancelled:
logger.info(f"Send cancelled for message {message_id}, keeping as draft")
# Clean up: reset the flag for potential future sends
message.is_send_cancelled = False
message.save(update_fields=["is_send_cancelled"])
return # Abort cleanly
# Proceed with normal send flow
send_message(message, ...)This approach ensures:
- ✅ No risk of interrupting mid-send
- ✅ Clean state management
- ✅ Task checks cancellation before touching SMTP
- ✅ Predictable behavior with proper logging
Would you like me to help draft the complete implementation with these changes?
| } catch (error) { | ||
| console.error('Failed to cancel send:', error); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Show user-facing error when cancel fails.
Currently, cancel failures are only logged to console. Users should see feedback when the undo operation fails:
} catch (error) {
console.error('Failed to cancel send:', error);
+ addToast(
+ <ToasterItem type="error">
+ <span>{t("Failed to cancel send. The message may have already been sent.")}</span>
+ </ToasterItem>
+ );
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/frontend/src/features/forms/components/message-form/index.tsx around
lines 275 to 278, the catch block for the cancel send operation only logs to
console and does not surface any user-facing feedback; modify the handler to
present an error notification to the user (e.g., call the app's
toast/notification API or set component error state and render an inline
message) with a clear message like "Failed to undo send" and include the error
details for support; ensure the notification path is used consistently with
other error UX in this component and keep the console.error for debugging.
NielsCodes
force-pushed
the
undo-send
branch
from
5cc053f to
9780fb7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/backend/core/api/viewsets/cancel_send.py (3)
65-82: Missing object-level permission check.The view fetches the message but never validates that the requesting user has permission to cancel it. Without this check, a user could potentially cancel another user's message.
Add permission check after fetching the message:
try: message = models.Message.objects.get(id=message_id) + self.check_object_permissions(request, message) # Check if already sent - if draft_blob is gone, it's too late if not message.draft_blob:
73-84: Race condition: message could be sent between check and revoke.There's a TOCTOU issue between verifying
draft_blobexists (line 78) and revoking the task (line 83). If the Celery task executes in this window, the message will be sent,draft_blobremoved, and then this code will attempt to revert it to draft state, leading to inconsistent data.Consider implementing a distributed lock (e.g., using Redis via Django cache) around the send and cancel operations:
from django.core.cache import cache # Before line 75 lock_key = f"message_send_lock_{message_id}" if not cache.add(lock_key, "locked", timeout=60): raise drf_exceptions.ValidationError("Operation in progress") try: # ... existing cancel logic ... finally: cache.delete(lock_key)Ensure the send task also acquires/releases the same lock.
83-84: Task revocation with terminate=True could disrupt email sending.Using
terminate=Truesends SIGTERM to the worker, forcefully killing the task even if mid-execution. If the task has started sending (SMTP connection open, partial transmission), this could leave the system in an inconsistent state.Consider:
- First try revoking without terminate:
celery_app.control.revoke(task_id, terminate=False)- Refresh the message from DB and re-check
draft_blobto verify state- Only use
terminate=Trueas a last resort if still safeAlternatively, design the Celery task to check a cancellation flag at the start and abort cleanly.
🧹 Nitpick comments (1)
src/backend/core/api/viewsets/cancel_send.py (1)
65-119: Consider async view for I/O-bound operations.This view performs multiple I/O operations (database queries, Celery task revocation, blob deletion). Based on learnings, async views are recommended for I/O-bound operations to improve responsiveness.
Consider converting to an async view:
async def post(self, request): """Cancel a queued send task.""" serializer = CancelSendSerializer(data=request.data) serializer.is_valid(raise_exception=True) # Use async database queries message = await models.Message.objects.aget(id=message_id) # ... rest of logic with await for I/O operationsBased on learnings, this applies to I/O-bound operations in Django views.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/backend/core/api/viewsets/cancel_send.py(1 hunks)src/backend/core/mda/outbound.py(3 hunks)src/frontend/src/features/forms/components/message-form/index.tsx(4 hunks)src/frontend/src/features/message/use-cancel-send.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/frontend/src/features/message/use-cancel-send.tsx
- src/backend/core/mda/outbound.py
- src/frontend/src/features/forms/components/message-form/index.tsx
🧰 Additional context used
📓 Path-based instructions (1)
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/api/viewsets/cancel_send.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,tasks.py} : Use asynchronous views and Celery tasks for I/O-bound or long-running operations
Applied to files:
src/backend/core/api/viewsets/cancel_send.py
⏰ 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). (8)
- GitHub Check: docker-publish-socks-proxy / docker-build-push
- GitHub Check: check-api-state
- GitHub Check: test-front
- GitHub Check: build-front
- GitHub Check: lint-back
- GitHub Check: test-back
- GitHub Check: lint-front
- GitHub Check: test-e2e
🔇 Additional comments (3)
src/backend/core/api/viewsets/cancel_send.py (3)
20-25: LGTM: Serializer validation is correct.The serializer properly validates both required fields with appropriate types and help text.
27-59: LGTM: Comprehensive API documentation.The OpenAPI schema provides clear documentation with appropriate response examples and request payload structure.
95-105: LGTM: Robust blob deletion handling.The code appropriately checks for blob existence, handles deletion failures gracefully with logging, and continues execution. The error handling ensures the operation isn't aborted if blob deletion fails.
| if needs_save: | ||
| message.save(update_fields=update_fields) | ||
| message.thread.update_stats() | ||
| logger.info(f"Message {message_id} reverted to draft state") |
There was a problem hiding this comment.
Potential AttributeError if message.thread is None.
Line 108 calls message.thread.update_stats() without verifying that thread is not None. If the foreign key allows null values, this will raise an AttributeError.
Add a null check before calling thread methods:
if needs_save:
message.save(update_fields=update_fields)
- message.thread.update_stats()
+ if message.thread:
+ message.thread.update_stats()
logger.info(f"Message {message_id} reverted to draft state")📝 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 needs_save: | |
| message.save(update_fields=update_fields) | |
| message.thread.update_stats() | |
| logger.info(f"Message {message_id} reverted to draft state") | |
| if needs_save: | |
| message.save(update_fields=update_fields) | |
| if message.thread: | |
| message.thread.update_stats() | |
| logger.info(f"Message {message_id} reverted to draft state") |
🤖 Prompt for AI Agents
In src/backend/core/api/viewsets/cancel_send.py around lines 106 to 109, calling
message.thread.update_stats() can raise AttributeError if message.thread is
None; add a null check before invoking thread methods (for example, if
message.thread is not None: message.thread.update_stats()) so update_stats is
only called when a related thread exists, preserving the existing
save/update_fields and log behavior.
|
@sylvinus two things I'd like to get your thoughts on: Delay setting Brute force redirect When using To check the issue with opening a draft in the current application:
|
2 participants
Purpose
Add "undo" option that allows users to cancel sending an email within 5 seconds.
Proposal
Instead of immediately sending an email, the email is sent to Celery with a delay.
The user will see a toast with a 5-second countdown and an "Undo" button.
When the user clicks "Undo", the Celery task is cancelled and the email is set back to Draft.
The user is redirected to the Drafts page, where the email is opened in the draft editor.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.