refactor: improve type safety patterns from @ts-expect-error cleanup#7968
refactor: improve type safety patterns from @ts-expect-error cleanup#7968
Conversation
Amp-Thread-ID: https://ampcode.com/threads/T-019ba9df-6ee8-7541-9105-c657cd9ec692 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019baa1c-6aa2-769b-a5f9-a705b5ef2b2b Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019babbe-2ab8-7426-aa86-bba47c1ff997 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019bafa8-5813-717a-89bf-1ba8b4c4faff Co-authored-by: Amp <amp@ampcode.com>
…ervice Amp-Thread-ID: https://ampcode.com/threads/T-019bafa8-5813-717a-89bf-1ba8b4c4faff Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019bafa8-5813-717a-89bf-1ba8b4c4faff Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019bafc7-9674-769c-bfea-dd60aa6c47a7 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019bafc7-9674-769c-bfea-dd60aa6c47a7 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019bb3bd-f607-735a-b1a8-fce5fe4f0125 Co-authored-by: Amp <amp@ampcode.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR systematically removes Changes
Possibly related PRs
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 01/13/2026, 04:21:18 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Tests: ❌ FailedResults: 468 passed, 18 failed, 1 flaky, 8 skipped (Total: 495) ❌ Failed Tests📊 Browser Reports
|
|
@coderabbitai review full |
|
@DrJKL: I'll conduct a full review of this PR. Given the comprehensive nature of this type safety refactoring (180 files, removing 527
Starting the full review now. 🧠 Learnings used✅ Actions performedFull review triggered. |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 3.33 MB (baseline 3.32 MB) • 🔴 +6.89 kBMain entry bundles and manifests
Status: 3 added / 3 removed Graph Workspace — 1.04 MB (baseline 1.04 MB) • 🔴 +1.05 kBGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 6.63 kB (baseline 6.63 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 1 added / 1 removed Panels & Settings — 372 kB (baseline 372 kB) • 🔴 +55 BConfiguration panels, inspectors, and settings screens
Status: 6 added / 6 removed UI Components — 200 kB (baseline 200 kB) • 🔴 +6 BReusable component library chunks
Status: 9 added / 9 removed Data & Services — 12.5 kB (baseline 12.5 kB) • ⚪ 0 BStores, services, APIs, and repositories
Status: 3 added / 3 removed Utilities & Hooks — 1.41 kB (baseline 1.41 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 1 added / 1 removed Vendor & Third-Party — 9.19 MB (baseline 9.19 MB) • 🔴 +874 BExternal libraries and shared vendor chunks
Status: 6 added / 6 removed Other — 5.25 MB (baseline 5.25 MB) • 🔴 +132 BBundles that do not match a named category
Status: 21 added / 21 removed |
github-actions
bot
removed
the
claude-review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 68
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (34)
src/lib/litegraph/src/polyfills.ts (1)
92-93: Bug: Incorrect radius variable used for bottom-left corner.Line 92 uses
bottom_right_radiuswhen drawing the bottom edge to the bottom-left corner. This should bebottom_left_radiusto correctly position the start of the bottom-left curve when corners have different radii.🐛 Proposed fix
// bottom left - this.lineTo(x + bottom_right_radius, y + h) + this.lineTo(x + bottom_left_radius, y + h) this.quadraticCurveTo(x, y + h, x, y + h - bottom_left_radius)src/scripts/ui/imagePreview.ts (1)
22-25: Guard against empty array to prevent runtime error.Accessing
imgs[0]without checking array length will throw ifimgsis empty, asundefined.naturalWidthis a TypeError.🛠️ Suggested fix
export function calculateImageGrid( imgs: ImageLike[], dw: number, dh: number ): { cellWidth: number cellHeight: number cols: number rows: number shiftX: number } { + if (imgs.length === 0) { + return { cellWidth: 0, cellHeight: 0, cols: 0, rows: 0, shiftX: 0 } + } + let best = 0 let w = imgs[0].naturalWidth let h = imgs[0].naturalHeightsrc/composables/maskeditor/useCanvasManager.test.ts (1)
127-145: Consider adding explicit style objects to imgCanvas and rgbCanvas for consistency.While
imgCanvasandrgbCanvascurrently have emptystyle: {}objects (lines 130, 145) andmaskCanvashas explicit style properties, this inconsistency could cause issues if the implementation later accesses style properties on these canvases. Consider initializing them consistently:♻️ Optional: Initialize all canvas style objects consistently
mockStore.imgCanvas = { width: 0, height: 0, - style: {} + style: { + mixBlendMode: '', + opacity: '', + backgroundColor: '' + } } mockStore.maskCanvas = { width: 0, height: 0, style: { mixBlendMode: '', - opacity: '' + opacity: '', + backgroundColor: '' } } mockStore.rgbCanvas = { width: 0, height: 0, - style: {} + style: { + mixBlendMode: '', + opacity: '', + backgroundColor: '' + } }src/scripts/ui/dialog.ts (1)
37-44: Sanitize HTML input to prevent XSS vulnerabilities.The
innerHTMLassignment at line 39 accepts unsanitized string input, creating an XSS vulnerability. Per coding guidelines, HTML content must be sanitized with DOMPurify.🔒 Proposed fix using DOMPurify
Add DOMPurify import at the top of the file:
import DOMPurify from 'dompurify'Then sanitize the HTML string:
show(html?: string | HTMLElement | HTMLElement[]) { if (typeof html === 'string') { - this.textElement.innerHTML = html + this.textElement.innerHTML = DOMPurify.sanitize(html) } else if (html) {src/workbench/extensions/manager/composables/nodePack/useMissingNodes.test.ts (2)
324-337: ConsiderPartial<LGraphNode>cast for clearer intent.Per repository learnings,
as Partial<LGraphNode> as LGraphNodeis preferred overas unknown as LGraphNodewhen creating partial mock objects. This makes the incomplete implementation explicit while maintaining type safety.♻️ Optional: Use Partial pattern
const createMockNode = (type: string, packId?: string, version?: string) => ({ 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 + }) as Partial<LGraphNode> as LGraphNodeVerify whether
Partial<LGraphNode>compiles cleanly given the nestedpropertiesobject structure. If TypeScript complains about required nested properties, the currentas unknown aspattern is acceptable.
406-423: Duplicate helper function; consider extracting to shared scope.This
createMockNodefunction duplicates the one at lines 324-337. Extract it to the top-leveldescribeblock or module scope to avoid redundancy.♻️ Extract shared helper
Move the helper function outside the nested
describeblocks (e.g., after line 93 nearmockWorkflowPacks) so both test sections can reuse it:const createMockNode = ( type: string, packId?: string, version?: string ): LGraphNode => ({ 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 Partial<LGraphNode> as LGraphNodeThen remove both local definitions.
src/workbench/utils/nodeDefOrderingUtil.ts (1)
89-94: TypeScript doesn't narrowMap.get()afterMap.has()check.The
valueMap.get(name)return type remainsTWidgetValue | undefinedeven after thehas()check, potentially allowingundefinedinto the array.♻️ Option 1: Use non-null assertion (safe here due to prior has() check)
for (const name of inputOrder) { if (valueMap.has(name)) { - reordered.push(valueMap.get(name)) + reordered.push(valueMap.get(name)!) usedNames.add(name) } }♻️ Option 2: Single get() with type guard (changes behavior if undefined is valid TWidgetValue)
for (const name of inputOrder) { - if (valueMap.has(name)) { - reordered.push(valueMap.get(name)) + const value = valueMap.get(name) + if (value !== undefined) { + reordered.push(value) usedNames.add(name) } }Option 1 is preferred if
TWidgetValuecan legitimately includeundefinedas a valid value.src/scripts/ui/toggleSwitch.ts (1)
40-64: Side effect: mutating caller's objects.Line 42 mutates the original
itemobject when it's not a string, which could affect the caller's data. This is pre-existing behavior but worth noting. The proposed fix in the previous comment (creatingnormalizedItems) would also address this by not mutating the originals.src/scripts/widgets.ts (1)
193-230: Consider caching original values to avoid redundant computation.
getComboValuesArrayis called twice when the filter removes all items - once for the filtered values and once for the warning check. Since the helper may invoke a function (line 24), this could have side effects or performance implications.♻️ Suggested optimization
if (isCombo && v !== 'fixed') { const comboWidget = targetWidget as IComboWidget - let values = getComboValuesArray( + const originalValues = getComboValuesArray( comboWidget.options.values, comboWidget, node ) + let values = originalValues const filter = comboFilter?.value if (filter) { let check: ((item: string) => boolean) | undefined // ... regex/check logic unchanged ... values = values.filter(check) - if (!values.length && comboWidget.options.values) { - const originalValues = getComboValuesArray( - comboWidget.options.values, - comboWidget, - node - ) - if (originalValues.length) { + if (!values.length && originalValues.length) { console.warn( 'Filter for node ' + node.id + ' has filtered out all items', filter ) - } } }src/composables/graph/useSelectionState.test.ts (1)
157-179: Misleading test name: "should provide non-reactive state computation".The test name suggests testing non-reactive behavior, but it actually verifies that a new
useSelectionState()instance correctly reflects the current (empty) store state after clearingselectedItems. This is testing that the composable reads from the shared store correctly, not non-reactivity.Consider renaming to something like "should reflect current store state in new composable instance" to better describe the actual behavior being tested.
browser_tests/fixtures/utils/taskHistory.ts (1)
119-140: Type annotation improves safety; consider usingTaskOutputdirectly.The explicit accumulator type is a good improvement. Since
TaskOutputis already imported (line 10), consider using it directly to avoid type drift:private createOutputs( filenames: string[], filetype: OutputFileType ): TaskOutput { return filenames.reduce( (outputs, filename, i) => { const nodeId = `${i + 1}` outputs[nodeId] = { [filetype]: [{ filename, subfolder: '', type: 'output' }] } const contentType = getContentType(filename, filetype) this.outputContentTypes.set(filename, contentType) return outputs }, - {} as Record< - string, - { - [key: string]: { filename: string; subfolder: string; type: string }[] - } - > + {} as TaskOutput ) }src/scripts/metadata/png.ts (1)
45-55: Promise may never settle if type guard fails.The
instanceof ArrayBufferguard is correct for type narrowing, but if the condition fails, the promise never resolves or rejects. This could cause the caller to hang indefinitely.Proposed fix to ensure the promise always settles
export function getFromPngFile(file: File) { return new Promise<Record<string, string>>((r) => { const reader = new FileReader() reader.onload = (event) => { if (event.target?.result instanceof ArrayBuffer) { r(getFromPngBuffer(event.target.result)) + } else { + r({}) } } + reader.onerror = () => r({}) reader.readAsArrayBuffer(file) }) }This ensures consistent behavior with the existing
getFromPngBufferfunction which returns{}on invalid input.src/components/dialog/content/MissingCoreNodesMessage.test.ts (1)
17-31: Consider usingPartial<LGraphNode>pattern for mock nodes.Based on learnings, when creating mock objects that partially implement an interface, prefer
as Partial<LGraphNode> as LGraphNodeoverObject.create(null). This explicitly acknowledges the incomplete implementation while maintaining type safety.♻️ Suggested refactor
function createMockNode(type: string, version?: string): LGraphNode { - return Object.assign(Object.create(null), { + return { type, properties: { cnr_id: 'comfy-core', ver: version }, id: 1, title: type, pos: [0, 0], size: [100, 100], flags: {}, graph: null, mode: 0, inputs: [], outputs: [] - }) + } as Partial<LGraphNode> as LGraphNode }browser_tests/tests/featureFlags.spec.ts (1)
332-373: Consider typing__appReadinessinstead of usingas any.Lines 334, 346, 354, 362, and 393 use
(window as any).__appReadiness. For consistency with the rest of the file's type improvements, consider adding__appReadinessto the module-level window augmentation (already present on lines 11-15) and using typed access inside the callback.♻️ Suggested improvement
// Track when various app components are ready - ;(window as any).__appReadiness = { + const win = window as Window & typeof globalThis & { + __appReadiness: { + featureFlagsReceived: boolean + apiInitialized: boolean + appInitialized: boolean + } + } + win.__appReadiness = { featureFlagsReceived: false, apiInitialized: false, appInitialized: false }src/lib/litegraph/src/LGraphCanvas.ts (6)
106-120: FixshowSearchBoxoptions mismatch (slot_tois ignored at runtime).
IShowSearchOptionsonly supportsslot_from, butlinkReleaseContextbuilds{ slot_to: ... }in the “output” branch and then casts toIShowSearchOptions(Line 850).showSearchBox()later only readsoptions.slot_from, so the “output” path won’t pre-select/connect correctly.-->Proposed fix
- const linkReleaseContext = + const linkReleaseContext: IShowSearchOptions = this.linkConnector.state.connectingTo === 'input' ? { node_from: firstLink.node as LGraphNode, slot_from: firstLink.fromSlot as INodeOutputSlot, type_filter_in: firstLink.fromSlot.type } : { node_to: firstLink.node as LGraphNode, - slot_to: firstLink.fromSlot as INodeInputSlot, + slot_from: firstLink.fromSlot as INodeInputSlot, type_filter_out: firstLink.fromSlot.type } @@ - this.showSearchBox(e, linkReleaseContext as IShowSearchOptions) + this.showSearchBox(e, linkReleaseContext)Also applies to: 850-851
1399-1499:onShowPropertyEditor: non-titleedits are written tonode.properties[...](likely wrong).
The new logic writes anykeyof LGraphNode(except'title') intonode.properties[property as string](Line 1496-1499). That changes semantics if this editor is ever used for real node fields (e.g.mode,pos,size, etc.).If this menu is truly “Title only”, restrict it to
property === 'title'and remove the generic branch (safer than silently corruptingnode.properties).
-->
1517-1523: Safer enum lookup: avoid iterating inherited keys.
for (const k in valuesRecord)can walk prototype keys (Line 1518). PreferObject.entries(values as Record<string, unknown>)or guard withhasOwnProperty.
-->
3681-3722: Keyboard shortcuts still fire while typing in<textarea>/ contenteditable.
You now early-return only forHTMLInputElement(Line 3682). That means space/escape/etc can still trigger canvas behaviors while focused in a textarea (or other editable elements), even though you later special-case Delete/Backspace.-->Proposed fix
- const targetEl = e.target - if (targetEl instanceof HTMLInputElement) return + const targetEl = e.target + if ( + targetEl instanceof HTMLInputElement || + targetEl instanceof HTMLTextAreaElement || + (targetEl instanceof HTMLElement && targetEl.isContentEditable) + ) + return
7448-7530:showEditPropertyValue: boolean checkbox writes strings; array/object parse can throw on undefined.
- Checkbox path calls
setValue('true'|'false')(Line 7456-7458), butsetValuedoesn’t convert to boolean, so you’ll persist a string.- For
type === 'array'|'object',JSON.parse(String(value))(Line 7520-7521) will throw ifvalueisundefined(e.g.inputmissing or cleared), crashing the interaction.-->Proposed fix
- if (input instanceof HTMLInputElement) { + if (input instanceof HTMLInputElement) { const checkbox = input checkbox.addEventListener('click', function () { dialog.modified() - // Convert boolean to string for setValue which expects string - setValue(checkbox.checked ? 'true' : 'false') + setValue(checkbox.checked) }) } @@ - function setValue(value: string | number | undefined) { + function setValue(value: unknown) { if ( - value !== undefined && + value !== undefined && info?.values && typeof info.values === 'object' && - info.values[value] != undefined + info.values[String(value)] != undefined ) { - value = info.values[value] + value = info.values[String(value)] } if (typeof node.properties[property] == 'number') { - value = Number(value) + value = Number(value) } + if (typeof node.properties[property] == 'boolean') { + value = Boolean(value) + } if (type == 'array' || type == 'object') { - value = JSON.parse(String(value)) + if (value == null || String(value).trim() === '') value = type === 'array' ? [] : {} + else value = JSON.parse(String(value)) } node.properties[property] = value
4709-4718: Add type augmentation for non-standardctx.start2D()method.The runtime guard is good, but
CanvasRenderingContext2Ddoesn't definestart2Din standard DOM typings. Per coding guidelines, use proper TypeScript types instead of@ts-expect-error. Add a type augmentation insrc/lib/litegraph/src/polyfills.ts(or a dedicated type declaration file) to extendCanvasRenderingContext2D.prototype.start2D, following the pattern already used for theroundRectpolyfill. This ensures the non-standard method is explicitly typed and intentional.browser_tests/tests/useSettingSearch.spec.ts (1)
13-41: Silent skip of extension registration ifappis undefined.Using optional chaining (
window['app']?.registerExtension) means ifappis not initialized, the test settings won't be registered and thebeforeEachsilently completes. Subsequent tests that depend on these settings could pass incorrectly or fail with confusing errors.Consider throwing to fail fast, consistent with other guarded patterns in this PR:
Proposed fix
await comfyPage.page.evaluate(() => { - window['app']?.registerExtension({ + const app = window['app'] + if (!app) throw new Error('App not initialized') + app.registerExtension({ name: 'TestSettingsExtension',browser_tests/tests/extensionAPI.spec.ts (1)
89-119: Replaceas unknown as SettingParamswith an explicit “extension setting” type (avoidunknowncasting).
Right now the cast defeats type-safety and makes it easy to accidentally rely on invalid fields.Based on learnings, avoid `as unknown as ...` and prefer proper typing.Proposed fix
import type { SettingParams } from '../../src/platform/settings/types' import { comfyPageFixture as test } from '../fixtures/ComfyPage' +type ExtensionSettingParams = Omit<SettingParams, 'id'> & { id: string } + test.describe('Topbar commands', () => { @@ test('Should allow adding settings', async ({ comfyPage }) => { await comfyPage.page.evaluate(() => { const app = window['app'] if (!app) throw new Error('App not initialized') app.registerExtension({ @@ onChange: () => { window.changeCount = (window.changeCount ?? 0) + 1 } - } as unknown as SettingParams + } satisfies ExtensionSettingParams ] }) }) @@ test('Should allow setting boolean settings', async ({ comfyPage }) => { await comfyPage.page.evaluate(() => { const app = window['app'] if (!app) throw new Error('App not initialized') app.registerExtension({ @@ onChange: () => { window.changeCount = (window.changeCount ?? 0) + 1 } - } as unknown as SettingParams + } satisfies ExtensionSettingParams ] }) })Also applies to: 121-154
browser_tests/fixtures/utils/litegraphUtils.ts (2)
149-176: Remove debugconsole.logfrompage.evaluate(noisy + slows CI).Proposed fix
const rawPos = node.getConnectionPos(type === 'input', index) const convertedPos = app.canvas.ds.convertOffsetToCanvas(rawPos) - - // Debug logging - convert Float64Arrays to regular arrays for visibility - - console.log( - `NodeSlotReference debug for ${type} slot ${index} on node ${id}:`, - { - nodePos: [node.pos[0], node.pos[1]], - nodeSize: [node.size[0], node.size[1]], - rawConnectionPos: [rawPos[0], rawPos[1]], - convertedPos: [convertedPos[0], convertedPos[1]], - currentGraphType: graph.constructor.name - } - ) return convertedPos
223-324: Useapp.canvas.graphconsistently for accessing the current graph (critical for subgraph support).
getPosition()correctly usesapp.canvas.graph, which reflects the active graph whether in the main graph or a subgraph. However,getSocketPosition()andgetValue()useapp.graph, which always returns the root graph. When a user navigates into a subgraph, these methods will fail to find nodes with "Node not found" errors because they're searching in the root graph instead of the current subgraph.Replace
app.graphwithapp.canvas.graphin bothgetSocketPosition()andgetValue()to match the correct pattern already established ingetPosition().src/lib/litegraph/src/LGraphNode.ts (2)
925-934: Updatewidgets_valuestype to allownullinstead of using type assertions.The serialized
widgets_valuesarray uses sparse indexing and intentionally storesnullfor missing widget slots. The type definition inISerialisedNodeshould beArray<TWidgetValue | null>rather thanTWidgetValue[], eliminating the need for thenull as unknown as TWidgetValueassertion.Required changes
-// in src/lib/litegraph/src/types/serialisation.ts - widgets_values?: TWidgetValue[] + widgets_values?: Array<TWidgetValue | null>- o.widgets_values[i] = widget - ? widget.value - : (null as unknown as TWidgetValue) + o.widgets_values[i] = widget ? widget.value : null
1889-1916: Remove the redundanttoLowerCase()call and unsound type assertion.Since all
TWidgetTypevalues are already lowercase ('toggle','number','slider', etc.) andTypeis constrained toTWidgetType, thetoLowerCase()call is redundant and theas Typecast is both unnecessary and violates the coding guideline prohibiting type assertions. Store the type directly without casting:const w: IBaseWidget & { type: Type } = { type: type, name: name, ... }Or simply:
const w: IBaseWidget & { type: Type } = { type, name, ... }browser_tests/fixtures/ComfyPage.ts (3)
891-1055: Avoid fixed in-page timeouts for UI emergence (flaky); poll instead.The
setTimeout(100/200ms)insidepage.evaluate()is a hiddenwaitForTimeoutequivalent and can be brittle under load/CI. Prefer polling for the condition (menu/dialog) with a bounded timeout inside the evaluate, or remove the sleeps and rely on Playwright-sidewaitForSelector/toPassonly. Based on learnings, avoid waitForTimeout-style sleeps in Playwright tests.
1854-1871: Move focus evaluation insidetoPass()callback and fix negation handling in message.The
isFocusedvalue is evaluated once beforetoPass(), preventing retries from observing focus state changes. Move the evaluation inside the callback so retries can re-evaluate as the DOM state changes.Also, the message incorrectly uses
isFocusedto conditionally add "not " instead of respectingthis.isNot, causing wrong assertion messages when using.not.toHaveFocus().Proposed fix
async toHaveFocus( this: MatcherContext, locator: Locator, options = { timeout: 256 } ) { - const isFocused = await locator.evaluate( - (el) => el === document.activeElement - ) - await expect(async () => { - expect(isFocused).toBe(!this.isNot) + const isFocused = await locator.evaluate((el) => el === document.activeElement) + expect(isFocused).toBeTruthy() }).toPass(options) + const isFocused = await locator.evaluate((el) => el === document.activeElement) return { - pass: isFocused, - message: () => `Expected element to ${isFocused ? 'not ' : ''}be focused.` + pass: isFocused, + message: () => + `Expected element ${this.isNot ? 'not ' : ''}to be focused.` } }
1823-1848: Custom matcher violates Playwright contract:passmust reflect actual condition, not!this.isNot.The implementation returns
pass: !this.isNotindependent of the actual assertion outcome. Per Playwright's custom matcher contract,passmust be the actual boolean result (e.g., whether the node is pinned), and the framework automatically inverts both the pass value and message when.notis used. Manually applying.notto the assertion and hardcodingpassbased on negation breaks the contract.Current behavior:
.not.toBePinned()fails only becausepass: !true, not because of actual state.Correct fix: return
pass: conditionPass(the actual result fromgetValue()), remove manual.notapplication, and let Playwright handle inversion.Corrected implementation
const makeMatcher = function <T>( getValue: (node: NodeReference) => Promise<T> | T, type: string ) { return async function ( this: MatcherContext, node: NodeReference, options?: { timeout?: number; intervals?: number[] } ) { - const value = await getValue(node) - let assertion = expect( - value, - 'Node is ' + (this.isNot ? '' : 'not ') + type - ) - if (this.isNot) { - assertion = assertion.not - } - await expect(async () => { - assertion.toBeTruthy() - }).toPass({ timeout: 250, ...options }) + const conditionPass = await getValue(node) + await expect(async () => { + expect(Boolean(conditionPass)).toBe(true) + }).toPass({ timeout: 250, ...options }) return { - pass: !this.isNot, - message: () => 'Node is ' + (this.isNot ? 'not ' : '') + type + pass: Boolean(conditionPass), + message: () => conditionPass + ? `Node is not ${type}` + : `Node is ${type}` } } }src/scripts/ui.ts (2)
237-243: Avoid unsafe cast for list type default.
text.toLowerCase() as 'queue' | 'history'will silently produce an invalid type iftextchanges. Prefer an explicit mapping.Proposed fix
constructor(text: string, type?: 'queue' | 'history', reverse?: boolean) { this.#text = text - this.#type = type || (text.toLowerCase() as 'queue' | 'history') + const derived = + text.toLowerCase() === 'queue' + ? 'queue' + : text.toLowerCase() === 'history' + ? 'history' + : undefined + this.#type = type ?? derived ?? 'queue' this.#reverse = reverse || false this.element = $el('div.comfy-list') as HTMLDivElement this.element.style.display = 'none' }
249-296: Avoid in-placereverse()on API response arrays.
reverse()mutates the array returned fromapi.getItems(), which can create hard-to-track ordering issues if the object is reused.Proposed fix
- ...(this.#reverse - ? (items[section as keyof typeof items] as TaskItem[]).reverse() - : (items[section as keyof typeof items] as TaskItem[]) - ).map((item: TaskItem) => { + ...((sectionItems) => { + const list = [...sectionItems] + return this.#reverse ? list.reverse() : list + })((items[section as keyof typeof items] as TaskItem[]) ?? []).map((item: TaskItem) => {src/extensions/core/nodeTemplates.ts (1)
123-148: Import flow: handle JSON parse errors + don’tawaitreadAsText (void).
JSON.parse(reader.result as string)can throw and currently bubbles out of the onload handler. Alsoawait reader.readAsText(file)doesn’t do anything (readAsText returns void).Proposed fix
reader.onload = async () => { - const importFile = JSON.parse(reader.result as string) - if (importFile?.templates) { - for (const template of importFile.templates) { - if (template?.name && template?.data) { - this.templates.push(template) - } - } - await this.store() - } + try { + const importFile = JSON.parse(String(reader.result ?? 'null')) + if (importFile?.templates) { + for (const template of importFile.templates) { + if (template?.name && template?.data) { + this.templates.push(template) + } + } + await this.store() + } + } catch (error) { + useToastStore().addAlert(error instanceof Error ? error.message : String(error)) + } } -reader.readAsText(file) +reader.readAsText(file)src/lib/litegraph/src/LGraph.ts (2)
1227-1246: Avoid type assertions here; prefer runtime method checks (litegraph guideline).Instead of casting
node as LGraphNode & { onTrigger?: ... }, do'onTrigger' in node+typeof === 'function'. This keeps “assertions last resort” intact. Based on learnings, type assertions should be a last resort in litegraph.
2203-2217: Verify meaning ofthis.version: schema version vs library version.
_configureBasewritesdata.versionintothis.version, butserialize()later emitsversion: LiteGraph.VERSION(different meaning). If both meanings must exist, consider separating names (e.g.,schemaVersionvslitegraphVersion) to prevent future misuse.
| const workflowName = await comfyPage.page.evaluate(async () => { | ||
| return window['app'].extensionManager.workflow.activeWorkflow.filename | ||
| const app = window['app'] | ||
| if (!app) throw new Error('App not initialized') | ||
| const extMgr = app.extensionManager as { | ||
| workflow?: { activeWorkflow?: { filename?: string } } | ||
| } | ||
| return extMgr.workflow?.activeWorkflow?.filename | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting repeated type to reduce duplication.
The inline type shape for extensionManager is repeated three times. While acceptable in test code, extracting to a local type alias would improve maintainability.
♻️ Optional: Extract shared type
// At top of file or in test describe block
type ExtMgrShape = {
workflow?: {
activeWorkflow?: {
filename?: string
delete?: () => Promise<void>
}
}
}
// Then in evaluate callbacks:
const extMgr = app.extensionManager as ExtMgrShapeAlso applies to: 47-54
🤖 Prompt for AI Agents
In @browser_tests/tests/browserTabTitle.spec.ts around lines 28 - 35, The inline
type for app.extensionManager is duplicated in multiple evaluate callbacks
(e.g., the callback used to compute workflowName and the one at lines 47-54);
extract a shared local type alias (for example ExtMgrShape) scoped at the top of
the test file or inside the describe block, include the optional
workflow.activeWorkflow.filename and delete signature as needed, and then cast
extensionManager with that alias (e.g., app.extensionManager as ExtMgrShape) in
each comfyPage.page.evaluate callback to remove duplication and improve
maintainability.
| await comfyPage.page.evaluate(() => { | ||
| const node = window['graph']._nodes_by_id['4'] | ||
| const graph = window['graph'] as GraphWithNodes | ||
| const node = graph._nodes_by_id['4'] | ||
| node.widgets.push(node.widgets[0]) | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a guard for consistency with other test files
Other test files in this PR add explicit guards (e.g., if (!graph) throw new Error('Graph not initialized')). While the test will fail if the graph doesn't exist, an explicit guard provides clearer error messages.
Optional improvement
await comfyPage.page.evaluate(() => {
const graph = window['graph'] as GraphWithNodes
+ if (!graph) throw new Error('Graph not initialized')
const node = graph._nodes_by_id['4']
node.widgets.push(node.widgets[0])
})📝 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.
| await comfyPage.page.evaluate(() => { | |
| const node = window['graph']._nodes_by_id['4'] | |
| const graph = window['graph'] as GraphWithNodes | |
| const node = graph._nodes_by_id['4'] | |
| node.widgets.push(node.widgets[0]) | |
| }) | |
| await comfyPage.page.evaluate(() => { | |
| const graph = window['graph'] as GraphWithNodes | |
| if (!graph) throw new Error('Graph not initialized') | |
| const node = graph._nodes_by_id['4'] | |
| node.widgets.push(node.widgets[0]) | |
| }) |
🤖 Prompt for AI Agents
In @browser_tests/tests/vueNodes/widgets/widgetReactivity.spec.ts around lines
19 - 23, Add an explicit guard before manipulating the graph inside the
comfyPage.page.evaluate callback: ensure window['graph'] exists and is typed
(GraphWithNodes) and throw a clear Error like "Graph not initialized" if not;
then proceed to access graph._nodes_by_id['4'] and node.widgets, so replace the
silent assumption in the comfyPage.page.evaluate block with a check that throws
a descriptive error when graph or the target node is missing.
| await comfyPage.page.evaluate(() => { | ||
| const widget = window['app'].graph.nodes[0].widgets[0] | ||
| const app = window['app'] | ||
| if (!app?.graph?.nodes?.[0]?.widgets?.[0]) return | ||
| const widget = app.graph.nodes[0].widgets[0] | ||
| widget.callback = (value: number) => { | ||
| window['widgetValue'] = value | ||
| window.widgetValue = value | ||
| } | ||
| }) |
There was a problem hiding this comment.
Early return may silently mask test failures.
The guard if (!app?.graph?.nodes?.[0]?.widgets?.[0]) return silently exits without setting up the callback. If the app/graph/widget is missing, the test will pass even though the actual behavior wasn't tested. Consider throwing an error instead for fail-fast behavior, consistent with other tests in this PR.
🐛 Suggested fix
await comfyPage.page.evaluate(() => {
const app = window['app']
- if (!app?.graph?.nodes?.[0]?.widgets?.[0]) return
- const widget = app.graph.nodes[0].widgets[0]
+ if (!app?.graph?.nodes?.[0]?.widgets?.[0]) {
+ throw new Error('App, graph, or widget not found')
+ }
+ const widget = app.graph.nodes[0].widgets[0]!
widget.callback = (value: number) => {
window.widgetValue = value
}
})📝 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.
| await comfyPage.page.evaluate(() => { | |
| const widget = window['app'].graph.nodes[0].widgets[0] | |
| const app = window['app'] | |
| if (!app?.graph?.nodes?.[0]?.widgets?.[0]) return | |
| const widget = app.graph.nodes[0].widgets[0] | |
| widget.callback = (value: number) => { | |
| window['widgetValue'] = value | |
| window.widgetValue = value | |
| } | |
| }) | |
| await comfyPage.page.evaluate(() => { | |
| const app = window['app'] | |
| if (!app?.graph?.nodes?.[0]?.widgets?.[0]) { | |
| throw new Error('App, graph, or widget not found') | |
| } | |
| const widget = app.graph.nodes[0].widgets[0]! | |
| widget.callback = (value: number) => { | |
| window.widgetValue = value | |
| } | |
| }) |
🤖 Prompt for AI Agents
In @browser_tests/tests/widget.spec.ts around lines 110 - 117, Replace the
silent early return in the comfyPage.page.evaluate callback with a fail-fast
error: inside the evaluate block that accesses window['app'] and checks
app?.graph?.nodes?.[0]?.widgets?.[0], throw a descriptive Error when the widget
is missing instead of returning, then proceed to assign widget.callback =
(value: number) => { window.widgetValue = value }; this ensures tests fail
loudly if the app/graph/widget is not present.
| await comfyPage.page.evaluate(() => { | ||
| const widget = window['app'].graph.nodes[0].widgets[0] | ||
| const app = window['app'] | ||
| if (!app?.graph?.nodes?.[0]?.widgets?.[0]) return | ||
| const widget = app.graph.nodes[0].widgets[0] | ||
| widget.callback = (value: number) => { | ||
| window['widgetValue'] = value | ||
| window.widgetValue = value | ||
| } | ||
| }) |
There was a problem hiding this comment.
Same early return concern as above.
This guard also uses early return which could silently skip the callback setup without failing the test.
🐛 Suggested fix
await comfyPage.page.evaluate(() => {
const app = window['app']
- if (!app?.graph?.nodes?.[0]?.widgets?.[0]) return
- const widget = app.graph.nodes[0].widgets[0]
+ if (!app?.graph?.nodes?.[0]?.widgets?.[0]) {
+ throw new Error('App, graph, or widget not found')
+ }
+ const widget = app.graph.nodes[0].widgets[0]!
widget.callback = (value: number) => {
window.widgetValue = value
}
})🤖 Prompt for AI Agents
In @browser_tests/tests/widget.spec.ts around lines 133 - 140, The evaluate
callback currently silently returns when the widget is missing
(window['app']?.graph?.nodes?.[0]?.widgets?.[0]) which can hide failures;
instead, inside comfyPage.page.evaluate ensure the widget exists and throw an
error if it does not (e.g., check app/graph/nodes[0]/widgets[0] and throw new
Error('widget not found') when absent) before assigning widget.callback so the
test fails loudly; update the block referencing window['app'],
app.graph.nodes[0].widgets[0], and widget.callback accordingly.
| ## Never Use `@ts-ignore` or `@ts-expect-error` | ||
|
|
||
| These directives suppress all errors on a line, making it easy to accidentally mask serious bugs. Instead: | ||
|
|
||
| 1. Fix the underlying type issue | ||
| 2. Use a type guard to narrow the type | ||
| 3. If truly unavoidable, use a targeted cast with explanation | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify the relationship between "never use @ts-expect-error" and "targeted cast with explanation".
The guidance on lines 134-136 suggests a "targeted cast with explanation" as a fallback option, but the earlier section (lines 10-12) states "Never Use Type Assertions (as)". Consider clarifying when a targeted cast might be acceptable (e.g., in type guard implementations, test mocks) to avoid confusion.
📝 Suggested clarification
1. Fix the underlying type issue
2. Use a type guard to narrow the type
-3. If truly unavoidable, use a targeted cast with explanation
+3. If truly unavoidable (e.g., type guard implementations, test mocks with `Partial<T>`), use a targeted cast with explanation📝 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.
| ## Never Use `@ts-ignore` or `@ts-expect-error` | |
| These directives suppress all errors on a line, making it easy to accidentally mask serious bugs. Instead: | |
| 1. Fix the underlying type issue | |
| 2. Use a type guard to narrow the type | |
| 3. If truly unavoidable, use a targeted cast with explanation | |
| ## Never Use `@ts-ignore` or `@ts-expect-error` | |
| These directives suppress all errors on a line, making it easy to accidentally mask serious bugs. Instead: | |
| 1. Fix the underlying type issue | |
| 2. Use a type guard to narrow the type | |
| 3. If truly unavoidable (e.g., type guard implementations, test mocks with `Partial<T>`), use a targeted cast with explanation |
🤖 Prompt for AI Agents
In @docs/typescript/type-safety.md around lines 130 - 137, Clarify the apparent
contradiction between the "Never Use Type Assertions (`as`)" guidance and the
fallback "targeted cast with explanation" in the "Never Use `@ts-ignore` or
`@ts-expect-error`" section by explicitly stating when a targeted cast is
acceptable: mention it as a last-resort, well-documented exception used only for
narrow cases such as type guard implementations, test mocks, interop with
third-party libs where types are known but not expressed, or migrating legacy
code; update the text around the headings "Never Use Type Assertions (`as`)" and
"Never Use `@ts-ignore` or `@ts-expect-error`" to include this reconciliation
and require adding a short comment explaining why the cast is safe and linking
to an example pattern or test demonstrating the invariant.
| if (!over || !currentImgs) return | ||
| // Set the overIndex so Open Image etc work | ||
| // @ts-expect-error fixme ts strict error | ||
| const idx = currentImgs.indexOf(over) | ||
| const idx = currentImgs.indexOf(over as HTMLImageElement) | ||
| node.overIndex = idx |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using type guard instead of type assertion.
The cast over as HTMLImageElement is a type assertion which the coding guidelines discourage. Since indexOf returns -1 for non-members anyway, this works, but a type guard would be more explicit.
♻️ Optional improvement using type guard
if (!over || !currentImgs) return
// Set the overIndex so Open Image etc work
- const idx = currentImgs.indexOf(over as HTMLImageElement)
+ const idx = over instanceof HTMLImageElement ? currentImgs.indexOf(over) : -1
node.overIndex = idx📝 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.
| if (!over || !currentImgs) return | |
| // Set the overIndex so Open Image etc work | |
| // @ts-expect-error fixme ts strict error | |
| const idx = currentImgs.indexOf(over) | |
| const idx = currentImgs.indexOf(over as HTMLImageElement) | |
| node.overIndex = idx | |
| if (!over || !currentImgs) return | |
| // Set the overIndex so Open Image etc work | |
| const idx = over instanceof HTMLImageElement ? currentImgs.indexOf(over) : -1 | |
| node.overIndex = idx |
🤖 Prompt for AI Agents
In @src/scripts/ui/imagePreview.ts around lines 119 - 122, The code uses a type
assertion over as HTMLImageElement when computing the index; replace this with a
type guard by checking if over is an instance of HTMLImageElement before calling
currentImgs.indexOf. Concretely, after the existing null checks on over and
currentImgs, use "if (over instanceof HTMLImageElement) { const idx =
currentImgs.indexOf(over); node.overIndex = idx; } else { node.overIndex = -1;
}" so you avoid the cast and explicitly handle non-image values while keeping
node.overIndex deterministic.
| export const adjustColor: <T extends string | null | undefined>( | ||
| color: T, | ||
| options: ColorAdjustOptions | ||
| ) => string = memoize( | ||
| ) => T | string = memoize( | ||
| applyColorAdjustments, | ||
| (color: string, options: ColorAdjustOptions): string => | ||
| (color: string | null | undefined, options: ColorAdjustOptions): string => | ||
| `${color}-${JSON.stringify(options)}` |
There was a problem hiding this comment.
Cache key collision between null/undefined values and literal strings.
The template literal ${color} produces identical strings for null and the string "null" (similarly for undefined). This causes cache collisions where adjustColor(null, opts) and adjustColor("null", opts) share the same key but return different results.
🔧 Suggested fix using JSON.stringify
export const adjustColor: <T extends string | null | undefined>(
color: T,
options: ColorAdjustOptions
) => T | string = memoize(
applyColorAdjustments,
(color: string | null | undefined, options: ColorAdjustOptions): string =>
- `${color}-${JSON.stringify(options)}`
+ `${JSON.stringify(color)}-${JSON.stringify(options)}`
)This produces distinct keys: "null" for null vs "\"null\"" for the string "null".
🤖 Prompt for AI Agents
In @src/utils/colorUtil.ts around lines 355 - 361, The memoized adjustColor
implementation builds a cache key using `${color}-${JSON.stringify(options)}`,
which conflates null/undefined with the literal strings "null"/"undefined";
change the keyResolver passed to memoize so it JSON.stringify() the color as
well (e.g., JSON.stringify(color) + '-' + JSON.stringify(options)) to produce
distinct keys for null/undefined versus the string literals; update the memoize
call that wraps applyColorAdjustments in the adjustColor declaration to use this
new key format.
src/utils/vintageClipboard.ts
Outdated
| // @ts-expect-error fixme ts strict error | ||
| graph.beforeChange() | ||
|
|
||
| const deserialised = JSON.parse(data) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding runtime validation for parsed data.
JSON.parse returns any, so deserialised lacks type safety. Since this is a deprecated function parsing legacy data, this is acceptable. However, for consistency with the PR's type safety goals, consider adding minimal runtime guards:
🔧 Optional: Add defensive guards for parsed structure
const deserialised = JSON.parse(data)
+ if (!deserialised?.nodes || !Array.isArray(deserialised.nodes)) return
+ if (!deserialised?.links || !Array.isArray(deserialised.links)) returnThis provides runtime safety without requiring a type assertion, which aligns with the learnings that type assertions should be avoided.
📝 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 deserialised = JSON.parse(data) | |
| const deserialised = JSON.parse(data) | |
| if (!deserialised?.nodes || !Array.isArray(deserialised.nodes)) return | |
| if (!deserialised?.links || !Array.isArray(deserialised.links)) return |
🤖 Prompt for AI Agents
In @src/utils/vintageClipboard.ts at line 95, Add minimal runtime validation
after the JSON.parse call that sets `deserialised` in vintageClipboard.ts: check
that `deserialised` is an object and that expected legacy fields (e.g., specific
keys or types you rely on later) exist and have the correct primitive types
before using them; if validation fails, handle it safely (return early, throw a
clear error, or fallback) so you avoid untyped assumptions instead of using a
type assertion on `deserialised`.
| const nodeNames = computed(() => { | ||
| // @ts-expect-error comfy_nodes is an Algolia-specific field | ||
| const { comfy_nodes } = nodePack | ||
| return comfy_nodes ?? [] | ||
| const packWithAlgolia = nodePack as typeof nodePack & { | ||
| comfy_nodes?: string[] | ||
| } | ||
| return packWithAlgolia.comfy_nodes ?? [] | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good improvement replacing @ts-expect-error with explicit type assertion.
The inline type extension pattern is acceptable here since components['schemas']['Node'] is auto-generated from the registry API schema and comfy_nodes appears to be an Algolia-specific property not present in the base type.
If this pattern is needed elsewhere, consider extracting a reusable type alias:
type NodeWithAlgoliaFields = components['schemas']['Node'] & {
comfy_nodes?: string[]
}This would centralize the Algolia augmentation and make it easier to maintain if additional Algolia-specific fields need to be added.
🤖 Prompt for AI Agents
In @src/workbench/extensions/manager/components/manager/infoPanel/InfoTabs.vue
around lines 69 - 74, Replace the inline ad-hoc type assertion in the nodeNames
computed getter with a reusable type alias: define a NodeWithAlgoliaFields type
as components['schemas']['Node'] & { comfy_nodes?: string[] } and cast nodePack
to that type inside the nodeNames computed (referencing nodeNames and nodePack)
so the Algolia-specific comfy_nodes augmentation is centralized and easier to
reuse/maintain.
| const app = window.app | ||
| if (!app) { | ||
| resolve(undefined) | ||
| return | ||
| } | ||
| app.ui.settings.addEventListener('Comfy.ColorPalette.change', resolve, { | ||
| once: true | ||
| }) | ||
|
|
||
| setTimeout(resolve, 5000) | ||
| }) |
There was a problem hiding this comment.
Ensure the settings listener is removed if the timeout wins (leaks across tests).
setTimeout(resolve, 5000) can resolve without removing the Comfy.ColorPalette.change listener (since only { once: true } removes it on event). Consider cleaning up on timeout.
🤖 Prompt for AI Agents
In @browser_tests/fixtures/ComfyPage.ts around lines 84 - 94, The timeout path
can resolve without removing the Comfy.ColorPalette.change listener, leaking
across tests; fix by capturing the event handler in a const (e.g., handler =
(ev) => resolve(ev)), add it via
app.ui.settings.addEventListener('Comfy.ColorPalette.change', handler, { once:
true }), and in the timeout callback call
app.ui.settings.removeEventListener('Comfy.ColorPalette.change', handler) (and
clear the timeout if needed) before calling resolve so the listener is always
cleaned up whether the event or timeout wins.
| await this.page.waitForFunction( | ||
| () => window['app']?.extensionManager?.workflow?.isBusy === false, | ||
| () => { | ||
| const app = window.app | ||
| if (!app) return true | ||
| const extMgr = app.extensionManager as { | ||
| workflow?: { isBusy?: boolean } | ||
| } | ||
| return extMgr.workflow?.isBusy === false | ||
| }, | ||
| undefined, | ||
| { timeout: 3000 } | ||
| ) |
There was a problem hiding this comment.
Busy-wait condition can hang when workflow extension is absent/undefined.
extMgr.workflow?.isBusy === false is false when workflow or isBusy is undefined, causing a guaranteed 3s timeout even when there’s nothing to wait for. Prefer “not busy” semantics.
Proposed fix
await this.page.waitForFunction(
() => {
const app = window.app
if (!app) return true
const extMgr = app.extensionManager as {
workflow?: { isBusy?: boolean }
}
- return extMgr.workflow?.isBusy === false
+ // Treat missing workflow/isBusy as "not busy" for test stability.
+ return extMgr.workflow?.isBusy !== true
},
undefined,
{ timeout: 3000 }
)📝 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.
| await this.page.waitForFunction( | |
| () => window['app']?.extensionManager?.workflow?.isBusy === false, | |
| () => { | |
| const app = window.app | |
| if (!app) return true | |
| const extMgr = app.extensionManager as { | |
| workflow?: { isBusy?: boolean } | |
| } | |
| return extMgr.workflow?.isBusy === false | |
| }, | |
| undefined, | |
| { timeout: 3000 } | |
| ) | |
| await this.page.waitForFunction( | |
| () => { | |
| const app = window.app | |
| if (!app) return true | |
| const extMgr = app.extensionManager as { | |
| workflow?: { isBusy?: boolean } | |
| } | |
| // Treat missing workflow/isBusy as "not busy" for test stability. | |
| return extMgr.workflow?.isBusy !== true | |
| }, | |
| undefined, | |
| { timeout: 3000 } | |
| ) |
🤖 Prompt for AI Agents
In @browser_tests/fixtures/ComfyPage.ts around lines 143 - 154, The
waitForFunction busy-check treats undefined workflow/isBusy as false which
causes unnecessary timeout; update the predicate inside
this.page.waitForFunction so it treats missing workflow or missing isBusy as
"not busy" (e.g., use extMgr.workflow?.isBusy !== true or explicitly check
extMgr.workflow == null || extMgr.workflow.isBusy === false) so the function
returns true when the workflow extension is absent; keep the surrounding
structure (this.page.waitForFunction, extMgr, workflow, isBusy) intact and only
change the boolean condition to "not true" semantics.
| type TestPalette = Omit<Palette, 'colors'> & { | ||
| colors: { | ||
| node_slot: Record<string, string> | ||
| litegraph_base: Partial<Palette['colors']['litegraph_base']> | ||
| comfy_base: Partial<Palette['colors']['comfy_base']> | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where custom palettes are validated/parsed and whether partial palette objects are accepted.
rg -n "CustomColorPalettes|addCustomColorPalette|paletteSchema|ColorPalette" src | head -n 200Repository: Comfy-Org/ComfyUI_frontend
Length of output: 16777
🏁 Script executed:
cat -n src/schemas/colorPaletteSchema.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 4678
🏁 Script executed:
head -n 25 browser_tests/tests/colorPalette.spec.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 771
Remove redundant Partial<> from litegraph_base and comfy_base or align node_slot with the schema.
The source Palette type already accepts partial colors via partialColorsSchema, so the Partial<> wrappers on litegraph_base and comfy_base are redundant. However, TestPalette uses Record<string, string> for node_slot, which is looser than the schema's nodeSlotSchema.partial() and allows arbitrary keys. Either use the actual partial schema type for consistency or document why the test needs this flexibility.
🤖 Prompt for AI Agents
In @browser_tests/tests/colorPalette.spec.ts around lines 10 - 16, The
TestPalette definition is inconsistent with the source Palette: remove the
redundant Partial<> wrappers from litegraph_base and comfy_base (they already
accept partial colors via partialColorsSchema) and replace the overly-permissive
node_slot: Record<string,string> with the schema-aligned type (e.g. use
Partial<Palette['colors']['node_slot']> or the exact type derived from
nodeSlotSchema.partial()) so the test type matches the real Palette shape or add
a short comment explaining why arbitrary keys are required.
| test('Can add custom color palette', async ({ comfyPage }) => { | ||
| await comfyPage.page.evaluate((p) => { | ||
| window['app'].extensionManager.colorPalette.addCustomColorPalette(p) | ||
| const app = window['app'] | ||
| if (!app) throw new Error('App not initialized') | ||
| const extMgr = app.extensionManager as { | ||
| colorPalette?: { addCustomColorPalette?: (p: unknown) => void } | ||
| } | ||
| extMgr.colorPalette?.addCustomColorPalette?.(p) | ||
| }, customColorPalettes.obsidian_dark) |
There was a problem hiding this comment.
Fail fast if addCustomColorPalette is missing (don’t optional-chain a required test API).
The test intention is “can add”; silently skipping the call makes failures harder to root-cause.
Proposed fix
await comfyPage.page.evaluate((p) => {
const app = window['app']
if (!app) throw new Error('App not initialized')
const extMgr = app.extensionManager as {
colorPalette?: { addCustomColorPalette?: (p: unknown) => void }
}
- extMgr.colorPalette?.addCustomColorPalette?.(p)
+ if (!extMgr.colorPalette?.addCustomColorPalette) {
+ throw new Error('colorPalette.addCustomColorPalette not available')
+ }
+ extMgr.colorPalette.addCustomColorPalette(p)
}, customColorPalettes.obsidian_dark)🤖 Prompt for AI Agents
In @browser_tests/tests/colorPalette.spec.ts around lines 185 - 193, The test
currently optional-chains extMgr.colorPalette?.addCustomColorPalette?.(p) which
silently skips the call if the API is missing; change the inline evaluate body
to fail fast by asserting the API exists and then calling it directly: retrieve
window['app'], assert app and app.extensionManager exist, assert
extensionManager.colorPalette and its addCustomColorPalette function are defined
(throw a helpful Error if not), then invoke addCustomColorPalette(p) so the test
fails immediately when the required API is absent.
| autoQueueMode!: string | ||
| graphHasChanged!: boolean | ||
| autoQueueEnabled!: boolean | ||
| menuContainer!: HTMLDivElement | ||
| queueSize!: Element | ||
| restoreMenuPosition!: () => void | ||
| loadFile!: () => void | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Initialize ComfyUI flags to concrete defaults (avoid “undefined means false” drift).
The definite assignment fields are now type-safe, but runtime values are still undefined until user interaction. Consider setting defaults (e.g., autoQueueEnabled = false, graphHasChanged = false, autoQueueMode = 'instant') to make behavior explicit.
Also applies to: 356-372, 406-409
| // Initialize with placeholder text before first status update | ||
| this.queueSize.textContent = 'Queue size: X' | ||
| } |
There was a problem hiding this comment.
New user-visible string should be localized.
'Queue size: X' is user-facing; consider moving it to i18n. As per coding guidelines, use vue-i18n for user-facing strings.
🤖 Prompt for AI Agents
In @src/scripts/ui.ts around lines 666 - 668, The hardcoded user-facing
placeholder "Queue size: X" set at the this.queueSize.textContent assignment
should be moved to i18n; replace the literal with a vue-i18n translation lookup
(e.g., use this.$t or i18n.t) referencing a new key like
"queue.queueSizePlaceholder" and add that key to your locale files with the
value "Queue size: X" (and translations). Ensure the code uses the translation
call where this.queueSize.textContent is set (in src/scripts/ui.ts) so the UI
string is localized.
| popup: ComfyPopup | null = null | ||
| element: HTMLElement | ||
| overIcon: string | ||
| iconSize: number | ||
| content: string | HTMLElement | ||
| icon: string | ||
| tooltip: string | ||
| classList: ClassList | ||
| hidden: boolean | ||
| enabled: boolean | ||
| action: (e: Event, btn: ComfyButton) => void | ||
| overIcon!: string | ||
| iconSize!: number | ||
| content!: string | HTMLElement | ||
| icon!: string | ||
| tooltip!: string | ||
| classList!: ClassList | ||
| hidden!: boolean | ||
| enabled!: boolean | ||
| action!: (e: Event, btn: ComfyButton) => void | ||
|
|
There was a problem hiding this comment.
Don’t “type away” optional props with !—make fields truly optional or provide defaults.
prop(..., icon) / prop(..., action) can return undefined at runtime, but fields are declared as required (e.g., action!: (...) => void). This is brittle and defeats the goal of “type safety without suppressions”. Based on learnings, avoid assertion crutches and model the real optionality.
Proposed fix (one direction: default + required field)
type ComfyButtonProps = {
...
- action?: (e: Event, btn: ComfyButton) => void
+ action?: (e: Event, btn: ComfyButton) => void
...
}
export class ComfyButton implements ComfyComponent<HTMLElement> {
...
- action!: (e: Event, btn: ComfyButton) => void
+ action!: (e: Event, btn: ComfyButton) => void
...
constructor({ ..., action, ... }: ComfyButtonProps) {
...
- this.action = prop(this, 'action', action)!
+ this.action = prop(this, 'action', action ?? (() => {}))!
this.element.addEventListener('click', (e) => {
...
- this.action?.(e, this)
+ this.action(e, this)
})Also applies to: 72-115, 123-124
| for (const el of [this.element, popup.element]) { | ||
| el.addEventListener('mouseenter', () => { | ||
| this.popup.open = !!++this.#over | ||
| popup.open = !!++this.#over | ||
| }) | ||
| el.addEventListener('mouseleave', () => { | ||
| this.popup.open = !!--this.#over | ||
| popup.open = !!--this.#over | ||
| }) | ||
| } |
There was a problem hiding this comment.
Guard against #over counter underflow (defensive).
popup.open = !!--this.#over can go negative if events get out of balance (e.g., nested elements / missed mouseleave). Consider clamping at 0.
🤖 Prompt for AI Agents
In @src/scripts/ui/components/button.ts around lines 164 - 171, The hover
counter can underflow; change the mouseenter/mouseleave handlers to update the
private counter without going below 0 and derive popup.open from whether the
counter is > 0. Specifically, replace the ++/-- logic around this.#over in the
event listeners attached to this.element and popup.element with safe increments
and safe decrements (clamp to zero, e.g., this.#over = Math.max(0, this.#over -
1)) and set popup.open = this.#over > 0 after updating so the counter never
becomes negative and open reflects the clamped count.
src/types/workflowSchemaTypes.ts
Outdated
| * ISerialisedGraph is the LiteGraph serialization format. | ||
| * | ||
| * TODO: Align these schemas to eliminate the need for this cast. | ||
| * @see https://github.com/Comfy-Org/ComfyUI_frontend/issues/XXXX |
There was a problem hiding this comment.
Update placeholder issue number.
The @see reference contains a placeholder XXXX instead of an actual issue number. Either create the tracking issue and update this reference, or remove the line if no issue exists yet.
-* @see https://github.com/Comfy-Org/ComfyUI_frontend/issues/XXXX
+* @see https://github.com/Comfy-Org/ComfyUI_frontend/issues/[ACTUAL_NUMBER]🤖 Prompt for AI Agents
In @src/types/workflowSchemaTypes.ts at line 12, The @see JSDoc line in
workflowSchemaTypes.ts currently uses a placeholder
"https://github.com/Comfy-Org/ComfyUI_frontend/issues/XXXX"; replace "XXXX" with
the actual issue number for the tracking issue (or, if no issue will be created,
remove the entire @see line) so the reference points to a valid GitHub issue;
search for the @see line in the file to locate and update it.
…ty sort Amp-Thread-ID: https://ampcode.com/threads/T-019bb479-36cd-721b-8415-b0723dfeea83 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019bb479-36cd-721b-8415-b0723dfeea83 Co-authored-by: Amp <amp@ampcode.com>
|
@coderabbitai review full |
|
@DrJKL: I'll conduct a comprehensive full review of this PR. Given the extensive scope of this type safety refactoring, I'll examine: Key Review Areas:
Focus Points:
Starting the full review now across all 180 changed files. 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 47
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (16)
src/scripts/metadata/png.ts (1)
45-55: Promise may never settle if the guard condition is false.The type guard correctly replaces the
@ts-expect-error, but the promise now silently hangs ifevent.target?.resultis not anArrayBuffer. WhilereadAsArrayBuffershould produce anArrayBufferon success, defensive code should ensure the promise always settles.Additionally, there's no
onerrorhandler for theFileReader, so read failures will also leave the promise hanging.🔧 Proposed fix to ensure the promise always settles
export function getFromPngFile(file: File) { return new Promise<Record<string, string>>((r) => { const reader = new FileReader() reader.onload = (event) => { if (event.target?.result instanceof ArrayBuffer) { r(getFromPngBuffer(event.target.result)) + } else { + r({}) } } + reader.onerror = () => r({}) reader.readAsArrayBuffer(file) }) }Alternatively, reject on error for more explicit failure handling:
🔧 Alternative: reject on failure
-export function getFromPngFile(file: File) { - return new Promise<Record<string, string>>((r) => { +export function getFromPngFile(file: File) { + return new Promise<Record<string, string>>((resolve, reject) => { const reader = new FileReader() reader.onload = (event) => { if (event.target?.result instanceof ArrayBuffer) { - r(getFromPngBuffer(event.target.result)) + resolve(getFromPngBuffer(event.target.result)) + } else { + reject(new Error('Failed to read file as ArrayBuffer')) } } + reader.onerror = () => reject(reader.error ?? new Error('FileReader error')) reader.readAsArrayBuffer(file) }) }src/utils/nodeDefUtil.test.ts (1)
219-226: Consider using assertion helper for consistency.This test block asserts
resultis not null but then uses optional chaining (?.) on subsequent lines. For consistency with other tests, consider using the assertion pattern or removing the optional chaining since the null check already passed.♻️ Suggested refactor for consistency
const result = mergeInputSpec(spec1, spec2) expect(result).not.toBeNull() - expect(result?.[0]).toBe('STRING') - expect(result?.[1]?.default).toBe('value2') - expect(result?.[1]?.tooltip).toBe('Tooltip 2') - expect(result?.[1]?.step).toBe(1) + expect(result![0]).toBe('STRING') + expect(result![1]?.default).toBe('value2') + expect(result![1]?.tooltip).toBe('Tooltip 2') + expect(result![1]?.step).toBe(1)Or create an assertion helper for STRING specs similar to the numeric/combo patterns.
src/scripts/ui/imagePreview.ts (1)
22-25: Potential runtime error ifimgsis empty.Line 23-24 accesses
imgs[0].naturalWidthandimgs[0].naturalHeightwithout checking if the array is non-empty. IfcalculateImageGridis called with an empty array, this will throw a TypeError.Consider adding an early return or guard:
🛡️ Proposed defensive guard
export function calculateImageGrid( imgs: ImageLike[], dw: number, dh: number ): { cellWidth: number cellHeight: number cols: number rows: number shiftX: number } { + if (imgs.length === 0) { + return { cellWidth: 0, cellHeight: 0, cols: 0, rows: 0, shiftX: 0 } + } + let best = 0 let w = imgs[0].naturalWidthsrc/components/dialog/content/MissingCoreNodesMessage.test.ts (1)
17-31: Consider simplifying the mock node creation.The
Object.assign(Object.create(null), {...})pattern creates a null-prototype object, which is unusual for mocks. Unless this is intentionally matching LiteGraph's internal object structure, a simpler approach would suffice:function createMockNode(type: string, version?: string): LGraphNode { return { type, properties: { cnr_id: 'comfy-core', ver: version }, // ... rest of properties } as Partial<LGraphNode> as LGraphNode }This would also align with the learned pattern for partial mock implementations. If the null-prototype behavior is intentional for test isolation, please add a brief comment explaining why.
src/components/common/TreeExplorerTreeNode.test.ts (1)
23-31: Use explicitPartialpattern for mock object.The mock object appears to partially implement the
RenderedTreeExplorerNodeinterface. Per repository conventions, use theas Partial<T> as Tpattern to make the incomplete implementation explicit.♻️ Apply Partial pattern
const mockNode = { key: '1', label: 'Test Node', leaf: false, totalLeaves: 3, icon: 'pi pi-folder', type: 'folder', handleRename: () => {} -} as RenderedTreeExplorerNode +} as Partial<RenderedTreeExplorerNode> as RenderedTreeExplorerNodeThis explicitly acknowledges the partial implementation while maintaining type safety. Based on learnings.
src/scripts/metadata/avif.ts (2)
278-278: Type mismatch andanyusage violate coding guidelines.Line 278 uses
Record<string, any>which violates the project guideline "Never useanytype". Additionally,parseExifDatareturnsRecord<number, string | undefined>but is assigned toRecord<string, any>, creating a type mismatch.Suggested fix
- const data: Record<string, any> = parseExifData(exifData) + const data = parseExifData(exifData)Then update the loop to handle the correctly inferred type. Since JavaScript coerces numeric keys to strings in
for...inloops, the runtime behavior is preserved, but you may want to explicitly convert the key if needed for downstream string comparisons.
328-345: Implicitundefinedreturn when length is neither 2 nor 4.The function returns
number | undefinedbut only has explicit returns forlength === 2andlength === 4. For any other length, it falls through and implicitly returnsundefined. Consider adding an explicit return or throwing an error for unsupported lengths to make the intent clear.Option: explicit return for clarity
} else if (length === 4) { return new DataView(arr.buffer, arr.byteOffset, arr.byteLength).getUint32( 0, isLittleEndian ) } + return undefined }scripts/collect-i18n-node-defs.ts (2)
55-69:getComfyWindow()is not available in the browser context—this will fail at runtime.
page.evaluate()serializes and runs the callback in the browser's JavaScript context. Functions defined in the Node.js script scope (likegetComfyWindow,isComfyWindow) are not accessible inside the evaluate callback. This code will throwReferenceError: getComfyWindow is not definedwhen executed.🐛 Proposed fix: inline the guard logic within the evaluate callback
const nodeDefs: ComfyNodeDefImpl[] = await comfyPage.page.evaluate( async () => { - const comfyWindow = getComfyWindow() - const api = comfyWindow.app.api + const win = window as Window & { app?: ComfyApp; LiteGraph?: LiteGraphGlobal } + if (!win.app || !win.LiteGraph) { + throw new Error('ComfyApp or LiteGraph not found on window') + } + const api = win.app.api const rawNodeDefs = await api.getNodeDefs() const { ComfyNodeDefImpl } = await import('../src/stores/nodeDefStore') return ( Object.values(rawNodeDefs) // Ignore DevTools nodes (used for internal testing) .filter((def: ComfyNodeDef) => !def.name.startsWith('DevTools')) .map((def: ComfyNodeDef) => new ComfyNodeDefImpl(def)) ) } )Alternatively, you can use Playwright's
exposeFunctionto make helpers available in the browser context, or simply inline the type assertion with a null check as shown above.
104-119: Same issue:getComfyWindow()is not available in this browser context.This
page.evaluate()callback also referencesgetComfyWindow(), which is defined in Node.js scope and unavailable in the browser.🐛 Proposed fix: inline the guard and type assertion
const widgetsMappings = await comfyPage.page.evaluate( (args) => { const [nodeName, displayName, inputNames] = args - const comfyWindow = getComfyWindow() - const node = comfyWindow.LiteGraph.createNode(nodeName, displayName) + const win = window as Window & { LiteGraph?: LiteGraphGlobal } + if (!win.LiteGraph) { + throw new Error('LiteGraph not found on window') + } + const node = win.LiteGraph.createNode(nodeName, displayName) if (!node?.widgets?.length) return {} return Object.fromEntries( node.widgets .filter( (w: WidgetInfo) => w?.name && !inputNames.includes(w.name) ) .map((w: WidgetInfo) => [w.name, w.label]) ) }, [nodeDef.name, nodeDef.display_name, inputNames] )src/composables/maskeditor/useCanvasManager.test.ts (1)
127-146: Minor inconsistency:imgCanvasandrgbCanvasincludestylebut implementation doesn't use it.Looking at
useCanvasManager.ts, onlymaskCanvas.styleandcanvasBackground.styleare accessed. Thestyle: {}onimgCanvasandrgbCanvasis defensive but unnecessary. Consider removing for clarity, or keep if anticipating future use.src/workbench/extensions/manager/composables/nodePack/useMissingNodes.test.ts (1)
406-423: DuplicatecreateMockNodehelper.This helper is nearly identical to the one defined at lines 324-337. Consider extracting it to a shared helper at the top of the file alongside
createMockNodeDefStoreto reduce duplication.🔧 Suggested refactor
Move the helper to the top of the file (after line 28):
+function createMockNode( + type: string, + packId?: string, + version?: string +): LGraphNode { + return { + 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 +}Then remove both nested definitions.
src/scripts/ui/dialog.ts (1)
37-44: Sanitize string HTML input with DOMPurify to prevent XSS attacks.The
innerHTMLassignment on line 39 accepts untrusted string input directly without sanitization. Per coding guidelines, all HTML handling insrc/**/*.tsmust use DOMPurify. While current callers use hardcoded strings, the method is public API and vulnerable to future misuse.Recommended fix
+import DOMPurify from 'dompurify' + show(html?: string | HTMLElement | HTMLElement[]) { if (typeof html === 'string') { - this.textElement.innerHTML = html + this.textElement.innerHTML = DOMPurify.sanitize(html) } else if (html) {browser_tests/tests/sidebar/workflows.spec.ts (1)
141-154: Early returns may silently pass the test when data is missing.Using
returnafterexpect(exportedWorkflow).toBeDefined()means ifexportedWorkflowis unexpectedlyundefined, the test will pass without running the slot assertions. This could mask issues where the workflow export fails silently.Consider using a type guard assertion that throws on failure:
💡 Suggested approach
expect(exportedWorkflow).toBeDefined() - if (!exportedWorkflow) return - const nodes = exportedWorkflow.nodes - if (!Array.isArray(nodes)) return + if (!exportedWorkflow || !Array.isArray(exportedWorkflow.nodes)) { + throw new Error('Exported workflow or nodes missing') + } + const nodes = exportedWorkflow.nodesbrowser_tests/tests/featureFlags.spec.ts (1)
334-338:as anyusage contradicts PR goals.This file uses
(window as any).__appReadinesswhich contradicts the PR's objective of removing type suppressions. Consider using the module-levelwindowdeclaration defined at lines 5-16, or extend the WindowWithMessages type to include__appReadiness.Suggested fix
Update the addInitScript to use proper typing:
await newPage.addInitScript(() => { // Track when various app components are ready - ;(window as any).__appReadiness = { + const win = window as Window & typeof globalThis & { + __appReadiness: { + featureFlagsReceived: boolean + apiInitialized: boolean + appInitialized: boolean + } + } + win.__appReadiness = { featureFlagsReceived: false, apiInitialized: false, appInitialized: false }src/lib/litegraph/src/LGraphCanvas.ts (2)
7437-7530: Boolean/toggle property editor writes"true"/"false"strings (not booleans).
At Lines 7451-7457 you pass'true' | 'false'intosetValue, butsetValue()never converts to boolean—sonode.properties[property]becomes a string. AlsoJSON.parse(String(value))(Line 7519) can throw and currently isn’t handled.Proposed fix
- function inner() { - setValue(input?.value) - } + function inner() { + if (input instanceof HTMLInputElement && input.type === 'checkbox') { + setValue(input.checked) + return + } + setValue(input?.value) + } const dirty = () => this.#dirty() - function setValue(value: string | number | undefined) { + function setValue(value: unknown) { if ( - value !== undefined && + value !== undefined && info?.values && typeof info.values === 'object' && - info.values[value] != undefined + info.values[String(value)] != undefined ) { - value = info.values[value] + value = info.values[String(value)] } + if (type === 'boolean' || type === 'toggle') { + if (typeof value === 'string') value = value === 'true' + else value = Boolean(value) + } + if (typeof node.properties[property] == 'number') { value = Number(value) } if (type == 'array' || type == 'object') { - value = JSON.parse(String(value)) + try { + value = JSON.parse(String(value)) + } catch { + // Keep dialog open; avoid mutating on invalid JSON + return + } } node.properties[property] = value if (node.graph) { node.graph._version++ } node.onPropertyChanged?.(property, value) options.onclose?.() dialog.close() dirty() }
107-121: Good type widening for search options, butas IShowSearchOptionsremains.
IShowSearchOptionsis now correctly inclusive of subgraph IO types (Lines 108-111). However Line 851 still forces a cast intoshowSearchBox, which undermines the PR goal of removing unsafe assertions (and can hide real mismatches).Suggested direction
- const linkReleaseContext = + const linkReleaseContext: IShowSearchOptions = this.linkConnector.state.connectingTo === 'input' ? { - node_from: firstLink.node as LGraphNode, - slot_from: firstLink.fromSlot as INodeOutputSlot, + node_from: firstLink.node as LGraphNode, + slot_from: firstLink.fromSlot as INodeOutputSlot, type_filter_in: firstLink.fromSlot.type } : { - node_to: firstLink.node as LGraphNode, - slot_to: firstLink.fromSlot as INodeInputSlot, + node_to: firstLink.node as LGraphNode, + slot_to: firstLink.fromSlot as INodeInputSlot, type_filter_out: firstLink.fromSlot.type } if ('shiftKey' in e && e.shiftKey) { if (this.allow_searchbox) { - this.showSearchBox(e, linkReleaseContext as IShowSearchOptions) + this.showSearchBox(e, linkReleaseContext) } }(If
firstLink.node/fromSlotaren’t reliably those types, prefer a runtime guard + early return over widening casts.)As per coding guidelines, type assertions should be a last resort.
Also applies to: 816-852
🤖 Fix all issues with AI agents
In @browser_tests/fixtures/utils/litegraphUtils.ts:
- Around line 258-264: The widget-access code in getSocketPosition() and
getValue() uses app?.graph whereas the rest of the file (including
getPosition()) uses app?.canvas?.graph; update these methods to fetch the graph
via app?.canvas?.graph, then locate the node by id (node =
canvas.graph.getNodeById(id)), validate node and node.widgets, and index into
node.widgets[index] as before so widget lookup is consistent with getPosition()
and other methods.
In @browser_tests/fixtures/utils/taskHistory.ts:
- Around line 123-139: The reduce accumulator is annotated with a verbose inline
Record type that duplicates the existing TaskOutput shape; change the reducer
initial value type to use TaskOutput instead of the inline Record so the
accumulator is typed by TaskOutput (update the filenames.reduce call and its
initializer to use TaskOutput as the cast/type for the empty object) and ensure
TaskOutput is imported/visible in taskHistory.ts so the code compiles and the
outputs variable retains the same structure.
In @browser_tests/helpers/actionbar.ts:
- Around line 43-54: The setMode method currently silently no-ops when
extMgr.queueSettings is missing; update the page.evaluate callback in setMode to
detect when extMgr.queueSettings is undefined and throw a descriptive error
(e.g., "queueSettings not initialized") so failures are explicit (or
alternatively change setMode to return a boolean indicating success); locate the
setMode function and modify the code inside page.evaluate that references
window['app'], extMgr, and queueSettings to either throw the error or return
true/false to signal success.
In @browser_tests/tests/browserTabTitle.spec.ts:
- Around line 12-19: Extract the repeated inline extensionManager type shape
into a shared helper and replace the three occurrences with calls to it: create
a function like getActiveWorkflowFilename(app: unknown) in
browser_tests/fixtures/utils (or similar), have it cast app to the shape
containing extensionManager.workflow.activeWorkflow.filename and return that
filename, then update the comfyPage.page.evaluate callback to call this helper
instead of duplicating the cast and extMgr logic (references:
comfyPage.page.evaluate, window['app'], extMgr,
workflow?.activeWorkflow?.filename).
In @browser_tests/tests/colorPalette.spec.ts:
- Around line 186-193: The test currently silently no-ops if
addCustomColorPalette is missing; inside the comfyPage.page.evaluate callback
(where you access window['app'] and extMgr via app.extensionManager and call
extMgr.colorPalette?.addCustomColorPalette?.(p) with
customColorPalettes.obsidian_dark), add an explicit runtime assertion that
addCustomColorPalette exists before calling it (e.g., throw a descriptive Error
if undefined) or assert its presence after retrieving extMgr.colorPalette to
ensure the test fails when the method is absent, so the test verifies both
existence and invocation rather than succeeding silently.
In @browser_tests/tests/groupNode.spec.ts:
- Around line 26-28: The test "Is added to node library sidebar" currently
declares an unused parameter named `_comfyPage`; remove the unused parameter or
replace it with an empty destructure (e.g., use `async ({}) =>` or simply `async
() =>`) to follow Playwright conventions and avoid misleading
underscore-prefixed names—update the test signature where it's defined to remove
`_comfyPage`.
In @browser_tests/tests/sidebar/nodeLibrary.spec.ts:
- Around line 268-275: The test currently casts the result of
comfyPage.getSetting to Record and then uses setting?.['foo/'].color which can
throw if setting['foo/'] is undefined; change assertions to consistently use
optional chaining and avoid awaiting synchronous expects: first assert the
nested property exists with expect(setting).toHaveProperty(['foo/','color']) or
assert expect(setting?.['foo/']).toBeDefined(), then use
expect(setting?.['foo/']?.color).not.toBeNull()/not.toBeUndefined()/not.toBe('')
without the unnecessary await; refer to comfyPage.getSetting, setting, and the
'foo/' key when making these edits.
In @browser_tests/tests/sidebar/workflows.spec.ts:
- Around line 183-188: The test currently does an early `return` after
`expect(downloadedContent).toBeDefined()`/`expect(downloadedContentZh).toBeDefined()`,
which can silently skip the final comparison; remove the `if (!downloadedContent
|| !downloadedContentZh) return` guard and proceed to `delete
downloadedContent.id` / `delete downloadedContentZh.id` and
`expect(downloadedContent).toEqual(downloadedContentZh)` so the equality
assertion always runs (or alternatively replace the guard with explicit failure
like `throw new Error(...)`), ensuring `downloadedContent` and
`downloadedContentZh` are asserted non-null before deleting `id` and comparing.
In @browser_tests/tests/subgraph-rename-dialog.spec.ts:
- Around line 28-53: The proposed refactor replacing the inline assertSubgraph
assertion with a generic string constant (SUBGRAPH_ASSERT_INLINE) or creating it
via new Function() breaks TypeScript's type-narrowing and will lose
context-specific assertions; keep the explicit inline assertSubgraph in the test
or, if you must refactor, extract a local helper function inside the same test
file with the same assertion signature(s) (preserving assertions for
inputs/outputs) rather than using SUBGRAPH_ASSERT_INLINE or new Function(), so
Playwright tests retain proper type safety and context-specific narrowing.
In @browser_tests/tests/subgraph.spec.ts:
- Around line 322-337: The inline verbose type on the local variable `graph`
(declared in subgraph.spec.ts) should be extracted to a shared interface to
reduce duplication; create an interface like `SubgraphGraphWithNodes` in
`subgraphUtils.ts` that extends the existing `SubgraphGraph` and adds the
optional `inputNode?: { onPointerDown?: (e: unknown, pointer: unknown,
linkConnector: unknown) => void }` signature, then import and use
`SubgraphGraphWithNodes` to type `graph` (and replace other repeated inline
annotations in this test file) so the code references the named type instead of
repeating the inline annotation.
In @browser_tests/tests/widget.spec.ts:
- Around line 110-123: The in-page guard inside comfyPage.page.evaluate silently
returns when app.graph.nodes[0].widgets[0] is missing, which can mask failures;
change the evaluate callback to throw an error with a clear message (e.g.,
"widget not found") instead of returning so the test fails visibly when
app/graph/widgets aren't initialized; keep the rest of the logic that sets
widget.callback and the subsequent widget.dragHorizontal/expect assertions
unchanged so failures are raised early and clearly (references:
comfyPage.page.evaluate, app.graph.nodes, widget.callback,
widget.dragHorizontal, window.widgetValue).
In @docs/typescript/type-safety.md:
- Around line 386-394: The "Never use `as`" rule in the summary is too absolute
for our repo's testing pattern; update the summary in type-safety.md to add a
brief exception clarifying that test files may use double-cast `as Partial<T> as
T` for constructing partial mocks (i.e., modify the bullet "Never use `as`" to
append "(exception: test files may use `as Partial<T> as T` for partial mocks)")
so contributors understand the intended testing workaround.
- Around line 204-226: Update the guidance that currently claims interfaces have
better performance than type aliases: soften the performance assertion and state
that modern TypeScript has narrowed performance differences; emphasize that
interfaces still offer clearer error messages and support declaration merging
(e.g., for shapes like NodeConfig) while recommending type aliases for unions,
primitives, and tuples. Replace the absolute phrasing with a brief note
mentioning performance differences are minimal in recent TypeScript versions and
highlight error-message clarity and declaration merging as the primary reasons
to prefer interfaces for object shapes.
In @scripts/collect-i18n-node-defs.ts:
- Line 71: The status message currently uses console.warn which signals a
warning level; change the call in collect-i18n-node-defs.ts that logs the
successful collection ("Collected ${nodeDefs.length} node definitions") to an
informational logger such as console.log or console.info so it’s not treated as
an error/CI warning—locate the console.warn invocation and replace it with
console.log (or console.info) to reflect normal successful operation.
In @src/components/common/EditableText.vue:
- Around line 78-85: The optional chaining on el.setSelectionRange is redundant
because after the HTMLInputElement check (el instanceof HTMLInputElement) the
element always has setSelectionRange; update the block around inputRef and
inputValue so that you call el.setSelectionRange(start, end) without the ?.
Ensure you keep the same logic computing fileName, start and end variables and
the early return if el is falsy or not an HTMLInputElement.
In @src/components/common/FormItem.vue:
- Around line 78-81: The current guard in FormItem.vue checks options && typeof
options[0] !== 'string' which treats an empty array as if it had a non-string
first element; update the condition to ensure options is a non-empty array
(e.g., Array.isArray(options) && options.length > 0 && typeof options[0] !==
'string') before setting attrs['optionLabel'] and attrs['optionValue'] so these
are only set when an actual option object exists.
In @src/components/common/TreeExplorerTreeNode.test.ts:
- Around line 64-65: Replace the weakly-typed bracket access to the Badge
component's prop with the type-safe props accessor: use
wrapper.findComponent(Badge).props('value') to retrieve the value (assign to
badgeValue) and then assert expect(String(badgeValue)).toBe('3'); this keeps the
removal of @ts-expect-error while improving typing and IDE support for the test.
In @src/components/dialog/content/MissingCoreNodesMessage.test.ts:
- Around line 78-80: Replace the unsafe cast of the mock with the
partial-then-full pattern: where you call
vi.mocked(useSystemStatsStore).mockReturnValue(mockSystemStatsStore as unknown
as ReturnType<typeof useSystemStatsStore>), change the cast to
mockSystemStatsStore as Partial<ReturnType<typeof useSystemStatsStore>> as
ReturnType<typeof useSystemStatsStore>; this makes the incomplete mock
implementation explicit while preserving the expected ReturnType for
useSystemStatsStore.
In @src/components/graph/GraphCanvas.vue:
- Around line 431-432: The null-check on canvasRef.value silently returns
instead of surfacing an unexpected missing canvas; update the onMounted
initialization (where canvasRef and comfyApp.setup are used) to assert or log an
error when canvasRef.value is null so we fail loudly during development — e.g.,
throw or call console.error/processLogger.error with context including the
component name and that comfyApp.setup was skipped, then only call
comfyApp.setup(canvasRef.value) when canvasRef.value is present; reference
canvasRef, comfyApp.setup and the onMounted initialization block to locate where
to add the assertion/log.
In @src/components/topbar/CurrentUserButton.vue:
- Around line 65-66: The component currently exposes the raw popover ref via
defineExpose({ popover, closePopover }); instead expose only imperative methods
(e.g., closePopover and the existing togglePopover/openPopover methods) to avoid
leaking the Popover instance: remove popover from defineExpose and add any
public action methods (togglePopover/openPopover) so external code can control
the popover without direct ref access; also update tests to call togglePopover
(or the exposed method) instead of accessing popover.toggle.
In @src/composables/maskeditor/useCanvasManager.test.ts:
- Around line 39-73: Replace the module-level mutable mockStore with a hoisted
factory using vi.hoisted: wrap creation of mockStore and the getter functions
(getImgCanvas, getMaskCanvas, getRgbCanvas, getImgCtx, getMaskCtx, getRgbCtx,
getCanvasBackground) inside vi.hoisted(() => { ... }) so the mockStore is
created per test scope and the getters close over that local store; move the
initial null-initialized mockStore object and the existing null-guarding getter
implementations into the hoisted callback and export the getters and mockStore
from that vi.hoisted result.
In @src/extensions/core/contextMenuFilter.ts:
- Around line 38-51: Add a defensive null/undefined guard around
LGraphCanvas.active_canvas before accessing .current_node in the
requestAnimationFrame callback: check that LGraphCanvas.active_canvas is truthy
(and optionally that .current_node exists) and bail early if not, so the
subsequent access to current_node, its widgets and the combo-widget matching
logic (references: LGraphCanvas.active_canvas, current_node, isComboWidget,
widgets) cannot cause a runtime null access.
In @src/extensions/core/groupNode.ts:
- Around line 669-678: The mergeIfValid call uses brittle casts
(Parameters<typeof mergeIfValid>[n]) for output, targetWidget and
primitiveConfig because widget config types are dynamic; add a concise inline
comment above the const output and the mergeIfValid invocation explaining why
the casts are required (e.g., dynamic runtime widget shape, compiler cannot
infer unions) and mark them as intentional to avoid accidental cleanup by future
maintainers, keeping the casts but clarifying their necessity and linking to the
mergeIfValid function name for context.
In @src/extensions/core/groupNodeTypes.ts:
- Around line 53-57: PartialLinkInfo currently merely aliases ILinkRouting which
provides no type-level distinction; either add a discriminating property (e.g.,
a readonly discriminant like "kind" or "__partial" on the PartialLinkInfo
interface) so TypeScript can differentiate partial/synthetic links from full
ILinkRouting, or keep it purely semantic and add a JSDoc on PartialLinkInfo
explaining the intended difference and why it aliases ILinkRouting; update
usages such as the group node getInputLink override to rely on the discriminant
if you choose the former.
In @src/extensions/core/webcamCapture.ts:
- Around line 40-45: The promise in resolveVideo is being resolved both by a
fallback setTimeout and the 'loadedmetadata' listener, so update the logic to
avoid redundant resolution: when attaching the 'loadedmetadata' handler for the
video (the listener that calls resolveVideo(video)), use a one-time listener
(e.g., pass the once option) or explicitly remove the listener after it fires,
and make the timeout handler clear the timeout if the event already fired (or
clear the timeout inside the event handler) so only one path calls resolveVideo;
reference the existing video.addEventListener('loadedmetadata', ...) and the
setTimeout(...) fallback when making this change.
In @src/extensions/core/widgetInputs.ts:
- Around line 111-117: The code assigns comboWidget.value = newValues[0] without
guarding against newValues being empty; update the logic in the block that
handles Array.isArray(rawValues) (where comboWidget.options.values is set) to
check newValues.length > 0 before assigning newValues[0]; if newValues is empty,
set comboWidget.value to a safe default (e.g., '' or null) and still invoke
comboWidget.callback with that safe value (or skip the callback if that is more
appropriate) so you never assign undefined to comboWidget.value.
In @src/lib/litegraph/src/LGraph.ts:
- Around line 705-718: The priority extraction uses '??' but the preceding
ternary always yields a number (0), so the fallback is never reached; update the
logic in computing priorityA and priorityB (variables ctorA, ctorB, A.priority,
B.priority) to nest the ternaries or otherwise chain checks so that if the
constructor has a numeric priority use it, else if the instance has a numeric
priority use that, else use 0 (i.e., remove the '??' and make the ternary return
the instance priority or 0 as the final fallback).
In @src/lib/litegraph/src/LGraphCanvas.ts:
- Around line 3682-3684: The keyboard event early-return currently only skips
when targetEl instanceof HTMLInputElement, which lets events leak when focus is
inside a textarea or contenteditable region; update the check in the keyboard
handler(s) to also return early when targetEl is an HTMLTextAreaElement or when
targetEl (or any of its ancestors) has isContentEditable === true — i.e.,
replace/extend the instanceof test on targetEl with a guard that checks
HTMLInputElement OR HTMLTextAreaElement OR walks up targetEl.parentNode to
detect isContentEditable, and apply the same fix to the other handler block
referenced around lines 3720-3732 so both keyboard handling sites use the same
focus-sensitive guard.
- Around line 165-172: checkPanels() is closing panels because newly created
panels lack panel.graph so the comparison panel.graph != this.graph evaluates
true; fix by ensuring any panel created for this canvas has its graph set—assign
panel.graph = this.graph when creating panels (in createPanel()) and/or when
showing node panels (e.g., in showShowNodePanel() after creating the panel set
panel.graph = this.graph) so checkPanels() recognizes them as belonging to this
graph.
In @src/lib/litegraph/src/LiteGraphGlobal.ts:
- Around line 838-874: The removal logic duplicates calls and returns values
incorrectly: consolidate into a single removal per event and avoid returning the
result (this is a void function). Specifically, for events in
pointerAndMouseEvents call oDOM.removeEventListener(this.pointerevents_method +
sEvent, fCall, capture) once when this.pointerevents_method is 'pointer' or
'mouse'; for events in pointerOnlyEvents call
oDOM.removeEventListener(this.pointerevents_method + sEvent, fCall, capture)
only when this.pointerevents_method is 'pointer'; otherwise call
oDOM.removeEventListener(sEvent, fCall, capture). Remove any duplicate calls and
any return statements around oDOM.removeEventListener; reference
pointerAndMouseEvents, pointerOnlyEvents, this.pointerevents_method,
oDOM.removeEventListener, sEvent, fCall, capture to locate and update the code.
In @src/lib/litegraph/src/node/NodeSlot.test.ts:
- Around line 16-27: The describe block title is misleading because it currently
reads 'outputAsSerialisable' but the tests exercise both outputAsSerialisable
and inputAsSerialisable; rename the describe to something inclusive like 'slot
serialization' or split into two describes (one for outputAsSerialisable and one
for inputAsSerialisable) so each unit under test is clearly grouped; update the
describe string and, if splitting, move the existing tests into the appropriate
new describe blocks while keeping the test bodies (they reference
outputAsSerialisable and inputAsSerialisable) unchanged.
In @src/lib/litegraph/src/subgraph/SubgraphSerialization.test.ts:
- Around line 288-301: Add a brief inline comment explaining that the deliberate
use of "as unknown as ExportedSubgraph" on the extendedFormat test object is
intentional to simulate a future schema/version and to validate deserialization
resilience; update the test near the extendedFormat declaration (the variable
named extendedFormat used when calling new Subgraph(new LGraph(),
extendedFormat)) to state that this type assertion is an acceptable exception to
normal rules for forward-compatibility testing.
In
@src/renderer/extensions/vueNodes/widgets/composables/useImageUploadWidget.ts:
- Around line 26-27: The helper findFileComboWidget currently assumes
node.widgets exists and that find() returns a value by using node.widgets! and
casting to IComboWidget; make it robust by adding null guards: inside
findFileComboWidget (and/or its callers) check that node.widgets is defined
before calling find, have findFileComboWidget return IComboWidget | undefined
(remove the unsafe cast), and at the call site where fileComboWidget is used
(e.g., where imageInputName was validated by isImageUploadInput) explicitly
check for undefined and throw a clear Error like `Widget "${imageInputName}" not
found on node` before proceeding.
In @src/scripts/metadata/avif.ts:
- Line 193: The return type on the function signature redundantly appends "|
null" to IsobmffBoxContentRange (which already includes null); update the
function declaration so the return type is simply IsobmffBoxContentRange (remove
the extra "| null") — look for the signature that ends with "):
IsobmffBoxContentRange | null {" and change it to use only
IsobmffBoxContentRange.
In @src/scripts/metadata/flac.ts:
- Around line 38-43: The reader.onload handler currently resolves with an empty
object when event.target?.result is not an ArrayBuffer; add an error log there
for consistency with the other signature-check branch: inside reader.onload
(function assigned to reader.onload) when the else branch is taken, call the
project's logger (e.g., processLogger.error or console.error) to record that
FileReader returned a non-ArrayBuffer result before calling r({}); keep using
getFromFlacBuffer and r as-is so only the unexpected-result path gains
observability.
In @src/scripts/ui/components/button.ts:
- Around line 31-39: Properties on ComfyButton (icon, overIcon, iconSize,
content, tooltip, classList, hidden, enabled, action) are declared with
definite-assignment (!) but the corresponding ComfyButtonProps are optional so
these can be undefined at runtime; update each property declaration to use
explicit union types (e.g., icon: string | undefined, overIcon: string |
undefined, iconSize: number | undefined, content: string | HTMLElement |
undefined, tooltip: string | undefined, classList: ClassList | undefined,
hidden: boolean | undefined, enabled: boolean | undefined, action: ((e: Event,
btn: ComfyButton) => void) | undefined) and remove the unnecessary ! assertions
so the prop() assignments (the prop(...) calls referenced in your comment)
reflect possible undefined values and keep existing runtime fallbacks intact.
In @src/scripts/ui/draggableList.ts:
- Around line 126-130: The code reads touch coordinates with e.touches[0]
without verifying the touch list; update the pointer extraction in the handler
that sets this.pointerStartX and this.pointerStartY to first check that
e.touches exists and e.touches.length > 0 (or e.changedTouches for certain
events) before accessing index 0, and fall back to using e.clientX/e.clientY or
abort the handler if no touch is available; update the logic around the
clientX/clientY extraction so it safely handles empty touch lists and avoids
runtime exceptions.
In @src/scripts/ui/toggleSwitch.ts:
- Around line 3-8: Export the ToggleSwitchItem interface so external consumers
can type the public API (e.g., the onChange callback) by adding the export
modifier to the existing interface declaration (ToggleSwitchItem) and update any
local references or public type re-exports if you have a barrel export so the
type is available from the package entry points; ensure consumers can import
ToggleSwitchItem where they implement the onChange handler.
- Around line 29-35: The callback currently casts items[index] to
ToggleSwitchItem unsafely because the original items array isn't normalized;
normalize the items array once before any access (e.g., create normalizedItems =
items.map(...) using the same normalization logic currently inside map) and then
use normalizedItems[index] and normalizedItems[selectedIndex] in the onChange
call (and anywhere updateSelected reads items) so the callback always receives a
proper ToggleSwitchItem instead of relying on type assertions.
In @src/scripts/widgets.ts:
- Around line 10-27: The helper type ComboValuesType and function
getComboValuesArray are defined between import blocks; move both the
ComboValuesType alias and the getComboValuesArray function so they appear after
all import statements (i.e., below the final import grouping), ensuring you do
not split the top import section—also keep INumericWidget and other type imports
together at the top with the other imports so only the helper type and function
are relocated.
In @src/types/litegraph-augmentation.d.ts:
- Around line 116-121: The call site is casting recreate as returning void and
not awaiting the new Promise-returning signature; update the caller where you
see the cast `(node as LGraphNode & { recreate?: () => void }).recreate?.()` to
cast to `() => Promise<LGraphNode | null>` and await the call (e.g. `const
newNode = await (node as LGraphNode & { recreate?: () => Promise<LGraphNode |
null> }).recreate?.()`), then handle the possibility that `newNode` is null
before proceeding. Ensure the awaited result is used or guarded appropriately to
satisfy the updated `recreate?(): Promise<LGraphNode | null>` contract.
In @src/utils/vintageClipboard.ts:
- Around line 89-95: Add a lightweight type assertion to the JSON.parse result
to give the IDE/typechecker shape info for deserialised.nodes and
deserialised.links: where deserialised is assigned (currently via
JSON.parse(data) near the graph.beforeChange() call), assert it to an
appropriate legacy shape (e.g., an object with nodes and links arrays) so
subsequent accesses get type inference; no runtime validation required since
this is deprecated handling—just change the JSON.parse assignment to include the
type assertion for deserialised.
In
@src/workbench/extensions/manager/composables/nodePack/useMissingNodes.test.ts:
- Line 289: Replace the inconsistent ref mutation using Object.assign on
workflowPacksRef with the idiomatic direct .value assignment: set
workflowPacksRef.value = mockWorkflowPacks (or [] as used elsewhere) instead of
Object.assign(workflowPacksRef, { value: mockWorkflowPacks }); if you used
Object.assign to silence a type error, adjust the ref typing so direct
assignment to workflowPacksRef.value is allowed.
- Around line 12-28: The helper createMockNodeDefStore should use the
PartialNodeDefStore type for consistency: change its signature to return
PartialNodeDefStore (instead of ReturnType<typeof useNodeDefStore>) and type the
local nodeDefsByName as PartialNodeDefStore['nodeDefsByName']; return the object
cast as PartialNodeDefStore. Keep references to PartialNodeDefStore,
createMockNodeDefStore, and useNodeDefStore so the intent and typings are clear.
In @src/workbench/utils/nodeDefOrderingUtil.ts:
- Around line 4-6: The interface HasInputOrder uses a loose input_order:
Record<string, string[]> which misses IDE/type safety for the known properties;
change it to a more specific type exposing the expected keys so accesses to
.required and .optional are typed (e.g., replace input_order?:
Record<string,string[]> with a typed shape that includes required?: string[] and
optional?: string[]), update any code using HasInputOrder to rely on those typed
properties (references: HasInputOrder, input_order, .required, .optional).
| return filenames.reduce( | ||
| (outputs, filename, i) => { | ||
| const nodeId = `${i + 1}` | ||
| outputs[nodeId] = { | ||
| [filetype]: [{ filename, subfolder: '', type: 'output' }] | ||
| } | ||
| const contentType = getContentType(filename, filetype) | ||
| this.outputContentTypes.set(filename, contentType) | ||
| return outputs | ||
| }, | ||
| {} as Record< | ||
| string, | ||
| { | ||
| [key: string]: { filename: string; subfolder: string; type: string }[] | ||
| } | ||
| > | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using TaskOutput directly as the accumulator type.
The inline type annotation duplicates the structure that's likely already defined in TaskOutput. If TaskOutput is compatible with an empty object initializer, you could simplify:
♻️ Suggested simplification
return filenames.reduce(
(outputs, filename, i) => {
const nodeId = `${i + 1}`
outputs[nodeId] = {
[filetype]: [{ filename, subfolder: '', type: 'output' }]
}
const contentType = getContentType(filename, filetype)
this.outputContentTypes.set(filename, contentType)
return outputs
},
- {} as Record<
- string,
- {
- [key: string]: { filename: string; subfolder: string; type: string }[]
- }
- >
+ {} as TaskOutput
)If TaskOutput has additional required properties that make this incompatible, the current verbose annotation is acceptable.
📝 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.
| return filenames.reduce( | |
| (outputs, filename, i) => { | |
| const nodeId = `${i + 1}` | |
| outputs[nodeId] = { | |
| [filetype]: [{ filename, subfolder: '', type: 'output' }] | |
| } | |
| const contentType = getContentType(filename, filetype) | |
| this.outputContentTypes.set(filename, contentType) | |
| return outputs | |
| }, | |
| {} as Record< | |
| string, | |
| { | |
| [key: string]: { filename: string; subfolder: string; type: string }[] | |
| } | |
| > | |
| ) | |
| return filenames.reduce( | |
| (outputs, filename, i) => { | |
| const nodeId = `${i + 1}` | |
| outputs[nodeId] = { | |
| [filetype]: [{ filename, subfolder: '', type: 'output' }] | |
| } | |
| const contentType = getContentType(filename, filetype) | |
| this.outputContentTypes.set(filename, contentType) | |
| return outputs | |
| }, | |
| {} as TaskOutput | |
| ) |
🤖 Prompt for AI Agents
In @browser_tests/fixtures/utils/taskHistory.ts around lines 123 - 139, The
reduce accumulator is annotated with a verbose inline Record type that
duplicates the existing TaskOutput shape; change the reducer initial value type
to use TaskOutput instead of the inline Record so the accumulator is typed by
TaskOutput (update the filenames.reduce call and its initializer to use
TaskOutput as the cast/type for the empty object) and ensure TaskOutput is
imported/visible in taskHistory.ts so the code compiles and the outputs variable
retains the same structure.
| public async setMode(mode: AutoQueueMode) { | ||
| await this.page.evaluate((mode) => { | ||
| window['app'].extensionManager.queueSettings.mode = mode | ||
| const app = window['app'] | ||
| if (!app) throw new Error('App not initialized') | ||
| const extMgr = app.extensionManager as { | ||
| queueSettings?: { mode: string } | ||
| } | ||
| if (extMgr.queueSettings) { | ||
| extMgr.queueSettings.mode = mode | ||
| } | ||
| }, mode) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Silent no-op when queueSettings is missing may mask test setup issues.
The guard on line 50 silently skips the assignment when queueSettings is undefined. If a test calls setMode expecting it to succeed but queueSettings doesn't exist, the test may pass incorrectly or exhibit confusing behavior.
Consider throwing an error for consistency with the app guard, or at minimum returning a boolean to indicate success:
🔧 Suggested improvement
public async setMode(mode: AutoQueueMode) {
await this.page.evaluate((mode) => {
const app = window['app']
if (!app) throw new Error('App not initialized')
const extMgr = app.extensionManager as {
queueSettings?: { mode: string }
}
- if (extMgr.queueSettings) {
- extMgr.queueSettings.mode = mode
- }
+ if (!extMgr.queueSettings) throw new Error('queueSettings not initialized')
+ extMgr.queueSettings.mode = mode
}, mode)
}🤖 Prompt for AI Agents
In @browser_tests/helpers/actionbar.ts around lines 43 - 54, The setMode method
currently silently no-ops when extMgr.queueSettings is missing; update the
page.evaluate callback in setMode to detect when extMgr.queueSettings is
undefined and throw a descriptive error (e.g., "queueSettings not initialized")
so failures are explicit (or alternatively change setMode to return a boolean
indicating success); locate the setMode function and modify the code inside
page.evaluate that references window['app'], extMgr, and queueSettings to either
throw the error or return true/false to signal success.
| const workflowName = await comfyPage.page.evaluate(async () => { | ||
| return window['app'].extensionManager.workflow.activeWorkflow.filename | ||
| const app = window['app'] | ||
| if (!app) throw new Error('App not initialized') | ||
| const extMgr = app.extensionManager as { | ||
| workflow?: { activeWorkflow?: { filename?: string } } | ||
| } | ||
| return extMgr.workflow?.activeWorkflow?.filename | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting the repeated type shape into a shared helper.
The inline type assertion for extensionManager is repeated three times in this file with slight variations. While this works correctly, consider extracting a helper function in a shared test utilities file (e.g., browser_tests/fixtures/utils/) to reduce duplication and improve maintainability.
// Example helper in browser_tests/fixtures/utils/appUtils.ts
export function getActiveWorkflowFilename(app: unknown): string | undefined {
const extMgr = (app as { extensionManager?: { workflow?: { activeWorkflow?: { filename?: string } } } }).extensionManager
return extMgr?.workflow?.activeWorkflow?.filename
}🤖 Prompt for AI Agents
In @browser_tests/tests/browserTabTitle.spec.ts around lines 12 - 19, Extract
the repeated inline extensionManager type shape into a shared helper and replace
the three occurrences with calls to it: create a function like
getActiveWorkflowFilename(app: unknown) in browser_tests/fixtures/utils (or
similar), have it cast app to the shape containing
extensionManager.workflow.activeWorkflow.filename and return that filename, then
update the comfyPage.page.evaluate callback to call this helper instead of
duplicating the cast and extMgr logic (references: comfyPage.page.evaluate,
window['app'], extMgr, workflow?.activeWorkflow?.filename).
| await comfyPage.page.evaluate((p) => { | ||
| window['app'].extensionManager.colorPalette.addCustomColorPalette(p) | ||
| const app = window['app'] | ||
| if (!app) throw new Error('App not initialized') | ||
| const extMgr = app.extensionManager as { | ||
| colorPalette?: { addCustomColorPalette?: (p: unknown) => void } | ||
| } | ||
| extMgr.colorPalette?.addCustomColorPalette?.(p) | ||
| }, customColorPalettes.obsidian_dark) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Improved type safety with explicit null guard and type narrowing.
The guarded access pattern with an explicit throw if app is undefined is cleaner and provides better error messages than silent failures. The inline type annotation for extMgr with optional chaining safely navigates the nested optional properties.
One minor consideration: if addCustomColorPalette is undefined, the test will silently succeed without actually calling the method. If the intent is to test that the method exists and works, consider adding an assertion:
💡 Optional: Add assertion for method existence
const extMgr = app.extensionManager as {
colorPalette?: { addCustomColorPalette?: (p: unknown) => void }
}
+ if (!extMgr.colorPalette?.addCustomColorPalette) {
+ throw new Error('addCustomColorPalette method not found')
+ }
extMgr.colorPalette?.addCustomColorPalette?.(p)📝 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.
| await comfyPage.page.evaluate((p) => { | |
| window['app'].extensionManager.colorPalette.addCustomColorPalette(p) | |
| const app = window['app'] | |
| if (!app) throw new Error('App not initialized') | |
| const extMgr = app.extensionManager as { | |
| colorPalette?: { addCustomColorPalette?: (p: unknown) => void } | |
| } | |
| extMgr.colorPalette?.addCustomColorPalette?.(p) | |
| }, customColorPalettes.obsidian_dark) | |
| await comfyPage.page.evaluate((p) => { | |
| const app = window['app'] | |
| if (!app) throw new Error('App not initialized') | |
| const extMgr = app.extensionManager as { | |
| colorPalette?: { addCustomColorPalette?: (p: unknown) => void } | |
| } | |
| if (!extMgr.colorPalette?.addCustomColorPalette) { | |
| throw new Error('addCustomColorPalette method not found') | |
| } | |
| extMgr.colorPalette?.addCustomColorPalette?.(p) | |
| }, customColorPalettes.obsidian_dark) |
🤖 Prompt for AI Agents
In @browser_tests/tests/colorPalette.spec.ts around lines 186 - 193, The test
currently silently no-ops if addCustomColorPalette is missing; inside the
comfyPage.page.evaluate callback (where you access window['app'] and extMgr via
app.extensionManager and call extMgr.colorPalette?.addCustomColorPalette?.(p)
with customColorPalettes.obsidian_dark), add an explicit runtime assertion that
addCustomColorPalette exists before calling it (e.g., throw a descriptive Error
if undefined) or assert its presence after retrieving extMgr.colorPalette to
ensure the test fails when the method is absent, so the test verifies both
existence and invocation rather than succeeding silently.
| test('Is added to node library sidebar', async ({ | ||
| comfyPage: _comfyPage | ||
| }) => { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using a more meaningful parameter name or removing it entirely.
The underscore-prefixed _comfyPage indicates an unused parameter. If the test doesn't use comfyPage, consider destructuring an empty object or using the Playwright pattern of just {}.
Optional: Remove unused parameter
- test('Is added to node library sidebar', async ({
- comfyPage: _comfyPage
- }) => {
+ test('Is added to node library sidebar', async () => {📝 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.
| test('Is added to node library sidebar', async ({ | |
| comfyPage: _comfyPage | |
| }) => { | |
| test('Is added to node library sidebar', async () => { |
🤖 Prompt for AI Agents
In @browser_tests/tests/groupNode.spec.ts around lines 26 - 28, The test "Is
added to node library sidebar" currently declares an unused parameter named
`_comfyPage`; remove the unused parameter or replace it with an empty
destructure (e.g., use `async ({}) =>` or simply `async () =>`) to follow
Playwright conventions and avoid misleading underscore-prefixed names—update the
test signature where it's defined to remove `_comfyPage`.
| /** | ||
| * @deprecated Widget to socket conversion is no longer necessary as they co-exist now. | ||
| * This method is a no-op stub for backward compatibility with extensions. | ||
| */ | ||
| convertWidgetToInput?(): boolean | ||
| recreate?(): Promise<LGraphNode | null> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --type=ts --type=js -C3 '\.recreate\s*\('Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
# Also check for recreate without the dot prefix (direct function calls, imports, etc.)
rg -n --type=ts --type=js -C2 'recreate' | head -100Repository: Comfy-Org/ComfyUI_frontend
Length of output: 6936
🏁 Script executed:
sed -n '575,590p' src/extensions/core/groupNodeManage.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 607
🏁 Script executed:
sed -n '1120,1135p' src/extensions/core/groupNode.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 500
Fix Promise handling and type casting in recreate() caller.
The type change to Promise<LGraphNode | null> is correct and well-documented, but there is a critical bug in src/extensions/core/groupNodeManage.ts:581 where the caller casts recreate as returning void and does not await the Promise:
;(node as LGraphNode & { recreate?: () => void }).recreate?.()
This violates the new type contract. The call must await the result and handle the null case:
await (node as LGraphNode & { recreate?: () => Promise<LGraphNode | null> }).recreate?.()
🤖 Prompt for AI Agents
In @src/types/litegraph-augmentation.d.ts around lines 116 - 121, The call site
is casting recreate as returning void and not awaiting the new Promise-returning
signature; update the caller where you see the cast `(node as LGraphNode & {
recreate?: () => void }).recreate?.()` to cast to `() => Promise<LGraphNode |
null>` and await the call (e.g. `const newNode = await (node as LGraphNode & {
recreate?: () => Promise<LGraphNode | null> }).recreate?.()`), then handle the
possibility that `newNode` is null before proceeding. Ensure the awaited result
is used or guarded appropriately to satisfy the updated `recreate?():
Promise<LGraphNode | null>` contract.
src/utils/vintageClipboard.ts
Outdated
| if (!graph) return | ||
|
|
||
| canvas.emitBeforeChange() | ||
| try { | ||
| // @ts-expect-error fixme ts strict error | ||
| graph.beforeChange() | ||
|
|
||
| const deserialised = JSON.parse(data) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good null guard addition.
The early return on line 89-90 properly prevents accessing methods on an undefined graph.
Consider adding a type assertion for the parsed data to improve type safety within the function:
- const deserialised = JSON.parse(data)
+ const deserialised = JSON.parse(data) as VintageSerialisableThis would provide type inference for deserialised.nodes and deserialised.links accesses below. Since this is a deprecated function handling legacy format, runtime validation may be overkill, but the type assertion costs nothing and improves IDE support.
📝 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.
| if (!graph) return | |
| canvas.emitBeforeChange() | |
| try { | |
| // @ts-expect-error fixme ts strict error | |
| graph.beforeChange() | |
| const deserialised = JSON.parse(data) | |
| if (!graph) return | |
| canvas.emitBeforeChange() | |
| try { | |
| graph.beforeChange() | |
| const deserialised = JSON.parse(data) as VintageSerialisable |
🤖 Prompt for AI Agents
In @src/utils/vintageClipboard.ts around lines 89 - 95, Add a lightweight type
assertion to the JSON.parse result to give the IDE/typechecker shape info for
deserialised.nodes and deserialised.links: where deserialised is assigned
(currently via JSON.parse(data) near the graph.beforeChange() call), assert it
to an appropriate legacy shape (e.g., an object with nodes and links arrays) so
subsequent accesses get type inference; no runtime validation required since
this is deprecated handling—just change the JSON.parse assignment to include the
type assertion for deserialised.
| type PartialNodeDefStore = Pick< | ||
| ReturnType<typeof useNodeDefStore>, | ||
| 'nodeDefsByName' | ||
| > | ||
| type PartialManagerStore = Pick< | ||
| ReturnType<typeof useComfyManagerStore>, | ||
| 'isPackInstalled' | ||
| > | ||
|
|
||
| function createMockNodeDefStore( | ||
| names: string[] | ||
| ): ReturnType<typeof useNodeDefStore> { | ||
| const nodeDefsByName = Object.fromEntries( | ||
| names.map((name) => [name, { name } as ComfyNodeDefImpl]) | ||
| ) | ||
| return { nodeDefsByName } as ReturnType<typeof useNodeDefStore> | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good use of Pick<> for partial store types and centralized mock factory.
The PartialNodeDefStore and PartialManagerStore types provide a clear contract for what the tests require. The createMockNodeDefStore helper centralizes mock creation, reducing duplication.
Minor note: The helper could use PartialNodeDefStore internally for consistency:
🔧 Optional refinement
function createMockNodeDefStore(
names: string[]
): ReturnType<typeof useNodeDefStore> {
const nodeDefsByName = Object.fromEntries(
names.map((name) => [name, { name } as ComfyNodeDefImpl])
)
- return { nodeDefsByName } as ReturnType<typeof useNodeDefStore>
+ const partial: PartialNodeDefStore = { nodeDefsByName }
+ return partial as ReturnType<typeof useNodeDefStore>
}📝 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.
| type PartialNodeDefStore = Pick< | |
| ReturnType<typeof useNodeDefStore>, | |
| 'nodeDefsByName' | |
| > | |
| type PartialManagerStore = Pick< | |
| ReturnType<typeof useComfyManagerStore>, | |
| 'isPackInstalled' | |
| > | |
| function createMockNodeDefStore( | |
| names: string[] | |
| ): ReturnType<typeof useNodeDefStore> { | |
| const nodeDefsByName = Object.fromEntries( | |
| names.map((name) => [name, { name } as ComfyNodeDefImpl]) | |
| ) | |
| return { nodeDefsByName } as ReturnType<typeof useNodeDefStore> | |
| } | |
| type PartialNodeDefStore = Pick< | |
| ReturnType<typeof useNodeDefStore>, | |
| 'nodeDefsByName' | |
| > | |
| type PartialManagerStore = Pick< | |
| ReturnType<typeof useComfyManagerStore>, | |
| 'isPackInstalled' | |
| > | |
| function createMockNodeDefStore( | |
| names: string[] | |
| ): ReturnType<typeof useNodeDefStore> { | |
| const nodeDefsByName = Object.fromEntries( | |
| names.map((name) => [name, { name } as ComfyNodeDefImpl]) | |
| ) | |
| const partial: PartialNodeDefStore = { nodeDefsByName } | |
| return partial as ReturnType<typeof useNodeDefStore> | |
| } |
🤖 Prompt for AI Agents
In
@src/workbench/extensions/manager/composables/nodePack/useMissingNodes.test.ts
around lines 12 - 28, The helper createMockNodeDefStore should use the
PartialNodeDefStore type for consistency: change its signature to return
PartialNodeDefStore (instead of ReturnType<typeof useNodeDefStore>) and type the
local nodeDefsByName as PartialNodeDefStore['nodeDefsByName']; return the object
cast as PartialNodeDefStore. Keep references to PartialNodeDefStore,
createMockNodeDefStore, and useNodeDefStore so the intent and typings are clear.
| // Update workflow packs | ||
| // @ts-expect-error - mockWorkflowPacks is a simplified version without full WorkflowPack interface. | ||
| workflowPacksRef.value = mockWorkflowPacks | ||
| Object.assign(workflowPacksRef, { value: mockWorkflowPacks }) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent ref mutation pattern.
This uses Object.assign to update the ref's value, but line 314 in the same describe block uses direct assignment (workflowPacksRef.value = []). The direct assignment is the idiomatic Vue pattern for updating refs.
If both approaches work, prefer consistency:
🔧 Suggested fix
- Object.assign(workflowPacksRef, { value: mockWorkflowPacks })
+ workflowPacksRef.value = mockWorkflowPacksIf this Object.assign approach was needed to resolve a type error, please clarify. Otherwise, direct assignment should be used for consistency with the rest of the file.
📝 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.
| Object.assign(workflowPacksRef, { value: mockWorkflowPacks }) | |
| workflowPacksRef.value = mockWorkflowPacks |
🤖 Prompt for AI Agents
In
@src/workbench/extensions/manager/composables/nodePack/useMissingNodes.test.ts
at line 289, Replace the inconsistent ref mutation using Object.assign on
workflowPacksRef with the idiomatic direct .value assignment: set
workflowPacksRef.value = mockWorkflowPacks (or [] as used elsewhere) instead of
Object.assign(workflowPacksRef, { value: mockWorkflowPacks }); if you used
Object.assign to silence a type error, adjust the ref typing so direct
assignment to workflowPacksRef.value is allowed.
| interface HasInputOrder { | ||
| input_order?: Record<string, string[]> | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider a more specific type for input_order.
The code at lines 28 and 38 specifically accesses .required and .optional properties. A more precise interface would provide better IDE autocomplete and catch typos at compile time:
interface HasInputOrder {
- input_order?: Record<string, string[]>
+ input_order?: {
+ required?: string[]
+ optional?: string[]
+ }
}That said, if extensibility for future input categories is intended, the current Record<string, string[]> is acceptable.
📝 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.
| interface HasInputOrder { | |
| input_order?: Record<string, string[]> | |
| } | |
| interface HasInputOrder { | |
| input_order?: { | |
| required?: string[] | |
| optional?: string[] | |
| } | |
| } |
🤖 Prompt for AI Agents
In @src/workbench/utils/nodeDefOrderingUtil.ts around lines 4 - 6, The interface
HasInputOrder uses a loose input_order: Record<string, string[]> which misses
IDE/type safety for the known properties; change it to a more specific type
exposing the expected keys so accesses to .required and .optional are typed
(e.g., replace input_order?: Record<string,string[]> with a typed shape that
includes required?: string[] and optional?: string[]), update any code using
HasInputOrder to rely on those typed properties (references: HasInputOrder,
input_order, .required, .optional).
| const app = window.app | ||
| if (!app?.graph) throw new Error('App not initialized') | ||
| const node = app.graph.getNodeById(id) | ||
| if (!node) throw new Error(`Node ${id} not found.`) | ||
| if (!node.widgets) throw new Error(`Node ${id} has no widgets.`) | ||
| const widget = node.widgets[index] | ||
| if (!widget) throw new Error(`Widget ${index} not found.`) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for graph access patterns in the litegraphUtils file
rg -n "app\.(graph|canvas\.graph)" browser_tests/fixtures/utils/litegraphUtils.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 701
🏁 Script executed:
#!/bin/bash
# Get context around line 314 to understand the other app.graph usage
sed -n '310,320p' browser_tests/fixtures/utils/litegraphUtils.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 568
🏁 Script executed:
#!/bin/bash
# Get more context to see the method names for both app.graph usages
sed -n '250,270p' browser_tests/fixtures/utils/litegraphUtils.ts
echo "---"
sed -n '305,325p' browser_tests/fixtures/utils/litegraphUtils.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 1563
🏁 Script executed:
#!/bin/bash
# Check if there are any comments or patterns suggesting why widget operations might need app.graph specifically
# Also check what methods nearby use for comparison
sed -n '185,240p' browser_tests/fixtures/utils/litegraphUtils.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 2018
Align widget access with the app?.canvas?.graph pattern used throughout the file.
The getSocketPosition() and getValue() methods use app?.graph, but the getPosition() method in the same class uses app?.canvas?.graph. All other methods in the file consistently use app?.canvas?.graph (10 occurrences vs. 2 using app?.graph). For consistency, both widget methods should use the same pattern.
🤖 Prompt for AI Agents
In @browser_tests/fixtures/utils/litegraphUtils.ts around lines 258 - 264, The
widget-access code in getSocketPosition() and getValue() uses app?.graph whereas
the rest of the file (including getPosition()) uses app?.canvas?.graph; update
these methods to fetch the graph via app?.canvas?.graph, then locate the node by
id (node = canvas.graph.getNodeById(id)), validate node and node.widgets, and
index into node.widgets[index] as before so widget lookup is consistent with
getPosition() and other methods.
| const finalizedDef = this.nodeDef as ComfyNodeDef & { | ||
| [GROUP]: GroupNodeConfig | ||
| } | ||
| await app.registerNodeDef(`${PREFIX}${SEPARATOR}` + this.name, finalizedDef) | ||
| useNodeDefStore().addNodeDef(finalizedDef) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and inspect registerNodeDef and addNodeDef implementations
rg -n -A 15 "registerNodeDef|addNodeDef" --type=ts src/scripts/app.ts src/stores/nodeDefStore.ts 2>/dev/null | head -80Repository: Comfy-Org/ComfyUI_frontend
Length of output: 5082
🏁 Script executed:
#!/bin/bash
# Find the type definitions for ComfyNodeDef, GroupNodeInputsSpec, and GroupNodeOutputType
rg -n "type ComfyNodeDef|interface ComfyNodeDef|type GroupNodeInputsSpec|type GroupNodeOutputType" --type=ts src/Repository: Comfy-Org/ComfyUI_frontend
Length of output: 482
🏁 Script executed:
#!/bin/bash
# Get context around lines 313-317 in groupNode.ts
sed -n '300,330p' src/extensions/core/groupNode.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 986
🏁 Script executed:
#!/bin/bash
# Find ComfyNodeDefV1 definition
rg -n "type ComfyNodeDefV1|interface ComfyNodeDefV1" --type=ts src/Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Find GroupNodeConfig definition
rg -n "type GroupNodeConfig|interface GroupNodeConfig" --type=ts src/Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Find registerNodeDef implementation in app.ts
rg -n -A 15 "registerNodeDef\s*\(" --type=ts src/scripts/app.ts | head -60Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1608
🏁 Script executed:
#!/bin/bash
# Check the type of this.nodeDef in groupNode.ts
sed -n '1,100p' src/extensions/core/groupNode.ts | rg -n "nodeDef|class|constructor"Repository: Comfy-Org/ComfyUI_frontend
Length of output: 202
🏁 Script executed:
#!/bin/bash
# Find where ComfyNodeDefV1 is defined or imported
rg -n "ComfyNodeDefV1" --type=ts src/ | head -20Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1806
🏁 Script executed:
#!/bin/bash
# Check the top of groupNode.ts for imports and class definition
head -80 src/extensions/core/groupNode.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 2583
🏁 Script executed:
#!/bin/bash
# Search for GroupNodeConfig in the entire src directory
rg -n "GroupNodeConfig" --type=ts src/ | head -20Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1937
🏁 Script executed:
#!/bin/bash
# Look at the ComfyNodeDef schema definitions more closely
cat src/schemas/nodeDefSchema.ts | head -100Repository: Comfy-Org/ComfyUI_frontend
Length of output: 3407
🏁 Script executed:
#!/bin/bash
# Check GroupNodeConfig class definition to see how this.nodeDef is created/assigned
sed -n '234,350p' src/extensions/core/groupNode.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 3624
🏁 Script executed:
#!/bin/bash
# Check how this.nodeDef is initialized in GroupNodeConfig
rg -n -B 5 -A 10 "this\.nodeDef\s*=" src/extensions/core/groupNode.ts | head -80Repository: Comfy-Org/ComfyUI_frontend
Length of output: 643
Verify that downstream consumers properly handle the custom input and output shapes.
The cast to ComfyNodeDef & { [GROUP]: GroupNodeConfig } masks a type incompatibility. This nodeDef is actually typed with Omit<ComfyNodeDef, 'input' | 'output'> & { input: GroupNodeInputsSpec; output: GroupNodeOutputType[] } (lines 246–256), which uses custom input and output shapes rather than the standard ComfyNodeDef structures. When passed to registerNodeDef and addNodeDef, which expect standard ComfyNodeDefV1 shapes, ensure these consumers handle the custom input/output formats correctly or refactor to avoid the unsafe type cast.
| const output = { widget: primitiveConfig } as unknown as Parameters< | ||
| typeof mergeIfValid | ||
| >[0] | ||
| const config = mergeIfValid( | ||
| // @ts-expect-error slot type mismatch - legacy API | ||
| output, | ||
| targetWidget, | ||
| targetWidget as Parameters<typeof mergeIfValid>[1], | ||
| false, | ||
| undefined, | ||
| primitiveConfig | ||
| primitiveConfig as Parameters<typeof mergeIfValid>[4] | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Type casts in mergeIfValid calls are necessary but fragile.
The casts to Parameters<typeof mergeIfValid>[n] are needed due to the dynamic nature of the widget configuration. Consider adding a brief comment explaining why these casts are necessary to aid future maintainers.
🤖 Prompt for AI Agents
In @src/extensions/core/groupNode.ts around lines 669 - 678, The mergeIfValid
call uses brittle casts (Parameters<typeof mergeIfValid>[n]) for output,
targetWidget and primitiveConfig because widget config types are dynamic; add a
concise inline comment above the const output and the mergeIfValid invocation
explaining why the casts are required (e.g., dynamic runtime widget shape,
compiler cannot infer unions) and mark them as intentional to avoid accidental
cleanup by future maintainers, keeping the casts but clarifying their necessity
and linking to the mergeIfValid function name for context.
src/extensions/core/webcamCapture.ts
Outdated
| setTimeout(() => resolveVideo(video), 500) // Fallback as loadedmetadata doesnt fire sometimes? | ||
| video.addEventListener( | ||
| 'loadedmetadata', | ||
| () => resolveVideo(video), | ||
| false | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Potential double-resolution of promise.
Both the timeout (line 40) and the loadedmetadata event listener (lines 41-45) call resolveVideo(video). While calling resolve multiple times on a Promise is harmless (subsequent calls are ignored), this could be cleaner with a flag or by removing the listener after resolution.
Optional: Prevent redundant resolution
+ let resolved = false
setTimeout(() => resolveVideo(video), 500) // Fallback as loadedmetadata doesnt fire sometimes?
video.addEventListener(
'loadedmetadata',
- () => resolveVideo(video),
+ () => {
+ if (!resolved) {
+ resolved = true
+ resolveVideo(video)
+ }
+ },
false
)📝 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.
| setTimeout(() => resolveVideo(video), 500) // Fallback as loadedmetadata doesnt fire sometimes? | |
| video.addEventListener( | |
| 'loadedmetadata', | |
| () => resolveVideo(video), | |
| false | |
| ) | |
| let resolved = false | |
| setTimeout(() => resolveVideo(video), 500) // Fallback as loadedmetadata doesnt fire sometimes? | |
| video.addEventListener( | |
| 'loadedmetadata', | |
| () => { | |
| if (!resolved) { | |
| resolved = true | |
| resolveVideo(video) | |
| } | |
| }, | |
| false | |
| ) |
🤖 Prompt for AI Agents
In @src/extensions/core/webcamCapture.ts around lines 40 - 45, The promise in
resolveVideo is being resolved both by a fallback setTimeout and the
'loadedmetadata' listener, so update the logic to avoid redundant resolution:
when attaching the 'loadedmetadata' handler for the video (the listener that
calls resolveVideo(video)), use a one-time listener (e.g., pass the once option)
or explicitly remove the listener after it fires, and make the timeout handler
clear the timeout if the event already fired (or clear the timeout inside the
event handler) so only one path calls resolveVideo; reference the existing
video.addEventListener('loadedmetadata', ...) and the setTimeout(...) fallback
when making this change.
| const ctorA = A.constructor | ||
| const ctorB = B.constructor | ||
| const priorityA = | ||
| ('priority' in ctorA && typeof ctorA.priority === 'number' | ||
| ? ctorA.priority | ||
| : 0) ?? | ||
| ('priority' in A && typeof A.priority === 'number' ? A.priority : 0) | ||
| const priorityB = | ||
| ('priority' in ctorB && typeof ctorB.priority === 'number' | ||
| ? ctorB.priority | ||
| : 0) ?? | ||
| ('priority' in B && typeof B.priority === 'number' ? B.priority : 0) | ||
| // if same priority, sort by order | ||
|
|
||
| return Ap == Bp ? A.order - B.order : Ap - Bp | ||
| return priorityA == priorityB ? A.order - B.order : priorityA - priorityB |
There was a problem hiding this comment.
Priority extraction logic has a fallback precedence issue.
The nullish coalescing (??) will never reach the fallback because the 'in' check with a ternary always returns a number (0 as default). The ?? is effectively dead code.
Suggested fix
- const priorityA =
- ('priority' in ctorA && typeof ctorA.priority === 'number'
- ? ctorA.priority
- : 0) ??
- ('priority' in A && typeof A.priority === 'number' ? A.priority : 0)
- const priorityB =
- ('priority' in ctorB && typeof ctorB.priority === 'number'
- ? ctorB.priority
- : 0) ??
- ('priority' in B && typeof B.priority === 'number' ? B.priority : 0)
+ const priorityA =
+ 'priority' in ctorA && typeof ctorA.priority === 'number'
+ ? ctorA.priority
+ : 'priority' in A && typeof A.priority === 'number'
+ ? A.priority
+ : 0
+ const priorityB =
+ 'priority' in ctorB && typeof ctorB.priority === 'number'
+ ? ctorB.priority
+ : 'priority' in B && typeof B.priority === 'number'
+ ? B.priority
+ : 0📝 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 ctorA = A.constructor | |
| const ctorB = B.constructor | |
| const priorityA = | |
| ('priority' in ctorA && typeof ctorA.priority === 'number' | |
| ? ctorA.priority | |
| : 0) ?? | |
| ('priority' in A && typeof A.priority === 'number' ? A.priority : 0) | |
| const priorityB = | |
| ('priority' in ctorB && typeof ctorB.priority === 'number' | |
| ? ctorB.priority | |
| : 0) ?? | |
| ('priority' in B && typeof B.priority === 'number' ? B.priority : 0) | |
| // if same priority, sort by order | |
| return Ap == Bp ? A.order - B.order : Ap - Bp | |
| return priorityA == priorityB ? A.order - B.order : priorityA - priorityB | |
| const ctorA = A.constructor | |
| const ctorB = B.constructor | |
| const priorityA = | |
| 'priority' in ctorA && typeof ctorA.priority === 'number' | |
| ? ctorA.priority | |
| : 'priority' in A && typeof A.priority === 'number' | |
| ? A.priority | |
| : 0 | |
| const priorityB = | |
| 'priority' in ctorB && typeof ctorB.priority === 'number' | |
| ? ctorB.priority | |
| : 'priority' in B && typeof B.priority === 'number' | |
| ? B.priority | |
| : 0 | |
| // if same priority, sort by order | |
| return priorityA == priorityB ? A.order - B.order : priorityA - priorityB |
🤖 Prompt for AI Agents
In @src/lib/litegraph/src/LGraph.ts around lines 705 - 718, The priority
extraction uses '??' but the preceding ternary always yields a number (0), so
the fallback is never reached; update the logic in computing priorityA and
priorityB (variables ctorA, ctorB, A.priority, B.priority) to nest the ternaries
or otherwise chain checks so that if the constructor has a numeric priority use
it, else if the instance has a numeric priority use that, else use 0 (i.e.,
remove the '??' and make the ternary return the instance priority or 0 as the
final fallback).
| interface IPanel extends Element, ICloseable { | ||
| node?: LGraphNode | ||
| graph?: LGraph | ||
| } | ||
|
|
||
| function isPanel(el: Element): el is IPanel { | ||
| return 'close' in el && typeof el.close === 'function' | ||
| } |
There was a problem hiding this comment.
checkPanels() will close panels immediately unless panel.graph is set.
At Line 8040, panel.graph != this.graph will be true for panels created via createPanel() unless you assign panel.graph somewhere (currently showShowNodePanel() sets panel.node but not panel.graph). This looks like a real runtime regression (panel instantly closes on the next checkPanels() call).
Proposed fix
-interface IPanel extends Element, ICloseable {
+interface IPanel extends Element, ICloseable {
node?: LGraphNode
- graph?: LGraph
+ graph?: LGraph | Subgraph
}
function isPanel(el: Element): el is IPanel {
return 'close' in el && typeof el.close === 'function'
} showShowNodePanel(node: LGraphNode): void {
this.SELECTED_NODE = node
this.closePanels()
const ref_window = this.getCanvasWindow()
const panel = this.createPanel(node.title || '', {
closable: true,
window: ref_window,
onOpen: () => {
this.NODEPANEL_IS_OPEN = true
},
onClose: () => {
this.NODEPANEL_IS_OPEN = false
this.node_panel = null
}
})
this.node_panel = panel
panel.id = 'node-panel'
panel.node = node
+ panel.graph = this.graph ?? undefined
panel.classList.add('settings')Also applies to: 8031-8042
🤖 Prompt for AI Agents
In @src/lib/litegraph/src/LGraphCanvas.ts around lines 165 - 172, checkPanels()
is closing panels because newly created panels lack panel.graph so the
comparison panel.graph != this.graph evaluates true; fix by ensuring any panel
created for this canvas has its graph set—assign panel.graph = this.graph when
creating panels (in createPanel()) and/or when showing node panels (e.g., in
showShowNodePanel() after creating the panel set panel.graph = this.graph) so
checkPanels() recognizes them as belonging to this graph.
| const targetEl = e.target | ||
| if (targetEl instanceof HTMLInputElement) return | ||
|
|
There was a problem hiding this comment.
Keyboard handling ignores <textarea> / contenteditable, causing shortcut leakage.
Line 3683 returns early only for HTMLInputElement. This means space/esc/copy/etc can still be handled by the canvas while focus is in a textarea (e.g., prompt multiline) or a contenteditable field (e.g., panel inline edits), which is a UX regression risk.
Proposed fix
- const targetEl = e.target
- if (targetEl instanceof HTMLInputElement) return
+ const targetEl = e.target
+ if (
+ targetEl instanceof HTMLInputElement ||
+ targetEl instanceof HTMLTextAreaElement ||
+ (targetEl instanceof HTMLElement && targetEl.isContentEditable) ||
+ targetEl instanceof HTMLSelectElement
+ )
+ returnAlso applies to: 3720-3732
🤖 Prompt for AI Agents
In @src/lib/litegraph/src/LGraphCanvas.ts around lines 3682 - 3684, The keyboard
event early-return currently only skips when targetEl instanceof
HTMLInputElement, which lets events leak when focus is inside a textarea or
contenteditable region; update the check in the keyboard handler(s) to also
return early when targetEl is an HTMLTextAreaElement or when targetEl (or any of
its ancestors) has isContentEditable === true — i.e., replace/extend the
instanceof test on targetEl with a guard that checks HTMLInputElement OR
HTMLTextAreaElement OR walks up targetEl.parentNode to detect isContentEditable,
and apply the same fix to the other handler block referenced around lines
3720-3732 so both keyboard handling sites use the same focus-sensitive guard.
2 participants
Summary
Comprehensive removal of
@ts-expect-errorsuppressions across the codebase, replacing them with proper type guards, null checks, and type annotations.Before: 528
@ts-expect-errorsuppressions insrc/After: 1 remaining (PrimeVue theme type issue in
src/main.ts)Changes by Area
GroupNode & Subgraphs
@ts-expect-errorsuppressions from groupNode.tsSubgraphGraphtype andassertSubgraph()utility for browser testsLiteGraph & Widgets
@ts-expect-errorsuppressions in widget-related filesLGraphNode,LGraphCanvas, andLiteGraphtypes with missing propertiesapp,LiteGraph,LGraphBadgeServices & Utils
litegraphService.tsPlatform & Renderer
Components & Composables
Browser Tests
browser_tests/types/global.d.tsextending Window interface with test properties(window as unknown as Record<...>)['prop']casts with direct accessCode Quality Improvements
WorkflowAsGraphtype alias documenting ComfyWorkflowJSON → ISerialisedGraph castdomWidget.tsclone pattern usinggetCloneArgs()factory methodComponentWidgetImplwhich was missing proper cloning of component-specific propertiesAGENTS.mdFiles Changed
180 files changed, 4,168 insertions, 22,247 deletions
Key new files:
src/types/workflowSchemaTypes.ts- WorkflowAsGraph type aliasbrowser_tests/types/global.d.ts- Window interface extensions for testsbrowser_tests/fixtures/utils/subgraphUtils.ts- Subgraph type guardsdocs/typescript/type-safety.md- Type safety guidelinesRemaining Suppressions
src/main.tsAura['primitive'].bluetype issue (external library).storybook/preview.tsapps/desktop-ui/.storybook/preview.tsTest Plan
pnpm typecheckpassespnpm lintpasses