Handle completion separators and popup dismissal - #31191
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8c00448f6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let separator_precedes_plain_text = self.draft.textarea.text()[after_separator..] | ||
| .chars() | ||
| .next() | ||
| .is_some_and(|c| !c.is_whitespace() && !matches!(c, '@' | '$')); |
There was a problem hiding this comment.
When a completion is followed by a single existing separator and an @ or $ token (for example completing @ma in @ma @next), this condition treats the suffix as not plain text and moves the cursor after the only space. The next characters the user types are then inserted immediately before the sigil, producing src/main.rs foo@next instead of keeping the existing token separated (src/main.rs foo @next). Preserve an extra separator for any non-whitespace suffix that should remain a distinct token.
Useful? React with 👍 / 👎.
a8c0044 to
b629fce
Compare
## Summary Autocomplete popup synchronization already identifies the active token's range and query, but accepting a result previously discarded that range and independently recomputed token boundaries around the cursor. Those calculations could disagree at ambiguous cursor positions. For example, in `@first| @second` (with `|` marking the cursor at the intervening space), the popup targets `@second`, while the old acceptance path replaces `@first`. This threads the active range through legacy file search, skill mentions, and mentions-v2 completion. File, image, and mention insertion now replace the same token that supplied the popup query, and image completion reuses the shared file insertion path. ## Stack This is PR 1 of 3. #31191 builds separator and dismissal behavior on these explicit replacement ranges, and #30463 then fixes token affinity between adjacent mentions.
f183e69 to
e804544
Compare
e44e267 to
304ad36
Compare
fcoury-oai
left a comment
There was a problem hiding this comment.
Ran a couple of smoke tests and they all worked fine.
The only suggestion I have is to extract the completion-separator logic into a dedicated new module since chat_composer is more than 11k lines at the moment.
I'll leave that up to you and approve the PR.
|
That's a good call, but I may defer that to the next PR to see if there's a more compelling cut-point. |
Summary
Autocomplete completion previously inserted a new space even when a separator was already present, which could leave redundant whitespace around neighboring text. Popup dismissal also tracked only the query string, so dismissing one token could suppress a different occurrence with the same text.
This gives completions one horizontal-separator policy across files, images, skills, and mentions. Existing separators are reused when possible, ordinary suffix text remains separated, and line breaks stay intact. Dismissal now identifies the complete whitespace-delimited token occurrence, so offset-only edits preserve dismissal while later identical tokens remain independent. Newly completed values beginning with
$or@can also remain closed when the next PR's affinity rule recognizes the token to the left of the cursor.Examples
The examples below use
|to represent the cursor.Reuse existing separators
Starting with an existing two-space gap:
After accepting
src/main.rsand typingfoo, completion previously added a third separator and left both original spaces beforenext:After this PR, completion reuses the first existing separator as the insertion point and preserves the second between the new text and
next:Keep a completed sigil-prefixed value closed
Starting before a line break:
and accepting a path whose result is itself prefixed with
@produces:Without this PR's completion dismissal, the affinity rule in #30463 rediscovers the completed token to the left of the cursor and reopens its popup:
With this PR, the inserted occurrence is dismissed and the popup remains closed:
Do not dismiss an identical later occurrence
Given two identical tokens:
after dismissing the first popup with Escape and moving to the second token, query-only dismissal previously suppressed the second popup too:
After this PR, dismissal also matches the token's ordinal among complete tokens, so the second occurrence opens normally:
Keep dismissal across offset-only edits
After dismissing
@ma, moving to its start, and pasting an email-like token:the
@mabytes embedded inemail@ma.comdo not count as another autocomplete token. The original@makeeps its dismissal even though its byte range moved.Stack
This is PR 2 of 3, stacked on #31190. It relies on the explicit replacement ranges introduced there and provides the completion lifecycle used by the targeting fix in #30463.