Fix windowed notebook and ToC getting broken on reloading from disk

packages/notebook/src/toc.ts

@@ -295,6 +295,23 @@ export class NotebookToCModel extends TableOfContentsModel<
     return Promise.resolve(headings);
   }
 
+  /**
+   * Test if two headings are equal or not.
+   *
+   * @param heading1 First heading
+   * @param heading2 Second heading
+   * @returns Whether the headings are equal.
+   */
+  protected override isHeadingEqual(
+    heading1: INotebookHeading,
+    heading2: INotebookHeading
+  ): boolean {
+    return (
+      super.isHeadingEqual(heading1, heading2) &&
+      heading1.cellRef === heading2.cellRef
+    );
+  }
+
   /**
    * Read table of content configuration from notebook metadata.
    *

packages/notebook/src/windowing.ts

@@ -261,7 +261,7 @@ export class NotebookWindowedLayout extends WindowedLayout {
 
     // Note: `index` is relative to the displayed cells, not all cells,
     // hence we compare with the widget itself.
-    if (widget === this.activeCell) {
+    if (widget === this.activeCell && widget !== this._willBeRemoved) {
       // Do not change display of the active cell to allow user to continue providing input
       // into the code mirror editor when out of view. We still hide the cell so to prevent
       // minor visual glitches when scrolling.

packages/notebook/test/toc.spec.ts

@@ -0,0 +1,79 @@
+// Copyright (c) Jupyter Development Team.
+// Distributed under the terms of the Modified BSD License.
+
+import {
+  INotebookModel,
+  NotebookPanel,
+  NotebookToCModel
+} from '@jupyterlab/notebook';
+import { Context } from '@jupyterlab/docregistry';
+import { NBTestUtils } from '@jupyterlab/notebook/lib/testutils';
+import { Sanitizer } from '@jupyterlab/apputils';
+import { signalToPromise } from '@jupyterlab/testing';
+import * as utils from './utils';
+
+describe('@jupyterlab/notebook', () => {
+  describe('NotebookToCModel', () => {
+    let context: Context<INotebookModel>;
+    let panel: NotebookPanel;
+    let model: NotebookToCModel;
+    const sanitizer = new Sanitizer();
+    const initialCells = [{ cell_type: 'markdown', source: '# heading' }];
+
+    beforeEach(async () => {
+      context = await NBTestUtils.createMockContext(false);
+      panel = utils.createNotebookPanel(context);
+      model = new NotebookToCModel(panel, null, sanitizer);
+      panel.model!.sharedModel.insertCells(0, initialCells);
+    });
+
+    afterEach(() => {
+      context.dispose();
+    });
+
+    describe('#headingsChanged', () => {
+      it('should be emitted on cell insertion/deletion', async () => {
+        // Should be called on insertion
+        panel.model!.sharedModel.insertCells(0, [
+          { cell_type: 'markdown', source: '# heading 2' }
+        ]);
+        let promise = signalToPromise(model.headingsChanged);
+        await model.refresh();
+        await promise;
+        expect(model.headings).toHaveLength(2);
+
+        // Should be called on deletion
+        panel.model!.sharedModel.deleteCell(0);
+        promise = signalToPromise(model.headingsChanged);
+        await model.refresh();
+        await promise;
+        expect(model.headings).toHaveLength(1);
+      });
+
+      it('should be emitted when reloading notebook model', async () => {
+        // Note: if in future the `NotebookPanel` gets reworked to retain
+        // widgets for cells which did not change (rather than re-created
+        // them each time), this test may be no longer needed.
+
+        // Setup the notebook model
+        const content = {
+          ...utils.DEFAULT_CONTENT,
+          cells: initialCells
+        };
+        let promise = signalToPromise(model.headingsChanged);
+        panel.model!.fromJSON(content);
+        await model.refresh();
+        await promise;
+
+        // Simulate update via "Revert" button, which will rebuilding the notebook
+        // widget from scratch (delete old cells and add new cells).
+        panel.model!.fromJSON(content);
+
+        // Should emit because cell references would have changed.
+        promise = signalToPromise(model.headingsChanged);
+        await model.refresh();
+        await promise;
+      });
+    });
+  });
+});

packages/toc/src/model.ts

@@ -177,10 +177,7 @@ export abstract class TableOfContentsModel<
         return this.refresh();
       }
 
-      if (
-        newHeadings &&
-        !Private.areHeadingsEqual(newHeadings, this._headings)
-      ) {
+      if (newHeadings && !this._areHeadingsEqual(newHeadings, this._headings)) {
         this._headings = newHeadings;
         this.stateChanged.emit();
         this._headingsChanged.emit();
@@ -245,40 +242,32 @@ export abstract class TableOfContentsModel<
     }
   }
 
-  private _activeHeading: H | null;
-  private _activeHeadingChanged: Signal<TableOfContentsModel<H, T>, H | null>;
-  private _collapseChanged: Signal<TableOfContentsModel<H, T>, H | null>;
-  private _configuration: TableOfContents.IConfig;
-  private _headings: H[];
-  private _headingsChanged: Signal<TableOfContentsModel<H, T>, void>;
-  private _isActive: boolean;
-  private _isRefreshing: boolean;
-  private _needsRefreshing: boolean;
-  private _title?: string;
-}
+  /**
+   * Test if two headings are equal or not.
+   *
+   * @param heading1 First heading
+   * @param heading2 Second heading
+   * @returns Whether the headings are equal.
+   */
+  protected isHeadingEqual(heading1: H, heading2: H): boolean {
+    return (
+      heading1.level === heading2.level &&
+      heading1.text === heading2.text &&
+      heading1.prefix === heading2.prefix
+    );
+  }
 
-/**
- * Private functions namespace
- */
-namespace Private {
   /**
    * Test if two list of headings are equal or not.
    *
    * @param headings1 First list of headings
    * @param headings2 Second list of headings
-   * @returns Whether the array are identical or not.
+   * @returns Whether the array are equal.
    */
-  export function areHeadingsEqual(
-    headings1: TableOfContents.IHeading[],
-    headings2: TableOfContents.IHeading[]
-  ): boolean {
+  private _areHeadingsEqual(headings1: H[], headings2: H[]): boolean {
     if (headings1.length === headings2.length) {
       for (let i = 0; i < headings1.length; i++) {
-        if (
-          headings1[i].level !== headings2[i].level ||
-          headings1[i].text !== headings2[i].text ||
-          headings1[i].prefix !== headings2[i].prefix
-        ) {
+        if (!this.isHeadingEqual(headings1[i], headings2[i])) {
           return false;
         }
       }
@@ -287,4 +276,15 @@ namespace Private {
 
     return false;
   }
+
+  private _activeHeading: H | null;
+  private _activeHeadingChanged: Signal<TableOfContentsModel<H, T>, H | null>;
+  private _collapseChanged: Signal<TableOfContentsModel<H, T>, H | null>;
+  private _configuration: TableOfContents.IConfig;
+  private _headings: H[];
+  private _headingsChanged: Signal<TableOfContentsModel<H, T>, void>;
+  private _isActive: boolean;
+  private _isRefreshing: boolean;
+  private _needsRefreshing: boolean;
+  private _title?: string;
 }