Anchor user-forced popup card to the caret line, not the field rect#368
Merged
Conversation
When the user pinned popup mode (or a per-app override forced it), the card was still anchored to the input field's bottom edge — the strategy that exists specifically to dodge unreliable caret geometry. With exact or derived caret quality that anchor wastes the precise signal and drops the popup far below where the eye is. Branch the anchor choice on the mirror reason: keep the field-rect anchor for caretGeometryEstimated (the caret rect really is unreliable there), and switch to a caret-rect anchor for userPreference and perAppOverride so the popup tracks the cursor like inline ghost text.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+161
to
+171
| case .userPreference, .perAppOverride: | ||
| // Caret geometry is trustworthy in these cases. Sit just under the caret line so the | ||
| // popup tracks the cursor like the inline ghost does, instead of floating below the | ||
| // entire field. | ||
| if !geometry.caretRect.isEmpty { | ||
| return geometry.caretRect.minY - Metrics.anchorGap | ||
| } | ||
| if let inputFrame = geometry.inputFrameRect?.standardized, !inputFrame.isEmpty { | ||
| return inputFrame.minY - Metrics.anchorGap | ||
| } | ||
| return geometry.caretRect.minY - Metrics.caretFallbackVerticalOffset |
Contributor
There was a problem hiding this comment.
caretRect is empty and inputFrameRect is nil/empty, this final fallback applies caretFallbackVerticalOffset to an already-empty caret rect whose minY is 0, producing -22. The screen-edge clamping rescues the card, but the intent — a "fallback vertical offset" — is meaningless here since there is no real caret position to offset from. Using Metrics.anchorGap instead makes the intent explicit and avoids relying on the clamp to fix a large negative offset.
Suggested change
| case .userPreference, .perAppOverride: | |
| // Caret geometry is trustworthy in these cases. Sit just under the caret line so the | |
| // popup tracks the cursor like the inline ghost does, instead of floating below the | |
| // entire field. | |
| if !geometry.caretRect.isEmpty { | |
| return geometry.caretRect.minY - Metrics.anchorGap | |
| } | |
| if let inputFrame = geometry.inputFrameRect?.standardized, !inputFrame.isEmpty { | |
| return inputFrame.minY - Metrics.anchorGap | |
| } | |
| return geometry.caretRect.minY - Metrics.caretFallbackVerticalOffset | |
| case .userPreference, .perAppOverride: | |
| // Caret geometry is trustworthy in these cases. Sit just under the caret line so the | |
| // popup tracks the cursor like the inline ghost does, instead of floating below the | |
| // entire field. | |
| if !geometry.caretRect.isEmpty { | |
| return geometry.caretRect.minY - Metrics.anchorGap | |
| } | |
| if let inputFrame = geometry.inputFrameRect?.standardized, !inputFrame.isEmpty { | |
| return inputFrame.minY - Metrics.anchorGap | |
| } | |
| // Both rects are degenerate; the card will be clamped to the screen edge by the caller. | |
| return Metrics.anchorGap |
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
1 participant
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
Follow-up to #351. When the user pinned popup mode (or a per-app override forced it), the card was still anchored to the input field's bottom edge — the strategy that exists specifically to dodge unreliable caret geometry. With
.exactor.derivedcaret quality that anchor wastes the precise signal and drops the popup far below where the eye is. This PR branches the anchor choice on the mirror reason: keep the field-rect anchor forcaretGeometryEstimated, switch to a caret-rect anchor foruserPreferenceandperAppOverrideso the popup tracks the cursor like inline ghost text does.Validation
Three new tests cover the fix:
test_make_userPreferenceAnchorsToCaretLine_notInputField— the bug case: card lands near the caret, not at the field's bottom edge.test_make_perAppOverrideAnchorsToCaretLine— same behavior for per-app override.test_make_estimatedReasonStillAnchorsToInputField— regression guard that the original.estimatedpath is unchanged.Risk / rollout notes
.caretGeometryEstimatedreason (the auto-mirror path users see by default). The fix only touches placement when popup mode is user-pinned or per-app forced, both of which require an explicit user action to reach today.Linked issues
Refs #351.
Greptile Summary
This PR fixes popup-card placement for user-pinned and per-app-forced mirror mode. Previously all mirror reasons anchored to the input field's bottom edge; now
.userPreferenceand.perAppOverrideanchor to the caret rect (with the field rect as a fallback), while.caretGeometryEstimatedkeeps the original field-first ordering.MirrorOverlayLayout.computeAnchorTopYis refactored from a flat function into aswitchoverCompletionRenderMode.MirrorReason, routing each case to the appropriate anchor strategy.MirrorOverlayLayoutTestsverify the caret-anchored path, the per-app path, and a regression guard for the estimated path.Confidence Score: 4/5
Safe to merge; the fix is narrowly scoped, only activates through an explicit user action, and all three expected behaviours are covered by the new tests.
The struct-level doc comment now directly contradicts the new caret-anchored behaviour for
.userPreference/.perAppOverride, which will mislead future readers. The degenerate triple-fallback for those same branches produces a negative Y that relies entirely on the screen clamp to stay on-screen — a small logic gap compared to the.caretGeometryEstimatedpath. Both are quality concerns rather than functional breakage for end users today.The struct-level docstring in
MirrorOverlayLayout.swiftneeds to be updated to reflect the new anchor strategy for.userPreferenceand.perAppOverride.Important Files Changed
.userPreference/.perAppOverride..userPreference/.perAppOverridepaths is untested.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[computeAnchorTopY] --> B{reason} B -- .caretGeometryEstimated --> C{inputFrameRect\navailable & non-empty?} C -- yes --> D[inputFrame.minY − anchorGap] C -- no --> E[caretRect.minY − caretFallbackVerticalOffset] B -- .userPreference\n.perAppOverride --> F{caretRect\nnon-empty?} F -- yes --> G[caretRect.minY − anchorGap] F -- no --> H{inputFrameRect\navailable & non-empty?} H -- yes --> I[inputFrame.minY − anchorGap] H -- no --> J[caretRect.minY − caretFallbackVerticalOffset\n⚠️ caretRect is empty here — minY = 0\nclamped to screen minY]Comments Outside Diff (2)
Cotabby/Support/MirrorOverlayLayout.swift, line 7-10 (link).userPreferenceand.perAppOverridebranches added here do exactly that. A reader skimming the type-level docs will get the wrong mental model for those paths.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!
CotabbyTests/MirrorOverlayLayoutTests.swift, line 145-157 (link)test_make_fallsBackToCaretRectWhenInputFrameMissingonly exercises.caretGeometryEstimated. There is no test for what happens when bothcaretRectis empty andinputFrameRectis nil under.userPreferenceor.perAppOverride. The triple-fallback path incomputeAnchorTopYnow has different code for those reasons but no corresponding test to guard it against future regression.Reviews (1): Last reviewed commit: "Anchor user-forced popup card to the car..." | Re-trigger Greptile