-
-
Notifications
You must be signed in to change notification settings - Fork 215
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
added a close button to delete the message chat in messages #4032
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe changes add a new API endpoint and corresponding front-end functionality for deleting messaging threads. A URL pattern is introduced to handle delete requests, which routes to a backend view function. This function verifies user authentication and authorization before deleting the specified thread from the database. In the user interface, a delete button is added next to conversation threads, with an event listener that triggers a confirmation dialog and sends a POST request upon confirmation. The UI then updates to remove the deleted conversation, and any active view is adjusted accordingly. Changes
Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
website/templates/messaging.html (1)
243-243
: Remove console.log statements.These debugging statements should be removed before deploying to production. They're marked with "to remove" comments but were left in the code.
- console.log("Attempting WebSocket connection..."); // to remove + // Connection attempt- console.log("WebSocket connection established"); // to remove + // Connection established- console.error("WebSocket error:", error); + // Handle WebSocket error silently or log to a monitoring service- console.error("Error creating WebSocket:", error); // to remove + // Handle WebSocket creation errorAlso applies to: 247-247, 261-261, 267-267
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
blt/urls.py
(2 hunks)website/templates/messaging.html
(4 hunks)website/views/user.py
(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
website/views/user.py (1)
website/models.py (1) (1)
Thread
(2364-2369)
blt/urls.py (1)
website/views/user.py (1) (1)
delete_thread
(1108-1119)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (4)
blt/urls.py (1)
1100-1100
: New URL pattern looks good.The API endpoint follows RESTful conventions and properly integrates with the existing URL patterns.
website/views/user.py (1)
1107-1119
: Well-implemented thread deletion function.The function includes:
- Proper authentication via the
@login_required
decorator- Authorization check to ensure only participants can delete threads
- Appropriate error handling with correct HTTP status codes
- Clear JSON responses for all scenarios
website/templates/messaging.html (2)
33-33
: Check participant condition for delete button.The delete button placement might have an issue with the
{% if participant != request.user %}
condition. Since this is inside a loop, the condition will only evaluate correctly for the last participant. If there are multiple participants besides the current user, this condition may not work as expected.Consider revising to ensure the delete button always appears exactly once per conversation thread, regardless of the number of participants.
415-458
: Delete thread functionality implementation looks good.The implementation:
- Prevents event propagation to avoid triggering the conversation click
- Confirms deletion with the user before proceeding
- Sends a proper POST request with CSRF token
- Handles the UI updates appropriately
- Closes WebSocket and resets state when deleting the active thread
However, consider enhancing the error messages based on the server response:
} else { - alert('Failed to delete conversation'); + alert(`Failed to delete conversation: ${data.message || 'Unknown error'}`); }
@codedrabbitai can you please confirm this deletes everything related to the chat? |
@coderabbitai can you please confirm this deletes everything related to the chat? |
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainAfter examining the code, I need more information about the Let me check more specifically how the deletion is implemented in the view function and the model relationships: Let me get more specific information about these models and the delete functionality: I need to understand more about the message deletion by looking at the Thread-Message relationship and any custom deletion logic: @DonnieBLT After examining the code, I can confirm that the PR implements thread deletion functionality through a dedicated endpoint. When a user clicks the close button for a chat:
In Django's ORM, when a model is deleted, it will automatically delete related objects if the foreign key relationship has However, without seeing the complete model definitions with the foreign key relationships, I can't be 100% certain that all related data is deleted. It depends on how the models are structured and whether cascade deletion is properly configured. If you want absolute confirmation, we would need to see the complete Thread and Message model definitions to verify the on_delete behavior. 🏁 Script executed:
Length of output: 4016 🏁 Script executed:
Length of output: 67406 🏁 Script executed:
Length of output: 4960 🏁 Script executed:
Length of output: 5832 🏁 Script executed:
Length of output: 5650
So when you click the close button, the thread and all its messages are permanently removed from the database. Nothing related to that conversation will remain. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, please check the tests and remove console.logs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
website/templates/messaging.html (1)
27-33
: Clarify the Conditional for Rendering the Delete Icon
The delete icon is rendered with the condition{% if participant != request.user %}
. Note that the variableparticipant
here comes from the preceding for-loop, meaning that its final value may not reliably represent the intended state for showing the delete button in multi-participant threads. Consider verifying whether this condition correctly reflects when a user should see the close (delete) button—for example, by checking a dedicated thread property (e.g., thread owner or a verified permission flag).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
website/templates/messaging.html
(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Run Tests
- GitHub Check: docker-test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
website/templates/messaging.html (1)
409-456
: Review and Enhance the Delete Thread Event Listener
The new deletion logic correctly stops event propagation, retrieves the thread’s ID, and sends a confirmation prompt before issuing a POST request to delete the thread. Upon successful deletion, it removes the thread element and clears the chat window if the deleted thread was active. A couple of improvements are suggested:
- Response Validation: Before parsing the JSON response, check
response.ok
to ensure the HTTP request succeeded. For example:- const response = await fetch(`/api/messaging/thread/${threadId}/delete/`, { - method: 'POST', - headers: { - 'X-CSRFToken': getCookie('csrftoken'), - 'Content-Type': 'application/json' - } - }); - - const data = await response.json(); + const response = await fetch(`/api/messaging/thread/${threadId}/delete/`, { + method: 'POST', + headers: { + 'X-CSRFToken': getCookie('csrftoken'), + 'Content-Type': 'application/json' + } + }); + if (!response.ok) { + throw new Error('Network response was not OK'); + } + const data = await response.json();- Error Feedback: Instead of using
alert()
for failures, consider implementing a more user-friendly inline notification for error states (this can help avoid disruptive UI experiences).- Console Logging: A console error is logged in the catch block. Note that previous reviews (e.g., by DonnieBLT) requested the removal of excessive console logs, so please consider cleaning these up in production code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes made
untitled-close-btn.mp4
resolves #4024
A close button was added to every chat/thread. On clicking this it will delete that thread/chat and messages on it from the database.
Summary by CodeRabbit
Summary by CodeRabbit