Conversation
…in all cases will return a value
…estable` and `b_lu_n_residue` with median value for `b_lu_croprotation` instead of 0
|
|
Caution Review failedThe pull request is closed. WalkthroughThis PR revises ammonia emission factor determination for fertilizers by introducing rotation-based land-type detection and a centralized method × land-type dispatch, updates tests accordingly (including new scenarios and stricter error handling), extends performance test inputs with three fields, adjusts .changeset config, and bumps package versions with changelog updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant F as determineManureAmmoniaEmissionFactor
participant D as cultivationDetails (b_lu_croprotation)
C->>F: fertilizerApplication, activeCultivations, cultivationDetails
F->>D: Inspect active cultivations' rotation values
Note right of F: Classify land type via sets:<br/>- Grassland: grass, clover<br/>- Cropland: potato, rapeseed, starch, maize, cereal, sugarbeet, catchcrop, alfalfa<br/>- Else: bare soil
F->>F: Switch on application method
alt Supported method
F->>F: Nested switch on land type
alt Supported land type
F-->>C: Return Decimal EF
else Unsupported land type
F-->>C: Throw Error("Unsupported method for land type")
end
else Unknown method
F-->>C: Throw Error("Unknown application method")
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #288 +/- ##
==========================================
- Coverage 93.26% 93.07% -0.20%
==========================================
Files 81 81
Lines 13014 13118 +104
Branches 1306 1326 +20
==========================================
+ Hits 12137 12209 +72
- Misses 875 907 +32
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:
|
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.test.ts (1)
20-28: Fix invalid mock data for cropland cultivation.Line 22 uses
b_lu_croprotation: "crop", which is not a valid value according to the schema. Per the PR objectives, valid cropland rotation values are: "potato", "rapeseed", "starch", "maize", "cereal", "sugarbeet", "catchcrop", "alfalfa". This test data should use one of these valid enum values to properly test the corrected land-type detection logic.Apply this diff to use a valid cropland rotation value:
mockCultivationDetailsMap.set("cropland-cultivation", { b_lu_catalogue: "cropland-cultivation", - b_lu_croprotation: "crop", + b_lu_croprotation: "maize", b_lu_yield: 0, b_lu_hi: 0, b_lu_n_harvestable: 0, b_lu_n_residue: 0, b_n_fixation: 0, })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.changeset/dark-feet-join.md(1 hunks).changeset/stale-knives-run.md(1 hunks).changeset/sweet-ways-itch.md(1 hunks).changeset/wicked-bikes-march.md(1 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.test.ts(10 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.ts(1 hunks)fdm-calculator/src/balance/nitrogen/performance.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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:
.changeset/sweet-ways-itch.md
🧬 Code graph analysis (1)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.test.ts (1)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.ts (1)
calculateNitrogenEmissionViaAmmoniaByFertilizers(21-135)
🔇 Additional comments (7)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.test.ts (5)
689-698: Improved error handling for unsupported application methods.The tests now correctly expect thrown errors for unsupported application methods instead of console warnings. This is a significant improvement in error handling that makes invalid configurations fail fast rather than silently using default values.
Also applies to: 713-722, 737-746
673-674: Confirm updated emission factor for bare soil incorporation.
Both the implementation and tests now use 0.46 (previously 0.22) for bare-soil incorporation—please verify this aligns with your source data or domain requirements.
233-237: ```bash
#!/bin/bashLocate fertilizers.ts
file=$(fd -e ts 'fertilizers.ts' | head -n1)
echo "Located file: $file"Verify function implementation
rg -nP 'determineManureAmmoniaEmissionFactor' -C2 "$file"
Check emission factor mappings for bare soil and slotted coulter
rg -nC3 'bare soil' "$file"
rg -nC3 'slotted coulter' "$file"--- `409-410`: **Emission factors for narrowband and slotted coulter on grassland match the implementation.** Verified that in `fertilizers.ts` both “narrowband” (lines 359–364) and “slotted coulter” (lines 232–237) return `0.17` for grassland. --- `519-520`: **Emission factor mappings are correct as implemented.** The cases for “slotted coulter” and “shallow injection” return 0.24 for both cropland and bare soil, matching the updated tests. </blockquote></details> <details> <summary>.changeset/wicked-bikes-march.md (1)</summary><blockquote> `1-5`: **LGTM - Changeset accurately describes the improvement.** The changeset correctly identifies this as a patch-level improvement where `determineManureAmmoniaEmissionFactor` now guarantees a return value through structured switch-case logic with explicit error handling for unsupported combinations. </blockquote></details> <details> <summary>fdm-calculator/src/balance/nitrogen/performance.test.ts (1)</summary><blockquote> `134-136`: **LGTM - Added required fields for updated emission calculations.** The addition of `p_app_method`, `p_name_nl`, and `p_id` fields is necessary to support the refactored emission factor calculation logic. The mock values are appropriate for performance testing. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.test.ts(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.test.ts (2)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.ts (1)
calculateNitrogenEmissionViaAmmoniaByFertilizers(21-135)fdm-calculator/src/balance/nitrogen/types.d.ts (1)
FieldInput(395-415)
SvenVw
removed
the
branch:development
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/hungry-rice-taste.md(1 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.test.ts(18 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.test.ts (2)
fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.ts (1)
calculateNitrogenEmissionViaAmmoniaByFertilizers(21-135)fdm-calculator/src/balance/nitrogen/types.d.ts (1)
FieldInput(395-415)
coderabbitai
Bot
added
the
branch:main
2 participants
Summary by CodeRabbit
Closes #286