Hotfix/20250618 nutrient balance#169
Conversation
|
WalkthroughThis update introduces robust handling for potential null or undefined numeric values across multiple nitrogen balance calculation modules by defaulting them to zero. It also ensures consistent retrieval and usage of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Loader
participant Action
participant getCalendar
User->>Loader: Request page with route params
Loader->>getCalendar: Extract calendar from params
getCalendar-->>Loader: Return calendar value
Loader-->>User: Return data including calendar
User->>Action: Submit form with route params
Action->>getCalendar: Extract calendar from params
getCalendar-->>Action: Return calendar value
Action-->>User: Redirect using calendar in URL
Possibly related PRs
Suggested labels
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #169 +/- ##
=======================================
Coverage 93.07% 93.07%
=======================================
Files 68 68
Lines 8061 8063 +2
Branches 957 974 +17
=======================================
+ Hits 7503 7505 +2
Misses 556 556
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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
fdm-calculator/src/balance/nitrogen/volatization/residues.ts (1)
69-72: Potential crash still lurking here
analysisWithYield.b_lu_yieldis passed straight tonew Decimal(...)without a?? 0guard.
If the analysis containsnullthis line will still throw. Apply the same pattern as the other conversions.-yieldForThisHarvest = new Decimal(analysisWithYield.b_lu_yield) +yieldForThisHarvest = new Decimal(analysisWithYield.b_lu_yield ?? 0)fdm-calculator/src/balance/nitrogen/supply/fertilizers/compost.ts (1)
1-1: Incorrect import style – will break at runtime
decimal.jsexports a default. Using a named import{ Decimal }results inundefined, so everynew Decimal(...)will crash.-import { Decimal } from "decimal.js" +import Decimal from "decimal.js"Make sure other modules don’t use the same pattern.
fdm-calculator/src/balance/nitrogen/target.ts (1)
112-114: Duplicate assignment hides the intended 115 kg N ha⁻¹ value
targetValueis immediately overwritten from115to125, making the first assignment dead code and potentially skewing results for the clay-dry class.- targetValue = new Decimal(115) - targetValue = new Decimal(125) + targetValue = new Decimal(115)Please confirm which value is correct and remove the redundant line.
🧹 Nitpick comments (8)
.changeset/vast-onions-judge.md (1)
5-5: Polish the changelog wording for clarity
Fixes redirects at harvest details page→Fixes redirects on the harvest-details page.Clearer wording and correct preposition will make the changeset more professional.
.changeset/cyan-peaches-wait.md (1)
5-5: Refine the description to read naturallyCurrent sentence is a bit clunky. Consider:
-Fix at balance calculation to convert null values to 0 and prevent exception due to Decimal +Fix balance calculations by converting null values to 0, preventing Decimal-constructor exceptionsfdm-calculator/src/balance/nitrogen/volatization/residues.ts (3)
78-81: Consistent defaulting looks good, but avoid magicDecimal(0)literals everywhereThe null-coalescing on
b_lu_yieldis correct. To avoid scattering?? 0/new Decimal(0)across the codebase, consider a tiny helper:export const toDecimal = (v: number | string | null | undefined) => new Decimal(v ?? 0)Then:
- yieldForThisHarvest = new Decimal(cultivationDetail.b_lu_yield ?? 0) + yieldForThisHarvest = toDecimal(cultivationDetail.b_lu_yield)
92-93: Same helper could tidy this blockRepeated
new Decimal(x ?? 0)pattern again – the suggested helper would reduce duplication and risk of forgetting the default in future edits.
98-99: Minor spacing nit
??0→?? 0for consistency with the rest of the file.-const b_lu_hi = new Decimal(cultivationDetail.b_lu_hi ??0) +const b_lu_hi = new Decimal(cultivationDetail.b_lu_hi ?? 0)fdm-calculator/src/balance/nitrogen/supply/fertilizers/compost.ts (1)
52-53: Mirror the helper suggestionSame duplication comment as in residues.ts – a shared
toDecimal()helper would keep this conversion terse and consistent.fdm-calculator/src/balance/nitrogen/target.ts (2)
30-39: ReplacemapwithforEach(orsome) to clarify intent
cultivations.map(...)is used purely for its side-effects; the resulting array is discarded.
UsingforEach(orsomeif you want an early exit) better communicates intent and avoids the tiny overhead/allocation of an unused array.-cultivations.map((cultivation) => { +cultivations.forEach((cultivation) => {
129-129: Avoid double-wrapping Decimal and keep the fallback concise
targetValueis already aDecimal(when defined). Wrapping it again is harmless but unnecessary.
A clearer pattern:-const target = new Decimal(targetValue ?? 0).times(timeFrameFraction) +const target = (targetValue ?? new Decimal(0)).times(timeFrameFraction)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.changeset/cyan-peaches-wait.md(1 hunks).changeset/vast-onions-judge.md(1 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.cultivation.$b_lu.harvest.$b_id_harvesting.tsx(6 hunks)fdm-calculator/src/balance/nitrogen/index.ts(2 hunks)fdm-calculator/src/balance/nitrogen/removal/residue.ts(2 hunks)fdm-calculator/src/balance/nitrogen/supply/fertilizers/compost.ts(2 hunks)fdm-calculator/src/balance/nitrogen/supply/fertilizers/manure.ts(2 hunks)fdm-calculator/src/balance/nitrogen/supply/fertilizers/mineral.ts(2 hunks)fdm-calculator/src/balance/nitrogen/supply/fertilizers/other.ts(2 hunks)fdm-calculator/src/balance/nitrogen/supply/mineralization.ts(1 hunks)fdm-calculator/src/balance/nitrogen/target.ts(1 hunks)fdm-calculator/src/balance/nitrogen/volatization/residues.ts(2 hunks)
🧰 Additional context used
🪛 LanguageTool
.changeset/vast-onions-judge.md
[uncategorized] ~5-~5: The preposition ‘to’ seems more likely in this position.
Context: ...vw/fdm-app": patch --- Fixes redirects at harvest details page
(AI_HYDRA_LEO_REPLACE_AT_TO)
⏰ Context from checks skipped due to timeout of 300000ms (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: core (22)
- GitHub Check: calculator (22)
🔇 Additional comments (12)
fdm-calculator/src/balance/nitrogen/supply/mineralization.ts (1)
45-48: 👍 RedundantDecimalconstructor call removed
mineralizationValueis already aDecimal; deleting the extra wrapper avoids an unnecessary allocation while preserving behaviour. Nice cleanup.fdm-calculator/src/balance/nitrogen/supply/fertilizers/compost.ts (1)
41-42: Good null-safety additionDefaulting
p_n_rtto zero preventsDecimalconstructor errors. Once the import is fixed this line is solid.fdm-calculator/src/balance/nitrogen/supply/fertilizers/manure.ts (2)
40-41: Good guard against null/undefined nitrogen rateThe
?? 0fallback preventsDecimalconstruction errors whenp_n_rtis missing. 👍
51-52: Consistent null-safe handling for application amountSame remark as above—this hardens the calculation path.
No further issues spotted here.fdm-calculator/src/balance/nitrogen/supply/fertilizers/mineral.ts (2)
39-40: Null-safe nitrogen rate looks goodThe
?? 0plus unit conversion is correct and prevents crashes.
LGTM.
50-51: Application amount fallback is correctKeeps calculations robust when data is incomplete. No objections.
fdm-calculator/src/balance/nitrogen/supply/fertilizers/other.ts (2)
36-37: Safe default forp_n_rtNullish coalescing prevents runtime issues. Looks good.
51-52: Safe default forp_app_amountSame positive comment as above. Implementation is sound.
fdm-calculator/src/balance/nitrogen/index.ts (2)
200-203: Graceful handling of missingb_areaDefaulting to zero avoids NaNs in farm-level aggregates. 👍
222-223: Field-level area fallback consistent with farm-level logicChange mirrors the total-area fix. All good.
fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.cultivation.$b_lu.harvest.$b_id_harvesting.tsx (1)
84-85: ```shell
#!/usr/bin/env bash
set -eecho "=== Listing project-level lib directory ==="
ls -1 fdm-app/lib || echo "fdm-app/lib not found"echo
echo "=== Calendar library files at fdm-app/lib ==="
ls -1 fdm-app/lib/calendar* || echo "No calendar files in fdm-app/lib"echo
echo "=== getCalendar implementation in calendar files ==="
rg -n -C3 "export (async )?function getCalendar" -g "fdm-app/lib/calendar*"</details> <details> <summary>fdm-calculator/src/balance/nitrogen/removal/residue.ts (1)</summary> `90-101`: **Good defensive defaults added** Using `?? 0` before constructing new `Decimal` objects prevents `DecimalError: NaN` exceptions when backend data is `null`. This hardens the calculation path. </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: 0
🧹 Nitpick comments (1)
fdm-app/CHANGELOG.md (1)
7-7: Refine preposition in commit message
Use “on the harvest details page” instead of “at harvest details page” for clarity.
Apply this diff:- - f2b1fc6: Fixes redirects at harvest details page + - f2b1fc6: Fixes redirects on the harvest details page
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
fdm-app/CHANGELOG.md(1 hunks)fdm-app/package.json(1 hunks)fdm-calculator/CHANGELOG.md(1 hunks)fdm-calculator/package.json(1 hunks)fdm-calculator/src/balance/nitrogen/removal/residue.ts(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- fdm-calculator/CHANGELOG.md
- fdm-app/package.json
- fdm-calculator/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- fdm-calculator/src/balance/nitrogen/removal/residue.ts
🧰 Additional context used
🪛 LanguageTool
fdm-app/CHANGELOG.md
[uncategorized] ~7-~7: The preposition ‘to’ seems more likely in this position.
Context: ...tch Changes - f2b1fc6: Fixes redirects at harvest details page - Updated dependen...
(AI_HYDRA_LEO_REPLACE_AT_TO)
⏰ Context from checks skipped due to timeout of 300000ms (5)
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: calculator (22)
- GitHub Check: data (22)
- GitHub Check: core (22)
🔇 Additional comments (2)
fdm-app/CHANGELOG.md (2)
3-5: Consistent formatting for new version entry
The header## 0.19.6and its “Patch Changes” section match the style of prior entries—no changes needed here.
8-9: Verify dependency update commit and version
Ensure that commit94a82f6corresponds to the release of@svenvw/fdm-calculator@0.3.3.
Run this script to confirm via GitHub’s API:#!/bin/bash # Verify commit exists in fdm-calculator and tag v0.3.3 is present curl -s https://api.github.com/repos/SvenVw/fdm-calculator/commits/94a82f6 | jq '{sha: .sha, message: .commit.message}' curl -s https://api.github.com/repos/SvenVw/fdm-calculator/tags | jq '.[] | select(.name=="v0.3.3")'
1 participant
@coderabbitai summart
Summary by CodeRabbit
Bug Fixes
Chores