Add nitrogen mineralization feature with NMI API integration#553
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a complete Mineralisatie feature: server integrations to call NMI nsupply/dyna APIs (with builders, Zod schemas, cached wrappers), many UI components (charts, KPI/cards, lists, skeletons), new routes (farm overview, field detail, dyna), and sidebar/feature-flag gating. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Loader as Remix Loader
participant DB
participant Cache
participant NMI
Browser->>Loader: GET /farm/{id}/{cal}/mineralization
Loader->>DB: fetch farm, fields, soils, cultivations
DB-->>Loader: inputs
Loader->>Cache: check per-field nsupply cache
alt cache miss
Loader->>NMI: POST /bemestingsplan/nsupply
NMI-->>Loader: nsupply response
Loader->>Cache: store result
end
Loader-->>Browser: stream asyncNSupply promises
Browser->>Browser: render Suspense fallback → charts/KPIs when ready
sequenceDiagram
participant Browser
participant Loader as Remix Loader
participant DB
participant Cache
participant NMI
Browser->>Loader: GET /farm/{id}/{cal}/mineralization/{b_id}/dyna
Loader->>DB: fetch field, cultivations, fertilizers, harvests
DB-->>Loader: context
Loader->>Loader: validate prerequisites (harvest dates)
alt validation fails
Loader-->>Browser: missingHarvestDate response
else
Loader->>Cache: check dyna cache
alt cache miss
Loader->>NMI: POST /bemestingsplan/dyna
NMI-->>Loader: dyna response
Loader->>Cache: store result
end
Loader-->>Browser: asyncDyna promise + chartEvents
Browser->>Browser: render Suspense → DynaChart, BalanceCard, AdviceCard
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 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❌ 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}
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: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fdm-app/app/components/blocks/sidebar/apps.tsx (1)
97-106:⚠️ Potential issue | 🟠 MajorWire the Mineralisatie app into the sidebar.
This Apps sidebar still only exposes Atlas, Balans, Bemestingsadvies, and Gebruiksruimte. The new
/mineralizationroute needs amineralizationLinkand menu item here so users can discover the module from the sidebar.Suggested direction
+ let mineralizationLink: string | undefined + if (isCreateFarmWizard || isFarmOverview) { + mineralizationLink = undefined + } else if (farmId && farmId !== "undefined") { + mineralizationLink = `/farm/${farmId}/${selectedCalendar}/mineralization` + } else { + mineralizationLink = undefined + } + return (Then render a
SidebarMenuItemformineralizationLinkalongside the other app entries.Also applies to: 274-306
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/sidebar/apps.tsx` around lines 97 - 106, Add a new mineralizationLink in the same pattern as omBalanceLink: declare let mineralizationLink: string | undefined and set it to undefined when isCreateFarmWizard or isFarmOverview is true, otherwise if farmId && farmId !== "undefined" set mineralizationLink = `/farm/${farmId}/${selectedCalendar}/mineralization` (else undefined). Then render a SidebarMenuItem for mineralizationLink alongside the existing SidebarMenuItem entries (the same place you render omBalanceLink/Atlas/Bemestingsadvies/Gebruiksruimte) so the Mineralisatie app appears in the sidebar; use the same visibility checks and props as the other menu items.
🟡 Minor comments (11)
fdm-app/app/components/blocks/mineralization/skeletons.tsx-79-85 (1)
79-85:⚠️ Potential issue | 🟡 MinorScope the 7-column spans to the
lgbreakpoint.
col-span-4/col-span-3apply even when the parent is onlymd:grid-cols-2, which can create implicit columns and break the fallback layout belowlg.Responsive layout fix
- <div className="col-span-4"> + <div className="lg:col-span-4"> <MineralizationChartSkeleton /> </div> - <div className="col-span-3"> + <div className="lg:col-span-3"> <MineralizationFieldsSkeleton /> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/mineralization/skeletons.tsx` around lines 79 - 85, The two child columns currently use global col-span classes which apply below the lg breakpoint and conflict with the md:grid-cols-2 layout; update the span classes for the MineralizationChartSkeleton and MineralizationFieldsSkeleton containers to be breakpoint-scoped (e.g., change col-span-4 to lg:col-span-4 and col-span-3 to lg:col-span-3) so the spans only take effect at lg and above while allowing the md fallback grid to work correctly.fdm-app/app/components/blocks/mineralization/dyna-field-list.tsx-102-110 (1)
102-110:⚠️ Potential issue | 🟡 MinorAdd accessible labels for the status icons.
The status column is icon-only, and
titleon SVG is not a reliable accessible name. Wrap the icons with explicit labels and hide the SVG from assistive tech.♿ Proposed fix
<TableCell className="text-center"> {error ? ( - <CircleX - className="h-4 w-4 text-destructive mx-auto" - title={error} - /> + <span role="img" aria-label={`Fout: ${error}`} title={error}> + <CircleX + aria-hidden="true" + className="h-4 w-4 text-destructive mx-auto" + /> + </span> ) : ( - <CircleCheck className="h-4 w-4 text-green-500 mx-auto" /> + <span role="img" aria-label="Berekening gelukt"> + <CircleCheck + aria-hidden="true" + className="h-4 w-4 text-green-500 mx-auto" + /> + </span> )} </TableCell>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/mineralization/dyna-field-list.tsx` around lines 102 - 110, The status column uses icon-only SVGs (CircleX / CircleCheck) and relies on title which isn't a reliable accessible name; wrap each icon in an explicit label (e.g., a visually-hidden span like "Error: {error}" or "Success") and hide the SVG from AT by setting aria-hidden on CircleX and CircleCheck and removing the title prop; update the TableCell rendering that checks error to include the sr-only label and aria-hidden on the SVGs so screen readers get a clear text status while the visuals remain the same.fdm-app/app/components/blocks/mineralization/field-list.tsx-80-85 (1)
80-85:⚠️ Potential issue | 🟡 MinorGive the error indicator an accessible name.
titlealone is not enough for screen readers or keyboard users. Add an explicit label and mark the SVG decorative.♿ Proposed fix
return ( - <span title={result.error}> - <CircleX className="h-4 w-4 text-muted-foreground inline" /> + <span + title={result.error} + role="img" + aria-label={`Fout: ${result.error}`} + > + <CircleX + aria-hidden="true" + className="h-4 w-4 text-muted-foreground inline" + /> </span> )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/mineralization/field-list.tsx` around lines 80 - 85, The error indicator currently only uses a title (result.error) which isn't accessible; update the block that renders CircleX in field-list.tsx (where result.error is checked) to provide an explicit accessible name and mark the SVG decorative: add a screen-reader-only visible text node (e.g., a span with the error message like "Error: {result.error}") for assistive tech, set the CircleX SVG to aria-hidden="true" (or role="img" with an appropriate aria-label removed) so it does not get announced twice, and keep the title for hover — ensure the accessible text conveys the error and is keyboard/screen-reader reachable.fdm-app/app/components/blocks/mineralization/leaching-chart.tsx-40-49 (1)
40-49:⚠️ Potential issue | 🟡 MinorParse date-only strings without UTC timezone shifts.
new Date("YYYY-MM-DD")is interpreted as UTC midnight, thengetMonth()andtoLocaleDateString()format in the browser's local timezone. In negative timezones (e.g., America/New_York), this shifts the date backward — April 1 displays as March 31, withgetMonth()returning 2 (March). This affects both the month tick labels and date tooltips.🐛 Proposed fix
+function parseDateOnly(dateStr: string): Date | null { + const match = /^(\d{4})-(\d{2})-(\d{2})/.exec(dateStr) + if (!match) return null + const [, year, month, day] = match + const d = new Date(Number(year), Number(month) - 1, Number(day)) + return Number.isNaN(d.getTime()) ? null : d +} + function formatDateTick(dateStr: string): string { - const d = new Date(dateStr) - if (Number.isNaN(d.getTime())) return dateStr + const d = parseDateOnly(dateStr) + if (!d) return dateStr return MONTH_LABELS_NL[d.getMonth()] ?? dateStr } function formatDateLabel(dateStr: string): string { - const d = new Date(dateStr) - if (Number.isNaN(d.getTime())) return dateStr + const d = parseDateOnly(dateStr) + if (!d) return dateStr return d.toLocaleDateString("nl-NL", { day: "numeric", month: "long" }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/mineralization/leaching-chart.tsx` around lines 40 - 49, The functions formatDateTick and formatDateLabel currently use new Date(dateStr) which treats "YYYY-MM-DD" as UTC and causes timezone shifts; modify both to detect date-only strings (e.g., /^\d{4}-\d{2}-\d{2}$/), parse into year/month/day parts and construct the Date with the local constructor new Date(year, monthIndex, day) (use monthIndex = month-1), then keep the existing invalid-date fallback and existing return values (MONTH_LABELS_NL[d.getMonth()] in formatDateTick and d.toLocaleDateString("nl-NL", { day: "numeric", month: "long" }) in formatDateLabel).fdm-app/app/components/blocks/mineralization/field-list.tsx-19-26 (1)
19-26:⚠️ Potential issue | 🟡 MinorFix the day-of-year off-by-one calculation.
Using
Math.ceil(...) + 1returns2for January 1, so the "% vandaag" value is based on tomorrow's data point instead of today's. Normalize to calendar midnight before calculation to work with whole days only.🐛 Proposed fix
function getCurrentDoy(): number { const now = new Date() const startOfYear = new Date(now.getFullYear(), 0, 1) - return ( - Math.ceil( - (now.getTime() - startOfYear.getTime()) / (1000 * 60 * 60 * 24), - ) + 1 - ) + const today = new Date(now.getFullYear(), now.getMonth(), now.getDate()) + return Math.floor( + (today.getTime() - startOfYear.getTime()) / (1000 * 60 * 60 * 24), + ) + 1 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/mineralization/field-list.tsx` around lines 19 - 26, getCurrentDoy currently normalizes to fractions of a day and uses Math.ceil(...) + 1 which yields 2 on Jan 1; update getCurrentDoy to normalize both now and startOfYear to calendar midnight (setHours(0,0,0,0)), compute the whole-day difference in milliseconds divided by msPerDay, then use Math.floor(...) + 1 to return 1 for January 1; locate and change the implementation in the getCurrentDoy function accordingly.fdm-app/app/components/blocks/mineralization/mineralization-chart.tsx-54-60 (1)
54-60:⚠️ Potential issue | 🟡 MinorOff-by-one in
getCurrentDoy.
Math.ceil((now - Jan 1) / msPerDay) + 1overshoots by 1 for any non-midnight time. E.g. atJan 1 12:00the diff is0.5 day,ceil(0.5) = 1,+1 = 2, but DOY should be1. AtJan 2 12:00it returns3instead of2. This shifts the "Vandaag" reference line on bothFarmMineralizationChartandFieldMineralizationChartforward by one day, and also affects downstream KPIs that import this helper.🛠 Proposed fix
function getCurrentDoy(): number { const now = new Date() const start = new Date(now.getFullYear(), 0, 1) - return Math.ceil( - (now.getTime() - start.getTime()) / (1000 * 60 * 60 * 24), - ) + 1 + return Math.floor( + (now.getTime() - start.getTime()) / (1000 * 60 * 60 * 24), + ) + 1 }Also, the
nsupply-kpi.tsxfile defines its own localgetCurrentDoywith the same off-by-one — please consolidate on this exported helper after the fix, and also align the DOY formula used in the_indexloader (which usesnew Date(year, 0, 0)/floor).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/mineralization/mineralization-chart.tsx` around lines 54 - 60, getCurrentDoy currently overcounts by 1 because it uses new Date(year,0,1) + Math.ceil(...) + 1; update the getCurrentDoy function in mineralization-chart.tsx to compute start = new Date(now.getFullYear(), 0, 0) and return Math.floor((now.getTime() - start.getTime()) / (1000 * 60 * 60 * 24)); remove the duplicate local getCurrentDoy in nsupply-kpi.tsx and import this fixed exported helper instead, and update the _index loader’s DOY computation to the same start/new-floor formula so all consumers (FarmMineralizationChart, FieldMineralizationChart and KPIs) use the consistent, correct DOY.fdm-app/app/components/blocks/mineralization/dyna-chart.tsx-113-129 (1)
113-129:⚠️ Potential issue | 🟡 Minor
EventDothides events atx === 0and assumesevents[0]is defined.
if (!viewBox?.x || ...)treatsx === 0as falsy and skips rendering — a legitimate edge case at the chart's left edge. UseviewBox?.x === undefinedinstead.EVENT_COLORS[events[0].type]throws ifeventsis ever empty.groupEventsByDatecurrently only creates non-empty arrays, but a small guard keeps this component safe to reuse.🛠 Proposed fix
- if (!viewBox?.x || viewBox.y === undefined) return null - const x = viewBox.x - const y = (viewBox.y ?? 0) + 10 - const color = EVENT_COLORS[events[0].type] + if (viewBox?.x === undefined || viewBox?.y === undefined) return null + const first = events[0] + if (!first) return null + const x = viewBox.x + const y = viewBox.y + 10 + const color = EVENT_COLORS[first.type]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/mineralization/dyna-chart.tsx` around lines 113 - 129, In EventDot, don't treat x===0 as falsy and don't assume events[0] exists: change the viewBox check to test viewBox?.x === undefined (or viewBox?.x == null) instead of !viewBox?.x so zero is allowed, and add a small guard like if (!events || events.length === 0) return null; when resolving color use a safe lookup with a fallback (e.g., const color = EVENT_COLORS[events[0].type] ?? DEFAULT_EVENT_COLOR) to avoid throwing if events is empty or an unknown type; keep the existing y calculation using viewBox.y fallback.fdm-app/app/routes/farm.$b_id_farm.$calendar.mineralization.$b_id.dyna.tsx-365-372 (1)
365-372:⚠️ Potential issue | 🟡 Minor
todayis derived from UTC; can be off by one for local TZ near midnight.
new Date().toISOString().split("T")[0]returns the UTC date, butb_date_calculationvalues coming back from DYNA represent calendar days in the farm's local context. In Europe/Amsterdam after ~22:00 CET (21:00 UTC in winter) the user is on day N locally whiletodayis still N; but during CEST late eveningstodaycan jump to N+1 before midnight. In the other direction, early local mornings can leavetodayon the previous day. This affects thetodayPointlookup and the "Verwachting" marker.Use a local-date formatter instead:
🛠 Proposed fix
- const today = new Date().toISOString().split("T")[0] ?? "" + const now = new Date() + const today = `${now.getFullYear()}-${String(now.getMonth() + 1).padStart(2, "0")}-${String(now.getDate()).padStart(2, "0")}`The same pattern appears in
dyna-chart.tsx(line 232) and should be updated there too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.mineralization.$b_id.dyna.tsx around lines 365 - 372, The code uses UTC date for "today" which can misalign with farm-local calendar; change the today calculation from new Date().toISOString().split("T")[0] to a local-date string (e.g. new Date().toLocaleDateString('en-CA') which yields YYYY-MM-DD in local time) so the lookup for todayPoint (yearData.find(...)) uses the farm's local day; update the same pattern where "today" is computed in farm.$b_id_farm.$calendar.mineralization.$b_id.dyna.tsx (variables: today, todayPoint, currentNAvailability, currentUptake, totalLeaching, yearData) and also in dyna-chart.tsx (the analogous today/todayPoint logic) to use the local-date formatter.fdm-app/app/integrations/mineralization.server.ts-598-605 (1)
598-605:⚠️ Potential issue | 🟡 Minor
generateInsightshardcodesnew Date().getFullYear()— wrong date when viewing a non-current calendar year.The loader in
farm.$b_id_farm.$calendar.mineralization.$b_id._index.tsxcomputescurrentDoyfrom today's date and passes it through unchanged, but this helper formats the insight sentence ("Op<date>is circa …") using the current year rather than the calendar year of the data being viewed. For a user viewing 2024 data, the sentence will still be dated in 2026 and, more importantly, the "season progress" narrative is derived from today's DOY against a past season's curve. Consider acceptingyearas a parameter and usingnew Date(year, 0).getFullYear(), and having the caller decide whether to generate insights at all for non-current years.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/integrations/mineralization.server.ts` around lines 598 - 605, generateInsights currently calls doyToDateString with new Date().getFullYear(), which hardcodes the current calendar year and yields incorrect dates for non-current calendar views; change generateInsights (in fdm-app/app/integrations/mineralization.server.ts) to accept a year parameter (e.g., year: number) and pass that to doyToDateString instead of new Date().getFullYear(), compute date via doyToDateString(currentDoy, new Date(year, 0).getFullYear()), and update the caller in farm.$b_id_farm.$calendar.mineralization.$b_id._index.tsx to decide whether to call generateInsights for non-current years or pass the calendar year when generating insights. Ensure all references to generateInsights and the created date variable are updated to use the new parameter.fdm-app/app/components/blocks/mineralization/dyna-advice.tsx-17-25 (1)
17-25:⚠️ Potential issue | 🟡 MinorPotential timezone off-by-one for date-only strings.
new Date("YYYY-MM-DD")is parsed as UTC, andtoLocaleDateString("nl-NL", …)then renders it in the local timezone. For users west of UTC this can display the previous day (e.g.b_date_recommended"2025-04-15"rendering as14 april 2025). Since the advice dates come from NMI as local Dutch dates, consider parsing the parts explicitly or constructing the Date in local time.🛠 Proposed fix
function formatDate(dateStr: string): string { - const d = new Date(dateStr) - if (Number.isNaN(d.getTime())) return dateStr + const m = /^(\d{4})-(\d{2})-(\d{2})/.exec(dateStr) + const d = m + ? new Date(Number(m[1]), Number(m[2]) - 1, Number(m[3])) + : new Date(dateStr) + if (Number.isNaN(d.getTime())) return dateStr return d.toLocaleDateString("nl-NL", {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/mineralization/dyna-advice.tsx` around lines 17 - 25, The formatDate function currently constructs a Date via new Date(dateStr) which treats "YYYY-MM-DD" as UTC and can produce an off-by-one day in local time; update formatDate to parse ISO date-only strings explicitly (e.g., split "YYYY-MM-DD" into year, month, day and construct new Date(year, month-1, day) so it uses local time) while preserving the existing fallback behavior (keep the Number.isNaN(d.getTime()) check and return the original string if parsing fails); if the input contains a time component (contains 'T' or timezone) still allow Date to parse it normally. Use the function name formatDate to locate and change the logic accordingly.fdm-app/app/routes/farm.$b_id_farm.$calendar.mineralization.$b_id._index.tsx-166-174 (1)
166-174:⚠️ Potential issue | 🟡 MinorDOY computation differs from
getCurrentDoyused by the chart and KPI components.Here the loader uses
Math.floor((now - new Date(year, 0, 0)) / msPerDay), whilefdm-app/app/components/blocks/mineralization/mineralization-chart.tsx(getCurrentDoy) usesMath.ceil((now - Jan 1) / msPerDay) + 1andnsupply-kpi.tsx(getCurrentDoy) uses yet another variant. These produce different DOY values for the same moment (the chart one is generally too large — see the comment onmineralization-chart.tsx). As a result, the insight text (“Op is circa X kg N/ha gemineraliseerd…”) can reference a different DOY than the “Vandaag” reference line on the chart and the “Tot vandaag” KPI.Please centralize DOY calculation in a single helper (e.g. export
getCurrentDoyfrommineralization-chart.tsxand reuse it both here and innsupply-kpi.tsx) after fixing the chart-side formula.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.mineralization.$b_id._index.tsx around lines 166 - 174, The loader computes DOY with a different formula than the chart and KPI components causing mismatched “Vandaag”/insight dates; centralize DOY logic by exporting a single getCurrentDoy helper from mineralization-chart.tsx (fix the chart-side formula to the correct definition while you’re there), then replace the local Math.floor-based calculation in the loader (the currentDoy used when calling generateInsights) and any DOY code in nsupply-kpi.tsx to import and call that shared getCurrentDoy; ensure generateInsights continues to receive the same DOY value so chart, KPI, and insights are consistent.
🧹 Nitpick comments (3)
fdm-calculator/src/mineralisatie/schemas.ts (1)
33-35: Add.min(1)boundary validation for N-supply responses.The schema currently allows
z.array(...)to be empty, but the API contract (per documentation) guarantees 365 or 366 daily points. While downstream code innsupply.tsalready guards against empty arrays and safely defaultstotalAnnualNto0, enforcing the constraint at the schema boundary ensures API contract violations fail at parsing time instead of silently degrading to defaults.Boundary validation
export const nsupplyResponseSchema = z.object({ - data: z.array(nsupplyDataPointSchema), + data: z.array(nsupplyDataPointSchema).min(1), })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/mineralisatie/schemas.ts` around lines 33 - 35, The nsupplyResponseSchema currently allows an empty array; update the array validator on nsupplyResponseSchema to require at least one element by adding a .min(1) call to the z.array(nsupplyDataPointSchema) definition so the parser fails on empty responses (refer to nsupplyResponseSchema and nsupplyDataPointSchema to locate the change).fdm-app/app/routes/farm.$b_id_farm.$calendar.mineralization.$b_id._index.tsx (1)
96-127: Redundant soil/cultivation fetches: loader fetches + each of 3getNSupplyForFieldcalls refetches.The loader already loads
getCurrentSoilData(fororganicMatter,soilType,completeness) and then triggers threegetNSupplyForFieldcalls, each of which internally re-runsgetField,getCurrentSoilData, andgetCultivationsfor the sameb_id. That's 4×getCurrentSoilDataand 3×getField/getCultivationsper field-detail load.Consider accepting pre-fetched inputs in
getNSupplyForField(optionalfield/soilDataArray/cultivationsparams) so the loader can hydrate them once and reuse across methods.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.mineralization.$b_id._index.tsx around lines 96 - 127, The loader is redundantly calling getCurrentSoilData and then calling getNSupplyForField three times which each re-fetch getField, getCurrentSoilData, and getCultivations; to fix, change getNSupplyForField to accept optional pre-fetched inputs (e.g., parameters named field, soilDataArray, cultivations) and update its implementation to use these when provided and only call getField/getCurrentSoilData/getCultivations as a fallback; then in the loader (where you already call getCurrentSoilData and derive organicMatter/soilType/completeness) pass those pre-fetched values into each getNSupplyForField invocation so the extra duplicate fetches are avoided (references: getNSupplyForField, getCurrentSoilData, getField, getCultivations, assessDataCompleteness).fdm-app/app/components/blocks/mineralization/dyna-chart.tsx (1)
218-229:fertilizingRecommendationsprop is declared but never consumed.
DynaChartPropsdeclaresfertilizingRecommendations?: DynaFertilizerAdvice | null, and the parent route passes it through, butDynaChartdestructures onlydata,events, andyear(line 225-229). Either wire up a recommendation marker (e.g.ReferenceLine/ReferenceDotatb_date_recommended) as the original Phase 2 objective implies, or drop the prop from the interface and the call site to avoid dead API surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/mineralization/dyna-chart.tsx` around lines 218 - 229, The prop fertilizingRecommendations declared on DynaChartProps is unused; update DynaChart to consume fertilizingRecommendations (or remove it from the interface/call site). Preferred fix: accept fertilizingRecommendations in the DynaChart parameter list and render a visual marker for its b_date_recommended (e.g., add a Recharts ReferenceLine or ReferenceDot tied to fertilizingRecommendations.b_date_recommended with a label/tooltip) so the recommendation date appears on the chart; ensure you reference the DynaChart component, DynaChartProps, fertilizingRecommendations and the b_date_recommended field when implementing the marker.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fdm-app/app/components/blocks/header/mineralization.tsx`:
- Around line 62-73: The menu currently nests NavLink inside
DropdownMenuCheckboxItem (in header/mineralization.tsx iterating fieldOptions),
causing broken clicks/keyboard behavior and incorrect ARIA semantics: replace
DropdownMenuCheckboxItem with DropdownMenuItem used asChild and wrap the NavLink
as the child so the NavLink receives the activation events; remove the checkbox
semantics (checked prop using b_id) and instead render a visual check glyph
inside the DropdownMenuItem when option.b_id === b_id to indicate current
selection; ensure the NavLink target URL still uses
`/farm/${b_id_farm}/${calendar}/mineralization/${option.b_id}` and preserve
option.b_name as the label.
In `@fdm-app/app/components/blocks/mineralization/dyna-balance.tsx`:
- Around line 16-24: mineralisatie currently subtracts b_n_greenmanure,
b_n_fertilizer_artificial and b_n_fertilizer_organic from nitrogenBalance.b_nw
but omits b_n_fertilizer_preceeding, causing the breakdown to exceed total;
update the mineralisatie calculation in the dyna-balance component to also
subtract nitrogenBalance.b_n_fertilizer_preceeding (so mineralisatie =
nitrogenBalance.b_nw - b_n_greenmanure - b_n_fertilizer_artificial -
b_n_fertilizer_organic - b_n_fertilizer_preceeding) and verify totalBalance
(nitrogenBalance.b_nw - nitrogenBalance.b_n_uptake) remains unchanged.
In `@fdm-app/app/components/blocks/mineralization/dyna-chart.tsx`:
- Around line 275-278: The ChartContainer in dyna-chart.tsx is using a
non-standard Tailwind class "h-100" which may result in zero height; update the
ChartContainer's className (the one where config={dynaChartConfig}) to use a
valid height utility consistent with the other tab (e.g., replace "h-100 w-full"
with "h-[400px] w-full" or a theme-safe class like "h-96 w-full") so both tabs
render with an explicit, matching height.
In `@fdm-app/app/components/blocks/mineralization/dyna-field-list.tsx`:
- Around line 41-48: The code pairs fields and promises by index which can
misalign if the two fetches differ; update the data flow so each DynaTableRow
receives the promise keyed to its field.b_id instead of using promises[i].
Either (A) produce a single array of objects from the fetch step like { field,
promise } and map that array in the component, or (B) convert promises into a
lookup keyed by b_id (e.g., promisesById) and pass
promise={promisesById[field.b_id]} to DynaTableRow; update any code that
constructs promises and the DynaTableRow prop usage accordingly (refer to
fields, promises, DynaTableRow, and field.b_id).
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.mineralization._index.tsx:
- Around line 78-82: The code reads the "method" search param into method and
casts to NSupplyMethod without validation; update the logic around URL/request
parsing so the value is checked against the allowed NSupplyMethod variants
before casting (e.g., compare url.searchParams.get("method") to the enum or a
whitelist) and either fall back to the default "minip" or return a bad-request
response for unknown values; update the variable referenced as method and any
downstream uses (calculator/API path calls) to rely only on the
validated/normalized value.
- Around line 164-166: computeFarmAverageCurve currently treats each field
equally; change it to compute an area-weighted average so larger fields
contribute proportionally. Update the call sites (the one using validResults at
the top and the other block around lines 258-279) to pass per-result area (e.g.,
results[].area or results[].field.area) into computeFarmAverageCurve and modify
computeFarmAverageCurve to aggregate by DOY using weighted sums: for each DOY
compute sum(area * value) / sum(area) while skipping results with missing DOY
values; ensure total area fallback avoids division by zero. Also update any type
signatures and tests that assume unweighted averages (functions:
computeFarmAverageCurve, and callers referencing validResults/results).
- Around line 120-122: The catch block currently does "throw
handleLoaderError(error)" which can throw undefined for plain Error objects;
change it to call handleLoaderError(error) into a variable (e.g., const
loaderResponse = handleLoaderError(error)) and then if loaderResponse is truthy
throw loaderResponse, otherwise rethrow the original error (throw error) or
throw a new Error with context; update the catch in
farm.$b_id_farm.$calendar.mineralization._index.tsx to use this pattern so
handleLoaderError and the original Error are both handled safely.
- Around line 105-109: getDynaForFarm is currently awaited via use() as
asyncDynaPromises which lets top-level failures (missing NMI / batch fetch)
crash the whole content; wrap the call to getDynaForFarm in a try/catch (or
pre-resolve it without unwrapping the raw promise) so that if it rejects you
fall back to an empty array (or a per-field error marker) and surface the error
only in the DYNA section while still rendering the N-supply overview.
Concretely, update the asyncDynaPromises creation (where getDynaForFarm(...) is
used) to catch exceptions, store the error/empty result, and keep downstream
code that maps per-field promises unchanged so per-field DYNA errors remain
isolated from the N-supply overview.
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.mineralization.$b_id.dyna.tsx:
- Around line 207-222: The loop over cultivationHarvests dereferences
h.harvestable.harvestable_analyses[0] directly which can be null; update the
extraction of the analysis/yield to use defensive optional chaining and a safe
fallback (e.g., const analysis = h.harvestable?.harvestable_analyses?.[0] and
const yieldVal = analysis?.b_lu_yield ?? null) before computing label and
pushing to chartEvents, and ensure the date and label use safe defaults so
missing harvestable data won’t throw in the chartEvents.push.
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.mineralization.tsx:
- Around line 94-96: The catch block currently throws the result of
handleLoaderError(error) which can be undefined for plain Error instances;
change it to preserve the original error when normalization fails by calling
handleLoaderError(error) and throwing the normalized value if present, otherwise
rethrow the original error (e.g. const normalized = handleLoaderError(error);
throw normalized ?? error). Update the catch around the loader (the try/catch
that currently does "throw handleLoaderError(error)") to use this pattern so
unexpected errors like the "Invalid field data" Error are not turned into
undefined.
- Around line 41-42: The code reads params.b_id which doesn't exist; update all
uses of b_id (the const b_id = params.b_id and later references) to use the
actual route param name params.b_id_farm (or rename the route param to $b_id if
you prefer) and ensure the loader/action components that reference b_id
consistently use params.b_id_farm (or add a clear fallback guard e.g., throw or
redirect if the expected param is missing) so no undefined value is propagated
into functions like the loader or any functions that use b_id.
In `@fdm-app/package.json`:
- Line 14: Remove the invalid test script from fdm-app's package.json (the
"test": "vitest run" entry) or move it into the fdm-calculator package.json
where vitest is declared; specifically, update the package.json in the fdm-app
package to delete the "test" script entry (or add it to fdm-calculator instead)
and ensure only the package that owns the test files lists vitest in
devDependencies so running pnpm --filter `@nmi-agro/fdm-app` test will not fail.
In `@fdm-calculator/src/mineralisatie/builders.ts`:
- Around line 327-335: The amendments array currently includes every fertilizer
with a p_date; update the filter to only include applications whose p_date falls
in the requested calendar year (e.g., check p_date.getFullYear() ===
requestedYear) before mapping to p_id, p_dose, p_app_method and
p_date_fertilization; modify the fertilizers.filter(...) used to build
amendments to also compare p_date.getFullYear() to the caller-provided year
(requestedYear/targetYear) so only that year's applications are attached to
amendments.
- Around line 337-418: The rotation building currently maps only [year], so
earlier cultivation years are omitted; change the source array for rotation to
include all relevant years from cultivations (e.g., collect distinct years from
cultivations' b_lu_start/b_lu_end that intersect up to the target year, include
the target year, sort in chronological order) and then map that years array
instead of [year]; keep the rest of the logic (getMainCultivation, harvests,
greenManure, irrigation) the same and preserve the existing condition that
amendments are included only when rotationYear === year.
In `@fdm-calculator/src/mineralisatie/dyna.ts`:
- Around line 71-81: The POST to "https://api.nmi-agro.nl/bemestingsplan/dyna"
currently uses fetch without timeout; wrap the fetch in an AbortController and
enforce a configurable timeout (e.g., via a constant or env var) so the request
using nmiApiKey and requestBody is aborted after the deadline; also add and
standardize an explicit NMI API version indicator (either an "NMI-API-Version"
header or a version query param) and apply the same change to the other two
endpoints ("bemestingsplan/nsupply" and "bemestingsplan/nutrients") so version
pinning is consistent across calls.
In `@fdm-calculator/src/mineralisatie/nsupply.ts`:
- Around line 63-103: The fetch to
"https://api.nmi-agro.nl/bemestingsplan/nsupply" needs an application-level
timeout and JSON parse errors must be converted into the same "Ongeldig antwoord
van NMI API" flow; implement an AbortController with a configurable timeout
(setTimeout -> controller.abort(), clearTimeout on success) around the fetch and
map an AbortError to a NmiApiError (e.g. 408 or a descriptive message) so the
call cannot hang, and wrap the await response.json() in a try/catch around the
area that feeds nsupplyResponseSchema.safeParse — on JSON parse failure throw
the same Error currently thrown for invalid schema (using
JSON.stringify(z.treeifyError(...)) style) so malformed JSON produces a
consistent "Ongeldig antwoord van NMI API" error instead of an unhandled
SyntaxError.
---
Outside diff comments:
In `@fdm-app/app/components/blocks/sidebar/apps.tsx`:
- Around line 97-106: Add a new mineralizationLink in the same pattern as
omBalanceLink: declare let mineralizationLink: string | undefined and set it to
undefined when isCreateFarmWizard or isFarmOverview is true, otherwise if farmId
&& farmId !== "undefined" set mineralizationLink =
`/farm/${farmId}/${selectedCalendar}/mineralization` (else undefined). Then
render a SidebarMenuItem for mineralizationLink alongside the existing
SidebarMenuItem entries (the same place you render
omBalanceLink/Atlas/Bemestingsadvies/Gebruiksruimte) so the Mineralisatie app
appears in the sidebar; use the same visibility checks and props as the other
menu items.
---
Minor comments:
In `@fdm-app/app/components/blocks/mineralization/dyna-advice.tsx`:
- Around line 17-25: The formatDate function currently constructs a Date via new
Date(dateStr) which treats "YYYY-MM-DD" as UTC and can produce an off-by-one day
in local time; update formatDate to parse ISO date-only strings explicitly
(e.g., split "YYYY-MM-DD" into year, month, day and construct new Date(year,
month-1, day) so it uses local time) while preserving the existing fallback
behavior (keep the Number.isNaN(d.getTime()) check and return the original
string if parsing fails); if the input contains a time component (contains 'T'
or timezone) still allow Date to parse it normally. Use the function name
formatDate to locate and change the logic accordingly.
In `@fdm-app/app/components/blocks/mineralization/dyna-chart.tsx`:
- Around line 113-129: In EventDot, don't treat x===0 as falsy and don't assume
events[0] exists: change the viewBox check to test viewBox?.x === undefined (or
viewBox?.x == null) instead of !viewBox?.x so zero is allowed, and add a small
guard like if (!events || events.length === 0) return null; when resolving color
use a safe lookup with a fallback (e.g., const color =
EVENT_COLORS[events[0].type] ?? DEFAULT_EVENT_COLOR) to avoid throwing if events
is empty or an unknown type; keep the existing y calculation using viewBox.y
fallback.
In `@fdm-app/app/components/blocks/mineralization/dyna-field-list.tsx`:
- Around line 102-110: The status column uses icon-only SVGs (CircleX /
CircleCheck) and relies on title which isn't a reliable accessible name; wrap
each icon in an explicit label (e.g., a visually-hidden span like "Error:
{error}" or "Success") and hide the SVG from AT by setting aria-hidden on
CircleX and CircleCheck and removing the title prop; update the TableCell
rendering that checks error to include the sr-only label and aria-hidden on the
SVGs so screen readers get a clear text status while the visuals remain the
same.
In `@fdm-app/app/components/blocks/mineralization/field-list.tsx`:
- Around line 80-85: The error indicator currently only uses a title
(result.error) which isn't accessible; update the block that renders CircleX in
field-list.tsx (where result.error is checked) to provide an explicit accessible
name and mark the SVG decorative: add a screen-reader-only visible text node
(e.g., a span with the error message like "Error: {result.error}") for assistive
tech, set the CircleX SVG to aria-hidden="true" (or role="img" with an
appropriate aria-label removed) so it does not get announced twice, and keep the
title for hover — ensure the accessible text conveys the error and is
keyboard/screen-reader reachable.
- Around line 19-26: getCurrentDoy currently normalizes to fractions of a day
and uses Math.ceil(...) + 1 which yields 2 on Jan 1; update getCurrentDoy to
normalize both now and startOfYear to calendar midnight (setHours(0,0,0,0)),
compute the whole-day difference in milliseconds divided by msPerDay, then use
Math.floor(...) + 1 to return 1 for January 1; locate and change the
implementation in the getCurrentDoy function accordingly.
In `@fdm-app/app/components/blocks/mineralization/leaching-chart.tsx`:
- Around line 40-49: The functions formatDateTick and formatDateLabel currently
use new Date(dateStr) which treats "YYYY-MM-DD" as UTC and causes timezone
shifts; modify both to detect date-only strings (e.g., /^\d{4}-\d{2}-\d{2}$/),
parse into year/month/day parts and construct the Date with the local
constructor new Date(year, monthIndex, day) (use monthIndex = month-1), then
keep the existing invalid-date fallback and existing return values
(MONTH_LABELS_NL[d.getMonth()] in formatDateTick and
d.toLocaleDateString("nl-NL", { day: "numeric", month: "long" }) in
formatDateLabel).
In `@fdm-app/app/components/blocks/mineralization/mineralization-chart.tsx`:
- Around line 54-60: getCurrentDoy currently overcounts by 1 because it uses new
Date(year,0,1) + Math.ceil(...) + 1; update the getCurrentDoy function in
mineralization-chart.tsx to compute start = new Date(now.getFullYear(), 0, 0)
and return Math.floor((now.getTime() - start.getTime()) / (1000 * 60 * 60 *
24)); remove the duplicate local getCurrentDoy in nsupply-kpi.tsx and import
this fixed exported helper instead, and update the _index loader’s DOY
computation to the same start/new-floor formula so all consumers
(FarmMineralizationChart, FieldMineralizationChart and KPIs) use the consistent,
correct DOY.
In `@fdm-app/app/components/blocks/mineralization/skeletons.tsx`:
- Around line 79-85: The two child columns currently use global col-span classes
which apply below the lg breakpoint and conflict with the md:grid-cols-2 layout;
update the span classes for the MineralizationChartSkeleton and
MineralizationFieldsSkeleton containers to be breakpoint-scoped (e.g., change
col-span-4 to lg:col-span-4 and col-span-3 to lg:col-span-3) so the spans only
take effect at lg and above while allowing the md fallback grid to work
correctly.
In `@fdm-app/app/integrations/mineralization.server.ts`:
- Around line 598-605: generateInsights currently calls doyToDateString with new
Date().getFullYear(), which hardcodes the current calendar year and yields
incorrect dates for non-current calendar views; change generateInsights (in
fdm-app/app/integrations/mineralization.server.ts) to accept a year parameter
(e.g., year: number) and pass that to doyToDateString instead of new
Date().getFullYear(), compute date via doyToDateString(currentDoy, new
Date(year, 0).getFullYear()), and update the caller in
farm.$b_id_farm.$calendar.mineralization.$b_id._index.tsx to decide whether to
call generateInsights for non-current years or pass the calendar year when
generating insights. Ensure all references to generateInsights and the created
date variable are updated to use the new parameter.
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.mineralization.$b_id._index.tsx:
- Around line 166-174: The loader computes DOY with a different formula than the
chart and KPI components causing mismatched “Vandaag”/insight dates; centralize
DOY logic by exporting a single getCurrentDoy helper from
mineralization-chart.tsx (fix the chart-side formula to the correct definition
while you’re there), then replace the local Math.floor-based calculation in the
loader (the currentDoy used when calling generateInsights) and any DOY code in
nsupply-kpi.tsx to import and call that shared getCurrentDoy; ensure
generateInsights continues to receive the same DOY value so chart, KPI, and
insights are consistent.
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.mineralization.$b_id.dyna.tsx:
- Around line 365-372: The code uses UTC date for "today" which can misalign
with farm-local calendar; change the today calculation from new
Date().toISOString().split("T")[0] to a local-date string (e.g. new
Date().toLocaleDateString('en-CA') which yields YYYY-MM-DD in local time) so the
lookup for todayPoint (yearData.find(...)) uses the farm's local day; update the
same pattern where "today" is computed in
farm.$b_id_farm.$calendar.mineralization.$b_id.dyna.tsx (variables: today,
todayPoint, currentNAvailability, currentUptake, totalLeaching, yearData) and
also in dyna-chart.tsx (the analogous today/todayPoint logic) to use the
local-date formatter.
---
Nitpick comments:
In `@fdm-app/app/components/blocks/mineralization/dyna-chart.tsx`:
- Around line 218-229: The prop fertilizingRecommendations declared on
DynaChartProps is unused; update DynaChart to consume fertilizingRecommendations
(or remove it from the interface/call site). Preferred fix: accept
fertilizingRecommendations in the DynaChart parameter list and render a visual
marker for its b_date_recommended (e.g., add a Recharts ReferenceLine or
ReferenceDot tied to fertilizingRecommendations.b_date_recommended with a
label/tooltip) so the recommendation date appears on the chart; ensure you
reference the DynaChart component, DynaChartProps, fertilizingRecommendations
and the b_date_recommended field when implementing the marker.
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.mineralization.$b_id._index.tsx:
- Around line 96-127: The loader is redundantly calling getCurrentSoilData and
then calling getNSupplyForField three times which each re-fetch getField,
getCurrentSoilData, and getCultivations; to fix, change getNSupplyForField to
accept optional pre-fetched inputs (e.g., parameters named field, soilDataArray,
cultivations) and update its implementation to use these when provided and only
call getField/getCurrentSoilData/getCultivations as a fallback; then in the
loader (where you already call getCurrentSoilData and derive
organicMatter/soilType/completeness) pass those pre-fetched values into each
getNSupplyForField invocation so the extra duplicate fetches are avoided
(references: getNSupplyForField, getCurrentSoilData, getField, getCultivations,
assessDataCompleteness).
In `@fdm-calculator/src/mineralisatie/schemas.ts`:
- Around line 33-35: The nsupplyResponseSchema currently allows an empty array;
update the array validator on nsupplyResponseSchema to require at least one
element by adding a .min(1) call to the z.array(nsupplyDataPointSchema)
definition so the parser fails on empty responses (refer to
nsupplyResponseSchema and nsupplyDataPointSchema to locate the change).
🪄 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: 55d579ed-e794-432f-b556-fd0faeea454a
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (32)
fdm-app/app/components/blocks/header/mineralization.tsxfdm-app/app/components/blocks/mineralization/data-completeness.tsxfdm-app/app/components/blocks/mineralization/dyna-advice.tsxfdm-app/app/components/blocks/mineralization/dyna-balance.tsxfdm-app/app/components/blocks/mineralization/dyna-chart.tsxfdm-app/app/components/blocks/mineralization/dyna-field-list.tsxfdm-app/app/components/blocks/mineralization/field-list.tsxfdm-app/app/components/blocks/mineralization/leaching-chart.tsxfdm-app/app/components/blocks/mineralization/method-selector.tsxfdm-app/app/components/blocks/mineralization/mineralization-chart.tsxfdm-app/app/components/blocks/mineralization/nsupply-kpi.tsxfdm-app/app/components/blocks/mineralization/skeletons.tsxfdm-app/app/components/blocks/sidebar/apps.tsxfdm-app/app/components/blocks/sidebar/farm.tsxfdm-app/app/integrations/mineralization.server.tsfdm-app/app/routes/farm.$b_id_farm.$calendar.mineralization.$b_id._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.mineralization.$b_id.dyna.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.mineralization.$b_id.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.mineralization._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.mineralization.tsxfdm-app/package.jsonfdm-calculator/package.jsonfdm-calculator/src/index.tsfdm-calculator/src/mineralisatie/assessment.tsfdm-calculator/src/mineralisatie/builders.test.tsfdm-calculator/src/mineralisatie/builders.tsfdm-calculator/src/mineralisatie/dyna.tsfdm-calculator/src/mineralisatie/errors.tsfdm-calculator/src/mineralisatie/index.tsfdm-calculator/src/mineralisatie/nsupply.tsfdm-calculator/src/mineralisatie/schemas.tsfdm-calculator/src/mineralisatie/types.d.ts
Comment on lines
+16
to
+24
| // Mineralisatie = b_nw - b_n_greenmanure - (artificial + organic fertilizer) | ||
| const mineralisatie = | ||
| nitrogenBalance.b_nw - | ||
| nitrogenBalance.b_n_greenmanure - | ||
| nitrogenBalance.b_n_fertilizer_artificial - | ||
| nitrogenBalance.b_n_fertilizer_organic | ||
|
|
||
| // Total balance = b_nw - b_n_uptake | ||
| const totalBalance = nitrogenBalance.b_nw - nitrogenBalance.b_n_uptake |
Contributor
There was a problem hiding this comment.
Subtract b_n_fertilizer_preceeding from the derived mineralisatie row.
The card displays “Voorvrucht” as a positive contribution, but the mineralisatie back-calculation does not remove it from b_nw. That makes the visible breakdown exceed “Totaal aanbod” whenever the preceding-crop contribution is non-zero.
🐛 Proposed fix
const mineralisatie =
nitrogenBalance.b_nw -
nitrogenBalance.b_n_greenmanure -
+ nitrogenBalance.b_n_fertilizer_preceeding -
nitrogenBalance.b_n_fertilizer_artificial -
nitrogenBalance.b_n_fertilizer_organic📝 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
| // Mineralisatie = b_nw - b_n_greenmanure - (artificial + organic fertilizer) | |
| const mineralisatie = | |
| nitrogenBalance.b_nw - | |
| nitrogenBalance.b_n_greenmanure - | |
| nitrogenBalance.b_n_fertilizer_artificial - | |
| nitrogenBalance.b_n_fertilizer_organic | |
| // Total balance = b_nw - b_n_uptake | |
| const totalBalance = nitrogenBalance.b_nw - nitrogenBalance.b_n_uptake | |
| // Mineralisatie = b_nw - b_n_greenmanure - (artificial + organic fertilizer) | |
| const mineralisatie = | |
| nitrogenBalance.b_nw - | |
| nitrogenBalance.b_n_greenmanure - | |
| nitrogenBalance.b_n_fertilizer_preceeding - | |
| nitrogenBalance.b_n_fertilizer_artificial - | |
| nitrogenBalance.b_n_fertilizer_organic | |
| // Total balance = b_nw - b_n_uptake | |
| const totalBalance = nitrogenBalance.b_nw - nitrogenBalance.b_n_uptake |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fdm-app/app/components/blocks/mineralization/dyna-balance.tsx` around lines
16 - 24, mineralisatie currently subtracts b_n_greenmanure,
b_n_fertilizer_artificial and b_n_fertilizer_organic from nitrogenBalance.b_nw
but omits b_n_fertilizer_preceeding, causing the breakdown to exceed total;
update the mineralisatie calculation in the dyna-balance component to also
subtract nitrogenBalance.b_n_fertilizer_preceeding (so mineralisatie =
nitrogenBalance.b_nw - b_n_greenmanure - b_n_fertilizer_artificial -
b_n_fertilizer_organic - b_n_fertilizer_preceeding) and verify totalBalance
(nitrogenBalance.b_nw - nitrogenBalance.b_n_uptake) remains unchanged.
Comment on lines
+337
to
+418
| const rotation: Record<string, unknown>[] = [year] | ||
| .map((rotationYear) => { | ||
| const yearStart = new Date(rotationYear, 0, 1) | ||
| const yearEnd = new Date(rotationYear, 11, 31, 23, 59, 59, 999) | ||
| const yearCultivations = cultivations.filter( | ||
| (c) => | ||
| c.b_lu_catalogue && | ||
| c.b_lu_start != null && | ||
| c.b_lu_start <= yearEnd && | ||
| (c.b_lu_end == null || c.b_lu_end >= yearStart), | ||
| ) | ||
|
|
||
| // Select main crop using May 15th rule | ||
| const mainCrop = getMainCultivation(yearCultivations, rotationYear) | ||
|
|
||
| if (!mainCrop?.b_lu_catalogue) return null | ||
|
|
||
| // Look up yield from crop_properties for the harvests array | ||
| const cropProp = cropProperties?.find( | ||
| (cp) => cp.b_lu_catalogue === mainCrop.b_lu_catalogue, | ||
| ) | ||
|
|
||
| // Prefer actual harvest records; fall back to inferring a single | ||
| // harvest from b_lu_end when no records are available. | ||
| const actualHarvestRecords = | ||
| mainCrop.b_lu && harvestsByBlu | ||
| ? (harvestsByBlu.get(mainCrop.b_lu) ?? []) | ||
| : [] | ||
| const harvests = | ||
| actualHarvestRecords.length > 0 | ||
| ? actualHarvestRecords | ||
| .filter((h) => h.b_lu_harvest_date != null) | ||
| .map((h) => ({ | ||
| b_date_harvest: h.b_lu_harvest_date! | ||
| .toISOString() | ||
| .split("T")[0], | ||
| ...(h.b_lu_yield != null | ||
| ? { b_lu_yield: h.b_lu_yield } | ||
| : cropProp?.b_lu_yield != null | ||
| ? { b_lu_yield: cropProp.b_lu_yield } | ||
| : {}), | ||
| })) | ||
| : mainCrop.b_lu_end | ||
| ? [ | ||
| { | ||
| b_date_harvest: mainCrop.b_lu_end | ||
| .toISOString() | ||
| .split("T")[0], | ||
| ...(cropProp?.b_lu_yield != null | ||
| ? { b_lu_yield: cropProp.b_lu_yield } | ||
| : {}), | ||
| }, | ||
| ] | ||
| : [] | ||
|
|
||
| // Catchcrop becomes green manure on the same rotation entry | ||
| const greenManure = yearCultivations.find( | ||
| (c) => c.b_lu_croprotation === "catchcrop" && c !== mainCrop, | ||
| ) | ||
|
|
||
| return { | ||
| year: rotationYear, | ||
| b_lu: mainCrop.b_lu_catalogue, | ||
| b_lu_start: mainCrop.b_lu_start?.toISOString().split("T")[0], | ||
| harvests, | ||
| ...(mainCrop.m_cropresidue != null | ||
| ? { m_cropresidue: mainCrop.m_cropresidue } | ||
| : {}), | ||
| ...(greenManure?.b_lu_catalogue | ||
| ? { | ||
| b_lu_green: greenManure.b_lu_catalogue, | ||
| b_date_green_incorporation: greenManure.b_lu_end | ||
| ?.toISOString() | ||
| .split("T")[0], | ||
| } | ||
| : {}), | ||
| irrigation: [], | ||
| // Amendments only on the current calendar year | ||
| amendments: rotationYear === year ? amendments : [], | ||
| } | ||
| }) | ||
| .filter((entry) => entry !== null) |
Contributor
There was a problem hiding this comment.
Build DYNA rotation entries for all relevant cultivation years.
The code maps only [year], so preceding cultivation years are never sent even though the function docs and DYNA balance model rely on rotation history. This can skew carry-over and green-manure effects.
Include historical rotation years
- const rotation: Record<string, unknown>[] = [year]
+ const rotationYears = Array.from(
+ new Set([
+ year,
+ ...cultivations
+ .map((c) => c.b_lu_start?.getFullYear())
+ .filter((y): y is number => y !== undefined && y <= year),
+ ]),
+ ).sort((a, b) => a - b)
+
+ const rotation: Record<string, unknown>[] = rotationYears
.map((rotationYear) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fdm-calculator/src/mineralisatie/builders.ts` around lines 337 - 418, The
rotation building currently maps only [year], so earlier cultivation years are
omitted; change the source array for rotation to include all relevant years from
cultivations (e.g., collect distinct years from cultivations'
b_lu_start/b_lu_end that intersect up to the target year, include the target
year, sort in chronological order) and then map that years array instead of
[year]; keep the rest of the logic (getMainCultivation, harvests, greenManure,
irrigation) the same and preserve the existing condition that amendments are
included only when rotationYear === year.
Comment on lines
+71
to
+81
| const response = await fetch( | ||
| "https://api.nmi-agro.nl/bemestingsplan/dyna", | ||
| { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| Authorization: `Bearer ${nmiApiKey}`, | ||
| }, | ||
| body: JSON.stringify(requestBody), | ||
| }, | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect NMI API calls and nearby headers/query params for an explicit version pin.
# Expected: Each bemestingsplan NMI call includes the agreed API/model version header or query parameter.
rg -n -C4 'api\.nmi-agro\.nl|bemestingsplan|Api-Version|api-version|version|calculatorVersion'Repository: nmi-agro/fdm
Length of output: 50369
🏁 Script executed:
cat -n fdm-calculator/src/mineralisatie/dyna.ts | head -120Repository: nmi-agro/fdm
Length of output: 5498
🏁 Script executed:
rg -n "bemestingsplan|nmi-agro\.nl" --type ts fdm-calculator/src/ -A 10 -B 2Repository: nmi-agro/fdm
Length of output: 19470
🏁 Script executed:
# Search for any references to version pinning in comments, PRs, or docs
rg -i "version.*pin|api.*version|model.*version" fdm-calculator/src/mineralisatie/ --type ts -B 2 -A 2 | head -80Repository: nmi-agro/fdm
Length of output: 38
🏁 Script executed:
# Check nutrient-advice for any version-related patterns or comments
grep -r "version" fdm-calculator/src/nutrient-advice/ --include="*.ts" -i -B 2 -A 2Repository: nmi-agro/fdm
Length of output: 1051
🏁 Script executed:
# Search for NMI API documentation or version references in the entire repo
rg -i "nmi.*version|api.*version|model.*version|v1|api-v" --type ts --type md --type json -B 1 -A 1 | head -100Repository: nmi-agro/fdm
Length of output: 6205
🏁 Script executed:
cat fdm-calculator/package.json | head -30Repository: nmi-agro/fdm
Length of output: 902
Add fetch timeout and clarify NMI API version pinning strategy.
The fetch call lacks a timeout, risking indefinite hangs on stalled upstream requests. Additionally, while version pinning the NMI API endpoint is a sound practice to prevent silent schema drift, the codebase shows no evidence of what the specific version header or query parameter should be. Confirm the NMI-supported version format with your API provider or team, then apply it consistently across all three NMI endpoints (bemestingsplan/dyna, bemestingsplan/nsupply, bemestingsplan/nutrients).
🛡️ Timeout hardening template
- const response = await fetch(
- "https://api.nmi-agro.nl/bemestingsplan/dyna",
- {
- method: "POST",
- headers: {
- "Content-Type": "application/json",
- Authorization: `Bearer ${nmiApiKey}`,
- },
- body: JSON.stringify(requestBody),
- },
- )
+ const controller = new AbortController()
+ const timeoutId = setTimeout(() => controller.abort(), 30_000)
+
+ let response: Response
+ try {
+ response = await fetch(
+ "https://api.nmi-agro.nl/bemestingsplan/dyna",
+ {
+ method: "POST",
+ headers: {
+ "Content-Type": "application/json",
+ Authorization: `Bearer ${nmiApiKey}`,
+ // Confirm NMI version header format and add here
+ },
+ body: JSON.stringify(requestBody),
+ signal: controller.signal,
+ },
+ )
+ } catch (error) {
+ if ((error as Error).name === "AbortError") {
+ throw new NmiApiError(503, "NMI API reageert niet op tijd.")
+ }
+ throw error
+ } finally {
+ clearTimeout(timeoutId)
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fdm-calculator/src/mineralisatie/dyna.ts` around lines 71 - 81, The POST to
"https://api.nmi-agro.nl/bemestingsplan/dyna" currently uses fetch without
timeout; wrap the fetch in an AbortController and enforce a configurable timeout
(e.g., via a constant or env var) so the request using nmiApiKey and requestBody
is aborted after the deadline; also add and standardize an explicit NMI API
version indicator (either an "NMI-API-Version" header or a version query param)
and apply the same change to the other two endpoints ("bemestingsplan/nsupply"
and "bemestingsplan/nutrients") so version pinning is consistent across calls.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (2)
fdm-app/app/components/blocks/mineralization/dyna-field-list.tsx (1)
27-33:⚠️ Potential issue | 🟠 MajorKey the DYNA promises before they reach this component.
This still derives
fieldId -> promisefrom array position, so the misalignment risk remains iffieldsandpromisesdiffer in order or count. Pass a keyed model instead, e.g.{ b_id, promise }[], and build the map from that identifier.🐛 Proposed direction
interface DynaFieldListProps { fields: { b_id: string; b_name: string | null }[] - promises: Promise<FarmDynaResult>[] + promises: { b_id: string; promise: Promise<FarmDynaResult> }[] b_id_farm: string calendar: string } @@ - const promisesById = new Map<string, Promise<FarmDynaResult>>() - for (let i = 0; i < fields.length; i++) { - const fieldId = fields[i].b_id - if (i < promises.length) { - promisesById.set(fieldId, promises[i]) - } - } + const promisesById = new Map( + promises.map(({ b_id, promise }) => [b_id, promise]), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/mineralization/dyna-field-list.tsx` around lines 27 - 33, The current build of promisesById in dyna-field-list.tsx still pairs promises to fields by array index (using fields and promises arrays), which can misalign; change the component API to accept a keyed collection (e.g. an array of items shaped { b_id, promise: Promise<FarmDynaResult> } or a Map<string, Promise<FarmDynaResult>>) and then construct promisesById directly from that identifier rather than relying on index alignment; update references to the promises prop and the creation of promisesById (variables: promises, fields, promisesById) so the map is populated by the provided b_id key for each promise.fdm-calculator/src/mineralisatie/builders.ts (1)
342-425:⚠️ Potential issue | 🟠 MajorDYNA rotation still omits historical cultivation years.
The builder still maps only
[year], while the integration explicitly fetches all cultivations so previous rotation years can influence carry-over effects. Build rotation entries for all relevant years up to the requested year, then attach amendments only to the requested year.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/mineralisatie/builders.ts` around lines 342 - 425, The rotation builder currently maps only [year], omitting prior cultivation years; change the rotation generation in builders.ts (the rotation variable creation) to compute all relevant calendar years from the cultivations set (e.g., derive min year from cultivations' b_lu_start and include any years up to the requested year) and map over that full year range to produce rotation entries (keep the existing logic in getMainCultivation, harvests, greenManure, etc.), ensuring amendments remain attached only when rotationYear === year. Use the cultivations array and their b_lu_start/b_lu_end to build the years list (and deduplicate/sort) so historical years are included for carry-over effects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fdm-app/app/components/blocks/mineralization/nsupply-kpi.tsx`:
- Around line 24-28: The avgN calculation currently averages per-field
totalAnnualN equally; change it to an area-weighted average by computing
sum(validResults.map(r => r.totalAnnualN * r.area)) divided by
sum(validResults.map(r => r.area)), handling the case of zero total area to
fallback to 0; update the avgN assignment (referencing avgN, validResults,
totalAnnualN, and area) to use this weighted formula so larger fields contribute
proportionally.
- Around line 248-284: The displayed "Methoden" count uses the hardcoded total 3
(expressed as {3 - errorCount} / 3) which is wrong when fewer than three results
are passed; instead compute the successful count directly from results and use
the actual number of input results as the denominator. Replace the {3 -
errorCount} expression with a computed successfulCount = results.filter(r =>
!r.error).length and render "{successfulCount} / {results.length}" in the
CardContent (update the place where {3 - errorCount} / 3 appears), leaving the
rest (the METHOD_LABELS mapping and the results.filter(...).map(...).join(...))
unchanged.
In `@fdm-app/app/integrations/mineralization.server.ts`:
- Around line 458-563: The per-field async work currently spawned by
nonBufferFields.map(async ...) fires all DYNA requests at once; limit
concurrency (e.g., use p-limit or a simple semaphore/pool) around the async
mapper so only N requests run concurrently (choose a small default like 4–8).
Wrap the body that builds fieldSoilData, dynaFertilizers, requestBody (uses
buildDynaRequest) and the call to getDyna in the limited task runner instead of
mapping directly, preserving the same return shape (b_id, b_name, result/error).
Ensure the limiter is created once per farm execution scope and used for each
field task so cold-cache farms don’t fan out unlimited simultaneous getDyna
calls.
- Around line 467-479: The pre-flight check currently only ensures
harvests.length > 0 for ongoingMainCrops but must require a real harvest date;
update the loop that iterates ongoingMainCrops (using fieldCultivations,
ongoingMainCrops, harvestsMap and crop.b_lu) to verify that at least one harvest
record has a non-null b_lu_harvest_date (e.g. use Array.some on
harvestsMap.get(crop.b_lu)) and throw the existing error ("Oogstdatum ontbreekt
voor lopend gewas") if no harvest with a b_lu_harvest_date is found.
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.mineralization._index.tsx:
- Around line 284-304: The loop currently sets area = result.area || 0 which
zeroes weights when area is missing, producing a flat zero curve; change the
aggregation in the results loop (the area variable, doyMap entries, and the
existing record structure) to also track unweightedSum and count (e.g.,
sumUnweighted and count) when area is null/undefined or <=0, only add to
sumWeighted/sumArea when area > 0, and then in the final map where
d_n_supply_actual is computed, return sumWeighted/sumArea when sumArea > 0,
otherwise return sumUnweighted/count as an unweighted average (handle count==0
safely). This preserves weighted averages when areas exist and falls back to an
unweighted average when areas are missing.
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.mineralization.$b_id._index.tsx:
- Around line 197-199: The catch block currently does "throw
handleLoaderError(error)" which can throw undefined if the normalization returns
nothing; change it to capture the normalized error first (e.g., const normalized
= handleLoaderError(error)) and then throw the normalized error if present,
otherwise rethrow the original error (throw normalized ?? error). Apply this
change in the loader for
farm.$b_id_farm.$calendar.mineralization.$b_id._index.tsx so it mirrors the safe
pattern used in the farm overview loader and references handleLoaderError
consistently.
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.mineralization.$b_id.dyna.tsx:
- Around line 247-249: In the catch block that currently does "throw
handleLoaderError(error)", wrap the call to handleLoaderError(error) so you
preserve the original error if normalization fails: call
handleLoaderError(error) inside a try and if it returns a normalized error throw
that, but if handleLoaderError itself throws or returns an unexpected/undefined
value then throw the original caught error; reference the existing catch scope
and the handleLoaderError function to implement this fallback.
- Around line 362-370: The code currently uses todayPoint for KPI labels even
when viewing future calendar years; change the logic so todayPoint is only
computed/used when isCurrentYear is true: compute today = new
Date().toLocaleDateString("en-CA") as before, but set todayPoint to the find
result only when isCurrentYear (e.g. todayPoint = isCurrentYear ?
yearData.find(d => d.b_date_calculation >= today) : undefined) and ensure
currentNAvailability and currentUptake fall back to lastPoint when todayPoint is
undefined; update references to todayPoint, currentNAvailability, and
currentUptake accordingly so future years never use a “today” value.
- Around line 130-141: The current pre-flight loop only checks that harvestsMap
has any records for crop.b_lu but not that those records include a dated
harvest; change the condition inside the loop that currently uses "if
(harvests.length === 0)" to instead verify there is at least one harvest with a
harvest date (e.g. check harvests.some(h => h.b_lu_harvest_date or
h.b_date_harvest) and treat as missing if none). Update the branch that returns
missingHarvestDate (which returns { missingHarvestDate: true, field, b_id,
b_id_farm, calendar: params.calendar ?? "" }) to trigger when no dated harvest
exists for the crop identified by ongoingMainCrops / crop.b_lu using the
harvestsMap lookup.
In `@fdm-calculator/src/mineralisatie/builders.ts`:
- Around line 49-69: The current activeOnMay15 lookup can return a catchcrop
that spans targetDate; change the logic to first look for any cultivation that
spans targetDate and is not a catchcrop, and only if none found fall back to a
spanning catchcrop, then to the existing non-spanning fallbacks. Concretely:
perform two searches on cultivations using b_lu_start and b_lu_end against
targetDate—first find(c => spans && c.b_lu_croprotation !== "catchcrop") then
find(c => spans) —and assign that result to activeOnMay15 (then keep the
existing fallback of first non-catchcrop in year, then any crop). Reference
functions/values: activeOnMay15, cultivations, b_lu_start, b_lu_end,
b_lu_croprotation, targetDate.
- Around line 123-139: The code extracts b_lu_brp from the first entry in
cultivations regardless of year or crop type, which can pick historical or catch
crops; change the logic in the b_lu_brp computation to first filter cultivations
for the requested timeframe and for the main crop before parsing b_lu_catalogue
(e.g. filter by c.year matching timeframe and by a main-crop flag such as
c.mainCrop / c.isMainCrop), then map/parse the catalogue string as currently
done and take the first valid parsed value; update the logic around the b_lu_brp
variable to reflect this filtered selection.
In `@fdm-calculator/src/mineralisatie/nsupply.ts`:
- Around line 178-183: getNSupply is created via
withCalculationCache(requestNSupply, "requestNSupply", pkg.calculatorVersion,
["nmiApiKey"]) but there is no TTL enforcement so cached results never expire;
update withCalculationCache signature to accept an optional ttl (e.g., in
seconds or ms) and propagate it into getCachedCalculation, then in
getCachedCalculation check the cached entry's created_at against now and
reject/refresh the cache when age > ttl (use the created_at timestamp parse and
a consistent timezone/epoch comparison); modify the call that creates getNSupply
to pass a 24h ttl (86400s or 24*60*60*1000ms depending on units) and ensure
tests cover expiration behavior — alternatively, if you prefer not to change
runtime behavior, update documentation/comments for withCalculationCache and the
PR objective to state caching is only invalidated by request/version changes.
---
Duplicate comments:
In `@fdm-app/app/components/blocks/mineralization/dyna-field-list.tsx`:
- Around line 27-33: The current build of promisesById in dyna-field-list.tsx
still pairs promises to fields by array index (using fields and promises
arrays), which can misalign; change the component API to accept a keyed
collection (e.g. an array of items shaped { b_id, promise:
Promise<FarmDynaResult> } or a Map<string, Promise<FarmDynaResult>>) and then
construct promisesById directly from that identifier rather than relying on
index alignment; update references to the promises prop and the creation of
promisesById (variables: promises, fields, promisesById) so the map is populated
by the provided b_id key for each promise.
In `@fdm-calculator/src/mineralisatie/builders.ts`:
- Around line 342-425: The rotation builder currently maps only [year], omitting
prior cultivation years; change the rotation generation in builders.ts (the
rotation variable creation) to compute all relevant calendar years from the
cultivations set (e.g., derive min year from cultivations' b_lu_start and
include any years up to the requested year) and map over that full year range to
produce rotation entries (keep the existing logic in getMainCultivation,
harvests, greenManure, etc.), ensuring amendments remain attached only when
rotationYear === year. Use the cultivations array and their b_lu_start/b_lu_end
to build the years list (and deduplicate/sort) so historical years are included
for carry-over effects.
🪄 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: 59328c76-535d-46c5-b125-6ced8ac8fcee
📒 Files selected for processing (20)
fdm-app/app/components/blocks/header/mineralization.tsxfdm-app/app/components/blocks/mineralization/dyna-advice.tsxfdm-app/app/components/blocks/mineralization/dyna-balance.tsxfdm-app/app/components/blocks/mineralization/dyna-chart.tsxfdm-app/app/components/blocks/mineralization/dyna-field-list.tsxfdm-app/app/components/blocks/mineralization/field-list.tsxfdm-app/app/components/blocks/mineralization/leaching-chart.tsxfdm-app/app/components/blocks/mineralization/mineralization-chart.tsxfdm-app/app/components/blocks/mineralization/nsupply-kpi.tsxfdm-app/app/components/blocks/mineralization/skeletons.tsxfdm-app/app/integrations/mineralization.server.tsfdm-app/app/routes/farm.$b_id_farm.$calendar.mineralization.$b_id._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.mineralization.$b_id.dyna.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.mineralization._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.mineralization.tsxfdm-calculator/src/mineralisatie/builders.tsfdm-calculator/src/mineralisatie/dyna.tsfdm-calculator/src/mineralisatie/nsupply.tsfdm-calculator/src/mineralisatie/schemas.tsfdm-calculator/src/mineralisatie/types.d.ts
✅ Files skipped from review due to trivial changes (2)
- fdm-calculator/src/mineralisatie/types.d.ts
- fdm-app/app/components/blocks/mineralization/mineralization-chart.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- fdm-calculator/src/mineralisatie/schemas.ts
- fdm-app/app/components/blocks/header/mineralization.tsx
- fdm-app/app/components/blocks/mineralization/dyna-balance.tsx
- fdm-calculator/src/mineralisatie/dyna.ts
- fdm-app/app/components/blocks/mineralization/dyna-chart.tsx
Comment on lines
+247
to
+249
| } catch (error) { | ||
| throw handleLoaderError(error) | ||
| } |
Contributor
There was a problem hiding this comment.
Preserve unexpected loader errors.
handleLoaderError(error) can fail to normalize plain errors; throwing the original error as fallback avoids losing the failure cause.
🐛 Proposed fix
} catch (error) {
- throw handleLoaderError(error)
+ const normalized = handleLoaderError(error)
+ throw normalized ?? error
}
}📝 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
| } catch (error) { | |
| throw handleLoaderError(error) | |
| } | |
| } catch (error) { | |
| const normalized = handleLoaderError(error) | |
| throw normalized ?? error | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.mineralization.$b_id.dyna.tsx
around lines 247 - 249, In the catch block that currently does "throw
handleLoaderError(error)", wrap the call to handleLoaderError(error) so you
preserve the original error if normalization fails: call
handleLoaderError(error) inside a try and if it returns a normalized error throw
that, but if handleLoaderError itself throws or returns an unexpected/undefined
value then throw the original caught error; reference the existing catch scope
and the handleLoaderError function to implement this fallback.
Comment on lines
+49
to
+69
| const activeOnMay15 = [...cultivations] | ||
| .sort((a, b) => { | ||
| const aTime = a.b_lu_start?.getTime() ?? 0 | ||
| const bTime = b.b_lu_start?.getTime() ?? 0 | ||
| return bTime - aTime | ||
| }) | ||
| .find((c) => { | ||
| if (!c.b_lu_start) return false | ||
| const start = c.b_lu_start | ||
| const end = c.b_lu_end ?? null | ||
| if (end) return start <= targetDate && end >= targetDate | ||
| return start <= targetDate | ||
| }) | ||
|
|
||
| if (activeOnMay15) return activeOnMay15 | ||
|
|
||
| // Fallback: first non-catchcrop in the year, then any crop | ||
| return ( | ||
| cultivations.find((c) => c.b_lu_croprotation !== "catchcrop") ?? | ||
| cultivations[0] | ||
| ) |
Contributor
There was a problem hiding this comment.
Don’t let a catchcrop win main-crop selection.
If a catchcrop spans May 15 and the main crop starts later, this returns the catchcrop as the principal cultivation. Prefer non-catchcrop cultivations before falling back to catchcrops.
🐛 Proposed fix
- const activeOnMay15 = [...cultivations]
- .sort((a, b) => {
- const aTime = a.b_lu_start?.getTime() ?? 0
- const bTime = b.b_lu_start?.getTime() ?? 0
- return bTime - aTime
- })
- .find((c) => {
+ const sortedCultivations = [...cultivations].sort((a, b) => {
+ const aTime = a.b_lu_start?.getTime() ?? 0
+ const bTime = b.b_lu_start?.getTime() ?? 0
+ return bTime - aTime
+ })
+ const isActiveOnMay15 = (c: T) => {
if (!c.b_lu_start) return false
const start = c.b_lu_start
const end = c.b_lu_end ?? null
if (end) return start <= targetDate && end >= targetDate
return start <= targetDate
- })
+ }
- if (activeOnMay15) return activeOnMay15
+ const activeMainCrop = sortedCultivations.find(
+ (c) => c.b_lu_croprotation !== "catchcrop" && isActiveOnMay15(c),
+ )
+ if (activeMainCrop) return activeMainCrop
- // Fallback: first non-catchcrop in the year, then any crop
+ // Fallback: first non-catchcrop in the year, then any crop
return (
cultivations.find((c) => c.b_lu_croprotation !== "catchcrop") ??
cultivations[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fdm-calculator/src/mineralisatie/builders.ts` around lines 49 - 69, The
current activeOnMay15 lookup can return a catchcrop that spans targetDate;
change the logic to first look for any cultivation that spans targetDate and is
not a catchcrop, and only if none found fall back to a spanning catchcrop, then
to the existing non-spanning fallbacks. Concretely: perform two searches on
cultivations using b_lu_start and b_lu_end against targetDate—first find(c =>
spans && c.b_lu_croprotation !== "catchcrop") then find(c => spans) —and assign
that result to activeOnMay15 (then keep the existing fallback of first
non-catchcrop in year, then any crop). Reference functions/values:
activeOnMay15, cultivations, b_lu_start, b_lu_end, b_lu_croprotation,
targetDate.
Comment on lines
+123
to
+139
| cultivations: { b_lu_catalogue?: string | null }[], | ||
| method: NSupplyMethod, | ||
| timeframe: Timeframe, | ||
| ): Record<string, unknown> { | ||
| const centroid = field.b_centroid | ||
| const a_lon = centroid ? centroid[0] : undefined | ||
| const a_lat = centroid ? centroid[1] : undefined | ||
|
|
||
| const b_lu_brp = cultivations | ||
| .filter((c) => c.b_lu_catalogue) | ||
| .map((c) => { | ||
| const code = (c.b_lu_catalogue ?? "").replace(/^nl_/, "") | ||
| const parsed = Number.parseInt(code, 10) | ||
| return Number.isNaN(parsed) ? undefined : parsed | ||
| }) | ||
| .find((v) => v !== undefined) | ||
|
|
Contributor
There was a problem hiding this comment.
Select the BRP code from the requested year’s main crop.
This currently uses the first catalogue entry in whatever order cultivations arrives. Because callers pass all cultivations, NSupply can be calculated with a historical or catchcrop BRP code.
🐛 Proposed fix
- cultivations: { b_lu_catalogue?: string | null }[],
+ cultivations: {
+ b_lu_catalogue?: string | null
+ b_lu_start?: Date | null
+ b_lu_end?: Date | null
+ b_lu_croprotation?: string | null
+ }[],
method: NSupplyMethod,
timeframe: Timeframe,
): Record<string, unknown> {
@@
- const b_lu_brp = cultivations
- .filter((c) => c.b_lu_catalogue)
- .map((c) => {
- const code = (c.b_lu_catalogue ?? "").replace(/^nl_/, "")
- const parsed = Number.parseInt(code, 10)
- return Number.isNaN(parsed) ? undefined : parsed
- })
- .find((v) => v !== undefined)
+ const targetYear = timeframe.start?.getFullYear()
+ const cropForBrp = targetYear
+ ? getMainCultivation(cultivations, targetYear)
+ : cultivations.find((c) => c.b_lu_catalogue)
+ const code = cropForBrp?.b_lu_catalogue?.replace(/^nl_/, "")
+ const parsedBrp = code ? Number.parseInt(code, 10) : Number.NaN
+ const b_lu_brp = Number.isNaN(parsedBrp) ? undefined : parsedBrp🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fdm-calculator/src/mineralisatie/builders.ts` around lines 123 - 139, The
code extracts b_lu_brp from the first entry in cultivations regardless of year
or crop type, which can pick historical or catch crops; change the logic in the
b_lu_brp computation to first filter cultivations for the requested timeframe
and for the main crop before parsing b_lu_catalogue (e.g. filter by c.year
matching timeframe and by a main-crop flag such as c.mainCrop / c.isMainCrop),
then map/parse the catalogue string as currently done and take the first valid
parsed value; update the logic around the b_lu_brp variable to reflect this
filtered selection.
Comment on lines
+178
to
+183
| export const getNSupply = withCalculationCache( | ||
| requestNSupply, | ||
| "requestNSupply", | ||
| pkg.calculatorVersion, | ||
| ["nmiApiKey"], | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect withCalculationCache to verify whether it enforces a 24h TTL/expiry.
rg -nP -C 12 'function\s+withCalculationCache|const\s+withCalculationCache|export\s+.*withCalculationCache|withCalculationCache\s*='
rg -nP -C 6 'ttl|expires|expiry|expire|24\s*\*|86400|cache.*duration|created_at|updated_at'Repository: nmi-agro/fdm
Length of output: 50371
🏁 Script executed:
head -250 fdm-core/src/calculator.ts | tail -100Repository: nmi-agro/fdm
Length of output: 4372
🏁 Script executed:
sed -n '260,330p' fdm-core/src/calculator.tsRepository: nmi-agro/fdm
Length of output: 1794
🏁 Script executed:
rg -n 'function\s+setCachedCalculation|const\s+setCachedCalculation|export\s+.*setCachedCalculation|setCachedCalculation\s*=' fdm-core/src/calculator.ts -A 30Repository: nmi-agro/fdm
Length of output: 1461
🏁 Script executed:
rg -n 'calculationCacheTable|calculation_cache' fdm-core/src/ -A 10 | head -80Repository: nmi-agro/fdm
Length of output: 5696
🏁 Script executed:
rg -n 'calculation_cache|calculationCache' fdm-core/src/ | rg -i 'delete|drop|clean|prune|purge|expire'Repository: nmi-agro/fdm
Length of output: 133
The nsupply cache lacks explicit 24-hour expiry enforcement.
The PR objective specifies 24h caching, but withCalculationCache does not enforce a TTL. Cache entries persist indefinitely—they are invalidated only when the request body or calculator version changes. Since created_at is logged but never inspected during reads, weather-driven or time-sensitive NMI results can serve stale data beyond 24 hours.
To meet the stated requirement, either:
- Introduce an explicit
ttlparameter towithCalculationCacheand enforce expiry ingetCachedCalculationby checkingcreated_at, or - Document that caching relies on request/version changes and update the PR objective accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fdm-calculator/src/mineralisatie/nsupply.ts` around lines 178 - 183,
getNSupply is created via withCalculationCache(requestNSupply, "requestNSupply",
pkg.calculatorVersion, ["nmiApiKey"]) but there is no TTL enforcement so cached
results never expire; update withCalculationCache signature to accept an
optional ttl (e.g., in seconds or ms) and propagate it into
getCachedCalculation, then in getCachedCalculation check the cached entry's
created_at against now and reject/refresh the cache when age > ttl (use the
created_at timestamp parse and a consistent timezone/epoch comparison); modify
the call that creates getNSupply to pass a 24h ttl (86400s or 24*60*60*1000ms
depending on units) and ensure tests cover expiration behavior — alternatively,
if you prefer not to change runtime behavior, update documentation/comments for
withCalculationCache and the PR objective to state caching is only invalidated
by request/version changes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
fdm-calculator/src/mineralization/nsupply.ts (1)
179-184:⚠️ Potential issue | 🟠 MajorEnforce the required 24-hour cache expiry.
This wrapper still uses
withCalculationCachewithout a TTL, so NSupply results can persist until the request body or calculator version changes. That does not satisfy the 24h caching requirement for weather/time-sensitive NMI output.#!/bin/bash # Description: Verify whether calculation caching supports a TTL or expiry check. # Expected: Reads should compare created_at/updated_at against a configured 24h TTL before returning cached NSupply results. rg -n -A70 -B15 'function\s+withCalculationCache|const\s+withCalculationCache|export function getCachedCalculation|function\s+getCachedCalculation|created_at|updated_at|ttl|expiry|expires'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/mineralization/nsupply.ts` around lines 179 - 184, getNSupply currently wraps requestNSupply with withCalculationCache but does not enforce a 24-hour TTL, so stale weather/time-sensitive NMI outputs can be returned; update the getNSupply call to pass a 24h expiry (e.g., a TTL parameter or options object) to withCalculationCache (or, if withCalculationCache has no TTL API, add TTL handling inside withCalculationCache/requestNSupply by comparing cached record timestamps like created_at/updated_at against a 24*60*60s threshold) so cached NSupply is considered expired after 24 hours; reference getNSupply, withCalculationCache and requestNSupply when making the change.fdm-calculator/src/mineralization/builders.ts (1)
351-434:⚠️ Potential issue | 🟠 MajorBuild DYNA rotation entries from the cultivation history, not only
[year].
buildDynaRequestaccepts all cultivations and documents preceding-year rotation history, but this maps only the target year. Carry-over and green-manure effects from prior rotation years are dropped.Proposed direction
- // Build rotation array — only include the current calculation year - // This ensures the simulation starts at 0 on January 1st of this year. - const rotation: Record<string, unknown>[] = [year] + const rotationYears = Array.from( + new Set([ + year, + ...cultivations + .flatMap((c) => [ + c.b_lu_start?.getFullYear(), + c.b_lu_end?.getFullYear(), + ]) + .filter((y): y is number => y !== undefined && y <= year), + ]), + ).sort((a, b) => a - b) + + const rotation: Record<string, unknown>[] = rotationYears .map((rotationYear) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/mineralization/builders.ts` around lines 351 - 434, The rotation array is built only for [year], losing prior-year rotation context; update the logic that builds rotation (currently the const rotation: Record<string, unknown>[] = [year].map(...).filter(...)) to iterate over the full rotation span derived from cultivations (e.g., collect all relevant rotation years from cultivations' b_lu_start/b_lu_end or a computed history range) instead of a single-element [year]; for each rotationYear call getMainCultivation(yearCultivations, rotationYear) and reuse the existing harvestsByBlu, cropProperties, greenManure and amendments handling, but keep the existing conditional that sets amendments only when rotationYear === year so only the target calendar year gets amendments. Ensure function names/variables to locate the change: rotation, cultivations, getMainCultivation, harvestsByBlu, cropProperties, amendments.
🧹 Nitpick comments (1)
fdm-app/app/integrations/mineralization.server.ts (1)
306-584: Significant duplication betweengetDynaForFieldand the inline body ofgetDynaForFarm.The per-field pipeline (crop_properties from catalogue,
harvestsByBlureconstruction,buildDynaRequestinvocation,getDynacall) is implemented twice: once ingetDynaForField(306-407) and again inline at 498-560.getDynaForFieldis exported but never consumed inside this file. Extracting a shared helper that takes pre-fetched inputs (field, soilData, cultivations, applications, fertilizerMap, catalogueEntries, harvestsMap, farmSector, timeframe, nmiApiKey) would letgetDynaForFarmreuse it and keep the two paths from drifting — for example, if a future change adjusts the harvest pre-flight or thecropPropertiesmapping in one place only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/integrations/mineralization.server.ts` around lines 306 - 584, There is duplicated per-field logic in getDynaForField and the inline task body in getDynaForFarm; extract a shared helper (e.g. runDynaForPrefetchedField or computeDynaForField) that takes the pre-fetched inputs (field, soilData map or built soilData, cultivations array, applications array, fertilizerMap, catalogueEntries, harvestsMap/harvestsByBlu, farmSector, timeframe, nmiApiKey) and performs the cropProperties mapping, harvestsByBlu construction, buildDynaRequest call and getDyna invocation, returning the same result shape; then update getDynaForFarm to call this helper from each task (passing field-specific slices) and refactor getDynaForField to reuse the same helper by first fetching its individual inputs and forwarding them, removing the duplicated code in both places.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fdm-app/app/components/blocks/mineralization/dyna-field-list.tsx`:
- Around line 86-123: The component DynaCells currently treats a missing
lastPoint (from result.calculationDyna) as zero and renders a success state;
change the guard to treat missing/empty calculationDyna as unavailable: compute
a boolean like hasValue = !error && !!result?.calculationDyna?.length &&
lastPoint !== undefined and use hasValue instead of error for deciding whether
to render rounded numbers (Math.round) and the success CircleCheck; when
hasValue is false render the "—" placeholders and the error/unavailable
indicator (CircleX) so missing data is not shown as a successful zero.
In `@fdm-app/app/integrations/mineralization.server.ts`:
- Around line 95-192: buildSoilDataMap currently drops per-parameter metadata so
assessDataCompleteness is called without soilMeta (in getNSupplyForField),
causing NMI-estimated params to be treated as measured; change buildSoilDataMap
to return both { soilData, soilMeta } (preserving source/date info from
getCurrentSoilData entries), then in getNSupplyForField destructure that result
and call assessDataCompleteness(soilData, method, soilMeta) (and thread the same
change into getNSupplyForFarm/other callers), ensuring the
NSupplyComputeInput/completeness uses the completeness computed with soilMeta so
NMI-estimated params are excluded as intended.
- Around line 110-114: getCurrentSoilData currently assigns a_depth_lower from
soilDataArray[0], which is arbitrary and may not match other parameters; change
the logic so that before setting soilData.a_depth_lower you either (a) verify
all entries in soilDataArray share the same a_depth_lower and use it, or (b)
select the depth from the specific parameter entry used by the builder (e.g.,
find the element in soilDataArray whose parameter matches the one being
assembled) and set soilData.a_depth_lower from that entry, and if depths differ
emit a clear error/log or reject the aggregation—update the code around
soilDataArray, soilData and the builders to perform this validation/selection
instead of unconditionally using soilDataArray[0].
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.mineralization._index.tsx:
- Around line 109-186: The top-level catch in the asyncDynaPromises loader
returns an empty array which causes dynaPromisesArray[i] to be undefined in
MineralizationFarmContent and breaks DynaFieldList; modify the loader's catch
around getDynaForFarm to fabricate a per-field rejected/errored Promise for each
field (e.g., map fields to Promise.reject or Promise.resolve with an error
result keyed by b_id) so callers always receive a same-length array or better
yet return an object keyed by b_id from getDynaForFarm; then update
MineralizationFarmContent to stop relying on index alignment by building
dynaPromises from keys (use dynaPromisesArray keyed by b_id or map fields to
(dynaPromisesArray[field.b_id] ?? a failed Promise) before creating
dynaPromises) so DynaFieldList receives a Promise per b_id even on top-level
failures.
In `@fdm-calculator/src/mineralization/assessment.ts`:
- Around line 96-138: The code currently pushes any non-null soilData into
available even when soilMeta?.[param]?.source === NMI_SOURCE; update the two
loops that build available/missing/estimated so they check the param source
(NMI_SOURCE = "nl-other-nmi") before pushing: for required params, if value is
null/undefined -> push to missing; else if source === NMI_SOURCE -> push to
missing (so it’s excluded from available and treated consistently), else push to
available; for optional params, if value is null/undefined -> push to estimated;
else if source === NMI_SOURCE -> push to estimated, else push to available. This
keeps available, missing, and estimated consistent with the
availableRequired/availableOptional scoring logic (refer to available, missing,
estimated, NMI_SOURCE, required, optional, soilData, soilMeta,
availableRequired, availableOptional).
In `@fdm-calculator/src/mineralization/builders.ts`:
- Around line 49-67: The current selection logic for activeMainOnMay15 and
activeAnyOnMay15 uses find() which may pick an arbitrary matching item; change
it to collect all matching cultivations from the cultivations array (respecting
the same date/span checks and b_lu_croprotation filter for activeMainOnMay15),
then sort those candidates by b_lu_start descending (most recent start first)
and pick the first element, or use a reduce to pick the candidate with the max
b_lu_start; update both the activeMainOnMay15 and activeAnyOnMay15 blocks to use
this sorted-or-reduced selection and keep using b_lu_start, b_lu_end,
b_lu_croprotation and targetDate identifiers.
In `@fdm-calculator/src/mineralization/schemas.ts`:
- Around line 47-74: The schema dynaDailyPointSchema currently requires
b_nw_recommended, b_n_uptake, and b_no3_leach as non-nullable numbers but the
API may return null; update the z.object to call .nullable() on those three
fields (i.e., b_nw_recommended, b_n_uptake, b_no3_leach) so validation accepts
null, and also update the DynaDailyPoint interface in types.d.ts to mark those
properties as number | null to match the schema and existing UI usage.
---
Duplicate comments:
In `@fdm-calculator/src/mineralization/builders.ts`:
- Around line 351-434: The rotation array is built only for [year], losing
prior-year rotation context; update the logic that builds rotation (currently
the const rotation: Record<string, unknown>[] = [year].map(...).filter(...)) to
iterate over the full rotation span derived from cultivations (e.g., collect all
relevant rotation years from cultivations' b_lu_start/b_lu_end or a computed
history range) instead of a single-element [year]; for each rotationYear call
getMainCultivation(yearCultivations, rotationYear) and reuse the existing
harvestsByBlu, cropProperties, greenManure and amendments handling, but keep the
existing conditional that sets amendments only when rotationYear === year so
only the target calendar year gets amendments. Ensure function names/variables
to locate the change: rotation, cultivations, getMainCultivation, harvestsByBlu,
cropProperties, amendments.
In `@fdm-calculator/src/mineralization/nsupply.ts`:
- Around line 179-184: getNSupply currently wraps requestNSupply with
withCalculationCache but does not enforce a 24-hour TTL, so stale
weather/time-sensitive NMI outputs can be returned; update the getNSupply call
to pass a 24h expiry (e.g., a TTL parameter or options object) to
withCalculationCache (or, if withCalculationCache has no TTL API, add TTL
handling inside withCalculationCache/requestNSupply by comparing cached record
timestamps like created_at/updated_at against a 24*60*60s threshold) so cached
NSupply is considered expired after 24 hours; reference getNSupply,
withCalculationCache and requestNSupply when making the change.
---
Nitpick comments:
In `@fdm-app/app/integrations/mineralization.server.ts`:
- Around line 306-584: There is duplicated per-field logic in getDynaForField
and the inline task body in getDynaForFarm; extract a shared helper (e.g.
runDynaForPrefetchedField or computeDynaForField) that takes the pre-fetched
inputs (field, soilData map or built soilData, cultivations array, applications
array, fertilizerMap, catalogueEntries, harvestsMap/harvestsByBlu, farmSector,
timeframe, nmiApiKey) and performs the cropProperties mapping, harvestsByBlu
construction, buildDynaRequest call and getDyna invocation, returning the same
result shape; then update getDynaForFarm to call this helper from each task
(passing field-specific slices) and refactor getDynaForField to reuse the same
helper by first fetching its individual inputs and forwarding them, removing the
duplicated code in both places.
🪄 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: 29149711-1e42-448a-8773-39a1bff4587a
📒 Files selected for processing (16)
fdm-app/app/components/blocks/mineralization/dyna-field-list.tsxfdm-app/app/components/blocks/mineralization/nsupply-kpi.tsxfdm-app/app/integrations/mineralization.server.tsfdm-app/app/routes/farm.$b_id_farm.$calendar.mineralization.$b_id._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.mineralization.$b_id.dyna.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.mineralization._index.tsxfdm-calculator/src/index.tsfdm-calculator/src/mineralization/assessment.tsfdm-calculator/src/mineralization/builders.test.tsfdm-calculator/src/mineralization/builders.tsfdm-calculator/src/mineralization/dyna.tsfdm-calculator/src/mineralization/errors.tsfdm-calculator/src/mineralization/index.tsfdm-calculator/src/mineralization/nsupply.tsfdm-calculator/src/mineralization/schemas.tsfdm-calculator/src/mineralization/types.d.ts
✅ Files skipped from review due to trivial changes (3)
- fdm-calculator/src/mineralization/errors.ts
- fdm-calculator/src/mineralization/index.ts
- fdm-calculator/src/mineralization/types.d.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- fdm-calculator/src/index.ts
- fdm-app/app/routes/farm.$b_id_farm.$calendar.mineralization.$b_id.dyna.tsx
Comment on lines
+47
to
+74
| export const dynaDailyPointSchema = z.object({ | ||
| /** Calendar date of this simulation step (ISO 8601) */ | ||
| b_date_calculation: z.string(), | ||
| /** N availability — central estimate */ | ||
| b_nw: z.number(), | ||
| /** N availability — lower bound */ | ||
| b_nw_min: z.number(), | ||
| /** N availability — upper bound */ | ||
| b_nw_max: z.number(), | ||
| /** N availability — recommended target */ | ||
| b_nw_recommended: z.number(), | ||
| /** N uptake by crop — central estimate */ | ||
| b_n_uptake: z.number(), | ||
| /** N uptake — lower bound */ | ||
| b_n_uptake_min: z.number(), | ||
| /** N uptake — upper bound */ | ||
| b_n_uptake_max: z.number(), | ||
| /** N uptake — recommended target */ | ||
| b_n_uptake_recommended: z.number(), | ||
| /** NO₃ leaching — central estimate */ | ||
| b_no3_leach: z.number(), | ||
| /** NO₃ leaching — lower bound */ | ||
| b_no3_leach_min: z.number(), | ||
| /** NO₃ leaching — upper bound */ | ||
| b_no3_leach_max: z.number(), | ||
| /** NO₃ leaching — recommended target */ | ||
| b_no3_leach_recommended: z.number(), | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare the DYNA daily point TypeScript type with the Zod schema nullability.
# Expected: Nullable fields in DynaDailyPoint should also be nullable in dynaDailyPointSchema.
rg -n -A45 -B5 'interface DynaDailyPoint|export const dynaDailyPointSchema'Repository: nmi-agro/fdm
Length of output: 8808
🏁 Script executed:
#!/bin/bash
# Search for DYNA API response fixtures, tests, or documentation
find . -type f \( -name "*.test.ts" -o -name "*.spec.ts" -o -name "*.json" \) | xargs grep -l "dyna\|DynaDailyPoint" 2>/dev/null | head -20Repository: nmi-agro/fdm
Length of output: 177
🏁 Script executed:
#!/bin/bash
# Look for any null-handling or optional chaining on DYNA fields
rg -n "b_nw_recommended|b_n_uptake|b_no3_leach" --type ts -A2 -B2 | head -100Repository: nmi-agro/fdm
Length of output: 9086
🏁 Script executed:
#!/bin/bash
# Search for API response or mock data containing DYNA results
rg -l "calculation_dyna|POST.*dyna" . --type ts --type json 2>/dev/null | head -10Repository: nmi-agro/fdm
Length of output: 241
🏁 Script executed:
#!/bin/bash
# Examine how DYNA response is processed and where nullable handling occurs
cat fdm-calculator/src/mineralization/dyna.ts | head -100Repository: nmi-agro/fdm
Length of output: 4026
🏁 Script executed:
#!/bin/bash
# Check the builders.ts file for any data transformation
cat fdm-calculator/src/mineralization/builders.ts | head -100Repository: nmi-agro/fdm
Length of output: 3862
🏁 Script executed:
#!/bin/bash
# Look for where DynaDailyPoint is created/used in relation to the schema
rg -n "DynaDailyPoint|dynaDailyPointSchema" fdm-calculator/src/mineralization/ -A3 -B1Repository: nmi-agro/fdm
Length of output: 2773
🏁 Script executed:
#!/bin/bash
# Verify that the schema is indeed missing .nullable() for these fields
cat -n fdm-calculator/src/mineralization/schemas.ts | sed -n '47,74p'Repository: nmi-agro/fdm
Length of output: 1265
🏁 Script executed:
#!/bin/bash
# Check if there are any test files that show DYNA API returning null values
find . -type f \( -name "*.test.ts" -o -name "*.spec.ts" \) -exec grep -l "b_nw_recommended\|b_n_uptake\|null" {} \; | xargs grep -l "dyna\|DYNA" 2>/dev/null | head -3Repository: nmi-agro/fdm
Length of output: 113
🏁 Script executed:
#!/bin/bash
# Look for any conversion or transformation that might handle nulls between schema and UI
rg "dynaDailyPointSchema|calculationDyna" fdm-app/ -A5 -B2 | head -80Repository: nmi-agro/fdm
Length of output: 2434
Add .nullable() to DYNA schema fields that the API can return as null.
The schema requires non-nullable numbers, but the API can return null for b_nw_recommended, b_n_uptake, and b_no3_leach fields. UI components already handle these as nullable (e.g., dyna-chart.tsx redefines the type with b_n_uptake: number | null and explicitly checks d.b_n_uptake !== null), and validation will fail on valid responses containing null values.
Proposed schema change
/** N availability — recommended target */
- b_nw_recommended: z.number(),
+ b_nw_recommended: z.number().nullable(),
/** N uptake by crop — central estimate */
- b_n_uptake: z.number(),
+ b_n_uptake: z.number().nullable(),
/** N uptake — lower bound */
- b_n_uptake_min: z.number(),
+ b_n_uptake_min: z.number().nullable().optional(),
/** N uptake — upper bound */
- b_n_uptake_max: z.number(),
+ b_n_uptake_max: z.number().nullable().optional(),
/** N uptake — recommended target */
- b_n_uptake_recommended: z.number(),
+ b_n_uptake_recommended: z.number().nullable().optional(),
/** NO₃ leaching — central estimate */
- b_no3_leach: z.number(),
+ b_no3_leach: z.number().nullable(),
/** NO₃ leaching — lower bound */
- b_no3_leach_min: z.number(),
+ b_no3_leach_min: z.number().nullable().optional(),
/** NO₃ leaching — upper bound */
- b_no3_leach_max: z.number(),
+ b_no3_leach_max: z.number().nullable().optional(),
/** NO₃ leaching — recommended target */
- b_no3_leach_recommended: z.number(),
+ b_no3_leach_recommended: z.number().nullable().optional(),Update the DynaDailyPoint interface in types.d.ts to match the schema changes, marking these fields as nullable.
📝 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
| export const dynaDailyPointSchema = z.object({ | |
| /** Calendar date of this simulation step (ISO 8601) */ | |
| b_date_calculation: z.string(), | |
| /** N availability — central estimate */ | |
| b_nw: z.number(), | |
| /** N availability — lower bound */ | |
| b_nw_min: z.number(), | |
| /** N availability — upper bound */ | |
| b_nw_max: z.number(), | |
| /** N availability — recommended target */ | |
| b_nw_recommended: z.number(), | |
| /** N uptake by crop — central estimate */ | |
| b_n_uptake: z.number(), | |
| /** N uptake — lower bound */ | |
| b_n_uptake_min: z.number(), | |
| /** N uptake — upper bound */ | |
| b_n_uptake_max: z.number(), | |
| /** N uptake — recommended target */ | |
| b_n_uptake_recommended: z.number(), | |
| /** NO₃ leaching — central estimate */ | |
| b_no3_leach: z.number(), | |
| /** NO₃ leaching — lower bound */ | |
| b_no3_leach_min: z.number(), | |
| /** NO₃ leaching — upper bound */ | |
| b_no3_leach_max: z.number(), | |
| /** NO₃ leaching — recommended target */ | |
| b_no3_leach_recommended: z.number(), | |
| }) | |
| export const dynaDailyPointSchema = z.object({ | |
| /** Calendar date of this simulation step (ISO 8601) */ | |
| b_date_calculation: z.string(), | |
| /** N availability — central estimate */ | |
| b_nw: z.number(), | |
| /** N availability — lower bound */ | |
| b_nw_min: z.number(), | |
| /** N availability — upper bound */ | |
| b_nw_max: z.number(), | |
| /** N availability — recommended target */ | |
| b_nw_recommended: z.number().nullable(), | |
| /** N uptake by crop — central estimate */ | |
| b_n_uptake: z.number().nullable(), | |
| /** N uptake — lower bound */ | |
| b_n_uptake_min: z.number().nullable().optional(), | |
| /** N uptake — upper bound */ | |
| b_n_uptake_max: z.number().nullable().optional(), | |
| /** N uptake — recommended target */ | |
| b_n_uptake_recommended: z.number().nullable().optional(), | |
| /** NO₃ leaching — central estimate */ | |
| b_no3_leach: z.number().nullable(), | |
| /** NO₃ leaching — lower bound */ | |
| b_no3_leach_min: z.number().nullable().optional(), | |
| /** NO₃ leaching — upper bound */ | |
| b_no3_leach_max: z.number().nullable().optional(), | |
| /** NO₃ leaching — recommended target */ | |
| b_no3_leach_recommended: z.number().nullable().optional(), | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fdm-calculator/src/mineralization/schemas.ts` around lines 47 - 74, The
schema dynaDailyPointSchema currently requires b_nw_recommended, b_n_uptake, and
b_no3_leach as non-nullable numbers but the API may return null; update the
z.object to call .nullable() on those three fields (i.e., b_nw_recommended,
b_n_uptake, b_no3_leach) so validation accepts null, and also update the
DynaDailyPoint interface in types.d.ts to mark those properties as number | null
to match the schema and existing UI usage.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
fdm-app/app/integrations/mineralization.server.ts (1)
537-549: Minor:fertilizerMap as anycast hides a structural-subtype gap.The
fertilizerMapbuilt ingetDynaForField(lines 520–535) holds a strict subset of the shape produced bygetFertilizers(onlyp_id+ nutrient fields). Theas anycast silences the resulting type mismatch withrunDynaForPrefetchedField's parameter. It works today because the callee only reads the listed nutrient fields, but the cast will also mask real breakage if that function starts touching anything else on the fertilizer value.Consider extracting a narrow
FertilizerNutrientPropstype shared by both constructions, or just type the parameter as that subset so both call sites type-check without a cast.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/integrations/mineralization.server.ts` around lines 537 - 549, The fertilizerMap cast to any hides a structural-subtype mismatch between getFertilizers and the subset constructed in getDynaForField; define a narrow shared type (e.g., FertilizerNutrientProps) that includes only p_id and the nutrient fields, update getDynaForField to type the constructed fertilizerMap as Record<string, FertilizerNutrientProps>, and change runDynaForPrefetchedField's parameter type to accept Record<string, FertilizerNutrientProps> (or use the new type where appropriate) so the call no longer requires fertilizerMap as any and the shape is enforced at both sites.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.mineralization.tsx:
- Around line 55-60: The loader-returned isMineralizationEnabled is never used
which causes flicker and wasted server calls; change the component to prefer the
server value from loaderData.isMineralizationEnabled and only fall back to the
client hook useFeatureFlagEnabled("mineralization") when loaderData is undefined
(or remove the server check entirely). Locate the loader's
isMineralizationEnabled (returned in loaderData) and update the component's flag
resolution logic so it first reads loaderData.isMineralizationEnabled and only
queries useFeatureFlagEnabled("mineralization") if the server value is
null/undefined, ensuring the initial render uses the server truthy/falsey value
to avoid flicker and stop redundant PostHog calls.
---
Nitpick comments:
In `@fdm-app/app/integrations/mineralization.server.ts`:
- Around line 537-549: The fertilizerMap cast to any hides a structural-subtype
mismatch between getFertilizers and the subset constructed in getDynaForField;
define a narrow shared type (e.g., FertilizerNutrientProps) that includes only
p_id and the nutrient fields, update getDynaForField to type the constructed
fertilizerMap as Record<string, FertilizerNutrientProps>, and change
runDynaForPrefetchedField's parameter type to accept Record<string,
FertilizerNutrientProps> (or use the new type where appropriate) so the call no
longer requires fertilizerMap as any and the shape is enforced at both sites.
🪄 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: 8fa5080a-c398-4fa9-abb5-f4a03fe8cd71
📒 Files selected for processing (13)
.changeset/bright-pans-exist.md.changeset/funky-doors-sort.mdfdm-app/app/components/blocks/mineralization/dyna-field-list.tsxfdm-app/app/components/blocks/sidebar/farm.tsxfdm-app/app/integrations/mineralization.server.tsfdm-app/app/root.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.mineralization._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.mineralization.tsxfdm-calculator/package.jsonfdm-calculator/src/mineralization/builders.test.tsfdm-calculator/src/mineralization/builders.tsfdm-calculator/src/mineralization/schemas.tsfdm-calculator/src/mineralization/types.d.ts
✅ Files skipped from review due to trivial changes (5)
- .changeset/bright-pans-exist.md
- fdm-app/app/root.tsx
- fdm-calculator/package.json
- .changeset/funky-doors-sort.md
- fdm-calculator/src/mineralization/types.d.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- fdm-app/app/components/blocks/sidebar/farm.tsx
- fdm-calculator/src/mineralization/schemas.ts
Collaborator
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
fdm-app/app/routes/farm.$b_id_farm.$calendar.mineralization.tsx (1)
56-60:⚠️ Potential issue | 🟡 MinorMake the client flag fallback reachable or remove it.
Line 60 always returns a boolean, so
loaderData.isMineralizationEnabled ?? clientFlagEnablednever falls back to the client hook. Either return the raw server flag when PostHog is unavailable, or drop the client hook/comment if the server value should be authoritative.🐛 Proposed fix if client fallback is intended
- const isMineralizationEnabled = - (await posthog?.isFeatureEnabled( - "mineralization", - session.principal_id, - )) ?? true + const isMineralizationEnabled = await posthog?.isFeatureEnabled( + "mineralization", + session.principal_id, + )Also applies to: 123-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.mineralization.tsx around lines 56 - 60, The server loader currently assigns isMineralizationEnabled with a boolean default so the client-side fallback (clientFlagEnabled / loaderData.isMineralizationEnabled ?? clientFlagEnabled) is never used; either return the raw nullable server flag by removing the "?? true" default from the posthog?.isFeatureEnabled assignment (keep const isMineralizationEnabled = await posthog?.isFeatureEnabled("mineralization", session.principal_id)) so the client can decide when posthog is unavailable, or remove the client fallback and any related comment/variable (clientFlagEnabled and uses of loaderData.isMineralizationEnabled ?? clientFlagEnabled) so the server value is authoritative; update both occurrences around isMineralizationEnabled and the block that references loaderData.isMineralizationEnabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.mineralization.tsx:
- Around line 56-60: The server loader currently assigns isMineralizationEnabled
with a boolean default so the client-side fallback (clientFlagEnabled /
loaderData.isMineralizationEnabled ?? clientFlagEnabled) is never used; either
return the raw nullable server flag by removing the "?? true" default from the
posthog?.isFeatureEnabled assignment (keep const isMineralizationEnabled = await
posthog?.isFeatureEnabled("mineralization", session.principal_id)) so the client
can decide when posthog is unavailable, or remove the client fallback and any
related comment/variable (clientFlagEnabled and uses of
loaderData.isMineralizationEnabled ?? clientFlagEnabled) so the server value is
authoritative; update both occurrences around isMineralizationEnabled and the
block that references loaderData.isMineralizationEnabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: beda6fb3-ceae-4eed-9690-2a0dda050762
📒 Files selected for processing (2)
fdm-app/app/integrations/mineralization.server.tsfdm-app/app/routes/farm.$b_id_farm.$calendar.mineralization.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- fdm-app/app/integrations/mineralization.server.ts
BoraIneviNMI
approved these changes
Apr 23, 2026
Collaborator
BoraIneviNMI
left a comment
BoraIneviNMI
left a comment
There was a problem hiding this comment.
I don't see anything particular. The cache seems to work properly and the pages are responsive.
I would change the text of the NSupplyMethod selector on the main mineralization route. It shows an ellipsis because of the description part of the dropdown. It is easy to add your own text but I am trying to find a way to do it with
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
2 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.
Summary by CodeRabbit
New Features
Navigation & UI
Tests & Chores
Closes #539