Match background notification volume to video player#1431
Match background notification volume to video player#1431TriumphantSam wants to merge 2 commits into
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
VideoDebatePlayer,updateVolumeFromPlayeris invoked on everyonProgresstick; if ReactPlayer or the underlying players provide a dedicated volume-change hook/event, consider using that instead or throttling the volume sync to avoid unnecessary work on frequent progress updates.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `VideoDebatePlayer`, `updateVolumeFromPlayer` is invoked on every `onProgress` tick; if ReactPlayer or the underlying players provide a dedicated volume-change hook/event, consider using that instead or throttling the volume sync to avoid unnecessary work on frequent progress updates.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d17693dada
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return null | ||
| } | ||
|
|
||
| const normalizedVolume = numericVolume > 1 ? numericVolume / 100 : numericVolume |
There was a problem hiding this comment.
normalizePlayerVolume only converts values greater than 1 from a 0–100 scale, so a YouTube getVolume() result of 1 is interpreted as already normalized and becomes 1.0. In that case, background notifications play at full volume even though the player is set to 1%, which breaks the intended volume matching behavior.
Useful? React with 👍 / 👎.
| if (typeof internalPlayer.getVolume === 'function') { | ||
| return normalizePlayerVolume(internalPlayer.getVolume()) |
There was a problem hiding this comment.
getReactPlayerVolume returns numeric volume directly from getVolume()/volume without checking mute state. For both YouTube and HTML media players, mute is tracked separately from the numeric volume, so a muted player can still report a non-zero volume; this makes background notification sounds audible while the video is muted.
Useful? React with 👍 / 👎.
1 participant
Summary
getVolume()values and HTML mediavolumevalues.Fixes CaptainFact/captain-fact#93.
Verification
npx --yes prettier@3.5.3 --check ...git diff --checkplayer_volumeNote: Full Jest/lint could not run locally because
npm ci --ignore-scriptstimed out before creating local npm binaries.