Merge pull request #691 from open-pencil/review-note-proof

fix: preserve Design JSX instance sizing through component sync

CHANGELOG.md

@@ -64,6 +64,7 @@
 - Honor `.pen` frame layout defaults and sizing and padding shorthands so imported auto-layout frames keep their computed dimensions and child positions. (#564)
 - Avoid macOS Keychain prompts during credential status checks and pause repeated credential access after failures until explicitly retried from Settings.
 
+- Honor explicit Design JSX instance dimensions and preserve authored overrides through component synchronization.
 - Route browser Command/Ctrl plus and minus shortcuts to canvas zoom instead of page zoom.
 - Resolve `$name` references in imported `.pen` fills, stroke fills, font families, dimensions, and spacing without requiring a `--` prefix. (#563)
 - Resolve bound fields in each layer’s mode, keep variable edits scoped and undoable, and make broken bindings visible and recoverable.

packages/core/src/design-jsx/reference/authoring.md

@@ -35,7 +35,7 @@ This reference describes scene creation, not React DOM output. Use the `render`
 - Child `propertyRefs` connect fields to stable property IDs, for example `[{ propertyId: 'message', field: 'TEXT' }]`. Supported fields are `TEXT`, `VISIBLE`, and `INSTANCE_SWAP`; text and swap references require text and instance nodes respectively. References do not depend on layer names.
 - Instance assignments use the native string values (including `'true'` / `'false'` for BOOLEAN properties and component IDs for swaps). For example `Instance({ of: noteId, properties: { message: 'Updated review' } })`. Assignments persist through component synchronization; unknown IDs and invalid values fail rather than silently creating inert overrides. Select variants through component-set variant props, not through instance property assignments.
 - Reuse existing local or library components before recreating them. Keep meaningful text, visibility, and swap properties exposed rather than hand-editing cloned child nodes.
-- Distinguish a main component's default size from its instance's placement constraints. Verify the actual instance bounds in narrower parents; a requested Fill dimension alone is not proof that inherited sizing changed. Do not compensate for a sizing mismatch with guessed heights, clipping, or manually positioned siblings.
+- Explicit instance `w` / `h` replace the inherited sizing mode on that axis; omitted dimensions retain the main component's sizing. Authored overrides survive component synchronization. Distinguish those placement constraints from the main component's default size, and verify actual bounds in narrower parents. Do not compensate for a sizing mismatch with guessed heights, clipping, or manually positioned siblings.
 
 ## Verification
 

packages/core/src/design-jsx/renderer.ts

@@ -430,13 +430,34 @@ async function renderInstanceNode(
     const label = typeof ref === 'string' || typeof ref === 'number' ? String(ref) : ''
     throw new Error(`<Instance> component not found: ${label}`)
   }
-  const overrides = {
+  const overrides: Partial<SceneNode> = {
     ...propsToOverrides(props, false, parentLayout),
     ...componentMetadata(props, 'INSTANCE', componentPropertyScope(graph, parentId))
   }
+  // Instances inherit their container layout, but explicitly authored dimensions
+  // must also replace the inherited sizing mode on that axis.
+  const layout = overrides.layoutMode ?? component.layoutMode
+  if (layout !== 'NONE') {
+    const axes =
+      layout === 'HORIZONTAL'
+        ? (['primaryAxisSizing', 'counterAxisSizing'] as const)
+        : (['counterAxisSizing', 'primaryAxisSizing'] as const)
+    for (const [dimension, field] of [
+      ['w', axes[0]],
+      ['h', axes[1]]
+    ] as const) {
+      const value = props[dimension]
+      if (typeof value === 'number') overrides[field] = 'FIXED'
+      else if (value === 'hug' || value === 'fill') overrides[field] = 'HUG'
+    }
+  }
   const instance =
     graph.createInstance(component.id, parentId, overrides) ?? graph.createNode('FRAME', parentId)
   try {
+    for (const [field, value] of Object.entries(overrides)) {
+      setInstanceOverride(instance.instanceOverrides, instance.id, instance.id, field, value)
+    }
+    graph.updateNode(instance.id, { instanceOverrides: instance.instanceOverrides })
     applyBindings(graph, instance.id, bindings)
     applyInstanceOverrides(graph, instance, tree.props.overrides)
     assignComponentProperties(graph, instance, props.properties)

packages/docs/reference/design-authoring.md

@@ -37,7 +37,7 @@ This reference describes scene creation, not React DOM output. Use the `render`
 - Child `propertyRefs` connect fields to stable property IDs, for example `[{ propertyId: 'message', field: 'TEXT' }]`. Supported fields are `TEXT`, `VISIBLE`, and `INSTANCE_SWAP`; text and swap references require text and instance nodes respectively. References do not depend on layer names.
 - Instance assignments use the native string values (including `'true'` / `'false'` for BOOLEAN properties and component IDs for swaps). For example `Instance({ of: noteId, properties: { message: 'Updated review' } })`. Assignments persist through component synchronization; unknown IDs and invalid values fail rather than silently creating inert overrides. Select variants through component-set variant props, not through instance property assignments.
 - Reuse existing local or library components before recreating them. Keep meaningful text, visibility, and swap properties exposed rather than hand-editing cloned child nodes.
-- Distinguish a main component's default size from its instance's placement constraints. Verify the actual instance bounds in narrower parents; a requested Fill dimension alone is not proof that inherited sizing changed. Do not compensate for a sizing mismatch with guessed heights, clipping, or manually positioned siblings.
+- Explicit instance `w` / `h` replace the inherited sizing mode on that axis; omitted dimensions retain the main component's sizing. Authored overrides survive component synchronization. Distinguish those placement constraints from the main component's default size, and verify actual bounds in narrower parents. Do not compensate for a sizing mismatch with guessed heights, clipping, or manually positioned siblings.
 
 ## Verification
 

skills/open-pencil/references/design-authoring.md

@@ -37,7 +37,7 @@ This reference describes scene creation, not React DOM output. Use the `render`
 - Child `propertyRefs` connect fields to stable property IDs, for example `[{ propertyId: 'message', field: 'TEXT' }]`. Supported fields are `TEXT`, `VISIBLE`, and `INSTANCE_SWAP`; text and swap references require text and instance nodes respectively. References do not depend on layer names.
 - Instance assignments use the native string values (including `'true'` / `'false'` for BOOLEAN properties and component IDs for swaps). For example `Instance({ of: noteId, properties: { message: 'Updated review' } })`. Assignments persist through component synchronization; unknown IDs and invalid values fail rather than silently creating inert overrides. Select variants through component-set variant props, not through instance property assignments.
 - Reuse existing local or library components before recreating them. Keep meaningful text, visibility, and swap properties exposed rather than hand-editing cloned child nodes.
-- Distinguish a main component's default size from its instance's placement constraints. Verify the actual instance bounds in narrower parents; a requested Fill dimension alone is not proof that inherited sizing changed. Do not compensate for a sizing mismatch with guessed heights, clipping, or manually positioned siblings.
+- Explicit instance `w` / `h` replace the inherited sizing mode on that axis; omitted dimensions retain the main component's sizing. Authored overrides survive component synchronization. Distinguish those placement constraints from the main component's default size, and verify actual bounds in narrower parents. Do not compensate for a sizing mismatch with guessed heights, clipping, or manually positioned siblings.
 
 ## Verification
 

tests/e2e/canvas/instance-sizing.spec.ts

@@ -0,0 +1,26 @@
+import { expect, test, useEditorSetupWithClear } from '#tests/e2e/fixtures'
+import type * as SizingFixture from '#tests/helpers/canvas/instance-sizing'
+
+test.use({ viewport: { width: 900, height: 700 } })
+
+const editor = useEditorSetupWithClear('/?test&no-chrome&no-rulers')
+
+test('Fill instances wrap inside narrower parents after component synchronization', async () => {
+  await editor.page.evaluate(async () => {
+    const store = window.openPencil?.getStore?.()
+    if (!store) throw new Error('OpenPencil store not initialized')
+    const fixtureURL = '/tests/helpers/canvas/instance-sizing.ts'
+    const { createInstanceSizingScene }: typeof SizingFixture = await import(fixtureURL)
+    const { boardId, componentId } = await createInstanceSizingScene(
+      store.graph,
+      store.state.currentPageId
+    )
+    await store.loadFontsForNodes([boardId, componentId])
+    store.clearSelection()
+    store.zoomToFit()
+    store.requestRender()
+  })
+  await editor.canvas.waitForRender()
+  editor.canvas.assertNoErrors()
+  expect(await editor.canvas.screenshotCanvasRegion()).toMatchSnapshot('instance-fill-sizing.png')
+})

tests/engine/render/jsx/instance-sizing.test.ts

@@ -0,0 +1,62 @@
+import { expect, test } from 'bun:test'
+
+import { Component, Frame, Instance, Rectangle, renderTree } from '@open-pencil/core/design-jsx'
+import { computeAllLayouts } from '@open-pencil/core/layout'
+
+import { getNodeOrThrow } from '#tests/helpers/assert'
+import { makeSceneGraph } from '#tests/helpers/scene'
+
+test('overriding one instance dimension preserves the other inherited dimension', async () => {
+  const graph = makeSceneGraph()
+  const component = await renderTree(graph, Component({ flex: 'col', w: 280, h: 100 }))
+  const result = await renderTree(graph, Instance({ of: component.id, w: 120 }))
+  graph.syncInstances(component.id)
+  computeAllLayouts(graph)
+  const instance = getNodeOrThrow(graph, result.id)
+  expect(instance.width).toBe(120)
+  expect(instance.height).toBe(100)
+  expect(instance.counterAxisSizing).toBe('FIXED')
+  expect(instance.primaryAxisSizing).toBe('FIXED')
+})
+
+for (const flex of ['row', 'col'] as const) {
+  test(`instances fill their parent across inherited fixed ${flex} dimensions`, async () => {
+    const graph = makeSceneGraph()
+    const component = await renderTree(
+      graph,
+      Component({ flex, w: 280, h: 100, children: Rectangle({ w: 20, h: 20 }) })
+    )
+    const parent = await renderTree(
+      graph,
+      Frame({
+        flex: 'col',
+        w: 220,
+        h: 180,
+        children: Instance({ of: component.id, w: 'fill', h: 'fill' })
+      })
+    )
+    const instance = graph.getChildren(parent.id)[0]
+    expect(instance.width).toBe(220)
+    expect(instance.height).toBe(180)
+    graph.syncInstances(component.id)
+    computeAllLayouts(graph)
+    expect(instance.width).toBe(220)
+    expect(instance.height).toBe(180)
+  })
+
+  test(`explicit instance dimensions override inherited Hug ${flex} sizing`, async () => {
+    const graph = makeSceneGraph()
+    const component = await renderTree(
+      graph,
+      Component({ flex, w: 'hug', h: 'hug', children: Rectangle({ w: 20, h: 20 }) })
+    )
+    const result = await renderTree(graph, Instance({ of: component.id, w: 120, h: 80 }))
+    const instance = getNodeOrThrow(graph, result.id)
+    expect(instance.width).toBe(120)
+    expect(instance.height).toBe(80)
+    graph.syncInstances(component.id)
+    computeAllLayouts(graph)
+    expect(instance.width).toBe(120)
+    expect(instance.height).toBe(80)
+  })
+}

tests/helpers/canvas/instance-sizing.ts

@@ -0,0 +1,65 @@
+import { Component, Frame, Instance, Text, renderTree } from '@open-pencil/core/design-jsx'
+import type { SceneGraph } from '@open-pencil/scene-graph'
+
+export async function createInstanceSizingScene(graph: SceneGraph, parentId: string) {
+  const library = graph.addPage('Sizing component')
+  const component = await renderTree(
+    graph,
+    Component({
+      name: 'Note',
+      flex: 'col',
+      w: 280,
+      h: 'hug',
+      p: 16,
+      bg: '#FFFFFF',
+      rounded: 10,
+      properties: [{ id: 'message', name: 'Message', type: 'TEXT', defaultValue: 'A short note.' }],
+      children: Text({
+        name: 'Message',
+        w: 'fill',
+        font: 'Inter',
+        size: 14,
+        lineHeight: 20,
+        color: '#252A31',
+        propertyRefs: [{ propertyId: 'message', field: 'TEXT' }],
+        children: 'A short note.'
+      })
+    }),
+    { parentId: library.id }
+  )
+  const board = await renderTree(
+    graph,
+    Frame({
+      name: 'Instance sizing',
+      flex: 'row',
+      w: 'hug',
+      h: 'hug',
+      gap: 32,
+      p: 32,
+      bg: '#E9E7E2',
+      children: [280, 220].map((width) =>
+        Frame({
+          name: `Container ${width}`,
+          flex: 'col',
+          w: width,
+          h: 'hug',
+          gap: 12,
+          children: [
+            Text({ children: `${width}px container`, font: 'Inter', size: 12, color: '#626975' }),
+            Instance({
+              of: component.id,
+              w: 'fill',
+              properties: {
+                message:
+                  'The blue feels right. Give the date a little more room at the bottom, and keep the quieter version for the print edition.'
+              }
+            })
+          ]
+        })
+      )
+    }),
+    { parentId }
+  )
+  graph.syncInstances(component.id)
+  return { boardId: board.id, componentId: component.id }
+}