Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Handle undefined DashboardData props #1726

Merged
merged 6 commits into from
Jan 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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;
mofojed marked this conversation as resolved.
Show resolved Hide resolved
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;
}
bmingles marked this conversation as resolved.
Show resolved Hide resolved

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
Loading