Conversation
… is undefined or 0
|
|
Caution Review failedThe pull request is closed. WalkthroughAdds guards in nitrogen residue calculations to return zero when b_lu_hi is 0, preventing division-by-zero. Updates both removal and ammonia emission logic, adds corresponding tests, normalizes Decimal usage for fallback yields, and bumps version to 0.6.1 with a changelog entry. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant Calc as calculate…ByResidue(s)
participant Map as cultivationDetailsMap
C->>Calc: compute(cultivations, harvests, Map)
loop per cultivation
Calc->>Map: lookup cultivationDetail
Note over Calc: Get b_lu_hi
alt b_lu_hi == 0
Note right of Calc: Early return 0 for this cultivation
Calc-->>C: add {value: 0} to results
else b_lu_hi > 0
Note over Calc: Compute yield, factors, emission/removal
Calc-->>C: add computed Decimal value
end
end
Calc-->>C: total = sum(values)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 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 @@
## main #267 +/- ##
=======================================
Coverage 92.77% 92.78%
=======================================
Files 87 87
Lines 13030 13044 +14
Branches 1289 1294 +5
=======================================
+ Hits 12089 12103 +14
Misses 939 939
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]](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/removal/residue.ts (1)
76-81: Type mismatch in fallback yield assignment (Decimal vs number).
yieldForThisHarvestis typedDecimal | nullbut assigned anumber. This will trip TS and creates inconsistency vs the ammonia path (which wraps withnew Decimal(...)).Apply this fix to keep types strict and consistent:
- if (yieldForThisHarvest === null) { - yieldForThisHarvest = cultivationDetail.b_lu_yield - } + if (yieldForThisHarvest === null) { + yieldForThisHarvest = new Decimal(cultivationDetail.b_lu_yield ?? 0) + }
🧹 Nitpick comments (5)
.changeset/pink-readers-serve.md (1)
5-5: Polish the changeset wording.Tighten phrasing for clarity and conventional wording of this class of bug.
-Fix dividing by 0 error at nitrogen balance calculation when b_lu_hi is undefined or 0 +Fix divide-by-zero in nitrogen balance when b_lu_hi is undefined or 0fdm-calculator/src/balance/nitrogen/removal/residue.ts (2)
126-126: Typo in comment.- // Aggregate the total maount of Nitrogen removed by crop residues + // Aggregate the total amount of Nitrogen removed by crop residues
104-111: Optional: centralize shared yield/HI logic.Both removal and ammonia modules duplicate averaging-yield and HI handling. Consider extracting a small helper to reduce divergence.
fdm-calculator/src/balance/nitrogen/removal/residue.test.ts (1)
20-31: Catalogue entry has mismatched identifier.Map key is "catalogue2" but
b_lu_catalogueinside the object is "catalogue1". Fix to avoid confusion and future lookups elsewhere.- { - b_lu_catalogue: "catalogue1", + { + b_lu_catalogue: "catalogue2", b_lu_croprotation: "quinoa",Also, import style is inconsistent across code/tests (
import Decimal from "decimal.js"vsimport { Decimal }). Please standardize on the default import used in production code to avoid module interop surprises.fdm-calculator/src/balance/nitrogen/emission/ammonia/residues.test.ts (1)
262-314: Strengthen assertions for the HI = 0 case.Assert exact zeros to prove the guard path was taken, not a near‑zero numeric artifact.
- //Check for approximation due to floating point - expect(result.total.toNumber()).toBeCloseTo(0, 2) - expect(result.cultivations).toEqual([ - { id: "cultivation1", value: expect.any(Decimal) }, - ]) + expect(result.total.equals(new Decimal(0))).toBe(true) + expect(result.cultivations).toEqual([ + { id: "cultivation1", value: new Decimal(0) }, + ])Also consider an additional test where
b_lu_hiisundefinedincultivationDetailsMapto verify the?? 0path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/pink-readers-serve.md(1 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/residues.test.ts(1 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/residues.ts(1 hunks)fdm-calculator/src/balance/nitrogen/removal/residue.test.ts(2 hunks)fdm-calculator/src/balance/nitrogen/removal/residue.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-14T14:31:55.384Z
Learnt from: SvenVw
PR: SvenVw/fdm#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:
fdm-calculator/src/balance/nitrogen/emission/ammonia/residues.ts
🧬 Code graph analysis (2)
fdm-calculator/src/balance/nitrogen/removal/residue.test.ts (3)
fdm-core/src/db/schema.ts (1)
cultivations(289-301)fdm-calculator/src/balance/nitrogen/types.d.ts (1)
FieldInput(395-415)fdm-calculator/src/balance/nitrogen/removal/residue.ts (1)
calculateNitrogenRemovalByResidue(19-135)
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-143)
🔇 Additional comments (2)
fdm-calculator/src/balance/nitrogen/removal/residue.ts (1)
104-111: Good guard to avoid divide-by-zero.Early-return on zero HI is correct and aligns with the issue’s intent.
Two follow-ups:
- Consider treating non-physical values uniformly:
if (b_lu_hi.lte(0)) return 0and optionally clamp or rejectb_lu_hi.gt(1).- Please add a unit test for “b_lu_hi is undefined” to ensure the
?? 0path is covered.fdm-calculator/src/balance/nitrogen/removal/residue.test.ts (1)
232-254: LGTM: covers HI = 0 path for removal.Asserts total and per-cultivation value are 0. Complements the guard.
coderabbitai
Bot
added
the
branch:main
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
fdm-calculator/src/balance/nitrogen/removal/residue.ts (2)
106-113: Reword comment and optionally validate HI rangeThe comment says “no residues possible,” but b_lu_hi = 0 implies no harvestable part (all residues), not “no residues.” Suggest rewording, and consider guarding out-of-range HI values.
Apply this diff to adjust the comment:
- // If cultivation has no residues possible return 0 + // Avoid division by zero: when harvest index (HI) is 0 (no harvestable fraction), + // per spec return 0 removal for residues.Additionally (outside the selected lines), consider inserting a minimal validity check after computing b_lu_hi/b_lu_hi_res:
// After lines 97–100 // Treat invalid or out-of-range HI defensively if (b_lu_hi.lte(0) || b_lu_hi.gt(1)) { return { id: cultivation.b_lu, value: new Decimal(0) } }
78-81: Confirmed — the?? 0fallback is calculator-local; recommend an inline comment
- New-harvest UI passes b_lu_yield={undefined} and list display uses
?? "–"(fdm-app), and core handlers/schema accept/store null/undefined (fdm-core), so I found no evidence the calculator fallback is persisted or injected back into the UI.- The
?? 0occurrences are limited to calculator code (e.g. fdm-calculator/src/balance/nitrogen/removal/residue.ts:78 and similar ammonia/residues files) and are used only for internal computation.Optional action: add a short inline comment at fdm-calculator/src/balance/nitrogen/removal/residue.ts:78 (and the matching files) clarifying "calc-only fallback — do not persist or surface as actual yield" or rename the variable to make intent explicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/pink-readers-serve.md(1 hunks)fdm-calculator/src/balance/nitrogen/emission/ammonia/residues.test.ts(1 hunks)fdm-calculator/src/balance/nitrogen/removal/residue.test.ts(2 hunks)fdm-calculator/src/balance/nitrogen/removal/residue.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/pink-readers-serve.md
🚧 Files skipped from review as they are similar to previous changes (2)
- fdm-calculator/src/balance/nitrogen/emission/ammonia/residues.test.ts
- fdm-calculator/src/balance/nitrogen/removal/residue.test.ts
🧰 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:
fdm-calculator/src/balance/nitrogen/removal/residue.ts
🔇 Additional comments (3)
fdm-calculator/src/balance/nitrogen/removal/residue.ts (3)
106-113: Guard for b_lu_hi = 0 prevents divide-by-zero — LGTMThis early return cleanly avoids the crash and aligns with the PR objective to return Decimal(0).
78-81: Consistent Decimal arithmetic — good normalizationWrapping the fallback yield in Decimal avoids mixed number/Decimal math and subtle precision bugs.
128-128: Comment-only changeNo action needed.
2 participants
Summary by CodeRabbit
Closes #265