Convert Explorer repository selection to dropdown (#529)

Context: The Cred Explorer loads data (currently on a per-repository
basis) that has previously been prepared by running the `sourcecred
load` cli command.

Currently, to select a repository to load, the user must manually type
the repository owner and name. This is a confusing UI, because it
suggests that any repository may be chosen, when in fact only repos
already loaded into the data store are available. The user is given no
feedback as to which repositories are valid options.

As of #516, the backend stores a registry listing available
repositories. This commit adds a `RepositorySelect` component which
loads the available from that registry, and makes them available in a
dropdown, in sorted order.

When the user manually selects one of the repositories, that selection
is persisted into localStorage and respected on future loads. If the
user hasn't made such a choice, then the first repository is selected by
default.

The implementation is highly influenced by testability considerations.
The default export, `<RepositorySelect onChange={} localStore={} />`, is
pretty straightforward. The `RepositorySelect` is somewhat cumbersome to
test because it asynchronously fetches data and then updates its state,
which affects the render output. So as to avoid testing inside async
react components wherever possible, I've factored out:

* `loadStatus`, which uses fetch and localStore to get the status of the
selector.
* `PureRepositorySelect`, which just renders a `Status`, such as
loading, failure, or valid
* `LocalStoreRepositorySelect`, which wraps the `PureRepositorySelect`
with logic to bind the repository select to localStore on change.

Test plan: Extensive unit tests were added. Also, to ensure that the
tests were testing the right thing, I manually tested:
- attempting to load invalid registry
- attempting to load with no registry
- attempting to load with empty registry
- loading without valid localStore
- changing the setting via dropdown
- loading from localStore after changing the dropdown

And all behavior was as expected.

Thanks to @wchargin for considerable help testing this PR.

src/app/credExplorer/App.js

@@ -16,13 +16,10 @@ import type {DynamicPluginAdapter} from "../pluginAdapter";
 import {type EdgeEvaluator} from "../../core/attribution/pagerank";
 import {WeightConfig} from "./WeightConfig";
 import type {PagerankNodeDecomposition} from "../../core/attribution/pagerankNodeDecomposition";
+import RepositorySelect, {type Repo} from "./RepositorySelect";
 
 import * as NullUtil from "../../util/null";
 
-const REPO_OWNER_KEY = "repoOwner";
-const REPO_NAME_KEY = "repoName";
-const MAX_ENTRIES_PER_LIST = 100;
-
 export default class AppPage extends React.Component<{||}> {
   static _LOCAL_STORE = new CheckedLocalStore(
     new BrowserLocalStore({
@@ -38,8 +35,7 @@ export default class AppPage extends React.Component<{||}> {
 
 type Props = {|+localStore: LocalStore|};
 type State = {
-  repoOwner: string,
-  repoName: string,
+  selectedRepo: ?Repo,
   data: {|
     graphWithMetadata: ?{|
       +graph: Graph,
@@ -52,28 +48,21 @@ type State = {
   edgeEvaluator: ?EdgeEvaluator,
 };
 
+const MAX_ENTRIES_PER_LIST = 100;
+
 export class App extends React.Component<Props, State> {
   constructor(props: Props) {
     super(props);
     this.state = {
-      repoOwner: "",
-      repoName: "",
+      selectedRepo: null,
       data: {graphWithMetadata: null, pnd: null},
       edgeEvaluator: null,
     };
   }
 
-  componentDidMount() {
-    const {localStore} = this.props;
-    this.setState((state) => ({
-      repoOwner: localStore.get(REPO_OWNER_KEY, state.repoOwner),
-      repoName: localStore.get(REPO_NAME_KEY, state.repoName),
-    }));
-  }
-
   render() {
     const {localStore} = this.props;
-    const {edgeEvaluator} = this.state;
+    const {edgeEvaluator, selectedRepo} = this.state;
     const {graphWithMetadata, pnd} = this.state.data;
     return (
       <div style={{maxWidth: "66em", margin: "0 auto", padding: "0 10px"}}>
@@ -81,33 +70,17 @@ export class App extends React.Component<Props, State> {
           <h1>Cred Explorer</h1>
         </header>
         <div>
-          <label>
-            Repository owner:
-            <input
-              value={this.state.repoOwner}
-              onChange={(e) => {
-                const value = e.target.value;
-                this.setState({repoOwner: value}, () => {
-                  localStore.set(REPO_OWNER_KEY, this.state.repoOwner);
-                });
-              }}
-            />
-          </label>
+          <RepositorySelect
+            localStore={localStore}
+            onChange={(selectedRepo) => this.setState({selectedRepo})}
+          />
           <br />
-          <label>
-            Repository name:
-            <input
-              value={this.state.repoName}
-              onChange={(e) => {
-                const value = e.target.value;
-                this.setState({repoName: value}, () => {
-                  localStore.set(REPO_NAME_KEY, this.state.repoName);
-                });
-              }}
-            />
-          </label>
-          <br />
-          <button onClick={() => this.loadData()}>Load data</button>
+          <button
+            disabled={selectedRepo == null}
+            onClick={() => this.loadData()}
+          >
+            Load data
+          </button>
           <button
             disabled={graphWithMetadata == null || edgeEvaluator == null}
             onClick={() => {
@@ -155,26 +128,20 @@ export class App extends React.Component<Props, State> {
   }
 
   loadData() {
-    const validRe = /^[A-Za-z0-9_-]+$/;
-    const {repoOwner, repoName} = this.state;
-    if (!repoOwner.match(validRe)) {
-      console.error(`Invalid repository owner: ${JSON.stringify(repoOwner)}`);
-      return;
-    }
-    if (!repoName.match(validRe)) {
-      console.error(`Invalid repository name: ${JSON.stringify(repoName)}`);
-      return;
+    const {selectedRepo} = this.state;
+    if (selectedRepo == null) {
+      throw new Error(`Impossible`);
     }
 
     const githubPromise = new GithubAdapter()
-      .load(repoOwner, repoName)
+      .load(selectedRepo.owner, selectedRepo.name)
       .then((adapter) => {
         const graph = adapter.graph();
         return {graph, adapter};
       });
 
     const gitPromise = new GitAdapter()
-      .load(repoOwner, repoName)
+      .load(selectedRepo.owner, selectedRepo.name)
       .then((adapter) => {
         const graph = adapter.graph();
         return {graph, adapter};

src/app/credExplorer/RepositorySelect.js

@@ -0,0 +1,188 @@
+// @flow
+
+import React, {type Node} from "react";
+import sortBy from "lodash.sortby";
+import deepEqual from "lodash.isequal";
+
+import * as NullUtil from "../../util/null";
+import type {LocalStore} from "../localStore";
+
+export const REPO_REGISTRY_API = "/api/v1/data/repositoryRegistry.json";
+export const REPO_KEY = "selectedRepository";
+
+export type Repo = {|+name: string, +owner: string|};
+
+export type Status =
+  | {|+type: "LOADING"|}
+  | {|
+      +type: "VALID",
+      +availableRepos: $ReadOnlyArray<Repo>,
+      +selectedRepo: Repo,
+    |}
+  | {|+type: "NO_REPOS"|}
+  | {|+type: "FAILURE"|};
+
+type Props = {|
+  +onChange: (x: Repo) => void,
+  +localStore: LocalStore,
+|};
+type State = {|status: Status|};
+export default class RepositorySelect extends React.Component<Props, State> {
+  constructor(props: Props) {
+    super(props);
+    this.state = {
+      status: {type: "LOADING"},
+    };
+  }
+
+  componentDidMount() {
+    loadStatus(this.props.localStore).then((status) => {
+      this.setState({status});
+      if (status.type === "VALID") {
+        this.props.onChange(status.selectedRepo);
+      }
+    });
+  }
+
+  onChange(selectedRepo: Repo) {
+    const status = this.state.status;
+    if (status.type === "VALID") {
+      const newStatus = {...status, selectedRepo};
+      this.setState({status: newStatus});
+    }
+    this.props.onChange(selectedRepo);
+  }
+
+  render() {
+    return (
+      <LocalStoreRepositorySelect
+        onChange={(selectedRepo) => this.onChange(selectedRepo)}
+        status={this.state.status}
+        localStore={this.props.localStore}
+      >
+        {({status, onChange}) => (
+          <PureRepositorySelect onChange={onChange} status={status} />
+        )}
+      </LocalStoreRepositorySelect>
+    );
+  }
+}
+
+function validateRepo(repo: Repo) {
+  const validRe = /^[A-Za-z0-9_-]+$/;
+  if (!repo.owner.match(validRe)) {
+    throw new Error(`Invalid repository owner: ${JSON.stringify(repo.owner)}`);
+  }
+  if (!repo.name.match(validRe)) {
+    throw new Error(`Invalid repository name: ${JSON.stringify(repo.name)}`);
+  }
+}
+
+function repoStringToRepo(x: string): Repo {
+  const pieces = x.split("/");
+  if (pieces.length !== 2) {
+    throw new Error(`Invalid repo string: ${x}`);
+  }
+  const repo = {owner: pieces[0], name: pieces[1]};
+  validateRepo(repo);
+  return repo;
+}
+
+export async function loadStatus(localStore: LocalStore): Promise<Status> {
+  try {
+    const response = await fetch(REPO_REGISTRY_API);
+    if (!response.ok) {
+      return {type: "FAILURE"};
+    }
+    const json = await response.json();
+    let availableRepos = Object.keys(json).map(repoStringToRepo);
+    availableRepos = sortBy(availableRepos, (r) => r.owner, (r) => r.name);
+    if (availableRepos.length === 0) {
+      return {type: "NO_REPOS"};
+    }
+    const localStoreRepo = localStore.get(REPO_KEY, null);
+    const selectedRepo = NullUtil.orElse(
+      availableRepos.find((x) => deepEqual(x, localStoreRepo)),
+      availableRepos[0]
+    );
+    return {type: "VALID", availableRepos, selectedRepo};
+  } catch (e) {
+    return {type: "FAILURE"};
+  }
+}
+
+export class LocalStoreRepositorySelect extends React.Component<{|
+  +status: Status,
+  +onChange: (repo: Repo) => void,
+  +localStore: LocalStore,
+  +children: ({
+    status: Status,
+    onChange: (selectedRepo: Repo) => void,
+  }) => Node,
+|}> {
+  render() {
+    return this.props.children({
+      status: this.props.status,
+      onChange: (repo) => {
+        this.props.onChange(repo);
+        this.props.localStore.set(REPO_KEY, repo);
+      },
+    });
+  }
+}
+
+type PureRepositorySelectProps = {|
+  +onChange: (x: Repo) => void,
+  +status: Status,
+|};
+export class PureRepositorySelect extends React.PureComponent<
+  PureRepositorySelectProps
+> {
+  renderSelect(availableRepos: $ReadOnlyArray<Repo>, selectedRepo: ?Repo) {
+    return (
+      <label>
+        <span>Please choose a repository to inspect:</span>{" "}
+        {selectedRepo != null && (
+          <select
+            value={`${selectedRepo.owner}/${selectedRepo.name}`}
+            onChange={(e) => {
+              const repoString = e.target.value;
+              const repo = repoStringToRepo(repoString);
+              this.props.onChange(repo);
+            }}
+          >
+            {availableRepos.map(({owner, name}) => {
+              const repoString = `${owner}/${name}`;
+              return (
+                <option value={repoString} key={repoString}>
+                  {repoString}
+                </option>
+              );
+            })}
+          </select>
+        )}
+      </label>
+    );
+  }
+
+  renderError(text: string) {
+    return <span style={{fontWeight: "bold", color: "red"}}>{text}</span>;
+  }
+
+  render() {
+    const {status} = this.props;
+    switch (status.type) {
+      case "LOADING":
+        // Just show an empty select while we wait.
+        return this.renderSelect([], null);
+      case "VALID":
+        return this.renderSelect(status.availableRepos, status.selectedRepo);
+      case "NO_REPOS":
+        return this.renderError("Error: No repositories found.");
+      case "FAILURE":
+        return this.renderError("Error: Unable to load repository registry.");
+      default:
+        throw new Error((status.type: empty));
+    }
+  }
+}

src/app/credExplorer/RepositorySelect.test.js

@@ -0,0 +1,362 @@
+// @flow
+import React from "react";
+import {shallow, mount} from "enzyme";
+
+import * as NullUtil from "../../util/null";
+import testLocalStore from "../testLocalStore";
+import RepositorySelect, {
+  PureRepositorySelect,
+  LocalStoreRepositorySelect,
+  loadStatus,
+  type Status,
+  REPO_KEY,
+  REPO_REGISTRY_API,
+} from "./RepositorySelect";
+
+require("../testUtil").configureEnzyme();
+require("../testUtil").configureAphrodite();
+
+describe("app/credExplorer/RepositorySelect", () => {
+  beforeEach(() => {
+    fetch.resetMocks();
+  });
+  describe("PureRepositorySelect", () => {
+    it("doesn't render a select while loading", () => {
+      const e = shallow(
+        <PureRepositorySelect status={{type: "LOADING"}} onChange={jest.fn()} />
+      );
+      const span = e.find("span");
+      expect(span.text()).toBe("Please choose a repository to inspect:");
+      const select = e.find("select");
+      expect(select).toHaveLength(0);
+    });
+    it("renders an error message if no repositories are available", () => {
+      const e = shallow(
+        <PureRepositorySelect
+          status={{type: "NO_REPOS"}}
+          onChange={jest.fn()}
+        />
+      );
+      const span = e.find("span");
+      expect(span.text()).toBe("Error: No repositories found.");
+    });
+    it("renders an error message if there was an error while loading", () => {
+      const e = shallow(
+        <PureRepositorySelect status={{type: "FAILURE"}} onChange={jest.fn()} />
+      );
+      const span = e.find("span");
+      expect(span.text()).toBe("Error: Unable to load repository registry.");
+    });
+    it("renders a select with all available repos as options", () => {
+      const availableRepos = [
+        {owner: "foo", name: "bar"},
+        {owner: "zod", name: "zoink"},
+      ];
+      const selectedRepo = availableRepos[0];
+      const e = shallow(
+        <PureRepositorySelect
+          status={{type: "VALID", availableRepos, selectedRepo}}
+          onChange={jest.fn()}
+        />
+      );
+      const options = e.find("option");
+      expect(options.map((x) => x.text())).toEqual(["foo/bar", "zod/zoink"]);
+    });
+    it("the selectedRepo is selected", () => {
+      const availableRepos = [
+        {owner: "foo", name: "bar"},
+        {owner: "zod", name: "zoink"},
+      ];
+      const selectedRepo = availableRepos[0];
+      const e = shallow(
+        <PureRepositorySelect
+          status={{type: "VALID", availableRepos, selectedRepo}}
+          onChange={jest.fn()}
+        />
+      );
+      expect(e.find("select").prop("value")).toBe("foo/bar");
+    });
+    it("clicking an option triggers the onChange", () => {
+      const availableRepos = [
+        {owner: "foo", name: "bar"},
+        {owner: "zod", name: "zoink"},
+      ];
+      const onChange = jest.fn();
+      const e = shallow(
+        <PureRepositorySelect
+          status={{
+            type: "VALID",
+            availableRepos,
+            selectedRepo: availableRepos[0],
+          }}
+          onChange={onChange}
+        />
+      );
+      e.find("select").simulate("change", {target: {value: "zod/zoink"}});
+      expect(onChange).toHaveBeenCalledTimes(1);
+      expect(onChange).toHaveBeenLastCalledWith(availableRepos[1]);
+    });
+  });
+
+  describe("loadStatus", () => {
+    function expectLoadValidStatus(
+      fetchReturn,
+      localStore,
+      expectedAvailableRepos,
+      expectedSelectedRepo
+    ) {
+      fetch.mockResponseOnce(fetchReturn);
+      const result = loadStatus(localStore);
+      expect(fetch).toHaveBeenCalledTimes(1);
+      expect(fetch).toHaveBeenCalledWith(REPO_REGISTRY_API);
+      expect.assertions(5);
+      return result.then((status) => {
+        expect(status.type).toBe("VALID");
+        if (status.type !== "VALID") {
+          throw new Error("Impossible");
+        }
+        expect(status.availableRepos).toEqual(expectedAvailableRepos);
+        expect(status.selectedRepo).toEqual(expectedSelectedRepo);
+      });
+    }
+    it("calls fetch and handles a simple success", () => {
+      const fetchResult = JSON.stringify({"foo/bar": true});
+      const repo = {owner: "foo", name: "bar"};
+      return expectLoadValidStatus(fetchResult, testLocalStore(), [repo], repo);
+    });
+    it("returns repos in sorted order, and selects the first repo", () => {
+      const fetchResult = JSON.stringify({
+        "foo/bar": true,
+        "a/z": true,
+        "a/b": true,
+      });
+      const repos = [
+        {owner: "a", name: "b"},
+        {owner: "a", name: "z"},
+        {owner: "foo", name: "bar"},
+      ];
+      return expectLoadValidStatus(
+        fetchResult,
+        testLocalStore(),
+        repos,
+        repos[0]
+      );
+    });
+    it("returns FAILURE on invalid fetch response", () => {
+      fetch.mockResponseOnce(JSON.stringify(["hello"]));
+      expect.assertions(1);
+      return loadStatus(testLocalStore()).then((status) => {
+        expect(status).toEqual({type: "FAILURE"});
+      });
+    });
+    it("returns FAILURE on fetch failure", () => {
+      fetch.mockReject(new Error("some failure"));
+      expect.assertions(1);
+      return loadStatus(testLocalStore()).then((status) => {
+        expect(status).toEqual({type: "FAILURE"});
+      });
+    });
+    it("loads selectedRepo from localStore, if available", () => {
+      const fetchResult = JSON.stringify({
+        "foo/bar": true,
+        "a/z": true,
+        "a/b": true,
+      });
+      const repos = [
+        {owner: "a", name: "b"},
+        {owner: "a", name: "z"},
+        {owner: "foo", name: "bar"},
+      ];
+      const localStore = testLocalStore();
+      localStore.set(REPO_KEY, {owner: "a", name: "z"});
+      return expectLoadValidStatus(fetchResult, localStore, repos, repos[1]);
+    });
+    it("ignores selectedRepo from localStore, if not available", () => {
+      const fetchResult = JSON.stringify({
+        "foo/bar": true,
+        "a/z": true,
+        "a/b": true,
+      });
+      const repos = [
+        {owner: "a", name: "b"},
+        {owner: "a", name: "z"},
+        {owner: "foo", name: "bar"},
+      ];
+      const localStore = testLocalStore();
+      localStore.set(REPO_KEY, {owner: "non", name: "existent"});
+      return expectLoadValidStatus(fetchResult, localStore, repos, repos[0]);
+    });
+    it("ignores malformed value in localStore", () => {
+      const fetchResult = JSON.stringify({
+        "foo/bar": true,
+        "a/z": true,
+        "a/b": true,
+      });
+      const repos = [
+        {owner: "a", name: "b"},
+        {owner: "a", name: "z"},
+        {owner: "foo", name: "bar"},
+      ];
+      const localStore = testLocalStore();
+      localStore.set(REPO_KEY, 42);
+      return expectLoadValidStatus(fetchResult, localStore, repos, repos[0]);
+    });
+  });
+
+  describe("LocalStoreRepositorySelect", () => {
+    it("instantiates the child component", () => {
+      const status = {type: "LOADING"};
+      const onChange = jest.fn();
+      const e = shallow(
+        <LocalStoreRepositorySelect
+          onChange={onChange}
+          status={status}
+          localStore={testLocalStore()}
+        >
+          {({status, onChange}) => (
+            <PureRepositorySelect status={status} onChange={onChange} />
+          )}
+        </LocalStoreRepositorySelect>
+      );
+      const child = e.find("PureRepositorySelect");
+      expect(child.props().status).toEqual(status);
+    });
+    it("passes onChange result up to parent", () => {
+      const status = {type: "LOADING"};
+      const onChange = jest.fn();
+      let childOnChange;
+      shallow(
+        <LocalStoreRepositorySelect
+          onChange={onChange}
+          status={status}
+          localStore={testLocalStore()}
+        >
+          {({status, onChange}) => {
+            childOnChange = onChange;
+            return <PureRepositorySelect status={status} onChange={onChange} />;
+          }}
+        </LocalStoreRepositorySelect>
+      );
+      const repo = {owner: "foo", name: "bar"};
+      NullUtil.get(childOnChange)(repo);
+      expect(onChange).toHaveBeenCalledWith(repo);
+      expect(onChange).toHaveBeenCalledTimes(1);
+    });
+    it("stores onChange result in localStore", () => {
+      const status = {type: "LOADING"};
+      const onChange = jest.fn();
+      const localStore = testLocalStore();
+      let childOnChange;
+      shallow(
+        <LocalStoreRepositorySelect
+          onChange={onChange}
+          status={status}
+          localStore={localStore}
+        >
+          {({status, onChange}) => {
+            childOnChange = onChange;
+            return <PureRepositorySelect status={status} onChange={onChange} />;
+          }}
+        </LocalStoreRepositorySelect>
+      );
+      const repo = {owner: "foo", name: "bar"};
+      NullUtil.get(childOnChange)(repo);
+      expect(localStore.get(REPO_KEY)).toEqual(repo);
+    });
+  });
+
+  describe("RepositorySelect", () => {
+    it("initially renders a LocalStoreRepositorySelect with status LOADING", () => {
+      const e = shallow(
+        <RepositorySelect onChange={jest.fn()} localStore={testLocalStore()} />
+      );
+      const child = e.find(LocalStoreRepositorySelect);
+      const status = child.props().status;
+      const onChange = jest.fn();
+      expect(status).toEqual({type: "LOADING"});
+      const grandChild = child.props().children({status, onChange});
+      expect(grandChild.type).toBe(PureRepositorySelect);
+    });
+
+    function waitForUpdate(enzymeWrapper) {
+      return new Promise((resolve) => {
+        setImmediate(() => {
+          enzymeWrapper.update();
+          resolve();
+        });
+      });
+    }
+
+    it("on successful load, sets the status on the child", async () => {
+      const onChange = jest.fn();
+      fetch.mockResponseOnce(JSON.stringify({"foo/bar": true}));
+      const e = shallow(
+        <RepositorySelect onChange={onChange} localStore={testLocalStore()} />
+      );
+      await waitForUpdate(e);
+      const childStatus = e.props().status;
+      const selectedRepo = {owner: "foo", name: "bar"};
+      const availableRepos = [selectedRepo];
+      expect(childStatus).toEqual({
+        type: "VALID",
+        selectedRepo,
+        availableRepos,
+      });
+    });
+
+    it("on successful load, passes the status to the onChange", async () => {
+      const onChange = jest.fn();
+      fetch.mockResponseOnce(JSON.stringify({"foo/bar": true}));
+      const e = shallow(
+        <RepositorySelect onChange={onChange} localStore={testLocalStore()} />
+      );
+      await waitForUpdate(e);
+      expect(onChange).toHaveBeenCalledTimes(1);
+      expect(onChange).toHaveBeenCalledWith({
+        owner: "foo",
+        name: "bar",
+      });
+    });
+
+    it("on failed load, onChange not called", async () => {
+      const onChange = jest.fn();
+      fetch.mockReject(new Error("something bad"));
+
+      const e = shallow(
+        <RepositorySelect onChange={onChange} localStore={testLocalStore()} />
+      );
+      await waitForUpdate(e);
+      expect(onChange).toHaveBeenCalledTimes(0);
+    });
+
+    it("child onChange triggers parent onChange", () => {
+      const onChange = jest.fn();
+      const e = mount(
+        <RepositorySelect onChange={onChange} localStore={testLocalStore()} />
+      );
+      const child = e.find(PureRepositorySelect);
+      const repo = {owner: "foo", name: "bar"};
+      child.props().onChange(repo);
+      expect(onChange).toHaveBeenCalledTimes(1);
+      expect(onChange).toHaveBeenCalledWith(repo);
+    });
+
+    it("selecting child option updates top-level state", async () => {
+      const onChange = jest.fn();
+      fetch.mockResponseOnce(JSON.stringify({"foo/bar": true, "z/a": true}));
+      const e = mount(
+        <RepositorySelect onChange={onChange} localStore={testLocalStore()} />
+      );
+      await waitForUpdate(e);
+      const child = e.find(PureRepositorySelect);
+      const repo = {owner: "z", name: "a"};
+      child.props().onChange(repo);
+      const status: Status = e.state().status;
+      expect(status.type).toEqual("VALID");
+      if (status.type !== "VALID") {
+        throw new Error("Impossible");
+      }
+      expect(status.selectedRepo).toEqual(repo);
+    });
+  });
+});