Comfy-Org / Comfy-Org/ComfyUI_frontend

bug: useMarkdownWidget input listener bound to wrong element + module-level Tiptap configure

Open
#11,499 0 comments 0 reactions 0 assignees View on GitHub
area:vue-migration area:widgets Potential Bug
Dominant language
TypeScript
Stars
2k
Forks
704
Avg merge
1d 8h
Merged PRs (30d)
512

Description

## Context

Surfaced during code review on #11409 (MMB detection refactor). These are **pre-existing bugs** in `useMarkdownWidget.ts` (and one shared with `useStringWidget.ts`), not introduced by #11409, but discovered while reviewing adjacent pointer handler changes. Filed separately to keep #11409 focused.

## Issue 1 — Input listener bound to Tiptap root, not textarea

**File**: `src/renderer/extensions/vueNodes/widgets/composables/useMarkdownWidget.ts:68-73`

The widget adds an `input` listener to `inputEl` (the Tiptap editor root), then checks `event.target instanceof HTMLTextAreaElement` to sync `widget.value`. The Tiptap root is **not** a `` — the check only passes when a hidden textarea's input bubbles up, which means the user's edits inside the Tiptap editor **silently skip** the `widget.value` sync.

**Impact**: Markdown widget edits may not persist to the graph's node state until a different code path triggers sync.

**Fix options**:
- Bind `input` listener directly to the textarea element.
- Listen to Tiptap's `onUpdate` lifecycle and call `widget.callback?.(widget.value)` there.

## Issue 2 — \`TiptapMarkdown.configure(...)\` mutates a module-level singleton per widget

**File**: `src/renderer/extensions/vueNodes/widgets/composables/useMarkdownWidget.ts:25-29`

Every widget instantiation calls `TiptapMarkdown.configure({...})`, which mutates a shared module-level extension. Two widgets with different options can race and clobber each other.

**Fix**: Hoist `.configure(...)` to module init so the extension is configured exactly once. Per-widget options should be passed via Tiptap's per-editor configuration.

## Issue 3 — Tiptap editor instance is never destroyed on widget removal

**File**: `src/renderer/extensions/vueNodes/widgets/composables/useMarkdownWidget.ts:30-46`

`new TiptapEditor({...})` is created per widget, but `editor.destroy()` is never called. When the widget's node is removed, the ProseMirror view, extension chain, and the DOM element they reference remain in memory. Each markdown widget that has ever existed accumulates; over a long editing session with repeated node add/remove cycles, this adds up.

**Fix**: Hook into the DOM widget's removal lifecycle (or `onRemoved` on the parent node) and call `editor.destroy()` there.

**Impact**: Memory growth during long editing sessions that repeatedly create/delete Note nodes.

## Issue 4 — Widget-store sync duplication across useStringWidget / useMarkdownWidget

**Files**:
- `src/renderer/extensions/vueNodes/widgets/composables/useStringWidget.ts:14-15` (standing TODO)
- `src/renderer/extensions/vueNodes/widgets/composables/useMarkdownWidget.ts:18-19` (standing TODO)

Both composables carry a standing TODO:

> TODO: This widget manually syncs with widgetValueStore via getValue/setValue. Consolidate with useStringWidget/useMarkdownWidget into shared helpers (domWidgetHelpers.ts).

The `resolveNodeRootGraphId` / `widgetStore.getWidget` / value read/write trio is duplicated verbatim between the two widgets' `addDOMWidget(..., { getValue, setValue })` options. Each future DOM widget that wants store-backed values re-derives this.

**Fix**: Extract a shared `createStoreBackedDOMWidgetValue(node, name)` helper that returns the `{ getValue, setValue }` pair.

**Impact**: Code-smell / change amplification. No functional bug today, but every new DOM widget that needs store persistence repeats the pattern.

## Acceptance criteria

- [ ] Typing in a Markdown widget's editor updates `widget.value` without relying on blur/focus side effects.
- [ ] Two Markdown widgets with different configurations do not corrupt each other.
- [ ] Removing a Note node with a markdown widget frees the Tiptap editor (no DOM / ProseMirror view retained).
- [ ] `getValue` / `setValue` widget-store sync is extracted into a shared helper, consumed by both useStringWidget and useMarkdownWidget.
- [ ] Unit tests covering all four fixes.

## Related

- PR #11409 (review context)
- Reviewer: @christian-byrne (via review on #11409)

Contributor guide

Open the contributing guide

Research direction

Start with src/renderer/extensions/vueNodes/widgets/composables/useMarkdownWidget.ts and compare its widget-store handling with useStringWidget.ts. Trace the TiptapEditor creation, input handling, and DOM widget removal lifecycle, then inspect the shared helper location in domWidgetHelpers.ts. Add unit tests for all four acceptance criteria and confirm each test passes.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
frontend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.