Update farm creation form with new fields and conditional logic#450
Conversation
…fore 2026, adding KvK number, grazing intention and organic certification
…he text format consistent
🦋 Changeset detectedLatest commit: a1ecd75 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds changeset entries; always displays KvK-nummer on the farm overview; hides/blocks derogation for 2026+; and overhauls the Add Farm form to include KvK, grazing intention, and organic certification fields with backend wiring to persist those settings. Changes
Sequence DiagramsequenceDiagram
actor User
participant Form as Farm Create Form
participant Server as Server Action
participant GrazingSvc as Grazing Service
participant OrganicSvc as Organic Service
participant DB as Database
User->>Form: Fill fields (Year, KvK, Grazing?, Organic?, Derogation?)
Form->>Form: If year >= 2026 hide/disable derogation
User->>Form: Submit
Form->>Server: POST form data
Server->>Server: Validate, enforce rules (no derogation if year >= 2026)
alt Grazing enabled
Server->>GrazingSvc: setGrazingIntention(principalId, farmId, year, true)
GrazingSvc->>DB: persist grazing intention
end
alt Organic enabled
Server->>OrganicSvc: addOrganicCertification(farmId, issued, skal, traces)
OrganicSvc->>DB: persist organic certification
end
Server->>DB: create/update farm record
Server->>User: Redirect / respond with result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 #450 +/- ##
============================================
Coverage 88.07% 88.07%
============================================
Files 91 91
Lines 4621 4621
Branches 1492 1492
============================================
Hits 4070 4070
Misses 551 551
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
changed the title
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: 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.create._index.tsx (1)
126-133:⚠️ Potential issue | 🟡 MinorEmpty destructuring pattern in loader signature.
The static analysis correctly flags the empty object pattern
{}. SinceLoaderFunctionArgsisn't being used, consider either using an underscore prefix or removing the parameter entirely.🔧 Proposed fix
-export async function loader({}: LoaderFunctionArgs) { +export async function loader(_args: LoaderFunctionArgs) { const yearSelection = getCalendarSelection()Or if the type annotation isn't needed:
-export async function loader({}: LoaderFunctionArgs) { +export async function loader() { const yearSelection = getCalendarSelection()
🤖 Fix all issues with AI agents
In `@fdm-app/app/routes/farm._index.tsx`:
- Around line 317-330: The conditional currently checking farm.b_postalcode_farm
to decide whether to render the KvK number is wrong; update the JSX conditional
in the farm listing block to check farm.b_businessid_farm instead so the KvK
(farm.b_businessid_farm) is shown when present and "Onbekend" otherwise—locate
the JSX that renders "KvK-nummer:" and replace the conditional reference to
farm.b_postalcode_farm with farm.b_businessid_farm.
In `@fdm-app/app/routes/farm.create._index.tsx`:
- Around line 686-720: Fix the typo and duplicate list header in the help-list:
change the misspelled word "zdoat" in the paragraph under "Bedrijfsgegevens
(Nu):" to "zodat", and rename the second duplicate list header "Percelen:" (the
item that talks about checking hoofdgewas, bufferstroken and bodemeigenschappen)
to a distinct label such as "Perceelsdetails:" (or merge its content into the
previous "Percelen:" item) so headers are not duplicated; update the
corresponding JSX strings where "Bedrijfsgegevens (Nu):", "Percelen:" and the
second "Percelen:" appear to reflect these text changes.
🧹 Nitpick comments (1)
fdm-app/app/routes/farm.create._index.tsx (1)
756-761: Remove commented-out code.This commented-out tip block should be removed or restored. Leaving dead code in the codebase reduces readability.
🧹 Proposed fix
<p className="text-muted-foreground"> Ja, je kunt ook je gegevens na het doorlopen van het stappenplan invullen of aanpassen. Met het stappenplan staat het alleen makkelijk op een rijtje om een nieuw bedrijf aan te maken. </p> - {/* <p className="text-xs text-muted-foreground italic"> - Tip: KVK- en SKAL-nummers zijn optioneel - maar handig voor het automatisch - invullen van rapportages. - </p> */} </div>
There was a problem hiding this comment.
Actionable comments posted: 0
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.create._index.tsx (1)
786-817:⚠️ Potential issue | 🟡 MinorNormalize derogation flags for year ≥ 2026 before processing
If a user enables derogation for 2025 and then switches to 2026, the hidden flag can still submit as true. This conflicts with the requirement to default to false on submission; consider normalizing the values before the block.
✅ Suggested normalization
- if (year < 2026 && has_derogation && derogation_start_year) { + const isDerogationAllowed = year < 2026 + const effectiveHasDerogation = isDerogationAllowed && has_derogation + const effectiveDerogationStartYear = isDerogationAllowed + ? derogation_start_year + : undefined + + if (effectiveHasDerogation && effectiveDerogationStartYear) { const years = Array.from( - { length: 2025 - derogation_start_year + 1 }, - (_, i) => derogation_start_year + i, + { length: 2025 - effectiveDerogationStartYear + 1 }, + (_, i) => effectiveDerogationStartYear + i, ) await Promise.all( years.map((year) => addDerogation(fdm, session.principal_id, b_id_farm, year), ), ) }
🧹 Nitpick comments (1)
fdm-app/app/routes/farm.create._index.tsx (1)
93-101: Normalize optionalderogation_start_yearparsingConsider converting empty-string inputs to
undefinedbefore coercion so optional numeric fields don’t fail withNaNif a cleared value posts as"".Based on learnings: In fdm-app Zod validation schemas, required numeric fields use direct z.coerce.number() without preprocessing, while optional numeric fields use z.preprocess((val) => (val === "" ? undefined : val), z.coerce.number().optional()) to convert empty strings to undefined.♻️ Proposed refactor
- derogation_start_year: z.coerce - .number() - .min(2006, { - error: "Startjaar moet minimaal 2006 zijn", - }) - .max(2025, { - error: "Startjaar mag maximaal 2025 zijn", - }) - .optional(), + derogation_start_year: z.preprocess( + (val) => (val === "" ? undefined : val), + z.coerce + .number() + .min(2006, { + error: "Startjaar moet minimaal 2006 zijn", + }) + .max(2025, { + error: "Startjaar mag maximaal 2025 zijn", + }) + .optional(), + ),
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@fdm-app/app/routes/farm.create._index.tsx`:
- Around line 724-737: The sentence in the JSX list item after the <strong
className="text-foreground">Bouwplan:</strong> contains a double verb ("Hier vul
je ... invullen"); update that text node to use a single correct verb form—e.g.,
change "Hier vul je op gewasniveau de bemestingen, oogsten en andere gegevens
van je gewassen invullen." to either "Hier vul je op gewasniveau de bemestingen,
oogsten en andere gegevens van je gewassen in." or "Hier kun je op gewasniveau
de bemestingen, oogsten en andere gegevens van je gewassen invullen."—modify the
string in the JSX accordingly.
- Around line 714-716: Replace the typo "buffestroken" with "bufferstroken" in
the Dutch UI string that reads "Check het hoofdgewas van de percelen, markeer
buffestroken en bekijk de geschatte bodemeigenschappen." (locate the
JSX/translation entry in the farm create route file where that exact sentence is
rendered) and, if this string is part of an i18n resource or translation key,
update the translation source entry instead of hardcoded JSX so the change
propagates correctly.
🧹 Nitpick comments (4)
fdm-app/app/routes/farm.$b_id_farm.settings.derogation.tsx (1)
86-91: Good server-side guard for the 2026 derogation cutoff.The UI on line 110 already filters to
year <= 2025, so this action-side check provides proper defense-in-depth. One minor nit from Biome: line 89 uses a template literal without interpolation.Suggested fix
if (year >= 2026) { return dataWithError( {}, - `Derogatie is niet meer beschikbaar vanaf 2026.`, + "Derogatie is niet meer beschikbaar vanaf 2026.", ) }fdm-app/app/routes/farm.create._index.tsx (3)
164-177: Consider resettinghas_derogationwhen year changes to ≥ 2026.When the user checks derogation for a pre-2026 year and then switches the year to 2026+, the derogation section hides but
has_derogationstaystruein form state. The action handles this correctly (line 811-812), but the stale form state could be confusing during development or if validation rules change later. A smalluseEffectto clear it would keep form state consistent with the UI.Suggested addition near line 177
}, [organicCertified, selectedYear]) + + // Reset derogation when year >= 2026 + // biome-ignore lint/correctness/useExhaustiveDependencies: form.setValue is stable + useEffect(() => { + if (Number(selectedYear) >= 2026) { + form.setValue("has_derogation", false) + } + }, [selectedYear])
70-126: Schema validates organic fields even whenorganic_certificationis false.The
organic_skalandorganic_tracesrefinements run regardless of theorganic_certificationvalue. Since the fields are hidden in the UI and defaults are""(which passes!val), this works in practice. However, if a future change initializes these with non-empty defaults or if form state leaks, validation could unexpectedly fail. A.superRefine()or conditional schema (e.g., discriminated union onorganic_certification) would be more robust, but the current approach is acceptable given the UI guards.
789-800: Sequential awaits for independent operations — considerPromise.all.The
setGrazingIntention(lines 830-837) andaddOrganicCertification(lines 840-849) calls are independent of each other and of the derogation block. Grouping them withPromise.all(as already done for derogation years and fertilizers) would reduce latency.Also applies to: 829-849
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@fdm-app/app/routes/farm.create._index.tsx`:
- Around line 70-134: The superRefine block uses the deprecated enum
z.ZodIssueCode.custom when calling ctx.addIssue; replace both occurrences with
the string literal "custom" (in the ctx.addIssue calls that validate
organic_skal and organic_traces) so the two addIssue calls use code: "custom"
instead of z.ZodIssueCode.custom.
🧹 Nitpick comments (2)
fdm-app/app/routes/farm.create._index.tsx (2)
550-552: Use existingorganicCertifiedvariable instead of a secondform.watchcall.
organicCertifiedis already derived fromform.watch("organic_certification")on line 173. Reuse it here instead of callingform.watchagain. Same applies tohas_derogationon line 393 — consider extracting it to a component-level variable for consistency.♻️ Suggested diff
- {form.watch( - "organic_certification", - ) && ( + {organicCertified && (
833-894: Partial farm state on setup failure.If
Promise.all(setupPromises)at line 894 throws (e.g.,addOrganicCertificationorenableFertilizerCataloguefails), the farm created at line 818 persists without its catalogues, derogation, grazing, or organic data. With more operations batched here now, the blast radius of a partial failure has grown. Consider whether the user has a recovery path (e.g., re-running setup from farm settings) or whether a compensating cleanup is warranted.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@fdm-app/app/routes/farm.create._index.tsx`:
- Around line 176-185: When organic_certification is toggled off the form should
clear organic_issued so re-checking can re-initialize it to the current
selectedYear; update the useEffect that watches organicCertified and
selectedYear (the hook using form.getValues and form.setValue) to set
form.setValue("organic_issued", undefined) when organicCertified becomes false,
and keep the existing logic that sets organic_issued to new
Date(Number(selectedYear), 0, 1) when organicCertified is true and
form.getValues("organic_issued") is empty so the DatePicker default and the
stored form value stay in sync.
🧹 Nitpick comments (2)
fdm-app/app/routes/farm.create._index.tsx (2)
393-395: Extracthas_derogationwatch to a named variable for consistency.Lines 172–173 store watched values in variables (
selectedYear,organicCertified), buthas_derogationis watched inline here. Extracting it keeps the pattern consistent and avoids a repeated string literal.
815-891: Farm creation is not rolled back if a subsequent setup step fails.
addFarmat line 815 creates the farm before any of the setup promises run. IfPromise.all(setupPromises)or the fertilizer setup (lines 893–909) throws, the farm persists in a partially configured state. A user retrying the form would create a duplicate farm.This is a pre-existing pattern, but the risk surface has grown now that derogation, grazing, organic certification, and catalogue enablement all run in the same batch.
Consider wrapping the entire operation in a database transaction, or at minimum making the action idempotent (e.g., check for an existing farm with the same name/business ID before creating a new one).
1 participant
Summary by CodeRabbit
New Features
Improvements
UI
Closes #437