feat(ag-ui): App-mode Google Map cockpit — AdvancedMarker + fit-to-bounds + dark style#738
feat(ag-ui): App-mode Google Map cockpit — AdvancedMarker + fit-to-bounds + dark style#738blove wants to merge 15 commits into
Conversation
…Marker + crash fix) Two-PR plan for the Phase-2 map cockpit follow-up: PR-A lands the already-built blank-page-on-reload crash fix (GoogleMapsLoader gate); PR-B adds fit-to-bounds, migrates google.maps.Marker → AdvancedMarkerElement with a cloud-based dark map style (DEMO_MAP_ID fallback), preserving flat day-colored dot pins. Grey-map is documented as environmental (tile-quota throttling), not a code bug. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… + AdvancedMarker) 7-task TDD plan layering on PR #736: pure computeBounds helper + unit test, fit-to-bounds effect with 0/1/≥2 fallbacks + loader-gate no-op guard, GOOGLE_MAPS_MAP_ID env plumbing, marker-library load, MapMarker→MapAdvancedMarker with day-colored div pins + mapId + DEMO_MAP_ID fallback, README, and live map smoke. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Claude finished @blove's task in 5m 10s —— View job Code Review
Overall this is a clean, well-structured migration. The One correctness bug found (see inline comment):
Two minor observations (see inline comments):
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| `; | ||
| writeFileSync(targetPath, contents); | ||
| console.log(`[inject-env] wrote generated-keys.local.ts (key length: ${key.length})`); | ||
| console.log(`[inject-env] wrote generated-keys.local.ts (key length: ${key.length}, mapId: ${mapId ? 'set' : 'unset'})`); |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (stops.length === 0) { | ||
| this.center.set(PARIS_CENTER); | ||
| this.zoom.set(12); | ||
| return; |
There was a problem hiding this comment.
Signal staleness bug — map won't reset to Paris after fitBounds + remove-all
After fitBounds() fires (≥2 stops), center() and zoom() signals are never updated — they stay at whatever value they held before (e.g., the initial PARIS_CENTER object and 12). Later, when all stops are removed, this branch calls this.center.set(PARIS_CENTER) and this.zoom.set(12).
Because Angular signals use Object.is() for equality:
Object.is(PARIS_CENTER, PARIS_CENTER)→true(same object reference) → no notification emittedObject.is(12, 12)→true→ no notification emitted
GoogleMap's ngOnChanges never fires, so the map stays at the fitBounds viewport instead of resetting to Paris/zoom-12.
Reproducer: add 2+ stops (fitBounds fires) → remove all stops → map doesn't return to Paris.
Fix: call the underlying API directly, consistent with how fitBounds is already imperative:
if (stops.length === 0) {
map.googleMap?.setCenter(PARIS_CENTER);
map.googleMap?.setZoom(12);
return;
}The 1-stop branch is fine as-is: this.center.set({ lat: ..., lng: ... }) produces a fresh object, so Object.is returns false and the signal propagates.
| protected readonly markerViews = computed(() => | ||
| this.stopsWithCoords().map((s) => ({ | ||
| id: s.id, | ||
| stop: s, | ||
| position: { lat: s.lat!, lng: s.lng! }, | ||
| content: this.makePin(s.day), | ||
| })), | ||
| ); |
There was a problem hiding this comment.
All pins rebuilt on any structural change — unchanged markers may briefly repaint
The comment says pins are "recreated only when stops change — not on every CD pass", which is accurate. But "when stops change" means every stopsWithCoords() emission calls makePin for all stops, not just the affected one.
For stops whose m.id is unchanged, @for track m.id keeps the MapAdvancedMarker component alive, but Angular still calls ngOnChanges({ content: newElement }) with the freshly-created HTMLElement. The underlying AdvancedMarkerElement replaces its content DOM node, causing a repaint for every existing marker whenever a single stop is added, removed, or geocoded.
For a demo app this is fine, but the comment overstates the optimization slightly. A Map<id, HTMLElement> cache (preserved across computed invocations via a class field) could limit rebuilds to only the changed stops if flicker becomes noticeable.
| east = Math.max(east, s.lng); | ||
| west = Math.min(west, s.lng); |
There was a problem hiding this comment.
Anti-meridian caveat (not a bug for typical itineraries)
Min/max longitude works correctly for routes within a single hemisphere, but produces a globe-spanning bounding box for itineraries that cross the anti-meridian (e.g. Tokyo lng: 140 → Honolulu lng: -158). fitBounds on such a box would zoom the map out to show the entire world.
Not an issue for a Paris-centric travel demo, but worth a short comment if this helper gets reused for international routes. The proper fix would use google.maps.LatLngBounds with its extend() method, which handles anti-meridian crossing natively.
… hides The map is full-bleed under the floating itinerary overlay (left) and the chat sidebar drawer (right), but fitBounds used uniform 48px padding — so it framed stops across the whole width and the rightmost marker landed under the chat rail. Measure each panel's footprint live (adapts to the drawer's open/closed state + responsive widths) and pass it as asymmetric fitBounds padding; fall back to uniform 48 when the panels are absent (unit tests / non-App-mode). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
App mode previously forced the sidebar route and disabled the other mode buttons. New rule: App mode layers over the map via the sidebar (right rail) OR popup (floating bubble), so switching between those keeps App mode on; embed is full-bleed chat that would cover the map, so selecting it turns App mode off. The persist effect now restores the actual mode on reload (seeding mode() from location.pathname to dodge the bootstrap race) and coerces a stray embed+appmode=on to sidebar. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Claude finished @blove's task in 3m 51s —— View job Code Review
SummaryThe core PR work (AdvancedMarker migration, One correctness bug (unresolved from prior review)
New commits look good
App-mode/popup coexistence (commit CodeQL alert ( The flagged Pre-existing minor observations (already noted by prior review)
|
| if (stops.length === 0) { | ||
| this.center.set(PARIS_CENTER); | ||
| this.zoom.set(12); | ||
| return; |
There was a problem hiding this comment.
Signal staleness bug still open — map won't reset to Paris after fitBounds + remove-all
The prior review flagged this and it hasn't been fixed yet. Confirming it's still present in the latest commits.
When fitBounds fires (≥2 stops), it calls the imperative Maps API directly — the center and zoom signals are never updated. They stay at their initial values (PARIS_CENTER / 12). Later, when all stops are removed:
this.center.set(PARIS_CENTER)→Object.is(PARIS_CENTER, PARIS_CENTER)→true→ no notification emittedthis.zoom.set(12)→Object.is(12, 12)→true→ no notification emitted
GoogleMap.ngOnChanges never fires; the map stays at the fitBounds viewport.
Minimal repro: seed with ≥2 stops (fitBounds fires) → remove all stops → map doesn't return to Paris.
Fix: call the imperative API directly, consistent with fitBounds:
| return; | |
| if (stops.length === 0) { | |
| map.googleMap?.setCenter(PARIS_CENTER); | |
| map.googleMap?.setZoom(12); | |
| return; | |
| } |
The 1-stop branch is fine: { lat: ..., lng: ... } is always a fresh object, so Object.is returns false and the signal propagates normally.
…t full width)
In Sidebar mode the chat drawer is a solid right rail, but the full-bleed map
extended under it, so fitBounds framed stops across the hidden width. Inset the
map by the chat lib's published drawer footprint (--ngaf-chat-occupy-right) so
it occupies its own column and markers center within the visible area; Popup
mode mounts no drawer, so the var is 0 and the map stays full-bleed under the
floating bubble. The descendant selector + width:auto beat the map component's
own :host { width: 100% } (an over-constrained left/right/width otherwise keeps
the full width and silently ignores the inset).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Claude finished @blove's task in 2m 15s —— View job Code Review
The new commits ( One correctness bug (flagged in all three reviews — still unresolved)
CodeQL alert (
New commits look good
Pre-existing minor observations (already noted, not blocking)
|
| if (stops.length === 0) { | ||
| this.center.set(PARIS_CENTER); | ||
| this.zoom.set(12); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Signal staleness bug (third review — still open)
this.center.set(PARIS_CENTER) and this.zoom.set(12) are no-ops when the signals are already at their initial values. fitBounds is called imperatively and never updates these signals. So after a fitBounds frame followed by removing all stops:
Object.is(PARIS_CENTER, PARIS_CENTER)→true(same const reference) → no notificationObject.is(12, 12)→true→ no notification
GoogleMap.ngOnChanges never fires; the viewport stays where fitBounds left it.
Reproducer: add ≥2 stops → fitBounds fires → remove all stops → map stays at the fitBounds viewport instead of returning to Paris/zoom-12.
Fix — call the imperative API directly, consistent with how fitBounds is already called:
| if (stops.length === 0) { | |
| this.center.set(PARIS_CENTER); | |
| this.zoom.set(12); | |
| return; | |
| } | |
| if (stops.length === 0) { | |
| map.googleMap?.setCenter(PARIS_CENTER); | |
| map.googleMap?.setZoom(12); | |
| return; | |
| } |
The 1-stop branch (this.center.set({ lat: ..., lng: ... })) is fine: a fresh object literal always fails Object.is, so the signal propagates normally.
| `; | ||
| writeFileSync(targetPath, contents); | ||
| console.log(`[inject-env] wrote generated-keys.local.ts (key length: ${key.length})`); | ||
| console.log(`[inject-env] wrote generated-keys.local.ts (key length: ${key.length}, mapId: ${mapId ? 'set' : 'unset'})`); |
There was a problem hiding this comment.
CodeQL: clear-text logging — still flagged
key.length is not the key itself, so this is a false positive semantically. But the scanner sees a variable derived from GOOGLE_MAPS_API_KEY passed to console.log and flags it. The mapId ? 'set' : 'unset' pattern you added for the new mapId is exactly the right idiom — applying the same treatment to the key reference silences the alert with zero behaviour change:
| console.log(`[inject-env] wrote generated-keys.local.ts (key length: ${key.length}, mapId: ${mapId ? 'set' : 'unset'})`); | |
| console.log(`[inject-env] wrote generated-keys.local.ts (key: ${key ? 'set' : 'unset'}, mapId: ${mapId ? 'set' : 'unset'})`); |
|
Claude finished @blove's task in 2m 14s —— View job Code Review
SummaryThe new commits (fitPadding panel-aware insets, App-mode/popup coexistence, map column fix) all look correct and are not blocking. Two issues flagged in prior reviews remain open and are addressed by one-click inline suggestions above. Two issues — both have inline code suggestions above1. Signal staleness bug —
2. CodeQL alert —
Everything else looks good
|
| if (stops.length === 0) { | ||
| this.center.set(PARIS_CENTER); | ||
| this.zoom.set(12); | ||
| return; |
There was a problem hiding this comment.
Signal staleness bug (4th review — still unresolved)
this.center.set(PARIS_CENTER) and this.zoom.set(12) are no-ops here. fitBounds is called imperatively and never updates these signals, so after a fitBounds frame followed by removing all stops:
Object.is(PARIS_CENTER, PARIS_CENTER)→true(same const reference) → no notificationObject.is(12, 12)→true→ no notification
GoogleMap.ngOnChanges never fires; the viewport stays at the fitBounds position.
Reproducer: add ≥2 geocoded stops → fitBounds fires → remove all stops → map stays at the fitBounds viewport instead of resetting to Paris/zoom-12.
map is already in scope (line 140) and guarded non-null at line 142, so the fix is one line per call:
| if (stops.length === 0) { | |
| this.center.set(PARIS_CENTER); | |
| this.zoom.set(12); | |
| return; | |
| if (stops.length === 0) { | |
| map.googleMap?.setCenter(PARIS_CENTER); | |
| map.googleMap?.setZoom(12); | |
| return; | |
| } |
The 1-stop branch (this.center.set({ lat: ..., lng: ... })) is fine — a fresh object literal always fails Object.is, so the signal propagates correctly.
| `; | ||
| writeFileSync(targetPath, contents); | ||
| console.log(`[inject-env] wrote generated-keys.local.ts (key length: ${key.length})`); | ||
| console.log(`[inject-env] wrote generated-keys.local.ts (key length: ${key.length}, mapId: ${mapId ? 'set' : 'unset'})`); |
There was a problem hiding this comment.
CodeQL: clear-text logging — still flagged
key.length is semantically safe (not the key itself), but CodeQL flags any console.log that references a variable derived from GOOGLE_MAPS_API_KEY. Apply the same ? 'set' : 'unset' idiom already used for mapId to silence the alert with no behaviour change:
| console.log(`[inject-env] wrote generated-keys.local.ts (key length: ${key.length}, mapId: ${mapId ? 'set' : 'unset'})`); | |
| console.log(`[inject-env] wrote generated-keys.local.ts (key: ${key ? 'set' : 'unset'}, mapId: ${mapId ? 'set' : 'unset'})`); |
…e/occupy hack) App mode now renders the map inside SidebarMode's <chat-sidebar> content slot, so it fills the area left of the drawer via the lib's flex layout (PR #740) — no shell-level absolute positioning or --ngaf-chat-occupy-right inset. Popup mode keeps the full-bleed shell map. Adds a ResizeObserver in MapCanvasComponent that re-triggers google.maps resize + re-fit when the content slot resizes (the push shrink / mode toggles), so tiles don't render grey at the stale size. NEEDS VERIFICATION in a reliable browser: the agent's flaky headless/extension tooling renders the map only intermittently. One good render confirmed the 981px column + vector map + polyline, but advanced-marker pins did not attach in that pass — unclear if real or a tooling artifact. Verify pins + tiles + the sidebar↔popup toggle before merging. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Claude finished @blove's task in 1m 41s —— View job Code Review — Fixes Ready (Push Blocked)
Both fixes were applied and committed locally but the bot cannot push to Fix 1 — Signal staleness bug (
|
2 participants
What
Phase 2 of the AG-UI itinerary redesign: the App-mode Google Map cockpit. Builds on PR-A (#736, merged — the
<google-map>loader-gate that fixed the blank-page-on-reload crash).computeBoundspure helper (map-bounds.ts) — derives aLatLngBoundsfrom coord'd stops; fully unit-tested.stopsWithCoords()only so reframing never fights focus-panning. 0 stops → Paris/zoom-12; 1 stop → center/zoom-13; ≥2 →fitBounds.<map-advanced-marker>with per-day colored dot pins (the pin<div>is built inmarkerViewsso it only rebuilds when stops change, not on every CD pass), replacing the deprecatedgoogle.maps.Marker. Loads themarkerlibrary.mapId(advanced markers require one; amapIdmap ignores inline JSONstyles, so the dark theme is a cloud style tied to the id).GOOGLE_MAPS_MAP_IDplumbed through the env mechanism;DEMO_MAP_IDfallback lets a fresh clone run a light map with no Console setup.GOOGLE_MAPS_API_KEY+GOOGLE_MAPS_MAP_ID.Verification
nx lint+nx test(47 passing, incl.map-bounds5/5) +nx buildforexamples-ag-ui-angular— all green on this branch after syncing currentmain.map-advanced-marker×3 mounted,mapIdapplied withisAdvancedMarkersAvailable: true,markerViewscomputed from seeded stops, App mode forces/sidebar, no console errors from our code.Known: live map tiles pending demo-project billing
The pixel-level render (dark tiles drawing) is not yet confirmed because the Maps demo GCP project currently can't serve tiles — a bare
new google.maps.Map()(no Map ID) also staysUNINITIALIZED, which is the billing-not-enabled signature, independent of this code. Once billing is linked on that project the same key + Map ID render. This is a project-provisioning gate, external to the PR.🤖 Generated with Claude Code