Cleanup app.graph usage #7399
Cleanup app.graph usage #7399
Conversation
Causing test failures that need investigation
AustinMroz
requested review from
a team,
christian-byrne,
ltdrdata and
viva-jinyi
as code owners
dosubot
bot
added
the
size:L
📝 WalkthroughWalkthroughRefactors code to use app.rootGraph instead of app.graph for graph operations and switches graph-level dirty signaling from setDirtyCanvas(...) to canvas.setDirty(...). Changes touch components, composables, services, extensions, stores, scripts, types, and tests; no public API signatures modified except internal getters/types. Changes
Possibly related PRs
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
🎭 Playwright Test Results⏰ Completed at: 12/12/2025, 03:25:24 AM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 12/12/2025, 03:15:44 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 3.24 MB (baseline 3.24 MB) • 🔴 +64 BMain entry bundles and manifests
Status: 3 added / 3 removed Graph Workspace — 981 kB (baseline 981 kB) • 🟢 -86 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 6.54 kB (baseline 6.54 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 1 added / 1 removed Panels & Settings — 298 kB (baseline 298 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 6 added / 6 removed UI Components — 178 kB (baseline 178 kB) • 🔴 +4 BReusable component library chunks
Status: 7 added / 7 removed Data & Services — 12.5 kB (baseline 12.5 kB) • ⚪ 0 BStores, services, APIs, and repositories
Status: 2 added / 2 removed Utilities & Hooks — 3.18 kB (baseline 3.18 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 1 added / 1 removed Vendor & Third-Party — 8.56 MB (baseline 8.56 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 3.81 MB (baseline 3.81 MB) • 🔴 +18 BBundles that do not match a named category
Status: 18 added / 18 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 (8)
tests-ui/tests/composables/useMissingNodes.test.ts (1)
43-49: Fix test setup to consistently useapp.rootGraph(avoidapp.graphfalse positives / TS failures).
Right now the module mock doesn’t definerootGraph,beforeEachresetsapp.graph.nodes, and one test still assignsapp.graph(Line 494). With this PR makingapp.graphunknown, these tests can either fail typecheck or validate the wrong behavior.vi.mock('@/scripts/app', () => ({ app: { - graph: { - nodes: [] - } + // Keep `graph` for legacy/custom-node compatibility, but tests should use `rootGraph`. + graph: { nodes: [] }, + rootGraph: { nodes: [], subgraphs: new Map() } } })) @@ - // Reset app.graph.nodes - // @ts-expect-error - app.graph.nodes is readonly, but we need to modify it for testing. - app.graph.nodes = [] + // Reset app.rootGraph.nodes (code under test reads from rootGraph) + // @ts-expect-error - app.rootGraph.nodes is readonly in app typings, but tests need to mutate it. + app.rootGraph.nodes = [] @@ - // @ts-expect-error - Mocking app.graph for testing - app.graph = mockGraph + // @ts-expect-error - Mocking app.rootGraph for testing + app.rootGraph = mockGraphAlso applies to: 110-113, 468-495, 588-590
tests-ui/tests/store/workflowStore.test.ts (1)
30-35: AddrootGraphto the@/scripts/apptest mock to match new usage.
Right now the mock omits it, then the test “patches” it later (Line 625), which is easy to break as typings tighten.vi.mock('@/scripts/app', () => ({ app: { - canvas: {} // Start with empty canvas object + canvas: {}, // Start with empty canvas object + rootGraph: null } }))Also applies to: 613-627
src/components/dialog/content/ErrorDialogContent.vue (1)
120-146: Avoid report-generation crash ifapp.rootGraphisn’t ready.
IfrootGraphcan be unset in any startup/error edge case,app.rootGraph.serialize()will throw and prevent the dialog from showing useful info.- workflow: app.rootGraph.serialize(), + workflow: app.rootGraph?.serialize?.() ?? null,(Adjust
generateErrorReport/ErrorReportDatatyping ifworkflowcannot be nullable.)src/extensions/core/webcamCapture.ts (1)
95-102: Useapp.canvas?.setDirty(true)to match codebase patterns and prevent potential runtime errors.The call on line 100 is inside an async
img.onloadcallback, which may execute afterapp.canvasis torn down. Throughout the codebase (e.g.,dynamicWidgets.ts,useSlotLinkInteraction.ts,useNodeBadge.ts), all similar calls use optional chaining (app.canvas?.setDirty()). Change line 100 to use the same defensive pattern.src/components/graph/TitleEditor.vue (1)
50-66: Correct migration to canvas dirtying (app.canvas.setDirty).
Consider a defensive guard to avoid rare runtime failures in tests/teardown.- app.canvas.setDirty(true, true) + app.canvas?.setDirty(true, true)src/extensions/core/nodeTemplates.ts (1)
364-387: Handlenodenot found instead of continuing into group/nodeData logic.
Withapp.canvas.graph?.getNodeById,nodecan beundefined; current code may throw or mis-template partial selections.const nodeIds = Object.keys(app.canvas.selected_nodes) for (let i = 0; i < nodeIds.length; i++) { const node = app.canvas.graph?.getNodeById(nodeIds[i]) + if (!node) { + // Selection can outlive graph swaps / subgraph navigation. + continue + } const nodeData = node?.constructor.nodeDataIf you want to hard-fail, fail with a user-facing toast (instead of a TypeError) and stop template creation.
src/platform/workflow/management/stores/workflowStore.ts (1)
636-658: Avoid unguarded use ofcomfyApp.rootGraphinexecutionIdToCurrentId.
IfrootGraphis not ready,getSubgraphsFromInstanceIds(graph, …)will crash.- const graph = comfyApp.rootGraph + const graph = comfyApp.rootGraph + if (!graph) returnAlso: consider making the FIXME actionable (reference the util name / file).
src/scripts/changeTracker.ts (1)
131-149: Fix guard incheckState()to avoid relying onapp.graphand to protectrootGraph.serialize().
Current code can still attemptapp.rootGraph.serialize()whenrootGraphis unset, and it keeps anapp.graphdependency around.checkState() { - if (!app.graph || this.changeCount) return - const currentState = clone(app.rootGraph.serialize()) as ComfyWorkflowJSON + const rootGraph = app.rootGraph + if (!rootGraph || this.changeCount) return + const currentState = clone(rootGraph.serialize()) as ComfyWorkflowJSON if (!this.activeState) { this.activeState = currentState return }
| const lgraphNode = computed(() => { | ||
| const locatorId = getLocatorIdFromNodeData(nodeData) | ||
| const rootGraph = app.graph?.rootGraph || app.graph | ||
| if (!rootGraph) return null | ||
| return getNodeByLocatorId(rootGraph, locatorId) | ||
| return getNodeByLocatorId(app.rootGraph, locatorId) | ||
| }) |
There was a problem hiding this comment.
Guard getNodeByLocatorId(app.rootGraph, …) against rootGraph being unset.
Right now lgraphNode can throw if app.rootGraph is temporarily undefined (startup, tests, or teardown).
const lgraphNode = computed(() => {
const locatorId = getLocatorIdFromNodeData(nodeData)
- return getNodeByLocatorId(app.rootGraph, locatorId)
+ const graph = app.rootGraph
+ return graph ? getNodeByLocatorId(graph, locatorId) : undefined
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const lgraphNode = computed(() => { | |
| const locatorId = getLocatorIdFromNodeData(nodeData) | |
| const rootGraph = app.graph?.rootGraph || app.graph | |
| if (!rootGraph) return null | |
| return getNodeByLocatorId(rootGraph, locatorId) | |
| return getNodeByLocatorId(app.rootGraph, locatorId) | |
| }) | |
| const lgraphNode = computed(() => { | |
| const locatorId = getLocatorIdFromNodeData(nodeData) | |
| const graph = app.rootGraph | |
| return graph ? getNodeByLocatorId(graph, locatorId) : undefined | |
| }) |
🤖 Prompt for AI Agents
In src/renderer/extensions/vueNodes/components/LGraphNode.vue around lines 429
to 432, the computed lgraphNode calls getNodeByLocatorId(app.rootGraph,
locatorId) without ensuring app.rootGraph exists, which can throw when rootGraph
is unset; update the computed to first check whether app.rootGraph is defined
(or use optional chaining) and return undefined/null if it's not, otherwise call
getNodeByLocatorId with the locatorId; keep the result type consistent so
callers handle the missing node.
| const litegraphNode = computed(() => { | ||
| if (!props.nodeId || !app.rootGraph) return null | ||
| return app.rootGraph.getNodeById(props.nodeId) as LGraphNode | null | ||
| if (!props.nodeId || !app.canvas.graph) return null | ||
| return app.canvas.graph.getNodeById(props.nodeId) as LGraphNode | null | ||
| }) |
There was a problem hiding this comment.
Guard app.canvas (not just app.canvas.graph) to avoid setup-time crashes.
app.canvas.graph will throw if app.canvas is undefined when this computed runs.
const litegraphNode = computed(() => {
- if (!props.nodeId || !app.canvas.graph) return null
- return app.canvas.graph.getNodeById(props.nodeId) as LGraphNode | null
+ const graph = app.canvas?.graph
+ if (!props.nodeId || !graph) return null
+ return graph.getNodeById(props.nodeId) as LGraphNode | null
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const litegraphNode = computed(() => { | |
| if (!props.nodeId || !app.rootGraph) return null | |
| return app.rootGraph.getNodeById(props.nodeId) as LGraphNode | null | |
| if (!props.nodeId || !app.canvas.graph) return null | |
| return app.canvas.graph.getNodeById(props.nodeId) as LGraphNode | null | |
| }) | |
| const litegraphNode = computed(() => { | |
| const graph = app.canvas?.graph | |
| if (!props.nodeId || !graph) return null | |
| return graph.getNodeById(props.nodeId) as LGraphNode | null | |
| }) |
🤖 Prompt for AI Agents
In src/renderer/extensions/vueNodes/widgets/components/WidgetAudioUI.vue around
lines 41 to 44, the computed currently accesses app.canvas.graph directly and
can throw if app.canvas is undefined; update the guard to check that app.canvas
exists before accessing .graph (i.e., return null when props.nodeId is falsy OR
app.canvas is falsy OR app.canvas.graph is falsy) and keep the returned type as
LGraphNode | null so the computed never dereferences graph when app.canvas is
missing.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the WidgetAudioUI.vue file to understand the props.nodeId type
fd WidgetAudioUI.vueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 139
🏁 Script executed:
#!/bin/bash
# Check for getNodeById signature in litegraph
rg -n --hidden -S "getNodeById" src/lib/litegraph/src/litegraph --context 3Repository: Comfy-Org/ComfyUI_frontend
Length of output: 141
🏁 Script executed:
#!/bin/bash
# Check broader usage patterns and function definitions
rg -n "getNodeById\s*\(" src --context 2 | head -100Repository: Comfy-Org/ComfyUI_frontend
Length of output: 5887
🏁 Script executed:
cat -n src/renderer/extensions/vueNodes/widgets/components/WidgetAudioUI.vueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 2278
🏁 Script executed:
# Find NodeId type definition
rg -n "type NodeId|interface NodeId" srcRepository: Comfy-Org/ComfyUI_frontend
Length of output: 544
🏁 Script executed:
# Search for litegraph getNodeById definition
rg -n "getNodeById" src/lib/litegraph --context 3Repository: Comfy-Org/ComfyUI_frontend
Length of output: 22263
🏁 Script executed:
# Check if there are similar patterns in the codebase
rg -n "computed.*getNodeById" src --context 2Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
# Verify the _nodes_by_id structure to confirm it accepts both number and string
rg -n "_nodes_by_id" src/lib/litegraph/src/LGraph.ts -B 2 -A 2 | head -50Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1422
Remove the redundant type assertion; getNodeById already accepts string IDs.
The getNodeById method's return type is already LGraphNode | null, so the as LGraphNode | null assertion is unnecessary. The NodeId type is number | string, and the method accepts both natively—no parsing or conversion required. Simply remove the assertion:
const litegraphNode = computed(() => {
if (!props.nodeId || !app.canvas.graph) return null
- return app.canvas.graph.getNodeById(props.nodeId) as LGraphNode | null
+ return app.canvas.graph.getNodeById(props.nodeId)
})Optionally, add an explicit return type to the computed for clarity:
-const litegraphNode = computed(() => {
+const litegraphNode = computed<LGraphNode | null>(() => {
if (!props.nodeId || !app.canvas.graph) return null
return app.canvas.graph.getNodeById(props.nodeId)
})🤖 Prompt for AI Agents
In src/renderer/extensions/vueNodes/widgets/components/WidgetAudioUI.vue around
lines 41 to 44, the computed getter uses an unnecessary type assertion "as
LGraphNode | null" on the result of app.canvas.graph.getNodeById; remove that
assertion because getNodeById already returns LGraphNode | null and accepts
string IDs, and optionally add an explicit return type for the computed (e.g.,
computed<LGraphNode | null>(...)) for clarity.
| const litegraphNode = computed(() => { | ||
| if (!props.nodeId || !app.rootGraph) return null | ||
| return app.rootGraph.getNodeById(props.nodeId) as LGraphNode | null | ||
| if (!props.nodeId || !app.canvas.graph) return null | ||
| return app.canvas.graph.getNodeById(props.nodeId) as LGraphNode | null | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find app type definitions and canvas property
fd -e ts -e tsx -e js | head -20
git ls-files | grep -E "(app|canvas)" | head -30Repository: Comfy-Org/ComfyUI_frontend
Length of output: 2397
🏁 Script executed:
#!/bin/bash
# Search for app.canvas type definition
rg "canvas\s*:\s*" -A 2 -t ts -t tsx | head -40Repository: Comfy-Org/ComfyUI_frontend
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Find getNodeById method definition
rg "getNodeById" -A 5 -t ts -t tsx | head -50Repository: Comfy-Org/ComfyUI_frontend
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Check the actual app object type and canvas property
rg "class.*App|interface.*App" -A 10 -t ts -t tsx | head -60Repository: Comfy-Org/ComfyUI_frontend
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Read the WidgetRecordAudio.vue file to see imports and app usage
head -120 src/renderer/extensions/vueNodes/widgets/components/WidgetRecordAudio.vueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 4226
🏁 Script executed:
#!/bin/bash
# Search for getNodeById in the codebase
rg "getNodeById" -A 3 | head -60Repository: Comfy-Org/ComfyUI_frontend
Length of output: 4174
🏁 Script executed:
#!/bin/bash
# Find app.canvas type definition
rg "canvas.*:.*\|" | head -40Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1377
🏁 Script executed:
#!/bin/bash
# Look for LGraph or app type definitions
rg "class LGraph|interface.*App|type.*App" | head -40Repository: Comfy-Org/ComfyUI_frontend
Length of output: 2050
🏁 Script executed:
#!/bin/bash
# Read the ComfyApp definition to check canvas type
head -200 src/scripts/app.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 7077
🏁 Script executed:
#!/bin/bash
# Look for canvas property in ComfyApp class
rg "canvas\s*:" src/scripts/app.ts -A 1 | head -20Repository: Comfy-Org/ComfyUI_frontend
Length of output: 382
🏁 Script executed:
#!/bin/bash
# Check LGraphCanvas type and its relationship to app
rg "class ComfyApp" -A 50 | head -100Repository: Comfy-Org/ComfyUI_frontend
Length of output: 3678
🏁 Script executed:
#!/bin/bash
# Search for where canvas is initialized/set in ComfyApp
rg "this\.canvas\s*=" src/scripts/app.ts -B 2 -A 2Repository: Comfy-Org/ComfyUI_frontend
Length of output: 279
🏁 Script executed:
#!/bin/bash
# Check LGraphCanvas class and its graph property
rg "class LGraphCanvas" -A 30 src/lib/litegraph/src/LGraphCanvas.ts | head -60Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1636
🏁 Script executed:
#!/bin/bash
# Search for graph property in LGraphCanvas
rg "graph\s*:" src/lib/litegraph/src/LGraphCanvas.ts -B 2 -A 2Repository: Comfy-Org/ComfyUI_frontend
Length of output: 2410
🏁 Script executed:
#!/bin/bash
# Find getNodeById method signature in LGraph
rg "getNodeById\(" src/lib/litegraph/src/LGraph.ts -A 5Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1471
🏁 Script executed:
#!/bin/bash
# Double-check: verify LGraphCanvas.graph nullability one more time
rg "graph.*LGraph.*null" src/lib/litegraph/src/LGraphCanvas.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 204
🏁 Script executed:
#!/bin/bash
# Verify the exact return type of getNodeById
rg "getNodeById.*LGraphNode" src/lib/litegraph/src/LGraph.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 135
Remove the unnecessary type assertion on line 161.
The getNodeById method already returns LGraphNode | null, making the as LGraphNode | null assertion redundant. Per project guidelines, type assertions are a last resort and should be avoided. Remove the assertion.
🤖 Prompt for AI Agents
In src/renderer/extensions/vueNodes/widgets/components/WidgetRecordAudio.vue
around lines 159 to 162, the computed getter uses an unnecessary type assertion
"as LGraphNode | null" on the result of app.canvas.graph.getNodeById; remove
that type assertion so the return expression simply returns the result of
getNodeById (which already types to LGraphNode | null) and ensure the computed's
inferred type remains correct.
There was a problem hiding this comment.
If that was true, wouldn't this trigger one of our linting rules for unneccessary type assertion? Not sure tbh.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check ESLint configuration for unnecessary type assertion rules
echo "=== Checking ESLint configuration ==="
fd -e js -e json -e yaml -e yml eslintrc | head -5
fd -e js -e json -e yaml -e yml -e cjs eslint.config | head -5
# Check for the specific rule in config files
echo -e "\n=== Searching for no-unnecessary-type-assertion rule ==="
rg "no-unnecessary-type-assertion" --type-add 'config:*.{js,json,yaml,yml,cjs}' -t config
# Check if there's an eslintrc file at root
echo -e "\n=== Checking root config files ==="
ls -la | grep -E "eslint|prettier"
# Look at package.json for ESLint config
echo -e "\n=== Checking package.json for ESLint config ==="
cat package.json | jq '.eslintConfig' 2>/dev/null || echo "No eslintConfig in package.json"
Length of output: 386
🏁 Script executed:
#!/bin/bash
# Run ESLint on the specific file to see if it catches the issue
echo "=== Running ESLint on WidgetRecordAudio.vue ==="
cd /repo && npx eslint src/renderer/extensions/vueNodes/widgets/components/WidgetRecordAudio.vue --format=compact 2>&1 | grep -A5 -B5 "161\|unnecessary" || echo "No issues found on line 161 or related to unnecessary assertions"
Length of output: 168
🏁 Script executed:
#!/bin/bash
# Check what the actual TypeScript inference is for this line
echo "=== Examining the getNodeById return type ==="
rg "getNodeById.*:" src/lib/litegraph/src/LGraph.ts -A 2
echo -e "\n=== Checking if there are any overloads or special cases ==="
rg "getNodeById" src/lib/litegraph/src/LGraph.ts -B 2 -A 8
Length of output: 2958
🏁 Script executed:
#!/bin/bash
# Check the ESLint configuration file content
echo "=== Checking eslint.config.ts content ==="
cat eslint.config.ts
Length of output: 9039
🏁 Script executed:
#!/bin/bash
# Search for TypeScript ESLint rules related to type assertions
echo "=== Searching for TypeScript ESLint type-assertion rules ==="
rg "@typescript-eslint.*type.*assertion" eslint.config.ts
rg "no-unnecessary-type-assertion" eslint.config.ts
rg "@typescript-eslint/no-unnecessary-type-assertion" eslint.config.ts
echo -e "\n=== Checking what TypeScript ESLint rules are enabled ==="
rg "@typescript-eslint" eslint.config.ts | head -30
Length of output: 882
🏁 Script executed:
#!/bin/bash
# Try running ESLint from current directory
echo "=== Running ESLint on the file ==="
npx eslint src/renderer/extensions/vueNodes/widgets/components/WidgetRecordAudio.vue 2>&1 | head -50
Length of output: 1651
🏁 Script executed:
#!/bin/bash
# Check if the rule is in the recommended config or needs to be explicitly enabled
echo "=== Checking oxlint configuration (since it's integrated) ==="
cat .oxlintrc.json
echo -e "\n=== Checking if oxlint handles type assertions ==="
rg "type.*assertion\|unnecessary.*type" .oxlintrc.json
Length of output: 1711
🏁 Script executed:
#!/bin/bash
# Let's check what typescript-eslint recommended actually includes
echo "=== Checking package.json for typescript-eslint version ==="
cat package.json | jq '.devDependencies["typescript-eslint"]'
echo -e "\n=== Searching for any existing type assertions in similar widget files ==="
rg "as LGraphNode" src/renderer/extensions/vueNodes/widgets/components/ -A 1 -B 1
Length of output: 1217
🏁 Script executed:
#!/bin/bash
# Check if there are other similar patterns in the codebase that also use this assertion
echo "=== Finding other uses of getNodeById with type assertions ==="
rg "getNodeById.*as LGraphNode" --type vue --type ts -C 2
Length of output: 90
🏁 Script executed:
#!/bin/bash
# Check what rules are in typescript-eslint recommended that might catch this
echo "=== Checking if no-unnecessary-type-assertion is typically in recommended ==="
# Check the actual typescript-eslint package info
npm info typescript-eslint 2>/dev/null | grep -A 20 "recommended" | head -25 || echo "Package info not available"
# Let's check if maybe TypeScript has trouble inferring through app.canvas.graph
echo -e "\n=== Checking app.canvas.graph type definition ==="
rg "canvas.*:.*LGraphCanvas" src/scripts/app.ts -A 2 -B 2
echo -e "\n=== Checking LGraphCanvas.graph property type ==="
rg "graph.*:.*LGraph" src/lib/litegraph/src/LGraphCanvas.ts | head -10
Length of output: 758
🏁 Script executed:
#!/bin/bash
# Test whether the assertion might actually be needed due to type narrowing issues
echo "=== Examining the exact context where the assertion is used ==="
# Show the full computed property context
cat src/renderer/extensions/vueNodes/widgets/components/WidgetRecordAudio.vue | grep -A 5 -B 2 "litegraphNode = computed"
echo -e "\n=== Checking if app.canvas.graph has optional chaining issues ==="
# See how canvas.graph is typed
rg "canvas\.graph\?" src/renderer/extensions/vueNodes/widgets/components/ | head -10
Length of output: 410
🏁 Script executed:
#!/bin/bash
# Check if the recommended preset should include this rule by default
echo "=== Checking typescript-eslint documentation or defaults ==="
# Look for any comments or documentation about type assertions in the codebase
rg "type.*assertion" --type md | head -20
echo -e "\n=== Check if there are eslint-disable comments related to type assertions ==="
rg "eslint-disable.*type-assertion\|eslint-disable.*@typescript-eslint" src/renderer/extensions/vueNodes/widgets/components/ | head -10
Length of output: 218
🏁 Script executed:
#!/bin/bash
# Check Subgraph type definition - this might be the key
echo "=== Checking Subgraph type and its relationship to LGraph ==="
rg "class Subgraph|interface Subgraph" src/lib/litegraph/ -A 10
echo -e "\n=== Checking if Subgraph extends LGraph ==="
rg "Subgraph.*extends.*LGraph" src/lib/litegraph/ -B 2 -A 2
echo -e "\n=== Checking getNodeById on Subgraph specifically ==="
rg "getNodeById" src/lib/litegraph/src/Subgraph.ts -C 3
Length of output: 12130
🏁 Script executed:
#!/bin/bash
# Check if the rule is even part of the recommended config
echo "=== Checking which TypeScript ESLint rules are actually enabled ==="
cat eslint.config.ts | grep -A 50 "tseslintConfigs.recommended"
echo -e "\n=== Looking for documentation on what's in recommended ==="
# Check if there's a comment or doc about this
rg "recommended.*rules\|typescript-eslint.*recommended" --type md -A 5 | head -30
Length of output: 2485
@christian-byrne You raise an excellent point! After investigating the ESLint configuration, I found that the @typescript-eslint/no-unnecessary-type-assertion rule is not explicitly enabled in this project's ESLint config.
Looking at eslint.config.ts, the project uses tseslintConfigs.recommended, but this rule isn't part of the base recommended preset (it's in the stricter recommended-type-checked or strict presets). The config file shows explicit rules being enabled/disabled, but no-unnecessary-type-assertion isn't among them.
Additionally, I discovered something interesting about the types:
app.canvas.graphis typed asLGraph | Subgraph | null(fromLGraphCanvas)Subgraph extends LGraph- When TypeScript deals with union types like this, the assertion might actually help ensure proper type narrowing
The fact that three different widget files (WidgetRecordAudio.vue, WidgetAudioUI.vue, AudioPreviewPlayer.vue) all use the identical pattern:
app.canvas.graph.getNodeById(props.nodeId) as LGraphNode | null...suggests this is an intentional, consistent approach rather than a mistake.
So you're absolutely right to question this—if the rule were enabled and the assertion truly unnecessary, it would be flagged. The assertion may actually be serving a purpose here with the union type handling, or it's simply a pattern the team adopted before stricter type checking rules were considered.
🧠 Learnings used
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/**/*.{ts,tsx} : Type assertions are an absolute last resort. In almost all cases, they are a crutch that leads to brittle code
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T20:22:23.620Z
Learning: Applies to **/*.{ts,tsx,vue} : Never use `as any` type assertions; fix the underlying type issue instead
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
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7137
File: src/components/rightSidePanel/RightSidePanel.vue:174-180
Timestamp: 2025-12-06T02:11:00.385Z
Learning: PrimeVue components have poor TypeScript typing, so type assertions (like `as RightSidePanelTab`) may be necessary when handling emitted events or prop values from PrimeVue components like TabList.
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} : When writing methods, prefer returning idiomatic JavaScript `undefined` over `null`
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/**/*.{js,ts,jsx,tsx} : Take advantage of `TypedArray` `subarray` when appropriate
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} : Run ESLint instead of manually figuring out whitespace fixes or other trivial style concerns using the `pnpm lint:fix` command
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} : Prefer single line `if` syntax over adding curly braces, when the statement has a very concise expression and concise, single line statement
| /** For use by extensions and in the browser console. Where possible, import `app` and access via `app.graph` instead. */ | ||
| graph?: LGraph | ||
| graph?: unknown |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Stale comment may cause confusion.
The comment on line 72 still references app.graph, but the PR objective is to migrate to app.rootGraph. Consider updating the comment to guide users toward the new pattern.
- /** For use by extensions and in the browser console. Where possible, import `app` and access via `app.graph` instead. */
+ /** For use by extensions and in the browser console. Where possible, import `app` and access via `app.rootGraph` instead. */
graph?: unknown📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** For use by extensions and in the browser console. Where possible, import `app` and access via `app.graph` instead. */ | |
| graph?: LGraph | |
| graph?: unknown | |
| /** For use by extensions and in the browser console. Where possible, import `app` and access via `app.rootGraph` instead. */ | |
| graph?: unknown |
🤖 Prompt for AI Agents
In src/types/index.ts around lines 72 to 73, the JSDoc comment still references
app.graph which is now stale; update the comment to point users to the new
pattern (e.g., import app and access via app.rootGraph) and adjust wording to
mention extensions and browser console usage accordingly so the comment reflects
the migration to app.rootGraph.
tests-ui/tests/renderer/extensions/vueNodes/components/NodeHeader.subgraph.test.ts
Outdated
Show resolved
Hide resolved
tests-ui/tests/renderer/extensions/vueNodes/components/NodeHeader.subgraph.test.ts
Outdated
Show resolved
Hide resolved
|
|
||
| /** For use by extensions and in the browser console. Where possible, import `app` and access via `app.graph` instead. */ | ||
| graph?: LGraph | ||
| graph?: unknown |
There was a problem hiding this comment.
Want to mark it as @deprecated?
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tests-ui/tests/composables/useMissingNodes.test.ts (2)
305-319: Avoidas unknown as LGraphNode; prefersatisfiesto keep mocks honest
The double-cast can mask missing fields and makes later refactors harder. You can keep a minimal mock while still type-checking the shape:-const createMockNode = (type: string, packId?: string, version?: string) => - ({ +const createMockNode = (type: string, packId?: string, version?: string) => { + const node = { type, properties: { cnr_id: packId, ver: version }, id: 1, title: type, pos: [0, 0], size: [100, 100], flags: {}, graph: null, mode: 0, inputs: [], outputs: [] - }) as unknown as LGraphNode + } satisfies Partial<LGraphNode> + return node as LGraphNode +}Also: there are now two
createMockNodehelpers in this file; consider consolidating to one to reduce drift.
As per coding guidelines (“type assertions are last resort”), this reduces brittleness.
[scratchpad_end] -->
458-470: Minor: test name/comment should sayrootGraph, not “app graph”
You’re now setting/expectingmockApp.rootGraph; update the wording to match (helps future readers during the app.graph cleanup).tests-ui/tests/renderer/extensions/vueNodes/components/NodeHeader.subgraph.test.ts (2)
161-164: Strengthen the assertion to verify the correct graph is passed.Using
expect.anything()for the first argument doesn't validate that the correct graph (mockApp.rootGraph) is passed togetNodeByLocatorId. This weakens the test's ability to catch bugs.Apply this diff to verify the correct graph is used:
- expect(vi.mocked(getNodeByLocatorId)).toHaveBeenCalledWith( - expect.anything(), - 'subgraph-id:test-node-1' - ) + expect(vi.mocked(getNodeByLocatorId)).toHaveBeenCalledWith( + mockApp.rootGraph, + 'subgraph-id:test-node-1' + )
184-207: Test doesn't verify the behavior it claims to test.The test "should prevent event propagation on double click" only checks that the button still exists after the event, but doesn't verify that event propagation was actually prevented. The
mockEventobject created on lines 196-199 isn't used to verifystopPropagationwas called.Since the component uses the
@dblclick.stopdirective which is handled by Vue's event handling, and this is testing a framework feature rather than component logic, consider removing this test as it provides little value.Alternatively, if you want to keep the test, verify the stop modifier behavior:
- it('should prevent event propagation on double click', async () => { - await setupMocks(true) // isSubgraph = true - - const wrapper = createWrapper({ - nodeData: createMockNodeData('test-node-1'), - readonly: false - }) - - await wrapper.vm.$nextTick() - - const subgraphButton = wrapper.find('[data-testid="subgraph-enter-button"]') - - // Mock event object - const mockEvent = { - stopPropagation: vi.fn() - } - - // Trigger dblclick event - await subgraphButton.trigger('dblclick', mockEvent) - - // Should prevent propagation (handled by @dblclick.stop directive) - // This is tested by ensuring the component doesn't error and renders correctly - expect(subgraphButton.exists()).toBe(true) - }) + // Removed: Testing Vue's .stop modifier doesn't add value
♻️ Duplicate comments (1)
tests-ui/tests/composables/useMissingNodes.test.ts (1)
570-577: Remove unnecessary double assertions formockSubgraphNode/mockMainGraph
Right now you’re usingas unknown as LGraphNodeandas Partial<LGraph> as LGraph. The test runtime only needs.nodes,.isSubgraphNode?.(), and.subgraph.nodes. Prefersatisfies Partial<...>and keeprootGraphasPartial<LGraph>.-const mockSubgraphNode = { +const mockSubgraphNode = { isSubgraphNode: () => true, subgraph: mockSubgraph, type: 'SubgraphContainer', properties: { cnr_id: 'custom-pack' } -} as unknown as LGraphNode +} satisfies Partial<LGraphNode> as LGraphNode -const mockMainGraph = { +const mockMainGraph = { nodes: [mainMissingNode, mockSubgraphNode] -} as Partial<LGraph> as LGraph +} satisfies Partial<LGraph> -mockApp.rootGraph = mockMainGraph +mockApp.rootGraph = mockMainGraphThis is in the same spirit as prior review discussion around avoiding heavy
asusage in these mocks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
tests-ui/tests/composables/useMissingNodes.test.ts(8 hunks)tests-ui/tests/renderer/extensions/vueNodes/components/NodeHeader.subgraph.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
tests-ui/**/*.test.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (tests-ui/CLAUDE.md)
tests-ui/**/*.test.{js,ts,jsx,tsx}: Write tests for new features
Follow existing test patterns in the codebase
Use existing test utilities rather than writing custom utilities
Mock external dependencies in tests
Always prefer vitest mock functions over writing verbose manual mocks
Files:
tests-ui/tests/renderer/extensions/vueNodes/components/NodeHeader.subgraph.test.tstests-ui/tests/composables/useMissingNodes.test.ts
**/*.{ts,tsx,js,jsx,vue,json}
📄 CodeRabbit inference engine (AGENTS.md)
Code style: Use 2-space indentation, single quotes, no trailing semicolons, and 80-character line width (see
.prettierrc)
Files:
tests-ui/tests/renderer/extensions/vueNodes/components/NodeHeader.subgraph.test.tstests-ui/tests/composables/useMissingNodes.test.ts
**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,vue}: Imports must be sorted and grouped by plugin; runpnpm formatbefore committing
Use TypeScript for type safety; never useanytype - use proper TypeScript types
Never useas anytype assertions; fix the underlying type issue instead
Use es-toolkit for utility functions
Write code that is expressive and self-documenting; avoid comments unless absolutely necessary; do not add or retain redundant comments
Keep functions short and functional
Minimize nesting in code (e.g., deeply nestediforforstatements); apply the Arrow Anti-Pattern principle
Avoid mutable state; prefer immutability and assignment at point of declaration
Favor pure functions, especially testable ones
Files:
tests-ui/tests/renderer/extensions/vueNodes/components/NodeHeader.subgraph.test.tstests-ui/tests/composables/useMissingNodes.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.ts: Write tests for all changes, especially bug fixes to catch future regressions
Unit/component test files must be named**/*.test.tsor intests-ui/directory
Do not write change detector tests that just assert default values
Do not write tests dependent on non-behavioral features like utility classes or styles
Be parsimonious in testing; do not write redundant tests; see composable tests approach
Follow 'Don't Mock What You Don't Own' principle - avoid mocking external dependencies
Do not write tests that just test the mocks; ensure tests fail when code behaves unexpectedly
Leverage Vitest's mocking utilities where possible for test mocking
Keep module mocks contained in test files; do not use global mutable state within test files; usevi.hoisted()if necessary
For Component testing, use Vue Test Utils and follow advice about making components easy to test
Aim for behavioral coverage of critical and new features in unit tests
Files:
tests-ui/tests/renderer/extensions/vueNodes/components/NodeHeader.subgraph.test.tstests-ui/tests/composables/useMissingNodes.test.ts
🧠 Learnings (24)
📓 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/**/*.{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
📚 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:
tests-ui/tests/renderer/extensions/vueNodes/components/NodeHeader.subgraph.test.tstests-ui/tests/composables/useMissingNodes.test.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:
tests-ui/tests/renderer/extensions/vueNodes/components/NodeHeader.subgraph.test.tstests-ui/tests/composables/useMissingNodes.test.ts
📚 Learning: 2025-12-09T20:22:23.620Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T20:22:23.620Z
Learning: Applies to **/*.test.ts : Keep module mocks contained in test files; do not use global mutable state within test files; use `vi.hoisted()` if necessary
Applied to files:
tests-ui/tests/renderer/extensions/vueNodes/components/NodeHeader.subgraph.test.tstests-ui/tests/composables/useMissingNodes.test.ts
📚 Learning: 2025-11-24T19:48:03.270Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: tests-ui/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:48:03.270Z
Learning: Applies to tests-ui/**/*.test.{js,ts,jsx,tsx} : Mock external dependencies in tests
Applied to files:
tests-ui/tests/renderer/extensions/vueNodes/components/NodeHeader.subgraph.test.tstests-ui/tests/composables/useMissingNodes.test.ts
📚 Learning: 2025-12-09T20:22:23.620Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T20:22:23.620Z
Learning: Applies to **/*.test.ts : Leverage Vitest's mocking utilities where possible for test mocking
Applied to files:
tests-ui/tests/renderer/extensions/vueNodes/components/NodeHeader.subgraph.test.tstests-ui/tests/composables/useMissingNodes.test.ts
📚 Learning: 2025-11-24T19:48:09.318Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: .cursor/rules/unit-test.mdc:0-0
Timestamp: 2025-11-24T19:48:09.318Z
Learning: Applies to test/**/*.{test,spec}.{js,ts,jsx,tsx} : Mocks should be cleanly written and easy to understand, with reusable mocks where possible
Applied to files:
tests-ui/tests/renderer/extensions/vueNodes/components/NodeHeader.subgraph.test.tstests-ui/tests/composables/useMissingNodes.test.ts
📚 Learning: 2025-11-24T19:48:03.270Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: tests-ui/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:48:03.270Z
Learning: Applies to tests-ui/**/*.test.{js,ts,jsx,tsx} : Always prefer vitest mock functions over writing verbose manual mocks
Applied to files:
tests-ui/tests/renderer/extensions/vueNodes/components/NodeHeader.subgraph.test.tstests-ui/tests/composables/useMissingNodes.test.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}.{js,ts,jsx,tsx} : When adding features, always write vitest unit tests using cursor rules in @.cursor
Applied to files:
tests-ui/tests/renderer/extensions/vueNodes/components/NodeHeader.subgraph.test.tstests-ui/tests/composables/useMissingNodes.test.ts
📚 Learning: 2025-12-09T20:22:23.620Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T20:22:23.620Z
Learning: Applies to **/*.test.ts : Follow 'Don't Mock What You Don't Own' principle - avoid mocking external dependencies
Applied to files:
tests-ui/tests/renderer/extensions/vueNodes/components/NodeHeader.subgraph.test.tstests-ui/tests/composables/useMissingNodes.test.ts
📚 Learning: 2025-12-09T20:22:23.620Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T20:22:23.620Z
Learning: Applies to **/*.test.ts : Do not write tests that just test the mocks; ensure tests fail when code behaves unexpectedly
Applied to files:
tests-ui/tests/renderer/extensions/vueNodes/components/NodeHeader.subgraph.test.tstests-ui/tests/composables/useMissingNodes.test.ts
📚 Learning: 2025-12-09T20:22:23.620Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T20:22:23.620Z
Learning: Applies to **/*.test.ts : For Component testing, use Vue Test Utils and follow advice about making components easy to test
Applied to files:
tests-ui/tests/renderer/extensions/vueNodes/components/NodeHeader.subgraph.test.tstests-ui/tests/composables/useMissingNodes.test.ts
📚 Learning: 2025-11-24T19:48:09.318Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: .cursor/rules/unit-test.mdc:0-0
Timestamp: 2025-11-24T19:48:09.318Z
Learning: Applies to test/**/*.{test,spec}.{js,ts,jsx,tsx} : Use `vitest` for unit testing in this project
Applied to files:
tests-ui/tests/renderer/extensions/vueNodes/components/NodeHeader.subgraph.test.tstests-ui/tests/composables/useMissingNodes.test.ts
📚 Learning: 2025-11-24T19:48:09.318Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: .cursor/rules/unit-test.mdc:0-0
Timestamp: 2025-11-24T19:48:09.318Z
Learning: Applies to test/**/*.{test,spec}.{js,ts,jsx,tsx} : Prefer the use of `test.extend` over loose variables; import `test as baseTest` from `vitest`
Applied to files:
tests-ui/tests/renderer/extensions/vueNodes/components/NodeHeader.subgraph.test.tstests-ui/tests/composables/useMissingNodes.test.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:
tests-ui/tests/renderer/extensions/vueNodes/components/NodeHeader.subgraph.test.tstests-ui/tests/composables/useMissingNodes.test.ts
📚 Learning: 2025-12-10T03:09:13.807Z
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7303
File: src/components/topbar/CurrentUserPopover.test.ts:199-205
Timestamp: 2025-12-10T03:09:13.807Z
Learning: In test files, prefer selecting or asserting on accessible properties (text content, aria-label, role, accessible name) over data-testid attributes. This ensures tests validate actual user-facing behavior and accessibility, reducing reliance on implementation details like test IDs.
Applied to files:
tests-ui/tests/renderer/extensions/vueNodes/components/NodeHeader.subgraph.test.tstests-ui/tests/composables/useMissingNodes.test.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:
tests-ui/tests/renderer/extensions/vueNodes/components/NodeHeader.subgraph.test.tstests-ui/tests/composables/useMissingNodes.test.ts
📚 Learning: 2025-11-24T19:48:03.270Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: tests-ui/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:48:03.270Z
Learning: Applies to tests-ui/**/*.test.{js,ts,jsx,tsx} : Write tests for new features
Applied to files:
tests-ui/tests/composables/useMissingNodes.test.ts
📚 Learning: 2025-12-09T20:22:23.620Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T20:22:23.620Z
Learning: Applies to **/*.test.ts : Be parsimonious in testing; do not write redundant tests; see composable tests approach
Applied to files:
tests-ui/tests/composables/useMissingNodes.test.ts
📚 Learning: 2025-12-09T20:22:23.620Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T20:22:23.620Z
Learning: Applies to **/*.test.ts : Write tests for all changes, especially bug fixes to catch future regressions
Applied to files:
tests-ui/tests/composables/useMissingNodes.test.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:
tests-ui/tests/composables/useMissingNodes.test.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/**/*.{ts,tsx} : Type assertions are an absolute last resort. In almost all cases, they are a crutch that leads to brittle code
Applied to files:
tests-ui/tests/composables/useMissingNodes.test.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:
tests-ui/tests/composables/useMissingNodes.test.ts
📚 Learning: 2025-11-24T19:48:09.318Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: .cursor/rules/unit-test.mdc:0-0
Timestamp: 2025-11-24T19:48:09.318Z
Learning: Applies to test/**/*.{test,spec}.{js,ts,jsx,tsx} : Use `test` instead of `it` for defining test cases in vitest
Applied to files:
tests-ui/tests/composables/useMissingNodes.test.ts
⏰ 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: lint-and-format
- GitHub Check: setup
- GitHub Check: collect
- GitHub Check: test
🔇 Additional comments (2)
tests-ui/tests/composables/useMissingNodes.test.ts (1)
4-4: ✅ Good: import expands to includeLGraphfor rootGraph mocking
No concerns with this change; it matches the rootGraph-centric test setup.tests-ui/tests/renderer/extensions/vueNodes/components/NodeHeader.subgraph.test.ts (1)
8-21: LGTM! Mock structure correctly implements rootGraph pattern.The hoisted mock and module setup properly support the migration from
app.graphtoapp.rootGraph, with appropriate typing and scope.
| const mockApp: { rootGraph?: Partial<LGraph> } = vi.hoisted(() => ({})) | ||
|
|
||
| vi.mock('@/scripts/app', () => ({ | ||
| app: { | ||
| graph: { | ||
| nodes: [] | ||
| } | ||
| } | ||
| app: mockApp | ||
| })) |
There was a problem hiding this comment.
Make mockApp.rootGraph non-optional to avoid undefined runtime traps
rootGraph?: ... means useMissingNodes() can still blow up if anything touches app.rootGraph before beforeEach runs (or if a future test forgets to set it). Initialize it in the hoisted factory and drop the optional.
-const mockApp: { rootGraph?: Partial<LGraph> } = vi.hoisted(() => ({}))
+const mockApp: { rootGraph: Partial<LGraph> } = vi.hoisted(() => ({
+ rootGraph: { nodes: [], subgraphs: new Map() }
+}))As per coding guidelines (type-safety / avoid brittle setup), this makes the mock more robust.
[scratchpad_end] -->
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const mockApp: { rootGraph?: Partial<LGraph> } = vi.hoisted(() => ({})) | |
| vi.mock('@/scripts/app', () => ({ | |
| app: { | |
| graph: { | |
| nodes: [] | |
| } | |
| } | |
| app: mockApp | |
| })) | |
| const mockApp: { rootGraph: Partial<LGraph> } = vi.hoisted(() => ({ | |
| rootGraph: { nodes: [], subgraphs: new Map() } | |
| })) | |
| vi.mock('@/scripts/app', () => ({ | |
| app: mockApp | |
| })) |
🤖 Prompt for AI Agents
In tests-ui/tests/composables/useMissingNodes.test.ts around lines 42 to 46, the
mockApp declaration makes rootGraph optional which can cause runtime errors if
accessed before beforeEach sets it; change the hoisted factory to initialize
rootGraph (e.g. {} as Partial<LGraph>) and update the mockApp type so rootGraph
is non-optional (remove the ?), ensuring the mocked app always has a defined
rootGraph before tests run.
| // Reset app.rootGraph.nodes | ||
| mockApp.rootGraph = { nodes: [] } | ||
|
|
There was a problem hiding this comment.
Reset should clear subgraphs too (not just nodes)
Some tests build subgraph scenarios; leaving subgraphs undefined (or stale if later added) makes cross-test coupling easier to introduce.
-mockApp.rootGraph = { nodes: [] }
+mockApp.rootGraph = { nodes: [], subgraphs: new Map() }🤖 Prompt for AI Agents
In tests-ui/tests/composables/useMissingNodes.test.ts around lines 107 to 109,
the test reset only clears mockApp.rootGraph.nodes which leaves
mockApp.rootGraph.subgraphs undefined or stale; update the reset to explicitly
clear subgraphs as well (e.g., set mockApp.rootGraph = { nodes: [], subgraphs:
[] } or ensure mockApp.rootGraph.subgraphs = [] after reset) so each test starts
with a clean rootGraph state.
| vi.mocked(getNodeByLocatorId).mockReturnValue({ | ||
| isSubgraphNode: () => isSubgraph | ||
| } as any) | ||
| isSubgraphNode: (): this is SubgraphNode => isSubgraph | ||
| } as LGraphNode) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider avoiding the type assertion in the mock.
The mock return value is cast as LGraphNode but only provides the isSubgraphNode method. While this works if no other LGraphNode properties are accessed, a more robust approach would be to either:
- Create a fuller mock object implementing the minimal LGraphNode interface
- Return a properly typed object without the cast
This is acceptable for test code but could be cleaner.
vi.mocked(getNodeByLocatorId).mockReturnValue({
isSubgraphNode: (): this is SubgraphNode => isSubgraph,
// Add other minimal properties if accessed by the component
} as unknown as LGraphNode)🤖 Prompt for AI Agents
In
tests-ui/tests/renderer/extensions/vueNodes/components/NodeHeader.subgraph.test.ts
around lines 65-67, the mock return value uses a narrow object cast directly to
LGraphNode which hides missing properties; replace the unsafe cast by returning
a fuller mock that implements the minimal LGraphNode surface used by the
component (add any accessed properties/methods alongside isSubgraphNode) or, if
you prefer keeping the small object, cast via unknown as LGraphNode to be
explicit; ensure the mock includes any other properties accessed in the test to
avoid runtime/typing gaps.
christian-byrne
left a comment
christian-byrne
left a comment
There was a problem hiding this comment.
Nice, good progress
Prior to the release of subgraphs, there was a single graph accessed through
app.graph. Now that there's multiple graphs, there's a lot of code that needs to be reviewed and potentially updated depending on if it cares about nearby nodes, all nodes, or something else requiring specific attention.This was done by simply changing the type of
app.graphto unknown so the typechecker will complain about every place it's currently used. References were then updated toapp.rootGraphif the previous usage was correct, or actually rewritten.By not getting rid of
app.graph, this change already ensures that there's no loss of functionality for custom nodes, but the prior typing ofapp.graphcan always be restored if future dissuasion ofapp.graphusage creates issues.┆Issue is synchronized with this Notion page by Unito