Fix: Improve robustness of location enrichment feature #323
Conversation
This commit introduces a new asynchronous agent, `geojsonEnricher`, responsible for processing textual responses and extracting structured geospatial data. Key changes: - Created a new `geojsonEnricher` agent in `lib/agents/geojson-enricher.tsx`. - Defined new TypeScript interfaces for `LocationResponse`, `MapCommand`, and GeoJSON objects in `lib/types/custom.ts`. - Integrated the new agent into the main application workflow in `app/actions.tsx`. - Added a `LocationResponseHandler` component to update the map state via the `MapDataContext`. - Updated the `Mapbox` component to render GeoJSON data and execute map commands.
This commit introduces a new asynchronous agent, `geojsonEnricher`, responsible for processing textual responses and extracting structured geospatial data. Key changes: - Created a new `geojsonEnricher` agent in `lib/agents/geojson-enricher.tsx`. - Defined new TypeScript interfaces for `LocationResponse`, `MapCommand`, and GeoJSON objects in `lib/types/custom.ts`. - Integrated the new agent into the main application workflow in `app/actions.tsx` with robust error handling. - Added a `LocationResponseHandler` component to update the map state via the `MapDataContext`. - Updated the `Mapbox` component to render GeoJSON data and execute map commands. NOTE: The application build is currently failing due to a persistent 'JavaScript heap out of memory' error. This appears to be a pre-existing issue with the repository's dependencies or build configuration that could not be resolved. The feature code is complete, but the build itself will need to be addressed separately.
…nt and map display feature, improving its robustness, correctness, and consistency. The following changes were made: - **Consistent Tool Output:** Added `type: 'tool'` to the `geojsonEnrichment` tool output object in `app/actions.tsx` and the `LocationResponse` interface. This ensures all tool messages follow a consistent data structure, simplifying message filtering and state management. - **Runtime Validation:** Implemented a robust runtime type guard (`isLocationResponse`) to validate the `geojsonEnrichment` tool output before it is passed to the UI. This prevents rendering errors caused by malformed data from the AI model. The validation is applied in both the agent and the consuming action. - **Corrected `fitBounds` Call:** Ensured the `fitBounds` function in `components/map/mapbox-map.tsx` is called with the correct signature by destructuring the `bounds` and `options` parameters. - **Agent and Prompt Improvements:** - Fixed a syntax error in the import statement in `lib/agents/geojson-enricher.tsx`. - Updated the agent's prompt to explicitly request raw JSON output, reducing the likelihood of parsing errors from markdown-wrapped responses. - Added logic to strip markdown code fences from the AI's response before parsing. - **Build Fix:** Corrected an invalid import path in `mapbox_mcp/hooks.ts` that was causing the build to fail.
|
👋 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 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.
|
WalkthroughIntroduces GeoJSON/location-aware enrichment into the submit flow, new map-related types and runtime guards, a LocationResponse handler component, Mapbox effects for GeoJSON and map commands, a map data context extension, a new geojsonEnricher agent, a minor MCP import path change, and dependency/build script updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant A as submit(action)
participant R as Researcher/Model
participant G as geojsonEnricher
participant UI as UI Renderer
participant H as LocationResponseHandler
participant C as MapDataContext
participant M as MapboxMap
U->>A: Submit query
A->>R: Get researcher response
par Race (30s timeout)
A->>G: Enrich to LocationResponse
A-->>A: 30s timeout
end
alt Enrichment success
A->>A: Add toolOutput: geojsonEnrichment
A->>UI: Use locationResponse.text as assistant content
else Timeout/Failure
A->>A: Fallback to location-agnostic response
end
UI->>H: Pass LocationResponse tool output
H->>C: setMapData({ geojson, mapCommands })
C-->>M: mapData changes
alt geojson present
M->>M: Update/attach source & layers
else no geojson
M->>M: Remove layers/source if exist
end
alt mapCommands present
M->>M: Execute flyTo/easeTo/fitBounds
M->>C: Clear mapCommands
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
Overall: Nice improvement to robustness and reliability. Resolve the outstanding build issue at the infra/dependency level, and consider minimal tests for runtime guards and agent edge cases. |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||||||||
…g when processing location-based queries. It also includes the original set of fixes for improving the robustness and correctness of the location enrichment feature. Key changes include: - **Timeout for GeoJSON Enrichment:** Implemented a 30-second timeout for the `geojsonEnricher` agent call in `app/actions.tsx`. If the agent is unresponsive, the system will now gracefully fall back to a standard text response instead of freezing. - **Consistent Tool Output:** Added `type: 'tool'` to the `geojsonEnrichment` tool output object and the `LocationResponse` interface, ensuring a consistent data structure across all tool messages. - **Runtime Validation:** Implemented a robust runtime type guard (`isLocationResponse`) to validate the `geojsonEnrichment` tool output before it is passed to the UI, preventing rendering errors from malformed data. - **Corrected `fitBounds` Call:** Ensured the `fitBounds` function in `components/map/mapbox-map.tsx` is called with the correct signature by destructuring the `bounds` and `options` parameters. - **Agent and Prompt Improvements:** - Fixed syntax and import errors in `lib/agents/geojson-enricher.tsx` and `mapbox_mcp/hooks.ts`. - Updated the agent's prompt to explicitly request raw JSON output. - Added logic to strip markdown code fences from the AI's response before parsing.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
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)
551-560: Fix Biome noSwitchDeclarations: wrap declaration in a blockVariable declared in a switch case must be scoped with braces to avoid cross‑case access.
As per static analysis hints
- case 'related': - const relatedQueries = createStreamableValue<PartialRelated>() - relatedQueries.done(JSON.parse(content as string)) - return { - id, - component: ( - <Section title="Related" separator={true}> - <SearchRelated relatedQueries={relatedQueries.value} /> - </Section> - ) - } + case 'related': { + const relatedQueries = createStreamableValue<PartialRelated>() + relatedQueries.done(JSON.parse(content as string)) + return { + id, + component: ( + <Section title="Related" separator={true}> + <SearchRelated relatedQueries={relatedQueries.value} /> + </Section> + ) + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (3)
bun.lockbis excluded by!**/bun.lockbdev.logis excluded by!**/*.logpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
app/actions.tsx(7 hunks)components/map/location-response-handler.tsx(1 hunks)components/map/map-data-context.tsx(2 hunks)components/map/mapbox-map.tsx(1 hunks)lib/agents/geojson-enricher.tsx(1 hunks)lib/types/custom.ts(1 hunks)lib/utils/type-guards.ts(1 hunks)mapbox_mcp/hooks.ts(1 hunks)package.json(4 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
app/actions.tsx
[error] 551-551: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
components/map/mapbox-map.tsx
[error] 595-595: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (5)
components/map/map-data-context.tsx (1)
5-8: New GeoJSON/command types import — LGTM.Importing
GeoJSONFeatureCollectionandMapCommandaligns with the PR’s new features.package.json (1)
87-89: New deps (uuid,zod) — verify import styles.For
uuid@^9, ensure ESM import usage (e.g.,import { v4 as uuidv4 } from 'uuid').zodaddition aligns with runtime validation goals.lib/agents/geojson-enricher.tsx (1)
50-58:streamTextreturns both.textand.textStream
ThestreamTextresult exposes a.textPromise that resolves the full response, solet responseText = await text;is correct. UsegenerateTextonly if you want a simpler non-streaming API.Likely an incorrect or invalid review comment.
app/actions.tsx (1)
589-606: GeoJSON tool rendering path looks correctRuntime-validate with isLocationResponse, render handler on success, skip with warning on invalid data. Good guard-before-render pattern.
Consider gating console warnings behind a debug flag to avoid noisy logs in production.
components/map/location-response-handler.tsx (1)
14-23: map_commands update correctly guarded – undefined preserves previous mapCommands and null clears commands in map-data-context.
| let locationResponse: LocationResponse; | ||
| try { | ||
| // Create a timeout promise that rejects after 30 seconds. | ||
| const timeoutPromise = new Promise((_, reject) => | ||
| setTimeout(() => reject(new Error('GeoJSON enrichment timed out after 30 seconds')), 30000) | ||
| ); | ||
|
|
||
| // Race the agent call against the timeout. | ||
| locationResponse = (await Promise.race([ | ||
| geojsonEnricher(answer), | ||
| timeoutPromise | ||
| ])) as LocationResponse; | ||
| } catch (e) { | ||
| console.error("Error during geojson enrichment:", e); | ||
| // Fallback to a response without location data, ensuring consistent structure. | ||
| locationResponse = { | ||
| type: 'tool', | ||
| text: answer, | ||
| geojson: null, | ||
| map_commands: null, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Clear timeout after Promise.race to avoid leaked timer/dangling event loop
The timeout isn’t cleared if enrichment resolves quickly. Clear it in a finally block.
Apply this diff:
- let locationResponse: LocationResponse;
+ let locationResponse: LocationResponse;
+ let timeoutId: ReturnType<typeof setTimeout> | undefined;
try {
- // Create a timeout promise that rejects after 30 seconds.
- const timeoutPromise = new Promise((_, reject) =>
- setTimeout(() => reject(new Error('GeoJSON enrichment timed out after 30 seconds')), 30000)
- );
+ // Create a timeout promise that rejects after 30 seconds.
+ const timeoutPromise = new Promise((_, reject) => {
+ timeoutId = setTimeout(
+ () => reject(new Error('GeoJSON enrichment timed out after 30 seconds')),
+ 30_000
+ );
+ });
// Race the agent call against the timeout.
locationResponse = (await Promise.race([
geojsonEnricher(answer),
timeoutPromise
])) as LocationResponse;
} catch (e) {
console.error("Error during geojson enrichment:", e);
// Fallback to a response without location data, ensuring consistent structure.
locationResponse = {
type: 'tool',
text: answer,
geojson: null,
map_commands: null,
};
- }
+ } finally {
+ if (timeoutId) clearTimeout(timeoutId);
+ }🤖 Prompt for AI Agents
In app/actions.tsx around lines 299 to 320, the timeout created for the 30s
Promise.race is not cleared if geojsonEnricher resolves quickly, leaking a
timer; store the setTimeout return value in a variable (typed appropriately),
build the timeoutPromise using that timer id, and ensure you call
clearTimeout(timerId) in a finally block after the Promise.race finishes (both
success and error paths) so the timer is cancelled and no dangling event-loop
callback remains.
| geojson?: GeoJSONFeatureCollection | null; | ||
| mapCommands?: MapCommand[] | null; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add defaults for new fields to avoid undefined checks.
Optional: initialize geojson and mapCommands in state to null to simplify consumer logic and avoid undefined/tri-state handling.
Example:
const [mapData, setMapData] = useState<MapData>({
drawnFeatures: [],
geojson: null,
mapCommands: null,
});🤖 Prompt for AI Agents
In components/map/map-data-context.tsx around lines 21 to 23, the MapData type
allows geojson and mapCommands to be optional which forces consumers to handle
undefined/tri-state; update the context state initialization to set explicit
defaults (initialize geojson and mapCommands to null and drawnFeatures to an
empty array) so consumers can rely on null instead of undefined, and update the
useState<MapData> call to supply these defaults when creating the initial
mapData state.
| // Effect to handle GeoJSON data updates | ||
| useEffect(() => { | ||
| if (!map.current) return; | ||
|
|
||
| const mapInstance = map.current; | ||
| const source = mapInstance.getSource('geojson-data'); | ||
|
|
||
| // If GeoJSON data is present, add or update the source and layers | ||
| if (mapData.geojson) { | ||
| if (source) { | ||
| (source as mapboxgl.GeoJSONSource).setData(mapData.geojson as any); | ||
| } else { | ||
| mapInstance.addSource('geojson-data', { | ||
| type: 'geojson', | ||
| data: mapData.geojson as any, | ||
| }); | ||
|
|
||
| // Add layer for points | ||
| mapInstance.addLayer({ | ||
| id: 'geojson-points', | ||
| type: 'circle', | ||
| source: 'geojson-data', | ||
| paint: { | ||
| 'circle-radius': 8, | ||
| 'circle-color': '#007cbf', | ||
| 'circle-stroke-width': 2, | ||
| 'circle-stroke-color': '#ffffff', | ||
| }, | ||
| filter: ['==', '$type', 'Point'], | ||
| }); | ||
|
|
||
| // Add layer for lines | ||
| mapInstance.addLayer({ | ||
| id: 'geojson-lines', | ||
| type: 'line', | ||
| source: 'geojson-data', | ||
| paint: { | ||
| 'line-color': '#ff4500', | ||
| 'line-width': 3, | ||
| }, | ||
| filter: ['==', '$type', 'LineString'], | ||
| }); | ||
| } | ||
| } else { | ||
| // If no GeoJSON data, remove layers and source if they exist | ||
| if (mapInstance.getLayer('geojson-points')) mapInstance.removeLayer('geojson-points'); | ||
| if (mapInstance.getLayer('geojson-lines')) mapInstance.removeLayer('geojson-lines'); | ||
| if (source) mapInstance.removeSource('geojson-data'); | ||
| } | ||
| }, [mapData.geojson]); |
There was a problem hiding this comment.
Guard against adding sources/layers before style is loaded.
If this effect runs before the map style is loaded, addSource/addLayer can throw. Add an isStyleLoaded check (or register a one‑time load handler) before mutating the style.
Apply this diff:
useEffect(() => {
- if (!map.current) return;
+ if (!map.current) return;
const mapInstance = map.current;
+ // Ensure style is loaded before adding/removing sources/layers
+ if (!mapInstance.isStyleLoaded()) {
+ const onLoad = () => {
+ // re-run the logic once style is ready
+ const sourceAfterLoad = mapInstance.getSource('geojson-data');
+ if (mapData.geojson) {
+ if (sourceAfterLoad) {
+ (sourceAfterLoad as mapboxgl.GeoJSONSource).setData(mapData.geojson as any);
+ } else {
+ mapInstance.addSource('geojson-data', { type: 'geojson', data: mapData.geojson as any });
+ mapInstance.addLayer({
+ id: 'geojson-points',
+ type: 'circle',
+ source: 'geojson-data',
+ paint: { 'circle-radius': 8, 'circle-color': '#007cbf', 'circle-stroke-width': 2, 'circle-stroke-color': '#ffffff' },
+ filter: ['==', '$type', 'Point'],
+ });
+ mapInstance.addLayer({
+ id: 'geojson-lines',
+ type: 'line',
+ source: 'geojson-data',
+ paint: { 'line-color': '#ff4500', 'line-width': 3 },
+ filter: ['==', '$type', 'LineString'],
+ });
+ }
+ } else {
+ if (mapInstance.getLayer('geojson-points')) mapInstance.removeLayer('geojson-points');
+ if (mapInstance.getLayer('geojson-lines')) mapInstance.removeLayer('geojson-lines');
+ if (sourceAfterLoad) mapInstance.removeSource('geojson-data');
+ }
+ mapInstance.off('load', onLoad);
+ };
+ mapInstance.on('load', onLoad);
+ return;
+ }
const source = mapInstance.getSource('geojson-data');Optional: add a polygon fill layer if you expect Polygon features.
📝 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.
| // Effect to handle GeoJSON data updates | |
| useEffect(() => { | |
| if (!map.current) return; | |
| const mapInstance = map.current; | |
| const source = mapInstance.getSource('geojson-data'); | |
| // If GeoJSON data is present, add or update the source and layers | |
| if (mapData.geojson) { | |
| if (source) { | |
| (source as mapboxgl.GeoJSONSource).setData(mapData.geojson as any); | |
| } else { | |
| mapInstance.addSource('geojson-data', { | |
| type: 'geojson', | |
| data: mapData.geojson as any, | |
| }); | |
| // Add layer for points | |
| mapInstance.addLayer({ | |
| id: 'geojson-points', | |
| type: 'circle', | |
| source: 'geojson-data', | |
| paint: { | |
| 'circle-radius': 8, | |
| 'circle-color': '#007cbf', | |
| 'circle-stroke-width': 2, | |
| 'circle-stroke-color': '#ffffff', | |
| }, | |
| filter: ['==', '$type', 'Point'], | |
| }); | |
| // Add layer for lines | |
| mapInstance.addLayer({ | |
| id: 'geojson-lines', | |
| type: 'line', | |
| source: 'geojson-data', | |
| paint: { | |
| 'line-color': '#ff4500', | |
| 'line-width': 3, | |
| }, | |
| filter: ['==', '$type', 'LineString'], | |
| }); | |
| } | |
| } else { | |
| // If no GeoJSON data, remove layers and source if they exist | |
| if (mapInstance.getLayer('geojson-points')) mapInstance.removeLayer('geojson-points'); | |
| if (mapInstance.getLayer('geojson-lines')) mapInstance.removeLayer('geojson-lines'); | |
| if (source) mapInstance.removeSource('geojson-data'); | |
| } | |
| }, [mapData.geojson]); | |
| // Effect to handle GeoJSON data updates | |
| useEffect(() => { | |
| if (!map.current) return; | |
| const mapInstance = map.current; | |
| // Ensure style is loaded before adding/removing sources/layers | |
| if (!mapInstance.isStyleLoaded()) { | |
| const onLoad = () => { | |
| // re-run the logic once style is ready | |
| const sourceAfterLoad = mapInstance.getSource('geojson-data'); | |
| if (mapData.geojson) { | |
| if (sourceAfterLoad) { | |
| (sourceAfterLoad as mapboxgl.GeoJSONSource).setData(mapData.geojson as any); | |
| } else { | |
| mapInstance.addSource('geojson-data', { type: 'geojson', data: mapData.geojson as any }); | |
| mapInstance.addLayer({ | |
| id: 'geojson-points', | |
| type: 'circle', | |
| source: 'geojson-data', | |
| paint: { | |
| 'circle-radius': 8, | |
| 'circle-color': '#007cbf', | |
| 'circle-stroke-width': 2, | |
| 'circle-stroke-color': '#ffffff' | |
| }, | |
| filter: ['==', '$type', 'Point'], | |
| }); | |
| mapInstance.addLayer({ | |
| id: 'geojson-lines', | |
| type: 'line', | |
| source: 'geojson-data', | |
| paint: { | |
| 'line-color': '#ff4500', | |
| 'line-width': 3 | |
| }, | |
| filter: ['==', '$type', 'LineString'], | |
| }); | |
| } | |
| } else { | |
| if (mapInstance.getLayer('geojson-points')) mapInstance.removeLayer('geojson-points'); | |
| if (mapInstance.getLayer('geojson-lines')) mapInstance.removeLayer('geojson-lines'); | |
| if (sourceAfterLoad) mapInstance.removeSource('geojson-data'); | |
| } | |
| mapInstance.off('load', onLoad); | |
| }; | |
| mapInstance.on('load', onLoad); | |
| return; | |
| } | |
| const source = mapInstance.getSource('geojson-data'); | |
| // If GeoJSON data is present, add or update the source and layers | |
| if (mapData.geojson) { | |
| if (source) { | |
| (source as mapboxgl.GeoJSONSource).setData(mapData.geojson as any); | |
| } else { | |
| mapInstance.addSource('geojson-data', { | |
| type: 'geojson', | |
| data: mapData.geojson as any, | |
| }); | |
| // Add layer for points | |
| mapInstance.addLayer({ | |
| id: 'geojson-points', | |
| type: 'circle', | |
| source: 'geojson-data', | |
| paint: { | |
| 'circle-radius': 8, | |
| 'circle-color': '#007cbf', | |
| 'circle-stroke-width': 2, | |
| 'circle-stroke-color': '#ffffff', | |
| }, | |
| filter: ['==', '$type', 'Point'], | |
| }); | |
| // Add layer for lines | |
| mapInstance.addLayer({ | |
| id: 'geojson-lines', | |
| type: 'line', | |
| source: 'geojson-data', | |
| paint: { | |
| 'line-color': '#ff4500', | |
| 'line-width': 3, | |
| }, | |
| filter: ['==', '$type', 'LineString'], | |
| }); | |
| } | |
| } else { | |
| // If no GeoJSON data, remove layers and source if they exist | |
| if (mapInstance.getLayer('geojson-points')) mapInstance.removeLayer('geojson-points'); | |
| if (mapInstance.getLayer('geojson-lines')) mapInstance.removeLayer('geojson-lines'); | |
| if (source) mapInstance.removeSource('geojson-data'); | |
| } | |
| }, [mapData.geojson]); |
🤖 Prompt for AI Agents
components/map/mapbox-map.tsx around lines 527 to 576: the effect mutates map
style (addSource/addLayer) without verifying the style is loaded which can throw
if executed too early; change the logic to check mapInstance.isStyleLoaded()
before calling addSource/addLayer and if the style is not loaded, register a
one‑time mapInstance.once('load', ...) handler to perform the source/layer
addition or updates; ensure the same guard is applied before removing
layers/sources (or perform removals inside the load handler if appropriate) and
optionally add a polygon fill layer when Polygon features are expected.
| useEffect(() => { | ||
| if (!map.current || !mapData.mapCommands || mapData.mapCommands.length === 0) return; | ||
|
|
||
| const mapInstance = map.current; | ||
|
|
||
| mapData.mapCommands.forEach(command => { | ||
| switch (command.command) { | ||
| case 'flyTo': | ||
| mapInstance.flyTo(command.params); | ||
| break; | ||
| case 'easeTo': | ||
| mapInstance.easeTo(command.params); | ||
| break; | ||
| case 'fitBounds': | ||
| // Destructure params to pass 'bounds' and 'options' separately, | ||
| // ensuring the function is called with the correct signature. | ||
| const { bounds, options } = command.params; | ||
| mapInstance.fitBounds(bounds, options || {}); | ||
| break; | ||
| default: | ||
| console.warn(`Unknown map command: ${command.command}`); | ||
| } | ||
| }); | ||
|
|
||
| // Clear commands after execution to prevent re-triggering | ||
| setMapData(prev => ({ ...prev, mapCommands: null })); | ||
|
|
||
| }, [mapData.mapCommands, setMapData]); |
There was a problem hiding this comment.
Fix switch‑case declaration in fitBounds case.
Declare variables inside a block to avoid leaking across cases.
Apply this diff (and satisfy linter):
- case 'fitBounds':
- // Destructure params to pass 'bounds' and 'options' separately,
- // ensuring the function is called with the correct signature.
- const { bounds, options } = command.params;
- mapInstance.fitBounds(bounds, options || {});
- break;
+ case 'fitBounds': {
+ // Destructure params to pass 'bounds' and 'options' separately,
+ // ensuring the function is called with the correct signature.
+ const { bounds, options } = command.params;
+ mapInstance.fitBounds(bounds, options || {});
+ break;
+ }Based on static analysis.
📝 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.
| useEffect(() => { | |
| if (!map.current || !mapData.mapCommands || mapData.mapCommands.length === 0) return; | |
| const mapInstance = map.current; | |
| mapData.mapCommands.forEach(command => { | |
| switch (command.command) { | |
| case 'flyTo': | |
| mapInstance.flyTo(command.params); | |
| break; | |
| case 'easeTo': | |
| mapInstance.easeTo(command.params); | |
| break; | |
| case 'fitBounds': | |
| // Destructure params to pass 'bounds' and 'options' separately, | |
| // ensuring the function is called with the correct signature. | |
| const { bounds, options } = command.params; | |
| mapInstance.fitBounds(bounds, options || {}); | |
| break; | |
| default: | |
| console.warn(`Unknown map command: ${command.command}`); | |
| } | |
| }); | |
| // Clear commands after execution to prevent re-triggering | |
| setMapData(prev => ({ ...prev, mapCommands: null })); | |
| }, [mapData.mapCommands, setMapData]); | |
| useEffect(() => { | |
| if (!map.current || !mapData.mapCommands || mapData.mapCommands.length === 0) return; | |
| const mapInstance = map.current; | |
| mapData.mapCommands.forEach(command => { | |
| switch (command.command) { | |
| case 'flyTo': | |
| mapInstance.flyTo(command.params); | |
| break; | |
| case 'easeTo': | |
| mapInstance.easeTo(command.params); | |
| break; | |
| case 'fitBounds': { | |
| // Destructure params to pass 'bounds' and 'options' separately, | |
| // ensuring the function is called with the correct signature. | |
| const { bounds, options } = command.params; | |
| mapInstance.fitBounds(bounds, options || {}); | |
| break; | |
| } | |
| default: | |
| console.warn(`Unknown map command: ${command.command}`); | |
| } | |
| }); | |
| // Clear commands after execution to prevent re-triggering | |
| setMapData(prev => ({ ...prev, mapCommands: null })); | |
| }, [mapData.mapCommands, setMapData]); |
🧰 Tools
🪛 Biome (2.1.2)
[error] 595-595: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
In components/map/mapbox-map.tsx around lines 579-606, the fitBounds case
declares const variables at case scope which can leak into other cases; fix by
adding a block for that case (case 'fitBounds': { const { bounds, options } =
command.params; mapInstance.fitBounds(bounds, options || {}); break; }) so the
variables are properly scoped and include the missing break to satisfy the
linter.
| - Identify actions in thetext that imply map movements (e.g., "fly to," "center on," "zoom to"). | ||
| - Create a list of command objects, for example: { "command": "flyTo", "params": { "center": [-71.05633, 42.356823], "zoom": 15 } }. | ||
| - If no map commands can be inferred, set "map_commands" to null. |
There was a problem hiding this comment.
Minor typo in prompt.
“inthetext” → “in the text”.
Apply this diff:
- - Identify actions in thetext that imply map movements (e.g., "fly to," "center on," "zoom to").
+ - Identify actions in the text that imply map movements (e.g., "fly to," "center on," "zoom to").📝 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.
| - Identify actions in thetext that imply map movements (e.g., "fly to," "center on," "zoom to"). | |
| - Create a list of command objects, for example: { "command": "flyTo", "params": { "center": [-71.05633, 42.356823], "zoom": 15 } }. | |
| - If no map commands can be inferred, set "map_commands" to null. | |
| - Identify actions in the text that imply map movements (e.g., "fly to," "center on," "zoom to"). | |
| - Create a list of command objects, for example: { "command": "flyTo", "params": { "center": [-71.05633, 42.356823], "zoom": 15 } }. | |
| - If no map commands can be inferred, set "map_commands" to null. |
🤖 Prompt for AI Agents
In lib/agents/geojson-enricher.tsx around lines 27 to 29, fix the typo in the
prompt text by changing "inthetext" to "in the text"; update the prompt string
where that fragment appears so it reads "in the text" (preserve surrounding
wording and punctuation).
| export interface GeoJSONGeometry { | ||
| type: 'Point' | 'LineString' | 'Polygon'; // Can be extended with other GeoJSON geometry types | ||
| coordinates: number[] | number[][] | number[][][]; | ||
| } | ||
|
|
||
| // Defines a single feature in a GeoJSON FeatureCollection. | ||
| export interface GeoJSONFeature { | ||
| type: 'Feature'; | ||
| geometry: GeoJSONGeometry; | ||
| properties: { | ||
| [key: string]: any; // Features can have any number of properties | ||
| }; | ||
| } | ||
|
|
||
| // Defines the structure for a GeoJSON FeatureCollection. | ||
| export interface GeoJSONFeatureCollection { | ||
| type: 'FeatureCollection'; | ||
| features: GeoJSONFeature[]; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer standard GeoJSON types to avoid drift
If feasible, reuse types from @types/geojson to cover Multi* geometries and properties shape.
Example:
import type { Feature, FeatureCollection, Geometry } from 'geojson';
export type GeoJSONGeometry = Geometry;
export type GeoJSONFeature = Feature;
export type GeoJSONFeatureCollection = FeatureCollection;Verify downstream code compatibility before changing exported types.
🤖 Prompt for AI Agents
In lib/types/custom.ts around lines 8 to 26, the file defines bespoke GeoJSON
interfaces that omit Multi* geometries and differ from the community standard;
replace these custom types by importing and re-exporting the canonical types
from the @types/geojson package (Geometry, Feature, FeatureCollection) so all
geometry variants and property typings are covered, update the exported type
names to alias those imports (GeoJSONGeometry = Geometry, GeoJSONFeature =
Feature, GeoJSONFeatureCollection = FeatureCollection), and then run a quick
search/compile to verify and adjust any downstream code that relied on the
narrower custom shapes to ensure compatibility before committing the change.
| function isGeoJSONFeatureCollection(obj: any): obj is GeoJSONFeatureCollection { | ||
| if (typeof obj !== 'object' || obj === null) return false; | ||
|
|
||
| const hasType = obj.type === 'FeatureCollection'; | ||
| const hasFeatures = Array.isArray(obj.features); | ||
|
|
||
| return hasType && hasFeatures; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Strengthen GeoJSON FeatureCollection validation (optional)
Currently only checks type and features array. Optionally ensure each feature is shaped correctly.
-function isGeoJSONFeatureCollection(obj: any): obj is GeoJSONFeatureCollection {
+function isGeoJSONFeatureCollection(obj: any): obj is GeoJSONFeatureCollection {
if (typeof obj !== 'object' || obj === null) return false;
- const hasType = obj.type === 'FeatureCollection';
- const hasFeatures = Array.isArray(obj.features);
-
- return hasType && hasFeatures;
+ const hasType = obj.type === 'FeatureCollection';
+ const hasFeatures = Array.isArray(obj.features);
+ const featuresValid =
+ hasFeatures &&
+ obj.features.every(
+ (f: any) =>
+ f &&
+ typeof f === 'object' &&
+ f.type === 'Feature' &&
+ f.geometry &&
+ typeof f.geometry === 'object'
+ );
+
+ return hasType && hasFeatures && featuresValid;
}📝 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.
| function isGeoJSONFeatureCollection(obj: any): obj is GeoJSONFeatureCollection { | |
| if (typeof obj !== 'object' || obj === null) return false; | |
| const hasType = obj.type === 'FeatureCollection'; | |
| const hasFeatures = Array.isArray(obj.features); | |
| return hasType && hasFeatures; | |
| } | |
| function isGeoJSONFeatureCollection(obj: any): obj is GeoJSONFeatureCollection { | |
| if (typeof obj !== 'object' || obj === null) return false; | |
| const hasType = obj.type === 'FeatureCollection'; | |
| const hasFeatures = Array.isArray(obj.features); | |
| const featuresValid = | |
| hasFeatures && | |
| obj.features.every( | |
| (f: any) => | |
| f && | |
| typeof f === 'object' && | |
| f.type === 'Feature' && | |
| f.geometry && | |
| typeof f.geometry === 'object' | |
| ); | |
| return hasType && hasFeatures && featuresValid; | |
| } |
🤖 Prompt for AI Agents
In lib/utils/type-guards.ts around lines 22 to 29, the current
isGeoJSONFeatureCollection only checks obj.type and that obj.features is an
array; strengthen validation by iterating over obj.features and ensuring each
element is a non-null object with type === 'Feature', has a geometry property
that is either null or a non-null object with a string type and a coordinates
field (or valid geometry shape), and optionally that properties exists (can be
any). Return false if any feature fails these checks; otherwise return true.
| export function isLocationResponse(obj: any): obj is LocationResponse { | ||
| if (typeof obj !== 'object' || obj === null) { | ||
| console.error("Validation failed: Input is not an object or is null."); | ||
| return false; | ||
| } | ||
|
|
||
| // 1. Check for the 'type' property (must be 'tool') | ||
| const hasType = obj.type === 'tool'; | ||
| if (!hasType) { | ||
| console.error("Validation failed: 'type' property is missing or not 'tool'.", obj); | ||
| return false; | ||
| } | ||
|
|
||
| // 2. Check for the 'text' property (must be a string) | ||
| const hasText = typeof obj.text === 'string'; | ||
| if (!hasText) { | ||
| console.error("Validation failed: 'text' property is missing or not a string.", obj); | ||
| return false; | ||
| } | ||
|
|
||
| // 3. Check for the 'geojson' property (must be a GeoJSONFeatureCollection or null) | ||
| const hasGeojson = obj.geojson === null || isGeoJSONFeatureCollection(obj.geojson); | ||
| if (!hasGeojson) { | ||
| console.error("Validation failed: 'geojson' property is not a valid GeoJSONFeatureCollection or null.", obj); | ||
| return false; | ||
| } | ||
|
|
||
| // 3. Check for the 'map_commands' property (optional, but if it exists, must be an array of MapCommands or null) | ||
| const hasMapCommands = | ||
| !('map_commands' in obj) || // Property doesn't exist (which is valid) | ||
| obj.map_commands === null || // Property is null (which is valid) | ||
| (Array.isArray(obj.map_commands) && obj.map_commands.every(isMapCommand)); // Property is an array of valid MapCommands | ||
|
|
||
| if (!hasMapCommands) { | ||
| console.error("Validation failed: 'map_commands' property is not a valid array of MapCommand objects or null.", obj); | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } No newline at end of file |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM on LocationResponse guard; minimal, readable validations
Good sequencing, clear logs, and optional map_commands handling. Consider replacing magic lists with a Set in isMapCommand for micro-clarity, and renumber the comment “3.” for map_commands to “4.”.
🤖 Prompt for AI Agents
In lib/utils/type-guards.ts around lines 37 to 76, fix the duplicated comment
numbering by changing the second comment labeled "3." (for map_commands) to
"4.", and also update the isMapCommand implementation in this file to replace
any magic-list/array of allowed command names with a readonly Set for membership
checks (use Set.has instead of array.includes) to improve clarity and
micro-performance.
| // Add other properties as needed based on your usage | ||
| }; | ||
| import { getModel } from 'QCX/lib/utils'; | ||
| import { getModel } from '../lib/utils'; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
Import path switch to local utils — confirm export and consistency.
Looks fine. Please verify that ../lib/utils exports getModel and consider aligning import style with your alias-based imports for consistency (e.g., @/lib/utils) if supported.
Run to confirm the export exists and usages resolve:
🏁 Script executed:
#!/bin/bash
# Show where getModel is exported
rg -nP -C2 'export\s+(const|function)\s+getModel\b' --type=ts --type=tsx
# Show imports of lib/utils across the repo to ensure consistency
rg -nP -C2 'from\s+[\'"].*/lib/utils[\'"]\s*;'Length of output: 96
🏁 Script executed:
#!/bin/bash
# Find where getModel is exported
rg -nP -C2 "export\s+(const|function)\s+getModel\b" -g "*.ts" -g "*.tsx"
# Find all imports from lib/utils
rg -nP -C2 "from\s+['\"].*/lib/utils['\"]" -g "*.ts" -g "*.tsx"Length of output: 10149
Confirm export and align import path
getModel is exported in lib/utils/index.ts. For consistency with alias-based imports, update mapbox_mcp/hooks.ts (line 11) to:
import { getModel } from '@/lib/utils';🤖 Prompt for AI Agents
In mapbox_mcp/hooks.ts around line 11, the import uses a relative path to
getModel from '../lib/utils' but the project exports getModel from
lib/utils/index.ts and uses alias-based imports; change the import to use the
alias by replacing the relative path with '@/lib/utils' so the line imports
getModel from '@/lib/utils' to match the export and project import conventions.
| "scripts": { | ||
| "dev": "next dev --turbo", | ||
| "build": "next build", | ||
| "build": "node --max-old-space-size=4096 node_modules/.bin/next build", |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Memory bump for build — consider cross‑platform NODE_OPTIONS.
Your command works; to be more portable, use env var via cross‑env (already in devDeps):
"build": "cross-env NODE_OPTIONS=--max_old_space_size=4096 next build"🤖 Prompt for AI Agents
In package.json around line 8, the "build" script uses a platform-dependent Node
invocation with --max-old-space-size; replace it with a cross-platform
environment var using the existing cross-env devDep. Update the script to set
NODE_OPTIONS=--max_old_space_size=4096 via cross-env and call next build
directly so Windows/macOS/Linux all respect the memory bump.
User description
This pull request addresses several issues related to the location enrichment and map display feature, improving its robustness, correctness, and consistency. Key changes include adding consistent
type: 'tool'properties, implementing runtime validation for AI-generated data, correcting thefitBoundsmap function call, and improving the reliability of the GeoJSON enrichment agent.PR created automatically by Jules for task 10442103015176452485
PR Type
Enhancement, Bug fix
Description
Add GeoJSON enrichment agent for location data extraction
Implement runtime validation with type guards
Fix map command execution and GeoJSON rendering
Increase Node.js memory allocation for build process
Diagram Walkthrough
File Walkthrough
7 files
Add runtime validation for location responsesDefine TypeScript interfaces for location dataIntegrate GeoJSON enricher with validationCreate agent for extracting location dataAdd GeoJSON rendering and map commandsExtend context with GeoJSON propertiesCreate handler for location response updates1 files
Fix import path reference1 files
Add Playwright test for location enrichment1 files
Increase Node memory and update dependencies1 files
Summary by CodeRabbit