Improve atlas field panel styling#571
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 30 minutes and 50 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughFields hover/selection UI was refactored to accept multiple layer IDs, query and prioritize features across those layers, render the hover panel consistently (mounted but toggled invisible when inactive), and make the fields-selection list scrollable with updated styling and an added selection-outline layer. Changes
Sequence DiagramsequenceDiagram
participant User
participant Map
participant FieldsPanelHover as FieldsPanelHover<br/>(Multi-layer)
participant StyleEngine as StyleEngine
participant PanelUI as PanelUI
User->>Map: Hover / mousedown on map
Map->>FieldsPanelHover: map event (mousedown/move)
FieldsPanelHover->>FieldsPanelHover: Throttle / debounce handlers
FieldsPanelHover->>Map: queryRenderedFeatures for each layerId
Map-->>FieldsPanelHover: features per layer
FieldsPanelHover->>FieldsPanelHover: filter valid layers, sort by priority (last first)
FieldsPanelHover->>StyleEngine: request styles (fill/outline)
StyleEngine-->>FieldsPanelHover: return styles (e.g. fill-opacity:0, outline)
FieldsPanelHover->>PanelUI: update state with prioritized feature
PanelUI->>PanelUI: render mounted Card (toggle invisible if empty), update scroll attributes
PanelUI-->>User: show field details (scrollable)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fdm-app/app/components/blocks/atlas/atlas-panels.tsx`:
- Around line 139-163: The cleanup must clear the pending delayed timeout and
cancel any pending throttled invocation to avoid calling setPanel with stale
state: inside the returned cleanup for the map listeners, call
clearTimeout(delayedUpdateTimeout) (guarding for undefined) and call
throttledUpdatePanel.cancel() if that method exists (throttledUpdatePanel is the
function wrapping throttledUpdatePanelInner); ensure you also clear the timeout
and cancel after removing listeners so delayedUpdatePanel, throttledUpdatePanel,
throttledUpdatePanelInner and updatePanel cannot run post-unmount.
- Around line 29-31: The effect is thrashing because the inline props layer and
layerExclude change identity each render; normalize them to stable string keys
inside the component (e.g., const layerKey = Array.isArray(layer) ?
layer.join(',') : String(layer || ''); and similarly for layerExclude) and then
use those derived keys (layerKey, layerExcludeKey) in the useEffect dependency
array and when registering/unregistering map event listeners (the effect that
uses zoomLevelFields, layer and layerExclude). Ensure you replace direct uses of
layer/layerExclude in dependencies with the stable keys and use the original
arrays only where actual array semantics are required (not in dependencies).
- Around line 41-48: The event handler in atlas-panels.tsx reads evt.point
unguarded (used in map.queryRenderedFeatures(evt.point, ...)) for both the
hover/zoom and load cases; add a type guard like "if (!('point' in evt)) return"
before any access to evt.point to avoid passing undefined to
map.queryRenderedFeatures and using stale pointer data, and ensure you call
throttledUpdatePanelInner.cancel() in the component cleanup to cancel any
pending trailing invocation of the throttled function
(throttledUpdatePanelInner) to prevent updates after unmount.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b51e56cb-57d4-4c3c-8a41-ca4b6b82429b
📒 Files selected for processing (7)
.changeset/moody-berries-wave.mdfdm-app/app/components/blocks/atlas/atlas-panels.tsxfdm-app/app/components/blocks/atlas/atlas-styles.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.atlas.fields._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.field.new._index.tsxfdm-app/app/routes/farm.create.$b_id_farm.$calendar.atlas.tsxfdm-app/app/tailwind.css
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
fdm-app/app/components/blocks/atlas/atlas-panels.tsx (3)
465-478: Missing dependency array causes effect to re-run on every render.This
useEffecthas no dependency array, so the scroll listener is torn down and re-registered (and a freshsetTimeoutis scheduled) after every render — including every throttled panel update from the other effect. At minimum, gate bypanel(the state that drives DOM creation) so the listener is (re)attached only when the scroll container is actually (re)created.♻️ Suggested fix
useEffect(() => { const scrollElement = scrollRef.current const scrollContainerElement = scrollContainerRef.current if (!scrollElement || !scrollContainerElement) return const handler = () => { handleScroll(scrollElement, scrollContainerElement) } const timeout = setTimeout(handler, 100) scrollElement.addEventListener("scroll", handler, { passive: true }) return () => { scrollElement.removeEventListener("scroll", handler) clearTimeout(timeout) } - }) + }, [panel])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/atlas/atlas-panels.tsx` around lines 465 - 478, The effect attaching the scroll listener in useEffect is missing a dependency array and thus runs on every render; update the useEffect that references scrollRef, scrollContainerRef, and handleScroll so it only re-runs when the DOM-backed panel changes (e.g., include panel in the dependency array) and ensure the handler uses the current scrollRef/scrollContainerRef values; leave the cleanup (removeEventListener and clearTimeout) as-is so the listener is only torn down/re-attached when panel changes.
358-361:overflow-y-scrollalways reserves a scrollbar track.With
overflow-y-scrollthe scrollbar gutter is shown even when the list is short enough to fit without scrolling, which fights the new scroll-fade design. Preferoverflow-y-autoso the gutter (and the fade gradients gated bydata-scroll-start/data-scroll-end) only appear when there is actually overflow.♻️ Suggested fix
- <div - ref={scrollRef} - className="overflow-y-scroll" - > + <div + ref={scrollRef} + className="overflow-y-auto" + >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/atlas/atlas-panels.tsx` around lines 358 - 361, The container using ref scrollRef currently uses the Tailwind class "overflow-y-scroll" which forces a scrollbar gutter even when content fits; change that class to "overflow-y-auto" in the JSX for the div (the element with ref={scrollRef}) so the vertical scrollbar and the scroll-fade gradients governed by data-scroll-start/data-scroll-end only appear when content actually overflows, leaving the rest of the markup and scrollRef usage unchanged.
46-139: Fragile TDZ-dependent closure inmakePanel.
makePanelis declared inside theif (zoom > zoomLevelFields)block and closes over the outerconst features(line 59), yet it is called at line 54 beforefeaturesis declared. It currently works only because ternary short-circuiting avoids evaluatingfeatures[0]whenfeature/layerare undefined. A future refactor (e.g., always formatting the area, readingfeatures.length, or adding a log statement) would break this silently with a TDZReferenceError.Consider passing the needed feature in explicitly and decoupling
makePanelfrom the outerfeaturesclosure:♻️ Suggested refactor
- if ( - features && - features.length > 0 && - features[0].properties - ) { - setPanel( - makePanel({ - layer: features[0].layer.id, - feature: features[0], - }), - ) - } else { - setPanel(makePanel({})) - } - - function makePanel({ - layer, - feature, - }: { - layer?: string - feature?: MapGeoJSONFeature - }) { - const active = layer && feature - const name = feature - ? layer === "fieldsSaved" - ? features[0].properties.b_name - : features[0].properties.b_lu_name - : "Naam" + const top = features[0] + if (top?.properties) { + setPanel(makePanel(top.layer.id, top)) + } else { + setPanel(makePanel()) + } + + function makePanel( + layer?: string, + feature?: MapGeoJSONFeature, + ) { + const active = layer && feature + const name = feature + ? layer === "fieldsSaved" + ? feature.properties.b_name + : feature.properties.b_lu_name + : "Naam"…and use
feature.properties.b_areain the description instead offeatures[0].properties.b_area.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/atlas/atlas-panels.tsx` around lines 46 - 139, The inner makePanel function closes over the outer features array and is defined/used in a way that can hit TDZ; refactor makePanel (and its calls) so it does not reference the outer features variable: accept the specific feature and layer as explicit parameters (e.g., makePanel({ layer?, feature? }) should only read from the passed feature.properties), move or keep its declaration so it no longer relies on the outer features closure, and update all calls (both the empty-panel and features[0] cases) to pass either undefined or features[0] explicitly; ensure all accesses use the passed feature (e.g., feature.properties.b_area, feature.properties.b_name/b_lu_name) instead of features[0].properties.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fdm-app/app/components/blocks/atlas/atlas-panels.tsx`:
- Around line 166-180: The map 'load' handler is registered with
map.once("load", updatePanel) but not removed on early unmount; modify the
cleanup to ensure the 'load' listener is unregistered by calling map.off("load",
updatePanel) (or switch to map.on and then off) so updatePanel cannot run after
unmount; keep the existing removal of "mousemove", "mousedown", "zoom", the
clearTimeout(delayedUpdateTimeout), and throttledUpdatePanelInner.cancel()
intact.
---
Nitpick comments:
In `@fdm-app/app/components/blocks/atlas/atlas-panels.tsx`:
- Around line 465-478: The effect attaching the scroll listener in useEffect is
missing a dependency array and thus runs on every render; update the useEffect
that references scrollRef, scrollContainerRef, and handleScroll so it only
re-runs when the DOM-backed panel changes (e.g., include panel in the dependency
array) and ensure the handler uses the current scrollRef/scrollContainerRef
values; leave the cleanup (removeEventListener and clearTimeout) as-is so the
listener is only torn down/re-attached when panel changes.
- Around line 358-361: The container using ref scrollRef currently uses the
Tailwind class "overflow-y-scroll" which forces a scrollbar gutter even when
content fits; change that class to "overflow-y-auto" in the JSX for the div (the
element with ref={scrollRef}) so the vertical scrollbar and the scroll-fade
gradients governed by data-scroll-start/data-scroll-end only appear when content
actually overflows, leaving the rest of the markup and scrollRef usage
unchanged.
- Around line 46-139: The inner makePanel function closes over the outer
features array and is defined/used in a way that can hit TDZ; refactor makePanel
(and its calls) so it does not reference the outer features variable: accept the
specific feature and layer as explicit parameters (e.g., makePanel({ layer?,
feature? }) should only read from the passed feature.properties), move or keep
its declaration so it no longer relies on the outer features closure, and update
all calls (both the empty-panel and features[0] cases) to pass either undefined
or features[0] explicitly; ensure all accesses use the passed feature (e.g.,
feature.properties.b_area, feature.properties.b_name/b_lu_name) instead of
features[0].properties.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ee1ed706-fa44-4b0d-bc94-d89a8d1874e2
📒 Files selected for processing (1)
fdm-app/app/components/blocks/atlas/atlas-panels.tsx
2 participants
Bug fixes
too many different cultivations to display, ensuring that the "Sla geselecteerde percelen op" button is always reachable.
Enhancements
Summary by CodeRabbit
Bug Fixes
Style