Use external IDs for stable selection tracking. (#10314)

app/gui2/e2e/edgeInteractions.spec.ts

@@ -18,7 +18,7 @@ async function initGraph(page: Page) {
 const EDGE_PARTS = 2
 
 /**
-  Scenario: We disconnect the `sum` parameter in the `prod` node by clicking on the edge and pressing the delete key.
+  Scenario: We disconnect the `sum` parameter in the `prod` node by clicking on the edge and clicking on the background.
  */
 test('Disconnect an edge from a port', async ({ page }) => {
   await initGraph(page)
@@ -32,7 +32,6 @@ test('Disconnect an edge from a port', async ({ page }) => {
     force: true,
   })
   await page.mouse.click(500, -500)
-  await page.keyboard.press('Delete')
   await expect(await edgesToNodeWithBinding(page, 'sum')).toHaveCount(EDGE_PARTS)
 })
 

app/gui2/src/components/GraphEditor.vue

@@ -38,6 +38,7 @@ import { provideInteractionHandler } from '@/providers/interactionHandler'
 import { provideKeyboard } from '@/providers/keyboard'
 import { provideWidgetRegistry } from '@/providers/widgetRegistry'
 import { provideGraphStore, type NodeId } from '@/stores/graph'
+import { asNodeId } from '@/stores/graph/graphDatabase'
 import type { RequiredImport } from '@/stores/graph/imports'
 import { provideProjectStore } from '@/stores/project'
 import { provideSuggestionDbStore } from '@/stores/suggestionDatabase'
@@ -206,11 +207,11 @@ const nodeSelection = provideGraphSelection(
   graphNavigator,
   graphStore.nodeRects,
   graphStore.isPortEnabled,
-  (id) => graphStore.db.nodeIdToNode.has(id),
   {
-    onSelected(id) {
-      graphStore.db.moveNodeToTop(id)
-    },
+    isValid: (id) => graphStore.db.nodeIdToNode.has(id),
+    pack: (id) => graphStore.db.nodeIdToNode.get(id)?.rootExpr.externalId,
+    unpack: (eid) => asNodeId(graphStore.db.idFromExternal(eid)),
+    onSelected: (id) => graphStore.db.moveNodeToTop(id),
   },
 )
 

app/gui2/src/composables/__tests__/selection.test.ts

@@ -14,8 +14,7 @@ function selectionWithMockData(sceneMousePos?: Ref<Vec2>) {
   rects.set(3, Rect.FromBounds(1, 20, 10, 30))
   rects.set(4, Rect.FromBounds(20, 20, 30, 30))
   const navigator = proxyRefs({ sceneMousePos: sceneMousePos ?? ref(Vec2.Zero), scale: 1 })
-  const allNodesValid = () => true
-  const selection = useSelection(navigator, rects, 0, allNodesValid)
+  const selection = useSelection(navigator, rects)
   selection.setSelection(new Set([1, 2]))
   return selection
 }

app/gui2/src/composables/selection.ts

@@ -3,50 +3,110 @@ import { selectionMouseBindings } from '@/bindings'
 import { useEvent, usePointer } from '@/composables/events'
 import type { PortId } from '@/providers/portInfo.ts'
 import { type NodeId } from '@/stores/graph'
+import { filter, filterDefined, map } from '@/util/data/iterable'
 import type { Rect } from '@/util/data/rect'
 import { intersectionSize } from '@/util/data/set'
 import type { Vec2 } from '@/util/data/vec2'
 import { dataAttribute, elementHierarchy } from '@/util/dom'
 import * as set from 'lib0/set'
-import { filter } from 'shared/util/data/iterable'
 import { computed, ref, shallowReactive, shallowRef } from 'vue'
 
+interface BaseSelectionOptions<T> {
+  margin?: number
+  isValid?: (element: T) => boolean
+  onSelected?: (element: T) => void
+  onDeselected?: (element: T) => void
+}
+interface SelectionPackingOptions<T, PackedT> {
+  /** The `pack` and `unpack` functions are used to maintain state in a transformed form.
+   *
+   * If provided, all operations that modify or query state will transparently operate on packed state. This can be
+   * used to expose a selection interface based on one element type (`T`), while allowing the selection set to be
+   * maintained using a more stable element type (`PackedT`).
+   *
+   * For example, the selection can expose a `NodeId` API, while internally storing `ExternalId`s.
+   */
+  pack: (element: T) => PackedT | undefined
+  unpack: (packed: PackedT) => T | undefined
+}
+export type SelectionOptions<T, PackedT> =
+  | BaseSelectionOptions<T>
+  | (BaseSelectionOptions<T> & SelectionPackingOptions<T, PackedT>)
+
 export function useSelection<T>(
   navigator: { sceneMousePos: Vec2 | null; scale: number },
   elementRects: Map<T, Rect>,
-  margin: number,
-  isValid: (element: T) => boolean,
-  callbacks: {
-    onSelected?: (element: T) => void
-    onDeselected?: (element: T) => void
-  } = {},
+  options?: BaseSelectionOptions<T>,
+): UseSelection<T, T>
+export function useSelection<T, PackedT>(
+  navigator: { sceneMousePos: Vec2 | null; scale: number },
+  elementRects: Map<T, Rect>,
+  options: BaseSelectionOptions<T> & SelectionPackingOptions<T, PackedT>,
+): UseSelection<T, PackedT>
+export function useSelection<T, PackedT>(
+  navigator: { sceneMousePos: Vec2 | null; scale: number },
+  elementRects: Map<T, Rect>,
+  options: SelectionOptions<T, PackedT> = {},
+): UseSelection<T, PackedT> {
+  const BASE_DEFAULTS: Required<BaseSelectionOptions<T>> = {
+    margin: 0,
+    isValid: () => true,
+    onSelected: () => {},
+    onDeselected: () => {},
+  }
+  const PACKING_DEFAULTS: SelectionPackingOptions<T, T> = {
+    pack: (element: T) => element,
+    unpack: (packed: T) => packed,
+  }
+  return useSelectionImpl(
+    navigator,
+    elementRects,
+    { ...BASE_DEFAULTS, ...options },
+    'pack' in options ? options
+      // The function signature ensures that if a `pack` function is not provided, PackedT = T.
+    : (PACKING_DEFAULTS as unknown as SelectionPackingOptions<T, PackedT>),
+  )
+}
+
+type UseSelection<T, PackedT> = ReturnType<typeof useSelectionImpl<T, PackedT>>
+function useSelectionImpl<T, PackedT>(
+  navigator: { sceneMousePos: Vec2 | null; scale: number },
+  elementRects: Map<T, Rect>,
+  { margin, isValid, onSelected, onDeselected }: Required<BaseSelectionOptions<T>>,
+  { pack, unpack }: SelectionPackingOptions<T, PackedT>,
 ) {
   const anchor = shallowRef<Vec2>()
   let initiallySelected = new Set<T>()
   // Selection, including elements that do not (currently) pass `isValid`.
-  const rawSelected = shallowReactive(new Set<T>())
+  const rawSelected = shallowReactive(new Set<PackedT>())
 
-  const selected = computed(() => set.from(filter(rawSelected, isValid)))
+  const unpackedRawSelected = computed(() => set.from(filterDefined(map(rawSelected, unpack))))
+  const selected = computed(() => set.from(filter(unpackedRawSelected.value, isValid)))
   const isChanging = computed(() => anchor.value != null)
   const committedSelection = computed(() =>
     isChanging.value ? set.from(filter(initiallySelected, isValid)) : selected.value,
   )
 
   function readInitiallySelected() {
-    initiallySelected = set.from(rawSelected)
+    initiallySelected = unpackedRawSelected.value
   }
 
   function setSelection(newSelection: Set<T>) {
-    for (const id of newSelection)
-      if (!rawSelected.has(id)) {
-        rawSelected.add(id)
-        callbacks.onSelected?.(id)
+    for (const id of newSelection) {
+      const packed = pack(id)
+      if (packed != null && !rawSelected.has(packed)) {
+        rawSelected.add(packed)
+        onSelected(id)
       }
-    for (const id of rawSelected)
-      if (!newSelection.has(id)) {
-        rawSelected.delete(id)
-        callbacks.onDeselected?.(id)
+    }
+
+    for (const packed of rawSelected) {
+      const id = unpack(packed)
+      if (id == null || !newSelection.has(id)) {
+        rawSelected.delete(packed)
+        if (id != null) onDeselected(id)
       }
+    }
   }
 
   function execAdd() {
@@ -143,10 +203,16 @@ export function useSelection<T>(
     // === Selected nodes ===
     selected,
     selectAll: () => {
-      for (const id of elementRects.keys()) rawSelected.add(id)
+      for (const id of elementRects.keys()) {
+        const packed = pack(id)
+        if (packed) rawSelected.add(packed)
+      }
     },
     deselectAll: () => rawSelected.clear(),
-    isSelected: (element: T) => rawSelected.has(element),
+    isSelected: (element: T) => {
+      const packed = pack(element)
+      return packed != null && rawSelected.has(packed)
+    },
     committedSelection,
     setSelection,
     // === Selection changes ===

app/gui2/src/providers/graphSelection.ts

@@ -1,8 +1,9 @@
 import type { NavigatorComposable } from '@/composables/navigator'
-import { useGraphHover, useSelection } from '@/composables/selection'
+import { useGraphHover, useSelection, type SelectionOptions } from '@/composables/selection'
 import { createContextStore } from '@/providers'
 import { type NodeId } from '@/stores/graph'
 import type { Rect } from '@/util/data/rect'
+import type { ExternalId } from 'shared/yjsModel'
 import { proxyRefs } from 'vue'
 
 const SELECTION_BRUSH_MARGIN_PX = 6
@@ -14,14 +15,10 @@ const { provideFn, injectFn } = createContextStore(
     navigator: NavigatorComposable,
     nodeRects: Map<NodeId, Rect>,
     isPortEnabled,
-    isValid: (id: NodeId) => boolean,
-    callbacks: {
-      onSelected?: (id: NodeId) => void
-      onDeselected?: (id: NodeId) => void
-    } = {},
+    options: SelectionOptions<NodeId, ExternalId>,
   ) =>
     proxyRefs({
-      ...useSelection(navigator, nodeRects, SELECTION_BRUSH_MARGIN_PX, isValid, callbacks),
+      ...useSelection(navigator, nodeRects, { margin: SELECTION_BRUSH_MARGIN_PX, ...options }),
       ...useGraphHover(isPortEnabled),
     }),
 )

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

@@ -488,8 +488,10 @@ export class GraphDb {
 
 declare const brandNodeId: unique symbol
 export type NodeId = AstId & { [brandNodeId]: never }
-export function asNodeId(id: Ast.AstId): NodeId {
-  return id as NodeId
+export function asNodeId(id: Ast.AstId): NodeId
+export function asNodeId(id: Ast.AstId | undefined): NodeId | undefined
+export function asNodeId(id: Ast.AstId | undefined): NodeId | undefined {
+  return id != null ? (id as NodeId) : undefined
 }
 
 export interface NodeDataFromAst {