Просмотр исходного кода

ref(pageFilters): Add ability to disable persistence (#49048)

Add a `disablePersistence` prop to `<PageFiltersContainer />` that
prevents the page filters from commit value changes to local storage.
This is useful for local filtering contexts like Dashboard Details and
Project Details.

## Dashboards Details
**Before ——** 
Project filter is set to "All Projects". Navigate to Dashboard Details,
change the filter's value, and the new value leaks out to the global
filtering context.


https://github.com/getsentry/sentry/assets/44172267/a878dff7-3011-4aec-af1e-4fe6f33a91f1

**After ——**
The new value doesn't leak. Navigate outside Dashboard Details and the
filter's value reverts back to "All Projects".


https://github.com/getsentry/sentry/assets/44172267/862652be-02a0-48eb-8e10-3b9102a3ad99

## Project Details
**Before ——** 
There is sometimes a desync message when there shouldn't be. This seems
to happen when the project filter value in local storage is `[-1]` ("All
Projects").

<img width="1728" alt="image"
src="https://github.com/getsentry/sentry/assets/44172267/3b437ae3-cb74-4835-b8c0-6c585145316a">


**After ——**
The desync message no longer sporadically appears.
<img width="1728" alt="image"
src="https://github.com/getsentry/sentry/assets/44172267/03074cc0-bc90-429f-bdaa-1fc240426e28">
Vu Luong 1 год назад
Родитель
Сommit
206b7b0051

+ 55 - 5
static/app/actionCreators/pageFilters.spec.jsx

@@ -1,8 +1,11 @@
+import {act} from 'sentry-test/reactTestingLibrary';
+
 import {
   initializeUrlState,
   revertToPinnedFilters,
   updateDateTime,
   updateEnvironments,
+  updatePersistence,
   updateProjects,
 } from 'sentry/actionCreators/pageFilters';
 import * as PageFilterPersistence from 'sentry/components/organizations/pageFilters/persistence';
@@ -60,7 +63,8 @@ describe('PageFilters ActionCreators', function () {
           environments: [],
           projects: [1],
         }),
-        new Set(['projects', 'environments'])
+        new Set(['projects', 'environments']),
+        true
       );
       expect(router.replace).toHaveBeenCalledWith(
         expect.objectContaining({
@@ -77,14 +81,57 @@ describe('PageFilters ActionCreators', function () {
       initializeUrlState({
         organization,
         queryParams: {},
-        pathname: '/mock-pathname/',
         skipLoadLastUsed: true,
+        pathname: '/mock-pathname/',
         router,
       });
 
       expect(localStorage.getItem).not.toHaveBeenCalled();
     });
 
+    it('does not update local storage (persist) when `shouldPersist` is false', async function () {
+      localStorage.setItem.mockClear();
+      localStorage.getItem.mockReturnValueOnce(
+        JSON.stringify({
+          environments: [],
+          projects: [],
+          pinnedFilters: ['projects'],
+        })
+      );
+
+      initializeUrlState({
+        organization,
+        queryParams: {},
+        shouldPersist: false,
+        router,
+      });
+
+      expect(PageFiltersStore.onInitializeUrlState).toHaveBeenCalledWith(
+        expect.objectContaining({
+          environments: [],
+          projects: [],
+        }),
+        new Set(['projects']),
+        false
+      );
+
+      // `onInitializeUrlState` is being spied on, so PageFiltersStore wasn't actually
+      // updated. We need to call `updatePersistence` manually.
+      updatePersistence(false);
+
+      await act(async () => {
+        // Filters shouldn't persist even when `save` is true
+        updateProjects([1], router, {save: true});
+
+        // Page filter values are asynchronously persisted to local storage after a tick,
+        // so we need to wait before checking for commits to local storage
+        await tick();
+      });
+
+      // New value wasn't committed to local storage
+      expect(localStorage.setItem).not.toHaveBeenCalled();
+    });
+
     it('does not change dates with no query params or defaultSelection', function () {
       initializeUrlState({
         organization,
@@ -103,7 +150,8 @@ describe('PageFilters ActionCreators', function () {
             utc: null,
           },
         }),
-        new Set()
+        new Set(),
+        true
       );
     });
 
@@ -130,7 +178,8 @@ describe('PageFilters ActionCreators', function () {
             utc: null,
           },
         }),
-        new Set()
+        new Set(),
+        true
       );
     });
 
@@ -211,7 +260,8 @@ describe('PageFilters ActionCreators', function () {
           projects: [1],
           environments: [],
         },
-        new Set()
+        new Set(),
+        true
       );
       expect(router.replace).toHaveBeenCalledWith(
         expect.objectContaining({

+ 17 - 3
static/app/actionCreators/pageFilters.tsx

@@ -122,6 +122,12 @@ export type InitializeUrlStateParams = {
   defaultSelection?: Partial<PageFilters>;
   forceProject?: MinimalProject | null;
   shouldForceProject?: boolean;
+  /**
+   * Whether to save changes to local storage. This setting should be page-specific:
+   * most pages should have it on (default) and some, like Dashboard Details, need it
+   * off.
+   */
+  shouldPersist?: boolean;
   showAbsolute?: boolean;
   /**
    * When used with shouldForceProject it will not persist the project id
@@ -145,6 +151,7 @@ export function initializeUrlState({
   router,
   memberProjects,
   skipLoadLastUsed,
+  shouldPersist = true,
   shouldForceProject,
   shouldEnforceSingleProject,
   defaultSelection,
@@ -248,7 +255,7 @@ export function initializeUrlState({
   }
 
   const pinnedFilters = storedPageFilters?.pinnedFilters ?? new Set();
-  PageFiltersStore.onInitializeUrlState(pageFilters, pinnedFilters);
+  PageFiltersStore.onInitializeUrlState(pageFilters, pinnedFilters, shouldPersist);
   updateDesyncedUrlState(router, shouldForceProject);
 
   const newDatetime = {
@@ -348,6 +355,13 @@ export function pinFilter(filter: PinnedPageFilter, pin: boolean) {
   persistPageFilters(null, {save: true});
 }
 
+/**
+ * Changes whether any value updates will be persisted into local storage.
+ */
+export function updatePersistence(shouldPersist: boolean) {
+  PageFiltersStore.updatePersistence(shouldPersist);
+}
+
 /**
  * Updates router/URL with new query params
  *
@@ -379,7 +393,7 @@ function updateParams(obj: PageFiltersUpdate, router?: Router, options?: Options
  * Pinned state is always persisted.
  */
 async function persistPageFilters(filter: PinnedPageFilter | null, options?: Options) {
-  if (!options?.save) {
+  if (!options?.save || !PageFiltersStore.shouldPersist) {
     return;
   }
 
@@ -409,7 +423,7 @@ async function persistPageFilters(filter: PinnedPageFilter | null, options?: Opt
  */
 async function updateDesyncedUrlState(router?: Router, shouldForceProject?: boolean) {
   // Cannot compare URL state without the router
-  if (!router) {
+  if (!router || !PageFiltersStore.shouldPersist) {
     return;
   }
 

+ 1 - 0
static/app/components/datePageFilter.spec.tsx

@@ -59,6 +59,7 @@ describe('DatePageFilter', function () {
     );
     expect(PageFiltersStore.getState()).toEqual({
       isReady: true,
+      shouldPersist: true,
       desyncedFilters: new Set(),
       pinnedFilters: new Set(),
       selection: {

+ 1 - 0
static/app/components/globalSdkUpdateAlert.spec.tsx

@@ -19,6 +19,7 @@ const makeFilterProps = (
 ): ReturnType<typeof importedUsePageFilters> => {
   return {
     isReady: true,
+    shouldPersist: true,
     desyncedFilters: new Set(),
     pinnedFilters: new Set(),
     selection: {

+ 2 - 0
static/app/components/organizations/datePageFilter.spec.tsx

@@ -58,6 +58,7 @@ describe('DatePageFilter', function () {
     );
     expect(PageFiltersStore.getState()).toEqual({
       isReady: true,
+      shouldPersist: true,
       desyncedFilters: new Set(),
       pinnedFilters: new Set(['datetime']),
       selection: {
@@ -100,6 +101,7 @@ describe('DatePageFilter', function () {
     );
     expect(PageFiltersStore.getState()).toEqual({
       isReady: true,
+      shouldPersist: true,
       desyncedFilters: new Set(),
       pinnedFilters: new Set(['datetime']),
       selection: {

+ 39 - 1
static/app/components/organizations/pageFilters/container.spec.jsx

@@ -8,7 +8,7 @@ import OrganizationsStore from 'sentry/stores/organizationsStore';
 import OrganizationStore from 'sentry/stores/organizationStore';
 import PageFiltersStore from 'sentry/stores/pageFiltersStore';
 import ProjectsStore from 'sentry/stores/projectsStore';
-import {getItem} from 'sentry/utils/localStorage';
+import {getItem, setItem} from 'sentry/utils/localStorage';
 
 const changeQuery = (routerContext, query) => ({
   ...routerContext,
@@ -144,6 +144,7 @@ describe('PageFiltersContainer', function () {
         isReady: true,
         desyncedFilters: new Set(),
         pinnedFilters: new Set(),
+        shouldPersist: true,
         selection: {
           datetime: {
             period: '14d',
@@ -175,6 +176,7 @@ describe('PageFiltersContainer', function () {
         isReady: true,
         desyncedFilters: new Set(),
         pinnedFilters: new Set(),
+        shouldPersist: true,
         selection: {
           datetime: {
             period: '14d',
@@ -204,6 +206,7 @@ describe('PageFiltersContainer', function () {
         isReady: true,
         desyncedFilters: new Set(),
         pinnedFilters: new Set(),
+        shouldPersist: true,
         selection: {
           datetime: {
             period: '14d',
@@ -248,6 +251,7 @@ describe('PageFiltersContainer', function () {
       isReady: true,
       desyncedFilters: new Set(),
       pinnedFilters: new Set(),
+      shouldPersist: true,
       selection: {
         datetime: {
           period: '7d',
@@ -420,6 +424,40 @@ describe('PageFiltersContainer', function () {
     expect(screen.getByRole('button', {name: 'Restore old values'})).toBeInTheDocument();
   });
 
+  it('does not update local storage when disablePersistence is true', async function () {
+    const initializationObj = initializeOrg({
+      organization: {
+        features: ['global-views'],
+      },
+      router: {
+        // we need this to be set to make sure org in context is same as
+        // current org in URL
+        params: {orgId: 'org-slug'},
+        location: {pathname: '/test', query: {project: []}},
+      },
+    });
+
+    renderComponent(
+      <PageFiltersContainer disablePersistence />,
+      initializationObj.routerContext,
+      initializationObj.organization
+    );
+
+    await act(async () => {
+      globalActions.updateProjects([1], initializationObj.router, {save: true});
+
+      // page filter values are asynchronously persisted to local storage after a tick,
+      // so we need to wait before checking for commits to local storage
+      await tick();
+    });
+
+    // Store value was updated
+    expect(PageFiltersStore.getState().selection.projects).toEqual([1]);
+
+    // But local storage wasn't updated
+    expect(setItem).not.toHaveBeenCalled();
+  });
+
   /**
    * GSH: (no global-views)
    * - mounts with no state from router

+ 12 - 0
static/app/components/organizations/pageFilters/container.tsx

@@ -7,6 +7,7 @@ import {
   InitializeUrlStateParams,
   updateDateTime,
   updateEnvironments,
+  updatePersistence,
   updateProjects,
 } from 'sentry/actionCreators/pageFilters';
 import * as Layout from 'sentry/components/layouts/thirds';
@@ -34,6 +35,12 @@ type Props = InitializeUrlStateProps & {
    * Custom alert message for the desynced filter state.
    */
   desyncedAlertMessage?: string;
+  /**
+   * When true, changes to page filters' value won't be saved to local storage, and will
+   * be forgotten when the user navigates to a different page. This is useful for local
+   * filtering contexts like in Dashboard Details.
+   */
+  disablePersistence?: boolean;
   /**
    * Whether to hide the desynced filter alert.
    */
@@ -61,6 +68,7 @@ function Container({skipLoadLastUsed, children, ...props}: Props) {
     shouldForceProject,
     specificProjectSlugs,
     skipInitializeUrlParams,
+    disablePersistence,
     desyncedAlertMessage,
     hideDesyncAlert,
     hideDesyncRevertButton,
@@ -94,6 +102,7 @@ function Container({skipLoadLastUsed, children, ...props}: Props) {
       forceProject,
       shouldForceProject,
       shouldEnforceSingleProject: enforceSingleProject,
+      shouldPersist: !disablePersistence,
       showAbsolute,
       skipInitializeUrlParams,
     });
@@ -115,6 +124,9 @@ function Container({skipLoadLastUsed, children, ...props}: Props) {
     // eslint-disable-next-line react-hooks/exhaustive-deps
   }, [projectsLoaded, shouldForceProject, enforceSingleProject]);
 
+  // Update store persistence when `disablePersistence` changes
+  useEffect(() => updatePersistence(!disablePersistence), [disablePersistence]);
+
   const lastQuery = useRef(location.query);
 
   // This happens e.g. using browser's navigation button, in which case

+ 9 - 0
static/app/stores/pageFiltersStore.spec.jsx

@@ -2,6 +2,7 @@ import {
   pinFilter,
   updateDateTime,
   updateEnvironments,
+  updatePersistence,
   updateProjects,
 } from 'sentry/actionCreators/pageFilters';
 import PageFiltersStore from 'sentry/stores/pageFiltersStore';
@@ -22,6 +23,7 @@ describe('PageFiltersStore', function () {
   it('getState()', function () {
     expect(PageFiltersStore.getState()).toEqual({
       isReady: false,
+      shouldPersist: true,
       desyncedFilters: new Set(),
       pinnedFilters: new Set(),
       selection: {
@@ -91,6 +93,13 @@ describe('PageFiltersStore', function () {
     expect(PageFiltersStore.getState().selection.environments).toEqual(['alpha']);
   });
 
+  it('updatePersistence()', async function () {
+    expect(PageFiltersStore.getState().shouldPersist).toEqual(true);
+    updatePersistence(false);
+    await tick();
+    expect(PageFiltersStore.getState().shouldPersist).toEqual(false);
+  });
+
   it('can mark filters as pinned', async function () {
     expect(PageFiltersStore.getState().pinnedFilters).toEqual(new Set());
     pinFilter('projects', true);

+ 21 - 2
static/app/stores/pageFiltersStore.tsx

@@ -21,6 +21,12 @@ interface CommonState {
    * The current page filter selection
    */
   selection: PageFilters;
+  /**
+   * Whether to save changes to local storage. This setting should be page-specific:
+   * most pages should have it on (default) and some, like Dashboard Details, need it
+   * off.
+   */
+  shouldPersist: boolean;
 }
 
 /**
@@ -44,13 +50,18 @@ interface PageFiltersStoreDefinition
   extends InternalDefinition,
     CommonStoreDefinition<State> {
   init(): void;
-  onInitializeUrlState(newSelection: PageFilters, pinned: Set<PinnedPageFilter>): void;
+  onInitializeUrlState(
+    newSelection: PageFilters,
+    pinned: Set<PinnedPageFilter>,
+    persist?: boolean
+  ): void;
   onReset(): void;
   pin(filter: PinnedPageFilter, pin: boolean): void;
   reset(selection?: PageFilters): void;
   updateDateTime(datetime: PageFilters['datetime']): void;
   updateDesyncedFilters(filters: Set<PinnedPageFilter>): void;
   updateEnvironments(environments: string[] | null): void;
+  updatePersistence(shouldPersist: boolean): void;
   updateProjects(projects: PageFilters['projects'], environments: null | string[]): void;
 }
 
@@ -58,6 +69,7 @@ const storeConfig: PageFiltersStoreDefinition = {
   selection: getDefaultSelection(),
   pinnedFilters: new Set(),
   desyncedFilters: new Set(),
+  shouldPersist: true,
   hasInitialState: false,
 
   init() {
@@ -76,11 +88,12 @@ const storeConfig: PageFiltersStoreDefinition = {
   /**
    * Initializes the page filters store data
    */
-  onInitializeUrlState(newSelection, pinned) {
+  onInitializeUrlState(newSelection, pinned, persist = true) {
     this._isReady = true;
 
     this.selection = newSelection;
     this.pinnedFilters = pinned;
+    this.shouldPersist = persist;
     this.trigger(this.getState());
   },
 
@@ -89,6 +102,7 @@ const storeConfig: PageFiltersStoreDefinition = {
       selection: this.selection,
       pinnedFilters: this.pinnedFilters,
       desyncedFilters: this.desyncedFilters,
+      shouldPersist: this.shouldPersist,
       isReady: this._isReady,
     };
   },
@@ -98,6 +112,11 @@ const storeConfig: PageFiltersStoreDefinition = {
     this.trigger(this.getState());
   },
 
+  updatePersistence(shouldPersist: boolean) {
+    this.shouldPersist = shouldPersist;
+    this.trigger(this.getState());
+  },
+
   updateDesyncedFilters(filters: Set<PinnedPageFilter>) {
     this.desyncedFilters = filters;
     this.trigger(this.getState());

+ 2 - 0
static/app/views/dashboards/detail.tsx

@@ -638,6 +638,7 @@ class DashboardDetail extends Component<Props, State> {
       <PageFiltersContainer
         desyncedAlertMessage='Using filter values saved to this dashboard. To edit saved filters, click "Edit Dashboard".'
         hideDesyncRevertButton
+        disablePersistence
         defaultSelection={{
           datetime: {
             start: null,
@@ -759,6 +760,7 @@ class DashboardDetail extends Component<Props, State> {
         <PageFiltersContainer
           desyncedAlertMessage='Using filter values saved to this dashboard. To edit saved filters, click "Edit Dashboard".'
           hideDesyncRevertButton
+          disablePersistence
           defaultSelection={{
             datetime: {
               start: null,

Некоторые файлы не были показаны из-за большого количества измененных файлов