Open
Conversation
dosubot
bot
added
the
size:XXL
🎭 Playwright Tests:
|
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 02/06/2026, 03:56:17 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
Contributor
📝 WalkthroughWalkthroughAdds a new painter feature: a Vue SFC painter widget and usePainter composable, painter widget type/registry and constructor, core extension enforcing Painter node size, localization strings, and widget registration in ComfyWidgets. Changes
Sequence DiagramsequenceDiagram
actor User
participant WidgetPainter as "WidgetPainter\n(renders UI)"
participant usePainter as "usePainter\n(state + draw)"
participant Canvas as "Canvas\n(offscreen/main)"
participant Server as "Server\n(mask upload)"
User->>WidgetPainter: open / interact UI (controls, pointer)
WidgetPainter->>usePainter: handlePointerDown/Move/Up, control updates
usePainter->>Canvas: convert coords, render stroke preview onto offscreen
Canvas->>usePainter: composite stroke into main canvas
User->>WidgetPainter: clear / save
WidgetPainter->>usePainter: clear() / serialize()
usePainter->>Server: upload serialized mask
Server-->>usePainter: upload result (URL/status)
usePainter-->>WidgetPainter: update modelValue / widget state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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
requested review from
KarryCharon,
Yorha4D,
christian-byrne and
shinshin86
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 22.5 kB (baseline 22.5 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 840 kB (baseline 840 kB) • 🔴 +3 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 69 kB (baseline 69 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 11 added / 11 removed Panels & Settings — 410 kB (baseline 410 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 24 added / 24 removed User & Accounts — 16 kB (baseline 16 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 5 added / 5 removed Editors & Dialogs — 3.47 kB (baseline 3.47 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 2 added / 2 removed UI Components — 37.8 kB (baseline 37.8 kB) • ⚪ 0 BReusable component library chunks
Status: 8 added / 8 removed Data & Services — 2.1 MB (baseline 2.1 MB) • 🔴 +710 BStores, services, APIs, and repositories
Status: 12 added / 12 removed Utilities & Hooks — 234 kB (baseline 234 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 14 added / 14 removed Vendor & Third-Party — 9.37 MB (baseline 9.37 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Status: 1 added / 1 removed Other — 7.13 MB (baseline 7.1 MB) • 🔴 +31.1 kBBundles that do not match a named category
Status: 109 added / 108 removed |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@src/components/painter/WidgetPainter.vue`:
- Around line 71-99: The toolbar uses raw <button> elements for the brush/eraser
toggles; replace them with the project's shared button components (e.g.,
TextButton or IconTextButton) to keep consistent styling and behavior: import
the desired component(s) from the button components, register them in the
component, and swap each <button> (the ones bound to tool and PAINTER_TOOLS and
the clear action at the other mentioned lines) for the shared component while
preserving the existing :class binding, `@click` handlers (e.g., `@click`="tool =
PAINTER_TOOLS.BRUSH"/ERASER), and translated label slots ({{ $t('painter.brush')
}} / {{ $t('painter.eraser') }}); if the shared component exposes props for
variant/active state or class forwarding, use those instead of duplicating
styles so the active state uses the component's selected variant.
- Around line 275-309: You’re using a props object (const props =
defineProps<...>()) instead of Vue 3.5-style destructuring; replace it with
destructured props like const { nodeId } = defineProps<{ nodeId: string }>() (or
use const { nodeId } = toRefs(defineProps(...)) if you need reactivity) and then
pass nodeId directly into usePainter (replace usePainter(props.nodeId, {
canvasEl, modelValue }) with usePainter(nodeId, { canvasEl, modelValue })); also
update any other references to props.nodeId to use the new nodeId binding.
- Around line 129-136: Replace the PrimeVue ColorPicker usage in
WidgetPainter.vue (instances bound to brushColorDisplay at the template around
the ColorPicker components and the :pt prop) with a native <input type="color">,
remove the :pt prop and any Tailwind classes using the `!` prefix, and delete
the PrimeVue import/registration for ColorPicker; ensure the new <input
type="color"> is v-model bound to brushColorDisplay (or the same reactive
prop/computed used by the current ColorPicker), apply equivalent Tailwind 4
classes (without `!`) to match sizing/rounded/border styles (e.g., use h-4 w-8
rounded-full border-none or wrapper styling), and update the other occurrence
(the block around lines 238–242) similarly so both uses are consistent and no
`!` overrides remain.
- Around line 266-268: The file directly imports PrimeVue's ColorPicker (import
ColorPicker from 'primevue/colorpicker') which violates the guideline; replace
this raw import and any direct usage of ColorPicker in WidgetPainter.vue with
your project’s approved wrapper component (e.g., FormColorPicker or
ColorCustomizationSelector): remove the ColorPicker import, import the chosen
wrapper instead, update template bindings/props/events to match the wrapper's
API, and keep existing related symbols (useElementSize, computed,
useTemplateRef) unchanged so layout and reactivity continue to work; ensure prop
names and v-model/event names are adapted to the wrapper’s contract.
In `@src/composables/painter/usePainter.ts`:
- Around line 509-545: Wrap the upload and JSON parsing in serializeValue() in a
try/catch around the api.fetchApi(...) call and the subsequent resp.json() so
network failures and invalid JSON are caught; on error set isDirty.value
appropriately, show a localized toast via
useToastStore().addAlert(t('painter.uploadErrorGeneric')) and rethrow or return
modelValue.value as appropriate. Also handle non-200 statuses inside the try by
using t('painter.uploadError', { status: resp.status, statusText:
resp.statusText }) for the toast instead of the hardcoded string, and only set
modelValue.value/isDirty.value after successful parsing; import useI18n() at the
top of the file and call const { t } = useI18n() inside the module, and add the
suggested keys painter.uploadError and painter.uploadErrorGeneric to
src/locales/en/main.json.
In `@src/renderer/extensions/vueNodes/widgets/composables/usePainterWidget.ts`:
- Around line 6-16: Convert the exported const to a function declaration named
usePainterWidget that returns ComfyWidgetConstructorV2; inside the returned
constructor function (the function that receives node: LGraphNode and inputSpec:
InputSpecV2) add a runtime type guard for inputSpec.default so you only pass a
string to node.addWidget (e.g. check typeof inputSpec.default === 'string' and
fall back to '' when undefined or of the wrong type) instead of casting (remove
the direct cast (inputSpec.default as string)); keep the widget options ({
serialize: true, canvasOnly: false }) and return the created widget from
node.addWidget.
src/renderer/extensions/vueNodes/widgets/composables/usePainterWidget.ts
Show resolved
Hide resolved
jtydhr88
force-pushed
the
feat/painter-node
branch
from
1d6c1e6 to
3985f12
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/composables/painter/usePainter.ts`:
- Around line 18-23: Export the PainterTool type and let TypeScript infer
literal types for PAINTER_TOOLS by removing the explicit Record<string,
PainterTool> annotation; specifically, change the declaration so PainterTool is
exported (export type PainterTool = 'brush' | 'eraser') and declare
PAINTER_TOOLS as an exported const with the object and trailing as const (export
const PAINTER_TOOLS = { BRUSH: 'brush', ERASER: 'eraser' } as const) so external
usage like tool.value in WidgetPainter.vue can use the exported PainterTool type
and the correct literal types are preserved.
- Around line 576-581: In the restoreCanvas code path (the block that draws the
loaded image into the canvas, e.g., in restoreCanvas within usePainter.ts)
change isDirty.value from true to false so the canvas is marked clean after
restoration; leave hasStrokes as-is if stroke state should reflect content. This
ensures the subsequent serialization/upload logic (the branch that only skips
upload when isDirty is false) correctly treats a restored canvas as not needing
an upload.
In `@src/extensions/core/painter.ts`:
- Around line 6-11: The nodeCreated callback is unnecessarily declared async
even though it contains no await; remove the async keyword from the nodeCreated
method (the one checking node.constructor.comfyClass === 'PainterNode' and
calling node.setSize) so it no longer returns an implicit Promise and remains a
plain synchronous function.
|
Would it be difficult to add a "mask only" brush mode (could be a toggle in the color picker to keep the interface uncluttered)? I can see it being useful to add bits to the mask output without actually necessarily painting over the image. |
jtydhr88
force-pushed
the
feat/painter-node
branch
from
3985f12 to
80002f8
Compare
jtydhr88
force-pushed
the
feat/painter-node
branch
from
80002f8 to
1d19d60
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/components/painter/WidgetPainter.vue`:
- Around line 148-167: The numeric opacity input in WidgetPainter.vue currently
uses `@click.prevent` which stops default click behavior and blocks caret
placement; change the listener to `@click.stop` (or remove it entirely) so the
click doesn't bubble to parents but still allows cursor placement, keeping the
existing `@change` handler that updates brushOpacityPercent and the input value
intact.
- Around line 64-69: The label elements for the painter controls are
inconsistently hidden in compact mode — keep layout consistent by either adding
the same v-if="!compact" guard to every label div that renders translation keys
(e.g. the divs showing $t('painter.tool'), $t('painter.size'),
$t('painter.color'), $t('painter.hardness') and other label divs between lines
105–192) or replace them with a small shared Label component/slot that
encapsulates the compact check (e.g. <Label :compact="compact">...</Label>) and
use that everywhere so the compact-mode visibility logic is centralized.
In `@src/composables/painter/usePainter.ts`:
- Around line 403-421: The resizeCanvas function currently captures prevData and
restores it with getCtx()?.putImageData, which silently clips content when
canvasWidth or canvasHeight shrink; update resizeCanvas to either (A) add a
clear inline comment above the prevData/putImageData block explaining that
silent clipping on shrink is intentional and acceptable for this app (mention
canvasWidth, canvasHeight, prevData, putImageData, getCtx, hasBaseSnapshot), or
(B) implement proportional scaling: create an offscreen/temp canvas, draw the
prevData into it, then use drawImage to scale the temp canvas into the new
el.width/el.height before replacing the main canvas (use getCtx/getCtx of temp,
drawImage instead of putImageData, and preserve strokeCanvas/baseCanvas state),
ensuring hasBaseSnapshot, strokeCtx and baseCtx handling remains consistent;
choose A for minimal change or B if you need to preserve painted content on
resize.
🧹 Nitpick comments (5)
src/components/painter/WidgetPainter.vue (1)
338-350: Usestringinstead ofunknownfor color setter parameter types.Both
brushColorDisplayandbackgroundColorDisplaysetters acceptunknown, but the native<input type="color">always produces astring. Usingunknownhere bypasses type safety unnecessarily.♻️ Proposed fix
const brushColorDisplay = computed({ get: () => toHexFromFormat(brushColor.value, 'hex'), - set: (val: unknown) => { + set: (val: string) => { brushColor.value = toHexFromFormat(val, 'hex') } }) const backgroundColorDisplay = computed({ get: () => toHexFromFormat(backgroundColor.value, 'hex'), - set: (val: unknown) => { + set: (val: string) => { backgroundColor.value = toHexFromFormat(val, 'hex') } })src/composables/painter/usePainter.ts (4)
499-510:isCanvasEmptyscans every pixel — expensive at large canvas sizes.At the maximum canvas size of 4096×4096, this iterates over ~67 million alpha values before every
serializeValuecall. Consider an early-out approach: track whether any strokes have been made (you already havehasStrokes) and only fall back to the pixel scan when necessary, or sample a subset of pixels for a fast-path check.Currently,
hasStrokesalready short-circuits on Line 500, which helps for the no-stroke case. However, afterhandleClear(Line 429:hasStrokes = false), this is already handled. The remaining concern is the full-canvas-cleared case where strokes existed but were entirely erased — requiring the full scan. This is a minor performance concern since it only fires on serialization.
78-95: Unsafe type assertions onnode.propertiesvalues.Lines 83–94 cast property values with
as PainterTool,as number,as stringwithout runtime validation. If the serialized properties contain unexpected types (e.g., from a corrupted workflow or schema migration), these casts silently produce incorrect state.Consider adding lightweight runtime guards:
♻️ Proposed fix (example for one property)
- if (props.painterTool != null) tool.value = props.painterTool as PainterTool + if (props.painterTool === 'brush' || props.painterTool === 'eraser') + tool.value = props.painterTool
617-621: Deep watchers on large store objects may cause performance overhead.Lines 617–620 watch
nodeOutputStore.nodeOutputsandnodeOutputStore.nodePreviewImageswith{ deep: true }. If these stores hold outputs for many nodes, every output change in the graph triggers the watcher. Consider narrowing the watch to only the relevant node's outputs, or using a computed that extracts the specific node's data.♻️ Example
- watch(() => nodeOutputStore.nodeOutputs, updateInputImageUrl, { deep: true }) - watch(() => nodeOutputStore.nodePreviewImages, updateInputImageUrl, { - deep: true - }) + const relevantOutputs = computed(() => { + const node = litegraphNode.value + const inputNode = node?.getInputNode(0) + if (!inputNode) return undefined + return nodeOutputStore.getNodeImageUrls(inputNode) + }) + watch(relevantOutputs, updateInputImageUrl)
32-36: AcceptMaybeRef<string>fornodeIdto handle reactive updates.While
nodeIdcurrently arrives as a plain string, thelitegraphNodecomputed (line 60) closes over it and won't update if the prop changes. Other composables in the codebase (e.g.,useNodePreviewState) useMaybeRefOrGetter<string>withtoValue()for this pattern. Consider aligningusePainterwith that approach for consistency and to guard against future component reuse scenarios.
Comment on lines
+64
to
+69
| <div | ||
| v-if="!compact" | ||
| class="flex w-28 items-center truncate text-sm text-muted-foreground" | ||
| > | ||
| {{ $t('painter.tool') }} | ||
| </div> |
Contributor
There was a problem hiding this comment.
Inconsistent compact-mode label visibility.
The "Tool" (Line 65) and "Size" (Line 106) labels are hidden in compact mode via v-if="!compact", but the "Color" (Line 129), "Hardness" (Line 171), and similar labels below lack this guard. In compact mode with grid-cols-1, these labels will still render as standalone full-width rows, making the layout inconsistent.
Either apply v-if="!compact" to all label divs, or wrap the labels in a shared template/component to centralize the compact logic.
Also applies to: 105-109, 127-192
🤖 Prompt for AI Agents
In `@src/components/painter/WidgetPainter.vue` around lines 64 - 69, The label
elements for the painter controls are inconsistently hidden in compact mode —
keep layout consistent by either adding the same v-if="!compact" guard to every
label div that renders translation keys (e.g. the divs showing
$t('painter.tool'), $t('painter.size'), $t('painter.color'),
$t('painter.hardness') and other label divs between lines 105–192) or replace
them with a small shared Label component/slot that encapsulates the compact
check (e.g. <Label :compact="compact">...</Label>) and use that everywhere so
the compact-mode visibility logic is centralized.
Comment on lines
+148
to
+167
| <input | ||
| type="number" | ||
| :value="brushOpacityPercent" | ||
| min="0" | ||
| max="100" | ||
| step="1" | ||
| class="w-7 appearance-none border-0 bg-transparent text-right text-xs text-node-text-muted outline-none [&::-webkit-inner-spin-button]:appearance-none [&::-webkit-outer-spin-button]:appearance-none [-moz-appearance:textfield]" | ||
| @click.prevent | ||
| @change=" | ||
| (e) => { | ||
| const val = Math.min( | ||
| 100, | ||
| Math.max(0, Number((e.target as HTMLInputElement).value)) | ||
| ) | ||
| brushOpacityPercent = val | ||
| ;(e.target as HTMLInputElement).value = String(val) | ||
| } | ||
| " | ||
| />%</span | ||
| > |
Contributor
There was a problem hiding this comment.
Opacity number input @click.prevent blocks caret placement.
Line 155: @click.prevent on the opacity <input type="number"> prevents the default click behavior, which means users cannot click to position the text cursor inside the field. This degrades the editing experience. If the intent is to prevent the parent from receiving the click, .stop is more appropriate.
♻️ Proposed fix
- `@click.prevent`
+ `@click.stop`🤖 Prompt for AI Agents
In `@src/components/painter/WidgetPainter.vue` around lines 148 - 167, The numeric
opacity input in WidgetPainter.vue currently uses `@click.prevent` which stops
default click behavior and blocks caret placement; change the listener to
`@click.stop` (or remove it entirely) so the click doesn't bubble to parents but
still allows cursor placement, keeping the existing `@change` handler that updates
brushOpacityPercent and the input value intact.
Comment on lines
+403
to
+421
| function resizeCanvas() { | ||
| const el = canvasEl.value | ||
| if (!el) return | ||
| const prevData = | ||
| el.width > 0 && el.height > 0 | ||
| ? getCtx()?.getImageData(0, 0, el.width, el.height) | ||
| : null | ||
| el.width = canvasWidth.value | ||
| el.height = canvasHeight.value | ||
| if (prevData) { | ||
| getCtx()?.putImageData(prevData, 0, 0) | ||
| } | ||
|
|
||
| strokeCanvas = null | ||
| strokeCtx = null | ||
| baseCanvas = null | ||
| baseCtx = null | ||
| hasBaseSnapshot = false | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check canvas-related patterns in the codebase
rg -n "putImageData|drawImage" src/composables/ -C 3Repository: Comfy-Org/ComfyUI_frontend
Length of output: 49980
🏁 Script executed:
#!/bin/bash
# Check the overall usePainter.ts file structure and resizeCanvas usage
wc -l src/composables/painter/usePainter.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 111
🏁 Script executed:
#!/bin/bash
# Look for resizeCanvas calls and surrounding context
rg -n "resizeCanvas" src/ -C 5Repository: Comfy-Org/ComfyUI_frontend
Length of output: 3540
🏁 Script executed:
#!/bin/bash
# Check if there's any canvas size validation or warning logic elsewhere
rg -n "getImageData|canvasWidth|canvasHeight" src/composables/painter/usePainter.ts -C 2 | head -80Repository: Comfy-Org/ComfyUI_frontend
Length of output: 2492
Add a clarifying comment on the silent content clipping behavior during canvas resize.
The resizeCanvas function preserves image data when dimensions change, but putImageData silently clips content that falls outside the new bounds. Since canvasWidth and canvasHeight are reactive refs that can shrink (from widget changes or image reloads), users could lose painted content unexpectedly. Either add a comment explaining why this clipping is acceptable, or implement scaling logic to preserve the painted area proportionally within the new dimensions.
🤖 Prompt for AI Agents
In `@src/composables/painter/usePainter.ts` around lines 403 - 421, The
resizeCanvas function currently captures prevData and restores it with
getCtx()?.putImageData, which silently clips content when canvasWidth or
canvasHeight shrink; update resizeCanvas to either (A) add a clear inline
comment above the prevData/putImageData block explaining that silent clipping on
shrink is intentional and acceptable for this app (mention canvasWidth,
canvasHeight, prevData, putImageData, getCtx, hasBaseSnapshot), or (B) implement
proportional scaling: create an offscreen/temp canvas, draw the prevData into
it, then use drawImage to scale the temp canvas into the new el.width/el.height
before replacing the main canvas (use getCtx/getCtx of temp, drawImage instead
of putImageData, and preserve strokeCanvas/baseCanvas state), ensuring
hasBaseSnapshot, strokeCtx and baseCtx handling remains consistent; choose A for
minimal change or B if you need to preserve painted content on resize.
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
4 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.
Summary
Add PainterNode widget for freehand mask drawing directly on the canvas, with brush/eraser tools, opacity, hardness, and background color controls.
need BE changes Comfy-Org/ComfyUI#12294
Screenshots (if applicable)
2026-01-31.22-17-31.mp4
┆Issue is synchronized with this Notion page by Unito