When embedded, force showing side nav when top nav is hidden (#32607)

* when embedded, force showing side nav when top nav is hidden

* don't force open navbar on visible paths

* update e2e tests

* fix types

e2e/test/scenarios/embedding/embedding-full-app.cy.spec.js

@@ -165,10 +165,12 @@ describeEE("scenarios > embedding > full app", () => {
         qs: { additional_info: false },
       });
 
-      // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-      cy.findByText("Our analytics").should("be.visible");
-      // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-      cy.findByText(/Edited/).should("not.exist");
+      cy.findByTestId("app-bar")
+        .findByText("Our analytics")
+        .should("be.visible");
+      cy.findByTestId("qb-header")
+        .findByText(/Edited/)
+        .should("not.exist");
     });
 
     it("should hide the question's action buttons by a param", () => {
@@ -282,12 +284,15 @@ describeEE("scenarios > embedding > full app", () => {
         qs: { additional_info: false },
       });
 
-      // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-      cy.findByText("Orders in a dashboard").should("be.visible");
-      // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-      cy.findByText(/Edited/).should("not.exist");
-      // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-      cy.findByText("Our analytics").should("be.visible");
+      cy.findByTestId("dashboard-header")
+        .findByText("Orders in a dashboard")
+        .should("be.visible");
+      cy.findByTestId("dashboard-header")
+        .findByText(/Edited/)
+        .should("not.exist");
+      cy.findByTestId("app-bar")
+        .findByText("Our analytics")
+        .should("be.visible");
     });
 
     it("should preserve embedding options with click behavior (metabase#24756)", () => {

frontend/src/metabase/nav/containers/Navbar.unit.spec.tsx

 import { Route } from "react-router";
 import fetchMock from "fetch-mock";
+import * as dom from "metabase/lib/dom";
 
 import {
   renderWithProviders,
@@ -15,6 +16,7 @@ import type { User } from "metabase-types/api";
 import { createMockDatabase, createMockUser } from "metabase-types/api/mocks";
 import {
   createMockAppState,
+  createMockEmbedState,
   createMockState,
 } from "metabase-types/store/mocks";
 
@@ -25,6 +27,7 @@ type SetupOpts = {
   route?: string;
   pathname?: string;
   user?: User | null;
+  embedOptions?: Record<string, string | boolean>;
 };
 
 async function setup({
@@ -32,18 +35,18 @@ async function setup({
   pathname = "/",
   route = pathname,
   user = createMockUser(),
+  embedOptions = {},
 }: SetupOpts = {}) {
   const hasContentToFetch = !!user;
   const isAdminApp = pathname.startsWith("/admin");
 
-  if (hasContentToFetch) {
-    setupCollectionsEndpoints({ collections: [] });
-    setupDatabasesEndpoints([createMockDatabase()]);
-    fetchMock.get("path:/api/bookmark", []);
-  }
+  setupCollectionsEndpoints({ collections: [] });
+  setupDatabasesEndpoints([createMockDatabase()]);
+  fetchMock.get("path:/api/bookmark", []);
 
   const storeInitialState = createMockState({
     app: createMockAppState({ isNavbarOpen: isOpen }),
+    embed: createMockEmbedState(embedOptions),
     currentUser: user,
   });
 
@@ -84,4 +87,88 @@ describe("nav > containers > Navbar > Core App", () => {
     await setup({ pathname: "/admin/" });
     expect(screen.queryByTestId("main-navbar-root")).not.toBeInTheDocument();
   });
+
+  describe("embedded", () => {
+    let isWithinIframeSpy: jest.SpyInstance;
+
+    beforeAll(() => {
+      isWithinIframeSpy = jest.spyOn(dom, "isWithinIframe");
+      isWithinIframeSpy.mockReturnValue(true);
+    });
+
+    afterAll(() => {
+      isWithinIframeSpy.mockRestore();
+    });
+
+    const normallyHiddenRoutes = ["/model/1", "/dashboard/1", "/question/1"];
+    const normallyVisibleRoutes = ["/"];
+    const allRoutes = [...normallyHiddenRoutes, ...normallyVisibleRoutes];
+
+    allRoutes.forEach(route => {
+      it(`should be visible when embedded and on ${route} top_nav=false&side_nav=true`, async () => {
+        await setup({
+          pathname: route,
+          isOpen: false, // this should be ignored and overridden by the embedding params
+          embedOptions: { top_nav: false, side_nav: true },
+        });
+
+        const navbar = screen.getByTestId("main-navbar-root");
+        expect(navbar).toHaveAttribute("aria-hidden", "false");
+      });
+    });
+
+    normallyVisibleRoutes.forEach(route => {
+      it(`should be visible when embedded and on ${route} top_nav=true&side_nav=true`, async () => {
+        await setup({
+          pathname: route,
+          isOpen: true,
+          embedOptions: { top_nav: true, side_nav: true },
+        });
+
+        const navbar = screen.getByTestId("main-navbar-root");
+        expect(navbar).toHaveAttribute("aria-hidden", "false");
+      });
+    });
+
+    normallyHiddenRoutes.forEach(route => {
+      it(`should not be visible when embedded and on ${route} top_nav=true&side_nav=true`, async () => {
+        await setup({
+          pathname: route,
+          isOpen: false,
+          embedOptions: { top_nav: true, side_nav: true },
+        });
+
+        const navbar = screen.getByTestId("main-navbar-root");
+        expect(navbar).toHaveAttribute("aria-hidden", "true");
+      });
+    });
+
+    normallyHiddenRoutes.forEach(route => {
+      it(`should not be visible when embedded and on ${route} top_nav=true&side_nav=true`, async () => {
+        await setup({
+          pathname: route,
+          isOpen: false,
+          embedOptions: { top_nav: true, side_nav: true },
+        });
+
+        const navbar = screen.getByTestId("main-navbar-root");
+        expect(navbar).toHaveAttribute("aria-hidden", "true");
+      });
+    });
+
+    // the current state of App.tsx is such that this should never even happen because we don't even render the parent component
+    // but this test will cover any future changes in the component tree
+    allRoutes.forEach(route => {
+      it(`should not be visible when embedded and on ${route} with side_nav=false`, async () => {
+        await setup({
+          pathname: route,
+          isOpen: false,
+          embedOptions: { side_nav: false },
+        });
+
+        const navbar = screen.getByTestId("main-navbar-root");
+        expect(navbar).toHaveAttribute("aria-hidden", "true");
+      });
+    });
+  });
 });

frontend/src/metabase/redux/app.js → frontend/src/metabase/redux/app.ts

 import { push, LOCATION_CHANGE } from "react-router-redux";
+import { createSelector } from "@reduxjs/toolkit";
+import type { Selector } from "@reduxjs/toolkit";
 
 import {
   combineReducers,
@@ -11,8 +13,12 @@ import {
   shouldOpenInBlankWindow,
 } from "metabase/lib/dom";
 
+import { getEmbedOptions, getIsEmbedded } from "metabase/selectors/embed";
+import { getIsAppBarVisible } from "metabase/selectors/app";
+import type { State, Dispatch } from "metabase-types/store";
+
 export const SET_ERROR_PAGE = "metabase/app/SET_ERROR_PAGE";
-export function setErrorPage(error) {
+export function setErrorPage(error: any) {
   console.error("Error:", error);
   return {
     type: SET_ERROR_PAGE,
@@ -20,13 +26,21 @@ export function setErrorPage(error) {
   };
 }
 
-export const openUrl = (url, options) => dispatch => {
-  if (shouldOpenInBlankWindow(url, options)) {
-    openInBlankWindow(url);
-  } else {
-    dispatch(push(url));
-  }
-};
+interface IOpenUrlOptions {
+  blank?: boolean;
+  event?: Event;
+  blankOnMetaOrCtrlKey?: boolean;
+  blankOnDifferentOrigin?: boolean;
+}
+
+export const openUrl =
+  (url: string, options: IOpenUrlOptions) => (dispatch: Dispatch) => {
+    if (shouldOpenInBlankWindow(url, options)) {
+      openInBlankWindow(url);
+    } else {
+      dispatch(push(url));
+    }
+  };
 
 const errorPage = handleActions(
   {
@@ -60,9 +74,23 @@ export const openNavbar = createAction(OPEN_NAVBAR);
 export const closeNavbar = createAction(CLOSE_NAVBAR);
 export const toggleNavbar = createAction(TOGGLE_NAVBAR);
 
-export function getIsNavbarOpen(state) {
-  return state.app.isNavbarOpen;
-}
+export const getIsNavbarOpen: Selector<State> = createSelector(
+  [
+    getIsEmbedded,
+    getEmbedOptions,
+    getIsAppBarVisible,
+    state => state.app.isNavbarOpen,
+  ],
+  (isEmbedded, embedOptions, isAppBarVisible, isNavbarOpen) => {
+    // in an embedded instance, when the app bar is hidden, but the nav bar is not
+    // we need to force the sidebar to be open or else it will be totally inaccessible
+    if (isEmbedded && embedOptions.side_nav === true && !isAppBarVisible) {
+      return true;
+    }
+
+    return isNavbarOpen;
+  },
+);
 
 const isNavbarOpen = handleActions(
   {
@@ -73,6 +101,7 @@ const isNavbarOpen = handleActions(
   checkIsSidebarInitiallyOpen(),
 );
 
+// eslint-disable-next-line import/no-default-export -- deprecated usage
 export default combineReducers({
   errorPage,
   isNavbarOpen,

frontend/src/metabase/selectors/app.ts

@@ -17,7 +17,6 @@ export interface RouterProps {
   location: Location;
 }
 
-const HOMEPAGE_PATH = /^\/$/;
 const PATHS_WITHOUT_NAVBAR = [
   /^\/auth/,
   /\/model\/.*\/query/,
@@ -25,11 +24,7 @@ const PATHS_WITHOUT_NAVBAR = [
   /\/model\/query/,
   /\/model\/metadata/,
 ];
-const EMBEDDED_PATHS_WITH_NAVBAR = [
-  HOMEPAGE_PATH,
-  /^\/collection\/.*/,
-  /^\/archive/,
-];
+
 const PATHS_WITH_COLLECTION_BREADCRUMBS = [
   /\/question\//,
   /\/model\//,
@@ -38,11 +33,11 @@ const PATHS_WITH_COLLECTION_BREADCRUMBS = [
 const PATHS_WITH_QUESTION_LINEAGE = [/\/question/, /\/model/];
 
 export const getRouterPath = (state: State, props: RouterProps) => {
-  return props.location.pathname;
+  return props?.location?.pathname ?? window.location.pathname;
 };
 
 export const getRouterHash = (state: State, props: RouterProps) => {
-  return props.location.hash;
+  return props?.location?.hash ?? window.location.hash;
 };
 
 export const getIsAdminApp = createSelector([getRouterPath], path => {
@@ -85,9 +80,7 @@ export const getIsNavBarEnabled = createSelector(
     if (isEmbedded && !embedOptions.side_nav) {
       return false;
     }
-    if (isEmbedded && embedOptions.side_nav === "default") {
-      return EMBEDDED_PATHS_WITH_NAVBAR.some(pattern => pattern.test(path));
-    }
+
     return !PATHS_WITHOUT_NAVBAR.some(pattern => pattern.test(path));
   },
 );