Fix final Tab acceptance routing#387
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let capturedEvent = classify(event: event) | ||
| guard !capturedEvent.kind.isAcceptance else { | ||
| // Acceptance is handled by the active default tap, because only that callback can | ||
| // make insertion and "consume the original key" one atomic decision. If the active | ||
| // tap is absent, the Tab naturally belongs to the focused app. | ||
| return Unmanaged.passUnretained(event) | ||
| } | ||
|
|
||
| _ = onEvent?(capturedEvent) |
There was a problem hiding this comment.
isAcceptance guard fires after classify() has already done its work — reading provider closures, comparing key codes, and constructing a CapturedInputEvent. Because every Tab keystroke reaches both taps and the observer always skips acceptance events, this is wasted per-keystroke classification work on a hot path. Moving the comment before classify() avoids the allocation for the one event class that most frequently arrives while the overlay is visible.
| let capturedEvent = classify(event: event) | |
| guard !capturedEvent.kind.isAcceptance else { | |
| // Acceptance is handled by the active default tap, because only that callback can | |
| // make insertion and "consume the original key" one atomic decision. If the active | |
| // tap is absent, the Tab naturally belongs to the focused app. | |
| return Unmanaged.passUnretained(event) | |
| } | |
| _ = onEvent?(capturedEvent) | |
| // Acceptance is handled by the active default tap, because only that callback can | |
| // make insertion and "consume the original key" one atomic decision. If the active | |
| // tap is absent, the Tab naturally belongs to the focused app. | |
| let capturedEvent = classify(event: event) | |
| guard !capturedEvent.kind.isAcceptance else { | |
| return Unmanaged.passUnretained(event) | |
| } | |
| _ = onEvent?(capturedEvent) |
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!
| /// Listen-only observer: classifies the event and notifies the coordinator. The return value | ||
| /// of `onEvent` is ignored here because a listen-only tap cannot drop or modify events. | ||
| /// Consumption of the accept key is handled by the separate active accept tap. | ||
| private func handleObserverTap(type: CGEventType, event: CGEvent) -> Unmanaged<CGEvent>? { | ||
| /// Handles the listen-only tap callback. Internal so tests can lock down which tap owns | ||
| /// acceptance without installing real global event taps. | ||
| func handleObserverTap(type: CGEventType, event: CGEvent) -> Unmanaged<CGEvent>? { | ||
| switch type { | ||
| case .tapDisabledByTimeout, .tapDisabledByUserInput: | ||
| CotabbyLogger.app.warning("Observer tap was disabled by system, re-enabling") |
There was a problem hiding this comment.
handleObserverTap and handleAcceptTap each have two independent doc-comment paragraphs pasted together without a blank /// separator — the original functional description followed immediately by the new "Internal so tests can…" sentence. Adding a blank /// line between them or merging into a single coherent paragraph would fix the readability break. The same issue exists for handleAcceptTap at its corresponding location.
1 participant
Summary
Route accept-key handling through the active consuming event tap so insertion and original Tab suppression are decided in the same callback. This fixes the final-token case where accepting a punctuation-only tail could hide/exhaust the suggestion before the accept tap decided whether to swallow the original Tab.
Also removes the synthetic replay fallback and aligns accept preflight with the actual user-visible invariant: a visible overlay with an active session, even if background refresh work has moved coordinator state out of
.ready.Validation
xcodebuild -project Cotabby.xcodeproj -scheme Cotabby -destination 'platform=macOS' build** BUILD SUCCEEDED **xcodebuild -project Cotabby.xcodeproj -scheme Cotabby -destination 'platform=macOS' build-for-testing** TEST BUILD SUCCEEDED **xcodebuild -project Cotabby.xcodeproj -scheme Cotabby -destination 'platform=macOS' test -only-testing:CotabbyTests/InputMonitorTestsCotabbyTests.xctestbundle could not be loaded:mapping process and mapped file (non-platform) have different Team IDs.Linked issues
Risk / rollout notes
Greptile Summary
This PR fixes the final-token Tab acceptance bug by making the consuming accept tap the sole owner of both insertion and original-key suppression, eliminating the race where the observer tap could hide the overlay before the accept tap decided whether to swallow the Tab. The synthetic replay path (
replayConsumedAcceptKey) is also removed since afalsereturn fromonEventnow naturally passes the original event through without any re-posting.handleAcceptTapnow constructs theCapturedInputEventitself and callsonEventdirectly; the result (true= consume,false= pass through) is used atomically to decide the callback's return value.handleObserverTapexplicitly skips acceptance events with a newisAcceptanceguard, keeping the observer strictly listen-only for all non-acceptance keys.shouldConsumeAcceptKeyProviderdrops the.readystate requirement, allowing acceptance while a background OCR refresh has movedstateto.debouncing— the full AX/session validation insideacceptSuggestionstill rejects stale sessions.Confidence Score: 4/5
Safe to merge; the fix correctly unifies insertion and Tab suppression into a single atomic callback, and the relaxed preflight still has a full AX/session validation backstop inside acceptSuggestion.
The architectural change is well-reasoned and the new design eliminates the race it targets. The two findings are style/efficiency issues only — a minor classify() call that runs unnecessarily for every Tab event, and concatenated doc-comments on the two tap handlers. No logic correctness concerns were identified.
InputMonitor.swift deserves a second look for the classify() placement and the doc-comment concatenation; all other files are straightforward parameter removals and routing simplifications.
Important Files Changed
Sequence Diagram
sequenceDiagram participant User as User (Tab key) participant ObsTap as Observer Tap (head, listen-only) participant AccTap as Accept Tap (tail, consuming) participant Coord as SuggestionCoordinator participant App as Focused App User->>ObsTap: keyDown (Tab) ObsTap->>ObsTap: classify() → .acceptance ObsTap->>ObsTap: guard !isAcceptance → skip onEvent ObsTap-->>AccTap: event passes through (listen-only) AccTap->>AccTap: matches acceptanceKeyCode? AccTap->>AccTap: shouldConsumeAcceptKeyProvider() alt Preflight fails (stale tap / no session) AccTap-->>App: passUnretained(event) — Tab reaches app else Preflight passes AccTap->>Coord: onEvent(CapturedInputEvent(.acceptance)) Coord->>Coord: handleInputEvent → acceptCurrentSuggestion() alt Acceptance succeeds Coord-->>AccTap: return true AccTap-->>App: return nil (Tab consumed) else Acceptance declined (passTabThrough) Coord->>Coord: clearSuggestion + hideOverlay → destroyAcceptTap Coord-->>AccTap: return false AccTap-->>App: passUnretained(event) — Tab reaches app end endReviews (1): Last reviewed commit: "Fix final Tab acceptance routing" | Re-trigger Greptile