From 83e7405e096f7fc787b3cbc9c11ebbd8d57d2c78 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 05:55:20 +0000 Subject: [PATCH 1/2] perf(plugin-map): key ObjectMap's dataConfig memo on [schema], not a per-render JSON.stringify MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `getDataConfig(schema)` was called bare in the render body and its result re-serialized with `JSON.stringify` on every render, only to hand back the object the memo already held. `getDataConfig` is a pure function of `schema` (it reads exactly `data`, `staticData`, `objectName`), so `[schema]` gives the same stable identity the fetch effect needs with no serialize and no per-render rebuild — the shape #5976 landed one line below for `mapConfig`. Dropping the serialize is also a correctness move. `JSON.stringify` throws on a value it cannot serialize, and the passthrough branch returns the author's own `schema.data` verbatim, inline rows included — so a record graph with a back-reference took the whole map subtree down from the render body. Part of #6018 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CSoz9uGhaaSgiq3hshtN7L --- .../src/ObjectMap.dataConfigMemo.test.tsx | 219 ++++++++++++++++++ packages/plugin-map/src/ObjectMap.tsx | 57 ++++- 2 files changed, 264 insertions(+), 12 deletions(-) create mode 100644 packages/plugin-map/src/ObjectMap.dataConfigMemo.test.tsx diff --git a/packages/plugin-map/src/ObjectMap.dataConfigMemo.test.tsx b/packages/plugin-map/src/ObjectMap.dataConfigMemo.test.tsx new file mode 100644 index 0000000000..e057507893 --- /dev/null +++ b/packages/plugin-map/src/ObjectMap.dataConfigMemo.test.tsx @@ -0,0 +1,219 @@ +/** + * 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#6018 — `dataConfig` bought its identity with a per-render serialize. + * + * ```ts + * const rawDataConfig = getDataConfig(schema); // bare call, every render + * const dataConfig = useMemo(() => rawDataConfig, [JSON.stringify(rawDataConfig)]); + * ``` + * + * `dataConfig` is a dependency of the fetch effect, and that effect calls + * `setData` — so a fresh identity there is a refetch loop, not merely waste. + * That hazard is what the deleted "prevent infinite loops" comment recorded, + * and it is why this file exists: the serialize is only removable if the + * identity contract it was standing in for survives without it. + * + * The replacement is the shape objectui#5976 landed one line below for + * `mapConfig`: `useMemo(() => getDataConfig(schema), [schema])`. `getDataConfig` + * is a pure function of `schema` and reads nothing else (it reads exactly + * `schema.data`, `schema.staticData`, `schema.objectName`), so `[schema]` is the + * whole dependency rather than a shorthand for one. + * + * ## What is asserted, and what is deliberately NOT + * + * This is a performance card, so the pin is the IDENTITY CONTRACT, not the + * timing. Nothing here counts renders as a proxy for speed and nothing measures + * how long a serialize takes — both are unfalsifiable on a loaded CI box. What + * is measured is the one observable the contract is about: **how many times the + * fetch effect fired**, read at the module boundary it crosses (`dataSource.find`). + * + * Both directions are pinned, because "stable identity" is equally satisfiable + * by freezing a stale config forever — a worse bug, and invisible to the + * stability assertion alone: + * + * - unchanged `schema` in ⇒ the effect does NOT re-fire; + * - genuinely changed `schema` in ⇒ it DOES, against the new object. + * + * ## Which of these survive a revert (measured, not assumed) + * + * The two identity-contract tests pass on the serialize form as well: the + * stringify key really did buy a stable identity, which is why objectui#6018 is + * a cost card and not a correctness card. They are regression pins for the + * replacement, and they are the tests the ruling asked to be verified against — + * their value is that they FAIL if the memo is ever re-keyed on something + * render-fresh again (which is the objectui#5976 defect, one line up). + * + * The third test is the one that is RED before the fix. `JSON.stringify` is not + * a total function: it throws on a value it cannot serialize. Sited in the + * render body, over a config that in the passthrough branch is the author's own + * `schema.data` object — inline `value` rows included, verbatim — that throw + * takes down the whole map subtree. A record graph carrying a back-reference + * (an `$expand`-ed lookup handed to the block as inline data) is the reachable + * shape; a `BigInt` id from an adapter is a second one. `[schema]` compares + * identities and never serializes, so the value never has to be serializable at + * all. + */ + +import React from 'react'; +import { render, screen, waitFor, fireEvent } from '@testing-library/react'; +import { describe, it, expect, vi } from 'vitest'; + +vi.mock('react-map-gl/maplibre', () => ({ + default: ({ children }: any) =>
{children}
, + Map: ({ children }: any) =>
{children}
, + NavigationControl: () =>
, + Marker: ({ children, longitude, latitude }: any) => ( +
+ {children} +
+ ), + Popup: ({ children }: any) =>
{children}
, +})); + +import { ObjectMap } from './ObjectMap'; + +const MAP = { latitudeField: 'latitude', longitudeField: 'longitude', titleField: 'name' }; + +const ROWS = [ + { id: '1', name: 'Harbour Depot', latitude: 47.6062, longitude: -122.3321 }, + { id: '2', name: 'Ridge Yard', latitude: 37.7749, longitude: -122.4194 }, +]; + +function makeAdapter() { + return { + find: vi.fn().mockResolvedValue({ data: ROWS }), + findOne: vi.fn(), + create: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + getObjectSchema: vi.fn().mockResolvedValue({ + name: 'store', + fields: { + name: { type: 'text' }, + latitude: { type: 'number' }, + longitude: { type: 'number' }, + }, + }), + }; +} + +/** + * Settle to the steady state: the map has painted its markers AND the second + * effect's `objectSchema` has landed. That second effect writes state which is + * itself a dependency of the fetch effect, so the resting call count is only + * meaningful once it has stopped moving. + */ +const settle = async (adapter: ReturnType) => { + await waitFor(() => expect(screen.queryByText('Loading map...')).toBeNull()); + await waitFor(() => expect(screen.getAllByTestId('map-marker')).toHaveLength(2)); + await waitFor(() => expect(adapter.getObjectSchema).toHaveBeenCalled()); + const seen = adapter.find.mock.calls.length; + await new Promise((r) => setTimeout(r, 0)); + expect(adapter.find.mock.calls.length).toBe(seen); +}; + +describe('ObjectMap — `dataConfig` identity contract (objectui#6018)', () => { + it('does not re-fire the fetch effect on a re-render with an unchanged `schema`', async () => { + const adapter = makeAdapter(); + const schema: any = { type: 'object-map', map: MAP, objectName: 'store' }; + + const { rerender } = render(); + await settle(adapter); + + const callsAtRest = adapter.find.mock.calls.length; + expect(callsAtRest).toBeGreaterThan(0); + + // The prop identity is unchanged — which is what every re-render this + // component causes ITSELF looks like, and what all three upstream callers + // hand over (a memoized node in each case). + rerender(); + await new Promise((r) => setTimeout(r, 0)); + + expect(adapter.find.mock.calls.length).toBe(callsAtRest); + }); + + it('does not re-fire the fetch effect on a state-driven re-render (search typing)', async () => { + const adapter = makeAdapter(); + const schema: any = { type: 'object-map', map: MAP, objectName: 'store' }; + + render(); + await settle(adapter); + + const callsAtRest = adapter.find.mock.calls.length; + + // The dominant case: a re-render driven by this component's own state, with + // the `schema` prop untouched by construction. A render-fresh `dataConfig` + // makes this a refetch — and each refetch's `setData` drives another render. + fireEvent.change(screen.getByPlaceholderText('Search locations…'), { + target: { value: 'Harbour' }, + }); + await waitFor(() => expect(screen.getAllByTestId('map-marker')).toHaveLength(1)); + await new Promise((r) => setTimeout(r, 0)); + + expect(adapter.find.mock.calls.length).toBe(callsAtRest); + }); + + // ------------------------------------------------------------------ + // Counter-probe. Without it, "stable identity" is satisfiable by never + // recomputing at all, which is a worse bug than the one being fixed. + // ------------------------------------------------------------------ + + it('DOES re-fire against the new object when `schema` genuinely changes', async () => { + const adapter = makeAdapter(); + + const { rerender } = render( + , + ); + await settle(adapter); + + expect(adapter.find.mock.calls.map((c) => c[0])).toContain('store'); + const callsBefore = adapter.find.mock.calls.length; + + rerender( + , + ); + + await waitFor(() => { + expect(adapter.find.mock.calls.length).toBeGreaterThan(callsBefore); + }); + // Recomputed, and visibly so: the query actually went to the new object. + expect(adapter.find.mock.calls.map((c) => c[0])).toContain('warehouse'); + }); + + // ------------------------------------------------------------------ + // The direction that is RED before the fix. + // ------------------------------------------------------------------ + + it('renders inline data the serializer cannot handle — identity needs no round-trip', async () => { + // A record carrying a back-reference to its own graph: what an `$expand`-ed + // lookup looks like once a host hands the resolved rows to the block as + // inline data. `getDataConfig`'s passthrough branch returns `schema.data` + // VERBATIM, so a per-render `JSON.stringify` over it runs over these rows — + // and throws `TypeError: Converting circular structure to JSON` from the + // render body, taking the whole map subtree down with it. + const depot: any = { id: '1', name: 'Harbour Depot', latitude: 47.6062, longitude: -122.3321 }; + depot.parent = depot; + + const schema: any = { + type: 'object-map', + map: MAP, + data: { provider: 'value', items: [depot] }, + }; + + expect(() => render()).not.toThrow(); + await waitFor(() => expect(screen.getAllByTestId('map-marker')).toHaveLength(1)); + }); +}); diff --git a/packages/plugin-map/src/ObjectMap.tsx b/packages/plugin-map/src/ObjectMap.tsx index 2e87977368..12e16b135a 100644 --- a/packages/plugin-map/src/ObjectMap.tsx +++ b/packages/plugin-map/src/ObjectMap.tsx @@ -585,11 +585,42 @@ export const ObjectMap: React.FC = ({ ); }, []); - const rawDataConfig = getDataConfig(schema); - // Memoize dataConfig using deep comparison to prevent infinite loops - const dataConfig = useMemo(() => { - return rawDataConfig; - }, [JSON.stringify(rawDataConfig)]); + /** + * Memoized on the ONE value `getDataConfig` reads, and nothing else + * (objectui#6018) — the same shape `mapConfig` below landed for + * objectui#5976, now that the two lines agree. + * + * `dataConfig` is a dependency of the fetch effect, and that effect calls + * `setData`, so a fresh identity here is a refetch loop rather than mere + * waste. That hazard is real and is what the previous form's "prevent + * infinite loops" comment recorded. What has changed is the PRICE of the + * guard, not the guard: identity was bought by calling `getDataConfig` bare + * in the render body and re-serializing the result with `JSON.stringify` on + * EVERY render, only to hand back the object the memo already held. + * + * `[schema]` is the whole dependency, not a shorthand for one: `getDataConfig` + * is a pure function of `schema` and reads exactly three keys off it + * (`data`, `staticData`, `objectName`), nothing ambient. So no deep-compare + * key is needed to make the identity hold — 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, which is precisely the loop path; and the three + * callers upstream each hand over a memoized node: `SchemaRenderer`'s + * `evaluatedSchema`, the gate's `mapped` in `useElementDataSourceSchema`, + * and `ListView`'s `viewComponentSchema`. + * + * Dropping the serialize is also a correctness move, not only a cost one. + * `JSON.stringify` is not a total function — it THROWS on a value it cannot + * serialize — and the passthrough branch of `getDataConfig` returns the + * author's own `schema.data` object verbatim, inline `value` rows included. + * So a record graph carrying a back-reference (an `$expand`-ed lookup handed + * to the block as inline data) or a `BigInt` id took the whole map subtree + * down from the render body. Comparing identities never serializes, so the + * config no longer has to be serializable at all. + * `ObjectMap.dataConfigMemo.test.tsx` pins both halves. + */ + const dataConfig = useMemo(() => getDataConfig(schema), [schema]); /** * Memoized on the ONE value `getMapConfig` reads, and nothing else @@ -610,13 +641,15 @@ export const ObjectMap: React.FC = ({ * 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. + * This deliberately did NOT copy the `JSON.stringify` dep key the + * `dataConfig` line above used to carry, and as of objectui#6018 that line + * no longer carries it either — both are keyed on `[schema]` now. The + * serialize idiom bought stability by paying a full serialize on every + * render; it is also key-order sensitive and drops `undefined` values — an + * equality THIS config in particular 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'; From 8311dbb0ea3a05d1c802ed7cb044f0206747f024 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 05:59:05 +0000 Subject: [PATCH 2/2] chore(changeset): patch @object-ui/plugin-map for the dataConfig memo fix Part of #6018 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CSoz9uGhaaSgiq3hshtN7L --- .changeset/wild-donkeys-shake.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 .changeset/wild-donkeys-shake.md diff --git a/.changeset/wild-donkeys-shake.md b/.changeset/wild-donkeys-shake.md new file mode 100644 index 0000000000..e50f7c70c5 --- /dev/null +++ b/.changeset/wild-donkeys-shake.md @@ -0,0 +1,18 @@ +--- +"@object-ui/plugin-map": patch +--- + +`ObjectMap` no longer serializes its data config on every render. + +`getDataConfig(schema)` was called bare in the render body and its result +re-serialized with `JSON.stringify` on every render, purely to give `dataConfig` +the stable identity its fetch effect depends on. `getDataConfig` is a pure +function of `schema`, so `useMemo(() => getDataConfig(schema), [schema])` gives +the same identity with no serialize and no per-render rebuild. + +This also fixes a crash. `JSON.stringify` throws on a value it cannot serialize, +and the config's passthrough branch returns the author's own `schema.data` +object verbatim — inline rows included. A map handed inline records carrying a +back-reference (an `$expand`-ed lookup) or a `BigInt` id threw from the render +body and took the whole map subtree down with it. Comparing identities never +serializes, so the config no longer has to be serializable at all.