Skip to content

Fix accept keybind swallowing keystroke during background refresh#345

Merged
FuJacob merged 1 commit into
mainfrom
fix-accept-keybind-state-race
May 28, 2026
Merged

Fix accept keybind swallowing keystroke during background refresh#345
FuJacob merged 1 commit into
mainfrom
fix-accept-keybind-state-race

Conversation

@FuJacob
Copy link
Copy Markdown
Owner

@FuJacob FuJacob commented May 28, 2026

Summary

The accept keybind was gated on state == .ready in SuggestionCoordinator+Acceptance.swift, but the state machine transitions to .debouncing / .generating while a previously ready suggestion is still buffered and its overlay is still on screen. This happens most reliably when the visual-context coordinator finishes OCR and calls schedulePredictionForCurrentFocusIfPossible, which fires schedulePrediction() without first clearing the active session or hiding the overlay. Because the accept tap is installed for the duration of overlay visibility, the user's accept keystroke is consumed by the tap, passTabThrough runs (no insertion), and the in-flight background regeneration then replaces the suggestion the user was trying to take. End result matches the reported "swallowed accept + regeneration" symptom.

Gate on interactionState.activeSession instead. validateSessionForAcceptance still reconciles against the live AX snapshot and rejects the accept when the buffered session no longer matches the field.

Validation

swiftlint lint --quiet Cotabby/App/Coordinators/SuggestionCoordinator+Acceptance.swift
# exit 0, no warnings

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 **

test-without-building failed to load CotabbyTests.xctest with the local-signing Team ID mismatch (xctest bundle signed with the user dev cert but loaded into the host app process). CLAUDE.md calls this out explicitly as a local-environment failure, not a code regression, and instructs to proceed once build-for-testing succeeds.

Linked issues

None filed for this specific symptom yet.

Risk / rollout notes

Behavioral change is narrow: an accept keystroke that previously fell into passTabThrough solely because state was .debouncing / .generating (while activeSession was still set) will now flow into prepareAcceptance and attempt to insert. validateSessionForAcceptance still rejects on selected text, prefix/trailing-text divergence, process change, or session exhaustion, so a stale suggestion does not get inserted into a field the user has since edited. Every site that clears activeSession also hides the overlay, so the existing invariant "overlay visible iff activeSession != nil" still drives the accept tap's lifecycle.

Greptile Summary

This PR fixes a keystroke-swallowing bug where a Tab/accept keypress was silently dropped during a background suggestion refresh. The previous gate (guard case .ready = state) was too strict because schedulePrediction() transitions state to .debouncing without clearing activeSession or hiding the overlay, so the accept tap remained installed but the keystroke fell through to passTabThrough instead of prepareAcceptance.

  • Core change: The guard in acceptSuggestion now checks interactionState.activeSession != nil instead of state == .ready, allowing acceptance when a previous suggestion is buffered and on screen even while a background regeneration is in flight.
  • Safety preserved: validateSessionForAcceptance still reconciles against the live AX snapshot and rejects accepts on selected text, prefix/trailing-text divergence, process change, and exhausted sessions — so no stale suggestion can be incorrectly inserted.
  • Invariant confirmed: Every code path that sets activeSession = nil also calls hideOverlay, and the accept tap is driven by overlay visibility; so activeSession != nil ↔ overlay visible continues to hold.

Confidence Score: 5/5

Safe to merge; the narrowly scoped guard change is backed by robust session-reconciliation validation and the accept tap's existing lifecycle invariants.

The one-line change is correct: activeSession != nil accurately captures "a buffered suggestion is still available," while the old state == .ready was incorrectly rejecting accepts during background refreshes. validateSessionForAcceptance provides multiple independent rejection conditions (selected text, prefix divergence, process change, exhausted session) so no stale suggestion can slip through. All activeSession = nil sites also call hideOverlay, preserving the overlay-visibility invariant that gates the accept tap. The @MainActor isolation means there are no race conditions between the async generation task and the synchronous accept path.

No files require special attention.

Important Files Changed

Filename Overview
Cotabby/App/Coordinators/SuggestionCoordinator+Acceptance.swift Replaces the state == .ready guard with activeSession != nil; the one-line logic change is correct and well-protected by downstream validateSessionForAcceptance reconciliation.

Sequence Diagram

sequenceDiagram
    participant User
    participant AcceptTap as Accept Tap (overlay visible)
    participant acceptSuggestion
    participant validate as validateSessionForAcceptance
    participant schedulePrediction

    Note over schedulePrediction: OCR finishes → schedulePrediction() called
    schedulePrediction->>schedulePrediction: "state = .debouncing (activeSession still set)"

    User->>AcceptTap: Tab keystroke
    AcceptTap->>acceptSuggestion: acceptSuggestion()

    Note over acceptSuggestion: OLD: guard case .ready = state → passTabThrough (swallowed)
    Note over acceptSuggestion: NEW: guard activeSession != nil → proceeds

    acceptSuggestion->>validate: prepareAcceptance(from: snapshot)
    validate->>validate: reconcile vs live AX snapshot
    alt reconciliation passes
        validate-->>acceptSuggestion: .ready(liveContext, session, chunk)
        acceptSuggestion->>acceptSuggestion: insert chunk, cancelPredictionWork()
        acceptSuggestion-->>User: suggestion accepted
    else reconciliation fails
        validate-->>acceptSuggestion: .invalid(reason)
        acceptSuggestion-->>User: passTabThrough (safe fallback)
    end
Loading

Reviews (1): Last reviewed commit: "Gate accept on the live session instead ..." | Re-trigger Greptile

The accept-keybind path guarded on `state == .ready`, but `state` can
transition to `.debouncing` / `.generating` while a previously ready
suggestion is still buffered and its overlay is still on screen — most
notably when the visual-context coordinator finishes OCR and calls
`schedulePredictionForCurrentFocusIfPossible`. The accept tap is
installed for the duration of overlay visibility, so the keystroke is
consumed by the tap while `passTabThrough` runs in the coordinator: no
text is inserted, and the in-flight background regeneration then
replaces the suggestion the user was trying to take.

Gate on `interactionState.activeSession` instead. The existing
`validateSessionForAcceptance` reconciliation still rejects the accept
when the live AX state no longer matches the buffered session.
@FuJacob FuJacob merged commit 4d4a271 into main May 28, 2026
4 checks passed
@FuJacob FuJacob deleted the fix-accept-keybind-state-race branch May 28, 2026 08:59
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.

1 participant