Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis change refactors the globe point rendering system to use orbit-based positioning instead of spherical coordinates. Points now orbit along configurable tilted circular paths with dynamic visibility tracking via refs. The globe material is updated to purple-tinted styling, and orbit ring resources are properly cleaned up. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 Tip CodeRabbit can use your project's `biome` configuration to improve the quality of JS/TS/CSS/JSON code reviews.Add a configuration file to your project to customize how CodeRabbit runs |
Deploying ui-kit with
|
| Latest commit: |
321294b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f39ce6e6.ui-kit-d46.pages.dev |
| Branch Preview URL: | https://fix-globe.ui-kit-d46.pages.dev |
Preview Package PublishedA preview version of this package has been published to npm! Version: Install this specific commitnpm install @b3-crow/ui-kit@0.0.39-pr57.1
# or
bun add @b3-crow/ui-kit@0.0.39-pr57.1
# or
yarn add @b3-crow/ui-kit@0.0.39-pr57.1Install latest commit from this PRnpm install @b3-crow/ui-kit@pr-preview
# or
bun add @b3-crow/ui-kit@pr-preview
# or
yarn add @b3-crow/ui-kit@pr-preview
|
There was a problem hiding this comment.
Pull request overview
This PR updates GlobeRenderer to add animated orbit rings, switch point positioning to orbit-based animation, and refresh the globe’s visual styling (material + atmosphere), along with minor UI styling tweaks for point icons.
Changes:
- Added orbit ring configs and runtime ring creation with opacity animation tied to point visibility.
- Reworked point positioning/scaling logic to animate points along orbit paths rather than lat/lng projection.
- Updated globe styling via a custom material and new atmosphere color; adjusted point icon container styling.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const timeout = setTimeout(() => { | ||
| setVisiblePoints(prev => new Set(prev).add(i)); | ||
| setVisiblePoints(prev => { | ||
| const next = new Set(prev).add(i); | ||
| visiblePointsRef.current = next; | ||
| return next; |
There was a problem hiding this comment.
On displayPoints change, the stagger-reveal logic only ever adds indices; it never clears visiblePoints / visiblePointsRef. That can leave points (and orbit ring opacity) visible from a previous points prop, skipping the intended reveal for new data. Consider resetting both sets before scheduling the new timeouts.
| // Create custom purple-tinted material for the globe | ||
| const globeMaterial = new THREE.MeshPhongMaterial({ | ||
| color: new THREE.Color(0.85, 0.45, 0.95), // Purple-magenta tint | ||
| emissive: new THREE.Color(0x3a2559), | ||
| emissiveIntensity: 0.25, | ||
| shininess: 5, | ||
| }); |
There was a problem hiding this comment.
globeMaterial is created inside the effect but never disposed in the cleanup. Since Three.js materials allocate GPU resources, consider disposing this material (and ideally removing/disposing the globe object as well) in the effect cleanup to avoid leaks on unmount/remount.
| const positions: PointPosition[] = displayPoints.map((_point, index) => { | ||
| if (index >= orbitConfigs.length) { | ||
| return { x: 0, y: 0, z: -1, scale: 0, opacity: 0, isInCenter: false }; | ||
| } |
There was a problem hiding this comment.
displayPoints.map returns a hidden placeholder when index >= orbitConfigs.length, which effectively limits the renderer to 3 points. Any additional points will become "visible" (via visiblePoints) but never get a real position, leading to missing/incorrect UI for callers that pass more than 3 points. Consider mapping points onto orbit configs (e.g., modulo assignment) or generating orbit configs/angles per point so all provided points animate and render.
| // Update orbit ring visibility based on visible points | ||
| orbitRings.forEach((ring, index) => { | ||
| const material = ring.material as THREE.LineBasicMaterial; | ||
| const targetOpacity = visiblePointsRef.current.has(index) ? 0.3 : 0; |
There was a problem hiding this comment.
Orbit ring opacity is keyed off visiblePointsRef.current.has(index), where index is the ring index (0..2). Since visiblePoints tracks point indices, this only works coincidentally for the first three points; it won’t correctly show a ring when points are assigned differently (or if you later support >3 points). Consider deriving each ring’s visibility from whether any visible point is mapped to that ring.
| // Update orbit ring visibility based on visible points | |
| orbitRings.forEach((ring, index) => { | |
| const material = ring.material as THREE.LineBasicMaterial; | |
| const targetOpacity = visiblePointsRef.current.has(index) ? 0.3 : 0; | |
| // Update orbit ring visibility based on visible points mapped to each ring | |
| const visibleRingIndices = new Set<number>(); | |
| visiblePointsRef.current.forEach(pointIndex => { | |
| if (pointIndex >= 0 && pointIndex < orbitConfigs.length) { | |
| visibleRingIndices.add(pointIndex); | |
| } | |
| }); | |
| orbitRings.forEach((ring, index) => { | |
| const material = ring.material as THREE.LineBasicMaterial; | |
| const targetOpacity = visibleRingIndices.has(index) ? 0.3 : 0; |
| const rendererRef = useRef<THREE.WebGLRenderer | null>(null); | ||
| const animationRef = useRef<number | undefined>(undefined); | ||
| const globeRef = useRef<ThreeGlobe | null>(null); | ||
| const orbitRingsRef = useRef<THREE.Line[]>([]); |
There was a problem hiding this comment.
orbitRingsRef is declared but never read anywhere in this component. If it’s not needed for external access or a follow-up effect, consider removing it to avoid dead state; otherwise add the usage that requires it.
| const orbitRingsRef = useRef<THREE.Line[]>([]); |
This pull request introduces a significant visual and functional overhaul to the
GlobeRenderercomponent, focusing on orbit ring animations, improved point positioning, and enhanced globe styling. The main changes group into new orbit ring features, globe appearance updates, and point animation logic improvements.Orbit ring animation and configuration:
OrbitConfiginterface and created three orbit rings with distinct tilts, speeds, and radii, animating their visibility based on the currently visible points. [1] [2]Globe appearance improvements:
Point animation and positioning logic:
UI tweaks:
Summary by CodeRabbit
Release Notes
New Features
Style
Improvements