Fix undoing node removal (#9561)

Fixes #9314

The node deletion does not remove AST node from the module, only unpin it from its parent; so undoing does not add this node, just modify it, and thus we weren't informed about metadata change.

app/gui2/e2e/undoRedo.spec.ts

@@ -0,0 +1,52 @@
+import test, { type Locator, type Page } from 'playwright/test'
+import * as actions from './actions'
+import { expect } from './customExpect'
+import { mockMethodCallInfo } from './expressionUpdates'
+import * as locate from './locate'
+
+test('Adding new node', async ({ page }) => {
+  await actions.goToGraph(page)
+
+  const nodesCount = await locate.graphNode(page).count()
+  await locate.addNewNodeButton(page).click()
+  await expect(locate.componentBrowserInput(page)).toBeVisible()
+  await page.keyboard.insertText('foo')
+  await page.keyboard.press('Control+Enter')
+  await expect(locate.graphNode(page)).toHaveCount(nodesCount + 1)
+  await expect(locate.graphNode(page).last().locator('.WidgetToken')).toHaveText(['foo'])
+  const newNodeBBox = await locate.graphNode(page).last().boundingBox()
+
+  await page.keyboard.press('Control+Z')
+  await expect(locate.graphNode(page)).toHaveCount(nodesCount)
+  await expect(
+    locate.graphNode(page).locator('.WidgetToken').filter({ hasText: 'foo' }),
+  ).toHaveCount(0)
+
+  await page.keyboard.press('Control+Shift+Z')
+  await expect(locate.graphNode(page)).toHaveCount(nodesCount + 1)
+  await expect(locate.graphNode(page).last().locator('.WidgetToken')).toHaveText(['foo'])
+  const restoredBox = await locate.graphNode(page).last().boundingBox()
+  await expect(restoredBox).toEqual(newNodeBBox)
+})
+
+test('Removing node', async ({ page }) => {
+  await actions.goToGraph(page)
+
+  const nodesCount = await locate.graphNode(page).count()
+  const deletedNode = locate.graphNodeByBinding(page, 'final')
+  const deletedNodeBBox = await deletedNode.boundingBox()
+  await deletedNode.click()
+  await page.keyboard.press('Delete')
+  await expect(locate.graphNode(page)).toHaveCount(nodesCount - 1)
+
+  await page.keyboard.press('Control+Z')
+  await expect(locate.graphNode(page)).toHaveCount(nodesCount)
+  await expect(deletedNode.locator('.WidgetToken')).toHaveText(['Main', '.', 'func1', 'prod'])
+  await expect(deletedNode.locator('.GraphNodeComment')).toHaveText('This node can be entered')
+  const restoredBBox = await deletedNode.boundingBox()
+  await expect(restoredBBox).toEqual(deletedNodeBBox)
+
+  await page.keyboard.press('Control+Shift+Z')
+  await expect(locate.graphNode(page)).toHaveCount(nodesCount - 1)
+  await expect(deletedNode).not.toBeVisible()
+})

app/gui2/shared/ast/mutableModule.ts

@@ -369,7 +369,6 @@ class UpdateBuilder {
 
   addNode(id: AstId) {
     this.nodesAdded.add(id)
-    this.updateAllFields(id)
   }
 
   updateAllFields(id: AstId) {
@@ -404,7 +403,9 @@ class UpdateBuilder {
   }
 
   finish(): ModuleUpdate {
-    const updateRoots = subtreeRoots(this.module, new Set(this.nodesUpdated.keys()))
+    const dirtyNodes = new Set(this.nodesUpdated)
+    this.nodesAdded.forEach((node) => dirtyNodes.add(node))
+    const updateRoots = subtreeRoots(this.module, dirtyNodes)
     return { ...this, updateRoots }
   }
 }

app/gui2/shared/yjsModel.ts

@@ -132,17 +132,18 @@ export class DistributedModule {
   }
 }
 
-export const localOrigins = ['local', 'local:CodeEditor'] as const
-export type LocalOrigin = (typeof localOrigins)[number]
-export type Origin = LocalOrigin | 'remote'
+export const localUserActionOrigins = ['local:userAction', 'local:userAction:CodeEditor'] as const
+export type LocalUserActionOrigin = (typeof localUserActionOrigins)[number]
+export type Origin = LocalUserActionOrigin | 'remote' | 'local:autoLayout'
 /** Locally-originated changes not otherwise specified. */
-export const defaultLocalOrigin: LocalOrigin = 'local'
-export function isLocalOrigin(origin: string): origin is LocalOrigin {
-  const localOriginNames: readonly string[] = localOrigins
+export const defaultLocalOrigin: LocalUserActionOrigin = 'local:userAction'
+export function isLocalUserActionOrigin(origin: string): origin is LocalUserActionOrigin {
+  const localOriginNames: readonly string[] = localUserActionOrigins
   return localOriginNames.includes(origin)
 }
 export function tryAsOrigin(origin: string): Origin | undefined {
-  if (isLocalOrigin(origin)) return origin
+  if (isLocalUserActionOrigin(origin)) return origin
+  if (origin === 'local:autoLayout') return origin
   if (origin === 'remote') return origin
 }
 

app/gui2/src/components/CodeEditor.vue

@@ -212,7 +212,7 @@ function commitPendingChanges() {
   if (!pendingChanges || !currentModule) return
   try {
     currentModule.applyTextEdits(changeSetToTextEdits(pendingChanges), graphStore.viewModule)
-    graphStore.commitEdit(currentModule, undefined, 'local:CodeEditor')
+    graphStore.commitEdit(currentModule, undefined, 'local:userAction:CodeEditor')
   } catch (error) {
     console.error(`Code Editor failed to modify module`, error)
     resetView()
@@ -264,7 +264,7 @@ function observeSourceChange(textEdits: SourceRangeEdit[], origin: Origin | unde
     return
   }
   // When we aren't in the `needResync` state, we can ignore updates that originated in the Code Editor.
-  if (origin === 'local:CodeEditor') return
+  if (origin === 'local:userAction:CodeEditor') return
   if (pendingChanges) {
     console.info(`Deferring update (editor dirty).`)
     needResync = true

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

@@ -348,19 +348,11 @@ export class GraphDb {
       const node = this.nodeIdToNode.get(nodeId)
       currentNodeIds.add(nodeId)
       if (node == null) {
-        let metadataFields: NodeDataFromMetadata = {
-          position: new Vec2(0, 0),
-          vis: undefined,
-        }
-        // We are notified of new or changed metadata by `updateMetadata`, so we only need to read existing metadata
-        // when we switch to a different function.
-        if (functionChanged) {
-          const nodeMeta = newNode.rootExpr.nodeMetadata
-          const pos = nodeMeta.get('position') ?? { x: 0, y: 0 }
-          metadataFields = {
-            position: new Vec2(pos.x, pos.y),
-            vis: nodeMeta.get('visualization'),
-          }
+        const nodeMeta = newNode.rootExpr.nodeMetadata
+        const pos = nodeMeta.get('position') ?? { x: 0, y: 0 }
+        const metadataFields = {
+          position: new Vec2(pos.x, pos.y),
+          vis: nodeMeta.get('visualization'),
         }
         this.nodeIdToNode.set(nodeId, { ...newNode, ...metadataFields, zIndex: this.highestZIndex })
       } else {

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

@@ -33,7 +33,12 @@ import { iteratorFilter } from 'lib0/iterator'
 import { defineStore } from 'pinia'
 import { SourceDocument } from 'shared/ast/sourceDocument'
 import type { ExpressionUpdate, StackItem } from 'shared/languageServerTypes'
-import type { LocalOrigin, SourceRangeKey, VisualizationMetadata } from 'shared/yjsModel'
+import type {
+  LocalUserActionOrigin,
+  Origin,
+  SourceRangeKey,
+  VisualizationMetadata,
+} from 'shared/yjsModel'
 import { defaultLocalOrigin, sourceRangeKey, visMetadataEquals } from 'shared/yjsModel'
 import {
   computed,
@@ -134,8 +139,13 @@ export const useGraphStore = defineStore('graph', () => {
       id: AstId
       changes: NodeMetadata
     }[]
-    const dirtyNodeSet = new Set(update.nodesUpdated)
-    if (moduleChanged || dirtyNodeSet.size !== 0) {
+    const dirtyNodeSet = new Set(
+      (function* () {
+        yield* update.nodesUpdated
+        yield* update.nodesAdded
+      })(),
+    )
+    if (moduleChanged || dirtyNodeSet.size !== 0 || update.nodesDeleted.size !== 0) {
       db.updateExternalIds(root)
       toRaw = new Map()
       visitRecursive(Ast.parseEnso(moduleSource.text), (node) => {
@@ -459,7 +469,7 @@ export const useGraphStore = defineStore('graph', () => {
         )
         nodeRects.set(nodeId, new Rect(position, rect.size))
       }
-    })
+    }, 'local:autoLayout')
   })
 
   function updateVizRect(id: NodeId, rect: Rect | undefined) {
@@ -542,7 +552,7 @@ export const useGraphStore = defineStore('graph', () => {
   function commitEdit(
     edit: MutableModule,
     skipTreeRepair?: boolean,
-    origin: LocalOrigin = defaultLocalOrigin,
+    origin: LocalUserActionOrigin = defaultLocalOrigin,
   ) {
     const root = edit.root()
     if (!(root instanceof Ast.BodyBlock)) {
@@ -578,9 +588,9 @@ export const useGraphStore = defineStore('graph', () => {
     return result!
   }
 
-  function batchEdits(f: () => void) {
+  function batchEdits(f: () => void, origin: Origin = defaultLocalOrigin) {
     assert(syncModule.value != null)
-    syncModule.value.transact(f, 'local')
+    syncModule.value.transact(f, origin)
   }
 
   function editNodeMetadata(ast: Ast.Ast, f: (metadata: Ast.MutableNodeMetadata) => void) {

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

@@ -36,7 +36,12 @@ import type {
   StackItem,
   VisualizationConfiguration,
 } from 'shared/languageServerTypes'
-import { DistributedProject, localOrigins, type ExternalId, type Uuid } from 'shared/yjsModel'
+import {
+  DistributedProject,
+  localUserActionOrigins,
+  type ExternalId,
+  type Uuid,
+} from 'shared/yjsModel'
 import {
   computed,
   markRaw,
@@ -539,7 +544,7 @@ export const useProjectStore = defineStore('project', () => {
     const moduleName = projectModel.findModuleByDocId(guid)
     if (moduleName == null) return null
     const mod = await projectModel.openModule(moduleName)
-    for (const origin of localOrigins) mod?.undoManager.addTrackedOrigin(origin)
+    for (const origin of localUserActionOrigins) mod?.undoManager.addTrackedOrigin(origin)
     return mod
   })
 

app/gui2/ydoc-server/edits.ts

@@ -46,7 +46,7 @@ export function applyDocumentUpdates(
   synced: EnsoFileParts,
   update: ModuleUpdate,
 ): AppliedUpdates {
-  const codeChanged = update.nodesUpdated.size !== 0
+  const codeChanged = update.nodesUpdated.size && update.nodesAdded.size && update.nodesDeleted.size
   let idsChanged = false
   let metadataChanged = false
   for (const { changes } of update.metadataUpdated) {