Clean up `NotebookSteps` component (#29554)

* Remove commented out code

* Fix typo

* Convert `steps` to TypeScript

* Convert `NotebookSteps` to TypeScript

* Turn `NotebookSteps` into functional component

* Turn `NotebookSteps` into a functional component

* Move `NotebookSteps` into its own directory

* Extract styled component

* Fix type

frontend/src/metabase/query_builder/components/notebook/Notebook.tsx

@@ -23,7 +23,7 @@ interface NotebookOwnProps {
   isRunnable: boolean;
   isResultDirty: boolean;
   hasVisualizeButton?: boolean;
-  updateQuestion: (question: Question) => void;
+  updateQuestion: (question: Question) => Promise<void>;
   runQuestionQuery: () => void;
   setQueryBuilderMode: (mode: string) => void;
 }
@@ -61,7 +61,7 @@ const Notebook = ({ className, ...props }: NotebookProps) => {
     await updateQuestion(cleanQuestion);
   }
 
-  // vizualize switches the view to the question's visualization.
+  // visualize switches the view to the question's visualization.
   async function visualize() {
     // Only cleanup the question if it's dirty, otherwise Metabase
     // will incorrectly display the Save button, even though there are no changes to save.

frontend/src/metabase/query_builder/components/notebook/NotebookSteps.jsx

-/* eslint-disable react/prop-types */
-import React from "react";
-
-import cx from "classnames";
-import NotebookStep from "./NotebookStep";
-
-import { getQuestionSteps } from "./lib/steps";
-
-export default class NotebookSteps extends React.Component {
-  constructor(props) {
-    super(props);
-    const isNew = !props.question.table();
-    this.state = {
-      openSteps: isNew
-        ? {
-            "0:filter": true,
-            // "0:aggregate": true,
-            "0:summarize": true,
-          }
-        : {},
-      lastOpenedStep: null,
-    };
-  }
-
-  openStep = id => {
-    this.setState({
-      openSteps: { ...this.state.openSteps, [id]: true },
-      lastOpenedStep: id,
-    });
-  };
-
-  closeStep = id => {
-    this.setState({
-      openSteps: { ...this.state.openSteps, [id]: false },
-      lastOpenedStep:
-        this.state.lastOpenedStep === id ? null : this.state.lastOpenedStep,
-    });
-  };
-
-  render() {
-    const {
-      question,
-      className,
-      reportTimezone,
-      sourceQuestion,
-      updateQuestion,
-    } = this.props;
-    const { openSteps, lastOpenedStep } = this.state;
-
-    if (!question) {
-      return null;
-    }
-
-    const steps = getQuestionSteps(question, openSteps);
-
-    return (
-      <div className={cx(className, "pt3")}>
-        {steps.map((step, index) => {
-          // pass a version of updateQuery that cleans subsequent steps etc
-          const updateQuery = async query => {
-            const datasetQuery = query.datasetQuery();
-            const updatedQuery = step.update(datasetQuery);
-            await updateQuestion(updatedQuery.question());
-            // mark the step as "closed" since we can assume it's been added or removed by the updateQuery
-            this.closeStep(step.id);
-          };
-          return (
-            <NotebookStep
-              key={step.id}
-              step={step}
-              sourceQuestion={sourceQuestion}
-              updateQuery={updateQuery}
-              openStep={this.openStep}
-              closeStep={this.closeStep}
-              isLastStep={index === steps.length - 1}
-              isLastOpened={lastOpenedStep === step.id}
-              reportTimezone={reportTimezone}
-            />
-          );
-        })}
-      </div>
-    );
-  }
-}

frontend/src/metabase/query_builder/components/notebook/NotebookSteps/NotebookSteps.styled.tsx

+import styled from "@emotion/styled";
+
+export const Container = styled.div`
+  padding-top: 1.5rem;
+`;

frontend/src/metabase/query_builder/components/notebook/NotebookSteps/NotebookSteps.tsx

+import React, { useCallback, useMemo, useState } from "react";
+
+import type Question from "metabase-lib/Question";
+import type StructuredQuery from "metabase-lib/queries/StructuredQuery";
+
+import { getQuestionSteps } from "../lib/steps";
+import { NotebookStep as INotebookStep } from "../lib/steps.types";
+import NotebookStep from "../NotebookStep";
+import { Container } from "./NotebookSteps.styled";
+
+interface NotebookStepsProps {
+  className?: string;
+  question: Question;
+  sourceQuestion?: Question;
+  reportTimezone?: string;
+  updateQuestion: (question: Question) => Promise<void>;
+}
+
+type OpenSteps = { [key: string]: boolean };
+
+function getInitialOpenSteps(question: Question): OpenSteps {
+  const isNew = !question.table();
+  return isNew
+    ? {
+        "0:filter": true,
+        "0:summarize": true,
+      }
+    : {};
+}
+
+function NotebookSteps({
+  className,
+  question,
+  sourceQuestion,
+  reportTimezone,
+  updateQuestion,
+}: NotebookStepsProps) {
+  const [openSteps, setOpenSteps] = useState<OpenSteps>(
+    getInitialOpenSteps(question),
+  );
+  const [lastOpenedStep, setLastOpenedStep] = useState<string | null>(null);
+
+  const steps = useMemo(() => {
+    if (!question) {
+      return [];
+    }
+    return getQuestionSteps(question, openSteps);
+  }, [question, openSteps]);
+
+  const handleStepOpen = useCallback((id: string) => {
+    setOpenSteps(openSteps => ({ ...openSteps, [id]: true }));
+    setLastOpenedStep(id);
+  }, []);
+
+  const handleStepClose = useCallback((id: string) => {
+    setOpenSteps(openSteps => ({ ...openSteps, [id]: false }));
+    setLastOpenedStep(lastOpenedStep =>
+      lastOpenedStep === id ? null : lastOpenedStep,
+    );
+  }, []);
+
+  const handleQueryChange = useCallback(
+    async (step: INotebookStep, query: StructuredQuery) => {
+      const datasetQuery = query.datasetQuery();
+      const updatedQuery = step.update(datasetQuery);
+      await updateQuestion(updatedQuery.question());
+
+      // mark the step as "closed" since we can assume
+      // it's been added or removed by the updateQuery
+      handleStepClose(step.id);
+    },
+    [updateQuestion, handleStepClose],
+  );
+
+  if (!question) {
+    return null;
+  }
+
+  return (
+    <Container className={className}>
+      {steps.map((step, index) => {
+        const isLast = index === steps.length - 1;
+        const isLastOpened = lastOpenedStep === step.id;
+        const onChange = (query: StructuredQuery) =>
+          handleQueryChange(step, query);
+
+        return (
+          <NotebookStep
+            key={step.id}
+            step={step}
+            sourceQuestion={sourceQuestion}
+            isLastStep={isLast}
+            isLastOpened={isLastOpened}
+            reportTimezone={reportTimezone}
+            updateQuery={onChange}
+            openStep={handleStepOpen}
+            closeStep={handleStepClose}
+          />
+        );
+      })}
+    </Container>
+  );
+}
+
+export default NotebookSteps;

frontend/src/metabase/query_builder/components/notebook/NotebookSteps/index.ts

+export { default } from "./NotebookSteps";

frontend/src/metabase/query_builder/components/notebook/lib/steps.js → frontend/src/metabase/query_builder/components/notebook/lib/steps.ts

 import _ from "underscore";
-import StructuredQuery from "metabase-lib/queries/StructuredQuery";
+
+import type Question from "metabase-lib/Question";
+import type StructuredQuery from "metabase-lib/queries/StructuredQuery";
+
+import { NotebookStep, NotebookStepFn } from "./steps.types";
 
 // This converts an MBQL query into a sequence of notebook "steps", with special logic to determine which steps are
 // allowed to be added at every other step, generating a preview query at each step, how to delete a step,
@@ -7,24 +11,32 @@ import StructuredQuery from "metabase-lib/queries/StructuredQuery";
 
 // identifier for this step, e.x. `0:data` (or `0:join:1` for sub-steps)
 
-const STEPS = [
+type NotebookStepDef = Pick<NotebookStep, "type" | "clean" | "revert"> & {
+  valid: NotebookStepFn<boolean>;
+  active: NotebookStepFn<boolean>;
+  subSteps?: (query: StructuredQuery) => number;
+};
+
+const STEPS: NotebookStepDef[] = [
   {
     type: "data",
     valid: query => !query.sourceQuery(),
     active: query => true,
     clean: query => query,
+    revert: null,
   },
   {
     type: "join",
-    valid: query => query.hasData() && query.database().hasFeature("join"),
-    // active: query => query.hasJoins(),
-    // revert: query => query.clearJoins(),
-    // clean: query => query.cleanJoins(),
+    valid: query => {
+      const database = query.database();
+      return query.hasData() && database != null && database.hasFeature("join");
+    },
     subSteps: query => query.joins().length,
-    active: (query, index) => query.joins().length > index,
+    active: (query, index) =>
+      typeof index === "number" && query.joins().length > index,
     revert: (query, index) => query.removeJoin(index),
     clean: (query, index) => {
-      const join = query.joins()[index];
+      const join = typeof index === "number" ? query.joins()[index] : null;
       if (!join || join.isValid() || join.hasGaps()) {
         return query;
       }
@@ -37,7 +49,12 @@ const STEPS = [
   },
   {
     type: "expression",
-    valid: query => query.hasData() && query.database().supportsExpressions(),
+    valid: query => {
+      const database = query.database();
+      return (
+        query.hasData() && database != null && database.supportsExpressions()
+      );
+    },
     active: query => query.hasExpressions(),
     revert: query => query.clearExpressions(),
     clean: query => query.cleanExpressions(),
@@ -49,20 +66,6 @@ const STEPS = [
     revert: query => query.clearFilters(),
     clean: query => query.cleanFilters(),
   },
-  // {
-  //   type: "aggregate",
-  //   valid: query => query.hasData(),
-  //   active: query => query.hasAggregations,
-  //   revert: query => query.clearAggregations(),
-  //   clean: query => query.cleanAggregations(),
-  // },
-  // {
-  //   type: "breakout",
-  //   valid: query => query.hasData() && query.hasAggregations() ,
-  //   active: query => query.hasBreakouts(),
-  //   revert: query => query.clearBreakouts(),
-  //   clean: query => query.cleanBreakouts(),
-  // },
   {
     // NOTE: summarize is a combination of aggregate and breakout
     type: "summarize",
@@ -100,11 +103,11 @@ const STEPS = [
 /**
  * Returns an array of "steps" to be displayed in the notebook for one "stage" (nesting) of a query
  */
-export function getQuestionSteps(question, openSteps = {}) {
-  const allSteps = [];
+export function getQuestionSteps(question: Question, openSteps = {}) {
+  const allSteps: NotebookStep[] = [];
 
-  let query = question.query();
-  if (query instanceof StructuredQuery) {
+  if (question.isStructured()) {
+    let query = question.query() as StructuredQuery;
     const database = question.database();
     const allowsNesting = database && database.hasFeature("nested-queries");
 
@@ -142,13 +145,21 @@ export function getQuestionSteps(question, openSteps = {}) {
 /**
  * Returns an array of "steps" to be displayed in the notebook for one "stage" (nesting) of a query
  */
-export function getStageSteps(stageQuery, stageIndex, openSteps) {
-  const getId = (step, itemIndex) =>
-    `${stageIndex}:${step.type}` + (itemIndex > 0 ? `:${itemIndex}` : ``);
-
-  function getStep(STEP, itemIndex = null) {
+export function getStageSteps(
+  stageQuery: StructuredQuery,
+  stageIndex: number,
+  openSteps: Record<NotebookStep["id"], boolean>,
+) {
+  const getId = (step: NotebookStepDef, itemIndex: number | null) => {
+    const isValidItemIndex = itemIndex != null && itemIndex > 0;
+    return (
+      `${stageIndex}:${step.type}` + (isValidItemIndex ? `:${itemIndex}` : "")
+    );
+  };
+
+  function getStep(STEP: NotebookStepDef, itemIndex: number | null = null) {
     const id = getId(STEP, itemIndex);
-    const step = {
+    const step: NotebookStep = {
       id: id,
       type: STEP.type,
       stageIndex: stageIndex,
@@ -158,14 +169,17 @@ export function getStageSteps(stageQuery, stageIndex, openSteps) {
       active: STEP.active(stageQuery, itemIndex),
       visible:
         STEP.valid(stageQuery, itemIndex) &&
-        (STEP.active(stageQuery, itemIndex) || openSteps[id]),
-      revert: STEP.revert ? query => STEP.revert(query, itemIndex) : null,
+        !!(STEP.active(stageQuery, itemIndex) || openSteps[id]),
+      revert: STEP.revert
+        ? (query: StructuredQuery) =>
+            STEP.revert ? STEP.revert(query, itemIndex) : null
+        : null,
       clean: query => STEP.clean(query, itemIndex),
       update: datasetQuery => {
         let newQuery = stageQuery.setDatasetQuery(datasetQuery);
         // clean each subsequent step individually. we have to do this rather than calling newQuery.clean() in case
         // the current step is in a temporarily invalid state
-        let current = step;
+        let current: NotebookStep | null = step;
         while ((current = current.next)) {
           // when switching to the next stage we need to setSourceQuery
           if (
@@ -201,7 +215,7 @@ export function getStageSteps(stageQuery, stageIndex, openSteps) {
     }),
   );
 
-  let previewQuery = stageQuery;
+  let previewQuery: StructuredQuery | null = stageQuery;
 
   let actions = [];
   // iterate over steps in reverse so we can revert query for previewing and accumulate valid actions
@@ -218,13 +232,17 @@ export function getStageSteps(stageQuery, stageIndex, openSteps) {
       if (step.valid) {
         actions.unshift({
           type: step.type,
-          action: ({ openStep }) => openStep(step.id),
+          action: ({
+            openStep,
+          }: {
+            openStep: (id: NotebookStep["id"]) => void;
+          }) => openStep(step.id),
         });
       }
       steps.splice(i, 1);
     }
     // revert the previewQuery for this step
-    if (step.revert) {
+    if (step.revert && previewQuery) {
       previewQuery = step.revert(previewQuery);
     }
   }

frontend/src/metabase/query_builder/components/notebook/lib/steps.types.ts

@@ -13,17 +13,22 @@ export type NotebookStepType =
   | "sort"
   | "limit";
 
+export type NotebookStepFn<ReturnType> = (
+  query: StructuredQuery,
+  index?: number | null,
+) => ReturnType;
+
 export interface NotebookStep {
   id: string;
   type: NotebookStepType;
   stageIndex: number;
-  itemIndex: number;
+  itemIndex: number | null;
   query: StructuredQuery;
   valid: boolean;
   active: boolean;
   visible: boolean;
-  revert: ((query: StructuredQuery) => StructuredQuery | null) | null;
-  clean: (query: StructuredQuery) => StructuredQuery;
+  revert: NotebookStepFn<StructuredQuery | null> | null;
+  clean: NotebookStepFn<StructuredQuery>;
   update: (datasetQuery: DatasetQuery) => StructuredQuery;
   actions: NotebookStepAction[];
   previewQuery: StructuredQuery | null;