Enhance fertilizer management UI with searchable forms#504
Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughAdds extensive fertilizer UI and data-model changes: toggleable nutrient columns, searchable catalogue picker distinguishing standard vs custom entries, a modernized Field/Controller fertilizer form with live summary and mobile UX tweaks, new fertilizer utils (defaults/mappings/payload), and loader/action updates exposing RVO mappings and is_custom flags. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Client (Browser)
participant Page as Fertilizer Page (Route)
participant Loader as Server Loader/Action
participant Utils as Fertilizer Utils
participant DB as Persistence/Catalogue
rect rgba(100,150,240,0.5)
User->>Page: Open "Add fertilizer" or select catalogue item
Page->>Loader: Request loader data (fertilizers)
Loader->>Utils: getRvoMappings(fertilizers)
Utils-->>Loader: rvoLabels, rvoToType
Loader-->>Page: Return fertilizers + rvoLabels + rvoToType
end
rect rgba(120,200,140,0.5)
User->>Page: Fill form / choose item & submit
Page->>Utils: buildCataloguePayload(formValues, rvoToType)
Page->>Loader: Call action (createFertilizerAction)
Loader->>DB: Persist fertilizer/catalogue entry
DB-->>Loader: Success
Loader-->>Page: Redirect/response
Page-->>User: Navigation/confirmation
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. 📢 Thoughts on this report? Let us know! |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
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 (2)
fdm-app/app/components/blocks/fertilizer/table.tsx (1)
198-211:⚠️ Potential issue | 🟠 MajorMake the clickable row keyboard reachable.
This turns the details navigation into a mouse-only interaction. A plain
onClickonTableRowis not focusable and does not support Enter/Space, so keyboard users can no longer open a fertilizer from the list. Please keep a real link in the row or add focus, keyboard handling, and an accessible label.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/fertilizer/table.tsx` around lines 198 - 211, The TableRow click handler makes rows mouse-only; update the TableRow (component with key={row.id}, data-state and onClick using row.getIsSelected(), row.original and navigate(`./${p_id}`)) to be keyboard-accessible by either turning the row into a real link or adding keyboard support: add a focusable attribute (tabIndex=0), an accessible label (aria-label or aria-labelledby that includes the fertilizer name/ID), and an onKeyDown handler that triggers the same navigate logic on Enter and Space; ensure role="button" is present if you keep it as an interactive non-anchor element and preserve the existing onClick behavior so both mouse and keyboard activate navigate(`./${p_id}`).fdm-app/app/routes/farm.$b_id_farm.fertilizers.new.custom.tsx (1)
207-221:⚠️ Potential issue | 🔴 CriticalMake the create flow atomic.
addFertilizerToCatalogueis committed beforeaddFertilizer. If the second write fails, the request surfaces an error after already persisting the catalogue row, so retries can create duplicates and leave orphaned data behind. Sincefarm.$b_id_farm.fertilizers.new.$p_id.tsxre-exports this same action, both create flows now share the same partial-write path. Please move this into a single transaction/core API or add compensation on failure.🤖 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.fertilizers.new.custom.tsx around lines 207 - 221, The current flow calls addFertilizerToCatalogue then addFertilizer separately which can leave a committed catalogue row if addFertilizer fails; change this to an atomic operation by moving both writes into a single transactional core API (e.g., implement createFertilizerWithCatalogue that opens a DB transaction, inserts the catalogue row and the fertilizer row, then commits or rolls back on error) or, if transactions aren’t available, implement compensation by deleting the catalogue row when addFertilizer throws (call addFertilizerToCatalogue, then call addFertilizer inside a try/catch and on error call deleteCatalogueEntry for the returned p_id_catalogue and rethrow). Ensure you update the callers that currently call addFertilizerToCatalogue and addFertilizer (the code around addFertilizerToCatalogue and addFertilizer) to use the new transactional API or compensation flow.
🧹 Nitpick comments (2)
fdm-app/app/components/blocks/fertilizer/utils.ts (1)
9-14: Prefer an explicit source type overRecord<string, any>.This helper is now the default-value source for multiple fertilizer routes. Keeping the input as
Record<string, any>means field drift between the shared fertilizer model,FormSchema, and this mapper will only surface at runtime. Tightening this to an explicit form-seed type, or extending the sharedFertilizercontract with the missing fields, would make those mismatches fail at compile time instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/fertilizer/utils.ts` around lines 9 - 14, The function buildFertilizerDefaults currently accepts fertilizer: Partial<Fertilizer> & Record<string, any>, which allows arbitrary runtime-only fields; replace Record<string, any> with a concrete form-seed type (e.g., define a FertilizerFormSeed or extend the shared Fertilizer interface with any missing form-only fields) and use that type in the buildFertilizerDefaults signature so mismatches against FormSchema will be caught at compile time; update any call sites that pass form data to conform to the new FertilizerFormSeed and keep helper utilities like toUndefined unchanged.fdm-app/app/routes/farm.$b_id_farm.fertilizers.new.custom.tsx (1)
54-64: Extract the RVO metadata assembly into one helper.The
p_type_rvolabel lookup andrvoToTypemapping now live in multiple loaders (farm.$b_id_farm.fertilizers.new._index.tsx,farm.$b_id_farm.fertilizers.$p_id.tsx, andfarm.$b_id_farm.fertilizers.new.$p_id.tsx). Centralizing this next tobuildFertilizerDefaultswill keep the create/view flows from drifting the next time the RVO options change.Also applies to: 86-93
🤖 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.fertilizers.new.custom.tsx around lines 54 - 64, The RVO label lookup and rvo->type mapping logic (using getFertilizerParametersDescription, p_type_rvo_options, and rvoLabelByValue) is duplicated across multiple loaders; extract this into a single helper function next to buildFertilizerDefaults and replace the inline assembly in farm.$b_id_farm.fertilizers.new.custom.tsx and the other loaders (farm.$b_id_farm.fertilizers.new._index.tsx, farm.$b_id_farm.fertilizers.$p_id.tsx, farm.$b_id_farm.fertilizers.new.$p_id.tsx) to return both the options and a Map of value->label (or an rvoToType mapping) so callers just call the helper instead of repeating the find/map logic.
🤖 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/fertilizer/form.tsx`:
- Around line 199-270: The custom checkbox renderer getAppMethodInput does not
display the array-level validation error for the required p_app_method_options
field; update the Controller render to show the field error (use
fieldState.error or form.formState.errors[param.parameter]) beneath the checkbox
grid, rendering a FieldError (or equivalent) when fieldState.error?.message
exists so users see the validation message when no method is selected; reference
getAppMethodInput, Controller, and FormSchemaKeys/p_app_method_options to locate
where to add the FieldError output.
- Around line 329-341: The "Gebruik als sjabloon" NavLink currently builds a
path `/farm/${b_id_farm}/fertilizers/new/${p_id}` but drops the returnUrl query
param; update the link logic to preserve the current returnUrl (read from the
current location's search via useLocation()/URLSearchParams or from props if
provided) and append it to the NavLink href as a query parameter (e.g.
`...?returnUrl=${encodeURIComponent(returnUrl)}`), ensuring you modify the
NavLink construction near the p_id conditional so the cloned-catalogue flow
keeps the caller context.
- Around line 95-101: The categories array in the fertilizer form omits the
"heavy_metals" category, which causes inputs defined under
FertilizerParameterDescriptionItem["category"] (e.g., p_as_rt, p_cd_rt, p_cr_rt,
p_pb_rt, p_hg_rt) to not render; add an entry { name: "heavy_metals", title:
"Zware metalen" } (or appropriate localized title) to the categories constant so
the heavy metal parameter fields are included by the form rendering logic that
uses categories.
In `@fdm-app/app/components/blocks/fertilizer/new-fertilizer-page.tsx`:
- Around line 13-22: The wrapper component for FertilizerForm forwards rvoLabels
but omits the rvoToType prop, so FertilizerForm can't update its badge/category
when p_type_rvo changes; update the wrapper to accept and pass rvoToType through
(add rvoToType to the component's props signature alongside rvoLabels and
forward rvoToType into the FertilizerForm JSX), and likewise ensure any other
wrapper usages in the same file that forward rvoLabels (the other block around
the FertilizerForm render) also accept and pass rvoToType.
In `@fdm-app/app/routes/farm`.$b_id_farm.fertilizers.$p_id.tsx:
- Around line 183-187: The form's defaultValues passed to useRemixForm (created
with FormSchema and buildFertilizerDefaults(fertilizer)) are cached, so add a
useEffect that calls form.reset(buildFertilizerDefaults(fertilizer)) whenever
the fertilizer prop changes (i.e., when route param p_id changes via
HeaderFertilizer) to avoid submitting stale values; reference the existing form
variable returned from useRemixForm and the buildFertilizerDefaults helper in
the effect's dependency array.
---
Outside diff comments:
In `@fdm-app/app/components/blocks/fertilizer/table.tsx`:
- Around line 198-211: The TableRow click handler makes rows mouse-only; update
the TableRow (component with key={row.id}, data-state and onClick using
row.getIsSelected(), row.original and navigate(`./${p_id}`)) to be
keyboard-accessible by either turning the row into a real link or adding
keyboard support: add a focusable attribute (tabIndex=0), an accessible label
(aria-label or aria-labelledby that includes the fertilizer name/ID), and an
onKeyDown handler that triggers the same navigate logic on Enter and Space;
ensure role="button" is present if you keep it as an interactive non-anchor
element and preserve the existing onClick behavior so both mouse and keyboard
activate navigate(`./${p_id}`).
In `@fdm-app/app/routes/farm`.$b_id_farm.fertilizers.new.custom.tsx:
- Around line 207-221: The current flow calls addFertilizerToCatalogue then
addFertilizer separately which can leave a committed catalogue row if
addFertilizer fails; change this to an atomic operation by moving both writes
into a single transactional core API (e.g., implement
createFertilizerWithCatalogue that opens a DB transaction, inserts the catalogue
row and the fertilizer row, then commits or rolls back on error) or, if
transactions aren’t available, implement compensation by deleting the catalogue
row when addFertilizer throws (call addFertilizerToCatalogue, then call
addFertilizer inside a try/catch and on error call deleteCatalogueEntry for the
returned p_id_catalogue and rethrow). Ensure you update the callers that
currently call addFertilizerToCatalogue and addFertilizer (the code around
addFertilizerToCatalogue and addFertilizer) to use the new transactional API or
compensation flow.
---
Nitpick comments:
In `@fdm-app/app/components/blocks/fertilizer/utils.ts`:
- Around line 9-14: The function buildFertilizerDefaults currently accepts
fertilizer: Partial<Fertilizer> & Record<string, any>, which allows arbitrary
runtime-only fields; replace Record<string, any> with a concrete form-seed type
(e.g., define a FertilizerFormSeed or extend the shared Fertilizer interface
with any missing form-only fields) and use that type in the
buildFertilizerDefaults signature so mismatches against FormSchema will be
caught at compile time; update any call sites that pass form data to conform to
the new FertilizerFormSeed and keep helper utilities like toUndefined unchanged.
In `@fdm-app/app/routes/farm`.$b_id_farm.fertilizers.new.custom.tsx:
- Around line 54-64: The RVO label lookup and rvo->type mapping logic (using
getFertilizerParametersDescription, p_type_rvo_options, and rvoLabelByValue) is
duplicated across multiple loaders; extract this into a single helper function
next to buildFertilizerDefaults and replace the inline assembly in
farm.$b_id_farm.fertilizers.new.custom.tsx and the other loaders
(farm.$b_id_farm.fertilizers.new._index.tsx,
farm.$b_id_farm.fertilizers.$p_id.tsx,
farm.$b_id_farm.fertilizers.new.$p_id.tsx) to return both the options and a Map
of value->label (or an rvoToType mapping) so callers just call the helper
instead of repeating the find/map logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e3df8a2d-797c-4c1c-a6d6-eb9a2f915114
📒 Files selected for processing (15)
.changeset/fertilizer-improvements.md.changeset/tall-singers-stand.mdfdm-app/app/components/blocks/farm/farm-title.tsxfdm-app/app/components/blocks/fertilizer/columns.tsxfdm-app/app/components/blocks/fertilizer/form.tsxfdm-app/app/components/blocks/fertilizer/new-custom-fertilizer-page.tsxfdm-app/app/components/blocks/fertilizer/new-fertilizer-page.tsxfdm-app/app/components/blocks/fertilizer/table.tsxfdm-app/app/components/blocks/fertilizer/utils.tsfdm-app/app/routes/farm.$b_id_farm.fertilizers.$p_id.tsxfdm-app/app/routes/farm.$b_id_farm.fertilizers._index.tsxfdm-app/app/routes/farm.$b_id_farm.fertilizers.new.$p_id.tsxfdm-app/app/routes/farm.$b_id_farm.fertilizers.new._index.tsxfdm-app/app/routes/farm.$b_id_farm.fertilizers.new.custom.tsxfdm-app/app/routes/farm.$b_id_farm.fertilizers.new.tsx
💤 Files with no reviewable changes (1)
- fdm-app/app/components/blocks/fertilizer/new-custom-fertilizer-page.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
fdm-app/app/components/blocks/fertilizer/table.tsx (1)
195-247:⚠️ Potential issue | 🟡 MinorKeep the empty-state colspan aligned with visible columns.
With
row.getVisibleCells()and column toggles enabled, the fallback row still spanscolumns.length, soGeen resultaten.ends up using the wrong colspan after users hide columns. Use the visible leaf column count there instead.🩹 Proposed fix
- <TableCell - colSpan={columns.length} + <TableCell + colSpan={table.getVisibleLeafColumns().length} className="h-24 text-center" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/fertilizer/table.tsx` around lines 195 - 247, The empty-state TableCell currently uses columns.length for colSpan which misaligns when columns are toggled; update the fallback cell's colSpan to use the visible column count from the table instance (e.g., table.getVisibleLeafColumns().length) so the "Geen resultaten." row spans only the visible columns; change the colSpan argument in the TableCell inside the no-rows fallback accordingly (referencing table and the fallback TableCell).fdm-app/app/routes/farm.$b_id_farm.fertilizers.new.$p_id.tsx (1)
98-113:⚠️ Potential issue | 🟠 MajorReject
p_ids that are outside the farm-scoped fertilizer list.The loader now derives
fertilizerOptions,rvoLabels, andrvoToTypefromfertilizers, but it still accepts any record returned bygetFertilizer(fdm, p_id). A tampered URL can therefore preload a fertilizer that is missing from this farm’s options/mappings, which leaves the form in an inconsistent state and can expose out-of-scope data.Proposed guard
const fertilizers = await getFertilizers( fdm, session.principal_id, b_id_farm, ) + if (!fertilizers.some(({ p_id: fertilizerId }) => fertilizerId === p_id)) { + throw data("not found: p_id", { + status: 404, + statusText: "not found: p_id", + }) + } + const fertilizerOptions = fertilizers.map((fertilizer) => { return { p_id: fertilizer.p_id, p_name_nl: fertilizer.p_name_nl, }🤖 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.fertilizers.new.$p_id.tsx around lines 98 - 113, The loader currently accepts any p_id from getFertilizer(fdm, p_id) even when that fertilizer is not part of the farm-scoped fertilizers list used to build fertilizerOptions/rvoLabels/rvoToType; add a guard after fetching fertilizers and before returning to validate that the fetched fertilizer's id (p_id) exists in the fertilizers array (e.g., compare against fertilizerOptions or fertilizers.map(f => f.id)); if the id is not present, reject the request by throwing a 404/BadRequest or redirecting (so the UI cannot preload out-of-scope data), otherwise proceed to return the existing payload (including rvoLabels and rvoToType from getRvoMappings).
♻️ Duplicate comments (1)
fdm-app/app/components/blocks/fertilizer/form.tsx (1)
99-105:⚠️ Potential issue | 🟠 MajorRender the
heavy_metalsgroup again.
groupedParametersstill collects"heavy_metals"entries, but the category list no longer renders that section, so inputs likep_as_rt,p_cd_rt,p_cr_rt,p_pb_rt, andp_hg_rtdisappear entirely.🩹 Proposed fix
const categories = [ { name: "general", title: "Algemeen" }, { name: "primary", title: "Primaire nutriënten" }, { name: "secondary", title: "Secundaire nutriënten" }, { name: "physical", title: "Fysische eigenschappen" }, { name: "trace", title: "Sporenelementen" }, + { name: "heavy_metals", title: "Zware metalen" }, ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/fertilizer/form.tsx` around lines 99 - 105, The categories array in form.tsx dropped the "heavy_metals" group so inputs collected into groupedParameters under "heavy_metals" (e.g., p_as_rt, p_cd_rt, p_cr_rt, p_pb_rt, p_hg_rt) are no longer rendered; restore that category by adding an object to the categories array with name: "heavy_metals" and an appropriate title (e.g., "Zware metalen" or similar) so the rendering loop that uses categories will include the heavy metals section and display those inputs again.
🧹 Nitpick comments (1)
fdm-app/app/routes/farm.$b_id_farm.fertilizers.new.$p_id.tsx (1)
119-124: Update or remove the stale component docblock.Lines 122-123 still describe a sidebar,
Outlet, and toaster, but this component now only rendersFarmTitleandFarmNewFertilizerBlock. The comment is misleading in its current form.🤖 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.fertilizers.new.$p_id.tsx around lines 119 - 124, The top docblock is stale—replace or remove it so it accurately describes the component’s current output: update the comment to state that this module renders the FarmTitle and FarmNewFertilizerBlock components to display the page for creating a new fertilizer from an existing one (remove references to sidebar, Outlet, and toaster), or delete the block entirely if you prefer minimal comments; reference the FarmTitle and FarmNewFertilizerBlock symbols to locate the component’s rendering logic when editing.
🤖 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/routes/farm`.$b_id_farm.fertilizers.new.custom.tsx:
- Around line 118-169: buildCataloguePayload currently forces p_type to null
which strips the fertilizer classification; change it to accept and use the
derived type (from the loader's rvoToType) instead of hard-coding null. Update
buildCataloguePayload (and its call sites) to take an additional argument (e.g.,
derivedType or rvoType) or compute p_type from formValues.p_type_rvo via the
existing rvoToType helper, and set p_type: derivedType (or the computed value)
in the returned object so addFertilizerToCatalogue can derive p_type_manure /
p_type_mineral / p_type_compost correctly.
---
Outside diff comments:
In `@fdm-app/app/components/blocks/fertilizer/table.tsx`:
- Around line 195-247: The empty-state TableCell currently uses columns.length
for colSpan which misaligns when columns are toggled; update the fallback cell's
colSpan to use the visible column count from the table instance (e.g.,
table.getVisibleLeafColumns().length) so the "Geen resultaten." row spans only
the visible columns; change the colSpan argument in the TableCell inside the
no-rows fallback accordingly (referencing table and the fallback TableCell).
In `@fdm-app/app/routes/farm`.$b_id_farm.fertilizers.new.$p_id.tsx:
- Around line 98-113: The loader currently accepts any p_id from
getFertilizer(fdm, p_id) even when that fertilizer is not part of the
farm-scoped fertilizers list used to build
fertilizerOptions/rvoLabels/rvoToType; add a guard after fetching fertilizers
and before returning to validate that the fetched fertilizer's id (p_id) exists
in the fertilizers array (e.g., compare against fertilizerOptions or
fertilizers.map(f => f.id)); if the id is not present, reject the request by
throwing a 404/BadRequest or redirecting (so the UI cannot preload out-of-scope
data), otherwise proceed to return the existing payload (including rvoLabels and
rvoToType from getRvoMappings).
---
Duplicate comments:
In `@fdm-app/app/components/blocks/fertilizer/form.tsx`:
- Around line 99-105: The categories array in form.tsx dropped the
"heavy_metals" group so inputs collected into groupedParameters under
"heavy_metals" (e.g., p_as_rt, p_cd_rt, p_cr_rt, p_pb_rt, p_hg_rt) are no longer
rendered; restore that category by adding an object to the categories array with
name: "heavy_metals" and an appropriate title (e.g., "Zware metalen" or similar)
so the rendering loop that uses categories will include the heavy metals section
and display those inputs again.
---
Nitpick comments:
In `@fdm-app/app/routes/farm`.$b_id_farm.fertilizers.new.$p_id.tsx:
- Around line 119-124: The top docblock is stale—replace or remove it so it
accurately describes the component’s current output: update the comment to state
that this module renders the FarmTitle and FarmNewFertilizerBlock components to
display the page for creating a new fertilizer from an existing one (remove
references to sidebar, Outlet, and toaster), or delete the block entirely if you
prefer minimal comments; reference the FarmTitle and FarmNewFertilizerBlock
symbols to locate the component’s rendering logic when editing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 29e12791-21d4-4ac0-bf74-bba23e69f40d
📒 Files selected for processing (10)
.changeset/neat-adults-attack.mdfdm-app/app/components/blocks/fertilizer/form.tsxfdm-app/app/components/blocks/fertilizer/new-fertilizer-page.tsxfdm-app/app/components/blocks/fertilizer/table.tsxfdm-app/app/components/blocks/fertilizer/utils.tsfdm-app/app/routes/farm.$b_id_farm.fertilizers.$p_id.tsxfdm-app/app/routes/farm.$b_id_farm.fertilizers.new.$p_id.tsxfdm-app/app/routes/farm.$b_id_farm.fertilizers.new._index.tsxfdm-app/app/routes/farm.$b_id_farm.fertilizers.new.custom.tsxfdm-core/src/fertilizer.d.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- fdm-app/app/components/blocks/fertilizer/new-fertilizer-page.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
fdm-app/app/components/blocks/fertilizer/table.tsx (1)
121-135:⚠️ Potential issue | 🟠 MajorSearch is still single-column.
This input only drives the
p_name_nlcolumn filter, so searching by mestcode (p_type_rvo) still will not return results, and there is no manure/compost/mineral filter control yet. That leaves the linked search/filter work incomplete.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/fertilizer/table.tsx` around lines 121 - 135, The search Input currently only updates table.getColumn("p_name_nl") so searching by mestcode (p_type_rvo) or by material type (manure/compost/mineral) won't work; change the behavior to drive a global/multi-column filter (e.g., use table.setGlobalFilter or apply the same value to both table.getColumn("p_name_nl") and table.getColumn("p_type_rvo")) and add a separate control (select or segmented control) bound to a new column filter or state for material type (e.g., p_material_type or the column you use to represent manure/compost/mineral) so the UI supports searching by code and filtering by material type in addition to the name column.fdm-app/app/routes/farm.$b_id_farm.fertilizers.new.$p_id.tsx (1)
81-106:⚠️ Potential issue | 🟡 MinorReorder to validate farm membership before fetching fertilizer details.
getFertilizer(fdm, p_id)on line 82 has no authorization check—it fetches any fertilizer by ID. While the farm scope validation at lines 101-106 does prevent unauthorized access, it's inefficient to fetch details before validating membership. Check thatp_idis infertilizers(fromgetFertilizers) first, then fetch the full details.🤖 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.fertilizers.new.$p_id.tsx around lines 81 - 106, Move the farm-membership validation so you call getFertilizers(...) and compute fertilizerOptions/rvo mappings before calling getFertilizer(...); specifically, first await getFertilizers(session.principal_id, b_id_farm) and check if fertilizers.some(f => f.p_id === p_id) (throw the existing data(...) 404 if not), then only call getFertilizer(fdm, p_id) and getFertilizerParametersDescription(); keep subsequent uses of fertilizerOptions, rvoLabels and rvoToType unchanged but ensure their creation precedes the membership check and the detailed getFertilizer call.
♻️ Duplicate comments (1)
fdm-app/app/routes/farm.$b_id_farm.fertilizers.new.custom.tsx (1)
139-150:⚠️ Potential issue | 🟠 Major
p_typecan still disappear for unmapped RVO codes.
getRvoMappings(fertilizers)only learns types from fertilizer records already present in this farm. If the submittedp_type_rvois not represented there,buildCataloguePayload(formValues, rvoToType)still falls back top_type: null, so the new catalogue item loses its manure/mineral/compost classification again.🤖 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.fertilizers.new.custom.tsx around lines 139 - 150, The new catalogue item can lose p_type because getRvoMappings(fertilizers) only knows RVO codes already in the farm; before calling buildCataloguePayload(formValues, rvoToType) ensure the RVO mapping for the submitted formValues.p_type_rvo is present: check rvoToType[formValues.p_type_rvo] and if missing, resolve it (e.g. call a shared lookup/lookupRvoType function or extend getRvoMappings to accept an extra code) and insert that mapping into rvoToType so buildCataloguePayload and the subsequent addFertilizerToCatalogue call receive a non-null p_type derived from the submitted p_type_rvo (or fall back to the explicit formValues.p_type if available).
🧹 Nitpick comments (1)
fdm-app/app/routes/farm.$b_id_farm.fertilizers.new.$p_id.tsx (1)
21-21: Move the shared action out of the route module.Reusing the create action is good, but importing it from another route file makes this route depend on another route module's loader/UI graph. A small server helper would keep the reuse without coupling route modules together.
Also applies to: 148-148
🤖 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.fertilizers.new.$p_id.tsx at line 21, The route is importing createFertilizerAction from another route module, coupling UI/loader graphs; extract the shared action into a server-side helper (e.g., a new module like fertilizers.server or a helpers folder) that exports the action function (name: createFertilizerAction) and any types it needs, update this route to import createFertilizerAction from that server helper instead of "./farm.$b_id_farm.fertilizers.new.custom", and update the other occurrence that imports the same symbol so both routes use the new server helper to avoid cross-route coupling.
🤖 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/fertilizer/table.tsx`:
- Around line 213-231: The TableRow is made focusable and given role="link"
which breaks native table semantics; remove tabIndex, role, onClick and
onKeyDown from the TableRow (the element rendered by TableRow) and instead place
a focusable element (e.g., a button or anchor) inside a TableCell that calls
handleNavigate and handles keyboard activation; keep the row rendering as-is for
semantics (keep data-state={row.getIsSelected() && "selected"} and existing row
classnames for hover/visuals), move the aria-label that references p_name onto
the new interactive element, and ensure the interactive element uses the same
click/keyboard logic and focus styles (focus-visible:ring-2, etc.) so
keyboard/screen reader users get proper table announcements while still being
able to activate the row via the inner control.
---
Outside diff comments:
In `@fdm-app/app/components/blocks/fertilizer/table.tsx`:
- Around line 121-135: The search Input currently only updates
table.getColumn("p_name_nl") so searching by mestcode (p_type_rvo) or by
material type (manure/compost/mineral) won't work; change the behavior to drive
a global/multi-column filter (e.g., use table.setGlobalFilter or apply the same
value to both table.getColumn("p_name_nl") and table.getColumn("p_type_rvo"))
and add a separate control (select or segmented control) bound to a new column
filter or state for material type (e.g., p_material_type or the column you use
to represent manure/compost/mineral) so the UI supports searching by code and
filtering by material type in addition to the name column.
In `@fdm-app/app/routes/farm`.$b_id_farm.fertilizers.new.$p_id.tsx:
- Around line 81-106: Move the farm-membership validation so you call
getFertilizers(...) and compute fertilizerOptions/rvo mappings before calling
getFertilizer(...); specifically, first await
getFertilizers(session.principal_id, b_id_farm) and check if fertilizers.some(f
=> f.p_id === p_id) (throw the existing data(...) 404 if not), then only call
getFertilizer(fdm, p_id) and getFertilizerParametersDescription(); keep
subsequent uses of fertilizerOptions, rvoLabels and rvoToType unchanged but
ensure their creation precedes the membership check and the detailed
getFertilizer call.
---
Duplicate comments:
In `@fdm-app/app/routes/farm`.$b_id_farm.fertilizers.new.custom.tsx:
- Around line 139-150: The new catalogue item can lose p_type because
getRvoMappings(fertilizers) only knows RVO codes already in the farm; before
calling buildCataloguePayload(formValues, rvoToType) ensure the RVO mapping for
the submitted formValues.p_type_rvo is present: check
rvoToType[formValues.p_type_rvo] and if missing, resolve it (e.g. call a shared
lookup/lookupRvoType function or extend getRvoMappings to accept an extra code)
and insert that mapping into rvoToType so buildCataloguePayload and the
subsequent addFertilizerToCatalogue call receive a non-null p_type derived from
the submitted p_type_rvo (or fall back to the explicit formValues.p_type if
available).
---
Nitpick comments:
In `@fdm-app/app/routes/farm`.$b_id_farm.fertilizers.new.$p_id.tsx:
- Line 21: The route is importing createFertilizerAction from another route
module, coupling UI/loader graphs; extract the shared action into a server-side
helper (e.g., a new module like fertilizers.server or a helpers folder) that
exports the action function (name: createFertilizerAction) and any types it
needs, update this route to import createFertilizerAction from that server
helper instead of "./farm.$b_id_farm.fertilizers.new.custom", and update the
other occurrence that imports the same symbol so both routes use the new server
helper to avoid cross-route coupling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9a82b1ed-1f0e-49b2-8d90-4487f34271cc
📒 Files selected for processing (4)
fdm-app/app/components/blocks/fertilizer/table.tsxfdm-app/app/components/blocks/fertilizer/utils.tsfdm-app/app/routes/farm.$b_id_farm.fertilizers.new.$p_id.tsxfdm-app/app/routes/farm.$b_id_farm.fertilizers.new.custom.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- fdm-app/app/components/blocks/fertilizer/utils.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
fdm-app/app/routes/farm.$b_id_farm.fertilizers.new._index.tsx (1)
25-26:⚠️ Potential issue | 🟠 MajorReturn a 400 response here instead of throwing a generic error.
Line 25-26 currently turns a missing route param into a loader exception/500, which breaks the route's normal typed error flow. Use the
data()function from react-router to return a proper 400 response consistent with other loaders in the codebase.Suggested fix
import type { LoaderFunctionArgs } from "react-router" -import { Link, useLoaderData, useNavigate, useSearchParams } from "react-router" +import { + data, + Link, + useLoaderData, + useNavigate, + useSearchParams, +} from "react-router" export async function loader({ request, params }: LoaderFunctionArgs) { const { b_id_farm } = params if (!b_id_farm) { - throw new Error("Farm ID is required") + throw data("Farm ID is required", { status: 400 }) }🤖 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.fertilizers.new._index.tsx around lines 25 - 26, The loader currently throws a generic Error when b_id_farm is missing; change this to return a 400 response using react-router's data() helper so the route's typed error flow remains intact: inside the loader where b_id_farm is checked (the variable b_id_farm in this file and the loader function that returns loader data), replace throw new Error("Farm ID is required") with a call to data(...) that constructs a 400 response (include a short message/body matching other loaders) so the loader returns a proper 400 result instead of raising an exception.fdm-app/app/components/blocks/fertilizer/table.tsx (1)
122-133:⚠️ Potential issue | 🟠 MajorSearch input still ignores the new RVO data.
This field only updates the
p_name_nlfilter, so typing a visibleMestcode (RVO)value still returns no match. The table now exposes that column, but the only search box cannot find it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/fertilizer/table.tsx` around lines 122 - 133, The search Input currently only reads/sets table.getColumn("p_name_nl"), so it never matches the new RVO code column; update the Input's value and onChange to read/set both columns (e.g. table.getColumn("p_name_nl") and table.getColumn("p_code_rvo") or whatever the RVO column id is) so typing will apply the same filter text to both columns; modify the value to reflect either column's filter (prefer the common input string) and in onChange call setFilterValue on both columns via table.getColumn(...)? .setFilterValue(...) so the search matches names and Mestcode (RVO).
🧹 Nitpick comments (1)
fdm-calculator/src/norms/nl/2026/filling/stikstofgebruiksnorm.test.ts (1)
609-666: Consider extracting a test fixture factory to reduce boilerplate.The
Fertilizerfixtures are repeated across 6 test cases with ~40 fields each, mostly set tonull. A helper function would reduce duplication and make tests easier to maintain when the interface expands again.♻️ Optional: Extract test fixture factory
// At the top of the test file or in a shared test-utils function createTestFertilizer(overrides: Partial<Fertilizer> & Pick<Fertilizer, 'p_id' | 'p_id_catalogue'>): Fertilizer { return { p_source: "", p_name_nl: null, p_name_en: null, p_description: null, p_app_method_options: null, p_app_amount: null, p_date_acquiring: null, p_picking_date: null, p_dm: null, p_density: null, p_om: null, p_a: null, p_hc: null, p_eom: null, p_eoc: null, p_c_rt: null, p_c_of: null, p_c_if: null, p_c_fr: null, p_cn_of: null, p_n_rt: null, p_n_if: null, p_n_of: null, p_n_wc: null, // ... other defaults p_type_rvo: null, p_type: null, ...overrides, } } // Usage in tests: const fertilizers: Fertilizer[] = [ createTestFertilizer({ p_id: "fert1", p_id_catalogue: "fert1", p_n_rt: 5, p_type_rvo: "115", }), ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-calculator/src/norms/nl/2026/filling/stikstofgebruiksnorm.test.ts` around lines 609 - 666, Tests repeat large Fertilizer objects; extract a fixture factory function (e.g., createTestFertilizer) that returns a Fertilizer with sensible defaults (mostly null/empty) and accepts an overrides Partial<Fertilizer> requiring at least p_id and p_id_catalogue; then replace inline arrays in the tests with calls like createTestFertilizer({ p_id: "fert1", p_id_catalogue: "fert1", p_n_rt: 5, p_type_rvo: "115" }) to keep unique fields while eliminating the ~40-field duplication.
🤖 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/fertilizer/table.tsx`:
- Around line 145-160: The menu currently allows hiding the navigation column
(p_name_nl) and can hide every column, so update the dropdown mapping to (1)
exclude or disable hiding for the navigation column by checking column.id ===
'p_name_nl' and rendering its DropdownMenuCheckboxItem as always
checked/disabled (or skipping it), and (2) guard the onCheckedChange handler
used with column.toggleVisibility to prevent turning off the last visible column
by checking table.getAllColumns().filter(c => c.getIsVisible()).length and only
calling column.toggleVisibility(false) if more than one column would remain
visible; reference table.getAllColumns(), column.getCanHide(),
column.getIsVisible(), column.toggleVisibility, and the 'p_name_nl' column id.
- Around line 165-174: The current markup nests a Button inside a NavLink
(NavLink > Button), creating nested interactive elements; change it so Button is
the accessible wrapper and renders the link by using Button's asChild prop and
placing NavLink inside it (i.e., Button asChild with NavLink as its child and
the Plus icon and text inside the NavLink) so the primary CTA is a single
interactive element; update the instance that references NavLink and Button (and
the Plus icon inside) accordingly and remove the nested-interactive structure.
In `@fdm-app/app/routes/farm`.$b_id_farm.fertilizers.new._index.tsx:
- Around line 162-166: The CommandInput used for catalogue search relies on
placeholder text only and lacks an explicit accessible name; update the
CommandInput (the component instance with props value={searchQuery}
onValueChange={setSearchQuery}) to provide an accessible name by adding either
an aria-label (e.g., aria-label="Zoek product of categorie") or aria-labelledby
pointing to a visible label element, or include an associated <label> with an id
and pass aria-labelledby; ensure the label text clearly describes the field so
assistive tech announces it properly.
- Around line 208-216: The current UI only renders a Badge when
fertilizer.isCustom, making catalogue items ambiguous; update the JSX around the
existing conditional that uses fertilizer.isCustom to always render a Badge (or
render two branches) using fertilizer.sourceName as the label text, keeping the
existing Pencil/icon and special styling when fertilizer.isCustom but showing a
neutral/catalogue badge (e.g., different variant/className) when not custom;
modify the block that currently references fertilizer.isCustom and Badge so
every fertilizer row displays a badge using fertilizer.sourceName.
---
Outside diff comments:
In `@fdm-app/app/components/blocks/fertilizer/table.tsx`:
- Around line 122-133: The search Input currently only reads/sets
table.getColumn("p_name_nl"), so it never matches the new RVO code column;
update the Input's value and onChange to read/set both columns (e.g.
table.getColumn("p_name_nl") and table.getColumn("p_code_rvo") or whatever the
RVO column id is) so typing will apply the same filter text to both columns;
modify the value to reflect either column's filter (prefer the common input
string) and in onChange call setFilterValue on both columns via
table.getColumn(...)? .setFilterValue(...) so the search matches names and
Mestcode (RVO).
In `@fdm-app/app/routes/farm`.$b_id_farm.fertilizers.new._index.tsx:
- Around line 25-26: The loader currently throws a generic Error when b_id_farm
is missing; change this to return a 400 response using react-router's data()
helper so the route's typed error flow remains intact: inside the loader where
b_id_farm is checked (the variable b_id_farm in this file and the loader
function that returns loader data), replace throw new Error("Farm ID is
required") with a call to data(...) that constructs a 400 response (include a
short message/body matching other loaders) so the loader returns a proper 400
result instead of raising an exception.
---
Nitpick comments:
In `@fdm-calculator/src/norms/nl/2026/filling/stikstofgebruiksnorm.test.ts`:
- Around line 609-666: Tests repeat large Fertilizer objects; extract a fixture
factory function (e.g., createTestFertilizer) that returns a Fertilizer with
sensible defaults (mostly null/empty) and accepts an overrides
Partial<Fertilizer> requiring at least p_id and p_id_catalogue; then replace
inline arrays in the tests with calls like createTestFertilizer({ p_id: "fert1",
p_id_catalogue: "fert1", p_n_rt: 5, p_type_rvo: "115" }) to keep unique fields
while eliminating the ~40-field duplication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d80a167c-9c98-4f1a-b0e3-f7e47d55a681
📒 Files selected for processing (4)
fdm-app/app/components/blocks/fertilizer/table.tsxfdm-app/app/routes/farm.$b_id_farm.fertilizers.new._index.tsxfdm-calculator/src/doses/calculate-dose.test.tsfdm-calculator/src/norms/nl/2026/filling/stikstofgebruiksnorm.test.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Sven Verweij <37927107+SvenVw@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/routes/farm`.$b_id_farm.fertilizers.new._index.tsx:
- Around line 168-170: There is a duplicate self-closing tag immediately before
the CommandList component that produces invalid JSX; open the file
farm.$b_id_farm.fertilizers.new._index.tsx, locate the stray "/>" directly above
the CommandList element (near the JSX that renders the input/command component)
and remove the extra "/>" so the preceding element is closed exactly once and
CommandList remains a separate sibling; verify the surrounding JSX (the
input/command component and <CommandList className="max-h-[450px]
overflow-y-auto p-2 border-t">) render correctly after the removal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ed8f9c97-6463-4093-8c02-2a64bf5d82cb
📒 Files selected for processing (3)
fdm-app/app/components/blocks/fertilizer-applications/card.tsxfdm-app/app/components/blocks/fertilizer/table.tsxfdm-app/app/routes/farm.$b_id_farm.fertilizers.new._index.tsx
Comment on lines
+168
to
+170
| /> | ||
| /> | ||
| <CommandList className="max-h-[450px] overflow-y-auto p-2 border-t"> |
Contributor
There was a problem hiding this comment.
Syntax error: duplicate closing tag will cause build failure.
Line 169 has an extra /> that creates invalid JSX. This will prevent the application from compiling.
🐛 Proposed fix
className="text-base py-6"
/>
- />
<CommandList className="max-h-[450px] overflow-y-auto p-2 border-t">📝 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.
Suggested change
| /> | |
| /> | |
| <CommandList className="max-h-[450px] overflow-y-auto p-2 border-t"> | |
| /> | |
| <CommandList className="max-h-[450px] overflow-y-auto p-2 border-t"> |
🧰 Tools
🪛 Biome (2.4.6)
[error] 169-169: Unexpected token. Did you mean {'>'} or >?
(parse)
🤖 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.fertilizers.new._index.tsx around lines
168 - 170, There is a duplicate self-closing tag immediately before the
CommandList component that produces invalid JSX; open the file
farm.$b_id_farm.fertilizers.new._index.tsx, locate the stray "/>" directly above
the CommandList element (near the JSX that renders the input/command component)
and remove the extra "/>" so the preceding element is closed exactly once and
CommandList remains a separate sibling; verify the surrounding JSX (the
input/command component and <CommandList className="max-h-[450px]
overflow-y-auto p-2 border-t">) render correctly after the removal.
BoraIneviNMI
approved these changes
Mar 20, 2026
Collaborator
BoraIneviNMI
left a comment
BoraIneviNMI
left a comment
There was a problem hiding this comment.
This definitely looks nicer from the user's perspective. Nice work!
This was referenced Mar 25, 2026
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 #500