Add carbon sequestration feature with calculations and metrics#465
Merged
Conversation
…t carbon sequestration, inlcuding current state, potential maximal state and relatable figures of how much carbon can be stored
🦋 Changeset detectedLatest commit: 8ecaebe 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 |
Contributor
WalkthroughAdds a Carbon Sequestration feature: new NMI estimate fields and schema, two calculator functions (water & NLV supply by SOM) with tests and exports, a CarbonSequestrationCard and skeleton, layout and atlas field UI refactors, and loader wiring to compute and pass carbon estimates to the UI. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Route as Route Loader
participant NMI as NMI API
participant Calc as fdm-calculator
participant UI as CarbonSequestrationCard
User->>Route: Navigate to field details
Route->>NMI: Fetch soil estimates (include new carbon fields)
NMI-->>Route: Return a_som_loi, b_som_potential, b_c_st03, b_c_st03_potential, b_c_delta
Route->>Calc: calculateWaterSupplyBySom(soil params)
Calc-->>Route: extraWaterStorage (mm)
Route->>Calc: calculateNlvSupplyBySom(soil params)
Calc-->>Route: extraNMineralization (kg N/ha)
Route->>Route: Aggregate carbonEstimates object
Route-->>UI: Pass carbonEstimates & soil params
UI->>UI: Compute % of potential, CO2eq, equivalents, euro estimate
UI-->>User: Render CarbonSequestrationCard with metrics and methodology
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## development #465 +/- ##
===============================================
+ Coverage 87.98% 88.10% +0.11%
===============================================
Files 91 93 +2
Lines 4629 4675 +46
Branches 1497 1498 +1
===============================================
+ Hits 4073 4119 +46
Misses 556 556
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:
|
…nge in water holding capacity for a topsoil based on change in SOM
coderabbitai
Bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
fdm-app/app/routes/farm.$b_id_farm.$calendar.atlas.tsx (1)
94-100:⚠️ Potential issue | 🟡 MinorAdd required
actionprop to<Header>.The
Headercomponent requires anactionprop (typed asaction: HeaderActionProps | undefinedwithout optional marker). Omitting it will cause a TypeScript compilation error. All other usages in the codebase explicitly passaction={undefined}. Apply the same pattern here.Fix
- <Header> + <Header action={undefined}>🤖 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.atlas.tsx around lines 94 - 100, The Header component requires an explicit action prop (typed as HeaderActionProps | undefined); update the JSX that renders <Header> (the block containing <HeaderFarm ... /> and <HeaderAtlas ... />) to pass action={undefined} (matching other usages) so TypeScript compiles without error.fdm-app/app/components/blocks/atlas-fields/cultivation-history.tsx (1)
240-272:⚠️ Potential issue | 🟡 MinorSkeleton description text diverges from the live card.
The skeleton's
CardDescription(Lines 247-249) says "De gewassen van de afgelopen jaren op dit perceel volgens Basisregistratie Gewaspercelen." while the live card (Lines 77-78) says "Dit zijn de gewassen zoals geregistreerd in de Basisregistratie Gewaspercelen (BRP)." Keeping these consistent avoids a visible text shift when the skeleton is replaced by the loaded content.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/atlas-fields/cultivation-history.tsx` around lines 240 - 272, The skeleton's CardDescription text in the CultivationHistorySkeleton component currently differs from the real card; update the string inside the CardDescription in CultivationHistorySkeleton so it exactly matches the live card text "Dit zijn de gewassen zoals geregistreerd in de Basisregistratie Gewaspercelen (BRP)." to prevent a visible shift when swapping skeleton -> loaded content; locate the CardDescription inside the CultivationHistorySkeleton function and replace the current Dutch sentence with the exact live-card sentence.fdm-app/app/routes/farm.$b_id_farm.$calendar.atlas.fields.$centroid.tsx (1)
160-165:⚠️ Potential issue | 🔴 CriticalRemove
a_cn_frfromsoilParameterEstimates— it's not accepted bySoilTextureCard.Line 164 adds
a_cn_frto thesoilParameterEstimatesobject passed toSoilTextureCard, but the component's prop type definition (atfdm-app/app/components/blocks/atlas-fields/soil-texture.tsx:13-17) only acceptsa_clay_mi,a_silt_mi, anda_sand_mi. The strict type definition will cause a TypeScript compilation error. Either removea_cn_frfrom this object, or updateSoilTextureCard's prop type if the field should be displayed.🤖 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.atlas.fields.$centroid.tsx around lines 160 - 165, The soilParameterEstimates object being passed to SoilTextureCard includes a non-supported key a_cn_fr which triggers a TypeScript error; remove a_cn_fr from the soilParameterEstimates object so it only contains a_clay_mi, a_silt_mi, and a_sand_mi (or alternatively, if you intend to display a_cn_fr, add it to SoilTextureCard's prop type and component interface instead). Locate the construction of soilParameterEstimates (the object with a_clay_mi/a_silt_mi/a_sand_mi/a_cn_fr) and delete the a_cn_fr entry or update SoilTextureCard's props to accept a_cn_fr and render it accordingly.
🧹 Nitpick comments (4)
fdm-app/app/components/blocks/atlas-fields/field-details.tsx (1)
22-28: Consider hoistingregionLabelsto module scope.This is a static constant recreated on every render. Moving it outside the component avoids unnecessary allocations.
Proposed change
+const regionLabels = { + klei: "Klei", + veen: "Veen", + loess: "Löss", + zand_nwc: "Noordelijk, westelijk, en centraal zand", + zand_zuid: "Zuidelijk zand", +} as const + export function FieldDetailsCard({ fieldDetails, }: { ... }) { - const regionLabels = { - klei: "Klei", - veen: "Veen", - loess: "Löss", - zand_nwc: "Noordelijk, westelijk, en centraal zand", - zand_zuid: "Zuidelijk zand", - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/atlas-fields/field-details.tsx` around lines 22 - 28, regionLabels is a static object recreated on every render inside the FieldDetails component; move the const regionLabels definition out to module scope (above the FieldDetails component) so it’s allocated once, then update any references within FieldDetails to use the hoisted regionLabels; ensure the identifier stays the same and remove the in-component declaration to avoid shadowing.fdm-app/app/components/blocks/atlas-fields/carbon-sequestration.tsx (1)
48-61: Derived metrics use pre-rounded intermediates, causing compounding rounding errors.
carKmEquivalent(line 59) and the euro estimate (line 200) are computed frompotentialCO2eq, which is itself already rounded. This can silently shift displayed values. For example,b_c_delta = 0.14→potentialCO2eqrounds to0.5→carKmEquivalent = 4000, but the unrounded path gives~4111 km.Compute derived values from raw inputs, rounding only at the presentation layer.
Proposed refactor
// API values const currentOM = Math.round(carbonEstimates.a_som_loi * 10) / 10 const maxOM = Math.round(carbonEstimates.b_som_potential * 10) / 10 - const potentialCO2eq = - Math.round(carbonEstimates.b_c_delta * 3.67 * 10) / 10 + const rawCO2eq = carbonEstimates.b_c_delta * 3.67 + const potentialCO2eq = Math.round(rawCO2eq * 10) / 10 // Derived values const percentageOfMax = - maxOM > 0 ? Math.min((currentOM / maxOM) * 100, 100) : 0 + carbonEstimates.b_som_potential > 0 + ? Math.min((carbonEstimates.a_som_loi / carbonEstimates.b_som_potential) * 100, 100) + : 0 // Impact calculations - const carKmEquivalent = Math.round(potentialCO2eq * 8000) + const carKmEquivalent = Math.round(rawCO2eq * 8000)And on line ~200:
- {Math.round( - potentialCO2eq * 75, - ).toLocaleString()} + {Math.round( + rawCO2eq * 75, + ).toLocaleString()}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/atlas-fields/carbon-sequestration.tsx` around lines 48 - 61, Right now currentOM, maxOM and potentialCO2eq are pre-rounded and later used to compute carKmEquivalent and the euro estimate, which causes compounding rounding errors; change computations so carKmEquivalent and the euro estimate are calculated from the raw API values (e.g., use carbonEstimates.b_c_delta * 3.67 for CO2eq and then *8000 for carKmEquivalent) and only apply Math.round / formatting at render time (keep currentOM/maxOM rounding for display only), updating references to potentialCO2eq, carKmEquivalent, and the euro-estimate variable so they use the unrounded intermediate values before presentation rounding.fdm-app/app/components/blocks/atlas-fields/cultivation-history.tsx (1)
1-1:HelpCircleis imported but unused in this component.
HelpCircleis imported on Line 1 but not referenced anywhere inCultivationHistoryCardorCultivationHistorySkeleton. It may be a leftover from development.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/atlas-fields/cultivation-history.tsx` at line 1, The import HelpCircle is unused; remove it from the import list at the top of the file (where ChevronDown, ChevronUp, Copy are imported) to clean up the module and avoid lint warnings; check the CultivationHistoryCard and CultivationHistorySkeleton components to ensure no remaining references to HelpCircle before deleting the import.fdm-app/app/routes/farm.$b_id_farm.$calendar.atlas.fields.$centroid.tsx (1)
274-281: Fixed floating button for "Terug naar kaart" is a nice UX addition.The
z-50ensures it overlays scrollable content. One thing to consider: on mobile with limited screen space, this floating button may overlap with content at the bottom of the page. A smallpb-20(or similar) on the<main>in the layout could prevent the last card from being obscured by the button.🤖 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.atlas.fields.$centroid.tsx around lines 274 - 281, The floating "Terug naar kaart" button (className "fixed bottom-6 right-6 z-50" inside the Button/NavLink) can obscure bottom content on small screens; add bottom padding to the page layout by applying a sufficient padding-bottom (e.g., pb-20) to the surrounding <main> element in the layout component so the last card isn’t hidden behind the fixed button—update the layout's main element (where this route renders) rather than the button itself.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/wicked-lies-lead.md:
- Line 5: Update the changeset text to list all primary additions from the PR:
mention the new CarbonSequestrationCard UI component, the new NMI API fields
b_c_st03, b_som_potential, b_c_st03_potential, and b_c_delta, the
layout/skeleton refactors, and the new calculation functions (referencing the
functions you added for carbon/sequestration calculations). Replace the
single-line description with a short bulleted or comma-separated summary
covering each item so release notes accurately reflect the PR surface.
In `@fdm-app/app/components/blocks/atlas-fields/cultivation-history.tsx`:
- Around line 158-206: The component renders cultivation.b_lu_name directly into
TooltipTrigger and TooltipContent which can be undefined; update both usages to
use a safe fallback (e.g., cultivation.b_lu_name ?? "Onbekend gewas") so the
tooltip trigger and content never render empty; locate the occurrences in this
file (the TooltipTrigger and TooltipContent blocks around cultivation.b_lu_name)
and replace the direct property access with the fallback expression (or
conditionally render a default string) to ensure a visible label even when
b_lu_name is undefined.
In `@fdm-app/app/components/blocks/atlas-fields/groundwater.tsx`:
- Around line 44-53: The unit label "cm-mv" renders even when b_gwl_ghg (and
similarly b_gwl_glg) is null because the check uses !== undefined; change the
conditional that controls the unit span to test for both null and undefined (use
!= null / loose null check) so the unit only renders when a real numeric value
exists for groundwaterEstimates.b_gwl_ghg and groundwaterEstimates.b_gwl_glg.
In `@fdm-app/app/components/blocks/atlas-fields/soil-texture.tsx`:
- Around line 78-86: The skeleton grid in the SoilTexture component is using a
fixed "grid-cols-3" which causes layout shift; update the div inside CardContent
that currently has className "grid grid-cols-3 gap-6" to match the real
content's responsive classes (e.g., "grid grid-cols-1 sm:grid-cols-3 gap-6") so
the Skeleton placeholders use the same responsive breakpoint behavior as the
actual grid.
In `@fdm-calculator/src/other/water-supply-by-som.ts`:
- Around line 33-151: The calculateWhc internal function and its callers in
calculateWaterSupplyBySom must guard against zero/negative inputs: clamp
a_som_loi, a_clay_mi and a_silt_mi to a small epsilon (e.g. 1e-6) at the start
of calculateWhc (and/or before computing normalizedClay/normalizedSilt) so
divisions and ln calls never receive 0 or negative values; update calculateWhc
signature usage and the normalizedClay/normalizedSilt computation to use the
clamped values (referencing calculateWaterSupplyBySom, calculateWhc,
normalizedClay, normalizedSilt, and params a_som_loi/a_clay_mi/a_silt_mi) and
ensure all places that divide by or ln() these values use the clamped variables.
---
Outside diff comments:
In `@fdm-app/app/components/blocks/atlas-fields/cultivation-history.tsx`:
- Around line 240-272: The skeleton's CardDescription text in the
CultivationHistorySkeleton component currently differs from the real card;
update the string inside the CardDescription in CultivationHistorySkeleton so it
exactly matches the live card text "Dit zijn de gewassen zoals geregistreerd in
de Basisregistratie Gewaspercelen (BRP)." to prevent a visible shift when
swapping skeleton -> loaded content; locate the CardDescription inside the
CultivationHistorySkeleton function and replace the current Dutch sentence with
the exact live-card sentence.
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.atlas.fields.$centroid.tsx:
- Around line 160-165: The soilParameterEstimates object being passed to
SoilTextureCard includes a non-supported key a_cn_fr which triggers a TypeScript
error; remove a_cn_fr from the soilParameterEstimates object so it only contains
a_clay_mi, a_silt_mi, and a_sand_mi (or alternatively, if you intend to display
a_cn_fr, add it to SoilTextureCard's prop type and component interface instead).
Locate the construction of soilParameterEstimates (the object with
a_clay_mi/a_silt_mi/a_sand_mi/a_cn_fr) and delete the a_cn_fr entry or update
SoilTextureCard's props to accept a_cn_fr and render it accordingly.
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.atlas.tsx:
- Around line 94-100: The Header component requires an explicit action prop
(typed as HeaderActionProps | undefined); update the JSX that renders <Header>
(the block containing <HeaderFarm ... /> and <HeaderAtlas ... />) to pass
action={undefined} (matching other usages) so TypeScript compiles without error.
---
Nitpick comments:
In `@fdm-app/app/components/blocks/atlas-fields/carbon-sequestration.tsx`:
- Around line 48-61: Right now currentOM, maxOM and potentialCO2eq are
pre-rounded and later used to compute carKmEquivalent and the euro estimate,
which causes compounding rounding errors; change computations so carKmEquivalent
and the euro estimate are calculated from the raw API values (e.g., use
carbonEstimates.b_c_delta * 3.67 for CO2eq and then *8000 for carKmEquivalent)
and only apply Math.round / formatting at render time (keep currentOM/maxOM
rounding for display only), updating references to potentialCO2eq,
carKmEquivalent, and the euro-estimate variable so they use the unrounded
intermediate values before presentation rounding.
In `@fdm-app/app/components/blocks/atlas-fields/cultivation-history.tsx`:
- Line 1: The import HelpCircle is unused; remove it from the import list at the
top of the file (where ChevronDown, ChevronUp, Copy are imported) to clean up
the module and avoid lint warnings; check the CultivationHistoryCard and
CultivationHistorySkeleton components to ensure no remaining references to
HelpCircle before deleting the import.
In `@fdm-app/app/components/blocks/atlas-fields/field-details.tsx`:
- Around line 22-28: regionLabels is a static object recreated on every render
inside the FieldDetails component; move the const regionLabels definition out to
module scope (above the FieldDetails component) so it’s allocated once, then
update any references within FieldDetails to use the hoisted regionLabels;
ensure the identifier stays the same and remove the in-component declaration to
avoid shadowing.
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.atlas.fields.$centroid.tsx:
- Around line 274-281: The floating "Terug naar kaart" button (className "fixed
bottom-6 right-6 z-50" inside the Button/NavLink) can obscure bottom content on
small screens; add bottom padding to the page layout by applying a sufficient
padding-bottom (e.g., pb-20) to the surrounding <main> element in the layout
component so the last card isn’t hidden behind the fixed button—update the
layout's main element (where this route renders) rather than the button itself.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
fdm-app/app/components/blocks/atlas-fields/carbon-sequestration.tsx (2)
48-56: Progress gauge uses rounded values, introducing minor inaccuracy.
percentageOfMaxis derived fromcurrentOMandmaxOM, which are already rounded to 1 decimal. This means the progress bar and percentage label can be slightly off compared to the raw API values. For a display-only gauge this is acceptable, but using raw values for the ratio would be more precise:Suggested improvement
- const percentageOfMax = - maxOM > 0 ? Math.min((currentOM / maxOM) * 100, 100) : 0 + const percentageOfMax = + carbonEstimates.b_som_potential > 0 + ? Math.min((carbonEstimates.a_som_loi / carbonEstimates.b_som_potential) * 100, 100) + : 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/atlas-fields/carbon-sequestration.tsx` around lines 48 - 56, The progress percentage is computed from the already-rounded values currentOM and maxOM causing small inaccuracies; change percentageOfMax to use the raw API values from carbonEstimates (e.g. use carbonEstimates.a_som_loi and carbonEstimates.b_som_potential directly, with a max check) to compute the ratio, then keep rounding only for display variables (currentOM / maxOM) so the gauge/percentage label uses the precise ratio while UI numbers remain rounded; update references to percentageOfMax accordingly.
1-12: Unused imports:Database,Target,Zap.These three icons are imported but never used in the component JSX. They should be removed to keep the new file clean.
Proposed fix
import { Leaf, Car, HelpCircle, ChevronDown, Droplets, Sprout, Euro, - Database, - Target, - Zap, } from "lucide-react"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/atlas-fields/carbon-sequestration.tsx` around lines 1 - 12, The import list in the CarbonSequestration component includes unused icons Database, Target, and Zap; remove those three symbols from the import statement (the line importing Leaf, Car, HelpCircle, ChevronDown, Droplets, Sprout, Euro, Database, Target, Zap from "lucide-react") so only the actually used icons remain (e.g., Leaf, Car, HelpCircle, ChevronDown, Droplets, Sprout, Euro), and run a quick lint/compile to confirm no other references to Database, Target, or Zap exist.fdm-app/app/components/blocks/atlas-fields/layout.tsx (1)
3-37: Clean layout composition with the newcarbonslot.The 12-column grid with sticky sidebar, responsive ordering, and
pb-20for the floating button clearance is well thought out.One note:
lg:row-span-10(line 25) is an arbitrarily large value to ensure the sidebar spans the full right-side content. This works with CSS Grid implicit rows but is brittle if more cards are added later. Consider documenting the intent with a comment, or alternatively, using a nested flex/grid layout for the two columns to avoid the magic number.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/atlas-fields/layout.tsx` around lines 3 - 37, The lg:row-span-10 on the cultivationHistory sidebar in FieldDetailsAtlasLayout is a brittle "magic number" used to force the sidebar to span the right-side content; replace this with a more robust layout or at least document the intent: either refactor the outer grid into a two-column container (e.g., a nested grid/flex pair where the left column is the main flow and the right column is a sticky sidebar so no arbitrary row-span is needed) or add an inline comment next to the lg:row-span-10 reference explaining why the value is required and when it should be revisited to prevent future regressions.
🤖 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/atlas-fields/carbon-sequestration.tsx`:
- Line 264: The skeleton's grid layout in the CarbonSequestration component is
not responsive: replace the hardcoded "grid-cols-3" on the skeleton container
with the same responsive classes used by the real card ("grid-cols-1
md:grid-cols-3") so the skeleton matches the real card's behavior; locate the
div with className containing "grid grid-cols-3 gap-6" inside the
CarbonSequestration component and update its className to "grid grid-cols-1
md:grid-cols-3 gap-6".
- Around line 74-88: The HelpCircle icon used as the TooltipTrigger is an SVG
and cannot receive keyboard focus; update the trigger so it's a focusable
element (e.g., wrap the HelpCircle in a <button> or render a button as the
TooltipTrigger child) to make the tooltip keyboard-accessible and screen-reader
friendly. Specifically, modify the TooltipTrigger/TooltipProvider block that
currently uses TooltipTrigger asChild with HelpCircle so the child is a
focusable button (or another focusable primitive) and add an appropriate
accessible name (aria-label or visually-hidden text) on that button; keep
Tooltip, TooltipContent and the existing tooltip text unchanged.
In `@fdm-app/app/components/blocks/atlas-fields/field-details.tsx`:
- Around line 104-113: The skeleton layout's responsive grid classes don't match
the real card; update the first skeleton container (inside CardContent with
className "space-y-6") to use grid classes "grid grid-cols-1 md:grid-cols-2
gap-4" instead of "grid-cols-2", and update the second skeleton container (the
one with "pt-0 sm:pt-4 border-t-0 sm:border-t") to use "grid grid-cols-1
sm:grid-cols-3 gap-4" instead of "grid-cols-3" so the skeleton matches the real
card's responsiveness.
- Around line 35-36: Replace the hardcoded "Status voor 2025" string in the
FieldDetails component with a year derived from a prop (e.g., calendar or
activeYear) so it stays current: add a prop to the FieldDetails component
signature (e.g., { calendar }: { calendar: number | string } or activeYear), use
that prop in the JSX (e.g., <p>{`Status voor ${calendar}`}</p>), and update any
parent/route that renders FieldDetails to pass the route calendar param into
this prop; ensure prop typing is added/adjusted in the component export or
interface.
In `@fdm-app/app/components/blocks/atlas-fields/groundwater.tsx`:
- Line 84: The skeleton grid uses a fixed three-column layout causing a layout
shift; update the skeleton's container (the div in the Groundwater component
rendering the skeleton) to match the real card's responsive classes by changing
its className from a fixed "grid grid-cols-3 gap-6" to the responsive "grid
grid-cols-1 sm:grid-cols-3 gap-6" so the skeleton and actual content share the
same breakpoints.
---
Nitpick comments:
In `@fdm-app/app/components/blocks/atlas-fields/carbon-sequestration.tsx`:
- Around line 48-56: The progress percentage is computed from the
already-rounded values currentOM and maxOM causing small inaccuracies; change
percentageOfMax to use the raw API values from carbonEstimates (e.g. use
carbonEstimates.a_som_loi and carbonEstimates.b_som_potential directly, with a
max check) to compute the ratio, then keep rounding only for display variables
(currentOM / maxOM) so the gauge/percentage label uses the precise ratio while
UI numbers remain rounded; update references to percentageOfMax accordingly.
- Around line 1-12: The import list in the CarbonSequestration component
includes unused icons Database, Target, and Zap; remove those three symbols from
the import statement (the line importing Leaf, Car, HelpCircle, ChevronDown,
Droplets, Sprout, Euro, Database, Target, Zap from "lucide-react") so only the
actually used icons remain (e.g., Leaf, Car, HelpCircle, ChevronDown, Droplets,
Sprout, Euro), and run a quick lint/compile to confirm no other references to
Database, Target, or Zap exist.
In `@fdm-app/app/components/blocks/atlas-fields/layout.tsx`:
- Around line 3-37: The lg:row-span-10 on the cultivationHistory sidebar in
FieldDetailsAtlasLayout is a brittle "magic number" used to force the sidebar to
span the right-side content; replace this with a more robust layout or at least
document the intent: either refactor the outer grid into a two-column container
(e.g., a nested grid/flex pair where the left column is the main flow and the
right column is a sticky sidebar so no arbitrary row-span is needed) or add an
inline comment next to the lg:row-span-10 reference explaining why the value is
required and when it should be revisited to prevent future regressions.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
fdm-app/app/components/blocks/atlas-fields/carbon-sequestration.tsx (1)
45-67: Extract domain constants and document their sources.The conversion factors
3.67(C→CO₂),8000(km/ton CO₂ for a car), and75(EUR/ton CO₂ via EU ETS) are hard-coded without documentation. Especially the75EUR value is a volatile market price that will become stale — consider making it a named constant with a comment noting the source/date, so it's easy to find and update.Suggested approach
+// Molecular weight ratio CO₂/C = 44/12 ≈ 3.67 +const C_TO_CO2_FACTOR = 3.67 +// Average EU passenger car ≈ 125 g CO₂/km → ~8 000 km per ton CO₂ +const CAR_KM_PER_TON_CO2 = 8000 +// EU ETS carbon price estimate (EUR/ton CO₂) — update periodically +const EUR_PER_TON_CO2 = 75 + export function CarbonSequestrationCard({ carbonEstimates }: { ... }) { ... - const rawCO2eq = carbonEstimates.b_c_delta * 3.67 + const rawCO2eq = carbonEstimates.b_c_delta * C_TO_CO2_FACTOR ... - const carKmEquivalent = Math.round(rawCO2eq * 8000) + const carKmEquivalent = Math.round(rawCO2eq * CAR_KM_PER_TON_CO2) ... - const euroEstimate = Math.round(rawCO2eq * 75) + const euroEstimate = Math.round(rawCO2eq * EUR_PER_TON_CO2)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/atlas-fields/carbon-sequestration.tsx` around lines 45 - 67, Extract the magic numbers into named constants (e.g., const C_TO_CO2 = 3.67, CAR_KM_PER_TON_CO2 = 8000, EUR_PER_TON_CO2 = 75) and replace usages in rawCO2eq, carKmEquivalent, and euroEstimate so the calculations reference these constants; add inline comments for each constant indicating its source and date (e.g., "C→CO2 conversion factor (atomic mass ratio) — source: ... , retrieved YYYY-MM-DD", "approx. car km per ton CO2 — source: ...", "EUR/ton CO2 (EU ETS spot or representative price) — source and date, note that this is volatile and may be configured elsewhere"); consider exporting EUR_PER_TON_CO2 to a config or constants module if it may change at runtime or require easy updates.
🤖 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/atlas-fields/carbon-sequestration.tsx`:
- Around line 149-161: The displayed plus sign with negative values comes from
rendering "+{potentialCO2eq}" (and similarly "~{carKmEquivalent}" and
"~{euroEstimate}") while b_c_delta can make these values ≤ 0; update the
rendering to either clamp the numeric values to zero (e.g., use Math.max(0,
potentialCO2eq), Math.max(0, carKmEquivalent), Math.max(0, euroEstimate)) or
compute a formatted string that conditionally prepends the "+" only for positive
numbers and shows "0" (or no sign) for non-positive values; adjust the JSX in
the CarbonSequestration component where potentialCO2eq, carKmEquivalent, and
euroEstimate are output to use the chosen clamping/formatting function so you
never render artifacts like "+-0.5" or "~ € -75".
---
Nitpick comments:
In `@fdm-app/app/components/blocks/atlas-fields/carbon-sequestration.tsx`:
- Around line 45-67: Extract the magic numbers into named constants (e.g., const
C_TO_CO2 = 3.67, CAR_KM_PER_TON_CO2 = 8000, EUR_PER_TON_CO2 = 75) and replace
usages in rawCO2eq, carKmEquivalent, and euroEstimate so the calculations
reference these constants; add inline comments for each constant indicating its
source and date (e.g., "C→CO2 conversion factor (atomic mass ratio) — source:
... , retrieved YYYY-MM-DD", "approx. car km per ton CO2 — source: ...",
"EUR/ton CO2 (EU ETS spot or representative price) — source and date, note that
this is volatile and may be configured elsewhere"); consider exporting
EUR_PER_TON_CO2 to a config or constants module if it may change at runtime or
require easy updates.
Comment on lines
+149
to
+161
| <div className="space-y-1"> | ||
| <p className="text-xs font-medium text-muted-foreground uppercase flex items-center gap-1"> | ||
| CO₂-vastlegging | ||
| </p> | ||
| <div className="flex items-baseline gap-1"> | ||
| <span className="text-xl font-bold"> | ||
| +{potentialCO2eq} | ||
| </span> | ||
| <span className="text-xs text-muted-foreground"> | ||
| ton CO₂eq/ha | ||
| </span> | ||
| </div> | ||
| </div> |
Contributor
There was a problem hiding this comment.
Negative b_c_delta produces "+-" display artifacts.
If b_c_delta is ≤ 0 (current carbon stock already at or above potential), potentialCO2eq becomes negative, and Line 155 renders +{potentialCO2eq} → e.g. +-0.5 ton CO₂eq/ha. The same issue cascades to carKmEquivalent (Line 177: ~-800 km) and euroEstimate (Line 211: ~€ -75).
Consider either clamping to zero or conditionally formatting the sign:
Suggested fix (clamping approach)
- const rawCO2eq = carbonEstimates.b_c_delta * 3.67
+ const rawCO2eq = Math.max(carbonEstimates.b_c_delta, 0) * 3.67🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fdm-app/app/components/blocks/atlas-fields/carbon-sequestration.tsx` around
lines 149 - 161, The displayed plus sign with negative values comes from
rendering "+{potentialCO2eq}" (and similarly "~{carKmEquivalent}" and
"~{euroEstimate}") while b_c_delta can make these values ≤ 0; update the
rendering to either clamp the numeric values to zero (e.g., use Math.max(0,
potentialCO2eq), Math.max(0, carKmEquivalent), Math.max(0, euroEstimate)) or
compute a formatted string that conditionally prepends the "+" only for positive
numbers and shows "0" (or no sign) for non-positive values; adjust the JSX in
the CarbonSequestration component where potentialCO2eq, carKmEquivalent, and
euroEstimate are output to use the chosen clamping/formatting function so you
never render artifacts like "+-0.5" or "~ € -75".
gerardhros
reviewed
Feb 18, 2026
gerardhros
reviewed
Feb 18, 2026
gerardhros
reviewed
Feb 18, 2026
gerardhros
reviewed
Feb 18, 2026
gerardhros
approved these changes
Feb 18, 2026
Collaborator
gerardhros
left a comment
gerardhros
left a comment
There was a problem hiding this comment.
Nice feature. it might be connected to the C-balance later so that there you directly see what and how the C-balance contribute to the roadmap to SOM limits. A few small questions left.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Closes #464