Repository: vigetlabs/microcosm
Pull request #335 (2017-05-24 16:37:58)

Fix bugs in history.remove
**What kind of change does this PR introduce?** (check at least one)

- [x] Bugfix

**Does this PR introduce a breaking change?** (check one)

- [x] No

If yes, please describe the impact and migration path for existing applications:

**Does this PR fulfill these requirements:**

- [x] All tests are passing

**Additional Information**

This PR fixes two bugs:
- If you remove an action which has no `next` reference, History assumed you were removing the head action and automatically set the head to the action's parent. This ensures that the head is reset only if the action being removed was the existing head.
- If you remove the head action, set the head to the removed action's parent


Patch:
diff --git a/src/history.js b/src/history.js
index 523bb038..552c991d 100644
--- a/src/history.js
+++ b/src/history.js
@@ -179,21 +179,28 @@ class History extends Emitter {
       return
     }
 
-    let next = action.next
+    // cache linking references and activeness
     let parent = action.parent
+    let next = action.next
+    let wasActive = this.isActive(action)
 
     this.clean(action)
 
+    // if there are no more actions left, we're done
     if (this.size <= 0) {
       this.begin()
       return
-    } else if (!next) {
+    }
+
+    // reset head/root references if necessary
+    if (action === this.head) {
       next = this.head = parent
     } else if (action === this.root) {
       this.root = next
     }
 
-    if (!action.disabled) {
+    // reconcile history if action was in active history branch
+    if (wasActive && !action.disabled) {
       this.reconcile(next)
     }
   }
@@ -297,6 +304,14 @@ class History extends Emitter {
 
     this.size = size
   }
+
+  /**
+   * Determine if provided action is within active history branch
+   * @param {Action} action
+   */
+  isActive(action) {
+    return this.toArray().indexOf(action) >= 0
+  }
 }
 
 export default History


Test patch:
diff --git a/test/unit/history/isActive.test.js b/test/unit/history/isActive.test.js
new file mode 100644
index 00000000..8aa6096f
--- /dev/null
+++ b/test/unit/history/isActive.test.js
@@ -0,0 +1,36 @@
+import History from '../../../src/history'
+
+describe('History::isActive', function() {
+  const action = n => n
+  const history = new History()
+
+  /*
+   * Set up the following tree:
+   *
+   *               |- [three] - [four]
+   * [one] - [two] +
+   *               |- [five] - [*six]
+   */
+
+  const one = history.append(action)
+  const two = history.append(action)
+  const three = history.append(action)
+  const four = history.append(action)
+
+  history.checkout(two)
+
+  const five = history.append(action)
+  const six = history.append(action)
+
+  it('returns true for actions in the active branch', function() {
+    expect(history.isActive(one)).toEqual(true)
+    expect(history.isActive(two)).toEqual(true)
+    expect(history.isActive(five)).toEqual(true)
+    expect(history.isActive(six)).toEqual(true)
+  })
+
+  it('returns false for actions not in the active branch', function() {
+    expect(history.isActive(three)).toEqual(false)
+    expect(history.isActive(four)).toEqual(false)
+  })
+})
diff --git a/test/unit/history/remove.test.js b/test/unit/history/remove.test.js
index 3cbeb6f3..365756a4 100644
--- a/test/unit/history/remove.test.js
+++ b/test/unit/history/remove.test.js
@@ -68,8 +68,6 @@ describe('History::remove', function() {
       history.append(function two() {}, 'resolve')
       let three = history.append(function three() {}, 'resolve')
 
-      history.archive()
-
       history.remove(three)
 
       expect(history.map(a => a.command.name)).toEqual(['$start', 'one', 'two'])
@@ -82,8 +80,6 @@ describe('History::remove', function() {
       history.append(function two() {}, 'resolve')
       let three = history.append(function three() {}, 'resolve')
 
-      history.archive()
-
       history.remove(three)
 
       expect(history.head.next).toBe(null)
@@ -113,8 +109,6 @@ describe('History::remove', function() {
       history.append(function two() {}, 'resolve')
       history.append(function three() {}, 'resolve')
 
-      history.archive()
-
       history.remove(history.root)
 
       expect(history.map(a => a.command.name)).toEqual(['one', 'two', 'three'])
@@ -129,8 +123,6 @@ describe('History::remove', function() {
       let two = history.append(function two() {}, 'resolve')
       history.append(function three() {}, 'resolve')
 
-      history.archive()
-
       history.remove(two)
 
       expect(history.map(a => a.command.name)).toEqual([
@@ -139,6 +131,86 @@ describe('History::remove', function() {
         'three'
       ])
     })
+
+    it('resets the head if removing the head', function() {
+      let history = new History({ maxHistory: Infinity })
+
+      let one = history.append(function one() {}, 'resolve')
+      let two = history.append(function two() {}, 'resolve')
+      history.append(function three() {}, 'resolve')
+
+      history.checkout(two)
+      history.remove(two)
+
+      expect(history.head.id).toBe(one.id)
+    })
+
+    it('reconciles at the next action', function() {
+      let history = new History({ maxHistory: Infinity })
+
+      history.append(function one() {}, 'resolve')
+      let two = history.append(function two() {}, 'resolve')
+      let three = history.append(function three() {}, 'resolve')
+
+      jest.spyOn(history, 'reconcile')
+
+      history.remove(two)
+
+      expect(history.reconcile).toHaveBeenCalledWith(three)
+    })
+
+    it('reconciles at the parent if the action is head of an active branch', function() {
+      let history = new History({ maxHistory: Infinity })
+
+      let one = history.append(function one() {}, 'resolve')
+      let two = history.append(function two() {}, 'resolve')
+      history.append(function three() {}, 'resolve')
+
+      history.checkout(two)
+
+      jest.spyOn(history, 'reconcile')
+
+      history.remove(two)
+
+      expect(history.reconcile).toHaveBeenCalledWith(one)
+    })
+
+    it('does not reconcile if the action is not in active branch', function() {
+      let history = new History({ maxHistory: Infinity })
+
+      history.append(function one() {}, 'resolve')
+      let two = history.append(function two() {}, 'resolve')
+      let three = history.append(function three() {}, 'resolve')
+
+      history.checkout(two)
+
+      jest.spyOn(history, 'reconcile')
+
+      history.remove(three)
+
+      expect(history.reconcile).not.toHaveBeenCalled()
+    })
+  })
+
+  describe('removing an unfocused branch terminator', function() {
+    it('leaves the head reference alone', function() {
+      let history = new History({ maxHistory: Infinity })
+
+      let one = history.append(function one() {}, 'resolve')
+      let two = history.append(function two() {}, 'resolve')
+
+      history.checkout(one)
+      let three = history.append(function three() {}, 'resolve')
+
+      // History tree now looks like this:
+      //                |- [two]
+      // [root] - [one] +
+      //                |- [*three]
+
+      history.remove(two)
+
+      expect(history.head.id).toBe(three.id)
+    })
   })
 
   describe('children', function() {