fix(geospatial): Align parameter with server expectation#219
fix(geospatial): Align parameter with server expectation#219ngoiyaeric wants to merge 8 commits into
Conversation
A geospatial request was sending a `query` parameter, but the server expected `searchText`. This caused a validation error. This commit renames the parameter to `searchText` to match the server's API, resolving the error.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning Rate limit exceeded@ngoiyaeric has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughValidate environment and profile UUID, load Mapbox config with fallback, build and validate an HTTPS Smithery MCP URL, initialize and connect an MCP client with retries, enhanced parsing/error handling, ensure client cleanup, and update tool args and UI logging. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI/Agent
participant Tool as GeospatialToolClient
participant Smithery as Smithery MCP Server
participant Mapbox as Mapbox Backend
UI->>Tool: execute(searchText, includeMapPreview)
Tool->>Tool: validate env & UUID, load config (file or fallback)
Tool->>Smithery: create HTTPS URL, init transport, connect (30s)
alt connected
Tool->>Smithery: listTools (log/warn if fails)
loop up to 3 attempts
Tool->>Smithery: invokeTool(args)
Smithery->>Mapbox: request geocoding/map
Mapbox-->>Smithery: response or error
Smithery-->>Tool: tool response
Tool->>Tool: parse content (extract JSON / map fields)
alt success
Tool-->>UI: MAP_QUERY_TRIGGER with mcp_response
else error
Tool->>Tool: retry or return structured error
end
end
else connection failure
Tool-->>UI: return mcp_response: null and error
end
Tool->>Tool: always close client (cleanup)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~30 minutes Possibly related PRs
Suggested labels
Poem
✨ 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:
|
||||||||||||
This commit addresses two issues: 1. **Parameter Mismatch:** The geospatial tool was sending a `query` parameter, but the server expected `searchText`. This has been corrected. 2. **Incorrect Server URL:** The tool was constructing an incorrect URL, leading to a 404 error. The `mcpServerBaseUrl` has been updated to the correct endpoint you provided, which includes the necessary user scope.
…e changes: 1. **Authentication:** I've updated the HTTP transport to send the API key as a Bearer token in the `Authorization` header, which resolves the 401 Unauthorized error. 2. **Server URL:** I've corrected the base URL for the Smithery server to point to your user-specific endpoint, fixing the 404 Not Found error. 3. **Tool Parameter:** I've changed the `mapbox_geocoding` tool call to use the `searchText` parameter instead of `query`. This aligns with the server's API and fixes the initial tool execution failure.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
lib/agents/tools/geospatial.tsx (1)
301-302: Add null checks for nested property access.The code accesses nested properties without checking if
parsedData.locationexists, which could lead to runtime errors if the location object is missing expected fields.- place_name: parsedData.location.place_name || parsedData.location.name, - address: parsedData.location.address || parsedData.location.formatted_address, + place_name: parsedData.location?.place_name || parsedData.location?.name || null, + address: parsedData.location?.address || parsedData.location?.formatted_address || null,
🧹 Nitpick comments (1)
lib/agents/tools/geospatial.tsx (1)
249-254: Consider extracting timeout duration as a constant.The timeout duration of 30 seconds is hardcoded in multiple places. Consider extracting it as a constant for better maintainability.
Add a constant at the top of the file:
const TOOL_CALL_TIMEOUT_MS = 30000; // 30 secondsThen update the timeout usage:
- setTimeout(() => reject(new Error('Tool call timeout after 30 seconds')), 30000); + setTimeout(() => reject(new Error(`Tool call timeout after ${TOOL_CALL_TIMEOUT_MS/1000} seconds`)), TOOL_CALL_TIMEOUT_MS);
📜 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(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
lib/agents/tools/geospatial.tsx (3)
mapbox_mcp/index.ts (1)
testMCPConnection(14-89)mapbox_mcp/hooks.ts (5)
mcp(65-81)query(91-156)mcp(83-89)address(158-174)location(195-212)components/map/map-query-handler.tsx (1)
GeospatialToolOutput(18-23)
🔇 Additional comments (4)
lib/agents/tools/geospatial.tsx (4)
237-238: Parameter name fix aligns with server expectations.The change from
querytosearchTextcorrectly addresses the server API validation error mentioned in the PR objectives. This fix ensures the parameter name matches what the server expects.
94-97: Security consideration for authentication headers.The authentication headers are properly configured with Bearer token authentication. The logging correctly masks sensitive information, which is good practice.
40-45: UUID validation is properly implemented.The UUID regex validation for the profile ID ensures the format is correct before proceeding. This is a good defensive programming practice.
136-142: Good debugging practice for tool availability.Listing available tools after connection helps with debugging and doesn't fail the flow if the listing fails. This is a well-designed approach.
| // Load configuration | ||
| let config; | ||
| try { | ||
| const mapboxMcpConfig = await import('QCX/mapbox_mcp_config.json'); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix the invalid import path.
The import path QCX/mapbox_mcp_config.json is incorrect. It should be a relative path from the current file location.
- const mapboxMcpConfig = await import('QCX/mapbox_mcp_config.json');
+ const mapboxMcpConfig = await import('../../../mapbox_mcp_config.json');🏁 Script executed:
#!/bin/bash
# Description: Verify the existence and location of mapbox_mcp_config.json file
# Search for the config file
fd -t f "mapbox_mcp_config.json"
# Check if the file exists at the repository root
if [ -f "mapbox_mcp_config.json" ]; then
echo "Config file found at repository root"
cat mapbox_mcp_config.json | head -20
fiLength of output: 539
Fix invalid import path — use repo-root relative path
Verified: mapbox_mcp_config.json is at the repository root. Update the import to a relative path from lib/agents/tools/geospatial.tsx.
- File: lib/agents/tools/geospatial.tsx — line 50
- Found: mapbox_mcp_config.json at repo root
Suggested change:
- const mapboxMcpConfig = await import('QCX/mapbox_mcp_config.json');
+ const mapboxMcpConfig = await import('../../../mapbox_mcp_config.json');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const mapboxMcpConfig = await import('QCX/mapbox_mcp_config.json'); | |
| const mapboxMcpConfig = await import('../../../mapbox_mcp_config.json'); |
🤖 Prompt for AI Agents
In lib/agents/tools/geospatial.tsx around line 50, the import uses a
non-repo-relative specifier; change the import to the repository-root relative
path to the JSON file (e.g., ../../../mapbox_mcp_config.json from
lib/agents/tools/geospatial.tsx). Update the import statement to reference that
relative path and ensure the JSON import syntax your build tool requires
(dynamic import or import with assertion) is used so the file resolves correctly
at runtime.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
lib/agents/tools/geospatial.tsx (1)
260-283: Harden tool result parsing to support MCP content arrays and JSON blocksMCP tool_results.content is often an array of content blocks (e.g., { type: 'json', data } or { type: 'text', text }). Current logic assumes string or object; array responses will fall into the generic-object branch and likely fail. Parse arrays first, then fallback to text fenced JSON or raw text.
- const geocodeResult = geocodeResultUnknown as { tool_results?: Array<{ content?: unknown }> }; - const toolResults = Array.isArray(geocodeResult.tool_results) ? geocodeResult.tool_results : []; - - if (toolResults.length === 0 || !toolResults[0]?.content) { + const geocodeResult = geocodeResultUnknown as { tool_results?: Array<{ content?: unknown }> }; + const toolResults = Array.isArray(geocodeResult.tool_results) ? geocodeResult.tool_results : []; + + if (toolResults.length === 0 || toolResults.every(r => r?.content == null)) { throw new Error('No content returned from mapping service'); } - let content = toolResults[0].content; + // Prefer the last tool result in case multiple were returned + let content: any = toolResults[toolResults.length - 1].content; - if (typeof content === 'string') { - const jsonRegex = /```(?:json)?\n?([\s\S]*?)\n?```/; - const match = content.match(jsonRegex); - if (match) { - content = match[1].trim(); - } - - try { - if (typeof content === 'string') { - content = JSON.parse(content); - } - } catch (parseError) { - console.warn('[GeospatialTool] Content is not JSON, using as string:', content); - } - } + // Handle common MCP content shapes + if (Array.isArray(content)) { + const jsonBlock = content.find((c: any) => c?.type === 'json' && c?.data); + if (jsonBlock) { + content = jsonBlock.data; + } else { + const textBlock = content.find((c: any) => c?.type === 'text' && typeof c?.text === 'string'); + if (textBlock?.text) { + const jsonRegex = /```(?:json)?\n?([\s\S]*?)\n?```/; + const match = textBlock.text.match(jsonRegex); + content = match ? match[1].trim() : textBlock.text; + } + } + } else if (typeof content === 'string') { + const jsonRegex = /```(?:json)?\n?([\s\S]*?)\n?```/; + const match = content.match(jsonRegex); + if (match) content = match[1].trim(); + try { + content = JSON.parse(content); + } catch { + console.warn('[GeospatialTool] Content is not JSON, using as string:', content); + } + }This makes parsing resilient to the typical MCP content variants and avoids false negatives on valid responses.
♻️ Duplicate comments (1)
lib/agents/tools/geospatial.tsx (1)
50-54: Fix invalid import path for mapbox_mcp_config.json (repeat of prior feedback)The path QCX/mapbox_mcp_config.json won’t resolve at runtime. The file is at the repo root; from lib/agents/tools/geospatial.tsx the relative path is ../../../mapbox_mcp_config.json.
Apply:
- const mapboxMcpConfig = await import('QCX/mapbox_mcp_config.json'); + const mapboxMcpConfig = await import('../../../mapbox_mcp_config.json');If your toolchain requires assertions for JSON dynamic imports, we can adjust accordingly, but the above mirrors the previously validated fix.
🧹 Nitpick comments (3)
lib/agents/tools/geospatial.tsx (3)
18-23: Gate environment logging behind non-production checkEven masked values can aid enumeration in logs. Restrict this to development to reduce exposure.
- console.log('[GeospatialTool] Environment check:', { - apiKey: apiKey ? `${apiKey.substring(0, 8)}...` : 'MISSING', - mapboxAccessToken: mapboxAccessToken ? `${mapboxAccessToken.substring(0, 8)}...` : 'MISSING', - profileId: profileId ? `${profileId.substring(0, 8)}...` : 'MISSING', - }); + if (process.env.NODE_ENV !== 'production') { + console.log('[GeospatialTool] Environment check:', { + apiKey: apiKey ? `${apiKey.substring(0, 8)}...` : 'MISSING', + mapboxAccessToken: mapboxAccessToken ? `${mapboxAccessToken.substring(0, 8)}...` : 'MISSING', + profileId: profileId ? `${profileId.substring(0, 8)}...` : 'MISSING', + }); + }
40-45: Validate UUID format cautiously and avoid logging full IDsTwo points:
- Not all Smithery profiles may be strict UUIDs; if Smithery ever permits non-UUID identifiers, this will block valid use. Please verify the assumption.
- Avoid logging the full profileId; mask it like you did above.
- if (!uuidRegex.test(profileId)) { - console.error('[GeospatialTool] Invalid profile ID format:', profileId); - return null; - } + if (!uuidRegex.test(profileId)) { + const masked = `${profileId.slice(0, 4)}…${profileId.slice(-4)}`; + console.error('[GeospatialTool] Invalid profile ID format:', masked); + return null; // If Smithery allows non-UUIDs, consider warning instead of failing. + }Would you like me to relax this to a warning if verification shows Smithery accepts non-UUID profiles?
143-145: Sanitize HTTP response details in logsIf connectionError.response?.data is large or contains sensitive details, this can spam logs or leak information. Consider trimming and redacting.
- httpStatus: connectionError.response?.status, - httpResponse: connectionError.response?.data, + httpStatus: connectionError.response?.status, + httpResponse: + typeof connectionError.response?.data === 'string' + ? connectionError.response.data.slice(0, 500) + '…' + : connectionError.response?.data + ? '[non-string payload redacted]' + : undefined,
📜 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(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
lib/agents/tools/geospatial.tsx (3)
mapbox_mcp/index.ts (1)
testMCPConnection(14-89)mapbox_mcp/hooks.ts (3)
mcp(65-81)query(91-156)mcp(83-89)components/map/map-query-handler.tsx (2)
GeospatialToolOutput(18-23)toolOutput(33-74)
🔇 Additional comments (8)
lib/agents/tools/geospatial.tsx (8)
228-231: Param rename to searchText aligns with server contractRenaming the tool arg from query to searchText matches the server’s expectation and resolves the validation error without changing behavior.
25-33: Solid presence check for required env varsClear, early return on missing env. This prevents downstream failures and simplifies the execute path.
76-79: HTTPS enforcement is good defensive validationThe explicit HTTPS check is a good guardrail before creating the transport.
95-98: Transport init looks correctUsing URL with StreamableHTTPClientTransport is appropriate and well-logged.
105-108: MCP client init is fineName/version set, defers connection until later.
115-126: Connection timeout via Promise.race is appropriate30s is reasonable; ensures the flow won’t hang indefinitely.
288-301: Mapping parsed payload to mcp_response is reasonableGracefully handles alt field names (place_name/name, address/formatted_address) and propagates mapUrl/map_url.
182-186: Confirmed: geospatialQuerySchema aligns with execute paramsThe schema defines:
- query — required non-empty string
- queryType — optional enum with default 'geocode'
- includeMap — optional boolean with default true
Files reviewed:
- lib/schema/geospatial.ts (contains the fields above plus optional coordinates/destination/radius/maxResults)
- lib/agents/tools/geospatial.tsx (execute signature: { query, queryType, includeMap })
No changes required.
| // Create Smithery URL with API key and profile ID | ||
| const mcpServerBaseUrl = `https://server.smithery.ai/@ngoiyaeric/mapbox-mcp-server?api_key=${apiKey}&profile=${profileId}`; | ||
| const smitheryUrlOptions = { config, apiKey, profileId }; | ||
| const mcpServerBaseUrl = `https://server.smithery.ai/mapbox-mcp-server/mcp?api_key=${smitheryUrlOptions.apiKey}&profile=${smitheryUrlOptions.profileId}`; | ||
| let serverUrlToUse: URL; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm createSmitheryUrl expectations and avoid duplicating credentials in the base URL
You’re passing api_key and profile in both the base URL and the createSmitheryUrl options. This can lead to duplication or mismatches. Prefer a clean base URL and let createSmitheryUrl handle query composition.
- const mcpServerBaseUrl = `https://server.smithery.ai/@ngoiyaeric/mapbox-mcp-server?api_key=${apiKey}&profile=${profileId}`;
+ const mcpServerBaseUrl = `https://server.smithery.ai/@ngoiyaeric/mapbox-mcp-server`;
const smitheryUrlOptions = { config, apiKey, profileId };Also, please verify whether createSmitheryUrl expects the /mcp path on the base, or appends it automatically.
🌐 Web query:
What is the correct base URL format for `createSmitheryUrl` from `@smithery/sdk`? Should the base include `/mcp`, and does `createSmitheryUrl` append `api_key` and `profile` query params when options include `apiKey` and `profileId`?
💡 Result:
The correct base URL format for createSmitheryUrl from @smithery/sdk should include the /mcp path if you are targeting an MCP (Managed Control Plane) Smithery server. When you use the function, it appends provided query parameters according to your options: if you supply apiKey and profileId, it will append corresponding api_key and profile query parameters to the final URL[3][1].
Base URL Format
- For MCP agents via Smithery, the base URL must contain
/mcp:- Example:
https://server.smithery.ai/neon/mcp[3]
- Example:
- To create a WebSocket URL, sometimes
/wsis appended at the end, but for HTTP/streamable agents, just/mcpis used[1][3].
Query Parameters
- If you pass options such as
apiKeyandprofileIdtocreateSmitheryUrl, the function adds them asapi_keyandprofilequery parameters to the generated URL[3].- Example:
createSmitheryUrl("https://server.smithery.ai/neon/mcp", { apiKey: "123123123", profileId: "bored-catshark-da8hf" }); // Result: https://server.smithery.ai/neon/mcp?profile=bored-catshark-da8hf&api_key=123123123
- Example:
This behavior is confirmed by code examples showing both the /mcp suffix and auto-appended query parameters for API key and profile[3].
Summary Table
| Aspect | Required Format | Example |
|---|---|---|
| Base URL | Must include /mcp if targeting MCP |
https://server.smithery.ai/neon/mcp |
| WebSocket URL | Add /ws as relevant for websocket |
https://server.smithery.ai/neon/ws |
| Query Params | Appends if options passed (apiKey, profileId) |
?profile=bored-catshark-da8hf&api_key=123123123 after /mcp |
References:
- [Code example with
/mcpand query params][3] - [Instruction to append
/wsfor WebSocket agents][1]
In summary, include /mcp in your base URL for MCP targets, and createSmitheryUrl will append api_key and profile query parameters automatically if apiKey and profileId are provided in the options[3][1].
Citations:
- 1: https://agentkit.inngest.com/integrations/smithery
- 2: https://smithy.io/2.0/aws/protocols/aws-query-protocol.html
- 3: https://github.com/inngest/agent-kit
- 4: https://docs.aws.amazon.com/bedrock-agentcore/latest/devguide/gateway-building-smithy-targets.html
- 5: https://github.com/smithery-ai/sdk/actions
- 6: https://learn.microsoft.com/en-us/graph/search-query-parameter
Include /mcp in the base URL and remove duplicated credentials — createSmitheryUrl will append them
createSmitheryUrl appends api_key and profile when given apiKey/profileId, and the base URL for MCP targets must include /mcp. Remove the query params from the base and add /mcp.
- File: lib/agents/tools/geospatial.tsx — lines 66-70
Suggested change:
- const mcpServerBaseUrl = `https://server.smithery.ai/@ngoiyaeric/mapbox-mcp-server?api_key=${apiKey}&profile=${profileId}`;
+ const mcpServerBaseUrl = `https://server.smithery.ai/@ngoiyaeric/mapbox-mcp-server/mcp`;
const smitheryUrlOptions = { config, apiKey, profileId };📝 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.
| // Create Smithery URL with API key and profile ID | |
| const mcpServerBaseUrl = `https://server.smithery.ai/@ngoiyaeric/mapbox-mcp-server?api_key=${apiKey}&profile=${profileId}`; | |
| const smitheryUrlOptions = { config, apiKey, profileId }; | |
| const mcpServerBaseUrl = `https://server.smithery.ai/mapbox-mcp-server/mcp?api_key=${smitheryUrlOptions.apiKey}&profile=${smitheryUrlOptions.profileId}`; | |
| let serverUrlToUse: URL; | |
| // Create Smithery URL with API key and profile ID | |
| const mcpServerBaseUrl = `https://server.smithery.ai/@ngoiyaeric/mapbox-mcp-server/mcp`; | |
| const smitheryUrlOptions = { config, apiKey, profileId }; | |
| let serverUrlToUse: URL; |
🤖 Prompt for AI Agents
In lib/agents/tools/geospatial.tsx around lines 66 to 70, the base Smithery URL
incorrectly includes query params for api_key and profileId and is missing the
required /mcp path; remove the query string from the base, append /mcp to the
path (e.g., .../mapbox-mcp-server/mcp), and rely on createSmitheryUrl to add
api_key and profile when given apiKey/profileId so you don't duplicate
credentials; keep smitheryUrlOptions and serverUrlToUse behavior unchanged so
createSmitheryUrl receives {config, apiKey, profileId} and produces the final
URL.
2 participants
User description
A geospatial request was sending a
queryparameter, but the server expectedsearchText. This caused a validation error.This commit renames the parameter to
searchTextto match the server's API, resolving the error.PR Type
Bug fix
Description
Fix parameter name mismatch in geospatial tool
Change
querytosearchTextfor server compatibilityDiagram Walkthrough
File Walkthrough
geospatial.tsx
Fix parameter name for server compatibilitylib/agents/tools/geospatial.tsx
queryparameter tosearchTextin toolArgs objectSummary by CodeRabbit
New Features
Bug Fixes
Chores