From fa6deff05033a7e6ca0585ce7763d5a30fd75969 Mon Sep 17 00:00:00 2001 From: Igor Dykhta Date: Wed, 6 Jan 2021 23:13:30 +0200 Subject: [PATCH] [Enhancement] Show an error notification for errors in deck (#1373) --- docs/api-reference/actions/actions.md | 3 +- docs/api-reference/reducers/ui-state.md | 3 +- src/actions/ui-state-actions.d.ts | 9 ++-- src/actions/ui-state-actions.js | 5 +- src/components/map-container.js | 30 +++++++++++- src/constants/action-types.d.ts | 2 +- src/constants/default-settings.js | 3 ++ src/reducers/ui-state-updaters.d.ts | 15 +++--- src/reducers/ui-state-updaters.js | 24 +++++++--- src/utils/notifications-utils.js | 2 +- test/node/reducers/ui-state-test.js | 62 ++++++++++++++++++------- 11 files changed, 115 insertions(+), 43 deletions(-) diff --git a/docs/api-reference/actions/actions.md b/docs/api-reference/actions/actions.md index 5e3326cd15..72ae57cce0 100644 --- a/docs/api-reference/actions/actions.md +++ b/docs/api-reference/actions/actions.md @@ -961,7 +961,8 @@ It also manges which settings are selected during image and map export ### addNotification -Add a notification to be displayed +Add a notification to be displayed. +Existing notification is going to be updated in case of matching ids. - **ActionTypes**: [`ActionTypes.ADD_NOTIFICATION`][12] - **Updaters**: [`uiStateUpdaters.addNotificationUpdater`][220] diff --git a/docs/api-reference/reducers/ui-state.md b/docs/api-reference/reducers/ui-state.md index 73f31446bb..adb525131e 100644 --- a/docs/api-reference/reducers/ui-state.md +++ b/docs/api-reference/reducers/ui-state.md @@ -69,7 +69,8 @@ export default composedReducer; ### addNotificationUpdater -Add a notification to be displayed +Add a notification to be displayed. +Existing notification is going to be updated in case of matching ids. - **Action**: [`addNotification`][54] diff --git a/src/actions/ui-state-actions.d.ts b/src/actions/ui-state-actions.d.ts index 2c1e12b545..dec2af8e73 100644 --- a/src/actions/ui-state-actions.d.ts +++ b/src/actions/ui-state-actions.d.ts @@ -94,19 +94,18 @@ export function removeNotification( export type SetExportImageSettingUpdaterAction = { payload: Partial; }; -export function setExportImageSetting(newSetting: - SetExportImageSettingUpdaterAction['payload'] +export function setExportImageSetting( + newSetting: SetExportImageSettingUpdaterAction['payload'] ): Merge; /** START_EXPORTING_IMAGE */ export function startExportingImage(options?: { ratio?: string; resolution?: string; - legend?: string, - center?: boolean + legend?: string; + center?: boolean; }): Merge; - /** SET_EXPORT_IMAGE_DATA_URI */ export type SetExportImageDataUriUpdaterAction = { payload: string; diff --git a/src/actions/ui-state-actions.js b/src/actions/ui-state-actions.js index a2807a24b4..8cb617a0a8 100644 --- a/src/actions/ui-state-actions.js +++ b/src/actions/ui-state-actions.js @@ -84,9 +84,10 @@ export const toggleMapControl = createAction(ActionTypes.TOGGLE_MAP_CONTROL, (pa export const openDeleteModal = createAction(ActionTypes.OPEN_DELETE_MODAL, datasetId => datasetId); /** - * Add a notification to be displayed + * Add a notification to be displayed. + * Existing notification will be updated in case of matching id. * @memberof uiStateActions - * @param notification - The `notification` object to be added + * @param notification - The `notification` object to be added or updated * @type {typeof import('./ui-state-actions').addNotification} * @public */ diff --git a/src/components/map-container.js b/src/components/map-container.js index 5b5382af0d..8515892ff1 100644 --- a/src/components/map-container.js +++ b/src/components/map-container.js @@ -25,6 +25,7 @@ import MapboxGLMap from 'react-map-gl'; import DeckGL from '@deck.gl/react'; import {createSelector} from 'reselect'; import WebMercatorViewport from 'viewport-mercator-project'; +import {errorNotification} from 'utils/notifications-utils'; // components import MapPopoverFactory from 'components/map/map-popover'; @@ -42,7 +43,11 @@ import {getLayerHoverProp, renderDeckGlLayer} from 'utils/layer-utils'; // default-settings import ThreeDBuildingLayer from 'deckgl-layers/3d-building-layer/3d-building-layer'; -import {FILTER_TYPES, GEOCODER_LAYER_ID} from 'constants/default-settings'; +import { + FILTER_TYPES, + GEOCODER_LAYER_ID, + THROTTLE_NOTIFICATION_TIME +} from 'constants/default-settings'; /** @type {{[key: string]: React.CSSProperties}} */ const MAP_STYLE = { @@ -251,6 +256,28 @@ export default function MapContainerFactory(MapPopover, MapControl, Editor) { setLayerBlending(gl, this.props.layerBlending); }; + _onDeckError = (error, layer) => { + const errorMessage = `An error in deck.gl: ${error.message} in ${layer.id}`; + const notificationId = `${layer.id}-${error.message}`; + + // Throttle error notifications, as React doesn't like too many state changes from here. + this._deckGLErrorsElapsed = this._deckGLErrorsElapsed || {}; + const lastShown = this._deckGLErrorsElapsed[notificationId]; + if (!lastShown || lastShown < Date.now() - THROTTLE_NOTIFICATION_TIME) { + this._deckGLErrorsElapsed[notificationId] = Date.now(); + + // Create new error notification or update existing one with same id. + // Update is required to preserve the order of notifications as they probably are going to "jump" based on order of errors. + const {uiStateActions} = this.props; + uiStateActions.addNotification( + errorNotification({ + message: errorMessage, + id: notificationId + }) + ); + } + }; + /* component render functions */ /* eslint-disable complexity */ @@ -399,6 +426,7 @@ export default function MapContainerFactory(MapPopover, MapControl, Editor) { onBeforeRender={this._onBeforeRender} onHover={visStateActions.onLayerHover} onClick={visStateActions.onLayerClick} + onError={this._onDeckError} ref={comp => { if (comp && comp.deck && !this._deck) { this._deck = comp.deck; diff --git a/src/constants/action-types.d.ts b/src/constants/action-types.d.ts index e8b8232e1c..24a9915894 100644 --- a/src/constants/action-types.d.ts +++ b/src/constants/action-types.d.ts @@ -23,7 +23,7 @@ export type ActionType = { REMOVE_DATASET: string; REORDER_LAYER: string; SET_FILTER: string; - SET_FILTER_ANIMATION_TIME: string; + SET_FILTER_ANIMATION_TIME: string; SET_FILTER_ANIMATION_WINDOW: string; SHOW_DATASET_TABLE: string; UPDATE_LAYER_BLENDING: string; diff --git a/src/constants/default-settings.js b/src/constants/default-settings.js index fae017c0a8..88be77ec87 100644 --- a/src/constants/default-settings.js +++ b/src/constants/default-settings.js @@ -790,6 +790,9 @@ export const DEFAULT_NOTIFICATION_TOPICS = keyMirror({ file: null }); +// Minimum time between identical notifications about deck.gl errors +export const THROTTLE_NOTIFICATION_TIME = 2500; + // Animation export const BASE_SPEED = 600; export const FPS = 60; diff --git a/src/reducers/ui-state-updaters.d.ts b/src/reducers/ui-state-updaters.d.ts index c8842f9883..4b147ce096 100644 --- a/src/reducers/ui-state-updaters.d.ts +++ b/src/reducers/ui-state-updaters.d.ts @@ -174,9 +174,7 @@ export function setExportMapHTMLModeUpdater( state: UiState, action: UiStateActions.SetExportHTMLMapModeUpdaterAction ): UiState; -export function showDatasetTableUpdater( - state: UiState -) +export function showDatasetTableUpdater(state: UiState); export function setLocaleUpdater( state: UiState, action: UiStateActions.SetLocaleUpdaterAction @@ -186,7 +184,10 @@ export function loadFilesUpdater(state: UiState, action: LoadFilesUpdaterAction) export function loadFilesErrUpdater(state: UiState, action: LoadFilesErrUpdaterAction): UiState; export function loadFilesSuccessUpdater(state: UiState): UiState; export function toggleSplitMapUpdater(state: UiState, action: ToggleSplitMapUpdaterAction): UiState; -export function initUiStateUpdater(state: UiState, action: { - type?: ActionTypes.INIT; - payload: KeplerGlInitPayload; -}): UiState; +export function initUiStateUpdater( + state: UiState, + action: { + type?: ActionTypes.INIT; + payload: KeplerGlInitPayload; + } +): UiState; diff --git a/src/reducers/ui-state-updaters.js b/src/reducers/ui-state-updaters.js index 8561c839dd..cdebabfb87 100644 --- a/src/reducers/ui-state-updaters.js +++ b/src/reducers/ui-state-updaters.js @@ -596,19 +596,31 @@ export const setExportMapHTMLModeUpdater = (state, {payload: mode}) => ({ }); /** - * Add a notification to be displayed + * Adds a new notification. + * Updates a notification in case of matching ids. * @memberof uiStateUpdaters * @param state `uiState` * @param action - * @param action.payload + * @param action.payload Params of a notification * @returns nextState * @type {typeof import('./ui-state-updaters').addNotificationUpdater} * @public */ -export const addNotificationUpdater = (state, {payload}) => ({ - ...state, - notifications: [...(state.notifications || []), createNotification(payload)] -}); +export const addNotificationUpdater = (state, {payload}) => { + let notifications; + + const payloadId = payload?.id; + const notificationToUpdate = payloadId ? state.notifications.find(n => n.id === payloadId) : null; + if (notificationToUpdate) { + notifications = state.notifications.map(n => + n.id === payloadId ? createNotification(payload) : n + ); + } else { + notifications = [...(state.notifications || []), createNotification(payload)]; + } + + return {...state, notifications}; +}; /** * Remove a notification diff --git a/src/utils/notifications-utils.js b/src/utils/notifications-utils.js index ed1f139263..c5e1ec8dd8 100644 --- a/src/utils/notifications-utils.js +++ b/src/utils/notifications-utils.js @@ -30,7 +30,7 @@ import { import {BUG_REPORT_LINK} from 'constants/user-guides'; /** - * Creates a notofication + * Creates a notification * @param {object} opt * @param {string} opt.message - Message to display during the notification * @param {string} opt.type - The type of message. One of DEFAULT_NOTIFICATION_TYPES diff --git a/test/node/reducers/ui-state-test.js b/test/node/reducers/ui-state-test.js index 71355bc3f6..ef40fce460 100644 --- a/test/node/reducers/ui-state-test.js +++ b/test/node/reducers/ui-state-test.js @@ -29,8 +29,9 @@ import { setExportSelectedDataset, setExportDataType, setExportFiltered, + startExportingImage, addNotification, - startExportingImage + removeNotification } from 'actions/ui-state-actions'; import {loadFiles, loadFilesErr} from 'actions/vis-state-actions'; import {keplerGlInit} from 'actions/actions'; @@ -42,7 +43,6 @@ import { DEFAULT_NOTIFICATION_TOPICS, DEFAULT_NOTIFICATION_TYPES } from 'constants/default-settings'; -import {removeNotification} from 'actions/ui-state-actions'; test('#uiStateReducer', t => { t.deepEqual( @@ -230,25 +230,51 @@ test('#uiStateReducer -> SET_EXPORT_FILTERED', t => { }); test('#uiStateReducer -> ADD_NOTIFICATION', t => { - const newState = reducer( - INITIAL_UI_STATE, - addNotification({ - type: DEFAULT_NOTIFICATION_TYPES.error, - message: 'TEST', - topic: DEFAULT_NOTIFICATION_TOPICS.global, - id: 'test-1' - }) + const sharedNotificationId = 'test-notification-id'; + + const notification1 = { + type: DEFAULT_NOTIFICATION_TYPES.error, + message: 'TEST', + topic: DEFAULT_NOTIFICATION_TOPICS.global, + id: 'test-1' + }; + const state0 = reducer(INITIAL_UI_STATE, addNotification(notification1)); + t.equal(state0.notifications.length, 1, 'AddNotification should add one new notification'); + t.deepEqual( + state0.notifications[0], + notification1, + 'AddNotification should have propagated data correctly' ); - t.equal(newState.notifications.length, 1, 'AddNotification should add one new notification'); + const notification2 = { + type: DEFAULT_NOTIFICATION_TYPES.info, + message: 'TEST', + topic: DEFAULT_NOTIFICATION_TOPICS.file, + id: sharedNotificationId + }; + const state1 = reducer(state0, addNotification(notification2)); + t.equal(state1.notifications.length, 2, 'AddNotification should add second notification'); t.deepEqual( - newState.notifications[0], - { - type: DEFAULT_NOTIFICATION_TYPES.error, - message: 'TEST', - topic: DEFAULT_NOTIFICATION_TOPICS.global, - id: 'test-1' - }, + state1.notifications[1], + notification2, + 'AddNotification should have propagated data correctly ' + ); + + const updatedNotification = { + type: DEFAULT_NOTIFICATION_TYPES.error, + message: 'TEST-updated-message', + topic: DEFAULT_NOTIFICATION_TOPICS.global, + id: sharedNotificationId + }; + const state2 = reducer(state1, addNotification(updatedNotification)); + t.equal( + state2.notifications.length, + 2, + "addNotification shouldn't add new notification with same id" + ); + t.deepEqual( + state2.notifications[1], + updatedNotification, 'AddNotification should have propagated data correctly ' );