fix: formatted volume out of bounds of 0 and 1 #37041
Merged
Conversation
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
Contributor
|
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 |
🦋 Changeset detectedLatest commit: ed5567f The changes in this PR will be included in the next version bump. This PR includes changesets to release 42 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
WalkthroughExtracts formatVolume into its own utility with clamping and normalization (0–100 → 0–1), adds unit tests, introduces a lib index that re-exports helpers and formatVolume, updates CustomSoundProvider imports to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (6)
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 |
juliajforesti
force-pushed
the
fix/custom-sound-volume
branch
from
22b52f5 to
3924036
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37041 +/- ##
===========================================
+ Coverage 67.37% 67.41% +0.03%
===========================================
Files 3331 3330 -1
Lines 113519 113381 -138
Branches 20599 20710 +111
===========================================
- Hits 76488 76434 -54
+ Misses 34424 34345 -79
+ Partials 2607 2602 -5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
.changeset/spicy-crabs-complain.md (1)
5-5: Tighten the note for clarity.Spell out both the clamp and normalize, and punctuate.
-Ensures the formatted volume value is kept between 0 and 1 +Clamp volume preference to 0–100% and normalize to 0–1 when applying to audio elements.apps/meteor/client/providers/CustomSoundProvider/lib/formatVolume.spec.ts (1)
1-19: Exercise the public API and broaden edge‑case coverage.
- Import via the lib entry point to test the public surface.
- Add boundary and typical decimal cases; Infinity cases already behave well with current code.
- If you adopt the non‑finite guard, add a NaN test (optional snippet below).
-import { formatVolume } from './formatVolume'; +import { formatVolume } from '.'; @@ describe('formatVolume', () => { @@ it('returns 0 if volume is -10', () => { expect(formatVolume(-10)).toBe(0); }); + + it('returns 0 if volume is 0', () => { + expect(formatVolume(0)).toBe(0); + }); + + it('normalizes common decimals (percent inputs)', () => { + expect(formatVolume(33)).toBe(0.33); + expect(formatVolume(66)).toBe(0.66); + }); + + it('handles Infinity and -Infinity', () => { + expect(formatVolume(Number.POSITIVE_INFINITY)).toBe(1); + expect(formatVolume(Number.NEGATIVE_INFINITY)).toBe(0); + }); });Optional (only if you implement the non‑finite guard suggested in formatVolume.ts):
it('defaults to 1 when volume is NaN', () => { // @ts-expect-error intentional invalid input for runtime guard expect(formatVolume(NaN)).toBe(1); });apps/meteor/client/providers/CustomSoundProvider/CustomSoundProvider.tsx (1)
7-7: Import path change LGTM.Re-export consolidation via ./lib is fine. Two small, optional follow-ups:
- Precompute normalized volumes to avoid repeated calls and ensure consistent rounding (if any):
const normalizedNotificationsVolume = useMemo(() => formatVolume(notificationsSoundVolume), [notificationsSoundVolume]); const normalizedVoipVolume = useMemo(() => formatVolume(voipRingerVolume), [voipRingerVolume]);Then use normalizedNotificationsVolume/normalizedVoipVolume in contextValue.
- Harden the play API against external misuse by clamping the provided volume (in case other callers pass arbitrary values):
const play = useEffectEvent((soundId: ICustomSound['_id'], { volume = 1, loop = false } = {}) => { // ... const vol = Math.max(0, Math.min(volume, 1)); audio.volume = vol; // ... });Please confirm no external consumer calls play(...) with raw percentages; if they do, the clamp above becomes important.
apps/meteor/client/providers/CustomSoundProvider/lib/index.ts (1)
1-2: Consolidated public API looks good.Star re-exports from helpers and formatVolume are fine. Consider updating nearby tests to import from '.' (see spec suggestion) to lock in this entry point.
apps/meteor/client/providers/CustomSoundProvider/lib/formatVolume.ts (1)
1-4: Simplify formatVolume: drop toPrecision, return full-resolution volume
- In
apps/meteor/client/providers/CustomSoundProvider/lib/formatVolume.ts, change:-export const formatVolume = (volume: number) => { - const clamped = Math.max(0, Math.min(volume, 100)); - return Number((clamped / 100).toPrecision(2)); -}; +export const formatVolume = (volume: number): number => { + const clamped = Math.max(0, Math.min(volume, 100)); + return clamped / 100; +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
.changeset/spicy-crabs-complain.md(1 hunks)apps/meteor/client/providers/CustomSoundProvider/CustomSoundProvider.tsx(1 hunks)apps/meteor/client/providers/CustomSoundProvider/lib/formatVolume.spec.ts(1 hunks)apps/meteor/client/providers/CustomSoundProvider/lib/formatVolume.ts(1 hunks)apps/meteor/client/providers/CustomSoundProvider/lib/helpers.ts(0 hunks)apps/meteor/client/providers/CustomSoundProvider/lib/index.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/meteor/client/providers/CustomSoundProvider/lib/helpers.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/client/providers/CustomSoundProvider/lib/formatVolume.spec.ts (1)
apps/meteor/client/providers/CustomSoundProvider/lib/formatVolume.ts (1)
formatVolume(1-4)
⏰ 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). (7)
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 📦 Meteor Build - coverage
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
dougfabris
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
ggazzo
pushed a commit
that referenced
this pull request
Oct 6, 2025
Merged
debdutdeb
pushed a commit
that referenced
this pull request
Oct 7, 2025
juliajforesti
added a commit
that referenced
this pull request
Oct 8, 2025
juliajforesti
added a commit
that referenced
this pull request
Oct 8, 2025
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Proposed changes (including videos or screenshots)
Ensures the formatted volume value is kept between 0 and 1
Issue(s)
Steps to test or reproduce
Further comments
CORE-1393
Summary by CodeRabbit