Add mirror overlay fallback for unreliable caret geometry#347
Merged
Conversation
When AXTextGeometryResolver lands on .estimated quality the host did not expose any of the trusted caret-position paths, so inline ghost text drifts as the user types. Route those cases through a new mirror render mode that draws the suggestion in a Cotabby-owned card anchored to the input field rect rather than the unreliable caret rect. A pure CompletionRenderModePolicy picks the mode from geometry + an optional user/per-app preference, defaulting to .auto so existing hosts that report .exact or .derived stay on the byte-for-byte inline path. Settings UI for the preference and per-app overrides will follow in a later phase; this PR is the architectural seam plus the auto trigger.
Comment on lines
+148
to
+150
| if geometry.caretRect.width > 0 || geometry.caretRect.minX > 0 { | ||
| return geometry.caretRect.midX | ||
| } |
Contributor
There was a problem hiding this comment.
The degenerate-caret guard has a gap:
caretRect.minX > 0 excludes a zero-width cursor positioned at exactly x = 0 (left edge of a screen), causing the code to fall back to the field center when the caret is actually present and valid. A caret with a non-null rect is the right threshold here.
Suggested change
| if geometry.caretRect.width > 0 || geometry.caretRect.minX > 0 { | |
| return geometry.caretRect.midX | |
| } | |
| if !geometry.caretRect.isNull && geometry.caretRect != .zero { | |
| return geometry.caretRect.midX | |
| } |
`MirrorOverlayLayout.make` folded the keycap reservation into `max(minCardWidth, measuredTextWidth + keycapReservation)`, so for any suggestion whose text width was below the 120pt floor the minimum absorbed the reservation and the card came out the same width whether the acceptance hint was on or off. That broke `MirrorOverlayLayoutTests.test_make_widerCardWhenAcceptanceHintEnabled` (140.0 vs. 140.0). Compute the text portion against the floor on its own, then add the keycap on top. `textBudget = maxCardWidth - keycapReservation` keeps the final card from exceeding `maxCardWidth`.
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
When
AXTextGeometryResolverfalls back to.estimatedquality, the host didn't expose any of the trusted caret-position paths and inline ghost text drifts as the user types — the "marching ghost" failure mode in Electron canvases and some web editors. This PR routes those presentations through a new mirror render mode that draws the suggestion in a Cotabby-owned card anchored to the input field rect (much more stable than the caret rect in affected hosts). A pureCompletionRenderModePolicypicks the mode from geometry plus an optional user/per-app preference, defaulting to.auto, so hosts that report.exactor.derivedstay on the byte-for-byte original inline path.Validation
16 new pure-value tests added: 8 for the render-mode policy (auto/explicit/per-app overrides, nil bundle id), 8 for the mirror layout (field anchor, caret fallback, screen-edge clamping on all sides, whitespace collapse, RTL passthrough, keycap reservation). All call sites for the updated
OverlayState.visiblesignature were migrated.Local
xcodebuild testruns hit a Team ID code-signing mismatch on this machine, which is the known local-dev case CLAUDE.md calls out; the tests compile and need CI signing or Xcode UI to execute.Risk / rollout notes
OverlayState.visiblegains a newmodeassociated value. ~8 internal/test call sites updated; no public API change beyond theSuggestionOverlayControllingprotocol's state type..auto; only.estimatedcaret quality triggers the new mode. Hosts that today resolve.exactor.derived(Gmail/Outlook text-marker path, most native AppKit fields, etc.) are unchanged.showInline. The panel, focus invariants, input pipeline, and acceptance hotkey are all untouched.xcodegen generatewas rerun;project.pbxprojdiff is the auto-discovery of the 5 new source files.SuggestionSettingsModellands in a follow-up phase.Greptile Summary
This PR introduces a mirror render mode for hosts where
AXTextGeometryResolverreturns.estimatedcaret quality, routing those presentations through aMirrorOverlayViewcard anchored to the input field rect instead of the drifting caret rect. A newCompletionRenderModePolicypure-value struct picks the mode from geometry quality and optional user/per-app preferences, defaulting to.autoso the existing inline path is completely unchanged for hosts that report.exactor.derived.CompletionRenderMode+CompletionRenderModePolicy— new pure value types; policy is.autoin Phase 1 and already accepts Phase 2 per-app override maps.MirrorOverlayLayout— pure layout math anchoring the card below the input field rect; 8 new tests cover clamping, fallback, whitespace collapse, and RTL passthrough.OverlayController— splitshostingViewintoinlineHostingView/mirrorHostingViewwith an identity-checkedpanel.contentViewswap for mode transitions;OverlayState.visiblegains amodeassociated value, migrating ~8 call sites.Confidence Score: 4/5
Safe to merge for Phase 1 behaviour; one defect in the presenter deduplication guard will silently drop mode flips when Phase 2 per-app overrides are wired.
The deduplication guard in
SuggestionOverlayPresenter.presentreturns nil without callingshowSuggestionwhenever text+geometry are unchanged — even when the render mode has changed. Its own inline comment says "we still need to invokeshowSuggestionso the panel re-renders", andsetCurrentBundleIdentifier's docstring promises per-app overrides take effect immediately. In Phase 1 the mode is derived purely from geometry quality, so this gap has no observable impact today. But the defect is already present in the code shipped by this PR and will activate as soon as Phase 2 wires the bundle-identifier updates through the presenter.Cotabby/Services/Suggestion/SuggestionOverlayPresenter.swift — the early-return deduplication guard at the top of
present.Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[present called with text + geometry] --> B{displayText empty?} B -- yes --> C[hide overlay] B -- no --> D{previousState.visible\nwith same text+geometry?} D -- yes --> E[return nil — no AppKit call\nMODE FLIP SILENTLY DROPPED] D -- no --> F[showSuggestion called] F --> G[renderModePolicy.mode\ngeometry + bundleIdentifier] G --> H{effectivePreference} H -- alwaysInline --> I[.inline] H -- alwaysMirror --> J[.mirror reason] H -- auto --> K{caretQuality == .estimated?} K -- yes --> L[.mirror caretGeometryEstimated] K -- no --> M[.inline] I & J & L & M --> N{switch mode} N -- .inline --> O[showInline GhostSuggestionView] N -- .mirror --> P[showMirror MirrorOverlayView] O & P --> Q[state = .visible text geometry mode] Q --> R[Diagnostic switch on previousState]Reviews (2): Last reviewed commit: "Reserve keycap width outside the min/max..." | Re-trigger Greptile