feat: add new globe#619
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds MapLibre GL as a dependency and implements a Map UI wrapper, marker/popup/cluster primitives, a GlobalMap React component rendering clustered city event markers with popups, a server utility grouping events by city, an index page integration (client:load), and a CSS rule for MapLibre popups. Changes
Sequence DiagramsequenceDiagram
participant User
participant IndexPage as "Index Page (server)"
participant EventsLib as "eventsGroupedByCities"
participant GlobalMap as "GlobalMap (client)"
participant MapUI as "Map UI wrapper"
participant MapLib as "MapLibre"
User->>IndexPage: Request page
IndexPage->>EventsLib: eventsGroupedByCities()
EventsLib-->>IndexPage: EventsByCities
IndexPage->>User: serve page (includes GlobalMap client:load)
User->>GlobalMap: browser loads GlobalMap (client)
GlobalMap->>MapUI: mount map, add sources/layers (clusters, points)
MapUI->>MapLib: instantiate MapLibre map and set style
MapLib-->>MapUI: map loaded / styledata ready
User->>MapUI: click cluster or point
MapUI->>GlobalMap: deliver feature + coordinates via handlers
GlobalMap-->>User: open MapPopup with event list / navigation links
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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@src/components/GlobalMap/GlobalMap.tsx`:
- Around line 37-72: The geoJsonData object is rebuilt on every render; wrap its
construction in React's useMemo inside the GlobalMap component: compute
geoJsonData using useMemo(() => { ...same code... }, [events]) so it only
recalculates when the events input changes, preserving the existing types
(FeatureCollection, Point, and the filter type guard) and keeping the
properties/events JSON.stringify logic intact; reference the geoJsonData
variable and the events dependency when applying the change.
In `@src/components/ui/map.tsx`:
- Around line 1128-1147: The initial map.addLayer call for clusterLayerId sets
paint values that don't use the passed-in clusterColors and don't match the
dynamic radii used later; update the paint in the initial map.addLayer (the
object created in the map.addLayer call that references clusterLayerId and
sourceId) to use the clusterColors prop for "circle-color" and to use the same
radius steps as the dynamic update (use clusterThresholds and the same radii
values used later, e.g., 20/30/40 or the variables used there) so initial render
matches subsequent style updates.
- Around line 956-968: Remove the leftover clusterProperties block from the
initial GeoJSON data passed to map.addSource for the route source (look for
map.addSource and the variable sourceId in src/components/ui/map.tsx); the
properties.clusterProperties object is a copy-paste artifact from
MapClusterLayer and is not applicable to a LineString route, so delete the
entire properties.clusterProperties entry (and if desired the enclosing
properties object if unused) so the initial Feature only defines type and
geometry for the LineString.
- Around line 678-701: The handler handleLocate sets setWaitingForLocation(true)
but never clears it when navigator.geolocation is absent; update handleLocate to
immediately reset waiting state in the "geolocation not in navigator" case
(e.g., add an else branch that calls setWaitingForLocation(false) and optionally
logs or calls an error callback), and keep the existing error callback that
clears state on failures; this change should be applied within the handleLocate
function (referenced symbols: handleLocate, setWaitingForLocation,
navigator.geolocation, onLocate, map).
- Around line 1194-1205: The cleanup block is missing removal of the unclustered
point label layer created as `${unclusteredLayerId}-label`; update the teardown
inside the try/catch to check for and remove that layer (e.g., if
(map.getLayer(`${unclusteredLayerId}-label`))
map.removeLayer(`${unclusteredLayerId}-label`)) alongside the existing removals
for clusterCountLayerId, unclusteredLayerId, clusterLayerId and sourceId so no
stale layers remain on remount.
- Around line 845-885: The popup close handler currently invokes onClose twice
because handleClose calls popup.remove() (which emits the "close" event) and
then calls onClose?.() directly; remove the direct onClose?.() call from
handleClose so the single source of truth is the popup's "close" event handler.
Also stop capturing the prop in the effect closure by introducing a mutable ref
(e.g., onCloseRef) that is kept up-to-date with the latest onClose prop and
change onCloseProp to call onCloseRef.current?.() so prop updates are respected;
keep popup.on("close", onCloseProp) and popup.off unchanged except for using the
ref-backed callback.
- Around line 291-330: The marker's event handlers (handleClick,
handleMouseEnter, handleMouseLeave, handleDragStart, handleDrag, handleDragEnd)
are captured once inside the useMemo with empty deps causing stale callbacks;
update the implementation to keep handlers up-to-date by storing the latest prop
callbacks (onClick, onMouseEnter, onMouseLeave, onDragStart, onDrag, onDragEnd)
in refs and referencing those refs inside the registered listener functions on
markerInstance, or alternatively move listener registration into a useEffect
that depends on those props and cleans up prior listeners; ensure you update
references to marker/markerInstance and remove or re-add listeners on change to
avoid leaks.
- Line 150: The current useImperativeHandle call exposes mapInstance as a
non-null MapLibreGL.Map which is unsafe when mapInstance can be null; update the
imperative handle to return a nullable type (e.g. MapLibreGL.Map | null) or
change the MapRef/forwardRef type to accept null and return mapInstance as that
nullable type, and ensure the identifier mapInstance and the useImperativeHandle
invocation are updated accordingly so consumers see a nullable ref instead of a
forced non-null MapLibreGL.Map.
🧹 Nitpick comments (6)
src/lib/events.ts (1)
637-644: Consider a lighter data-fetching path for the map.
getEventsCollectioncallseventWithComputedfor every event, which resolves talks, speakers, organizers, volunteers, and cover images — none of which are used byGlobalMap. This adds unnecessary build-time overhead (manygetEntrycalls).Consider either creating a lightweight fetcher that only resolves city/country data, or extracting a simpler variant of
eventWithComputedfor this use case.src/components/GlobalMap/GlobalMap.tsx (2)
33-35: UnusedmapRef— dead code.
mapRefis created and passed to<Map ref={mapRef}>(line 81) but never read anywhere in this component. Remove it to reduce noise.Suggested fix
export function GlobalMap({ events, className }: GlobalMapProps) { - const mapRef = useRef<maplibregl.Map | null>(null); const [selectedCity, setSelectedCity] = useState<PopupInfo | null>(null);And in the JSX:
- <Map - ref={mapRef} + <Map center={[10, 30]}
93-106:JSON.parsewithout error handling on user-constructed GeoJSON properties.While the data is self-generated via
JSON.stringify(line 58), atry/catcharoundJSON.parse(line 97) would prevent a crash if properties are ever malformed or if MapLibre modifies them during clustering.Suggested fix
const properties = feature.properties; - const cityEvents = - typeof properties?.events === "string" - ? JSON.parse(properties.events) - : properties?.events; + let cityEvents = []; + try { + cityEvents = + typeof properties?.events === "string" + ? JSON.parse(properties.events) + : (properties?.events ?? []); + } catch { + console.error("Failed to parse city events"); + }src/pages/index.astro (2)
41-42: ConsolidateMdPublicimport with existingreact-icons/mdimport block.
MdPublicis imported separately on line 42 while otherreact-icons/mdicons are imported on lines 15–21.Suggested fix
import { MdArrowForward, MdEvent, MdFeed, MdOndemandVideo, MdPodcasts, + MdPublic, } from "react-icons/md"; ... -import { MdPublic } from "react-icons/md";
370-370: Considerclient:visibleinstead ofclient:loadfor the GlobalMap.The map section is well below the fold (after News, Podcasts, and VODs). Using
client:visiblewould defer loading the MapLibre GL bundle (~200KB+) until the user scrolls near the map, improving initial page load performance.- <GlobalMap events={eventsByCities} client:load /> + <GlobalMap events={eventsByCities} client:visible />package.json (1)
55-56: Remove duplicate icon library:lucide-reactis redundant with existingreact-icons.The project already depends on
react-icons@5.5.0which includes Lucide icons via thelupack. All icons used inmap.tsx(X, Minus, Plus, Locate, Maximize, Loader2) are available asLuX,LuMinus,LuPlus,LuLocate,LuMaximize,LuLoader2fromreact-icons/lu—and the codebase already imports fromreact-icons/luelsewhere. Migrate the map component to usereact-icons/luinstead to avoid the extra dependency and reduce bundle size.
JeanneGrenet
left a comment
JeanneGrenet
left a comment
There was a problem hiding this comment.
Maybe it could be great to have a difference between events and meetup on the event list
| dark: "https://basemaps.cartocdn.com/gl/dark-matter-gl-style/style.json", | ||
| light: "https://basemaps.cartocdn.com/gl/positron-gl-style/style.json", |
There was a problem hiding this comment.
We should check to use another map provider because we need a license for carto
| coordinates: [city.location.lng, city.location.lat] as [ | ||
| number, | ||
| number, | ||
| ], |
There was a problem hiding this comment.
Can we use the real address of the event instead of the city ?
There was a problem hiding this comment.
- for this to happen we will be adding more data to the project for each event/meetup while we have global one for the city .
- the usage of lang and lat that exists already in the project makes more sense now since it's only used by the map
houssembaltii
force-pushed
the
feat/add-new-globe-mapcn
branch
from
dff3f9f to
c218673
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/ui/map.tsx (1)
371-398: Consider moving marker property updates into auseEffect.Updating marker properties (position, draggable, offset, rotation, alignment) during render (lines 371-398) are side effects. While this works because the marker instance is stable, it's more idiomatic to wrap these in a
useEffectwith appropriate dependencies. This is a minor refactor suggestion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/map.tsx` around lines 371 - 398, The code is mutating Marker properties during render; move all marker updates (position, draggable, offset, rotation, rotationAlignment, pitchAlignment) into a useEffect that runs when the Marker instance or any relevant prop changes: include marker, longitude, latitude, draggable, and markerOptions.{offset,rotation,rotationAlignment,pitchAlignment} in the dependency array; inside the effect guard that marker exists (e.g., if (!marker) return), compute newOffsetX/Y like current code and call marker.setLngLat, setDraggable, setOffset, setRotation, setRotationAlignment, setPitchAlignment only when values differ to preserve current checks. Ensure no cleanup is required besides leaving the marker intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/ui/map.tsx`:
- Around line 371-398: The code is mutating Marker properties during render;
move all marker updates (position, draggable, offset, rotation,
rotationAlignment, pitchAlignment) into a useEffect that runs when the Marker
instance or any relevant prop changes: include marker, longitude, latitude,
draggable, and markerOptions.{offset,rotation,rotationAlignment,pitchAlignment}
in the dependency array; inside the effect guard that marker exists (e.g., if
(!marker) return), compute newOffsetX/Y like current code and call
marker.setLngLat, setDraggable, setOffset, setRotation, setRotationAlignment,
setPitchAlignment only when values differ to preserve current checks. Ensure no
cleanup is required besides leaving the marker intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b76c77b-7458-4e83-9b8c-cc2604551a45
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
package.jsonsrc/components/GlobalMap/GlobalMap.tsxsrc/components/ui/map.tsxsrc/lib/events.tssrc/pages/index.astrosrc/styles/globals.css
✅ Files skipped from review due to trivial changes (2)
- package.json
- src/styles/globals.css
houssembaltii
force-pushed
the
feat/add-new-globe-mapcn
branch
from
6302c27 to
1aa1563
Compare
| .maplibregl-popup-content { | ||
| background: transparent !important; | ||
| } |
There was a problem hiding this comment.
Is it possible to avoid using !important ?
| </div> | ||
| </div> | ||
| </div> | ||
| <div className="absolute left-1/2 top-4 -translate-x-1/2 rounded-lg border-2 bg-black px-4 py-2"> |
There was a problem hiding this comment.
I feel like this is too visible and taking too much visual space:
| const [selectedCity, setSelectedCity] = useState<PopupInfo | null>(null); | ||
|
|
||
| const { geoJsonData, totalEvents } = useMemo(() => { | ||
| const features = Object.entries(events) |
There was a problem hiding this comment.
I feel like this should be done server side. At the time of retrieving the collection data
This changes the events prop for the component
These feels odd and does not really feed the global app styling
|
houssembaltii
force-pushed
the
feat/add-new-globe-mapcn
branch
from
1aa1563 to
0b5b06f
Compare
JeanneGrenet
left a comment
JeanneGrenet
left a comment
There was a problem hiding this comment.
Can you please rebase this PR
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ted cities Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… specific selector Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Disable built-in attribution to prevent layout shifts on mobile - Add pointer-events-none custom attribution that never blocks touch - Move "Drag to explore" to top-left, desktop-only (hidden on mobile) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Moves the city GeoJSON feature building from a client-side useMemo in GlobalMap to a server-side getCitiesGeoJson function in events.ts, reducing client bundle size and aligning with the Astro data-fetching pattern. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replaces hardcoded hex values with design tokens, fixes blurry text by removing backdrop-blur, adds visual hierarchy to the city/country header, and aligns typography and spacing with the site's EventCard patterns. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…n map Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
houssembaltii
force-pushed
the
feat/add-new-globe-mapcn
branch
from
3d5eac2 to
45c6f9f
Compare
3 participants
Map Tiles OpenFreeMap
The globe uses OpenFreeMap as the tile provider a free, open-source map tile service with no API key required.
Summary by CodeRabbit
New Features
Style
Chores