Update nitrogen balance with fieldInput data#303
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## development #303 +/- ##
============================================
Coverage 93.12% 93.12%
============================================
Files 81 81
Lines 13314 13314
Branches 1360 1360
============================================
Hits 12398 12398
Misses 914 914
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:
|
|
Caution Review failedThe pull request is closed. WalkthroughLoader now locates and returns the field-specific Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser
participant Loader
participant NitrogenBalance
participant NitrogenBalanceDetails
Browser->>Loader: request /farm/.../balance/nitrogen/:b_id
Loader-->>Browser: { fieldResult, fieldInput, ... }
Browser->>NitrogenBalance: render with loader data
NitrogenBalance->>NitrogenBalanceDetails: props(balance=fieldResult.balance, fieldInput=fieldInput)
NitrogenBalanceDetails-->>Browser: render details
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen.$b_id.tsx (1)
136-139: Return only the current field’s input instead of the full dataset.Reducing the payload and coupling here will avoid accidental mismatches later and makes the consumer simpler. Compute the per‑field input in the loader and return just that.
Apply this focused change inside the existing
then:- return { - fieldResult: fieldResult, - fieldInput: input, - } + const fieldInputForField = + input.fields?.find((f: any) => (f.b_id ?? f?.meta?.b_id) === b_id) ?? null + return { + fieldResult, + fieldInput: fieldInputForField, + }Please confirm the correct identifier path on items in
input.fields(e.g.,f.b_idvsf.meta.b_id) and adjust the predicate accordingly.
📜 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.$b_id.tsx(3 hunks)
🔇 Additional comments (2)
fdm-app/app/routes/farm.$b_id_farm.$calendar.balance.nitrogen.$b_id.tsx (2)
181-181: LGTM: correct Suspense + use() pattern.Destructuring the resolved promise from
use(nitrogenBalanceResult)is idiomatic in React 19 and fits the router’s deferred pattern.
391-393: The review comment is valid. Index-based field selection is brittle.Looking at the loader (lines 55–138),
collectInputForNitrogenBalance()returns aninputobject with afieldsarray that may contain multiple fields. The loader receives a specificb_idfrom route params and already correctly filters the nitrogen balance result to find the matching field (lines 113–117):let fieldResult = nitrogenBalanceResult.fields.find( (field: { b_id: string }) => field.b_id === b_id, )However, at line 392, the code passes
fieldInput.fields[0]unconditionally, assuming the first field is correct. This breaks if:
- The fields array ordering changes
- Multiple fields are present and the first isn't the one being displayed
- The array is empty
The loader has access to both
b_id(params) and the matchedfieldobject (line 75). The fix should either:
- In the loader (preferred): Filter
input.fieldsto only the matching field before returning, or return a pointer to the selected field.- In the component: Use
.find()to match the field byb_id, with a fallback to the first element or null.The suggestion in the review is sound. Apply either option depending on whether the loader refactor is feasible in your context.
fix: exception introduced by #294 at nitrogen balance field page
Summary by CodeRabbit