Add calculate nitrogen balance target#143
Conversation
…ogen-balance-target
🦋 Changeset detectedLatest commit: 7fc4005 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThe changes introduce a new nitrogen balance target calculation feature, update types and logic to support this, and enhance the UI with bar chart visualizations and comparative displays of nitrogen balance versus target at both farm and field levels. New tests and documentation are added to support the new functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI (Farm/Field Page)
participant Loader
participant NitrogenCalcModule
User->>UI (Farm/Field Page): Navigates to nitrogen balance view
UI (Farm/Field Page)->>Loader: Loads farm, field, and balance data
Loader->>NitrogenCalcModule: Calls calculateNitrogenBalance(input)
NitrogenCalcModule->>NitrogenCalcModule: Computes field/farm balances and target
NitrogenCalcModule-->>Loader: Returns balance and target values
Loader-->>UI (Farm/Field Page): Supplies balance, target, and related data
UI (Farm/Field Page)->>UI (Farm/Field Page): Renders balance vs target, icons, and bar chart
UI (Farm/Field Page)-->>User: Displays nitrogen balance visualization and indicators
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
✨ Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (20)
.changeset/small-jokes-rescue.md (1)
1-3: 🛠️ Refactor suggestion
Semantic version bump should be
minor
Exposing new public types is a backwards-compatible API addition and warrants a minor version bump.Proposed diff:
--- a/.changeset/small-jokes-rescue.md +++ b/.changeset/small-jokes-rescue.md @@ - "@svenvw/fdm-core": patch + "@svenvw/fdm-core": minor📝 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.--- "@svenvw/fdm-core": minor ---🤖 Prompt for AI Agents
In .changeset/small-jokes-rescue.md at lines 1 to 3, the semantic version bump is incorrectly set to patch. Since new public types are being exposed, which is a backwards-compatible API addition, change the version bump from patch to minor to correctly reflect the nature of the update..changeset/long-buttons-sink.md (1)
1-3: 🛠️ Refactor suggestion
Version bump should be
minor
Adding a new catalogue property introduces a new public API; update to a minor version bump.Proposed diff:
--- a/.changeset/long-buttons-sink.md +++ b/.changeset/long-buttons-sink.md @@ - "@svenvw/fdm-core": patch + "@svenvw/fdm-core": minor📝 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.--- "@svenvw/fdm-core": minor ---🤖 Prompt for AI Agents
In the .changeset/long-buttons-sink.md file at lines 1 to 3, the version bump is incorrectly set to patch. Since a new catalogue property is added, which is a public API change, update the version bump from patch to minor to reflect this new feature addition.fdm-app/app/routes/farm.create.$b_id_farm.$calendar.cultivations.$b_lu_catalogue.crop.harvest.$b_id_harvesting.tsx (1)
115-118: 💡 Verification agent
🧩 Analysis chain
Verify consistent data model migration across codebase.
The change from
harvestables[0]toharvestablealigns with the data model refactoring described in the AI summary. The optional chaining is correctly preserved for safety.Please ensure all references to the old
harvestablesarray pattern have been updated:
🏁 Script executed:
#!/bin/bash # Description: Find any remaining harvestables array references # Expected: No results or only intentional legacy code # Search for old harvestables array pattern rg "harvestables\[" --type ts --type tsx rg "\.harvestables\." --type ts --type tsxLength of output: 143
Let’s retry the search across both
.tsand.tsxfiles without relying on built-in types:
🏁 Script executed:
#!/bin/bash # Description: Find any remaining harvestables array references in TS and TSX # Expected: No results or only intentional legacy code # Search for old harvestables array pattern rg -g "*.ts" -g "*.tsx" "harvestables\[" -n rg -g "*.ts" -g "*.tsx" "\.harvestables\." -nLength of output: 1495
Update remaining
harvestables[0]referencesWe’ve detected leftover array‐based accesses that need converting to the new singular
harvestablefield:• fdm-core/src/harvest.ts:550
• fdm-app/app/routes/farm.create.$b_id_farm.$calendar.cultivations.$b_lu_catalogue.crop._index.tsx:140, 142, 144, 146Please replace each
harvestables[0](and any chained properties) with the newharvestableaccess pattern—e.g.:- h1.harvestables[0].harvestable_analyses[0].b_lu_yield + h1.harvestable?.harvestable_analyses?.[0].b_lu_yieldEnsure you handle optional chaining consistently.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In fdm-app/app/routes/farm.create.$b_id_farm.$calendar.cultivations.$b_lu_catalogue.crop.harvest.$b_id_harvesting.tsx around lines 115 to 118, update all remaining references from the old array-based access pattern harvestables[0] to the new singular harvestable field. Replace any occurrence of harvestables[0] and its chained properties with harvestable, preserving optional chaining for safety. Also, extend this update to other files mentioned (fdm-core/src/harvest.ts line 550 and fdm-app/app/routes/farm.create.$b_id_farm.$calendar.cultivations.$b_lu_catalogue.crop._index.tsx lines 140, 142, 144, 146) to ensure consistent data model migration across the codebase.fdm-calculator/src/balance/nitrogen/removal/index.test.ts (1)
6-51: 💡 Verification agent
🧩 Analysis chain
Verify expected calculation values and add test documentation.
The test logic appears sound, but the hardcoded expected values (-21.2, -20, -1.2) need verification against the actual calculation formulas.
Based on the test data:
b_lu_yield: 1000withb_lu_n_harvestable: 20should give -20 for harvest removal- For residue calculation, the expected -1.2 suggests a specific formula involving
b_lu_n_residue: 2,b_lu_hi: 0.4, and yieldConsider adding inline comments explaining how these expected values are calculated to improve test maintainability.
🏁 Script executed:
#!/bin/bash # Description: Verify the nitrogen removal calculation logic and expected test values # Expected: Find the implementation formulas to confirm test assertions # Search for nitrogen removal calculation implementations ast-grep --pattern 'function calculateNitrogenRemovalByHarvests($_, $_, $_) { $$$ }' ast-grep --pattern 'function calculateNitrogenRemovalByResidue($_, $_, $_) { $$$ }' # Also search for the calculation logic in the main removal function rg -A 10 "calculateNitrogenRemovalByHarvests" rg -A 10 "calculateNitrogenRemovalByResidue"Length of output: 21394
Add inline comments explaining the nitrogen removal formulas in the index.test.ts
To improve test maintainability, document how each expected value is derived:
• In fdm-calculator/src/balance/nitrogen/removal/index.test.ts, update the assertions as follows:
- expect(result.total.toNumber()).toBeCloseTo(-21.2) // -20 from harvest + -1.2 from residue + expect(result.total.toNumber()).toBeCloseTo(-21.2) + // Harvest removal: 1000 kg × 20 g/kg ÷ 1000 = 20 kg → -20 + // Residue removal: 1000 kg × (1 - 0.4) × 2 g/kg ÷ 1000 = 1.2 kg → -1.2 - expect(result.harvests.total.toNumber()).toBeCloseTo(-20) + expect(result.harvests.total.toNumber()).toBeCloseTo(-20) + // 1000 kg × 20 g/kg ÷ 1000 = 20 kg → -20 - expect(result.residues.total.toNumber()).toBeCloseTo(-1.2) + expect(result.residues.total.toNumber()).toBeCloseTo(-1.2) + // 1000 kg × (1 - 0.4) × 2 g/kg ÷ 1000 = 1.2 kg → -1.2This makes it clear how the numbers are calculated and eases future updates.
📝 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.it("should calculate total nitrogen removal from harvests and residues", () => { const cultivations: FieldInput["cultivations"] = [ { b_lu: "cultivation1", b_lu_catalogue: "catalogue1", m_cropresidue: true, }, ] const harvests: FieldInput["harvests"] = [ { b_id_harvesting: "harvest1", b_lu: "cultivation1", b_lu_harvest_date: new Date(), harvestable: { b_id_harvestable: "harvestable1", harvestable_analyses: [ { b_lu_yield: 1000, b_lu_n_harvestable: 20 }, ], }, }, ] const cultivationDetailsMap = new Map([ [ "catalogue1", { b_lu_catalogue: "catalogue1", b_lu_croprotation: "cereal", b_lu_yield: 1000, b_lu_n_harvestable: 20, b_lu_hi: 0.4, b_lu_n_residue: 2, b_n_fixation: 0, }, ], ]) const result: NitrogenRemoval = calculateNitrogenRemoval( cultivations, harvests, cultivationDetailsMap, ) expect(result.total.toNumber()).toBeCloseTo(-21.2) // Harvest removal: 1000 kg × 20 g/kg ÷ 1000 = 20 kg → -20 // Residue removal: 1000 kg × (1 - 0.4) × 2 g/kg ÷ 1000 = 1.2 kg → -1.2 expect(result.harvests.total.toNumber()).toBeCloseTo(-20) // 1000 kg × 20 g/kg ÷ 1000 = 20 kg → -20 expect(result.residues.total.toNumber()).toBeCloseTo(-1.2) // 1000 kg × (1 - 0.4) × 2 g/kg ÷ 1000 = 1.2 kg → -1.2 })🤖 Prompt for AI Agents
In fdm-calculator/src/balance/nitrogen/removal/index.test.ts around lines 6 to 51, the test assertions use hardcoded expected values (-21.2, -20, -1.2) without explanation. Add inline comments before each expect statement to document how these values are calculated based on the input data and formulas, such as explaining that -20 comes from yield multiplied by nitrogen content in harvest, and -1.2 is derived from residue nitrogen calculation using residue nitrogen factor, harvest index, and yield. This will improve test clarity and maintainability.fdm-data/src/cultivations/catalogues/brp.ts (1)
29-47: 🛠️ Refactor suggestion
Refactor validation logic for better maintainability.
The validation logic for
b_lu_croprotationworks correctly but is verbose and difficult to maintain. Consider using a more concise approach with predefined valid values.- // Validate b_lu_croprotation - cultivation.b_lu_croprotation !== "other" && - cultivation.b_lu_croprotation !== "clover" && - cultivation.b_lu_croprotation !== "nature" && - cultivation.b_lu_croprotation !== "potato" && - cultivation.b_lu_croprotation !== "grass" && - cultivation.b_lu_croprotation !== "rapeseed" && - cultivation.b_lu_croprotation !== "starch" && - cultivation.b_lu_croprotation !== "maize" && - cultivation.b_lu_croprotation !== "cereal" && - cultivation.b_lu_croprotation !== "sugarbeet" && - cultivation.b_lu_croprotation !== "alfalfa" && - cultivation.b_lu_croprotation !== "catchcrop" - ? (() => { - throw new Error( - `Invalid value for b_lu_croprotation: ${cultivation.b_lu_croprotation}.`, - ) - })() - : cultivation.b_lu_croprotation + // Validate b_lu_croprotation + const validCropRotations = [ + "other", "clover", "nature", "potato", "grass", "rapeseed", + "starch", "maize", "cereal", "sugarbeet", "alfalfa", "catchcrop" + ] as const + + if (!validCropRotations.includes(cultivation.b_lu_croprotation as any)) { + throw new Error( + `Invalid value for b_lu_croprotation: ${cultivation.b_lu_croprotation}. Expected one of: ${validCropRotations.join(", ")}` + ) + } + + const cropRotation = cultivation.b_lu_croprotation📝 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.// Validate b_lu_croprotation const validCropRotations = [ "other", "clover", "nature", "potato", "grass", "rapeseed", "starch", "maize", "cereal", "sugarbeet", "alfalfa", "catchcrop" ] as const if (!validCropRotations.includes(cultivation.b_lu_croprotation as any)) { throw new Error( `Invalid value for b_lu_croprotation: ${cultivation.b_lu_croprotation}. Expected one of: ${validCropRotations.join(", ")}` ) } const cropRotation = cultivation.b_lu_croprotation🤖 Prompt for AI Agents
In fdm-data/src/cultivations/catalogues/brp.ts around lines 29 to 47, the validation for b_lu_croprotation is done by multiple inequality checks which is verbose and hard to maintain. Refactor this by defining an array or set of valid values for b_lu_croprotation and then check if cultivation.b_lu_croprotation is included in that collection. If not included, throw the error as before; otherwise, return the value. This approach improves readability and maintainability.fdm-calculator/src/balance/nitrogen/supply/deposition.test.ts (1)
28-28: 💡 Verification agent
🧩 Analysis chain
Verify the hardcoded expected value for nitrogen deposition.
The test expects a very specific value (19.572) for nitrogen deposition calculation. Consider whether this hardcoded value is reliable and won't break with future changes to the underlying deposition data or calculation logic.
Let me verify if this expected value is documented or derived from a known formula:
🏁 Script executed:
#!/bin/bash # Description: Search for documentation or calculation details for nitrogen deposition values # Expected: Find references to deposition calculation formulas or data sources rg -A 10 -B 5 "19\.572|19\.626|9\.7592" --type ts rg -A 5 "deposition.*calculation|nitrogen.*deposition.*formula" --type tsLength of output: 5170
I need to inspect how the test obtains its input data and whether it mocks or pins the deposition source. Let me dump the entire test file and look for fixtures or HTTP stubbing:
🏁 Script executed:
#!/bin/bash # Show the full deposition test to see data setup and mocking cat fdm-calculator/src/balance/nitrogen/supply/deposition.test.ts # Search for any fixtures or HTTP mocks in the repo rg -n "fixture|__fixtures__|nock|fetchMock|msw" --type tsLength of output: 2893
I need to confirm how
calculateNitrogenSupplyByDepositionfetches and uses the public data URL and whether it’s pinned to a static data source. Let me dump those implementation files:
🏁 Script executed:
#!/bin/bash # Show getFdmPublicDataUrl implementation cat fdm-calculator/src/balance/nitrogen/index.ts # Show deposition calculation logic cat fdm-calculator/src/balance/nitrogen/supply/deposition.tsLength of output: 19644
Mock external GeoTIFF data in deposition tests to prevent brittle hardcoded values
The tests in fdm-calculator/src/balance/nitrogen/supply/deposition.test.ts call calculateNitrogenSupplyByDeposition, which fetches a live GeoTIFF from storage.googleapis.com and computes values like 19.572, 19.626, and 9.7592. Relying on an external dataset will make these assertions break as soon as the source data or TIFF reader behavior changes.
Suggested fixes:
- In deposition.test.ts, mock
geotiff.fromUrl(and its returned image) so you control:
getBoundingBox(),getWidth()/getHeight(), andreadRasters({ window })to return a known Float32Array([X])
Then assertdeposition = X * ((days + 1) / 365).- Use Vitest’s
vi.mock()or dependency injection to overridefromUrlbefore importing the module.- Update expected values in the test to reflect your mocked raster value, isolating logic from external data.
- Ensure no real network call occurs in CI by verifying the mock is active in every test case.
🤖 Prompt for AI Agents
In fdm-calculator/src/balance/nitrogen/supply/deposition.test.ts at line 28, the test relies on a hardcoded expected value (19.572) derived from a live GeoTIFF fetched from an external URL, which makes the test brittle and prone to failure if the external data changes. To fix this, mock the geotiff.fromUrl function and its returned image methods (getBoundingBox, getWidth, getHeight, readRasters) to return controlled, static values. Then update the test to assert the deposition calculation based on these mocked values, ensuring no real network calls occur and the test is stable and isolated from external data changes.fdm-calculator/src/balance/nitrogen/target.test.ts (1)
66-66: 💡 Verification agent
🧩 Analysis chain
Verify target calculation values against specification.
The test assertions use specific expected values (80, 125, 50, 70, 125, etc.) but it would be good to verify these match the specification or calculation logic.
Also applies to: 79-79, 94-94, 109-109, 124-124, 139-139, 154-154, 173-173
🏁 Script executed:
#!/bin/bash # Description: Find the target calculation implementation to verify the expected values # Expected: Implementation showing how target values are determined ast-grep --pattern 'export function calculateTargetForNitrogenBalance($$$) { $$$ }'Length of output: 87
🏁 Script executed:
#!/bin/bash # List all TypeScript files in the nitrogen directory rg --files -g "fdm-calculator/src/balance/nitrogen/*.ts" # Search for any “calculate” functions in that directory rg -n "calculate" fdm-calculator/src/balance/nitrogen # If a target implementation file exists, show its first 200 lines if [[ -f fdm-calculator/src/balance/nitrogen/target.ts ]]; then sed -n '1,200p' fdm-calculator/src/balance/nitrogen/target.ts fiLength of output: 28936
Confirm target values in calculateTargetForNitrogenBalance
The implementation in
src/balance/nitrogen/target.tshard-codes these base values from Ros et al. 2023:
- Grassland + sand + dry → 80
- Grassland + other soils → 125
- Arable + sand + dry → 50
- Arable + sand + average → 70
- Arable + sand + wet → 125
- Arable + clay + dry → 115 then immediately overwritten to 125
- Arable + clay + other → 125
Please verify against the Ros et al. 2023 specification that these are the intended values. In particular, confirm whether the dry-clay arable case should be 115 or 125 and remove the redundant assignment if necessary.
Files/locations to review:
- fdm-calculator/src/balance/nitrogen/target.ts (clay/dry logic around lines 60–75)
- fdm-calculator/src/balance/nitrogen/target.test.ts (expected values for clay/dry and other clay cases)
🤖 Prompt for AI Agents
In fdm-calculator/src/balance/nitrogen/target.test.ts at line 66 and related test lines (79, 94, 109, 124, 139, 154, 173), verify that the expected target values in the test assertions match the specification from Ros et al. 2023 as implemented in fdm-calculator/src/balance/nitrogen/target.ts around lines 60 to 75. Specifically, confirm whether the dry-clay arable target should be 115 or 125 and remove any redundant assignments in the implementation. Update the test expected values accordingly to reflect the correct specification.fdm-core/src/harvest.test.ts (1)
203-203: 💡 Verification agent
🧩 Analysis chain
Verify the data model change from array to singular object.
The change from
harvestables[0]toharvestableindicates a significant structural change in the data model. Ensure this breaking change has been consistently applied throughout the codebase.Also applies to: 219-219, 263-264
🏁 Script executed:
#!/bin/bash # Description: Verify that all references to harvestables array have been updated to singular harvestable # Search for any remaining array access patterns for harvestables rg -A 3 -B 3 "harvestables\[" --type ts # Search for any remaining references to harvestables as an array rg -A 3 -B 3 "\.harvestables\s*\." --type tsLength of output: 5563
Inconsistent singular “harvestable” usage
Although the core test (
harvest.test.ts) and the harvest logic now useharvest.harvestableinstead ofharvestables[0], the UI layer still references the old array form. Please update all remainingharvestables[0]occurrences to the new singular model. For example:• fdm-app/app/routes/farm.create.$b_id_farm.$calendar.cultivations.$b_lu_catalogue.crop._index.tsx
– Replace
ts h1.harvestables[0].harvestable_analyses[0].b_lu_yield h2.harvestables[0].harvestable_analyses[0].b_lu_yield … h1.harvestables[0].harvestable_analyses[0].b_lu_n_harvestable h2.harvestables[0].harvestable_analyses[0].b_lu_n_harvestable
with
ts h1.harvestable.harvestable_analyses[0].b_lu_yield h2.harvestable.harvestable_analyses[0].b_lu_yield … h1.harvestable.harvestable_analyses[0].b_lu_n_harvestable h2.harvestable.harvestable_analyses[0].b_lu_n_harvestable• Run a global search for
harvestables[0]in thefdm-app/directory and replace all instances withharvestable.🤖 Prompt for AI Agents
In fdm-core/src/harvest.test.ts around line 203 and also in fdm-app/ directory files such as app/routes/farm.create.$b_id_farm.$calendar.cultivations.$b_lu_catalogue.crop._index.tsx, update all references from the old array form harvestables[0] to the new singular object form harvestable. This includes changing expressions like harvestables[0].harvestable_analyses[0].b_lu_yield to harvestable.harvestable_analyses[0].b_lu_yield. Perform a global search for harvestables[0] in the fdm-app/ directory and replace all occurrences with harvestable to ensure consistency with the updated data model.fdm-calculator/src/balance/nitrogen/removal/harvest.ts (1)
66-74: 🛠️ Refactor suggestion
Consider edge case: empty harvestable analyses.
The current logic handles 1 or multiple analyses but doesn't explicitly handle the case where
removalsHarvest.length === 0. While this might be prevented by data validation elsewhere, consider adding explicit handling for robustness.let removalHarvest = new Decimal(0) - if (removalsHarvest.length === 1) { + if (removalsHarvest.length === 0) { + // No harvestable analyses found, keep default value of 0 + removalHarvest = new Decimal(0) + } else if (removalsHarvest.length === 1) { removalHarvest = removalsHarvest[0] } else if (removalsHarvest.length > 1) {📝 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.let removalHarvest = new Decimal(0) if (removalsHarvest.length === 0) { // No harvestable analyses found, keep default value of 0 removalHarvest = new Decimal(0) } else if (removalsHarvest.length === 1) { removalHarvest = removalsHarvest[0] } else if (removalsHarvest.length > 1) { // If multiple harvestable analyses exist take the average removalHarvest = removalsHarvest .reduce((a, b) => a.add(b), new Decimal(0)) .dividedBy(new Decimal(removalsHarvest.length)) }🤖 Prompt for AI Agents
In fdm-calculator/src/balance/nitrogen/removal/harvest.ts around lines 66 to 74, the code does not explicitly handle the case when removalsHarvest is empty. To fix this, add a condition to check if removalsHarvest.length is 0 and explicitly set removalHarvest to Decimal(0) in that case, ensuring robustness against empty input arrays.fdm-core/src/harvest.d.ts (1)
7-7: 💡 Verification agent
🧩 Analysis chain
Verify the breaking change: harvestables → harvestable.
The change from plural
harvestables: HarvestableType[]to singularharvestable: Harvestableis a significant breaking change that enforces a 1-to-1 relationship. Ensure all consuming code has been updated accordingly.
🏁 Script executed:
#!/bin/bash # Search for any remaining references to the old plural form rg -i "harvestables" --type ts --type tsx # Search for usage of the old interface names rg "HarvestType|HarvestableType|HarvestableAnalysisType" --type ts --type tsxLength of output: 177
🏁 Script executed:
#!/bin/bash # Search for any references to the old plural property or type names across all files rg -n "harvestables" rg -n "HarvestableType"Length of output: 5918
Fix required: propagate
harvestablerename across all consuming modulesThe change in fdm-core/src/harvest.d.ts from
– harvestables: HarvestableType[] + harvestable: Harvestableis a breaking change. A quick
rg -n harvestablesstill shows dozens of references to the old plural form. You must update every consumer to use the new singular property (or handle an array if that’s intended).Key locations needing updates:
- fdm-core/src/cultivation.ts (lines 597, 663, 666, 672)
- fdm-core/src/harvest.ts (inserts/deletes at lines 85, 337–339, 528–550, 565–569)
- fdm-core/src/db/schema.ts (table definitions & type aliases at lines 353–355, 364–365, 373, 432)
- fdm-docs/docs/core-concepts/02-schema.md (schema docs at lines 169, 205, 220)
- fdm-app/app/routes/ (all occurrences of
harvestables[...]in thefarm.createandfarm.$b_id_farm...pages)- fdm-app/app/components/custom/harvest/ (
types.tsx,list.tsx)Ensure each reference to
object.harvestablesis replaced with
object.harvestableor refactored to work with an array if multiple harvestables remain valid.
🤖 Prompt for AI Agents
In fdm-core/src/harvest.d.ts at line 7, the property name changed from the plural harvestables: HarvestableType[] to the singular harvestable: Harvestable, which is a breaking change. You need to update all consuming modules to reflect this rename by replacing every reference to object.harvestables with object.harvestable or refactor the code to handle an array if multiple harvestables are still needed. Key files to update include fdm-core/src/cultivation.ts (lines 597, 663, 666, 672), fdm-core/src/harvest.ts (lines 85, 337–339, 528–550, 565–569), fdm-core/src/db/schema.ts (lines 353–355, 364–365, 373, 432), fdm-docs/docs/core-concepts/02-schema.md (lines 169, 205, 220), and all relevant files in fdm-app/app/routes and fdm-app/app/components/custom/harvest/. Ensure consistent usage of the singular property or adjust logic to handle arrays as appropriate.fdm-calculator/src/balance/nitrogen/input.ts (1)
125-131: 💡 Verification agent
🧩 Analysis chain
Minor inconsistency: fdmPublicDataUrl not in NitrogenBalanceInput type.
The function returns
fdmPublicDataUrlbut this property doesn't appear in theNitrogenBalanceInputtype definition shown in the relevant code snippets. Verify if this is intentional or if the type needs updating.
🏁 Script executed:
#!/bin/bash # Check the complete NitrogenBalanceInput type definition ast-grep --pattern 'export type NitrogenBalanceInput = { $$$ }'Length of output: 737
🏁 Script executed:
#!/bin/bash # Locate usages and definition of fdmPublicDataUrl within the nitrogen balance input generator rg "fdmPublicDataUrl" -n fdm-calculator/src/balance/nitrogenLength of output: 2806
Add missing
fdmPublicDataUrltoNitrogenBalanceInputThe
input.tsgenerator returns anfdmPublicDataUrlproperty, but theNitrogenBalanceInputtype infdm-calculator/src/balance/nitrogen/types.d.tsis missing this field. Update the type so they stay in sync:export type NitrogenBalanceInput = { fields: FieldInput[] fertilizerDetails: FertilizerDetail[] cultivationDetails: CultivationDetail[] timeFrame: { start: Date end: Date } + fdmPublicDataUrl: string }This ensures consumers of
NitrogenBalanceInputwon’t see a type error when accessing the URL.📝 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.// fdm-calculator/src/balance/nitrogen/types.d.ts export type NitrogenBalanceInput = { fields: FieldInput[] fertilizerDetails: FertilizerDetail[] cultivationDetails: CultivationDetail[] timeFrame: { start: Date end: Date } fdmPublicDataUrl: string }🤖 Prompt for AI Agents
In fdm-calculator/src/balance/nitrogen/input.ts around lines 125 to 131, the returned object includes the property fdmPublicDataUrl, but this property is missing from the NitrogenBalanceInput type definition in fdm-calculator/src/balance/nitrogen/types.d.ts. To fix this, add the fdmPublicDataUrl field with the appropriate type to the NitrogenBalanceInput type so that the return value matches the type definition and prevents type errors when accessing this property.fdm-calculator/src/balance/nitrogen/target.ts (2)
112-113:
⚠️ Potential issueFix duplicate assignment bug.
There's a duplicate assignment where
targetValueis set to 115 and immediately overwritten to 125 on the next line.targetValue = new Decimal(115) - targetValue = new Decimal(125) } else if (cultivationType === "arable" && soilType === "clay") {📝 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.targetValue = new Decimal(115) } else if (cultivationType === "arable" && soilType === "clay") {🤖 Prompt for AI Agents
In fdm-calculator/src/balance/nitrogen/target.ts at lines 112 to 113, there is a duplicate assignment to targetValue where it is first set to 115 and then immediately overwritten to 125. Remove the redundant assignment so that targetValue is assigned only once with the intended value.
29-39: 🛠️ Refactor suggestion
Add validation for empty cultivations array.
The function doesn't validate that the cultivations array contains elements, which could lead to unexpected behavior.
// Determine whether field is grassland or arable + if (cultivations.length === 0) { + throw new Error("No cultivations provided for target calculation") + } + let cultivationType = "arable"📝 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.// Determine whether field is grassland or arable if (cultivations.length === 0) { throw new Error("No cultivations provided for target calculation") } let cultivationType = "arable" cultivations.map((cultivation) => { const cultivationDetail = cultivationDetailsMap.get( cultivation.b_lu_catalogue, ) if (cultivationDetail?.b_lu_croprotation === "grass") { cultivationType = "grassland" } })🤖 Prompt for AI Agents
In fdm-calculator/src/balance/nitrogen/target.ts around lines 29 to 39, add a validation check to ensure the cultivations array is not empty before processing it. If the array is empty, handle this case appropriately, such as returning a default value or throwing an error, to prevent unexpected behavior later in the function.fdm-calculator/src/conversions/soil.ts (1)
96-97:
⚠️ Potential issueFix incorrect unit in documentation.
The bulk density unit should be g/cm³, not kg/m³. The clamping values (0.5 to 3) confirm this.
- * for agricultural soils (0.5 to 3 kg / m³) to maintain accuracy. - * @param a_som_loi - The soil organic matter content as a percentage (%). + * for agricultural soils (0.5 to 3 g/cm³) to maintain accuracy. + * @param a_som_loi - The soil organic matter content as a percentage (%).📝 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.* for agricultural soils (0.5 to 3 g/cm³) to maintain accuracy. * @param a_som_loi - The soil organic matter content as a percentage (%).🤖 Prompt for AI Agents
In fdm-calculator/src/conversions/soil.ts around lines 96 to 97, the documentation incorrectly states the bulk density unit as kg/m³. Update the unit to g/cm³ to match the clamping values (0.5 to 3) and ensure the documentation accurately reflects the expected unit for bulk density.fdm-core/src/soil.ts (1)
482-482:
⚠️ Potential issueFix unit notation for organic carbon.
The unit should be "g C/kg" to match the conversion functions and standard notation.
- unit: "g C/g", + unit: "g C/kg",📝 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.unit: "g C/kg",🤖 Prompt for AI Agents
In fdm-core/src/soil.ts at line 482, the unit notation for organic carbon is incorrectly set as "g C/g". Change this to "g C/kg" to align with the conversion functions and standard notation for organic carbon units.fdm-calculator/src/balance/nitrogen/index.ts (2)
314-319:
⚠️ Potential issueAdd null safety for date sorting.
The sort function doesn't handle null or undefined sampling dates, which could cause runtime errors.
// Sort the soil analyses by date (most recent first) soilAnalyses.sort((a, b) => { + // Handle null/undefined dates by treating them as very old dates + const dateA = a.b_sampling_date ? new Date(a.b_sampling_date).getTime() : 0; + const dateB = b.b_sampling_date ? new Date(b.b_sampling_date).getTime() : 0; return ( - new Date(b.b_sampling_date).getTime() - - new Date(a.b_sampling_date).getTime() + dateB - dateA ) })📝 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.// Sort the soil analyses by date (most recent first) soilAnalyses.sort((a, b) => { // Handle null/undefined dates by treating them as very old dates const dateA = a.b_sampling_date ? new Date(a.b_sampling_date).getTime() : 0; const dateB = b.b_sampling_date ? new Date(b.b_sampling_date).getTime() : 0; return ( dateB - dateA ); });🤖 Prompt for AI Agents
In fdm-calculator/src/balance/nitrogen/index.ts around lines 314 to 319, the sorting logic for soilAnalyses does not handle cases where b_sampling_date might be null or undefined, leading to potential runtime errors. Update the sort comparator to first check if either date is null or undefined and handle those cases safely, for example by treating missing dates as the earliest or latest, before converting to Date and comparing their time values.
129-136:
⚠️ Potential issueAvoid mutating input parameters.
Directly modifying the
timeFrameobject can cause unexpected side effects for callers. Create a local copy instead.+ // Create a local copy of timeFrame to avoid mutation + const localTimeFrame = { ...timeFrame }; + // If timeframe is broader than field existence, shorten it - if (field.b_start?.getTime() > timeFrame.start.getTime()) { - timeFrame.start = field.b_start + if (field.b_start?.getTime() > localTimeFrame.start.getTime()) { + localTimeFrame.start = field.b_start } - if (field.b_end?.getTime() < timeFrame.end.getTime()) { - timeFrame.end = field.b_end + if (field.b_end?.getTime() < localTimeFrame.end.getTime()) { + localTimeFrame.end = field.b_end }Then use
localTimeFramein subsequent function calls instead oftimeFrame.📝 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.// Create a local copy of timeFrame to avoid mutation const localTimeFrame = { ...timeFrame }; // If timeframe is broader than field existence, shorten it if (field.b_start?.getTime() > localTimeFrame.start.getTime()) { localTimeFrame.start = field.b_start } if (field.b_end?.getTime() < localTimeFrame.end.getTime()) { localTimeFrame.end = field.b_end }🤖 Prompt for AI Agents
In fdm-calculator/src/balance/nitrogen/index.ts around lines 129 to 136, avoid mutating the input parameter timeFrame directly. Instead, create a local copy of timeFrame (e.g., localTimeFrame) before these conditional checks, then update localTimeFrame.start and localTimeFrame.end as needed. Use this localTimeFrame in subsequent function calls to prevent side effects on the original timeFrame object.fdm-core/src/cultivation.ts (1)
758-772: 💡 Verification agent
🧩 Analysis chain
Verify impact of harvestables structure change.
The change from
harvestablesarray to singularharvestableobject is a significant structural modification. Ensure this change is consistently applied across all consuming code.
🏁 Script executed:
#!/bin/bash # Description: Search for usage of harvestables vs harvestable to verify consistency echo "=== Searching for 'harvestables' usage (should be updated) ===" rg -n "harvestables" --type ts echo "=== Searching for 'harvestable' usage (new pattern) ===" rg -n "harvestable[^s]" --type ts echo "=== Check test files for harvestables usage ===" fd "test" --type f | xargs rg -l "harvestables"Length of output: 47042
🏁 Script executed:
#!/usr/bin/env bash # Locate the crop index file in the fdm-app routes and inspect the loader output usage file=$(fd -t f -g "*cultivations*crop._index.tsx" | head -n1) echo "Inspecting file: $file" sed -n '100,180p' "$file"Length of output: 3845
Incomplete Migration of Harvestable Structure — Fix Required
The change from an array of
harvestablesto a singularharvestableobject isn’t consistently applied. There are still many consumers expectingharvestables[]—they must be updated to use the new shape (harvestable.harvestable_analyses) throughout:• fdm-app loader (…crop._index.tsx)
–interface HarvestInfo { harvestables: { … }[] }
– allh1.harvestables[0]…/harvestables[0].harvestable_analyses[0]usages• UI components in fdm-app/app/components/custom/harvest/
–list.tsx,form.tsxreferencingharvestable?.harvestable_analyses?.[0]
– pages underroutes/farm…cultivations…harvest…usingharvestables• fdm-calculator modules
–balance/nitrogen/volatization/residues.ts& tests
–balance/nitrogen/removal/*& tests• Type definitions in fdm-core
–src/cultivation.d.ts,src/harvest.d.tsstill define arrays
– updateharvestablesTypeSelect,harvestableSamplingTypeSelect, etc.• Service layer in fdm-core/src/harvest.ts & src/cultivation.ts
– functions that build or consumeharvestablesarraysPlease refactor all usages to the new singular
harvestableobject and itsharvestable_analysesarray.🤖 Prompt for AI Agents
In fdm-core/src/cultivation.ts around lines 758 to 772, the data structure for harvestables has been changed from an array named 'harvestables' to a singular object 'harvestable' containing an array 'harvestable_analyses'. However, this change is not consistently applied across the codebase. To fix this, update all references and type definitions in the entire project that still expect 'harvestables' as an array to use the new singular 'harvestable' object and access its 'harvestable_analyses' array accordingly. This includes updating interfaces, UI components, service layers, and tests to reflect the new structure consistently.fdm-core/src/harvest.ts (2)
550-550: 🛠️ Refactor suggestion
Verify data structure consistency in getHarvestSimplified.
The function correctly assigns the singular
harvestableobject, but there's still inconsistency withharvestable_analyses:
- Line 588 assigns the entire
harvestableAnalysesarray toharvest.harvestable.harvestable_analyses- This conflicts with the pattern of converting to singular objects
- The access pattern in
removeHarvestexpects an array with[0]indexingTo maintain consistency with the singular object pattern, consider updating the type definition and assignment:
- harvest.harvestable.harvestable_analyses = harvestableAnalyses + harvest.harvestable.harvestable_analysis = harvestableAnalyses[0]This would align with the TODO comment's mention of "a single analysis" and make the data structure more predictable.
Also applies to: 583-583, 588-588
🤖 Prompt for AI Agents
In fdm-core/src/harvest.ts around lines 550, 583, and 588, the assignment of harvestable_analyses is inconsistent with the singular object pattern used for harvestable. Instead of assigning the entire harvestableAnalyses array to harvest.harvestable.harvestable_analyses, update the type definition to expect a single analysis object and assign only the first element of harvestableAnalyses to harvest.harvestable.harvestable_analyses. This change will align with the TODO comment about "a single analysis" and ensure the data structure is consistent and predictable, matching the expected access pattern in removeHarvest.
299-302: 🛠️ Refactor suggestion
Potential inconsistency in data structure handling.
There's an inconsistency in how
harvestable_analysesis being handled:
- Line 301 accesses
harvestable_analyses[0], treating it as an array- But in
getHarvestSimplified(line 588), the entireharvestableAnalysesarray is assigned toharvestable_analysesThis suggests
harvestable_analysesis still an array, which is inconsistent with the pattern of converting from arrays to singular objects.Consider making the data structure consistent. If the intention is to have a single analysis per harvestable (as suggested by the TODO comment), update the assignment in
getHarvestSimplified:- harvest.harvestable.harvestable_analyses = harvestableAnalyses + harvest.harvestable.harvestable_analysis = harvestableAnalyses[0]And update the access pattern here:
const b_id_harvestable_analysis = - harvest.harvestable.harvestable_analyses[0] + harvest.harvestable.harvestable_analysis .b_id_harvestable_analysis📝 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.const b_id_harvestable = harvest.harvestable.b_id_harvestable const b_id_harvestable_analysis = harvest.harvestable.harvestable_analysis .b_id_harvestable_analysis🤖 Prompt for AI Agents
In fdm-core/src/harvest.ts around lines 299 to 302, the code accesses harvest.harvestable.harvestable_analyses[0] as if it is an array, but elsewhere harvestable_analyses is assigned the entire array, causing inconsistency. To fix this, modify getHarvestSimplified (around line 588) to assign only the first element of harvestableAnalyses to harvestable_analyses if only one analysis is intended. Then update the access here to treat harvestable_analyses as a singular object rather than an array, ensuring consistent data structure usage throughout the code.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development #143 +/- ##
===============================================
- Coverage 93.85% 93.83% -0.02%
===============================================
Files 65 66 +1
Lines 7779 7900 +121
Branches 916 943 +27
===============================================
+ Hits 7301 7413 +112
- Misses 476 485 +9
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:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
.changeset/curly-streets-cross.md (1)
5-5: Improve grammar for better clarity.The description could be clearer and more grammatically correct.
-Show at nitrogen balance a comparison of the balance with the target level at farm and field level +Show a comparison of the nitrogen balance with the target level on both farm and field pages🧰 Tools
🪛 LanguageTool
[uncategorized] ~5-~5: Possible missing comma found.
Context: ...w/fdm-app": minor --- Show at nitrogen balance a comparison of the balance with the ta...(AI_HYDRA_LEO_MISSING_COMMA)
.changeset/thin-planets-see.md (1)
5-5: Fix grammar for subject-verb agreement.The sentence has grammatical issues with noun number and verb agreement.
-At the nitrogen balance page for farm and field show a bar chart to compare supply, removal and emission +Show a bar chart on the nitrogen balance pages for farms and fields to compare supply, removal, and emission🧰 Tools
🪛 LanguageTool
[uncategorized] ~5-~5: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ... the nitrogen balance page for farm and field show a bar chart to compare supply, rem...(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
[uncategorized] ~5-~5: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...itrogen balance page for farm and field show a bar chart to compare supply, removal ...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
fdm-app/app/components/custom/balance/nitrogen-chart.tsx (2)
22-28: Simplify object property assignments.The object property assignments are redundant and can be simplified using shorthand syntax.
const chartData = [ { - supply: supply, - removal: removal, - emission: emission, + supply, + removal, + emission, }, ]
17-17: Consider using the balance prop or removing it.The
balanceprop is defined in the component interface but never used in the chart rendering. Either utilize this prop in the visualization or remove it from the interface.Should the balance be displayed as a reference line or additional visual element in the chart?
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen.$b_id.tsx (2)
163-174: Improve template literal formatting and icon styling consistency.The target comparison logic is good, but the formatting and styling could be improved.
<div className="flex items-center gap-4"> <p> - {`${nitrogenBalanceResult.balance} / - ${nitrogenBalanceResult.target}`} + {`${nitrogenBalanceResult.balance} / ${nitrogenBalanceResult.target}`} </p> {nitrogenBalanceResult.balance <= nitrogenBalanceResult.target ? ( - <CircleCheck className="text-green-500 bg-green-100 p-0 rounded-full " /> + <CircleCheck className="text-green-500 bg-green-100 rounded-full" /> ) : ( - <CircleAlert className="text-red-500 bg-red-100 p-0 rounded-full " /> + <CircleAlert className="text-red-500 bg-red-100 rounded-full" /> )} </div>
243-250: Fix grammar in descriptive text.There's a missing period and the sentence structure could be improved.
<CardDescription> - De stikstofbalans voor {field.b_name} van{" "} - {farm.b_name_farm} . De balans is het + De stikstofbalans voor {field.b_name} van{" "} + {farm.b_name_farm}. De balans is het verschil tussen de totale aanvoer, afvoer en emissie van stikstof. Een positieve balans betekent een overschot aan stikstof, een negatieve balans een tekort. </CardDescription>fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx (1)
282-296: Optimize field data lookup for better performance.The current implementation performs
fields.find()operations inside the map function, which results in O(n²) complexity. Consider creating a lookup map for better performance.- {nitrogenBalanceResult.fields.map( - (field) => ( + {(() => { + const fieldsMap = new Map(fields.map(f => [f.b_id, f])); + return nitrogenBalanceResult.fields.map((field) => { + const fieldData = fieldsMap.get(field.b_id); + return ( <div className="flex items-center" key={field.b_id} > {field.balance <= field.target ? ( <CircleCheck className="text-green-500 bg-green-100 p-0 rounded-full w-6 h-6" /> ) : ( <CircleAlert className="text-red-500 bg-red-100 p-0 rounded-full w-6 h-6" /> )} <div className="ml-4 space-y-1"> <NavLink to={`./${field.b_id}`} > <p className="text-sm font-medium leading-none hover:underline"> - { - fields.find( - (f) => - f.b_id === - field.b_id, - )?.b_name - } + {fieldData?.b_name} </p> </NavLink> <p className="text-sm text-muted-foreground"> - { - fields.find( - (f) => - f.b_id === - field.b_id, - )?.b_area - }{" "} + {fieldData?.b_area}{" "} ha </p> </div> <div className="ml-auto font-medium"> {field.balance} /{" "} {field.target} </div> </div> - ), - )} + ); + }); + })()}Also applies to: 302-303
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/curly-streets-cross.md(1 hunks).changeset/thin-planets-see.md(1 hunks)fdm-app/app/components/custom/balance/nitrogen-chart.tsx(1 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen.$b_id.tsx(5 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx(7 hunks)fdm-calculator/src/balance/nitrogen/types.d.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- fdm-calculator/src/balance/nitrogen/types.d.ts
🧰 Additional context used
🪛 LanguageTool
.changeset/thin-planets-see.md
[uncategorized] ~5-~5: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ... the nitrogen balance page for farm and field show a bar chart to compare supply, rem...
(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
[uncategorized] ~5-~5: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...itrogen balance page for farm and field show a bar chart to compare supply, removal ...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
.changeset/curly-streets-cross.md
[uncategorized] ~5-~5: Possible missing comma found.
Context: ...w/fdm-app": minor --- Show at nitrogen balance a comparison of the balance with the ta...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (10)
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen.$b_id.tsx (3)
124-124: LGTM! Good addition of farm context.Adding farm data to the loader return provides useful context for the field-level view.
154-156: LGTM! Better card title and icon choice.The updated title "Overschot / Doel (Perceel)" and ArrowRightLeft icon better represent the comparison functionality.
258-271: LGTM! Excellent integration of the nitrogen balance chart.The chart component is properly integrated with the correct props mapping from the nitrogen balance result.
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx (7)
30-34: LGTM! Import additions are well-organized.The new imports for icons (
ArrowRightLeft,Check,CircleAlert,CircleCheck) and theNitrogenBalanceChartcomponent are properly added and used consistently throughout the component for the balance vs target functionality.Also applies to: 39-39
103-103: LGTM! Farm data integration is correct.The farm object is properly added to the loader return data and correctly destructured in the component, enabling the display of farm-specific information in the UI.
Also applies to: 116-116
126-126: LGTM! Card title and icon updates are semantically appropriate.The change from "Bedrijfsoverschot" to "Overschot / Doel (Bedrijf)" and the icon change from
HousetoArrowRightLeftbetter represents the comparative nature of balance vs target display.Also applies to: 128-128
135-146: LGTM! Balance vs target display with conditional indicators is well-implemented.The conditional rendering logic correctly shows a green check icon when balance is within target (
balance <= target) and a red alert icon when it exceeds the target. The display format clearly shows both values.
215-222: LGTM! Descriptive text enhances user understanding.The addition of contextual description using the farm name and explaining the nitrogen balance concept improves user experience and provides valuable context.
229-236: LGTM! Chart component integration is properly implemented.The
NitrogenBalanceChartcomponent is correctly integrated with appropriate props passed from the nitrogen balance result data.
269-274: LGTM! Field-level balance indicators are consistent.The conditional icons for each field follow the same logic as the farm-level display, providing consistent visual feedback across the interface.
|
see some checks failing... |
Coverage is 0.02% lower... |
2 participants
Summary by CodeRabbit