Merge pull request #355 from mathuo/341-dockview-stuck-in-broken-state-after-apifromjson-fails-due-to-invalid-component-name

feat: fromJSON error handling and cleanup

packages/dockview-core/src/__tests__/dockview/dockviewComponent.spec.ts

@@ -4064,13 +4064,14 @@ describe('dockviewComponent', () => {
         expect(groupDragEvents.length).toBe(1);
     });
 
-    test('that loading a corrupt layout throws an error and leaves a clean dockview behind', () => {
+    test('corrupt layout: bad inline view', () => {
         const container = document.createElement('div');
 
         const dockview = new DockviewComponent({
             parentElement: container,
             components: {
-                default: PanelContentPartTest,
+                panelA: PanelContentPartTest,
+                panelB: PanelContentPartTest,
             },
             tabComponents: {
                 test_tab_id: PanelTabPartTest,
@@ -4080,14 +4081,22 @@ describe('dockviewComponent', () => {
 
         dockview.layout(1000, 500);
 
+        let el = dockview.element.querySelector('.view-container');
+        expect(el).toBeTruthy();
+        expect(el!.childNodes.length).toBe(0);
+
         dockview.addPanel({
             id: 'panel_1',
-            component: 'default',
+            component: 'panelA',
         });
 
         expect(dockview.groups.length).toBe(1);
         expect(dockview.panels.length).toBe(1);
 
+        el = dockview.element.querySelector('.view-container');
+        expect(el).toBeTruthy();
+        expect(el!.childNodes.length).toBeGreaterThan(0);
+
         expect(() => {
             dockview.fromJSON({
                 grid: {
@@ -4122,7 +4131,124 @@ describe('dockviewComponent', () => {
                 panels: {
                     panelA: {
                         id: 'panelA',
+                        contentComponent: 'panelA',
+                        title: 'Panel A',
+                    },
+                    panelB: {
+                        id: 'panelB',
                         contentComponent: 'somethingBad',
+                        title: 'Panel B',
+                    },
+                },
+                activeGroup: '1',
+            });
+        }).toThrow(
+            "Cannot create 'panelB', no component 'somethingBad' provided"
+        );
+
+        expect(dockview.groups.length).toBe(0);
+        expect(dockview.panels.length).toBe(0);
+
+        el = dockview.element.querySelector('.view-container');
+        expect(el).toBeTruthy();
+        expect(el!.childNodes.length).toBe(0);
+    });
+
+    test('corrupt layout: bad floating view', () => {
+        const container = document.createElement('div');
+
+        const dockview = new DockviewComponent({
+            parentElement: container,
+            components: {
+                panelA: PanelContentPartTest,
+                panelB: PanelContentPartTest,
+            },
+            tabComponents: {
+                test_tab_id: PanelTabPartTest,
+            },
+            orientation: Orientation.HORIZONTAL,
+        });
+
+        dockview.layout(1000, 500);
+
+        let el = dockview.element.querySelector('.view-container');
+        expect(el).toBeTruthy();
+        expect(el!.childNodes.length).toBe(0);
+
+        dockview.addPanel({
+            id: 'panel_1',
+            component: 'panelA',
+        });
+
+        dockview.addPanel({
+            id: 'panel_2',
+            component: 'panelA',
+            floating: true,
+        });
+
+        expect(dockview.groups.length).toBe(2);
+        expect(dockview.panels.length).toBe(2);
+
+        el = dockview.element.querySelector('.dv-resize-container');
+        expect(el).toBeTruthy();
+
+        el = dockview.element.querySelector('.view-container');
+        expect(el).toBeTruthy();
+        expect(el!.childNodes.length).toBeGreaterThan(0);
+
+        expect(() => {
+            dockview.fromJSON({
+                grid: {
+                    root: {
+                        type: 'branch',
+                        data: [
+                            {
+                                type: 'leaf',
+                                data: {
+                                    views: ['panelA'],
+                                    activeView: 'panelA',
+                                    id: '1',
+                                },
+                                size: 841,
+                            },
+                            {
+                                type: 'leaf',
+                                data: {
+                                    views: ['panelB'],
+                                    activeView: 'panelB',
+                                    id: '2',
+                                },
+                                size: 842,
+                            },
+                        ],
+                        size: 530,
+                    },
+                    width: 1683,
+                    height: 530,
+                    orientation: Orientation.HORIZONTAL,
+                },
+                floatingGroups: [
+                    {
+                        data: {
+                            views: ['panelB'],
+                            activeView: 'panelB',
+                            id: '3',
+                        },
+                        position: { left: 0, top: 0, height: 100, width: 100 },
+                    },
+                    {
+                        data: {
+                            views: ['panelC'],
+                            activeView: 'panelC',
+                            id: '4',
+                        },
+                        position: { left: 0, top: 0, height: 100, width: 100 },
+                    },
+                ],
+                panels: {
+                    panelA: {
+                        id: 'panelA',
+                        contentComponent: 'panelA',
                         title: 'Panel A',
                     },
                     panelB: {
@@ -4130,14 +4256,24 @@ describe('dockviewComponent', () => {
                         contentComponent: 'panelB',
                         title: 'Panel B',
                     },
+                    panelC: {
+                        id: 'panelC',
+                        contentComponent: 'panelC',
+                        title: 'Panel C',
+                    },
                 },
                 activeGroup: '1',
             });
-        }).toThrow(
+        }).toThrow("Cannot create 'panelC', no component 'panelC' provided");
-            "Cannot create 'panelA', no component 'somethingBad' provided"
-        );
 
         expect(dockview.groups.length).toBe(0);
         expect(dockview.panels.length).toBe(0);
+
+        el = dockview.element.querySelector('.dv-resize-container');
+        expect(el).toBeFalsy();
+
+        el = dockview.element.querySelector('.view-container');
+        expect(el).toBeTruthy();
+        expect(el!.childNodes.length).toBe(0);
     });
 });

packages/dockview-core/src/__tests__/gridview/gridviewComponent.spec.ts

@@ -2597,4 +2597,84 @@ describe('gridview', () => {
             activePanel: 'panel_1',
         });
     });
+
+    test('that loading a corrupt layout throws an error and leaves a clean gridview behind', () => {
+        const gridview = new GridviewComponent({
+            parentElement: container,
+            proportionalLayout: true,
+            orientation: Orientation.HORIZONTAL,
+            components: { default: TestGridview },
+        });
+
+        let el = gridview.element.querySelector('.view-container');
+        expect(el).toBeTruthy();
+        expect(el!.childNodes.length).toBe(0);
+
+        expect(() => {
+            gridview.fromJSON({
+                grid: {
+                    height: 400,
+                    width: 800,
+                    orientation: Orientation.HORIZONTAL,
+                    root: {
+                        type: 'branch',
+                        size: 400,
+                        data: [
+                            {
+                                type: 'leaf',
+                                size: 200,
+                                data: {
+                                    id: 'panel_1',
+                                    component: 'default',
+                                    snap: false,
+                                },
+                            },
+                            {
+                                type: 'branch',
+                                size: 400,
+                                data: [
+                                    {
+                                        type: 'leaf',
+                                        size: 250,
+                                        data: {
+                                            id: 'panel_1',
+                                            component: 'default',
+                                            snap: false,
+                                        },
+                                    },
+                                    {
+                                        type: 'leaf',
+                                        size: 150,
+                                        data: {
+                                            id: 'panel_1',
+                                            component: 'somethingBad',
+                                            snap: false,
+                                        },
+                                    },
+                                ],
+                            },
+                            {
+                                type: 'leaf',
+                                size: 200,
+                                data: {
+                                    id: 'panel_1',
+                                    component: 'default',
+                                    snap: false,
+                                },
+                            },
+                        ],
+                    },
+                },
+                activePanel: 'panel_1',
+            });
+        }).toThrow(
+            "Cannot create 'panel_1', no component 'somethingBad' provided"
+        );
+
+        expect(gridview.groups.length).toBe(0);
+
+        el = gridview.element.querySelector('.view-container');
+        expect(el).toBeTruthy();
+        expect(el!.childNodes.length).toBe(0);
+    });
 });

packages/dockview-core/src/dockview/dockviewComponent.ts

@@ -55,6 +55,7 @@ import {
     GroupDragEvent,
     TabDragEvent,
 } from './components/titlebar/tabsContainer';
+import { DockviewGroupPanelApi } from '../api/dockviewGroupPanelApi';
 
 const DEFAULT_FLOATING_GROUP_OVERFLOW_SIZE = 100;
 
@@ -80,6 +81,92 @@ export interface SerializedDockview {
     floatingGroups?: SerializedFloatingGroup[];
 }
 
+function typeValidate3(data: GroupPanelViewState, path: string): void {
+    if (typeof data.id !== 'string') {
+        throw new Error(`${path}.id must be a string`);
+    }
+
+    if (
+        typeof data.activeView !== 'string' ||
+        typeof data.activeView !== 'undefined'
+    ) {
+        throw new Error(`${path}.activeView must be a string of undefined`);
+    }
+}
+
+function typeValidate2(
+    data: SerializedGridObject<GroupPanelViewState>,
+    path: string
+): void {
+    if (typeof data.size !== 'number' && typeof data.size !== 'undefined') {
+        throw new Error(`${path}.size must be a number or undefined`);
+    }
+
+    if (
+        typeof data.visible !== 'boolean' &&
+        typeof data.visible !== 'undefined'
+    ) {
+        throw new Error(`${path}.visible must be a boolean or undefined`);
+    }
+
+    if (data.type === 'leaf') {
+        if (
+            typeof data.data !== 'object' ||
+            data.data === null ||
+            Array.isArray(data.data)
+        ) {
+            throw new Error('object must be a non-null object');
+        }
+
+        typeValidate3(data.data, `${path}.data`);
+    } else if (data.type === 'branch') {
+        if (!Array.isArray(data.data)) {
+            throw new Error(`${path}.data must be an array`);
+        }
+    } else {
+        throw new Error(`${path}.type must be onew of {'branch', 'leaf'}`);
+    }
+}
+
+function typeValidate(data: SerializedDockview): void {
+    if (typeof data !== 'object' || data === null) {
+        throw new Error('object must be a non-null object');
+    }
+
+    const { grid, panels, activeGroup, floatingGroups } = data;
+
+    if (typeof grid !== 'object' || grid === null) {
+        throw new Error("'.grid' must be a non-null object");
+    }
+
+    if (typeof grid.height !== 'number') {
+        throw new Error("'.grid.height' must be a number");
+    }
+
+    if (typeof grid.width !== 'number') {
+        throw new Error("'.grid.width' must be a number");
+    }
+
+    if (typeof grid.root !== 'object' || grid.root === null) {
+        throw new Error("'.grid.root' must be a non-null object");
+    }
+
+    if (grid.root.type !== 'branch') {
+        throw new Error(".grid.root.type must be of type 'branch'");
+    }
+
+    if (
+        grid.orientation !== Orientation.HORIZONTAL &&
+        grid.orientation !== Orientation.VERTICAL
+    ) {
+        throw new Error(
+            `'.grid.width' must be one of {${Orientation.HORIZONTAL}, ${Orientation.VERTICAL}}`
+        );
+    }
+
+    typeValidate2(grid.root, '.grid.root');
+}
+
 export type DockviewComponentUpdateOptions = Pick<
     DockviewComponentOptions,
     | 'orientation'
@@ -637,39 +724,55 @@ export class DockviewComponent
     fromJSON(data: SerializedDockview): void {
         this.clear();
 
+        if (typeof data !== 'object' || data === null) {
+            throw new Error('serialized layout must be a non-null object');
+        }
+
         const { grid, panels, activeGroup } = data;
 
         if (grid.root.type !== 'branch' || !Array.isArray(grid.root.data)) {
             throw new Error('root must be of type branch');
         }
 
-        // take note of the existing dimensions
+        try {
-        const width = this.width;
+            // take note of the existing dimensions
-        const height = this.height;
+            const width = this.width;
+            const height = this.height;
 
-        const createGroupFromSerializedState = (data: GroupPanelViewState) => {
+            const createGroupFromSerializedState = (
-            const { id, locked, hideHeader, views, activeView } = data;
+                data: GroupPanelViewState
+            ) => {
+                const { id, locked, hideHeader, views, activeView } = data;
 
-            if (typeof id !== 'string') {
+                if (typeof id !== 'string') {
-                throw new Error('group id must be of type string');
+                    throw new Error('group id must be of type string');
-            }
+                }
 
-            let group: DockviewGroupPanel | undefined;
+                const group = this.createGroup({
-
-            try {
-                group = this.createGroup({
                     id,
                     locked: !!locked,
                     hideHeader: !!hideHeader,
                 });
 
-                this._onDidAddGroup.fire(group);
+                const createdPanels: IDockviewPanel[] = [];
 
                 for (const child of views) {
+                    /**
+                     * Run the deserializer step seperately since this may fail to due corrupted external state.
+                     * In running this section first we avoid firing lots of 'add' events in the event of a failure
+                     * due to a corruption of input data.
+                     */
                     const panel = this._deserializer.fromJSON(
                         panels[child],
                         group
                     );
+                    createdPanels.push(panel);
+                }
+
+                this._onDidAddGroup.fire(group);
+
+                for (let i = 0; i < views.length; i++) {
+                    const panel = createdPanels[i];
 
                     const isActive =
                         typeof activeView === 'string' &&
@@ -691,61 +794,82 @@ export class DockviewComponent
                 }
 
                 return group;
-            } catch (err) {
+            };
-                /**
-                 * This is an odd case... we have failed to deserialize a view but we have already created a group,
-                 * but we havn't registered that group with the gridview.
-                 * We cannot use the removeGroup method because the group has only been partially added, we must
-                 * manually dipose() of the view and remove it from being stored in the map.
-                 */
-                if (group) {
-                    group.dispose();
-                    this._groups.delete(group.id);
-                }
 
-                /**
+            this.gridview.deserialize(grid, {
-                 * re-throw the error becasue we don't actually want to catch it, we just
+                fromJSON: (node: ISerializedLeafNode<GroupPanelViewState>) => {
-                 * needed to do some clean-up before continuing.
+                    return createGroupFromSerializedState(node.data);
-                 */
-                throw err;
-            }
-        };
-
-        this.gridview.deserialize(grid, {
-            fromJSON: (node: ISerializedLeafNode<GroupPanelViewState>) => {
-                return createGroupFromSerializedState(node.data);
-            },
-        });
-
-        this.layout(width, height, true);
-
-        const serializedFloatingGroups = data.floatingGroups ?? [];
-
-        for (const serializedFloatingGroup of serializedFloatingGroups) {
-            const { data, position } = serializedFloatingGroup;
-            const group = createGroupFromSerializedState(data);
-
-            this.addFloatingGroup(
-                group,
-                {
-                    x: position.left,
-                    y: position.top,
-                    height: position.height,
-                    width: position.width,
                 },
-                { skipRemoveGroup: true, inDragMode: false }
+            });
-            );
-        }
 
-        for (const floatingGroup of this.floatingGroups) {
+            this.layout(width, height, true);
-            floatingGroup.overlay.setBounds();
-        }
 
-        if (typeof activeGroup === 'string') {
+            const serializedFloatingGroups = data.floatingGroups ?? [];
-            const panel = this.getPanel(activeGroup);
+
-            if (panel) {
+            for (const serializedFloatingGroup of serializedFloatingGroups) {
-                this.doSetGroupActive(panel);
+                const { data, position } = serializedFloatingGroup;
+
+                const group = createGroupFromSerializedState(data);
+
+                this.addFloatingGroup(
+                    group,
+                    {
+                        x: position.left,
+                        y: position.top,
+                        height: position.height,
+                        width: position.width,
+                    },
+                    { skipRemoveGroup: true, inDragMode: false }
+                );
             }
+
+            for (const floatingGroup of this.floatingGroups) {
+                floatingGroup.overlay.setBounds();
+            }
+
+            if (typeof activeGroup === 'string') {
+                const panel = this.getPanel(activeGroup);
+                if (panel) {
+                    this.doSetGroupActive(panel);
+                }
+            }
+        } catch (err) {
+            /**
+             * Takes all the successfully created groups and remove all of their panels.
+             */
+            for (const group of this.groups) {
+                for (const panel of group.panels) {
+                    this.removePanel(panel, {
+                        removeEmptyGroup: false,
+                        skipDispose: false,
+                    });
+                }
+            }
+
+            /**
+             * To remove a group we cannot call this.removeGroup(...) since this makes assumptions about
+             * the underlying HTMLElement existing in the Gridview.
+             */
+            for (const group of this.groups) {
+                group.dispose();
+                this._groups.delete(group.id);
+                this._onDidRemoveGroup.fire(group);
+            }
+
+            // iterate over a reassigned array since original array will be modified
+            for (const floatingGroup of [...this.floatingGroups]) {
+                floatingGroup.dispose();
+            }
+
+            // fires clean-up events and clears the underlying HTML gridview.
+            this.clear();
+
+            /**
+             * even though we have cleaned-up we still want to inform the caller of their error
+             * and we'll do this through re-throwing the original error since afterall you would
+             * expect trying to load a corrupted layout to result in an error and not silently fail...
+             */
+            throw err;
         }
 
         this._onDidLayoutFromJSON.fire();
@@ -1052,11 +1176,11 @@ export class DockviewComponent
         const floatingGroup = this.floatingGroups.find(
             (_) => _.group === group
         );
-
         if (floatingGroup) {
             if (!options?.skipDispose) {
                 floatingGroup.group.dispose();
                 this._groups.delete(group.id);
+                // TODO: fire group removed event?
             }
             floatingGroup.dispose();
 

packages/dockview-core/src/gridview/baseComponentGridview.ts

@@ -78,7 +78,7 @@ export abstract class BaseGrid<T extends IGridPanelView>
     private _onDidLayoutChange = new Emitter<void>();
     readonly onDidLayoutChange = this._onDidLayoutChange.event;
 
-    private readonly _onDidRemoveGroup = new Emitter<T>();
+    protected readonly _onDidRemoveGroup = new Emitter<T>();
     readonly onDidRemoveGroup: Event<T> = this._onDidRemoveGroup.event;
 
     protected readonly _onDidAddGroup = new Emitter<T>();

packages/dockview-core/src/gridview/gridviewComponent.ts

@@ -177,60 +177,82 @@ export class GridviewComponent
 
         const { grid, activePanel } = serializedGridview;
 
-        const queue: Function[] = [];
+        try {
+            const queue: Function[] = [];
 
-        // take note of the existing dimensions
+            // take note of the existing dimensions
-        const width = this.width;
+            const width = this.width;
-        const height = this.height;
+            const height = this.height;
 
-        this.gridview.deserialize(grid, {
+            this.gridview.deserialize(grid, {
-            fromJSON: (node) => {
+                fromJSON: (node) => {
-                const { data } = node;
+                    const { data } = node;
-                const view = createComponent(
+                    const view = createComponent(
-                    data.id,
+                        data.id,
-                    data.component,
+                        data.component,
-                    this.options.components || {},
+                        this.options.components || {},
-                    this.options.frameworkComponents || {},
+                        this.options.frameworkComponents || {},
-                    this.options.frameworkComponentFactory
+                        this.options.frameworkComponentFactory
-                        ? {
+                            ? {
-                              createComponent:
+                                  createComponent:
-                                  this.options.frameworkComponentFactory
+                                      this.options.frameworkComponentFactory
-                                      .createComponent,
+                                          .createComponent,
-                          }
+                              }
-                        : undefined
+                            : undefined
-                );
+                    );
 
-                queue.push(() =>
+                    queue.push(() =>
-                    view.init({
+                        view.init({
-                        params: data.params,
+                            params: data.params,
-                        minimumWidth: data.minimumWidth,
+                            minimumWidth: data.minimumWidth,
-                        maximumWidth: data.maximumWidth,
+                            maximumWidth: data.maximumWidth,
-                        minimumHeight: data.minimumHeight,
+                            minimumHeight: data.minimumHeight,
-                        maximumHeight: data.maximumHeight,
+                            maximumHeight: data.maximumHeight,
-                        priority: data.priority,
+                            priority: data.priority,
-                        snap: !!data.snap,
+                            snap: !!data.snap,
-                        accessor: this,
+                            accessor: this,
-                        isVisible: node.visible,
+                            isVisible: node.visible,
-                    })
+                        })
-                );
+                    );
 
-                this._onDidAddGroup.fire(view);
+                    this._onDidAddGroup.fire(view);
 
-                this.registerPanel(view);
+                    this.registerPanel(view);
 
-                return view;
+                    return view;
-            },
+                },
-        });
+            });
 
-        this.layout(width, height, true);
+            this.layout(width, height, true);
 
-        queue.forEach((f) => f());
+            queue.forEach((f) => f());
 
-        if (typeof activePanel === 'string') {
+            if (typeof activePanel === 'string') {
-            const panel = this.getPanel(activePanel);
+                const panel = this.getPanel(activePanel);
-            if (panel) {
+                if (panel) {
-                this.doSetGroupActive(panel);
+                    this.doSetGroupActive(panel);
+                }
             }
+        } catch (err) {
+            /**
+             * To remove a group we cannot call this.removeGroup(...) since this makes assumptions about
+             * the underlying HTMLElement existing in the Gridview.
+             */
+            for (const group of this.groups) {
+                group.dispose();
+                this._groups.delete(group.id);
+                this._onDidRemoveGroup.fire(group);
+            }
+
+            // fires clean-up events and clears the underlying HTML gridview.
+            this.clear();
+
+            /**
+             * even though we have cleaned-up we still want to inform the caller of their error
+             * and we'll do this through re-throwing the original error since afterall you would
+             * expect trying to load a corrupted layout to result in an error and not silently fail...
+             */
+            throw err;
         }
 
         this._onDidLayoutfromJSON.fire();