Keep rapid Tab accepts owned while the overlay holds its next present - #838
Conversation
The ghost-text rewrite in #827 lets OverlayController.showSuggestion return without publishing state: it holds a presentation while a pixel caret read is in flight (PixelCaretLocator, for union-run paragraphs and estimated single-line carets) or while the host caret lags its published text (CaretLagPolicy). During that hold `overlayState` still names the previous presentation, and advanceInline refuses to slide while the panel is held, so after a Tab accept the published overlay text is the pre-accept tail. A rapid follow-up Tab then failed SuggestionSessionReconciler. overlayAllowsAcceptance (visible text must equal the session tail), passTabThrough cleared the session, and the accept tap returned the original Tab to the host: the ghost vanished and browser focus jumped to the page's next control. A slower Tab landed after the capture callback re-ran the present and succeeded, which is why only fast Tabbing broke. The controller now exposes the text it is holding (`heldPresentationText`), and acceptance treats that in-flight present as offered: the visible ghost must equal the tail, or the tail must be exactly what Cotabby is about to paint. A mismatched ghost with nothing held is still stale UI and passes Tab through as before.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (9)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughOverlayController now exposes text for a suggestion presentation that is pending. Acceptance preparation passes that text to session validation, which checks it alongside visible overlay text. Tests cover deferred presentations, rapid acceptance, and stale visible overlays. ChangesPending Presentation Acceptance
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue was established. Rapid Tab acceptance still warrants the planned live Chrome and Obsidian checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new rule is limited to pending text that matches the active suggestion. Existing permission, focused-field, and session checks still precede insertion. Targeted tests cover rapid acceptance, but the behavior has not been verified in a live host. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /// or a lagging host caret: it records the text as held and leaves `state` on the previous | ||
| /// presentation. `landHeldPresentation()` then applies it, as the capture callback would. | ||
| var defersPresentations = false | ||
| private(set) var heldPresentationText: String? | ||
| private var heldGeometry: SuggestionOverlayGeometry? | ||
|
|
There was a problem hiding this comment.
heldPresentationText, even if Tab still reaches the host. Please add focused tests for the production hold and clearing behavior.
Knowledge Base Used: Suggestion overlay presentation
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!
…anchor Brings in FuJacob/cotabby through 7724926 (FuJacob#825, FuJacob#827, FuJacob#834, FuJacob#835, FuJacob#836, FuJacob#837, FuJacob#838): ghost text matched to the host's own rendering plus finishing the word being typed, Obsidian/Electron accessibility priming, rapid Tab accepts kept owned while the overlay presents, Apple Intelligence greyed out when unavailable, a new llama-recall eval fixture, the installer artwork and kickstart script, and a CI test-flakiness fix. Resolution, where both sides changed the same thing: - Suppression reasons: upstream's `CompletionContentPolicy` rejects any completion without a letter or digit (`noWordContent`), which subsumes this fork's `periodOnly`, so that case and its helper are removed and the bare-dot test now asserts the broader reason. Behaviour change worth knowing: a lone closing bracket or question mark is now withheld too, where this fork deliberately let those through; its test records that. - Prompt renderer: upstream's `personaLine(_:prefix:)` (the name only appears at a sign-off) wins; this fork's learned-phrases section is unchanged beside it. - Eval case schema: upstream added `visualContextSummary` and `clipboardContext` for its recall fixture; this fork's `screenText` and `learnedPhrases` stay alongside them. The harness prefers the pre-rendered summary and still puts raw `screenText` through the live local passes. Both scorer helpers kept. - Kept from this fork: the separate test host, the 2-4 word eval preset, phrase memory, the clock-time rule, the debug-artifact gate. Full suite: 2,724 tests, 16 skipped, 0 failures; the installed app's defaults byte-identical afterwards and no debug artifacts written. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Summary
Regression from #827:
OverlayController.showSuggestioncan now hold a presentation (caret-lag hold, or while the pixel caret locator reads the host). During that holdoverlayStatestill shows the previous ghost, so a rapid second Tab failedSuggestionSessionReconciler.overlayAllowsAcceptance(visible text ≠ remaining tail), the coordinator calledpassTabThrough, the session was cleared, and Tab reached the host — moving focus to page buttons in browsers. The overlay now exposesheldPresentationText, and the acceptance rule also authorizes the tail when it is exactly the text the overlay is about to paint. A stale visible ghost with nothing held still passes Tab through as before.Validation
CODE_SIGNING_ALLOWED=NO,build/DerivedData): 2642 tests, 0 failures, 16 skipped.test_rapidTabsAcceptEachWordWhileTheOverlayIsStillHoldingThePreviousPresentreproduces the leak with the rule disabled (second Tab not consumed, only " hello" inserted) and passes with the fix; plus held-present / stale-ghost coordinator tests, an interaction-state test, and a pure-rule test.-cotabby-debug, mash Tab through a multi-word suggestion in a Chrome composer and in Obsidian, and confirm notab-passed-throughfollowspixel-caret-hold/caret-lag-hold.Linked issues
Refs #827
Risk / rollout notes
SuggestionOverlayControllinggainsheldPresentationTextwith a nil default, so other conformers are unaffected.hideOverlayend the post-exhaustion window; if live logs showreconcile-mismatch/ "focused field changed" before a pass-through, that needs a follow-up.Summary by CodeRabbit
The PR appears safe to merge, with non-blocking production-overlay test coverage still worth adding.
Summary
The PR lets acceptance recognize a suggestion tail whose next overlay presentation is still held, preventing a rapid follow-up Tab from passing through when the published overlay shows the previous tail.
Reviews (1) · Last reviewed commit: "Keep rapid Tab accepts owned while the o..."