Feature/cinematic camera - #176
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
backend
📝 WalkthroughWalkthroughChangesSatellite experience
Repository configuration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CameraControls
participant useUIStore
participant useCinematicCamera
participant CesiumViewer
User->>CameraControls: select camera mode
CameraControls->>useUIStore: setCameraMode(mode)
useCinematicCamera->>useUIStore: read selected satellite and mode
useCinematicCamera->>CesiumViewer: update camera on preRender
sequenceDiagram
participant useFlybyEngine
participant SatelliteCatalog
participant orbitCalc
participant useNotificationStore
participant NotificationCenter
useFlybyEngine->>SatelliteCatalog: fetch selected satellite data
useFlybyEngine->>orbitCalc: propagate positions and calculate elevation
useFlybyEngine->>useNotificationStore: create qualifying notification
NotificationCenter->>useNotificationStore: read active notifications
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 Warning |
codeant-ai
Bot
added
the
size:XL
github-actions
Bot
added
AI
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| useSpotlightEffect({ | ||
| viewer: viewerInstance, | ||
| selectedId: selectedSatelliteId, | ||
| entitiesRef, | ||
| hoveredId: hoveredObject?.catalog_number ?? null, | ||
| }); |
There was a problem hiding this comment.
Suggestion: The new call does not satisfy useSpotlightEffect's required argument contract: it omits catalogMapRef, collisionSetRef, and datasetVersion. This causes the TypeScript build to fail and prevents the spotlight effect from being initialized. [api mismatch]
Severity Level: Critical 🚨
- ❌ Frontend TypeScript build fails during compilation.
- ❌ EarthTwin globe and cinematic features cannot be deployed.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** frontend/src/components/EarthTwin.tsx
**Line:** 355:360
**Comment:**
*Api Mismatch: The new call does not satisfy `useSpotlightEffect`'s required argument contract: it omits `catalogMapRef`, `collisionSetRef`, and `datasetVersion`. This causes the TypeScript build to fail and prevents the spotlight effect from being initialized.
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| const playBeep = () => { | ||
| try { | ||
| const AudioContext = window.AudioContext || (window as any).webkitAudioContext; | ||
| if (!AudioContext) return; |
There was a problem hiding this comment.
Suggestion: Every notification creates a new AudioContext, but the context is never closed after the oscillator finishes. Repeated alerts accumulate audio resources and can eventually exhaust or pressure browser audio resources. [resource leak]
Severity Level: Major ⚠️
- ⚠️ Long-lived sessions retain completed audio resources.
- ⚠️ Repeated flyby alerts can increase browser audio-memory pressure.
(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:** 16:16
**Comment:**
*Resource Leak: Every notification creates a new `AudioContext`, but the context is never closed after the oscillator finishes. Repeated alerts accumulate audio resources and can eventually exhaust or pressure browser audio resources.
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| const handleTrack = () => { | ||
| setSelectedSatelliteId(notification.satelliteId); | ||
| navigate('/dashboard/satellites'); | ||
| dismiss(notification.id); |
There was a problem hiding this comment.
Suggestion: The action sets the selected satellite and then navigates to /dashboard/satellites, but that route renders the satellite table rather than EarthTwin. Consequently, the user is not taken to the globe or cinematic camera view promised by the TRACK LIVE action. [api mismatch]
Severity Level: Major ⚠️
- ❌ TRACK LIVE opens the satellite catalog instead of EarthTwin.
- ⚠️ Flyby users must manually navigate to the globe.
(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:** 61:64
**Comment:**
*Api Mismatch: The action sets the selected satellite and then navigates to `/dashboard/satellites`, but that route renders the satellite table rather than `EarthTwin`. Consequently, the user is not taken to the globe or cinematic camera view promised by the TRACK LIVE action.
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: This hook creates its own independent useBookmarkStorage state. Bookmark updates performed through the globe's separate useBookmarks instance update local storage but do not update this captured bookmarks array, so newly added or edited targets are not considered by the flyby engine until the component is remounted. [stale reference]
Severity Level: Major ⚠️
- ❌ Newly added globe bookmarks are omitted from flyby detection.
- ⚠️ Edited bookmark coordinates remain stale for alerts.
(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: This hook creates its own independent `useBookmarkStorage` state. Bookmark updates performed through the globe's separate `useBookmarks` instance update local storage but do not update this captured `bookmarks` array, so newly added or edited targets are not considered by the flyby engine until the component 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| // Run immediately, then on interval | ||
| checkFlybys(); | ||
| const intervalId = setInterval(checkFlybys, CHECK_INTERVAL_MS); | ||
|
|
||
| return () => clearInterval(intervalId); |
There was a problem hiding this comment.
Suggestion: The asynchronous check continues after this effect is cleaned up because cleanup only clears the interval. If the selected satellites, bookmarks, or warning preference changes while Promise.allSettled or propagation is still running, the old closure can add alerts for deselected satellites or removed locations. [stale reference]
Severity Level: Major ⚠️
- ⚠️ Users can receive flyby alerts for deselected satellites.
- ⚠️ Removed bookmark locations can still generate alerts.
(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:** 124:128
**Comment:**
*Stale Reference: The asynchronous check continues after this effect is cleaned up because cleanup only clears the interval. If the selected satellites, bookmarks, or warning preference changes while `Promise.allSettled` or propagation is still running, the old closure can add alerts for deselected satellites or removed locations.
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
| setActiveSector: (sector) => set({ activeSector: sector }), | ||
| setGlobalSearchOpen: (open) => set({ globalSearchOpen: open }), | ||
| toggleFlybyHistory: () => set((state) => ({ isFlybyHistoryOpen: !state.isFlybyHistoryOpen })), | ||
| setCameraMode: (mode) => set({ cameraMode: mode }), |
There was a problem hiding this comment.
Suggestion: Camera mode is retained when selectedSatelliteId is cleared. After selecting CHASE, COCKPIT, or another cinematic mode, closing the selection hides CameraControls but leaves the mode active; selecting a different satellite later immediately re-enters the old mode unexpectedly. Reset cameraMode to FREE when selection is cleared, or explicitly reset it when a new satellite is selected. [state and lifecycle behavior]
Severity Level: Major ⚠️
- ⚠️ New satellite selections unexpectedly resume prior camera modes.
- ❌ Manual camera control is overridden after reselection.
- ⚠️ Camera controls disappear while stale mode remains active.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** frontend/src/store/uiStore.ts
**Line:** 70:70
**Comment:**
*State And Lifecycle Behavior: Camera mode is retained when `selectedSatelliteId` is cleared. After selecting CHASE, COCKPIT, or another cinematic mode, closing the selection hides `CameraControls` but leaves the mode active; selecting a different satellite later immediately re-enters the old mode unexpectedly. Reset `cameraMode` to `FREE` when selection is cleared, or explicitly reset it when a new satellite is selected.
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| const alt = obj.semimajor_axis - EARTH_RADIUS_KM; | ||
| if (alt < 0 || alt > 100000) return null; | ||
|
|
||
| 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 ecc = obj.eccentricity ?? 0; | ||
| const trueAnomaly = currentMeanAnomaly + 2 * ecc * Math.sin(currentMeanAnomaly); |
There was a problem hiding this comment.
Suggestion: The propagator always uses the semimajor axis as the satellite's geocentric radius and approximates true anomaly without applying the eccentric-orbit radius formula. For non-circular orbits, this produces incorrect altitude and ground-track positions, so the globe, camera tracking, and flyby predictions can all place the satellite incorrectly. Compute the instantaneous radius from eccentricity and true anomaly before returning alt. [logic error]
Severity Level: Major ⚠️
- ❌ Flyby predictions use incorrect closest-approach positions.
- ⚠️ Elevation and ETA notifications become inaccurate.
- ⚠️ Cinematic camera tracking follows incorrect altitude.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** frontend/src/utils/orbitCalc.ts
**Line:** 17:26
**Comment:**
*Logic Error: The propagator always uses the semimajor axis as the satellite's geocentric radius and approximates true anomaly without applying the eccentric-orbit radius formula. For non-circular orbits, this produces incorrect altitude and ground-track positions, so the globe, camera tracking, and flyby predictions can all place the satellite incorrectly. Compute the instantaneous radius from eccentricity and true anomaly before returning `alt`.
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: 9
🧹 Nitpick comments (7)
frontend/src/utils/orbitCalc.ts (1)
70-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the dead elevation computation and the exploratory comments.
Lines 78-83 compute
d,cosEl, andelRad, but the function returns the value from line 89.elRadis never read. The unused local will also fail ano-unused-varslint rule. The comments on lines 85-87 record a thought process rather than the final behavior.The returned
atan2formula is correct. Keep it and drop the rest.♻️ Proposed cleanup
export function calculateElevationAngle(satelliteAltKm: number, groundDistanceKm: number): number { const rE = EARTH_RADIUS_KM; const rS = EARTH_RADIUS_KM + satelliteAltKm; - + // Central angle between observer and satellite's nadir const gammaRad = groundDistanceKm / rE; - - // 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) ) - + + // Elevation above the local horizon; negative when the satellite is below it. const el = Math.atan2(Math.cos(gammaRad) - (rE / rS), Math.sin(gammaRad)); return el * (180 / Math.PI); }🤖 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 70 - 91, In calculateElevationAngle, remove the unused d, cosEl, and elRad calculations, along with the exploratory comments between them and the final return. Preserve the existing gammaRad setup and returned atan2-based elevation formula unchanged.frontend/src/hooks/useCinematicCamera.ts (1)
51-52: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse one position property instead of allocating per frame.
onPreRenderruns on every rendered frame. Line 52 constructs a newCesium.ConstantPositionPropertyeach time and reassignsentity.position. Each reassignment allocates an object and raisesdefinitionChangedon the entity, which forces the visualizer to rebind the property. Create the property once per tracked entity and callsetValueon it.♻️ Proposed fix
+ const positionPropRef = useRef<Cesium.ConstantPositionProperty | null>(null); + const positionEntityIdRef = useRef<string | null>(null);// 2. Update physical entity position so it visibly moves! - entity.position = new Cesium.ConstantPositionProperty(p0); + if (positionEntityIdRef.current !== targetId || !positionPropRef.current) { + positionPropRef.current = new Cesium.ConstantPositionProperty(p0); + positionEntityIdRef.current = targetId; + entity.position = positionPropRef.current; + } else { + positionPropRef.current.setValue(p0); + }🤖 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/useCinematicCamera.ts` around lines 51 - 52, Update the tracked-entity setup and onPreRender logic in useCinematicCamera so each entity creates and assigns one ConstantPositionProperty only once, then reuse that property by calling setValue(p0) on every frame instead of reallocating and reassigning entity.position.frontend/src/store/notificationStore.ts (1)
55-55: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCap the notification history.
Line 55 prepends without limit.
NotificationCenterrenders every entry in the history panel on lines 111-124, so the list and the store both grow for the lifetime of the session. Trim to a fixed maximum.♻️ Proposed fix
+const MAX_NOTIFICATIONS = 100;- return { notifications: [newNotification, ...state.notifications] }; + return { notifications: [newNotification, ...state.notifications].slice(0, MAX_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/store/notificationStore.ts` at line 55, Cap the notification history in the state update that prepends new notifications, retaining only a fixed maximum number of entries after adding newNotification. Reuse the store’s existing maximum-limit constant or define one near the notification state, and preserve newest-first ordering for NotificationCenter.frontend/src/components/ui/NotificationCenter.tsx (2)
51-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIcon-only controls in this PR expose no accessible name. Every new interactive control renders a
MaterialIconas its only child, or hides its text label at small breakpoints. Assistive technology receives no name for these controls, and the toggles do not publish their state. Addtype="button", anaria-label, andaria-pressedoraria-expandedwhere the control carries state.
frontend/src/components/ui/NotificationCenter.tsx#L51-L56: addaria-label="Close flyby alerts panel"to the panel close button.frontend/src/components/ui/NotificationCenter.tsx#L66-L71: addaria-label="Toggle audio alerts"andaria-pressed={preferences.soundEnabled}to the audio toggle.frontend/src/components/ui/FlybyNotification.tsx#L85-L90: add anaria-labelthat names the satellite being dismissed.frontend/src/components/layouts/MainLayout.tsx#L258-L263: addaria-label="Toggle flyby alerts"andaria-expanded={isFlybyHistoryOpen}to the radar button.frontend/src/components/ui/CameraControls.tsx#L29-L41: addaria-label={${mode.label} camera mode}andaria-pressed={isActive}, because line 40 hides the visible label below thesmbreakpoint.🤖 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/NotificationCenter.tsx` around lines 51 - 56, Add type="button" and accessible state/name attributes to all identified controls: in frontend/src/components/ui/NotificationCenter.tsx lines 51-56 label the close button “Close flyby alerts panel,” and lines 66-71 label the audio toggle “Toggle audio alerts” with aria-pressed={preferences.soundEnabled}; in frontend/src/components/ui/FlybyNotification.tsx lines 85-90 add an aria-label naming the satellite being dismissed; in frontend/src/components/layouts/MainLayout.tsx lines 258-263 label the radar button “Toggle flyby alerts” with aria-expanded={isFlybyHistoryOpen}; and in frontend/src/components/ui/CameraControls.tsx lines 29-41 label each mode button with `${mode.label} camera mode` and set aria-pressed={isActive}.
27-33: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the exit animation apply to the
AnimatePresencechild.
AnimatePresenceonly animates its direct children, but the direct child here is a plaindivon line 29. That removes dismissed notifications without theFlybyNotificationexitanimation. Movekey,className, and the animation props to a motion child ofAnimatePresence, or wrapFlybyNotificationwith a motion component that carriesexit.🤖 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/NotificationCenter.tsx` around lines 27 - 33, Update the AnimatePresence mapping in NotificationCenter so its direct child is a motion component carrying the notification key, pointer-events class, and exit animation props. Keep FlybyNotification rendered inside that motion wrapper so dismissed notifications use its intended exit animation.frontend/src/components/EarthTwin.tsx (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared
CatalogObjecttype inEarthTwin.tsx.
EarthTwin.tsxstill declares a localCatalogObjectthat duplicates@/types/satellite, including the interface used bycatalogMapRefandkeplerToLatLonAlt(obj). ImportCatalogObjectfrom the shared type and remove the local declaration so future shape changes converge at one definition.🤖 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/EarthTwin.tsx` at line 6, Update EarthTwin.tsx to import and use the shared CatalogObject type from `@/types/satellite` for catalogMapRef and keplerToLatLonAlt(obj), then remove the duplicate local CatalogObject declaration.frontend/src/components/ui/CameraControls.tsx (1)
3-3: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winImport
CameraModeas a type.
CameraModeis exported only as a type, butCameraControls.tsximports it in the value position whileverbatimModuleSyntaxis enabled. This leaves an unresolved runtime import for a removed value import.♻️ Proposed fix
-import { useUIStore, CameraMode } from '`@/store/uiStore`'; +import { useUIStore } from '`@/store/uiStore`'; +import type { CameraMode } from '`@/store/uiStore`';🤖 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/CameraControls.tsx` at line 3, Update the import in CameraControls to import CameraMode as a type while retaining the value import for useUIStore, ensuring no runtime import is generated for the type-only symbol.
🤖 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 @.gitignore:
- Line 100: Update the .gitignore entry for alembic.ini so the repository
includes a tracked backend Alembic configuration or shared template required by
the README migration setup; if ignoring it is intentional, ensure setup
generates a fixed repository-shared config while keeping secrets in environment
variables.
In `@frontend/src/components/layouts/MainLayout.tsx`:
- Around line 416-417: Update MainLayout so CameraControls is rendered only on
routes that also render EarthTwin, rather than unconditionally in the
application shell. Move it alongside the globe rendering or gate it using the
existing route condition, while preserving NotificationCenter and the current
camera-control behavior on globe routes.
In `@frontend/src/components/ui/FlybyNotification.tsx`:
- Around line 13-37: Update playBeep to close the newly created AudioContext
when the oscillator finishes, using the oscillator’s completion event after its
scheduled stop. Preserve the existing audio setup and warning behavior while
ensuring each notification releases its context resources.
- Around line 45-59: Update the FlybyNotification effect to guard all
Notification API access with a window availability check, remove
Notification.requestPermission from the effect, and ensure the alert fires only
once per notification rather than when sound preferences change. Use a ref or
equivalent keyed to the notification identity, remove the unused preferences
binding, and adjust the effect dependencies so audio toggles do not re-trigger
the beep or browser notification; permission requests should remain in an
explicit preferences-panel control.
In `@frontend/src/hooks/useCinematicCamera.ts`:
- Around line 124-138: Update the orientation interpolation in the cinematic
camera update around the Cesium.Cartesian3.lerp calls: after interpolating and
normalizing nextDir, rebuild nextUp using the cross product with nextDir and the
interpolated up reference, then normalize it so direction and up form an
orthonormal basis before viewer.camera.setView.
In `@frontend/src/hooks/useFlybyEngine.ts`:
- Around line 44-53: Refactor checkFlybys so catalog elements are fetched and
cached on a substantially longer refresh interval, while the existing 30-second
tick only propagates cached elements. Add AbortController cancellation to the
effect cleanup, pass its signal through in-flight work, and verify it is not
aborted before addNotification; update the interval lifecycle around the effect
containing checkFlybys. Refine the propagation grid in checkFlybys with a coarse
scan followed by a finer scan around the best sample so maxElevation captures
closer approaches.
- Around line 31-38: Update the geolocation failure and unavailable branches in
useFlybyEngine so userLocationRef.current remains null instead of being set to {
lat: 0, lon: 0 }; preserve the existing targetLocations behavior that skips the
current-location entry when the ref is null, allowing only bookmarked locations
to drive predictions.
- Line 51: Update the fulfilled-result handling in useFlybyEngine to remove the
runtime any cast before calling keplerToLatLonAlt. Pass the API object through
its existing SpaceObject-compatible shape, or introduce a shared typed interface
for the conversion, while preserving the current propagation behavior and
avoiding any.
In `@frontend/src/store/notificationStore.ts`:
- Around line 37-46: Update the duplicate detection in addNotification to match
notifications by satelliteId, locationName, and nearby eta regardless of the
dismissed flag. Remove the !n.dismissed condition so dismissNotification records
continue suppressing repeated engine-tick notifications.
---
Nitpick comments:
In `@frontend/src/components/EarthTwin.tsx`:
- Line 6: Update EarthTwin.tsx to import and use the shared CatalogObject type
from `@/types/satellite` for catalogMapRef and keplerToLatLonAlt(obj), then remove
the duplicate local CatalogObject declaration.
In `@frontend/src/components/ui/CameraControls.tsx`:
- Line 3: Update the import in CameraControls to import CameraMode as a type
while retaining the value import for useUIStore, ensuring no runtime import is
generated for the type-only symbol.
In `@frontend/src/components/ui/NotificationCenter.tsx`:
- Around line 51-56: Add type="button" and accessible state/name attributes to
all identified controls: in frontend/src/components/ui/NotificationCenter.tsx
lines 51-56 label the close button “Close flyby alerts panel,” and lines 66-71
label the audio toggle “Toggle audio alerts” with
aria-pressed={preferences.soundEnabled}; in
frontend/src/components/ui/FlybyNotification.tsx lines 85-90 add an aria-label
naming the satellite being dismissed; in
frontend/src/components/layouts/MainLayout.tsx lines 258-263 label the radar
button “Toggle flyby alerts” with aria-expanded={isFlybyHistoryOpen}; and in
frontend/src/components/ui/CameraControls.tsx lines 29-41 label each mode button
with `${mode.label} camera mode` and set aria-pressed={isActive}.
- Around line 27-33: Update the AnimatePresence mapping in NotificationCenter so
its direct child is a motion component carrying the notification key,
pointer-events class, and exit animation props. Keep FlybyNotification rendered
inside that motion wrapper so dismissed notifications use its intended exit
animation.
In `@frontend/src/hooks/useCinematicCamera.ts`:
- Around line 51-52: Update the tracked-entity setup and onPreRender logic in
useCinematicCamera so each entity creates and assigns one
ConstantPositionProperty only once, then reuse that property by calling
setValue(p0) on every frame instead of reallocating and reassigning
entity.position.
In `@frontend/src/store/notificationStore.ts`:
- Line 55: Cap the notification history in the state update that prepends new
notifications, retaining only a fixed maximum number of entries after adding
newNotification. Reuse the store’s existing maximum-limit constant or define one
near the notification state, and preserve newest-first ordering for
NotificationCenter.
In `@frontend/src/utils/orbitCalc.ts`:
- Around line 70-91: In calculateElevationAngle, remove the unused d, cosEl, and
elRad calculations, along with the exploratory comments between them and the
final return. Preserve the existing gammaRad setup and returned atan2-based
elevation formula unchanged.
🪄 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: e3e6ae86-1b19-4018-8a05-813fc611de51
⛔ Files ignored due to path filters (27)
backend/api/v1/__pycache__/__init__.cpython-313.pycis excluded by!**/*.pycbackend/api/v1/endpoints/__pycache__/__init__.cpython-313.pycis excluded by!**/*.pycbackend/api/v1/endpoints/__pycache__/agents.cpython-313.pycis excluded by!**/*.pycbackend/api/v1/endpoints/__pycache__/auth.cpython-313.pycis excluded by!**/*.pycbackend/api/v1/endpoints/__pycache__/catalog.cpython-313.pycis excluded by!**/*.pycbackend/api/v1/endpoints/__pycache__/collisions.cpython-313.pycis excluded by!**/*.pycbackend/api/v1/endpoints/__pycache__/dashboard.cpython-313.pycis excluded by!**/*.pycbackend/api/v1/endpoints/__pycache__/satellites.cpython-313.pycis excluded by!**/*.pycbackend/api/v1/endpoints/__pycache__/weather.cpython-313.pycis excluded by!**/*.pycbackend/app/__pycache__/__init__.cpython-313.pycis excluded by!**/*.pycbackend/app/__pycache__/main.cpython-313.pycis excluded by!**/*.pycbackend/app/core/__pycache__/__init__.cpython-313.pycis excluded by!**/*.pycbackend/app/core/__pycache__/config.cpython-313.pycis excluded by!**/*.pycbackend/app/core/__pycache__/scheduler.cpython-313.pycis excluded by!**/*.pycbackend/app/core/__pycache__/security.cpython-313.pycis excluded by!**/*.pycbackend/database/__pycache__/__init__.cpython-313.pycis excluded by!**/*.pycbackend/database/__pycache__/session.cpython-313.pycis excluded by!**/*.pycbackend/models/__pycache__/__init__.cpython-313.pycis excluded by!**/*.pycbackend/models/__pycache__/db_models.cpython-313.pycis excluded by!**/*.pycbackend/orbital/__pycache__/__init__.cpython-313.pycis excluded by!**/*.pycbackend/orbital/providers/__pycache__/__init__.cpython-313.pycis excluded by!**/*.pycbackend/orbital/providers/__pycache__/base.cpython-313.pycis excluded by!**/*.pycbackend/orbital/providers/__pycache__/cache.cpython-313.pycis excluded by!**/*.pycbackend/orbital/providers/__pycache__/celestrak.cpython-313.pycis excluded by!**/*.pycbackend/orbital/providers/__pycache__/chain.cpython-313.pycis excluded by!**/*.pycbackend/orbital/providers/__pycache__/spacetrack.cpython-313.pycis excluded by!**/*.pycbackend/tests/__pycache__/test_backend.cpython-312-pytest-9.1.1.pycis excluded by!**/*.pyc
📒 Files selected for processing (12)
.gitignorefrontend/src/components/EarthTwin.tsxfrontend/src/components/layouts/MainLayout.tsxfrontend/src/components/ui/CameraControls.tsxfrontend/src/components/ui/FlybyNotification.tsxfrontend/src/components/ui/NotificationCenter.tsxfrontend/src/hooks/useCinematicCamera.tsfrontend/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
| env/ | ||
| .env | ||
| .pytest_cache/ | ||
| alembic.ini |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files | rg '(^|/)alembic\.ini$|(^|/)alembic(/|$)' || true
rg -n --hidden --glob '!.git/**' 'alembic\.ini|alembic' . || true
git check-ignore -v --no-index alembic.ini || trueRepository: 7-Blocks/Kepler
Length of output: 259
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Tracked Alembic-related files:\n'
git ls-files | rg '(^|/)alembic\.ini$|(^|/)alembic(/|$)|alembic|alembic' || true
printf '\nREADME Alembic context:\n'
sed -n '260,300p' README.md 2>/dev/null || true
printf '\nALEMBIC_CONFIG references:\n'
rg -n --hidden --glob '!.git/**' 'ALEMBIC_CONFIG|alembic\.ini|alembic upgrade|alembic init|alembic_version|alembic' . \
--glob '!/.git/**' || true
printf '\nRoot/config files:\n'
git ls-files '*/' | sed 's#/.*##' | sort -u | rg '^(alembic|alembic\.ini|pyproject|setup\.py|requirements|Pipfile|poetry\.lock)$' || trueRepository: 7-Blocks/Kepler
Length of output: 948
Keep alembic.ini tracked or provide a repository-shared template.
README step 9 runs alembic upgrade head in backend, but the repository does not include backend/alembic.ini or another shared Alembic config. Keep ignoring alembic.ini only if the repository intentionally provides a fixed generated config during setup. Otherwise, commit the migration config/template and keep secrets in environment variables.
🤖 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 @.gitignore at line 100, Update the .gitignore entry for alembic.ini so the
repository includes a tracked backend Alembic configuration or shared template
required by the README migration setup; if ignoring it is intentional, ensure
setup generates a fixed repository-shared config while keeping secrets in
environment variables.
| <NotificationCenter /> | ||
| <CameraControls /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Render CameraControls only where a globe exists.
Line 417 mounts CameraControls in the application shell, so it renders on every route under this layout. The component shows itself whenever selectedSatelliteId is set (frontend/src/components/ui/CameraControls.tsx line 19). The camera modes act on the Cesium viewer that EarthTwin owns, through useCinematicCamera. On a route without the globe, such as /dashboard/satellites or /dashboard/settings, a previously selected satellite still makes the control bar appear, and every mode button then produces no visible result.
Move CameraControls next to the globe, or gate it on the route that renders EarthTwin.
🤖 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/layouts/MainLayout.tsx` around lines 416 - 417,
Update MainLayout so CameraControls is rendered only on routes that also render
EarthTwin, rather than unconditionally in the application shell. Move it
alongside the globe rendering or gate it using the existing route condition,
while preserving NotificationCenter and the current camera-control behavior on
globe routes.
| const playBeep = () => { | ||
| try { | ||
| const AudioContext = window.AudioContext || (window as any).webkitAudioContext; | ||
| if (!AudioContext) return; | ||
|
|
||
| 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 | 🟠 Major | ⚡ Quick win
Close the AudioContext after the beep.
Line 18 creates a new AudioContext on every notification and never closes it. Browsers limit the number of concurrent AudioContext instances per document. Chrome throws once the limit is reached. The catch on line 34 swallows that error, so alert audio stops working silently after a few notifications.
Close the context when the oscillator ends, or create one context at module scope and reuse it.
🐛 Proposed fix
osc.start();
osc.stop(ctx.currentTime + 0.5);
+ osc.onended = () => {
+ void ctx.close();
+ };
} catch (e) {📝 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 playBeep = () => { | |
| try { | |
| const AudioContext = window.AudioContext || (window as any).webkitAudioContext; | |
| if (!AudioContext) return; | |
| 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); | |
| } | |
| }; | |
| const playBeep = () => { | |
| try { | |
| const AudioContext = window.AudioContext || (window as any).webkitAudioContext; | |
| if (!AudioContext) return; | |
| 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); | |
| osc.onended = () => { | |
| void ctx.close(); | |
| }; | |
| } catch (e) { | |
| console.warn('Audio play failed', e); | |
| } | |
| }; |
🤖 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 13 - 37,
Update playBeep to close the newly created AudioContext when the oscillator
finishes, using the oscillator’s completion event after its scheduled stop.
Preserve the existing audio setup and warning behavior while ensuring each
notification releases its context resources.
| 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.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the Notification global, and do not re-fire the alert when the user toggles audio.
Two problems in this effect:
-
Lines 51-57 read
Notification.permissionwithout checking that the API exists. The Notification API is absent in non-secure contexts and in some mobile browsers. The unguarded access throws inside the effect and breaks the component render. Check'Notification' in windowfirst. -
The dependency array on line 59 includes
preferences.soundEnabled. When the user toggles audio inNotificationCenter, this effect re-runs for every mounted notification. The beep plays again and a second browser notification appears for the same pass. Fire the alert once per notification.
Line 57 also calls Notification.requestPermission() during render of a notification rather than from a user gesture. Several browsers reject a permission request that has no user activation. Move the request to an explicit control in the preferences panel.
🐛 Proposed fix
+ const alertedRef = useRef(false);
+
useEffect(() => {
- if (preferences.soundEnabled) {
+ // Alert once per notification. Reading soundEnabled from the store here
+ // keeps the toggle out of the dependency array, so flipping it does not
+ // replay the alert for every mounted notification.
+ if (alertedRef.current) return;
+ alertedRef.current = true;
+
+ if (useNotificationStore.getState().preferences.soundEnabled) {
playBeep();
}
-
- // Optional: Use browser notifications API if permitted
- if (Notification.permission === 'granted') {
+
+ // Optional: Use browser notifications API if available and permitted.
+ if (!('Notification' in window)) return;
+
+ 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]);
+ }, [notification]);Update the import on line 1 to include useRef, and drop the now-unused preferences binding on line 41 if nothing else reads it.
🤖 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 effect to guard all Notification API access with a
window availability check, remove Notification.requestPermission from the
effect, and ensure the alert fires only once per notification rather than when
sound preferences change. Use a ref or equivalent keyed to the notification
identity, remove the unused preferences binding, and adjust the effect
dependencies so audio toggles do not re-trigger the beep or browser
notification; permission requests should remain in an explicit preferences-panel
control.
| Cesium.Cartesian3.lerp(currentPos, targetPos, LERP_FACTOR, nextPos); | ||
| Cesium.Cartesian3.lerp(currentDir, targetDir, LERP_FACTOR, nextDir); | ||
| Cesium.Cartesian3.lerp(currentUp, targetUp, LERP_FACTOR, nextUp); | ||
|
|
||
| Cesium.Cartesian3.normalize(nextDir, nextDir); | ||
| Cesium.Cartesian3.normalize(nextUp, nextUp); | ||
|
|
||
| // Force the camera view | ||
| viewer.camera.setView({ | ||
| destination: nextPos, | ||
| orientation: { | ||
| direction: nextDir, | ||
| up: nextUp, | ||
| } | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Cesium Camera setView orientation direction up must be orthogonal
💡 Result:
In CesiumJS, when using the setView or flyTo methods with a custom orientation defined by direction and up vectors, these vectors must form an orthonormal basis [1][2][3]. Specifically, the direction and up vectors must be orthogonal (at a 90-degree angle to each other) and both must be normalized (unit length) [1][3]. The camera's internal orientation is maintained as an orthonormal basis, where: - The view direction is represented by the direction vector [1][2]. - The up vector defines the camera's vertical orientation [2][3]. - A third vector, right, is automatically calculated as the cross product of direction and up (right = direction x up) [1][2]. If the provided direction and up vectors are not orthogonal, the resulting camera orientation may be inconsistent or behave unexpectedly, as the engine requires a valid orthonormal basis to correctly construct the view matrix [1][3]. When manually providing these vectors, ensure they are unit vectors and satisfy the dot product condition (dot(direction, up) == 0).
Citations:
- 1: https://cesium.com/downloads/cesiumjs/releases/b30/Build/Documentation/Camera.html
- 2: https://cesium.com/learn/cesiumjs/ref-doc/Camera.html?classFilter=camera
- 3: https://cesium.com/downloads/cesiumjs/releases/1.84/Build/Documentation/Camera.html
Don’t interpolate direction and up independently.
Cesium’s Camera.setView orientation contract requires normalized direction and up to form an orthonormal basis. Lines 125-126 move each vector toward its target separately, and lines 128-129 only normalize them independently, so setView receives a pair whose relative angle drifts out of orthogonal. Rebuild up from the cross product after interpolating before passing it to setView.
♻️ Proposed fix
Cesium.Cartesian3.normalize(nextDir, nextDir);
- Cesium.Cartesian3.normalize(nextUp, nextUp);
+ // Rebuild an orthonormal basis: independent lerp of dir/up loses perpendicularity.
+ const right = Cesium.Cartesian3.cross(nextDir, nextUp, new Cesium.Cartesian3());
+ Cesium.Cartesian3.normalize(right, right);
+ Cesium.Cartesian3.cross(right, nextDir, nextUp);
+ Cesium.Cartesian3.normalize(nextUp, nextUp);📝 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.
| Cesium.Cartesian3.lerp(currentPos, targetPos, LERP_FACTOR, nextPos); | |
| Cesium.Cartesian3.lerp(currentDir, targetDir, LERP_FACTOR, nextDir); | |
| Cesium.Cartesian3.lerp(currentUp, targetUp, LERP_FACTOR, nextUp); | |
| Cesium.Cartesian3.normalize(nextDir, nextDir); | |
| Cesium.Cartesian3.normalize(nextUp, nextUp); | |
| // Force the camera view | |
| viewer.camera.setView({ | |
| destination: nextPos, | |
| orientation: { | |
| direction: nextDir, | |
| up: nextUp, | |
| } | |
| }); | |
| Cesium.Cartesian3.lerp(currentPos, targetPos, LERP_FACTOR, nextPos); | |
| Cesium.Cartesian3.lerp(currentDir, targetDir, LERP_FACTOR, nextDir); | |
| Cesium.Cartesian3.lerp(currentUp, targetUp, LERP_FACTOR, nextUp); | |
| Cesium.Cartesian3.normalize(nextDir, nextDir); | |
| // Rebuild an orthonormal basis: independent lerp of dir/up loses perpendicularity. | |
| const right = Cesium.Cartesian3.cross(nextDir, nextUp, new Cesium.Cartesian3()); | |
| Cesium.Cartesian3.normalize(right, right); | |
| Cesium.Cartesian3.cross(right, nextDir, nextUp); | |
| Cesium.Cartesian3.normalize(nextUp, nextUp); | |
| // Force the camera view | |
| viewer.camera.setView({ | |
| destination: nextPos, | |
| orientation: { | |
| direction: nextDir, | |
| up: nextUp, | |
| } | |
| }); |
🤖 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/useCinematicCamera.ts` around lines 124 - 138, Update the
orientation interpolation in the cinematic camera update around the
Cesium.Cartesian3.lerp calls: after interpolating and normalizing nextDir,
rebuild nextUp using the cross product with nextDir and the interpolated up
reference, then normalize it so direction and up form an orthonormal basis
before viewer.camera.setView.
| (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 fall back to latitude 0, longitude 0 when geolocation is unavailable.
If the user denies geolocation, lines 33 and 37 set the observer to (0, 0). That point is in the Gulf of Guinea. The engine then predicts passes over it and labels them "Current Location" on line 58, so the user receives alerts for a place they never selected. The targetLocations builder on lines 57-61 already skips the entry when userLocationRef.current is null.
Leave the ref null and let bookmarks drive the predictions.
🐛 Proposed fix
(error) => {
- console.warn('Geolocation denied or failed, using default location (0,0).', error);
- userLocationRef.current = { lat: 0, lon: 0 };
+ // Leave the ref null: flyby predictions then use bookmarked
+ // locations only, instead of an arbitrary point at (0, 0).
+ console.warn('Geolocation denied or failed; flyby alerts will use bookmarks only.', 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) => { | |
| // Leave the ref null: flyby predictions then use bookmarked | |
| // locations only, instead of an arbitrary point at (0, 0). | |
| console.warn('Geolocation denied or failed; flyby alerts will use bookmarks only.', 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 - 38, Update the
geolocation failure and unavailable branches in useFlybyEngine so
userLocationRef.current remains null instead of being set to { lat: 0, lon: 0 };
preserve the existing targetLocations behavior that skips the current-location
entry when the ref is null, allowing only bookmarked locations to drive
predictions.
| const checkFlybys = async () => { | ||
| try { | ||
| // Fetch data for all tracked satellites | ||
| const satPromises = selectedSatelliteIds.map(id => api.getCatalogObjectByNorad(id)); | ||
| const responses = await Promise.allSettled(satPromises); | ||
|
|
||
| const satellites = responses | ||
| .filter((res): res is PromiseFulfilledResult<any> => res.status === 'fulfilled') | ||
| .map(res => res.value.data as SpaceObject) | ||
| .filter(Boolean); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Decouple the catalog fetch from the 30-second prediction tick, and cancel in-flight work.
checkFlybys runs every 30 seconds (line 126) and issues one API request per selected satellite (line 47). Orbital elements change on the order of hours, so the refetch produces no accuracy gain and multiplies backend requests by the number of tracked satellites.
The effect cleanup on line 128 clears the interval only. It does not cancel a request that is already in flight. After the user changes the selection or the component unmounts, the pending checkFlybys still resolves and still calls addNotification on line 105. Notifications for a stale selection then appear.
Cache the fetched elements and refresh them on a long interval. Propagate from the cached elements on the 30-second tick. Add an AbortController and check it before calling addNotification.
Separately, the propagation grid on line 77 steps one minute at a time. A LEO satellite traverses the visible sky in a few minutes, so the sampled minimum can miss the true closest approach and understate maxElevation at the gate on line 98. Consider a coarse pass followed by a finer scan around the best sample.
Also applies to: 124-129
🤖 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 44 - 53, Refactor
checkFlybys so catalog elements are fetched and cached on a substantially longer
refresh interval, while the existing 30-second tick only propagates cached
elements. Add AbortController cancellation to the effect cleanup, pass its
signal through in-flight work, and verify it is not aborted before
addNotification; update the interval lifecycle around the effect containing
checkFlybys. Refine the propagation grid in checkFlybys with a coarse scan
followed by a finer scan around the best sample so maxElevation captures closer
approaches.
| const responses = await Promise.allSettled(satPromises); | ||
|
|
||
| const satellites = responses | ||
| .filter((res): res is PromiseFulfilledResult<any> => res.status === 'fulfilled') |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare SpaceObject with the CatalogObject fields that keplerToLatLonAlt reads.
set -euo pipefail
echo '--- SpaceObject ---'
ast-grep run --pattern 'interface SpaceObject { $$$ }' --lang typescript frontend/src || true
ast-grep run --pattern 'type SpaceObject = $$$' --lang typescript frontend/src || true
echo '--- CatalogObject in shared types ---'
fd -t f 'satellite.ts' frontend/src/types --exec cat -n {}
echo '--- getCatalogObjectByNorad signature ---'
rg -nP --type=ts -C5 'getCatalogObjectByNorad' frontend/srcRepository: 7-Blocks/Kepler
Length of output: 6187
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- useFlybyEngine outline ---'
ast-grep outline frontend/src/hooks/useFlybyEngine.ts || true
echo '--- useFlybyEngine relevant source ---'
sed -n '1,120p' frontend/src/hooks/useFlybyEngine.ts | cat -n
echo '--- orbital propagator definitions/usages ---'
rg -n --type=ts -C3 'keplerToLatLonAlt|CatalogObject|SpaceObject|semimajor_axis|arg_of_perigee|mean_motion|inclination|mean_anomaly|raan|eccentricity|epoch' frontend/srcRepository: 7-Blocks/Kepler
Length of output: 50371
Remove the runtime any cast before Kepler propagation.
SpaceObject already contains the orbit fields keplerToLatLonAlt reads, so sat as any is unnecessary and hides future field changes. Let keplerToLatLonAlt receive the API object with an any-free conversion if a shared interface is desired.
🤖 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` at line 51, Update the fulfilled-result
handling in useFlybyEngine to remove the runtime any cast before calling
keplerToLatLonAlt. Pass the API object through its existing
SpaceObject-compatible shape, or introduce a shared typed interface for the
conversion, while preserving the current propagation behavior and avoiding any.
| addNotification: (notificationData) => set((state) => { | ||
| // Avoid duplicate active notifications for the same satellite and location within a short timeframe | ||
| 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 | ||
| ); | ||
|
|
||
| if (isDuplicate) return state; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
A dismissed notification reappears on the next engine tick.
The duplicate check on lines 39-44 requires !n.dismissed. dismissNotification on lines 57-61 sets dismissed: true and keeps the record in the array. The flyby engine re-evaluates the same pass every 30 seconds and calls addNotification again with a near-identical eta. The dismissed record no longer matches the predicate, so the store creates a new notification and NotificationCenter shows the toast again.
The user dismisses an alert and it returns within 30 seconds, for as long as the pass stays inside the warning window.
Match against dismissed notifications too when suppressing duplicates.
🐛 Proposed fix
addNotification: (notificationData) => set((state) => {
- // Avoid duplicate active notifications for the same satellite and location within a short timeframe
+ // Suppress duplicates for the same satellite and location within a short
+ // timeframe. Dismissed notifications are included: a dismissed pass must
+ // not reappear when the engine re-evaluates it on the next tick.
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
);📝 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.
| addNotification: (notificationData) => set((state) => { | |
| // Avoid duplicate active notifications for the same satellite and location within a short timeframe | |
| 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 | |
| ); | |
| if (isDuplicate) return state; | |
| addNotification: (notificationData) => set((state) => { | |
| // Suppress duplicates for the same satellite and location within a short | |
| // timeframe. Dismissed notifications are included: a dismissed pass must | |
| // not reappear when the engine re-evaluates it on the next tick. | |
| 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 | |
| ); | |
| if (isDuplicate) return state; |
🤖 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 37 - 46, Update the
duplicate detection in addNotification to match notifications by satelliteId,
locationName, and nearby eta regardless of the dismissed flag. Remove the
!n.dismissed condition so dismissNotification records continue suppressing
repeated engine-tick notifications.
There was a problem hiding this comment.
Pull request overview
This PR adds a flyby-alert notification system and a cinematic satellite-tracking camera mode to the Kepler globe UI, introducing new Zustand state, background polling, and new UI panels/controls.
Changes:
- Added a flyby engine that periodically predicts near-term passes over the user’s current location and bookmarks and raises alerts.
- Introduced a cinematic camera hook + UI controls for multiple camera perspectives (Free/Chase/Cockpit/Earth Observer/Orbital).
- Refactored orbit propagation helpers into a shared utility module and integrated the new UI surfaces into the main layout.
Reviewed changes
Copilot reviewed 11 out of 39 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/tsconfig.app.json | Removes ignoreDeprecations from app TS config. |
| frontend/src/utils/orbitCalc.ts | Adds shared orbital math helpers (propagation, distance, elevation). |
| frontend/src/store/uiStore.ts | Adds UI state for flyby history panel + camera mode selection. |
| frontend/src/store/notificationStore.ts | Adds Zustand store for flyby notifications and user preferences. |
| frontend/src/hooks/useFlybyEngine.ts | Implements periodic flyby prediction + notification creation. |
| frontend/src/hooks/useCinematicCamera.ts | Implements per-frame satellite tracking + camera interpolation modes. |
| frontend/src/components/ui/NotificationCenter.tsx | Adds flyby toasts + history/settings panel UI. |
| frontend/src/components/ui/FlybyNotification.tsx | Adds individual toast rendering + sound/browser notification behavior. |
| frontend/src/components/ui/CameraControls.tsx | Adds bottom-center camera mode switcher UI. |
| frontend/src/components/layouts/MainLayout.tsx | Mounts notification center + camera controls; adds toolbar entry to toggle panel. |
| frontend/src/components/EarthTwin.tsx | Integrates spotlight + cinematic camera hooks; switches to shared orbit util. |
| .gitignore | Adds common Python-related ignores. |
Suppressed comments (3)
frontend/src/components/ui/FlybyNotification.tsx:58
- Requesting notification permission as a side effect of showing an alert can repeatedly prompt the user (one prompt per flyby toast) and is blocked by some browsers unless triggered by explicit user interaction. Prefer only showing notifications when permission is already granted; if you want to request permission, do it from an explicit UI action in the settings panel.
frontend/src/components/ui/NotificationCenter.tsx:70 - The audio toggle is an icon-only button without an accessible name. Add an
aria-labeldescribing the current state/action.
frontend/src/components/ui/FlybyNotification.tsx:89 - The dismiss (X) button is icon-only and lacks an accessible name. Add an
aria-labelso it’s usable with screen readers.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| export function calculateElevationAngle(satelliteAltKm: number, groundDistanceKm: number): number { | ||
| const rE = EARTH_RADIUS_KM; | ||
| const rS = EARTH_RADIUS_KM + satelliteAltKm; | ||
|
|
||
| // Central angle between observer and satellite's nadir | ||
| const gammaRad = groundDistanceKm / rE; | ||
|
|
||
| // 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); | ||
| } |
| import { useEffect, useRef } from 'react'; | ||
| import * as Cesium from 'cesium'; | ||
| import { useUIStore, CameraMode } from '@/store/uiStore'; | ||
| import { CatalogObject } from '@/types/satellite'; |
| // Run immediately, then on interval | ||
| checkFlybys(); | ||
| const intervalId = setInterval(checkFlybys, CHECK_INTERVAL_MS); | ||
|
|
| const playBeep = () => { | ||
| try { | ||
| const AudioContext = window.AudioContext || (window as any).webkitAudioContext; | ||
| if (!AudioContext) return; | ||
|
|
||
| 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); | ||
| } | ||
| }; |
| import { prefersReducedMotion } from './SatelliteSpotlight/GlowEffect'; | ||
| import { useSatelliteSelection } from '@/hooks/useSatelliteSelection'; | ||
| import { useSpotlightEffect } from '@/hooks/useSpotlightEffect'; | ||
| import { useCinematicCamera } from '@/hooks/useCinematicCamera'; | ||
| import { keplerToLatLonAlt } from '@/utils/orbitCalc'; | ||
| import { useUIStore } from '@/store/uiStore'; |
| // 2. Update physical entity position so it visibly moves! | ||
| entity.position = new Cesium.ConstantPositionProperty(p0); | ||
|
|
| // Handle transitioning to FREE mode | ||
| if (mode === 'FREE' || !targetId) { | ||
| if (lastMode.current !== 'FREE') { | ||
| lastMode.current = 'FREE'; | ||
| // Release any overrides if needed, but Cesium camera allows manual control natively | ||
| // when we stop overriding it in preRender. | ||
| } | ||
| return; | ||
| } |
| <button | ||
| onClick={toggleFlybyHistory} | ||
| className={`relative transition-ui cursor-pointer p-2 min-w-[44px] min-h-[44px] flex items-center justify-center ${isFlybyHistoryOpen ? 'text-primary-container drop-shadow-[0_0_8px_rgba(0,229,255,0.6)]' : 'text-primary hover:text-primary-fixed'}`} | ||
| > | ||
| <MaterialIcon name="radar" /> | ||
| </button> |
| <button | ||
| onClick={toggleHistory} | ||
| className="text-on-surface-variant hover:text-primary-container transition-ui" | ||
| > | ||
| <MaterialIcon name="close" /> | ||
| </button> |
|
Hello @SohammPawarr, Please attach screenshots of the updates/changes along with your PR. Thanks! |
User description
Description
Implemented two major visual and real-time tracking features for the Kepler globe:
Free,Chase,Cockpit,Earth Observer, andOrbital) with a sleek UI control panel.Related Issue : #170
Testing Details
Cesium.Cartesian3.lerpsmoothing logic for camera view transitions.CodeAnt-AI Description
Add satellite flyby alerts and cinematic tracking cameras
What Changed
Impact
✅ Earlier notice of visible satellite passes✅ Faster access to live satellite tracking✅ Smoother cinematic globe views💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit