feat(ui): Semi-fix loading initial data on issue details (#18419)

This basically re-implements #13689 in a more simple way. This is possible due to other changes that we have done to issue details (#13875, #14731).

This also (semi) fixes an issue with loading Issue Details with an environment in the URL. Previously, it would fetch details API serially: 1) without env and 2) with env.
There would be a slight flicker between loading -> finished req #1 -> loading -> finished req #2. Now this seems to fire both at near the same time and cancels the initial request almost immediately.

This is still not ideal but is an interim-fix as the ideal solution is a bit more involved, but will be on its way.

The tests introduced in #18452 should cover the changes in this PR.

Author: billyvg

src/sentry/static/sentry/app/components/organizations/globalSelectionHeader/globalSelectionHeader.tsx

@@ -111,17 +111,6 @@ type Props = {
    */
   forceProject?: MinimalProject | null;
 
-  /**
-   * GlobalSelectionStore is not always initialized (e.g. Group Details) before this is rendered
-   *
-   * This component intentionally attempts to sync store --> URL Parameter
-   * only when mounted, except when this prop changes.
-   *
-   * XXX: This comes from GlobalSelectionStore and currently does not reset,
-   * so it happens at most once. Can add a reset as needed.
-   */
-  forceUrlSync: boolean;
-
   /// Props passed to child components ///
 
   /**
@@ -198,7 +187,6 @@ class GlobalSelectionHeader extends React.Component<Props, State> {
     showDateSelector: PropTypes.bool,
     hasCustomRouting: PropTypes.bool,
     resetParamsOnChange: PropTypes.arrayOf(PropTypes.string),
-    forceUrlSync: PropTypes.bool,
     showAbsolute: PropTypes.bool,
     showRelative: PropTypes.bool,
     allowClearTimeRange: PropTypes.bool,
@@ -337,11 +325,6 @@ class GlobalSelectionHeader extends React.Component<Props, State> {
       return true;
     }
 
-    // Update if `forceUrlSync` changes
-    if (!this.props.forceUrlSync && nextProps.forceUrlSync) {
-      return true;
-    }
-
     if (this.props.organization !== nextProps.organization) {
       return true;
     }
@@ -350,14 +333,7 @@ class GlobalSelectionHeader extends React.Component<Props, State> {
   }
 
   componentDidUpdate(prevProps) {
-    const {
-      hasCustomRouting,
-      location,
-      selection,
-      forceUrlSync,
-      forceProject,
-      shouldForceProject,
-    } = this.props;
+    const {hasCustomRouting, location, forceProject, shouldForceProject} = this.props;
 
     if (hasCustomRouting) {
       return;
@@ -393,23 +369,6 @@ class GlobalSelectionHeader extends React.Component<Props, State> {
       }
     }
 
-    if (forceUrlSync && !prevProps.forceUrlSync) {
-      const {project, environment} = getStateFromQuery(location.query);
-
-      if (
-        !isEqual(project, selection.projects) ||
-        !isEqual(environment, selection.environments)
-      ) {
-        updateParamsWithoutHistory(
-          {
-            project: selection.projects,
-            environment: selection.environments,
-          },
-          this.getRouter()
-        );
-      }
-    }
-
     // If component has updated (e.g. due to re-render from a router action),
     // update store values with values from router. Router should be source of truth
     this.updateStoreIfChange(prevProps, this.props);

src/sentry/static/sentry/app/stores/globalSelectionStore.jsx

@@ -84,16 +84,7 @@ const GlobalSelectionStore = Reflux.createStore({
    * Initializes the global selection store
    * If there are query params apply these, otherwise check local storage
    */
-  loadInitialData(
-    organization,
-    queryParams,
-    {api, forceUrlSync, onlyIfNeverLoaded} = {}
-  ) {
-    // If this option is true, only load if it has never been loaded before
-    if (onlyIfNeverLoaded && this._hasLoaded) {
-      return;
-    }
-
+  loadInitialData(organization, queryParams, {api, skipLastUsed} = {}) {
     this._hasLoaded = true;
     this.organization = organization;
     const query = pick(queryParams, Object.values(URL_PARAM));
@@ -113,7 +104,7 @@ const GlobalSelectionStore = Reflux.createStore({
           [DATE_TIME.UTC]: parsed.utc || null,
         },
       };
-    } else {
+    } else if (!skipLastUsed) {
       try {
         const localStorageKey = `${LOCAL_STORAGE_KEY}:${organization.slug}`;
 
@@ -129,15 +120,12 @@ const GlobalSelectionStore = Reflux.createStore({
         // use default if invalid
       }
     }
-    this.loadSelectionIfValid(globalSelection, organization, forceUrlSync, api);
+    this.loadSelectionIfValid(globalSelection, organization, api);
   },
 
-  async loadSelectionIfValid(globalSelection, organization, forceUrlSync, api) {
+  async loadSelectionIfValid(globalSelection, organization, api) {
     if (await isValidSelection(globalSelection, organization, api)) {
-      this.selection = {
-        ...globalSelection,
-        ...(forceUrlSync ? {forceUrlSync: true} : {}),
-      };
+      this.selection = globalSelection;
       this.trigger(this.selection);
     }
   },

src/sentry/static/sentry/app/types/index.tsx

@@ -448,7 +448,6 @@ export type DocumentIntegration = {
 export type GlobalSelection = {
   projects: number[];
   environments: string[];
-  forceUrlSync?: boolean;
   datetime: {
     start: Date | null;
     end: Date | null;

src/sentry/static/sentry/app/utils/withGlobalSelection.tsx

@@ -7,7 +7,6 @@ import getDisplayName from 'app/utils/getDisplayName';
 import {GlobalSelection} from 'app/types';
 
 type InjectedGlobalSelectionProps = {
-  forceUrlSync?: boolean;
   selection: GlobalSelection;
 };
 
@@ -52,10 +51,9 @@ const withGlobalSelection = <P extends InjectedGlobalSelectionProps>(
     },
 
     render() {
-      const {forceUrlSync, ...selection} = this.state.selection;
+      const {selection} = this.state;
       return (
         <WrappedComponent
-          forceUrlSync={!!forceUrlSync}
           selection={selection as GlobalSelection}
           {...(this.props as P)}
         />

src/sentry/static/sentry/app/views/organizationContext.jsx

@@ -205,15 +205,13 @@ const OrganizationContext = createReactClass({
       // Make an exception for issue details in the case where it is accessed directly (e.g. from email)
       // We do not want to load the user's last used env/project in this case, otherwise will
       // lead to very confusing behavior.
-      if (
-        !this.props.routes.find(
-          ({path}) => path && path.includes('/organizations/:orgId/issues/:groupId/')
-        )
-      ) {
-        GlobalSelectionStore.loadInitialData(organization, this.props.location.query, {
-          api: this.props.api,
-        });
-      }
+      const skipLastUsed = !!this.props.routes.find(
+        ({path}) => path && path.includes('/organizations/:orgId/issues/:groupId/')
+      );
+      GlobalSelectionStore.loadInitialData(organization, this.props.location.query, {
+        skipLastUsed,
+        api: this.props.api,
+      });
     } else if (error) {
       // If user is superuser, open sudo window
       const user = ConfigStore.get('user');

src/sentry/static/sentry/app/views/organizationGroupDetails/groupEventDetails/groupEventDetails.tsx

@@ -9,7 +9,6 @@ import {Client} from 'app/api';
 import {fetchSentryAppComponents} from 'app/actionCreators/sentryAppComponents';
 import {withMeta} from 'app/components/events/meta/metaProxy';
 import EventEntries from 'app/components/events/eventEntries';
-import GlobalSelectionStore from 'app/stores/globalSelectionStore';
 import GroupEventDetailsLoadingError from 'app/components/errors/groupEventDetailsLoadingError';
 import GroupSidebar from 'app/components/group/sidebar';
 import LoadingIndicator from 'app/components/loadingIndicator';
@@ -98,26 +97,7 @@ class GroupEventDetails extends React.Component<Props, State> {
   }
 
   componentWillUnmount() {
-    const {api, organization} = this.props;
-
-    // Note: We do not load global selection store with any data when this component is used
-    // This is handled in `<OrganizationContext>` by examining the routes.
-    //
-    // When this view gets unmounted, attempt to load initial data so that projects/envs
-    // gets loaded with the last used one (via local storage). `forceUrlSync` will make
-    // `<GlobalSelectionHeader>` sync values from store to the URL (if they are different),
-    // otherwise they can out of sync because the component only syncs in `DidMount`, and
-    // the timing for that is not guaranteed.
-    //
-    // TBD: if this behavior is actually desired
-    if (organization.projects) {
-      GlobalSelectionStore.loadInitialData(organization, this.props.location.query, {
-        api: this.props.api,
-        onlyIfNeverLoaded: true,
-        forceUrlSync: true,
-      });
-    }
-
+    const {api} = this.props;
     api.clear();
   }
 

tests/js/spec/components/organizations/globalSelectionHeader.spec.jsx

@@ -301,31 +301,6 @@ describe('GlobalSelectionHeader', function() {
     expect(checkboxes.text()).toBe('staging');
   });
 
-  it('updates URL to match GlobalSelection store when re-rendered with `forceUrlSync` prop', async function() {
-    const wrapper = mountWithTheme(
-      <GlobalSelectionHeader router={router} organization={organization} />,
-      routerContext
-    );
-
-    await tick();
-    wrapper.update();
-
-    // Force load, will load from mocked localStorage
-    GlobalSelectionStore.loadInitialData(organization, {}, {forceUrlSync: true});
-
-    await tick();
-    wrapper.update();
-
-    expect(router.replace).toHaveBeenCalledWith(
-      expect.objectContaining({
-        query: {
-          environment: ['staging'],
-          project: [3],
-        },
-      })
-    );
-  });
-
   it('updates GlobalSelection store with default period', async function() {
     mountWithTheme(
       <GlobalSelectionHeader organization={organization} />,

tests/js/spec/views/organizationContext.spec.jsx

@@ -89,7 +89,11 @@ describe('OrganizationContext', function() {
       true,
       true
     );
-    expect(GlobalSelectionStore.loadInitialData).toHaveBeenCalledWith(org, {}, {api});
+    expect(GlobalSelectionStore.loadInitialData).toHaveBeenCalledWith(
+      org,
+      {},
+      {api, skipLastUsed: false}
+    );
   });
 
   it('fetches new org when router params change', async function() {
@@ -259,7 +263,7 @@ describe('OrganizationContext', function() {
     expect(getOrgMock).toHaveBeenCalledTimes(1);
   });
 
-  it('does not call `GlobalSelectionStore.loadInitialData` on group details route', async function() {
+  it('calls `GlobalSelectionStore.loadInitialData` with `skipLastUsed` option when loadigno group details route', async function() {
     expect(GlobalSelectionStore.loadInitialData).not.toHaveBeenCalled();
     wrapper = createWrapper({
       routes: [{path: '/organizations/:orgId/issues/:groupId/'}],
@@ -273,6 +277,10 @@ describe('OrganizationContext', function() {
     expect(wrapper.state('loading')).toBe(false);
     expect(wrapper.state('error')).toBe(null);
 
-    expect(GlobalSelectionStore.loadInitialData).not.toHaveBeenCalled();
+    expect(GlobalSelectionStore.loadInitialData).toHaveBeenCalledWith(
+      org,
+      {},
+      {api, skipLastUsed: true}
+    );
   });
 });