Skip to content

ObjectMap's marker useMemo never memoizes — getMapConfig runs unmemoized in the render body, so mapConfig has a fresh identity every render #5976

Description

@yinlianghui

Measured while implementing #5953 (PR #5975). Out of that card's scope — filing plainly for triage to grade.

What was measured

packages/plugin-map/src/ObjectMap.tsx:594 calls the config builder straight in the render body, unmemoized:

const mapConfig = getMapConfig(schema);

Every render therefore produces a new object identity. The marker transform names it in its dependency array (ObjectMap.tsx:737):

}, [data, mapConfig, objectSchema]);

so that useMemo recomputes on every single render — it is a useMemo in spelling only. The recomputation walks all records through extractCoordinates and the display-name resolver. Downstream, filteredMarkers / clusteredData / markerBounds each depend on markers, so the invalidation cascades through the whole marker pipeline.

The sibling dataConfig immediately above it (ObjectMap.tsx:590) shows the shape the file already knows about, deliberately memoized on a deep-compare key:

const dataConfig = useMemo(() => rawDataConfig, [JSON.stringify(rawDataConfig)]);

mapConfig got no equivalent.

Second-order cost inside getMapConfig itself

Because it is unmemoized, everything getMapConfig does happens per render, not per config change:

  • ObjectMapConfigSchema.safeParse(config) — a full zod parse on every render when a declared map block is present;
  • warnOnLegacyFilterMapConfig, warnOnTopLevelStyleUrl, warnOnShadowedFlatMapKeys.

The warn helpers are documented as warning "once per distinct stash" and carry their own dedupe, so this is a cost question rather than a log-spam one — but it is being paid on a per-render cadence.

Why this is worth its own card rather than a rider

It is not a correctness bug on any path I exercised: the recompute is wasteful, not wrong, and #5953's own change (adding objectSchema to that dep array, which it genuinely needs — the object definition arrives from an async fetch after first paint) neither causes nor worsens it. The fix is a different shape from that card's — memoize mapConfig, presumably on the same deep-compare key idiom the neighbouring dataConfig already uses — and touching it there would have been unreviewable scope creep inside a display-name fix.

Unassigned; filed plainly for triage to grade.


Generated by Claude Code

Activity

  1. added
    domain:uiobjectui ui stream: fix lands on the published library or apps — objectui execution seat
    on Aug 24, 2026
  2. added theissue type on Aug 24, 2026
  3. os-zhuang commented on Aug 24, 2026

    @os-zhuang
    Contributor

    Triage: lands in ObjectMap (published renderer) → domain:ui, type Bug, pm:queue. Rationale: the marker useMemo declaring memoization that never happens — getMapConfig running unmemoized in the render body so mapConfig gets a fresh identity every render — is the component's own declared intent not being enforced; downstream consumers keying on that identity re-render or re-fire effects per render.

    ⚠️ Serial note: #5953 is in flight on ObjectMap (marker titles via getRecordDisplayName, pm:dispatched) — same file; hard-serialize behind it and re-measure the memo site on its merged ref. Adjacent finding #5977 (a test narrating a getMapConfig literal that no longer exists) is ungraded and likely folds into whichever PR touches this surface next — the claiming seat answers fold-or-serial when it gets graded.


    Generated by Claude Code

  4. self-assigned this
    on Aug 24, 2026
  5. yinlianghui commented on Aug 24, 2026

    @yinlianghui
    CollaboratorAuthor

    Claim: PM loop round 38 (refill slot)
    Session: session_01CSoz9uGhaaSgiq3hshtN7L
    Branch: claude/issue-5976-objectmap-config-memo
    Worktree: objectui-issue-5976
    Domain: domain:ui
    File surface: packages/plugin-map/src/ObjectMap.tsx + its __tests__ + a changeset
    Container & model: S, mode:subagent, model: opus
    Clause-②: no — an internal memoization defect in one renderer. No declaration moves, no published surface widens.
    Serial constraints cleared: packages/plugin-map/** — no in-flight claim. Concurrent siblings: #4730 (packages/i18n/src/locales/**), #5927 (packages/types/src/zod/**), #5998 (packages/plugin-detail/.../record-path.tsx), #5565 (apps/console + packages/app-shell). Disjoint by full path.

    裁决 — triage's grade, carried as-is

    Triage seat, 2026-08-24: domain:ui, type Bug, pm:queue. Verbatim rationale: "the marker useMemo declaring memoization that never happens — getMapConfig running unmemoized in the render body so mapConfig gets a fresh identity every render — is the component's own declared intent not being enforced; downstream consumers keying on that identity re-render or re-fire effects per render."

    ⚠️ The useMemo is not merely useless — it is actively misleading. A useMemo whose dependency has a fresh identity on every render recomputes every render while declaring that it does not. The next person reads the memo and believes the work is cached. So the fix is not "add a dep" — it is making the declared intent true, and making it stay true.

    ⛔ The serial fence is DISCHARGED — re-measure on the merged ref

    Triage's note said "#5953 is in flight on ObjectMap — same file; hard-serialize behind it and re-measure the memo site on its merged ref."

    #5953 has since merged (PR #5975). The fence is spent. But the instruction that came with it is not: re-measure the memo site on current main, because #5953 rewrote this exact region — it moved marker titles onto getRecordDisplayName(objectSchema, record, …), removed the || 'name' fallback from both getMapConfig branches, and added objectSchema to the memo deps. So the memo's dependency list has already changed under this card once. ⛔ Do not work from the card's quoted code; re-derive it and report the delta.

    ⛔ Do NOT fold #5977

    Triage floated that #5977 (a test narrating a getMapConfig literal that no longer exists) "likely folds into whichever PR touches this surface next — the claiming seat answers fold-or-serial when it gets graded."

    I answer: serial, not fold — because it cannot be claimed. I checked its labels: #5977 carries finding only, with no domain:* label. domain:* is triage's single-producer output, and an unlabelled card is not claimable by anyone, this seat included. ⛔ Do not touch it, do not close it, do not fold its subject in. If your work happens to falsify or repair what that test narrates, ⛔ say so in the report and leave the card alone — routing it is triage's, not ours.

    PM mechanism assumptions — measure, do not assume

    1. "Never memoizes" is a claim about identity, and identity is measurable. ⛔ Do not fix by inspection. Pin the defect first: render twice with unchanged inputs and assert the produced mapConfig (or the marker array derived from it) is toBe-identical across renders. That assertion must be red before your fix and green after — if it is green before, the card's premise has drifted and you ⛔ stop and report.
    2. The downstream consequence is the part worth protecting. Triage's stated harm is "downstream consumers keying on that identity re-render or re-fire effects per render." Find at least one such consumer and name it. If none exists, the defect is real but its blast radius is smaller than the card prices, and that is a reportable finding, not a reason to skip the fix.
    3. ⚠️ A useMemo can be defeated from either end. Either the memo's own dep array contains a fresh-identity value, or getMapConfig is called in the render body outside the memo entirely and the memo wraps something downstream of it. The card says the latter. Measure which, because the fixes differ: the first needs the upstream identity stabilised, the second needs the call moved inside. ⛔ Applying the wrong one produces a green-looking diff that changes nothing.
    4. ⛔ Do not stabilise an identity by widening a dep array with JSON.stringify or a deep-equality hook. That trades a per-render recompute for a per-render serialize and hides the problem behind a cost that does not show up in a render count. If the honest fix needs a structural change (lifting the config, deriving it from stable inputs), do that and say so.

    Verification

    Reverse-verify with the direction predicted before running: revert the fix and show the identity assertion goes red; prove the mutation on disk by grepping the injected text and separately the removed text (an editor's exit code proves nothing on a zero-hit anchor); restore under trap … EXIT INT TERM and confirm git diff HEAD --stat is empty afterwards.

    Counter-probe required: an assertion that the config identity does change when it genuinely should — a fixture that alters objectSchema or the map config inputs between renders and asserts a new identity. Without it, "identity is stable" is satisfiable by freezing a stale config forever, which is a worse bug than the one you are fixing and is invisible to the positive assertion alone.

    Gates by name with exit codes: pnpm --filter @object-ui/plugin-map type-check, root-form vitest over packages/plugin-map/src (⚠️ this repo's vitest guard refuses the --filter forms — use pnpm exec vitest run <paths> from the repo root), eslint on changed files with the delta against merge-base stated. ⚠️ Note the type-check script is spelled type-check, hyphenated, and echo its name so a zero-match silent pass cannot read as green.

    Changeset required (published renderer). Draft PR, Fixes #5976. ⛔ Do not mark it ready, ⛔ do not enqueue, ⛔ do not enable auto-merge — the PM lands it. Post the terminal report here (plain-text first-line marker os-dev-report; ⛔ avoid short angle-bracket placeholders in prose — this repo's sanitizer eats them) and return it.


    Generated by Claude Code

  6. yinlianghui commented on Aug 24, 2026

    @yinlianghui
    CollaboratorAuthor

    os-dev-report

    {
      "issue": 5976,
      "status": "done",
      "branch": "claude/issue-5976-objectmap-config-memo",
      "pr": "https://github.com/objectstack-ai/objectui/pull/6016",
      "premise_still_valid": true,
      "summary": "Re-measured on current main (BASE c0091b82b, post-#5953/PR#5975): the defect is intact — ObjectMap.tsx:594 still read `const mapConfig = getMapConfig(schema);` bare in the render body. Delta vs the card's quoted code, as instructed: marker titles now go through getRecordDisplayName(objectSchema, record, {titleField, fallback}), the `|| 'name'` fallback is gone from BOTH getMapConfig branches, and the marker memo's deps are now [data, mapConfig, objectSchema] rather than the card's [data, mapConfig] — none of which causes or worsens the defect. Failure mode MEASURED, not inferred: it is (b), the builder called OUTSIDE the memo; the memo's own dep list is sound, its input was not. Fixed as `useMemo(() => getMapConfig(schema), [schema])` — no JSON.stringify, no deep-equality hook. Per the PM's mid-task clarification I checked the honest option first and it holds: getMapConfig is a pure function of schema and reads nothing else, and the schema identity reaching this component is already stable — every re-render ObjectMap causes itself leaves the prop untouched by construction, and all three callers hand over a memoized node (SchemaRenderer's evaluatedSchema at SchemaRenderer.tsx:516; the gate's `mapped` useMemo in useElementDataSourceSchema; ListView's viewComponentSchema at ListView.tsx:2007). So no serialize was needed and none was shipped. Two further reasons it would have been the wrong trade beyond its per-render cost: JSON.stringify drops `undefined` values, and an ABSENT titleField is load-bearing here since #5953 — it must never compare equal to a present one. The sibling dataConfig at :590 keeps its stringify key and is untouched (not the same defect — it does memoize); filed separately as #6018.",
      "tests": "All gates run on the final commit 0f754a839 (branch head), worktree objectui-issue-5976.\n\nGATE `pnpm --filter @object-ui/plugin-map type-check` — EXIT 0. pnpm echoed the script, so this is not a zero-match silent pass: `> @object-ui/plugin-map@17.6.0 type-check` / `> tsc --noEmit && tsc -p tsconfig.test.json`. First run of this gate was EXIT 2 with `Cannot find module '@object-ui/types'` etc. — the unbuilt-dependency-closure trap, not a real failure; cleared by `pnpm --workspace-concurrency=2 --filter '@object-ui/plugin-map^...' build` (EXIT 0). Second run was EXIT 2 for a genuine reason: TS2550 `Property 'at' does not exist on type 'unknown[]'` x4 — Array.prototype.at is outside this package's configured lib target. Rewritten to index arithmetic; third run EXIT 0.\n\nGATE root-form vitest, `pnpm exec vitest run packages/plugin-map/src --maxWorkers=2` — EXIT 0, `Test Files 15 passed (15)` / `Tests 88 passed (88)`. (Root form, no --filter, per this repo's invocation guard.)\n\nGATE eslint, `pnpm exec eslint --no-inline-config --format json` on the changed files — EXIT 0. Delta against merge-base c0091b82b: ObjectMap.tsx is UNCHANGED at 0 errors / 15 warnings before and after, with an identical rule breakdown (no-explicit-any 12, react-hooks/exhaustive-deps 2, react-hooks/use-memo 1). Baseline measured by checking out the merge-base copy of that file under a restoring trap and linting it, not by assumption. New test file: 0 errors, 4 warnings, all @typescript-eslint/no-explicit-any on the react-map-gl mock props — the established idiom here, and fewer than its siblings carry (markerTitle 7, schemaDataShorthand 9). No new rule class.\n\nGATE `node scripts/check-changeset-presence.mjs` — EXIT 0: \"2 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)\". (objectui has no skip-changeset label mechanism, so a real changeset was written; nothing was labelled.)\n\nControl-byte self-scan over all three changed files plus the PR body: `grep -naP '[\\x00-\\x08\\x0b\\x0c\\x0e-\\x1f\\x7f]'` found nothing.\n\nDEFECT PINNED FIRST, on unmodified source. New file packages/plugin-map/src/ObjectMap.configMemo.test.tsx (5 tests) measures identity at the two module boundaries this component actually crosses: getRecordDisplayName call count (a direct read of how many times the marker memo evaluated — that is the memo the card names) and the initialViewState object handed to MapGL (tail of the cascade, so a toBe on it pins the whole chain markers -> filteredMarkers -> markerBounds -> initialViewState). Direction predicted BEFORE running: three stability pins red, two counter-probes green. Observed exactly that — `Tests 3 failed | 2 passed (5)`, EXIT 1. Verbatim from the identity failure: \"AssertionError: expected { bounds: [ ...(2) ], ...(1) } to be { bounds: [ ...(2) ], ...(1) } // Object.is equality / Received: serializes to the same string / Compared values have no visual difference.\" — deep-equal, identity-fresh, the defect's exact signature. After the fix all 5 pass.\n\nREVERSE-VERIFICATION, run from the COMMITTED state so the restore point is real. Direction predicted before running: reverting the memo returns the same 3-red/2-green split. Observed exactly: `Tests 3 failed | 2 passed (5)`, EXIT 1. Mutation proven on disk before the run, both directions separately — injected bare call grep -c = 1, removed memo line grep -c = 0 (an editor exit code proves nothing on a zero-hit anchor). Restore ran under `trap '...' EXIT INT TERM`, printed RESTORED, and `git diff HEAD --stat` was EMPTY afterwards with the memo line back (grep -c = 1). NO REBUILD WAS INVOLVED and none was needed: the root vitest config aliases @object-ui/* to packages/*/src (vitest.config.mts:257) and the mutated file is the test's own relative import ('./ObjectMap'), so nothing resolved through dist/ — stated explicitly because a dist-resolved ablation stays silently green. The same trap discipline was used for the eslint baseline measurement.\n\nCOUNTER-PROBES (required, and load-bearing — 'identity is stable' is also satisfiable by freezing a stale config forever, which is worse and invisible to the positive assertion alone). Both assert a NEW identity with a visible consequence, and both were green before the fix as well as after, which is what makes them a real control: (1) changing the declared titleField re-resolves titles and the new binding reaches the rendered popup (Harbour Depot -> HD-01); (2) changing the declared center/zoom rebuilds the camera to the newly declared position. A third stability pin uses a STATE-driven re-render (search typing) rather than a bare rerender, covering the dominant real-world case where the schema prop is unchanged by construction.",
      "open_questions": [],
      "out_of_scope_findings": [
        "filed as #6018 (finding, unassigned): ObjectMap's sibling dataConfig at :590 pays a JSON.stringify on every render to buy an identity that [schema] now demonstrably gives for free — same structural shape as this card, but NOT the same defect (it does memoize), and it guards a fetch effect with a real loop hazard in its history, so it needs its own measurement rather than a fold. The card records what I did and did not measure, including that I looked for a reachable JSON.stringify false-equal there and did not find one — recorded as 'not found', not 'cannot happen'.",
        "not filed, reported here only — PM assumption 2, answered by measurement: every consumer keying on the mapConfig identity is a DERIVED-VALUE memo, not an effect (filteredMarkers, clusteredData which calls clusterMarkers, markerBounds which calls computeMarkerBounds, selectedMarker, initialViewState). NO useEffect re-fired: neither of the component's two effects names mapConfig or markers. So triage's phrasing 'downstream consumers re-render or re-fire effects per render' is half right — the re-render/recompute cascade is real and is what the fix removes, but nothing re-fires an effect. The blast radius is narrower than the card prices it. Recorded as the dispatch asked, not as a reason to have skipped the fix.",
        "not filed, reported here only: no lint rule would have caught this and none would catch its regression. react-hooks/exhaustive-deps is active in this repo but only at WARN, and it says nothing at all about a builder called OUTSIDE a memo — the memo's own dep list was always exhaustive. The new identity test is the only thing making the declared intent STAY true, which is why it asserts identity rather than a render count.",
        "not filed, observation only: SchemaRenderer builds a fresh `{...evaluatedSchema, className}` (SchemaRenderer.tsx:1025) when a node declares responsive styles, which would hand every block a fresh schema identity on each SchemaRenderer render. It does not defeat this fix — ObjectMap's own state changes do not re-render its parent — but it is the one path on which a downstream [schema] memo in ANY block could be invalidated more often than necessary. Not measured further; out of scope for a plugin-map card, and it belongs to @object-ui/react."
      ]
    }

    ⛔ Left as the dispatch requires: PR #6016 is a draft, not marked ready, not enqueued, auto-merge not enabled. #5977 untouched — nothing in this work falsifies or repairs what it narrates (it concerns stale prose above an arm of ObjectMap.listViewMapConfigReach.test.tsx, a file this PR does not modify). CI was not waited on; gate status at hand-off is in_progress on GitHub's side, with the four local gates above already green on 0f754a839.


    Generated by Claude Code


    Generated by Claude Code

  7. yinlianghui commented on Aug 24, 2026

    @yinlianghui
    CollaboratorAuthor

    PM review — ACCEPT

    PR #6016, head 0f754a839, merge-base c0091b82b. Gate read at the usual mark.

    Re-derived myself

    The fix is one line and [schema] is genuinely the whole dependency — I checked rather than accepting it. getMapConfig(schema: MapConfigSource) at ObjectMap.tsx:345 reads schema.mapStyle, schema.map, schema.locationField, schema.latitudeField and hands schema to its three warn helpers. It closes over nothing else. So useMemo(() => getMapConfig(schema), [schema]) is exhaustive, not a shorthand.

    And the upstream stability claim holds where it matters. SchemaRenderer.tsx:516 is const evaluatedSchema = useMemo(...) — memoized, as reported.

    The mid-task clarification, and what you did with it

    I sent a correction because my own dispatch forbade JSON.stringify while the card pointed at the neighbouring dataConfig:590, which uses exactly that. I asked you to check the honest option first and to justify a serialize if you shipped one.

    ✅ You checked, it held, and no serialize was shipped. But the part worth crediting is that you did not stop at "identity is stable, so [schema] works" — you produced a reason the deep-compare key would have been wrong here specifically, not merely costly:

    JSON.stringify … 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.

    That is the strongest form of the argument and it is a this-file fact, not a general caution: #5953 removed the || 'name' fallback from both getMapConfig branches precisely so an absent titleField means something. A stringify key would have made { titleField: undefined } and {} compare equal — reintroducing, through a memo key, the collapse #5953 landed to remove. ✅ Finding that is better than obeying the instruction.

    The measurement

    • ✅ Failure mode measured, not inferred. My assumption 3 named two ways a useMemo can be defeated and warned that the wrong fix produces a green-looking diff that changes nothing. You measured it as (b) — the builder called outside the memo, the memo's own dep list sound — and fixed the input rather than the dep array.
    • ✅ The defect was pinned on unmodified source first, at two module boundaries that are real reads rather than proxies: getRecordDisplayName call count (a direct count of marker-memo evaluations) and the initialViewState object handed to MapGL (the tail of the cascade, so one toBe pins markers → filteredMarkers → markerBounds → initialViewState). Direction predicted before running — 3 red, 2 green — and observed exactly.
    • ✅ The failure text is the defect's signature, quoted rather than summarised: "Object.is equality / Received: serializes to the same string / Compared values have no visual difference." Deep-equal, identity-fresh. That single line is the whole card.
    • ✅ The counter-probes assert a new identity with a visible consequence — a changed titleField reaching the rendered popup (Harbour Depot → HD-01), a changed center/zoom rebuilding the camera — and were green before the fix as well as after. That is what makes them controls rather than decoration, and it is what stops "identity is stable" from being satisfiable by freezing a stale config forever.
    • ✅ The third stability pin uses a STATE-driven re-render (search typing), not a bare rerender(). That is the dominant real-world case and the one a bare rerender cannot represent, since a bare rerender is the test harness handing over a new prop — the opposite of the situation being fixed.
    • ✅ "No rebuild was involved and none was needed" is stated with its reason — the root vitest config aliases @object-ui/* to packages/*/src (vitest.config.mts:257) and the mutated file is the test's own relative import. Said explicitly because a dist-resolved ablation stays silently green, which is the failure this seat has been asking devs to rule out by name.
    • ✅ The TS2550 Property 'at' does not exist detour was reported, not buried. A genuine second red between two unbuilt-closure reds, correctly separated from them and fixed rather than worked around.

    The three findings, all correctly graded

    1. ✅ ObjectMap's dataConfig pays a JSON.stringify on every render to buy an identity that [schema] already gives for free #6018 filed for dataConfig:590. Right call, and the reasoning is right: it is the same shape but not the same defect — it does memoize — and it guards a fetch effect with a real loop hazard in its history. ✅ And the honest sentence is the one that matters: you looked for a reachable JSON.stringify false-equal there and recorded it as "not found", not "cannot happen". That distinction is the whole difference between a finding someone can act on and one that quietly closes a door.
    2. ✅ PM assumption 2 answered by measurement, and it corrected triage. Every consumer keying on the identity is a derived-value memo, not an effect — filteredMarkers, clusteredData, markerBounds, selectedMarker, initialViewState — and neither of the component's two effects names mapConfig or markers. So triage's "re-render or re-fire effects per render" is half right: the recompute cascade is real, nothing re-fires an effect, and the blast radius is narrower than the card prices it. ✅ Reported as an honest narrowing rather than used as a reason to have skipped the fix — which is exactly what the dispatch asked for and the harder half to get right.
    3. ✅ "No lint rule would have caught this and none would catch its regression." react-hooks/exhaustive-deps is warn-level here and, more to the point, says nothing about a builder called outside a memo — the dep list was always exhaustive. That is why the new test asserts identity rather than a render count, and saying so is what stops the next person from deleting it as redundant with the linter.

    The observation at SchemaRenderer.tsx:1025 — verified, and it is the right kind of caveat

    You flagged that SchemaRenderer builds a fresh { ...evaluatedSchema, className: mergedClassName } when a node declares responsive styles. I read it: it is conditional on scopeClass, so the fresh object is built only for a node with responsive styles, and then on every SchemaRenderer render.

    ✅ Your bound on it is correct and worth stating plainly for the record: that rebuild happens on the parent's render, and ObjectMap's own state changes do not re-render its parent — so for the case this card is about (search typing, selection, zoom, data landing) the memo holds. The caveat is that a node with responsive styles gets its [schema] memo invalidated whenever the parent re-renders for its own reasons. That is @object-ui/react's to own, it applies to any block with a [schema] memo, and it does not weaken this fix. ⛔ Correctly left unmeasured and out of scope; ✅ correctly not left unsaid.


    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

domain:uiobjectui ui stream: fix lands on the published library or apps — objectui execution seatpm:dispatched

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions