[MM-69549] Modularize Calls webapp state management (active_calls, sessions, hosts, availability) - #1231
Open
M-ZubairAhmed wants to merge 19 commits into
Open
[MM-69549] Modularize Calls webapp state management (active_calls, sessions, hosts, availability)#1231M-ZubairAhmed wants to merge 19 commits into
M-ZubairAhmed wants to merge 19 commits into
Conversation
…r future enhancements related to SIP call handling.
…on dispatches in various files to utilize the new `activeCallRegistered` function, improving code consistency and readability. Adjusted selectors to reflect changes in state structure for SIP call details.
…etSipCallDetailsFromCallState` to a more logical position within the import statements, enhancing readability and maintainability of the test file.
…mport paths for session actions and improved user ID retrieval from sessions. Introduced new selectors and actions for channel call availability, enhancing the overall structure and maintainability of the codebase. Removed deprecated components and streamlined the call state management process.
…dated action types and action creators to improve consistency, renaming `activeCallRegistered` to `activeCallAdded`. Adjusted import paths for call availability actions and selectors, enhancing code organization. Introduced new state management for channel call availability, including actions and reducers, to streamline functionality.
…t now only renders the disable calls message based on channel availability, simplifying the logic and improving clarity.
…cy in action type definitions across files, maintaining clarity in the state management structure.
…n types and actions for host changes, replacing the previous `CALL_HOST` action. Updated selectors and reducers to support the new host management structure, enhancing clarity and maintainability in call state management.
…nd functionality. Introduced new actions for participant management, including muting and removing participants, while updating selectors to utilize the new ActiveCall type. Removed deprecated host actions to streamline the codebase.
…or selectors and components to improve type safety and maintainability.
…ated setup file. Updated paths for improved clarity and maintainability in test setup.
… `fetchIsCallActiveInChannel` for improved clarity and functionality. Update selectors and imports to enhance maintainability, including the addition of `hasPermissionToRenderCallsButtonInChannelHeader` for better permission handling in channel headers.
M-ZubairAhmed
marked this pull request as ready for review
June 29, 2026 18:08
…to MM-69412_call-props-in-callstate-isphone
Member
Author
|
/update-branch |
Contributor
|
Error trying to update the PR. |
The new phone-call tests initialized only botSession, but getBotID() reads the independently-cached botID field. With botID empty, isPhoneCallChannel short-circuits to false, so the SIP-hangup path never runs: - TestRemoveUserSessionPhoneCall: lingering SIP session not dropped - TestHandleLiveKitSIPParticipant/last_SIP_participant_left...: call never ended (EndAt stayed 0) Set botID alongside botSession (matching logs_test.go / limits_test.go), including the non-bot-DM subtest so it exercises the channel-type check rather than short-circuiting on the empty bot ID. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
M-ZubairAhmed
changed the title
Member
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
📝 Walkthroughcommon_action_types : usescommon_selectors --> emptyRootReducer : fallback |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webapp/src/index.tsx (1)
843-859: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep the channel-header menu action in sync with channel switches.
Line 854 now gates the Enable/Disable calls menu by channel-specific permissions, but the subscriber below only refreshes it on first load. This can leave the menu action visible or hidden based on a previous channel.
Proposed fix
- let channelHeaderMenuID: string; + let channelHeaderMenuID = ''; + const unregisterChannelHeaderMenuAction = () => { + if (channelHeaderMenuID) { + registry.unregisterComponent(channelHeaderMenuID); + channelHeaderMenuID = ''; + } + }; + const registerChannelHeaderMenuAction = () => { channelHeaderMenuID = registry.registerChannelHeaderMenuAction( ChannelHeaderMenuItem, () => store.dispatch(toggleCallsAvailabilityForChannel()), ); }; + this.unsubscribers.push(unregisterChannelHeaderMenuAction); const registerHeaderMenuComponentIfNeeded = async (channelID: string) => { try { - registry.unregisterComponent(channelHeaderMenuID); + unregisterChannelHeaderMenuAction(); if (hasPermissionToRenderCallsButtonInChannelHeader(store.getState(), channelID)) { registerChannelHeaderMenuAction(); } } catch (err) { - registry.unregisterComponent(channelHeaderMenuID); + unregisterChannelHeaderMenuAction(); logErr(err); } };- const firstLoad = !currChannelId; currChannelId = currentChannelId; - // We only want to register the header menu component on first load and not - // on every channel switch. - if (firstLoad) { - registerHeaderMenuComponentIfNeeded(currentChannelId); - } + registerHeaderMenuComponentIfNeeded(currentChannelId);Also applies to: 983-987
🤖 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 `@webapp/src/index.tsx` around lines 843 - 859, The channel-header menu action is now computed from channel-specific permissions, but it is only refreshed on initial setup, so it can get out of sync when switching channels. Update the channel change subscriber to call the existing refresh path, using registerHeaderMenuComponentIfNeeded and the channel ID from the current channel state, so the Enable/Disable calls item is re-evaluated on every channel switch. Make sure the unregister/register flow remains centralized in registerChannelHeaderMenuAction and registerHeaderMenuComponentIfNeeded to avoid stale visibility.
🧹 Nitpick comments (1)
webapp/src/state/active_calls/actions.test.ts (1)
15-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
fetchIsCallActiveInChannel.This file only exercises
activeCallAdded, so the new REST helper inactions.tsstill ships without success/error-path coverage. A small mockedRestClient.fetchsuite would lock down thetrueand fallback-falsebranches.🤖 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 `@webapp/src/state/active_calls/actions.test.ts` around lines 15 - 33, Add test coverage for fetchIsCallActiveInChannel in actions.test.ts, since the current suite only validates activeCallAdded. Create a small mocked RestClient.fetch setup around fetchIsCallActiveInChannel to cover the success path returning true and the failure/fallback path returning false, using the fetchIsCallActiveInChannel symbol to locate the new helper and ensure both branches are locked down.
🤖 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 `@webapp/src/actions.ts`:
- Around line 590-621: The hydration path in the thunk that handles call state
should not skip Redux refreshes when a call already exists in state. Move the
call data reconciliation actions in this block—activeCallAdded, hostChanged, and
sessionsReceived—so they always run for the call in channel, and keep the
callStartAtForCallInChannel(...) guard only around the incoming notification
branch using ringingEnabled and incomingCallOnChannel. This ensures the stored
call, host, and session data are refreshed during activation/reconnect even for
already-known calls.
In `@webapp/src/components/expanded_view/component.tsx`:
- Around line 403-410: The host control calls are still using channel.id instead
of the active call’s callID, so update ExpandedView to thread the current
ActiveCall['callID'] from state and pass that through to
hostMuteAllParticipants, hostRemoveParticipant, and HostControlsMenu. Use the
existing symbols handleMuteOthers, handleRemoveParticipant, and HostControlsMenu
to replace any fallback ''/channel-based IDs with the active call ID
consistently.
In `@webapp/src/selectors.ts`:
- Around line 257-258: The call dismissal lookup in callDismissedNotification is
using channelID to read dismissedCalls, but this store is keyed by call ID.
Update the selector to accept and use the call’s callID consistently, matching
dismissedCallForCurrentChannel and any callers such as actions.ts, so previously
dismissed calls are correctly recognized.
In `@webapp/src/state/active_calls/reducer.ts`:
- Around line 42-45: The CALL_ENDED branch in active_calls/reducer.ts should not
remove an entry using only channelID; it needs to verify the stored callID
matches action.data.callID before deleting. Update the CALL_ENDED handling in
the reducer so it looks up the active call for that channel and only clears it
when both channelID and callID correspond to the same call, preventing an older
end event from removing a newer active call.
In `@webapp/src/state/calls_availability/actions.ts`:
- Line 13: The toggle logic is only flipping callsNotAvailableInChannel(...),
which covers explicit stored overrides but ignores the effective availability
when callsConfig.DefaultEnabled is false and no row exists yet. Update the
thunk/action in calls_availability/actions.ts to compute the current effective
state first, then invert that state when building the payload so the first
enable/disable action works correctly for default-disabled channels. Use
callsNotAvailableInChannel and the surrounding toggle action as the key places
to adjust the logic and payload generation.
In `@webapp/src/state/calls_availability/selectors.ts`:
- Around line 44-46: The selector in calls_availability/selectors.ts is
short-circuiting all non-system-admins when defaultEnabled(state) is false,
which overrides explicit per-channel enables. Update the availability logic in
the selector that drives the header button so it still returns true when
callsAvailability[channelId].enabled is explicitly set, and only falls back to
the global default when there is no channel-specific setting; keep the behavior
aligned with callsAvailableInChannelWithDefault(...) and the render path in
index.tsx.
In `@webapp/src/state/hosts/selectors.ts`:
- Around line 17-27: The host selectors are using the currently viewed channel
instead of the active call channel, which can make the live call UI read the
wrong host record. Update getHostForCurrentCall in selectors.ts to key off the
active call channel from the call state/store used by the call widget, and keep
getHostIDForCurrentChannel/getHostChangeAtForCurrentChannel delegating through
that corrected selector so the call widget always reflects the active call.
---
Outside diff comments:
In `@webapp/src/index.tsx`:
- Around line 843-859: The channel-header menu action is now computed from
channel-specific permissions, but it is only refreshed on initial setup, so it
can get out of sync when switching channels. Update the channel change
subscriber to call the existing refresh path, using
registerHeaderMenuComponentIfNeeded and the channel ID from the current channel
state, so the Enable/Disable calls item is re-evaluated on every channel switch.
Make sure the unregister/register flow remains centralized in
registerChannelHeaderMenuAction and registerHeaderMenuComponentIfNeeded to avoid
stale visibility.
---
Nitpick comments:
In `@webapp/src/state/active_calls/actions.test.ts`:
- Around line 15-33: Add test coverage for fetchIsCallActiveInChannel in
actions.test.ts, since the current suite only validates activeCallAdded. Create
a small mocked RestClient.fetch setup around fetchIsCallActiveInChannel to cover
the success path returning true and the failure/fallback path returning false,
using the fetchIsCallActiveInChannel symbol to locate the new helper and ensure
both branches are locked down.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b38bdc3f-5af6-415d-88d9-48d22931c65d
📒 Files selected for processing (64)
build/sync/plan.ymlserver/api_livekit_webhook_test.goserver/session_test.gostandalone/src/index.tsstandalone/src/recording/components/recording_view/index.tsxstandalone/src/recording/index.tsxwebapp/jest.config.jswebapp/src/action_types.tswebapp/src/actions.tswebapp/src/components/call_widget/component.tsxwebapp/src/components/call_widget/index.tswebapp/src/components/call_widget/participant.tsxwebapp/src/components/call_widget/participants_list.tsxwebapp/src/components/channel_call_toast.tsxwebapp/src/components/channel_header_button.tsxwebapp/src/components/channel_header_dropdown_button/index.tswebapp/src/components/channel_header_menu_button/index.tswebapp/src/components/channel_header_menu_item/index.tsxwebapp/src/components/custom_post_types/post_type/index.tswebapp/src/components/expanded_view/call_participant.tsxwebapp/src/components/expanded_view/call_participant_rhs.tsxwebapp/src/components/expanded_view/component.tsxwebapp/src/components/expanded_view/index.tswebapp/src/components/host_controls_menu.tsxwebapp/src/i18n.mock.jsonwebapp/src/index.tsxwebapp/src/reducers.tswebapp/src/selectors.tswebapp/src/slash_commands.tsxwebapp/src/state/README.mdwebapp/src/state/active_calls/action_types.tswebapp/src/state/active_calls/actions.test.tswebapp/src/state/active_calls/actions.tswebapp/src/state/active_calls/reducer.test.tswebapp/src/state/active_calls/reducer.tswebapp/src/state/calls_availability/action_types.tswebapp/src/state/calls_availability/actions.test.tswebapp/src/state/calls_availability/actions.tswebapp/src/state/calls_availability/reducer.test.tswebapp/src/state/calls_availability/reducer.tswebapp/src/state/calls_availability/selectors.tswebapp/src/state/common_action_types.tswebapp/src/state/common_actions.test.tswebapp/src/state/common_actions.tswebapp/src/state/common_selectors.tswebapp/src/state/hosts/action_types.tswebapp/src/state/hosts/actions.test.tswebapp/src/state/hosts/actions.tswebapp/src/state/hosts/reducer.test.tswebapp/src/state/hosts/reducer.tswebapp/src/state/hosts/selectors.tswebapp/src/state/screen_sharing_ids/actions.test.tswebapp/src/state/screen_sharing_ids/actions.tswebapp/src/state/screen_sharing_ids/reducer.test.tswebapp/src/state/screen_sharing_ids/reducer.tswebapp/src/state/sessions/action_types.tswebapp/src/state/sessions/actions.test.tswebapp/src/state/sessions/actions.tswebapp/src/state/sessions/reducer.test.tswebapp/src/state/sessions/reducer.tswebapp/src/state/sessions/selectors.tswebapp/src/utils.tswebapp/src/websocket_handlers.tswebapp/tests/setup.js
💤 Files with no reviewable changes (5)
- webapp/tests/setup.js
- build/sync/plan.yml
- webapp/src/components/channel_header_menu_button/index.ts
- webapp/src/state/sessions/action_types.ts
- webapp/src/action_types.ts
Comment on lines
+590
to
+621
| if (!callStartAtForCallInChannel(getState(), callAndChannelState.channel_id)) { | ||
| actions.push( | ||
| activeCallAdded(callAndChannelState.channel_id, { | ||
| callID: callAndChannelState.call.id, | ||
| startAt: callAndChannelState.call.start_at, | ||
| ownerID: callAndChannelState.call.owner_id, | ||
| threadID: callAndChannelState.call.thread_id, | ||
| }), | ||
| ); | ||
|
|
||
| actions.push(hostChanged(callAndChannelState.channel_id, callAndChannelState.call.host_id, callAndChannelState.call.start_at)); | ||
|
|
||
| actions.push(sessionsReceived(callAndChannelState.channel_id, getSessionsMapFromSessions(callAndChannelState.call.sessions))); | ||
|
|
||
| if (ringingEnabled(getState())) { | ||
| // dismissedNotification is populated after the actions array has been batched, so manually check: | ||
| const dismissed = callAndChannelState.call.dismissed_notification; | ||
| if (dismissed) { | ||
| const currentUserID = getCurrentUserId(getState()); | ||
| if (Object.hasOwn(dismissed, currentUserID) && dismissed[currentUserID]) { | ||
| actions.push({ | ||
| type: DISMISS_CALL, | ||
| data: { | ||
| callID: callAndChannelState.call.id, | ||
| }, | ||
| }); | ||
| continue; | ||
| } | ||
| } | ||
| dispatch(incomingCallOnChannel(callAndChannelState.channel_id, callAndChannelState.call.id, callAndChannelState.call.owner_id, callAndChannelState.call.start_at)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Refresh known calls during hydration.
The if (!callStartAtForCallInChannel(...)) guard skips activeCallAdded, hostChanged, and sessionsReceived for calls already in Redux. Since this thunk is used during activation/reconnect, any host or session changes missed while offline can stay stale. Keep the guard only around the “new incoming call” notification path.
Proposed fix
- if (!callStartAtForCallInChannel(getState(), callAndChannelState.channel_id)) {
- actions.push(
- activeCallAdded(callAndChannelState.channel_id, {
- callID: callAndChannelState.call.id,
- startAt: callAndChannelState.call.start_at,
- ownerID: callAndChannelState.call.owner_id,
- threadID: callAndChannelState.call.thread_id,
- }),
- );
-
- actions.push(hostChanged(callAndChannelState.channel_id, callAndChannelState.call.host_id, callAndChannelState.call.start_at));
-
- actions.push(sessionsReceived(callAndChannelState.channel_id, getSessionsMapFromSessions(callAndChannelState.call.sessions)));
-
- if (ringingEnabled(getState())) {
+ const isKnownActiveCall = Boolean(callStartAtForCallInChannel(getState(), callAndChannelState.channel_id));
+ actions.push(
+ activeCallAdded(callAndChannelState.channel_id, {
+ callID: callAndChannelState.call.id,
+ startAt: callAndChannelState.call.start_at,
+ ownerID: callAndChannelState.call.owner_id,
+ threadID: callAndChannelState.call.thread_id,
+ }),
+ );
+
+ actions.push(hostChanged(callAndChannelState.channel_id, callAndChannelState.call.host_id, callAndChannelState.call.start_at));
+ actions.push(sessionsReceived(callAndChannelState.channel_id, getSessionsMapFromSessions(callAndChannelState.call.sessions)));
+
+ if (!isKnownActiveCall && ringingEnabled(getState())) {
// dismissedNotification is populated after the actions array has been batched, so manually check:
const dismissed = callAndChannelState.call.dismissed_notification;
if (dismissed) {
const currentUserID = getCurrentUserId(getState());
if (Object.hasOwn(dismissed, currentUserID) && dismissed[currentUserID]) {
@@
}
}
dispatch(incomingCallOnChannel(callAndChannelState.channel_id, callAndChannelState.call.id, callAndChannelState.call.owner_id, callAndChannelState.call.start_at));
- }
}📝 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
| if (!callStartAtForCallInChannel(getState(), callAndChannelState.channel_id)) { | |
| actions.push( | |
| activeCallAdded(callAndChannelState.channel_id, { | |
| callID: callAndChannelState.call.id, | |
| startAt: callAndChannelState.call.start_at, | |
| ownerID: callAndChannelState.call.owner_id, | |
| threadID: callAndChannelState.call.thread_id, | |
| }), | |
| ); | |
| actions.push(hostChanged(callAndChannelState.channel_id, callAndChannelState.call.host_id, callAndChannelState.call.start_at)); | |
| actions.push(sessionsReceived(callAndChannelState.channel_id, getSessionsMapFromSessions(callAndChannelState.call.sessions))); | |
| if (ringingEnabled(getState())) { | |
| // dismissedNotification is populated after the actions array has been batched, so manually check: | |
| const dismissed = callAndChannelState.call.dismissed_notification; | |
| if (dismissed) { | |
| const currentUserID = getCurrentUserId(getState()); | |
| if (Object.hasOwn(dismissed, currentUserID) && dismissed[currentUserID]) { | |
| actions.push({ | |
| type: DISMISS_CALL, | |
| data: { | |
| callID: callAndChannelState.call.id, | |
| }, | |
| }); | |
| continue; | |
| } | |
| } | |
| dispatch(incomingCallOnChannel(callAndChannelState.channel_id, callAndChannelState.call.id, callAndChannelState.call.owner_id, callAndChannelState.call.start_at)); | |
| } | |
| } | |
| const isKnownActiveCall = Boolean(callStartAtForCallInChannel(getState(), callAndChannelState.channel_id)); | |
| actions.push( | |
| activeCallAdded(callAndChannelState.channel_id, { | |
| callID: callAndChannelState.call.id, | |
| startAt: callAndChannelState.call.start_at, | |
| ownerID: callAndChannelState.call.owner_id, | |
| threadID: callAndChannelState.call.thread_id, | |
| }), | |
| ); | |
| actions.push(hostChanged(callAndChannelState.channel_id, callAndChannelState.call.host_id, callAndChannelState.call.start_at)); | |
| actions.push(sessionsReceived(callAndChannelState.channel_id, getSessionsMapFromSessions(callAndChannelState.call.sessions))); | |
| if (!isKnownActiveCall && ringingEnabled(getState())) { | |
| // dismissedNotification is populated after the actions array has been batched, so manually check: | |
| const dismissed = callAndChannelState.call.dismissed_notification; | |
| if (dismissed) { | |
| const currentUserID = getCurrentUserId(getState()); | |
| if (Object.hasOwn(dismissed, currentUserID) && dismissed[currentUserID]) { | |
| actions.push({ | |
| type: DISMISS_CALL, | |
| data: { | |
| callID: callAndChannelState.call.id, | |
| }, | |
| }); | |
| continue; | |
| } | |
| } | |
| dispatch(incomingCallOnChannel(callAndChannelState.channel_id, callAndChannelState.call.id, callAndChannelState.call.owner_id, callAndChannelState.call.start_at)); | |
| } |
🤖 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 `@webapp/src/actions.ts` around lines 590 - 621, The hydration path in the
thunk that handles call state should not skip Redux refreshes when a call
already exists in state. Move the call data reconciliation actions in this
block—activeCallAdded, hostChanged, and sessionsReceived—so they always run for
the call in channel, and keep the callStartAtForCallInChannel(...) guard only
around the incoming notification branch using ringingEnabled and
incomingCallOnChannel. This ensures the stored call, host, and session data are
refreshed during activation/reconnect even for already-known calls.
Comment on lines
+403
to
+410
| handleMuteOthers = () => { | ||
| if (!this.props.channel) { | ||
| logErr('ExpandedView: host muting other failed, channel should be defined'); | ||
| return; | ||
| } | ||
|
|
||
| logDebug('ExpandedView: host muting others'); | ||
| hostMuteAllParticipants(this.props.channel.id); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Pass the active call ID here, not channel.id.
These paths now call host endpoints shaped as /calls/{callID}/host/..., and the new action signatures are typed as ActiveCall['callID']. This component still passes this.props.channel.id (or '') into hostMuteAllParticipants, hostRemoveParticipant, and HostControlsMenu, so host controls will target the wrong resource whenever the call ID differs from the channel ID. Please thread the current active call’s callID from state and use that value consistently here.
Also applies to: 909-911, 1165-1176
🤖 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 `@webapp/src/components/expanded_view/component.tsx` around lines 403 - 410,
The host control calls are still using channel.id instead of the active call’s
callID, so update ExpandedView to thread the current ActiveCall['callID'] from
state and pass that through to hostMuteAllParticipants, hostRemoveParticipant,
and HostControlsMenu. Use the existing symbols handleMuteOthers,
handleRemoveParticipant, and HostControlsMenu to replace any fallback
''/channel-based IDs with the active call ID consistently.
Comment on lines
257
to
+258
| export const callDismissedNotification = (state: GlobalState, channelID: string) => { | ||
| return Boolean(pluginReduxStore(state).dismissedCalls[channelID]); | ||
| return Boolean(getPluginStore(state).dismissedCalls[channelID]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Index dismissed calls by call ID, not channel ID.
Line 258 now reads from dismissedCalls[channelID], but this slice is keyed by call ID and the adjacent dismissedCallForCurrentChannel selector uses call.callID. Callers passing a channel ID, such as webapp/src/actions.ts, will miss prior dismissals and can re-notify for dismissed calls.
Proposed fix
export const callDismissedNotification = (state: GlobalState, channelID: string) => {
- return Boolean(getPluginStore(state).dismissedCalls[channelID]);
+ const callID = getPluginStore(state).activeCalls[channelID]?.callID;
+ return Boolean(callID && getPluginStore(state).dismissedCalls[callID]);
};📝 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
| export const callDismissedNotification = (state: GlobalState, channelID: string) => { | |
| return Boolean(pluginReduxStore(state).dismissedCalls[channelID]); | |
| return Boolean(getPluginStore(state).dismissedCalls[channelID]); | |
| export const callDismissedNotification = (state: GlobalState, channelID: string) => { | |
| const callID = getPluginStore(state).activeCalls[channelID]?.callID; | |
| return Boolean(callID && getPluginStore(state).dismissedCalls[callID]); | |
| }; |
🤖 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 `@webapp/src/selectors.ts` around lines 257 - 258, The call dismissal lookup in
callDismissedNotification is using channelID to read dismissedCalls, but this
store is keyed by call ID. Update the selector to accept and use the call’s
callID consistently, matching dismissedCallForCurrentChannel and any callers
such as actions.ts, so previously dismissed calls are correctly recognized.
Comment on lines
+42
to
+45
| case CALL_ENDED: { | ||
| const nextState = {...initialState}; | ||
| delete nextState[action.data.channelID]; | ||
| return nextState; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Guard CALL_ENDED with the stored callID.
This branch drops the entry purely by channelID, even though the action carries callID as well. If an older end event arrives after a new call has already started in the same channel, the reducer will remove the newer call from activeCalls.
Suggested fix
case CALL_ENDED: {
+ const current = initialState[action.data.channelID];
+ if (!current || current.callID !== action.data.callID) {
+ return initialState;
+ }
+
const nextState = {...initialState};
delete nextState[action.data.channelID];
return nextState;
}📝 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
| case CALL_ENDED: { | |
| const nextState = {...initialState}; | |
| delete nextState[action.data.channelID]; | |
| return nextState; | |
| case CALL_ENDED: { | |
| const current = initialState[action.data.channelID]; | |
| if (!current || current.callID !== action.data.callID) { | |
| return initialState; | |
| } | |
| const nextState = {...initialState}; | |
| delete nextState[action.data.channelID]; | |
| return nextState; |
🤖 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 `@webapp/src/state/active_calls/reducer.ts` around lines 42 - 45, The
CALL_ENDED branch in active_calls/reducer.ts should not remove an entry using
only channelID; it needs to verify the stored callID matches action.data.callID
before deleting. Update the CALL_ENDED handling in the reducer so it looks up
the active call for that channel and only clears it when both channelID and
callID correspond to the same call, preventing an older end event from removing
a newer active call.
| import {getPluginPath} from 'src/utils'; | ||
|
|
||
| import {CHANNEL_CALLS_AVAILABILITY_UPDATED} from './action_types'; | ||
| import {callsNotAvailableInChannel} from './selectors'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Toggle the effective availability, not only explicit overrides.
Line 33 only flips callsNotAvailableInChannel(...), which is true only for stored enabled: false rows. If callsConfig.DefaultEnabled is false and the channel has no row yet, the effective state is still disabled, but this thunk posts {enabled: false} again. That makes the first enable attempt a no-op in default-disabled channels.
Suggested fix
-import {callsNotAvailableInChannel} from './selectors';
+import {callsAvailableInChannelWithDefault} from './selectors';
@@
- body: JSON.stringify({enabled: callsNotAvailableInChannel(getState(), currentChannelID)}),
+ body: JSON.stringify({enabled: !callsAvailableInChannelWithDefault(getState(), currentChannelID)}),Also applies to: 31-33
🤖 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 `@webapp/src/state/calls_availability/actions.ts` at line 13, The toggle logic
is only flipping callsNotAvailableInChannel(...), which covers explicit stored
overrides but ignores the effective availability when callsConfig.DefaultEnabled
is false and no row exists yet. Update the thunk/action in
calls_availability/actions.ts to compute the current effective state first, then
invert that state when building the payload so the first enable/disable action
works correctly for default-disabled channels. Use callsNotAvailableInChannel
and the surrounding toggle action as the key places to adjust the logic and
payload generation.
Comment on lines
+44
to
+46
| if (!defaultEnabled(state)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't short-circuit explicit channel enables when the global default is off.
Line 44 returns false for every non-system-admin whenever DefaultEnabled is false. That also hides the header button in channels whose callsAvailability[channelId].enabled was explicitly set to true, even though callsAvailableInChannelWithDefault(...) treats those channels as available and index.tsx uses this selector to decide whether the button renders.
Suggested fix
- if (!defaultEnabled(state)) {
+ if (!defaultEnabled(state) && !callsAvailableInChannel(state, channelId)) {
return false;
}📝 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
| if (!defaultEnabled(state)) { | |
| return false; | |
| } | |
| if (!defaultEnabled(state) && !callsAvailableInChannel(state, channelId)) { | |
| return false; | |
| } |
🤖 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 `@webapp/src/state/calls_availability/selectors.ts` around lines 44 - 46, The
selector in calls_availability/selectors.ts is short-circuiting all
non-system-admins when defaultEnabled(state) is false, which overrides explicit
per-channel enables. Update the availability logic in the selector that drives
the header button so it still returns true when
callsAvailability[channelId].enabled is explicitly set, and only falls back to
the global default when there is no channel-specific setting; keep the behavior
aligned with callsAvailableInChannelWithDefault(...) and the render path in
index.tsx.
Comment on lines
+17
to
+27
| export const getHostForCurrentCall = (state: GlobalState) => { | ||
| const currentChannelID = getCurrentChannelId(state); | ||
| return getPluginStore(state).hosts[currentChannelID]; | ||
| }; | ||
|
|
||
| export const getHostIDForCurrentChannel = (state: GlobalState) => { | ||
| return getHostForCurrentCall(state)?.hostID; | ||
| }; | ||
|
|
||
| export const getHostChangeAtForCurrentChannel = (state: GlobalState) => { | ||
| return getHostForCurrentCall(state)?.hostChangeAt; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the active call channel here, not getCurrentChannelId.
getHostForCurrentCall is keyed off the currently viewed channel, but the global call widget consumes these selectors for the live call UI (webapp/src/components/call_widget/index.ts, Lines 83-84). If I join a call and then browse to another channel, the widget will start reading that other channel’s host record, so host controls/timestamps can drift away from the actual active call.
🤖 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 `@webapp/src/state/hosts/selectors.ts` around lines 17 - 27, The host selectors
are using the currently viewed channel instead of the active call channel, which
can make the live call UI read the wrong host record. Update
getHostForCurrentCall in selectors.ts to key off the active call channel from
the call state/store used by the call widget, and keep
getHostIDForCurrentChannel/getHostChangeAtForCurrentChannel delegating through
that corrected selector so the call widget always reflects the active call.
bgardner8008
requested changes
Jul 2, 2026
bgardner8008
left a comment
bgardner8008
left a comment
Contributor
There was a problem hiding this comment.
Thanks Zubair. Mostly I have to rely on claude to review react code, but between claude and coderabbit I think the review looks complete.
| }; | ||
|
|
||
| export const getHostForCurrentCall = (state: GlobalState) => { | ||
| const currentChannelID = getCurrentChannelId(state); |
Contributor
There was a problem hiding this comment.
This returns channelID for currently viewed channel, the call might be on another channel. It should use "const currentChannelID = channelIDForCurrentCall(state);" instead.
| }; | ||
| }; | ||
|
|
||
| export const hydradeCallsAndChannelStatesExcept = (skipChannelID?: string) => { |
Contributor
There was a problem hiding this comment.
should be "hydrate" instead?
| return getHostForCurrentCall(state)?.hostID; | ||
| }; | ||
|
|
||
| export const getHostChangeAtForCurrentChannel = (state: GlobalState) => { |
Contributor
There was a problem hiding this comment.
Should probably be renamed to getHostChangeAtForCurrentCall once fixed below?
| return getPluginStore(state).hosts[currentChannelID]; | ||
| }; | ||
|
|
||
| export const getHostIDForCurrentChannel = (state: GlobalState) => { |
Contributor
There was a problem hiding this comment.
should be renamed to getHostIDForCurrentCall?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This pull request introduces significant refactoring and cleanup of call state management and related actions in the codebase. The main focus is on removing legacy or redundant actions, consolidating state logic, and improving the structure and naming of selectors and actions related to calls, sessions, and hosts. It also improves test setup and updates some configuration and mock file references.
Removed legacy action types and actions such as
CALL_STATEandCALL_HOST, and replaced their usage with more focused actions likeactiveCallAddedandhostChangedRefactored session and host-related actions and selectors to use the new
sessionsandhostsstate modules, replacing older, less descriptive imports and usages.Removed a set of host-related async actions (
hostMake,hostMute,hostScreenOff,hostLowerHand,hostRemove,hostMuteOthers) from the main actions file, likely moving them to a more appropriate location or replacing them with new implementations. (webapp/src/actions.ts)Removed the legacy
getCallActiveaction and replaced its usage withfetchIsCallActiveInChannelUpdated Jest configuration to point to the correct i18n mock file and removed a redundant test setup file from the build sync plan.
Ticket Link
Fixes https://mattermost.atlassian.net/browse/MM-69549
Screenshots
Release Note