Fix perspective-workspace restore call on errored table

Signed-off-by: Andrew Stein <steinlink@gmail.com>
finos · Nov 15, 2024 · c4fd31e · c4fd31e
1 parent e9b6cfa
commit c4fd31e
Show file tree

Hide file tree

Showing 9 changed files with 84 additions and 23 deletions.
diff --git a/packages/perspective-viewer-datagrid/src/js/model/toolbar.js b/packages/perspective-viewer-datagrid/src/js/model/toolbar.js
@@ -26,18 +26,21 @@ export function toggle_edit_mode(mode = undefined) {
             ];
     }
 
-    this.model._edit_mode = mode;
-    this.parentElement?.setSelection();
-    this.model._selection_state = {
-        selected_areas: [],
-        dirty: true,
-    };
+    if (this.model) {
+        this.model._edit_mode = mode;
+        this.parentElement?.setSelection();
+        this.model._selection_state = {
+            selected_areas: [],
+            dirty: true,
+        };
 
-    this._edit_mode = mode;
-    this.dataset.editMode = mode;
-    if (this._edit_button !== undefined) {
-        this._edit_button.dataset.editMode = mode;
+        this._edit_mode = mode;
+        if (this._edit_button !== undefined) {
+            this._edit_button.dataset.editMode = mode;
+        }
     }
+
+    this.dataset.editMode = mode;
 }
 
 export function toggle_scroll_lock(force = undefined) {

diff --git a/packages/perspective-workspace/test/js/table.spec.js b/packages/perspective-workspace/test/js/table.spec.js
@@ -58,6 +58,57 @@ function tests(context, compare) {
 
         return compare(page, `${context}-replace-table-frees-table.txt`);
     });
+
+    test("replaceTable() works when previous table errored", async ({
+        page,
+    }) => {
+        const config = {
+            viewers: {
+                One: { table: "errored", name: "One" },
+            },
+            detail: {
+                main: {
+                    currentIndex: 0,
+                    type: "tab-area",
+                    widgets: ["One"],
+                },
+            },
+        };
+
+        const result = await page.evaluate(async (config) => {
+            const workspace = document.getElementById("workspace");
+            await workspace.addTable(
+                "errored",
+                new Promise((_, reject) => setTimeout(reject, 50))
+            );
+
+            try {
+                await workspace.restore(config);
+            } catch (e) {
+                console.error(e);
+                return e.toString();
+            }
+        }, config);
+
+        // NOTE This is the error message we expect when `restore()` is called
+        // without a `Table`, subject to change.
+        expect(result).toEqual("Error: `restore()` called before `load()`");
+        await page.evaluate(async () => {
+            await workspace.replaceTable(
+                "errored",
+                window.__WORKER__.table("x\n1")
+            );
+        });
+
+        await page.evaluate(async () => {
+            await workspace.flush();
+        });
+
+        return compare(
+            page,
+            `${context}-replace-table-works-with-errored-table.txt`
+        );
+    });
 }
 
 test.describe("Workspace table functions", () => {

diff --git a/rust/perspective-viewer/src/rust/components/column_settings_sidebar/style_tab.rs b/rust/perspective-viewer/src/rust/components/column_settings_sidebar/style_tab.rs
@@ -52,11 +52,12 @@ impl StyleTabProps {
                 .presentation
                 .update_columns_config_value(props.column_name.clone(), update);
             let columns_configs = props.presentation.all_columns_configs();
-            let plugin_config = props.renderer.get_active_plugin()?.save();
+            let plugin_config = props.renderer.get_active_plugin()?.save()?;
             props
                 .renderer
                 .get_active_plugin()?
-                .restore(&plugin_config, Some(&columns_configs));
+                .restore(&plugin_config, Some(&columns_configs))?;
+
             props.renderer.update(&props.session).await?;
             let detail = serde_wasm_bindgen::to_value(&columns_configs).unwrap();
             props.custom_events.dispatch_column_style_changed(&detail);

diff --git a/rust/perspective-viewer/src/rust/components/viewer.rs b/rust/perspective-viewer/src/rust/components/viewer.rs
@@ -258,7 +258,7 @@ impl Component for PerspectiveViewer {
                         Some(presentation.all_columns_configs())
                     };
 
-                    renderer.reset(columns_config.as_ref()).await;
+                    renderer.reset(columns_config.as_ref()).await?;
                     presentation.reset_available_themes(None).await;
                     let result = renderer.draw(session.validate().await?.create_view()).await;
                     if let Some(sender) = sender {

diff --git a/rust/perspective-viewer/src/rust/js/plugin.rs b/rust/perspective-viewer/src/rust/js/plugin.rs
@@ -61,11 +61,11 @@ extern "C" {
     #[wasm_bindgen(method, catch)]
     pub fn column_style_controls(this: &JsPerspectiveViewerPlugin, view_type: &str, group: Option<&str>) -> ApiResult<JsValue>;
 
-    #[wasm_bindgen(method)]
-    pub fn save(this: &JsPerspectiveViewerPlugin) -> JsValue;
+    #[wasm_bindgen(method, catch)]
+    pub fn save(this: &JsPerspectiveViewerPlugin) -> ApiResult<JsValue>;
 
-    #[wasm_bindgen(method, js_name=restore)]
-    pub fn _restore(this: &JsPerspectiveViewerPlugin, token: &JsValue, columns_config: &JsValue);
+    #[wasm_bindgen(method, js_name=restore, catch)]
+    pub fn _restore(this: &JsPerspectiveViewerPlugin, token: &JsValue, columns_config: &JsValue) -> ApiResult<()>;
 
     #[wasm_bindgen(method)]
     pub fn delete(this: &JsPerspectiveViewerPlugin);
@@ -103,7 +103,11 @@ extern "C" {
 }
 
 impl JsPerspectiveViewerPlugin {
-    pub fn restore(&self, token: &JsValue, columns_config: Option<&ColumnConfigMap>) {
+    pub fn restore(
+        &self,
+        token: &JsValue,
+        columns_config: Option<&ColumnConfigMap>,
+    ) -> ApiResult<()> {
         let columns_config = JsValue::from_serde_ext(&columns_config).unwrap();
         self._restore(token, &columns_config)
     }

diff --git a/rust/perspective-viewer/src/rust/model/get_viewer_config.rs b/rust/perspective-viewer/src/rust/model/get_viewer_config.rs
@@ -54,7 +54,7 @@ pub trait GetViewerConfigModel: HasSession + HasRenderer + HasPresentation {
             let js_plugin = renderer.get_active_plugin()?;
             let settings = presentation.is_settings_open();
             let plugin = js_plugin.name();
-            let plugin_config: serde_json::Value = js_plugin.save().into_serde_ext()?;
+            let plugin_config: serde_json::Value = js_plugin.save()?.into_serde_ext()?;
             let theme = presentation.get_selected_theme_name().await;
             let title = presentation.get_title();
             let columns_config = presentation.all_columns_configs();

diff --git a/rust/perspective-viewer/src/rust/model/restore_and_render.rs b/rust/perspective-viewer/src/rust/model/restore_and_render.rs
@@ -81,12 +81,12 @@ pub trait RestoreAndRender: HasRenderer + HasSession + HasPresentation {
                 let plugin_update = if let Some(x) = plugin_config {
                     wasm_bindgen::JsValue::from_serde_ext(&*x).unwrap()
                 } else {
-                    plugin.save()
+                    plugin.save()?
                 };
 
                 presentation.update_columns_configs(columns_config);
                 let columns_config = presentation.all_columns_configs();
-                plugin.restore(&plugin_update, Some(&columns_config));
+                plugin.restore(&plugin_update, Some(&columns_config))?;
                 session.validate().await?.create_view().await
             });
 

diff --git a/rust/perspective-viewer/src/rust/renderer.rs b/rust/perspective-viewer/src/rust/renderer.rs
@@ -123,11 +123,13 @@ impl Renderer {
         }))
     }
 
-    pub async fn reset(&self, columns_config: Option<&ColumnConfigMap>) {
+    pub async fn reset(&self, columns_config: Option<&ColumnConfigMap>) -> ApiResult<()> {
         self.0.borrow_mut().plugins_idx = None;
         if let Ok(plugin) = self.get_active_plugin() {
-            plugin.restore(&json!({}), columns_config);
+            plugin.restore(&json!({}), columns_config)?;
         }
+
+        Ok(())
     }
 
     pub fn delete(&self) -> ApiResult<()> {

diff --git a/tools/perspective-test/results.tar.gz b/tools/perspective-test/results.tar.gz