From 83b59ec969d96cc1b2164aef34ee8980adc40296 Mon Sep 17 00:00:00 2001 From: Tanmay Deep Sharma Date: Wed, 6 May 2026 17:14:08 +0700 Subject: [PATCH] fix(voice): address PR #14346 review feedback in one pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Multi-agent / module-state correctness - Hoist WhatsApp outbound init lock to module scope so header + contact-panel buttons share one guard; add an active-session guard so a second click returns { status: 'locked' } instead of cleanup()-ing the live call. - isLocalWhatsappCall() filter on voice_call.outbound_connected and voice_call.ended cable handlers — account-wide broadcasts no longer feed foreign SDP into this tab's PeerConnection or stop its recorder. - Permission-flow path (200 with no call id) now releases the prepareOutboundOffer() mic + RTCPeerConnection instead of leaving the mic indicator stuck on. - Drop the intentionallyClosing guard around sendWhatsappTerminateBeacon so a hangup-then-close race still terminates Meta's side (beacon endpoint is idempotent). - Distinguish locked init from permission_requested in callers to avoid a false "call initiated" alert. Provider routing - joinCall outbound short-circuit now scoped to WhatsApp-like calls so FloatingCallWidget's auto-join for outbound Twilio still works. - isWhatsappLikeCall(callId-keyed) so calls seeded by message.updated / refresh path (which lack provider metadata) route to the WhatsApp flow. - syncConversationCallVisibility per-call filter via shouldShowCall, so outbound calls aren't ripped from under the caller on assignee change. - removeCallsForConversation tears down each active call via teardownByProvider — WhatsApp gets cleanupWhatsappSession (closes pc, stops recorder/mic) instead of a Twilio-only endClientCall. - await reject before dismissing in rejectIncomingCall so a failing reject keeps the call surfaced for retry. Per-bubble overhead - Split useCallSession into the root-mount hook + a lightweight useCallActions for components like VoiceCall.vue that just need state + actions without registering global window/Twilio listeners. Globals attach once via a refcount, dismissed-call sids live at module scope so the seed watcher can't re-add a locally dismissed ringing call. Lookup correctness - /contacts/:id/conversations accepts an optional inbox_id filter; the WhatsApp call button passes inboxId so a contact's older WhatsApp thread doesn't fall outside the BE's 20-row cap. Twilio lifecycle / security - Defer accepted_by_agent claim to the participant-join webhook so a failed agent device init doesn't leave the call ringing-but-claimed with no recovery path. mark_agent_joined still raises 409 if another agent has already claimed. - Verify X-Twilio-Signature on recording_status — the controller fetches the recording with channel auth credentials, so an unsigned POST could coerce credential-bearing requests to an attacker-controlled host. Legacy data - Migration to delete orphaned inboxes whose channel_type still says 'Channel::Voice' after the model was removed. The polymorphic belongs_to :channel lookup on those rows otherwise crashes the inbox serializer with `uninitialized constant Channel::Voice`. --- .../contacts/conversations_controller.rb | 4 + app/javascript/dashboard/api/contacts.js | 5 +- .../Contacts/VoiceCallButton.vue | 17 +- .../message/bubbles/VoiceCall.vue | 6 +- .../conversation/ConversationHeader.vue | 3 + .../dashboard/composables/useCallSession.js | 306 +++++++++++------- .../composables/useWhatsappCallSession.js | 49 ++- .../dashboard/helper/actionCable.js | 23 +- app/javascript/dashboard/helper/voice.js | 16 +- app/javascript/dashboard/stores/calls.js | 7 +- ...00_cleanup_legacy_channel_voice_inboxes.rb | 53 +++ db/schema.rb | 2 +- .../controllers/twilio/voice_controller.rb | 18 ++ .../provider/twilio/conference_service.rb | 15 +- .../twilio/conference_service_spec.rb | 15 +- 15 files changed, 380 insertions(+), 159 deletions(-) create mode 100644 db/migrate/20260502090000_cleanup_legacy_channel_voice_inboxes.rb diff --git a/app/controllers/api/v1/accounts/contacts/conversations_controller.rb b/app/controllers/api/v1/accounts/contacts/conversations_controller.rb index 20d66fb4d..e9ef0224b 100644 --- a/app/controllers/api/v1/accounts/contacts/conversations_controller.rb +++ b/app/controllers/api/v1/accounts/contacts/conversations_controller.rb @@ -4,6 +4,10 @@ class Api::V1::Accounts::Contacts::ConversationsController < Api::V1::Accounts:: conversations = Current.account.conversations.includes( :assignee, :contact, :inbox, :taggings ).where(contact_id: @contact.id) + # Optional inbox scoping so callers (e.g. the WhatsApp call button) can + # find the latest open conversation in a specific inbox even when the + # 20-row cap below would otherwise cut it off. + conversations = conversations.where(inbox_id: params[:inbox_id]) if params[:inbox_id].present? # Apply permission-based filtering using the existing service conversations = Conversations::PermissionFilterService.new( diff --git a/app/javascript/dashboard/api/contacts.js b/app/javascript/dashboard/api/contacts.js index dc79365e3..c39a4cf9d 100644 --- a/app/javascript/dashboard/api/contacts.js +++ b/app/javascript/dashboard/api/contacts.js @@ -35,8 +35,9 @@ class ContactAPI extends ApiClient { return axios.patch(`${this.url}/${id}?include_contact_inboxes=false`, data); } - getConversations(contactId) { - return axios.get(`${this.url}/${contactId}/conversations`); + getConversations(contactId, { inboxId } = {}) { + const params = inboxId ? { inbox_id: inboxId } : {}; + return axios.get(`${this.url}/${contactId}/conversations`, { params }); } getContactableInboxes(contactId) { diff --git a/app/javascript/dashboard/components-next/Contacts/VoiceCallButton.vue b/app/javascript/dashboard/components-next/Contacts/VoiceCallButton.vue index d338d2108..776d264eb 100644 --- a/app/javascript/dashboard/components-next/Contacts/VoiceCallButton.vue +++ b/app/javascript/dashboard/components-next/Contacts/VoiceCallButton.vue @@ -70,12 +70,17 @@ const whatsappCallSession = useWhatsappCallSession(); // Find the most recent open conversation for this contact in the picked inbox. // WhatsApp /initiate is conversation-scoped (unlike Twilio's contact-scoped path). +// Pass inboxId so the BE applies the filter before the 20-row cap — without it, +// contacts whose latest WhatsApp conversation falls outside the 20 most recent +// across all inboxes would be treated as having no conversation. const findWhatsappConversationId = async inboxId => { - const { data } = await ContactAPI.getConversations(props.contactId); + const { data } = await ContactAPI.getConversations(props.contactId, { + inboxId, + }); const conversations = data?.payload || []; - const match = conversations - .filter(c => c.inbox_id === inboxId) - .sort((a, b) => (b.last_activity_at || 0) - (a.last_activity_at || 0))[0]; + const match = [...conversations].sort( + (a, b) => (b.last_activity_at || 0) - (a.last_activity_at || 0) + )[0]; return match?.id || null; }; @@ -92,6 +97,10 @@ const startWhatsappCall = async (inboxId, conversationIdHint) => { const response = await whatsappCallSession.initiateOutboundCall(conversationId); + // The composable returns { status: 'locked' } when an init is already in + // flight or a call is already active; treat that as a soft no-op rather than + // claiming success. + if (response?.status === 'locked') return; if (!response?.id) { // Permission flow returns no id — banner already handled server-side; surface to user. useAlert(t('CONTACT_PANEL.CALL_INITIATED')); diff --git a/app/javascript/dashboard/components-next/message/bubbles/VoiceCall.vue b/app/javascript/dashboard/components-next/message/bubbles/VoiceCall.vue index fb5018362..f43149590 100644 --- a/app/javascript/dashboard/components-next/message/bubbles/VoiceCall.vue +++ b/app/javascript/dashboard/components-next/message/bubbles/VoiceCall.vue @@ -4,7 +4,7 @@ import { useI18n } from 'vue-i18n'; import { useStore } from 'vuex'; import { useMessageContext } from '../provider.js'; import { VOICE_CALL_STATUS } from '../constants'; -import { useCallSession } from 'dashboard/composables/useCallSession'; +import { useCallActions } from 'dashboard/composables/useCallSession'; import { formatDuration } from 'shared/helpers/timeHelper'; import Icon from 'dashboard/components-next/icon/Icon.vue'; @@ -40,8 +40,10 @@ const { currentUserId, inboxId, } = useMessageContext(); +// Lightweight consumer — bubble doesn't own global listeners; the +// FloatingCallWidget is the singleton root that drives the session. const { joinCall, endCall, activeCall, hasActiveCall, isJoining } = - useCallSession(); + useCallActions(); const status = computed(() => call.value?.status); const isOutbound = computed(() => call.value?.direction === 'outgoing'); diff --git a/app/javascript/dashboard/components/widgets/conversation/ConversationHeader.vue b/app/javascript/dashboard/components/widgets/conversation/ConversationHeader.vue index c989b4e28..cf8171385 100644 --- a/app/javascript/dashboard/components/widgets/conversation/ConversationHeader.vue +++ b/app/javascript/dashboard/components/widgets/conversation/ConversationHeader.vue @@ -121,6 +121,9 @@ const startWhatsappCall = async () => { currentChat.value.id ); + // Composable returns { status: 'locked' } when init is already in flight or + // a call is active; soft no-op so a parallel click doesn't trigger a banner. + if (response?.status === 'locked') return; // Permission template path returns no call id — show banner, no widget yet. if (!response?.id) { const status = response?.status; diff --git a/app/javascript/dashboard/composables/useCallSession.js b/app/javascript/dashboard/composables/useCallSession.js index f6dd3bb05..ab3dbdfb8 100644 --- a/app/javascript/dashboard/composables/useCallSession.js +++ b/app/javascript/dashboard/composables/useCallSession.js @@ -14,134 +14,117 @@ import { handleVoiceCallCreated } from 'dashboard/helper/voice'; import Timer from 'dashboard/helper/Timer'; const isWhatsappCall = call => call?.provider === 'whatsapp'; +// Calls seeded after a refresh / arriving via message.updated may lack provider +// metadata. Treat anything that has a callId (Meta's wacid) as WhatsApp — the +// Twilio path keys off provider_call_id (a CA…) and never sets callId. +const isWhatsappLikeCall = call => isWhatsappCall(call) || !!call?.callId; -export function useCallSession() { - const store = useStore(); - const callsStore = useCallsStore(); - const whatsappSession = useWhatsappCallSession(); - const { t } = useI18n(); - const isJoining = ref(false); - const callDuration = ref(0); - const durationTimer = new Timer(elapsed => { - callDuration.value = elapsed; +// Dismissed call sids must not be re-seeded by the conversation-load watcher. +// Lives at module scope so all consumers share the same set. +const dismissedCallSids = new Set(); +const markDismissed = callSid => { + if (callSid) dismissedCallSids.add(callSid); +}; + +// Globals attached once across all useCallSession() consumers — bubbles in a +// long thread call this composable many times, and a per-instance Timer + +// window listener stack would multiply work. +let globalsAttachedCount = 0; +let globalDurationTimer = null; +const globalCallDuration = ref(0); +let storedCallsStoreRef = null; +// Shared join lock so two surfaces (bubble + widget) clicking concurrently +// see one in-flight join, not two unrelated isJoining refs. +const globalIsJoining = ref(false); + +const handleBeforeUnloadGlobal = event => { + const store = storedCallsStoreRef; + if (!store) return; + if (!store.hasActiveCall && !store.hasIncomingCall) return; + event.preventDefault(); + event.returnValue = ''; +}; +const handlePageHideGlobal = () => sendWhatsappTerminateBeacon(); +const handleTwilioDisconnectedGlobal = () => + storedCallsStoreRef?.clearActiveCall(); + +const attachGlobalsOnFirstMount = callsStore => { + globalsAttachedCount += 1; + if (globalsAttachedCount > 1) return; + storedCallsStoreRef = callsStore; + globalDurationTimer = new Timer(elapsed => { + globalCallDuration.value = elapsed; }); - - const activeCall = computed(() => callsStore.activeCall); - const incomingCalls = computed(() => callsStore.incomingCalls); - const hasActiveCall = computed(() => callsStore.hasActiveCall); - const hasIncomingCall = computed(() => callsStore.hasIncomingCall); - - watch( - hasActiveCall, - active => { - if (active) { - durationTimer.start(); - } else { - durationTimer.stop(); - callDuration.value = 0; - } - }, - { immediate: true } + TwilioVoiceClient.addEventListener( + 'call:disconnected', + handleTwilioDisconnectedGlobal ); + window.addEventListener('beforeunload', handleBeforeUnloadGlobal); + window.addEventListener('pagehide', handlePageHideGlobal); +}; - // Warn before a refresh/close drops a live or ringing call. Cable events - // aren't replayed on reconnect, so a confirmed refresh during ringing would - // leave the agent unable to accept; for active calls the WebRTC session - // dies outright (no rejoin path). - const handleBeforeUnload = event => { - if (!hasActiveCall.value && !hasIncomingCall.value) return; - event.preventDefault(); - event.returnValue = ''; - }; - - // Cable broadcasts (voice_call.incoming / message.created) are one-shot, so - // on a hard refresh they leave the calls store empty. Seed it from any - // ringing voice_call message in the conversation cache. - const seedCallsFromHydratedMessages = () => { - const conversations = store.getters.getAllConversations || []; - const currentUserId = store.getters.getCurrentUserID; - conversations.forEach(conv => { - (conv.messages || []).forEach(msg => { - if (msg.content_type !== 'voice_call') return; - if (msg.call?.status !== 'ringing') return; - handleVoiceCallCreated(msg, currentUserId); - }); - }); - }; - - // Terminate only the active call — ringing calls stay alive on Meta so the - // agent can pick them up after reload (seeded back via the watcher above). - const handlePageHide = () => { - sendWhatsappTerminateBeacon(); - }; - - const handleTwilioDisconnected = () => callsStore.clearActiveCall(); - - onMounted(() => { - TwilioVoiceClient.addEventListener( - 'call:disconnected', - handleTwilioDisconnected - ); - window.addEventListener('beforeunload', handleBeforeUnload); - window.addEventListener('pagehide', handlePageHide); - seedCallsFromHydratedMessages(); - }); - - // Re-seed when conversations stream in after mount; addCall merges by callSid. - watch( - () => store.getters.getAllConversations?.length, - () => seedCallsFromHydratedMessages() +const detachGlobalsOnLastUnmount = () => { + globalsAttachedCount -= 1; + if (globalsAttachedCount > 0) return; + globalDurationTimer?.stop(); + globalDurationTimer = null; + globalCallDuration.value = 0; + storedCallsStoreRef = null; + TwilioVoiceClient.removeEventListener( + 'call:disconnected', + handleTwilioDisconnectedGlobal ); + window.removeEventListener('beforeunload', handleBeforeUnloadGlobal); + window.removeEventListener('pagehide', handlePageHideGlobal); +}; - onUnmounted(() => { - durationTimer.stop(); - TwilioVoiceClient.removeEventListener( - 'call:disconnected', - handleTwilioDisconnected - ); - window.removeEventListener('beforeunload', handleBeforeUnload); - window.removeEventListener('pagehide', handlePageHide); - }); - +// Build the action surface used by both the root session composable and the +// lighter useCallActions consumer. All state is module-scoped — the actions +// don't depend on per-instance refs, so they're cheap to call from anywhere. +const buildCallActions = ({ callsStore, whatsappSession, t }) => { const findCall = callSid => callsStore.calls.find(c => c.callSid === callSid); const endCall = async ({ conversationId, inboxId, callSid }) => { const call = findCall(callSid); - if (isWhatsappCall(call)) { + if (isWhatsappLikeCall(call)) { // Pass call.callId so a wiped module state (e.g. a prior accept attempt // tore down the WebRTC session) doesn't stop us hitting /terminate. - await whatsappSession.endActiveCall(call.callId); - durationTimer.stop(); + await whatsappSession.endActiveCall(call?.callId); + globalDurationTimer?.stop(); callsStore.clearActiveCall(); return; } await VoiceAPI.leaveConference({ inboxId, conversationId, callSid }); TwilioVoiceClient.endClientCall(); - durationTimer.stop(); + globalDurationTimer?.stop(); callsStore.clearActiveCall(); }; const joinCall = async ({ conversationId, inboxId, callSid }) => { - if (isJoining.value) return null; + if (globalIsJoining.value) return null; const call = findCall(callSid); - // Outbound calls were initiated by this agent — there is no inbound offer - // to accept and the WebRTC session is already mid-handshake. Routing - // through acceptIncomingCall would call prepareInboundAnswer → cleanup() - // and destroy the live outbound session, then 409 from the backend. - if (call?.callDirection === 'outbound') return null; + // Outbound *WhatsApp* calls have no separate join step — the offer was + // sent at initiate time and the answer is applied by the cable handler. + // Routing through acceptIncomingCall here would call prepareInboundAnswer → + // cleanup() and destroy the live outbound session. Outbound *Twilio* + // calls still need joinConference + joinClientCall (FloatingCallWidget + // auto-joins them), so don't short-circuit those. + if (call?.callDirection === 'outbound' && isWhatsappLikeCall(call)) { + return null; + } - isJoining.value = true; + globalIsJoining.value = true; try { - if (isWhatsappCall(call)) { + if (isWhatsappLikeCall(call)) { await whatsappSession.acceptIncomingCall({ callId: call.callId, sdpOffer: call.sdpOffer, iceServers: call.iceServers, }); callsStore.setCallActive(callSid); - durationTimer.start(); + globalDurationTimer?.start(); return { callId: call.callId }; } @@ -161,13 +144,14 @@ export function useCallSession() { }); callsStore.setCallActive(callSid); - durationTimer.start(); + globalDurationTimer?.start(); return { conferenceSid: joinResponse?.conference_sid }; } catch (error) { useAlert(error?.response?.data?.error || t('CONTACT_PANEL.CALL_FAILED')); if (error?.response?.status === 409) { TwilioVoiceClient.endClientCall(); + markDismissed(callSid); callsStore.dismissCall(callSid); } // eslint-disable-next-line no-console @@ -176,46 +160,134 @@ export function useCallSession() { cleanupWhatsappSession(); return null; } finally { - isJoining.value = false; + globalIsJoining.value = false; } }; - const rejectIncomingCall = callSid => { + // Await provider-side reject before dismissing the local entry; if the API + // call fails the call should stay surfaced so the agent can retry instead of + // disappearing while the backend still rings. + const rejectIncomingCall = async callSid => { const call = findCall(callSid); - if (isWhatsappCall(call) && call?.callId) { - // Outbound calls that are still ringing must be terminated, not rejected - // (reject is the inbound-side verb on Meta's API). Pass call.callId so - // a wiped module state still hits /terminate. - if (call.callDirection === 'outbound') { - whatsappSession.endActiveCall(call.callId); + try { + if (isWhatsappLikeCall(call) && call?.callId) { + if (call.callDirection === 'outbound') { + // Outbound calls that are still ringing must be terminated, not + // rejected (reject is the inbound-side verb on Meta's API). + await whatsappSession.endActiveCall(call.callId); + } else { + await whatsappSession.rejectIncomingCall(call.callId); + } } else { - whatsappSession.rejectIncomingCall(call.callId); + TwilioVoiceClient.endClientCall(); } - } else { - TwilioVoiceClient.endClientCall(); + } finally { + markDismissed(callSid); + callsStore.dismissCall(callSid); } - callsStore.dismissCall(callSid); }; const dismissCall = callSid => { + markDismissed(callSid); callsStore.dismissCall(callSid); }; + return { endCall, joinCall, rejectIncomingCall, dismissCall }; +}; + +const buildReactiveSurface = callsStore => { + const activeCall = computed(() => callsStore.activeCall); + const incomingCalls = computed(() => callsStore.incomingCalls); + const hasActiveCall = computed(() => callsStore.hasActiveCall); const formattedCallDuration = computed(() => { - const minutes = Math.floor(callDuration.value / 60); - const seconds = callDuration.value % 60; + const total = globalCallDuration.value; + const minutes = Math.floor(total / 60); + const seconds = total % 60; return `${minutes.toString().padStart(2, '0')}:${seconds.toString().padStart(2, '0')}`; }); - return { activeCall, incomingCalls, hasActiveCall, - isJoining, + isJoining: globalIsJoining, formattedCallDuration, - joinCall, - endCall, - rejectIncomingCall, - dismissCall, }; +}; + +// Root-mount composable. Call once at the dashboard root (FloatingCallWidget +// is the natural anchor — always mounted, lifetime spans the whole session). +// This is the only path that registers global window/Twilio listeners and +// owns the duration Timer. +export function useCallSession() { + const store = useStore(); + const callsStore = useCallsStore(); + const whatsappSession = useWhatsappCallSession(); + const { t } = useI18n(); + + const reactive = buildReactiveSurface(callsStore); + + // Cable broadcasts (voice_call.incoming / message.created) are one-shot, so + // on a hard refresh they leave the calls store empty. Seed it from any + // ringing voice_call message in the conversation cache. Skip calls the + // agent has already dismissed locally so they don't re-pop on the next + // conversation update. + const seedCallsFromHydratedMessages = () => { + const conversations = store.getters.getAllConversations || []; + const currentUserId = store.getters.getCurrentUserID; + conversations.forEach(conv => { + (conv.messages || []).forEach(msg => { + if (msg.content_type !== 'voice_call') return; + if (msg.call?.status !== 'ringing') return; + const callSid = msg.call?.provider_call_id; + if (callSid && dismissedCallSids.has(callSid)) return; + handleVoiceCallCreated(msg, currentUserId); + }); + }); + }; + + watch( + reactive.hasActiveCall, + active => { + if (active) { + globalDurationTimer?.start(); + } else { + globalDurationTimer?.stop(); + globalCallDuration.value = 0; + } + }, + { immediate: true } + ); + + onMounted(() => { + attachGlobalsOnFirstMount(callsStore); + seedCallsFromHydratedMessages(); + }); + + // Re-seed when conversations stream in after mount; addCall merges by callSid + // and dismissed sids are filtered, so this is idempotent. + watch( + () => store.getters.getAllConversations?.length, + () => seedCallsFromHydratedMessages() + ); + + onUnmounted(() => detachGlobalsOnLastUnmount()); + + const actions = buildCallActions({ callsStore, whatsappSession, t }); + + return { ...reactive, ...actions }; +} + +// Lightweight consumer for components that need to read state and trigger +// actions but should NOT mount global listeners (e.g., per-message bubbles +// rendered in a thread). Reads from the same module-level state that +// useCallSession owns, so the duration timer and dismissed set stay coherent. +export function useCallActions() { + const callsStore = useCallsStore(); + const whatsappSession = useWhatsappCallSession(); + const { t } = useI18n(); + + const reactive = buildReactiveSurface(callsStore); + const actions = buildCallActions({ callsStore, whatsappSession, t }); + + return { ...reactive, ...actions }; } diff --git a/app/javascript/dashboard/composables/useWhatsappCallSession.js b/app/javascript/dashboard/composables/useWhatsappCallSession.js index 455451703..c8e1e1a75 100644 --- a/app/javascript/dashboard/composables/useWhatsappCallSession.js +++ b/app/javascript/dashboard/composables/useWhatsappCallSession.js @@ -12,7 +12,12 @@ let mediaRecorder = null; let recorderChunks = []; let audioContext = null; let activeCallId = null; -let intentionallyClosing = false; +// Module-scoped so multiple composable callers (header button + contact-panel +// button) share the same lock. A per-instance ref let two parallel callers +// both pass the guard and tear down each other's WebRTC state in cleanup(). +// ref() so consumers can reactively gate buttons on it (the composable +// re-exports this as `isInitiating`). +const isInitiatingOutbound = ref(false); // Inbound calls record from the moment the agent clicks accept (their click = // pickup). Outbound calls must wait — Meta's `connect` webhook (which lands // during ringing) negotiates remote tracks ~20s before the contact actually @@ -122,7 +127,6 @@ const cleanup = () => { recorderChunks = []; audioContext = null; activeCallId = null; - intentionallyClosing = false; recorderArmed = false; }; @@ -216,9 +220,12 @@ const beaconTerminate = callId => { } }; -export function useWhatsappCallSession() { - const isInitiating = ref(false); +export const hasActiveWhatsappCall = () => !!(activeCallId || pc); +export const isLocalWhatsappCall = callId => + !!callId && activeCallId != null && callId === activeCallId; + +export function useWhatsappCallSession() { const prepareInboundAnswer = async (sdpOffer, iceServers) => { cleanup(); localStream = await navigator.mediaDevices.getUserMedia({ audio: true }); @@ -275,7 +282,6 @@ export function useWhatsappCallSession() { }; const rejectIncomingCall = async callId => { - intentionallyClosing = true; try { await WhatsappCallsAPI.reject(callId); } finally { @@ -284,16 +290,27 @@ export function useWhatsappCallSession() { }; const initiateOutboundCall = async conversationId => { - if (isInitiating.value) return null; - isInitiating.value = true; + // Module-scoped lock + active-session guard so a second click — from the + // same composable instance OR a different one (header vs contact panel) + // OR while a call is already live — can't tear down the in-flight setup + // via prepareOutboundOffer's cleanup(). + if (isInitiatingOutbound.value) return { status: 'locked' }; + if (hasActiveWhatsappCall()) return { status: 'locked' }; + isInitiatingOutbound.value = true; try { const sdpOffer = await prepareOutboundOffer(); const response = await WhatsappCallsAPI.initiate( conversationId, sdpOffer ); - // The permission-request branch returns no call id; let the caller render the banner. - activeCallId = response?.id || null; + if (response?.id) { + activeCallId = response.id; + return response; + } + // No call id back: this is the permission-request branch. The mic + + // PeerConnection allocated by prepareOutboundOffer aren't useful until + // the contact opts in and the agent retries — release them. + cleanup(); return response; } catch (e) { cleanup(); @@ -309,7 +326,7 @@ export function useWhatsappCallSession() { } throw e; } finally { - isInitiating.value = false; + isInitiatingOutbound.value = false; } }; @@ -323,7 +340,6 @@ export function useWhatsappCallSession() { cleanup(); return; } - intentionallyClosing = true; try { await stopRecorderAndUpload(callId); await WhatsappCallsAPI.terminate(callId).catch(() => {}); @@ -333,7 +349,7 @@ export function useWhatsappCallSession() { }; return { - isInitiating, + isInitiating: isInitiatingOutbound, prepareInboundAnswer, prepareOutboundOffer, acceptIncomingCall, @@ -348,6 +364,9 @@ export function useWhatsappCallSession() { export const applyOutboundAnswer = async (callId, sdpAnswer) => { if (!pc) return; + // voice_call.outbound_connected is broadcast account-wide; only apply + // the SDP answer if it's for this tab's in-flight outbound call. + if (activeCallId != null && callId !== activeCallId) return; activeCallId = callId; await pc.setRemoteDescription({ type: 'answer', sdp: sdpAnswer }); }; @@ -386,6 +405,10 @@ export const setWhatsappCallMuted = muted => { }; export const sendWhatsappTerminateBeacon = () => { - if (!activeCallId || intentionallyClosing) return; + // Always fire when there's a live callId. The beacon endpoint is idempotent, + // so racing it with an in-flight Axios terminate (which unload may abort) is + // fine — Meta gets exactly one terminate either way, and we avoid leaving + // the call ringing on Meta until its carrier-side timeout. + if (!activeCallId) return; beaconTerminate(activeCallId); }; diff --git a/app/javascript/dashboard/helper/actionCable.js b/app/javascript/dashboard/helper/actionCable.js index 14afeec5c..e53a30053 100644 --- a/app/javascript/dashboard/helper/actionCable.js +++ b/app/javascript/dashboard/helper/actionCable.js @@ -9,6 +9,7 @@ import { applyOutboundAnswer, armOutboundRecorder, handleWhatsappRemoteEnd, + isLocalWhatsappCall, } from 'dashboard/composables/useWhatsappCallSession'; const { isImpersonating } = useImpersonation(); @@ -238,6 +239,10 @@ class ActionCableConnector extends BaseActionCableConnector { // eslint-disable-next-line class-methods-use-this onVoiceCallOutboundConnected = async data => { if (data?.provider !== 'whatsapp' || !data.sdp_answer) return; + // Account-wide broadcast: skip SDPs for calls another agent is handling, + // otherwise applyOutboundAnswer would feed a foreign SDP into this tab's + // peer connection. + if (!isLocalWhatsappCall(data.id)) return; try { await applyOutboundAnswer(data.id, data.sdp_answer); } catch (_) { @@ -259,12 +264,18 @@ class ActionCableConnector extends BaseActionCableConnector { // eslint-disable-next-line class-methods-use-this onVoiceCallEnded = async data => { if (data?.provider !== 'whatsapp') return; - // Await upload before removeCall — the store's sync teardown would otherwise - // wipe the recorder chunks before they reach the server. - try { - await handleWhatsappRemoteEnd(data.id); - } catch (_) { - /* noop */ + // The store entry should always be removed for this account-wide broadcast, + // but the WebRTC/recorder teardown must only run for the call this tab owns + // — otherwise an unrelated agent's call ending would stop this tab's + // recorder and upload its chunks against the wrong call id. + if (isLocalWhatsappCall(data.id)) { + // Await upload before removeCall — the store's sync teardown would otherwise + // wipe the recorder chunks before they reach the server. + try { + await handleWhatsappRemoteEnd(data.id); + } catch (_) { + /* noop */ + } } useCallsStore().removeCall(data.call_id); }; diff --git a/app/javascript/dashboard/helper/voice.js b/app/javascript/dashboard/helper/voice.js index 3418b161c..f479267ea 100644 --- a/app/javascript/dashboard/helper/voice.js +++ b/app/javascript/dashboard/helper/voice.js @@ -142,6 +142,20 @@ export function syncConversationCallVisibility(conversation, currentUserId) { const assigneeId = extractAssigneeId(conversation); if (!isAssignedToAnotherAgent(assigneeId, currentUserId)) return; + // Outbound calls belong to the initiator regardless of who the conversation + // is currently assigned to (auto-assignment may flip mid-call). Mirror + // shouldShowCall's outbound exception so an in-progress outbound call isn't + // ripped out from under the caller when the conversation reassigns. const callsStore = useCallsStore(); - callsStore.removeCallsForConversation(conversation.id); + const callsToRemove = callsStore.calls.filter( + call => + call.conversationId === conversation.id && + !shouldShowCall({ + callDirection: call.callDirection, + senderId: call.senderId, + assigneeId, + currentUserId, + }) + ); + callsToRemove.forEach(call => callsStore.removeCall(call.callSid)); } diff --git a/app/javascript/dashboard/stores/calls.js b/app/javascript/dashboard/stores/calls.js index 48f8015cf..1c923e96f 100644 --- a/app/javascript/dashboard/stores/calls.js +++ b/app/javascript/dashboard/stores/calls.js @@ -84,9 +84,10 @@ export const useCallsStore = defineStore('calls', { call => call.conversationId === conversationId ); - if (callsToRemove.some(call => call.isActive)) { - TwilioVoiceClient.endClientCall(); - } + // Tear down each active call via its own provider so a WhatsApp call + // gets cleanupWhatsappSession() (closes pc, stops recorder/mic) instead + // of the Twilio-only endClientCall() — otherwise mic stays open. + callsToRemove.filter(call => call.isActive).forEach(teardownByProvider); this.calls = this.calls.filter( call => call.conversationId !== conversationId diff --git a/db/migrate/20260502090000_cleanup_legacy_channel_voice_inboxes.rb b/db/migrate/20260502090000_cleanup_legacy_channel_voice_inboxes.rb new file mode 100644 index 000000000..f8957a550 --- /dev/null +++ b/db/migrate/20260502090000_cleanup_legacy_channel_voice_inboxes.rb @@ -0,0 +1,53 @@ +class CleanupLegacyChannelVoiceInboxes < ActiveRecord::Migration[7.1] + # Inboxes whose channel_type column still says 'Channel::Voice' became + # orphans after 20260326120001_drop_channel_voice.rb dropped both the + # channel_voice table and the model class. The polymorphic + # `belongs_to :channel` lookup on those rows fails to constantize + # `Channel::Voice`, crashing the inbox serializer with + # `uninitialized constant Channel::Voice`. + # + # Delete the orphan inboxes and their dependents via raw SQL so we + # bypass the polymorphic load that would crash inside Rails callbacks. + def up + legacy_ids = ActiveRecord::Base.connection + .exec_query("SELECT id FROM inboxes WHERE channel_type = 'Channel::Voice'") + .rows.flatten + + return if legacy_ids.empty? + + say_with_time "Cleaning up #{legacy_ids.size} legacy Channel::Voice inbox(es): #{legacy_ids.inspect}" do + delete_dependents(legacy_ids) + execute("DELETE FROM inboxes WHERE id IN (#{legacy_ids.join(',')})") + end + end + + def down + raise ActiveRecord::IrreversibleMigration + end + + private + + # Tables with an inbox_id FK that need clearing before the inbox row is removed. + # Order matters where one table FKs another (messages → conversations). + DEPENDENT_TABLES = %w[ + messages + conversations + contact_inboxes + inbox_members + agent_bot_inboxes + campaigns + webhooks + integrations_hooks + inbox_assignment_policies + ].freeze + + def delete_dependents(inbox_ids) + in_clause = inbox_ids.join(',') + DEPENDENT_TABLES.each do |table| + next unless ActiveRecord::Base.connection.table_exists?(table) + next unless ActiveRecord::Base.connection.column_exists?(table, :inbox_id) + + execute("DELETE FROM #{table} WHERE inbox_id IN (#{in_clause})") + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 9ce734ba7..240ef77f0 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.1].define(version: 2026_04_30_114500) do +ActiveRecord::Schema[7.1].define(version: 2026_05_02_090000) do # These extensions should be enabled to support this database enable_extension "pg_stat_statements" enable_extension "pg_trgm" diff --git a/enterprise/app/controllers/twilio/voice_controller.rb b/enterprise/app/controllers/twilio/voice_controller.rb index eb29c00bd..40da0b323 100644 --- a/enterprise/app/controllers/twilio/voice_controller.rb +++ b/enterprise/app/controllers/twilio/voice_controller.rb @@ -7,6 +7,12 @@ class Twilio::VoiceController < ApplicationController }.freeze before_action :set_inbox! + # Twilio's recording webhook fetches the audio file with the channel's + # auth credentials, so accepting an unsigned POST would let an attacker + # who guesses (or is leaked) a ConferenceSid coerce credential-bearing + # requests to an arbitrary host. Verify the signature before doing + # anything with the payload. + before_action :verify_twilio_signature!, only: :recording_status def status Voice::StatusUpdateService.new( @@ -186,6 +192,18 @@ class Twilio::VoiceController < ApplicationController call.update!(twilio_conference_sid: sid) end + def verify_twilio_signature! + signature = request.headers['X-Twilio-Signature'].to_s + auth_token = inbox_channel.auth_token.to_s + return head :forbidden if signature.blank? || auth_token.blank? + + validator = Twilio::Security::RequestValidator.new(auth_token) + payload = request.request_method.to_s.upcase == 'POST' ? request.request_parameters : request.query_parameters + return if validator.validate(request.original_url, payload, signature) + + head :forbidden + end + def set_inbox! digits = params[:phone].to_s.gsub(/\D/, '') phone_number = "+#{digits}" diff --git a/enterprise/app/services/voice/provider/twilio/conference_service.rb b/enterprise/app/services/voice/provider/twilio/conference_service.rb index 0ee368d00..45b15d0c6 100644 --- a/enterprise/app/services/voice/provider/twilio/conference_service.rb +++ b/enterprise/app/services/voice/provider/twilio/conference_service.rb @@ -8,8 +8,14 @@ class Voice::Provider::Twilio::ConferenceService call.conference_sid end + # Surface the 409 collision to a second agent who clicks accept, but DON'T + # claim accepted_by_agent here — the actual claim happens when Twilio's + # participant-join webhook fires for this agent's leg (Voice::Conference::Manager). + # If we claimed up-front and the browser's joinClientCall failed (device init + # error, tab close, network drop), the call would stay ringing-but-claimed + # and every other agent would 409 with no recovery path. def mark_agent_joined(user:) - claim_call!(user) + raise_already_accepted!(call.accepted_by_agent) if claimed_by_other_agent?(user) assign_conversation!(user) end @@ -25,13 +31,6 @@ class Voice::Provider::Twilio::ConferenceService private - def claim_call!(user) - call.with_lock do - raise_already_accepted!(call.accepted_by_agent) if claimed_by_other_agent?(user) - call.update!(accepted_by_agent: user) if call.accepted_by_agent_id != user.id - end - end - def claimed_by_other_agent?(user) call.accepted_by_agent_id.present? && call.accepted_by_agent_id != user.id end diff --git a/spec/enterprise/services/voice/provider/twilio/conference_service_spec.rb b/spec/enterprise/services/voice/provider/twilio/conference_service_spec.rb index 519519eed..71902f146 100644 --- a/spec/enterprise/services/voice/provider/twilio/conference_service_spec.rb +++ b/spec/enterprise/services/voice/provider/twilio/conference_service_spec.rb @@ -36,12 +36,23 @@ describe Voice::Provider::Twilio::ConferenceService do end describe '#mark_agent_joined' do - it 'sets accepted_by_agent on the Call' do + it 'assigns the conversation to the agent without claiming the call (claim defers to participant-join webhook)' do agent = create(:user, account: account) + create(:inbox_member, inbox: channel.inbox, user: agent) service.mark_agent_joined(user: agent) - expect(call.reload.accepted_by_agent_id).to eq(agent.id) + expect(call.reload.accepted_by_agent_id).to be_nil + expect(conversation.reload.assignee_id).to eq(agent.id) + end + + it 'raises CallAlreadyAccepted when another agent has already claimed the call' do + first_agent = create(:user, account: account) + second_agent = create(:user, account: account) + call.update!(accepted_by_agent: first_agent) + + expect { service.mark_agent_joined(user: second_agent) } + .to raise_error(CustomExceptions::CallAlreadyAccepted) end end