feat(perf): Add store for Committers (#20725)

src/sentry/static/sentry/app/actionCreators/committers.tsx

@@ -0,0 +1,44 @@
+import * as Sentry from '@sentry/react';
+
+import CommitterActions from 'app/actions/committerActions';
+import {Client} from 'app/api';
+import CommitterStore, {getCommitterStoreKey} from 'app/stores/committerStore';
+import {Committer} from 'app/types';
+
+type ParamsGet = {
+  orgSlug: string;
+  projectSlug: string;
+  eventId: string;
+};
+
+export function getCommitters(api: Client, params: ParamsGet) {
+  const {orgSlug, projectSlug, eventId} = params;
+  const path = `/projects/${orgSlug}/${projectSlug}/events/${eventId}/committers/`;
+
+  // HACK(leedongwei): Actions fired by the ActionCreators are queued to
+  // the back of the event loop, allowing another getRepo for the same
+  // repo to be fired before the loading state is updated in store.
+  // This hack short-circuits that and update the state immediately.
+  const storeKey = getCommitterStoreKey(orgSlug, projectSlug, eventId);
+  CommitterStore.state[storeKey] = {
+    ...CommitterStore.state[storeKey],
+    committersLoading: true,
+  };
+  CommitterActions.load(orgSlug, projectSlug, eventId);
+
+  return api
+    .requestPromise(path, {
+      method: 'GET',
+    })
+    .then((res: {committers: Committer[]}) => {
+      CommitterActions.loadSuccess(orgSlug, projectSlug, eventId, res.committers);
+    })
+    .catch(err => {
+      CommitterActions.loadError(orgSlug, projectSlug, eventId, err);
+      Sentry.withScope(scope => {
+        scope.setLevel(Sentry.Severity.Warning);
+        scope.setFingerprint(['getCommitters-action-creator']);
+        Sentry.captureException(err);
+      });
+    });
+}

src/sentry/static/sentry/app/actions/committerActions.tsx

@@ -0,0 +1,3 @@
+import Reflux from 'reflux';
+
+export default Reflux.createActions(['reset', 'load', 'loadError', 'loadSuccess']);

src/sentry/static/sentry/app/stores/committerStore.tsx

@@ -0,0 +1,106 @@
+import Reflux from 'reflux';
+
+import CommitterActions from 'app/actions/committerActions';
+import {Committer} from 'app/types';
+
+type CommitterStoreInterface = {
+  state: {
+    // Use `getCommitterStoreKey` to generate key
+    [key: string]: {
+      committers?: Committer[];
+      committersLoading?: boolean;
+      committersError?: Error;
+    };
+  };
+
+  load(orgSlug: string, projectSlug: string, eventId: string): void;
+  loadSuccess(
+    orgSlug: string,
+    projectSlug: string,
+    eventId: string,
+    data: Committer[]
+  ): void;
+  loadError(orgSlug: string, projectSlug: string, eventId: string, error: Error): void;
+
+  get(
+    orgSlug: string,
+    projectSlug: string,
+    eventId: string
+  ): {
+    committers?: Committer[];
+    committersLoading?: boolean;
+    committersError?: Error;
+  };
+};
+
+export const CommitterStoreConfig: Reflux.StoreDefinition & CommitterStoreInterface = {
+  listenables: CommitterActions,
+  state: {},
+
+  init() {
+    this.reset();
+  },
+
+  reset() {
+    this.state = {};
+    this.trigger(this.state);
+  },
+
+  load(orgSlug: string, projectSlug: string, eventId: string) {
+    const key = getCommitterStoreKey(orgSlug, projectSlug, eventId);
+    this.state = {
+      ...this.state,
+      [key]: {
+        committers: undefined,
+        committersLoading: true,
+        committersError: undefined,
+      },
+    };
+
+    this.trigger(this.state);
+  },
+
+  loadError(orgSlug: string, projectSlug: string, eventId: string, err: Error) {
+    const key = getCommitterStoreKey(orgSlug, projectSlug, eventId);
+    this.state = {
+      ...this.state,
+      [key]: {
+        committers: undefined,
+        committersLoading: false,
+        committersError: err,
+      },
+    };
+
+    this.trigger(this.state);
+  },
+
+  loadSuccess(orgSlug: string, projectSlug: string, eventId: string, data: Committer[]) {
+    const key = getCommitterStoreKey(orgSlug, projectSlug, eventId);
+    this.state = {
+      ...this.state,
+      [key]: {
+        committers: data,
+        committersLoading: false,
+        committersError: undefined,
+      },
+    };
+
+    this.trigger(this.state);
+  },
+
+  get(orgSlug: string, projectSlug: string, eventId: string) {
+    const key = getCommitterStoreKey(orgSlug, projectSlug, eventId);
+    return {...this.state[key]};
+  },
+};
+
+export function getCommitterStoreKey(
+  orgSlug: string,
+  projectSlug: string,
+  eventId: string
+): string {
+  return `${orgSlug} ${projectSlug} ${eventId}`;
+}
+
+type CommitterStore = Reflux.Store & CommitterStoreInterface;
+export default Reflux.createStore(CommitterStoreConfig) as CommitterStore;

src/sentry/static/sentry/app/types/index.tsx

@@ -1079,6 +1079,11 @@ export type Commit = {
   releases: BaseRelease[];
 };
 
+export type Committer = {
+  author: User;
+  commits: Commit[];
+};
+
 export type CommitFile = {
   id: string;
   author: CommitAuthor;

src/sentry/static/sentry/app/utils/withCommitters.tsx

@@ -0,0 +1,93 @@
+import React from 'react';
+import Reflux from 'reflux';
+import createReactClass from 'create-react-class';
+
+import {Client} from 'app/api';
+import {Organization, Project, Event, Group, Committer} from 'app/types';
+import {getCommitters} from 'app/actionCreators/committers';
+import CommitterStore from 'app/stores/committerStore';
+import getDisplayName from 'app/utils/getDisplayName';
+
+type DependentProps = {
+  api: Client;
+
+  organization: Organization;
+  project: Project;
+  event: Event;
+  group?: Group;
+};
+
+// XXX: State does not include loading/error because components using this
+// HOC (suggestedOwners, eventCause) do not have loading/error states. However,
+// the store maintains those states if it is needed in the future.
+type InjectedProps = {
+  committers: Committer[];
+};
+
+const initialState: InjectedProps = {
+  committers: [],
+};
+
+const withCommitters = <P extends DependentProps>(
+  WrappedComponent: React.ComponentType<P>
+) =>
+  createReactClass<
+    Omit<P, keyof InjectedProps> & Partial<InjectedProps> & DependentProps,
+    InjectedProps
+  >({
+    displayName: `withCommitters(${getDisplayName(WrappedComponent)})`,
+    mixins: [Reflux.listenTo(CommitterStore, 'onStoreUpdate') as any],
+
+    getInitialState() {
+      const {organization, project, event} = this.props as P & DependentProps;
+      const repoData = CommitterStore.get(organization.slug, project.slug, event.id);
+
+      return {...initialState, ...repoData};
+    },
+
+    componentDidMount() {
+      const {group} = this.props as P & DependentProps;
+
+      // No committers if group doesn't have any releases
+      if (!!group?.firstRelease) {
+        this.fetchCommitters();
+      }
+    },
+
+    fetchCommitters() {
+      const {api, organization, project, event} = this.props as P & DependentProps;
+      const repoData = CommitterStore.get(organization.slug, project.slug, event.id);
+
+      if (
+        (!repoData.committers && !repoData.committersLoading) ||
+        repoData.committersError
+      ) {
+        getCommitters(api, {
+          orgSlug: organization.slug,
+          projectSlug: project.slug,
+          eventId: event.id,
+        });
+      }
+    },
+
+    onStoreUpdate() {
+      const {organization, project, event} = this.props as P & DependentProps;
+      const repoData = CommitterStore.get(organization.slug, project.slug, event.id);
+      this.setState({committers: repoData.committers});
+    },
+
+    render() {
+      const {committers = []} = this.state as InjectedProps;
+
+      // XXX: We do not pass loading/error states because the components using
+      // this HOC (suggestedOwners, eventCause) do not have loading/error states
+      return (
+        <WrappedComponent
+          {...(this.props as P & DependentProps)}
+          committers={committers}
+        />
+      );
+    },
+  });
+
+export default withCommitters;

tests/js/spec/actionCreators/committers.spec.jsx

@@ -0,0 +1,98 @@
+import {getCommitters} from 'app/actionCreators/committers';
+import CommitterActions from 'app/actions/committerActions';
+import CommitterStore, {getCommitterStoreKey} from 'app/stores/committerStore';
+
+describe('CommitterActionCreator', function() {
+  const organization = TestStubs.Organization();
+  const project = TestStubs.Project();
+  const event = TestStubs.Event();
+
+  const storeKey = getCommitterStoreKey(organization.slug, project.slug, event.id);
+  const endpoint = `/projects/${organization.slug}/${project.slug}/events/${event.id}/committers/`;
+
+  const api = new MockApiClient();
+  const mockData = {
+    committers: [
+      {
+        author: TestStubs.CommitAuthor(),
+        commits: [TestStubs.Commit()],
+      },
+    ],
+  };
+  let mockResponse;
+
+  beforeEach(() => {
+    MockApiClient.clearMockResponses();
+    mockResponse = MockApiClient.addMockResponse({
+      url: endpoint,
+      body: mockData,
+    });
+
+    CommitterStore.init();
+
+    jest.restoreAllMocks();
+    jest.spyOn(CommitterActions, 'load');
+    jest.spyOn(CommitterActions, 'loadSuccess');
+
+    /**
+     * XXX(leedongwei): We would want to ensure that Store methods are not
+     * called to be 100% sure that the short-circuit is happening correctly.
+     *
+     * However, it seems like we cannot attach a listener to the method
+     * See: https://github.com/reflux/refluxjs/issues/139#issuecomment-64495623
+     */
+    // jest.spyOn(CommitterStore, 'load');
+    // jest.spyOn(CommitterStore, 'loadSuccess');
+  });
+
+  /**
+   * XXX(leedongwei): I wanted to separate the ticks and run tests to assert the
+   * state change at every tick but it is incredibly flakey.
+   */
+  it('fetches a Committer and emits actions', async () => {
+    getCommitters(api, {
+      orgSlug: organization.slug,
+      projectSlug: project.slug,
+      eventId: event.id,
+    }); // Fire Action.load
+    expect(CommitterActions.load).toHaveBeenCalledWith(
+      organization.slug,
+      project.slug,
+      event.id
+    );
+    expect(CommitterActions.loadSuccess).not.toHaveBeenCalled();
+
+    await tick(); // Run Store.load and fire Action.loadSuccess
+    await tick(); // Run Store.loadSuccess
+
+    expect(mockResponse).toHaveBeenCalledWith(endpoint, expect.anything());
+    expect(CommitterActions.loadSuccess).toHaveBeenCalledWith(
+      organization.slug,
+      project.slug,
+      event.id,
+      mockData.committers
+    );
+
+    expect(CommitterStore.state).toEqual({
+      [storeKey]: {
+        committers: mockData.committers,
+        committersLoading: false,
+        committersError: undefined,
+      },
+    });
+  });
+
+  it('short-circuits the JS event loop', async () => {
+    expect(CommitterStore.state.committersLoading).toEqual(undefined);
+
+    getCommitters(api, {
+      orgSlug: organization.slug,
+      projectSlug: project.slug,
+      eventId: event.id,
+    }); // Fire Action.load
+
+    expect(CommitterActions.load).toHaveBeenCalled();
+    // expect(CommitterStore.load).not.toHaveBeenCalled();
+    expect(CommitterStore.state[storeKey].committersLoading).toEqual(true); // Short-circuit
+  });
+});

tests/js/spec/utils/withCommitters.spec.jsx

@@ -0,0 +1,161 @@
+import React from 'react';
+
+import {mount} from 'sentry-test/enzyme';
+
+import CommitterStore from 'app/stores/committerStore';
+import withCommitters from 'app/utils/withCommitters';
+
+describe('withCommitters HoC', function() {
+  const organization = TestStubs.Organization();
+  const project = TestStubs.Project();
+  const event = TestStubs.Event();
+  const group = TestStubs.Group({firstRelease: {}});
+
+  const endpoint = `/projects/${organization.slug}/${project.slug}/events/${event.id}/committers/`;
+
+  const api = new MockApiClient();
+  const mockData = {
+    committers: [
+      {
+        author: TestStubs.CommitAuthor(),
+        commits: [TestStubs.Commit()],
+      },
+    ],
+  };
+
+  beforeEach(() => {
+    MockApiClient.clearMockResponses();
+    MockApiClient.addMockResponse({
+      url: endpoint,
+      body: mockData,
+    });
+
+    jest.restoreAllMocks();
+    CommitterStore.init();
+  });
+
+  it('adds committers prop', async () => {
+    const Component = () => null;
+    const Container = withCommitters(Component);
+    const wrapper = mount(
+      <Container
+        api={api}
+        organization={organization}
+        project={project}
+        event={event}
+        group={group}
+      />
+    );
+
+    await tick(); // Run Store.load
+    await tick(); // Run Store.loadSuccess
+    wrapper.update(); // Re-render component with Store data
+
+    const mountedComponent = wrapper.find(Component);
+    expect(mountedComponent.prop('committers')).toEqual(mockData.committers);
+    expect(mountedComponent.prop('committersLoading')).toEqual(undefined);
+    expect(mountedComponent.prop('committersError')).toEqual(undefined);
+  });
+
+  it('prevents repeated calls', async () => {
+    const Component = () => null;
+    const Container = withCommitters(Component);
+
+    // XXX(leedongwei): We cannot spy on `fetchCommitters` as Jest can't
+    // replace the method in the prototype due to createReactClass.
+    // As such, I'm using `componentDidMount` as a proxy.
+    jest.spyOn(api, 'requestPromise');
+    jest.spyOn(Container.prototype, 'componentDidMount');
+    // jest.spyOn(Container.prototype, 'fetchCommitters');
+
+    // Mount and run component
+    mount(
+      <Container
+        api={api}
+        organization={organization}
+        project={project}
+        event={event}
+        group={group}
+      />
+    );
+    await tick();
+    await tick();
+
+    // Mount and run duplicates
+    mount(
+      <Container
+        api={api}
+        organization={organization}
+        project={project}
+        event={event}
+        group={group}
+      />
+    );
+    await tick();
+    mount(
+      <Container
+        api={api}
+        organization={organization}
+        project={project}
+        event={event}
+        group={group}
+      />
+    );
+    await tick();
+
+    expect(api.requestPromise).toHaveBeenCalledTimes(1);
+    expect(Container.prototype.componentDidMount).toHaveBeenCalledTimes(3);
+    // expect(Container.prototype.fetchCommitters).toHaveBeenCalledTimes(3);
+  });
+
+  /**
+   * Same as 'prevents repeated calls', but with the async fetch/checks
+   * happening on same tick.
+   *
+   * Additionally, this test checks that withCommitters.fetchCommitters does
+   * not check for (store.orgSlug !== orgSlug) as the short-circuit does not
+   * change the value for orgSlug
+   */
+  it('prevents simultaneous calls', async () => {
+    const Component = () => null;
+    const Container = withCommitters(Component);
+
+    jest.spyOn(api, 'requestPromise');
+    jest.spyOn(Container.prototype, 'componentDidMount');
+
+    // Mount and run duplicates
+    mount(
+      <Container
+        api={api}
+        organization={organization}
+        project={project}
+        event={event}
+        group={group}
+      />
+    );
+    mount(
+      <Container
+        api={api}
+        organization={organization}
+        project={project}
+        event={event}
+        group={group}
+      />
+    );
+    mount(
+      <Container
+        api={api}
+        organization={organization}
+        project={project}
+        event={event}
+        group={group}
+      />
+    );
+
+    await tick();
+    await tick();
+
+    expect(api.requestPromise).toHaveBeenCalledTimes(1);
+    expect(Container.prototype.componentDidMount).toHaveBeenCalledTimes(3);
+  });
+});