From 3b1429b8b028d61e4cf3805446ef36dab0061897 Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Tue, 22 Oct 2019 08:45:11 -0700 Subject: [PATCH] Add global error bubble Summary: Improved the display of error messages. Where previously only one error message could be displayed (quite obtrusively), with this change multiple errors can now be displayed and stack traces and further info can be hidden. Reviewed By: passy Differential Revision: D18036569 fbshipit-source-id: 2bc3dfa7a5196f931370a6e6dbf27c55b6cfb2bf --- src/App.tsx | 12 +- src/chrome/ErrorBar.tsx | 168 +++++++++++++++++--- src/dispatcher/server.tsx | 4 +- src/reducers/__tests__/connections.node.tsx | 75 ++++++++- src/reducers/connections.tsx | 89 +++++++++-- src/ui/components/ErrorBlock.tsx | 1 + 6 files changed, 303 insertions(+), 46 deletions(-) diff --git a/src/App.tsx b/src/App.tsx index dbfd7d910..f4c6fbf87 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -8,7 +8,7 @@ */ import React from 'react'; -import {FlexColumn, FlexRow, Client} from 'flipper'; +import {FlexColumn, FlexRow} from 'flipper'; import {connect} from 'react-redux'; import TitleBar from './chrome/TitleBar'; import MainSidebar from './chrome/MainSidebar'; @@ -36,7 +36,7 @@ import { import {Logger} from './fb-interfaces/Logger'; import BugReporter from './fb-stubs/BugReporter'; import {State as Store} from './reducers/index'; -import {StaticView} from './reducers/connections'; +import {StaticView, FlipperError} from './reducers/connections'; import PluginManager from './chrome/PluginManager'; import StatusBar from './chrome/StatusBar'; import SettingsSheet from './chrome/SettingsSheet'; @@ -49,7 +49,7 @@ type OwnProps = { type StateFromProps = { leftSidebarVisible: boolean; - error: string | null; + errors: FlipperError[]; activeSheet: ActiveSheet; share: ShareType | null; staticView: StaticView; @@ -126,6 +126,7 @@ export class App extends React.Component { return ( + {this.getSheet} {this.props.leftSidebarVisible && } @@ -136,7 +137,6 @@ export class App extends React.Component { )} - ); } @@ -145,12 +145,12 @@ export class App extends React.Component { export default connect( ({ application: {leftSidebarVisible, activeSheet, share}, - connections: {error, staticView}, + connections: {errors, staticView}, }) => ({ leftSidebarVisible, activeSheet, share: share, - error, + errors, staticView, }), )(App); diff --git a/src/chrome/ErrorBar.tsx b/src/chrome/ErrorBar.tsx index 405116a9e..d867ab9a2 100644 --- a/src/chrome/ErrorBar.tsx +++ b/src/chrome/ErrorBar.tsx @@ -8,28 +8,158 @@ */ import {styled, colors} from 'flipper'; -import React from 'react'; +import React, {useState, memo} from 'react'; +import {connect} from 'react-redux'; +import {FlipperError, dismissError} from '../reducers/connections'; +import {State as Store} from '../reducers/index'; +import {ErrorBlock, ButtonGroup, Button} from 'flipper'; +import {FlexColumn, FlexRow} from 'flipper'; -const ErrorBarContainer = styled('div')({ - backgroundColor: colors.cherry, - bottom: 0, - color: '#fff', - left: 0, - lineHeight: '26px', - position: 'absolute', - right: 0, - textAlign: 'center', - zIndex: 2, -}); - -type Props = { - text: string | null | undefined; +type StateFromProps = { + errors: FlipperError[]; }; -export default function ErrorBar(props: Props) { - if (props.text == null) { +type DispatchFromProps = { + dismissError: typeof dismissError; +}; + +type Props = DispatchFromProps & StateFromProps; + +const ErrorBar = memo(function ErrorBar(props: Props) { + const [collapsed, setCollapsed] = useState(false); + + if (!props.errors.length) { return null; - } else { - return {props.text}; } + + const errorCount = props.errors.reduce( + (sum, error) => sum + (error.occurrences || 1), + 0, + ); + + return ( + + + {props.errors.map((error, index) => ( + props.dismissError(index)} + key={index} + error={error} + /> + ))} + + setCollapsed(c => !c)} + title="Show / hide errors"> + {collapsed ? `▼ ${errorCount}` : '▲'} + + + ); +}); + +export default connect( + ({connections: {errors}}) => ({ + errors, + }), + { + dismissError, + }, +)(ErrorBar); + +function ErrorTile({ + onDismiss, + error, +}: { + onDismiss: () => void; + error: FlipperError; +}) { + const [collapsed, setCollapsed] = useState(true); + return ( + + + {error.message} + {error.occurrences! > 1 && ( + + {error.occurrences} + + )} + + + {(error.details || error.error) && ( + + )} + + + + + {!collapsed && ( + + + {error.details} + {error.error && } + + + )} + + ); } + +const ErrorBarContainer = styled('div')({ + boxShadow: '2px 2px 2px #ccc', + userSelect: 'text', +}); + +const DismissAllErrors = styled('div')({ + boxShadow: '2px 2px 2px #ccc', + backgroundColor: colors.cherryDark3, + color: '#fff', + textAlign: 'center', + borderBottomLeftRadius: '4px', + borderBottomRightRadius: '4px', + position: 'absolute', + width: '48px', + height: '16px', + zIndex: 2, + right: '20px', + fontSize: '6pt', + lineHeight: '16px', + cursor: 'pointer', + alignItems: 'center', +}); + +const ErrorDetails = styled('div')({ + width: '100%', + marginTop: 4, +}); + +const ErrorRows = styled('div')({ + backgroundColor: colors.cherry, + color: '#fff', + maxHeight: '600px', + overflowY: 'auto', + overflowX: 'hidden', + transition: 'max-height 0.3s ease', + '&.collapsed': { + maxHeight: '0px', + }, +}); + +const ErrorRow = styled('div')({ + padding: '4px 12px', + borderBottom: '1px solid ' + colors.cherryDark3, + verticalAlign: 'middle', + lineHeight: '28px', +}); + +const ErrorCounter = styled(FlexColumn)({ + backgroundColor: colors.cherryDark3, + color: colors.cherry, + width: 24, + height: 24, + borderRadius: 24, + marginTop: 2, + lineHeight: '24px', + textAlign: 'center', +}); diff --git a/src/dispatcher/server.tsx b/src/dispatcher/server.tsx index 999e35f00..140476d18 100644 --- a/src/dispatcher/server.tsx +++ b/src/dispatcher/server.tsx @@ -49,14 +49,14 @@ export default (store: Store, logger: Logger) => { }); server.addListener('error', err => { - const payload: string = + const message: string = err.code === 'EADDRINUSE' ? "Couldn't start websocket server. Looks like you have multiple copies of Flipper running." : err.message || 'Unknown error'; store.dispatch({ type: 'SERVER_ERROR', - payload, + payload: {message}, }); }); diff --git a/src/reducers/__tests__/connections.node.tsx b/src/reducers/__tests__/connections.node.tsx index 7cc254b1a..6a6649891 100644 --- a/src/reducers/__tests__/connections.node.tsx +++ b/src/reducers/__tests__/connections.node.tsx @@ -14,16 +14,85 @@ import BaseDevice from '../../devices/BaseDevice'; test('REGISTER_DEVICE doesnt remove error', () => { const initialState: State = reducer(undefined, { type: 'SERVER_ERROR', - payload: 'something went wrong', + payload: {message: 'something went wrong'}, }); // Precondition - expect(initialState.error).toEqual('something went wrong'); + expect(initialState.errors).toEqual([ + {message: 'something went wrong', occurrences: 1}, + ]); const endState = reducer(initialState, { type: 'REGISTER_DEVICE', payload: new BaseDevice('serial', 'physical', 'title', 'Android'), }); - expect(endState.error).toEqual('something went wrong'); + expect(endState.errors).toEqual([ + {message: 'something went wrong', occurrences: 1}, + ]); +}); + +test('errors are collected on a by name basis', () => { + const initialState: State = reducer(undefined, { + type: 'SERVER_ERROR', + payload: { + message: 'error1', + error: 'stack1', + }, + }); + + expect(initialState.errors).toMatchInlineSnapshot(` + Array [ + Object { + "error": "stack1", + "message": "error1", + "occurrences": 1, + }, + ] + `); + + const state2: State = reducer(initialState, { + type: 'SERVER_ERROR', + payload: { + message: 'error2', + error: 'stack2', + }, + }); + // There are now two errors + expect(state2.errors).toMatchInlineSnapshot(` + Array [ + Object { + "error": "stack1", + "message": "error1", + "occurrences": 1, + }, + Object { + "error": "stack2", + "message": "error2", + "occurrences": 1, + }, + ] + `); + const state3: State = reducer(state2, { + type: 'SERVER_ERROR', + payload: { + message: 'error1', + error: 'stack3', + }, + }); + // Still two errors, but error1 has been updated and occurrences increased + expect(state3.errors).toMatchInlineSnapshot(` + Array [ + Object { + "error": "stack3", + "message": "error1", + "occurrences": 2, + }, + Object { + "error": "stack2", + "message": "error2", + "occurrences": 1, + }, + ] + `); }); diff --git a/src/reducers/connections.tsx b/src/reducers/connections.tsx index 0236ddd63..a74193137 100644 --- a/src/reducers/connections.tsx +++ b/src/reducers/connections.tsx @@ -27,6 +27,13 @@ export type StaticView = | typeof WelcomeScreen | typeof SupportRequestForm; +export type FlipperError = { + occurrences?: number; + message: string; + details?: string; + error?: Error | string; +}; + export type State = { devices: Array; androidEmulators: Array; @@ -37,7 +44,7 @@ export type State = { userPreferredPlugin: null | string; userPreferredApp: null | string; userLRUPlugins: {[key: string]: Array}; - error: null | string; + errors: FlipperError[]; clients: Array; uninitializedClients: Array<{ client: UninitializedClient; @@ -79,7 +86,7 @@ export type Action = } | { type: 'SERVER_ERROR'; - payload: null | string; + payload: null | FlipperError; } | { type: 'NEW_CLIENT'; @@ -107,7 +114,7 @@ export type Action = } | { type: 'CLIENT_SETUP_ERROR'; - payload: {client: UninitializedClient; error: Error}; + payload: {client: UninitializedClient; error: FlipperError}; } | { type: 'CLIENT_SHOW_MORE_OR_LESS'; @@ -117,6 +124,10 @@ export type Action = | { type: 'SET_STATIC_VIEW'; payload: StaticView; + } + | { + type: 'DISMISS_ERROR'; + payload: number; }; const DEFAULT_PLUGIN = 'DeviceLogs'; @@ -131,7 +142,7 @@ const INITAL_STATE: State = { userPreferredPlugin: null, userPreferredApp: null, userLRUPlugins: {}, - error: null, + errors: [], clients: [], uninitializedClients: [], deepLinkPayload: null, @@ -356,7 +367,10 @@ const reducer = (state: State = INITAL_STATE, action: Actions): State => { } case 'SERVER_ERROR': { const {payload} = action; - return {...state, error: payload}; + if (!payload) { + return state; + } + return {...state, errors: mergeError(state.errors, payload)}; } case 'START_CLIENT_SETUP': { const {payload} = action; @@ -385,10 +399,11 @@ const reducer = (state: State = INITAL_STATE, action: Actions): State => { const {payload} = action; const errorMessage = - payload.error instanceof Error ? payload.error.message : payload.error; - console.error( - `Client setup error: ${errorMessage} while setting up client: ${payload.client.os}:${payload.client.deviceName}:${payload.client.appName}`, - ); + payload.error instanceof Error + ? payload.error.message + : '' + payload.error; + const details = `Client setup error: ${errorMessage} while setting up client: ${payload.client.os}:${payload.client.deviceName}:${payload.client.appName}`; + console.error(details); return { ...state, uninitializedClients: state.uninitializedClients @@ -398,7 +413,11 @@ const reducer = (state: State = INITAL_STATE, action: Actions): State => { : c, ) .sort((a, b) => a.client.appName.localeCompare(b.client.appName)), - error: `Client setup error: ${errorMessage}`, + errors: mergeError(state.errors, { + message: `Client setup error: ${errorMessage}`, + details, + error: payload.error instanceof Error ? payload.error : undefined, + }), }; } case 'CLIENT_SHOW_MORE_OR_LESS': { @@ -428,6 +447,14 @@ const reducer = (state: State = INITAL_STATE, action: Actions): State => { userLRUPlugins: clearLRUPlugins, }; } + case 'DISMISS_ERROR': { + const errors = state.errors.slice(); + errors.splice(action.payload, 1); + return { + ...state, + errors, + }; + } default: return state; } @@ -438,23 +465,48 @@ export default (state: State = INITAL_STATE, action: Actions): State => { if (nextState.selectedDevice) { const {selectedDevice} = nextState; - const deviceNotSupportedError = 'iOS Devices are not yet supported'; + const deviceNotSupportedErrorMessage = 'iOS Devices are not yet supported'; const error = selectedDevice.os === 'iOS' && selectedDevice.deviceType === 'physical' && !iosUtil.isAvailable() - ? deviceNotSupportedError + ? deviceNotSupportedErrorMessage : null; - if (nextState.error === deviceNotSupportedError) { - nextState.error = error; - } else { - nextState.error = error || nextState.error; + if (error) { + const deviceNotSupportedError = nextState.errors.find( + error => error.message === deviceNotSupportedErrorMessage, + ); + if (deviceNotSupportedError) { + deviceNotSupportedError.message = error; + } else { + nextState.errors.push({message: error}); + } } } return nextState; }; +function mergeError( + errors: FlipperError[], + newError: FlipperError, +): FlipperError[] { + const idx = errors.findIndex(error => error.message === newError.message); + const results = errors.slice(); + if (idx !== -1) { + results[idx] = { + ...newError, + occurrences: (errors[idx].occurrences || 0) + 1, + }; + } else { + results.push({ + ...newError, + occurrences: 1, + }); + } + return results; +} + export const selectDevice = (payload: BaseDevice): Action => ({ type: 'SELECT_DEVICE', payload, @@ -495,3 +547,8 @@ function extractAppNameFromAppId(appId: string | null): string | null { // Expect the name of the app to be on the first matching return matchedRegex && matchedRegex[1]; } + +export const dismissError = (index: number): Action => ({ + type: 'DISMISS_ERROR', + payload: index, +}); diff --git a/src/ui/components/ErrorBlock.tsx b/src/ui/components/ErrorBlock.tsx index 742203d4b..2d074746b 100644 --- a/src/ui/components/ErrorBlock.tsx +++ b/src/ui/components/ErrorBlock.tsx @@ -18,6 +18,7 @@ export const ErrorBlockContainer = styled(CodeBlock)({ color: '#a94442', overflow: 'auto', padding: 10, + whiteSpace: 'pre', }); /**