ref(codeowners): Bug fixing on results for SelectAsync settings form (#31383)

See API-2389

This PR will improve the reliability of the SelectAsyncField component used in settings forms. By default this form always made the request on load due to the defaultOptions prop being hardcoded. Now that it is an prop, the options can be passed in to reduce identical API calls (say for rendering many of a form on a single page, as we want to do with mappings).

Additionally, the field also stores its last selection in local state, which gets rid of the issues when typing into a field but not saving it.

Though I would have liked for this to be a PR just focused on the base component. Merging that without changes to the User/Team mappings would have introduced some weird bugs that I'd rather just avoid. Before, we were trying to seed the API results with the initial value of the field, instead of using the defaultOptions prop baked into react-select.

From the demos, you can see the changes work well, and searching in the form no longer has weird, unrelated results, or blank values.

static/app/components/forms/selectAsyncControl.tsx

@@ -4,22 +4,25 @@ import debounce from 'lodash/debounce';
 
 import {addErrorMessage} from 'sentry/actionCreators/indicator';
 import {Client} from 'sentry/api';
+import SelectControl, {
+  ControlProps,
+  GeneralSelectValue,
+} from 'sentry/components/forms/selectControl';
 import {t} from 'sentry/locale';
 import handleXhrErrorResponse from 'sentry/utils/handleXhrErrorResponse';
 
-import SelectControl, {ControlProps, GeneralSelectValue} from './selectControl';
-
-type Result = {
+export type Result = {
   value: string;
   label: string;
 };
 
-type Props = {
+export type SelectAsyncControlProps = {
   url: string;
   onResults: (data: any) => Result[]; // TODO(ts): Improve data type
   onQuery: (query: string | undefined) => {};
   forwardedRef: React.Ref<ReactSelect<GeneralSelectValue>>;
   value: ControlProps['value'];
+  defaultOptions?: boolean | GeneralSelectValue[];
 };
 
 type State = {
@@ -27,11 +30,12 @@ type State = {
 };
 
 /**
- * Performs an API request to `url` when menu is initially opened
+ * Performs an API request to `url` to fetch the options
  */
-class SelectAsyncControl extends React.Component<Props> {
+class SelectAsyncControl extends React.Component<SelectAsyncControlProps> {
   static defaultProps = {
     placeholder: '--',
+    defaultOptions: true,
   };
 
   constructor(props) {
@@ -101,7 +105,7 @@ class SelectAsyncControl extends React.Component<Props> {
   };
 
   render() {
-    const {value, forwardedRef, ...props} = this.props;
+    const {value, forwardedRef, defaultOptions, ...props} = this.props;
     return (
       <SelectControl
         // The key is used as a way to force a reload of the options:
@@ -109,7 +113,7 @@ class SelectAsyncControl extends React.Component<Props> {
         key={value}
         ref={forwardedRef}
         value={value}
-        defaultOptions
+        defaultOptions={defaultOptions}
         loadOptions={this.handleLoadOptions}
         onInputChange={this.handleInputChange}
         async

static/app/components/integrationExternalMappingForm.tsx

@@ -2,25 +2,30 @@ import {Component} from 'react';
 import styled from '@emotion/styled';
 import capitalize from 'lodash/capitalize';
 
+import {SelectAsyncControlProps} from 'sentry/components/forms/selectAsyncControl';
 import {t, tct} from 'sentry/locale';
 import {ExternalActorMapping, Integration} from 'sentry/types';
-import {getExternalActorEndpointDetails} from 'sentry/utils/integrationUtil';
+import {
+  getExternalActorEndpointDetails,
+  sentryNameToOption,
+} from 'sentry/utils/integrationUtil';
 import {FieldFromConfig} from 'sentry/views/settings/components/forms';
 import Form from 'sentry/views/settings/components/forms/form';
 import FormModel from 'sentry/views/settings/components/forms/model';
 import {Field} from 'sentry/views/settings/components/forms/type';
 
-type Props = Pick<Form['props'], 'onCancel' | 'onSubmitSuccess' | 'onSubmitError'> & {
-  integration: Integration;
-  mapping?: ExternalActorMapping;
-  type: 'user' | 'team';
-  getBaseFormEndpoint: (mapping?: ExternalActorMapping) => string;
-  sentryNamesMapper: (v: any) => {id: string; name: string}[];
-  dataEndpoint: string;
-  onResults?: (data: any, mappingKey?: string) => void;
-  isInline?: boolean;
-  mappingKey?: string;
-};
+type Props = Pick<Form['props'], 'onCancel' | 'onSubmitSuccess' | 'onSubmitError'> &
+  Pick<SelectAsyncControlProps, 'defaultOptions'> & {
+    integration: Integration;
+    mapping?: ExternalActorMapping;
+    type: 'user' | 'team';
+    getBaseFormEndpoint: (mapping?: ExternalActorMapping) => string;
+    sentryNamesMapper: (v: any) => {id: string; name: string}[];
+    dataEndpoint: string;
+    onResults?: (data: any, mappingKey?: string) => void;
+    isInline?: boolean;
+    mappingKey?: string;
+  };
 
 export default class IntegrationExternalMappingForm extends Component<Props> {
   model = new FormModel();
@@ -34,6 +39,25 @@ export default class IntegrationExternalMappingForm extends Component<Props> {
     };
   }
 
+  getDefaultOptions(mapping?: ExternalActorMapping) {
+    const {defaultOptions, type} = this.props;
+    if (typeof defaultOptions === 'boolean') {
+      return defaultOptions;
+    }
+    const options = [...(defaultOptions ?? [])];
+    if (!mapping) {
+      return options;
+    }
+    // For organizations with >100 entries, we want to make sure their
+    // saved mapping gets populated in the results if it wouldn't have
+    // been in the initial 100 API results, which is why we add it here
+    const mappingId = mapping[`${type}Id`];
+    const mappingOption = options.find(({value}) => mappingId && value === mappingId);
+    return !!mappingOption
+      ? options
+      : [{value: mappingId, label: mapping.sentryName}, ...options];
+  }
+
   get formFields(): Field[] {
     const {
       dataEndpoint,
@@ -44,8 +68,6 @@ export default class IntegrationExternalMappingForm extends Component<Props> {
       sentryNamesMapper,
       type,
     } = this.props;
-    const optionMapper = sentryNames =>
-      sentryNames.map(({name, id}) => ({value: id, label: name}));
     const fields: Field[] = [
       {
         name: `${type}Id`,
@@ -54,27 +76,10 @@ export default class IntegrationExternalMappingForm extends Component<Props> {
         label: isInline ? undefined : tct('Sentry [type]', {type: capitalize(type)}),
         placeholder: t(`Select Sentry ${capitalize(type)}`),
         url: dataEndpoint,
+        defaultOptions: this.getDefaultOptions(mapping),
         onResults: result => {
           onResults?.(result, isInline ? mapping?.externalName : mappingKey);
-          // TODO(Leander): The code below only fixes the problem when viewed, not when edited
-          // Pagination still has bugs for results not on initial return of the query
-
-          // For organizations with >100 entries, we want to make sure their
-          // saved mapping gets populated in the results if it wouldn't have
-          // been in the initial 100 API results, which is why we add it here
-          if (
-            mapping &&
-            !result.find(entry => {
-              const id = type === 'user' ? entry.user.id : entry.id;
-              return id === mapping[`${type}Id`];
-            })
-          ) {
-            return optionMapper([
-              {id: mapping[`${type}Id`], name: mapping.sentryName},
-              ...sentryNamesMapper(result),
-            ]);
-          }
-          return optionMapper(sentryNamesMapper(result));
+          return sentryNamesMapper(result).map(sentryNameToOption);
         },
       },
     ];

static/app/components/integrationExternalMappings.tsx

@@ -20,7 +20,11 @@ import EmptyMessage from 'sentry/views/settings/components/emptyMessage';
 
 type Props = Pick<
   IntegrationExternalMappingForm['props'],
-  'dataEndpoint' | 'getBaseFormEndpoint' | 'sentryNamesMapper' | 'onResults'
+  | 'dataEndpoint'
+  | 'getBaseFormEndpoint'
+  | 'sentryNamesMapper'
+  | 'onResults'
+  | 'defaultOptions'
 > & {
   organization: Organization;
   integration: Integration;
@@ -41,6 +45,7 @@ class IntegrationExternalMappings extends Component<Props, State> {
       dataEndpoint,
       sentryNamesMapper,
       onResults,
+      defaultOptions,
     } = this.props;
     const mappingName = mapping.sentryName ?? '';
     return hasAccess ? (
@@ -53,6 +58,7 @@ class IntegrationExternalMappings extends Component<Props, State> {
         sentryNamesMapper={sentryNamesMapper}
         onResults={onResults}
         isInline
+        defaultOptions={defaultOptions}
       />
     ) : (
       mappingName

static/app/utils/integrationUtil.tsx

@@ -1,6 +1,7 @@
 import capitalize from 'lodash/capitalize';
 import * as qs from 'query-string';
 
+import {Result} from 'sentry/components/forms/selectAsyncControl';
 import {
   IconBitbucket,
   IconGeneric,
@@ -246,3 +247,8 @@ export const getExternalActorEndpointDetails = (
     apiEndpoint: isValidMapping ? `${baseEndpoint}${mapping.id}/` : baseEndpoint,
   };
 };
+
+export const sentryNameToOption = ({id, name}): Result => ({
+  value: id,
+  label: name,
+});

static/app/views/organizationIntegrations/integrationExternalTeamMappings.tsx

@@ -1,5 +1,6 @@
 import {Fragment} from 'react';
 import {withRouter, WithRouterProps} from 'react-router';
+import uniqBy from 'lodash/uniqBy';
 
 import {addErrorMessage, addSuccessMessage} from 'sentry/actionCreators/indicator';
 import {openModal} from 'sentry/actionCreators/modal';
@@ -8,6 +9,7 @@ import IntegrationExternalMappingForm from 'sentry/components/integrationExterna
 import IntegrationExternalMappings from 'sentry/components/integrationExternalMappings';
 import {t} from 'sentry/locale';
 import {ExternalActorMapping, Integration, Organization, Team} from 'sentry/types';
+import {sentryNameToOption} from 'sentry/utils/integrationUtil';
 import withOrganization from 'sentry/utils/withOrganization';
 
 type Props = AsyncComponent['props'] &
@@ -18,6 +20,7 @@ type Props = AsyncComponent['props'] &
 
 type State = AsyncComponent['state'] & {
   teams: Team[];
+  initialResults: Team[];
   queryResults: {
     // For inline forms, the mappingKey will be the external name (since multiple will be rendered at one time)
     // For the modal form, the mappingKey will be this.modalMappingKey (since only one modal form is rendered at any time)
@@ -30,6 +33,7 @@ class IntegrationExternalTeamMappings extends AsyncComponent<Props, State> {
     return {
       ...super.getDefaultState(),
       teams: [],
+      initialResults: [],
       queryResults: {},
     };
   }
@@ -37,11 +41,14 @@ class IntegrationExternalTeamMappings extends AsyncComponent<Props, State> {
   getEndpoints(): ReturnType<AsyncComponent['getEndpoints']> {
     const {organization, location} = this.props;
     return [
+      // We paginate on this query, since we're filtering by hasExternalTeams:true
       [
         'teams',
         `/organizations/${organization.slug}/teams/`,
         {query: {...location?.query, query: 'hasExternalTeams:true'}},
       ],
+      // We use this query as defaultOptions to reduce identical API calls
+      ['initialResults', `/organizations/${organization.slug}/teams/`],
     ];
   }
 
@@ -86,22 +93,31 @@ class IntegrationExternalTeamMappings extends AsyncComponent<Props, State> {
     return externalTeamMappings.sort((a, b) => parseInt(a.id, 10) - parseInt(b.id, 10));
   }
 
-  modalMappingKey = 'MODAL_RESULTS';
+  modalMappingKey = '__MODAL_RESULTS__';
 
   get dataEndpoint() {
     const {organization} = this.props;
     return `/organizations/${organization.slug}/teams/`;
   }
 
+  get defaultTeamOptions() {
+    const {initialResults} = this.state;
+    return this.sentryNamesMapper(initialResults).map(sentryNameToOption);
+  }
+
   getBaseFormEndpoint(mapping?: ExternalActorMapping) {
     if (!mapping) {
       return '';
     }
     const {organization} = this.props;
-    const {queryResults} = this.state;
-    const mappingResults =
+    const {queryResults, initialResults} = this.state;
+    const fieldResults =
       queryResults[mapping.externalName] ?? queryResults[this.modalMappingKey];
-    const team = mappingResults?.find(item => item.id === mapping.teamId);
+    const team =
+      // First, search for the team in the query results...
+      fieldResults?.find(item => item.id === mapping.teamId) ??
+      // Then in the initial results, if nothing was found.
+      initialResults?.find(item => item.id === mapping.teamId);
     return `/teams/${organization.slug}/${team?.slug ?? ''}/external-teams/`;
   }
 
@@ -109,12 +125,26 @@ class IntegrationExternalTeamMappings extends AsyncComponent<Props, State> {
     return teams.map(({id, slug}) => ({id, name: slug}));
   }
 
+  /**
+   * This method combines the results from searches made on a form dropping repeated entries
+   * that have identical 'id's. This is because we need the result of the the search query when
+   * the user submits to get the team slug, but it won't always be the last query they've made.
+   *
+   * If they search (but not select) after making a selection, and we didn't keep a running collection of results,
+   * we wouldn't have the team to generate the endpoint from.
+   */
+  combineResultsById = (resultList1, resultList2) => {
+    return uniqBy([...resultList1, ...resultList2], 'id');
+  };
+
   handleResults = (results, mappingKey?: string) => {
     if (mappingKey) {
+      const {queryResults} = this.state;
       this.setState({
         queryResults: {
-          ...this.state.queryResults,
-          [mappingKey]: results,
+          ...queryResults,
+          // Ensure we always have a team to pull the slug from
+          [mappingKey]: this.combineResultsById(results, queryResults[mappingKey] ?? []),
         },
       });
     }
@@ -131,6 +161,7 @@ class IntegrationExternalTeamMappings extends AsyncComponent<Props, State> {
             integration={integration}
             dataEndpoint={this.dataEndpoint}
             getBaseFormEndpoint={map => this.getBaseFormEndpoint(map)}
+            defaultOptions={this.defaultTeamOptions}
             mapping={mapping}
             mappingKey={this.modalMappingKey}
             sentryNamesMapper={this.sentryNamesMapper}
@@ -157,6 +188,7 @@ class IntegrationExternalTeamMappings extends AsyncComponent<Props, State> {
         mappings={this.mappings}
         dataEndpoint={this.dataEndpoint}
         getBaseFormEndpoint={mapping => this.getBaseFormEndpoint(mapping)}
+        defaultOptions={this.defaultTeamOptions}
         sentryNamesMapper={this.sentryNamesMapper}
         onCreate={this.openModal}
         onDelete={this.handleDelete}

static/app/views/organizationIntegrations/integrationExternalUserMappings.tsx

@@ -14,6 +14,7 @@ import {
   Member,
   Organization,
 } from 'sentry/types';
+import {sentryNameToOption} from 'sentry/utils/integrationUtil';
 import withOrganization from 'sentry/utils/withOrganization';
 
 type Props = AsyncComponent['props'] &
@@ -24,17 +25,21 @@ type Props = AsyncComponent['props'] &
 
 type State = AsyncComponent['state'] & {
   members: (Member & {externalUsers: ExternalUser[]})[];
+  initialResults: Member[];
 };
 
 class IntegrationExternalUserMappings extends AsyncComponent<Props, State> {
   getEndpoints(): ReturnType<AsyncComponent['getEndpoints']> {
     const {organization} = this.props;
     return [
+      // We paginate on this query, since we're filtering by hasExternalUsers:true
       [
         'members',
         `/organizations/${organization.slug}/members/`,
         {query: {query: 'hasExternalUsers:true', expand: 'externalUsers'}},
       ],
+      // We use this query as defaultOptions to reduce identical API calls
+      ['initialResults', `/organizations/${organization.slug}/members/`],
     ];
   }
 
@@ -88,6 +93,11 @@ class IntegrationExternalUserMappings extends AsyncComponent<Props, State> {
     return `/organizations/${organization.slug}/external-users/`;
   }
 
+  get defaultUserOptions() {
+    const {initialResults} = this.state;
+    return this.sentryNamesMapper(initialResults).map(sentryNameToOption);
+  }
+
   sentryNamesMapper(members: Member[]) {
     return members
       .filter(member => member.user)
@@ -108,6 +118,7 @@ class IntegrationExternalUserMappings extends AsyncComponent<Props, State> {
             integration={integration}
             dataEndpoint={this.dataEndpoint}
             getBaseFormEndpoint={() => this.baseFormEndpoint}
+            defaultOptions={this.defaultUserOptions}
             mapping={mapping}
             sentryNamesMapper={this.sentryNamesMapper}
             onCancel={closeModal}
@@ -133,6 +144,7 @@ class IntegrationExternalUserMappings extends AsyncComponent<Props, State> {
           mappings={this.mappings}
           dataEndpoint={this.dataEndpoint}
           getBaseFormEndpoint={() => this.baseFormEndpoint}
+          defaultOptions={this.defaultUserOptions}
           sentryNamesMapper={this.sentryNamesMapper}
           onCreate={this.openModal}
           onDelete={this.handleDelete}

static/app/views/settings/components/forms/selectAsyncField.tsx

@@ -1,26 +1,38 @@
 import * as React from 'react';
 
-import SelectAsyncControl from 'sentry/components/forms/selectAsyncControl';
+import SelectAsyncControl, {Result} from 'sentry/components/forms/selectAsyncControl';
 import InputField from 'sentry/views/settings/components/forms/inputField';
 
 // projects can be passed as a direct prop as well
 type Props = Omit<InputField['props'], 'highlighted' | 'visible' | 'required'>;
+import {GeneralSelectValue} from 'sentry/components/forms/selectControl';
 
 export type SelectAsyncFieldProps = React.ComponentPropsWithoutRef<
   typeof SelectAsyncControl
 > &
   Props;
 
-class SelectAsyncField extends React.Component<SelectAsyncFieldProps> {
+type SelectAsyncFieldState = {
+  results: Result[];
+  latestSelection?: GeneralSelectValue;
+};
+class SelectAsyncField extends React.Component<
+  SelectAsyncFieldProps,
+  SelectAsyncFieldState
+> {
   state = {
     results: [],
+    latestSelection: undefined,
   };
+
+  componentDidMount() {}
+
   // need to map the option object to the value
   // this is essentially the same code from ./selectField handleChange()
   handleChange = (
     onBlur: Props['onBlur'],
     onChange: Props['onChange'],
-    optionObj: {value: string | any[]},
+    optionObj: GeneralSelectValue,
     event: React.MouseEvent
   ) => {
     let {value} = optionObj;
@@ -32,18 +44,26 @@ class SelectAsyncField extends React.Component<SelectAsyncFieldProps> {
     } else if (!Array.isArray(optionObj)) {
       value = optionObj.value;
     }
+    this.setState({latestSelection: optionObj});
     onChange?.(value, event);
     onBlur?.(value, event);
   };
 
-  findValue(propsValue) {
+  findValue(propsValue: string): GeneralSelectValue {
+    const {defaultOptions} = this.props;
+    const {results, latestSelection} = this.state;
     /**
      * The propsValue is the `id` of the object (user, team, etc), and
      * react-select expects a full value object: {value: "id", label: "name"}
-     *
-     * Returning {} here will show the user a dropdown with "No options".
      **/
-    return this.state.results.find(({value}) => value === propsValue) || {};
+    return (
+      // When rendering the selected value, first look at the API results...
+      results.find(({value}) => value === propsValue) ??
+      // Then at the defaultOptions passed in props...
+      defaultOptions?.find(({value}) => value === propsValue) ??
+      // Then at the latest value selected in the form
+      latestSelection
+    );
   }
 
   render() {
@@ -57,7 +77,15 @@ class SelectAsyncField extends React.Component<SelectAsyncFieldProps> {
             onChange={this.handleChange.bind(this, onBlur, onChange)}
             onResults={data => {
               const results = onResults(data);
-              this.setState({results});
+              const resultSelection = results.find(result => result.value === value);
+              this.setState(
+                resultSelection
+                  ? {
+                      results,
+                      latestSelection: resultSelection,
+                    }
+                  : {results}
+              );
               return results;
             }}
             onSelectResetsInput