From 0c80547e0d3f1f482e32678e6367493d84f6ead6 Mon Sep 17 00:00:00 2001
From: Alex Hall <alex.mojaki@gmail.com>
Date: Wed, 19 Jan 2022 14:41:04 +0200
Subject: [PATCH] (core) Change UndoStack._linkMap to store an array of action
 groups for each linkId

Summary:
While working on type conversion, I ran into a bug when multiple action groups belonging to a bundle are quickly sent. `DocData._sendActionsImpl` can set the same `linkId: this._lastActionNum` on multiple action groups before `_lastActionNum` is updated with a result from the server. Only one of these groups gets saved in `UndoStack._linkMap` so undoing a bundle misses out on some action groups.

This diff associates each `linkId` key with an array of action groups instead of just one, then combines them all together when undoing.

Test Plan: I've confirmed that this fixes my problem within my type conversion diff, but I haven't found a way to reproduce the general problem in master. I think the existing tests are probably fine since undo is tested extensively everywhere, but I'd also like to see if there are existing bugs which this fixes.

Reviewers: georgegevoian

Reviewed By: georgegevoian

Differential Revision: https://phab.getgrist.com/D3223
---
 app/client/components/UndoStack.ts | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/app/client/components/UndoStack.ts b/app/client/components/UndoStack.ts
index 09edcdd5..9a1e2640 100644
--- a/app/client/components/UndoStack.ts
+++ b/app/client/components/UndoStack.ts
@@ -2,9 +2,10 @@ import {CursorPos} from 'app/client/components/Cursor';
 import {GristDoc} from 'app/client/components/GristDoc';
 import * as dispose from 'app/client/lib/dispose';
 import {MinimalActionGroup} from 'app/common/ActionGroup';
-import {PromiseChain} from 'app/common/gutil';
+import {PromiseChain, setDefault} from 'app/common/gutil';
 import {fromKo, Observable} from 'grainjs';
 import * as ko from 'knockout';
+import sortBy = require('lodash/sortBy');
 
 export interface ActionGroupWithCursorPos extends MinimalActionGroup {
   cursorPos?: CursorPos;
@@ -27,7 +28,7 @@ export class UndoStack extends dispose.Disposable {
   private _gristDoc: GristDoc;
   private _stack: ActionGroupWithCursorPos[];
   private _pointer: number;
-  private _linkMap: {[actionNum: number]: MinimalActionGroup};
+  private _linkMap: Map<number, MinimalActionGroup[]>;
 
   // Chain of promises which send undo actions to the server. This delays the execution of the
   // next action until the current one has been received and moved the pointer index.
@@ -43,7 +44,7 @@ export class UndoStack extends dispose.Disposable {
     this._pointer = 0;
 
     // Map leading from actionNums to the action groups which link to them.
-    this._linkMap = {};
+    this._linkMap = new Map();
 
     // Observables for when there is nothing to undo/redo.
     this.undoDisabledObs = ko.observable(true);
@@ -74,7 +75,7 @@ export class UndoStack extends dispose.Disposable {
 
     if (ag.linkId) {
       // Link action. Add the action to the linkMap, but not to any stacks.
-      this._linkMap[ag.linkId] = ag;
+      setDefault(this._linkMap, ag.linkId, []).push(ag);
     } else if (otherIndex > -1) {
       // Undo/redo action from the current session.
       this._pointer = ag.isUndo ? otherIndex : otherIndex + 1;
@@ -136,13 +137,18 @@ export class UndoStack extends dispose.Disposable {
   private _findActionBundle(ag: MinimalActionGroup) {
     const prevNums = new Set();
     const actionGroups = [];
+    const queue = [ag];
     // Follow references through the linkMap adding items to the array bundle.
-    while (ag && !prevNums.has(ag.actionNum)) {
+    while (queue.length) {
+      ag = queue.pop()!;
       // Checking that actions are only accessed once prevents an infinite circular loop.
+      if (prevNums.has(ag.actionNum)) {
+        break;
+      }
       actionGroups.push(ag);
       prevNums.add(ag.actionNum);
-      ag = this._linkMap[ag.actionNum];
+      queue.push(...this._linkMap.get(ag.actionNum) || []);
     }
-    return actionGroups;
+    return sortBy(actionGroups, group => group.actionNum);
   }
 }