Scroll to created column when extracting or combining columns in chill mode (#42650) (#42832)

* Set scrollToLastColumn ui control from column extract action

* Scroll to last column when the uiControl is set

* Scroll to last column when combining a column

* Scroll to last column when combining a column via the + shortcut

* Scroll to last column when extracting a column via the + shortcut

* Remove all references to settingsSyncOptions

* Check that the column is added at the end

* Disable scrollToLastColumn once used

* Add test that verifies scrollToLastColumn ui control has been reset

Co-authored-by: Romeo Van Snick <romeo@romeovansnick.be>

e2e/test/scenarios/visualizations-tabular/column-shortcuts/column-shortcuts.cy.spec.ts

@@ -169,7 +169,6 @@ describeWithSnowplow("extract shortcut", () => {
             query: {
               "source-table": PEOPLE_ID,
               limit: 1,
-              fields: [["field", PEOPLE.EMAIL, null]],
             },
           },
           {
@@ -210,7 +209,6 @@ describeWithSnowplow("extract shortcut", () => {
             query: {
               "source-table": PEOPLE_ID,
               limit: 1,
-              fields: [["field", PEOPLE.EMAIL, { "base-type": "type/String" }]],
             },
           },
           {
@@ -232,6 +230,37 @@ describeWithSnowplow("extract shortcut", () => {
       });
     });
   });
+
+  it("should disable the scroll behaviour after it has been rendered", () => {
+    createQuestion(
+      {
+        query: {
+          "source-table": PEOPLE_ID,
+          limit: 1,
+        },
+      },
+      {
+        visitQuestion: true,
+      },
+    );
+
+    extractColumnAndCheck({
+      column: "Email",
+      option: "Host",
+    });
+
+    cy.get("#main-data-grid").scrollTo("left", { duration: 2000 / 60 });
+
+    cy.findAllByRole("columnheader", { name: "ID" })
+      .should("be.visible")
+      .click();
+
+    // Change sort direction
+    popover().findAllByRole("button").first().click();
+
+    // ID should still be visible (ie. no scrolling to the end should have happened)
+    cy.findAllByRole("columnheader", { name: "ID" }).should("be.visible");
+  });
 });
 
 function extractColumnAndCheck({
@@ -262,7 +291,12 @@ function extractColumnAndCheck({
 
   cy.wait(`@${requestAlias}`);
 
-  cy.findByRole("columnheader", { name: newColumn }).should("be.visible");
+  cy.findAllByRole("columnheader")
+    .last()
+    .should("have.text", newColumn)
+    .should("be.visible");
+
+  cy.findAllByRole("columnheader").last().should("have.text", newColumn);
   if (value) {
     cy.findByRole("gridcell", { name: value }).should("be.visible");
   }

e2e/test/scenarios/visualizations-tabular/column-shortcuts/combine-column-shortcut.cy.spec.ts

@@ -85,7 +85,10 @@ function combineColumns({
 
   cy.wait(`@${requestAlias}`);
 
-  cy.findByRole("columnheader", { name: newColumn }).should("be.visible");
+  cy.findAllByRole("columnheader")
+    .last()
+    .should("have.text", newColumn)
+    .should("be.visible");
 
   if (newValue) {
     cy.findByRole("gridcell", { name: newValue }).should("be.visible");

e2e/test/scenarios/visualizations-tabular/drillthroughs/column_extract_drill.cy.spec.js

@@ -132,15 +132,6 @@ describeWithSnowplow("extract action", () => {
           option: "Year",
           extraction: "Extract day, month…",
         });
-        const columnIndex = 7;
-        checkColumnIndex({
-          column: "Created At",
-          columnIndex,
-        });
-        checkColumnIndex({
-          column: "Year",
-          columnIndex: columnIndex + 1,
-        });
       });
 
       it("saved question without viz settings", () => {
@@ -150,15 +141,6 @@ describeWithSnowplow("extract action", () => {
           option: "Year",
           extraction: "Extract day, month…",
         });
-        const columnIndex = 7;
-        checkColumnIndex({
-          column: "Created At",
-          columnIndex,
-        });
-        checkColumnIndex({
-          column: "Year",
-          columnIndex: columnIndex + 1,
-        });
       });
 
       it("saved question with viz settings", () => {
@@ -205,15 +187,6 @@ describeWithSnowplow("extract action", () => {
           option: "Year",
           extraction: "Extract day, month…",
         });
-        const columnIndex = 1;
-        checkColumnIndex({
-          column: "Created At",
-          columnIndex,
-        });
-        checkColumnIndex({
-          column: "Year",
-          columnIndex: columnIndex + 1,
-        });
       });
     });
 
@@ -351,16 +324,16 @@ function extractColumnAndCheck({
   popover().findByText(option).click();
   cy.wait(`@${requestAlias}`);
 
-  cy.findByRole("columnheader", { name: newColumn }).should("be.visible");
+  cy.findAllByRole("columnheader")
+    .last()
+    .should("have.text", newColumn)
+    .should("be.visible");
+
   if (value) {
     cy.findByRole("gridcell", { name: value }).should("be.visible");
   }
 }
 
-function checkColumnIndex({ column, columnIndex }) {
-  cy.findAllByRole("columnheader").eq(columnIndex).should("have.text", column);
-}
-
 describeWithSnowplow("extract action", () => {
   beforeEach(() => {
     restore();

frontend/src/metabase/query_builder/actions/core/core.js

@@ -114,7 +114,7 @@ export const reloadCard = createThunkAction(RELOAD_CARD, () => {
 export const SET_CARD_AND_RUN = "metabase/qb/SET_CARD_AND_RUN";
 export const setCardAndRun = (
   nextCard,
-  { shouldUpdateUrl = true, prevQueryResults, settingsSyncOptions } = {},
+  { shouldUpdateUrl = true, prevQueryResults } = {},
 ) => {
   return async (dispatch, getState) => {
     // clone
@@ -135,7 +135,6 @@ export const setCardAndRun = (
       runQuestionQuery({
         shouldUpdateUrl,
         prevQueryResults,
-        settingsSyncOptions,
       }),
     );
 
@@ -157,16 +156,17 @@ export const setCardAndRun = (
 export const NAVIGATE_TO_NEW_CARD = "metabase/qb/NAVIGATE_TO_NEW_CARD";
 export const navigateToNewCardInsideQB = createThunkAction(
   NAVIGATE_TO_NEW_CARD,
-  ({ nextCard, previousCard, objectId, settingsSyncOptions }) => {
+  ({ nextCard, previousCard, objectId }) => {
     return async (dispatch, getState) => {
       if (previousCard === nextCard) {
         // Do not reload questions with breakouts when clicked on a legend item
       } else if (cardIsEquivalent(previousCard, nextCard)) {
         // This is mainly a fallback for scenarios where a visualization legend is clicked inside QB
         dispatch(
-          setCardAndRun(await loadCard(nextCard.id, { dispatch, getState }), {
-            settingsSyncOptions,
-          }),
+          setCardAndRun(
+            await loadCard(nextCard.id, { dispatch, getState }),
+            {},
+          ),
         );
       } else {
         const card = getCardAfterVisualizationClick(nextCard, previousCard);
@@ -183,10 +183,7 @@ export const navigateToNewCardInsideQB = createThunkAction(
           // When the dataset query changes, we should change the type,
           // to start building a new ad-hoc question based on a dataset
           dispatch(
-            setCardAndRun(
-              { ...card, type: "question" },
-              { prevQueryResults, settingsSyncOptions },
-            ),
+            setCardAndRun({ ...card, type: "question" }, { prevQueryResults }),
           );
         }
         if (objectId !== undefined) {

frontend/src/metabase/query_builder/actions/querying.js

@@ -95,7 +95,6 @@ export const runQuestionQuery = ({
   ignoreCache = false,
   overrideWithQuestion = null,
   prevQueryResults = undefined,
-  settingsSyncOptions = undefined,
 } = {}) => {
   return async (dispatch, getState) => {
     dispatch(loadStartUIControls());
@@ -140,7 +139,6 @@ export const runQuestionQuery = ({
         return dispatch(
           queryCompleted(question, queryResults, {
             prevQueryResults: prevQueryResults ?? getQueryResults(getState()),
-            settingsSyncOptions,
           }),
         );
       })
@@ -180,7 +178,7 @@ export const QUERY_COMPLETED = "metabase/qb/QUERY_COMPLETED";
 export const queryCompleted = (
   question,
   queryResults,
-  { prevQueryResults, settingsSyncOptions } = {},
+  { prevQueryResults } = {},
 ) => {
   return async (dispatch, getState) => {
     const [{ data }] = queryResults;
@@ -193,7 +191,6 @@ export const queryCompleted = (
       question = question.syncColumnsAndSettings(
         queryResults[0],
         prevQueryResults?.[0],
-        settingsSyncOptions,
       );
 
       question = question.maybeResetDisplay(

frontend/src/metabase/querying/utils/drills/column-extract-drill/column-extract-drill.tsx

+import { useDispatch } from "metabase/lib/redux";
+import { setUIControls } from "metabase/query_builder/actions";
 import { trackColumnExtractViaHeader } from "metabase/querying/analytics";
 import { ClickActionsView } from "metabase/visualizations/components/ClickActions";
 import type {
@@ -13,10 +15,10 @@ export const columnExtractDrill: Drill<Lib.ColumnExtractDrillThruInfo> = ({
   question,
   drill,
   drillInfo,
-  clicked,
   applyDrill,
 }) => {
   const DrillPopover = ({ onClose, onClick }: ClickActionPopoverProps) => {
+    const dispatch = useDispatch();
     const extractions = Lib.extractionsForDrill(drill);
 
     const actions: RegularClickAction[] = drillInfo.extractions.map(
@@ -29,7 +31,6 @@ export const columnExtractDrill: Drill<Lib.ColumnExtractDrillThruInfo> = ({
         question: () => applyDrill(drill, extraction.tag),
         extra: () => ({
           extraction: extractions[index],
-          settingsSyncOptions: { column: clicked.column },
         }),
       }),
     );
@@ -40,6 +41,7 @@ export const columnExtractDrill: Drill<Lib.ColumnExtractDrillThruInfo> = ({
       };
 
       trackColumnExtractViaHeader(query, stageIndex, extraction, question);
+      dispatch(setUIControls({ scrollToLastColumn: true }));
       onClick(action);
     }
 

frontend/src/metabase/querying/utils/drills/combine-columns-drill/combine-columns-drill.tsx

 import { t } from "ttag";
 
+import { useDispatch } from "metabase/lib/redux";
+import { setUIControls } from "metabase/query_builder/actions";
 import { trackColumnCombineViaColumnHeader } from "metabase/querying/analytics";
 import type {
   ClickActionPopoverProps,
@@ -24,21 +26,25 @@ export const combineColumnsDrill: Drill<Lib.CombineColumnsDrillThruInfo> = ({
   const DrillPopover = ({
     onChangeCardAndRun,
     onClose,
-  }: ClickActionPopoverProps) => (
-    <CombineColumnsDrill
-      column={column}
-      query={query}
-      stageIndex={stageIndex}
-      onSubmit={newQuery => {
-        const nextQuestion = question.setQuery(newQuery);
-        const nextCard = nextQuestion.card();
+  }: ClickActionPopoverProps) => {
+    const dispatch = useDispatch();
+    return (
+      <CombineColumnsDrill
+        column={column}
+        query={query}
+        stageIndex={stageIndex}
+        onSubmit={newQuery => {
+          const nextQuestion = question.setQuery(newQuery);
+          const nextCard = nextQuestion.card();
 
-        trackColumnCombineViaColumnHeader(newQuery, nextQuestion);
-        onChangeCardAndRun({ nextCard });
-        onClose();
-      }}
-    />
-  );
+          trackColumnCombineViaColumnHeader(newQuery, nextQuestion);
+          dispatch(setUIControls({ scrollToLastColumn: true }));
+          onChangeCardAndRun({ nextCard });
+          onClose();
+        }}
+      />
+    );
+  };
 
   return [
     {

frontend/src/metabase/visualizations/click-actions/actions/CombineColumnsAction/CombineColumnsAction.tsx

 import { t } from "ttag";
 
+import { useDispatch } from "metabase/lib/redux";
+import { setUIControls } from "metabase/query_builder/actions";
 import { trackColumnCombineViaPlusModal } from "metabase/query_builder/analytics";
 import { CombineColumns } from "metabase/query_builder/components/expressions/CombineColumns";
 import type { LegacyDrill } from "metabase/visualizations/types";
@@ -25,6 +27,7 @@ export const CombineColumnsAction: LegacyDrill = ({ question, clicked }) => {
   }: ClickActionPopoverProps) => {
     const query = question.query();
     const stageIndex = -1;
+    const dispatch = useDispatch();
 
     function handleSubmit(name: string, clause: Lib.ExpressionClause) {
       const newQuery = Lib.expression(query, stageIndex, name, clause);
@@ -33,6 +36,7 @@ export const CombineColumnsAction: LegacyDrill = ({ question, clicked }) => {
 
       trackColumnCombineViaPlusModal(newQuery, nextQuestion);
 
+      dispatch(setUIControls({ scrollToLastColumn: true }));
       onChangeCardAndRun({ nextCard });
       onClose();
     }

frontend/src/metabase/visualizations/click-actions/actions/ExtractColumn/ExtractColumn.tsx

 import { t } from "ttag";
 
+import { useDispatch } from "metabase/lib/redux";
+import { setUIControls } from "metabase/query_builder/actions";
 import { trackColumnExtractViaPlusModal } from "metabase/query_builder/analytics";
 import { ExtractColumn } from "metabase/query_builder/components/expressions/ExtractColumn";
 import type { LegacyDrill } from "metabase/visualizations/types";
@@ -25,6 +27,7 @@ export const ExtractColumnAction: LegacyDrill = ({ question, clicked }) => {
   }: ClickActionPopoverProps) => {
     const query = question.query();
     const stageIndex = -1;
+    const dispatch = useDispatch();
 
     function handleSubmit(
       _clause: Lib.Clause,
@@ -43,6 +46,7 @@ export const ExtractColumnAction: LegacyDrill = ({ question, clicked }) => {
         nextQuestion,
       );
 
+      dispatch(setUIControls({ scrollToLastColumn: true }));
       onChangeCardAndRun({ nextCard });
       onClose();
     }

frontend/src/metabase/visualizations/components/TableInteractive/TableInteractive.jsx

@@ -19,10 +19,11 @@ import CS from "metabase/css/core/index.css";
 import { withMantineTheme } from "metabase/hoc/MantineTheme";
 import { getScrollBarSize } from "metabase/lib/dom";
 import { formatValue } from "metabase/lib/formatting";
-import { zoomInRow } from "metabase/query_builder/actions";
+import { setUIControls, zoomInRow } from "metabase/query_builder/actions";
 import {
   getRowIndexToPKMap,
   getQueryBuilderMode,
+  getUiControls,
 } from "metabase/query_builder/selectors";
 import { getIsEmbeddingSdk } from "metabase/selectors/embed";
 import { EmotionCacheProvider } from "metabase/styled-components/components/EmotionCacheProvider";
@@ -84,6 +85,7 @@ const mapStateToProps = state => ({
   queryBuilderMode: getQueryBuilderMode(state),
   rowIndexToPkMap: getRowIndexToPKMap(state),
   isEmbeddingSdk: getIsEmbeddingSdk(state),
+  scrollToLastColumn: getUiControls(state).scrollToLastColumn,
 });
 
 const mapDispatchToProps = dispatch => ({
@@ -277,6 +279,19 @@ class TableInteractive extends Component {
         this._totalContentWidth = total;
       }
     }
+
+    // Reset the scrollToLastColumn ui control for subsequent renders.
+    //
+    // We need to include width and height here to avoid unsetting the ui control
+    // before the component content has a chance to render (see the guard clause in
+    // the render method).
+    if (
+      this.props.scrollToLastColumn &&
+      this.props.width &&
+      this.props.height
+    ) {
+      this.props.dispatch(setUIControls({ scrollToLastColumn: false }));
+    }
   }
 
   remeasureColumnWidths() {
@@ -1041,6 +1056,7 @@ class TableInteractive extends Component {
       data: { cols, rows },
       className,
       scrollToColumn,
+      scrollToLastColumn,
       theme,
       question,
     } = this.props;
@@ -1066,7 +1082,10 @@ class TableInteractive extends Component {
             // (https://github.com/bvaughn/react-virtualized/blob/master/docs/Grid.md#prop-types)
             // For some reason, for TableInteractive's main grid scrollLeft appears to be more prior
             const mainGridProps = {};
-            if (scrollToColumn >= 0) {
+
+            if (scrollToLastColumn) {
+              mainGridProps.scrollToColumn = cols.length + 2;
+            } else if (scrollToColumn >= 0) {
               mainGridProps.scrollToColumn = scrollToColumn;
             } else {
               mainGridProps.scrollLeft = scrollLeft;

frontend/src/metabase/visualizations/components/Visualization/Visualization.jsx

@@ -300,7 +300,7 @@ class Visualization extends PureComponent {
   };
 
   // Add the underlying card of current series to onChangeCardAndRun if available
-  handleOnChangeCardAndRun = ({ nextCard, objectId, settingsSyncOptions }) => {
+  handleOnChangeCardAndRun = ({ nextCard, objectId }) => {
     const { rawSeries } = this.props;
 
     const previousCard =
@@ -311,7 +311,6 @@ class Visualization extends PureComponent {
       nextCard,
       previousCard,
       objectId,
-      settingsSyncOptions,
     });
   };