Fix deletion-failure bug (#10888) (#10889)

Fix intermittent deletion failures (#10888)

- Fix bug in deletion logic (the `syncModule` itself was passed to a function that tried to commit it as an edit)
- Refactor APIs to avoid similar bugs (remove `direct` edit option)

# Important Notes
- `graphStore.getMutable` provides safer access to some of the functionality that was exposed by `direct` editing.
- `graphStore.transact` has been eliminated; it was redundant with `graphStore.batchEdits`.

app/gui2/src/components/ColorPickerMenu.vue

@@ -24,7 +24,7 @@ const editedNodeInitialColors = new Map<NodeId, string | undefined>()
 
 function setColor(color: string | undefined) {
   currentColor.value = color
-  graphStore.transact(() => {
+  graphStore.batchEdits(() => {
     if (color) {
       for (const node of selection.selected) {
         if (!editedNodeInitialColors.has(node))

app/gui2/src/components/GraphEditor.vue

@@ -336,7 +336,7 @@ const graphBindingsHandler = graphBindings.handler({
       selected,
       (id) => graphStore.db.nodeIdToNode.get(id)?.vis?.visible === true,
     )
-    graphStore.transact(() => {
+    graphStore.batchEdits(() => {
       for (const nodeId of selected) {
         graphStore.setNodeVisualization(nodeId, { visible: !allVisible })
       }

app/gui2/src/components/GraphEditor/GraphEdges.vue

@@ -54,7 +54,7 @@ function edgeInteractionClick(graphNavigator: GraphNavigator) {
   }
   const target = graph.mouseEditedEdge.target ?? selection?.hoveredPort
   const targetNode = target && graph.getPortNodeId(target)
-  graph.transact(() => {
+  graph.batchEdits(() => {
     if (source != null && sourceNode != targetNode) {
       if (target == null) {
         if (graph.mouseEditedEdge?.disconnectedEdgeTarget != null)

app/gui2/src/composables/astDocumentation.ts

@@ -11,12 +11,7 @@ export function useAstDocumentation(graphStore: GraphStore, ast: ToValue<Ast | u
         const astValue = toValue(ast)
         if (!astValue) return
         if (value.trimStart() !== '') {
-          graphStore.edit(
-            (edit) =>
-              edit.getVersion(astValue).getOrInitDocumentation().setDocumentationText(value),
-            true,
-            true,
-          )
+          graphStore.getMutable(astValue).getOrInitDocumentation().setDocumentationText(value)
         } else {
           // Remove the documentation node.
           const documented = astValue.documentingAncestor()

app/gui2/src/stores/graph/index.ts

@@ -300,8 +300,7 @@ export const { injectFn: useGraphStore, provideFn: provideGraphStore } = createC
     }
 
     function deleteNodes(ids: Iterable<NodeId>) {
-      edit(
-        (edit) => {
+      edit((edit) => {
         for (const id of ids) {
           const node = db.nodeIdToNode.get(id)
           if (!node) continue
@@ -313,10 +312,7 @@ export const { injectFn: useGraphStore, provideFn: provideGraphStore } = createC
           nodeRects.delete(id)
           nodeHoverAnimations.delete(id)
         }
-        },
-        true,
-        true,
-      )
+      })
     }
 
     function setNodeContent(
@@ -346,10 +342,6 @@ export const { injectFn: useGraphStore, provideFn: provideGraphStore } = createC
       })
     }
 
-    function transact(fn: () => void) {
-      syncModule.value!.transact(fn)
-    }
-
     const undoManager = {
       undo() {
         proj.module?.undoManager.undo()
@@ -542,11 +534,9 @@ export const { injectFn: useGraphStore, provideFn: provideGraphStore } = createC
      *
      *  @param skipTreeRepair - If the edit is certain not to produce incorrect or non-canonical syntax, this may be set
      *  to `true` for better performance.
-     *  @param direct - Apply all changes directly to the synchronized module; they will be committed even if the callback
-     *  exits by throwing an exception.
      */
-    function edit<T>(f: (edit: MutableModule) => T, skipTreeRepair?: boolean, direct?: boolean): T {
-      const edit = direct ? syncModule.value : syncModule.value?.edit()
+    function edit<T>(f: (edit: MutableModule) => T, skipTreeRepair?: boolean): T {
+      const edit = syncModule.value?.edit()
       assert(edit != null)
       let result
       edit.transact(() => {
@@ -556,11 +546,18 @@ export const { injectFn: useGraphStore, provideFn: provideGraphStore } = createC
           assert(root instanceof Ast.BodyBlock)
           Ast.repair(root, edit)
         }
-        if (!direct) syncModule.value!.applyEdit(edit)
+        syncModule.value!.applyEdit(edit)
       })
       return result!
     }
 
+    /** Obtain a version of the given `Ast` for direct mutation. The `ast` must exist in the current module.
+     *  This can be more efficient than creating and committing an edit, but skips tree-repair and cannot be aborted.
+     */
+    function getMutable<T extends Ast.Ast>(ast: T): Ast.Mutable<T> {
+      return syncModule.value!.getVersion(ast)
+    }
+
     function batchEdits(f: () => void, origin: Origin = defaultLocalOrigin) {
       assert(syncModule.value != null)
       syncModule.value.transact(f, origin)
@@ -691,7 +688,6 @@ export const { injectFn: useGraphStore, provideFn: provideGraphStore } = createC
     })
 
     return proxyRefs({
-      transact,
       db: markRaw(db),
       mockExpressionUpdate,
       editedNodeInfo,
@@ -710,6 +706,7 @@ export const { injectFn: useGraphStore, provideFn: provideGraphStore } = createC
       pickInCodeOrder,
       ensureCorrectNodeOrder,
       batchEdits,
+      getMutable,
       overrideNodeColor,
       getNodeColorOverride,
       setNodeContent,