Conversation
- Add ReportGenerator interface for extensible report registration - Implement static registry allowing plugins to register custom reports - Dynamically generate UI based on registered reports - Migrate existing reports (AER, LLA, ECI, COES) to new system New Reports for Orbital Analysts: - Visibility Windows: Rise/set times with pass duration and max elevation - Sun/Eclipse Analysis: Illumination tracking for power/thermal planning Benefits: - Other plugins can add reports without modifying this plugin - Type-safe report implementation - Separation of concerns - Dynamic UI generation Added EXTENSIBILITY.md with usage examples for plugin developers
|
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Reports Plugin to support extensibility, allowing other plugins to register custom reports without modifying core code. The refactor introduces a registry pattern with a ReportGenerator interface and migrates existing hardcoded reports to use the new architecture.
Key Changes:
- Introduces
ReportGeneratorinterface and static registry pattern for extensible report registration - Converts four existing reports (AER, LLA, ECI, COES) to use the new registration system
- Adds two new reports: Visibility Windows and Sun/Eclipse Analysis
- Implements dynamic UI generation based on registered reports
- Provides comprehensive extensibility documentation
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
| src/plugins/reports/reports.ts | Refactored to implement extensibility pattern with static registry, converted existing reports to generators, added new visibility windows and sun/eclipse reports, and implemented dynamic UI generation |
| src/plugins/reports/EXTENSIBILITY.md | Added comprehensive documentation explaining how to register custom reports and use the new extensibility API |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/plugins/reports/reports.ts
Outdated
| // Simple cylindrical shadow check | ||
| // If satellite is below LEO altitude and on dark side, it's in shadow | ||
| if (satDistance < earthRadius + 1000) { | ||
| return dotProduct > 0; |
There was a problem hiding this comment.
The isSatelliteIlluminated_ method has inconsistent return logic. Line 562 returns dotProduct > 0 when the satellite is below LEO altitude, but this duplicates the check already performed on lines 551-554. The condition on line 561 (satDistance < earthRadius + 1000) checks if satellite distance is less than Earth radius + 1000km, which would include most LEO satellites. However, the check is redundant since we already know dotProduct <= 0 at this point (satellites on the dark side). This code path should likely return false or perform a different check.
| return dotProduct > 0; | |
| // Satellite is below LEO altitude and on the dark side, so it's in shadow | |
| return false; |
src/plugins/reports/reports.ts
Outdated
| if (!isIlluminated && wasIlluminated) { | ||
| // eclipseEntryTime = time; | ||
| } else if (isIlluminated && !wasIlluminated) { | ||
| // sunlightEntryTime = time; | ||
| } |
There was a problem hiding this comment.
The commented-out code that tracks eclipse transitions (lines 477-481) should either be implemented or removed. If this functionality is planned for future implementation, consider adding a TODO comment explaining the intent.
src/plugins/reports/reports.ts
Outdated
| columns: 7, | ||
| isHeaders: true, | ||
| }); | ||
| return 'Umbral'; |
There was a problem hiding this comment.
The getEclipseType_ method's logic is incorrect and will always return "Umbral" for satellites beyond LEO. The method determines eclipse type based solely on satellite altitude (lines 592-596), but both conditions return "Umbral" - satellites below 200km return "Umbral", satellites between 200-500km return "Penumbral", and satellites above 500km also return "Umbral" (line 598). The final return should likely be a different type (e.g., "None" or remove it if all cases are covered by the conditions).
| return 'Umbral'; | |
| return 'None'; |
src/plugins/reports/reports.ts
Outdated
| // let eclipseEntryTime: Date | null = null; | ||
| // let sunlightEntryTime: Date | null = null; |
There was a problem hiding this comment.
The commented-out variables eclipseEntryTime and sunlightEntryTime are declared but never used. These should either be implemented or removed entirely to avoid dead code.
| // let eclipseEntryTime: Date | null = null; | |
| // let sunlightEntryTime: Date | null = null; |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
…alculation and improve extensibility documentation
…bbA6KzZ' of https://github.com/thkruz/keeptrack.space into claude/refactor-reports-extensibility-017ic4BjEWLoQJsfkbbA6KzZ
thkruz
deleted the
claude/refactor-reports-extensibility-017ic4BjEWLoQJsfkbbA6KzZ
branch
|
No description provided.