fix: add beforeChange/afterChange to convertToSubgraph for proper undo

[godwiniheuwa]

Summary

• Adds missing beforeChange() and afterChange() lifecycle calls to convertToSubgraph() method

Problem

When converting nodes to a subgraph and then pressing Ctrl+Z to undo, the node positions were being changed from their original locations instead of being properly restored.

Root Cause

The convertToSubgraph() method in LGraph.ts was missing the beforeChange() and afterChange() lifecycle calls that are needed for proper undo/redo state tracking. These calls record the graph state before and after modifications.

The inverse operation unpackSubgraph() already has these calls (see line 1742), so this is simply matching the existing pattern.

Solution

Add beforeChange() at the start of the method (after validation) and afterChange() before the return.

Testing

1. Create a workflow with several nodes positioned in specific locations
2. Select 2-3 nodes
3. Right-click → "Convert Selection to Subgraph"
4. Press Ctrl+Z to undo
5. Verify nodes return to their exact original positions

Fixes comfyanonymous/ComfyUI#11514

┆Issue is synchronized with this Notion page by Unito

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds undo-friendly wrapping to subgraph conversion and unpacking operations in LGraph.ts by introducing private implementation methods and wrapping them with beforeChange() and afterChange() lifecycle calls to support undo functionality when operations are canceled.

Changes

Cohort / File(s)	Summary
Undo-wrapped subgraph operations src/lib/litegraph/src/LGraph.ts	Refactored convertToSubgraph() to wrap core logic in private _convertToSubgraphImpl() with beforeChange()/afterChange() calls; similarly refactored unpackSubgraph() with private _unpackSubgraphImpl() wrapper. Minor formatting adjustments around modified regions.

Possibly related PRs

• fix: unpacking a missing node causes it to disappear #7341: Modifies unpackSubgraph() behavior to handle missing node placeholders, complementing this PR's undo support additions to the same operation.

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	PR adds beforeChange/afterChange lifecycle calls to convertToSubgraph and wraps implementation in try-finally, directly addressing issue #11514's requirement to preserve node positions during undo/cancel operations.
Out of Scope Changes check	✅ Passed	All changes are scoped to LGraph.ts with focused modifications to convertToSubgraph and unpackSubgraph methods; no unrelated alterations detected.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 33d8cb7 and 008b1db.

📒 Files selected for processing (1)

• src/lib/litegraph/src/LGraph.ts

🧰 Additional context used

📓 Path-based instructions (9)

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/lib/litegraph/src/LGraph.ts

src/**/*.ts

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

src/**/*.ts: Use es-toolkit for utility functions
Use TypeScript for type safety

Files:

• src/lib/litegraph/src/LGraph.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

Files:

• src/lib/litegraph/src/LGraph.ts

src/**/*.{vue,ts,tsx}

📄 CodeRabbit inference engine (src/CLAUDE.md)

Follow Vue 3 composition API style guide

Files:

• src/lib/litegraph/src/LGraph.ts

src/lib/litegraph/**/*.{js,ts,jsx,tsx}

📄 CodeRabbit inference engine (src/lib/litegraph/CLAUDE.md)

src/lib/litegraph/**/*.{js,ts,jsx,tsx}: Run ESLint instead of manually figuring out whitespace fixes or other trivial style concerns using the pnpm lint:fix command
Take advantage of TypedArray subarray when appropriate
The size and pos properties of Rectangle share the same array buffer (subarray); they may be used to set the rectangle's size and position
Prefer single line if syntax over adding curly braces, when the statement has a very concise expression and concise, single line statement
Do not replace &&= or ||= with = when there is no reason to do so. If you do find a reason to remove either &&= or ||=, leave a comment explaining why the removal occurred
When writing methods, prefer returning idiomatic JavaScript undefined over null

Files:

• src/lib/litegraph/src/LGraph.ts

src/lib/litegraph/**/*.{ts,tsx}

📄 CodeRabbit inference engine (src/lib/litegraph/CLAUDE.md)

Type assertions are an absolute last resort. In almost all cases, they are a crutch that leads to brittle code

Files:

• src/lib/litegraph/src/LGraph.ts

**/*.{ts,tsx,vue}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx,vue}: Use TypeScript exclusively; do not write new JavaScript code
Use sorted and grouped imports organized by plugin/source
Enforce ESLint rules including Vue + TypeScript rules, disallow floating promises, disallow unused imports, and restrict i18n raw text in templates
Do not use any type or as any type assertions; fix the underlying type issue instead
Write code that is expressive and self-documenting; avoid redundant comments and clean as you go
Keep functions short and functional; minimize nesting and follow the arrow anti-pattern
Avoid mutable state; prefer immutability and assignment at point of declaration
Use function declarations instead of function expressions when possible
Use es-toolkit for utility functions
Implement proper error handling in code

Files:

• src/lib/litegraph/src/LGraph.ts

**/*.{ts,tsx,vue,js,jsx,json,css}

📄 CodeRabbit inference engine (AGENTS.md)

Apply Prettier formatting with 2-space indentation, single quotes, no trailing semicolons, and 80-character line width

Files:

• src/lib/litegraph/src/LGraph.ts

**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Minimize the surface area (exported values) of each module and composable

Files:

• src/lib/litegraph/src/LGraph.ts

🧠 Learnings (9)

📓 Common learnings

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{test,spec}.{ts,tsx} : Use provided test helpers `createTestSubgraph` and `createTestSubgraphNode` from `./fixtures/subgraphHelpers` for consistent subgraph test setup

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{test,spec}.{ts,tsx} : When writing tests for subgraph-related code, always import from the barrel export at `@/lib/litegraph/src/litegraph` to avoid circular dependency issues

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{js,ts,jsx,tsx} : Take advantage of `TypedArray` `subarray` when appropriate

📚 Learning: 2025-11-24T19:47:56.371Z

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{test,spec}.{ts,tsx} : Use provided test helpers `createTestSubgraph` and `createTestSubgraphNode` from `./fixtures/subgraphHelpers` for consistent subgraph test setup

Applied to files:

• src/lib/litegraph/src/LGraph.ts

📚 Learning: 2025-11-24T19:47:56.371Z

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{js,ts,jsx,tsx} : Take advantage of `TypedArray` `subarray` when appropriate

Applied to files:

• src/lib/litegraph/src/LGraph.ts

📚 Learning: 2025-11-24T19:47:56.371Z

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{test,spec}.{ts,tsx} : When writing tests for subgraph-related code, always import from the barrel export at `@/lib/litegraph/src/litegraph` to avoid circular dependency issues

Applied to files:

• src/lib/litegraph/src/LGraph.ts

📚 Learning: 2025-11-24T19:47:56.371Z

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{js,ts,jsx,tsx} : Do not replace `&&=` or `||=` with `=` when there is no reason to do so. If you do find a reason to remove either `&&=` or `||=`, leave a comment explaining why the removal occurred

Applied to files:

• src/lib/litegraph/src/LGraph.ts

📚 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/lib/litegraph/src/LGraph.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/lib/litegraph/src/LGraph.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/lib/litegraph/src/LGraph.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/lib/litegraph/src/LGraph.ts

🔇 Additional comments (4)

src/lib/litegraph/src/LGraph.ts (4)

1506-1522: LGTM! Proper undo/redo lifecycle wrapping.

The try-finally pattern ensures afterChange() is always called, even if the conversion throws an exception. This correctly fixes the undo behavior described in the linked issue.

---

1746-1762: LGTM! Consistent pattern applied to unpackSubgraph.

The try-finally wrapper mirrors the convertToSubgraph implementation, ensuring proper undo state tracking for both operations. This consistency is good for maintainability.

---

1611-1613: Verify nested beforeChange/afterChange behavior.

The remove() method at lines 945-1000 also calls beforeChange()/afterChange() internally. With the new wrapper in convertToSubgraph, this creates nested undo state changes when nodes are removed during conversion.

This may be intentional for granular undo tracking, but could also cause unexpected behavior where the undo history captures intermediate states. Please verify that this nested pattern produces the expected undo behavior (i.e., Ctrl+Z restores all original positions in a single step, not incrementally).

---

1524-1527: Consider using private keyword for type safety.

The method uses underscore prefix convention but lacks the TypeScript private keyword. For consistent encapsulation with the rest of the codebase, consider adding the private modifier.

🔎 Suggested change

-  private _convertToSubgraphImpl(items: Set<Positionable>): {
+  private _convertToSubgraphImpl(items: Set<Positionable>): {

Wait, looking again - the private keyword is already present on line 1524. The method declaration is correct.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

🎨 Storybook Build Status

Build is starting...

⏰ Started at: 12/31/2025, 11:04:57 AM UTC

🚀 Building Storybook

• 📦 Installing dependencies...
• 🔧 Building Storybook components...
• 🌐 Preparing deployment to Cloudflare Pages...

---

⏱️ Please wait while the Storybook build is in progress...

[github-actions]

🎭 Playwright Test Results

Tests are starting...

⏰ Started at: 12/31/2025, 11:04:56 AM UTC

🚀 Running Tests

• 🧪 chromium: Running tests...
• 🧪 chromium-0.5x: Running tests...
• 🧪 chromium-2x: Running tests...
• 🧪 mobile-chrome: Running tests...

---

⏱️ Please wait while tests are running...

[RUiNtheExtinct]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.