Hold TextKit-mirror caret anchors perfectly still through word accepts#691
Conversation
The accept jitter report was about hosts whose caret comes from the hidden TextKit layout estimate (.layoutEstimated), not native-exact hosts. The merged publish-window hold already stops the pre-publish re-estimate there (the mirror re-ran against the old text and anchored a full word left), but two mirror-specific motion sources remained: - The accept-time width slide pre-empted the estimator re-anchor that the caret repair was designed to perform on these hosts. The slid anchor then disagreed with the next estimate by the ghost-vs-host font error, and the settle read as a jerk. Accepts on a layout-estimated overlay now skip the slide and re-present through the repair, which is fed the pending insertion and lands exactly where the post-publish estimate will. - The stability gate compared the held mirror anchor against the RAW resolver caret (an AXFrame proportional guess, or an overridden derived rect), a different coordinate system that routinely sits a word away, so every reconcile tick re-presented and re-estimated. For layout-estimated overlays the gate now ignores raw-caret drift: the estimate is a pure function of text, field frame, and style, so only those inputs (or a field switch) re-anchor. Also annotates ControlTokenMarkers, TerminalAppDetector, and BrowserAppDetector nonisolated: three Swift 6 forward-compat warnings that arrived with the surface-classifier merge, same root cause as #684.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 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 | ||
| } |
There was a problem hiding this comment.
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!
1 participant
Summary
Follow-up to #690 targeting the surface the jitter report was actually about: hosts whose caret anchor comes from the hidden TextKit layout estimate (
.layoutEstimated, the #670 mirror), where every word accept jerked the remaining ghost left and then back right.The publish-window hold merged in #690 already removes the biggest mechanism here (it is quality-agnostic): during the pre-publish window the +30ms refresh used to re-run the mirror against the OLD text with no pending insertion, anchoring the tail a full word left, and the next poll snapped it back. Two mirror-specific motion sources remained:
advanceInlineslid the panel by a font-measured width, bypassing exactly the re-anchor the caret repair was designed to perform on these hosts (the estimator is fed the pending insertion and models the true advance, soft wrap included). The slid anchor then disagreed with the next estimate by the ghost-vs-host font error, and the settle read as a jerk. Accepts on a layout-estimated overlay now skip the slide and re-present through the repair, landing exactly where the post-publish estimate will reproduce, so the accept itself is the only movement.Net accept cycle on a mirror host: one anchored advance at the keystroke (estimator with pending insertion), hold through the publish window (#690), hold on the post-publish tick (the fresh estimate is byte-identical to the held one, and raw drift is no longer consulted). Nothing moves after the accept.
Also annotates
ControlTokenMarkers,TerminalAppDetector, andBrowserAppDetectorasnonisolated: three Swift 6 forward-compat warnings that arrived with the surface-classifier merge, same root cause and same fix pattern as #684, restoring the zero-warning build.Validation
New regression locks:
.layoutEstimatedoverlay never callsadvanceInlineand re-presents the tail through the estimator path; an accept on trusted-exact geometry still attempts the slide first with the right arguments.Linked issues
Refs #690 and #670. Reported directly: "if text mirror is used to create caret position, every time accept word, the suggestion jerks left then goes back".
Risk / rollout notes
.layoutEstimated; exact and derived hosts keep Keep ghost text perfectly still through word accepts (TextEdit jitter) #690's behavior bit-for-bit.Greptile Summary
This PR fixes post-accept caret jitter on TextKit-mirror hosts (overlays whose position comes from the hidden layout estimator rather than raw AX caret data) by addressing two root causes that #690's publish-window hold did not cover.
SuggestionCoordinator+Acceptance.swift):presentAdvancedOverlaynow skipsadvanceInline(width-based slide) for layout-estimated overlays and routes directly throughpresentOverlay, where the layout repair is fed the pending insertion and places the tail exactly where the post-publish estimate will reproduce it.SuggestionOverlayStabilityGate.swift): The caret-drift re-anchor check is skipped for.layoutEstimatedquality; only frame moves, text changes, and focus-sequence changes can demand a re-anchor, eliminating the per-tick false-drift that the raw resolver caret was causing.BrowserAppDetector,ControlTokenMarkers, andTerminalAppDetectorare annotatednonisolatedfollowing the same pattern as Eliminate all 68 Swift 6 forward-compat build warnings #684.Confidence Score: 4/5
Safe to merge for the targeted hosts; the behavior change is narrowly scoped to
.layoutEstimatedoverlays and exact/derived hosts are unaffected.Core logic is correct and backed by focused regression tests. Two spots in
SuggestionCoordinator+Acceptance.swiftwarrant attention: thenilquality arm silently routes to the slide rather than the estimator, and the type-ahead path lacks the matching guard for layout-estimated overlays.Cotabby/App/Coordinators/SuggestionCoordinator+Acceptance.swift — the
nilquality branch inpresentAdvancedOverlayand the unguarded slide inadvanceActiveSessionIfTypedCharactersMatchare the two spots worth a second look.Important Files Changed
presentAdvancedOverlayto skipadvanceInlineand route through the estimator path; theniloverlay-state arm and the unguarded type-ahead path are worth a follow-upcaretQuality != .layoutEstimated; only frame, text, and focus-sequence checks remain active for mirror-host overlays — well-reasoned and correctly scopednonisolatedkeyword to the enum for Swift 6 forward-compatibility; no behaviour changenonisolatedkeyword to the enum for Swift 6 forward-compatibility; no behaviour changenonisolatedkeyword to the enum for Swift 6 forward-compatibility; no behaviour changeadvanceInlineoverride toRigOverlayControllerthat records calls and always returnsfalse; allows tests to distinguish slide-attempt paths from re-present pathscaretQualityFlowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Word Accept - Tab keystroke] --> B{overlayState visible?} B -- No --> C[heldOverlayQuality = nil] B -- Yes --> D[heldOverlayQuality = geometry.caretQuality] C --> E{quality != .layoutEstimated?} D --> E E -- Yes, nil or exact/derived --> F[advanceInline attempt] F -- returns true --> G[Slide succeeded, return] F -- returns false --> H[Fall through to estimator path] E -- No, .layoutEstimated --> H H --> I[predictedCaretRect from liveContext] I --> J[presentOverlay with pendingInsertion] J --> K[layoutRepairedAnchor runs estimator] K --> L[Anchor placed by layout estimate] subgraph StabilityGate [Reconcile tick — shouldRePresent] M{caretQuality == .layoutEstimated?} -- Yes --> N[Skip caret-drift check] M -- No --> O[Check caret drift > 6pt] O -- Exceeds tolerance --> P[Re-anchor] N --> Q[Frame / text / focus-seq checks only] Q --> R{Any real input changed?} R -- Yes --> P R -- No --> S[Hold geometry] endComments Outside Diff (1)
Cotabby/App/Coordinators/SuggestionCoordinator+Acceptance.swift, line 509-518 (link)advanceActiveSessionIfTypedCharactersMatchstill callsadvanceInlineunconditionally; if the slide returnsfalseit falls back tosession.baseContext.caretRect, the raw resolver caret from session start. For a layout-estimated overlay that is the same coordinate mismatch this PR eliminates for Tab-accepts, meaning a user who types through the suggestion on a TextKit-mirror host could still see a one-frame jitter on the type-match advance. The PR scope is Tab-accept jitter, so this is not a regression, but the inconsistency is worth noting for a follow-up.Reviews (1): Last reviewed commit: "fix(overlay): hold TextKit-mirror anchor..." | Re-trigger Greptile