Victorwads/my work version - #732
victorwads wants to merge 7 commits into
Conversation
Keep component overrides, variable-backed icon colors, page backgrounds, and fixed text sizing intact across lazy FIG materialization.
…orwads/my-work-version
📝 WalkthroughWalkthroughThe changes add optional MCP canvas following with settings, localization, documentation, and tests. Page colors now derive from page background data and persist through imports, page switches, exports, and saves. FIG imports use scoped variable resolution and imported-state guards to preserve component overrides. Auto-layout preserves fixed text dimensions beside smaller HUG siblings. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to Lazy FIG imports may lose resolved component overrides, and optional canvas following can report successful MCP operations as failures. These issues should be fixed before merge. Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (4 passed)
Full details: Pr Description: ContextExplanation The PR body does not provide review context. GitHub metadata shows that it contains only links to PRs Resolution Replace the two-link body with the repository PR template sections. Add one concise Summary that explains the combined MCP follow-agent and Figma import-fidelity changes and their purpose. Add one to three meaningful What changed details. Add the commands that were actually run and their results, state relevant checks that were not run and why, record changelog status, and complete the AI assistance section. Full details: Title checkExplanation The title "Victorwads/my work version" is vague and does not identify the pull request's actual changes, which cover imported page backgrounds, component synchronization, variable-color resolution, layout behavior, and MCP agent tracking.
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: open-pencil/open-pencil/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 359ba370-f91c-4f24-9922-ffc4e94f5233
⛔ Files ignored due to path filters (2)
tests/e2e/canvas/page-background.spec.ts-snapshots/imported-navy-page-openpencil-darwin.pngis excluded by!**/*.pngtests/e2e/canvas/text/fixed-stretch-visual.spec.ts-snapshots/fixed-stretch-labels-openpencil-darwin.pngis excluded by!**/*.png
📒 Files selected for processing (38)
CHANGELOG.mdpackages/core/src/editor/component-sync.tspackages/core/src/editor/page-viewports.tspackages/core/src/editor/pages.tspackages/core/src/figma-api/page-backgrounds.tspackages/core/src/io/formats/fig/export.tspackages/core/src/kiwi/fig/import.tspackages/core/src/kiwi/fig/lazy-import.tspackages/core/src/kiwi/fig/parse/transfer.tspackages/core/src/kiwi/fig/population/delta.tspackages/core/src/kiwi/fig/variable-color-resolver.tspackages/core/src/layout.tspackages/core/src/layout/yoga-helpers.tspackages/docs/programmable/mcp-server.mdpackages/fig/src/instance-overrides/populate.tspackages/fig/tests/scoped-instance-overrides.test.tspackages/scene-graph/src/index.tspackages/vue/src/i18n/locales/de/automation.jsonpackages/vue/src/i18n/locales/es/automation.jsonpackages/vue/src/i18n/locales/fr/automation.jsonpackages/vue/src/i18n/locales/it/automation.jsonpackages/vue/src/i18n/locales/ja/automation.jsonpackages/vue/src/i18n/locales/pl/automation.jsonpackages/vue/src/i18n/locales/ru/automation.jsonpackages/vue/src/i18n/locales/zh-cn/automation.jsonpackages/vue/src/i18n/messages/automation.tssrc/app/automation/bridge/tool-handlers.tssrc/app/automation/mcp/follow-agent.tssrc/app/automation/mcp/preferences.tssrc/components/settings/mcp/MCPSettingsPanel.vuetests/e2e/canvas/page-background.spec.tstests/e2e/canvas/text/fixed-stretch-visual.spec.tstests/engine/app/automation/follow-agent.test.tstests/engine/editor/page-backgrounds.test.tstests/engine/io/fig/import/population-delta.test.tstests/engine/layout/auto-layout/text/fixed-stretch.test.tstests/helpers/canvas/page-background.tstools/i18n/mixed-script-baseline.txt
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review.
| rootIds | ||
| ) | ||
| withFigVariableColorResolver(context.changeMap, () => { | ||
| graph.preserveSourceMetadataDuring(() => { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🤖 get_repo_knowledge executed:
get_repo_knowledge open-pencil/open-pencil /tmp/coderabbit-repo-knowledge/open-pencil-open-pencil-36f3d216/learnings /tmp/coderabbit-repo-knowledge/open-pencil-open-pencil-36f3d216/conventions
Length of output: 6578
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- lazy-import outline ---'
ast-grep outline packages/core/src/kiwi/fig/lazy-import.ts
printf '%s\n' '--- lazy-import source ---'
cat -n packages/core/src/kiwi/fig/lazy-import.ts
printf '%s\n' '--- bound symbols ---'
rg -n -S 'preserveSourceMetadataDuring|applyImportedStateDuring|populateAndApplyOverrides|scheduleComponentSync|isApplyingImportedState' packages/core/srcRepository: open-pencil/open-pencil
Length of output: 6523
🏁 Script executed:
printf '%s\n' '--- lazy-import source ---'
cat -n packages/core/src/kiwi/fig/lazy-import.ts
printf '%s\n' '--- relevant definitions and call sites ---'
rg -n -S 'preserveSourceMetadataDuring|applyImportedStateDuring|populateAndApplyOverrides|scheduleComponentSync|isApplyingImportedState' packages/core/srcRepository: open-pencil/open-pencil
Length of output: 5777
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- relevant outlines ---'
ast-grep outline packages/core/src/kiwi/fig/instance-overrides.ts
ast-grep outline packages/core/src/editor/graph-events.ts
ast-grep outline packages/core/src/editor/component-sync.ts
printf '%s\n' '--- instance override definitions ---'
rg -n -A100 -B20 -S 'function populateAndApplyOverrides|export function populateAndApplyOverrides|populateAndApplyOverrides' packages/core/src/kiwi packages/core/src
printf '%s\n' '--- graph events ---'
cat -n packages/core/src/editor/graph-events.ts
printf '%s\n' '--- component sync ---'
cat -n packages/core/src/editor/component-sync.ts
printf '%s\n' '--- scope implementations and flags ---'
rg -n -A35 -B15 -S 'preserveSourceMetadataDuring|applyImportedStateDuring|isApplyingImportedState' packages packages/core/srcRepository: open-pencil/open-pencil
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- locate bound definitions ---'
rg -l -S 'function populateAndApplyOverrides|export function populateAndApplyOverrides|preserveSourceMetadataDuring|applyImportedStateDuring|isApplyingImportedState' packages/fig packages/scene-graph packages/core/src | sort
printf '%s\n' '--- populateAndApplyOverrides implementation ---'
rg -n -A180 -B20 -S 'function populateAndApplyOverrides|export function populateAndApplyOverrides' packages/fig
printf '%s\n' '--- scope definitions ---'
rg -n -A45 -B20 -S 'preserveSourceMetadataDuring|applyImportedStateDuring|isApplyingImportedState' packages/scene-graph packages/figRepository: open-pencil/open-pencil
Length of output: 34973
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- SceneGraph event emission and sync ---'
rg -n -A80 -B20 -S 'applyNodeChanges|syncInstances|populateInstanceChildren' packages/scene-graph/src/index.ts packages/scene-graph/src
printf '%s\n' '--- sync implementation references ---'
rg -n -A60 -B20 -S 'syncInstances\(' packages/scene-graph packages/figRepository: open-pencil/open-pencil
Length of output: 47966
Use the imported-state scope for lazy population.
populateAndApplyOverrides updates nodes and emits events that reach scheduleComponentSync. preserveSourceMetadataDuring does not set isApplyingImportedState, so the scheduler can queue a microtask. flushComponentSync can then reset resolved lazy overrides through syncInstances. Replace this wrapper with applyImportedStateDuring and add a lazy-population regression test.
| store.requestRender() | ||
| store.flashNodes(extractNodeIds(result)) | ||
| } | ||
| if (mcpFollowAgent.value) await followAgentActivity(target, toolName, toolArgs, result) | ||
| return { ok: true, result } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '55,100p' src/app/automation/bridge/tool-handlers.ts
sed -n '1,90p' src/app/automation/mcp/follow-agent.ts
rg -n 'handleTool|tool-handlers|followAgentActivity|switchPage\(|zoomToFit\(' src/app/automation src/app/tabs packages/core/src/editor | head -120Repository: open-pencil/open-pencil
Length of output: 5728
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- tool handler ---'
sed -n '1,115p' src/app/automation/bridge/tool-handlers.ts
printf '%s\n' '--- bridge handlers ---'
sed -n '1,100p' src/app/automation/bridge/handlers.ts
printf '%s\n' '--- webmcp runtime ---'
sed -n '1,100p' src/app/automation/webmcp/runtime.ts
printf '%s\n' '--- follow-agent preference references ---'
rg -n -C 3 'mcpFollowAgent|follow agent|followAgent|follow-agent' src packages --glob '*.{ts,tsx}' | head -240
printf '%s\n' '--- switchPage implementation ---'
sed -n '160,215p' packages/core/src/editor/pages.ts
printf '%s\n' '--- viewport methods ---'
sed -n '1,115p' packages/core/src/editor/viewport.tsRepository: open-pencil/open-pencil
Length of output: 18289
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- tool handler ---'
sed -n '1,115p' src/app/automation/bridge/tool-handlers.ts
printf '%s\n' '--- bridge handlers ---'
sed -n '1,100p' src/app/automation/bridge/handlers.ts
printf '%s\n' '--- webmcp runtime ---'
sed -n '1,100p' src/app/automation/webmcp/runtime.ts
printf '%s\n' '--- follow-agent preference references ---'
rg -n -C 3 'mcpFollowAgent|followAgentActivity|follow-agent' src packages --glob '*.{ts,tsx}' | head -240
printf '%s\n' '--- switchPage implementation ---'
sed -n '160,215p' packages/core/src/editor/pages.ts
printf '%s\n' '--- viewport methods ---'
sed -n '1,115p' packages/core/src/editor/viewport.tsRepository: open-pencil/open-pencil
Length of output: 17438
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- command-handler callers ---'
rg -n -C 5 'createAutomationCommandHandlers|handleRequest\(' src --glob '*.{ts,tsx}' | head -260
printf '%s\n' '--- MCP transport error boundaries ---'
rg -n -C 5 'createAutomationCommandHandlers|handleRequest|JSON-RPC|jsonrpc|MCP|mcp' src/app --glob '*.{ts,tsx}' | head -360
printf '%s\n' '--- follow-agent dependencies ---'
sed -n '1,90p' src/app/automation/mcp/follow-agent.ts
rg -n -C 4 'async function preparePage|function preparePage|resolvePageFonts|switchTab|function switchTab' packages/core/src src/app/tabs --glob '*.{ts,tsx}' | head -280
printf '%s\n' '--- relevant tests ---'
rg -n -C 4 'followAgentActivity|mcpFollowAgent|follow agent|createAutomationToolHandler|handleRequest' src packages --glob '*.{test,spec}.{ts,tsx}' | head -300Repository: open-pencil/open-pencil
Length of output: 37829
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- websocket error response ---'
sed -n '45,78p' src/app/automation/bridge/server.ts
printf '%s\n' '--- page preparation ---'
sed -n '1,175p' packages/core/src/editor/pages.ts
printf '%s\n' '--- MCP settings UI contract ---'
rg -n -C 6 'followAgentDescription|mcpFollowAgent|Follow agent' src packages --glob '*.{ts,tsx,vue}' | head -220
printf '%s\n' '--- non-render tool definitions ---'
rg -n -C 5 \"name: 'switch_page'|name: 'zoom|name: 'get_|mutates: false|mutates: true\" packages/core/src src --glob '*.{ts,tsx}' | head -320Repository: open-pencil/open-pencil
Length of output: 14984
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- websocket error response ---'
sed -n '45,78p' src/app/automation/bridge/server.ts
printf '%s\n' '--- page preparation ---'
sed -n '1,175p' packages/core/src/editor/pages.ts
printf '%s\n' '--- MCP settings UI contract ---'
rg -n -C 6 'followAgentDescription|mcpFollowAgent|Follow agent' src packages --glob '*.{ts,tsx,vue}' | head -220
printf '%s\n' '--- non-render tool definitions ---'
rg -n -C 5 "name: 'switch_page'|name: 'zoom|name: 'get_|mutates: false|mutates: true" packages/core/src src --glob '*.{ts,tsx}' | head -320Repository: open-pencil/open-pencil
Length of output: 22921
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- automation target resolution ---'
sed -n '1,180p' src/app/automation/bridge/target.ts
printf '%s\n' '--- page tool implementation ---'
sed -n '1,70p' packages/core/src/tools/read/pages.ts
printf '%s\n' '--- tool definition mutation semantics ---'
rg -n -C 5 'mutates|execution.*mutation|type ToolDef|defineTool' packages/core/src/tools --glob '*.ts' | head -260Repository: open-pencil/open-pencil
Length of output: 24711
Keep follow-agent failures separate from tool results.
A non-render tool can return a result successfully, but createAutomationToolHandler awaits followAgentActivity before returning { ok: true, result }. When page_id targets a different page, followAgentActivity awaits store.switchPage(), whose page preparation awaits population and font-loading operations that can reject. The rejection reaches src/app/automation/bridge/server.ts, which converts it into an { ok: false, error } response. This changes a successful MCP operation into an error when the optional “Follow agent” UI behavior fails.
Catch errors at this post-execution call and return the tool result regardless of follow-agent failure.
Suggested correction
- if (mcpFollowAgent.value) await followAgentActivity(target, toolName, toolArgs, result)
+ if (mcpFollowAgent.value) {
+ try {
+ await followAgentActivity(target, toolName, toolArgs, result)
+ } catch (error) {
+ console.warn('[MCP] Failed to follow agent activity:', error)
+ }
+ }
return { ok: true, result }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| store.requestRender() | |
| store.flashNodes(extractNodeIds(result)) | |
| } | |
| if (mcpFollowAgent.value) await followAgentActivity(target, toolName, toolArgs, result) | |
| return { ok: true, result } | |
| } | |
| } | |
| store.requestRender() | |
| store.flashNodes(extractNodeIds(result)) | |
| } | |
| if (mcpFollowAgent.value) { | |
| try { | |
| await followAgentActivity(target, toolName, toolArgs, result) | |
| } catch (error) { | |
| console.warn('[MCP] Failed to follow agent activity:', error) | |
| } | |
| } | |
| return { ok: true, result } | |
| } | |
| } |
#728
#725