Removing multiple panes during a rerender causes a runtime error

[Gelio]

Hey! Thanks for a great library, it's a pleasure to use!

I encountered a runtime exception when I removed multiple panes during a single rerender:

Analysis

I believe the problematic piece of code is this loop that calls removeView for each removed pane:

allotment/src/allotment.tsx

Lines 297 to 303 in cd38c27

	exit.forEach((flag, index) => {
	if (flag) {
	splitViewRef.current?.removeView(index);
	panes.splice(index, 1);
	views.current.splice(index, 1);
	}
	});

I believe the problem is with the order of removal. If we have 3 panes (indices [0, 1, 2]) and remove panes with indices 1 and 2 during a single render, then this loop calls removeView(1) and then removeView(2). The second call fails because after the first removeView(1) there are only 2 panes (with indices [0, 1]). If the order of operations was reversed and we removed panes from last to first, we would call removeView(2) first and then removeView(1), which would work as expected.

Workarouund

The workaround that I came up with is to remove one pane at a time in a very quick succession to give those useEffects time to refresh the indices.

Instead of just

const onClick = () => {
  setExtraPanesCount(0);
};

I now use:

const onClick = () => {
  const removePane = () => {
    setExtraPanesCount((panes) => {
      if (panes > 1) {
        setTimeout(removePane, 0);
      }
      return panes - 1;
    });
  };

  removePane();
};

which works well.

it would be ideal if I could remove all the panes at once but that requires fixing this bug.

[johnwalley]

Hi @Gelio, thanks for the detailed issue.

I'll prioritise getting this fixed as throwing an exception for a normal piece of functionality isn't great. I'll ping you when the release with the fix goes out.

[Gelio]

Thanks for a quick response! Don't worry about it too much, the workaround I described works well enough that it is a minor nuisance for me now. Take your time 🙂

[johnwalley]

I've created a PR (#320) with a fix 🙂. I also have a PR (#321) I'm working on which adds end-to-end tests which should help catch this type of error in the future.

I'll get them merged and published sometime this week.

[Gelio]

Thanks for the effort, I appreciate that you came up with a fix so soon 👍 I'm looking forward to having the fix released, but there's no rush, so take your time