Switch to emission; add nitrate scaffolding; refactor mineralization#236
Conversation
…rogen balance as it has been replaced with default values instead of MINIP calculations
🦋 Changeset detectedLatest commit: 54cea0c The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Rate limit exceeded@SvenVw has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 44 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughTerminology and API shift from "volatilization" to "emission" across calculator, app UI, and docs. Adds nitrate-emission scaffolding and an emission orchestrator. Replaces MINIP mineralization with a per-year default, timeframe-weighted approach requiring cultivations and cultivationDetails. Updates types, tests, charts, routes, and changeset entries. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as App Route/Components
participant Calc as calculateNitrogenBalance
participant Em as calculateNitrogenEmission
participant Amm as Ammonia (fertilizers+residues)
participant NO3 as Nitrate (scaffold)
UI->>Calc: Request nitrogen balance (field/farm)
Calc->>Em: calculateNitrogenEmission(cultivations, harvests, fertilizerApplications, maps)
Em->>Amm: calculateNitrogenEmissionViaAmmonia(...)
Em->>NO3: calculateNitrogenEmissionViaNitrate()
Em-->>Calc: { total, ammonia, nitrate }
Calc-->>UI: Balance with emission (field/farm)
sequenceDiagram
participant Calc as Supply.calculateNitrogenSupply
participant Min as Mineralization.calculateNitrogenSupplyBySoilMineralization
participant Def as Defaults lookup
Calc->>Min: cultivations, soilAnalysis, cultivationDetails, timeFrame
Min->>Def: per-year: soilType + isGrassland → annualDefault
Def-->>Min: annualDefault
Min-->>Calc: { total, years[] } (prorated by timeframe)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## development #236 +/- ##
===============================================
- Coverage 92.94% 92.78% -0.16%
===============================================
Files 85 87 +2
Lines 13004 13091 +87
Branches 1310 1316 +6
===============================================
+ Hits 12086 12146 +60
- Misses 916 943 +27
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…lculation so that it can include nitrate leaching as well
… 'volatilization' to 'emission'
coderabbitai
Bot
changed the title
coderabbitai
Bot
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (8)
fdm-calculator/src/balance/nitrogen/emission/ammonia/residues.ts (1)
96-104: Guard against division by zero when b_lu_hi is 0 or missing
b_lu_yield.dividedBy(b_lu_hi)will produce Infinity ifb_lu_hiis 0, and combined with an emission factor of 0 this can yield NaN. Add an early return forb_lu_hi <= 0.// Get the harvest index for crop residues const b_lu_hi = new Decimal(cultivationDetail.b_lu_hi ?? 0) const b_lu_hi_res = new Decimal(1).minus(b_lu_hi) + // Avoid division by zero or negative/invalid HI + if (b_lu_hi.lte(0)) { + return { + id: cultivation.b_lu, + value: new Decimal(0), + } + } + // Get the Nitrogen content of the crop residues const b_lu_n_residue = new Decimal( cultivationDetail.b_lu_n_residue ?? 0, ) @@ // Calculate the amount of Nitrogen volatilized by crop residues of this cultivation const removal = b_lu_yield .dividedBy(b_lu_hi) .times(b_lu_hi_res) .times(b_lu_n_residue) .times(emissionFactor) .dividedBy(new Decimal(1000)) // Convert from g N / ha to kg N / ha .times(-1) // Return negative valueAlso applies to: 111-118
fdm-calculator/src/balance/nitrogen/index.test.ts (1)
1-147: Action required — fix remaining "volatilization" referencesSearch found leftover occurrences that need renaming or confirmation of intent — please update identifiers, comments and tests to the canonical term (e.g., "ammonia" / "ammonia emission" / "emission") and re-run the search.
Files/locations to change:
- fdm-calculator/src/index.ts:17 —
NitrogenVolatilizationNumeric- fdm-calculator/src/balance/nitrogen/types.d.ts:310 — JSDoc: "The total amount of nitrogen volatilized as ammonia."
- fdm-calculator/src/balance/nitrogen/index.ts:156 — comment: "// Calculate the amount of Nitrogen that is volatilized"
- fdm-calculator/src/balance/nitrogen/emission/index.ts — multiple comment lines mentioning "volatilization"/"volatilized" (docs and function comments)
- fdm-calculator/src/balance/nitrogen/emission/ammonia/index.ts — multiple comment lines mentioning "volatilization"/"volatilized"
- fdm-calculator/src/balance/nitrogen/emission/ammonia/residues.ts — variable names and comments (e.g.,
volatilizationResidue, related comments and aggregates)- fdm-calculator/src/balance/nitrogen/emission/ammonia/residues.test.ts — test titles mentioning "nitrogen volatilization"
Notes:
- No matches were found in fdm-app from the search output.
- After renaming, update exports/types and tests accordingly and re-run the grep to confirm no remaining occurrences.
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/compost.ts (1)
78-81: Fix Decimal constructor usage — usenew Decimal(...)Decimal is a class; calling it without
newcan throw at runtime. Update the reducer seed in compost.ts and audit similar occurrences.Files/locations to fix (found by repo search):
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/compost.ts: ~80
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/other.ts: ~84
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/mineral.ts: ~77
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/manure.ts: ~81
- fdm-calculator/src/balance/nitrogen/supply/fertilizers/compost.ts: ~63
- fdm-calculator/src/balance/nitrogen/supply/fertilizers/mineral.ts: ~61
- fdm-calculator/src/balance/nitrogen/supply/fertilizers/other.ts: ~61
- fdm-calculator/src/balance/nitrogen/supply/fertilizers/manure.ts: ~62
- fdm-calculator/src/balance/nitrogen/supply/fixation.ts: ~57
- fdm-calculator/src/balance/nitrogen/index.ts: lines ~203, 207–210, 242, 245, 248, 251
Apply this patch in compost.ts:
- const totalValue = applications.reduce((acc, application) => { - return acc.add(application.value) - }, Decimal(0)) + const totalValue = applications.reduce((acc, application) => { + return acc.add(application.value) + }, new Decimal(0))Please apply the same
new Decimal(...)fix at the other locations listed (or run a repo-wide search forDecimal(and replace occurrences that are not alreadynew Decimal().fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/mineral.ts (2)
53-60: Wrong argument todetermineMineralAmmoniaEmissionFactorand falsy check onp_ef_nh3.
- The function expects
FertilizerDetailbut receivesapplication(a fertilizer application). This will fail type-checking and at runtime.- Using
if (p_ef_nh3)treats a valid 0 emission factor as falsy and incorrectly falls back to the computed factor.Fix both and type
emissionFactorasDecimal.Apply this diff:
- // Determine emission factor - let emissionFactor = null - if (p_ef_nh3) { - emissionFactor = new Decimal(p_ef_nh3) - } else { - emissionFactor = determineMineralAmmoniaEmissionFactor(application) - } + // Determine emission factor + let emissionFactor: Decimal + if (p_ef_nh3 !== null && p_ef_nh3 !== undefined) { + emissionFactor = new Decimal(p_ef_nh3) + } else { + emissionFactor = determineMineralAmmoniaEmissionFactor(fertilizerDetail) + }
75-78: Bug: Decimal called withoutnewin reducer initial value.Same issue as in compost: use
new Decimal(0).- }, Decimal(0)) + }, new Decimal(0))fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/manure.ts (1)
79-82: Bug: Decimal called withoutnewin reducer initial value.Use
new Decimal(0)to avoid runtime errors.- }, Decimal(0)) + }, new Decimal(0))fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/mineral.test.ts (1)
48-52: Cross-file: likely incorrect argument passed to determineMineralAmmoniaEmissionFactor in mineral.tsIn mineral.ts, the emission factor fallback calls determineMineralAmmoniaEmissionFactor(application), but the test below exercises determineMineralAmmoniaEmissionFactor(fertilizerDetail). The factor formula depends on fertilizer detail fields (p_n_rt, p_no3_rt, p_nh4_rt, p_s_rt), which are not on application. This strongly suggests mineral.ts should pass fertilizerDetail.
Proposed fix in fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/mineral.ts:
- } else { - emissionFactor = determineMineralAmmoniaEmissionFactor(application) - } + } else { + emissionFactor = determineMineralAmmoniaEmissionFactor(fertilizerDetail) + }Please verify types for determineMineralAmmoniaEmissionFactor to accept FertilizerDetail.
fdm-calculator/src/balance/nitrogen/index.ts (1)
157-163: Use emission.total (not only ammonia) in field balance, and align with sign convention
- The field-level balance currently uses emission.ammonia.total, omitting nitrate. Farm-level aggregation uses emission.total. This is inconsistent.
- Combined with positive calculators, adding a positive emission inflates the balance.
Fix by using emission.total at field level. If you apply the negation in emission/index.ts (suggested separately), the math remains add-all-components as before.
- // Calculate the amount of Nitrogen that is volatilized const emission = calculateNitrogenEmission( cultivations, harvests, fertilizerApplications, cultivationDetailsMap, fertilizerDetailsMap, )And later (see below for the balance fix).
🧹 Nitpick comments (27)
.changeset/hip-lizards-hope.md (1)
5-5: Tighten release note phrasingClearer and more consistent wording.
Apply this diff:
-Update the nitrogen balance description with the refactoring of 'volatilization' to 'emission' +Update the nitrogen balance description to reflect replacing "volatilization" with "emission"..changeset/dry-bugs-care.md (1)
5-5: Fix typo and improve grammarUse “explanation” and smoother phrasing.
Apply this diff:
-Update the explaination how mineralization is calculated at nitrogen balance as it has been replaced with default values instead of MINIP calculations +Update the explanation of how mineralization is calculated in the nitrogen balance: use default values instead of MINIP calculations..changeset/loud-ears-push.md (1)
5-5: Grammar: “defaults values” → “default values”Also prefer “using MINIP”.
Apply this diff:
-Use defaults values for mineralization instead of calculating it with MINIP +Use default values for mineralization instead of calculating it using MINIP..changeset/cold-jobs-buy.md (1)
5-5: Clarify release noteMake the sentence direct and active.
Apply this diff:
-Refactored that `emission` replaces `volatilization` at nitrogen balance calculation so that it can include nitrate leaching as well +Refactor nitrogen balance: replace "volatilization" with "emission" to enable inclusion of nitrate leaching.fdm-calculator/src/balance/nitrogen/target.test.ts (1)
189-200: Confirm inclusive day-count assumption; compute expected dynamically to reduce brittlenessThe expected 40.11 suggests inclusive 183/365 scaling. If inclusive/exclusive semantics change, the hardcoded value may become brittle. Consider deriving expected from the timeframe.
Apply this diff:
- const result = calculateTargetForNitrogenBalance( + const result = calculateTargetForNitrogenBalance( cultivations, soilAnalysis, cultivationDetailsMap, shortTimeFrame, ) - expect(result.toNumber()).toBeCloseTo(40.11, 1) // Expected value adjusted for half year + const msPerDay = 24 * 60 * 60 * 1000 + const daysInclusive = + Math.round((shortTimeFrame.end.getTime() - shortTimeFrame.start.getTime()) / msPerDay) + 1 + const expected = 80 * (daysInclusive / 365) + expect(result.toNumber()).toBeCloseTo(expected, 2) // Proportional weighting based on inclusive daysfdm-calculator/src/balance/nitrogen/emission/ammonia/residues.test.ts (1)
6-6: Align test descriptions with “emission” terminologySeveral
itdescriptions still say “volatilization”, which is inconsistent with the new API and domain language.Apply these edits to the test names:
- it("should calculate nitrogen volatilization for a single cultivation with no harvests", () => { + it("should calculate nitrogen emission for a single cultivation with no harvests", () => { - it("should calculate nitrogen volatilization for a single cultivation with harvests", () => { + it("should calculate nitrogen emission for a single cultivation with harvests", () => {Also applies to: 12-16, 48-52, 102-106, 129-133, 165-169, 202-206, 250-254
fdm-calculator/src/balance/nitrogen/emission/ammonia/residues.ts (3)
10-18: Docstring still uses “volatilized”; update to “emitted”The function name and API were renamed to “emission”, but the documentation still uses “volatilized”.
- * Calculates the amount of Nitrogen volatilized via ammonia emissions from crop residues. + * Calculates the amount of Nitrogen emitted via ammonia (NH3) from crop residues. @@ - * This function determines the nitrogen volatilization from crop residues based on the cultivations performed, harvest yields, - * and cultivation details, including the nitrogen content of the crop residues. It calculates an emission factor - * based on the nitrogen content and then estimates the total nitrogen volatilization. + * This function determines the nitrogen emission from crop residues based on the cultivations performed, harvest yields, + * and cultivation details, including the nitrogen content of the crop residues. It calculates an emission factor + * based on the nitrogen content and then estimates the total nitrogen emission.
30-30: Rename local variables from “volatilization” to “emission” for consistencyLocal names and comments still use the old terminology. Renaming improves consistency and readability without changing behavior.
- const volatilizationResidue = cultivations.map((cultivation) => { + const residueEmission = cultivations.map((cultivation) => { @@ - // Aggregate the total amount of Nitrogen volatilized by crop residues - const totalValue = volatilizationResidue.reduce((acc, residue) => { + // Aggregate the total amount of Nitrogen emitted by crop residues + const totalValue = residueEmission.reduce((acc, residue) => { return acc.add(residue.value) }, new Decimal(0)) return { total: totalValue, - cultivations: volatilizationResidue, + cultivations: residueEmission, }Also applies to: 126-134
43-48: Simplify and clarify the residue presence checkRedundant comparison to
false. The truthiness check already coversfalse,null, andundefined.- if (!cultivation.m_cropresidue || cultivation.m_cropresidue === false) { + if (!cultivation.m_cropresidue) { return { id: cultivation.b_lu, value: new Decimal(0), } }fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/manure.test.ts (1)
323-323: Fix minor typo in test name (“double space”)Small readability nit.
- it("should return 0 for unsupported method on grassland", () => { + it("should return 0 for unsupported method on grassland", () => {fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/other.ts (2)
36-60: Optional: compute only for “other” types to simplify and avoid zero entriesFiltering to “other” upfront reduces noise in the applications array and avoids populating zero-value entries for manure/mineral/compost.
Apply this refactor:
- const applications = fertilizerApplications.map((application) => { - // Get fertilizerDetails of application using the Map - const fertilizerDetail = fertilizerDetailsMap.get( - application.p_id_catalogue, - ) - - if (!fertilizerDetail) { - throw new Error( - `Fertilizer application ${application.p_app_id} has no fertilizerDetails`, - ) - } - const p_nh4_rt = new Decimal(fertilizerDetail.p_nh4_rt ?? 0) - - // If the fertilizer used is not of the type other fertilizers - if ( - fertilizerDetail.p_type === "manure" || - fertilizerDetail.p_type === "mineral" || - fertilizerDetail.p_type === "compost" - ) { - return { - id: application.p_app_id, - value: new Decimal(0), - } - } + const applications = fertilizerApplications + .filter((application) => { + const fertilizerDetail = fertilizerDetailsMap.get( + application.p_id_catalogue, + ) + if (!fertilizerDetail) { + throw new Error( + `Fertilizer application ${application.p_app_id} has no fertilizerDetails`, + ) + } + return !( + fertilizerDetail.p_type === "manure" || + fertilizerDetail.p_type === "mineral" || + fertilizerDetail.p_type === "compost" + ) + }) + .map((application) => { + const fertilizerDetail = fertilizerDetailsMap.get( + application.p_id_catalogue, + )! + const p_nh4_rt = new Decimal(fertilizerDetail.p_nh4_rt ?? 0) // Determine emission factor const emissionFactor = determineManureAmmoniaEmissionFactor( application, cultivations, cultivationDetailsMap, ) // Calculate for this application the amount of Nitrogen supplied by manure const p_app_amount = new Decimal(application.p_app_amount ?? 0) const applicationValue = p_app_amount .times(p_nh4_rt) .times(emissionFactor) .dividedBy(1000) // convert from g N to kg N return { id: application.p_app_id, value: applicationValue, } - }) + })
82-85: Nit: use a consistent Decimal constructor styleElsewhere in this file you use new Decimal(0). For consistency, avoid mixing call and constructor styles.
Apply:
- const totalValue = applications.reduce((acc, application) => { - return acc.add(application.value) - }, Decimal(0)) + const totalValue = applications.reduce( + (acc, application) => acc.add(application.value), + new Decimal(0), + )fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/mineral.ts (1)
12-14: Nit: Correct function name in JSDoc.Typo in the reference to the helper.
- * `determineMineralAmmoniaEmmissionFactor`), and calculates the ammonia emissions + * `determineMineralAmmoniaEmissionFactor`), and calculates the ammonia emissionsfdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/other.test.ts (2)
114-151: Fix misleading test description (asserts 0.01, not 0).The test name says it should return 0 for “other”, but the assertions expect 0.01 (which is correct per current logic). Rename the test for clarity.
- it("should return 0 for applications of 'other' type as per current function logic", () => { + it("should calculate emissions for 'other' type as per current function logic", () => {
34-112: Optional: Make expectations resilient to ordering.If
applicationsordering ever changes, index-based assertions will become brittle. Consider asserting by locating entries byid.I can provide a small helper to map results by id if you'd like.
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/manure.ts (2)
103-107: Add explicit return type for helper.Annotate the return type to
Decimalfor stronger typing and editor support.export function determineManureAmmoniaEmissionFactor( fertilizerApplication: FertilizerApplication, cultivations: FieldInput["cultivations"], cultivationDetails: Map<string, CultivationDetail>, -) { +): Decimal {
114-139: Nit: Typo in variable namecurrentCultvations.Rename to
currentCultivationsfor readability and consistency.- const currentCultvations = cultivations.filter((cultivation) => { + const currentCultivations = cultivations.filter((cultivation) => { @@ - const isGrasslands = currentCultvations.map((x) => { + const isGrasslands = currentCultivations.map((x) => { @@ - const isCropland = currentCultvations.length > 0 + const isCropland = currentCultivations.length > 0fdm-calculator/src/balance/nitrogen/emission/index.ts (2)
10-19: JSDoc is outdated and contains typos; update terminology and return description
- Use “emission” (not “volatilization”) consistently.
- Fix typos: “appplications” → “applications”, “NitrogenEmmissionAmmonia” → correct type name.
- Document all parameters including fertilizerApplications and fertilizerDetailsMap.
Apply:
/** - * Calculates the total nitrogen volatilization from a field, through ammonia emissions from fertilizer appplications and crop residues. + * Calculates the total nitrogen emission for a field by orchestrating ammonia (NH3) and nitrate (NO3) pathways. * - * This function orchestrates the calculation of ammonia emission by calling separate functions for fertilizer applications and - * residue volatilization, then aggregates the results. + * This function orchestrates the calculation of ammonia emissions from fertilizer applications and residues, + * and nitrate leaching, then aggregates the results. * @param cultivations - A list of cultivations on the field. * @param harvests - A list of harvests from the field. - * @param cultivationDetailsMap - A map containing details for each cultivation, including its nitrogen content and residue management practices. - * @returns The NitrogenEmmissionAmmonia object containing the total amount of Nitrogen volatilized and the individual ammonia values. + * @param fertilizerApplications - Fertilizer applications on the field. + * @param cultivationDetailsMap - Map with cultivation details including N content and residue practices. + * @param fertilizerDetailsMap - Map with fertilizer details including types and emission factors. + * @returns NitrogenEmission object with totals and a breakdown for ammonia and nitrate. */ @@ - /** Calculate the total amount of Nitrogen volatilized as Ammonia by fertilizer */ + /** Calculate ammonia emissions (NH3) from fertilizers and residues */ @@ - /** Calculate the total amount of Nitrogen volatilized as Ammonia by crop residues */ + /** Calculate nitrate (NO3) emissions/leaching */Also applies to: 27-27, 36-36
37-37: Nitrate scaffold has no inputs; confirm intended APIcalculateNitrogenEmissionViaNitrate() currently takes no inputs. If nitrate leaching should depend on field, soil, or timeframe, consider adding appropriate parameters now to avoid API churn later.
Would you like me to draft a nitrate API that takes SoilAnalysis, cultivations, and timeframe?
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/mineral.test.ts (2)
60-60: Spelling nit: “Emmission” → “Emission” in test descriptionPurely cosmetic; improves clarity in test output.
- it("should calculate ammonia emissions for mineral applications without p_ef_nh3 (using determineMineralAmmoniaEmmissionFactor)", () => { + it("should calculate ammonia emissions for mineral applications without p_ef_nh3 (using determineMineralAmmoniaEmissionFactor)", () => {
164-164: Spelling nit in describe blockRename “Emmission” to “Emission”.
-describe("determineMineralAmmoniaEmmissionFactor", () => { +describe("determineMineralAmmoniaEmissionFactor", () => {fdm-calculator/src/balance/nitrogen/supply/mineralization.ts (3)
21-26: Function signature expanded; update JSDoc to match API and behaviorThe JSDoc above still references MINIP-only and doesn’t document cultivations/cultivationDetails. It should reflect defaults-based, timeframe-weighted logic and the per-year breakdown returned.
Would you like me to provide a replacement JSDoc block?
27-31: Typo: “minerlizationPerYear” → “mineralizationPerYear” and ensure correct typingRename the variable for readability and consistency. Also, consider typing the array to avoid any implicit any.
- const minerlizationPerYear = [] + const mineralizationPerYear: { year: number; value: Decimal }[] = [] @@ - minerlizationPerYear.push({ + mineralizationPerYear.push({ @@ - years: minerlizationPerYear, + years: mineralizationPerYear,Also applies to: 81-85, 89-91
101-123: Defaults might leave non-dalgrond/veen soils at 0; confirm intended behaviorCurrently only “dalgrond” and “veen” are mapped; all other soil types default to 0. If that’s intentional, fine; otherwise consider a sensible default or error/warn when soil type is unmapped.
I can extend this switch to other soil types if you provide the intended defaults.
fdm-calculator/src/balance/nitrogen/supply/mineralization.test.ts (1)
115-115: Correct the test description to match the expected behavior.The test description says "should return mineralization for arable land for grassland not in the May 15 - July 15 window" but the cultivation ends on May 14, which means it doesn't overlap with the May 15 - July 15 window. The test is expecting arable land mineralization (20), not grassland mineralization. Consider clarifying the test description.
Apply this diff to clarify the test description:
- it("should return mineralization for arable land for grassland not in the May 15 - July 15 window", () => { + it("should return arable land mineralization when grassland cultivation doesn't overlap May 15 - July 15 window", () => {fdm-calculator/src/balance/nitrogen/emission/ammonia/index.ts (2)
12-12: Fix typo in documentation: "appplications" should be "applications".There's a typo in the JSDoc comment.
Apply this diff to fix the typo:
- * Calculates the total nitrogen volatilization from a field, through ammonia emissions from fertilizer appplications and crop residues. + * Calculates the total nitrogen volatilization from a field, through ammonia emissions from fertilizer applications and crop residues.
19-19: Fix typo in documentation: "Emmission" should be "Emission".There's a typo in the JSDoc @returns description.
Apply this diff to fix the typo:
- * @returns The NitrogenEmmissionAmmonia object containing the total amount of Nitrogen volatilized and the individual ammonia values. + * @returns The NitrogenEmissionAmmonia object containing the total amount of Nitrogen volatilized and the individual ammonia values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (30)
.changeset/cold-jobs-buy.md(1 hunks).changeset/dry-bugs-care.md(1 hunks).changeset/hip-lizards-hope.md(1 hunks).changeset/loud-ears-push.md(1 hunks)fdm-app/app/components/blocks/balance/nitrogen-chart.tsx(4 hunks)fdm-app/app/components/blocks/balance/nitrogen-details.tsx(7 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen.$b_id.tsx(2 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx(2 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/compost.test.ts(5 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/compost.ts(2 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/index.ts(3 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/manure.test.ts(4 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/manure.ts(2 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/mineral.test.ts(5 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/mineral.ts(2 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/other.test.ts(4 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/other.ts(2 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/index.ts(2 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/residues.test.ts(7 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/residues.ts(2 hunks)fdm-calculator/src/balance/nitrogen/emission/index.ts(1 hunks)fdm-calculator/src/balance/nitrogen/emission/nitrate/index.ts(1 hunks)fdm-calculator/src/balance/nitrogen/index.test.ts(2 hunks)fdm-calculator/src/balance/nitrogen/index.ts(8 hunks)fdm-calculator/src/balance/nitrogen/supply/index.ts(1 hunks)fdm-calculator/src/balance/nitrogen/supply/mineralization.test.ts(1 hunks)fdm-calculator/src/balance/nitrogen/supply/mineralization.ts(2 hunks)fdm-calculator/src/balance/nitrogen/target.test.ts(1 hunks)fdm-calculator/src/balance/nitrogen/types.d.ts(8 hunks)fdm-docs/docs/balance/01-nitrogen.md(5 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-05-26T10:32:00.674Z
Learnt from: SvenVw
PR: SvenVw/fdm#134
File: fdm-calculator/src/balance/nitrogen/index.ts:162-168
Timestamp: 2025-05-26T10:32:00.674Z
Learning: In the nitrogen balance calculation system (fdm-calculator), removal and volatilization values are negative by definition. This means the balance calculation using supply.total.add(removal.total).add(volatilization.total) is correct, as it effectively computes supply - |removal| - |volatilization|.
Applied to files:
.changeset/hip-lizards-hope.md.changeset/cold-jobs-buy.mdfdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsxfdm-calculator/src/balance/nitrogen/index.tsfdm-calculator/src/balance/nitrogen/types.d.tsfdm-docs/docs/balance/01-nitrogen.md
📚 Learning: 2025-05-26T10:32:15.538Z
Learnt from: SvenVw
PR: SvenVw/fdm#134
File: fdm-calculator/src/balance/nitrogen/index.ts:236-238
Timestamp: 2025-05-26T10:32:15.538Z
Learning: In nitrogen balance calculations for agricultural systems, removal and volatilization are calculated as negative values by definition since they represent nitrogen losses from the system. The balance calculation uses addition (.add()) for all components because removal and volatilization are already negative, so adding them effectively subtracts the losses from the supply.
Applied to files:
.changeset/cold-jobs-buy.mdfdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsxfdm-calculator/src/balance/nitrogen/index.tsfdm-calculator/src/balance/nitrogen/types.d.tsfdm-docs/docs/balance/01-nitrogen.md
📚 Learning: 2025-08-11T11:55:26.053Z
Learnt from: SvenVw
PR: SvenVw/fdm#233
File: fdm-app/app/integrations/nmi.ts:54-0
Timestamp: 2025-08-11T11:55:26.053Z
Learning: The NMI API Estimates endpoint (`https://api.nmi-agro.nl/estimates`) always returns the fields `b_gwl_ghg`, `b_gwl_glg`, and `cultivations` according to its specification. These fields should be kept as required (not optional) in the TypeScript return type and Zod validation schema in `fdm-app/app/integrations/nmi.ts`.
Applied to files:
fdm-calculator/src/balance/nitrogen/target.test.tsfdm-calculator/src/balance/nitrogen/index.test.ts
📚 Learning: 2025-05-27T19:56:48.556Z
Learnt from: SvenVw
PR: SvenVw/fdm#143
File: fdm-app/app/components/custom/balance/nitrogen-chart.tsx:73-85
Timestamp: 2025-05-27T19:56:48.556Z
Learning: In nitrogen balance charts, supply should be in a separate stack from removal and emission. Supply represents nitrogen inputs while removal and emission represent different types of nitrogen outputs, so they should be visually grouped differently using different stackId values.
Applied to files:
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsxfdm-app/app/components/blocks/balance/nitrogen-chart.tsxfdm-calculator/src/balance/nitrogen/types.d.ts
📚 Learning: 2025-01-23T15:18:57.212Z
Learnt from: SvenVw
PR: SvenVw/fdm#49
File: fdm-core/src/db/schema.ts:407-426
Timestamp: 2025-01-23T15:18:57.212Z
Learning: In the farm data model, each cultivation (b_lu) can have only one termination date but can have multiple harvest dates. This is enforced through the database schema where cultivationTerminating uses b_lu as primary key while cultivationHarvesting uses a composite primary key of b_id_harvestable and b_lu.
Applied to files:
fdm-calculator/src/balance/nitrogen/index.test.ts
📚 Learning: 2025-02-13T08:35:59.306Z
Learnt from: SvenVw
PR: SvenVw/fdm#71
File: fdm-app/app/routes/farm.$b_id_farm.field.$b_id.cultivation.$b_lu.harvest.$b_id_harvesting.tsx:114-124
Timestamp: 2025-02-13T08:35:59.306Z
Learning: The HarvestForm component in fdm-app expects undefined (not 0) for b_lu_yield when no yield information is available, as 0 would incorrectly imply that yield data exists.
Applied to files:
fdm-calculator/src/balance/nitrogen/emission/ammonia/residues.ts
📚 Learning: 2025-07-21T12:06:07.070Z
Learnt from: SvenVw
PR: SvenVw/fdm#156
File: fdm-calculator/src/norms/nl/2025/stikstofgebruiksnorm.ts:295-303
Timestamp: 2025-07-21T12:06:07.070Z
Learning: Functions in the fdm-calculator with "NL2025" in their names are specifically designed for Netherlands 2025 agricultural norms calculation and hardcoded 2025 dates are appropriate in this context, as different years would have separate calculation modules.
Applied to files:
fdm-app/app/components/blocks/balance/nitrogen-details.tsx
🧬 Code Graph Analysis (14)
fdm-calculator/src/balance/nitrogen/emission/nitrate/index.ts (1)
fdm-calculator/src/balance/nitrogen/types.d.ts (1)
NitrogenEmissionNitrate(296-301)
fdm-calculator/src/balance/nitrogen/emission/index.ts (4)
fdm-core/src/db/schema.ts (1)
cultivations(289-301)fdm-calculator/src/balance/nitrogen/types.d.ts (4)
FieldInput(395-415)CultivationDetail(420-429)FertilizerDetail(434-443)NitrogenEmission(307-317)fdm-calculator/src/balance/nitrogen/emission/ammonia/index.ts (1)
calculateNitrogenEmissionViaAmmonia(21-51)fdm-calculator/src/balance/nitrogen/emission/nitrate/index.ts (1)
calculateNitrogenEmissionViaNitrate(4-10)
fdm-calculator/src/balance/nitrogen/supply/index.ts (2)
fdm-calculator/src/balance/nitrogen/supply/mineralization.ts (1)
calculateNitrogenSupplyBySoilMineralization(20-92)fdm-core/src/db/schema.ts (2)
cultivations(289-301)soilAnalysis(605-648)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/compost.test.ts (2)
fdm-calculator/src/balance/nitrogen/types.d.ts (3)
FieldInput(395-415)CultivationDetail(420-429)FertilizerDetail(434-443)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/compost.ts (1)
calculateNitrogenEmissionViaAmmoniaByCompost(24-86)
fdm-calculator/src/balance/nitrogen/emission/ammonia/residues.test.ts (2)
fdm-calculator/src/balance/nitrogen/types.d.ts (2)
FieldInput(395-415)CultivationDetail(420-429)fdm-calculator/src/balance/nitrogen/emission/ammonia/residues.ts (1)
calculateNitrogenEmissionViaAmmoniaByResidues(19-135)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/other.test.ts (2)
fdm-calculator/src/balance/nitrogen/types.d.ts (3)
FieldInput(395-415)CultivationDetail(420-429)FertilizerDetail(434-443)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/other.ts (1)
calculateNitrogenEmissionViaAmmoniaByOtherFertilizers(24-90)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/mineral.test.ts (2)
fdm-calculator/src/balance/nitrogen/types.d.ts (2)
FieldInput(395-415)FertilizerDetail(434-443)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/mineral.ts (1)
calculateNitrogenEmissionViaAmmoniaByMineralFertilizers(21-83)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/manure.test.ts (2)
fdm-calculator/src/balance/nitrogen/types.d.ts (3)
FieldInput(395-415)CultivationDetail(420-429)FertilizerDetail(434-443)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/manure.ts (1)
calculateNitrogenEmissionViaAmmoniaByManure(25-87)
fdm-calculator/src/balance/nitrogen/index.ts (2)
fdm-calculator/src/balance/nitrogen/emission/index.ts (1)
calculateNitrogenEmission(20-46)fdm-calculator/src/balance/nitrogen/types.d.ts (1)
NitrogenBalance(354-379)
fdm-calculator/src/balance/nitrogen/emission/ammonia/index.ts (3)
fdm-calculator/src/balance/nitrogen/types.d.ts (4)
FieldInput(395-415)CultivationDetail(420-429)FertilizerDetail(434-443)NitrogenEmissionAmmonia(273-290)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/index.ts (1)
calculateNitrogenEmissionViaAmmoniaByFertilizers(21-75)fdm-calculator/src/balance/nitrogen/emission/ammonia/residues.ts (1)
calculateNitrogenEmissionViaAmmoniaByResidues(19-135)
fdm-calculator/src/balance/nitrogen/types.d.ts (1)
fdm-calculator/src/index.ts (1)
NitrogenBalanceNumeric(6-6)
fdm-calculator/src/balance/nitrogen/supply/mineralization.test.ts (2)
fdm-calculator/src/balance/nitrogen/types.d.ts (4)
CultivationDetail(420-429)SoilAnalysisPicked(381-390)FieldInput(395-415)NitrogenBalanceInput(448-456)fdm-calculator/src/balance/nitrogen/supply/mineralization.ts (1)
calculateNitrogenSupplyBySoilMineralization(20-92)
fdm-app/app/components/blocks/balance/nitrogen-details.tsx (2)
fdm-calculator/src/balance/nitrogen/types.d.ts (1)
NitrogenEmissionNumeric(547-550)fdm-core/src/db/schema.ts (1)
fertilizers(131-139)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/index.ts (6)
fdm-core/src/db/schema.ts (1)
cultivations(289-301)fdm-calculator/src/balance/nitrogen/types.d.ts (4)
FieldInput(395-415)CultivationDetail(420-429)FertilizerDetail(434-443)NitrogenEmissionAmmoniaFertilizers(190-251)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/mineral.ts (1)
calculateNitrogenEmissionViaAmmoniaByMineralFertilizers(21-83)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/manure.ts (1)
calculateNitrogenEmissionViaAmmoniaByManure(25-87)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/compost.ts (1)
calculateNitrogenEmissionViaAmmoniaByCompost(24-86)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/other.ts (1)
calculateNitrogenEmissionViaAmmoniaByOtherFertilizers(24-90)
🪛 LanguageTool
.changeset/dry-bugs-care.md
[grammar] ~5-~5: Ensure spelling is correct
Context: ...svenvw/fdm-docs": minor --- Update the explaination how mineralization is calculated at nit...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
fdm-docs/docs/balance/01-nitrogen.md
[grammar] ~117-~117: There might be a mistake here.
Context: ...al N Mineralization (kg N / ha / year) | | :--- | :--- | :--- | | dalgrond | Ar...
(QB_NEW_EN)
[grammar] ~118-~118: There might be a mistake here.
Context: ... N / ha / year) | | :--- | :--- | :--- | | dalgrond | Arable or Grassland | 20 ...
(QB_NEW_EN)
[grammar] ~119-~119: There might be a mistake here.
Context: ... dalgrond | Arable or Grassland | 20 | | veen | Grassland | 160 | | veen | ...
(QB_NEW_EN)
[grammar] ~120-~120: There might be a mistake here.
Context: ...land | 20 | | veen | Grassland | 160 | | veen | Arable Land | 20 | | Other | ...
(QB_NEW_EN)
[grammar] ~121-~121: There might be a mistake here.
Context: ...nd | 160 | | veen | Arable Land | 20 | | Other | Arable or Grassland | 0 | ...
(QB_NEW_EN)
[grammar] ~243-~243: There might be a mistake here.
Context: ... field in the output. * Farm Balance: 1. The total N supplied, removed, and total...
(QB_NEW_EN)
[grammar] ~244-~244: There might be a mistake here.
Context: ...per field) are summed across all fields. 2. These total farm-level amounts (in kg N)...
(QB_NEW_EN)
[grammar] ~250-~250: There might be a mistake here.
Context: ...removal, and total emission (kg N / ha). * A list of balances for each field (`Nitr...
(QB_NEW_EN)
[style] ~256-~256: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...val (total, harvests, residues). * Detailed breakdown of emission (total, plus sub-...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.17.2)
fdm-docs/docs/balance/01-nitrogen.md
124-124: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
252-252: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
253-253: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
254-254: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
255-255: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
256-256: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🔇 Additional comments (50)
fdm-calculator/src/balance/nitrogen/target.test.ts (1)
22-23: LGTM: per-cultivation date fields align with timeframe-weighted logicAdding b_lu_start and b_lu_end on cultivations is consistent with the new per-timeframe mineralization/emission pathways.
fdm-calculator/src/balance/nitrogen/emission/ammonia/residues.test.ts (1)
3-4: API rename and type import path updates look correctThe test now targets
calculateNitrogenEmissionViaAmmoniaByResiduesand imports types from the shared../../types. This aligns with the emission terminology refactor.fdm-calculator/src/balance/nitrogen/emission/ammonia/residues.ts (1)
19-24: API rename is consistent and clearThe exported function name aligns with the wider refactor from volatilization to emission.
fdm-calculator/src/balance/nitrogen/index.test.ts (2)
37-38: Including cultivation period on the test input is correctAdding
b_lu_startandb_lu_endaligns with the updatedFieldInputtype and timeframe-weighted logic mentioned in the refactor.
113-113: Assertion updated to emission propertyChecking
result.emissioninstead ofresult.volatilizationmatches the domain rename.fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/manure.test.ts (1)
7-7: Rename adoption looks good; tests still assert expected factors and totals
- Imports updated to the shared types path and the renamed API
calculateNitrogenEmissionViaAmmoniaByManure.- Call sites and describe blocks are consistent with the new naming.
Also applies to: 9-11, 13-13, 20-25, 102-107, 151-156, 181-187
fdm-app/app/components/blocks/balance/nitrogen-chart.tsx (2)
16-22: Prop rename to emission is correct and consistent with the domain shiftThe prop signature update from volatilization to emission looks correct and aligns with the wider refactor.
40-43: Good: supply stacked separately from outputs; emission label/color updatedStack IDs keep supply separate from removal/emission, matching prior guidance. Label and color for emission are consistent with the new terminology and theming.
Also applies to: 83-87
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/other.ts (1)
24-29: Rename verified — no lingering references to the old APINo matches found for calculateAmmoniaEmissionsByOtherFertilizers. The new function calculateNitrogenEmissionViaAmmoniaByOtherFertilizers is imported and used in:
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/index.ts — import at line 10, invocation at line 54
fdm-calculator/src/balance/nitrogen/supply/index.ts (1)
62-67: Signature update wired correctly for mineralizationDefinition has the 4-argument signature and all call sites pass four arguments — no stale 2-arg calls found.
- fdm-calculator/src/balance/nitrogen/supply/mineralization.ts — export function calculateNitrogenSupplyBySoilMineralization(...) at line ~20 (4 args)
- fdm-calculator/src/balance/nitrogen/supply/index.ts — call at lines ~62–65 passes (cultivations, soilAnalysis, cultivationDetailsMap, timeFrame)
- fdm-calculator/src/balance/nitrogen/supply/mineralization.test.ts — calls at lines ~76, 105, 134, 163, 192, 221 all pass 4 args
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx (2)
287-289: UI now reads total emission at farm level — correctDisplaying resolvedNitrogenBalanceResult.emission (top-level numeric) is consistent with the aggregated API shape at the farm level.
322-325: Chart input aligned to emission propChart call site updated to emission={...emission}, consistent with the component’s new signature and the broader refactor.
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen.$b_id.tsx (2)
334-335: Field “Emissie” card uses result.emission.total — correctMatches the new API and UI terminology.
363-364: Chart call updated to emission total — correctThe NitrogenBalanceChart now correctly receives emission={result.emission.total}.
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/compost.ts (1)
24-29: Rename and type import path look consistent with the new API surface.Export name and types path align with the broader “emission via ammonia” refactor.
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/mineral.ts (2)
21-25: Rename and type import path look consistent with the new API surface.Public API rename matches other modules; parameter/return types remain stable.
61-67: Domain check: Confirm using total N (p_n_rt) as the multiplier.Manure/compost use
p_nh4_rt, while mineral usesp_n_rt. If the emission factor is meant to scale ammonium-N only, this may be inconsistent. If total-N is intended for mineral, ignore this.Can you confirm the intended basis (NH4-N vs total N) for mineral fertilizer emissions per the model spec? I can adjust the implementation and tests accordingly if needed.
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/other.test.ts (1)
7-9: Test updates to new API name and types path are consistent.Imports and describe block match the refactored function name.
Also applies to: 15-15
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/manure.ts (1)
25-31: Rename and type import path look consistent with the new API surface.Public API rename aligns with the module’s new naming.
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/compost.test.ts (2)
6-9: Test updates to new API name and types path are consistent.Imports and describe block updated correctly for the renamed function.
Also applies to: 9-9
96-102: LGTM: Calls reflect the new API and assert expected behavior.The scenarios cover empty input, compost-only, non-compost, missing details, and mixed types; calculations match the manure-factor logic used by compost.
Also applies to: 144-150, 172-179, 251-257
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/mineral.test.ts (2)
8-8: LGTM: test suite name matches the renamed APIThe suite targets calculateNitrogenEmissionViaAmmoniaByMineralFertilizers and aligns with the refactor.
156-160: Good negative test: missing fertilizer detailsThis assertion validates error handling for missing fertilizer details. Solid coverage.
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/index.ts (2)
21-26: LGTM: API rename and aggregator composition are coherentThe aggregator cleanly composes the specific calculators, and the return shape matches NitrogenEmissionAmmoniaFertilizers.
35-41: LGTM: correct dependency injection for manure pathPassing cultivations and cultivationDetailsMap is consistent with the manure factor logic.
fdm-calculator/src/balance/nitrogen/index.ts (2)
24-33: Doc comment updated to “emission”: goodTerminology change to emission is reflected here.
206-210: Confirm negative emission totals are supported by all downstream consumersFound usages — please verify these consumers expect negative emission values (or adjust sign/formatting):
- fdm-calculator/src/balance/nitrogen/index.ts:175 — balance: supply.total.add(removal.total).add(emission.ammonia.total)
- fdm-calculator/src/balance/nitrogen/index.ts:233 — fieldBalance.emission.total.times(fieldArea)
- fdm-app/app/components/blocks/balance/nitrogen-details.tsx:566 — UI: "Emissie (Totaal): {emission.total} kg N / ha"
- fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen.$b_id.tsx:334, 363 — {result.emission.total} / emission={result.emission.total}
Action: if emission.* values are negative by definition, keep aggregation as supply + removal + emission; otherwise invert or format (abs/sign) at the consumer layer as appropriate.
fdm-calculator/src/balance/nitrogen/types.d.ts (2)
104-107: LGTM! The newyearsproperty enhances mineralization tracking.The addition of per-year mineralization data provides better granularity for tracking nitrogen supply from soil mineralization across multiple years, which aligns well with the timeframe-weighted logic mentioned in the summary.
293-317: Well-structured emission type hierarchy.The new
NitrogenEmissionNitrateandNitrogenEmissiontypes provide a clear separation between ammonia and nitrate emissions, which aligns with the broader terminology shift from volatilization to emission. This structure supports future extensibility for additional emission types.fdm-calculator/src/balance/nitrogen/supply/mineralization.test.ts (5)
12-49: LGTM! Comprehensive test data setup.The mock cultivation details provide good coverage with both grassland and non-grassland crop rotations, which effectively tests the new mineralization logic based on land use type.
61-84: LGTM! Test correctly validates non-grassland behavior.The test properly verifies that non-grassland cultivations (corn) return zero mineralization, aligning with the new default-based calculation logic.
144-171: LGTM! Good edge case handling.The test properly validates that undefined cultivation end dates are treated as extending indefinitely, which is a sensible default behavior.
173-200: LGTM! Accurate partial year calculation.The test correctly verifies that mineralization is pro-rated based on the actual timeframe duration (180 days out of 365), with the expected value of ~78.9 kg N/ha being accurate for the calculation.
202-229: LGTM! Multi-year calculation is properly tested.The test correctly validates that mineralization accumulates over multiple years (2 years × 160 kg N/ha = 320 kg N/ha total for grassland on veen soil).
fdm-app/app/components/blocks/balance/nitrogen-details.tsx (5)
11-11: LGTM! Type import correctly updated to emission terminology.The import change from
NitrogenVolatilizationNumerictoNitrogenEmissionNumericaligns with the broader terminology shift across the codebase.
377-402: LGTM! Function signatures properly updated for emission types.The function parameter types and internal calls have been correctly updated to use the emission type hierarchy.
404-452: LGTM! Consistent renaming to emission terminology.All function names, parameter types, and section keys have been consistently updated from "volatilization" to "emission" terminology.
454-495: LGTM! Residues emission handling properly updated.The function has been correctly renamed and updated to use the appropriate emission types and section keys.
557-583: LGTM! Main render function correctly updated.The main
renderEmissionfunction and its usage in the component have been properly updated to use emission data instead of volatilization data.fdm-calculator/src/balance/nitrogen/emission/ammonia/index.ts (3)
21-27: LGTM! Function signature properly updated.The function has been correctly renamed to
calculateNitrogenEmissionViaAmmoniawith the appropriate return typeNitrogenEmissionAmmonia.
29-41: LGTM! Correct orchestration of ammonia emission calculations.The function properly delegates to specialized calculators for fertilizers and residues emissions.
43-51: LGTM! Proper aggregation of ammonia emissions.The ammonia object is correctly constructed with total emissions and component breakdowns, maintaining the expected structure.
fdm-docs/docs/balance/01-nitrogen.md (8)
14-26: LGTM! Clear explanation of the updated nitrogen balance formula.The documentation correctly reflects the terminology shift from "volatilization" to "emission" and provides a clear formula with well-defined components.
110-126: LGTM! Well-documented mineralization calculation method.The new default-value approach with grassland determination logic is clearly explained, including the lookup table for different soil types and land uses.
152-157: LGTM! Comprehensive emission section with future extensibility.The renamed emission section properly introduces the concept and mentions future nitrate leaching implementation.
160-165: Good placeholder for future nitrate leaching implementation.The documentation clearly indicates that nitrate leaching is planned but not yet implemented, returning 0 for now.
166-216: LGTM! Detailed ammonia emission calculations for fertilizers.The documentation provides comprehensive formulas and emission factors for different fertilizer types and application methods.
219-235: LGTM! Clear documentation of residue emission calculations.The emission factor formula and conditions are well-documented.
241-246: LGTM! Clear field and farm balance descriptions.The documentation properly explains the field surplus calculation and farm-level aggregation using the updated emission terminology.
249-257: LGTM! Output structure properly documented.The output description correctly reflects the emission terminology and breakdown structure.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
fdm-calculator/src/balance/nitrogen/supply/fertilizers/mineral.test.ts (1)
34-59: Reduce fixture duplication with a small builder/helperYou’re repeating the same four nullable fields per entry. A tiny helper keeps tests resilient to future shape changes and improves readability.
Apply this change within the map initialization:
- const fertilizerDetailsMap = new Map<string, FertilizerDetail>([ - [ - "app1", - { - p_id_catalogue: "app1", - p_n_rt: 20, - p_type: "mineral", - p_ef_nh3: null, - p_nh4_rt: null, - p_no3_rt: null, - p_s_rt: null, - }, - ], - [ - "app2", - { - p_id_catalogue: "app2", - p_n_rt: 15, - p_type: "mineral", - p_ef_nh3: null, - p_nh4_rt: null, - p_no3_rt: null, - p_s_rt: null - }, - ], - ]) + const fertilizerDetailsMap = new Map<string, FertilizerDetail>([ + ["app1", makeMineralDetail("app1", 20)], + ["app2", makeMineralDetail("app2", 15)], + ])Add this helper near the top of the file (or in a shared test utils module):
function makeMineralDetail(id: string, p_n_rt: number): FertilizerDetail { return { p_id_catalogue: id, p_n_rt, p_type: "mineral", p_ef_nh3: null, p_nh4_rt: null, p_no3_rt: null, p_s_rt: null, } }fdm-calculator/src/balance/nitrogen/supply/mineralization.test.ts (2)
144-171: Clarify test name to reflect behavior when grassland is outside the windowThe current title reads a bit contradictory. Suggest rewording to clearly state that grassland is treated as arable when it does not overlap the May 15–July 15 window.
Apply:
-it("should return mineralization for arable land for grassland not in the May 15 - July 15 window", () => { +it("should treat as arable when grassland does not overlap the May 15–July 15 window", () => {
231-258: Add assertions for per-year breakdown and lock leap-year semanticsSince the API returns a years breakdown and the implementation deliberately uses a 365-day denominator even for leap years, assert both years and values to lock the intended design and prevent regressions.
Apply:
const result = calculateNitrogenSupplyBySoilMineralization( cultivations, soilAnalysis, mockCultivationDetails, timeFrame, ) - expect(result.total.toNumber()).toBeCloseTo(320, 0) + expect(result.total.toNumber()).toBeCloseTo(320, 0) + // Also verify per-year breakdown (365-day denominator for both years) + expect(result.years).toHaveLength(2) + const y2023 = result.years.find((y) => y.year === 2023)! + const y2024 = result.years.find((y) => y.year === 2024)! + expect(y2023.value.toNumber()).toBeCloseTo(160, 0) + expect(y2024.value.toNumber()).toBeCloseTo(160, 0)Question for the team: Do we want to keep the 365-day divisor for leap years by design? If not, I can add/adjust tests to reflect 366-day weighting for leap years and open a follow-up refactor PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
fdm-calculator/src/balance/nitrogen/supply/fertilizers/mineral.test.ts(2 hunks)fdm-calculator/src/balance/nitrogen/supply/mineralization.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
fdm-calculator/src/balance/nitrogen/supply/mineralization.test.ts (2)
fdm-calculator/src/balance/nitrogen/types.d.ts (4)
CultivationDetail(420-429)SoilAnalysisPicked(381-390)FieldInput(395-415)NitrogenBalanceInput(448-456)fdm-calculator/src/balance/nitrogen/supply/mineralization.ts (1)
calculateNitrogenSupplyBySoilMineralization(20-92)
🔇 Additional comments (7)
fdm-calculator/src/balance/nitrogen/supply/fertilizers/mineral.test.ts (1)
41-44: LGTM: test fixtures aligned with the extended FertilizerDetail shapeAdding the new emission-related fields keeps the fixture in sync with the updated types. No change to test behavior.
fdm-calculator/src/balance/nitrogen/supply/mineralization.test.ts (6)
12-49: LGTM: Well-scoped cultivationDetails fixturesThe fixtures cover grass vs non-grass croprotation and exercise the intended map lookup by catalogue id. Looks good.
61-84: LGTM: Non-grassland returns zeroGood sanity check ensuring corn (non-grass) yields zero mineralization for zand under the new API.
86-113: LGTM: Veen grassland yields expected totalCovers grassland detection and full-year weighting; the 160 expectation matches the defaults-driven approach.
115-142: LGTM: Dalgrond caseValidates dalgrond defaults under full-year coverage; this strengthens soil-type branching coverage.
173-200: LGTM: Undefined end date scenario coveredGreat addition to lock in behavior for open-ended cultivations (treated as far-future end date) within the window.
51-59: Confirmed — SoilAnalysis fields are nullable; no changes requiredThe repository shows the soil analysis properties used in the test are defined to allow null, so assigning null in SoilAnalysisPicked is valid.
- fdm-app/app/components/blocks/soil/types.d.ts — fields (a_c_of, a_cn_fr, a_density_sa, a_n_rt, a_som_loi, b_gwl_class) are typed as number | undefined | null.
- fdm-calculator/src/balance/nitrogen/types.d.ts — SoilAnalysisPicked is a Pick<SoilAnalysis,...> and thus inherits the same nullability.
- fdm-core/src/soil.d.ts & fdm-core/src/db/schema.ts — SoilAnalysis properties reference schema.soilAnalysisTypeSelect[...] and the DB schema allows nullable numeric values.
- fdm-calculator/src/balance/nitrogen/index.ts — code initializes soilAnalysis properties to null using fdmSchema.soilAnalysisTypeSelect[...] (explicit null assignments).
- fdm-calculator/src/conversions/soil.ts — conversion helpers return null when inputs are missing (consistent with nullable types).
- fdm-calculator/src/balance/nitrogen/supply/mineralization.test.ts — the mock assigning nulls is consistent with the above.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (6)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/other.ts (1)
68-75: Action: update comment in other.ts — ammonia sign convention verifiedChecked the codebase: all ammonia emission calculators use .times(-1) (so they return negative values), the aggregator adds emission.ammonia.total directly (no extra negation), and I found no consumers negating/abs-ing emission.ammonia.total.
Files of interest
- Change comment: fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/other.ts (lines ~68–75)
- Ammonia calculators returning negative (.times(-1)):
- fdm-calculator/src/balance/nitrogen/emission/ammonia/residues.ts:118
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/manure.ts:71
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/mineral.ts:67
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/compost.ts:70
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/other.ts:74
- Aggregator (no extra negation): fdm-calculator/src/balance/nitrogen/index.ts:175
- UI renders emission.ammonia (no negation found): fdm-app/app/components/blocks/balance/nitrogen-details.tsx
Apply wording fix:
- // Calculate for this application the amount of Nitrogen supplied by manure + // Calculate for this application the ammonia-N emitted from other fertilizersfdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/mineral.ts (2)
53-59: Bug: wrong argument passed to determineMineralAmmoniaEmissionFactor and brittle null-check on p_ef_nh3.
- You’re passing
application(a FertilizerApplication) wheredetermineMineralAmmoniaEmissionFactorexpects aFertilizerDetail. This will compute EF with zeros and break the calculation/tests.if (p_ef_nh3)treats 0 as falsy. If 0 is a valid EF, we should use a nullish check.Apply the following fix and tighten types so
emissionFactoris guaranteed a Decimal:- // Determine emission factor - let emissionFactor = null - if (p_ef_nh3) { - emissionFactor = new Decimal(p_ef_nh3) - } else { - emissionFactor = determineMineralAmmoniaEmissionFactor(application) - } + // Determine emission factor + let emissionFactor: Decimal + if (p_ef_nh3 !== null && p_ef_nh3 !== undefined) { + emissionFactor = new Decimal(p_ef_nh3) + } else { + emissionFactor = determineMineralAmmoniaEmissionFactor(fertilizerDetail) + }
61-68: Update inline comment in mineral fertilizer emission — verify sign & nitrate handlingShort summary: the comment change is fine and there is no double-negation of ammonia emissions, but I found an inconsistency: field-level balance excludes nitrate while farm-level aggregation includes emission.total (which adds nitrate). That needs fixing.
Used prior guidance: nitrate should be excluded from the balance; removal and volatilization are negative by convention.
Files to address
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/mineral.ts
- Apply the comment tweak (see diff below).
- fdm-calculator/src/balance/nitrogen/index.ts
- Field-level balance uses emission.ammonia.total (line ~175) — OK.
- Farm-level aggregation uses fieldBalance.emission.total when computing totalFarmEmission / avgFarmEmission (lines ~232–234, avg & avgFarmBalance at ~254–257) — this includes nitrate. Change aggregation to use ammonia only (emission.ammonia.total) or otherwise make the balance consistent with the product rule.
- fdm-calculator/src/balance/nitrogen/emission/index.ts
- emission.total is computed as ammonia.total.add(nitrate.total) (line ~40). Keep for reporting if desired, but do not use emission.total in the farm balance if nitrate must be excluded.
Verification notes (from run)
- Search for additional negations returned no matches — no evidence of a double-negation of ammonia values.
- Unit tests assert negative ammonia emission values (e.g. mineral fertilizer tests expect -0.08), confirming emissions are represented negative downstream.
- Nitrate emission implementation currently returns zero, but types and emission.total exist; farm aggregation uses emission.total and would include nitrate if implemented.
Suggested diff to apply (as requested)
- // Calculate for this application the amount of Nitrogen supplied by mineral fertilizer + // Calculate NH3 emission (kg N) for this application ... - .dividedBy(1000) // convert from g N to kg N - .times(-1) // Return negative value + .dividedBy(1000) // convert from g N to kg N + .times(-1) // Emissions are represented as negative values by conventionRecommendation: Apply the comment update, and also fix the farm-level aggregation in calculateNitrogenBalancesFieldToFarm to use ammonia-only emission for the balance (or make the field-level balance include nitrate consistently) to respect the product rule that nitrate should be excluded from the balance.
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/mineral.test.ts (1)
86-90: This test will fail until the helper call in production code is fixed.
calculateNitrogenEmissionViaAmmoniaByMineralFertilizerscurrently passes the application todetermineMineralAmmoniaEmissionFactorinstead of the fertilizer detail. That yields EF≈0 and breaks these assertions.Linking fix: see mineral.ts Lines 53–59 patch to pass
fertilizerDetailand use a nullish check onp_ef_nh3.Also applies to: 99-103
fdm-calculator/src/balance/nitrogen/supply/mineralization.ts (1)
1-1: Remove unused/invalid fdmSchema type import and fix MINIP param typingsfdmSchema isn’t a type namespace. Use SoilAnalysisPicked keys (already imported) for param types and drop the fdmSchema type import.
-import type { fdmSchema } from "@svenvw/fdm-core"-export function calculateNitrogenSupplyBySoilMineralizationUsingMinip( - a_c_of: fdmSchema.soilAnalysisTypeSelect["a_c_of"], - a_cn_fr: fdmSchema.soilAnalysisTypeSelect["a_cn_fr"], - a_density_sa: fdmSchema.soilAnalysisTypeSelect["a_density_sa"], +export function calculateNitrogenSupplyBySoilMineralizationUsingMinip( + a_c_of: SoilAnalysisPicked["a_c_of"], + a_cn_fr: SoilAnalysisPicked["a_cn_fr"], + a_density_sa: SoilAnalysisPicked["a_density_sa"], ): Decimal {Also applies to: 138-141
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/manure.test.ts (1)
109-120: Clarify negative-emission sign in manure test, make toBeCloseTo precision explicit, and fix farm-aggregator & UI to use ammonia-only emissionsVerified findings (brief):
- manure emissions are intentionally negative (manure.ts multiplies application value by -1) — test expectation of negative values is correct.
- Field-level balance uses emission.ammonia.total (correct).
- Farm-level aggregation currently uses fieldBalance.emission.total (ammonia + nitrate) — this incorrectly includes nitrate in the farm balance.
- Field UI displays emission.total (includes nitrate) and passes it to the chart — inconsistent with the balance which uses ammonia-only.
- convertDecimalToNumberRecursive uses Decimal.round() which will drop fractional precision (probably undesirable).
Files/changes to apply (concise diffs and locations):
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/manure.test.ts — update comment to state negative sign and make precision explicit:
- // Expected values: - // app1: 1000 * 0.5 * 0.68 / 1000 = 0.34 kg N (broadcasting on grassland) - // app2: 500 * 0.4 * 0.264 / 1000 = 0.0528 kg N (narrowband on grassland) - // Total: 0.34 + 0.0528 = 0.3928 kg N - - expect(result.total.toNumber()).toBeCloseTo(-0.3928) + // Expected absolute values (emissions are represented as negative flows in the balance): + // app1: 1000 * 0.5 * 0.68 / 1000 = 0.34 kg N (broadcasting on grassland) -> -0.34 kg N + // app2: 500 * 0.4 * 0.264 / 1000 = 0.0528 kg N (narrowband on grassland) -> -0.0528 kg N + // Total: 0.34 + 0.0528 = 0.3928 kg N -> -0.3928 kg N + + expect(result.total.toNumber()).toBeCloseTo(-0.3928, 4) expect(result.applications.length).toBe(2) expect(result.applications[0].id).toBe("app1") - expect(result.applications[0].value.toNumber()).toBeCloseTo(-0.34) + expect(result.applications[0].value.toNumber()).toBeCloseTo(-0.34, 2) expect(result.applications[1].id).toBe("app2") - expect(result.applications[1].value.toNumber()).toBeCloseTo(-0.0528) + expect(result.applications[1].value.toNumber()).toBeCloseTo(-0.0528, 4)
- fdm-calculator/src/balance/nitrogen/index.ts — aggregate only ammonia emissions at farm level:
- totalFarmEmission = totalFarmEmission.add( - fieldBalance.emission.total.times(fieldArea), - ) + totalFarmEmission = totalFarmEmission.add( + fieldBalance.emission.ammonia.total.times(fieldArea), + )
- fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen.$b_id.tsx — show / pass ammonia-only emission for field page:
- {result.emission.total} + {result.emission.ammonia.total} ... - <NitrogenBalanceChart - balance={result.balance} - supply={result.supply.total} - removal={result.removal.total} - emission={result.emission.total} - /> + <NitrogenBalanceChart + balance={result.balance} + supply={result.supply.total} + removal={result.removal.total} + emission={result.emission.ammonia.total} + />
- fdm-app/app/components/blocks/balance/nitrogen-details.tsx — when showing the top-line emission value in the details view, prefer naming/using ammonia explicitly (or show both ammonia and nitrate separately). Example change:
- Emissie (Totaal): {emission.total} kg N / ha + Emissie (NH3, totaal): {emission.ammonia.total} kg N / ha + {/* optionally render nitrate.emission separately if desired */}
- fdm-calculator/src/balance/nitrogen/index.ts — preserve fractional precision when converting Decimal -> number:
- if (data instanceof Decimal) { - return data.round().toNumber() - } + if (data instanceof Decimal) { + return data.toNumber() + }Notes / actions:
- Apply the test diff above to make expectations explicit and deterministic.
- Apply the aggregator change (use emission.ammonia.total) so farm-level balance excludes nitrate as intended.
- Update field UI (cards + chart props) to use ammonia totals so displayed emission and chart magnitude match the balance sign/convention.
- Optionally, in the details view show both ammonia and nitrate (clearly labeled) if you want to keep nitrate visible to users.
- Replace Decimal.round() in conversion with toNumber() (or otherwise choose desired decimal places) to avoid silently dropping fractional kg values in numeric outputs.
🧹 Nitpick comments (20)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/compost.ts (4)
10-23: Document the negative return convention in the JSDoc.Since emissions are now returned as negative values, capture this in the docstring to avoid misuse by downstream consumers.
Apply this doc tweak:
/** * Calculates the ammonia emissions specifically from compost applications. * * This function iterates through fertilizer applications, filters for compost types, * determines the appropriate emission factor, and calculates the ammonia emissions * for each relevant application. It then aggregates these values to provide a total. + * + * Note: By convention, emissions are returned as negative Decimal values to represent + * nitrogen outflows from the system. * * @param cultivations - An array of cultivation records for the field. * @param fertilizerApplications - An array of fertilizer application records. * @param cultivationDetailsMap - A Map where keys are cultivation IDs and values are detailed cultivation information. * @param fertilizerDetailsMap - A Map where keys are fertilizer catalogue IDs and values are detailed fertilizer information. * @returns An object containing the total ammonia emissions from compost and a breakdown by individual application. * @throws Error if a fertilizer application references a non-existent fertilizer detail. */
42-46: Improve error message with catalogue ID context.Include the catalogue ID to ease debugging when details are missing.
- throw new Error( - `Fertilizer application ${application.p_app_id} has no fertilizerDetails`, - ) + throw new Error( + `Fertilizer application ${application.p_app_id} (catalogueId=${application.p_id_catalogue}) has no fertilizerDetails`, + )
78-82: Use a consistent Decimal initialization style.Elsewhere you use new Decimal(0). Prefer consistency and update the reducer seed.
- }, Decimal(0)) + }, new Decimal(0))
83-86: Minor: use object property shorthand.Keeps the return concise.
- return { - total: totalValue, - applications: applications, - } + return { + total: totalValue, + applications, + }fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/other.ts (4)
10-23: Docstring should state the sign convention (negative) and remove manure wording for “other”.Currently the doc says “emissions from 'other'” but doesn’t note the returned sign. Also, it mentions manure in a general description context which is confusing here.
Apply:
- * Calculates the ammonia emissions from "other" fertilizer types. + * Calculates the ammonia emissions from "other" fertilizer types. + * Note: emissions are returned as negative values (loss from the field system), + * so they can be summed directly into the nitrogen balance without additional negation. @@ - * the ammonia emission is currently calculated like manure and other organic fertilizers. + * the ammonia emission is currently calculated using the same emission-factor logic as organic fertilizers. @@ - * @returns An object containing the total ammonia emissions from other fertilizers and a breakdown by individual application. + * @returns An object containing the total ammonia emissions (negative) from other fertilizers and a breakdown by individual application.
42-46: Enrich error message with the referenced catalogue ID.This makes debugging data issues faster.
- throw new Error( - `Fertilizer application ${application.p_app_id} has no fertilizerDetails`, - ) + throw new Error( + `Fertilizer application ${application.p_app_id} (catalogue ${application.p_id_catalogue}) has no fertilizerDetails`, + )
83-86: Consistency: construct Decimal consistently.Elsewhere you use
new Decimal(0). Minor style nit to use it here too.- }, Decimal(0)) + }, new Decimal(0))
36-60: Optional: pre-filter non-“other” applications instead of emitting zero entries.Returning zero entries for non-target types increases noise and work downstream. If the contract does not require “all applications mirrored,” filter first.
This changes the applications array length; proceed only if consumers don’t rely on a 1:1 mapping with inputs.
- const applications = fertilizerApplications.map((application) => { + const applications = fertilizerApplications + .filter((application) => { + const detail = fertilizerDetailsMap.get(application.p_id_catalogue) + return detail + ? (detail.p_type !== "manure" && + detail.p_type !== "mineral" && + detail.p_type !== "compost") + : true // will be caught later by the missing-detail error + }) + .map((application) => { @@ - if ( - fertilizerDetail.p_type === "manure" || - fertilizerDetail.p_type === "mineral" || - fertilizerDetail.p_type === "compost" - ) { - return { - id: application.p_app_id, - value: new Decimal(0), - } - } + // Only "other" fertilizer types reach this pointIf we keep the current contract, consider adding a comment clarifying why non-other apps are included with zero values.
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/other.test.ts (3)
114-151: Fix test description to match negative emission expectation. Add a sign check.The test title says “return 0” but asserts
-0.01. Tighten this to avoid confusion.- it("should return 0 for applications of 'other' type as per current function logic", () => { + it("should calculate negative emissions for applications of type 'other'", () => { @@ - expect(result.total.toNumber()).toBeCloseTo(-0.01) + expect(result.total.toNumber()).toBeCloseTo(-0.01) + expect(result.total.toNumber()).toBeLessThan(0) @@ - expect(result.applications[0].value.toNumber()).toBeCloseTo(-0.01) + expect(result.applications[0].value.toNumber()).toBeCloseTo(-0.01) + expect(result.applications[0].value.toNumber()).toBeLessThan(0)
34-112: Optional: assert that the emission-factor function isn’t called for non-“other” types.This would lock the contract that only “other” apps are processed here, reducing risk of regressions.
If desired, add (after the call):
// after calling the function in this test block: const manureModule = await import("./manure") const efMock = vi.mocked(manureModule.determineManureAmmoniaEmissionFactor) expect(efMock).not.toHaveBeenCalled()Note: You may need to import the module at top-level and use
vi.mocked(...)depending on your Vitest config.
1-175: Optional: add edge-case tests for null/undefined inputs.Strengthen coverage for defaulting behavior.
Proposed additions:
it("returns 0 when p_app_amount is missing", () => { const fertilizerApplications: FieldInput["fertilizerApplications"] = [{ p_app_id: "appX", p_id_catalogue: "otherX", p_app_amount: null as unknown as number, // simulate missing p_app_date: new Date("2023-01-01"), p_app_method: "broadcasting", p_id: "appX", }] const fertilizerDetailsMap = new Map<string, FertilizerDetail>([["otherX", { p_id_catalogue: "otherX", p_type: "other", p_nh4_rt: 0.5, p_n_rt: null, p_no3_rt: null, p_s_rt: null, p_ef_nh3: null, }]]) const res = calculateNitrogenEmissionViaAmmoniaByOtherFertilizers([], fertilizerApplications, new Map(), fertilizerDetailsMap) expect(res.total.toNumber()).toBe(0) }) it("returns 0 when p_nh4_rt is missing", () => { const fertilizerApplications: FieldInput["fertilizerApplications"] = [{ p_app_id: "appY", p_id_catalogue: "otherY", p_app_amount: 1000, p_app_date: new Date("2023-01-01"), p_app_method: "broadcasting", p_id: "appY", }] const fertilizerDetailsMap = new Map<string, FertilizerDetail>([["otherY", { p_id_catalogue: "otherY", p_type: "other", p_nh4_rt: null, p_n_rt: null, p_no3_rt: null, p_s_rt: null, p_ef_nh3: null, }]]) const res = calculateNitrogenEmissionViaAmmoniaByOtherFertilizers([], fertilizerApplications, new Map(), fertilizerDetailsMap) expect(res.total.toNumber()).toBe(0) })I can push these tests in a follow-up commit if you prefer.
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/mineral.ts (2)
75-79: Initialize reducer with a Decimal instance for type correctness and consistency.Elsewhere you consistently use
new Decimal(0). UsingDecimal(0)can trip TS types depending on the d.ts. Align for consistency.- }, Decimal(0)) + }, new Decimal(0))
12-14: Correct typos in JSDoc reference to the helper.Spelling is off (“Emmission”). Align with the actual function name for searchability.
- * determines the appropriate emission factor (either from `p_ef_nh3` or by calling - * `determineMineralAmmoniaEmmissionFactor`), and calculates the ammonia emissions + * determines the appropriate emission factor (either from `p_ef_nh3` or by calling + * `determineMineralAmmoniaEmissionFactor`), and calculates the ammonia emissionsfdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/mineral.test.ts (2)
60-60: Fix typos: “Emmission” → “Emission” in test descriptions.Keeps test names greppable and consistent with function names.
- it("should calculate ammonia emissions for mineral applications without p_ef_nh3 (using determineMineralAmmoniaEmmissionFactor)", () => { + it("should calculate ammonia emissions for mineral applications without p_ef_nh3 (using determineMineralAmmoniaEmissionFactor)", () => {-describe("determineMineralAmmoniaEmmissionFactor", () => { +describe("determineMineralAmmoniaEmissionFactor", () => {Also applies to: 164-165
22-46: Optional: broaden coverage with edge and aggregation cases.
- Add a case where
p_ef_nh3is explicitly 0 to ensure nullish checks don’t misroute to the formula.- Add multiple mixed applications (mineral + non-mineral) and verify total equals sum of per-application values.
- Add a case where
p_app_amountis null/undefined to confirm it’s handled as 0.If you’d like, I can draft these tests with concrete fixtures.
Also applies to: 106-140
fdm-calculator/src/balance/nitrogen/supply/mineralization.ts (5)
12-19: Update JSDoc: function no longer uses MINIP and now takes four parametersThe documentation still references MINIP and doesn’t describe the new parameters (cultivations, cultivationDetails). Please update to avoid confusion.
-/** - * Calculates the amount of nitrogen supplied through soil mineralization using Minip. - * - * This function determines the mineralization based on the soil analyses conducted. - * @param soilAnalysis - Combined soil analysis data for the field. - * @param timeFrame - The timeframe for which to calculate the nitrogen mineralization. - * @returns The NitrogenSupplyMineralization object containing the total amount of Nitrogen mineralized. - */ +/** + * Calculates the amount of nitrogen supplied through soil mineralization using a defaults-based, + * per-year approach. Each year is evaluated for grassland status in a May 15–July 15 window and + * prorated to the provided timeframe. + * + * @param cultivations - Field cultivations with start/end dates and catalogue keys. + * @param soilAnalysis - Combined soil analysis data for the field (incl. b_soiltype_agr). + * @param cultivationDetails - Map keyed by b_lu_catalogue providing crop rotation (e.g., grass). + * @param timeFrame - The timeframe for which to calculate the nitrogen mineralization. + * @returns NitrogenSupplyMineralization with total and per-year values (Decimal). + */
27-27: Typo: rename minerlizationPerYear → mineralizationPerYearImproves readability and avoids propagating a misspelling.
- const minerlizationPerYear = [] + const mineralizationPerYear = [] @@ - minerlizationPerYear.push({ + mineralizationPerYear.push({ year, value: adjustedMineralization, }) @@ - years: minerlizationPerYear, + years: mineralizationPerYear,Also applies to: 82-85, 91-91
61-65: Micro-optimization: compute timeframe bounds once outside the looptimeframeStartTime and timeframeEndTime are loop-invariant. Compute them once for clarity and minor efficiency.
- const timeframeStartTime = timeFrame.start.getTime() - const timeframeEndTime = timeFrame.end.getTime() + // timeframeStartTime and timeframeEndTime are computed once outside the loopAdd the following near line 29 (before the for-loop):
const timeframeStartTime = timeFrame.start.getTime() const timeframeEndTime = timeFrame.end.getTime()
70-72: Prefer date-fns isLeapYear for clarityThe manual leap-year check works, but isLeapYear improves readability and intent.
-import { differenceInCalendarDays } from "date-fns" +import { differenceInCalendarDays, isLeapYear } from "date-fns" @@ - const daysInYear = - new Date(year, 1, 29).getMonth() === 1 ? 366 : 365 + const daysInYear = isLeapYear(new Date(year, 0, 1)) ? 366 : 365
55-60: Behavioral note: defaults cover only “dalgrond” and “veen”; all others yield 0If that’s intentional for the current scaffolding, fine. Otherwise consider adding explicit defaults for other soil types to avoid silent zeros.
I can extend the defaults mapping for the other soil types if you share the spec.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (15)
.changeset/cold-jobs-buy.md(1 hunks).changeset/dry-bugs-care.md(1 hunks).changeset/hip-lizards-hope.md(1 hunks).changeset/loud-ears-push.md(1 hunks)fdm-app/app/components/blocks/balance/nitrogen-chart.tsx(4 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/compost.test.ts(7 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/compost.ts(3 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/manure.test.ts(5 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/manure.ts(3 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/mineral.test.ts(6 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/mineral.ts(3 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/other.test.ts(4 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/other.ts(3 hunks)fdm-calculator/src/balance/nitrogen/supply/mineralization.ts(2 hunks)fdm-calculator/src/balance/nitrogen/types.d.ts(8 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/dry-bugs-care.md
🚧 Files skipped from review as they are similar to previous changes (7)
- .changeset/hip-lizards-hope.md
- .changeset/cold-jobs-buy.md
- .changeset/loud-ears-push.md
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/manure.ts
- fdm-calculator/src/balance/nitrogen/types.d.ts
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/compost.test.ts
- fdm-app/app/components/blocks/balance/nitrogen-chart.tsx
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: SvenVw
PR: SvenVw/fdm#236
File: fdm-calculator/src/balance/nitrogen/index.ts:173-0
Timestamp: 2025-08-14T14:31:55.362Z
Learning: In nitrogen balance calculations for agricultural systems, the balance should only include ammonia emissions (emission.ammonia.total) and should not include nitrate leaching from the emission calculation. The nitrate component (emission.nitrate) should be excluded from the balance formula.
🧬 Code Graph Analysis (4)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/other.test.ts (2)
fdm-calculator/src/balance/nitrogen/types.d.ts (3)
FieldInput(395-415)CultivationDetail(420-429)FertilizerDetail(434-443)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/other.ts (1)
calculateNitrogenEmissionViaAmmoniaByOtherFertilizers(24-91)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/mineral.test.ts (2)
fdm-calculator/src/balance/nitrogen/types.d.ts (2)
FieldInput(395-415)FertilizerDetail(434-443)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/mineral.ts (1)
calculateNitrogenEmissionViaAmmoniaByMineralFertilizers(21-84)
fdm-calculator/src/balance/nitrogen/supply/mineralization.ts (3)
fdm-core/src/db/schema.ts (3)
cultivations(289-301)soilAnalysis(605-648)soilAnalysisTypeSelect(650-650)fdm-calculator/src/balance/nitrogen/types.d.ts (5)
FieldInput(395-415)SoilAnalysisPicked(381-390)CultivationDetail(420-429)NitrogenBalanceInput(448-456)NitrogenSupplyMineralization(99-108)fdm-core/src/index.ts (1)
fdmSchema(17-17)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/manure.test.ts (2)
fdm-calculator/src/balance/nitrogen/types.d.ts (3)
FieldInput(395-415)CultivationDetail(420-429)FertilizerDetail(434-443)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/manure.ts (1)
calculateNitrogenEmissionViaAmmoniaByManure(25-88)
🔇 Additional comments (24)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/compost.ts (5)
7-7: Import path update looks correct.../../../types is the correct relative path from this file to nitrogen/types.ts.
58-63: Function naming suggests manure-only logic; confirm compost coverage.determineManureAmmoniaEmissionFactor sounds manure-specific. Please confirm its logic is valid for compost or consider renaming to a more generic determineOrganicAmmoniaEmissionFactor if it handles compost too.
49-55: Applications array includes non-compost entries with zero values — confirm contract.Returning zeros for non-compost apps can be useful, but confirm this is the desired API contract for all ammonia fertilizer calculators (for consistent downstream handling and charts). If consumers expect only compost entries, consider filtering; otherwise, keep as-is.
24-29: Function rename verified — no remaining references to calculateAmmoniaEmissionsByCompostNo occurrences of the old name were found. The new symbol is present/imported in:
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/compost.ts
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/index.ts
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/compost.test.ts
No further action required.
64-71: Sign convention verified — no double-negation; units correct.All ammonia emission calculators return negative values (they multiply result by -1) and the nitrogen balance uses emission.ammonia.total (so values are not negated again). Unit conversion is consistent: p_app_amount is kg/ha and p_nh4_rt is g N/kg, dividing by 1000 converts g→kg.
Files verified:
- fdm-calculator/src/balance/nitrogen/emission/ammonia/residues.ts — .times(-1) (≈line 118)
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/mineral.ts — .times(-1) (≈line 67)
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/manure.ts — .times(-1) (≈line 71)
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/other.ts — .times(-1) (≈line 74)
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/compost.ts — .times(-1) (≈line 70)
- fdm-calculator/src/balance/nitrogen/index.ts — balance: supply.total.add(removal.total).add(emission.ammonia.total) (≈line 175)
- fdm-calculator/src/balance/nitrogen/emission/index.ts — emission.total = ammonia.total.add(nitrate.total) (nitrate included in emission.total but NOT used in the balance)
- fdm-calculator/src/balance/nitrogen/types.d.ts — p_nh4_rt documented as "g N / kg fertilizer"
- fdm-core/src/fertilizer.d.ts & docs — p_app_amount is kg / ha
Used project conventions/learnings: removal and volatilization are negative by design; nitrate is excluded from the balance.
Conclusion: No change required. (Optional: centralize the negative sign at a single orchestrator if you prefer strictly positive per-calculator outputs for maintainability.)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/other.ts (4)
7-7: Types import path change looks correct.Importing
NitrogenEmissionAmmoniaFertilizerset al. from../../../typesaligns with the refactor.
62-66: Confirm using manure emission factor for “other” fertilizer types is domain-intended.Function naming suggests manure context; applying it to “other” is fine if intended, but please confirm this rule.
Do you want me to audit all ammonia calculators to confirm a consistent factor selection rule across fertilizer types and open a follow-up issue if we need a separate factor for “other”?
24-29: API rename: all call sites updated — no stale usages found.Searched for the old symbol
calculateAmmoniaEmissionsByOtherFertilizers(no matches). The new namecalculateNitrogenEmissionViaAmmoniaByOtherFertilizersis referenced in:
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/index.ts
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/other.ts
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/other.test.ts
1-91: Resolved — nitrogen balance excludes nitrate; ammonia-only confirmedChecked per prior learning (exclude nitrate). Search found no occurrences of
emission.nitratein fdm-calculator/src/balance/nitrogen and the balance aggregates ammonia only.
- fdm-calculator/src/balance/nitrogen/index.ts:175 — balance: supply.total.add(removal.total).add(emission.ammonia.total)
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/other.ts — calculates ammonia emissions (no nitrate included)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/other.test.ts (1)
7-9: Imports and API rename reflected correctly in tests.
- Types import path updated to
../../../types.- Test targets
calculateNitrogenEmissionViaAmmoniaByOtherFertilizers.fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/mineral.ts (2)
6-6: Import path change looks correct.Types appear to have moved; updated path is consistent with the rest of the PR.
21-24: Public API rename is clear and consistent with emission naming.Signature and return type are unchanged; downstream callers/tests updated accordingly.
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/mineral.test.ts (3)
54-58: LGTM: expectations reflect the negative-emission convention.As long as upstream aggregation doesn’t negate again, asserting negative values here is correct and consistent.
8-17: API rename and usages look consistent across tests.Imports, describe blocks, and invocations match the new public API.
Also applies to: 48-52, 131-135, 156-160
136-140: Minor: assertion style is consistent, but consider asserting Decimal equality via toNumber() with precision.You’re already using
toBeCloseTowhere needed; for zeros,toBe(0)is fine. No action required.fdm-calculator/src/balance/nitrogen/supply/mineralization.ts (2)
36-54: Grassland detection: confirm croprotation value "grass" matches catalogue dataThe logic hinges on b_lu_croprotation === "grass". If the catalogue uses a different canonical value (e.g., "grassland"), this will silently return false. Please confirm the exact string domain for that field.
69-79: Confirm intended [start, end) semantics in day prorationdifferenceInCalendarDays(end, start) effectively treats the interval as [start, end). If timeFrame.end should be inclusive, you’ll undercount by one day. Please verify the contract.
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers/manure.test.ts (7)
7-7: Types import path adjustment looks correct.The relative jump to "../../../types" matches the file location and aligns with the public types under nitrogen.
9-11: Public API rename reflected in imports — OK.Importing calculateNitrogenEmissionViaAmmoniaByManure and determineManureAmmoniaEmissionFactor from "./manure" matches the implementation rename.
13-13: Describe title updated to the new API — OK.Name change is consistently reflected in the test suite title.
20-25: Updated call site to new API — OK.Invocation and parameter order remain unchanged; behavior preserved.
102-107: Updated call site in second test — OK.Consistent with the rename; parameters preserved.
151-156: Updated call site in non-manure scenario — OK.Rename applied; assertions still target zeros for non-manure types.
181-186: Updated call site in error-path scenario — OK.Error message assertion remains accurate and specific.
1 participant
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores