Skip to content

feat(ag-ui): AdvancedMarkerElement + Map ID for the App-mode map#745

Merged
blove merged 4 commits into
mainfrom
blove/ag-ui-map-advanced-markers
Jul 6, 2026
Merged

feat(ag-ui): AdvancedMarkerElement + Map ID for the App-mode map#745
blove merged 4 commits into
mainfrom
blove/ag-ui-map-advanced-markers

Conversation

@blove

@blove blove commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Salvages the genuinely-unique map-rendering improvements from #738 onto main's current (shipped) App-mode layout — without #738's competing layout rewrite (which reverts #737's shipped absolute-background approach + reintroduces the removed launcher hint). #738's layout commits are intentionally dropped; this keeps the verified layout and lands only the rendering upgrades.

Cherry-picked from #738 (4 commits):

  • Plumb GOOGLE_MAPS_MAP_ID through the env mechanism (env → inject-env → generated-keys)
  • Load the Maps marker library
  • Migrate <map-marker><map-advanced-marker> (MapAdvancedMarker, custom pin content), drop the inline DARK_STYLE array in favor of a cloud-based style tied to the Map ID (falls back to Google's DEMO_MAP_ID so a fresh clone still runs — light map)
  • README docs for GOOGLE_MAPS_API_KEY + GOOGLE_MAPS_MAP_ID

Deliberately NOT included: #738's fitBounds-with-panel-insets (coupled to its layout + a map-bounds.ts util; it's the already-deferred POL.1) and all of #738's App-mode layout commits (superseded by #737).

Verified: lint 0 errors, unit ✓, build ✓, ag-ui e2e 28/28, and live in Chrome — 3 advanced markers render on the shipped layout, marker library loaded, no console errors (only a benign gmp-click hint from the Angular wrapper). Dark cockpit style activates when a dark-styled Map ID is set in .env.

Supersedes #738 (which I'll close).

🤖 Generated with Claude Code

@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 4:54am

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

Copy link
Copy Markdown
Contributor

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


Code Review

  • Load tools
  • Read CLAUDE.md for repo guidelines
  • Read PR diff and changed files
  • Analyze for correctness, security, type-safety, Angular/RxJS issues
  • Post inline comments for specific issues
  • Post final summary

Overall: looks good. The migration from MapMarkerMapAdvancedMarker is done correctly end-to-end, and the GOOGLE_MAPS_MAP_ID plumbing through env → inject-env → generated-keys → environment → component is clean and consistent.

Checked — no issues:

  • Type safety: MapAdvancedMarker imported and used consistently; [content] receives HTMLElement which satisfies Element | null; win.open(marker) passes a MapAdvancedMarker which correctly implements MapAnchorPoint
  • Security: JSON.stringify wraps the key and map ID in inject-env.mjs; encodeURIComponent(key) still applied in the script URL; makePin only interpolates values from the hardcoded DAY_COLORS array (no injection risk)
  • Environment fallback: environment.googleMapsMapId || 'DEMO_MAP_ID' correctly handles the empty-string stub in generated-keys.ts — a fresh clone renders the map (light theme) without any console setup
  • marker library addition: correct library name required by the Advanced Markers API
  • @for track m.id + viewChildren index matching: order is stable since both derive from the same stopsWithCoords() array in the same sequence
  • SSR safety: markerViews() (and thus document.createElement) is only evaluated when the template reads it, which is inside @if (loader.loaded()) — never reached in SSR

One observation (inline comment posted on line 109): markerViews builds new HTMLElement pins for all stops whenever any stop mutates, including text-only changes (agent renames a place, adds a note). The prior DOM nodes are replaced in the Maps API and GC'd — not a correctness bug, but more DOM churn than the comment implies. See the inline for a cache sketch if you want to guard it.

`;
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'})`);
@blove blove merged commit 710f99a into main Jul 6, 2026
27 of 28 checks passed
stop: s,
position: { lat: s.lat!, lng: s.lng! },
content: this.makePin(s.day),
})),

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.

makePin allocates a new HTMLElement for every stop on every execution of this computed. stopsWithCoords() recomputes whenever any stop mutates — including text-only changes (agent renames a place, adds a note) — so all pin nodes are silently replaced even when no day or position changed. The old elements handed to the Maps API are detached and GC'd, so it's not a correctness bug, but it's more aggressive DOM churn than the @for track m.id rhythm implies.

An easy guard: key the element by (s.id, s.day) with a Map cache so pins are only rebuilt when the day (i.e., colour) actually changes:

Suggested change
})),
protected readonly markerViews = computed(() => {
const prev = this._pinCache;
const next = new Map<string, HTMLElement>();
return this.stopsWithCoords().map((s) => {
const cacheKey = `${s.id}:${s.day}`;
const content = prev.get(cacheKey) ?? this.makePin(s.day);
next.set(cacheKey, content);
this._pinCache = next;
return { id: s.id, stop: s, position: { lat: s.lat!, lng: s.lng! }, content };
});
});
private _pinCache = new Map<string, HTMLElement>();

Or, if you prefer a lighter touch, just leave a comment clarifying that "stops change" includes text edits, so readers aren't surprised.

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.

2 participants