Elevation Heat Map Implementation for Resolution Search#533
Elevation Heat Map Implementation for Resolution Search#533ngoiyaeric wants to merge 9 commits intomainfrom
Conversation
- Added /api/elevation endpoint to fetch real elevation data via Mapbox Tilequery API - Created ElevationHeatmapLayer component for Mapbox visualization - Updated Resolution Search agent to request elevation analysis when appropriate - Integrated elevation data processing and persistence in app/actions.tsx - Added utility functions for bounds calculation and elevation decoding in lib/utils/elevation.ts Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
The main blockers are performance/reliability risks in /api/elevation: it fans out to hundreds of external Mapbox requests concurrently and accepts an unbounded gridSize, creating a DoS/rate-limit vector. The client/server integration also uses brittle GET query construction with raw JSON.stringify and a base URL fallback that can break in production. On the map side, ElevationHeatmapLayer doesn’t remove event listeners on cleanup and won’t update the GeoJSON source when points change, which can cause leaks and stale rendering.
Additional notes (1)
- Performance |
app/actions.tsx:161-161
elevationPointsDatais persisted into the assistant message content aselevationPoints. This can be a very large payload (hundreds of points), and chat persistence will grow quickly, potentially impacting DB size, network transfer, and rehydration time.
Given this is derived data, persisting everything may be unnecessary; you could persist only { bounds, gridSize, geometry } and re-fetch on restore, or persist a compressed representation.
Summary of changes
What changed
Elevation data pipeline
- Added a new
/api/elevationendpoint (app/api/elevation/route.ts) to sample elevation over a grid inside a bounding box, optionally clipped by a GeoJSON polygon, using Mapboxtilequeryagainstmapbox-terrain-v2. - Persisted elevation results in the chat/AI message payload by storing
elevationPointsalongside otherresolution_search_resultmetadata.
UI / Map rendering
- Added a client-side Mapbox layer component
ElevationHeatmapLayer(components/map/elevation-heatmap-layer.tsx) that renders:- a
heatmaplayer colored from low→high elevation - a
circlepoints layer visible at zoom>=14, with click popups showing elevation
- a
- Updated
getUIStateFromAIStateto restore the heatmap layer from persistedelevationPoints.
Agent schema & prompt
- Extended the
resolutionSearchoutput schema (lib/agents/resolution-search.tsx) with optionalelevationDatato let the agent request an elevation heatmap and provide bounds.
Utilities
- Added
lib/utils/elevation.tswith helpers for bounds calculation and Terrain-RGB decoding/coloring.
| // 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; | ||
| uiStream.append( | ||
| <ElevationHeatmapLayer | ||
| id={`${groupeId}-elevation`} | ||
| points={elevationData.points} | ||
| statistics={elevationData.statistics} | ||
| /> | ||
| ); | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.error('Error fetching elevation data:', error); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The server action builds a GET URL with JSON.stringify(...) directly in the query string for bounds and geometry. This is brittle (special characters like {, ", spaces) and can easily exceed URL length limits, especially for polygon geometries. It also forces you to depend on NEXT_PUBLIC_BASE_URL/localhost to call your own backend, which is unnecessary and error-prone in server environments.
Net effect: elevation fetch can silently fail in production (bad URL, truncated query, wrong base URL), even though the feature is “correct” locally.
Suggestion
Prefer a same-origin relative URL and send complex payloads via POST with a JSON body (or at least encode with URLSearchParams). For example:
- Use
POST /api/elevationand pass{ features, gridSize }or{ bounds, gridSize, geometry }in the body. - If you keep
GET, build params withnew URLSearchParams({ bounds: JSON.stringify(...), ... })to ensure encoding.
Also consider calling the handler logic directly (shared function) instead of fetching your own app.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit implementing the POST-based request + safer param handling.
| 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); |
There was a problem hiding this comment.
This endpoint fires one Mapbox Tilequery request per grid point ((gridSize+1)^2). With the default gridSize=20, that’s 441 external requests per call, all launched concurrently via Promise.all. That’s a major reliability and cost issue:
- High chance of hitting Mapbox rate limits / request bursts.
- Large fan-out can starve server resources.
- Latency will be dominated by the slowest requests.
This needs concurrency limiting and/or a different sampling strategy (vector tile fetch, coarser grid, adaptive sampling, or batch where possible).
Suggestion
Add a concurrency limiter (e.g. p-limit) around the per-point fetches and consider capping gridSize server-side. Example approach:
- Validate
gridSizeto a safe range (e.g.5..30) and reject/clip larger values. - Use
pLimit(8)(or similar) to process points with bounded concurrency. - Optionally add caching keyed by
(bounds, gridSize, geometryHash)to avoid repeated fan-out.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with concurrency limiting + gridSize validation.
| 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.
geometryParam is parsed without a try/catch. If the query contains invalid JSON (which is plausible given the current client-side URL construction), this will throw and return a 500 rather than a 400.
Separately, turf.polygon(geometry.coordinates) assumes the geometry is a Polygon and that coordinates is shaped correctly. If geometry.type is not Polygon or coordinates are malformed, this will also throw and bubble to the outer catch (500).
Suggestion
Harden request parsing:
- Wrap
JSON.parse(geometryParam)in a try/catch and return400on failure. - Validate
geometry.type === 'Polygon'before callingturf.polygon(...).
Reply with "@CharlieHelps yes please" if you'd like me to add a commit adding defensive parsing/validation and consistent 400 responses.
| 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; | ||
|
|
||
| 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}` | ||
| ); |
There was a problem hiding this comment.
This endpoint allows unbounded gridSize from a query param, which can be abused to trigger massive fan-out to Mapbox and tie up server resources (DoS-by-proxy). Even with concurrency limiting, you should validate input and enforce ceilings.
Also, if MAPBOX_ACCESS_TOKEN is missing, the endpoint still attempts requests; better to fail fast with a clear 500/503 error.
Suggestion
Add server-side validation and fail-fast checks:
- Clamp or reject
gridSizeoutside a safe range. - Validate bounds array length and numeric ranges.
- If no Mapbox token is configured, return an explicit error response.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit implementing validation + clearer error handling.
| 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.
POST /api/elevation calls GET by constructing a new NextRequest(url) and then calling GET(new NextRequest(url)). This is an unusual pattern and can behave differently than a direct function call because it creates a synthetic request (and may not carry headers, cookies, IP, etc.). It also makes error handling and status codes harder to manage because you always call response.json() without checking response.ok.
If you want an internal helper, extract the shared logic into a pure function (e.g., queryElevation({bounds, gridSize, geometry})) and call it from both handlers.
Suggestion
Refactor shared logic into a helper and avoid synthetic NextRequest objects:
async function queryElevation({ bounds, gridSize, geometry }: { bounds: number[]; gridSize: number; geometry?: any }) {
// existing GET logic, but return a plain object
}
export async function GET(req: NextRequest) {
// parse params; return NextResponse.json(await queryElevation(...))
}
export async function POST(req: NextRequest) {
// parse body; call queryElevation per feature
// ensure you propagate per-item errors/statuses appropriately
}Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this suggestion.
| const onMapLoad = () => { | ||
| if (!map.getSource(sourceId)) { | ||
| // Add the data source | ||
| map.addSource(sourceId, { | ||
| type: 'geojson', | ||
| data: geojson | ||
| }) | ||
|
|
||
| // Add heatmap layer | ||
| map.addLayer({ | ||
| id: heatmapLayerId, | ||
| type: 'heatmap', | ||
| source: sourceId, | ||
| paint: { | ||
| // Increase weight based on elevation | ||
| 'heatmap-weight': [ | ||
| 'interpolate', | ||
| ['linear'], | ||
| ['get', 'intensity'], | ||
| 0, 0, | ||
| 1, 1 | ||
| ], | ||
| // Increase intensity as zoom increases | ||
| 'heatmap-intensity': [ | ||
| 'interpolate', | ||
| ['linear'], | ||
| ['zoom'], | ||
| 0, 1, | ||
| 15, 3 | ||
| ], | ||
| // Color ramp for the heatmap (elevation-based) | ||
| // Blue (low) -> Cyan -> White -> Orange -> Red (high) | ||
| '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)' | ||
| ], | ||
| // Adjust heatmap radius by zoom level | ||
| 'heatmap-radius': [ | ||
| 'interpolate', | ||
| ['linear'], | ||
| ['zoom'], | ||
| 0, 2, | ||
| 15, 20 | ||
| ], | ||
| // Opacity | ||
| 'heatmap-opacity': [ | ||
| 'interpolate', | ||
| ['linear'], | ||
| ['zoom'], | ||
| 7, 0.7, | ||
| 15, 0.5 | ||
| ] | ||
| } | ||
| }) | ||
|
|
||
| // Add circle layer for high zoom levels | ||
| 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 | ||
| ] | ||
| } | ||
| }) | ||
|
|
||
| // Add click handler to show elevation value | ||
| const clickHandler = (e: any) => { | ||
| if (!e.features || e.features.length === 0) return | ||
|
|
||
| const feature = e.features[0] | ||
| const elevation = feature.properties?.elevation | ||
|
|
||
| if (elevation !== undefined) { | ||
| new mapboxgl.Popup() | ||
| .setLngLat(e.lngLat) | ||
| .setHTML(`<strong>Elevation:</strong> ${elevation}m`) | ||
| .addTo(map) | ||
| } | ||
| } | ||
|
|
||
| map.on('click', pointsLayerId, clickHandler) | ||
|
|
||
| // Change cursor on hover | ||
| const mouseEnterHandler = () => { | ||
| map.getCanvas().style.cursor = 'pointer' | ||
| } | ||
| const mouseLeaveHandler = () => { | ||
| map.getCanvas().style.cursor = '' | ||
| } | ||
|
|
||
| map.on('mouseenter', pointsLayerId, mouseEnterHandler) | ||
| map.on('mouseleave', pointsLayerId, mouseLeaveHandler) | ||
| } | ||
| } | ||
|
|
||
| if (map.isStyleLoaded()) { | ||
| onMapLoad() | ||
| } else { | ||
| map.once('load', onMapLoad) | ||
| } | ||
|
|
||
| // Cleanup | ||
| 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.
The map layer registers event handlers (click, mouseenter, mouseleave) but the cleanup only removes layers/sources. The handlers will persist across re-renders/style reloads and can accumulate, leading to duplicated popups/cursor toggling and memory leaks.
Also, the code only adds layers if the source doesn’t exist; it never updates the GeoJSON when points changes (same id). So subsequent renders with new data won’t refresh the visualization.
Suggestion
Track handlers and remove them in cleanup:
map.off('click', pointsLayerId, clickHandler)etc.
And update data when the source already exists:
const source = map.getSource(sourceId) as mapboxgl.GeoJSONSource; source.setData(geojson).
Optionally also handle styledata/style reloads if your app changes styles.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit that fixes handler cleanup and ensures setData updates the source.
- Added /api/elevation endpoint using Mapbox Terrain vector tiles - Created ElevationHeatmapLayer with dynamic mapbox-gl import to prevent server errors - Integrated ElevationHeatmapLayer in app/actions.tsx using next/dynamic with ssr: false - Updated Resolution Search agent to support elevation analysis requests - Added elevation data persistence to chat history Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
|
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 (1)
WalkthroughAdds elevation analysis and visualization: a new /api/elevation endpoint to query Mapbox Terrain, elevation utility functions, an ElevationHeatmapLayer, a MapResultsContainer to render geojson + elevation, and updates to AI/UI state and resolution-search flows to include elevationPoints. Changes
Sequence DiagramsequenceDiagram
participant Client as Client/UI
participant Actions as app/actions.tsx
participant ElevAPI as /api/elevation
participant Mapbox as Mapbox Terrain API
participant MapComp as MapResultsContainer
participant Heatmap as ElevationHeatmapLayer
Client->>Actions: trigger resolution search
Actions->>Actions: extract bounds / prepare request
Actions->>ElevAPI: GET/POST bounds + gridSize (or geometry)
ElevAPI->>Mapbox: tilequery for grid points
Mapbox-->>ElevAPI: terrain RGB features
ElevAPI->>ElevAPI: decode RGB, compute per-point elevation & stats
ElevAPI-->>Actions: return elevationPoints + statistics
Actions->>MapComp: render geoJson + elevationPoints
MapComp->>Heatmap: pass points & stats
Heatmap->>Heatmap: build GeoJSON, compute intensities, add layers
Heatmap-->>Client: display heatmap and popups
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/actions.tsx (1)
204-215: 🧹 Nitpick | 🔵 TrivialLarge elevation payload serialized into chat history may degrade performance.
elevationPointsData(up to 441 points, each withlng,lat,elevation) is stored verbatim viaJSON.stringifyinto the AI state messages. Over multiple resolution searches in a single chat, this can significantly inflate the persisted chat size, impacting load times and storage costs.Consider storing only the elevation request parameters (bounds, gridSize) and re-fetching on reload, or storing a compressed/summarized form.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/actions.tsx` around lines 204 - 215, The message currently serializes the full elevationPointsData into the AI chat state (see the object built with id: groupeId, role: 'assistant', content: JSON.stringify({...analysisResult, image: dataUrl, mapboxImage: mapboxDataUrl, googleImage: googleDataUrl, elevationPoints: elevationPointsData, type: 'resolution_search_result'})); instead, stop embedding the full points array—replace elevationPoints with either a small elevationRequestParams object (e.g., bounds, gridSize, resolutionSearchId) or a compressed/summarized payload (min/max/mean/std/count or a base64-compressed blob) and persist that under a new key like elevationRequest or elevationSummary; update any consumers that read elevationPoints to re-fetch the detailed points on reload using the stored request params or to decompress the summary, and ensure the stringified content uses the new elevationRequest/elevationSummary field instead of elevationPointsData.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
app/actions.tsxapp/api/elevation/route.tscomponents/map/elevation-heatmap-layer.tsxlib/agents/resolution-search.tsxlib/utils/elevation.ts
🧰 Additional context used
🧬 Code graph analysis (3)
lib/utils/elevation.ts (1)
components/map/mapbox-map.tsx (1)
feature(79-165)
components/map/elevation-heatmap-layer.tsx (2)
components/map/geojson-layer.tsx (5)
GeoJsonLayer(13-100)map(16-97)map(89-96)map(24-80)GeoJsonLayerProps(8-11)components/map/mapbox-map.tsx (1)
map(67-168)
app/actions.tsx (1)
components/map/elevation-heatmap-layer.tsx (1)
ElevationHeatmapLayer(24-193)
🔇 Additional comments (4)
components/map/elevation-heatmap-layer.tsx (1)
24-51: LGTM on props interface and GeoJSON construction.The intensity normalization with a fallback to
0.5whenmin === maxis a good defensive practice, and the GeoJSON construction is straightforward.lib/agents/resolution-search.tsx (1)
74-75: LGTM on prompt updates.The instructions for the AI to set
elevationData.requestedand provide bounds when relevant are clear and well-integrated with the existing prompt structure.app/actions.tsx (2)
24-27: LGTM on dynamic import setup.Using
next/dynamicwithssr: falsefor the Mapbox-dependentElevationHeatmapLayeris the correct approach to avoid server-sidemapbox-glmodule errors. The named-export.then(mod => mod.ElevationHeatmapLayer)pattern is correct.
778-804: LGTM ongetUIStateFromAIStateelevation rendering.The null-safe check
elevationPoints && elevationPoints.pointsbefore renderingElevationHeatmapLayeris correct, and the component is rendered alongside existing GeoJSON/carousel results as expected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/actions.tsx`:
- Around line 129-159: This server action should not call its own /api/elevation
over HTTP; extract the elevation logic into a shared function (e.g.,
getElevationData) inside the same module as app/api/elevation/route.ts and
import it into app/actions.tsx, then replace the fetch block that builds
`${process.env.NEXT_PUBLIC_BASE_URL...}/api/elevation?...` with a direct call
like await getElevationData({bounds: analysisResult.elevationData.bounds,
gridSize: analysisResult.elevationData.gridSize, geometry:
drawnFeatures[0]?.geometry}); assign the returned object to elevationPointsData
and keep the existing uiStream.append(<ElevationHeatmapLayer
id={`${groupeId}-elevation`} points={...} statistics={...} />) logic; if
extracting the shared function is infeasible right now, at minimum build the
query using URLSearchParams or encodeURIComponent for bounds and geometry to
avoid malformed URLs before calling fetch.
In `@app/api/elevation/route.ts`:
- Around line 42-56: The code currently builds a large points array in route.ts
(points from the nested loops that push { lng, lat, elevation: null }) and then
fires unbounded concurrent fetches to the Mapbox Tilequery API (the Promise.all
at the fetch stage), which will hit rate limits; update the fetch phase to use a
concurrency-limited strategy (e.g., process points in chunks or use a
concurrency utility like p-limit) so only N requests run concurrently (choose a
safe default like 5–10), and replace the direct Promise.all over all points with
batched Promise.all calls or a p-limit wrapped fetch; ensure you reference the
points array and the function or inline block that performs the Tilequery fetch
to apply the concurrency control and handle/retry 429 responses gracefully.
- Around line 119-128: The catch blocks in the GET and POST handlers (the
try/catch surrounding the elevation fetch/processing in route.ts) currently
return error instanceof Error ? error.message to the client; change these to
return a generic error message (e.g. "Internal server error" or "Failed to fetch
elevation data") instead while keeping the detailed error logged server-side via
console.error(error). Update both the GET catch and the POST catch so
NextResponse.json responses do not include error.message or other internal
details and still return status: 500.
- Around line 31-32: Wrap the JSON.parse call for geometryParam in a try/catch
(same pattern as boundsParam) so a malformed geometry query returns a 400 with a
descriptive error instead of throwing a 500; specifically, update the logic
around geometryParam and the geometry variable in route handler (where
geometryParam is parsed into geometry) to catch SyntaxError from JSON.parse and
call return new NextResponse("Invalid geometry parameter", { status: 400 }) (or
the project’s existing bad-request response helper).
- Around line 143-159: The POST handler's loop assumes feature.geometry exists
and constructs internal GETs without passing/validating the auth token; add a
guard in the features.map callback to return null if !feature ||
!feature.geometry or if feature.geometry.type is missing before accessing .type,
and ensure the internal request to GET (created with new NextRequest and URL in
this block) includes the same authorization/token check used by the outer POST
handler (either validate the token up front and abort if missing, or attach the
token header/query param to the URL/NextRequest) so internal GETs don't silently
fail; update the code that builds params/const url and new NextRequest to reuse
the POST handler's token validation and include token in the internal request.
- Line 31: Validate and cap gridSize after parsing: replace the raw parseInt
usage of gridSizeParam -> gridSize with logic that ensures it's a positive
integer and does not exceed a MAX (e.g., const MAX_GRID_SIZE = 50). Use
Number.isFinite/Number.isInteger on the parsed value and if it's <=0 or NaN
return a 400 error; if it exceeds MAX_GRID_SIZE either clamp it to MAX_GRID_SIZE
or return 400 per API policy (prefer clamping for backward compatibility).
Update references to gridSize in route.ts (where gridSizeParam and gridSize are
used) to rely on this validated/capped value.
- Line 58: Check for a missing Mapbox token immediately after the token is
resolved (the const token = ... line) and return an early 500 response if token
is undefined; update the API handler in route.ts to validate token before making
any per-point Mapbox requests (so functions/blocks that use token for fetches
don’t run), and send a clear error message stating the Mapbox access token is
not configured.
In `@components/map/elevation-heatmap-layer.tsx`:
- Around line 148-174: The effect registers clickHandler, mouseEnterHandler and
mouseLeaveHandler and creates mapboxgl.Popup instances (via new
mapboxgl.Popup()) but never removes those listeners or closes popups, causing
leaks; fix by hoisting/tracking the handlers and popups (e.g., store references
in refs or on a local array) and in the cleanup remove listeners with
map.off(pointsLayerId, 'click', clickHandler) / map.off('mouseenter',
pointsLayerId, mouseEnterHandler) / map.off('mouseleave', pointsLayerId,
mouseLeaveHandler) before removing the layer/source, and call popup.remove() for
each tracked Popup instance so popups are closed on unmount or when points
change.
- Around line 183-189: The cleanup currently removes layers/sources directly and
can throw if the map style is unloaded; update the return cleanup in
elevation-heatmap-layer.tsx to first check map && map.isStyleLoaded() before
calling map.getLayer(pointsLayerId)/getLayer(heatmapLayerId)/getSource(sourceId)
and removing them; ensure you keep the same checks for existence
(map.getLayer(...) / map.getSource(...)) but only run them when
map.isStyleLoaded() returns true to mirror the pattern used in GeoJsonLayer.
- Around line 53-61: The onMapLoad function currently only adds the GeoJSON
source when map.getSource(sourceId) is falsy, so updates to points are ignored;
modify onMapLoad to handle the existing-source path by, when
map.getSource(sourceId) returns a source, calling (map.getSource(sourceId) as
any).setData(geojson) (or obtain the source into a variable and call setData) so
the source's data is updated with the new geojson; keep the existing
map.addSource(sourceId, { type: 'geojson', data: geojson }) branch for the
initial creation and ensure you reference sourceId, map.getSource, onMapLoad,
setData and geojson in the change.
In `@lib/agents/resolution-search.tsx`:
- Around line 32-36: The elevationData schema's bounds currently uses
z.array(z.number()) which permits any length and can lead to undefined values
when downstream code destructures into [west, south, east, north]; update the
bounds definition in the elevationData z.object to require exactly four numbers
by using a tuple of four z.number() entries (i.e., replace the z.array(...) with
a z.tuple([...]) while keeping .optional() and .describe()), so validation
guarantees the four-element bounding box expected by the code that destructures
it.
In `@lib/utils/elevation.ts`:
- Around line 1-2: Remove the unused namespace import "import * as turf from
'@turf/turf';" from this module and keep only the type import "import type {
Position } from 'geojson';", ensuring no references to the turf symbol remain in
elevation.ts; this avoids bundling the whole Turf library unnecessarily and
satisfies the linter/typechecker.
- Around line 27-35: The current bounding-box computation spreads large arrays
into Math.min/Math.max (e.g., Math.min(...lngs), Math.max(...lats)) which can
throw RangeError for very large coordinate lists; replace these spread calls
with safe reductions (e.g., use Array.prototype.reduce or a simple for-loop) to
compute min/max for lngs and lats without spreading, updating the return to use
the reduced values; reference the allCoordinates mapping and variables lngs/lats
(or compute mins/maxs directly from allCoordinates) and ensure the final
returned array remains [west, south, east, north].
- Around line 12-22: The loop in features.forEach accesses feature.geometry.type
without checking for null/undefined, which will throw for features with null
geometry; update the features.forEach callback to guard early (e.g., if
(!feature || !feature.geometry) return/continue) before inspecting
geometry.type, then handle Polygon, LineString, and Point as before; reference
the features.forEach callback and the feature.geometry checks so the fix is
applied where feature.geometry.type is used.
---
Outside diff comments:
In `@app/actions.tsx`:
- Around line 204-215: The message currently serializes the full
elevationPointsData into the AI chat state (see the object built with id:
groupeId, role: 'assistant', content: JSON.stringify({...analysisResult, image:
dataUrl, mapboxImage: mapboxDataUrl, googleImage: googleDataUrl,
elevationPoints: elevationPointsData, type: 'resolution_search_result'}));
instead, stop embedding the full points array—replace elevationPoints with
either a small elevationRequestParams object (e.g., bounds, gridSize,
resolutionSearchId) or a compressed/summarized payload (min/max/mean/std/count
or a base64-compressed blob) and persist that under a new key like
elevationRequest or elevationSummary; update any consumers that read
elevationPoints to re-fetch the detailed points on reload using the stored
request params or to decompress the summary, and ensure the stringified content
uses the new elevationRequest/elevationSummary field instead of
elevationPointsData.
| // 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; | ||
| uiStream.append( | ||
| <ElevationHeatmapLayer | ||
| id={`${groupeId}-elevation`} | ||
| points={elevationData.points} | ||
| statistics={elevationData.statistics} | ||
| /> | ||
| ); | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.error('Error fetching elevation data:', error); | ||
| } | ||
| } |
There was a problem hiding this comment.
Server action fetches its own API route — fragile URL construction and unnecessary HTTP overhead.
This code runs inside a 'use server' action, yet makes an HTTP request to its own /api/elevation endpoint. This has several problems:
- URL fragility:
process.env.NEXT_PUBLIC_BASE_URL || 'http://localhost:3000'will break in production if the env var is missing or misconfigured (e.g., behind a load balancer, in a containerized deployment, or on Vercel where the URL varies per preview deployment). - Missing URL encoding:
JSON.stringify(...)output forboundsandgeometrycontains characters like[,{,"that must be percent-encoded. WithoutencodeURIComponentorURLSearchParams, the URL is malformed. - Unnecessary round-trip: The server is calling itself over HTTP. Import the elevation logic directly to avoid network overhead, URL issues, and the need to know the server's own address.
🐛 Proposed fix — use URLSearchParams at minimum
If refactoring to call the logic directly is too large a change, at least fix the URL encoding:
- 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)}`
- : ''
- }`
- );
+ const params = new URLSearchParams({
+ bounds: JSON.stringify(analysisResult.elevationData.bounds),
+ gridSize: String(analysisResult.elevationData.gridSize || 20),
+ });
+ if (drawnFeatures.length > 0 && drawnFeatures[0].geometry) {
+ params.set('geometry', JSON.stringify(drawnFeatures[0].geometry));
+ }
+ const elevationResponse = await fetch(
+ `${process.env.NEXT_PUBLIC_BASE_URL || 'http://localhost:3000'}/api/elevation?${params}`
+ );Ideally, extract the elevation-fetching logic from app/api/elevation/route.ts into a shared function and call it directly here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/actions.tsx` around lines 129 - 159, This server action should not call
its own /api/elevation over HTTP; extract the elevation logic into a shared
function (e.g., getElevationData) inside the same module as
app/api/elevation/route.ts and import it into app/actions.tsx, then replace the
fetch block that builds
`${process.env.NEXT_PUBLIC_BASE_URL...}/api/elevation?...` with a direct call
like await getElevationData({bounds: analysisResult.elevationData.bounds,
gridSize: analysisResult.elevationData.gridSize, geometry:
drawnFeatures[0]?.geometry}); assign the returned object to elevationPointsData
and keep the existing uiStream.append(<ElevationHeatmapLayer
id={`${groupeId}-elevation`} points={...} statistics={...} />) logic; if
extracting the shared function is infeasible right now, at minimum build the
query using URLSearchParams or encodeURIComponent for bounds and geometry to
avoid malformed URLs before calling fetch.
| const gridSize = gridSizeParam ? parseInt(gridSizeParam) : 20; | ||
| const geometry = geometryParam ? JSON.parse(geometryParam) : null; |
There was a problem hiding this comment.
Missing try/catch around JSON.parse(geometryParam) — will crash with an unhandled 500.
boundsParam parsing is correctly wrapped in a try/catch (Lines 25-29), but geometryParam parsing on Line 32 is not. A malformed geometry query string will throw and be caught by the outer catch, returning a generic 500 instead of a descriptive 400.
🛡️ Proposed fix
- const geometry = geometryParam ? JSON.parse(geometryParam) : null;
+ let geometry = null;
+ if (geometryParam) {
+ try {
+ geometry = JSON.parse(geometryParam);
+ } catch (e) {
+ return NextResponse.json({ error: 'Invalid geometry parameter' }, { status: 400 });
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const gridSize = gridSizeParam ? parseInt(gridSizeParam) : 20; | |
| const geometry = geometryParam ? JSON.parse(geometryParam) : null; | |
| const gridSize = gridSizeParam ? parseInt(gridSizeParam) : 20; | |
| let geometry = null; | |
| if (geometryParam) { | |
| try { | |
| geometry = JSON.parse(geometryParam); | |
| } catch (e) { | |
| return NextResponse.json({ error: 'Invalid geometry parameter' }, { status: 400 }); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/elevation/route.ts` around lines 31 - 32, Wrap the JSON.parse call
for geometryParam in a try/catch (same pattern as boundsParam) so a malformed
geometry query returns a 400 with a descriptive error instead of throwing a 500;
specifically, update the logic around geometryParam and the geometry variable in
route handler (where geometryParam is parsed into geometry) to catch SyntaxError
from JSON.parse and call return new NextResponse("Invalid geometry parameter", {
status: 400 }) (or the project’s existing bad-request response helper).
| return NextResponse.json({ error: 'Invalid bounds parameter' }, { status: 400 }); | ||
| } | ||
|
|
||
| const gridSize = gridSizeParam ? parseInt(gridSizeParam) : 20; |
There was a problem hiding this comment.
No upper-bound validation on gridSize — potential DoS vector.
A caller can pass gridSize=500, generating 501² = 251,001 Mapbox API calls from a single request. Add a reasonable cap (e.g., 50) and validate that gridSize is a positive integer.
🛡️ Proposed fix
- const gridSize = gridSizeParam ? parseInt(gridSizeParam) : 20;
+ const gridSize = Math.min(Math.max(gridSizeParam ? parseInt(gridSizeParam, 10) : 20, 1), 50);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const gridSize = gridSizeParam ? parseInt(gridSizeParam) : 20; | |
| const gridSize = Math.min(Math.max(gridSizeParam ? parseInt(gridSizeParam, 10) : 20, 1), 50); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/elevation/route.ts` at line 31, Validate and cap gridSize after
parsing: replace the raw parseInt usage of gridSizeParam -> gridSize with logic
that ensures it's a positive integer and does not exceed a MAX (e.g., const
MAX_GRID_SIZE = 50). Use Number.isFinite/Number.isInteger on the parsed value
and if it's <=0 or NaN return a 400 error; if it exceeds MAX_GRID_SIZE either
clamp it to MAX_GRID_SIZE or return 400 per API policy (prefer clamping for
backward compatibility). Update references to gridSize in route.ts (where
gridSizeParam and gridSize are used) to rely on this validated/capped value.
| 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 }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Unbounded concurrent HTTP requests to Mapbox will hit rate limits and cause failures.
With the default gridSize=20, the grid generates (20+1)² = 441 points, each firing a separate fetch to the Mapbox Tilequery API via Promise.all (Line 88) with zero concurrency control. The Mapbox Tilequery API has rate limits (typically 600 requests/minute on standard plans). A single call to this endpoint nearly exhausts that budget, and concurrent users will easily exceed it — leading to 429s, dropped data points, and degraded UX.
🐛 Proposed fix — batch with concurrency limit
Use a concurrency-limited approach (e.g., a simple chunked Promise.all or a library like p-limit):
+import pLimit from 'p-limit';
+const limit = pLimit(10); // max 10 concurrent requests
-const elevationPromises = points.map(async (point) => {
+const elevationPromises = points.map((point) => limit(async () => {
try {
const response = await fetch(
`https://api.mapbox.com/v4/mapbox.mapbox-terrain-v2/tilequery/${point.lng},${point.lat}.json?access_token=${token}`
);
// ... rest unchanged
} catch (error) {
console.error(`Error fetching elevation for ${point.lng},${point.lat}:`, error);
}
return point;
-});
+}));Alternatively, consider using the Mapbox Terrain-RGB raster tiles directly (fewer requests by fetching tile images and decoding pixel values) instead of one tilequery per point.
Also applies to: 62-86, 88-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/elevation/route.ts` around lines 42 - 56, The code currently builds a
large points array in route.ts (points from the nested loops that push { lng,
lat, elevation: null }) and then fires unbounded concurrent fetches to the
Mapbox Tilequery API (the Promise.all at the fetch stage), which will hit rate
limits; update the fetch phase to use a concurrency-limited strategy (e.g.,
process points in chunks or use a concurrency utility like p-limit) so only N
requests run concurrently (choose a safe default like 5–10), and replace the
direct Promise.all over all points with batched Promise.all calls or a p-limit
wrapped fetch; ensure you reference the points array and the function or inline
block that performs the Tilequery fetch to apply the concurrency control and
handle/retry 429 responses gracefully.
| } | ||
| } | ||
|
|
||
| const token = process.env.MAPBOX_ACCESS_TOKEN || process.env.NEXT_PUBLIC_MAPBOX_ACCESS_TOKEN; |
There was a problem hiding this comment.
Mapbox access token fallback and missing-token check.
If neither MAPBOX_ACCESS_TOKEN nor NEXT_PUBLIC_MAPBOX_ACCESS_TOKEN is set, token will be undefined, and all Mapbox requests will fail silently (caught by the per-point catch). Return an early 500 if the token is missing to surface the misconfiguration clearly.
🛡️ Proposed fix
const token = process.env.MAPBOX_ACCESS_TOKEN || process.env.NEXT_PUBLIC_MAPBOX_ACCESS_TOKEN;
+ if (!token) {
+ return NextResponse.json({ error: 'Mapbox access token not configured' }, { status: 500 });
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const token = process.env.MAPBOX_ACCESS_TOKEN || process.env.NEXT_PUBLIC_MAPBOX_ACCESS_TOKEN; | |
| const token = process.env.MAPBOX_ACCESS_TOKEN || process.env.NEXT_PUBLIC_MAPBOX_ACCESS_TOKEN; | |
| if (!token) { | |
| return NextResponse.json({ error: 'Mapbox access token not configured' }, { status: 500 }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/elevation/route.ts` at line 58, Check for a missing Mapbox token
immediately after the token is resolved (the const token = ... line) and return
an early 500 response if token is undefined; update the API handler in route.ts
to validate token before making any per-point Mapbox requests (so
functions/blocks that use token for fetches don’t run), and send a clear error
message stating the Mapbox access token is not configured.
| 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.
Cleanup lacks isStyleLoaded() guard — may throw if map style is unloaded.
The existing GeoJsonLayer component (see components/map/geojson-layer.tsx, Lines 90-97) wraps removeLayer/removeSource in an if (map.isStyleLoaded()) check. Without this guard, calling removeLayer/removeSource on an already-disposed map or during a style change can throw.
🛡️ Proposed fix
return () => {
- if (map) {
+ if (map && map.isStyleLoaded()) {
if (map.getLayer(pointsLayerId)) map.removeLayer(pointsLayerId)
if (map.getLayer(heatmapLayerId)) map.removeLayer(heatmapLayerId)
if (map.getSource(sourceId)) map.removeSource(sourceId)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return () => { | |
| if (map) { | |
| if (map.getLayer(pointsLayerId)) map.removeLayer(pointsLayerId) | |
| if (map.getLayer(heatmapLayerId)) map.removeLayer(heatmapLayerId) | |
| if (map.getSource(sourceId)) map.removeSource(sourceId) | |
| } | |
| } | |
| return () => { | |
| if (map && map.isStyleLoaded()) { | |
| if (map.getLayer(pointsLayerId)) map.removeLayer(pointsLayerId) | |
| if (map.getLayer(heatmapLayerId)) map.removeLayer(heatmapLayerId) | |
| if (map.getSource(sourceId)) map.removeSource(sourceId) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/map/elevation-heatmap-layer.tsx` around lines 183 - 189, The
cleanup currently removes layers/sources directly and can throw if the map style
is unloaded; update the return cleanup in elevation-heatmap-layer.tsx to first
check map && map.isStyleLoaded() before calling
map.getLayer(pointsLayerId)/getLayer(heatmapLayerId)/getSource(sourceId) and
removing them; ensure you keep the same checks for existence (map.getLayer(...)
/ map.getSource(...)) but only run them when map.isStyleLoaded() returns true to
mirror the pattern used in GeoJsonLayer.
| elevationData: z.object({ | ||
| requested: z.boolean().describe('Whether elevation heat map was requested or appropriate for this analysis'), | ||
| bounds: z.array(z.number()).optional().describe('Bounding box for elevation query [west, south, east, north]'), | ||
| gridSize: z.number().optional().describe('Grid resolution for elevation sampling (default: 20)'), | ||
| }).optional().describe('Elevation heat map configuration for the analyzed area') |
There was a problem hiding this comment.
bounds schema lacks length constraint — should enforce exactly 4 elements.
z.array(z.number()) allows the AI to return bounds of any length. Since downstream code destructures it as [west, south, east, north] (see app/api/elevation/route.ts Line 34), an incorrect length will produce undefined values and silent bugs.
🛡️ Proposed fix — use a tuple
- bounds: z.array(z.number()).optional().describe('Bounding box for elevation query [west, south, east, north]'),
+ bounds: z.tuple([z.number(), z.number(), z.number(), z.number()]).optional().describe('Bounding box for elevation query [west, south, east, north]'),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/agents/resolution-search.tsx` around lines 32 - 36, The elevationData
schema's bounds currently uses z.array(z.number()) which permits any length and
can lead to undefined values when downstream code destructures into [west,
south, east, north]; update the bounds definition in the elevationData z.object
to require exactly four numbers by using a tuple of four z.number() entries
(i.e., replace the z.array(...) with a z.tuple([...]) while keeping .optional()
and .describe()), so validation guarantees the four-element bounding box
expected by the code that destructures it.
| import * as turf from '@turf/turf'; | ||
| import type { Position } from 'geojson'; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unused turf import bloats the client bundle.
@turf/turf is imported but never used in this file. Since @turf/turf re-exports the entire Turf.js library, this adds significant dead weight. The Position type import from geojson is sufficient here.
♻️ Remove unused import
-import * as turf from '@turf/turf';
import type { Position } from 'geojson';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import * as turf from '@turf/turf'; | |
| import type { Position } from 'geojson'; | |
| import type { Position } from 'geojson'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/utils/elevation.ts` around lines 1 - 2, Remove the unused namespace
import "import * as turf from '@turf/turf';" from this module and keep only the
type import "import type { Position } from 'geojson';", ensuring no references
to the turf symbol remain in elevation.ts; this avoids bundling the whole Turf
library unnecessarily and satisfies the linter/typechecker.
| features.forEach(feature => { | ||
| if (feature.geometry.type === 'Polygon') { | ||
| // Handle MultiPolygon if necessary, but standard Draw is Polygon | ||
| feature.geometry.coordinates.forEach((ring: Position[]) => { | ||
| allCoordinates.push(...ring); | ||
| }); | ||
| } else if (feature.geometry.type === 'LineString') { | ||
| allCoordinates.push(...feature.geometry.coordinates); | ||
| } else if (feature.geometry.type === 'Point') { | ||
| allCoordinates.push(feature.geometry.coordinates); | ||
| } |
There was a problem hiding this comment.
Missing null-check on feature.geometry before accessing .type.
If a feature has a null or undefined geometry (which GeoJSON allows), accessing feature.geometry.type on Line 13 will throw a TypeError at runtime.
🛡️ Proposed fix
features.forEach(feature => {
+ if (!feature.geometry) return;
if (feature.geometry.type === 'Polygon') {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/utils/elevation.ts` around lines 12 - 22, The loop in features.forEach
accesses feature.geometry.type without checking for null/undefined, which will
throw for features with null geometry; update the features.forEach callback to
guard early (e.g., if (!feature || !feature.geometry) return/continue) before
inspecting geometry.type, then handle Polygon, LineString, and Point as before;
reference the features.forEach callback and the feature.geometry checks so the
fix is applied where feature.geometry.type is used.
| const lngs = allCoordinates.map(coord => coord[0]); | ||
| const lats = allCoordinates.map(coord => coord[1]); | ||
|
|
||
| return [ | ||
| Math.min(...lngs), // west | ||
| Math.min(...lats), // south | ||
| Math.max(...lngs), // east | ||
| Math.max(...lats), // north | ||
| ]; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Math.min(...lngs) / Math.max(...) with spread can throw RangeError on very large coordinate arrays.
For features with many coordinates, the spread into Math.min/Math.max exceeds the JS engine's call stack argument limit. In practice, drawn features are small, so this is a low-probability edge case — but it's worth noting for robustness.
♻️ Safe alternative using reduce
- const lngs = allCoordinates.map(coord => coord[0]);
- const lats = allCoordinates.map(coord => coord[1]);
-
- return [
- Math.min(...lngs), // west
- Math.min(...lats), // south
- Math.max(...lngs), // east
- Math.max(...lats), // north
- ];
+ let west = Infinity, south = Infinity, east = -Infinity, north = -Infinity;
+ for (const coord of allCoordinates) {
+ if (coord[0] < west) west = coord[0];
+ if (coord[1] < south) south = coord[1];
+ if (coord[0] > east) east = coord[0];
+ if (coord[1] > north) north = coord[1];
+ }
+ return [west, south, east, north];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const lngs = allCoordinates.map(coord => coord[0]); | |
| const lats = allCoordinates.map(coord => coord[1]); | |
| return [ | |
| Math.min(...lngs), // west | |
| Math.min(...lats), // south | |
| Math.max(...lngs), // east | |
| Math.max(...lats), // north | |
| ]; | |
| let west = Infinity, south = Infinity, east = -Infinity, north = -Infinity; | |
| for (const coord of allCoordinates) { | |
| if (coord[0] < west) west = coord[0]; | |
| if (coord[1] < south) south = coord[1]; | |
| if (coord[0] > east) east = coord[0]; | |
| if (coord[1] > north) north = coord[1]; | |
| } | |
| return [west, south, east, north]; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/utils/elevation.ts` around lines 27 - 35, The current bounding-box
computation spreads large arrays into Math.min/Math.max (e.g.,
Math.min(...lngs), Math.max(...lats)) which can throw RangeError for very large
coordinate lists; replace these spread calls with safe reductions (e.g., use
Array.prototype.reduce or a simple for-loop) to compute min/max for lngs and
lats without spreading, updating the return to use the reduced values; reference
the allCoordinates mapping and variables lngs/lats (or compute mins/maxs
directly from allCoordinates) and ensure the final returned array remains [west,
south, east, north].
- Moved map layers into MapResultsContainer with next/dynamic (ssr: false) - Refactored ElevationHeatmapLayer to use dynamic mapbox-gl import to avoid server-side errors - Fixed 'ssr: false' not allowed in Server Components build error - Updated app/actions.tsx to use the new SSR-safe container - Verified API endpoint and utility functions Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
app/actions.tsxapp_actions_final_fix.patchcomponents/map/elevation-heatmap-layer.tsxcomponents/map/map-results-container.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-02-08T16:50:33.791Z
Learnt from: ngoiyaeric
Repo: QueueLab/QCX PR: 0
File: :0-0
Timestamp: 2026-02-08T16:50:33.791Z
Learning: The PR branch implements RAG (Retrieval-Augmented Generation) via `lib/actions/rag.ts` which uses Supabase RPC functions `generate_embedding()` and `hybrid_search()` to retrieve relevant context from past conversations. However, there's a bug on line 433 of `app/actions.tsx` where `currentSystemPrompt` is passed to the researcher instead of `augmentedSystemPrompt`, preventing the retrieved context from being used.
Applied to files:
app/actions.tsx
🧬 Code graph analysis (3)
components/map/map-results-container.tsx (2)
components/map/geojson-layer.tsx (4)
GeoJsonLayer(13-100)map(89-96)map(24-80)GeoJsonLayerProps(8-11)components/map/elevation-heatmap-layer.tsx (1)
ElevationHeatmapLayer(23-143)
components/map/elevation-heatmap-layer.tsx (1)
components/map/map-context.tsx (1)
useMap(24-30)
app/actions.tsx (3)
components/map/map-results-container.tsx (1)
MapResultsContainer(19-32)lib/types/index.ts (2)
Chat(49-57)AIMessage(59-77)components/map/geojson-layer.tsx (2)
map(16-97)GeoJsonLayer(13-100)
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app_actions_final_fix.patch`:
- Around line 1-9: This file is a leftover SEARCH/REPLACE patch (contains
conflict markers and duplicate imports for ElevationHeatmapLayer and
ResolutionCarousel); delete the file app_actions_final_fix.patch from the
commit/repo (or revert the commit that added it) so the repository no longer
contains the development artifact, and verify no other files contain the same
SEARCH/REPLACE/<<<<<<< markers referencing ElevationHeatmapLayer or
ResolutionCarousel.
In `@app/actions.tsx`:
- Around line 559-565: The 'related' case currently returns isCollapsed: true (a
raw boolean) which mismatches UIState.isCollapsed?: StreamableValue<boolean>;
update the 'related' branch to wrap the boolean in a StreamableValue by calling
createStreamableValue(true) so consumers can call
useStreamableValue(item.isCollapsed) safely; locate the switch case handling
'related' (where relatedQueries is parsed and SearchRelated is returned) and
replace the plain boolean assignment with isCollapsed:
createStreamableValue(true).
- Around line 459-461: The RAG-augmented prompt returned by the RAG pipeline is
never used — replace the direct use of currentSystemPrompt when calling
researcher with the augmented prompt produced by the RAG flow: call the RAG
helper (the logic in lib/actions/rag.ts that uses generate_embedding and
hybrid_search) to produce augmentedSystemPrompt (or similar), await its result,
merge or embed retrieved conversational context into the system prompt, and pass
augmentedSystemPrompt (not currentSystemPrompt) into researcher(...) so
researcher receives the RAG-augmented prompt and toolResponses/fullResponse
reflect retrieved context.
- Around line 553-605: The switch cases for 'related', 'inquiry', and
'resolution_search_result' call JSON.parse(content as string) unguarded and can
throw during chat restore; wrap each parse in a try-catch (same pattern used in
the 'tool' case) to catch JSON errors, log/handle the error, and return a safe
fallback UI instead of letting getUIStateFromAIState throw. Specifically, update
the 'related' case before creating SearchRelated(relatedQueries), the 'inquiry'
case before rendering <CopilotDisplay inquiry=...>, and the
'resolution_search_result' case before extracting
analysisResult.geoJson/image/mapboxImage/googleImage/elevationPoints for
ResolutionCarousel and MapResultsContainer so malformed content yields a logged
error and a benign component (e.g., a simple BotMessage or collapsed
placeholder) rather than crashing.
In `@components/map/elevation-heatmap-layer.tsx`:
- Around line 127-139: The cleanup currently removes layers/sources but does not
deregister the pending listener added with map.once('load', onMapLoad'), which
can cause orphaned layers if the effect unmounts before the load event; update
the effect cleanup to call map.off('load', onMapLoad) (or cancel the once
listener) to remove the queued handler and also guard removals with
map.isStyleLoaded() before removing layers/sources; locate the listener
registration and onMapLoad function in elevation-heatmap-layer.tsx and add the
deregistration plus the isStyleLoaded() check in the returned cleanup block.
In `@components/map/map-results-container.tsx`:
- Around line 10-17: MapResultsContainerProps currently uses unsafe any types
for elevationPoints; export the concrete types from
components/map/elevation-heatmap-layer.tsx (the point interface and the
statistics interface — e.g. ElevationPoint and ElevationStatistics) and import
them into components/map/map-results-container.tsx, then change elevationPoints
to { points: ElevationPoint[]; statistics: ElevationStatistics } and update any
usages accordingly (look for elevationPoints.points and
elevationPoints.statistics and adjust imports and type annotations in
MapResultsContainerProps).
---
Duplicate comments:
In `@app/actions.tsx`:
- Around line 120-142: This server action builds a malformed self-URL by
embedding raw JSON from analysisResult.elevationData.bounds and
drawnFeatures[0].geometry into the query string; instead, percent-encode those
query values using encodeURIComponent(JSON.stringify(...)) or — better — move
the elevation-fetching logic into a shared function (e.g., getElevationPoints or
fetchElevationData) that accepts (bounds, gridSize, geometry) and returns the
same elevationPointsData shape, call that function directly from this server
action (replace the internal fetch block that sets elevationPointsData) and have
the /api/elevation handler delegate to the same shared function so both the API
route and the server action reuse the logic and avoid fragile self-HTTP calls.
In `@components/map/elevation-heatmap-layer.tsx`:
- Around line 57-62: The onMapLoad handler currently only adds the GeoJSON
source when it doesn't exist, causing updates to points to be ignored; modify
onMapLoad to handle the existing source by getting it via
map.getSource(sourceId) and calling source.setData(geojson) in an else branch so
the source's data is updated when `points` changes (mirror the GeoJsonLayer
pattern that updates data in lines handling `source.setData(geojson)`). Ensure
you reference the same sourceId, geojson variable, and call setData on the
retrieved source object.
- Around line 110-123: The event handlers for the points layer leak because you
register clickHandler and anonymous mouseenter/mouseleave functions with map.on
but never remove them; change the anonymous mouseenter and mouseleave listeners
to named functions (e.g., onPointMouseEnter, onPointMouseLeave), keep the
existing clickHandler reference, and in the component's cleanup/unmount routine
call map.off(pointsLayerId, 'click', clickHandler), map.off(pointsLayerId,
'mouseenter', onPointMouseEnter) and map.off(pointsLayerId, 'mouseleave',
onPointMouseLeave) to deregister all three handlers; ensure you reference the
same function objects used when calling map.on so removal succeeds.
| <<<<<<< SEARCH | ||
| import dynamic from 'next/dynamic' | ||
| import { ResolutionCarousel } from '@/components/resolution-carousel' | ||
|
|
||
| const ElevationHeatmapLayer = dynamic(() => import('@/components/map/elevation-heatmap-layer').then(mod => mod.ElevationHeatmapLayer), { ssr: false }) | ||
| ======= | ||
| import { ElevationHeatmapLayer } from '@/components/map/elevation-heatmap-layer' | ||
| import { ResolutionCarousel } from '@/components/resolution-carousel' | ||
| >>>>>>> REPLACE |
There was a problem hiding this comment.
Development artifact committed to repository — should be removed.
app_actions_final_fix.patch is an aider-style SEARCH/REPLACE patch file generated during development. It's already been applied (the final app/actions.tsx imports MapResultsContainer rather than ElevationHeatmapLayer directly), so this file serves no further purpose and adds noise to the repository.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app_actions_final_fix.patch` around lines 1 - 9, This file is a leftover
SEARCH/REPLACE patch (contains conflict markers and duplicate imports for
ElevationHeatmapLayer and ResolutionCarousel); delete the file
app_actions_final_fix.patch from the commit/repo (or revert the commit that
added it) so the repository no longer contains the development artifact, and
verify no other files contain the same SEARCH/REPLACE/<<<<<<< markers
referencing ElevationHeatmapLayer or ResolutionCarousel.
| } else { | ||
| const { fullResponse, hasError, toolResponses } = await researcher( | ||
| currentSystemPrompt, |
There was a problem hiding this comment.
RAG-augmented system prompt is never computed — retrieved context is discarded before calling researcher.
Line 430 fetches currentSystemPrompt from getSystemPrompt, but the augmentedSystemPrompt produced by the RAG pipeline (lib/actions/rag.ts via generate_embedding + hybrid_search) is never invoked. researcher on line 460 therefore receives the bare system prompt, and all retrieved conversational context is unused. Based on learnings, this is a known bug in this codebase — currentSystemPrompt is passed to the researcher instead of the RAG-augmented version.
🐛 Outline fix — augment prompt before calling researcher
const currentSystemPrompt = (await getSystemPrompt(userId)) || ''
+ // Augment with RAG context
+ const augmentedSystemPrompt = await augmentSystemPromptWithRAG(currentSystemPrompt, messages)
const mapProvider = formData?.get('mapProvider') as 'mapbox' | 'google'
// ...
const { fullResponse, hasError, toolResponses } = await researcher(
- currentSystemPrompt,
+ augmentedSystemPrompt,Based on learnings from a prior review of this repository: the currentSystemPrompt is passed to researcher instead of the RAG-augmented system prompt, preventing retrieved conversational context from reaching the model.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/actions.tsx` around lines 459 - 461, The RAG-augmented prompt returned by
the RAG pipeline is never used — replace the direct use of currentSystemPrompt
when calling researcher with the augmented prompt produced by the RAG flow: call
the RAG helper (the logic in lib/actions/rag.ts that uses generate_embedding and
hybrid_search) to produce augmentedSystemPrompt (or similar), await its result,
merge or embed retrieved conversational context into the system prompt, and pass
augmentedSystemPrompt (not currentSystemPrompt) into researcher(...) so
researcher receives the RAG-augmented prompt and toolResponses/fullResponse
reflect retrieved context.
| switch (type) { | ||
| case 'response': | ||
| return { | ||
| id, | ||
| component: <BotMessage content={content as string} /> | ||
| } | ||
| break | ||
| case 'assistant': | ||
| const answer = createStreamableValue(content as string) | ||
| answer.done(content as string) | ||
| switch (type) { | ||
| case 'response': | ||
| return { | ||
| id, | ||
| component: ( | ||
| <Section title="response"> | ||
| <BotMessage content={answer.value} /> | ||
| </Section> | ||
| ) | ||
| } | ||
| case 'related': | ||
| const relatedQueries = createStreamableValue<RelatedQueries>({ | ||
| items: [] | ||
| }) | ||
| relatedQueries.done(JSON.parse(content as string)) | ||
| return { | ||
| id, | ||
| component: ( | ||
| <Section title="Related" separator={true}> | ||
| <SearchRelated relatedQueries={relatedQueries.value} /> | ||
| </Section> | ||
| ) | ||
| } | ||
| case 'followup': | ||
| return { | ||
| id, | ||
| component: ( | ||
| <Section title="Follow-up" className="pb-8"> | ||
| <FollowupPanel /> | ||
| </Section> | ||
| ) | ||
| } | ||
| case 'resolution_search_result': { | ||
| const analysisResult = JSON.parse(content as string); | ||
| const geoJson = analysisResult.geoJson as FeatureCollection; | ||
| const image = analysisResult.image as string; | ||
| const mapboxImage = analysisResult.mapboxImage as string; | ||
| const googleImage = analysisResult.googleImage as string; | ||
| case 'related': | ||
| const relatedQueries = JSON.parse(content as string) | ||
| return { | ||
| id, | ||
| component: <SearchRelated queries={relatedQueries} />, | ||
| isCollapsed: true | ||
| } | ||
| case 'followup': | ||
| return { | ||
| id, | ||
| component: ( | ||
| <Section title="Follow-up"> | ||
| <FollowupPanel /> | ||
| </Section> | ||
| ) | ||
| } | ||
| case 'inquiry': | ||
| return { | ||
| id, | ||
| component: <CopilotDisplay inquiry={JSON.parse(content as string)} /> | ||
| } | ||
| case 'resolution_search_result': { | ||
| const analysisResult = JSON.parse(content as string); | ||
| const geoJson = analysisResult.geoJson as FeatureCollection; | ||
| const image = analysisResult.image as string; | ||
| const mapboxImage = analysisResult.mapboxImage as string; | ||
| const googleImage = analysisResult.googleImage as string; | ||
| const elevationPoints = analysisResult.elevationPoints; | ||
|
|
||
| return { | ||
| id, | ||
| component: ( | ||
| <> | ||
| <ResolutionCarousel | ||
| mapboxImage={mapboxImage} | ||
| googleImage={googleImage} | ||
| initialImage={image} | ||
| /> | ||
| {geoJson && ( | ||
| <GeoJsonLayer id={id} data={geoJson} /> | ||
| )} | ||
| </> | ||
| ) | ||
| } | ||
| } | ||
| return { | ||
| id, | ||
| component: ( | ||
| <> | ||
| <ResolutionCarousel | ||
| mapboxImage={mapboxImage} | ||
| googleImage={googleImage} | ||
| initialImage={image} | ||
| /> | ||
| <MapResultsContainer | ||
| id={id} | ||
| geoJson={geoJson} | ||
| elevationPoints={elevationPoints} | ||
| /> | ||
| </> | ||
| ) | ||
| } | ||
| break | ||
| } |
There was a problem hiding this comment.
JSON.parse throws unguarded in 'related', 'inquiry', and 'resolution_search_result' cases — breaks entire chat restore on corrupted state.
The 'tool' case (line 607) wraps parsing in try-catch, but the other three message types call JSON.parse(content as string) directly. A single malformed persisted message causes getUIStateFromAIState to throw, failing all subsequent message rendering in the chat.
🐛 Proposed fix — wrap all parsing cases consistently
- case 'related':
- const relatedQueries = JSON.parse(content as string)
- return {
- id,
- component: <SearchRelated queries={relatedQueries} />,
- isCollapsed: true
- }
+ case 'related': {
+ try {
+ const relatedQueries = JSON.parse(content as string)
+ const isCollapsed = createStreamableValue(true)
+ isCollapsed.done(true)
+ return {
+ id,
+ component: <SearchRelated queries={relatedQueries} />,
+ isCollapsed: isCollapsed.value
+ }
+ } catch {
+ return { id, component: null }
+ }
+ }
case 'inquiry':
- return {
- id,
- component: <CopilotDisplay inquiry={JSON.parse(content as string)} />
- }
+ try {
+ return {
+ id,
+ component: <CopilotDisplay inquiry={JSON.parse(content as string)} />
+ }
+ } catch {
+ return { id, component: null }
+ }
case 'resolution_search_result': {
- const analysisResult = JSON.parse(content as string);
+ try {
+ const analysisResult = JSON.parse(content as string)
// ... existing rendering ...
+ } catch {
+ return { id, component: null }
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/actions.tsx` around lines 553 - 605, The switch cases for 'related',
'inquiry', and 'resolution_search_result' call JSON.parse(content as string)
unguarded and can throw during chat restore; wrap each parse in a try-catch
(same pattern used in the 'tool' case) to catch JSON errors, log/handle the
error, and return a safe fallback UI instead of letting getUIStateFromAIState
throw. Specifically, update the 'related' case before creating
SearchRelated(relatedQueries), the 'inquiry' case before rendering
<CopilotDisplay inquiry=...>, and the 'resolution_search_result' case before
extracting analysisResult.geoJson/image/mapboxImage/googleImage/elevationPoints
for ResolutionCarousel and MapResultsContainer so malformed content yields a
logged error and a benign component (e.g., a simple BotMessage or collapsed
placeholder) rather than crashing.
| case 'related': | ||
| const relatedQueries = JSON.parse(content as string) | ||
| return { | ||
| id, | ||
| component: <SearchRelated queries={relatedQueries} />, | ||
| isCollapsed: true | ||
| } |
There was a problem hiding this comment.
isCollapsed: true (plain boolean) is a type mismatch — UIState expects StreamableValue<boolean>.
Line 564 returns isCollapsed: true while the UIState type (line 691) declares isCollapsed?: StreamableValue<boolean>. The 'tool' case (lines 609–610) correctly creates and returns a StreamableValue. If the consumer calls useStreamableValue(item.isCollapsed) on a plain boolean, the collapse behavior for related-query results will not work as intended.
The diff above (in the preceding comment) also fixes this by using createStreamableValue(true).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/actions.tsx` around lines 559 - 565, The 'related' case currently returns
isCollapsed: true (a raw boolean) which mismatches UIState.isCollapsed?:
StreamableValue<boolean>; update the 'related' branch to wrap the boolean in a
StreamableValue by calling createStreamableValue(true) so consumers can call
useStreamableValue(item.isCollapsed) safely; locate the switch case handling
'related' (where relatedQueries is parsed and SearchRelated is returned) and
replace the plain boolean assignment with isCollapsed:
createStreamableValue(true).
| 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.
map.once('load', onMapLoad) is never deregistered — orphaned layers on early unmount.
When map.isStyleLoaded() is false, onMapLoad is queued via map.once('load', ...) (line 130). If the component unmounts (or the effect re-runs) before the load event fires, the cleanup on lines 133–139 executes with no layers/source to remove, then later onMapLoad fires and adds source + layers that are never cleaned up because no further cleanup will run.
Fix: cancel the pending listener at the top of cleanup, and add the missing isStyleLoaded() guard (previously flagged) in the same pass.
🐛 Proposed fix
return () => {
- if (map) {
+ map.off('load', onMapLoad) // cancel pending once-listener before it fires
+ if (map && map.isStyleLoaded()) {
if (map.getLayer(pointsLayerId)) map.removeLayer(pointsLayerId)
if (map.getLayer(heatmapLayerId)) map.removeLayer(heatmapLayerId)
if (map.getSource(sourceId)) map.removeSource(sourceId)
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/map/elevation-heatmap-layer.tsx` around lines 127 - 139, The
cleanup currently removes layers/sources but does not deregister the pending
listener added with map.once('load', onMapLoad'), which can cause orphaned
layers if the effect unmounts before the load event; update the effect cleanup
to call map.off('load', onMapLoad) (or cancel the once listener) to remove the
queued handler and also guard removals with map.isStyleLoaded() before removing
layers/sources; locate the listener registration and onMapLoad function in
elevation-heatmap-layer.tsx and add the deregistration plus the isStyleLoaded()
check in the returned cleanup block.
| interface MapResultsContainerProps { | ||
| id: string | ||
| geoJson?: FeatureCollection | ||
| elevationPoints?: { | ||
| points: any[] | ||
| statistics: any | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer typed props over any[] / any for elevationPoints.
points: any[] and statistics: any bypass type checking. The ElevationPoint interface and the statistics shape are already defined in elevation-heatmap-layer.tsx — export them and reference them here.
♻️ Proposed refactor
In components/map/elevation-heatmap-layer.tsx:
+export interface ElevationPoint {
lng: number;
lat: number;
elevation: number;
}
+export interface ElevationStatistics {
+ min: number;
+ max: number;
+ average: number;
+ count: number;
+}In components/map/map-results-container.tsx:
+import type { ElevationPoint, ElevationStatistics } from './elevation-heatmap-layer'
interface MapResultsContainerProps {
id: string
geoJson?: FeatureCollection
elevationPoints?: {
- points: any[]
- statistics: any
+ points: ElevationPoint[]
+ statistics: ElevationStatistics
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/map/map-results-container.tsx` around lines 10 - 17,
MapResultsContainerProps currently uses unsafe any types for elevationPoints;
export the concrete types from components/map/elevation-heatmap-layer.tsx (the
point interface and the statistics interface — e.g. ElevationPoint and
ElevationStatistics) and import them into
components/map/map-results-container.tsx, then change elevationPoints to {
points: ElevationPoint[]; statistics: ElevationStatistics } and update any
usages accordingly (look for elevationPoints.points and
elevationPoints.statistics and adjust imports and type annotations in
MapResultsContainerProps).
- Explicitly typed createAI with AIState and UIState to prevent 'never' array inference - Moved AIState and UIState type definitions to the top of the file - Added missing React import for React.ReactNode - Fixed type mismatch in aiState.update calls Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
- Fixed incorrect writer() call arguments in regular search flow - Restored streamText createStreamableValue for researcher and writer coordination - Ensured all agents in app/actions.tsx are called with correct signatures - Maintained SSR safety and type safety for map layers Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/actions.tsx (2)
213-219: 🧹 Nitpick | 🔵 Trivial
resolution_search_resultmessage stores large blobs — consider storing only references.Each persisted
resolution_search_resultmessage now serializes the full base64 image (dataUrl),mapboxImage,googleImage, and the elevation points array (up togridSize²point objects) into a single DB column. At the defaultgridSizeof 20 this adds ~400 elevation records on top of the existing image payloads per chat turn.Consider storing images/elevation data out-of-band (e.g., object storage with a URL reference) and keeping only the URL in the AI state message, similar to how
"IMAGE_PROCESSED"placeholders are used elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/actions.tsx` around lines 213 - 219, The message currently in actions.tsx serializes full blobs into the DB by embedding image: dataUrl, mapboxImage: mapboxDataUrl, googleImage: googleDataUrl and elevationPoints: elevationPointsData directly into JSON.stringify(...) for the `resolution_search_result` message; instead, persist only references: upload the images and elevationPoints payload to object storage (or DB table) and replace those keys in the object passed to JSON.stringify with URLs or short placeholders (e.g., imageUrl, mapboxImageUrl, googleImageUrl, elevationPointsUrl or "IMAGE_PROCESSED" tokens); update any consumers that read analysisResult.image / mapboxImage / googleImage / elevationPoints to fetch the data from the returned URL when needed and ensure uploads happen before the message is saved.
179-190: 🛠️ Refactor suggestion | 🟠 Major
sanitizedHistoryis dead code — and the image sanitization it was meant to apply never reachesaiState.done().
sanitizedHistoryis computed (replacing raw imagedata:URIs with"IMAGE_PROCESSED") but is never referenced after line 190.querySuggestoron line 191 correctly usessanitizedMessages, whileaiState.done()on line 200 uses the rawaiState.get().messages, meaning large base64 image blobs survive into the persisted AI state unchanged.The sanitized history should either be removed entirely (if intentionally not needed) or wired into
aiState.done().♻️ Proposed fix — wire `sanitizedHistory` into `aiState.done()` or remove it
-const currentMessages = aiState.get().messages; -const sanitizedHistory = currentMessages.map((m: any) => { - if (m.role === "user" && Array.isArray(m.content)) { - return { - ...m, - content: m.content.map((part: any) => - part.type === "image" ? { ...part, image: "IMAGE_PROCESSED" } : part - ) - } - } - return m -}); const relatedQueries = await querySuggestor(uiStream, sanitizedMessages); ... aiState.done({ ...aiState.get(), messages: [ - ...aiState.get().messages, + ...sanitizedHistory, // use sanitized version to avoid persisting raw base64 { id: groupeId, role: 'assistant', content: analysisResult.summary || 'Analysis complete.', type: 'response' }, ... ] });If image stripping in the persistent history is not desired, remove the
sanitizedHistoryblock entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/actions.tsx` around lines 179 - 190, sanitizedHistory is computed but never used, so large base64 image blobs remain in persisted state; update the aiState.done(...) call to use the sanitized history instead of the raw aiState.get().messages (or remove the sanitizedHistory block if persistence of sanitized messages is not desired). Specifically, replace the messages argument passed to aiState.done (or wherever aiState.get().messages is persisted) with sanitizedHistory (or use sanitizedMessages consistently), ensuring querySuggestor still receives sanitizedMessages and aiState.done receives sanitizedHistory so images are stripped before persistence.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
app/actions.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-02-08T16:50:33.791Z
Learnt from: ngoiyaeric
Repo: QueueLab/QCX PR: 0
File: :0-0
Timestamp: 2026-02-08T16:50:33.791Z
Learning: The PR branch implements RAG (Retrieval-Augmented Generation) via `lib/actions/rag.ts` which uses Supabase RPC functions `generate_embedding()` and `hybrid_search()` to retrieve relevant context from past conversations. However, there's a bug on line 433 of `app/actions.tsx` where `currentSystemPrompt` is passed to the researcher instead of `augmentedSystemPrompt`, preventing the retrieved context from being used.
Applied to files:
app/actions.tsx
🧬 Code graph analysis (1)
app/actions.tsx (5)
lib/types/index.ts (2)
AIMessage(59-77)Chat(49-57)components/map/map-results-container.tsx (1)
MapResultsContainer(19-32)lib/actions/chat.ts (1)
saveChat(90-121)lib/actions/chat-db.ts (2)
saveChat(83-126)Chat(9-9)components/map/map-query-handler.tsx (1)
prevData(39-48)
🔇 Additional comments (2)
app/actions.tsx (2)
32-43: LGTM — clean type exports forAIStateandUIState.Exporting these types enables proper typing of
createAI<AIState, UIState>and consumers ofgetUIStateFromAIState.
157-165: LGTM — correct guard before appendingMapResultsContainer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/actions.tsx`:
- Around line 550-554: The onSetAIState handler currently calls saveChat(state
as Chat) with a missing userId and an unsafe cast; fix by supplying the required
second argument and a properly shaped Chat object: retrieve the current user's
id from your auth/session context (e.g., getCurrentUser() or request.session),
map AIState (chatId, messages, etc.) to the OldChatType/Chat fields (id ->
chatId, title/path/defaults as needed, userId -> retrieved id) and call
saveChat(chat, userId) instead of casting; also add a guard to skip persistence
or surface a clear error if no userId is available so saveChat is never invoked
with undefined userId.
---
Outside diff comments:
In `@app/actions.tsx`:
- Around line 213-219: The message currently in actions.tsx serializes full
blobs into the DB by embedding image: dataUrl, mapboxImage: mapboxDataUrl,
googleImage: googleDataUrl and elevationPoints: elevationPointsData directly
into JSON.stringify(...) for the `resolution_search_result` message; instead,
persist only references: upload the images and elevationPoints payload to object
storage (or DB table) and replace those keys in the object passed to
JSON.stringify with URLs or short placeholders (e.g., imageUrl, mapboxImageUrl,
googleImageUrl, elevationPointsUrl or "IMAGE_PROCESSED" tokens); update any
consumers that read analysisResult.image / mapboxImage / googleImage /
elevationPoints to fetch the data from the returned URL when needed and ensure
uploads happen before the message is saved.
- Around line 179-190: sanitizedHistory is computed but never used, so large
base64 image blobs remain in persisted state; update the aiState.done(...) call
to use the sanitized history instead of the raw aiState.get().messages (or
remove the sanitizedHistory block if persistence of sanitized messages is not
desired). Specifically, replace the messages argument passed to aiState.done (or
wherever aiState.get().messages is persisted) with sanitizedHistory (or use
sanitizedMessages consistently), ensuring querySuggestor still receives
sanitizedMessages and aiState.done receives sanitizedHistory so images are
stripped before persistence.
---
Duplicate comments:
In `@app/actions.tsx`:
- Around line 472-475: Replace the plain currentSystemPrompt passed into
researcher with the RAG-augmented prompt variable produced earlier (e.g.,
augmentedSystemPrompt or ragAugmentedPrompt) so the researcher call uses the
RAG-augmented prompt; locate where the RAG augmentation is created and swap
currentSystemPrompt for that augmented prompt in the researcher(...) invocation
(the call site around createStreamableValue('') / researcher(...)).
- Around line 594-619: The case 'resolution_search_result' unguardedly calls
JSON.parse(content as string) which can throw on invalid input; update this
branch to safely parse content (e.g., use a shared safeParse helper or wrap
JSON.parse in try/catch) and validate the result before using analysisResult,
ensuring you handle parse failures by returning a safe fallback or error UI;
modify the logic around analysisResult, geoJson, image, mapboxImage,
googleImage, and elevationPoints to only access them after successful
parse/validation so ResolutionCarousel and MapResultsContainer are not rendered
with undefined/invalid data.
- Around line 567-579: The 'related' and 'inquiry' branches call JSON.parse on
persisted content without error handling and set isCollapsed to a plain boolean
while the code expects a StreamableValue<boolean>; wrap the JSON.parse calls in
try/catch (for example in the switch cases handling 'related' and 'inquiry') and
fall back to a safe default (e.g., empty array or null) when parsing fails, and
convert the plain boolean to the expected StreamableValue form when returning
the object (e.g., replace isCollapsed: true with an appropriate
StreamableValue<boolean> instance or shape used across the codebase); update the
logic around SearchRelated and any component props that consume the parsed data
to handle the fallback value.
- Around line 133-155: The current fetch block that builds a fragile HTTP
self-call string (the fetch(...) invocation) can break due to unencoded params
and calling your own API over HTTP; instead either call the elevation logic
directly from server code (import and invoke the handler/function that computes
elevation) or, if you must call the route, build a safe relative URL and use
URLSearchParams/encodeURIComponent for the bounds, gridSize and geometry params;
update the code around analysisResult.elevationData and elevationPointsData to
replace the string-concatenated fetch(...) with a call to the internal elevation
function (preferred) or a fetch using new URLSearchParams({ bounds:
JSON.stringify(...), gridSize: String(...), geometry: JSON.stringify(...) }) and
a relative path ('/api/elevation') so you avoid environment-based self-calls and
ensure params are encoded.
- Fixed saveChat call signature with userId and proper type casting - Fixed BotMessage, SearchRelated, and CopilotDisplay prop names and types for historical restoration - Ensured all restored UI components receive correctly typed StreamableValues where expected - Verified build completion locally Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
- Removed circular 'AI' provider imports from client components (ResolutionCarousel, SearchRelated, etc.) - Switched to using useActions<any>() and useUIState<any>() in components to break circularity with actions.tsx - Verified build success with all elevation heatmap features - Fixed type errors in components caused by removed AI type imports Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/header-search-button.tsx (2)
44-47: 🧹 Nitpick | 🔵 Trivial
!actionsguard is unreachable —useActions()never returns a falsy value.
useActions<any>()returns the server actions object from the RSC context, which is always a non-null object. The early-return/toast and thedisabled={... || !actions}conditions on lines 155 and 167 are dead code.♻️ Proposed fix
- if (!actions) { - toast.error('Search actions are not available.') - return - } -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/header-search-button.tsx` around lines 44 - 47, The guard that checks if (!actions) is unreachable because useActions<any>() always returns a non-null actions object; remove the entire if (!actions) { toast.error(...) return } block in HeaderSearchButton and eliminate all checks of || !actions in the disabled props (and any related toast/error handling) so the component relies on the real actionable state instead of a dead null-check on the actions variable returned by useActions.
123-128:⚠️ Potential issue | 🔴 Critical
file_mapbox/file_googlefields are never read by the server — carousel dual-image feature is broken.The client appends
file_mapboxandfile_googleasBlobentries, butapp/actions.tsx(lines 79–80) readsmapboxImageandgoogleImageas string fields withformData.get('mapboxImage'). Those fields are never populated, somapboxDataUrl/googleDataUrlare alwaysnull. The comparison slider inResolutionCarouselwill never render, and the saved AI state storesmapboxImage: nullpermanently.Fix by reading the Blob fields on the server and converting them to data URLs (see proposed diff in
app/actions.tsx), or convert the Blobs to base64 data URLs on the client before appending under the expected field names.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/header-search-button.tsx` around lines 123 - 128, The client currently appends blobs as 'file_mapbox'/'file_google' but the server (app/actions.tsx) expects string data URLs under 'mapboxImage' and 'googleImage', so convert mapboxBlob/googleBlob to base64 data URLs on the client before appending: in header-search-button.tsx take mapboxBlob/googleBlob, read them with FileReader or createObjectURL+canvas to produce data URL strings, append those strings to formData as 'mapboxImage' and 'googleImage' (you may keep the existing 'file_mapbox'/'file_google' for backward compatibility), ensuring the server-side formData.get('mapboxImage')/get('googleImage') returns the expected strings so mapboxDataUrl/googleDataUrl and ResolutionCarousel render correctly.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
app/actions.tsxcomponents/header-search-button.tsxcomponents/mobile-icons-bar.tsxcomponents/resolution-carousel.tsxcomponents/search-related.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-02-08T16:50:33.791Z
Learnt from: ngoiyaeric
Repo: QueueLab/QCX PR: 0
File: :0-0
Timestamp: 2026-02-08T16:50:33.791Z
Learning: The PR branch implements RAG (Retrieval-Augmented Generation) via `lib/actions/rag.ts` which uses Supabase RPC functions `generate_embedding()` and `hybrid_search()` to retrieve relevant context from past conversations. However, there's a bug on line 433 of `app/actions.tsx` where `currentSystemPrompt` is passed to the researcher instead of `augmentedSystemPrompt`, preventing the retrieved context from being used.
Applied to files:
app/actions.tsx
🧬 Code graph analysis (3)
components/search-related.tsx (2)
components/chat.tsx (1)
aiState(78-83)components/chat-messages.tsx (1)
ChatMessagesProps(7-9)
components/mobile-icons-bar.tsx (2)
components/sidebar/chat-history-client.tsx (1)
ChatHistoryClientProps(26-28)components/chat.tsx (2)
aiState(78-83)Chat(29-216)
components/header-search-button.tsx (1)
components/chat.tsx (1)
aiState(78-83)
🔇 Additional comments (3)
components/search-related.tsx (1)
12-29: LGTM — type widening is intentional.Removing the
AIcircular import and wideninguseUIStatetoanyaligns with the PR-wide refactor to break circular dependencies.components/mobile-icons-bar.tsx (1)
5-29: LGTM — consistent with the PR-wide circular-import removal.app/actions.tsx (1)
598-602: No action needed. The code at lines 598-602 correctly implements theCopilotDisplaycomponent usage. The component's prop interface acceptscontentas a string (notinquiry), and the component internally parses the JSON on line 14 ofcomponents/copilot-display.tsx. Passingcontent={content as string}wherecontentisJSON.stringify(inquiry)is the correct approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/actions.tsx`:
- Around line 485-489: The isCollapsed completion promise is never resolved in
the non-inquire chat path causing subscribers to hang; update the error and
success branches that currently call isGenerating.done() and uiStream.done()
(the early-return when hasError and the normal success path after generation) to
also call isCollapsed.done() so isCollapsed is always completed regardless of
inquire vs writer/researcher flow; locate the branches around the hasError check
and the success completion (the same blocks that call isGenerating.done() and
uiStream.done()) and add a matching isCollapsed.done() call before returning or
finishing.
- Around line 77-90: The handler branch for action === 'resolution_search' reads
mapboxImage and googleImage as string fields but the client sends Blob fields
named file_mapbox and file_google; update the code to call
formData.get('file_mapbox') and formData.get('file_google'), check they are
Blobs/Files, await .arrayBuffer() on each present blob, and convert to data URLs
(like you do for file via Buffer.from(...).toString('base64')) to populate
mapboxDataUrl and googleDataUrl so the ResolutionCarousel comparison slider
receives the correct image data.
- Around line 82-85: In the early-return branch where "!file" currently only
calls isGenerating.done(false) and returns, also close the UI stream and mark
the collapsed state done to avoid leaking subscribers: call the appropriate
cleanup on uiStream (e.g., uiStream.close() or uiStream.end()/uiStream.done()
depending on its API) and call isCollapsed.done(false) before returning from the
function that contains isGenerating.done(false) (the branch that returns the
object with id: nanoid(), isGenerating: isGenerating.value, component: null,
isCollapsed: isCollapsed.value).
- Around line 568-570: The current restore logic in app/actions.tsx drops user
and system messages because of the early check "if (role === 'user' || role ===
'system') return null"; instead, modify the restore mapping from AIState ->
UIState to map messages with role === 'user' (and system if you want them
visible) into <UserMessage> nodes (preserving their type values like 'input',
'input_related', 'inquiry') rather than returning null; keep existing handling
for assistant messages unchanged and only return null for truly unknown roles,
updating the block that contains that role check so it constructs and returns a
UserMessage UI node with the original content and metadata.
- Around line 573-588: The switch cases 'response' and 'related' in
app/actions.tsx declare block-scoped consts (botMessageStream, relatedQueries,
relatedStream) without braces which triggers the no-case-declarations lint rule;
fix by wrapping each case's statements in its own block (e.g., case 'response':
{ ... } and case 'related': { ... }) so createStreamableValue, botMessageStream,
relatedQueries, relatedStream and the returned components (BotMessage,
SearchRelated) are properly scoped and the ESLint rule is satisfied.
In `@components/resolution-carousel.tsx`:
- Line 28: The variable declaration for actions uses a redundant double-cast:
remove the trailing "as any" from the line that assigns actions so it reads only
const actions = useActions<any>() (or better, tighten the generic instead of
using any); update the declaration in components/resolution-carousel.tsx where
useActions<any>() is called to eliminate the no-op cast.
---
Outside diff comments:
In `@components/header-search-button.tsx`:
- Around line 44-47: The guard that checks if (!actions) is unreachable because
useActions<any>() always returns a non-null actions object; remove the entire if
(!actions) { toast.error(...) return } block in HeaderSearchButton and eliminate
all checks of || !actions in the disabled props (and any related toast/error
handling) so the component relies on the real actionable state instead of a dead
null-check on the actions variable returned by useActions.
- Around line 123-128: The client currently appends blobs as
'file_mapbox'/'file_google' but the server (app/actions.tsx) expects string data
URLs under 'mapboxImage' and 'googleImage', so convert mapboxBlob/googleBlob to
base64 data URLs on the client before appending: in header-search-button.tsx
take mapboxBlob/googleBlob, read them with FileReader or createObjectURL+canvas
to produce data URL strings, append those strings to formData as 'mapboxImage'
and 'googleImage' (you may keep the existing 'file_mapbox'/'file_google' for
backward compatibility), ensuring the server-side
formData.get('mapboxImage')/get('googleImage') returns the expected strings so
mapboxDataUrl/googleDataUrl and ResolutionCarousel render correctly.
---
Duplicate comments:
In `@app/actions.tsx`:
- Around line 473-476: The code calls researcher(...) with currentSystemPrompt,
causing the RAG-augmented prompt to be ignored; update the call where streamText
is created (createStreamableValue and streamText) to pass the RAG-augmented
prompt variable produced by your RAG augmentation step (instead of
currentSystemPrompt) into researcher so researcher receives the augmented prompt
and toolResponses/fullResponse reflect RAG context.
- Around line 551-558: The onSetAIState handler calls saveChat with an incorrect
shape and missing userId: update onSetAIState to reliably obtain userId (await
getCurrentUserIdOnServer() || 'anonymous'), construct a proper Chat object
matching the saveChat signature (use the AIState fields: chatId -> id, messages,
and include any required properties like title/createdAt/userId as defined by
the Chat type) instead of casting to any, and then call saveChat(chat, userId).
Also ensure you await or properly handle errors (try/catch or .catch) on
saveChat to surface failures; refer to onSetAIState, getCurrentUserIdOnServer,
AIState and saveChat to locate fixes.
- Around line 134-156: This server-side code is making a self-HTTP call to
/api/elevation with JSON.stringify in query params which can produce malformed
URLs; instead either call the elevation helper directly (e.g., use the internal
elevation function used by the API instead of fetch) or at minimum build a safe
URL by encoding params (use encodeURIComponent on
JSON.stringify(analysisResult.elevationData.bounds) and geometry) and ensure you
use the correct base (avoid NEXT_PUBLIC_BASE_URL on server). Update the block
that references analysisResult.elevationData, elevationResponse,
elevationPointsData, and drawnFeatures to call the internal elevation logic (or
encode params) and return the points without performing an unsafe self-fetch.
- Around line 572-628: Guard all untrusted JSON.parse calls in the switch cases
by wrapping them in try/catch and validating the parsed shape before using it:
for the 'related' case, try parsing content into relatedQueries, fall back to an
empty array (or return a safe error/placeholder component) on parse/validation
failure, and ensure isCollapsed uses the expected type (if the UI expects a
streamable or signal instead of a plain boolean, wrap the boolean with
createStreamableValue or the correct constructor rather than passing true
directly); for 'inquiry' and 'resolution_search_result' wrap JSON.parse(content)
in try/catch, validate required fields (e.g., analysisResult.geoJson is a
FeatureCollection, image/mapboxImage/googleImage are strings, elevationPoints
exists) and return a safe fallback component or skip rendering when validation
fails; use the existing helpers and symbols (createStreamableValue,
relatedStream, BotMessage, SearchRelated, CopilotDisplay, ResolutionCarousel,
MapResultsContainer, analysisResult, geoJson, elevationPoints) to implement
these checks and safe fallbacks.
| if (action === 'resolution_search') { | ||
| const file_mapbox = formData?.get('file_mapbox') as File; | ||
| const file_google = formData?.get('file_google') as File; | ||
| const file = (formData?.get('file') as File) || file_mapbox || file_google; | ||
| const timezone = (formData?.get('timezone') as string) || 'UTC'; | ||
| const lat = formData?.get('latitude') ? parseFloat(formData.get('latitude') as string) : undefined; | ||
| const lng = formData?.get('longitude') ? parseFloat(formData.get('longitude') as string) : undefined; | ||
| const location = (lat !== undefined && lng !== undefined) ? { lat, lng } : undefined; | ||
| const file = formData?.get('file') as File; | ||
| const mapboxImage = formData?.get('mapboxImage') as string; | ||
| const googleImage = formData?.get('googleImage') as string; | ||
|
|
||
| if (!file) { | ||
| throw new Error('No file provided for resolution search.'); | ||
| isGenerating.done(false); | ||
| return { id: nanoid(), isGenerating: isGenerating.value, component: null, isCollapsed: isCollapsed.value }; | ||
| } | ||
|
|
||
| const mapboxBuffer = file_mapbox ? await file_mapbox.arrayBuffer() : null; | ||
| const mapboxDataUrl = mapboxBuffer ? `data:${file_mapbox.type};base64,${Buffer.from(mapboxBuffer).toString('base64')}` : null; | ||
|
|
||
| const googleBuffer = file_google ? await file_google.arrayBuffer() : null; | ||
| const googleDataUrl = googleBuffer ? `data:${file_google.type};base64,${Buffer.from(googleBuffer).toString('base64')}` : null; | ||
|
|
||
| const buffer = await file.arrayBuffer(); | ||
| const dataUrl = `data:${file.type};base64,${Buffer.from(buffer).toString('base64')}`; | ||
| const mapboxDataUrl = mapboxImage || null; | ||
| const googleDataUrl = googleImage || null; |
There was a problem hiding this comment.
mapboxImage/googleImage will always be null — FormData field names don't match what the client sends.
header-search-button.tsx appends Blobs under file_mapbox and file_google, but here the server reads them as string fields mapboxImage and googleImage. formData.get('mapboxImage') returns null, making mapboxDataUrl/googleDataUrl always null. The ResolutionCarousel comparison slider will never render.
🐛 Proposed fix — read and convert the blob fields
- const file = formData?.get('file') as File;
- const mapboxImage = formData?.get('mapboxImage') as string;
- const googleImage = formData?.get('googleImage') as string;
+ const file = formData?.get('file') as File;
+ const mapboxFile = formData?.get('file_mapbox') as File | null;
+ const googleFile = formData?.get('file_google') as File | null;- const mapboxDataUrl = mapboxImage || null;
- const googleDataUrl = googleImage || null;
+ const mapboxDataUrl = mapboxFile
+ ? `data:${mapboxFile.type};base64,${Buffer.from(await mapboxFile.arrayBuffer()).toString('base64')}`
+ : null;
+ const googleDataUrl = googleFile
+ ? `data:${googleFile.type};base64,${Buffer.from(await googleFile.arrayBuffer()).toString('base64')}`
+ : null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (action === 'resolution_search') { | |
| const file_mapbox = formData?.get('file_mapbox') as File; | |
| const file_google = formData?.get('file_google') as File; | |
| const file = (formData?.get('file') as File) || file_mapbox || file_google; | |
| const timezone = (formData?.get('timezone') as string) || 'UTC'; | |
| const lat = formData?.get('latitude') ? parseFloat(formData.get('latitude') as string) : undefined; | |
| const lng = formData?.get('longitude') ? parseFloat(formData.get('longitude') as string) : undefined; | |
| const location = (lat !== undefined && lng !== undefined) ? { lat, lng } : undefined; | |
| const file = formData?.get('file') as File; | |
| const mapboxImage = formData?.get('mapboxImage') as string; | |
| const googleImage = formData?.get('googleImage') as string; | |
| if (!file) { | |
| throw new Error('No file provided for resolution search.'); | |
| isGenerating.done(false); | |
| return { id: nanoid(), isGenerating: isGenerating.value, component: null, isCollapsed: isCollapsed.value }; | |
| } | |
| const mapboxBuffer = file_mapbox ? await file_mapbox.arrayBuffer() : null; | |
| const mapboxDataUrl = mapboxBuffer ? `data:${file_mapbox.type};base64,${Buffer.from(mapboxBuffer).toString('base64')}` : null; | |
| const googleBuffer = file_google ? await file_google.arrayBuffer() : null; | |
| const googleDataUrl = googleBuffer ? `data:${file_google.type};base64,${Buffer.from(googleBuffer).toString('base64')}` : null; | |
| const buffer = await file.arrayBuffer(); | |
| const dataUrl = `data:${file.type};base64,${Buffer.from(buffer).toString('base64')}`; | |
| const mapboxDataUrl = mapboxImage || null; | |
| const googleDataUrl = googleImage || null; | |
| if (action === 'resolution_search') { | |
| const file = formData?.get('file') as File; | |
| const mapboxFile = formData?.get('file_mapbox') as File | null; | |
| const googleFile = formData?.get('file_google') as File | null; | |
| if (!file) { | |
| isGenerating.done(false); | |
| return { id: nanoid(), isGenerating: isGenerating.value, component: null, isCollapsed: isCollapsed.value }; | |
| } | |
| const buffer = await file.arrayBuffer(); | |
| const dataUrl = `data:${file.type};base64,${Buffer.from(buffer).toString('base64')}`; | |
| const mapboxDataUrl = mapboxFile | |
| ? `data:${mapboxFile.type};base64,${Buffer.from(await mapboxFile.arrayBuffer()).toString('base64')}` | |
| : null; | |
| const googleDataUrl = googleFile | |
| ? `data:${googleFile.type};base64,${Buffer.from(await googleFile.arrayBuffer()).toString('base64')}` | |
| : null; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/actions.tsx` around lines 77 - 90, The handler branch for action ===
'resolution_search' reads mapboxImage and googleImage as string fields but the
client sends Blob fields named file_mapbox and file_google; update the code to
call formData.get('file_mapbox') and formData.get('file_google'), check they are
Blobs/Files, await .arrayBuffer() on each present blob, and convert to data URLs
(like you do for file via Buffer.from(...).toString('base64')) to populate
mapboxDataUrl and googleDataUrl so the ResolutionCarousel comparison slider
receives the correct image data.
| if (!file) { | ||
| throw new Error('No file provided for resolution search.'); | ||
| isGenerating.done(false); | ||
| return { id: nanoid(), isGenerating: isGenerating.value, component: null, isCollapsed: isCollapsed.value }; | ||
| } |
There was a problem hiding this comment.
uiStream and isCollapsed are never closed on the early-return path — resource leak.
When !file, isGenerating.done(false) is called but uiStream and isCollapsed are left open. Any subscriber to isCollapsed.value will hang indefinitely.
🐛 Proposed fix
if (!file) {
isGenerating.done(false);
+ uiStream.done();
+ isCollapsed.done(false);
return { id: nanoid(), isGenerating: isGenerating.value, component: null, isCollapsed: isCollapsed.value };
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/actions.tsx` around lines 82 - 85, In the early-return branch where
"!file" currently only calls isGenerating.done(false) and returns, also close
the UI stream and mark the collapsed state done to avoid leaking subscribers:
call the appropriate cleanup on uiStream (e.g., uiStream.close() or
uiStream.end()/uiStream.done() depending on its API) and call
isCollapsed.done(false) before returning from the function that contains
isGenerating.done(false) (the branch that returns the object with id: nanoid(),
isGenerating: isGenerating.value, component: null, isCollapsed:
isCollapsed.value).
| if (hasError) { | ||
| isGenerating.done(false) | ||
| uiStream.done() | ||
| return | ||
| } |
There was a problem hiding this comment.
isCollapsed is never completed in the researcher/writer path — resource leak on every normal chat turn.
isCollapsed.done() is only called in the inquire branch (line 460). In the else branch — which handles the vast majority of chat turns — both the early-return on error (lines 485–488) and the success path (lines 520–521) close uiStream and isGenerating but leave isCollapsed open. Every returned isCollapsed.value will stall indefinitely for any subscriber.
🐛 Proposed fix
if (hasError) {
isGenerating.done(false)
uiStream.done()
+ isCollapsed.done(false)
return
} isGenerating.done(false)
uiStream.done()
+ isCollapsed.done(false)
}Also applies to: 520-522
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/actions.tsx` around lines 485 - 489, The isCollapsed completion promise
is never resolved in the non-inquire chat path causing subscribers to hang;
update the error and success branches that currently call isGenerating.done()
and uiStream.done() (the early-return when hasError and the normal success path
after generation) to also call isCollapsed.done() so isCollapsed is always
completed regardless of inquire vs writer/researcher flow; locate the branches
around the hasError check and the success completion (the same blocks that call
isGenerating.done() and uiStream.done()) and add a matching isCollapsed.done()
call before returning or finishing.
| if (role === 'user' || role === 'system') { | ||
| return null | ||
| } |
There was a problem hiding this comment.
All user messages are silently dropped when restoring a chat.
The early return null for role === 'user' || role === 'system' means that every user message (type: 'input', 'input_related', 'inquiry') stored in AIState is excluded from the restored UIState. When a chat is reloaded from history, users will only see assistant responses — none of their own inputs. Restore the user messages as <UserMessage> nodes.
🐛 Proposed fix
- if (role === 'user' || role === 'system') {
- return null
- }
+ if (role === 'system') return null
+ if (role === 'user') {
+ const text = typeof content === 'string' ? content : ''
+ return { id, component: <UserMessage content={text} /> }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/actions.tsx` around lines 568 - 570, The current restore logic in
app/actions.tsx drops user and system messages because of the early check "if
(role === 'user' || role === 'system') return null"; instead, modify the restore
mapping from AIState -> UIState to map messages with role === 'user' (and system
if you want them visible) into <UserMessage> nodes (preserving their type values
like 'input', 'input_related', 'inquiry') rather than returning null; keep
existing handling for assistant messages unchanged and only return null for
truly unknown roles, updating the block that contains that role check so it
constructs and returns a UserMessage UI node with the original content and
metadata.
| case 'response': | ||
| const botMessageStream = createStreamableValue(content as string) | ||
| botMessageStream.done(content as string) | ||
| return { | ||
| id, | ||
| component: <BotMessage content={botMessageStream.value} /> | ||
| } | ||
| break | ||
| case 'assistant': | ||
| const answer = createStreamableValue(content as string) | ||
| answer.done(content as string) | ||
| switch (type) { | ||
| case 'response': | ||
| return { | ||
| id, | ||
| component: ( | ||
| <Section title="response"> | ||
| <BotMessage content={answer.value} /> | ||
| </Section> | ||
| ) | ||
| } | ||
| case 'related': | ||
| const relatedQueries = createStreamableValue<RelatedQueries>({ | ||
| items: [] | ||
| }) | ||
| relatedQueries.done(JSON.parse(content as string)) | ||
| return { | ||
| id, | ||
| component: ( | ||
| <Section title="Related" separator={true}> | ||
| <SearchRelated relatedQueries={relatedQueries.value} /> | ||
| </Section> | ||
| ) | ||
| } | ||
| case 'followup': | ||
| return { | ||
| id, | ||
| component: ( | ||
| <Section title="Follow-up" className="pb-8"> | ||
| <FollowupPanel /> | ||
| </Section> | ||
| ) | ||
| } | ||
| case 'resolution_search_result': { | ||
| const analysisResult = JSON.parse(content as string); | ||
| const geoJson = analysisResult.geoJson as FeatureCollection; | ||
| const image = analysisResult.image as string; | ||
| const mapboxImage = analysisResult.mapboxImage as string; | ||
| const googleImage = analysisResult.googleImage as string; | ||
| case 'related': | ||
| const relatedQueries = JSON.parse(content as string) | ||
| const relatedStream = createStreamableValue(relatedQueries) | ||
| relatedStream.done(relatedQueries) | ||
| return { | ||
| id, | ||
| component: <SearchRelated relatedQueries={relatedStream.value} />, | ||
| isCollapsed: true | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
const declarations inside switch cases without block braces — lint error risk.
The 'response' and 'related' cases declare const variables that are block-scoped to the entire switch body. While the names don't currently collide, this triggers the no-case-declarations ESLint rule and is fragile to future additions. Wrap each case in braces, matching the correct style already used by 'resolution_search_result'.
♻️ Proposed fix
- case 'response':
- const botMessageStream = createStreamableValue(content as string)
- botMessageStream.done(content as string)
- return {
- id,
- component: <BotMessage content={botMessageStream.value} />
- }
- case 'related':
- const relatedQueries = JSON.parse(content as string)
- const relatedStream = createStreamableValue(relatedQueries)
- relatedStream.done(relatedQueries)
- return {
- id,
- component: <SearchRelated relatedQueries={relatedStream.value} />,
- isCollapsed: true
- }
+ case 'response': {
+ const botMessageStream = createStreamableValue(content as string)
+ botMessageStream.done(content as string)
+ return {
+ id,
+ component: <BotMessage content={botMessageStream.value} />
+ }
+ }
+ case 'related': {
+ const relatedQueries = JSON.parse(content as string)
+ const relatedStream = createStreamableValue(relatedQueries)
+ relatedStream.done(relatedQueries)
+ return {
+ id,
+ component: <SearchRelated relatedQueries={relatedStream.value} />,
+ isCollapsed: true
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/actions.tsx` around lines 573 - 588, The switch cases 'response' and
'related' in app/actions.tsx declare block-scoped consts (botMessageStream,
relatedQueries, relatedStream) without braces which triggers the
no-case-declarations lint rule; fix by wrapping each case's statements in its
own block (e.g., case 'response': { ... } and case 'related': { ... }) so
createStreamableValue, botMessageStream, relatedQueries, relatedStream and the
returned components (BotMessage, SearchRelated) are properly scoped and the
ESLint rule is satisfied.
| 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 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant double-cast — as any after useActions<any>() is a no-op.
useActions<any>() already returns any; the trailing as any is dead type noise. Remove it.
♻️ Proposed fix
- const actions = useActions<any>() as any
+ const actions = useActions<any>()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const actions = useActions<any>() as any | |
| const actions = useActions<any>() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/resolution-carousel.tsx` at line 28, The variable declaration for
actions uses a redundant double-cast: remove the trailing "as any" from the line
that assigns actions so it reads only const actions = useActions<any>() (or
better, tighten the generic instead of using any); update the declaration in
components/resolution-carousel.tsx where useActions<any>() is called to
eliminate the no-op cast.
- Implemented /api/elevation endpoint and ElevationHeatmapLayer visualization - Resolved circular dependencies between app/actions.tsx and client components - Fixed Webpack runtime error by removing direct require for missing mapbox_mcp_config.json - Ensured SSR safety with MapResultsContainer and dynamic component loading - Corrected type errors and function signatures for AI agents and persistence layer Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
- Moved inline HTMX fallback script from `app/layout.tsx` to `public/htmx-fallback.js` - Loaded the script using `next/script` with `strategy="beforeInteractive"` to improve security and avoid CSP issues with inline scripts - Maintained the same logic for setting up safety fallback handlers for HTMX events Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
…, and related geospatial improvements
2 participants
This PR implements a complete elevation heat map system for the Resolution Search feature.
Key changes:
/api/elevationendpoint provides elevation data for a grid of points within a given bounding box, optionally filtered by a GeoJSON polygon. It uses the Mapbox Terrain vector tileset for reliable data fetching.ElevationHeatmapLayercomponent adds a Mapbox heatmap layer (low to high elevation) and a points layer (visible at zoom 14+) with interactive popups showing exact elevation values.resolutionSearchagent's schema and system prompt were updated to allow the AI to proactively request elevation analysis for drawn features.submitserver action andgetUIStateFromAIStatewere updated to handle the fetching, rendering, and restoration of elevation heatmaps, ensuring they persist across chat reloads.lib/utils/elevation.tsfor bounds calculation and Terrain-RGB decoding.PR created automatically by Jules for task 9510993991100510830 started by @ngoiyaeric
Summary by CodeRabbit
New Features
Refactor