fixing an undefined case

[saranrapjs]

I'm not sure that I have a clear description of when exactly this bug takes places, because it takes place so deep into the history of documents where we've recorded steps that it's hard to describe exactly what's happening that leads to this, but not infrequently the let next = inserted[j] line will be undefined, which leads to error later on within this undo cleanup loop. This fix guards against that. I'm realizing that we actually patched this previously, but didn't do so on the prototype, so it's possible this change got lost in the transition to the public module!

I was trying to take a stab at developing a test case for this (trying to understand the find test harness) but in the interest of time, I figured I'd send this patch your way in case the test case would be relatively simple and I'm just missing something!

[marijnh]

Ah, I screwed up with that loop index magic brinkmanship and the inner loop would sometimes break out while leaving an index at an incorrect position. Attached patch fixes it. Could you verify that it helps?

[saranrapjs]

That definitely does the trick! Presume this'll get published out as a version bump?

[marijnh]

Yes, I've released 1.0.1

[saranrapjs]

I've found another related case of this, but am unable to come up with a great test-case for it. Here's some hopefully helpful evidence I've found:

This sameEnd condition happens, where the nextJ value is zero, and so the nextJ becomes -1, which breaks on the next iteration of the loop.

At that moment, the deleted[i] looks like this:

DeletedSpan {
  from: 1014,
  to: 1033,
  data:
   [ Change {
       step: [Object],
       userId: 464,
       timestamp: 1507826480302,
       id: 715,
       isPersisted: true } ],
  pos: 969,
  slice:
   Slice {
     content: Fragment { content: [Array], size: 19 },
     openStart: 0,
     openEnd: 0 } }

And the inserted[j] looks like this:

Span {
  from: 968,
  to: 969,
  data:
   [ Change {
       step: [Object],
       userId: 464,
       timestamp: 1507826481569,
       id: 716,
       isPersisted: true } ] }

...happy to collect further info if it feels relevant. A simple undefined check on next seems to fix the bug, but that may create an incorrect result in some way I don't understand... Also: lemme know if you'd rather move this into an issue, sorry to be contorting a pull request for this purpose!

[marijnh]

where the nextJ value is zero

That shouldn't happen, but I did figure out a case where it does. Does patch 2fc94e7 solve this for you?

(That loop is a horror, with all the splicing and side effects it does, but since this is the inner loop of a rather demanding piece of computation, I tried to avoid unnecessary allocations.)

[saranrapjs]

That seems to fix the bug we were seeing!

[saranrapjs]

(I presume that this patch will get published as well?)

[marijnh]

It's been released as 1.0.2