Fix #12140 (#13724)

* Split expression suggestions into separate component
Fix error messages not appearing in ExpressionsWidget

* Enable test

* Fix suggestion popup keyboard behavior

* Don't try to parse empty input (#12140)

* Remove stray console.log

* Revert test enabling

* Prettier

* Address feedback

frontend/src/metabase/query_builder/components/ExpressionPopover.jsx

@@ -56,19 +56,6 @@ export default class ExpressionPopover extends React.Component {
               }
             }}
           />
-          {error &&
-            (Array.isArray(error) ? (
-              error.map(error => (
-                <div
-                  className="text-error mb1"
-                  style={{ whiteSpace: "pre-wrap" }}
-                >
-                  {error.message}
-                </div>
-              ))
-            ) : (
-              <div className="text-error mb1">{error.message}</div>
-            ))}
           {onChangeName && (
             <input
               className="input block full my1"

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

+import React from "react";
+import PropTypes from "prop-types";
+
+import { t } from "ttag";
+import cx from "classnames";
+
+import Popover from "metabase/components/Popover";
+
+import { isObscured } from "metabase/lib/dom";
+
+const SUGGESTION_SECTION_NAMES = {
+  fields: t`Fields`,
+  aggregations: t`Aggregations`,
+  operators: t`Operators`,
+  metrics: t`Metrics`,
+  other: t`Other`,
+};
+
+export default class ExpressionEditorSuggestions extends React.Component {
+  static propTypes = {
+    suggestions: PropTypes.array,
+    onSuggestionMouseDown: PropTypes.func, // signature is f(index)
+    highlightedIndex: PropTypes.number.isRequired,
+  };
+
+  componentDidUpdate(prevProps, prevState) {
+    if (
+      (prevProps && prevProps.highlightedIndex) !== this.props.highlightedIndex
+    ) {
+      if (this._selectedRow && isObscured(this._selectedRow)) {
+        this._selectedRow.scrollIntoView({ block: "nearest" });
+      }
+    }
+  }
+
+  // when a given suggestion is clicked
+  onSuggestionMouseDown(event, index) {
+    event.preventDefault();
+    event.stopPropagation();
+
+    this.props.onSuggestionMouseDown && this.props.onSuggestionMouseDown(index);
+  }
+
+  render() {
+    const { suggestions, highlightedIndex } = this.props;
+
+    if (!suggestions.length) {
+      return null;
+    }
+
+    return (
+      <Popover
+        className="not-rounded border-dark"
+        hasArrow={false}
+        tetherOptions={{
+          attachment: "top left",
+          targetAttachment: "bottom left",
+        }}
+        sizeToFit
+      >
+        <ul className="pb1" style={{ minWidth: 150, overflowY: "auto" }}>
+          {suggestions.map((suggestion, i) =>
+            // insert section title. assumes they're sorted by type
+            [
+              (i === 0 || suggestion.type !== suggestions[i - 1].type) && (
+                <li className="mx2 h6 text-uppercase text-bold text-medium py1 pt2">
+                  {SUGGESTION_SECTION_NAMES[suggestion.type] || suggestion.type}
+                </li>
+              ),
+              <li
+                ref={r => {
+                  if (i === highlightedIndex) {
+                    this._selectedRow = r;
+                  }
+                }}
+                style={{ paddingTop: 5, paddingBottom: 5 }}
+                className={cx(
+                  "px2 cursor-pointer text-white-hover bg-brand-hover hover-parent hover--inherit",
+                  {
+                    "text-white bg-brand": i === highlightedIndex,
+                  },
+                )}
+                onMouseDownCapture={e => this.onSuggestionMouseDown(e, i)}
+              >
+                {suggestion.range ? (
+                  <span>
+                    {suggestion.name.slice(0, suggestion.range[0])}
+                    <span
+                      className={cx("text-brand text-bold hover-child", {
+                        "text-white bg-brand": i === highlightedIndex,
+                      })}
+                    >
+                      {suggestion.name.slice(
+                        suggestion.range[0],
+                        suggestion.range[1],
+                      )}
+                    </span>
+                    {suggestion.name.slice(suggestion.range[1])}
+                  </span>
+                ) : (
+                  suggestion.name
+                )}
+              </li>,
+            ],
+          )}
+        </ul>
+      </Popover>
+    );
+  }
+}

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

@@ -13,11 +13,7 @@ import colors from "metabase/lib/colors";
 
 import memoize from "lodash.memoize";
 
-import {
-  setCaretPosition,
-  getSelectionPosition,
-  isObscured,
-} from "metabase/lib/dom";
+import { setCaretPosition, getSelectionPosition } from "metabase/lib/dom";
 
 import {
   KEYCODE_ENTER,
@@ -36,12 +32,63 @@ import TokenizedInput from "./TokenizedInput";
 
 import { isExpression } from "metabase/lib/expressions";
 
-const SUGGESTION_SECTION_NAMES = {
-  fields: t`Fields`,
-  aggregations: t`Aggregations`,
-  operators: t`Operators`,
-  metrics: t`Metrics`,
-  other: t`Other`,
+import ExpressionEditorSuggestions from "./ExpressionEditorSuggestions";
+
+const HelpText = ({ helpText, width }) =>
+  helpText ? (
+    <Popover
+      tetherOptions={{
+        attachment: "top left",
+        targetAttachment: "bottom left",
+      }}
+      style={{ width: width }}
+      isOpen
+    >
+      <p
+        className="p2 m0 text-monospace text-bold"
+        style={{ background: colors["bg-yellow"] }}
+      >
+        {helpText.structure}
+      </p>
+      <div className="p2 border-top">
+        <p className="mt0 text-bold">{helpText.description}</p>
+        <p className="text-code m0 text-body">{helpText.example}</p>
+      </div>
+      <div className="p2 border-top">
+        {helpText.args.map(({ name, description }) => (
+          <div>
+            <h4 className="text-medium">{name}</h4>
+            <p className="mt1 text-bold">{description}</p>
+          </div>
+        ))}
+        <a
+          className="link text-bold block my1"
+          target="_blank"
+          href={MetabaseSettings.docsUrl("users-guide/expressions")}
+        >
+          <Icon name="reference" size={12} className="mr1" />
+          {t`Learn more`}
+        </a>
+      </div>
+    </Popover>
+  ) : null;
+
+const Errors = ({ compileError }) => {
+  if (!compileError) {
+    return null;
+  }
+
+  compileError = Array.isArray(compileError) ? compileError : [compileError];
+
+  return (
+    <div>
+      {compileError.map(error => (
+        <div className="text-error mt1 mb1" style={{ whiteSpace: "pre-wrap" }}>
+          {error.message}
+        </div>
+      ))}
+    </div>
+  );
 };
 
 @ExplicitSize()
@@ -86,18 +133,20 @@ export default class ExpressionEditorTextfield extends React.Component {
       const parserOptions = this._getParserOptions(newProps);
       const source = format(newProps.expression, parserOptions);
 
-      const { expression, compileError, syntaxTree } = this._processSource({
-        source,
-        ...this._getParserOptions(newProps),
-      });
-
+      const { expression, compileError, syntaxTree } =
+        source && source.length
+          ? this._processSource({
+              source,
+              ...this._getParserOptions(newProps),
+            })
+          : { expression: null, compileError: null, syntaxTree: null };
       this.setState({
         source,
         expression,
         compileError,
         syntaxTree,
         suggestions: [],
-        highlightedSuggestion: 0,
+        highlightedSuggestionIndex: 0,
       });
     }
   }
@@ -109,17 +158,9 @@ export default class ExpressionEditorTextfield extends React.Component {
     );
   }
 
-  componentDidUpdate(prevProps, prevState) {
-    if (prevState.highlightedSuggestion !== this.state.highlightedSuggestion) {
-      if (this._selectedRow && isObscured(this._selectedRow)) {
-        this._selectedRow.scrollIntoView({ block: "nearest" });
-      }
-    }
-  }
-
-  onSuggestionAccepted = () => {
-    const { source } = this.state;
-    const suggestion = this.state.suggestions[this.state.highlightedSuggestion];
+  onSuggestionSelected = index => {
+    const { source, suggestions } = this.state;
+    const suggestion = suggestions && suggestions[index];
 
     if (suggestion) {
       let prefix = source.slice(0, suggestion.index);
@@ -139,21 +180,10 @@ export default class ExpressionEditorTextfield extends React.Component {
         this._setCaretPosition((prefix + suggestion.text).length, true),
       );
     }
-
-    this.setState({
-      highlightedSuggestion: 0,
-    });
-  };
-
-  onSuggestionMouseDown = (event, index) => {
-    // when a suggestion is clicked, we'll highlight the clicked suggestion and then hand off to the same code that deals with ENTER / TAB keydowns
-    event.preventDefault();
-    event.stopPropagation();
-    this.setState({ highlightedSuggestion: index }, this.onSuggestionAccepted);
   };
 
   onInputKeyDown = e => {
-    const { suggestions, highlightedSuggestion } = this.state;
+    const { suggestions, highlightedSuggestionIndex } = this.state;
 
     if (e.keyCode === KEYCODE_LEFT || e.keyCode === KEYCODE_RIGHT) {
       setTimeout(() => this._triggerAutosuggest());
@@ -177,18 +207,20 @@ export default class ExpressionEditorTextfield extends React.Component {
       return;
     }
     if (e.keyCode === KEYCODE_ENTER) {
-      this.onSuggestionAccepted();
+      this.onSuggestionSelected(highlightedSuggestionIndex);
       e.preventDefault();
     } else if (e.keyCode === KEYCODE_UP) {
       this.setState({
-        highlightedSuggestion:
-          (highlightedSuggestion + suggestions.length - 1) % suggestions.length,
+        highlightedSuggestionIndex:
+          (highlightedSuggestionIndex + suggestions.length - 1) %
+          suggestions.length,
       });
       e.preventDefault();
     } else if (e.keyCode === KEYCODE_DOWN) {
       this.setState({
-        highlightedSuggestion:
-          (highlightedSuggestion + suggestions.length + 1) % suggestions.length,
+        highlightedSuggestionIndex:
+          (highlightedSuggestionIndex + suggestions.length + 1) %
+          suggestions.length,
       });
       e.preventDefault();
     }
@@ -197,7 +229,7 @@ export default class ExpressionEditorTextfield extends React.Component {
   clearSuggestions() {
     this.setState({
       suggestions: [],
-      highlightedSuggestion: 0,
+      highlightedSuggestionIndex: 0,
     });
   }
 
@@ -250,11 +282,19 @@ export default class ExpressionEditorTextfield extends React.Component {
       suggestions,
       helpText,
       syntaxTree,
-    } = this._processSource({
-      source,
-      targetOffset,
-      ...this._getParserOptions(),
-    });
+    } = source
+      ? this._processSource({
+          source,
+          targetOffset,
+          ...this._getParserOptions(),
+        })
+      : {
+          expression: null,
+          compileError: null,
+          suggestions: [],
+          helpText: null,
+          syntaxTree: null,
+        };
 
     const isValid = expression !== undefined;
     // don't show suggestions if
@@ -270,17 +310,13 @@ export default class ExpressionEditorTextfield extends React.Component {
       compileError,
       suggestions: showSuggestions ? suggestions : [],
       helpText,
+      highlightedSuggestionIndex: 0,
     });
   }
 
   render() {
     const { placeholder } = this.props;
-    let { compileError } = this.state;
-    const { source, suggestions, helpText, syntaxTree } = this.state;
-
-    if (compileError && !compileError.length) {
-      compileError = t`unknown error`;
-    }
+    const { compileError, source, suggestions, syntaxTree } = this.state;
 
     const inputClassName = cx("input text-bold text-monospace", {
       "text-dark": source,
@@ -318,104 +354,13 @@ export default class ExpressionEditorTextfield extends React.Component {
           onClick={this.onInputClick}
           autoFocus
         />
-        {helpText && (
-          <Popover
-            tetherOptions={{
-              attachment: "top left",
-              targetAttachment: "bottom left",
-            }}
-            style={{ width: this.props.width }}
-            isOpen
-          >
-            <p
-              className="p2 m0 text-monospace text-bold"
-              style={{ background: colors["bg-yellow"] }}
-            >
-              {helpText.structure}
-            </p>
-            <div className="p2 border-top">
-              <p className="mt0 text-bold">{helpText.description}</p>
-              <p className="text-code m0 text-body">{helpText.example}</p>
-            </div>
-            <div className="p2 border-top">
-              {helpText.args.map(({ name, description }) => (
-                <div>
-                  <h4 className="text-medium">{name}</h4>
-                  <p className="mt1 text-bold">{description}</p>
-                </div>
-              ))}
-              <a
-                className="link text-bold block my1"
-                target="_blank"
-                href={MetabaseSettings.docsUrl("users-guide/expressions")}
-              >
-                <Icon name="reference" size={12} className="mr1" />
-                {t`Learn more`}
-              </a>
-            </div>
-          </Popover>
-        )}
-        {suggestions.length ? (
-          <Popover
-            className="not-rounded border-dark"
-            hasArrow={false}
-            tetherOptions={{
-              attachment: "top left",
-              targetAttachment: "bottom left",
-            }}
-            sizeToFit
-          >
-            <ul className="pb1" style={{ minWidth: 150, overflowY: "auto" }}>
-              {suggestions.map((suggestion, i) =>
-                // insert section title. assumes they're sorted by type
-                [
-                  (i === 0 || suggestion.type !== suggestions[i - 1].type) && (
-                    <li className="mx2 h6 text-uppercase text-bold text-medium py1 pt2">
-                      {SUGGESTION_SECTION_NAMES[suggestion.type] ||
-                        suggestion.type}
-                    </li>
-                  ),
-                  <li
-                    ref={r => {
-                      if (i === this.state.highlightedSuggestion) {
-                        this._selectedRow = r;
-                      }
-                    }}
-                    style={{ paddingTop: 5, paddingBottom: 5 }}
-                    className={cx(
-                      "px2 cursor-pointer text-white-hover bg-brand-hover hover-parent hover--inherit",
-                      {
-                        "text-white bg-brand":
-                          i === this.state.highlightedSuggestion,
-                      },
-                    )}
-                    onMouseDownCapture={e => this.onSuggestionMouseDown(e, i)}
-                  >
-                    {suggestion.range ? (
-                      <span>
-                        {suggestion.name.slice(0, suggestion.range[0])}
-                        <span
-                          className={cx("text-brand text-bold hover-child", {
-                            "text-white bg-brand":
-                              i === this.state.highlightedSuggestion,
-                          })}
-                        >
-                          {suggestion.name.slice(
-                            suggestion.range[0],
-                            suggestion.range[1],
-                          )}
-                        </span>
-                        {suggestion.name.slice(suggestion.range[1])}
-                      </span>
-                    ) : (
-                      suggestion.name
-                    )}
-                  </li>,
-                ],
-              )}
-            </ul>
-          </Popover>
-        ) : null}
+        <Errors compileError={compileError} />
+        <HelpText helpText={this.state.helpText} width={this.props.width} />
+        <ExpressionEditorSuggestions
+          suggestions={suggestions}
+          onSuggestionMouseDown={this.onSuggestionSelected}
+          highlightedIndex={this.state.highlightedSuggestionIndex}
+        />
       </div>
     );
   }