Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion Cotabby/App/Coordinators/SuggestionCoordinator+Acceptance.swift
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,21 @@ extension SuggestionCoordinator {
insertionChunk: String,
liveContext: FocusedInputContext
) {
if overlayController.advanceInline(to: remainingText, insertedText: insertionChunk) {
// A layout-estimated anchor lives in the hidden text layout's coordinate system, and the
// estimator models the accepted chunk's true advance (soft wrap included) better than any
// width shift can: that re-anchor, fed with the pending insertion below, is exactly what
// the caret repair was built for on these hosts. Sliding instead leaves the anchor a
// ghost-vs-host font error away from the next estimate and the settle showed up as a
// post-accept jerk. Skip the slide so the estimator places the tail where the
// post-publish estimate will also land; nothing moves afterwards.
let heldOverlayQuality: CaretGeometryQuality?
if case let .visible(_, geometry, _) = overlayState {
heldOverlayQuality = geometry.caretQuality
} else {
heldOverlayQuality = nil
}
if heldOverlayQuality != .layoutEstimated,
overlayController.advanceInline(to: remainingText, insertedText: insertionChunk) {
return
}
Comment on lines +250 to 259

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 nil quality treated identically to a trusted caret

When overlayState is not .visible, heldOverlayQuality is nil, and nil != .layoutEstimated evaluates to true, so the code attempts advanceInline before falling back to the estimator. In practice presentAdvancedOverlay is only reached via the .advanced branch where an overlay should be visible, so the nil arm is unreachable under normal operation. However, if the overlay were hidden between acceptance validation and this call (e.g. by a concurrent reconcile), the nil case silently routes to the slide path rather than the estimator — the opposite of the safe default. A guard-with-early-fallthrough or an explicit nil → skip-slide mapping would make the intent clearer and remove the latent ambiguity.

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!

Fix in Codex Fix in Claude Code


Expand Down
2 changes: 1 addition & 1 deletion Cotabby/Support/BrowserAppDetector.swift
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import Foundation
///
/// Matching is by case-insensitive bundle-identifier prefix to tolerate channel suffixes
/// (`com.google.Chrome.canary`, `com.google.Chrome.beta`, etc.).
enum BrowserAppDetector {
nonisolated enum BrowserAppDetector {
/// Every browser family, used for the broad "typing in a browser" tone hint.
private static let browserBundlePrefixes: [String] = [
"com.apple.safari",
Expand Down
2 changes: 1 addition & 1 deletion Cotabby/Support/ControlTokenMarkers.swift
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import Foundation
/// into a text field expecting a completion, so removing them cannot eat legitimate prose or code.
/// `<think>…</think>` reasoning blocks are intentionally not listed here: they are handled separately
/// so the text around them survives.
enum ControlTokenMarkers {
nonisolated enum ControlTokenMarkers {
/// A Llama-3 role-header block, e.g. `<|start_header_id|>assistant<|end_header_id|>`. The role
/// name sits *between* the markers, so removing the markers individually would leak the role word
/// into the ghost text; this strips the whole block. `|` is escaped because it is a regex
Expand Down
22 changes: 16 additions & 6 deletions Cotabby/Support/SuggestionOverlayStabilityGate.swift
Original file line number Diff line number Diff line change
Expand Up @@ -74,12 +74,22 @@ enum SuggestionOverlayStabilityGate {
if isAwaitingPostInsertionSync {
return false
}
// Hold small caret deltas (post-insertion AX noise and exact-advance residual); re-anchor on
// genuine moves and on accumulated drift past the tolerance. Compared against the held
// (already-advanced) caret, not a per-tick previous value, so slow drift still gets corrected.
if abs(currentGeometry.caretRect.origin.x - newCaretRect.origin.x) > caretDriftTolerance
|| abs(currentGeometry.caretRect.origin.y - newCaretRect.origin.y) > caretDriftTolerance {
return true
// A layout-estimated anchor was computed by the hidden text layout from (text, field
// frame, style), not from the resolver's caret. Fresh snapshots still carry the RAW
// resolver rect (an AXFrame proportional guess, or a derived rect the repair overrode),
// which lives in a different trust system and routinely sits a word or more away from the
// estimate; treating that gap as caret drift re-presented (and re-estimated) on every
// reconcile tick. With text and field unchanged the estimate is pure-function stable, so
// only the frame check below can demand a re-anchor for these overlays.
if currentGeometry.caretQuality != .layoutEstimated {
// Hold small caret deltas (post-insertion AX noise and exact-advance residual); re-anchor
// on genuine moves and on accumulated drift past the tolerance. Compared against the held
// (already-advanced) caret, not a per-tick previous value, so slow drift still gets
// corrected.
if abs(currentGeometry.caretRect.origin.x - newCaretRect.origin.x) > caretDriftTolerance
|| abs(currentGeometry.caretRect.origin.y - newCaretRect.origin.y) > caretDriftTolerance {
return true
}
}
// `observedCharWidth` is intentionally NOT compared here. Drift in that value also affects
// `GhostSuggestionLayout.singleLineFits` (and therefore the panel-origin branch), so during
Expand Down
2 changes: 1 addition & 1 deletion Cotabby/Support/TerminalAppDetector.swift
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import Foundation
/// Terminal apps have their own completion, history, and shell integrations that conflict with
/// ghost-text autocomplete. Cotabby stays out of the way automatically so the user doesn't have to
/// manually disable each terminal they use.
enum TerminalAppDetector {
nonisolated enum TerminalAppDetector {
/// Bundle identifiers of well-known macOS terminal emulators.
private static let terminalBundleIdentifiers: Set<String> = [
"com.apple.Terminal",
Expand Down
36 changes: 36 additions & 0 deletions CotabbyTests/SuggestionCoordinatorPredictionTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,42 @@ final class SuggestionCoordinatorPredictionTests: XCTestCase {

// MARK: - Post-insertion stillness

func test_accept_layoutEstimatedOverlaySkipsTheSlideAndReAnchorsViaTheEstimator() {
// TextKit-mirror hosts: the overlay anchor came from the hidden layout estimate, so a
// width-based slide leaves it a ghost-vs-host font error away from the next estimate and
// the settle reads as a post-accept jerk. The accept must skip the slide and go through
// the presenting path, where the layout repair (fed the pending insertion) re-anchors at
// exactly the position the post-publish estimate will reproduce.
let rig = retained(makeCoordinatorRig())
let context = FocusedInputContext(snapshot: rig.focusProvider.snapshot.context!, generation: 1)
_ = rig.interactionState.startSession(fullText: " world again", liveContext: context, latency: 0.05)
rig.overlayController.showSuggestion(
" world again",
geometry: CotabbyTestFixtures.overlayGeometry(caretQuality: .layoutEstimated)
)

XCTAssertTrue(rig.coordinator.acceptCurrentSuggestion())

XCTAssertTrue(
rig.overlayController.advanceInlineCalls.isEmpty,
"A layout-estimated overlay must never width-slide on accept"
)
XCTAssertEqual(rig.overlayController.shownTexts.last, " again", "The estimator path re-presented the tail")
}

func test_accept_trustedGeometryStillAttemptsTheSlideFirst() {
let rig = retained(makeCoordinatorRig())
let context = FocusedInputContext(snapshot: rig.focusProvider.snapshot.context!, generation: 1)
_ = rig.interactionState.startSession(fullText: " world again", liveContext: context, latency: 0.05)
rig.overlayController.showSuggestion(" world again", geometry: CotabbyTestFixtures.overlayGeometry())

XCTAssertTrue(rig.coordinator.acceptCurrentSuggestion())

XCTAssertEqual(rig.overlayController.advanceInlineCalls.count, 1)
XCTAssertEqual(rig.overlayController.advanceInlineCalls.first?.remaining, " again")
XCTAssertEqual(rig.overlayController.advanceInlineCalls.first?.inserted, " world")
}

func test_reconcileDuringPostInsertionSyncWindow_neverReAnchorsTheOverlay() {
// The TextEdit accept jitter: Tab inserts " world" and the overlay advances immediately,
// but the +30ms refresh can read AX BEFORE the host publishes the insert. That snapshot's
Expand Down
8 changes: 8 additions & 0 deletions CotabbyTests/SuggestionCoordinatorTestSupport.swift
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,19 @@ final class RigOverlayController: SuggestionOverlayControlling {
var onStateChange: ((OverlayState) -> Void)?
private(set) var shownTexts: [String] = []
private(set) var hideReasons: [String] = []
/// Records slide attempts (and declines them, like the protocol default) so tests can assert
/// which accept paths even try to slide versus re-anchor through a present.
private(set) var advanceInlineCalls: [(remaining: String, inserted: String)] = []

init(state: OverlayState = .hidden(reason: "initial")) {
self.state = state
}

func advanceInline(to remainingText: String, insertedText: String) -> Bool {
advanceInlineCalls.append((remainingText, insertedText))
return false
}

func showSuggestion(_ text: String, geometry: SuggestionOverlayGeometry) {
shownTexts.append(text)
state = .visible(text: text, geometry: geometry, mode: .inline)
Expand Down
70 changes: 69 additions & 1 deletion CotabbyTests/SuggestionOverlayStabilityGateTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,13 @@ final class SuggestionOverlayStabilityGateTests: XCTestCase {
private static func geometry(
caretRect: CGRect = caretRect,
inputFrameRect: CGRect? = inputFrame,
caretQuality: CaretGeometryQuality = .exact,
focusChangeSequence: UInt64 = 7
) -> SuggestionOverlayGeometry {
SuggestionOverlayGeometry(
caretRect: caretRect,
inputFrameRect: inputFrameRect,
caretQuality: .exact,
caretQuality: caretQuality,
observedCharWidth: 8,
isRightToLeft: false,
focusChangeSequence: focusChangeSequence
Expand Down Expand Up @@ -330,4 +331,71 @@ final class SuggestionOverlayStabilityGateTests: XCTestCase {
)
)
}

// MARK: - Layout-estimated anchors (TextKit mirror hosts)

func test_layoutEstimatedAnchor_ignoresRawCaretDrift() {
// The held anchor came from the hidden text layout; fresh snapshots still carry the RAW
// resolver caret (an AXFrame proportional guess), which routinely sits a word or more
// away. Treating that gap as drift re-presented and re-estimated on every reconcile
// tick, and around accepts it was the jerk-left-then-back: with text and field unchanged
// the estimate cannot move, so the gate must hold.
let current: OverlayState = .visible(
text: " again",
geometry: Self.geometry(
caretRect: CGRect(x: 320, y: 210, width: 2, height: 18),
caretQuality: .layoutEstimated
),
mode: .inline
)

XCTAssertFalse(
SuggestionOverlayStabilityGate.shouldRePresent(
currentOverlay: current,
newText: " again",
newCaretRect: Self.caretRect,
newInputFrameRect: Self.inputFrame,
newFocusChangeSequence: 7
)
)
}

func test_layoutEstimatedAnchor_stillReAnchorsOnFrameTextOrFieldChange() {
// The estimate is a pure function of (text, field frame, style): when one of its real
// inputs changes, or the field itself does, the re-anchor must still happen.
let current: OverlayState = .visible(
text: " again",
geometry: Self.geometry(caretQuality: .layoutEstimated),
mode: .inline
)

XCTAssertTrue(
SuggestionOverlayStabilityGate.shouldRePresent(
currentOverlay: current,
newText: " again",
newCaretRect: Self.caretRect,
newInputFrameRect: Self.inputFrame.offsetBy(dx: 40, dy: 0),
newFocusChangeSequence: 7
),
"A field-frame move re-positions the estimate and must re-anchor"
)
XCTAssertTrue(
SuggestionOverlayStabilityGate.shouldRePresent(
currentOverlay: current,
newText: " different",
newCaretRect: Self.caretRect,
newInputFrameRect: Self.inputFrame,
newFocusChangeSequence: 7
)
)
XCTAssertTrue(
SuggestionOverlayStabilityGate.shouldRePresent(
currentOverlay: current,
newText: " again",
newCaretRect: Self.caretRect,
newInputFrameRect: Self.inputFrame,
newFocusChangeSequence: 8
)
)
}
}