Skip to content

Commit

Permalink
fix: Handle undefined DashboardData props (#1726)
Browse files Browse the repository at this point in the history
- Handle undefined props on DashboardData. Don't render console panel if
no session
- Fixed a set state on unmounted HeapUsage bug
- Removed chatty async interval debug logging

fixes #1684

**Testing**
- Run server with PSK enabled
- Login and open any file
- Logout should no longer show "ConsolePanel.tsx:372 Uncaught TypeError:
Cannot destructure property 'config' of 'sessionWrapper' as it is
undefined." error in debug console (Note that login will still fail
until #1685 is done)
  • Loading branch information
bmingles authored Jan 22, 2024
1 parent e79297b commit 45fa929
Show file tree
Hide file tree
Showing 10 changed files with 149 additions and 76 deletions.
21 changes: 18 additions & 3 deletions packages/code-studio/src/main/AppMainContainer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ interface AppMainContainerProps {
match: {
params: { notebookPath: string };
};
connection: IdeConnection;
connection?: IdeConnection;
session?: IdeSession;
sessionConfig?: SessionConfig;
setActiveTool: (tool: string) => void;
Expand Down Expand Up @@ -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'
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -650,6 +662,7 @@ export class AppMainContainer extends Component<
): DehydratedDashboardPanelProps & { fetch?: () => Promise<unknown> } {
const { connection } = this.props;
const { metadata } = props;

if (
metadata?.type != null &&
(metadata?.id != null || metadata?.name != null)
Expand All @@ -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);
}

Expand All @@ -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,
});
}
Expand Down
11 changes: 9 additions & 2 deletions packages/console/src/HeapUsage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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) {
Expand All @@ -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,
Expand Down
41 changes: 18 additions & 23 deletions packages/dashboard-core-plugins/src/panels/ConsolePanel.test.tsx
Original file line number Diff line number Diff line change
@@ -1,20 +1,24 @@
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<string, unknown>),
Console: props => mockConsole(props),
default: props => mockConsole(props),
}));

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({
Expand Down Expand Up @@ -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<EventEmitter>(),
container = TestUtils.createMockProxy<Container>({
tab: undefined,
}),
commandHistoryStorage = makeCommandHistoryStorage(),
timeZone = 'MockTimeZone',
sessionWrapper = makeSessionWrapper(),
Expand All @@ -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();
});

Expand Down
19 changes: 17 additions & 2 deletions packages/dashboard-core-plugins/src/panels/ConsolePanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -63,7 +64,7 @@ interface ConsolePanelProps extends DashboardPanelProps {

panelState?: PanelState;

sessionWrapper: SessionWrapper;
sessionWrapper?: SessionWrapper;

timeZone: string;
unzip?: (file: File) => Promise<JSZipObject[]>;
Expand Down Expand Up @@ -159,6 +160,10 @@ export class ConsolePanel extends PureComponent<

subscribeToFieldUpdates(): void {
const { sessionWrapper } = this.props;
if (sessionWrapper == null) {
return;
}

const { session } = sessionWrapper;

this.objectSubscriptionCleanup = session.subscribeToFieldUpdates(
Expand Down Expand Up @@ -244,6 +249,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 =
Expand Down Expand Up @@ -359,7 +367,7 @@ export class ConsolePanel extends PureComponent<
return <ObjectIcon type={type} />;
}

render(): ReactElement {
render(): ReactElement | null {
const {
commandHistoryStorage,
glContainer,
Expand All @@ -368,6 +376,13 @@ export class ConsolePanel extends PureComponent<
timeZone,
unzip,
} = this.props;

if (sessionWrapper == null) {
return (
<LoadingOverlay isLoading={false} errorMessage="Console is disabled." />
);
}

const { consoleSettings, error, objectMap } = this.state;
const { config, session, connection, details = {}, dh } = sessionWrapper;
const { workerName, processInfoId } = details;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ const log = Log.module('FileExplorerPanel');

type StateProps = {
fileStorage: FileStorage;
language: string;
language?: string;
session?: IdeSession;
};

Expand Down
4 changes: 2 additions & 2 deletions packages/dashboard-core-plugins/src/panels/LogPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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<LogPanelProps, LogPanelState> {
Expand Down
30 changes: 16 additions & 14 deletions packages/dashboard-core-plugins/src/panels/NotebookPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ interface Metadata extends PanelMetadata {
id: string;
}
interface NotebookSetting {
isMinimapEnabled: boolean;
isMinimapEnabled?: boolean;
}

interface FileMetadata {
Expand All @@ -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<WorkspaceSettings>) => void;
}

Expand Down Expand Up @@ -790,7 +795,7 @@ class NotebookPanel extends Component<NotebookPanelProps, NotebookPanelState> {
const { defaultNotebookSettings, updateSettings } = this.props;
const newSettings = {
defaultNotebookSettings: {
isMinimapEnabled: !defaultNotebookSettings.isMinimapEnabled,
isMinimapEnabled: !(defaultNotebookSettings.isMinimapEnabled ?? false),
},
};
updateSettings(newSettings);
Expand Down Expand Up @@ -1176,10 +1181,10 @@ class NotebookPanel extends Component<NotebookPanelProps, NotebookPanelState> {
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'
);
}
Expand Down Expand Up @@ -1216,7 +1221,7 @@ class NotebookPanel extends Component<NotebookPanelProps, NotebookPanelState> {
const isExistingItem = fileMetadata?.id != null;
const settings = this.getSettings(
initialSettings,
defaultNotebookSettings.isMinimapEnabled
defaultNotebookSettings.isMinimapEnabled ?? false
);
const isSessionConnected = session != null;
const isLanguageMatching = sessionLanguage === settings.language;
Expand Down Expand Up @@ -1428,10 +1433,7 @@ class NotebookPanel extends Component<NotebookPanelProps, NotebookPanelState> {
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(
Expand All @@ -1443,7 +1445,7 @@ const mapStateToProps = (
const { type: sessionLanguage } = sessionConfig ?? {};
return {
fileStorage,
defaultNotebookSettings: defaultNotebookSettings as NotebookSetting,
defaultNotebookSettings,
session,
sessionLanguage,
};
Expand Down
Loading

0 comments on commit 45fa929

Please sign in to comment.