test: remove explicit any types from test files - Part 8 #8197
Conversation
- Define V1RawPrompt and CloudRawPrompt tuple types for queue responses - Export QueueIndex, PromptInputs, ExtraData, OutputsToExecute from apiSchema - Type normalizeQueuePrompt with proper discriminated union - Type #postItem body as Record<string, unknown> - Type storeUserData data as BodyInit | Record<string, unknown> | null - Type getCustomNodesI18n return as Record<string, unknown>
- Add GroupNodeConfigEntry interface to LGraph.ts for config typing - Extend GroupNodeWorkflowData with title and widgets_values node props - Type config as Record<number, GroupNodeConfigEntry> instead of unknown - Export new types from litegraph barrel file - Fix all class properties with definite assignment assertions - Type all method parameters (changeTab, changeNode, changeGroup, etc.) - Fix event handlers with proper Event and CustomEvent types - Type storeModification, getEditElement, and build*Page methods - Fix save button callback with proper generic types for node ordering - Add override modifier to show method - Use optional chaining for node.recreate() call
…types - Type nodeOutputs as Record<string, ExecutedWsMessage['output']> - Import CanvasPointerEvent from litegraph - Type prompt callback value as string | number - Type prompt callback function as (v: string) => void - Type event parameter as CanvasPointerEvent
ComfyDialog: - Type constructor buttons parameter as HTMLButtonElement[] | null - Type show method parameter as string | HTMLElement | HTMLElement[] - Use definite assignment for textElement ComfyAsyncDialog: - Make class generic with DialogAction<T> type - Type resolve function, show/showModal return types - Fix button creation with proper HTMLButtonElement cast - Add generic static prompt method ManageGroupDialog: - Update show method signature to match base class - Extract nodeType from parameter for string comparisons
groupNodeManage.ts: - Add bounds checking before accessing node indices in link/external loops - Fix selectedNodeInnerIndex getter with proper runtime guards - Change storeModification section param to string | symbol, removing casts asyncDialog.ts: - Initialize #resolve with no-op function for defensive safety api.ts: - Add warning log for unexpected non-array queue prompts
- Add zCustomNodesI18n schema and CustomNodesI18n type to apiSchema.ts - Update getCustomNodesI18n to use new CustomNodesI18n type - Simplify storeUserData data parameter from BodyInit | Record<string, unknown> | null to unknown
Replace all 10 `any` types with proper TypeScript types: - Use IWidget for widget mocks with correct type properties - Use Size for onResize handler parameter - Use CanvasRenderingContext2D for onDrawBackground - Use CanvasPointerEvent for mouse event handlers - Use Partial<T> for partial mock objects (Load3d, LGraph, ToastStore) - Use shallowRef for LGraphNode ref typing - Replace type hacks (as unknown as never) with proper types All tests passing, 0 typecheck errors.
Replace all 8 `any` types with proper TypeScript types:
- Use proper function signatures in mock definitions
- Use JobListItem['state'] for state type assertion
- Use Partial<TaskItemImpl> for partial task mocks
- Use typed parameters for all mock functions (downloadFile, copyToClipboard, interrupt, deleteItem, etc.)
- Use Record<string, { id: string }> for nodeDefsByName
- Handle optional parameters correctly in downloadFile mock
All 36 tests passing, 0 typecheck errors.
Removed all 5 any types and replaced with proper TypeScript types. Changes: - Replaced any with Partial<Load3d> for mock objects - Used ReturnType<typeof useLoad3dService> for service mocks - Used ReturnType<typeof useToastStore> for store mocks - Used Load3d['sceneManager'] etc. for nested manager types - Minimized type assertions to 2 necessary as unknown casts Remaining type assertions: - mockNode: Partial LGraphNode mock (constructor incompatible) - sceneManager: Simplified gridHelper mock (vs 82+ Three.js properties) Both mocks accurately reflect implementation usage patterns. All tests passing (33/33), 0 typecheck errors. Part of #8092
Replaced any with ReturnType<typeof useToastStore> for mockToastStore. All tests passing (13/13), 0 typecheck errors. Part of #8092
Replaced all 3 any types with unknown for generic request parameters. All tests passing (11/11), 0 typecheck errors. Part of #8092
Removed all 11 any types and replaced with proper TypeScript types. Changes: - Typed executionStore mock with proper structure - Typed workflowStore.activeWorkflow based on actual usage - Replaced (settingStore.get as any) with vi.mocked(settingStore.get) - Fixed vi.fn() signature to accept key parameter All tests passing (8/9, 1 skipped), 0 typecheck errors. Part of #8092
Removed all 3 any types and replaced with proper TypeScript types. Changes: - Replaced as any with Partial<ReturnType<typeof useSettingStore>> - Used as unknown as for partial mock subgraph (114+ missing properties) All tests passing (3/3), 0 typecheck errors. Part of #8092
Removed all 7 any types by removing unnecessary type assertions. TypeScript can infer return types correctly for mockImplementation callbacks returning literal values. All tests passing (8/8), 0 typecheck errors. Part of #8092
Removed all 3 any types and replaced with proper TypeScript types. Changes: - Replaced as any with as unknown as for partial mockCanvasStore - Removed as any from null assignments (TypeScript infers correctly) All tests passing (7/7), 0 typecheck errors. Part of #8092
Replaced any with proper type for mockCanvas.
Changes:
- Typed mockCanvas as { selectedItems: Set<Positionable> }
- Added as unknown as Positionable casts for MockNode instances
- Added as unknown as cast for getCanvas mock return value
All tests passing (18/18), 0 typecheck errors.
Part of #8092
Removed all 14 any types from Pinia store mocks. Changes: - Replaced as any with as unknown as ReturnType<typeof store> pattern - Removed as any from $state and _p properties (empty objects) All tests passing (6/6), 0 typecheck errors. Part of #8092
Replaced any with Record<string, unknown> for mountComponent props parameter. Uses as unknown as cast to allow testing edge cases with invalid prop types. All tests passing (11/11), 0 typecheck errors. Part of #8092
- Replace `props: any` with `Record<string, unknown>` - Use ComponentProps type assertion for type safety - Refactor tests to check input.element.value instead of internal state - This tests actual DOM behavior rather than implementation details Part of Phase 2 - Quick wins (1 instance removed)
- Replace `as any` with proper `Mock` type from vitest - Import Mock type and access mock.calls with type safety - Explicitly type selectionCallback as `() => void` - Break down mock access into steps for clarity Part of Phase 2 - Quick wins (1 instance removed)
- Replace `props: any` with `Record<string, unknown>` - Use ComponentProps type assertion for type safety - Import ComponentProps from vue-component-type-helpers Part of Phase 2 - Quick wins (1 instance removed)
- Replace `props: any` and return type with proper types - Use Record<string, unknown> with ComponentProps cast - Refactor tests to check FormItem component props instead of internal state - Import ComponentProps from vue-component-type-helpers Part of Phase 2 - Quick wins (1 instance removed)
- Replace `props: any` with ComponentProps<typeof ApiKeyForm> - Import ComponentProps from vue-component-type-helpers - Linter optimized to direct ComponentProps usage Part of Phase 2 - Quick wins (1 instance removed)
- Replace `events: any[]` with properly typed `MockEvent[]` - Define MockEvent interface matching the test data structure - Improve type safety for event testing Part of Phase 2 - Quick wins (1 instance removed)
dosubot
bot
added
the
size:XL
🎭 Playwright Tests:
|
- Replace mockColorContext: any with proper MockColorContext interface - Apply double-cast at draw() call sites to satisfy DefaultConnectionColors type - Pattern: colorContext: mockColorContext as unknown as DefaultConnectionColors
| // Access the mock calls - TypeScript can't infer the mock structure dynamically | ||
| const selectionCallback = (mockTerminal.onSelectionChange as any).mock | ||
| .calls[0][0] | ||
| const mockCalls = (mockTerminal.onSelectionChange as Mock).mock.calls |
There was a problem hiding this comment.
In general, I would avoid type assertions.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| onMouseLeave: undefined, | ||
| onResize: undefined, | ||
| onDrawBackground: undefined | ||
| } as unknown as LGraphNode |
There was a problem hiding this comment.
[quality] medium Priority
Issue: Type assertion pattern could be more type-safe
Context: Using 'as unknown as' double assertion bypasses type checking completely
Suggestion: Consider using a more specific interface or utility type instead of the double assertion pattern
| } as Partial<LGraph> as LGraph, | ||
| widgets: [] | ||
| } as any | ||
| } as unknown as LGraphNode |
There was a problem hiding this comment.
[quality] medium Priority
Issue: Complex double type assertion with multiple 'as' statements
Context: The pattern 'as Partial as T' followed by 'as unknown as U' is unnecessarily complex and reduces type safety
Suggestion: Consider extracting a proper mock factory function or using a more specific mock interface for better maintainability
| const downloadFileMock = vi.fn() | ||
| vi.mock('@/base/common/downloadUtil', () => ({ | ||
| downloadFile: (...args: any[]) => downloadFileMock(...args) | ||
| downloadFile: (url: string, filename?: string) => { |
There was a problem hiding this comment.
[quality] medium Priority
Issue: Good improvement replacing 'any' with proper function signatures
Context: The mock functions now have proper type signatures instead of 'any[]' rest parameters
Suggestion: Consider creating a centralized mock factory for commonly used mock functions to ensure consistency across test files
| _p: {} as any | ||
| } as any) | ||
| _p: {} | ||
| } as unknown as ReturnType<typeof useCanvasStore>) |
There was a problem hiding this comment.
[quality] low Priority
Issue: Extensive store mock setup with many boilerplate properties
Context: The Pinia store mocks require many mandatory properties that don't contribute to test logic
Suggestion: Consider creating a utility function like 'createMockPiniaStore' to reduce boilerplate and improve test maintainability
| components: { IconField, InputIcon, InputText } | ||
| }, | ||
| props, | ||
| props: props as unknown as ComponentProps<typeof UrlInput>, |
There was a problem hiding this comment.
[quality] medium Priority
Issue: Good use of ComponentProps pattern for type safety
Context: Using ComponentProps from vue-component-type-helpers provides proper typing for component props
Suggestion: This is a good pattern. Consider documenting this approach in the test guidelines for consistency across the codebase
There was a problem hiding this comment.
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: test: remove explicit any types from test files - Part 8 (#8197)
Impact: 394 additions, 234 deletions across 20 files
Issue Distribution
- Critical: 0
- High: 0
- Medium: 4
- Low: 1
Category Breakdown
- Architecture: 0 issues
- Security: 0 issues
- Performance: 0 issues
- Code Quality: 5 issues
Key Findings
Architecture & Design
This PR continues the systematic effort to eliminate explicit any types from test files. The changes follow established patterns from previous parts of this initiative. The approach is architecturally sound and maintains consistency with Vue 3 Composition API patterns and TypeScript best practices.
Security Considerations
No security-related issues identified. Test files do not introduce security vulnerabilities, and the type improvements actually enhance safety by providing better compile-time checking.
Performance Impact
Minimal performance impact. The changes are purely related to TypeScript typing in test files and do not affect runtime performance. The improved typing may slightly increase TypeScript compilation time but provides better developer experience.
Integration Points
The changes maintain compatibility with existing testing infrastructure:
- Vitest test runner continues to work properly
- Vue Test Utils integration remains intact
- PrimeVue component testing patterns are preserved
- Mock setup patterns are improved with proper typing
Positive Observations
- Excellent Progress on Type Safety: Successfully removed 23 explicit any types while maintaining test functionality
- Consistent ComponentProps Pattern: Good adoption of ComponentProps pattern from vue-component-type-helpers
- Improved Mock Typing: Replaced generic function patterns with properly typed function signatures
- Test Quality Maintenance: All existing test cases continue to pass with improved type safety
- Systematic Approach: Well-organized progression through files with clear patterns and documentation
References
- Repository Architecture Guide: https://github.com/Comfy-Org/comfy-claude-prompt-library/blob/master/project-summaries-for-agents/ComfyUI_frontend/REPOSITORY_GUIDE.md
- Vue 3 TypeScript Best Practices: https://vuejs.org/guide/typescript/composition-api.html
Next Steps
- Address the type assertion patterns mentioned in inline comments
- Consider creating utility functions for common mock patterns to reduce boilerplate
- Document the ComponentProps pattern in test guidelines for consistency
- Continue with remaining phases of the any-type removal initiative
This is a comprehensive automated review. The changes represent solid progress toward better type safety in the test suite.
github-actions
bot
removed
the
claude-review
- Replace widgetValue: any with unknown in createNodeWithWidget helper
- Replace (ctx.measureText as any) with (ctx.measureText as Mock) - Import Mock type from vitest
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/subgraph/SubgraphSlotVisualFeedback.test.ts (1)
18-53: Remove theas unknown ascasts by typingmockColorContextonce.The mock already matches the required shape; the repeated assertions obscure type safety. If
MockColorContextextendsDefaultConnectionColors, you can pass it directly.♻️ Proposed refactor
- let mockColorContext: MockColorContext + let mockColorContext: MockColorContext ... - mockColorContext = { + mockColorContext = { defaultInputColor: '#FF0000', defaultOutputColor: '#00FF00', getConnectedColor: vi.fn().mockReturnValue('#0000FF'), getDisconnectedColor: vi.fn().mockReturnValue('#AAAAAA') - } as unknown as MockColorContext + } ... - colorContext: mockColorContext as unknown as DefaultConnectionColors, + colorContext: mockColorContext,Apply the same removal of casts to the other
drawcalls.As per coding guidelines, type assertions should be a last resort.
Also applies to: 69-74, 89-94, 114-119, 133-138, 179-183
🤖 Fix all issues with AI agents
In `@src/lib/litegraph/src/subgraph/SubgraphSlotVisualFeedback.test.ts`:
- Around line 4-14: Replace the duplicated test interface by deriving
MockColorContext from the real type: change the local interface MockColorContext
to extend or alias DefaultConnectionColors (e.g., "interface MockColorContext
extends DefaultConnectionColors {}" or "type MockColorContext =
DefaultConnectionColors") and remove the repeated properties (defaultInputColor,
defaultOutputColor, getConnectedColor, getDisconnectedColor) so the test uses
the production typing and stays in sync with the real connection color contract.
| import type { DefaultConnectionColors } from '@/lib/litegraph/src/interfaces' | ||
| import { LGraphNode } from '@/lib/litegraph/src/litegraph' | ||
|
|
||
| import { createTestSubgraph } from './__fixtures__/subgraphHelpers' | ||
|
|
||
| interface MockColorContext { | ||
| defaultInputColor: string | ||
| defaultOutputColor: string | ||
| getConnectedColor: ReturnType<typeof vi.fn> | ||
| getDisconnectedColor: ReturnType<typeof vi.fn> | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Derive MockColorContext from DefaultConnectionColors to avoid duplicate signatures.
Right now the interface repeats production methods. Prefer extending/combining the real type so tests don’t drift from production typings.
♻️ Proposed refactor
-import type { DefaultConnectionColors } from '@/lib/litegraph/src/interfaces'
+import type { DefaultConnectionColors } from '@/lib/litegraph/src/interfaces'
-interface MockColorContext {
- defaultInputColor: string
- defaultOutputColor: string
- getConnectedColor: ReturnType<typeof vi.fn>
- getDisconnectedColor: ReturnType<typeof vi.fn>
-}
+type MockColorContext = DefaultConnectionColors & {
+ defaultInputColor: string
+ defaultOutputColor: string
+}Based on learnings, avoid duplicating interface definitions in tests.
🧰 Tools
🪛 ESLint
[error] 5-5: Unable to resolve path to module '@/lib/litegraph/src/litegraph'.
(import-x/no-unresolved)
[error] 7-7: Unable to resolve path to module './fixtures/subgraphHelpers'.
(import-x/no-unresolved)
🤖 Prompt for AI Agents
In `@src/lib/litegraph/src/subgraph/SubgraphSlotVisualFeedback.test.ts` around
lines 4 - 14, Replace the duplicated test interface by deriving MockColorContext
from the real type: change the local interface MockColorContext to extend or
alias DefaultConnectionColors (e.g., "interface MockColorContext extends
DefaultConnectionColors {}" or "type MockColorContext =
DefaultConnectionColors") and remove the repeated properties (defaultInputColor,
defaultOutputColor, getConnectedColor, getDisconnectedColor) so the test uses
the production typing and stays in sync with the real connection color contract.
- Define MockSearchClient interface with search: Mock property - Replace mockSearchClient: any with MockSearchClient type - Apply double-cast when setting up mock return value - Note: File still has other any instances for global properties and test casts
- Replace overrides: any with Record<string, unknown> in createMockNodeDef - Replace 'unknown' as any with double-cast to NodeSourceType
- Define MockSettingStore interface with proper Mock types - Create mockSettingStoreTyped with double-cast to useSettingStore return type - Replace all mockSettingStore usages in initializeIfNewUser calls - Import useSettingStore type from correct path
- Replace widgetValues: any[] with unknown[]
- Replace activeWorkflow mock with LoadedComfyWorkflow type and double-cast - Replace selectedItems mocks with Positionable type and double-cast - Import Mock type from vitest for checkState mock
🔧 Auto-fixes AppliedThis PR has been automatically updated to fix linting and formatting issues.
Changes made:
|
- Define MockNodeInput interface for test inputs - Replace widgets value: any with unknown - Replace const node: any with proper object and double-cast to LGraphNode - Add explicit return type boolean to isInputConnected
- Define NodeConstructorWithSlotOffset interface for constructor with optional slot_start_y - Replace 3 'as any' casts with proper double-cast pattern - All instances related to dynamic slot_start_y property on node constructor
…test.ts - Define MockPointerEvent interface for canvas pointer events - Define MockRenderLink interface for partial render link mocks - Import CanvasPointerEvent type from events module - Replace 3 'as any' casts with proper typed interfaces and double-cast
- Define MockPointerEvent interface for canvas events - Import CanvasPointerEvent type from events module - Replace 3 'as any' casts with proper typed interface and double-cast
- Update mockQueryParams type to include string[] for Vue Router query params - Replace params?: any with unknown and inline type assertion - Remove unnecessary 'as any' casts from array assignments
- Define MockWebSocket interface with proper Mock types - Replace mockWebSocket: any with MockWebSocket type - Replace event: any with unknown in event handlers - Import Mock type from vitest
|
Will split this into 3 or 4 parts for easy review. |
also, many |
Summary
Part 8 of the systematic removal of explicit
anytypes from test files. Focus on Phase 2 "quick wins" - files with 1-2 instances.Changes
Phase 2 - Quick Wins (9 files, 23 any types removed)
Component Test Helpers
Mock Typing
Mocktype from vitestComposable Tests
Patterns Established
import type { ComponentProps } from 'vue-component-type-helpers'Record<string, unknown>withas unknown as ComponentProps<...>for edge casesimport type { Mock } from 'vitest'as unknown as ReturnType<typeof store>Progress
Related
anytypes from the codebase┆Issue is synchronized with this Notion page by Unito