Skip to content

fix: delete associated audio, memories, and tasks when deleting conversation#5573

Open
mdmohsin7 wants to merge 7 commits intomainfrom
fix/delete-conversation-cascade-cleanup
Open

fix: delete associated audio, memories, and tasks when deleting conversation#5573
mdmohsin7 wants to merge 7 commits intomainfrom
fix/delete-conversation-cascade-cleanup

Conversation

@mdmohsin7
Copy link
Member

Summary

  • Always deletes audio files (chunks + merged) when a conversation is deleted
  • Optionally deletes associated memories and tasks via a new checkbox in the delete confirmation dialog
  • Backend accepts delete_memories and delete_action_items query params; audio cleanup runs unconditionally as a background task
  • Memory vectors are also cleaned up from Pinecone when memories are deleted
  • Added deleteAssociatedData l10n key with translations for all 34 locales

Closes #4868

Test plan

  • Swipe-to-delete a conversation → dialog appears with checkbox (unchecked by default)
  • Confirm delete without checkbox → conversation deleted, audio cleaned up, memories/tasks preserved
  • Confirm delete with checkbox checked → conversation, audio, memories, and tasks all deleted
  • Delete from conversation detail page menu → same dialog behavior
  • Cancel dialog → no deletion occurs
  • Offline → shows error dialog, no deletion
  • Ask Omi chat about deleted content → confirms info is gone when checkbox was checked

🤖 Generated with Claude Code

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 11, 2026

Greptile Summary

This PR extends the conversation-delete flow across the full stack: audio files are always cleaned up on the backend, and a new optional checkbox lets users also purge associated memories and tasks. The changes touch the FastAPI delete endpoint, Flutter's conversation provider, the swipe-to-delete list item, the detail page, a new dialog widget, and 34 locale files.

Key concerns:

  • Regression — showConversationDeleteConfirmation preference removed (conversation_list_item.dart): The swipe-to-delete path previously respected a user setting that skipped the confirmation dialog. That check has been dropped entirely, so users who disabled confirmation now always see the dialog. The preference key remains in SharedPreferencesUtil but is never read.
  • 204 response with a body (conversations.py): The DELETE endpoint is annotated status_code=204 but returns {"status": "Ok"}. HTTP 204 must have no body; FastAPI silently strips it, but the intent is misleading and could confuse clients.
  • Pinecone orphan risk on crash (conversations.py): Firestore memory records are deleted synchronously before the Pinecone vector-delete background tasks are enqueued. A process crash after Firestore deletion leaves orphaned vectors with no way to recover the IDs.
  • _deleteAssociatedDataOptions edge-case leak (conversation_provider.dart): In a narrow race between the 3-second undo timer and a second deletion, the map entry for the first conversation may never be cleaned up, replicating a pre-existing leak in memoriesToDelete/deleteTimestamps.
  • Offline dialog not awaited (conversation_list_item.dart): The offline error dialog is now fire-and-forget, causing the swipe item to snap back immediately rather than after the user dismisses the error.

Confidence Score: 2/5

  • Not safe to merge as-is — a confirmed UX regression (removed user preference) and a protocol violation (204 with body) should be addressed first.
  • The core feature logic is sound and the localization coverage is thorough, but the removal of the showConversationDeleteConfirmation preference is a clear behavioral regression for existing users, the 204-with-body is a protocol violation, and the Pinecone orphan risk is a real data-consistency concern. These issues lower confidence meaningfully.
  • app/lib/pages/conversations/widgets/conversation_list_item.dart (removed preference), backend/routers/conversations.py (204 body + orphan risk)

Important Files Changed

Filename Overview
backend/routers/conversations.py Extends DELETE endpoint to accept delete_memories and delete_action_items query params; always schedules audio file cleanup as a background task. Issues: 204 status with a non-empty return body, and potential orphaned Pinecone vectors if the process crashes between synchronous Firestore deletion and background vector tasks.
app/lib/providers/conversation_provider.dart Adds _deleteAssociatedDataOptions map to track per-conversation delete preferences through the undo window, passing them to deleteConversationServer. Cleanup paths are mostly correct but a rare edge-case can leave stale entries in _deleteAssociatedDataOptions.
app/lib/pages/conversations/widgets/conversation_list_item.dart Replaces inline swipe-delete dialog with showDeleteConversationDialog; removes the showConversationDeleteConfirmation user-preference check (regression — users who disabled confirmation now always see the dialog). The offline error dialog is also no longer awaited, changing Dismissible snap-back timing.
app/lib/pages/conversation_detail/page.dart Converts _handleDelete to async, awaits the new showDeleteConversationDialog, checks context.mounted before using context after await. Clean and correct implementation.
app/lib/widgets/dialog.dart Adds DeleteConversationOptions class and showDeleteConversationDialog function with a stateful checkbox. Well-structured with platform-adaptive dialogs (Cupertino/Material). Minor: the Material wrapper is only applied to the checkbox content on Apple, not to the full dialog, which may cause inconsistent tap-ripple behavior.
app/lib/backend/http/api/conversations.dart Adds deleteMemories and deleteTasks optional params to deleteConversationServer, building the query string manually. Functional but could use Uri query-parameter encoding to avoid potential special-character issues.
app/lib/l10n/app_en.arb Adds deleteAssociatedData key with English translation. No issues.

Sequence Diagram

sequenceDiagram
    participant U as User
    participant FL as Flutter UI
    participant CP as ConversationProvider
    participant API as Backend API
    participant FS as Firestore
    participant GCS as GCS Audio Storage
    participant PC as Pinecone

    U->>FL: Swipe-to-delete / Delete button
    FL->>FL: showDeleteConversationDialog()
    U->>FL: Confirm (± deleteAssociatedData checkbox)
    FL->>CP: deleteConversationLocally(conversation, deleteAssociatedData)
    CP->>CP: Store _deleteAssociatedDataOptions[id]
    CP->>CP: Remove from local list, notifyListeners()
    Note over CP: 3-second undo window
    CP->>API: DELETE /v1/conversations/{id}?delete_memories=&delete_action_items=
    API->>FS: delete_conversation(uid, id)
    API->>PC: delete_vector(uid, id)
    API-->>API: background: delete_conversation_audio_files
    API-->>GCS: Delete chunks/ & audio/ blobs
    opt delete_memories=true
        API->>FS: get_memory_ids_for_conversation(uid, id)
        API->>FS: delete_memories_for_conversation(uid, id)
        API-->>PC: background: delete_memory_vector(uid, memory_id) ×N
    end
    opt delete_action_items=true
        API->>FS: delete_action_items_for_conversation(uid, id)
    end
    API-->>FL: 204 No Content
Loading

Comments Outside Diff (1)

  1. app/lib/providers/conversation_provider.dart, line 680-684 (link)

    _deleteAssociatedDataOptions can leak entries in the "fast second delete" path

    When a second conversation is deleted within 3 seconds of the first, deleteConversationLocally immediately calls deleteConversationOnServer(lastDeletedConversationId!) for the previous pending ID — which correctly cleans up that entry. However, if the 3-second Future.delayed fires for conversation A but the condition lastDeletedConversationId == conversation.id is false (because a later deletion updated lastDeletedConversationId to B after A's immediate flush already happened), deleteConversationOnServer is never called for A and the entry in _deleteAssociatedDataOptions (along with memoriesToDelete and deleteTimestamps) is never removed.

    This pre-existing leak for memoriesToDelete/deleteTimestamps is now also replicated for _deleteAssociatedDataOptions. Consider cleaning up all three maps unconditionally inside the Future.delayed callback:

    Future.delayed(const Duration(seconds: 3), () {
      if (memoriesToDelete.containsKey(conversation.id)) {
        if (lastDeletedConversationId == conversation.id) {
          deleteConversationOnServer(conversation.id);
        } else {
          // Ensure maps are cleaned up even if server call was already made elsewhere
          memoriesToDelete.remove(conversation.id);
          deleteTimestamps.remove(conversation.id);
          _deleteAssociatedDataOptions.remove(conversation.id);
        }
      }
    });
    

Last reviewed commit: 29faaa9

Comment on lines 191 to 193
confirmDismiss: (direction) async {
HapticFeedback.mediumImpact();
bool showDeleteConfirmation = SharedPreferencesUtil().showConversationDeleteConfirmation;

if (!showDeleteConfirmation) return Future.value(true);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

showConversationDeleteConfirmation preference silently dropped

The previous implementation respected a user preference (SharedPreferencesUtil().showConversationDeleteConfirmation) that allowed skipping the confirmation dialog on swipe-to-delete. That preference still exists in preferences.dart but is no longer read here, meaning users who had set it to false (no confirmation) will now always see the dialog — a silent behavioral regression.

If the intent is to always require confirmation for the new checkbox UX, the preference should either be migrated/removed from SharedPreferencesUtil or the code should explicitly document that the preference is intentionally ignored, rather than leaving a dead settings key.

Suggested change
confirmDismiss: (direction) async {
HapticFeedback.mediumImpact();
bool showDeleteConfirmation = SharedPreferencesUtil().showConversationDeleteConfirmation;
if (!showDeleteConfirmation) return Future.value(true);
confirmDismiss: (direction) async {
HapticFeedback.mediumImpact();

Comment on lines 196 to +210
@@ -221,13 +206,27 @@ class _ConversationListItemState extends State<ConversationListItem> {
),
context: context,
);
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Offline error dialog not awaited — Dismissible snaps back immediately

showDialog is called without await, so return false executes synchronously and the Dismissible animation snaps the item back while the error dialog appears simultaneously. In the previous code the confirmDismiss future was the dialog future itself, meaning the item stayed in its swipe position until the dialog was dismissed. Consider awaiting the dialog so the two events are sequenced:

Suggested change
if (!connectivityProvider.isConnected) {
await showDialog(
builder: (c) => getDialog(
context,
() => Navigator.pop(context),
() => Navigator.pop(context),
context.l10n.unableToDeleteConversation,
context.l10n.pleaseCheckInternetConnectionAndTryAgain,
singleButton: true,
okButtonText: context.l10n.ok,
),
context: context,
);
return false;
}


return {"status": "Ok"}


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

204 No Content response must not return a body

The endpoint is decorated with status_code=204 but the function returns {"status": "Ok"}. HTTP 204 (No Content) must have no response body. FastAPI/Starlette typically strips the body, but the return type is misleading and some proxy layers may surface this. The response body should either be removed or the status code changed to 200.

Suggested change
return

Comment on lines +221 to +227

if delete_memories:
memory_ids = memories_db.get_memory_ids_for_conversation(uid, conversation_id)
memories_db.delete_memories_for_conversation(uid, conversation_id)
for memory_id in memory_ids:
background_tasks.add_task(delete_memory_vector, uid, memory_id)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Memory vectors deleted after Firestore records — orphan risk on crash

delete_memories_for_conversation is called synchronously (removing Firestore records), and the Pinecone vector deletes are then scheduled as background tasks. If the process dies after the Firestore commit but before the background tasks complete, the Pinecone vectors for those memories will be permanently orphaned because the document IDs are now gone and can't be re-fetched.

Consider either:

  1. Fetching and passing the memory IDs into background tasks that handle both Firestore deletion and vector deletion atomically, or
  2. Moving the Firestore deletion itself into a background task alongside the vector deletion so both fail or succeed together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Deleting summaries does not delete the underlying transcript or audio file

1 participant