Fix regression with DiffableMaps and duplicate agglomerate skeletons

CHANGELOG.unreleased.md

@@ -46,6 +46,7 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released
 - Fixed a bug where merging annotations with large tree IDs could lead to an error. [#8643](https://github.com/scalableminds/webknossos/pull/8643)
 - Fixed that the segment stats were sometimes not displayed in the context menu. [#8645](https://github.com/scalableminds/webknossos/pull/8645)
 - Fixed a bug in zarr streaming where directly after the datastore startup, chunk responses would have status 404 (leading zarr clients to fill with fill_value). Now it will yield status 503, so that clients can retry or escalate this as an error. [#8644](https://github.com/scalableminds/webknossos/pull/8644)
+- Fixed regression which caused the import of trees (also of agglomerate skeletons) to crash if the annotation was not empty. [#8656](https://github.com/scalableminds/webknossos/pull/8656)
 
 ### Removed
 - The old "Selective Segment Visibility" feature that allowed to only see the active and the hovered segment was removed. From now on the visibility of segments can be controlled with checkboxes in the segment list and with the "Hide unlisted segments" toggle in the layer settings. [#8546](https://github.com/scalableminds/webknossos/pull/8546)

frontend/javascripts/test/fixtures/hybridtracing_object.ts

@@ -30,7 +30,7 @@ const colorLayer: APIColorLayer = {
   additionalAxes: [],
 };
 
-const initalTreeOne: Tree = {
+export const initialTreeOne: Tree = {
   treeId: 1,
   name: "TestTree",
   nodes: new DiffableMap(),
@@ -67,7 +67,7 @@ export const initialSkeletonTracing: SkeletonTracing = {
   createdTimestamp: 0,
   tracingId: "tracingId",
   trees: new TreeMap([
-    [1, initalTreeOne],
+    [1, initialTreeOne],
     [2, initialTreeTwo],
   ]),
   treeGroups: [],

frontend/javascripts/test/reducers/skeletontracing_reducer.spec.ts

@@ -9,12 +9,19 @@ import { TreeTypeEnum, type Vector3 } from "viewer/constants";
 import { enforceSkeletonTracing } from "viewer/model/accessors/skeletontracing_accessor";
 import {
   initialState as defaultState,
+  initialTreeOne,
   initialSkeletonTracing,
 } from "test/fixtures/hybridtracing_object";
 import SkeletonTracingReducer from "viewer/model/reducers/skeletontracing_reducer";
 import * as SkeletonTracingActions from "viewer/model/actions/skeletontracing_actions";
 import { max } from "viewer/model/helpers/iterator_utils";
-import { type Node, TreeMap, type MutableNode, type Tree } from "viewer/model/types/tree_types";
+import {
+  type Node,
+  TreeMap,
+  type MutableNode,
+  type Tree,
+  MutableTreeMap,
+} from "viewer/model/types/tree_types";
 
 const initialState: WebknossosState = update(defaultState, {
   annotation: {
@@ -1784,4 +1791,50 @@ describe("SkeletonTracing", () => {
     expect(trees.getOrThrow(3).isVisible).toBe(true);
     expect(trees.getOrThrow(4).isVisible).toBe(true);
   });
+
+  it("should add a new tree with addTreesAndGroupsAction (without relabeling)", () => {
+    expect(initialState.annotation.skeleton?.trees.size()).toBe(2);
+    let ids: number[] | null = null;
+    const treeIdCallback = (_ids: number[]) => (ids = _ids);
+
+    const state = applyActions(initialState, [
+      // Id-relabeling is only done if the skeleton is not empty.
+      // Therefore, delete the two existing trees so that the new tree will
+      // be added without id-relabeling.
+      SkeletonTracingActions.deleteTreeAction(1),
+      SkeletonTracingActions.deleteTreeAction(2),
+      SkeletonTracingActions.addTreesAndGroupsAction(
+        new MutableTreeMap([[4, { ...initialTreeOne, treeId: 4 }]]),
+        [],
+        treeIdCallback,
+      ),
+    ]);
+
+    // The original tree id 4 should have survived.
+    expect(ids).toEqual([4]);
+    expect(state.annotation.skeleton?.trees.size()).toBe(1);
+    expect(state.annotation.skeleton?.trees.getNullable(4)).toBeDefined();
+    expect(state.annotation.skeleton?.trees.getNullable(4)?.treeId).toBe(4);
+  });
+
+  it("should add a new tree with addTreesAndGroupsAction (with id relabeling)", () => {
+    expect(initialState.annotation.skeleton?.trees.size()).toBe(2);
+    let ids: number[] | null = null;
+    const treeIdCallback = (_ids: number[]) => (ids = _ids);
+
+    const state = applyActions(initialState, [
+      SkeletonTracingActions.addTreesAndGroupsAction(
+        new MutableTreeMap([[8_000_000, { ...initialTreeOne, treeId: 8_000_000 }]]),
+        [],
+        treeIdCallback,
+      ),
+    ]);
+
+    // The original tree id 8_000_000 should have been changed to the next free id (3).
+    expect(ids).toEqual([3]);
+    expect(state.annotation.skeleton?.trees.size()).toBe(3);
+    expect(state.annotation.skeleton?.trees.getNullable(3)).toBeDefined();
+    expect(state.annotation.skeleton?.trees.getNullable(3)?.treeId).toBe(3);
+    expect(state.annotation.skeleton?.trees.getNullable(8_000_000)).toBeUndefined();
+  });
 });

frontend/javascripts/viewer/model/reducers/skeletontracing_reducer_helpers.ts

@@ -595,9 +595,8 @@ export function addTreesAndGroups(
     }
   }
 
-  // Materialize with toArray because we are mutating
-  // the collection within the loop.
-  for (const tree of trees.values().toArray()) {
+  const newTrees = new MutableTreeMap();
+  for (const tree of trees.values()) {
     const newNodes: MutableNodeMap = new DiffableMap();
 
     for (const node of tree.nodes.values()) {
@@ -632,11 +631,11 @@ export function addTreesAndGroups(
     tree.groupId = tree.groupId != null ? groupIdMap[tree.groupId] : tree.groupId;
     tree.treeId = newTreeId;
 
-    trees.mutableSet(newTreeId, tree);
+    newTrees.mutableSet(newTreeId, tree);
     newTreeId++;
   }
 
-  return [trees, treeGroups, newNodeId - 1];
+  return [newTrees, treeGroups, newNodeId - 1];
 }
 
 export function deleteTrees(