Keeps preview across different tabs

e2e/test/scenarios/embedding/embedding-dashboard.cy.spec.js

@@ -11,6 +11,9 @@ import {
   downloadAndAssert,
   assertSheetRowsCount,
   modal,
+  getIframeBody,
+  describeEE,
+  setTokenFeatures,
   dashboardParametersContainer,
   goToTab,
 } from "e2e/support/helpers";
@@ -23,7 +26,7 @@ import {
   mapParameters,
 } from "./shared/embedding-dashboard";
 
-const { ORDERS, PEOPLE, PRODUCTS } = SAMPLE_DATABASE;
+const { ORDERS, PEOPLE, PRODUCTS, ORDERS_ID } = SAMPLE_DATABASE;
 
 describe("scenarios > embedding > dashboard parameters", () => {
   beforeEach(() => {
@@ -471,6 +474,112 @@ describe("scenarios > embedding > dashboard parameters with defaults", () => {
   });
 });
 
+describeEE("scenarios > embedding > dashboard appearance", () => {
+  beforeEach(() => {
+    restore();
+    cy.signInAsAdmin();
+    setTokenFeatures("all");
+  });
+
+  it("should not rerender the static embed preview unnecessarily (metabase#38271)", () => {
+    const textFilter = {
+      id: "3",
+      name: "Text filter",
+      slug: "filter-text",
+      type: "string/contains",
+    };
+
+    const dashboardDetails = {
+      name: "dashboard name",
+      enable_embedding: true,
+      embedding_params: {
+        /**
+         * Make sure the parameter is shown in embed preview, because it previously
+         * caused the iframe to rerender even when only the hash part of the embed
+         * preview URL is changed.
+         *
+         * @see useSyncedQueryString in frontend/src/metabase/hooks/use-synced-query-string.ts
+         */
+        [textFilter.slug]: "enabled",
+      },
+      parameters: [textFilter],
+    };
+
+    const questionDetails = {
+      name: "Orders",
+      query: {
+        "source-table": ORDERS_ID,
+      },
+    };
+    cy.createQuestionAndDashboard({
+      questionDetails,
+      dashboardDetails,
+    }).then(({ body: { dashboard_id } }) => {
+      visitDashboard(dashboard_id);
+    });
+
+    cy.intercept(
+      "GET",
+      "api/preview_embed/dashboard/*",
+      cy.spy().as("previewEmbedSpy"),
+    ).as("previewEmbed");
+
+    openStaticEmbeddingModal({
+      activeTab: "parameters",
+      previewMode: "preview",
+      // EE users don't have to accept terms
+      acceptTerms: false,
+    });
+
+    cy.wait("@previewEmbed");
+
+    modal().within(() => {
+      cy.findByRole("tab", { name: "Appearance" }).click();
+      cy.get("@previewEmbedSpy").should("have.callCount", 1);
+
+      cy.log("Assert dashboard theme");
+      getIframeBody()
+        .findByTestId("embed-frame")
+        .should("not.have.class", "Theme--transparent");
+      // We're getting an input element which is 0x0 in size
+      cy.findByLabelText("Transparent").click({ force: true });
+      getIframeBody()
+        .findByTestId("embed-frame")
+        .should("have.class", "Theme--transparent");
+      cy.get("@previewEmbedSpy").should("have.callCount", 1);
+
+      cy.log("Assert dashboard title");
+      getIframeBody().findByText(dashboardDetails.name).should("exist");
+      // We're getting an input element which is 0x0 in size
+      cy.findByLabelText("Dashboard title").click({ force: true });
+      getIframeBody().findByText(dashboardDetails.name).should("not.exist");
+      cy.get("@previewEmbedSpy").should("have.callCount", 1);
+
+      cy.log("Assert dashboard border");
+      getIframeBody()
+        .findByTestId("embed-frame")
+        .should("have.css", "border-top-width", "1px");
+      // We're getting an input element which is 0x0 in size
+      cy.findByLabelText("Border").click({ force: true });
+      getIframeBody()
+        .findByTestId("embed-frame")
+        .should("have.css", "border-top-width", "0px");
+      cy.get("@previewEmbedSpy").should("have.callCount", 1);
+
+      cy.log("Assert font");
+      getIframeBody().should("have.css", "font-family", "Lato, sans-serif");
+      cy.findByLabelText("Font").click();
+    });
+
+    // Since the select popover is rendered outside of the modal, we need to exit the modal context first.
+    popover().findByText("Oswald").click();
+    modal().within(() => {
+      getIframeBody().should("have.css", "font-family", "Oswald, sans-serif");
+      cy.get("@previewEmbedSpy").should("have.callCount", 1);
+    });
+  });
+});
+
 function openFilterOptions(name) {
   filterWidget().contains(name).click();
 }

e2e/test/scenarios/embedding/embedding-questions.cy.spec.js

@@ -16,7 +16,7 @@ import {
 
 const { ORDERS, PRODUCTS } = SAMPLE_DATABASE;
 
-describe("scenarios > embedding > questions ", () => {
+describe("scenarios > embedding > questions", () => {
   beforeEach(() => {
     restore();
     cy.signInAsAdmin();

e2e/test/scenarios/embedding/reproductions/37914-embed-preview-parameter-values-not-working.cy.spec.js

@@ -5,6 +5,7 @@ import {
   addOrUpdateDashboardCard,
   dashboardHeader,
   modal,
+  getIframeBody,
 } from "e2e/support/helpers";
 import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
 
@@ -52,7 +53,9 @@ const dashboardDetails = {
 
 describe("issue 37914", () => {
   beforeEach(() => {
-    cy.intercept("GET", "/api/embed/dashboard/**").as("getEmbed");
+    cy.intercept("GET", "/api/preview_embed/dashboard/**").as(
+      "previewDashboard",
+    );
 
     restore();
     cy.signInAsAdmin();
@@ -115,7 +118,7 @@ describe("issue 37914", () => {
       cy.findByText("Preview").click();
 
       // Makes it less likely to flake.
-      cy.wait("@getEmbed");
+      cy.wait("@previewDashboard");
 
       getIframeBody().within(() => {
         cy.log(
@@ -142,32 +145,3 @@ describe("issue 37914", () => {
     });
   });
 });
-
-// Code grabbed from https://www.cypress.io/blog/2020/02/12/working-with-iframes-in-cypress
-const getIframeDocument = () => {
-  return (
-    cy
-      .get("iframe")
-      // Cypress yields jQuery element, which has the real
-      // DOM element under property "0".
-      // From the real DOM iframe element we can get
-      // the "document" element, it is stored in "contentDocument" property
-      // Cypress "its" command can access deep properties using dot notation
-      // https://on.cypress.io/its
-      .its("0.contentDocument")
-      .should("exist")
-  );
-};
-
-const getIframeBody = () => {
-  // get the document
-  return (
-    getIframeDocument()
-      // automatically retries until body is loaded
-      .its("body")
-      .should("not.be.undefined")
-      // wraps "body" DOM element to allow
-      // chaining more Cypress commands, like ".find(...)"
-      .then(cy.wrap)
-  );
-};

e2e/test/scenarios/sharing/reproductions/26988-embedding-dynamic-settings.cy.spec.js

@@ -12,7 +12,9 @@ import { ORDERS_ID } from "metabase-types/api/mocks/presets";
 describeEE("issue 26988", () => {
   beforeEach(() => {
     restore();
-    cy.intercept("GET", "/api/embed/dashboard/*").as("dashboard");
+    cy.intercept("GET", "/api/preview_embed/dashboard/*").as(
+      "previewDashboard",
+    );
 
     cy.signInAsAdmin();
     setTokenFeatures("all");
@@ -40,7 +42,7 @@ describeEE("issue 26988", () => {
       acceptTerms: false,
     });
 
-    cy.wait("@dashboard");
+    cy.wait("@previewDashboard");
     getIframeBody().should("have.css", "font-family", `Lato, sans-serif`);
 
     cy.findByLabelText("Playing with appearance options")
@@ -49,13 +51,11 @@ describeEE("issue 26988", () => {
       .click();
     popover().findByText("Oswald").click();
 
-    cy.wait("@dashboard");
     getIframeBody().should("have.css", "font-family", `Oswald, sans-serif`);
 
     cy.get("@font-control").click();
     popover().findByText("Slabo 27px").click();
 
-    cy.wait("@dashboard");
     getIframeBody().should(
       "have.css",
       "font-family",

frontend/src/metabase/dashboard/hoc/DashboardControls.jsx

@@ -42,8 +42,12 @@ export const DashboardControls = ComposedComponent =>
         this.loadDashboardParams();
       }
 
-      componentDidUpdate() {
-        this.updateDashboardParams();
+      componentDidUpdate(prevProps) {
+        if (prevProps.location !== this.props.location) {
+          this.syncUrlHashToState();
+        } else {
+          this.syncStateToUrlHash();
+        }
         this._showNav(!this.state.isFullscreen);
       }
 
@@ -72,7 +76,16 @@ export const DashboardControls = ComposedComponent =>
         this.setHideParameters(options.hide_parameters);
       };
 
-      updateDashboardParams = () => {
+      syncUrlHashToState() {
+        const { location } = this.props;
+
+        const { refresh, fullscreen, theme } = parseHashOptions(location.hash);
+        this.setRefreshPeriod(refresh);
+        this.setFullscreen(fullscreen);
+        this.setTheme(theme);
+      }
+
+      syncStateToUrlHash = () => {
         const { location, replace } = this.props;
 
         const options = parseHashOptions(location.hash);
@@ -224,7 +237,6 @@ export const DashboardControls = ComposedComponent =>
             hasNightModeToggle={this.state.theme !== "transparent"}
             setRefreshElapsedHook={this.setRefreshElapsedHook}
             loadDashboardParams={this.loadDashboardParams}
-            updateDashboardParams={this.updateDashboardParams}
             onNightModeChange={this.setNightMode}
             onFullscreenChange={this.setFullscreen}
             onRefreshPeriodChange={this.setRefreshPeriod}

frontend/src/metabase/hooks/use-synced-query-string.ts

@@ -1,11 +1,21 @@
 import { useEffect } from "react";
 import querystring from "querystring";
+import { IS_EMBED_PREVIEW } from "metabase/lib/embed";
 
 export function useSyncedQueryString(
   fn: () => Record<string, any>,
   deps?: any[],
 ) {
   useEffect(() => {
+    /**
+     * We don't want to sync the query string to the URL because when previewing,
+     * this changes the URL of the iframe by appending the query string to the src.
+     * This causes the iframe to reload when changing the preview hash from appearance
+     * settings because now the base URL (including the query string) is different.
+     */
+    if (IS_EMBED_PREVIEW) {
+      return;
+    }
     const object = fn();
     const searchString = buildSearchString(object);
 

frontend/src/metabase/lib/dom.js

@@ -26,7 +26,7 @@ window.METABASE = true;
 // used for detecting if we're previewing an embed
 export const IFRAMED_IN_SELF = (function () {
   try {
-    return window.self !== window.top && window.top.METABASE;
+    return window.self !== window.parent && window.parent.METABASE;
   } catch (e) {
     return false;
   }

frontend/src/metabase/public/components/EmbedFrame/EmbedFrame.tsx

@@ -1,5 +1,6 @@
 import type { ReactNode } from "react";
-import { useState } from "react";
+import { useEffect, useState } from "react";
+
 import { withRouter } from "react-router";
 import { connect } from "react-redux";
 import cx from "classnames";
@@ -24,6 +25,8 @@ import type {
 } from "metabase-types/api";
 import type { State } from "metabase-types/store";
 
+import { useDispatch } from "metabase/lib/redux";
+import { setOptions } from "metabase/redux/embed";
 import type Question from "metabase-lib/Question";
 import { getValuePopulatedParameters } from "metabase-lib/parameters/utils/parameter-values";
 
@@ -104,6 +107,11 @@ function EmbedFrame({
     initializeIframeResizer(() => setInnerScroll(false));
   });
 
+  const dispatch = useDispatch();
+  useEffect(() => {
+    dispatch(setOptions(location));
+  }, [dispatch, location]);
+
   const {
     bordered = isWithinIframe(),
     titled = true,