Skip to content

fix(editor): rank a completion list that arrives late for where the cursor is now - #3135

Draft
datlechin wants to merge 1 commit into
mainfrom
fix/completion-live-prefix
Draft

datlechin wants to merge 1 commit into
mainfrom
fix/completion-live-prefix

Conversation

@datlechin

Copy link
Copy Markdown
Member

Typing a word quickly while the first completion request of a session is still out presents a list ranked for the letters typed before the request was read, so Return commits the wrong keyword: sel then Return gave set. This is what EditorAutocompleteFocusUITests/testCommittedKeywordTakesTheTypedCase has been catching.

Evidence

  • CI sweep over 7 days of macOS Tests runs (2026-09-18 to 09-25): testCommittedKeywordTakesTheTypedCase failed its first run and passed on retry in 7 of 127 executions, twice on main (35812418402 on 3f827b0, 36121207600 on c4833e6) and on five unrelated PRs (35494222457, 35654778508, 35709500014, 35836071911, 36135765290). Every failure is the first iteration right after launch: "Typing 'sel' and accepting should commit 'select'; got 'set'". testPopupReturnsAfterARequestWithNoMatches failed the same way once (35532237312, "got 'set'").
  • The xcresult of 36135765290 shard 0 shows First Run failed and Retry 1 passed for that case.
  • The new unit test below reproduces the ordering headlessly and, on the old code, presents ["set", "select"] with set selected.

Cause

SuggestionViewModel withholds cursor moves while a request is in flight and never replays them.

  1. s opens a session: showCompletions asks QueryCompletionAdapter.completionSuggestionsRequested, which waits its 50 ms debounce, reads the cursor and text, and awaits service.completions. The first request after launch is the slow one.
  2. e and l arrive through cursorsUpdated. Nothing is presented yet, so the re-rank is skipped (isPresented is false, since fix(editor): end a completion session that presented nothing so the popup can open again #2921), and guard itemsRequestTask == nil else { return } drops the move.
  3. The request comes back ranked for the prefix it read (s or se), and showCompletions assigns it to items with row 0 selected, without looking at where the cursor is now.
  4. Return applies row 0. completionWindowApplyCompletion takes its replacement range from the live cursor, so sel is replaced with set.

The adapter's own comment on completionOnCursorMove says a list ordered for an earlier prefix commits the wrong item on Return; this path presented one without going through it. It shipped: the unreconciled presentation is in v0.75.0 and every release before it (git tag --contains on #1611 and #2921).

Change

  • SuggestionViewModel.showCompletions reconciles the answer with the live cursor before presenting it. If the cursor is still where the delegate answered (windowPosition), the list is shown as returned. If it moved, the list comes from completionOnCursorMove for the live cursor, the same call an open popup makes on every keystroke, and when that has nothing the session ends instead of showing a stale list. Comparing against windowPosition rather than the trigger position matters: the adapter reads the cursor after its debounce, so an empty-prefix list after . stays as it is.
  • QueryCompletionAdapter sessions remember the token they were opened for, and completionOnCursorMove declines a cursor in another token. Before, it re-ranked the old context's candidates by whatever word the cursor was in, so a request for s that resolved after se u would have offered keywords for u. With the popup open this was unreachable by typing, because a space or . empties the token and closes it on the way; the in-flight path skips those intermediate moves, so it needs the check. It also closes the popup on Option-Right into the next word and on Backspace over the . that opened a column list, where it used to re-rank in the wrong context.
  • No change to the popup's placement: it opens where the delegate answered, as it does when the list arrives in time and the user keeps typing.

Tests

  • SuggestionLiveCursorTests (TableProEditorKit, headless, swift test): a delegate that parks on a continuation, so the ordering is deterministic. The window reports itself key, the pattern SuggestionSessionLifetimeTests already runs on CI.
    • test_showCompletions_ranksForTheKeysTypedWhileTheRequestWasOut: the request is opened for s, e and l go through cursorsUpdated while it is out, then it answers ["set", "select"]. Expects ["select"] presented and selected. On the old code it presents ["set", "select"] with set selected (4 assertions fail). Reverting the reconciliation in showCompletions turns it red.
    • test_showCompletions_endsTheSessionWhenNothingMatchesWhereTheCursorMoved: nothing ranks for the live cursor. Expects no presentation and the session ended. On the old code the stale list is presented (5 assertions fail).
    • test_showCompletions_presentsTheAnswerAsIsWhenTheCursorStayedPut: the cursor did not move, so the answer is shown untouched and completionOnCursorMove is never asked. Passes on old and new code; it turns red if the reconciliation is made unconditional, which would end every empty-prefix session after ..
  • QueryCompletionAdapterLifecycleTests/sessionDeclinesACursorOnAnotherToken (TableProTests): a session opened on se re-ranks sel to ["select"] and returns nil for se u. Removing the tokenStart guard in completionOnCursorMove turns it red (run locally: 17 executed, 1 failed).
  • The UI test is unchanged; it is the end-to-end proof on CI.

Verification

  • swift test --build-system native --package-path Packages/TableProEditor: 174 XCTest cases, 0 failures, 2 skipped; the 504 Swift Testing tests pass except GutterNumberOffsetTests "A window's gutter keeps its margin and its room to grow", which fails the same way on this machine with the change stashed and passes on CI (Package Tests for fix(datagrid): report a query tab's first run in the status bar, and put the six quarantined UI cases back on the gate #3130 on 2026-09-25), so it is local rendering, not this change.
  • verify.sh test QueryCompletionAdapterLifecycleTests: 17 executed, 17 passed.
  • swiftlint lint --strict on the four Swift files: 0 violations.
  • UI tests were not run locally (the automation prompt blocks XCUITest on this machine); CI is the UI proof.
  • CI: pending.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant