From 3eb09fcd63c477b91aed7fab27ab2746a5c94005 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Wouts?= Date: Fri, 19 Apr 2019 21:05:08 +1000 Subject: [PATCH] Add a status at the top, with a refresh status and link (#148) This fixes #146. --- package.json | 2 + src/components/Error.tsx | 21 ---- src/components/Popup.tsx | 184 ++++++++++++++++---------------- src/components/Status.tsx | 45 ++++++++ src/components/design/Link.tsx | 1 + src/environment/testing/fake.ts | 1 + src/popup.tsx | 4 +- src/state/core.spec.ts | 59 ++++------ src/state/core.ts | 33 ++++-- src/storage/api.ts | 5 + src/storage/implementation.ts | 5 + src/storage/loaded-state.ts | 8 ++ yarn.lock | 12 +++ 13 files changed, 222 insertions(+), 158 deletions(-) delete mode 100644 src/components/Error.tsx create mode 100644 src/components/Status.tsx diff --git a/package.json b/package.json index 1eb26f14..e4326e36 100644 --- a/package.json +++ b/package.json @@ -6,10 +6,12 @@ "@fortawesome/fontawesome-svg-core": "^1.2.17", "@fortawesome/free-solid-svg-icons": "^5.8.1", "@fortawesome/react-fontawesome": "^0.1.4", + "@types/moment": "^2.13.0", "assert-never": "^1.1.0", "bootstrap": "^4.3.1", "mobx": "^5.9.4", "mobx-react-lite": "^1.2.0", + "moment": "^2.24.0", "react": "^16.8.6", "react-bootstrap": "^1.0.0-beta.8", "react-dom": "^16.8.6", diff --git a/src/components/Error.tsx b/src/components/Error.tsx deleted file mode 100644 index a8f8c9e1..00000000 --- a/src/components/Error.tsx +++ /dev/null @@ -1,21 +0,0 @@ -import styled from "@emotion/styled"; -import { observer } from "mobx-react-lite"; -import React from "react"; - -export interface ErrorProps { - lastError: string | null; -} - -const ErrorContainer = styled.p` - border: 1px solid #d00; - background: #fdd; - color: #400; - padding: 8px; -`; - -export const Error = observer((props: ErrorProps) => { - if (!props.lastError) { - return <>; - } - return Error: {props.lastError}; -}); diff --git a/src/components/Popup.tsx b/src/components/Popup.tsx index 63e5a2b5..da22ec0d 100644 --- a/src/components/Popup.tsx +++ b/src/components/Popup.tsx @@ -1,12 +1,12 @@ import { observer } from "mobx-react-lite"; -import React, { useEffect, useState } from "react"; +import React, { useState } from "react"; import { Badge, Tab, Tabs } from "react-bootstrap"; import { Filter } from "../filtering/filters"; import { Core } from "../state/core"; import { PullRequest } from "../storage/loaded-state"; -import { Error } from "./Error"; import { PullRequestList } from "./PullRequestList"; import { Settings } from "./Settings"; +import { Status } from "./Status"; export interface PopupProps { core: Core; @@ -21,13 +21,6 @@ export const Popup = observer((props: PopupProps) => { currentFilter: Filter.INCOMING }); - useEffect(() => { - props.core - .load() - .then(() => props.core.refreshPullRequests()) - .catch(console.error); - }, []); - const onOpen = (pullRequestUrl: string) => { props.core.openPullRequest(pullRequestUrl).catch(console.error); }; @@ -47,93 +40,100 @@ export const Popup = observer((props: PopupProps) => { return ( <> - - {props.core.token && !props.core.lastError && ( - <> - setState({ currentFilter: key })} - > - - Incoming PRs{" "} - {props.core.filteredPullRequests && ( - 0 - ? "danger" - : "secondary" - } - > - {props.core.filteredPullRequests.incoming.length} - - )} - + + {props.core.token && + // Don't show the list if there was an error, we're not refreshing + // anymore (because of the error) and we don't have any loaded state. + !( + props.core.lastError && + !props.core.refreshing && + !props.core.loadedState + ) && ( + <> + setState({ currentFilter: key })} + > + + Incoming PRs{" "} + {props.core.filteredPullRequests && ( + 0 + ? "danger" + : "secondary" + } + > + {props.core.filteredPullRequests.incoming.length} + + )} + + } + eventKey={Filter.INCOMING} + /> + + Muted{" "} + {props.core.filteredPullRequests && ( + + {props.core.filteredPullRequests.muted.length} + + )} + + } + eventKey={Filter.MUTED} + /> + + Already reviewed{" "} + {props.core.filteredPullRequests && ( + + {props.core.filteredPullRequests.reviewed.length} + + )} + + } + eventKey={Filter.REVIEWED} + /> + + My PRs{" "} + {props.core.filteredPullRequests && ( + + {props.core.filteredPullRequests.mine.length} + + )} + + } + eventKey={Filter.MINE} + /> + + - - Muted{" "} - {props.core.filteredPullRequests && ( - - {props.core.filteredPullRequests.muted.length} - - )} - + emptyMessage={ + state.currentFilter === Filter.INCOMING + ? `Nothing to review, yay!` + : `There's nothing to see here.` } - eventKey={Filter.MUTED} - /> - - Already reviewed{" "} - {props.core.filteredPullRequests && ( - - {props.core.filteredPullRequests.reviewed.length} - - )} - - } - eventKey={Filter.REVIEWED} - /> - - My PRs{" "} - {props.core.filteredPullRequests && ( - - {props.core.filteredPullRequests.mine.length} - - )} - + allowMuting={ + state.currentFilter === Filter.INCOMING || + state.currentFilter === Filter.MUTED } - eventKey={Filter.MINE} + onOpen={onOpen} + onMute={onMute} /> - - - - )} + + )} {props.core.overallStatus !== "loading" && } ); diff --git a/src/components/Status.tsx b/src/components/Status.tsx new file mode 100644 index 00000000..6dd0091f --- /dev/null +++ b/src/components/Status.tsx @@ -0,0 +1,45 @@ +import { observer } from "mobx-react-lite"; +import moment from "moment"; +import React from "react"; +import { Alert } from "react-bootstrap"; +import { Core } from "../state/core"; +import { Link } from "./design/Link"; + +export interface StatusProps { + core: Core; +} + +export const Status = observer((props: StatusProps) => { + // TODO: Refresh button. + // TODO: Ensure it works on dev. + let lastUpdated; + if (props.core.loadedState && props.core.loadedState.startRefreshTimestamp) { + lastUpdated = ( +
+ Last updated{" "} + {moment(props.core.loadedState.startRefreshTimestamp).fromNow()} + {". "} + {props.core.refreshing ? ( + "Refreshing..." + ) : ( + { + props.core.triggerBackgroundRefresh(); + }} + > + Refresh now + + )} +
+ ); + } + if (props.core.lastError) { + return ( + +
Error: {props.core.lastError}
+ {lastUpdated} + + ); + } + return lastUpdated ? {lastUpdated} : <>; +}); diff --git a/src/components/design/Link.tsx b/src/components/design/Link.tsx index 20acc47d..d7aae9e8 100644 --- a/src/components/design/Link.tsx +++ b/src/components/design/Link.tsx @@ -4,4 +4,5 @@ export const Link = styled.a` text-decoration: none; font-weight: bold; color: #000; + cursor: pointer; `; diff --git a/src/environment/testing/fake.ts b/src/environment/testing/fake.ts index 4c1ae85e..46d73248 100644 --- a/src/environment/testing/fake.ts +++ b/src/environment/testing/fake.ts @@ -37,6 +37,7 @@ function fakeStore() { return { lastError: fakeStorage(null), lastCheck: fakeStorage(null), + currentlyRefreshing: fakeStorage(false), muteConfiguration: fakeStorage(NOTHING_MUTED), notifiedPullRequests: fakeStorage([]), token: fakeStorage(null), diff --git a/src/popup.tsx b/src/popup.tsx index 4146d63e..0c15a239 100644 --- a/src/popup.tsx +++ b/src/popup.tsx @@ -12,6 +12,8 @@ import { Core } from "./state/core"; library.add(faBellSlash); const env = buildEnvironment(chromeApiSingleton); +const core = new Core(env); +core.load().catch(console.error); ReactDOM.render( <> @@ -38,7 +40,7 @@ ReactDOM.render( } `} /> - + , document.getElementById("root") ); diff --git a/src/state/core.spec.ts b/src/state/core.spec.ts index 22682bff..cdef9479 100644 --- a/src/state/core.spec.ts +++ b/src/state/core.spec.ts @@ -16,6 +16,7 @@ describe("Core", () => { // Other things are stored, they should be ignored. env.store.lastError.currentValue = "error"; + env.store.currentlyRefreshing.currentValue = true; env.store.lastCheck.currentValue = { userLogin: "fwouts", repos: [], @@ -79,6 +80,7 @@ describe("Core", () => { ] }; env.store.lastError.currentValue = "error"; + env.store.currentlyRefreshing.currentValue = true; env.store.lastCheck.currentValue = state; env.store.notifiedPullRequests.currentValue = notifiedPullRequestUrls; env.store.muteConfiguration.currentValue = muteConfiguration; @@ -88,7 +90,7 @@ describe("Core", () => { expect(core.token).toEqual("valid-token"); expect(core.lastError).toEqual("error"); - expect(core.refreshing).toBe(false); + expect(core.refreshing).toBe(true); expect(core.loadedState).toEqual(state); expect(core.muteConfiguration).toEqual(muteConfiguration); expect(Array.from(core.notifiedPullRequestUrls)).toEqual( @@ -156,6 +158,7 @@ describe("Core", () => { repos: [], openPullRequests: [] }; + env.store.currentlyRefreshing.currentValue = true; env.store.lastCheck.currentValue = state; // Initialise. @@ -166,6 +169,7 @@ describe("Core", () => { expect(core.loadedState).toBeNull(); expect(env.store.token.currentValue).toEqual("token-kevin"); expect(env.store.lastError.currentValue).toEqual(null); + expect(env.store.currentlyRefreshing.currentValue).toBe(false); expect(env.store.notifiedPullRequests.currentValue).toEqual([]); expect(env.store.lastCheck.currentValue).toEqual(null); expect(env.store.muteConfiguration.currentValue).toEqual(NOTHING_MUTED); @@ -207,39 +211,6 @@ describe("Core", () => { expect(core.lastError).toBeNull(); }); - it("updates badge when it starts refreshing", async () => { - const env = buildTestingEnvironment(); - const core = new Core(env); - env.store.token.currentValue = "valid-token"; - - // Initialise. - await core.load(); - expect(core.refreshing).toBe(false); - expect(env.badger.updated).toEqual([ - { - kind: "initializing" - } - ]); - - // Refresh with a pending promise. - const githubLoaderPromise = new Promise(() => {}); - env.githubLoader.mockReturnValue(githubLoaderPromise); - - // Note: we don't use await, as it would block the thread. - core.refreshPullRequests(); - - expect(env.githubLoader).toHaveBeenCalled(); - expect(core.refreshing).toBe(true); - expect(env.badger.updated).toEqual([ - { - kind: "initializing" - }, - { - kind: "initializing" - } - ]); - }); - test("successful refresh after no stored state updates badge", async () => { const env = buildTestingEnvironment(); const core = new Core(env); @@ -279,7 +250,10 @@ describe("Core", () => { unreviewedPullRequestCount: 0 } ]); - expect(env.messenger.sent).toEqual([{ kind: "reload" }]); + expect(env.messenger.sent).toEqual([ + { kind: "reload" }, + { kind: "reload" } + ]); }); test("successful refresh after a previous state updates badge", async () => { @@ -329,7 +303,10 @@ describe("Core", () => { unreviewedPullRequestCount: 0 } ]); - expect(env.messenger.sent).toEqual([{ kind: "reload" }]); + expect(env.messenger.sent).toEqual([ + { kind: "reload" }, + { kind: "reload" } + ]); }); test("successful refresh after a previous error updates badge and clears error", async () => { @@ -372,7 +349,10 @@ describe("Core", () => { unreviewedPullRequestCount: 0 } ]); - expect(env.messenger.sent).toEqual([{ kind: "reload" }]); + expect(env.messenger.sent).toEqual([ + { kind: "reload" }, + { kind: "reload" } + ]); }); test("failed refresh updates badge and error", async () => { @@ -417,7 +397,10 @@ describe("Core", () => { kind: "error" } ]); - expect(env.messenger.sent).toEqual([{ kind: "reload" }]); + expect(env.messenger.sent).toEqual([ + { kind: "reload" }, + { kind: "reload" } + ]); }); it("notifies of new pull requests and saves notified state", async () => { diff --git a/src/state/core.ts b/src/state/core.ts index 6c002456..39777c7d 100644 --- a/src/state/core.ts +++ b/src/state/core.ts @@ -37,6 +37,7 @@ export class Core { this.token = await this.env.store.token.load(); this.overallStatus = "loading"; if (this.token !== null) { + this.refreshing = await this.env.store.currentlyRefreshing.load(); this.lastError = await this.env.store.lastError.load(); this.loadedState = await this.env.store.lastCheck.load(); this.notifiedPullRequestUrls = new Set( @@ -44,6 +45,7 @@ export class Core { ); this.muteConfiguration = await this.env.store.muteConfiguration.load(); } else { + this.refreshing = false; this.lastError = null; this.token = null; this.loadedState = null; @@ -57,6 +59,7 @@ export class Core { async setNewToken(token: string) { this.token = token; await this.env.store.token.save(token); + await this.saveRefreshing(false); await this.saveError(null); await this.saveNotifiedPullRequests([]); await this.saveLoadedState(null); @@ -74,12 +77,18 @@ export class Core { console.debug("Not online, skipping refresh."); return; } - this.refreshing = true; + if (this.refreshing) { + return; + } + await this.saveRefreshing(true); + await this.triggerReload(); this.updateBadge(); try { - await this.saveLoadedState( - await this.env.githubLoader(this.token, this.loadedState) - ); + const startRefreshTimestamp = Date.now(); + await this.saveLoadedState({ + startRefreshTimestamp, + ...(await this.env.githubLoader(this.token, this.loadedState)) + }); const unreviewedPullRequests = this.unreviewedPullRequests || []; await this.env.notifier.notify( unreviewedPullRequests, @@ -91,7 +100,7 @@ export class Core { this.saveError(e.message); throw e; } finally { - this.refreshing = false; + await this.saveRefreshing(false); this.updateBadge(); this.triggerReload(); } @@ -179,6 +188,11 @@ export class Core { await this.env.store.lastError.save(error); } + private async saveRefreshing(refreshing: boolean) { + this.refreshing = refreshing; + await this.env.store.currentlyRefreshing.save(refreshing); + } + private async saveLoadedState(lastCheck: LoadedState | null) { this.loadedState = lastCheck; await this.env.store.lastCheck.save(lastCheck); @@ -214,10 +228,17 @@ export class Core { this.env.badger.update(badgeState); } - private triggerBackgroundRefresh() { + triggerBackgroundRefresh() { this.env.messenger.send({ kind: "refresh" }); + + // Note: this is a hack in place because outside of a Chrome extension (ie + // when developing with webpack dev server), we don't have a background + // script that will refresh. + if (process.env.NODE_ENV === "development") { + this.refreshPullRequests().catch(console.error); + } } private triggerReload() { diff --git a/src/storage/api.ts b/src/storage/api.ts index 229bc621..bb45b8d5 100644 --- a/src/storage/api.ts +++ b/src/storage/api.ts @@ -12,6 +12,11 @@ export interface Store { */ lastCheck: ValueStorage; + /** + * Storage of whether a refresh is happening in the background. + */ + currentlyRefreshing: ValueStorage; + /** * Storage of the currently muted pull requests. */ diff --git a/src/storage/implementation.ts b/src/storage/implementation.ts index a355c321..27162ffe 100644 --- a/src/storage/implementation.ts +++ b/src/storage/implementation.ts @@ -11,6 +11,11 @@ export function buildStore(chromeApi: ChromeApi): Store { return { lastError: chromeValueStorage(chromeApi, "error"), lastCheck: chromeValueStorage(chromeApi, "lastCheck"), + currentlyRefreshing: chromeValueStorageWithDefault( + chromeApi, + "currentlyRefreshing", + false + ), muteConfiguration: chromeValueStorageWithDefault( chromeApi, "mute", diff --git a/src/storage/loaded-state.ts b/src/storage/loaded-state.ts index 3b267262..f1febef6 100644 --- a/src/storage/loaded-state.ts +++ b/src/storage/loaded-state.ts @@ -1,4 +1,12 @@ export interface LoadedState { + /** + * The timestamp at which we started loading the state. + * + * Note that since loading the state can take a few minutes, this can be quite + * different from the end time. + */ + startRefreshTimestamp?: number; + // TODO: Make it required once the field has been populated for long enough. userLogin?: string; diff --git a/yarn.lock b/yarn.lock index 38a99953..c7872ed7 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1062,6 +1062,13 @@ dependencies: "@types/jest-diff" "*" +"@types/moment@^2.13.0": + version "2.13.0" + resolved "https://registry.yarnpkg.com/@types/moment/-/moment-2.13.0.tgz#604ebd189bc3bc34a1548689404e61a2a4aac896" + integrity sha1-YE69GJvDvDShVIaJQE5hoqSqyJY= + dependencies: + moment "*" + "@types/node@^11.13.5": version "11.13.5" resolved "https://registry.yarnpkg.com/@types/node/-/node-11.13.5.tgz#266564afa8a6a09dc778dfacc703ed3f09c80516" @@ -4984,6 +4991,11 @@ mobx@^5.9.4: resolved "https://registry.yarnpkg.com/mobx/-/mobx-5.9.4.tgz#1dee92aba33f67b7baeeb679e3bd376a12e55812" integrity sha512-L9JjTX2rtQUAhCIgnHokfntNOsF14uioT9LqStf6Mya+16j56ZBe21E8Y9V59tfr2aH2kLQPD10qtCJXBuTAxw== +moment@*, moment@^2.24.0: + version "2.24.0" + resolved "https://registry.yarnpkg.com/moment/-/moment-2.24.0.tgz#0d055d53f5052aa653c9f6eb68bb5d12bf5c2b5b" + integrity sha512-bV7f+6l2QigeBBZSM/6yTNq4P2fNpSWj/0e7jQcy87A8e7o2nAfP/34/2ky5Vw4B9S446EtIhodAzkFCcR4dQg== + move-concurrently@^1.0.1: version "1.0.1" resolved "https://registry.yarnpkg.com/move-concurrently/-/move-concurrently-1.0.1.tgz#be2c005fda32e0b29af1f05d7c4b33214c701f92"