Skip to content

feat: add audio input and audio output widgets (#47)#48

Open
claude[bot] wants to merge 8 commits intomainfrom
claude/add-audio-in-out-widgets
Open

feat: add audio input and audio output widgets (#47)#48
claude[bot] wants to merge 8 commits intomainfrom
claude/add-audio-in-out-widgets

Conversation

@claude
Copy link
Copy Markdown
Contributor

@claude claude Bot commented Apr 27, 2026

Adds AudioInputWidget and AudioOutputWidget components for the new audio in/out resource types, enabling users to view audio device properties directly from the control interface.

Closes #47

Frontend

  • audio-input/audio-input.svelte — main widget for the audio input component. Uses AudioInClient to query getProperties with polling support via RefetchController.
  • audio-input/properties.svelte — displays supported codecs, sample rate, and channel count.
  • audio-output/audio-output.svelte — main widget for the audio output component. Uses AudioOutClient to query getProperties with polling support.
  • audio-output/properties.svelte — displays supported codecs, sample rate, and channel count.
  • client-map.ts — registers AudioInClient and AudioOutClient under rdk:component:audio_input and rdk:component:audio_output.
  • builtin.ts — maps both audio resource APIs to their widgets so they appear in the control view.
  • components/index.ts — exports AudioInputWidget and AudioOutputWidget.

Testing

  • Added unit tests for both audio-input/properties.svelte and audio-output/properties.svelte covering codec display, empty codec list, sample rate, and channel count rendering.
  • Ran pnpm lint and pnpm check (both pass clean). pnpm test fails due to a pre-existing Playwright browser installation issue in the environment, unrelated to this change.

DRI

@DTCurrie

Add `AudioInputWidget` and `AudioOutputWidget` components for the
audio in/out resource types. Each widget displays GetProperties
(supported codecs, sample rate, channels) with polling support.

Register both components in the client map and builtin resource map
so they appear in the control view.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Apr 27, 2026

🦋 Changeset detected

Latest commit: 838215d

The changes in this PR will be included in the next version bump.

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

Alphabetically "Audio" (Au) sorts after "Arm" (Ar). The PR inserted
the audio exports before the Arm block; move them to their correct
position between ArmQuickMoveWidget and BaseWidget.
Copy link
Copy Markdown
Contributor Author

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See inline comments for design observations. One ordering bug was fixed directly (commit b6ffc80).

Comment thread src/lib/components/widgets/audio-output/properties.svelte
Comment thread src/lib/components/widgets/audio-input/audio-input.svelte
Comment thread src/lib/components/widgets/audio-output/audio-output.svelte
@DTCurrie
Copy link
Copy Markdown
Member

@claude review the changes

@claude
Copy link
Copy Markdown
Contributor Author

claude Bot commented Apr 27, 2026

Claude finished @DTCurrie's task in 3m 36s —— View job


Review

  • Read CLAUDE.md and gather context
  • Read all changed files
  • Run git diff
  • Review correctness, security, API design, patterns, tests
  • Post inline comments for line-specific issues
  • Post summary

Overall the PR is well-structured and follows existing widget conventions. The do-command refactor is a clear improvement. Five issues worth addressing before merge, ranging from a bug to design gaps.

Bug

Blob URL memory leak (audio-input.svelte:114)
captureDownloadUrl is revoked when a new capture starts but never when the component is destroyed. Add an $effect cleanup:

$effect(() => {
  return () => {
    if (captureDownloadUrl) URL.revokeObjectURL(captureDownloadUrl)
  }
})

Design issues

Unrecognized file extension → silent invalid codec (audio-output.svelte:74)
When file.type misses and the extension isn't in the map (.m4a, .aiff, etc.), playCodec gets the raw extension string. selectedCodec only falls back to availableCodecs[0] when playCodec is empty, so an unrecognized extension will be sent to the SDK as-is, producing an opaque server error with no UI feedback.

No cancellation for play (audio-output.svelte:181)
getAudio has an AbortController; play does not. If the server hangs the button stays disabled indefinitely. Wire an AbortController signal into callOptions the same way startCapture does.

as unknown as type cast for AudioInfo (audio-output.svelte:97)
The double cast silently bypasses type safety. Try importing AudioInfo from the internal SDK path, or at minimum file a tracking issue so the cast gets revisited once the SDK re-exports it.

Exclude<Resource, MLModelClient> may resolve to never (create-do-command-client.svelte.ts:11)
Exclude only works correctly when Resource is a union type. If Resource is an interface or base class, Exclude returns never and the subsequent as DoCommandable cast silently lies. Verify the shape of Resource in the SDK.

Test gaps

Main widget logic is untested (properties.spec.ts:35)
Tests only cover the Properties display sub-component. The capture/play state machines (abort vs. error branching, blob URL creation, byte counter) have no coverage.

Positives

  • createDoCommandClient properly moves reactive client construction out of $derived — a clear correctness fix.
  • supportsDoCommand is the right abstraction for the gate check; comparing constructor references is correct.
  • $state.raw for captureAbortController is the right call (replaced wholesale, no deep reactivity needed).
  • Error narrowing (error instanceof Error ? error : new Error(String(error))) is consistent throughout.
  • on:change / slot="input" patterns match the existing codebase — prime-core components are still Svelte 4-style, so this is intentional and correct.
  • Changeset is correctly marked minor.
    | Branch

@DTCurrie
Copy link
Copy Markdown
Member

Screenshot 2026-04-27 at 6 57 13 PM Screenshot 2026-04-27 at 6 57 08 PM

Tested in the playground and things were working, will do a deeper review

Comment thread src/lib/components/widgets/audio-input/audio-input.svelte Outdated
Comment thread src/lib/components/widgets/audio-output/audio-output.svelte Outdated
Comment thread src/lib/components/widgets/audio-output/audio-output.svelte
Comment thread src/lib/components/widgets/audio-output/audio-output.svelte
Comment thread src/lib/components/widgets/do-command/create-do-command-client.svelte.ts Outdated
Comment thread src/lib/components/widgets/audio-input/__tests__/properties.spec.ts
@claude
Copy link
Copy Markdown
Contributor Author

claude Bot commented Apr 27, 2026

Claude finished @DTCurrie's task in 3m 4s —— View job


Review

  • Read CLAUDE.md
  • Read all changed files
  • Verify Viam proto/SDK contracts
  • Analyze correctness, security, API design, patterns, tests
  • Post inline comments
  • Post summary

Prior inline comment retracted

My earlier inline comment on audio-input.svelte:35 said the widget only surfaced getProperties and didn't use getAudio. DTCurrie is right to push backgetAudio is used in startCapture (line 72). That comment was incorrect; the widget does implement the streaming capture flow. Apologies for the noise.


Verified against Viam proto/SDK

I fetched the proto definitions and TypeScript SDK source to validate the implementation:

getAudio call signature (audioin.proto) — matches exactly. Codec is string (not an enum), duration_seconds is float, previous_timestamp_nanoseconds is int64. The PR passes selectedCodec (string), captureDuration (number), 0n (bigint) — all correct.

play call signature (audioout.proto / SDK client) — AudioInfo has fields codec (string), sample_rate_hz / num_channels (int32), mapped to camelCase sampleRateHz / numChannels in TypeScript. The plain-object cast works at runtime because Buf constructors accept PartialMessage<T>. Confirmed: AudioInfo is not exported from the SDK barrel, so the as unknown as Parameters<...>[1] cast is necessary. The cast is safe given the field shapes match.

Client map keysrdk:component:audio_in and rdk:component:audio_out are the correct API strings (matching viam.component.audioin.v1 / viam.component.audioout.v1). The PR description incorrectly says audio_input/audio_output but the code is right.


Remaining issues

Bug — Blob URL memory leak on component destroy

captureDownloadUrl is revoked on new capture start (lines 61–64) but never when the component unmounts while a URL is live. This is a real leak. Fix: add an $effect cleanup:

$effect(() => {
  return () => {
    if (captureDownloadUrl) URL.revokeObjectURL(captureDownloadUrl)
  }
})

Fix this →

Design — Unrecognized file extension sends invalid codec to SDK

When file.type misses and the extension isn't in mimeToCodec (e.g. .m4a, .aiff), playCodec is set to the raw extension string (e.g. 'm4a'). Because selectedCodec = $derived(playCodec || availableCodecs[0]!) only falls back when playCodec is empty, an unrecognized extension produces an opaque server error with no UI feedback.

Suggestion: fall through to '' when extension is unrecognized so the availableCodecs[0] fallback kicks in:

const detected = mimeToCodec[file.type] ?? mimeToCodec[`audio/${file.name.split('.').pop()?.toLowerCase()}`] ?? ''
playCodec = detected

Design — No abort path for play

getAudio uses an AbortController; play does not. If the server hangs, the button stays disabled indefinitely with no escape. Worth wiring a controller through callOptions the same way startCapture does.

Test gaps — State machine behavior untested

Tests cover properties.svelte display only. The startCapture and play state machines — abort vs. error branching, blob URL creation, byte counter — have zero coverage. Consider adding at least:

  • startCapture → error path sets captureStatus = 'error'
  • stopCapture → abort sets captureStatus = 'done' (not 'error')
  • successful capture → captureDownloadUrl is populated

Positives

  • createDoCommandClient correctly extracts reactive client construction into a .svelte.ts file with a get current() getter — reactivity is preserved properly.
  • $state.raw for captureAbortController is the right call — replaced wholesale, no deep reactivity needed.
  • supportsDoCommand uses value-level comparison (client !== MLModelClient) which is correct; the type-level Exclude<Resource, MLModelClient> concern from the prior review is a type cosmetic only, not a runtime bug.
  • Error narrowing (error instanceof Error ? error : new Error(String(error))) is consistent throughout.
  • on:change / slot="input" for prime-core components is correct — those are Svelte 4-style and intentionally use the older API.
  • Changeset is correctly marked minor.
  • Playground testing by @DTCurrie confirms the happy path works end-to-end.
    | Branch

@claude
Copy link
Copy Markdown
Contributor Author

claude Bot commented Apr 27, 2026

Claude finished @DTCurrie's task in 1m 20s —— View job


Review update

  • Read CLAUDE.md
  • Read audio-input.svelte and audio-output.svelte to verify DTCurrie's corrections
  • Acknowledge corrections and update review

Corrections acknowledged

@DTCurrie is right on both counts — I apologize for the incorrect observations:

  • getAudio IS usedaudio-input.svelte calls client.current.getAudio(...) at line 72 inside startCapture. The widget surfaces both getAudio (via the capture UI) and getProperties. My earlier comment was wrong.
  • play IS usedaudio-output.svelte calls client.current.play(...) at line 97 inside the play function. Same situation. My earlier comment was wrong.

Both widgets implement the full primary interface for their respective clients. The "only surfaces getProperties" observation should be disregarded entirely.


Remaining issues (still valid)

The five issues from the earlier review that don't touch those two incorrect observations are still accurate:

Bug — Blob URL memory leak (audio-input.svelte:61–64 / 114)
captureDownloadUrl is revoked on new capture but never when the component is destroyed. Add:

$effect(() => {
  return () => { if (captureDownloadUrl) URL.revokeObjectURL(captureDownloadUrl) }
})

Design — Unrecognized extension silently passes invalid codec (audio-output.svelte:76)
mimeToCodec[file.type] ?? file.name.split('.').pop()?.toLowerCase() ?? '' — the extension branch returns the raw string (e.g. 'm4a'). Because selectedCodec = $derived(playCodec || availableCodecs[0]!) only falls back when playCodec is empty, a non-empty-but-unrecognized string gets sent to the SDK.

Design — No abort for play (audio-output.svelte:81–111)
The play function has no AbortController, so a hung server call leaves the button disabled indefinitely.

Design — as unknown as cast (audio-output.svelte:103)
The double cast for AudioInfo bypasses type safety; importing from the internal SDK path would remove the need for it.

Design — Exclude<Resource, MLModelClient> shape concern (create-do-command-client.svelte.ts:11)
Worth verifying Resource is a union type (not an interface) to confirm Exclude works as expected.

Test coverage — capture/play state machines (abort vs. error paths, blob URL creation, byte counter) remain untested.


Glad to see it working in the playground. The core implementation is solid — the remaining items are improvements rather than blockers.
| Branch

@claude
Copy link
Copy Markdown
Contributor Author

claude Bot commented Apr 27, 2026

Claude finished @DTCurrie's task in 1m 16s —— View job


Clarifying the blob URL cleanup fix

  • Read CLAUDE.md
  • Read audio-input.svelte to identify the correct fix location
  • Post corrected inline comment

The previous suggestion block at line 114 was malformed — it echoed the existing URL.createObjectURL line rather than showing an addition. Posted a corrected inline comment at line 46 with a proper suggestion block.

The fix: add a new $effect after the last state declaration:

$effect(() => {
    return () => {
        if (captureDownloadUrl) URL.revokeObjectURL(captureDownloadUrl)
    }
})

Because the $effect body has no reactive reads, Svelte treats it as a mount-once effect — its returned cleanup runs exactly once when the component is destroyed, revoking any live blob URL at that point.
| Branch

Comment thread src/lib/components/widgets/audio-input/audio-input.svelte Outdated
@DTCurrie
Copy link
Copy Markdown
Member

@claude everything should be resolved except: "Design — No abort for play (audio-output.svelte:81–111)"

We cannot stop the unary RPC, even with an abort controller. Once the server begins playing the audio, it continues to play it.

@claude
Copy link
Copy Markdown
Contributor Author

claude Bot commented Apr 28, 2026

Claude finished @DTCurrie's task in 1m 4s —— View job


Review update

  • Read CLAUDE.md and current state of changed files
  • Verify all other issues are resolved
  • Acknowledge abort limitation for unary RPC
  • Post final summary

All remaining issues resolved

@DTCurrie confirmed — the "no abort for play" concern is closed as won't fix / by design. That's the right call: play is a unary RPC and the server begins audio playback as soon as it receives the request. An AbortController on the client side would cancel the HTTP/2 stream, but the server has already received all the audio data and will continue playing regardless. There's no mechanism to interrupt server-side audio playback mid-stream via gRPC cancellation.

Everything else from the prior review has been addressed:

Issue Status
Blob URL memory leak on component destroy ✅ Fixed — $effect cleanup added to create-audio-capturer.svelte.ts (lines 12–16)
Unrecognized file extension sends invalid codec ✅ Fixed — audio-output.svelte now shows 'Unsupported audio format' error instead of passing raw extension to SDK
as unknown as double cast for AudioInfo ✅ Fixed — create-audio-player.svelte.ts uses commonApi.AudioInfo.fromJson(...)
Capture/play state machine tests missing ✅ Added — audio-capture.svelte.spec.ts covers success, error, and abort paths; audio-play.svelte.spec.ts covers success and error paths
No abort for play ✅ Closed — unary RPC, server-side playback cannot be stopped by cancelling the client stream

The Exclude<Resource, MLModelClient> concern also resolves cleanly: (typeof clientMap)[keyof typeof clientMap] is a union of constructor types, so Exclude operates correctly.

Implementation looks solid. LGTM.
| Branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add audio in/out components

1 participant