Manage Fertilizer Applications via Badge Interaction in Fields and Rotation Tables#475
Merged
Conversation
🦋 Changeset detectedLatest commit: f135c32 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 |
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds interactive fertilizer-badge navigation and a modify_fertilizer dialog flow: clicking a badge navigates to a dialog listing fertilizer applications (per field/cultivation), supports grouped listing, multi-application edits and deletes, new loaders/actions, table/columns, form/schema refactors, utilities, and thin route wrappers. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Fields/Rotation Table
participant Router as Browser/Router
participant Loader as ModifyFertilizer Loader
participant Core as FDM Core API
participant Dialog as Fertilizer Dialog
User->>UI: Click fertilizer badge
UI->>Router: Navigate to /modify_fertilizer?p_id=...&fieldIds=...
Router->>Loader: loader(params, request)
Loader->>Core: fetch fertilizer + applications for fieldIds
Core-->>Loader: return applications + permission flags
Loader-->>Dialog: loader data (applications, canModify, returnUrl)
Dialog->>Dialog: render DataTable (grouping, modify actions)
User->>Dialog: Edit (navigate with appIds) or Delete (submit action)
alt Edit
Router->>Loader: loader for edit form (appIds)
Loader->>Core: fetch application details
Core-->>Loader: return app data
User->>Router: submit edit -> action
Loader->>Core: updateFertilizerApplication(appIds)
Core-->>Loader: success -> redirect
else Delete
Dialog->>Loader: action submit to remove application(s)
Loader->>Core: remove application(s)
Core-->>Loader: success -> redirect/update
end
Dialog->>UI: Close -> UI reloads/reflects changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## development #475 +/- ##
============================================
Coverage 87.87% 87.87%
============================================
Files 94 94
Lines 4930 4930
Branches 1582 1582
============================================
Hits 4332 4332
Misses 598 598
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
fdm-app/app/routes/farm.$b_id_farm.$calendar.rotation_.fertilizer._index.tsx (1)
97-113: Redundantas string[]cast.
appIdPairsis already typed asstring[]from the function parameter; the cast on line 103 is unnecessary.♻️ Suggested cleanup
- for (const appId of appIdPairs as string[]) { + for (const appId of appIdPairs) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation_.fertilizer._index.tsx around lines 97 - 113, Remove the redundant type assertion in parseAppIds: appIdPairs is already declared as string[], so drop the unnecessary "as string[]" cast on the for-loop; update the loop to iterate directly over appIdPairs (for (const appId of appIdPairs)) and keep the rest of the logic (splitting into b_id and p_app_id and pushing into applicationRefs) unchanged.fdm-app/app/components/blocks/rotation/fertilizer-display.tsx (2)
10-10: Unused imports:useLocationanduseParams.These are imported but never referenced in this component.
♻️ Remove unused imports
-import { NavLink, useLocation, useParams } from "react-router" +import { NavLink } from "react-router"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/rotation/fertilizer-display.tsx` at line 10, The import statement in fertilizer-display.tsx imports unused symbols useLocation and useParams from "react-router"; remove those unused imports to clean up the file by changing the import to only include NavLink (i.e., update the import that currently references NavLink, useLocation, useParams so it references NavLink alone). Locate the import line containing NavLink, useLocation, useParams and delete useLocation and useParams from that declaration.
87-110: NavLink wraps the badge content correctly, enabling per-fertilizer navigation.The
fieldIdsparameter construction and URL encoding are handled properly. Clicking the fertilizer badge will navigate to themodify_fertilizerdialog route with the correct field context.One minor concern: the entire NavLink (icon + label) sits inside the
<Badge>, but the Badge itself isn't the click target—only the NavLink child is. This can create a subtle dead-zone at the badge padding where clicks don't navigate. Consider putting the Badge inside the NavLink, or making the NavLink the outermost element with badge styling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/rotation/fertilizer-display.tsx` around lines 87 - 110, The Badge currently contains a NavLink child which makes only the inner content clickable and creates dead padding; update the markup so the NavLink is the outer element (wrap the <Badge> with the NavLink that builds the URL using fertilizer.p_id and fieldIds) or alternatively make the NavLink the full-size click target by moving badge styling onto the NavLink (ensure classes from fertilizerIconClassMap and fertilizer.p_name_nl remain applied to the visual badge/label). Adjust the JSX around NavLink and Badge in fertilizer-display.tsx so clicks anywhere inside the badge area trigger navigation.fdm-app/app/routes/farm.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx (1)
180-180: Stale comment: "Harvests" should be "Applications".- // Harvests with no date get filtered out in the mapping function + // Applications with no date get filtered out in the mapping function🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx at line 180, Update the stale inline comment "// Harvests with no date get filtered out in the mapping function" to use the correct term "Applications" (e.g. "// Applications with no date get filtered out in the mapping function") so the comment accurately reflects the logic; locate this exact comment string in the rotation modification handler (the mapping/filtering block) and replace "Harvests" with "Applications".
🤖 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-applications/form.tsx`:
- Line 256: Replace the incorrect label text "Oogstdatum" in the fertilizer
application form component by updating the JSX label prop (label="Oogstdatum")
to the domain-correct text "Bemestingsdatum" (or "Datum"); locate the label
attribute in the form component (the fertilizer application form JSX in
form.tsx) and change the string, and also update any corresponding i18n key or
test/aria labels that reference "Oogstdatum" so visible text and
accessibility/test identifiers remain consistent.
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation_.fertilizer._index.tsx:
- Around line 850-866: The transaction started with fdm.transaction(...)
wrapping updateFertilizerApplication calls is not awaited, causing
redirectWithSuccess to run before DB writes finish; update the modify_fertilizer
action to await the Promise returned by fdm.transaction (same pattern used for
addFertilizerApplication), e.g., assign or await fdm.transaction(...) so that
updateFertilizerApplication calls complete for session.principal_id and each
p_app_id before calling redirectWithSuccess and handle errors/throw if the
transaction rejects.
- Around line 246-266: The loop that builds fertilizerApplication uses a falsy
check on exampleFertilizerApplication[key] which wrongly drops legitimate values
like 0 or empty string; update the condition in the nested loop to explicitly
check for null/undefined (e.g., exampleFertilizerApplication[key] === null ||
exampleFertilizerApplication[key] === undefined or use == null) and do the same
for app[key] so only absent values are removed; keep the existing
date-vs-primitive comparison logic using keyTypes[key] with the null-safe checks
so you don't call .getTime() on a null/undefined value when comparing Date
fields; target the fertilizerApplication, exampleFertilizerApplication,
fertilizerApplications, keys and keyTypes symbols mentioned in the diff.
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx:
- Around line 152-167: mapByOrder currently uses the loop index i as the
grouping key which can misalign applications across fields; change it to group
by date proximity using the application.p_app_date instead: for each
ApplicationExtended in mapByOrder, parse p_app_date and try to find an existing
record[key] whose dates array contains a date within a chosen threshold (e.g.,
same day or within N days) and push into that group, otherwise create a new
record entry (preserving id construction using application.p_id plus a unique
suffix). Update references to FertAppRecord and record[key].dates to store
normalized dates (e.g., as ISO strings or timestamps) so comparisons are
reliable and ensure record[key].applications still receives the application.
- Around line 388-407: The deletion branch is kicking off fdm.transaction(...)
without awaiting it, so removeFertilizerApplication calls may still be in-flight
when dataWithSuccess is returned; change the branch so you await the Promise
returned by fdm.transaction (e.g., await fdm.transaction(...)) before returning
dataWithSuccess, and wrap the await in a try/catch to surface transaction errors
(use the same session.principal_id, formData.appIds and
removeFertilizerApplication identifiers to locate the code).
- Around line 279-292: The delete Form currently omits method="post" and passes
encoded "b_id:p_app_id" pairs, so the action removeFertilizerApplication is
never invoked and receives wrong IDs; update the <Form> (component Form) used
for deletion to include method="post" and supply it with a plain list of
p_app_id values (not the b_id:p_app_id encoded strings) — keep modifiableAppIds
(used in the NavLink/bijwerken) if needed, but add a separate plainIds value (or
derive p_app_id by splitting the encoded pairs before rendering) for the delete
hidden input so removeFertilizerApplication receives only p_app_id values.
---
Nitpick comments:
In `@fdm-app/app/components/blocks/rotation/fertilizer-display.tsx`:
- Line 10: The import statement in fertilizer-display.tsx imports unused symbols
useLocation and useParams from "react-router"; remove those unused imports to
clean up the file by changing the import to only include NavLink (i.e., update
the import that currently references NavLink, useLocation, useParams so it
references NavLink alone). Locate the import line containing NavLink,
useLocation, useParams and delete useLocation and useParams from that
declaration.
- Around line 87-110: The Badge currently contains a NavLink child which makes
only the inner content clickable and creates dead padding; update the markup so
the NavLink is the outer element (wrap the <Badge> with the NavLink that builds
the URL using fertilizer.p_id and fieldIds) or alternatively make the NavLink
the full-size click target by moving badge styling onto the NavLink (ensure
classes from fertilizerIconClassMap and fertilizer.p_name_nl remain applied to
the visual badge/label). Adjust the JSX around NavLink and Badge in
fertilizer-display.tsx so clicks anywhere inside the badge area trigger
navigation.
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation_.fertilizer._index.tsx:
- Around line 97-113: Remove the redundant type assertion in parseAppIds:
appIdPairs is already declared as string[], so drop the unnecessary "as
string[]" cast on the for-loop; update the loop to iterate directly over
appIdPairs (for (const appId of appIdPairs)) and keep the rest of the logic
(splitting into b_id and p_app_id and pushing into applicationRefs) unchanged.
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx:
- Line 180: Update the stale inline comment "// Harvests with no date get
filtered out in the mapping function" to use the correct term "Applications"
(e.g. "// Applications with no date get filtered out in the mapping function")
so the comment accurately reflects the logic; locate this exact comment string
in the rotation modification handler (the mapping/filtering block) and replace
"Harvests" with "Applications".
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/quick-dots-tease.mdfdm-app/app/components/blocks/fertilizer-applications/form.tsxfdm-app/app/components/blocks/rotation/fertilizer-display.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.rotation_.fertilizer._index.tsxfdm-app/app/routes/farm.create.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
fdm-app/app/routes/farm.$b_id_farm.$calendar.rotation_.fertilizer._index.tsx (2)
246-271: Past falsy-check issue is now resolved with explicit null/undefined guards — nice fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation_.fertilizer._index.tsx around lines 246 - 271, The reviewer noted a duplicate review comment marker; remove the stray or duplicate marker/comment and any leftover "[duplicate_comment]" text from the review or PR description so only a single, clear review comment remains; ensure the code around fertilizerApplication, keys, exampleFertilizerApplication, fertilizerApplications and keyTypes is untouched and the explicit null/undefined guards remain.
855-876:fdm.transactionis now correctly awaited in the update path — past critical issue resolved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation_.fertilizer._index.tsx around lines 855 - 876, The update path correctly awaits fdm.transaction when mapping over validatedData.p_app_id and calling updateFertilizerApplication, so there is no functional change needed in code; remove the duplicate review/comment marker ([duplicate_comment]) from the PR discussion and ensure there aren’t duplicated comment blocks referencing fdm.transaction, updateFertilizerApplication, or validatedData.p_app_id to avoid confusion.fdm-app/app/routes/farm.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx (1)
155-170: Positional-index grouping inmapByOrdermisaligns applications across fields when counts differ.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx around lines 155 - 170, mapByOrder currently uses the loop index i as the grouping key which causes misalignment when different fields have different numbers of applications; change it to compute a stable grouping key from application data (e.g., prefer a dedicated order property like application.p_app_order if available, otherwise fall back to a deterministic key such as `${application.p_app_date}_${application.p_id}`), then use that key to initialize and append to record[key].applications and record[key].dates instead of using the positional index; update references inside mapByOrder (record: FertAppRecord, applications: ApplicationExtended[], and properties p_app_date and p_id) to use the new computed key so groups stay aligned across fields.
🧹 Nitpick comments (2)
fdm-app/app/components/blocks/rotation/fertilizer-display.tsx (1)
129-134: Replace the(cultivation as FieldRow).b_idcast with a conditional expression in theuseMemodependency.The cast is type-unsafe and will silently evaluate to
undefinedwhencultivation.type !== "field", which makes the dependency's intent unclear.♻️ Proposed fix
- }, [ - uniqueFertilizers, - (cultivation as FieldRow).b_id, - cultivation.type, - cultivation.fields, - ]) + }, [ + uniqueFertilizers, + cultivation.type === "field" + ? (cultivation as FieldRow).b_id + : undefined, + cultivation.type, + cultivation.fields, + ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/rotation/fertilizer-display.tsx` around lines 129 - 134, In the useMemo dependency array inside fertilizer-display.tsx, replace the unsafe cast "(cultivation as FieldRow).b_id" with a conditional expression that only accesses b_id when cultivation.type === "field" (e.g. cultivation.type === "field" ? (cultivation as FieldRow).b_id : undefined) so the dependency accurately reflects the value used and avoids a silent undefined from an invalid cast.fdm-app/app/routes/farm.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx (1)
318-326:recordsis recomputed on every render — consider memoizing withuseMemo.
groupAndOrderFertAppsiterates over all fields × all applications. SincefertilizerApplicationsis stable (loader data), wrapping inuseMemois a free win.♻️ Proposed fix
- const records = groupAndOrderFertApps(fertilizerApplications) + const records = useMemo( + () => groupAndOrderFertApps(fertilizerApplications), + [fertilizerApplications], + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx around lines 318 - 326, The records value is being recomputed on every render by directly calling groupAndOrderFertApps(fertilizerApplications); wrap that call in a useMemo to memoize it (similar to canModifyAnything) so records only recalculates when fertilizerApplications changes; update the code to compute records = useMemo(() => groupAndOrderFertApps(fertilizerApplications), [fertilizerApplications]) referencing the existing groupAndOrderFertApps function and fertilizerApplications data.
🤖 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-applications/form.tsx`:
- Around line 127-130: The guard in the effect uses a falsy check on
fertilizerApplication?.p_app_amount so a legitimate value of 0 is skipped;
update the condition in the effect that calls form.setValue("p_app_amount", ...)
to check explicitly for null/undefined (e.g.,
fertilizerApplication?.p_app_amount !== undefined or != null) or use
Object.prototype.hasOwnProperty on fertilizerApplication for "p_app_amount" so
zero is accepted and still restored by form.setValue.
- Around line 297-328: The outer <Field> wrapper and redundant <FieldError>
around the DatePicker should be removed to avoid nested Field components and
duplicate errors: inside the Controller render for "p_app_date" (where you
currently wrap DatePicker with <Field data-invalid=... className="gap-1"> and
render a second <FieldError> when fieldState.invalid), delete that outer <Field>
and the extra <FieldError>, and if you need to preserve the data-invalid or
className props move them onto the DatePicker props (or into the fieldState
handling already provided by DatePicker) so DatePicker remains the single owner
of its Field and FieldError rendering.
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation_.fertilizer._index.tsx:
- Around line 261-269: The equality-check logic can call (app[key] as
Date).getTime() after fertilizerApplication[key] was deleted when app[key] is
null; update the loop around keyTypes/key comparison in the same block to skip
the second comparison after deletion (e.g., perform the null/date check first
and if you delete fertilizerApplication[key] then continue to the next key or
restructure into an if/else) so that exampleFertilizerApplication,
fertilizerApplication, app and key are not dereferenced after removal.
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx:
- Around line 246-252: The code that computes applicationAmount uses
record.applications.map(...).filter(...).sort() which sorts numbers
lexicographically because Array.prototype.sort was called without a comparator;
change the sort call in the pipeline that feeds formatNumberRange (the
expression building applicationAmount) to use a numeric comparator (e.g., (a, b)
=> a - b) and ensure the mapped values (p_app_amount) are numeric (and still
filtered for null/undefined) before calling formatNumberRange so first/last
numbers are correct; update the sort invocation near the
applicationAmount/formatNumberRange usage accordingly.
- Around line 73-77: The code calls getFertilizer(params.p_id) and returns
fertilizer metadata without verifying the caller’s access to the farm, exposing
fertilizer names to any authenticated user; update the flow to perform an
authorization check (e.g., verify the current user has access to the
farm/rotation via the existing farm/rotation auth helper used elsewhere in this
route) before calling getFertilizer, or move the auth into getFertilizer itself
so it validates access for params.p_id, and only then construct and return the
FertilizerInfo (references: getFertilizer, originalFertilizer, params.p_id).
---
Duplicate comments:
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation_.fertilizer._index.tsx:
- Around line 246-271: The reviewer noted a duplicate review comment marker;
remove the stray or duplicate marker/comment and any leftover
"[duplicate_comment]" text from the review or PR description so only a single,
clear review comment remains; ensure the code around fertilizerApplication,
keys, exampleFertilizerApplication, fertilizerApplications and keyTypes is
untouched and the explicit null/undefined guards remain.
- Around line 855-876: The update path correctly awaits fdm.transaction when
mapping over validatedData.p_app_id and calling updateFertilizerApplication, so
there is no functional change needed in code; remove the duplicate
review/comment marker ([duplicate_comment]) from the PR discussion and ensure
there aren’t duplicated comment blocks referencing fdm.transaction,
updateFertilizerApplication, or validatedData.p_app_id to avoid confusion.
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx:
- Around line 155-170: mapByOrder currently uses the loop index i as the
grouping key which causes misalignment when different fields have different
numbers of applications; change it to compute a stable grouping key from
application data (e.g., prefer a dedicated order property like
application.p_app_order if available, otherwise fall back to a deterministic key
such as `${application.p_app_date}_${application.p_id}`), then use that key to
initialize and append to record[key].applications and record[key].dates instead
of using the positional index; update references inside mapByOrder (record:
FertAppRecord, applications: ApplicationExtended[], and properties p_app_date
and p_id) to use the new computed key so groups stay aligned across fields.
---
Nitpick comments:
In `@fdm-app/app/components/blocks/rotation/fertilizer-display.tsx`:
- Around line 129-134: In the useMemo dependency array inside
fertilizer-display.tsx, replace the unsafe cast "(cultivation as FieldRow).b_id"
with a conditional expression that only accesses b_id when cultivation.type ===
"field" (e.g. cultivation.type === "field" ? (cultivation as FieldRow).b_id :
undefined) so the dependency accurately reflects the value used and avoids a
silent undefined from an invalid cast.
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx:
- Around line 318-326: The records value is being recomputed on every render by
directly calling groupAndOrderFertApps(fertilizerApplications); wrap that call
in a useMemo to memoize it (similar to canModifyAnything) so records only
recalculates when fertilizerApplications changes; update the code to compute
records = useMemo(() => groupAndOrderFertApps(fertilizerApplications),
[fertilizerApplications]) referencing the existing groupAndOrderFertApps
function and fertilizerApplications data.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
fdm-app/app/components/blocks/fertilizer-applications/form.tsxfdm-app/app/components/blocks/rotation/fertilizer-display.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.rotation_.fertilizer._index.tsx
Comment on lines
+73
to
+77
| const originalFertilizer = await getFertilizer(fdm, params.p_id) | ||
| const fertilizer: FertilizerInfo = { | ||
| p_id: originalFertilizer.p_id, | ||
| p_name_nl: originalFertilizer.p_name_nl, | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "\.ts$" | grep -E "(fertilizer|farm)" | head -20Repository: SvenVw/fdm
Length of output: 966
🏁 Script executed:
rg "export.*function getFertilizer|export.*const getFertilizer" --type ts -A 25Repository: SvenVw/fdm
Length of output: 11055
🏁 Script executed:
cat -n fdm-app/app/routes/farm.\$b_id_farm.\$calendar.rotation.modify_fertilizer.\$p_id.tsx | head -100Repository: SvenVw/fdm
Length of output: 3671
🏁 Script executed:
rg "getFertilizer|checkPermission|principal" fdm-app/app/routes/farm.\$b_id_farm.\$calendar.rotation.modify_fertilizer.\$p_id.tsx -n -B 3 -A 3Repository: SvenVw/fdm
Length of output: 2128
getFertilizer does not perform authorization checks before returning fertilizer metadata.
Any authenticated user can retrieve fertilizer names by calling this endpoint with an arbitrary p_id, regardless of whether they have access to the farm. While this only exposes metadata (fertilizer name) rather than application data, consider either validating that the user has access to the relevant farm/fields before calling getFertilizer, or document that fertilizer metadata is intentionally public to authenticated users.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx
around lines 73 - 77, The code calls getFertilizer(params.p_id) and returns
fertilizer metadata without verifying the caller’s access to the farm, exposing
fertilizer names to any authenticated user; update the flow to perform an
authorization check (e.g., verify the current user has access to the
farm/rotation via the existing farm/rotation auth helper used elsewhere in this
route) before calling getFertilizer, or move the auth into getFertilizer itself
so it validates access for params.p_id, and only then construct and return the
FertilizerInfo (references: getFertilizer, originalFertilizer, params.p_id).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
fdm-app/app/components/blocks/fertilizer-applications/form.tsx (1)
134-137:⚠️ Potential issue | 🟡 MinorFalsy guard skips
setValuewhenp_app_amountis0.
fertilizerApplication?.p_app_amountis falsy for0, so a stored amount of zero would never be restored. Use a null/undefined check instead.🐛 Proposed fix
- if (fertilizerApplication?.p_app_amount) { + if (fertilizerApplication?.p_app_amount != null) { form.setValue("p_app_amount", fertilizerApplication.p_app_amount) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/fertilizer-applications/form.tsx` around lines 134 - 137, The current falsy guard skips restoring p_app_amount when it's 0; change the check in the effect to test for null/undefined instead of truthiness (e.g., check fertilizerApplication?.p_app_amount != null or typeof fertilizerApplication?.p_app_amount !== 'undefined') and then call form.setValue("p_app_amount", fertilizerApplication.p_app_amount) so zero values are preserved; update the conditional around form.setValue in the effect that references fertilizerApplication, p_app_amount and form.setValue.fdm-app/app/routes/farm.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx (2)
155-170: Index-based grouping can misalign applications across fields.This was noted in a prior review.
mapByOrderuses the loop indexias the grouping key. If fields have different numbers of applications, positions shift and unrelated applications get grouped together. Consider grouping by date proximity or another domain-meaningful key.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx around lines 155 - 170, mapByOrder currently uses the loop index (i) to group ApplicationExtended entries which causes misalignment when different fields have different counts; change grouping to use a domain-meaningful key such as the application date instead of the index: inside mapByOrder use application.p_app_date (normalized to a day/ISO string or a date-bucket like startOfDay) as the key (or a composite key like `${application.p_app_date}_${application.p_id}` if needed for uniqueness), ensure FertAppRecord entries are created/merged by that date-key, and push applications and dates into the correct date-bucket so related applications stay grouped regardless of array positions.
59-77:getFertilizerdoes not verify user access to the farm.This was flagged in a previous review.
getFertilizer(fdm, params.p_id)returns metadata for any fertilizer without checking that the user has access to the associated farm. While applications are fetched withsession.principal_id(which is permission-checked), the fertilizer name leaks to any authenticated user who can guess thep_id.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx around lines 59 - 77, The loader currently calls getFertilizer(fdm, params.p_id) without verifying the user has access to the farm; update the loader to enforce access control by either using or adding an access-checked helper (e.g., getFertilizerForFarm or a query that joins fertilizer→farm) that validates params.p_id belongs to params.b_id_farm and that session.principal_id has permission; if the fertilizer is not associated with the requested farm or the principal lacks access, throw an appropriate 404/403 (instead of returning the name), otherwise proceed to construct the FertilizerInfo. Ensure you reference loader, getFertilizer, params.p_id, params.b_id_farm, and session.principal_id when making the change.
🧹 Nitpick comments (5)
fdm-app/app/components/blocks/rotation/fertilizer-display.tsx (2)
82-111: RedundantkeyonNavLinkwhen wrapped inTooltip.The
NavLinkat line 83 carrieskey={fertilizer.p_id}, but whencultivation.type !== "field", the enclosing<Tooltip>at line 117 also haskey={fertilizer.p_id}. React only needs the key on the outermost element returned from the.map(). The inner key is harmless but unnecessary.Also applies to: 114-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/rotation/fertilizer-display.tsx` around lines 82 - 111, The inner key on the NavLink is redundant because the mapped element's outer wrapper (the Tooltip when cultivation.type !== "field") already uses key={fertilizer.p_id}; remove the duplicate key prop from the NavLink (the element rendered by NavLink in fertilizer-display.tsx) so only the outermost returned element in the map retains key={fertilizer.p_id}; keep the Tooltip's key when present and ensure no other child elements in the same map also carry the same key.
62-65:(cultivation as FieldRow).b_idinuseMemodeps isundefinedfor non-field rows.When
cultivation.typeis not"field", casting toFieldRowand accessing.b_idyieldsundefined. The memo still works becauseundefined === undefined, but the assertion is misleading. Consider optional chaining or a conditional:♻️ Suggested improvement
}, [ uniqueFertilizers, - (cultivation as FieldRow).b_id, + cultivation.type === "field" ? cultivation.b_id : undefined, cultivation.type, cultivation.fields, ])Also applies to: 129-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/rotation/fertilizer-display.tsx` around lines 62 - 65, The useMemo dependency currently casts cultivation to FieldRow and reads (cultivation as FieldRow).b_id which is undefined when cultivation.type !== "field"; change the dependency to use a safe conditional or optional chaining instead (e.g., if cultivation.type === "field" use cultivation.b_id else use cultivation.fields.map(f => f.b_id) or the equivalent indexable value) so the dependency reflects the actual value and not an undefined cast; apply the same fix to the other useMemo/dependency at the second occurrence referencing (cultivation as FieldRow).b_id.fdm-app/app/routes/farm.$b_id_farm.$calendar.rotation_.fertilizer._index.tsx (1)
98-114:parseAppIdsthrows on invalid format here but the sibling inmodify_fertilizerroute silently filters.The
parseAppIdsin this file (line 106-108) throwsnew Error(...)on malformed input, while the one inmodify_fertilizer.$p_id.tsx(line 388-392) silently filters out invalid pairs. This inconsistency means the same malformedappIdsquery param causes a 500 in one route and silent data loss in the other. Consider unifying the behavior (preferably in a shared utility).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation_.fertilizer._index.tsx around lines 98 - 114, parseAppIds currently throws on malformed "b_id:p_app_id" entries while the sibling in modify_fertilizer silently filters invalid pairs; make them consistent by extracting a shared utility (e.g., normalizeAppIds or parseAppIds in a shared utils module) that mirrors the modify_fertilizer behavior: split each appId on ":" and skip any entries with length < 2 (optionally log a warning), returning only valid {b_id, p_app_id} objects; update the parseAppIds usage here and in modify_fertilizer.$p_id to call the shared function so both routes behave identically.fdm-app/app/components/blocks/fertilizer-applications/formschema.tsx (2)
38-49: Consider using.optional()instead of.or(z.undefined())for clarity.
.or(z.undefined())creates a union type that still requires the key to be present (but acceptsundefinedas a value), whereas.optional()allows the key to be absent entirely. If the intent is to allow missing keys in partial form submissions,.optional()would be more idiomatic. If keys are always present but values can be undefined (common in form state), the current approach works, but it's a subtle distinction worth documenting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/fertilizer-applications/formschema.tsx` around lines 38 - 49, The FormSchemaPartial creation uses value.or(z.undefined()) which forces keys to exist with undefined values; replace that with value.optional() so keys can be entirely absent for partial submissions. Update the mapping in FormSchemaPartial (the Object.entries(fields).map callback) to use value.optional() for each field, and remove or adjust the explicit cast if needed so the resulting z.object types reflect optional fields; refer to the FormSchemaPartial constant and the fields variable when making the change.
69-71: Type definition partially duplicatesFormSchemaModifyinference.
FieldFertilizerFormValuesmanually addsp_app_id?: string | undefinedon top ofFormSchema's inferred type. SinceFormSchemaModifyalready extendsFormSchemawith a requiredp_app_id, consider deriving this type fromFormSchemaModify(e.g.,Partial<z.infer<typeof FormSchemaModify>>or a mapped type) to keep the source of truth in one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/fertilizer-applications/formschema.tsx` around lines 69 - 71, FieldFertilizerFormValues duplicates the inferred FormSchema type by manually adding p_app_id; instead derive it from FormSchemaModify to keep a single source of truth—replace the manual union with a type based on z.infer<typeof FormSchemaModify> (or Partial<z.infer<typeof FormSchemaModify>> if you want p_app_id optional) so FieldFertilizerFormValues reuses FormSchemaModify rather than extending FormSchema ad-hoc; update the type declaration for FieldFertilizerFormValues accordingly and remove the manual p_app_id addition.
🤖 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-applications/form.tsx`:
- Around line 59-68: The fertilizerApplication prop is typed as
Partial<FieldFertilizerFormValues> but the component accesses
fertilizerApplication?.p_app_ids (plural), which is not declared; to fix, either
add p_app_ids?: string[] to the FieldFertilizerFormValues type or change the
prop typing to accept the broader model (e.g., Partial<FertilizerApplication> |
null | undefined) so p_app_ids is valid; update the declaration referencing
FieldFertilizerFormValues and/or the prop signature in the component to reflect
this change and ensure any usages (lines accessing p_app_ids) compile.
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation_.fertilizer._index.tsx:
- Around line 812-823: The form schema selection incorrectly passes
FormSchemaModify when loaderData.exampleFertilizerApplication is undefined, but
FormSchemaModify requires a p_app_id that does not exist for fresh "add" flows;
update the ternary logic in the component where fertilizerApplication,
exampleFertilizerApplication, and schema are set (referencing loaderData,
FormSchemaModify, FormSchemaPartialModify, fertilizerApplication,
exampleFertilizerApplication) so that when neither exampleFertilizerApplication
nor fertilizerApplication exists you pass undefined for schema (allowing the
form component to use its internal FormSchema fallback), otherwise keep using
FormSchemaPartialModify when example exists and FormSchemaModify only when a
p_app_id is present.
- Around line 224-228: The code unsafe-casts the Promise.all result to
FertilizerApplication[] despite getFertilizerApplication returning
Promise<FertilizerApplication | null>; change the flow to await
Promise.all(applicationRefs.map(...)) into a typed array of
(FertilizerApplication | null)[], then remove nulls with a type guard (e.g.
filter((v): v is FertilizerApplication => v !== null) or reject nulls and throw)
before assigning to fertilizerApplications so fertilizerApplications is truly
FertilizerApplication[]; update any downstream assumptions accordingly and
reference getFertilizerApplication, applicationRefs, and fertilizerApplications
when making the change.
- Around line 864-897: The loop that updates multiple applications fetches the
original via getFertilizerApplication but only preserves the original p_id; to
avoid nulling optional fields when using FormSchemaPartialModify, pass fallbacks
for p_app_amount, p_app_method, and p_app_date into updateFertilizerApplication
(e.g., use validatedData.p_app_amount ?? original.p_app_amount,
validatedData.p_app_method ?? original.p_app_method, validatedData.p_app_date ??
original.p_app_date) so undefined from validatedData does not overwrite existing
values; update the call inside the p_app_ids.map where
updateFertilizerApplication is invoked.
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx:
- Around line 204-211: formatNumberRange currently computes the "single value"
case using lastNumber - firstNumber < firstNumber / 100 which fails when
firstNumber is 0 (e.g., produces "0 - 0" instead of "0"). Update the
formatNumberRange function to treat the values as a single value when
firstNumber === lastNumber or when the difference is within a small relative
tolerance computed from the absolute value (e.g., use Math.abs(firstNumber) as
the denominator or explicitly check equality), and return `${firstNumber}
${unit}` in that case; otherwise return the range `${firstNumber} -
${lastNumber} ${unit}`. Ensure the function still handles the empty array case
unchanged.
---
Duplicate comments:
In `@fdm-app/app/components/blocks/fertilizer-applications/form.tsx`:
- Around line 134-137: The current falsy guard skips restoring p_app_amount when
it's 0; change the check in the effect to test for null/undefined instead of
truthiness (e.g., check fertilizerApplication?.p_app_amount != null or typeof
fertilizerApplication?.p_app_amount !== 'undefined') and then call
form.setValue("p_app_amount", fertilizerApplication.p_app_amount) so zero values
are preserved; update the conditional around form.setValue in the effect that
references fertilizerApplication, p_app_amount and form.setValue.
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx:
- Around line 155-170: mapByOrder currently uses the loop index (i) to group
ApplicationExtended entries which causes misalignment when different fields have
different counts; change grouping to use a domain-meaningful key such as the
application date instead of the index: inside mapByOrder use
application.p_app_date (normalized to a day/ISO string or a date-bucket like
startOfDay) as the key (or a composite key like
`${application.p_app_date}_${application.p_id}` if needed for uniqueness),
ensure FertAppRecord entries are created/merged by that date-key, and push
applications and dates into the correct date-bucket so related applications stay
grouped regardless of array positions.
- Around line 59-77: The loader currently calls getFertilizer(fdm, params.p_id)
without verifying the user has access to the farm; update the loader to enforce
access control by either using or adding an access-checked helper (e.g.,
getFertilizerForFarm or a query that joins fertilizer→farm) that validates
params.p_id belongs to params.b_id_farm and that session.principal_id has
permission; if the fertilizer is not associated with the requested farm or the
principal lacks access, throw an appropriate 404/403 (instead of returning the
name), otherwise proceed to construct the FertilizerInfo. Ensure you reference
loader, getFertilizer, params.p_id, params.b_id_farm, and session.principal_id
when making the change.
---
Nitpick comments:
In `@fdm-app/app/components/blocks/fertilizer-applications/formschema.tsx`:
- Around line 38-49: The FormSchemaPartial creation uses value.or(z.undefined())
which forces keys to exist with undefined values; replace that with
value.optional() so keys can be entirely absent for partial submissions. Update
the mapping in FormSchemaPartial (the Object.entries(fields).map callback) to
use value.optional() for each field, and remove or adjust the explicit cast if
needed so the resulting z.object types reflect optional fields; refer to the
FormSchemaPartial constant and the fields variable when making the change.
- Around line 69-71: FieldFertilizerFormValues duplicates the inferred
FormSchema type by manually adding p_app_id; instead derive it from
FormSchemaModify to keep a single source of truth—replace the manual union with
a type based on z.infer<typeof FormSchemaModify> (or Partial<z.infer<typeof
FormSchemaModify>> if you want p_app_id optional) so FieldFertilizerFormValues
reuses FormSchemaModify rather than extending FormSchema ad-hoc; update the type
declaration for FieldFertilizerFormValues accordingly and remove the manual
p_app_id addition.
In `@fdm-app/app/components/blocks/rotation/fertilizer-display.tsx`:
- Around line 82-111: The inner key on the NavLink is redundant because the
mapped element's outer wrapper (the Tooltip when cultivation.type !== "field")
already uses key={fertilizer.p_id}; remove the duplicate key prop from the
NavLink (the element rendered by NavLink in fertilizer-display.tsx) so only the
outermost returned element in the map retains key={fertilizer.p_id}; keep the
Tooltip's key when present and ensure no other child elements in the same map
also carry the same key.
- Around line 62-65: The useMemo dependency currently casts cultivation to
FieldRow and reads (cultivation as FieldRow).b_id which is undefined when
cultivation.type !== "field"; change the dependency to use a safe conditional or
optional chaining instead (e.g., if cultivation.type === "field" use
cultivation.b_id else use cultivation.fields.map(f => f.b_id) or the equivalent
indexable value) so the dependency reflects the actual value and not an
undefined cast; apply the same fix to the other useMemo/dependency at the second
occurrence referencing (cultivation as FieldRow).b_id.
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation_.fertilizer._index.tsx:
- Around line 98-114: parseAppIds currently throws on malformed "b_id:p_app_id"
entries while the sibling in modify_fertilizer silently filters invalid pairs;
make them consistent by extracting a shared utility (e.g., normalizeAppIds or
parseAppIds in a shared utils module) that mirrors the modify_fertilizer
behavior: split each appId on ":" and skip any entries with length < 2
(optionally log a warning), returning only valid {b_id, p_app_id} objects;
update the parseAppIds usage here and in modify_fertilizer.$p_id to call the
shared function so both routes behave identically.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
fdm-app/app/components/blocks/fertilizer-applications/form.tsxfdm-app/app/components/blocks/fertilizer-applications/formschema.tsxfdm-app/app/components/blocks/rotation/fertilizer-display.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.rotation_.fertilizer._index.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
fdm-app/app/components/blocks/fertilizer-applications/form.tsx (1)
142-146:⚠️ Potential issue | 🟡 MinorFalsy guard skips
setValuewhenp_app_amountis0.The condition
fertilizerApplication?.p_app_amountis falsy for0, so a legitimately stored amount of zero would never be restored. Use a null/undefined check instead.🐛 Proposed fix
- if (fertilizerApplication?.p_app_amount) { + if (fertilizerApplication?.p_app_amount != null) { form.setValue("p_app_amount", fertilizerApplication.p_app_amount) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/fertilizer-applications/form.tsx` around lines 142 - 146, The useEffect currently skips setting p_app_amount when its value is 0 because it uses a truthy check; change the guard to explicitly test for null/undefined (e.g. check fertilizerApplication exists and fertilizerApplication.p_app_amount != null) before calling form.setValue("p_app_amount", fertilizerApplication.p_app_amount) inside the same useEffect so zero values are restored correctly; update the condition that references fertilizerApplication?.p_app_amount to an explicit null/undefined check.
🧹 Nitpick comments (2)
fdm-app/app/routes/farm.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx (1)
255-267: Misleading filter parameter name:dateshould bemethod.Line 263 filters application method names, but the callback parameter is named
date.✏️ Proposed fix
- .filter((date) => date !== null), + .filter((method) => method !== null),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx around lines 255 - 267, The filter callback in the applicationMethods construction uses a misleading parameter name `date`; rename it to `method` (or similar) to reflect that it's filtering application method strings, and ensure the logic remains .filter((method) => method !== null) after mapping application.p_app_method_name ?? application.p_app_method so the intent is clear; update the map/filter arrow functions in the applicationMethods expression that operate on record.applications accordingly.fdm-app/app/components/blocks/fertilizer-applications/form.tsx (1)
226-262: Inconsistentcontrolprop onControllercomponents.The
p_app_dateController (Line 315) explicitly passescontrol={form.control}, whilep_app_methodandp_app_amountControllers rely on the implicit context fromRemixFormProvider. Both approaches work, but pick one for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/fertilizer-applications/form.tsx` around lines 226 - 262, The Controllers are inconsistent: p_app_date passes control={form.control} while p_app_method and p_app_amount rely on RemixFormProvider context; make them consistent by explicitly passing control={form.control} to the p_app_method and p_app_amount Controller components (or alternatively remove the explicit prop from p_app_date), referencing the Controller elements named "p_app_method" and "p_app_amount" and the existing "p_app_date" usage so all Controllers uniformly use form.control from the same source (form.control) rather than mixing implicit context and explicit props.
🤖 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.$calendar.rotation_.fertilizer._index.tsx:
- Around line 425-426: The loader exposes canReselect (set in loadByAppIds) but
the component still renders the "Wijzig selectie" button and field selection
dialog even when canReselect is false; update the component to read canReselect
and conditionally hide or disable the selection UI (the "Wijzig selectie" button
and the field selection dialog rendering) when canReselect === false so users in
edit flow cannot change fields; ensure any handlers that open the dialog or
mutate selected field IDs (the click handler for the button and the selection
submit handler) are also disabled/guarded when canReselect is false to avoid
inconsistent appId-bound form state.
- Around line 394-407: cultivationNames is empty when navigation supplies appIds
but no cultivationIds, causing the page title to render "Bemesting toevoegen aan
" — fix by falling back to the fertilizer application(s) referenced by appIds
when cultivationIds is empty: in the logic that computes
cultivationIds/cultivationNames (symbols: cultivationIds, cultivationNames,
catalogueCultivations, appIds), detect if cultivationIds is empty and appIds are
present, resolve those application objects (or their cultivation/catalogue id
field) and derive b_lu_catalogue -> b_lu_name via catalogueCultivations to
populate cultivationNames; additionally ensure the title rendering code
gracefully handles an empty cultivationNames (avoid trailing space) when no
fallback is available.
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx:
- Around line 199-203: The current mapping converts record keys to numbers using
Number.parseFloat when building entries (const entries =
Object.entries(record).map(...)) which yields NaN for UUID keys (e.g.,
mapEach/p_app_id) and makes entries.sort unstable; change the logic to keep the
key as a string when parseFloat yields NaN and sort using a comparator that uses
numeric comparison when both keys are valid numbers and falls back to
locale/string comparison otherwise (or simply always treat keys as strings for
lexicographic order when not numeric). Locate the entries creation and sort (the
Object.entries(record).map(...) and entries.sort(...) lines) and implement the
numeric-then-string fallback comparator so UUID keys sort deterministically.
- Around line 385-387: Replace the typo in the TabsTrigger label that currently
reads "Niet groupen" with the correct Dutch word "Niet groeperen"; locate the
element with value="mapEach" (the TabsTrigger component) and update its
displayed text accordingly so the UI shows "Niet groeperen".
---
Duplicate comments:
In `@fdm-app/app/components/blocks/fertilizer-applications/form.tsx`:
- Around line 142-146: The useEffect currently skips setting p_app_amount when
its value is 0 because it uses a truthy check; change the guard to explicitly
test for null/undefined (e.g. check fertilizerApplication exists and
fertilizerApplication.p_app_amount != null) before calling
form.setValue("p_app_amount", fertilizerApplication.p_app_amount) inside the
same useEffect so zero values are restored correctly; update the condition that
references fertilizerApplication?.p_app_amount to an explicit null/undefined
check.
---
Nitpick comments:
In `@fdm-app/app/components/blocks/fertilizer-applications/form.tsx`:
- Around line 226-262: The Controllers are inconsistent: p_app_date passes
control={form.control} while p_app_method and p_app_amount rely on
RemixFormProvider context; make them consistent by explicitly passing
control={form.control} to the p_app_method and p_app_amount Controller
components (or alternatively remove the explicit prop from p_app_date),
referencing the Controller elements named "p_app_method" and "p_app_amount" and
the existing "p_app_date" usage so all Controllers uniformly use form.control
from the same source (form.control) rather than mixing implicit context and
explicit props.
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx:
- Around line 255-267: The filter callback in the applicationMethods
construction uses a misleading parameter name `date`; rename it to `method` (or
similar) to reflect that it's filtering application method strings, and ensure
the logic remains .filter((method) => method !== null) after mapping
application.p_app_method_name ?? application.p_app_method so the intent is
clear; update the map/filter arrow functions in the applicationMethods
expression that operate on record.applications accordingly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
fdm-app/app/components/blocks/fertilizer-applications/form.tsxfdm-app/app/components/blocks/fertilizer-applications/formschema.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.rotation_.fertilizer._index.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- fdm-app/app/components/blocks/fertilizer-applications/formschema.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
fdm-app/app/routes/farm.$b_id_farm.$calendar.rotation_.fertilizer._index.tsx (2)
614-783:⚠️ Potential issue | 🟡 MinorDisable or hide selection UI when
canReselectis false.In the appIds edit flow, reselection is intentionally disabled but the dialog is still accessible. Consider gating the “Wijzig selectie” button and dialog on
canReselect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation_.fertilizer._index.tsx around lines 614 - 783, The selection dialog and its trigger should be gated by the canReselect flag: prevent access to the Dialog when canReselect is false by either not rendering the DialogTrigger/Button or rendering the Button as disabled and ensuring handleSelectionDialogOpenChange / open state cannot open the dialog; update the block that renders Dialog, DialogTrigger and the "Wijzig selectie" Button to check canReselect (use canReselect to conditionally render or set disabled on Button) and short-circuit any calls to handleSelectionDialogOpenChange so the Dialog never opens when canReselect is false.
514-514:⚠️ Potential issue | 🟡 MinorAvoid an empty title when
cultivationNamesis empty (appIds flow).Right now the title becomes
Bemesting toevoegen aanwith a trailing space. Add a fallback (e.g., omit “aan …” or derive names from appIds).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation_.fertilizer._index.tsx at line 514, The title string uses loaderData.cultivationNames directly and produces a trailing space when it's empty; update the title generation (where title={`Bemesting toevoegen aan ${loaderData.cultivationNames.join(", ")}`}) to conditionally include the "aan ..." suffix only when loaderData.cultivationNames.length > 0, otherwise use a fallback like "Bemesting toevoegen" (or build names from loaderData.appIds if available) so the title never ends with an empty "aan ".fdm-app/app/routes/farm.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx (1)
228-233:⚠️ Potential issue | 🟡 MinorIndex-based grouping can misalign applications across fields.
Grouping by positional index risks pairing unrelated applications when fields have differing counts or gaps. Consider grouping by a domain key (date proximity, same-day bucket, etc.).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx around lines 228 - 233, The current mappers.object uses mapByOrder (createMapper((_application, i) => i.toString())) which groups by positional index and can misalign applications across fields; replace or deprecate mapByOrder and implement a domain-key mapper (e.g., mapByDateBucket) that uses application.p_app_date normalized to a deterministic bucket (same-day start timestamp or proximity bucket) so applications occurring on the same day or within the same window are grouped together; update references to use mappers.mapByDate or the new mappers.mapByDateBucket and ensure createMapper/mapEach consume the normalized key consistently.
🤖 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.$calendar.rotation_.fertilizer._index.tsx:
- Around line 194-200: fieldIds may be null which causes new Set(fieldIds) to
throw; change construction of fieldIdsSet to use a safe fallback (e.g., const
fieldIdsSet = new Set(fieldIds ?? [])) and ensure selectedFieldIds logic still
uses that set to filter fieldsWithCultivation.map(cultivation =>
cultivation.b_id) so selection works when fieldIds is absent.
- Around line 219-297: The returned fields and selectedFieldIds can contain
duplicates when multiple applicationRefs map to the same b_id; deduplicate by
b_id before returning so UI counts and lists aren't inflated. After resolving
applicationRefs/getField (variables: fields, applicationRefs, getField), build a
uniqueFields collection (e.g., map by b_id) and return that instead of fields,
and set selectedFieldIds from uniqueFields.map(f => f.b_id); keep appIds as-is
(fertilizerApplications.map(app => app.p_app_id)) and ensure types still satisfy
StrategizedLoaderData["fields"].
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx:
- Around line 352-395: The percelen cell is being conditionally wrapped in a
nested <TableCell> when columnVisibility.count is true and omitted entirely when
false; fix by always rendering one dedicated <TableCell> for the percelen list
(use fieldNames rendering or the single-field fallback) and remove the
inner/nested <TableCell>, then render a separate sibling <TableCell>
(conditionally) for the count (matching the “Aantal Bemestingen” header) that
displays record.applications.length; update the TableRow code that uses
TableCell, columnVisibility.count, fieldNames and record.applications
accordingly and ensure a safe fallback when fieldNames is empty.
---
Duplicate comments:
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation_.fertilizer._index.tsx:
- Around line 614-783: The selection dialog and its trigger should be gated by
the canReselect flag: prevent access to the Dialog when canReselect is false by
either not rendering the DialogTrigger/Button or rendering the Button as
disabled and ensuring handleSelectionDialogOpenChange / open state cannot open
the dialog; update the block that renders Dialog, DialogTrigger and the "Wijzig
selectie" Button to check canReselect (use canReselect to conditionally render
or set disabled on Button) and short-circuit any calls to
handleSelectionDialogOpenChange so the Dialog never opens when canReselect is
false.
- Line 514: The title string uses loaderData.cultivationNames directly and
produces a trailing space when it's empty; update the title generation (where
title={`Bemesting toevoegen aan ${loaderData.cultivationNames.join(", ")}`}) to
conditionally include the "aan ..." suffix only when
loaderData.cultivationNames.length > 0, otherwise use a fallback like "Bemesting
toevoegen" (or build names from loaderData.appIds if available) so the title
never ends with an empty "aan ".
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx:
- Around line 228-233: The current mappers.object uses mapByOrder
(createMapper((_application, i) => i.toString())) which groups by positional
index and can misalign applications across fields; replace or deprecate
mapByOrder and implement a domain-key mapper (e.g., mapByDateBucket) that uses
application.p_app_date normalized to a deterministic bucket (same-day start
timestamp or proximity bucket) so applications occurring on the same day or
within the same window are grouped together; update references to use
mappers.mapByDate or the new mappers.mapByDateBucket and ensure
createMapper/mapEach consume the normalized key consistently.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fdm-app/app/routes/farm.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.rotation_.fertilizer._index.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-applications/table.tsx`:
- Line 175: The TableHeader element (the <TableHeader> component) uses the
"sticky" class but lacks a positioning offset; update its className to include
"top-0" (e.g., className="sticky top-0" or merge into existing classes) so the
thead will correctly stick to the top during scroll; ensure you modify the
TableHeader invocation in fertilizer-applications/table.tsx to include top-0
alongside any other classes.
- Around line 163-171: The table uses useReactTable with
initialState.columnVisibility which only applies on first render, so when the
memoized columnVisibility (computed from numFields, fertilizerApplications,
records) changes the table won't update; switch to a controlled state: create a
React state (e.g., columnVisibilityState via useState) initialized from the
computed columnVisibility, update that state whenever the computed
columnVisibility changes, and pass it into useReactTable via the state prop
(state: { columnVisibility: columnVisibilityState }) and an onStateChange
handler to keep it in sync; update references to initialState in the
useReactTable call and ensure the state setter name (e.g.,
setColumnVisibilityState) and the computed columnVisibility are used to drive
updates.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fdm-app/app/components/blocks/fertilizer-applications/columns.tsxfdm-app/app/components/blocks/fertilizer-applications/table.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- fdm-app/app/components/blocks/fertilizer-applications/columns.tsx
Contributor
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 (1)
fdm-app/app/routes/farm.$b_id_farm.$calendar.rotation_.fertilizer._index.tsx (1)
864-875:⚠️ Potential issue | 🟡 MinorMake the form card title mode-aware.
The card title stays “Bemesting toevoegen” even in modify mode, while the rest of the page switches to “wijzigen”. This creates conflicting UI state.
💡 Proposed fix
- <CardTitle>Bemesting toevoegen</CardTitle> + <CardTitle> + {isModification + ? "Bemesting wijzigen" + : "Bemesting toevoegen"} + </CardTitle>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation_.fertilizer._index.tsx around lines 864 - 875, The CardTitle is hardcoded to "Bemesting toevoegen" while the rest of the UI uses isModification to show "wijzigen"; update the CardTitle usage so it reads conditionally based on isModification (e.g., use isModification ? "Bemesting wijzigen" : "Bemesting toevoegen") to match the CardDescription and ensure consistent mode-aware labeling; locate the CardTitle component near the CardDescription block that references loaderData.fieldAmount and isModification and replace the static string accordingly.
🧹 Nitpick comments (1)
fdm-app/app/components/blocks/fertilizer-applications/columns.tsx (1)
82-93: Avoid using a hook inside the inline tablecellcallback.This pattern is brittle with React Hooks linting and harder to maintain. The
useMemohere recalculates on every render sincerow.originalis the dependency and likely changes frequently in table updates. The computation is simple and pure—it doesn't need memoization and isn't passed to memoized children. Extract as a plain computation instead.♻️ Proposed fix (remove hook from inline callback)
- const fieldNames = useMemo( - () => - Object.entries( - Object.fromEntries( - row.original.applications.map((app) => [ - app.b_id, - app.b_name, - ]), - ), - ).sort((a, b) => (a[1] < b[1] ? -1 : a[1] > b[1] ? 1 : 0)), - [row.original], - ) + const fieldNames = Object.entries( + Object.fromEntries( + row.original.applications.map((app) => [ + app.b_id, + app.b_name, + ]), + ), + ).sort((a, b) => (a[1] < b[1] ? -1 : a[1] > b[1] ? 1 : 0))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/fertilizer-applications/columns.tsx` around lines 82 - 93, The inline cell callback uses useMemo to compute fieldNames which violates hook rules and is unnecessary; replace the useMemo call with a plain synchronous computation that derives fieldNames from row.original.applications (e.g., build the Object.fromEntries/map sort result directly) and remove the dependency on useMemo so the computation runs inline inside the cell callback instead of calling useMemo; update references to fieldNames accordingly and ensure no hooks remain inside the cell renderer.
🤖 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-applications/columns.tsx`:
- Around line 38-45: formatDateRange currently uses the first and last entries
of the provided dates array (which reflects raw application order) and can
therefore show an incorrect range; update formatDateRange to compute the min and
max by sorting or by finding the earliest and latest timestamps (e.g., map dates
to getTime() and use Math.min/Math.max or sort a copied array) before formatting
so the displayed range is always from the earliest to the latest date; apply the
same fix to the other identical date-range logic present in the file (the second
occurrence around the other formatter).
- Around line 124-125: The current access to fieldNames[0][1] will throw when
applications is empty; update the code in columns.tsx to guard that single-field
fallback by checking that fieldNames and its first element exist (or that
applications.length > 0) before using [0][1], and provide a safe default (e.g.,
empty string or a localized placeholder) when the value is missing; locate the
usage of fieldNames and applications in the component (the fallback expression
referencing fieldNames[0][1]) and replace it with a guarded access that returns
the default when fieldNames[0] or its index 1 is undefined.
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation_.fertilizer._index.tsx:
- Around line 990-1015: The fieldIds array (derived via url.searchParams.get and
split/filter) can contain duplicates which causes duplicate calls to
addFertilizerApplication inside fdm.transaction; deduplicate fieldIds after
parsing (e.g., Array.from(new Set(...)) or similar) before the existence check
and before mapping to addFertilizerApplication, then use the deduplicated array
for the transaction and for the success message so the count reflects unique
fields; ensure dataWithError still triggers when the deduplicated list is empty
and that redirectWithSuccess uses deduped.length for the message.
---
Outside diff comments:
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation_.fertilizer._index.tsx:
- Around line 864-875: The CardTitle is hardcoded to "Bemesting toevoegen" while
the rest of the UI uses isModification to show "wijzigen"; update the CardTitle
usage so it reads conditionally based on isModification (e.g., use
isModification ? "Bemesting wijzigen" : "Bemesting toevoegen") to match the
CardDescription and ensure consistent mode-aware labeling; locate the CardTitle
component near the CardDescription block that references loaderData.fieldAmount
and isModification and replace the static string accordingly.
---
Nitpick comments:
In `@fdm-app/app/components/blocks/fertilizer-applications/columns.tsx`:
- Around line 82-93: The inline cell callback uses useMemo to compute fieldNames
which violates hook rules and is unnecessary; replace the useMemo call with a
plain synchronous computation that derives fieldNames from
row.original.applications (e.g., build the Object.fromEntries/map sort result
directly) and remove the dependency on useMemo so the computation runs inline
inside the cell callback instead of calling useMemo; update references to
fieldNames accordingly and ensure no hooks remain inside the cell renderer.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fdm-app/app/components/blocks/fertilizer-applications/columns.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.rotation_.fertilizer._index.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
fdm-app/app/components/blocks/fertilizer-applications/columns.tsx (2)
38-45:⚠️ Potential issue | 🟡 MinorCompute date range from min/max instead of input order.
Line 40 and Line 41 assume the array is already sorted, which can show an incorrect range when application dates arrive in arbitrary order.
💡 Proposed fix
function formatDateRange(dates: Date[]) { if (dates.length === 0) return "" - const firstDate = dates[0] - const lastDate = dates[dates.length - 1] + const sorted = [...dates].sort((a, b) => a.getTime() - b.getTime()) + const firstDate = sorted[0] + const lastDate = sorted[sorted.length - 1] return firstDate.getTime() === lastDate.getTime() ? `${format(firstDate, "PP", { locale: nl })}` : `${format(firstDate, "PP", { locale: nl })} - ${format(lastDate, "PP", { locale: nl })}` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/fertilizer-applications/columns.tsx` around lines 38 - 45, formatDateRange assumes the input array is sorted and uses the first and last elements, which can produce wrong ranges; update formatDateRange to compute the minimum and maximum dates from the array (e.g., map to getTime() and use Math.min/Math.max or find min/max by iteration) and then use those min/max Date objects for the equality check and formatting so the displayed range is correct regardless of input order; keep the function name formatDateRange and preserve the same return behavior for empty and single-date cases.
124-125:⚠️ Potential issue | 🟡 MinorGuard the single-field fallback for empty rows.
Line 124 can throw when
applicationsis empty andfieldNames[0]is undefined.🛡️ Proposed fix
- fieldNames[0][1] + fieldNames[0]?.[1] ?? ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/fertilizer-applications/columns.tsx` around lines 124 - 125, The code accesses fieldNames[0][1] unguarded which will throw when applications is empty because fieldNames[0] can be undefined; update the usage (where columns/rows are built) to safely access the fallback field name—for example replace direct access to fieldNames[0][1] with a guarded expression like fieldNames[0]?.[1] ?? '' (or an explicit conditional that checks applications.length > 0 or fieldNames[0] != null) so the component returns a safe default instead of throwing; ensure you update the occurrence referencing fieldNames and the single-field fallback logic so empty applications do not cause an exception.
🤖 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.$calendar.field.fertilizer._index.tsx:
- Line 364: The BreadcrumbItem is being rendered empty; update the
BreadcrumbItem in the fertilizer route component (the <BreadcrumbItem
...>{}</BreadcrumbItem> instance) to render the current label (e.g., replace {}
with the variable holding the title or mode label such as title or modeLabel) so
the breadcrumb shows the current mode/title instead of being blank.
- Around line 586-593: The current returnUrl logic trusts returnUrlParam via
isOfOrigin but that helper allows protocol-relative URLs like //evil.tld; update
the validation to reject protocol-relative values and ensure the resolved origin
strictly equals url.origin: if returnUrlParam is present, first reject it if it
starts with "//", otherwise construct new URL(returnUrlParam, url.origin) and
check urlObj.origin === url.origin before using it; if validation fails, fall
back to the existing create/default paths (use the same ternary that builds
`/farm/create/${b_id_farm}/${calendar}/rotation` or
`/farm/${b_id_farm}/${calendar}/rotation`). Reference returnUrlParam, returnUrl,
isOfOrigin, url.origin, and the existing create/default returnUrl branches.
---
Duplicate comments:
In `@fdm-app/app/components/blocks/fertilizer-applications/columns.tsx`:
- Around line 38-45: formatDateRange assumes the input array is sorted and uses
the first and last elements, which can produce wrong ranges; update
formatDateRange to compute the minimum and maximum dates from the array (e.g.,
map to getTime() and use Math.min/Math.max or find min/max by iteration) and
then use those min/max Date objects for the equality check and formatting so the
displayed range is correct regardless of input order; keep the function name
formatDateRange and preserve the same return behavior for empty and single-date
cases.
- Around line 124-125: The code accesses fieldNames[0][1] unguarded which will
throw when applications is empty because fieldNames[0] can be undefined; update
the usage (where columns/rows are built) to safely access the fallback field
name—for example replace direct access to fieldNames[0][1] with a guarded
expression like fieldNames[0]?.[1] ?? '' (or an explicit conditional that checks
applications.length > 0 or fieldNames[0] != null) so the component returns a
safe default instead of throwing; ensure you update the occurrence referencing
fieldNames and the single-field fallback logic so empty applications do not
cause an exception.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fdm-app/app/components/blocks/fertilizer-applications/columns.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.field.fertilizer._index.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
fdm-app/app/routes/farm.$b_id_farm.$calendar.field.fertilizer._index.tsx (1)
597-603:⚠️ Potential issue | 🔴 Critical
returnUrlvalidation is still vulnerable to unsafe redirect targets.Current validation trusts
isOfOrigin, which accepts values without//and can allow non-path schemes. Normalize throughnew URL(..., url.origin)and only allow same-originhttp/httpspath redirects.🔒 Suggested hardening
- const returnUrlParam = url.searchParams.get("returnUrl") - const returnUrl = - returnUrlParam && isOfOrigin(returnUrlParam, url.origin) - ? returnUrlParam - : url.searchParams.has("create") - ? `/farm/create/${b_id_farm}/${calendar}/rotation` - : `/farm/${b_id_farm}/${calendar}/rotation` + const returnUrlParam = url.searchParams.get("returnUrl") + const safeReturnUrl = (() => { + if (!returnUrlParam) return undefined + if (returnUrlParam.startsWith("//")) return undefined + try { + const target = new URL(returnUrlParam, url.origin) + if (target.origin !== url.origin) return undefined + if (!["http:", "https:"].includes(target.protocol)) + return undefined + return `${target.pathname}${target.search}${target.hash}` + } catch { + return undefined + } + })() + const returnUrl = + safeReturnUrl ?? + (url.searchParams.has("create") + ? `/farm/create/${b_id_farm}/${calendar}/rotation` + : `/farm/${b_id_farm}/${calendar}/rotation`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.field.fertilizer._index.tsx around lines 597 - 603, The returnUrl logic allows unsafe redirects because isOfOrigin can accept schemes without '//' and non-http(s) schemes; update the handling of returnUrlParam used to set returnUrl by attempting to construct new URL(returnUrlParam, url.origin) and then only accept it if the resulting URL.origin === url.origin and the protocol is 'http:' or 'https:'; otherwise fall back to the existing create or farm rotation paths. Replace the current isOfOrigin check with this normalized URL validation around the variables returnUrlParam and returnUrl so only same-origin http/https path redirects are permitted.
🧹 Nitpick comments (1)
fdm-app/app/components/blocks/fertilizer-applications/columns.tsx (1)
176-176: Avoid hard-castingtable.options.metainModifyCell.Line [176] assumes
metaalways exists; if this column config is reused withoutmeta, rendering will throw. A guarded read is safer.♻️ Proposed fix
- const returnUrl = (table.options.meta as { returnUrl: string }).returnUrl + const returnUrl = + (table.options.meta as { returnUrl?: string } | undefined) + ?.returnUrl ?? ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/fertilizer-applications/columns.tsx` at line 176, In ModifyCell (columns.tsx) avoid hard-casting table.options.meta; instead read it defensively — check/table.options.meta existence (use optional chaining or a safe type guard) and provide a sensible default for returnUrl before using it so rendering won’t throw when meta is absent or returnUrl is undefined.
🤖 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-applications/columns.tsx`:
- Around line 89-120: The conditional currently checks
row.original.applications.length to decide whether to show the DropdownMenu,
which incorrectly treats multiple applications on the same parcel as multiple
parcels; change the decision to use the unique parcel count by checking
fieldNames.length (i.e., use fieldNames.length > 1) so when there is only one
unique parcel you render the parcel name directly via fieldNames[0]?.[1] ?? ""
and only show the DropdownMenu when fieldNames.length > 1; update any displayed
text that relied on applications.length (e.g., the "(1 perceel)" branch) to
reflect fieldNames.length instead.
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.field.fertilizer._index.tsx:
- Around line 660-674: The transaction started with fdm.transaction(...) when
creating fertilizer applications is not awaited, causing a race before the
subsequent redirect; update the add branch to await the transaction promise
(i.e., prepend await to the fdm.transaction(...) call that wraps
addFertilizerApplication) so the transaction commits or throws before
proceeding, keeping the same arguments (tx, session.principal_id, fieldIds →
addFertilizerApplication) and matching the pattern used in the modify branch.
---
Duplicate comments:
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.field.fertilizer._index.tsx:
- Around line 597-603: The returnUrl logic allows unsafe redirects because
isOfOrigin can accept schemes without '//' and non-http(s) schemes; update the
handling of returnUrlParam used to set returnUrl by attempting to construct new
URL(returnUrlParam, url.origin) and then only accept it if the resulting
URL.origin === url.origin and the protocol is 'http:' or 'https:'; otherwise
fall back to the existing create or farm rotation paths. Replace the current
isOfOrigin check with this normalized URL validation around the variables
returnUrlParam and returnUrl so only same-origin http/https path redirects are
permitted.
---
Nitpick comments:
In `@fdm-app/app/components/blocks/fertilizer-applications/columns.tsx`:
- Line 176: In ModifyCell (columns.tsx) avoid hard-casting table.options.meta;
instead read it defensively — check/table.options.meta existence (use optional
chaining or a safe type guard) and provide a sensible default for returnUrl
before using it so rendering won’t throw when meta is absent or returnUrl is
undefined.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
fdm-app/app/components/blocks/fertilizer-applications/columns.tsxfdm-app/app/lib/url-utils.tsfdm-app/app/routes/farm.$b_id_farm.$calendar.field.fertilizer._index.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
fdm-app/app/components/blocks/fertilizer-applications/columns.tsx (1)
143-143: Misleading parameter name:dateshould bemethod.The filter callback parameter is named
date, but this is filtering application methods, not dates.✏️ Proposed fix
- .filter((date) => date !== null), + .filter((method) => method !== null),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/fertilizer-applications/columns.tsx` at line 143, Rename the misleading filter callback parameter in the .filter call inside columns.tsx from date to method so it accurately reflects what's being filtered (application methods); update the .filter((date) => date !== null) invocation to .filter((method) => method !== null) (or equivalent) where the surrounding logic uses application methods to keep names consistent with the nearby mapping/usage.
🤖 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-applications/columns.tsx`:
- Around line 97-105: The inner ternary rendering "(1 perceel)" inside the
DropdownMenu branch is dead code because the outer condition checks
fieldNames.length > 1; update the JSX in the DropdownMenu (DropdownMenuTrigger /
Button block) to remove the unreachable fieldNames.length === 1 branch and
simply render the plural form using `(${fieldNames.length} percelen)` (or move
the singular "(1 perceel)" case to the code path that handles fieldNames.length
=== 1 outside the DropdownMenu), ensuring you only reference the fieldNames
variable and keep DropdownMenu, DropdownMenuTrigger and Button unchanged
otherwise.
In `@fdm-app/app/lib/url-utils.ts`:
- Around line 74-75: The current check (!href.includes("//") || new
URL(href).origin === origin) allows non-HTTP schemes like javascript: or data:
to bypass origin validation; update the validation used before calling
redirectWithSuccess to explicitly allow either (a) safe relative paths (e.g.,
start with "/" or "./" or "../" or do not contain a ":" before any "/") or (b)
absolute URLs with http or https schemes whose origin equals origin, by parsing
href with URL inside a try/catch and rejecting anything with non-http(s) schemes
(including data:, javascript:, mailto:, etc.); ensure the resulting boolean
logic is used where redirectWithSuccess is called and centralize this check in
url-utils (referencing href and origin) so redirects only occur for validated
http(s) same-origin or safe relative URLs.
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.field.fertilizer._index.tsx:
- Around line 103-110: The selected-field computation currently uses URL b_id
values when applicationRefs exist, causing the UI to reflect tampered query
params; change the logic that sets fieldIds to use the fetched application
records instead of url.searchParams when applicationRefs is non-empty — derive
fieldIds from applicationRefs.map(ref => ref.b_id) (or the actual field id
property on the fetched application objects) so the modify-mode selection
matches the real applications returned by the p_app_id fetch (update the block
that defines fieldIds and any other occurrences around where applicationRefs is
used, e.g., the similar logic at the other occurrences noted).
---
Nitpick comments:
In `@fdm-app/app/components/blocks/fertilizer-applications/columns.tsx`:
- Line 143: Rename the misleading filter callback parameter in the .filter call
inside columns.tsx from date to method so it accurately reflects what's being
filtered (application methods); update the .filter((date) => date !== null)
invocation to .filter((method) => method !== null) (or equivalent) where the
surrounding logic uses application methods to keep names consistent with the
nearby mapping/usage.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
fdm-app/app/components/blocks/fertilizer-applications/columns.tsxfdm-app/app/components/blocks/fertilizer-applications/table.tsxfdm-app/app/lib/url-utils.tsfdm-app/app/routes/farm.$b_id_farm.$calendar.field.fertilizer._index.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- fdm-app/app/components/blocks/fertilizer-applications/table.tsx
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
fdm-app/app/routes/farm.$b_id_farm.$calendar.field.fertilizer._index.tsx (1)
192-208:⚠️ Potential issue | 🟠 MajorDerive selected
fieldIdsfrom fetched applications, not query fragments.At Line [207],
fieldIdsis still built from URL-providedb_idvalues (applicationRefs). A tampered query can show the wrong selected parcels in modify mode while updates still target real records byp_app_id. UsefertilizerApplicationsas the source of truth.💡 Suggested fix
- fieldIds = [...new Set(applicationRefs.map((ref) => ref.b_id))] + fieldIds = [ + ...new Set( + fertilizerApplications.map( + (application) => application.b_id, + ), + ), + ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.field.fertilizer._index.tsx around lines 192 - 208, The selected fieldIds are being derived from the untrusted applicationRefs (URL query) instead of the actual fetched fertilizerApplications; change the assignment so fieldIds is computed from fertilizerApplications (e.g., map over fertilizerApplications to extract b_id and dedupe) after the Promise.all completes, keeping getFertilizerApplication and p_app_id usage intact so the UI reflects the real records fetched rather than the query fragments.
🧹 Nitpick comments (1)
fdm-app/app/lib/url-utils.ts (1)
59-60: Clarify the condition wording in the docstring.Line 59 currently reads as “no protocol or
//”, which is ambiguous. The function actually skips origin checks only when neither scheme nor protocol-relative prefix is present.✏️ Suggested wording update
- * origin given. No checks are performed if no protocol or `//` is detected in the URL. + * origin given. No origin checks are performed if neither a URI scheme nor a + * protocol-relative prefix (`//`) is detected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/lib/url-utils.ts` around lines 59 - 60, Update the docstring in url-utils.ts to clarify the conditional wording: replace “no protocol or `//`” with wording that makes the intent explicit (e.g., “neither a scheme (protocol) nor a protocol-relative `//` prefix”) so it correctly states that origin checks are skipped only when both scheme and protocol-relative prefix are absent; locate and edit the doc comment above the function in url-utils.ts that describes origin detection to use the new phrasing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.field.fertilizer._index.tsx:
- Around line 192-208: The selected fieldIds are being derived from the
untrusted applicationRefs (URL query) instead of the actual fetched
fertilizerApplications; change the assignment so fieldIds is computed from
fertilizerApplications (e.g., map over fertilizerApplications to extract b_id
and dedupe) after the Promise.all completes, keeping getFertilizerApplication
and p_app_id usage intact so the UI reflects the real records fetched rather
than the query fragments.
---
Nitpick comments:
In `@fdm-app/app/lib/url-utils.ts`:
- Around line 59-60: Update the docstring in url-utils.ts to clarify the
conditional wording: replace “no protocol or `//`” with wording that makes the
intent explicit (e.g., “neither a scheme (protocol) nor a protocol-relative `//`
prefix”) so it correctly states that origin checks are skipped only when both
scheme and protocol-relative prefix are absent; locate and edit the doc comment
above the function in url-utils.ts that describes origin detection to use the
new phrasing.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
fdm-app/app/components/blocks/fertilizer-applications/columns.tsxfdm-app/app/lib/url-utils.tsfdm-app/app/routes/farm.$b_id_farm.$calendar.field.fertilizer._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- fdm-app/app/routes/farm.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
fdm-app/app/routes/farm.$b_id_farm.$calendar.field.fertilizer._index.tsx (1)
229-265: Minor efficiency improvement: usebreakafter key deletion.After deleting a key from
fertilizerApplication, continuing to check remaining applications for that same key is redundant since the outcome won't change.♻️ Suggested optimization
if ( exampleFertilizerApplication[key] === null || typeof exampleFertilizerApplication[key] === "undefined" || app[key] === null || typeof app[key] === "undefined" ) { delete fertilizerApplication[key] - continue + break } if (keyTypes[key] === "number") { const a = app[key] as number const b = exampleFertilizerApplication[key] as number if (a !== b && Math.abs(a - b) > Math.abs(b) / 100) { delete fertilizerApplication[key] + break } } else if ( keyTypes[key] === "date" && ( exampleFertilizerApplication[key] as Date ).getTime() !== (app[key] as Date).getTime() ) { delete fertilizerApplication[key] + break } else if (exampleFertilizerApplication[key] !== app[key]) { delete fertilizerApplication[key] + break }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.field.fertilizer._index.tsx around lines 229 - 265, After deleting a key from fertilizerApplication inside the nested loops, exit the inner loop early to avoid redundant checks: when you call delete fertilizerApplication[key] within the loop over fertilizerApplications (which compares app against exampleFertilizerApplication using keyTypes and keys), add a break immediately after the delete so the code moves on to the next key; this applies to every branch that performs delete fertilizerApplication[key] (including the null/undefined branch, the number-difference branch, the date-comparison branch, and the final !== branch).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.field.fertilizer._index.tsx:
- Around line 229-265: After deleting a key from fertilizerApplication inside
the nested loops, exit the inner loop early to avoid redundant checks: when you
call delete fertilizerApplication[key] within the loop over
fertilizerApplications (which compares app against exampleFertilizerApplication
using keyTypes and keys), add a break immediately after the delete so the code
moves on to the next key; this applies to every branch that performs delete
fertilizerApplication[key] (including the null/undefined branch, the
number-difference branch, the date-comparison branch, and the final !== branch).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
fdm-app/app/components/blocks/fertilizer-applications/table.tsxfdm-app/app/lib/url-utils.tsfdm-app/app/routes/farm.$b_id_farm.$calendar.field.fertilizer._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- fdm-app/app/components/blocks/fertilizer-applications/table.tsx
- fdm-app/app/routes/farm.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx
…t, add confirmation dialog for deletion, improve dropdown for multiple fields
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 (1)
fdm-app/app/components/blocks/fertilizer-applications/form.tsx (1)
127-131:⚠️ Potential issue | 🟡 MinorValidate hydrated date values before setting form state.
new Date(...)can produceInvalid Datefrom stale/corrupt storage values; currently that gets written directly into the form state.🛡️ Proposed fix
- const hydrated = - k === "p_app_date" && v - ? new Date(v as any) - : (v as any) - form.setValue(k as any, hydrated) + if (k === "p_app_date" && v) { + const parsed = new Date(v as any) + if (Number.isNaN(parsed.getTime())) continue + form.setValue(k as any, parsed) + continue + } + form.setValue(k as any, v as any)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/fertilizer-applications/form.tsx` around lines 127 - 131, When hydrating the p_app_date value before calling form.setValue, validate the created Date object and avoid writing Invalid Date into form state: in the block that computes hydrated (the k === "p_app_date" ? new Date(v) : v branch), create the Date, check its validity (e.g., test isNaN(date.getTime()) or date instanceof Date && !isNaN(...)) and if invalid use null/undefined or skip calling form.setValue for that key; update the logic around form.setValue(k as any, hydrated) so only valid Date objects (or a safe fallback) are written to the form.
🧹 Nitpick comments (1)
fdm-app/app/components/blocks/fertilizer-applications/form.tsx (1)
107-114: Useundefinedinstead of""for unselectedp_app_method.Using empty string as the “no selection” state can submit
""as a real value. Preferundefinedfor reset and value fallback.♻️ Proposed fix
- form.setValue("p_app_method", "") + form.setValue("p_app_method", undefined) ... - value={field.value ?? ""} + value={field.value}Based on learnings: In fdm-app Select components, using
undefinedinstead of empty string ("") as fallback value prevents empty strings from being submitted as form values.Also applies to: 237-238
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/fertilizer-applications/form.tsx` around lines 107 - 114, The effect that resets p_app_method should set it to undefined instead of an empty string to avoid submitting "" as a real value; update the useEffect (the block referencing p_id, fertilizerApplication and calling form.setValue("p_app_method", "")) to call form.setValue("p_app_method", undefined). Also search for other places where p_app_method (or similar Select-backed fields) are reset or given a fallback (the other occurrence noted around lines 237-238) and change those to use undefined as well so Select components don't submit empty strings.
🤖 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-applications/dialog.tsx`:
- Around line 83-85: The empty-state copy uses the ternary that checks
isForRotation but the non-rotation branch always uses the singular "dit
perceel"; update that JSX ternary so the non-rotation branch uses numFields to
choose between singular and plural wording (e.g., use numFields > 1 ? pluralText
: singularText). Edit the ternary expression around isForRotation in the dialog
component to reference numFields and return the correct Dutch string when
multiple fields exist.
---
Outside diff comments:
In `@fdm-app/app/components/blocks/fertilizer-applications/form.tsx`:
- Around line 127-131: When hydrating the p_app_date value before calling
form.setValue, validate the created Date object and avoid writing Invalid Date
into form state: in the block that computes hydrated (the k === "p_app_date" ?
new Date(v) : v branch), create the Date, check its validity (e.g., test
isNaN(date.getTime()) or date instanceof Date && !isNaN(...)) and if invalid use
null/undefined or skip calling form.setValue for that key; update the logic
around form.setValue(k as any, hydrated) so only valid Date objects (or a safe
fallback) are written to the form.
---
Nitpick comments:
In `@fdm-app/app/components/blocks/fertilizer-applications/form.tsx`:
- Around line 107-114: The effect that resets p_app_method should set it to
undefined instead of an empty string to avoid submitting "" as a real value;
update the useEffect (the block referencing p_id, fertilizerApplication and
calling form.setValue("p_app_method", "")) to call form.setValue("p_app_method",
undefined). Also search for other places where p_app_method (or similar
Select-backed fields) are reset or given a fallback (the other occurrence noted
around lines 237-238) and change those to use undefined as well so Select
components don't submit empty strings.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
fdm-app/app/components/blocks/fertilizer-applications/columns.tsxfdm-app/app/components/blocks/fertilizer-applications/dialog.tsxfdm-app/app/components/blocks/fertilizer-applications/form.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.field.modify_fertilizer.$p_id.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.rotation.modify_fertilizer.$p_id.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- fdm-app/app/routes/farm.$b_id_farm.$calendar.field.modify_fertilizer.$p_id.tsx
- fdm-app/app/components/blocks/fertilizer-applications/columns.tsx
SvenVw
approved these changes
Feb 27, 2026
Collaborator
SvenVw
left a comment
SvenVw
left a comment
There was a problem hiding this comment.
Thanks, I made some changes to the FertilizerApplicationListDialog by moving it into a shared component, adding an AlertDialog for deleting and improving the dropdown for multiple fields
Collaborator
Author
@SvenVw looks good. I hope it will become a useful feature for the users. |
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.
Enhancements
Chores
Closes #440
Summary by CodeRabbit