Add per-field nitrogen balance, error capture, aggregation, UI updates#295
Conversation
🦋 Changeset detectedLatest commit: 12ff3a0 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 |
|
Caution Review failedThe pull request is closed. WalkthroughCalculator now returns per-field results (balance or errorMessage); farm aggregation skips failed fields and records hasErrors/fieldErrorMessages. Routes/loaders return Suspense-friendly promises and consume fieldResult shapes to render partial farm results and field-specific error UI; single-field loading queries a single field. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Route as Farm Nitrogen Page (_index.tsx)
participant Loader as Loader
participant Calc as calculateNitrogenBalance
participant Field as calculateNitrogenBalanceField
participant Agg as Aggregate-to-Farm
User->>Route: Open /farm/{farmId}/{calendar}/balance/nitrogen
Route->>Loader: loader()
Loader->>Calc: calculateNitrogenBalance(input)
loop per field
Calc->>Field: compute field
alt success
Field-->>Calc: { b_id, b_area, balance }
else failure
Field-->>Calc: { b_id, errorMessage }
end
end
Calc->>Agg: aggregate(successful field results)
Agg-->>Loader: { fields: fieldResults[], hasErrors, fieldErrorMessages, totals }
Loader-->>Route: nitrogenBalanceResult (promise)
alt hasErrors
Route->>Route: show farm totals + warning and inline field errors
else
Route->>Route: render normal balances
end
sequenceDiagram
autonumber
actor User
participant Route as Field Nitrogen Page ($b_id.tsx)
participant Loader as Loader
participant Input as collectInputForNitrogenBalance(b_id)
participant Calc as calculateNitrogenBalance
User->>Route: Open /farm/{farmId}/{calendar}/balance/nitrogen/{b_id}
Route->>Loader: loader({ b_id })
Loader->>Input: collectInput(..., b_id)
Input-->>Loader: input with single field
Loader->>Calc: calculateNitrogenBalance(single-field input)
alt field success
Calc-->>Loader: fieldResult with balance
Loader-->>Route: fieldResult
Route->>Route: render field details
else field failure
Calc-->>Loader: fieldResult with errorMessage + errorId
Loader-->>Route: fieldResult
Route->>Route: show field-specific error UI + link to settings
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development #295 +/- ##
============================================
Coverage 93.07% 93.07%
============================================
Files 81 81
Lines 13118 13176 +58
Branches 1326 1339 +13
============================================
+ Hits 12209 12264 +55
- Misses 907 910 +3
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
coderabbitai
Bot
added
branch:development
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen.$b_id.tsx (2)
88-90: Handle “field not found” with 404If getField returns null/undefined, the route will later crash when accessing field.b_id/b_name. Add a 404 similar to the farm check.
Apply:
- const field = await getField(fdm, session.principal_id, b_id) + const field = await getField(fdm, session.principal_id, b_id) + if (!field) { + throw data("not found: b_id", { + status: 404, + statusText: "not found: b_id", + }) + }
260-368: EnsurefieldInputprop matches FieldInput type
You’re passingfieldResult.balance(NitrogenBalanceNumeric) tofieldInput, but that prop expects the originalFieldInputobject. Swap in the proper FieldInput (e.g. the variable holding the raw field input) instead offieldResult.balance.fdm-calculator/src/balance/nitrogen/types.d.ts (2)
548-551: Duplicate "mineral" key in NitrogenEmissionAmmoniaFertilizersNumericThis duplicates the mineral section and can confuse tooling/readers. Remove the duplicate.
Apply this diff:
- mineral: { - total: number - applications: { id: string; value: number }[] - }
576-581: NitrogenEmissionNumeric missing nitrate; inconsistent with Decimal typeNitrogenEmission includes nitrate, but the numeric counterpart omits it. Add nitrate to keep parity.
Apply this diff:
export type NitrogenEmissionNumeric = { total: number ammonia: NitrogenEmissionAmmoniaNumeric + nitrate: { total: number } }fdm-calculator/src/balance/nitrogen/index.ts (1)
315-319: Use subtraction for farm-level average balance
Farm-level average balance must mirror the field-level formula: subtract removal and emission, not add.- const avgFarmBalance = avgFarmSupply - .add(avgFarmRemoval) - .add(avgFarmEmission) + const avgFarmBalance = avgFarmSupply + .minus(avgFarmRemoval) + .minus(avgFarmEmission)Verify domain intent matches this calculation.
🧹 Nitpick comments (12)
.changeset/pink-pants-leave.md (1)
5-5: Good detail; optional brevityConsider splitting into short bullets for readability in the changelog. Content is accurate.
.changeset/gentle-actors-teach.md (1)
5-5: Grammar nit“If a field has an error during the nitrogen balance calculation...” reads a bit better than “In case a field has an error...”.
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen.$b_id.tsx (4)
91-99: Returning a Promise directly from loader data — use defer or confirm supportYou’re returning a nested Promise (nitrogenBalancePromise) without using defer. This is fine only if your SSR/runtime is configured to support React Router v7 streaming with raw Promises + React 19 use(). Otherwise, prefer defer for portability.
Option:
-import { - data, - type LoaderFunctionArgs, - type MetaFunction, - NavLink, - useLoaderData, - useLocation, -} from "react-router" +import { + data, + defer, + type LoaderFunctionArgs, + type MetaFunction, + NavLink, + useLoaderData, + useLocation, +} from "react-router" - return { - nitrogenBalanceResult: nitrogenBalancePromise, - field: field, - farm: farm, - } + return defer({ + nitrogenBalanceResult: nitrogenBalancePromise, + field, + farm, + })Also verify you’re on React 19 (or equivalent support for use()) and React Router v7 streaming.
100-105: Variable shadowing inside find()The callback parameter name field shadows the outer field variable. Rename the inner param for clarity.
-const fieldResult = nitrogenBalanceResult.fields.find( - (field: { b_id: string }) => field.b_id === b_id, -) +const fieldResult = nitrogenBalanceResult.fields.find( + (f: { b_id: string }) => f.b_id === b_id, +)
165-226: Prefer error codes over regex parsing of message textMatching on human-readable error strings is brittle. Emit structured error codes (e.g., MISSING_SOIL_PARAMS with detail flags like a_n_rt, b_soiltype_agr, a_c_of) and branch on those codes.
45-46: Minor meta title spacingAdd a space before the last pipe for consistency.
- title: `Stikstof | Perceel | Nutriëntenbalans| ${clientConfig.name}`, + title: `Stikstof | Perceel | Nutriëntenbalans | ${clientConfig.name}`,fdm-calculator/src/balance/nitrogen/input.ts (1)
19-35: Update JSDoc for new optional b_id parameterThe function signature now includes b_id but the JSDoc lacks this param description.
Add:
* @param timeframe - The timeframe for which to collect the data. +* @param b_id - Optional field id to collect input for a single field only.Also applies to: 41-42
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx (2)
170-179: Accessibility: add an accessible label to the error tooltip triggerThe icon alone isn’t announced by screen readers. Add aria-label/title.
Apply this diff:
- <Tooltip> - <TooltipTrigger> + <Tooltip> + <TooltipTrigger aria-label="Niet alle percelen konden worden berekend" title="Niet alle percelen konden worden berekend"> <CircleAlert className="text-orange-500 bg-orange-100 rounded-full" /> </TooltipTrigger>
95-97: Minor: avoid fetching fields twice (perf micro-optimization)Loader fetches fields for the list, and collectInputForNitrogenBalance fetches them again for calculation. If possible, consider allowing collectInputForNitrogenBalance to accept a pre-fetched field list to avoid the duplicate DB call.
fdm-calculator/src/balance/nitrogen/index.ts (3)
301-314: Minor: spacing/typos and clarity in aggregationNo functional change, but consider cleaning stray spaces and clarifying hasErrors logic.
Apply this diff:
- let totalFarmArea = new Decimal(0) + let totalFarmArea = new Decimal(0) ... - totalFarmArea = totalFarmArea .add(fieldArea); + totalFarmArea = totalFarmArea.add(fieldArea) ... - const avgFarmSupply = totalFarmArea .isZero() + const avgFarmSupply = totalFarmArea.isZero() ? new Decimal(0) - : totalFarmSupply.dividedBy(totalFarmArea ) - const avgFarmRemoval = totalFarmArea .isZero() + : totalFarmSupply.dividedBy(totalFarmArea) + const avgFarmRemoval = totalFarmArea.isZero() ? new Decimal(0) - : totalFarmRemoval.dividedBy(totalFarmArea ) - const avgFarmEmission = totalFarmArea .isZero() + : totalFarmRemoval.dividedBy(totalFarmArea) + const avgFarmEmission = totalFarmArea.isZero() ? new Decimal(0) - : totalFarmEmission.dividedBy(totalFarmArea ) - const avgFarmTarget = totalFarmArea .isZero() + : totalFarmEmission.dividedBy(totalFarmArea) + const avgFarmTarget = totalFarmArea.isZero() ? new Decimal(0) - : totalFarmTarget.dividedBy(totalFarmArea ) + : totalFarmTarget.dividedBy(totalFarmArea) ... - hasErrors: hasErrors || (fieldsWithBalanceResults.length !== successfulFieldBalances.length), + hasErrors: + hasErrors || + fieldsWithBalanceResults.length !== successfulFieldBalances.length,Also applies to: 321-330
336-355: Avoid integer rounding during numeric conversionRounding all Decimals to integers in convertDecimalToNumberRecursive loses precision and can skew aggregates. Prefer raw toNumber or round at UI.
Apply this diff:
- if (data instanceof Decimal) { - return data.round().toNumber() - } + if (data instanceof Decimal) { + return data.toNumber() + }
57-63: Nit: fix comment typo "GeoTIIF"Small doc fix.
Apply this diff:
- // Fetch all deposition data in a single, batched request to avoid requesting the GeoTIIF for every field + // Fetch all deposition data in a single, batched request to avoid requesting the GeoTIFF for every field
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.changeset/cruel-bushes-care.md(1 hunks).changeset/gentle-actors-teach.md(1 hunks).changeset/pink-pants-leave.md(1 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen.$b_id.tsx(6 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx(8 hunks)fdm-calculator/src/balance/nitrogen/index.ts(7 hunks)fdm-calculator/src/balance/nitrogen/input.ts(2 hunks)fdm-calculator/src/balance/nitrogen/types.d.ts(3 hunks)fdm-calculator/src/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-01-31T15:05:14.310Z
Learnt from: SvenVw
PR: SvenVw/fdm#67
File: fdm-app/app/routes/farm.create.$b_id_farm.fields.$b_id.tsx:601-610
Timestamp: 2025-01-31T15:05:14.310Z
Learning: The `updateField` function in fdm-core has optional parameters after `fdm` and `b_id`. The TypeScript definitions might show 8 required parameters due to a potential version mismatch.
Applied to files:
fdm-calculator/src/balance/nitrogen/input.ts
📚 Learning: 2024-12-16T10:56:07.561Z
Learnt from: SvenVw
PR: SvenVw/fdm#16
File: fdm-app/app/routes/app.addfarm.$b_id_farm.cultivations.$b_lu_catalogue.fertilizers.tsx:1-1
Timestamp: 2024-12-16T10:56:07.561Z
Learning: The project uses `react-router` v7, and the `data` function is exported and used for error handling in loaders and actions.
Applied to files:
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx
📚 Learning: 2025-04-18T13:49:17.029Z
Learnt from: SvenVw
PR: SvenVw/fdm#124
File: fdm-app/app/components/custom/farm/farm-title.tsx:3-3
Timestamp: 2025-04-18T13:49:17.029Z
Learning: In the fdm project, NavLink and other routing components can be imported from either "react-router" or "react-router-dom" as react-router-dom is included in react-router.
Applied to files:
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx
🧬 Code graph analysis (5)
fdm-calculator/src/balance/nitrogen/types.d.ts (1)
fdm-calculator/src/index.ts (3)
NitrogenBalanceFieldResultNumeric(8-8)NitrogenBalanceFieldNumeric(7-7)NitrogenBalanceNumeric(6-6)
fdm-calculator/src/balance/nitrogen/input.ts (3)
fdm-core/src/index.ts (3)
FdmType(71-71)getField(98-98)getFields(99-99)fdm-core/src/db/schema.ts (1)
fieldsTypeSelect(105-105)fdm-calculator/src/balance/nitrogen/types.d.ts (1)
NitrogenBalanceInput(467-475)
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen.$b_id.tsx (1)
fdm-calculator/src/balance/nitrogen/index.ts (1)
calculateNitrogenBalance(39-118)
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx (3)
fdm-calculator/src/balance/nitrogen/index.ts (1)
calculateNitrogenBalance(39-118)fdm-app/app/store/field-filter.ts (1)
useFieldFilterStore(10-24)fdm-calculator/src/balance/nitrogen/types.d.ts (1)
NitrogenBalanceFieldResultNumeric(595-600)
fdm-calculator/src/balance/nitrogen/index.ts (4)
fdm-calculator/src/shared/public-data-url.ts (1)
getFdmPublicDataUrl(1-3)fdm-calculator/src/balance/nitrogen/supply/deposition.ts (1)
calculateAllFieldsNitrogenSupplyByDeposition(18-74)fdm-calculator/src/balance/nitrogen/types.d.ts (6)
NitrogenBalanceFieldResult(353-358)FieldInput(413-433)NitrogenBalance(364-397)NitrogenBalanceField(323-348)NitrogenBalanceNumeric(603-612)NitrogenBalanceFieldNumeric(583-590)fdm-calculator/src/balance/nitrogen/supply/index.ts (1)
calculateNitrogenSupply(29-79)
🔇 Additional comments (4)
.changeset/cruel-bushes-care.md (1)
5-5: LGTMClear, concise summary of the per-field capability. No action needed.
fdm-calculator/src/index.ts (1)
7-8: New public types exported — verified Both NitrogenBalanceFieldNumeric and NitrogenBalanceFieldResultNumeric are defined in balance/nitrogen/types and no references to NitrogenVolatilizationNumeric were found.fdm-calculator/src/balance/nitrogen/types.d.ts (1)
350-358: FieldResult types addition looks goodNew NitrogenBalanceFieldResult and its numeric counterpart align with per-field error handling goals.
Also applies to: 592-600
fdm-calculator/src/balance/nitrogen/index.ts (1)
221-229: Balance formula is correct – removal and emission values are negative
removal.total and emission.ammonia.total are defined as negative losses, so supply.total.add(removal.total).add(emission.ammonia.total) correctly computes supply − |removal| − |emission|.Likely an incorrect or invalid review comment.
coderabbitai
Bot
changed the title
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-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx (1)
171-211: Remove unreachable UI branch forresolvedNitrogenBalanceResult.errorMessageSame mismatch as loader: that property doesn’t exist. The branch is unreachable noise.
- if (resolvedNitrogenBalanceResult.errorMessage) { - /* big card with JSON */ - } + // Partial errors are indicated in the header via hasErrors tooltip.
🧹 Nitpick comments (9)
fdm-calculator/src/balance/nitrogen/input.ts (2)
41-60: Good single-field path + null-guard; add param docsOptional b_id + getField() guard looks correct. Please update JSDoc to document the new parameter.
* @param timeframe - The timeframe for which to collect the data. + * @param b_id - Optional field id: when provided, only collect data for this field. * @returns A promise that resolves with a `NitrogenBalanceInput` object containing all the necessary data.
63-118: Potential concurrency inside a single DB transactionPromise.all over fields (and inner per‑cultivation fetches) runs many queries concurrently on the same tx handle. Some drivers disallow concurrent queries per connection, causing “query in progress” errors under load.
- If FdmType uses a single connection per tx, consider batching (p-limit) or running field loops sequentially inside the transaction.
- Alternatively, move per-field parallelism outside the transaction, keeping tx sections short.
Would you like a p-limit based patch?
fdm-calculator/src/balance/nitrogen/index.test.ts (1)
132-136: Tests align with per-field error propagationAsserting hasErrors and fieldErrorMessages is correct.
Consider also asserting that the field-level result contains an error for tighter coverage:
- expect(result.fieldErrorMessages.length).toBeGreaterThan(0) + expect(result.fieldErrorMessages.length).toBeGreaterThan(0) + expect(result.fields[0]?.errorMessage).toBeDefined()fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen.$b_id.tsx (2)
112-134: Report structured error; capture as Error and include request pathYou’re reporting a string to Sentry. Wrap as Error and include the real request path for diagnostics.
- const errorId = reportError( - fieldResult.errorMessage, + const errorId = reportError( + new Error(fieldResult.errorMessage), { - page: "farm/{b_id_farm}/{calendar}/balance/nitrogen/{b_id}", + route: "farm/:b_id_farm/:calendar/balance/nitrogen/:b_id", scope: "loader", }, { + path: new URL(request.url).pathname, b_id, b_id_farm, timeframe, fieldArea: field?.b_area, userId: session.principal_id, }, )
186-222: Avoid parsing error strings in UI; prefer structured error codes/fieldsRegex over errorMessage is brittle. Prefer calculator returning a code (e.g., "MISSING_SOIL_PARAMS") and a list of missing parameters to render labels deterministically.
If agreeable, I can propose a small schema: { errorCode: "MISSING_SOIL_PARAMS", missing: ["a_n_rt","b_soiltype_agr","a_c_of|a_som_loi"] } and wire the UI accordingly.
fdm-calculator/src/balance/nitrogen/types.d.ts (1)
572-577: Numeric emission nitrate field added; verify producersNitrogenEmissionNumeric now includes nitrate.total. Ensure all producers (conversion and calculator) set this consistently to avoid undefined at runtime.
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx (1)
355-392: Per-field status/UI looks correct; optional UX improvementThe “Bekijk foutmelding” link is helpful. Consider a tooltip with the first error message for quick scanning without navigation.
fdm-calculator/src/balance/nitrogen/index.ts (2)
162-241: Consider more robust error message extraction.The try-catch structure properly captures field-level errors and returns them as
NitrogenBalanceFieldResult. However, line 240's error message extraction could be more robust:Current approach:
String(error).replace("Error: ", "")This relies on string manipulation and assumes a specific error format.
Apply this diff for more reliable error message extraction:
} catch (error) { return { b_id: field.b_id, b_area: field.b_area ?? 0, - errorMessage: String(error).replace("Error: ", ""), + errorMessage: error instanceof Error ? error.message : String(error), } }
367-390: LGTM! Minor optimization opportunity.The conversion correctly handles both successful and failed field results, preserving
errorMessagefor failed fields. The explicit field mapping (lines 372-387) ensures the correct structure forNitrogenBalanceFieldResultNumeric.Optional optimization: The recursive conversion at lines 367-369 converts
balance.fields, then line 372 overwrites it with a new mapping. While this works correctly, it performs unnecessary work convertingbalance.fieldstwice. Consider excludingfieldsfrom the initial recursive conversion or restructuring to avoid the duplication. This is a minor performance consideration and not critical.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
fdm-app/app/lib/error.tsis excluded by!fdm-app/app/lib/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
.changeset/brown-trees-sin.md(1 hunks).changeset/crazy-poets-juggle.md(1 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen.$b_id.tsx(7 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx(9 hunks)fdm-app/package.json(1 hunks)fdm-calculator/src/balance/nitrogen/index.test.ts(1 hunks)fdm-calculator/src/balance/nitrogen/index.ts(7 hunks)fdm-calculator/src/balance/nitrogen/input.ts(2 hunks)fdm-calculator/src/balance/nitrogen/types.d.ts(4 hunks)
✅ Files skipped from review due to trivial changes (2)
- .changeset/crazy-poets-juggle.md
- .changeset/brown-trees-sin.md
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-12-16T10:56:07.561Z
Learnt from: SvenVw
PR: SvenVw/fdm#16
File: fdm-app/app/routes/app.addfarm.$b_id_farm.cultivations.$b_lu_catalogue.fertilizers.tsx:1-1
Timestamp: 2024-12-16T10:56:07.561Z
Learning: The project uses `react-router` v7, and the `data` function is exported and used for error handling in loaders and actions.
Applied to files:
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx
📚 Learning: 2025-04-18T13:49:17.029Z
Learnt from: SvenVw
PR: SvenVw/fdm#124
File: fdm-app/app/components/custom/farm/farm-title.tsx:3-3
Timestamp: 2025-04-18T13:49:17.029Z
Learning: In the fdm project, NavLink and other routing components can be imported from either "react-router" or "react-router-dom" as react-router-dom is included in react-router.
Applied to files:
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx
📚 Learning: 2025-01-31T15:05:14.310Z
Learnt from: SvenVw
PR: SvenVw/fdm#67
File: fdm-app/app/routes/farm.create.$b_id_farm.fields.$b_id.tsx:601-610
Timestamp: 2025-01-31T15:05:14.310Z
Learning: The `updateField` function in fdm-core has optional parameters after `fdm` and `b_id`. The TypeScript definitions might show 8 required parameters due to a potential version mismatch.
Applied to files:
fdm-calculator/src/balance/nitrogen/input.ts
🧬 Code graph analysis (6)
fdm-calculator/src/balance/nitrogen/index.ts (4)
fdm-calculator/src/shared/public-data-url.ts (1)
getFdmPublicDataUrl(1-3)fdm-calculator/src/balance/nitrogen/supply/deposition.ts (1)
calculateAllFieldsNitrogenSupplyByDeposition(18-74)fdm-calculator/src/balance/nitrogen/types.d.ts (6)
NitrogenBalanceFieldResult(353-358)FieldInput(413-433)NitrogenBalance(364-397)NitrogenBalanceField(323-348)NitrogenBalanceNumeric(600-609)NitrogenBalanceFieldNumeric(580-587)fdm-calculator/src/balance/nitrogen/supply/index.ts (1)
calculateNitrogenSupply(29-79)
fdm-calculator/src/balance/nitrogen/index.test.ts (1)
fdm-calculator/src/balance/nitrogen/index.ts (1)
calculateNitrogenBalance(39-124)
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen.$b_id.tsx (7)
fdm-app/app/lib/auth.server.ts (1)
getSession(55-92)fdm-app/app/lib/calendar.ts (1)
getTimeframe(13-37)fdm-core/src/farm.ts (1)
getFarm(88-153)fdm-core/src/field.ts (1)
getField(143-199)fdm-calculator/src/balance/nitrogen/input.ts (1)
collectInputForNitrogenBalance(36-149)fdm-calculator/src/balance/nitrogen/index.ts (1)
calculateNitrogenBalance(39-124)fdm-app/app/lib/error.ts (2)
reportError(12-37)handleLoaderError(39-149)
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx (8)
fdm-app/app/lib/auth.server.ts (1)
getSession(55-92)fdm-core/src/farm.ts (1)
getFarm(88-153)fdm-core/src/field.ts (1)
getFields(216-313)fdm-calculator/src/balance/nitrogen/input.ts (1)
collectInputForNitrogenBalance(36-149)fdm-calculator/src/balance/nitrogen/index.ts (1)
calculateNitrogenBalance(39-124)fdm-app/app/lib/error.ts (2)
reportError(12-37)handleLoaderError(39-149)fdm-calculator/src/balance/nitrogen/types.d.ts (1)
NitrogenBalanceFieldResultNumeric(592-597)fdm-app/app/components/ui/tooltip.tsx (3)
Tooltip(30-30)TooltipTrigger(30-30)TooltipContent(30-30)
fdm-calculator/src/balance/nitrogen/types.d.ts (1)
fdm-calculator/src/index.ts (3)
NitrogenBalanceFieldResultNumeric(8-8)NitrogenBalanceFieldNumeric(7-7)NitrogenBalanceNumeric(6-6)
fdm-calculator/src/balance/nitrogen/input.ts (3)
fdm-core/src/index.ts (4)
fdmSchema(17-17)FdmType(71-71)getField(98-98)getFields(99-99)fdm-core/src/db/schema.ts (1)
fieldsTypeSelect(105-105)fdm-calculator/src/balance/nitrogen/types.d.ts (1)
NitrogenBalanceInput(467-475)
🔇 Additional comments (6)
fdm-calculator/src/balance/nitrogen/types.d.ts (1)
350-358: Types for field-level results look goodNitrogenBalanceFieldResult captures the success/error shape cleanly.
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx (1)
240-251: Good incomplete-totals indicator via tooltipShowing a tooltip when hasErrors is true meets the “incomplete totals” acceptance criterion.
fdm-calculator/src/balance/nitrogen/index.ts (4)
21-22: LGTM!The new type imports support the per-field error handling structure. These are essential for the changes that follow.
42-63: LGTM!The setup is well-organized:
- Maps provide O(1) lookup for fertilizer and cultivation details.
- Single batched deposition fetch optimizes GeoTIFF access.
- Clear and maintainable structure.
114-123: LGTM!The error parameters (
hasErrorsandfieldErrorMessages) are correctly propagated to the farm-level aggregation, then converted to numeric format for the final result. This aligns with the PR objective to display incomplete totals and field-specific errors.
254-338: LGTM!The aggregation logic correctly:
- Filters out failed fields for farm-level calculations (lines 267-269).
- Computes weighted averages using only successful fields (lines 278-302).
- Includes all field results (successful and failed) in the output (line 330).
- Propagates error flags and messages to the farm level (lines 331-335).
This aligns perfectly with the PR objective: successfully calculated fields render, and the farm-level summary indicates incomplete totals.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx (1)
164-199: Dead code: Top-levelerrorMessagecheck never executes.
calculateNitrogenBalancereturnsNitrogenBalanceNumericwithhasErrorsandfieldErrorMessages, not a top-levelerrorMessage. This branch is unreachable; farm-level errors throw and are caught byhandleLoaderErrorat line 126.Remove this dead code:
- if (resolvedNitrogenBalanceResult.errorMessage) { - return ( - <div className="flex items-center justify-center"> - <Card className="w-[350px]"> - <CardHeader> - <CardTitle> - Helaas is het niet mogelijk om je balans uit te - rekenen - </CardTitle> - </CardHeader> - <CardContent> - <div className="text-muted-foreground"> - <p> - Er is helaas wat misgegaan. Probeer opnieuw of - neem contact op met Ondersteuning en deel de - volgende foutmelding: - </p> - <div className="mt-8 w-full max-w-2xl"> - <pre className="bg-gray-200 dark:bg-gray-800 p-4 rounded-md overflow-x-auto text-sm text-gray-800 dark:text-gray-200"> - {JSON.stringify( - { - message: - resolvedNitrogenBalanceResult.errorMessage, - timestamp: new Date(), - }, - null, - 2, - )} - </pre> - </div> - </div> - </CardContent> - </Card> - </div> - ) - } -
🧹 Nitpick comments (1)
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx (1)
100-113: Consider summarizing long error messages.When many fields fail,
fieldErrorMessages.join(",\n")could produce a very long error message in Sentry. Consider including a summary (e.g., "X out of Y fields failed") along with a limited sample of errors.Example:
if (nitrogenBalanceResult.hasErrors) { + const errorSummary = `${nitrogenBalanceResult.fieldErrorMessages.length} field(s) failed calculation` + const sampleErrors = nitrogenBalanceResult.fieldErrorMessages.slice(0, 5) reportError( - nitrogenBalanceResult.fieldErrorMessages.join(",\n"), + `${errorSummary}\n${sampleErrors.join(",\n")}${nitrogenBalanceResult.fieldErrorMessages.length > 5 ? "\n..." : ""}`, { page: "farm/{b_id_farm}/{calendar}/balance/nitrogen/_index", scope: "loader", }, { b_id_farm, timeframe, userId: session.principal_id, + totalFieldErrors: nitrogenBalanceResult.fieldErrorMessages.length, }, ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx(9 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-12-16T10:56:07.561Z
Learnt from: SvenVw
PR: SvenVw/fdm#16
File: fdm-app/app/routes/app.addfarm.$b_id_farm.cultivations.$b_lu_catalogue.fertilizers.tsx:1-1
Timestamp: 2024-12-16T10:56:07.561Z
Learning: The project uses `react-router` v7, and the `data` function is exported and used for error handling in loaders and actions.
Applied to files:
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx
📚 Learning: 2025-04-18T13:49:17.029Z
Learnt from: SvenVw
PR: SvenVw/fdm#124
File: fdm-app/app/components/custom/farm/farm-title.tsx:3-3
Timestamp: 2025-04-18T13:49:17.029Z
Learning: In the fdm project, NavLink and other routing components can be imported from either "react-router" or "react-router-dom" as react-router-dom is included in react-router.
Applied to files:
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx
🧬 Code graph analysis (1)
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx (10)
fdm-app/app/lib/auth.server.ts (1)
getSession(55-92)fdm-app/app/lib/calendar.ts (1)
getTimeframe(13-37)fdm-core/src/farm.ts (1)
getFarm(88-153)fdm-core/src/field.ts (1)
getFields(216-313)fdm-calculator/src/balance/nitrogen/input.ts (1)
collectInputForNitrogenBalance(36-149)fdm-calculator/src/balance/nitrogen/index.ts (1)
calculateNitrogenBalance(39-119)fdm-app/app/lib/error.ts (2)
reportError(12-37)handleLoaderError(39-149)fdm-app/app/store/field-filter.ts (1)
useFieldFilterStore(10-24)fdm-calculator/src/balance/nitrogen/types.d.ts (1)
NitrogenBalanceFieldResultNumeric(592-597)fdm-app/app/components/ui/tooltip.tsx (3)
Tooltip(30-30)TooltipTrigger(30-30)TooltipContent(30-30)
🔇 Additional comments (4)
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen._index.tsx (4)
1-44: LGTM: Imports support new error handling features.All new imports (error utilities, tooltip components, CircleX icon) are properly utilized in the updated error handling and UI.
88-118: Verify error handling for async function.Errors thrown inside
asyncData(e.g., fromcollectInputForNitrogenBalance) will not be caught by the loader's try-catch at line 125. They will reject the Promise and be handled by React's error boundary whenuse()is called at line 159.Confirm this is the intended behavior. If farm-level calculation errors should be handled more gracefully (showing the farm/field layout with an error message), wrap the async function content in try-catch and return an error state.
Example approach if graceful handling is desired:
const asyncData = (async () => { try { const nitrogenBalanceInput = await collectInputForNitrogenBalance(...) const nitrogenBalanceResult = await calculateNitrogenBalance(nitrogenBalanceInput) if (nitrogenBalanceResult.hasErrors) { reportError(...) } return { nitrogenBalanceResult, error: null } } catch (error) { const errorId = reportError(error, { /* ... */ }) return { nitrogenBalanceResult: null, error: { message: String(error), errorId } } } })()Based on learnings (React Router v7 uses
datafunction for error handling)
228-243: Excellent per-field error handling implementation.The UI correctly handles partial results with clear error indicators:
- Farm-level summary shows warning tooltip when some fields failed (lines 228-237)
- Per-field list displays CircleAlert icon for failed fields (lines 343-352)
- Failed fields show "Bekijk foutmelding" link instead of balance/target (lines 367-379)
This aligns perfectly with the PR objectives for graceful degradation and clear error communication.
Also applies to: 343-379
204-210: LGTM: Field filtering handles edge cases safely.The filtering logic correctly:
- Uses Map for O(1) field lookups
- Handles missing field data with safe fallback
- Respects user preference for productive fields only
| total: number | ||
| ammonia: NitrogenEmissionAmmoniaNumeric | ||
| nitrate: { total: number } | ||
| } |
There was a problem hiding this comment.
why {} withhin the {} for nitrate, being different for ammonia?
There was a problem hiding this comment.
It is a placeholder, as nitrate is not implemented yet
| throw new Error(`Field not found: ${String(b_id)}`) | ||
| } | ||
| farmFields = [field] | ||
| } else { |
There was a problem hiding this comment.
is this farmFields = [field] a subset, and does that work with any "b_id"?
There was a problem hiding this comment.
Yes, for all b_id that are present for that farm. Otherwise an error is thrown
gerardhros
left a comment
gerardhros
left a comment
There was a problem hiding this comment.
LGTM. 2 minor questions.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Closes #294