ref(ui): Readonly selected group store state (#72482)

static/app/components/stream/group.spec.tsx

@@ -137,4 +137,17 @@ describe('StreamGroup', function () {
     // skipHover - Prevent stacktrace preview from being rendered
     await userEvent.click(screen.getByText('RequestError'), {skipHover: true});
   });
+
+  it('can select row', async function () {
+    const {router, organization} = initializeOrg();
+    render(<StreamGroup id="1337" query="is:unresolved" />, {router, organization});
+
+    expect(await screen.findByTestId('group')).toBeInTheDocument();
+    const checkbox = screen.getByRole('checkbox', {name: 'Select Issue'});
+    expect(checkbox).not.toBeChecked();
+    await userEvent.click(checkbox);
+    expect(checkbox).toBeChecked();
+    await userEvent.click(checkbox);
+    expect(checkbox).not.toBeChecked();
+  });
 });

static/app/components/stream/group.tsx

@@ -94,8 +94,8 @@ function GroupCheckbox({
   group: Group;
   displayReprocessingLayout?: boolean;
 }) {
-  const selectedGroups = useLegacyStore(SelectedGroupStore);
-  const isSelected = selectedGroups[group.id];
+  const {records: selectedGroupMap} = useLegacyStore(SelectedGroupStore);
+  const isSelected = selectedGroupMap.get(group.id) ?? false;
 
   const onChange = useCallback(
     (evt: React.ChangeEvent<HTMLInputElement>) => {

static/app/stores/selectedGroupStore.spec.tsx

@@ -1,16 +1,20 @@
 import GroupStore from 'sentry/stores/groupStore';
 import SelectedGroupStore from 'sentry/stores/selectedGroupStore';
 
-function makeRecords(records: Record<string, boolean>) {
-  return new Map(Object.entries(records));
+function setRecords(records: Record<string, boolean>) {
+  SelectedGroupStore.onGroupChange(new Set(Object.keys(records)));
+  for (const [key, isSelected] of Object.entries(records)) {
+    if (isSelected) {
+      SelectedGroupStore.toggleSelect(key);
+    }
+  }
 }
 
 describe('SelectedGroupStore', function () {
   let trigger: jest.SpyInstance;
 
   beforeEach(function () {
-    SelectedGroupStore.records = new Map();
-
+    SelectedGroupStore.init();
     trigger = jest.spyOn(SelectedGroupStore, 'trigger').mockImplementation(() => {});
   });
 
@@ -21,16 +25,16 @@ describe('SelectedGroupStore', function () {
   describe('prune()', function () {
     it('removes records no longer in the GroupStore', function () {
       jest.spyOn(GroupStore, 'getAllItemIds').mockImplementation(() => ['3']);
-      SelectedGroupStore.records = makeRecords({1: true, 2: true, 3: true});
+      setRecords({1: true, 2: true, 3: true});
       SelectedGroupStore.prune();
-      expect([...SelectedGroupStore.records.entries()]).toEqual([['3', true]]);
+      expect([...SelectedGroupStore.getState().records.entries()]).toEqual([['3', true]]);
     });
 
     it("doesn't have any effect when already in sync", function () {
       jest.spyOn(GroupStore, 'getAllItemIds').mockImplementation(() => ['1', '2', '3']);
-      SelectedGroupStore.records = makeRecords({1: true, 2: true, 3: true});
+      setRecords({1: true, 2: true, 3: true});
       SelectedGroupStore.prune();
-      expect([...SelectedGroupStore.records.entries()]).toEqual([
+      expect([...SelectedGroupStore.getState().records.entries()]).toEqual([
         ['1', true],
         ['2', true],
         ['3', true],
@@ -40,18 +44,18 @@ describe('SelectedGroupStore', function () {
 
   describe('add()', function () {
     it("defaults value of new ids to 'allSelected()'", function () {
-      SelectedGroupStore.records = makeRecords({1: true});
+      setRecords({1: true});
       SelectedGroupStore.add(['2']);
-      expect([...SelectedGroupStore.records.entries()]).toEqual([
+      expect([...SelectedGroupStore.getState().records.entries()]).toEqual([
         ['1', true],
         ['2', true],
       ]);
     });
 
     it('does not update existing ids', function () {
-      SelectedGroupStore.records = makeRecords({1: false, 2: true});
+      setRecords({1: false, 2: true});
       SelectedGroupStore.add(['3']);
-      expect([...SelectedGroupStore.records.entries()]).toEqual([
+      expect([...SelectedGroupStore.getState().records.entries()]).toEqual([
         ['1', false],
         ['2', true],
         ['3', false],
@@ -88,17 +92,17 @@ describe('SelectedGroupStore', function () {
 
   describe('allSelected()', function () {
     it('returns true when all ids are selected', function () {
-      SelectedGroupStore.records = makeRecords({1: true, 2: true});
+      setRecords({1: true, 2: true});
       expect(SelectedGroupStore.allSelected()).toBe(true);
     });
 
     it('returns false when some ids are selected', function () {
-      SelectedGroupStore.records = makeRecords({1: true, 2: false});
+      setRecords({1: true, 2: false});
       expect(SelectedGroupStore.allSelected()).toBe(false);
     });
 
     it('returns false when no ids are selected', function () {
-      SelectedGroupStore.records = makeRecords({1: false, 2: false});
+      setRecords({1: false, 2: false});
       expect(SelectedGroupStore.allSelected()).toBe(false);
     });
 
@@ -109,36 +113,36 @@ describe('SelectedGroupStore', function () {
 
   describe('anySelected()', function () {
     it('returns true if any ids are selected', function () {
-      SelectedGroupStore.records = makeRecords({1: true, 2: false});
+      setRecords({1: true, 2: false});
       expect(SelectedGroupStore.anySelected()).toBe(true);
     });
 
     it('returns false when no ids are selected', function () {
-      SelectedGroupStore.records = makeRecords({1: false, 2: false});
+      setRecords({1: false, 2: false});
       expect(SelectedGroupStore.anySelected()).toBe(false);
     });
   });
 
   describe('multiSelected()', function () {
     it('returns true when multiple ids are selected', function () {
-      SelectedGroupStore.records = makeRecords({1: true, 2: true, 3: false});
+      setRecords({1: true, 2: true, 3: false});
       expect(SelectedGroupStore.multiSelected()).toBe(true);
     });
 
     it('returns false when a single id is selected', function () {
-      SelectedGroupStore.records = makeRecords({1: true, 2: false});
+      setRecords({1: true, 2: false});
       expect(SelectedGroupStore.multiSelected()).toBe(false);
     });
 
     it('returns false when no ids are selected', function () {
-      SelectedGroupStore.records = makeRecords({1: false, 2: false});
+      setRecords({1: false, 2: false});
       expect(SelectedGroupStore.multiSelected()).toBe(false);
     });
   });
 
   describe('getSelectedIds()', function () {
     it('returns selected ids', function () {
-      SelectedGroupStore.records = makeRecords({1: true, 2: false, 3: true});
+      setRecords({1: true, 2: false, 3: true});
       const ids = SelectedGroupStore.getSelectedIds();
 
       expect(ids.has('1')).toBe(true);
@@ -147,7 +151,7 @@ describe('SelectedGroupStore', function () {
     });
 
     it('returns empty set with no selected ids', function () {
-      SelectedGroupStore.records = makeRecords({1: false});
+      setRecords({1: false});
       const ids = SelectedGroupStore.getSelectedIds();
 
       expect(ids.has('1')).toBe(false);
@@ -157,12 +161,12 @@ describe('SelectedGroupStore', function () {
 
   describe('isSelected()', function () {
     it('returns true if id is selected', function () {
-      SelectedGroupStore.records = makeRecords({1: true});
+      setRecords({1: true});
       expect(SelectedGroupStore.isSelected('1')).toBe(true);
     });
 
     it('returns false if id is unselected or unknown', function () {
-      SelectedGroupStore.records = makeRecords({1: false});
+      setRecords({1: false});
       expect(SelectedGroupStore.isSelected('1')).toBe(false);
       expect(SelectedGroupStore.isSelected('2')).toBe(false);
       expect(SelectedGroupStore.isSelected('')).toBe(false);
@@ -171,9 +175,9 @@ describe('SelectedGroupStore', function () {
 
   describe('deselectAll()', function () {
     it('sets all records to false', function () {
-      SelectedGroupStore.records = makeRecords({1: true, 2: true, 3: false});
+      setRecords({1: true, 2: true, 3: false});
       SelectedGroupStore.deselectAll();
-      expect([...SelectedGroupStore.records.entries()]).toEqual([
+      expect([...SelectedGroupStore.getState().records.entries()]).toEqual([
         ['1', false],
         ['2', false],
         ['3', false],
@@ -188,19 +192,19 @@ describe('SelectedGroupStore', function () {
 
   describe('toggleSelect()', function () {
     it('toggles state given pre-existing id', function () {
-      SelectedGroupStore.records = makeRecords({1: true});
+      setRecords({1: true});
       SelectedGroupStore.toggleSelect('1');
-      expect(SelectedGroupStore.records.get('1')).toBe(false);
+      expect(SelectedGroupStore.getState().records.get('1')).toBe(false);
     });
 
     it('does not toggle state given unknown id', function () {
       SelectedGroupStore.toggleSelect('1');
       SelectedGroupStore.toggleSelect('');
-      expect([...SelectedGroupStore.records.entries()]).toEqual([]);
+      expect([...SelectedGroupStore.getState().records.entries()]).toEqual([]);
     });
 
     it('triggers an update given pre-existing id', function () {
-      SelectedGroupStore.records = makeRecords({1: true});
+      setRecords({1: true});
       SelectedGroupStore.toggleSelect('1');
       expect(trigger).toHaveBeenCalled();
     });
@@ -213,18 +217,18 @@ describe('SelectedGroupStore', function () {
 
   describe('toggleSelectAll()', function () {
     it('selects all ids if any are unselected', function () {
-      SelectedGroupStore.records = makeRecords({1: true, 2: false});
+      setRecords({1: true, 2: false});
       SelectedGroupStore.toggleSelectAll();
-      expect([...SelectedGroupStore.records.entries()]).toEqual([
+      expect([...SelectedGroupStore.getState().records.entries()]).toEqual([
         ['1', true],
         ['2', true],
       ]);
     });
 
     it('unselects all ids if all are selected', function () {
-      SelectedGroupStore.records = makeRecords({1: true, 2: true});
+      setRecords({1: true, 2: true});
       SelectedGroupStore.toggleSelectAll();
-      expect([...SelectedGroupStore.records.entries()]).toEqual([
+      expect([...SelectedGroupStore.getState().records.entries()]).toEqual([
         ['1', false],
         ['2', false],
       ]);
@@ -243,7 +247,7 @@ describe('SelectedGroupStore', function () {
       SelectedGroupStore.add(ids);
       SelectedGroupStore.toggleSelect('12');
       SelectedGroupStore.shiftToggleItems('14');
-      expect([...SelectedGroupStore.records.entries()]).toEqual([
+      expect([...SelectedGroupStore.getState().records.entries()]).toEqual([
         ['11', false],
         ['12', true],
         ['13', true],
@@ -257,7 +261,7 @@ describe('SelectedGroupStore', function () {
       SelectedGroupStore.add(ids);
       SelectedGroupStore.toggleSelect('14');
       SelectedGroupStore.shiftToggleItems('12');
-      expect([...SelectedGroupStore.records.entries()]).toEqual([
+      expect([...SelectedGroupStore.getState().records.entries()]).toEqual([
         ['11', false],
         ['12', true],
         ['13', true],
@@ -273,7 +277,7 @@ describe('SelectedGroupStore', function () {
 
       // Select everything
       SelectedGroupStore.shiftToggleItems('15');
-      expect([...SelectedGroupStore.records.entries()]).toEqual([
+      expect([...SelectedGroupStore.getState().records.entries()]).toEqual([
         ['11', true],
         ['12', true],
         ['13', true],
@@ -284,7 +288,7 @@ describe('SelectedGroupStore', function () {
       // Deslect between 14 and 12
       SelectedGroupStore.toggleSelect('14');
       SelectedGroupStore.shiftToggleItems('12');
-      expect([...SelectedGroupStore.records.entries()]).toEqual([
+      expect([...SelectedGroupStore.getState().records.entries()]).toEqual([
         ['11', true],
         ['12', false],
         ['13', false],
@@ -293,4 +297,10 @@ describe('SelectedGroupStore', function () {
       ]);
     });
   });
+
+  it('returns a stable reference from getState', function () {
+    setRecords({1: true, 2: true});
+    const state = SelectedGroupStore.getState();
+    expect(Object.is(state, SelectedGroupStore.getState())).toBe(true);
+  });
 });

static/app/stores/selectedGroupStore.tsx

@@ -2,7 +2,7 @@ import {createStore} from 'reflux';
 
 import GroupStore from 'sentry/stores/groupStore';
 
-import type {CommonStoreDefinition} from './types';
+import type {StrictStoreDefinition} from './types';
 
 interface InternalDefinition {
   /**
@@ -16,15 +16,12 @@ interface InternalDefinition {
   records: Map<string, boolean>;
 }
 
-interface SelectedGroupStoreDefinition
-  extends CommonStoreDefinition<Record<string, boolean>>,
-    InternalDefinition {
+interface SelectedGroupStoreDefinition extends StrictStoreDefinition<InternalDefinition> {
   add(ids: string[]): void;
   allSelected(): boolean;
   anySelected(): boolean;
   deselectAll(): void;
   getSelectedIds(): Set<string>;
-  init(): void;
   isSelected(itemId: string): boolean;
   multiSelected(): boolean;
   onGroupChange(itemIds: Set<string>): void;
@@ -36,9 +33,7 @@ interface SelectedGroupStoreDefinition
 }
 
 const storeConfig: SelectedGroupStoreDefinition = {
-  records: new Map(),
-  lastSelected: null,
-  unsubscribeListeners: [],
+  state: {records: new Map(), lastSelected: null},
 
   init() {
     // XXX: Do not use `this.listenTo` in this store. We avoid usage of reflux
@@ -48,12 +43,11 @@ const storeConfig: SelectedGroupStoreDefinition = {
   },
 
   reset() {
-    this.records = new Map();
-    this.lastSelected = null;
+    this.state = {records: new Map(), lastSelected: null};
   },
 
   getState() {
-    return Object.fromEntries(this.records);
+    return this.state;
   },
 
   onGroupChange(itemIds) {
@@ -65,29 +59,27 @@ const storeConfig: SelectedGroupStoreDefinition = {
   add(ids) {
     const allSelected = this.allSelected();
 
-    ids
-      .filter(id => !this.records.has(id))
-      .forEach(id => this.records.set(id, allSelected));
+    const newRecords = new Map(this.state.records);
+    ids.filter(id => !newRecords.has(id)).forEach(id => newRecords.set(id, allSelected));
+    this.state = {...this.state, records: newRecords};
   },
 
   prune() {
     const existingIds = new Set(GroupStore.getAllItemIds());
-    this.lastSelected = null;
 
     // Remove everything that no longer exists
-    [...this.records.keys()]
+    const newRecords = new Map(this.state.records);
+    [...this.state.records.keys()]
       .filter(id => !existingIds.has(id))
-      .forEach(id => this.records.delete(id));
+      .forEach(id => newRecords.delete(id));
+
+    this.state = {...this.state, lastSelected: null, records: newRecords};
   },
 
   allSelected() {
     const itemIds = this.getSelectedIds();
 
-    return itemIds.size > 0 && itemIds.size === this.records.size;
-  },
-
-  numSelected() {
-    return this.getSelectedIds().size;
+    return itemIds.size > 0 && itemIds.size === this.state.records.size;
   },
 
   anySelected() {
@@ -99,66 +91,73 @@ const storeConfig: SelectedGroupStoreDefinition = {
   },
 
   getSelectedIds() {
-    return new Set([...this.records.keys()].filter(id => this.records.get(id)));
+    return new Set(
+      [...this.state.records.keys()].filter(id => this.state.records.get(id))
+    );
   },
 
   isSelected(itemId) {
-    return !!this.records.get(itemId);
+    return !!this.state.records.get(itemId);
   },
 
   deselectAll() {
-    this.records.forEach((_, id) => this.records.set(id, false));
+    const newRecords = new Map(this.state.records);
+    this.state.records.forEach((_, id) => newRecords.set(id, false));
+    this.state = {...this.state, records: newRecords};
     this.trigger();
   },
 
   toggleSelect(itemId) {
-    if (!this.records.has(itemId)) {
+    if (!this.state.records.has(itemId)) {
       return;
     }
 
-    const newState = !this.records.get(itemId);
-    this.records.set(itemId, newState);
-    this.lastSelected = itemId;
+    const newSelectedState = !this.state.records.get(itemId);
+    const newRecords = new Map(this.state.records);
+    newRecords.set(itemId, newSelectedState);
 
+    this.state = {...this.state, records: newRecords, lastSelected: itemId};
     this.trigger();
   },
 
   toggleSelectAll() {
     const allSelected = !this.allSelected();
-    this.lastSelected = null;
 
-    this.records.forEach((_, id) => this.records.set(id, allSelected));
+    const newRecords = new Map(this.state.records);
+    newRecords.forEach((_, id) => newRecords.set(id, allSelected));
+    this.state = {...this.state, records: newRecords, lastSelected: null};
     this.trigger();
   },
 
   shiftToggleItems(itemId) {
-    if (!this.records.has(itemId)) {
+    if (!this.state.records.has(itemId)) {
       return;
     }
-    if (!this.lastSelected) {
+    if (!this.state.lastSelected) {
       this.toggleSelect(itemId);
       return;
     }
 
     const ids = GroupStore.getAllItemIds();
-    const lastIdx = ids.findIndex(id => id === this.lastSelected);
+    const lastIdx = ids.findIndex(id => id === this.state.lastSelected);
     const currentIdx = ids.findIndex(id => id === itemId);
 
     if (lastIdx === -1 || currentIdx === -1) {
       return;
     }
 
-    const newValue = !this.records.get(itemId);
+    const newValue = !this.state.records.get(itemId);
     const selected =
       lastIdx < currentIdx
         ? ids.slice(lastIdx, currentIdx)
         : ids.slice(currentIdx, lastIdx);
 
-    [...selected, this.lastSelected, itemId]
-      .filter(id => this.records.has(id))
-      .forEach(id => this.records.set(id, newValue));
+    const newRecords = new Map(this.state.records);
+    [...selected, this.state.lastSelected, itemId]
+      .filter(id => newRecords.has(id))
+      .forEach(id => newRecords.set(id, newValue));
 
-    this.lastSelected = itemId;
+    this.state = {...this.state, records: newRecords, lastSelected: itemId};
     this.trigger();
   },
 };

static/app/views/issueList/actions/index.spec.tsx

@@ -65,13 +65,13 @@ describe('IssueListActions', function () {
       it('after checking "Select all" checkbox, displays bulk select message', async function () {
         render(<WrappedComponent queryCount={1500} />);
 
-        await userEvent.click(screen.getByRole('checkbox'));
+        await userEvent.click(screen.getByRole('checkbox', {name: 'Select all'}));
       });
 
       it('can bulk select', async function () {
         render(<WrappedComponent queryCount={1500} />);
 
-        await userEvent.click(screen.getByRole('checkbox'));
+        await userEvent.click(screen.getByRole('checkbox', {name: 'Select all'}));
         await userEvent.click(screen.getByTestId('issue-list-select-all-notice-link'));
       });
 
@@ -82,7 +82,7 @@ describe('IssueListActions', function () {
         });
 
         render(<WrappedComponent queryCount={1500} />);
-        await userEvent.click(screen.getByRole('checkbox'));
+        await userEvent.click(screen.getByRole('checkbox', {name: 'Select all'}));
 
         await userEvent.click(screen.getByTestId('issue-list-select-all-notice-link'));
 
@@ -113,7 +113,7 @@ describe('IssueListActions', function () {
           organization: OrganizationFixture({features: ['issue-priority-ui']}),
         });
 
-        await userEvent.click(screen.getByRole('checkbox'));
+        await userEvent.click(screen.getByRole('checkbox', {name: 'Select all'}));
         await userEvent.click(screen.getByTestId('issue-list-select-all-notice-link'));
         await userEvent.click(await screen.findByRole('button', {name: 'Set Priority'}));
         await userEvent.click(screen.getByRole('menuitemradio', {name: 'High'}));
@@ -144,14 +144,9 @@ describe('IssueListActions', function () {
       it('after checking "Select all" checkbox, displays bulk select message', async function () {
         render(<WrappedComponent queryCount={15} />);
 
-        await userEvent.click(screen.getByRole('checkbox'));
-      });
-
-      it('can bulk select', async function () {
-        render(<WrappedComponent queryCount={15} />);
-
-        await userEvent.click(screen.getByRole('checkbox'));
-
+        const checkbox = screen.getByRole('checkbox', {name: 'Select all'});
+        await userEvent.click(checkbox);
+        expect(checkbox).toBeChecked();
         await userEvent.click(screen.getByTestId('issue-list-select-all-notice-link'));
       });
 
@@ -163,7 +158,7 @@ describe('IssueListActions', function () {
 
         render(<WrappedComponent queryCount={15} />);
 
-        await userEvent.click(screen.getByRole('checkbox'));
+        await userEvent.click(screen.getByRole('checkbox', {name: 'Select all'}));
 
         await userEvent.click(screen.getByTestId('issue-list-select-all-notice-link'));
 
@@ -435,7 +430,7 @@ describe('IssueListActions', function () {
           {organization: orgWithPerformanceIssues}
         );
 
-        await userEvent.click(screen.getByRole('checkbox'));
+        await userEvent.click(screen.getByRole('checkbox', {name: 'Select all'}));
 
         await userEvent.click(screen.getByTestId('issue-list-select-all-notice-link'));
 
@@ -483,7 +478,7 @@ describe('IssueListActions', function () {
           {organization: orgWithPerformanceIssues}
         );
 
-        await userEvent.click(screen.getByRole('checkbox'));
+        await userEvent.click(screen.getByRole('checkbox', {name: 'Select all'}));
 
         await userEvent.click(screen.getByTestId('issue-list-select-all-notice-link'));
 

static/app/views/issueList/actions/index.tsx

@@ -107,6 +107,7 @@ function ActionsBarPriority({
           <Checkbox
             onChange={() => SelectedGroupStore.toggleSelectAll()}
             checked={pageSelected || (anySelected ? 'indeterminate' : false)}
+            aria-label={pageSelected ? t('Deselect all') : t('Select all')}
             disabled={displayReprocessingActions}
           />
         </ActionsCheckbox>
@@ -377,6 +378,7 @@ function IssueListActions({
               <Checkbox
                 onChange={() => SelectedGroupStore.toggleSelectAll()}
                 checked={pageSelected || (anySelected ? 'indeterminate' : false)}
+                aria-label={pageSelected ? t('Deselect all') : t('Select all')}
                 disabled={displayReprocessingActions}
               />
             </ActionsCheckbox>
@@ -461,10 +463,10 @@ function IssueListActions({
 
 function useSelectedGroupsState() {
   const [allInQuerySelected, setAllInQuerySelected] = useState(false);
-  const selectedIds = useLegacyStore(SelectedGroupStore);
+  const selectedGroupState = useLegacyStore(SelectedGroupStore);
+  const selectedIds = SelectedGroupStore.getSelectedIds();
 
-  const selected = SelectedGroupStore.getSelectedIds();
-  const projects = [...selected]
+  const projects = [...selectedIds]
     .map(id => GroupStore.get(id))
     .filter((group): group is Group => !!group?.project)
     .map(group => group.project.slug);
@@ -482,7 +484,7 @@ function useSelectedGroupsState() {
 
   useEffect(() => {
     setAllInQuerySelected(false);
-  }, [selectedIds]);
+  }, [selectedGroupState]);
 
   return {
     pageSelected,