Address review comments

Author: vbabich

package-lock.json

@@ -170,8 +170,10 @@ function AppInit(props: AppInitProps) {
       const coreClient = createCoreClient();
       const authType = getAuthType();
       log.info(`Login using auth type ${authType}...`);
-      const loginOptions = await getLoginOptions(authType);
-      const sessionDetails = await getSessionDetails(authType);
+      const [loginOptions, sessionDetails] = await Promise.all([
+        getLoginOptions(authType),
+        getSessionDetails(authType),
+      ]);
       await coreClient.login(loginOptions);
 
       const newPlugins = await loadPlugins();

packages/code-studio/src/main/AppInit.tsx

@@ -320,7 +320,7 @@ export class ConsolePanel extends PureComponent<
       unzip,
     } = this.props;
     const { consoleSettings, error, objectMap } = this.state;
-    const { config, session, connection, details } = sessionWrapper;
+    const { config, session, connection, details = {} } = sessionWrapper;
     const { workerName, processInfoId } = details;
     const { id: sessionId, type: language } = config;
 

packages/dashboard-core-plugins/src/panels/ConsolePanel.tsx

@@ -23,7 +23,7 @@ export interface SessionWrapper {
   session: IdeSession;
   connection: IdeConnection;
   config: SessionConfig;
-  details: SessionDetails;
+  details?: SessionDetails;
 }
 
 /**

packages/dashboard-core-plugins/src/redux/actions.ts

@@ -1,17 +1,24 @@
-import { TimeoutError } from '@deephaven/utils';
-import { makeMessage, requestParentResponse } from './MessageUtils';
+import {
+  makeMessage,
+  makeResponse,
+  Message,
+  requestParentResponse,
+} from './MessageUtils';
 
-// describe('Throws exception if called on a window without parent', async () => {
-//   expect(await requestParentResponse<string>('request', 'response')).toThrow();
-// });
-
-jest.useFakeTimers();
+it('Throws an exception if called on a window without parent', async () => {
+  await expect(requestParentResponse('request')).rejects.toThrow(
+    'window.opener is null, unable to send request.'
+  );
+});
 
 describe('requestParentResponse', () => {
-  const mockPostMessage = jest.fn();
   let addListenerSpy: jest.SpyInstance;
   let removeListenerSpy: jest.SpyInstance;
   let listenerCallback;
+  let messageId;
+  const mockPostMessage = jest.fn((data: Message<unknown>) => {
+    messageId = data.id;
+  });
   const originalWindowOpener = window.opener;
   beforeEach(() => {
     addListenerSpy = jest
@@ -27,12 +34,13 @@ describe('requestParentResponse', () => {
     removeListenerSpy.mockRestore();
     mockPostMessage.mockClear();
     window.opener = originalWindowOpener;
+    messageId = undefined;
   });
 
   it('Posts message to parent and subscribes to response', async () => {
-    requestParentResponse<string>('request', 'response');
+    requestParentResponse('request');
     expect(mockPostMessage).toHaveBeenCalledWith(
-      expect.objectContaining({ message: 'request' }),
+      expect.objectContaining(makeMessage('request', messageId)),
       '*'
     );
     expect(addListenerSpy).toHaveBeenCalledWith(
@@ -43,31 +51,32 @@ describe('requestParentResponse', () => {
 
   it('Resolves with the payload from the parent window response and unsubscribes', async () => {
     const PAYLOAD = 'PAYLOAD';
-    const promise = requestParentResponse<string>('request', 'response');
+    const promise = requestParentResponse('request');
     listenerCallback({
-      data: makeMessage<string>('response', PAYLOAD),
+      data: makeResponse(messageId, PAYLOAD),
     });
     const result = await promise;
     expect(result).toBe(PAYLOAD);
     expect(removeListenerSpy).toHaveBeenCalledWith('message', listenerCallback);
   });
 
-  it('Rejects on time out', async () => {
-    const promise = requestParentResponse<string>('request', 'response');
-    expect(removeListenerSpy).not.toHaveBeenCalled();
+  it('Ignores unrelated response, rejects on timeout', async () => {
+    jest.useFakeTimers();
+    const promise = requestParentResponse('request');
+    listenerCallback({
+      data: makeMessage('wrong-id'),
+    });
     jest.runOnlyPendingTimers();
-    expect(removeListenerSpy).toHaveBeenCalled();
-    await expect(promise).rejects.toThrow(TimeoutError);
+    await expect(promise).rejects.toThrow('Request timed out');
+    jest.useRealTimers();
   });
 
-  // it('Ignores unrelated messages', async () => {
-  //   window.opener = { postMessage: mockPostMessage };
-  //   const PAYLOAD = 'PAYLOAD';
-  //   const promise = requestParentResponse<string>('request', 'response');
-  //   listenerCallback({
-  //     data: makeMessage<string>('response', `TEST_${messageId}`, PAYLOAD),
-  //   });
-  //   const result = await promise;
-  //   expect(result).toBe(PAYLOAD);
-  // });
+  it('Times out if no response', async () => {
+    jest.useFakeTimers();
+    const promise = requestParentResponse('request');
+    jest.runOnlyPendingTimers();
+    expect(removeListenerSpy).toHaveBeenCalled();
+    await expect(promise).rejects.toThrow('Request timed out');
+    jest.useRealTimers();
+  });
 });

packages/jsapi-utils/src/MessageUtils.test.ts

@@ -1,46 +1,72 @@
+import shortid from 'shortid';
 import Log from '@deephaven/log';
 import { TimeoutError } from '@deephaven/utils';
 
 const log = Log.module('MessageUtils');
 
-export const LOGIN_OPTIONS_REQUEST = 'io.deephaven.loginOptions.request';
-export const LOGIN_OPTIONS_RESPONSE = 'io.deephaven.loginOptions.response';
+export const LOGIN_OPTIONS_REQUEST =
+  'io.deephaven.message.LoginOptions.request';
 
-export const SESSION_DETAILS_REQUEST = 'io.deephaven.sessionDetails.request';
-export const SESSION_DETAILS_RESPONSE = 'io.deephaven.sessionDetails.response';
+export const SESSION_DETAILS_REQUEST =
+  'io.deephaven.message.SessionDetails.request';
 
 export interface Message<T> {
   message: string;
   payload?: T;
-  id?: string;
+  id: string;
 }
 
-export function makeMessage<T>(message: string, payload?: T): Message<T> {
-  return { message, payload };
+export interface Response<T> {
+  id: string;
+  payload: T;
+}
+
+/**
+ * Make message object with optional payload
+ * @param message Message string
+ * @param id Unique message id
+ * @param payload Payload to send
+ * @returns Message
+ */
+export function makeMessage<T>(
+  message: string,
+  id = shortid(),
+  payload?: T
+): Message<T> {
+  return { message, id, payload };
+}
+
+/**
+ * Make response object for given message id
+ * @param messageId Id of the request message to respond to
+ * @param payload Payload to respond with
+ * @returns Response
+ */
+export function makeResponse<T>(messageId: string, payload: T): Response<T> {
+  return { id: messageId, payload };
 }
 
 /**
  * Request data from the parent window and wait for response
  * @param request Request message to send to the parent window
- * @param response Response message to wait for
  * @param timeout Timeout in ms
  * @returns Payload of the given type, or undefined
  */
 export async function requestParentResponse<T>(
   request: string,
-  response: string,
   timeout = 30000
-): Promise<T | undefined> {
+): Promise<T> {
   if (window.opener == null) {
     throw new Error('window.opener is null, unable to send request.');
   }
   return new Promise((resolve, reject) => {
     let timeoutId: NodeJS.Timeout;
-    const listener = (event: MessageEvent<Message<T>>) => {
+    const id = shortid();
+    const listener = (event: MessageEvent<Response<T>>) => {
       const { data } = event;
       log.debug('Received message', data);
-      if (data?.message !== response) {
-        log.debug('Ignore received message', data);
+      if (data?.id !== id) {
+        log.debug("Ignore message, id doesn't match", data);
         return;
       }
       clearTimeout(timeoutId);
@@ -52,6 +78,6 @@ export async function requestParentResponse<T>(
       window.removeEventListener('message', listener);
       reject(new TimeoutError('Request timed out'));
     }, timeout);
-    window.opener.postMessage(makeMessage(request), '*');
+    window.opener.postMessage(makeMessage(request, id), '*');
   });
 }