feat: soft-delete conversations with batch checkbox support#566
feat: soft-delete conversations with batch checkbox support#566HirenGajjar wants to merge 7 commits into
Conversation
📝 WalkthroughWalkthroughImplements one-sided soft deletion of conversations by adding per-user deletion flags to the database schema, updating queries to exclude deleted conversations, creating a repository delete method, exposing a DELETE API endpoint, and providing frontend UI for bulk multi-select deletion with corresponding i18n strings and error handling. ChangesConversation Soft-Delete Feature
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
I think that the schema-check fails because the removed files in the |
|
@FrenchGithubUser |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/views/conversation/ConversationsView.vue (1)
1-160:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix Prettier violations to unblock Frontend CI.
This file is currently failing
prettier --check src, which is blocking the pipeline.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/views/conversation/ConversationsView.vue` around lines 1 - 160, The file fails Prettier checks; run your formatter or apply Prettier rules to ConversationsView.vue and fix spacing/quoting/order issues so the file matches project formatting. Specifically, format the template and script blocks, ensure import statements and const declarations (e.g., imports for DataTable/Column, useConversationSearch, and the consts t, notificationsStore, selectedIds) follow project Prettier settings, and reformat the isConversationRead and deleteSelected functions (including consistent spacing around operators, ternary branches, and arrow functions) to resolve the CI failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/storage/src/models/conversation.rs`:
- Around line 158-161: Run cargo fmt to fix the formatting warning in this file;
then add an API-level validation for
DeleteConversationsRequest::conversation_ids so empty Vecs are rejected before
DB calls — e.g., in the request handler that receives DeleteConversationsRequest
check that request.conversation_ids.is_empty() and return a 400/validation error
if so (or wire a validator on DeleteConversationsRequest if your project uses a
validation crate), ensuring you reference DeleteConversationsRequest and its
conversation_ids field when adding the check.
In `@backend/storage/src/repositories/conversation_repository.rs`:
- Around line 446-469: Validate and return early when the conversation_ids slice
is empty in the API handler delete_conversations.rs to avoid issuing a no-op DB
query; add a guard at the start of the handler that returns a 400/204 (choose
project convention) if the incoming list is empty. Also decide and implement
consistent "resurrection" behavior: if you want a new message to undelete the
conversation, update create_conversation_message to clear the appropriate
deleted_by_sender/deleted_by_receiver flag(s) for the conversation (identify
conversation by conversation_id used in create_conversation_message) when
inserting a message; otherwise document/leave behavior as permanent. Ensure you
reference the conversation_repository::delete_conversations logic and
list_conversations/search_conversations filters when making the change so flags
remain consistent.
In `@frontend/src/views/conversation/ConversationsView.vue`:
- Line 19: Replace the hardcoded label "Delete selected" in
ConversationsView.vue with a localized i18n lookup (e.g. use this.$t(...) or the
<i18n> key binding used elsewhere) so the delete action uses the translations;
locate the attribute label="Delete selected" and change it to use the same i18n
key pattern as other labels in this view (for example
conversations.deleteSelected) and add the corresponding key to your locale
files.
- Around line 127-140: The deleteSelected function currently removes
conversations from searchResults and clears selectedIds regardless of the DELETE
API outcome; modify deleteSelected to await the fetch response, check
response.ok (or HTTP status) and only on a successful response update
searchResults and reset selectedIds.value, otherwise do not change local state
and surface an error (e.g., throw, return, or show a UI error). Ensure you
reference the same symbols: deleteSelected, selectedIds.value, and
searchResults.value so the UI update is strictly gated by a successful response.
---
Outside diff comments:
In `@frontend/src/views/conversation/ConversationsView.vue`:
- Around line 1-160: The file fails Prettier checks; run your formatter or apply
Prettier rules to ConversationsView.vue and fix spacing/quoting/order issues so
the file matches project formatting. Specifically, format the template and
script blocks, ensure import statements and const declarations (e.g., imports
for DataTable/Column, useConversationSearch, and the consts t,
notificationsStore, selectedIds) follow project Prettier settings, and reformat
the isConversationRead and deleteSelected functions (including consistent
spacing around operators, ternary branches, and arrow functions) to resolve the
CI failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f084a3e8-e5e4-4504-932e-4ba2acadf3c2
📒 Files selected for processing (12)
backend/api/src/handlers/conversations/delete_conversations.rsbackend/api/src/handlers/conversations/mod.rsbackend/common/src/error/mod.rsbackend/storage/.sqlx/query-1a6dc57754fac6101be5a22d8a2dccd75115f1d7e8549df2a1fbfa58ae7f685f.jsonbackend/storage/.sqlx/query-4d9607ff2d309b1bca47660d3ba69c794d9eef2fcae0e584e103882e72414d9a.jsonbackend/storage/.sqlx/query-90af7add601e367b15cc3de3add6a5711724242eb7728522014c9a6116c74b0e.jsonbackend/storage/.sqlx/query-d5d60d7bc3f8609f335124dd9db0e473d5e1e55cb53a4a82eed74234964b67cd.jsonbackend/storage/.sqlx/query-f711b5827411954a9c91da7bfbbe1aa0e1cf3f9caff40ac25617472f92e26bcd.jsonbackend/storage/migrations/20250514000001_add_conversation_soft_delete.sqlbackend/storage/src/models/conversation.rsbackend/storage/src/repositories/conversation_repository.rsfrontend/src/views/conversation/ConversationsView.vue
| #[derive(Debug, Deserialize, ToSchema)] | ||
| pub struct DeleteConversationsRequest { | ||
| pub conversation_ids: Vec<i64>, | ||
| } No newline at end of file |
There was a problem hiding this comment.
Address the formatting warning and consider validating conversation_ids.
The static analysis tool flagged a formatting issue. Run cargo fmt to fix it.
Additionally, consider validating that conversation_ids is non-empty at the API layer to avoid unnecessary database round-trips.
🧰 Tools
🪛 GitHub Check: cargo fmt
[warning] 159-159:
Diff in /home/runner/work/arcadia/arcadia/backend/storage/src/models/conversation.rs
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/storage/src/models/conversation.rs` around lines 158 - 161, Run cargo
fmt to fix the formatting warning in this file; then add an API-level validation
for DeleteConversationsRequest::conversation_ids so empty Vecs are rejected
before DB calls — e.g., in the request handler that receives
DeleteConversationsRequest check that request.conversation_ids.is_empty() and
return a 400/validation error if so (or wire a validator on
DeleteConversationsRequest if your project uses a validation crate), ensuring
you reference DeleteConversationsRequest and its conversation_ids field when
adding the check.
|
@HirenGajjar looks better now, thanks! Could you now run And thanks for this PR! |
|
@HirenGajjar let me know when you're done with the coderabbit comments and I'll have a look at your changes :) btw feel free to join the discord server if you wanna chat some time! (link in readme) |
- validate empty conversation_ids in delete handler (400) - guard UI state update behind successful DELETE response - use i18n for delete button label - add missing newline at end of conversation.rs - run cargo fmt and prettier
|
could you also post a screenshot of how the UI looks like? |
FrenchGithubUser
force-pushed
the
main
branch
2 times, most recently
from
4c57b76 to
bf0ed97
Compare
|
Here's a UI mockup showing the new feature — a checkbox column is added as the first column in the conversations table. |
|
don't you have a screenshot @HirenGajjar ? |
|
| ) -> Result<HttpResponse> { | ||
| if body.conversation_ids.is_empty() { | ||
| return Ok(HttpResponse::BadRequest().json(serde_json::json!({ | ||
| "error": "conversation_ids cannot be empty" |
There was a problem hiding this comment.
In general use a variant from the existing Error enum, you can add a variant if necessary.
| @@ -0,0 +1,3 @@ | |||
| ALTER TABLE conversations | |||
There was a problem hiding this comment.
db changes should be added in the existing migration file, don't create new ones until we have a first release.
| </div> | ||
| </ContentContainer> | ||
|
|
||
| <div v-if="selectedIds.length > 0" class="delete-bar"> |
There was a problem hiding this comment.
| <div v-if="selectedIds.length > 0" class="delete-bar"> | |
| <div v-if="selectedIds.length > 0" class="actions"> |
This will also be used for other action btns
| const deleteSelected = async () => { | ||
| if (selectedIds.value.length === 0) return | ||
|
|
||
| const response = await fetch('/api/conversations', { |
There was a problem hiding this comment.
Don't use a raw fetch but instead use the existing api client, like it is done in other components. It also looks like this won't actually use the configured api endpoint, but just the window url
| body: JSON.stringify({ conversation_ids: selectedIds.value }), | ||
| }) | ||
|
|
||
| if (!response.ok) return |
There was a problem hiding this comment.
use .then() and .finally() instead
|
|
||
| # kiwiirc client | ||
| kiwiirc/config.json | ||
| .sqlx/ |
- move migration to existing initdb.sql, remove separate migration file - use Error enum variant for empty conversation_ids validation - rename delete-bar css class to actions - use axios api client instead of raw fetch - use .then()/.finally() pattern - remove .sqlx/ from .gitignore - run cargo fmt and prettier
|
Addressed all review comments:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/views/conversation/ConversationsView.vue (1)
119-127:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSync pagination totals after successful delete.
Line 123 removes rows locally, but
totalResultsis not decremented, so paginator totals can become stale until a refresh.Suggested patch
const deleteSelected = () => { + const idsToDelete = [...selectedIds.value] api - .delete('/api/conversations', { data: { conversation_ids: selectedIds.value } }) + .delete('/api/conversations', { data: { conversation_ids: idsToDelete } }) .then(() => { - searchResults.value = searchResults.value.filter((c) => !selectedIds.value.includes(c.conversation_id as number)) + searchResults.value = searchResults.value.filter((c) => !idsToDelete.includes(c.conversation_id as number)) + totalResults.value = Math.max(0, totalResults.value - idsToDelete.length) }) .finally(() => { selectedIds.value = [] }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/views/conversation/ConversationsView.vue` around lines 119 - 127, The deleteSelected handler removes items from searchResults but doesn't update the pagination count; inside the successful .then() of deleteSelected (the function name), after filtering searchResults.value, decrement totalResults.value (or the pagination total variable) by the number of removed items (e.g., const removed = selectedIds.value.length) and guard it to never go below zero, so the paginator totals stay in sync with searchResults.value; leave the selectedIds reset in .finally() as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/storage/migrations/20250312215600_initdb.sql`:
- Around line 1572-1574: The init migration 20250312215600_initdb.sql was edited
to add two columns (ALTER TABLE conversations ADD COLUMN deleted_by_sender
BOOLEAN ... and deleted_by_receiver ...); revert that change in the historical
init file and instead create a new forward migration (new timestamped SQL file)
that runs ALTER TABLE conversations ADD COLUMN deleted_by_sender BOOLEAN NOT
NULL DEFAULT FALSE, ADD COLUMN deleted_by_receiver BOOLEAN NOT NULL DEFAULT
FALSE so existing DBs get the change when migrations are run and the original
init migration remains immutable.
---
Duplicate comments:
In `@frontend/src/views/conversation/ConversationsView.vue`:
- Around line 119-127: The deleteSelected handler removes items from
searchResults but doesn't update the pagination count; inside the successful
.then() of deleteSelected (the function name), after filtering
searchResults.value, decrement totalResults.value (or the pagination total
variable) by the number of removed items (e.g., const removed =
selectedIds.value.length) and guard it to never go below zero, so the paginator
totals stay in sync with searchResults.value; leave the selectedIds reset in
.finally() as-is.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 29e0e828-4a46-4afc-9318-53a4f0920ced
📒 Files selected for processing (4)
backend/api/src/handlers/conversations/delete_conversations.rsbackend/common/src/error/mod.rsbackend/storage/migrations/20250312215600_initdb.sqlfrontend/src/views/conversation/ConversationsView.vue
| ALTER TABLE conversations | ||
| ADD COLUMN deleted_by_sender BOOLEAN NOT NULL DEFAULT FALSE, | ||
| ADD COLUMN deleted_by_receiver BOOLEAN NOT NULL DEFAULT FALSE; |
There was a problem hiding this comment.
Do not evolve schema by editing a historical init migration.
Line 1572 adds new schema changes to an old migration file. That risks missing columns in already-initialized environments because this file is not replayed there. Please move this ALTER TABLE into a new forward migration and keep 20250312215600_initdb.sql immutable.
Suggested change
-ALTER TABLE conversations
- ADD COLUMN deleted_by_sender BOOLEAN NOT NULL DEFAULT FALSE,
- ADD COLUMN deleted_by_receiver BOOLEAN NOT NULL DEFAULT FALSE;-- New migration file (new timestamp), e.g. backend/storage/migrations/<new>_add_conversation_soft_delete_flags.sql
ALTER TABLE conversations
ADD COLUMN deleted_by_sender BOOLEAN NOT NULL DEFAULT FALSE,
ADD COLUMN deleted_by_receiver BOOLEAN NOT NULL DEFAULT FALSE;📝 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.
| ALTER TABLE conversations | |
| ADD COLUMN deleted_by_sender BOOLEAN NOT NULL DEFAULT FALSE, | |
| ADD COLUMN deleted_by_receiver BOOLEAN NOT NULL DEFAULT FALSE; | |
| -- (previous migration content remains unchanged) | |
| -- The ALTER TABLE statements for deleted_by_sender and deleted_by_receiver have been removed from this historical init migration | |
| -- These changes should be added to a new forward migration file instead |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/storage/migrations/20250312215600_initdb.sql` around lines 1572 -
1574, The init migration 20250312215600_initdb.sql was edited to add two columns
(ALTER TABLE conversations ADD COLUMN deleted_by_sender BOOLEAN ... and
deleted_by_receiver ...); revert that change in the historical init file and
instead create a new forward migration (new timestamped SQL file) that runs
ALTER TABLE conversations ADD COLUMN deleted_by_sender BOOLEAN NOT NULL DEFAULT
FALSE, ADD COLUMN deleted_by_receiver BOOLEAN NOT NULL DEFAULT FALSE so existing
DBs get the change when migrations are run and the original init migration
remains immutable.
2 participants
Closes #564
What this does
deleted_by_senderordeleted_by_receiverto true depending on who deletesChanges
deleted_by_senderanddeleted_by_receiverboolean columns toconversationsDELETE /api/conversationsendpoint accepting{ conversation_ids: [id, ...] }Summary by CodeRabbit
New Features
Chores