refactor(ui-voip): Simplify useAvailableViewTracker logic and revert currentViews type from Set to Array - #41687
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughThe media call view tracker now exposes ChangesMedia call view tracking
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/ui-voip/src/providers/useAvailableViewTracker.ts`:
- Around line 19-41: Update the state updaters in registerView and
unregisterView to clone prev before calling add or delete, preserving the early
returns when the view is already registered or absent. Return the cloned set
only when the view actually changes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1e2fd5e8-2466-48b9-b379-b7e22a18e101
📒 Files selected for processing (6)
packages/ui-voip/src/context/MediaCallInstanceContext.tspackages/ui-voip/src/providers/useAvailableViewTracker.tspackages/ui-voip/src/views/MediaCallPopout.tsxpackages/ui-voip/src/views/MediaCallRoomSection/MediaCallRoomSection.tsxpackages/ui-voip/src/views/MediaCallWidget/MediaCallWidget.tsxpackages/ui-voip/src/views/MediaCallWidget/OngoingCallWithScreen.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Hacktron Security Check
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 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/MediaCallRoomSection/MediaCallRoomSection.tsxpackages/ui-voip/src/views/MediaCallPopout.tsxpackages/ui-voip/src/providers/useAvailableViewTracker.tspackages/ui-voip/src/context/MediaCallInstanceContext.tspackages/ui-voip/src/views/MediaCallWidget/MediaCallWidget.tsxpackages/ui-voip/src/views/MediaCallWidget/OngoingCallWithScreen.tsx
🧠 Learnings (6)
📚 Learning: 2026-02-26T19:22:29.385Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryActions.tsx:40-40
Timestamp: 2026-02-26T19:22:29.385Z
Learning: For TSX files in the UI VOIP package, ensure that when a media session state is 'unavailable', the voiceCall action is excluded from the actions object passed to CallHistoryActions so it does not render in the menu. This filtering should occur upstream (before getItems is called) to avoid tooltips or UI hints for unavailable actions. If there are multiple actions with availability states, implement a centralized helper to filter actions based on session state.
Applied to files:
packages/ui-voip/src/views/MediaCallRoomSection/MediaCallRoomSection.tsxpackages/ui-voip/src/views/MediaCallPopout.tsxpackages/ui-voip/src/views/MediaCallWidget/MediaCallWidget.tsxpackages/ui-voip/src/views/MediaCallWidget/OngoingCallWithScreen.tsx
📚 Learning: 2026-05-05T12:34:29.042Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 40331
File: packages/ui-voip/src/views/MediaCallWidget/OngoingCallWithScreen.tsx:69-69
Timestamp: 2026-05-05T12:34:29.042Z
Learning: In Rocket.Chat’s `packages/ui-voip` UI (e.g., media/call widgets), voice/media calls are only supported in Direct Message (DM) rooms. Rocket.Chat models a DM as a “room” with exactly two participants, so handlers like `onClickDirectMessage` are the correct destination—even when the UI text/element says “Open in room” (e.g., on the shared screen card/`StreamCard`). During review, don’t flag a “DM vs room” mismatch for these cases; they intentionally map to the same destination.
Applied to files:
packages/ui-voip/src/views/MediaCallRoomSection/MediaCallRoomSection.tsxpackages/ui-voip/src/views/MediaCallPopout.tsxpackages/ui-voip/src/views/MediaCallWidget/MediaCallWidget.tsxpackages/ui-voip/src/views/MediaCallWidget/OngoingCallWithScreen.tsx
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
packages/ui-voip/src/views/MediaCallRoomSection/MediaCallRoomSection.tsxpackages/ui-voip/src/views/MediaCallPopout.tsxpackages/ui-voip/src/views/MediaCallWidget/MediaCallWidget.tsxpackages/ui-voip/src/views/MediaCallWidget/OngoingCallWithScreen.tsx
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
packages/ui-voip/src/views/MediaCallRoomSection/MediaCallRoomSection.tsxpackages/ui-voip/src/views/MediaCallPopout.tsxpackages/ui-voip/src/providers/useAvailableViewTracker.tspackages/ui-voip/src/context/MediaCallInstanceContext.tspackages/ui-voip/src/views/MediaCallWidget/MediaCallWidget.tsxpackages/ui-voip/src/views/MediaCallWidget/OngoingCallWithScreen.tsx
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/ui-voip/src/providers/useAvailableViewTracker.tspackages/ui-voip/src/context/MediaCallInstanceContext.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/ui-voip/src/providers/useAvailableViewTracker.tspackages/ui-voip/src/context/MediaCallInstanceContext.ts
🔇 Additional comments (6)
packages/ui-voip/src/context/MediaCallInstanceContext.ts (1)
20-20: LGTM!Also applies to: 40-40
packages/ui-voip/src/providers/useAvailableViewTracker.ts (1)
1-1: LGTM!packages/ui-voip/src/views/MediaCallPopout.tsx (1)
34-34: LGTM!packages/ui-voip/src/views/MediaCallRoomSection/MediaCallRoomSection.tsx (1)
61-61: LGTM!packages/ui-voip/src/views/MediaCallWidget/MediaCallWidget.tsx (1)
16-16: LGTM!packages/ui-voip/src/views/MediaCallWidget/OngoingCallWithScreen.tsx (1)
41-41: LGTM!
| const registerView = useCallback((view: AvailableViews) => { | ||
| if (viewsRef.current.has(view)) return; | ||
| setViews((prev) => { | ||
| if (prev.has(view)) { | ||
| return prev; | ||
| } | ||
|
|
||
| prev.add(view); | ||
|
|
||
| viewsRef.current.add(view); | ||
| const filteredViews = [...viewsRef.current].filter(filter); | ||
| setCurrentViews(getViewsSetStateAction(filteredViews)); | ||
| return new Set(prev); | ||
| }); | ||
| }, []); | ||
|
|
||
| const unregisterView = useCallback((view: AvailableViews) => { | ||
| if (!viewsRef.current.has(view)) return; | ||
| setViews((prev) => { | ||
| if (!prev.has(view)) { | ||
| return prev; | ||
| } | ||
|
|
||
| viewsRef.current.delete(view); | ||
| const filteredViews = [...viewsRef.current].filter(filter); | ||
| setCurrentViews(getViewsSetStateAction(filteredViews)); | ||
| prev.delete(view); | ||
|
|
||
| return new Set(prev); | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## candidate file"
wc -l packages/ui-voip/src/providers/useAvailableViewTracker.ts 2>/dev/null || true
cat -n packages/ui-voip/src/providers/useAvailableViewTracker.ts 2>/dev/null || true
echo
echo "## package React version references"
rg -n '"react"|"`@types/react`"|react@' -S package.json packages 2>/dev/null | head -200 || trueRepository: RocketChat/Rocket.Chat
Length of output: 3998
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## React 19.2.7 runtime/pure updater behavior probe"
node - <<'JS'
function cloneBeforeAdd(prev, view) {
if (prev.has(view)) return prev;
return new Set(prev).add(view);
}
function mutateThenClone(prev, view) {
if (prev.has(view)) return prev;
prev.add(view);
return new Set(prev);
}
function cloneBeforeDelete(prev, view) {
if (prev.has(view)) return new Set(prev).delete(view) ? prev : prev;
return prev;
}
function mutateThenCloneDelete(prev, view) {
if (!prev.has(view)) return prev;
prev.delete(view);
return new Set(prev);
}
function withStrictMode(updater, initialState, events) {
const states = [initialState];
for (const [v] of events) {
const replay = updater(states[states.length - 1], v);
const actual = update ? updater(updater(states[states.length - 1], v), v) : updater(replay, v);
states.push(actual);
}
return states;
}
const view = 'widget';
const init = new Set();
console.log('cloneBeforeAdd', JSON.stringify(Array.from(withStrictMode(cloneBeforeAdd, init, [[1], [2]]).map(s => Array.from(s)))));
console.log('mutateThenClone', JSON.stringify(Array.from(mutateThenClone, init, [[1], [2]]).map(s => Array.from(s))));
JSRepository: RocketChat/Rocket.Chat
Length of output: 888
🌐 Web query:
React useState reducer/updater should be pure input can be mutated by future renders documentation
💡 Result:
In React, updater functions passed to state setters (such as those from useState or useReducer) must be pure [1][2][3]. They should take the pending state as their only argument and return the next state without modifying any pre-existing variables or objects [1][4][3]. You must not mutate the state inside an updater function [3]. Because React may call your updater function multiple times—specifically in development mode (Strict Mode) to help detect impurities—any mutation will result in observable, incorrect behavior, such as state being updated more times than intended [1][4][3]. The fundamental rules for updater functions are: 1. Purity: They must be pure, meaning they produce the same output for the same input and have no side effects [1][5]. 2. Immutability: Treat state as immutable [6][7]. Instead of mutating the existing state object or array (e.g., using.push or direct property assignment), always return a new object or array that represents the updated state [6][3][7]. 3. No Side Effects: Updater functions should only calculate and return the new state; they should not trigger side effects (such as making network requests, modifying the DOM, or setting other state variables) [2][5]. React uses these functions to calculate the next state during the render phase [2]. If an updater function is impure, the extra calls in Strict Mode will reveal the mistake by causing bugs, such as duplicated additions to an array, which would not happen if the function simply returned a new, unmutated copy of the data [1][3].
Citations:
- 1: https://react.dev/reference/react/useState
- 2: https://react.dev/learn/queueing-a-series-of-state-updates
- 3: https://uk.react.dev/reference/react/useState
- 4: https://react.dev/learn/keeping-components-pure
- 5: https://stackoverflow.com/questions/71780110/is-it-okey-to-use-side-effects-in-the-usestate-hook-callback
- 6: https://react.dev/learn/updating-objects-in-state
- 7: https://uk.react.dev/learn/updating-objects-in-state
Clone the previous state set before changing it.
registerView and unregisterView mutate prev, which React updater functions should treat as immutable. Clone before add or delete so the updater returns a different set object only when the view actually changed.
Proposed fix
if (prev.has(view)) {
return prev;
}
- prev.add(view);
- return new Set(prev);
+ const next = new Set(prev);
+ next.add(view);
+ return next;
});
}, []);
@@
if (!prev.has(view)) {
return prev;
}
- prev.delete(view);
- return new Set(prev);
+ const next = new Set(prev);
+ next.delete(view);
+ return next;
});📝 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.
| const registerView = useCallback((view: AvailableViews) => { | |
| if (viewsRef.current.has(view)) return; | |
| setViews((prev) => { | |
| if (prev.has(view)) { | |
| return prev; | |
| } | |
| prev.add(view); | |
| viewsRef.current.add(view); | |
| const filteredViews = [...viewsRef.current].filter(filter); | |
| setCurrentViews(getViewsSetStateAction(filteredViews)); | |
| return new Set(prev); | |
| }); | |
| }, []); | |
| const unregisterView = useCallback((view: AvailableViews) => { | |
| if (!viewsRef.current.has(view)) return; | |
| setViews((prev) => { | |
| if (!prev.has(view)) { | |
| return prev; | |
| } | |
| viewsRef.current.delete(view); | |
| const filteredViews = [...viewsRef.current].filter(filter); | |
| setCurrentViews(getViewsSetStateAction(filteredViews)); | |
| prev.delete(view); | |
| return new Set(prev); | |
| }); | |
| }, []); | |
| const registerView = useCallback((view: AvailableViews) => { | |
| setViews((prev) => { | |
| if (prev.has(view)) { | |
| return prev; | |
| } | |
| const next = new Set(prev); | |
| next.add(view); | |
| return next; | |
| }); | |
| }, []); | |
| const unregisterView = useCallback((view: AvailableViews) => { | |
| setViews((prev) => { | |
| if (!prev.has(view)) { | |
| return prev; | |
| } | |
| const next = new Set(prev); | |
| next.delete(view); | |
| return next; | |
| }); | |
| }, []); |
🤖 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 `@packages/ui-voip/src/providers/useAvailableViewTracker.ts` around lines 19 -
41, Update the state updaters in registerView and unregisterView to clone prev
before calling add or delete, preserving the early returns when the view is
already registered or absent. Return the cloned set only when the view actually
changes.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## ui-voip/refactorSessionState #41687 +/- ##
=============================================================
Coverage 68.63% 68.63%
=============================================================
Files 4164 4164
Lines 158962 158960 -2
Branches 28205 28172 -33
=============================================================
- Hits 109111 109110 -1
+ Misses 44668 44664 -4
- Partials 5183 5186 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
1 participant
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit