Skip to content

fix(datagrid): give Return back to the filter field until a suggestion is selected - #2927

Merged
datlechin merged 2 commits into
TableProApp:mainfrom
filipac:fix/filter-autocomplete-trigger
Sep 16, 2026
Merged

datlechin merged 2 commits into
TableProApp:mainfrom
filipac:fix/filter-autocomplete-trigger

Conversation

@filipac

@filipac filipac commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Problem

In the grid's filter bar, typing a condition and pressing Return did not apply the filter. The completion list had opened, and it owned Return. The user had to press Escape first, then Return.

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:

  • It also closed the PostgreSQL :: 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.isTokenChar accepts " and the backtick, so region="EU" on MySQL still opened the list and still swallowed Return.
  • It was a second, clause-blind copy of SQLCompletionService.isSuppressedEmptyPrefix.
  • It did nothing for the positions where the list is correctly open and Return still failed: region='EU' AND nam, users., active IS NOT NULL.

Root cause

Two separate defects, one symptom.

The list owned Return from the moment it appeared. presentSuggestions and showPopover both set selectedIndex = 0, so a row was selected before the user touched an arrow key, and suggestionCommandOutcome mapped insertNewline to .accept. On the .sqlTokens path submitsOnAccept is false (6bdef58, #1384, deliberately, so a completion mid-expression does not run the filter), so Return accepted a suggestion and never called onSubmit.

The filter field had its own trigger rule. It reaches CompletionEngine.filterCompletions directly, 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.selectedIndex is Int?. Down enters the list at the first row, Up at the last, and inside the list both clamp, matching the query editor's panel. Return and Tab pass through to the field until the user has picked a row; once one is picked, Return accepts it and Tab accepts without submitting, exactly as before.

This is AppKit's documented contract, not an invention. NSTextView.h says of the completion list's selected index: "default is 0, and -1 indicates no selection", and NSControlTextEditingDelegate, 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.indexOfSelectedItem reports -1 for the same state. Measured in a standalone AppKit harness: an NSTextField with an auto-triggered completion list at index 0 never fires its action, not on the first Return and 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 Return commits the typed search with no token inserted, Down highlights the first row, and Return then accepts it.

It deliberately differs from the query editor's popup, which preselects. There Return's only competing meaning is inserting a newline that Cmd+Z undoes; in a one-line filter field Return is the field's only action. The app already draws that line itself in the AI chat composer, where the mention popup takes Return only after an explicit @ and only while it has candidates.

On the .sqlTokens path Return with a selection accepts without submitting, which is AppKit's behaviour rather than Finder's. That is deliberate: a half-written region='EU' AND name should not run the moment its column is completed.

One trigger rule, shared. isSuppressedEmptyPrefix moves out of SQLCompletionService into SQLCompletionTriggerPolicy, and RawSQLFilterCompletionProvider applies it too. shouldOfferTokenCompletion is deleted. Measured on the analyzer, the filter path now behaves like the editor: region='EU', region IN ('EU') and id = 1 AND stay shut; id:: opens the type list, keeping #2925; users. opens on its dot; region='EU' AND na opens on its prefix.

The two halves are orthogonal, and both are needed. The policy alone leaves Return broken 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:

  • FilterRowView raw SQL field (the bug site) and its plain value field. The value field's completions are column names plus SQL keywords, so Return there used to silently swap a typed value for a column name; it now applies what was typed, and Down then Return still completes.
  • CompareTableScopeEditor, source and target filters. Tab with nothing selected now moves focus instead of accepting, which commits the draft through the existing focusedField handler.
  • HighlightRuleRow passes .staticValues([]), so its popup can never open and nothing changes.

Validation

  • verify.sh build: PASS
  • verify.sh test FilterValueTextFieldTests RawSQLFilterCompletionTriggerTests SQLCompletionServiceEmptyPrefixTests CompletionEngineFilterTests StringCatalogIntegrityTests FilterFocusStateTests: PASS
  • verify.sh uitest FilterBarReturnUITests: written and compiling, not executed. Every UI suite on this machine fails to start with Timed 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 lint over every changed file: 0 violations
  • verify.sh docs: PASS

The 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 .staticValues source, plus RawSQLFilterCompletionTriggerTests, which walks the trigger rule over the real analyzer.

FilterBarReturnUITests is 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.

@datlechin datlechin changed the title fix: prevent premature autocomplete in raw SQL filters fix(datagrid): give Return back to the filter field until a suggestion is selected Sep 16, 2026
@datlechin
datlechin merged commit 6204d39 into TableProApp:main Sep 16, 2026
4 checks passed
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.

2 participants