Add MINIP NLV calculations and expose calculated soil supply values - #718
Conversation
… soil organic matter from soil) that can have a 'calculated' source
…lization from soil organic mattter. Rename `calculateNlvSupplyBySom` to `calculateNlvSupplyIncreaseBySomPotential`
…upply_base`) across soil cards, the field overview dashboard, fields table, and the soil analysis atlas map layer.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds the ChangesNLV calculation API
Core soil contract
Application integration
Atlas analytics
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RouteLoader
participant SoilDataAPI
participant SoilEnrichment
participant NlvCalculator
participant SoilUI
RouteLoader->>SoilDataAPI: fetch current soil data
SoilDataAPI-->>SoilEnrichment: return soil parameters
SoilEnrichment->>NlvCalculator: calculate missing d_n_supply_base
NlvCalculator-->>SoilEnrichment: return rounded NLV
SoilEnrichment-->>SoilUI: render enriched soil data
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id._index.tsx (1)
234-260: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winInline
awaitinside thePromise.allarray breaks parallelism.
enrichCurrentSoilDataWithNlv(await getCurrentSoilData(...))is evaluated as an array element with a suspendingawait. Because array elements are evaluated left-to-right, this pauses construction of the array beforegetSoilAnalyses(...)andgetMeasures(...)(elements after it) are even invoked — those two calls no longer run concurrently with the rest, they now wait ongetCurrentSoilDatato finish first, adding sequential latency to the loader.⚡ Proposed fix — enrich after Promise.all resolves
fertilizerApplications, fertilizers, - enrichCurrentSoilDataWithNlv( - await getCurrentSoilData(fdm, session.principal_id, b_id, timeframe), - ), + currentSoilDataRaw, soilAnalyses, measures, ] = await Promise.all([ getField(fdm, session.principal_id, b_id), getFields(fdm, session.principal_id, b_id_farm, timeframe), getCultivations(fdm, session.principal_id, b_id, timeframe), getCultivations(fdm, session.principal_id, b_id), getFertilizerApplications(fdm, session.principal_id, b_id, timeframe), getFertilizers(fdm, session.principal_id, b_id_farm), - getSoilAnalyses(fdm, session.principal_id, b_id, { + getCurrentSoilData(fdm, session.principal_id, b_id, timeframe), + getSoilAnalyses(fdm, session.principal_id, b_id, { start: null, end: timeframe.end, }), getMeasures(fdm, session.principal_id, b_id, timeframe), ]) + + const currentSoilData = enrichCurrentSoilDataWithNlv(currentSoilDataRaw)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.field.$b_id._index.tsx around lines 234 - 260, Remove the inline await from the Promise.all array in the loader, keeping getCurrentSoilData as its own parallel promise alongside getSoilAnalyses and getMeasures. After Promise.all resolves, pass the resolved current soil data through enrichCurrentSoilDataWithNlv before returning or using the loader results, preserving the existing enriched value for currentSoilData.fdm-app/app/components/blocks/soil/cards.tsx (1)
107-118: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep calculated NLV traceable to its source analysis.
The enrichment preserves the originating
a_idand sampling date, but this change removes the view link and hides the date forsource === "calculated". Users see “Berekend” without being able to inspect the analysis behind the value. Keep a view-only link and display the inherited source date.As per coding guidelines, numbers must remain transparent and auditable and connected insights discoverable.
Proposed direction
- const EditIcon = canModify ? Pencil : ExternalLink + const EditIcon = source === "calculated" || !canModify ? ExternalLink : Pencil - {source !== "nl-other-nmi" && source !== "calculated" ? ( + {source !== "nl-other-nmi" ? ( - <TooltipContent>{canModify ? "Bewerken" : "Bekijken"}</TooltipContent> + <TooltipContent> + {source === "calculated" || !canModify ? "Bekijken" : "Bewerken"} + </TooltipContent> - {!(!date || source === "nl-other-nmi" || source === "calculated") && ( + {!(!date || source === "nl-other-nmi") && (Also applies to: 148-177
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fdm-app/app/components/blocks/soil/cards.tsx` around lines 107 - 118, Update the soil card rendering around the source-based edit/view link and date display so source === "calculated" retains a view-only NavLink to the inherited analysis, using canModify to prevent editing. Restore the preserved sampling date for calculated NLV entries, while keeping existing exclusions for nl-other-nmi and the current behavior for other sources unchanged.Source: Coding guidelines
🧹 Nitpick comments (1)
fdm-app/app/routes/farm.$b_id_farm.$calendar.atlas.soil-analysis._index.tsx (1)
96-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated NLV derivation logic — consider reusing
enrichCurrentSoilDataWithNlv.This reimplements the same som/clay/cn extraction,
calculateNlvcall, and 1-decimal rounding already implemented infdm-app/app/lib/soil.server.ts. Since the logic is correct here (it does guard against overwriting an existing value), the main risk is drift if the formula/rounding changes in one place but not the other — e.g.,field.tsxalready reimplements this without the existing-value guard.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.atlas.soil-analysis._index.tsx around lines 96 - 119, The field feature-building logic duplicates NLV enrichment already provided by enrichCurrentSoilDataWithNlv in soil.server.ts. Reuse that helper for current soil data before constructing soilProps, preserving the existing-value guard and removing the local som/clay/cn extraction, calculateNlv call, and rounding logic from the fields map.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/swift-ears-argue.md:
- Line 5: Update the changeset text to correct “calulate” to “calculate” and
“mattter” to “matter”, and reference the exported function as `calculateNlv`
with lowercase “l” instead of `calculateNLv`; preserve the existing rename
description for `calculateNlvSupplyBySom`.
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.field.tsx:
- Around line 192-206: Update the NLV derivation near d_n_supply_base to first
reuse an existing currentSoilData entry whose parameter is d_n_supply_base,
preserving its measured value instead of recalculating it. Only call
calculateNlv when no existing value is present, and align the behavior with
enrichCurrentSoilDataWithNlv and the atlas soil-analysis loader.
In `@fdm-calculator/src/other/nlv-supply.ts`:
- Around line 39-90: Update calculateNlv with overloads that narrow the return
type: number when format is omitted or "number", and Decimal when format is
"decimal". Keep the existing calculation and runtime branching unchanged, while
ensuring the implementation signature remains compatible with both overloads.
In `@fdm-core/src/db/schema.ts`:
- Around line 677-680: Remove the “calculated” option from the persisted
soil-analysis source options and the editable enum/options generation used by
SoilAnalysisForm. If display of derived results is required, expose it through a
separate derived-source/display-only option rather than the a_source enum,
ensuring users cannot persist “Berekend” as an analysis source.
---
Outside diff comments:
In `@fdm-app/app/components/blocks/soil/cards.tsx`:
- Around line 107-118: Update the soil card rendering around the source-based
edit/view link and date display so source === "calculated" retains a view-only
NavLink to the inherited analysis, using canModify to prevent editing. Restore
the preserved sampling date for calculated NLV entries, while keeping existing
exclusions for nl-other-nmi and the current behavior for other sources
unchanged.
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.field.$b_id._index.tsx:
- Around line 234-260: Remove the inline await from the Promise.all array in the
loader, keeping getCurrentSoilData as its own parallel promise alongside
getSoilAnalyses and getMeasures. After Promise.all resolves, pass the resolved
current soil data through enrichCurrentSoilDataWithNlv before returning or using
the loader results, preserving the existing enriched value for currentSoilData.
---
Nitpick comments:
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.atlas.soil-analysis._index.tsx:
- Around line 96-119: The field feature-building logic duplicates NLV enrichment
already provided by enrichCurrentSoilDataWithNlv in soil.server.ts. Reuse that
helper for current soil data before constructing soilProps, preserving the
existing-value guard and removing the local som/clay/cn extraction, calculateNlv
call, and rounding logic from the fields map.
🪄 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 Plus
Run ID: 9d49687c-538b-4155-8d35-267289b52105
📒 Files selected for processing (24)
.changeset/grumpy-mirrors-see.md.changeset/rich-masks-occur.md.changeset/swift-ears-argue.mdfdm-app/app/components/blocks/atlas/atlas-soil-analysis.tsfdm-app/app/components/blocks/field-dashboard/tiles.tsxfdm-app/app/components/blocks/fields/columns.tsxfdm-app/app/components/blocks/soil/cards.tsxfdm-app/app/lib/soil.server.tsfdm-app/app/routes/farm.$b_id_farm.$calendar.atlas.fields.$centroid.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.atlas.soil-analysis._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.atlas_.soil-analysis.$b_id.soil._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.atlas_.soil-analysis.$b_id.soil.analysis.$a_id.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.soil._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.field.tsxfdm-calculator/src/index.tsfdm-calculator/src/other/nlv-supply-by-som.test.tsfdm-calculator/src/other/nlv-supply-by-som.tsfdm-calculator/src/other/nlv-supply.test.tsfdm-calculator/src/other/nlv-supply.tsfdm-core/src/db/schema.tsfdm-core/src/soil.tsfdm-core/src/soil.types.d.tsfdm-docs/blog/2026-03-04-release-notes.mdx
💤 Files with no reviewable changes (2)
- fdm-calculator/src/other/nlv-supply-by-som.test.ts
- fdm-calculator/src/other/nlv-supply-by-som.ts
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Closes #717