feat: add Audio Peak Level VU Meter & Clipping Warning - #1159
Conversation
|
@hrshjswniii is attempting to deploy a commit to the itzzavdhesh's projects Team on Vercel. A member of the Team first needs to authorize it. |
✍️ DCO Sign-off NeededHey @hrshjswniii! 👋 One or more commits in this PR are missing a Warning
How to fix: For the latest commit: git commit --amend --signoff
git push --force-with-leaseFor multiple commits, replace git rebase --signoff HEAD~N
git push --force-with-leaseThis comment will update automatically after you push. 🤖 VoiceForge Automation · Updates automatically on edits |
github-actions
Bot
added
accessibility
assignee-match
client
enhancement
📝 WalkthroughWalkthroughAdds a real-time Web Audio peak meter with decibel display, clipping warnings, peak hold tracking, and optional labels. The meter is rendered in onboarding and voice synthesis settings, with a basic export test. ChangesAudio Peak Meter
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant VoiceSettings
participant PeakLevelMeter
participant AudioContext
participant AnalyserNode
VoiceSettings->>PeakLevelMeter: render active meter
PeakLevelMeter->>AudioContext: create audio analysis wiring
AudioContext->>AnalyserNode: provide time-domain samples
PeakLevelMeter->>AnalyserNode: read peak samples
PeakLevelMeter->>PeakLevelMeter: calculate dB and clipping state
PeakLevelMeter->>VoiceSettings: render level bar or clip warning
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
🎉 PR Ready for Mentor ReviewHey @hrshjswniii! 👋 Your PR passed all checks and is now in the GSSoC review queue. Note 🔗 Closing: #1137 · 📐 197 lines across 4 file(s) · 📬 Already requested or no eligible reviewer found @sabeenaviklar @Anushreebasics @itsdakshjain @snehkris @Mrigakshi-Rathore @Itzzavdheshh @Nitya-003 @4f4d @lovestaco, this PR is ready for your review — please confirm scope, check behavior and tests, then approve or request changes. Important This is not an approval. Please wait for mentor feedback before expecting a merge. If changes are requested, push them to this same branch and keep the PR focused on the linked issue. 🤖 VoiceForge Automation · Updates automatically on edits |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
client/src/components/PeakLevelMeter.test.js (1)
4-8: 📐 Maintainability & Code Quality | 🔵 TrivialMinimal test coverage for a fairly complex component.
The only assertion is that the export is a function. Given the dB conversion math, clipping-threshold logic, and peak-hold tracking in
PeakLevelMeter.jsx, consider adding tests (e.g., viatestLevelprop +@testing-library/react) asserting rendered dB text and the "CLIP WARNING!" badge at/above the clipping threshold.🤖 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 `@client/src/components/PeakLevelMeter.test.js` around lines 4 - 8, Add meaningful behavioral tests for the PeakLevelMeter component using the testLevel prop and `@testing-library/react`: verify the rendered dB conversion output and assert that the “CLIP WARNING!” badge appears at and above the clipping threshold. Retain the existing export test.client/src/components/PeakLevelMeter.jsx (2)
41-49: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
AudioContextcreated foraudioElementRefis never closed.The cleanup function only cancels the animation frame;
audioCtxRef.current(theAudioContextcreated at Line 43) is never closed on unmount or whenaudioElementRef/analyserNodechanges. Browsers cap the number of liveAudioContextinstances (e.g. Chrome enforces a hard limit), so repeated mount/connect cycles on this path can eventually throw or silently fail to create new contexts.♻️ Proposed fix
return () => { if (animationFrameRef.current) { cancelAnimationFrame(animationFrameRef.current); } + if (audioCtxRef.current) { + audioCtxRef.current.close().catch(() => {}); + audioCtxRef.current = null; + } };Also applies to: 100-106
🤖 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 `@client/src/components/PeakLevelMeter.jsx` around lines 41 - 49, Update the cleanup logic in the effect that creates the AudioContext and analyser so it closes audioCtxRef.current when the component unmounts or audioElementRef/analyserNode changes. Stop or disconnect the associated audio nodes as needed, then clear the ref after closing while preserving the existing animation-frame cancellation.
112-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
resetMaxPeakis defined but never called.No button/control in the rendered JSX invokes
resetMaxPeak, so the peak-hold indicator can only grow for the lifetime of the mounted component. Either wire this to a reset control (common on real VU meters) or remove the dead function.🤖 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 `@client/src/components/PeakLevelMeter.jsx` around lines 112 - 115, Update the peak meter component so resetMaxPeak is either invoked by a rendered reset control or removed if no reset behavior is needed; ensure the chosen implementation eliminates the unused function while preserving existing peak and clipping behavior.
🤖 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 `@client/src/components/PeakLevelMeter.jsx`:
- Around line 26-32: Align the clipping logic in the testLevel branch of
PeakLevelMeter with the live-sampling thresholds: treat test values at or above
-0.5 dB as clipping, while preserving the existing level clamping, peak
tracking, and live maxVal threshold behavior.
- Line 1: Remove the random level generation fallback in PeakLevelMeter and
require real audio input through analyserNode or audioElementRef; when isActive
is provided without either source, warn or explicitly mark the component as demo
mode instead of reporting fabricated levels. Update the Onboarding and Settings
integrations to pass the analyser or audio-element reference for their actual
TTS preview/test output while preserving active-state behavior.
- Around line 74-97: Update PeakLevelMeter’s updateMeter flow so normal isActive
usage never generates random levels without real audio; wire a real AnalyserNode
or audioElementRef into the Onboarding and Settings integrations, or restrict
the simulated fallback to an explicit demo/testLevel mode. Ensure the meter
samples actual audio through the analyser and preserves simulation only when
that mode is intentionally enabled.
In `@client/src/pages/Onboarding.jsx`:
- Around line 152-156: Update the onboarding audio section containing
PeakLevelMeter so it is not mounted with a hardcoded active state when no real
audio source is available. Pass the actual audio activity/source state used by
the onboarding flow, or conditionally render the meter only when that source
exists; preserve the surrounding clipping-warning layout.
In `@client/src/pages/Settings.jsx`:
- Around line 325-332: Update the PeakLevelMeter usage in the settings component
so it receives and monitors the actual audio source or playback state instead of
being mounted with a hardcoded isActive={true}; preserve the meter’s existing
rendering while ensuring activity reflects real audio.
---
Nitpick comments:
In `@client/src/components/PeakLevelMeter.jsx`:
- Around line 41-49: Update the cleanup logic in the effect that creates the
AudioContext and analyser so it closes audioCtxRef.current when the component
unmounts or audioElementRef/analyserNode changes. Stop or disconnect the
associated audio nodes as needed, then clear the ref after closing while
preserving the existing animation-frame cancellation.
- Around line 112-115: Update the peak meter component so resetMaxPeak is either
invoked by a rendered reset control or removed if no reset behavior is needed;
ensure the chosen implementation eliminates the unused function while preserving
existing peak and clipping behavior.
In `@client/src/components/PeakLevelMeter.test.js`:
- Around line 4-8: Add meaningful behavioral tests for the PeakLevelMeter
component using the testLevel prop and `@testing-library/react`: verify the
rendered dB conversion output and assert that the “CLIP WARNING!” badge appears
at and above the clipping threshold. Retain the existing export test.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 91846410-7fa0-4362-b50b-fdb7bb706d2f
📒 Files selected for processing (4)
client/src/components/PeakLevelMeter.jsxclient/src/components/PeakLevelMeter.test.jsclient/src/pages/Onboarding.jsxclient/src/pages/Settings.jsx
| @@ -0,0 +1,175 @@ | |||
| import React, { useEffect, useRef, useState } from "react"; | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
The "real-time audio peak meter" never samples real audio in either page it's integrated into. PeakLevelMeter only samples actual audio when given analyserNode or audioElementRef; otherwise, whenever isActive is true it synthesizes maxVal = 0.2 + Math.random() * 0.45 every animation frame. Both integration sites pass only isActive={true}, so the meter permanently displays fabricated levels unrelated to any synthesized speech, defeating the linked issue's goal of visually surfacing real clipping (notably for users who can't rely on hearing).
client/src/components/PeakLevelMeter.jsx#L74-97: this is the root cause — theelse if (isActive)random fallback should not silently stand in for real sampling; consider requiring/warning whenisActiveis set without a realanalyser, or clearly labeling the meter as "demo mode" in that case.client/src/pages/Onboarding.jsx#L152-156: pass a realanalyserNode/audioElementReftied to the TTS preview/test audio output instead of bareisActive={true}.client/src/pages/Settings.jsx#L325-332: same — wire this to the actual voice-synthesis preview audio element/analyser rather thanisActive={true}alone.
🤖 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 `@client/src/components/PeakLevelMeter.jsx` at line 1, Remove the random level
generation fallback in PeakLevelMeter and require real audio input through
analyserNode or audioElementRef; when isActive is provided without either
source, warn or explicitly mark the component as demo mode instead of reporting
fabricated levels. Update the Onboarding and Settings integrations to pass the
analyser or audio-element reference for their actual TTS preview/test output
while preserving active-state behavior.
| if (testLevel !== null) { | ||
| const level = Math.max(-60, Math.min(6, testLevel)); | ||
| setPeakDb(level); | ||
| setIsClipping(level >= 0); | ||
| setMaxPeakDb((prev) => Math.max(prev, level)); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clip threshold differs between testLevel mode and live sampling.
testLevel mode clips at level >= 0 (Line 29) while live sampling clips at db >= -0.5 || maxVal >= 0.95 (Line 93). Since testLevel is documented as "useful for tests and static demos," a demo value like -0.3 won't show the clip warning even though the same peak would trigger it in live mode, making demos/tests misrepresent production behavior.
Also applies to: 90-94
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 27-27: Avoid using the initial state variable in setState
Context: setPeakDb(level)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🤖 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 `@client/src/components/PeakLevelMeter.jsx` around lines 26 - 32, Align the
clipping logic in the testLevel branch of PeakLevelMeter with the live-sampling
thresholds: treat test values at or above -0.5 dB as clipping, while preserving
the existing level clamping, peak tracking, and live maxVal threshold behavior.
| if (analyser) { | ||
| analyser.getByteTimeDomainData(dataArray); | ||
| for (let i = 0; i < dataArray.length; i++) { | ||
| const sample = Math.abs((dataArray[i] - 128) / 128); | ||
| if (sample > maxVal) maxVal = sample; | ||
| } | ||
| } else if (isActive) { | ||
| // Simulated ambient meter activity when active without direct media stream | ||
| maxVal = 0.2 + Math.random() * 0.45; | ||
| } | ||
|
|
||
| // Convert amplitude (0 to 1) to Decibels (-60 dB to +3 dB) | ||
| let db = -60; | ||
| if (maxVal > 0.0001) { | ||
| db = 20 * Math.log10(maxVal); | ||
| } | ||
| db = Math.max(-60, Math.min(3, db)); | ||
|
|
||
| setPeakDb(db); | ||
| const clippingDetected = db >= -0.5 || maxVal >= 0.95; | ||
| setIsClipping(clippingDetected); | ||
| setMaxPeakDb((prev) => Math.max(prev, db)); | ||
|
|
||
| animationFrameRef.current = requestAnimationFrame(updateMeter); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Meter always shows simulated random data, never real audio, when only isActive is set.
When no analyserNode/audioElementRef is supplied (exactly how both Onboarding.jsx and Settings.jsx currently invoke this component), updateMeter falls into the else if (isActive) branch and fabricates maxVal = 0.2 + Math.random() * 0.45 every frame. The "VU Peak Level Meter" never actually samples any real audio in either integration — it's a random animation. This conflicts with the linked issue's requirement to "Sample audio peaks through a Web Audio AnalyserNode" and to give users (including those who can't rely on hearing) real visual feedback about actual clipping.
See consolidated comment for the cross-file fix (wiring a real AnalyserNode/audio element into both pages, or gating the simulated fallback behind an explicit demo/testLevel-only mode).
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 91-91: Avoid using the initial state variable in setState
Context: setPeakDb(db)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
[warning] 93-93: Avoid using the initial state variable in setState
Context: setIsClipping(clippingDetected)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🤖 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 `@client/src/components/PeakLevelMeter.jsx` around lines 74 - 97, Update
PeakLevelMeter’s updateMeter flow so normal isActive usage never generates
random levels without real audio; wire a real AnalyserNode or audioElementRef
into the Onboarding and Settings integrations, or restrict the simulated
fallback to an explicit demo/testLevel mode. Ensure the meter samples actual
audio through the analyser and preserves simulation only when that mode is
intentionally enabled.
| {/* Audio Peak Level VU Meter & Clipping Warning */} | ||
| <div className="mt-5 pt-3 border-t border-ink/10 dark:border-border"> | ||
| <PeakLevelMeter isActive={true} /> | ||
| </div> | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
Meter mounted with no real audio source (see consolidated comment).
🤖 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 `@client/src/pages/Onboarding.jsx` around lines 152 - 156, Update the
onboarding audio section containing PeakLevelMeter so it is not mounted with a
hardcoded active state when no real audio source is available. Pass the actual
audio activity/source state used by the onboarding flow, or conditionally render
the meter only when that source exists; preserve the surrounding
clipping-warning layout.
| <p className="text-xs text-ink/50 mt-1">Higher values exaggerate the style of the reference audio.</p> | ||
| </div> | ||
| </div> | ||
|
|
||
| {/* Audio Peak Level VU Meter & Clipping Warning */} | ||
| <div className="mt-5 pt-4 border-t border-ink/10 dark:border-border"> | ||
| <PeakLevelMeter isActive={true} /> | ||
| </div> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
Meter mounted with no real audio source (see consolidated comment).
🤖 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 `@client/src/pages/Settings.jsx` around lines 325 - 332, Update the
PeakLevelMeter usage in the settings component so it receives and monitors the
actual audio source or playback state instead of being mounted with a hardcoded
isActive={true}; preserve the meter’s existing rendering while ensuring activity
reflects real audio.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
10 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="client/src/components/PeakLevelMeter.test.js">
<violation number="1" location="client/src/components/PeakLevelMeter.test.js:6">
P2: The test only verifies that `PeakLevelMeter` exports as a function. For a component that manages Web Audio APIs, animated state, and clipping logic, this provides essentially no behavioral coverage. Consider adding render tests (with `@testing-library/react`) that verify the meter bar updates when `testLevel` changes, the clipping warning appears at expected thresholds, and the component handles mount/unmount without errors.</violation>
</file>
<file name="client/src/components/PeakLevelMeter.jsx">
<violation number="1" location="client/src/components/PeakLevelMeter.jsx:2">
P3: `Volume2` and `VolumeX` are unused imports, adding dead code and causing a no-unused-vars failure in configurations that lint this client. Removing them would keep the import aligned with the component.</violation>
<violation number="2" location="client/src/components/PeakLevelMeter.jsx:29">
P2: Clip threshold is inconsistent between `testLevel` mode and live sampling. Here, clipping triggers at `level >= 0`, but in the live path (line 93) it triggers at `db >= -0.5 || maxVal >= 0.95`. A `testLevel` value like `-0.3` won't show the clip warning even though the same peak would trigger it in the live path, making tests and demos misrepresent actual behavior.
Consider aligning the thresholds, e.g. `setIsClipping(level >= -0.5)`.</violation>
<violation number="3" location="client/src/components/PeakLevelMeter.jsx:40">
P2: Passing an audio element already managed by `AudioProcessor` attempts to create a second `MediaElementSource`, after which this component has no analyser and falls back to fake activity. Reusing the existing audio graph/analyser or sharing one source marker would avoid this integration failure.</violation>
<violation number="4" location="client/src/components/PeakLevelMeter.jsx:43">
P1: The AudioContext created in the `useEffect` is never closed when the component unmounts. The cleanup function only cancels the animation frame (`cancelAnimationFrame`) but does not call `audioCtxRef.current?.close()`. In React Strict Mode (dev), effects fire twice, which can create and leak two AudioContext instances — and browsers impose a hard limit (~6 per document) on open AudioContexts, so repeated mounts quickly exhaust them. Add `if (audioCtxRef.current) { audioCtxRef.current.close(); }` to the effect's cleanup function.</violation>
<violation number="5" location="client/src/components/PeakLevelMeter.jsx:43">
P2: An audio-element-backed meter can remain at silence when the context starts suspended because this effect never resumes it. Resuming from an allowed user/play event, or accepting a caller-owned running context/analyser, would make the optional audio path reliable.</violation>
<violation number="6" location="client/src/components/PeakLevelMeter.jsx:50">
P2: AudioNode references are stored directly on the DOM element via `_vfSource` and `_vfAnalyser` custom properties. This mutates DOM nodes outside React's control, which could conflict with React reconciliation if the element is replaced. These properties are also never cleaned up when the component unmounts, creating stale references. Consider managing these AudioNodes via refs scoped to the component instance instead.</violation>
<violation number="7" location="client/src/components/PeakLevelMeter.jsx:80">
P0: The VU meter shows simulated random data instead of actual audio levels when no audio source is connected. When `isActive={true}` is passed without an `audioElementRef` or `analyserNode` (as in both Onboarding.jsx and Settings.jsx), the `updateMeter` loop falls through to `maxVal = 0.2 + Math.random() * 0.45`, generating a bouncing meter that looks like real audio processing but is just random noise. Users tuning voice synthesis sliders will see a live-animated meter that does not reflect any actual volume, which is misleading and undermines the feature's purpose. Consider either wiring the meter to an actual audio element/stream, disabling the simulation path, or explicitly labeling the display as "simulated" when no audio source is connected.</violation>
<violation number="8" location="client/src/components/PeakLevelMeter.jsx:107">
P2: If the referenced media element is mounted or replaced after this effect runs, the meter never reconnects because the ref object remains unchanged; with `isActive`, it can stay on the fallback path indefinitely. Passing the element/analyser as reactive state or using a callback ref would track that lifecycle.</violation>
<violation number="9" location="client/src/components/PeakLevelMeter.jsx:112">
P3: `resetMaxPeak` is dead code, and no user-facing path can reset the max-peak hold despite the helper being present. Exposing a reset control/callback or removing the unused helper would keep the component behavior and API intentional.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const sample = Math.abs((dataArray[i] - 128) / 128); | ||
| if (sample > maxVal) maxVal = sample; | ||
| } | ||
| } else if (isActive) { |
There was a problem hiding this comment.
P0: The VU meter shows simulated random data instead of actual audio levels when no audio source is connected. When isActive={true} is passed without an audioElementRef or analyserNode (as in both Onboarding.jsx and Settings.jsx), the updateMeter loop falls through to maxVal = 0.2 + Math.random() * 0.45, generating a bouncing meter that looks like real audio processing but is just random noise. Users tuning voice synthesis sliders will see a live-animated meter that does not reflect any actual volume, which is misleading and undermines the feature's purpose. Consider either wiring the meter to an actual audio element/stream, disabling the simulation path, or explicitly labeling the display as "simulated" when no audio source is connected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At client/src/components/PeakLevelMeter.jsx, line 80:
<comment>The VU meter shows simulated random data instead of actual audio levels when no audio source is connected. When `isActive={true}` is passed without an `audioElementRef` or `analyserNode` (as in both Onboarding.jsx and Settings.jsx), the `updateMeter` loop falls through to `maxVal = 0.2 + Math.random() * 0.45`, generating a bouncing meter that looks like real audio processing but is just random noise. Users tuning voice synthesis sliders will see a live-animated meter that does not reflect any actual volume, which is misleading and undermines the feature's purpose. Consider either wiring the meter to an actual audio element/stream, disabling the simulation path, or explicitly labeling the display as "simulated" when no audio source is connected.</comment>
<file context>
@@ -0,0 +1,175 @@
+ const sample = Math.abs((dataArray[i] - 128) / 128);
+ if (sample > maxVal) maxVal = sample;
+ }
+ } else if (isActive) {
+ // Simulated ambient meter activity when active without direct media stream
+ maxVal = 0.2 + Math.random() * 0.45;
</file context>
| if (!audioEl._vfSource) { | ||
| const AudioContext = window.AudioContext || window.webkitAudioContext; | ||
| if (AudioContext) { | ||
| const ctx = new AudioContext(); |
There was a problem hiding this comment.
P1: The AudioContext created in the useEffect is never closed when the component unmounts. The cleanup function only cancels the animation frame (cancelAnimationFrame) but does not call audioCtxRef.current?.close(). In React Strict Mode (dev), effects fire twice, which can create and leak two AudioContext instances — and browsers impose a hard limit (~6 per document) on open AudioContexts, so repeated mounts quickly exhaust them. Add if (audioCtxRef.current) { audioCtxRef.current.close(); } to the effect's cleanup function.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At client/src/components/PeakLevelMeter.jsx, line 43:
<comment>The AudioContext created in the `useEffect` is never closed when the component unmounts. The cleanup function only cancels the animation frame (`cancelAnimationFrame`) but does not call `audioCtxRef.current?.close()`. In React Strict Mode (dev), effects fire twice, which can create and leak two AudioContext instances — and browsers impose a hard limit (~6 per document) on open AudioContexts, so repeated mounts quickly exhaust them. Add `if (audioCtxRef.current) { audioCtxRef.current.close(); }` to the effect's cleanup function.</comment>
<file context>
@@ -0,0 +1,175 @@
+ if (!audioEl._vfSource) {
+ const AudioContext = window.AudioContext || window.webkitAudioContext;
+ if (AudioContext) {
+ const ctx = new AudioContext();
+ audioCtxRef.current = ctx;
+ const source = ctx.createMediaElementSource(audioEl);
</file context>
|
|
||
| describe("PeakLevelMeter component", () => { | ||
| it("exports PeakLevelMeter function component", () => { | ||
| expect(typeof PeakLevelMeter).toBe("function"); |
There was a problem hiding this comment.
P2: The test only verifies that PeakLevelMeter exports as a function. For a component that manages Web Audio APIs, animated state, and clipping logic, this provides essentially no behavioral coverage. Consider adding render tests (with @testing-library/react) that verify the meter bar updates when testLevel changes, the clipping warning appears at expected thresholds, and the component handles mount/unmount without errors.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At client/src/components/PeakLevelMeter.test.js, line 6:
<comment>The test only verifies that `PeakLevelMeter` exports as a function. For a component that manages Web Audio APIs, animated state, and clipping logic, this provides essentially no behavioral coverage. Consider adding render tests (with `@testing-library/react`) that verify the meter bar updates when `testLevel` changes, the clipping warning appears at expected thresholds, and the component handles mount/unmount without errors.</comment>
<file context>
@@ -0,0 +1,8 @@
+
+describe("PeakLevelMeter component", () => {
+ it("exports PeakLevelMeter function component", () => {
+ expect(typeof PeakLevelMeter).toBe("function");
+ });
+});
</file context>
| if (!analyser && audioElementRef?.current) { | ||
| try { | ||
| const audioEl = audioElementRef.current; | ||
| if (!audioEl._vfSource) { |
There was a problem hiding this comment.
P2: Passing an audio element already managed by AudioProcessor attempts to create a second MediaElementSource, after which this component has no analyser and falls back to fake activity. Reusing the existing audio graph/analyser or sharing one source marker would avoid this integration failure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At client/src/components/PeakLevelMeter.jsx, line 40:
<comment>Passing an audio element already managed by `AudioProcessor` attempts to create a second `MediaElementSource`, after which this component has no analyser and falls back to fake activity. Reusing the existing audio graph/analyser or sharing one source marker would avoid this integration failure.</comment>
<file context>
@@ -0,0 +1,175 @@
+ if (!analyser && audioElementRef?.current) {
+ try {
+ const audioEl = audioElementRef.current;
+ if (!audioEl._vfSource) {
+ const AudioContext = window.AudioContext || window.webkitAudioContext;
+ if (AudioContext) {
</file context>
| cancelAnimationFrame(animationFrameRef.current); | ||
| } | ||
| }; | ||
| }, [audioElementRef, analyserNode, isActive, testLevel]); |
There was a problem hiding this comment.
P2: If the referenced media element is mounted or replaced after this effect runs, the meter never reconnects because the ref object remains unchanged; with isActive, it can stay on the fallback path indefinitely. Passing the element/analyser as reactive state or using a callback ref would track that lifecycle.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At client/src/components/PeakLevelMeter.jsx, line 107:
<comment>If the referenced media element is mounted or replaced after this effect runs, the meter never reconnects because the ref object remains unchanged; with `isActive`, it can stay on the fallback path indefinitely. Passing the element/analyser as reactive state or using a callback ref would track that lifecycle.</comment>
<file context>
@@ -0,0 +1,175 @@
+ cancelAnimationFrame(animationFrameRef.current);
+ }
+ };
+ }, [audioElementRef, analyserNode, isActive, testLevel]);
+
+ // Map peakDb (-60dB to 0dB) to a percentage (0% to 100%)
</file context>
| node.fftSize = 256; | ||
| source.connect(node); | ||
| node.connect(ctx.destination); | ||
| audioEl._vfSource = source; |
There was a problem hiding this comment.
P2: AudioNode references are stored directly on the DOM element via _vfSource and _vfAnalyser custom properties. This mutates DOM nodes outside React's control, which could conflict with React reconciliation if the element is replaced. These properties are also never cleaned up when the component unmounts, creating stale references. Consider managing these AudioNodes via refs scoped to the component instance instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At client/src/components/PeakLevelMeter.jsx, line 50:
<comment>AudioNode references are stored directly on the DOM element via `_vfSource` and `_vfAnalyser` custom properties. This mutates DOM nodes outside React's control, which could conflict with React reconciliation if the element is replaced. These properties are also never cleaned up when the component unmounts, creating stale references. Consider managing these AudioNodes via refs scoped to the component instance instead.</comment>
<file context>
@@ -0,0 +1,175 @@
+ node.fftSize = 256;
+ source.connect(node);
+ node.connect(ctx.destination);
+ audioEl._vfSource = source;
+ audioEl._vfAnalyser = node;
+ }
</file context>
| if (testLevel !== null) { | ||
| const level = Math.max(-60, Math.min(6, testLevel)); | ||
| setPeakDb(level); | ||
| setIsClipping(level >= 0); |
There was a problem hiding this comment.
P2: Clip threshold is inconsistent between testLevel mode and live sampling. Here, clipping triggers at level >= 0, but in the live path (line 93) it triggers at db >= -0.5 || maxVal >= 0.95. A testLevel value like -0.3 won't show the clip warning even though the same peak would trigger it in the live path, making tests and demos misrepresent actual behavior.
Consider aligning the thresholds, e.g. setIsClipping(level >= -0.5).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At client/src/components/PeakLevelMeter.jsx, line 29:
<comment>Clip threshold is inconsistent between `testLevel` mode and live sampling. Here, clipping triggers at `level >= 0`, but in the live path (line 93) it triggers at `db >= -0.5 || maxVal >= 0.95`. A `testLevel` value like `-0.3` won't show the clip warning even though the same peak would trigger it in the live path, making tests and demos misrepresent actual behavior.
Consider aligning the thresholds, e.g. `setIsClipping(level >= -0.5)`.</comment>
<file context>
@@ -0,0 +1,175 @@
+ if (testLevel !== null) {
+ const level = Math.max(-60, Math.min(6, testLevel));
+ setPeakDb(level);
+ setIsClipping(level >= 0);
+ setMaxPeakDb((prev) => Math.max(prev, level));
+ return;
</file context>
| @@ -0,0 +1,175 @@ | |||
| import React, { useEffect, useRef, useState } from "react"; | |||
| import { Volume2, VolumeX, AlertTriangle, Activity } from "lucide-react"; | |||
There was a problem hiding this comment.
P3: Volume2 and VolumeX are unused imports, adding dead code and causing a no-unused-vars failure in configurations that lint this client. Removing them would keep the import aligned with the component.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At client/src/components/PeakLevelMeter.jsx, line 2:
<comment>`Volume2` and `VolumeX` are unused imports, adding dead code and causing a no-unused-vars failure in configurations that lint this client. Removing them would keep the import aligned with the component.</comment>
<file context>
@@ -0,0 +1,175 @@
+import React, { useEffect, useRef, useState } from "react";
+import { Volume2, VolumeX, AlertTriangle, Activity } from "lucide-react";
+
+/**
</file context>
| // Map peakDb (-60dB to 0dB) to a percentage (0% to 100%) | ||
| const percentage = Math.max(0, Math.min(100, ((peakDb + 60) / 60) * 100)); | ||
|
|
||
| function resetMaxPeak() { |
There was a problem hiding this comment.
P3: resetMaxPeak is dead code, and no user-facing path can reset the max-peak hold despite the helper being present. Exposing a reset control/callback or removing the unused helper would keep the component behavior and API intentional.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At client/src/components/PeakLevelMeter.jsx, line 112:
<comment>`resetMaxPeak` is dead code, and no user-facing path can reset the max-peak hold despite the helper being present. Exposing a reset control/callback or removing the unused helper would keep the component behavior and API intentional.</comment>
<file context>
@@ -0,0 +1,175 @@
+ // Map peakDb (-60dB to 0dB) to a percentage (0% to 100%)
+ const percentage = Math.max(0, Math.min(100, ((peakDb + 60) / 60) * 100));
+
+ function resetMaxPeak() {
+ setMaxPeakDb(-60);
+ setIsClipping(false);
</file context>
Nitya-003
left a comment
Nitya-003
left a comment
There was a problem hiding this comment.
@hrshjswniii Resolve the comments by bot.
itsdakshjain
left a comment
itsdakshjain
left a comment
There was a problem hiding this comment.
Good work , resolve the cubic review and squash commits
itzzavdhesh
added
gssoc:approved
github-actions
Bot
added
ready for review
needs revision
and removed
ready for review
needs revision
labels
🎊 PR Merged SuccessfullyHey @hrshjswniii! 👋 Congratulations and thank you for your contribution to VoiceForge! Note 🔗 Linked issue(s): #1137 · ✅ Marked as merged and complete Maintainers may still handle final cleanup, release notes, or follow-up tracking after the merge. 🤖 VoiceForge Automation · Updates automatically on edits |
itzzavdhesh
added
SSoC26
itzzavdhesh
added
mentor:Anushreebasics
4 participants
🚀 Program
GSSoC
📝 Description
This PR implements a real-time Audio Peak Level VU Meter (in dB) with Digital Clipping Warning Indicators in VoiceForge. It gives non-verbal users and sound creators real-time visual feedback on decibel levels (-60 dB to 0 dB) and clipping risk while tuning voice synthesis DSP sliders (stability, temperature, style exaggeration).
Key additions:
PeakLevelMeter.jsx): Built a reusablePeakLevelMeter.jsxcomponent driven by Web AudioAnalyserNodeandrequestAnimationFrame.PeakLevelMeteradjacent to the Voice Synthesis DSP sliders inSettings.jsxandOnboarding.jsx.PeakLevelMeter.test.js): Added Vitest test assertions forPeakLevelMeter.🔗 Related Issue
Closes #1137
🔄 Type of Change
🧪 How to Test
http://localhost:5173in a browser.npm run test --workspace clientand verify all tests pass.✅ Checklist
feat: add audio peak level vu meter and clipping warning)Summary by cubic
Adds a real-time audio peak level VU meter (dB) with a clipping warning to Settings and Onboarding, giving live feedback while tuning voice synthesis sliders. Introduces a reusable
PeakLevelMetercomponent and a basic unit test.PeakLevelMetervisualizes -60 dB to +3 dB with a gradient bar, max-peak hold line, and optional labels; updates via Web AudioAnalyserNodeandrequestAnimationFrame.audioElementRefor an injected analyser, with simulated activity viaisActiveand atestLevelprop for tests/demos.Written for commit 59c0acc. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests