fix: Stop media player when message is not accessible - #41600
fix: Stop media player when message is not accessible#41600yash-rajpal wants to merge 3 commits into
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 |
|
Walkthrough
ChangesMedia playback invalidation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.7.0 #41600 +/- ##
=================================================
- Coverage 68.68% 68.39% -0.30%
=================================================
Files 4139 4139
Lines 159681 158374 -1307
Branches 28306 28362 +56
=================================================
- Hits 109677 108316 -1361
- Misses 44869 44915 +46
- Partials 5135 5143 +8
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
yash-rajpal
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
![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
🧹 Nitpick comments (1)
apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.tsx (1)
110-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove implementation comments.
As per path instructions,
**/*.{ts,tsx,js}should "Avoid code comments in the implementation". These two explanatory comments should be dropped (or turned into descriptive variable/effect names if the intent needs preserving).Also applies to: 140-140
🤖 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 `@apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.tsx` at line 110, Remove the explanatory implementation comments near the hard-delete handling and the corresponding code around the second referenced location in MediaPlayerProvider. Preserve the existing behavior; do not replace the comments unless intent must be represented through clearer descriptive names.Source: Path instructions
🤖 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 `@apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.tsx`:
- Around line 111-138: Update all three notification-listener effects in the
MediaPlayerProvider to attach whenever track is loaded, regardless of playback
state: remove the playing guard and remove playing from each effect’s dependency
array. Preserve the existing track, room/message ID validation, subscriptions,
cleanup, and close behavior.
---
Nitpick comments:
In `@apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.tsx`:
- Line 110: Remove the explanatory implementation comments near the hard-delete
handling and the corresponding code around the second referenced location in
MediaPlayerProvider. Preserve the existing behavior; do not replace the comments
unless intent must be represented through clearer descriptive names.
🪄 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 Plus
Run ID: d84c06fa-72b8-4a7e-bf00-dc97ae56ac84
📒 Files selected for processing (1)
apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Hacktron Security Check
- GitHub Check: 📦 Build Packages
- 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/MediaPlayerProvider/MediaPlayerProvider.tsx
🧠 Learnings (2)
📚 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:
apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.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:
apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.tsx
🔇 Additional comments (1)
apps/meteor/client/providers/MediaPlayerProvider/MediaPlayerProvider.tsx (1)
2-9: LGTM!Also applies to: 32-32, 107-108
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
gabriellsh
left a comment
gabriellsh
left a comment
There was a problem hiding this comment.
Technically, I don't think this is a regression. If you had access to a room/message, you could download the audio and have it forever.
Another point is, whenever you see a message that has a file, in most cases a temporary URL will be generated through the files endpoint that already checks for the message/room. That URL allows you to access the file until it expires, which can be a long time.
In summary, IDK if we should implement these checks this way. Whenever the access is properly removed, it will eventually disappear. I'll bring this discussion forward.
| useEffect(() => { | ||
| if (!track || !userId) { | ||
| return; | ||
| } | ||
|
|
||
| const { rid } = track; | ||
|
|
||
| if (!rid) { | ||
| return; | ||
| } | ||
|
|
||
| return subscribeToNotifyUser(`${userId}/subscriptions-changed`, (event, subscription) => { | ||
| if (event === 'removed' && subscription.rid === rid) { | ||
| close(); | ||
| } | ||
| }); | ||
| }, [userId, subscribeToNotifyUser, track, close]); |
There was a problem hiding this comment.
If you leave a public room, you might still have access to the message, so listening for subscription removal would stop playing a sound even if you still have access to the message.
Another scenario is, you're listening to an audio in a public room, without joining it, and loosing the permission preview-c-room.
| useEffect(() => { | ||
| if (!track) { | ||
| return; | ||
| } | ||
|
|
||
| const { rid, mid } = track; | ||
|
|
||
| if (!rid || !mid) { | ||
| return; | ||
| } | ||
|
|
||
| const unsubscribeFromDeleteMessage = subscribeToNotifyRoom(`${rid}/deleteMessage`, ({ _id }) => { | ||
| if (_id === mid) { | ||
| close(); | ||
| } | ||
| }); | ||
|
|
||
| const unsubscribeFromDeleteMessageBulk = subscribeToNotifyRoom(`${rid}/deleteMessageBulk`, ({ ids }) => { | ||
| if (ids?.includes(mid)) { | ||
| close(); | ||
| } | ||
| }); | ||
|
|
||
| return () => { | ||
| unsubscribeFromDeleteMessage(); | ||
| unsubscribeFromDeleteMessageBulk(); | ||
| }; | ||
| }, [track, subscribeToNotifyRoom, close]); | ||
|
|
||
| //For soft message delete, when Message_ShowDeletedStatus is on and server updates message.t to rm | ||
| useEffect(() => { | ||
| if (!track) { | ||
| return; | ||
| } | ||
|
|
||
| const unsub = Messages.use.subscribe((state, prevState) => { | ||
| const { mid } = track; | ||
| if (!mid) { | ||
| return; | ||
| } | ||
|
|
||
| const message = state.records.get(mid); | ||
|
|
||
| if (message && message !== prevState.records.get(mid) && message.t === 'rm') { | ||
| close(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
I'd like for this to at least be isolated in it's own hook.
dionisio-bot
Bot
removed
the
stat: ready to merge
yash-rajpal
changed the title
3 participants
Proposed changes (including videos or screenshots)
Recently we made a global audio player which didn't require the audio message to be mounted but this created issue that audio player should stop once the user no longer has access to the message, either by message being deleted or the user being no longer part of the room.
This PR implements this edge case by listening to the required stream events, it stops playing audio when user can no longer access the message.
Issue(s)
Steps to test or reproduce
Further comments
CORE-2484
Summary by CodeRabbit