refactor: eliminate unsafe type assertions from Group 2 test files #8258
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reduced 'as unknown as' instances from ~107 to 6 (94% reduction) across 18 test files in graph components and composables. Created centralized mock utilities: - src/utils/__tests__/litegraphTestUtils.ts with 7 factory functions - createMockLGraphNode(), createMockPositionable(), createMockLGraphGroup() - createMockSubgraphNode(), createMockCanvas(), createMockLGraphCanvas() - createPiniaStoreProps() for consistent store mocking Type safety improvements: - Object.assign pattern to avoid constructor conflicts - Union types (Partial<T> | Record<string, unknown>) for flexible mocking - 8 test files now use centralized utilities Remaining 6 'as unknown as' assertions are required for incomplete Pinia store mocks where providing complete $state properties is impractical. Results: - ✅ 0 typecheck errors (down from 14) - ✅ 0 lint errors - ✅ 118/118 tests passing Files modified: - src/components/graph/SelectionToolbox.test.ts - src/components/graph/selectionToolbox/BypassButton.test.ts - src/components/graph/selectionToolbox/ColorPickerButton.test.ts - src/components/graph/selectionToolbox/ExecuteButton.test.ts - src/components/sidebar/tabs/queue/ResultGallery.test.ts - src/composables/canvas/useSelectedLiteGraphItems.test.ts - src/composables/graph/useGraphHierarchy.test.ts - src/composables/graph/useSelectionState.test.ts - src/composables/maskeditor/useCanvasHistory.test.ts - src/composables/maskeditor/useCanvasManager.test.ts - src/composables/maskeditor/useCanvasTransform.test.ts - src/composables/node/useNodePricing.test.ts - src/composables/node/useWatchWidget.test.ts - src/composables/useCoreCommands.test.ts New file: - src/utils/__tests__/litegraphTestUtils.ts
Myestery
requested review from
a team,
christian-byrne and
comfyui-wiki
as code owners
dosubot
bot
added
the
size:XXL
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 01/23/2026, 02:47:50 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Tests:
|
Contributor
📝 WalkthroughWalkthroughRefactors tests and adds centralized litegraph test utilities; tightens and standardizes mock typings across many test suites; updates several test-only mocks to concrete signatures; refactors useChainCallback generic typing and adjusts widgetInputs typings/signature usage. Changes
Possibly related PRs
Suggested reviewers
✨ Finishing touches
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 |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 22.7 kB (baseline 22.7 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 949 kB (baseline 949 kB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 80.7 kB (baseline 80.7 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed Panels & Settings — 440 kB (baseline 440 kB) • 🟢 -8 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed User & Accounts — 3.94 kB (baseline 3.94 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 3 added / 3 removed Editors & Dialogs — 2.83 kB (baseline 2.83 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 2 added / 2 removed UI Components — 33.7 kB (baseline 33.7 kB) • ⚪ 0 BReusable component library chunks
Status: 5 added / 5 removed Data & Services — 3.17 MB (baseline 3.17 MB) • 🔴 +44 BStores, services, APIs, and repositories
Status: 8 added / 8 removed Utilities & Hooks — 24 kB (baseline 24 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 7 added / 7 removed Vendor & Third-Party — 10.7 MB (baseline 10.7 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 6.36 MB (baseline 6.36 MB) • 🟢 -192 BBundles that do not match a named category
Status: 30 added / 30 removed |
🔧 Auto-fixes AppliedThis PR has been automatically updated to fix linting and formatting issues.
Changes made:
|
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/composables/graph/useSelectionState.test.ts (1)
91-160: Replaceas unknown aswith explicitPartial<T> as Tcasts in store mocks.All four store mocks in this block use
as unknown as ReturnType<...>which obscures their partial nature. UsePartial<ReturnType<...>> as ReturnType<...>instead to make the incomplete implementation explicit while maintaining type safety.♻️ Suggested update (pattern)
- } as unknown as ReturnType<typeof useCanvasStore>) + } as Partial<ReturnType<typeof useCanvasStore>> as ReturnType< + typeof useCanvasStore + >)Apply the same change to the
useNodeDefStore,useSidebarTabStore, anduseNodeHelpStoremocks on lines 123, 139, and 160.src/composables/queue/useJobMenu.test.ts (1)
146-162: UsePartial<TaskItemImpl>for MockTaskRef to make incomplete mock implementation explicit.
Record<string, unknown>obscures the actual type contract. Since TaskItemImpl is imported and available, usingPartial<TaskItemImpl>with the double-cast pattern (as Partial<TaskItemImpl> as TaskItemImpl | undefined) explicitly acknowledges the mock's incomplete implementation while maintaining type safety, consistent with test-file typing guidance.♻️ Suggested refactor
-type MockTaskRef = Record<string, unknown> +type MockTaskRef = Partial<TaskItemImpl> @@ - taskRef: overrides.taskRef as TaskItemImpl | undefined, + taskRef: overrides.taskRef as Partial<TaskItemImpl> as TaskItemImpl | undefined,
🤖 Fix all issues with AI agents
In `@src/components/graph/SelectionToolbox.test.ts`:
- Around line 14-21: The test helper createMockCanvas currently casts to the
invalid type useCanvasStore.prototype.canvas (pinia composables have no
prototype); fix by removing that incorrect prototype reference and cast to a
real type or no type: either omit the annotation entirely, or import/use the
proper type like ReturnType<typeof useCanvasStore>['canvas'] (or the explicit
Canvas type if exported) and cast the returned object to that type; update
createMockCanvas to use the correct type reference (or no annotation) so the
mock's type is valid.
- Around line 23-31: The mock in createMockExtensionService implements only part
of the useExtensionService return shape; change its cast to make that explicit
by casting the object as Partial<ReturnType<typeof useExtensionService>> and
then as ReturnType<typeof useExtensionService> (i.e., use "as
Partial<ReturnType<typeof useExtensionService>> as ReturnType<typeof
useExtensionService>") so the incomplete mock is explicit while preserving the
original expected type; update the return statement in
createMockExtensionService accordingly.
In `@src/components/graph/selectionToolbox/BypassButton.test.ts`:
- Line 15: The test defines mockLGraphNode at module scope using
createMockLGraphNode which can leak state between tests; change to generate a
fresh mock per test by either moving the call into a beforeEach or introduce a
factory function (e.g., getMockLGraphNode) and call getMockLGraphNode() inside
each test that needs it so tests don't share/mutate the same mockLGraphNode
instance.
In `@src/components/graph/selectionToolbox/ColorPickerButton.test.ts`:
- Around line 16-25: The mock returned by createMockWorkflow only supplies
changeTracker but not other required fields of LoadedComfyWorkflow; update the
return value to use an explicit two-step cast so the incomplete object is first
typed as Partial<LoadedComfyWorkflow> and then cast to LoadedComfyWorkflow
(i.e., construct the object with changeTracker and overrides, cast it as
Partial<LoadedComfyWorkflow>, then cast that result to LoadedComfyWorkflow) so
the test intent is explicit while keeping the function signature
createMockWorkflow and the changeTracker mock unchanged.
In `@src/components/graph/selectionToolbox/ExecuteButton.test.ts`:
- Around line 47-49: The MockCommandStore type falsely promises a full store by
intersecting ReturnType<typeof useCommandStore> with { execute: Mock } while the
mock only implements execute; replace it by typing the mock as
Partial<ReturnType<typeof useCommandStore>> (e.g. const mock = { execute:
jest.fn() } as Partial<ReturnType<typeof useCommandStore>> as ReturnType<typeof
useCommandStore>) so missing properties are explicit, update usages of
MockCommandStore to use that pattern, and remove the MockCommandStore alias if
it’s no longer needed; ensure execute remains a Mock and all tests compile.
In `@src/components/sidebar/tabs/queue/ResultGallery.test.ts`:
- Line 95: The test currently casts mockGalleryItems directly to
ResultItemImpl[]; change this to the explicit double-cast pattern to indicate
the items are partials by casting mockGalleryItems as Partial<ResultItemImpl>[]
as ResultItemImpl[] and update the assignment at allGalleryItems to use that
double-cast so the incomplete implementation is explicit (reference:
allGalleryItems and ResultItemImpl in ResultGallery.test.ts).
In `@src/composables/functional/useChainCallback.ts`:
- Around line 8-13: The exported const useChainCallback likely should be a
function declaration per guidelines; verify whether the arrow was deliberately
chosen for its lexical this behavior for the chaining pattern—if not, change the
exported const useChainCallback to a named function declaration preserving the
same generic signature and parameters (originalCallback and callbacks) and
update any imports/usages accordingly; if the arrow is intentional, add a short
code comment above useChainCallback stating that the arrow function is required
for lexical this semantics to avoid automated refactors.
- Around line 17-24: The declared return type Ret is misleading because the
returned wrapper never returns the originalCallback's result; update the wrapper
to reflect side-effect-only behavior by changing its return type to void (remove
or stop using the Ret generic) so the function signature and the cast at the end
become (this: O, ...args: Args) => void; ensure you update any type parameters
or uses in useChainCallback that reference Ret and keep the implementation
calling originalCallback and callbacks as-is (referencing originalCallback and
callbacks in the returned function).
In `@src/composables/graph/useGraphHierarchy.test.ts`:
- Around line 37-56: The mock currently uses an "unknown" escape hatch; instead
declare mockCanvasStore with an explicit Partial type and avoid the double
cast—e.g. const mockCanvasStore: Partial<ReturnType<typeof useCanvasStore>> = {
canvas: { graph: { groups: mockGroups } }, $id: 'canvas', $state: {}, $patch:
vi.fn(), $reset: vi.fn(), $subscribe: vi.fn(), $onAction: vi.fn(), $dispose:
vi.fn(), _customProperties: new Set(), _p: {} }; then pass it into the mocked
factory with a single, deliberate cast when needed:
vi.mocked(useCanvasStore).mockReturnValue(mockCanvasStore as ReturnType<typeof
useCanvasStore>); this preserves type intent for mockCanvasStore and removes the
"unknown" escape hatch while keeping useCanvasStore mockable.
In `@src/composables/maskeditor/useImageLoader.test.ts`:
- Around line 117-133: The tests use verbose inline casts for mockStore members;
define a proper mock type/interface for your test (e.g., MockStore with
maskCanvas: {width:number;height:number}, rgbCanvas, imgCtx: {clearRect:
jest.Mock}, maskCtx: {clearRect: jest.Mock}, etc.) or cast mockStore once to
that type near the top of the test, then simplify assertions in the
useImageLoader/loadImages test to use direct property access like
mockStore.maskCanvas!.width and mockStore.imgCtx!.clearRect
toHaveBeenCalledWith(0,0,0,0) instead of repeated inline casts.
- Around line 10-30: Replace the ad-hoc unknown-typed mocks with explicit mock
interfaces/types: declare a MockStore type (with imgCanvas: HTMLCanvasElement |
null, maskCanvas, rgbCanvas, imgCtx: CanvasRenderingContext2D | null, maskCtx,
image: HTMLImageElement | null) and a MockDataStore type (with inputData typed
to the real input shape or any|null), then change mockStore and mockDataStore to
use Partial<MockStore>/Partial<MockDataStore> or those concrete types; update
the test setup in beforeEach that assigns canvases/contexts/images to match the
new types and remove the inline casts in the assertions that reference mockStore
and mockDataStore (the symbols to change: mockStore, mockDataStore, and the test
setup in beforeEach).
In `@src/composables/queue/useJobMenu.test.ts`:
- Around line 104-106: Replace the ad-hoc mock typing for nodeDefStoreMock with
the real ComfyNodeDefImpl to keep types aligned: import ComfyNodeDefImpl from
the store module and change nodeDefStoreMock.nodeDefsByName's type to
Record<string, Partial<ComfyNodeDefImpl>> so the mock can be partial while
preserving the actual shape; update any places creating entries to match
Partial<ComfyNodeDefImpl> fields used in tests.
In `@src/composables/useBrowserTabTitle.test.ts`:
- Around line 42-44: The test's reactive mock `settingStore` includes a
redundant type assertion "'Enabled' as string"; remove the unnecessary "as
string" so the `get` mock returns the string literal directly (keep the vi.fn
signature unchanged) — update the `settingStore` definition in
useBrowserTabTitle.test.ts where `get: vi.fn((_key: string) => 'Enabled' as
string)` appears to `get: vi.fn((_key: string) => 'Enabled')`.
♻️ Duplicate comments (1)
src/composables/maskeditor/useImageLoader.test.ts (1)
205-208: Same verbose cast pattern as above.These casts follow the same pattern and would benefit from the mock interface refactoring suggested earlier.
Comment on lines
+14
to
+21
| function createMockCanvas(): typeof useCanvasStore.prototype.canvas { | ||
| return { | ||
| setDirty: vi.fn(), | ||
| state: { | ||
| selectionChanged: false | ||
| } | ||
| } as typeof useCanvasStore.prototype.canvas | ||
| } |
Contributor
There was a problem hiding this comment.
Incorrect type reference: useCanvasStore.prototype.canvas is invalid.
useCanvasStore is a Pinia store composable function, not a class, so it doesn't have a prototype property. This cast is misleading and may not provide the intended type safety.
🛠️ Suggested fix
-function createMockCanvas(): typeof useCanvasStore.prototype.canvas {
- return {
- setDirty: vi.fn(),
- state: {
- selectionChanged: false
- }
- } as typeof useCanvasStore.prototype.canvas
-}
+function createMockCanvas() {
+ return {
+ setDirty: vi.fn(),
+ state: {
+ selectionChanged: false
+ }
+ } as Partial<ReturnType<typeof useCanvasStore>['canvas']> as ReturnType<typeof useCanvasStore>['canvas']
+}Or simply return the object without a type annotation since it's only used within this test file.
🤖 Prompt for AI Agents
In `@src/components/graph/SelectionToolbox.test.ts` around lines 14 - 21, The test
helper createMockCanvas currently casts to the invalid type
useCanvasStore.prototype.canvas (pinia composables have no prototype); fix by
removing that incorrect prototype reference and cast to a real type or no type:
either omit the annotation entirely, or import/use the proper type like
ReturnType<typeof useCanvasStore>['canvas'] (or the explicit Canvas type if
exported) and cast the returned object to that type; update createMockCanvas to
use the correct type reference (or no annotation) so the mock's type is valid.
Comment on lines
+23
to
+31
| function createMockExtensionService(): ReturnType<typeof useExtensionService> { | ||
| return { | ||
| extensionCommands: { value: new Map() }, | ||
| loadExtensions: vi.fn(), | ||
| registerExtension: vi.fn(), | ||
| invokeExtensions: vi.fn(() => []), | ||
| invokeExtensionsAsync: vi.fn() | ||
| } as ReturnType<typeof useExtensionService> | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using explicit Partial cast for incomplete mock.
The mock only implements a subset of useExtensionService return type. Based on learnings, prefer as Partial<ReturnType<typeof useExtensionService>> as ReturnType<typeof useExtensionService> to make the incomplete implementation explicit.
♻️ Suggested pattern
function createMockExtensionService(): ReturnType<typeof useExtensionService> {
return {
extensionCommands: { value: new Map() },
loadExtensions: vi.fn(),
registerExtension: vi.fn(),
invokeExtensions: vi.fn(() => []),
invokeExtensionsAsync: vi.fn()
- } as ReturnType<typeof useExtensionService>
+ } as Partial<ReturnType<typeof useExtensionService>> as ReturnType<typeof useExtensionService>
}📝 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
| function createMockExtensionService(): ReturnType<typeof useExtensionService> { | |
| return { | |
| extensionCommands: { value: new Map() }, | |
| loadExtensions: vi.fn(), | |
| registerExtension: vi.fn(), | |
| invokeExtensions: vi.fn(() => []), | |
| invokeExtensionsAsync: vi.fn() | |
| } as ReturnType<typeof useExtensionService> | |
| } | |
| function createMockExtensionService(): ReturnType<typeof useExtensionService> { | |
| return { | |
| extensionCommands: { value: new Map() }, | |
| loadExtensions: vi.fn(), | |
| registerExtension: vi.fn(), | |
| invokeExtensions: vi.fn(() => []), | |
| invokeExtensionsAsync: vi.fn() | |
| } as Partial<ReturnType<typeof useExtensionService>> as ReturnType<typeof useExtensionService> | |
| } |
🤖 Prompt for AI Agents
In `@src/components/graph/SelectionToolbox.test.ts` around lines 23 - 31, The mock
in createMockExtensionService implements only part of the useExtensionService
return shape; change its cast to make that explicit by casting the object as
Partial<ReturnType<typeof useExtensionService>> and then as ReturnType<typeof
useExtensionService> (i.e., use "as Partial<ReturnType<typeof
useExtensionService>> as ReturnType<typeof useExtensionService>") so the
incomplete mock is explicit while preserving the original expected type; update
the return statement in createMockExtensionService accordingly.
| title: 'Test Node', | ||
| mode: LGraphEventMode.ALWAYS | ||
| } | ||
| const mockLGraphNode = createMockLGraphNode({ type: 'TestNode' }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Module-level mock may affect test isolation.
mockLGraphNode is created once at module scope (line 15) and reused across all tests. If any test mutates this object, it could affect subsequent tests. Consider creating the mock in beforeEach or using a factory function call within each test.
♻️ Suggested improvement
-const mockLGraphNode = createMockLGraphNode({ type: 'TestNode' })
+function getMockLGraphNode() {
+ return createMockLGraphNode({ type: 'TestNode' })
+}Then call getMockLGraphNode() in each test that needs it.
🤖 Prompt for AI Agents
In `@src/components/graph/selectionToolbox/BypassButton.test.ts` at line 15, The
test defines mockLGraphNode at module scope using createMockLGraphNode which can
leak state between tests; change to generate a fresh mock per test by either
moving the call into a beforeEach or introduce a factory function (e.g.,
getMockLGraphNode) and call getMockLGraphNode() inside each test that needs it
so tests don't share/mutate the same mockLGraphNode instance.
Comment on lines
+16
to
+25
| function createMockWorkflow( | ||
| overrides: Partial<LoadedComfyWorkflow> = {} | ||
| ): LoadedComfyWorkflow { | ||
| return { | ||
| changeTracker: { | ||
| checkState: vi.fn() as Mock | ||
| }, | ||
| ...overrides | ||
| } as LoadedComfyWorkflow | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Apply explicit Partial cast for incomplete workflow mock.
The mock only provides changeTracker but LoadedComfyWorkflow has additional required fields like isLoaded, originalContent, content, initialState, and activeState. Based on learnings, use the two-step cast to make the incomplete implementation explicit.
♻️ Suggested fix
function createMockWorkflow(
overrides: Partial<LoadedComfyWorkflow> = {}
): LoadedComfyWorkflow {
return {
changeTracker: {
checkState: vi.fn() as Mock
},
...overrides
- } as LoadedComfyWorkflow
+ } as Partial<LoadedComfyWorkflow> as LoadedComfyWorkflow
}🤖 Prompt for AI Agents
In `@src/components/graph/selectionToolbox/ColorPickerButton.test.ts` around lines
16 - 25, The mock returned by createMockWorkflow only supplies changeTracker but
not other required fields of LoadedComfyWorkflow; update the return value to use
an explicit two-step cast so the incomplete object is first typed as
Partial<LoadedComfyWorkflow> and then cast to LoadedComfyWorkflow (i.e.,
construct the object with changeTracker and overrides, cast it as
Partial<LoadedComfyWorkflow>, then cast that result to LoadedComfyWorkflow) so
the test intent is explicit while keeping the function signature
createMockWorkflow and the changeTracker mock unchanged.
Comment on lines
+47
to
+49
| type MockCommandStore = ReturnType<typeof useCommandStore> & { | ||
| execute: Mock | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider aligning MockCommandStore with the full store type.
MockCommandStore intersects ReturnType<typeof useCommandStore> with { execute: Mock }, but the mock object at line 97-99 only provides execute. This works because TypeScript allows the cast, but it may cause issues if tests later access other useCommandStore properties.
Based on learnings, prefer as Partial<ReturnType<typeof useCommandStore>> as ReturnType<typeof useCommandStore> pattern to make the incomplete implementation explicit.
♻️ Suggested pattern
- mockCommandStore = {
- execute: vi.fn()
- } as MockCommandStore
+ mockCommandStore = {
+ execute: vi.fn()
+ } as Partial<ReturnType<typeof useCommandStore>> as ReturnType<typeof useCommandStore>Then remove the MockCommandStore type alias if no longer needed.
🤖 Prompt for AI Agents
In `@src/components/graph/selectionToolbox/ExecuteButton.test.ts` around lines 47
- 49, The MockCommandStore type falsely promises a full store by intersecting
ReturnType<typeof useCommandStore> with { execute: Mock } while the mock only
implements execute; replace it by typing the mock as Partial<ReturnType<typeof
useCommandStore>> (e.g. const mock = { execute: jest.fn() } as
Partial<ReturnType<typeof useCommandStore>> as ReturnType<typeof
useCommandStore>) so missing properties are explicit, update usages of
MockCommandStore to use that pattern, and remove the MockCommandStore alias if
it’s no longer needed; ensure execute remains a Mock and all tests compile.
Comment on lines
+10
to
30
| const mockStore: { | ||
| imgCanvas: unknown | ||
| maskCanvas: unknown | ||
| rgbCanvas: unknown | ||
| imgCtx: unknown | ||
| maskCtx: unknown | ||
| image: unknown | ||
| } = { | ||
| imgCanvas: null, | ||
| maskCanvas: null, | ||
| rgbCanvas: null, | ||
| imgCtx: null, | ||
| maskCtx: null, | ||
| image: null | ||
| } | ||
|
|
||
| const mockDataStore = { | ||
| inputData: null as any | ||
| const mockDataStore: { | ||
| inputData: unknown | ||
| } = { | ||
| inputData: null | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider defining explicit mock interfaces to reduce inline casts.
Using unknown for all properties is type-safe but requires verbose inline casts at every assertion site. Define the mock shape explicitly to match what's assigned in beforeEach:
♻️ Suggested refactor
-const mockStore: {
- imgCanvas: unknown
- maskCanvas: unknown
- rgbCanvas: unknown
- imgCtx: unknown
- maskCtx: unknown
- image: unknown
-} = {
+interface MockCanvas {
+ width: number
+ height: number
+}
+
+interface MockContext {
+ clearRect: ReturnType<typeof vi.fn>
+}
+
+const mockStore: {
+ imgCanvas: MockCanvas | null
+ maskCanvas: MockCanvas | null
+ rgbCanvas: MockCanvas | null
+ imgCtx: MockContext | null
+ maskCtx: MockContext | null
+ image: HTMLImageElement | null
+} = {
imgCanvas: null,
maskCanvas: null,
rgbCanvas: null,
imgCtx: null,
maskCtx: null,
image: null
}
-const mockDataStore: {
- inputData: unknown
-} = {
+const mockDataStore: {
+ inputData: {
+ baseLayer: { image: HTMLImageElement } | null
+ maskLayer: { image: HTMLImageElement } | null
+ paintLayer: { image: HTMLImageElement } | null
+ } | null
+} = {
inputData: null
}This eliminates the need for inline casts in assertions (lines 117-120, 128-133, 205-208) and makes the mock structure self-documenting. Based on learnings, explicit partial types are preferred over unknown with inline casts.
📝 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
| const mockStore: { | |
| imgCanvas: unknown | |
| maskCanvas: unknown | |
| rgbCanvas: unknown | |
| imgCtx: unknown | |
| maskCtx: unknown | |
| image: unknown | |
| } = { | |
| imgCanvas: null, | |
| maskCanvas: null, | |
| rgbCanvas: null, | |
| imgCtx: null, | |
| maskCtx: null, | |
| image: null | |
| } | |
| const mockDataStore = { | |
| inputData: null as any | |
| const mockDataStore: { | |
| inputData: unknown | |
| } = { | |
| inputData: null | |
| } | |
| interface MockCanvas { | |
| width: number | |
| height: number | |
| } | |
| interface MockContext { | |
| clearRect: ReturnType<typeof vi.fn> | |
| } | |
| const mockStore: { | |
| imgCanvas: MockCanvas | null | |
| maskCanvas: MockCanvas | null | |
| rgbCanvas: MockCanvas | null | |
| imgCtx: MockContext | null | |
| maskCtx: MockContext | null | |
| image: HTMLImageElement | null | |
| } = { | |
| imgCanvas: null, | |
| maskCanvas: null, | |
| rgbCanvas: null, | |
| imgCtx: null, | |
| maskCtx: null, | |
| image: null | |
| } | |
| const mockDataStore: { | |
| inputData: { | |
| baseLayer: { image: HTMLImageElement } | null | |
| maskLayer: { image: HTMLImageElement } | null | |
| paintLayer: { image: HTMLImageElement } | null | |
| } | null | |
| } = { | |
| inputData: null | |
| } |
🤖 Prompt for AI Agents
In `@src/composables/maskeditor/useImageLoader.test.ts` around lines 10 - 30,
Replace the ad-hoc unknown-typed mocks with explicit mock interfaces/types:
declare a MockStore type (with imgCanvas: HTMLCanvasElement | null, maskCanvas,
rgbCanvas, imgCtx: CanvasRenderingContext2D | null, maskCtx, image:
HTMLImageElement | null) and a MockDataStore type (with inputData typed to the
real input shape or any|null), then change mockStore and mockDataStore to use
Partial<MockStore>/Partial<MockDataStore> or those concrete types; update the
test setup in beforeEach that assigns canvases/contexts/images to match the new
types and remove the inline casts in the assertions that reference mockStore and
mockDataStore (the symbols to change: mockStore, mockDataStore, and the test
setup in beforeEach).
Comment on lines
+117
to
+133
| expect((mockStore.maskCanvas as { width: number }).width).toBe(512) | ||
| expect((mockStore.maskCanvas as { height: number }).height).toBe(512) | ||
| expect((mockStore.rgbCanvas as { width: number }).width).toBe(512) | ||
| expect((mockStore.rgbCanvas as { height: number }).height).toBe(512) | ||
| }) | ||
|
|
||
| it('should clear canvas contexts', async () => { | ||
| const loader = useImageLoader() | ||
|
|
||
| await loader.loadImages() | ||
|
|
||
| expect(mockStore.imgCtx.clearRect).toHaveBeenCalledWith(0, 0, 0, 0) | ||
| expect(mockStore.maskCtx.clearRect).toHaveBeenCalledWith(0, 0, 0, 0) | ||
| expect( | ||
| (mockStore.imgCtx as { clearRect: ReturnType<typeof vi.fn> }).clearRect | ||
| ).toHaveBeenCalledWith(0, 0, 0, 0) | ||
| expect( | ||
| (mockStore.maskCtx as { clearRect: ReturnType<typeof vi.fn> }).clearRect | ||
| ).toHaveBeenCalledWith(0, 0, 0, 0) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Verbose inline casts could be eliminated with proper mock types.
The repeated inline casts like (mockStore.maskCanvas as { width: number }).width and (mockStore.imgCtx as { clearRect: ReturnType<typeof vi.fn> }).clearRect are functional but verbose. If the mock interfaces are properly defined as suggested above, these assertions simplify to:
expect(mockStore.maskCanvas!.width).toBe(512)
expect(mockStore.imgCtx!.clearRect).toHaveBeenCalledWith(0, 0, 0, 0)🤖 Prompt for AI Agents
In `@src/composables/maskeditor/useImageLoader.test.ts` around lines 117 - 133,
The tests use verbose inline casts for mockStore members; define a proper mock
type/interface for your test (e.g., MockStore with maskCanvas:
{width:number;height:number}, rgbCanvas, imgCtx: {clearRect: jest.Mock},
maskCtx: {clearRect: jest.Mock}, etc.) or cast mockStore once to that type near
the top of the test, then simplify assertions in the useImageLoader/loadImages
test to use direct property access like mockStore.maskCanvas!.width and
mockStore.imgCtx!.clearRect toHaveBeenCalledWith(0,0,0,0) instead of repeated
inline casts.
Comment on lines
104
to
106
| const nodeDefStoreMock = { | ||
| nodeDefsByName: {} as Record<string, any> | ||
| nodeDefsByName: {} as Record<string, { id: string }> | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and inspect the nodeDefStore to understand the real type of nodeDefsByName
find src/stores -name "*nodeDefStore*" -type fRepository: Comfy-Org/ComfyUI_frontend
Length of output: 128
🏁 Script executed:
#!/bin/bash
# Also check the test file to understand the context
cat -n src/composables/queue/useJobMenu.test.ts | sed -n '95,120p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 832
🏁 Script executed:
#!/bin/bash
# Search for nodeDefsByName usage patterns in the codebase
rg -n "nodeDefsByName" src -A 2 -B 2 --max-count=20Repository: Comfy-Org/ComfyUI_frontend
Length of output: 22588
🏁 Script executed:
#!/bin/bash
# Inspect the nodeDefStore to see ComfyNodeDefImpl and the return type
cat -n src/stores/nodeDefStore.ts | head -100Repository: Comfy-Org/ComfyUI_frontend
Length of output: 4068
🏁 Script executed:
#!/bin/bash
# Check the full return type definition of useNodeDefStore
cat -n src/stores/nodeDefStore.ts | sed -n '440,460p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 596
🏁 Script executed:
#!/bin/bash
# Search for ComfyNodeDefImpl definition
rg -n "class ComfyNodeDefImpl|export.*ComfyNodeDefImpl" src/stores -A 5Repository: Comfy-Org/ComfyUI_frontend
Length of output: 872
🏁 Script executed:
#!/bin/bash
# Verify the type derivation approach would work
cat -n src/stores/nodeDefStore.ts | sed -n '300,305p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 389
🏁 Script executed:
#!/bin/bash
# Check how nodeDefsByName is used in the useJobMenu.ts to understand what properties are actually needed
cat -n src/composables/queue/useJobMenu.ts | sed -n '135,145p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 440
🏁 Script executed:
#!/bin/bash
# Check if ComfyNodeDefImpl is already imported anywhere in test files
rg -n "import.*ComfyNodeDefImpl" src/composables/queue/Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Check what the ref property actually resolves to in the returned store type
cat -n src/stores/nodeDefStore.ts | sed -n '301,303p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 293
🏁 Script executed:
#!/bin/bash
# Verify how other similar mocks in the codebase handle the type
cat -n src/stores/modelToNodeStore.test.ts | sed -n '70,85p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 686
🏁 Script executed:
#!/bin/bash
# Check if there's a type export of nodeDefsByName or similar pattern
rg -n "type NodeDefsByName|export.*Record.*ComfyNodeDefImpl" src/storesRepository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Check if NodeDefStoreType is defined and how it's used
rg -n "NodeDefStoreType" src/storesRepository: Comfy-Org/ComfyUI_frontend
Length of output: 266
🏁 Script executed:
#!/bin/bash
# Let's understand the full return type by looking at the end of useNodeDefStore
cat -n src/stores/nodeDefStore.ts | sed -n '440,475p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1129
🏁 Script executed:
#!/bin/bash
# Verify how nodeDefsByName is actually accessed in the test and in production code
cat -n src/composables/queue/useJobMenu.test.ts | sed -n '200,210p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 500
🏁 Script executed:
#!/antml/bash
# Check if there's any Vue ref unwrapping or storeToRefs being used
rg -n "storeToRefs|\.value" src/composables/queue/useJobMenu.ts | head -20Repository: Comfy-Org/ComfyUI_frontend
Length of output: 460
Replace ad-hoc type with real ComfyNodeDefImpl.
The { id: string } shape is incomplete and risks drift from the actual store definition. Import ComfyNodeDefImpl from the store and use Record<string, Partial<ComfyNodeDefImpl>> to acknowledge the incomplete mock while maintaining type safety.
♻️ Suggested refactor
+import type { ComfyNodeDefImpl } from '@/stores/nodeDefStore'
+
const nodeDefStoreMock = {
- nodeDefsByName: {} as Record<string, { id: string }>
+ nodeDefsByName: {} as Record<string, Partial<ComfyNodeDefImpl>>
}📝 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
| const nodeDefStoreMock = { | |
| nodeDefsByName: {} as Record<string, any> | |
| nodeDefsByName: {} as Record<string, { id: string }> | |
| } | |
| import type { ComfyNodeDefImpl } from '@/stores/nodeDefStore' | |
| const nodeDefStoreMock = { | |
| nodeDefsByName: {} as Record<string, Partial<ComfyNodeDefImpl>> | |
| } |
🤖 Prompt for AI Agents
In `@src/composables/queue/useJobMenu.test.ts` around lines 104 - 106, Replace the
ad-hoc mock typing for nodeDefStoreMock with the real ComfyNodeDefImpl to keep
types aligned: import ComfyNodeDefImpl from the store module and change
nodeDefStoreMock.nodeDefsByName's type to Record<string,
Partial<ComfyNodeDefImpl>> so the mock can be partial while preserving the
actual shape; update any places creating entries to match
Partial<ComfyNodeDefImpl> fields used in tests.
Comment on lines
42
to
44
| const settingStore = reactive({ | ||
| get: vi.fn(() => 'Enabled') | ||
| get: vi.fn((_key: string) => 'Enabled' as string) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant type assertion on string literal.
The as string cast on 'Enabled' is unnecessary since string literals are already typed as string by default in this context.
♻️ Minor cleanup
const settingStore = reactive({
- get: vi.fn((_key: string) => 'Enabled' as string)
+ get: vi.fn((_key: string) => 'Enabled')
})📝 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
| const settingStore = reactive({ | |
| get: vi.fn(() => 'Enabled') | |
| get: vi.fn((_key: string) => 'Enabled' as string) | |
| }) | |
| const settingStore = reactive({ | |
| get: vi.fn((_key: string) => 'Enabled') | |
| }) |
🤖 Prompt for AI Agents
In `@src/composables/useBrowserTabTitle.test.ts` around lines 42 - 44, The test's
reactive mock `settingStore` includes a redundant type assertion "'Enabled' as
string"; remove the unnecessary "as string" so the `get` mock returns the string
literal directly (keep the vi.fn signature unchanged) — update the
`settingStore` definition in useBrowserTabTitle.test.ts where `get: vi.fn((_key:
string) => 'Enabled' as string)` appears to `get: vi.fn((_key: string) =>
'Enabled')`.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/graph/SelectionToolbox.test.ts (1)
385-404: Inline mock should also use explicitPartialcast.This inline mock duplicates the pattern from
createMockExtensionServicebut with different return values. Consider reusing the helper or applying the samePartialcast pattern for consistency.♻️ Suggested approach
mockExtensionService.mockReturnValue({ extensionCommands: { value: new Map([ ['test-command', { id: 'test-command', title: 'Test Command' }] ]) }, loadExtensions: vi.fn(), registerExtension: vi.fn(), invokeExtensions: vi.fn(() => ['test-command']), invokeExtensionsAsync: vi.fn() - } as ReturnType<typeof useExtensionService>) + } as Partial<ReturnType<typeof useExtensionService>> as ReturnType<typeof useExtensionService>)
🤖 Fix all issues with AI agents
In `@src/composables/graph/useSelectionState.test.ts`:
- Around line 91-104: The test mock for useCanvasStore is using an unsafe
double-cast ("as unknown as ReturnType<typeof useCanvasStore>"); change these to
explicit partial typings by casting the mock object to Partial<ReturnType<typeof
useCanvasStore>> (e.g., vi.mocked(useCanvasStore).mockReturnValue({ ... } as
Partial<ReturnType<typeof useCanvasStore>>)). Do the same for the other store
mocks in this file (the blocks around the mocked stores at the other ranges
referenced) so mocks remain explicit about missing properties while keeping
type-safety and readability; reference useCanvasStore and mockSelectedItems to
find the exact mock objects to update.
In `@src/composables/maskeditor/useCanvasTools.test.ts`:
- Around line 11-17: The mockStore uses unsafe null! assertions for
canvas/context fields; change its type to the real store type (e.g.,
ReturnType<typeof useMaskEditorStore>) or an explicit interface where the canvas
and context properties are nullable (CanvasRenderingContext2D | null and
HTMLCanvasElement | null), then initialize fields to null (no ! cast) so tests
can assign real elements later; update the same pattern at the other occurrences
(lines referenced) to replace "null! as ..." with properly typed nullable fields
on mockStore.
In `@src/composables/node/useNodePricing.test.ts`:
- Around line 723-744: The inline verbose mock in the test should be extracted
to a helper: either add a new helper function
createMockNodeWithPriceBadge(nodeName, priceBadge, widgets, inputs) or extend
the existing createMockNode to accept a price_badge in its nodeData parameter;
then replace the long inline createMockNode call in useNodePricing.test.ts with
a call to createMockNodeWithPriceBadge (or the extended createMockNode) passing
the same name, the price_badge object (engine/expr/depends_on) and the
simplified inputs array (use connected:true/false instead of link values) so the
test remains concise and the unique symbols to update are createMockNode,
createMockNodeWithPriceBadge, and the price_badge nodeData structure.
In `@src/composables/useCoreCommands.test.ts`:
- Around line 174-195: Extract the duplicated Pinia mock into a helper: create a
function createMockSettingStore(getReturnValue: boolean): ReturnType<typeof
useSettingStore> that returns the common mock object (with get mocked to return
getReturnValue and all other methods/fields copied from the existing mock), then
replace the two vi.mocked(useSettingStore).mockReturnValue(...) calls with
vi.mocked(useSettingStore).mockReturnValue(createMockSettingStore(false)) and
vi.mocked(useSettingStore).mockReturnValue(createMockSettingStore(true))
respectively; ensure the helper includes get, addSetting, loadSettingValues,
set, exists, getDefaultValue, settingValues, settingsById, $id, $state, $patch,
$reset, $subscribe, $onAction, $dispose, _customProperties and _p to match the
original mock shape.
- Line 162: The test-level mockSubgraph is created once at module scope using
createMockSubgraph(), which lets vi.fn() instances retain call history across
tests; move the call to createMockSubgraph() into the test-level setup (e.g.,
inside beforeEach) or create it inline within each test so each test gets a
fresh mockSubgraph instance; update references to the module-scoped mockSubgraph
in tests to use the newly created local variable to ensure isolation and avoid
relying solely on vi.clearAllMocks().
In `@src/utils/__tests__/litegraphTestUtils.ts`:
- Around line 9-47: The helpers return concrete objects with a direct cast; make
the "partial mock" intent explicit by constructing a const partial: Partial<T>
inside each helper and then return partial as Partial<T> as T; update
createMockLGraphNode, createMockPositionable, and createMockLGraphGroup to build
a partial object (including current default fields and overrides) typed as
Partial<LGraphNode>/Partial<Positionable>/Partial<LGraphGroup> and return it
using the explicit Partial<T> as T cast.
♻️ Duplicate comments (13)
src/components/sidebar/tabs/queue/ResultGallery.test.ts (1)
95-95: Consider using the double-cast pattern for explicit partial indication.Based on learnings, when creating mock objects that partially implement an interface, prefer
as Partial<T>[] as T[]to make the incomplete implementation explicit. SinceMockResultItemis already defined asPartial<ResultItemImpl> & {...}, the double-cast would better communicate intent.♻️ Suggested improvement
- allGalleryItems: mockGalleryItems as ResultItemImpl[], + allGalleryItems: mockGalleryItems as Partial<ResultItemImpl>[] as ResultItemImpl[],src/composables/maskeditor/useImageLoader.test.ts (2)
10-30: Consider explicit mock types instead ofunknownfields.Typing
mockStore/mockDataStoreasunknownforces repeated casts later. Prefer explicit mock shapes orPartial<MaskEditorStore> as MaskEditorStoreto keep type intent clear. Based on learnings, explicit partial types are preferred overunknownwith inline casts.
117-133: Inline casts can be eliminated with stronger mock typing.The repeated
(mockStore.* as { ... })casts are noisy; once mock types are explicit, these assertions can be simplified. Based on learnings, explicit partial mock types are preferred.Also applies to: 205-208
src/composables/graph/useGraphHierarchy.test.ts (1)
37-56: Avoid theunknownescape hatch in the store mock.
Use an explicitPartial<ReturnType<typeof useCanvasStore>>and a single cast.Based on learnings, keep partial mock intent explicit.♻️ Suggested update
- } as unknown as Partial<ReturnType<typeof useCanvasStore>> + } as Partial<ReturnType<typeof useCanvasStore>> vi.mocked(useCanvasStore).mockReturnValue( - mockCanvasStore as ReturnType<typeof useCanvasStore> + mockCanvasStore as Partial<ReturnType<typeof useCanvasStore>> as ReturnType< + typeof useCanvasStore + > )src/components/graph/SelectionToolbox.test.ts (2)
14-21: Incorrect type reference:useCanvasStore.prototype.canvasis invalid.
useCanvasStoreis a Pinia store composable function, not a class, so it doesn't have aprototypeproperty. This cast is misleading and won't provide type safety.♻️ Suggested fix
-function createMockCanvas(): typeof useCanvasStore.prototype.canvas { - return { - setDirty: vi.fn(), - state: { - selectionChanged: false - } - } as typeof useCanvasStore.prototype.canvas -} +function createMockCanvas() { + return { + setDirty: vi.fn(), + state: { + selectionChanged: false + } + } as Partial<ReturnType<typeof useCanvasStore>['canvas']> as ReturnType<typeof useCanvasStore>['canvas'] +}
23-31: Use explicitPartialcast for incomplete mock.The mock only implements a subset of
useExtensionServicereturn type. Based on learnings, preferas Partial<ReturnType<typeof useExtensionService>> as ReturnType<typeof useExtensionService>to make the incomplete implementation explicit.♻️ Suggested fix
function createMockExtensionService(): ReturnType<typeof useExtensionService> { return { extensionCommands: { value: new Map() }, loadExtensions: vi.fn(), registerExtension: vi.fn(), invokeExtensions: vi.fn(() => []), invokeExtensionsAsync: vi.fn() - } as ReturnType<typeof useExtensionService> + } as Partial<ReturnType<typeof useExtensionService>> as ReturnType<typeof useExtensionService> }src/components/graph/selectionToolbox/ColorPickerButton.test.ts (1)
16-25: Apply explicitPartialcast for incomplete workflow mock.The mock only provides
changeTrackerbutLoadedComfyWorkflowhas additional required fields. Based on learnings, use the two-step cast to make the incomplete implementation explicit.♻️ Suggested fix
function createMockWorkflow( overrides: Partial<LoadedComfyWorkflow> = {} ): LoadedComfyWorkflow { return { changeTracker: { checkState: vi.fn() as Mock }, ...overrides - } as LoadedComfyWorkflow + } as Partial<LoadedComfyWorkflow> as LoadedComfyWorkflow }src/components/graph/selectionToolbox/BypassButton.test.ts (1)
15-15: Module-level mock may affect test isolation.
mockLGraphNodeis created once at module scope and reused across all tests. If any test mutates this object, it could affect subsequent tests. Consider creating the mock inbeforeEachor using a factory function.♻️ Suggested improvement
-const mockLGraphNode = createMockLGraphNode({ type: 'TestNode' }) +function getMockLGraphNode() { + return createMockLGraphNode({ type: 'TestNode' }) +}Then call
getMockLGraphNode()in each test that needs it.src/components/graph/selectionToolbox/ExecuteButton.test.ts (1)
47-49:MockCommandStoretype intersection is misleading.The type intersects
ReturnType<typeof useCommandStore>with{ execute: Mock }, but the mock object only providesexecute. Based on learnings, prefer explicitPartialpattern.♻️ Suggested pattern
-type MockCommandStore = ReturnType<typeof useCommandStore> & { - execute: Mock -} +// Remove type alias and use inline cast:Then at line 97-99:
mockCommandStore = { execute: vi.fn() - } as MockCommandStore + } as Partial<ReturnType<typeof useCommandStore>> as ReturnType<typeof useCommandStore>src/composables/useBrowserTabTitle.test.ts (1)
42-44: Redundant type assertion on string literal.The
as stringcast on'Enabled'is unnecessary since string literals already satisfy thestringtype.♻️ Minor cleanup
const settingStore = reactive({ - get: vi.fn((_key: string) => 'Enabled' as string) + get: vi.fn((_key: string) => 'Enabled') })src/composables/queue/useJobMenu.test.ts (1)
104-106: Replace ad-hoc type with realComfyNodeDefImpl.The
{ id: string }shape is incomplete and risks drift from the actual store definition. ImportComfyNodeDefImplfrom the store and useRecord<string, Partial<ComfyNodeDefImpl>>to acknowledge the incomplete mock while maintaining type safety.♻️ Suggested refactor
+import type { ComfyNodeDefImpl } from '@/stores/nodeDefStore' + const nodeDefStoreMock = { - nodeDefsByName: {} as Record<string, { id: string }> + nodeDefsByName: {} as Record<string, Partial<ComfyNodeDefImpl>> }src/composables/functional/useChainCallback.ts (2)
8-13: Function expression vs function declaration already flagged.The concern about using a function expression instead of a function declaration per coding guidelines has been noted in a previous review comment. Please address that feedback regarding whether the arrow function's
thisbinding semantics are intentionally required here.
17-24: Return type mismatch already flagged.The concern that the function is cast to return
Ret(line 24) but never actually returns the original callback's result has been noted in a previous review comment. The current implementation discards any return value fromoriginalCallback, making theRetreturn type misleading. Please address that feedback by either capturing and returning the result or changing the return type tovoid.
Comment on lines
91
to
+104
| vi.mocked(useCanvasStore).mockReturnValue({ | ||
| selectedItems: mockSelectedItems, | ||
| // Add minimal required properties for the store | ||
| $id: 'canvas', | ||
| $state: {} as any, | ||
| $state: { | ||
| selectedItems: mockSelectedItems | ||
| }, | ||
| $patch: vi.fn(), | ||
| $reset: vi.fn(), | ||
| $subscribe: vi.fn(), | ||
| $onAction: vi.fn(), | ||
| $dispose: vi.fn(), | ||
| _customProperties: new Set(), | ||
| _p: {} as any | ||
| } as any) | ||
| _p: {} | ||
| } as unknown as ReturnType<typeof useCanvasStore>) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer Partial<ReturnType<...>> over unknown casts for store mocks.
You can keep these mocks explicit about incompleteness without as unknown as. This improves test readability and matches our test-mock typing guidance.
♻️ Suggested pattern (apply to all store mocks in this block)
- } as unknown as ReturnType<typeof useCanvasStore>)
+ } as Partial<ReturnType<typeof useCanvasStore>> as ReturnType<
+ typeof useCanvasStore
+ >)
...
- } as unknown as ReturnType<typeof useNodeDefStore>)
+ } as Partial<ReturnType<typeof useNodeDefStore>> as ReturnType<
+ typeof useNodeDefStore
+ >)
...
- } as unknown as ReturnType<typeof useSidebarTabStore>)
+ } as Partial<ReturnType<typeof useSidebarTabStore>> as ReturnType<
+ typeof useSidebarTabStore
+ >)
...
- } as unknown as ReturnType<typeof useNodeHelpStore>)
+ } as Partial<ReturnType<typeof useNodeHelpStore>> as ReturnType<
+ typeof useNodeHelpStore
+ >)Based on learnings, this makes partial mocks explicit without unsafe double-casts.
Also applies to: 107-123, 127-139, 144-160
🤖 Prompt for AI Agents
In `@src/composables/graph/useSelectionState.test.ts` around lines 91 - 104, The
test mock for useCanvasStore is using an unsafe double-cast ("as unknown as
ReturnType<typeof useCanvasStore>"); change these to explicit partial typings by
casting the mock object to Partial<ReturnType<typeof useCanvasStore>> (e.g.,
vi.mocked(useCanvasStore).mockReturnValue({ ... } as Partial<ReturnType<typeof
useCanvasStore>>)). Do the same for the other store mocks in this file (the
blocks around the mocked stores at the other ranges referenced) so mocks remain
explicit about missing properties while keeping type-safety and readability;
reference useCanvasStore and mockSelectedItems to find the exact mock objects to
update.
Comment on lines
11
to
+17
| const mockStore = { | ||
| maskCtx: null as any, | ||
| imgCtx: null as any, | ||
| maskCanvas: null as any, | ||
| imgCanvas: null as any, | ||
| rgbCtx: null as any, | ||
| rgbCanvas: null as any, | ||
| maskCtx: null! as CanvasRenderingContext2D, | ||
| imgCtx: null! as CanvasRenderingContext2D, | ||
| maskCanvas: null! as HTMLCanvasElement, | ||
| imgCanvas: null! as HTMLCanvasElement, | ||
| rgbCtx: null! as CanvasRenderingContext2D, | ||
| rgbCanvas: null! as HTMLCanvasElement, |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid null! assertions by typing the mock store with nullable fields.
The repeated null! as ... assignments reintroduce unsafe assertions and add noise. Consider typing mockStore as the real store type (or ReturnType<typeof useMaskEditorStore>) with nullable canvas/context fields so tests can assign null directly and avoid assertions.
Also applies to: 167-167, 252-252, 372-372, 382-382, 404-404, 415-415
🤖 Prompt for AI Agents
In `@src/composables/maskeditor/useCanvasTools.test.ts` around lines 11 - 17, The
mockStore uses unsafe null! assertions for canvas/context fields; change its
type to the real store type (e.g., ReturnType<typeof useMaskEditorStore>) or an
explicit interface where the canvas and context properties are nullable
(CanvasRenderingContext2D | null and HTMLCanvasElement | null), then initialize
fields to null (no ! cast) so tests can assign real elements later; update the
same pattern at the other occurrences (lines referenced) to replace "null! as
..." with properly typed nullable fields on mockStore.
Comment on lines
+723
to
+744
| const node = createMockNode( | ||
| { | ||
| name: 'TestInputGroupNode', | ||
| api_node: true, | ||
| price_badge: { | ||
| engine: 'jsonata', | ||
| expr: '{"type":"usd","usd": inputGroups.videos * 0.05}', | ||
| depends_on: { | ||
| widgets: [], | ||
| inputs: [], | ||
| input_groups: ['videos'] | ||
| } | ||
| } | ||
| }, | ||
| [], | ||
| [ | ||
| { name: 'videos.clip1', link: 1 }, // connected | ||
| { name: 'videos.clip2', link: 2 }, // connected | ||
| { name: 'videos.clip3', link: null }, // disconnected | ||
| { name: 'other_input', link: 3 } // connected but not in group | ||
| ], | ||
| constructor: { | ||
| nodeData: { | ||
| name: 'TestInputGroupNode', | ||
| api_node: true, | ||
| price_badge: { | ||
| engine: 'jsonata', | ||
| expr: '{"type":"usd","usd": inputGroups.videos * 0.05}', | ||
| depends_on: { | ||
| widgets: [], | ||
| inputs: [], | ||
| input_groups: ['videos'] | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| ] | ||
| ) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting complex mock to helper.
This inline mock construction for input_groups test is verbose. Consider using createMockNodeWithPriceBadge with the appropriate parameters, or extending createMockNode to accept price_badge in nodeData.
♻️ Potential simplification
The test could use createMockNodeWithPriceBadge if inputs are added:
const node = createMockNodeWithPriceBadge(
'TestInputGroupNode',
{
engine: 'jsonata',
expr: '{"type":"usd","usd": inputGroups.videos * 0.05}',
depends_on: { widgets: [], inputs: [], input_groups: ['videos'] }
},
[],
[
{ name: 'videos.clip1', connected: true },
{ name: 'videos.clip2', connected: true },
{ name: 'videos.clip3', connected: false },
{ name: 'other_input', connected: true }
]
)🤖 Prompt for AI Agents
In `@src/composables/node/useNodePricing.test.ts` around lines 723 - 744, The
inline verbose mock in the test should be extracted to a helper: either add a
new helper function createMockNodeWithPriceBadge(nodeName, priceBadge, widgets,
inputs) or extend the existing createMockNode to accept a price_badge in its
nodeData parameter; then replace the long inline createMockNode call in
useNodePricing.test.ts with a call to createMockNodeWithPriceBadge (or the
extended createMockNode) passing the same name, the price_badge object
(engine/expr/depends_on) and the simplified inputs array (use
connected:true/false instead of link values) so the test remains concise and the
unique symbols to update are createMockNode, createMockNodeWithPriceBadge, and
the price_badge nodeData structure.
| } as Partial<typeof app.canvas.subgraph> as typeof app.canvas.subgraph | ||
| } | ||
|
|
||
| const mockSubgraph = createMockSubgraph() |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Module-level mock may affect test isolation.
mockSubgraph is created once at module scope and reused. Since createMockSubgraph() creates mocks with vi.fn(), these mock functions accumulate call history across tests. While vi.clearAllMocks() in beforeEach resets call counts, consider creating a fresh subgraph per test for better isolation.
♻️ Suggested improvement
Move the creation into beforeEach or create inline in tests:
-const mockSubgraph = createMockSubgraph()
+let mockSubgraph: ReturnType<typeof createMockSubgraph>
beforeEach(() => {
vi.clearAllMocks()
setActivePinia(createPinia())
+ mockSubgraph = createMockSubgraph()
app.canvas.subgraph = undefined
// ...
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const mockSubgraph = createMockSubgraph() | |
| let mockSubgraph: ReturnType<typeof createMockSubgraph> | |
| beforeEach(() => { | |
| vi.clearAllMocks() | |
| setActivePinia(createPinia()) | |
| mockSubgraph = createMockSubgraph() | |
| app.canvas.subgraph = undefined | |
| // ... | |
| }) |
🤖 Prompt for AI Agents
In `@src/composables/useCoreCommands.test.ts` at line 162, The test-level
mockSubgraph is created once at module scope using createMockSubgraph(), which
lets vi.fn() instances retain call history across tests; move the call to
createMockSubgraph() into the test-level setup (e.g., inside beforeEach) or
create it inline within each test so each test gets a fresh mockSubgraph
instance; update references to the module-scoped mockSubgraph in tests to use
the newly created local variable to ensure isolation and avoid relying solely on
vi.clearAllMocks().
Comment on lines
174
to
+195
| vi.mocked(useSettingStore).mockReturnValue({ | ||
| get: vi.fn().mockReturnValue(false) // Skip confirmation dialog | ||
| } as any) | ||
| get: vi.fn().mockReturnValue(false), // Skip confirmation dialog | ||
| addSetting: vi.fn(), | ||
| loadSettingValues: vi.fn(), | ||
| set: vi.fn(), | ||
| exists: vi.fn(), | ||
| getDefaultValue: vi.fn(), | ||
| settingValues: {}, | ||
| settingsById: {}, | ||
| $id: 'setting', | ||
| $state: { | ||
| settingValues: {}, | ||
| settingsById: {} | ||
| }, | ||
| $patch: vi.fn(), | ||
| $reset: vi.fn(), | ||
| $subscribe: vi.fn(), | ||
| $onAction: vi.fn(), | ||
| $dispose: vi.fn(), | ||
| _customProperties: new Set(), | ||
| _p: {} | ||
| } as ReturnType<typeof useSettingStore>) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract duplicated Pinia store mock to a helper.
The useSettingStore mock configuration is duplicated between lines 174-195 and 244-265, differing only in the get return value. Extract to a helper function to reduce duplication.
♻️ Suggested refactor
function createMockSettingStore(
getReturnValue: boolean
): ReturnType<typeof useSettingStore> {
return {
get: vi.fn().mockReturnValue(getReturnValue),
addSetting: vi.fn(),
loadSettingValues: vi.fn(),
set: vi.fn(),
exists: vi.fn(),
getDefaultValue: vi.fn(),
settingValues: {},
settingsById: {},
$id: 'setting',
$state: { settingValues: {}, settingsById: {} },
$patch: vi.fn(),
$reset: vi.fn(),
$subscribe: vi.fn(),
$onAction: vi.fn(),
$dispose: vi.fn(),
_customProperties: new Set(),
_p: {}
} as ReturnType<typeof useSettingStore>
}Then use:
vi.mocked(useSettingStore).mockReturnValue(createMockSettingStore(false))
// and
vi.mocked(useSettingStore).mockReturnValue(createMockSettingStore(true))Also applies to: 244-265
🤖 Prompt for AI Agents
In `@src/composables/useCoreCommands.test.ts` around lines 174 - 195, Extract the
duplicated Pinia mock into a helper: create a function
createMockSettingStore(getReturnValue: boolean): ReturnType<typeof
useSettingStore> that returns the common mock object (with get mocked to return
getReturnValue and all other methods/fields copied from the existing mock), then
replace the two vi.mocked(useSettingStore).mockReturnValue(...) calls with
vi.mocked(useSettingStore).mockReturnValue(createMockSettingStore(false)) and
vi.mocked(useSettingStore).mockReturnValue(createMockSettingStore(true))
respectively; ensure the helper includes get, addSetting, loadSettingValues,
set, exists, getDefaultValue, settingValues, settingsById, $id, $state, $patch,
$reset, $subscribe, $onAction, $dispose, _customProperties and _p to match the
original mock shape.
Comment on lines
+9
to
+47
| export function createMockLGraphNode( | ||
| overrides: Partial<LGraphNode> | Record<string, unknown> = {} | ||
| ): LGraphNode { | ||
| const partial = { | ||
| id: 1, | ||
| pos: [0, 0], | ||
| size: [100, 100], | ||
| title: 'Test Node', | ||
| mode: LGraphEventMode.ALWAYS, | ||
| ...(overrides as Partial<LGraphNode>) | ||
| } | ||
| return partial as LGraphNode | ||
| } | ||
|
|
||
| /** | ||
| * Creates a mock Positionable object | ||
| */ | ||
| export function createMockPositionable( | ||
| overrides: Partial<Positionable> = {} | ||
| ): Positionable { | ||
| return { | ||
| id: 1, | ||
| pos: [0, 0], | ||
| ...overrides | ||
| } as Positionable | ||
| } | ||
|
|
||
| /** | ||
| * Creates a mock LGraphGroup with minimal required properties | ||
| */ | ||
| export function createMockLGraphGroup( | ||
| overrides: Partial<LGraphGroup> = {} | ||
| ): LGraphGroup { | ||
| return { | ||
| id: 1, | ||
| pos: [0, 0], | ||
| boundingRect: new Rectangle(0, 0, 100, 100), | ||
| ...overrides | ||
| } as LGraphGroup |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make partial mocks explicit to match test guidelines.
These helpers return incomplete shapes; prefer the explicit Partial<T> as T pattern for clarity.
♻️ Proposed refactor
export function createMockLGraphNode(
overrides: Partial<LGraphNode> | Record<string, unknown> = {}
): LGraphNode {
- const partial = {
+ const partial: Partial<LGraphNode> = {
id: 1,
pos: [0, 0],
size: [100, 100],
title: 'Test Node',
mode: LGraphEventMode.ALWAYS,
...(overrides as Partial<LGraphNode>)
}
- return partial as LGraphNode
+ return partial as Partial<LGraphNode> as LGraphNode
}
@@
export function createMockPositionable(
overrides: Partial<Positionable> = {}
): Positionable {
- return {
+ return {
id: 1,
pos: [0, 0],
...overrides
- } as Positionable
+ } as Partial<Positionable> as Positionable
}
@@
export function createMockLGraphGroup(
overrides: Partial<LGraphGroup> = {}
): LGraphGroup {
- return {
+ return {
id: 1,
pos: [0, 0],
boundingRect: new Rectangle(0, 0, 100, 100),
...overrides
- } as LGraphGroup
+ } as Partial<LGraphGroup> as LGraphGroup
}📝 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
| export function createMockLGraphNode( | |
| overrides: Partial<LGraphNode> | Record<string, unknown> = {} | |
| ): LGraphNode { | |
| const partial = { | |
| id: 1, | |
| pos: [0, 0], | |
| size: [100, 100], | |
| title: 'Test Node', | |
| mode: LGraphEventMode.ALWAYS, | |
| ...(overrides as Partial<LGraphNode>) | |
| } | |
| return partial as LGraphNode | |
| } | |
| /** | |
| * Creates a mock Positionable object | |
| */ | |
| export function createMockPositionable( | |
| overrides: Partial<Positionable> = {} | |
| ): Positionable { | |
| return { | |
| id: 1, | |
| pos: [0, 0], | |
| ...overrides | |
| } as Positionable | |
| } | |
| /** | |
| * Creates a mock LGraphGroup with minimal required properties | |
| */ | |
| export function createMockLGraphGroup( | |
| overrides: Partial<LGraphGroup> = {} | |
| ): LGraphGroup { | |
| return { | |
| id: 1, | |
| pos: [0, 0], | |
| boundingRect: new Rectangle(0, 0, 100, 100), | |
| ...overrides | |
| } as LGraphGroup | |
| export function createMockLGraphNode( | |
| overrides: Partial<LGraphNode> | Record<string, unknown> = {} | |
| ): LGraphNode { | |
| const partial: Partial<LGraphNode> = { | |
| id: 1, | |
| pos: [0, 0], | |
| size: [100, 100], | |
| title: 'Test Node', | |
| mode: LGraphEventMode.ALWAYS, | |
| ...(overrides as Partial<LGraphNode>) | |
| } | |
| return partial as Partial<LGraphNode> as LGraphNode | |
| } | |
| /** | |
| * Creates a mock Positionable object | |
| */ | |
| export function createMockPositionable( | |
| overrides: Partial<Positionable> = {} | |
| ): Positionable { | |
| return { | |
| id: 1, | |
| pos: [0, 0], | |
| ...overrides | |
| } as Partial<Positionable> as Positionable | |
| } | |
| /** | |
| * Creates a mock LGraphGroup with minimal required properties | |
| */ | |
| export function createMockLGraphGroup( | |
| overrides: Partial<LGraphGroup> = {} | |
| ): LGraphGroup { | |
| return { | |
| id: 1, | |
| pos: [0, 0], | |
| boundingRect: new Rectangle(0, 0, 100, 100), | |
| ...overrides | |
| } as Partial<LGraphGroup> as LGraphGroup | |
| } |
🤖 Prompt for AI Agents
In `@src/utils/__tests__/litegraphTestUtils.ts` around lines 9 - 47, The helpers
return concrete objects with a direct cast; make the "partial mock" intent
explicit by constructing a const partial: Partial<T> inside each helper and then
return partial as Partial<T> as T; update createMockLGraphNode,
createMockPositionable, and createMockLGraphGroup to build a partial object
(including current default fields and overrides) typed as
Partial<LGraphNode>/Partial<Positionable>/Partial<LGraphGroup> and return it
using the explicit Partial<T> as T cast.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Reduced unsafe
as unknown astype assertions from ~107 to 6 (94% reduction) across 18 test files in graph components and composables.Changes
src/utils/__tests__/litegraphTestUtils.tscreateMockLGraphNode(),createMockPositionable(),createMockLGraphGroup(),createMockSubgraphNode()Object.assignto avoid constructor property conflictsPartial<T> | Record<string, unknown>for flexible mock creationRemaining Assertions
6
as unknown asassertions remain for incomplete Pinia store mocks where providing complete$stateproperties is impractical.Results
Files Modified (15)
src/components/graph/SelectionToolbox.test.tssrc/components/graph/selectionToolbox/{BypassButton,ColorPickerButton,ExecuteButton}.test.tssrc/components/sidebar/tabs/queue/ResultGallery.test.tssrc/composables/canvas/useSelectedLiteGraphItems.test.tssrc/composables/graph/{useGraphHierarchy,useSelectionState}.test.tssrc/composables/maskeditor/{useCanvasHistory,useCanvasManager,useCanvasTransform}.test.tssrc/composables/node/{useNodePricing,useWatchWidget}.test.tssrc/composables/useCoreCommands.test.tssrc/utils/__tests__/litegraphTestUtils.ts(new)┆Issue is synchronized with this Notion page by Unito