fix: Don't show error page when command palette documentation link is clicked (#47862)

frontend/src/metabase/core/components/Link/Link.styled.tsx

-import isPropValid from "@emotion/is-prop-valid";
 import { css } from "@emotion/react";
 import styled from "@emotion/styled";
-import { Link } from "react-router";
+import { Link, type LinkProps } from "react-router";
 
+import { doNotForwardProps } from "metabase/common/utils/doNotForwardProps";
 import { focusOutlineStyle } from "metabase/core/style/input";
 
-import type { LinkProps } from "./types";
+type LinkVariantProp = { variant?: "default" | "brand" | "brandBold" };
 
-const isLinkPropValid = (propName: string) => {
-  return isPropValid(propName) || propName === "activeClassName";
-};
-
-export const LinkRoot = styled(Link, {
-  shouldForwardProp: isLinkPropValid,
-})<LinkProps>`
+export const LinkRoot = styled(
+  Link,
+  doNotForwardProps("variant"),
+)<LinkVariantProp>`
   opacity: ${props => (props.disabled ? "0.4" : "")};
   pointer-events: ${props => (props.disabled ? "none" : "")};
   transition: opacity 0.3s linear;
@@ -21,7 +18,7 @@ export const LinkRoot = styled(Link, {
   ${focusOutlineStyle("brand")};
 
   ${props => variants[props.variant ?? "default"] ?? ""}
-`;
+` as unknown as React.FC<LinkProps & LinkVariantProp>;
 
 export const variants = {
   default: "",

frontend/src/metabase/core/components/Link/types.ts

-import type { AnchorHTMLAttributes, CSSProperties, ReactNode } from "react";
+import type { LinkProps as RouterLinkProps } from "react-router";
 
 import type { TooltipProps } from "metabase/core/components/Tooltip/Tooltip";
 
-export interface LinkProps extends AnchorHTMLAttributes<HTMLAnchorElement> {
-  to: string;
+export interface LinkProps extends RouterLinkProps {
   variant?: "default" | "brand" | "brandBold";
-  disabled?: boolean;
-  className?: string;
-  children?: ReactNode;
   tooltip?: string | TooltipProps;
-  activeClassName?: string;
-  activeStyle?: CSSProperties;
-  onlyActiveOnIndex?: boolean;
 }

frontend/src/metabase/core/components/TabLink/TabLink.tsx

@@ -7,7 +7,7 @@ import { TabContext, getTabId, getTabPanelId } from "../Tab";
 
 import { TabLabel, TabLinkRoot } from "./TabLink.styled";
 
-export interface TabLinkProps<T> extends LinkProps {
+export interface TabLinkProps<T> extends Omit<LinkProps, "value"> {
   value?: T;
 }
 

frontend/src/metabase/palette/components/PaletteResultItem.tsx

-import { Link } from "react-router";
 import { t } from "ttag";
 
-import { color } from "metabase/lib/colors";
+import ExternalLink from "metabase/core/components/ExternalLink";
+import Link from "metabase/core/components/Link";
 import { Box, Flex, Icon, Text } from "metabase/ui";
 
 import type { PaletteActionImpl } from "../types";
-import { getCommandPaletteIcon } from "../utils";
+import {
+  getCommandPaletteIcon,
+  isAbsoluteURL,
+  locationDescriptorToURL,
+} from "../utils";
 
 interface PaletteResultItemProps {
   item: PaletteActionImpl;
@@ -27,13 +31,13 @@ export const PaletteResultItem = ({ item, active }: PaletteResultItemProps) => {
       gap="0.5rem"
       fw={700}
       style={{
-        cursor: item.disabled ? "default" : "cursor",
+        cursor: item.disabled ? "default" : "pointer",
         borderRadius: "0.5rem",
         flexGrow: 1,
         flexBasis: 0,
       }}
-      bg={active ? color("brand") : "none"}
-      c={active ? color("text-white") : color("text-dark")}
+      bg={active ? "var(--mb-color-brand)" : undefined}
+      c={active ? "var(--mb-color-text-white)" : "var(--mb-color-text-dark)"}
       aria-label={item.name}
       aria-disabled={item.disabled ? true : false}
     >
@@ -69,7 +73,9 @@ export const PaletteResultItem = ({ item, active }: PaletteResultItemProps) => {
           {item.extra?.isVerified && (
             <Icon
               name="verified_filled"
-              color={active ? color("text-white") : color("brand")}
+              color={
+                active ? "var(--mb-color-text-white)" : "var(--mb-color-brand)"
+              }
               style={{
                 verticalAlign: "sub",
                 marginLeft: "0.25rem",
@@ -80,7 +86,11 @@ export const PaletteResultItem = ({ item, active }: PaletteResultItemProps) => {
             <Text
               component="span"
               ml="0.25rem"
-              c={active ? color("brand-light") : color("text-light")}
+              c={
+                active
+                  ? "var(--mb-color-brand-light)"
+                  : "var(--mb-color-text-light)"
+              }
               fz="0.75rem"
               lh="1rem"
               fw="normal"
@@ -91,7 +101,9 @@ export const PaletteResultItem = ({ item, active }: PaletteResultItemProps) => {
         </Box>
         <Text
           component="span"
-          color={active ? "text-white" : "text-light"}
+          color={
+            active ? "var(--mb-color-text-white)" : "var(--mb-color-text-light)"
+          }
           fw="normal"
           style={{
             textOverflow: "ellipsis",
@@ -110,13 +122,32 @@ export const PaletteResultItem = ({ item, active }: PaletteResultItemProps) => {
       )}
     </Flex>
   );
-
   if (item.extra?.href) {
-    return (
-      <Box component={Link} to={item.extra.href} w="100%">
-        {content}
-      </Box>
-    );
+    const url = locationDescriptorToURL(item.extra.href);
+    if (isAbsoluteURL(url)) {
+      return (
+        <Box
+          component={
+            // This is needed to make external links work when Metabase is
+            // hosted on a subpath
+            ExternalLink
+          }
+          href={url}
+          target="_blank"
+          role="link"
+          w="100%"
+          lh={1}
+        >
+          {content}
+        </Box>
+      );
+    } else {
+      return (
+        <Box component={Link} to={item.extra.href} role="link" w="100%" lh={1}>
+          {content}
+        </Box>
+      );
+    }
   } else {
     return content;
   }

frontend/src/metabase/palette/components/PaletteResultItem.unit.spec.tsx

-import { render, screen } from "__support__/ui";
-import { color } from "metabase/lib/colors";
+import { Route } from "react-router";
+
+import {
+  fireEvent,
+  render,
+  renderWithProviders,
+  screen,
+  waitFor,
+} from "__support__/ui";
 
 import type { PaletteActionImpl } from "../types";
 
 import { PaletteResultItem } from "./PaletteResultItem";
+import { PaletteResultList } from "./PaletteResultsList";
 
 const mockPaletteActionImpl = (opts: Partial<PaletteActionImpl>) =>
   ({
@@ -32,7 +40,7 @@ describe("PaletteResultItem", () => {
 
     expect(await screen.findByRole("img", { name: /model/ })).toHaveAttribute(
       "color",
-      color("brand"),
+      "var(--mb-color-brand)",
     );
   });
 
@@ -41,7 +49,7 @@ describe("PaletteResultItem", () => {
 
     expect(await screen.findByRole("img", { name: /model/ })).toHaveAttribute(
       "color",
-      color("green"),
+      "green",
     );
   });
 
@@ -53,7 +61,7 @@ describe("PaletteResultItem", () => {
 
     expect(await screen.findByRole("img", { name: /model/ })).toHaveAttribute(
       "color",
-      color("white"),
+      "var(--mb-color-text-white)",
     );
   });
 
@@ -62,3 +70,108 @@ describe("PaletteResultItem", () => {
     expect(screen.queryByRole("img")).not.toBeInTheDocument();
   });
 });
+
+/** For some tests we need to render the PaletteResultsList so that the Enter key works */
+const setupInList = ({ item }: { item: Partial<PaletteActionImpl> }) => {
+  const items = [item];
+  const utils = renderWithProviders(
+    <>
+      <Route
+        path="/"
+        component={() => (
+          <PaletteResultList
+            items={items.map(item => mockPaletteActionImpl(item))}
+            onRender={({
+              item,
+              active,
+            }: {
+              item: string | PaletteActionImpl;
+              active: boolean;
+            }) => {
+              // items whose type is string are not relevant to these tests
+              if (typeof item === "string") {
+                return <></>;
+              }
+              return <PaletteResultItem item={item} active={active} />;
+            }}
+          />
+        )}
+      />
+      <Route path="search" component={() => null} />
+    </>,
+    { withRouter: true, withKBar: true },
+  );
+  const link = screen.getByRole("link");
+  return { ...utils, link };
+};
+
+describe("Mouse/keyboard interactions", () => {
+  const initialLocation = {
+    pathname: "/",
+  };
+  describe("The 'Search documentation for...' command palette item", () => {
+    const searchDocs: Partial<PaletteActionImpl> = {
+      id: "search_docs",
+      name: 'Search documentation for "hedgehogs"',
+      section: "docs",
+      keywords: "hedgehogs",
+      icon: "document",
+      extra: {
+        href: "https://www.metabase.com/search?query=hedgehogs",
+      },
+    };
+
+    it("should NOT navigate via React router on click (metabase#47829)", async () => {
+      const { history, link } = setupInList({ item: searchDocs });
+      fireEvent.click(link);
+      expect(history?.getCurrentLocation()).toMatchObject(initialLocation);
+      expect(link).toHaveAttribute("target", "_blank");
+    });
+  });
+
+  describe("The 'View and filter all N results' command palette item", () => {
+    const searchLocation = {
+      pathname: "search",
+      query: {
+        q: "hedgehogs",
+      },
+    };
+
+    const viewResults: Partial<PaletteActionImpl> = {
+      id: "search-results-metadata",
+      name: "View and filter all 2 results",
+      section: "search",
+      keywords: "hedgehogs",
+      icon: "link",
+      perform: () => {},
+      extra: {
+        href: searchLocation,
+      },
+    };
+
+    it("should navigate via React router when the Enter key is pressed", async () => {
+      const { history, link } = setupInList({ item: viewResults });
+      fireEvent(window, new KeyboardEvent("keydown", { key: "Enter" }));
+      await waitFor(() => {
+        expect(history?.getCurrentLocation()).toMatchObject(searchLocation);
+      });
+      expect(link).not.toHaveAttribute("target", "_blank");
+    });
+
+    it("should navigate via React router on left click", async () => {
+      const { history, link } = setupInList({ item: viewResults });
+      // A normal, left click
+      fireEvent.click(link);
+      expect(history?.getCurrentLocation()).toMatchObject(searchLocation);
+      expect(link).not.toHaveAttribute("target", "_blank");
+    });
+
+    it("should NOT navigate via React router on middle click", async () => {
+      const { history, link } = setupInList({ item: viewResults });
+      // A middle click
+      fireEvent.click(link, { button: 1 });
+      expect(history?.getCurrentLocation()).toMatchObject(initialLocation);
+      expect(link).not.toHaveAttribute("target", "_blank");
+    });
+  });
+});

frontend/src/metabase/palette/components/PaletteResultsList.tsx

@@ -94,7 +94,7 @@ export const PaletteResultList: React.FC<PaletteResultListProps> = props => {
         block: "nearest",
       });
     } else {
-      parentRef.current?.scrollTo({ top: 0, behavior: "smooth" });
+      parentRef.current?.scrollTo?.({ top: 0, behavior: "smooth" });
     }
   }, [activeIndex]);
 
@@ -120,7 +120,7 @@ export const PaletteResultList: React.FC<PaletteResultListProps> = props => {
       if (item.command) {
         item.command.perform(item);
         query.toggle();
-      } else {
+      } else if (!item.extra?.href) {
         query.setSearch("");
         query.setCurrentRootAction(item.id);
       }

frontend/src/metabase/palette/hooks/useCommandPalette.tsx

@@ -100,9 +100,6 @@ export const useCommandPalette = ({
         section: "docs",
         keywords: debouncedSearchText, // Always match the debouncedSearchText string
         icon: "document",
-        perform: () => {
-          window.open(link);
-        },
         extra: {
           href: link,
         },
@@ -129,14 +126,11 @@ export const useCommandPalette = ({
     if (!isSearchTypeaheadEnabled) {
       return [
         {
-          id: `search-disabled`,
+          id: `search-without-typeahead`,
           name: t`View search results for "${debouncedSearchText}"`,
           section: "search",
           keywords: debouncedSearchText,
           icon: "link" as const,
-          perform: () => {
-            dispatch(push(searchLocation));
-          },
           priority: Priority.HIGH,
           extra: {
             href: searchLocation,
@@ -173,7 +167,6 @@ export const useCommandPalette = ({
             icon: "link" as IconName,
             perform: () => {
               trackSearchClick("view_more", 0, "command-palette");
-              dispatch(push(searchLocation));
             },
             priority: Priority.HIGH,
             extra: {

frontend/src/metabase/palette/utils.ts

+import type { LocationDescriptor } from "history";
+import type { MouseEvent } from "react";
 import { t } from "ttag";
 import _ from "underscore";
 
@@ -79,11 +81,13 @@ export const getCommandPaletteIcon = (
 ): { name: IconName; color: string } => {
   const icon = {
     name: item.icon as IconName,
-    color: item.extra?.iconColor ? color(item.extra.iconColor) : color("brand"),
+    color: item.extra?.iconColor
+      ? color(item.extra.iconColor)
+      : "var(--mb-color-brand)",
   };
 
   if (isActive) {
-    icon.color = color("white");
+    icon.color = "var(--mb-color-text-white)";
   }
 
   if (isActive && (item.icon === "folder" || item.icon === "collection")) {
@@ -92,3 +96,25 @@ export const getCommandPaletteIcon = (
 
   return icon;
 };
+
+export const isAbsoluteURL = (url: string) =>
+  url.startsWith("http://") || url.startsWith("https://");
+
+export const locationDescriptorToURL = (
+  locationDescriptor: LocationDescriptor,
+) => {
+  if (typeof locationDescriptor === "string") {
+    return locationDescriptor;
+  } else {
+    const { pathname = "", query = null, hash = null } = locationDescriptor;
+    const queryString = query
+      ? "?" + new URLSearchParams(query).toString()
+      : "";
+    const hashString = hash ? "#" + hash : "";
+
+    return `${pathname}${queryString}${hashString}`;
+  }
+};
+
+export const isNormalClick = (e: MouseEvent): boolean =>
+  !e.ctrlKey && !e.shiftKey && !e.metaKey && !e.altKey && e.button === 0;

frontend/src/metabase/palette/utils.unit.spec.ts

+import type { LocationDescriptor } from "history";
+
 import type { PaletteActionImpl } from "./types";
-import { navigateActionIndex, processResults, processSection } from "./utils";
+import {
+  locationDescriptorToURL,
+  navigateActionIndex,
+  processResults,
+  processSection,
+} from "./utils";
 
 interface mockAction {
   name: string;
@@ -206,4 +213,50 @@ describe("command palette utils", () => {
       expect(navigateActionIndex(items, 3, -4)).toBe(3);
     });
   });
+
+  describe("locationDescriptorToURL", () => {
+    it(`should return the string if locationDescriptor is a string`, () => {
+      expect(locationDescriptorToURL("/a/b/c")).toBe(`/a/b/c`);
+    });
+
+    it("should return the correct URL when a LocationDescriptor is provided", () => {
+      const locationDescriptor: LocationDescriptor = { pathname: "/a/b/c" };
+      expect(locationDescriptorToURL(locationDescriptor)).toBe(`/a/b/c`);
+    });
+
+    it("should return the correct URL when query is provided", () => {
+      const locationDescriptor = {
+        pathname: "/a/b/c",
+        query: { en: "hello", es: "hola" },
+      };
+      expect(locationDescriptorToURL(locationDescriptor)).toBe(
+        `/a/b/c?en=hello&es=hola`,
+      );
+    });
+
+    it("should return the correct URL when pathname and hash are provided", () => {
+      const locationDescriptor = { pathname: "/a/b/c", hash: "top" };
+      expect(locationDescriptorToURL(locationDescriptor)).toBe(`/a/b/c#top`);
+    });
+
+    it("should return the correct URL with pathname, query, and hash", () => {
+      const locationDescriptor = {
+        pathname: "/a/b/c",
+        query: { en: "hello", es: "hola" },
+        hash: "top",
+      };
+      expect(locationDescriptorToURL(locationDescriptor)).toBe(
+        `/a/b/c?en=hello&es=hola#top`,
+      );
+    });
+
+    it("should handle empty values appropriately", () => {
+      const locationDescriptor: LocationDescriptor = {
+        pathname: "/a/b/c",
+        query: undefined,
+        hash: undefined,
+      };
+      expect(locationDescriptorToURL(locationDescriptor)).toBe(`/a/b/c`);
+    });
+  });
 });