Replace falsy checks with nullish-specific checks#503
Merged
Conversation
…culations, ensuring accurate results when valid zero or false inputs are provided.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces falsy guards with explicit null/undefined checks and nullish coalescing (??/??=) across fdm-calculator so legitimate 0 and false values are preserved in soil conversions, NL2025/NL2026 norm calculations, and harvest nitrogen removal; adds regression tests covering zero-value scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
coderabbitai
Bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
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/norms/nl/2026/value/stikstofgebruiksnorm.ts (1)
86-100:⚠️ Potential issue | 🟠 MajorThe falsy-period bug is still present in this matcher.
These
??changes are only partial.if (sub.period_start_month && sub.period_end_month)still treats0as missing, the sort key still collapses0andundefinedto the same value, and the fallback matcher on Lines 159-173 still uses|| 1. A0period value can still be skipped or re-defaulted here.Also applies to: 137-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/norms/nl/2026/value/stikstofgebruiksnorm.ts` around lines 86 - 100, The filter and matching logic in potentialMatches (and the other matcher block) treat 0 as falsy so period values of 0 get dropped or conflated with undefined; change the presence checks from truthy checks to explicit null/undefined checks (e.g., replace `if (sub.period_start_month && sub.period_end_month)` with `sub.period_start_month != null && sub.period_end_month != null`), stop collapsing 0/undefined in your sort key by using nullish checks or an explicit sentinel when building the sort key (e.g., use `sub.period_start_month === undefined ? <sentinel> : sub.period_start_month` or `??` instead of `||` so 0 is preserved), and update the fallback matcher (the block that currently uses `|| 1`) to use `??` or explicit undefined checks so defaulting doesn’t override a valid 0 period value; apply the same fixes to the analogous code around the other matcher.fdm-calculator/src/norms/nl/2025/value/stikstofgebruiksnorm.ts (1)
176-190:⚠️ Potential issue | 🟠 MajorThe NL2025 subtype matcher still drops explicit
0values.This only fixes part of the path.
if (sub.period_start_month && sub.period_end_month)still rejects0, the sort key still makes0andundefinedequivalent, and the fallback branch on Lines 249-264 still uses|| 1. So the falsy-check regression is still reachable here.Also applies to: 227-240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/norms/nl/2025/value/stikstofgebruiksnorm.ts` around lines 176 - 190, The subtype matching currently treats 0 as falsy and conflates undefined with 0: replace the truthy checks like if (sub.period_start_month && sub.period_end_month) with explicit null/undefined checks (e.g., sub.period_start_month != null && sub.period_end_month != null), change any fallbacks using || (e.g., sub.period_start_day || 1) to nullish coalescing ?? (e.g., sub.period_start_day ?? 1) so 0 is preserved, and adjust the sort/key logic to use a distinct sentinel for missing values (for example (sub.period_start_month ?? -1) or similar) so 0 and undefined are not treated equivalently; apply the same fixes for the other occurrence around the alternative branch and the fallback branch that builds start/end periods and the sort key (the potentialMatches filter, startPeriod/endPeriod creation, sort key, and the fallback branch).
🧹 Nitpick comments (1)
fdm-calculator/src/balance/nitrogen/removal/harvest.test.ts (1)
322-325: Don’t lock this regression test toDecimal("-0").The behavior that matters here is “explicit zero stays zero”, not whether
decimal.jspreserves a signed-zero representation. Asserting onnew Decimal("-0")makes the test brittle for no real product benefit.Suggested fix
- expect(result).toEqual({ - total: new Decimal(0), - harvests: [{ id: "harvest1", value: new Decimal("-0") }], - }) + expect(result.total.equals(0)).toBe(true) + expect(result.harvests).toHaveLength(1) + expect(result.harvests[0]).toMatchObject({ id: "harvest1" }) + expect(result.harvests[0].value.equals(0)).toBe(true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/balance/nitrogen/removal/harvest.test.ts` around lines 322 - 325, The test currently asserts a signed zero (new Decimal("-0")) which is brittle; change the expectation to assert a plain zero instead so the spec "explicit zero stays zero" holds without depending on sign. In harvest.test.ts update the expected harvest value (and any similar assertions on result.total) to use new Decimal(0) or assert value.isZero() for the harvest with id "harvest1" (referencing the result variable and the harvests array) so the test checks zero semantics without relying on decimal.js signed-zero formatting.
🤖 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/conversions/soil.ts`:
- Around line 13-17: Update the parameter types to allow undefined where the
functions explicitly guard for it: add "| undefined" to the input type of
calculateOrganicCarbon and the three other exported conversion functions in this
file that currently check "=== null || === undefined" (the ones whose guards are
at lines 43, 73, and 106). This will align the TypeScript signatures with the
runtime checks and the test cases that pass undefined; adjust the parameter
declarations so they accept fdmSchema...TypeSelect["..."] | undefined for each
corresponding input.
In `@fdm-calculator/src/norms/nl/2025/value/dierlijke-mest-gebruiksnorm.test.ts`:
- Around line 25-39: The test uses an omitted is_derogatie_bedrijf (undefined)
which behaves the same under both || false and ?? false, so change the
regression test to explicitly set farm.is_derogatie_bedrijf to null to exercise
the null/undefined branch, and add a second test case where
farm.is_derogatie_bedrijf is a falsy-but-not-null value (e.g., 0) to confirm the
new ?? false behavior; locate and update the mockInput in the test for
calculateNL2025DierlijkeMestGebruiksNorm to include these explicit values and
assert the same expected normValue/normSource.
In `@fdm-calculator/src/norms/nl/2025/value/stikstofgebruiksnorm.test.ts`:
- Around line 763-782: The test currently never exercises zero-valued period_*
fields because those values come from stikstofgebruiksnorm-data, not mockInput;
update the test "should handle explicit zero values for period days/months
(regression test for falsy bug)" to (1) inject or stub the lookup data from
stikstofgebruiksnorm-data so the returned cultivation entry contains explicit
period_*: 0 (e.g., by mocking the data module or the resolver used by
calculateNL2025StikstofGebruiksNorm), and (2) strengthen the assertion to verify
the exact expected subtype and numeric result (for example assert result.subtype
=== 'tijdelijk_grasland' and result.normValue equals the known expected value or
a precise range) instead of the weak toBeGreaterThan(0) check; reference
calculateNL2025StikstofGebruiksNorm and the stikstofgebruiksnorm-data module
when making the mock/stub.
In `@fdm-calculator/src/norms/nl/2026/value/stikstofgebruiksnorm.test.ts`:
- Around line 705-720: The test currently doesn't exercise the zero-valued
period fields in stikstofgebruiksnorm-data; update the test for
calculateNL2026StikstofGebruiksNorm to explicitly use (or mock) a
stikstofgebruiksnorm-data entry whose period_days or period_months is 0 so the
zero-sensitive logic is exercised. Specifically, either import/mock the
stikstofgebruiksnorm-data used by calculateNL2026StikstofGebruiksNorm and add an
entry with period_days: 0 (and/or period_months: 0) and set your cultivation's
b_lu_catalogue to match that entry, or stub the data loader function the routine
uses; then assert not only that result.normValue > 0 but also that the returned
period identifier (or matched data object) equals the expected entry to ensure
correct period matching.
---
Outside diff comments:
In `@fdm-calculator/src/norms/nl/2025/value/stikstofgebruiksnorm.ts`:
- Around line 176-190: The subtype matching currently treats 0 as falsy and
conflates undefined with 0: replace the truthy checks like if
(sub.period_start_month && sub.period_end_month) with explicit null/undefined
checks (e.g., sub.period_start_month != null && sub.period_end_month != null),
change any fallbacks using || (e.g., sub.period_start_day || 1) to nullish
coalescing ?? (e.g., sub.period_start_day ?? 1) so 0 is preserved, and adjust
the sort/key logic to use a distinct sentinel for missing values (for example
(sub.period_start_month ?? -1) or similar) so 0 and undefined are not treated
equivalently; apply the same fixes for the other occurrence around the
alternative branch and the fallback branch that builds start/end periods and the
sort key (the potentialMatches filter, startPeriod/endPeriod creation, sort key,
and the fallback branch).
In `@fdm-calculator/src/norms/nl/2026/value/stikstofgebruiksnorm.ts`:
- Around line 86-100: The filter and matching logic in potentialMatches (and the
other matcher block) treat 0 as falsy so period values of 0 get dropped or
conflated with undefined; change the presence checks from truthy checks to
explicit null/undefined checks (e.g., replace `if (sub.period_start_month &&
sub.period_end_month)` with `sub.period_start_month != null &&
sub.period_end_month != null`), stop collapsing 0/undefined in your sort key by
using nullish checks or an explicit sentinel when building the sort key (e.g.,
use `sub.period_start_month === undefined ? <sentinel> : sub.period_start_month`
or `??` instead of `||` so 0 is preserved), and update the fallback matcher (the
block that currently uses `|| 1`) to use `??` or explicit undefined checks so
defaulting doesn’t override a valid 0 period value; apply the same fixes to the
analogous code around the other matcher.
---
Nitpick comments:
In `@fdm-calculator/src/balance/nitrogen/removal/harvest.test.ts`:
- Around line 322-325: The test currently asserts a signed zero (new
Decimal("-0")) which is brittle; change the expectation to assert a plain zero
instead so the spec "explicit zero stays zero" holds without depending on sign.
In harvest.test.ts update the expected harvest value (and any similar assertions
on result.total) to use new Decimal(0) or assert value.isZero() for the harvest
with id "harvest1" (referencing the result variable and the harvests array) so
the test checks zero semantics without relying on decimal.js signed-zero
formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cf39ff2d-4a72-48d9-bc6f-55f830811bfa
📒 Files selected for processing (15)
.changeset/spotty-planets-worry.mdfdm-calculator/src/balance/nitrogen/removal/harvest.test.tsfdm-calculator/src/balance/nitrogen/removal/harvest.tsfdm-calculator/src/conversions/soil.test.tsfdm-calculator/src/conversions/soil.tsfdm-calculator/src/norms/nl/2025/value/dierlijke-mest-gebruiksnorm.test.tsfdm-calculator/src/norms/nl/2025/value/dierlijke-mest-gebruiksnorm.tsfdm-calculator/src/norms/nl/2025/value/fosfaatgebruiksnorm.test.tsfdm-calculator/src/norms/nl/2025/value/fosfaatgebruiksnorm.tsfdm-calculator/src/norms/nl/2025/value/stikstofgebruiksnorm.test.tsfdm-calculator/src/norms/nl/2025/value/stikstofgebruiksnorm.tsfdm-calculator/src/norms/nl/2026/value/fosfaatgebruiksnorm.test.tsfdm-calculator/src/norms/nl/2026/value/fosfaatgebruiksnorm.tsfdm-calculator/src/norms/nl/2026/value/stikstofgebruiksnorm.test.tsfdm-calculator/src/norms/nl/2026/value/stikstofgebruiksnorm.ts
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
fdm-calculator/src/conversions/soil.ts (1)
103-108: Use a nullish check forb_soiltype_agrhere too.
!b_soiltype_agris the last broad falsy guard left in this file. Switching it to a nullish check keeps this helper aligned with the PR’s stated behavior change and avoids mixing two missing-value semantics in the same module.Suggested diff
export function calculateBulkDensity( a_som_loi: fdmSchema.soilAnalysisTypeSelect["a_som_loi"] | undefined, b_soiltype_agr: | fdmSchema.soilAnalysisTypeSelect["b_soiltype_agr"] | undefined, ): fdmSchema.soilAnalysisTypeSelect["a_density_sa"] { - if (a_som_loi === null || a_som_loi === undefined || !b_soiltype_agr) { + if ( + a_som_loi === null || + a_som_loi === undefined || + b_soiltype_agr === null || + b_soiltype_agr === undefined + ) { return null }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/conversions/soil.ts` around lines 103 - 108, The guard in the helper that checks b_soiltype_agr mixes falsy semantics with the explicit null/undefined checks used for a_som_loi; change the condition in the function that returns fdmSchema.soilAnalysisTypeSelect["a_density_sa"] so b_soiltype_agr is checked for nullishness (b_soiltype_agr === null || b_soiltype_agr === undefined or equivalently b_soiltype_agr == null) instead of using the broad !b_soiltype_agr check, keeping the parameter checks consistent with a_som_loi and the module's intended missing-value semantics.
🤖 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/norms/nl/2025/value/stikstofgebruiksnorm.ts`:
- Around line 239-251: The comparator builds aStart/bStart and aEnd/bEnd using
(period_*_day ?? -1) which treats missing day as -1, but startPeriod/endPeriod
treat missing day as 1; update the comparator to use (period_*_day ?? 1) for
both start and end calculations so omitted day values produce the same ordering
as the constructed startPeriod/endPeriod used elsewhere (change the expressions
that compute aStart, bStart, aEnd, bEnd to use ?? 1 instead of ?? -1).
---
Nitpick comments:
In `@fdm-calculator/src/conversions/soil.ts`:
- Around line 103-108: The guard in the helper that checks b_soiltype_agr mixes
falsy semantics with the explicit null/undefined checks used for a_som_loi;
change the condition in the function that returns
fdmSchema.soilAnalysisTypeSelect["a_density_sa"] so b_soiltype_agr is checked
for nullishness (b_soiltype_agr === null || b_soiltype_agr === undefined or
equivalently b_soiltype_agr == null) instead of using the broad !b_soiltype_agr
check, keeping the parameter checks consistent with a_som_loi and the module's
intended missing-value semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f037968b-95f7-4ce2-a60f-e99917efff69
📒 Files selected for processing (7)
fdm-calculator/src/balance/nitrogen/removal/harvest.test.tsfdm-calculator/src/conversions/soil.tsfdm-calculator/src/norms/nl/2025/value/dierlijke-mest-gebruiksnorm.test.tsfdm-calculator/src/norms/nl/2025/value/stikstofgebruiksnorm.test.tsfdm-calculator/src/norms/nl/2025/value/stikstofgebruiksnorm.tsfdm-calculator/src/norms/nl/2026/value/stikstofgebruiksnorm.test.tsfdm-calculator/src/norms/nl/2026/value/stikstofgebruiksnorm.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- fdm-calculator/src/norms/nl/2026/value/stikstofgebruiksnorm.test.ts
- fdm-calculator/src/norms/nl/2025/value/stikstofgebruiksnorm.test.ts
- fdm-calculator/src/balance/nitrogen/removal/harvest.test.ts
- fdm-calculator/src/norms/nl/2026/value/stikstofgebruiksnorm.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/conversions/soil.ts`:
- Around line 70-73: Add a regression unit test that calls
calculateCarbonNitrogenRatio(10, 0) and asserts it returns null (matching
existing tests that assert null for invalid inputs), so the branch that treats
a_n_rt = 0 as invalid (to avoid division by zero) remains covered; place the
assertion alongside the other null/invalid cases in the soil tests and include a
short comment that this ensures 0 is intentionally treated as invalid.
🪄 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: cc1286a0-5284-49ad-9203-a0db7dc2db32
📒 Files selected for processing (2)
fdm-calculator/src/conversions/soil.tsfdm-calculator/src/norms/nl/2025/value/stikstofgebruiksnorm.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- fdm-calculator/src/norms/nl/2025/value/stikstofgebruiksnorm.ts
BoraIneviNMI
approved these changes
Mar 11, 2026
Collaborator
BoraIneviNMI
left a comment
BoraIneviNMI
left a comment
There was a problem hiding this comment.
I went through the changed lines and tried to run the test myself. It is nice that you added regression test after making these changes. Also all previous review comments seem to be adressed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Tests
closes #502