Skip to content

Chore: Typescript cleanup (1 / N) #7817

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
DrJKL merged 11 commits into from
Jan 6, 2026
Merged

Chore: Typescript cleanup (1 / N) #7817

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2ef9999
fix: remove 41 unused parameter @ts-expect-error suppressions in LGra…
DrJKL Dec 31, 2025
6542975
fix: remove 15 @ts-expect-error suppressions in LGraphCanvas.ts
DrJKL Dec 31, 2025
70adacc
chore: remove unused parameter/variable @ts-expect-error suppressions
DrJKL Dec 31, 2025
087c298
fix(litegraph): remove 31 @ts-expect-error suppressions
DrJKL Dec 31, 2025
071213b
fix: Restore casting through unknown for the contextmenu mocks. These…
DrJKL Dec 31, 2025
c192c7b
fix(types): Fix typing issue in groupNode
DrJKL Dec 31, 2025
31307fa
[automated] Apply ESLint and Prettier fixes
actions-user Dec 31, 2025
f8c8e4b
fix: remove 13 @ts-expect-error suppressions in litegraph tests
DrJKL Dec 31, 2025
f92f832
fix: remove 16 @ts-expect-error suppressions via typed event capture
DrJKL Dec 31, 2025
a41cbab
fix(litegraph): remove 28 'Fix after merge' @ts-expect-error suppress…
DrJKL Dec 31, 2025
34fa21c
fix: restore canvas parameter to onMouseUp callback
DrJKL Jan 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions src/extensions/core/groupNode.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,8 +196,7 @@ export class GroupNodeConfig {
primitiveToWidget: {}
nodeInputs: {}
outputVisibility: any[]
// @ts-expect-error fixme ts strict error
nodeDef: ComfyNodeDef
nodeDef: (ComfyNodeDef & { [GROUP]: GroupNodeConfig }) | undefined
// @ts-expect-error fixme ts strict error
inputs: any[]
// @ts-expect-error fixme ts strict error
Expand Down Expand Up @@ -231,8 +230,7 @@ export class GroupNodeConfig {
output: [],
output_name: [],
output_is_list: [],
// @ts-expect-error Unused, doesn't exist
output_is_hidden: [],
output_node: false, // This is a lie (to satisfy the interface)
name: source + SEPARATOR + this.name,
display_name: this.name,
category: 'group nodes' + (SEPARATOR + source),
Expand Down Expand Up @@ -261,6 +259,7 @@ export class GroupNodeConfig {
}
// @ts-expect-error fixme ts strict error
this.#convertedToProcess = null
if (!this.nodeDef) return
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Silent failure: early return without notification.

The early return when nodeDef is undefined causes registerType to fail silently. This could make debugging difficult if something goes wrong during node definition construction.

🔎 Consider adding error logging or throwing an error 
 if (!this.nodeDef) return
+if (!this.nodeDef) {
+  console.error(`Failed to register group node type: nodeDef is undefined for ${this.name}`)
+  return
+}

Committable suggestion skipped: line range outside the PR's diff.

await app.registerNodeDef(`${PREFIX}${SEPARATOR}` + this.name, this.nodeDef)
useNodeDefStore().addNodeDef(this.nodeDef)
}
Expand Down
5 changes: 1 addition & 4 deletions src/extensions/core/load3d/CameraManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ export class CameraManager implements CameraManagerInterface {
orthographicCamera: THREE.OrthographicCamera
activeCamera: THREE.Camera

// @ts-expect-error unused variable
private renderer: THREE.WebGLRenderer
private eventManager: EventManagerInterface

private controls: OrbitControls | null = null
Expand Down Expand Up @@ -42,10 +40,9 @@ export class CameraManager implements CameraManagerInterface {
}

constructor(
renderer: THREE.WebGLRenderer,
_renderer: THREE.WebGLRenderer,
eventManager: EventManagerInterface
) {
this.renderer = renderer
this.eventManager = eventManager

this.perspectiveCamera = new THREE.PerspectiveCamera(
Expand Down
5 changes: 1 addition & 4 deletions src/extensions/core/load3d/SceneManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,21 +27,18 @@ export class SceneManager implements SceneManagerInterface {
private renderer: THREE.WebGLRenderer

private getActiveCamera: () => THREE.Camera
// @ts-expect-error unused variable
private getControls: () => OrbitControls

constructor(
renderer: THREE.WebGLRenderer,
getActiveCamera: () => THREE.Camera,
getControls: () => OrbitControls,
_getControls: () => OrbitControls,
eventManager: EventManagerInterface
) {
this.renderer = renderer
this.eventManager = eventManager
this.scene = new THREE.Scene()

this.getActiveCamera = getActiveCamera
Comment on lines 31 to 41
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

rg -n -C3 'new\s+SceneManager\s*\(' --type=ts

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 531

🏁 Script executed:

# Get full context of SceneManager constructor call in Load3d.ts
rg -n -A 5 'this.sceneManager = new SceneManager' src/extensions/core/load3d/Load3d.ts

# Check if _getControls is referenced anywhere in SceneManager
rg -n '_getControls\|getControls\|controls' src/extensions/core/load3d/SceneManager.ts

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 254

🏁 Script executed:

# Check entire SceneManager.ts file structure
cat src/extensions/core/load3d/SceneManager.ts

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 13165

Remove the unused _getControls parameter or document its intended use.

The _getControls parameter is passed by the caller (Load3d.ts) but never stored, assigned, or used anywhere in the SceneManager class. Unlike renderer and getActiveCamera which serve active purposes, this parameter adds unnecessary surface area to the module. Either remove it from both the constructor signature and the caller in Load3d.ts, or document why it needs to be accepted if there's a planned use case.

🤖 Prompt for AI Agents 
In @src/extensions/core/load3d/SceneManager.ts around lines 31 - 41, The
constructor for SceneManager currently accepts an unused parameter _getControls;
remove this unused parameter from the SceneManager constructor signature and
from every call site (notably where Load3d constructs SceneManager) and clean up
any related type annotations, or if you intend to keep it, store it as a private
property (e.g., private getControls: () => OrbitControls) and add a short JSDoc
comment on the constructor explaining its planned purpose so it’s not treated as
dead code; update any interfaces or imports referencing the old signature
accordingly (look for the SceneManager constructor and the Load3d.ts call that
passes _getControls).

this.getControls = getControls

this.gridHelper = new THREE.GridHelper(20, 20)
this.gridHelper.position.set(0, 0, 0)
Expand Down
5 changes: 1 addition & 4 deletions src/extensions/core/load3d/ViewHelperManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,13 @@ export class ViewHelperManager implements ViewHelperManagerInterface {
private getActiveCamera: () => THREE.Camera
private getControls: () => OrbitControls
private eventManager: EventManagerInterface
// @ts-expect-error unused variable
private renderer: THREE.WebGLRenderer

constructor(
renderer: THREE.WebGLRenderer,
_renderer: THREE.WebGLRenderer,
getActiveCamera: () => THREE.Camera,
getControls: () => OrbitControls,
eventManager: EventManagerInterface
) {
this.renderer = renderer
this.getActiveCamera = getActiveCamera
this.getControls = getControls
this.eventManager = eventManager
Expand Down
11 changes: 4 additions & 7 deletions src/lib/litegraph/src/LGraphButton.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,14 @@ import { LGraphButton, Rectangle } from '@/lib/litegraph/src/litegraph'
describe('LGraphButton', () => {
describe('Constructor', () => {
it('should create a button with default options', () => {
// @ts-expect-error TODO: Fix after merge - LGraphButton constructor type issues
const button = new LGraphButton({})
const button = new LGraphButton({ text: '' })
expect(button).toBeInstanceOf(LGraphButton)
expect(button.name).toBeUndefined()
expect(button._last_area).toBeInstanceOf(Rectangle)
})

it('should create a button with custom name', () => {
// @ts-expect-error TODO: Fix after merge - LGraphButton constructor type issues
const button = new LGraphButton({ name: 'test_button' })
const button = new LGraphButton({ text: '', name: 'test_button' })
expect(button.name).toBe('test_button')
})

Expand Down Expand Up @@ -158,9 +156,8 @@ describe('LGraphButton', () => {
const button = new LGraphButton({
text: '→',
fontSize: 20,
// @ts-expect-error TODO: Fix after merge - color property not defined in type
color: '#FFFFFF',
backgroundColor: '#333333',
fgColor: '#FFFFFF',
bgColor: '#333333',
xOffset: -10,
yOffset: 5
})
Expand Down
65 changes: 24 additions & 41 deletions src/lib/litegraph/src/LGraphCanvas.titleButtons.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { beforeEach, describe, expect, it, vi } from 'vitest'

import {
LGraph,
LGraphCanvas,
LGraphNode,
LiteGraph
Expand Down Expand Up @@ -46,8 +47,8 @@ describe('LGraphCanvas Title Button Rendering', () => {

canvasElement.getContext = vi.fn().mockReturnValue(ctx)

// @ts-expect-error TODO: Fix after merge - LGraphCanvas constructor type issues
canvas = new LGraphCanvas(canvasElement, null, {
const graph = new LGraph()
canvas = new LGraphCanvas(canvasElement, graph, {
skip_render: true,
skip_events: true
})
Expand All @@ -56,43 +57,41 @@ describe('LGraphCanvas Title Button Rendering', () => {
node.pos = [100, 200]
node.size = [200, 100]

// Mock required methods
node.drawTitleBarBackground = vi.fn()
// @ts-expect-error Property 'drawTitleBarText' does not exist on type 'LGraphNode'
node.drawTitleBarText = vi.fn()
node.drawBadges = vi.fn()
// @ts-expect-error TODO: Fix after merge - drawToggles not defined in type
node.drawToggles = vi.fn()
// @ts-expect-error TODO: Fix after merge - drawNodeShape not defined in type
node.drawNodeShape = vi.fn()
node.drawSlots = vi.fn()
// @ts-expect-error TODO: Fix after merge - drawContent not defined in type
node.drawContent = vi.fn()
node.drawWidgets = vi.fn()
node.drawCollapsedSlots = vi.fn()
node.drawTitleBox = vi.fn()
node.drawTitleText = vi.fn()
node.drawProgressBar = vi.fn()
node._setConcreteSlots = vi.fn()
node.arrange = vi.fn()
// @ts-expect-error TODO: Fix after merge - isSelectable not defined in type
node.isSelectable = vi.fn().mockReturnValue(true)

const nodeWithMocks = node as LGraphNode & {
drawTitleBarText: ReturnType<typeof vi.fn>
drawToggles: ReturnType<typeof vi.fn>
drawNodeShape: ReturnType<typeof vi.fn>
drawContent: ReturnType<typeof vi.fn>
isSelectable: ReturnType<typeof vi.fn>
}
nodeWithMocks.drawTitleBarText = vi.fn()
nodeWithMocks.drawToggles = vi.fn()
nodeWithMocks.drawNodeShape = vi.fn()
nodeWithMocks.drawContent = vi.fn()
nodeWithMocks.isSelectable = vi.fn().mockReturnValue(true)
Comment on lines +70 to +82
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

nodeWithMocks is unused after assignment.

The nodeWithMocks variable is created and has mock methods assigned to it, but all subsequent test code continues to use the original node variable. Since nodeWithMocks is just a type-cast reference to the same object, the mock assignments do work, but the explicit variable declaration adds confusion without benefit.

Consider simplifying by assigning mocks directly to node with inline type assertions only where needed, or use nodeWithMocks consistently throughout the tests.

🔎 Suggested simplification 
-    const nodeWithMocks = node as LGraphNode & {
-      drawTitleBarText: ReturnType<typeof vi.fn>
-      drawToggles: ReturnType<typeof vi.fn>
-      drawNodeShape: ReturnType<typeof vi.fn>
-      drawContent: ReturnType<typeof vi.fn>
-      isSelectable: ReturnType<typeof vi.fn>
-    }
-    nodeWithMocks.drawTitleBarText = vi.fn()
-    nodeWithMocks.drawToggles = vi.fn()
-    nodeWithMocks.drawNodeShape = vi.fn()
-    nodeWithMocks.drawContent = vi.fn()
-    nodeWithMocks.isSelectable = vi.fn().mockReturnValue(true)
+    // Assign additional mocks needed by drawNode
+    ;(node as LGraphNode & { drawTitleBarText: unknown }).drawTitleBarText = vi.fn()
+    ;(node as LGraphNode & { drawToggles: unknown }).drawToggles = vi.fn()
+    ;(node as LGraphNode & { drawNodeShape: unknown }).drawNodeShape = vi.fn()
+    ;(node as LGraphNode & { drawContent: unknown }).drawContent = vi.fn()
+    ;(node as LGraphNode & { isSelectable: unknown }).isSelectable = vi.fn().mockReturnValue(true)

Or alternatively, use nodeWithMocks throughout the test file instead of node.

🤖 Prompt for AI Agents 
In @src/lib/litegraph/src/LGraphCanvas.titleButtons.test.ts around lines 70 -
82, The variable nodeWithMocks is created and assigned mock implementations for
drawTitleBarText, drawToggles, drawNodeShape, drawContent and isSelectable but
then never used, which is confusing; either remove nodeWithMocks and attach the
mocks directly to node (using inline type assertions where needed) or replace
subsequent references to node with nodeWithMocks throughout the test so the
mocks are clearly applied — update assignments to target node (or consistently
use nodeWithMocks) and ensure the mock methods (drawTitleBarText, drawToggles,
drawNodeShape, drawContent, isSelectable) are the ones exercised by the tests.

})

describe('drawNode title button rendering', () => {
it('should render visible title buttons', () => {
const button1 = node.addTitleButton({
name: 'button1',
text: 'A',
// @ts-expect-error TODO: Fix after merge - visible property not in LGraphButtonOptions
visible: true
text: 'A'
})

const button2 = node.addTitleButton({
name: 'button2',
text: 'B',
// @ts-expect-error TODO: Fix after merge - visible property not in LGraphButtonOptions
visible: true
text: 'B'
})

// Mock button methods
Expand Down Expand Up @@ -127,9 +126,7 @@ describe('LGraphCanvas Title Button Rendering', () => {
it('should skip invisible title buttons', () => {
const visibleButton = node.addTitleButton({
name: 'visible',
text: 'V',
// @ts-expect-error TODO: Fix after merge - visible property not in LGraphButtonOptions
visible: true
text: 'V'
})

const invisibleButton = node.addTitleButton({
Expand Down Expand Up @@ -171,9 +168,7 @@ describe('LGraphCanvas Title Button Rendering', () => {
for (let i = 0; i < 3; i++) {
const button = node.addTitleButton({
name: `button${i}`,
text: String(i),
// @ts-expect-error TODO: Fix after merge - visible property not in LGraphButtonOptions
visible: true
text: String(i)
})
button.getWidth = vi.fn().mockReturnValue(15) // All same width for simplicity
const spy = vi.spyOn(button, 'draw')
Expand All @@ -196,18 +191,12 @@ describe('LGraphCanvas Title Button Rendering', () => {
it('should render buttons in low quality mode', () => {
const button = node.addTitleButton({
name: 'test',
text: 'T',
// @ts-expect-error TODO: Fix after merge - visible property not in LGraphButtonOptions
visible: true
text: 'T'
})

button.getWidth = vi.fn().mockReturnValue(20)
const drawSpy = vi.spyOn(button, 'draw')

// Set low quality rendering
// @ts-expect-error TODO: Fix after merge - lowQualityRenderingRequired not defined in type
canvas.lowQualityRenderingRequired = true

canvas.drawNode(node, ctx)

// Buttons should still be rendered in low quality mode
Expand All @@ -219,16 +208,12 @@ describe('LGraphCanvas Title Button Rendering', () => {
it('should handle buttons with different widths', () => {
const smallButton = node.addTitleButton({
name: 'small',
text: 'S',
// @ts-expect-error TODO: Fix after merge - visible property not in LGraphButtonOptions
visible: true
text: 'S'
})

const largeButton = node.addTitleButton({
name: 'large',
text: 'LARGE',
// @ts-expect-error TODO: Fix after merge - visible property not in LGraphButtonOptions
visible: true
text: 'LARGE'
})

smallButton.getWidth = vi.fn().mockReturnValue(15)
Expand Down Expand Up @@ -256,9 +241,7 @@ describe('LGraphCanvas Title Button Rendering', () => {

const button = node.addTitleButton({
name: 'test',
text: 'X',
// @ts-expect-error TODO: Fix after merge - visible property not in LGraphButtonOptions
visible: true
text: 'X'
})

button.getWidth = vi.fn().mockReturnValue(20)
Expand Down
Loading