-
-
Notifications
You must be signed in to change notification settings - Fork 9
Diagnostics-for-account-and-installations #242
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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`; |
Copilot
AI
Aug 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
Copilot
AI
Aug 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 }; |
Copilot
AI
Aug 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)); |
Copilot
AI
Aug 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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