Conversation
📝 WalkthroughWalkthroughThis PR contains documentation updates to agent rules and refactors PTT (Push-To-Talk) interface logic from separate press/release handlers to toggle-based control with auto-connect behavior, alongside web-specific audio track management enhancements in the LiveKit store for proper playback and cleanup. Changes
Sequence DiagramsequenceDiagram
participant User
participant PTTInterface as PTT Interface
participant LiveKitStore as LiveKit Store
participant WebAudio as Web Audio Elements
User->>PTTInterface: Toggle PTT Button
alt Not Connected
PTTInterface->>LiveKitStore: Auto-connect (select default channel)
LiveKitStore->>LiveKitStore: connectToRoom()
LiveKitStore->>LiveKitStore: Register TrackSubscribed handler
end
PTTInterface->>LiveKitStore: Start transmitting
Note over LiveKitStore: Remote audio track received
LiveKitStore->>LiveKitStore: TrackSubscribed event fires
LiveKitStore->>WebAudio: Create & attach HTMLAudioElement
LiveKitStore->>WebAudio: Append to document.body
WebAudio-->>User: Audio playback to user
User->>PTTInterface: Toggle PTT Button (off)
PTTInterface->>LiveKitStore: Stop transmitting
Note over LiveKitStore: Remote audio track unsubscribed
LiveKitStore->>WebAudio: TrackUnsubscribed event fires
WebAudio->>WebAudio: Pause & remove element
LiveKitStore->>LiveKitStore: Remove from webAudioElements Map
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/dispatch-console/ptt-interface.tsx (1)
80-101:⚠️ Potential issue | 🟠 MajorDouble-tap can trigger concurrent connection attempts.
handlePTTTogglechecksisConnectedbut notisConnecting. A rapid double-tap while disconnected will callconnect()twice concurrently becauseisConnectingis set insideconnectToRoom(after the first async tick), and this handler doesn't gate on it. This can result in two rooms being created or an error from the second connection attempt.Proposed fix — guard on `isConnecting`
const handlePTTToggle = useCallback(async () => { + if (isConnecting) return; + if (!isConnected) { // If not connected, try to connect firstAnd add
isConnectingto the dependency array:- }, [isConnected, availableChannels, pttChannel, pttTransmitting, selectChannel, connect, startTransmitting, stopTransmitting]); + }, [isConnected, isConnecting, availableChannels, pttChannel, pttTransmitting, selectChannel, connect, startTransmitting, stopTransmitting]);
🤖 Fix all issues with AI agents
In `@src/stores/app/livekit-store.ts`:
- Around line 273-293: The forEach callback passed to track.detach() uses a
concise arrow that implicitly returns a value (el => el.remove()), which Biome
flags; change it to a block-bodied callback so it does not return anything—for
example in the RoomEvent.TrackUnsubscribed handler where
track.detach().forEach(...) replace the concise arrow with a block: (el) => {
el.remove(); } (or a function(el) { el.remove(); }) to ensure the callback
returns void and avoid the static analysis warning.
🧹 Nitpick comments (4)
src/components/dispatch-console/ptt-interface.tsx (3)
189-189: Icons use gluestack<Icon as={...}>wrapper instead of lucide components directly.Lines 189, 204, and 210 render icons via the gluestack
Iconcomponent. The project guidelines specify usinglucide-react-nativeicons directly in the markup without the gluestack wrapper. As per coding guidelines: "Uselucide-react-nativefor icons and use those components directly in the markup and don't use the gluestack-ui icon component."Example for one icon (apply pattern to all three)
- <Icon as={isConnected ? Wifi : WifiOff} size="2xs" color={isConnected ? '#22c55e' : colorScheme === 'dark' ? '#6b7280' : '#9ca3af'} /> + {isConnected ? ( + <Wifi size={12} color="#22c55e" /> + ) : ( + <WifiOff size={12} color={colorScheme === 'dark' ? '#6b7280' : '#9ca3af'} /> + )}Also applies to: 204-204, 210-210
243-246: Remove deadmutedButtonstyle.This style is explicitly noted as no longer used. Keeping dead code with a "backwards compatibility" comment is misleading — styles are internal to this component and have no external consumers.
Proposed removal
- // Note: mutedButton style kept for backwards compatibility but no longer used - mutedButton: { - backgroundColor: 'rgba(239, 68, 68, 0.1)', - },
131-158: Status indicator labels ("TX", "RX", "OFF", "...") are not wrapped int()for translation.These are user-visible text strings rendered in the connection indicator badges. Per coding guidelines, all text should use
react-i18nextfor translations..agent/rules/agent.md (1)
91-92: Markdown formatting nit: colon on a new line after bold heading.The pattern
**Anti-Repetition Protocol**\n:(and similar on lines 95-96, 99-100) places the colon on a new line. In standard Markdown this won't render as a definition list — the colon will appear as a standalone character. Move it to the same line or use a different format.Suggested fix (apply to all three)
-**Anti-Repetition Protocol** -: If a previously suggested fix is reported as failed, ... +**Anti-Repetition Protocol:** If a previously suggested fix is reported as failed, ...
| room.on(RoomEvent.TrackUnsubscribed, (track, publication, participant) => { | ||
| // On web, detach and remove audio elements | ||
| if (Platform.OS === 'web' && track.kind === Track.Kind.Audio) { | ||
| try { | ||
| track.detach().forEach((el) => el.remove()); | ||
| const trackSid = track.sid || publication.trackSid; | ||
| if (trackSid) { | ||
| webAudioElements.delete(trackSid); | ||
| } | ||
| logger.debug({ | ||
| message: 'Detached audio track', | ||
| context: { trackSid, participantIdentity: participant.identity }, | ||
| }); | ||
| } catch (err) { | ||
| logger.error({ | ||
| message: 'Failed to detach audio track', | ||
| context: { error: err, trackSid: track.sid }, | ||
| }); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Static analysis: forEach callback should not return a value.
el.remove() returns void, but the concise arrow (el) => el.remove() still constitutes an implicit return, which Biome flags as suspicious in an iterable callback. Use a block body.
Proposed fix
- track.detach().forEach((el) => el.remove());
+ track.detach().forEach((el) => { el.remove(); });📝 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.
| room.on(RoomEvent.TrackUnsubscribed, (track, publication, participant) => { | |
| // On web, detach and remove audio elements | |
| if (Platform.OS === 'web' && track.kind === Track.Kind.Audio) { | |
| try { | |
| track.detach().forEach((el) => el.remove()); | |
| const trackSid = track.sid || publication.trackSid; | |
| if (trackSid) { | |
| webAudioElements.delete(trackSid); | |
| } | |
| logger.debug({ | |
| message: 'Detached audio track', | |
| context: { trackSid, participantIdentity: participant.identity }, | |
| }); | |
| } catch (err) { | |
| logger.error({ | |
| message: 'Failed to detach audio track', | |
| context: { error: err, trackSid: track.sid }, | |
| }); | |
| } | |
| } | |
| }); | |
| room.on(RoomEvent.TrackUnsubscribed, (track, publication, participant) => { | |
| // On web, detach and remove audio elements | |
| if (Platform.OS === 'web' && track.kind === Track.Kind.Audio) { | |
| try { | |
| track.detach().forEach((el) => { el.remove(); }); | |
| const trackSid = track.sid || publication.trackSid; | |
| if (trackSid) { | |
| webAudioElements.delete(trackSid); | |
| } | |
| logger.debug({ | |
| message: 'Detached audio track', | |
| context: { trackSid, participantIdentity: participant.identity }, | |
| }); | |
| } catch (err) { | |
| logger.error({ | |
| message: 'Failed to detach audio track', | |
| context: { error: err, trackSid: track.sid }, | |
| }); | |
| } | |
| } | |
| }); |
🧰 Tools
🪛 Biome (2.3.13)
[error] 277-277: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
In `@src/stores/app/livekit-store.ts` around lines 273 - 293, The forEach callback
passed to track.detach() uses a concise arrow that implicitly returns a value
(el => el.remove()), which Biome flags; change it to a block-bodied callback so
it does not return anything—for example in the RoomEvent.TrackUnsubscribed
handler where track.detach().forEach(...) replace the concise arrow with a
block: (el) => { el.remove(); } (or a function(el) { el.remove(); }) to ensure
the callback returns void and avoid the static analysis warning.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
1 participant
Summary by CodeRabbit
New Features
Improvements