Browse Source

ref(pageFilters): Only check desync state on first load (#50110)

We should only check for desynced page filter state when the site first
loads, not on subsequent route changes (e.g. when the user clicks on an
internal link). Internal route changes and whatever updates to page
filter values that they carry are assumed to be intentional and thus
should not trigger a desync state warning.

For example, user clicks on "Open In Discover" from inside Issue
Details. They get to the Discover page with the project and date filters
prefilled with custom values. Neither filter should be marked as
desynced since the difference between the current value and the saved
ones is intentional.
<img width="618" alt="image"
src="https://github.com/getsentry/sentry/assets/44172267/426cb15c-e9ec-4c65-93ee-bddb248614aa">
<img width="1254" alt="image"
src="https://github.com/getsentry/sentry/assets/44172267/792446d5-f623-4471-9139-8b921a41a109">
Vu Luong 1 year ago
parent
commit
8f353a4882

+ 16 - 10
static/app/actionCreators/pageFilters.tsx

@@ -50,10 +50,6 @@ type Options = {
    * Persist changes to the page filter selection into local storage
    */
   save?: boolean;
-  /**
-   * Skip checking desync state after updating the page filter value.
-   */
-  skipDesyncUpdate?: boolean;
 };
 
 /**
@@ -257,8 +253,21 @@ export function initializeUrlState({
   const pinnedFilters = organization.features.includes('new-page-filter')
     ? new Set<PinnedPageFilter>(['projects', 'environments', 'datetime'])
     : storedPageFilters?.pinnedFilters ?? new Set();
+
+  // We should only check and update the desync state if the site has just been loaded
+  // (not counting route changes). To check this, we can use the `isReady` state: if it's
+  // false, then the site was just loaded. Once it's true, `isReady` stays true
+  // through route changes.
+  const shouldCheckDesyncedURLState = !PageFiltersStore.getState().isReady;
+
   PageFiltersStore.onInitializeUrlState(pageFilters, pinnedFilters, shouldPersist);
-  updateDesyncedUrlState(router, shouldForceProject);
+
+  if (shouldCheckDesyncedURLState) {
+    checkDesyncedUrlState(router, shouldForceProject);
+  } else {
+    // Clear desync state on route changes
+    PageFiltersStore.updateDesyncedFilters(new Set());
+  }
 
   const newDatetime = {
     ...datetime,
@@ -307,7 +316,6 @@ export function updateProjects(
   if (options?.environments) {
     persistPageFilters('environments', options);
   }
-  !options?.skipDesyncUpdate && updateDesyncedUrlState(router);
 }
 
 /**
@@ -326,7 +334,6 @@ export function updateEnvironments(
   PageFiltersStore.updateEnvironments(environment);
   updateParams({environment}, router, options);
   persistPageFilters('environments', options);
-  !options?.skipDesyncUpdate && updateDesyncedUrlState(router);
 }
 
 /**
@@ -346,7 +353,6 @@ export function updateDateTime(
   PageFiltersStore.updateDateTime({...selection.datetime, ...datetime});
   updateParams(datetime, router, options);
   persistPageFilters('datetime', options);
-  !options?.skipDesyncUpdate && updateDesyncedUrlState(router);
 }
 
 /**
@@ -423,7 +429,7 @@ async function persistPageFilters(filter: PinnedPageFilter | null, options?: Opt
  * If shouldForceProject is enabled, then we do not record any url desync
  * for the project.
  */
-async function updateDesyncedUrlState(router?: Router, shouldForceProject?: boolean) {
+async function checkDesyncedUrlState(router?: Router, shouldForceProject?: boolean) {
   // Cannot compare URL state without the router
   if (!router || !PageFiltersStore.shouldPersist) {
     return;
@@ -591,5 +597,5 @@ export function revertToPinnedFilters(orgSlug: string, router: InjectedRouter) {
   updateParams(newParams, router, {
     keepCursor: true,
   });
-  updateDesyncedUrlState(router);
+  PageFiltersStore.updateDesyncedFilters(new Set());
 }

+ 1 - 2
static/app/components/charts/chartZoom.tsx

@@ -172,8 +172,7 @@ class ChartZoom extends Component<Props> {
               : startFormatted,
             end: endFormatted ? getUtcToLocalDateObject(endFormatted) : endFormatted,
           },
-          router,
-          {skipDesyncUpdate: true}
+          router
         );
       }
 

+ 10 - 8
static/app/components/organizations/datePageFilter.spec.tsx

@@ -1,7 +1,7 @@
 import {initializeOrg} from 'sentry-test/initializeOrg';
-import {act, fireEvent, render, screen, userEvent} from 'sentry-test/reactTestingLibrary';
+import {fireEvent, render, screen, userEvent} from 'sentry-test/reactTestingLibrary';
 
-import {updateDateTime} from 'sentry/actionCreators/pageFilters';
+import {initializeUrlState} from 'sentry/actionCreators/pageFilters';
 import {DatePageFilter} from 'sentry/components/organizations/datePageFilter';
 import OrganizationStore from 'sentry/stores/organizationStore';
 import PageFiltersStore from 'sentry/stores/pageFiltersStore';
@@ -145,12 +145,14 @@ describe('DatePageFilter', function () {
       },
     });
 
-    // Manually mark the date filter as desynced
-    act(() =>
-      updateDateTime({period: '14d', start: null, end: null, utc: false}, desyncRouter, {
-        save: false,
-      })
-    );
+    PageFiltersStore.reset();
+    initializeUrlState({
+      memberProjects: [],
+      organization: desyncOrganization,
+      queryParams: {statsPeriod: '14d'},
+      router: desyncRouter,
+      shouldEnforceSingleProject: false,
+    });
 
     render(<DatePageFilter />, {
       context: desyncRouterContext,

+ 9 - 3
static/app/components/organizations/environmentPageFilter/index.spec.tsx

@@ -1,7 +1,7 @@
 import {initializeOrg} from 'sentry-test/initializeOrg';
 import {act, fireEvent, render, screen, userEvent} from 'sentry-test/reactTestingLibrary';
 
-import {updateEnvironments} from 'sentry/actionCreators/pageFilters';
+import {initializeUrlState, updateEnvironments} from 'sentry/actionCreators/pageFilters';
 import {EnvironmentPageFilter} from 'sentry/components/organizations/environmentPageFilter';
 import OrganizationStore from 'sentry/stores/organizationStore';
 import PageFiltersStore from 'sentry/stores/pageFiltersStore';
@@ -157,8 +157,14 @@ describe('EnvironmentPageFilter', function () {
       },
     });
 
-    // Manually mark the environment filter as desynced
-    act(() => updateEnvironments(['staging'], desyncRouter, {save: false}));
+    PageFiltersStore.reset();
+    initializeUrlState({
+      memberProjects: [],
+      organization: desyncOrganization,
+      queryParams: {project: ['1'], environment: 'staging'},
+      router: desyncRouter,
+      shouldEnforceSingleProject: false,
+    });
 
     render(<EnvironmentPageFilter />, {
       context: desyncRouterContext,

+ 9 - 3
static/app/components/organizations/projectPageFilter/index.spec.tsx

@@ -8,7 +8,7 @@ import {
   within,
 } from 'sentry-test/reactTestingLibrary';
 
-import {updateProjects} from 'sentry/actionCreators/pageFilters';
+import {initializeUrlState, updateProjects} from 'sentry/actionCreators/pageFilters';
 import {ProjectPageFilter} from 'sentry/components/organizations/projectPageFilter';
 import {ALL_ACCESS_PROJECTS} from 'sentry/constants/pageFilters';
 import OrganizationStore from 'sentry/stores/organizationStore';
@@ -229,8 +229,14 @@ describe('ProjectPageFilter', function () {
       },
     });
 
-    // Manually mark the project filter as desynced
-    act(() => updateProjects([2], desyncRouter, {save: false}));
+    PageFiltersStore.reset();
+    initializeUrlState({
+      memberProjects: [],
+      organization: desyncOrganization,
+      queryParams: {project: '2'},
+      router: desyncRouter,
+      shouldEnforceSingleProject: false,
+    });
 
     render(<ProjectPageFilter />, {
       context: desyncRouterContext,

+ 18 - 0
static/app/stores/pageFiltersStore.tsx

@@ -127,6 +127,12 @@ const storeConfig: PageFiltersStoreDefinition = {
       return;
     }
 
+    if (this.desyncedFilters.has('projects')) {
+      const newDesyncedFilters = new Set(this.desyncedFilters);
+      newDesyncedFilters.delete('projects');
+      this.desyncedFilters = newDesyncedFilters;
+    }
+
     this.selection = {
       ...this.selection,
       projects,
@@ -140,6 +146,12 @@ const storeConfig: PageFiltersStoreDefinition = {
       return;
     }
 
+    if (this.desyncedFilters.has('datetime')) {
+      const newDesyncedFilters = new Set(this.desyncedFilters);
+      newDesyncedFilters.delete('datetime');
+      this.desyncedFilters = newDesyncedFilters;
+    }
+
     this.selection = {
       ...this.selection,
       datetime,
@@ -152,6 +164,12 @@ const storeConfig: PageFiltersStoreDefinition = {
       return;
     }
 
+    if (this.desyncedFilters.has('environments')) {
+      const newDesyncedFilters = new Set(this.desyncedFilters);
+      newDesyncedFilters.delete('environments');
+      this.desyncedFilters = newDesyncedFilters;
+    }
+
     this.selection = {
       ...this.selection,
       environments: environments ?? [],