Run focus AX read off the synchronous keyboard event tap#315
Open
FuJacob wants to merge 1 commit into
Open
Conversation
The keyboard monitor is an active session event tap that the system routes the live input stream through and waits on. On keystrokes that schedule a prediction, the coordinator called FocusTracker.refreshNow() inline, which performs a blocking Accessibility tree walk. Against an unresponsive app that walk can exceed the tap deadline, so macOS disables the tap and drops the in-flight events queued behind it. Dropped key-ups strand held keys (movement keys keep firing) and desync modifier shortcuts. Defer the AX refresh and prediction scheduling to the next main-actor turn so the keystroke flows through the tap untouched. The debounced generation path already re-reads focus, so nothing downstream depends on the inline read.
Comment on lines
+152
to
+158
| func scheduleDeferredFocusRefreshAndPrediction() { | ||
| Task { @MainActor [weak self] in | ||
| guard let self else { return } | ||
| self.focusModel.refreshNow() | ||
| self.schedulePrediction() | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
scheduleDeferredFocusRefreshAndPrediction is only called from within this file, yet it sits in the non-private extension and is therefore visible at module scope. All three call sites are in the same file, and the existing internal-only formatting helpers (focusDiagnostics, formatRect) already live in the private extension block below. Moving this method there (or adding an explicit private modifier) keeps the public surface of SuggestionCoordinator consistent with the existing convention and avoids accidental callers in other coordinator files.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
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
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
On keystrokes that schedule a prediction, the suggestion coordinator called
FocusTracker.refreshNow()directly inside the keyboard event-tap callback. That callback runs synchronously inside an active.cgSessionEventTapthat the system routes the live input stream through and blocks on, andrefreshNow()performs a blocking Accessibility tree walk. Against an unresponsive frontmost app that walk can exceed the tap deadline, so macOS disables the tap and drops the events queued behind it. Dropped key-ups leave keys logically held (continuous movement keys keep firing) and desync modifier shortcuts. This moves the AX refresh and prediction scheduling onto the next main-actor turn so the keystroke flows through the tap untouched.Validation
The deferred read is safe because the debounced generation path (
generateFromCurrentFocus) already re-reads focus before generating, so nothing downstream depends on the inline snapshot. The acceptance path already deferred its AX refresh viaasyncAfter, so it was unaffected.Linked issues
Risk / rollout notes
Greptile Summary
This PR fixes a macOS input-stream reliability issue by moving the blocking Accessibility tree walk (
FocusTracker.refreshNow()) off the synchronous CGEvent tap callback and onto the next main-actor turn viaTask { @MainActor }. Previously, a slow or unresponsive frontmost app could cause the AX read to exceed the tap deadline, causing macOS to disable the tap entirely and drop key-up events.scheduleDeferredFocusRefreshAndPrediction(), which wraps therefreshNow()+schedulePrediction()sequence in a[weak self]main-actor task, replacing three inline call sites across the active-session and default input-event paths.generateFromCurrentFocusalready performs its ownrefreshNow()re-read after the debounce window expires, so no downstream path depends on the inline snapshot being current.Confidence Score: 4/5
Safe to merge; the timing change is well-contained and the generation path already re-reads focus independently.
The core fix is correct: deferring the blocking AX walk off the event tap prevents tap disablement under slow apps, and the weak self task is safe against coordinator deallocation. The only finding is that the new helper method is inadvertently exposed at module scope instead of being private, which is a minor inconsistency with the existing convention in this file. Nothing in the changed logic affects correctness.
No files require special attention beyond the access-control nit on the new helper method in SuggestionCoordinator+Input.swift.
Important Files Changed
Task { @MainActor }. Logic is sound; the new helper method should beprivateto match the existing convention for internal-only helpers in this file.Sequence Diagram
sequenceDiagram participant ET as CGEvent Tap (sync) participant SC as SuggestionCoordinator participant MA as Main Actor Queue participant FM as FocusModel participant WC as WorkController Note over ET,SC: Before this PR ET->>SC: handleInputEvent(event) SC->>FM: refreshNow() [blocks tap] FM-->>SC: snapshot updated SC->>WC: schedulePrediction() SC-->>ET: return false Note over ET,SC: After this PR ET->>SC: handleInputEvent(event) SC->>MA: "Task @MainActor enqueued" SC-->>ET: return false (tap unblocked) MA->>FM: refreshNow() [next turn] FM-->>MA: snapshot updated MA->>WC: schedulePrediction() WC->>WC: replaceDebouncedWork(delay: debounceMs) Note over WC: generateFromCurrentFocus re-reads focus after debounceReviews (1): Last reviewed commit: "Run focus AX read off the synchronous ke..." | Re-trigger Greptile