Skip to content

ObjectMap: two pieces of state stop tracking their source after mount — the data prop is absent from the fetch effect's deps, and the clustering zoom seed never sees the initial camera #5003

Description

@yinlianghui

Observation-class, found while implementing #4941. Filed unassigned, not claiming. Neither item is reachable through the console path today (see "Why this is dormant"), which is why it is a finding rather than a bug report.

1. props.data is read by the fetch effect but is not one of its dependencies

packages/plugin-map/src/ObjectMap.tsx — the effect starts by preferring records passed as a prop:

if ((rest as any).data) { ... setData(passed); setLoading(false); return; }

but its dependency list is [dataConfig, dataSource, hasInlineData, schema.filter, schema.sort, objectSchema]. rest.data is not in it (eslint already reports the pair as react-hooks/exhaustive-deps warnings on this effect, for rest and schema).

A host that renders the map with data={[]} while its own query is in flight and then passes the rows would keep showing the empty map: the prop changes, the effect does not re-run, and the copy in state stays empty.

2. The clustering zoom seed never sees the initial camera

const [currentZoom, setCurrentZoom] = useState(mapConfig.zoom || 3) seeds the value that clusterMarkers uses for its grid cell size, and onZoom is the only thing that updates it. The initial camera is applied by MapLibre's constructor (via initialViewState), before react-map-gl attaches the React handlers, so no onZoom fires for it — the seed stays nominal until the user zooms for the first time.

This was equally true before #4941 (the seed was then the synthesized 10); the fix did not introduce it, and it now starts from 3, which is closer to a fitted continental camera than 10 was. Recorded so it is not rediscovered as a regression of that PR.

Why this is dormant

  • The console path never passes data as a prop; it goes through ElementDataSourceGate and the component's own dataSource.find.
  • Every fetch sets loading back to true, which unmounts MapGL and remounts it when the data lands, so a data change through the effect is always reflected (that is also why the camera fit in plugin-map: no fitBounds to data — markers land on an adjacent world copy, initial viewport shows an empty map #4941 needs no imperative fitBounds call).
  • Clustering only engages above 100 markers (or an explicit opt-in), so a stale seed has no visible effect on the small sets in the examples.

If picked up

Item 1 wants the prop threaded properly rather than added blindly to the deps array — rest is a fresh object every render, so naming rest would refetch on every render. The narrow fix is to destructure the prop (data: dataProp) and depend on that one value.

Activity

  1. os-support-ai commented on Aug 19, 2026

    @os-support-ai
    Collaborator

    Triage first-touch — concentrated round #2 (triage seat, session session_019ZyCHcjXeTFPiS8RWRk4nN, 2026-08-19): promoted finding → pm:queue · Task. Dormant on the console path but real for any embedding host passing data as a prop (rows arriving after mount are never shown), and the fix shape is specified: destructure data: dataProp and depend on that one value (⛔ not rest — refetch-every-render trap the body names); seed the clustering zoom from the applied initial camera or first onZoom. Both items one PR in packages/plugin-map/src/ObjectMap.tsx, with tests for the prop-update path. Serial constraint: #5020 edits the same file (doc comment) — claim whichever lands first and merge main before the second; note the overlap in the claim comment. Size/model suggestion: S–M, sonnet.


    Generated by Claude Code

  2. os-support-ai commented on Aug 19, 2026

    @os-support-ai
    Collaborator

    Claim: PM loop round 13

    Session: session_01RV6yuVCxymHYE16PL9vQkE
    Branch: claude/issue-5003-objectmap-prop-and-zoom-seed
    Worktree: objectui-issue-5003
    Domain: repo:objectui
    File surface: packages/plugin-map/src/ObjectMap.tsx + its tests (stop on breach; explain in the report)
    Container & model: S–M, mode:subagent, model: sonnet (taking the triage seat's size/model suggestion)
    Clause-②: no — neither item changes what the contract accepts or rejects, and nothing on the public surface widens. Item 1 makes an existing declared prop actually tracked after mount; item 2 corrects an internal seed. No schema key gains or loses acceptance.
    Serial constraints cleared: #5020 is the one card triage named as editing this same file (its clusterRadius doc comment) — it is pm:queue, unassigned, not in flight, so there is no live hold and this claim takes the file first. Open PRs hold disjoint surfaces, each measured by git diff --name-only origin/main...: PR #5290 → apps/console/**; PR #5292 → eslint-rules/** + eslint.config.js; PR #5294 → packages/plugin-view/**. No in-flight card is known to touch packages/plugin-map; if one turns out to, stop and report.

    Scope — both items, one PR, Fixes #5003

    Triage promoted this finding → pm:queue · Task on 2026-08-19 and specified the fix shape. Carried through:

    Item 1 — props.data is read by the fetch effect but is not one of its dependencies. ⛔ Do not add rest to the deps array: rest is a fresh object every render, so naming it converts the bug into a refetch-every-render trap. The narrow fix the card names is to destructure the prop (data: dataProp) and depend on that one value. The eslint react-hooks/exhaustive-deps warnings already reported on this effect (for rest and schema) are the signal, not the specification — satisfying the linter is not the goal, threading the prop correctly is.

    Item 2 — the clustering zoom seed never sees the initial camera. useState(mapConfig.zoom || 3) seeds what clusterMarkers uses for grid cell size, and onZoom is the only writer. MapLibre's constructor applies the initial camera via initialViewState before react-map-gl attaches React handlers, so no onZoom ever fires for it. Seed from the applied initial camera (or the first onZoom).

    Verification the report must carry

    The card documents why this is dormant, and that is exactly what makes it easy to "fix" without evidence:

    • the console path never passes data as a prop (it goes through ElementDataSourceGate and the component's own dataSource.find);
    • every fetch sets loading back to true, which unmounts and remounts MapGL, so a data change through the effect is always reflected;
    • clustering only engages above 100 markers (or explicit opt-in).

    So a test that renders through the console path will pass before and after — it proves nothing. Test the prop-update path directly: mount with data={[]}, then pass rows, and assert the rows are shown. Reverse-verify: predict which assertions go red with the fix reverted, name them before running, and report predicted-vs-observed. If an assertion is green on both legs, it is not pinning the fix — say so rather than counting it.

    Item 2's own note: this was equally true before #4941 (the seed was then the synthesized 10); that PR did not introduce it and the current 3 is closer to a fitted continental camera than 10 was. Recorded so it is not re-reported as a regression of #4941.


    Generated by Claude Code

  3. os-support-ai commented on Aug 19, 2026

    @os-support-ai
    Collaborator
    {
      "issue": 5003,
      "status": "done",
      "branch": "claude/issue-5003-objectmap-prop-and-zoom-seed",
      "pr": "https://github.com/objectstack-ai/objectui/pull/5297",
      "premise_still_valid": true,
      "summary": "Fixed both dormant state-tracking bugs in ObjectMap: (1) declared `data` as its own prop (`data: dataProp`) instead of reading it off the untyped `rest` spread, and added that single value to the fetch effect's deps — the whole `rest` object is deliberately excluded to avoid a refetch-every-render trap; (2) added an `onLoad` handler that seeds `currentZoom` from the actually-applied initial camera (MapLibre resolves `initialViewState`, including a bounds fit, before react-map-gl attaches `onZoom`), so clustering at first paint no longer relies on a nominal `mapConfig.zoom || 3` seed. Also documented the newly-declared `data` prop in the package README (it was previously reachable but undocumented).",
      "tests": "New files ObjectMap.dataProp.test.tsx and ObjectMap.zoomSeed.test.tsx, mounting outside the console/schema path per the card's warning (console path never passes `data` as a prop; clustering needs an explicit opt-in or >100 markers to engage). `pnpm --filter @object-ui/plugin-map run type-check` (tsc --noEmit + tsconfig.test.json) clean; `pnpm exec vitest run packages/plugin-map --maxWorkers=2` = 11 files / 65 passed. Both run at final head d031db984. Reverse verification: committed the fix (32bef3bd0), predicted before running that `dataProp`'s 'picks up rows passed as a prop AFTER mount' and `zoomSeed`'s 'starts clustered under the nominal seed, then splits once the applied camera is read' would go red on pre-fix ObjectMap.tsx (checked out from origin/main, tests kept) and the other two new tests would stay green; ran the suite and observed exactly that (2 failed | 63 passed, the two predicted names in the failure list). Restored the fix from the branch tip (`git checkout <branch> -- ObjectMap.tsx`, clean diff against HEAD) and reran: 65 passed (65). The two non-discriminating tests (dataProp test 2, zoomSeed test 2) are called out as such in the PR body rather than counted as pins — both were green on both legs. No build artifact sits between edit and test: the test files import `./ObjectMap` by relative path within the package, and the root vitest.config.mts also aliases `@object-ui/plugin-map` to `packages/plugin-map/src`. Also ran at the same head: check-changeset-presence.mjs, check-changeset-no-major.mjs (patch, not major), check-control-bytes.mjs, and eslint on the touched files (0 errors; the pre-existing react-hooks/exhaustive-deps warning for `rest` on this effect is gone, leaving only the pre-existing, out-of-scope `schema.data` one — see out_of_scope_findings).",
      "open_questions": [],
      "out_of_scope_findings": [
        "not filed: the same fetch effect also reads `schema.data` directly (a separate short-circuit checking whether `schema.data` itself is an array, distinct from the `ViewData`-shaped `schema.data` `getDataConfig` normally expects) without it in the dependency array either -- eslint's react-hooks/exhaustive-deps flags it (`missing dependency: 'schema.data'`) the same way it flagged `rest` before this fix. Left alone: it's a different code path from what #5003 named (props.data, not schema.data), the 'correct' fix isn't pinned by an authoritative source the way item 1's was (unclear whether the array-shaped-schema.data branch is reachable at all given `ObjectMapSchema.data: ViewData`, i.e. it may be dead code, in which case the right fix is deletion, not a new dep), and the file-surface/gate-family bounded-fix exemption's condition 2 (correct shape already pinned) isn't clearly met. Could not search open issues for a duplicate or file a new one before writing this report: `search_issues` and `list_issues` both returned 'API rate limit already exceeded for user ID 318092878' on every attempt (the shared bot identity) -- PM should file this once the org-wide GitHub API rate limit clears, or confirm no filing is warranted after inspecting whether the array branch is reachable."
      ]
    }

    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

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions