Default to selecting last loaded repository (#531)

In #529, I made the cred explorer populate a dropdown with the list of
repositories that are available to explore. That dropdown defaults to
selecting the alphabetically first repository.

This has an unfortunate consequence in that it makes it impossible for
us to explicitly set a default - for example, we would like
sourcecred.github.io/explorer to show sourcecred/sourcecred by default,
but instead it shows example-git.

So that we can choose the default, I've changed the logic so that it
instead shows the most-recently-loaded data first. This required
a breaking change to the repoRegistry serialized format, so I've also
refactored the module to use compat, which I should have done from the
beginning.

Test plan:
Unit tests for the repo selector are updated. The CLI load command
unfortunately has no tests, so I manually tested that it always provides
the lastest repository last, and appropriately handles the case where
the same repository is loaded multiple times.

src/app/credExplorer/App.js

@@ -16,7 +16,8 @@ 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 RepositorySelect from "./RepositorySelect";
+import type {Repo} from "./repoRegistry";
 
 import * as NullUtil from "../../util/null";
 

src/app/credExplorer/RepositorySelect.js

@@ -7,11 +7,9 @@ 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";
+import {type Repo, fromJSON, REPO_REGISTRY_API} from "./repoRegistry";
 export const REPO_KEY = "selectedRepository";
 
-export type Repo = {|+name: string, +owner: string|};
-
 export type Status =
   | {|+type: "LOADING"|}
   | {|
@@ -95,17 +93,17 @@ export async function loadStatus(localStore: LocalStore): Promise<Status> {
       return {type: "FAILURE"};
     }
     const json = await response.json();
-    let availableRepos = Object.keys(json).map(repoStringToRepo);
+    const availableRepos = fromJSON(json);
-    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]
+      availableRepos[availableRepos.length - 1]
     );
-    return {type: "VALID", availableRepos, selectedRepo};
+    const sortedRepos = sortBy(availableRepos, (r) => r.owner, (r) => r.name);
+    return {type: "VALID", availableRepos: sortedRepos, selectedRepo};
   } catch (e) {
     return {type: "FAILURE"};
   }

src/app/credExplorer/RepositorySelect.test.js

@@ -10,9 +10,10 @@ import RepositorySelect, {
   loadStatus,
   type Status,
   REPO_KEY,
-  REPO_REGISTRY_API,
 } from "./RepositorySelect";
 
+import {toJSON, type RepoRegistry, REPO_REGISTRY_API} from "./repoRegistry";
+
 require("../testUtil").configureEnzyme();
 require("../testUtil").configureAphrodite();
 
@@ -20,6 +21,10 @@ describe("app/credExplorer/RepositorySelect", () => {
   beforeEach(() => {
     fetch.resetMocks();
   });
+
+  function mockRegistry(registry: RepoRegistry) {
+    fetch.mockResponseOnce(JSON.stringify(toJSON(registry)));
+  }
   describe("PureRepositorySelect", () => {
     it("doesn't render a select while loading", () => {
       const e = shallow(
@@ -100,12 +105,10 @@ describe("app/credExplorer/RepositorySelect", () => {
 
   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);
@@ -120,27 +123,19 @@ describe("app/credExplorer/RepositorySelect", () => {
       });
     }
     it("calls fetch and handles a simple success", () => {
-      const fetchResult = JSON.stringify({"foo/bar": true});
+      mockRegistry([{owner: "foo", name: "bar"}]);
       const repo = {owner: "foo", name: "bar"};
-      return expectLoadValidStatus(fetchResult, testLocalStore(), [repo], repo);
+      return expectLoadValidStatus(testLocalStore(), [repo], repo);
     });
-    it("returns repos in sorted order, and selects the first repo", () => {
+    it("returns repos in sorted order, and selects the last 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(
+      const nonSortedRepos = [repos[2], repos[0], repos[1]];
-        fetchResult,
+      mockRegistry(nonSortedRepos);
-        testLocalStore(),
+      return expectLoadValidStatus(testLocalStore(), repos, repos[1]);
-        repos,
-        repos[0]
-      );
     });
     it("returns FAILURE on invalid fetch response", () => {
       fetch.mockResponseOnce(JSON.stringify(["hello"]));
@@ -157,49 +152,37 @@ describe("app/credExplorer/RepositorySelect", () => {
       });
     });
     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"},
       ];
+      mockRegistry(repos);
       const localStore = testLocalStore();
       localStore.set(REPO_KEY, {owner: "a", name: "z"});
-      return expectLoadValidStatus(fetchResult, localStore, repos, repos[1]);
+      return expectLoadValidStatus(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"},
       ];
+      mockRegistry(repos);
       const localStore = testLocalStore();
       localStore.set(REPO_KEY, {owner: "non", name: "existent"});
-      return expectLoadValidStatus(fetchResult, localStore, repos, repos[0]);
+      return expectLoadValidStatus(localStore, repos, repos[2]);
     });
     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"},
       ];
+      mockRegistry(repos);
       const localStore = testLocalStore();
       localStore.set(REPO_KEY, 42);
-      return expectLoadValidStatus(fetchResult, localStore, repos, repos[0]);
+      return expectLoadValidStatus(localStore, repos, repos[2]);
     });
   });
 
@@ -289,13 +272,13 @@ describe("app/credExplorer/RepositorySelect", () => {
 
     it("on successful load, sets the status on the child", async () => {
       const onChange = jest.fn();
-      fetch.mockResponseOnce(JSON.stringify({"foo/bar": true}));
+      const selectedRepo = {owner: "foo", name: "bar"};
+      mockRegistry([selectedRepo]);
       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",
@@ -306,16 +289,17 @@ describe("app/credExplorer/RepositorySelect", () => {
 
     it("on successful load, passes the status to the onChange", async () => {
       const onChange = jest.fn();
-      fetch.mockResponseOnce(JSON.stringify({"foo/bar": true}));
+      const repo = {
+        owner: "foo",
+        name: "bar",
+      };
+      mockRegistry([repo]);
       const e = shallow(
         <RepositorySelect onChange={onChange} localStore={testLocalStore()} />
       );
       await waitForUpdate(e);
       expect(onChange).toHaveBeenCalledTimes(1);
-      expect(onChange).toHaveBeenCalledWith({
+      expect(onChange).toHaveBeenCalledWith(repo);
-        owner: "foo",
-        name: "bar",
-      });
     });
 
     it("on failed load, onChange not called", async () => {
@@ -343,20 +327,20 @@ describe("app/credExplorer/RepositorySelect", () => {
 
     it("selecting child option updates top-level state", async () => {
       const onChange = jest.fn();
-      fetch.mockResponseOnce(JSON.stringify({"foo/bar": true, "z/a": true}));
+      const repos = [{owner: "foo", name: "bar"}, {owner: "z", name: "a"}];
+      mockRegistry(repos);
       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(repos[0]);
-      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);
+      expect(status.selectedRepo).toEqual(repos[0]);
     });
   });
 });

src/app/credExplorer/repoRegistry.js

@@ -0,0 +1,32 @@
+// @flow
+
+// The repoRegistry is written by the CLI load command
+// (src/cli/commands/load.js) and is read by the RepositorySelect component
+// (src/app/credExplorer/RepositorySelect.js)
+import deepEqual from "lodash.isequal";
+import {toCompat, fromCompat, type Compatible} from "../../util/compat";
+
+export const REPO_REGISTRY_FILE = "repositoryRegistry.json";
+export const REPO_REGISTRY_API = "/api/v1/data/repositoryRegistry.json";
+
+const REPO_REGISTRY_COMPAT = {type: "REPO_REGISTRY", version: "0.1.0"};
+
+export type Repo = {|+name: string, +owner: string|};
+export type RepoRegistry = $ReadOnlyArray<Repo>;
+export type RepoRegistryJSON = Compatible<RepoRegistry>;
+
+export function toJSON(r: RepoRegistry): RepoRegistryJSON {
+  return toCompat(REPO_REGISTRY_COMPAT, r);
+}
+
+export function fromJSON(j: RepoRegistryJSON): RepoRegistry {
+  return fromCompat(REPO_REGISTRY_COMPAT, j);
+}
+
+export function addRepo(r: Repo, reg: RepoRegistry): RepoRegistry {
+  return [...reg.filter((x) => !deepEqual(x, r)), r];
+}
+
+export function emptyRegistry(): RepoRegistry {
+  return [];
+}

src/app/credExplorer/repoRegistry.test.js

@@ -0,0 +1,53 @@
+// @flow
+
+import {
+  toJSON,
+  fromJSON,
+  addRepo,
+  emptyRegistry,
+  type RepoRegistry,
+} from "./repoRegistry";
+
+describe("app/credExplorer/repoRegistry", () => {
+  const r = (owner, name) => ({owner, name});
+  describe("to/fromJSON compose on", () => {
+    function checkExample(x: RepoRegistry) {
+      expect(fromJSON(toJSON(x))).toEqual(x);
+      expect(toJSON(fromJSON(toJSON(x)))).toEqual(toJSON(x));
+    }
+    it("empty registry", () => {
+      checkExample(emptyRegistry());
+    });
+    it("nonempty registry", () => {
+      checkExample([r("foo", "bar"), r("zoo", "zod")]);
+    });
+  });
+  describe("addRepo", () => {
+    it("adds to empty registry", () => {
+      expect(addRepo(r("foo", "bar"), emptyRegistry())).toEqual([
+        r("foo", "bar"),
+      ]);
+    });
+    it("adds to nonempty registry", () => {
+      const registry = [r("foo", "bar")];
+      expect(addRepo(r("zoo", "zod"), registry)).toEqual([
+        r("foo", "bar"),
+        r("zoo", "zod"),
+      ]);
+    });
+    it("adding repo that is already the last has no effect", () => {
+      const registry = [r("zoo", "zod"), r("foo", "bar")];
+      expect(addRepo(r("foo", "bar"), registry)).toEqual(registry);
+    });
+    it("adding already-existing repo shifts it to the end", () => {
+      const registry = [r("zoo", "zod"), r("foo", "bar")];
+      expect(addRepo(r("zoo", "zod"), registry)).toEqual([
+        r("foo", "bar"),
+        r("zoo", "zod"),
+      ]);
+    });
+  });
+  it("empty registry is empty", () => {
+    expect(emptyRegistry()).toEqual([]);
+  });
+});

src/cli/commands/load.js

@@ -14,6 +14,14 @@ import {
   sourcecredDirectoryFlag,
 } from "../common";
 
+import {
+  toJSON,
+  fromJSON,
+  addRepo,
+  emptyRegistry,
+  REPO_REGISTRY_FILE,
+} from "../../app/credExplorer/repoRegistry";
+
 const execDependencyGraph = require("../../tools/execDependencyGraph").default;
 
 export default class PluginGraphCommand extends Command {
@@ -152,21 +160,21 @@ function loadPlugin({basedir, plugin, repoOwner, repoName, githubToken}) {
   }
 }
 
-const REPO_REGISTRY_FILE = "repositoryRegistry.json";
-
 function addToRepoRegistry(options) {
   // TODO: Make this function transactional before loading repositories in
   // parallel.
   const {basedir, repoOwner, repoName} = options;
+  const repo = {owner: repoOwner, name: repoName};
   const outputFile = path.join(basedir, REPO_REGISTRY_FILE);
   let registry = null;
   if (fs.existsSync(outputFile)) {
     const contents = fs.readFileSync(outputFile);
-    registry = JSON.parse(contents.toString());
+    const registryJSON = JSON.parse(contents.toString());
+    registry = fromJSON(registryJSON);
   } else {
-    registry = {};
+    registry = emptyRegistry();
   }
+  registry = addRepo(repo, registry);
 
-  registry[`${repoOwner}/${repoName}`] = true;
+  fs.writeFileSync(outputFile, stringify(toJSON(registry)));
-  fs.writeFileSync(outputFile, stringify(registry));
 }