Eclipse/Solar Analysis Plugin

[thkruz]

No description provided.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ thkruz
❌ claude
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[Copilot]

Pull Request Overview

This PR adds a new Eclipse/Solar Analysis plugin that provides comprehensive eclipse prediction and solar geometry analysis capabilities for satellites in KeepTrack.

Key Changes:

• New plugin for analyzing satellite eclipse events, calculating solar beta angles, and predicting eclipse transitions
• Integration with existing satellite tracking and time management systems
• UI for displaying real-time eclipse status, predictions, and statistics with CSV export functionality

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
src/settings/default-plugins.ts	Registers the new EclipseSolarAnalysis plugin with order 93
src/plugins/plugins.ts	Imports and initializes the EclipseSolarAnalysis plugin
src/plugins/keeptrack-plugins-configuration.ts	Adds TypeScript type definition for EclipseSolarAnalysis configuration
src/plugins/eclipse-solar-analysis/eclipse-types.ts	Defines TypeScript interfaces and enums for eclipse events, periods, statistics, and configuration
src/plugins/eclipse-solar-analysis/eclipse-solar-analysis.ts	Implements the main plugin UI, real-time status updates, event handling, and data export
src/plugins/eclipse-solar-analysis/eclipse-calculations.ts	Provides core calculation methods for eclipse predictions, beta angles, and sun geometry
src/app/analysis/sat-math.ts	Modifies eclipse status checking logic and reorganizes imports

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

[nitpick] Event listeners are registered in registerEventHandlers_() but are never removed. If the plugin is reinitialized or the DOM elements are recreated, this could lead to duplicate event listeners being attached.

Consider either:

1. Removing event listeners when the plugin is destroyed/closed
2. Using a flag to ensure handlers are only registered once
3. Using event delegation on a parent element that persists

[Copilot]

A second interval is created for beta angle updates but is never stored or cleared. This causes a memory leak as the interval will continue running even after the menu is closed.

Consider storing this interval reference and clearing it in stopRealTimeUpdates_():

private betaAngleInterval_: NodeJS.Timeout | null = null;

// In startRealTimeUpdates_():
this.betaAngleInterval_ = setInterval(() => {
  this.updateBetaAngle_();
}, 10000);

// In stopRealTimeUpdates_():
if (this.betaAngleInterval_) {
  clearInterval(this.betaAngleInterval_);
  this.betaAngleInterval_ = null;
}

[Copilot]

This condition !(obj.position instanceof Boolean) is logically incorrect. The instanceof Boolean check will never be true for primitive boolean values. If obj.position is a boolean primitive (e.g., false), false instanceof Boolean returns false, so !(false instanceof Boolean) becomes true, which would incorrectly allow the code to proceed.

The intent appears to be checking if obj.position is not a boolean. The correct check should be:

if (obj instanceof BaseObject || typeof obj.position !== 'boolean') {

Suggested change

	if (obj instanceof BaseObject || !(obj.position instanceof Boolean)) {
	if (obj instanceof BaseObject || typeof obj.position !== 'boolean') {

[Copilot]

The type assertions have been removed (e.g., changing obj.position.x as Kilometers to just obj.position.x), but the variables x, y, and z are still declared with the type Kilometers earlier in the function. This could lead to type safety issues if the values don't match the expected type.

Consider either:

1. Keeping the type assertions if the values should be Kilometers
2. Changing the variable declarations to match the actual types

Without seeing the full function context, if these values should be Kilometers, the original code was more type-safe.

[Copilot]

Potential null reference errors when clearing statistics. If any of the elements don't exist, attempting to set the value property on null will throw a runtime error.

Consider adding null checks:

const totalEl = getEl('eclipse-stats-total');
const durationEl = getEl('eclipse-stats-duration');
const fractionEl = getEl('eclipse-stats-fraction');
const avgEl = getEl('eclipse-stats-avg');

if (totalEl) (<HTMLInputElement>totalEl).value = '--';
if (durationEl) (<HTMLInputElement>durationEl).value = '--';
if (fractionEl) (<HTMLInputElement>fractionEl).value = '--';
if (avgEl) (<HTMLInputElement>avgEl).value = '--';

Suggested change

	(<HTMLInputElement>getEl('eclipse-stats-total')).value = '--';
	(<HTMLInputElement>getEl('eclipse-stats-duration')).value = '--';
	(<HTMLInputElement>getEl('eclipse-stats-fraction')).value = '--';
	(<HTMLInputElement>getEl('eclipse-stats-avg')).value = '--';
	const totalEl = getEl('eclipse-stats-total');
	if (totalEl) (<HTMLInputElement>totalEl).value = '--';
	const durationEl = getEl('eclipse-stats-duration');
	if (durationEl) (<HTMLInputElement>durationEl).value = '--';
	const fractionEl = getEl('eclipse-stats-fraction');
	if (fractionEl) (<HTMLInputElement>fractionEl).value = '--';
	const avgEl = getEl('eclipse-stats-avg');
	if (avgEl) (<HTMLInputElement>avgEl).value = '--';

[Copilot]

[nitpick] The HTML input elements use inconsistent ID naming conventions. Most use kebab-case (e.g., eclipse-status-current, beta-angle-current), but the pattern isn't entirely consistent. Consider using a consistent prefix like eclipse- or esa- (for Eclipse Solar Analysis) for all IDs in this plugin to avoid potential conflicts with other plugins.

[Copilot]

The calculateSunGeometry method's sun elevation calculation could be misleading. The comment states "angle above satellite's horizon" but the implementation uses the velocity vector as "forward", which doesn't represent the satellite's actual horizon. The horizon would typically be perpendicular to the position vector (nadir direction). This could confuse future maintainers.

Consider clarifying the documentation to match the implementation:

// Sun elevation angle relative to satellite's velocity vector

Or adjust the implementation to calculate true horizon-based elevation.

[Copilot]

[nitpick] The beta angle rate calculation in updateBetaAngle_() performs a full position propagation 24 hours into the future on every call. Since this is called every 10 seconds when the menu is open, this could be a performance concern for long simulation runs or when the propagation is computationally expensive.

Consider:

1. Caching the beta angle rate and updating it less frequently (e.g., every minute)
2. Using a numerical derivative with a shorter time step (e.g., 1 hour) for faster computation
3. Only calculating the rate when specifically requested by the user

Suggested change

	// Calculate beta angle rate (change over 1 day)
	const nextDayTime = new Date(currentTime.getTime() + 24 * 3600 * 1000);
	const nextDayBeta = EclipseCalculations.calculateSolarBetaAngle(this.currentSatellite_, nextDayTime);
	const betaRate = nextDayBeta - betaAngle;
	const rateEl = getEl('beta-angle-rate');
	if (rateEl) {
	(<HTMLInputElement>rateEl).value = `${betaRate.toFixed(2)} °/day`;
	// Calculate beta angle rate (change over 1 hour)
	const nextHourTime = new Date(currentTime.getTime() + 1 * 3600 * 1000);
	const nextHourBeta = EclipseCalculations.calculateSolarBetaAngle(this.currentSatellite_, nextHourTime);
	const betaRate = nextHourBeta - betaAngle;
	const rateEl = getEl('beta-angle-rate');
	if (rateEl) {
	(<HTMLInputElement>rateEl).value = `${betaRate.toFixed(2)} °/hr`;

[Copilot]

When no eclipse periods are found, minEclipseDuration is set to 0 as Milliseconds, but the variable minDuration is initialized to Infinity. This is inconsistent. If periods exist but all have very long durations, minDuration would remain as Infinity after the loop (though this scenario is unlikely).

Consider initializing differently:

let minDuration = periods.length > 0 ? periods[0].duration : 0;

Or ensure the return value is consistent:

minEclipseDuration: (minDuration === Infinity ? 0 : minDuration) as Milliseconds,

Suggested change

	minEclipseDuration: minDuration as Milliseconds,
	minEclipseDuration: (minDuration === Infinity ? 0 : minDuration) as Milliseconds,

[sonarqubecloud]

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud