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

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,
@@ -106,8 +115,7 @@ class GlobalSelectionHeader extends React.Component {
     const hasMultipleProjectFeature = this.hasMultipleProjectSelection();
 
     const stateFromRouter = getStateFromQuery(location.query);
-    // We should update store if there are any relevant URL parameters when component
-    // is mounted
+    // We should update store if there are any relevant URL parameters when component is mounted
     if (Object.values(stateFromRouter).some(i => !!i)) {
       if (!stateFromRouter.start && !stateFromRouter.end && !stateFromRouter.period) {
         stateFromRouter.period = DEFAULT_STATS_PERIOD;
@@ -163,7 +171,7 @@ class GlobalSelectionHeader extends React.Component {
     }
 
     // Update if URL parameters change
-    if (this.didQueryChange(this.props, nextProps)) {
+    if (this.changedQueryKeys(this.props, nextProps).length > 0) {
       return true;
     }
 
@@ -187,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 &&
@@ -199,14 +207,39 @@ class GlobalSelectionHeader extends React.Component {
       return true;
     }
 
+    // Update if `forceUrlSync` changes
+    if (!this.props.forceUrlSync && nextProps.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) {
+      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);
@@ -216,35 +249,56 @@ class GlobalSelectionHeader extends React.Component {
     return new Set(this.props.organization.features).has('global-views');
   };
 
-  didQueryChange = (prevProps, nextProps) => {
+  /**
+   * Identifies the query params (that are relevant to this component) that have changed
+   *
+   * @return {String[]} Returns an array of param keys that have changed
+   */
+  changedQueryKeys = (prevProps, nextProps) => {
     const urlParamKeys = Object.values(URL_PARAM);
     const prevQuery = pick(prevProps.location.query, urlParamKeys);
     const nextQuery = pick(nextProps.location.query, urlParamKeys);
 
     // If no next query is specified keep the previous global selection values
     if (Object.keys(prevQuery).length === 0 && Object.keys(nextQuery).length === 0) {
-      return false;
+      return [];
     }
 
-    return !isEqual(prevQuery, nextQuery);
+    const changedKeys = Object.values(urlParamKeys).filter(
+      key => !isEqual(prevQuery[key], nextQuery[key])
+    );
+
+    return changedKeys;
   };
 
   updateStoreIfChange = (prevProps, nextProps) => {
     // Don't do anything if query parameters have not changed
     //
     // e.g. if selection store changed, don't trigger more actions
     // to update global selection store (otherwise we'll get recursive updates)
-    if (!this.didQueryChange(prevProps, nextProps)) {
+    const changedKeys = this.changedQueryKeys(prevProps, nextProps);
+
+    if (!changedKeys.length) {
       return;
     }
 
     const {project, environment, period, start, end, utc} = getStateFromQuery(
       nextProps.location.query
     );
 
-    updateDateTime({start, end, period, utc});
-    updateEnvironments(environment || []);
-    updateProjects(project || []);
+    if (changedKeys.includes(URL_PARAM.PROJECT)) {
+      updateProjects(project || []);
+    }
+    if (changedKeys.includes(URL_PARAM.ENVIRONMENT)) {
+      updateEnvironments(environment || []);
+    }
+    if (
+      [URL_PARAM.START, URL_PARAM.END, URL_PARAM.UTC, URL_PARAM.PERIOD].find(key =>
+        changedKeys.includes(key)
+      )
+    ) {
+      updateDateTime({start, end, period, utc});
+    }
   };
 
   // Returns `router` from props if `hasCustomRouting` property is false

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}
       />
     );