From 9b93deaaaac5aecef871b5d1c8e8a1f809b23253 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 24 Aug 2026 10:05:43 +0000 Subject: [PATCH] fix(plugin-map): resolve marker titles through getRecordDisplayName ObjectMap was the only view renderer outside ADR-0079's unified record display-name resolver. getMapConfig filled an absent title binding with the string literal 'name' and the marker transform did a bare record[titleField] read, so every object whose display field is not literally 'name' titled every marker popup undefined. Both literals are removed and the read site calls getRecordDisplayName, passing the declared binding as its explicit titleField option so an authored map.titleField still wins. objectSchema joins the marker memo's deps, since it arrives from an async fetch after first paint. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CSoz9uGhaaSgiq3hshtN7L --- .../objectmap-marker-title-resolver-5953.md | 27 ++ .../src/ObjectMap.markerTitle.test.tsx | 238 ++++++++++++++++++ packages/plugin-map/src/ObjectMap.tsx | 58 ++++- 3 files changed, 318 insertions(+), 5 deletions(-) create mode 100644 .changeset/objectmap-marker-title-resolver-5953.md create mode 100644 packages/plugin-map/src/ObjectMap.markerTitle.test.tsx diff --git a/.changeset/objectmap-marker-title-resolver-5953.md b/.changeset/objectmap-marker-title-resolver-5953.md new file mode 100644 index 0000000000..fa48d3279b --- /dev/null +++ b/.changeset/objectmap-marker-title-resolver-5953.md @@ -0,0 +1,27 @@ +--- +'@object-ui/plugin-map': patch +--- + +`ObjectMap` now resolves marker titles through `@object-ui/core`'s +`getRecordDisplayName` (ADR-0079), instead of reading a hard-coded `'name'` key. + +`getMapConfig` used to fill an absent title binding with the string literal +`'name'`, and the marker transform then did a bare `record[titleField]` read. For +every object whose display field is not literally `name`, that read was +`undefined`, so each marker popup titled itself `undefined`. The literal is gone +from both branches that carried it, and the read site hands the decision to the +same resolver `ObjectKanban`, `ObjectCalendar` and `ObjectGantt` already title +their items through — `ObjectMap` was the fourth renderer and the only one still +outside it. + +An authored `map.titleField` keeps winning outright: it is passed through as the +resolver's explicit `titleField` option, which it checks first. What is new +underneath is everything a static field-name binding cannot express — the +object's declared `nameField`, its deprecated `displayNameField` alias, a legacy +`titleFormat` template, type-aware field derivation, and a name-ish probe over +the record's own keys for the inline-data case where no object definition is +fetched at all. + +Records with no resolvable name now read `Record #` rather than `undefined` +or a uniform `Marker`; the `Marker` placeholder is kept only for a record that +carries no id either. No authoring surface changes and no new map config keys. diff --git a/packages/plugin-map/src/ObjectMap.markerTitle.test.tsx b/packages/plugin-map/src/ObjectMap.markerTitle.test.tsx new file mode 100644 index 0000000000..6a0682bf3a --- /dev/null +++ b/packages/plugin-map/src/ObjectMap.markerTitle.test.tsx @@ -0,0 +1,238 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#5953 — marker titles resolve through ADR-0079's unified + * `getRecordDisplayName`, not a hard-coded `'name'` key. + * + * ## What this file pins that `ObjectMap.listViewMapConfigReach.test.tsx` does not + * + * That file pins REACH: a declared `map` block arrives at `getMapConfig` and + * drives the read. Every one of its arms declares `titleField: 'title'` and + * asserts a positive title, so it exercises exactly the one path that was never + * broken — an AUTHORED binding. It never asserts the defect (its `'Marker'` / + * `undefined` mention is prose in a comment, not an assertion), and nothing in + * it changes here. + * + * The uncovered half, and this file's subject, is the path with NO authored + * binding, where `getMapConfig` used to forge one: + * + * - the FLAT form (`ObjectView` / `ListView`'s flatten product) without a + * `titleField`, which forged `'name'` and rendered `undefined` for every + * object whose display field is not literally `name` — the card's symptom; + * - a DECLARED `map` block without a `titleField`, which rendered the + * `'Marker'` placeholder for every record alike; + * - `titleFormat`, and the record-key probe — precedence steps a static + * field-name binding structurally cannot carry, so no upstream deriver + * (`defaultMapFromObject`, objectui#5909/PR#5955) can reach them either. + * + * The two placeholder arms at the bottom pin the resolution of the competing + * placeholders: `Record #` where the record has an id, this component's own + * `'Marker'` only where it has none. + */ + +import React from 'react'; +import { render, screen, waitFor, fireEvent } from '@testing-library/react'; +import { describe, it, expect, vi } from 'vitest'; +import { SchemaRendererProvider } from '@object-ui/react'; +import { ObjectMap } from './ObjectMap'; + +vi.mock('react-map-gl/maplibre', () => ({ + default: ({ children }: any) =>
{children}
, + Map: ({ children }: any) =>
{children}
, + NavigationControl: () =>
, + Marker: ({ children, longitude, latitude, onClick }: any) => ( +
onClick?.({ originalEvent: { stopPropagation() {} } })} + > + {children} +
+ ), + Popup: ({ children }: any) =>
{children}
, +})); + +/** + * The card's object shape: the display field is `site_name`, NOT `name`, and no + * record carries a `name` key at all — so the deleted `'name'` literal read + * `undefined` on every one of them. + */ +const SITES = [ + { id: '1', site_name: 'Harbour Depot', code_label: 'HD-01', latitude: 47.6062, longitude: -122.3321 }, + { id: '2', site_name: 'Ridge Yard', code_label: 'RY-02', latitude: 37.7749, longitude: -122.4194 }, +]; + +const makeDataSource = (objectDef: any, records: any[] = SITES) => ({ + find: vi.fn().mockResolvedValue(records), + findOne: vi.fn(), + create: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + getObjectSchema: vi.fn().mockResolvedValue(objectDef), +}); + +/** Mount the real `ObjectMap`, settle its two fetches, and open marker 0's popup. */ +async function openFirstPopup(schema: Record, dataSource?: any) { + render( + + + , + ); + await waitFor(() => expect(screen.queryByLabelText('Map')).not.toBeNull()); + await waitFor(() => expect(screen.getAllByTestId('map-marker').length).toBeGreaterThan(0)); + fireEvent.click(screen.getAllByTestId('map-marker')[0]); + await waitFor(() => expect(screen.queryByTestId('map-popup')).not.toBeNull()); + return screen.getByTestId('map-popup'); +} + +/** The object-provider data config every arm below fetches through. */ +const OBJECT_DATA = { provider: 'object', object: 'site' }; + +describe('marker titles resolve through getRecordDisplayName (objectui#5953)', () => { + // ── THE DISCRIMINATING ARM ──────────────────────────────────────────────── + // The FLAT form the ObjectView / ListView flatten product emits, with no + // `titleField` in it. `getMapConfig` used to fill that absence with `'name'` + // and the read site did `record['name']` — `undefined`, on every marker. + it('flat config with no `titleField`: resolves the declared `nameField`', async () => { + const ds = makeDataSource({ + name: 'site', + nameField: 'site_name', + fields: { site_name: { type: 'text' }, code_label: { type: 'text' } }, + }); + + const popup = await openFirstPopup( + { type: 'object-map', latitudeField: 'latitude', longitudeField: 'longitude', data: OBJECT_DATA }, + ds, + ); + + expect(popup.textContent).toContain('Harbour Depot'); + // The precise regression: not merely "something rendered", but that the + // hard-coded key is gone. `undefined` reaches the DOM as the string. + expect(popup.textContent).not.toContain('undefined'); + }); + + // The same absence through the DECLARED block, whose read-site symptom was + // the other, differently-shaped placeholder: `'Marker'` on every record. + it('declared `map` block with no `titleField`: resolves the declared `nameField`', async () => { + const ds = makeDataSource({ + name: 'site', + nameField: 'site_name', + fields: { site_name: { type: 'text' }, code_label: { type: 'text' } }, + }); + + const popup = await openFirstPopup( + { type: 'object-map', map: { latitudeField: 'latitude', longitudeField: 'longitude' }, data: OBJECT_DATA }, + ds, + ); + + expect(popup.textContent).toContain('Harbour Depot'); + expect(popup.textContent).not.toContain('Marker'); + }); + + // ── THE RULING THIS FIX MUST NOT BREAK ──────────────────────────────────── + // An authored `titleField` keeps winning outright. This is a bug fix, not a + // widening: the accepted authoring set is unchanged and the author's choice + // still outranks the object's own declaration. + it("an authored `titleField` still wins over the object's `nameField`", async () => { + const ds = makeDataSource({ + name: 'site', + nameField: 'site_name', + fields: { site_name: { type: 'text' }, code_label: { type: 'text' } }, + }); + + const popup = await openFirstPopup( + { + type: 'object-map', + map: { latitudeField: 'latitude', longitudeField: 'longitude', titleField: 'code_label' }, + data: OBJECT_DATA, + }, + ds, + ); + + expect(popup.textContent).toContain('HD-01'); + expect(popup.textContent).not.toContain('Harbour Depot'); + }); + + // ── STEPS A STATIC FIELD-NAME BINDING CANNOT CARRY ──────────────────────── + // A composite `titleFormat` template. No upstream deriver can bind this to a + // `titleField`, because there is no single field to name — which is why the + // fix has to live at the read site. + it('resolves a `titleFormat` template, which no field-name binding can express', async () => { + const ds = makeDataSource( + { + name: 'site', + titleFormat: '{code_label} — {site_name}', + fields: { site_name: { type: 'text' }, code_label: { type: 'text' } }, + }, + ); + + const popup = await openFirstPopup( + { type: 'object-map', latitudeField: 'latitude', longitudeField: 'longitude', data: OBJECT_DATA }, + ds, + ); + + expect(popup.textContent).toContain('HD-01 — Harbour Depot'); + }); + + // Inline `value` data never fetches an object definition at all (the schema + // effect is gated on `!hasInlineData`), so `objectSchema` stays `null` here. + // The resolver's record-key probe still finds the name off the record's own + // `*_name` key — a path the old bare `record['name']` read had no way to reach. + it('inline `value` data with no object definition: the record-key probe resolves it', async () => { + const popup = await openFirstPopup({ + type: 'object-map', + latitudeField: 'latitude', + longitudeField: 'longitude', + data: { provider: 'value', items: SITES }, + }); + + expect(popup.textContent).toContain('Harbour Depot'); + expect(popup.textContent).not.toContain('undefined'); + }); + + // ── THE PLACEHOLDER RESOLUTION ──────────────────────────────────────────── + // Two placeholders were in play once the resolver arrived: this component's + // `'Marker'` and the resolver's `Record #` floor. They are split by + // whether the record has an id, because that is exactly the split in how much + // either can say. `Record #` names ONE record; `'Marker'` describes every + // pin on the map equally and so distinguishes none of them. + it('no resolvable name, but an id: titles itself `Record #`, not the old placeholder', async () => { + const ds = makeDataSource( + { name: 'site', fields: { latitude: { type: 'number' }, longitude: { type: 'number' } } }, + [{ id: '77', latitude: 10, longitude: 20 }], + ); + + const popup = await openFirstPopup( + { type: 'object-map', latitudeField: 'latitude', longitudeField: 'longitude', data: OBJECT_DATA }, + ds, + ); + + expect(popup.textContent).toContain('Record #77'); + expect(popup.textContent).not.toContain('Marker'); + }); + + it("no resolvable name and no id: keeps this component's `Marker` placeholder", async () => { + const ds = makeDataSource( + { name: 'site', fields: { latitude: { type: 'number' }, longitude: { type: 'number' } } }, + [{ latitude: 10, longitude: 20 }], + ); + + const popup = await openFirstPopup( + { type: 'object-map', latitudeField: 'latitude', longitudeField: 'longitude', data: OBJECT_DATA }, + ds, + ); + + // `'Marker'` is passed as the resolver's `fallback`, displacing its generic + // `'Untitled'`. Asserting the NEGATIVE too is what makes this a pin on the + // choice rather than on "some string appeared". + expect(popup.textContent).toContain('Marker'); + expect(popup.textContent).not.toContain('Untitled'); + }); +}); diff --git a/packages/plugin-map/src/ObjectMap.tsx b/packages/plugin-map/src/ObjectMap.tsx index 5bec397964..d764233dcc 100644 --- a/packages/plugin-map/src/ObjectMap.tsx +++ b/packages/plugin-map/src/ObjectMap.tsx @@ -25,7 +25,12 @@ import type { ObjectMapSchema, ObjectMapConfig, DataSource, ViewData } from '@ob import { ObjectMapConfigSchema } from '@object-ui/types/zod'; import { useNavigationOverlay } from '@object-ui/react'; import { NavigationOverlay, cn, useIsMobile } from '@object-ui/components'; -import { extractRecords, buildExpandFields, convertSortToQueryParams } from '@object-ui/core'; +import { + extractRecords, + buildExpandFields, + convertSortToQueryParams, + getRecordDisplayName, +} from '@object-ui/core'; import MapGL, { NavigationControl, Marker, Popup } from 'react-map-gl/maplibre'; import type { MapRef } from 'react-map-gl/maplibre'; import 'maplibre-gl/dist/maplibre-gl.css'; @@ -372,7 +377,13 @@ function getMapConfig(schema: MapConfigSource): ObjectMapConfig { locationField: schema.locationField, latitudeField: schema.latitudeField, longitudeField: schema.longitudeField, - titleField: schema.titleField || 'name', + // No `|| 'name'` (objectui#5953). An ABSENT binding must stay absent so + // the read site can hand the decision to `getRecordDisplayName`; the + // literal used to forge a binding the author never wrote, and a forged + // `'name'` outranks the object's own declared `nameField` at step 0 of + // that resolver. A binding the flatten product really carries survives + // here and still wins. + titleField: schema.titleField, descriptionField: schema.descriptionField, zoom: schema.zoom, center: schema.center, @@ -392,7 +403,13 @@ function getMapConfig(schema: MapConfigSource): ObjectMapConfig { latitudeField: 'latitude', longitudeField: 'longitude', locationField: 'location', - titleField: 'name', + // Deliberately NO `titleField` (objectui#5953). The coordinate keys above + // are conventional guesses this component must make — nothing else can + // read a location out of an unconfigured record. A marker TITLE is not in + // that position: `getRecordDisplayName` resolves it from the object + // definition, and it does so better than any literal here could (declared + // `nameField`, `titleFormat`, type-aware derivation, then a name-ish probe + // over the record's own keys, of which `name` is only the first). descriptionField: 'description', style, }; @@ -665,7 +682,34 @@ export const ObjectMap: React.FC = ({ return null; } - const title = mapConfig.titleField ? record[mapConfig.titleField] : 'Marker'; + // ADR-0079's unified record display-name resolver — the same one + // `ObjectKanban` (:301), `ObjectCalendar` (:356) and `ObjectGantt` + // (:600) already title their items through. `ObjectMap` was the fourth + // renderer and the only one still doing a bare property read against a + // hard-coded `'name'` key, so every object whose display field is not + // literally `name` titled EVERY marker popup `undefined` (objectui#5953). + // + // The declared binding is passed through as the explicit option rather + // than dropped: `getRecordDisplayName` checks `options.titleField` + // first, so an authored `map.titleField` still wins outright. What it + // adds underneath are the steps a static field-name binding + // structurally cannot carry — the object's `nameField`, its deprecated + // `displayNameField` alias, the legacy `titleFormat` TEMPLATE, and the + // record-key probe that runs when no object definition reached us + // (inline `value` data never fetches one; see the schema effect above). + // + // `fallback: 'Marker'` keeps this component's own placeholder, but only + // in the one position where the resolver has nothing left: an id-LESS + // record. A record WITH an id now reads `Record #`, which beats + // `'Marker'` for the reason `'Marker'` was always weak on a map — every + // marker is a marker, so the word separates none of them, while the id + // names exactly one record. `fallback` is a declared option of the + // resolver, so choosing between the two placeholders needs no change to + // `@object-ui/core`. + const title = getRecordDisplayName(objectSchema, record, { + titleField: mapConfig.titleField, + fallback: 'Marker', + }); const description = mapConfig.descriptionField ? record[mapConfig.descriptionField] : undefined; // Ensure lat/lng are within valid ranges @@ -686,7 +730,11 @@ export const ObjectMap: React.FC = ({ .filter((marker): marker is NonNullable => marker !== null); return { markers: validMarkers, invalidCount: invalid }; - }, [data, mapConfig]); + // `objectSchema` is a dependency now, not incidentally: it lands from an + // async fetch AFTER the first paint, and the titles above are resolved + // from it. Omitting it would leave the first-painted markers titled from + // a null object definition for the rest of the component's life. + }, [data, mapConfig, objectSchema]); const selectedMarker = useMemo(() => markers.find(m => m.id === selectedMarkerId),