CLI - Refactor types #3599
CLI - Refactor types #3599
Conversation
|
There was a problem hiding this comment.
Pull Request Overview
This PR enables strict TypeScript type checking by replacing any types with unknown and proper type assertions, and enforces stricter ESLint rules to improve type safety across the CLI codebase. The changes include enabling exactOptionalPropertyTypes, updating the ESLint configuration to error on explicit any usage, and fixing numerous type issues throughout the codebase.
Key changes:
- Replaced
anytypes withunknownand added proper type assertions - Enabled
exactOptionalPropertyTypesin tsconfig.json requiring explicit handling of optional properties - Updated ESLint configuration to enforce
@typescript-eslint/no-explicit-any: error
Reviewed Changes
Copilot reviewed 88 out of 89 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| cli/tsconfig.json | Added exactOptionalPropertyTypes and path mappings for shared types |
| cli/eslint.config.mjs | Replaced workspace config with explicit rules, enabled no-explicit-any error |
| cli/package.json | Added ESLint dependencies for TypeScript checking |
| cli/src/shared/modes.ts | Added non-null assertions and proper optional property handling |
| cli/src/types/messages.ts | Migrated to importing shared types from @roo-code/types |
| cli/src/state/atoms/*.ts | Replaced any with unknown and added proper type guards |
| cli/src/services/*.ts | Updated method signatures to use unknown instead of any |
| cli/src/host/*.ts | Added comprehensive type definitions for VSCode API mocks |
| cli/src/commands/*.ts | Updated command handlers with proper typing |
| test files | Fixed test mocks to use proper types instead of any |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
cli/src/host/ExtensionHost.ts:1
- [nitpick] The optional property handling for
currentApiConfigName,listApiConfigMeta, androuterModelsis duplicated in multiple places. Consider extracting this into a helper function to reduce duplication.
| provider: ProviderName | ||
| // Provider-specific fields | ||
| [key: string]: any | ||
| [key: string]: unknown |
There was a problem hiding this comment.
I wonder if it's too much work, but we could make a zod schema which is a union of each specific provider and then use that as the type here, as well as reuse the schema for validation.
| expect(formatPrice(undefined)).toBe("N/A") | ||
| }) | ||
|
|
||
| it("should handle null price", () => { |
There was a problem hiding this comment.
I'd argue it shouldn't, if the type it accepts is number the compiler should complain when there's a type mismatch
Context
Resolve: #3493
Implementation
Screenshots
How to Test