Repository navigation
fix(datagrid): give Return back to the filter field until a suggestion is selected - #2927
Merged
datlechin merged 2 commits intoSep 16, 2026
Merged
Conversation
This was referenced Sep 16, 2026
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.
Problem
In the grid's filter bar, typing a condition and pressing
Returndid not apply the filter. The completion list had opened, and it ownedReturn. The user had to pressEscapefirst, thenReturn.The first version of this PR narrowed when the list opens, by testing the character before the caret. Review found that approach wrong in four ways:
::cast type list in the filter bar, undoing fix(editor): open the type list on a PostgreSQL cast with nothing typed #2925, which landed three commits earlier and is listed one line above in the changelog.SQLTokenBoundary.isTokenCharaccepts"and the backtick, soregion="EU"on MySQL still opened the list and still swallowedReturn.SQLCompletionService.isSuppressedEmptyPrefix.Returnstill failed:region='EU' AND nam,users.,active IS NOT NULL.Root cause
Two separate defects, one symptom.
The list owned
Returnfrom the moment it appeared.presentSuggestionsandshowPopoverboth setselectedIndex = 0, so a row was selected before the user touched an arrow key, andsuggestionCommandOutcomemappedinsertNewlineto.accept. On the.sqlTokenspathsubmitsOnAcceptisfalse(6bdef58, #1384, deliberately, so a completion mid-expression does not run the filter), soReturnaccepted a suggestion and never calledonSubmit.The filter field had its own trigger rule. It reaches
CompletionEngine.filterCompletionsdirectly, so it skipped the clause-aware empty-prefix rule the editor uses and had to re-derive one from characters.Fix
Nothing is selected until an arrow key selects it.
SuggestionState.selectedIndexisInt?.Downenters the list at the first row,Upat the last, and inside the list both clamp, matching the query editor's panel.ReturnandTabpass through to the field until the user has picked a row; once one is picked,Returnaccepts it andTabaccepts without submitting, exactly as before.This is AppKit's documented contract, not an invention.
NSTextView.hsays of the completion list's selected index: "default is 0, and -1 indicates no selection", andNSControlTextEditingDelegate, the delegate a text field's completion actually goes through, says "Set the value to -1 to indicate there should not be an initial selection."NSComboBox.indexOfSelectedItemreports -1 for the same state. Measured in a standalone AppKit harness: anNSTextFieldwith an auto-triggered completion list at index 0 never fires its action, not on the firstReturnand not on the second, which is this bug reproduced with no TablePro code in it; at -1 the typed text is left alone.Finder's search field is the shipping shape and was checked directly: its suggestion menu opens with nothing highlighted, one
Returncommits the typed search with no token inserted,Downhighlights the first row, andReturnthen accepts it.It deliberately differs from the query editor's popup, which preselects. There
Return's only competing meaning is inserting a newline thatCmd+Zundoes; in a one-line filter fieldReturnis the field's only action. The app already draws that line itself in the AI chat composer, where the mention popup takesReturnonly after an explicit@and only while it has candidates.On the
.sqlTokenspathReturnwith a selection accepts without submitting, which is AppKit's behaviour rather than Finder's. That is deliberate: a half-writtenregion='EU' AND nameshould not run the moment its column is completed.One trigger rule, shared.
isSuppressedEmptyPrefixmoves out ofSQLCompletionServiceintoSQLCompletionTriggerPolicy, andRawSQLFilterCompletionProviderapplies it too.shouldOfferTokenCompletionis deleted. Measured on the analyzer, the filter path now behaves like the editor:region='EU',region IN ('EU')andid = 1 ANDstay shut;id::opens the type list, keeping #2925;users.opens on its dot;region='EU' AND naopens on its prefix.The two halves are orthogonal, and both are needed. The policy alone leaves
Returnbroken wherever the prefix is non-empty. The selection model alone leaves the list opening where the editor's would not.Blast radius
Four construction sites, three of them affected:
FilterRowViewraw SQL field (the bug site) and its plain value field. The value field's completions are column names plus SQL keywords, soReturnthere used to silently swap a typed value for a column name; it now applies what was typed, andDownthenReturnstill completes.CompareTableScopeEditor, source and target filters.Tabwith nothing selected now moves focus instead of accepting, which commits the draft through the existingfocusedFieldhandler.HighlightRuleRowpasses.staticValues([]), so its popup can never open and nothing changes.Validation
verify.sh build: PASSverify.sh test FilterValueTextFieldTests RawSQLFilterCompletionTriggerTests SQLCompletionServiceEmptyPrefixTests CompletionEngineFilterTests StringCatalogIntegrityTests FilterFocusStateTests: PASSverify.sh uitest FilterBarReturnUITests: written and compiling, not executed. Every UI suite on this machine fails to start withTimed out while enabling automation mode; an untouched control suite (AuxiliaryWindowCloseUITests) fails identically, so it is the environment and not this change. It needs a run on CI.verify.sh lintover every changed file: 0 violationsverify.sh docs: PASSThe previous version's integration test passed for the wrong reason: it built a provider over an empty
SQLSchemaProvider(), so no items were ever produced and no popup ever opened, and it slept 200ms against a 50ms debounce plus a schema round trip. It is replaced by three tests that drive the coordinator's real key handling with a synchronous.staticValuessource, plusRawSQLFilterCompletionTriggerTests, which walks the trigger rule over the real analyzer.FilterBarReturnUITestsis the first UI coverage any filter field has had. It asserts on Clear, which is enabled only once a filter is applied, rather than on the popover, whose rows the runner does not resolve reliably.Accessibility: the announcement is now "%lld suggestions available, press Down Arrow to browse", translated in all five catalog languages, because the list no longer offers an action until the user arrows into it.