Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 36 additions & 1 deletion app/actions.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { BotMessage } from '@/components/message'
import { SearchSection } from '@/components/search-section'
import SearchRelated from '@/components/search-related'
import { GeoJsonLayer } from '@/components/map/geojson-layer'
import { ElevationHeatmapLayer } from '@/components/map/elevation-heatmap-layer'
import { ResolutionCarousel } from '@/components/resolution-carousel'
import { ResolutionImage } from '@/components/resolution-image'
import { CopilotDisplay } from '@/components/copilot-display'
Expand Down Expand Up @@ -123,6 +124,36 @@ async function submit(formData?: FormData, skip?: boolean) {
);
}

// Handle elevation heat map if requested
if (analysisResult.elevationData?.requested && analysisResult.elevationData.bounds) {
try {
const baseUrl = process.env.NEXT_PUBLIC_BASE_URL || 'http://localhost:3000';
const elevationResponse = await fetch(
`${baseUrl}/api/elevation?bounds=${JSON.stringify(analysisResult.elevationData.bounds)}&gridSize=${analysisResult.elevationData.gridSize || 20}`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

rg -n "gridSize" --type=ts -C4

Repository: QueueLab/QCX

Length of output: 4728


🏁 Script executed:

# Also check for gridSize in JavaScript/JSON files
rg -n "gridSize" --type=js -C4

Repository: QueueLab/QCX

Length of output: 38


🏁 Script executed:

# Search for schema validation and type definitions
rg -n "gridSize" -C5

Repository: QueueLab/QCX

Length of output: 5199


🏁 Script executed:

# Check specifically for the elevation API route
fd -type f -name "*elevation*" | head -20

Repository: QueueLab/QCX

Length of output: 226


🏁 Script executed:

# Look for schema files that might validate gridSize
fd -type f -name "*schema*" | head -20

Repository: QueueLab/QCX

Length of output: 226


🏁 Script executed:

cat -n app/api/elevation/route.ts | head -80

Repository: QueueLab/QCX

Length of output: 3936


Add gridSize ceiling and URL-encode bounds parameter to prevent runaway Mapbox API costs.

The LLM-generated gridSize in analysisResult.elevationData.gridSize is forwarded unchecked across three files with no validation layer. Since the elevation API generates gridSize² coordinate points and makes one Mapbox Tilequery API call per point, an uncapped gridSize creates quadratic cost: gridSize=100 → 10,000 calls; gridSize=500 → 250,000 calls.

Additionally, JSON.stringify(bounds) is embedded in the URL without encodeURIComponent, allowing unencoded characters like {, ", : to be sent as query parameters.

🛡️ Proposed fix — cap gridSize and encode the query parameter
+ const MAX_GRID_SIZE = 50;
+ const safeGridSize = Math.min(Math.max(1, Number(analysisResult.elevationData.gridSize) || 20), MAX_GRID_SIZE);
  const elevationResponse = await fetch(
-   `${baseUrl}/api/elevation?bounds=${JSON.stringify(analysisResult.elevationData.bounds)}&gridSize=${analysisResult.elevationData.gridSize || 20}`
+   `${baseUrl}/api/elevation?bounds=${encodeURIComponent(JSON.stringify(analysisResult.elevationData.bounds))}&gridSize=${safeGridSize}`
  );
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/actions.tsx` at line 128, The URL builds the elevation call with an
unchecked analysisResult.elevationData.gridSize and an unencoded bounds JSON,
which can cause quadratic Mapbox costs and malformed query strings; clamp/ceil
gridSize at the origin (e.g., replace analysisResult.elevationData.gridSize with
a validated value using Math.floor/Math.min to enforce a MAX_GRID_SIZE and a
sensible minimum) and also validate again in the elevation API handler
(/api/elevation) to be defensive, and URL-encode the bounds parameter by
wrapping JSON.stringify(analysisResult.elevationData.bounds) with
encodeURIComponent before interpolating into the `${baseUrl}/api/elevation?...`
URL.

);
Comment on lines +128 to +134

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Building the elevation API URL by embedding raw JSON.stringify(bounds) into the query string is brittle. Characters like [ ] , and spaces can cause malformed URLs unless encoded, and large bounds strings can quickly become unwieldy.

Also, using NEXT_PUBLIC_BASE_URL from server code is a smell (it’s intended for client exposure) and the hard-coded http://localhost:3000 fallback can break in production/serverless where the canonical host is not known at build time.

Prefer calling the route via a relative URL from server code and encode query params explicitly.

Suggestion

Use a relative URL and URLSearchParams to ensure proper encoding and avoid env-coupled base URLs:

const params = new URLSearchParams({
  bounds: JSON.stringify(analysisResult.elevationData.bounds),
  gridSize: String(analysisResult.elevationData.gridSize ?? 20),
});

const elevationResponse = await fetch(`/api/elevation?${params.toString()}`);

If you need absolute URLs (e.g., for non-Next fetch contexts), derive it from req.headers server-side instead of NEXT_PUBLIC_*.

Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this suggestion.


if (elevationResponse.ok) {
const elevationData = await elevationResponse.json();

if (elevationData.points && elevationData.points.length > 0) {
(analysisResult as any).elevationPoints = elevationData.points;
(analysisResult as any).elevationStatistics = elevationData.statistics;
Comment thread
coderabbitai[bot] marked this conversation as resolved.

Comment on lines +137 to +140

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This introduces any writes into analysisResult to persist elevationPoints/elevationStatistics. That’s a clear maintainability issue: it hides shape changes and makes it easy to forget to persist/restore fields consistently.

Since the PR already extends the structured schema with elevationData, it would be better to extend the analysis result type (or a local interface) to include these persisted fields and avoid (analysisResult as any) mutations.

Suggestion

Define a typed shape for the analysis payload you persist (e.g., ResolutionSearchResult) and assign properties without any:

type ResolutionSearchResult = {
  // existing fields...
  elevationPoints?: ElevationPoint[];
  elevationStatistics?: ElevationStatistics;
};

const result: ResolutionSearchResult = analysisResult;
result.elevationPoints = elevationData.points;
result.elevationStatistics = elevationData.statistics;

Then persist result and parse it similarly on restore.

Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this suggestion.

uiStream.append(
<ElevationHeatmapLayer
id={groupeId}
points={elevationData.points}
statistics={elevationData.statistics}
/>
);
}
}
} catch (error) {
console.error('Error fetching elevation data:', error);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}


messages.push({ role: 'assistant', content: analysisResult.summary || 'Analysis complete.' });

const sanitizedMessages: CoreMessage[] = messages.map((m: any) => {
Expand Down Expand Up @@ -173,7 +204,9 @@ async function submit(formData?: FormData, skip?: boolean) {
...analysisResult,
image: dataUrl,
mapboxImage: mapboxDataUrl,
googleImage: googleDataUrl
googleImage: googleDataUrl,
elevationPoints: (analysisResult as any).elevationPoints,
elevationStatistics: (analysisResult as any).elevationStatistics
}),
type: 'resolution_search_result'
},
Expand Down Expand Up @@ -745,6 +778,8 @@ export const getUIStateFromAIState = (aiState: AIState): UIState => {
const image = analysisResult.image as string;
const mapboxImage = analysisResult.mapboxImage as string;
const googleImage = analysisResult.googleImage as string;
const elevationPoints = analysisResult.elevationPoints;
const elevationStatistics = analysisResult.elevationStatistics;

return {
id,
Expand Down
92 changes: 92 additions & 0 deletions app/api/elevation/route.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
import { NextRequest, NextResponse } from 'next/server';
import { generateCoordinateGrid, decodeMapboxTerrainRGB, getElevationStats } from '@/lib/utils/elevation';
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

Comment on lines +1 to +3

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

decodeMapboxTerrainRGB is imported but unused in this route. More importantly, this route uses the contour layer’s ele property, not Terrain-RGB decoding. Either remove the unused import, or (preferably) switch to an approach consistent with the intended Terrain-RGB decoding utility.

Leaving this mismatch increases confusion and makes it harder to reason about correctness.

Suggestion

Either remove decodeMapboxTerrainRGB from the imports or implement the Terrain-RGB sampling path and use the decoder.

If keeping Tilequery/contours, tighten naming/comments so the API clearly returns contour-based elevations.

Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this suggestion.

export const runtime = 'edge';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

runtime = 'edge' is ill-suited for a route that fans out to 441+ sequential HTTP calls.

fetchElevationData processes up to 441 Mapbox Tilequery calls in sequential batches. Edge functions must begin sending a response within 25 seconds, and Vercel explicitly recommends migrating from edge to Node.js for improved performance and reliability. Fluid Compute (Node.js runtime) supports maximum durations up to 800 seconds on Pro/Enterprise, while Fluid Compute is currently only supported by the Node.js and Python runtimes. This route has no edge-specific requirements (no geo-routing, no sub-millisecond header inspection), so the edge constraint only adds risk.

🛠️ Proposed fix
-export const runtime = 'edge';
+// Use default Node.js runtime for long-running Mapbox fan-out requests
🤖 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 4, The route currently exports runtime =
'edge' which is unsuitable because fetchElevationData performs up to 441
sequential Mapbox Tilequery calls and can exceed edge function time limits;
change the runtime to a Node.js server runtime by removing or replacing the
export of runtime = 'edge' so the route runs on the Node.js environment (e.g.,
remove the export or set runtime to a Node-compatible value per your platform),
ensuring fetchElevationData and the route handler execute under the Node.js
runtime to allow longer execution times and improved reliability.


export async function GET(req: NextRequest) {
try {
const { searchParams } = new URL(req.url);
const boundsParam = searchParams.get('bounds');
const gridSizeParam = searchParams.get('gridSize');

if (!boundsParam) {
return NextResponse.json(
{ error: 'Missing required parameter: bounds' },
{ status: 400 }
);
}

const bounds = JSON.parse(boundsParam) as [number, number, number, number]; // [west, south, east, north]
const gridSize = gridSizeParam ? parseInt(gridSizeParam) : 20;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

JSON.parse on untrusted query param without targeted error handling; no validation on parsed values.

If boundsParam is malformed JSON, the outer catch returns a generic 500 instead of a 400. Additionally, neither bounds nor gridSize are validated after parsing — a gridSize of 0 causes division-by-zero in generateCoordinateGrid, and a very large value generates an enormous number of outbound HTTP requests (e.g., gridSize=100 → 10,201 Mapbox API calls).

Proposed fix
+    let bounds: [number, number, number, number];
+    try {
+      bounds = JSON.parse(boundsParam);
+    } catch {
+      return NextResponse.json({ error: 'Invalid bounds format' }, { status: 400 });
+    }
+    if (!Array.isArray(bounds) || bounds.length !== 4 || bounds.some(v => typeof v !== 'number' || !isFinite(v))) {
+      return NextResponse.json({ error: 'bounds must be an array of 4 finite numbers [west, south, east, north]' }, { status: 400 });
+    }
-    const bounds = JSON.parse(boundsParam) as [number, number, number, number];
-    const gridSize = gridSizeParam ? parseInt(gridSizeParam) : 20;
+    const gridSize = gridSizeParam ? parseInt(gridSizeParam, 10) : 20;
+    if (!Number.isFinite(gridSize) || gridSize < 2 || gridSize > 50) {
+      return NextResponse.json({ error: 'gridSize must be an integer between 2 and 50' }, { 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.

Suggested change
const bounds = JSON.parse(boundsParam) as [number, number, number, number]; // [west, south, east, north]
const gridSize = gridSizeParam ? parseInt(gridSizeParam) : 20;
let bounds: [number, number, number, number];
try {
bounds = JSON.parse(boundsParam);
} catch {
return NextResponse.json({ error: 'Invalid bounds format' }, { status: 400 });
}
if (!Array.isArray(bounds) || bounds.length !== 4 || bounds.some(v => typeof v !== 'number' || !isFinite(v))) {
return NextResponse.json({ error: 'bounds must be an array of 4 finite numbers [west, south, east, north]' }, { status: 400 });
}
const gridSize = gridSizeParam ? parseInt(gridSizeParam, 10) : 20;
if (!Number.isFinite(gridSize) || gridSize < 2 || gridSize > 50) {
return NextResponse.json({ error: 'gridSize must be an integer between 2 and 50' }, { 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 19 - 20, Parse boundsParam with a
targeted try/catch and return a 400 for malformed JSON, then validate the parsed
bounds array (ensure it's an array of four finite numbers in
[west,south,east,north] and west<east, south<north) before using it; parse
gridSizeParam into an integer and enforce a sane range (e.g., >0 and a max cap
like 50) or reject with 400 to avoid division-by-zero and massive request
fan-out; apply these checks in the same request handler that reads boundsParam
and gridSizeParam and before calling generateCoordinateGrid so invalid input is
rejected early.


Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bounds is parsed via JSON.parse(boundsParam) without validation. If a client sends invalid JSON, you’ll throw into the outer catch and return a 500, but this is a client error and should return 400.

Separately, gridSize is not clamped. Large values can explode request count ((gridSize+1)^2) and generate hundreds/thousands of external calls, which is a DoS vector against your own Edge function and your Mapbox quota.

Suggestion

Treat bad inputs as 400s and clamp gridSize to a sane range:

let bounds: [number, number, number, number];
try {
  const parsed = JSON.parse(boundsParam);
  if (!Array.isArray(parsed) || parsed.length !== 4 || parsed.some(n => typeof n !== 'number')) {
    return NextResponse.json({ error: 'Invalid bounds' }, { status: 400 });
  }
  bounds = parsed as [number, number, number, number];
} catch {
  return NextResponse.json({ error: 'Invalid bounds JSON' }, { status: 400 });
}

const requestedGridSize = gridSizeParam ? Number.parseInt(gridSizeParam, 10) : 20;
const gridSize = Number.isFinite(requestedGridSize)
  ? Math.min(50, Math.max(5, requestedGridSize))
  : 20;

Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this suggestion.

// Generate grid points
// generateCoordinateGrid expects bounds: [west, south, east, north]
// But check the implementation I just wrote:
// export function generateCoordinateGrid(bounds: [number, number, number, number], gridSize: number = 20)
// inside: const [west, south, east, north] = bounds;
// Implementation seems consistent.

const points = generateCoordinateGrid(bounds, gridSize);

// Fetch elevation data
const mapboxToken = process.env.NEXT_PUBLIC_MAPBOX_ACCESS_TOKEN;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
if (!mapboxToken) {
return NextResponse.json(
{ error: 'Mapbox access token not configured' },
{ status: 500 }
);
}

// Mapbox Tilequery API allows multiple coordinates? No, it's one by one or comma separated for features, but for terrain-rgb tilequery usually returns value at point.
// The user snippet did fetch per point. I will do parallel fetch with limit.
// Since runtime is edge, we can do parallel fetches.

// Batching strategy: Mapbox Tilequery API supports multiple queries?
// "Retrieve features from vector tiles based on a longitude and latitude."
// It doesn't support batch points in one request. We must make N requests.
// N = 20*20 = 400 requests. This might be too many for browser fetch limit or edge limit.
// But let's try. Maybe reduce grid size default if needed. 400 is a lot.
// User snippet uses 20x20.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

// Limit concurrency to avoid hitting rate limits or fetch errors
const batchSize = 10;
const results = [];

for (let i = 0; i < points.length; i += batchSize) {
const batch = points.slice(i, i + batchSize);
const batchPromises = batch.map(async (point) => {
try {
const url = `https://api.mapbox.com/v4/mapbox.mapbox-terrain-v2/tilequery/${point.lng},${point.lat}.json?layers=contour&limit=1&access_token=${mapboxToken}`;

const response = await fetch(url);
if (!response.ok) return { ...point, elevation: 0 };

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Failed/rate-limited requests silently default to elevation: 0, corrupting heatmap data.

Line 62 defaults non-OK responses to elevation: 0, and line 77's filter (p.elevation !== undefined) is a no-op since every point always has an elevation value. This means HTTP failures or rate-limit responses produce sea-level elevation values that are indistinguishable from real data, misleading users.

Either mark failed points distinctly (e.g., elevation: null) and filter them out, or propagate the error.

Proposed fix
-          if (!response.ok) return { ...point, elevation: 0 };
+          if (!response.ok) return { ...point, elevation: null };
           // ...
         } catch (e) {
           console.error(e);
-          return { ...point, elevation: 0 };
+          return { ...point, elevation: null };
         }

     // ...
-    const validPoints = results.filter(p => p.elevation !== undefined);
+    const validPoints = results.filter(p => p.elevation !== null && p.elevation !== undefined);

Also applies to: 77-77

🤖 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 62, The code currently sets failed/non-OK
fetches to elevation: 0 which corrupts results; change the non-OK branch that
returns { ...point, elevation: 0 } to return { ...point, elevation: null }
instead, and update the later filter from p.elevation !== undefined to
explicitly exclude null (e.g., p.elevation !== null or p.elevation != null
depending on desired semantics) so rate-limited/failed points are omitted from
the heatmap; ensure any downstream code that assumes a numeric elevation handles
nullable elevations or throws/propagates errors as appropriate (references: the
response variable used in the fetch branch and the filter using p.elevation !==
undefined).


const data = await response.json();
const elevation = data.features && data.features.length > 0 ? data.features[0].properties?.ele : 0;
return { ...point, elevation };
} catch (e) {
console.error(e);
return { ...point, elevation: 0 };
}
});

const batchResults = await Promise.all(batchPromises);
results.push(...batchResults);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

const validPoints = results.filter(p => p.elevation !== undefined);
const statistics = getElevationStats(validPoints);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This route makes up to (gridSize+1)^2 Tilequery requests (default 441) per call. Even with batching at 10 concurrency, that’s still extremely heavy for an Edge function and likely to hit Mapbox rate limits / timeouts. Returning elevation: 0 on failures also silently pollutes stats and the heat map.

You need either a different data source (e.g., raster terrain-rgb tiles sampled client/server-side) or a strategy that drastically reduces request count (adaptive sampling, smaller default, early exit, caching). At minimum, distinguish failed samples from real 0m elevation and exclude them from stats/visualization.

Suggestion

Minimum viable mitigation:

  1. Mark failures as null instead of 0 and filter them out of stats.
  2. Lower default gridSize (e.g., 10) and enforce max.
  3. Add caching headers and/or server-side caching (KV/Upstash) keyed by bounds+gridSize.

Example adjustment:

if (!response.ok) return { ...point, elevation: null };
// ...
const elevation = /* extracted */;
return { ...point, elevation: typeof elevation === 'number' ? elevation : null };

const validPoints = results.filter(p => typeof p.elevation === 'number');

Longer-term: consider sampling Mapbox terrain-rgb raster tiles (1 request per tile, many points per tile) instead of Tilequery per point.

Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this suggestion.

return NextResponse.json({
points: validPoints,
statistics
});

} catch (error) {
console.error('Error fetching elevation data:', error);
return NextResponse.json(
{ error: 'Failed to fetch elevation data', details: error instanceof Error ? error.message : 'Unknown error' },
{ status: 500 }
);
}
}
215 changes: 215 additions & 0 deletions components/map/elevation-heatmap-layer.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,215 @@
'use client'

import { useEffect, useRef } from 'react'
import mapboxgl from 'mapbox-gl'
import { useMap } from './map-context'
import { getElevationStats } from '@/lib/utils/elevation'

interface ElevationPoint {
lat: number;
lng: number;
elevation: number;
}
Comment on lines +8 to +12

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Duplicate ElevationPoint interface — import from lib/utils/elevation.ts instead.

The same interface is already exported from lib/utils/elevation.ts (and getElevationStats is already imported from there on line 6). Keeping a local copy risks drift.

Proposed fix
-import { getElevationStats } from '@/lib/utils/elevation'
-
-interface ElevationPoint {
-  lat: number;
-  lng: number;
-  elevation: number;
-}
+import { getElevationStats, type ElevationPoint } from '@/lib/utils/elevation'
📝 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.

Suggested change
interface ElevationPoint {
lat: number;
lng: number;
elevation: number;
}
import { getElevationStats, type ElevationPoint } from '@/lib/utils/elevation'
🤖 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 8 - 12, Remove the
locally defined ElevationPoint interface and import it from the shared module
where getElevationStats is already imported; specifically, delete the local
`interface ElevationPoint { lat: number; lng: number; elevation: number; }` and
add `ElevationPoint` to the existing import statement that brings in
`getElevationStats`, then update any type references in this file to use the
imported `ElevationPoint` to avoid duplication and drift.


interface ElevationHeatmapLayerProps {
id?: string;
points: ElevationPoint[];
statistics?: {
min: number;
max: number;
average: number;
count: number;
};
}

export function ElevationHeatmapLayer({ id = 'elevation', points, statistics }: ElevationHeatmapLayerProps) {
const { map } = useMap()
Comment on lines +25 to +26

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Effect dependency on points and statistics objects causes teardown/rebuild every render.

points is an array and statistics is an object — unless the parent memoizes these, they'll be new references each render, triggering the full layer teardown and re-add cycle (flicker). Consider comparing serialized data or using a ref-based dirty check to short-circuit when the actual data hasn't changed.

Also applies to: 212-212

🤖 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 25 - 26, The effect
in ElevationHeatmapLayer is re-running each render because the dependencies
`points` and `statistics` are new object/array references; change the effect to
depend on stable primitive keys instead by computing a deterministic signature
(e.g. JSON.stringify or a lightweight hash) for `points` and `statistics` inside
the component (or useMemo) and store previous signatures in refs (e.g.
prevPointsKey, prevStatsKey) to short-circuit teardown/rebuild when signatures
are equal; update the map layer only when the computed signature changes,
referencing the ElevationHeatmapLayer component and its internal effect that
currently watches `points`/`statistics`.

const sourceId = `elevation-heatmap-source-${id}`
const heatmapLayerId = `elevation-heatmap-layer-${id}`
const pointsLayerId = `elevation-points-layer-${id}`

// Keep track of layers added to remove them cleanly
const layersAdded = useRef(false);

useEffect(() => {
if (!map || !points || points.length === 0) return

// Calculate stats if not provided
const stats = statistics || getElevationStats(points);
const { min, max } = stats;

// Normalize elevation for heat map intensity (0-1 scale)
const intensity = (val: number) => (max === min ? 0.5 : (val - min) / (max - min));

// Convert points to GeoJSON
const geojson: GeoJSON.FeatureCollection = {
type: 'FeatureCollection',
features: points.map(point => ({
type: 'Feature',
geometry: {
type: 'Point',
coordinates: [point.lng, point.lat]
},
properties: {
elevation: point.elevation,
intensity: intensity(point.elevation)
}
}))
}

const addLayers = () => {
if (map.getSource(sourceId)) {
// Update data if source exists
(map.getSource(sourceId) as mapboxgl.GeoJSONSource).setData(geojson);
} else {
// Add the data source
map.addSource(sourceId, {
type: 'geojson',
data: geojson
})
}

// Add heatmap layer if not exists
if (!map.getLayer(heatmapLayerId)) {
map.addLayer({
id: heatmapLayerId,
type: 'heatmap',
source: sourceId,
maxzoom: 15,
paint: {
// Increase weight based on elevation intensity
'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 proxy)
// Blue (low) -> Green (mid) -> Yellow (high) -> Red (very 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
]
}
})
layersAdded.current = true;
}

// Add circle layer for high zoom levels if not exists
if (!map.getLayer(pointsLayerId)) {
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
]
}
})
layersAdded.current = true;

// Popup logic
map.on('click', pointsLayerId, (e) => {
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.toFixed(1)}m`)
.addTo(map)
}
Comment on lines +173 to +180

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The popup HTML is built via setHTML with interpolated data. While elevation is expected to be numeric, feature.properties ultimately comes from GeoJSON and Mapbox internals; using setHTML is a footgun for future changes (e.g., adding names/labels) and makes XSS easier to accidentally introduce.

Prefer setText for simple content, or sanitize/escape if you truly need HTML markup.

Suggestion

Use setText (or build DOM nodes) instead of setHTML:

new mapboxgl.Popup()
  .setLngLat(e.lngLat)
  .setText(`Elevation: ${Number(elevation).toFixed(1)}m`)
  .addTo(map);

Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.

})

// Cursor logic
map.on('mouseenter', pointsLayerId, () => {
map.getCanvas().style.cursor = 'pointer'
})

map.on('mouseleave', pointsLayerId, () => {
map.getCanvas().style.cursor = ''
})
}
Comment on lines +169 to +191

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Event listeners are never removed — memory/handler leak on every effect re-run.

map.on('click', ...), map.on('mouseenter', ...), and map.on('mouseleave', ...) are registered each time addLayers runs but are never cleaned up in the useEffect return. Since the effect depends on points (a new array reference each render unless memoized), this accumulates duplicate handlers.

Store the handler references and call map.off(...) in the cleanup function.

Proposed fix sketch
+  const handlersRef = useRef<{
+    click?: (e: mapboxgl.MapMouseEvent & { features?: mapboxgl.MapboxGeoJSONFeature[] }) => void;
+    mouseenter?: () => void;
+    mouseleave?: () => void;
+  }>({});

   // ... inside addLayers, after adding the points layer:

-        map.on('click', pointsLayerId, (e) => { ... })
-        map.on('mouseenter', pointsLayerId, () => { ... })
-        map.on('mouseleave', pointsLayerId, () => { ... })
+        const onClick = (e: mapboxgl.MapMouseEvent & { features?: mapboxgl.MapboxGeoJSONFeature[] }) => {
+          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.toFixed(1)}m`)
+              .addTo(map)
+          }
+        }
+        const onMouseEnter = () => { map.getCanvas().style.cursor = 'pointer' }
+        const onMouseLeave = () => { map.getCanvas().style.cursor = '' }
+        handlersRef.current = { click: onClick, mouseenter: onMouseEnter, mouseleave: onMouseLeave }
+        map.on('click', pointsLayerId, onClick)
+        map.on('mouseenter', pointsLayerId, onMouseEnter)
+        map.on('mouseleave', pointsLayerId, onMouseLeave)

   // ... in cleanup:
+      if (handlersRef.current.click) map.off('click', pointsLayerId, handlersRef.current.click)
+      if (handlersRef.current.mouseenter) map.off('mouseenter', pointsLayerId, handlersRef.current.mouseenter)
+      if (handlersRef.current.mouseleave) map.off('mouseleave', pointsLayerId, handlersRef.current.mouseleave)
🤖 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 169 - 191, The event
listeners registered inside addLayers (the map.on('click', pointsLayerId, ...),
map.on('mouseenter', pointsLayerId, ...), and map.on('mouseleave',
pointsLayerId, ...)) cause handler leaks because they are not removed when the
useEffect re-runs; capture each listener callback in a const (e.g.,
clickHandler, enterHandler, leaveHandler) when you call map.on, and in the
useEffect cleanup call map.off(pointsLayerId, 'click', clickHandler),
map.off(pointsLayerId, 'mouseenter', enterHandler), and map.off(pointsLayerId,
'mouseleave', leaveHandler) (or map.off(event, handler) matching your mapbox
API), ensuring you remove the exact functions added by addLayers before unmount
or re-run; keep references local to the effect so they match the registered
handlers.

}

if (map.isStyleLoaded()) {
addLayers()
} else {
map.on('load', addLayers)
}
Comment on lines +194 to +198

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

map.on('load', addLayers) is not removed in cleanup — can fire after unmount.

If the map style hasn't loaded when the effect runs, addLayers is registered on 'load'. If the component unmounts before the style loads, the callback fires on a stale closure and attempts to add layers/sources to a map that may no longer expect them.

Proposed fix
     if (map.isStyleLoaded()) {
       addLayers()
     } else {
       map.on('load', addLayers)
     }

     return () => {
+      map.off('load', addLayers)
       if (map.isStyleLoaded()) {
         if (map.getLayer(pointsLayerId)) map.removeLayer(pointsLayerId)
         if (map.getLayer(heatmapLayerId)) map.removeLayer(heatmapLayerId)
         if (map.getSource(sourceId)) map.removeSource(sourceId)
       }
       layersAdded.current = false;
     }
📝 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.

Suggested change
if (map.isStyleLoaded()) {
addLayers()
} else {
map.on('load', addLayers)
}
if (map.isStyleLoaded()) {
addLayers()
} else {
map.on('load', addLayers)
}
return () => {
map.off('load', addLayers)
if (map.isStyleLoaded()) {
if (map.getLayer(pointsLayerId)) map.removeLayer(pointsLayerId)
if (map.getLayer(heatmapLayerId)) map.removeLayer(heatmapLayerId)
if (map.getSource(sourceId)) map.removeSource(sourceId)
}
layersAdded.current = false;
}
🤖 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 194 - 198, The
effect registers addLayers via map.on('load', addLayers) when
map.isStyleLoaded() is false but never removes that listener on unmount; update
the effect cleanup to call map.off('load', addLayers) (and only if the listener
was attached) so the callback cannot fire after unmount, and keep existing
cleanup that removes sources/layers added by addLayers to avoid stale operations
on the map.


// Cleanup handled by return function of useEffect
return () => {
// We don't remove source immediately to avoid flickering if component re-renders quickly
// But for clean unmount we should.
// However, usually we want layers to persist until explicitly removed or component unmounted.
if (map.isStyleLoaded()) {
if (map.getLayer(pointsLayerId)) map.removeLayer(pointsLayerId)
if (map.getLayer(heatmapLayerId)) map.removeLayer(heatmapLayerId)
if (map.getSource(sourceId)) map.removeSource(sourceId)
}
Comment on lines +168 to +209

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Event handlers are attached with map.on('click' | 'mouseenter' | 'mouseleave', pointsLayerId, ...) but never detached. Removing the layer/source on cleanup does not necessarily remove delegated handlers in Mapbox GL; over time (re-renders / style reloads) this can leak handlers and cause duplicate popups/cursor changes.

Also, when map.isStyleLoaded() is false you attach map.on('load', addLayers) but do not remove that listener on cleanup, which can also lead to duplicate layer additions.

Suggestion

Store handler functions and unregister them in cleanup, and also unregister the load listener:

const onClick = (e: mapboxgl.MapLayerMouseEvent) => { /* ... */ };
const onEnter = () => { map.getCanvas().style.cursor = 'pointer'; };
const onLeave = () => { map.getCanvas().style.cursor = ''; };

const onLoad = () => addLayers();

if (map.isStyleLoaded()) addLayers();
else map.on('load', onLoad);

map.on('click', pointsLayerId, onClick);
map.on('mouseenter', pointsLayerId, onEnter);
map.on('mouseleave', pointsLayerId, onLeave);

return () => {
  map.off('load', onLoad);
  map.off('click', pointsLayerId, onClick);
  map.off('mouseenter', pointsLayerId, onEnter);
  map.off('mouseleave', pointsLayerId, onLeave);
  // then remove layers/sources
};

Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this suggestion.

layersAdded.current = false;
}
}, [map, id, points, statistics]) // Re-run if points change

return null
}
10 changes: 8 additions & 2 deletions lib/agents/resolution-search.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,12 @@ const resolutionSearchSchema = z.object({
cogInfo: z.object({
applicable: z.boolean(),
description: z.string().optional()
}).optional().describe('Information about whether Cloud Optimized GeoTIFF (COG) data is applicable or available for this area.')
}).optional().describe('Information about whether Cloud Optimized GeoTIFF (COG) data is applicable or available for this area.'),
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'),
}).optional().describe('Elevation heat map configuration for the analyzed area'),
Comment on lines +32 to +36

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

bounds array lacks length constraint; gridSize lacks min/max guard.

The bounds schema accepts any numeric array, but downstream code (generateCoordinateGrid, the API route) destructures exactly four elements [west, south, east, north]. A malformed array silently produces wrong results. Similarly, gridSize of 0 causes division-by-zero in generateCoordinateGrid, and very large values (e.g., 1000) would generate ~1M API calls in the elevation route.

Proposed fix: constrain the schema
   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'),
+    bounds: z.array(z.number()).length(4).optional().describe('Bounding box for elevation query [west, south, east, north]'),
+    gridSize: z.number().int().min(2).max(50).optional().describe('Grid resolution for elevation sampling (2-50)'),
   }).optional().describe('Elevation heat map configuration for the analyzed area'),
📝 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.

Suggested change
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'),
}).optional().describe('Elevation heat map configuration for the analyzed area'),
elevationData: z.object({
requested: z.boolean().describe('Whether elevation heat map was requested or appropriate for this analysis'),
bounds: z.array(z.number()).length(4).optional().describe('Bounding box for elevation query [west, south, east, north]'),
gridSize: z.number().int().min(2).max(50).optional().describe('Grid resolution for elevation sampling (2-50)'),
}).optional().describe('Elevation heat map configuration for the analyzed area'),
🤖 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 allows any-length numeric arrays and unbounded gridSize, which leads to
downstream breakage in generateCoordinateGrid (it expects [west, south, east,
north]) and division-by-zero or extreme work for gridSize; update the
elevationData.bounds to require exactly four numbers (use a fixed-length tuple
or .min(4).max(4)/.length(4) on the array) and make gridSize an integer with a
safe range (e.g., .int().min(1).max(200) or another capped maximum) so callers
are validated before generateCoordinateGrid and the elevation API route are
invoked. Ensure references are applied to the elevationData schema fields named
bounds and gridSize so invalid input is rejected early.

})

export interface DrawnFeature {
Expand Down Expand Up @@ -66,7 +71,8 @@ Your analysis should be comprehensive and include the following components:
2. **Points of Interest (POI):** Detect and name any significant landmarks, infrastructure (e.g., bridges, major roads), or notable buildings.
3. **Coordinate Extraction:** If possible, confirm or refine the geocoordinates (latitude/longitude) of the center of the image.
4. **COG Applicability:** Determine if this location would benefit from Cloud Optimized GeoTIFF (COG) analysis for high-precision temporal or spectral data.
5. **Structured Output:** Return your findings in a structured JSON format. The output must include a 'summary' (a detailed text description of your analysis) and a 'geoJson' object.
5. **Elevation Heat Map:** If the user has drawn polygons or if terrain analysis would be valuable, indicate that elevation data should be fetched by setting elevationData.requested to true and providing appropriate bounds.
6. **Structured Output:** Return your findings in a structured JSON format. The output must include a 'summary' (a detailed text description of your analysis) and a 'geoJson' object.

Your analysis should be based solely on the visual information in the image and your general knowledge. Do not attempt to access external websites or perform web searches.

Expand Down
Loading