-
Notifications
You must be signed in to change notification settings - Fork 12.9k
regression: "Voice call" action enabled when a call is in progress #37967
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughFlows a MediaCallState (alias for MediaCallExternalState) through the ui-voip context into call-history and contextual-bar UIs; adds isCallingBlocked(state) and uses it to disable voice-call actions and show a "Call in progress" tooltip when calls are not allowed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Context as MediaCallContext
participant Component as CallHistory / ContextualBar
participant Actions as getItems
participant UI as Rendered UI
rect rgba(200,230,201,0.4)
Context->>Component: provide current MediaCallState
end
Component->>Actions: getItems(state)
Actions->>Actions: evaluate isCallingBlocked(state)
alt not blocked (state == "new" or "closed")
Actions->>UI: return enabled voiceCall item
else blocked (unauthorized / unlicensed / ongoing)
Actions->>UI: return disabled voiceCall item with tooltip "Call in progress"
end
UI->>User: render button/menu with disabled/tooltip
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/meteor/client/views/mediaCallHistory/CallHistoryRowInternalUser.tsx (1)
41-55: Consider extracting shared action mapping logic.The
getItemsfunction and the logic for computingdisabledstate andtooltipare duplicated betweenCallHistoryRowInternalUser.tsxandCallHistoryRowExternalUser.tsx. Consider extracting this into a shared utility function to reduce duplication and ensure consistency.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
apps/meteor/client/views/mediaCallHistory/CallHistoryRowExternalUser.tsxapps/meteor/client/views/mediaCallHistory/CallHistoryRowInternalUser.tsxapps/meteor/client/views/mediaCallHistory/useMediaCallInternalHistoryActions.tspackages/i18n/src/locales/en.i18n.jsonpackages/ui-voip/src/context/MediaCallContext.tspackages/ui-voip/src/context/index.tspackages/ui-voip/src/index.tspackages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryActions.tsxpackages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryContextualbar.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/ui-voip/src/context/MediaCallContext.tspackages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryContextualbar.tsxpackages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryActions.tsxapps/meteor/client/views/mediaCallHistory/CallHistoryRowInternalUser.tsxapps/meteor/client/views/mediaCallHistory/useMediaCallInternalHistoryActions.tspackages/ui-voip/src/context/index.tsapps/meteor/client/views/mediaCallHistory/CallHistoryRowExternalUser.tsxpackages/ui-voip/src/index.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37773
File: apps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsx:24-34
Timestamp: 2025-12-18T15:18:31.688Z
Learning: In apps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsx, for internal call history items, the item.contactId is guaranteed to always match either the caller.id or callee.id in the call data, so the contact resolution in getContact will never result in undefined.
📚 Learning: 2025-12-18T15:18:31.688Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37773
File: apps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsx:24-34
Timestamp: 2025-12-18T15:18:31.688Z
Learning: In apps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsx, for internal call history items, the item.contactId is guaranteed to always match either the caller.id or callee.id in the call data, so the contact resolution in getContact will never result in undefined.
Applied to files:
packages/ui-voip/src/context/MediaCallContext.tspackages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryContextualbar.tsxpackages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryActions.tsxapps/meteor/client/views/mediaCallHistory/CallHistoryRowInternalUser.tsxapps/meteor/client/views/mediaCallHistory/useMediaCallInternalHistoryActions.tspackages/ui-voip/src/context/index.tsapps/meteor/client/views/mediaCallHistory/CallHistoryRowExternalUser.tsxpackages/ui-voip/src/index.ts
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
packages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryContextualbar.tsxpackages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryActions.tsxapps/meteor/client/views/mediaCallHistory/CallHistoryRowInternalUser.tsxapps/meteor/client/views/mediaCallHistory/useMediaCallInternalHistoryActions.tsapps/meteor/client/views/mediaCallHistory/CallHistoryRowExternalUser.tsx
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
apps/meteor/client/views/mediaCallHistory/CallHistoryRowInternalUser.tsxpackages/i18n/src/locales/en.i18n.json
📚 Learning: 2025-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
packages/i18n/src/locales/en.i18n.json
📚 Learning: 2025-11-17T22:38:48.631Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37505
File: packages/i18n/src/locales/en.i18n.json:3765-3765
Timestamp: 2025-11-17T22:38:48.631Z
Learning: Rocket.Chat i18n copy: Keep sentence case for the value of "Notification_Desktop_show_voice_calls" in packages/i18n/src/locales/en.i18n.json (“Show desktop notifications for voice calls”) per design directive; do not change to Title Case even if nearby labels differ.
Applied to files:
packages/i18n/src/locales/en.i18n.json
🧬 Code graph analysis (5)
packages/ui-voip/src/context/MediaCallContext.ts (1)
packages/ui-voip/src/context/index.ts (2)
MediaCallExternalState(2-2)isAbleToMakeCall(3-3)
packages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryContextualbar.tsx (1)
packages/ui-voip/src/context/MediaCallContext.ts (2)
useMediaCallExternalContext(141-152)isAbleToMakeCall(124-126)
apps/meteor/client/views/mediaCallHistory/CallHistoryRowInternalUser.tsx (2)
packages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryActions.tsx (1)
HistoryActionCallbacks(11-13)packages/ui-voip/src/context/MediaCallContext.ts (2)
isAbleToMakeCall(124-126)useMediaCallContext(129-138)
apps/meteor/client/views/mediaCallHistory/useMediaCallInternalHistoryActions.ts (1)
packages/ui-voip/src/context/MediaCallContext.ts (1)
useMediaCallContext(129-138)
apps/meteor/client/views/mediaCallHistory/CallHistoryRowExternalUser.tsx (1)
packages/ui-voip/src/context/MediaCallContext.ts (2)
useMediaCallContext(129-138)isAbleToMakeCall(124-126)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (12)
packages/ui-voip/src/context/MediaCallContext.ts (1)
122-126: LGTM! Clean implementation of call gating logic.The type definition and utility function correctly implement the call state gating mechanism. The logic properly prevents new calls when:
- A call is already in progress ('calling', 'ringing', 'ongoing')
- User lacks authorization ('unauthorized')
- Workspace lacks license ('unlicensed')
The implementation is type-safe, concise, and aligns with the PR objective to disable voice call actions when a call is active. Consuming components in CallHistoryContextualbar.tsx and CallHistoryActions.tsx correctly use
isAbleToMakeCallto disable call actions with appropriate "Call in progress" tooltip feedback.packages/i18n/src/locales/en.i18n.json (1)
979-979: NewCall_in_progressstring looks consistent and ready to useKey naming, placement near other call-related strings, and the value “Call in progress” all align with existing i18n conventions and nearby entries. No changes needed.
apps/meteor/client/views/mediaCallHistory/useMediaCallInternalHistoryActions.ts (1)
32-49: State-aware gating for voiceCall action looks correct.The destructured
statefromuseMediaCallContextand the additional guards for unauthorized/unlicensed states properly prevent initiating a call when not permitted or when a call is already in progress.apps/meteor/client/views/mediaCallHistory/CallHistoryRowInternalUser.tsx (1)
70-83: State integration correctly enables call capability awareness.The component now retrieves state from the context and passes it to
getItems, enabling the UI to reflect call availability based on the current media call state.apps/meteor/client/views/mediaCallHistory/CallHistoryRowExternalUser.tsx (1)
15-35: State-driven action gating correctly prevents concurrent calls.The component properly retrieves state, checks for unauthorized/unlicensed conditions, and uses
isAbleToMakeCallto disable the voice call action with an appropriate tooltip when a call is in progress. The dependency array correctly includesstate.packages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryContextualbar.tsx (2)
69-77: State propagation to actions enables coordinated call capability UI.The contextual bar correctly retrieves state from the external context and passes it to
CallHistoryActions, ensuring the header actions reflect call availability.
123-131: Call button correctly disabled when call is in progress.The button's
disabledandtitleprops properly useisAbleToMakeCall(state)to prevent initiating a new call when one is already active, with a clear tooltip explanation.packages/ui-voip/src/index.ts (1)
3-4: Export refactoring improves API clarity.Separating type exports and aliasing
useMediaCallExternalContextasuseMediaCallContextprovides a cleaner public API surface while maintaining backward compatibility.packages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryActions.tsx (3)
6-8: State imports correctly positioned for internal package usage.Importing from the local context module is appropriate for internal package files and maintains proper module boundaries.
37-51: Action item generation correctly integrates call state.The
getItemsfunction properly computesdisabledstate for the voiceCall action based onisAbleToMakeCall(state)and attaches a localized tooltip when the action is unavailable.
53-56: State propagation completes the call capability flow.The component accepts
stateas a prop and correctly forwards it togetItems, enabling state-aware action rendering throughout the call history UI.packages/ui-voip/src/context/index.ts (1)
2-3: Public API extended with call state utilities.Adding
MediaCallStatetype alias andisAbleToMakeCallto the public exports enables consuming code to implement state-aware call capability checks consistently.
apps/meteor/client/views/mediaCallHistory/CallHistoryRowExternalUser.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/views/mediaCallHistory/CallHistoryRowInternalUser.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 9 files
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.0.0 #37967 +/- ##
=================================================
+ Coverage 70.63% 70.64% +0.01%
=================================================
Files 3145 3145
Lines 108710 108724 +14
Branches 19519 19576 +57
=================================================
+ Hits 76782 76813 +31
+ Misses 29922 29916 -6
+ Partials 2006 1995 -11
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
packages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryContextualbar.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryContextualbar.tsx (1)
123-131: Implementation correctly prevents calls during active calls.The Call button is appropriately disabled when
isCallingBlocked(state)returns true, and the tooltip provides clear feedback to users. This addresses the PR objective.Optional: Memoize the blocked state to avoid duplicate calls
While the performance impact is negligible, you could extract the result to avoid calling
isCallingBlocked(state)twice:const { voiceCall, directMessage } = actions; const { duration, callId, direction, startedAt } = data; const { state } = useMediaCallExternalContext(); +const callBlocked = isCallingBlocked(state); const date = useFullStartDate(startedAt); return ( <ContextualbarDialog onClose={onClose}> ... {voiceCall && ( <Button success onClick={voiceCall} - disabled={isCallingBlocked(state)} - title={isCallingBlocked(state) ? t('Call_in_progress') : undefined} + disabled={callBlocked} + title={callBlocked ? t('Call_in_progress') : undefined} >
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryContextualbar.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryContextualbar.tsx
🧠 Learnings (2)
📚 Learning: 2025-12-18T15:18:31.688Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37773
File: apps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsx:24-34
Timestamp: 2025-12-18T15:18:31.688Z
Learning: In apps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsx, for internal call history items, the item.contactId is guaranteed to always match either the caller.id or callee.id in the call data, so the contact resolution in getContact will never result in undefined.
Applied to files:
packages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryContextualbar.tsx
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
packages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryContextualbar.tsx
🧬 Code graph analysis (1)
packages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryContextualbar.tsx (1)
packages/ui-voip/src/context/MediaCallContext.ts (2)
useMediaCallExternalContext(141-152)isCallingBlocked(124-126)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
packages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryContextualbar.tsx (2)
21-22: LGTM!The imports are correctly added to support call state checking.
69-69: No issues found. Thestatedestructure fromuseMediaCallExternalContext()correctly receives typeMediaCallExternalState, andisCallingBlocked()properly handles all possible state values:State('closed', 'new', 'calling', 'ringing', 'ongoing'), 'unauthorized', and 'unlicensed'. All context return types have compatible state properties, and the logic blocks calls as expected for all non-'new' and non-'closed' states.
Proposed changes (including videos or screenshots)
Issue(s)
VGA-133
Steps to test or reproduce
Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.