From 33962b764c05a09eda6e57c338555116a4eab72f Mon Sep 17 00:00:00 2001 From: Danila Poyarkov Date: Wed, 19 Aug 2026 18:05:25 +0300 Subject: [PATCH] fix(fig): harden component property round trips - Allocate collision-safe property GUIDs across every export path - Preserve instance-swap component keys and unresolved GUID targets - Prefer edited assignments over imported raw metadata - Cover recursive serialization, assignments, and key preservation --- CHANGELOG.md | 1 + packages/core/src/io/formats/fig/export.ts | 56 +++++++++ packages/core/src/kiwi/fig/import.ts | 14 +-- packages/fig/src/node-change/export-node.ts | 36 +++--- packages/fig/src/node-change/serialize.ts | 2 +- packages/fig/tests/export.test.ts | 109 ++++++++++++++++-- .../fig/export/component-properties.test.ts | 43 +++++++ .../io/fig/import/component-props.test.ts | 10 +- .../io/fig/instance-swap-roundtrip.test.ts | 25 +++- 9 files changed, 257 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index df1a3bd32..c7f970025 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,7 @@ ### Fixed +- Preserve app-created component properties and instance-swap targets across `.fig` save and reload cycles. (#548) - Reconnect desktop automation to an already-running MCP server by allowing access to its discovery file. (#546) - Keep text-editing carets, hit testing, and selection highlights aligned with vertically centered or bottom-aligned text. (#539) - Match AI chat code-block syntax colors and backgrounds to the active light or dark theme. (#537) diff --git a/packages/core/src/io/formats/fig/export.ts b/packages/core/src/io/formats/fig/export.ts index 08497f764..267563456 100644 --- a/packages/core/src/io/formats/fig/export.ts +++ b/packages/core/src/io/formats/fig/export.ts @@ -163,6 +163,51 @@ function assignVariableGuids( } } +interface ComponentPropertyGuidState { + ids: string[] + maxLocalId0: number + maxLocalId1: number +} + +function collectComponentPropertyGuidState(graph: SceneGraph): ComponentPropertyGuidState { + const ids = new Set() + let maxLocalId0 = 0 + let maxLocalId1 = 0 + for (const node of graph.getAllNodes()) { + for (const definition of node.componentPropertyDefinitions) ids.add(definition.id) + for (const reference of node.componentPropertyReferences) ids.add(reference.propertyId) + for (const propertyId of Object.keys(node.componentPropertyAssignments)) ids.add(propertyId) + for (const spec of node.variantPropSpecs) ids.add(spec.propDefId) + } + for (const propertyId of ids) { + const match = /^(\d+):(\d+)$/.exec(propertyId) + if (!match) continue + const sessionID = Number.parseInt(match[1], 10) + const localID = Number.parseInt(match[2], 10) + if (sessionID === 0) maxLocalId0 = Math.max(maxLocalId0, localID) + if (sessionID === 1) maxLocalId1 = Math.max(maxLocalId1, localID) + } + return { ids: [...ids], maxLocalId0, maxLocalId1 } +} + +function assignComponentPropertyGuids( + propertyIds: readonly string[], + localIdCounter: { value: number }, + propertyIdToGuid: Map, + assignedGuidValues: Set, + nodeSourceGuidValues: Set +): void { + for (const propertyId of propertyIds) { + const guid = assignVariableGuid( + propertyId, + localIdCounter, + assignedGuidValues, + nodeSourceGuidValues + ) + propertyIdToGuid.set(propertyId, guid) + } +} + function appendVariableNodeChanges( graph: SceneGraph, nodeChanges: KiwiNodeChange[], @@ -466,6 +511,9 @@ export async function exportFigFile( } } } + const propertyGuidState = collectComponentPropertyGuidState(graph) + maxLocalId0 = Math.max(maxLocalId0, propertyGuidState.maxLocalId0) + maxLocalId1 = Math.max(maxLocalId1, propertyGuidState.maxLocalId1) localIdCounter.value = Math.max(localIdCounter.value, maxLocalId0 + 1, maxLocalId1 + 1) const { canvasEntries, internalCanvasGuid } = buildCanvasEntries( @@ -488,6 +536,14 @@ export async function exportFigFile( nodeSourceGuidValues ) + assignComponentPropertyGuids( + propertyGuidState.ids, + localIdCounter, + propertyIdToGuid, + assignedGuidValues, + nodeSourceGuidValues + ) + for (const entry of canvasEntries) nodeChanges.push(entry.canvasNc) const orderedCanvasEntries = [ diff --git a/packages/core/src/kiwi/fig/import.ts b/packages/core/src/kiwi/fig/import.ts index 1d3648985..8ba2a9ba2 100644 --- a/packages/core/src/kiwi/fig/import.ts +++ b/packages/core/src/kiwi/fig/import.ts @@ -407,7 +407,10 @@ function remapComponentIds(graph: SceneGraph, guidToNodeId: Map) * how it was exported), not this import's freshly-assigned node ID — remap * them the same way remapComponentIds fixes up instance.componentId. */ -function remapInstanceSwapPropertyValues(graph: SceneGraph, guidToNodeId: Map): void { +function remapInstanceSwapPropertyValues( + graph: SceneGraph, + guidToNodeId: Map +): void { const defsById = new Map() for (const node of graph.getAllNodes()) { for (const def of node.componentPropertyDefinitions) { @@ -421,13 +424,8 @@ function remapInstanceSwapPropertyValues(graph: SceneGraph, guidToNodeId: Map { if (def.type !== 'INSTANCE_SWAP') return def const remappedDefault = def.defaultValue ? guidToNodeId.get(def.defaultValue) : undefined - const remappedPreferred = def.preferredValues?.map((value) => guidToNodeId.get(value) ?? value) - if (!remappedDefault && !remappedPreferred) return def - return { - ...def, - defaultValue: remappedDefault ?? def.defaultValue, - preferredValues: remappedPreferred ?? def.preferredValues - } + if (!remappedDefault) return def + return { ...def, defaultValue: remappedDefault } }) const changed = defs.some((def, i) => def !== node.componentPropertyDefinitions[i]) if (changed) graph.updateNode(node.id, { componentPropertyDefinitions: defs }) diff --git a/packages/fig/src/node-change/export-node.ts b/packages/fig/src/node-change/export-node.ts index c207e8ae3..f1b1fa3c6 100644 --- a/packages/fig/src/node-change/export-node.ts +++ b/packages/fig/src/node-change/export-node.ts @@ -1,5 +1,5 @@ import type { NodeChange, Paint } from '@open-pencil/kiwi/fig/codec' -import { guidToString, stringToGuid } from '@open-pencil/kiwi/fig/guid' +import { stringToGuid } from '@open-pencil/kiwi/fig/guid' import { DEFAULT_STROKE_MITER_LIMIT } from '@open-pencil/scene-graph' import type { ComponentPropertyDefinition, @@ -68,7 +68,7 @@ interface SceneNodeToKiwiContext { /** GUIDs minted for component property IDs (e.g. "prop:abc123") that aren't * already Figma-GUID-shaped, keyed by the original ID so refs/assignments/ * variantPropSpecs pointing at the same property reuse the same GUID. */ - propertyIdToGuid?: Map + propertyIdToGuid: Map componentPropertyDefinitionsById: ReadonlyMap fractionalPosition: (index: number) => string mapToFigmaType: (type: SceneNode['type']) => string @@ -148,8 +148,9 @@ function componentPropertyValue( if (type === 'BOOLEAN') return { boolValue: value === 'true' } if (type === 'INSTANCE_SWAP') { const target = context.graph.getNode(value) - if (!target) return { textValue: { characters: value } } - const guid = getOrCreateNodeGuid(context, target.id, localIdCounter) + const guid = target + ? getOrCreateNodeGuid(context, target.id, localIdCounter) + : parseGuidOrNull(value) return guid ? { guidValue: guid } : { textValue: { characters: value } } } return { textValue: { characters: value } } @@ -372,10 +373,13 @@ function getOrCreatePropertyGuid( propertyId: string, localIdCounter: { value: number } ): GUID { - const existing = context.propertyIdToGuid?.get(propertyId) + const existing = context.propertyIdToGuid.get(propertyId) if (existing) return existing - const guid = parseGuidOrNull(propertyId) ?? { sessionID: 1, localID: localIdCounter.value++ } - context.propertyIdToGuid?.set(propertyId, guid) + const parsed = parseGuidOrNull(propertyId) + if (parsed) return parsed + const guid = { sessionID: 1, localID: localIdCounter.value++ } + context.propertyIdToGuid.set(propertyId, guid) + context.assignedGuidValues?.add(`${guid.sessionID}:${guid.localID}`) return guid } @@ -624,7 +628,10 @@ function applyInstancePayload( } nc.symbolData = symbolData as KiwiNodeChange['symbolData'] } - if (node.source.fig.componentPropAssignments.length > 0) { + if ( + node.source.fig.componentPropAssignments.length > 0 && + !node.source.editedFields.includes('componentPropertyAssignments') + ) { nc.componentPropAssignments = materializeFigmaPayload( node.source.fig.componentPropAssignments, context.blobs, @@ -653,15 +660,14 @@ function applyInstancePayload( function componentPropertyPreferredValues( definition: ComponentPropertyDefinition, - context: SceneNodeToKiwiContext, - localIdCounter: { value: number } + context: SceneNodeToKiwiContext ) { if (definition.type === 'INSTANCE_SWAP' && definition.preferredValues?.length) { return { - instanceSwapValues: definition.preferredValues.map((nodeId) => { - const target = context.graph.getNode(nodeId) - const guid = target ? getOrCreateNodeGuid(context, target.id, localIdCounter) : undefined - return { type: 'COMPONENT', key: guid ? guidToString(guid) : nodeId } + instanceSwapValues: definition.preferredValues.map((value) => { + const target = context.graph.getNode(value) + const key = target?.componentKey || target?.sourceLibraryKey || value + return { type: 'COMPONENT', key } }) } } @@ -723,7 +729,7 @@ function applyComponentMetadata( name: def.name, type: componentPropertyTypeForKiwi(def.type), initialValue: componentPropertyValue(def.type, def.defaultValue, context, localIdCounter), - preferredValues: componentPropertyPreferredValues(def, context, localIdCounter) + preferredValues: componentPropertyPreferredValues(def, context) })) if (shouldSerializeRawBackedField(node, 'componentPropDefs', componentPropDefs.length > 0)) { nc.componentPropDefs = componentPropDefs diff --git a/packages/fig/src/node-change/serialize.ts b/packages/fig/src/node-change/serialize.ts index a3cc4a2fe..c8c25f754 100644 --- a/packages/fig/src/node-change/serialize.ts +++ b/packages/fig/src/node-change/serialize.ts @@ -494,7 +494,7 @@ export function sceneNodeToKiwi( runtime: FigNodeChangeExportRuntime = EMPTY_EXPORT_RUNTIME, componentPropertyDefinitionsById = buildComponentPropIndex(graph), modeIdToGuid?: Map, - propertyIdToGuid?: Map + propertyIdToGuid = new Map() ): KiwiNodeChange[] { // Raw paints retain library asset refs; effects use this map because their // Kiwi schema accepts only GUID-backed aliases. diff --git a/packages/fig/tests/export.test.ts b/packages/fig/tests/export.test.ts index 9a1056cf0..760b84c98 100644 --- a/packages/fig/tests/export.test.ts +++ b/packages/fig/tests/export.test.ts @@ -1,6 +1,7 @@ import { describe, expect, test } from 'bun:test' import { SceneGraph } from '@open-pencil/scene-graph' +import type { GUID } from '@open-pencil/scene-graph/primitives' import { buildComponentPropIndex, @@ -151,7 +152,14 @@ describe('@open-pencil/fig SceneGraph export policy', () => { ] }) - const [change] = sceneNodeToKiwi(componentSet, { sessionID: 1, localID: 1 }, 0, { value: 2 }, graph, []) + const [change] = sceneNodeToKiwi( + componentSet, + { sessionID: 1, localID: 1 }, + 0, + { value: 2 }, + graph, + [] + ) expect(change.componentPropDefs).toHaveLength(1) expect(change.componentPropDefs?.[0].id).toEqual( @@ -172,8 +180,8 @@ describe('@open-pencil/fig SceneGraph export policy', () => { componentPropertyReferences: [{ propertyId: 'prop:icon1234', field: 'INSTANCE_SWAP' }] }) - const nodeIdToGuid = new Map() - const propertyIdToGuid = new Map() + const nodeIdToGuid = new Map() + const propertyIdToGuid = new Map() const localIdCounter = { value: 2 } const [componentChange] = sceneNodeToKiwi( component, @@ -212,21 +220,26 @@ describe('@open-pencil/fig SceneGraph export policy', () => { propertyIdToGuid )[0] - expect(componentChange.componentPropDefs?.[0].id).toEqual(slotChange.componentPropRefs?.[0].defID) + expect(componentChange.componentPropDefs?.[0].id).toEqual( + slotChange.componentPropRefs?.[0].defID + ) }) test('points an INSTANCE_SWAP default value at the same GUID the target component is exported with', () => { const graph = new SceneGraph() const page = graph.getPages()[0] - const icon = graph.createNode('COMPONENT', page.id, { name: 'Icon/Tune' }) + const icon = graph.createNode('COMPONENT', page.id, { + name: 'Icon/Tune', + componentKey: 'icon-tune-key' + }) const button = graph.createNode('COMPONENT', page.id, { componentPropertyDefinitions: [ { id: 'prop:iconswap1', name: 'Icon', type: 'INSTANCE_SWAP', defaultValue: icon.id } ] }) - const nodeIdToGuid = new Map() - const propertyIdToGuid = new Map() + const nodeIdToGuid = new Map() + const propertyIdToGuid = new Map() const localIdCounter = { value: 2 } const [iconChange] = sceneNodeToKiwi( icon, @@ -266,5 +279,87 @@ describe('@open-pencil/fig SceneGraph export policy', () => { ) expect(buttonChange.componentPropDefs?.[0].initialValue).toEqual({ guidValue: iconChange.guid }) + expect(buttonChange.componentPropDefs?.[0].preferredValues).toBeUndefined() + }) + + test('exports INSTANCE_SWAP preferred values as component keys', () => { + const graph = new SceneGraph() + const page = graph.getPages()[0] + const icon = graph.createNode('COMPONENT', page.id, { + name: 'Icon/Tune', + componentKey: 'icon-tune-key' + }) + const button = graph.createNode('COMPONENT', page.id, { + componentPropertyDefinitions: [ + { + id: 'prop:iconswap2', + name: 'Icon', + type: 'INSTANCE_SWAP', + defaultValue: icon.id, + preferredValues: [icon.id, 'external-library-key'] + } + ] + }) + + const [buttonChange] = sceneNodeToKiwi( + button, + { sessionID: 1, localID: 1 }, + 0, + { value: 2 }, + graph, + [] + ) + + expect(buttonChange.componentPropDefs?.[0].preferredValues?.instanceSwapValues).toEqual([ + { type: 'COMPONENT', key: 'icon-tune-key' }, + { type: 'COMPONENT', key: 'external-library-key' } + ]) + }) + + test('preserves unresolved GUID-shaped INSTANCE_SWAP values as GUIDs', () => { + const graph = new SceneGraph() + const page = graph.getPages()[0] + const component = graph.createNode('COMPONENT', page.id, { + componentPropertyDefinitions: [ + { id: 'prop:iconswap3', name: 'Icon', type: 'INSTANCE_SWAP', defaultValue: '70:1' } + ] + }) + + const [change] = sceneNodeToKiwi( + component, + { sessionID: 1, localID: 1 }, + 0, + { value: 2 }, + graph, + [] + ) + + expect(change.componentPropDefs?.[0].initialValue).toEqual({ + guidValue: { sessionID: 70, localID: 1 } + }) + }) + + test('shares synthetic property GUIDs across recursive serialization without a supplied map', () => { + const graph = new SceneGraph() + const page = graph.getPages()[0] + const component = graph.createNode('COMPONENT', page.id, { + componentPropertyDefinitions: [ + { id: 'prop:recursive', name: 'Label', type: 'TEXT', defaultValue: 'Default' } + ] + }) + graph.createNode('TEXT', component.id, { + componentPropertyReferences: [{ propertyId: 'prop:recursive', field: 'TEXT' }] + }) + + const changes = sceneNodeToKiwi( + component, + { sessionID: 1, localID: 1 }, + 0, + { value: 2 }, + graph, + [] + ) + + expect(changes[0].componentPropDefs?.[0].id).toEqual(changes[1].componentPropRefs?.[0].defID) }) }) diff --git a/tests/engine/io/fig/export/component-properties.test.ts b/tests/engine/io/fig/export/component-properties.test.ts index 35321f4b8..a830732d2 100644 --- a/tests/engine/io/fig/export/component-properties.test.ts +++ b/tests/engine/io/fig/export/component-properties.test.ts @@ -171,4 +171,47 @@ describe('Figma component property roundtrip', () => { '30:2': 'false' }) }) + + test('exports edited imported assignments instead of stale raw values', async () => { + const graph = new SceneGraph() + const page = graph.getPages()[0] + const component = graph.createNode('COMPONENT', page.id, { + name: 'Card', + componentPropertyDefinitions: [ + { id: '30:1', name: 'Label', type: 'TEXT', defaultValue: 'Default' } + ] + }) + component.source.id = '10:1' + const instance = graph.createInstance(component.id, page.id, { + name: 'Card instance', + componentPropertyAssignments: { '30:1': 'Original' } + }) + if (!instance) throw new Error('Expected instance') + instance.source.id = '20:1' + + const imported = importNodeChanges( + parseFigBuffer((await exportFigFile(graph)).buffer as ArrayBuffer).nodeChanges, + [], + undefined, + { populate: 'all' } + ) + const importedInstance = [...imported.getAllNodes()].find( + (node) => node.name === 'Card instance' + ) + if (!importedInstance) throw new Error('Expected imported instance') + imported.updateNode(importedInstance.id, { + componentPropertyAssignments: { '30:1': 'Edited' } + }) + + const reloaded = importNodeChanges( + parseFigBuffer((await exportFigFile(imported)).buffer as ArrayBuffer).nodeChanges, + [], + undefined, + { populate: 'all' } + ) + const reloadedInstance = [...reloaded.getAllNodes()].find( + (node) => node.name === 'Card instance' + ) + expect(reloadedInstance?.componentPropertyAssignments).toEqual({ '30:1': 'Edited' }) + }) }) diff --git a/tests/engine/io/fig/import/component-props.test.ts b/tests/engine/io/fig/import/component-props.test.ts index 7069132e3..0925ceef4 100644 --- a/tests/engine/io/fig/import/component-props.test.ts +++ b/tests/engine/io/fig/import/component-props.test.ts @@ -338,14 +338,20 @@ describe('Figma component property import', () => { id: '3:2', name: 'icon', type: 'INSTANCE_SWAP', - defaultValue: '4:1', + defaultValue: expect.any(String), preferredValues: ['icon/user-key'] } ]) + const defaultTarget = component?.componentPropertyDefinitions[0]?.defaultValue + expect(defaultTarget ? graph.getNode(defaultTarget)?.name : undefined).toBe('icon/mail') const sourceInstance = Array.from(graph.getAllNodes()).find( (node) => node.name === 'Menu item source' ) - expect(sourceInstance?.componentPropertyAssignments).toEqual({ '3:2': '4:3' }) + expect(sourceInstance?.componentPropertyAssignments).toEqual({ + '3:2': expect.any(String) + }) + const assignedTarget = sourceInstance?.componentPropertyAssignments['3:2'] + expect(assignedTarget ? graph.getNode(assignedTarget)?.name : undefined).toBe('icon/user') const clone = Array.from(graph.getAllNodes()).find((node) => node.name === 'Menu item clone') const icon = clone?.childIds .map((id) => graph.getNode(id)) diff --git a/tests/engine/io/fig/instance-swap-roundtrip.test.ts b/tests/engine/io/fig/instance-swap-roundtrip.test.ts index 82389a483..d18d2d4fd 100644 --- a/tests/engine/io/fig/instance-swap-roundtrip.test.ts +++ b/tests/engine/io/fig/instance-swap-roundtrip.test.ts @@ -10,8 +10,14 @@ describe('INSTANCE_SWAP component property round trip', () => { const graph = new SceneGraph() const page = graph.getPages()[0] - const iconA = graph.createNode('COMPONENT', page.id, { name: 'Icon/A' }) - const iconB = graph.createNode('COMPONENT', page.id, { name: 'Icon/B' }) + const iconA = graph.createNode('COMPONENT', page.id, { + name: 'Icon/A', + componentKey: 'icon-a-key' + }) + const iconB = graph.createNode('COMPONENT', page.id, { + name: 'Icon/B', + componentKey: 'icon-b-key' + }) const button = graph.createNode('COMPONENT', page.id, { name: 'Button', componentPropertyDefinitions: [ @@ -27,7 +33,8 @@ describe('INSTANCE_SWAP component property round trip', () => { const slot = graph.createInstance(iconA.id, button.id) if (!slot) throw new Error('failed to create slot instance') graph.updateNode(slot.id, { - componentPropertyReferences: [{ propertyId: 'prop:iconswap01', field: 'INSTANCE_SWAP' }] + componentPropertyReferences: [{ propertyId: 'prop:iconswap01', field: 'INSTANCE_SWAP' }], + componentPropertyAssignments: { 'prop:iconswap01': iconB.id } }) const exported = await exportFigFile(graph) @@ -44,12 +51,18 @@ describe('INSTANCE_SWAP component property round trip', () => { const defaultTarget = def?.defaultValue ? reloaded.getNode(def.defaultValue) : undefined expect(defaultTarget?.name).toBe('Icon/A') - const preferredTargets = (def?.preferredValues ?? []).map((id) => reloaded.getNode(id)?.name) - expect(preferredTargets.sort()).toEqual(['Icon/A', 'Icon/B']) + const preferredValues = new Set(def?.preferredValues) + expect(preferredValues).toEqual(new Set(['icon-a-key', 'icon-b-key'])) const reloadedSlot = reloadedButton ? reloaded.getChildren(reloadedButton.id)[0] : undefined expect(reloadedSlot?.type).toBe('INSTANCE') - const slotComponent = reloadedSlot?.componentId ? reloaded.getNode(reloadedSlot.componentId) : undefined + const slotComponent = reloadedSlot?.componentId + ? reloaded.getNode(reloadedSlot.componentId) + : undefined expect(slotComponent?.name).toBe('Icon/A') + + const assignment = reloadedSlot?.componentPropertyAssignments[def?.id ?? ''] + const assignmentTarget = assignment ? reloaded.getNode(assignment) : undefined + expect(assignmentTarget?.name).toBe('Icon/B') }) })