Update persisted idmap when metadata changes (#11761)

Fixes #11729

The issue was quite rare indeed: it happened only when somehow nodes had idmap and metadata cleared from the file, and then only metadata change was applied.

In this case, because there was no idMap change, idMapToPerstist was also empty; metadata were stored with proper ids, but those ids weren't assigned to any span.

CHANGELOG.md

@@ -47,6 +47,8 @@
 - [Pasting tabular data now creates Table.input expressions][11695].
 - [No halo is displayed around components when hovering][11715].
 - [The hover area of the component output port extended twice its size][11715].
+- [Fixed a rare bug where the component position wasn't persisted after closing
+  project][11761]
 - [In the table visualization and table widget, the table context menu can now
   be opened on OS X][11755].
 - [Fix some UI elements drawing on top of visualization toolbar dropdown
@@ -81,6 +83,7 @@
 [11684]: https://github.com/enso-org/enso/pull/11684
 [11695]: https://github.com/enso-org/enso/pull/11695
 [11715]: https://github.com/enso-org/enso/pull/11715
+[11761]: https://github.com/enso-org/enso/pull/11761
 [11768]: https://github.com/enso-org/enso/pull/11768
 
 #### Enso Standard Library

app/gui/integration-test/project-view/tableVisualisation.spec.ts

@@ -34,6 +34,9 @@ test('Load Table Visualisation', async ({ page }) => {
 })
 
 test('Copy/paste from Table Visualization', async ({ page, context }) => {
+  const expectClipboard = expect.poll(() =>
+    page.evaluate(() => window.navigator.clipboard.readText()),
+  )
   await context.grantPermissions(['clipboard-read', 'clipboard-write'])
   await actions.goToGraph(page)
 
@@ -47,8 +50,7 @@ test('Copy/paste from Table Visualization', async ({ page, context }) => {
 
   // Copy from table visualization
   await page.keyboard.press(`${CONTROL_KEY}+C`)
-  let clipboardContent = await page.evaluate(() => window.navigator.clipboard.readText())
-  expect(clipboardContent).toMatch(/^0,0\t0,1\r\n1,0\t1,1\r\n2,0\t2,1$/)
+  await expectClipboard.toMatch(/^0,0\t0,1\r\n1,0\t1,1\r\n2,0\t2,1$/)
 
   // Paste to Node.
   await actions.clickAtBackground(page)
@@ -82,8 +84,7 @@ test('Copy/paste from Table Visualization', async ({ page, context }) => {
   await node.getByText('2,1').hover()
   await page.mouse.up()
   await page.keyboard.press(`${CONTROL_KEY}+C`)
-  clipboardContent = await page.evaluate(() => window.navigator.clipboard.readText())
-  expect(clipboardContent).toMatch(/^0,0\t0,1\r\n1,0\t1,1\r\n2,0\t2,1$/)
+  await expectClipboard.toMatch(/^0,0\t0,1\r\n1,0\t1,1\r\n2,0\t2,1$/)
 
   // Copy from table input widget with headers
   await node.getByText('0,0').hover()
@@ -93,8 +94,7 @@ test('Copy/paste from Table Visualization', async ({ page, context }) => {
   await page.mouse.down({ button: 'right' })
   await page.mouse.up({ button: 'right' })
   await page.getByText('Copy with Headers').click()
-  clipboardContent = await page.evaluate(() => window.navigator.clipboard.readText())
-  expect(clipboardContent).toMatch(/^Column #1\tColumn #2\r\n0,0\t0,1\r\n1,0\t1,1\r\n2,0\t2,1$/)
+  await expectClipboard.toMatch(/^Column #1\tColumn #2\r\n0,0\t0,1\r\n1,0\t1,1\r\n2,0\t2,1$/)
 })
 
 async function expectTableInputContent(page: Page, node: Locator) {

app/ydoc-server/src/edits.ts

@@ -38,6 +38,7 @@ const MAX_SIZE_FOR_NORMAL_DIFF = 30000
 interface AppliedUpdates {
   newCode: string | undefined
   newIdMap: IdMap | undefined
+  newPersistedIdMap: IdMap | undefined
   newMetadata: fileFormat.IdeMetadata | undefined
 }
 
@@ -62,17 +63,14 @@ export function applyDocumentUpdates(
   }
 
   let newIdMap = undefined
+  let newPersistedIdMap = undefined
   let newCode = undefined
   let newMetadata: fileFormat.IdeMetadata | undefined = undefined
 
   const syncModule = new MutableModule(doc.ydoc)
   const root = syncModule.root()
   assert(root != null)
-  if (codeChanged || idsChanged || synced.idMapJson == null) {
-    const { code, info } = printWithSpans(root)
-    if (codeChanged) newCode = code
-    newIdMap = spanMapToIdMap(info)
-  }
+
   if (codeChanged || idsChanged || metadataChanged) {
     // Update the metadata object.
     // Depth-first key order keeps diffs small.
@@ -99,7 +97,28 @@ export function applyDocumentUpdates(
     })
   }
 
-  return { newCode, newIdMap, newMetadata }
+  if (codeChanged || idsChanged || metadataChanged || synced.idMapJson == null) {
+    const { code, info } = printWithSpans(root)
+    if (codeChanged) newCode = code
+    const idMap = spanMapToIdMap(info)
+    if (codeChanged || idsChanged || synced.idMapJson == null) newIdMap = idMap
+    newPersistedIdMap = newMetadata && getIdMapToPersist(idMap, newMetadata)
+  }
+
+  return { newCode, newIdMap, newPersistedIdMap, newMetadata }
+}
+
+/**
+ * Get the subset of idMap which should be persisted in file.
+ */
+export function getIdMapToPersist(
+  idMap: IdMap,
+  metadata: fileFormat.IdeMetadata,
+): IdMap | undefined {
+  const entriesIntersection = idMap
+    .entries()
+    .filter(([, id]) => id in metadata.node || id in (metadata.widget ?? {}))
+  return new IdMap(entriesIntersection)
 }
 
 function translateVisualizationToFile(
@@ -163,7 +182,7 @@ export function translateVisualizationFromFile(
  * A simplified diff algorithm.
  *
  * The `fast-diff` package uses Myers' https://neil.fraser.name/writing/diff/myers.pdf with some
- * optimizations to generate minimal diff. Unfortunately, event this algorithm is still too slow
+ * optimizations to generate minimal diff. Unfortunately, even this algorithm is still too slow
  * for our metadata. Therefore we need to use faster algorithm which will not produce theoretically
  * minimal diff.
  *

app/ydoc-server/src/languageServerSession.ts

@@ -36,6 +36,7 @@ import * as Y from 'yjs'
 import {
   applyDiffAsTextEdits,
   applyDocumentUpdates,
+  getIdMapToPersist,
   prettyPrintDiff,
   translateVisualizationFromFile,
 } from './edits'
@@ -463,26 +464,12 @@ class ModulePersistence extends ObservableV2<{ removed: () => void }> {
     const moduleUpdate = syncModule.applyUpdate(update, 'remote')
     if (moduleUpdate && this.syncedContent) {
       const synced = splitFileContents(this.syncedContent)
-      const { newCode, newIdMap, newMetadata } = applyDocumentUpdates(
+      const { newCode, newIdMap, newPersistedIdMap, newMetadata } = applyDocumentUpdates(
         this.doc,
         synced,
         moduleUpdate,
       )
-      this.sendLsUpdate(synced, newCode, newIdMap, newMetadata)
-    }
-  }
-
-  private static getIdMapToPersist(
-    idMap: IdMap | undefined,
-    metadata: fileFormat.IdeMetadata,
-  ): IdMap | undefined {
-    if (idMap === undefined) {
-      return
-    } else {
-      const entriesIntersection = idMap
-        .entries()
-        .filter(([, id]) => id in metadata.node || id in (metadata.widget ?? {}))
-      return new IdMap(entriesIntersection)
+      this.sendLsUpdate(synced, newCode, newIdMap, newPersistedIdMap, newMetadata)
     }
   }
 
@@ -508,6 +495,7 @@ class ModulePersistence extends ObservableV2<{ removed: () => void }> {
     synced: EnsoFileParts,
     newCode: string | undefined,
     newIdMap: IdMap | undefined,
+    newPersistedIdMap: IdMap | undefined,
     newMetadata: fileFormat.IdeMetadata | undefined,
   ) {
     if (this.syncedContent == null || this.syncedVersion == null) return
@@ -525,10 +513,7 @@ class ModulePersistence extends ObservableV2<{ removed: () => void }> {
           ...newSnapshot,
         },
       })
-    const idMapToPersist =
-      (newIdMap || newMetadata) &&
-      ModulePersistence.getIdMapToPersist(newIdMap, newMetadata ?? this.syncedMeta.ide)
-    const newIdMapToPersistJson = idMapToPersist && serializeIdMap(idMapToPersist)
+    const newIdMapToPersistJson = newPersistedIdMap && serializeIdMap(newPersistedIdMap)
     const code = newCode ?? synced.code
     const newContent = combineFileParts({
       code,
@@ -713,6 +698,7 @@ class ModulePersistence extends ObservableV2<{ removed: () => void }> {
         contentsReceived,
         this.syncedCode ?? undefined,
         unsyncedIdMap,
+        this.syncedMeta && getIdMapToPersist(unsyncedIdMap, this.syncedMeta.ide),
         this.syncedMeta?.ide,
       )
   }

app/ydoc-shared/src/ast/tree.ts

@@ -162,7 +162,23 @@ export abstract class Ast {
     return this.fields.get('id')
   }
 
-  /** TODO: Add docs */
+  /**
+   * Get an id used to communication with engine.
+   *
+   * This id has two purposes:
+   * - It is used in communication with Engine (regarding visualization, execution context frames, etc.)
+   * - This is used as key files metadata (keeping e.g. node's positions)
+   *
+   * ## Id vs. externalId
+   *
+   * It subtly differs in meaning from standard id: while the latter keeps identity from ydoc conflict resolution
+   * perspective, this id rather try to keep what is the node or widget identity from user perspective. The best
+   * example showing this difference is adding a new argument, like changing  `foo = bar` to `foo = bar a`:
+   * - `id` should be kept on `Main.bar` expression, and `Main.bar a` gets new id, so any concurrent edit on `bar`
+   *   will be handled properly
+   * - `externalId` should be moved from `bar` to `bar a` expression, as this is still the same node (so its
+   *   position and other properties should be kept)
+   */
   get externalId(): ExternalId {
     const id = this.fields.get('metadata').get('externalId')
     assert(id != null)