refactor: eliminate unsafe type assertions in widget components#612
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
|
@copilot Analyse if all task is completed and all code hgh quality functional and tested and is actually used |
Yes, all tasks are complete and the code meets high quality standards: ✅ Task Completion
✅ Code QualityFunctional: All widgets tested with 34 widget-specific tests passing
Type Safety: Full compile-time checking enabled
✅ Code UsageAll new code is actively used:
No dead code, no breaking changes, backward compatible. |
There was a problem hiding this comment.
Pull Request Overview
This PR successfully eliminates 26+ unsafe as any type assertions across widget components by introducing a hasMethod<T, K>() type guard utility and refactoring type-unsafe patterns. The changes improve TypeScript strict mode compliance and type safety while maintaining runtime behavior through proper fallback logic.
Key Changes:
- Added
hasMethod()type guard utility for safe runtime method existence checking with type narrowing - Replaced unsafe
as anycasts with type-safe runtime checks in 4 widget components - Corrected non-existent service method calls (
getImplementationTime→getTotalImplementationTime,getRequiredResources→getRequiredExpertise) - Removed unused interfaces with
[key: string]: anyindex signatures
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/typeGuards.ts | Added hasMethod() type guard with runtime method validation and type narrowing |
| src/components/widgets/implementationguide/TechnicalDetailsWidget.tsx | Replaced 10 as any assertions with hasMethod() checks and getOptionalProperty() helper; improved return type from any to explicit object type |
| src/components/widgets/businessvalue/ValueCreationWidget.tsx | Replaced 8 as any assertions with hasMethod() checks; removed non-existent getROIEstimate() method call with incorrect signature |
| src/components/widgets/businessvalue/CostEstimationWidget.tsx | Replaced as any casts with safer unknown intermediary pattern and runtime validation |
| src/components/widgets/assessmentcenter/SecuritySummaryWidget.tsx | Removed 3 unused interfaces with any index signatures; corrected method names to match actual service interface (getTotalImplementationTime, getRequiredExpertise) |
| export function hasMethod<T extends object, K extends PropertyKey>( | ||
| obj: T | null | undefined, | ||
| methodName: K | ||
| ): obj is T & Record<K, Function> { |
There was a problem hiding this comment.
The return type uses the Function type which is considered too broad in TypeScript strict mode. Consider using a more specific function signature type instead.
Recommendation:
): obj is T & Record<K, (...args: any[]) => any> {This is more type-safe while still being generic enough for the use case.
| ): obj is T & Record<K, Function> { | |
| ): obj is T & Record<K, (...args: any[]) => any> { |
| details && | ||
| typeof details === "object" && | ||
| propertyName in details && | ||
| typeof (details as Record<string, unknown>)[propertyName] === "string" | ||
| ) { | ||
| return (details as Record<string, unknown>)[propertyName] as string; |
There was a problem hiding this comment.
The getOptionalProperty helper still uses type assertions (as Record<string, unknown> and as string), which partially defeats the purpose of this refactoring. Consider using a type guard approach instead:
const getOptionalProperty = (
details: unknown,
propertyName: string,
defaultValue: string
): string => {
if (
details &&
typeof details === "object" &&
propertyName in details
) {
const value = (details as Record<string, unknown>)[propertyName];
if (typeof value === "string") {
return value;
}
}
return defaultValue;
};This version only needs one minimal assertion and validates the type before returning.
| details && | |
| typeof details === "object" && | |
| propertyName in details && | |
| typeof (details as Record<string, unknown>)[propertyName] === "string" | |
| ) { | |
| return (details as Record<string, unknown>)[propertyName] as string; | |
| details !== null && | |
| typeof details === "object" && | |
| Object.prototype.hasOwnProperty.call(details, propertyName) | |
| ) { | |
| const value = (details as { [key: string]: unknown })[propertyName]; | |
| if (typeof value === "string") { | |
| return value; | |
| } |
| if (!isNullish(details) && typeof details === "object") { | ||
| const detailsObj = details as unknown as Record<string, unknown>; | ||
| const expertiseRequired = detailsObj.expertiseRequired; | ||
| if (isArray(expertiseRequired)) { | ||
| return expertiseRequired as string[]; | ||
| } | ||
| if (isString(anyDetails.expertiseRequired)) { | ||
| return [anyDetails.expertiseRequired]; | ||
| if (isString(expertiseRequired)) { | ||
| return [expertiseRequired]; | ||
| } | ||
| } |
There was a problem hiding this comment.
While this is an improvement over as any, the code still uses multiple type assertions. The intermediate unknown cast followed by Record<string, unknown> and then another assertion to string[] creates a chain of casts that could be simplified.
Consider a more direct approach:
if (!isNullish(details) && typeof details === "object" && "expertiseRequired" in details) {
const expertise = (details as { expertiseRequired?: unknown }).expertiseRequired;
if (isArray(expertise) && expertise.every(item => isString(item))) {
return expertise as string[];
}
if (isString(expertise)) {
return [expertise];
}
}This validates the actual content type before casting, making it safer.
|
@copilot apply changes based on the comments in this thread |
- Changed hasMethod return type from Function to (...args: any[]) => any for better type safety - Improved getOptionalProperty to reduce type assertions and use Object.prototype.hasOwnProperty - Enhanced expertise validation in CostEstimationWidget to validate array contents before casting Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
Applied all three code review suggestions in commit a569747:
All 1,295 tests passing ✅ |
| const getOptionalProperty = ( | ||
| details: unknown, | ||
| propertyName: string, | ||
| defaultValue: string | ||
| ): string => { | ||
| if ( | ||
| details !== null && | ||
| typeof details === "object" && | ||
| Object.prototype.hasOwnProperty.call(details, propertyName) | ||
| ) { | ||
| const value = (details as { [key: string]: unknown })[propertyName]; | ||
| if (typeof value === "string") { | ||
| return value; | ||
| } | ||
| } |
There was a problem hiding this comment.
[nitpick] The getOptionalProperty helper still uses type assertions (as Record<string, unknown> and as string) which doesn't fully eliminate unsafe type coercion. While this is safer than the original as any, you could further improve type safety by using a type guard pattern:
const getOptionalProperty = (
details: unknown,
propertyName: string,
defaultValue: string
): string => {
if (
details &&
typeof details === "object" &&
propertyName in details
) {
const value = (details as Record<string, unknown>)[propertyName];
if (typeof value === "string") {
return value;
}
}
return defaultValue;
};This separates the property access from the type check, making the type narrowing clearer.
Pull Request Description
Eliminated 26 unsafe type assertions (
as any) across widget components that violated TypeScript strict mode requirements. The codebase relied on runtime type coercion to access optional service methods and properties, creating potential runtime errors and breaking IntelliSense.Changes
Type Guard Infrastructure
hasMethod<T, K>()utility for runtime method existence checking with proper type narrowinghasMethodreturn type fromFunctionto(...args: any[]) => anyfor stricter TypeScript compliancetypeof (obj as any).method === "function"pattern throughout codebaseWidget Refactoring
(details as any)?.technologies) with optimized typed helper function usingObject.prototype.hasOwnProperty.call()and minimal type assertionsexpertise.every(item => isString(item))) before type casting for improved type safety[key: string]: anyindex signatures; corrected method signatures (getImplementationTime→getTotalImplementationTime)Example
Before:
After:
Code Review Improvements
Based on code review feedback, the following enhancements were made to further improve type safety:
hasMethodType Signature: Changed return type fromFunctionto(...args: any[]) => anyfor better TypeScript strict mode compliancegetOptionalPropertyOptimization: Reduced type assertions and improved null checking usingObject.prototype.hasOwnProperty.call()with single minimal cast and value validationexpertise.every(item => isString(item))before type casting to ensure array elements are actually stringsType of Change
Component(s) Modified
CIA Impact Area
Security Level Impact
Test Coverage Impact
Testing Performed
All 1,295 existing tests pass. TypeScript strict mode compilation successful.
Screenshots/Examples
N/A - Type safety refactoring with no UI changes
Related Issues
Closes #73
Checklist
Additional Notes
Zero remaining
as anyassertions in widget layer. All service method calls now enforce correct signatures at compile time. Enhanced type guards with stricter function signatures and improved runtime validation reduce type assertion chains while maintaining full backward compatibility.Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.