Feature/procedural space background - #180
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
|
Warning Review limit reached
Next review available in: 54 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe frontend adds five satellite camera modes, cinematic Cesium camera control, transparent globe rendering, offline imagery, and a procedural parallax space background. The UI store and main layout expose the new camera controls. ChangesSatellite experience
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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
added
the
size:XL
github-actions
Bot
added
AI
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| <div | ||
| className="relative w-full h-full overflow-hidden bg-bg-deep-space border-b border-border-panel" | ||
| > | ||
| <ProceduralSpaceBackground viewer={viewerInstance} /> |
There was a problem hiding this comment.
Suggestion: The procedural canvas is mounted inside an EarthTwin container with an opaque bg-bg-deep-space background, while the canvas implementation places it at a negative stacking level. This can put the canvas behind its parent's background and make the new procedural stars and nebula invisible, leaving only the opaque panel visible. Keep the background canvas in the visible stacking context or remove the negative z-index. [css layout issue]
Severity Level: Major ⚠️
- ❌ Dashboard globe can show only a flat dark background.
- ⚠️ Procedural stars and nebulae disappear behind opaque layers.
- ⚠️ The advertised animated space environment is unavailable.
(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:** 733:733
**Comment:**
*Css Layout Issue: The procedural canvas is mounted inside an EarthTwin container with an opaque `bg-bg-deep-space` background, while the canvas implementation places it at a negative stacking level. This can put the canvas behind its parent's background and make the new procedural stars and nebula invisible, leaving only the opaque panel visible. Keep the background canvas in the visible stacking context or remove the negative z-index.
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| </div> | ||
|
|
||
| <NotificationCenter /> | ||
| <CameraControls /> |
There was a problem hiding this comment.
Suggestion: Mounting CameraControls at the layout level makes the toolbar appear on every route whenever the global selectedSatelliteId is set. The Satellites page sets that same global selection, so users can see camera-mode controls on a page without a Cesium viewer, and the selected mode remains active when later opening the globe. Render this control only in the EarthTwin/dashboard context or clear/namespace the globe camera state on route changes. [api mismatch]
Severity Level: Major ⚠️
- ⚠️ Camera toolbar appears on non-globe pages.
- ⚠️ Satellites page controls can modify globe-only state.
- ⚠️ Dashboard may open with stale cinematic camera mode.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** frontend/src/components/layouts/MainLayout.tsx
**Line:** 417:417
**Comment:**
*Api Mismatch: Mounting `CameraControls` at the layout level makes the toolbar appear on every route whenever the global `selectedSatelliteId` is set. The Satellites page sets that same global selection, so users can see camera-mode controls on a page without a Cesium viewer, and the selected mode remains active when later opening the globe. Render this control only in the EarthTwin/dashboard context or clear/namespace the globe camera state on route changes.
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| 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.
Suggestion: The asynchronous scan is not invalidated when this effect is cleaned up. If the selected satellites, bookmarks, or warning preference changes while Promise.allSettled is awaiting API responses, the old scan continues and can add notifications using stale inputs after the replacement scan has started. Track an active-run token or cancellation flag and check it before processing results and adding notifications. [race condition]
Severity Level: Major ⚠️
- ⚠️ Stale flyby alerts can appear after tracking changes.
- ⚠️ Bookmark edits can produce alerts for old locations.
- ⚠️ Preference changes do not invalidate pending scans.
(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:** 48:48
**Comment:**
*Race Condition: The asynchronous scan is not invalidated when this effect is cleaned up. If the selected satellites, bookmarks, or warning preference changes while `Promise.allSettled` is awaiting API responses, the old scan continues and can add notifications using stale inputs after the replacement scan has started. Track an active-run token or cancellation flag and check it before processing results and adding notifications.
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
| if (obj.semimajor_axis == null || obj.inclination == null || obj.raan == null || | ||
| obj.arg_of_perigee == null || obj.mean_anomaly == null || obj.mean_motion == null) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Suggestion: The propagator rejects any catalog object whose mean_anomaly is null, even though CatalogObject permits that field to be null and computeOrbitPositions only requires mean_motion. As a result, those otherwise renderable satellites are skipped by EarthTwin and cannot be targeted or propagated by the flyby and camera features. Treat the missing anomaly consistently with the existing orbit-highlighting behavior or exclude such objects explicitly at the data-loading boundary. [api mismatch]
Severity Level: Major ⚠️
- ❌ Catalog objects are skipped by `EarthTwin` entity creation.
- ❌ Camera targeting receives no propagated position.
- ❌ Flyby propagation skips current catalog records.
- ⚠️ Fixing only mean anomaly is insufficient with current API serialization.
(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:** 12:15
**Comment:**
*Api Mismatch: The propagator rejects any catalog object whose `mean_anomaly` is null, even though `CatalogObject` permits that field to be null and `computeOrbitPositions` only requires `mean_motion`. As a result, those otherwise renderable satellites are skipped by `EarthTwin` and cannot be targeted or propagated by the flyby and camera features. Treat the missing anomaly consistently with the existing orbit-highlighting behavior or exclude such objects explicitly at the data-loading boundary.
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 AudioContext = window.AudioContext || (window as any).webkitAudioContext; | ||
| if (!AudioContext) return; | ||
|
|
||
| const ctx = new AudioContext(); |
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 can therefore accumulate open audio contexts and eventually hit browser resource limits. Close the context after playback completes or reuse a shared context. [resource leak]
Severity Level: Major ⚠️
- ⚠️ Long sessions retain one audio context per alert.
- ⚠️ Repeated flyby alerts increase browser audio-resource usage.
- ⚠️ Excessive contexts can cause later audio playback failures.
(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:18
**Comment:**
*Resource Leak: Each alert creates a new `AudioContext`, but the context is never closed after the oscillator stops. Repeated flyby alerts can therefore accumulate open audio contexts and eventually hit browser resource limits. Close the context after playback completes or reuse a shared context.
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| } else if (Notification.permission !== 'denied') { | ||
| Notification.requestPermission(); | ||
| } | ||
| }, [notification, preferences.soundEnabled]); |
There was a problem hiding this comment.
Suggestion: This effect is intended to alert once for a notification, but it also depends on preferences.soundEnabled. Changing the sound preference reruns the effect for every currently displayed notification and creates another browser notification, causing duplicate external alerts. Separate the one-time notification side effect from the sound preference or otherwise track which notification has already been announced. [state lifecycle]
Severity Level: Major ⚠️
- ⚠️ Audio preference changes replay existing flyby beeps.
- ⚠️ Browser users receive duplicate external flyby alerts.
- ⚠️ Notification settings produce unrelated 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:** 59:59
**Comment:**
*State Lifecycle: This effect is intended to alert once for a notification, but it also depends on `preferences.soundEnabled`. Changing the sound preference reruns the effect for every currently displayed notification and creates another browser notification, causing duplicate external alerts. Separate the one-time notification side effect from the sound preference or otherwise track which notification has already been announced.
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| return ( | ||
| <canvas | ||
| ref={canvasRef} | ||
| className="fixed inset-0 pointer-events-none z-[-1]" |
There was a problem hiding this comment.
Suggestion: The background is rendered as a fixed viewport-sized canvas even though this component is mounted inside the smaller EarthTwin panel. It is not constrained to the panel's bounds, so it can paint a full-screen background from inside the globe component and its viewport-sized rendering does not match the panel layout. Size the canvas relative to the EarthTwin container and use absolute positioning within that container. [css layout issue]
Severity Level: Major ⚠️
- ⚠️ Procedural background extends beyond the EarthTwin panel.
- ⚠️ Dashboard sections can receive unintended background rendering.
- ⚠️ Globe-panel resizing does not resize the canvas to panel bounds.
(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/ProceduralSpaceBackground.tsx
**Line:** 155:155
**Comment:**
*Css Layout Issue: The background is rendered as a fixed viewport-sized canvas even though this component is mounted inside the smaller `EarthTwin` panel. It is not constrained to the panel's bounds, so it can paint a full-screen background from inside the globe component and its viewport-sized rendering does not match the panel layout. Size the canvas relative to the EarthTwin container and use absolute positioning within that container.
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| 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; | ||
| } |
There was a problem hiding this comment.
Suggestion: Clearing selectedSatelliteId only causes this handler to return; it does not reset cameraMode. After a user selects a cinematic mode, clears the selection, and selects another satellite later, the old mode is still active and the handler immediately resumes overriding the camera for the new satellite. Reset the mode to FREE when the target is cleared, or make selection clearing perform that reset. [stale reference]
Severity Level: Major ⚠️
- ⚠️ Selecting a later satellite unexpectedly reactivates cinematic tracking.
- ⚠️ User camera control is overridden after clearing selection.
- ⚠️ Camera mode UI state disagrees with the user's cleared target.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** frontend/src/hooks/useCinematicCamera.ts
**Line:** 26:33
**Comment:**
*Stale Reference: Clearing `selectedSatelliteId` only causes this handler to return; it does not reset `cameraMode`. After a user selects a cinematic mode, clears the selection, and selects another satellite later, the old mode is still active and the handler immediately resumes overriding the camera for the new satellite. Reset the mode to `FREE` when the target is cleared, or make selection clearing perform that reset.
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| createdAt: new Date(), | ||
| }; | ||
|
|
||
| return { notifications: [newNotification, ...state.notifications] }; |
There was a problem hiding this comment.
Suggestion: Every non-duplicate notification is prepended to state.notifications, while dismissal only marks entries and does not remove them. The flyby engine runs every 30 seconds and the history UI renders the entire array, so a long-running session can accumulate unbounded history and progressively increase memory use and rendering cost. Retain a bounded history or remove old dismissed entries. [performance]
Severity Level: Major ⚠️
- ⚠️ Long-running sessions retain all dismissed flyby alerts.
- ⚠️ Flyby history rendering cost grows with session duration.
- ⚠️ Repeated alert creation increases client memory usage.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** frontend/src/store/notificationStore.ts
**Line:** 55:55
**Comment:**
*Performance: Every non-duplicate notification is prepended to `state.notifications`, while dismissal only marks entries and does not remove them. The flyby engine runs every 30 seconds and the history UI renders the entire array, so a long-running session can accumulate unbounded history and progressively increase memory use and rendering cost. Retain a bounded history or remove old dismissed entries.
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 fixThere was a problem hiding this comment.
Pull request overview
This PR replaces the static space backdrop with a procedural, animated canvas background behind the Cesium globe, and adds new UX features for satellite tracking including flyby notifications, a flyby history/settings panel, and cinematic camera modes.
Changes:
- Add a layered procedural starfield/nebula canvas synchronized to Cesium camera motion and zoom.
- Introduce a flyby alert engine with notification UI, preferences, and history panel.
- Add cinematic camera modes (Free/Chase/Cockpit/Earth Observer/Orbital) and shared orbital math utilities.
Reviewed changes
Copilot reviewed 12 out of 40 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/tsconfig.app.json | Removes a TS ignoreDeprecations config entry. |
| frontend/src/utils/orbitCalc.ts | Adds shared orbital math utilities used by flyby + camera features. |
| frontend/src/store/uiStore.ts | Adds flyby panel UI state and camera mode state. |
| frontend/src/store/notificationStore.ts | Adds Zustand store for flyby notifications + preferences. |
| frontend/src/hooks/useFlybyEngine.ts | Adds periodic flyby checking and notification generation. |
| frontend/src/hooks/useCinematicCamera.ts | Adds Cesium preRender-driven cinematic camera tracking modes. |
| frontend/src/components/ui/ProceduralSpaceBackground.tsx | Adds procedural canvas background rendered behind the globe. |
| frontend/src/components/ui/NotificationCenter.tsx | Adds global flyby toasts + history/settings panel UI. |
| frontend/src/components/ui/FlybyNotification.tsx | Adds flyby toast UI with optional audio + browser notifications. |
| frontend/src/components/ui/CameraControls.tsx | Adds on-screen camera mode switcher when a satellite is selected. |
| frontend/src/components/layouts/MainLayout.tsx | Mounts NotificationCenter + CameraControls and adds a header toggle. |
| frontend/src/components/EarthTwin.tsx | Integrates procedural background and cinematic camera into the Cesium viewer. |
| .gitignore | Adds common Python ignores. |
Suppressed comments (2)
frontend/src/components/ui/ProceduralSpaceBackground.tsx:109
drawTiledusescanvas.width/heightfor its tiling bounds. If the canvas is ever scaled (e.g., for devicePixelRatio), these values are in device pixels and will cause unnecessary extra tiling work. Usecanvas.clientWidth/clientHeightfor the loop bounds so the logic stays in CSS pixels.
frontend/src/components/ui/ProceduralSpaceBackground.tsx:130- With the canvas potentially being device-pixel scaled, clearing via
canvas.width/heightmixes coordinate spaces and can do extra work. Clear usingclientWidth/clientHeight(CSS pixels) to match the drawing coordinate system.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| 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); |
|
|
||
| // Run immediately, then on interval | ||
| checkFlybys(); | ||
| const intervalId = setInterval(checkFlybys, CHECK_INTERVAL_MS); |
| // 2. Update physical entity position so it visibly moves! | ||
| entity.position = new Cesium.ConstantPositionProperty(p0); | ||
|
|
| // 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(); | ||
| } |
| <canvas | ||
| ref={canvasRef} | ||
| className="fixed inset-0 pointer-events-none z-[-1]" | ||
| style={{ background: '#030508' }} | ||
| /> |
| // 1. Calculate precise real-time position and velocity | ||
| const now = new Date(); | ||
| const p0_geo = keplerToLatLonAlt(catalogData, 0); |
| 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); |
| // Resize observer to keep main canvas full screen | ||
| const resizeObserver = new ResizeObserver(() => { | ||
| canvas.width = window.innerWidth; | ||
| canvas.height = window.innerHeight; | ||
| }); | ||
| resizeObserver.observe(document.body); | ||
|
|
|
Hello @SohammPawarr, Please attach screenshots of the updates/changes along with your PR. Thanks! |
|
hi @krishkhinchi i have attached the ss of the commit |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:XL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (5)
frontend/src/components/ui/FlybyNotification.tsx (1)
85-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd an accessible name to the close button.
The button contains only a
MaterialIcon. The rendered content is the ligature textcloseinside aspan, so screen readers announce the raw glyph name. Addaria-labelandtype="button".♿ Proposed change
<button + type="button" + aria-label="Dismiss flyby alert" onClick={() => dismiss(notification.id)}🤖 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 85 - 90, Update the close button in FlybyNotification to include an accessible aria-label describing its dismiss action and explicitly set type="button", while preserving the existing dismiss onClick behavior and MaterialIcon.frontend/src/utils/orbitCalc.ts (1)
77-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the dead slant-range computation and the draft comments.
Lines 78-83 compute
d,cosEl, andelRad, but the function returns the value from line 89.elRadis never read. The comments on lines 85-87 record intermediate reasoning and do not describe the final behavior.♻️ Proposed cleanup
- // 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 angle: el = atan2(cos(gamma) - rE/rS, sin(gamma)) const el = Math.atan2(Math.cos(gammaRad) - (rE / rS), Math.sin(gammaRad));🤖 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 77 - 89, Remove the unused slant-range and intermediate elevation calculations (`d`, `cosEl`, and `elRad`) from the orbit elevation calculation, along with the draft reasoning comments. Keep the final `el` computation using `Math.atan2` unchanged.frontend/src/hooks/useCinematicCamera.ts (2)
52-53: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse one
ConstantPositionPropertyinstead of allocating per frame.This line runs on every
preRenderframe. Each frame creates a newConstantPositionPropertyand reassignsentity.position, which allocates garbage and raises a definition-changed event each frame. Keep one property per tracked entity and callsetValue.♻️ Proposed refactor
+ const positionPropRef = useRef<Cesium.ConstantPositionProperty | null>(null);- entity.position = new Cesium.ConstantPositionProperty(p0); + if (!(entity.position instanceof Cesium.ConstantPositionProperty)) { + entity.position = new Cesium.ConstantPositionProperty(p0); + } else { + entity.position.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 52 - 53, Update the per-frame position update in the cinematic camera’s preRender handler to reuse a single ConstantPositionProperty per tracked entity instead of constructing and assigning one every frame. Store the property alongside the tracked entity state, then call its setValue with the new p0 position while preserving the existing movement behavior.
8-8: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider making the interpolation frame-rate independent.
LERP_FACTORapplies once per rendered frame. The camera therefore converges twice as fast at 60 FPS as at 30 FPS. Scale the factor by the frame delta time to keep the motion consistent across devices.const SMOOTHING_PER_SECOND = 5.0; const factor = 1 - Math.exp(-SMOOTHING_PER_SECOND * dtSeconds);🤖 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` at line 8, Update the interpolation logic in useCinematicCamera to derive the camera smoothing factor from the frame delta time instead of applying the fixed LERP_FACTOR per render. Use an exponential frame-rate-independent factor based on a per-second smoothing constant, and pass that factor to the camera position interpolation.frontend/src/components/ui/CameraControls.tsx (1)
30-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIcon-only buttons in the new UI have no accessible name. Each of these buttons renders only a
MaterialIcon, which outputs the ligature text inside aspan. Screen readers announce the raw glyph name, and toggle state is conveyed only by color. Addtype="button", an explicit label, and a state attribute where the button is a toggle.
frontend/src/components/ui/CameraControls.tsx#L30-L42: addtype="button",aria-label={mode.label}, andaria-pressed={isActive}to each mode button.frontend/src/components/layouts/MainLayout.tsx#L258-L263: addtype="button",aria-label="Flyby alerts", andaria-expanded={isFlybyHistoryOpen}to the radar toggle.frontend/src/components/ui/FlybyNotification.tsx#L85-L90: addtype="button"andaria-label="Dismiss flyby alert"to the close button.🤖 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` around lines 30 - 42, Add the requested button accessibility attributes at all affected sites: in frontend/src/components/ui/CameraControls.tsx lines 30-42, update each mode button with type="button", aria-label={mode.label}, and aria-pressed={isActive}; in frontend/src/components/layouts/MainLayout.tsx lines 258-263, update the radar toggle with type="button", aria-label="Flyby alerts", and aria-expanded={isFlybyHistoryOpen}; in frontend/src/components/ui/FlybyNotification.tsx lines 85-90, update the close button with type="button" and aria-label="Dismiss flyby alert".
🤖 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: Remove alembic.ini from the ignore rules and add the shared
configuration file to version control with a placeholder database URL,
preserving the script_location setting. Configure migrations to use DATABASE_URL
as the runtime override.
In `@frontend/src/components/EarthTwin.tsx`:
- Around line 567-573: Update the Natural Earth imagery load in the viewer
initialization effect to use the closed-over viewer instance rather than
viewerRef.current, and add the provider only if that viewer is still active and
not destroyed. Track the provider-load promise for cleanup, dispose the provider
or cancel pending work when the effect is cleaned up, and attach a catch handler
for load failures.
In `@frontend/src/components/layouts/MainLayout.tsx`:
- Around line 416-417: Update NotificationCenter and its useFlybyEngine
integration so mounting the dashboard does not call
navigator.geolocation.getCurrentPosition. Defer the location request until the
user enables flyby alerts or selects a satellite, while preserving the existing
flyby behavior after either interaction.
In `@frontend/src/components/ui/FlybyNotification.tsx`:
- Around line 13-37: Update playBeep to prevent accumulating AudioContext
instances by reusing a shared context or closing the newly created context when
the oscillator finishes. Ensure cleanup occurs after the beep completes while
preserving the existing audio behavior and error handling.
- Around line 45-59: Update the FlybyNotification useEffect to run only when
notification.id changes, while reading the current sound preference from the
store at effect time so toggling audio does not repeat alert side effects. Guard
all browser notification access with a typeof Notification check before using
permission, requestPermission, or the constructor, preserving sound playback and
alert behavior when the API is available.
In `@frontend/src/components/ui/NotificationCenter.tsx`:
- Line 44: Update the NotificationCenter history panel class to avoid
narrow-screen clipping: use a responsive width that fits small viewports and
reduce the default right offset, while preserving the existing larger-screen
sm:right-72 behavior and other styling.
In `@frontend/src/hooks/useCinematicCamera.ts`:
- Around line 125-139: In the cinematic camera update flow, re-orthogonalize the
normalized nextUp vector against nextDir before the viewer.camera.setView call:
derive the right vector from nextDir and nextUp, then rebuild nextUp from that
right vector and nextDir, preserving the normalized direction/up pair passed to
setView.
In `@frontend/src/hooks/useFlybyEngine.ts`:
- Around line 31-38: Update the geolocation handling in useFlybyEngine so
denied, failed, or unavailable geolocation leaves userLocationRef.current null
instead of assigning { lat: 0, lon: 0 }, ensuring only bookmarked locations are
evaluated. Add a finite timeout option to the getCurrentPosition call so it
cannot wait indefinitely, while preserving successful location assignment.
- Around line 76-92: Refine the closest-approach search in the propagation loop
around the coarse minimum found by the minute-based sweep. Add a finer-grained
time sweep over a short window surrounding timeOfClosestApproachSec, updating
minDistance, timeOfClosestApproachSec, and closestPos with the same distance
calculation and preserving the existing downstream maxElevation calculation.
- Around line 44-53: Update the checkFlybys effect flow to track cancellation
during cleanup, pass an AbortSignal to each api.getCatalogObjectByNorad request
when supported, and stop processing responses or calling addNotification once
the effect has been torn down. Extend cleanup beyond clearing the interval to
abort in-flight requests and mark the operation inactive.
In `@frontend/src/store/notificationStore.ts`:
- Line 55: Cap the notification history in the state update that prepends new
notifications, ensuring notifications remains bounded while retaining the newest
entries and discarding the oldest beyond the limit. Update the relevant
notificationStore logic around the notifications list and preserve the existing
duplicate guard and ordering behavior.
In `@frontend/src/utils/orbitCalc.ts`:
- Around line 20-23: Validate the parsed epoch in the orbit calculation before
using epochDate.getTime(); when obj.epoch is unparseable, return null so invalid
coordinates never reach Cesium or the useCinematicCamera preRender listener.
Preserve the existing current-time fallback when obj.epoch is absent and the
normal calculation for valid dates.
---
Nitpick comments:
In `@frontend/src/components/ui/CameraControls.tsx`:
- Around line 30-42: Add the requested button accessibility attributes at all
affected sites: in frontend/src/components/ui/CameraControls.tsx lines 30-42,
update each mode button with type="button", aria-label={mode.label}, and
aria-pressed={isActive}; in frontend/src/components/layouts/MainLayout.tsx lines
258-263, update the radar toggle with type="button", aria-label="Flyby alerts",
and aria-expanded={isFlybyHistoryOpen}; in
frontend/src/components/ui/FlybyNotification.tsx lines 85-90, update the close
button with type="button" and aria-label="Dismiss flyby alert".
In `@frontend/src/components/ui/FlybyNotification.tsx`:
- Around line 85-90: Update the close button in FlybyNotification to include an
accessible aria-label describing its dismiss action and explicitly set
type="button", while preserving the existing dismiss onClick behavior and
MaterialIcon.
In `@frontend/src/hooks/useCinematicCamera.ts`:
- Around line 52-53: Update the per-frame position update in the cinematic
camera’s preRender handler to reuse a single ConstantPositionProperty per
tracked entity instead of constructing and assigning one every frame. Store the
property alongside the tracked entity state, then call its setValue with the new
p0 position while preserving the existing movement behavior.
- Line 8: Update the interpolation logic in useCinematicCamera to derive the
camera smoothing factor from the frame delta time instead of applying the fixed
LERP_FACTOR per render. Use an exponential frame-rate-independent factor based
on a per-second smoothing constant, and pass that factor to the camera position
interpolation.
In `@frontend/src/utils/orbitCalc.ts`:
- Around line 77-89: Remove the unused slant-range and intermediate elevation
calculations (`d`, `cosEl`, and `elRad`) from the orbit elevation calculation,
along with the draft reasoning comments. Keep the final `el` computation using
`Math.atan2` 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: 900bae80-927b-4d72-91f8-9eb9fd4f0e2e
⛔ 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 (13)
.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/components/ui/ProceduralSpaceBackground.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.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether alembic.ini and Python caches are currently tracked.
git ls-files | rg -n 'alembic\.ini|\.pyc$|__pycache__' || echo "No matching tracked files."Repository: 7-Blocks/Kepler
Length of output: 180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== .gitignore relevant entries =="
nl -ba .gitignore | sed -n '1,160p' | rg -n 'alembic|alembic\.ini|__pycache__|\.pyc' || true
echo
echo "== repo files with alembic =="
git ls-files | rg -n 'alembic|alembic\.ini|alembic_versions|versions' || true
echo
echo "== all tracked migration/config references =="
git ls-files | rg -n '\.(ini|py|toml|yml|yaml)$' | xargs -r rg -n 'script_location|sqlalchemy\.url|db\.url|alembic|ALEMBIC|DATABASE_URL' || true
echo
echo "== Alembic files in repo, excluding tracked =="
git ls-files --others --exclude-standard | rg -n 'alembic|alembic\.ini|versions' || true
echo
echo "== environment variable references =="
rg -n 'DATABASE_URL|ALEMBIC\w*|sqlalchemy\.url|script_location' -S . --glob '!**/.git/**' --glob '!**/node_modules/**' || trueRepository: 7-Blocks/Kepler
Length of output: 7005
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== .gitignore first 160 lines =="
awk '{printf "%6d\t%s\n", NR, $0}' .gitignore | sed -n '1,160p'
echo
echo "== repo files with alembic or versions =="
git ls-files | rg -n '(^|/)alembic\.ini$|alembic|versions' || echo "No matching tracked files."
echo
echo "== untracked files matching alembic/version/cache =="
git ls-files --others --exclude-standard | rg -n '(^|/)alembic\.ini$|alembic|versions|__pycache__|\.pyc' || echo "No untracked matching files."
echo
echo "== migration/config references in tracked files =="
git ls-files | rg '(\.ini|\.py|\.toml|\.yml|\.yaml)$' || true | xargs -r -n1000 sh -c 'rm -f ".tmp_rg_batch" ; rg -n "script_location|sqlalchemy\.url|db\.url|alembic|ALEMBIC|DATABASE_URL|env\.config" "$@" && touch .tmp_batch' sh {} + || trueRepository: 7-Blocks/Kepler
Length of output: 4937
Reconsider ignoring alembic.ini.
alembic.ini contains shared migration configuration, including script_location. If the repo has no tracked Alembic files other than this ignore rule, contributors cannot run migrations without recreating the file. Commit the file with a placeholder URL and override the database URL through DATABASE_URL.
🤖 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, Remove alembic.ini from the ignore rules and add the
shared configuration file to version control with a placeholder database URL,
preserving the script_location setting. Configure migrations to use DATABASE_URL
as the runtime override.
| // Add offline Natural Earth imagery layer as fallback | ||
| Cesium.TileMapServiceImageryProvider.fromUrl( | ||
| Cesium.buildModuleUrl('Assets/Textures/NaturalEarthII') | ||
| ).then((provider) => { | ||
| if (viewerRef.current && !viewerRef.current.isDestroyed()) { | ||
| viewerRef.current.imageryLayers.addImageryProvider(provider); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP -C2 '<(?:React\.)?StrictMode\b' frontend
rg -n -C4 'TileMapServiceImageryProvider\.fromUrl|addImageryProvider' frontend/src/components/EarthTwin.tsxRepository: 7-Blocks/Kepler
Length of output: 884
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the Cesium viewer creation effect scope and cleanup.
sed -n '520,590p' frontend/src/components/EarthTwin.tsx | cat -n
# Check whether another provider add exists nearby and whether the request has any catch/finally handling.
sed -n '540,575p' frontend/src/components/EarthTwin.tsx | cat -nRepository: 7-Blocks/Kepler
Length of output: 4292
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the full effect cleanup and any later viewer assignment/additions involving Natural Earth.
sed -n '590,660p' frontend/src/components/EarthTwin.tsx | cat -n
rg -n -C3 'NaturalEarthII|TileMapServiceImageryProvider|viewerRef\.current\s*=|addImageryProvider' frontend/src/components/EarthTwin.tsxRepository: 7-Blocks/Kepler
Length of output: 3790
Scope the imagery request to the created viewer and handle rejection.
viewerRef.current can be updated by a later effect run before fromUrl() resolves, so this callback may add the Natural Earth provider to the replacement viewer. The app mounts under StrictMode, which can duplicate this during reinitialization when the first load is fast. Also, the request is unhandled if the imagery fails to load.
Keep the closed-over viewer for this provider load, dispose on cleanup, add the provider only while that viewer remains active, and attach a .catch() handler.
🤖 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` around lines 567 - 573, Update the
Natural Earth imagery load in the viewer initialization effect to use the
closed-over viewer instance rather than viewerRef.current, and add the provider
only if that viewer is still active and not destroyed. Track the provider-load
promise for cleanup, dispose the provider or cancel pending work when the effect
is cleaned up, and attach a catch handler for load failures.
| <NotificationCenter /> | ||
| <CameraControls /> |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
NotificationCenter requests geolocation on every dashboard load.
NotificationCenter calls useFlybyEngine on line 11 of frontend/src/components/ui/NotificationCenter.tsx. That hook calls navigator.geolocation.getCurrentPosition in a mount effect. Mounting the component here means the browser shows the location permission prompt as soon as the dashboard opens, before the user asks for any flyby feature. Request the location only after the user enables flyby alerts or selects a satellite.
🤖 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 NotificationCenter and its useFlybyEngine integration so mounting the
dashboard does not call navigator.geolocation.getCurrentPosition. Defer the
location request until the user enables flyby alerts or selects a satellite,
while preserving the existing flyby behavior after either interaction.
| 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.
playBeep constructs a new AudioContext on every call and never closes it. Browsers cap the number of concurrent AudioContext instances per document; Chrome allows about six. After that limit, the constructor throws, the catch block logs a warning, and alert audio stops for the rest of the session. Reuse one shared context, or close it when the oscillator ends.
🛠️ Proposed fix
+let sharedCtx: AudioContext | null = null;
+
const playBeep = () => {
try {
const AudioContext = window.AudioContext || (window as any).webkitAudioContext;
if (!AudioContext) return;
-
- const ctx = new AudioContext();
+
+ const ctx = sharedCtx ?? (sharedCtx = new AudioContext());
+ if (ctx.state === 'suspended') void ctx.resume();
+
const osc = ctx.createOscillator(); osc.start();
osc.stop(ctx.currentTime + 0.5);
+ osc.onended = () => {
+ osc.disconnect();
+ gainNode.disconnect();
+ };📝 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); | |
| } | |
| }; | |
| let sharedCtx: AudioContext | null = null; | |
| const playBeep = () => { | |
| try { | |
| const AudioContext = window.AudioContext || (window as any).webkitAudioContext; | |
| if (!AudioContext) return; | |
| const ctx = sharedCtx ?? (sharedCtx = new AudioContext()); | |
| if (ctx.state === 'suspended') void ctx.resume(); | |
| 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 = () => { | |
| osc.disconnect(); | |
| gainNode.disconnect(); | |
| }; | |
| } 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 prevent accumulating AudioContext instances by reusing a
shared context or closing the newly created context when the oscillator
finishes. Ensure cleanup occurs after the beep completes while preserving the
existing audio behavior and error handling.
| 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
Fire the alert side effects once per notification.
The dependency array includes preferences.soundEnabled. When the user toggles audio in NotificationCenter while an alert is visible, this effect runs again. It replays the beep and posts a second browser notification for the same flyby. Depend on notification.id only and read the preference from the store at call time.
Notification also needs a guard. The constructor is absent in some browsers, including older iOS Safari, and in non-secure contexts. Line 51 dereferences Notification.permission unconditionally, which throws a ReferenceError and unmounts the alert through the error boundary.
🛠️ Proposed fix
useEffect(() => {
- if (preferences.soundEnabled) {
+ if (useNotificationStore.getState().preferences.soundEnabled) {
playBeep();
}
-
+
// Optional: Use browser notifications API if permitted
+ if (typeof Notification === 'undefined') 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]);
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, [notification.id]);🤖 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 useEffect to run only when notification.id changes,
while reading the current sound preference from the store at effect time so
toggling audio does not repeat alert side effects. Guard all browser
notification access with a typeof Notification check before using permission,
requestPermission, or the constructor, preserving sound playback and alert
behavior when the API is available.
| (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.
If the user denies geolocation, userLocationRef.current becomes (0, 0). That point is in the Gulf of Guinea. The engine then emits "Current Location" flyby alerts for a place the user is not at. Leave the reference null instead, so only bookmarked locations are evaluated. Also add a timeout option, because getCurrentPosition waits indefinitely by default.
🛠️ 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 flybys are disabled.', error);
+ userLocationRef.current = null;
+ },
+ { timeout: 10000, maximumAge: 300000 }
);
} 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 flybys are disabled.', error); | |
| userLocationRef.current = null; | |
| }, | |
| { timeout: 10000, maximumAge: 300000 } | |
| ); | |
| } 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 handling in useFlybyEngine so denied, failed, or unavailable
geolocation leaves userLocationRef.current null instead of assigning { lat: 0,
lon: 0 }, ensuring only bookmarked locations are evaluated. Add a finite timeout
option to the getCurrentPosition call so it cannot wait indefinitely, while
preserving successful location assignment.
| 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.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Cancel in-flight work when the effect is cleaned up.
checkFlybys awaits api.getCatalogObjectByNorad for every tracked satellite. The cleanup on line 128 clears the interval only. A request that is already in flight still resolves after the effect is torn down, and the code then calls addNotification for a selection that is no longer active. Add a cancellation flag and an AbortSignal if the API client supports one.
🛠️ Proposed fix
+ let cancelled = false;
+
const checkFlybys = async () => {
try {
const satPromises = selectedSatelliteIds.map(id => api.getCatalogObjectByNorad(id));
const responses = await Promise.allSettled(satPromises);
+ if (cancelled) return; const intervalId = setInterval(checkFlybys, CHECK_INTERVAL_MS);
- return () => clearInterval(intervalId);
+ return () => {
+ cancelled = true;
+ clearInterval(intervalId);
+ };📝 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 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); | |
| let cancelled = false; | |
| 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 (cancelled) return; | |
| const satellites = responses | |
| .filter((res): res is PromiseFulfilledResult<any> => res.status === 'fulfilled') | |
| .map(res => res.value.data as SpaceObject) | |
| .filter(Boolean); |
| 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); | |
| const intervalId = setInterval(checkFlybys, CHECK_INTERVAL_MS); | |
| return () => { | |
| cancelled = true; | |
| clearInterval(intervalId); | |
| }; |
🤖 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, Update the
checkFlybys effect flow to track cancellation during cleanup, pass an
AbortSignal to each api.getCatalogObjectByNorad request when supported, and stop
processing responses or calling addNotification once the effect has been torn
down. Extend cleanup beyond clearing the interval to abort in-flight requests
and mark the operation inactive.
| // Propagate forward minute-by-minute | ||
| for (let min = 0; min <= PROPAGATION_MINUTES; min++) { | ||
| const timeOffsetSec = min * 60; | ||
| const pos = keplerToLatLonAlt(catalogObj, timeOffsetSec); | ||
|
|
||
| if (pos) { | ||
| const dist = calculateGroundDistanceKm(loc.lat, loc.lon, pos.lat, pos.lon); | ||
| if (dist < minDistance) { | ||
| minDistance = dist; | ||
| timeOfClosestApproachSec = timeOffsetSec; | ||
| closestPos = pos; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (closestPos) { | ||
| const maxElevation = calculateElevationAngle(closestPos.alt, minDistance); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Refine the closest approach with a finer time step.
The loop samples once per minute. A low-Earth-orbit ground track advances about 420 km between samples. The reported maxElevationDeg and ETA therefore come from an arbitrary point near the pass, not from the true closest approach. A short pass can also fall between two samples and produce no alert at all. Run a second, finer sweep around the coarse minimum.
🛠️ Proposed fix
+ // Refine around the coarse minimum at 5-second resolution.
+ if (closestPos) {
+ const coarse = timeOfClosestApproachSec;
+ for (let t = Math.max(0, coarse - 60); t <= coarse + 60; t += 5) {
+ const pos = keplerToLatLonAlt(catalogObj, t);
+ if (!pos) continue;
+ const dist = calculateGroundDistanceKm(loc.lat, loc.lon, pos.lat, pos.lon);
+ if (dist < minDistance) {
+ minDistance = dist;
+ timeOfClosestApproachSec = t;
+ closestPos = pos;
+ }
+ }
+ }
+
if (closestPos) {
const maxElevation = calculateElevationAngle(closestPos.alt, minDistance);📝 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.
| // Propagate forward minute-by-minute | |
| for (let min = 0; min <= PROPAGATION_MINUTES; min++) { | |
| const timeOffsetSec = min * 60; | |
| const pos = keplerToLatLonAlt(catalogObj, timeOffsetSec); | |
| if (pos) { | |
| const dist = calculateGroundDistanceKm(loc.lat, loc.lon, pos.lat, pos.lon); | |
| if (dist < minDistance) { | |
| minDistance = dist; | |
| timeOfClosestApproachSec = timeOffsetSec; | |
| closestPos = pos; | |
| } | |
| } | |
| } | |
| if (closestPos) { | |
| const maxElevation = calculateElevationAngle(closestPos.alt, minDistance); | |
| // Propagate forward minute-by-minute | |
| for (let min = 0; min <= PROPAGATION_MINUTES; min++) { | |
| const timeOffsetSec = min * 60; | |
| const pos = keplerToLatLonAlt(catalogObj, timeOffsetSec); | |
| if (pos) { | |
| const dist = calculateGroundDistanceKm(loc.lat, loc.lon, pos.lat, pos.lon); | |
| if (dist < minDistance) { | |
| minDistance = dist; | |
| timeOfClosestApproachSec = timeOffsetSec; | |
| closestPos = pos; | |
| } | |
| } | |
| } | |
| // Refine around the coarse minimum at 5-second resolution. | |
| if (closestPos) { | |
| const coarse = timeOfClosestApproachSec; | |
| for (let t = Math.max(0, coarse - 60); t <= coarse + 60; t += 5) { | |
| const pos = keplerToLatLonAlt(catalogObj, t); | |
| if (!pos) continue; | |
| const dist = calculateGroundDistanceKm(loc.lat, loc.lon, pos.lat, pos.lon); | |
| if (dist < minDistance) { | |
| minDistance = dist; | |
| timeOfClosestApproachSec = t; | |
| closestPos = pos; | |
| } | |
| } | |
| } | |
| if (closestPos) { | |
| const maxElevation = calculateElevationAngle(closestPos.alt, minDistance); |
🤖 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 76 - 92, Refine the
closest-approach search in the propagation loop around the coarse minimum found
by the minute-based sweep. Add a finer-grained time sweep over a short window
surrounding timeOfClosestApproachSec, updating minDistance,
timeOfClosestApproachSec, and closestPos with the same distance calculation and
preserving the existing downstream maxElevation calculation.
| createdAt: new Date(), | ||
| }; | ||
|
|
||
| return { notifications: [newNotification, ...state.notifications] }; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Cap the notification history.
notifications grows without a bound. useFlybyEngine runs every 30 seconds and evaluates every tracked satellite against every bookmarked location, so entries accumulate for the whole session. The duplicate guard on lines 39-44 skips only non-dismissed entries inside a 5-minute ETA window, so dismissed entries remain forever. NotificationCenter renders the full list on lines 111-124, so both memory use and render cost grow over time. Truncate the list.
🛠️ Proposed fix
+const MAX_NOTIFICATIONS = 100;- return { notifications: [newNotification, ...state.notifications] };
+ return { notifications: [newNotification, ...state.notifications].slice(0, MAX_NOTIFICATIONS) };📝 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.
| return { notifications: [newNotification, ...state.notifications] }; | |
| const MAX_NOTIFICATIONS = 100; | |
| 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, ensuring
notifications remains bounded while retaining the newest entries and discarding
the oldest beyond the limit. Update the relevant notificationStore logic around
the notifications list and preserve the existing duplicate guard and ordering
behavior.
| 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.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard against an unparseable epoch.
new Date(obj.epoch) returns Invalid Date for any non-parseable string. getTime() then returns NaN, and the function returns { lat: NaN, lon: NaN, alt } instead of null. Cesium.Cartesian3.fromDegrees throws a DeveloperError for NaN inputs. In useCinematicCamera this call runs inside the preRender listener, so the throw repeats on every frame.
🛡️ Proposed fix
- const epochDate = obj.epoch ? new Date(obj.epoch) : new Date();
const now = new Date();
+ const epochDate = obj.epoch ? new Date(obj.epoch) : now;
+ if (Number.isNaN(epochDate.getTime())) return null;
const elapsedDays = (now.getTime() - epochDate.getTime()) / 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 epochDate = obj.epoch ? new Date(obj.epoch) : now; | |
| if (Number.isNaN(epochDate.getTime())) return null; | |
| const elapsedDays = (now.getTime() - epochDate.getTime()) / 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 parsed
epoch in the orbit calculation before using epochDate.getTime(); when obj.epoch
is unparseable, return null so invalid coordinates never reach Cesium or the
useCinematicCamera preRender listener. Preserve the existing current-time
fallback when obj.epoch is absent and the normal calculation for valid dates.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/layouts/MainLayout.tsx (1)
23-26: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRemove the duplicate store declarations.
sidebarCollapsed,rightDrawerOpen,toggleSidebar, andtoggleRightDrawerare already declared by theuseUIStore()destructuring at Lines 15-22. Redeclaring them here causes a TypeScript compile error.Keep only
isFlybyHistoryOpenandtoggleFlybyHistoryin the full-store destructuring, or convert all store access to selector calls.🤖 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 23 - 26, Remove the duplicate sidebarCollapsed, rightDrawerOpen, toggleSidebar, and toggleRightDrawer declarations from MainLayout’s selector section, since they already come from the useUIStore() destructuring. Retain only isFlybyHistoryOpen and toggleFlybyHistory there, while preserving the existing store access behavior.
🤖 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.
Outside diff comments:
In `@frontend/src/components/layouts/MainLayout.tsx`:
- Around line 23-26: Remove the duplicate sidebarCollapsed, rightDrawerOpen,
toggleSidebar, and toggleRightDrawer declarations from MainLayout’s selector
section, since they already come from the useUIStore() destructuring. Retain
only isFlybyHistoryOpen and toggleFlybyHistory there, while preserving the
existing store access behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 44a05d81-52fb-4a4d-8cc3-06d292f480e5
📒 Files selected for processing (2)
frontend/src/components/EarthTwin.tsxfrontend/src/components/layouts/MainLayout.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/EarthTwin.tsx
github-actions
Bot
added
github-actions
krishkhinchi
added
ECSoC26
ecsoc-sentinel
Bot
added
ECSoC26-L2
User description
Description
Replaced the current static space background with a fully procedural, animated space environment. Instead of relying on a fixed background image, this PR implements a highly performant HTML5 Canvas layered behind the Cesium globe that renders thousands of stars and softly animated nebulae.
preRenderevent to perfectly synchronize background offsets with cameraheadingandpitch.CameraModeandCatalogObjectTypeScript imports.Related Issue: #171
Testing Details
tsc) passes successfully.CodeAnt-AI Description
Add live satellite flyby alerts, cinematic tracking views, and a procedural space backdrop
What Changed
Impact
✅ Earlier satellite flyby warnings✅ Live satellite tracking from alerts✅ Smoother cinematic camera views✅ Offline globe loading💡 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