[Feature/sat] Satellite prediction#879
[Feature/sat] Satellite prediction#879MichaelWheeley wants to merge 58 commits intoaccius:Stagingfrom
Conversation
pass tle data from useSatellites.js
# Conflicts: # src/hooks/useSatellites.js
accius
left a comment
There was a problem hiding this comment.
Good progress on satellite prediction — the pass table and auto-refresh are nice additions. A few things to address:
Must fix
XSS risk in onclick handler — Satellite names and TLE lines are injected unescaped into an inline onclick:
onclick="openSatellitePredict('${sat.name}', '${sat.tle1}', '${sat.tle2}')"A satellite name containing ' breaks the handler; malicious content is injectable. Use data- attributes with a delegated event listener, or escape the values.
Interval leak — window.satellitePredictInterval is overwritten if a second modal opens before the first interval fires. The old interval keeps running. Clear the previous one before assigning:
if (window.satellitePredictInterval) clearInterval(window.satellitePredictInterval);Carries all #877 issues — This PR includes #877's commits, so the hardcoded colors, broken nested <table> HTML, duplicate style attribute, and valueAsNumber ?? fallback bugs all apply here too. Fix those in #877 first.
Should fix
Commented-out code — Large blocks in useSatelliteLayer.js (~30 lines) are commented out rather than removed. Either delete them now or open an issue to track it — don't ship dead code in comments.
Hardcoded button colors — background: #440000; border: 1px solid #ff4444 on the satellite name button won't work in Light/Retro themes. Use CSS variables.
Worth discussing
dayjs dependency — Does the project already have date utilities that could handle the format() and diff() calls? Adding a new dependency for a few date operations adds bundle weight. Not a blocker if the team is fine with it.
60s full recomputation — The interval recomputes all orbital passes every minute. Since pass times only shift meaningfully over hours, you could just update the "from now" column without rerunning the orbital math.
color changes replace ?? with || incase of NaN minElev default consistency to 5.0 restrict minElev to -5 to 89
…ed legacy code since satellite location calculations are now done as a hook in useSatellites.js
…no longer updated unless modal is reopened or if 'satellites' is updated. Display Time From Now as hours:minutes:seconds
accius
left a comment
There was a problem hiding this comment.
Review — Satellite prediction modal (draft)
Nice feature. A few things to address before exiting draft:
Correctness / safety
-
String interpolation into
onclickis fragile and XSS-adjacent:onclick="openSatellitePredict('${sat.name}', '${sat.tle1}', '${sat.tle2}')"
A satellite name containing a single quote (or a TLE line containing backslash/newlines) will break the handler or allow HTML injection. TLE source is currently trusted, but please switch to a data-attribute +
addEventListenerpattern, or register the sat by an id lookup in a module-level map and pass only an index. -
Invalid HTML — nested
<table>inside<table>. The sat-card already wraps everything in one<table>, and the PR opens three more nested<table>elements inside its body, plus a stray trailing</table>. Browsers recover, but layout/paint is unpredictable and it will bite future styling changes. Use<tbody>sections with classes instead. -
setInterval memory leak on re-open:
window.satellitePredictIntervalis overwritten without clearing the previous interval when the user clicks another satellite name while a modal is already open. Clear it at the top ofopenSatellitePredictunconditionally. -
keydown listener leak:
handleKeyDownonly removes itself on Enter/Escape. Backdrop-click close and the "replace existing modal" path leave the listener attached, and each re-open adds another. Always remove in the cleanup paths. -
window.openSatellitePredictnever cleaned up in the useEffect return. On hot reload it's fine, but consider returning a cleanup that deletes the global. -
generateModalContent(currentPasses)uses the recomputedcurrentPasses, but the initial render usespassesfrom an earliercomputePassesElevationcall. These two calls happen back-to-back with no TLE change — recompute once and use the single array.
Code quality
-
Inconsistent config access: the component already receives
configviauseLayerprops but the PR addsuseAppConfig()to grabglobalConfig. Pick one path — mixing them means settings changes won't propagate consistently. -
Hardcoded colors (
#302115,#233b46,#00f800,#252e17) in the satellite card HTML break theme support. Please use CSS variables per the checklist. -
Orbit.js modification from upstream — changing
0.5 → 0.05inorbitalPeriodskip factor increases compute cost ~10x. Please add a short comment explaining the observed failure mode that motivated it, and consider whether we can keep the upstream default in the non-first-pass path. -
computePassesSwathimported from upstream but appears unused in this PR. Drop dead code until needed. -
package.json adds
dayjs. The repo already uses plainDate/Intlin many places; adding a date lib just for formatting and.add(7, 'day')is heavyweight. ConsiderIntl.DateTimeFormat+date.getTime() + 7*86400000. -
lastElevationis read but reassignment after loop entry is unused in the branch whereongoingPassis true (just noting for future cleanup).
Dependency
- Stacked on #877. Please mark this ready for review only after #877 lands (or rebase so it's self-contained).
UX
- Modal
width: 50vwwill be unusable on mobile — suggestmin-width+max-widthresponsive handling. Checklist has "Responsive at different screen sizes" unchecked. - Closing on Enter is surprising; Escape is standard. Suggest dropping Enter.
Direction is great — mostly safety and cleanup needed.
…tle1, tle2 from positions
…DX location lock is unchecked
Signed-off-by: Michael R Wheeley <MichaelWheeley@users.noreply.github.com>
still waiting on #877 before this one is ready for review@accius in #879 you said April 5,
now handled in event listener
done
changes from previous PR merged
removed dead code in useSatelliteLayer.js, this appeared to be legacy code, unused because satellite location calculations are now done as a hook in useSatellites.js
Hard-coded colors where removed in earlier PR and merged into this one.
use of
Modified, the intensive satellite calculation now only occurs once as the modal opens. Refresh time is every second and the 'Time From Now' is displayed as +hours:minutes:seconds. Displayed time is replaced with word 'VISIBLE' for a visible satellite, or the whole line is dropped from the table if the satellite has already passed. @accius You also said April 14,
duplicate
duplicate
duplicate
event listeners cleaned-up on exit from main window and from modal.
clean-up added to
duplicate
removed reference to
duplicate
There was a bug in the imported algorithm that masked passes within 0.5 orbital periods of the current time. The bug has been fixed.
removed unused
duplicate
has been cleaned up
modal min/max height and width set as follows, min-width: 50vw;
max-width: 95vw;
min-height: 25vh;
max-height: 90vh;
Keyboard 'Enter' key removed as option to exit modal. |
… 5 degrees. Set default Boulder CO altitude 1630m.
What does this PR do?
Type of change
How to test
Checklist
server.js: caches have TTLs and size caps (we serve 2,000+ concurrent users)var(--accent-cyan), etc.).bak,.old,console.logdebug lines, or test scripts includedScreenshots (if visual change)
each satellite listed is now a PREDICT button - press it to get a prediction modal
