Fully Rename MaskEditor to Image Canvas with Settings Migration #7958
Conversation
brucew4yn3rp
requested review from
a team,
KarryCharon,
Myestery,
Yorha4D,
shinshin86 and
trsommer
as code owners
dosubot
bot
added
the
size:XXL
📝 WalkthroughWalkthroughThis PR renames and migrates the Mask Editor feature to Image Canvas across code, assets, CSS, stores, commands, localization, and documentation; it also adds migrations to preserve user settings and keybindings during the rename. Changes
Possibly related PRs
✨ Finishing touches
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 |
🎭 Playwright Tests: ❌ FailedResults: 504 passed, 1 failed, 0 flaky, 8 skipped (Total: 513) ❌ Failed Tests📊 Browser Reports
|
🎨 Storybook Build Status
⏰ Started at: 01/11/2026, 05:17:25 PM UTC 🚀 Building Storybook
⏱️ Please wait while the Storybook build is in progress... |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 3.32 MB (baseline 3.32 MB) • 🔴 +3.72 kBMain entry bundles and manifests
Status: 3 added / 3 removed Graph Workspace — 1.03 MB (baseline 1.03 MB) • 🔴 +32 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 6.63 kB (baseline 6.63 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 1 added / 1 removed Panels & Settings — 337 kB (baseline 337 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 6 added / 6 removed UI Components — 199 kB (baseline 199 kB) • ⚪ 0 BReusable component library chunks
Status: 8 added / 8 removed Data & Services — 12.5 kB (baseline 12.5 kB) • ⚪ 0 BStores, services, APIs, and repositories
Status: 2 added / 2 removed Utilities & Hooks — 1.41 kB (baseline 1.41 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 1 added / 1 removed Vendor & Third-Party — 9.19 MB (baseline 9.19 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 4.74 MB (baseline 4.74 MB) • ⚪ 0 BBundles that do not match a named category
Status: 16 added / 16 removed |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/composables/imagecanvas/splineUtils.ts (1)
3-126: Consider relocating utility functions to a dedicated utils directory.This file exports pure mathematical utility functions rather than Vue composables. Since it doesn't use composition API features (
ref,computed,watch, etc.), consider moving it to autils/directory for better code organization and alignment with the naming convention that composables follow theuseXyz.tspattern.Note: This is a pre-existing organizational consideration, not introduced by this PR.
src/components/imagecanvas/controls/ToggleControl.vue (1)
19-33: Consider usingdefineModelfor v-model binding.The current manual v-model implementation works correctly, but Vue 3.5's
defineModelprovides a simpler, more idiomatic approach for two-way binding.♻️ Refactor to use defineModel
<script setup lang="ts"> interface Props { label: string - modelValue: boolean } defineProps<Props>() -const emit = defineEmits<{ - 'update:modelValue': [value: boolean] -}>() - -const onChange = (event: Event) => { - const checked = (event.target as HTMLInputElement).checked - emit('update:modelValue', checked) -} +const modelValue = defineModel<boolean>({ required: true }) + +const onChange = (event: Event) => { + modelValue.value = (event.target as HTMLInputElement).checked +} </script>As per coding guidelines: "Prefer
defineModelover separately defining a prop and emit for v-model bindings."src/components/imagecanvas/controls/SliderControl.vue (1)
27-29: Refactor to Vue 3.5 reactive props destructuring.The component uses
withDefaults, but the coding guidelines require Vue 3.5 style default prop declaration with reactive props destructuring.♻️ Proposed refactor
-withDefaults(defineProps<Props>(), { - step: 1 -}) +const { label, min, max, step = 1, modelValue } = defineProps<Props>()Based on coding guidelines: "Use TypeScript Vue 3.5 style default prop declaration with reactive props destructuring - avoid withDefaults or runtime props."
packages/design-system/src/css/style.css (1)
489-494: Undefined CSS variables referenced in theme inline block.These lines reference CSS variables that are not defined:
--interface-panel-hover-surface(line 490)--interface-panel-selected-surface(lines 491-493)--interface-button-hover-surface(line 494)Only their
--palette-*prefixed counterparts are defined (lines 295-309). Add the missing variable definitions in:root:Suggested fix: Add missing variable mappings in :root
--palette-interface-button-hover-surface: color-mix( in srgb, var(--interface-panel-surface) 82%, var(--contrast-mix-color) ); + + --interface-panel-hover-surface: var(--palette-interface-panel-hover-surface); + --interface-panel-selected-surface: var(--palette-interface-panel-selected-surface); + --interface-button-hover-surface: var(--palette-interface-button-hover-surface);
🤖 Fix all issues with AI agents
In @packages/design-system/src/css/style.css:
- Around line 285-309: The palette CSS uses var(--contrast-mix-color) in
color-mix() calls but only --palette-contrast-mix-color is defined; add a
mapping so --contrast-mix-color points to --palette-contrast-mix-color (or
replace the usages to use --palette-contrast-mix-color) so color-mix() receives
a defined variable; update the token definitions around
--palette-contrast-mix-color and the palette tokens (--palette-interface-stroke,
--palette-interface-panel-hover-surface,
--palette-interface-panel-selected-surface,
--palette-interface-button-hover-surface) accordingly.
In @src/composables/graph/useImageMenuOptions.ts:
- Around line 14-17: The click handler openImageCanvas should guard against a
synchronous throw from commandStore.execute: wrap the call to
useCommandStore().execute('Comfy.ImageCanvas.OpenImageCanvas') inside a
try/catch in the openImageCanvas function (or check for a registration method if
available) and swallow or log the error so a missing command id doesn’t bubble
out and break the context menu; reference the openImageCanvas function and
commandStore.execute call when making the change.
In @src/composables/imagecanvas/useBrushDrawing.ts:
- Line 203: The code now calls loadBrushFromCache('imagecanvas_brush_settings')
which will lose users' previous settings stored under
'maskeditor_brush_settings'; update the loading logic in useBrushDrawing (and
the other occurrence around line 1192) to first attempt to read the old key
'maskeditor_brush_settings' and if present migrate/overwrite the new key
'imagecanvas_brush_settings' (or merge and then persist via the existing
save/persist function), otherwise fall back to reading
'imagecanvas_brush_settings', ensuring the migration runs once so users keep
their prior brush settings.
In @src/extensions/core/imagecanvas.ts:
- Around line 151-159: The deprecation warning is logged during init() at
startup even if ComfyApp.open_imagecanvas is never used; change it so init()
only assigns ComfyApp.open_imagecanvas = openImageCanvasFromClipspace and move
the console.warn into a wrapper function that replaces open_imagecanvas with a
function which logs the deprecation when invoked (or logs once on first
invocation) before delegating to openImageCanvasFromClipspace; update references
to ComfyApp.open_imagecanvas and openImageCanvasFromClipspace accordingly so the
warning fires on actual use, not on registration.
In @src/platform/settings/settingStore.ts:
- Around line 262-263: The migration uses api.storeSetting with newSpeedKey and
oldSpeedKey and bypasses type safety with "as any" because the new image canvas
keys (e.g., Comfy.ImageCanvas.BrushAdjustmentSpeed and related keys) are missing
from the Settings type; add those new keys to the Settings interface/type
(matching their expected value types and defaults), update any Settings-related
enums or union types that list keys, then remove the "as any" casts in the
migration so api.storeSetting(newSpeedKey, ...) and
api.storeSetting(oldSpeedKey, ...) are properly typed.
- Around line 290-302: Fix the typo in the commandMappings object: update the
value for 'Comfy.MaskEditor.OpenMaskEditor' from
'Comfy.ImageCanvas.OpenimageCanvas' to the correct registered command ID
'Comfy.ImageCanvas.OpenImageCanvas' so the keybinding migration in
settingStore.ts matches the command registered in imagecanvas.ts (look for the
commandMappings constant and the registered command
'Comfy.ImageCanvas.OpenImageCanvas').
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @src/platform/settings/settingStore.ts:
- Line 325: The call to api.storeSetting uses an `as any` assertion for
`newBindingsKey` (and the similar occurrence at the other call), remove the `as
any` by making the key and value types explicit: update the Settings schema/type
so `newBindingsKey` is typed as the appropriate setting key (e.g., keyof
Settings or the specific SettingKey union) and ensure `migratedBindings` matches
the corresponding Settings value type, then call api.storeSetting with the
concrete types (or a typed key variable) instead of using `as any`; adjust the
Settings type definitions and the variable declarations for
`newBindingsKey`/`migratedBindings` so the compiler can infer correct types and
the `as any` can be eliminated.
- Around line 306-352: Extract the duplicated migration logic into a single
async helper function (e.g., migrateKeybindingSetting) that accepts the setting
key string (like newBindingsKey / unsetBindingsKey), reads
settingValues.value[key], verifies Array.isArray, maps bindings replacing
binding.commandId using commandMappings, and if any modified updates
settingValues.value[key], calls await api.storeSetting(key as any,
migratedBindings) and returns a boolean indicating whether it migrated; then
call this helper for both 'Comfy.Keybinding.NewBindings' and
'Comfy.Keybinding.UnsetBindings' and set anyMigrated = anyMigrated || await
migrateKeybindingSetting(key). Ensure you reuse commandMappings, settingValues,
and api.storeSetting and preserve the original shape of binding objects.
- Around line 251-279: The two migration blocks for
'Comfy.MaskEditor.BrushAdjustmentSpeed' ->
'Comfy.ImageCanvas.BrushAdjustmentSpeed' and 'Comfy.MaskEditor.UseDominantAxis'
-> 'Comfy.ImageCanvas.UseDominantAxis' are duplicated; extract the logic into a
small async helper like migrateSetting(oldKey: string, newKey: string) that
checks settingValues.value[oldKey] !== undefined && settingValues.value[newKey]
=== undefined, copies the value into settingValues.value[newKey], deletes
settingValues.value[oldKey], then awaits api.storeSetting(newKey as any,
oldValue) and await api.storeSetting(oldKey as any, undefined); replace both
blocks with calls to migrateSetting(oldSpeedKey, newSpeedKey) and
migrateSetting(oldAxisKey, newAxisKey).
- Around line 246-283: The migrateMaskEditorToImageCanvas function currently
calls api.storeSetting without error handling which can leave settings
inconsistent; wrap each migration block (the BrushAdjustmentSpeed and
UseDominantAxis branches) in try-catch, persist the new key with
api.storeSetting(newKey, oldValue) before deleting the old in
settingValues.value, and only delete/clear the old key after the new store
succeeds; on any catch, log the error (or rethrow) and avoid deleting the
original setting so state remains consistent; apply the same guarded pattern
around migrateMaskEditorKeybindings or call it after both migrations succeed.
- Around line 289-355: The function migrateMaskEditorKeybindings currently
computes and returns anyMigrated but the caller doesn't use it; change the
function to return Promise<void> by removing the anyMigrated variable and final
return, delete assignments to anyMigrated inside both NewBindings and
UnsetBindings migration blocks, and leave the await api.storeSetting calls
intact; also update the caller that invoked migrateMaskEditorKeybindings to
ignore the result (no changes to behavior) or optionally add a log inside
migrateMaskEditorKeybindings if you want to record migrations instead of
returning a value.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/platform/settings/settingStore.ts
🧰 Additional context used
📓 Path-based instructions (6)
src/**/*.{vue,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{vue,ts}: Leverage VueUse functions for performance-enhancing styles
Implement proper error handling
Use vue-i18n in composition API for any string literals. Place new translation entries in src/locales/en/main.json
Files:
src/platform/settings/settingStore.ts
src/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.ts: Use es-toolkit for utility functions
Use TypeScript for type safety
src/**/*.ts: Derive component types usingvue-component-type-helpers(ComponentProps,ComponentSlots) instead of separate type files
Use es-toolkit for utility functions
Minimize the surface area (exported values) of each module and composable
Favor pure functions, especially testable ones
Files:
src/platform/settings/settingStore.ts
src/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.{ts,tsx,vue}: Sanitize HTML with DOMPurify to prevent XSS attacks
Avoid using @ts-expect-error; use proper TypeScript types instead
Use es-toolkit for utility functions instead of other utility libraries
Implement proper TypeScript types throughout the codebase
src/**/*.{ts,tsx,vue}: Use separateimport typestatements instead of inlinetypein mixed imports
Apply Prettier formatting with 2-space indentation, single quotes, no trailing semicolons, 80-character width
Sort and group imports by plugin, runpnpm formatbefore committing
Never useanytype - use proper TypeScript types
Never useas anytype assertions - fix the underlying type issue
Write code that is expressive and self-documenting - avoid unnecessary comments
Do not add or retain redundant comments - clean as you go
Avoid mutable state - prefer immutability and assignment at point of declaration
Watch out for Code Smells and refactor to avoid them
Files:
src/platform/settings/settingStore.ts
src/**/*.{vue,ts,tsx}
📄 CodeRabbit inference engine (src/CLAUDE.md)
Follow Vue 3 composition API style guide
Files:
src/platform/settings/settingStore.ts
src/**/*.{ts,vue}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,vue}: Usereffor reactive state,computed()for derived values, andwatch/watchEffectfor side effects in Composition API
Avoid usingrefwithwatchif acomputedwould suffice - minimize refs and derived state
Useprovide/injectfor dependency injection only when simpler alternatives (Store or shared composable) won't work
Leverage VueUse functions for performance-enhancing composables
Use VueUse function for useI18n in composition API for string literals
Files:
src/platform/settings/settingStore.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,tsx}: Keep functions short and functional
Minimize nesting (if statements, for loops, etc.)
Use function declarations instead of function expressions when possible
Files:
src/platform/settings/settingStore.ts
🧠 Learnings (5)
📚 Learning: 2025-12-09T03:39:54.501Z
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7169
File: src/platform/remote/comfyui/jobs/jobTypes.ts:1-107
Timestamp: 2025-12-09T03:39:54.501Z
Learning: In the ComfyUI_frontend project, Zod is on v3.x. Do not suggest Zod v4 standalone validators (z.uuid, z.ulid, z.cuid2, z.nanoid) until an upgrade to Zod 4 is performed. When reviewing TypeScript files (e.g., src/platform/remote/comfyui/jobs/jobTypes.ts) validate against Zod 3 capabilities and avoid introducing v4-specific features; flag any proposal to upgrade or incorporate v4-only validators and propose staying with compatible 3.x patterns.
Applied to files:
src/platform/settings/settingStore.ts
📚 Learning: 2025-12-13T11:03:11.264Z
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 7416
File: src/stores/imagePreviewStore.ts:5-7
Timestamp: 2025-12-13T11:03:11.264Z
Learning: In the ComfyUI_frontend repository, lint rules require keeping 'import type' statements separate from non-type imports, even if importing from the same module. Do not suggest consolidating them into a single import statement. Ensure type imports remain on their own line (import type { ... } from 'module') and regular imports stay on separate lines.
Applied to files:
src/platform/settings/settingStore.ts
📚 Learning: 2025-12-17T00:40:09.635Z
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7537
File: src/components/ui/button/Button.stories.ts:45-55
Timestamp: 2025-12-17T00:40:09.635Z
Learning: Prefer pure function declarations over function expressions (e.g., use function foo() { ... } instead of const foo = () => { ... }) for pure functions in the repository. Function declarations are more functional-leaning, offer better hoisting clarity, and can improve readability and tooling consistency. Apply this guideline across TypeScript files in Comfy-Org/ComfyUI_frontend, including story and UI component code, except where a function expression is semantically required (e.g., callbacks, higher-order functions with closures).
Applied to files:
src/platform/settings/settingStore.ts
📚 Learning: 2025-12-30T22:22:33.836Z
Learnt from: kaili-yang
Repo: Comfy-Org/ComfyUI_frontend PR: 7805
File: src/composables/useCoreCommands.ts:439-439
Timestamp: 2025-12-30T22:22:33.836Z
Learning: When accessing reactive properties from Pinia stores in TypeScript files, avoid using .value on direct property access (e.g., useStore().isOverlayExpanded). Pinia auto-wraps refs when accessed directly, returning the primitive value. The .value accessor is only needed when destructuring store properties or when using storeToRefs().
Applied to files:
src/platform/settings/settingStore.ts
📚 Learning: 2025-12-11T12:25:15.470Z
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 7358
File: src/components/dialog/content/signin/SignUpForm.vue:45-54
Timestamp: 2025-12-11T12:25:15.470Z
Learning: This repository uses CI automation to format code (pnpm format). Do not include manual formatting suggestions in code reviews for Comfy-Org/ComfyUI_frontend. If formatting issues are detected, rely on the CI formatter or re-run pnpm format. Focus reviews on correctness, readability, performance, accessibility, and maintainability rather than style formatting.
Applied to files:
src/platform/settings/settingStore.ts
🧬 Code graph analysis (1)
src/platform/settings/settingStore.ts (1)
src/scripts/api.ts (1)
api(1336-1336)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: setup
- GitHub Check: collect
- GitHub Check: lint-and-format
🔇 Additional comments (1)
src/platform/settings/settingStore.ts (1)
202-204: LGTM! Migration invocation is correctly placed.The migration is properly positioned in the initialization flow after settings are loaded from the server but before any settings are registered, which ensures user settings are preserved during the rename.
|
I might be wrong, but I feel this could confuse users. “Image Canvas” doesn’t suggest masking or editing features to me at all. I agree the current name is terrible, but I don’t think Image Canvas is the right choice. How about Image Editing & Masking (or something similar)? |
|
Thanks @trsommer. I get the concern, but I’d push back a bit on the idea that Image Canvas doesn’t imply editing. In most graphics tools, a “canvas” is explicitly the surface where editing happens. Examples of this naming include Photoshop’s canvas, Procreate’s canvas, even HTML5. 
Also, the transitional label was “Mask Editor | Image Canvas”, and the natural evolution is the drop-off of the Mask Editor legacy name as to not anchor the naming to one or two features. With that said, open to feedback if others feel strongly. |
|
Hi @christian-byrne would you / another team member mind reviewing when you have a chance? I think the other requested reviews are just auto-triggered because of codeowner statuses but not fully relevant since the changes are just naming-related. |
|
Hi, firstly, thanks for contributing this. Convert it as draft for now |
4 participants
Summary
What was previously a simple Mask Editor has been refactored and now functions as a more general Image Canvas, with masking being only one of its features. To avoid confusing users, the context menu option was renamed to
Mask Editor | Image Canvas. As a longer-term solution, we should fully adopt the Image Canvas name.Re-work of #7950
Changes
Renaming & Refactoring
maskEditortoimageCanvasMaskEditor→ImageCanvasmaskEditor→imageCanvasmask-editor→image-canvasBug Fixes
maskEditor_is_opended()→imageCanvas_is_opened()Migration
settingStore.tsto preserve user settings and keybindings when upgrading:Comfy.MaskEditor.BrushAdjustmentSpeed→Comfy.imageCanvas.BrushAdjustmentSpeedComfy.MaskEditor.UseDominantAxis→Comfy.imageCanvas.UseDominantAxisComfy.MaskEditor.*toComfy.imageCanvas.*Breaking Changes
While this is technically a breaking change for extensions that reference the old
maskEditornaming, the migration ensures that end users will not lose any settings or keybindings from the update.┆Issue is synchronized with this Notion page by Unito