From bf5b5d0d7622771b68aa7941f99c03d94105b605 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ey=C3=BE=C3=B3r=20Magn=C3=BAsson?= Date: Tue, 24 Sep 2019 09:35:20 +0200 Subject: [PATCH] fix(dashboard): ensure fresh store state when merging data We were merging API responses to a stale version of the store object. That got merged to the global store state, causing some changes to be lost or overwritten. --- dashboard/src/containers/entity-result.tsx | 6 +- dashboard/src/containers/graph.tsx | 18 ++--- dashboard/src/containers/logs.tsx | 12 ++-- dashboard/src/containers/overview.tsx | 18 ++--- dashboard/src/containers/sidebar.tsx | 4 +- dashboard/src/contexts/api-handlers.ts | 45 +++++++------ dashboard/src/contexts/api.tsx | 24 ++++--- dashboard/src/contexts/ws-handlers.ts | 78 +++++++++++----------- 8 files changed, 105 insertions(+), 100 deletions(-) diff --git a/dashboard/src/containers/entity-result.tsx b/dashboard/src/containers/entity-result.tsx index 070c4f864f..19c9ce3b27 100644 --- a/dashboard/src/containers/entity-result.tsx +++ b/dashboard/src/containers/entity-result.tsx @@ -47,15 +47,15 @@ interface Props { */ export default ({ name, moduleName, type, onClose }: Props) => { const { - actions: { loadTestResult, loadTaskResult }, + actions, store: { entities: { tasks, tests }, requestStates: { fetchTestResult, fetchTaskResult } }, } = useApi() const loadResults = () => { if (type === "test") { - loadTestResult({ name, moduleName, force: true }) + actions.loadTestResult({ name, moduleName, force: true }) } else if (type === "run" || type === "task") { - loadTaskResult({ name, force: true }) + actions.loadTaskResult({ name, force: true }) } } diff --git a/dashboard/src/containers/graph.tsx b/dashboard/src/containers/graph.tsx index 177425b052..90cf29fb5a 100644 --- a/dashboard/src/containers/graph.tsx +++ b/dashboard/src/containers/graph.tsx @@ -36,34 +36,34 @@ export interface GraphOutputWithNodeStatus extends GraphOutput { export default () => { const { - actions: { loadGraph, loadConfig }, + actions, store: { entities: { modules, services, tests, tasks, graph }, requestStates: { fetchGraph, fetchTaskStates } }, } = useApi() + const { + actions: { selectGraphNode, stackGraphToggleItemsView, clearGraphNodeSelection }, + state: { selectedGraphNode, isSidebarOpen, stackGraph: { filters } }, + } = useUiState() + useEffect(() => { async function fetchData() { - return await loadConfig() + return await actions.loadConfig() } fetchData() }, []) useEffect(() => { async function fetchData() { - return await loadGraph() + return await actions.loadGraph() } fetchData() }, []) - const { - actions: { selectGraphNode, stackGraphToggleItemsView, clearGraphNodeSelection }, - state: { selectedGraphNode, isSidebarOpen, stackGraph: { filters } }, - } = useUiState() - if (fetchGraph.error) { return } - if (fetchGraph.loading) { + if (!fetchGraph.initLoadComplete) { return } diff --git a/dashboard/src/containers/logs.tsx b/dashboard/src/containers/logs.tsx index d73d638fba..cb4c5afe66 100644 --- a/dashboard/src/containers/logs.tsx +++ b/dashboard/src/containers/logs.tsx @@ -15,7 +15,7 @@ import Spinner from "../components/spinner" export default () => { const { - actions: { loadConfig, loadLogs }, + actions, store: { entities: { logs, services }, requestStates: { fetchLogs, fetchConfig } }, } = useApi() @@ -23,22 +23,22 @@ export default () => { useEffect(() => { async function fetchData() { - return await loadConfig() + return await actions.loadConfig() } fetchData() }, []) useEffect(() => { async function fetchData() { - return await loadLogs({ serviceNames }) + return await actions.loadLogs({ serviceNames }) } if (serviceNames.length) { fetchData() } - }, [fetchConfig.didFetch]) // run again only after config had been fetched + }, [fetchConfig.initLoadComplete]) // run again only after config had been fetched - if (fetchConfig.loading || fetchLogs.loading) { + if (!(fetchConfig.initLoadComplete && fetchLogs.initLoadComplete)) { return } @@ -49,6 +49,6 @@ export default () => { } return ( - + ) } diff --git a/dashboard/src/containers/overview.tsx b/dashboard/src/containers/overview.tsx index f64091e8d9..fbf2da2e17 100644 --- a/dashboard/src/containers/overview.tsx +++ b/dashboard/src/containers/overview.tsx @@ -77,12 +77,12 @@ const mapTasks = (taskEntities: Task[], moduleName: string): ModuleProps["taskCa export default () => { const { + actions, store: { projectRoot, entities: { modules, services, tests, tasks }, requestStates: { fetchConfig, fetchStatus }, }, - actions: { loadConfig, loadStatus }, } = useApi() const { @@ -94,18 +94,9 @@ export default () => { }, } = useUiState() - // TODO use useAsyncEffect? - // https://dev.to/n1ru4l/homebrew-react-hooks-useasynceffect-or-how-to-handle-async-operations-with-useeffect-1fa8 useEffect(() => { async function fetchData() { - return await loadConfig() - } - fetchData() - }, []) - - useEffect(() => { - async function fetchData() { - return await loadStatus() + return await actions.loadConfig() } fetchData() }, []) @@ -118,7 +109,10 @@ export default () => { return } - if (fetchConfig.loading || fetchStatus.loading) { + // Note that we don't call the loadStatus function here since the Sidebar ensures that the status is always loaded. + // FIXME: We should be able to call loadStatus safely and have the handler check if the status + // has already been fetched or is pending. + if (!(fetchConfig.initLoadComplete && fetchStatus.initLoadComplete)) { return } diff --git a/dashboard/src/containers/sidebar.tsx b/dashboard/src/containers/sidebar.tsx index 4308ea737a..66f6ed15bf 100644 --- a/dashboard/src/containers/sidebar.tsx +++ b/dashboard/src/containers/sidebar.tsx @@ -43,13 +43,13 @@ const builtinPages: Page[] = [ const SidebarContainer = () => { const { - actions: { loadStatus }, + actions, store: { entities: { providers } }, } = useApi() useEffect(() => { async function fetchData() { - return await loadStatus() + return await actions.loadStatus() } fetchData() }, []) diff --git a/dashboard/src/contexts/api-handlers.ts b/dashboard/src/contexts/api-handlers.ts index 23b86c0bb0..8e01ea2386 100644 --- a/dashboard/src/contexts/api-handlers.ts +++ b/dashboard/src/contexts/api-handlers.ts @@ -53,7 +53,7 @@ interface LoadHandlerParams { export async function loadConfigHandler({ store, dispatch, force = false }: LoadHandlerParams) { const requestKey = "fetchConfig" - if (!force && store.requestStates[requestKey].didFetch) { + if (!force && store.requestStates[requestKey].initLoadComplete) { return } @@ -66,8 +66,9 @@ export async function loadConfigHandler({ store, dispatch, force = false }: Load return } - const processedStore = processConfig(store, res) - dispatch({ store: processedStore, type: "fetchSuccess", requestKey }) + const produceNextStore = (currentStore: Store) => processConfig(currentStore, res) + + dispatch({ type: "fetchSuccess", requestKey, produceNextStore }) } function processConfig(store: Store, config: ConfigDump) { @@ -111,15 +112,13 @@ function processConfig(store: Store, config: ConfigDump) { } } - const processedStore = produce(store, storeDraft => { + return produce(store, storeDraft => { storeDraft.entities.modules = modules storeDraft.entities.services = services storeDraft.entities.tests = tests storeDraft.entities.tasks = tasks storeDraft.projectRoot = config.projectRoot }) - - return processedStore } interface LoadLogsHandlerParams extends LoadHandlerParams, FetchLogsParams { } @@ -127,7 +126,7 @@ interface LoadLogsHandlerParams extends LoadHandlerParams, FetchLogsParams { } export async function loadLogsHandler({ serviceNames, store, dispatch, force = false }: LoadLogsHandlerParams) { const requestKey = "fetchLogs" - if ((!force && store.requestStates[requestKey].didFetch) || !serviceNames.length) { + if ((!force && store.requestStates[requestKey].initLoadComplete) || !serviceNames.length) { return } dispatch({ requestKey, type: "fetchStart" }) @@ -140,7 +139,9 @@ export async function loadLogsHandler({ serviceNames, store, dispatch, force = f return } - dispatch({ store: processLogs(store, res), type: "fetchSuccess", requestKey }) + const produceNextStore = (currentStore: Store) => processLogs(currentStore, res) + + dispatch({ type: "fetchSuccess", requestKey, produceNextStore }) } function processLogs(store: Store, logs: ServiceLogEntry[]) { @@ -152,7 +153,7 @@ function processLogs(store: Store, logs: ServiceLogEntry[]) { export async function loadStatusHandler({ store, dispatch, force = false }: LoadHandlerParams) { const requestKey = "fetchStatus" - if (!force && store.requestStates[requestKey].didFetch) { + if (!force && store.requestStates[requestKey].initLoadComplete) { return } @@ -166,11 +167,13 @@ export async function loadStatusHandler({ store, dispatch, force = false }: Load return } - dispatch({ store: processStatus(store, res), type: "fetchSuccess", requestKey }) + const produceNextStore = (currentStore: Store) => processStatus(currentStore, res) + + dispatch({ type: "fetchSuccess", requestKey, produceNextStore }) } function processStatus(store: Store, status: StatusCommandResult) { - const processedStore = produce(store, storeDraft => { + return produce(store, storeDraft => { for (const serviceName of Object.keys(status.services)) { storeDraft.entities.services[serviceName] = { ...storeDraft.entities.services[serviceName], @@ -191,8 +194,6 @@ function processStatus(store: Store, status: StatusCommandResult) { } storeDraft.entities.providers = status.providers }) - - return processedStore } interface LoadTaskResultHandlerParams extends LoadHandlerParams, FetchTaskResultParams { } @@ -202,7 +203,7 @@ export async function loadTaskResultHandler( ) { const requestKey = "fetchTaskResult" - if (!force && store.requestStates[requestKey].didFetch) { + if (!force && store.requestStates[requestKey].initLoadComplete) { return } @@ -216,7 +217,9 @@ export async function loadTaskResultHandler( return } - dispatch({ store: processTaskResult(store, res), type: "fetchSuccess", requestKey }) + const produceNextStore = (currentStore: Store) => processTaskResult(currentStore, res) + + dispatch({ type: "fetchSuccess", requestKey, produceNextStore }) } function processTaskResult(store: Store, result: TaskResultOutput) { @@ -232,7 +235,7 @@ interface LoadTestResultParams extends LoadHandlerParams, FetchTestResultParams export async function loadTestResultHandler({ store, dispatch, force = false, ...fetchParams }: LoadTestResultParams) { const requestKey = "fetchTestResult" - if (!force && store.requestStates[requestKey].didFetch) { + if (!force && store.requestStates[requestKey].initLoadComplete) { return } @@ -246,7 +249,9 @@ export async function loadTestResultHandler({ store, dispatch, force = false, .. return } - dispatch({ store: processTestResult(store, res), type: "fetchSuccess", requestKey }) + const produceNextStore = (currentStore: Store) => processTestResult(currentStore, res) + + dispatch({ type: "fetchSuccess", requestKey, produceNextStore }) } function processTestResult(store: Store, result: TestResultOutput) { @@ -260,7 +265,7 @@ function processTestResult(store: Store, result: TestResultOutput) { export async function loadGraphHandler({ store, dispatch, force = false }: LoadHandlerParams) { const requestKey = "fetchGraph" - if (!force && store.requestStates[requestKey].didFetch) { + if (!force && store.requestStates[requestKey].initLoadComplete) { return } @@ -274,7 +279,9 @@ export async function loadGraphHandler({ store, dispatch, force = false }: LoadH return } - dispatch({ store: processGraph(store, res), type: "fetchSuccess", requestKey }) + const produceNextStore = (currentStore: Store) => processGraph(currentStore, res) + + dispatch({ type: "fetchSuccess", requestKey, produceNextStore }) } function processGraph(store: Store, graph: GraphOutput) { diff --git a/dashboard/src/contexts/api.tsx b/dashboard/src/contexts/api.tsx index 7ab8b7c4e2..c479a77117 100644 --- a/dashboard/src/contexts/api.tsx +++ b/dashboard/src/contexts/api.tsx @@ -9,7 +9,6 @@ import { useReducer, useEffect, useContext } from "react" import React from "react" import produce from "immer" -import { merge } from "lodash" import { AxiosError } from "axios" import { ServiceLogEntry } from "garden-service/build/src/types/plugin/service/getServiceLogs" @@ -101,7 +100,7 @@ export interface Service { interface RequestState { loading: boolean, - didFetch: boolean + initLoadComplete: boolean error?: AxiosError, } @@ -141,6 +140,8 @@ const requestKeys: RequestKey[] = [ "fetchTaskStates", ] +type ProduceNextStore = (store: Store) => Store + interface ActionBase { type: "fetchStart" | "fetchSuccess" | "fetchFailure" | "wsMessageReceived" } @@ -153,7 +154,7 @@ interface ActionStart extends ActionBase { interface ActionSuccess extends ActionBase { requestKey: RequestKey type: "fetchSuccess" - store: Store + produceNextStore: ProduceNextStore } interface ActionError extends ActionBase { @@ -164,7 +165,7 @@ interface ActionError extends ActionBase { interface WsMessageReceived extends ActionBase { type: "wsMessageReceived" - store: Store + produceNextStore: ProduceNextStore } export type Action = ActionStart | ActionError | ActionSuccess | WsMessageReceived @@ -193,7 +194,7 @@ interface Actions { } const initialRequestState = requestKeys.reduce((acc, key) => { - acc[key] = { loading: false, didFetch: false } + acc[key] = { loading: false, initLoadComplete: false } return acc }, {} as { [K in RequestKey]: RequestState }) @@ -224,9 +225,10 @@ function reducer(store: Store, action: Action): Store { }) break case "fetchSuccess": - nextStore = produce(merge(store, action.store), storeDraft => { + // Produce the next store state from the fetch result and update the request state + nextStore = produce(action.produceNextStore(store), storeDraft => { storeDraft.requestStates[action.requestKey].loading = false - storeDraft.requestStates[action.requestKey].didFetch = true + storeDraft.requestStates[action.requestKey].initLoadComplete = true }) break case "fetchFailure": @@ -234,11 +236,11 @@ function reducer(store: Store, action: Action): Store { storeDraft.requestStates[action.requestKey].loading = false storeDraft.requestStates[action.requestKey].error = action.error // set didFetch to true on failure so the user can choose to force load the status - storeDraft.requestStates[action.requestKey].didFetch = true + storeDraft.requestStates[action.requestKey].initLoadComplete = true }) break case "wsMessageReceived": - nextStore = { ...merge(store, action.store) } + nextStore = action.produceNextStore(store) break } @@ -253,7 +255,7 @@ function useApiActions(store: Store, dispatch: React.Dispatch) { loadConfig: async (params: LoadActionParams = {}) => loadConfigHandler({ store, dispatch, ...params }), loadStatus: async (params: LoadActionParams = {}) => loadStatusHandler({ store, dispatch, ...params }), loadLogs: async (params: LoadLogsParams) => loadLogsHandler({ store, dispatch, ...params }), - loadTaskResult: async (params: LoadTaskResultParams) => loadTaskResultHandler({ name, store, dispatch, ...params }), + loadTaskResult: async (params: LoadTaskResultParams) => loadTaskResultHandler({ store, dispatch, ...params }), loadTestResult: async (params: LoadTestResultParams) => loadTestResultHandler({ store, dispatch, ...params }), loadGraph: async (params: LoadActionParams = {}) => loadGraphHandler({ store, dispatch, ...params }), } @@ -287,7 +289,7 @@ export const ApiProvider: React.FC = ({ children }) => { // TODO: Add websocket state as dependency (second argument) so that the websocket is re-initialised // if the connection breaks. useEffect(() => { - return initWebSocket(store, dispatch) + return initWebSocket(dispatch) }, []) return ( diff --git a/dashboard/src/contexts/ws-handlers.ts b/dashboard/src/contexts/ws-handlers.ts index 3473188a2f..558a56f788 100644 --- a/dashboard/src/contexts/ws-handlers.ts +++ b/dashboard/src/contexts/ws-handlers.ts @@ -16,6 +16,7 @@ import { supportedEventNames, } from "./api" import getApiUrl from "../api/get-api-url" +import produce from "immer" export type WsEventMessage = ServerWebsocketMessage & { type: "event", @@ -30,7 +31,7 @@ export function isSupportedEvent(data: ServerWebsocketMessage): data is WsEventM return data.type === "event" && supportedEventNames.has((data as WsEventMessage).name) } -export function initWebSocket(store: Store, dispatch: React.Dispatch) { +export function initWebSocket(dispatch: React.Dispatch) { const url = getApiUrl() const ws = new WebSocket(`ws://${url}/ws`) ws.onopen = event => { @@ -46,7 +47,8 @@ export function initWebSocket(store: Store, dispatch: React.Dispatch) { console.error(parsedMsg) } if (isSupportedEvent(parsedMsg)) { - dispatch({ store: processWebSocketMessage(store, parsedMsg), type: "wsMessageReceived" }) + const produceNextStore = (store: Store) => processWebSocketMessage(store, parsedMsg) + dispatch({ type: "wsMessageReceived", produceNextStore }) } } return function cleanUp() { @@ -56,45 +58,45 @@ export function initWebSocket(store: Store, dispatch: React.Dispatch) { // Process the graph response and return a normalized store function processWebSocketMessage(store: Store, message: WsEventMessage) { - const storeDraft = { ...store } const taskType = message.payload["type"] === "task" ? "run" : message.payload["type"] // convert "task" to "run" const taskState = message.name const entityName = message.payload["name"] - // We don't handle taskGraphComplete events - if (taskType && taskState !== "taskGraphComplete") { - storeDraft.requestStates.fetchTaskStates.loading = true - switch (taskType) { - case "publish": - break - case "deploy": - storeDraft.entities.services[entityName] = { - ...storeDraft.entities.services[entityName], - taskState, - } - break - case "build": - storeDraft.entities.modules[entityName] = { - ...store.entities.modules[entityName], - taskState, - } - break - case "run": - storeDraft.entities.tasks[entityName] = { - ...store.entities.tasks[entityName], - taskState, - } - break - case "test": - storeDraft.entities.tests[entityName] = { - ...store.entities.tests[entityName], taskState, - } - break + return produce(store, storeDraft => { + // We don't handle taskGraphComplete events + if (taskType && taskState !== "taskGraphComplete") { + storeDraft.requestStates.fetchTaskStates.loading = true + switch (taskType) { + case "publish": + break + case "deploy": + storeDraft.entities.services[entityName] = { + ...storeDraft.entities.services[entityName], + taskState, + } + break + case "build": + storeDraft.entities.modules[entityName] = { + ...store.entities.modules[entityName], + taskState, + } + break + case "run": + storeDraft.entities.tasks[entityName] = { + ...store.entities.tasks[entityName], + taskState, + } + break + case "test": + storeDraft.entities.tests[entityName] = { + ...store.entities.tests[entityName], + taskState, + } + break + } } - } - - if (taskState === "taskGraphComplete") { // add to requestState graph whenever its taskGraphComplete - storeDraft.requestStates.fetchTaskStates.loading = false - } - return storeDraft + if (taskState === "taskGraphComplete") { // add to requestState graph whenever its taskGraphComplete + storeDraft.requestStates.fetchTaskStates.loading = false + } + }) }