(core) Automatically finalize action bundles when unrelated actions/bundles come in.

Summary:
Type conversions and formula tranforms wait for the user and bundle multiple
actions. When an unrelated action is done (e.g. adding a page widget or a
column), we want to finalize the transform before applying it.

The approach turns out fairly complicated. There is an implicit queue of
bundles (which we don't let grow beyond 2, as that's too abnormal). Bundles may
be finalized by a user clicking something, or by an unrelated action/bundle, or
(as before) by transform DOM getting disposed.

- Updated RecordLayout to use bundleActions() helper
- Added support for nesting bundleActions inside another bundle (needed for
  setting visibleCol during type change)
- In an unrelated tweak, when in debug-log in ActiveDoc, use a short representation of result.

Test Plan: Added a unittest for action bundling during type transform

Reviewers: paulfitz

Reviewed By: paulfitz

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

app/client/components/ColumnTransform.ts

@@ -9,6 +9,7 @@ import {ColumnRec} from 'app/client/models/entities/ColumnRec';
 import {ViewFieldRec} from 'app/client/models/entities/ViewFieldRec';
 import {TableData} from 'app/client/models/TableData';
 import {FieldBuilder} from 'app/client/widgets/FieldBuilder';
+import {UserAction} from 'app/common/DocActions';
 import {Disposable, Observable} from 'grainjs';
 import * as ko from 'knockout';
 import noop = require('lodash/noop');
@@ -33,8 +34,17 @@ export class ColumnTransform extends Disposable {
   protected formulaUpToDate = Observable.create(this, true);
   protected _tableData: TableData;
 
-    // This is set to true in the interval between execute() and dispose().
-  private _isExecuting: boolean = false;
+  // Whether _doFinalize should execute the transform, or cancel it.
+  protected _shouldExecute: boolean = false;
+
+  // Ask DocData to finalize the action bundle by calling the finalize callback provided to
+  // startBundlingActions. Finalizing should always be triggered this way, for a uniform flow,
+  // since finalizing could be triggered either from DocData or from cancel/execute methods.
+  // This is a noop until startBundlingActions is called.
+  private _triggerFinalize: (() => void) = noop;
+
+  // This is set to true once finalize has started.
+  private _isFinalizing: boolean = false;
 
   constructor(protected gristDoc: GristDoc, private _fieldBuilder: FieldBuilder) {
     super();
@@ -65,8 +75,8 @@ export class ColumnTransform extends Disposable {
     throw new Error("Not Implemented");
   }
 
-  public finalize() {
-    // Implemented in FormulaTransform.
+  public async finalize(): Promise<void> {
+    return this._triggerFinalize();
   }
 
   /**
@@ -90,13 +100,28 @@ export class ColumnTransform extends Disposable {
    * Helper called by contructor to prepare the column transform.
    * @param {String} colType: A pure or complete type for the transformed column.
    */
-  public async prepare(colType?: string) {
-    colType = colType || this.origColumn.type.peek();
-    // Start bundling all actions during the transform, but include a verification callback to ensure
-    // no errant actions are added to the bundle.
-    this._tableData.docData.startBundlingActions(`Transformed column ${this.origColumn.colId()}.`,
-      action => (action[2] === "gristHelper_Transform" || action[1] === "_grist_Tables_column" ||
-        action[0] === "SetDisplayFormula" || action[1] === "_grist_Views_section_field"));
+  public async prepare(optColType?: string) {
+    const colType: string = optColType || this.origColumn.type.peek();
+
+    // Start bundling all actions during the transform. The verification callback ensures
+    // no errant actions are added to the bundle; if there are, finalize is immediately called.
+    const bundlingInfo = this._tableData.docData.startBundlingActions({
+      description: `Transformed column ${this.origColumn.colId()}.`,
+      shouldIncludeInBundle: this._shouldIncludeInBundle.bind(this),
+      prepare: this._doPrepare.bind(this, colType),
+      finalize: this._doFinalize.bind(this)
+    });
+
+    // triggerFinalize tells DocData to call the finalize callback we passed above; this way
+    // DocData knows when it's finished.
+    this._triggerFinalize = bundlingInfo.triggerFinalize;
+
+    // preparePromise resolves once prepare() callback has got a chance to run and finish.
+    await bundlingInfo.preparePromise;
+  }
+
+  private async _doPrepare(colType: string) {
+    if (this.isDisposed()) { return; }
     this.isCallPending(true);
     try {
       const newColRef = await this.addTransformColumn(colType);
@@ -111,6 +136,21 @@ export class ColumnTransform extends Disposable {
     }
   }
 
+  private _shouldIncludeInBundle(actions: UserAction[]) {
+    // Allow certain expected actions. If we encounter anything else, the user must have
+    // started doing something else, and we should finalize the transform.
+    return actions.every(action => (
+      // ['AddColumn', USER_TABLE, 'gristHelper_Transform', colInfo]
+      (action[2] === 'gristHelper_Transform') ||
+      // ["SetDisplayFormula", USER_TABLE, ...]
+      (action[0] === 'SetDisplayFormula') ||
+      // ['UpdateRecord', '_grist_Table_column', transformColId, ...]
+      (action[1] === '_grist_Tables_column') ||
+      // ['UpdateRecord', '_grist_Views_section_field', transformColId, ...] (e.g. resize)
+      (action[1] === '_grist_Views_section_field')
+    ));
+  }
+
   /**
    * Adds the tranform column and returns its colRef. May be overridden by derived classes to create
    * differently-prepared transform columns.
@@ -131,39 +171,43 @@ export class ColumnTransform extends Disposable {
     // Nothing in base class.
   }
 
-  public cancel() {
-    this.field.colRef(this.origColumn.getRowId());
-    this._tableData.sendTableAction(['RemoveColumn', this.transformColumn.colId()]);
-    // TODO: Cancelling a column transform should cancel all involved useractions.
-    this._tableData.docData.stopBundlingActions();
-    this.dispose();
+  public async cancel(): Promise<void> {
+    this._shouldExecute = false;
+    return this._triggerFinalize();
   }
 
-  // TODO: Values flicker during executing since transform column remains a formula as values are copied
-  // back to the original column. The CopyFromColumn useraction really ought to be "CopyAndRemove" since
-  // that seems the best way to avoid calculating the formula on wrong values.
-  protected async execute() {
-    if (this._isExecuting) {
+  protected async execute(): Promise<void> {
+    this._shouldExecute = true;
+    return this._triggerFinalize();
+  }
+
+  // This is passed as a callback to startBundlingActions(), and should NOT be called directly.
+  // Instead, call _triggerFinalize() is used to trigger it.
+  private async _doFinalize(): Promise<void> {
+    if (this.isDisposed() || this._isFinalizing) {
       return;
     }
-    this._isExecuting = true;
+    this._isFinalizing = true;
 
-    // Define variables used in '.then' since this may be disposed
+    // Define variables used after await, since this will be disposed by then.
     const transformColId = this.transformColumn.colId();
     const field = this.field;
     const fieldBuilder = this._fieldBuilder;
     const origRef = this.origColumn.getRowId();
     const tableData = this._tableData;
     this.isCallPending(true);
-
     try {
-      return await tableData.sendTableAction(['CopyFromColumn', transformColId, this.origColumn.colId(),
-        JSON.stringify(fieldBuilder.options())]);
+      if (this._shouldExecute) {
+        // TODO: Values flicker during executing since transform column remains a formula as values are copied
+        // back to the original column. The CopyFromColumn useraction really ought to be "CopyAndRemove" since
+        // that seems the best way to avoid calculating the formula on wrong values.
+        return await tableData.sendTableAction(['CopyFromColumn', transformColId, this.origColumn.colId(),
+          JSON.stringify(fieldBuilder.options())]);
+      }
     } finally {
       // Wait until the change completed to set column back, to avoid value flickering.
       field.colRef(origRef);
       tableData.sendTableAction(['RemoveColumn', transformColId]);
-      tableData.docData.stopBundlingActions();
       this.dispose();
     }
   }
@@ -177,7 +221,7 @@ export class ColumnTransform extends Disposable {
     this.transformColumn.isTransforming(bool);
   }
 
-  protected isExecuting(): boolean {
-    return this._isExecuting;
+  protected isFinalizing(): boolean {
+    return this._isFinalizing;
   }
 }

app/client/components/FormulaTransform.ts

@@ -47,8 +47,4 @@ export class FormulaTransform extends ColumnTransform {
       ),
     ];
   }
-
-  public finalize() {
-    this.cancel();
-  }
 }

app/client/components/RecordLayout.js

@@ -170,7 +170,7 @@ RecordLayout.updateLayoutSpecWithFields = function(spec, viewFields) {
  * remove fields as well as create fields and possibly new columns. And it needs the results of
  * these operations to update the spec before saving it.
  */
-RecordLayout.prototype.saveLayoutSpec = function(layoutSpec) {
+RecordLayout.prototype.saveLayoutSpec = async function(layoutSpec) {
   // The layout hasn't actually changed. Skip the rest to avoid creating no-op actions (the
   // resulting no-op undo would be particularly confusing).
   if (JSON.stringify(layoutSpec) === this.viewSection.layoutSpec.peek()) {
@@ -252,53 +252,51 @@ RecordLayout.prototype.saveLayoutSpec = function(layoutSpec) {
   let positions = addedPositions.concat(hiddenPositions);
   let addActions = gutil.arrayRepeat(addColNum, addColAction);
 
-  docData.startBundlingActions('Updating record layout.', action => {
-    return [tableId, '_grist_Views_section', '_grist_Views_section_field'].includes(action[1]);
-  });
-  return Promise.try(() => {
-    return addColNum > 0 ? docModel.dataTables[tableId].sendTableActions(addActions) : [];
-  })
-  .then(results => {
-    let colRefs = results.map(r => r.colRef).concat(hiddenColRefs);
-    const addFieldNum = colRefs.length;
-    // Add fields for newly added columns and previously hidden columns.
-    return addFieldNum > 0 ?
-      docModel.viewFields.sendTableAction(["BulkAddRecord", gutil.arrayRepeat(addFieldNum, null), {
-        parentId: gutil.arrayRepeat(addFieldNum, this.viewSection.getRowId()),
-        colRef: colRefs,
-        parentPos: positions
-      }]) : [];
-  })
-  .each((fieldRef, i) => {
-    // Call the stored callback for each fieldRef, which each set the correct layoutSpec leaf
-    // to the newly obtained fieldRef.
-    callbacks[i](fieldRef);
-  })
-  .then(addedRefs => {
-    let actions = [];
-
-    // Records present before that were not present after editing must be removed.
-    let finishedRefs = new Set(existingRefs.concat(addedRefs));
-    let removed = origRefs.filter(fieldRef => !finishedRefs.has(fieldRef));
-    if (removed.length > 0) {
-      actions.push(["BulkRemoveRecord", "_grist_Views_section_field", removed]);
-    }
+  await docData.bundleActions('Updating record layout.', () => {
+    return Promise.try(() => {
+      return addColNum > 0 ? docModel.dataTables[tableId].sendTableActions(addActions) : [];
+    })
+    .then(results => {
+      let colRefs = results.map(r => r.colRef).concat(hiddenColRefs);
+      const addFieldNum = colRefs.length;
+      // Add fields for newly added columns and previously hidden columns.
+      return addFieldNum > 0 ?
+        docModel.viewFields.sendTableAction(["BulkAddRecord", gutil.arrayRepeat(addFieldNum, null), {
+          parentId: gutil.arrayRepeat(addFieldNum, this.viewSection.getRowId()),
+          colRef: colRefs,
+          parentPos: positions
+        }]) : [];
+    })
+    .each((fieldRef, i) => {
+      // Call the stored callback for each fieldRef, which each set the correct layoutSpec leaf
+      // to the newly obtained fieldRef.
+      callbacks[i](fieldRef);
+    })
+    .then(addedRefs => {
+      let actions = [];
+
+      // Records present before that were not present after editing must be removed.
+      let finishedRefs = new Set(existingRefs.concat(addedRefs));
+      let removed = origRefs.filter(fieldRef => !finishedRefs.has(fieldRef));
+      if (removed.length > 0) {
+        actions.push(["BulkRemoveRecord", "_grist_Views_section_field", removed]);
+      }
 
-    // Positions must be updated for fields which were not added/removed.
-    if (existingRefs.length > 0) {
-      actions.push(["BulkUpdateRecord", "_grist_Views_section_field", existingRefs, {
-        "parentPos": existingPositions
-      }]);
-    }
+      // Positions must be updated for fields which were not added/removed.
+      if (existingRefs.length > 0) {
+        actions.push(["BulkUpdateRecord", "_grist_Views_section_field", existingRefs, {
+          "parentPos": existingPositions
+        }]);
+      }
 
-    // And update the layoutSpecObj itself.
-    actions.push(["UpdateRecord", "_grist_Views_section", this.viewSection.getRowId(), {
-      "layoutSpec": JSON.stringify(layoutSpec)
-    }]);
+      // And update the layoutSpecObj itself.
+      actions.push(["UpdateRecord", "_grist_Views_section", this.viewSection.getRowId(), {
+        "layoutSpec": JSON.stringify(layoutSpec)
+      }]);
 
-    return docData.sendActions(actions);
-  })
-  .finally(() => docData.stopBundlingActions());
+      return docData.sendActions(actions);
+    })
+  });
 };
 
 /**

app/client/components/TypeTransform.ts

@@ -33,6 +33,7 @@ export class TypeTransform extends ColumnTransform {
 
   constructor(gristDoc: GristDoc, fieldBuilder: FieldBuilder) {
     super(gristDoc, fieldBuilder);
+    this._shouldExecute = true;
 
     // The display widget of the new transform column. Used to build the transform config menu.
     // Only set while transforming.
@@ -59,7 +60,7 @@ export class TypeTransform extends ColumnTransform {
         )
       ),
       cssButtonRow(
-        basicButton(dom.on('click', () => { this.cancel(); disableButtons.set(true); }),
+        basicButton(dom.on('click', () => { this.cancel().catch(reportError); disableButtons.set(true); }),
           'Cancel', testId("type-transform-cancel"),
           dom.cls('disabled', disableButtons)
         ),
@@ -86,7 +87,7 @@ export class TypeTransform extends ColumnTransform {
   }
 
   protected async resetToDefaultFormula() {
-    if (!this.isExecuting()) {
+    if (!this.isFinalizing()) {
       const toType = this.transformColumn.type.peek();
       const formula = TypeConversion.getDefaultFormula(this.gristDoc.docModel, this.origColumn,
         toType, this.field.visibleColRef(), this.field.widgetOptionsJson());
@@ -133,8 +134,4 @@ export class TypeTransform extends ColumnTransform {
       TypeConversion.setDisplayFormula(docModel, tcol, changedInfo.visibleCol)
     ]);
   }
-
-  public finalize() {
-    return this.execute();
-  }
 }

app/client/models/DocData.ts

@@ -16,15 +16,16 @@ import defaults = require('lodash/defaults');
 
 const gristNotify = (window as any).gristNotify;
 
-type BundleCallback = (action: UserAction) => boolean;
-
 export class DocData extends BaseDocData {
   public readonly sendActionsEmitter = new Emitter();
   public readonly sendActionsDoneEmitter = new Emitter();
 
-  // Action verification callback to avoid undesired bundling. Also an indicator that actions are
-  // currently being bundled.
-  private _bundleCallback?: BundleCallback|null = null;
+  private _bundlesPending: number = 0;          // How many bundles are currently pending.
+  private _lastBundlePromise?: Promise<void>;   // Promise for completion of the last pending bundle.
+  private _triggerBundleFinalize?: () => void;  // When a bundle is pending, trigger its finalize() callback.
+
+  // When a bundle is pending and actions should be checked, the callback to check them.
+  private _shouldIncludeInBundle?: (actions: UserAction[]) => boolean;
 
   private _nextDesc: string|null = null;        // The description for the next incoming action.
   private _lastActionNum: number|null = null;   // ActionNum of the last action in the current bundle, or null.
@@ -71,25 +72,80 @@ export class DocData extends BaseDocData {
 
   // Sets a bundle to collect all incoming actions. Throws an error if any actions which
   // do not match the verification callback are sent.
-  public startBundlingActions(desc: string|null, callback: BundleCallback) {
-    this._nextDesc = desc;
-    this._lastActionNum = null;
-    this._bundleCallback = callback;
-  }
+  public startBundlingActions<T>(options: BundlingOptions<T>): BundlingInfo<T> {
+    if (this._bundlesPending >= 2) {
+      // We don't expect a full-blown queue of bundles or actions at any point. If a bundle is
+      // pending, a new bundle should immediately finalize it. Here we refuse to queue up more
+      // actions than that. (This could crop up in theory while disconnected, but is hard to
+      // trigger to test.)
+      throw new Error('Too many actions already pending');
+    }
+    this._bundlesPending++;
 
-  // Ends the active bundle collecting all incoming actions.
-  public stopBundlingActions() {
-    this._bundleCallback = null;
+    // Promise to allow waiting for the result of prepare() callback before it's even called.
+    let prepareResolve!: (value: T) => void;
+    const preparePromise = new Promise<T>(resolve => { prepareResolve = resolve; });
+
+    // Manually-triggered promise for when finalize() should be called. It's triggered by user,
+    // and when an unrelated action or a new bundle is started.
+    let triggerFinalize!: () => void;
+    const triggerFinalizePromise = new Promise<void>(resolve => { triggerFinalize = resolve; });
+
+    const doBundleActions = async () => {
+      if (this._lastBundlePromise) {
+        this._triggerBundleFinalize?.();
+        await this._lastBundlePromise;
+      }
+      this._nextDesc = options.description;
+      this._lastActionNum = null;
+      this._triggerBundleFinalize = triggerFinalize;
+      const value = await options.prepare();
+      prepareResolve(value);
+      this._shouldIncludeInBundle = options.shouldIncludeInBundle;
+
+      try {
+        await triggerFinalizePromise;
+        // Unset _shouldIncludeInBundle so that actions sent by finalize() are included in the
+        // bundle. If they were checked and incorrectly failed the check, we'd have a deadlock.
+        // TODO The downside is that when sending multiple unrelated actions quickly, the first
+        // can trigger finalize, and subsequent ones can get bundled in while finalize() is
+        // running. This changes the order of actions and may create problems (e.g. with undo).
+        this._shouldIncludeInBundle = undefined;
+        await options.finalize();
+      } finally {
+        // In all cases, reset the bundle-specific values we set above
+        this._shouldIncludeInBundle = undefined;
+        this._triggerBundleFinalize = undefined;
+        this._bundlesPending--;
+        if (this._bundlesPending === 0) {
+          this._lastBundlePromise = undefined;
+        }
+      }
+    };
+
+    this._lastBundlePromise = doBundleActions();
+    return {preparePromise, triggerFinalize};
   }
 
   // Execute a callback that may send multiple actions, and bundle those actions together. The
   // callback may return a promise, in which case bundleActions() will wait for it to resolve.
-  public async bundleActions<T>(desc: string|null, callback: () => T|Promise<T>): Promise<T> {
-    this.startBundlingActions(desc, () => true);
-    try {
+  // If nestInActiveBundle is true, and there is an active bundle, then simply calls callback()
+  // without starting a new bundle.
+  public async bundleActions<T>(desc: string|null, callback: () => T|Promise<T>,
+                                options: {nestInActiveBundle?: boolean} = {}): Promise<T> {
+    if (options.nestInActiveBundle && this._bundlesPending) {
       return await callback();
+    }
+    const bundlingInfo = this.startBundlingActions<T>({
+      description: desc,
+      shouldIncludeInBundle: () => true,
+      prepare: callback,
+      finalize: async () => undefined,
+    });
+    try {
+      return await bundlingInfo.preparePromise;
     } finally {
-      this.stopBundlingActions();
+      bundlingInfo.triggerFinalize();
     }
   }
 
@@ -123,19 +179,18 @@ export class DocData extends BaseDocData {
     const eventData = {actions};
     this.sendActionsEmitter.emit(eventData);
     const options = { desc: optDesc };
-    const bundleCallback = this._bundleCallback;
-    if (bundleCallback) {
-      actions.forEach(action => {
-        if (!bundleCallback(action)) {
-          gristNotify(`Attempted to add invalid action to current bundle: ${action}.`);
-        }
-      });
+    if (this._shouldIncludeInBundle && !this._shouldIncludeInBundle(actions)) {
+      this._triggerBundleFinalize?.();
+      await this._lastBundlePromise;
+    }
+    if (this._bundlesPending) {
       defaults(options, {
         desc: this._nextDesc,
         linkId: this._lastActionNum,
       });
       this._nextDesc = null;
     }
+
     const result: ApplyUAResult = await this._bundleSender.applyUserActions(actions, options);
     this._lastActionNum = result.actionNum;
     this.sendActionsDoneEmitter.emit(eventData);
@@ -183,3 +238,36 @@ class BundleSender {
     return this._sendPromise;
   }
 }
+
+
+/**
+ * Options to startBundlingAction().
+ */
+export interface BundlingOptions<T = unknown> {
+  // Description of the action bundle.
+  description: string|null;
+
+  // Checker for whether an action belongs in the current bundle. If not, finalize() will be
+  // called immediately. Note that this checker is NOT applied for actions sent from prepare()
+  // or finalize() callbacks, only those in between.
+  shouldIncludeInBundle: (actions: UserAction[]) => boolean;
+
+  // Callback to start this action bundle.
+  prepare: () => T|Promise<T>;
+
+  // Callback to finalize this action bundle.
+  finalize: () => Promise<void>;
+}
+
+/**
+ * Result of startBundlingActions(), to allow waiting for prepare() to complete, and to trigger
+ * finalize() manually.
+ */
+export interface BundlingInfo<T = unknown> {
+  // Promise for when the prepare() has completed. Note that sometimes it's delayed until the
+  // previous bundle has been finalized.
+  preparePromise: Promise<T>;
+
+  // Ask DocData to call the finalize callback immediately.
+  triggerFinalize: () => void;
+}

app/client/models/entities/ViewFieldRec.ts

@@ -150,7 +150,7 @@ export function createViewFieldRec(this: ViewFieldRec, docModel: DocModel): void
         this._fieldOrColumn().visibleCol.saveOnly(colRef),
         this._fieldOrColumn().saveDisplayFormula(colRef ? `$${this.colId()}.${col.colId()}` : '')
       ]);
-    })
+    }, {nestInActiveBundle: this.column.peek().isTransforming.peek()})
   );
 
   // The display column to use for the field, or the column itself when no displayCol is set.

app/client/widgets/FieldBuilder.ts

@@ -289,7 +289,7 @@ export class FieldBuilder extends Disposable {
                dom.onDispose(() => {
                  // When losing focus, if there's an active column transform, finalize it.
                  if (this.columnTransform) {
-                   this.columnTransform.finalize();
+                   this.columnTransform.finalize().catch(reportError);
                  }
                }),
                kf.row(
@@ -453,7 +453,7 @@ export class FieldBuilder extends Disposable {
     // If the user attempts to edit a value during transform, finalize (i.e. cancel or execute)
     // the transform.
     if (this.columnTransform) {
-      this.columnTransform.finalize();
+      this.columnTransform.finalize().catch(reportError);
       return;
     }
 

app/server/lib/ActiveDoc.ts

@@ -14,7 +14,6 @@ import remove = require('lodash/remove');
 import zipObject = require('lodash/zipObject');
 import * as moment from 'moment-timezone';
 import * as tmp from 'tmp';
-import * as util from 'util';
 
 import {getEnvContent, LocalActionBundle} from 'app/common/ActionBundle';
 import {SandboxActionBundle, UserActionBundle} from 'app/common/ActionBundle';
@@ -1072,7 +1071,7 @@ export class ActiveDoc extends EventEmitter {
     const result: ApplyUAResult = await new Promise<ApplyUAResult>(
       (resolve, reject) =>
         this._sharing!.addUserAction({action, client, resolve, reject}));
-    this.logDebug(docSession, "_applyUserActions returning %s", util.inspect(result));
+    this.logDebug(docSession, "_applyUserActions returning %s", shortDesc(result));
 
     if (result.isModification) {
       this._fetchCache.clear();  // This could be more nuanced.