Hold overlay position across post-accept AX reconciles#349
Merged
Conversation
- Pin the 1pt tolerance contract with `test_inputFrameAtExactTolerance_holdsGeometry`. The gate uses a strict `>` comparison so a drift of exactly 1.0pt is held; a future change to `>=` will now flip this test instead of silently changing behavior. - Document why `observedCharWidth` is intentionally outside the gate. Including it would re-introduce the post-accept jitter this file exists to suppress; the drag-time tradeoff is acceptable and the right fix for any wrong-layout frame in practice would live in `GhostSuggestionLayout`.
2e2ec07 to
7b2e84a
Compare
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
The ghost-text overlay visibly shifted slightly LEFT and DOWN for one frame on every Tab accept, then snapped back. The fix holds the existing overlay geometry across reconcile ticks whenever the field, displayed text, and on-screen field bounds have not materially changed; legitimate context changes (window drag, field switch, new text) still re-anchor.
Cause
Tab acceptance produces two overlay renders in succession:
acceptSuggestioncallspresentOverlay(at: predictedCaret, ...)using apredictedCaretRectcomputed from the pre-accept caret plus the chunk width.schedulePostInsertionRefreshcallsfocusModel.refreshNow()and thenreconcileActiveSession(with: focusModel.snapshot), which in turn callspresentOverlay(at: liveContext.caretRect, ...)again.In the second render, AX commonly returns slightly different values for the same underlying state:
caretRectdrifts by a sub-pixel to a few-pixel amount as the host's caret rendering settles.observedCharWidthis recomputed from the field's child text runs whose composition changed when the inserted chunk committed. Even a small change can flipGhostSuggestionLayout.singleLineFits, switchingpanelOriginXfromfirstLineAnchor(right of caret) tousableFrame.minX(field left edge) and adding a-lineHeightY offset. That is the left-and-down shift the user sees.caretQualitycan momentarily degrade from.exactto.derivedor.estimatedif AX is mid-rerender, which pushes the layout through a different fallback branch.The reconcile call site at
SuggestionCoordinator+Prediction.swift:317re-presented the overlay unconditionally, so any of these drifted values caused a one-frame jitter even though the underlying state had not meaningfully changed.Fix
New pure helper
SuggestionOverlayStabilityGate.shouldRePresent(...)(Cotabby/Support/) decides whether a reconcile tick should reposition the overlay. It returns:trueif the overlay is hidden (fresh show), the focus session changed, the displayed text changed, or the host editor's frame moved on screen beyond a 1pt tolerance.falseotherwise (the post-accept jitter case): hold the geometry from the priorpresentOverlaycall exactly as it was last drawn.The reconcile branch in
SuggestionCoordinator+Prediction.swiftnow callspresentOverlayonly when the gate returnstrue. The +30ms post-insertion path therefore no longer re-anchors against drifted AX measurements, while window drags and field switches still re-anchor.The 1pt input-frame tolerance is the smallest amount that swallows sub-pixel AX noise on mixed Retina / non-Retina setups; whole-pixel window movement still trips the gate.
Validation
xcodebuild -project Cotabby.xcodeproj -scheme Cotabby -destination 'platform=macOS' build CODE_SIGNING_ALLOWED=NO→
** BUILD SUCCEEDED **xcodebuild test ... -only-testing:CotabbyTests CODE_SIGNING_ALLOWED=NO→
Executed 381 tests, with 1 test skipped and 0 failures (0 unexpected)(the skipped test is the gated FM eval, unrelated)swiftlint lint --quiet→ no violations.8 new unit tests in
SuggestionOverlayStabilityGateTestscover: hidden→render, identical-state→hold, focus-session-changed→render, text-changed→render, frame-moved→render, sub-pixel-frame-noise→hold, frame-appeared/disappeared→render, both-frames-nil→hold.Manual check expected after merge: focus a real text field, generate a suggestion, press Tab to accept one word, and watch the remaining ghost text. It should advance to the new caret position once and stay there without the post-Tab jitter.
Linked issues
(none filed; reported in-conversation by the project owner)
Risk / rollout notes
case .validbranch ofreconcileActiveSession. All other overlay paths (apply(result:)on a fresh generation, the partial-acceptpresentOverlayin+Acceptance.swift,advanceActiveSessionIfTypedCharactersMatch) callpresentOverlaydirectly and are unaffected.project.ymlis the source of truth for the Xcode project. The two new files (Cotabby/Support/SuggestionOverlayStabilityGate.swift,CotabbyTests/SuggestionOverlayStabilityGateTests.swift) are picked up automatically by XcodeGen;xcodegen generatewas rerun and the resultingCotabby.xcodeproj/project.pbxprojis committed.acceptSuggestionsignificantly wrong, the gate would now keep the overlay at that wrong position until the user accepts another chunk or the field changes. The eval is the right place to catch that: the 1pt tolerance is intentionally tight enough that a real misprediction would also moveinputFrameRect(the caret implementation in AppKit/AX typically tracks the field origin), but if a host app turned out to move the caret without moving the field bounds, we'd need to add a separate caret-delta threshold. Calling that out so the next reviewer of this area knows the tradeoff.Greptile Summary
This PR fixes a one-frame ghost-text jitter visible after every Tab accept by introducing
SuggestionOverlayStabilityGate, a pure helper that gates the reconcile path'spresentOverlaycall. The gate holds existing overlay geometry when the focus session, displayed text, and input-frame position are all stable within a 1pt tolerance; legitimate re-anchors (window drag, field switch, text change) still pass through.SuggestionOverlayStabilityGateenum (Cotabby/Support/) with a single static methodshouldRePresent; it comparesfocusChangeSequence, text, and all four rect dimensions against storedOverlayStategeometry and returns early-out booleans for each re-anchor condition.SuggestionCoordinator+Prediction.swiftcall site (line 326) wraps the existingpresentOverlaycall in anif shouldRePresentguard, leaving all other overlay paths (fresh generation, partial accept in+Acceptance.swift, typed-through advance) unaffected.Confidence Score: 5/5
Safe to merge; the gate is narrowly scoped to the reconcile path and all other overlay call sites are unaffected.
The change is a pure, side-effect-free predicate inserted at one call site. It cannot suppress a legitimate re-anchor because every re-anchor condition (focus session change, text change, frame movement beyond 1pt) is explicitly covered, and the guard defaults to re-presenting when overlayState is hidden. The 1pt tolerance boundary is pinned by a dedicated test so the contract is explicit. No existing tests were modified, and the build and full test suite pass.
No files require special attention.
Important Files Changed
Sequence Diagram
sequenceDiagram participant User participant Coordinator as SuggestionCoordinator participant Gate as StabilityGate participant Overlay as OverlayPresenter participant AX as Accessibility API User->>Coordinator: Tab accept Coordinator->>AX: predictedCaretRect (pre-accept) Coordinator->>Overlay: presentOverlay(predictedCaret) — immediate Overlay-->>Coordinator: "overlayState = .visible(text, geometry)" Note over Coordinator: +30ms schedulePostInsertionRefresh Coordinator->>AX: refreshNow() → reconcileActiveSession AX-->>Coordinator: liveContext (drifted caretRect / charWidth) Coordinator->>Gate: shouldRePresent(overlayState, newText, newInputFrameRect, newFocusChangeSequence) alt "text changed OR frame moved >1pt OR focus session changed" Gate-->>Coordinator: true → re-anchor Coordinator->>Overlay: presentOverlay(liveContext.caretRect) else same text, stable frame, same session Gate-->>Coordinator: false → hold geometry Note over Overlay: Overlay stays at predicted position (no jitter) endReviews (3): Last reviewed commit: "Address Greptile review on overlay stabi..." | Re-trigger Greptile