From 047c22983debe4a3e68d959f1f33c1df9048ac32 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 24 Aug 2026 13:07:45 +0000 Subject: [PATCH 1/2] fix(plugin-map): memoize ObjectMap's mapConfig so the marker useMemo actually memoizes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getMapConfig(schema) ran unmemoized in the render body, giving mapConfig a fresh object identity every render. The marker transform names it in its dependency array, so that useMemo recomputed on every render while declaring that it does not, cascading through filteredMarkers / clusteredData / markerBounds / initialViewState. Memoized on [schema] — the single value getMapConfig reads — rather than on a JSON.stringify deep-compare key: the schema identity reaching this component is already stable across the renders that matter. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CSoz9uGhaaSgiq3hshtN7L --- .changeset/olive-cameras-repeat.md | 17 ++ .../src/ObjectMap.configMemo.test.tsx | 203 ++++++++++++++++++ packages/plugin-map/src/ObjectMap.tsx | 29 ++- 3 files changed, 248 insertions(+), 1 deletion(-) create mode 100644 .changeset/olive-cameras-repeat.md create mode 100644 packages/plugin-map/src/ObjectMap.configMemo.test.tsx diff --git a/.changeset/olive-cameras-repeat.md b/.changeset/olive-cameras-repeat.md new file mode 100644 index 0000000000..6688e0b3d6 --- /dev/null +++ b/.changeset/olive-cameras-repeat.md @@ -0,0 +1,17 @@ +--- +"@object-ui/plugin-map": patch +--- + +`ObjectMap`: make the marker `useMemo` actually memoize. + +`getMapConfig(schema)` ran unmemoized in the render body, so `mapConfig` carried +a fresh object identity on every render. The marker transform names `mapConfig` +in its dependency array, so it recomputed on every single render — walking every +record through `extractCoordinates` and the display-name resolver, and re-running +`ObjectMapConfigSchema.safeParse` on each pass — while declaring that it does not. +The invalidation cascaded on into `filteredMarkers`, `clusteredData`, +`markerBounds` and `initialViewState`. + +`mapConfig` is now memoized on `schema`, the one value `getMapConfig` reads. +Behaviour is unchanged; the config is still rebuilt whenever the schema +genuinely changes. diff --git a/packages/plugin-map/src/ObjectMap.configMemo.test.tsx b/packages/plugin-map/src/ObjectMap.configMemo.test.tsx new file mode 100644 index 0000000000..5dbfa81333 --- /dev/null +++ b/packages/plugin-map/src/ObjectMap.configMemo.test.tsx @@ -0,0 +1,203 @@ +/** + * 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#5976 — the marker `useMemo` declares memoization that never happened. + * + * `getMapConfig(schema)` was called straight in the render body, so `mapConfig` + * carried a FRESH OBJECT IDENTITY on every render. The marker transform names + * it in its dependency array (`[data, mapConfig, objectSchema]`), so that memo + * recomputed on every single render while declaring that it does not — a + * `useMemo` in spelling only. The invalidation then cascaded: `filteredMarkers` + * → `clusteredData` / `markerBounds` → `initialViewState` all key on the array + * it produces, so the whole marker pipeline rebuilt per render. + * + * ## Why these assertions, and not a render count + * + * The defect is about IDENTITY, so it is measured as identity — at the two + * module boundaries this component actually crosses: + * + * - `getRecordDisplayName` (`@object-ui/core`) is called once per record from + * INSIDE the marker memo, so its call count is a direct read of how many + * times that memo evaluated. This is the memo the card names. + * - `initialViewState` is handed to `MapGL` as a prop, and it sits at the tail + * of the cascade (`markers` → `filteredMarkers` → `markerBounds` → + * `initialViewState`). A `toBe` assertion on it therefore pins the whole + * chain, not just the first link. + * + * ## The counter-probe is load-bearing + * + * "Identity is stable" is also satisfiable by freezing a stale config forever, + * which is a worse bug than the one being fixed and is invisible to the + * positive assertion alone. So every stability assertion here is paired with + * one that the identity DOES change when the schema genuinely changes — with a + * visible consequence (the marker title actually re-resolves through the new + * binding, the camera actually moves to the newly declared centre). + */ + +import React from 'react'; +import { render, screen, waitFor, fireEvent } from '@testing-library/react'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const probe = vi.hoisted(() => ({ + displayNameCalls: 0, + viewStates: [] as unknown[], +})); + +vi.mock('@object-ui/core', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + getRecordDisplayName: (...args: Parameters) => { + probe.displayNameCalls += 1; + return actual.getRecordDisplayName(...args); + }, + }; +}); + +vi.mock('react-map-gl/maplibre', () => { + const MapImpl = ({ children, initialViewState }: any) => { + probe.viewStates.push(initialViewState); + return
{children}
; + }; + return { + default: MapImpl, + Map: MapImpl, + NavigationControl: () =>
, + Marker: ({ children, longitude, latitude, onClick }: any) => ( +
onClick?.({ originalEvent: { stopPropagation() {} } })} + > + {children} +
+ ), + Popup: ({ children }: any) =>
{children}
, + }; +}); + +import { ObjectMap } from './ObjectMap'; + +/** + * Inline rows via the array shorthand: the `value` provider settles in one + * effect and never fetches an object definition, so nothing async churns the + * marker memo behind the measurement. + */ +const ROWS = [ + { id: '1', name: 'Harbour Depot', code: 'HD-01', latitude: 47.6062, longitude: -122.3321 }, + { id: '2', name: 'Ridge Yard', code: 'RY-02', latitude: 37.7749, longitude: -122.4194 }, +]; + +const baseSchema = (map: Record): any => ({ + type: 'object-map', + map: { latitudeField: 'latitude', longitudeField: 'longitude', ...map }, + data: ROWS, +}); + +const settle = async () => { + await waitFor(() => expect(screen.queryByText('Loading map...')).toBeNull()); + await waitFor(() => expect(screen.getAllByTestId('map-marker')).toHaveLength(2)); +}; + +beforeEach(() => { + probe.displayNameCalls = 0; + probe.viewStates.length = 0; +}); + +describe('ObjectMap — `mapConfig` identity (objectui#5976)', () => { + it('holds the marker memo across a re-render with unchanged inputs', async () => { + const schema = baseSchema({ titleField: 'name' }); + + const { rerender } = render(); + await settle(); + + const callsAtRest = probe.displayNameCalls; + expect(callsAtRest).toBeGreaterThan(0); + + rerender(); + + // The memo's declared intent, asserted: same inputs in, no re-evaluation. + expect(probe.displayNameCalls).toBe(callsAtRest); + }); + + it('holds the whole downstream cascade — `initialViewState` keeps its identity', async () => { + const schema = baseSchema({ titleField: 'name' }); + + const { rerender } = render(); + await settle(); + + const cameraAtRest = probe.viewStates.at(-1); + expect(cameraAtRest).toBeDefined(); + + rerender(); + + // `markers` → `filteredMarkers` → `markerBounds` → `initialViewState`: + // a fresh `mapConfig` invalidates every link, so this identity is a read + // of the entire chain. + expect(probe.viewStates.at(-1)).toBe(cameraAtRest); + }); + + it('holds the marker memo across a state-driven re-render (search typing)', async () => { + const schema = baseSchema({ titleField: 'name' }); + + render(); + await settle(); + + const callsAtRest = probe.displayNameCalls; + + // A re-render this component causes ITSELF — the dominant case, and the + // one where the `schema` prop identity is unchanged by construction. + fireEvent.change(screen.getByPlaceholderText('Search locations…'), { + target: { value: 'Harbour' }, + }); + await waitFor(() => expect(screen.getAllByTestId('map-marker')).toHaveLength(1)); + + // `filteredMarkers` legitimately re-runs on the query; `markers` must not. + expect(probe.displayNameCalls).toBe(callsAtRest); + }); + + // --------------------------------------------------------------------- + // Counter-probes: the identity MUST change when the schema genuinely does. + // Without these, "stable identity" is satisfiable by a frozen stale config. + // --------------------------------------------------------------------- + + it('re-resolves marker titles when the declared `titleField` changes', async () => { + const { rerender } = render(); + await settle(); + + fireEvent.click(screen.getAllByTestId('map-marker')[0]); + expect(screen.getByTestId('map-popup').textContent).toContain('Harbour Depot'); + + const callsBefore = probe.displayNameCalls; + + rerender(); + await waitFor(() => expect(screen.getAllByTestId('map-marker')).toHaveLength(2)); + + // Recomputed, and visibly so: the new binding reached the rendered title. + expect(probe.displayNameCalls).toBeGreaterThan(callsBefore); + await waitFor(() => + expect(screen.getByTestId('map-popup').textContent).toContain('HD-01'), + ); + }); + + it('rebuilds the camera when the declared centre/zoom changes', async () => { + const { rerender } = render(); + await settle(); + + const cameraBefore = probe.viewStates.at(-1) as Record; + + rerender(); + await waitFor(() => expect(screen.getAllByTestId('map-marker')).toHaveLength(2)); + + const cameraAfter = probe.viewStates.at(-1) as Record; + expect(cameraAfter).not.toBe(cameraBefore); + expect(cameraAfter).toMatchObject({ latitude: 10, longitude: 20, zoom: 7 }); + }); +}); diff --git a/packages/plugin-map/src/ObjectMap.tsx b/packages/plugin-map/src/ObjectMap.tsx index d764233dcc..2e87977368 100644 --- a/packages/plugin-map/src/ObjectMap.tsx +++ b/packages/plugin-map/src/ObjectMap.tsx @@ -591,7 +591,34 @@ export const ObjectMap: React.FC = ({ return rawDataConfig; }, [JSON.stringify(rawDataConfig)]); - const mapConfig = getMapConfig(schema); + /** + * Memoized on the ONE value `getMapConfig` reads, and nothing else + * (objectui#5976). This call used to sit bare in the render body, so + * `mapConfig` carried a fresh object identity on every render — and the + * marker transform below names it in its dependency array, so that `useMemo` + * recomputed on every single render while declaring that it does not. The + * invalidation cascaded from there: `filteredMarkers` → `clusteredData` / + * `markerBounds` → `initialViewState` all key on the array it produces. + * + * `[schema]` is the whole dependency, not a shorthand for one: `getMapConfig` + * is a pure function of `schema` and reads nothing else. No deep-compare key + * is needed to make that identity hold, because the identity that reaches + * this component is ALREADY stable across the renders that matter — every + * re-render `ObjectMap` causes itself (data landing, the object definition + * landing, search typing, zoom, selection, geolocation) leaves the prop + * untouched by construction, and the three callers upstream each hand over a + * memoized node: `SchemaRenderer`'s `evaluatedSchema`, the gate's `mapped` + * in `useElementDataSourceSchema`, and `ListView`'s `viewComponentSchema`. + * + * So this deliberately does NOT copy the `JSON.stringify` dep key the + * `dataConfig` line above uses. That idiom buys stability by paying a + * serialize on every render, and it is also key-order sensitive and drops + * `undefined` values — an equality this config cannot afford, since an + * ABSENT `titleField` is load-bearing here (objectui#5953) and must never + * compare equal to a present one. Identity is enough; nothing here needs + * value equality. + */ + const mapConfig = useMemo(() => getMapConfig(schema), [schema]); const hasInlineData = dataConfig?.provider === 'value'; // Fetch data based on provider From 0f754a83928104fb224b6b7e9eca9a613bdd2978 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 24 Aug 2026 13:13:53 +0000 Subject: [PATCH 2/2] test(plugin-map): avoid Array.prototype.at in the identity pin `at` is outside this package's configured `lib` target, so `tsc -p tsconfig.test.json` rejected it (TS2550). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CSoz9uGhaaSgiq3hshtN7L --- packages/plugin-map/src/ObjectMap.configMemo.test.tsx | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/plugin-map/src/ObjectMap.configMemo.test.tsx b/packages/plugin-map/src/ObjectMap.configMemo.test.tsx index 5dbfa81333..4851e1416f 100644 --- a/packages/plugin-map/src/ObjectMap.configMemo.test.tsx +++ b/packages/plugin-map/src/ObjectMap.configMemo.test.tsx @@ -101,6 +101,9 @@ const baseSchema = (map: Record): any => ({ data: ROWS, }); +/** `Array.prototype.at` is outside this package's configured `lib` target. */ +const lastCamera = (): unknown => probe.viewStates[probe.viewStates.length - 1]; + const settle = async () => { await waitFor(() => expect(screen.queryByText('Loading map...')).toBeNull()); await waitFor(() => expect(screen.getAllByTestId('map-marker')).toHaveLength(2)); @@ -133,7 +136,7 @@ describe('ObjectMap — `mapConfig` identity (objectui#5976)', () => { const { rerender } = render(); await settle(); - const cameraAtRest = probe.viewStates.at(-1); + const cameraAtRest = lastCamera(); expect(cameraAtRest).toBeDefined(); rerender(); @@ -141,7 +144,7 @@ describe('ObjectMap — `mapConfig` identity (objectui#5976)', () => { // `markers` → `filteredMarkers` → `markerBounds` → `initialViewState`: // a fresh `mapConfig` invalidates every link, so this identity is a read // of the entire chain. - expect(probe.viewStates.at(-1)).toBe(cameraAtRest); + expect(lastCamera()).toBe(cameraAtRest); }); it('holds the marker memo across a state-driven re-render (search typing)', async () => { @@ -191,12 +194,12 @@ describe('ObjectMap — `mapConfig` identity (objectui#5976)', () => { const { rerender } = render(); await settle(); - const cameraBefore = probe.viewStates.at(-1) as Record; + const cameraBefore = lastCamera() as Record; rerender(); await waitFor(() => expect(screen.getAllByTestId('map-marker')).toHaveLength(2)); - const cameraAfter = probe.viewStates.at(-1) as Record; + const cameraAfter = lastCamera() as Record; expect(cameraAfter).not.toBe(cameraBefore); expect(cameraAfter).toMatchObject({ latitude: 10, longitude: 20, zoom: 7 }); });