feat: optimization_tool renders as a live GL JS map (MCP Apps + MCP-UI)#191
feat: optimization_tool renders as a live GL JS map (MCP Apps + MCP-UI)#191mattpodwysocki wants to merge 7 commits into
Conversation
Third app tool in the MCP Apps pattern series, following directions and isochrones. The tool calls Mapbox's Optimization API with 2-12 stops and returns the optimized visit order plus the route geometry; the OptimizationAppUIResource renders the trip line with numbered (1, 2, 3, ...) markers at each stop in visit order, with the first stop colored green and the last red (if not a roundtrip), and the camera fit to all stops. Tightens the camera padding (top: 70, bottom/left/right: 30) to match the directions and isochrone tools so the route fills the viewport without manual zoom. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Same pattern as directions/isochrone — fit AFTER the host applies the size-changed notification so the trip fills the final viewport. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| }); | ||
|
|
||
| // Place numbered markers in optimized order | ||
| stops.forEach(function(stop, idx) { |
There was a problem hiding this comment.
Same marker-leak pattern as #189/#190, but N=12 markers per render here vs 2 in directions or 1 in isochrone — orphan stacks add up much faster. stops.forEach creates a mapboxgl.Marker per stop with no ref tracked. The trip-line layer/source are cleaned up correctly at l316-317, but a second ui/notifications/tool-result in the same iframe adds another 12 markers on top of the prior ones.
Fix: hold the marker array in module scope, forEach(m => m.remove()) and reset before the new loop.
There was a problem hiding this comment.
Fixed in 7511fda. The shared renderOptimizationAppHtml now holds stopMarkers in module scope, .remove()s each before the new forEach, and resets the array — so re-renders don't accumulate the 12-marker stack. Same pattern as the marker fixes in #189/#190. Post-refactor the file is src/resources/ui-apps/optimizationAppHtml.ts.
| .describe( | ||
| '"last" pins the last input coordinate as the end; "any" lets the optimizer choose.' | ||
| ), | ||
| roundtrip: z |
There was a problem hiding this comment.
Per Mapbox Optimization API docs, roundtrip=false requires both source='first' AND destination='last' (you can't not-roundtrip if either endpoint is flexible). The schema doesn't enforce this — so { roundtrip: false } alone with defaults (source='any', destination='any') passes Zod and Mapbox 422s with a generic InvalidInput that surfaces as Optimization API error: ... to the user with no hint about which combo was wrong.
One .refine() checking roundtrip === false → source === 'first' && destination === 'last' would convert this into a precise schema error before the network round-trip.
There was a problem hiding this comment.
Fixed in 7511fda. Added a .refine() on OptimizationInputSchema enforcing roundtrip !== false || (source === 'first' && destination === 'last'), with a precise message explaining the constraint. New test covers the rejection path.
| name: wp.name | ||
| })); | ||
|
|
||
| const durationMin = trip.duration |
There was a problem hiding this comment.
[nit] Same falsy-zero as DirectionsAppTool: trip.distance ? ... : 'unknown' reports unknown when distance/duration is exactly 0 (degenerate inputs like two identical coordinates with roundtrip=false). Use trip.distance != null (or typeof === 'number'). Same on the duration line just above.
There was a problem hiding this comment.
No longer applicable post-refactor. The OptimizationAppTool.ts text formatter that used trip.distance ? ... : 'unknown' was deleted when MCP App support was folded into the existing optimization_tool. The current OptimizationTool.ts divides directly because both fields are typed as non-optional numbers in OptimizationResponseSchema. The iframe's own summary builder (buildOptSummary) uses typeof === 'number' for the same reason.
| el.textContent = String(stop.order); | ||
|
|
||
| new mapboxgl.Marker({ element: el }) | ||
| .setLngLat(stop.location) |
There was a problem hiding this comment.
[nit] setLngLat(stop.location) has no guard for missing or non-array location. The tool's own output always emits [lng, lat] from validated input, but the iframe accepts tool-result from any postMessage source (no origin check yet, see #189). If one stop arrives with location: null or location: undefined, the forEach throws mid-loop — earlier markers render, later ones don't. The fitBounds block at l361 is outside the forEach so it still runs, but the loading/error UI state is left half-set. Cheap guard: if (!Array.isArray(stop.location) || stop.location.length < 2) return; inside the forEach.
There was a problem hiding this comment.
Fixed in 7511fda. The forEach now skips entries where stop.location isn't a numeric [lng, lat] pair, so a malformed payload skips the bad marker rather than throwing mid-loop and leaving the UI half-rendered.
| .setLngLat(stop.location) | ||
| .setPopup( | ||
| new mapboxgl.Popup().setText( | ||
| 'Stop ' + stop.order + (stop.name ? ' — ' + stop.name : '') + |
There was a problem hiding this comment.
[nit] The popup label 'Stop N — <stop.name>' uses wp.name from the Mapbox Optimization API response, which per docs is the road name the input coordinate was snapped to, not a place name. A user who passed coordinates for 'Starbucks on Main' and 'Trader Joe's on Oak' will see Stop 1 — Main Street / Stop 2 — Oak Avenue and likely read those as place identifiers (the tool 'identified my Starbucks as Main Street'). Two cheap options: (a) drop name and just show 'Stop N (input #N)', or (b) relabel it as 'on <road>' so it's clearly a road, not a place.
There was a problem hiding this comment.
Fixed in 7511fda (went with option b). The popup now reads Stop N (input #N) — on <road> when stop.name is present, so it's clearly a road the input snapped to rather than a place identifier. Code comment in the loop body documents why.
# Conflicts: # CHANGELOG.md # src/tools/index.ts
…tion_app_tool) Same pattern as the directions/isochrone refactors. Drops the sibling optimization_app_tool; the existing optimization_tool now emits both meta.ui.resourceUri (MCP Apps) and inline rawHtml (MCP-UI). Shared optimizationAppHtml.ts renders the trip line with numbered visit-order markers; the iframe normalizes both GeoJSON and polyline-encoded geometry inputs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
UI fixes in optimizationAppHtml.ts:
- Guard stop.location: skip stops whose location is missing or not a
[lng, lat] pair, instead of throwing setLngLat mid-loop. The iframe
accepts tool-result from any postMessage source, so a malformed
payload would have left earlier markers rendered and the rest absent.
- Relabel the stop popup as "on <road>" instead of "Stop N — <name>".
The Mapbox Optimization API populates waypoint.name with the road
name the input coordinate snapped to, not a place name, so the prior
label implied the tool had identified a place ("Stop 1 — Main Street"
when the user passed a Starbucks coordinate).
Schema refinement in OptimizationTool.input.schema.ts:
- Refuse roundtrip=false unless source='first' AND destination='last'.
The Mapbox API requires both endpoints to be fixed for a one-way trip
and otherwise returns a generic InvalidInput 422. Converts that into
a precise Zod error before the network round-trip.
One new test covers the refinement.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Superseded by #199. This PR (and the rest of the per-tool app stack: #190, #191, #192, #193, #194, #195) declares its own
#199 funnels all rendering through one terminal |
Same pattern as #189/#190 applied to optimization_tool: drops optimization_app_tool, adds meta.ui + inline MCP-UI to the existing tool, shared HTML template. 715 tests pass. Stacked on #190.