fix: Add cleanup for VideoConfManager listener in VideoConfProvider#38930
fix: Add cleanup for VideoConfManager listener in VideoConfProvider#38930Makeepan-dev wants to merge 2 commits intoRocketChat:developfrom
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 |
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ 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). (2)
WalkthroughFixed a memory leak in VideoConfProvider by consolidating the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/client/providers/VideoConfProvider.tsx (1)
38-47: Inconsistent cleanup pattern — align with established style or fix formatting.The
.off()method is valid onVideoConfManager, so this code works correctly. However, the rest of the file uses the pattern of returning the unsubscribe function directly from.on()(lines 21–27 and 29–36). This new block breaks that consistency with explicit.off()calls. Additionally, line 39 has a spacing issue (handleStop =()should behandleStop = () =>).For consistency with the rest of the file, capture the return values from
.on()and call them in cleanup:♻️ Refactor to match established pattern
useEffect(() => { - const handleStop =() => setOutgoing(undefined); - VideoConfManager.on('direct/stopped', handleStop); - VideoConfManager.on('calling/ended', handleStop); - - return () => { - VideoConfManager.off('direct/stopped', handleStop); - VideoConfManager.off('calling/ended', handleStop); - } + const handleStop = () => setOutgoing(undefined); + const offStop = VideoConfManager.on('direct/stopped', handleStop); + const offEnded = VideoConfManager.on('calling/ended', handleStop); + return () => { + offStop(); + offEnded(); + }; }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/providers/VideoConfProvider.tsx` around lines 38 - 47, The handler declaration has a spacing typo and the cleanup should follow the established unsubscribe-return pattern: change "const handleStop =() => setOutgoing(undefined);" to "const handleStop = () => setOutgoing(undefined);", store the unsubscribe functions returned by VideoConfManager.on('direct/stopped', handleStop) and VideoConfManager.on('calling/ended', handleStop) into variables (e.g., unsubscribeDirect and unsubscribeCalling) and call those functions in the effect cleanup instead of calling VideoConfManager.off; reference handleStop, VideoConfManager.on, and setOutgoing to locate the code to update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/client/providers/VideoConfProvider.tsx`:
- Around line 38-47: The handler declaration has a spacing typo and the cleanup
should follow the established unsubscribe-return pattern: change "const
handleStop =() => setOutgoing(undefined);" to "const handleStop = () =>
setOutgoing(undefined);", store the unsubscribe functions returned by
VideoConfManager.on('direct/stopped', handleStop) and
VideoConfManager.on('calling/ended', handleStop) into variables (e.g.,
unsubscribeDirect and unsubscribeCalling) and call those functions in the effect
cleanup instead of calling VideoConfManager.off; reference handleStop,
VideoConfManager.on, and setOutgoing to locate the code to update.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
apps/meteor/client/providers/VideoConfProvider.tsx
📜 Review details
⏰ 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: cubic · AI code reviewer
- 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:
apps/meteor/client/providers/VideoConfProvider.tsx
🧠 Learnings (1)
📚 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:
apps/meteor/client/providers/VideoConfProvider.tsx
Makeepan-dev
force-pushed
the
develop
branch
from
f9678c1 to
e2814d3
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38930 +/- ##
===========================================
- Coverage 70.66% 70.65% -0.01%
===========================================
Files 3189 3189
Lines 112715 112713 -2
Branches 20397 20460 +63
===========================================
- Hits 79652 79641 -11
- Misses 31015 31021 +6
- Partials 2048 2051 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
2 participants
Proposed changes (including videos or screenshots)
While looking through the VideoConfProvider component, I noticed that we were setting up event listeners for direct/stopped and calling/ended inside a useEffect hook, but we weren't actually cleaning them up when the component unmounted.
Issue(s)
Closes #38895
Steps to test or reproduce
Start a video call or a direct call in a local workspace.
End the call or navigate away from the provider to trigger an unmount.
If you use a memory profiler or add temporary logs to the VideoConfManager, you'll see that without this change, the listeners stay attached. With this fix, they’re correctly cleared out as soon as the component unmounts.
Further comments
This was tested on a local build after working through some environment setup with Deno 2.6.10 and Yarn 4. The monorepo builds successfully with turbo.
Summary by CodeRabbit