Conversation
…, and related geospatial improvements
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (13)
✨ 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 |
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
Key issues are performance and correctness risks in the new elevation API: it can trigger hundreds/thousands of external Mapbox requests per call and lacks strict input/token validation. Several components regress to any for useActions/useUIState, reducing safety and maintainability. clearChat() no longer rotates chatId, which can break “new chat” semantics and persistence behavior. The heatmap layer also needs proper event-handler cleanup to avoid leaks/duplication on re-renders.
Additional notes (2)
-
Maintainability |
components/search-related.tsx:24-24
setMessages((currentMessages: any) => ...)is anotheranyleak. Even if you don’t want to type the full message union, you can at least type it asUIState(exported fromapp/actions.tsx) to preserve basic structure ({id, component, isCollapsed?}) and reduce runtime mistakes. -
Maintainability |
components/map/elevation-heatmap-layer.tsx:23-31
ElevationHeatmapLayerusesuseState<any>(null)formapboxgl. Since the module is only used fornew mapboxgl.Popup(), you can avoid storing the whole module in state by dynamically importing just when needed (on click) or storing a ref. Current approach can cause an extra render and keepsanyaround unnecessarily.
Summary of changes
Overview
This diff re-introduces the “full features” set from PR #533 onto current main, notably:
-
Elevation heatmap pipeline
- Adds a new API route
GET/POST /api/elevationthat queries Mapbox Terrain via Tilequery. - Adds client rendering via
ElevationHeatmapLayerand integrates it alongside GeoJSON overlays throughMapResultsContainer. - Extends the resolution-search agent schema/prompt to optionally request elevation sampling via
elevationData.
- Adds a new API route
-
Map results composition
- Replaces direct
<GeoJsonLayer />usage inapp/actions.tsxUI generation with<MapResultsContainer />to support multiple map overlays.
- Replaces direct
-
Chat persistence and auth
- Uses
getCurrentUserIdOnServer()directly to derive the user id. - Simplifies
onSetAIStateto save{ chatId, messages }ondone.
- Uses
-
Client integration tweaks
- Several components swap
useActions<typeof AI>()/useUIState<typeof AI>()toany. - Root layout replaces inline
<script dangerouslySetInnerHTML>with a staticpublic/htmx-fallback.jsloaded vianext/script.
- Several components swap
Notable new files
app/api/elevation/route.tscomponents/map/elevation-heatmap-layer.tsxcomponents/map/map-results-container.tsxlib/utils/elevation.tspublic/htmx-fallback.js
| const points: Array<{ lng: number; lat: number; elevation: number | null }> = []; | ||
| const lonStep = (east - west) / gridSize; | ||
| const latStep = (north - south) / gridSize; | ||
|
|
||
| const polygon = geometry ? turf.polygon(geometry.coordinates) : null; | ||
|
|
||
| for (let i = 0; i <= gridSize; i++) { | ||
| for (let j = 0; j <= gridSize; j++) { | ||
| const lng = west + (lonStep * i); | ||
| const lat = south + (latStep * j); | ||
|
|
||
| if (polygon) { | ||
| const point = turf.point([lng, lat]); | ||
| if (!turf.booleanPointInPolygon(point, polygon)) { | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| points.push({ lng, lat, elevation: null }); | ||
| } | ||
| } | ||
|
|
||
| const token = process.env.MAPBOX_ACCESS_TOKEN || process.env.NEXT_PUBLIC_MAPBOX_ACCESS_TOKEN; | ||
|
|
||
| // Fetch elevation data using Mapbox Terrain vector tiles (v2) | ||
| // This tileset contains contour lines with 'ele' property | ||
| const elevationPromises = points.map(async (point) => { | ||
| try { | ||
| const response = await fetch( | ||
| `https://api.mapbox.com/v4/mapbox.mapbox-terrain-v2/tilequery/${point.lng},${point.lat}.json?access_token=${token}` | ||
| ); | ||
|
|
||
| if (response.ok) { | ||
| const data = await response.json(); | ||
| if (data.features && data.features.length > 0) { | ||
| // Find the feature with the highest elevation in this point's vicinity | ||
| // or just take the first one if it has 'ele' | ||
| const elevations = data.features | ||
| .map((f: any) => f.properties.ele) | ||
| .filter((e: any) => e !== undefined); | ||
|
|
||
| if (elevations.length > 0) { | ||
| point.elevation = Math.max(...elevations); | ||
| } | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.error(`Error fetching elevation for ${point.lng},${point.lat}:`, error); | ||
| } | ||
| return point; | ||
| }); | ||
|
|
||
| const elevationData = await Promise.all(elevationPromises); | ||
| const validPoints = elevationData.filter(p => p.elevation !== null); | ||
|
|
||
| if (validPoints.length === 0) { | ||
| return NextResponse.json({ | ||
| success: true, | ||
| points: [], | ||
| statistics: { min: 0, max: 0, average: 0, count: 0 }, | ||
| bounds, | ||
| gridSize, | ||
| }); | ||
| } | ||
|
|
||
| const elevations = validPoints.map(p => p.elevation as number); | ||
| const minElevation = Math.min(...elevations); | ||
| const maxElevation = Math.max(...elevations); | ||
| const avgElevation = elevations.reduce((sum, e) => sum + e, 0) / elevations.length; | ||
|
|
||
| return NextResponse.json({ | ||
| success: true, | ||
| points: validPoints, | ||
| statistics: { | ||
| min: minElevation, | ||
| max: maxElevation, | ||
| average: Math.round(avgElevation * 10) / 10, | ||
| count: validPoints.length, | ||
| }, | ||
| bounds, | ||
| gridSize, | ||
| }); | ||
|
|
There was a problem hiding this comment.
/api/elevation performs one Mapbox Tilequery request per grid point (worst case (gridSize+1)^2). With the default gridSize=20, that’s 441 external requests per request, and it scales quadratically. This is likely to hit Mapbox rate limits, slow responses significantly, and increase server costs.
Also, POST calls GET repeatedly (via GET(new NextRequest(url))) which multiplies the above per-feature; this can become explosive under batch usage.
Suggestion
Consider switching to a raster-based approach (Mapbox Terrain-RGB tiles) or otherwise drastically reducing external calls:
- Prefer fetching a small set of terrain tiles and sampling pixels server-side.
- Enforce hard limits: cap
gridSize(e.g. max 30), cap number of points, and cap number of features inPOST. - Add caching (e.g.
unstable_cache/KV) keyed by{bounds, gridSize, geometryHash}. - If you keep tilequery, throttle concurrency (e.g.
p-limit) and consider a single request per cell centroid at lower grid sizes.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit implementing caps + concurrency limiting + basic caching hooks.
| const token = process.env.MAPBOX_ACCESS_TOKEN || process.env.NEXT_PUBLIC_MAPBOX_ACCESS_TOKEN; | ||
|
|
||
| // Fetch elevation data using Mapbox Terrain vector tiles (v2) | ||
| // This tileset contains contour lines with 'ele' property | ||
| const elevationPromises = points.map(async (point) => { | ||
| try { | ||
| const response = await fetch( | ||
| `https://api.mapbox.com/v4/mapbox.mapbox-terrain-v2/tilequery/${point.lng},${point.lat}.json?access_token=${token}` | ||
| ); |
There was a problem hiding this comment.
The access token is read from env, but if it’s missing/empty the code still issues requests like ...access_token=null, causing many failing external calls and slow responses. This should fail fast with a clear 500/400 response.
Additionally, you’re interpolating access_token directly into a URL string. It’s safer to construct using URL/URLSearchParams to avoid accidental injection/encoding issues.
Suggestion
Add an early guard:
- If
!token, returnNextResponse.json({ error: 'Missing MAPBOX access token' }, { status: 500 }). - Build the request with
const u = new URL(...); u.searchParams.set('access_token', token);.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this guard + safer URL construction.
| let bounds; | ||
| try { | ||
| bounds = JSON.parse(boundsParam); | ||
| } catch (e) { | ||
| return NextResponse.json({ error: 'Invalid bounds parameter' }, { status: 400 }); | ||
| } | ||
|
|
||
| const gridSize = gridSizeParam ? parseInt(gridSizeParam) : 20; | ||
| const geometry = geometryParam ? JSON.parse(geometryParam) : null; | ||
|
|
||
| const [west, south, east, north] = bounds; | ||
|
|
||
| const points: Array<{ lng: number; lat: number; elevation: number | null }> = []; | ||
| const lonStep = (east - west) / gridSize; | ||
| const latStep = (north - south) / gridSize; | ||
|
|
||
| const polygon = geometry ? turf.polygon(geometry.coordinates) : null; | ||
|
|
There was a problem hiding this comment.
geometry is JSON.parse(geometryParam) without a try/catch. A malformed geometry query will throw and result in a 500 instead of a controlled 400, unlike bounds which is validated.
Similarly, gridSize isn’t validated for NaN, negative, or huge values (which directly impacts request fan-out).
Suggestion
Validate inputs consistently:
- Wrap
geometryparsing in try/catch and return 400 on failure. - Validate
gridSize: default to 20 whenNaN, clamp to a maximum, and require>= 1. - Validate
boundsis an array of 4 finite numbers andwest<east,south<north.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with robust validation + clamping.
| export async function POST(req: NextRequest) { | ||
| try { | ||
| const body = await req.json(); | ||
| const { features, gridSize = 20 } = body; | ||
|
|
||
| if (!features || !Array.isArray(features)) { | ||
| return NextResponse.json( | ||
| { error: 'Missing or invalid features array' }, | ||
| { status: 400 } | ||
| ); | ||
| } | ||
|
|
||
| const results = await Promise.all( | ||
| features.map(async (feature: any) => { | ||
| if (feature.geometry.type !== 'Polygon') { | ||
| return null; | ||
| } | ||
|
|
||
| const bbox = turf.bbox(feature); | ||
| const params = new URLSearchParams({ | ||
| bounds: JSON.stringify(bbox), | ||
| gridSize: gridSize.toString(), | ||
| geometry: JSON.stringify(feature.geometry), | ||
| }); | ||
|
|
||
| const url = new URL(`/api/elevation?${params}`, req.url); | ||
| const response = await GET(new NextRequest(url)); | ||
| return await response.json(); | ||
| }) | ||
| ); |
There was a problem hiding this comment.
In POST, you construct new URL(/api/elevation?${params}, req.url) and then call GET(new NextRequest(url)). This is an unusual pattern and can have subtle issues:
req.urlincludes the current path; using it as a base may produce unexpected results if the route is not at the same origin/path.- You lose the original request context (headers, geo, etc.).
- You duplicate JSON parsing/validation behavior and create a tight coupling between handlers.
Prefer extracting the elevation computation into a shared function and calling it from both handlers.
Suggestion
Refactor to a shared internal function, e.g. computeElevation({ bounds, gridSize, geometry }), used by both GET and POST. POST can compute bbox/geometry then call the shared function directly (no synthetic NextRequest).
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this refactor.
| // Handle elevation heat map if requested | ||
| let elevationPointsData = null; | ||
| if (analysisResult.elevationData?.requested && analysisResult.elevationData.bounds) { | ||
| try { | ||
| const elevationResponse = await fetch( | ||
| `${process.env.NEXT_PUBLIC_BASE_URL || 'http://localhost:3000'}/api/elevation?` + | ||
| `bounds=${JSON.stringify(analysisResult.elevationData.bounds)}&gridSize=${analysisResult.elevationData.gridSize || 20}${ | ||
| drawnFeatures.length > 0 && drawnFeatures[0].geometry | ||
| ? `&geometry=${JSON.stringify(drawnFeatures[0].geometry)}` | ||
| : '' | ||
| }` | ||
| ); | ||
|
|
||
| if (elevationResponse.ok) { | ||
| const elevationData = await elevationResponse.json(); | ||
| if (elevationData.success && elevationData.points.length > 0) { | ||
| elevationPointsData = elevationData; | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.error('Error fetching elevation data:', error); | ||
| } | ||
| } |
There was a problem hiding this comment.
Server action builds an elevation fetch URL using NEXT_PUBLIC_BASE_URL || 'http://localhost:3000' and sends large JSON blobs via query params (bounds, optional geometry). This risks:
- Broken in production if
NEXT_PUBLIC_BASE_URLis unset/mis-set, or in preview environments with different hosts. - URL length limits (especially when
geometryis included). - Encoding issues (raw JSON in query string).
Since this code runs server-side, you can call the route via a relative URL only if you have a proper fetch base; but in Next.js server actions, safest is to avoid HTTP loopback and call shared logic directly (or use headers() to derive origin).
Suggestion
Prefer one of:
-
Move elevation fetching logic into a shared server utility (e.g.
lib/server/elevation.ts) and call it directly from the action, bypassing HTTP. -
If you must call the API route, use
new URL(req.url)-style origin derivation (e.g. fromheaders()/next/headers) and POST JSON ({ bounds, gridSize, geometry }) to avoid huge query strings.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit converting this to a POST with JSON + safer origin handling.
| const { mapProvider } = useSettingsStore() | ||
| const { mapData } = useMapData() | ||
| // Cast the actions to our defined interface to avoid build errors | ||
| const actions = useActions<typeof AI>() as unknown as HeaderActions | ||
| const [, setMessages] = useUIState<typeof AI>() | ||
| const actions = useActions<any>() as unknown as HeaderActions | ||
| const [, setMessages] = useUIState<any>() | ||
| const [isAnalyzing, setIsAnalyzing] = useState(false) |
There was a problem hiding this comment.
Several client components replaced useActions<typeof AI>() / useUIState<typeof AI>() with any.
Even if tsc passes, this is unsafe but type-valid and removes compile-time guarantees for action names/return shapes. It also increases the chance of runtime issues when actions change.
Given this PR already exports AIState/UIState types, the better fix is to reintroduce correct generic typing (or export a dedicated AppAI type) rather than widening to any.
Suggestion
Avoid any here by restoring the AI type binding:
- Re-add
import { AI } from '@/app/actions'and useuseActions<typeof AI>()/useUIState<typeof AI>(). - If circular deps were the reason, export a
type AppAI = typeof AIfrom a non-RSC module or atypes.tsfile and import only the type.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit that fixes these to use proper generics (type-only imports to avoid runtime cycles).
| export const MobileIconsBar: React.FC<MobileIconsBarProps> = ({ onAttachmentClick, onSubmitClick }) => { | ||
| const [, setMessages] = useUIState<typeof AI>() | ||
| const [, setMessages] = useUIState<any>() | ||
| const { clearChat } = useActions() | ||
| const { toggleCalendar } = useCalendarToggle() |
There was a problem hiding this comment.
Same issue as above: useUIState<any>() discards action/UI-state typing and can hide breaking changes. This is a code quality regression compared to the prior typed usage.
Suggestion
Restore strong typing for useUIState/useActions by importing AI (or a type-only alias) and using useUIState<typeof AI>(). If you need to avoid importing the server module at runtime, use import type { AppAI } ... from a shared types file.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit applying this pattern across all affected components.
| export function ResolutionCarousel({ mapboxImage, googleImage, initialImage }: ResolutionCarouselProps) { | ||
| const actions = useActions<typeof AI>() as any | ||
| const [, setMessages] = useUIState<typeof AI>() | ||
| const actions = useActions<any>() as any | ||
| const [, setMessages] = useUIState<any>() | ||
| const [isAnalyzing, setIsAnalyzing] = React.useState(false) |
There was a problem hiding this comment.
Same pattern: useActions<any>() / useUIState<any>() in a core interaction component makes it easy to call non-existent actions or mis-handle return values. This is particularly risky since submit returns a complex UI payload.
Suggestion
Re-introduce proper generics by importing the AI type (type-only if needed) and removing any.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit updating ResolutionCarousel, HeaderSearchButton, SearchRelated, and MobileIconsBar together for consistent typing.
| const onMapLoad = () => { | ||
| if (!map.getSource(sourceId)) { | ||
| map.addSource(sourceId, { | ||
| type: 'geojson', | ||
| data: geojson | ||
| }) | ||
|
|
||
| map.addLayer({ | ||
| id: heatmapLayerId, | ||
| type: 'heatmap', | ||
| source: sourceId, | ||
| paint: { | ||
| 'heatmap-weight': ['interpolate', ['linear'], ['get', 'intensity'], 0, 0, 1, 1], | ||
| 'heatmap-intensity': ['interpolate', ['linear'], ['zoom'], 0, 1, 15, 3], | ||
| 'heatmap-color': [ | ||
| 'interpolate', | ||
| ['linear'], | ||
| ['heatmap-density'], | ||
| 0, 'rgba(33,102,172,0)', | ||
| 0.2, 'rgb(103,169,207)', | ||
| 0.4, 'rgb(209,229,240)', | ||
| 0.6, 'rgb(253,219,199)', | ||
| 0.8, 'rgb(239,138,98)', | ||
| 1, 'rgb(178,24,43)' | ||
| ], | ||
| 'heatmap-radius': ['interpolate', ['linear'], ['zoom'], 0, 2, 15, 20], | ||
| 'heatmap-opacity': ['interpolate', ['linear'], ['zoom'], 7, 0.7, 15, 0.5] | ||
| } | ||
| }) | ||
|
|
||
| map.addLayer({ | ||
| id: pointsLayerId, | ||
| type: 'circle', | ||
| source: sourceId, | ||
| minzoom: 14, | ||
| paint: { | ||
| 'circle-radius': ['interpolate', ['linear'], ['zoom'], 14, 3, 22, 8], | ||
| 'circle-color': [ | ||
| 'interpolate', | ||
| ['linear'], | ||
| ['get', 'intensity'], | ||
| 0, '#2166ac', | ||
| 0.25, '#67a9cf', | ||
| 0.5, '#f7f7f7', | ||
| 0.75, '#ef8a62', | ||
| 1, '#b2182b' | ||
| ], | ||
| 'circle-stroke-color': 'white', | ||
| 'circle-stroke-width': 1, | ||
| 'circle-opacity': ['interpolate', ['linear'], ['zoom'], 14, 0, 15, 0.8] | ||
| } | ||
| }) | ||
|
|
||
| const clickHandler = (e: any) => { | ||
| if (!e.features || e.features.length === 0) return | ||
| const elevation = e.features[0].properties?.elevation | ||
| if (elevation !== undefined) { | ||
| new mapboxgl.Popup() | ||
| .setLngLat(e.lngLat) | ||
| .setHTML(`<strong>Elevation:</strong> ${elevation}m`) | ||
| .addTo(map) | ||
| } | ||
| } | ||
|
|
||
| map.on('click', pointsLayerId, clickHandler) | ||
| map.on('mouseenter', pointsLayerId, () => { map.getCanvas().style.cursor = 'pointer' }) | ||
| map.on('mouseleave', pointsLayerId, () => { map.getCanvas().style.cursor = '' }) | ||
| } | ||
| } | ||
|
|
||
| if (map.isStyleLoaded()) { | ||
| onMapLoad() | ||
| } else { | ||
| map.once('load', onMapLoad) | ||
| } | ||
|
|
||
| return () => { | ||
| if (map) { | ||
| if (map.getLayer(pointsLayerId)) map.removeLayer(pointsLayerId) | ||
| if (map.getLayer(heatmapLayerId)) map.removeLayer(heatmapLayerId) | ||
| if (map.getSource(sourceId)) map.removeSource(sourceId) | ||
| } | ||
| } |
There was a problem hiding this comment.
ElevationHeatmapLayer registers map event handlers (click, mouseenter, mouseleave) but cleanup only removes layers/sources. If the effect re-runs (e.g. points updated, style reload), you can accumulate handlers or leave handlers referencing removed layers.
Also, the handler functions are created inside the effect, but never deregistered via map.off.
Suggestion
Store handler references and remove them during cleanup:
map.off('click', pointsLayerId, clickHandler)- likewise for mouseenter/mouseleave handlers
- Consider updating the source data when it already exists (
(map.getSource(sourceId) as GeoJSONSource).setData(geojson)) rather than no-op when present.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit that adds proper event cleanup and updates existing sources on re-render.
| interface MapResultsContainerProps { | ||
| id: string | ||
| geoJson?: FeatureCollection | ||
| elevationPoints?: { | ||
| points: any[] | ||
| statistics: any | ||
| } | ||
| } |
There was a problem hiding this comment.
MapResultsContainer types elevationPoints as { points: any[]; statistics: any }. This defeats most of the benefit of adding an elevation feature and makes it easy to pass malformed data into the map layer.
You already define ElevationPoint and statistics shape in ElevationHeatmapLayer; these should be shared and reused here to keep the integration safe.
Suggestion
Export shared types (e.g. from components/map/elevation-types.ts or from ElevationHeatmapLayer) and type elevationPoints as:
{ points: ElevationPoint[]; statistics: ElevationStatistics }
Reply with "@CharlieHelps yes please" if you'd like me to add a commit introducing shared types and removing these anys.
1 participant
This PR re-implements the functionality from PR #533 on the current main branch. It includes: \n- Elevation heatmap support\n- Google Satellite preview and comparison\n- Elevation data fetching and visualization\n- Follow-up chat and related queries integration\n- Geospatial tool improvements