Extension sidebar: Fix incorrect rendering of menu buttons (#106145)

* fix(ui): use component render instead of usecallback to render button * add and fix component tests
2025-05-28 16:39:23 +02:00
parent 827419d32e
commit 98d1821948
3 changed files with 161 additions and 81 deletions
@@ -1,9 +1,4 @@
-import { css, cx } from '@emotion/css';
-import { useCallback } from 'react';
-
-import { GrafanaTheme2 } from '@grafana/data';
-import { useTranslate } from '@grafana/i18n';
-import { Dropdown, Menu, ToolbarButton, useTheme2 } from '@grafana/ui';
+import { Dropdown, Menu } from '@grafana/ui';

 import { NavToolbarSeparator } from '../NavToolbar/NavToolbarSeparator';

@@ -12,12 +7,11 @@ import {
  getComponentMetaFromComponentId,
  useExtensionSidebarContext,
 } from './ExtensionSidebarProvider';
+import { ExtensionToolbarItemButton } from './ExtensionToolbarItemButton';

 export function ExtensionToolbarItem() {
-  const styles = getStyles(useTheme2());
  const { availableComponents, dockedComponentId, setDockedComponentId, isOpen, isEnabled } =
    useExtensionSidebarContext();
-  const { t } = useTranslate();

  let dockedComponentTitle = '';
  if (dockedComponentId) {
@@ -40,52 +34,20 @@ export function ExtensionToolbarItem() {
    return null;
  }

-  // conditionally renders a button to open or close the sidebar
-  // not using a component to avoid passing refs with the `Dropdown` component
-  const renderButton = useCallback(
-    (isOpen: boolean, title?: string, onClick?: () => void) => {
-      if (isOpen) {
-        // render button to close the sidebar
-        return (
-          <ToolbarButton
-            className={cx(styles.button, styles.buttonActive)}
-            icon="ai-sparkle"
-            data-testid="extension-toolbar-button-close"
-            variant="default"
-            onClick={() => setDockedComponentId(undefined)}
-            tooltip={t('navigation.extension-sidebar.button-tooltip.close', 'Close {{title}}', { title })}
-          />
-        );
-      }
-      // if a title is provided, use it in the tooltip
-      let tooltip = t('navigation.extension-sidebar.button-tooltip.open-all', 'Open AI assistants and sidebar apps');
-      if (title) {
-        tooltip = t('navigation.extension-sidebar.button-tooltip.open', 'Open {{title}}', { title });
-      }
-      return (
-        <ToolbarButton
-          className={cx(styles.button)}
-          icon="ai-sparkle"
-          data-testid="extension-toolbar-button-open"
-          variant="default"
-          onClick={onClick}
-          tooltip={tooltip}
-        />
-      );
-    },
-    [setDockedComponentId, styles.button, styles.buttonActive, t]
-  );
-
  if (components.length === 1) {
    return (
      <>
-        {renderButton(isOpen, components[0].title, () => {
-          if (isOpen) {
-            setDockedComponentId(undefined);
-          } else {
-            setDockedComponentId(getComponentIdFromComponentMeta(components[0].pluginId, components[0]));
-          }
-        })}
+        <ExtensionToolbarItemButton
+          isOpen={isOpen}
+          title={components[0].title}
+          onClick={() => {
+            if (isOpen) {
+              setDockedComponentId(undefined);
+            } else {
+              setDockedComponentId(getComponentIdFromComponentMeta(components[0].pluginId, components[0]));
+            }
+          }}
+        />
        <NavToolbarSeparator />
      </>
    );
@@ -114,40 +76,22 @@ export function ExtensionToolbarItem() {
  );
  return (
    <>
-      {isOpen &&
-        renderButton(isOpen, dockedComponentTitle, () => {
-          if (isOpen) {
-            setDockedComponentId(undefined);
-          }
-        })}
-      {!isOpen && (
+      {isOpen ? (
+        <ExtensionToolbarItemButton
+          isOpen
+          title={dockedComponentTitle}
+          onClick={() => {
+            if (isOpen) {
+              setDockedComponentId(undefined);
+            }
+          }}
+        />
+      ) : (
        <Dropdown overlay={MenuItems} placement="bottom-end">
-          {renderButton(isOpen)}
+          <ExtensionToolbarItemButton isOpen={false} />
        </Dropdown>
      )}
      <NavToolbarSeparator />
    </>
  );
 }
-
-function getStyles(theme: GrafanaTheme2) {
-  return {
-    button: css({
-      // this is needed because with certain breakpoints the button will get `width: auto`
-      // and the icon will stretch
-      aspectRatio: '1 / 1 !important',
-      width: '28px',
-      height: '28px',
-      padding: 0,
-      justifyContent: 'center',
-      borderRadius: theme.shape.radius.circle,
-      margin: theme.spacing(0, 0.25),
-    }),
-    buttonActive: css({
-      borderRadius: theme.shape.radius.circle,
-      backgroundColor: theme.colors.primary.transparent,
-      border: `1px solid ${theme.colors.primary.borderTransparent}`,
-      color: theme.colors.text.primary,
-    }),
-  };
-}
@@ -0,0 +1,57 @@
+import { render, screen, fireEvent } from '@testing-library/react';
+
+import { useTranslate } from '@grafana/i18n';
+
+import { ExtensionToolbarItemButton } from './ExtensionToolbarItemButton';
+
+// Mock the useTranslate hook
+jest.mock('@grafana/i18n', () => ({
+  useTranslate: jest.fn(),
+}));
+
+describe('ExtensionToolbarItemButton', () => {
+  const mockTranslate = (_: string, fallback: string, values?: Record<string, string>) => {
+    if (values) {
+      return fallback.replace('{{title}}', values.title);
+    }
+    return fallback;
+  };
+
+  beforeEach(() => {
+    (useTranslate as jest.Mock).mockReturnValue({ t: mockTranslate });
+  });
+
+  it('renders open button with default tooltip when no title is provided', () => {
+    render(<ExtensionToolbarItemButton isOpen={false} />);
+
+    const button = screen.getByTestId('extension-toolbar-button-open');
+    expect(button).toBeInTheDocument();
+    expect(button).toHaveAttribute('aria-label', 'Open AI assistants and sidebar apps');
+  });
+
+  it('renders open button with custom tooltip when title is provided', () => {
+    render(<ExtensionToolbarItemButton isOpen={false} title="Test App" />);
+
+    const button = screen.getByTestId('extension-toolbar-button-open');
+    expect(button).toBeInTheDocument();
+    expect(button).toHaveAttribute('aria-label', 'Open Test App');
+  });
+
+  it('renders close button with custom tooltip when isOpen is true', () => {
+    render(<ExtensionToolbarItemButton isOpen={true} title="Test App" />);
+
+    const button = screen.getByTestId('extension-toolbar-button-close');
+    expect(button).toBeInTheDocument();
+    expect(button).toHaveAttribute('aria-label', 'Close Test App');
+  });
+
+  it('calls onClick handler when button is clicked', () => {
+    const handleClick = jest.fn();
+    render(<ExtensionToolbarItemButton isOpen={false} onClick={handleClick} />);
+
+    const button = screen.getByTestId('extension-toolbar-button-open');
+    fireEvent.click(button);
+
+    expect(handleClick).toHaveBeenCalledTimes(1);
+  });
+});
@@ -0,0 +1,79 @@
+import { css, cx } from '@emotion/css';
+import React from 'react';
+
+import { GrafanaTheme2 } from '@grafana/data';
+import { useTranslate } from '@grafana/i18n';
+import { ToolbarButton, useStyles2 } from '@grafana/ui';
+
+interface ToolbarItemButtonProps {
+  isOpen: boolean;
+  title?: string;
+  onClick?: () => void;
+}
+
+function ExtensionToolbarItemButtonComponent(
+  { isOpen, title, onClick }: ToolbarItemButtonProps,
+  ref: React.ForwardedRef<HTMLButtonElement>
+) {
+  const styles = useStyles2(getStyles);
+  const { t } = useTranslate();
+
+  if (isOpen) {
+    // render button to close the sidebar
+    return (
+      <ToolbarButton
+        ref={ref}
+        className={cx(styles.button, styles.buttonActive)}
+        icon="ai-sparkle"
+        data-testid="extension-toolbar-button-close"
+        variant="default"
+        onClick={onClick}
+        tooltip={t('navigation.extension-sidebar.button-tooltip.close', 'Close {{title}}', { title })}
+      />
+    );
+  }
+  // if a title is provided, use it in the tooltip
+  let tooltip = t('navigation.extension-sidebar.button-tooltip.open-all', 'Open AI assistants and sidebar apps');
+  if (title) {
+    tooltip = t('navigation.extension-sidebar.button-tooltip.open', 'Open {{title}}', { title });
+  }
+  return (
+    <ToolbarButton
+      ref={ref}
+      className={cx(styles.button)}
+      icon="ai-sparkle"
+      data-testid="extension-toolbar-button-open"
+      variant="default"
+      onClick={onClick}
+      tooltip={tooltip}
+    />
+  );
+}
+
+// Wrapped the component with React.forwardRef to enable ref forwarding, which is required
+// for proper integration with the Dropdown component from @grafana/ui
+export const ExtensionToolbarItemButton = React.forwardRef<HTMLButtonElement, ToolbarItemButtonProps>(
+  ExtensionToolbarItemButtonComponent
+);
+
+function getStyles(theme: GrafanaTheme2) {
+  return {
+    button: css({
+      // this is needed because with certain breakpoints the button will get `width: auto`
+      // and the icon will stretch
+      aspectRatio: '1 / 1 !important',
+      width: '28px',
+      height: '28px',
+      padding: 0,
+      justifyContent: 'center',
+      borderRadius: theme.shape.radius.circle,
+      margin: theme.spacing(0, 0.25),
+    }),
+    buttonActive: css({
+      borderRadius: theme.shape.radius.circle,
+      backgroundColor: theme.colors.primary.transparent,
+      border: `1px solid ${theme.colors.primary.borderTransparent}`,
+      color: theme.colors.text.primary,
+    }),
+  };
+}