Implement nitrate leaching calculations for nitrogen balance#330
Conversation
🦋 Changeset detectedLatest commit: 648f143 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 |
|
Caution Review failedThe pull request is closed. WalkthroughRestructures nitrogen emission from a scalar to an object Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI (routes / chart / details)
participant Calc as fdm-calculator
participant Data as Field/Farm Balance
rect rgb(245,250,245)
Note over UI,Calc: Request/response for nitrogen balance with split emissions
UI->>Calc: calculateNitrogenBalance(inputs)
Calc->>Calc: compute ammonia emission (existing logic)
Calc->>Calc: balance := supply.total + removal.total + emission.ammonia.total
Calc->>Calc: determineNitrateLeachingFactor(landType, soilType, gwl)
alt balance > 0
Calc->>Calc: nitrate = balance × factor
else balance ≤ 0
Calc->>Calc: nitrate = 0
end
Calc->>Data: return per-field & farm emission { total, ammonia, nitrate }
Data->>UI: render Ammoniakemissie and Nitraatemissie (charts/cards)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## development #330 +/- ##
===============================================
+ Coverage 87.34% 87.59% +0.25%
===============================================
Files 79 79
Lines 3872 3951 +79
Branches 1099 1141 +42
===============================================
+ Hits 3382 3461 +79
Misses 490 490
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:
|
…inction between ammonia and nitrate
… separately in the chart and cards
coderabbitai
Bot
changed the title
coderabbitai
Bot
added
branch:development
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
fdm-calculator/src/balance/nitrogen/emission/index.ts (1)
9-9: Remove unused import from emission/index.ts.The import
calculateNitrogenEmissionViaNitrateat line 9 is unused in this file. While the function is called elsewhere in the codebase, it's imported directly from./emission/nitrateinnitrogen/index.ts(line 23), not through the emission module. Remove the unused import.fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen.$b_id.tsx (1)
397-402: Verifyemissionprop type compatibility withNitrogenBalanceChart.The chart component now expects
emissionas an object with{ total, ammonia, nitrate }. Here you're passingresult.emission.totalwhich is a number, but the chart expects the full emission object.Based on the nitrogen-chart.tsx changes, the component expects the full emission object. Apply this fix:
<NitrogenBalanceChart balance={result.balance} supply={result.supply.total} removal={result.removal.total} - emission={result.emission.total} + emission={result.emission} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.changeset/funny-loops-own.md(1 hunks).changeset/giant-ghosts-bet.md(1 hunks).changeset/spotty-clubs-cheer.md(1 hunks).changeset/sunny-breads-prove.md(1 hunks)fdm-app/app/components/blocks/balance/nitrogen-chart.tsx(3 hunks)fdm-app/app/components/blocks/balance/nitrogen-details.tsx(2 hunks)fdm-app/app/components/blocks/fertilizer-applications/metrics.tsx(3 hunks)fdm-app/app/components/blocks/sidebar/apps.tsx(3 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen.$b_id.tsx(4 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx(4 hunks)fdm-calculator/src/balance/nitrogen/emission/index.ts(2 hunks)fdm-calculator/src/balance/nitrogen/emission/nitrate/index.test.ts(1 hunks)fdm-calculator/src/balance/nitrogen/emission/nitrate/index.ts(1 hunks)fdm-calculator/src/balance/nitrogen/index.ts(7 hunks)fdm-calculator/src/balance/nitrogen/types.d.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 236
File: fdm-calculator/src/balance/nitrogen/index.ts:173-0
Timestamp: 2025-08-14T14:31:55.384Z
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.
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 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.
📚 Learning: 2025-08-13T10:33:05.313Z
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 0
File: :0-0
Timestamp: 2025-08-13T10:33:05.313Z
Learning: In the fdm project, fdm-calculator integration for new features like b_lu_variety is handled in separate updates from the core data model changes. When fdm-core functions are updated to support new fields, fdm-calculator can consume these enhanced APIs without requiring changes in the same PR that introduces the core functionality.
Applied to files:
.changeset/spotty-clubs-cheer.md.changeset/funny-loops-own.md
📚 Learning: 2025-08-14T14:31:55.384Z
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 236
File: fdm-calculator/src/balance/nitrogen/index.ts:173-0
Timestamp: 2025-08-14T14:31:55.384Z
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.
Applied to files:
.changeset/spotty-clubs-cheer.mdfdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx.changeset/funny-loops-own.md.changeset/sunny-breads-prove.mdfdm-app/app/components/blocks/balance/nitrogen-chart.tsxfdm-app/app/components/blocks/fertilizer-applications/metrics.tsxfdm-calculator/src/balance/nitrogen/types.d.tsfdm-calculator/src/balance/nitrogen/emission/index.ts.changeset/giant-ghosts-bet.mdfdm-app/app/components/blocks/balance/nitrogen-details.tsxfdm-calculator/src/balance/nitrogen/emission/nitrate/index.tsfdm-calculator/src/balance/nitrogen/index.tsfdm-calculator/src/balance/nitrogen/emission/nitrate/index.test.ts
📚 Learning: 2025-02-14T09:56:37.606Z
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 75
File: fdm-app/app/routes/farm.$b_id_farm.field.$b_id.fertilizer.tsx:68-71
Timestamp: 2025-02-14T09:56:37.606Z
Learning: The `calculateDose` function in `svenvw/fdm-calculator` is a synchronous function that includes built-in validation for negative application amounts and nutrient rates.
Applied to files:
.changeset/spotty-clubs-cheer.md.changeset/funny-loops-own.md
📚 Learning: 2025-05-26T10:32:00.674Z
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 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/spotty-clubs-cheer.md.changeset/funny-loops-own.mdfdm-app/app/components/blocks/fertilizer-applications/metrics.tsxfdm-calculator/src/balance/nitrogen/types.d.tsfdm-calculator/src/balance/nitrogen/emission/index.ts.changeset/giant-ghosts-bet.mdfdm-calculator/src/balance/nitrogen/index.ts
📚 Learning: 2025-05-26T10:32:15.538Z
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 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/spotty-clubs-cheer.md.changeset/funny-loops-own.mdfdm-app/app/components/blocks/fertilizer-applications/metrics.tsxfdm-calculator/src/balance/nitrogen/types.d.tsfdm-calculator/src/balance/nitrogen/emission/index.tsfdm-calculator/src/balance/nitrogen/index.ts
📚 Learning: 2025-07-21T12:06:07.070Z
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 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:
.changeset/spotty-clubs-cheer.md
📚 Learning: 2025-11-21T10:02:25.556Z
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 343
File: fdm-calculator/src/balance/organic-matter/types.d.ts:12-132
Timestamp: 2025-11-21T10:02:25.556Z
Learning: In the organic matter balance calculation system (fdm-calculator), degradation values are negative by definition. This means the balance calculation using supply.total.plus(degradation.total) is correct, as it effectively computes supply - |degradation|. This follows the same pattern as the nitrogen balance system.
Applied to files:
.changeset/spotty-clubs-cheer.md.changeset/funny-loops-own.md
📚 Learning: 2025-03-04T10:56:35.540Z
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 88
File: fdm-calculator/src/doses/calculate-dose.ts:18-18
Timestamp: 2025-03-04T10:56:35.540Z
Learning: In the FDM calculator, fertilizer nutrient rates (p_n_rt, p_p_rt, p_k_rt) are measured in g/kg, and are converted to fractions by dividing by 10 during dose calculations.
Applied to files:
.changeset/spotty-clubs-cheer.md
📚 Learning: 2025-09-23T12:27:07.391Z
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 274
File: fdm-app/app/routes/farm.$b_id_farm._index.tsx:151-204
Timestamp: 2025-09-23T12:27:07.391Z
Learning: In the FDM application, field overview functionality is implemented as a dedicated page accessible via `farm/{farmId}/{calendar}/field` rather than as a direct listing on the dashboard. The dashboard includes a "Perceelsoverzicht" quick action card that provides navigation to this comprehensive field management interface.
Applied to files:
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen.$b_id.tsx.changeset/giant-ghosts-bet.md
📚 Learning: 2025-01-09T16:03:37.764Z
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 42
File: fdm-app/app/routes/farm/_b_id_farm/layout.tsx:46-95
Timestamp: 2025-01-09T16:03:37.764Z
Learning: A shared layout component `FarmLayoutBase` has been created in `components/custom/farm-layout-base.tsx` to maintain consistency across farm-related pages. The component handles farm selection dropdown, breadcrumb navigation, and provides a common layout structure.
Applied to files:
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen.$b_id.tsx
📚 Learning: 2025-09-23T12:29:34.184Z
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 274
File: fdm-app/app/routes/farm.$b_id_farm._index.tsx:160-163
Timestamp: 2025-09-23T12:29:34.184Z
Learning: In the FDM application, the fertilizer application route intentionally uses `${calendar}/field/fertilizer` instead of the originally planned `/farm/{farmId}/add/fertilizer` structure. This design decision prioritizes starting from the field list view to provide better field selection workflow before applying fertilizer, rather than direct dashboard-to-action navigation.
Applied to files:
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen.$b_id.tsx
📚 Learning: 2025-01-09T16:03:37.764Z
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 42
File: fdm-app/app/routes/farm/_b_id_farm/layout.tsx:46-95
Timestamp: 2025-01-09T16:03:37.764Z
Learning: The farm layout system has been reorganized into separate components (`FarmHeader`, `ContentLayout`, `PaginationLayout`) to support different navigation patterns (sidebar, pagination) while maintaining consistent styling. Each layout component is designed to be used independently or combined as needed.
Applied to files:
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx
📚 Learning: 2025-08-11T11:55:26.053Z
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 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-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsxfdm-app/app/components/blocks/fertilizer-applications/metrics.tsxfdm-calculator/src/balance/nitrogen/emission/index.tsfdm-calculator/src/balance/nitrogen/emission/nitrate/index.ts
📚 Learning: 2025-11-24T10:43:09.278Z
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 343
File: fdm-app/app/components/blocks/balance/organic-matter-chart.tsx:19-25
Timestamp: 2025-11-24T10:43:09.278Z
Learning: In the organic matter balance chart (fdm-app), degradation values are multiplied by -1 before plotting to show supply and degradation bars side-by-side with positive magnitudes, enabling direct visual comparison. This is a deliberate design choice for the organic matter chart visualization, different from the nitrogen balance chart which uses separate stacks.
Applied to files:
.changeset/sunny-breads-prove.md
📚 Learning: 2025-05-27T19:56:48.556Z
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 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:
.changeset/sunny-breads-prove.mdfdm-app/app/components/blocks/balance/nitrogen-chart.tsxfdm-app/app/components/blocks/fertilizer-applications/metrics.tsxfdm-calculator/src/balance/nitrogen/types.d.tsfdm-calculator/src/balance/nitrogen/index.ts
📚 Learning: 2024-11-27T12:15:36.425Z
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 9
File: fdm-data/src/cultivations/index.test.ts:57-59
Timestamp: 2024-11-27T12:15:36.425Z
Learning: In `fdm-data/src/cultivations/index.test.ts`, the `fdm` object created by `drizzle` does not have an `.end()` method. Cleanup code should not attempt to call `fdm.end();`.
Applied to files:
fdm-calculator/src/balance/nitrogen/emission/nitrate/index.test.ts
🧬 Code graph analysis (4)
fdm-app/app/components/blocks/balance/nitrogen-details.tsx (1)
fdm-calculator/src/balance/nitrogen/types.d.ts (1)
NitrogenEmissionNumeric(577-581)
fdm-calculator/src/balance/nitrogen/emission/nitrate/index.ts (1)
fdm-calculator/src/balance/nitrogen/types.d.ts (4)
FieldInput(417-437)SoilAnalysisPicked(403-412)CultivationDetail(442-451)NitrogenEmissionNitrate(296-301)
fdm-calculator/src/balance/nitrogen/index.ts (1)
fdm-calculator/src/balance/nitrogen/emission/nitrate/index.ts (1)
calculateNitrogenEmissionViaNitrate(32-123)
fdm-calculator/src/balance/nitrogen/emission/nitrate/index.test.ts (1)
fdm-calculator/src/balance/nitrogen/emission/nitrate/index.ts (2)
calculateNitrogenEmissionViaNitrate(32-123)determineNitrateLeachingFactor(143-204)
🪛 GitHub Actions: Tests
fdm-calculator/src/balance/nitrogen/emission/nitrate/index.test.ts
[error] 63-63: AssertionError: expected '-16' to be '16'. total should be 16 but got -16.
[error] 94-94: AssertionError: expected '-33' to be '33'. total should be 33 but got -33.
[error] 126-126: AssertionError: expected '-44' to be '44'. total should be 44 but got -44.
[error] 195-195: AssertionError: expected '-16' to be '16'. total should be 16 but got -16.
🪛 GitHub Check: calculator (24)
fdm-calculator/src/balance/nitrogen/emission/nitrate/index.test.ts
[failure] 195-195: src/balance/nitrogen/emission/nitrate/index.test.ts > calculateNitrogenEmissionViaNitrate > should prioritize grassland over cropland
AssertionError: expected '-16' to be '16' // Object.is equality
Expected: "16"
Received: "-16"
❯ src/balance/nitrogen/emission/nitrate/index.test.ts:195:41
[failure] 126-126: src/balance/nitrogen/emission/nitrate/index.test.ts > calculateNitrogenEmissionViaNitrate > should return zero emission for bare soil
AssertionError: expected '-44' to be '44' // Object.is equality
Expected: "44"
Received: "-44"
❯ src/balance/nitrogen/emission/nitrate/index.test.ts:126:41
[failure] 94-94: src/balance/nitrogen/emission/nitrate/index.test.ts > calculateNitrogenEmissionViaNitrate > should calculate nitrate emission for cropland on clay soil
AssertionError: expected '-33' to be '33' // Object.is equality
Expected: "33"
Received: "-33"
❯ src/balance/nitrogen/emission/nitrate/index.test.ts:94:41
[failure] 63-63: src/balance/nitrogen/emission/nitrate/index.test.ts > calculateNitrogenEmissionViaNitrate > should calculate nitrate emission for grassland on sandy soil
AssertionError: expected '-16' to be '16' // Object.is equality
Expected: "16"
Received: "-16"
❯ src/balance/nitrogen/emission/nitrate/index.test.ts:63:41
🔇 Additional comments (26)
.changeset/funny-loops-own.md (1)
1-5: LGTM!The changeset correctly documents the minor version bump for
@svenvw/fdm-calculatorwith an accurate description of the emission structure change..changeset/giant-ghosts-bet.md (1)
1-5: LGTM!The changeset correctly documents the minor version bump for
@svenvw/fdm-appwith an accurate description of the UI enhancement to display nitrate emissions..changeset/sunny-breads-prove.md (1)
1-5: LGTM!The changeset correctly documents the minor version bump for
@svenvw/fdm-appwith an accurate description of the visualization improvements for separate ammonia and nitrate emission displays..changeset/spotty-clubs-cheer.md (1)
1-5: LGTM!The changeset correctly documents the minor version bump for
@svenvw/fdm-calculatorwith an accurate description of the nitrate emission calculation feature.fdm-app/app/components/blocks/sidebar/apps.tsx (2)
116-116: Good conditional logic improvement.Linking
defaultOpento the existence ofnitrogenBalanceLinkensures the collapsible only opens when the balance section is accessible, improving the user experience.
156-156: Cleaner conditional rendering.Returning
nullinstead of rendering empty components when links are absent is more idiomatic and improves performance by avoiding unnecessary DOM nodes.Also applies to: 170-170
fdm-app/app/components/blocks/fertilizer-applications/metrics.tsx (2)
400-400: Correct labeling with proper chemical notation.The label and tooltip now correctly specify ammonia emissions with proper subscript notation (NH₃), improving clarity and scientific accuracy.
Also applies to: 410-410
419-420: Correct data path for restructured emission object.Accessing
emission.ammonia.totalaligns with the new emission structure where ammonia and nitrate are separated. This is consistent with the calculator's updated API.fdm-calculator/src/balance/nitrogen/emission/index.ts (2)
38-38: Correct: emission total excludes nitrate.Setting
emission.totaltoammonia.totalaligns with the learning that the nitrogen balance should only include ammonia emissions, not nitrate leaching. This is the correct implementation.Based on learnings, the balance formula should exclude nitrate.
40-42: Appropriate placeholder for nitrate calculation.Initializing nitrate as a placeholder with
Decimal(0)is appropriate since, as the comment indicates, nitrate emission depends on the nitrogen surplus and will be calculated later in the balance flow.fdm-app/app/components/blocks/balance/nitrogen-details.tsx (2)
404-417: Appropriate nitrate emission rendering.The
renderNitrateEmissionsfunction follows the established pattern and correctly displays the nitrate total. The emptyAccordionContentis appropriate since nitrate is calculated as a single factor-based value without detailed breakdowns.
587-589: Correct integration of nitrate emission display.Adding a separate
Accordionfor nitrate emissions alongside ammonia maintains visual separation and consistency with the updated emission structure.fdm-calculator/src/balance/nitrogen/types.d.ts (2)
380-384: Type structure correctly models the new emission breakdown.The change from a single
Decimalto an object withtotal,ammonia, andnitratefields aligns well with the PR objective to add nitrate leaching calculation. This structure also supports the learning that the balance should only include ammonia emissions (viaemission.ammonia) while keeping nitrate separate.
608-612: Numeric type mirrors the Decimal type correctly.The
NitrogenBalanceNumericemission type correctly mirrors theNitrogenBalancetype structure. Minor note: there's a trailing comma on line 610 afterammoniathat isn't present on line 611 afternitrate- this is inconsistent but not a blocking issue.fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen.$b_id.tsx (1)
351-382: Emission display correctly split into ammonia and nitrate cards.The UI now properly displays ammonia and nitrate emissions separately with appropriate labels ("Ammoniakemissie" and "Nitraatemissie") and icons. This aligns with the learning that ammonia and nitrate should be treated distinctly in the nitrogen balance system.
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx (1)
287-318: Correctly implements separate ammonia and nitrate emission display for farm-level view.The emission cards properly access
emission.ammoniaandemission.nitratefrom theNitrogenBalanceNumerictype where these are direct number values. The chart correctly receives the full emission object.fdm-app/app/components/blocks/balance/nitrogen-chart.tsx (2)
21-35: Chart correctly updated to display separate ammonia and nitrate emission bars.The emission prop type change and chart data transformation correctly support the new emission structure. The
Math.abs()ensures emissions display as positive values in the chart regardless of their sign in the data.Minor note: The undefined checks on lines 32-34 (
emission.ammonia === undefined) are defensive but technically unnecessary since the type signature requires these fields. Consider simplifying if you prefer cleaner code, though keeping them is harmless.
81-104: Stacking configuration correctly groups emission bars with removal.Both
emissionAmmoniaandemissionNitratebars usestackId="b", which groups them with removal. This is correct per the established pattern that supply (stackId="a") should be visually separate from removal and emissions (stackId="b"). Based on learnings, supply represents nitrogen inputs while removal and emission represent different types of nitrogen outputs.fdm-calculator/src/balance/nitrogen/emission/nitrate/index.test.ts (1)
199-310:determineNitrateLeachingFactortests look comprehensive.The tests cover:
- All soil types (peat, clay, loess, sandy)
- All GWL classes for sandy soil
- Both grassland and cropland land types
- Error cases for unknown soil type, GWL class, and land type
These tests are well-structured and provide good coverage of the leaching factor logic.
fdm-calculator/src/balance/nitrogen/index.ts (4)
23-23: LGTM!The import of
calculateNitrogenEmissionViaNitrateis correctly added to support the new nitrate emission calculation.
235-248: Balance calculation correctly excludes nitrate emissions.The implementation correctly calculates the balance using only ammonia emissions before computing nitrate leaching, which aligns with the nitrogen balance methodology. Based on learnings, "the balance should only include ammonia emissions (emission.ammonia.total)."
Minor: Line 240 has a typo – "Emssion" should be "Emission".
356-361: Average calculation logic is correct.The division by
totalFarmAreato compute per-hectare averages is appropriate. Once the accumulator bug above is fixed, these values will be accurate.
376-380: LGTM!The emission structure correctly exposes
total,ammonia, andnitratecomponents, aligning with the updated type definitions.fdm-calculator/src/balance/nitrogen/emission/nitrate/index.ts (3)
32-99: Land type determination logic looks correct.The function correctly:
- Prioritizes grassland over cropland when both are present
- Excludes specific bare soil crop codes from being classified as grassland/cropland
- Falls back to "bare soil" when no qualifying cultivations exist
One observation: when
hasGrasslandis true,landTypeis set to "grassland" regardless of whetherhasCroplandis also true. This prioritization seems intentional for mixed-use fields.
143-204: Well-structured leaching factor determination with appropriate error handling.The function correctly:
- Groups soil types for easier maintenance
- Handles GWL-dependent factors for sandy soils
- Throws descriptive errors for unknown soil types and GWL classes
Note: The current implementation treats "bare soil" the same as "cropland" (higher leaching factors). This appears intentional as a conservative assumption.
Please verify that using cropland factors for "bare soil" is the intended behavior, or if bare soil should have its own leaching factors defined.
111-122: LGTM!The emission calculation correctly:
- Only applies when there's a nitrogen surplus (balance > 0)
- Returns a negative value (multiplied by -1), consistent with the convention that emissions/losses are negative
Based on learnings, this aligns with the pattern where "removal and volatilization are calculated as negative values by definition since they represent nitrogen losses."
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fdm-docs/docs/insights/balance/01-nitrogen-balance.md (1)
280-298: Clarify farm balance formula to align with established nitrogen balance principles.The documentation contains an inconsistency between field and farm level balance formulas. Line 282 correctly defines field balance as
N Supply - N Removal - Ammonia Emission, excluding nitrate leaching. However, line 285 defines farm balance asN Supply - N Removal - Total N Emission, which would include both ammonia and nitrate.This contradicts:
- The overall balance formula (line 15):
N Supply - N Removal - N-NH3 Emission(ammonia only)- The field balance definition (line 282): ammonia only
- Established principles from PR 236: the balance should only include ammonia emissions and exclude nitrate leaching
The farm balance should remain consistent with the field balance calculation and use only ammonia emissions, not total emissions. Revise line 285:
- 2. These total farm-level amounts (in kg N) are then divided by the total farm area (ha) to provide an average farm-level balance (`N Supply - N Removal - Total N Emission`) in kg N / ha. + 2. These total farm-level amounts (in kg N) are then divided by the total farm area (ha) to provide an average farm-level balance (`N Supply - N Removal - Ammonia Emission`) in kg N / ha.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/full-lamps-stand.md(1 hunks)fdm-docs/docs/insights/balance/01-nitrogen-balance.md(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 236
File: fdm-calculator/src/balance/nitrogen/index.ts:173-0
Timestamp: 2025-08-14T14:31:55.384Z
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.
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 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.
📚 Learning: 2025-08-14T14:31:55.384Z
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 236
File: fdm-calculator/src/balance/nitrogen/index.ts:173-0
Timestamp: 2025-08-14T14:31:55.384Z
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.
Applied to files:
.changeset/full-lamps-stand.mdfdm-docs/docs/insights/balance/01-nitrogen-balance.md
📚 Learning: 2025-03-04T10:56:35.540Z
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 88
File: fdm-calculator/src/doses/calculate-dose.ts:18-18
Timestamp: 2025-03-04T10:56:35.540Z
Learning: In the FDM calculator, fertilizer nutrient rates (p_n_rt, p_p_rt, p_k_rt) are measured in g/kg, and are converted to fractions by dividing by 10 during dose calculations.
Applied to files:
.changeset/full-lamps-stand.mdfdm-docs/docs/insights/balance/01-nitrogen-balance.md
📚 Learning: 2025-02-14T09:56:37.606Z
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 75
File: fdm-app/app/routes/farm.$b_id_farm.field.$b_id.fertilizer.tsx:68-71
Timestamp: 2025-02-14T09:56:37.606Z
Learning: The `calculateDose` function in `svenvw/fdm-calculator` is a synchronous function that includes built-in validation for negative application amounts and nutrient rates.
Applied to files:
.changeset/full-lamps-stand.md
📚 Learning: 2025-05-26T10:32:15.538Z
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 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:
fdm-docs/docs/insights/balance/01-nitrogen-balance.md
📚 Learning: 2025-05-26T10:32:00.674Z
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 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:
fdm-docs/docs/insights/balance/01-nitrogen-balance.md
📚 Learning: 2025-07-21T12:06:07.070Z
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 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-docs/docs/insights/balance/01-nitrogen-balance.md
📚 Learning: 2025-11-21T10:02:25.556Z
Learnt from: SvenVw
Repo: SvenVw/fdm PR: 343
File: fdm-calculator/src/balance/organic-matter/types.d.ts:12-132
Timestamp: 2025-11-21T10:02:25.556Z
Learning: In the organic matter balance calculation system (fdm-calculator), degradation values are negative by definition. This means the balance calculation using supply.total.plus(degradation.total) is correct, as it effectively computes supply - |degradation|. This follows the same pattern as the nitrogen balance system.
Applied to files:
fdm-docs/docs/insights/balance/01-nitrogen-balance.md
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Sven Verweij <37927107+SvenVw@users.noreply.github.com>
gerardhros
left a comment
gerardhros
left a comment
There was a problem hiding this comment.
LGTM. it might be that the leaching fractions will change since RIVM is updating these numbers. i'll ask for the final documentation.
2 participants
Summary by CodeRabbit
New Features
UI
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.
Closes #239