Skip to content

fix(voice): borrow upstream call fixes into the CE voice stack and restore the Calls page - #52

Merged
YJack0000 merged 7 commits into
developfrom
fix/voice-borrow
Sep 13, 2026
Merged

fix(voice): borrow upstream call fixes into the CE voice stack and restore the Calls page#52
YJack0000 merged 7 commits into
developfrom
fix/voice-borrow

Conversation

@YJack0000

Copy link
Copy Markdown

Four behaviours upstream fixed for their Twilio / WhatsApp calling are re-implemented for the fork's Pathors-based CE voice, and the Calls page, which has pointed at an endpoint that did not exist since the EE voice stack was removed, works again.

Refs pathorsAI/pathors#2780 · proposal: https://claude.ai/code/artifact/26679670-e46d-47d9-8eec-54433e0e98ee (section 06)

How to test

  1. Merge two contacts where the mergee has calls: the calls now belong to the surviving contact instead of pointing at a deleted one.
  2. Let Pathors post a transcript after a call has ended (PATCH …/pathors/calls/:id with transcript): the voice bubble shows the transcript without a page reload.
  3. Record a long voice note (several minutes) in the composer: the remuxer no longer risks a stack overflow on MediaRecorder's unknown-size clusters (regression spec covers 100 000 clusters).
  4. Open the same live Pathors call in two tabs, join from one: the other tab's Join button disappears; the agent who joined can leave and rejoin.
  5. Sidebar → Calls (visible whenever the voice channel feature is on): the history list loads with contact, inbox, agent, status and direction filters; a call whose contact was deleted shows the "deleted contact" label.

What changed

  • ContactMergeAction#merge_calls moves calls to the base contact; Contact has_many :calls, dependent: :destroy_async.
  • Pathors calls webhook permits and persists transcript through the same post-terminal path as recording_url.
  • webmOpusToOgg: the parser fix itself already arrived with the upstream sync; this adds the missing regression spec.
  • VoiceCall.vue: Join is gated on acceptedByAgentId being empty or the current user (the field is the persisted "handled by" attribution, so it is compared, not cleared).
  • New CE Api::V1::Accounts::CallsController#index + CallFinder (admins see the account, agents see calls in conversations they can open), _call.json.jbuilder, route outside every enterprise guard; Sidebar and calls/routes.js gate the page on channel_voice instead of Cloud/Enterprise. zh_TW string for the deleted-contact label.

No migrations.

Contact merge reassigned conversations, messages, notes and contact
inboxes but left calls pointing at the contact that is about to be
destroyed. calls.contact_id has no foreign key, so those rows survived
as orphans and the call list rendered them without a contact.

Move the mergee's calls onto the base contact inside the same
transaction, and declare has_many :calls on Contact so a plain destroy
cleans them up the way it already does for every other child record.
The calls table, Call#push_event_data and the VoiceCall bubble all
already carry a transcript, but the webhook controller never let one
through: update_params did not permit :transcript and neither of the
apply paths wrote it, so the field stayed nil for every call.

Permit it and treat it like recording_url — a post-call artifact that
necessarily arrives after the terminal status, and therefore one of the
fields a stale-status webhook may still carry. persist already touches
the linked message, so the bubble re-renders with the transcribed text
without any frontend change.
MediaRecorder streams every Cluster with an unknown size, and the EBML
walker descends into those inline rather than recursing so a long
recording does not cost one stack frame per cluster. Nothing guarded
that: reinstating the recursive descent still produced a byte-identical
file for short inputs.

Build a WebM the way MediaRecorder shapes one — an unknown-size Segment
holding a long run of unknown-size Clusters — and assert every frame
reaches the OGG output, which overflows the stack without the inline
descent.
The join endpoint records accepted_by_agent_id and touches the linked
message, so every dashboard already receives who answered. The bubble
ignored it and kept offering Join, sending late clickers into a 409 from
the Pathors relay.

Gate the button on the field instead of clearing it on leave. Nothing
clears accepted_by_agent_id today, and nothing should: it is the
persisted attribution the bubble's "Handled by" line and the calls list's
"Picked by" column read, and leave is a purely client-side LiveKit
disconnect with no request a crashed or closed tab could be relied on to
send. Comparing the field against the current user keeps the call closed
to everyone else while letting the agent who left rejoin their own call.
The sidebar's Calls page fetches GET /api/v1/accounts/:id/calls, but
that route only ever existed behind the enterprise guard, so the page
404d on every load in this fork.

Add the community endpoint: a CallFinder that scopes by account,
restricts non-administrators to the conversations they can open (the
same bar the Pathors join endpoint enforces), applies the status,
direction, inbox and agent filters the filter bar sends, and paginates
25 to a page. Call gains the inverse of its display mappings so the
dashboard's inbound/outbound and dashed statuses round-trip.

The serializer keeps contact and agent nullable — an AI-handled call has
no agent, and a contact deleted after the call would otherwise drop its
history — and zh_TW picks up the deleted-contact fallback string the
list already renders.
The Calls entry and route were gated on Chatwoot Cloud / Enterprise because
the history endpoint only existed there. The fork now serves it from its own
CE controller, so the only gate left is the channel_voice feature flag.
@claude

claude Bot commented Sep 12, 2026

Copy link
Copy Markdown

Review

Solid PR overall — the new endpoint reuses Conversations::PermissionFilterService for agent visibility, the create-race and stale-status webhook semantics are preserved, the display↔storage mappings are tidy inverses, and the test coverage (admin vs. agent visibility, filters, deleted contact, late transcript, 100k-cluster remuxer regression) is thoughtful. Findings below, most important first.

1. The enterprise merge_calls override now shadows the new OSS one

enterprise/app/actions/enterprise/contact_merge_action.rb still defines merge_calls, and ContactMergeAction.prepend_mod_with means it wins whenever the enterprise overlay loads — which is the default here since enterprise/ exists in the repo. In that configuration the new OSS merge_calls (app/actions/contact_merge_action.rb:50) is dead code, and the two implementations now drift (update_all vs. .update, which instantiates each call and runs validations/callbacks). Per the CLAUDE.md drift checklist, delete the enterprise override (it contains only this method) so there is a single implementation.

2. _call.json.jbuilder guards a deleted contact but not a deleted conversation or inbox

app/views/api/v1/models/_call.json.jbuilder renders call.conversation.display_id, call.inbox.name, and call.inbox.channel.try(:medium) unguarded. The calls table has no DB foreign keys, and the call-cleanup associations for Conversation/Inbox/Account exist only in the enterprise concerns (enterprise/app/models/enterprise/concerns/conversation.rb, inbox.rb, account.rb) — this PR ported only the Contact one to OSS.

Consequences:

  • In a CE deploy (enterprise disabled — the premise of this PR), deleting a conversation or an inbox permanently orphans its calls, and a single orphan 500s the entire Calls index for administrators (agents are shielded because the visibility subquery inner-joins live conversations).
  • Even with the enterprise overlay loaded, dependent: :destroy_async leaves a window after an inbox/conversation delete where the page 500s until the cleanup job runs — the exact window the contact nil-guard was added for.

Suggest mirroring the Contact change (add has_many :calls, dependent: :destroy_async to Conversation and Inbox in OSS, watching for double-definition with the enterprise concerns) and/or nil-guarding conversation/inbox in the partial the same way contact is. Note the "renders a call whose contact was deleted" spec passes only because the destroy_async job does not run inline in tests — the same spec written against conversation.destroy! would fail today.

3. The new transcript spec does not actually exercise apply_post_call_artifacts

In stale_status?, status.present? is required — so the new spec ("accepts a late transcript written back after the call completed"), which PATCHes only transcript with no status, goes through apply_update, not the stale path. The transcript half of apply_post_call_artifacts is untested. Add a case that sends a regressing status plus transcript to a completed call (mirroring the existing "keeps the recording_url from a webhook whose status would regress" spec).

4. CallFinder mixes injected state with ambient Current

The finder takes current_user/current_account in the constructor but reads Current.account_user&.administrator? for the admin check — the only finder in the codebase doing this, and it breaks the finder outside a request cycle. It is also redundant: Conversations::PermissionFilterService#perform already returns the scope unfiltered for administrators (it looks up the AccountUser itself). Either drop the early return entirely, or derive the role from the injected pair (AccountUser.find_by(account_id:, user_id:)).

5. Minor / nits

  • Double COUNT per request: the finder runs @calls.count, then the jbuilder's @calls.total_pages makes Kaminari run its own identical count. Use total_count/total_pages off the paginated relation for the meta block instead of a separate count.
  • Avatar N+1: contact.avatar_url / agent avatar_url each hit Active Storage per row (up to 50 extra queries per page of 25). Preloading the avatar_attachment: :blob association for contact and agent would remove it — fine to defer if the page is fast enough.
  • Join gate never reopens for other agents (VoiceCall.vue): since accepted_by_agent_id is never cleared, once agent A joins, agent B never sees Join again even if A leaves mid-call and goes offline. The Pathors backend already arbitrates the race (409), so the UI gate is stricter than the server. The comment says this is deliberate — just confirm you are OK with there being no takeover path for an abandoned live call.
  • zh_TW locale edit: CLAUDE.md says only en.yml/en.json change in PRs, with other locales flowing through Crowdin. If zh_TW is hand-maintained in this fork that is fine; otherwise the next Crowdin sync will overwrite this string.

Upstream-merge risk

Low. Most of the diff is new fork-local files. The shared-file touches (routes.rb, contact.rb, Sidebar.vue, calls/routes.js, .rubocop.yml) are one-liners; the sidebar/route gating change (installationTypes → featureFlag) is the most likely future conflict and is well-commented. The merge_calls body replacement will conflict if upstream ever changes that method — resolving finding 1 (single implementation in OSS) keeps that surface minimal.

(Could not run the test suites in this review environment; findings are from static inspection of the PR merge ref.)

@YJack0000

Copy link
Copy Markdown
Author

@yui0303 AI check 沒問題(rubocop / rspec 118 / eslint / vitest),可以 review/merge。無 migration。跟 voice 相關:Join 按鈕改成比對 acceptedByAgentId(不清欄位,讓同一個人離開後能再加入),Calls 頁入口改依 channel_voice 旗標。

@YJack0000
YJack0000 merged commit c18f7d5 into develop Sep 13, 2026
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants