chore: Implement voice call dtmf tone recorder on storybook#40302
Open
gabriellsh wants to merge 1 commit into
Open
chore: Implement voice call dtmf tone recorder on storybook#40302gabriellsh wants to merge 1 commit into
gabriellsh wants to merge 1 commit into
Conversation
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 |
|
Contributor
WalkthroughThis pull request extends the tone player functionality by exposing previously internal APIs, introducing a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Component as Keypad Component
participant Hook as useTonePlayerRecorder
participant Recorder as TonePlayerRecorder
participant Player as TonePlayer
participant AudioContext as Web Audio API
participant MediaRecorder as MediaRecorder
participant Browser as Browser
User->>Component: Press Key
Component->>Hook: recordAndDownloadTone(digit)
Hook->>Recorder: recordTone(highFreq, lowFreq, 250ms)
Recorder->>MediaRecorder: start()
Recorder->>Player: playTone(highFreq, lowFreq)
Player->>AudioContext: Generate & play tone
AudioContext->>MediaRecorder: Capture audio stream
Note over Recorder: Wait 250ms + flush buffer
Recorder->>MediaRecorder: stop()
MediaRecorder->>Recorder: Return Blob
Recorder-->>Hook: Return Blob
Hook->>Hook: Create object URL from Blob
Hook->>Browser: Trigger download (tone-{digit}.webm)
Hook->>Browser: Revoke object URL
Browser->>User: Download Complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![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: 2
🧹 Nitpick comments (2)
packages/ui-voip/src/hooks/useTonePlayerRecorder.ts (2)
54-55: Drop the inline comment to match repo conventions.As per coding guidelines ("Avoid code comments in the implementation"), the explanatory comment should be removed. The code is self-explanatory with a descriptive variable/context.
♻️ Proposed change
this.startRecording(); this.play(highFreq, lowFreq, durationMs); - // Wait for the tone to finish plus a small buffer so the encoder flushes await new Promise<void>((resolve) => setTimeout(resolve, durationMs + 100));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-voip/src/hooks/useTonePlayerRecorder.ts` around lines 54 - 55, Remove the inline explanatory comment before the await in useTonePlayerRecorder; the code "await new Promise<void>((resolve) => setTimeout(resolve, durationMs + 100));" is self-explanatory given the descriptive variable names (durationMs) and the surrounding context, so delete the comment line only and leave the await statement and promise intact.
84-92:playToneis duplicated fromuseTonePlayer.The
playTonecallback here is identical to the one inuseTonePlayer.ts(lines 113–121). SinceTonePlayerRecorder extends TonePlayer, you could compose by callinguseTonePlayerinternally, or extract a shared helper, to avoid drift if the tone duration/mapping changes in one place but not the other. Low priority given this hook is only used by a dev-only Storybook story.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-voip/src/hooks/useTonePlayerRecorder.ts` around lines 84 - 92, playTone in useTonePlayerRecorder is duplicated from useTonePlayer; extract the shared behavior so updates to DIGIT_TONE_MAP or duration don’t drift. Refactor by having useTonePlayerRecorder call useTonePlayer and reuse its playTone callback (or move the play logic into a small shared helper referenced by both hooks), keeping references to tonePlayer.current.play(DIGIT_TONE_MAP[digit][0], DIGIT_TONE_MAP[digit][1], 250) and the DIGIT_TONE_MAP symbol intact so the existing play behavior is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ui-voip/src/hooks/useTonePlayerRecorder.ts`:
- Around line 60-67: destroy() nulling mediaRecorder and clearing
recordingChunks while a stopRecording() promise is pending causes the onstop
handler to resolve with an empty Blob; instead either reject the pending stop
promise in destroy or defer chunk cleanup to the onstop handler. Update
destroy() to detect any in-flight stopRecording() promise (store its
resolve/reject when created in stopRecording()), and if present call its
reject(...) with an appropriate error before nulling mediaRecorder and
optionally leave recordingChunks cleanup to the existing onstop handler;
reference the methods/fields mediaRecorder, recordingChunks, stopRecording(),
destroy(), and the onstop handler registered around line 34 to locate and modify
the logic.
- Around line 113-119: The download revocation currently calls
URL.revokeObjectURL(url) immediately after anchor.click(), which can race with
the browser's async download; update the logic in useTonePlayerRecorder so the
blob URL (url) is revoked only after giving the browser time to start reading it
(e.g., schedule revocation via setTimeout with a short delay or listen for a
reliable completion event), and ensure the anchor element is cleaned up
afterward; adjust the code around the variables url and anchor in
useTonePlayerRecorder to defer revocation rather than calling
URL.revokeObjectURL(url) synchronously after anchor.click().
---
Nitpick comments:
In `@packages/ui-voip/src/hooks/useTonePlayerRecorder.ts`:
- Around line 54-55: Remove the inline explanatory comment before the await in
useTonePlayerRecorder; the code "await new Promise<void>((resolve) =>
setTimeout(resolve, durationMs + 100));" is self-explanatory given the
descriptive variable names (durationMs) and the surrounding context, so delete
the comment line only and leave the await statement and promise intact.
- Around line 84-92: playTone in useTonePlayerRecorder is duplicated from
useTonePlayer; extract the shared behavior so updates to DIGIT_TONE_MAP or
duration don’t drift. Refactor by having useTonePlayerRecorder call
useTonePlayer and reuse its playTone callback (or move the play logic into a
small shared helper referenced by both hooks), keeping references to
tonePlayer.current.play(DIGIT_TONE_MAP[digit][0], DIGIT_TONE_MAP[digit][1], 250)
and the DIGIT_TONE_MAP symbol intact so the existing play behavior is preserved.
🪄 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
Run ID: a7f94dba-f008-4639-89c3-f2a05eaeb1a5
📒 Files selected for processing (3)
packages/ui-voip/src/components/Keypad/Keypad.stories.tsxpackages/ui-voip/src/hooks/useTonePlayer.tspackages/ui-voip/src/hooks/useTonePlayerRecorder.ts
📜 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)
- 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:
packages/ui-voip/src/components/Keypad/Keypad.stories.tsxpackages/ui-voip/src/hooks/useTonePlayer.tspackages/ui-voip/src/hooks/useTonePlayerRecorder.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37328
File: packages/ui-voip/src/v2/useTonePlayer.ts:29-30
Timestamp: 2025-10-28T19:39:58.182Z
Learning: In packages/ui-voip/src/v2/useTonePlayer.ts, the DTMF tones generated by TonePlayer are for local auditory feedback to the user only. The actual DTMF signals are transmitted separately through the WebRTC channel. Therefore, filter configurations should prioritize user comfort and audio quality over technical DTMF frequency accuracy.
📚 Learning: 2025-10-28T19:39:58.182Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37328
File: packages/ui-voip/src/v2/useTonePlayer.ts:29-30
Timestamp: 2025-10-28T19:39:58.182Z
Learning: In packages/ui-voip/src/v2/useTonePlayer.ts, the DTMF tones generated by TonePlayer are for local auditory feedback to the user only. The actual DTMF signals are transmitted separately through the WebRTC channel. Therefore, filter configurations should prioritize user comfort and audio quality over technical DTMF frequency accuracy.
Applied to files:
packages/ui-voip/src/components/Keypad/Keypad.stories.tsxpackages/ui-voip/src/hooks/useTonePlayer.tspackages/ui-voip/src/hooks/useTonePlayerRecorder.ts
📚 Learning: 2026-02-26T19:22:29.385Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/views/CallHistoryContextualbar/CallHistoryActions.tsx:40-40
Timestamp: 2026-02-26T19:22:29.385Z
Learning: For TSX files in the UI VOIP package, ensure that when a media session state is 'unavailable', the voiceCall action is excluded from the actions object passed to CallHistoryActions so it does not render in the menu. This filtering should occur upstream (before getItems is called) to avoid tooltips or UI hints for unavailable actions. If there are multiple actions with availability states, implement a centralized helper to filter actions based on session state.
Applied to files:
packages/ui-voip/src/components/Keypad/Keypad.stories.tsx
📚 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:
packages/ui-voip/src/components/Keypad/Keypad.stories.tsx
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/ui-voip/src/hooks/useTonePlayer.tspackages/ui-voip/src/hooks/useTonePlayerRecorder.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/ui-voip/src/hooks/useTonePlayer.tspackages/ui-voip/src/hooks/useTonePlayerRecorder.ts
Comment on lines
+60
to
+67
| public override destroy() { | ||
| if (this.mediaRecorder && this.mediaRecorder.state !== 'inactive') { | ||
| this.mediaRecorder.stop(); | ||
| this.mediaRecorder = null; | ||
| this.recordingChunks = []; | ||
| } | ||
| super.destroy(); | ||
| } |
Contributor
There was a problem hiding this comment.
destroy() while a stopRecording() promise is pending will leave it resolving with an empty blob.
If stopRecording() is awaited and destroy() runs before the recorder's onstop fires (e.g. unmount during recording), the handler registered on line 34 still fires, but by then mediaRecorder is nulled and recordingChunks is emptied, so the consumer gets an empty Blob instead of an error. For a developer-only tool this is very unlikely to matter, but consider rejecting the pending promise or leaving chunk cleanup to the onstop handler.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui-voip/src/hooks/useTonePlayerRecorder.ts` around lines 60 - 67,
destroy() nulling mediaRecorder and clearing recordingChunks while a
stopRecording() promise is pending causes the onstop handler to resolve with an
empty Blob; instead either reject the pending stop promise in destroy or defer
chunk cleanup to the onstop handler. Update destroy() to detect any in-flight
stopRecording() promise (store its resolve/reject when created in
stopRecording()), and if present call its reject(...) with an appropriate error
before nulling mediaRecorder and optionally leave recordingChunks cleanup to the
existing onstop handler; reference the methods/fields mediaRecorder,
recordingChunks, stopRecording(), destroy(), and the onstop handler registered
around line 34 to locate and modify the logic.
Comment on lines
+113
to
+119
| const url = URL.createObjectURL(blob); | ||
| const anchor = document.createElement('a'); | ||
| anchor.href = url; | ||
| anchor.download = `tone-${digit}.webm`; | ||
| anchor.click(); | ||
| URL.revokeObjectURL(url); | ||
| }, []); |
Contributor
There was a problem hiding this comment.
Revoking the object URL immediately after click() can race with the download in some browsers.
anchor.click() kicks off the download asynchronously, and calling URL.revokeObjectURL(url) on the very next line can invalidate the blob before the browser begins reading it, leading to intermittently failed/empty downloads (notably in Firefox and some Chromium versions). Since this is a dev-only utility the impact is low, but a safer pattern is to defer revocation:
♻️ Suggested fix
const url = URL.createObjectURL(blob);
const anchor = document.createElement('a');
anchor.href = url;
anchor.download = `tone-${digit}.webm`;
anchor.click();
- URL.revokeObjectURL(url);
+ setTimeout(() => URL.revokeObjectURL(url), 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui-voip/src/hooks/useTonePlayerRecorder.ts` around lines 113 - 119,
The download revocation currently calls URL.revokeObjectURL(url) immediately
after anchor.click(), which can race with the browser's async download; update
the logic in useTonePlayerRecorder so the blob URL (url) is revoked only after
giving the browser time to start reading it (e.g., schedule revocation via
setTimeout with a short delay or listen for a reliable completion event), and
ensure the anchor element is cleaned up afterward; adjust the code around the
variables url and anchor in useTonePlayerRecorder to defer revocation rather
than calling URL.revokeObjectURL(url) synchronously after anchor.click().
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #40302 +/- ##
===========================================
- Coverage 69.80% 69.77% -0.03%
===========================================
Files 3296 3297 +1
Lines 119173 119285 +112
Branches 21435 21465 +30
===========================================
+ Hits 83183 83231 +48
- Misses 32684 32751 +67
+ Partials 3306 3303 -3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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)
Important
This is meant to be used as a tool by developers, and does not change any of the original logic.
This PR does not affect end-users in any shape or form.
The sounds generated by
useTonePlayerneeded to be exported as files. In order to do it the current implementation needed expansion for including aMediaRecorder, and some other changes to allow for downloading files were needed in the hook aswell.Since this took a little bit of effort, and does not change any of the current logic, I decided to add it permanently to storybook through this PR. In case we ever change the tones, regenerating the assets should be straightforward by simply running the storybook.
Issue(s)
VMUX-87
Steps to test or reproduce
Nothing needs to be tested. This change is exclusive to storybook and does not modify the original implementation.
Further comments
Summary by CodeRabbit