Skip to content

feat(ag-ui): App-mode cockpit polish — map in sidebar flex slot, fit-to-bounds, mode routing, context-aware prompts#747

Merged
blove merged 21 commits into
mainfrom
feat/ag-ui-map-cockpit-pr-b
Jul 6, 2026
Merged

feat(ag-ui): App-mode cockpit polish — map in sidebar flex slot, fit-to-bounds, mode routing, context-aware prompts#747
blove merged 21 commits into
mainfrom
feat/ag-ui-map-cockpit-pr-b

Conversation

@blove

@blove blove commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

What

The remaining App-mode cockpit work on top of #745 (AdvancedMarker + Map ID, merged) and #740 (chat-sidebar flexbox lib refactor, merged). Supersedes closed #738 with the delta that isn't on main:

  • Map lives in the chat-sidebar flex content slot (SidebarMode) instead of a shell-level absolute overlay — it fills the column beside the drawer with the height threaded, no occupy-var insets. Popup keeps a full-bleed shell backdrop behind the floating bubble; plain sidebar shows the feat(ag-ui): App-mode promo hero in sidebar mode #737 promo.
  • Fit-to-bounds: pure computeBounds helper (unit-tested) + an effect that frames the map to all stops on structural change (0 stops → Paris, 1 → center/zoom-13, ≥2 → fitBounds with panel-aware padding), keyed off stopsWithCoords() so it never fights focus-panning.
  • Mode ↔ App-mode routing: popup/sidebar preserve App mode; embed turns it off (and enabling App mode from embed coerces to sidebar). Param-sync navigates to the explicit mode — fixes a fresh /popup load bouncing to /embed.
  • Context-aware welcome prompts: prompts tagged capability/itinerary; App mode on features "Read my itinerary" with itinerary prompts leading the More-prompts dropdown, off leads with "Search docs…". Copy unchanged, so no e2e assertions move.
  • Launch configs for the demo (frontend + uvicorn backend with the AG_UI_INTERNAL_TOKEN unset guard) + the PR-B spec/plan docs.

Verification

  • nx lint (0 errors) + nx test (50 passing, incl. map-bounds 5/5) + nx build for examples-ag-ui-angular, on a branch current with main.
  • Live in Chrome (real browser, live LLM): featured "Read my itinerary" click → user → assistant → get_itinerary client tool → assistant itinerary reply; dark map + pins render; markers frame centered in the visible column; popup = full-bleed map behind bubble; embed exits App mode; clean /popup reload stays on popup; suggestion ordering flips live with the toggle.

🤖 Generated with Claude Code

blove and others added 21 commits June 24, 2026 21:41
…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>
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>
… + 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>
… 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>
…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>
…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>
 promo; resolve shell routing onto main's helpers, keep map-in-slot)
…ounced 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>
…tinerary 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>
…p map-in-slot + verified routing; sidebar off-state shows the #737 promo
…n 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>
@vercel

vercel Bot commented Jul 6, 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 Jul 6, 2026 2:37pm

Request Review

@blove blove enabled auto-merge (squash) July 6, 2026 14:36

@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 Jul 6, 2026

Copy link
Copy Markdown
Contributor

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


PR Review

  • Gather context (CLAUDE.md, PR diff, changed files)
  • Review for correctness bugs and broken behaviour
  • Review for security issues
  • Review for TypeScript type-safety and Angular/RxJS pitfalls
  • Check test coverage
  • Post findings

Overall: looks good — clean architecture, correct signal wiring, solid test coverage for computeBounds. A few things worth fixing:

Issues found (inline comments posted):

  1. Bare document global in fitPadding() (map-canvas.component.ts:201,206) — MapCanvasComponent uses inject(ElementRef)/inject(DestroyRef) but calls document.querySelector directly. AgUiShell already shows the right pattern with inject(DOCUMENT). SSR-unsafe and inconsistent. Easy fix: add private readonly document = inject(DOCUMENT) and switch both calls.

  2. suggestionsForAppMode has no unit tests (welcome-suggestions.ts:86) — it's a pure function with two branching paths; computeBounds next door has 5 specs. Worth adding a small spec to guard against a future slice/destructuring regression.

  3. Double navigation when switching to embed with App mode on (ag-ui-shell.component.ts:212–216) — appMode.set('off') schedules the param-sync effect to navigate, then the explicit router.navigate below fires too. Both target /embed so behaviour is correct, but it's a redundant router cycle. The original .then() idiom existed to prevent this; the new ordering loses that guard. Not blocking.

  4. Redundant stopsWithCoords() read in the framing effect (map-canvas.component.ts:144) — the signal is read explicitly for tracking and then again inside frameToBounds (which runs synchronously in the same effect). Both reads are tracked correctly; the explicit call is misleading. Nit.

Not issues:

  • Reading stopsWithCoords() in the ResizeObserver callback is fine — signal reads outside reactive contexts just return the current value.
  • afterNextRender + destroyRef.onDestroy cleanup is the correct Angular 17+ idiom here.
  • The FEATURED_SUGGESTIONS/MORE_SUGGESTIONS back-compat exports are in the examples/ag-ui file; examples/chat has its own copy and is unaffected.
  • locationMode() in the shell is a near-duplicate of parseMode() (could consolidate) but is pre-existing and not introduced by this PR.

@blove blove merged commit 2869683 into main Jul 6, 2026
8 checks passed
const mapRect = this.hostRef.nativeElement.getBoundingClientRect();
if (mapRect.width === 0) return pad; // not laid out (e.g. jsdom) — uniform

const overlay = document.querySelector('.ag-ui-shell__itinerary-overlay');

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.

Bare document global instead of injected DOCUMENT token.

MapCanvasComponent injects ElementRef and DestroyRef but then calls document.querySelector directly at lines 201 and 206. AgUiShell already shows the correct pattern (inject(DOCUMENT)). This won't break in a browser-only app, but it's inconsistent and will throw in SSR/Universal.

Suggested change
const overlay = document.querySelector('.ag-ui-shell__itinerary-overlay');
const overlay = (this.document ?? document).querySelector('.ag-ui-shell__itinerary-overlay');

Or inject DOCUMENT at the top of the class:

private readonly document = inject(DOCUMENT);

and replace both bare document.querySelector calls with this.document.querySelector. Fix this →

Comment on lines +212 to +216
if (next === 'embed' && this.appMode() === 'on') {
this.appMode.set('off');
this.persistence.write('appMode', 'off');
}
void this.router.navigate(['/', next], { queryParamsHandling: 'preserve' });

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.

Double navigation when switching to embed with App mode on.

appMode.set('off') marks the param-sync effect dirty. Even though Angular effects are asynchronous, both the effect's navigate (replaceUrl: true) and the explicit navigate below fire for the same destination. The result is correct (both go to /embed) but wastes a router cycle.

The original .then() dance was specifically to avoid this. One clean alternative: skip the explicit navigate and let the param-sync effect own it, by also writing this.mode() or by setting a _pendingMode signal the effect reads.

This is a latent inefficiency, not a bug — Angular Router coalesces back-to-back same-route navigations. Fine to leave as-is with a note.

// and reframing to all stops never fight (they fire on different signals).
effect(() => {
const map = this.googleMap();
this.stopsWithCoords(); // re-frame on structural change (add/remove/geocode)

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 explicit this.stopsWithCoords() read on this line is redundant: frameToBounds (called on the very next line) also reads this.stopsWithCoords() synchronously within the effect's tracking scope, so the signal is tracked either way. The comment is accurate and the behavior is correct, but the explicit call is misleading — it implies frameToBounds doesn't track it. Could drop the standalone call and rely on frameToBounds's read, or add a note clarifying both reads are tracked.

* capability prompts; itinerary prompts trail (they still work, just less
* prominent without the map).
*/
export function suggestionsForAppMode(appModeOn: boolean): {

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.

suggestionsForAppMode is untested. It's a pure function with two clearly distinct code paths (app mode on vs. off) and the "featured" item being the first element of each array — exactly the kind of thing that's cheap to unit-test and has bitten people before (wrong array destructuring, off-by-one slice, etc.). The computeBounds helper next door has 5 specs; this would benefit from the same treatment. Fix this →

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