Role editor: hierarchical validation and tabs

web/packages/teleport/src/Roles/RoleEditor/EditorHeader.tsx

@@ -17,29 +17,53 @@
  */
 
 import React from 'react';
-import { Flex, ButtonText, H2 } from 'design';
+import { Flex, ButtonText, H2, Indicator, Box } from 'design';
 import { HoverTooltip } from 'design/Tooltip';
 import { Trash } from 'design/Icon';
 
 import useTeleport from 'teleport/useTeleport';
 import { Role } from 'teleport/services/resources';
 
+import { EditorTab, EditorTabs } from './EditorTabs';
+
 /** Renders a header button with role name and delete button. */
 export const EditorHeader = ({
   role = null,
   onDelete,
+  selectedEditorTab,
+  onEditorTabChange,
+  isProcessing,
+  standardEditorId,
+  yamlEditorId,
 }: {
-  onDelete?(): void;
   role?: Role;
+  onDelete(): void;
+  selectedEditorTab: EditorTab;
+  onEditorTabChange(t: EditorTab): void;
+  isProcessing: boolean;
+  standardEditorId: string;
+  yamlEditorId: string;
 }) => {
   const ctx = useTeleport();
   const isCreating = !role;
 
   const hasDeleteAccess = ctx.storeUser.getRoleAccess().remove;
 
   return (
-    <Flex alignItems="center" mb={3} justifyContent="space-between">
-      <H2>{isCreating ? 'Create a New Role' : role?.metadata.name}</H2>
+    <Flex alignItems="center" mb={3} gap={2}>
+      <Box flex="1">
+        <H2>{isCreating ? 'Create a New Role' : role?.metadata.name}</H2>
+      </Box>
+      <Box flex="0 0 24px" lineHeight={0}>
+        {isProcessing && <Indicator size={24} color="text.muted" />}
+      </Box>
+      <EditorTabs
+        onTabChange={onEditorTabChange}
+        selectedEditorTab={selectedEditorTab}
+        disabled={isProcessing}
+        standardEditorId={standardEditorId}
+        yamlEditorId={yamlEditorId}
+      />
       {!isCreating && (
         <HoverTooltip
           position="bottom"

web/packages/teleport/src/Roles/RoleEditor/EditorTabs.tsx

@@ -18,8 +18,8 @@
 
 import React from 'react';
 import { SlideTabs } from 'design/SlideTabs';
+import * as Icon from 'design/Icon';
 
-const tabs = ['Standard', 'YAML'];
 export enum EditorTab {
   Standard,
   Yaml,
@@ -28,20 +28,44 @@ export enum EditorTab {
 export const EditorTabs = ({
   onTabChange,
   selectedEditorTab,
-  isProcessing,
+  disabled,
+  standardEditorId,
+  yamlEditorId,
 }: {
   onTabChange(t: EditorTab): void;
   selectedEditorTab: EditorTab;
-  isProcessing: boolean;
+  disabled: boolean;
+  standardEditorId: string;
+  yamlEditorId: string;
 }) => {
+  const standardLabel = 'Switch to standard editor';
+  const yamlLabel = 'Switch to YAML editor';
   return (
     <SlideTabs
       appearance="round"
-      tabs={tabs}
+      tabs={[
+        {
+          key: 'standard',
+          icon: Icon.ListAddCheck,
+          tooltipContent: standardLabel,
+          tooltipPosition: 'bottom',
+          ariaLabel: standardLabel,
+          controls: standardEditorId,
+        },
+        {
+          key: 'yaml',
+          icon: Icon.Code,
+          tooltipContent: yamlLabel,
+          tooltipPosition: 'bottom',
+          ariaLabel: yamlLabel,
+          controls: yamlEditorId,
+        },
+      ]}
       onChange={onTabChange}
-      size="medium"
+      size="small"
+      fitContent
       activeIndex={selectedEditorTab}
-      isProcessing={isProcessing}
+      disabled={disabled}
     />
   );
 };

web/packages/teleport/src/Roles/RoleEditor/RoleEditor.test.tsx

@@ -75,18 +75,15 @@ afterEach(() => {
 
 test('rendering and switching tabs for new role', async () => {
   render(<TestRoleEditor />);
-  expect(screen.getByRole('tab', { name: 'Standard' })).toHaveAttribute(
-    'aria-selected',
-    'true'
-  );
+  expect(getStandardEditorTab()).toHaveAttribute('aria-selected', 'true');
   expect(
     screen.queryByRole('button', { name: /Reset to Standard Settings/i })
   ).not.toBeInTheDocument();
   expect(screen.getByLabelText('Role Name')).toHaveValue('new_role_name');
   expect(screen.getByLabelText('Description')).toHaveValue('');
   expect(screen.getByRole('button', { name: 'Create Role' })).toBeEnabled();
 
-  await user.click(screen.getByRole('tab', { name: 'YAML' }));
+  await user.click(getYamlEditorTab());
   expect(fromFauxYaml(await getTextEditorContents())).toEqual(
     withDefaults({
       kind: 'role',
@@ -103,7 +100,7 @@ test('rendering and switching tabs for new role', async () => {
   );
   expect(screen.getByRole('button', { name: 'Create Role' })).toBeEnabled();
 
-  await user.click(screen.getByRole('tab', { name: 'Standard' }));
+  await user.click(getStandardEditorTab());
   await screen.findByLabelText('Role Name');
   expect(
     screen.queryByRole('button', { name: /Reset to Standard Settings/i })
@@ -127,41 +124,60 @@ test('rendering and switching tabs for a non-standard role', async () => {
       originalRole={{ object: originalRole, yaml: originalYaml }}
     />
   );
-  expect(screen.getByRole('tab', { name: 'YAML' })).toHaveAttribute(
-    'aria-selected',
-    'true'
-  );
+  expect(getYamlEditorTab()).toHaveAttribute('aria-selected', 'true');
   expect(fromFauxYaml(await getTextEditorContents())).toEqual(originalRole);
   expect(screen.getByRole('button', { name: 'Update Role' })).toBeDisabled();
 
-  await user.click(screen.getByRole('tab', { name: 'Standard' }));
+  await user.click(getStandardEditorTab());
   expect(
     screen.getByRole('button', { name: 'Reset to Standard Settings' })
   ).toBeVisible();
   expect(screen.getByLabelText('Role Name')).toHaveValue('some-role');
   expect(screen.getByLabelText('Description')).toHaveValue('');
   expect(screen.getByRole('button', { name: 'Update Role' })).toBeDisabled();
 
-  await user.click(screen.getByRole('tab', { name: 'YAML' }));
+  await user.click(getYamlEditorTab());
   expect(fromFauxYaml(await getTextEditorContents())).toEqual(originalRole);
   expect(screen.getByRole('button', { name: 'Update Role' })).toBeDisabled();
 
   // Switch once again, reset to standard
-  await user.click(screen.getByRole('tab', { name: 'Standard' }));
+  await user.click(getStandardEditorTab());
   expect(screen.getByRole('button', { name: 'Update Role' })).toBeDisabled();
   await user.click(
     screen.getByRole('button', { name: 'Reset to Standard Settings' })
   );
   expect(screen.getByRole('button', { name: 'Update Role' })).toBeEnabled();
   await user.type(screen.getByLabelText('Description'), 'some description');
 
-  await user.click(screen.getByRole('tab', { name: 'YAML' }));
+  await user.click(getYamlEditorTab());
   const editorContents = fromFauxYaml(await getTextEditorContents());
   expect(editorContents.metadata.description).toBe('some description');
   expect(editorContents.spec.deny).toEqual({});
   expect(screen.getByRole('button', { name: 'Update Role' })).toBeEnabled();
 });
 
+test('no double conversions when clicking already active tabs', async () => {
+  render(<TestRoleEditor />);
+  await user.click(getYamlEditorTab());
+  await user.click(getStandardEditorTab());
+  await user.type(screen.getByLabelText('Role Name'), '_2');
+  await user.click(getStandardEditorTab());
+  expect(screen.getByLabelText('Role Name')).toHaveValue('new_role_name_2');
+
+  await user.click(getYamlEditorTab());
+  await user.clear(await findTextEditor());
+  await user.type(
+    await findTextEditor(),
+    // Note: this is actually correct JSON syntax; the testing library uses
+    // braces for special keys, so we need to use double opening braces.
+    '{{"kind":"role", metadata:{{"name":"new_role_name_3"}}'
+  );
+  await user.click(getYamlEditorTab());
+  expect(await getTextEditorContents()).toBe(
+    '{"kind":"role", metadata:{"name":"new_role_name_3"}}'
+  );
+});
+
 test('canceling standard editor', async () => {
   const onCancel = jest.fn();
   render(<TestRoleEditor onCancel={onCancel} />);
@@ -175,7 +191,7 @@ test('canceling standard editor', async () => {
 test('canceling yaml editor', async () => {
   const onCancel = jest.fn();
   render(<TestRoleEditor onCancel={onCancel} />);
-  await user.click(screen.getByRole('tab', { name: 'YAML' }));
+  await user.click(getYamlEditorTab());
   await user.click(screen.getByRole('button', { name: 'Cancel' }));
   expect(onCancel).toHaveBeenCalled();
   expect(userEventService.captureUserEvent).toHaveBeenCalledWith({
@@ -222,7 +238,7 @@ test('saving a new role after editing as YAML', async () => {
   render(<TestRoleEditor onSave={onSave} />);
   expect(screen.getByRole('button', { name: 'Create Role' })).toBeEnabled();
 
-  await user.click(screen.getByRole('tab', { name: 'YAML' }));
+  await user.click(getYamlEditorTab());
   await user.clear(await findTextEditor());
   await user.type(await findTextEditor(), '{{"foo":"bar"}');
   await user.click(screen.getByRole('button', { name: 'Create Role' }));
@@ -247,7 +263,7 @@ test('error while yamlifying', async () => {
     .spyOn(yamlService, 'stringify')
     .mockRejectedValue(new Error('me no speak yaml'));
   render(<TestRoleEditor />);
-  await user.click(screen.getByRole('tab', { name: 'YAML' }));
+  await user.click(getYamlEditorTab());
   expect(screen.getByText('me no speak yaml')).toBeVisible();
 });
 
@@ -256,8 +272,8 @@ test('error while parsing', async () => {
     .spyOn(yamlService, 'parse')
     .mockRejectedValue(new Error('me no speak yaml'));
   render(<TestRoleEditor />);
-  await user.click(screen.getByRole('tab', { name: 'YAML' }));
-  await user.click(screen.getByRole('tab', { name: 'Standard' }));
+  await user.click(getYamlEditorTab());
+  await user.click(getStandardEditorTab());
   expect(screen.getByText('me no speak yaml')).toBeVisible();
 });
 
@@ -280,6 +296,12 @@ const TestRoleEditor = (props: RoleEditorProps) => {
   );
 };
 
+const getStandardEditorTab = () =>
+  screen.getByRole('tab', { name: 'Switch to standard editor' });
+
+const getYamlEditorTab = () =>
+  screen.getByRole('tab', { name: 'Switch to YAML editor' });
+
 const findTextEditor = async () =>
   within(await screen.findByTestId('text-editor-container')).getByRole(
     'textbox'

web/packages/teleport/src/Roles/RoleEditor/RoleEditor.tsx

@@ -16,8 +16,8 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-import { Alert, Box, Flex } from 'design';
-import React, { useState } from 'react';
+import { Alert, Flex } from 'design';
+import React, { useId, useState } from 'react';
 import { useAsync } from 'shared/hooks/useAsync';
 
 import { Role, RoleWithYaml } from 'teleport/services/resources';
@@ -32,7 +32,7 @@ import {
   roleToRoleEditorModel as roleToRoleEditorModel,
 } from './standardmodel';
 import { YamlEditorModel } from './yamlmodel';
-import { EditorTab, EditorTabs } from './EditorTabs';
+import { EditorTab } from './EditorTabs';
 import { EditorHeader } from './EditorHeader';
 import { StandardEditor } from './StandardEditor';
 import { YamlEditor } from './YamlEditor';
@@ -59,6 +59,10 @@ export const RoleEditor = ({
   onSave,
   onDelete,
 }: RoleEditorProps) => {
+  const idPrefix = useId();
+  const standardEditorId = `${idPrefix}-standard`;
+  const yamlEditorId = `${idPrefix}-yaml`;
+
   const [standardModel, setStandardModel] = useState<StandardEditorModel>(
     () => {
       const role = originalRole?.object ?? newRole();
@@ -114,6 +118,10 @@ export const RoleEditor = ({
     saveAttempt.status === 'processing';
 
   async function onTabChange(activeIndex: EditorTab) {
+    // The code below is not idempotent, so we need to protect ourselves from
+    // an accidental model replacement.
+    if (activeIndex === selectedEditorTab) return;
+
     switch (activeIndex) {
       case EditorTab.Standard: {
         if (!yamlModel.content) {
@@ -160,7 +168,15 @@ export const RoleEditor = ({
 
   return (
     <Flex flexDirection="column" flex="1">
-      <EditorHeader role={originalRole?.object} onDelete={onDelete} />
+      <EditorHeader
+        role={originalRole?.object}
+        onDelete={onDelete}
+        selectedEditorTab={selectedEditorTab}
+        onEditorTabChange={onTabChange}
+        isProcessing={isProcessing}
+        standardEditorId={standardEditorId}
+        yamlEditorId={yamlEditorId}
+      />
       {saveAttempt.status === 'error' && (
         <Alert mt={3} dismissible>
           {saveAttempt.statusText}
@@ -176,32 +192,29 @@ export const RoleEditor = ({
           {yamlifyAttempt.statusText}
         </Alert>
       )}
-      <Box mb={3}>
-        <EditorTabs
-          onTabChange={onTabChange}
-          selectedEditorTab={selectedEditorTab}
-          isProcessing={isProcessing}
-        />
-      </Box>
       {selectedEditorTab === EditorTab.Standard && (
-        <StandardEditor
-          originalRole={originalRole}
-          onSave={object => handleSave({ object })}
-          onCancel={handleCancel}
-          standardEditorModel={standardModel}
-          isProcessing={isProcessing}
-          onChange={setStandardModel}
-        />
+        <div id={standardEditorId}>
+          <StandardEditor
+            originalRole={originalRole}
+            onSave={object => handleSave({ object })}
+            onCancel={handleCancel}
+            standardEditorModel={standardModel}
+            isProcessing={isProcessing}
+            onChange={setStandardModel}
+          />
+        </div>
       )}
       {selectedEditorTab === EditorTab.Yaml && (
-        <YamlEditor
-          yamlEditorModel={yamlModel}
-          onChange={setYamlModel}
-          onSave={async yaml => void (await handleSave({ yaml }))}
-          isProcessing={isProcessing}
-          onCancel={handleCancel}
-          originalRole={originalRole}
-        />
+        <Flex flexDirection="column" flex="1" id={yamlEditorId}>
+          <YamlEditor
+            yamlEditorModel={yamlModel}
+            onChange={setYamlModel}
+            onSave={async yaml => void (await handleSave({ yaml }))}
+            isProcessing={isProcessing}
+            onCancel={handleCancel}
+            originalRole={originalRole}
+          />
+        </Flex>
       )}
     </Flex>
   );