Fix saving after renames (#511)

* Add a test

Improve tests

* Fix saving after rename

* Add a warning

* Handle race condition if rename and save happens quickly

by having a second listener on the local `fileChaned` signal

* Fix typo in a test comment

Co-authored-by: David Brochart <[email protected]>

---------

Co-authored-by: David Brochart <[email protected]>

Author: krassowski

packages/docprovider-extension/src/filebrowser.ts

@@ -86,7 +86,8 @@ export const rtcContentProvider: JupyterFrontEndPlugin<ICollaborativeContentProv
         user: app.serviceManager.user,
         trans,
         globalAwareness,
-        docmanagerSettings
+        docmanagerSettings,
+        fileChanged: defaultDrive.fileChanged
       });
       registry.register('rtc', rtcContentProvider);
       return rtcContentProvider;

packages/docprovider/src/ydrive.ts

@@ -14,7 +14,12 @@ import {
 import { PromiseDelegate } from '@lumino/coreutils';
 import { ISignal, Signal } from '@lumino/signaling';
 
-import { DocumentChange, ISharedDocument, YDocument } from '@jupyter/ydoc';
+import {
+  DocumentChange,
+  ISharedDocument,
+  StateChange,
+  YDocument
+} from '@jupyter/ydoc';
 
 import { WebSocketProvider } from './yprovider';
 import {
@@ -49,6 +54,7 @@ namespace RtcContentProvider {
     trans: TranslationBundle;
     globalAwareness: Awareness | null;
     docmanagerSettings: ISettingRegistry.ISettings | null;
+    fileChanged?: ISignal<Contents.IDrive, Contents.IChangedArgs>;
   }
 }
 
@@ -65,6 +71,7 @@ export class RtcContentProvider
     this.sharedModelFactory = new SharedModelFactory(this._onCreate);
     this._providers = new Map<string, WebSocketProvider>();
     this._docmanagerSettings = options.docmanagerSettings;
+    this._driveFileChanged = options.fileChanged;
   }
 
   /**
@@ -189,6 +196,10 @@ export class RtcContentProvider
           content: false
         };
         return this.get(localPath, fetchOptions);
+      } else {
+        console.warn(
+          `Could not find a provider for ${localPath}, falling back to REST API save`
+        );
       }
     }
 
@@ -199,7 +210,7 @@ export class RtcContentProvider
    * A signal emitted when a file operation takes place.
    */
   get fileChanged(): ISignal<this, Contents.IChangedArgs> {
-    return this._ydriveFileChanged;
+    return this._providerFileChanged;
   }
 
   private _onCreate = (
@@ -241,13 +252,99 @@ export class RtcContentProvider
         this._globalAwareness?.setLocalStateField('documents', documents);
       }
 
-      const key = `${options.format}:${options.contentType}:${options.path}`;
+      let path = options.path;
+      let key = `${options.format}:${options.contentType}:${path}`;
       this._providers.set(key, provider);
 
+      const handlePathChange = (
+        pathChange: StateChange<string | undefined>
+      ) => {
+        const oldPath = pathChange.oldValue;
+        const newPath = pathChange.newValue;
+        if (!oldPath || !newPath) {
+          // This is expected when shared model initializes and the path is first populated
+          console.debug('New or old path not given', pathChange);
+          return;
+        }
+
+        const oldKey = `${options.format}:${options.contentType}:${oldPath}`;
+        if (oldKey !== key) {
+          console.error(
+            'The computed old provider key is different from the current key'
+          );
+          return;
+        }
+        const newKey = `${options.format}:${options.contentType}:${newPath}`;
+
+        // Check if the provider is still registered (it may have been disposed if document was closed)
+        const provider = this._providers.get(oldKey);
+        if (!provider) {
+          console.warn(
+            `Could not find a provider to update after rename ${oldKey}, ${newKey}`
+          );
+          return;
+        }
+
+        // Re-register the provider under the new key
+        this._providers.set(newKey, provider);
+        this._providers.delete(oldKey);
+
+        // Update the provider key so that it can be disposed correctly when shared document closes
+        key = newKey;
+        path = newPath;
+
+        // Update the documents field
+        const state = this._globalAwareness?.getLocalState() || {};
+        const documents: string[] = state.documents || [];
+        const oldPathIndex = documents.indexOf(oldPath);
+        if (documents.includes(oldPath) && !documents.includes(newPath)) {
+          documents.splice(oldPathIndex, 1);
+          documents.push(newPath);
+          this._globalAwareness?.setLocalStateField('documents', documents);
+        }
+      };
+
+      // The information about file being renamed can come from two places:
+      // - from the sharedModel via changed signal with documentChange
+      // - from the fileChanged signal of the drive
+      // Neither method is foolproof:
+      // - the shared model approach can be delayed as the change needs to be
+      //   reflected by the server and come back, in which case we get a race condition
+      // - the fileChanged signal is emitted with a larger delay for renames of collaborators
+      // Thus we need both.
+      const handleFileChangedSignal = (
+        _: Contents.IDrive,
+        change: Contents.IChangedArgs
+      ) => {
+        if (change.type !== 'rename') {
+          return;
+        }
+        const oldPath = change.oldValue?.path;
+        const newPath = change.newValue?.path;
+        if (oldPath !== path) {
+          return;
+        }
+        handlePathChange({
+          oldValue: oldPath,
+          newValue: newPath,
+          name: 'path'
+        });
+      };
+
+      this._driveFileChanged?.connect(handleFileChangedSignal);
+
       sharedModel.changed.connect(async (_, change) => {
         if (!change.stateChange) {
           return;
         }
+
+        const pathChanges = change.stateChange.filter(
+          change => change.name === 'path'
+        );
+        for (const pathChange of pathChanges) {
+          handlePathChange(pathChange);
+        }
+
         const hashChanges = change.stateChange.filter(
           change => change.name === 'hash'
         );
@@ -267,7 +364,7 @@ export class RtcContentProvider
         const newPath = sharedModel.state.path ?? options.path;
         const model = await this.get(newPath as string, { content: false });
 
-        this._ydriveFileChanged.emit({
+        this._providerFileChanged.emit({
           type: 'save',
           newValue: { ...model, hash: hashChange.newValue },
           // we do not have the old model because it was discarded when server made the change,
@@ -285,12 +382,15 @@ export class RtcContentProvider
 
         // Remove the document path from the list of opened ones for this user.
         const state = this._globalAwareness?.getLocalState() || {};
-        const documents: any[] = state.documents || [];
-        const index = documents.indexOf(options.path);
+        const documents: string[] = state.documents || [];
+        const index = documents.indexOf(path);
         if (index > -1) {
           documents.splice(index, 1);
         }
         this._globalAwareness?.setLocalStateField('documents', documents);
+
+        // Disconnect signal
+        this._driveFileChanged?.disconnect(handleFileChangedSignal);
       });
     } catch (error) {
       // Falling back to the contents API if opening the websocket failed
@@ -306,7 +406,10 @@ export class RtcContentProvider
   private _trans: TranslationBundle;
   private _globalAwareness: Awareness | null;
   private _providers: Map<string, WebSocketProvider>;
-  private _ydriveFileChanged = new Signal<this, Contents.IChangedArgs>(this);
+  // This is for emitting signals to be proxied to `Drive.fileChanged`
+  private _providerFileChanged = new Signal<this, Contents.IChangedArgs>(this);
+  // This is for listening to `Drive.fileChanged` signal
+  private _driveFileChanged?: ISignal<Contents.IDrive, Contents.IChangedArgs>;
   private _serverSettings: ServerConnection.ISettings;
   private _docmanagerSettings: ISettingRegistry.ISettings | null;
 }

ui-tests/tests/notebook.spec.ts

@@ -146,6 +146,52 @@ test.describe('Initialization', () => {
     await page.notebook.close(true);
     await guestPage.notebook.close(true);
   });
+
+  test('Rename a notebook', async({ page, tmpPath }) => {
+    await page.filebrowser.refresh();
+    await page.notebook.open(exampleNotebook);
+    await page.notebook.activate(exampleNotebook);
+
+    // Enter edit mode on cell number three
+    await page.notebook.enterCellEditingMode(3);
+    expect(await page.notebook.isCellInEditingMode(3)).toBe(true);
+
+    const oldPath = `${tmpPath}/${exampleNotebook}`;
+    const newPath = `${tmpPath}/differentName.ipynb`;
+
+    // Rename notebook
+    await page.contents.renameFile(oldPath, newPath);
+
+    // Intercept REST API save endpoint
+    let savedUsingRestAPI = false;
+    const saveRoutePattern = `**/api/contents/${newPath}**`;
+    await page.route(saveRoutePattern, route => {
+      if (route.request().method() === 'PUT') {
+        savedUsingRestAPI = true;
+      }
+      route.continue();
+    });
+
+    // Save the notebook manually
+    await page.notebook.save();
+
+    // Wait three seconds to make sure the cell does not jump
+    await page.waitForTimeout(3000);
+
+    // The save operation should not trigger a PUT to the renamed path,
+    // instead the save request should have gone through the websocket.
+    expect.soft(savedUsingRestAPI).toBe(false);
+
+    // This tests against a out-of-band reloads that can cause
+    // change of active cell and of the notebook edit/command mode
+    expect(await page.notebook.isCellInEditingMode(3)).toBe(true);
+
+    // Cleanup network route
+    await page.unroute(saveRoutePattern);
+
+    // Rename it back for cleanup
+    await page.contents.renameFile(newPath, oldPath);
+  });
 });
 
 test.describe('Ten clients', () => {