Address code-review findings across plugin, server & unity (115 fixes)#1
Merged
Conversation
added 30 commits
June 23, 2026 12:19
Under manifest documentAccess: dynamic-page, synchronous figma.getNodeById throws and non-current pages' children are not loaded. Replace the 6 sync getNodeById call sites with await getNodeByIdAsync, and load all pages once at startup (gating the message handler) so cross-page traversal is valid.
…meout Bridge.send armed a single 30s timer for every tool, falsely failing large export_unity/export_project_unity round-trips on a healthy plugin connection. Add an optional per-call timeout (10 min for exports, 30s default for queries), thread it through the PluginSender implementations, and raise the follower /rpc abort above the export ceiling.
Each cache Get() called AssetDatabase.SaveAssets() (flushes ALL dirty assets) once per generated sprite, stalling the build. Rely on the importer's existing end-of-build SaveAssets in Build/BuildPageProject finally blocks instead; the in-memory sprite is returned directly, not reloaded.
Styled (no-rowTemplate) rows added a Button but never wired selection or a row component, so clicks did nothing and ApplySelectionVisual painted no highlight. Attach a FigForgeListRow/FigForgeTableRow (single graphic writer, owns all states) bound to the row background, mirroring the template path.
The contract gate only compared one CANONICAL_SCHEMA integer. Also assert the plugin MANIFEST_VERSION is accepted by the importer and that the TS Manifest and C# Manifest top-level field sets match. Document the check's shallowness, and add a disabled game-ci scaffold (needs UNITY_LICENSE) so the largest subsystem can be compiled in CI later.
- README/architecture: ScreenManager/BaseScreen -> FrameManager/FigForgeScreen (the classes that actually exist), and bridge timeout note is now per-tool. - README MCP tool table: list all 15 tools (was missing 7). - architecture: export sandbox is FIGFORGE_WORKSPACE (defaults to cwd), not 'server cwd'. - plugin-guide: add the missing manifest root fields (canonicalSchema, vanilla, settings). - README/unity README: document the com.unity.inputsystem dependency. - README git-URL install example bumped v1.0.1 -> v1.0.57.
…ckaging Release built artifacts on Node 20 (CI and package.json engines require >=22.6) and shipped them without typechecking or testing. Bump to Node 22 and run the contract gate, plugin/server typecheck, server tests, and builds before packaging.
…ild refs - dedupKey now hashes the full PNG byte stream with two independent 32-bit lanes (64-bit key) + length instead of one 32-bit FNV + 2 probe bytes, so a hash collision can no longer silently substitute a different node's sprite. True duplicates still dedup byte-for-byte; the wire format is unchanged. - captureSubtree now excludes excludedIds from a parent's children array (mirrors walk()'s own guard), so an excluded node nested in a canonical part subtree no longer leaves a dangling child id the importer can't resolve.
- FigForgeModal.BindPrimary/Secondary/Tertiary now remove only their own previously-installed ModalData closure (tracked per action) instead of RemoveAllListeners(), so Inspector/user-added onPrimary listeners survive Open(ModalData) and don't stack across repeated opens. - FigForgeLiveImport stages the whole bundle into a unique temp sibling folder and validates it (non-empty, project.json re-parses) before deleting the old import and moving the staged bundle into place, so a malformed/empty live push can no longer wipe the previously-good import.
- Canonical prefab path: distinct refs that differed only in punctuation/whitespace (e.g. 'Btn/Primary' vs 'Btn-Primary') sanitized to the same .prefab file and silently overwrote each other. Append a stable SigHash of the full ref when SafeAsset is lossy; clean alphanumeric refs keep their name. Deterministic per ref, so existing prefab reuse survives re-import. - Radio ToggleGroup was keyed only by the immediate parent, so two independent radio sets under one frame became mutually exclusive. Key the group by (parent, canonical ref) via a per-build map so same-ref radios share a group while distinct refs get their own.
- Leader now installs a permanent 'error' handler on the http server (after bind) and on the WebSocketServer, so a runtime error (e.g. EMFILE/ENFILE on accept) is logged to stderr instead of crashing the bridge as an unhandled event and severing the plugin socket. - resolveAndValidateOutputPath now returns the canonicalized path it actually validated (not the raw resolved path), so a symlinked ancestor can't redirect a write between the containment check and the write. Test updated to expect the canonical path (os.tmpdir() is itself a symlink on macOS). Deferred: the post-takeover plugin-reconnect race in election.ts — bridge.send already returns a fast clear error (not a hang) when disconnected, and the fix is timing-sensitive concurrency best verified against the live multi-process system rather than changed blind.
…uard leak - BuildElement: a broken managed prefab can make InstantiatePrefab return null; fall back to a labelled placeholder instead of NRE'ing on inst.name. - FigForgeFrameInspector: DrawFields and Collect recursed through group refs with no cycle guard, so cyclic accessor wiring hung the editor / StackOverflow'd. DrawFields now tracks the current expansion path; Collect tracks visited ids. - FrameManager.AddGuard(frame,…): a frame whose name sanitized to empty silently became a GLOBAL guard (the string overload treats an empty key as global), applying one frame's precondition to all navigation. Now warns and skips.
A follower that lost the takeover race (or whose freshly-elected leader hasn't re-attached the plugin WS yet) immediately got 'Figma plugin is not connected' during a normal leader restart. Follower now exposes pingStatus() with the /ping pluginConnected flag; the send path waits up to 3s (250ms poll) for the plugin to reconnect before issuing the call. Bounded, side-effect-free (only re-probes /ping), happy path untouched; a truly-down plugin still errors after the wait. typecheck/build clean, 29/29 tests pass.
The hide loop fires OnHide() before Current is assigned; a handler that navigates re-entrantly there had its result clobbered when the outer Show unwound. Add a generation token: each ShowInternal claims a generation before the hide loop and only commits Current/visibility if it's still current, so the innermost nav wins. Normal (non-re-entrant) path is behavior-identical.
NavBinder.Start does a scene-global find, so N binders each wired every link → N listeners per button → one click fired Show N times. Add a NonSerialized 'bound' marker on FigForgeNavLink; binders skip already-bound links. Resets per scene load, so each link is wired exactly once per load.
…tes, stable sections Full imports now declare the expected frame-class set up front (BeginBatch from the importer, EndBatch in its finally): - SiblingFrameClassNames returns that fixed set during a batch, so every frame in a section computes the same nested class name (no split accessors / CS0102). - EndBatch sweeps orphan Frames.<Old>.g.cs, Frames/<Old>.g.cs (+ group folder), and Dialogs.<Old>.g.cs left by a renamed/removed Figma frame. Scoped to full imports so a single-frame rebuild never deletes untouched frames. - Removals go through AssetDatabase.DeleteAsset (keeps .meta + DB consistent); the previously-silent folder-delete failure now logs a warning.
…rt missing nodes - loadUiFont throws a clear error when no fonts are available instead of indexing all[0]. - create_canonical/create_shell (MCP) and create-button/canonical/shell (UI) are now gated behind exportInFlight, so a create can't interleave with and corrupt a running export (they switch figma.currentPage mid-export). - get_screenshot/export_unity collect ids that don't resolve / aren't exportable into a new 'missing' array so callers can tell partial from complete; success entries unchanged.
…ped-reply log - A non-1995 importer port is blocked by Figma's manifest allowlist; the catch now says so instead of the misleading 'Couldn't reach Unity'. - bridgeToken reconnect detaches the old socket's handlers before close so its async onclose can't null the new socket reference (same pattern in disconnectMcp). - A dropped mcp-response (socket not OPEN mid-reconnect) is now logged for diagnosis.
…xis collisions - exportAsync already bakes a node's own drop/inner shadow + layer blur into its PNG, so the non-vanilla path now strips those from style.effects when hasAsset (keeping only live backdrop blur), matching the vanilla path — no more double shadow/blur. - normalizeVariantEntries suffixes (axis_2, …) a fallback axis-key collision so a distinct variant property isn't silently dropped; recognized buckets keep intended first-wins.
…p import - WindowStyles tracked its HideAndDontSave textures and disposes them in OnDisable, so rebuilding _styles (e.g. on domain reload) no longer leaks native textures. The Make* helpers stay static (called from field initializers) and stash into a scratch list the constructor hands to the instance — instance field initializers can't call instance methods (CS0236). - ImportZip prompts before merging into an existing import folder and clears it on Replace, so the result is exactly the zip's contents (no lingering stale assets).
base.materialForRendering already reads this.material (our override), which uploads to the active material — so the explicit pre-call uploaded the full uniform set a second time on every access. Removed it (mirrors ImageBlend/TextBlend/VectorGraphic); the stencil-clone configure is unchanged. Per-rebuild dirty-gating was deferred as unsafe.
Procedural sprite caches persist one .asset per pixel-size and never delete old ones, so _GeneratedSprites grows across re-Forges. Add Window/FigForge/Clean Up Orphaned Generated Sprites: a dependency-checked sweep that deletes only sprites no scene/prefab references (auto-sweep at import start is unsafe — the folder is shared across all imported pages).
The C# CanonicalShape reads shadow/shadows as a fallback, but the TS ButtonShape declared only effects and the exporter never emits shadow/shadows. Add optional shadow?/shadows? to the TS ButtonShape and a 'legacy, read for older manifests; not emitted by 2.0' note on both sides so the contract surface is explicit. No runtime behavior change.
- exporter: Set-based asset-name disambiguation (no compounding suffixes, O(1)); memoize componentSetStateSource per set; depth-cap state recursion; detect shear/non-uniform scale in decomposeFlip and fall back to node.rotation. - vector: reject non-finite path coords (→ PNG fallback) before earcut; use an interior point for ring-containment; widen the AA edge-twin key past 2^20 verts.
- main: pushSelection try/catch + latest-wins token; hold exportInFlight across export_project_unity's collectScreens. - mapper: don't mix snapped/unsnapped space when a parent axis snap inverts. - naming: multi-word canonical refs survive (second token = instance, rest = ref). - traverser: opacity-0 nodes are not exportable (no blank baked PNG). - variants: pick the highest-priority source per axis instead of array first-wins. - ui: preview onerror feedback; log unknown messages from main; hard-cap oversized Unity sends and distinguish serialization vs network failures.
- constant-time WS upgrade token compare (timingSafeEqual); document Content-Length
as an optimization only; follower retries takeover on a connection-level error;
drop the unused screenshotFormat enum; validate number[] wire bytes (0..255 ints);
scrub the caller path from the containment refusal + cap forwarded error length;
save_screenshots returns { written, skipped } and errors when nothing was written.
Escape() wire registry-key literals; reserve sibling section names so distinct sections don't merge; emit a deterministic FigForge.Generated.asmdef.meta (stable GUID); on a failed component-swap re-add the base element/frame so a GameObject is never left without its FigForge component.
- manifest discovery reads a bounded 8KB prefix (not whole files); build catches also Debug.LogError so failures show in the Console. - live-import caps body (413) + queue depth (503), and version-gates the bundle before the destructive swap. - sprite atlas reconfigured in place (no GUID churn); OS-font copies dedupe by content + warn on licensing; fill drawer no longer dirties the object on repaint.
DestroyImmediate the scratch GameObject in a finally so a throwing SaveAsPrefabAsset can't leak it into the scene; cache ShapeFills() to avoid double allocation; size-gate WarmUpGeneratedGraphics' intermediate canvas flushes and drop the duplicate trailing flush.
- Modal.IsOpen uses activeInHierarchy (matches the stack); Stepper gains a real onValueChanged UnityEvent. - LayeredRect.OnValidate queues one delayCall per dirty burst; compositor foreign-hash skips its own present graphics (no self-dirty loop); CachedQuad keeps near-zero-alpha additive output; Text/ImageBlend clean up the present quad in OnDestroy too. - FrameManager warns specifically on an unregistered frame; ScrollRect resets drag/ elastic in OnDisable; Table clamps template cells to the column count.
added 8 commits
June 23, 2026 17:06
- CI/release setup-node caches npm by both lockfiles. - unity/.npmignore excludes EditMode Tests from the packed UPM tarball. - check-canonical-schema.mjs asserts every TS CanonicalKind is parseable by the C# TryParseKind (catches drift); document the intentionally-open CanonicalRef strings and the fontFaceDilate canonical default.
Share one INVISIBLE_PAINT_EPS between traverser and vector (consistent faint-paint handling); escape node.type before it reaches tree innerHTML; parsePath returns null on genuine parse failure vs [] for an empty path; drop the unused planById param from parentDims; document figma.mixed strokeWeight as conservatively visible.
…unity Use a double-underscore dedup suffix; safeFolderName collapses underscore runs, so the suffix can't alias a base that already sanitizes to name_<n>.
- Editor: clamp FrameManager.editorColumns before ApplyModifiedProperties (not after); ShadowData/AssetDiagnostic contract comments match the TS unions; warn when a declared canonical extra-child id isn't in the manifest. - Runtime: Progress min/max setters re-clamp silently (no spurious onValueChanged); ToastHost tints the background by severity when there's no accent + uses the FindFirstObjectByType guard; Ring emits a clean triangle fan for the thickness=1 pie wedge; redirected guards keep the originating trigger/link provenance.
The stage-then-swap refactor declared ProjIndex index inside the try block, but index.screens.Length is read after the swap (post-try) — out of scope. Declare it above the try; the catch rethrows, so the post-swap use only runs when it was assigned.
- Frames can be marked `persistent`: end-user-controlled overlays that Show()/SetVisible(true) reveal OVER the current screen without hiding it, and that navigation never auto-hides. Hide()/SetVisible(false) dismiss them. The flag is preserved across re-imports, and the importer pre-wires NavBinder.screenManager so the runtime Start() lookup is skipped. Adds Frames.Hide/SetVisible codegen + reserved members. - Dropdown popup now clips its option rows with a dedicated rounded stencil source on the Viewport (same pattern as List/Table). The old path reused the captured popup background as the Mask graphic, so a fill-less popup shape wrote no stencil and hid the whole option list. CANONICAL_SCHEMA / CanonicalSchema -> 64. - Radio sets keep exactly one selection: ToggleGroup.allowSwitchOff is false, and the first radio built for a group is turned on when the captured design shipped an all-off set. - Drop the legacy FrameManager.shell single-shell slot (shells now live in Screens via Is Shell + Shell Key).
Granular, list-/grid-shaped accessors so consumer code can read and edit
rows without round-tripping the whole collection through SetItems/SetRows.
Table (FigForgeTable):
- RowCount / ColumnCount / SelectedRow; GetCell / GetRow; SetCell / SetRow
(patch the rendered TMP in place, no Rebuild); AddRow / InsertRow /
RemoveRow (Rebuild, fix up selection); GetCellText / GetRowObject.
- Column headers: GetColumnNames / GetColumnName scrape the captured Header
strip's Cell1..CellM titles, with a `columnNames` override + "Column {n}"
fallback — so a selected row can be printed against its column names.
List (FigForgeList):
- ItemCount / SelectedItem; GetItem / GetTitle / GetSubtitle; SetItem
(in place); AddItem / InsertItem / RemoveItem; GetTitleText /
GetSubtitleText / GetRowObject. Item-shaped (Title/Subtitle), not cells.
Preview-row hardening (both): reads fall back to the rendered row's visible
text when the data model has no entry for a rendered index — design-time
preview rows render but never populate _rows/_items. So Items/Rows,
RowCount/ItemCount, GetRow/GetItem and SelectedRow/SelectedItem all reflect
what's on screen, and a validly-selected preview row never returns null.
Once SetRows/SetItems is called, the data model is authoritative verbatim.
Bump unity + plugin packages to 1.0.58 (lockstep). Plugin/export path unchanged this cut — the release is the Unity runtime accessor surface.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the full set of 115 findings from a thorough read-only review of the repo (plugin · MCP bridge · Unity importer), fixed in 34 focused commits grouped by concern.
Summary by tier
documentAccess: "dynamic-page"migration inmain.ts(6 syncgetNodeById→getNodeByIdAsync;loadAllPagesAsync()at startup) — MCP node lookups/exports were broken per Figma's API contract.SaveAssets(import-stall fix); styled List/Table rows selectable; CI manifest-version + field-parity gate; release builds now run typecheck/tests.Verification
tsc --noEmit+ esbuild build: clean.tsc+ build +npm test: 29/29 pass.scripts/check-canonical-schema.mjs: passes (canonical schema, manifest version, Manifest field parity, new CanonicalKind coverage check).FigForgeLayeredRectmaterial rendering (pixel-identical), styled list/table selection, and live-import/zip-replace flows.Notes
Assets/manifest scan root (kept by design — the perf concern was instead fixed via a bounded prefix read).