fix: migrate DOM widgets to ComponentWidgetImpl and add element value bridge#9230
Open
christian-byrne wants to merge 4 commits intomainfrom
Open
fix: migrate DOM widgets to ComponentWidgetImpl and add element value bridge#9230christian-byrne wants to merge 4 commits intomainfrom
christian-byrne wants to merge 4 commits intomainfrom
Conversation
dosubot
bot
added
the
size:XL
🎨 Storybook: ✅ Built — View Storybook |
🎭 Playwright: ❌ 335 passed, 108 failed · 4 flaky❌ Failed Tests📊 Browser Reports
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoved TipTap packages; replaced TipTap-based markdown and raw DOM textarea flows with component-backed widgets; added useDomValueBridge to synchronize input/textarea element values with Vue reactivity and integrated its lifecycle into domWidget to start/stop bridges and propagate updates to widget.store. Changes
Sequence Diagram(s)sequenceDiagram
participant DOM as DOM Element
participant Bridge as useDomValueBridge
participant Store as Widget Store / domWidget
participant Vue as Widget Vue Component
rect rgba(200,220,255,0.5)
DOM ->> Bridge: read initial value / inspect descriptor
Bridge ->> Store: initialize widget.value (delegate or snapshot)
end
rect rgba(200,255,200,0.5)
DOM -->> Bridge: user input event
Bridge ->> Store: update widget.value (watch)
Store ->> Vue: reactive update (store -> v-model)
Vue ->> Store: user edits via update:modelValue
end
rect rgba(255,220,200,0.5)
Store ->> Bridge: widget removal -> stop effectScope
Bridge -->> DOM: restore original descriptor / remove listener
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
📦 Bundle: 4.28 MB gzip 🟢 -212 kBDetailsSummary
Category Glance App Entry Points — 17.7 kB (baseline 17.7 kB) • 🟢 -12 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 912 kB (baseline 912 kB) • 🟢 -55 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 72.6 kB (baseline 72.1 kB) • 🔴 +501 BTop-level views, pages, and routed surfaces
Status: 11 added / 11 removed Panels & Settings — 436 kB (baseline 436 kB) • 🔴 +188 BConfiguration panels, inspectors, and settings screens
Status: 22 added / 22 removed User & Accounts — 16.1 kB (baseline 16 kB) • 🔴 +56 BAuthentication, profile, and account management bundles
Status: 7 added / 7 removed Editors & Dialogs — 764 B (baseline 736 B) • 🔴 +28 BModals, dialogs, drawers, and in-app editors
Status: 1 added / 1 removed UI Components — 47.1 kB (baseline 47 kB) • 🔴 +84 BReusable component library chunks
Status: 12 added / 12 removed Data & Services — 2.73 MB (baseline 2.73 MB) • 🔴 +1.64 kBStores, services, APIs, and repositories
Status: 14 added / 14 removed Utilities & Hooks — 54.1 kB (baseline 55.5 kB) • 🟢 -1.47 kBHelpers, composables, and utility bundles
Status: 14 added / 15 removed Vendor & Third-Party — 8.01 MB (baseline 8.86 MB) • 🟢 -845 kBExternal libraries and shared vendor chunks
Status: 5 added / 6 removed Other — 7.91 MB (baseline 7.91 MB) • 🟢 -1.21 kBBundles that do not match a named category
Status: 120 added / 120 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: 3
🧹 Nitpick comments (1)
src/composables/element/useDomValueBridge.test.ts (1)
72-139: Add a non-configurable descriptor regression test.Current descriptor-path tests cover chaining/restoration, but not
configurable: false. Adding that case would protect against runtime throw regressions in bridge setup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/composables/element/useDomValueBridge.test.ts` around lines 72 - 139, Add a new test that ensures useDomValueBridge handles a non-configurable descriptor without throwing and restores behavior: create a non-configurable property descriptor on element for 'value' via Object.defineProperty (configurable: false) with appropriate get/set delegating to HTMLTextAreaElement.prototype's native descriptor, wrap useDomValueBridge call inside an effectScope().run and call scope.stop(), then assert no exception occurred, the descriptor remained non-configurable during the scope (Object.getOwnPropertyDescriptor(element, 'value') is defined), and after scope.stop() the descriptor is still present and unchanged (verify configurable is false and the custom getter/setter were preserved or called as expected). Reference useDomValueBridge, effectScope, Object.defineProperty, and the 'value' property on element in the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/composables/element/useDomValueBridge.ts`:
- Around line 43-53: Before calling Object.defineProperty on element.value, read
the current descriptor with Object.getOwnPropertyDescriptor(element, 'value')
and only perform the override if the descriptor is missing or
descriptor.configurable === true; if it's non-configurable, skip the redefine
and set a flag (e.g., skippedOverride) so callers know no override was
installed. Do the same check in the cleanup/restore logic (where you currently
attempt to restore prevGet/prevSet) and only restore or delete the property if
the original descriptor was configurable (otherwise leave it untouched);
reference the symbols element, prevGet, prevSet, notifyChange and the
cleanup/restore block to locate both places to add the conditional guard.
In `@src/renderer/extensions/vueNodes/widgets/composables/useStringWidget.ts`:
- Line 22: Change the placeholder fallback in useStringWidget.ts to use nullish
coalescing so explicit empty placeholders are preserved: replace the use of
opts.placeholder || name with a nullish check (opts.placeholder ?? name) in the
props object (see the props: { placeholder: ... } expression) so only
null/undefined fall back to name.
In `@src/scripts/domWidget.ts`:
- Around line 430-432: The current useChainCallback call unconditionally calls
bridgeScope.stop() in widget.onRemove which wrongly tears down the bridge during
subgraph moves; change the teardown to only run for genuine deletions (e.g.
check the node/widget destruction/attachment state before calling
bridgeScope.stop) and/or move bridgeScope.stop into a true destroy callback,
while adding a complementary setup re-run when the widget is (re)attached (e.g.,
run the bridge setup from the widget attach/onAdd handler); update the
references around widget.onRemove, useChainCallback, and bridgeScope.stop and
ensure the bridge setup is invoked in the widget attach/onAdd path so
restructured widgets regain synchronization.
---
Nitpick comments:
In `@src/composables/element/useDomValueBridge.test.ts`:
- Around line 72-139: Add a new test that ensures useDomValueBridge handles a
non-configurable descriptor without throwing and restores behavior: create a
non-configurable property descriptor on element for 'value' via
Object.defineProperty (configurable: false) with appropriate get/set delegating
to HTMLTextAreaElement.prototype's native descriptor, wrap useDomValueBridge
call inside an effectScope().run and call scope.stop(), then assert no exception
occurred, the descriptor remained non-configurable during the scope
(Object.getOwnPropertyDescriptor(element, 'value') is defined), and after
scope.stop() the descriptor is still present and unchanged (verify configurable
is false and the custom getter/setter were preserved or called as expected).
Reference useDomValueBridge, effectScope, Object.defineProperty, and the 'value'
property on element in the test.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
package.jsonpnpm-workspace.yamlsrc/composables/element/useDomValueBridge.test.tssrc/composables/element/useDomValueBridge.tssrc/renderer/extensions/vueNodes/widgets/components/WidgetTextarea.test.tssrc/renderer/extensions/vueNodes/widgets/components/WidgetTextarea.vuesrc/renderer/extensions/vueNodes/widgets/composables/useMarkdownWidget.tssrc/renderer/extensions/vueNodes/widgets/composables/useStringWidget.tssrc/scripts/domWidget.ts
💤 Files with no reviewable changes (2)
- pnpm-workspace.yaml
- package.json
src/renderer/extensions/vueNodes/widgets/composables/useStringWidget.ts
Outdated
Show resolved
Hide resolved
DrJKL
reviewed
Feb 26, 2026
| event.preventDefault() | ||
| app.canvas.processMouseWheel(event) | ||
| }) | ||
| addWidget(node, widget as BaseDOMWidget<object | string>) |
Contributor
There was a problem hiding this comment.
Is it not really a BaseDOMWidget<object | string>?
Contributor
Author
There was a problem hiding this comment.
Yep, that is what it is — line 28 already has widget as BaseDOMWidget<object | string>. This was pre-existing from the original code.
DrJKL
reviewed
Feb 26, 2026
| import { useWidgetValueStore } from '@/stores/widgetValueStore' | ||
|
|
||
| const TRACKPAD_DETECTION_THRESHOLD = 50 | ||
| import WidgetTextarea from '../components/WidgetTextarea.vue' |
Contributor
There was a problem hiding this comment.
I never finished prohibiting ancestor segments in imports. 🪦
Contributor
Author
There was a problem hiding this comment.
Acknowledged — not something to address in this PR but would be a nice lint rule to have.
⚡ Performance Report
Raw data{
"timestamp": "2026-03-04T22:45:19.771Z",
"gitSha": "0f3d653d594ddbaa9be8ee4284fa63e2a7e68133",
"branch": "fix/dom-widget-value-ownership",
"measurements": [
{
"name": "canvas-idle",
"durationMs": 2026.4040000000136,
"styleRecalcs": 122,
"styleRecalcDurationMs": 19.678,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 388.601,
"heapDeltaBytes": -2952832
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1829.7849999999585,
"styleRecalcs": 167,
"styleRecalcDurationMs": 48.81,
"layouts": 12,
"layoutDurationMs": 3.052,
"taskDurationMs": 835.831,
"heapDeltaBytes": -3083808
},
{
"name": "dom-widget-clipping",
"durationMs": 443.1640000000243,
"styleRecalcs": 31,
"styleRecalcDurationMs": 14.498000000000005,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 217.645,
"heapDeltaBytes": 2597016
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 620.5949999999802,
"styleRecalcs": 75,
"styleRecalcDurationMs": 18.623,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 435.21999999999997,
"heapDeltaBytes": 15741312
},
{
"name": "subgraph-idle",
"durationMs": 1999.9239999999645,
"styleRecalcs": 121,
"styleRecalcDurationMs": 25.902,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 417.096,
"heapDeltaBytes": -3141808
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1975.665000000049,
"styleRecalcs": 171,
"styleRecalcDurationMs": 54.50400000000001,
"layouts": 16,
"layoutDurationMs": 3.7909999999999995,
"taskDurationMs": 971.9570000000001,
"heapDeltaBytes": -5032700
}
]
} |
DrJKL
reviewed
Feb 26, 2026
DrJKL
reviewed
Feb 26, 2026
| function onInput() { | ||
| notifyChange?.() | ||
| } | ||
| element.addEventListener('input', onInput) |
Contributor
Author
There was a problem hiding this comment.
Switched to useEventListener from @vueuse/core in c060787 — handles cleanup automatically via scope disposal.
DrJKL
reviewed
Feb 26, 2026
| if (existingDescriptor) { | ||
| Object.defineProperty(element, 'value', existingDescriptor) | ||
| } else { | ||
| delete (element as unknown as Record<string, unknown>).value |
Contributor
Author
There was a problem hiding this comment.
Yeah the cast is unfortunate but necessary — delete only works on index-signature types, and HTMLTextAreaElement does not have one. This restores the element to its prototype descriptor.
DrJKL
reviewed
Feb 26, 2026
Contributor
There was a problem hiding this comment.
We should come up with a clean API for extension authors so we can add a proper deprecation warning here. How to wire up your custom widget to the widgetValueStore properly.
Contributor
Author
There was a problem hiding this comment.
Agreed, that deserves its own effort. This PR keeps the existing escape hatch working but a proper public API for extension widget authors would be the right long-term fix.
@coderabbitai Make an Issue to design a clean public API for extension authors to wire custom widgets to the widgetValueStore, replacing the current value-descriptor pattern with a supported integration point.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
@christian-byrne I'll create that issue for you.
🏁 Script executed:
#!/bin/bash
# Create GitHub issue for designing a clean public API for extension widget integration
gh issue create \
--title "Design clean public API for extension authors to integrate custom widgets with widgetValueStore" \
--body "## Context
PR `#9230` introduces \`useDomValueBridge\` to synchronize DOM widget values with the \`widgetValueStore\` by intercepting \`Object.defineProperty\` on element.value and listening for input events. While this approach works as a bridge for extension-created DOM widgets, it relies on internal value-descriptor patterns that are not part of a supported public API.
## Problem
Extension authors currently lack a clean, documented integration point to wire custom widgets to the \`widgetValueStore\`. The current value-descriptor pattern is an implementation detail rather than a stable extension API.
## Proposed Solution
Design and implement a proper public API that:
- Provides a clear, documented way for extension authors to integrate custom widgets with the store
- Replaces the current value-descriptor pattern with a supported integration mechanism
- Maintains backward compatibility where feasible
- Includes documentation and migration guidance
## References
- PR: https://github.com/Comfy-Org/ComfyUI_frontend/pull/9230
- Discussion: https://github.com/Comfy-Org/ComfyUI_frontend/pull/9230#discussion_r2857107802
- Requested by: `@christian-byrne`
## Acceptance Criteria
- [ ] Public API design documented
- [ ] Extension author documentation created
- [ ] Deprecation path defined for current pattern (if applicable)
- [ ] Implementation plan outlined" \
--assignee christian-byrne
Length of output: 57
@christian-byrne Issue created!
I've opened #9296 to track the design of a clean public API for extension authors to integrate custom widgets with the widgetValueStore. The issue captures the current value-descriptor pattern as a temporary workaround and outlines the need for a proper supported integration point with documentation and migration guidance.
🧠 Learnings used
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T19:47:02.860Z
Learning: Applies to src/**/*.vue : Utilize ref and reactive for reactive state
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8753
File: src/renderer/extensions/vueNodes/widgets/components/WidgetDOM.vue:17-28
Timestamp: 2026-02-09T03:24:50.064Z
Learning: In this codebase: When destructuring reactive properties from Pinia stores, use `storeToRefs()` to maintain reactivity. Example: `const canvasStore = useCanvasStore(); const { canvas } = storeToRefs(canvasStore)`. Then access with `.value` (e.g., `canvas.value`).
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-23T21:34:45.361Z
Learning: Applies to src/**/*.ts : Use `ref` for reactive state, `computed()` for derived values, and `watch`/`watchEffect` for side effects
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8594
File: src/lib/litegraph/src/widgets/BaseWidget.ts:146-155
Timestamp: 2026-02-06T09:27:31.576Z
Learning: In the ComfyUI frontend codebase: `LGraphNode.removeWidget()` (and the `widget.onRemove?.()` hook) is called during subgraph conversion operations where widgets are being moved or restructured, not just when they're being permanently deleted. Therefore, widget unregistration from `widgetValueStore` should not be tied to `onRemove`, as this would cause premature state loss during legitimate graph restructuring operations.
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-23T21:34:45.361Z
Learning: Applies to src/**/*.ts : Minimize addition of new refs or other state; if possible to accomplish with just a prop, don't add a `ref`; if possible to use ref/prop directly, don't add a `computed`; if possible to use `computed` for derived values, don't use a `watch`
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T19:47:02.860Z
Learning: Applies to src/**/*.{vue,ts} : Leverage VueUse functions for performance-enhancing styles
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.
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.
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.
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).
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().
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7906
File: src/components/sidebar/tabs/AssetsSidebarTab.vue:545-552
Timestamp: 2026-01-12T17:39:27.738Z
Learning: In Vue/TypeScript files (src/**/*.{ts,tsx,vue}), prefer if/else statements over ternary operators when performing side effects or actions (e.g., mutating state, calling methods with side effects). Ternaries should be reserved for computing and returning values.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8248
File: src/platform/assets/schemas/assetSchema.ts:109-118
Timestamp: 2026-01-22T23:44:59.888Z
Learning: Do not export declarations (e.g., schemas, constants, types) unless they are actually used elsewhere in the codebase. Keep the public API surface minimal by exporting only items with concrete usage. This reduces coupling and keeps imports explicit. In src/platform/assets/schemas/assetSchema.ts, ensure only necessary exports are exposed; remove unused exports or switch to non-exported/internal where appropriate.
Learnt from: JemiloII
Repo: Comfy-Org/ComfyUI_frontend PR: 8282
File: src/composables/usePaste.test.ts:372-383
Timestamp: 2026-01-23T21:06:15.240Z
Learning: In paste/clipboard handling code, do not rely on ephemeral DataTransfer data across async boundaries. If an operation awaits async tasks (such as node creation) during a paste, clone the DataTransfer (e.g., using cloneDataTransfer) to persist files and string data before the paste event completes. This ensures clipboard contents remain available for subsequent processing.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8722
File: src/composables/maskeditor/useBrushDrawing.ts:5-5
Timestamp: 2026-02-08T03:19:12.009Z
Learning: Treat unresolved import warnings caused by pnpm workspace catalog mappings (pnpm-workspace.yaml) and unplugin-typegpu in vite.config.mts as false positives. Do not flag these in PR reviews; they are expected in sandboxes without node_modules and are not indicative of actual missing dependencies in the TS codebase.
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 8966
File: src/extensions/core/load3d.ts:427-430
Timestamp: 2026-02-19T02:06:23.468Z
Learning: In TypeScript, you can use typeof SomeClass in type annotation positions (e.g., param: typeof LGraphNode) even when SomeClass is imported with import type. This is a type query that only exists at compile time and is erased in runtime, so it is safe to combine with type-only imports. Apply this pattern to TS files broadly when you need a type that references the shape of a class or constructor function without importing the value at runtime.
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 8966
File: src/extensions/core/uploadAudio.ts:91-94
Timestamp: 2026-02-19T02:06:38.395Z
Learning: In TypeScript files, you can use a type annotation like 'nodeType: typeof MyClass' even if MyClass is imported via 'import type'. Both the type-only import and 'typeof' operate at the type level and are erased at compile time. This pattern is commonly used for constructor types (e.g., 'nodeType: typeof LGraphNode'). Apply this pattern across TypeScript files in the repository (src/**/*.ts) as appropriate, ensuring the imported symbol is a type-only import when possible for clarity and to avoid runtime imports.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8992
File: src/lib/litegraph/src/widgets/GradientSliderWidget.ts:18-18
Timestamp: 2026-02-20T21:08:19.814Z
Learning: When drawing with CanvasRenderingContext2D in TypeScript/JavaScript, wrap the drawing logic with ctx.save() at the start and ctx.restore() at the end to preserve and restore the canvas state. Do not manually destructure and restore individual properties (e.g., fillStyle, strokeStyle); rely on save/restore to manage state changes in a scoped manner. This should be applied to all TS files that perform canvas drawing.
Learnt from: dante01yoon
Repo: Comfy-Org/ComfyUI_frontend PR: 9075
File: src/scripts/api.featureFlags.test.ts:237-268
Timestamp: 2026-02-22T04:27:33.379Z
Learning: In Vite/Vitest, import.meta.env.DEV is true for any mode that is not 'production' (i.e., DEV is the opposite of PROD, and can be true in 'test', 'development', etc.). Do not assume DEV implies only 'development' mode. When reviewing code and tests, treat DEV as a non-production flag and verify environment-specific logic accordingly. Reference: https://vite.dev/guide/env-and-mode#modes
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/scripts/domWidget.ts (1)
2-2: Separateimport typefrom value imports.Per coding guidelines, use separate
import typestatements instead of inlinetypein mixed imports.♻️ Proposed fix
-import { type Component, effectScope, toRaw, watch } from 'vue' +import type { Component } from 'vue' +import { effectScope, toRaw, watch } from 'vue'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scripts/domWidget.ts` at line 2, Change the mixed import to use a dedicated type-only import for Component and a separate value import for the runtime helpers: replace the single line that imports "Component, effectScope, toRaw, watch" with two statements so Component is imported via "import type { Component } from 'vue'" and the others via "import { effectScope, toRaw, watch } from 'vue'"; update the import near the top of domWidget.ts where those symbols are referenced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/scripts/domWidget.ts`:
- Line 2: Change the mixed import to use a dedicated type-only import for
Component and a separate value import for the runtime helpers: replace the
single line that imports "Component, effectScope, toRaw, watch" with two
statements so Component is imported via "import type { Component } from 'vue'"
and the others via "import { effectScope, toRaw, watch } from 'vue'"; update the
import near the top of domWidget.ts where those symbols are referenced.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/composables/element/useDomValueBridge.test.tssrc/composables/element/useDomValueBridge.tssrc/renderer/extensions/vueNodes/widgets/composables/useStringWidget.tssrc/scripts/domWidget.ts
4 tasks
christian-byrne
added
the
preview-cpu
… bridge - Add useDomValueBridge composable that bridges DOM element .value to Vue reactivity via Object.defineProperty interception + input events - Wire bridge into addDOMWidget for extension widget compatibility - Migrate useStringWidget from raw textarea + addDOMWidget to ComponentWidgetImpl + WidgetTextarea.vue - Migrate useMarkdownWidget from Tiptap + addDOMWidget to ComponentWidgetImpl + WidgetMarkdown.vue - Remove all Tiptap dependencies (no longer used anywhere) - Add spellcheck setting support to WidgetTextarea.vue Fixes #9194 Amp-Thread-ID: https://ampcode.com/threads/T-019c977f-02f8-701d-b258-95157da8c261
The migration to ComponentWidgetImpl dropped the .comfy-multiline-input CSS class that e2e tests and GraphCanvas.vue spellcheck logic depend on. Amp-Thread-ID: https://ampcode.com/threads/T-019c97e6-7c3f-7179-aebe-25a7de8d7405
- Guard non-configurable value descriptors in useDomValueBridge - Use useEventListener for auto-cleanup input listener - Use nullish coalescing for placeholder fallback - Make bridge lifecycle-safe for subgraph restructuring - Add non-configurable descriptor test Amp-Thread-ID: https://ampcode.com/threads/T-019c98be-fa4e-77fa-aa8e-74ec9b709624
christian-byrne
force-pushed
the
fix/dom-widget-value-ownership
branch
from
f96d45c to
50bd1ee
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/composables/element/useDomValueBridge.ts (1)
49-52: Preserve original descriptor flags when overridingelement.value.The override currently hardcodes
enumerable: true, which can change observable element behavior (Object.keys, descriptor introspection). Prefer preserving existing/native enumerability.♻️ Suggested diff
if (canOverride) { + const enumerable = + existingDescriptor?.enumerable ?? nativeDescriptor?.enumerable ?? true Object.defineProperty(element, 'value', { configurable: true, - enumerable: true, + enumerable, get() { return prevGet.call(element) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/composables/element/useDomValueBridge.ts` around lines 49 - 52, The override for element's 'value' property currently hardcodes enumerable: true; instead, read the existing property descriptor (via Object.getOwnPropertyDescriptor on the element or its prototype) and preserve its flags (enumerable, configurable, and any accessors/writable) when calling Object.defineProperty for 'value' inside useDomValueBridge; ensure you fall back to sensible defaults if no descriptor exists so you don't change observable behavior of methods like Object.keys or descriptor inspection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/composables/element/useDomValueBridge.ts`:
- Around line 49-52: The override for element's 'value' property currently
hardcodes enumerable: true; instead, read the existing property descriptor (via
Object.getOwnPropertyDescriptor on the element or its prototype) and preserve
its flags (enumerable, configurable, and any accessors/writable) when calling
Object.defineProperty for 'value' inside useDomValueBridge; ensure you fall back
to sensible defaults if no descriptor exists so you don't change observable
behavior of methods like Object.keys or descriptor inspection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 641d0e58-ed19-4ee3-ab5f-3162c3e6fb13
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
package.jsonpnpm-workspace.yamlsrc/composables/element/useDomValueBridge.test.tssrc/composables/element/useDomValueBridge.tssrc/renderer/extensions/vueNodes/widgets/components/WidgetTextarea.test.tssrc/renderer/extensions/vueNodes/widgets/components/WidgetTextarea.vue
💤 Files with no reviewable changes (2)
- pnpm-workspace.yaml
- package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/extensions/vueNodes/widgets/components/WidgetTextarea.vue
christian-byrne
added
New Browser Test Expectations
DrJKL
removed
the
New Browser Test Expectations
DrJKL
added
New Browser Test Expectations
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
3 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
Migrate internal DOM widgets (textarea, markdown) from raw DOM element creation via
addDOMWidgettoComponentWidgetImpl, and add an element value bridge for extension compatibility.Changes
useDomValueBridgecomposable — a VuecustomRefbridged to a DOM element.valueviaObject.definePropertyinterception +inputevent listener. Wired intoaddDOMWidgetso extension widgets that writewidget.inputEl.value = "x"(bypassingwidget.value) sync to the store.useStringWidgetrewritten to useComponentWidgetImpl+WidgetTextarea.vueinstead of creating an orphaned raw<textarea>viaaddDOMWidget. Removes ~92 lines of manual DOM setup, event listeners, and store sync code.useMarkdownWidgetrewritten to useComponentWidgetImpl+WidgetMarkdown.vueinstead of Tiptap editor + raw DOM. Removes all Tiptap dependencies from the project.WidgetTextarea.vuegains spellcheck setting support anddata-testid.@tiptap/core,@tiptap/extension-link,@tiptap/extension-table,@tiptap/extension-table-cell,@tiptap/extension-table-header,@tiptap/extension-table-row,@tiptap/pm,@tiptap/starter-kit,tiptap-markdown)useMarkdownWidget.tscreated a Tiptap editor witheditable: false— it was only used as a markdown-to-HTML renderer, not as a WYSIWYG editor. Editing was done via a raw<textarea>appended alongside it.WidgetMarkdown.vue(which predates this PR) already replaces that same pattern usingmarkedwith GFM support for rendering + a<textarea>for editing. Table rendering is preserved viamarked'sgfm: trueoption, and link rendering is handled by the custommarkedrenderer inmarkdownRendererUtil.ts.Review Focus
useDomValueBridgeintercepts both programmatic writes (element.value = "x") and user input events. It chains through any existingObject.definePropertyon the element (e.g. from extensions).typepreserved:customtextfor textarea,MARKDOWNfor markdown — ensures registry mappings and extension compatibility.AUDIO_UI) intentionally deferred — the<audio>element is the API surface (.src,.classList, playback methods), not just a rendering detail.Fixes #9194
┆Issue is synchronized with this Notion page by Unito