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),