feat(ui): Do not load Global Selection values from local storage in Issue Details

This is for the case where an Issue is accessed from a direct URL. Previously it would load the last accessed project/env from local storage. This is not desired behavior when accessing an Issue directly. This will look for a route to Issue details and avoid loading the store.

However we also want to initialize the store if we leave the issue details page. Otherwise (this is exacerbated in the single project case), you could have last accessed project `Foo`, open url to issue in `Bar`, and then navigating from Issue to Issues list could result in a project that is neither `Foo` nor `Bar`.

Fixes SEN-709
Fixes SEN-658

Author: billyvg

src/sentry/static/sentry/app/components/organizations/globalSelectionHeader/index.jsx

@@ -17,6 +17,7 @@ import {
   updateProjects,
 } from 'app/actionCreators/globalSelection';
 import BackToIssues from 'app/components/organizations/backToIssues';
+import ConfigStore from 'app/stores/configStore';
 import Header from 'app/components/organizations/header';
 import HeaderItemPosition from 'app/components/organizations/headerItemPosition';
 import HeaderSeparator from 'app/components/organizations/headerSeparator';
@@ -26,10 +27,9 @@ import MultipleProjectSelector from 'app/components/organizations/multipleProjec
 import SentryTypes from 'app/sentryTypes';
 import TimeRangeSelector from 'app/components/organizations/timeRangeSelector';
 import Tooltip from 'app/components/tooltip';
+import space from 'app/styles/space';
 import withGlobalSelection from 'app/utils/withGlobalSelection';
-import ConfigStore from 'app/stores/configStore';
 import withProjects from 'app/utils/withProjects';
-import space from 'app/styles/space';
 
 import {getStateFromQuery} from './utils';
 
@@ -75,6 +75,15 @@ class GlobalSelectionHeader extends React.Component {
      */
     showRelative: PropTypes.bool,
 
+    // 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: PropTypes.bool,
+
     // Callbacks //
     onChangeProjects: PropTypes.func,
     onUpdateProjects: PropTypes.func,
@@ -186,7 +195,7 @@ class GlobalSelectionHeader extends React.Component {
       return true;
     }
 
-    //update if any projects are starred or reordered
+    // update if any projects are starred or reordered
     if (
       this.props.projects &&
       nextProps.projects &&
@@ -198,14 +207,39 @@ class GlobalSelectionHeader extends React.Component {
       return true;
     }
 
+    // Update if `forceUrlSync` changes
+    if (this.props.forceUrlSync !== nextProps.forceUrlSync && !this.props.forceUrlSync) {
+      return true;
+    }
+
     return false;
   }
 
   componentDidUpdate(prevProps) {
-    if (this.props.hasCustomRouting) {
+    const {hasCustomRouting, location, forceUrlSync, selection} = this.props;
+
+    if (hasCustomRouting) {
       return;
     }
 
+    // Kind of gross
+    if (forceUrlSync !== prevProps.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/routes.jsx

@@ -954,6 +954,8 @@ function routes() {
             }
             component={errorHandler(LazyLoad)}
           >
+            {/* XXX: if we change the path for group details, we *must* update `OrganizationContext`.
+            There is behavior that depends on this path and unfortunately no great way to test for this contract */}
             <IndexRoute
               componentPromise={() =>
                 import(/* webpackChunkName: "OrganizationGroupEventDetails" */ './views/organizationGroupDetails/groupEventDetails')

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

@@ -49,14 +49,21 @@ const GlobalSelectionStore = Reflux.createStore({
   },
 
   reset(state) {
+    this._hasLoaded = false;
     this.selection = state || getDefaultSelection();
   },
 
   /**
    * Initializes the global selection store
    * If there are query params apply these, otherwise check local storage
    */
-  loadInitialData(organization, queryParams) {
+  loadInitialData(organization, queryParams, {forceUrlSync, onlyIfNeverLoaded} = {}) {
+    // If this option is true, only load if it has never been loaded before
+    if (onlyIfNeverLoaded && this._hasLoaded) {
+      return;
+    }
+
+    this._hasLoaded = true;
     this.organization = organization;
     const query = pick(queryParams, Object.values(URL_PARAM));
     const hasQuery = Object.keys(query).length > 0;
@@ -87,12 +94,16 @@ const GlobalSelectionStore = Reflux.createStore({
           globalSelection = {datetime: defaultDateTime, ...JSON.parse(storedValue)};
         }
       } catch (ex) {
+        console.error(ex); // eslint-disable-line no-console
         // use default if invalid
       }
     }
 
     if (isValidSelection(globalSelection, organization)) {
-      this.selection = globalSelection;
+      this.selection = {
+        ...globalSelection,
+        ...(forceUrlSync ? {forceUrlSync: true} : {}),
+      };
       this.trigger(this.selection);
     }
   },

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

@@ -2,8 +2,8 @@ import React from 'react';
 import Reflux from 'reflux';
 import createReactClass from 'create-react-class';
 
-import getDisplayName from 'app/utils/getDisplayName';
 import GlobalSelectionStore from 'app/stores/globalSelectionStore';
+import getDisplayName from 'app/utils/getDisplayName';
 
 /**
  * Higher order component that uses GlobalSelectionStore and provides the
@@ -19,13 +19,33 @@ const withGlobalSelection = WrappedComponent =>
       };
     },
 
+    componentDidMount() {
+      this.updateSelection();
+    },
+
     onUpdate() {
-      this.setState({
-        selection: GlobalSelectionStore.get(),
-      });
+      this.updateSelection();
     },
+
+    updateSelection() {
+      const selection = GlobalSelectionStore.get();
+
+      if (this.state.selection !== selection) {
+        this.setState({
+          selection,
+        });
+      }
+    },
+
     render() {
-      return <WrappedComponent selection={this.state.selection} {...this.props} />;
+      const {forceUrlSync, ...selection} = this.state.selection;
+      return (
+        <WrappedComponent
+          forceUrlSync={forceUrlSync}
+          selection={selection}
+          {...this.props}
+        />
+      );
     },
   });
 

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

@@ -34,6 +34,7 @@ const OrganizationContext = createReactClass({
 
   propTypes: {
     api: PropTypes.object,
+    routes: PropTypes.arrayOf(PropTypes.object),
     includeSidebar: PropTypes.bool,
     useLastOrganization: PropTypes.bool,
     organizationsLoading: PropTypes.bool,
@@ -136,7 +137,17 @@ const OrganizationContext = createReactClass({
 
         TeamStore.loadInitialData(data.teams);
         ProjectsStore.loadInitialData(data.projects);
-        GlobalSelectionStore.loadInitialData(data, this.props.location.query);
+
+        // 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(data, this.props.location.query);
+        }
         OrganizationEnvironmentsStore.loadInitialData(environments);
 
         this.setState({

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

@@ -1,25 +1,26 @@
-import React from 'react';
-import PropTypes from 'prop-types';
-import {isEqual} from 'lodash';
 import {browserHistory} from 'react-router';
+import {isEqual} from 'lodash';
+import PropTypes from 'prop-types';
+import React from 'react';
 
-import SentryTypes from 'app/sentryTypes';
+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';
-import ResolutionBox from 'app/components/resolutionBox';
 import MutedBox from 'app/components/mutedBox';
+import OrganizationEnvironmentsStore from 'app/stores/organizationEnvironmentsStore';
+import ResolutionBox from 'app/components/resolutionBox';
+import SentryTypes from 'app/sentryTypes';
+import fetchSentryAppInstallations from 'app/utils/fetchSentryAppInstallations';
 import withApi from 'app/utils/withApi';
-import withOrganization from 'app/utils/withOrganization';
 import withGlobalSelection from 'app/utils/withGlobalSelection';
-import fetchSentryAppInstallations from 'app/utils/fetchSentryAppInstallations';
-import {fetchSentryAppComponents} from 'app/actionCreators/sentryAppComponents';
-import OrganizationEnvironmentsStore from 'app/stores/organizationEnvironmentsStore';
+import withOrganization from 'app/utils/withOrganization';
 
-import GroupEventToolbar from './eventToolbar';
 import {fetchGroupEventAndMarkSeen, getEventEnvironment} from './utils';
+import GroupEventToolbar from './eventToolbar';
 
 class GroupEventDetails extends React.Component {
   static propTypes = {
@@ -77,7 +78,24 @@ class GroupEventDetails extends React.Component {
   }
 
   componentWillUnmount() {
-    this.props.api.clear();
+    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
+    GlobalSelectionStore.loadInitialData(organization, this.props.location.query, {
+      onlyIfNeverLoaded: true,
+      forceUrlSync: true,
+    });
+
+    api.clear();
   }
 
   fetchData = () => {

src/sentry/static/sentry/app/views/organizationGroupDetails/index.jsx

@@ -21,14 +21,13 @@ class OrganizationGroupDetails extends React.Component {
   }
 
   render() {
-    // eslint-disable-next-line no-unused-vars
     const {selection, ...props} = this.props;
 
     return (
       <GroupDetails
         environments={selection.environments}
-        enableSnuba={true}
-        showGlobalHeader={true}
+        enableSnuba
+        showGlobalHeader
         {...props}
       />
     );