feat(backend,app): add /v1/conversations/{id}/trash endpoint with 30-day Trash UI (#7122)#7201
feat(backend,app): add /v1/conversations/{id}/trash endpoint with 30-day Trash UI (#7122)#7201mvanhorn wants to merge 6 commits into
Conversation
…day Trash UI (BasedHardware#7122) Adds a non-breaking soft-delete path so accidentally deleted conversations can be recovered. The existing DELETE /v1/conversations/{id}?cascade=true is unchanged - it remains the permanent-delete path used by the cascade cron and by the new "Delete forever" UI. Backend - New trashed_at field on Conversation (nullable, backward-compat) - POST /v1/conversations/{id}/trash flips trashed_at without cascade. Vectors and photos are preserved so restore is lossless. - POST /v1/conversations/{id}/restore clears trashed_at - GET /v1/conversations/trash returns trashed conversations newest first - get_conversations / get_conversations_count / iter_all_conversations filter trashed_at IS NULL by default with include_trashed override - filter_visible_conversation_ids hides trashed from chat search results (Pinecone query_vectors and Typesense search both call it) - New utils/other/purge_trashed.py cron runs daily at 03:00 UTC, finds trashed_at older than 30 days via collection_group query, invokes the existing cascade-delete path (conversation + photos + vectors + audio files + memories + memory vectors + action items). Wired into utils/other/jobs.start_job. PII-sanitized log lines. App - New Settings > Trash page (trash_page.dart) with Restore and Delete forever per row, days remaining indicator, empty state - "Move to Trash" added alongside the existing Delete in the conversation list item context menu - existing Delete is unchanged - API client adds trashConversationServer / restoreConversationServer / getTrashedConversations - ServerConversation schema gains trashedAt (Optional<DateTime>) - l10n keys added to app_en.arb. Until flutter gen-l10n is rerun, TrashLocalizationExtension on AppLocalizations provides the English values; once the maintainer's omi-add-missing-language-keys-l10n skill propagates translations and gen-l10n is rerun, the extension becomes a no-op (class methods shadow it) and can be removed. Closes BasedHardware#7122
- tests/integration/test_conversation_trash.py: trash, restore, trashed listing, default exclusion, missing-target 404, filter_visible_conversation_ids, and Typesense search exclusion. - tests/unit/test_purge_trashed_cron.py: schedule predicate, recent retention (29d kept), expired purge cleanup chain (conversation + vector + audio + memories + memory vectors + action items), and error-continuation behavior. - tests/integration/conftest.py: make firebase_admin and dotenv imports optional so mocked tests can collect without ADC creds. initialize_firebase fixture is a no-op when firebase_admin is None.
… fix search test Addresses self-review findings: - Trashing a public/shared conversation now revokes the Redis share mapping and sets visibility back to private. Restore does NOT auto-restore sharing - the user must re-share manually. This prevents shared links from continuing to serve trashed content through get_shared_conversation_by_id during the 30-day window. - get_conversations and get_conversations_without_photos now over-fetch up to min(limit * 3, 500) when include_trashed=False, then post-filter and cap at the requested limit. Without this, the list endpoint could return short or empty pages when the newest N conversations are all trashed. - TestSearchRedaction tests now patch conversations_db.filter_visible_conversation_ids as a pass-through so existing search tests continue to pass after the new visibility check was added in search.py. - Added shared-revoke-on-trash and over-fetch-pagination test cases to test_conversation_trash.py.
…ile rows Addresses two Round 2 review findings: - iter_all_conversations defaults to include_trashed=True. /v1/users/export uses this iterator and users exporting their data while items are in Trash should receive their full set, not a partial export. Callers that want only currently-visible conversations pass include_trashed=False. - conversation_list_item.dart: the overflow menu (Move to Trash) was only attached to the isNew=true branch in the first card layout, leaving conversations older than 60 seconds without any UI affordance to trash them on mobile. The non-new Row now includes _buildOverflowMenu(context) alongside the time/duration/star widgets, matching the second card layout and the desktop variant.
Greptile SummaryAdds a soft-delete "Trash" layer to conversations: a
Confidence Score: 3/5The app-facing endpoints and Flutter UI are solid, but the nightly purge cron will fail on its first run without a Firestore Collection Group index that is not included in the PR. The collection-group query in
Important Files Changed
Sequence DiagramsequenceDiagram
participant App
participant API as FastAPI Router
participant DB as Firestore (conversations_db)
participant Redis
participant Pinecone
participant Cron as Daily Cron (03:00 UTC)
App->>API: "POST /v1/conversations/{id}/trash"
API->>DB: trash_conversation(uid, id)
DB->>DB: "set trashed_at = now()"
DB->>Redis: remove public share if needed
DB-->>API: updated conversation dict
API-->>App: 200 Conversation
App->>API: "POST /v1/conversations/{id}/restore"
API->>DB: restore_conversation(uid, id)
DB->>DB: DELETE trashed_at field
API-->>App: 200 Conversation
App->>API: GET /v1/conversations/trash
API->>DB: get_trashed_conversations(uid)
DB-->>API: "docs where trashed_at != null"
API-->>App: 200 List[Conversation]
App->>API: Vector/Search query
API->>Pinecone: query_vectors
Pinecone-->>API: raw conversation_ids
API->>DB: filter_visible_conversation_ids(uid, ids)
DB-->>API: ids where trashed_at is null
API-->>App: filtered results
Cron->>DB: "list_expired_trashed(cutoff = now - 30d)"
DB-->>Cron: (uid, conversation_id) pairs
Cron->>DB: delete_conversation(uid, id)
Cron->>Pinecone: delete_vector(uid, id)
Cron->>DB: delete_memories / action_items
|
| for uid, conversation_id in conversations_db.list_expired_trashed(cutoff): | ||
| safe_uid = sanitize_pii(uid) | ||
| safe_conversation_id = sanitize_pii(conversation_id) | ||
| try: | ||
| conversations_db.delete_conversation(uid, conversation_id) | ||
| delete_vector(uid, conversation_id) | ||
| delete_conversation_audio_files(uid, conversation_id) | ||
|
|
||
| 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: | ||
| delete_memory_vector(uid, memory_id) | ||
|
|
||
| action_items_db.delete_action_items_for_conversation(uid, conversation_id) | ||
| purged_count += 1 |
There was a problem hiding this comment.
Missing Firestore Collection Group index for the purge query
db.collection_group(conversations_collection).where(filter=FieldFilter('trashed_at', '<', cutoff_dt)) is a collection-group inequality query. Firestore does not auto-generate Collection Group indexes — one must be created manually in the Firestore console or via firestore.indexes.json before this query executes. Without it the first nightly cron run at 03:00 UTC will throw a 400 FAILED_PRECONDITION exception inside list_expired_trashed, which propagates out of the for uid, conversation_id in … loop in purge_expired_trashed_conversations (that call is outside the per-conversation try/except), crashing the entire purge function and leaving all expired-trashed conversations permanently un-purged — a data-retention failure. The PR should include a firestore.indexes.json that registers a Collection Group index on conversations / trashed_at ASC.
| def get_conversations_count( | ||
| uid: str, include_discarded: bool = False, include_trashed: bool = False, statuses: List[str] = [] | ||
| ): | ||
| conversations_ref = db.collection('users').document(uid).collection(conversations_collection) | ||
| if not include_discarded: | ||
| conversations_ref = conversations_ref.where(filter=FieldFilter('discarded', '==', False)) | ||
| if statuses: | ||
| conversations_ref = conversations_ref.where(filter=FieldFilter('status', 'in', statuses)) | ||
| if not include_trashed: | ||
| count = 0 | ||
| for doc in conversations_ref.stream(): | ||
| if _is_not_trashed(doc.to_dict()): | ||
| count += 1 | ||
| return count | ||
| result = conversations_ref.count().get() | ||
| return int(result[0][0].value) |
There was a problem hiding this comment.
Full-collection scan on
get_conversations_count when include_trashed=False
When include_trashed=False, the function streams every document in the user's conversations subcollection and calls _is_not_trashed(doc.to_dict()) on each one, making it an O(n) client-side filter. For users with thousands of conversations this is slow and produces high read billing. The PR notes a backfill migration is out of scope, but once legacy documents have trashed_at=null consistently, a server-side WHERE trashed_at == NULL filter could replace the full scan. A comment acknowledging this known limitation would help future contributors avoid adding more callers that rely on an efficient count.
| extension TrashLocalizationExtension on AppLocalizations { | ||
| String get trash => 'Trash'; | ||
| String get trashEmpty => 'Trash is empty'; | ||
| String get trashDescription => 'Conversations in Trash are permanently deleted after 30 days.'; | ||
| String get moveToTrash => 'Move to Trash'; | ||
| String get restoreConversation => 'Restore'; | ||
| String get deleteForever => 'Delete forever'; |
There was a problem hiding this comment.
restoreConversation already exists as a key in app_en.arb (line 112), so it is already present in the generated AppLocalizations class. Dart will silently prefer the class method over the extension, making this getter dead code. It should be removed from the extension to avoid confusion when the temporary extension is eventually cleaned up.
| extension TrashLocalizationExtension on AppLocalizations { | |
| String get trash => 'Trash'; | |
| String get trashEmpty => 'Trash is empty'; | |
| String get trashDescription => 'Conversations in Trash are permanently deleted after 30 days.'; | |
| String get moveToTrash => 'Move to Trash'; | |
| String get restoreConversation => 'Restore'; | |
| String get deleteForever => 'Delete forever'; | |
| extension TrashLocalizationExtension on AppLocalizations { | |
| String get trash => 'Trash'; | |
| String get trashEmpty => 'Trash is empty'; | |
| String get trashDescription => 'Conversations in Trash are permanently deleted after 30 days.'; | |
| String get moveToTrash => 'Move to Trash'; | |
| String get deleteForever => 'Delete forever'; |
…cleanups) - Add backend/firestore.indexes.json with a Collection Group index on conversations/trashed_at ASC so the nightly purge cron's collection_group inequality query has the required index (greptile P1). - Document the O(n) client-side scan in get_conversations_count when include_trashed=False; once legacy docs are backfilled, a server-side WHERE trashed_at == NULL filter becomes feasible (greptile P2). - Remove dead restoreConversation getter from TrashLocalizationExtension; it already exists in AppLocalizations via app_en.arb (greptile P2).
|
Addressed in 0f9565a:
Lint check failure was on |
|
Hi @mvanhorn iirc Firestore indexes here are created manually (by maintainer ig) so |
Summary
Adds a non-breaking
POST /v1/conversations/{id}/trashendpoint with a Settings → Trash UI. The existingDELETE /v1/conversations/{id}?cascade=trueis unchanged and remains the permanent-delete path used by the cascade cron and by the new "Delete forever" UI.Why this matters
Issue #7122 asks for a Trash bin so accidentally deleted conversations can be recovered. For users who store months of life-log data, an accidental delete is currently unrecoverable.
Demo
Simulated demo (Remotion) of the move-to-trash flow and the Settings → Trash page. Backend can't run end-to-end without Firebase / Modal / Deepgram credentials in my workspace, so this is a programmatic mock against the real Omi color palette and the Trash page layout in this PR, not a live capture.
Design notes
Why a new
trashed_atfield instead of extendingdiscarded. The existingdiscardedboolean is a system heuristic (set_conversation_as_discardedflags short/empty convos). Trash is user-initiated and needs a timestamp for the 30-day purge. Mixing both into one field would force every consumer (search, count, list, iter_all) to distinguish "auto-discarded short convo" from "user-trashed long convo" by inference. A separate explicit field matches the project's existing pattern.Why the new endpoint is additive. The Flutter app at
app/lib/backend/http/api/conversations.dart:92always callsDELETE ...?cascade=true. Changing default DELETE semantics under that flag would orphan vector and audio state for current mobile clients. The trash endpoint is a new write path; existing call sites are untouched.API
/v1/conversations/{id}/trashtrashed_at, revokes share if public/v1/conversations/{id}/restoretrashed_at. Restore does NOT auto re-share./v1/conversations/trash/v1/conversations/{id}?cascade=trueinclude_trashed=Falseis the default forget_conversations/get_conversations_count.iter_all_conversationsdefaults toinclude_trashed=Trueso/v1/users/exportstill streams the full stored set.App
Settings → Trash lists trashed conversations with Restore and Delete-forever actions and a 30-day countdown indicator. Conversation list rows gain a "Move to Trash" overflow action on both the new and non-new card layouts. Existing Delete is unchanged.
Cron
backend/utils/other/purge_trashed.pyruns daily at 03:00 UTC, findstrashed_at < now - 30dvia Firestorecollection_group, and calls the existing cascade-delete logic (conversation + photos + vectors + audio + memories + memory vectors + action items). Wired intoutils/other/jobs.start_job. Logs usesanitize_pii().Vector / search
Trashed conversations are filtered at query time in
vector_db.query_vectors,vector_db.query_vectors_by_metadata, andutils/conversations/search.search_conversationsvia a newdatabase.conversations.filter_visible_conversation_idshelper. Vectors stay in Pinecone until permanent delete, so restore is lossless.Tests
backend/tests/integration/test_conversation_trash.py(502 lines): trash, restore, listing, default exclusion, share-revoke-on-trash, missing-target 404,filter_visible_conversation_ids, Typesense search exclusion, over-fetch pagination.backend/tests/unit/test_purge_trashed_cron.py(139 lines): schedule predicate, 29-day retention, 31-day expired purge cleanup chain, error continuation.backend/tests/unit/test_lock_bypass_fixes.py::TestSearchRedactionupdated to mockfilter_visible_conversation_idsas a pass-through.All 16 pass locally.
l10n
New keys are added to
app_en.arb. Non-English locales fall back to English at runtime viaTrashLocalizationExtensiononAppLocalizationsuntilflutter gen-l10nis rerun andomi-add-missing-language-keys-l10npropagates translations. The extension is then shadowed by the canonical class methods and can be removed.Out of scope
discarded+trashed_attrashed_at = nullon legacy docs (would let list/count use Firestore-nativeWHERE trashed_at == Nonefilters instead of the current over-fetch and post-filter)Fixes #7122