Diagnostics-for-account-and-installations#242
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
@austenstone can you approve and merge this? |
…rving diagnostics - Added recalculate targets functionality with save all button - Enhanced time-saved chart with dynamic Y-axis and target lines - Fixed type issues in target-calculation-service.ts using parseFloat for string conversions - Simplified status.service.ts repositories API call - Added value-modeling-explanation.md documentation - Maintained all diagnostic functionality and type safety
frontend/src/app/main/copilot/copilot-value-modeling/copilot-value-modeling.component.ts
Fixed
Show fixed
Hide fixed
…and defining RecalculateTargetsResponse type
There was a problem hiding this comment.
Pull Request Overview
This PR creates a comprehensive diagnostics page to debug GitHub App accounts and installations. The implementation adds installation validation capabilities, value modeling enhancements, and a user-friendly diagnostics interface.
- Adds a new diagnostics component with detailed installation validation and error reporting
- Enhances value modeling with target recalculation and improved numeric parsing
- Includes type definitions for comprehensive diagnostic data structures
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/app/types/diagnostics.types.ts | Type definitions for diagnostic response structures |
| frontend/src/app/services/api/targets.service.ts | Added recalculate targets endpoint with response types |
| frontend/src/app/services/api/setup.service.ts | Added installation validation service method |
| frontend/src/app/main/main.component.html | Added diagnostics navigation menu item |
| frontend/src/app/main/diagnostics/main-diagnostics.component.ts | Main diagnostics component with validation UI and details dialog |
| frontend/src/app/main/copilot/copilot-value/time-saved-chart/time-saved-chart.component.ts | Dynamic chart scaling based on target values |
| frontend/src/app/main/copilot/copilot-value-modeling/copilot-value-modeling.component.ts | Added target recalculation functionality |
| frontend/src/app/main/copilot/copilot-value-modeling/copilot-value-modeling.component.html | Added recalculate and save buttons |
| frontend/src/app/app.routes.ts | Added diagnostics route configuration |
| backend/src/services/value_modeling_doc.md | Documentation for value modeling calculations |
| backend/src/services/value-modeling-explanation.md | Detailed value modeling explanation |
| backend/src/services/target-calculation-service.ts | Enhanced numeric parsing and improved default value handling |
| backend/src/services/status.service.ts | Fixed repository data structure access |
| backend/src/routes/index.ts | Added validate-installations route |
| backend/src/controllers/setup.controller.ts | Added comprehensive installation validation endpoint |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const url = URL.createObjectURL(dataBlob); | ||
| const link = document.createElement('a'); | ||
| link.href = url; | ||
| link.download = `installation-diagnostics-${new Date().toISOString().split('T')[0]}.json`; |
There was a problem hiding this comment.
This download logic is duplicated in the dialog component. Consider extracting this functionality into a shared utility service to avoid code duplication.
| link.download = `installation-diagnostics-${new Date().toISOString().split('T')[0]}.json`; | |
| MainDiagnosticsComponent.downloadJsonFile( | |
| this.lastResult, | |
| `installation-diagnostics-${new Date().toISOString().split('T')[0]}.json` | |
| ); | |
| } | |
| // Shared utility for downloading JSON data as a file | |
| private static downloadJsonFile(data: any, filename: string): void { | |
| const dataStr = JSON.stringify(data, null, 2); | |
| const dataBlob = new Blob([dataStr], { type: 'application/json' }); | |
| const url = URL.createObjectURL(dataBlob); | |
| const link = document.createElement('a'); | |
| link.href = url; | |
| link.download = filename; |
| link.href = url; | ||
| link.download = `installation-diagnostics-${new Date().toISOString().split('T')[0]}.json`; | ||
| link.click(); | ||
| URL.revokeObjectURL(url); |
There was a problem hiding this comment.
This is identical to the downloadResults method in the main component. Consider extracting this functionality into a shared utility service to avoid code duplication.
| URL.revokeObjectURL(url); | |
| const filename = `installation-diagnostics-${new Date().toISOString().split('T')[0]}.json`; | |
| this.downloadService.downloadJson(this.data, filename); |
| const distinctUsers = this.getDistinctSurveyUsers(this.surveysWeekly); | ||
| if (distinctUsers.length === 0) { | ||
| return { current: 0, target: 0, max: 10 }; | ||
| return { current: 2, target: 2, max: 10 }; |
There was a problem hiding this comment.
The magic number 2 for default hours appears multiple times in this method. Consider defining this as a named constant at the class level to improve maintainability.
Note: See the diff below for a potential fix:
@@ -703,15 +708,15 @@
* Calculate weekly time saved in hours per developer
*/
calculateWeeklyTimeSavedHrs(): Target {
- // If no surveys, return default values with 2 hrs current
+ // If no surveys, return default values with DEFAULT_WEEKLY_TIME_SAVED_HRS current
if (this.surveysWeekly.length === 0) {
- return { current: 2, target: 2, max: 10 };
+ return { current: TargetCalculationService.DEFAULT_WEEKLY_TIME_SAVED_HRS, target: TargetCalculationService.DEFAULT_WEEKLY_TIME_SAVED_HRS, max: 10 };
}
// Get distinct users who submitted surveys
const distinctUsers = this.getDistinctSurveyUsers(this.surveysWeekly);
if (distinctUsers.length === 0) {
- return { current: 2, target: 2, max: 10 };
+ return { current: TargetCalculationService.DEFAULT_WEEKLY_TIME_SAVED_HRS, target: TargetCalculationService.DEFAULT_WEEKLY_TIME_SAVED_HRS, max: 10 };
}
// Group surveys by user to get average time saved per user
|
|
||
| // Use default value of 2 if calculated value is 0 or very small | ||
| const currentValue = avgWeeklyTimeSaved < 0.1 ? 2 : this.roundToDecimal(avgWeeklyTimeSaved); | ||
| const targetValue = avgWeeklyTimeSaved < 0.1 ? 3 : this.roundToDecimal(Math.min(avgWeeklyTimeSaved * 1.5, maxWeeklyTimeSaved * 0.8)); |
There was a problem hiding this comment.
The magic numbers 2, 3, and 0.1 should be defined as named constants. This will improve code readability and make it easier to adjust these thresholds in the future.
| const targetValue = avgWeeklyTimeSaved < 0.1 ? 3 : this.roundToDecimal(Math.min(avgWeeklyTimeSaved * 1.5, maxWeeklyTimeSaved * 0.8)); | |
| const currentValue = avgWeeklyTimeSaved < MIN_WEEKLY_TIME_SAVED_THRESHOLD ? DEFAULT_WEEKLY_TIME_SAVED : this.roundToDecimal(avgWeeklyTimeSaved); | |
| const targetValue = avgWeeklyTimeSaved < MIN_WEEKLY_TIME_SAVED_THRESHOLD ? DEFAULT_TARGET_WEEKLY_TIME_SAVED : this.roundToDecimal(Math.min(avgWeeklyTimeSaved * 1.5, maxWeeklyTimeSaved * 0.8)); |
PR for creating a diagnostics page to debug accounts and installations