Browse Source

fix(discover-homepage): refresh loses changes (#40189)

Fixes a bug where the eventView wasn't being parsed from the URL,
so if a user made changes and refreshed the page, the changes would be
dropped. This also meant that sharing a link wasn't possible.

This reverts the code that ignores the query params and instead moves
the redirect logic up into the HomepageQueryAPI component where we can
detect when the fetch finishes and trigger a redirect if there's a
saved query and we don't already have a valid one in the URL.

Co-authored-by: Edward Gou <edward.gou@sentry.io>
Nar Saynorath 2 years ago
parent
commit
31b9e5da40

+ 33 - 0
static/app/views/eventsV2/homepage.spec.tsx

@@ -105,6 +105,39 @@ describe('Discover > Homepage', () => {
     screen.getByText('event.type:error');
   });
 
+  it('renders event view from URL params over homepage query', async () => {
+    initialData = initializeOrg({
+      ...initializeOrg(),
+      organization,
+      router: {
+        location: {
+          ...TestStubs.location(),
+          query: {
+            ...EventView.fromSavedQuery(DEFAULT_EVENT_VIEW).generateQueryStringObject(),
+            field: ['project'],
+          },
+        },
+      },
+    });
+
+    render(
+      <Homepage
+        organization={organization}
+        location={initialData.router.location}
+        router={initialData.router}
+        setSavedQuery={jest.fn()}
+        loading={false}
+      />,
+      {context: initialData.routerContext, organization: initialData.organization}
+    );
+
+    expect(mockHomepage).toHaveBeenCalled();
+    await screen.findByText('project');
+
+    // This is the field in the mocked response for the homepage
+    expect(screen.queryByText('environment')).not.toBeInTheDocument();
+  });
+
   it('applies URL changes with the homepage pathname', async () => {
     render(
       <Homepage

+ 24 - 15
static/app/views/eventsV2/homepage.tsx

@@ -1,10 +1,11 @@
-import {InjectedRouter} from 'react-router';
+import {browserHistory, InjectedRouter} from 'react-router';
 import {Location} from 'history';
 
 import {Client} from 'sentry/api';
 import AsyncComponent from 'sentry/components/asyncComponent';
 import PageFiltersContainer from 'sentry/components/organizations/pageFilters/container';
 import {Organization, PageFilters, SavedQuery} from 'sentry/types';
+import EventView from 'sentry/utils/discover/eventView';
 import withApi from 'sentry/utils/withApi';
 import withOrganization from 'sentry/utils/withOrganization';
 import withPageFilters from 'sentry/utils/withPageFilters';
@@ -19,20 +20,36 @@ type Props = {
   router: InjectedRouter;
   selection: PageFilters;
   setSavedQuery: (savedQuery: SavedQuery) => void;
-  homepageQuery?: SavedQuery;
-  savedQuery?: SavedQuery;
 };
 
 type HomepageQueryState = AsyncComponent['state'] & {
-  key: number;
   savedQuery?: SavedQuery | null;
 };
 
 class HomepageQueryAPI extends AsyncComponent<Props, HomepageQueryState> {
   shouldReload = true;
 
+  componentDidUpdate(prevProps, prevState) {
+    const hasFetchedSavedQuery = !prevState.savedQuery && this.state.savedQuery;
+    const hasInitiallyLoaded = prevState.loading && !this.state.loading;
+    const sidebarClicked = this.state.savedQuery && this.props.location.search === '';
+    const hasValidEventViewInURL = EventView.fromLocation(this.props.location).isValid();
+
+    if (
+      this.state.savedQuery &&
+      ((hasInitiallyLoaded && hasFetchedSavedQuery && !hasValidEventViewInURL) ||
+        sidebarClicked)
+    ) {
+      const eventView = EventView.fromSavedQuery(this.state.savedQuery);
+      browserHistory.replace(
+        eventView.getResultsViewUrlTarget(this.props.organization.slug, true)
+      );
+    }
+    super.componentDidUpdate(prevProps, prevState);
+  }
+
   getEndpoints(): ReturnType<AsyncComponent['getEndpoints']> {
-    const {organization, location} = this.props;
+    const {organization} = this.props;
 
     const endpoints: ReturnType<AsyncComponent['getEndpoints']> = [];
     if (
@@ -44,12 +61,6 @@ class HomepageQueryAPI extends AsyncComponent<Props, HomepageQueryState> {
         `/organizations/${organization.slug}/discover/homepage/`,
       ]);
     }
-    // HACK: We're using state here to manage a component key so we can force remounting the entire discover result
-    // This is because we need <Results> to rerun its constructor with the new homepage query to get it to display properly
-    // We're checking to see that location.search is empty because that is the only time we should be fetching the homepage query
-    if (location.search === '' && this.state) {
-      this.setState({key: Date.now()});
-    }
     return endpoints;
   }
 
@@ -73,7 +84,6 @@ class HomepageQueryAPI extends AsyncComponent<Props, HomepageQueryState> {
         loading={loading}
         setSavedQuery={this.setSavedQuery}
         isHomepage
-        key={`results-${this.state.key}`}
       />
     );
   }
@@ -82,9 +92,8 @@ class HomepageQueryAPI extends AsyncComponent<Props, HomepageQueryState> {
 function HomepageContainer(props: Props) {
   return (
     <PageFiltersContainer
-      skipLoadLastUsed={
-        props.organization.features.includes('global-views') && !!props.savedQuery
-      }
+      skipLoadLastUsed={props.organization.features.includes('global-views')}
+      skipInitializeUrlParams
     >
       <HomepageQueryAPI {...props} />
     </PageFiltersContainer>

+ 9 - 19
static/app/views/eventsV2/results.tsx

@@ -104,28 +104,18 @@ function getYAxis(location: Location, eventView: EventView, savedQuery?: SavedQu
 
 export class Results extends Component<Props, State> {
   static getDerivedStateFromProps(nextProps: Readonly<Props>, prevState: State): State {
-    if (
-      !nextProps.isHomepage ||
-      prevState.savedQuery ||
-      nextProps.savedQuery === undefined // When user clicks on Discover in sidebar
-    ) {
-      const eventView = EventView.fromSavedQueryOrLocation(
-        nextProps.savedQuery,
-        nextProps.location
-      );
-      return {...prevState, eventView, savedQuery: nextProps.savedQuery};
-    }
-
-    return prevState;
+    const eventView = EventView.fromSavedQueryOrLocation(
+      nextProps.savedQuery,
+      nextProps.location
+    );
+    return {...prevState, eventView, savedQuery: nextProps.savedQuery};
   }
 
   state: State = {
-    // If this is the homepage, force an invalid eventView so we can handle
-    // the redirect first. This can't rely on the location because the
-    // location may have a valid eventView configuration
-    eventView: this.props.isHomepage
-      ? EventView.fromSavedQuery({...DEFAULT_EVENT_VIEW, fields: []})
-      : EventView.fromSavedQueryOrLocation(this.props.savedQuery, this.props.location),
+    eventView: EventView.fromSavedQueryOrLocation(
+      this.props.savedQuery,
+      this.props.location
+    ),
     error: '',
     errorCode: 200,
     totalValues: null,

+ 3 - 0
static/app/views/eventsV2/resultsHeader.tsx

@@ -150,6 +150,9 @@ class ResultsHeader extends Component<Props, State> {
             isHomepage={isHomepage}
             setHomepageQuery={updatedHomepageQuery => {
               this.setState({homepageQuery: updatedHomepageQuery});
+              if (isHomepage) {
+                setSavedQuery(updatedHomepageQuery);
+              }
             }}
             homepageQuery={homepageQuery}
           />