I've corrected an issue with service selection and response parsing i… #220
Conversation
…n your `GeospatialTool`. The `GeospatialTool` was failing with a "No content returned from mapping service" error. I traced this back to two problems: 1. The logic for selecting a service was only handling the 'directions' query type. I've updated it to use a switch statement, which correctly maps other query types like 'distance' to the appropriate service, such as 'mapbox_matrix'. 2. The code was trying to parse a `tool_results` property from the mapping service's response, but the service actually returns a `content` property. I've adjusted the parsing logic to use the correct property. These fixes ensure the right service is called for each query and that the response is parsed correctly, which should resolve the execution failure.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughReplaces the old geospatial schema and runtime wiring with a discriminated Zod schema and an MCP-driven geospatial tool: geospatial.execute now accepts the new schema type, dynamically resolves and invokes MCP tools (with retries/timeouts), parses MCP responses into normalized location output, and adds smithery dependency. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant GeospatialTool
participant MCP_Helper
participant MCP_Service
Caller->>GeospatialTool: execute(GeospatialQuery params)
GeospatialTool->>MCP_Helper: getConnectedMcpClient(env/config)
MCP_Helper-->>GeospatialTool: MCPClient (connected)
GeospatialTool->>MCPClient: listTools()
GeospatialTool->>GeospatialTool: resolve toolName for queryType
GeospatialTool->>MCPClient: callTool(toolName, toolArgs) with retry & 30s timeout
MCPClient->>MCP_Service: invoke external tool
MCP_Service-->>MCPClient: raw serviceResponse
MCPClient-->>GeospatialTool: toolCallResult
GeospatialTool->>GeospatialTool: extract JSON / parse locations -> mcp_response
alt success
GeospatialTool-->>Caller: MAP_QUERY_TRIGGER { mcp_response, queryType, ... }
else failure
GeospatialTool-->>Caller: MAP_QUERY_TRIGGER { error, ... }
end
GeospatialTool->>MCPClient: close() (finally)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨No code suggestions found for the PR. |
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
lib/agents/tools/geospatial.tsx (4)
231-231: Type the tool call result for clarity and safer parsingAdd a minimal structural type to document expectations from MCP and help catch mistakes.
- let toolCallResult; + let toolCallResult: { content?: Array<{ text?: string | null }> } | undefined;
234-239: Timeout + retries are solid; consider an AbortController if supportedThe Promise.race-based 30s timeout with retries is good. If MCP's client supports abort signals, switching to an AbortController would avoid work continuing in the background after timeouts.
251-251: Avoid logging the full raw tool result in productionThe raw payload can be large and may contain sensitive data. Gate the log and trim to a preview.
- console.log('[GeospatialTool] Raw tool result:', toolCallResult); + if (process.env.NODE_ENV !== 'production') { + const preview = (() => { + try { return JSON.stringify(toolCallResult).slice(0, 2000); } catch { return String(toolCallResult).slice(0, 2000); } + })(); + console.log('[GeospatialTool] Raw tool result preview:', preview); + }
313-320: Public API shape changed: add queryType and error to GeospatialToolOutputThe returned payload now includes queryType and error, but the consumer interface (components/map/map-query-handler.tsx:17-22) doesn’t declare them. This is safe at runtime but can cause TypeScript excess-property warnings where object literals are typed. Update the interface to reflect the new fields (optional).
Proposed interface update (outside this file):
// components/map/map-query-handler.tsx interface GeospatialToolOutput { type: string; // e.g., "MAP_QUERY_TRIGGER" originalUserInput: string; timestamp: string; mcp_response: McpResponseData | null; // New (optional) fields returned by geospatialTool: queryType?: string; error?: string | null; }Would you like me to open a follow-up PR to align the types and add a quick unit test for the content parsing fallback?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/agents/tools/geospatial.tsx(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
lib/agents/tools/geospatial.tsx (3)
components/map/map-query-handler.tsx (3)
GeospatialToolOutput(18-23)toolOutput(33-74)MapQueryHandlerProps(25-28)mapbox_mcp/hooks.ts (3)
query(91-156)address(158-174)location(195-212)mapbox_mcp/index.ts (1)
testMCPConnection(14-89)
I've just corrected a couple of issues that were causing problems with my location-based abilities. First, I was misinterpreting some of the data I received from geospatial services. I've fixed that now. Second, I realized some of my functions for calculating directions and distances were incorrectly expecting geographic coordinates instead of place names. I've updated my approach to properly handle place names for these requests. I've also improved my error handling. Now, if a location query fails, I can give you a more specific reason why, instead of just a generic error message. These changes make my geospatial skills more robust, so I can better handle your requests for directions, distances, and geocoding.
I performed an extensive debugging process to fix multiple issues in the GeospatialTool, and as a result, it is now significantly more robust. The key changes I made are: 1. **Corrected Tool Selection:** I updated the tool to use the `_by_places` variants of the Mapbox tools (`mapbox_directions_by_places`, `mapbox_matrix_by_places`) for direction and distance queries, as these are designed to work with place names. 2. **Structured Argument Passing:** I updated the tool's `execute` function signature to accept a `destination` parameter, in line with the tool's Zod schema. This allows structured data (`query` as origin, `destination` as destination) to be passed as a `places` array to the underlying service, which replaces a brittle string-splitting logic. 3. **Improved Error Handling:** I improved the tool to inspect the `isError` flag in the response from the mapping service and propagate the specific error message. This will provide much better feedback on failures. 4. **Build Fixes:** I resolved a transient TypeScript build error (`Cannot find name 'GeospatialQuery'`) by using an inline type definition for the `execute` function parameters, ensuring a stable build. These changes collectively address a series of related bugs, starting from a generic "No content returned" error to more specific argument validation errors, ultimately leading to a correctly implemented and robust geospatial tool.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
lib/agents/tools/geospatial.tsx (1)
246-251: Fix MCP call signature — pass (name, args) instead of an objectThe MCP JS client in this repo is used with positional arguments (see mapbox_mcp/hooks.ts). Passing an object here will likely fail at runtime.
Apply this diff:
- toolCallResult = await Promise.race([ - mcpClient.callTool({ name: toolName, arguments: toolArgs }), + toolCallResult = await Promise.race([ + mcpClient.callTool(toolName, toolArgs), new Promise((_, reject) => { setTimeout(() => reject(new Error('Tool call timeout after 30 seconds')), 30000); }), ]);
♻️ Duplicate comments (3)
lib/agents/tools/geospatial.tsx (3)
210-225: Map to actual tool names exposed by your MCP server (or resolve dynamically)The hardcoded names 'mapbox_directions_by_places'/'mapbox_matrix_by_places'/'mapbox_geocoding' may not exist. In this repo, MCP tools referenced are typically: geocode_location, calculate_distance, search_nearby_places, generate_map_link. Mismatched names will produce “tool not found.”
Replace the switch with a dynamic resolver that prefers available tools (falling back to static names):
- let toolName; - switch (queryType) { - case 'directions': - toolName = 'mapbox_directions_by_places'; - break; - case 'distance': - toolName = 'mapbox_matrix_by_places'; - break; - case 'geocode': - case 'reverse': - case 'search': - case 'map': - default: - toolName = 'mapbox_geocoding'; - break; - } + let toolName: string | undefined; + try { + const { tools } = await mcpClient.listTools(); + const names = new Set((tools ?? []).map((t: any) => t.name)); + const prefer = (...cands: string[]) => cands.find(n => names.has(n)); + switch (queryType) { + case 'directions': + toolName = prefer('mapbox_directions_by_places', 'mapbox_directions', 'get_directions'); + break; + case 'distance': + toolName = prefer('calculate_distance', 'mapbox_matrix_by_places', 'mapbox_matrix'); + break; + case 'search': + toolName = prefer('search_nearby_places', 'mapbox_geocoding', 'geocode_location'); + break; + case 'map': + toolName = prefer('generate_map_link', 'mapbox_geocoding', 'geocode_location'); + break; + case 'reverse': + case 'geocode': + default: + toolName = prefer('geocode_location', 'mapbox_geocoding'); + } + } catch { + // Fallback if listTools fails + switch (queryType) { + case 'directions': toolName = 'mapbox_directions'; break; + case 'distance': toolName = 'mapbox_matrix'; break; + default: toolName = 'mapbox_geocoding'; + } + }
272-279: Don’t assume payload is in content[0]; scan all content blocks and prefer fenced JSONLimiting to content[0].text is brittle and will reintroduce “No content returned” for valid multi-block responses.
Apply this diff:
- const responseContent = serviceResponse?.content; - - if (!responseContent || responseContent.length === 0 || !responseContent[0].text) { - throw new Error('No content returned from mapping service'); - } - - let content: any = responseContent[0].text; + type McpContentBlock = { text?: string | null } | { [k: string]: any }; + const blocks: McpContentBlock[] = Array.isArray(serviceResponse?.content) ? (serviceResponse!.content as McpContentBlock[]) : []; + const textBlocks = blocks + .map(b => (typeof (b as any)?.text === 'string' ? ((b as any).text as string) : null)) + .filter((t): t is string => !!t && t.trim().length > 0); + if (textBlocks.length === 0) { + throw new Error('No content returned from mapping service'); + } + const preferredText = textBlocks.find(t => /```(?:json)?\s*[\s\S]*?```/i.test(t)) ?? textBlocks[0]; + let content: any = preferredText;
227-236: Fix geospatial tool: use server tool names and correct argument keysThe geospatial tool is calling MCP tools with mismatched names/args (e.g. using searchText and mapbox_* names) while the MCP client code exposes tools like geocode_location, calculate_distance, and search_nearby_places. This will produce bad requests or no-ops.
Files to fix:
- lib/agents/tools/geospatial.tsx — replace the tool-name mapping and the toolArgs construction (currently around the switch + the block using searchText / places).
- Reference: mapbox_mcp/hooks.ts shows the expected tool schemas (geocode_location expects { query, includeMapPreview? }; calculate_distance expects { from, to, profile, includeRouteMap? }; search_nearby_places expects { location, query, radius, limit }).
Apply this diff to align args with the MCP server:
- switch (queryType) { - case 'directions': - toolName = 'mapbox_directions_by_places'; - break; - case 'distance': - toolName = 'mapbox_matrix_by_places'; - break; - case 'geocode': - case 'reverse': - case 'search': - case 'map': - default: - toolName = 'mapbox_geocoding'; - break; - } - - let toolArgs; - if (toolName === 'mapbox_directions_by_places' || toolName === 'mapbox_matrix_by_places') { - const places = [query]; - if (destination) { - places.push(destination); - } - toolArgs = { places, includeMapPreview: includeMap !== false }; - } else { - toolArgs = { searchText: query, includeMapPreview: includeMap !== false }; - } + // Align tool names and arguments with the MCP server (see mapbox_mcp/hooks.ts) + switch (queryType) { + case 'directions': + case 'distance': + // The MCP exposes a `calculate_distance` tool for routes/distances + toolName = 'calculate_distance'; + break; + case 'search': + toolName = 'search_nearby_places'; + break; + case 'geocode': + case 'reverse': + case 'map': + default: + toolName = 'geocode_location'; + break; + } + + let toolArgs: any; + if (toolName === 'calculate_distance') { + if (destination) { + toolArgs = { + from: query, + to: destination, + profile: 'driving', + includeRouteMap: includeMap !== false, + }; + } else { + // No destination — fall back to geocoding the query instead of sending an invalid distance request. + toolName = 'geocode_location'; + toolArgs = { query, includeMapPreview: includeMap !== false }; + } + } else if (toolName === 'search_nearby_places') { + toolArgs = { + location: query, + query, + radius: 1000, + limit: 5, + }; + } else { + // geocode_location and safe default + toolArgs = { query, includeMapPreview: includeMap !== false }; + }Reasoning/evidence: mapbox_mcp/index.ts and mapbox_mcp/hooks.ts call geocode_location with { query, includeMapPreview } and calculate_distance with { from, to, profile, includeRouteMap }, so geospatial.tsx must use the same names/keys to avoid 4xx/invalid-tool-arg errors.
🧹 Nitpick comments (6)
lib/agents/tools/geospatial.tsx (6)
296-314: Broaden response mapping to handle common location shapes (lat/lng, center array, geometry.coordinates)Requiring parsedData.location only handles one shape. MCP tools often return { lat, lng }, { center: [lng, lat] }, or { geometry: { coordinates: [lng, lat] } }.
Apply this diff:
- if (typeof content === 'object' && content !== null) { - const parsedData = content as any; - - if (parsedData.location) { - mcpData = { - location: { - latitude: parsedData.location.latitude, - longitude: parsedData.location.longitude, - place_name: parsedData.location.place_name || parsedData.location.name, - address: parsedData.location.address || parsedData.location.formatted_address, - }, - mapUrl: parsedData.mapUrl || parsedData.map_url, - }; - } else { - throw new Error("Response missing required 'location' field"); - } - } else { - throw new Error("Unexpected response format from mapping service"); - } + if (typeof content === 'object' && content !== null) { + const parsedData = content as any; + const loc = parsedData.location ?? parsedData.result?.location ?? null; + let latitude: number | undefined; + let longitude: number | undefined; + let place_name: string | undefined; + let address: string | undefined; + + if (loc) { + latitude = loc.latitude ?? loc.lat; + longitude = loc.longitude ?? loc.lng ?? loc.lon ?? loc.long; + place_name = loc.place_name ?? loc.name; + address = loc.address ?? loc.formatted_address; + } + + // Common alt shapes + const center = parsedData.center ?? parsedData.coordinates ?? parsedData.geometry?.coordinates; + if ((latitude == null || longitude == null) && Array.isArray(center) && center.length >= 2) { + // center/coordinates convention: [lng, lat] + longitude = Number(center[0]); + latitude = Number(center[1]); + } + + if (latitude != null && longitude != null) { + mcpData = { + location: { latitude, longitude, place_name, address }, + mapUrl: parsedData.mapUrl || parsedData.map_url, + }; + } else { + throw new Error("Response missing required coordinates"); + } + } else { + throw new Error("Unexpected response format from mapping service"); + }
170-170: Reduce PII/log noise — truncate logged query/destinationLogging full user queries can leak PII and flood logs. Truncate to a safe preview.
- console.log('[GeospatialTool] Execute called with:', { query, queryType, includeMap, destination }); + console.log('[GeospatialTool] Execute called with:', { + query: query?.slice(0, 80), + queryType, + includeMap, + destination: destination?.slice(0, 80) + });
265-271: Don’t rely on isError unless documented by your MCP clientIn this repo, callTool throws on tool errors and the result is a content array. The isError flag may not exist and could produce false paths.
If isError isn’t part of your client’s ToolResult, drop this check and rely on try/catch around callTool. Otherwise guard with a strict boolean check:
- if (serviceResponse.isError) { + if (serviceResponse && (serviceResponse as any).isError === true) { const errorMessage = serviceResponse.content?.[0]?.text || 'Unknown error from mapping service'; throw new Error(errorMessage); }
263-263: Avoid dumping full tool results to logstoolCallResult may contain large payloads. Consider logging a compact summary instead.
- console.log('[GeospatialTool] Raw tool result:', toolCallResult); + console.log('[GeospatialTool] Raw tool result:', { + hasContent: !!(toolCallResult as any)?.content, + contentBlocks: Array.isArray((toolCallResult as any)?.content) ? (toolCallResult as any).content.length : 0 + });
240-261: Optional: add backoff to retries and avoid orphaned in-flight callsNice use of retries and timeout. Consider exponential backoff (e.g., 500ms, 1s, 2s) and, if the SDK supports it, an abort signal to cancel in-flight calls after timeout to prevent server-side work from accumulating.
331-338: Expose queryType to consumers (type hint)You return queryType in the payload, but components/map/map-query-handler.tsx’s GeospatialToolOutput doesn’t include it. It’s fine at runtime, but adding it to the type improves clarity.
Outside this file, update the interface:
// components/map/map-query-handler.tsx interface GeospatialToolOutput { type: string; originalUserInput: string; timestamp: string; mcp_response: McpResponseData | null; queryType?: string; // add }I can open a follow-up PR if you want.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/agents/tools/geospatial.tsx(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
lib/agents/tools/geospatial.tsx (2)
components/map/map-query-handler.tsx (4)
toolOutput(33-74)GeospatialToolOutput(18-23)MapQueryHandlerProps(25-28)prevData(39-49)mapbox_mcp/hooks.ts (3)
query(91-156)address(158-174)location(195-212)
🔇 Additional comments (1)
lib/agents/tools/geospatial.tsx (1)
159-169: Confirmed: schema includes optionaldestinationfield
- lib/schema/geospatial.ts —
destination: z.string().optional()(lines ~32–34)- lib/agents/tools/geospatial.tsx —
executeacceptsdestination?: string(lines 158–169)No changes required.
I've completed an extensive, iterative debugging process to resolve a cascade of issues within the `GeospatialTool`. I have significantly refactored it, and it is now robust and correct.
The key changes are:
1. **Correct Tool Selection:** The tool now uses the `_by_places` variants of the Mapbox tools (`mapbox_directions_by_places`, `mapbox_matrix_by_places`) for direction and distance queries, which are designed to work with place names.
2. **Structured Argument Passing:** The tool's `execute` function signature has been updated to correctly accept all parameters from its Zod schema (e.g., `destination`). This allows structured data to be provided and passed as a `places` array to the underlying service, eliminating brittle string parsing.
3. **Robust Response & Error Handling:**
- The tool now inspects the `isError` flag in the service response and propagates specific error messages, providing much better feedback on failures.
- The logic for parsing successful responses has been fixed to correctly handle the `results` array returned by the Mapbox API, resolving the "missing 'location' field" error.
4. **Build Stability:** I resolved transient TypeScript build errors by using a stable inline type definition for the `execute` function parameters, ensuring a successful build.
These changes collectively address a series of related bugs, from generic errors to specific validation and parsing failures, resulting in a correctly implemented and reliable geospatial tool.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
lib/agents/tools/geospatial.tsx (1)
347-353: Add optionalqueryTypeto GeospatialToolOutputlib/agents/tools/geospatial.tsx now returns
queryType; update the consumer type to match.
- File to update: components/map/map-query-handler.tsx — modify
interface GeospatialToolOutputto includequeryType?: string;(insert afteroriginalUserInput).Suggested diff:
@@ interface GeospatialToolOutput { type: string; // e.g., "MAP_QUERY_TRIGGER" originalUserInput: string; + queryType?: string; timestamp: string; mcp_response: McpResponseData | null; }
♻️ Duplicate comments (2)
lib/agents/tools/geospatial.tsx (2)
210-225: Wrong MCP tool-name mapping; resolve dynamically to actual server toolsThe switch maps to non-existent names (
mapbox_directions_by_places,mapbox_matrix_by_places,mapbox_geocoding). In this repo, the MCP server exposes tools likegeocode_location,calculate_distance,search_nearby_places,generate_map_link(see mapbox_mcp/hooks.ts). Current mappings will 404 or return errors at runtime.Replace the static switch with a dynamic resolver that prefers available tools and falls back to known names:
- let toolName; - switch (queryType) { - case 'directions': - toolName = 'mapbox_directions_by_places'; - break; - case 'distance': - toolName = 'mapbox_matrix_by_places'; - break; - case 'geocode': - case 'reverse': - case 'search': - case 'map': - default: - toolName = 'mapbox_geocoding'; - break; - } + const toolName = await (async () => { + try { + const { tools } = await mcpClient.listTools(); + const names = new Set((tools ?? []).map((t: any) => t.name)); + const prefer = (...cands: string[]) => cands.find(n => names.has(n)); + switch (queryType) { + case 'directions': + case 'distance': + return prefer('calculate_distance', 'mapbox_matrix', 'mapbox_directions')!; + case 'search': + return prefer('search_nearby_places', 'mapbox_geocoding')!; + case 'map': + return prefer('generate_map_link', 'mapbox_geocoding')!; + case 'reverse': + case 'geocode': + default: + return prefer('geocode_location', 'mapbox_geocoding')!; + } + } catch { + switch (queryType) { + case 'directions': return 'mapbox_directions'; + case 'distance': return 'mapbox_matrix'; + case 'search': return 'mapbox_geocoding'; + case 'map': return 'mapbox_geocoding'; + case 'reverse': + case 'geocode': + default: return 'mapbox_geocoding'; + } + } + })();
265-279: Parse all content blocks and prefer JSON; don’t assume content[0].textMCP responses often include multiple content blocks; JSON might be in a later block or in a
json-typed block. Restricting tocontent[0].textis brittle and can re-trigger “No content returned”.Apply this parsing refactor:
- const serviceResponse = toolCallResult as { content?: Array<{ text?: string }>, isError?: boolean }; - - if (serviceResponse.isError) { - const errorMessage = serviceResponse.content?.[0]?.text || 'Unknown error from mapping service'; - throw new Error(errorMessage); - } - - const responseContent = serviceResponse?.content; - - if (!responseContent || responseContent.length === 0 || !responseContent[0].text) { - throw new Error('No content returned from mapping service'); - } - - let content: any = responseContent[0].text; + type McpContent = { type?: string; text?: string | null; json?: any; [k: string]: any }; + const serviceResponse = toolCallResult as { content?: McpContent[]; isError?: boolean }; + const blocks = Array.isArray(serviceResponse?.content) ? serviceResponse!.content! : []; + + if (serviceResponse.isError) { + const firstText = blocks.find(b => typeof b?.text === 'string')?.text; + throw new Error(firstText || 'Unknown error from mapping service'); + } + + if (blocks.length === 0) { + throw new Error('No content returned from mapping service'); + } + + // Prefer a json-typed block; otherwise parse a text block (fenced JSON if present) + const jsonBlock = blocks.find(b => b && 'json' in b && (b as any).json != null) as (McpContent | undefined); + let content: any; + if (jsonBlock && jsonBlock.json != null) { + content = jsonBlock.json; + } else { + const textBlocks = blocks + .map(b => (typeof b?.text === 'string' ? (b!.text as string) : null)) + .filter((t): t is string => !!t && t.trim().length > 0); + + if (textBlocks.length === 0) { + throw new Error('No content returned from mapping service'); + } + + const preferredText = + textBlocks.find(t => /```(?:json)?\s*[\s\S]*?```/i.test(t)) ?? textBlocks[0]; + + let maybeJson: string = preferredText; + const fenced = preferredText.match(/```(?:json)?\n?([\s\S]*?)\n?```/i); + if (fenced) { + maybeJson = fenced[1].trim(); + } + content = maybeJson; + }
🧹 Nitpick comments (3)
lib/agents/tools/geospatial.tsx (3)
227-236: Require destination for distance/directions and shape args appropriatelyIf the selected tool is a distance/directions tool, a missing
destinationwill likely fail. Also, arg shapes differ across tools. At minimum, validatedestination. Optionally, send both common arg shapes so the server can pick what it needs.- let toolArgs; - if (toolName === 'mapbox_directions_by_places' || toolName === 'mapbox_matrix_by_places') { - const places = [query]; - if (destination) { - places.push(destination); - } - toolArgs = { places, includeMapPreview: includeMap !== false }; - } else { - toolArgs = { searchText: query, includeMapPreview: includeMap !== false }; - } + let toolArgs: Record<string, any>; + const isRoute = ['calculate_distance', 'mapbox_matrix', 'mapbox_directions'].includes(toolName); + if (isRoute) { + if (!destination) { + throw new Error('“destination” is required for directions/distance queries'); + } + // Provide multiple common shapes; MCP server can ignore extras. + toolArgs = { + origin: query, + destination, + from: query, + to: destination, + places: [query, destination], + includeMapPreview: includeMap !== false, + }; + } else { + toolArgs = { searchText: query, query, includeMapPreview: includeMap !== false }; + }Please verify expected arg names against your MCP server’s handlers (calculate_distance/geocode_location).
263-263: Avoid logging raw tool results to reduce noise/PII riskThe entire MCP response can be large and may include user-derived strings. Log a concise summary instead.
- console.log('[GeospatialTool] Raw tool result:', toolCallResult); + console.log('[GeospatialTool] Tool call completed');Optionally, log only block counts/types after you construct
blocksin the parsing section.
299-312: Improve result-shape tolerance and coordinate extractionDifferent handlers may emit coordinates as
center: [lng,lat],lat/lng, or nestedcoordinates. Add fallbacks and validate numbers. Also considermap_urlon either the root or first result.- if (parsedData.results && Array.isArray(parsedData.results) && parsedData.results.length > 0) { - // For now, just take the first result. - const firstResult = parsedData.results[0]; - mcpData = { - location: { - latitude: firstResult.coordinates?.latitude, - longitude: firstResult.coordinates?.longitude, - place_name: firstResult.name || firstResult.place_name, - address: firstResult.full_address || firstResult.address, - }, - mapUrl: parsedData.mapUrl, - }; - } + if (parsedData.results && Array.isArray(parsedData.results) && parsedData.results.length > 0) { + const firstResult = parsedData.results[0]; + const lat = + firstResult?.coordinates?.latitude ?? + firstResult?.lat ?? + (Array.isArray(firstResult?.center) ? firstResult.center[1] : undefined); + const lng = + firstResult?.coordinates?.longitude ?? + firstResult?.lng ?? + (Array.isArray(firstResult?.center) ? firstResult.center[0] : undefined); + if (typeof lat !== 'number' || typeof lng !== 'number') { + throw new Error('Invalid coordinates in mapping service result'); + } + mcpData = { + location: { + latitude: lat, + longitude: lng, + place_name: firstResult.name || firstResult.place_name, + address: firstResult.full_address || firstResult.address || firstResult.formatted_address, + }, + mapUrl: parsedData.mapUrl || parsedData.map_url || firstResult.mapUrl || firstResult.map_url, + }; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/agents/tools/geospatial.tsx(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
lib/agents/tools/geospatial.tsx (2)
components/map/map-query-handler.tsx (4)
toolOutput(33-74)GeospatialToolOutput(18-23)MapQueryHandlerProps(25-28)prevData(39-49)mapbox_mcp/hooks.ts (3)
query(91-156)address(158-174)location(195-212)
🔇 Additional comments (3)
lib/agents/tools/geospatial.tsx (3)
170-170: LGTM: helpful input loggingIncluding
destinationin the execute log will aid debugging.
246-251: LGTM: robust timeout around tool call with retriesWrapping the tool invocation in a 30s Promise.race with retry is solid.
159-169: Confirmed:destinationis present in schema — no change requiredVerified that geospatialQuerySchema includes the optional field.
- lib/schema/geospatial.ts — lines ~32–34:
destination: z.string().optional().describe("Destination for directions queries (when different from main query)")- lib/agents/tools/geospatial.tsx — execute signature includes
destination?: string(lines ~163–169)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
lib/agents/tools/geospatial.tsx (2)
180-193: Tool name resolution correctly handles available tools.The dynamic resolver implementation addresses the previous issue by checking available tools and using
preferto select the appropriate tool. The fallback values ensure the code won't break if tools are unavailable.
228-240: Content parsing implementation handles multiple blocks correctly.The code properly addresses the previous concern about parsing only
content[0]by scanning all content blocks and preferring JSON-fenced blocks. The fallback logic ensures robustness.
🧹 Nitpick comments (2)
lib/schema/geospatial.tsx (1)
13-121: Consider adding coordinate validation for string-based location inputs.While the schema validates coordinate objects, the string-based location fields (
origin,destination,location) could benefit from additional validation to catch common issues early.Consider adding a custom refinement to validate coordinate strings when they're provided in "lat,lng" format:
+const coordinateStringRegex = /^-?\d+\.?\d*\s*,\s*-?\d+\.?\d*$/; +const validateLocationString = z.string() + .min(1, "Location cannot be empty") + .refine((val) => { + // If it looks like coordinates, validate the ranges + if (coordinateStringRegex.test(val)) { + const [lat, lng] = val.split(',').map(s => parseFloat(s.trim())); + return lat >= -90 && lat <= 90 && lng >= -180 && lng <= 180; + } + return true; // Non-coordinate strings are valid + }, "Invalid coordinate values"); export const geospatialQuerySchema = z.discriminatedUnion('queryType', [ // ... in the geocode object: - location: z.string() - .min(1, "Location cannot be empty") + location: validateLocationString .describe("The location to geocode - address, place name, or landmark"),lib/agents/tools/geospatial.tsx (1)
195-205: Consider validating required parameters before tool invocation.While the schema enforces required fields, adding runtime validation before the tool call would provide better error messages if the schema validation is bypassed.
Add parameter validation before building toolArgs:
// Build arguments const toolArgs = (() => { switch (queryType) { case 'directions': - case 'distance': return { places: [params.origin, params.destination], includeMapPreview: includeMap, mode: params.mode || 'driving' }; + case 'distance': + if (!params.origin || !params.destination) { + throw new Error(`Missing required parameters for ${queryType}: origin and destination`); + } + return { places: [params.origin, params.destination], includeMapPreview: includeMap, mode: params.mode || 'driving' };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
bun.lockbis excluded by!**/bun.lockb
📒 Files selected for processing (4)
lib/agents/tools/geospatial.tsx(4 hunks)lib/schema/geospatial.ts(0 hunks)lib/schema/geospatial.tsx(1 hunks)package.json(1 hunks)
💤 Files with no reviewable changes (1)
- lib/schema/geospatial.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
lib/schema/geospatial.tsx (2)
components/map/map-data-context.tsx (1)
MapData(7-17)mapbox_mcp/hooks.ts (1)
DistanceResult(24-31)
lib/agents/tools/geospatial.tsx (5)
lib/schema/geospatial.tsx (1)
geospatialQuerySchema(13-121)components/message.tsx (1)
BotMessage(11-29)mapbox_mcp/index.ts (1)
testMCPConnection(14-89)components/map/map-query-handler.tsx (1)
GeospatialToolOutput(18-23)mapbox_mcp/hooks.ts (4)
mcp(65-81)query(91-156)mcp(83-89)address(158-174)
🔇 Additional comments (7)
package.json (1)
79-79: LGTM! The smithery dependency addition aligns with the MCP-driven geospatial tool implementation.The addition of the
smitherypackage (version^0.5.2) properly supports the new MCP-based workflow inlib/agents/tools/geospatial.tsx, which usescreateSmitheryUrlfrom@smithery/sdk.lib/schema/geospatial.tsx (2)
3-11: Well-structured schema design with good use of discriminated unions.The implementation correctly uses Zod's discriminated union pattern for type safety and conditional requirements. The schema design thoughtfully separates concerns by query type and provides clear documentation for each field.
127-152: The classifier function provides reasonable heuristics.The implementation uses simple keyword matching which works well for basic classification. The regex pattern for coordinate detection is appropriate.
Note: For future enhancements, you might consider adding support for more complex patterns like "what's near [location]" for search queries or "show route from A to B" for directions.
lib/agents/tools/geospatial.tsx (4)
36-123: Robust MCP connection setup with comprehensive error handling.The
getConnectedMcpClientfunction implements excellent defensive programming with proper environment validation, config fallback, timeout protection, and detailed logging at each step.
242-253: Response normalization handles multiple formats well.The code correctly handles both
resultsarray andlocationobject formats, with proper field mapping for different response structures.
257-265: Ensure MCP client cleanup happens in all error paths.The finally block correctly ensures client cleanup, and the error handling provides clear user feedback.
267-267: Return value structure is well-designed for downstream processing.The MAP_QUERY_TRIGGER payload includes all necessary information for UI components to render the results appropriately.
2 participants
User description
…n your
GeospatialTool.The
GeospatialToolwas failing with a "No content returned from mapping service" error. I traced this back to two problems:The logic for selecting a service was only handling the 'directions' query type. I've updated it to use a switch statement, which correctly maps other query types like 'distance' to the appropriate service, such as 'mapbox_matrix'.
The code was trying to parse a
tool_resultsproperty from the mapping service's response, but the service actually returns acontentproperty. I've adjusted the parsing logic to use the correct property.These fixes ensure the right service is called for each query and that the response is parsed correctly, which should resolve the execution failure.
PR Type
Bug fix
Description
Fixed service selection logic to handle all query types
Corrected response parsing to use
contentpropertyAdded proper switch statement for query type mapping
Renamed variables for better clarity and consistency
Diagram Walkthrough
File Walkthrough
geospatial.tsx
Fix service selection and response parsing logiclib/agents/tools/geospatial.tsx
service selection
contentproperty instead oftool_resultsgeocodeResultUnknowntotoolCallResultforclarity
'mapbox_matrix'
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores