Add field-level indicators detail page with map and radar chart#619
Open
SvenVw wants to merge 6 commits into
Open
Add field-level indicators detail page with map and radar chart#619SvenVw wants to merge 6 commits into
SvenVw wants to merge 6 commits into
Conversation
…for the indicators and a map to quickly switch fields
Contributor
📝 WalkthroughWalkthroughAdds a field-level indicators detail page displaying BLN3 scores, map, and radar chart for individual farm fields. Refactors cultivation history calculation to use a shared ChangesIndicators field-detail page with cultivation refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
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 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 |
11 tasks
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Collaborator
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
coderabbitai
Bot
changed the title
![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: 6
🧹 Nitpick comments (1)
fdm-calculator/src/bln3/input.test.ts (1)
453-487: ⚡ Quick winAdd boundary and tie-break coverage for
findHoofdteelt.This scenario proves the year remapping, but the fragile parts of the shared helper are still unpinned: exact May 15 / July 15 boundaries and equal-overlap ties. One focused case for each would protect both BLN3 and
determineNLHoofdteeltfrom silent 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-calculator/src/bln3/input.test.ts` around lines 453 - 487, Add focused unit tests around findHoofdteelt (and determineNLHoofdteelt) to cover two fragile cases: one where a cultivation's coverage starts or ends exactly on the May 15 and July 15 boundaries to assert whether it is considered in-window, and one tie-break case where two cultivations have exactly equal overlap with the May 15–July 15 window to assert the chosen winner (and expected tie-resolution behavior). Create small mock cultivations with b_lu_start/b_lu_end set to the exact boundary dates (May 15 and July 15) and another pair with identical overlap durations, call findHoofdteelt/determineNLHoofdteelt (via collectInputForBln3Score or directly) and assert the deterministic result so future changes to boundary or tie logic will be detected.
🤖 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/indicators/field-input-dialog.tsx`:
- Around line 113-115: The list rendering in cultivations.map((c, i) => (<li
key={i}>...)) uses the array index as key which can break identity on reorder;
update the key to a stable unique identifier from each cultivation object (e.g.,
use c.id, c.uuid or c.slug) in the <li> instead of i, and if cultivations items
lack a unique id add one at creation or derive a stable key (e.g., a
concatenation of immutable fields) so the Badge/list items keep consistent
identity.
In `@fdm-app/app/components/blocks/indicators/field-map.tsx`:
- Around line 23-24: The current FieldMap implementation imports and uses
FieldsSourceNotClickable and diverges from the agreed contract; update the
component to use the standard contract: replace/stop using
FieldsSourceNotClickable and instead respect FieldsSourceAvailable and the
fieldsSelected flag for source selection UI, ensure navigation uses relative
routing (e.g., navigate relative to current route rather than absolute paths)
and update any handlers/methods that build routes or selection state (look for
functions/props named fieldsSelected, FieldsSourceAvailable, and any navigation
helpers in FieldMap) so the component's source availability, selection, and
navigation behaviors match the technical notes across the affected blocks
(including the other occurrences around lines ~87-93 and ~153-170).
In `@fdm-app/app/components/blocks/indicators/indicator-card.tsx`:
- Around line 54-56: The width calculations for the score bars can go negative
when values are below zero; update the calculations for indexWidth and
impactWidth to clamp inputs into the [0,100] range before composing them.
Specifically, compute indexWidth by clamping indexValue with Math.max(0,
Math.min(indexValue, 100)) and compute impactWidth by clamping impactValue and
the remaining space with Math.max(0, Math.min(impactValue, 100 - indexWidth));
adjust any references in indicator-card.tsx to use these clamped values.
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.indicators.$b_id.tsx:
- Around line 161-173: The code currently checks b_id_farm and b_id but does not
validate the route param calendar before it’s used for year arithmetic, which
can produce NaN; update the handling around the calendar variable (the const
calendar = params.calendar block and the later similar block at lines ~289-299)
to parse calendar as a number (e.g., const year = Number(calendar) or
parseInt(calendar, 10)), verify !Number.isNaN(year) (or isFinite(year)), and if
invalid throw data("invalid: calendar", { status: 400, statusText: "invalid:
calendar" }) so downstream cultivation/summary logic uses a validated numeric
year.
In `@fdm-calculator/src/bln3/input.ts`:
- Around line 42-48: Update the JSDoc for collectInputForBln3Score to reflect
that the timeframe parameter is applied to soil analyses and measures only and
does not filter cultivations; change the description that currently says
"timeframe filters cultivations" to state that cultivations are fetched without
timeframe (full multi-year history) while timeframe still limits getSoilAnalyses
and getMeasures. Mention the function name collectInputForBln3Score and the
parameter timeframe so reviewers can find and update the doc string accordingly.
In `@fdm-calculator/src/shared/hoofdteelt.ts`:
- Around line 37-38: The reference window is constructed with new
Date("YYYY-MM-DD") (windowStart, windowEnd) which creates UTC-midnight dates and
can misalign with cultivation dates created as local-midnight (b_lu_start,
b_lu_end), causing incorrect overlap checks in the overlap calculation; change
the construction to match the incoming cultivation date strategy (e.g., new
Date(year, monthIndex, day) using monthIndex-1 semantics or use Date.UTC and
construct local dates consistently) so windowStart/windowEnd and
b_lu_start/b_lu_end use the same timezone semantics and the overlap logic (where
overlap is computed) is correct.
---
Nitpick comments:
In `@fdm-calculator/src/bln3/input.test.ts`:
- Around line 453-487: Add focused unit tests around findHoofdteelt (and
determineNLHoofdteelt) to cover two fragile cases: one where a cultivation's
coverage starts or ends exactly on the May 15 and July 15 boundaries to assert
whether it is considered in-window, and one tie-break case where two
cultivations have exactly equal overlap with the May 15–July 15 window to assert
the chosen winner (and expected tie-resolution behavior). Create small mock
cultivations with b_lu_start/b_lu_end set to the exact boundary dates (May 15
and July 15) and another pair with identical overlap durations, call
findHoofdteelt/determineNLHoofdteelt (via collectInputForBln3Score or directly)
and assert the deterministic result so future changes to boundary or tie logic
will be detected.
🪄 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: c65e4e7c-c5f3-4981-ad65-5c7dd697d0b2
📒 Files selected for processing (16)
.changeset/social-glasses-write.mdfdm-app/app/components/blocks/farm/farm-title.tsxfdm-app/app/components/blocks/header/indicators.tsxfdm-app/app/components/blocks/indicators/field-input-dialog.tsxfdm-app/app/components/blocks/indicators/field-map.tsxfdm-app/app/components/blocks/indicators/indicator-card.tsxfdm-app/app/components/blocks/indicators/radar-chart.tsxfdm-app/app/components/blocks/sidebar/apps.tsxfdm-app/app/integrations/bln3.server.tsfdm-app/app/lib/indicators.tsfdm-app/app/routes/farm.$b_id_farm.$calendar.indicators.$b_id.tsxfdm-calculator/src/bln3/input.test.tsfdm-calculator/src/bln3/input.tsfdm-calculator/src/index.tsfdm-calculator/src/norms/nl/2025/value/hoofdteelt.tsfdm-calculator/src/shared/hoofdteelt.ts
Comment on lines
+113
to
+115
| {cultivations.map((c, i) => ( | ||
| <li key={i}> | ||
| <Badge |
Contributor
There was a problem hiding this comment.
Use a stable key for cultivation items instead of array index.
Index keys can cause incorrect item identity when list ordering changes.
💡 Proposed fix
- {cultivations.map((c, i) => (
- <li key={i}>
+ {cultivations.map((c) => (
+ <li key={`${c.name}-${c.year ?? "na"}-${c.croprotation ?? "na"}`}>🧰 Tools
🪛 Biome (2.4.15)
[error] 114-114: Avoid using the index of an array as key property in an element.
(lint/suspicious/noArrayIndexKey)
🤖 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/indicators/field-input-dialog.tsx` around lines
113 - 115, The list rendering in cultivations.map((c, i) => (<li key={i}>...))
uses the array index as key which can break identity on reorder; update the key
to a stable unique identifier from each cultivation object (e.g., use c.id,
c.uuid or c.slug) in the <li> instead of i, and if cultivations items lack a
unique id add one at creation or derive a stable key (e.g., a concatenation of
immutable fields) so the Badge/list items keep consistent identity.
Comment on lines
+23
to
+24
| import { FieldsSourceNotClickable } from "~/components/blocks/atlas/atlas-sources" | ||
| import { |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Align map source/selection/navigation with the agreed technical contract.
This implementation diverges from the issue’s technical notes (FieldsSourceAvailable, fieldsSelected, relative routing), which risks inconsistent behavior with other map flows.
Also applies to: 87-93, 153-170
🤖 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/indicators/field-map.tsx` around lines 23 - 24,
The current FieldMap implementation imports and uses FieldsSourceNotClickable
and diverges from the agreed contract; update the component to use the standard
contract: replace/stop using FieldsSourceNotClickable and instead respect
FieldsSourceAvailable and the fieldsSelected flag for source selection UI,
ensure navigation uses relative routing (e.g., navigate relative to current
route rather than absolute paths) and update any handlers/methods that build
routes or selection state (look for functions/props named fieldsSelected,
FieldsSourceAvailable, and any navigation helpers in FieldMap) so the
component's source availability, selection, and navigation behaviors match the
technical notes across the affected blocks (including the other occurrences
around lines ~87-93 and ~153-170).
Comment on lines
+54
to
+56
| const indexWidth = Math.min(indexValue, 100) | ||
| const impactWidth = Math.min(impactValue, 100 - indexWidth) | ||
| return ( |
Contributor
There was a problem hiding this comment.
Clamp score bar widths to [0,100] to prevent negative CSS widths.
Current logic allows negative widths when a display value is below zero.
💡 Proposed fix
- const indexWidth = Math.min(indexValue, 100)
- const impactWidth = Math.min(impactValue, 100 - indexWidth)
+ const indexWidth = Math.max(0, Math.min(indexValue, 100))
+ const impactWidth = Math.max(0, Math.min(impactValue, 100 - indexWidth))📝 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
| const indexWidth = Math.min(indexValue, 100) | |
| const impactWidth = Math.min(impactValue, 100 - indexWidth) | |
| return ( | |
| const indexWidth = Math.max(0, Math.min(indexValue, 100)) | |
| const impactWidth = Math.max(0, Math.min(impactValue, 100 - indexWidth)) | |
| return ( |
🤖 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/indicators/indicator-card.tsx` around lines 54
- 56, The width calculations for the score bars can go negative when values are
below zero; update the calculations for indexWidth and impactWidth to clamp
inputs into the [0,100] range before composing them. Specifically, compute
indexWidth by clamping indexValue with Math.max(0, Math.min(indexValue, 100))
and compute impactWidth by clamping impactValue and the remaining space with
Math.max(0, Math.min(impactValue, 100 - indexWidth)); adjust any references in
indicator-card.tsx to use these clamped values.
Comment on lines
+161
to
+173
| const calendar = params.calendar | ||
| if (!b_id_farm) { | ||
| throw data("invalid: b_id_farm", { | ||
| status: 400, | ||
| statusText: "invalid: b_id_farm", | ||
| }) | ||
| } | ||
| if (!b_id) { | ||
| throw data("invalid: b_id", { | ||
| status: 400, | ||
| statusText: "invalid: b_id", | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Validate calendar before deriving year-based cultivation data.
calendar is used for numeric year logic without validation. A malformed route param produces NaN and breaks summary generation silently.
💡 Proposed fix
const b_id_farm = params.b_id_farm
const b_id = params.b_id
const calendar = params.calendar
@@
if (!b_id) {
throw data("invalid: b_id", {
status: 400,
statusText: "invalid: b_id",
})
}
+ const calendarYear = Number.parseInt(calendar ?? "", 10)
+ if (!Number.isFinite(calendarYear)) {
+ throw data("invalid: calendar", {
+ status: 400,
+ statusText: "invalid: calendar",
+ })
+ }
@@
- const maxCalendarYear = parseInt(calendar)
+ const maxCalendarYear = calendarYearAlso applies to: 289-299
🤖 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.indicators.$b_id.tsx around
lines 161 - 173, The code currently checks b_id_farm and b_id but does not
validate the route param calendar before it’s used for year arithmetic, which
can produce NaN; update the handling around the calendar variable (the const
calendar = params.calendar block and the later similar block at lines ~289-299)
to parse calendar as a number (e.g., const year = Number(calendar) or
parseInt(calendar, 10)), verify !Number.isNaN(year) (or isFinite(year)), and if
invalid throw data("invalid: calendar", { status: 400, statusText: "invalid:
calendar" }) so downstream cultivation/summary logic uses a validated numeric
year.
Comment on lines
+42
to
48
| // Fetch all cultivations without timeframe to cover multi-year history | ||
| const [field, soilAnalyses, cultivations, measures] = await Promise.all( | ||
| [ | ||
| getField(fdm, principal_id, b_id), | ||
| getSoilAnalyses(fdm, principal_id, b_id, timeframe), | ||
| getCultivations(fdm, principal_id, b_id, timeframe), | ||
| getCultivations(fdm, principal_id, b_id), | ||
| getMeasures(fdm, principal_id, b_id, timeframe), |
Contributor
There was a problem hiding this comment.
Update the timeframe contract in the JSDoc.
Line 31 still says timeframe filters cultivations, but Lines 42-48 now intentionally fetch the full cultivation history. Since collectInputForBln3Score is exported, the current docs describe the wrong API behavior.
🤖 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-calculator/src/bln3/input.ts` around lines 42 - 48, Update the JSDoc for
collectInputForBln3Score to reflect that the timeframe parameter is applied to
soil analyses and measures only and does not filter cultivations; change the
description that currently says "timeframe filters cultivations" to state that
cultivations are fetched without timeframe (full multi-year history) while
timeframe still limits getSoilAnalyses and getMeasures. Mention the function
name collectInputForBln3Score and the parameter timeframe so reviewers can find
and update the doc string accordingly.
Comment on lines
+37
to
+38
| const windowStart = new Date(`${year}-05-15`) | ||
| const windowEnd = new Date(`${year}-07-15`) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for tz in UTC Europe/Amsterdam; do
TZ="$tz" node - <<'NODE'
const year = 2025;
const parsedStart = new Date(`${year}-05-15`);
const parsedEnd = new Date(`${year}-07-15`);
const numericStart = new Date(year, 4, 15);
const numericEnd = new Date(year, 6, 15);
console.log(JSON.stringify({
tz: process.env.TZ,
parsedStart: parsedStart.toISOString(),
numericStart: numericStart.toISOString(),
startDeltaMs: parsedStart.getTime() - numericStart.getTime(),
parsedEnd: parsedEnd.toISOString(),
numericEnd: numericEnd.toISOString(),
endDeltaMs: parsedEnd.getTime() - numericEnd.getTime(),
}, null, 2));
NODE
doneRepository: nmi-agro/fdm
Length of output: 553
🏁 Script executed:
fd "hoofdteelt.ts" --type fRepository: nmi-agro/fdm
Length of output: 149
🏁 Script executed:
cat -n fdm-calculator/src/shared/hoofdteelt.tsRepository: nmi-agro/fdm
Length of output: 2860
🏁 Script executed:
rg "findHoofdteelt" --type ts -A 3 -B 3Repository: nmi-agro/fdm
Length of output: 5818
🏁 Script executed:
rg "b_lu_start\|b_lu_end" --type ts -l | head -20Repository: nmi-agro/fdm
Length of output: 38
🏁 Script executed:
cat -n fdm-calculator/src/norms/nl/2025/value/hoofdteelt.tsRepository: nmi-agro/fdm
Length of output: 1215
🏁 Script executed:
rg "b_lu_start.*new Date|new Date.*b_lu_start" --type ts -B 3 -A 3Repository: nmi-agro/fdm
Length of output: 50369
🏁 Script executed:
rg "new Date\(" fdm-calculator/src/norms/nl/2025/filling/input.ts -A 2 -B 2Repository: nmi-agro/fdm
Length of output: 961
🏁 Script executed:
rg "getCultivations|getDefaultCultivation" fdm-calculator/src/bln3/input.ts -A 5 | head -40Repository: nmi-agro/fdm
Length of output: 394
Use an explicit date-construction strategy for the reference window.
The window boundaries on lines 37–38 are created using new Date("YYYY-MM-DD"), which parses as UTC midnight. If b_lu_start / b_lu_end values are constructed as local-midnight dates (e.g., new Date(year, month, day)), the overlap calculation at line 52 will be off by the timezone offset. In UTC+2 timezones (e.g., Europe/Amsterdam during summer), this shifts the boundary by 2 hours and can flip inclusion of cultivations that start or end exactly on May 15 or July 15.
Construct windowStart and windowEnd using the same method as the incoming cultivation dates, or use an explicit UTC-aware constructor to make the intent clear.
🤖 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-calculator/src/shared/hoofdteelt.ts` around lines 37 - 38, The reference
window is constructed with new Date("YYYY-MM-DD") (windowStart, windowEnd) which
creates UTC-midnight dates and can misalign with cultivation dates created as
local-midnight (b_lu_start, b_lu_end), causing incorrect overlap checks in the
overlap calculation; change the construction to match the incoming cultivation
date strategy (e.g., new Date(year, monthIndex, day) using monthIndex-1
semantics or use Date.UTC and construct local dates consistently) so
windowStart/windowEnd and b_lu_start/b_lu_end use the same timezone semantics
and the overlap logic (where overlap is computed) is correct.
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
1 participant
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.
Summary by CodeRabbit
Release Notes
New Features
Tests
Closes #576