From ff1c157a86faeb6d0478d8cd39823790d88872be Mon Sep 17 00:00:00 2001 From: Danila Poyarkov Date: Fri, 11 Sep 2026 10:28:15 +0300 Subject: [PATCH] refactor(fig): clarify imported instance reconciliation --- packages/core/src/clipboard.ts | 3 +- packages/core/src/kiwi/fig/import.ts | 2 +- packages/fig/src/node-change/index.ts | 2 +- .../node-change/instance-linkage/index.ts} | 10 ++- packages/scene-graph/src/instances.ts | 73 ++++++++----------- .../import-nodes/instance-linkage.test.ts | 29 +++++++- .../instance-sync-duplication.test.ts | 3 +- 7 files changed, 68 insertions(+), 54 deletions(-) rename packages/{core/src/kiwi/fig/import-linkage.ts => fig/src/node-change/instance-linkage/index.ts} (89%) diff --git a/packages/core/src/clipboard.ts b/packages/core/src/clipboard.ts index 1000d200b..371129666 100644 --- a/packages/core/src/clipboard.ts +++ b/packages/core/src/clipboard.ts @@ -4,6 +4,7 @@ import { populateAndApplyOverrides } from '@open-pencil/fig/instance-overrides' import type { InstanceNodeChange } from '@open-pencil/fig/instance-overrides' import { nodeChangeToProps, + linkImportedInstanceChildren, shouldImportTextAsAutoSize, sortChildren } from '@open-pencil/fig/node-change' @@ -12,8 +13,6 @@ import type { GUID, NodeChange as KiwiNodeChange } from '@open-pencil/kiwi/fig/c import { decodeBinarySchema, compileSchema, ByteBuffer } from '@open-pencil/kiwi/schema-runtime' import type { SceneGraph, SceneNode } from '@open-pencil/scene-graph' -import { linkImportedInstanceChildren } from '#core/kiwi/fig/import-linkage' - import { decodeBase64, decodeBase64Text, encodeBase64, encodeBase64Text } from './bytes' import { shapeTextForClipboard } from './canvas/text/clipboard' import { diff --git a/packages/core/src/kiwi/fig/import.ts b/packages/core/src/kiwi/fig/import.ts index 98a5b7747..b8954aaea 100644 --- a/packages/core/src/kiwi/fig/import.ts +++ b/packages/core/src/kiwi/fig/import.ts @@ -8,6 +8,7 @@ import { getOpenPencilPluginValue, guidToString, importCanvasGuides, + linkImportedInstanceChildren, nodeChangeToProps, shouldImportTextAsAutoSize, sortChildren, @@ -23,7 +24,6 @@ import type { } from '@open-pencil/scene-graph' import { BLACK } from '#core/constants' -import { linkImportedInstanceChildren } from '#core/kiwi/fig/import-linkage' import { setLazyFigImportContext } from '#core/kiwi/fig/lazy-import' type AssetRef = { key: string; version?: string } diff --git a/packages/fig/src/node-change/index.ts b/packages/fig/src/node-change/index.ts index 115023802..3c347ccb0 100644 --- a/packages/fig/src/node-change/index.ts +++ b/packages/fig/src/node-change/index.ts @@ -7,7 +7,7 @@ export * from './export-node' export * from './export-runtime' export * from './font/features' export * from './font/style' -export * from './font/variations' +export * from './instance-linkage' export * from './paint' export * from './path/commands' export * from './plugin-data' diff --git a/packages/core/src/kiwi/fig/import-linkage.ts b/packages/fig/src/node-change/instance-linkage/index.ts similarity index 89% rename from packages/core/src/kiwi/fig/import-linkage.ts rename to packages/fig/src/node-change/instance-linkage/index.ts index 82e1d6785..06aad56e5 100644 --- a/packages/core/src/kiwi/fig/import-linkage.ts +++ b/packages/fig/src/node-change/instance-linkage/index.ts @@ -67,11 +67,15 @@ function linkSubtree( // without stable keys, and for legacy files without overrideKey). const remainingComp = compChildren.filter((c) => !linkedComp.has(c.id)) const remainingInst = instChildren.filter((c) => !linkedInst.has(c.id)) - const count = Math.min(remainingComp.length, remainingInst.length) - for (let i = 0; i < count; i++) { + // Positional evidence is safe only when every remaining slot has a matching type. + // Otherwise preserve all unmatched instance children and let sync clone missing ones. + const compatible = + remainingComp.length === remainingInst.length && + remainingComp.every((child, index) => child.type === remainingInst[index]?.type) + if (!compatible) return + for (let i = 0; i < remainingComp.length; i++) { const compChild = remainingComp[i] const instChild = remainingInst[i] - if (compChild.type !== instChild.type) continue linkPair(compChild, instChild) } } diff --git a/packages/scene-graph/src/instances.ts b/packages/scene-graph/src/instances.ts index 5adeed8e0..d238824da 100644 --- a/packages/scene-graph/src/instances.ts +++ b/packages/scene-graph/src/instances.ts @@ -150,26 +150,30 @@ function matchFallbackChildren( instChildMap: Map, usedInstChildIds: Set ): void { - const unmatchedCompChildIds = compParent.childIds.filter((id) => !instChildMap.has(id)) - const unmatchedInstChildren = instParent.childIds - .map((id) => graph.nodes.get(id)) - .filter((n): n is SceneNode => n !== undefined && !usedInstChildIds.has(n.id)) + const fallbackByType = new Map>() + for (const childId of instParent.childIds) { + const child = graph.nodes.get(childId) + if (!child || usedInstChildIds.has(child.id)) continue + let byName = fallbackByType.get(child.type) + if (!byName) { + byName = new Map() + fallbackByType.set(child.type, byName) + } + const queue = byName.get(child.name) + if (queue) queue.push(child) + else byName.set(child.name, [child]) + } - if (unmatchedCompChildIds.length === 0 || unmatchedInstChildren.length === 0) return - - // Match by name and type with forward (FIFO) iteration to preserve sibling order. - for (const compChildId of unmatchedCompChildIds) { + // Match by name and type with FIFO queues to preserve sibling order in linear time. + for (const compChildId of compParent.childIds) { + if (instChildMap.has(compChildId)) continue const compChild = graph.nodes.get(compChildId) if (!compChild) continue - const matchIdx = unmatchedInstChildren.findIndex( - (instChild) => instChild.type === compChild.type && instChild.name === compChild.name - ) - if (matchIdx !== -1) { - const [instChild] = unmatchedInstChildren.splice(matchIdx, 1) - instChildMap.set(compChildId, instChild) - usedInstChildIds.add(instChild.id) - linkMatchedChild(overrides, instParentId, instChild, compChildId) - } + const match = fallbackByType.get(compChild.type)?.get(compChild.name)?.shift() + if (!match) continue + instChildMap.set(compChildId, match) + usedInstChildIds.add(match.id) + linkMatchedChild(overrides, instParentId, match, compChildId) } } @@ -183,31 +187,18 @@ function sortInstanceChildren( const orderMap = new Map() for (let i = 0; i < compChildOrder.length; i++) orderMap.set(compChildOrder[i], i) - // Children whose resolved mapping (sourceComponentId override, else componentId) - // is one of this component's children sort in component order; children with no - // such mapping (e.g. imported extras or user-added nodes inside the instance) - // sort to the END rather than the front, keeping their relative order among - // themselves. This keeps component children in canonical order instead of yanking - // unmapped extras to the front on every sync. - instParent.childIds.sort((a, b) => { - const nodeA = graph.nodes.get(a) - const nodeB = graph.nodes.get(b) - const sourceA = nodeA - ? getInstanceOverride(overrides, instParentId, nodeA.id, 'sourceComponentId') + const ranks = new Map() + for (let index = 0; index < instParent.childIds.length; index++) { + const childId = instParent.childIds[index] + const node = graph.nodes.get(childId) + const source = node + ? getInstanceOverride(overrides, instParentId, node.id, 'sourceComponentId') : undefined - const sourceB = nodeB - ? getInstanceOverride(overrides, instParentId, nodeB.id, 'sourceComponentId') - : undefined - - const mappedA = typeof sourceA === 'string' ? sourceA : nodeA?.componentId - const mappedB = typeof sourceB === 'string' ? sourceB : nodeB?.componentId - const idxA = mappedA ? (orderMap.get(mappedA) ?? -1) : -1 - const idxB = mappedB ? (orderMap.get(mappedB) ?? -1) : -1 - if (idxA === -1 && idxB === -1) return 0 - if (idxA === -1) return 1 - if (idxB === -1) return -1 - return idxA - idxB - }) + const mapped = typeof source === 'string' ? source : node?.componentId + const componentIndex = mapped ? orderMap.get(mapped) : undefined + ranks.set(childId, componentIndex ?? compChildOrder.length + index) + } + instParent.childIds.sort((left, right) => (ranks.get(left) ?? 0) - (ranks.get(right) ?? 0)) } /** True when syncing `compParentId` into `instParentId` would form a cycle. */ diff --git a/tests/engine/editor/clipboard/import-nodes/instance-linkage.test.ts b/tests/engine/editor/clipboard/import-nodes/instance-linkage.test.ts index 314cebeef..6ad23cd15 100644 --- a/tests/engine/editor/clipboard/import-nodes/instance-linkage.test.ts +++ b/tests/engine/editor/clipboard/import-nodes/instance-linkage.test.ts @@ -1,7 +1,9 @@ import { describe, expect, it } from 'bun:test' -import { importClipboardNodes, SceneGraph } from '@open-pencil/core' +import { importClipboardNodes } from '@open-pencil/core' import type { NodeChange } from '@open-pencil/core' +import { linkImportedInstanceChildren } from '@open-pencil/fig/node-change' +import { SceneGraph } from '@open-pencil/scene-graph' import { getNodeOrThrow } from '#tests/helpers/assert' @@ -85,9 +87,7 @@ describe('importClipboardNodes: instance child linkage', () => { expect(instChild.height).toBe(50) }) - it('does not mis-link an extra same-type child inserted before a renamed serialized child (overrideKey)', async () => { - const { linkImportedInstanceChildren } = await import('#core/kiwi/fig/import-linkage') - const { SceneGraph } = await import('@open-pencil/scene-graph') + it('does not mis-link an extra same-type child inserted before a renamed serialized child (overrideKey)', () => { const graph = new SceneGraph() const page = graph.addPage('Test') @@ -135,4 +135,25 @@ describe('importClipboardNodes: instance child linkage', () => { expect(inst.childIds[0]).toBe(renamed.id) expect(inst.childIds[1]).toBe(extra.id) }) + + it('leaves ambiguous positional children unmapped when cardinality differs', () => { + const graph = new SceneGraph() + const page = graph.addPage('Test') + const component = graph.createNode('COMPONENT', page.id, { name: 'Card' }) + graph.createNode('FRAME', component.id, { name: 'Header' }) + const instance = graph.createNode('INSTANCE', page.id, { + name: 'Card', + componentId: component.id + }) + const extra = graph.createNode('FRAME', instance.id, { name: 'Badge' }) + const serialized = graph.createNode('FRAME', instance.id, { name: 'Header v2' }) + + linkImportedInstanceChildren(graph, new Set([instance.id])) + + expect(extra.componentId).toBeNull() + expect(serialized.componentId).toBeNull() + graph.syncInstances(component.id) + expect(instance.childIds).toHaveLength(3) + expect(instance.childIds).toEqual(expect.arrayContaining([extra.id, serialized.id])) + }) }) diff --git a/tests/engine/scene-graph/instance-sync-duplication.test.ts b/tests/engine/scene-graph/instance-sync-duplication.test.ts index 97684637d..94ebabadb 100644 --- a/tests/engine/scene-graph/instance-sync-duplication.test.ts +++ b/tests/engine/scene-graph/instance-sync-duplication.test.ts @@ -1,9 +1,8 @@ import { describe, expect, test } from 'bun:test' +import { linkImportedInstanceChildren } from '@open-pencil/fig/node-change' import { SceneGraph, recordInstanceOverride } from '@open-pencil/scene-graph' -import { linkImportedInstanceChildren } from '#core/kiwi/fig/import-linkage' - describe('instance synchronization child deduplication', () => { test('syncInstances on instance with imported children without componentId does not duplicate children', () => { const graph = new SceneGraph()