-
Notifications
You must be signed in to change notification settings - Fork 131
Create Satellite Lists Plugin #1250
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
Create Satellite Lists Plugin #1250
Conversation
…e lists This commit adds a new SatelliteListsPlugin that extends the watchlist functionality to support multiple named lists of satellites. Key features include: - Create and manage multiple named satellite lists - Add/remove satellites using 6-digit sccnum - Rename and delete individual lists - Export individual lists as JSON files - Import individual lists from JSON files - Export all lists as a collection - Import collections of lists - Persistent storage in offline mode - Full UI integration with side menu and satellite info box Files added: - src/plugins/satellite-lists/satellite-lists.ts: Main plugin implementation - test/satellite-lists.test.ts: Unit tests for the plugin Files modified: - src/engine/utils/persistence-manager.ts: Added SATELLITE_LISTS storage key - src/plugins/plugins.ts: Registered the new plugin - src/plugins/keeptrack-plugins-configuration.ts: Added plugin configuration type - src/settings/default-plugins.ts: Enabled plugin by default
|
|
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this.addSatToCurrentList(id, true); | ||
| }); | ||
|
|
||
| this.sortCurrentList(); | ||
| this.updateCurrentListDisplay(); |
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.
Persist satellites added via side menu
Adding satellites through the side-menu input never hits persistence: onAddSatellites_ calls addSatToCurrentList(id, true), which skips its internal save, and then exits after sorting/updating the view without calling saveLists(). As a result, satellites added via the UI are lost on refresh/offline reload or export unless some later action (rename/delete/etc.) happens to save the list. Saving the list after the batch add would preserve the new entries.
Useful? React with 👍 / 👎.
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 pull request introduces a new SatelliteListsPlugin that extends the existing watchlist functionality to support multiple named satellite lists. The plugin allows users to create, manage, import, and export collections of satellites for monitoring.
Key Changes
- New plugin implementation with full CRUD operations for satellite lists
- Persistent storage support in offline mode using browser storage
- UI integration with side menu, satellite info box, and optional top menu notification
- JSON import/export functionality for individual lists and collections
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| test/satellite-lists.test.ts | Unit tests for basic plugin functionality including list creation, satellite management, and list operations |
| src/plugins/satellite-lists/satellite-lists.ts | Main plugin implementation with UI, event handling, persistence, and import/export logic |
| src/settings/default-plugins.ts | Added plugin configuration to enable SatelliteListsPlugin by default at order 42 |
| src/plugins/plugins.ts | Registered the new plugin in the plugin manager initialization |
| src/plugins/keeptrack-plugins-configuration.ts | Added TypeScript type definition for plugin configuration |
| src/engine/utils/persistence-manager.ts | Added SATELLITE_LISTS storage key for offline persistence |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| selectSatManagerInstance?.selectSat(id); | ||
| } | ||
|
|
Copilot
AI
Nov 17, 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.
Missing JSDoc documentation. This public method should have documentation describing its parameters and behavior, especially the optional isMultiAdd parameter.
| /** | |
| * Adds a satellite to the currently selected list. | |
| * | |
| * @param {number} id - The NORAD ID of the satellite to add. | |
| * @param {boolean} [isMultiAdd=false] - If true, skips sorting, display update, and saving. | |
| * Used when adding multiple satellites in a batch to avoid redundant operations. | |
| * | |
| * If the satellite is already in the list, a warning is shown and it is not added again. | |
| * When isMultiAdd is false (default), the list is sorted, the display is updated, and the lists are saved. | |
| */ |
| ServiceLocator.getColorSchemeManager().calculateColorBuffers(true); | ||
| } | ||
| } | ||
|
|
Copilot
AI
Nov 17, 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.
Missing JSDoc documentation. This public method should have documentation describing its parameter and return value.
| /** | |
| * Checks if a satellite with the given ID is present in the current list. | |
| * @param id - The ID of the satellite to check. | |
| * @returns True if the satellite is on the current list, false otherwise. | |
| */ |
| ServiceLocator.getLineManager().lines.forEach((line) => { | ||
| if (line instanceof SensorToSatLine && line.sat.id === id) { | ||
| line.isGarbage = true; | ||
| } | ||
| }); |
Copilot
AI
Nov 17, 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.
Inefficient loop: This forEach iterates over all lines in the line manager to mark SensorToSatLine instances as garbage. If there are many lines, this could be a performance bottleneck. Consider maintaining a map of satellite ID to lines for more efficient removal, or at least break early if possible.
| ServiceLocator.getLineManager().lines.forEach((line) => { | |
| if (line instanceof SensorToSatLine && line.sat.id === id) { | |
| line.isGarbage = true; | |
| } | |
| }); | |
| // More efficient: filter only relevant lines and mark as garbage | |
| ServiceLocator.getLineManager().lines | |
| .filter((line) => line instanceof SensorToSatLine && line.sat.id === id) | |
| .forEach((line) => { line.isGarbage = true; }); |
| private clearAllLists() { | ||
| const orbitManagerInstance = ServiceLocator.getOrbitManager(); | ||
|
|
||
| for (const list of this.lists) { | ||
| for (const obj of list.satellites) { | ||
| orbitManagerInstance.removeInViewOrbit(obj.id); | ||
| } | ||
| } | ||
|
|
||
| ServiceLocator.getLineManager().lines.forEach((line) => { | ||
| if (line instanceof SensorToSatLine) { | ||
| line.isGarbage = true; | ||
| } | ||
| }); |
Copilot
AI
Nov 17, 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.
Inefficient nested loops: This code has O(n*m) complexity where n is the number of lists and m is the average number of satellites per list. When clearing all lists, all lines are marked as garbage for every list, which means lines.forEach is called multiple times unnecessarily. Consider consolidating the line cleanup to happen once after all satellites are processed.
| getEl(this.EL.REMOVE_FROM_LIST)!.style.display = 'block'; | ||
| getEl(this.EL.ADD_TO_LIST)!.style.display = 'none'; | ||
| } else { | ||
| getEl(this.EL.ADD_TO_LIST)!.style.display = 'block'; | ||
| getEl(this.EL.REMOVE_FROM_LIST)!.style.display = 'none'; |
Copilot
AI
Nov 17, 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.
Missing null check: getEl(this.EL.REMOVE_FROM_LIST) and getEl(this.EL.ADD_TO_LIST) could return null, but the code uses the non-null assertion operator (!) and directly accesses the style property. This could cause a runtime error if the elements don't exist. Add proper null checks.
| getEl(this.EL.REMOVE_FROM_LIST)!.style.display = 'block'; | |
| getEl(this.EL.ADD_TO_LIST)!.style.display = 'none'; | |
| } else { | |
| getEl(this.EL.ADD_TO_LIST)!.style.display = 'block'; | |
| getEl(this.EL.REMOVE_FROM_LIST)!.style.display = 'none'; | |
| const removeEl = getEl(this.EL.REMOVE_FROM_LIST); | |
| const addEl = getEl(this.EL.ADD_TO_LIST); | |
| if (removeEl) removeEl.style.display = 'block'; | |
| if (addEl) addEl.style.display = 'none'; | |
| } else { | |
| const addEl = getEl(this.EL.ADD_TO_LIST); | |
| const removeEl = getEl(this.EL.REMOVE_FROM_LIST); | |
| if (addEl) addEl.style.display = 'block'; | |
| if (removeEl) removeEl.style.display = 'none'; |
| getEl('satellite-lists-current-name')!.textContent = 'No List Selected'; | ||
| getEl('satellite-lists-list')!.innerHTML = ''; |
Copilot
AI
Nov 17, 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.
Missing null check: getEl('satellite-lists-current-name') and getEl('satellite-lists-list') could return null, but the code uses the non-null assertion operator (!) and directly accesses properties. This could cause a runtime error if the elements don't exist.
| getEl('satellite-lists-current-name')!.textContent = currentList.name; | ||
|
|
||
| let listHtml = ''; | ||
| const catalogManagerInstance = ServiceLocator.getCatalogManager(); | ||
|
|
||
| for (const obj of currentList.satellites) { | ||
| const sat = catalogManagerInstance.getSat(obj.id); | ||
|
|
||
| if (!sat) { | ||
| continue; | ||
| } | ||
|
|
||
| listHtml += ` | ||
| <div class="row"> | ||
| <div class="col s3 m3 l3"> | ||
| <span class="sat-sccnum" style="cursor: pointer;">${sat.sccNum}</span> | ||
| </div> | ||
| <div class="col s7 m7 l7"> | ||
| <span class="sat-name" data-sat-id="${sat.id || 'Unknown'}" style="cursor: pointer;">${sat.name || 'Unknown'}</span> | ||
| </div> | ||
| <div class="col s2 m2 l2 center-align remove-icon"> | ||
| <img class="satellite-list-remove" data-sat-id="${sat.id}" src="${bookmarkRemovePng}" style="cursor: pointer;"></img> | ||
| </div> | ||
| </div>`; | ||
| } | ||
|
|
||
| getEl('satellite-lists-list')!.innerHTML = listHtml; |
Copilot
AI
Nov 17, 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.
Missing null check: getEl('satellite-lists-current-name') and getEl('satellite-lists-list') could return null, but the code uses the non-null assertion operator (!). This could cause a runtime error if the elements don't exist.
|
|
||
| export class SatelliteListsPlugin extends KeepTrackPlugin { | ||
| readonly id = 'SatelliteListsPlugin'; | ||
| dependencies_ = []; |
Copilot
AI
Nov 17, 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 dependencies_ property is defined as an empty array, but the plugin uses several other plugins and services (SatInfoBox, SelectSatManager, TopMenu). If these are required dependencies, they should be declared here to ensure proper initialization order.
| dependencies_ = []; | |
| dependencies_ = ['SatInfoBox', 'SelectSatManager', 'TopMenu']; |
| } | ||
| } | ||
|
|
||
| updateCurrentListDisplay() { |
Copilot
AI
Nov 17, 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.
Missing JSDoc documentation. This public method should have documentation describing what it does, especially since it's a key display update method that could be called from other parts of the codebase.
| import { getEl, hideEl, showEl } from '@app/engine/utils/get-el'; | ||
| import { isThisNode } from '@app/engine/utils/isThisNode'; | ||
| import { PersistenceManager, StorageKey } from '@app/engine/utils/persistence-manager'; | ||
| import { BaseObject, CatalogSource, DetailedSatellite } from '@ootk/src/main'; |
Copilot
AI
Nov 17, 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.
Unused import DetailedSatellite.
| import { BaseObject, CatalogSource, DetailedSatellite } from '@ootk/src/main'; | |
| import { BaseObject, CatalogSource } from '@ootk/src/main'; |
|


…e lists
This commit adds a new SatelliteListsPlugin that extends the watchlist functionality to support multiple named lists of satellites. Key features include:
Files added:
Files modified: