Conversation
… it for shared profiles Previously shared profiles loaded the sharer's contacts instead of their own voice. Now we extract and store an embedding when a user uploads their speech profile, and load that embedding during transcription for shared profile speaker identification. Also removes the broken persistent speaker_id mapping that incorrectly cached session-local IDs across sessions.
- Merge speech_profile_sharing.dart into speech_profile.dart - Add Pydantic model for share/revoke request body - Validate target user exists before sharing - Remove dead get_user_speaker_embedding function - Use single Firestore read per shared owner in transcription - Add l10n keys for sharing UI strings - Add Logger.debug to Dart API functions - Fix indentation and EOF newlines
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature for sharing speech profiles between users. It adds the necessary backend API endpoints for sharing, revoking, and listing shared profiles, along with corresponding UI elements in the mobile app. The backend is also updated to extract and utilize speaker embeddings from both user-owned and shared profiles for improved speaker identification. The implementation is solid, but the identified high-severity issues related to database query efficiency and error handling still need to be addressed.
| docs = shared_ref.stream() | ||
| shared = [] | ||
| for doc in docs: | ||
| data = doc.to_dict() | ||
| if not data.get('revoked_at'): | ||
| shared.append(data['shared_with_uid']) | ||
| return shared |
There was a problem hiding this comment.
This implementation fetches all sharing documents for a user and then filters them in Python to find the non-revoked ones. This is inefficient as it can lead to reading many unnecessary documents from Firestore. It's better to apply a filter directly in the query to only fetch the relevant documents, which is more performant and cost-effective.
| docs = shared_ref.stream() | |
| shared = [] | |
| for doc in docs: | |
| data = doc.to_dict() | |
| if not data.get('revoked_at'): | |
| shared.append(data['shared_with_uid']) | |
| return shared | |
| shares_query = shared_ref.where(filter=firestore.FieldFilter('revoked_at', '==', None)) | |
| return [doc.to_dict()['shared_with_uid'] for doc in shares_query.stream()] |
| except Exception as e: | ||
| print(f"Failed to extract speaker embedding during profile upload: {e}", uid) |
There was a problem hiding this comment.
Catching a broad except Exception is risky as it can hide bugs and swallow system-level exceptions (e.g., KeyboardInterrupt). It's better to catch more specific exceptions that you expect to handle, such as those related to network requests or database operations. Additionally, using print for logging errors is not ideal for production environments. A structured logger should be used to provide more context and allow for better error tracking and monitoring.
| except Exception as e: | ||
| print(f"Failed to load shared profile from {owner_uid}: {e}", uid, session_id) |
There was a problem hiding this comment.
Similar to another comment, using a broad except Exception can mask underlying issues and make debugging difficult. It's recommended to catch more specific exceptions, for instance, those related to Firestore database operations when calling user_db.get_user_profile. Also, print is not suitable for production-level error logging; a dedicated logging library would provide better observability and error management.
|
working on these and tests |
1 participant
Issue #3040
Description
Adds support for sharing speech profiles between users