Skip to content

Invert crop-residue nitrogen removal logic#548

Merged
SvenVw merged 4 commits into
developmentfrom
fix-crop-residue
Apr 2, 2026
Merged

Invert crop-residue nitrogen removal logic#548
SvenVw merged 4 commits into
developmentfrom
fix-crop-residue

Conversation

@SvenVw
Copy link
Copy Markdown
Collaborator

@SvenVw SvenVw commented Apr 2, 2026

Summary by CodeRabbit

  • Bug Fixes
    • Corrected nitrogen removal for crop residues: residues left in the field are no longer counted toward removal, while residues removed are properly included. This produces more accurate total nitrogen removal values in calculations and reports, resolving inconsistencies in scenarios with mixed residue handling.

… removal. When residues are removed from the field they are included in the nitrogen removal
@SvenVw SvenVw requested a review from gerardhros April 2, 2026 12:00
@SvenVw SvenVw self-assigned this Apr 2, 2026
@sentry
Copy link
Copy Markdown

sentry Bot commented Apr 2, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 2, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 40e7d86b-9b40-45c4-a66d-ce0f32e338c7

📥 Commits

Reviewing files that changed from the base of the PR and between 22d207a and ea6cc5b.

📒 Files selected for processing (3)
  • .changeset/better-snakes-clap.md
  • fdm-calculator/src/balance/nitrogen/removal/residue.test.ts
  • fdm-calculator/src/balance/nitrogen/removal/residue.ts
✅ Files skipped from review due to trivial changes (1)
  • .changeset/better-snakes-clap.md
🚧 Files skipped from review as they are similar to previous changes (2)
  • fdm-calculator/src/balance/nitrogen/removal/residue.ts
  • fdm-calculator/src/balance/nitrogen/removal/residue.test.ts

📝 Walkthrough

Walkthrough

Invert crop-residue short-circuit: residues left on-field are excluded from nitrogen removal while residues removed from-field are included; a changeset was added and tests updated to reflect the new behavior.

Changes

Cohort / File(s) Summary
Changeset Metadata
.changeset/better-snakes-clap.md
Add patch changeset for @nmi-agro/fdm-calculator documenting the crop-residue removal fix.
Nitrogen removal implementation
fdm-calculator/src/balance/nitrogen/removal/residue.ts
Invert the m_cropresidue short-circuit: only m_cropresidue === false returns zero; other values proceed with residue nitrogen computation.
Tests — removal index & residue
fdm-calculator/src/balance/nitrogen/removal/index.test.ts, fdm-calculator/src/balance/nitrogen/removal/residue.test.ts
Adjust test inputs and assertions to match the inverted m_cropresidue semantics (swap true/false in scenarios, update expected totals and residue components).

Sequence Diagram(s)

(No sequence diagrams generated — change is a localized control-flow inversion within a single module and test updates.)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested labels

bug, fdm-calculator

Suggested reviewers

  • gerardhros

Poem

🐰 I hopped through rows of code and grain,
Flipped a flag so residues behave plain.
Left on the soil? They skip the score,
Taken away? They count once more.
Happy hops — tidy fields, neat gain! 🥕✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title Check ✅ Passed Title check skipped as CodeRabbit has written the PR title.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-crop-residue

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot changed the title @coderabbitai Invert crop-residue nitrogen removal logic Apr 2, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
.changeset/better-snakes-clap.md (1)

5-5: Tighten the changelog sentence.

This is user-facing release-note text, so it is worth making it a bit clearer and using “crop residues” as two words.

✏️ Suggested wording
-Fix that when cropresidues are left they are not included in the nitrogen removal. When residues are removed from the field they are included in the nitrogen removal
+Fix nitrogen removal for crop residues: residues left on the field are no longer counted as removed, while residues removed from the field are.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.changeset/better-snakes-clap.md at line 5, Update the changelog sentence in
.changeset/better-snakes-clap.md to use “crop residues” (two words) and tighten
the wording: replace the current lines mentioning "cropresidues" and "nitrogen
removal" with a single clear sentence such as “Ensure crop residues left on the
field are excluded from nitrogen removal calculations; when residues are removed
from the field they are included in nitrogen removal.” Make sure the new text is
user-facing and succinct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@fdm-calculator/src/balance/nitrogen/removal/residue.ts`:
- Around line 42-43: The guard around the residue-removal calculation treats
null as "removed" because it only checks truthiness; change the condition so
null is excluded by explicitly guarding on cultivation.m_cropresidue (e.g.,
replace if (cultivation.m_cropresidue) with if (cultivation.m_cropresidue !==
false) or equivalent) so only an explicit false (residues removed) allows the
removal calculation to run; update the check in residue.ts where
cultivation.m_cropresidue is used to match the sibling implementations
(ammonia/residues and organic-matter/residues).

---

Nitpick comments:
In @.changeset/better-snakes-clap.md:
- Line 5: Update the changelog sentence in .changeset/better-snakes-clap.md to
use “crop residues” (two words) and tighten the wording: replace the current
lines mentioning "cropresidues" and "nitrogen removal" with a single clear
sentence such as “Ensure crop residues left on the field are excluded from
nitrogen removal calculations; when residues are removed from the field they are
included in nitrogen removal.” Make sure the new text is user-facing and
succinct.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: b6dfece9-8c34-4ca9-87b7-8a6bb7c2ce9f

📥 Commits

Reviewing files that changed from the base of the PR and between 6b5983e and c570b8a.

📒 Files selected for processing (2)
  • .changeset/better-snakes-clap.md
  • fdm-calculator/src/balance/nitrogen/removal/residue.ts

Comment thread fdm-calculator/src/balance/nitrogen/removal/residue.ts Outdated
@SvenVw
Copy link
Copy Markdown
Collaborator Author

SvenVw commented Apr 2, 2026

@coderabbitai resume

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 2, 2026

✅ Actions performed

Reviews resumed.

@SvenVw SvenVw merged commit fdd8dbd into development Apr 2, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant