Skip to content

Conversation

@kixelated
Copy link
Collaborator

@kixelated kixelated commented Oct 22, 2025

It was changed to on-demand, but that breaks the public signals.

In the future we should only run VAD when there's a subscription or the signal is actively being used.

Summary by CodeRabbit

  • Bug Fixes
    • Improved reliability of audio features by preventing errors when the audio context closes during recording or playback operations.

Was set to only run when requested, but it makes the signal useless.
TODO: Figure out a good API to request stuff run locally.
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 22, 2025

Walkthrough

This pull request adds AudioContext closure guards across four audio-related files to prevent AudioWorkletNode instantiation when the context state is "closed". The changes are applied to captions, encoder, speaking, and source modules. Additionally, in the encoder module, the asynchronous race pattern for loading the CaptureWorklet is replaced with a direct await of context.audioWorklet.addModule(), removing the ready/cancellation path and implementing an early exit if the AudioContext is closed.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "Always run VAD when enabled" aligns with the stated objective of re-enabling voice activity detection functionality whenever it is enabled, as documented in the PR description. The four file changes across audio modules add defensive guards to prevent AudioWorkletNode creation when the AudioContext is closed, which directly supports achieving the objective of reliably running VAD when enabled. The title captures the feature-level intent even though it doesn't detail the low-level implementation mechanism of adding context state checks. The title is clear, concise, and directly related to the main purpose of the changeset.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch always-speaking

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (4)
js/hang/src/publish/audio/captions.ts (1)

105-106: Clarify the comment wording.

The comment states "Ensure the context is running" but the code checks if the context is "closed". An AudioContext can be in "suspended", "running", or "closed" states. A suspended context would pass this check but wouldn't be "running". Consider updating the comment to accurately reflect what's being checked.

Apply this diff to improve comment accuracy:

-			// Ensure the context is running before creating the worklet
+			// Skip worklet creation if the context is already closed
 			if (ctx.state === "closed") return;
js/hang/src/watch/audio/source.ts (1)

122-123: Clarify the comment wording.

The comment states "Ensure the context is running" but the code checks if the context is "closed". Consider updating the comment to accurately reflect the check being performed.

Apply this diff to improve comment accuracy:

-			// Ensure the context is running before creating the worklet
+			// Skip worklet creation if the context is already closed
 			if (context.state === "closed") return;
js/hang/src/publish/audio/encoder.ts (1)

107-108: Add a comment for consistency.

The guard at Line 108 is correct and follows the same pattern as the other files in this PR (captions.ts, source.ts, speaking.ts). However, those files include a comment explaining the guard, while this one doesn't. Consider adding a comment for consistency.

Apply this diff to add a comment:

 			await context.audioWorklet.addModule(CaptureWorklet);
+			// Skip worklet creation if the context is already closed
 			if (context.state === "closed") return;
js/hang/src/publish/audio/speaking.ts (1)

77-78: Clarify the comment wording.

The comment states "Ensure the context is running" but the code checks if the context is "closed". Consider updating the comment to accurately reflect the check being performed.

Apply this diff to improve comment accuracy:

-			// Ensure the context is running before creating the worklet
+			// Skip worklet creation if the context is already closed
 			if (ctx.state === "closed") return;
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7bd983b and a02ff41.

📒 Files selected for processing (4)
  • js/hang/src/publish/audio/captions.ts (1 hunks)
  • js/hang/src/publish/audio/encoder.ts (1 hunks)
  • js/hang/src/publish/audio/speaking.ts (1 hunks)
  • js/hang/src/watch/audio/source.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{rs,js}/hang/**

📄 CodeRabbit inference engine (CLAUDE.md)

{rs,js}/hang/**: All media-specific logic (encoding/streaming, codec handling) must live in the hang layer.
Keep the hang layer generic: expose primitives (e.g., access to individual frames) and avoid application/UI-specific code.

Files:

  • js/hang/src/watch/audio/source.ts
  • js/hang/src/publish/audio/captions.ts
  • js/hang/src/publish/audio/speaking.ts
  • js/hang/src/publish/audio/encoder.ts
⏰ 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). (1)
  • GitHub Check: Check

@kixelated kixelated merged commit e5f8f63 into main Oct 22, 2025
1 check passed
@kixelated kixelated deleted the always-speaking branch October 22, 2025 21:26
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.

2 participants