Conversation
dosubot
bot
added
the
size:S
🎨 Storybook: ✅ Built — View Storybook |
🎭 Playwright: ✅ 551 passed, 0 failed · 4 flaky📊 Browser Reports
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReplaces local cloning with a centralized Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
📦 Bundle: 4.49 MB gzip 🔴 +188 BDetailsSummary
Category Glance App Entry Points — 17.7 kB (baseline 17.7 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 913 kB (baseline 913 kB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 72.1 kB (baseline 72.1 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed Panels & Settings — 436 kB (baseline 436 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed User & Accounts — 16 kB (baseline 16 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 5 added / 5 removed Editors & Dialogs — 736 B (baseline 736 B) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 1 added / 1 removed UI Components — 47 kB (baseline 47 kB) • ⚪ 0 BReusable component library chunks
Status: 5 added / 5 removed Data & Services — 2.73 MB (baseline 2.73 MB) • 🔴 +85 BStores, services, APIs, and repositories
Status: 13 added / 13 removed Utilities & Hooks — 55.5 kB (baseline 55.5 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 11 added / 11 removed Vendor & Third-Party — 8.86 MB (baseline 8.86 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 7.91 MB (baseline 7.91 MB) • 🔴 +16 BBundles that do not match a named category
Status: 51 added / 51 removed |
⚡ Performance Report
Raw data{
"timestamp": "2026-03-04T23:30:02.364Z",
"gitSha": "100baa46a662df963a5f3f21be094087a760d90f",
"branch": "fix/changetracker-use-shared-clone",
"measurements": [
{
"name": "canvas-idle",
"durationMs": 2032.2830000000067,
"styleRecalcs": 122,
"styleRecalcDurationMs": 24.158,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 531.9430000000001,
"heapDeltaBytes": -3385268
},
{
"name": "canvas-mouse-sweep",
"durationMs": 2199.5390000000157,
"styleRecalcs": 195,
"styleRecalcDurationMs": 73.389,
"layouts": 12,
"layoutDurationMs": 4.183,
"taskDurationMs": 1210.342,
"heapDeltaBytes": -4123308
},
{
"name": "dom-widget-clipping",
"durationMs": 613.0400000000122,
"styleRecalcs": 45,
"styleRecalcDurationMs": 15.695,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 401.56,
"heapDeltaBytes": 7525052
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 677.0720000000097,
"styleRecalcs": 79,
"styleRecalcDurationMs": 19.921999999999997,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 464.398,
"heapDeltaBytes": 15752920
},
{
"name": "subgraph-idle",
"durationMs": 2032.5899999999706,
"styleRecalcs": 123,
"styleRecalcDurationMs": 31.877000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 597.0070000000001,
"heapDeltaBytes": -3773084
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 2063.692000000003,
"styleRecalcs": 178,
"styleRecalcDurationMs": 71.59199999999998,
"layouts": 17,
"layoutDurationMs": 4.710000000000001,
"taskDurationMs": 1140.422,
"heapDeltaBytes": -4491984
}
]
} |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/stores/imagePreviewStore.ts (1)
368-370: MakerestoreOutputs()clone defensively for true snapshot/restore symmetry.
snapshotOutputs()deep-clones, butrestoreOutputs()currently stores the caller object by reference. Cloning inrestoreOutputs()keeps store internals isolated and avoids external mutation bleed-through.As per coding guidelines, "Only expose state and actions that are used externally; keep internal state private."Proposed change
function restoreOutputs( outputs: Record<string, ExecutedWsMessage['output']> ) { - app.nodeOutputs = outputs - nodeOutputs.value = { ...outputs } + const snapshot = structuredClone(outputs) + app.nodeOutputs = snapshot + nodeOutputs.value = { ...snapshot } }Also applies to: 372-377
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/imagePreviewStore.ts` around lines 368 - 370, The restoreOutputs function currently assigns the passed-in object by reference which breaks snapshot/restore symmetry with snapshotOutputs; update restoreOutputs to defensively deep-clone the incoming output (e.g., via structuredClone) before assigning to app.nodeOutputs so store internals remain isolated from external mutations — ensure you reference restoreOutputs, snapshotOutputs, app.nodeOutputs and the ExecutedWsMessage['output'] shape when making the change.
🤖 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/stores/imagePreviewStore.ts`:
- Around line 368-370: The restoreOutputs function currently assigns the
passed-in object by reference which breaks snapshot/restore symmetry with
snapshotOutputs; update restoreOutputs to defensively deep-clone the incoming
output (e.g., via structuredClone) before assigning to app.nodeOutputs so store
internals remain isolated from external mutations — ensure you reference
restoreOutputs, snapshotOutputs, app.nodeOutputs and the
ExecutedWsMessage['output'] shape when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c539840f-33d9-4124-8b29-4f791ccd1092
📒 Files selected for processing (2)
src/scripts/changeTracker.tssrc/stores/imagePreviewStore.ts
christian-byrne
force-pushed
the
fix/changetracker-use-shared-clone
branch
from
4af647e to
05265c1
Compare
christian-byrne
force-pushed
the
fix/changetracker-use-shared-clone
branch
from
05265c1 to
1676447
Compare
dosubot
bot
added
size:XS
DrJKL
approved these changes
Mar 5, 2026
DrJKL
reviewed
Mar 5, 2026
Comment on lines
21
to
23
| function clone<T>(obj: T): T { | ||
| return JSON.parse(JSON.stringify(obj)) | ||
| } |
Contributor
There was a problem hiding this comment.
@coderabbitai Make an issue to follow this pattern for the rest of the uses of clone in this file.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
@DrJKL, I've created a GitHub issue to track replacing the remaining local clone() usages in src/scripts/changeTracker.ts with the shared clone util from @/scripts/utils. The issue covers three remaining call sites:
reset()—this.activeState = clone(state)reset()—this.initialState = clone(this.activeState)checkState()—const currentState = clone(app.rootGraph.serialize()) as ComfyWorkflowJSON
The fix would remove the local function clone<T> definition and replace it with an import of clone from @/scripts/utils.
🏁 Script executed:
gh issue create \
--repo Comfy-Org/ComfyUI_frontend \
--title "refactor(changeTracker): replace local clone() with shared clone util from `@/scripts/utils`" \
--assignee DrJKL \
--body "## Summary
Follow-up to `#9387`. The PR replaced \`clone(app.nodeOutputs)\` in \`changeTracker.store()\` with \`useNodeOutputStore().snapshotOutputs()\`, which internally uses the shared \`clone()\` utility from \`@/scripts/utils\` (preferring \`structuredClone\` with a JSON fallback over the local \`JSON.parse(JSON.stringify(...))\` implementation).
The remaining usages of the local \`clone\` function in \`src/scripts/changeTracker.ts\` should be migrated to use the shared utility as well.
## Remaining call sites
- \`reset()\`: \`this.activeState = clone(state)\`
- \`reset()\`: \`this.initialState = clone(this.activeState)\`
- \`checkState()\`: \`const currentState = clone(app.rootGraph.serialize()) as ComfyWorkflowJSON\`
## Proposed change
1. Remove the local \`clone\` function definition (~line 22 in \`src/scripts/changeTracker.ts\`).
2. Add \`import { clone } from '@/scripts/utils'\`.
3. Replace all remaining local \`clone()\` call sites with the imported utility.
## Context
- PR: https://github.com/Comfy-Org/ComfyUI_frontend/pull/9387
- Review comment: https://github.com/Comfy-Org/ComfyUI_frontend/pull/9387#discussion_r2887461142
- Requested by: `@DrJKL`
"
Length of output: 57
@DrJKL, the issue has been created: #9414
It tracks replacing the three remaining local clone() call sites in src/scripts/changeTracker.ts with the shared clone util from @/scripts/utils, and removing the local function definition.
🧠 Learnings used
Learnt from: pythongosssss
Repo: Comfy-Org/ComfyUI_frontend PR: 9393
File: src/stores/appModeStore.ts:22-27
Timestamp: 2026-03-04T23:21:14.479Z
Learning: In `src/stores/appModeStore.ts`, the `hasNodes` computed reads `app.rootGraph?.nodes?.length` (non-reactive) but tracks `workflowStore.activeWorkflow` and `mode.value` as reactive dependencies. This is intentional and correct: node edits are only possible in graph mode, so `hasNodes` will always re-evaluate when switching into app/builder mode via the `mode.value` dependency change.
Learnt from: JemiloII
Repo: Comfy-Org/ComfyUI_frontend PR: 8282
File: src/composables/usePaste.test.ts:372-383
Timestamp: 2026-01-23T21:06:26.840Z
Learning: The `cloneDataTransfer` function in `src/composables/usePaste.ts` is used to persist DataTransfer clipboard data for async operations (e.g., awaiting node creation during paste). DataTransfer objects have ephemeral data that doesn't survive async boundaries, so this cloning is necessary to capture files and string data before the paste event completes.
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: 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: Myestery
Repo: Comfy-Org/ComfyUI_frontend PR: 8961
File: src/scripts/app.ts:1216-1220
Timestamp: 2026-02-18T20:58:43.671Z
Learning: In src/scripts/app.ts, using api.getFolderPaths() inside Promise.all is safe because the underlying API (src/scripts/api.ts) catches errors and resolves with a non-rejection value (null or {}) when the call fails. No additional per-call error handling is required for this Promise.all usage. Downstream code should handle possible empty object or null values returned by this API.
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
Myestery
pushed a commit
that referenced
this pull request
Mar 5, 2026
…puts snapshot (#9387) ## Summary Follow-up to #9380. Replaces local `clone()` with shared util and centralizes output snapshotting. ## Changes - **What**: Replaced local `JSON.parse(JSON.stringify)` clone in `changeTracker.ts` with shared `clone()` from `@/scripts/utils` (prefers `structuredClone` with JSON fallback). Added `snapshotOutputs()` to `useNodeOutputStore` as symmetric counterpart to existing `restoreOutputs()`, and wired `changeTracker.store()` to use it. - **Breaking**: None ## Review Focus Symmetry between `snapshotOutputs()` and `restoreOutputs()` in the node output store. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9387-refactor-changeTracker-use-shared-clone-util-and-centralize-nodeOutputs-snapshot-3196d73d365081a289c3cb414f57929e) by [Unito](https://www.unito.io)
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
2 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
Follow-up to #9380. Replaces local
clone()with shared util and centralizes output snapshotting.Changes
JSON.parse(JSON.stringify)clone inchangeTracker.tswith sharedclone()from@/scripts/utils(prefersstructuredClonewith JSON fallback). AddedsnapshotOutputs()touseNodeOutputStoreas symmetric counterpart to existingrestoreOutputs(), and wiredchangeTracker.store()to use it.Review Focus
Symmetry between
snapshotOutputs()andrestoreOutputs()in the node output store.┆Issue is synchronized with this Notion page by Unito