Fix - Native editor buttons are incorrectly rendering on top of model metadata screen (#31142)

* Make NativeQueryEditor testable
- Mock just NativeQueryEditor.prototype.loadAceEditor instead of entire NativeQueryEditor

* Fix invalid propTypes

* Fix DatasetQueryEditor not re-rendering when props change
- remove custom arePropsEqual argument from React.memo() on NativeQueryEditor
- Fixes #30680

* Add a dirty initial NativeQueryEditor test setup

* Add DatasetQueryEditor tests

* Revert "Add a dirty initial NativeQueryEditor test setup"

This reverts commit 0f985db272a2e8f1be755bda07c3b642fdf90f83.

* Get rid of test errors

* Clean up DatasetQueryEditor test

* Add a test case for re-rendering of DatasetQueryEditor

* Make setup function more reusable

* Add 2 more test cases

* Bring back global NativeQueryEditor mock as many tests depend on it

* Add an e2e test for #30680

* Optimize DatasetEditor by conditionally rendering DatasetQueryEditor only when it can be seen

* Remove redundant code

* ESLint

* Use createMockCollection instead of a cast

* Move all Native Query creation to createMockNativeDatasetQuery

* Use setupCollectionsEndpoints

* Introduce setupNativeQuerySnippetEndpoints and use it in DatasetQueryEditor.unit.spec.tsx

* Extract explanatory importDatasetQueryEditor

* Remove file accidentally added when merging master

* Add missing data-testid that got lost during a merge

e2e/test/scenarios/native/reproductions/30680-native-editor-buttons.cy.spec.js

+import { restore, runNativeQuery } from "e2e/support/helpers";
+
+describe("issue 30680", () => {
+  beforeEach(() => {
+    restore();
+    cy.signInAsAdmin();
+  });
+
+  it("should not render native editor buttons when 'Metadata' tab is open", () => {
+    cy.visit("/model/new");
+
+    cy.findByTestId("new-model-options")
+      .findByText("Use a native query")
+      .click();
+
+    cy.get(".ace_editor").type("select * from orders ");
+    runNativeQuery();
+
+    cy.findByTestId("editor-tabs-metadata-name").click();
+
+    cy.findByTestId("native-query-editor-sidebar").should("not.exist");
+  });
+});

frontend/src/metabase/models/containers/NewModelOptions/NewModelOptions.tsx

@@ -50,7 +50,7 @@ const NewModelOptions = (props: NewModelOptionsProps) => {
   const itemsCount = (hasDataAccess ? 1 : 0) + (hasNativeWrite ? 1 : 0);
 
   return (
-    <OptionsRoot>
+    <OptionsRoot data-testid="new-model-options">
       <Grid className="justifyCenter">
         {hasDataAccess && (
           <OptionsGridItem itemsCount={itemsCount}>

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

@@ -463,13 +463,21 @@ function DatasetEditor(props) {
       <Root>
         <MainContainer>
           <QueryEditorContainer isResizable={isEditingQuery}>
-            <DatasetQueryEditor
-              {...props}
-              isActive={isEditingQuery}
-              height={editorHeight}
-              viewHeight={height}
-              onResizeStop={handleResize}
-            />
+            {/**
+             * Optimization: DatasetQueryEditor can be expensive to re-render
+             * and we don't need it on the "Metadata" tab.
+             *
+             * @see https://github.com/metabase/metabase/pull/31142/files#r1211352364
+             */}
+            {isEditingQuery && editorHeight > 0 && (
+              <DatasetQueryEditor
+                {...props}
+                isActive={isEditingQuery}
+                height={editorHeight}
+                viewHeight={height}
+                onResizeStop={handleResize}
+              />
+            )}
           </QueryEditorContainer>
           <TableContainer isSidebarOpen={!!sidebar}>
             <DebouncedFrame className="flex-full" enabled>

frontend/src/metabase/query_builder/components/DatasetEditor/DatasetQueryEditor.jsx

@@ -93,9 +93,4 @@ function DatasetQueryEditor({
 
 DatasetQueryEditor.propTypes = propTypes;
 
-export default memo(
-  DatasetQueryEditor,
-  // should prevent the editor from re-rendering in "metadata" mode
-  // when it's completely covered with the results table
-  (prevProps, nextProps) => prevProps.height === 0 && nextProps.height === 0,
-);
+export default memo(DatasetQueryEditor);

frontend/src/metabase/query_builder/components/DatasetEditor/DatasetQueryEditor.unit.spec.tsx

+import { screen } from "@testing-library/react";
+import _ from "underscore";
+
+import {
+  setupCollectionsEndpoints,
+  setupDatabasesEndpoints,
+  setupNativeQuerySnippetEndpoints,
+} from "__support__/server-mocks";
+import { createMockEntitiesState } from "__support__/store";
+import { renderWithProviders } from "__support__/ui";
+import { Card } from "metabase-types/api";
+import {
+  createMockCard,
+  createMockCollection,
+  createMockNativeDatasetQuery,
+} from "metabase-types/api/mocks";
+import { createSampleDatabase } from "metabase-types/api/mocks/presets";
+import { createMockState } from "metabase-types/store/mocks";
+import { checkNotNull } from "metabase/core/utils/types";
+import { getMetadata } from "metabase/selectors/metadata";
+
+const { NativeQueryEditor } = jest.requireActual(
+  "metabase/query_builder/components/NativeQueryEditor",
+);
+
+const TEST_DB = createSampleDatabase();
+
+const TEST_NATIVE_CARD = createMockCard({
+  dataset_query: createMockNativeDatasetQuery({
+    type: "native",
+    database: TEST_DB.id,
+    native: {
+      query: "select * from orders",
+      "template-tags": undefined,
+    },
+  }),
+});
+
+const ROOT_COLLECTION = createMockCollection({ id: "root" });
+
+interface SetupOpts {
+  card?: Card;
+  height?: number;
+  isActive: boolean;
+  readOnly?: boolean;
+}
+
+const setup = async ({
+  card = TEST_NATIVE_CARD,
+  height = 300,
+  isActive,
+  readOnly = false,
+}: SetupOpts) => {
+  setupDatabasesEndpoints([TEST_DB]);
+  setupCollectionsEndpoints([ROOT_COLLECTION]);
+  setupNativeQuerySnippetEndpoints();
+
+  const storeInitialState = createMockState({
+    entities: createMockEntitiesState({
+      databases: [createSampleDatabase()],
+      questions: [card],
+    }),
+  });
+  const metadata = getMetadata(storeInitialState);
+  const question = checkNotNull(metadata.question(card.id));
+  const query = question.query();
+  const DatasetQueryEditor = await importDatasetQueryEditor();
+
+  const { rerender } = renderWithProviders(
+    <DatasetQueryEditor
+      isActive={isActive}
+      height={height}
+      query={query}
+      question={question}
+      readOnly={readOnly}
+      onResizeStop={_.noop}
+    />,
+  );
+
+  return { query, question, rerender };
+};
+
+/**
+ * NativeQueryEditor is globally mocked in test/register-visualizations.js but
+ * its actual implementation is needed in this test suite because we need to
+ * investigate its children.
+ *
+ * We're actually testing NativeQueryEditor indirectly by using DatasetQueryEditor
+ * (which uses NativeQueryEditor), so the NativeQueryEditor has to be unmocked
+ * the moment we import DatasetQueryEditor.
+ *
+ * Unmocking happens in beforeEach, so we can really only import the component
+ * during the unit test.
+ *
+ * Should the import be at the beginning of this file, the mock NativeQueryEditor
+ * would have been used in tests instead of the actual implementation.
+ */
+const importDatasetQueryEditor = async () => {
+  const { default: DatasetQueryEditor } = await import(
+    "metabase/query_builder/components/DatasetEditor/DatasetQueryEditor"
+  );
+  return DatasetQueryEditor;
+};
+
+describe("DatasetQueryEditor", () => {
+  beforeEach(() => {
+    jest.unmock("metabase/query_builder/components/NativeQueryEditor");
+
+    jest
+      .spyOn(NativeQueryEditor.prototype, "loadAceEditor")
+      .mockImplementation(_.noop);
+  });
+
+  it("renders sidebar when query tab is active", async () => {
+    await setup({ isActive: true });
+
+    expect(
+      screen.getByTestId("native-query-editor-sidebar"),
+    ).toBeInTheDocument();
+  });
+
+  it("shows the native query editor container when query tab is active", async () => {
+    await setup({ isActive: true });
+
+    expect(screen.getByTestId("native-query-editor-container")).toBeVisible();
+  });
+
+  it("does not render sidebar when query tab is inactive", async () => {
+    await setup({ isActive: false });
+
+    expect(
+      screen.queryByTestId("native-query-editor-sidebar"),
+    ).not.toBeInTheDocument();
+  });
+
+  it("hides the native query editor container when query tab is inactive", async () => {
+    await setup({ isActive: false });
+
+    expect(
+      screen.getByTestId("native-query-editor-container"),
+    ).not.toBeVisible();
+  });
+
+  it("re-renders DatasetQueryEditor when height is 0 and isActive prop changes", async () => {
+    const { query, question, rerender } = await setup({
+      height: 0,
+      isActive: true,
+    });
+    const DatasetQueryEditor = await importDatasetQueryEditor();
+
+    expect(
+      screen.getByTestId("native-query-editor-sidebar"),
+    ).toBeInTheDocument();
+
+    rerender(
+      <DatasetQueryEditor
+        isActive={false}
+        height={0}
+        query={query}
+        question={question}
+        readOnly={false}
+        onResizeStop={_.noop}
+      />,
+    );
+
+    expect(
+      screen.queryByTestId("native-query-editor-sidebar"),
+    ).not.toBeInTheDocument();
+  });
+});

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

@@ -59,7 +59,7 @@ import { NativeQueryEditorRoot } from "./NativeQueryEditor.styled";
 const AUTOCOMPLETE_DEBOUNCE_DURATION = 700;
 const AUTOCOMPLETE_CACHE_DURATION = AUTOCOMPLETE_DEBOUNCE_DURATION * 1.2; // tolerate 20%
 
-class NativeQueryEditor extends Component {
+export class NativeQueryEditor extends Component {
   _localUpdate = false;
 
   constructor(props) {

frontend/src/metabase/query_builder/components/NativeQueryEditor/NativeQueryEditorSidebar/NativeQueryEditorSidebar.jsx

@@ -27,12 +27,12 @@ const propTypes = {
   runQuery: PropTypes.func,
   snippetCollections: PropTypes.array,
   snippets: PropTypes.array,
-  features: {
+  features: PropTypes.shape({
     dataReference: PropTypes.bool,
     variables: PropTypes.bool,
     snippets: PropTypes.bool,
     promptInput: PropTypes.bool,
-  },
+  }),
   onShowPromptInput: PropTypes.func,
 };
 
@@ -76,7 +76,7 @@ const NativeQueryEditorSidebar = props => {
   const canRunQuery = runQuery && cancelQuery;
 
   return (
-    <Container>
+    <Container data-testid="native-query-editor-sidebar">
       {canUsePromptInput && features.promptInput && !isPromptInputVisible ? (
         <Tooltip tooltip={t`Ask a question`}>
           <SidebarButton

frontend/test/__support__/server-mocks/index.ts

@@ -14,6 +14,7 @@ export * from "./group";
 export * from "./metabot";
 export * from "./metric";
 export * from "./model-indexes";
+export * from "./native-query-snippet";
 export * from "./search";
 export * from "./segment";
 export * from "./session";

frontend/test/__support__/server-mocks/native-query-snippet.ts

+import fetchMock from "fetch-mock";
+
+export function setupNativeQuerySnippetEndpoints() {
+  fetchMock.get("path:/api/native-query-snippet", () => []);
+}