fix(voice): improve Pocket playback speed quality - #206
Conversation
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
REQUEST_CHANGES: one blocking lifecycle regression is present in the exact three-dot comparison. Pocket playback now drops its assistant-speech/capture-protection guard as soon as AVAudioPlayerNode reports its final source buffer completed, removing the existing route-aware output-latency grace; buffered Bluetooth, AirPlay, virtual, and unknown routes can therefore resume capture while speech is still physically audible. I statically reviewed all eight changed files and the complete Pocket playback flow (creation, routing, speed processing, enqueue, progress accounting, completion, interruption, failure, and cleanup), plus accessibility, localization, navigation/consent, async never-completes and races, tests, project laws/design-system applicability, and overlap/duplicate status. No additional supported findings survived the final self-check. Supplied GitHub evidence was inspected: the listed DCO, frontend, clean-room, and platform Rust checks succeeded, while combined status was pending; required checks still govern merge readiness. No local code, tests, builds, package managers, hooks, binaries, or project scripts were run.
Deterministic publication result: 1 blocking and 0 non-blocking finding(s) publishable; 0 duplicate(s) suppressed.
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
REQUEST_CHANGES: one blocking lifecycle regression is present in the exact three-dot comparison. The new finish path has no degraded path if the native pending-buffer completion bookkeeping never reaches zero, so Pocket playback can remain active indefinitely; the replaced implementation explicitly bounded this condition and its regression test was deleted. I statically reviewed all eight changed files and the complete Pocket playback flow (player creation, speed processing, output routing, enqueue, progress/delivery accounting, burst/grace handling, completion, interruption, failure, and cleanup), plus accessibility, localization, navigation/consent, async error/never-completes/race behavior, test honesty, project laws/design-system applicability, and finding overlap. The prior output-latency capture-grace issue is fixed in the current comparison and its resolved thread has a substantive human reply, so it is neither a new finding nor a duplicate suppression. No other supported findings survived the final self-check. Supplied GitHub evidence was inspected: DCO, frontend, clean-room, and platform Rust checks were successful, Transcript virtualization was in progress, and combined status was pending; required checks still govern merge readiness. No local code, tests, builds, package managers, hooks, binaries, or project scripts were run.
Deterministic publication result: 1 blocking and 0 non-blocking finding(s) publishable; 0 duplicate(s) suppressed.
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
APPROVE: no publishable findings remain in the exact three-dot comparison. I statically reviewed all eight changed files and traced every changed Pocket flow: native player creation and ownership, speed processing, output routing, streaming append/flush/finish, one-shot preview/speak, enqueue and delivery accounting, route-aware capture grace, timeout fallback, interruption, stop, failure, and cleanup. The stream finish path now bounds missing native completion bookkeeping and independently preserves route-aware capture grace. A similar never-completes risk remains in the changed one-shot preview/speak drain loop, but it is the same underlying missing-native-completion issue as the resolved protected thread whose substantive human reply states the fix scope; policy therefore suppresses it rather than publishing it again. Final checks covered accessibility and i18n (no UI/string-surface change requiring localization), navigation/consent (not affected), async errors, never-completes, lifecycle and races, test honesty, design-system/project rules, duplicate overlap, and concrete user effects; no other supported issue survived. Supplied GitHub evidence was structurally valid and inspected: DCO, frontend, transcript virtualization, clean-room, and macOS/Windows/Linux Rust checks all completed successfully, while combined status was pending; required checks still govern merge readiness. No local code, tests, builds, package managers, hooks, binaries, or project scripts were run.
Deterministic publication result: 0 blocking and 0 non-blocking finding(s) publishable; 1 duplicate(s) suppressed.
Pending checks: 1 check(s) are not complete.
This approval reflects the completed code review only; merge readiness remains governed by the repository's required checks.
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
APPROVE: no publishable findings remain in the exact three-dot comparison. I statically reviewed all eight changed files and traced every changed Pocket flow: native player creation and ownership, AVAudioUnitTimePitch speed processing, output-device routing, streaming append/flush/finish, one-shot preview/speak, enqueue and delivery accounting, route-aware capture grace, bounded native-completion fallbacks, interruption, explicit stop, output failure, and cleanup. Both previously reported issues are fixed in the current comparison: streaming playback preserves route-aware capture protection after drain, and missing completion bookkeeping is bounded for both streaming and one-shot playback. Final self-checks covered every changed user flow; accessibility and i18n/localization (no changed visual, interactive, or translatable UI surface); navigation and consent guards (not affected); async errors, never-completes, lifecycle, cancellation, and races; test honesty and discriminating coverage; project laws and design-system rules; duplicate/overlapping findings; and concrete evidence plus user effect for blocking severity. No supported concern survived. Supplied GitHub evidence was structurally valid and inspected: DCO, frontend build/tests, transcript virtualization, clean-room install, and macOS/Windows/Linux Rust checks all completed successfully, while combined status was pending; required checks still govern merge readiness. No local code, tests, builds, package managers, hooks, binaries, or project scripts were run.
Deterministic publication result: 0 blocking and 0 non-blocking finding(s) publishable; 0 duplicate(s) suppressed.
Pending checks: 1 check(s) are not complete.
This approval reflects the completed code review only; merge readiness remains governed by the repository's required checks.
Summary
Pocket speech at accelerated and reduced speeds on macOS now uses AVAudioUnitTimePitch, preserving natural pitch while avoiding the metallic and choppy artifacts from the previous time-stretching path. Playback at 1x bypasses the effect, selected output-device routing remains supported, and interruption accounting reports only audio that the native player completed.
The lower-quality sstretch dependency and cross-platform fallback are removed. Pocket playback is macOS-only in this final state.
Related issue
None found.
Reviewer-reproducible examples
Testing
John manually compared accelerated Pocket playback in a macOS development build and accepted the improved audio quality.