Show soil descriptions on the soil atlas#436
Conversation
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
WalkthroughThis PR adds Bodemdata soil legend integration to the farm atlas feature. A new route loader fetches soil class data from an external Bodemdata API, while client-side code implements MRU caching, error handling, and UI rendering of fetched legend information in soil popups. Changes
Sequence DiagramsequenceDiagram
participant User
participant Client as Client (Atlas UI)
participant RouteLoader as Route Loader
participant BodemAPI as Bodemdata API
User->>Client: Click on soil location
Client->>Client: Check MRU cache for soilcode
alt Cache hit
Client->>Client: Retrieve cached bodemdata
else Cache miss
Client->>RouteLoader: Fetch /bodemdata/{soilcode}
RouteLoader->>BodemAPI: Fetch soil legend
BodemAPI-->>RouteLoader: Return legend data
RouteLoader-->>Client: Return parsed response
Client->>Client: Clean HTML tags & cache result
end
Client->>Client: Merge bodemdata into popup.properties
Client->>User: Render popup with bodemdata.omschrijving
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release/2026-01 #436 +/- ##
================================================
Coverage 88.07% 88.07%
================================================
Files 91 91
Lines 4621 4621
Branches 1492 1492
================================================
Hits 4070 4070
Misses 551 551
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.atlas.soil.bodemdata.$soilcode.ts:
- Around line 3-9: Wrap the external fetch in the loader function (loader) with
a try/catch and a timeout via AbortController: create an AbortController, set a
short timeout (e.g. 5s) that calls controller.abort(), pass controller.signal to
fetch(`https://legenda-bodemkaart.bodemdata.nl/soilmaplegendserver/item/bodemklasse/${params.soilcode}`),
and clear the timeout on success; in the catch block handle both AbortError and
other errors and return a proper response using the data() helper (or throw a
Response) with an appropriate status and error message so calling code receives
a controlled error instead of an unhandled exception.
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.atlas.soil.tsx:
- Around line 194-206: The closure computes cachedIndex from the outer
cachedBodemData but then mutates state in setCachedBodemData's functional
updater, causing race conditions; move the index computation inside the updater
by calling findIndex on the updater argument (e.g., current => { const idx =
current.findIndex(item => item.key === soilcode); if (idx > -1) { const update =
[...current]; const [cached] = update.splice(idx,1); update.push(cached); return
update; } return current; }) and obtain the found.value from current[idx] (or
the spliced cached) so you still return the correct value to the caller while
avoiding the stale-closure bug in setCachedBodemData.
🧹 Nitpick comments (2)
fdm-app/app/routes/farm.$b_id_farm.$calendar.atlas.soil.tsx (2)
208-210: Hardcodedundefinedin the fetch URL.The URL path
/farm/undefined/all/atlas/soil/bodemdata/${soilcode}hardcodes"undefined"for the farm ID. While this works because the server-side loader doesn't use these route params, this is fragile and unclear to future maintainers. Consider using a dedicated API route path that doesn't require placeholder route segments.
216-218: HTML tag stripping is limited to<p>tags.Consider using a more robust approach to strip HTML tags if the API response could contain other HTML elements.
♻️ Alternative: strip all HTML tags
- data.omschrijving = data.omschrijving - ?.replaceAll("<p>", "") - .replaceAll("</p>", "") + data.omschrijving = data.omschrijving + ?.replace(/<[^>]*>/g, "")
| export async function loader({ params }: Route.LoaderArgs) { | ||
| // Fetching client-side leads to CORS and CSP errors. | ||
| // CSP issues can be resolved but CORS issues can't be without contacting Bodemdata. | ||
| return fetch( | ||
| `https://legenda-bodemkaart.bodemdata.nl/soilmaplegendserver/item/bodemklasse/${params.soilcode}`, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Add error handling and consider a timeout for the external fetch.
The loader returns the fetch result directly without error handling. If the external Bodemdata API is unavailable or returns an error, this will propagate as an unhandled exception. Additionally, there's no timeout, so a slow external service could hang requests indefinitely.
🛡️ Proposed fix with error handling and timeout
+import { data } from "react-router"
import type { Route } from "./+types/farm.$b_id_farm.$calendar.atlas.soil.bodemdata.$soilcode"
export async function loader({ params }: Route.LoaderArgs) {
// Fetching client-side leads to CORS and CSP errors.
// CSP issues can be resolved but CORS issues can't be without contacting Bodemdata.
- return fetch(
- `https://legenda-bodemkaart.bodemdata.nl/soilmaplegendserver/item/bodemklasse/${params.soilcode}`,
- )
+ try {
+ const controller = new AbortController()
+ const timeoutId = setTimeout(() => controller.abort(), 5000)
+
+ const response = await fetch(
+ `https://legenda-bodemkaart.bodemdata.nl/soilmaplegendserver/item/bodemklasse/${params.soilcode}`,
+ { signal: controller.signal }
+ )
+ clearTimeout(timeoutId)
+
+ if (!response.ok) {
+ return data({ success: false, error: "External API error" }, { status: 502 })
+ }
+
+ const json = await response.json()
+ return data({ success: true, data: json })
+ } catch (error) {
+ return data({ success: false, error: "Failed to fetch soil data" }, { status: 502 })
+ }
}Based on learnings: In React Router v7, the data() function is used for creating responses in loaders and actions.
📝 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.
| export async function loader({ params }: Route.LoaderArgs) { | |
| // Fetching client-side leads to CORS and CSP errors. | |
| // CSP issues can be resolved but CORS issues can't be without contacting Bodemdata. | |
| return fetch( | |
| `https://legenda-bodemkaart.bodemdata.nl/soilmaplegendserver/item/bodemklasse/${params.soilcode}`, | |
| ) | |
| } | |
| import { data } from "react-router" | |
| import type { Route } from "./+types/farm.$b_id_farm.$calendar.atlas.soil.bodemdata.$soilcode" | |
| export async function loader({ params }: Route.LoaderArgs) { | |
| // Fetching client-side leads to CORS and CSP errors. | |
| // CSP issues can be resolved but CORS issues can't be without contacting Bodemdata. | |
| try { | |
| const controller = new AbortController() | |
| const timeoutId = setTimeout(() => controller.abort(), 5000) | |
| const response = await fetch( | |
| `https://legenda-bodemkaart.bodemdata.nl/soilmaplegendserver/item/bodemklasse/${params.soilcode}`, | |
| { signal: controller.signal } | |
| ) | |
| clearTimeout(timeoutId) | |
| if (!response.ok) { | |
| return data({ success: false, error: "External API error" }, { status: 502 }) | |
| } | |
| const json = await response.json() | |
| return data({ success: true, data: json }) | |
| } catch (error) { | |
| return data({ success: false, error: "Failed to fetch soil data" }, { status: 502 }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.atlas.soil.bodemdata.$soilcode.ts
around lines 3 - 9, Wrap the external fetch in the loader function (loader) with
a try/catch and a timeout via AbortController: create an AbortController, set a
short timeout (e.g. 5s) that calls controller.abort(), pass controller.signal to
fetch(`https://legenda-bodemkaart.bodemdata.nl/soilmaplegendserver/item/bodemklasse/${params.soilcode}`),
and clear the timeout on success; in the catch block handle both AbortError and
other errors and return a proper response using the data() helper (or throw a
Response) with an appropriate status and error message so calling code receives
a controlled error instead of an unhandled exception.
| const cachedIndex = cachedBodemData.findIndex( | ||
| (item) => item.key === soilcode, | ||
| ) | ||
| if (cachedIndex > -1) { | ||
| // If found in the cache, move the cached item to the end of the list | ||
| const found = cachedBodemData[cachedIndex] | ||
| setCachedBodemData((cachedBodemData) => { | ||
| const update = [...cachedBodemData] | ||
| const cached = update.splice(cachedIndex, 1) | ||
| update.push(cached[0]) | ||
| return update | ||
| }) | ||
| return found.value |
There was a problem hiding this comment.
Stale closure issue in cache update logic.
The cachedIndex is computed from the cachedBodemData closure value, but then used inside setCachedBodemData's functional update which receives the current state. If another cache update occurs between computing the index and executing the state update, the splice will operate on the wrong index.
🐛 Proposed fix: compute index inside functional update
if (cachedIndex > -1) {
// If found in the cache, move the cached item to the end of the list
const found = cachedBodemData[cachedIndex]
- setCachedBodemData((cachedBodemData) => {
- const update = [...cachedBodemData]
- const cached = update.splice(cachedIndex, 1)
- update.push(cached[0])
+ setCachedBodemData((current) => {
+ const currentIndex = current.findIndex(
+ (item) => item.key === soilcode,
+ )
+ if (currentIndex === -1) return current
+ const update = [...current]
+ const cached = update.splice(currentIndex, 1)
+ update.push(cached[0])
return update
})
return found.value
}🤖 Prompt for AI Agents
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.atlas.soil.tsx around lines 194
- 206, The closure computes cachedIndex from the outer cachedBodemData but then
mutates state in setCachedBodemData's functional updater, causing race
conditions; move the index computation inside the updater by calling findIndex
on the updater argument (e.g., current => { const idx = current.findIndex(item
=> item.key === soilcode); if (idx > -1) { const update = [...current]; const
[cached] = update.splice(idx,1); update.push(cached); return update; } return
current; }) and obtain the found.value from current[idx] (or the spliced cached)
so you still return the correct value to the caller while avoiding the
stale-closure bug in setCachedBodemData.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Finished now. |
|
Now the badge shows the first soil code too. |
3 participants
Enhancements
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.