fix(macos): fullscreen dictation, correct text injection, and MLX backend crash#848
Open
hussaintrawadi wants to merge 3 commits into
Open
fix(macos): fullscreen dictation, correct text injection, and MLX backend crash#848hussaintrawadi wants to merge 3 commits into
hussaintrawadi wants to merge 3 commits into
Conversation
…o the focused app The floating dictation pill is an ordinary NSWindow, which macOS never admits to another app's native fullscreen Space. The pill therefore never appears in fullscreen, its WKWebView stays occluded, and getUserMedia never resolves, so recording only starts after leaving fullscreen. Convert the pill to a key-capable NSPanel (the window class fullscreen Spaces accept; key-capable so WebKit permits mic capture) with a CanJoinAllSpaces | FullScreenAuxiliary collection behavior, and order it front with orderFrontRegardless on every show. Also fix desktop text injection: capture_focus() read the system-wide focused element, which resolved to the pill itself while it briefly held key focus. That dropped the transcript (paste-into-self guard) and let the pill swallow the user's next keypress. Target the frontmost application instead (always the real dictation target, since the pill is a non-activating panel), fall back to it for apps that expose no AX focus (terminals, some Electron windows), and restore the pill without re-taking key focus after paste so the target keeps keyboard focus. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
mlx_lm binds a module-level GPU stream (mlx_lm/generate.py) to whichever thread first imports it. The server dispatches inference via asyncio.to_thread, whose pool hands successive calls to different worker threads, so that stream is missing on later calls. This surfaces as "There is no Stream(gpu, N) in current thread" for LLM refinement and, under worse timing, a hard SIGABRT inside mlx::core::metal::get_command_encoder that kills the backend. Route MLX Whisper (STT) load/transcribe and MLX Qwen (LLM) load/generate through a single-worker executor so GPU stream affinity is preserved and Metal command encoding is serialized. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The desktop host stops draining the sidecar's stdout/stderr once it sees the "Uvicorn running" line, which closes the pipe. Any later write (logging, tqdm) then raises BrokenPipeError and surfaces as a 500 from whatever request happened to log, observed as "[Errno 32] Broken pipe" during transcription. Wrap the frozen entry point's streams so pipe writes never raise. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughChanges route MLX model loading/inference through a dedicated single-thread executor instead of the generic thread pool, add a pipe-safe stdout/stderr wrapper in the backend server, and modify macOS Tauri focus capture fallback logic plus dictate pill window ordering and the paste_final_text command signature/flow. ChangesBackend MLX Threading and Pipe Safety
Estimated code review effort: 3 (Moderate) | ~25 minutes macOS Focus Capture and Dictate Pill Window Handling
Estimated code review effort: 4 (Complex) | ~50 minutes Sequence Diagram(s)sequenceDiagram
participant Backend as STT/LLM Backend
participant RunMLX as run_mlx
participant Executor as Single-thread Executor
Backend->>RunMLX: run_mlx(sync_fn, args)
RunMLX->>Executor: run_in_executor(sync_fn)
Executor-->>RunMLX: result
RunMLX-->>Backend: awaited result
sequenceDiagram
participant Hotkey as Hotkey/StartRecording
participant Window as Dictate Pill Window
participant PasteCmd as paste_final_text
participant Target as Target App
Hotkey->>Window: show() + force_order_front()
Window-->>Hotkey: window ordered to active Space
PasteCmd->>Target: check if frontmost, activate if needed
PasteCmd->>Window: hide pill webview
PasteCmd->>Target: send synthetic paste keystroke
PasteCmd->>Window: show pill webview + force_order_front()
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/backends/mlx_backend.py (1)
321-326: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRoute MLX Whisper unload through the dedicated worker (
backend/backends/mlx_backend.py:321-326)
unload_model()still mutatesself.modelon the caller thread, whiletranscribe()andload_model_async()run on the single MLX worker. Because unload is called synchronously from the service layer, this can race with an in-flight transcription and clearself.modelmid-call. Queue unload on the same worker, or guard load/transcribe/unload with a shared lock if the sync API has to stay.🤖 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 `@backend/backends/mlx_backend.py` around lines 321 - 326, The unload_model path still changes self.model on the caller thread, which can race with transcribe() and load_model_async() on the MLX worker. Update unload_model() in MLXBackend to run through the same dedicated worker used by transcribe() and load_model_async(), or add a shared lock around load/transcribe/unload so the model cannot be cleared mid-call.backend/backends/qwen_llm_backend.py (1)
205-217: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRoute MLX unload through the worker thread
unload_model()clearsself.model/self.tokenizeron the event-loop thread, but MLX load, inference, and unload are meant to stay on the single dedicated worker thread. That can race with an in-flightrun_mlx(self._generate_sync, ...)call and tear down state mid-generation. Route unload throughrun_mlxto keep the lifecycle serialized.🤖 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 `@backend/backends/qwen_llm_backend.py` around lines 205 - 217, The unload path in load_model currently calls unload_model() directly on the event-loop thread, which can race with MLX work on the dedicated worker thread. Update qwen_llm_backend.load_model so the existing model teardown is also routed through run_mlx, keeping _load_model_sync, _generate_sync, and unload serialized on the same worker thread. Preserve the current model-size checks and only trigger the threaded unload when switching models.
🧹 Nitpick comments (1)
backend/server.py (1)
51-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueApply Ruff SIM105 suggestion.
Static analysis flags this try/except/pass as a candidate for
contextlib.suppress.♻️ Proposed refactor
+import contextlib + def flush(self): - try: - self._base.flush() - except (BrokenPipeError, OSError): - pass + with contextlib.suppress(BrokenPipeError, OSError): + self._base.flush()🤖 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 `@backend/server.py` around lines 51 - 55, Replace the try/except/pass in Server.flush with contextlib.suppress to follow the Ruff SIM105 suggestion. Update the flush method on the Server class so the self._base.flush() call is wrapped in a suppress context for BrokenPipeError and OSError, removing the explicit exception handler while preserving the same behavior.Source: Linters/SAST tools
🤖 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 `@backend/server.py`:
- Around line 45-55: The stream wrapper in write and flush is catching too
broadly and still misses closed-stream errors; narrow the exception handling
around the _base.write and _base.flush calls to only the expected
broken-pipe/closed-stream cases, and avoid swallowing unrelated OSError
failures. Update the error handling in the stream wrapper methods (the write and
flush implementations) so stdout/stderr issues are handled explicitly rather
than masking other stream problems.
---
Outside diff comments:
In `@backend/backends/mlx_backend.py`:
- Around line 321-326: The unload_model path still changes self.model on the
caller thread, which can race with transcribe() and load_model_async() on the
MLX worker. Update unload_model() in MLXBackend to run through the same
dedicated worker used by transcribe() and load_model_async(), or add a shared
lock around load/transcribe/unload so the model cannot be cleared mid-call.
In `@backend/backends/qwen_llm_backend.py`:
- Around line 205-217: The unload path in load_model currently calls
unload_model() directly on the event-loop thread, which can race with MLX work
on the dedicated worker thread. Update qwen_llm_backend.load_model so the
existing model teardown is also routed through run_mlx, keeping
_load_model_sync, _generate_sync, and unload serialized on the same worker
thread. Preserve the current model-size checks and only trigger the threaded
unload when switching models.
---
Nitpick comments:
In `@backend/server.py`:
- Around line 51-55: Replace the try/except/pass in Server.flush with
contextlib.suppress to follow the Ruff SIM105 suggestion. Update the flush
method on the Server class so the self._base.flush() call is wrapped in a
suppress context for BrokenPipeError and OSError, removing the explicit
exception handler while preserving the same behavior.
🪄 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
Run ID: 1124ab45-48c7-41f5-b2de-2b958ac3f966
📒 Files selected for processing (7)
backend/backends/mlx_backend.pybackend/backends/qwen_llm_backend.pybackend/server.pybackend/services/mlx_thread.pytauri/src-tauri/src/focus_capture.rstauri/src-tauri/src/hotkey_monitor.rstauri/src-tauri/src/main.rs
Comment on lines
+45
to
+55
| def write(self, data): | ||
| try: | ||
| return self._base.write(data) | ||
| except (BrokenPipeError, OSError): | ||
| return len(data) | ||
|
|
||
| def flush(self): | ||
| try: | ||
| self._base.flush() | ||
| except (BrokenPipeError, OSError): | ||
| pass |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -e
git ls-files backend/server.py
echo '--- backend/server.py lines 1-120 ---'
cat -n backend/server.py | sed -n '1,120p'
echo '--- search _is_writable usages ---'
rg -n "_is_writable" backend/server.py . || trueRepository: jamiepine/voicebox
Length of output: 5493
🏁 Script executed:
sed -n '1,120p' backend/server.py | cat -n
rg -n "_is_writable|PipeSafeStream|sys\.stdout|sys\.stderr" backend/server.pyRepository: jamiepine/voicebox
Length of output: 5448
🏁 Script executed:
python3 - <<'PY'
import io, os, errno, tempfile
# Closed stream behavior
f = tempfile.TemporaryFile(mode="w+")
f.close()
try:
f.write("x")
except Exception as e:
print(type(e).__name__, repr(str(e)))
# Broken pipe behavior on this platform
r, w = os.pipe()
os.close(r)
try:
os.write(w, b"x")
except Exception as e:
print(type(e).__name__, getattr(e, "errno", None), repr(str(e)))
finally:
try: os.close(w)
except OSError: pass
print("BrokenPipeError is OSError:", issubclass(BrokenPipeError, OSError))
PYRepository: jamiepine/voicebox
Length of output: 276
Narrow the stream error handling. Catching OSError here swallows more than broken pipes, so unrelated stdout/stderr failures can disappear. A closed stream still raises ValueError, so this wrapper doesn’t cover that case either.
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 52-55: Use contextlib.suppress(BrokenPipeError, OSError) instead of try-except-pass
Replace try-except-pass with with contextlib.suppress(BrokenPipeError, OSError): ...
(SIM105)
🤖 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 `@backend/server.py` around lines 45 - 55, The stream wrapper in write and
flush is catching too broadly and still misses closed-stream errors; narrow the
exception handling around the _base.write and _base.flush calls to only the
expected broken-pipe/closed-stream cases, and avoid swallowing unrelated OSError
failures. Update the error handling in the stream wrapper methods (the write and
flush implementations) so stdout/stderr issues are handled explicitly rather
than masking other stream problems.
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
1 participant
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
Three independent bug fixes for macOS push-to-talk dictation, found while using
VoiceBox daily on an M4 / macOS 26. Each is a separate commit.
1. Dictation now works in native fullscreen apps —
fix(macos)The dictation pill was an ordinary
NSWindow, which macOS never places onanother app's native fullscreen Space. In fullscreen the pill therefore never
appeared, its
WKWebViewstayed occluded, andgetUserMedia()never resolved —so recording only began after leaving fullscreen. The pill is now a key‑capable
NSPanel(the window class fullscreen Spaces admit; key‑capable so WebKit stillgrants mic capture) with a
CanJoinAllSpaces | FullScreenAuxiliarycollectionbehavior, ordered front via
orderFrontRegardless.2. Correct text injection & keyboard focus —
fix(macos)capture_focus()read the system‑wide focused element, which resolved to thepill itself while it briefly held key focus. Two consequences:
recorded but never injected — reproducible in terminals and some Electron apps;
user's field.
Focus now targets the frontmost application (always the real dictation target,
since the pill is a non‑activating panel), falls back to it for apps that expose
no accessibility focus, and the pill is restored after paste without re‑taking
key focus so the target keeps keyboard focus.
3. MLX backend crash & refinement failure —
fix(mlx)+fix(backend)mlx_lmbinds a module‑level GPU stream(
mlx_lm/generate.py) to whichever thread first imports it, but the serverdispatches inference through an
asyncio.to_threadpool. Successive calls ondifferent workers hit
There is no Stream(gpu, N) in current thread(LLMrefinement) or, under worse timing, a hard
SIGABRTinsidemlx::core::metal::get_command_encoderthat takes down the backend. All MLXWhisper (STT) and Qwen (LLM) load/inference now runs on a single dedicated
worker thread, preserving stream affinity and serializing Metal.
after startup, closing the pipe; later writes raised
BrokenPipeErrorandsurfaced as a 500 (
[Errno 32] Broken pipe) during transcription. The frozenentry point's streams now swallow pipe errors.
Testing
over the fullscreen app, records, transcribes, and pastes.
no longer requires re‑focusing the field.
prewarm/transcribe/refine — previously failed on refinement and eventually
crashed (macOS crash report:
SIGABRTinget_command_encoder); now stableacross many rounds with no stream errors and no new crash reports.
cargo checkand Python syntax pass on a clean upstream checkout.Platform notes
Changes 1 and 2 are macOS‑only (
#[cfg(target_os = "macos")]). Change 3 helps anyApple‑Silicon user (the MLX crash) and every platform (the broken pipe).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes