Atlas: Add "Bodemanalyses" layer#607
Open
BoraIneviNMI wants to merge 26 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughIntroduces a new Bodemanalyses (soil-analyses) atlas layer that visualizes farm fields on an interactive map coloured by soil parameter values. Users can select different parameters to re-colour the map client-side, click fields to navigate to a detailed page, and review current values and historical analyses. Includes shading infrastructure, legend UI, clickable map interactions, three new detail routes with permission-aware rendering, and updates to header/sidebar navigation. ChangesBodemanalyses atlas
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
fdm-app/app/components/blocks/soil/list.tsx (1)
51-59:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
format()will throw whenb_sampling_dateisnull.
b_sampling_datewas widened toDate | nullintypes.d.ts, but line 54 passes it directly todate-fnsformat()without a null guard. The only existing guard is thea_source === "nl-other-nmi"check — any analysis with a non-NMI source and anullsampling date will crash at runtime.🐛 Proposed fix
- {analysis.a_source === "nl-other-nmi" - ? "Geschat met NMI BodemSchat" - : format( - analysis.b_sampling_date, - "PP", - { - locale: nl, - }, - )} + {analysis.a_source === "nl-other-nmi" + ? "Geschat met NMI BodemSchat" + : analysis.b_sampling_date + ? format( + analysis.b_sampling_date, + "PP", + { + locale: nl, + }, + ) + : "Onbekende datum"}🤖 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 `@fdm-app/app/components/blocks/soil/list.tsx` around lines 51 - 59, The code calls date-fns format() with analysis.b_sampling_date which is now Date | null; add a null guard before calling format in the JSX inside components/blocks/soil/list.tsx so that when analysis.a_source !== "nl-other-nmi" and b_sampling_date is null you render a safe fallback (e.g., a localized "no date" string or empty placeholder) instead of passing null to format; update the branch that currently returns format(analysis.b_sampling_date, "PP", { locale: nl }) to check analysis.b_sampling_date first and only call format when it's non-null.fdm-app/app/components/blocks/soil/cards.tsx (1)
198-262:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
canModifymay beundefinedat runtime due to record access.When
canModifyAllSoilAnalysesisfalseandcanModifySoilAnalysis[card.a_id]is missing (the default{}provides no entries), the expressioncanModifyAllSoilAnalyses || canModifySoilAnalysis[card.a_id]evaluates toundefined, which is then passed toSoilDataCardwhose prop type declarescanModify: boolean. TypeScript only catches this withnoUncheckedIndexedAccess. Coerce explicitly to keep types and runtime aligned.🛡️ Proposed fix
canModify={ - canModifyAllSoilAnalyses || - canModifySoilAnalysis[card.a_id] + canModifyAllSoilAnalyses || + canModifySoilAnalysis[card.a_id] === true }🤖 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 `@fdm-app/app/components/blocks/soil/cards.tsx` around lines 198 - 262, The canModify prop can be undefined because canModifySoilAnalysis[card.a_id] may be missing; update the computed prop passed to SoilDataCard (the expression using canModifyAllSoilAnalyses and canModifySoilAnalysis[card.a_id]) to always produce a boolean (e.g. coerce with !! or fall back to false like canModifyAllSoilAnalyses || !!canModifySoilAnalysis[card.a_id] or (canModifyAllSoilAnalyses || canModifySoilAnalysis[card.a_id] === true)), ensuring the value is a boolean when passed to SoilDataCard and referencing the same symbols: canModifyAllSoilAnalyses, canModifySoilAnalysis, card.a_id, and SoilDataCard.
🧹 Nitpick comments (8)
fdm-app/app/store/selected-soil-parameter.ts (1)
16-17: 💤 Low valueSimplify the setter.
The setter wraps
setin a redundant lambda; it can be expressed more concisely:♻️ Proposed refactor
- setSelectedParameter: (selectedParameter) => - set({ selectedParameter: selectedParameter }), + setSelectedParameter: (selectedParameter) => + set({ selectedParameter }),🤖 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 `@fdm-app/app/store/selected-soil-parameter.ts` around lines 16 - 17, The setSelectedParameter setter currently wraps set in an unnecessary arrow that reassigns the same name; simplify it by passing the payload directly to set so setSelectedParameter uses the incoming selectedParameter to update state without the redundant lambda—locate setSelectedParameter in selected-soil-parameter.ts and replace the current implementation that calls set({ selectedParameter: selectedParameter }) inside an extra arrow with a direct call that assigns the parameter to selectedParameter.fdm-app/app/components/blocks/atlas/atlas-sources.tsx (1)
353-358: ⚡ Quick winUse a layer-scoped click listener for consistency and precision.
FieldSourceClickableregisters a globalmap.on("click", clickOnMap)handler, then usesqueryRenderedFeaturesto detect hits. The existingFieldsSourceAvailablecomponent (line 204) uses the layer-scopedmap.on("click", id, handleClick)form, which fires only when a feature in that layer is clicked and is more precise. Keeping both patterns alive in the same file adds maintenance burden.♻️ Proposed refactor
- useEffect(() => { - function clickOnMap(evt: MapLayerMouseEvent) { - if (!map) return - - if ( - excludedLayerId && - map.queryRenderedFeatures(evt.point, { - layers: [excludedLayerId], - }).length - ) { - return - } - - const features = map.queryRenderedFeatures(evt.point, { - layers: [id], - }) - - if (features.length > 0 && features[0].layer.id === id) { - onFieldClick(features[0] as unknown as Feature<Geometry, Field>) - } - } - - if (map) { - map.on("click", clickOnMap) - return () => { - map.off("click", clickOnMap) - } - } - }, [map, id, excludedLayerId, onFieldClick]) + useEffect(() => { + function clickOnMap(evt: MapLayerMouseEvent) { + if (!map) return + if ( + excludedLayerId && + map.queryRenderedFeatures(evt.point, { + layers: [excludedLayerId], + }).length + ) { + return + } + if (evt.features && evt.features.length > 0) { + onFieldClick(evt.features[0] as unknown as Feature<Geometry, Field>) + } + } + + if (map) { + map.on("click", id, clickOnMap) + return () => { + map.off("click", id, clickOnMap) + } + } + }, [map, id, excludedLayerId, onFieldClick])🤖 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 `@fdm-app/app/components/blocks/atlas/atlas-sources.tsx` around lines 353 - 358, FieldSourceClickable currently attaches a global click handler via map.on("click", clickOnMap) and then filters hits with queryRenderedFeatures; update it to use a layer-scoped listener like map.on("click", layerId, clickOnMap) (the same pattern used by FieldsSourceAvailable and its handleClick) so clicks fire only for features in that layer; ensure you pass the correct layer id used when adding the source/layer, change the cleanup to map.off("click", layerId, clickOnMap), and remove the queryRenderedFeatures-based hit-filtering or adapt it to rely on the layer-scoped event payload.fdm-app/app/components/blocks/atlas/atlas-panels.tsx (1)
104-137: 💤 Low value
makePaneluses outerfeaturesarray but is defined inside the mouse-event handler — ensure stale-closure is not an issue.
makePanelat line 104 closes overfeatures(line 66) andlayerExclude/clickRedirectsToDetailsPage. This is fine within a singleupdatePanelinvocation, but the entireuseEffectis re-created only when dependencies change. Becauserenderis now in the deps array and is a function reference, any inlinerenderpassed from the parent will cause the effect and its throttled wrappers to be torn down and rebuilt on every render. This is a correctness risk (dropped events during throttle teardown) for callers that don't memoizerender.Consider documenting that
rendershould be a stable reference (e.g. wrapped inuseCallbackat the call site).🤖 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 `@fdm-app/app/components/blocks/atlas/atlas-panels.tsx` around lines 104 - 137, makePanel closes over variables like features, layerExclude, clickRedirectsToDetailsPage and the render function causing effect teardown when an inline render prop changes; stabilize render to avoid dropped events by moving render into a ref and referencing that ref inside makePanel (create renderRef = useRef(render), update renderRef.current in a small effect when render changes, and use renderRef.current(feature) inside makePanel) or alternatively document/require that the parent passes a memoized render (useCallback). Update references to render in makePanel and in updatePanel/useEffect to use the ref so the throttled handlers aren’t torn down on every render.fdm-app/app/routes/farm.$b_id_farm.$calendar.atlas.soil-analysis.$b_id.soil.analysis.$a_id.tsx (2)
72-93: 💤 Low valueParallelize independent fetches.
getFieldandgetSoilAnalysisdon't depend on each other; running them sequentially adds avoidable latency on this detail page.♻️ Proposed refactor
- // Get details of field - const field = await getField(fdm, session.principal_id, b_id) - if (!field) { - throw data("Field is not found", { - status: 404, - statusText: "Field is not found", - }) - } - - // Get the soil analyses - const soilAnalysis = await getSoilAnalysis( - fdm, - session.principal_id, - a_id, - ) + const [field, soilAnalysis] = await Promise.all([ + getField(fdm, session.principal_id, b_id), + getSoilAnalysis(fdm, session.principal_id, a_id), + ]) + if (!field) { + throw data("Field is not found", { + status: 404, + statusText: "Field is not found", + }) + }🤖 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 `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.atlas.soil-analysis.$b_id.soil.analysis.$a_id.tsx around lines 72 - 93, getField and getSoilAnalysis are called sequentially causing extra latency; run them concurrently using Promise.all (or Promise.allSettled) so both fetches start together, then check results and throw the same 404 data errors if either returned null. Specifically, call getField(fdm, session.principal_id, b_id) and getSoilAnalysis(fdm, session.principal_id, a_id) in parallel, await the combined promise, then preserve the existing null checks/throws for field and soilAnalysis to retain the same error messages.
21-36: 💤 Low valueStale JSDoc on the loader.
The block comment is copy-pasted from another loader and references "soil analyses, current soil data" — none of which this loader returns. It also misses
a_idin theparamsdescription and the 404 thrown for missing soil analysis. Worth a quick sync to avoid misleading future readers.🤖 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 `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.atlas.soil-analysis.$b_id.soil.analysis.$a_id.tsx around lines 21 - 36, The JSDoc for the loader is stale: update the comment above the loader function to accurately list the actual return values and include the missing route param `a_id` (in addition to `b_id_farm` and `b_id`), and describe the precise errors thrown (400 when farm or field ID is missing and 404 when the requested soil analysis is not found); locate the comment immediately above the loader function named "loader" in this file and replace the copy-pasted description about soil analyses/current soil data with a short, accurate summary of what this loader actually returns and the exact error conditions it throws.fdm-app/app/components/blocks/header/atlas.tsx (1)
23-32: 💤 Low valueLGTM, but be aware of substring overlap in
isSoil.
isSoilistruefor any/soil-analysisURL because the substring/soilis contained in/soil-analysis. The ternary handles this correctly becauseisSoilAnalysisis checked first, but ifisSoilis later used standalone elsewhere in this file (or extended), it will also match the soil-analysis route. Consider tightening the check (e.g., a regex or path segment check) for future-proofing.🤖 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 `@fdm-app/app/components/blocks/header/atlas.tsx` around lines 23 - 32, The current checks compute isElevation, isSoilAnalysis, isSoil and derive currentName, but isSoil uses pathname.includes("/soil") which also matches "/soil-analysis"; update the isSoil test to ensure it only matches the "soil" path segment (for example by testing the path segments or using a regex like match for "/soil" followed by end or "/") so isSoil no longer returns true for "/soil-analysis"; adjust the derived currentName logic if needed to keep the existing prioritization of isElevation and isSoilAnalysis.fdm-app/app/routes/farm.$b_id_farm.$calendar.atlas.soil-analysis.$b_id.soil._index.tsx (2)
132-132: 💤 Low valueStray trailing whitespace artifact.
The
{" "}after the closing</div>is a leftover JSX text node that adds an unintended space between siblings on flex layouts. Safe to remove.🤖 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 `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.atlas.soil-analysis.$b_id.soil._index.tsx at line 132, Remove the stray JSX text node {" "} that follows the closing </div> (the leftover trailing whitespace artifact) so it no longer inserts an unintended space between sibling elements in the JSX render; locate the JSX fragment containing </div>{" "} in the soil analysis component and delete the {" "} token leaving only the intended tags.
75-92: 💤 Low valueSequential awaits in loader add latency.
getSoilAnalysesandgetCurrentSoilDataare independent and can run in parallel viaPromise.all, reducing load time on this read-heavy page (especially with a per-field detail navigation pattern).♻️ Proposed refactor
- // Get the soil analyses - const soilAnalyses = await getSoilAnalyses( - fdm, - session.principal_id, - b_id, - { - start: null, - end: timeframe.end, - }, - ) - - // Get current soil data - const currentSoilData = await getCurrentSoilData( - fdm, - session.principal_id, - b_id, - timeframe, - ) + // Fetch analyses and current soil data in parallel + const [soilAnalyses, currentSoilData] = await Promise.all([ + getSoilAnalyses(fdm, session.principal_id, b_id, { + start: null, + end: timeframe.end, + }), + getCurrentSoilData(fdm, session.principal_id, b_id, timeframe), + ])🤖 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 `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.atlas.soil-analysis.$b_id.soil._index.tsx around lines 75 - 92, The loader currently awaits getSoilAnalyses(...) and then getCurrentSoilData(...) sequentially, adding unnecessary latency; run them in parallel by invoking both calls without awaiting and use Promise.all to await both results together, e.g., call getSoilAnalyses(fdm, session.principal_id, b_id, { start: null, end: timeframe.end }) and getCurrentSoData(fdm, session.principal_id, b_id, timeframe) concurrently and destructure the returned array into soilAnalyses and currentSoilData so the rest of the loader uses those values.
🤖 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 `@fdm-app/app/components/blocks/atlas/atlas-legend.tsx`:
- Around line 218-231: The table rows generated by displayedOptions are rendered
as <tr> children of <table>, causing validateDOMNesting warnings and potential
SSR hydration mismatches; update the JSX in the atlas-legend component (where
displayedOptions.map renders <tr key={opt.value}>) to wrap those rows inside a
<tbody> element (i.e., move the map output into a <tbody> so
<table><tbody>{displayedOptions.map(...<tr>...)}</tbody></table>), ensuring keys
remain on each <tr>.
In `@fdm-app/app/components/blocks/atlas/atlas-panels.tsx`:
- Around line 32-39: The prop type for render is declared required but callers
and the runtime check treat it as optional; update the prop declaration to make
render optional by changing its type from render: (feature: GeoJSONFeature) =>
ReactNode to render?: (feature: GeoJSONFeature) => ReactNode in the component
props (the destructured props block that includes zoomLevelFields, layer,
layerExclude, render, clickRedirectsToDetailsPage) so TypeScript matches the
runtime fallback logic and existing call sites.
In `@fdm-app/app/components/blocks/atlas/atlas-soil-analysis.tsx`:
- Around line 354-361: The warning in getShadingParameterMapper fires for any
non-gradient parameter even when no custom mapper exists; change the condition
to only warn when the parameter is not in GRADIENT_SHADED_SOIL_PARAMETERS AND it
actually has a custom mapper in SHADING_PARAMETER_MAPPERS. In other words,
update the if in getShadingParameterMapper to check both !(parameter in
GRADIENT_SHADED_SOIL_PARAMETERS) && (parameter in SHADING_PARAMETER_MAPPERS)
before calling console.warn, leaving the return of
SHADING_PARAMETER_MAPPERS[parameter] ?? DEFAULT_PARAMETER_MAPPER unchanged; this
prevents spurious logs when getShadingParameterMapper is called from
getSoilAnalysisLayerStyle for enum-shaded parameters.
- Around line 118-154: There is a typo in the GRADIENT_SHADED_SOIL_PARAMETERS
map: the calcium carbonate key is misspelled as "a_cao3_if" and should be
"a_caco3_if"; update the key in the GRADIENT_SHADED_SOIL_PARAMETERS constant so
it matches the canonical parameter name used elsewhere (so lookups in
getSoilAnalysisLayerStyle and schema integration will correctly find
"a_caco3_if" and use the gradient shading).
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.atlas.soil-analysis._index.tsx:
- Around line 359-363: The stroke Layer (id={heatmapStrokeLayerId}) is missing
the layout visibility toggle so turning off showFields only hides the fill
layer; update the stroke Layer to include the same layout prop used for the fill
layer (e.g., layerLayout or visibility based on showFields) so its visibility
follows the showFields toggle, ensuring the stroke and fill layers use the same
visibility logic.
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.atlas.soil-analysis.$b_id.soil.analysis.$a_id.tsx:
- Around line 122-130: The page header text contradicts the read-only rendering
of the form: when you render <SoilAnalysisForm editable={false}> the copy
"Bekijk en bewerk de gegevens van deze bodemanalyse" is misleading; update the
heading/description to view-only wording (e.g., "Bekijk de gegevens van deze
bodemanalyse" or similar) in the component that renders the title/description
near the <SoilAnalysisForm> so the UI copy matches the <SoilAnalysisForm
editable={false}> prop.
- Around line 96-98: The current filter for soilParameterDescription uses a
truthy check "(item) => soilAnalysis[item.parameter]" which drops valid
zero/empty/false values; change the predicate to a presence check so parameters
are included if they exist on the soilAnalysis object (e.g., use "item.parameter
in soilAnalysis" or Object.prototype.hasOwnProperty.call(soilAnalysis,
item.parameter) or check !== undefined) when building soilParameterDescription
returned from getSoilParametersDescription(), preserving values like 0, "" or
false for rendering.
---
Outside diff comments:
In `@fdm-app/app/components/blocks/soil/cards.tsx`:
- Around line 198-262: The canModify prop can be undefined because
canModifySoilAnalysis[card.a_id] may be missing; update the computed prop passed
to SoilDataCard (the expression using canModifyAllSoilAnalyses and
canModifySoilAnalysis[card.a_id]) to always produce a boolean (e.g. coerce with
!! or fall back to false like canModifyAllSoilAnalyses ||
!!canModifySoilAnalysis[card.a_id] or (canModifyAllSoilAnalyses ||
canModifySoilAnalysis[card.a_id] === true)), ensuring the value is a boolean
when passed to SoilDataCard and referencing the same symbols:
canModifyAllSoilAnalyses, canModifySoilAnalysis, card.a_id, and SoilDataCard.
In `@fdm-app/app/components/blocks/soil/list.tsx`:
- Around line 51-59: The code calls date-fns format() with
analysis.b_sampling_date which is now Date | null; add a null guard before
calling format in the JSX inside components/blocks/soil/list.tsx so that when
analysis.a_source !== "nl-other-nmi" and b_sampling_date is null you render a
safe fallback (e.g., a localized "no date" string or empty placeholder) instead
of passing null to format; update the branch that currently returns
format(analysis.b_sampling_date, "PP", { locale: nl }) to check
analysis.b_sampling_date first and only call format when it's non-null.
---
Nitpick comments:
In `@fdm-app/app/components/blocks/atlas/atlas-panels.tsx`:
- Around line 104-137: makePanel closes over variables like features,
layerExclude, clickRedirectsToDetailsPage and the render function causing effect
teardown when an inline render prop changes; stabilize render to avoid dropped
events by moving render into a ref and referencing that ref inside makePanel
(create renderRef = useRef(render), update renderRef.current in a small effect
when render changes, and use renderRef.current(feature) inside makePanel) or
alternatively document/require that the parent passes a memoized render
(useCallback). Update references to render in makePanel and in
updatePanel/useEffect to use the ref so the throttled handlers aren’t torn down
on every render.
In `@fdm-app/app/components/blocks/atlas/atlas-sources.tsx`:
- Around line 353-358: FieldSourceClickable currently attaches a global click
handler via map.on("click", clickOnMap) and then filters hits with
queryRenderedFeatures; update it to use a layer-scoped listener like
map.on("click", layerId, clickOnMap) (the same pattern used by
FieldsSourceAvailable and its handleClick) so clicks fire only for features in
that layer; ensure you pass the correct layer id used when adding the
source/layer, change the cleanup to map.off("click", layerId, clickOnMap), and
remove the queryRenderedFeatures-based hit-filtering or adapt it to rely on the
layer-scoped event payload.
In `@fdm-app/app/components/blocks/header/atlas.tsx`:
- Around line 23-32: The current checks compute isElevation, isSoilAnalysis,
isSoil and derive currentName, but isSoil uses pathname.includes("/soil") which
also matches "/soil-analysis"; update the isSoil test to ensure it only matches
the "soil" path segment (for example by testing the path segments or using a
regex like match for "/soil" followed by end or "/") so isSoil no longer returns
true for "/soil-analysis"; adjust the derived currentName logic if needed to
keep the existing prioritization of isElevation and isSoilAnalysis.
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.atlas.soil-analysis.$b_id.soil._index.tsx:
- Line 132: Remove the stray JSX text node {" "} that follows the closing </div>
(the leftover trailing whitespace artifact) so it no longer inserts an
unintended space between sibling elements in the JSX render; locate the JSX
fragment containing </div>{" "} in the soil analysis component and delete the {"
"} token leaving only the intended tags.
- Around line 75-92: The loader currently awaits getSoilAnalyses(...) and then
getCurrentSoilData(...) sequentially, adding unnecessary latency; run them in
parallel by invoking both calls without awaiting and use Promise.all to await
both results together, e.g., call getSoilAnalyses(fdm, session.principal_id,
b_id, { start: null, end: timeframe.end }) and getCurrentSoData(fdm,
session.principal_id, b_id, timeframe) concurrently and destructure the returned
array into soilAnalyses and currentSoilData so the rest of the loader uses those
values.
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.atlas.soil-analysis.$b_id.soil.analysis.$a_id.tsx:
- Around line 72-93: getField and getSoilAnalysis are called sequentially
causing extra latency; run them concurrently using Promise.all (or
Promise.allSettled) so both fetches start together, then check results and throw
the same 404 data errors if either returned null. Specifically, call
getField(fdm, session.principal_id, b_id) and getSoilAnalysis(fdm,
session.principal_id, a_id) in parallel, await the combined promise, then
preserve the existing null checks/throws for field and soilAnalysis to retain
the same error messages.
- Around line 21-36: The JSDoc for the loader is stale: update the comment above
the loader function to accurately list the actual return values and include the
missing route param `a_id` (in addition to `b_id_farm` and `b_id`), and describe
the precise errors thrown (400 when farm or field ID is missing and 404 when the
requested soil analysis is not found); locate the comment immediately above the
loader function named "loader" in this file and replace the copy-pasted
description about soil analyses/current soil data with a short, accurate summary
of what this loader actually returns and the exact error conditions it throws.
In `@fdm-app/app/store/selected-soil-parameter.ts`:
- Around line 16-17: The setSelectedParameter setter currently wraps set in an
unnecessary arrow that reassigns the same name; simplify it by passing the
payload directly to set so setSelectedParameter uses the incoming
selectedParameter to update state without the redundant lambda—locate
setSelectedParameter in selected-soil-parameter.ts and replace the current
implementation that calls set({ selectedParameter: selectedParameter }) inside
an extra arrow with a direct call that assigns the parameter to
selectedParameter.
🪄 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: 60b5674c-c4cb-4ffe-a1ac-52f4fdfa2edc
📒 Files selected for processing (18)
.changeset/orange-yaks-refuse.md.changeset/ready-clowns-pick.mdfdm-app/app/components/blocks/atlas/atlas-legend.tsxfdm-app/app/components/blocks/atlas/atlas-panels.tsxfdm-app/app/components/blocks/atlas/atlas-soil-analysis.tsxfdm-app/app/components/blocks/atlas/atlas-sources.tsxfdm-app/app/components/blocks/atlas/atlas-styles.tsxfdm-app/app/components/blocks/fields-new/soil.tsxfdm-app/app/components/blocks/header/atlas.tsxfdm-app/app/components/blocks/sidebar/apps.tsxfdm-app/app/components/blocks/soil/cards.tsxfdm-app/app/components/blocks/soil/list.tsxfdm-app/app/components/blocks/soil/types.d.tsfdm-app/app/routes/farm.$b_id_farm.$calendar.atlas.soil-analysis.$b_id.soil._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.atlas.soil-analysis.$b_id.soil.analysis.$a_id.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.atlas.soil-analysis._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.soil._index.tsxfdm-app/app/store/selected-soil-parameter.ts
Contributor
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 `@fdm-app/app/components/blocks/atlas/atlas-legend.tsx`:
- Around line 190-195: The "Geen data op hele bedrijf" banner is shown when
fieldsData exists but contains an empty FeatureCollection; update the JSX
conditional in atlas-legend.tsx so the warning only renders when there are
actual parcels — i.e., require fieldsData.features?.length > 0 in addition to
!anyDataAvailable (or alternatively render a different empty-state message when
features.length === 0); locate the conditional that uses anyDataAvailable and
fieldsData and add the features length guard (fieldsData.features?.length > 0)
or branch for the empty-feature case.
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.atlas.soil-analysis._index.tsx:
- Around line 174-218: selectedParameter is read from
useSelectedAtlasSoilParameterStore and may be stale; before any shading-related
lookup (calls to getShadingParameterMapper, getSoilAnalysisLayerStyle, or using
shadingConfig[selectedParameter]) validate it against the known keys (e.g., the
list derived from soilParametersDescriptions or a whitelist) and if invalid
replace it with the safe default "a_som_loi" for the current render and also
call the store setter (setSelectedParameter) to repair the persisted value so
future rehydration is fixed; do this normalization once near where
selectedParameter is read (before computing min/max and before calling
getShadingParameterMapper/getSoilAnalysisLayerStyle).
- Around line 47-55: The route's exported metadata (export const meta:
MetaFunction) still uses a generic title/description for the parcel atlas;
update the title to something like `Bodemanalyses - Atlas |
${clientConfig.name}` and replace the description content with a short, accurate
blurb about soil analyses (e.g., viewing and exploring bodemanalyses results on
the atlas) so browser tabs and previews reflect "Bodemanalyses" instead of the
generic parcel text.
- Around line 221-249: The saved viewport key "mapViewState" is too global;
change both the read in the viewState initializer and the write in the useEffect
to use a scoped key that includes b_id_farm, calendar and the atlas layer
identifier (e.g., build a key like
`mapViewState:${b_id_farm}:${calendar}:${layer}`), keep using the same composed
key variable in the useState initializer (where sessionStorage.getItem is
called) and in the useEffect (where sessionStorage.setItem is called), and fall
back to initialViewState as before when no scoped entry exists or storage access
fails.
🪄 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: 00a831ff-448c-472a-ac39-ba45d2badf19
📒 Files selected for processing (9)
fdm-app/app/components/blocks/atlas/atlas-legend.tsxfdm-app/app/components/blocks/atlas/atlas-panels.tsxfdm-app/app/components/blocks/atlas/atlas-soil-analysis.tsxfdm-app/app/components/blocks/atlas/atlas-sources.tsxfdm-app/app/components/blocks/soil/cards.tsxfdm-app/app/components/blocks/soil/list.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.atlas.soil-analysis.$b_id.soil._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.atlas.soil-analysis.$b_id.soil.analysis.$a_id.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.atlas.soil-analysis._index.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- fdm-app/app/components/blocks/soil/list.tsx
- fdm-app/app/routes/farm.$b_id_farm.$calendar.atlas.soil-analysis.$b_id.soil.analysis.$a_id.tsx
- fdm-app/app/routes/farm.$b_id_farm.$calendar.atlas.soil-analysis.$b_id.soil._index.tsx
- fdm-app/app/components/blocks/atlas/atlas-panels.tsx
- fdm-app/app/components/blocks/soil/cards.tsx
- fdm-app/app/components/blocks/atlas/atlas-soil-analysis.tsx
Comment on lines
+221
to
+249
| const [viewState, setViewState] = useState<ViewState>(() => { | ||
| if (typeof window !== "undefined") { | ||
| try { | ||
| const savedViewState = sessionStorage.getItem("mapViewState") | ||
| if (savedViewState) { | ||
| return JSON.parse(savedViewState) | ||
| } | ||
| } catch { | ||
| // ignore storage errors (e.g., private mode) | ||
| } | ||
| } | ||
| return initialViewState as ViewState | ||
| }) | ||
|
|
||
| const [showFields, setShowFields] = useState(true) | ||
|
|
||
| const onViewportChange = useCallback((event: ViewStateChangeEvent) => { | ||
| setViewState(event.viewState) | ||
| }, []) | ||
|
|
||
| const mapRef = useRef<MapRef>(null) | ||
|
|
||
| useEffect(() => { | ||
| if (typeof window !== "undefined") { | ||
| try { | ||
| sessionStorage.setItem( | ||
| "mapViewState", | ||
| JSON.stringify(viewState), | ||
| ) |
Contributor
There was a problem hiding this comment.
Scope the saved viewport key to this farm/layer.
"mapViewState" is global, so panning one atlas page affects the initial viewport of every other farm/layer that reuses the same key. That can reopen this route centered on an unrelated location instead of the current farm. Include at least b_id_farm, calendar, and the atlas layer in the storage key.
💡 Minimal fix
+ const mapViewStateKey = `mapViewState:${b_id_farm}:${calendar}:soil-analysis`
+
const [viewState, setViewState] = useState<ViewState>(() => {
if (typeof window !== "undefined") {
try {
- const savedViewState = sessionStorage.getItem("mapViewState")
+ const savedViewState = sessionStorage.getItem(mapViewStateKey)
if (savedViewState) {
return JSON.parse(savedViewState)
}
} catch {
// ignore storage errors (e.g., private mode)
@@
useEffect(() => {
if (typeof window !== "undefined") {
try {
sessionStorage.setItem(
- "mapViewState",
+ mapViewStateKey,
JSON.stringify(viewState),
)
} catch {
// ignore storage errors (e.g., private mode)
}
}
- }, [viewState])
+ }, [mapViewStateKey, viewState])🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 248-248: Clear text storage of sensitive information
This stores sensitive data returned by an access to longitude as clear text.
This stores sensitive data returned by an access to latitude as clear text.
This stores sensitive data returned by an access to longitude as clear text.
This stores sensitive data returned by an access to latitude as clear text.
This stores sensitive data returned by an access to longitude as clear text.
This stores sensitive data returned by an access to latitude as clear text.
This stores sensitive data returned by an access to longitude as clear text.
This stores sensitive data returned by an access to latitude as clear text.
This stores sensitive data returned by an access to longitude as clear text.
This stores sensitive data returned by an access to latitude as clear text.
This stores sensitive data returned by an access to longitude as clear text.
This stores sensitive data returned by an access to latitude as clear text.
This stores sensitive data returned by an access to longitude as clear text.
This stores sensitive data returned by an access to latitude as clear text.
This stores sensitive data returned by an access to longitude as clear text.
This stores sensitive data returned by an access to latitude as clear text.
🤖 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 `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.atlas.soil-analysis._index.tsx
around lines 221 - 249, The saved viewport key "mapViewState" is too global;
change both the read in the viewState initializer and the write in the useEffect
to use a scoped key that includes b_id_farm, calendar and the atlas layer
identifier (e.g., build a key like
`mapViewState:${b_id_farm}:${calendar}:${layer}`), keep using the same composed
key variable in the useState initializer (where sessionStorage.getItem is
called) and in the useEffect (where sessionStorage.setItem is called), and fall
back to initialViewState as before when no scoped entry exists or storage access
fails.
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
fdm-app/app/store/selected-soil-parameter.ts (1)
3-6: 🏗️ Heavy liftDecouple store state from the component module boundary.
The store depends on
~/components/blocks/atlas/atlas-soil-analysisfor parameter metadata. Moving these constants/types (including the default key) into a shared non-UI module will reduce coupling and prevent component↔store dependency tangles.♻️ Suggested direction (store-side diff)
import { create } from "zustand" import { createJSONStorage, persist } from "zustand/middleware" import { + DEFAULT_SHADED_SOIL_PARAMETER, getShadedSoilParameters, type ShadedSoilParameters, -} from "~/components/blocks/atlas/atlas-soil-analysis" +} from "~/domain/soil-parameters" import { ssrSafeSessionJSONStorage } from "./storage" @@ - selectedParameter: "a_som_loi", + selectedParameter: DEFAULT_SHADED_SOIL_PARAMETER, @@ - state.setSelectedParameter("a_som_loi") + state.setSelectedParameter( + DEFAULT_SHADED_SOIL_PARAMETER, + )Also applies to: 18-39
🤖 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 `@fdm-app/app/store/selected-soil-parameter.ts` around lines 3 - 6, The store currently imports getShadedSoilParameters and ShadedSoilParameters from the UI component module which couples state to components; move the parameter metadata and the default parameter key out of the UI module into a shared non-UI module (e.g., a new module exporting the parameters, the ShadedSoilParameters type, and the default key), update selected-soil-parameter.ts to import from that new shared module instead of ~/components/blocks/atlas/atlas-soil-analysis, and ensure any usages of getShadedSoilParameters, ShadedSoilParameters, and the default key in the store are updated to reference the new exports.fdm-app/app/routes/farm.$b_id_farm.$calendar.atlas.soil-analysis._index.tsx (1)
275-293: ⚡ Quick winConsider adding defensive parsing for date values in case non-ISO formats are introduced.
valueis typed asstring | number(fromfeature.properties), butformatDate()expects a Date-compatible input. Whiledate-fnsdoes accept ISO strings directly, it will throw aRangeErrorif the value is a non-ISO date string or unparseable. Adding a safety check would prevent runtime errors:Suggested fix
) : parameterDescription?.type === "date" ? ( <p> - {formatDate(value, "PP", { - locale: nl, - })} + {(() => { + const d = new Date(value as string | number) + return Number.isNaN(d.getTime()) + ? "Geen data" + : formatDate(d, "PP", { locale: nl }) + })()}🤖 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 `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.atlas.soil-analysis._index.tsx around lines 275 - 293, When parameterDescription?.type === "date", defensively parse the value before passing it to formatDate to avoid RangeError on non-ISO strings: in the date branch (where you currently call formatDate(value,...)) detect if value is a number (treat as timestamp) or if it's a string try parseISO(value) (or new Date(value) within a try/catch), confirm the resulting Date is valid (isNaN(date.getTime()) === false) and only then call formatDate(date,...); if parsing fails, render a safe fallback (e.g., the original value or "Geen data") so formatDate isn't invoked with an invalid input. Apply this change in the component handling parameterDescription?.type, value, selectedParameter and SHADED_SOIL_TYPES.
🤖 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 `@fdm-app/app/components/blocks/atlas/atlas-legend.tsx`:
- Around line 251-257: The code force-casts props.min/props.max to numbers
causing NaN when they are undefined; update the initialization of min/max (where
you currently have "let min = props.min as number" / "let max = props.max as
number") to provide explicit fallbacks (e.g., use gradDef.range or a safe
default like 0 and 1) before any centering math, and only perform the centering
adjustment when both min and max are valid numbers and gradDef.center is a
number; ensure domain={[min, max]} always receives finite numeric values.
- Around line 182-189: The code crashes because shadingConfig[selectedParameter]
may be undefined; update the atlas-legend render to guard the lookup by deriving
a safeParam = shadingConfig[selectedParameter] ? selectedParameter :
<defaultKey> (or compute a fallback via useSelectedAtlasSoilParameterStore
normalization) and use safeParam when checking shading; then render
EnumSoilAnalysisLegend or GradientSoilAnalysisLegend based on
shadingConfig[safeParam].shading (or provide a default shading) so the component
never reads .shading from undefined. Ensure the fix references shadingConfig and
selectedParameter and keep EnumSoilAnalysisLegend/GradientSoilAnalysisLegend
usage unchanged except for passing the safe param.
---
Nitpick comments:
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.atlas.soil-analysis._index.tsx:
- Around line 275-293: When parameterDescription?.type === "date", defensively
parse the value before passing it to formatDate to avoid RangeError on non-ISO
strings: in the date branch (where you currently call formatDate(value,...))
detect if value is a number (treat as timestamp) or if it's a string try
parseISO(value) (or new Date(value) within a try/catch), confirm the resulting
Date is valid (isNaN(date.getTime()) === false) and only then call
formatDate(date,...); if parsing fails, render a safe fallback (e.g., the
original value or "Geen data") so formatDate isn't invoked with an invalid
input. Apply this change in the component handling parameterDescription?.type,
value, selectedParameter and SHADED_SOIL_TYPES.
In `@fdm-app/app/store/selected-soil-parameter.ts`:
- Around line 3-6: The store currently imports getShadedSoilParameters and
ShadedSoilParameters from the UI component module which couples state to
components; move the parameter metadata and the default parameter key out of the
UI module into a shared non-UI module (e.g., a new module exporting the
parameters, the ShadedSoilParameters type, and the default key), update
selected-soil-parameter.ts to import from that new shared module instead of
~/components/blocks/atlas/atlas-soil-analysis, and ensure any usages of
getShadedSoilParameters, ShadedSoilParameters, and the default key in the store
are updated to reference the new exports.
🪄 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: 30de9252-fd56-4037-92fc-6b3dcf4adf57
📒 Files selected for processing (3)
fdm-app/app/components/blocks/atlas/atlas-legend.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.atlas.soil-analysis._index.tsxfdm-app/app/store/selected-soil-parameter.ts
Contributor
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 `@fdm-app/app/components/blocks/atlas/atlas-legend.tsx`:
- Around line 153-156: The anyDataAvailable check treats properties with null as
valid because it uses the "in" operator; update the logic in the
anyDataAvailable computation (and the similar check around the other block using
selectedParameter) to verify that feature.properties[selectedParameter] is not
null or undefined (e.g., use != null or typeof !== "undefined" and !== null) in
addition to checking key presence, so only real values count as available data;
locate the anyDataAvailable variable and the matching conditional that currently
uses "selectedParameter in feature.properties" and replace that predicate with
an existence-and-not-null check on feature.properties[selectedParameter].
🪄 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: 0414eed4-d8b1-4228-a227-41c8d97174c6
📒 Files selected for processing (1)
fdm-app/app/components/blocks/atlas/atlas-legend.tsx
Collaborator
|
Nice, but could you make the legend vertical instead of horizontal? Also for consistency, could you use the same floating action button for go back to map as at gewaspercelen? |
Collaborator
Author
I am working on it right now. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
fdm-app/app/components/blocks/atlas/atlas-legend.tsx (3)
134-136: 💤 Low value
shadingConfigis reconstructed on every render — consider memoizing.
getShadedSoilParameters()is called andObject.fromEntriesruns fresh on every render. For a legend that re-renders on hover/parameter changes, this is cheap but unnecessary.♻️ Proposed refactor
- // Parameter shading config - const shadingConfig = Object.fromEntries( - getShadedSoilParameters().map((item) => [item.parameter, item]), - ) + // Parameter shading config + const shadingConfig = useMemo( + () => + Object.fromEntries( + getShadedSoilParameters().map((item) => [item.parameter, item]), + ), + [], + )🤖 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 `@fdm-app/app/components/blocks/atlas/atlas-legend.tsx` around lines 134 - 136, shadingConfig is being rebuilt on every render by calling getShadedSoilParameters() and Object.fromEntries each time; memoize this computation (e.g., using React.useMemo) so shadingConfig is only recomputed when its inputs change. Locate the shadingConfig creation in atlas-legend.tsx (the call to getShadedSoilParameters() and Object.fromEntries) and wrap it in a memo tied to the relevant dependency (such as any props/state or the getShadedSoilParameters function itself) so renders from hover/parameter UI changes don’t rebuild the object unnecessarily.
273-282: 💤 Low valueOff-by-one risk if
gradientarray has odd length.The loop steps
i += 2and reads bothgradient[i](offset) andgradient[i + 1](color). If the underlyingGRADIENT_DEFINITIONSdata ever has an odd-length array, the last stop will havestopColor={undefined}(rendered as the string"undefined"), silently breaking the gradient. A length assertion or a safe access (gradient[i + 1] ?? "transparent") would guard against data regressions.🤖 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 `@fdm-app/app/components/blocks/atlas/atlas-legend.tsx` around lines 273 - 282, The loop building gradientSvg reads pairs from gradient with i += 2 and can read gradient[i + 1] out-of-bounds if the array is odd-length; update the loop in atlas-legend (where gradientSvg and gradient are used, and GRADIENT_DEFINITIONS supplies the data) to guard against an odd-length array by either validating gradient.length is even before the loop or by using a safe access for the color (e.g., use gradient[i + 1] ?? 'transparent' or a fallback color) and ensure offset is coerced to a number before formatting so the stop element never receives undefined values.
213-213: ⚡ Quick win
EnumSoilAnalysisLegendsilently renders nothing for any enum parameter other than"b_soiltype_agr".The early-return
[]on line 213 is hard-coded to a single parameter name. If a second enum-shaded parameter is ever added toshadingConfig, the legend will appear to work (no error) but render an empty table. Consider either generalising the swatch lookup or adding an assertion/warning.🤖 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 `@fdm-app/app/components/blocks/atlas/atlas-legend.tsx` at line 213, EnumSoilAnalysisLegend currently hard-codes an early return for props.selectedParameter === "b_soiltype_agr", causing silent empty renders for any other enum parameter; change the logic to detect enum shading dynamically by looking up shadingConfig[props.selectedParameter] (or equivalent mapping used in this file) and checking its type/enum flag, then use that entry to fetch swatches; if no enum config exists for the selected parameter emit a warning (console.warn or app logger) instead of returning an empty array so missing configs are visible. Ensure references include EnumSoilAnalysisLegend, props.selectedParameter, shadingConfig (or the component's swatch lookup function) and the swatch retrieval path so the conditional is generalized.
🤖 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 `@fdm-app/app/components/blocks/atlas/atlas-legend.tsx`:
- Around line 252-266: gradDef can be undefined which causes crashes when
accessing gradDef.center or gradDef.gradient; update the logic around the
gradDef resolution (the const gradDef using GRADIENT_DEFINITIONS and
GRADIENT_SHADED_SOIL_PARAMETERS) to guard against missing mappings: if the
lookup yields undefined, fall back to a safe default gradient definition (or
skip gradient-specific adjustments) before using
gradDef.center/gradDef.gradient, and ensure
getShadingParameterMapper(props.selectedParameter) still receives a valid
parameter; in short, add a null check for gradDef and apply a sensible default
or early-return behavior so downstream uses of gradDef (center, gradient) never
dereference undefined.
In `@fdm-app/app/components/blocks/header/field.tsx`:
- Around line 34-39: The BreadcrumbSeparator currently always renders at xl
breakpoint and creates an orphan when compact mode hides the "Perceel"
BreadcrumbItem; update the BreadcrumbSeparator element to use the same
conditional classes as the BreadcrumbItem (e.g., className={cn("hidden",
!compact && "xl:block")}) so the separator is also hidden in compact mode and
only shows when the breadcrumb item is visible; locate the BreadcrumbSeparator
and BreadcrumbItem in the header component to apply the change.
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.atlas_.soil-analysis.$b_id.soil.analysis.$a_id.tsx:
- Around line 82-93: The loader currently fetches soilAnalysis by a_id using
getSoilAnalysis but never verifies it belongs to the route's field (b_id);
update the loader to check that the returned soilAnalysis.field_id (or the
appropriate property on the soilAnalysis object that references the field/bed)
equals the route param b_id and if not, throw the same 404 data error; ensure
you reference getSoilAnalysis, soilAnalysis, b_id and a_id when making the check
so an a_id from another field cannot be rendered under this field route.
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.atlas_.soil-analysis.$b_id.tsx:
- Around line 19-31: Validate the route params before using them: check that
params.b_id_farm, params.calendar and params.b_id are present and non-empty at
the top of the loader/action (before calling getTimeframe or getFields). If any
are missing, return/throw a clear 400-level error (e.g., throw new
Response('Missing required route param: b_id_farm', { status: 400 }))
referencing the param name so callers get a proper 400 instead of downstream
errors; update the logic around getTimeframe(params) and getFields(fdm,
session.principal_id, b_id_farm, timeframe) to run only after these validations.
---
Nitpick comments:
In `@fdm-app/app/components/blocks/atlas/atlas-legend.tsx`:
- Around line 134-136: shadingConfig is being rebuilt on every render by calling
getShadedSoilParameters() and Object.fromEntries each time; memoize this
computation (e.g., using React.useMemo) so shadingConfig is only recomputed when
its inputs change. Locate the shadingConfig creation in atlas-legend.tsx (the
call to getShadedSoilParameters() and Object.fromEntries) and wrap it in a memo
tied to the relevant dependency (such as any props/state or the
getShadedSoilParameters function itself) so renders from hover/parameter UI
changes don’t rebuild the object unnecessarily.
- Around line 273-282: The loop building gradientSvg reads pairs from gradient
with i += 2 and can read gradient[i + 1] out-of-bounds if the array is
odd-length; update the loop in atlas-legend (where gradientSvg and gradient are
used, and GRADIENT_DEFINITIONS supplies the data) to guard against an odd-length
array by either validating gradient.length is even before the loop or by using a
safe access for the color (e.g., use gradient[i + 1] ?? 'transparent' or a
fallback color) and ensure offset is coerced to a number before formatting so
the stop element never receives undefined values.
- Line 213: EnumSoilAnalysisLegend currently hard-codes an early return for
props.selectedParameter === "b_soiltype_agr", causing silent empty renders for
any other enum parameter; change the logic to detect enum shading dynamically by
looking up shadingConfig[props.selectedParameter] (or equivalent mapping used in
this file) and checking its type/enum flag, then use that entry to fetch
swatches; if no enum config exists for the selected parameter emit a warning
(console.warn or app logger) instead of returning an empty array so missing
configs are visible. Ensure references include EnumSoilAnalysisLegend,
props.selectedParameter, shadingConfig (or the component's swatch lookup
function) and the swatch retrieval path so the conditional is generalized.
🪄 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: 0e5d23c2-633f-4577-bedc-3ff42d6e5073
📒 Files selected for processing (5)
fdm-app/app/components/blocks/atlas/atlas-legend.tsxfdm-app/app/components/blocks/header/field.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.atlas_.soil-analysis.$b_id.soil._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.atlas_.soil-analysis.$b_id.soil.analysis.$a_id.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.atlas_.soil-analysis.$b_id.tsx
✅ Files skipped from review due to trivial changes (1)
- fdm-app/app/routes/farm.$b_id_farm.$calendar.atlas_.soil-analysis.$b_id.soil._index.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@fdm-app/app/components/blocks/atlas/atlas-soil-analysis.ts`:
- Line 35: The color literal for the "dekzand" swatch in the atlas soil palette
(object with value: "dekzand", label: "Dekzand") is invalid ("#FFF99"); update
that value to a valid hex color (e.g., change "#FFF99" to a valid 6- or 3-digit
hex such as "#FFFF99" or "#FFF999" as appropriate) so the swatch renders
correctly.
- Around line 158-162: The center-spanning branch currently sets the normalized
range variable toMin instead of updating the data-range upper bound, which
breaks diverging gradients; replace the assignment "toMin = center + radius"
with "fromMax = center + radius" so the symmetric upper data bound (fromMax) is
updated alongside fromMin, using the variables min, center, max, radius, fromMin
and fromMax in the same block.
🪄 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: 93495fe0-fd4c-4838-b776-00ba3f759692
📒 Files selected for processing (3)
fdm-app/app/components/blocks/atlas/atlas-legend.tsxfdm-app/app/components/blocks/atlas/atlas-soil-analysis.tsfdm-app/app/routes/farm.$b_id_farm.$calendar.atlas_.soil-analysis.$b_id.soil._index.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- fdm-app/app/routes/farm.$b_id_farm.$calendar.atlas_.soil-analysis.$b_id.soil._index.tsx
| export const SHADED_SOIL_TYPES = [ | ||
| { value: "moerige_klei", label: "Moerige klei", fill: "#D37FD0" }, | ||
| { value: "rivierklei", label: "Rivierklei", fill: "#81FE00" }, | ||
| { value: "dekzand", label: "Dekzand", fill: "#FFF99" }, |
Contributor
There was a problem hiding this comment.
Fix the invalid dekzand color literal.
Line 35 uses #FFF99, which is not a valid hex color. That means the dekzand swatch/color won't render reliably when the enum palette is used.
🤖 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 `@fdm-app/app/components/blocks/atlas/atlas-soil-analysis.ts` at line 35, The
color literal for the "dekzand" swatch in the atlas soil palette (object with
value: "dekzand", label: "Dekzand") is invalid ("#FFF99"); update that value to
a valid hex color (e.g., change "#FFF99" to a valid 6- or 3-digit hex such as
"#FFFF99" or "#FFF999" as appropriate) so the swatch renders correctly.
Comment on lines
+158
to
+162
| if (min <= center && max >= center) { | ||
| const radius = Math.max(max - center, center - min) | ||
| fromMin = center - radius | ||
| toMin = center + radius | ||
| } |
Contributor
There was a problem hiding this comment.
Assign the symmetric upper bound to fromMax, not toMin.
In the center-spanning branch, Line 161 updates the normalized range instead of the data range. That breaks diverging gradients like ph, so both the map fill and legend stops drift around the center value.
🛠️ Minimal fix
if (min <= center && max >= center) {
const radius = Math.max(max - center, center - min)
fromMin = center - radius
- toMin = center + radius
+ fromMax = center + radius
}📝 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.
Suggested change
| if (min <= center && max >= center) { | |
| const radius = Math.max(max - center, center - min) | |
| fromMin = center - radius | |
| toMin = center + radius | |
| } | |
| if (min <= center && max >= center) { | |
| const radius = Math.max(max - center, center - min) | |
| fromMin = center - radius | |
| fromMax = center + radius | |
| } |
🤖 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 `@fdm-app/app/components/blocks/atlas/atlas-soil-analysis.ts` around lines 158
- 162, The center-spanning branch currently sets the normalized range variable
toMin instead of updating the data-range upper bound, which breaks diverging
gradients; replace the assignment "toMin = center + radius" with "fromMax =
center + radius" so the symmetric upper data bound (fromMax) is updated
alongside fromMin, using the variables min, center, max, radius, fromMin and
fromMax in the same block.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
New Features
Bug Fixes
Closes #580
Resolves FDM-245
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes