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: 21 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 ignored due to path filters (27)
📒 Files selected for processing (13)
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! |
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✅ Faster access to live satellite tracking✅ Clearer orbital viewing modes✅ Animated space backdrop💡 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.