feat(voice): add mic input device selector and stabilize media capture for composer#1616
Merged
Merged
Conversation
…tion logic - Introduced a new interface for audio input devices. - Added a prop to enable microphone device selection. - Implemented device enumeration and selection handling in the MicCloudComposer component. - Updated UI to display a dropdown for selecting available audio input devices when the selector is enabled.
… audio capture integrity - Removed the flag to prevent audio capture device replacement. - Added comments to clarify the rationale behind the changes and the use of for video input.
…able - Deleted the idx_profile_state and idx_profile_class indexes to streamline the database schema. - This change aims to improve performance and reduce redundancy in the user_profile table.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an optional microphone device selector to MicCloudComposer (enumeration, selection, exact-device getUserMedia, and tests), enables it in Conversations, removes a CEF fake-audio flag to preserve real audio devices while injecting Y4M video, and narrows Phase 3 profile migration to column-only updates. ChangesMicrophone Device Selection Feature
CEF Fake-Camera Configuration
Profile Table Schema Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/features/human/MicCloudComposer.tsx (1)
234-259:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent stream leak and misclassified mic errors in
startRecording.Line 247 can throw after Line 234 already granted a stream; current catch path (Line 253 onward) returns without stopping that stream, which can leave the mic active. It also labels every failure as permission-denied, including device selection/enumeration failures.
💡 Proposed fix
- let stream: MediaStream; + let stream: MediaStream; try { // Audio constraints tuned for STT accuracy: @@ stream = await navigator.mediaDevices.getUserMedia({ audio: { ...(selectedDeviceId ? { deviceId: { exact: selectedDeviceId } } : {}), @@ }, }); - // After the first successful grant, refresh device labels (they are - // blank until the user has given permission). - if (showDeviceSelector) { - const all = await navigator.mediaDevices.enumerateDevices(); - const inputs = all - .filter(d => d.kind === 'audioinput') - .map((d, i) => ({ deviceId: d.deviceId, label: d.label || `Microphone ${i + 1}` })); - setDevices(inputs); - } } catch (err) { startInFlightRef.current = false; const msg = err instanceof Error ? err.message : String(err); composerLog('getUserMedia rejected: %s', msg); - onError?.(`Microphone permission denied: ${msg}`); + const name = err instanceof DOMException ? err.name : ''; + if (name === 'NotAllowedError' || name === 'SecurityError') { + onError?.(`Microphone permission denied: ${msg}`); + } else if (name === 'NotFoundError' || name === 'OverconstrainedError') { + onError?.('Selected microphone is unavailable. Choose a different input device.'); + } else { + onError?.(`Failed to access microphone: ${msg}`); + } return; } + + // After the first successful grant, refresh device labels (best-effort). + if (showDeviceSelector && navigator.mediaDevices?.enumerateDevices) { + try { + const all = await navigator.mediaDevices.enumerateDevices(); + const inputs = all + .filter(d => d.kind === 'audioinput') + .map((d, i) => ({ deviceId: d.deviceId, label: d.label || `Microphone ${i + 1}` })); + setDevices(inputs); + } catch (err) { + composerLog('post-grant enumerateDevices failed: %s', err); + } + }🤖 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 `@app/src/features/human/MicCloudComposer.tsx` around lines 234 - 259, In startRecording (around getUserMedia, enumerateDevices and the catch block) ensure any MediaStream acquired is stopped if a later step throws: capture the stream returned by navigator.mediaDevices.getUserMedia in a local variable, and in the catch path call stop() on all its tracks (stream.getTracks().forEach(t=>t.stop())) before returning and before resetting startInFlightRef.current; also improve the error classification sent to onError by inspecting the thrown error (e.g., DOMException.name or message) so only true permission errors produce "permission denied" and other errors (enumerateDevices failures, device-not-found, etc.) forward a precise message (include the actual err.message/string) instead of always saying permission denied.
🤖 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.
Outside diff comments:
In `@app/src/features/human/MicCloudComposer.tsx`:
- Around line 234-259: In startRecording (around getUserMedia, enumerateDevices
and the catch block) ensure any MediaStream acquired is stopped if a later step
throws: capture the stream returned by navigator.mediaDevices.getUserMedia in a
local variable, and in the catch path call stop() on all its tracks
(stream.getTracks().forEach(t=>t.stop())) before returning and before resetting
startInFlightRef.current; also improve the error classification sent to onError
by inspecting the thrown error (e.g., DOMException.name or message) so only true
permission errors produce "permission denied" and other errors (enumerateDevices
failures, device-not-found, etc.) forward a precise message (include the actual
err.message/string) instead of always saying permission denied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c9ee3629-74fe-4593-8b73-a2c5abd466c9
📒 Files selected for processing (4)
app/src-tauri/src/lib.rsapp/src/features/human/MicCloudComposer.tsxapp/src/pages/Conversations.tsxsrc/openhuman/memory/store/unified/profile.rs
💤 Files with no reviewable changes (1)
- src/openhuman/memory/store/unified/profile.rs
graycyrus
requested changes
May 13, 2026
Contributor
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Review Summary
The CEF flag fix in lib.rs is the correct root cause fix — --use-fake-device-for-media-stream was silently replacing the real microphone with a sine-wave test tone. That one-line removal and the excellent explanatory comment are solid.
The device selector in MicCloudComposer is a useful addition for multi-input systems, but ships 107 new lines of stateful device-enumeration logic with no new tests (the existing 14 tests don't cover any of the new code paths). The PR checklist says "Tests added or updated" — this is not satisfied.
Missing test coverage
At minimum before merge:
showDeviceSelector=truerenders the<select>whenenumerateDevicesreturns multiple devices- The selected
deviceIdis passed as{ exact: selectedDeviceId }ingetUserMedia - Changing the selector updates the constraint
devicechangeevent triggers re-enumerationenumerateDevicesthrowing doesn't crash the component- Post-grant label refresh calls
enumerateDevicesagain
The existing beforeEach already stubs navigator.mediaDevices — just add enumerateDevices, addEventListener, and removeEventListener to the stub.
Issue #1610 coverage
This PR addresses 2 of 9 acceptance criteria (device selection + beep root cause). Remaining: permission state UI, specific failure messages, diagnostics/Sentry, beep text handling. Consider noting in the PR body which criteria are deferred to follow-up work.
Adds 6 new tests for the device enumeration and selector introduced in the previous commit — covering the loadDevices useEffect, fallback labels, deviceId constraint forwarding, post-permission label refresh, and enumerateDevices error handling. Closes the coverage gap reported by diff-cover (was 33%; new lines all covered).
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/features/human/MicCloudComposer.test.tsx (1)
316-333: ⚡ Quick winAvoid fixed sleeps in this test path (flaky + slower).
Line 330 uses an arbitrary
setTimeout(50); this makes the test timing-dependent. Prefer asserting the absence/presence behavior directly without wall-clock waits.Suggested change
- // Give any async effects a chance to run - await new Promise(r => setTimeout(r, 50)); expect(screen.queryByRole('combobox', { name: /microphone device/i })).not.toBeInTheDocument(); expect(enumerateDevicesMock).not.toHaveBeenCalled();As per coding guidelines: "Prefer behavior over implementation in tests; use helpers from
app/src/test/".🤖 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 `@app/src/features/human/MicCloudComposer.test.tsx` around lines 316 - 333, The test uses a fixed sleep (await new Promise(r => setTimeout(r, 50))) which is flaky; replace this with a behavior-based wait (e.g., use RTL's waitFor or existing helpers from app/src/test/) to await stable UI state and then assert absence of the combobox and that enumerateDevicesMock was not called. Locate the test for MicCloudComposer in MicCloudComposer.test.tsx and replace the arbitrary timeout with a wait that checks screen.queryByRole('combobox', { name: /microphone device/i }) remains null (or use waitFor(() => expect(...).not.toBeInTheDocument())) and then assert enumerateDevicesMock not.toHaveBeenCalled(); use the project test helpers instead of wall-clock sleeps.
🤖 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.
Nitpick comments:
In `@app/src/features/human/MicCloudComposer.test.tsx`:
- Around line 316-333: The test uses a fixed sleep (await new Promise(r =>
setTimeout(r, 50))) which is flaky; replace this with a behavior-based wait
(e.g., use RTL's waitFor or existing helpers from app/src/test/) to await stable
UI state and then assert absence of the combobox and that enumerateDevicesMock
was not called. Locate the test for MicCloudComposer in
MicCloudComposer.test.tsx and replace the arbitrary timeout with a wait that
checks screen.queryByRole('combobox', { name: /microphone device/i }) remains
null (or use waitFor(() => expect(...).not.toBeInTheDocument())) and then assert
enumerateDevicesMock not.toHaveBeenCalled(); use the project test helpers
instead of wall-clock sleeps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9d17c535-7b45-4cdc-b6ce-4f9314cc3eee
📒 Files selected for processing (1)
app/src/features/human/MicCloudComposer.test.tsx
- Updated existing test for permission-denied errors to specify NotAllowedError. - Added new tests for handling OverconstrainedError, NotReadableError, and generic errors from getUserMedia. - Included a test to verify the device selector is disabled when only one device is available. - Improved error messaging in the MicCloudComposer component to provide more specific feedback based on the error type.
…aths Removes idx_profile_state and idx_profile_class from PHASE3_INDEXES_SQL and the init.rs migration chain to match the earlier removal from PROFILE_INIT_SQL. Both indexes are now fully absent from new installs and existing-DB migrations alike.
…ability - Simplified the assertion for error handling in the MicCloudComposer tests. - Enhanced the formatting of the mock device enumeration to improve clarity.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@app/src/features/human/MicCloudComposer.test.tsx`:
- Around line 349-366: Replace the fixed sleep used to wait for async effects in
the test for MicCloudComposer with React Testing Library's waitFor: remove the
new Promise(r => setTimeout(r, 50)) and instead call await waitFor(() =>
expect(screen.queryByRole('combobox', { name: /microphone device/i
})).not.toBeInTheDocument()); keep the existing mocks (enumerateDevicesMock and
getUserMediaMock) and the final assertion that enumerateDevicesMock was not
called so the test polls reliably instead of relying on a fixed timeout.
🪄 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: b4e2d302-512e-4d0a-b9db-e71418673bd9
📒 Files selected for processing (4)
app/src/features/human/MicCloudComposer.test.tsxapp/src/features/human/MicCloudComposer.tsxsrc/openhuman/memory/store/unified/init.rssrc/openhuman/memory/store/unified/profile.rs
💤 Files with no reviewable changes (1)
- src/openhuman/memory/store/unified/profile.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/features/human/MicCloudComposer.tsx
graycyrus
previously approved these changes
May 13, 2026
Contributor
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
All four findings addressed cleanly — index removal completed, error classification differentiated by DOMException.name, selector visible on single-mic systems, and devicechange listener properly wrapped. Tests added for each. LGTM.
12 tasks
AusAgentSmith
pushed a commit
to AusAgentSmith/openhuman
that referenced
this pull request
May 23, 2026
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
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.
Summary
Added an audio input device selector to MicCloudComposer so users can choose which microphone to record from.
Implemented device enumeration and selection wiring for capture setup in the voice composer flow.
Improved fake media stream handling to make audio capture behavior more reliable in test/dev and edge capture paths.
Removed unused user_profile database indexes to reduce schema clutter and maintenance overhead.
Applied Prettier formatting updates across touched files for consistency.
Problem
Voice capture in the composer did not expose microphone selection, which can cause wrong-device recording on multi-input systems.
Media capture behavior around fake/virtual streams could lead to unstable or inconsistent audio capture outcomes.
Unused profile table indexes increased schema complexity without clear runtime benefit.
Solution
Extended MicCloudComposer with audio device discovery, selector UI/state, and selected-device capture wiring.
Refactored media-capture internals to better preserve audio stream integrity when fake streams are involved.
Cleaned profile persistence code by removing unused user_profile index declarations.
Kept scope focused to existing flows (composer + capture plumbing) with minimal surface-area changes outside impacted modules.
Submission Checklist
Tests added or updated (happy path + at least one failure / edge case) per Testing Strategy
Diff coverage ≥ 80% — changed lines (Vitest + cargo-llvm-cov merged via diff-cover) meet the gate enforced by
.github/workflows/coverage.yml. Runpnpm test:coverageandpnpm test:rustlocally; PRs below 80% on changed lines will not merge.Coverage matrix updated — added/removed/renamed feature rows in
docs/TEST-COVERAGE-MATRIX.mdreflect this change (orN/A: behaviour-only change)All affected feature IDs from the matrix are listed in the PR description under
## RelatedNo new external network dependencies introduced (mock backend used per Testing Strategy)
Manual smoke checklist updated if this touches release-cut surfaces (
docs/RELEASE-MANUAL-SMOKE.md)Linked issue closed via
Closes #NNNin the## RelatedsectionImpact
Platform/runtime: Desktop app UI + capture flow (
app/) and minor core schema cleanup (src/openhuman/...); no new platform targets.Performance: Potentially improved by removing unused DB indexes; no expected regressions in normal capture paths.
Security/privacy: No new external dependencies; device selection increases user control over recording source.
Compatibility: Backward-compatible behavior with added optional device-selection capability.
Related
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores