Azure Monitor: Scroll on resource picker fix and general clean up (#48311)

* Fix bug where resource picker doesn't open automatically to a previously selected resource.

public/app/plugins/datasource/grafana-azure-monitor-datasource/components/ResourcePicker/NestedEntry.tsx

@@ -16,6 +16,7 @@ interface NestedEntryProps {
   isSelectable: boolean;
   isOpen: boolean;
   isDisabled: boolean;
+  scrollIntoView?: boolean;
   onToggleCollapse: (row: ResourceRow) => void;
   onSelectedChange: (row: ResourceRow, selected: boolean) => void;
 }
@@ -26,6 +27,7 @@ export const NestedEntry: React.FC<NestedEntryProps> = ({
   isDisabled,
   isOpen,
   isSelectable,
+  scrollIntoView,
   level,
   onToggleCollapse,
   onSelectedChange,
@@ -33,10 +35,6 @@ export const NestedEntry: React.FC<NestedEntryProps> = ({
   const theme = useTheme2();
   const styles = useStyles2(getStyles);
   const hasChildren = !!entry.children;
-  // Subscriptions, resource groups, resources, and variables are all selectable, so
-  // the top-level variable group is the only thing that cannot be selected.
-  // const isSelectable = entry.type !== ResourceRowType.VariableGroup;
-  // const isSelectable = selectableEntryTypes?.some((e) => e === entry.type);
 
   const handleToggleCollapse = useCallback(() => {
     onToggleCollapse(entry);
@@ -50,12 +48,12 @@ export const NestedEntry: React.FC<NestedEntryProps> = ({
     [entry, onSelectedChange]
   );
 
-  const checkboxId = `checkbox_${entry.id}`;
+  const checkboxId = `${scrollIntoView ? 'table' : 'summary'}_checkbox_${entry.uri}`;
 
   // Scroll to the selected element if it's not in the view
   // Only do it once, when the component is mounted
   useEffect(() => {
-    if (isSelected) {
+    if (isSelected && scrollIntoView) {
       document.getElementById(checkboxId)?.scrollIntoView({
         behavior: 'smooth',
         block: 'center',
@@ -65,9 +63,6 @@ export const NestedEntry: React.FC<NestedEntryProps> = ({
 
   return (
     <div className={styles.nestedEntry} style={{ marginLeft: level * (3 * theme.spacing.gridSize) }}>
-      {/* When groups are selectable, I *think* we will want to show a 2-wide space instead
-              of the collapse button for leaf rows that have no children to get them to align */}
-
       {hasChildren ? (
         <IconButton
           className={styles.collapseButton}

public/app/plugins/datasource/grafana-azure-monitor-datasource/components/ResourcePicker/NestedResourceTable.tsx

@@ -1,61 +0,0 @@
-import { cx } from '@emotion/css';
-import React from 'react';
-
-import { useStyles2 } from '@grafana/ui';
-
-import NestedRows from './NestedRows';
-import getStyles from './styles';
-import { ResourceRow, ResourceRowGroup, ResourceRowType } from './types';
-
-interface NestedResourceTableProps {
-  rows: ResourceRowGroup;
-  selectedRows: ResourceRowGroup;
-  noHeader?: boolean;
-  requestNestedRows: (row: ResourceRow) => Promise<void>;
-  onRowSelectedChange: (row: ResourceRow, selected: boolean) => void;
-  selectableEntryTypes: ResourceRowType[];
-}
-
-const NestedResourceTable: React.FC<NestedResourceTableProps> = ({
-  rows,
-  selectedRows,
-  noHeader,
-  requestNestedRows,
-  onRowSelectedChange,
-  selectableEntryTypes,
-}) => {
-  const styles = useStyles2(getStyles);
-
-  return (
-    <>
-      <table className={styles.table}>
-        {!noHeader && (
-          <thead>
-            <tr className={cx(styles.row, styles.header)}>
-              <td className={styles.cell}>Scope</td>
-              <td className={styles.cell}>Type</td>
-              <td className={styles.cell}>Location</td>
-            </tr>
-          </thead>
-        )}
-      </table>
-
-      <div className={styles.tableScroller}>
-        <table className={styles.table}>
-          <tbody>
-            <NestedRows
-              rows={rows}
-              selectedRows={selectedRows}
-              level={0}
-              requestNestedRows={requestNestedRows}
-              onRowSelectedChange={onRowSelectedChange}
-              selectableEntryTypes={selectableEntryTypes}
-            />
-          </tbody>
-        </table>
-      </div>
-    </>
-  );
-};
-
-export default NestedResourceTable;

public/app/plugins/datasource/grafana-azure-monitor-datasource/components/ResourcePicker/NestedRow.tsx

@@ -4,7 +4,6 @@ import React, { useEffect, useState } from 'react';
 import { FadeTransition, LoadingPlaceholder, useStyles2 } from '@grafana/ui';
 
 import { NestedEntry } from './NestedEntry';
-import NestedRows from './NestedRows';
 import getStyles from './styles';
 import { ResourceRow, ResourceRowGroup, ResourceRowType } from './types';
 import { findRow } from './utils';
@@ -16,6 +15,7 @@ interface NestedRowProps {
   requestNestedRows: (row: ResourceRow) => Promise<void>;
   onRowSelectedChange: (row: ResourceRow, selected: boolean) => void;
   selectableEntryTypes: ResourceRowType[];
+  scrollIntoView?: boolean;
 }
 
 const NestedRow: React.FC<NestedRowProps> = ({
@@ -25,11 +25,12 @@ const NestedRow: React.FC<NestedRowProps> = ({
   requestNestedRows,
   onRowSelectedChange,
   selectableEntryTypes,
+  scrollIntoView,
 }) => {
   const styles = useStyles2(getStyles);
   const [rowStatus, setRowStatus] = useState<'open' | 'closed' | 'loading'>('closed');
 
-  const isSelected = !!selectedRows.find((v) => v.id === row.id);
+  const isSelected = !!selectedRows.find((v) => v.uri === row.uri);
   const isDisabled = selectedRows.length > 0 && !isSelected;
   const isOpen = rowStatus === 'open';
 
@@ -49,7 +50,7 @@ const NestedRow: React.FC<NestedRowProps> = ({
     // Assuming we don't have multi-select yet
     const selectedRow = selectedRows[0];
 
-    const containsChild = selectedRow && !!findRow(row.children ?? [], selectedRow.id);
+    const containsChild = selectedRow && !!findRow(row.children ?? [], selectedRow.uri);
 
     if (containsChild) {
       setRowStatus('open');
@@ -69,6 +70,7 @@ const NestedRow: React.FC<NestedRowProps> = ({
             onToggleCollapse={onRowToggleCollapse}
             onSelectedChange={onRowSelectedChange}
             isSelectable={selectableEntryTypes.some((type) => type === row.type)}
+            scrollIntoView={scrollIntoView}
           />
         </td>
 
@@ -77,16 +79,21 @@ const NestedRow: React.FC<NestedRowProps> = ({
         <td className={styles.cell}>{row.location ?? '-'}</td>
       </tr>
 
-      {isOpen && row.children && Object.keys(row.children).length > 0 && (
-        <NestedRows
-          rows={row.children}
-          selectedRows={selectedRows}
-          level={level + 1}
-          requestNestedRows={requestNestedRows}
-          onRowSelectedChange={onRowSelectedChange}
-          selectableEntryTypes={selectableEntryTypes}
-        />
-      )}
+      {isOpen &&
+        row.children &&
+        Object.keys(row.children).length > 0 &&
+        row.children.map((childRow) => (
+          <NestedRow
+            key={childRow.uri}
+            row={childRow}
+            selectedRows={selectedRows}
+            level={level + 1}
+            requestNestedRows={requestNestedRows}
+            onRowSelectedChange={onRowSelectedChange}
+            selectableEntryTypes={selectableEntryTypes}
+            scrollIntoView={scrollIntoView}
+          />
+        ))}
 
       <FadeTransition visible={rowStatus === 'loading'}>
         <tr>

public/app/plugins/datasource/grafana-azure-monitor-datasource/components/ResourcePicker/NestedRows.tsx

@@ -1,38 +0,0 @@
-import React from 'react';
-
-import NestedRow from './NestedRow';
-import { ResourceRow, ResourceRowGroup, ResourceRowType } from './types';
-
-interface NestedRowsProps {
-  rows: ResourceRowGroup;
-  level: number;
-  selectedRows: ResourceRowGroup;
-  requestNestedRows: (row: ResourceRow) => Promise<void>;
-  onRowSelectedChange: (row: ResourceRow, selected: boolean) => void;
-  selectableEntryTypes: ResourceRowType[];
-}
-
-const NestedRows: React.FC<NestedRowsProps> = ({
-  rows,
-  selectedRows,
-  level,
-  requestNestedRows,
-  onRowSelectedChange,
-  selectableEntryTypes,
-}) => (
-  <>
-    {rows.map((row) => (
-      <NestedRow
-        key={row.id}
-        row={row}
-        selectedRows={selectedRows}
-        level={level}
-        requestNestedRows={requestNestedRows}
-        onRowSelectedChange={onRowSelectedChange}
-        selectableEntryTypes={selectableEntryTypes}
-      />
-    ))}
-  </>
-);
-
-export default NestedRows;

public/app/plugins/datasource/grafana-azure-monitor-datasource/components/ResourcePicker/ResourcePicker.test.tsx

@@ -40,7 +40,7 @@ const defaultProps = {
 
 describe('AzureMonitor ResourcePicker', () => {
   beforeEach(() => {
-    window.HTMLElement.prototype.scrollIntoView = function () {};
+    window.HTMLElement.prototype.scrollIntoView = jest.fn();
   });
   it('should pre-load subscriptions when there is no existing selection', async () => {
     render(<ResourcePicker {...defaultProps} resourceURI={noResourceURI} />);
@@ -53,20 +53,40 @@ describe('AzureMonitor ResourcePicker', () => {
 
   it('should show a subscription as selected if there is one saved', async () => {
     render(<ResourcePicker {...defaultProps} resourceURI={singleSubscriptionSelectionURI} />);
-    const subscriptionCheckbox = await screen.findByLabelText('Dev Subscription');
-    expect(subscriptionCheckbox).toBeChecked();
+    const subscriptionCheckboxes = await screen.findAllByLabelText('Dev Subscription');
+    expect(subscriptionCheckboxes.length).toBe(2);
+    expect(subscriptionCheckboxes[0]).toBeChecked();
+    expect(subscriptionCheckboxes[1]).toBeChecked();
   });
 
   it('should show a resourceGroup as selected if there is one saved', async () => {
     render(<ResourcePicker {...defaultProps} resourceURI={singleResourceGroupSelectionURI} />);
-    const resourceGroupCheckbox = await screen.findByLabelText('A Great Resource Group');
-    expect(resourceGroupCheckbox).toBeChecked();
+    const resourceGroupCheckboxes = await screen.findAllByLabelText('A Great Resource Group');
+    expect(resourceGroupCheckboxes.length).toBe(2);
+    expect(resourceGroupCheckboxes[0]).toBeChecked();
+    expect(resourceGroupCheckboxes[1]).toBeChecked();
   });
 
   it('should show a resource as selected if there is one saved', async () => {
     render(<ResourcePicker {...defaultProps} resourceURI={singleResourceSelectionURI} />);
-    const resourceCheckbox = await screen.findByLabelText('db-server');
-    expect(resourceCheckbox).toBeChecked();
+    const resourceCheckboxes = await screen.findAllByLabelText('db-server');
+    expect(resourceCheckboxes.length).toBe(2);
+    expect(resourceCheckboxes[0]).toBeChecked();
+    expect(resourceCheckboxes[1]).toBeChecked();
+  });
+
+  it('opens the selected nested resources', async () => {
+    render(<ResourcePicker {...defaultProps} resourceURI={singleResourceSelectionURI} />);
+    const collapseSubscriptionBtn = await screen.findByLabelText('Collapse Dev Subscription');
+    expect(collapseSubscriptionBtn).toBeInTheDocument();
+    const collapseResourceGroupBtn = await screen.findByLabelText('Collapse A Great Resource Group');
+    expect(collapseResourceGroupBtn).toBeInTheDocument();
+  });
+
+  it('scrolls down to the selected resource', async () => {
+    render(<ResourcePicker {...defaultProps} resourceURI={singleResourceSelectionURI} />);
+    await screen.findByLabelText('Collapse A Great Resource Group');
+    expect(window.HTMLElement.prototype.scrollIntoView).toBeCalledTimes(1);
   });
 
   it('should be able to expand a subscription when clicked and reveal resource groups', async () => {

public/app/plugins/datasource/grafana-azure-monitor-datasource/components/ResourcePicker/ResourcePicker.tsx

@@ -1,14 +1,14 @@
-import { css } from '@emotion/css';
+import { cx } from '@emotion/css';
 import React, { useCallback, useEffect, useMemo, useState } from 'react';
 
-import { GrafanaTheme2 } from '@grafana/data';
 import { Alert, Button, Icon, Input, LoadingPlaceholder, Tooltip, useStyles2, Collapse, Label } from '@grafana/ui';
 
 import ResourcePickerData from '../../resourcePicker/resourcePickerData';
 import messageFromError from '../../utils/messageFromError';
 import { Space } from '../Space';
 
-import NestedResourceTable from './NestedResourceTable';
+import NestedRow from './NestedRow';
+import getStyles from './styles';
 import { ResourceRow, ResourceRowGroup, ResourceRowType } from './types';
 import { addResources, findRow, parseResourceURI } from './utils';
 
@@ -142,29 +142,61 @@ const ResourcePicker = ({
         </div>
       ) : (
         <>
-          <NestedResourceTable
-            rows={azureRows}
-            requestNestedRows={requestNestedRows}
-            onRowSelectedChange={handleSelectionChanged}
-            selectedRows={selectedResourceRows}
-            selectableEntryTypes={selectableEntryTypes}
-          />
+          <table className={styles.table}>
+            <thead>
+              <tr className={cx(styles.row, styles.header)}>
+                <td className={styles.cell}>Scope</td>
+                <td className={styles.cell}>Type</td>
+                <td className={styles.cell}>Location</td>
+              </tr>
+            </thead>
+          </table>
+
+          <div className={styles.tableScroller}>
+            <table className={styles.table}>
+              <tbody>
+                {azureRows.map((row) => (
+                  <NestedRow
+                    key={row.uri}
+                    row={row}
+                    selectedRows={selectedResourceRows}
+                    level={0}
+                    requestNestedRows={requestNestedRows}
+                    onRowSelectedChange={handleSelectionChanged}
+                    selectableEntryTypes={selectableEntryTypes}
+                    scrollIntoView={true}
+                  />
+                ))}
+              </tbody>
+            </table>
+          </div>
 
           <div className={styles.selectionFooter}>
             {selectedResourceRows.length > 0 && (
               <>
                 <h5>Selection</h5>
-                <NestedResourceTable
-                  rows={selectedResourceRows}
-                  requestNestedRows={requestNestedRows}
-                  onRowSelectedChange={handleSelectionChanged}
-                  selectedRows={selectedResourceRows}
-                  noHeader={true}
-                  selectableEntryTypes={selectableEntryTypes}
-                />
+
+                <div className={styles.tableScroller}>
+                  <table className={styles.table}>
+                    <tbody>
+                      {selectedResourceRows.map((row) => (
+                        <NestedRow
+                          key={row.uri}
+                          row={row}
+                          selectedRows={selectedResourceRows}
+                          level={0}
+                          requestNestedRows={requestNestedRows}
+                          onRowSelectedChange={handleSelectionChanged}
+                          selectableEntryTypes={selectableEntryTypes}
+                        />
+                      ))}
+                    </tbody>
+                  </table>
+                </div>
                 <Space v={2} />
               </>
             )}
+
             <Collapse
               collapsible
               label="Advanced"
@@ -229,18 +261,3 @@ const ResourcePicker = ({
 };
 
 export default ResourcePicker;
-
-const getStyles = (theme: GrafanaTheme2) => ({
-  selectionFooter: css({
-    position: 'sticky',
-    bottom: 0,
-    background: theme.colors.background.primary,
-    paddingTop: theme.spacing(2),
-  }),
-  loadingWrapper: css({
-    textAlign: 'center',
-    paddingTop: theme.spacing(2),
-    paddingBottom: theme.spacing(2),
-    color: theme.colors.text.secondary,
-  }),
-});

public/app/plugins/datasource/grafana-azure-monitor-datasource/components/ResourcePicker/styles.ts

@@ -69,6 +69,20 @@ const getStyles = (theme: GrafanaTheme2) => ({
   nestedRowCheckbox: css({
     zIndex: 0,
   }),
+
+  selectionFooter: css({
+    position: 'sticky',
+    bottom: 0,
+    background: theme.colors.background.primary,
+    paddingTop: theme.spacing(2),
+  }),
+
+  loadingWrapper: css({
+    textAlign: 'center',
+    paddingTop: theme.spacing(2),
+    paddingBottom: theme.spacing(2),
+    color: theme.colors.text.secondary,
+  }),
 });
 
 export default getStyles;