From 141346d8296af358dde97120078de6ea1f494eb1 Mon Sep 17 00:00:00 2001 From: Brian Ingles Date: Thu, 11 Jan 2024 11:10:18 -0600 Subject: [PATCH 1/6] Handle undefined dashboard settings #1684 --- .../code-studio/src/main/AppMainContainer.tsx | 21 ++++++++-- .../src/panels/ConsolePanel.test.tsx | 41 ++++++++----------- .../src/panels/ConsolePanel.tsx | 16 +++++++- .../src/panels/FileExplorerPanel.tsx | 2 +- .../src/panels/LogPanel.tsx | 4 +- .../src/panels/NotebookPanel.tsx | 30 +++++++------- .../src/redux/selectors.ts | 18 ++++---- packages/react-hooks/src/useAsyncInterval.ts | 20 --------- 8 files changed, 79 insertions(+), 73 deletions(-) diff --git a/packages/code-studio/src/main/AppMainContainer.tsx b/packages/code-studio/src/main/AppMainContainer.tsx index f08e334c9b..3eb433e969 100644 --- a/packages/code-studio/src/main/AppMainContainer.tsx +++ b/packages/code-studio/src/main/AppMainContainer.tsx @@ -123,7 +123,7 @@ interface AppMainContainerProps { match: { params: { notebookPath: string }; }; - connection: IdeConnection; + connection?: IdeConnection; session?: IdeSession; sessionConfig?: SessionConfig; setActiveTool: (tool: string) => void; @@ -255,6 +255,10 @@ export class AppMainContainer extends Component< initWidgets(): void { const { connection } = this.props; + if (connection == null) { + return; + } + if (connection.subscribeToFieldUpdates == null) { log.warn( 'subscribeToFieldUpdates not supported, not initializing widgets' @@ -614,6 +618,10 @@ export class AppMainContainer extends Component< startListeningForDisconnect(): void { const { connection } = this.props; + if (connection == null) { + return; + } + connection.addEventListener( dh.IdeConnection.EVENT_DISCONNECT, this.handleDisconnect @@ -630,6 +638,10 @@ export class AppMainContainer extends Component< stopListeningForDisconnect(): void { const { connection } = this.props; + if (connection == null) { + return; + } + connection.removeEventListener( dh.IdeConnection.EVENT_DISCONNECT, this.handleDisconnect @@ -650,6 +662,7 @@ export class AppMainContainer extends Component< ): DehydratedDashboardPanelProps & { fetch?: () => Promise } { const { connection } = this.props; const { metadata } = props; + if ( metadata?.type != null && (metadata?.id != null || metadata?.name != null) @@ -666,12 +679,14 @@ export class AppMainContainer extends Component< name: metadata.name, title: metadata.name, }; + return { - fetch: () => connection.getObject(widget), + fetch: async () => connection?.getObject(widget), ...props, localDashboardId: id, }; } + return DashboardUtils.hydrate(props, id); } @@ -684,7 +699,7 @@ export class AppMainContainer extends Component< const { connection } = this.props; this.emitLayoutEvent(PanelEvent.OPEN, { dragEvent, - fetch: () => connection.getObject(widget), + fetch: async () => connection?.getObject(widget), widget, }); } diff --git a/packages/dashboard-core-plugins/src/panels/ConsolePanel.test.tsx b/packages/dashboard-core-plugins/src/panels/ConsolePanel.test.tsx index 866d041e38..66159d6e3f 100644 --- a/packages/dashboard-core-plugins/src/panels/ConsolePanel.test.tsx +++ b/packages/dashboard-core-plugins/src/panels/ConsolePanel.test.tsx @@ -1,12 +1,16 @@ import React from 'react'; import { render } from '@testing-library/react'; import { CommandHistoryStorage } from '@deephaven/console'; -import type { Container } from '@deephaven/golden-layout'; +import type { Container, EventEmitter } from '@deephaven/golden-layout'; import type { IdeConnection, IdeSession } from '@deephaven/jsapi-types'; +import { dh } from '@deephaven/jsapi-shim'; import { SessionConfig, SessionWrapper } from '@deephaven/jsapi-utils'; +import { TestUtils } from '@deephaven/utils'; import { ConsolePanel } from './ConsolePanel'; -const mockConsole = jest.fn(() => null); +type IdeSessionConstructor = new (language: string) => IdeSession; + +const mockConsole = jest.fn((_props: unknown) => null); jest.mock('@deephaven/console', () => ({ ...(jest.requireActual('@deephaven/console') as Record), Console: props => mockConsole(props), @@ -14,7 +18,7 @@ jest.mock('@deephaven/console', () => ({ })); function makeSession(language = 'TEST_LANG'): IdeSession { - return new dh.IdeSession(language) as unknown as IdeSession; + return new (dh.IdeSession as unknown as IdeSessionConstructor)(language); } function makeConnection({ @@ -42,31 +46,15 @@ function makeSessionWrapper({ return { session, connection, config, dh }; } -function makeEventHub() { - return { - emit: jest.fn(), - on: jest.fn(), - off: jest.fn(), - trigger: jest.fn(), - unbind: jest.fn(), - }; -} - -function makeGlContainer(): Container { - return { - emit: jest.fn(), - on: jest.fn(), - off: jest.fn(), - } as unknown as Container; -} - function makeCommandHistoryStorage(): CommandHistoryStorage { return {} as CommandHistoryStorage; } function renderConsolePanel({ - eventHub = makeEventHub(), - container = makeGlContainer(), + eventHub = TestUtils.createMockProxy(), + container = TestUtils.createMockProxy({ + tab: undefined, + }), commandHistoryStorage = makeCommandHistoryStorage(), timeZone = 'MockTimeZone', sessionWrapper = makeSessionWrapper(), @@ -78,11 +66,18 @@ function renderConsolePanel({ commandHistoryStorage={commandHistoryStorage} timeZone={timeZone} sessionWrapper={sessionWrapper} + localDashboardId="mock-localDashboardId" + plugins={new Map()} /> ); } beforeEach(() => { + // Mocking the Console component causes it to be treated as a functional + // component which causes React to log an error about passing refs. Disable + // logging to supress this + TestUtils.disableConsoleOutput('error'); + mockConsole.mockClear(); }); diff --git a/packages/dashboard-core-plugins/src/panels/ConsolePanel.tsx b/packages/dashboard-core-plugins/src/panels/ConsolePanel.tsx index c8d9becfb5..d1f2257060 100644 --- a/packages/dashboard-core-plugins/src/panels/ConsolePanel.tsx +++ b/packages/dashboard-core-plugins/src/panels/ConsolePanel.tsx @@ -63,7 +63,7 @@ interface ConsolePanelProps extends DashboardPanelProps { panelState?: PanelState; - sessionWrapper: SessionWrapper; + sessionWrapper?: SessionWrapper; timeZone: string; unzip?: (file: File) => Promise; @@ -159,6 +159,10 @@ export class ConsolePanel extends PureComponent< subscribeToFieldUpdates(): void { const { sessionWrapper } = this.props; + if (sessionWrapper == null) { + return; + } + const { session } = sessionWrapper; this.objectSubscriptionCleanup = session.subscribeToFieldUpdates( @@ -244,6 +248,9 @@ export class ConsolePanel extends PureComponent< handleOpenObject(object: VariableDefinition, forceOpen = true): void { const { sessionWrapper } = this.props; + if (sessionWrapper == null) { + return; + } const { session } = sessionWrapper; const { root } = this.context; const oldPanelId = @@ -359,7 +366,7 @@ export class ConsolePanel extends PureComponent< return ; } - render(): ReactElement { + render(): ReactElement | null { const { commandHistoryStorage, glContainer, @@ -368,6 +375,11 @@ export class ConsolePanel extends PureComponent< timeZone, unzip, } = this.props; + + if (sessionWrapper == null) { + return null; + } + const { consoleSettings, error, objectMap } = this.state; const { config, session, connection, details = {}, dh } = sessionWrapper; const { workerName, processInfoId } = details; diff --git a/packages/dashboard-core-plugins/src/panels/FileExplorerPanel.tsx b/packages/dashboard-core-plugins/src/panels/FileExplorerPanel.tsx index 1b07cd85be..e1c46d555e 100644 --- a/packages/dashboard-core-plugins/src/panels/FileExplorerPanel.tsx +++ b/packages/dashboard-core-plugins/src/panels/FileExplorerPanel.tsx @@ -21,7 +21,7 @@ const log = Log.module('FileExplorerPanel'); type StateProps = { fileStorage: FileStorage; - language: string; + language?: string; session?: IdeSession; }; diff --git a/packages/dashboard-core-plugins/src/panels/LogPanel.tsx b/packages/dashboard-core-plugins/src/panels/LogPanel.tsx index 1ad84167d9..4e1ce3e66c 100644 --- a/packages/dashboard-core-plugins/src/panels/LogPanel.tsx +++ b/packages/dashboard-core-plugins/src/panels/LogPanel.tsx @@ -14,11 +14,11 @@ import { getDashboardSessionWrapper } from '../redux'; const log = Log.module('LogPanel'); interface LogPanelProps extends DashboardPanelProps { - session: IdeSession; + session?: IdeSession; } interface LogPanelState { - session: IdeSession; + session?: IdeSession; } class LogPanel extends PureComponent { diff --git a/packages/dashboard-core-plugins/src/panels/NotebookPanel.tsx b/packages/dashboard-core-plugins/src/panels/NotebookPanel.tsx index 3f5e306317..05cf51859f 100644 --- a/packages/dashboard-core-plugins/src/panels/NotebookPanel.tsx +++ b/packages/dashboard-core-plugins/src/panels/NotebookPanel.tsx @@ -64,7 +64,7 @@ interface Metadata extends PanelMetadata { id: string; } interface NotebookSetting { - isMinimapEnabled: boolean; + isMinimapEnabled?: boolean; } interface FileMetadata { @@ -78,16 +78,21 @@ interface PanelState { fileMetadata: FileMetadata | null; } -interface NotebookPanelProps extends DashboardPanelProps { +interface NotebookPanelMappedProps { + defaultNotebookSettings: NotebookSetting; fileStorage: FileStorage; + session?: IdeSession; + sessionLanguage?: string; +} + +interface NotebookPanelProps + extends DashboardPanelProps, + NotebookPanelMappedProps { isDashboardActive: boolean; isPreview: boolean; metadata: Metadata; - session: IdeSession; - sessionLanguage: string; panelState: PanelState; notebooksUrl: string; - defaultNotebookSettings: NotebookSetting; updateSettings: (settings: Partial) => void; } @@ -790,7 +795,7 @@ class NotebookPanel extends Component { const { defaultNotebookSettings, updateSettings } = this.props; const newSettings = { defaultNotebookSettings: { - isMinimapEnabled: !defaultNotebookSettings.isMinimapEnabled, + isMinimapEnabled: !(defaultNotebookSettings.isMinimapEnabled ?? false), }, }; updateSettings(newSettings); @@ -1176,10 +1181,10 @@ class NotebookPanel extends Component { const { defaultNotebookSettings } = this.props; const { settings: initialSettings } = this.state; return this.getOverflowActions( - defaultNotebookSettings.isMinimapEnabled, + defaultNotebookSettings.isMinimapEnabled ?? false, this.getSettings( initialSettings, - defaultNotebookSettings.isMinimapEnabled + defaultNotebookSettings.isMinimapEnabled ?? false ).wordWrap === 'on' ); } @@ -1216,7 +1221,7 @@ class NotebookPanel extends Component { const isExistingItem = fileMetadata?.id != null; const settings = this.getSettings( initialSettings, - defaultNotebookSettings.isMinimapEnabled + defaultNotebookSettings.isMinimapEnabled ?? false ); const isSessionConnected = session != null; const isLanguageMatching = sessionLanguage === settings.language; @@ -1428,10 +1433,7 @@ class NotebookPanel extends Component { const mapStateToProps = ( state: RootState, ownProps: { localDashboardId: string } -): Pick< - NotebookPanelProps, - 'defaultNotebookSettings' | 'fileStorage' | 'session' | 'sessionLanguage' -> => { +): NotebookPanelMappedProps => { const fileStorage = getFileStorage(state); const defaultNotebookSettings = getDefaultNotebookSettings(state); const sessionWrapper = getDashboardSessionWrapper( @@ -1443,7 +1445,7 @@ const mapStateToProps = ( const { type: sessionLanguage } = sessionConfig ?? {}; return { fileStorage, - defaultNotebookSettings: defaultNotebookSettings as NotebookSetting, + defaultNotebookSettings, session, sessionLanguage, }; diff --git a/packages/dashboard-core-plugins/src/redux/selectors.ts b/packages/dashboard-core-plugins/src/redux/selectors.ts index 12806932e0..597b16b311 100644 --- a/packages/dashboard-core-plugins/src/redux/selectors.ts +++ b/packages/dashboard-core-plugins/src/redux/selectors.ts @@ -7,6 +7,8 @@ import { Link } from '../linker/LinkerUtils'; import { FilterSet } from '../panels'; import { ColumnSelectionValidator } from '../linker/ColumnSelectionValidator'; +const EMPTY_OBJECT = Object.freeze({}); + const EMPTY_MAP = new Map(); const EMPTY_ARRAY = Object.freeze([]); @@ -107,10 +109,8 @@ export const getDashboardConsoleSettings = ( store: RootState, dashboardId: string ): Record => - getDashboardData(store, dashboardId).consoleSettings as Record< - string, - unknown - >; + (getDashboardData(store, dashboardId).consoleSettings ?? + EMPTY_OBJECT) as Record; /** * @@ -121,8 +121,8 @@ export const getDashboardConsoleSettings = ( export const getDashboardConnection = ( store: RootState, dashboardId: string -): IdeConnection => - getDashboardData(store, dashboardId).connection as IdeConnection; +): IdeConnection | undefined => + getDashboardData(store, dashboardId).connection as IdeConnection | undefined; /** * @@ -133,5 +133,7 @@ export const getDashboardConnection = ( export const getDashboardSessionWrapper = ( store: RootState, dashboardId: string -): SessionWrapper => - getDashboardData(store, dashboardId).sessionWrapper as SessionWrapper; +): SessionWrapper | undefined => + getDashboardData(store, dashboardId).sessionWrapper as + | SessionWrapper + | undefined; diff --git a/packages/react-hooks/src/useAsyncInterval.ts b/packages/react-hooks/src/useAsyncInterval.ts index c5f27156ee..84a772dd1d 100644 --- a/packages/react-hooks/src/useAsyncInterval.ts +++ b/packages/react-hooks/src/useAsyncInterval.ts @@ -1,9 +1,6 @@ import { useCallback, useEffect, useRef } from 'react'; -import Log from '@deephaven/log'; import { useIsMountedRef } from './useIsMountedRef'; -const log = Log.module('useAsyncInterval'); - /** * Calls the given async callback at a target interval. * @@ -41,12 +38,6 @@ export function useAsyncInterval( ? targetIntervalMs : now - trackingStartedRef.current; - log.debug( - `tick #${trackingCountRef.current}.`, - elapsedSinceLastTick, - 'ms elapsed since last tick.' - ); - trackingStartedRef.current = now; await callback(); @@ -62,21 +53,10 @@ export function useAsyncInterval( const nextTickInterval = Math.max(0, targetIntervalMs - overage); - log.debug( - 'Next tick target:', - targetIntervalMs, - ', overage', - overage, - ', adjusted:', - nextTickInterval - ); - setTimeoutRef.current = window.setTimeout(tick, nextTickInterval); }, [callback, isMountedRef, targetIntervalMs]); useEffect(() => { - log.debug('Setting target interval:', targetIntervalMs); - trackingStartedRef.current = null; tick(); From 6507abe82ddcd83cdd513f0ecda049658b91b910 Mon Sep 17 00:00:00 2001 From: Brian Ingles Date: Thu, 11 Jan 2024 11:11:03 -0600 Subject: [PATCH 2/6] Fix state update on unmounted heap usage #1684 --- packages/console/src/HeapUsage.tsx | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/packages/console/src/HeapUsage.tsx b/packages/console/src/HeapUsage.tsx index 91892321dd..4d0b47fdef 100644 --- a/packages/console/src/HeapUsage.tsx +++ b/packages/console/src/HeapUsage.tsx @@ -4,7 +4,7 @@ import { Tooltip } from '@deephaven/components'; import type { QueryConnectable } from '@deephaven/jsapi-types'; import { Plot, useChartTheme } from '@deephaven/chart'; import Log from '@deephaven/log'; -import { useAsyncInterval } from '@deephaven/react-hooks'; +import { useAsyncInterval, useIsMountedRef } from '@deephaven/react-hooks'; import './HeapUsage.scss'; const log = Log.module('HeapUsage'); @@ -41,9 +41,16 @@ function HeapUsage({ usages: [], }); + const isMountedRef = useIsMountedRef(); + const setUsageUpdateInterval = useCallback(async () => { try { const newUsage = await connection.getWorkerHeapInfo(); + + if (!isMountedRef.current) { + return; + } + setMemoryUsage(newUsage); if (bgMonitoring || isOpen) { @@ -69,7 +76,7 @@ function HeapUsage({ } catch (e) { log.warn('Unable to get heap usage', e); } - }, [isOpen, connection, monitorDuration, bgMonitoring]); + }, [connection, isMountedRef, bgMonitoring, isOpen, monitorDuration]); useAsyncInterval( setUsageUpdateInterval, From 7d20ebbe2f61e751ebcb242adc250d95b6281815 Mon Sep 17 00:00:00 2001 From: Brian Ingles Date: Fri, 19 Jan 2024 09:29:16 -0600 Subject: [PATCH 3/6] Console disabled message #1684 --- .../dashboard-core-plugins/src/panels/ConsolePanel.tsx | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/dashboard-core-plugins/src/panels/ConsolePanel.tsx b/packages/dashboard-core-plugins/src/panels/ConsolePanel.tsx index d1f2257060..da1a2d3062 100644 --- a/packages/dashboard-core-plugins/src/panels/ConsolePanel.tsx +++ b/packages/dashboard-core-plugins/src/panels/ConsolePanel.tsx @@ -4,6 +4,7 @@ import React, { PureComponent, ReactElement, RefObject } from 'react'; import shortid from 'shortid'; import debounce from 'lodash.debounce'; import { connect } from 'react-redux'; +import { LoadingOverlay } from '@deephaven/components'; import { CommandHistoryStorage, Console, @@ -377,7 +378,12 @@ export class ConsolePanel extends PureComponent< } = this.props; if (sessionWrapper == null) { - return null; + return ( + + ); } const { consoleSettings, error, objectMap } = this.state; From c2b029ced23ebf4686d23c104bafe5835a82457b Mon Sep 17 00:00:00 2001 From: Brian Ingles Date: Fri, 19 Jan 2024 10:45:56 -0600 Subject: [PATCH 4/6] Error handling #1684 --- .../react-hooks/src/useAsyncInterval.test.ts | 56 ++++++++++++++++++- packages/react-hooks/src/useAsyncInterval.ts | 9 ++- 2 files changed, 61 insertions(+), 4 deletions(-) diff --git a/packages/react-hooks/src/useAsyncInterval.test.ts b/packages/react-hooks/src/useAsyncInterval.test.ts index 95db744678..d33e248914 100644 --- a/packages/react-hooks/src/useAsyncInterval.test.ts +++ b/packages/react-hooks/src/useAsyncInterval.test.ts @@ -1,9 +1,24 @@ import { renderHook, act } from '@testing-library/react-hooks'; +import Log from '@deephaven/log'; import { TestUtils } from '@deephaven/utils'; import useAsyncInterval from './useAsyncInterval'; +jest.mock('@deephaven/log', () => { + const logger = { + error: jest.fn(), + }; + return { + __esModule: true, + default: { + module: jest.fn(() => logger), + }, + }; +}); + const { asMock } = TestUtils; +const mockLoggerInstance = Log.module('mock.logger'); + beforeEach(() => { jest.clearAllMocks(); expect.hasAssertions(); @@ -16,12 +31,22 @@ afterAll(() => { }); describe('useAsyncInterval', () => { - function createCallback(ms: number) { + /** + * Creates a callback function that resolves after the given number of + * milliseconds. Accepts an optional array of booleans that determine whether + * calls to the callback will reject instead of resolve. They are mapped by + * index to the order in which the callback is called. + */ + function createCallback(ms: number, rejectWith: (Error | undefined)[] = []) { return jest .fn( async (): Promise => - new Promise(resolve => { - setTimeout(resolve, ms); + new Promise((resolve, reject) => { + const rejectArg = rejectWith.shift(); + setTimeout( + rejectArg == null ? resolve : () => reject(rejectArg), + ms + ); // Don't track the above call to `setTimeout` asMock(setTimeout).mock.calls.pop(); @@ -155,4 +180,29 @@ describe('useAsyncInterval', () => { expect(window.setTimeout).not.toHaveBeenCalled(); }); + + it('should handle tick errors', async () => { + const callbackDelayMs = 50; + const mockError = new Error('mock.error'); + const callback = createCallback(callbackDelayMs, [mockError, undefined]); + + renderHook(() => useAsyncInterval(callback, targetIntervalMs)); + + // First callback fires immediately + expect(callback).toHaveBeenCalledTimes(1); + + // Mimick the callback Promise rejecting + act(() => jest.advanceTimersByTime(callbackDelayMs)); + await TestUtils.flushPromises(); + + expect(mockLoggerInstance.error).toHaveBeenCalledWith( + 'A tick error occurred:', + mockError + ); + + // Advance to next interval + act(() => jest.advanceTimersByTime(targetIntervalMs)); + + expect(callback).toHaveBeenCalledTimes(2); + }); }); diff --git a/packages/react-hooks/src/useAsyncInterval.ts b/packages/react-hooks/src/useAsyncInterval.ts index 84a772dd1d..a40b0cae4a 100644 --- a/packages/react-hooks/src/useAsyncInterval.ts +++ b/packages/react-hooks/src/useAsyncInterval.ts @@ -1,6 +1,9 @@ import { useCallback, useEffect, useRef } from 'react'; +import Log from '@deephaven/log'; import { useIsMountedRef } from './useIsMountedRef'; +const log = Log.module('useAsyncInterval'); + /** * Calls the given async callback at a target interval. * @@ -40,7 +43,11 @@ export function useAsyncInterval( trackingStartedRef.current = now; - await callback(); + try { + await callback(); + } catch (err) { + log.error('A tick error occurred:', err); + } if (!isMountedRef.current) { return; From d6259daa3a21cde98590354028216b6ed7d9616c Mon Sep 17 00:00:00 2001 From: Brian Ingles Date: Fri, 19 Jan 2024 10:49:58 -0600 Subject: [PATCH 5/6] Fixed comment #1684 --- packages/react-hooks/src/useAsyncInterval.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/react-hooks/src/useAsyncInterval.test.ts b/packages/react-hooks/src/useAsyncInterval.test.ts index d33e248914..6d7ebf6179 100644 --- a/packages/react-hooks/src/useAsyncInterval.test.ts +++ b/packages/react-hooks/src/useAsyncInterval.test.ts @@ -33,9 +33,9 @@ afterAll(() => { describe('useAsyncInterval', () => { /** * Creates a callback function that resolves after the given number of - * milliseconds. Accepts an optional array of booleans that determine whether - * calls to the callback will reject instead of resolve. They are mapped by - * index to the order in which the callback is called. + * milliseconds. Accepts an optional array of Error | undefined. They are + * mapped by index to the order in which the callback is called. An Error + * instance will cause a rejection, undefined will cause a resolution. */ function createCallback(ms: number, rejectWith: (Error | undefined)[] = []) { return jest From 493c5a1bf6881cc7097af2d1df09ab943bede613 Mon Sep 17 00:00:00 2001 From: Brian Ingles Date: Mon, 22 Jan 2024 12:45:58 -0600 Subject: [PATCH 6/6] Fixed console message #1684 --- packages/dashboard-core-plugins/src/panels/ConsolePanel.tsx | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/dashboard-core-plugins/src/panels/ConsolePanel.tsx b/packages/dashboard-core-plugins/src/panels/ConsolePanel.tsx index da1a2d3062..9ffdbf330a 100644 --- a/packages/dashboard-core-plugins/src/panels/ConsolePanel.tsx +++ b/packages/dashboard-core-plugins/src/panels/ConsolePanel.tsx @@ -379,10 +379,7 @@ export class ConsolePanel extends PureComponent< if (sessionWrapper == null) { return ( - + ); }