chore: enable 69 oxlint rules across 4 phases

[DrJKL]

Summary

Enable 69 new oxlint rules across 4 phases, bringing the linter config from ~15 rules to ~84 active rules with zero warnings and zero errors.

Changes

• What: Phased rollout of oxlint rules organized by risk/effort:
  • Phase 1 (30 rules): Auto-fixable, low-risk rules (unicorn, eslint, typescript, vitest)
  • Phase 2 (15 rules): Correctness rules with few expected violations
  • Phase 3 (16 rules): Consistency/convention rules, medium effort
  • Phase 4 (8 rules): High-effort/opinionated rules
• Added promise plugin to oxlint
• 7 high-violation rules disabled with TODO comments for incremental cleanup:
  • no-param-reassign (104), prefer-destructuring (581)
  • promise/prefer-await-to-callbacks (76), promise/prefer-await-to-then (91)
  • unicorn/consistent-function-scoping (147), unicorn/no-array-for-each (165)
  • typescript/prefer-nullish-coalescing (372)
• 4 rules deferred as not applicable (unicorn/no-null, no-await-in-loop, sort-keys, sort-imports)
• Config upgraded to JSONC (oxlint supports it) for TODO comments

Review Focus

• Inline disables in litegraph code (no-this-assignment, prefer-spread, no-useless-call) — these are genuine needs due to hoisted function declarations and class instance semantics
• eqeqeq fixes involving NodeId (number|string) comparisons — used String() coercion
• no-return-assign fixes converting arrow expression bodies to statement bodies
• unicorn/prefer-add-event-listener fix in litegraphService.ts introduced const variable for TS narrowing

┆Issue is synchronized with this Notion page by Unito

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

This PR performs extensive code modernization across the codebase, including: ESLint configuration expansion with new rules (promise, unicorn, TypeScript, Vue), systematic refactoring of Vue component prop access from prop objects to destructured variables, standardization of string operations (Array.from→spread, template literals, replaceAll), strict equality enforcement throughout, error handling improvements with consistent variable naming, and test infrastructure updates with function references in describe blocks.

Changes

Cohort / File(s)	Summary
ESLint & Linting Configuration .oxlintrc.json, eslint.config.ts, pnpm-workspace.yaml, scripts/cicd/check-json.sh	Added promise plugin, expanded rule set (unicorn, TypeScript, Vue, vitest), updated oxlint/oxfmt versions, excluded .oxlintrc.json from JSON linting, and added vue/return-in-computed-property rule.
Vue Component Props Destructuring src/components/**/[A-Z]*.vue (100+ files)	Systematic refactoring from const props = defineProps<...>() to const { prop1, prop2 } = defineProps<...>(), affecting template bindings and script logic across breadcrumb, common, dialog, graph, queue, sidebar, topbar, and other component directories.
String Operations Modernization src/**/*.ts, src/**/*.vue, src/lib/litegraph/**/*	Replaced Array.from() with spread syntax, string concatenation with template literals, substring() with slice(), replace() with replaceAll(), and getElementById() with querySelector() across composables, components, and litegraph modules.
Equality Operator Strictness src/lib/litegraph/**/*.ts, src/core/**/*.ts, src/composables/**/*.ts	Comprehensive conversion from loose equality (==, !=) to strict equality (===, !==) in conditionals, comparisons, and guards throughout litegraph core (LGraph.ts, LGraphCanvas.ts, LGraphNode.ts, etc.), core graph utilities, and composables.
Error Handling & Exception Standardization src/**/*.ts, src/**/*.vue	Renamed catch parameters from e/err to error/errorCaught, replaced bare string throws with Error objects, and added generic catch blocks; affects composables (billing, graph, maskeditor, node, queue), components, and core utilities.
Test Infrastructure Updates src/**/*.test.ts (150+ files)	Changed describe blocks from string literals to function/component references (e.g., describe(ComponentName, ...) or describe(functionName, ...)), reordered imports for consistency, and updated test suite naming conventions.
Minor Refactorings & Code Cleanup src/lib/litegraph/**/*.ts, src/components/**/*.vue, src/composables/**/*.ts	Removed explicit undefined returns, refactored conditional structures, changed Object.prototype.hasOwnProperty.call to Object.hasOwn, replaced Array operations with modern syntax, and adjusted control flow for readability without altering behavior.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• Chore: Typescript cleanup (1 / N) #7817: Overlapping refactoring of litegraph core files (LGraphCanvas, LGraph, LGraphNode) and corresponding test suite updates with similar strictness and modernization patterns.

Suggested reviewers

• Myestery
• pythongosssss
• viva-jinyi
• ltdrdata
• christian-byrne
• jtydhr88

Poem

🐰 Hops through the code with joy and care,
Props now destructured everywhere!
Strict equality, modern ways,
Carrot-filled refactoring days! 🥕✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 24.42% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: enabling 69 oxlint rules in 4 phases. It accurately reflects the primary objective of the PR.
Description check	✅ Passed	The description follows the template structure with Summary, Changes, and Review Focus sections. It thoroughly explains the phased rollout, rule additions, and key considerations. All required information is present and well-organized.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch drjkl/I-have-a-ruler-and-I-know-how-to-use-it

---

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

[claude]

[quality] critical Priority

Issue: Removed nullish coalescing with empty array causes runtime error (duplicate of line 365)
Context: When data.inputs is undefined, data.inputs?.map() returns undefined. Passing undefined to the Map constructor will throw: "Iterator value undefined is not an entry object"
Suggestion: Restore the ?? [] fallback: new Map(data.inputs?.map((input) => [input.name, input]) ?? [])

[claude]

Comprehensive PR Review

This review is generated by Claude. It may not always be accurate, as with human reviewers. If you believe that any of the comments are invalid or incorrect, please state why for each. For others, please implement the changes in one way or another.

Review Summary

PR: chore: enable 69 oxlint rules across 4 phases (#8722)
Impact: 2488 additions, 2283 deletions across 411 files

Issue Distribution

• Critical: 3
• High: 0
• Medium: 1
• Low: 0

Category Breakdown

• Architecture: 0 issues
• Security: 0 issues
• Performance: 0 issues
• Code Quality: 4 issues

Key Findings

Critical Runtime Bugs

Found 3 instances where removing ?? [] nullish coalescing causes runtime errors:

1. src/services/litegraphService.ts:365 - new Map(data.inputs?.map(...)) will throw when data.inputs is undefined
2. src/services/litegraphService.ts:473 - Same issue, duplicate location
3. src/stores/menuItemStore.ts:94 - new Set(extension.commands?.map(...)) will throw when extension.commands is undefined

When the optional chaining ?. evaluates to undefined, the map() is never called and returns undefined. Passing undefined to Map/Set constructors throws: "Iterator value undefined is not an entry object". The ?? [] fallback was necessary and must be restored.

Code Quality

String() Coercion for NodeId Comparisons: Throughout the PR, NodeId (number | string union type) comparisons use String() coercion to normalize values. While this works, it could be more type-safe and explicit. Consider creating a helper function for NodeId comparisons or documenting this pattern.

Positive Observations

1. Excellent phased approach: Breaking the linting rules into 4 phases (auto-fixable, correctness, consistency, high-effort) is well-organized and makes the PR reviewable
2. Good use of inline disables: The oxlint-disable comments in litegraph code are appropriately justified (e.g., unicorn/no-this-assignment where inner functions need access to both their own and outer this)
3. addEventListener improvements: Converting from onload/onerror to addEventListener is a good improvement that removes @ts-expect-error comments
4. Performance improvements: Converting arrays to Sets for recommendedNodes and recommendedWidgetNames improves lookup performance from O(n) to O(1)
5. Function declaration consistency: Converting function expressions to function declarations (const fApplyMultiNode = function → function fApplyMultiNode) improves code consistency
6. Error handling: Proper use of throw new Error() instead of throwing string literals
7. Vue 3.5 prop destructuring: Utilizing reactive prop destructuring to eliminate props.x access is modern and clean

Testing Recommendations

Given the critical bugs found:

1. Priority: Test scenarios where data.inputs or extension.commands could be undefined
2. Run the existing test suite to catch any runtime errors
3. Consider adding integration tests for the litegraph service input handling
4. Test extension loading with extensions that have no commands

Next Steps

1. CRITICAL: Fix the 3 Map/Set constructor bugs before merge - these will cause runtime crashes
2. Consider the NodeId comparison approach for consistency
3. Verify all tests pass after fixes
4. This PR is otherwise well-executed and improves code quality significantly

---

This is a comprehensive automated review. For architectural decisions requiring human judgment, please request additional manual review.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@src/platform/updates/common/releaseStore.ts`:
- Around line 281-285: The catch block in releaseStore.ts sets a hard-coded
fallback string 'Unknown error occurred' to error.value; replace this literal
with a localized message using vue-i18n (e.g., call t('errors.unknown') or the
existing i18n helper used in this module) so the code assigns error.value =
errorCaught instanceof Error ? errorCaught.message : t('errors.unknown'); then
add the key "errors": { "unknown": "Unknown error occurred" } to
src/locales/en/main.json; ensure the file imports/has access to the t() function
the same way other src/*.ts files do (e.g., via useI18n() or the module’s i18n
helper) and run type-checks.

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/components/rightSidePanel/parameters/SectionWidgets.vue (1)

72-85: ⚠️ Potential issue | 🟡 Minor

Normalize proxy nodeId types to preserve prior matching behavior.
With the move from == to === on Line 76, any string/number mismatch will now fail. If widget._overlay.nodeId or nodeId can be numeric, consider coercing to string as you already do on Line 84.

💡 Suggested adjustment

-    return proxyWidgets.some(
-      ([nodeId, widgetName]) =>
-        widget._overlay.nodeId === nodeId &&
-        widget._overlay.widgetName === widgetName
-    )
+    return proxyWidgets.some(
+      ([nodeId, widgetName]) =>
+        String(widget._overlay.nodeId) === String(nodeId) &&
+        widget._overlay.widgetName === widgetName
+    )

src/lib/litegraph/src/LGraphCanvas.ts (1)

1645-1683: ⚠️ Potential issue | 🟡 Minor

Use i18n for the new “No color” label.

This label is user-facing; please route it through the project’s i18n helper and add an entry in src/locales/en/main.json.

As per coding guidelines: "Use vue-i18n in Composition API for any string literals. Place new translation entries in src/locales/en/main.json".

🤖 Fix all issues with AI agents

In `@src/components/common/UserCredit.test.ts`:
- Line 42: The test suite passes the Vue component object to describe(), causing
the suite label to render as [object Object]; change the describe call to use a
stable string by deriving the component name (e.g., use UserCredit.__name ??
UserCredit.name ?? 'UserCredit') so the suite label is readable and
stable—update the describe(UserCredit, () => { ... }) invocation to
describe(UserCredit.__name ?? UserCredit.name ?? 'UserCredit', () => { ... })
while leaving the rest of the test body unchanged.

In `@src/components/sidebar/tabs/AssetsSidebarListView.test.ts`:
- Around line 58-60: The mock for getOutputAssetMetadata currently returns an
empty object but the real function's contract is OutputAssetMetadata | null;
update the vi.mock implementation so getOutputAssetMetadata returns null (e.g.,
getOutputAssetMetadata: () => null) to exercise the null/absent-metadata code
paths and match the actual return type used by the component under test.

🧹 Nitpick comments (4)

src/components/bottomPanel/tabs/terminal/BaseTerminal.test.ts (1)

90-107: Consider using a function declaration instead of arrow function.

Per coding guidelines, prefer function declarations over function expressions when possible.

♻️ Suggested refactor

-const mountBaseTerminal = () => {
-  return mount(BaseTerminal, {
+function mountBaseTerminal() {
+  return mount(BaseTerminal, {
     global: {
       plugins: [
         createTestingPinia({
           createSpy: vi.fn
         }),
         i18n
       ],
       stubs: {
         Button: {
           template: '<button v-bind="$attrs"><slot /></button>',
           props: ['icon', 'severity', 'size']
         }
       }
     }
-  })
-}
+  })
+}

src/composables/useCachedRequest.test.ts (1)

206-214: Consider avoiding the lint suppression by destructuring call.
This keeps the test clean without oxlint-disable noise and avoids the false positive.

♻️ Suggested change

-    // oxlint-disable-next-line no-useless-call -- false positive: call is an object method, not Function.prototype.call
-    const result = await cachedRequest.call(null)
+    const { call } = cachedRequest
+    const result = await call(null)
     expect(result).toBeNull()
     expect(mockRequestFn).toHaveBeenCalledTimes(1)

     // Make the same request again
-    // oxlint-disable-next-line no-useless-call -- false positive: call is an object method, not Function.prototype.call
-    const result2 = await cachedRequest.call(null)
+    const result2 = await call(null)

src/components/sidebar/tabs/queue/ResultGallery.test.ts (1)

81-100: Type mountGallery props and use a function declaration.

This avoids implicit/loose typing for test helpers and aligns with the repo’s preference for function declarations and component-prop typing.

Proposed update

 import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
 import { createApp, nextTick } from 'vue'
+import type { ComponentProps } from 'vue-component-type-helpers'
@@
-  const mountGallery = (props = {}) => {
-    return mount(ResultGallery, {
+  function mountGallery(
+    props: Partial<ComponentProps<typeof ResultGallery>> = {}
+  ) {
+    return mount(ResultGallery, {
@@
-      props: {
-        allGalleryItems: mockGalleryItems as ResultItemImpl[],
-        activeIndex: 0,
-        ...props
-      },
+      props: {
+        allGalleryItems: mockGalleryItems as ResultItemImpl[],
+        activeIndex: 0,
+        ...props
+      } as ComponentProps<typeof ResultGallery>,
       attachTo: document.querySelector('#app') || undefined
     })
   }

As per coding guidelines, “Derive component types using vue-component-type-helpers (ComponentProps, ComponentSlots) instead of separate type files” and “Do not use function expressions if it's possible to use function declarations instead.” Based on learnings, “In test files, for test helpers like mountComponent, type the props parameter as Partial<ComponentProps>… cast as ComponentProps.”

src/lib/litegraph/src/LGraphNode.ts (1)

1159-1162: Return undefined instead of null when the slot is missing.

This aligns the API with the LiteGraph guideline and makes call sites more consistent.

♻️ Suggested update

-   * `@returns` data or if it is not connected returns null
+   * `@returns` data or if it is not connected returns undefined
  */
  getInputDataByName(slot_name: string, force_update: boolean): unknown {
    const slot = this.findInputSlot(slot_name)
-   return slot === -1 ? null : this.getInputData(slot, force_update)
+   return slot === -1 ? undefined : this.getInputData(slot, force_update)
  }

Based on learnings: Prefer returning undefined over null in TypeScript/JavaScript.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, examine the test file around lines 58-60
cat -n src/components/sidebar/tabs/AssetsSidebarListView.test.ts | head -80

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 2438

---

🏁 Script executed:

# Find and examine the real assetMetadataSchema implementation
fd -a 'assetMetadataSchema\.(ts|mts|tsx)$' src

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 139

---

🏁 Script executed:

# Search for getOutputAssetMetadata function definition
rg -n 'getOutputAssetMetadata\s*[:(]' src --type ts -B 2 -A 5

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 7343

---

🏁 Script executed:

# Check the rest of assetMetadataSchema implementation to see what it returns
cat -n src/platform/assets/schemas/assetMetadataSchema.ts

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 1542

---

Change mock return value to null (Line 59).
The real getOutputAssetMetadata returns OutputAssetMetadata | null, returning null when metadata is absent. The current mock returns {}, which exercises only the "metadata present" path. Use getOutputAssetMetadata: () => null to match the actual contract and ensure null-handling code paths are tested.

🤖 Prompt for AI Agents

In `@src/components/sidebar/tabs/AssetsSidebarListView.test.ts` around lines 58 -
60, The mock for getOutputAssetMetadata currently returns an empty object but
the real function's contract is OutputAssetMetadata | null; update the vi.mock
implementation so getOutputAssetMetadata returns null (e.g.,
getOutputAssetMetadata: () => null) to exercise the null/absent-metadata code
paths and match the actual return type used by the component under test.

[AustinMroz]

From the first quarter, will continue later

[AustinMroz]

Please move the library import to the library import group 🥺

[AustinMroz]

Is the linter also forcing that props which are never used outside the template be defined? This looks awful.

[AustinMroz]

This one feels awful. Can we swap to a ternary and remove all those brackets?

[AustinMroz]

Super Nit: I would rather we force == undefined, but this appears to be a linter limitation

[AustinMroz]

This also makes me unhappy, but I'll allow it.

[AustinMroz]

This should not be mixed in with the library imports

[DrJKL]

https://github.com/un-ts/eslint-plugin-import-x/blob/HEAD/docs/rules/order.md#import-xorder

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/lib/litegraph/src/LGraphNode.ts (1)

2594-2603: ⚠️ Potential issue | 🟠 Major

Handle string '0' wildcard slot types in the strict-equality check.

With strict equality, a slot type serialized as '0' no longer matches the wildcard branch, so wildcard-to-typed fallback can be skipped and connections may fail. Normalize slotType (or include '0') before the check.

💡 Suggested fix

-    if (
-      opts.wildcardToTyped &&
-      (slotType === 0 || slotType === '*' || slotType === '')
-    ) {
+    const isWildcard =
+      slotType === 0 || slotType === '0' || slotType === '*' || slotType === ''
+    if (opts.wildcardToTyped && isWildcard) {

🤖 Fix all issues with AI agents

In `@src/lib/litegraph/src/LGraphNode.ts`:
- Around line 941-942: The current serialization check uses a truthy guard that
skips empty-string titles, breaking round‑trip of intentionally blank titles;
change the condition in LGraphNode.serialize (the lines comparing this.title and
this.constructor.title) to only compare equality (e.g., if (this.title !==
this.constructor.title) o.title = this.title) so that empty strings are
preserved while still omitting default constructor titles.

🧹 Nitpick comments (1)

src/lib/litegraph/src/LGraphNode.ts (1)

1163-1166: Prefer undefined over null for "no data" returns.

These helpers return null for missing data, but litegraph guidelines prefer undefined. Consider switching the return value and updating the JSDoc/return types accordingly.

💡 Example fix (one call site)

-    return slot === -1 ? null : this.getInputData(slot, force_update)
+    return slot === -1 ? undefined : this.getInputData(slot, force_update)

As per coding guidelines: Prefer returning undefined over null in TypeScript/JavaScript.

Also applies to: 1292-1299

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Preserve intentionally blank titles during serialization.

The new truthy guard skips persisting '', which changes behavior if an empty title is a valid state. Consider comparing only against constructor.title to keep empty strings round‑trippable.

💡 Suggested fix

-    if (this.title && this.title !== this.constructor.title)
+    if (this.title !== this.constructor.title)
       o.title = this.title

📝 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.

Suggested change

	if (this.title && this.title !== this.constructor.title)
	o.title = this.title
	if (this.title !== this.constructor.title)
	o.title = this.title

🤖 Prompt for AI Agents

In `@src/lib/litegraph/src/LGraphNode.ts` around lines 941 - 942, The current
serialization check uses a truthy guard that skips empty-string titles, breaking
round‑trip of intentionally blank titles; change the condition in
LGraphNode.serialize (the lines comparing this.title and this.constructor.title)
to only compare equality (e.g., if (this.title !== this.constructor.title)
o.title = this.title) so that empty strings are preserved while still omitting
default constructor titles.