Move map when the selected field changes#263
Conversation
🦋 Changeset detectedLatest commit: ca7f0f1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAdds imperative map fitting on viewState changes in two route components by introducing a Mapbox mapRef and useEffect to call fitBounds. Also adds a changeset entry for a patch release describing the new auto-centering behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant RC as React Component
participant VS as viewState
participant UE as useEffect
participant M as Mapbox Map (mapRef)
Note over RC: Initial render
RC->>VS: compute bounds & fitBoundsOptions
RC->>M: mount MapGL with ref
UE->>M: fitBounds(VS.bounds, VS.fitBoundsOptions)
M-->>RC: viewport adjusted
Note over RC: On field selection change
U->>RC: select field
RC->>VS: recompute bounds/options
UE->>M: fitBounds(updated bounds, options)
M-->>RC: viewport adjusted
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ Finishing touches
🧪 Generate unit tests
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## development #263 +/- ##
============================================
Coverage 92.92% 92.92%
============================================
Files 79 79
Lines 12926 12926
Branches 1266 1266
============================================
Hits 12012 12012
Misses 912 912
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
coderabbitai
Bot
added
branch:development
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
fdm-app/app/routes/farm.create.$b_id_farm.$calendar.fields.$b_id._index.tsx (1)
253-256: Prevent over-firing of fitBounds on unrelated renders.Depend on stable keys for bounds/options to avoid redundant refits.
- useEffect(() => { - mapRef.current?.fitBounds(viewState.bounds, viewState.fitBoundsOptions) - }, [viewState]) + useEffect(() => { + mapRef.current?.fitBounds(viewState.bounds, viewState.fitBoundsOptions) + }, [ + JSON.stringify(viewState.bounds), + JSON.stringify(viewState.fitBoundsOptions), + ])fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.atlas.tsx (1)
114-116: Stabilize effect triggers for map fitting.Avoid repeated fits from identity-changed
viewState.- useEffect(() => { - mapRef.current?.fitBounds(viewState.bounds, viewState.fitBoundsOptions) - }, [viewState]) + useEffect(() => { + mapRef.current?.fitBounds(viewState.bounds, viewState.fitBoundsOptions) + }, [ + JSON.stringify(viewState.bounds), + JSON.stringify(viewState.fitBoundsOptions), + ])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/quick-parents-grin.md(1 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.atlas.tsx(3 hunks)fdm-app/app/routes/farm.create.$b_id_farm.$calendar.fields.$b_id._index.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2024-11-25T14:42:26.660Z
Learnt from: SvenVw
PR: SvenVw/fdm#6
File: fdm-app/app/components/blocks/field-map.tsx:0-0
Timestamp: 2024-11-25T14:42:26.660Z
Learning: In `fdm-app/app/components/blocks/field-map.tsx`, explicit cleanup of Mapbox GL resources is not necessary, as `react-map-gl` handles it automatically upon component unmount, and `MapRef` does not have a `remove` method.
Applied to files:
fdm-app/app/routes/farm.create.$b_id_farm.$calendar.fields.$b_id._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.atlas.tsx
📚 Learning: 2025-01-09T16:03:37.764Z
Learnt from: SvenVw
PR: SvenVw/fdm#42
File: fdm-app/app/routes/farm/_b_id_farm/layout.tsx:46-95
Timestamp: 2025-01-09T16:03:37.764Z
Learning: A shared layout component `FarmLayoutBase` has been created in `components/custom/farm-layout-base.tsx` to maintain consistency across farm-related pages. The component handles farm selection dropdown, breadcrumb navigation, and provides a common layout structure.
Applied to files:
fdm-app/app/routes/farm.create.$b_id_farm.$calendar.fields.$b_id._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.atlas.tsx
📚 Learning: 2025-01-31T14:29:37.599Z
Learnt from: SvenVw
PR: SvenVw/fdm#67
File: fdm-app/app/components/custom/atlas/atlas.d.tsx:8-8
Timestamp: 2025-01-31T14:29:37.599Z
Learning: In the Atlas component's MapFieldsProps interface, mapStyle is intentionally restricted to "mapbox://styles/mapbox/satellite-streets-v12" as it's currently the only supported style option.
Applied to files:
fdm-app/app/routes/farm.create.$b_id_farm.$calendar.fields.$b_id._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.atlas.tsx
📚 Learning: 2025-08-11T12:24:32.200Z
Learnt from: SvenVw
PR: SvenVw/fdm#233
File: fdm-app/app/components/blocks/atlas-fields/cultivation-history.tsx:53-53
Timestamp: 2025-08-11T12:24:32.200Z
Learning: In `fdm-app/app/components/blocks/atlas-fields/cultivation-history.tsx`, the NMI API for cultivations guarantees that each year will be unique in the cultivation history data, so using `cultivation.year` as a React list key is safe and won't cause duplicate key warnings.
Applied to files:
fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.atlas.tsx
📚 Learning: 2025-01-31T15:06:35.764Z
Learnt from: SvenVw
PR: SvenVw/fdm#67
File: fdm-app/app/components/custom/atlas/atlas-sources.tsx:21-66
Timestamp: 2025-01-31T15:06:35.764Z
Learning: In react-map-gl components, when querying rendered features via map.queryRenderedFeatures(), the effect's dependency array must include any props that affect the map's rendered state (like source data) to ensure features are queried against the current map state.
Applied to files:
fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.atlas.tsx
| mapStyle={loaderData.mapboxStyle} | ||
| mapboxAccessToken={loaderData.mapboxToken} | ||
| interactiveLayerIds={[id]} | ||
| ref={mapRef} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guarantee first fit when the map is ready.
Call fitBounds on onLoad to cover the initial render under ClientOnly.
- ref={mapRef}
+ ref={mapRef}
+ onLoad={() =>
+ mapRef.current?.fitBounds(
+ viewState.bounds,
+ viewState.fitBoundsOptions
+ )
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ref={mapRef} | |
| ref={mapRef} | |
| onLoad={() => | |
| mapRef.current?.fitBounds( | |
| viewState.bounds, | |
| viewState.fitBoundsOptions | |
| ) | |
| } |
🤖 Prompt for AI Agents
In fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.atlas.tsx around
line 143, the map ref is set but the initial viewport may not be fit because the
map is rendered inside ClientOnly; add an onLoad handler (or pass an onLoad prop
to the map component) that, when the map instance is available, calls fitBounds
with the same bounds you use elsewhere (compute bounds from markers or stored
bbox) so the first render is correctly framed; ensure the handler uses
mapRef.current (or the provided instance) and only runs once when the map loads.
There was a problem hiding this comment.
Why isn't adding ...viewState is enough to get the map centered on the initially selected field?
There was a problem hiding this comment.
@BoraIneviNMI You're absolutely right! Spreading ...viewState should be sufficient to center the map on the initially selected field.
The getViewState(fields) function computes the appropriate initial positioning (center, zoom, bounds) from the field geometry, and when you spread these props onto the MapGL component with {...viewState}, it sets the initial viewport correctly.
My suggestion for the onLoad handler was based on a potential edge case where the map might not respect the initial bounds due to timing issues with the ClientOnly wrapper, but if ...viewState is working properly for initial positioning, then the onLoad handler would indeed be redundant.
The useEffect with fitBounds is primarily for handling dynamic updates when the viewState changes (like when switching between different fields), not for the initial render.
Are you experiencing any issues with the initial centering, or is the current implementation with ...viewState working as expected for your use case?
🧠 Learnings used
Learnt from: SvenVw
PR: SvenVw/fdm#6
File: fdm-app/app/components/blocks/field-map.tsx:0-0
Timestamp: 2024-11-25T14:42:26.660Z
Learning: In `fdm-app/app/components/blocks/field-map.tsx`, explicit cleanup of Mapbox GL resources is not necessary, as `react-map-gl` handles it automatically upon component unmount, and `MapRef` does not have a `remove` method.
Learnt from: SvenVw
PR: SvenVw/fdm#67
File: fdm-app/app/components/custom/atlas/atlas.d.tsx:8-8
Timestamp: 2025-01-31T14:29:37.599Z
Learning: In the Atlas component's MapFieldsProps interface, mapStyle is intentionally restricted to "mapbox://styles/mapbox/satellite-streets-v12" as it's currently the only supported style option.
Learnt from: SvenVw
PR: SvenVw/fdm#42
File: fdm-app/app/routes/farm/_b_id_farm/layout.tsx:46-95
Timestamp: 2025-01-09T16:03:37.764Z
Learning: The `FarmLayout` component in `components/custom/farm-layout.tsx` provides a reusable layout structure for farm-related pages, with support for farm selection dropdown, customizable breadcrumb titles, and flexible content rendering through either children or Outlet components.
Learnt from: SvenVw
PR: SvenVw/fdm#233
File: fdm-app/app/components/blocks/atlas-fields/cultivation-history.tsx:53-53
Timestamp: 2025-08-11T12:24:32.200Z
Learning: In `fdm-app/app/components/blocks/atlas-fields/cultivation-history.tsx`, the NMI API for cultivations guarantees that each year will be unique in the cultivation history data, so using `cultivation.year` as a React list key is safe and won't cause duplicate key warnings.
There was a problem hiding this comment.
No I wasn't experiencing any issues. Thanks for the suggestion though.
There was a problem hiding this comment.
@BoraIneviNMI Perfect! You're absolutely right - the ...viewState approach is clean and sufficient for your use case. Thanks for the clarification, and I'm glad the implementation is working smoothly! 🐰
(\ /)
( ._. )
o_(")(") *nods understandingly*
🧠 Learnings used
Learnt from: SvenVw
PR: SvenVw/fdm#6
File: fdm-app/app/components/blocks/field-map.tsx:0-0
Timestamp: 2024-11-25T14:42:26.660Z
Learning: In `fdm-app/app/components/blocks/field-map.tsx`, explicit cleanup of Mapbox GL resources is not necessary, as `react-map-gl` handles it automatically upon component unmount, and `MapRef` does not have a `remove` method.
Learnt from: SvenVw
PR: SvenVw/fdm#67
File: fdm-app/app/components/custom/atlas/atlas.d.tsx:8-8
Timestamp: 2025-01-31T14:29:37.599Z
Learning: In the Atlas component's MapFieldsProps interface, mapStyle is intentionally restricted to "mapbox://styles/mapbox/satellite-streets-v12" as it's currently the only supported style option.
Learnt from: SvenVw
PR: SvenVw/fdm#42
File: fdm-app/app/routes/farm/_b_id_farm/layout.tsx:46-95
Timestamp: 2025-01-09T16:03:37.764Z
Learning: The `FarmLayout` component in `components/custom/farm-layout.tsx` provides a reusable layout structure for farm-related pages, with support for farm selection dropdown, customizable breadcrumb titles, and flexible content rendering through either children or Outlet components.
Learnt from: SvenVw
PR: SvenVw/fdm#233
File: fdm-app/app/components/blocks/atlas-fields/cultivation-history.tsx:53-53
Timestamp: 2025-08-11T12:24:32.200Z
Learning: In `fdm-app/app/components/blocks/atlas-fields/cultivation-history.tsx`, the NMI API for cultivations guarantees that each year will be unique in the cultivation history data, so using `cultivation.year` as a React list key is safe and won't cause duplicate key warnings.
| mapStyle={loaderData.mapboxStyle} | ||
| mapboxAccessToken={loaderData.mapboxToken} | ||
| interactiveLayerIds={[id]} | ||
| ref={mapRef} |
There was a problem hiding this comment.
Ensure initial fit after map mounts (SSR-safe).
With the container gate, the first effect may run before the map exists. Trigger a fit on onLoad.
- ref={mapRef}
+ ref={mapRef}
+ onLoad={() =>
+ mapRef.current?.fitBounds(
+ viewState.bounds,
+ viewState.fitBoundsOptions
+ )
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ref={mapRef} | |
| ref={mapRef} | |
| onLoad={() => | |
| mapRef.current?.fitBounds( | |
| viewState.bounds, | |
| viewState.fitBoundsOptions | |
| ) | |
| } |
🤖 Prompt for AI Agents
In fdm-app/app/routes/farm.create.$b_id_farm.$calendar.fields.$b_id._index.tsx
around line 389 (ref={mapRef}), the initial fit can run before the map exists
due to the container gate; add an onLoad handler on the map component that runs
when the map instance is ready and performs the fit/fitBounds (checking
mapRef.current or using the map instance passed to onLoad), so the initial
viewport is applied only after the map mounts (SSR-safe) and remove/avoid
relying solely on the first effect to perform the initial fit.
SvenVw
left a comment
SvenVw
left a comment
There was a problem hiding this comment.
Thanks! I checked it and it works perfectly.
2 participants
Summary by CodeRabbit
New Features
Chores