Merge pull request #7676 from afshin/issue-4009

Resolve race condition between default file browser and tree URLs.

packages/application-extension/package.json

@@ -40,6 +40,8 @@
     "@jupyterlab/apputils": "^2.0.0-alpha.4",
     "@jupyterlab/coreutils": "^4.0.0-alpha.4",
     "@lumino/algorithm": "^1.2.1",
+    "@lumino/coreutils": "^1.4.0",
+    "@lumino/disposable": "^1.3.2",
     "@lumino/widgets": "^1.9.4",
     "react": "~16.9.0"
   },

packages/application-extension/src/index.tsx

@@ -33,6 +33,10 @@ import {
 
 import { each, iter, toArray } from '@lumino/algorithm';
 
+import { PromiseDelegate } from '@lumino/coreutils';
+
+import { DisposableDelegate, DisposableSet } from '@lumino/disposable';
+
 import { Widget, DockLayout } from '@lumino/widgets';
 
 import * as React from 'react';
@@ -247,63 +251,87 @@ const router: JupyterFrontEndPlugin<IRouter> = {
 };
 
 /**
- * The tree route handler provider.
+ * The default tree route resolver plugin.
  */
-const tree: JupyterFrontEndPlugin<void> = {
-  id: '@jupyterlab/application-extension:tree',
+const tree: JupyterFrontEndPlugin<JupyterFrontEnd.ITreeResolver> = {
+  id: '@jupyterlab/application-extension:tree-resolver',
   autoStart: true,
   requires: [JupyterFrontEnd.IPaths, IRouter, IWindowResolver],
+  provides: JupyterFrontEnd.ITreeResolver,
   activate: (
     app: JupyterFrontEnd,
     paths: JupyterFrontEnd.IPaths,
     router: IRouter,
     resolver: IWindowResolver
-  ) => {
+  ): JupyterFrontEnd.ITreeResolver => {
     const { commands } = app;
     const treePattern = new RegExp(`^${paths.urls.tree}([^?]+)`);
     const workspacePattern = new RegExp(
       `^${paths.urls.workspaces}/[^?/]+/tree/([^?]+)`
     );
-
-    commands.addCommand(CommandIDs.tree, {
-      execute: async (args: IRouter.ILocation) => {
-        const treeMatch = args.path.match(treePattern);
-        const workspaceMatch = args.path.match(workspacePattern);
-        const match = treeMatch || workspaceMatch;
-        const path = match ? decodeURI(match[1]) : undefined;
-        const workspace = PathExt.basename(resolver.name);
-        const query = URLExt.queryStringToObject(args.search ?? '');
-        const fileBrowserPath = query['file-browser-path'];
-
-        // Remove the file browser path from the query string.
-        delete query['file-browser-path'];
-
-        // Remove the tree portion of the URL.
-        const url =
-          (workspaceMatch
-            ? URLExt.join(paths.urls.workspaces, workspace)
-            : paths.urls.app) +
-          URLExt.objectToQueryString(query) +
-          args.hash;
-
-        router.navigate(url);
-
-        try {
-          await commands.execute('filebrowser:open-path', { path });
-
-          if (fileBrowserPath) {
-            await commands.execute('filebrowser:open-path', {
-              path: fileBrowserPath
-            });
+    const set = new DisposableSet();
+    const delegate = new PromiseDelegate<JupyterFrontEnd.ITreeResolver.Paths>();
+
+    set.add(
+      commands.addCommand(CommandIDs.tree, {
+        execute: async (args: IRouter.ILocation) => {
+          if (set.isDisposed) {
+            return;
           }
-        } catch (error) {
-          console.warn('Tree routing failed.', error);
+
+          const treeMatch = args.path.match(treePattern);
+          const workspaceMatch = args.path.match(workspacePattern);
+          const match = treeMatch || workspaceMatch;
+          const file = match ? decodeURI(match[1]) : '';
+          const workspace = PathExt.basename(resolver.name);
+          const query = URLExt.queryStringToObject(args.search ?? '');
+          const browser = query['file-browser-path'] || '';
+
+          // Remove the file browser path from the query string.
+          delete query['file-browser-path'];
+
+          // Remove the tree portion of the URL.
+          const url =
+            (workspaceMatch
+              ? URLExt.join(paths.urls.workspaces, workspace)
+              : paths.urls.app) +
+            URLExt.objectToQueryString(query) +
+            args.hash;
+
+          // Route to the cleaned URL.
+          router.navigate(url);
+
+          // Clean up artifacts immediately upon routing.
+          set.dispose();
+
+          delegate.resolve({ browser, file });
         }
+      })
+    );
+    set.add(
+      router.register({ command: CommandIDs.tree, pattern: treePattern })
+    );
+    set.add(
+      router.register({ command: CommandIDs.tree, pattern: workspacePattern })
+    );
+
+    // If a route is handled by the router without the tree command being
+    // invoked, resolve to `null` and clean up artifacts.
+    const listener = () => {
+      if (set.isDisposed) {
+        return;
       }
-    });
+      set.dispose();
+      delegate.resolve(null);
+    };
+    router.routed.connect(listener);
+    set.add(
+      new DisposableDelegate(() => {
+        router.routed.disconnect(listener);
+      })
+    );
 
-    router.register({ command: CommandIDs.tree, pattern: treePattern });
-    router.register({ command: CommandIDs.tree, pattern: workspacePattern });
+    return { paths: delegate.promise };
   }
 };
 

packages/application/src/frontend.ts

@@ -305,6 +305,39 @@ export namespace JupyterFrontEnd {
       readonly workspaces: string;
     };
   }
+
+  /**
+   * The application tree resolver token.
+   *
+   * #### Notes
+   * Not all Jupyter front-end applications will have a tree resolver
+   * implemented on the client-side. This token should not be required as a
+   * dependency if it is possible to make it an optional dependency.
+   */
+  export const ITreeResolver = new Token<ITreeResolver>(
+    '@jupyterlab/application:ITreeResolver'
+  );
+
+  /**
+   * An interface for a front-end tree route resolver.
+   */
+  export interface ITreeResolver {
+    /**
+     * A promise that resolves to the routed tree paths or null.
+     */
+    readonly paths: Promise<ITreeResolver.Paths>;
+  }
+
+  /**
+   * A namespace for tree resolver types.
+   */
+  export namespace ITreeResolver {
+    /**
+     * The browser and file paths if the tree resolver encountered and handled
+     * a tree URL or `null` if not. Empty string paths should be ignored.
+     */
+    export type Paths = { browser: string; file: string } | null;
+  }
 }
 
 /**

packages/coreutils/src/text.ts

@@ -102,7 +102,7 @@ export namespace Text {
    * @returns the same string, but with each word capitalized.
    */
   export function titleCase(str: string) {
-    return str
+    return (str || '')
       .toLowerCase()
       .split(' ')
       .map(word => word.charAt(0).toUpperCase() + word.slice(1))

packages/filebrowser-extension/src/index.ts

@@ -4,6 +4,7 @@
 import {
   ILabShell,
   ILayoutRestorer,
+  IRouter,
   JupyterFrontEnd,
   JupyterFrontEndPlugin
 } from '@jupyterlab/application';
@@ -134,7 +135,8 @@ const factory: JupyterFrontEndPlugin<IFileBrowserFactory> = {
   activate: activateFactory,
   id: '@jupyterlab/filebrowser-extension:factory',
   provides: IFileBrowserFactory,
-  requires: [IIconRegistry, IDocumentManager, IStateDB]
+  requires: [IIconRegistry, IDocumentManager],
+  optional: [IStateDB, IRouter, JupyterFrontEnd.ITreeResolver]
 };
 
 /**
@@ -209,12 +211,14 @@ export default plugins;
 /**
  * Activate the file browser factory provider.
  */
-function activateFactory(
+async function activateFactory(
   app: JupyterFrontEnd,
   icoReg: IIconRegistry,
   docManager: IDocumentManager,
-  state: IStateDB
-): IFileBrowserFactory {
+  state: IStateDB | null,
+  router: IRouter | null,
+  tree: JupyterFrontEnd.ITreeResolver | null
+): Promise<IFileBrowserFactory> {
   const { commands } = app;
   const tracker = new WidgetTracker<FileBrowser>({ namespace });
   const createFileBrowser = (
@@ -226,12 +230,11 @@ function activateFactory(
       manager: docManager,
       driveName: options.driveName || '',
       refreshInterval: options.refreshInterval,
-      state: options.state === null ? undefined : options.state || state
-    });
-    const widget = new FileBrowser({
-      id,
-      model
+      state:
+        options.state === null ? undefined : options.state || state || undefined
     });
+    const restore = options.restore;
+    const widget = new FileBrowser({ id, model, restore });
 
     // Add a launcher toolbar item.
     let launcher = new ToolbarButton({
@@ -248,9 +251,43 @@ function activateFactory(
 
     return widget;
   };
-  const defaultBrowser = createFileBrowser('filebrowser');
 
-  return { createFileBrowser, defaultBrowser, tracker };
+  // Manually restore the default file browser.
+  const id = 'filebrowser';
+  const defaultBrowser = createFileBrowser(id, { restore: false });
+  const plugin = { createFileBrowser, defaultBrowser, tracker };
+  const restoring = 'jp-mod-restoring';
+
+  defaultBrowser.addClass(restoring);
+
+  if (!router) {
+    void defaultBrowser.model.restore(id).then(() => {
+      defaultBrowser.removeClass(restoring);
+    });
+    return plugin;
+  }
+
+  const listener = async () => {
+    router.routed.disconnect(listener);
+
+    const paths = await tree?.paths;
+    if (paths) {
+      // Restore the model without populating it.
+      await defaultBrowser.model.restore(id, false);
+      if (paths.file) {
+        await commands.execute(CommandIDs.openPath, { path: paths.file });
+      }
+      if (paths.browser) {
+        await commands.execute(CommandIDs.openPath, { path: paths.browser });
+      }
+    } else {
+      await defaultBrowser.model.restore(id);
+    }
+    defaultBrowser.removeClass(restoring);
+  };
+  router.routed.connect(listener);
+
+  return plugin;
 }
 
 /**
@@ -263,8 +300,8 @@ function activateBrowser(
   labShell: ILabShell,
   restorer: ILayoutRestorer,
   settingRegistry: ISettingRegistry,
-  commandPalette: ICommandPalette,
-  mainMenu: IMainMenu
+  commandPalette: ICommandPalette | null,
+  mainMenu: IMainMenu | null
 ): void {
   const browser = factory.defaultBrowser;
   const { commands } = app;

packages/filebrowser-extension/style/index.css

@@ -4,3 +4,7 @@
 |----------------------------------------------------------------------------*/
 
 @import url('~@jupyterlab/filebrowser/style/index.css');
+
+#filebrowser.jp-mod-restoring .jp-DirListing-content {
+  display: none;
+}

packages/filebrowser/src/browser.ts

@@ -64,19 +64,17 @@ export class FileBrowser extends Widget {
     this._manager = model.manager;
     this._crumbs = new BreadCrumbs({ model });
     this.toolbar = new Toolbar<Widget>();
-
     this._directoryPending = false;
-    let newFolder = new ToolbarButton({
+
+    const newFolder = new ToolbarButton({
       iconClassName: 'jp-NewFolderIcon',
       onClick: () => {
         this.createNewDirectory();
       },
       tooltip: 'New Folder'
     });
-
-    let uploader = new Uploader({ model });
-
-    let refresher = new ToolbarButton({
+    const uploader = new Uploader({ model });
+    const refresher = new ToolbarButton({
       iconClassName: 'jp-RefreshIcon',
       onClick: () => {
         void model.refresh();
@@ -94,13 +92,16 @@ export class FileBrowser extends Widget {
     this.toolbar.addClass(TOOLBAR_CLASS);
     this._listing.addClass(LISTING_CLASS);
 
-    let layout = new PanelLayout();
+    const layout = new PanelLayout();
+
     layout.addWidget(this.toolbar);
     layout.addWidget(this._crumbs);
     layout.addWidget(this._listing);
-
     this.layout = layout;
-    void model.restore(this.id);
+
+    if (options.restore !== false) {
+      void model.restore(this.id);
+    }
   }
 
   /**
@@ -309,5 +310,12 @@ export namespace FileBrowser {
      * The default is a shared instance of `DirListing.Renderer`.
      */
     renderer?: DirListing.IRenderer;
+
+    /**
+     * Whether a file browser automatically restores state when instantiated.
+     *
+     * The default is `true`.
+     */
+    restore?: boolean;
   }
 }

packages/filebrowser/src/model.ts

@@ -338,41 +338,53 @@ export class FileBrowserModel implements IDisposable {
    *
    * @param id - The unique ID that is used to construct a state database key.
    *
+   * @param populate - If `false`, the restoration ID will be set but the file
+   * browser state will not be fetched from the state database.
+   *
    * @returns A promise when restoration is complete.
    *
    * #### Notes
    * This function will only restore the model *once*. If it is called multiple
    * times, all subsequent invocations are no-ops.
    */
-  restore(id: string): Promise<void> {
+  async restore(id: string, populate = true): Promise<void> {
+    const { manager } = this;
+    const key = `file-browser-${id}:cwd`;
     const state = this._state;
     const restored = !!this._key;
-    if (!state || restored) {
-      return Promise.resolve(void 0);
+
+    if (restored) {
+      return;
     }
 
-    const manager = this.manager;
-    const key = `file-browser-${id}:cwd`;
-    const ready = manager.services.ready;
-    return Promise.all([state.fetch(key), ready])
-      .then(([value]) => {
-        if (!value) {
-          this._restored.resolve(undefined);
-          return;
-        }
+    // Set the file browser key for state database fetch/save.
+    this._key = key;
 
-        const path = (value as ReadonlyJSONObject)['path'] as string;
-        const localPath = manager.services.contents.localPath(path);
-        return manager.services.contents
-          .get(path)
-          .then(() => this.cd(localPath))
-          .catch(() => state.remove(key));
-      })
-      .catch(() => state.remove(key))
-      .then(() => {
-        this._key = key;
+    if (!populate || !state) {
+      this._restored.resolve(undefined);
+      return;
+    }
+
+    await manager.services.ready;
+
+    try {
+      const value = await state.fetch(key);
+
+      if (!value) {
         this._restored.resolve(undefined);
-      }); // Set key after restoration is done.
+        return;
+      }
+
+      const path = (value as ReadonlyJSONObject)['path'] as string;
+      const localPath = manager.services.contents.localPath(path);
+
+      await manager.services.contents.get(path);
+      await this.cd(localPath);
+    } catch (error) {
+      await state.remove(key);
+    }
+
+    this._restored.resolve(undefined);
   }
 
   /**

packages/filebrowser/src/tokens.ts

@@ -78,6 +78,19 @@ export namespace IFileBrowserFactory {
      */
     driveName?: string;
 
+    /**
+     * The time interval for browser refreshing, in ms.
+     */
+    refreshInterval?: number;
+
+    /**
+     * Whether to restore the file browser state after instantiation.
+     *
+     * #### Notes
+     * The default value is `true`.
+     */
+    restore?: boolean;
+
     /**
      * The state database to use for saving file browser state and restoring it.
      *
@@ -86,10 +99,5 @@ export namespace IFileBrowserFactory {
      * database will be automatically passed in and used for state restoration.
      */
     state?: IStateDB | null;
-
-    /**
-     * The time interval for browser refreshing, in ms.
-     */
-    refreshInterval?: number;
   }
 }