fix(editor): suggestion window monitor cleanup on accept#1290
Merged
Conversation
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
Fixes #1278 — Enter and ⌘+Enter stop working after picking a table from the autocomplete dropdown.
The suggestion completion window registers a local
NSEvent.keyDownmonitor to intercept Return/Tab as "accept suggestion". When the user accepted,SuggestionViewModel.applySelectedItemcalledwindow?.close()— that isNSWindow.close(), which does not route throughSuggestionController.close()override. The override is whereremoveEventMonitors()andmodel.willClose()run. So after acceptance the monitor stayed installed and every subsequent Enter was re-interpreted as "apply the same suggestion again" (logs show 5 silent applies for 5 Enter presses). ⌘+Enter was swallowed for the same reason.Bonus: the double-tap handler in
SuggestionContentViewwas passingmodel.activeTextView?.view.window(the editor window) intoapplySelectedItem. That meant double-clicking a suggestion closed the editor window instead of the suggestion window. Confirmed in the same logs (windowDidResignKey→windowWillCloseright afterdblclick.apply).Fix
Two layers, Apple-correct:
SuggestionViewModel.applySelectedItemno longer takes awindow:parameter. It exposes anonApplyclosure that the controller wires ininitto call its ownclose(). Both call sites (keyboard handler and double-tap) now just callapplySelectedItem(item:), eliminating Bug A entirely.NSWindow.willCloseNotification. The cleanup (removeEventMonitors,model.willClose, KVO invalidate, resign-observer teardown) moves intohandleWindowWillClose(_:). Theclose()override only handles the popover branch +super.close(). Result: cleanup is idempotent and runs no matter who closes the window —controller.close(),window.close(), system close, or future call sites.Tests
Four new tests in
LocalPackages/CodeEditSourceEditor/Tests/.../SuggestionApplyTests.swift:test_apply_invokesOnApplyCallback— model triggers the wired close callbacktest_apply_skipsCallbackWhenActiveTextViewIsNil— guard workstest_nsWindowClose_clearsModelState— directNSWindow.close()path triggers cleanup via notificationtest_controllerClose_clearsModelState— controller path also triggers cleanupTest plan
xcodebuild test -only-testing:CodeEditSourceEditorTests/SuggestionApplyTestsgreenSELECT * FROM→ pick a table with Enter → press Enter again → cursor goes to new line