Skip to content

[performance] Satellite - use filters, only calculate selected satellites#1022

Merged
accius merged 4 commits into
accius:Stagingfrom
MichaelWheeley:sat_dev_05262026
May 31, 2026
Merged

[performance] Satellite - use filters, only calculate selected satellites#1022
accius merged 4 commits into
accius:Stagingfrom
MichaelWheeley:sat_dev_05262026

Conversation

@MichaelWheeley
Copy link
Copy Markdown
Collaborator

@MichaelWheeley MichaelWheeley commented May 27, 2026

What does this PR do?

  • refactors satellite filters,
  • passes filters to useSatellites.js and enables calculations only for those selected

Type of change

  • Bug fix
  • New feature
  • Performance improvement
  • Refactor / code cleanup
  • Documentation
  • Translation
  • Map layer plugin

How to test

  1. enable all satellites, confirm all satellites appear, choose at least one to confirm satellite details window is functional
  2. browser devtools -> Performance add 4xCPU throttling
  3. compare none vs all satellites selected

Checklist

  • App loads without console errors
  • Tested in Dark, Light, and Retro themes
  • Responsive at different screen sizes (desktop + mobile)
  • If touching server.js: caches have TTLs and size caps (we serve 2,000+ concurrent users)
  • If adding an API route: includes caching and error handling
  • If adding a panel: wired into Modern, Classic, and Dockable layouts
  • No hardcoded colors — uses CSS variables (var(--accent-cyan), etc.)
  • No .bak, .old, console.log debug lines, or test scripts included

Screenshots (if visual change)

- pass satellite filters to useSatellites component, only calculate position if satellite appears in list otherwise push with uncalculated fake position
@MichaelWheeley MichaelWheeley changed the title Sat dev 05262026 [performance] Satellite - use filters, only calculate selected satellites May 27, 2026
@MichaelWheeley MichaelWheeley marked this pull request as ready for review May 27, 2026 18:54
@MichaelWheeley MichaelWheeley requested a review from accius May 27, 2026 18:54
Comment thread src/hooks/useSatellites.js
accius
accius previously requested changes May 28, 2026
Copy link
Copy Markdown
Owner

@accius accius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perf win here is real and the gating idea is right, but a couple things worth a look before this goes in.

  1. There's a Rules-of-Hooks violation in useSatellitesFilters.js:
export default function useSatellitesFilters(satellitesData, externalState) {
  const state = externalState || useSatelliteFilterState();

useSatelliteFilterState is a hook, so calling it conditionally based on whether externalState is truthy breaks the "hooks must be called in the same order every render" rule. In this PR App.jsx always passes externalState, so at runtime it happens to work, but the lint rule will flag it, and if anyone ever calls useSatellitesFilters(data) without state (the old signature) the hook ordering breaks between renders.

Cleanest fix is to drop the fallback and require externalState since App.jsx is the only caller now. Or split into two genuinely separate hooks (one owns the state, one is pure-filter) and let the caller wire them together.

  1. The default param on useSatellites:
export const useSatellites = (observerLocation, satelliteConfig, filteredNames = []) => {

combined with the isCalcNeeded gate means if anyone calls this without a filteredNames arg, every satellite gets skipped (the empty array's .includes is always false). Today only App.jsx calls it so it's fine, but the default value is doing the opposite of what a "safe default" should do. Maybe default to null and treat null as "no filter, calculate all" so the old behavior is preserved if a caller forgets to pass it.

  1. Smaller question on the placeholders: when a satellite is filtered out you push a zeroed object (lat: 0, lon: 0, track: [], footprintRadius: 0, isVisible: false) instead of just not pushing. Does the satellite list panel, popup, or footprint rendering treat a zeroed sat any differently from an absent one? Probably fine since isVisible: false should gate most of it, but worth a quick sanity check before merge.

Otherwise structure is clean. The 60 * 60 * 1000 readability swap is nice too.

@accius
Copy link
Copy Markdown
Owner

accius commented May 28, 2026

Will look more at this when I land and am home, day got away from me and haven't reviewed the auto triage yet.

…k order

- fix: useSatellites() make parameter filteredNames default to `null` rather than `[]` so that if parameter is missing then all satellites will calculate
- optimize: `useSatellites.js` for case where calculation is not needed push satellite `name` field only, add comment to explain reasoning
@MichaelWheeley MichaelWheeley requested review from accius and lbatalha May 28, 2026 22:47
@MichaelWheeley MichaelWheeley marked this pull request as draft May 29, 2026 00:52
@MichaelWheeley MichaelWheeley marked this pull request as ready for review May 29, 2026 04:01
@MichaelWheeley MichaelWheeley requested a review from accius May 29, 2026 04:02
@accius accius dismissed their stale review May 31, 2026 13:32

All three review items addressed in the latest push. Hook violation fixed (now throws if externalState missing), default param changed to null so a missing filter calculates everything, and the zeroed-placeholder problem replaced with a name-only stub plus defensive Number.isFinite / safeNum / safeStr / safeArr guards across useSatelliteLayer.js. Unblocking the merge gate.

@accius accius merged commit da085c4 into accius:Staging May 31, 2026
5 checks passed
@MichaelWheeley MichaelWheeley deleted the sat_dev_05262026 branch May 31, 2026 14:15
@accius accius mentioned this pull request Jun 2, 2026
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants