feat: Media call - generated DTMF tone sounds#37074
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 |
🦋 Changeset detectedLatest commit: 9d38ddc 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 |
WalkthroughAdds a Web Audio–based DTMF-like tone playback hook and integrates it into the VoIP dialpad: new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Keypad as Keypad UI
participant Provider as MediaCallProvider
participant Hook as useTonePlayer
participant Engine as TonePlayer (Web Audio)
participant Audio as HTMLAudioElement / Sink
User->>Keypad: Press digit
Keypad->>Provider: onTone(digit)
Provider->>Provider: isValidTone(digit)?
alt Valid tone
Provider->>Hook: playTone(digit)
Hook->>Engine: play(freqHi, freqLo, duration)
Engine->>Audio: setSinkId? / ensure playback
Audio-->>User: Sound via selected audioOutput
Engine-->>Engine: stop & disconnect oscillators after duration
else Invalid tone
Provider-->>Keypad: ignore playback
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37074 +/- ##
===========================================
+ Coverage 67.63% 67.65% +0.01%
===========================================
Files 3341 3339 -2
Lines 114016 113920 -96
Branches 20667 20616 -51
===========================================
- Hits 77120 77073 -47
+ Misses 34218 34176 -42
+ Partials 2678 2671 -7
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
.changeset/plenty-tips-care.md (1)
5-5: Clarify changelog wording for searchability.Spell out “DTMF”/“keypad” so folks can find it via changelog grep.
-Introduces audio feedback for the Voice Call Dialpad. +Introduces DTMF keypad audio feedback for the Voice Call Dialpad.packages/ui-voip/src/v2/MediaCallProvider.tsx (1)
184-189: Type‑safe call; drop theanycast.
isValidTonealready narrowstoneto the correct union. Pass it directly.- if (isValidTone(tone)) { - playTone(tone as any); - } + if (isValidTone(tone)) { + playTone(tone); + }packages/ui-voip/src/v2/components/Keypad/Keypad.stories.tsx (1)
12-15: Removeanyand (optionally) validate keypress.Keep the story type‑safe and mirror runtime behavior.
-import { useTonePlayer } from '../../useTonePlayer'; +import { useTonePlayer, isValidTone } from '../../useTonePlayer'; @@ -export const KeypadStoryWithTone: StoryFn<typeof Keypad> = () => { - const playTone = useTonePlayer(); - return <Keypad onKeyPress={(key) => playTone(key as any)} />; -}; +export const KeypadStoryWithTone: StoryFn<typeof Keypad> = () => { + const playTone = useTonePlayer(); + return <Keypad onKeyPress={(key) => isValidTone(key) && playTone(key)} />; +};packages/ui-voip/src/v2/useTonePlayer.ts (4)
50-73: Harden playback: resume context, de‑click envelope, and avoid overlapping timers.Improves reliability on browsers that auto‑suspend AudioContext; reduces clicks; prevents overlap artifacts.
- public play(highFreq: number, lowFreq: number, durationMs?: number) { + private stopTimer?: number; + + public play(highFreq: number, lowFreq: number, durationMs?: number) { + // Resume if suspended (common on first user gesture) + if (this.audioContext.state === 'suspended') { + void this.audioContext.resume().catch(() => {}); + } + // Stop any previous scheduled stop to avoid overlap artifacts + if (this.stopTimer) { + clearTimeout(this.stopTimer); + this.stopTimer = undefined; + } const highFrequencyOscillator = TonePlayer.setupOscillator(this.audioContext, this.gainNode); const lowFrequencyOscillator = TonePlayer.setupOscillator(this.audioContext, this.gainNode); @@ - lowFrequencyOscillator.start(); - highFrequencyOscillator.start(); + const now = this.audioContext.currentTime; + // Simple attack to avoid clicks + this.gainNode.gain.cancelScheduledValues(now); + this.gainNode.gain.setValueAtTime(0.0, now); + this.gainNode.gain.linearRampToValueAtTime(0.5, now + 0.01); + lowFrequencyOscillator.start(now); + highFrequencyOscillator.start(now); @@ - setTimeout(() => { + this.stopTimer = window.setTimeout(() => { + // Simple release to avoid clicks + const t = this.audioContext.currentTime; + this.gainNode.gain.cancelScheduledValues(t); + this.gainNode.gain.setValueAtTime(this.gainNode.gain.value, t); + this.gainNode.gain.linearRampToValueAtTime(0.0, t + 0.01); lowFrequencyOscillator.stop(); highFrequencyOscillator.stop(); highFrequencyOscillator.disconnect(); lowFrequencyOscillator.disconnect(); - }, durationMs ?? 400); + this.stopTimer = undefined; + }, durationMs ?? 400);
75-79: Clear pending timer and disconnect graph on destroy.Avoids stray timeouts after teardown.
public destroy() { + if (this.stopTimer) { + clearTimeout(this.stopTimer); + this.stopTimer = undefined; + } this.audioContext.close(); this.audioElement.pause(); this.audioElement.srcObject = null; }
97-99: Micro‑optimization and clearer type guard.Use the
inoperator to narrow without allocating keys array.-export const isValidTone = (tone: string): tone is keyof typeof DIGIT_TONE_MAP => { - return Object.keys(DIGIT_TONE_MAP).includes(tone); -}; +export const isValidTone = (tone: string): tone is keyof typeof DIGIT_TONE_MAP => tone in DIGIT_TONE_MAP;
15-17: Optional: Safari fallback for older engines.If you need to support legacy WebKit, consider
webkitAudioContext.- this.audioContext = new AudioContext(); + const Ctor = (window as any).AudioContext || (window as any).webkitAudioContext; + this.audioContext = new Ctor();
📜 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 (4)
.changeset/plenty-tips-care.md(1 hunks)packages/ui-voip/src/v2/MediaCallProvider.tsx(3 hunks)packages/ui-voip/src/v2/components/Keypad/Keypad.stories.tsx(1 hunks)packages/ui-voip/src/v2/useTonePlayer.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/ui-voip/src/v2/MediaCallProvider.tsx (1)
packages/ui-voip/src/v2/useTonePlayer.ts (2)
useTonePlayer(101-126)isValidTone(97-99)
packages/ui-voip/src/v2/components/Keypad/Keypad.stories.tsx (1)
packages/ui-voip/src/v2/useTonePlayer.ts (1)
useTonePlayer(101-126)
⏰ 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)
- 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
🔇 Additional comments (1)
packages/ui-voip/src/v2/MediaCallProvider.tsx (1)
184-189: Validate before sending to remote?Do we intend to send non‑DTMF tokens upstream? If not, guard
session.sendTonewith the same validator to keep local/remote behavior consistent. If yes (e.g., supporting A–D), consider extendingDIGIT_TONE_MAPand validator.- const onTone = (tone: string) => { - session.sendTone(tone); - if (isValidTone(tone)) { - playTone(tone); - } - }; + const onTone = (tone: string) => { + if (isValidTone(tone)) { + session.sendTone(tone); + playTone(tone); + } else { + session.sendTone(tone); // keep if remote expects more tokens; remove otherwise + } + };
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/ui-voip/src/v2/MediaCallProvider.tsx (1)
184-189: Validate tones before sending
session.sendTone (in useMediaSession.ts) calls mainCall.sendDTMF(tone) directly and only catches errors to log them. To prevent unnecessary console errors or unintended DTMF behavior for invalid inputs, apply the same isValidTone guard before invoking session.sendTone.
📜 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 (1)
packages/ui-voip/src/v2/MediaCallProvider.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/ui-voip/src/v2/MediaCallProvider.tsx (1)
packages/ui-voip/src/v2/useTonePlayer.ts (2)
useTonePlayer(101-126)isValidTone(97-99)
⏰ 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: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
packages/ui-voip/src/v2/MediaCallProvider.tsx (3)
24-24: LGTM!The import correctly brings in the tone validation and playback hook from the new
useTonePlayermodule.
44-44: LGTM!Adding
audioOutputto the destructured values enables the tone player to route audio to the selected output device.
182-183: LGTM!The tone player hook is correctly initialized with the selected audio output device ID, with proper handling of undefined values.
gabriellsh
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
3 participants
Proposed changes (including videos or screenshots)
Issue(s)
VGA-8
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Chores