From c469a68d6e9e95625e89ea4460822a83a1f27c84 Mon Sep 17 00:00:00 2001 From: George Gevoian Date: Tue, 28 May 2024 15:13:10 -0700 Subject: [PATCH] (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 | 6 +- app/client/models/VirtualTable.ts | 112 +++++++++++++++++------------- app/client/ui/AdminPanel.ts | 1 + app/client/ui/AdminPanelCss.ts | 4 +- app/client/ui/TimingPage.ts | 4 +- app/client/ui/WebhookPage.ts | 7 +- test/nbrowser/Timing.ts | 19 +++++ 7 files changed, 96 insertions(+), 57 deletions(-) diff --git a/app/client/models/DocData.ts b/app/client/models/DocData.ts index 747a6f19..2c2234fd 100644 --- a/app/client/models/DocData.ts +++ b/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 { const tableName = String(actions[0]?.[1]); diff --git a/app/client/models/VirtualTable.ts b/app/client/models/VirtualTable.ts index 2bcab26e..c955beb4 100644 --- a/app/client/models/VirtualTable.ts +++ b/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; // get initial state of the table. sync(editor: IEdit): Promise; // incorporate external changes. beforeEdit(editor: IEdit): Promise; // called prior to committing a change. @@ -63,43 +62,39 @@ export class VirtualTableData extends TableData { public ext: IExternalTable; public cache: DocDataCache; - constructor(docData: DocData, tableId: string, tableData: TableDataAction|null, columnTypes: ColTypeMap) { - super(docData, tableId, tableData, columnTypes); - } - - public setExt(_ext: IExternalTable) { - this.ext = _ext; - this.cache = new DocDataCache(this.ext.initialActions); - } - - public get name() { - return this.ext.name; - } - - public fetchData() { + public override fetchData() { return super.fetchData(async () => { const data = await this.ext.fetchAll(); - this.cache.docData.getTable(this.name)?.loadData(data); + this.cache.docData.getTable(this.getName())?.loadData(data); return data; }); } - public async sendTableActions(userActions: UserAction[]): Promise { + public override async sendTableActions(userActions: UserAction[]): Promise { 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 { + public override async sendTableAction(action: UserAction): Promise { const retValues = await this.sendTableActions([action]); return retValues[0]; } + public setExt(_ext: IExternalTable) { + this.ext = _ext; + this.cache = new DocDataCache(this.ext.initialActions); + } + + public getName() { + return this.ext.name; + } + + public sync() { + return this.ext.sync(this._editor()); + } + 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 { - public lazySync = debounce(this.sync, 1000, { +export class VirtualTableRegistration extends DisposableWithEvents { + public lazySync = debounce(this._sync, 1000, { maxWait: 2000, trailing: true, }); - public tableData: VirtualTableData; + private _tableData: VirtualTableData; - public constructor(private _owner: DisposableWithEvents, - _gristDoc: GristDoc, - _ext: IExternalTable) { - if (!_gristDoc.docModel.docData.getTable(_ext.name)) { + constructor(gristDoc: GristDoc, ext: IExternalTable) { + super(); + if (!gristDoc.docModel.docData.getTable(ext.name)) { - // register the virtual table - _gristDoc.docModel.docData.registerVirtualTable(_ext.name, VirtualTableData); + // Register the virtual table + gristDoc.docModel.docData.registerVirtualTableFactory(ext.name, VirtualTableData); // then process initial actions - for (const action of _ext.initialActions) { - _gristDoc.docData.receiveAction(action); + for (const action of ext.initialActions) { + 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.setExt(_ext); + this._tableData.gristDoc = gristDoc; + this._tableData.setExt(ext); // subscribe to schema changes - this.tableData.schemaChange().catch(e => reportError(e)); - _owner.listenTo(_gristDoc, 'schemaUpdateAction', () => this.tableData.schemaChange()); + this._tableData.schemaChange().catch(e => reportError(e)); + 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() { - if (this._owner.isDisposed()) { + private async _sync() { + 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[]; +} diff --git a/app/client/ui/AdminPanel.ts b/app/client/ui/AdminPanel.ts index dfecb1ff..b726b667 100644 --- a/app/client/ui/AdminPanel.ts +++ b/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'), ]); } diff --git a/app/client/ui/AdminPanelCss.ts b/app/client/ui/AdminPanelCss.ts index b4bd9709..9fb8b15f 100644 --- a/app/client/ui/AdminPanelCss.ts +++ b/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) { 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, diff --git a/app/client/ui/TimingPage.ts b/app/client/ui/TimingPage.ts index c64cac28..14d0ab94 100644 --- a/app/client/ui/TimingPage.ts +++ b/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); } } diff --git a/app/client/ui/WebhookPage.ts b/app/client/ui/WebhookPage.ts index f04bc104..9e263fc0 100644 --- a/app/client/ui/WebhookPage.ts +++ b/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(); 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(); - }); } diff --git a/test/nbrowser/Timing.ts b/test/nbrowser/Timing.ts index 7c151151..b21e1304 100644 --- a/test/nbrowser/Timing.ts +++ b/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) => ({