Merge pull request #551 from 0xemc/fix/instance-fill-override-roundtrip

fix(fig): live edits to instance-descendant fields discarded on save

packages/core/src/figma-api/accessor-utils.ts

@@ -1,3 +1,4 @@
+import { recordInstanceOverride } from '@open-pencil/scene-graph'
 import type { SceneGraph, SceneNode } from '@open-pencil/scene-graph'
 
 import { assertNodeEditable } from '#core/editor/capabilities'
@@ -35,5 +36,14 @@ export function updateNode(
   changes: Partial<SceneNode>
 ): void {
   assertProxyEditable(target, internals)
-  graph(target, internals).updateNode(nodeId(target, internals), changes)
+  const g = graph(target, internals)
+  const id = nodeId(target, internals)
+  const applied = Object.fromEntries(
+    Object.keys(changes)
+      .filter((key) => Reflect.get(changes, key) !== undefined)
+      .map((key) => [key, Reflect.get(changes, key)])
+  ) as Partial<SceneNode>
+  if (Object.keys(applied).length === 0) return
+  g.updateNode(id, applied)
+  recordInstanceOverride(g, id, Object.keys(applied))
 }

packages/core/src/figma-api/proxy.ts

@@ -1,3 +1,4 @@
+import { recordInstanceOverride } from '@open-pencil/scene-graph'
 import type {
   SceneGraph,
   SceneNode,
@@ -137,7 +138,10 @@ export class FigmaNodeProxy {
 
   private _update(changes: Partial<SceneNode>): void {
     assertNodeEditable(this[INTERNAL_GRAPH], this[INTERNAL_ID])
-    this[INTERNAL_GRAPH].updateNode(this[INTERNAL_ID], changes)
+    const graph = this[INTERNAL_GRAPH]
+    const id = this[INTERNAL_ID]
+    graph.updateNode(id, changes)
+    recordInstanceOverride(graph, id, Object.keys(changes))
   }
 
   private _raw(): SceneNode {

packages/core/src/figma-api/strokes.ts

@@ -1,3 +1,4 @@
+import { recordInstanceOverride } from '@open-pencil/scene-graph'
 import type { SceneGraph, SceneNode, Stroke } from '@open-pencil/scene-graph'
 import { copyStrokes } from '@open-pencil/scene-graph/copy'
 
@@ -6,6 +7,7 @@ export function setFirstStrokeWeight(graph: SceneGraph, node: SceneNode, weight:
   const strokes = copyStrokes(node.strokes)
   strokes[0].weight = weight
   graph.updateNode(node.id, { strokes })
+  recordInstanceOverride(graph, node.id, ['strokes'])
 }
 
 export function setFirstStrokeAlign(graph: SceneGraph, node: SceneNode, align: string): void {
@@ -13,6 +15,7 @@ export function setFirstStrokeAlign(graph: SceneGraph, node: SceneNode, align: s
   const strokes = copyStrokes(node.strokes)
   strokes[0].align = align as Stroke['align']
   graph.updateNode(node.id, { strokes })
+  recordInstanceOverride(graph, node.id, ['strokes'])
 }
 
 export function setIndependentStrokeWeight(
@@ -25,4 +28,5 @@ export function setIndependentStrokeWeight(
     [field]: value,
     independentStrokeWeights: true
   })
+  recordInstanceOverride(graph, nodeId, [field, 'independentStrokeWeights'])
 }

packages/core/src/figma-api/text.ts

@@ -1,3 +1,4 @@
+import { recordInstanceOverride } from '@open-pencil/scene-graph'
 import type { SceneGraph, SceneNode } from '@open-pencil/scene-graph'
 
 import { styleNameToWeight, weightToStyleName, type FigmaFontName } from './fonts'
@@ -13,6 +14,7 @@ export function setFontName(graph: SceneGraph, nodeId: string, fontName: FigmaFo
     fontWeight: weight,
     italic
   })
+  recordInstanceOverride(graph, nodeId, ['fontFamily', 'fontWeight'])
 }
 
 export function insertCharacters(
@@ -23,6 +25,7 @@ export function insertCharacters(
 ): void {
   const text = node.text.slice(0, start) + characters + node.text.slice(start)
   graph.updateNode(node.id, { text })
+  recordInstanceOverride(graph, node.id, ['text'])
 }
 
 export function deleteCharacters(
@@ -33,4 +36,5 @@ export function deleteCharacters(
 ): void {
   const text = node.text.slice(0, start) + node.text.slice(end)
   graph.updateNode(node.id, { text })
+  recordInstanceOverride(graph, node.id, ['text'])
 }

packages/fig/src/instance-overrides/index.ts

@@ -18,6 +18,7 @@ import { isEqual } from 'es-toolkit/predicate'
 
 import { guidToString, resolvedNumericBindingUpdate } from '@open-pencil/fig/node-change'
 import type { SceneGraph, SceneNode } from '@open-pencil/scene-graph'
+import { hasInstanceOverride } from '@open-pencil/scene-graph'
 import {
   copyFills,
   copyStyleRuns,
@@ -129,6 +130,7 @@ function propagateResolvedFills(
       const source = graph.getNode(node.componentId)
       if (!source || isEqual(source.fills, node.fills)) continue
       if (protectedNodes.has(node.id) && !protectedNodes.has(source.id)) continue
+      if (hasInstanceOverride(graph, node.id, 'fills')) continue
       graph.updateNode(node.id, { fills: copyFills(source.fills) })
       changed = true
     }

packages/fig/src/instance-overrides/symbol/overrides.ts

@@ -1,8 +1,29 @@
+import { hasInstanceOverride } from '@open-pencil/scene-graph'
+
 import { applyOverridePatch } from '../patches'
 import { resolveOverrideTarget } from '../resolve'
 import type { OverrideContext } from '../types'
 import { patchFromSymbolOverride } from './patches'
 
+/**
+ * Strips fields from a stored-override patch that were since live-edited
+ * (recordInstanceOverride) — reapplying the stale stored value on every
+ * lazy-population pass would otherwise silently discard the edit.
+ */
+function dropLiveOverriddenFields(
+  ctx: OverrideContext,
+  targetId: string,
+  patch: ReturnType<typeof patchFromSymbolOverride>
+): void {
+  if (!patch?.props) return
+  const kept = Object.fromEntries(
+    Object.entries(patch.props).filter(
+      ([field]) => !hasInstanceOverride(ctx.graph, targetId, field)
+    )
+  )
+  patch.props = Object.keys(kept).length > 0 ? (kept as typeof patch.props) : undefined
+}
+
 function isActiveInstance(ctx: OverrideContext, nodeId: string | undefined): nodeId is string {
   return nodeId !== undefined && (!ctx.activeNodeIds || ctx.activeNodeIds.has(nodeId))
 }
@@ -51,6 +72,7 @@ export function applySymbolOverrides(ctx: OverrideContext, propertiesOnly = fals
 
       const patch = patchFromSymbolOverride(ctx, targetId, ov)
       if (!patch) continue
+      dropLiveOverriddenFields(ctx, targetId, patch)
       preserveInstanceRootBounds(nc.size !== undefined, nodeId, targetId, patch)
       if (propertiesOnly) patch.swapComponentId = undefined
       if (!patch.swapComponentId && !patch.props) continue

packages/fig/src/node-change/export-node.ts

@@ -30,6 +30,7 @@ type KiwiBooleanOperation = NonNullable<NodeChange['booleanOperation']>
 interface KiwiSymbolOverridePayload {
   guidPath?: { guids?: GUID[] }
   textData?: { characters?: string }
+  fillPaints?: Paint[]
   [key: string]: unknown
 }
 
@@ -405,11 +406,7 @@ function serializeTextOverrides(
     const target = context.graph.getNode(targetId)
     if (!target || !isDescendantOf(context, targetId, instance.id)) continue
 
-    const sourceId = target.componentId
-    if (!sourceId) continue
-    const source = context.graph.getNode(sourceId)
-    const overrideGuid = source?.overrideKey ? parseGuidOrNull(source.overrideKey) : null
-    const targetGuid = overrideGuid ?? getOrCreateNodeGuid(context, sourceId, localIdCounter)
+    const targetGuid = resolveOverrideTargetGuid(context, target, localIdCounter)
     if (!targetGuid) continue
 
     result.push({
@@ -420,6 +417,46 @@ function serializeTextOverrides(
   return result
 }
 
+/**
+ * Resolves the descendant-node identity a symbol override applies to: the
+ * GUID of the corresponding node inside the target's own main component
+ * (cloneNodeProps stamps componentId with that node's id on every clone).
+ */
+function resolveOverrideTargetGuid(
+  context: SceneNodeToKiwiContext,
+  target: SceneNode,
+  localIdCounter: { value: number }
+): GUID | undefined {
+  const sourceId = target.componentId
+  if (!sourceId) return undefined
+  const source = context.graph.getNode(sourceId)
+  const overrideGuid = source?.overrideKey ? parseGuidOrNull(source.overrideKey) : null
+  return overrideGuid ?? getOrCreateNodeGuid(context, sourceId, localIdCounter)
+}
+
+function serializeFillOverrides(
+  context: SceneNodeToKiwiContext,
+  instance: SceneNode,
+  localIdCounter: { value: number }
+): KiwiSymbolOverridePayload[] {
+  const result: KiwiSymbolOverridePayload[] = []
+  for (const key of Object.keys(instance.overrides)) {
+    if (!key.endsWith(':fills')) continue
+    const targetId = key.slice(0, -':fills'.length)
+    const target = context.graph.getNode(targetId)
+    if (!target || !isDescendantOf(context, targetId, instance.id)) continue
+
+    const targetGuid = resolveOverrideTargetGuid(context, target, localIdCounter)
+    if (!targetGuid) continue
+
+    result.push({
+      guidPath: { guids: [targetGuid] },
+      fillPaints: target.fills.map((fill) => context.fillToKiwiPaint(fill))
+    })
+  }
+  return result
+}
+
 function overridePathKey(payload: KiwiSymbolOverridePayload): string | null {
   const guids = payload.guidPath?.guids
   return guids?.length
@@ -427,12 +464,12 @@ function overridePathKey(payload: KiwiSymbolOverridePayload): string | null {
     : null
 }
 
-function mergeTextOverrides(
+function mergeOverrides(
   symbolOverrides: KiwiSymbolOverridePayload[],
-  textOverrides: KiwiSymbolOverridePayload[]
+  newOverrides: KiwiSymbolOverridePayload[]
 ): void {
-  for (const textOverride of textOverrides) {
-    const pathKey = overridePathKey(textOverride)
+  for (const override of newOverrides) {
+    const pathKey = overridePathKey(override)
     let existingIndex = -1
     if (pathKey) {
       for (let index = symbolOverrides.length - 1; index >= 0; index--) {
@@ -441,13 +478,8 @@ function mergeTextOverrides(
         break
       }
     }
-    if (existingIndex < 0) symbolOverrides.push(textOverride)
-    else {
-      symbolOverrides[existingIndex] = {
-        ...symbolOverrides[existingIndex],
-        textData: textOverride.textData
-      }
-    }
+    if (existingIndex < 0) symbolOverrides.push(override)
+    else symbolOverrides[existingIndex] = { ...symbolOverrides[existingIndex], ...override }
   }
 }
 
@@ -622,7 +654,8 @@ function applyInstancePayload(
         }) as KiwiSymbolOverridePayload[])
       )
     }
-    mergeTextOverrides(symbolOverrides, serializeTextOverrides(context, node, localIdCounter))
+    mergeOverrides(symbolOverrides, serializeTextOverrides(context, node, localIdCounter))
+    mergeOverrides(symbolOverrides, serializeFillOverrides(context, node, localIdCounter))
     if (symbolOverrides.length > 0) symbolData.symbolOverrides = symbolOverrides
     if (node.source.fig.uniformScaleFactor != null) {
       symbolData.uniformScaleFactor = node.source.fig.uniformScaleFactor

packages/scene-graph/src/index.ts

@@ -2,7 +2,14 @@
 export * from './images'
 export * from './components/properties'
 export * from './copy'
-export { copyInstanceComponentProps } from './instances'
+export {
+  copyInstanceComponentProps,
+  hasInstanceOverride,
+  INSTANCE_SYNC_FIELDS,
+  INSTANCE_SYNC_PROPS,
+  INSTANCE_SYNC_TEXT_PROPS,
+  recordInstanceOverride
+} from './instances'
 export * from './snap'
 export * from './export-scale'
 export * from './coordinate'

packages/scene-graph/src/instances.ts

@@ -4,7 +4,16 @@ import type { NodeCloneMode } from './copy'
 
 export type { NodeCloneMode } from './copy'
 
-const INSTANCE_SYNC_PROPS: (keyof SceneNode)[] = [
+export const INSTANCE_SYNC_TEXT_PROPS = [
+  'name',
+  'text',
+  'fontSize',
+  'fontWeight',
+  'fontFamily',
+  'textDirection'
+] as const
+
+export const INSTANCE_SYNC_PROPS: (keyof SceneNode)[] = [
   'width',
   'height',
   'minWidth',
@@ -49,6 +58,8 @@ const INSTANCE_SYNC_PROPS: (keyof SceneNode)[] = [
   'variableModes'
 ]
 
+export const INSTANCE_SYNC_FIELDS = [...INSTANCE_SYNC_PROPS, ...INSTANCE_SYNC_TEXT_PROPS] as const
+
 function setSceneProp<K extends keyof SceneNode>(
   target: Partial<SceneNode>,
   key: K,
@@ -142,20 +153,7 @@ function syncChildren(
     const instChild = instChildMap.get(compChildId)
     if (!compChild || !instChild) continue
 
-    for (const key of INSTANCE_SYNC_PROPS) {
-      const overrideKey = `${instChild.id}:${key}`
-      if (overrideKey in overrides) continue
-      copyProp(instChild, compChild, key)
-    }
-
-    for (const key of [
-      'name',
-      'text',
-      'fontSize',
-      'fontWeight',
-      'fontFamily',
-      'textDirection'
-    ] as const) {
+    for (const key of INSTANCE_SYNC_FIELDS) {
       const overrideKey = `${instChild.id}:${key}`
       if (overrideKey in overrides) continue
       copyProp(instChild, compChild, key)
@@ -284,3 +282,49 @@ export function getInstances(graph: SceneGraph, componentId: string): SceneNode[
   }
   return instances
 }
+
+/** Nearest INSTANCE at or above `nodeId` — self, parent, grandparent, etc. */
+export function findInstanceAncestor(graph: SceneGraph, nodeId: string): SceneNode | undefined {
+  let current = graph.nodes.get(nodeId)
+  while (current) {
+    if (current.type === 'INSTANCE') return current
+    current = current.parentId ? graph.nodes.get(current.parentId) : undefined
+  }
+  return undefined
+}
+
+/**
+ * True when a node field is protected from instance synchronization.
+ */
+export function hasInstanceOverride(graph: SceneGraph, nodeId: string, field: string): boolean {
+  const instance = findInstanceAncestor(graph, nodeId)
+  if (!instance) return false
+  const key = nodeId === instance.id ? field : `${nodeId}:${field}`
+  return key in instance.overrides
+}
+
+/*
+ * syncInstances) won't clobber them, and — if `nodeId` sits inside an INSTANCE —
+ * so the .fig exporter knows to write the diff out as a symbol override. A no-op
+ * for fields outside INSTANCE_SYNC_PROPS or nodes with no INSTANCE ancestor.
+ */
+export function recordInstanceOverride(
+  graph: SceneGraph,
+  nodeId: string,
+  fields: Iterable<string>
+): void {
+  const instance = findInstanceAncestor(graph, nodeId)
+  if (!instance) return
+
+  const relevant = [...fields].filter((field) =>
+    (INSTANCE_SYNC_FIELDS as readonly string[]).includes(field)
+  )
+
+  if (relevant.length === 0) return
+
+  const overrides = { ...instance.overrides }
+  for (const field of relevant) {
+    overrides[nodeId === instance.id ? field : `${nodeId}:${field}`] = true
+  }
+  graph.updateNode(instance.id, { overrides })
+}

tests/engine/figma/api/create/instance-fill-override.test.ts

@@ -0,0 +1,51 @@
+import { describe, expect, test } from 'bun:test'
+
+import { createAPI } from '../helpers'
+
+describe('setting fills on an instance descendant', () => {
+  test('records an instance override so the change survives resync', () => {
+    const api = createAPI()
+    const icon = api.createComponent()
+    icon.resize(16, 16)
+
+    const instance = icon.createInstance()
+    const vector = api.graph.createNode('VECTOR', instance.id, { width: 16, height: 16 })
+    api.graph.updateNode(vector.id, { componentId: icon.id })
+
+    const vectorProxy = api.getNodeById(vector.id)
+    if (!vectorProxy) throw new Error('vector proxy not found')
+    vectorProxy.fills = [
+      { type: 'SOLID', color: { r: 0, g: 0, b: 1, a: 1 }, opacity: 1, visible: true }
+    ]
+
+    const raw = api.graph.getNode(instance.id)
+    expect(raw?.overrides[`${vector.id}:fills`]).toBe(true)
+  })
+
+  test('preserves text edits on an instance descendant during resync', () => {
+    const api = createAPI()
+    const component = api.createComponent()
+    const text = api.createText()
+    text.characters = 'Default'
+    component.appendChild(text)
+
+    const instance = component.createInstance()
+    const instanceText = instance.children[0]
+    if (!instanceText) throw new Error('instance text not found')
+    instanceText.characters = 'Custom'
+
+    expect(api.graph.getNode(instance.id)?.overrides[`${instanceText.id}:text`]).toBe(true)
+    text.characters = 'Updated default'
+    api.graph.syncInstances(component.id)
+
+    expect(instanceText.characters).toBe('Custom')
+  })
+  test('does not record an override for a node with no instance ancestor', () => {
+    const api = createAPI()
+    const frame = api.createFrame()
+    frame.fills = [{ type: 'SOLID', color: { r: 1, g: 0, b: 0, a: 1 }, opacity: 1, visible: true }]
+
+    const raw = api.graph.getNode(frame.id)
+    expect(raw?.overrides ?? {}).toEqual({})
+  })
+})

tests/engine/io/fig/instance/override-resave.test.ts

@@ -0,0 +1,61 @@
+import { describe, expect, test } from 'bun:test'
+
+import { FigmaAPI } from '@open-pencil/core'
+import { exportFigFile, parseFigFile } from '@open-pencil/core/io'
+import { initCodec } from '@open-pencil/core/kiwi'
+import { SceneGraph } from '@open-pencil/scene-graph'
+
+const BLUE = { r: 0, g: 0, b: 1, a: 1 }
+const RED = { r: 1, g: 0, b: 0, a: 1 }
+
+function findInstanceAndVector(graph: SceneGraph) {
+  const instance = graph.getAllNodes().find((n) => n.type === 'INSTANCE')
+  if (!instance) throw new Error('instance not found')
+  const vector = graph.getChildren(instance.id)[0]
+  return { instance, vector }
+}
+
+describe('re-editing an instance fill across multiple save/reload cycles', () => {
+  test('a second live edit overrides the first, not the other way around', async () => {
+    await initCodec()
+
+    const graph = new SceneGraph()
+    const page = graph.getPages()[0]
+    const icon = graph.createNode('COMPONENT', page.id, { name: 'Icon' })
+    graph.createNode('VECTOR', icon.id, {
+      name: 'path',
+      width: 16,
+      height: 16,
+      fills: [{ type: 'SOLID', color: { r: 0, g: 0, b: 0, a: 1 }, opacity: 1, visible: true }]
+    })
+
+    const instance = graph.createInstance(icon.id, page.id)
+    if (!instance) throw new Error('failed to create instance')
+    const figma1 = new FigmaAPI(graph)
+    const vectorProxy1 = figma1.getNodeById(graph.getChildren(instance.id)[0].id)
+    if (!vectorProxy1) throw new Error('vector proxy not found')
+    vectorProxy1.fills = [{ type: 'SOLID', color: BLUE, opacity: 1, visible: true }]
+
+    // Round 1: export the blue edit, reimport it — this is the state a
+    // freshly-reopened file would be in, with `blue` baked into the stored
+    // symbolOverride but no live `overrides` bookkeeping (per §1 of the fix).
+    const bytes1 = await exportFigFile(graph)
+    const graph2 = await parseFigFile(bytes1.buffer as ArrayBuffer)
+    const { vector: vector2 } = findInstanceAndVector(graph2)
+    expect(vector2.fills[0]?.color).toEqual(BLUE)
+
+    // Round 2: live-edit the reimported instance's icon to red, then export
+    // and reimport again. Without the fix, `applySymbolOverrides` reapplies
+    // the STALE stored (blue) override on this same export pass, silently
+    // discarding the fresh (red) edit.
+    const figma2 = new FigmaAPI(graph2)
+    const vectorProxy2 = figma2.getNodeById(vector2.id)
+    if (!vectorProxy2) throw new Error('vector proxy not found')
+    vectorProxy2.fills = [{ type: 'SOLID', color: RED, opacity: 1, visible: true }]
+
+    const bytes2 = await exportFigFile(graph2)
+    const graph3 = await parseFigFile(bytes2.buffer as ArrayBuffer)
+    const { vector: vector3 } = findInstanceAndVector(graph3)
+    expect(vector3.fills[0]?.color).toEqual(RED)
+  })
+})

tests/engine/io/fig/instance/override-roundtrip.test.ts

@@ -0,0 +1,46 @@
+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 descendant fill override round trip', () => {
+  test('a recolored nested instance child keeps its color after a save/reload cycle', async () => {
+    await initCodec()
+
+    const graph = new SceneGraph()
+    const page = graph.getPages()[0]
+    const icon = graph.createNode('COMPONENT', page.id, { name: 'Icon' })
+    const path = graph.createNode('VECTOR', icon.id, {
+      name: 'path',
+      width: 16,
+      height: 16,
+      fills: [{ type: 'SOLID', color: { r: 0, g: 0, b: 0, a: 1 }, opacity: 1, visible: true }]
+    })
+
+    const instance = graph.createInstance(icon.id, page.id)
+    if (!instance) throw new Error('failed to create instance')
+    const instancePath = graph.getChildren(instance.id)[0]
+    expect(instancePath.componentId).toBe(path.id)
+
+    graph.updateNode(instancePath.id, {
+      fills: [{ type: 'SOLID', color: { r: 0, g: 0, b: 1, a: 1 }, opacity: 1, visible: true }]
+    })
+    const { recordInstanceOverride } = await import('@open-pencil/scene-graph')
+    recordInstanceOverride(graph, instancePath.id, ['fills'])
+
+    const exported = await exportFigFile(graph)
+    const reloaded = await parseFigFile(exported.buffer as ArrayBuffer)
+
+    const reloadedInstance = reloaded.getAllNodes().find((n) => n.type === 'INSTANCE')
+    expect(reloadedInstance).toBeDefined()
+    const reloadedPath = reloadedInstance ? reloaded.getChildren(reloadedInstance.id)[0] : undefined
+    expect(reloadedPath?.fills[0]?.color).toEqual({ r: 0, g: 0, b: 1, a: 1 })
+
+    const reloadedIcon = reloaded
+      .getAllNodes()
+      .find((n) => n.type === 'COMPONENT' && n.name === 'Icon')
+    const reloadedIconPath = reloadedIcon ? reloaded.getChildren(reloadedIcon.id)[0] : undefined
+    expect(reloadedIconPath?.fills[0]?.color).toEqual({ r: 0, g: 0, b: 0, a: 1 })
+  })
+})