ref(ui): Strict group store state (#72172)

static/app/stores/groupStore.spec.tsx

@@ -3,7 +3,8 @@ import {GroupFixture} from 'sentry-fixture/group';
 import {ProjectFixture} from 'sentry-fixture/project';
 
 import GroupStore from 'sentry/stores/groupStore';
-import type {Group, GroupStats, TimeseriesValue} from 'sentry/types';
+import type {TimeseriesValue} from 'sentry/types/core';
+import type {Group, GroupStats} from 'sentry/types/group';
 import {GroupActivityType} from 'sentry/types/group';
 
 const MOCK_PROJECT = ProjectFixture();
@@ -163,6 +164,14 @@ describe('GroupStore', function () {
     });
   });
 
+  describe('getState()', function () {
+    it('returns a stable reference', () => {
+      GroupStore.add([g('1'), g('2')]);
+      const state = GroupStore.getState();
+      expect(Object.is(state, GroupStore.getState())).toBe(true);
+    });
+  });
+
   describe('update methods', function () {
     beforeEach(function () {
       jest.spyOn(GroupStore, 'trigger');

static/app/stores/groupStore.tsx

@@ -3,12 +3,12 @@ import {createStore} from 'reflux';
 import type {Indicator} from 'sentry/actionCreators/indicator';
 import {t} from 'sentry/locale';
 import IndicatorStore from 'sentry/stores/indicatorStore';
-import type {Activity, BaseGroup, Group, GroupStats} from 'sentry/types';
+import type {Activity, BaseGroup, Group, GroupStats} from 'sentry/types/group';
 import toArray from 'sentry/utils/array/toArray';
 import type RequestError from 'sentry/utils/requestError/requestError';
 
 import SelectedGroupStore from './selectedGroupStore';
-import type {CommonStoreDefinition} from './types';
+import type {StrictStoreDefinition} from './types';
 
 function showAlert(msg: string, type: Indicator['type']) {
   IndicatorStore.addMessage(msg, type, {duration: 4000});
@@ -28,6 +28,10 @@ type ItemIds = string[] | undefined;
 interface InternalDefinition {
   addActivity: (groupId: string, data: Activity, index?: number) => void;
   indexOfActivity: (groupId: string, id: string) => number;
+  /**
+   * Does not include pending changes
+   * TODO: Remove mutation and replace state with items
+   */
   items: Item[];
 
   pendingChanges: Map<ChangeId, Change>;
@@ -37,15 +41,15 @@ interface InternalDefinition {
   updateItems: (itemIds: ItemIds) => void;
 }
 
-interface GroupStoreDefinition extends CommonStoreDefinition<Item[]>, InternalDefinition {
+interface GroupStoreDefinition extends StrictStoreDefinition<Item[]>, InternalDefinition {
   add: (items: Item[]) => void;
   addStatus: (id: string, status: string) => void;
   addToFront: (items: Item[]) => void;
   clearStatus: (id: string, status: string) => void;
 
-  get: (id: string) => Item | undefined;
+  get: (id: string) => Readonly<Item> | undefined;
   getAllItemIds: () => string[];
-  getAllItems: () => Item[];
+  getAllItems: () => Readonly<Item[]>;
 
   hasStatus: (id: string, status: string) => boolean;
   init: () => void;
@@ -54,6 +58,8 @@ interface GroupStoreDefinition extends CommonStoreDefinition<Item[]>, InternalDe
 
   loadInitialData: (items: Item[]) => void;
 
+  mergeItems: (items: Item[]) => Item[];
+
   onAssignTo: (changeId: string, itemId: string, data: any) => void;
   onAssignToError: (changeId: string, itemId: string, error: RequestError) => void;
   onAssignToSuccess: (changeId: string, itemId: string, response: any) => void;
@@ -81,9 +87,36 @@ interface GroupStoreDefinition extends CommonStoreDefinition<Item[]>, InternalDe
   reset: () => void;
 }
 
+function mergePendingChanges(
+  items: Readonly<Item[]>,
+  pendingChanges: Map<ChangeId, Change>
+): Readonly<Item[]> {
+  // Merge pending changes into the existing group items. This gives the
+  // apperance of optimistic updates
+  const pendingById: Record<string, Change[]> = {};
+
+  pendingChanges.forEach(change => {
+    change.itemIds.forEach(itemId => {
+      const existing = pendingById[itemId] ?? [];
+      pendingById[itemId] = [...existing, change];
+    });
+  });
+
+  // Merge pending changes into the item if it has them
+  return items.map(item =>
+    pendingById[item.id] === undefined
+      ? item
+      : {
+          ...item,
+          ...pendingById[item.id].reduce((a, change) => ({...a, ...change.data}), {}),
+        }
+  );
+}
+
 const storeConfig: GroupStoreDefinition = {
   pendingChanges: new Map(),
   items: [],
+  state: [],
   statuses: {},
 
   init() {
@@ -96,24 +129,22 @@ const storeConfig: GroupStoreDefinition = {
   reset() {
     this.pendingChanges = new Map();
     this.items = [];
+    this.state = [];
     this.statuses = {};
   },
 
-  // TODO(dcramer): this should actually come from an action of some sorts
   loadInitialData(items) {
     this.reset();
 
-    const itemIds = new Set<string>();
-    items.forEach(item => {
-      itemIds.add(item.id);
-      this.items.push(item);
-    });
+    const itemIds = items.map(item => item.id);
+    this.items = [...this.items, ...items];
 
-    this.trigger(itemIds);
+    this.updateItems(itemIds);
   },
 
   updateItems(itemIds: ItemIds) {
     const idSet = new Set(itemIds);
+    this.state = mergePendingChanges(this.items, this.pendingChanges);
     this.trigger(idSet);
     SelectedGroupStore.onGroupChange(idSet);
   },
@@ -193,7 +224,7 @@ const storeConfig: GroupStoreDefinition = {
   },
 
   indexOfActivity(groupId, id) {
-    const group = this.get(groupId);
+    const group = this.items.find(item => item.id === groupId);
     if (!group) {
       return -1;
     }
@@ -206,8 +237,8 @@ const storeConfig: GroupStoreDefinition = {
     return -1;
   },
 
-  addActivity(id, data, index = -1) {
-    const group = this.get(id);
+  addActivity(groupId, data, index = -1) {
+    const group = this.items.find(item => item.id === groupId);
     if (!group) {
       return;
     }
@@ -222,11 +253,11 @@ const storeConfig: GroupStoreDefinition = {
       group.numComments++;
     }
 
-    this.updateItems([id]);
+    this.updateItems([groupId]);
   },
 
   updateActivity(groupId, id, data) {
-    const group = this.get(groupId);
+    const group = this.items.find(item => item.id === groupId);
     if (!group) {
       return;
     }
@@ -244,7 +275,7 @@ const storeConfig: GroupStoreDefinition = {
   },
 
   removeActivity(groupId, id) {
-    const group = this.get(groupId);
+    const group = this.items.find(item => item.id === groupId);
     if (!group) {
       return -1;
     }
@@ -273,30 +304,11 @@ const storeConfig: GroupStoreDefinition = {
   },
 
   getAllItems() {
-    // Merge pending changes into the existing group items. This gives the
-    // apperance of optimistic updates
-    const pendingById: Record<string, Change[]> = {};
-
-    this.pendingChanges.forEach(change => {
-      change.itemIds.forEach(itemId => {
-        const existing = pendingById[itemId] ?? [];
-        pendingById[itemId] = [...existing, change];
-      });
-    });
-
-    // Merge pending changes into the item if it has them
-    return this.items.map(item =>
-      pendingById[item.id] === undefined
-        ? item
-        : {
-            ...item,
-            ...pendingById[item.id].reduce((a, change) => ({...a, ...change.data}), {}),
-          }
-    );
+    return this.state;
   },
 
   getState() {
-    return this.getAllItems();
+    return this.state;
   },
 
   onAssignTo(_changeId, itemId, _data) {