diff --git a/.changeset/objectmap-data-prop-and-zoom-seed-5003.md b/.changeset/objectmap-data-prop-and-zoom-seed-5003.md new file mode 100644 index 000000000..6ae4ce307 --- /dev/null +++ b/.changeset/objectmap-data-prop-and-zoom-seed-5003.md @@ -0,0 +1,26 @@ +--- +"@object-ui/plugin-map": patch +--- + +Fix two pieces of `ObjectMap` state that stopped tracking their source after mount +(objectui#5003): + +- **`data` prop threading**: the fetch effect preferred records passed via `props.data`, + but `data` was read off the `rest` spread and was not one of the effect's dependencies. + A host rendering `` while its own query is in flight, then + re-rendering with the resolved rows, kept showing the empty map — the prop changed, the + effect never re-ran to notice. `data` is now a declared prop (`data: dataProp`), tracked + directly in the effect's dependency array; the whole `rest` object is intentionally + **not** added there (it is a fresh object every render, which would refetch on every + render instead). +- **Clustering zoom seed**: `currentZoom` — what `clusterMarkers` uses for its grid cell + size — was seeded with a nominal `mapConfig.zoom || 3` and updated only by `onZoom`. + MapLibre applies the initial camera (including a `bounds` fit) via its constructor, + before react-map-gl attaches React's event handlers, so no `onZoom` ever fired for that + first camera — the seed stayed nominal until the user's first zoom. It is now also + seeded from `onLoad`, which fires once the initial camera has settled, so clustering at + first paint reflects the camera MapLibre actually applied. + +Both were dormant on the console path (never exercised in the example apps) — see the +issue for why — so this ships with dedicated tests exercising the prop-update path and +the pre-`onZoom` clustering state directly. diff --git a/packages/plugin-map/README.md b/packages/plugin-map/README.md index e9a91b93b..48f3a04cc 100644 --- a/packages/plugin-map/README.md +++ b/packages/plugin-map/README.md @@ -147,6 +147,7 @@ import { ObjectMap } from '@object-ui/plugin-map'; | `schema` | The map schema — the keys above. | | `dataSource` | Resolves the `object` provider. Not needed for `staticData` or an inline `data` array. | | `className` | Classes for the wrapper around the map. | +| `data` | Records to render directly, bypassing the component's own fetch — the shape `ListView` passes when it already holds the rows. Tracked live: passing a new array after mount (e.g. once a host's own in-flight query resolves) updates the map. | | `onMarkerClick` | Called with the clicked record. | | `onRowClick` | Record click handler; takes priority over the `navigation` overlay. | | `onEdit` / `onDelete` | Passing either adds that button to the marker popup (and to the mobile record sheet). | diff --git a/packages/plugin-map/src/ObjectMap.dataProp.test.tsx b/packages/plugin-map/src/ObjectMap.dataProp.test.tsx new file mode 100644 index 000000000..d4ad952df --- /dev/null +++ b/packages/plugin-map/src/ObjectMap.dataProp.test.tsx @@ -0,0 +1,86 @@ +/** + * 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. + * + * Regression (objectui#5003): `props.data` was read by the fetch effect (via + * the `rest` spread) but was not one of its dependencies. A host rendering + * `` while its own query is in flight, then passing + * the resolved rows, saw the effect's copy of `data` stay empty forever — the + * prop changed, but the effect never re-ran to notice. + * + * This is UNREACHABLE through the console path: `ElementDataSourceGate` (see + * `ObjectMap.elementDataSource.test.tsx`) always drives the map through its + * own `dataSource.find`, never through this prop. So the tests below mount + * with `data` directly — the only way to exercise the buggy dependency list. + */ +import React from 'react'; +import { render, screen, waitFor } from '@testing-library/react'; +import { describe, it, expect, vi } from 'vitest'; +import { ObjectMap } from './ObjectMap'; + +// Mock react-map-gl/maplibre the same way the sibling ObjectMap tests do — no +// WebGL canvas in the test env, and every assertion here is about the marker +// count reaching the DOM, not the map surface itself. +vi.mock('react-map-gl/maplibre', () => ({ + default: (props: any) =>
{props.children}
, + Map: ({ children }: any) =>
{children}
, + NavigationControl: () =>
, + Marker: ({ children, longitude, latitude }: any) => ( +
+ {children} +
+ ), + Popup: ({ children }: any) =>
{children}
, +})); + +const MAP_CONFIG = { latitudeField: 'latitude', longitudeField: 'longitude', titleField: 'name' }; + +const rows = [ + { id: '1', name: 'Loc 1', latitude: 40, longitude: -74 }, + { id: '2', name: 'Loc 2', latitude: 41, longitude: -75 }, +]; + +describe('ObjectMap — props.data threading (objectui#5003)', () => { + it('picks up rows passed as a prop AFTER mount, not just at mount', async () => { + const schema: any = { type: 'map', map: MAP_CONFIG }; + + const { rerender } = render(); + + await waitFor(() => expect(screen.queryByText('Loading map...')).toBeNull()); + expect(screen.queryAllByTestId('map-marker')).toHaveLength(0); + + // The host's own query resolves later and re-renders with the rows — + // exactly the sequence the issue describes (a query in flight at first + // paint). Nothing else about the schema changes. + rerender(); + + await waitFor(() => { + expect(screen.getAllByTestId('map-marker')).toHaveLength(2); + }); + const markers = screen.getAllByTestId('map-marker'); + expect(markers[0]).toHaveAttribute('data-lat', '40'); + expect(markers[1]).toHaveAttribute('data-lat', '41'); + }); + + it('does not refetch on every render — a stable `data` prop is read once', async () => { + const schema: any = { type: 'map', map: MAP_CONFIG }; + + const { rerender } = render(); + await waitFor(() => expect(screen.getAllByTestId('map-marker')).toHaveLength(2)); + + // Re-render with the SAME `data` array identity but a different unrelated + // prop. If the effect depended on the whole `rest` object (a fresh object + // every render) instead of the declared `data` prop, this would be + // indistinguishable from a genuine data change and would flip back + // through the `loading` gate, unmounting `MapGL` for a beat. + rerender(); + + // Give any spurious effect a tick to fire before asserting steady state. + await new Promise((r) => setTimeout(r, 0)); + expect(screen.queryByText('Loading map...')).toBeNull(); + expect(screen.getAllByTestId('map-marker')).toHaveLength(2); + }); +}); diff --git a/packages/plugin-map/src/ObjectMap.tsx b/packages/plugin-map/src/ObjectMap.tsx index 5331a46a1..c53995bf4 100644 --- a/packages/plugin-map/src/ObjectMap.tsx +++ b/packages/plugin-map/src/ObjectMap.tsx @@ -50,6 +50,15 @@ export interface ObjectMapProps { schema: ObjectMapSchema; dataSource?: DataSource; className?: string; + /** + * Records to render directly, bypassing this component's own fetch — the + * shape `ListView` passes when it already holds the rows. Declared as its + * own prop (not read off the `rest` spread) so the fetch effect can depend + * on this one value: naming the whole `rest` object in that effect's deps + * instead would refetch on every render, since `rest` is a fresh object + * each render (objectui#5003). + */ + data?: any[]; onMarkerClick?: (record: any) => void; onRowClick?: (record: any) => void; onEdit?: (record: any) => void; @@ -474,13 +483,13 @@ export const ObjectMap: React.FC = ({ schema, dataSource, className, + data: dataProp, onMarkerClick, onRowClick, onEdit, onDelete, enableClustering, clusterRadius = 50, - ...rest }) => { const [data, setData] = useState([]); const [loading, setLoading] = useState(true); @@ -542,14 +551,14 @@ export const ObjectMap: React.FC = ({ try { setLoading(true); - // Prioritize data passed via props (from ListView) - if ((rest as any).data) { // Check props.data directly first - const passed = (rest as any).data; - if (Array.isArray(passed)) { - setData(passed); - setLoading(false); - return; - } + // Prioritize data passed via props (from ListView). `dataProp` is a + // declared prop (not the `rest` spread), so it can sit in this + // effect's dependency array below without turning into a + // refetch-every-render trap (objectui#5003). + if (Array.isArray(dataProp)) { + setData(dataProp); + setLoading(false); + return; } // Check schema.data next @@ -597,7 +606,7 @@ export const ObjectMap: React.FC = ({ }; fetchData(); - }, [dataConfig, dataSource, hasInlineData, schema.filter, schema.sort, objectSchema]); + }, [dataProp, dataConfig, dataSource, hasInlineData, schema.filter, schema.sort, objectSchema]); // Fetch object schema for field metadata useEffect(() => { @@ -663,6 +672,28 @@ export const ObjectMap: React.FC = ({ const [currentZoom, setCurrentZoom] = useState(mapConfig.zoom || 3); + /** + * Seed `currentZoom` from the camera MapLibre actually applies at mount, + * instead of leaving it at the nominal `mapConfig.zoom || 3` above until + * the user's first zoom. `initialViewState` (computed below) — including a + * `bounds` fit — is resolved by the constructor before react-map-gl attaches + * its React event handlers, so no `onZoom` fires for that first camera. + * `onLoad` fires once the style has loaded and the initial camera has + * settled (fit-bounds included), so reading the zoom off that event + * captures the real applied value without waiting on user interaction + * (objectui#5003). + */ + const handleMapLoad = useCallback((e: { target?: { getZoom?: () => number } }) => { + try { + const zoom = e?.target?.getZoom?.(); + if (typeof zoom === 'number' && Number.isFinite(zoom)) { + setCurrentZoom(Math.round(zoom)); + } + } catch { + /* ignore — falls back to the nominal seed / next onZoom */ + } + }, []); + const navigation = useNavigationOverlay({ navigation: schema.navigation, objectName: schema.objectName, @@ -789,6 +820,7 @@ export const ObjectMap: React.FC = ({ touchZoomRotate={true} dragRotate={true} touchPitch={true} + onLoad={handleMapLoad} onZoom={(e) => setCurrentZoom(Math.round(e.viewState.zoom))} onError={handleMapError} > diff --git a/packages/plugin-map/src/ObjectMap.zoomSeed.test.tsx b/packages/plugin-map/src/ObjectMap.zoomSeed.test.tsx new file mode 100644 index 000000000..b8de6d409 --- /dev/null +++ b/packages/plugin-map/src/ObjectMap.zoomSeed.test.tsx @@ -0,0 +1,107 @@ +/** + * 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. + * + * Regression (objectui#5003): `currentZoom` — the value `clusterMarkers` uses + * for its grid cell size — was seeded with the NOMINAL `mapConfig.zoom || 3` + * and updated only by `onZoom`. MapLibre applies the initial camera + * (`initialViewState`, including a `bounds` fit) via its constructor, before + * react-map-gl attaches React's event handlers, so no `onZoom` ever fires for + * that first camera — the seed stayed nominal until the user's first zoom. + * + * This was equally true before objectui#4941 (the old nominal seed was `10`); + * that PR did not introduce it. Recorded here so it is not rediscovered as a + * regression of that PR. + * + * Dormant on the console path: clustering only engages above 100 markers (or + * explicit opt-in), so a stale seed has no visible effect on the small sets + * in the examples. These tests opt in explicitly and use a marker pair whose + * grid-cell membership flips between the nominal seed (3) and a zoomed-in + * camera (12), so the cluster/no-cluster split is a direct, observable proxy + * for what `currentZoom` actually holds. + */ +import React from 'react'; +import { act, render, screen, waitFor } from '@testing-library/react'; +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { ObjectMap } from './ObjectMap'; + +let capturedProps: any = null; + +vi.mock('react-map-gl/maplibre', () => ({ + default: (props: any) => { + capturedProps = props; + return
{props.children}
; + }, + Map: ({ children }: any) =>
{children}
, + NavigationControl: () =>
, + Marker: ({ children, longitude, latitude }: any) => ( +
+ {children} +
+ ), + Popup: ({ children }: any) =>
{children}
, +})); + +// ~1 degree apart in both axes: at the nominal seed (zoom 3, cell size +// 50/2^3 = 6.25deg) both fall in the same grid cell and cluster into one; at +// zoom 12 (cell size 50/2^12 ≈ 0.0122deg) they land in different cells and +// render as two separate markers. The split is the observable signal. +const twoNearbyCities = [ + { id: '1', name: 'Loc 1', latitude: 40, longitude: -74 }, + { id: '2', name: 'Loc 2', latitude: 41, longitude: -75 }, +]; + +const schema: any = { + type: 'map', + map: { latitudeField: 'latitude', longitudeField: 'longitude', titleField: 'name' }, + // No declared `zoom` — the nominal `mapConfig.zoom || 3` seed applies. + data: { provider: 'value', items: twoNearbyCities }, +}; + +describe('ObjectMap — clustering zoom seed (objectui#5003)', () => { + beforeEach(() => { + capturedProps = null; + }); + + it('starts clustered under the nominal seed, then splits once the applied camera is read', async () => { + render(); + + await waitFor(() => expect(screen.queryByText('Loading map...')).toBeNull()); + await waitFor(() => expect(capturedProps).not.toBeNull()); + + // Before `onLoad` fires: the nominal seed (3) groups the pair into one + // cluster — this is the bug's starting state, present on both legs. The + // cluster pin is itself wrapped in a `Marker`, so `map-marker` reads 1 + // here too (one wrapper, holding the one `map-cluster` div). + await waitFor(() => { + expect(screen.getAllByTestId('map-cluster')).toHaveLength(1); + }); + expect(screen.getAllByTestId('map-marker')).toHaveLength(1); + + // MapLibre's 'load' event: the camera has settled. `onLoad` is undefined + // on the unfixed component (no handler wired), so this is a no-op there — + // predicted red assertion below is what tells the two legs apart. + act(() => { + capturedProps.onLoad?.({ target: { getZoom: () => 12 } }); + }); + + await waitFor(() => { + expect(screen.getAllByTestId('map-marker')).toHaveLength(2); + }); + expect(screen.queryAllByTestId('map-cluster')).toHaveLength(0); + }); + + it('ignores an onLoad event carrying no readable zoom', async () => { + render(); + await waitFor(() => expect(capturedProps).not.toBeNull()); + await waitFor(() => expect(screen.getAllByTestId('map-cluster')).toHaveLength(1)); + + // No `target`, or a `target` without `getZoom` — must not throw, and must + // leave the seed (and therefore the clustering) unchanged. + expect(() => act(() => { capturedProps.onLoad?.({}); })).not.toThrow(); + expect(screen.getAllByTestId('map-cluster')).toHaveLength(1); + }); +});