Merge branch 'master' into extract-fixability-v2

Author: Mihir-Mavalankar

static/app/components/editableText.spec.tsx

@@ -44,6 +44,21 @@ describe('EditableText', () => {
     expect(handleChange).toHaveBeenCalledTimes(0);
   });
 
+  it('reverts to previous value when allowEmpty is true', async () => {
+    const handleChange = jest.fn();
+
+    render(
+      <EditableText value="foo" onChange={handleChange} allowEmpty errorMessage="error" />
+    );
+
+    await userEvent.click(screen.getByText('foo'));
+    await userEvent.clear(screen.getByRole('textbox'));
+    await userEvent.click(document.body);
+
+    expect(handleChange).not.toHaveBeenCalled();
+    expect(screen.getByText('foo')).toBeInTheDocument();
+  });
+
   it('displays a disabled value', async () => {
     render(<EditableText value="foo" onChange={jest.fn()} isDisabled />);
 

static/app/components/editableText.tsx

@@ -7,13 +7,16 @@ import {Input} from 'sentry/components/core/input';
 import TextOverflow from 'sentry/components/textOverflow';
 import {IconEdit} from 'sentry/icons/iconEdit';
 import {space} from 'sentry/styles/space';
-import {defined} from 'sentry/utils';
-import useKeypress from 'sentry/utils/useKeyPress';
 import useOnClickOutside from 'sentry/utils/useOnClickOutside';
 
 type Props = {
   onChange: (value: string) => void;
   value: string;
+  /**
+   * When true, clearing the input and blurring cancels the edit and restores
+   * the previous value instead of showing an error toast.
+   */
+  allowEmpty?: boolean;
   'aria-label'?: string;
   autoSelect?: boolean;
   className?: string;
@@ -40,136 +43,170 @@ function EditableText({
   className,
   'aria-label': ariaLabel,
   placeholder,
+  allowEmpty = false,
 }: Props) {
   const [isEditing, setIsEditing] = useState(false);
-  const [inputValue, setInputValue] = useState(value);
+  // Immediately reflect the last committed value while we wait for the parent prop update
+  const [optimisticValue, setOptimisticValue] = useState<string | null>(null);
+  // Current keystrokes while editing; cleared whenever editing ends
+  const [draftValue, setDraftValue] = useState<string | null>(null);
 
-  const isEmpty = !inputValue.trim();
+  const currentValue = optimisticValue ?? value;
+  const currentDraft = draftValue ?? currentValue;
+  const isDraftEmpty = !currentDraft.trim();
 
   const innerWrapperRef = useRef<HTMLDivElement>(null);
   const labelRef = useRef<HTMLDivElement>(null);
   const inputRef = useRef<HTMLInputElement>(null);
+  const previousValueRef = useRef(value);
+
+  const showStatusMessage = useCallback(
+    (status: 'error' | 'success') => {
+      if (status === 'error') {
+        if (errorMessage) {
+          addErrorMessage(errorMessage);
+        }
+        return;
+      }
 
-  const enter = useKeypress('Enter');
-  const esc = useKeypress('Escape');
+      if (successMessage) {
+        addSuccessMessage(successMessage);
+      }
+    },
+    [errorMessage, successMessage]
+  );
 
-  function revertValueAndCloseEditor() {
-    if (value !== inputValue) {
-      setInputValue(value);
-    }
+  const exitEditing = useCallback(() => {
+    setIsEditing(false);
+  }, []);
 
-    if (isEditing) {
-      setIsEditing(false);
-    }
-  }
+  const handleCancel = useCallback(() => {
+    setDraftValue(null);
+    exitEditing();
+  }, [exitEditing]);
 
-  // check to see if the user clicked outside of this component
-  useOnClickOutside(innerWrapperRef, () => {
-    if (!isEditing) {
-      return;
+  const handleCommit = useCallback(() => {
+    if (isDraftEmpty) {
+      showStatusMessage('error');
+      return false;
     }
 
-    if (isEmpty) {
-      displayStatusMessage('error');
-      return;
+    if (currentDraft !== currentValue) {
+      onChange(currentDraft);
+      showStatusMessage('success');
     }
 
-    if (inputValue !== value) {
-      onChange(inputValue);
-      displayStatusMessage('success');
+    exitEditing();
+    setOptimisticValue(currentDraft);
+    setDraftValue(null);
+    return true;
+  }, [
+    currentDraft,
+    currentValue,
+    exitEditing,
+    isDraftEmpty,
+    onChange,
+    showStatusMessage,
+  ]);
+
+  const handleEmptyBlur = useCallback(() => {
+    if (allowEmpty) {
+      handleCancel();
+    } else {
+      showStatusMessage('error');
     }
+  }, [allowEmpty, handleCancel, showStatusMessage]);
 
-    setIsEditing(false);
-  });
-
-  const onEnter = useCallback(() => {
-    if (enter) {
-      if (isEmpty) {
-        displayStatusMessage('error');
-        return;
-      }
-
-      if (inputValue !== value) {
-        onChange(inputValue);
-        displayStatusMessage('success');
-      }
-
-      setIsEditing(false);
+  // Close editing if the field becomes disabled (e.g. form revalidation)
+  useEffect(() => {
+    if (isDisabled) {
+      handleCancel();
     }
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, [enter, inputValue, onChange]);
+  }, [handleCancel, isDisabled]);
 
-  const onEsc = useCallback(() => {
-    if (esc) {
-      revertValueAndCloseEditor();
+  // Reset our optimistic/draft state whenever the controlled value changes externally
+  useEffect(() => {
+    if (previousValueRef.current === value) {
+      return;
     }
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, [esc]);
 
-  useEffect(() => {
-    revertValueAndCloseEditor();
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, [isDisabled, value]);
+    previousValueRef.current = value;
+    setOptimisticValue(null);
 
-  // focus the cursor in the input field on edit start
-  useEffect(() => {
     if (isEditing) {
-      const inputElement = inputRef.current;
-      if (defined(inputElement)) {
-        inputElement.focus();
-      }
+      setDraftValue(null);
+      exitEditing();
     }
-  }, [isEditing]);
+  }, [exitEditing, isEditing, value]);
 
+  // Focus the input whenever we enter editing mode
   useEffect(() => {
     if (isEditing) {
-      // if Enter is pressed, save the value and close the editor
-      onEnter();
-      // if Escape is pressed, revert the value and close the editor
-      onEsc();
+      inputRef.current?.focus();
     }
-  }, [onEnter, onEsc, isEditing]); // watch the Enter and Escape key presses
+  }, [isEditing]);
 
-  function displayStatusMessage(status: 'error' | 'success') {
-    if (status === 'error') {
-      if (errorMessage) {
-        addErrorMessage(errorMessage);
-      }
+  const handleClickOutside = useCallback(() => {
+    if (!isEditing) {
       return;
     }
 
-    if (successMessage) {
-      addSuccessMessage(successMessage);
+    if (isDraftEmpty) {
+      handleEmptyBlur();
+      return;
     }
-  }
 
-  function handleInputChange(event: React.ChangeEvent<HTMLInputElement>) {
-    setInputValue(event.target.value);
-  }
+    handleCommit();
+  }, [handleCommit, handleEmptyBlur, isDraftEmpty, isEditing]);
+
+  useOnClickOutside(innerWrapperRef, handleClickOutside);
+
+  const handleInputChange = useCallback((event: React.ChangeEvent<HTMLInputElement>) => {
+    setDraftValue(event.target.value);
+  }, []);
+
+  const handleEditClick = useCallback(() => {
+    if (isDisabled) {
+      return;
+    }
 
-  function handleEditClick() {
+    setDraftValue(currentValue);
     setIsEditing(true);
-  }
+  }, [currentValue, isDisabled]);
+
+  const handleKeyDown = useCallback(
+    (event: React.KeyboardEvent<HTMLInputElement>) => {
+      if (event.key === 'Enter') {
+        event.preventDefault();
+        handleCommit();
+      } else if (event.key === 'Escape') {
+        event.preventDefault();
+        handleCancel();
+      }
+    },
+    [handleCancel, handleCommit]
+  );
 
   return (
     <Wrapper isDisabled={isDisabled} isEditing={isEditing} className={className}>
       {isEditing ? (
         <InputWrapper
           ref={innerWrapperRef}
-          isEmpty={isEmpty}
+          isEmpty={isDraftEmpty}
           data-test-id="editable-text-input"
         >
           <StyledInput
             aria-label={ariaLabel}
             name={name}
             ref={inputRef}
-            value={inputValue}
+            value={currentDraft}
             onChange={handleInputChange}
+            onKeyDown={handleKeyDown}
             onFocus={event => autoSelect && event.target.select()}
             maxLength={maxLength}
             placeholder={placeholder}
           />
-          <InputLabel>{inputValue}</InputLabel>
+          <InputLabel>{currentDraft}</InputLabel>
         </InputWrapper>
       ) : (
         <Label
@@ -178,7 +215,7 @@ function EditableText({
           isDisabled={isDisabled}
           data-test-id="editable-text-label"
         >
-          <InnerLabel>{inputValue || placeholder}</InnerLabel>
+          <InnerLabel>{currentValue || placeholder}</InnerLabel>
           {!isDisabled && <IconEdit />}
         </Label>
       )}

static/app/views/automations/components/editableAutomationName.tsx

@@ -10,7 +10,7 @@ export function EditableAutomationName() {
     <FormField name="name" inline={false} flexibleControlStateSize stacked>
       {({onChange, value}) => (
         <EditableText
-          isDisabled={false}
+          allowEmpty
           value={value || ''}
           onChange={newValue => {
             // Mark that the user has manually set the automation name
@@ -21,7 +21,6 @@ export function EditableAutomationName() {
               },
             });
           }}
-          errorMessage={t('Please set a name for your alert.')}
           placeholder={t('New Alert')}
         />
       )}

static/app/views/automations/components/forms/automationNameUtils.spec.tsx

@@ -78,9 +78,9 @@ describe('automationNameUtils', () => {
         actionFilters: [ActionFilterFixture({actions})],
       }) as AutomationBuilderState;
 
-    it('should return "New Alert" for empty actions', () => {
+    it('should return "" for empty actions', () => {
       const builderState = createBuilderState([]);
-      expect(getAutomationName(builderState)).toBe('New Alert');
+      expect(getAutomationName(builderState)).toBe('');
     });
 
     it('should return single action description', () => {

static/app/views/automations/components/forms/automationNameUtils.tsx

@@ -45,7 +45,7 @@ export function getAutomationName(builderState: AutomationBuilderState): string
   const allActions = builderState.actionFilters.flatMap(group => group.actions || []);
 
   if (allActions.length === 0) {
-    return 'New Alert';
+    return '';
   }
 
   const count = allActions.length;
@@ -78,7 +78,7 @@ export function getAutomationName(builderState: AutomationBuilderState): string
   // Fallback if even a single action is too long
   if (actionsToInclude === 0) {
     if (count === 0) {
-      automationName = 'New Alert';
+      automationName = '';
     } else {
       automationName = t('New Alert (%s actions)', count);
     }

static/app/views/detectors/components/forms/common/describeSection.tsx

@@ -1,3 +1,5 @@
+import styled from '@emotion/styled';
+
 import TextareaField from 'sentry/components/forms/fields/textareaField';
 import {Container} from 'sentry/components/workflowEngine/ui/container';
 import Section from 'sentry/components/workflowEngine/ui/section';
@@ -10,7 +12,7 @@ export function DescribeSection() {
         title={t('Description')}
         description={t('Additional context about this monitor for other team members.')}
       >
-        <TextareaField
+        <MinHeightTextarea
           name="description"
           stacked
           inline={false}
@@ -19,8 +21,17 @@ export function DescribeSection() {
             'Example monitor description\n\nTo debug follow these steps:\n1. \u2026\n2. \u2026\n3. \u2026'
           )}
           rows={6}
+          autosize
         />
       </Section>
     </Container>
   );
 }
+
+// Min height helps prevent resize after placeholder is replaced with user input
+const MinHeightTextarea = styled(TextareaField)`
+  padding: 0;
+  textarea {
+    min-height: 140px;
+  }
+`;

static/app/views/detectors/components/forms/cron/fields.tsx

@@ -75,16 +75,17 @@ export function cronFormDataToEndpointPayload(
           schedule_type: ScheduleType.INTERVAL,
         };
 
+  const name = data.name || 'New Monitor';
   return {
     type: 'monitor_check_in_failure',
-    name: data.name,
+    name,
     description: data.description || null,
     owner: data.owner,
     projectId: data.projectId,
     workflowIds: data.workflowIds,
     dataSources: [
       {
-        name: data.name,
+        name,
         config,
       },
     ],