Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR enhances WebSocket URL building with codec normalization and authentication parameters, while introducing Opus audio header-stripping detection throughout the audio processing pipeline. A new utility function identifies stripped headers based on metadata, and this flag is propagated from client initialization through packet processing on the backend. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Backend as WebSocket Controller
participant Detector as Header Detector
participant Decoder as Audio Decoder
participant Producer as Audio Stream Producer
Client->>Backend: audio_start payload with opus_header_stripped=True
Backend->>Detector: is_opus_header_stripped(audio_start_data)
Detector-->>Backend: strip_header=True
Backend->>Backend: Initialize session with strip_header flag
Client->>Backend: audio_chunk (Opus payload)
Backend->>Decoder: decode_packet(opus_payload, strip_header=True)
Decoder->>Decoder: Conditionally strip BLE header
Decoder-->>Producer: Decoded audio data
Producer->>Producer: Process audio stream
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
- Updated `useAudioStreamingOrchestrator` to ensure WebSocket URLs are correctly formatted and include codec parameters for Opus and PCM. - Introduced a new utility function `is_opus_header_stripped` to determine if incoming Opus payloads have the BLE header removed, improving audio processing accuracy. - Modified the WebSocket handling in `websocket_controller.py` to accommodate the new header stripping logic and updated audio session initialization. - Added unit tests for the new utility function to ensure correct behavior across various input scenarios.
- Improved URL normalization for WebSocket connections by trimming user input and converting HTTP(S) schemes to WS(S). - Ensured the presence of the `/ws` endpoint path in the WebSocket URL. - Updated codec handling to enforce Opus as the stream codec, either by replacing existing codec parameters or appending it if missing.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/hooks/useAudioStreamingOrchestrator.ts`:
- Around line 66-68: The code declares `const isAdvanced` twice in
`useAudioStreamingOrchestrator.ts`, which causes a compilation error; remove the
duplicate declaration so there is only a single `const isAdvanced =
settings.jwtToken && settings.isAuthenticated;` (keep the one used by the
surrounding logic) and update any subsequent references to `isAdvanced` if
necessary to use that single variable.
In
`@backends/advanced/src/advanced_omi_backend/controllers/websocket_controller.py`:
- Line 1410: The fallback for checking whether to strip the Opus header is
inconsistent: change the getattr call that reads getattr(client_state,
"opus_header_stripped", False) to use True so it matches the default behavior of
is_opus_header_stripped(); i.e., replace the default False with True so both the
property check and is_opus_header_stripped() assume the header is already
stripped and avoid incorrect stripping when an audio chunk arrives before
audio-start.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
app/src/hooks/useAudioStreamingOrchestrator.tsbackends/advanced/src/advanced_omi_backend/controllers/websocket_controller.pybackends/advanced/src/advanced_omi_backend/utils/omi_codec_utils.pybackends/advanced/tests/test_omi_codec_utils.pyextras/local-wearable-client/backend_sender.py
| const isAdvanced = settings.jwtToken && settings.isAuthenticated; | ||
|
|
||
| const isAdvanced = settings.jwtToken && settings.isAuthenticated; |
There was a problem hiding this comment.
Duplicate variable declaration will cause compilation error.
Lines 66 and 68 both declare const isAdvanced. This is a syntax error in TypeScript/JavaScript — you cannot redeclare a const variable in the same scope.
🐛 Proposed fix: Remove the duplicate declaration
if (/[?&]codec=/i.test(url)) {
url = url.replace(/([?&])codec=[^&]*/i, '$1codec=opus');
} else {
const sep = url.includes('?') ? '&' : '?';
url = url + sep + 'codec=opus';
}
- const isAdvanced = settings.jwtToken && settings.isAuthenticated;
-
const isAdvanced = settings.jwtToken && settings.isAuthenticated;
if (isAdvanced) {📝 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.
| const isAdvanced = settings.jwtToken && settings.isAuthenticated; | |
| const isAdvanced = settings.jwtToken && settings.isAuthenticated; | |
| if (/[?&]codec=/i.test(url)) { | |
| url = url.replace(/([?&])codec=[^&]*/i, '$1codec=opus'); | |
| } else { | |
| const sep = url.includes('?') ? '&' : '?'; | |
| url = url + sep + 'codec=opus'; | |
| } | |
| const isAdvanced = settings.jwtToken && settings.isAuthenticated; | |
| if (isAdvanced) { |
🧰 Tools
🪛 Biome (2.4.4)
[error] 68-68: Shouldn't redeclare 'isAdvanced'. Consider to delete it or rename it.
(lint/suspicious/noRedeclare)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/hooks/useAudioStreamingOrchestrator.ts` around lines 66 - 68, The
code declares `const isAdvanced` twice in `useAudioStreamingOrchestrator.ts`,
which causes a compilation error; remove the duplicate declaration so there is
only a single `const isAdvanced = settings.jwtToken &&
settings.isAuthenticated;` (keep the one used by the surrounding logic) and
update any subsequent references to `isAdvanced` if necessary to use that single
variable.
| audio_stream_producer, | ||
| payload, | ||
| _decode_packet, | ||
| not getattr(client_state, "opus_header_stripped", False), |
There was a problem hiding this comment.
Default value inconsistency with is_opus_header_stripped.
The fallback here is False (assume header is NOT stripped → strip it), while is_opus_header_stripped() defaults to True (assume header IS stripped → don't strip). If an audio chunk somehow arrives before audio-start, this mismatch could lead to incorrect decoding.
While the protocol should guarantee audio-start precedes audio-chunk, aligning the defaults would be safer:
🛡️ Proposed fix to align defaults
await _handle_omi_audio_chunk(
client_state,
audio_stream_producer,
payload,
_decode_packet,
- not getattr(client_state, "opus_header_stripped", False),
+ not getattr(client_state, "opus_header_stripped", True),
user.user_id,
client_id,
packet_count,
)📝 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.
| not getattr(client_state, "opus_header_stripped", False), | |
| not getattr(client_state, "opus_header_stripped", True), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backends/advanced/src/advanced_omi_backend/controllers/websocket_controller.py`
at line 1410, The fallback for checking whether to strip the Opus header is
inconsistent: change the getattr call that reads getattr(client_state,
"opus_header_stripped", False) to use True so it matches the default behavior of
is_opus_header_stripped(); i.e., replace the default False with True so both the
property check and is_opus_header_stripped() assume the header is already
stripped and avoid incorrect stripping when an audio chunk arrives before
audio-start.
AnkushMalaker
left a comment
AnkushMalaker
left a comment
There was a problem hiding this comment.
one duplicate that coderabbit caught,
the other, I think I wanna try my devkit once and see if we can remove this header thing across
| const isAdvanced = settings.jwtToken && settings.isAuthenticated; | ||
|
|
||
| const isAdvanced = settings.jwtToken && settings.isAuthenticated; |
2 participants
Summary by CodeRabbit
New Features
Bug Fixes