Skip to content

fix(ag-ui): gate <google-map> on Maps API load (blank-page-on-reload)#736

Merged
blove merged 4 commits into
mainfrom
worktree-ag-ui-app-mode-mapfix
Jun 25, 2026
Merged

fix(ag-ui): gate <google-map> on Maps API load (blank-page-on-reload)#736
blove merged 4 commits into
mainfrom
worktree-ag-ui-app-mode-mapfix

Conversation

@blove

@blove blove commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes a blank-page crash in the AG-UI demo: reloading with App mode persisted on rendered only the toolbar (empty body, no console error).

Root cause (confirmed against @angular/google-maps source, google-maps.mjs:136): <google-map>'s constructor throws "Namespace google not found" when window.google is absent. The Maps JS API is injected asynchronously at bootstrap, so on a fresh load it loses the race — the map throws during the shell's initial change-detection pass and aborts the whole render. It's dev-mode-only (the throw is ngDevMode-gated), which is why #732's CI and the Phase-2 live smoke missed it (the smoke only ever toggled App mode at runtime, when the script had already loaded — never reloaded with it persisted on).

Fix: a small GoogleMapsLoader service exposing a loaded signal (it owns the script injection and flips loaded on script.onload). <google-map> is gated with @if (loader.loaded()) so it never constructs before the API is present. app.config loads eagerly through the same service so the GeocodingService keeps working. markerOptions uses the numeric SymbolPath literal (0) as defense-in-depth against eager google.* reads.

Test plan

  • Lint clean (0 errors), 42/42 unit tests, dev + prod build green
  • Verified locally: fresh reload with App mode on now renders the full shell (panel, mode labels, chat) instead of a blank toolbar — no render abort, no console error
  • Rebased on latest main

Known separate issue (NOT addressed here)

There's a separate, pre-existing issue where the map can render a grey base (overlays draw, dark tiles don't). It is independent of this crash fix (reproduces without it), and its cause is still under investigation — needs a Chrome DevTools Network look at the /maps/vt tile HTTP statuses (200 → render/size bug; 403/429 → key/quota). Tracked separately. This PR strictly improves the reload path (blank page → rendered).

🤖 Generated with Claude Code

…crash)

Reloading the demo with App mode persisted on rendered a blank page (only
the toolbar). Root cause (confirmed against @angular/google-maps
google-maps.mjs:136): the GoogleMap constructor throws "Namespace google
not found" when window.google is absent — and the Maps script loads
async, so on a fresh load it loses the race, the map throws during the
shell's initial render, and the whole shell render aborts (dev-mode only,
ngDevMode-gated, which is why #732 CI/smoke missed it; the P2 smoke only
toggled App mode at runtime, never reloaded with it persisted on).

Fix: a GoogleMapsLoader service exposing a `loaded` signal (owns the
script injection, flips on script.onload); gate <google-map> with
@if (loader.loaded()) so it never constructs before the API is present.
app.config loads eagerly through the same service so the GeocodingService
keeps working. markerOptions uses the numeric SymbolPath literal (0) as
defense-in-depth against eager google.* reads.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@vercel

vercel Bot commented Jun 24, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
threadplane Ready Ready Preview, Comment Jun 25, 2026 4:05am

Request Review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated approval: this PR received an intelligent (AI) code review. See the review comments on this PR.

@github-actions

github-actions Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Claude finished @blove's task in 1m 43s —— View job


Code Review

  • Gather context (PR description, changed files)
  • Read changed files and diff
  • Analyze for correctness, security, and Angular pitfalls
  • Post inline review comments
  • Post top-level summary

Summary

The fix is correct and well-reasoned. Gating <google-map> behind @if (loader.loaded()) is the right approach — it directly addresses the root cause (@angular/google-maps throwing in its constructor before window.google is present). The service design is clean: providedIn: 'root' ensures one instance, the started flag prevents double-injection, and calling ensureLoaded() from both app.config's initializer and the component constructor is safe and idempotent.

Two issues in GoogleMapsLoader:

  1. setInterval leak (lines 41–49): the polling loop that handles a pre-existing <script> tag is never cancelled if google.maps never arrives (e.g. prior script failed). Inline comment with a suggested fix posted. Fix this →

  2. No onerror handler (lines 52–57): if the Maps JS API request fails (bad key, quota exceeded, network), loaded stays false forever — silent failure with no developer visibility. Inline comment with a suggested fix posted. Fix this →

  3. No unit tests for GoogleMapsLoader: the idempotency contract, the polling path, and the new loaded signal gate are untested. The existing 42 tests don't cover the new service — worth adding at least a basic spec for the happy path and the no-key short-circuit.

Everything else looks good: the path: 0 literal is correct (avoids eager google.maps.SymbolPath reads), signal.set() from async callbacks is fine in zoneless mode, and the encodeURIComponent on the API key is properly applied.

Comment on lines +41 to +49
if (existing) {
// A load is already in flight (e.g. a prior instance). Poll for readiness.
const poll = setInterval(() => {
if ((globalThis as { google?: { maps?: unknown } }).google?.maps) {
clearInterval(poll);
this.loaded.set(true);
}
}, 100);
return;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setInterval here is never cancelled if google.maps never becomes available (e.g. the existing script tag's network request failed, or it injected a script with a bad key). This leaks indefinitely.

Consider a bounded retry or a script.onerror listener on the original element:

Suggested change
if (existing) {
// A load is already in flight (e.g. a prior instance). Poll for readiness.
const poll = setInterval(() => {
if ((globalThis as { google?: { maps?: unknown } }).google?.maps) {
clearInterval(poll);
this.loaded.set(true);
}
}, 100);
return;
if (existing) {
// A load is already in flight (e.g. a prior instance). Poll for readiness.
let attempts = 0;
const poll = setInterval(() => {
if ((globalThis as { google?: { maps?: unknown } }).google?.maps) {
clearInterval(poll);
this.loaded.set(true);
} else if (++attempts > 100) { // give up after ~10 s
clearInterval(poll);
}
}, 100);
return;
}

Fix this →

Comment on lines +52 to +57
const script = doc.createElement('script');
script.src = `https://maps.googleapis.com/maps/api/js?key=${encodeURIComponent(key)}&libraries=geocoding`;
script.async = true;
script.setAttribute('data-google-maps', '');
script.addEventListener('load', () => this.loaded.set(true));
doc.head.appendChild(script);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No error event handler — if the script request fails (network error, invalid/quota-exceeded API key), loaded stays false forever and the map area remains blank with no indication to the user or developer.

Suggested change
const script = doc.createElement('script');
script.src = `https://maps.googleapis.com/maps/api/js?key=${encodeURIComponent(key)}&libraries=geocoding`;
script.async = true;
script.setAttribute('data-google-maps', '');
script.addEventListener('load', () => this.loaded.set(true));
doc.head.appendChild(script);
const script = doc.createElement('script');
script.src = `https://maps.googleapis.com/maps/api/js?key=${encodeURIComponent(key)}&libraries=geocoding`;
script.async = true;
script.setAttribute('data-google-maps', '');
script.addEventListener('load', () => this.loaded.set(true));
script.addEventListener('error', () => {
this.started = false; // allow a future retry
console.error('[GoogleMapsLoader] Failed to load Maps JS API');
});
doc.head.appendChild(script);

Fix this →

In App mode the Google Map is the full-bleed background and the chat
sidebar floats over it, so the placeholder "Use the launcher…" hint
(projected into chat-sidebar's default content slot) floated unreadably
over the map. Gate it on appMode !== 'on' so it only shows in plain
sidebar mode where the area behind the chat is genuinely empty.

Verified via DOM probe: /sidebar appMode off → hint present (1);
/sidebar?appmode=on with the map mounted → hint absent (0).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Claude finished @blove's task in 1m 50s —— View job


Code Review

  • Gather context (PR description, changed files)
  • Read changed files and diff
  • Check if prior review issues were addressed
  • Analyze for correctness, security, and Angular pitfalls
  • Post inline review comments
  • Post top-level summary

Summary

This synchronize was a rebase on main — the two issues flagged in the prior review are still unaddressed. Re-raised as inline comments with code suggestions on the new commit. Everything else in the PR is correct.

Still needs fixing in google-maps-loader.ts:

  1. Unbounded setInterval leak (line 49): the polling loop for the pre-existing-script path never cancels if google.maps never arrives (e.g. prior script failed). Inline suggestion posted. Fix this →

  2. No onerror handler on the injected script (line 57): if the Maps JS API fails to load (bad key, quota exceeded, network), loaded stays false forever and started stays true, so there is no retry path and no developer-visible error. Inline suggestion posted. Fix this →

Everything else looks good:

  • @if (loader.loaded()) gate is the correct fix for the root cause
  • path: 0 (numeric SymbolPath.CIRCLE) safely avoids eager google.* reads
  • provideEnvironmentInitializer(() => inject(GoogleMapsLoader).ensureLoaded()) is idempotent and correct
  • shell.appMode() !== 'on' guard on the hint text is a proper signal comparison
  • signal.set() from async callbacks is fine in zoneless mode

this.loaded.set(true);
}
}, 100);
return;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setInterval is never cancelled if google.maps never becomes available (e.g. the existing script's network request failed). This leaks indefinitely — add a bounded retry:

Suggested change
return;
let attempts = 0;
const poll = setInterval(() => {
if ((globalThis as { google?: { maps?: unknown } }).google?.maps) {
clearInterval(poll);
this.loaded.set(true);
} else if (++attempts > 100) { // give up after ~10 s
clearInterval(poll);
}
}, 100);

script.async = true;
script.setAttribute('data-google-maps', '');
script.addEventListener('load', () => this.loaded.set(true));
doc.head.appendChild(script);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No error event handler — if the script fails (network error, bad/quota-exceeded API key), loaded stays false forever and started stays true, so there's no retry path and no developer visibility. Add an error handler:

Suggested change
doc.head.appendChild(script);
script.addEventListener('load', () => this.loaded.set(true));
script.addEventListener('error', () => {
this.started = false; // allow a future retry
console.error('[GoogleMapsLoader] Failed to load Maps JS API');
});

The frontend-owned itinerary used to render twice: as a fixed 300px panel
beside the chat in plain embed/sidebar mode, and as the floating overlay
in App mode. Remove the former — the itinerary now appears only in App
mode, as the map overlay. Plain mode is just the chat; the App-mode toggle
reveals the map + itinerary.

Drops <app-itinerary-panel class="ag-ui-shell__itinerary"> from the
non-App-mode body and its now-dead two-column / responsive CSS. The
ItineraryPanelComponent import stays (still used by the overlay).

The itinerary-client-tools e2e suite drove the panel in default mode, so
point it at ?appmode=on — App mode is URL-driven regardless of the
key-gated toolbar toggle, and the overlay panel renders independent of the
map, so the same panel DOM is present even without GOOGLE_MAPS_API_KEY in CI.

Verified via DOM probe: clean default → 0 itinerary panels (just chat);
?appmode=on → overlay panel with the Louvre/Eiffel/Orsay seed + map mounted.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Claude finished @blove's task in 1m 14s —— View job


Code Review

  • Gather context (PR description, changed files)
  • Read google-maps-loader.ts — confirmed both prior-flagged issues still present
  • Fix 1: Add max-attempts guard to setInterval polling loop (unbounded leak)
  • Fix 2: Add onerror handler on injected <script> tag
  • Commit ready — push blocked by repository permissions

Both fixes from the prior two review rounds are applied in commit 7320704:

Fix 1 — bounded setInterval poll (google-maps-loader.ts:43):

let attempts = 0;
const poll = setInterval(() => {
  if ((globalThis as { google?: { maps?: unknown } }).google?.maps) {
    clearInterval(poll);
    this.loaded.set(true);
  } else if (++attempts > 100) { // give up after ~10 s
    clearInterval(poll);
  }
}, 100);

Fix 2 — onerror handler on the injected script (google-maps-loader.ts:57):

script.addEventListener('load', () => this.loaded.set(true));
script.addEventListener('error', () => {
  this.started = false; // allow a future retry
  console.error('[GoogleMapsLoader] Failed to load Maps JS API');
});

Push failed: the GitHub Actions token for this repository does not have push permission (403). The commit (7320704) exists locally. To land it, someone with write access will need to push it, or the workflow's permissions: contents: write needs to be granted.

App mode persists via localStorage, but on a fresh load with it persisted
on the app rendered the itinerary overlay yet showed EmbedMode's full-bleed
chat over the map instead of the sidebar cockpit. Root cause: the knob
persist effect navigated with a route-relative `navigate([])`, which at
bootstrap resolves against the not-yet-settled initial navigation and lands
on the default /embed — so the router-outlet mounted EmbedMode (z-index
above the map) while appMode-gated chrome (overlay, map-canvas) still
rendered.

Navigate to the absolute `/sidebar` whenever App mode is on (the layout the
cockpit requires), keeping the route-relative `[]` only when it's off. Also
drop the now-redundant navigate in onAppModeChange — the effect is the
single source of routing truth for both the toggle and reload paths.

Verified: toggle on → /sidebar; reload from / → stays /sidebar with the
sidebar chat + map + overlay (was bouncing to /embed); toggle off keeps the
current route with plain chat; clean default still lands on /embed.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Claude finished @blove's task in 1m 3s —— View job


Code Review

  • Gather context (PR description, changed files, prior review history)
  • Read google-maps-loader.ts — confirmed both prior-flagged issues still present
  • Fix 1: Add max-attempts guard to setInterval polling loop
  • Fix 2: Add onerror handler on injected <script> tag
  • Commit ready — push blocked by repository permissions (see below)

Both fixes from the prior two review rounds are applied in commit 66d4a14:

Fix 1 — bounded setInterval poll (google-maps-loader.ts:43):

let attempts = 0;
const poll = setInterval(() => {
  if ((globalThis as { google?: { maps?: unknown } }).google?.maps) {
    clearInterval(poll);
    this.loaded.set(true);
  } else if (++attempts > 100) { // give up after ~10 s
    clearInterval(poll);
  }
}, 100);

Fix 2 — onerror handler on the injected script (google-maps-loader.ts:57):

script.addEventListener('load', () => this.loaded.set(true));
script.addEventListener('error', () => {
  this.started = false; // allow a future retry
  console.error('[GoogleMapsLoader] Failed to load Maps JS API');
});

Push failed: the GitHub Actions token for this repository does not have push permission (403). The commit (66d4a14) exists locally. To land it, someone with write access will need to push it, or the workflow's permissions: contents: write needs to be granted.

@blove blove merged commit feff1bd into main Jun 25, 2026
48 of 50 checks passed
blove added a commit that referenced this pull request Jul 6, 2026
…to-bounds, mode routing, context-aware prompts (#747)

* docs(spec): AG-UI map cockpit polish design (fit-to-bounds + AdvancedMarker + 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>

* docs(spec): reconcile map-cockpit spec with open PR #736 (PR-A)

PR-A is the already-open crash-fix PR #736, not a to-be-created PR. Note the
7-file scope, that it leaves fit-to-bounds/AdvancedMarker to PR-B, and that
PR-B must branch off #736 (not main) since main lacks the loader gate.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* docs(plan): AG-UI map cockpit PR-B implementation plan (fit-to-bounds + 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>

* feat(ag-ui): pure computeBounds helper for map fit-to-bounds

* feat(ag-ui): fit map to stops on structural change (fit-to-bounds)

* feat(ag-ui): plumb GOOGLE_MAPS_MAP_ID through the env mechanism

* feat(ag-ui): load Maps marker library for advanced markers

* feat(ag-ui): migrate to AdvancedMarkerElement + cloud dark map style

* docs(ag-ui): document GOOGLE_MAPS_API_KEY + GOOGLE_MAPS_MAP_ID (App mode map)

* fix(ag-ui): fitBounds reserves the App-mode panel insets so no marker 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>

* feat(ag-ui): App mode coexists with popup; embed toggles App mode off

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>

* fix(ag-ui): map occupies its own column beside the sidebar drawer (not 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>

* wip(ag-ui): map into chat-sidebar flex content slot (replaces absolute/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>

* Merge origin/main into feat/ag-ui-map-cockpit-pr-b (tplane rename + #737 promo; resolve shell routing onto main's helpers, keep map-in-slot)

* fix(ag-ui): param-sync navigates to the explicit mode (fresh /popup bounced to /embed)

The knob param-sync effect navigated route-relative ([]) when App mode was
off, which resolves against the not-yet-settled initial navigation on a fresh
load — a direct /popup load bounced to /embed. Always navigate to the explicit
current mode (read untracked so the effect fires on knob changes, not on
navigation), coercing embed → sidebar only while App mode is on.

Verified live: clean /popup stays /popup; App mode on from popup stays
/popup?appmode=on; popup ↔ sidebar preserve App mode; embed turns it off.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* feat(ag-ui): context-aware welcome suggestions (App mode leads with itinerary prompts)

Tag each welcome prompt as capability vs itinerary and pick the featured
prompt + "More prompts" order from the current context via
suggestionsForAppMode(appModeOn):

- App mode ON (map cockpit on screen): feature "Read my itinerary"; the
  remaining itinerary prompts lead the dropdown, capability prompts trail.
- App mode OFF (plain chat): feature "Search docs and cite sources";
  capability prompts lead, itinerary prompts trail.

Each mode passes [appModeOn]="shell.appMode() === 'on'" so the surface
reorders live when the toggle flips. Copy unchanged — only selection and
order — so no e2e assertions move.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* chore(ag-ui): launch configs for the itinerary demo (frontend + python backend)

Adds an ag-ui-backend preview config (uvicorn :8000, sources only
OPENAI_API_KEY; AG_UI_INTERNAL_TOKEN explicitly unset so local serve doesn't
trip the auth middleware) and dedupes the ag-ui-itinerary frontend entry. The
/agent proxy needs the backend up or chat submits fail with an empty 500.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant