feat(map): styleEpoch readiness and private useMapLayers - #69
Conversation
|
@km-git007 is attempting to deploy a commit to the mapcn oss program Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe map lifecycle now gates interaction readiness on map load and style readiness, exposes ChangesMap lifecycle and layers
Repository hygiene
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Map
participant subscribeStyleReady
participant MapLibre
participant useMapLayers
Map->>subscribeStyleReady: register style-ready callback
MapLibre-->>subscribeStyleReady: emit styledata and report isStyleLoaded()
subscribeStyleReady->>Map: invoke handleStyleReady
Map->>Map: increment styleEpoch
Map->>useMapLayers: render layers after readiness
useMapLayers->>MapLibre: add or recreate sources and layers
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Key declarative layers on styleEpoch so theme style swaps recreate sources through one shared lifecycle hook, not style.load/pendingStyle.
31ce67e to
457a040
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/registry/map.tsx`:
- Around line 309-310: Move the projectionRef.current assignment out of the
render path and synchronize it in a useEffect tied to projection, so
handleStyleReady only observes committed projection values.
- Around line 1604-1609: Update handleMouseMove so hover deduplication only runs
when feature.id is non-null; allow features without IDs to continue reaching
onHover on every mouse move, while preserving lastFeatureId tracking and
deduplication for valid IDs.
- Around line 1259-1274: Update the layer synchronization useEffect to clear
paint and layout properties that existed in the previous layer configuration but
are absent from the current layer.paint or layer.layout. Track the prior key
sets per layer, reset removed properties appropriately before applying current
values, and preserve the existing isLoaded, map, and layer guards.
- Around line 433-441: Update the style replacement flow around setStyle and
handleStyleReady to clear isStyleReady before invoking mapInstance.setStyle,
while preserving isLoaded. Restore isStyleReady from handleStyleReady once the
new style is ready so useMapLayers and other style-sensitive effects remain
paused during replacement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c3ad47b-11d5-4ec8-a22c-d9c4a906eb35
📒 Files selected for processing (5)
.gitignorepublic/r/map.jsonsrc/app/(main)/docs/advanced-usage/page.tsxsrc/app/(main)/docs/api-reference/page.tsxsrc/registry/map.tsx
Close the ready gate during style swaps, omit invalid route dasharrays, and keep shell listeners keyed to the same lifecycle revision.
Keep styleEpoch readiness and useMapLayers; bring in data-theme detection, public prop type exports, and homepage/style cleanups.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/(main)/docs/advanced-usage/page.tsx (1)
49-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEnsure the documentation example is valid TypeScript.
As per coding guidelines, "Write concise, technical TypeScript code with accurate examples". The
useMapCodesnippet has implicitanyparameters, an untyped prop, and raw JSX at the root which references an undefined variableparks. Adding basic type annotations and wrapping the usage in a default export component ensures the example is valid TypeScript and copy-paste friendly.♻️ Proposed refactor
- const handleClick = (e) => { + const handleClick = (e: any) => { console.log("Clicked at:", e.lngLat); }; map.on("click", handleClick); return () => map.off("click", handleClick); }, [map, isLoaded]); return null; } // Custom sources/layers must recreate when styleEpoch advances. -function CustomParkLayer({ data }) { +function CustomParkLayer({ data }: { data: GeoJSON.FeatureCollection }) { const { map, isLoaded, styleEpoch } = useMap(); useEffect(() => { if (!map || !isLoaded) return; map.addSource("parks", { type: "geojson", data }); map.addLayer({ id: "parks-fill", type: "fill", source: "parks", paint: { "fill-color": "`#22c55e`", "fill-opacity": 0.4 }, }); return () => { if (map.getLayer("parks-fill")) map.removeLayer("parks-fill"); if (map.getSource("parks")) map.removeSource("parks"); }; }, [map, isLoaded, styleEpoch, data]); return null; } // Usage -<Map center={[-74, 40.7]} zoom={10}> - <MapEventListener /> - <CustomParkLayer data={parks} /> -</Map>`; +export default function App() { + return ( + <Map center={[-74, 40.7]} zoom={10}> + <MapEventListener /> + <CustomParkLayer data={{ type: "FeatureCollection", features: [] }} /> + </Map> + ); +}`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/`(main)/docs/advanced-usage/page.tsx around lines 49 - 88, Update the useMapCode documentation example to use explicit TypeScript types for the click event, map-layer props, and data, then wrap the usage JSX in a default-exported component that defines or receives parks instead of referencing an undefined root variable. Keep the existing MapEventListener and CustomParkLayer behavior unchanged while making the snippet compile and remain copy-paste friendly.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/app/`(main)/docs/advanced-usage/page.tsx:
- Around line 49-88: Update the useMapCode documentation example to use explicit
TypeScript types for the click event, map-layer props, and data, then wrap the
usage JSX in a default-exported component that defines or receives parks instead
of referencing an undefined root variable. Keep the existing MapEventListener
and CustomParkLayer behavior unchanged while making the snippet compile and
remain copy-paste friendly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d6356e4e-d3bb-48e5-aef7-a5410d69d16d
📒 Files selected for processing (5)
.gitignorepublic/r/map.jsonsrc/app/(main)/docs/advanced-usage/page.tsxsrc/app/(main)/docs/api-reference/page.tsxsrc/registry/map.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/app/(main)/docs/api-reference/page.tsx
- public/r/map.json
- src/registry/map.tsx
Drop redundant lifecycle helpers and leaner layer create/sync paths without touching the proven style readiness owner.
|
Good nit. Typed the advanced-usage |
Make the styleEpoch docs snippet valid TypeScript so it is safer to copy.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/registry/map.tsx (1)
108-116: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFall back to the system theme when the explicit theme is removed.
When
classordata-themeis cleared,docThemebecomesnulland the previous theme remains stale.Proposed fix
const observer = new MutationObserver(() => { const docTheme = getDocumentTheme(); - if (docTheme) { - setDetectedTheme(docTheme); - } + setDetectedTheme(docTheme ?? getSystemTheme()); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/registry/map.tsx` around lines 108 - 116, Update the MutationObserver callback in the theme detection flow to handle a null result from getDocumentTheme(): setDetectedTheme to the system/default theme when class or data-theme is removed, while preserving the explicit docTheme path when present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@public/r/map.json`:
- Line 17: The Map provider currently mounts children whenever mapInstance
exists, before the combined readiness gate is satisfied. Update the children
render in Map to require isLoaded (hasLoadEvent && isStyleReady) alongside
mapInstance, so consumers mount only after map and style readiness and unmount
during style replacement.
- Line 17: The generated map artifact still contains hardcoded colored and RGBA
paint defaults. Update the source symbols DEFAULT_ARC_PAINT,
GEOJSON_DEFAULT_COLORS, DEFAULT_CLUSTER_COLORS, pointColor defaults, and
blankMapStyle to use semantic grayscale CSS theme tokens from globals.css via
MapLibre-compatible color values, then regenerate public/r/map.json so its
embedded content matches the compliant source.
- Line 17: Update the hover-state lifecycle in useMapLayers so hoveredId cannot
persist across GeoJSON source rebuilds triggered by MapGeoJSON changes such as
promoteId or topology updates. Include the source-rebuild dependency (for
example, topologyKey) in the hover effect’s dependency handling and clear any
active feature state during cleanup before the replacement source is created.
In `@src/registry/map.tsx`:
- Line 1530: The event-handler refs are mutated during render and can expose
values from discarded renders. In src/registry/map.tsx:1530, move the
latestRef.current update for { onClick, onHover } into an effect; likewise, in
src/registry/map.tsx:1807, move the corresponding { data, onClick, onHover }
update into an effect so both refs synchronize only after commit.
---
Outside diff comments:
In `@src/registry/map.tsx`:
- Around line 108-116: Update the MutationObserver callback in the theme
detection flow to handle a null result from getDocumentTheme(): setDetectedTheme
to the system/default theme when class or data-theme is removed, while
preserving the explicit docTheme path when present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f55c2457-8234-47da-a71b-cefafe682665
📒 Files selected for processing (3)
public/r/map.jsonsrc/app/(main)/docs/advanced-usage/page.tsxsrc/registry/map.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/(main)/docs/advanced-usage/page.tsx
Mount map children only when style-ready, sync handler refs after commit, and fall back to system theme when the document theme clears.
Bind GeoJSON/Arc handlers from effect closures like MapRoute so we do not need a render-time ref write or a syncing useEffect.
Bring in control transition and animation timing tweaks from main.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@public/r/map.json`:
- Line 17: Update mergeHoverPaint so every hoverPaint override is represented by
a feature-state case expression, including keys absent from the base paint; use
a valid non-hover fallback for those keys rather than assigning hoverValue
directly. Ensure fillHoverPaint and MapArc hoverPaint properties such as
fill-opacity remain unchanged for non-hovered features.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a487e5a-9f26-4049-a540-48e41d1304ce
📒 Files selected for processing (2)
public/r/map.jsonsrc/registry/map.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/registry/map.tsx
Always wrap hoverPaint overrides in a case expression so keys absent from the base paint do not apply to every feature.
|
Hey @AnmolSaini16, when you get a chance could you take a look at this PR? It covers the styleEpoch / theme-swap layer work we talked about earlier (the full end state after #68). Happy to walk through anything or tweak based on feedback. |
|
Added one narrow follow-up after the resolved review: hover props now expose only MapLibre paint keys that support feature-state, the heuristic fallback is gone, and less-common hover keys require an explicit base paint value. Existing styleEpoch/useMapLayers work and history are unchanged. Registry, touched lint, TypeScript, build, MapLibre style validation, and browser hover/theme checks pass. |
1 participant
Goal
Make theme/style swaps reliable for declarative map layers.
One readiness owner (
styleEpoch) plus one private layer lifecycle (useMapLayers), so Route / GeoJSON / Arc / Cluster recreate on the new style instead of relying onstyle.load+pendingStyleremount tricks or copy-pasted MapLibre source/layer code.Why this PR exists
This work was originally planned as three smaller PRs that together would reach the same end state.
PR #68 was the first slice (readiness /
styleEpochonly). It did not land as intended: after miscommunication, the timeout issue was treated as fixed by the upstreamstyle.load/pendingStylepath (35e328a), so thestyleEpochfoundation never shipped.This PR ships the full intended end state in one place so the architecture is reviewable as a whole.
What changed
subscribeStyleReadyowns style-ready. Context exposesstyleEpoch(bumps once per ready style).isLoadedis the safe-to-touch gate and closes during in-flightsetStyle.useMapLayersowns GeoJSON source/layer create-teardown,setData, and paint/layout sync. Shells keep transforms + semantic events only.styleEpochadvances.setStyleuses{ diff: false }(with an inline note) after{ diff: true }hung without a reliable post-armstyledata.public/r/map.jsonregenerated.src/registry/map.tsxis smaller than currentmain(2175 vs 2225).Out of scope
Exporting
useMapLayers, MapHeatmap, Marker/Controls refactors, popup module, nav/block catalog work.Test plan
/docs/routes,/docs/geojson,/docs/arcs,/docs/clusters(layers return; no missing source/layer errors)tsc --noEmit+ lint on touched filespublic/r/map.jsonhasstyleEpoch; nopendingStyle/ permanentstyle.loadreadiness ownerwc -l src/registry/map.tsxbelowmainSummary by CodeRabbit
Bug Fixes
styleEpochreadiness signal for safer layer setup.Documentation
useMapdocs and examples to includestyleEpochand explain when to use it.Chores
.gitignoreto ignore local git worktrees (.worktrees/).