Open
Conversation
Collaborator
Author
|
Sorry for the massive PR |
Member
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
egorikftp
reviewed
Mar 4, 2026
WalkthroughThis pull request introduces a complete Figma plugin implementation for Valkyrie's icon vector export workflow. The addition includes project configuration (package.json, tsconfig.json, manifest.json), build infrastructure with ESBuild and WASM integration, main plugin code handling Figma selection and SVG export tasks, and a full-featured UI implemented in TypeScript with controllers, features, and core utilities. The plugin exports selected icons to Kotlin ImageVector format with configurable output options and includes error handling, settings persistence, and bulk action support. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (6)
tools/figma-plugin/src/ui/core/types.ts (1)
1-3: Keepcoreindependent fromfeaturesfor type ownership.
core/types.tsimporting fromfeatures/converterAdaptercreates upward coupling. Consider movingConvertResultinto a shared/core-neutral module and import it from both places.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/figma-plugin/src/ui/core/types.ts` around lines 1 - 3, core/types.ts introduces an upward coupling by importing ConvertResult from features/converterAdapter; instead, extract the ConvertResult type into a neutral shared module (e.g., a new types/shared or types/core-neutral file) and have both features/converterAdapter and core/types.ts import ConvertResult from that shared module; update ConvertResultWithSvg in core/types.ts to reference the relocated ConvertResult and remove the import from features to keep core independent.tools/figma-plugin/scripts/build.mjs (1)
72-72: Watch mode keeps a stale inlined wasm bridge after converter rebuilds.
wasmBridgeScriptis computed once at startup. In--watch, converter updates are not reflected until restart.♻️ Proposed refactor
-const wasmBridgeScript = await buildWasmBridgeScript(); - async function writeInlinedUiHtml() { + const wasmBridgeScript = await buildWasmBridgeScript(); const [uiHtml, uiScript] = await Promise.all([ readFile(srcUiHtmlPath, "utf8"), readFile(distUiJsPath, "utf8"), ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/figma-plugin/scripts/build.mjs` at line 72, The script currently computes wasmBridgeScript once at startup (const wasmBridgeScript = await buildWasmBridgeScript();) so in --watch mode updated converter output isn’t picked up; change the flow to rebuild the bridge on each rebuild by calling buildWasmBridgeScript() inside the watcher/rebuild handler (or replace the top-level const with a getWasmBridgeScript() async call invoked whenever packaging/inlining runs). Locate references to wasmBridgeScript and ensure they call the function (or await buildWasmBridgeScript()) during each rebuild cycle so the latest converter output is inlined.tools/figma-plugin/src/ui/core/status.ts (1)
4-14: Extract the diagnostics delimiter into a shared constant.At Line 4, the parser relies on a literal marker that is also hardcoded in
tools/figma-plugin/src/shared/errorFormatter.ts(lines 7-14). Centralizing that token avoids silent parser drift on future copy edits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/figma-plugin/src/ui/core/status.ts` around lines 4 - 14, Replace the local DIAGNOSTICS_MARKER constant in splitDiagnostics with a single shared constant exported from the shared error formatter module so both parsers use the same token; create or export a named constant (e.g., DIAGNOSTICS_DELIMITER) from the error formatter module where the literal is currently duplicated, update splitDiagnostics to import that constant instead of declaring DIAGNOSTICS_MARKER, and remove the duplicated literal to ensure both splitDiagnostics and the formatter (errorFormatter) reference the same exported symbol.tools/figma-plugin/src/ui/features/render.ts (1)
281-286: Consider including node ID inresultKeyfor uniqueness.The current key
${result.success}:${result.iconName}:${result.fileName}could collide if two nodes have the same name and produce the same fileName. This would cause incorrect scroll position restoration.Since
ConvertResultWithSvglikely has access to the original node ID, including it would guarantee uniqueness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/figma-plugin/src/ui/features/render.ts` around lines 281 - 286, The result key generation in the loop (the resultKey variable used on the result-card element) can collide for nodes with identical iconName/fileName; update the key to include the original node ID from the ConvertResultWithSvg result object (e.g., use result.nodeId or result.node.id if that is the actual property) so the key becomes uniquely tied to the source node, and keep the rest of the logic that sets card.dataset.resultKey and the scroll/restore code unchanged so they use the new unique key.tools/figma-plugin/src/ui/features/settings.ts (1)
16-27: InconsistentpackageNamehandling betweengetSettingsValuesandgetConvertOptions.
getConvertOptionstrimspackageName(line 62), butgetSettingsValuesdoes not (line 18). This means saved settings may include leading/trailing whitespace that conversion will strip, causing a mismatch between persisted and applied values.Consider trimming in both functions or trimming during
applySettingsto keep behavior consistent.Proposed fix
export function getSettingsValues(): PluginSettings { return { - packageName: packageInput.value, + packageName: packageInput.value.trim(), outputFormat: outputFormatInput.value as PluginSettings["outputFormat"],Also applies to: 60-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/figma-plugin/src/ui/features/settings.ts` around lines 16 - 27, Saved packageName includes untrimmed whitespace in getSettingsValues while getConvertOptions trims it, causing persisted vs applied mismatch; update getSettingsValues to return packageName: packageInput.value.trim() and also ensure any applySettings or load path (e.g., applySettings) uses trimmed values so storage and runtime use the same canonical packageName; locate getSettingsValues, getConvertOptions, and applySettings functions to apply the trim consistently to the packageName value.tools/figma-plugin/src/ui/features/converterAdapter.ts (1)
110-112: The.Successsuffix check is dead code and can be removed.The Kotlin
ConverterResultsealed interface defines two variants:
Successwith@SerialName("success")Errorwith@SerialName("error")This means the JSON
typefield will always be either"success"or"error"(the exact values from@SerialName). Thetype.endsWith(".Success")branch will never match.Simplify to exact match
function isSuccessType(type: string): boolean { - return type === "success" || type.endsWith(".Success"); + return type === "success"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/figma-plugin/src/ui/features/converterAdapter.ts` around lines 110 - 112, isSuccessType contains a dead branch checking for ".Success"; remove the endsWith branch and simplify isSuccessType to only return true for the exact "success" string (update the function isSuccessType to perform a strict equality check against "success" and remove any reference to ".Success").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/figma-plugin/README.md`:
- Around line 43-45: The README's runtime artifact list is inconsistent with the
build script: update the list in README (remove or rename
`valkyrie-sdk-figma-converter.mjs` and instead list
`valkyrie-sdk-figma-converter.uninstantiated.mjs` and
`valkyrie-sdk-figma-converter.wasm`) so it matches the artifacts produced by
tools/figma-plugin/scripts/build.mjs; alternatively, if the build should emit
the single `.mjs` file, change the build logic in build.mjs to output
`valkyrie-sdk-figma-converter.mjs` instead—pick one approach and make README and
build.mjs consistent by referencing the exact filenames shown in the script.
In `@tools/figma-plugin/scripts/build.mjs`:
- Around line 53-58: The current string-replacement patching of uninstantiated
into wasmBridgeScript (replacing "export async function instantiate" and the
fetch(new URL('./valkyrie-sdk-figma-converter.wasm',import.meta.url).href)
pattern with the data: wasmBase64) can silently no-op if the converter output
format changes; after performing each .replace() verify the replacement actually
occurred by checking the result for the original substrings and throw a
descriptive Error if still present (e.g., check for "export async function
instantiate" and the original fetch(new
URL...valkyrie-sdk-figma-converter.wasm...) string and fail early), referencing
variables/functions wasmBridgeScript, uninstantiated, instantiate, and
wasmBase64 so the checks are applied immediately after those replace() calls.
- Around line 80-83: The build currently uses
uiHtml.replace("/*__WASM_BRIDGE__*/", ...) and replace("/*__UI_SCRIPT__*/", ...)
which silently no-ops if those placeholder tokens are missing; update the logic
around inlinedHtml/uiHtml to detect missing tokens and fail-fast: check
uiHtml.includes("/*__WASM_BRIDGE__*/") and uiHtml.includes("/*__UI_SCRIPT__*/")
(or verify the resulting inlinedHtml is different from uiHtml) before calling
escapeScriptTag(wasmBridgeScript)/escapeScriptTag(uiScript), and throw a clear
Error (or process.exit(1)) mentioning the missing placeholder(s) so the build
fails instead of producing a broken dist/ui.html.
In `@tools/figma-plugin/src/main/code.ts`:
- Around line 184-194: The decodeUtf8 function's fallback treats each raw byte
as a separate code point and corrupts multi-byte UTF-8 sequences; update
decodeUtf8 to rely on TextDecoder only (remove the manual byte-to-char fallback)
or replace the fallback with a correct UTF‑8 decoding algorithm. Locate the
decodeUtf8 function and either (a) throw or return an error when TextDecoder is
unavailable so callers don't get garbled text, or (b) implement a
standards-compliant UTF‑8 decoder to assemble multi‑byte sequences correctly;
ensure any change preserves the existing return type (string) and is used by
callers that expect proper Unicode (e.g., emoji and non‑ASCII icon names).
In `@tools/figma-plugin/src/ui/controllers/requestController.ts`:
- Around line 59-67: The acknowledgeRequestStart function clears the pending
timeout but doesn't create a new one, so a request moved to activeRequest.state
= "started" can hang indefinitely if "conversion-ready" never arrives; update
acknowledgeRequestStart to call the existing timeout-scheduling helper (or add
one if missing) immediately after setting activeRequest.state = "started" to
start a new timeout that will abort/cleanup the activeRequest (e.g., set state
to "timedout" or call the existing failure handler) if conversion-ready isn't
received; reference acknowledgeRequestStart, clearPendingTimeout, and
activeRequest.state when implementing the new timeout scheduling so the cleanup
logic mirrors other timeout paths.
In `@tools/figma-plugin/src/ui/controllers/runLifecycleState.ts`:
- Around line 26-28: The "timed-out" case currently only calls
setStatus(formatPluginError(createTimeoutError()), "error") and should also
restore prior results like the "superseded" branch does; modify the "timed-out"
handler to detect and restore the previous/loading results (e.g., call the same
restore routine used in the "superseded" branch such as
setResults/restorePreviousResults or reapply the saved loading state) before or
alongside setting the error status so the UI doesn't remain stuck in a loading
state. Ensure you reuse the same symbols/mechanism used by the "superseded"
branch so behavior is consistent with that case.
In `@tools/figma-plugin/src/ui/features/conversion.ts`:
- Around line 117-119: Wrap the per-icon call to convert(icon.svg, icon.name,
options) inside a try/catch so a thrown error for one icon doesn't abort the
batch; on success build the ConvertResultWithSvg as before and push to
nextResults, and on catch push a failure result for that icon (include
icon.name, svg, and an error/status field or message in the ConvertResultWithSvg
shape) so results and status remain complete and the loop continues to the next
icon.
In `@tools/figma-plugin/src/ui/features/converterAdapter.ts`:
- Around line 87-89: The JSON.parse call feeding to toPluginConvertResult can
throw on malformed JSON; wrap the parse and conversion in a try/catch inside the
same function in converterAdapter.ts, catch any error from JSON.parse (or
invalid shape) and return a structured WasmConvertResult error object instead of
letting the exception propagate; specifically, surround JSON.parse(json) as
WasmConvertResult and the call to toPluginConvertResult(...) with a try block
and in catch return an error result that sets the appropriate error fields
expected by WasmConvertResult so callers can handle parse failures gracefully.
In `@tools/figma-plugin/src/ui/ui.html`:
- Around line 714-724: The form labels are plain <span class="field-label">
elements and are not associated with their controls, so update them to proper
<label for="..."> elements matching the control IDs (e.g., replace the span for
"Package name" with <label for="package">, and the span for "Output format" with
<label for="output-format">) to ensure accessibility; also do the same for other
occurrences of span.field-label in this snippet (the label text should remain
the same and the for attribute must exactly match the input/select id).
- Around line 757-760: The status span with id "status" is updated dynamically
but lacks ARIA live-region semantics, so assistive tech won't announce changes;
update the element with appropriate live-region attributes (e.g., add
role="status" or aria-live="polite" and aria-atomic="true") and ensure the
status container or the element with id "status" (and optionally the status-icon
with id "status-icon") is the element receiving the dynamic text so screen
readers will announce changes.
In `@tools/figma-plugin/src/ui/ui.ts`:
- Around line 24-42: The message handler is registered too late and can miss
responses (initSettingsListeners() sends "load-settings" before onMessage(...)
attaches the handler); move the onMessage(createMainMessageHandler({...})) call
to run before any startup side effects (initSettingsListeners(),
initializeBulkActions(), updateBulkActionState(), setStatus or
requestController.requestConversion) so that createMainMessageHandler (with
selectionController and requestController) is attached first, then invoke the
startup functions and attach UI listeners.
---
Nitpick comments:
In `@tools/figma-plugin/scripts/build.mjs`:
- Line 72: The script currently computes wasmBridgeScript once at startup (const
wasmBridgeScript = await buildWasmBridgeScript();) so in --watch mode updated
converter output isn’t picked up; change the flow to rebuild the bridge on each
rebuild by calling buildWasmBridgeScript() inside the watcher/rebuild handler
(or replace the top-level const with a getWasmBridgeScript() async call invoked
whenever packaging/inlining runs). Locate references to wasmBridgeScript and
ensure they call the function (or await buildWasmBridgeScript()) during each
rebuild cycle so the latest converter output is inlined.
In `@tools/figma-plugin/src/ui/core/status.ts`:
- Around line 4-14: Replace the local DIAGNOSTICS_MARKER constant in
splitDiagnostics with a single shared constant exported from the shared error
formatter module so both parsers use the same token; create or export a named
constant (e.g., DIAGNOSTICS_DELIMITER) from the error formatter module where the
literal is currently duplicated, update splitDiagnostics to import that constant
instead of declaring DIAGNOSTICS_MARKER, and remove the duplicated literal to
ensure both splitDiagnostics and the formatter (errorFormatter) reference the
same exported symbol.
In `@tools/figma-plugin/src/ui/core/types.ts`:
- Around line 1-3: core/types.ts introduces an upward coupling by importing
ConvertResult from features/converterAdapter; instead, extract the ConvertResult
type into a neutral shared module (e.g., a new types/shared or
types/core-neutral file) and have both features/converterAdapter and
core/types.ts import ConvertResult from that shared module; update
ConvertResultWithSvg in core/types.ts to reference the relocated ConvertResult
and remove the import from features to keep core independent.
In `@tools/figma-plugin/src/ui/features/converterAdapter.ts`:
- Around line 110-112: isSuccessType contains a dead branch checking for
".Success"; remove the endsWith branch and simplify isSuccessType to only return
true for the exact "success" string (update the function isSuccessType to
perform a strict equality check against "success" and remove any reference to
".Success").
In `@tools/figma-plugin/src/ui/features/render.ts`:
- Around line 281-286: The result key generation in the loop (the resultKey
variable used on the result-card element) can collide for nodes with identical
iconName/fileName; update the key to include the original node ID from the
ConvertResultWithSvg result object (e.g., use result.nodeId or result.node.id if
that is the actual property) so the key becomes uniquely tied to the source
node, and keep the rest of the logic that sets card.dataset.resultKey and the
scroll/restore code unchanged so they use the new unique key.
In `@tools/figma-plugin/src/ui/features/settings.ts`:
- Around line 16-27: Saved packageName includes untrimmed whitespace in
getSettingsValues while getConvertOptions trims it, causing persisted vs applied
mismatch; update getSettingsValues to return packageName:
packageInput.value.trim() and also ensure any applySettings or load path (e.g.,
applySettings) uses trimmed values so storage and runtime use the same canonical
packageName; locate getSettingsValues, getConvertOptions, and applySettings
functions to apply the trim consistently to the packageName value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 28b33597-2a59-4182-9321-a6312afb7a91
⛔ Files ignored due to path filters (1)
tools/figma-plugin/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (29)
.gitignoreDEVELOPMENT.mdtools/figma-plugin/README.mdtools/figma-plugin/manifest.jsontools/figma-plugin/package.jsontools/figma-plugin/scripts/build.mjstools/figma-plugin/src/main/code.tstools/figma-plugin/src/shared/errorFormatter.tstools/figma-plugin/src/shared/messages.tstools/figma-plugin/src/shared/pluginSettings.tstools/figma-plugin/src/ui/controllers/messageHandlers.tstools/figma-plugin/src/ui/controllers/requestController.tstools/figma-plugin/src/ui/controllers/runLifecycleState.tstools/figma-plugin/src/ui/controllers/selectionController.tstools/figma-plugin/src/ui/core/api.tstools/figma-plugin/src/ui/core/dom.tstools/figma-plugin/src/ui/core/state.tstools/figma-plugin/src/ui/core/status.tstools/figma-plugin/src/ui/core/types.tstools/figma-plugin/src/ui/core/utils.tstools/figma-plugin/src/ui/features/bulkActions.tstools/figma-plugin/src/ui/features/conversion.tstools/figma-plugin/src/ui/features/converterAdapter.tstools/figma-plugin/src/ui/features/highlight.tstools/figma-plugin/src/ui/features/render.tstools/figma-plugin/src/ui/features/settings.tstools/figma-plugin/src/ui/ui.htmltools/figma-plugin/src/ui/ui.tstools/figma-plugin/tsconfig.json
…ccessibility while cleaning post-refactor boundaries
… progress request
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tools/figma-plugin/src/ui/controllers/selectionController.ts (1)
71-103: Consider simplifying redundantscheduleAutoConversioncalls.When
uiState === "auto-export-disabled"(line 93), the subsequentscheduleAutoConversion()call will always returnfalsesinceautoExportInput.checkedis false. The call is harmless (it clears any pending timeout) but could be more explicit:if (uiState === "auto-export-disabled") { - scheduleAutoConversion(); + // Clear any pending auto-conversion since auto-export is disabled + if (autoRunTimeoutId !== null) { + window.clearTimeout(autoRunTimeoutId); + autoRunTimeoutId = null; + } showSelectionEmptyState(uiState); return; }Alternatively, a brief comment explaining the intent would suffice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/figma-plugin/src/ui/controllers/selectionController.ts` around lines 71 - 103, In handleSelectionChanged, the scheduleAutoConversion() call inside the uiState === "auto-export-disabled" branch is redundant because autoExportInput.checked is false so scheduleAutoConversion will always return false; update the branch in the deriveSelectionUiState flow to either remove that scheduleAutoConversion() call (leaving showSelectionEmptyState(uiState)) or keep it but add a short explanatory comment clarifying it’s only used to clear any pending timeout, referencing the handleSelectionChanged and scheduleAutoConversion functions so reviewers can find and verify the change.tools/figma-plugin/src/shared/pluginSettings.ts (1)
37-44: Ambiguous return value conflates validnullwith parse failure.
parseAutoMirrorOptionreturnsnullfor both valid "keep source" semantics (when input isnull,undefined, or"") and for invalid inputs (line 43). This makes the caller unable to distinguish between a validnullselection and a parse failure.Currently this doesn't cause a bug because
DEFAULT_PLUGIN_SETTINGS.autoMirroris alsonull, but if the default changes, the nullish coalescing at line 77 would silently override valid explicitnullvalues.Consider returning a discriminated result or using a different sentinel for parse failure if you want to guard against future breakage:
♻️ Optional defensive refactor
-export function parseAutoMirrorOption(value: unknown): AutoMirrorOption | null { - if (value === null || value === undefined) return null; +export function parseAutoMirrorOption(value: unknown): { valid: true; value: AutoMirrorOption } | { valid: false } { + if (value === null || value === undefined) return { valid: true, value: null }; if (typeof value === "boolean") return value; - if (value === "") return null; - if (value === "true") return true; - if (value === "false") return false; - return null; + if (value === "") return { valid: true, value: null }; + if (value === "true") return { valid: true, value: true }; + if (value === "false") return { valid: true, value: false }; + return { valid: false }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/figma-plugin/src/shared/pluginSettings.ts` around lines 37 - 44, parseAutoMirrorOption currently returns null both for a legitimate "keep source" selection and for parse failures, making callers (e.g., the code that uses nullish coalescing around DEFAULT_PLUGIN_SETTINGS.autoMirror) unable to distinguish them; change parseAutoMirrorOption to return a discriminated result type (for example { success: true, value: AutoMirrorOption | null } | { success: false, error: string }) instead of plain AutoMirrorOption | null, update callers to check success and handle success.value (including explicit null) versus failure.error, and adjust the code that currently uses nullish coalescing so it only falls back when success is false rather than when value is null.tools/figma-plugin/src/ui/features/settings.ts (1)
16-27: Consolidate duplicated settings mapping to avoid drift.Line 16-27 and Line 60-70 both read almost the same UI fields. Extract a shared helper so new settings can’t silently diverge between save payloads and conversion options.
♻️ Suggested refactor
+function readCurrentSettings(): PluginSettings { + return { + packageName: packageInput.value.trim(), + outputFormat: outputFormatInput.value as PluginSettings["outputFormat"], + useComposeColors: useComposeColorsInput.checked, + addTrailingComma: addTrailingCommaInput.checked, + useExplicitMode: useExplicitModeInput.checked, + usePathDataString: usePathDataStringInput.checked, + autoMirror: parseAutoMirrorOption(autoMirrorInput.value), + autoExport: autoExportInput.checked, + }; +} + export function getSettingsValues(): PluginSettings { - return { - packageName: packageInput.value.trim(), - outputFormat: outputFormatInput.value as PluginSettings["outputFormat"], - useComposeColors: useComposeColorsInput.checked, - addTrailingComma: addTrailingCommaInput.checked, - useExplicitMode: useExplicitModeInput.checked, - usePathDataString: usePathDataStringInput.checked, - autoMirror: parseAutoMirrorOption(autoMirrorInput.value), - autoExport: autoExportInput.checked, - }; + return readCurrentSettings(); } @@ export function getConvertOptions(): ConvertOptions { + const settings = readCurrentSettings(); return { - packageName: packageInput.value.trim(), - outputFormat: outputFormatInput.value as ConvertOptions["outputFormat"], - useComposeColors: useComposeColorsInput.checked, - addTrailingComma: addTrailingCommaInput.checked, - useExplicitMode: useExplicitModeInput.checked, - usePathDataString: usePathDataStringInput.checked, + packageName: settings.packageName, + outputFormat: settings.outputFormat as ConvertOptions["outputFormat"], + useComposeColors: settings.useComposeColors, + addTrailingComma: settings.addTrailingComma, + useExplicitMode: settings.useExplicitMode, + usePathDataString: settings.usePathDataString, indentSize: 4, - autoMirror: parseAutoMirrorOption(autoMirrorInput.value), + autoMirror: settings.autoMirror, }; }Also applies to: 60-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/figma-plugin/src/ui/features/settings.ts` around lines 16 - 27, The mapping of UI fields to settings is duplicated (getSettingsValues and the block at lines ~60-70), which risks divergence; extract a single helper (e.g., buildSettingsFromInputs or mapInputsToSettings) that reads packageInput, outputFormatInput, useComposeColorsInput, addTrailingCommaInput, useExplicitModeInput, usePathDataStringInput, autoMirrorInput (via parseAutoMirrorOption), and autoExportInput and returns the PluginSettings object, then replace both getSettingsValues and the other mapping site to call this helper so all save payloads and conversion options use the same source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/figma-plugin/scripts/build.mjs`:
- Around line 121-123: The catch block that currently only logs the error using
process.stderr.write for the "Failed to inline ui.html" case should not silently
continue in non-watch builds; update the catch in build.mjs so that after
writing the error it either rethrows the caught error or calls process.exit(1)
when the build is not running in watch mode (check the existing watch flag
variable, e.g., isWatch/watchMode), ensuring inlining failures fail the build
rather than only being logged.
---
Nitpick comments:
In `@tools/figma-plugin/src/shared/pluginSettings.ts`:
- Around line 37-44: parseAutoMirrorOption currently returns null both for a
legitimate "keep source" selection and for parse failures, making callers (e.g.,
the code that uses nullish coalescing around DEFAULT_PLUGIN_SETTINGS.autoMirror)
unable to distinguish them; change parseAutoMirrorOption to return a
discriminated result type (for example { success: true, value: AutoMirrorOption
| null } | { success: false, error: string }) instead of plain AutoMirrorOption
| null, update callers to check success and handle success.value (including
explicit null) versus failure.error, and adjust the code that currently uses
nullish coalescing so it only falls back when success is false rather than when
value is null.
In `@tools/figma-plugin/src/ui/controllers/selectionController.ts`:
- Around line 71-103: In handleSelectionChanged, the scheduleAutoConversion()
call inside the uiState === "auto-export-disabled" branch is redundant because
autoExportInput.checked is false so scheduleAutoConversion will always return
false; update the branch in the deriveSelectionUiState flow to either remove
that scheduleAutoConversion() call (leaving showSelectionEmptyState(uiState)) or
keep it but add a short explanatory comment clarifying it’s only used to clear
any pending timeout, referencing the handleSelectionChanged and
scheduleAutoConversion functions so reviewers can find and verify the change.
In `@tools/figma-plugin/src/ui/features/settings.ts`:
- Around line 16-27: The mapping of UI fields to settings is duplicated
(getSettingsValues and the block at lines ~60-70), which risks divergence;
extract a single helper (e.g., buildSettingsFromInputs or mapInputsToSettings)
that reads packageInput, outputFormatInput, useComposeColorsInput,
addTrailingCommaInput, useExplicitModeInput, usePathDataStringInput,
autoMirrorInput (via parseAutoMirrorOption), and autoExportInput and returns the
PluginSettings object, then replace both getSettingsValues and the other mapping
site to call this helper so all save payloads and conversion options use the
same source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7cc847ab-631f-4af1-a67e-604a3f6ff3b5
⛔ Files ignored due to path filters (1)
tools/figma-plugin/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
DEVELOPMENT.mdtools/figma-plugin/DEVELOPMENT.mdtools/figma-plugin/README.mdtools/figma-plugin/package.jsontools/figma-plugin/scripts/build.mjstools/figma-plugin/src/fast-text-encoding.d.tstools/figma-plugin/src/main/code.tstools/figma-plugin/src/shared/errorFormatter.tstools/figma-plugin/src/shared/pluginSettings.tstools/figma-plugin/src/ui/controllers/requestController.tstools/figma-plugin/src/ui/controllers/runLifecycleState.tstools/figma-plugin/src/ui/controllers/selectionController.tstools/figma-plugin/src/ui/core/status.tstools/figma-plugin/src/ui/core/types.tstools/figma-plugin/src/ui/features/conversion.tstools/figma-plugin/src/ui/features/converterAdapter.tstools/figma-plugin/src/ui/features/render.tstools/figma-plugin/src/ui/features/settings.tstools/figma-plugin/src/ui/ui.htmltools/figma-plugin/src/ui/ui.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- tools/figma-plugin/package.json
- tools/figma-plugin/src/ui/features/converterAdapter.ts
- tools/figma-plugin/src/ui/core/types.ts
- tools/figma-plugin/README.md
- DEVELOPMENT.md
There was a problem hiding this comment.
🧹 Nitpick comments (4)
tools/figma-plugin/README.md (1)
16-20: Clarify command execution context (repo root vs plugin dir).Line 16–20 and Line 24 currently imply local package execution, but the PR objective uses root-level
pnpm --dir tools/figma-plugin .... Adding one explicit note will reduce onboarding friction.✍️ Suggested doc patch
## Scripts +Run these commands from `tools/figma-plugin/` (or from repo root using `pnpm --dir tools/figma-plugin <script>`). + - `pnpm build:converter` - compile Kotlin/Wasm converter executable assets - `pnpm build` - build plugin assets into `dist/` - `pnpm build:all` - build converter + plugin assets - `pnpm watch` - watch mode for development - `pnpm typecheck` - TypeScript checksAlso applies to: 24-26
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/figma-plugin/README.md` around lines 16 - 20, Update the README entries for the listed commands (pnpm build:converter, pnpm build, pnpm build:all, pnpm watch, pnpm typecheck and the commands around lines 24-26) to explicitly state the intended execution context by showing the root-level invocation form (pnpm --dir tools/figma-plugin <command>) and also an alternative for running them from inside the plugin directory (cd tools/figma-plugin && pnpm <command>); add a single succinct note above the command list clarifying which form is preferred and when to use each so contributors know whether to run commands from the repo root or from the plugin folder.tools/figma-plugin/DEVELOPMENT.md (2)
3-9: Consider clarifying the working directory assumption.The commands implicitly assume execution from the
tools/figma-plugin/directory (evidenced by the pnpm commands and relative Gradle paths), but this isn't explicitly stated. Adding a brief note at the top would help prevent confusion for new contributors.📝 Suggested improvement
## Build and run +> **Note:** Run all commands from the `tools/figma-plugin/` directory. + - Build converter for Wasm executable: `../../gradlew -p ../../ :sdk:figma:converter:compileProductionExecutableKotlinWasmJs`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/figma-plugin/DEVELOPMENT.md` around lines 3 - 9, Update the "Build and run" section in DEVELOPMENT.md to explicitly state the working directory assumption: add a short note that the listed commands (e.g., the Gradle command that references ../../gradlew and the pnpm commands like pnpm install, pnpm build, pnpm build:all, pnpm watch) are intended to be run from the tools/figma-plugin/ directory (or show the equivalent absolute/relative command variants), so contributors know where to execute them; place this note at the top of the section near the header so it’s immediately visible.
5-5: Simplify the Gradle command by removing the redundant project flag.The
-p ../../flag is redundant since../../gradlewalready executes the wrapper from the repository root, which Gradle uses as the project directory by default.♻️ Proposed simplification
-- Build converter for Wasm executable: `../../gradlew -p ../../ :sdk:figma:converter:compileProductionExecutableKotlinWasmJs` +- Build converter for Wasm executable: `../../gradlew :sdk:figma:converter:compileProductionExecutableKotlinWasmJs`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/figma-plugin/DEVELOPMENT.md` at line 5, Update the build instruction in DEVELOPMENT.md to remove the redundant project flag by changing the command that currently includes `-p ../../` so it reads without that flag; locate the line containing the Wasm build command string `../../gradlew -p ../../ :sdk:figma:converter:compileProductionExecutableKotlinWasmJs` and replace it with the simplified `../../gradlew :sdk:figma:converter:compileProductionExecutableKotlinWasmJs`.tools/figma-plugin/src/ui/features/settings.ts (1)
9-20: Consider validatingoutputFormatvalue instead of bare type assertion.The cast on line 12 assumes the DOM value is always valid. While the controlled plugin UI makes this safe in practice, a validation helper (similar to
parseAutoMirrorOptionon line 17) would provide consistent type safety.🔧 Optional improvement for consistency
You could leverage the existing
asOutputFormathelper frompluginSettings.ts(which returnsnullfor invalid values) and fall back to a default:+import { DEFAULT_PLUGIN_SETTINGS } from "../../shared/pluginSettings"; + function readCurrentSettings(): PluginSettings { + const outputFormatValue = outputFormatInput.value; + const outputFormat = outputFormatValue === "backing_property" || outputFormatValue === "lazy_property" + ? outputFormatValue + : DEFAULT_PLUGIN_SETTINGS.outputFormat; + return { packageName: packageInput.value.trim(), - outputFormat: outputFormatInput.value as PluginSettings["outputFormat"], + outputFormat, useComposeColors: useComposeColorsInput.checked,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/figma-plugin/src/ui/features/settings.ts` around lines 9 - 20, The readCurrentSettings function currently casts outputFormatInput.value with a bare type assertion; replace that with validation using the asOutputFormat helper (from pluginSettings.ts) to parse outputFormatInput.value and, if it returns null, fall back to a sensible default before returning the PluginSettings object. Keep the rest of the fields as-is (packageInput, useComposeColorsInput, parseAutoMirrorOption, etc.) and ensure the symbol names used are outputFormatInput, asOutputFormat, and readCurrentSettings so the change is easy to locate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tools/figma-plugin/DEVELOPMENT.md`:
- Around line 3-9: Update the "Build and run" section in DEVELOPMENT.md to
explicitly state the working directory assumption: add a short note that the
listed commands (e.g., the Gradle command that references ../../gradlew and the
pnpm commands like pnpm install, pnpm build, pnpm build:all, pnpm watch) are
intended to be run from the tools/figma-plugin/ directory (or show the
equivalent absolute/relative command variants), so contributors know where to
execute them; place this note at the top of the section near the header so it’s
immediately visible.
- Line 5: Update the build instruction in DEVELOPMENT.md to remove the redundant
project flag by changing the command that currently includes `-p ../../` so it
reads without that flag; locate the line containing the Wasm build command
string `../../gradlew -p ../../
:sdk:figma:converter:compileProductionExecutableKotlinWasmJs` and replace it
with the simplified `../../gradlew
:sdk:figma:converter:compileProductionExecutableKotlinWasmJs`.
In `@tools/figma-plugin/README.md`:
- Around line 16-20: Update the README entries for the listed commands (pnpm
build:converter, pnpm build, pnpm build:all, pnpm watch, pnpm typecheck and the
commands around lines 24-26) to explicitly state the intended execution context
by showing the root-level invocation form (pnpm --dir tools/figma-plugin
<command>) and also an alternative for running them from inside the plugin
directory (cd tools/figma-plugin && pnpm <command>); add a single succinct note
above the command list clarifying which form is preferred and when to use each
so contributors know whether to run commands from the repo root or from the
plugin folder.
In `@tools/figma-plugin/src/ui/features/settings.ts`:
- Around line 9-20: The readCurrentSettings function currently casts
outputFormatInput.value with a bare type assertion; replace that with validation
using the asOutputFormat helper (from pluginSettings.ts) to parse
outputFormatInput.value and, if it returns null, fall back to a sensible default
before returning the PluginSettings object. Keep the rest of the fields as-is
(packageInput, useComposeColorsInput, parseAutoMirrorOption, etc.) and ensure
the symbol names used are outputFormatInput, asOutputFormat, and
readCurrentSettings so the change is easy to locate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: abea7525-da1f-4599-a843-966df7d9976c
📒 Files selected for processing (4)
tools/figma-plugin/DEVELOPMENT.mdtools/figma-plugin/README.mdtools/figma-plugin/scripts/build.mjstools/figma-plugin/src/ui/features/settings.ts
Member
|
@t-regbs Is there any way to check if the Figma plugin built successfully? Something like this: Valkyrie/.github/workflows/validation.yml Line 42 in 2d3bddf |
Collaborator
Author
Yeah that would be |
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
2 participants
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.
figma-plugin-demo-hq-2560.mp4
figma-plugin-demo-2-hq-2560.mp4
PR adds the first end-to-end Figma plugin workflow for ImageVector generation.
It includes:
ImageVectorthrough the Wasm-backed converterArchitecture
tools/figma-plugin/src/main/code.tsruns in Figma main runtimetools/figma-plugin/src/ui/ui.tsis the UI entrypointcore/contains UI primitives (api,dom,state,status,types,utils)controllers/contains orchestration (messageHandlers, request/selection lifecycle)features/contains product behavior (settings,conversion,render,bulkActions, etc.)tools/figma-plugin/src/shared/messages,pluginSettings,errorFormatter)Validation
pnpm --dir tools/figma-plugin build:all[build everything needed for plugin runtime]build:converter[compile Kotlin/Wasm converter artifacts]:../../gradlew -p ../../ :sdk:figma:converter:compileProductionExecutableKotlinWasmJsbuild[bundle plugin main/UI and inline wasm bridge into dist/ui.html]:node ./scripts/build.mjspnpm --dir tools/figma-plugin typecheck[run TypeScript checks without emitting files]Testing on Figma
Plugins -> Development -> Import plugin from manifest...tools/figma-plugin/manifest.jsonPlugins -> Development -> Hot Reload pluginValkyrie ImageVector Export📝 Changelog
If this PR introduces user-facing changes, please update the relevant Unreleased section in changelogs: