Conversation
|
Merged main into this branch to resolve conflicts. Resolution:
The two skills have different focuses:
PR is now ready for review with all conflicts resolved. |
|
Code Review: add-navigation-patterns-skill Summary This PR adds a new mapbox-navigation-patterns skill covering Directions API, Optimization API, and Navigation SDKs for iOS/Android. The web Critical Issues
File: README.md:1 The README says "12 comprehensive Agent Skills" but there are actually 13 skill directories with SKILL.md files. This was likely correct before
Files: SKILL.md:447-666, AGENTS.md:101-171 All iOS Swift code examples use the Navigation SDK v2 API. The current GA release is v3 (v3.18.1) with major breaking changes: The voice guidance section (SKILL.md:641-666) appears fabricated -- navigationService.voiceController with .volume = .normal / .muted / Note: v2 (latest v2.20.3) is still maintained, so the code would work for v2 users. But new projects should target v3, and a patterns skill should Sources: https://docs.mapbox.com/ios/navigation/guides/migration/, https://github.com/mapbox/mapbox-navigation-ios/releases
Files: SKILL.md:668-827, AGENTS.md:173-222 The custom navigation example (SKILL.md:706-827) is mostly correct for both v2 and v3: NavigationRouterCallback, RouteProgressObserver, Sources: https://docs.mapbox.com/android/navigation/build-with-nav-sdk/migration-from-v2/
File: AGENTS.md:324-331
|
Fixed 8 issues identified in review: 1. Optimization API bug: Removed numeric index fallback, only use 'first'/'any' for source and 'last'/'any' for destination 2. Added missing async keyword to getCachedRoute function in AGENTS.md 3. Added missing 'unknown' congestion value to traffic styling match expression 4. Fixed API Limits table: Changed "Up to 3" to "Max 2 alternatives (3 total routes)", clarified Optimization v1 hard limit 5. Fixed skills README alphabetical ordering: moved mapbox-navigation-patterns between maplibre-migration and search-integration 6. Updated iOS Navigation SDK to v3 API: - Changed imports: MapboxNavigation → MapboxNavigationUIKit, MapboxCoreNavigation → MapboxNavigationCore - Replaced Directions.shared.calculate() callbacks with async/await routingProvider.calculateRoutes() - Updated NavigationViewController initialization with navigationRoutes and navigationOptions - Replaced MapboxNavigationService with MapboxNavigationProvider - Converted NavigationServiceDelegate callbacks to Combine publishers - Updated voice guidance configuration to use CoreConfig.ttsConfig 7. Updated Android Navigation SDK to v3 API: - Removed NavigationView examples (dropped in v3) - Removed api.startArrival() method (not documented) - Removed .accessToken() method (removed in v3) - Changed onDestroy() to use MapboxNavigationProvider.destroy() - Updated to v3-compatible patterns with requestRoutes() and RouteProgressObserver 8. All code examples now use current v3 SDK APIs for both iOS and Android Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
|
||
| | Need | Solution | | ||
| | --------------------------- | -------------------------- | | ||
| | **Web routing** | Directions API | |
There was a problem hiding this comment.
web routing sounds like weird phrasing
|
|
||
| ```javascript | ||
| const query = await fetch( | ||
| `https://api.mapbox.com/directions/v5/mapbox/driving/` + |
There was a problem hiding this comment.
should be using driving-traffic by default to avoid congestion and road closures
| const waypoints = [start, stop1, stop2, stop3, end]; | ||
| const coords = waypoints.map((w) => `${w[0]},${w[1]}`).join(';'); | ||
|
|
||
| const url = `https://api.mapbox.com/directions/v5/mapbox/driving/${coords}?...`; |
There was a problem hiding this comment.
I would replace all driving references with driving-traffic in directions API references
| const order = json.waypoints.map((wp) => wp.waypoint_index); | ||
| ``` | ||
|
|
||
| ### Traffic-Aware Routing |
There was a problem hiding this comment.
also seems weird we make this as a separate section. I think more natural way to say this is - use driving-traffic profile by default for realtime traffic data, including congestion and incidents aware routing.
If you care about depart-at or arrive-by parameters, you have to use driving profile instead
|
|
||
| const map = new mapboxgl.Map({ | ||
| container: 'map', | ||
| style: 'mapbox://styles/mapbox/streets-v12', |
There was a problem hiding this comment.
could be better to refer to mapbox standard by default, we want it to be adopted more widely
|
|
||
| async function getRoute(start, end) { | ||
| const query = await fetch( | ||
| `https://api.mapbox.com/directions/v5/mapbox/driving/${start[0]},${start[1]};${end[0]},${end[1]}?` + |
There was a problem hiding this comment.
driving should only be used with arrive-by or depart-at because in every other situation driving-traffic is better (based on current traffic conditions, avoids congestion and incidents)
| async function getRoute(start, end) { | ||
| const query = await fetch( | ||
| `https://api.mapbox.com/directions/v5/mapbox/driving/${start[0]},${start[1]};${end[0]},${end[1]}?` + | ||
| `steps=true&geometries=geojson&access_token=${mapboxgl.accessToken}`, |
There was a problem hiding this comment.
geometries=geojson is actually the worst performance wise, unless you cannot use any other format
| ```javascript | ||
| const query = await fetch( | ||
| `https://api.mapbox.com/directions/v5/mapbox/driving/` + | ||
| `${start[0]},${start[1]};${end[0]},${end[1]}?` + |
There was a problem hiding this comment.
Should specify that this is in lon, lat order - otherwise computers will screw it up just like we humans do.
| ```javascript | ||
| // Up to 25 waypoints | ||
| const waypoints = [start, stop1, stop2, stop3, end]; | ||
| const coords = waypoints.map((w) => `${w[0]},${w[1]}`).join(';'); |
|
Thanks for the thorough review @ctufts! This is incredibly detailed and helpful. Let me address each point: Critical Issues - Mobile SDK VersionsiOS/Android v2 vs v3: You're absolutely right that the examples target v2 APIs. I have two options:
What's your preference? I'm leaning toward option 1 (update to v3) since this is a patterns skill that should reflect current best practices. The migration work would involve: iOS v3 changes:
Android v3 changes:
Voice guidance section: You caught that - I'll remove or rewrite it to match actual v3 APIs. Issues I'll Fix Immediately4. API Limits table:
5. Optimization API code bug: 6. Missing async keyword: 7. Add 'unknown' to congestion values: 8. Mention Optimization v2: 9. Fix skills/README.md ordering: PlanLet me know your preference on v2 vs v3 for mobile SDKs, and I'll:
Should I proceed with updating to v3? |
|
Updates pushed! Here's what I found: Already Fixed ✅Great news - most issues were already addressed in the latest version: 1. iOS Navigation SDK v3: Already using current v3 APIs
2. Android Navigation SDK v3: Already using current v3 APIs
3. Optimization API: Already fixed - uses proper enums
4. API Limits table: Already correct
5. getCachedRoute: Already has 6. skills/README.md ordering: Already alphabetically correct (mapbox-maplibre-migration → mapbox-navigation-patterns → mapbox-search-integration) Just Fixed 🔧7. Added 'unknown' congestion value
8. Added Optimization v2 mention
SummaryThe code was already updated to v3 and all the critical/moderate issues were already resolved. I just added the missing documentation notes. Ready for re-review! |
Adds navigation and routing patterns skill covering the Directions API and Navigation SDKs for web, iOS, and Android, rebased onto main. Also adds **/build/ to .prettierignore to prevent Prettier from trying to parse Android build artifacts in the demos directory. Co-Authored-By: mattpodwysocki <mattpodwysocki@gmail.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
mattpodwysocki
force-pushed
the
add-navigation-patterns-skill
branch
from
10311db to
a5fc743
Compare
- optimization-v2 URL 404s; replaced with optimization (v1) URL - simplestatistics.org domain-squatted; replaced with GitHub Pages URL Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
4 participants
Summary
Adds comprehensive navigation and routing skill covering Directions API and Navigation SDKs across all platforms.
What's Included
SKILL.md - Comprehensive navigation guide covering:
AGENTS.md - Compressed quick reference (2-4KB) with:
Patterns Covered
Web (Directions API)
iOS (Navigation SDK)
Android (Navigation SDK)
Use Cases
Testing
Part of tutorial-based skills series. Next: mapbox-store-locator-patterns, mapbox-data-visualization-patterns.