(core) Removing virtual tables when they are not needed

Summary:
Clearing virtual tables after user navigates away from the pages
that show them. Leaving them behind will reveal them on the Raw
Data page, with a buggy experience as user can't view the data
there.

Test Plan: Extended tests.

Reviewers: paulfitz

Reviewed By: paulfitz

Subscribers: jarek, georgegevoian

Differential Revision: https://phab.getgrist.com/D4258

app/client/models/DocData.ts

@@ -186,10 +186,14 @@ export class DocData extends BaseDocData {
     return this.sendActions([action], optDesc).then((retValues) => retValues[0]);
   }
 
-  public registerVirtualTable(tableId: string, Cons: typeof TableData) {
+  public registerVirtualTableFactory(tableId: string, Cons: typeof TableData) {
     this._virtualTablesFunc.set(tableId, Cons);
   }
 
+  public unregisterVirtualTableFactory(tableId: string) {
+    this._virtualTablesFunc.delete(tableId);
+  }
+
   // See documentation of sendActions().
   private async _sendActionsImpl(actions: UserAction[], optDesc?: string): Promise<any[]> {
     const tableName = String(actions[0]?.[1]);

app/client/models/VirtualTable.ts

@@ -1,6 +1,5 @@
 import { reportError } from 'app/client/models/errors';
 import { GristDoc } from 'app/client/components/GristDoc';
-import { DocData } from 'app/client/models/DocData';
 import { TableData } from 'app/client/models/TableData';
 import { concatenateSummaries, summarizeStoredAndUndo } from 'app/common/ActionSummarizer';
 import { TableDelta } from 'app/common/ActionSummary';
@@ -8,7 +7,6 @@ import { ProcessedAction } from 'app/common/AlternateActions';
 import { DisposableWithEvents } from 'app/common/DisposableWithEvents';
 import { DocAction, TableDataAction, UserAction } from 'app/common/DocActions';
 import { DocDataCache } from 'app/common/DocDataCache';
-import { ColTypeMap } from 'app/common/TableData';
 import { RowRecord } from 'app/plugin/GristData';
 import debounce = require('lodash/debounce');
 
@@ -43,6 +41,7 @@ export interface IEdit {
 export interface IExternalTable {
   name: string;  // the tableId of the virtual table (e.g. GristHidden_WebhookTable)
   initialActions: DocAction[];  // actions to create the table.
+  destroyActions?: DocAction[];  // actions to destroy the table (auto generated if not defined), pass [] to disable.
   fetchAll(): Promise<TableDataAction>;  // get initial state of the table.
   sync(editor: IEdit): Promise<void>;    // incorporate external changes.
   beforeEdit(editor: IEdit): Promise<void>;  // called prior to committing a change.
@@ -63,8 +62,24 @@ export class VirtualTableData extends TableData {
   public ext: IExternalTable;
   public cache: DocDataCache;
 
-  constructor(docData: DocData, tableId: string, tableData: TableDataAction|null, columnTypes: ColTypeMap) {
+  public override fetchData() {
-    super(docData, tableId, tableData, columnTypes);
+    return super.fetchData(async () => {
+      const data = await this.ext.fetchAll();
+      this.cache.docData.getTable(this.getName())?.loadData(data);
+      return data;
+    });
+  }
+
+  public override async sendTableActions(userActions: UserAction[]): Promise<any[]> {
+    const actions = await this._sendTableActionsCore(userActions,
+                                                     {isUser: true});
+    await this.ext.afterEdit(this._editor(actions));
+    return actions.map(action => action.retValues);
+  }
+
+  public override async sendTableAction(action: UserAction): Promise<any> {
+    const retValues = await this.sendTableActions([action]);
+    return retValues[0];
   }
 
   public setExt(_ext: IExternalTable) {
@@ -72,34 +87,14 @@ export class VirtualTableData extends TableData {
     this.cache = new DocDataCache(this.ext.initialActions);
   }
 
-  public get name() {
+  public getName() {
     return this.ext.name;
   }
 
-  public fetchData() {
-    return super.fetchData(async () => {
-      const data = await this.ext.fetchAll();
-      this.cache.docData.getTable(this.name)?.loadData(data);
-      return data;
-    });
-  }
-
-  public async sendTableActions(userActions: UserAction[]): Promise<any[]> {
-    const actions = await this._sendTableActionsCore(userActions,
-                                                     {isUser: true});
-    await this.ext.afterEdit(this._editor(actions));
-    return actions.map(action => action.retValues);
-  }
-
   public sync() {
     return this.ext.sync(this._editor());
   }
 
-  public async sendTableAction(action: UserAction): Promise<any> {
-    const retValues = await this.sendTableActions([action]);
-    return retValues[0];
-  }
-
   public async schemaChange() {
     await this.ext.afterAnySchemaChange(this._editor());
   }
@@ -108,7 +103,7 @@ export class VirtualTableData extends TableData {
     const summary = concatenateSummaries(
       actions
         .map(action => summarizeStoredAndUndo(action.stored, action.undo)));
-    const delta = summary.tableDeltas[this.name];
+    const delta = summary.tableDeltas[this.getName()];
     return {
       actions,
       delta,
@@ -135,7 +130,7 @@ export class VirtualTableData extends TableData {
     }
     const actions = await this.cache.sendTableActions(userActions);
     if (isUser) {
-      const newTable = await this.cache.docData.requireTable(this.name);
+      const newTable = await this.cache.docData.requireTable(this.getName());
       try {
         await this.ext.beforeEdit({
           ...this._editor(actions),
@@ -155,7 +150,7 @@ export class VirtualTableData extends TableData {
         this.docData.receiveAction(docAction);
         this.cache.docData.receiveAction(docAction);
         if (isUser) {
-          const code = `ext-${this.name}-${_counterForUndoActions}`;
+          const code = `ext-${this.getName()}-${_counterForUndoActions}`;
           _counterForUndoActions++;
           this.gristDoc.getUndoStack().pushAction({
             actionNum: code,
@@ -197,46 +192,67 @@ export class VirtualTableData extends TableData {
  * one second after last call (or at most 2 seconds after the first
  * call).
  */
-export class VirtualTable {
+export class VirtualTableRegistration extends DisposableWithEvents {
-  public lazySync = debounce(this.sync, 1000, {
+  public lazySync = debounce(this._sync, 1000, {
     maxWait: 2000,
     trailing: true,
   });
-  public tableData: VirtualTableData;
+  private _tableData: VirtualTableData;
 
-  public constructor(private _owner: DisposableWithEvents,
+  constructor(gristDoc: GristDoc, ext: IExternalTable) {
-                     _gristDoc: GristDoc,
+    super();
-                     _ext: IExternalTable) {
+    if (!gristDoc.docModel.docData.getTable(ext.name)) {
-    if (!_gristDoc.docModel.docData.getTable(_ext.name)) {
 
-      // register the virtual table
+      // Register the virtual table
-      _gristDoc.docModel.docData.registerVirtualTable(_ext.name, VirtualTableData);
+      gristDoc.docModel.docData.registerVirtualTableFactory(ext.name, VirtualTableData);
 
       // then process initial actions
-      for (const action of _ext.initialActions) {
+      for (const action of ext.initialActions) {
-        _gristDoc.docData.receiveAction(action);
+        gristDoc.docData.receiveAction(action);
       }
-
       // pass in gristDoc and external interface
-      this.tableData = _gristDoc.docModel.docData.getTable(_ext.name)! as VirtualTableData;
+      this._tableData = gristDoc.docModel.docData.getTable(ext.name)! as VirtualTableData;
       //this.tableData.docApi = this.docApi;
-      this.tableData.gristDoc = _gristDoc;
+      this._tableData.gristDoc = gristDoc;
-      this.tableData.setExt(_ext);
+      this._tableData.setExt(ext);
 
       // subscribe to schema changes
-      this.tableData.schemaChange().catch(e => reportError(e));
+      this._tableData.schemaChange().catch(e => reportError(e));
-      _owner.listenTo(_gristDoc, 'schemaUpdateAction', () => this.tableData.schemaChange());
+      this.listenTo(gristDoc, 'schemaUpdateAction', () => this._tableData.schemaChange());
     } else {
-      this.tableData = _gristDoc.docModel.docData.getTable(_ext.name)! as VirtualTableData;
+      throw new Error(`Virtual table ${ext.name} already exists`);
     }
-    // debounce is typed as returning a promise, but doesn't appear to actually do so?
+    // debounce is typed as returning a promise, but doesn't appear to actually //do so?
     Promise.resolve(this.lazySync()).catch(e => reportError(e));
+
+    this.onDispose(() => {
+      const reverse = ext.destroyActions ?? generateDestroyActions(ext.initialActions);
+      reverse.forEach(action => gristDoc.docModel.docData.receiveAction(action));
+      gristDoc.docModel.docData.unregisterVirtualTableFactory(ext.name);
+    });
   }
 
-  public async sync() {
+  private async _sync() {
-    if (this._owner.isDisposed()) {
+    if (this.isDisposed()) {
       return;
     }
-    await this.tableData.sync();
+    await this._tableData.sync();
   }
 }
+
+/**
+ * This is a helper method that generates undo actions for actions that create a virtual
+ * table. It just removes everything using the ids in the initial actions. It tries to fail
+ * if actions are more complex than simple create table/columns actions.
+ */
+function generateDestroyActions(initialActions: DocAction[]): DocAction[] {
+  return initialActions.map(action => {
+    switch (action[0]) {
+      case 'AddTable': return ['RemoveTable', action[1]];
+      case 'AddColumn': return ['RemoveColumn', action[1]];
+      case 'AddRecord': return ['RemoveRecord', action[1], action[2]];
+      case 'BulkAddRecord': return ['BulkRemoveRecord', action[1], action[2]];
+      default: throw new Error(`Cannot generate destroy action for ${action[0]}`);
+    }
+  }).reverse() as unknown as DocAction[];
+}

app/client/ui/AdminPanel.ts

@@ -109,6 +109,7 @@ Please log in as an administrator.`)),
           url: dom('pre', `/admin?boot-key=${exampleKey}`)
         }),
       ),
+      testId('admin-panel-error'),
     ]);
   }
 

app/client/ui/AdminPanelCss.ts

@@ -2,13 +2,13 @@ import {transition} from 'app/client/ui/transitions';
 import {toggle} from 'app/client/ui2018/checkbox';
 import {mediaSmall, testId, theme, vars} from 'app/client/ui2018/cssVars';
 import {icon} from 'app/client/ui2018/icons';
-import {dom, DomContents, IDisposableOwner, Observable, styled} from 'grainjs';
+import {dom, DomContents, DomElementArg, IDisposableOwner, Observable, styled} from 'grainjs';
 
 export function HidableToggle(owner: IDisposableOwner, value: Observable<boolean|null>) {
   return toggle(value, dom.hide((use) => use(value) === null));
 }
 
-export function AdminSection(owner: IDisposableOwner, title: DomContents, items: DomContents[]) {
+export function AdminSection(owner: IDisposableOwner, title: DomContents, items: DomElementArg[]) {
   return cssSection(
     cssSectionTitle(title),
     ...items,

app/client/ui/TimingPage.ts

@@ -2,7 +2,7 @@ import BaseView = require('app/client/components/BaseView');
 import {GristDoc} from 'app/client/components/GristDoc';
 import {ViewSectionHelper} from 'app/client/components/ViewLayout';
 import {makeT} from 'app/client/lib/localization';
-import {IEdit, IExternalTable, VirtualTable} from 'app/client/models/VirtualTable';
+import {IEdit, IExternalTable, VirtualTableRegistration} from 'app/client/models/VirtualTable';
 import {urlState} from 'app/client/models/gristUrlState';
 import {docListHeader} from 'app/client/ui/DocMenuCss';
 import {isNarrowScreenObs, mediaSmall} from 'app/client/ui2018/cssVars';
@@ -163,7 +163,7 @@ export class TimingPage extends DisposableWithEvents {
 
     // And wire up the UI.
     const ext = this.autoDispose(new TimingExternalTable(data));
-    new VirtualTable(this, this._gristDoc, ext);
+    this.autoDispose(new VirtualTableRegistration(this._gristDoc, ext));
     this._data.set(data);
   }
 }

app/client/ui/WebhookPage.ts

@@ -2,7 +2,7 @@ import {GristDoc} from 'app/client/components/GristDoc';
 import {ViewSectionHelper} from 'app/client/components/ViewLayout';
 import {makeT} from 'app/client/lib/localization';
 import {reportMessage, reportSuccess} from 'app/client/models/errors';
-import {IEdit, IExternalTable, VirtualTable} from 'app/client/models/VirtualTable';
+import {IEdit, IExternalTable, VirtualTableRegistration} from 'app/client/models/VirtualTable';
 import {docListHeader} from 'app/client/ui/DocMenuCss';
 import {bigPrimaryButton} from 'app/client/ui2018/buttons';
 import {mediaSmall, testId} from 'app/client/ui2018/cssVars';
@@ -337,7 +337,7 @@ class WebhookExternalTable implements IExternalTable {
 export class WebhookPage extends DisposableWithEvents {
 
   public docApi = this.gristDoc.docPageModel.appModel.api.getDocAPI(this.gristDoc.docId());
-  public sharedTable: VirtualTable;
+  public sharedTable: VirtualTableRegistration;
   private _webhookExternalTable: WebhookExternalTable;
 
 
@@ -345,10 +345,9 @@ export class WebhookPage extends DisposableWithEvents {
     super();
     //this._webhooks = observableArray<WebhookSummary>();
     this._webhookExternalTable = new WebhookExternalTable(this.docApi);
-    const table = new VirtualTable(this, gristDoc, this._webhookExternalTable);
+    const table = this.autoDispose(new VirtualTableRegistration(gristDoc, this._webhookExternalTable));
     this.listenTo(gristDoc, 'webhooks', async () => {
       await table.lazySync();
-
     });
   }
 

test/nbrowser/Timing.ts

@@ -147,6 +147,25 @@ describe("Timing", function () {
     await driver.navigate().refresh();
     await gu.waitForUrl('/settings');
   });
+
+  it('clears virtual table when navigated away', async function() {
+    // Start timing and go to results.
+    await startTiming.click();
+    await modal.wait();
+    await optionReload.click();
+    await modalConfirm.click();
+
+    // Wait for the results page.
+    await gu.waitToPass(async () => {
+      assert.isTrue(await driver.findContentWait('div', 'Formula timer', 1000).isDisplayed());
+      assert.equal(await gu.getCell(0, 1).getText(), 'Table1');
+    });
+
+    // Now go to the raw data page, and make sure we see only Table1.
+    await driver.find('.test-tools-raw').click();
+    await driver.findWait('.test-raw-data-list', 2000);
+    assert.deepEqual(await driver.findAll('.test-raw-data-table-id', e => e.getText()), ['Table1']);
+  });
 });
 
 const element = (testId: string) => ({