feat: add real-time satellite flyby notifications - #175
Conversation
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
github-actions
Bot
added
bug
codeant-ai
Bot
added
the
size:XL
📝 WalkthroughWalkthroughThe PR adds periodic satellite flyby detection, typed notification state, animated alerts, notification history, audio preferences, tracking actions, and shared orbital calculation utilities. It also wires flyby history into ChangesFlyby notification flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MainLayout
participant NotificationCenter
participant useFlybyEngine
participant SatelliteCatalog
participant useNotificationStore
participant FlybyNotification
MainLayout->>NotificationCenter: render notification center
NotificationCenter->>useFlybyEngine: start periodic flyby checks
useFlybyEngine->>SatelliteCatalog: fetch selected satellite data
useFlybyEngine->>useNotificationStore: create qualifying notification
NotificationCenter->>FlybyNotification: render active alert
FlybyNotification->>useNotificationStore: dismiss or track alert
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| const ctx = new AudioContext(); | ||
| const osc = ctx.createOscillator(); | ||
| const gainNode = ctx.createGain(); |
There was a problem hiding this comment.
Suggestion: Each alert creates a new AudioContext, but the context is never closed after the oscillator stops. Repeated flyby alerts or preference-triggered effect reruns therefore retain audio contexts and can eventually hit browser audio-resource limits; close the context after playback completes. [resource leak]
Severity Level: Major ⚠️
- ⚠️ Long-running sessions accumulate audio contexts.
- ⚠️ Repeated flybys increase browser audio-resource usage.
- ⚠️ Excessive alerts may eventually prevent further sound playback.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** frontend/src/components/ui/FlybyNotification.tsx
**Line:** 18:20
**Comment:**
*Resource Leak: Each alert creates a new `AudioContext`, but the context is never closed after the oscillator stops. Repeated flyby alerts or preference-triggered effect reruns therefore retain audio contexts and can eventually hit browser audio-resource limits; close the context after playback completes.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| useEffect(() => { | ||
| if (preferences.soundEnabled) { | ||
| playBeep(); | ||
| } | ||
|
|
||
| // Optional: Use browser notifications API if permitted | ||
| if (Notification.permission === 'granted') { | ||
| new Notification(`Flyby Alert: ${notification.satelliteName}`, { | ||
| body: `Approaching ${notification.locationName}. ETA: ${notification.eta.toLocaleTimeString()}`, | ||
| icon: '/vite.svg' | ||
| }); | ||
| } else if (Notification.permission !== 'denied') { | ||
| Notification.requestPermission(); | ||
| } | ||
| }, [notification, preferences.soundEnabled]); |
There was a problem hiding this comment.
Suggestion: The effect depends on the entire notification object and preferences.soundEnabled, so changing the audio preference reruns it for every currently active toast. This replays browser notifications for old alerts and can replay their sounds when audio is enabled; trigger the alert side effects only when a notification is newly created, using a stable notification identifier. [logic error]
Severity Level: Major ⚠️
- ⚠️ Toggling Audio Alerts repeats sounds for active flyby toasts.
- ⚠️ Existing alerts can produce duplicate browser notifications.
- ⚠️ Preference changes cause unwanted alert side effects.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** frontend/src/components/ui/FlybyNotification.tsx
**Line:** 45:59
**Comment:**
*Logic Error: The effect depends on the entire `notification` object and `preferences.soundEnabled`, so changing the audio preference reruns it for every currently active toast. This replays browser notifications for old alerts and can replay their sounds when audio is enabled; trigger the alert side effects only when a notification is newly created, using a stable notification identifier.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| export function useFlybyEngine() { | ||
| const { selectedSatelliteIds } = useUIStore(); | ||
| const { preferences, addNotification } = useNotificationStore(); | ||
| const { bookmarks } = useBookmarkStorage(); |
There was a problem hiding this comment.
Suggestion: The engine owns a separate useBookmarkStorage() state instance from the one used by EarthTwin through useBookmarks(). Bookmark changes made in the globe therefore update local storage but do not update this engine's bookmarks state, so newly added, edited, or deleted locations are not reflected in flyby checks until the engine is remounted. [stale reference]
Severity Level: Major ⚠️
- ⚠️ Bookmark-based flyby targets remain stale.
- ⚠️ Added locations are omitted from notification scans.
- ⚠️ Deleted locations may continue receiving checks.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** frontend/src/hooks/useFlybyEngine.ts
**Line:** 17:17
**Comment:**
*Stale Reference: The engine owns a separate `useBookmarkStorage()` state instance from the one used by `EarthTwin` through `useBookmarks()`. Bookmark changes made in the globe therefore update local storage but do not update this engine's `bookmarks` state, so newly added, edited, or deleted locations are not reflected in flyby checks until the engine is remounted.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/components/ui/FlybyNotification.tsx`:
- Around line 18-35: Update playBeep to reuse a single persistent AudioContext
stored in module state instead of constructing one per alert; initialize it
lazily when needed, and create each oscillator and gain node from that context
while preserving the existing beep timing and warning behavior.
- Around line 45-59: Update the FlybyNotification delivery logic in the
useEffect so browser alerts are emitted only once per notification.id, even when
preferences.soundEnabled changes or the component re-renders. Track delivered
IDs with a suitable persistent set or split the sound effect from the
browser-notification effect, ensuring sound preference changes only affect
playback and do not recreate browser notifications.
- Line 67: Update FlybyNotification’s ETA calculation to use a current timestamp
state rather than only the render-time Date.now() value. Add minute-based
updates while the toast remains visible, clean up the timer when dismissed or
unmounted, and derive minutesAway from that tracked timestamp so the displayed
countdown refreshes each minute.
In `@frontend/src/hooks/useFlybyEngine.ts`:
- Around line 41-48: Update the checkFlybys flow in useFlybyEngine’s useEffect
to track whether the effect is still active, and have the async response
handling return before adding notifications when cleanup has run. Set that
inactive state in the effect cleanup alongside clearing the interval, preserving
notification behavior for requests belonging to the current tracked-satellite
selection.
- Around line 31-37: Update the geolocation failure callback and the
non-geolocation branch in the surrounding hook to leave userLocationRef.current
null instead of assigning the fallback coordinates (0, 0). Preserve the
successful geolocation assignment so flyby alerts are created only after a real
location is available.
In `@frontend/src/store/notificationStore.ts`:
- Around line 39-43: Update the duplicate check in the notification store’s
isDuplicate logic to remove the !n.dismissed condition. Duplicate suppression
must compare satelliteId, locationName, and the five-minute ETA window
regardless of dismissal state, while dismissed remains display-only.
In `@frontend/src/utils/orbitCalc.ts`:
- Around line 20-23: Validate the epoch in the orbit propagation logic before
calculating elapsedDays: when an explicitly provided obj.epoch produces an
invalid Date, return the function’s established invalid-input result rather than
propagating NaN values. Keep the default current-date behavior for a missing
epoch and preserve normal propagation for valid epochs.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4fabf9f3-a4d9-445d-b035-3bd70b1fe25c
📒 Files selected for processing (9)
frontend/src/components/EarthTwin.tsxfrontend/src/components/layouts/MainLayout.tsxfrontend/src/components/ui/FlybyNotification.tsxfrontend/src/components/ui/NotificationCenter.tsxfrontend/src/hooks/useFlybyEngine.tsfrontend/src/store/notificationStore.tsfrontend/src/store/uiStore.tsfrontend/src/utils/orbitCalc.tsfrontend/tsconfig.app.json
💤 Files with no reviewable changes (1)
- frontend/tsconfig.app.json
| const ctx = new AudioContext(); | ||
| const osc = ctx.createOscillator(); | ||
| const gainNode = ctx.createGain(); | ||
|
|
||
| osc.type = 'sine'; | ||
| osc.frequency.setValueAtTime(880, ctx.currentTime); // A5 | ||
| osc.frequency.exponentialRampToValueAtTime(440, ctx.currentTime + 0.1); // Drop to A4 | ||
|
|
||
| gainNode.gain.setValueAtTime(0.1, ctx.currentTime); | ||
| gainNode.gain.exponentialRampToValueAtTime(0.01, ctx.currentTime + 0.5); | ||
|
|
||
| osc.connect(gainNode); | ||
| gainNode.connect(ctx.destination); | ||
|
|
||
| osc.start(); | ||
| osc.stop(ctx.currentTime + 0.5); | ||
| } catch (e) { | ||
| console.warn('Audio play failed', e); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate file:"
fd -a 'FlybyNotification\.tsx$' . || true
echo
echo "File contents/outline:"
file="$(fd 'FlybyNotification\.tsx$' . | head -n1 || true)"
if [ -n "${file:-}" ]; then
wc -l "$file"
ast-grep outline "$file" --view compact || true
echo
cat -n "$file"
fi
echo
echo "Search for playBeep/usages:"
rg -n "playBeep|AudioContext|FlybyNotification" frontend src . 2>/dev/null | head -n 200 || trueRepository: 7-Blocks/Kepler
Length of output: 9976
🌐 Web query:
MDN AudioContext close recommended practice after use
💡 Result:
The recommended practice for using AudioContext is to create a single instance and reuse it throughout your application's lifecycle, rather than creating and closing it frequently [1]. When you no longer need an AudioContext, calling the close method is the standard way to release system audio resources, such as those that might block the creation of additional contexts or impact CPU/battery usage [2][1][3]. Key considerations regarding AudioContext.close: 1. Purpose: The primary purpose of close is to forcibly release system audio resources, stop audio processing, and suspend the progression of audio time [2][1]. 2. Resource Management: Calling close does not automatically release all objects created by the AudioContext; it specifically targets system audio resources [2][3]. Garbage collection will eventually handle the memory associated with the context and its nodes once you release your references to them (e.g., by setting the variable to null) [4]. 3. Promise-based: The close method returns a Promise [2]. It is good practice to wait for this promise to resolve if you need to ensure all blocking resources are released before proceeding (for example, before attempting to initialize a new context) [2][5]. 4. Alternatives for Temporary Pauses: If you only need to temporarily halt audio output or reduce CPU/battery consumption without destroying the context entirely, consider using the suspend method instead of close [1]. In summary, use close when you are finished with the audio functionality for an extended period or the application lifecycle ends [2][1]. For active applications, prefer reusing a single instance and using suspend/resume to manage temporary interruptions or state changes [6][1][7].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/AudioContext
- 2: https://developer.mozilla.org/en-US/docs/Web/API/AudioContext/close
- 3: https://googlechrome.github.io/samples/webaudio-audiocontext-close/
- 4: https://stackoverflow.com/questions/37118355/how-to-release-audiocontext-in-microsoft-edge
- 5: https://github.com/mdn/content/blob/main/files/en-us/web/api/audiocontext/close/index.md
- 6: https://developer.mozilla.org/en-US/docs/Web/API/Web_Audio_API/Best_practices
- 7: https://developer.mozilla.org/en-US/docs/Web/API/BaseAudioContext/state
🌐 Web query:
Web Audio API AudioContext context limits number of active contexts browser
💡 Result:
The Web Audio API specification does not mandate a specific maximum number of active AudioContext instances [1][2]. Instead, it allows browser implementations to define their own limits [2]. When this limit is exceeded, an implementation may throw a NotSupportedError when attempting to create a new AudioContext [2]. Because limits are implementation-defined, they vary across browsers and platforms: - Browser-Specific and Platform-Specific Limits: Browsers may impose different caps based on the underlying OS or hardware [3][2]. For instance, source code for the WebKit engine (used by Safari) has historically included platform-specific constraints, such as a limit of four hardware contexts on Windows [3]. - Historical Context in Chrome: Older versions of Google Chrome (prior to version 66) enforced a limit of six AudioContext instances per tab [1][4]. - Best Practices: It is widely recommended to create a single AudioContext and reuse it throughout your application rather than initializing multiple instances [5]. This is both more resource-efficient and avoids potential issues with browser-imposed limits or performance bottlenecks [5]. If your application requires multiple audio graphs, you should manage them within a single AudioContext or use OfflineAudioContext for non-real-time rendering tasks where applicable [5].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/AudioContext/AudioContext
- 2: https://www.w3.org/TR/webaudio-1.1/
- 3: https://github.com/WebKit/WebKit/blob/main/Source/WebCore/Modules/webaudio/AudioContext.cpp
- 4: https://github.com/mdn/content/blob/main/files/en-us/web/api/audiocontext/audiocontext/index.md
- 5: https://developer.mozilla.org/en-US/docs/Web/API/AudioContext
Reuse a single AudioContext instead of creating one per alert.
playBeep creates a new AudioContext for every notification while never closing or reusing it. Keep one persistent context in module state and have each beep start/stop on that context to avoid context limits and resource buildup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/components/ui/FlybyNotification.tsx` around lines 18 - 35,
Update playBeep to reuse a single persistent AudioContext stored in module state
instead of constructing one per alert; initialize it lazily when needed, and
create each oscillator and gain node from that context while preserving the
existing beep timing and warning behavior.
| useEffect(() => { | ||
| if (preferences.soundEnabled) { | ||
| playBeep(); | ||
| } | ||
|
|
||
| // Optional: Use browser notifications API if permitted | ||
| if (Notification.permission === 'granted') { | ||
| new Notification(`Flyby Alert: ${notification.satelliteName}`, { | ||
| body: `Approaching ${notification.locationName}. ETA: ${notification.eta.toLocaleTimeString()}`, | ||
| icon: '/vite.svg' | ||
| }); | ||
| } else if (Notification.permission !== 'denied') { | ||
| Notification.requestPermission(); | ||
| } | ||
| }, [notification, preferences.soundEnabled]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate FlybyNotification.tsx"
fd -a 'FlybyNotification\.tsx$' . || true
echo
echo "Show relevant file outline and lines"
file="$(fd 'FlybyNotification\.tsx$' . | head -n 1)"
if [ -n "$file" ]; then
echo "FILE=$file"
wc -l "$file"
cat -n "$file"
fi
echo
echo "Search notification data shape and prefs usage"
rg -n "soundEnabled|FlybyNotification|notification/id|id:" -S .Repository: 7-Blocks/Kepler
Length of output: 30719
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Show notification store"
cat -n frontend/src/store/notificationStore.ts
echo
echo "Show NotificationCenter component"
cat -n frontend/src/components/ui/NotificationCenter.tsx
echo
echo "Static verifier: FlybyNotification effect dependencies and delivery side effects"
python3 - <<'PY'
from pathlib import Path
import re
p = Path('frontend/src/components/ui/FlybyNotification.tsx')
text = p.read_text()
m = re.search(r"useEffect\s*\(\s*\(\)\s*=>\s*\{(.*?)\n\s*\}\s*,\s*\[(.*?)\]\s*\);", text, re.S)
if not m:
print("NO_DETECTED_EFFECT")
raise SystemExit(1)
body = m.group(1)
deps = [d.strip() for d in m.group(2).split(',') if d.strip()]
print("deps=", deps)
print("has_sound_effect=", "preferences.soundEnabled" in body)
print("has_create_notification=", "new Notification(" in body)
print("has_request_permission=", "Notification.requestPermission(" in body)
uses_notification_id = bool(re.search(r"notification\.id", body))
uses_sound_enabled = bool(re.search(r"preferences\.soundEnabled", body))
uses_notification_name_or_eta = bool(re.search(r"notification\.(satelliteName|locationName|eta)", body))
print("uses_notification_id_in_effect=", uses_notification_id)
print("uses_sound_enabled_in_effect=", uses_sound_enabled)
print("uses_notification_field_in_effect=", uses_notification_name_or_eta)
print("issue_sound_dependency_retriggers=", uses_sound_enabled)
print("issue_notification_creates_on_each_retrieval=", uses_notification_id is False and uses_notification_name_or_eta)
PYRepository: 7-Blocks/Kepler
Length of output: 10860
Deliver alert alerts only on first appearance.
preferences.soundEnabled changes the dependency for every active FlybyNotification, so changing audio settings re-runs delivery for dismissed/old toasts. Track notification.id in a delivery set and create only the browser notification, or split the sound and browser-notification effects so audio changes do not create new browser notifications.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/components/ui/FlybyNotification.tsx` around lines 45 - 59,
Update the FlybyNotification delivery logic in the useEffect so browser alerts
are emitted only once per notification.id, even when preferences.soundEnabled
changes or the component re-renders. Track delivered IDs with a suitable
persistent set or split the sound effect from the browser-notification effect,
ensuring sound preference changes only affect playback and do not recreate
browser notifications.
| dismiss(notification.id); | ||
| }; | ||
|
|
||
| const minutesAway = Math.max(0, Math.round((notification.eta.getTime() - Date.now()) / 60000)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching FlybyNotification:"
fd -a 'FlybyNotification\.tsx$' . || true
file="$(fd 'FlybyNotification\.tsx$' . | head -n1 || true)"
if [ -n "${file:-}" ]; then
echo
echo "Line count:"
wc -l "$file"
echo
echo "File contents with line numbers:"
cat -n "$file"
fi
echo
echo "Search for FlybyNotification usages:"
rg -n "FlybyNotification|eta|minutesAway" . --glob '!node_modules' --glob '!dist' --glob '!build' || trueRepository: 7-Blocks/Kepler
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Matching files:"
fd -a 'FlybyNotification\.tsx$|FlybyNotification\.ts$' . || true
echo
echo "Search focused usages:"
rg -n "FlybyNotification|minutesAway|Date\.now\(\)|eta\.getTime\(\)|T-\{minutesAway MIN\}\"" . \
--glob '!node_modules' --glob '!build' --glob '!dist' \
--glob '!*.svg' --glob '!*.png' --glob '!*.jpg' --glob '!*.jpeg' \
--glob '!*.webp' --glob '!*.gif' --glob '!*.ico' --glob '!*.woff*' || trueRepository: 7-Blocks/Kepler
Length of output: 3899
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd 'FlybyNotification\.tsx$' . | head -n1 || true)"
echo "FlybyNotification file: $file"
wc -l "$file"
echo
sed -n '1,140p' "$file" | cat -n
echo
echo "NotificationStore relevant lines:"
sed -n '1,120p' frontend/src/store/notificationStore.ts | cat -n
echo
echo "NotificationCenter relevant lines:"
sed -n '1,80p' frontend/src/components/ui/NotificationCenter.tsx | cat -nRepository: 7-Blocks/Kepler
Length of output: 14011
🌐 Web query:
React Framer Motion AnimatePresence motion.div timer interval state cleanup clear on unmount
💡 Result:
To correctly handle timer or interval cleanup within a component being unmounted by AnimatePresence, you must leverage the useEffect cleanup function in combination with Framer Motion's usePresence hook [1][2][3]. When a component is removed from the React tree but wrapped in AnimatePresence, it remains mounted in the DOM while its exit animation runs [4][5]. Standard useEffect cleanup functions will run as soon as the component is removed from the React tree, which may stop your timers prematurely or lead to errors if you attempt to update state on an unmounted component [6][7]. To manage this properly: 1. Use the usePresence hook to track the component's exit status [2][3]. 2. Create your timer or interval inside a useEffect that depends on the isPresent state returned by usePresence [2][3]. 3. If you need to manually control when the component is finally removed (e.g., waiting for a timer to finish alongside an animation), use the safeToRemove callback returned by usePresence [1][2][3]. Example pattern for delayed removal with a timer: import { useEffect } from 'react'; import { usePresence, motion } from 'motion/react'; function DelayedComponent { const [isPresent, safeToRemove] = usePresence; useEffect( => { if (!isPresent) { // Logic to run during exit phase (e.g., wait 1s) const timer = setTimeout(safeToRemove, 1000); // Cleanup the timer if the component is somehow // forced to unmount before the timer finishes return => clearTimeout(timer); } }, [isPresent, safeToRemove]); return <motion.div exit={{ opacity: 0 }}>Content</motion.div>; } By including a cleanup function (return => clearTimeout(timer)), you ensure that no memory leaks or errors occur if the exit state changes rapidly [6]. If your goal is simply to stop an interval from trying to update state on an unmounted component, the standard useEffect cleanup is sufficient; however, if you are coordinating custom exit timing, usePresence and safeToRemove are the authoritative tools [1][2][7].
Citations:
- 1: https://motion.dev/docs/react-animate-presence
- 2: https://www.mintlify.com/motiondivision/motion/api/use-presence
- 3: https://www.userinterface.wiki/mastering-animate-presence
- 4: https://github.com/framer/motion/blob/main/packages/framer-motion/src/components/AnimatePresence/index.tsx
- 5: https://dev.to/regexyl/how-animatepresence-in-framer-motion-works-161p
- 6: https://react.dev/reference/react/useEffect
- 7: Unable to sync setInterval() and motion animation[BUG] motiondivision/motion#324
Update the ETA each minute.
Date.now() is cached in render, so T-{minutesAway} MIN stays fixed while the toast is rendered with no other FlybyNotification state changes. Track a current timestamp state and update it until the toast is dismissed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/components/ui/FlybyNotification.tsx` at line 67, Update
FlybyNotification’s ETA calculation to use a current timestamp state rather than
only the render-time Date.now() value. Add minute-based updates while the toast
remains visible, clean up the timer when dismissed or unmounted, and derive
minutesAway from that tracked timestamp so the displayed countdown refreshes
each minute.
| (error) => { | ||
| console.warn('Geolocation denied or failed, using default location (0,0).', error); | ||
| userLocationRef.current = { lat: 0, lon: 0 }; | ||
| } | ||
| ); | ||
| } else { | ||
| userLocationRef.current = { lat: 0, lon: 0 }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not use (0, 0) as a location fallback.
When geolocation fails, this code adds Null Island as a notification target. The engine then creates flyby alerts for an unrelated location. Keep userLocationRef.current null until location access succeeds.
Proposed fix
(error) => {
- console.warn('Geolocation denied or failed, using default location (0,0).', error);
- userLocationRef.current = { lat: 0, lon: 0 };
+ console.warn('Geolocation denied or failed. Current-location flyby alerts are disabled.', error);
+ userLocationRef.current = null;
}
);
} else {
- userLocationRef.current = { lat: 0, lon: 0 };
+ userLocationRef.current = null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (error) => { | |
| console.warn('Geolocation denied or failed, using default location (0,0).', error); | |
| userLocationRef.current = { lat: 0, lon: 0 }; | |
| } | |
| ); | |
| } else { | |
| userLocationRef.current = { lat: 0, lon: 0 }; | |
| (error) => { | |
| console.warn('Geolocation denied or failed. Current-location flyby alerts are disabled.', error); | |
| userLocationRef.current = null; | |
| } | |
| ); | |
| } else { | |
| userLocationRef.current = null; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/hooks/useFlybyEngine.ts` around lines 31 - 37, Update the
geolocation failure callback and the non-geolocation branch in the surrounding
hook to leave userLocationRef.current null instead of assigning the fallback
coordinates (0, 0). Preserve the successful geolocation assignment so flyby
alerts are created only after a real location is available.
| useEffect(() => { | ||
| if (selectedSatelliteIds.length === 0) return; | ||
|
|
||
| const checkFlybys = async () => { | ||
| try { | ||
| // Fetch data for all tracked satellites | ||
| const satPromises = selectedSatelliteIds.map(id => api.getCatalogObjectByNorad(id)); | ||
| const responses = await Promise.allSettled(satPromises); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Stop stale checks from adding notifications.
Cleanup clears the interval but does not stop checkFlybys calls already awaiting API responses. If tracked satellites change or the component unmounts during a request, the old call can add notifications for stale satellite IDs.
Proposed fix
useEffect(() => {
if (selectedSatelliteIds.length === 0) return;
+ let isCurrent = true;
const checkFlybys = async () => {
try {
// Fetch data for all tracked satellites
const satPromises = selectedSatelliteIds.map(id => api.getCatalogObjectByNorad(id));
const responses = await Promise.allSettled(satPromises);
+ if (!isCurrent) return;
const satellites = responses
@@
- return () => clearInterval(intervalId);
+ return () => {
+ isCurrent = false;
+ clearInterval(intervalId);
+ };
}, [selectedSatelliteIds, bookmarks, preferences.warningMinutes, addNotification]);Also applies to: 124-128
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/hooks/useFlybyEngine.ts` around lines 41 - 48, Update the
checkFlybys flow in useFlybyEngine’s useEffect to track whether the effect is
still active, and have the async response handling return before adding
notifications when cleanup has run. Set that inactive state in the effect
cleanup alongside clearing the interval, preserving notification behavior for
requests belonging to the current tracked-satellite selection.
| const isDuplicate = state.notifications.some( | ||
| (n) => n.satelliteId === notificationData.satelliteId && | ||
| n.locationName === notificationData.locationName && | ||
| !n.dismissed && | ||
| Math.abs(n.eta.getTime() - notificationData.eta.getTime()) < 5 * 60 * 1000 // 5 min window |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep dismissed notifications in duplicate suppression.
Line 42 excludes dismissed records. After a user dismisses an alert, the periodic flyby engine can add the same flyby again while its ETA remains inside the five-minute window. Dismissal then fails to suppress the alert.
Remove the dismissed-state condition from this identity check. Keep dismissed only for display state.
Proposed fix
(n) => n.satelliteId === notificationData.satelliteId &&
n.locationName === notificationData.locationName &&
- !n.dismissed &&
Math.abs(n.eta.getTime() - notificationData.eta.getTime()) < 5 * 60 * 1000 // 5 min window📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isDuplicate = state.notifications.some( | |
| (n) => n.satelliteId === notificationData.satelliteId && | |
| n.locationName === notificationData.locationName && | |
| !n.dismissed && | |
| Math.abs(n.eta.getTime() - notificationData.eta.getTime()) < 5 * 60 * 1000 // 5 min window | |
| const isDuplicate = state.notifications.some( | |
| (n) => n.satelliteId === notificationData.satelliteId && | |
| n.locationName === notificationData.locationName && | |
| Math.abs(n.eta.getTime() - notificationData.eta.getTime()) < 5 * 60 * 1000 // 5 min window |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/store/notificationStore.ts` around lines 39 - 43, Update the
duplicate check in the notification store’s isDuplicate logic to remove the
!n.dismissed condition. Duplicate suppression must compare satelliteId,
locationName, and the five-minute ETA window regardless of dismissal state,
while dismissed remains display-only.
| const epochDate = obj.epoch ? new Date(obj.epoch) : new Date(); | ||
| const now = new Date(); | ||
| const elapsedDays = (now.getTime() - epochDate.getTime()) / 86400000 + (timeOffsetSec / 86400); | ||
| const currentMeanAnomaly = ((obj.mean_anomaly + (elapsedDays * obj.mean_motion * 360)) % 360) * Math.PI / 180; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject invalid epochs before propagation.
If obj.epoch is malformed, new Date(obj.epoch).getTime() is NaN. The function then returns non-null lat and lon values of NaN, which violates its invalid-input contract and corrupts downstream rendering and flyby calculations.
Proposed fix
- const epochDate = obj.epoch ? new Date(obj.epoch) : new Date();
const now = new Date();
- const elapsedDays = (now.getTime() - epochDate.getTime()) / 86400000 + (timeOffsetSec / 86400);
+ const epochMs = obj.epoch ? new Date(obj.epoch).getTime() : now.getTime();
+ if (!Number.isFinite(epochMs)) return null;
+
+ const elapsedDays = (now.getTime() - epochMs) / 86400000 + (timeOffsetSec / 86400);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const epochDate = obj.epoch ? new Date(obj.epoch) : new Date(); | |
| const now = new Date(); | |
| const elapsedDays = (now.getTime() - epochDate.getTime()) / 86400000 + (timeOffsetSec / 86400); | |
| const currentMeanAnomaly = ((obj.mean_anomaly + (elapsedDays * obj.mean_motion * 360)) % 360) * Math.PI / 180; | |
| const now = new Date(); | |
| const epochMs = obj.epoch ? new Date(obj.epoch).getTime() : now.getTime(); | |
| if (!Number.isFinite(epochMs)) return null; | |
| const elapsedDays = (now.getTime() - epochMs) / 86400000 + (timeOffsetSec / 86400); | |
| const currentMeanAnomaly = ((obj.mean_anomaly + (elapsedDays * obj.mean_motion * 360)) % 360) * Math.PI / 180; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/utils/orbitCalc.ts` around lines 20 - 23, Validate the epoch in
the orbit propagation logic before calculating elapsedDays: when an explicitly
provided obj.epoch produces an invalid Date, return the function’s established
invalid-input result rather than propagating NaN values. Keep the default
current-date behavior for a missing epoch and preserve normal propagation for
valid epochs.
There was a problem hiding this comment.
Pull request overview
This PR introduces a client-side “flyby alert” system in the frontend: it periodically evaluates upcoming satellite passes for the user’s current location and saved bookmarks, generates toast notifications, and provides a history/settings panel for managing alert behavior.
Changes:
- Added orbital math utilities and a polling “flyby engine” hook to predict visible passes and trigger alerts.
- Implemented notification state management (alerts + preferences) and new UI components for toasts and alert history/settings.
- Integrated the notification center into the main app layout and refactored EarthTwin to reuse the shared orbit calculation utility.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/tsconfig.app.json | Removes a TypeScript deprecation-suppression setting. |
| frontend/src/utils/orbitCalc.ts | Adds shared orbit propagation + distance/elevation helpers for flyby prediction. |
| frontend/src/store/uiStore.ts | Adds UI state + toggle for opening the flyby history panel. |
| frontend/src/store/notificationStore.ts | Introduces zustand store for flyby notifications and user preferences. |
| frontend/src/hooks/useFlybyEngine.ts | Adds the periodic flyby detection/prediction loop and notification generation. |
| frontend/src/components/ui/NotificationCenter.tsx | Adds toast display plus flyby history/settings panel UI. |
| frontend/src/components/ui/FlybyNotification.tsx | Adds the individual flyby toast UI + audio/browser notification behaviors. |
| frontend/src/components/layouts/MainLayout.tsx | Mounts the notification center globally and adds a top-bar toggle button. |
| frontend/src/components/EarthTwin.tsx | Refactors to import keplerToLatLonAlt from the new shared utility. |
Suppressed comments (1)
frontend/src/components/ui/NotificationCenter.tsx:70
- This icon-only toggle button needs an
aria-labelso assistive tech can announce what the control does (enable/disable audio alerts).
<button
onClick={() => updatePreferences({ soundEnabled: !preferences.soundEnabled })}
className={`text-lg transition-ui ${preferences.soundEnabled ? 'text-primary-container' : 'text-on-surface-variant'}`}
>
<MaterialIcon name={preferences.soundEnabled ? 'volume_up' : 'volume_off'} />
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Slant range (distance from observer to satellite) | ||
| const d = Math.sqrt(rE ** 2 + rS ** 2 - 2 * rE * rS * Math.cos(gammaRad)); | ||
|
|
||
| // Elevation angle calculation | ||
| const cosEl = (rS * Math.sin(gammaRad)) / d; | ||
|
|
||
| let elRad = Math.acos(cosEl); | ||
|
|
||
| // If gamma > 90 deg, the satellite is definitely below the horizon, but Math.acos handles 0 to PI. | ||
| // Actually, wait, a standard way is to use atan2 or just simple geometry: | ||
| // el = atan( (cos(gamma) - (rE / rS)) / sin(gamma) ) | ||
|
|
||
| const el = Math.atan2(Math.cos(gammaRad) - (rE / rS), Math.sin(gammaRad)); | ||
| return el * (180 / Math.PI); |
| useEffect(() => { | ||
| if (selectedSatelliteIds.length === 0) return; | ||
|
|
||
| const checkFlybys = async () => { | ||
| try { | ||
| // Fetch data for all tracked satellites | ||
| const satPromises = selectedSatelliteIds.map(id => api.getCatalogObjectByNorad(id)); | ||
| const responses = await Promise.allSettled(satPromises); | ||
|
|
| } | ||
| } catch (err) { | ||
| console.error('Flyby engine error:', err); | ||
| } |
| const ctx = new AudioContext(); | ||
| const osc = ctx.createOscillator(); | ||
| const gainNode = ctx.createGain(); | ||
|
|
||
| osc.type = 'sine'; | ||
| osc.frequency.setValueAtTime(880, ctx.currentTime); // A5 | ||
| osc.frequency.exponentialRampToValueAtTime(440, ctx.currentTime + 0.1); // Drop to A4 | ||
|
|
||
| gainNode.gain.setValueAtTime(0.1, ctx.currentTime); | ||
| gainNode.gain.exponentialRampToValueAtTime(0.01, ctx.currentTime + 0.5); | ||
|
|
||
| osc.connect(gainNode); | ||
| gainNode.connect(ctx.destination); | ||
|
|
||
| osc.start(); | ||
| osc.stop(ctx.currentTime + 0.5); |
| // Optional: Use browser notifications API if permitted | ||
| if (Notification.permission === 'granted') { | ||
| new Notification(`Flyby Alert: ${notification.satelliteName}`, { | ||
| body: `Approaching ${notification.locationName}. ETA: ${notification.eta.toLocaleTimeString()}`, | ||
| icon: '/vite.svg' | ||
| }); | ||
| } else if (Notification.permission !== 'denied') { | ||
| Notification.requestPermission(); | ||
| } |
| <button | ||
| onClick={toggleHistory} | ||
| className="text-on-surface-variant hover:text-primary-container transition-ui" | ||
| > | ||
| <MaterialIcon name="close" /> | ||
| </button> |
| <button | ||
| onClick={() => dismiss(notification.id)} | ||
| className="text-on-surface-variant hover:text-primary-container transition-ui" | ||
| > | ||
| <MaterialIcon name="close" className="text-sm" /> | ||
| </button> |
github-actions
Bot
added
backend
5ee1046 to
bad376a
Compare
|
Hello @SohammPawarr, Please attach screenshots of the updates/changes along with your PR. Thanks! |
This PR introduces a real-time, Mission Control-style notification system that tracks active satellites and alerts users whenever a selected satellite is about to pass over their current location or any of their bookmarked cities.
Changes Made
Orbital Calculation Engine (src/utils/orbitCalc.ts): Extracted the keplerToLatLonAlt logic from EarthTwin.tsx into a reusable utility. Added new Haversine distance and Max Elevation Angle calculations.
Background Flyby Hook (src/hooks/useFlybyEngine.ts): Implemented a lightweight polling interval that runs globally in the background. It propagates selected satellite orbits up to 60 minutes into the future to calculate the Time of Closest Approach (ETA) and visibility.
State Management (src/store/notificationStore.ts): Created a Zustand store to handle active alerts, history logging, and user preferences (warning time window, audio toggles).
Notification UI (src/components/ui/NotificationCenter.tsx & FlybyNotification.tsx):
Designed an immersive, telemetry-style toast notification.
Implemented a "History & Settings" slide-out panel accessible from the top command bar.
Built a synthesized double-beep audio alert using the Web Audio API for critical flyby detection.
Related Issue
Fixes #169
Testing Details
Verified the background interval logic triggers without blocking the main UI thread.
Tested orbital math predictions to ensure accurate Max Elevation and ETA metrics.
Confirmed that verbatim module imports for TS interfaces (like SpaceObject and FlybyNotification) are correctly typed to pass Vite's verbatimModuleSyntax strict checks.
Tested the rendering of the Notification Center and History Panel inside MainLayout.tsx.