fix: resolve merged PR build break (dup types, missing forwardRef wir… - #153
Conversation
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
github-actions
Bot
added
bug
codeant-ai
Bot
added
the
size:XS
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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)
📝 WalkthroughWalkthroughEarthTwin removes its reduced-motion dependency, uses a fixed 1.5-second satellite camera flight, reorders several state hooks, and removes local interface declarations. ChangesEarthTwin updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Fixes a production build break introduced by PR #148 by removing conflicting in-file type declarations and restoring/refining the imperative ref API used by the dashboard to trigger “View on Globe” satellite focusing.
Changes:
- Removed duplicate local
CatalogObject/CollisionRiskinterface declarations and rely on shared type imports. - Ensured
EarthTwinexposes an imperativeflyToSatelliteviaforwardRef/useImperativeHandleand wires focus requests intoSpotlightManager. - Standardized the camera fly-to animation duration to 1.5 seconds.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const pos = keplerToLatLonAlt(obj); | ||
| if (pos) { | ||
| const destination = Cesium.Cartesian3.fromDegrees(pos.lon, pos.lat, pos.alt * 1000 + 2000000); | ||
| viewer.camera.flyTo({ destination, duration: prefersReducedMotion() ? 0 : 1.5 }); | ||
| viewer.camera.flyTo({ destination, duration: 1.5 }); | ||
| } |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| const { activeSector, setSelectedSatelliteId } = useUIStore(); | ||
| const [useFallback, setUseFallback] = useState(true); | ||
| const [hoveredObject, setHoveredObject] = useState<CatalogObject | null>(null); | ||
| const [tooltipPos, setTooltipPos] = useState({ x: 0, y: 0 }); |
There was a problem hiding this comment.
Suggestion: tooltipPos is populated from Cesium's canvas-local mouse coordinates but is later applied to a position: fixed tooltip. When the globe is offset from the viewport origin, the tooltip is displaced by the globe's bounding-rectangle offset (and can also be wrong after scrolling). Convert the Cesium coordinates to client/viewport coordinates before storing them for the fixed-position element. [css layout issue]
Severity Level: Major ⚠️
- ⚠️ Dashboard hover tooltips appear displaced from satellites.
- ⚠️ Scrolled or offset globe layouts worsen tooltip placement.
- ⚠️ Hover-based satellite inspection becomes unreliable.Steps of Reproduction ✅
1. Open the Dashboard, where the globe is rendered inside a positioned section at
`frontend/src/pages/Dashboard.tsx:78-80`, rather than at the viewport origin.
2. Move the pointer over a rendered satellite. `useSatelliteHover` receives Cesium's
canvas-local `movement.endPosition` and stores `x` and `y` directly at
`frontend/src/hooks/useSatelliteHover.ts:32-45`.
3. `SpotlightManager` forwards those values to `EarthTwin` through `onHoverChange` at
`frontend/src/components/SatelliteSpotlight/SpotlightManager.tsx:61-75`, updating the
`tooltipPos` state declared at `frontend/src/components/EarthTwin.tsx:98`.
4. EarthTwin applies the local coordinates to a `position: fixed` tooltip at
`frontend/src/components/EarthTwin.tsx:386-390`. Because fixed positioning expects
viewport coordinates, the tooltip is offset by the globe's page position and can remain
incorrect after scrolling.
(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:** 98:98
**Comment:**
*Css Layout Issue: `tooltipPos` is populated from Cesium's canvas-local mouse coordinates but is later applied to a `position: fixed` tooltip. When the globe is offset from the viewport origin, the tooltip is displaced by the globe's bounding-rectangle offset (and can also be wrong after scrolling). Convert the Cesium coordinates to client/viewport coordinates before storing them for the fixed-position element.
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 (pos) { | ||
| const destination = Cesium.Cartesian3.fromDegrees(pos.lon, pos.lat, pos.alt * 1000 + 2000000); | ||
| viewer.camera.flyTo({ destination, duration: prefersReducedMotion() ? 0 : 1.5 }); | ||
| viewer.camera.flyTo({ destination, duration: 1.5 }); |
There was a problem hiding this comment.
Suggestion: The external fly-to path hard-codes a 1.5-second camera animation, bypassing the existing prefersReducedMotion() behavior used by the double-click path. Users who request reduced motion still receive an animated camera transition from “View on Globe”; use the same reduced-motion check and jump directly when appropriate. [logic error]
Severity Level: Major ⚠️
- ⚠️ Dashboard globe navigation animates for reduced-motion users.
- ⚠️ External navigation differs from double-click camera behavior.Steps of Reproduction ✅
1. Open the Dashboard and activate the Satellite of the Day “View on Globe” action, which
calls `earthTwinRef.current?.flyToSatellite(catalogNumber)` at
`frontend/src/pages/Dashboard.tsx:84`.
2. `EarthTwin.flyToSatellite()` resolves the catalog object and destination at
`frontend/src/components/EarthTwin.tsx:320-327`.
3. With the browser media preference `(prefers-reduced-motion: reduce)` enabled, the
external path still executes `viewer.camera.flyTo({ destination, duration: 1.5 })` at
`frontend/src/components/EarthTwin.tsx:328`.
4. The equivalent double-click path explicitly checks `prefersReducedMotion()` and uses
zero duration at `frontend/src/hooks/useSatelliteSelection.ts:100-105`, demonstrating that
the external path bypasses the established accessibility behavior.
(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:** 328:328
**Comment:**
*Logic Error: The external fly-to path hard-codes a 1.5-second camera animation, bypassing the existing `prefersReducedMotion()` behavior used by the double-click path. Users who request reduced motion still receive an animated camera transition from “View on Globe”; use the same reduced-motion check and jump directly when appropriate.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/EarthTwin.tsx (1)
1-2: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve reduced-motion behavior for external fly-to.
“View on Globe” now animates for 1.5 seconds even when the user requests reduced motion, while
frontend/src/hooks/useSatelliteSelection.ts:34-116still honors that preference. Restore the import and use the same conditional duration here; otherwise this PR introduces a user-visible behavior change.- viewer.camera.flyTo({ destination, duration: 1.5 }); + viewer.camera.flyTo({ + destination, + duration: prefersReducedMotion() ? 0 : 1.5, + });Also applies to: 328-328
🤖 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 1 - 2, Update the external fly-to behavior in EarthTwin to preserve reduced-motion preferences. Restore the reduced-motion import and apply the same conditional animation duration used by useSatelliteSelection, including the “View on Globe” path, so reduced-motion users do not receive the 1.5-second animation.
🤖 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/EarthTwin.tsx`:
- Around line 1-2: Update the external fly-to behavior in EarthTwin to preserve
reduced-motion preferences. Restore the reduced-motion import and apply the same
conditional animation duration used by useSatelliteSelection, including the
“View on Globe” path, so reduced-motion users do not receive the 1.5-second
animation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cbd9111d-c17e-4bda-87bc-ab3bc7da5aa3
📒 Files selected for processing (1)
frontend/src/components/EarthTwin.tsx
|
Please resolve the merge conflict |
|
Fixed and merged - the build break was two issues in EarthTwin.tsx: a leftover duplicate component declaration from the original merge, and some missing ref wiring for the SpotlightManager. Also had to merge in the changes that landed on main since my branch was created. Verified with a clean local build before pushing, and Vercel's deployed fine now. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
frontend/src/components/EarthTwin.tsx:23
- This file still declares local
CatalogObject/CollisionRiskinterfaces even though identical shared types exist in@/types/satellite(and SpotlightManager already imports from there). Keeping duplicate type definitions increases drift risk and undermines the intent of fixing the earlier duplicate-type conflict; prefer importing the shared types here and deleting the local interfaces.
import { SpotlightManager } from './SatelliteSpotlight/SpotlightManager';
interface CatalogObject {
id: number;
name: string;
frontend/src/components/EarthTwin.tsx:589
containerElis later used by SpotlightManager/useSatelliteSelection for keyboard navigation and calls tocontainerEl.focus(...). A plain div isn’t focusable by default, so focus and keydown handling won’t work unless the element is made focusable (e.g.tabIndex=0). You can settabIndexoncontainerRef.currenthere when initializing the viewer to ensure keyboard navigation works.
setViewerInstance(viewer);
// eslint-disable-next-line react-hooks/set-state-in-effect
setContainerEl(containerRef.current);
frontend/src/components/EarthTwin.tsx:589
- EarthTwin’s Cesium init effect still wires its own
ScreenSpaceEventHandler(MOUSE_MOVE + LEFT_CLICK) while SpotlightManager also creates a handler and manages hover/selection/camera focus. Having two independent handlers on the same canvas can result in duplicate listeners and conflicting click/hover behavior (e.g. single-click fly-to firing even though SpotlightManager’s selection hook uses double-click for fly-to). Consider removing the legacy handler wiring in EarthTwin and relying on SpotlightManager’s hooks + callbacks.
viewerRef.current = viewer;
// eslint-disable-next-line react-hooks/set-state-in-effect
setUseFallback(false);
// eslint-disable-next-line react-hooks/set-state-in-effect
setViewerInstance(viewer);
// eslint-disable-next-line react-hooks/set-state-in-effect
setContainerEl(containerRef.current);
User description
Summary
Fixes the production build break introduced when #148 was merged. Two issues in
EarthTwin.tsx:CatalogObjectandCollisionRiskwere both imported from@/types/satelliteand re-declared as local interfaces in the same file, which TypeScript doesn't allow (TS2440).flyToSatellitefeature referencedEarthTwinHandle,useImperativeHandle, and afocusRequestmechanism that weren't fully present, which brokeDashboard.tsx(it expects to attach arefto<EarthTwin>). Restored theforwardRef/useImperativeHandlewrapper and wiredfocusRequestthroughSpotlightManagerso "View on Globe" correctly flies the camera and opens the satellite's info panel.Related Issue
Follow-up fix for the build break caused by #148.
Type of Change
Screenshots / Screen Recordings
N/A, this is a build/type fix with no visual changes.
Testing Performed
Verified with a clean
npm run build(tsc -b && vite build) locally before pushing, no errors.Breaking Changes
None. Purely restores intended behavior from #148; no API or prop changes beyond what #148 already introduced.
Checklist
CodeAnt-AI Description
Restore Earth globe builds and satellite focusing
What Changed
Impact
✅ Successful production builds✅ Working “View on Globe” navigation✅ Consistent satellite camera animations💡 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