Fix notebook kernel selection and auto-kernel-start (#254)

* Fix bug: cannot select kernel. Properly plug-in kernelspecs middleware to redux store configuration

* Properly auto-launch kernel with nteract's epic

* Keep kernel lazy start for notebook viewer

* Add unit tets

src/Explorer/Notebook/NotebookClientV2.ts

@@ -234,7 +234,13 @@ export class NotebookClientV2 {
       console.error(`${title}: ${message}`);
     };
 
-    this.store = configureStore(initialState, params.contentProvider, traceErrorFct, [cacheKernelSpecsMiddleware]);
+    this.store = configureStore(
+      initialState,
+      params.contentProvider,
+      traceErrorFct,
+      [cacheKernelSpecsMiddleware],
+      !params.isReadOnly
+    );
 
     // Additional configuration
     this.store.dispatch(configOption("editorType").action(params.cellEditorType ?? "monaco"));

src/Explorer/Notebook/NotebookComponent/NotebookComponentBootstrapper.tsx

@@ -98,7 +98,7 @@ export class NotebookComponentBootstrapper {
       actions.fetchContentFulfilled({
         filepath: undefined,
         model: NotebookComponentBootstrapper.wrapModelIntoContent(name, undefined, content),
-        kernelRef: undefined, // must be undefined or it will be auto-started by the epic
+        kernelRef: undefined,
         contentRef: this.contentRef
       })
     );

src/Explorer/Notebook/NotebookComponent/epics.test.ts

@@ -3,11 +3,11 @@ import { StateObservable } from "redux-observable";
 import { Subject, of } from "rxjs";
 import { toArray } from "rxjs/operators";
 import { makeNotebookRecord } from "@nteract/commutable";
-import { actions, state } from "@nteract/core";
+import { actions, state, epics } from "@nteract/core";
 import * as sinon from "sinon";
 
 import { CdbAppState, makeCdbRecord } from "./types";
-import { launchWebSocketKernelEpic, autoStartKernelEpic } from "./epics";
+import { launchWebSocketKernelEpic } from "./epics";
 import { NotebookUtil } from "../NotebookUtil";
 
 import { sessions } from "rx-jupyter";
@@ -491,55 +491,3 @@ describe("launchWebSocketKernelEpic", () => {
     });
   });
 });
-
-describe("autoStartKernelEpic", () => {
-  const contentRef = "fakeContentRef";
-  const kernelRef = "fake";
-
-  it("automatically starts kernel when content fetch is successful if kernelRef is defined", async () => {
-    const state$ = new StateObservable(new Subject<CdbAppState>(), initialState);
-
-    const action$ = of(
-      actions.fetchContentFulfilled({
-        contentRef,
-        kernelRef,
-        filepath: "filepath",
-        model: {}
-      })
-    );
-
-    const responseActions = await autoStartKernelEpic(action$, state$)
-      .pipe(toArray())
-      .toPromise();
-
-    expect(responseActions).toMatchObject([
-      {
-        type: actions.RESTART_KERNEL,
-        payload: {
-          contentRef,
-          kernelRef,
-          outputHandling: "None"
-        }
-      }
-    ]);
-  });
-
-  it("Don't start kernel when content fetch is successful if kernelRef is not defined", async () => {
-    const state$ = new StateObservable(new Subject<CdbAppState>(), initialState);
-
-    const action$ = of(
-      actions.fetchContentFulfilled({
-        contentRef,
-        kernelRef: undefined,
-        filepath: "filepath",
-        model: {}
-      })
-    );
-
-    const responseActions = await autoStartKernelEpic(action$, state$)
-      .pipe(toArray())
-      .toPromise();
-
-    expect(responseActions).toMatchObject([]);
-  });
-});

src/Explorer/Notebook/NotebookComponent/epics.ts

@@ -95,39 +95,6 @@ const addInitialCodeCellEpic = (
   );
 };
 
-/**
- * Automatically start kernel if kernelRef is present.
- * The kernel is normally lazy-started when a cell is being executed, but a running kernel is
- * required for code completion to work.
- * For notebook viewer, there is no kernel
- * @param action$
- * @param state$
- */
-export const autoStartKernelEpic = (
-  action$: Observable<actions.FetchContentFulfilled>,
-  state$: StateObservable<AppState>
-): Observable<{} | actions.CreateCellBelow> => {
-  return action$.pipe(
-    ofType(actions.FETCH_CONTENT_FULFILLED),
-    mergeMap(action => {
-      const state = state$.value;
-      const { contentRef, kernelRef } = action.payload;
-
-      if (!kernelRef) {
-        return EMPTY;
-      }
-
-      return of(
-        actions.restartKernel({
-          contentRef,
-          kernelRef,
-          outputHandling: "None"
-        })
-      );
-    })
-  );
-};
-
 /**
  * Updated kernels.formWebSocketURL so we pass the userId as a query param
  */
@@ -879,7 +846,6 @@ const closeContentFailedToFetchEpic = (
 
 export const allEpics = [
   addInitialCodeCellEpic,
-  autoStartKernelEpic,
   focusInitialCodeCellEpic,
   notificationsToUserEpic,
   launchWebSocketKernelEpic,

src/Explorer/Notebook/NotebookComponent/store.test.ts

@@ -0,0 +1,13 @@
+import { getCoreEpics } from "./store";
+import { epics } from "@nteract/core";
+
+describe("configure redux store", () => {
+  it("configures store with correct epic if based on autoStartKernelOnNotebookOpen", () => {
+    // For now, assume launchKernelWhenNotebookSetEpic is the last epic
+    let filteredEpics = getCoreEpics(true);
+    expect(filteredEpics.pop()).toEqual(epics.launchKernelWhenNotebookSetEpic);
+
+    filteredEpics = getCoreEpics(false);
+    expect(filteredEpics.pop()).not.toEqual(epics.launchKernelWhenNotebookSetEpic);
+  });
+});

src/Explorer/Notebook/NotebookComponent/store.ts

@@ -1,6 +1,6 @@
 import { AppState, epics as coreEpics, reducers, IContentProvider } from "@nteract/core";
 import { compose, Store, AnyAction, Middleware, Dispatch, MiddlewareAPI } from "redux";
-import { createEpicMiddleware, Epic } from "redux-observable";
+import { Epic } from "redux-observable";
 import { allEpics } from "./epics";
 import { coreReducer, cdbReducer } from "./reducers";
 import { catchError } from "rxjs/operators";
@@ -15,7 +15,8 @@ export default function configureStore(
   initialState: Partial<CdbAppState>,
   contentProvider: IContentProvider,
   onTraceFailure: (title: string, message: string) => void,
-  customMiddlewares?: Middleware<{}, any, Dispatch<AnyAction>>[]
+  customMiddlewares?: Middleware<{}, any, Dispatch<AnyAction>>[],
+  autoStartKernelOnNotebookOpen?: boolean
 ): Store<CdbAppState, AnyAction> {
   /**
    * Catches errors in reducers
@@ -54,6 +55,29 @@ export default function configureStore(
     return epics.map(epic => protect(epic));
   };
 
+  const filteredCoreEpics = getCoreEpics(autoStartKernelOnNotebookOpen);
+
+  const mythConfigureStore = makeConfigureStore<CdbAppState>()({
+    packages: [configuration],
+    reducers: {
+      app: reducers.app,
+      core: coreReducer as any,
+      cdb: cdbReducer
+    },
+    epics: protectEpics([...filteredCoreEpics, ...allEpics]),
+    epicDependencies: { contentProvider },
+    epicMiddleware: customMiddlewares.concat(catchErrorMiddleware),
+    enhancer: composeEnhancers
+  });
+
+  const store = mythConfigureStore(initialState as any);
+
+  // TODO Fix typing issue here: createStore() output type doesn't quite match AppState
+  // return store as Store<AppState, AnyAction>;
+  return store as any;
+}
+
+export const getCoreEpics = (autoStartKernelOnNotebookOpen: boolean): Epic[] => {
   // This list needs to be consistent and in sync with core.allEpics until we figure
   // out how to safely filter out the ones we are overriding here.
   const filteredCoreEpics = [
@@ -79,22 +103,9 @@ export default function configureStore(
     coreEpics.sendInputReplyEpic
   ];
 
-  const mythConfigureStore = makeConfigureStore<CdbAppState>()({
+  if (autoStartKernelOnNotebookOpen) {
-    packages: [configuration],
+    filteredCoreEpics.push(coreEpics.launchKernelWhenNotebookSetEpic);
-    reducers: {
+  }
-      app: reducers.app,
-      core: coreReducer as any,
-      cdb: cdbReducer
-    },
-    epics: protectEpics([...filteredCoreEpics, ...allEpics]),
-    epicDependencies: { contentProvider },
-    epicMiddleware: [catchErrorMiddleware],
-    enhancer: composeEnhancers
-  });
 
-  const store = mythConfigureStore(initialState as any);
+  return filteredCoreEpics;
-
+};
-  // TODO Fix typing issue here: createStore() output type doesn't quite match AppState
-  // return store as Store<AppState, AnyAction>;
-  return store as any;
-}

src/Explorer/Notebook/notebookClientV2.test.ts

@@ -0,0 +1,48 @@
+jest.mock("./NotebookComponent/store");
+jest.mock("@nteract/core");
+import { NotebookClientV2 } from "./NotebookClientV2";
+import configureStore from "./NotebookComponent/store";
+import { defineConfigOption } from "@nteract/mythic-configuration";
+
+describe("auto start kernel", () => {
+  it("configure autoStartKernelOnNotebookOpen properly depending whether notebook is/is not read-only", async () => {
+    (configureStore as jest.Mock).mockReturnValue({
+      dispatch: () => {
+        /* noop */
+      }
+    });
+
+    defineConfigOption({
+      label: "editorType",
+      key: "editorType",
+      defaultValue: "foo"
+    });
+
+    defineConfigOption({
+      label: "autoSaveInterval",
+      key: "autoSaveInterval",
+      defaultValue: 1234
+    });
+
+    [true, false].forEach(isReadOnly => {
+      new NotebookClientV2({
+        connectionInfo: {
+          authToken: "autToken",
+          notebookServerEndpoint: "notebookServerEndpoint"
+        },
+        databaseAccountName: undefined,
+        defaultExperience: undefined,
+        isReadOnly,
+        contentProvider: undefined
+      });
+
+      expect(configureStore).toHaveBeenCalledWith(
+        expect.anything(), // initial state
+        undefined, // content provider
+        expect.anything(), // onTraceFailure
+        expect.anything(), // customMiddlewares
+        !isReadOnly
+      );
+    });
+  });
+});