From 783cdf9dbc86fc6d60b5a348fc0863476451a0e4 Mon Sep 17 00:00:00 2001 From: xemc <0xemc@protonmail.com> Date: Tue, 18 Aug 2026 08:10:11 +1000 Subject: [PATCH] fix(fig): component property definitions/references/assignments never survive a save/reload round trip parseGuidOrNull requires the strict Figma GUID shape (sessionID:localID), but every OpenPencil code path that creates a component property definition uses `prop:` IDs, which never match. applyComponentMetadata silently dropped componentPropDefs/componentPropRefs/componentPropAssignments/ variantPropSpecs whenever any entry failed that check, so any component property created by the app (as opposed to imported straight from a real Figma file) vanished on the very next save. Mint a stable synthetic GUID for non-Figma-shaped property IDs instead of dropping them, memoized per export so defs/refs/assignments/variantPropSpecs pointing at the same property stay consistent. Also fix INSTANCE_SWAP default/preferred/assignment values, which reference target node IDs and had the same GUID-shape assumption, and were never being resolved back to real node IDs on import in the first place (remapComponentIds only handled instance.componentId, not these fields). Co-Authored-By: Claude Sonnet 5 --- packages/core/src/io/formats/fig/export.ts | 11 +- packages/core/src/kiwi/fig/import.ts | 55 +++++++- .../src/kiwi/fig/node-change/serialize.ts | 6 +- packages/fig/src/node-change/export-node.ts | 107 ++++++++------ packages/fig/src/node-change/serialize.ts | 4 +- packages/fig/tests/export.test.ts | 132 ++++++++++++++++++ .../io/fig/instance-swap-roundtrip.test.ts | 55 ++++++++ 7 files changed, 321 insertions(+), 49 deletions(-) create mode 100644 tests/engine/io/fig/instance-swap-roundtrip.test.ts diff --git a/packages/core/src/io/formats/fig/export.ts b/packages/core/src/io/formats/fig/export.ts index ca5d0b6cd..c9e057dd7 100644 --- a/packages/core/src/io/formats/fig/export.ts +++ b/packages/core/src/io/formats/fig/export.ts @@ -343,6 +343,7 @@ interface InternalResourceContext { blobIndexByHex: Map assignedGuidValues: Set componentPropertyDefinitionsById: ReturnType + propertyIdToGuid: Map } function appendInternalResources(context: InternalResourceContext): void { @@ -365,7 +366,8 @@ function appendInternalResources(context: InternalResourceContext): void { context.blobIndexByHex, context.assignedGuidValues, context.componentPropertyDefinitionsById, - context.modeIdToGuid + context.modeIdToGuid, + context.propertyIdToGuid ) ) } @@ -429,6 +431,7 @@ export async function exportFigFile( assignedGuidValues.add(`${docGuid.sessionID}:${docGuid.localID}`) const varIdToGuid = new Map() const modeIdToGuid = new Map() + const propertyIdToGuid = new Map() const fontDigestMap = await buildFontDigestMap(graph) const glyphBlobMap = new Map() const blobIndexByHex = new Map() @@ -499,7 +502,8 @@ export async function exportFigFile( blobIndexByHex, assignedGuidValues, componentPropertyDefinitionsById, - modeIdToGuid + modeIdToGuid, + propertyIdToGuid ) ) } @@ -518,7 +522,8 @@ export async function exportFigFile( glyphBlobMap, blobIndexByHex, assignedGuidValues, - componentPropertyDefinitionsById + componentPropertyDefinitionsById, + propertyIdToGuid }) const msg: Record = { diff --git a/packages/core/src/kiwi/fig/import.ts b/packages/core/src/kiwi/fig/import.ts index 5ae529b39..bec4b5984 100644 --- a/packages/core/src/kiwi/fig/import.ts +++ b/packages/core/src/kiwi/fig/import.ts @@ -15,7 +15,11 @@ import { } from '@open-pencil/fig/node-change' import type { NodeChange, VariableDataValuesEntry, Color, GUID } from '@open-pencil/kiwi/fig/codec' import { SceneGraph } from '@open-pencil/scene-graph' -import type { VariableType, VariableValue } from '@open-pencil/scene-graph' +import type { + ComponentPropertyDefinition, + VariableType, + VariableValue +} from '@open-pencil/scene-graph' import { BLACK } from '#core/constants' import { setLazyFigImportContext } from '#core/kiwi/fig/lazy-import' @@ -394,6 +398,54 @@ function remapComponentIds(graph: SceneGraph, guidToNodeId: Map) }) } +/** + * INSTANCE_SWAP definitions/assignments store a target node's GUID (matching + * 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 { + const defsById = new Map() + for (const node of graph.getAllNodes()) { + for (const def of node.componentPropertyDefinitions) { + if (!defsById.has(def.id)) defsById.set(def.id, def) + } + } + + graph.preserveSourceMetadataDuring(() => { + for (const node of graph.getAllNodes()) { + if (node.componentPropertyDefinitions.length > 0) { + const defs = node.componentPropertyDefinitions.map((def) => { + 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 + } + }) + const changed = defs.some((def, i) => def !== node.componentPropertyDefinitions[i]) + if (changed) graph.updateNode(node.id, { componentPropertyDefinitions: defs }) + } + + if (Object.keys(node.componentPropertyAssignments).length > 0) { + let changed = false + const assignments = { ...node.componentPropertyAssignments } + for (const [propId, value] of Object.entries(assignments)) { + if (defsById.get(propId)?.type !== 'INSTANCE_SWAP') continue + const remapped = guidToNodeId.get(value) + if (remapped) { + assignments[propId] = remapped + changed = true + } + } + if (changed) graph.updateNode(node.id, { componentPropertyAssignments: assignments }) + } + } + }) +} + function applyVariantPropSpecs(graph: SceneGraph): void { for (const node of graph.getAllNodes()) { if (node.type !== 'COMPONENT' || node.variantPropSpecs.length === 0 || !node.parentId) continue @@ -509,6 +561,7 @@ export function importNodeChanges( importVariableEntries(changeMap, parentMap, graph, assetRefs) importVariableBindings(changeMap, guidToNodeId, graph) remapComponentIds(graph, guidToNodeId) + remapInstanceSwapPropertyValues(graph, guidToNodeId) applyVariantPropSpecs(graph) const firstPageId = graph.getPages()[0]?.id diff --git a/packages/core/src/kiwi/fig/node-change/serialize.ts b/packages/core/src/kiwi/fig/node-change/serialize.ts index 6e97ee40b..9cf8b3ad3 100644 --- a/packages/core/src/kiwi/fig/node-change/serialize.ts +++ b/packages/core/src/kiwi/fig/node-change/serialize.ts @@ -38,7 +38,8 @@ export function sceneNodeToKiwi( blobIndexByHex?: Map, assignedGuidValues?: Set, componentPropertyDefinitionsById?: ReadonlyMap, - modeIdToGuid?: Map + modeIdToGuid?: Map, + propertyIdToGuid?: Map ): KiwiNodeChange[] { return sceneNodeToKiwiWithRuntime( node, @@ -55,6 +56,7 @@ export function sceneNodeToKiwi( assignedGuidValues, coreFigExportRuntime, componentPropertyDefinitionsById, - modeIdToGuid + modeIdToGuid, + propertyIdToGuid ) } diff --git a/packages/fig/src/node-change/export-node.ts b/packages/fig/src/node-change/export-node.ts index 1596b9547..c207e8ae3 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 { stringToGuid } from '@open-pencil/kiwi/fig/guid' +import { guidToString, stringToGuid } from '@open-pencil/kiwi/fig/guid' import { DEFAULT_STROKE_MITER_LIMIT } from '@open-pencil/scene-graph' import type { ComponentPropertyDefinition, @@ -65,6 +65,10 @@ interface SceneNodeToKiwiContext { modeIdToGuid?: Map /** Variable GUIDs used only where raw effect aliases cannot retain asset refs. */ assetRefToVarGuid?: Map + /** 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 componentPropertyDefinitionsById: ReadonlyMap fractionalPosition: (index: number) => string mapToFigmaType: (type: SceneNode['type']) => string @@ -135,11 +139,17 @@ function componentPropertyTypeForKiwi(type: string) { return type } -function componentPropertyValue(type: string, value: string, graph: SceneGraph) { +function componentPropertyValue( + type: string, + value: string, + context: SceneNodeToKiwiContext, + localIdCounter: { value: number } +) { if (type === 'BOOLEAN') return { boolValue: value === 'true' } if (type === 'INSTANCE_SWAP') { - const target = graph.getNode(value) - const guid = parseGuidOrNull(target?.source.id ?? value) + const target = context.graph.getNode(value) + if (!target) return { textValue: { characters: value } } + const guid = getOrCreateNodeGuid(context, target.id, localIdCounter) return guid ? { guidValue: guid } : { textValue: { characters: value } } } return { textValue: { characters: value } } @@ -351,6 +361,24 @@ function getOrCreateNodeGuid( return guid } +/** + * Component property IDs ("prop:abc123") never match the Figma GUID shape, + * so parseGuidOrNull always rejects them — mint a stable synthetic GUID from + * the shared node-id counter instead, memoized so every def/ref/assignment/ + * variantPropSpec pointing at the same property ID round-trips consistently. + */ +function getOrCreatePropertyGuid( + context: SceneNodeToKiwiContext, + propertyId: string, + localIdCounter: { value: number } +): GUID { + const existing = context.propertyIdToGuid?.get(propertyId) + if (existing) return existing + const guid = parseGuidOrNull(propertyId) ?? { sessionID: 1, localID: localIdCounter.value++ } + context.propertyIdToGuid?.set(propertyId, guid) + return guid +} + function isDescendantOf(context: SceneNodeToKiwiContext, nodeId: string, ancestorId: string) { let current = context.graph.getNode(nodeId) while (current?.parentId) { @@ -623,10 +651,18 @@ function applyInstancePayload( } } -function componentPropertyPreferredValues(definition: ComponentPropertyDefinition) { +function componentPropertyPreferredValues( + definition: ComponentPropertyDefinition, + context: SceneNodeToKiwiContext, + localIdCounter: { value: number } +) { if (definition.type === 'INSTANCE_SWAP' && definition.preferredValues?.length) { return { - instanceSwapValues: definition.preferredValues.map((key) => ({ type: 'COMPONENT', key })) + 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 } + }) } } if (definition.type === 'VARIANT' && definition.variantOptions?.length) { @@ -665,7 +701,8 @@ function shouldSerializeRawBackedField( function applyComponentMetadata( context: SceneNodeToKiwiContext, node: SceneNode, - nc: KiwiNodeChange + nc: KiwiNodeChange, + localIdCounter: { value: number } ): void { if (node.componentKey) nc.componentKey = node.componentKey if (node.sourceLibraryKey) nc.sourceLibraryKey = node.sourceLibraryKey @@ -681,45 +718,33 @@ function applyComponentMetadata( } if (node.symbolDescription) nc.symbolDescription = node.symbolDescription if (node.symbolLinks.length > 0) nc.symbolLinks = structuredClone(node.symbolLinks) - const componentPropDefs = node.componentPropertyDefinitions - .map((def) => { - const id = parseGuidOrNull(def.id) - return id - ? { - id, - name: def.name, - type: componentPropertyTypeForKiwi(def.type), - initialValue: componentPropertyValue(def.type, def.defaultValue, context.graph), - preferredValues: componentPropertyPreferredValues(def) - } - : null - }) - .filter((def): def is NonNullable => def !== null) + const componentPropDefs = node.componentPropertyDefinitions.map((def) => ({ + id: getOrCreatePropertyGuid(context, def.id, localIdCounter), + name: def.name, + type: componentPropertyTypeForKiwi(def.type), + initialValue: componentPropertyValue(def.type, def.defaultValue, context, localIdCounter), + preferredValues: componentPropertyPreferredValues(def, context, localIdCounter) + })) if (shouldSerializeRawBackedField(node, 'componentPropDefs', componentPropDefs.length > 0)) { nc.componentPropDefs = componentPropDefs } - const componentPropRefs = node.componentPropertyReferences - .map((ref) => { - const defID = parseGuidOrNull(ref.propertyId) - if (!defID) return null - return { defID, componentPropNodeField: componentPropertyNodeField(ref.field) } - }) - .filter((ref): ref is NonNullable => ref !== null) + const componentPropRefs = node.componentPropertyReferences.map((ref) => ({ + defID: getOrCreatePropertyGuid(context, ref.propertyId, localIdCounter), + componentPropNodeField: componentPropertyNodeField(ref.field) + })) if (shouldSerializeRawBackedField(node, 'componentPropRefs', componentPropRefs.length > 0)) { nc.componentPropRefs = componentPropRefs } const componentPropAssignments = Object.entries(node.componentPropertyAssignments) .map(([propertyId, value]) => { - const defID = parseGuidOrNull(propertyId) const definition = context.componentPropertyDefinitionsById.get(propertyId) - return defID && definition - ? { - defID, - value: componentPropertyValue(definition.type, value, context.graph) - } - : null + if (!definition) return null + return { + defID: getOrCreatePropertyGuid(context, propertyId, localIdCounter), + value: componentPropertyValue(definition.type, value, context, localIdCounter) + } }) .filter((assignment): assignment is NonNullable => assignment !== null) if ( @@ -733,12 +758,10 @@ function applyComponentMetadata( nc.componentPropAssignments = componentPropAssignments } - const variantPropSpecs = node.variantPropSpecs - .map((spec) => { - const propDefId = parseGuidOrNull(spec.propDefId) - return propDefId ? { propDefId, value: spec.value } : null - }) - .filter((spec): spec is NonNullable => spec !== null) + const variantPropSpecs = node.variantPropSpecs.map((spec) => ({ + propDefId: getOrCreatePropertyGuid(context, spec.propDefId, localIdCounter), + value: spec.value + })) if (shouldSerializeRawBackedField(node, 'variantPropSpecs', variantPropSpecs.length > 0)) { nc.variantPropSpecs = variantPropSpecs } @@ -941,7 +964,7 @@ export function sceneNodeToKiwiWithContext( if (node.locked) nc.locked = true applyNodeVisualProps(context, node, nc) - applyComponentMetadata(context, node, nc) + applyComponentMetadata(context, node, nc, localIdCounter) applyInstancePayload(context, node, nc, localIdCounter) if (node.type === 'COMPONENT_SET') upsertPluginData(node, NODE_TYPE_PLUGIN_KEY, node.type) if (nc.type === 'CANVAS') nc.pageType = 'DESIGN' diff --git a/packages/fig/src/node-change/serialize.ts b/packages/fig/src/node-change/serialize.ts index 084933a2f..a3cc4a2fe 100644 --- a/packages/fig/src/node-change/serialize.ts +++ b/packages/fig/src/node-change/serialize.ts @@ -493,7 +493,8 @@ export function sceneNodeToKiwi( assignedGuidValues?: Set, runtime: FigNodeChangeExportRuntime = EMPTY_EXPORT_RUNTIME, componentPropertyDefinitionsById = buildComponentPropIndex(graph), - modeIdToGuid?: Map + modeIdToGuid?: Map, + propertyIdToGuid?: Map ): KiwiNodeChange[] { // Raw paints retain library asset refs; effects use this map because their // Kiwi schema accepts only GUID-backed aliases. @@ -510,6 +511,7 @@ export function sceneNodeToKiwi( modeIdToGuid, assetRefToVarGuid, componentPropertyDefinitionsById, + propertyIdToGuid, fractionalPosition, mapToFigmaType, fillToKiwiPaint, diff --git a/packages/fig/tests/export.test.ts b/packages/fig/tests/export.test.ts index 925672431..9a1056cf0 100644 --- a/packages/fig/tests/export.test.ts +++ b/packages/fig/tests/export.test.ts @@ -135,4 +135,136 @@ describe('@open-pencil/fig SceneGraph export policy', () => { expect(change.derivedTextData?.glyphs).toHaveLength(1) expect(blobs).toHaveLength(1) }) + + test('mints a synthetic GUID for app-created (non-Figma-shaped) component property IDs', () => { + const graph = new SceneGraph() + const page = graph.getPages()[0] + const componentSet = graph.createNode('COMPONENT_SET', page.id, { + componentPropertyDefinitions: [ + { + id: 'prop:abc12345', + name: 'Style', + type: 'VARIANT', + defaultValue: 'Primary', + variantOptions: ['Primary', 'Secondary'] + } + ] + }) + + const [change] = sceneNodeToKiwi(componentSet, { sessionID: 1, localID: 1 }, 0, { value: 2 }, graph, []) + + expect(change.componentPropDefs).toHaveLength(1) + expect(change.componentPropDefs?.[0].id).toEqual( + expect.objectContaining({ sessionID: expect.any(Number), localID: expect.any(Number) }) + ) + expect(change.componentPropDefs?.[0].name).toBe('Style') + }) + + test('reuses the same synthetic GUID for a def and the ref that points at it', () => { + const graph = new SceneGraph() + const page = graph.getPages()[0] + const component = graph.createNode('COMPONENT', page.id, { + componentPropertyDefinitions: [ + { id: 'prop:icon1234', name: 'Icon', type: 'INSTANCE_SWAP', defaultValue: '' } + ] + }) + const slot = graph.createNode('INSTANCE', component.id, { + componentPropertyReferences: [{ propertyId: 'prop:icon1234', field: 'INSTANCE_SWAP' }] + }) + + const nodeIdToGuid = new Map() + const propertyIdToGuid = new Map() + const localIdCounter = { value: 2 } + const [componentChange] = sceneNodeToKiwi( + component, + { sessionID: 1, localID: 1 }, + 0, + localIdCounter, + graph, + [], + nodeIdToGuid, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + propertyIdToGuid + ) + const slotChange = sceneNodeToKiwi( + slot, + componentChange.guid, + 0, + localIdCounter, + graph, + [], + nodeIdToGuid, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + propertyIdToGuid + )[0] + + 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 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 localIdCounter = { value: 2 } + const [iconChange] = sceneNodeToKiwi( + icon, + { sessionID: 1, localID: 1 }, + 0, + localIdCounter, + graph, + [], + nodeIdToGuid, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + propertyIdToGuid + ) + const [buttonChange] = sceneNodeToKiwi( + button, + { sessionID: 1, localID: 1 }, + 1, + localIdCounter, + graph, + [], + nodeIdToGuid, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + propertyIdToGuid + ) + + expect(buttonChange.componentPropDefs?.[0].initialValue).toEqual({ guidValue: iconChange.guid }) + }) }) diff --git a/tests/engine/io/fig/instance-swap-roundtrip.test.ts b/tests/engine/io/fig/instance-swap-roundtrip.test.ts new file mode 100644 index 000000000..82389a483 --- /dev/null +++ b/tests/engine/io/fig/instance-swap-roundtrip.test.ts @@ -0,0 +1,55 @@ +import { describe, expect, test } from 'bun:test' + +import { exportFigFile, parseFigFile } from '@open-pencil/core/io' +import { initCodec } from '@open-pencil/core/kiwi' +import { SceneGraph } from '@open-pencil/scene-graph' + +describe('INSTANCE_SWAP component property round trip', () => { + test('defaultValue, preferredValues, and componentId survive a save/reload cycle', async () => { + await initCodec() + + 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 button = graph.createNode('COMPONENT', page.id, { + name: 'Button', + componentPropertyDefinitions: [ + { + id: 'prop:iconswap01', + name: 'Icon', + type: 'INSTANCE_SWAP', + defaultValue: iconA.id, + preferredValues: [iconA.id, iconB.id] + } + ] + }) + 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' }] + }) + + const exported = await exportFigFile(graph) + const reloaded = await parseFigFile(exported.buffer as ArrayBuffer) + + const reloadedButton = reloaded + .getAllNodes() + .find((n) => n.type === 'COMPONENT' && n.name === 'Button') + expect(reloadedButton).toBeDefined() + + const def = reloadedButton?.componentPropertyDefinitions[0] + expect(def?.type).toBe('INSTANCE_SWAP') + + 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 reloadedSlot = reloadedButton ? reloaded.getChildren(reloadedButton.id)[0] : undefined + expect(reloadedSlot?.type).toBe('INSTANCE') + const slotComponent = reloadedSlot?.componentId ? reloaded.getNode(reloadedSlot.componentId) : undefined + expect(slotComponent?.name).toBe('Icon/A') + }) +})