Conversation
![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 functionality to save audio files from offline synced conversations, making their storage consistent with real-time audio. The changes involve importing new utility functions, adding a new function upload_wav_as_audio_chunks to handle WAV file processing and uploading, and integrating this new function into the conversation processing logic for both new and updated conversations. The overall approach seems sound, and the suggested improvement regarding error handling is valid and has been retained.
| except Exception as e: | ||
| print(f"Error uploading audio chunks for conversation {conversation_id}: {e}") |
There was a problem hiding this comment.
Catching a generic Exception can mask unexpected issues and make debugging harder. It's generally better to catch more specific exceptions that you anticipate, such as IOError or wave.Error for file operations, or Exception as a last resort after specific ones. Additionally, using print for error messages is not ideal for production systems; consider using a proper logging mechanism (e.g., Python's logging module) which allows for configurable log levels and destinations.
| except Exception as e: | |
| print(f"Error uploading audio chunks for conversation {conversation_id}: {e}") | |
| except (IOError, wave.Error, Exception) as e: | |
| logging.error(f"Error uploading audio chunks for conversation {conversation_id}: {e}") |
|
@mdmohsin7 Blocking issues before merge: Can you run by AI for @beastoin |
|
Done @beastoin Already tested locally as well (convos get created alongside the audio files stored correctly and playing them back works) |
|
@mdmohsin7 In by AI for @beastoin |
|
Hey @mdmohsin7, sorry about this — we're closing this one as not planned for now. Thanks for putting in the work, appreciate it. |
|
Hey @mdmohsin7 👋 Thank you so much for taking the time to contribute to Omi! We truly appreciate you putting in the effort to submit this pull request. After careful review, we've decided not to merge this particular PR. Please don't take this personally — we genuinely try to merge as many contributions as possible, but sometimes we have to make tough calls based on:
Your contribution is still valuable to us, and we'd love to see you contribute again in the future! If you'd like feedback on how to improve this PR or want to discuss alternative approaches, please don't hesitate to reach out. Thank you for being part of the Omi community! 💜 |
But we need this, right now users can't have playback or download audio of conversations that were synced offline because the audio of those conversations isn't stored even if private cloud sync is enabled. Overall playback and download experience is inconsistent because of this issue |
|
Hey 👋 — thanks for putting this together! Before we can review, could you share a quick live demo (screenshot, screen recording, or terminal output) showing this working on your local or dev environment? In the AI era, writing code is the easy part — what really makes a PR stand out is proof that it works end-to-end. A short video or even a screenshot goes a long way in helping reviewers feel confident about merging. Feel free to update this PR whenever you have something to show. Thanks! 🙏 |
|
@greptile-apps review |
Greptile SummaryThis PR extends the offline sync pipeline to save audio files to cloud storage when Private Cloud Sync is enabled, mirroring the realtime WebSocket path. Each decoded WAV segment is split into 5-second PCM16 chunks and uploaded via Remaining concerns:
Confidence Score: 2/5
Last reviewed commit: 80f81ba |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if n_channels != 1 or sampwidth != 2: | ||
| print( | ||
| f"Warning: WAV format mismatch for {wav_path} - expected mono PCM16, got {n_channels}ch {sampwidth*8}bit" | ||
| ) |
There was a problem hiding this comment.
WAV format validation only logs a warning but does not abort. A stereo or non-16bit file will continue to line 653 and be processed as if it were mono PCM16, corrupting the audio data. This should be a hard guard, not just a log:
| if n_channels != 1 or sampwidth != 2: | |
| print( | |
| f"Warning: WAV format mismatch for {wav_path} - expected mono PCM16, got {n_channels}ch {sampwidth*8}bit" | |
| ) | |
| if n_channels != 1 or sampwidth != 2: | |
| print( | |
| f"Error: WAV format mismatch for {wav_path} - expected mono PCM16, got {n_channels}ch {sampwidth*8}bit. Skipping upload." | |
| ) | |
| return |
| try: | ||
| with wave.open(wav_path, 'rb') as wav_file: | ||
| n_channels = wav_file.getnchannels() | ||
| sampwidth = wav_file.getsampwidth() | ||
| framerate = wav_file.getframerate() | ||
| n_frames = wav_file.getnframes() | ||
|
|
||
| # Validate WAV format matches expected PCM16 mono | ||
| if n_channels != 1 or sampwidth != 2: | ||
| print( | ||
| f"Warning: WAV format mismatch for {wav_path} - expected mono PCM16, got {n_channels}ch {sampwidth*8}bit" | ||
| ) | ||
|
|
||
| pcm_data = wav_file.readframes(n_frames) | ||
|
|
||
| # Calculate chunk size in bytes using actual WAV parameters | ||
| # PCM16 mono: 2 bytes per sample | ||
| bytes_per_second = framerate * n_channels * sampwidth | ||
| chunk_size = int(chunk_duration * bytes_per_second) | ||
|
|
||
| # Split into chunks and upload | ||
| offset = 0 | ||
| chunk_index = 0 | ||
| while offset < len(pcm_data): | ||
| chunk_data = pcm_data[offset : offset + chunk_size] | ||
| chunk_timestamp = start_timestamp + (chunk_index * chunk_duration) | ||
|
|
||
| upload_audio_chunk(chunk_data, uid, conversation_id, chunk_timestamp) | ||
|
|
||
| offset += chunk_size | ||
| chunk_index += 1 | ||
|
|
||
| # Free memory | ||
| del pcm_data | ||
|
|
||
| except Exception as e: | ||
| print(f"Error uploading audio chunks for conversation {conversation_id}: {e}") |
There was a problem hiding this comment.
The entire function body is wrapped in a bare except Exception as e: print(...) that swallows all errors. If any upload_audio_chunk call fails (network timeout, permission error, bucket misconfiguration), the exception is silently caught, the loop exits, and the function returns with no indication of failure. The caller in process_segment (lines 717, 756) cannot detect this failure, so create_audio_files_from_chunks will later return an empty or partial list, and the conversation's audio_files will either not be written or be incomplete—with no error surfaced to the API response.
Either raise the exception or return a success/failure flag so the caller can report meaningful errors:
except Exception as e:
print(f"Error uploading audio chunks for conversation {conversation_id}: {e}")
raise # let the caller decide how to handle partial uploads
Previously the validation only logged a warning and continued processing, which would silently corrupt audio data for non-mono or non-PCM16 files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Hey @mdmohsin7 👋 Friendly reminder — this PR has been quiet for a bit. If you don't need further assistance from the CTO, feel free to go ahead and merge it. Let's close the loop and not let it go stale. Thanks! |
|
Don't let your PR go stale. Unfortunately, I won't review further PRs from the internal team proactively, so please reopen and mention others who have review permissions. It's important to keep PRs from going stale. |
|
Hey @mdmohsin7 👋 Thank you so much for taking the time to contribute to Omi! We truly appreciate you putting in the effort to submit this pull request. After careful review, we've decided not to merge this particular PR. Please don't take this personally — we genuinely try to merge as many contributions as possible, but sometimes we have to make tough calls based on:
Your contribution is still valuable to us, and we'd love to see you contribute again in the future! If you'd like feedback on how to improve this PR or want to discuss alternative approaches, please don't hesitate to reach out. Thank you for being part of the Omi community! 💜 |
2 participants
Audio files that were synced through offline sync were not being saved (like how realtime audio is saved when save to cloud is on). This PR adds the ability to save the audio files of offline synced conversations