Production audit – Batch 6 (Call State, Lifecycle & Regression Test fixes) by arxdeployments · Pull Request #7 · arxdeployments/hive

arxdeployments · 2026-08-06T06:59:26Z

…era nothing could turn off, and three failures nobody was told about

Six confirmed defects from the web audit's remaining 40, under one theme: things the app loses, or lies about, without telling you. Each was re-verified against this tree before being touched, and each fix was adversarially reviewed — three of the six changed materially as a result, noted below.

A TEXT MESSAGE THE SERVER NEVER RECEIVED WAS DELETED FROM THE THREAD. ws.send() on a socket whose network has died neither throws nor drops; it buffers into a connection nobody is reading. So sendMessage returned true and the optimistic bubble sat on 'sending' with nothing anywhere able to resolve it: message_ack and the matching error frame are the only two things that can, the server publishes new_message to everyone except the sender, and message_status keys on the real uuid the optimistic row does not have yet. The reconnect refetch then replaced the array wholesale keeping only 'failed' rows, and the message was gone — no toast, no error, no retry. The comment on that filter said exactly this: scoped to 'failed' "deliberately NOT 'sending' — there is no ack timeout anywhere, so a preserved 'sending' bubble would hang for ever."

So this builds the ack timeout that comment says does not exist. sendMessage arms a deadline per temp_id; message_ack and error disarm it; expiry marks the row 'failed', which the existing carry-over preserves and which makes the existing retry control reachable. The deadline alone is not enough and the second half is the ordering-critical one: losing the socket flushes every pending send immediately, before anything can reconnect — wsConnected only goes true in _onOpen, and connect() early-returns while CONNECTING or OPEN, so no _onOpen can happen without passing through _onClose or _abandonSocket first. Without that flush a ghost socket abandoned after one 10s pong timeout reconnects and refetches before the 15s deadline fires, and the bug survives untouched.

Two things the review changed here. handleJumpToMessage runs the same wholesale replace with the same 'failed'-only filter, on a path with no socket loss and no elapsed deadline — so sending a message and then tapping a pinned message, a reply quote or a search hit deleted it outright, and the deadline then found no row to mark and gave up. That carry-over now keeps 'sending' too; an around-window is centred on an older message and by construction cannot contain a send still in flight. And disconnect() now flushes rather than leaving it to _onClose: _abandonSocket's docblock records the measurement that a close on a dead network can park in CLOSING and never fire onclose at all, which would leave deadlines armed past the end of the session, writing into a store nothing clears on logout. Its handlers are detached first, so a late onclose cannot flush the next socket's in-flight sends.

Worth stating plainly: where the server DID persist the message but the ack was lost to the close, the refetch returns the real message and the carried 'failed' bubble sits beside it — and the natural response, tapping retry, POSTs it again and delivers a second real message. That window is one publish wide (the ack is sent immediately after the insert) and cannot be closed without matching fetched rows to temp_ids, which the GET does not carry. It is the right trade against silently deleting a clinical message, but it is a real trade. The routine 4001 close is NOT that case: the server revalidates the token before dispatch and sends neither an ack nor an error frame, so the message genuinely never existed and marking it failed is correct. That is also the dominant trigger in the wild — not an exotic half-open TCP socket, but "the message frame is the first frame sent after the 15-minute access token lapses".

TWO RAPID MIC CLICKS LEFT THE MICROPHONE OPEN.
useAudioRecorder.start() guarded re-entry on state !== 'idle', but state is React state and is not set until AFTER the getUserMedia await, and no render happens during it — so both clicks read 'idle' and both opened a stream, and the second assignment to streamRef orphaned the first. releaseStream() could then only ever stop the survivor. Three more things rode on the orphan: its MediaRecorder kept appending into the shared chunk array, so the sent file interleaved two takes behind two container headers; its 200ms tick kept re-rendering the composer for the life of the page; and its track-watcher listeners stayed attached. A synchronous ref claimed before the first await closes it, cleared only in releaseStream() so its meaning never depends on React scheduling.

That guard alone regressed touch, which the review caught: with start() now refusing the second pointer, useHoldToTalk's { ok: false } path reset the shared held flag and the first finger's release stopped sending. The gesture now claims itself synchronously on pointerdown, so a second finger never reaches onStart and release-to-send still belongs to the first — the same argument one level up. Also closed here, the same leak by a different trigger: unmounting during the await (the composer is keyed on the conversation, so switching threads mid-acquisition does it) ran the cleanup while streamRef was still null, and the stream arrived into a dead instance nothing would ever call again.

A FAILED PUBLISH LEFT THE CAMERA CAPTURING WITH NO CONTROL ABLE TO STOP IT. _publishLocalMedia acquires real hardware and then publishes it, and a track whose publishTrack rejects never reaches trackPublications — so neither leave() (which only calls room.disconnect(), and disconnect walks publications) nor setCameraEnabled can reach it. The camera light stayed on for the rest of the call, and _attemptRejoin could strand one per attempt. Because the publish loop is sequential over a getUserMedia-ordered array, the mic was often already published when the camera failed, so the audio-only retry added a SECOND microphone publication — and setMicEnabled resolves a source to the first match, so Mute silently half-worked and the far side still heard us.

The obvious fix is a trap the review caught: await unpublishTrack(...) blocks on engine.negotiate(), whose deadline is 15s, against the very SFU that has just demonstrated it will not answer — holding the camera light on for those 15 seconds and delaying the audio-only fallback by the same, which is a behaviour change on the common path. So the hardware is stopped first and synchronously (LocalTrack.stop() chains to mediaStreamTrack.stop()), and the unpublish is fired without awaiting: in livekit-client 2.20.2 it deletes from trackPublications and emits LocalTrackUnpublished before its only yield engine.negotiate(), so the property we need is already established.

MUTE/UNMUTE WAS AN UNHANDLED REJECTION THAT TOLD THE PEER THE WRONG STATE. setMicEnabled had no try/catch and all three call sites are fire-and-forget, so a rejecting setMicrophoneEnabled — reachable when unmuting reopens a device the browser has ended, or when a mid-reconnect republish rejects — vanished into an unhandled rejection while ActiveCallView had already sent call:toggle_media claiming the state it never reached. LiveKit emits no TrackMuted/TrackUnmuted on a failed toggle, so nothing corrected the peer's roster until an unrelated toggle or a reconnect resync. It now returns the state actually reached, which the caller awaits before telling the peer, and says so with a toast; the store is still written only on success, so the local button cannot lie either. The no-room path (the mute button is live throughout "Connecting") reports the unchanged state rather than the requested one, because joinCall brings the mic up live moments later.

This also removes a call-killer. A rejecting re-mute inside _attemptRejoin lands in its catch, which retries — but joinCall early-returns because the room already exists, so each retry re-runs only the failing re-mute, and after five attempts _finalizeDisconnect fires onUnexpectedDisconnect, which toasts "Call disconnected" and ends the call for both parties. A microphone that would not re-mute after a network blip was destroying healthy calls.

A REFUSED SCREEN SHARE FAILED IN COMPLETE SILENCE. startScreenShare logged and returned false with nothing on screen; stopScreenShare had no catch at all, so a rejection there was unhandled while the user's screen stayed shared. Both now report. A dismissed picker deliberately stays silent — backing out is a normal thing to do — which means telling it apart from a real block by message, since Chromium raises NotAllowedError for both: a cancelled picker is PERMISSION_DENIED and an OS-level refusal is PERMISSION_DENIED_BY_SYSTEM. That string is the one thing here no automated test can validate; it wants one manual check on a Mac with Screen Recording revoked before it is trusted. It fails safe either way — an unrecognised error still toasts. The review added the branch that matters most in practice: iOS Safari and any insecure context have no getDisplayMedia at all and throw DeviceUnsupportedError, so without it every iPhone tap on Share would have been sent looking for a setting that does not exist.

A TOO-SHORT VOICE NOTE AUTO-SENT THE NEXT ONE.
Send pressed while paused sets a pending-send flag and finalises the recording, but onstop lands on 'idle' rather than 'preview' when the take is empty or under the minimum duration — and nothing cleared the flag there. It outlived its recording: the bar came back with Discard disabled, and the NEXT take was uploaded the instant it finalised, with no review. An idempotent effect releases both flags whenever the recorder returns to idle, which by definition means there is no take to send.

VERIFIED
npm run lint (0 problems), npm run build, check_contracts.py (no drift), and the full Playwright suite — 54 passed, 0 failed, 0 skipped, up from 45. Nine of those are new and every one of them fails against the previous code, on the assertion that is the defect: the jump case fails with the message simply absent from the store, and the publish case fails on a camera track still reading 'live'. The one added data-testid (the reply-quote block) exists to reach an otherwise undrivable path and changes no behaviour.

REMAINING, TRACKED AND NOT ADDRESSED HERE
Newly found while fixing the voice note: all three of the recorder's abort branches leave result set, leaking the paused partial preview's object URL on every abandoned take. Still open from the 44: the 1.75 MB entry chunk that pulls LiveKit and the emoji dataset onto the login path, admin search's uncancelled per-keystroke requests, two load errors that render as empty states, and the accessibility work.

Summary by CodeRabbit

Bug Fixes
- Improved microphone mute/unmute reliability and reporting during calls.
- Added clearer error handling for microphone and screen-sharing failures.
- Prevented microphone leaks and duplicate recordings during rapid interactions or navigation.
- Improved hold-to-talk behavior when multiple pointers are used.
- Preserved sending and failed messages during reconnects, with unacknowledged messages now marked failed after a timeout.
- Prevented abandoned voice recordings from being sent accidentally.
Tests
- Added coverage for call media failures, message acknowledgment recovery, and recorder resource cleanup.

…era nothing could turn off, and three failures nobody was told about Six confirmed defects from the web audit's remaining 40, under one theme: things the app loses, or lies about, without telling you. Each was re-verified against this tree before being touched, and each fix was adversarially reviewed — three of the six changed materially as a result, noted below. A TEXT MESSAGE THE SERVER NEVER RECEIVED WAS DELETED FROM THE THREAD. `ws.send()` on a socket whose network has died neither throws nor drops; it buffers into a connection nobody is reading. So `sendMessage` returned true and the optimistic bubble sat on 'sending' with nothing anywhere able to resolve it: `message_ack` and the matching `error` frame are the only two things that can, the server publishes `new_message` to everyone except the sender, and `message_status` keys on the real uuid the optimistic row does not have yet. The reconnect refetch then replaced the array wholesale keeping only 'failed' rows, and the message was gone — no toast, no error, no retry. The comment on that filter said exactly this: scoped to 'failed' "deliberately NOT 'sending' — there is no ack timeout anywhere, so a preserved 'sending' bubble would hang for ever." So this builds the ack timeout that comment says does not exist. `sendMessage` arms a deadline per temp_id; `message_ack` and `error` disarm it; expiry marks the row 'failed', which the existing carry-over preserves and which makes the existing retry control reachable. The deadline alone is not enough and the second half is the ordering-critical one: losing the socket flushes every pending send immediately, before anything can reconnect — `wsConnected` only goes true in `_onOpen`, and `connect()` early-returns while CONNECTING or OPEN, so no `_onOpen` can happen without passing through `_onClose` or `_abandonSocket` first. Without that flush a ghost socket abandoned after one 10s pong timeout reconnects and refetches before the 15s deadline fires, and the bug survives untouched. Two things the review changed here. `handleJumpToMessage` runs the same wholesale replace with the same 'failed'-only filter, on a path with no socket loss and no elapsed deadline — so sending a message and then tapping a pinned message, a reply quote or a search hit deleted it outright, and the deadline then found no row to mark and gave up. That carry-over now keeps 'sending' too; an around-window is centred on an older message and by construction cannot contain a send still in flight. And `disconnect()` now flushes rather than leaving it to `_onClose`: `_abandonSocket`'s docblock records the measurement that a close on a dead network can park in CLOSING and never fire `onclose` at all, which would leave deadlines armed past the end of the session, writing into a store nothing clears on logout. Its handlers are detached first, so a late `onclose` cannot flush the next socket's in-flight sends. Worth stating plainly: where the server DID persist the message but the ack was lost to the close, the refetch returns the real message and the carried 'failed' bubble sits beside it — and the natural response, tapping retry, POSTs it again and delivers a second real message. That window is one publish wide (the ack is sent immediately after the insert) and cannot be closed without matching fetched rows to temp_ids, which the GET does not carry. It is the right trade against silently deleting a clinical message, but it is a real trade. The routine 4001 close is NOT that case: the server revalidates the token before dispatch and sends neither an ack nor an error frame, so the message genuinely never existed and marking it failed is correct. That is also the dominant trigger in the wild — not an exotic half-open TCP socket, but "the message frame is the first frame sent after the 15-minute access token lapses". TWO RAPID MIC CLICKS LEFT THE MICROPHONE OPEN. `useAudioRecorder.start()` guarded re-entry on `state !== 'idle'`, but `state` is React state and is not set until AFTER the getUserMedia await, and no render happens during it — so both clicks read 'idle' and both opened a stream, and the second assignment to `streamRef` orphaned the first. `releaseStream()` could then only ever stop the survivor. Three more things rode on the orphan: its MediaRecorder kept appending into the shared chunk array, so the sent file interleaved two takes behind two container headers; its 200ms tick kept re-rendering the composer for the life of the page; and its track-watcher listeners stayed attached. A synchronous ref claimed before the first await closes it, cleared only in `releaseStream()` so its meaning never depends on React scheduling. That guard alone regressed touch, which the review caught: with `start()` now refusing the second pointer, `useHoldToTalk`'s `{ ok: false }` path reset the shared `held` flag and the first finger's release stopped sending. The gesture now claims itself synchronously on pointerdown, so a second finger never reaches `onStart` and release-to-send still belongs to the first — the same argument one level up. Also closed here, the same leak by a different trigger: unmounting during the await (the composer is keyed on the conversation, so switching threads mid-acquisition does it) ran the cleanup while `streamRef` was still null, and the stream arrived into a dead instance nothing would ever call again. A FAILED PUBLISH LEFT THE CAMERA CAPTURING WITH NO CONTROL ABLE TO STOP IT. `_publishLocalMedia` acquires real hardware and then publishes it, and a track whose `publishTrack` rejects never reaches `trackPublications` — so neither `leave()` (which only calls `room.disconnect()`, and disconnect walks publications) nor `setCameraEnabled` can reach it. The camera light stayed on for the rest of the call, and `_attemptRejoin` could strand one per attempt. Because the publish loop is sequential over a getUserMedia-ordered array, the mic was often already published when the camera failed, so the audio-only retry added a SECOND microphone publication — and `setMicEnabled` resolves a source to the first match, so Mute silently half-worked and the far side still heard us. The obvious fix is a trap the review caught: `await unpublishTrack(...)` blocks on `engine.negotiate()`, whose deadline is 15s, against the very SFU that has just demonstrated it will not answer — holding the camera light on for those 15 seconds and delaying the audio-only fallback by the same, which is a behaviour change on the common path. So the hardware is stopped first and synchronously (`LocalTrack.stop()` chains to `mediaStreamTrack.stop()`), and the unpublish is fired without awaiting: in livekit-client 2.20.2 it deletes from `trackPublications` and emits `LocalTrackUnpublished` before its only `yield engine.negotiate()`, so the property we need is already established. MUTE/UNMUTE WAS AN UNHANDLED REJECTION THAT TOLD THE PEER THE WRONG STATE. `setMicEnabled` had no try/catch and all three call sites are fire-and-forget, so a rejecting `setMicrophoneEnabled` — reachable when unmuting reopens a device the browser has ended, or when a mid-reconnect republish rejects — vanished into an unhandled rejection while `ActiveCallView` had already sent `call:toggle_media` claiming the state it never reached. LiveKit emits no TrackMuted/TrackUnmuted on a failed toggle, so nothing corrected the peer's roster until an unrelated toggle or a reconnect resync. It now returns the state actually reached, which the caller awaits before telling the peer, and says so with a toast; the store is still written only on success, so the local button cannot lie either. The no-room path (the mute button is live throughout "Connecting") reports the unchanged state rather than the requested one, because `joinCall` brings the mic up live moments later. This also removes a call-killer. A rejecting re-mute inside `_attemptRejoin` lands in its catch, which retries — but `joinCall` early-returns because the room already exists, so each retry re-runs only the failing re-mute, and after five attempts `_finalizeDisconnect` fires `onUnexpectedDisconnect`, which toasts "Call disconnected" and ends the call for both parties. A microphone that would not re-mute after a network blip was destroying healthy calls. A REFUSED SCREEN SHARE FAILED IN COMPLETE SILENCE. `startScreenShare` logged and returned false with nothing on screen; `stopScreenShare` had no catch at all, so a rejection there was unhandled while the user's screen stayed shared. Both now report. A dismissed picker deliberately stays silent — backing out is a normal thing to do — which means telling it apart from a real block by message, since Chromium raises `NotAllowedError` for both: a cancelled picker is PERMISSION_DENIED and an OS-level refusal is PERMISSION_DENIED_BY_SYSTEM. That string is the one thing here no automated test can validate; it wants one manual check on a Mac with Screen Recording revoked before it is trusted. It fails safe either way — an unrecognised error still toasts. The review added the branch that matters most in practice: iOS Safari and any insecure context have no `getDisplayMedia` at all and throw `DeviceUnsupportedError`, so without it every iPhone tap on Share would have been sent looking for a setting that does not exist. A TOO-SHORT VOICE NOTE AUTO-SENT THE NEXT ONE. Send pressed while paused sets a pending-send flag and finalises the recording, but `onstop` lands on 'idle' rather than 'preview' when the take is empty or under the minimum duration — and nothing cleared the flag there. It outlived its recording: the bar came back with Discard disabled, and the NEXT take was uploaded the instant it finalised, with no review. An idempotent effect releases both flags whenever the recorder returns to idle, which by definition means there is no take to send. VERIFIED npm run lint (0 problems), npm run build, check_contracts.py (no drift), and the full Playwright suite — 54 passed, 0 failed, 0 skipped, up from 45. Nine of those are new and every one of them fails against the previous code, on the assertion that is the defect: the jump case fails with the message simply absent from the store, and the publish case fails on a camera track still reading 'live'. The one added `data-testid` (the reply-quote block) exists to reach an otherwise undrivable path and changes no behaviour. REMAINING, TRACKED AND NOT ADDRESSED HERE Newly found while fixing the voice note: all three of the recorder's abort branches leave `result` set, leaking the paused partial preview's object URL on every abandoned take. Still open from the 44: the 1.75 MB entry chunk that pulls LiveKit and the emoji dataset onto the login path, admin search's uncancelled per-keystroke requests, two load errors that render as empty states, and the accessibility work.

coderabbitai · 2026-08-06T06:59:44Z

📝 Walkthrough

Walkthrough

The PR adds failure recovery for LiveKit media operations, WebSocket message acknowledgements, voice recorder races, and hold-to-talk pointer ownership. It also adds browser tests for media failures, message recovery, and microphone-stream cleanup.

Changes

Reliability recovery

Layer / File(s)	Summary
LiveKit media failure handling `frontend/src/services/livekitClient.js`, `frontend/src/utils/callErrors.js`, `frontend/src/components/calls/ActiveCallView.jsx`, `frontend/tests/livekit-media-failures.spec.js`	LiveKit publication failures now clean up tracks and publications. Microphone and screen-share operations preserve actual state, handle failures, and show notifications.
Message acknowledgement recovery `frontend/src/services/websocket.js`, `frontend/src/components/chat/ChatPanel.jsx`, `frontend/tests/message-ack-timeout.spec.js`	WebSocket messages now fail after acknowledgement timeouts or connection events. Chat refetches retain failed and sending optimistic messages.
Recorder and gesture lifecycle `frontend/src/hooks/useAudioRecorder.js`, `frontend/src/hooks/useHoldToTalk.js`, `frontend/src/components/chat/MessageComposer.jsx`, `frontend/src/components/chat/MessageBubble.jsx`, `frontend/tests/voice-recorder-leak.spec.js`	Recorder startup claims the microphone once and cleans up late or failed resources. Hold-to-talk accepts one owning pointer. Voice-send state resets when recording becomes idle. A reply preview receives a test identifier.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

arxdeployments/hive#1: Overlaps the chat, call, recorder, and WebSocket code paths.
arxdeployments/hive#2: Overlaps the call, chat, recorder, and call-error changes.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main call-state, lifecycle, and regression-test fixes in the pull request.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch production-audit-batch-6

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 5

🧹 Nitpick comments (3)

frontend/src/services/livekitClient.js (1)
809-817: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add the call context to the mic-toggle failure log.

Line 810 logs the failure without callId. Every other diagnostic in this file attaches { callId: this.callId }. The returned value drives a call:toggle_media signaling frame in ActiveCallView, so a failure here needs to be correlatable to a call.

The rest of this branch is correct: the store is not written, the reached state is re-read after the await, and notifyMicToggleFailed gives direction-specific and reason-specific feedback.
♻️ Proposed change to include the call context
     } catch (err) {
-      console.error('[LiveKit] mic toggle failed:', err);
+      console.error('[LiveKit] mic toggle failed', { callId: this.callId, enabled }, err);
As per coding guidelines: "join failures and signaling events must be logged with the specified call context".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/src/services/livekitClient.js` around lines 809 - 817, Update the
failure log in the mic-toggle catch branch to include the call context object
with this.callId, matching the diagnostic format used elsewhere in the
surrounding service. Keep the existing rollback-free behavior, failure
notification, and state re-read unchanged.
Source: Coding guidelines
frontend/tests/livekit-media-failures.spec.js (1)
311-331: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert that a failed stop leaves isScreenSharing untouched.

The failing-stop scenario checks the rejection and the toast. It does not check the store. stopScreenShare keeps its useCallStore.setState inside the try on purpose, and the comment at livekitClient.js lines 889-893 states the reason: the share is still published when the call throws, so the flag must not move.

A regression that hoisted that setState out of the try would clear the flag while the screen stays shared. The Share control would then read "Share screen" mid-share. This spec would still pass. Add the store assertion so the documented guarantee is covered.
💚 Proposed addition to cover the state guarantee
+      const { default: useCallStore } = await import('/src/stores/callStore.js');
         livekitClient.room = {
           localParticipant: {
             setScreenShareEnabled: async () => { throw new Error('negotiation timed out'); },
           },
         };
+        // The share is still published when the stop throws, so the flag must
+        // not move — the setState lives inside the try for exactly this reason.
+        useCallStore.setState({ isScreenSharing: true });
         let stopThrew = null;
         try {
           await livekitClient.stopScreenShare();
         } catch (err) {
           stopThrew = String(err);
         }
         await new Promise((r) => { setTimeout(r, 50); });
 
         return {
           cancelled,
           blocked,
           unsupported,
           stopThrew,
+          stillSharing: useCallStore.getState().isScreenSharing,
           stopToasts: drain(),
           rejections: [...rejections],
         };
Reset the flag in the existing finally, and assert it:
expect(seen.stillSharing, 'a failed stop cleared the flag while the screen was still shared').toBe(true);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/tests/livekit-media-failures.spec.js` around lines 311 - 331, Update
the failed stop scenario around livekitClient.stopScreenShare() to capture the
existing finally-state value for isScreenSharing and return it as
seen.stillSharing, then assert it remains true after the rejection. Preserve the
current rejection and toast assertions while covering the guarantee that a
failed stop does not clear the sharing flag.
frontend/src/utils/callErrors.js (1)
126-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider the non-Chromium NotAllowedError case.

Line 127 keeps every NotAllowedError silent unless the message contains "by system". Chromium distinguishes a cancelled picker from a system block by that wording. Firefox and Safari do not. On those engines a genuine permission block produces no message at all, so the Share control appears to do nothing.

The trade-off is deliberate and documented, and the alternative produces a toast on every cancelled picker. If you want to close the gap without that noise, you can report only when the failure arrives faster than a human picker interaction, or gate the silent branch on a Chromium check.

Related: the TypeError branch on line 122 also catches an ordinary programming TypeError thrown from inside the LiveKit call and labels it "not supported in this browser".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/src/utils/callErrors.js` around lines 126 - 133, Update the
NotAllowedError handling in the call error logic to surface genuine permission
blocks in Firefox and Safari even when error.message is empty, while preserving
Chromium’s silent cancelled-picker behavior through a browser check or
equivalent timing distinction. Also narrow the TypeError handling so only the
expected unsupported-browser failure from the LiveKit call is labeled as
unsupported, rather than masking arbitrary programming TypeErrors.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@frontend/src/components/calls/ActiveCallView.jsx`:
- Around line 246-252: Update handleToggleMute to read the current mute target
from useCallStore.getState() immediately before calling setMicEnabled, rather
than using the render-scoped isMuted value. Preserve awaiting the returned
reached state and sending that value in the call:toggle_media websocket message.

In `@frontend/src/hooks/useHoldToTalk.js`:
- Around line 47-51: Update the pointer-tracking logic in the hold-to-talk hook
to store the first event’s pointerId instead of only using downRef. In the
start, move, up, and cancel handlers, ignore events whose pointerId does not
match the owning pointer; clear the stored owner in reset while preserving
existing recording behavior for the matching pointer.

In `@frontend/src/services/websocket.js`:
- Around line 409-415: Preserve uncertain delivery when acknowledgements are
missing instead of marking messages failed or allowing retries to create
duplicates. In frontend/src/services/websocket.js ranges 409-415, 431-433,
479-482, 1299, and 1519-1525, update _failPendingAck and related timeout/retry
flows accordingly; in range 1245-1260, add durable server-side idempotency using
the send key. In frontend/src/components/chat/ChatPanel.jsx ranges 235-243 and
960-974, reconcile fetched messages with optimistic rows by that key.

In `@frontend/tests/message-ack-timeout.spec.js`:
- Around line 76-106: Extend the test around the retry flow in “a send the
server never received survives the reconnect as retryable” to open Bob’s page
after retrying and verify the retried message appears there within 5 seconds.
Keep Alice’s existing sent-status assertions, and use the established cross-user
setup/helpers for Bob rather than treating Alice’s local status as delivery
confirmation.
- Around line 135-147: Update the local post helper to assert each response is
successful before returning or consuming it, including the response status or
body in any assertion message; ensure all fixture writes through post, including
the oldest, filler, and quoting requests, fail immediately when the API is
unhealthy.

---

Nitpick comments:
In `@frontend/src/services/livekitClient.js`:
- Around line 809-817: Update the failure log in the mic-toggle catch branch to
include the call context object with this.callId, matching the diagnostic format
used elsewhere in the surrounding service. Keep the existing rollback-free
behavior, failure notification, and state re-read unchanged.

In `@frontend/src/utils/callErrors.js`:
- Around line 126-133: Update the NotAllowedError handling in the call error
logic to surface genuine permission blocks in Firefox and Safari even when
error.message is empty, while preserving Chromium’s silent cancelled-picker
behavior through a browser check or equivalent timing distinction. Also narrow
the TypeError handling so only the expected unsupported-browser failure from the
LiveKit call is labeled as unsupported, rather than masking arbitrary
programming TypeErrors.

In `@frontend/tests/livekit-media-failures.spec.js`:
- Around line 311-331: Update the failed stop scenario around
livekitClient.stopScreenShare() to capture the existing finally-state value for
isScreenSharing and return it as seen.stillSharing, then assert it remains true
after the rejection. Preserve the current rejection and toast assertions while
covering the guarantee that a failed stop does not clear the sharing flag.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: ac94c50a-563c-4642-9e81-e71fd561960d

📥 Commits

Reviewing files that changed from the base of the PR and between 9d56e12 and fde4b84.

📒 Files selected for processing (12)

frontend/src/components/calls/ActiveCallView.jsx
frontend/src/components/chat/ChatPanel.jsx
frontend/src/components/chat/MessageBubble.jsx
frontend/src/components/chat/MessageComposer.jsx
frontend/src/hooks/useAudioRecorder.js
frontend/src/hooks/useHoldToTalk.js
frontend/src/services/livekitClient.js
frontend/src/services/websocket.js
frontend/src/utils/callErrors.js
frontend/tests/livekit-media-failures.spec.js
frontend/tests/message-ack-timeout.spec.js
frontend/tests/voice-recorder-leak.spec.js

coderabbitai · 2026-08-06T07:04:37Z

+  const handleToggleMute = async () => {
    // New mic-enabled state is `isMuted` (we're flipping the current mute flag).
-    livekitClient.setMicEnabled(isMuted);
+    const reached = await livekitClient.setMicEnabled(isMuted);
    wsClient.send({
-      type: 'call:toggle_media', call_id: callId, media_type: 'audio', enabled: isMuted,
+      type: 'call:toggle_media', call_id: callId, media_type: 'audio', enabled: reached,
    });
  };


🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Read the mute target from the store, not from the render closure.

Line 248 passes this render's isMuted, destructured at line 143. handleToggleCamera at line 271 deliberately reads useCallStore.getState().isCameraOn instead, and the comment above it states why: two taps inside one animation frame both see the same stale value.

handleToggleMute has the same exposure, and it has less protection than the camera path. setCameraEnabled serialises taps through _cameraQueue; setMicEnabled has no queue. If the user taps Mute twice before React re-renders, both calls compute setMicEnabled(false), the store settles on isMuted: true, and a second call:toggle_media frame repeats enabled: false. The second tap appears to do nothing.

Awaiting the reached state before sending the frame is correct. Only the target read needs to change.

🐛 Proposed fix to read the target from the store

const handleToggleMute = async () => { - // New mic-enabled state is `isMuted` (we're flipping the current mute flag). - const reached = await livekitClient.setMicEnabled(isMuted); + // Read from the STORE, not this render's `isMuted`, so two taps inside one + // frame ask for opposite states — the same rule as `handleToggleCamera`. + // The new mic-enabled state is the current mute flag. + const reached = await livekitClient.setMicEnabled(useCallStore.getState().isMuted); wsClient.send({ type: 'call:toggle_media', call_id: callId, media_type: 'audio', enabled: reached, }); };

As per path instructions: flag "stale closures over store state".

📝 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.

Suggested change

const handleToggleMute = async () => {

// New mic-enabled state is `isMuted` (we're flipping the current mute flag).

livekitClient.setMicEnabled(isMuted);

const reached = await livekitClient.setMicEnabled(isMuted);

wsClient.send({

type: 'call:toggle_media', call_id: callId, media_type: 'audio', enabled: isMuted,

type: 'call:toggle_media', call_id: callId, media_type: 'audio', enabled: reached,

});

};

const handleToggleMute = async () => {

// Read from the STORE, not this render's `isMuted`, so two taps inside one

// frame ask for opposite states — the same rule as `handleToggleCamera`.

// The new mic-enabled state is the current mute flag.

const reached = await livekitClient.setMicEnabled(useCallStore.getState().isMuted);

wsClient.send({

type: 'call:toggle_media', call_id: callId, media_type: 'audio', enabled: reached,

});

};

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/calls/ActiveCallView.jsx` around lines 246 - 252, Update handleToggleMute to read the current mute target from useCallStore.getState() immediately before calling setMicEnabled, rather than using the render-scoped isMuted value. Preserve awaiting the returned reached state and sending that value in the call:toggle_media websocket message.

Source: Path instructions

coderabbitai · 2026-08-06T07:04:37Z

+  // A gesture is owned by the first pointer that lands, until it is released.
+  // `held` cannot express that: it is React state, so it has not updated yet
+  // while onStart is still awaiting the microphone, and a second finger reads
+  // the same stale value.
+  const downRef = useRef(false);


🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Track the owning pointer ID.

downRef blocks a second start, but it does not bind move, up, or cancel events to the first pointer. If a second finger releases on this control, onPointerUp can reset, cancel, or send the first pointer's recording.

Store the first e.pointerId. Clear it in reset. Ignore events from other pointers.

Proposed fix

const downRef = useRef(false); +const pointerIdRef = useRef(null); const reset = useCallback(() => { // ... downRef.current = false; + pointerIdRef.current = null; }, []); if (downRef.current) return; downRef.current = true; +pointerIdRef.current = e.pointerId; const onPointerMove = useCallback((e) => { + if (e.pointerId !== pointerIdRef.current) return; if (!held || !origin.current || locked) return; // ... }, [held, locked, onLock]); const onPointerUp = useCallback((e) => { + if (e.pointerId !== pointerIdRef.current) return; if (!held) return; // ... }, [held, drag.x, locked, reset, onCancel, onSend]);

Also applies to: 64-77

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/hooks/useHoldToTalk.js` around lines 47 - 51, Update the pointer-tracking logic in the hold-to-talk hook to store the first event’s pointerId instead of only using downRef. In the start, move, up, and cancel handlers, ignore events whose pointerId does not match the owning pointer; clear the stored owner in reset while preserving existing recording behavior for the matching pointer.

coderabbitai · 2026-08-06T07:04:37Z

+  _failPendingAck(tempId) {
+    const pending = this._pendingAcks.get(tempId);
+    if (!pending) return;
+    this._clearPendingAck(tempId);
+    const msgs = useChatStore.getState().messages[pending.convId] || [];
+    if (!msgs.some(m => m.temp_id === tempId && m.status === 'sending')) return;
+    useChatStore.getState().replaceOptimisticMessage(pending.convId, tempId, { status: 'failed' });


🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash set -euo pipefail # Inspect the client-side delivery and reconciliation contract. rg -n -C 5 'temp_id|message_ack|_failPendingAck|_failAllPendingAcks|failedLocal|localOnly' \ frontend/src/services/websocket.js \ frontend/src/components/chat/ChatPanel.jsx # Verify that server-side message creation enforces idempotency for the sender, # conversation, and temp_id, and that GET serialization returns the key needed # for client reconciliation. fd -a '^messaging\.py$' . | while IFS= read -r file; do echo "=== $file ===" rg -n -C 8 'temp_id|message_ack|idempot|conversation.*messages|serialize' "$file" done

Repository: arxdeployments/hive

Length of output: 30584

🏁 Script executed:

#!/bin/bash set -euo pipefail echo '=== message model and schema fields ===' rg -n -C 6 'class Message|temp_id|message_ack|type": "message_ack"|send_message\(' \ backend frontend/src | head -n 300 echo '=== message read serializers and fetch paths ===' rg -n -C 8 'serialize_message|GET.*messages|list_messages|messages.*conversation|has_more|around' \ backend | head -n 400 echo '=== websocket message receive and acknowledgement flow ===' rg -n -C 12 'type.*message|msg_type|send_message|message_ack|temp_id' \ backend | head -n 400

Repository: arxdeployments/hive

Length of output: 50375

🏁 Script executed:

#!/bin/bash set -euo pipefail echo '=== backend/app/db/models.py: Message definition ===' sed -n '205,260p' backend/app/db/models.py echo '=== backend/app/services/messaging.py: send_message ===' sed -n '109,252p' backend/app/services/messaging.py echo '=== backend/app/realtime/hub.py: websocket send path ===' sed -n '175,222p' backend/app/realtime/hub.py echo '=== message history route and serializer output ===' rg -l 'router.*conversations|`@router.get`.*messages|serialize_message' backend/app/api backend/app/services \ | while IFS= read -r file; do echo "--- $file ---" rg -n -C 5 'messages|serialize_message|temp_id' "$file" | head -n 180 done echo '=== backend temp_id handling outside echo/transport fields ===' rg -n 'temp_id' backend/app --glob '*.py'

Repository: arxdeployments/hive

Length of output: 41470

Do not treat a missing acknowledgement as proof that a message was not persisted.

The server persists the message before publishing message_ack. The messages table has no temp_id, and history responses cannot reconcile persisted messages with optimistic rows. Retrying with a new temp_id can create a duplicate message.

frontend/src/services/websocket.js#L409-L415, #L431-L433, #L479-L482, #L1299, #L1519-L1525: preserve uncertain delivery instead of marking every unacknowledged send as failed.

frontend/src/services/websocket.js#L1245-L1260: add server-side durable idempotency for the send key.

frontend/src/components/chat/ChatPanel.jsx#L235-L243, #L960-L974: reconcile local rows with fetched messages by that key.

📍 Affects 2 files

frontend/src/services/websocket.js#L409-L415 (this comment)

frontend/src/services/websocket.js#L431-L433

frontend/src/services/websocket.js#L479-L482

frontend/src/services/websocket.js#L1245-L1260

frontend/src/services/websocket.js#L1299-L1299

frontend/src/services/websocket.js#L1519-L1525

frontend/src/components/chat/ChatPanel.jsx#L235-L243

frontend/src/components/chat/ChatPanel.jsx#L960-L974

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/services/websocket.js` around lines 409 - 415, Preserve uncertain delivery when acknowledgements are missing instead of marking messages failed or allowing retries to create duplicates. In frontend/src/services/websocket.js ranges 409-415, 431-433, 479-482, 1299, and 1519-1525, update _failPendingAck and related timeout/retry flows accordingly; in range 1245-1260, add durable server-side idempotency using the send key. In frontend/src/components/chat/ChatPanel.jsx ranges 235-243 and 960-974, reconcile fetched messages with optimistic rows by that key.

coderabbitai · 2026-08-06T07:04:37Z

+test('a send the server never received survives the reconnect as retryable', async ({ page }) => {
+  const socket = await interceptSocket(page);
+  await uiLogin(page, seeded.users.alice.email, seeded.users.alice.password);
+  await expect(page.getByTestId('conversation-search')).toBeVisible();
+  await openConversationWith(page, 'Bob E2E');
+
+  socket.drop = true;
+  const body = `never acked ${suffix}`;
+  await sendText(page, body);
+  await expect(page.locator('[data-testid="message-status"][data-status="sending"]')).toBeVisible();
+
+  // Lose the socket well inside the 15s deadline. This is the ordering that
+  // matters: the reconnect refetch runs long before the deadline would fire, so
+  // resolving the send at socket loss is what keeps the bubble alive.
+  socket.drop = false;
+  socket.kill();
+
+  // The retry control is only rendered for 'failed', so its presence IS the
+  // assertion that the message survived the refetch in a recoverable state.
+  await expect(
+    page.getByTestId('message-retry'),
+    'the unacked message was deleted by the reconnect refetch instead of being made retryable'
+  ).toBeVisible({ timeout: 20000 });
+  await expect(page.getByText(body).first()).toBeVisible();
+
+  // ...and the control actually works: retrying puts the message on the server.
+  await page.getByTestId('message-retry').click();
+  await expect(page.locator('[data-testid="message-status"][data-status="sent"]').first())
+    .toBeVisible({ timeout: 15000 });
+  await expect(page.getByTestId('message-retry')).toHaveCount(0);
+});


📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Verify peer delivery within five seconds.

This test uses only Alice's page. A local sent status does not prove that Bob received the retried message. Open a Bob page and assert that the message appears there within 5 seconds.

As per coding guidelines, frontend/tests/**/*.js requires: “Playwright E2E tests must ... verify cross-user message delivery within 5 seconds.”

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/tests/message-ack-timeout.spec.js` around lines 76 - 106, Extend the test around the retry flow in “a send the server never received survives the reconnect as retryable” to open Bob’s page after retrying and verify the retried message appears there within 5 seconds. Keep Alice’s existing sent-status assertions, and use the established cross-user setup/helpers for Bob rather than treating Alice’s local status as delivery confirmation.

Source: Coding guidelines

coderabbitai · 2026-08-06T07:04:37Z

+  const post = (body) =>
+    page.request.post(`${process.env.E2E_API_URL || 'http://127.0.0.1:8000'}/api/conversations/${convId}/messages`, {
+      headers: { 'Content-Type': 'application/json', 'X-Requested-With': 'XMLHttpRequest' },
+      data: body,
+    });
+
+  const oldest = await (await post({ content: `oldest ${suffix}`, type: 'text' })).json();
+  for (let i = 0; i < 54; i += 1) {
+    await post({ content: `filler ${i} ${suffix}`, type: 'text' });
+  }
+  // The newest message quotes the oldest, giving a one-click route to a target
+  // that is now off the end of the loaded window.
+  await post({ content: `quoting ${suffix}`, type: 'text', reply_to: oldest._id });


📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Fail immediately when fixture writes fail.

post() returns unchecked responses. If the API is unhealthy, the test can fail later with a missing _id or an invalid window state. Assert response.ok() for every fixture request and include the response status or body in the assertion message.

Proposed fix

- const post = (body) => - page.request.post(`${process.env.E2E_API_URL || 'http://127.0.0.1:8000'}/api/conversations/${convId}/messages`, { + const post = async (body) => { + const response = await page.request.post(`${process.env.E2E_API_URL || 'http://127.0.0.1:8000'}/api/conversations/${convId}/messages`, { headers: { 'Content-Type': 'application/json', 'X-Requested-With': 'XMLHttpRequest' }, data: body, }); + expect(response.ok(), `fixture write failed with HTTP ${response.status()}`).toBeTruthy(); + return response; + };

As per coding guidelines, frontend/tests/**/*.js requires: “Playwright E2E tests must fail loudly when the API is unhealthy.”

📝 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.

Suggested change

const post = (body) =>

page.request.post(`${process.env.E2E_API_URL || 'http://127.0.0.1:8000'}/api/conversations/${convId}/messages`, {

headers: { 'Content-Type': 'application/json', 'X-Requested-With': 'XMLHttpRequest' },

data: body,

});

const oldest = await (await post({ content: `oldest ${suffix}`, type: 'text' })).json();

for (let i = 0; i < 54; i += 1) {

await post({ content: `filler ${i} ${suffix}`, type: 'text' });

}

// The newest message quotes the oldest, giving a one-click route to a target

// that is now off the end of the loaded window.

await post({ content: `quoting ${suffix}`, type: 'text', reply_to: oldest._id });

const post = async (body) => {

const response = await page.request.post(`${process.env.E2E_API_URL || 'http://127.0.0.1:8000'}/api/conversations/${convId}/messages`, {

headers: { 'Content-Type': 'application/json', 'X-Requested-With': 'XMLHttpRequest' },

data: body,

});

expect(response.ok(), `fixture write failed with HTTP ${response.status()}`).toBeTruthy();

return response;

};

const oldest = await (await post({ content: `oldest ${suffix}`, type: 'text' })).json();

for (let i = 0; i < 54; i += 1) {

await post({ content: `filler ${i} ${suffix}`, type: 'text' });

}

// The newest message quotes the oldest, giving a one-click route to a target

// that is now off the end of the loaded window.

await post({ content: `quoting ${suffix}`, type: 'text', reply_to: oldest._id });

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/tests/message-ack-timeout.spec.js` around lines 135 - 147, Update the local post helper to assert each response is successful before returning or consuming it, including the response status or body in any assertion message; ensure all fixture writes through post, including the oldest, filler, and quoting requests, fail immediately when the API is unhealthy.

Source: Coding guidelines

coderabbitai Bot reviewed Aug 6, 2026

View reviewed changes

arxdeployments merged commit 1dd80c5 into main Aug 6, 2026
7 checks passed

Navigation Menu

Repository navigation

Production audit – Batch 6 (Call State, Lifecycle & Regression Test fixes) - #7

Production audit – Batch 6 (Call State, Lifecycle & Regression Test fixes)#7
arxdeployments merged 1 commit into
mainfrom
production-audit-batch-6

arxdeployments commented Aug 6, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Aug 6, 2026 •

edited

Loading

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot Aug 6, 2026

Uh oh!

coderabbitai Bot Aug 6, 2026

Uh oh!

coderabbitai Bot Aug 6, 2026

Uh oh!

coderabbitai Bot Aug 6, 2026

Uh oh!

coderabbitai Bot Aug 6, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

arxdeployments commented Aug 6, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

arxdeployments commented Aug 6, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Aug 6, 2026 •

edited

Loading