Custom expression editor: correctly accept the suggestion (#15872)

This is carried out via lexical replacement, i.e. finding the token to
be replaced, instead of the rather-brittle and often-incorrect regex for
prefix/postfix.

* Always place the caret after the replacement text

* Only accept suggested "(" when there is no forthcoming "(" already

frontend/src/metabase/lib/expressions/suggest.js

@@ -90,16 +90,6 @@ export function suggest({
     partialSuggestionMode = true;
   }
 
-  const identifierTrimOptions = lastInputTokenIsUnclosedIdentifierString
-    ? {
-        // use the last token's pattern anchored to the end of the text
-        prefixTrim: new RegExp(lastInputToken.tokenType.PATTERN.source + "$"),
-      }
-    : {
-        prefixTrim: new RegExp(Identifier.PATTERN.source + "$"),
-        postfixTrim: new RegExp("^" + Identifier.PATTERN.source + "\\s*"),
-      };
-
   const context = getContext({
     cst,
     tokenVector,
@@ -174,7 +164,6 @@ export function suggest({
             alternates: EDITOR_FK_SYMBOLS.symbols.map(symbol =>
               getDimensionName(dimension, symbol),
             ),
-            ...identifierTrimOptions,
           })),
         );
       }
@@ -184,7 +173,6 @@ export function suggest({
             type: "segments",
             name: segment.name,
             text: formatSegmentName(segment),
-            ...identifierTrimOptions,
           })),
         );
       }
@@ -194,7 +182,6 @@ export function suggest({
             type: "metrics",
             name: metric.name,
             text: formatMetricName(metric),
-            ...identifierTrimOptions,
           })),
         );
       }
@@ -248,9 +235,6 @@ export function suggest({
           type: "functions",
           name: "case",
           text: "case(",
-          postfixText: ")",
-          prefixTrim: /\w+$/,
-          postfixTrim: /^\w+(\(\)?|$)/,
         };
         finalSuggestions.push(caseSuggestion);
       }
@@ -317,9 +301,6 @@ function functionSuggestion(type, clause, parens = true) {
     type: type,
     name: name,
     text: name + (parens ? "(" : " "),
-    postfixText: parens ? ")" : " ",
-    prefixTrim: /\w+$/,
-    postfixTrim: parens ? /^\w+(\(\)?|$)/ : /^\w+\s*/,
   };
 }
 

frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield.jsx

@@ -172,22 +172,31 @@ export default class ExpressionEditorTextfield extends React.Component {
     const suggestion = suggestions && suggestions[index];
 
     if (suggestion) {
-      let prefix = source.slice(0, suggestion.index);
-      if (suggestion.prefixTrim) {
-        prefix = prefix.replace(suggestion.prefixTrim, "");
-      }
-      let postfix = source.slice(suggestion.index);
-      if (suggestion.postfixTrim) {
-        postfix = postfix.replace(suggestion.postfixTrim, "");
-      }
-      if (!postfix && suggestion.postfixText) {
-        postfix = suggestion.postfixText;
+      const { tokens } = tokenize(source);
+      const token = tokens.find(t => t.end >= suggestion.index);
+      if (token) {
+        const prefix = source.slice(0, token.start);
+        const postfix = source.slice(token.end);
+        const suggested = suggestion.text;
+
+        // e.g. source is "isnull(A" and suggested is "isempty("
+        // the result should be "isempty(A" and NOT "isempty((A"
+        const openParen = _.last(suggested) === "(";
+        const alreadyOpenParen = _.first(postfix.trimLeft()) === "(";
+        const extraTrim = openParen && alreadyOpenParen ? 1 : 0;
+        const replacement = suggested.slice(0, suggested.length - extraTrim);
+
+        const updatedExpression = prefix + replacement.trim() + postfix;
+        this.onExpressionChange(updatedExpression);
+        const caretPos = updatedExpression.length - postfix.length;
+        setTimeout(() => {
+          this._setCaretPosition(caretPos, true);
+        });
+      } else {
+        const newExpression = source + suggestion.text;
+        this.onExpressionChange(newExpression);
+        setTimeout(() => this._setCaretPosition(newExpression.length, true));
       }
-
-      this.onExpressionChange(prefix + suggestion.text + postfix);
-      setTimeout(() =>
-        this._setCaretPosition((prefix + suggestion.text).length, true),
-      );
     }
   };
 

frontend/test/metabase/scenarios/question/notebook.cy.spec.js

@@ -907,6 +907,48 @@ describe("scenarios > question > notebook", () => {
       cy.get("[contenteditable='true']").type("[Price] ");
       cy.contains("/").should("not.exist");
     });
+
+    it("should correctly accept the chosen field suggestion", () => {
+      openProductsTable({ mode: "notebook" });
+      cy.findByText("Custom column").click();
+      cy.get("[contenteditable='true']").type(
+        "[Rating]{leftarrow}{leftarrow}{leftarrow}",
+      );
+
+      // accept the only suggested item, i.e. "[Rating]"
+      cy.get("[contenteditable='true']").type("{enter}");
+
+      // if the replacement is correct -> "[Rating]"
+      // if the replacement is wrong -> "[Rating] ng"
+      cy.get("[contenteditable='true']")
+        .contains("[Rating] ng")
+        .should("not.exist");
+    });
+
+    it("should correctly accept the chosen function suggestion", () => {
+      openProductsTable({ mode: "notebook" });
+      cy.findByText("Custom column").click();
+      cy.get("[contenteditable='true']").type("LTRIM([Title])");
+
+      // Place the cursor between "is" and "empty"
+      cy.get("[contenteditable='true']").type(
+        Array(13)
+          .fill("{leftarrow}")
+          .join(""),
+      );
+
+      // accept the first suggested function, i.e. "length"
+      cy.get("[contenteditable='true']").type("{enter}");
+
+      cy.get("[contenteditable='true']").contains("length([Title])");
+    });
+  });
+
+  it("should correctly insert function suggestion with the opening parenthesis", () => {
+    openProductsTable({ mode: "notebook" });
+    cy.findByText("Custom column").click();
+    cy.get("[contenteditable='true']").type("LOW{enter}");
+    cy.get("[contenteditable='true']").contains("lower(");
   });
 });