Merge pull request 'v0.7.0 — map click → dossier target; fix "no owner"' (#19) from map-click-dossier into master

AGENTS.md

@@ -72,7 +72,7 @@ history/
     │   └── utils.ts        # cn()
     └── components/
         ├── PresetPicker.tsx / NationSelect.tsx   # home + nation pick screens
-        ├── WorldMap.tsx     # MapLibre basemap + choropleth + click popup + player highlight
+        ├── WorldMap.tsx     # MapLibre basemap + choropleth + click→dossier + hover tooltip + player highlight
         ├── NationPanel.tsx  # a nation's stats/territory/diplomacy dossier
         ├── ActionPanel.tsx  # tabs: Action / Diplomacy / Advisor + pending-queue list (per-item trash button + confirm dialog) + "Jump forward"
         ├── EventFeed.tsx    # scrolling event log (newest first)
@@ -140,6 +140,17 @@ history/
   (model down) surfaces as an HTTP 502 to the client (shown as a toast).
 - **Stable per-nation colors.** `shared.nationColor(id)` hashes a nation id → hue, shared by the
   server's `RegionMap` and any client coloring so the choropleth always agrees.
+- **Map click → dossier target (owner resolved from the RegionMap).** Clicking a map region does
+  NOT open a popup — it switches the dossier to that region's owning nation
+  (`setViewingId(ownerId)` in `GameScreen`); the manual "Inspect" selector and a map click both set
+  the same target and stay in sync. The owning nation is resolved from the authoritative `RegionMap`
+  by the feature's stable ISO_A3 code, which `applyRegions` stamps into `properties.regionId`
+  (properties survive into the rendered features) and the shared `regionOwner()` helper looks up.
+  It must **never** be read from the feature's top-level `id`: MapLibre repacks a GeoJSON source's
+  `id` into a numeric index when it stores the source, so at click/hover time `feature.id` is a
+  number, never the ISO_A3 (that repacking was the "no owner" bug, #6). Unowned regions (no
+  `RegionMap` entry) leave the current dossier target unchanged — never a broken dossier. The
+  hover path (a non-closeable tooltip with owner + key stats) uses the same resolution.
 - **Deterministic stub.** `server/stub.ts` answers the model contract deterministically per
   queued round item: each `action` item → one military/success event, each `diplomacy` item → one
   diplomatic/partial event, an empty round → one political "The world moves on" event. The player

CHANGELOG.md

@@ -2,6 +2,14 @@
 
 All notable changes to **History** are listed here, newest first.
 
+## v0.7.0 — 2026-10-11 19:17
+
+- Clicking a map region now switches the selected dossier to that region's owning nation instead of opening a popup (#7). The closeable click popup is gone; a hover tooltip (owner + key stats) is kept. The manual "Inspect" selector still works and stays in sync — both it and a map click drive the same dossier target.
+- Fixed: a clicked owned region no longer reports "no owner" (#6). Root cause: MapLibre repacks a GeoJSON source's top-level `id` (the ISO_A3 string) into a numeric index when it stores the source, so the feature a click/hover handler receives has a *number* for `id` — the old `regions[f.id]` lookup resolved to nothing, and the popup read "No owner" even for owned regions (the choropleth fill still worked because it is stamped from the original `id` before `setData`). Fix: `applyRegions` now stamps the stable ISO_A3 code into `properties.regionId` (and `ownerId`), which survives into the rendered features, and both the click and hover handlers resolve the owning nation from that code via the authoritative RegionMap — never from the repacked `feature.id`.
+- Unowned regions (no entry in the RegionMap) do not change the dossier when clicked — the current target is left in place (a deliberate, non-crashing choice rather than a special "unowned" dossier state).
+- New shared helper `regionOwner(regions, isoA3)` in `@shared/protocol` (the pure, single source of truth for "ISO_A3 → owner record").
+- **Unit tests** — new `regionOwner` tests (owned → owner record, unowned → undefined, empty/missing → undefined) plus a #6 regression guard that pins the exact MapLibre-repacked feature shape (numeric `id`, ISO_A3 in `properties.regionId`) and asserts resolution via `properties.regionId` works while the old `feature.id` path fails (44 tests, up from 39).
+
 ## v0.6.0 — 2026-10-11 18:27
 
 - Advisor fetches now show a "thinking" indicator (mirroring the turn's "Simulating…" hint) while the model responds, and the Ask-advisor button disables during the fetch; the top-bar status shows "Advisor thinking…" in the meantime. Fixes #15.

package.json

@@ -1,6 +1,6 @@
 {
   "name": "history",
-  "version": "0.6.0",
+  "version": "0.7.0",
   "private": true,
   "type": "module",
   "description": "AI-driven alternate-history simulator — the LLM is the world engine (Pax Historia style).",

server/map.test.ts

@@ -1,6 +1,6 @@
 import { describe, expect, it } from 'vitest';
 import { buildRegionMap } from './map';
-import { nationColor } from '../shared/protocol';
+import { nationColor, regionOwner, type RegionMap } from '../shared/protocol';
 import type { GameState } from '../shared/types';
 
 const state: GameState = {
@@ -33,6 +33,9 @@ const state: GameState = {
   version: 1,
 };
 
+// The RegionMap built from the fixture: the authoritative ISO_A3 → owner source.
+const regions: RegionMap = buildRegionMap(state);
+
 describe('buildRegionMap', () => {
   it('maps each ISO_A3 region to its owner + that nation\'s stable color', () => {
     const m = buildRegionMap(state);
@@ -44,6 +47,67 @@ describe('buildRegionMap', () => {
   });
 });
 
+describe('regionOwner', () => {
+  it('resolves an owned region\'s ISO_A3 to its owner record', () => {
+    expect(regionOwner(regions, 'GBR')).toEqual({
+      color: nationColor('uk'),
+      owner: 'UK',
+      ownerId: 'uk',
+    });
+    expect(regionOwner(regions, 'DEU')).toEqual({
+      color: nationColor('de'),
+      owner: 'DE',
+      ownerId: 'de',
+    });
+  });
+
+  it('returns undefined for an unowned region (no RegionMap entry)', () => {
+    expect(regionOwner(regions, 'FRA')).toBeUndefined();
+    expect(regionOwner(regions, 'USA')).toBeUndefined();
+  });
+
+  it('returns undefined for an empty or missing code', () => {
+    expect(regionOwner(regions, '')).toBeUndefined();
+    expect(regionOwner(regions, null)).toBeUndefined();
+    expect(regionOwner(regions, undefined)).toBeUndefined();
+  });
+});
+
+/**
+ * #6 regression guard. The click/hover handlers resolve the owning nation from a
+ * map feature. MapLibre repacks a GeoJSON source's top-level `id` (the ISO_A3
+ * string) into a *numeric index* when it stores the source, so a feature the map
+ * hands back at interaction time has a number for `id` and the ISO_A3 only lives
+ * in `properties.regionId`. This test pins the exact data shape and proves the
+ * fix: resolution via `properties.regionId` works, while the pre-fix path that
+ * read `feature.id` does not.
+ */
+describe('#6 — owner must be resolved from properties.regionId, not the repacked feature.id', () => {
+  // The shape MapLibre returns for a source feature after setData(): `id` is a
+  // number (here 0, as observed in the live map), and the ISO_A3 is preserved
+  // only in properties (the client stamps it as `regionId`).
+  const repackedFeature = {
+    id: 0,
+    properties: { name: 'DE', regionId: 'DEU' },
+  };
+
+  it('resolves the owner from properties.regionId (the fix)', () => {
+    const code = typeof repackedFeature.properties.regionId === 'string' ? repackedFeature.properties.regionId : '';
+    expect(regionOwner(regions, code)).toEqual({
+      color: nationColor('de'),
+      owner: 'DE',
+      ownerId: 'de',
+    });
+  });
+
+  it('the old path (feature.id as a string key) does NOT resolve — the bug', () => {
+    // String(0) === '0' is not a valid ISO_A3 key, so the pre-fix lookup failed
+    // and the click popup reported "No owner" even for owned regions.
+    expect(regions[String(repackedFeature.id)]).toBeUndefined();
+    expect(regionOwner(regions, String(repackedFeature.id))).toBeUndefined();
+  });
+});
+
 describe('nationColor', () => {
   it('is deterministic per id and distinct across different ids', () => {
     expect(nationColor('uk')).toBe(nationColor('uk'));

shared/protocol.ts

@@ -56,6 +56,23 @@ export interface RegionOwner {
 }
 export type RegionMap = Record<string, RegionOwner>;
 
+/**
+ * Look up the owner record for a map region from the authoritative RegionMap by
+ * its ISO_A3 code. Returns `undefined` for an empty/missing code or an unowned
+ * region. Pure + shared: the client resolves a clicked or hovered map feature by
+ * first reading the feature's stable ISO_A3 code (stored in `properties.regionId`
+ * by the client when it repacks the source — see WorldMap) and then calling this.
+ *
+ * The code must NOT be taken from the feature's top-level `id`: MapLibre repacks
+ * a GeoJSON source's `id` (an ISO_A3 string) into a numeric index when it stores
+ * the source, so at click/hover time `feature.id` is a number, never the ISO_A3
+ * — resolving from it is exactly the "no owner" bug (#6).
+ */
+export function regionOwner(regions: RegionMap, isoA3: string | null | undefined): RegionOwner | undefined {
+  if (!isoA3) return undefined;
+  return regions[isoA3];
+}
+
 /** Server -> client Server-Sent Events payload (see server/index.ts /sse). */
 export type SseMessage =
   | { type: 'state'; state: GameState }

src/components/WorldMap.tsx

@@ -8,7 +8,7 @@ import 'maplibre-gl/dist/maplibre-gl.css';
 // Point it at a Vite-emitted copy of the worker module so it loads in prod.
 import workerUrl from 'maplibre-gl/dist/maplibre-gl-worker.mjs?url';
 maplibregl.setWorkerUrl(workerUrl);
-import type { RegionMap, RegionOwner } from '@shared/protocol';
+import { regionOwner, type RegionMap, type RegionOwner } from '@shared/protocol';
 import type { Nation } from '@shared/types';
 import { cn } from '@/lib/utils';
 
@@ -101,21 +101,36 @@ function cameraForPreset(presetId: string): { center: [number, number]; zoom: nu
  * carries its owner's stable fill color, a subtle stroke, and a flag for the
  * player's own regions (rendered as a brighter, thicker border by a dedicated
  * layer). Unknown owners fall back to a faint neutral fill — never an error.
+ *
+ * Crucially, the base GeoJSON feature's top-level `id` (the ISO_A3 code) is
+ * stamped into `properties.regionId`, and the owner's id into `properties.ownerId`.
+ * MapLibre repacks a GeoJSON source's top-level `id` into a numeric index when it
+ * stores it, so a later click/hover event exposes that number on
+ * `e.features[i].id` — the top-level `id` is *not* a reliable lookup key at
+ * interaction time (the root cause of #6). `properties`, however, survive into the
+ * rendered features, so the interaction handlers resolve the owning nation from
+ * `properties.regionId` via the RegionMap (see `regionOwner`).
  */
 function applyRegions(geo: FeatureCollection, regions: RegionMap, playerNationId: string): FeatureCollection {
   const out: FeatureCollection = JSON.parse(JSON.stringify(geo));
   for (const f of out.features) {
     f.properties = f.properties ?? {};
-    const r = f.id ? regions[f.id as string] : undefined;
+    // The base GeoJSON `id` is the ISO_A3 code; keep it in a property that
+    // survives into the features MapLibre hands back on click/hover.
+    const code = f.id == null ? '' : String(f.id);
+    f.properties.regionId = code;
+    const r = code ? regions[code] : undefined;
     if (r) {
       f.properties.fill = r.color;
       f.properties.owner = r.owner;
+      f.properties.ownerId = r.ownerId;
       f.properties.stroke = 'rgba(255,255,255,0.10)';
       f.properties.isPlayer = r.ownerId === playerNationId ? 1 : 0;
     } else {
       f.properties.fill = 'rgba(148,163,184,0.05)';
       f.properties.stroke = 'rgba(255,255,255,0.06)';
       f.properties.owner = '';
+      f.properties.ownerId = '';
       f.properties.isPlayer = 0;
     }
   }
@@ -150,35 +165,19 @@ function hoverHtml(countryName: string, region?: RegionOwner, nation?: Nation):
   </div>`;
 }
 
-/** Click popup: the full owning-nation profile with all four stats. */
-function clickHtml(countryName: string, region?: RegionOwner, nation?: Nation): string {
-  if (!region || !nation) {
-    return `<div class="hm-popup"><div class="hm-popup-title">${esc(countryName || 'Unclaimed')}</div><div class="hm-popup-sub">No owner</div></div>`;
-  }
-  const stats = [
-    statRow('Mil', nation.stats.military),
-    statRow('Eco', nation.stats.economy),
-    statRow('Stab', nation.stats.stability),
-    statRow('Pop', nation.stats.population),
-  ].join('');
-  const regions = nation.territory.length;
-  return `<div class="hm-popup">
-    <div class="hm-popup-title">${esc(countryName || region.owner)}</div>
-    <div class="hm-popup-owner hm-popup-owner-big"><span class="hm-chip" style="background:${region.color}"></span>${esc(region.owner)}</div>
-    <div class="hm-popup-sub">${regions} region${regions === 1 ? '' : 's'} · click focuses the dossier</div>
-    <div class="hm-stats">${stats}</div>
-  </div>`;
-}
-
 interface Props {
   regions: RegionMap;
   playerNationId: string;
-  /** All nations, for hover/click stats and color chips. */
+  /** All nations, for the hover tooltip's owner stats and color chips. */
   nations: Record<string, Nation>;
   /** The active scenario id, for a sensible initial camera. */
   presetId: string;
   className?: string;
-  /** Called when a region is clicked, so the dossier can focus its owner. */
+  /**
+   * Called with a region's owning-nation id when that region is clicked, so the
+   * dossier switches to that nation. Not called for unowned regions (the dossier
+   * keeps its current target). See the component doc for the full behavior.
+   */
   onFocusNation?: (id: string) => void;
 }
 
@@ -187,10 +186,18 @@ interface Props {
  * nation. Rendered as a fixed, full-viewport background layer (z-0) behind the
  * floating HUD windows: a dark-navy basemap with subtle terrain, per-nation
  * stable fills (the choropleth is the star), thin borders, a brighter/thicker
- * border for the player, hover + click popups with
- * owner + stats, a transient flash on territory changes, and a
- * per-scenario initial camera. Zoom and pan are driven directly by the user
- * (MapLibre wheel/trackpad + drag) — no floating control and no external API.
+ * border for the player, a hover tooltip (owner + key stats), a **click that
+ * switches the dossier to the region's owning nation** (no popup — see #7), a
+ * transient flash on territory changes, and a per-scenario initial camera.
+ *
+ * Ownership for a hovered/clicked region is resolved from the authoritative
+ * RegionMap by the feature's stable ISO_A3 code in `properties.regionId`
+ * (see `regionOwner` in @shared/protocol) — never from the feature's top-level
+ * `id`, which MapLibre repacks into a numeric index at interaction time (#6).
+ * Unowned regions (no RegionMap entry) simply do not change the dossier.
+ *
+ * Zoom and pan are driven directly by the user (MapLibre wheel/trackpad + drag)
+ * — no floating control and no external API.
  */
 export default function WorldMap({
   regions,
@@ -243,7 +250,6 @@ export default function WorldMap({
     (containerRef.current as unknown as { __map?: maplibregl.Map }).__map = map;
 
     const hoverPopup = new maplibregl.Popup({ closeButton: false, closeOnClick: false, offset: 10 });
-    const clickPopup = new maplibregl.Popup({ closeButton: true, closeOnClick: true, offset: 12 });
     let lastHoverId: string | null = null;
 
     const loadGeo = (): Promise<FeatureCollection> =>
@@ -300,24 +306,33 @@ export default function WorldMap({
     }
     flashApiRef.current = animateFlash;
 
+    // The region a hovered/clicked feature belongs to is identified by its stable
+    // ISO_A3 code, read from `properties.regionId` (stamped by `applyRegions`).
+    // We must NOT use the feature's top-level `id`: MapLibre repacks a GeoJSON
+    // source's `id` into a numeric index when it stores the source, so the value
+    // exposed on `e.features[i].id` at interaction time is a number, never the
+    // ISO_A3 — resolving the owner from it is the "no owner" bug (#6).
     const onHover = (e: maplibregl.MapLayerMouseEvent) => {
       const f = e.features?.[0];
       if (!f) return;
-      const id = f.id == null ? null : String(f.id);
-      if (id && id !== lastHoverId) {
-        lastHoverId = id;
+      const p = f.properties as { name?: string; regionId?: string };
+      const code = typeof p?.regionId === 'string' ? p.regionId : '';
+      if (code && code !== lastHoverId) {
+        lastHoverId = code;
         const geo = baseGeoRef.current;
-        const feat = geo?.features.find((x) => x.id != null && String(x.id) === id);
+        // The base GeoJSON feature's top-level `id` IS the ISO_A3, so match on
+        // that to recover the feature for the hover-highlight outline (the
+        // feature MapLibre hands us has a repacked numeric `id` we can't use).
+        const feat = geo?.features.find((x) => x.id != null && String(x.id) === code);
         (map.getSource(HOVER_SOURCE) as GeoSource | undefined)?.setData(
           feat ? { type: 'FeatureCollection', features: [JSON.parse(JSON.stringify(feat))] } : emptyFC(),
         );
       }
-      const p = f.properties as { name?: string };
-      const region = id ? regionsRef.current[id] : undefined;
+      const region = regionOwner(regionsRef.current, code);
       const nation = region ? nationsRef.current[region.ownerId] : undefined;
       hoverPopup
         .setLngLat(e.lngLat)
-        .setHTML(hoverHtml(p?.name ?? id ?? '', region, nation))
+        .setHTML(hoverHtml(p?.name ?? code, region, nation))
         .addTo(map);
       map.getCanvas().style.cursor = 'pointer';
     };
@@ -329,14 +344,17 @@ export default function WorldMap({
       map.getCanvas().style.cursor = '';
     };
 
+    // #7: a click does NOT open a popup — it switches the dossier to the region's
+    // owning nation. The owner is resolved from the stable ISO_A3 in
+    // `properties.regionId` via the RegionMap (never the repacked top-level
+    // `f.id`, #6). A region with no owner in the RegionMap (unowned territory)
+    // leaves the current dossier target unchanged — never a broken dossier.
     const onClick = (e: maplibregl.MapLayerMouseEvent) => {
       const f = e.features?.[0];
       if (!f) return;
-      const id = f.id == null ? null : String(f.id);
-      const p = f.properties as { name?: string };
-      const region = id ? regionsRef.current[id] : undefined;
-      const nation = region ? nationsRef.current[region.ownerId] : undefined;
-      clickPopup.setLngLat(e.lngLat).setHTML(clickHtml(p?.name ?? id ?? '', region, nation)).addTo(map);
+      const p = f.properties as { regionId?: string };
+      const code = typeof p?.regionId === 'string' ? p.regionId : '';
+      const region = regionOwner(regionsRef.current, code);
       if (region?.ownerId) onFocusNationRef.current?.(region.ownerId);
     };