Support common units for fertilizer application amounts (l/ha, m3/ha, ton/ha)#565
Merged
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 15 minutes and 17 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds per-fertilizer application units (kg/ha, l/ha, m3/ha, ton/ha) across DB, core, app, data and calculator: migration, unit conversion helpers, API changes to accept/display p_app_amount_display and p_app_amount_unit, UI/schema updates, and type/test adjustments to preserve stored kg/ha while showing user units. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant AppForm as "fdm-app Form"
participant AppRoute as "fdm-app Route"
participant CoreAPI as "fdm-core API"
participant DB as "Database"
participant Conversion as "Conversion (to/fromKgPerHa)"
User->>AppForm: Enter p_app_amount_display + select fertilizer
AppForm->>AppRoute: Submit p_app_amount_display, fertilizer id
AppRoute->>CoreAPI: addFertilizerApplication(p_app_amount_display, fertId)
CoreAPI->>DB: SELECT fertilizer (p_app_amount_unit, p_density)
DB-->>CoreAPI: return unit, density
CoreAPI->>Conversion: toKgPerHa(p_app_amount_display, unit, density)
Conversion-->>CoreAPI: return p_app_amount (kg/ha)
CoreAPI->>DB: INSERT application with p_app_amount (kg/ha)
DB-->>CoreAPI: success
Note right of User: Later, listing
AppRoute->>CoreAPI: getFertilizerApplications(...)
CoreAPI->>DB: SELECT applications + fertilizer unit/density
DB-->>CoreAPI: return rows
CoreAPI->>Conversion: fromKgPerHa(p_app_amount, unit, density) for each
Conversion-->>CoreAPI: return p_app_amount_display
CoreAPI-->>AppRoute: applications with p_app_amount_display + unit
AppRoute-->>AppForm: render "X unit"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
BoraIneviNMI
commented
Apr 8, 2026
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.fertilizer._index.tsx (1)
332-381:⚠️ Potential issue | 🔴 CriticalPUT still writes the old amount field.
The POST branch was switched to
p_app_amount_display, but the PUT branch below still destructures/passesp_app_amount. Editing a non-kg/haapplication from this screen will be stored with the wrong quantity.🩹 Suggested fix
- const { p_app_id, p_id, p_app_amount, p_app_date, p_app_method } = + const { + p_app_id, + p_id, + p_app_amount_display, + p_app_date, + p_app_method, + } = formValues @@ - p_app_amount, + p_app_amount_display,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.field.$b_id.fertilizer._index.tsx around lines 332 - 381, The PUT branch still reads and passes p_app_amount instead of the display-aware p_app_amount_display, causing wrong quantities to be stored; update the FormSchemaModify extraction to destructure p_app_amount_display (matching the POST branch) and pass p_app_amount_display into updateFertilizerApplication (keep parameter order consistent with addFertilizerApplication), and ensure any validation/error messages reference p_app_amount_display where applicable.fdm-app/app/components/blocks/fertilizer-applications/types.d.tsx (1)
4-20:⚠️ Potential issue | 🟠 Major
FertilizerApplicationis still missing the new read-model fields.
columns.tsxandlist.tsxnow readp_app_amount_displayandp_app_amount_unit, but this interface only exposesp_app_amount. That leaves the changed UI typed against the old contract and hides thenulldisplay case when density is missing.🩹 Suggested type update
export interface FertilizerApplication { p_app_id: string p_app_ids: string[] p_name_nl: string p_app_amount: number + p_app_amount_display: number | null + p_app_amount_unit: AppAmountUnit p_app_date: Date p_app_method: ApplicationMethods | null }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/fertilizer-applications/types.d.tsx` around lines 4 - 20, Update the FertilizerApplication type to match the new read-model: add the display and unit fields used by the UI — include p_app_amount_display: string | null and p_app_amount_unit: AppAmountUnit | null on the FertilizerApplication interface (keeping existing p_app_amount and p_app_method as-is) so columns/list components that read p_app_amount_display and p_app_amount_unit are correctly typed and the null display case when density is missing is represented.fdm-app/app/components/blocks/gerrit/types.ts (1)
11-17:⚠️ Potential issue | 🟠 MajorPropagate the application unit through Gerrit's plan DTO.
ParsedPlanApplicationnow exposes onlyp_app_amount_display. The Gerrit table still readsapp.p_app_amountand hardcodeskg/ha, so non-kg plans either type-break or render the wrong value/unit. Addp_app_amount_unithere as well and migrate the consumer top_app_amount_display.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/gerrit/types.ts` around lines 11 - 17, The ParsedPlanApplication DTO is missing the application unit; add a new field p_app_amount_unit to the ParsedPlanApplication interface alongside p_app_amount_display, and update any consumers (notably the Gerrit table rendering logic that currently reads app.p_app_amount and hardcodes "kg/ha") to use app.p_app_amount_display for the numeric/value and app.p_app_amount_unit for the unit instead of assuming kg/ha; ensure downstream code expects p_app_amount_unit as string | null if units can be absent.fdm-app/app/routes/farm.$b_id_farm.$calendar.gerrit.tsx (1)
262-275:⚠️ Potential issue | 🔴 CriticalPreview metrics are feeding display-unit amounts into kg/ha calculators.
syntheticAppsonly getsp_app_amount_displayand is then cast toFertilizerApplicationbefore it is passed tocalculateDose, norm filling, OM balance, and N balance. Forl/ha,m3/ha, andton/ha, Gerrit's preview numbers will be wrong, and density-dependent cases will not fail until save. Convert each planned amount to kg/ha and populatep_app_amountbefore calling the calculator, or move this preview calculation behind anfdm-coreAPI that already owns the conversion logic.Also applies to: 352-355
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.gerrit.tsx around lines 262 - 275, syntheticApps is only setting p_app_amount_display and then casting to FertilizerApplication, causing calculators (calculateDose, norm filling, OM/N balance) to receive display units; update the syntheticApps creation to convert each app.p_app_amount_display into the canonical kg/ha value and set p_app_amount (using the same conversion logic fdm-core uses or by calling the fdm-core conversion API) before casting, ensuring density-dependent units (l/ha, m3/ha, ton/ha) are converted and any conversion failures are surfaced; apply the same fix to the other synthetic preview block that builds FertilizerApplication-like objects so all previews use p_app_amount in kg/ha.fdm-app/app/routes/farm.$b_id_farm.$calendar.field.fertilizer._index.tsx (2)
217-280:⚠️ Potential issue | 🟠 MajorBulk-edit prefill still uses stored kg/ha values.
The modify loader keeps comparing/seeding
p_app_amount, so anl/ha,m3/ha, orton/haapplication opens with the converted kg/ha amount instead of the user-facing display value. This flow should prefill fromp_app_amount_display, falling back top_app_amountonly when the display value isnull.🩹 Suggested direction
const exampleFertilizerApplication: Partial<FertilizerApplication> = { ...fertilizerApplications[0], + p_app_amount_display: + fertilizerApplications[0].p_app_amount_display ?? + fertilizerApplications[0].p_app_amount, } @@ const keyTypes = { p_id: "string", p_app_date: "date", p_app_method: "string", - p_app_amount: "number", + p_app_amount_display: "number", } as const @@ - delete exampleFertilizerApplication.p_app_amount + delete exampleFertilizerApplication.p_app_amount_display🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.field.fertilizer._index.tsx around lines 217 - 280, The prefill logic uses raw p_app_amount (kg/ha) instead of the user-facing value; change the selection and comparison to prefer p_app_amount_display with fallback to p_app_amount: when building exampleFertilizerApplication and when constructing fertilizerApplication from keys (and inside the number branch that compares values) read value = app.p_app_amount_display ?? app.p_app_amount and example = exampleFertilizerApplication.p_app_amount_display ?? exampleFertilizerApplication.p_app_amount (treat null/undefined the same), then perform the numeric equality/difference checks on those derived values; keep all other key handling (keyTypes, date branch, deletion logic) the same and ensure keys/p_app_amount are deleted only when both display and raw are missing.
622-650:⚠️ Potential issue | 🔴 CriticalBulk update still submits the pre-PR amount contract.
updateFertilizerApplicationnow expects the display-unit amount, but this branch still sendsvalidatedData.p_app_amount ?? original.p_app_amount. Saving mixed-unit edits will reinterpret those numbers as kg/ha.🩹 Suggested fix
- validatedData.p_app_amount ?? original.p_app_amount, + validatedData.p_app_amount_display ?? + original.p_app_amount_display ?? + original.p_app_amount,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.field.fertilizer._index.tsx around lines 622 - 650, The bulk-update branch is still passing raw/mixed-unit p_app_amount values into updateFertilizerApplication (using validatedData.p_app_amount ?? original.p_app_amount), but updateFertilizerApplication now expects a display-unit amount; convert both the incoming validatedData.p_app_amount and fallback original.p_app_amount to the display-unit representation before calling updateFertilizerApplication. Locate the call site that builds the arguments for updateFertilizerApplication (the p_app_amount parameter) and replace the direct value with the result of the same conversion utility used elsewhere in the codebase (e.g., amountToDisplayUnit/convertToDisplayAmount or the project’s unit conversion helper), applying it to validatedData.p_app_amount when present and to original.p_app_amount for the fallback so mixed-unit edits are normalized to display units.
🧹 Nitpick comments (3)
fdm-core/src/fertilizer.test.ts (2)
1378-1405: Assert the new parameter directly instead of only bumping the count.
toHaveLength(25)will still pass ifp_app_amount_unitdisappears and some unrelated entry is added. A targeted assertion on thep_app_amount_unititem and its four allowed options will make this test actually cover the new metadata.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/fertilizer.test.ts` around lines 1378 - 1405, The test using getFertilizerParametersDescription should not only assert length; update the test to explicitly find the parameter with parameter === "p_app_amount_unit" and assert it exists, has type "enum", and that its options array contains exactly the four expected option values (assert length and/or exact members) and that unit/name/description/category properties exist; locate this validation within the same test block that currently checks NL-nl/default locale and add assertions against the p_app_amount_unit object to ensure the new metadata is present and correct.
98-112: The new unit field is only being threaded through, not tested.The same
p_app_amount_unit: undefinedpattern is repeated across the updated fixtures in this file, so these CRUD tests still only exercise the legacykg/hapath. Please add at least one non-default unit case and assert that the returned fertilizer/application includes the expectedp_app_amount_unitand converted display amount.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/fertilizer.test.ts` around lines 98 - 112, Update the test that calls addFertilizerToCatalogue in fertilizer.test.ts to include at least one non-default unit by setting p_app_amount_unit to a non-kg/ha value (e.g., "g/m2" or "lbs/acre") instead of undefined, then assert on the returned result from addFertilizerToCatalogue that the p_app_amount_unit equals the non-default unit and that the returned fertilizer/application contains the expected converted display amount (check the returned object's display/amount field produced by the addFertilizerToCatalogue call). Ensure you modify the fixture values used in the test and add explicit assertions for p_app_amount_unit and the converted display amount so the non-default unit path is exercised.fdm-core/src/fertilizer.d.ts (1)
75-76: Consider usingAppAmountUnittype for consistency.The
FertilizerApplication.p_app_amount_unitusesstring | nullwhileFertilizerCatalogue.p_app_amount_unituses the stricterAppAmountUnit | undefined. For type consistency and better IntelliSense, consider using the same type:- p_app_amount_unit?: string | null + p_app_amount_unit?: AppAmountUnit | nullThis would provide better type safety when consuming the API response.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/fertilizer.d.ts` around lines 75 - 76, Change the FertilizerApplication.p_app_amount_unit type from string | null to the AppAmountUnit union (e.g., AppAmountUnit | null) to match the stricter typed usage; update FertilizerCatalogue.p_app_amount_unit if necessary so both use the same AppAmountUnit nullable/optional shape for consistent type safety and IntelliSense.
🤖 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-app/app/components/blocks/fertilizer-applications/form.tsx`:
- Around line 181-192: The effect useEffect that rehydrates p_app_amount_display
is currently triggered by p_id which causes overwriting when switching
fertilizers and clears the field when p_app_amount_display is null; remove p_id
from the dependency array so the effect only runs when
fertilizerApplication?.p_app_amount_display changes, and keep the guard
(p_app_amount_display !== null && typeof p_app_amount_display !== "undefined")
so you never set a null value (allowing the form to retain the kg/ha fallback or
user input). Update the dependency list to
[fertilizerApplication?.p_app_amount_display, form.setValue] and leave the body
unchanged.
- Around line 112-120: The conversionFactors object in the fertilizer
application form (conversionFactors) incorrectly assumes a density of 1 kg/L and
re-scales p_app_amount_display without considering actual fertilizer density,
causing wrong saved values and placeholders; instead, stop re-mutating
p_app_amount_display using those hardcoded factors and use the canonical
conversion utilities (e.g., toKgPerHa from fdm-core/src/fertilizer.ts) together
with the selected fertilizer's density when converting between units; update the
fertilizer-change handler and any UI placeholder computation (places referencing
conversionFactors at lines around the fertilizer change and example amount
logic) to call the proper conversion routine that accepts density or to convert
to an internal kg/ha baseline via toKgPerHa and then convert to the target
display unit using density-aware functions.
In `@fdm-app/app/components/blocks/fertilizer-applications/list.tsx`:
- Around line 109-112: The code renders application.p_app_amount_display
directly which can be null and will display "null"; update the JSX in the list
item to use a fallback: when application.p_app_amount_display is null or
undefined, render application.p_app_amount plus the unit "kg/ha" (or combine
with application.p_app_amount_unit when available), e.g. replace the direct
usage of application.p_app_amount_display with a null-coalescing/conditional
expression that falls back to application.p_app_amount and a default unit to
avoid showing the literal "null".
In `@fdm-core/src/fertilizer.ts`:
- Around line 791-797: The p_app_amount conversion calls toKgPerHa using
p_app_amount_display and fertilizer.p_app_amount_unit but omits
fertilizer.p_density, causing errors for volume units; update both write-path
calls that compute p_app_amount (the block using p_app_amount_display and the
other similar block around lines 852-858) to pass fertilizer.p_density as the
third argument to toKgPerHa so volume-based units like "l/ha" and "m3/ha" can
convert successfully.
- Around line 929-935: The p_app_amount_display mapping currently uses
fromKgPerHa(... )?.toNumber() which collapses a missing conversion sentinel to
undefined; change it so when conversion fails it returns null instead of
undefined (e.g., call fromKgPerHa(p_app_amount, p_app_amount_unit, p_density),
capture the result, and return result?.toNumber() ?? null or explicitly return
null when the conversion result is falsy), keeping the initial guard on
app.p_app_amount (app.p_app_amount !== null && app.p_app_amount !== undefined)
and referencing p_app_amount_display, fromKgPerHa, p_app_amount_unit, p_density,
and app.p_app_amount so the UI receives null when conversion is impossible.
In `@fdm-core/src/unit-conversion.test.ts`:
- Around line 123-127: The test in the "should accept input of type Decimal"
block calls the wrong helper (toKgPerHa) while sitting under the fromKgPerHa
suite; replace the toKgPerHa call with fromKgPerHa, pass the Decimal value that
corresponds to the kg/ha input used in the original toKgPerHa test (so the
inverse value is tested), and update the expectation to the original
concentration numeric (e.g., expect the returned Decimal toNumber() to equal
20). Ensure you reference the Decimal type and the fromKgPerHa function in the
adjusted assertion.
---
Outside diff comments:
In `@fdm-app/app/components/blocks/fertilizer-applications/types.d.tsx`:
- Around line 4-20: Update the FertilizerApplication type to match the new
read-model: add the display and unit fields used by the UI — include
p_app_amount_display: string | null and p_app_amount_unit: AppAmountUnit | null
on the FertilizerApplication interface (keeping existing p_app_amount and
p_app_method as-is) so columns/list components that read p_app_amount_display
and p_app_amount_unit are correctly typed and the null display case when density
is missing is represented.
In `@fdm-app/app/components/blocks/gerrit/types.ts`:
- Around line 11-17: The ParsedPlanApplication DTO is missing the application
unit; add a new field p_app_amount_unit to the ParsedPlanApplication interface
alongside p_app_amount_display, and update any consumers (notably the Gerrit
table rendering logic that currently reads app.p_app_amount and hardcodes
"kg/ha") to use app.p_app_amount_display for the numeric/value and
app.p_app_amount_unit for the unit instead of assuming kg/ha; ensure downstream
code expects p_app_amount_unit as string | null if units can be absent.
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.field.$b_id.fertilizer._index.tsx:
- Around line 332-381: The PUT branch still reads and passes p_app_amount
instead of the display-aware p_app_amount_display, causing wrong quantities to
be stored; update the FormSchemaModify extraction to destructure
p_app_amount_display (matching the POST branch) and pass p_app_amount_display
into updateFertilizerApplication (keep parameter order consistent with
addFertilizerApplication), and ensure any validation/error messages reference
p_app_amount_display where applicable.
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.field.fertilizer._index.tsx:
- Around line 217-280: The prefill logic uses raw p_app_amount (kg/ha) instead
of the user-facing value; change the selection and comparison to prefer
p_app_amount_display with fallback to p_app_amount: when building
exampleFertilizerApplication and when constructing fertilizerApplication from
keys (and inside the number branch that compares values) read value =
app.p_app_amount_display ?? app.p_app_amount and example =
exampleFertilizerApplication.p_app_amount_display ??
exampleFertilizerApplication.p_app_amount (treat null/undefined the same), then
perform the numeric equality/difference checks on those derived values; keep all
other key handling (keyTypes, date branch, deletion logic) the same and ensure
keys/p_app_amount are deleted only when both display and raw are missing.
- Around line 622-650: The bulk-update branch is still passing raw/mixed-unit
p_app_amount values into updateFertilizerApplication (using
validatedData.p_app_amount ?? original.p_app_amount), but
updateFertilizerApplication now expects a display-unit amount; convert both the
incoming validatedData.p_app_amount and fallback original.p_app_amount to the
display-unit representation before calling updateFertilizerApplication. Locate
the call site that builds the arguments for updateFertilizerApplication (the
p_app_amount parameter) and replace the direct value with the result of the same
conversion utility used elsewhere in the codebase (e.g.,
amountToDisplayUnit/convertToDisplayAmount or the project’s unit conversion
helper), applying it to validatedData.p_app_amount when present and to
original.p_app_amount for the fallback so mixed-unit edits are normalized to
display units.
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.gerrit.tsx:
- Around line 262-275: syntheticApps is only setting p_app_amount_display and
then casting to FertilizerApplication, causing calculators (calculateDose, norm
filling, OM/N balance) to receive display units; update the syntheticApps
creation to convert each app.p_app_amount_display into the canonical kg/ha value
and set p_app_amount (using the same conversion logic fdm-core uses or by
calling the fdm-core conversion API) before casting, ensuring density-dependent
units (l/ha, m3/ha, ton/ha) are converted and any conversion failures are
surfaced; apply the same fix to the other synthetic preview block that builds
FertilizerApplication-like objects so all previews use p_app_amount in kg/ha.
---
Nitpick comments:
In `@fdm-core/src/fertilizer.d.ts`:
- Around line 75-76: Change the FertilizerApplication.p_app_amount_unit type
from string | null to the AppAmountUnit union (e.g., AppAmountUnit | null) to
match the stricter typed usage; update FertilizerCatalogue.p_app_amount_unit if
necessary so both use the same AppAmountUnit nullable/optional shape for
consistent type safety and IntelliSense.
In `@fdm-core/src/fertilizer.test.ts`:
- Around line 1378-1405: The test using getFertilizerParametersDescription
should not only assert length; update the test to explicitly find the parameter
with parameter === "p_app_amount_unit" and assert it exists, has type "enum",
and that its options array contains exactly the four expected option values
(assert length and/or exact members) and that unit/name/description/category
properties exist; locate this validation within the same test block that
currently checks NL-nl/default locale and add assertions against the
p_app_amount_unit object to ensure the new metadata is present and correct.
- Around line 98-112: Update the test that calls addFertilizerToCatalogue in
fertilizer.test.ts to include at least one non-default unit by setting
p_app_amount_unit to a non-kg/ha value (e.g., "g/m2" or "lbs/acre") instead of
undefined, then assert on the returned result from addFertilizerToCatalogue that
the p_app_amount_unit equals the non-default unit and that the returned
fertilizer/application contains the expected converted display amount (check the
returned object's display/amount field produced by the addFertilizerToCatalogue
call). Ensure you modify the fixture values used in the test and add explicit
assertions for p_app_amount_unit and the converted display amount so the
non-default unit path is exercised.
🪄 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: f83ac9e0-3ba8-4da8-8185-59ba4dc7a0c6
📒 Files selected for processing (31)
.changeset/late-cycles-tan.md.changeset/ninety-sloths-notice.mdfdm-agents/src/agents/gerrit/agent.tsfdm-app/app/components/blocks/fertilizer-applications/columns.tsxfdm-app/app/components/blocks/fertilizer-applications/form.tsxfdm-app/app/components/blocks/fertilizer-applications/formschema.tsxfdm-app/app/components/blocks/fertilizer-applications/list.tsxfdm-app/app/components/blocks/fertilizer-applications/types.d.tsxfdm-app/app/components/blocks/fertilizer/formschema.tsxfdm-app/app/components/blocks/fertilizer/utils.tsfdm-app/app/components/blocks/gerrit/types.tsfdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.fertilizer._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.field.fertilizer._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.gerrit.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.rotation_.fertilizer._index.tsxfdm-calculator/src/doses/calculate-dose.test.tsfdm-calculator/src/doses/get-dose-field.test.tsfdm-calculator/src/norms/nl/2026/filling/stikstofgebruiksnorm.test.tsfdm-core/src/catalogues.tsfdm-core/src/cultivation.test.tsfdm-core/src/db/migrations/0026_p_app_amount_unit.sqlfdm-core/src/db/migrations/meta/0026_snapshot.jsonfdm-core/src/db/migrations/meta/_journal.jsonfdm-core/src/db/schema.tsfdm-core/src/fertilizer.d.tsfdm-core/src/fertilizer.test.tsfdm-core/src/fertilizer.tsfdm-core/src/index.tsfdm-core/src/unit-conversion.test.tsfdm-core/src/unit-conversion.tsfdm-data/src/fertilizers/d.ts
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fdm-app/app/components/blocks/pdf/bemestingsplan/BemestingsplanPDF.tsx (1)
883-885:⚠️ Potential issue | 🟡 MinorRename the totals header now that the values are no longer always kilograms.
Line 884 still says
Totaal (kg), but the cell now renderston,L, andm³as well. A neutral label likeTotaalavoids misleading units.Also applies to: 1007-1012
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/pdf/bemestingsplan/BemestingsplanPDF.tsx` around lines 883 - 885, Rename the totals header labels in the BemestingsplanPDF component so they don't hardcode "(kg)"; locate the PdfTableCell usage rendering "Totaal (kg)" (e.g., the PdfTableCell/Text at or near the PdfTableCell in BemestingsplanPDF) and change the displayed string to a neutral "Totaal" (also update the other instance around the block that covers lines ~1007-1012) so the header no longer implies kilograms when values may be ton, L, or m³.
♻️ Duplicate comments (1)
fdm-app/app/components/blocks/fertilizer-applications/form.tsx (1)
41-46:⚠️ Potential issue | 🟠 MajorRemove the hardcoded unit-factor conversion.
This is still the same unresolved correctness issue from the earlier review: the form only knows the unit, then rescales with a fixed factor table that assumes
1 kg/L. That makes fertilizer switches and even the placeholder wrong for liquids. Convert through canonicalkg/hawith the selected fertilizer's real density, or don't mutate the amount client-side at all.Also applies to: 112-120, 194-216, 346-357
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/fertilizer-applications/form.tsx` around lines 41 - 46, The form currently applies a hardcoded unit-factor assumption (effectively 1 kg/L) when converting amounts — update the code that rescales fertilizer amounts (where FertilizerOption.p_app_amount_unit is used and where the unit-factor table/mapping is referenced) to either (a) perform conversions via a canonical intermediate of kg/ha using the selected fertilizer's actual density value from the FertilizerOption (use density to convert L ↔ kg, then convert to kg/ha based on area units), or (b) stop mutating the amount client-side and keep the raw value/selected unit until server-side/central conversion; remove any fixed 1:1 factor table and replace with density-driven conversion logic or the no-mutation approach in all locations that currently rescale (the spots corresponding to the earlier comments where unit conversion occurs).
🧹 Nitpick comments (1)
fdm-core/src/catalogues.test.ts (1)
750-773: Add one case for the actualp_type_rvo-derived default.The new assertions cover the DB default (
no p_type_rvo) and the explicit override, but they still don't exercise the new branch inextendCatalogueFertilizer()wherep_type_rvois present andp_app_amount_unitis omitted. A small test for that path would pin thesuggestUnitFromRvoCode()integration this PR adds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/catalogues.test.ts` around lines 750 - 773, Add a test in catalogues.test.ts that exercises the branch where p_type_rvo is provided but p_app_amount_unit is omitted: call syncFertilizerCatalogueArray with an item that includes p_type_rvo (use a code that suggestUnitFromRvoCode maps to a known unit) but do not set p_app_amount_unit, then read back the record via fdm.select(...).from(schema.fertilizersCatalogue) and assert the stored p_app_amount_unit equals the expected unit; this ensures extendCatalogueFertilizer() uses suggestUnitFromRvoCode() to populate the default unit when p_app_amount_unit is missing.
🤖 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-app/app/components/blocks/fertilizer-applications/form.tsx`:
- Around line 198-216: The current useState of lastConversionFactor and the
useEffect compare factors which can be stale when the amount field is empty;
change the state to track the previous fertilizer identifier or unit (e.g.
previousPId or previousUnit) instead of lastConversionFactor, update the
useEffect to compare previousPId/previousUnit with the current p_id or unit (and
include that value in the dependency array), and always update the
previousPId/previousUnit state even when p_app_amount_display is undefined so
future conversions are applied correctly; update references from
lastConversionFactor to the new previous identifier and keep the conversion math
using currentConversionFactor and the previously stored conversionFactor derived
from the previous identifier.
In `@fdm-app/app/components/blocks/pdf/bemestingsplan/BemestingsplanPDF.tsx`:
- Around line 908-916: The detailed "Geplande bemestingen" rows still render
app.quantity with a hardcoded "kg/ha" label; update those rows in
BemestingsplanPDF (where you render app.quantity and app.quantity_unit) to reuse
the same unit-aware logic used for the totals: compute amount_display from
app.quantity (applying the same unit conversions used in the totals block) and
look up amount_unit from the mapping for app.quantity_unit (the mapping that
returns "kg", "ton", "L", "m³"); then render "{amount_display} {amount_unit}"
instead of the fixed "kg/ha" string—apply this change to all occurrences noted
(the detailed rows around the Geplande bemestingen table and the other
occurrences you pointed out).
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.bemestingsplan[.]pdf.tsx:
- Around line 284-286: The current fallback turns a null
app.p_app_amount_display into 0 while keeping app.p_app_amount_unit, causing
density units like "l/ha" to display zero; update the mapping so that when
app.p_app_amount_display is null/undefined you set quantity_display to
app.p_app_amount and quantity_unit to the canonical "kg/ha" (and keep quantity =
app.p_app_amount ?? 0), i.e. use app.p_app_amount_display if present otherwise
fall back to the stored app.p_app_amount with its unit forced to "kg/ha"
(referencing quantity, quantity_display, quantity_unit, app.p_app_amount,
app.p_app_amount_display, app.p_app_amount_unit).
In `@fdm-core/src/catalogues.test.ts`:
- Around line 593-605: The afterEach currently deletes any row matching
"test_%"—narrow this to only IDs created by this test suite by adding a local
array (e.g. createdCatalogueIds) and pushing every id returned by
createTestCatalogueId (or createId wrapper) into it; change
createTestCatalogueId to push the new id to createdCatalogueIds before returning
it; then update the afterEach to only delete rows whose keys are IN that array
using fdm.delete(...).where(in(schema.fertilizersCatalogue.p_id_catalogue,
createdCatalogueIds)) and similarly for
schema.cultivationsCatalogue.b_lu_catalogue, guarding against empty arrays (skip
delete if none).
In `@fdm-core/src/fertilizer.types.d.ts`:
- Line 12: The types are too permissive: make p_app_amount_unit non-nullable and
require p_app_amount_display in the fertilizer types to match runtime behavior;
update the catalogue/interface declaration(s) that currently have
p_app_amount_unit: AppAmountUnit | null to p_app_amount_unit: AppAmountUnit, and
change any optional p_app_amount_display to a required property typed as number
| null (also update the return shape of extendFertilizerApplication to always
include p_app_amount_unit: AppAmountUnit and p_app_amount_display: number |
null); apply the same change where p_app_amount_unit/p_app_amount_display appear
(including the other occurrences noted around lines 75-76) so public types match
the runtime contract.
In `@fdm-core/src/unit-conversion.ts`:
- Around line 29-39: The density check currently uses if (!density) which misses
Decimal(0) and allows zero/negative densities; update the guards in the
functions handling l/ha → kg/ha and m3/ha → kg/ha (e.g., in
toKgPerHa/fromKgPerHa or the switch cases that reference density) to explicitly
reject null/undefined/zero/negative by using a condition like density == null ||
new Decimal(density).lte(0); apply this replacement for both occurrences shown
(and the similar checks around lines 62-66) so the code throws or returns the
intended error/null for non-positive densities.
---
Outside diff comments:
In `@fdm-app/app/components/blocks/pdf/bemestingsplan/BemestingsplanPDF.tsx`:
- Around line 883-885: Rename the totals header labels in the BemestingsplanPDF
component so they don't hardcode "(kg)"; locate the PdfTableCell usage rendering
"Totaal (kg)" (e.g., the PdfTableCell/Text at or near the PdfTableCell in
BemestingsplanPDF) and change the displayed string to a neutral "Totaal" (also
update the other instance around the block that covers lines ~1007-1012) so the
header no longer implies kilograms when values may be ton, L, or m³.
---
Duplicate comments:
In `@fdm-app/app/components/blocks/fertilizer-applications/form.tsx`:
- Around line 41-46: The form currently applies a hardcoded unit-factor
assumption (effectively 1 kg/L) when converting amounts — update the code that
rescales fertilizer amounts (where FertilizerOption.p_app_amount_unit is used
and where the unit-factor table/mapping is referenced) to either (a) perform
conversions via a canonical intermediate of kg/ha using the selected
fertilizer's actual density value from the FertilizerOption (use density to
convert L ↔ kg, then convert to kg/ha based on area units), or (b) stop mutating
the amount client-side and keep the raw value/selected unit until
server-side/central conversion; remove any fixed 1:1 factor table and replace
with density-driven conversion logic or the no-mutation approach in all
locations that currently rescale (the spots corresponding to the earlier
comments where unit conversion occurs).
---
Nitpick comments:
In `@fdm-core/src/catalogues.test.ts`:
- Around line 750-773: Add a test in catalogues.test.ts that exercises the
branch where p_type_rvo is provided but p_app_amount_unit is omitted: call
syncFertilizerCatalogueArray with an item that includes p_type_rvo (use a code
that suggestUnitFromRvoCode maps to a known unit) but do not set
p_app_amount_unit, then read back the record via
fdm.select(...).from(schema.fertilizersCatalogue) and assert the stored
p_app_amount_unit equals the expected unit; this ensures
extendCatalogueFertilizer() uses suggestUnitFromRvoCode() to populate the
default unit when p_app_amount_unit is missing.
🪄 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: d5df1322-8d35-4956-8788-9d5afbe11a2d
📒 Files selected for processing (15)
fdm-app/app/components/blocks/fertilizer-applications/form.tsxfdm-app/app/components/blocks/fertilizer-applications/list.tsxfdm-app/app/components/blocks/pdf/bemestingsplan/BemestingsplanPDF.tsxfdm-app/app/components/blocks/pdf/bemestingsplan/types.d.tsfdm-app/app/routes/farm.$b_id_farm.$calendar.bemestingsplan[.]pdf.tsxfdm-core/src/catalogues.test.tsfdm-core/src/catalogues.tsfdm-core/src/cultivation.test.tsfdm-core/src/db/schema.tsfdm-core/src/fertilizer.test.tsfdm-core/src/fertilizer.tsfdm-core/src/fertilizer.types.d.tsfdm-core/src/index.tsfdm-core/src/unit-conversion.test.tsfdm-core/src/unit-conversion.ts
✅ Files skipped from review due to trivial changes (4)
- fdm-core/src/index.ts
- fdm-app/app/components/blocks/fertilizer-applications/list.tsx
- fdm-core/src/cultivation.test.ts
- fdm-core/src/unit-conversion.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- fdm-core/src/db/schema.ts
- fdm-core/src/catalogues.ts
- fdm-core/src/fertilizer.test.ts
- fdm-core/src/fertilizer.ts
Comment on lines
+284
to
+286
| quantity: app.p_app_amount ?? 0, | ||
| quantity_display: app.p_app_amount_display ?? 0, | ||
| quantity_unit: app.p_app_amount_unit ?? "kg/ha", |
Contributor
There was a problem hiding this comment.
Fall back to canonical kg/ha as a pair when display conversion is unavailable.
Lines 285-286 currently turn p_app_amount_display: null into 0 while still exposing the preferred unit. For density-dependent fertilizers, that will make the PDF show 0 l/ha / 0 m³/ha and zero totals instead of falling back to the stored p_app_amount in kg/ha, which is the required behavior.
Suggested fix
- quantity: app.p_app_amount ?? 0,
- quantity_display: app.p_app_amount_display ?? 0,
- quantity_unit: app.p_app_amount_unit ?? "kg/ha",
+ quantity: app.p_app_amount,
+ quantity_display:
+ app.p_app_amount_display ?? app.p_app_amount,
+ quantity_unit:
+ app.p_app_amount_display != null
+ ? app.p_app_amount_unit
+ : "kg/ha",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| quantity: app.p_app_amount ?? 0, | |
| quantity_display: app.p_app_amount_display ?? 0, | |
| quantity_unit: app.p_app_amount_unit ?? "kg/ha", | |
| quantity: app.p_app_amount, | |
| quantity_display: | |
| app.p_app_amount_display ?? app.p_app_amount, | |
| quantity_unit: | |
| app.p_app_amount_display != null | |
| ? app.p_app_amount_unit | |
| : "kg/ha", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.bemestingsplan[.]pdf.tsx around
lines 284 - 286, The current fallback turns a null app.p_app_amount_display into
0 while keeping app.p_app_amount_unit, causing density units like "l/ha" to
display zero; update the mapping so that when app.p_app_amount_display is
null/undefined you set quantity_display to app.p_app_amount and quantity_unit to
the canonical "kg/ha" (and keep quantity = app.p_app_amount ?? 0), i.e. use
app.p_app_amount_display if present otherwise fall back to the stored
app.p_app_amount with its unit forced to "kg/ha" (referencing quantity,
quantity_display, quantity_unit, app.p_app_amount, app.p_app_amount_display,
app.p_app_amount_unit).
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
fdm-core/src/unit-conversion.ts (1)
28-39:⚠️ Potential issue | 🟠 MajorReject non-positive
densityvalues in both conversion directions.Line 29/35 still accepts
0and negative densities, and Lines 62-65 still missnew Decimal(0)becauseif (!density)only filters primitive falsy values. That breaks the documented contract here: volume conversions can quietly return0/Infinityinstead of throwing or returningnull.Suggested fix
export function toKgPerHa( value: number | Decimal | string, unit: AppAmountUnit, density?: number | Decimal | null, // kg/l ): Decimal { const d = new Decimal(value) + const densityDecimal = + density === null || density === undefined ? null : new Decimal(density) switch (unit) { case "kg/ha": return new Decimal(d) case "ton/ha": return new Decimal(1000).times(d) case "l/ha": - if (density === null || density === undefined) + if (densityDecimal === null || densityDecimal.lte(0)) throw new Error( "Density (p_density) is required for l/ha → kg/ha conversion", ) - return new Decimal(density).times(d) + return densityDecimal.times(d) case "m3/ha": - if (density === null || density === undefined) + if (densityDecimal === null || densityDecimal.lte(0)) throw new Error( "Density (p_density) is required for m3/ha → kg/ha conversion", ) - return new Decimal(1000).times(d).times(density) + return new Decimal(1000).times(d).times(densityDecimal) default: throw new Error(`${unit} → kg/ha conversion is not supported`) } } @@ export function fromKgPerHa( valueKgPerHa: number | Decimal | string, unit: AppAmountUnit, density?: number | Decimal | null, // kg/l ): Decimal | null { const d = new Decimal(valueKgPerHa) + const densityDecimal = + density === null || density === undefined ? null : new Decimal(density) switch (unit) { case "kg/ha": return d case "ton/ha": return d.dividedBy(1000) case "l/ha": - if (!density) return null - return d.dividedBy(new Decimal(density)) + if (densityDecimal === null || densityDecimal.lte(0)) return null + return d.dividedBy(densityDecimal) case "m3/ha": - if (!density) return null - return d.dividedBy(new Decimal(density).times(1000)) + if (densityDecimal === null || densityDecimal.lte(0)) return null + return d.dividedBy(densityDecimal.times(1000)) default: return null } }Also applies to: 61-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/unit-conversion.ts` around lines 28 - 39, The l/ha and m3/ha conversion branches currently only guard against null/undefined (and elsewhere use if (!density)) and thus allow zero or negative densities; update both checks to reject non-positive densities by replacing existing guards (in the "l/ha" and "m3/ha" branches and the other spot using if (!density)) with a validation that density is not null/undefined and that new Decimal(density).lte(new Decimal(0)) triggers an Error (e.g., throw new Error("Density (p_density) must be greater than 0 for ... conversion")); reference the density variable and the "l/ha" / "m3/ha" conversion cases and the other density check to locate and change all occurrences.
🧹 Nitpick comments (1)
fdm-calculator/src/balance/nitrogen/types.d.ts (1)
499-502: Prefer the shared application type here to avoid contract drift.Using a local
Omit<...>duplicates the fertilizer-application contract and can diverge from other calculator modules. ReuseCalculatorFertilizerApplication[]here for consistency with doses/organic-matter typing.♻️ Proposed refactor
import type { Cultivation, CultivationCatalogue, Fertilizer, - FertilizerApplication, Field, Harvest, SoilAnalysis, } from "@nmi-agro/fdm-core" import type { Decimal } from "decimal.js" +import type { CalculatorFertilizerApplication } from "../../shared/types" @@ - fertilizerApplications: Omit< - FertilizerApplication, - "p_app_amount_display" | "p_app_amount_unit" - >[] + fertilizerApplications: CalculatorFertilizerApplication[]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/balance/nitrogen/types.d.ts` around lines 499 - 502, The property fertilizerApplications currently uses a local Omit<FertilizerApplication, "p_app_amount_display" | "p_app_amount_unit">[] which duplicates the fertilizer-application contract; replace that type with the shared CalculatorFertilizerApplication[] so this file reuses the canonical application type (aligning with doses/organic-matter types) and avoids contract drift—update the fertilizerApplications declaration to use CalculatorFertilizerApplication[] and remove the local Omit usage.
🤖 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-app/app/routes/farm`.$b_id_farm.$calendar.gerrit.tsx:
- Around line 715-728: The code is rounding the converted display amount too
early: avoid calling toDecimalPlaces(2).toNumber() on p_app_amount_display when
building unitConvertedAmount so full precision of the converted p_app_amount
(derived from p_app_amount, p_app_amount_unit, p_density) is preserved; only
round when rendering the UI, but pass the unrounded numeric value into
addFertilizerApplication and any persisted p_app_amount fields so
storage/acceptance uses full precision (update the code paths that construct
unitConvertedAmount and the call sites around addFertilizerApplication to use
the unrounded p_app_amount_display value).
- Around line 722-736: The serialized fallback sets p_app_amount_unit to "kg/ha"
when fromKgPerHa() returned null but the accept path later calls
addFertilizerApplication() with only the numeric value, causing incorrect
re-conversion; fix by detecting and preventing stale-unit saves or persisting a
canonical amount: in the accept/save logic (the code that calls
addFertilizerApplication()), check if app.p_app_amount_unit !==
fertilizer.p_app_amount_unit and reject/save a validation error, or compute and
persist the canonical p_app_amount in the fertilizer's preferred unit before
calling addFertilizerApplication() (use fromKgPerHa()/toKgPerHa conversion
helpers or extend addFertilizerApplication() to accept an explicit
unit/canonical value so core is not forced to re-interpret the fallback).
---
Duplicate comments:
In `@fdm-core/src/unit-conversion.ts`:
- Around line 28-39: The l/ha and m3/ha conversion branches currently only guard
against null/undefined (and elsewhere use if (!density)) and thus allow zero or
negative densities; update both checks to reject non-positive densities by
replacing existing guards (in the "l/ha" and "m3/ha" branches and the other spot
using if (!density)) with a validation that density is not null/undefined and
that new Decimal(density).lte(new Decimal(0)) triggers an Error (e.g., throw new
Error("Density (p_density) must be greater than 0 for ... conversion"));
reference the density variable and the "l/ha" / "m3/ha" conversion cases and the
other density check to locate and change all occurrences.
---
Nitpick comments:
In `@fdm-calculator/src/balance/nitrogen/types.d.ts`:
- Around line 499-502: The property fertilizerApplications currently uses a
local Omit<FertilizerApplication, "p_app_amount_display" |
"p_app_amount_unit">[] which duplicates the fertilizer-application contract;
replace that type with the shared CalculatorFertilizerApplication[] so this file
reuses the canonical application type (aligning with doses/organic-matter types)
and avoids contract drift—update the fertilizerApplications declaration to use
CalculatorFertilizerApplication[] and remove the local Omit usage.
🪄 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: 7cf27954-66c7-4c82-89a9-491e794fcdda
📒 Files selected for processing (26)
fdm-app/app/components/blocks/fertilizer-applications/form.tsxfdm-app/app/components/blocks/fertilizer-applications/utils.tsfdm-app/app/components/blocks/fertilizer/form.tsxfdm-app/app/components/blocks/fertilizer/formschema.tsxfdm-app/app/components/blocks/gerrit/plan-table.tsxfdm-app/app/components/blocks/gerrit/types.tsfdm-app/app/components/blocks/pdf/bemestingsplan/BemestingsplanPDF.tsxfdm-app/app/components/blocks/pdf/bemestingsplan/types.d.tsfdm-app/app/routes/farm.$b_id_farm.$calendar.bemestingsplan[.]pdf.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.fertilizer._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.field.fertilizer._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.gerrit.tsxfdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.tsfdm-calculator/src/balance/nitrogen/types.d.tsfdm-calculator/src/balance/organic-matter/types.tsfdm-calculator/src/doses/calculate-dose.test.tsfdm-calculator/src/doses/calculate-dose.tsfdm-calculator/src/norms/nl/2025/filling/fosfaatgebruiksnorm.test.tsfdm-calculator/src/norms/nl/2026/filling/fosfaatgebruiksnorm.test.tsfdm-calculator/src/norms/nl/2026/filling/stikstofgebruiksnorm.test.tsfdm-calculator/src/shared/types.d.tsfdm-core/src/catalogues.test.tsfdm-core/src/fertilizer.tsfdm-core/src/fertilizer.types.d.tsfdm-core/src/index.tsfdm-core/src/unit-conversion.ts
✅ Files skipped from review due to trivial changes (1)
- fdm-calculator/src/doses/calculate-dose.test.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.fertilizer._index.tsx
- fdm-app/app/routes/farm.$b_id_farm.$calendar.bemestingsplan[.]pdf.tsx
- fdm-calculator/src/norms/nl/2026/filling/stikstofgebruiksnorm.test.ts
- fdm-app/app/routes/farm.$b_id_farm.$calendar.field.fertilizer._index.tsx
- fdm-app/app/components/blocks/pdf/bemestingsplan/types.d.ts
- fdm-app/app/components/blocks/fertilizer/formschema.tsx
- fdm-app/app/components/blocks/fertilizer-applications/form.tsx
- fdm-core/src/catalogues.test.ts
- fdm-core/src/index.ts
- fdm-app/app/components/blocks/pdf/bemestingsplan/BemestingsplanPDF.tsx
- fdm-app/app/components/blocks/gerrit/types.ts
- fdm-core/src/fertilizer.ts
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-app/app/routes/farm`.$b_id_farm.$calendar.gerrit.tsx:
- Around line 715-731: The current unitConvertedAmount logic drops the numeric
amount when fromKgPerHa(...) returns null (keeping fert.p_app_amount_unit but
setting p_app_amount_display to null), causing downstream rendering to show 0 in
the preferred unit; update the ternary that builds unitConvertedAmount so that
when fert is present but p_app_amount_display is null you fall back to using
app.p_app_amount with unit "kg/ha" (i.e. ensure p_app_amount_display becomes
app.p_app_amount and p_app_amount_unit becomes "kg/ha" instead of leaving
display null), referencing p_app_amount_display, fromKgPerHa,
unitConvertedAmount, fert.p_app_amount_unit and app.p_app_amount to locate where
to change the logic.
In `@fdm-core/src/unit-conversion.ts`:
- Around line 247-260: Add an explicit mapping for RVO code "112" in the
RVO_RECOMMENDED_UNITS array so suggestUnitFromRvoCode() no longer falls back to
"kg/ha" for that code; specifically add an entry with p_type_rvo: "112" and the
correct unit (same as compost entries, e.g., "ton/ha") and an appropriate type
label (e.g., "zeer schone compost" or "compost") so the default unit selection
matches other compost codes like "111" and aligns with the 25% discount group
handling.
🪄 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: 7c68ab4b-f34c-400e-9cdd-c48c1952f377
📒 Files selected for processing (20)
fdm-app/app/routes/farm.$b_id_farm.$calendar.gerrit.tsxfdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.tsfdm-calculator/src/balance/nitrogen/types.d.tsfdm-calculator/src/balance/organic-matter/types.tsfdm-calculator/src/doses/calculate-dose.test.tsfdm-calculator/src/doses/calculate-dose.tsfdm-calculator/src/norms/nl/2025/filling/fosfaatgebruiksnorm.test.tsfdm-calculator/src/norms/nl/2025/filling/fosfaatgebruiksnorm.tsfdm-calculator/src/norms/nl/2025/filling/input.test.tsfdm-calculator/src/norms/nl/2025/filling/types.d.tsfdm-calculator/src/norms/nl/2026/filling/fosfaatgebruiksnorm.test.tsfdm-calculator/src/norms/nl/2026/filling/fosfaatgebruiksnorm.tsfdm-calculator/src/norms/nl/2026/filling/input.test.tsfdm-calculator/src/norms/nl/2026/filling/stikstofgebruiksnorm.test.tsfdm-calculator/src/norms/nl/2026/filling/types.d.tsfdm-core/src/fertilizer.tsfdm-core/src/fertilizer.types.d.tsfdm-core/src/index.tsfdm-core/src/unit-conversion.test.tsfdm-core/src/unit-conversion.ts
✅ Files skipped from review due to trivial changes (10)
- fdm-calculator/src/balance/nitrogen/emission/ammonia/fertilizers.ts
- fdm-calculator/src/doses/calculate-dose.test.ts
- fdm-calculator/src/norms/nl/2025/filling/fosfaatgebruiksnorm.test.ts
- fdm-calculator/src/norms/nl/2026/filling/fosfaatgebruiksnorm.test.ts
- fdm-calculator/src/norms/nl/2026/filling/types.d.ts
- fdm-calculator/src/norms/nl/2025/filling/input.test.ts
- fdm-calculator/src/norms/nl/2026/filling/stikstofgebruiksnorm.test.ts
- fdm-core/src/unit-conversion.test.ts
- fdm-calculator/src/norms/nl/2025/filling/fosfaatgebruiksnorm.ts
- fdm-core/src/fertilizer.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- fdm-calculator/src/doses/calculate-dose.ts
- fdm-calculator/src/balance/organic-matter/types.ts
- fdm-calculator/src/balance/nitrogen/types.d.ts
- fdm-core/src/index.ts
Collaborator
Author
|
Maybe the farm fertilizer table should show warning for fertilizers that have a volume per area application unit and no density specified, so users can quickly fix the fertilizers that they configured wrongly. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
fdm-core/src/unit-conversion.test.ts (1)
32-51: Consider adding throw-cases for non-positive density intoKgPerHa.
fromKgPerHaalready assertsnullfordensity: 0anddensity: -1, but the symmetric guards intoKgPerHa(which also reject zero/negative viaDecimal(0).greaterThanOrEqualTo(density)) aren't exercised — a regression that silently accepteddensity <= 0wouldn't fail any test here.Suggested additions to
throwingTestsconst throwingTests: ConversionUnitTestCase[] = [ { input: 20, unit: "l/ha", density: undefined, throws: "Positive density (p_density) is required for l/ha → kg/ha conversion", }, { input: 20, unit: "m3/ha", density: undefined, throws: "Positive density (p_density) is required for m3/ha → kg/ha conversion", }, + { + input: 20, + unit: "l/ha", + density: 0, + throws: "Positive density (p_density) is required for l/ha → kg/ha conversion", + }, + { + input: 20, + unit: "m3/ha", + density: -1, + throws: "Positive density (p_density) is required for m3/ha → kg/ha conversion", + }, { input: 20, unit: "ft3/ha" as AppAmountUnit, density: 2, throws: "ft3/ha → kg/ha conversion is not supported", }, ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/unit-conversion.test.ts` around lines 32 - 51, The test suite's throwingTests array is missing cases that assert toKgPerHa rejects non-positive densities; add ConversionUnitTestCase entries to throwingTests that call toKgPerHa (via the same test harness) with density: 0 and density: -1 for units that require density (e.g., "l/ha" and "m3/ha") and expect the same error message pattern as fromKgPerHa ("Positive density (p_density) is required for ... → kg/ha conversion"), ensuring the tests reference the existing throwingTests array and the toKgPerHa conversion paths so zero/negative density is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@fdm-core/src/unit-conversion.test.ts`:
- Around line 32-51: The test suite's throwingTests array is missing cases that
assert toKgPerHa rejects non-positive densities; add ConversionUnitTestCase
entries to throwingTests that call toKgPerHa (via the same test harness) with
density: 0 and density: -1 for units that require density (e.g., "l/ha" and
"m3/ha") and expect the same error message pattern as fromKgPerHa ("Positive
density (p_density) is required for ... → kg/ha conversion"), ensuring the tests
reference the existing throwingTests array and the toKgPerHa conversion paths so
zero/negative density is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 20646a7d-a59c-4553-a9a6-2a366949fdd7
📒 Files selected for processing (4)
fdm-app/app/routes/farm.$b_id_farm.$calendar.gerrit.tsxfdm-core/src/fertilizer.tsfdm-core/src/unit-conversion.test.tsfdm-core/src/unit-conversion.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- fdm-core/src/fertilizer.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
fdm-core/src/fertilizer-application-unit-conversion.ts (1)
210-210:⚠️ Potential issue | 🟡 MinorRVO code
120still contradicts the documented unit rule.Lines 91-94 classify liquid concentrates as
l/ha, but"120"("other - mineral concentrate") is still registered askg/ha, sosuggestUnitFromRvoCode("120")will seed the wrong default for this typically-liquid product.🔧 Proposed fix
- { p_type_rvo: "120", type: "other - mineral concentrate", unit: "kg/ha" }, + { p_type_rvo: "120", type: "other - mineral concentrate", unit: "l/ha" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/fertilizer-application-unit-conversion.ts` at line 210, The RVO code "120" is incorrectly seeded as "kg/ha" while the code is classified as a liquid concentrate and should default to "l/ha"; update the data entry where p_type_rvo === "120" (the record with type "other - mineral concentrate") to set unit: "l/ha" so suggestUnitFromRvoCode("120") returns the correct liquid unit, and run/update any unit-mapping tests that assume liquid concentrate codes use "l/ha".
🧹 Nitpick comments (1)
fdm-core/src/fertilizer-application-unit-conversion.ts (1)
22-27: Minor: drop redundantDecimalwrapping.
dis already aDecimal, sonew Decimal(d)on Line 25 is unnecessary. Same applies to Line 27 where.times(d)already returns a newDecimal.♻️ Proposed tidy-up
const d = new Decimal(value) switch (unit) { case "kg/ha": - return new Decimal(d) + return d case "ton/ha": - return new Decimal(1000).times(d) + return d.times(1000)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/fertilizer-application-unit-conversion.ts` around lines 22 - 27, The code creates d as a Decimal already, so remove redundant wrapping in the switch: return d directly in the "kg/ha" case and return Decimal(1000).times(d) (or Decimal(1000).times(d) without wrapping the result) in the "ton/ha" case; update the switch inside the fertilizer-application-unit-conversion module where d is defined to avoid new Decimal(d) and unnecessary new Decimal around the .times(d) result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@fdm-core/src/fertilizer-application-unit-conversion.ts`:
- Line 210: The RVO code "120" is incorrectly seeded as "kg/ha" while the code
is classified as a liquid concentrate and should default to "l/ha"; update the
data entry where p_type_rvo === "120" (the record with type "other - mineral
concentrate") to set unit: "l/ha" so suggestUnitFromRvoCode("120") returns the
correct liquid unit, and run/update any unit-mapping tests that assume liquid
concentrate codes use "l/ha".
---
Nitpick comments:
In `@fdm-core/src/fertilizer-application-unit-conversion.ts`:
- Around line 22-27: The code creates d as a Decimal already, so remove
redundant wrapping in the switch: return d directly in the "kg/ha" case and
return Decimal(1000).times(d) (or Decimal(1000).times(d) without wrapping the
result) in the "ton/ha" case; update the switch inside the
fertilizer-application-unit-conversion module where d is defined to avoid new
Decimal(d) and unnecessary new Decimal around the .times(d) result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 538763fd-4051-4f29-a90d-561f784b1a3e
📒 Files selected for processing (7)
fdm-core/src/catalogues.tsfdm-core/src/db/schema.tsfdm-core/src/fertilizer-application-unit-conversion.test.tsfdm-core/src/fertilizer-application-unit-conversion.tsfdm-core/src/fertilizer.tsfdm-core/src/fertilizer.types.d.tsfdm-core/src/index.ts
✅ Files skipped from review due to trivial changes (1)
- fdm-core/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- fdm-core/src/db/schema.ts
- fdm-core/src/catalogues.ts
- fdm-core/src/fertilizer.types.d.ts
- fdm-core/src/fertilizer.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fdm-core/src/fertilizer.ts (1)
166-172:⚠️ Potential issue | 🟠 Major
p_app_amount_unitis create-only at the API boundary.This adds the new field for
addFertilizerToCatalogue, butupdateFertilizerFromCatalogue()still cannot accept it. Any edit flow that exposes the new selector will silently keep the old unit, so the preferred display unit and its hash never update after edits.Suggested fix
export async function updateFertilizerFromCatalogue( fdm: FdmType, principal_id: PrincipalId, b_id_farm: schema.farmsTypeInsert["b_id_farm"], p_id_catalogue: schema.fertilizersCatalogueTypeInsert["p_id_catalogue"], properties: Partial<{ p_name_nl: schema.fertilizersCatalogueTypeInsert["p_name_nl"] p_name_en: schema.fertilizersCatalogueTypeInsert["p_name_en"] p_description: schema.fertilizersCatalogueTypeInsert["p_description"] p_app_method_options: schema.fertilizersCatalogueTypeInsert["p_app_method_options"] + p_app_amount_unit: schema.fertilizersCatalogueTypeInsert["p_app_amount_unit"] p_dm: schema.fertilizersCatalogueTypeInsert["p_dm"] p_density: schema.fertilizersCatalogueTypeInsert["p_density"] // ... }>, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-core/src/fertilizer.ts` around lines 166 - 172, The update flow ignores the new p_app_amount_unit field so edits don't change the stored unit; modify updateFertilizerFromCatalogue to accept and persist p_app_amount_unit (same type as schema.fertilizersCatalogueTypeInsert["p_app_amount_unit"]) by adding it to the list/mapping of updatable properties, ensure any validation/transformation used in addFertilizerToCatalogue (e.g., unit normalization/hash computation) is reused for updates, and update related types/validators if there's a distinct update schema so the API accepts p_app_amount_unit during update requests.
🤖 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-agents/src/tools/fertilizer-planner/index.ts`:
- Around line 376-387: p_app_amount_display is defined as a free-form string but
should be a numeric value to match the numeric display-value contract used
elsewhere; update the Zod schema for p_app_amount_display (the field in the
schema in index.ts near p_app_amount_unit) to be a number (.number().optional())
and update its description to indicate it is the numeric application amount
(unit is carried separately in p_app_amount_unit) so ADK/Zod validation matches
fdm-core expectations.
In `@fdm-core/src/fertilizer.ts`:
- Around line 791-800: The current call getFertilizer(fdm, p_id) only checks
existence; change it to enforce farm scoping by either using a farm-scoped
lookup or validating the returned fertilizer.farm_id against the target
application/field farm id (e.g., compare fertilizer.farm_id === field.farm_id or
targetFarmId) and throw/return an error if they differ; update the same pattern
used later (the block at 854-862) so p_id cannot reference inventory from
another farm.
---
Outside diff comments:
In `@fdm-core/src/fertilizer.ts`:
- Around line 166-172: The update flow ignores the new p_app_amount_unit field
so edits don't change the stored unit; modify updateFertilizerFromCatalogue to
accept and persist p_app_amount_unit (same type as
schema.fertilizersCatalogueTypeInsert["p_app_amount_unit"]) by adding it to the
list/mapping of updatable properties, ensure any validation/transformation used
in addFertilizerToCatalogue (e.g., unit normalization/hash computation) is
reused for updates, and update related types/validators if there's a distinct
update schema so the API accepts p_app_amount_unit during update requests.
🪄 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: 27682fbf-15d5-4ac0-a39f-c0dac6e9c62b
📒 Files selected for processing (16)
.changeset/fiery-candies-kneel.mdfdm-agents/src/agents/gerrit/agent.tsfdm-agents/src/tools/fertilizer-planner/index.tsfdm-app/app/components/blocks/fertilizer-applications/utils.tsfdm-app/app/components/blocks/fertilizer/formschema.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.gerrit.tsxfdm-core/src/catalogues.tsfdm-core/src/fertilizer-application-unit-conversion.test.tsfdm-core/src/fertilizer-application-unit-conversion.tsfdm-core/src/fertilizer.test.tsfdm-core/src/fertilizer.tsfdm-core/src/fertilizer.types.d.tsfdm-data/src/fertilizers/catalogues/baat.jsonfdm-data/src/fertilizers/catalogues/baat.tsfdm-data/src/fertilizers/catalogues/srm.tsfdm-data/src/fertilizers/d.ts
✅ Files skipped from review due to trivial changes (3)
- .changeset/fiery-candies-kneel.md
- fdm-data/src/fertilizers/d.ts
- fdm-data/src/fertilizers/catalogues/baat.json
🚧 Files skipped from review as they are similar to previous changes (6)
- fdm-app/app/components/blocks/fertilizer/formschema.tsx
- fdm-core/src/fertilizer.test.ts
- fdm-core/src/fertilizer.types.d.ts
- fdm-app/app/components/blocks/fertilizer-applications/utils.ts
- fdm-core/src/fertilizer-application-unit-conversion.ts
- fdm-core/src/fertilizer-application-unit-conversion.test.ts
Comment on lines
+791
to
+800
| // Validate that the fertilizer exists and get it | ||
| const fertilizer = await getFertilizer(fdm, p_id) | ||
|
|
||
| const p_app_id = createId() | ||
|
|
||
| const p_app_amount = toKgPerHa( | ||
| p_app_amount_display, | ||
| fertilizer.p_app_amount_unit, | ||
| fertilizer.p_density, | ||
| ) |
Contributor
There was a problem hiding this comment.
Scope the fertilizer lookup to the same farm before writing the application.
getFertilizer(fdm, p_id) only proves the fertilizer exists. It does not verify that the selected p_id belongs to the same farm as the target field/application, so these write paths can still attach another farm's inventory item if its ID is known. That is both a data-integrity bug and an authorization gap.
Also applies to: 854-862
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fdm-core/src/fertilizer.ts` around lines 791 - 800, The current call
getFertilizer(fdm, p_id) only checks existence; change it to enforce farm
scoping by either using a farm-scoped lookup or validating the returned
fertilizer.farm_id against the target application/field farm id (e.g., compare
fertilizer.farm_id === field.farm_id or targetFarmId) and throw/return an error
if they differ; update the same pattern used later (the block at 854-862) so
p_id cannot reference inventory from another farm.
SvenVw
approved these changes
Apr 17, 2026
Collaborator
SvenVw
left a comment
SvenVw
left a comment
There was a problem hiding this comment.
Thanks, very nice feature!
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.
Enhancements
addFertilizerApplicationandupdateFertilizerApplicationfunctions now expect an application value in the unit defined for the fertilizer, instead of kg/ha for every fertilizer. The unit is included in the return values ofgetFertilizer,getFertilizersetc. asp_app_amount_unit.getFertilizerApplication,getFertilizerApplicationsetc. now includep_app_amount_displayandp_app_amount_unitwhich are to be shown to the user instead of p_app_amount andkg/ha.Closes #520
Summary by CodeRabbit