Refactor Shapefile Upload to Use fdm-rvo Package and Enable Field Review#536
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughMoves shapefile parsing into a new fdm-rvo module, centralizes multipart upload handling in a shared server action, replaces client-side DBF parsing with server-side parsing, introduces a two-step upload→review→save flow, adds shapefile tests, and surfaces buffer-strip editability and concurrent import processing. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Client (Browser)
participant Form as Upload Form
participant Server as App Server (genericAction)
participant RvoLib as fdm-rvo (shapefile)
participant DB as Database
participant UI as Review UI
Browser->>Form: select .shp/.shx/.dbf/.prj
Form->>Server: POST multipart (intent=upload)
Server->>RvoLib: parse geometry & attributes
RvoLib-->>Server: geometries + attributes
Server->>RvoLib: convert → RvoField[]
Server->>DB: compute diffs, perimeter, buffer-strip
Server-->>Browser: return RvoImportReviewData (render Review UI)
Browser->>UI: user edits / toggle buffer-strip
UI->>Server: POST (intent=save_fields, userChoices + review data)
Server->>DB: apply changes inside transaction (concurrent handlers)
DB-->>Server: commit
Server-->>Browser: redirect / success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
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 docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
fdm-rvo/src/shpjs.d.ts (1)
1-1: Consider adding proper type definitions forshpjsexports.The empty module declaration allows imports but types everything as
any. Sinceshapefile.tsuses specific functions (parseShp,parseDbf,combine), adding minimal type stubs would improve type safety:♻️ Suggested type definitions
-declare module "shpjs" +declare module "shpjs" { + import type { FeatureCollection, Geometry, GeoJsonProperties } from "geojson" + + export function parseShp( + shp: ArrayBuffer, + shx?: ArrayBuffer, + ): Promise<Geometry[]> + + export function parseDbf<T = GeoJsonProperties>( + dbf: ArrayBuffer, + encoding?: string, + ): Promise<T[]> + + export function combine<G extends Geometry, P>( + arr: [G[], P[]], + ): FeatureCollection<G, P> +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-rvo/src/shpjs.d.ts` at line 1, Replace the bare module declaration with minimal type stubs for shpjs so imports are typed: declare the module "shpjs" and add exported signatures for parseShp, parseDbf, and combine (matching how shapefile.ts calls them), providing appropriate parameter and return types (e.g., ArrayBuffer/Buffer or URL input for parseShp/parseDbf and GeoJSON-like feature collections for returns) and export types/interfaces for Feature/FeatureCollection used by combine; ensure the file exports these named functions/types so TypeScript can verify uses of parseShp, parseDbf, and combine.fdm-app/app/routes/farm.create.$b_id_farm.$calendar.upload.tsx (2)
431-431: Unknown intent returns empty object silently.If an unexpected
intentvalue is submitted, the action returns{}without any error indication. Consider returning an explicit error:♻️ Suggested fix
- return {} + return { + success: false, + message: `Onbekende actie: ${intent ?? "(geen)"}`, + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm.create`.$b_id_farm.$calendar.upload.tsx at line 431, The action handler currently falls through and returns an empty object for unknown intent values (the final "return {}"), which hides errors; update the action for the intent switch in the upload action (the function handling form 'intent') to explicitly handle the default case by returning or throwing a clear error/response (e.g., return a Response with status 400 and an error message or throw a BadRequest) that includes the unrecognized intent value and context (referencing the intent variable and the upload action function) so callers and logs surface the issue instead of silently succeeding.
380-385: Consider validating JSON structure more strictly.The JSON data comes from hidden form fields that could theoretically be tampered with. While basic array check exists, malformed items could cause runtime errors in
processRvoImport. The current error handling will catch these, but a schema validation could provide better error messages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm.create`.$b_id_farm.$calendar.upload.tsx around lines 380 - 385, The current code only checks that RvoImportReviewDataJson parses to an array but not that each item has the expected shape, which can lead to runtime errors in processRvoImport; validate rvoImportReviewData items more strictly (e.g., after parsing RvoImportReviewDataJson into rvoImportReviewData and userChoicesJson into userChoices) by asserting required properties and types on each element (using Array.isArray(rvoImportReviewData) && rvoImportReviewData.every(...)) or apply a lightweight schema validator, and throw a clear error referencing which item is invalid before calling processRvoImport so you get actionable messages instead of generic runtime failures.fdm-rvo/src/shapefile.ts (1)
150-153: Extract magic number to a named constant for clarity.The value
253402297199appears to be a sentinel timestamp indicating "no end date". Consider extracting it:♻️ Suggested refactor
+/** Sentinel timestamp value representing "no end date" in RVO shapefile exports */ +const NO_END_DATE_SENTINEL = 253402297199 + export function convertShapefileFeatureIntoRvoField( ... EndDate: - EINDDAT !== 253402297199 + EINDDAT !== NO_END_DATE_SENTINEL ? new Date(EINDDAT).toISOString() : undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-rvo/src/shapefile.ts` around lines 150 - 153, Extract the magic number 253402297199 into a named constant (e.g. NO_END_DATE_TS or SENTINEL_NO_END_DATE) at the top of the module and replace the literal in the EndDate mapping so it reads a comparison against that constant (the mapping that checks EINDDAT !== 253402297199 and sets EndDate to new Date(EINDDAT).toISOString() or undefined). Update any related comments to reference the constant name for clarity.
🤖 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.create`.$b_id_farm.$calendar.upload.tsx:
- Line 357: Fix the typo in the success message string: locate the object
setting message: "Percelen zijn klaar voor beeordeling! 🎉" (the `message`
property in the upload handler) and change "beeordeling" to "beoordeling" so the
message reads "Percelen zijn klaar voor beoordeling! 🎉".
- Around line 147-162: The beforeunload handler currently checks
location.pathname.startsWith(expectedRedirectPath) which uses the current path
and will still trigger during programmatic redirects; instead add and check a
submission-tracking flag: create an isSubmitting ref/state (e.g.,
isSubmittingRef) that you set true in your form submit/save handler before
performing the redirect, and have handleBeforeUnload return early if
isSubmittingRef.current is true; keep the guard on rvoImportReviewData and
remove reliance on location.pathname in useEffect so handleBeforeUnload uses
isSubmittingRef and rvoImportReviewData to decide whether to show the warning.
In `@fdm-rvo/package.json`:
- Line 66: Remove the redundant `@turf/turf` dependency from package.json because
the code only uses multiPolygon and polygon from `@turf/helpers` (already
declared); delete the "@turf/turf": "^7.3.4" entry and run install to update
lockfile so the bundle no longer includes the full turf meta-package.
In `@fdm-rvo/src/shapefile.test.ts`:
- Around line 143-159: The MOCK_MULTIPOLYGON test mock uses an incorrect
coordinate nesting for a MultiPolygon: update the coordinates passed to
geometry("MultiPolygon", ...) so they are Position[][][] (an array of polygons,
each polygon is an array of linear rings) instead of Position[][]. Specifically,
wrap the existing ring arrays into an outer array to represent a single polygon
(or group separate polygons into their own arrays) when constructing
MOCK_MULTIPOLYGON in the test that defines geometry("MultiPolygon", ...),
ensuring the structure matches GeoJSON MultiPolygon expectations.
In `@fdm-rvo/src/shapefile.ts`:
- Around line 102-104: The function signature for
convertShapefileFeatureIntoRvoField is too narrow: change the Feature generic
from Feature<Polygon, Partial<RvoProperties>> to Feature<Polygon | MultiPolygon,
Partial<RvoProperties>> so the TypeScript types match the runtime handling; make
the same change for the other function(s) that use Feature<Polygon,...> around
lines 183-186 (replace with Feature<Polygon | MultiPolygon,...>) and keep the
existing runtime geometry narrowing logic unchanged.
---
Nitpick comments:
In `@fdm-app/app/routes/farm.create`.$b_id_farm.$calendar.upload.tsx:
- Line 431: The action handler currently falls through and returns an empty
object for unknown intent values (the final "return {}"), which hides errors;
update the action for the intent switch in the upload action (the function
handling form 'intent') to explicitly handle the default case by returning or
throwing a clear error/response (e.g., return a Response with status 400 and an
error message or throw a BadRequest) that includes the unrecognized intent value
and context (referencing the intent variable and the upload action function) so
callers and logs surface the issue instead of silently succeeding.
- Around line 380-385: The current code only checks that RvoImportReviewDataJson
parses to an array but not that each item has the expected shape, which can lead
to runtime errors in processRvoImport; validate rvoImportReviewData items more
strictly (e.g., after parsing RvoImportReviewDataJson into rvoImportReviewData
and userChoicesJson into userChoices) by asserting required properties and types
on each element (using Array.isArray(rvoImportReviewData) &&
rvoImportReviewData.every(...)) or apply a lightweight schema validator, and
throw a clear error referencing which item is invalid before calling
processRvoImport so you get actionable messages instead of generic runtime
failures.
In `@fdm-rvo/src/shapefile.ts`:
- Around line 150-153: Extract the magic number 253402297199 into a named
constant (e.g. NO_END_DATE_TS or SENTINEL_NO_END_DATE) at the top of the module
and replace the literal in the EndDate mapping so it reads a comparison against
that constant (the mapping that checks EINDDAT !== 253402297199 and sets EndDate
to new Date(EINDDAT).toISOString() or undefined). Update any related comments to
reference the constant name for clarity.
In `@fdm-rvo/src/shpjs.d.ts`:
- Line 1: Replace the bare module declaration with minimal type stubs for shpjs
so imports are typed: declare the module "shpjs" and add exported signatures for
parseShp, parseDbf, and combine (matching how shapefile.ts calls them),
providing appropriate parameter and return types (e.g., ArrayBuffer/Buffer or
URL input for parseShp/parseDbf and GeoJSON-like feature collections for
returns) and export types/interfaces for Feature/FeatureCollection used by
combine; ensure the file exports these named functions/types so TypeScript can
verify uses of parseShp, parseDbf, and combine.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5e9f2939-9fe7-42ed-96fd-483018c32e56
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
fdm-app/app/components/blocks/mijnpercelen/form-upload.tsxfdm-app/app/routes/farm.create.$b_id_farm.$calendar.upload.tsxfdm-app/package.jsonfdm-rvo/package.jsonfdm-rvo/src/index.tsfdm-rvo/src/shapefile.test.tsfdm-rvo/src/shapefile.tsfdm-rvo/src/shpjs.d.ts
💤 Files with no reviewable changes (1)
- fdm-app/package.json
Comment on lines
+147
to
+162
| // Warn the user before refreshing or leaving when data is present | ||
| const expectedRedirectPath = `/farm/create/${b_id_farm}/${calendar}/fields` | ||
| useEffect(() => { | ||
| if (rvoImportReviewData && rvoImportReviewData.length > 0) { | ||
| const handleBeforeUnload = (e: BeforeUnloadEvent) => { | ||
| if (location.pathname.startsWith(expectedRedirectPath)) return | ||
| e.preventDefault() | ||
| e.returnValue = | ||
| "Als u de pagina ververst, wordt de verbinding met RVO verbroken en moet u opnieuw inloggen met eHerkenning. Wilt u doorgaan?" | ||
| return e.returnValue | ||
| } | ||
| window.addEventListener("beforeunload", handleBeforeUnload) | ||
| return () => | ||
| window.removeEventListener("beforeunload", handleBeforeUnload) | ||
| } | ||
| }, [location.pathname, expectedRedirectPath, rvoImportReviewData]) |
Contributor
There was a problem hiding this comment.
beforeunload guard may not correctly distinguish redirect from refresh.
The check location.pathname.startsWith(expectedRedirectPath) compares against the current path, not the navigation target. When a form submission triggers a redirect, beforeunload fires before the pathname changes, so this guard will still show the warning on successful saves.
Consider tracking submission state instead:
♻️ Suggested approach
+ const [isSubmittingFields, setIsSubmittingFields] = useState(false)
+
useEffect(() => {
- if (rvoImportReviewData && rvoImportReviewData.length > 0) {
+ if (rvoImportReviewData && rvoImportReviewData.length > 0 && !isSubmittingFields) {
const handleBeforeUnload = (e: BeforeUnloadEvent) => {
- if (location.pathname.startsWith(expectedRedirectPath)) return
e.preventDefault()
e.returnValue = "..."
return e.returnValue
}
...
}
- }, [location.pathname, expectedRedirectPath, rvoImportReviewData])
+ }, [rvoImportReviewData, isSubmittingFields])
// Then set isSubmittingFields = true in the save form's onSubmit🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fdm-app/app/routes/farm.create`.$b_id_farm.$calendar.upload.tsx around lines
147 - 162, The beforeunload handler currently checks
location.pathname.startsWith(expectedRedirectPath) which uses the current path
and will still trigger during programmatic redirects; instead add and check a
submission-tracking flag: create an isSubmitting ref/state (e.g.,
isSubmittingRef) that you set true in your form submit/save handler before
performing the redirect, and have handleBeforeUnload return early if
isSubmittingRef.current is true; keep the guard on rvoImportReviewData and
remove reliance on location.pathname in useEffect so handleBeforeUnload uses
isSubmittingRef and rvoImportReviewData to decide whether to show the warning.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/rvo/import-review-table.tsx`:
- Around line 725-728: The visibility logic for the buffer strip columns is
reversed: in columnVisibility swap the boolean expressions so bufferstrook is
visible when flags?.b_bufferstrip_info_available is truthy and
bufferstrook_editable is visible when that flag is falsy; specifically update
the bufferstrook and bufferstrook_editable properties in the columnVisibility
object (the variables named bufferstrook and bufferstrook_editable in
import-review-table.tsx) to use the inverted expressions.
In `@fdm-app/app/routes/farm.create`.$b_id_farm.$calendar.upload.tsx:
- Around line 403-467: The code currently parses RvoImportReviewDataJson from
the client and passes it into processRvoImport and updateField
(rvoImportReviewData, processRvoImport, updateField, RvoImportReviewDataJson,
onFieldAdded) which lets a caller tamper with persistent review fields; instead
persist the server-generated review dataset (or recompute it) keyed to the
upload/session and never parse RvoImportReviewDataJson on save; change the
handler to load the server-side review data by upload id and pass that to
processRvoImport and the updateField loop, and accept only minimal userChoices
from the request body (ids/actions) so updateField uses server-owned values
rather than any client-supplied properties like localField.b_id or
IndBufferstrook.
- Around line 114-127: The updater passed to setRvoImportReviewData can return
undefined when the "not found" branches use a bare return, which will wipe
rvoImportReviewData; change those branches to return the existing state instead
(i.e., return data or the previous value) so that when getItemId doesn't find id
or data is falsy the state remains unchanged; update the notifier inside the
setRvoImportReviewData callback (the block referencing getItemId, index, id, and
item) to return the unmodified data on the early-exit paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b21a745e-f680-4bc5-805b-da4e418fee6e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
fdm-app/app/components/blocks/rvo/import-review-table.tsxfdm-app/app/routes/farm.create.$b_id_farm.$calendar.upload.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
fdm-app/app/routes/farm.create.$b_id_farm.$calendar.upload.tsx (3)
123-136:⚠️ Potential issue | 🟡 MinorReturn the previous state on the no-op branches.
The updater at Lines 124 and 132 currently uses bare
return. In a React state setter that resolves toundefined, so a missedidmatch clears the whole review step instead of leaving it unchanged.♻️ Proposed fix
setRvoImportReviewData((data) => { - if (!data) return + if (!data) return data const index = data.findIndex( (originalItem) => getItemId(originalItem) === id, ) if (index === -1) { console.warn( `Item with id ${id} not found so nothing is modified.`, ) - return + return data } const newData = [...data] newData[index] = item return newData })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm.create`.$b_id_farm.$calendar.upload.tsx around lines 123 - 136, The updater passed to setRvoImportReviewData can return undefined on the early-return paths which will clear state; update the callback used in setRvoImportReviewData so that both no-data and "item not found" branches return the previous state (the incoming data) instead of bare return; locate the anonymous updater where getItemId(originalItem) === id is checked and replace the bare returns with "return data" to make it a no-op when nothing should change.
214-225:⚠️ Potential issue | 🟡 MinorDon't key the unload guard off the current pathname.
During
save_fields,beforeunloadfires before React Router applies the redirect, solocation.pathnameis still the upload route and this warning is shown even on a successful save. Track a submit/ref flag and suppress the prompt from that instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm.create`.$b_id_farm.$calendar.upload.tsx around lines 214 - 225, The beforeunload guard (handleBeforeUnload) currently checks location.pathname to detect a redirect which is unreliable because beforeunload fires before React Router updates the path; replace that logic by tracking an explicit submit flag/ref (e.g., isSubmittingRef) set to true at the start of your save/submit function (save_fields) and set back to false on error/cancel, then in handleBeforeUnload check isSubmittingRef.current (and/or a more specific isRedirectingRef) to suppress the prompt instead of using location.pathname or expectedRedirectPath; update the save_fields submit flow to set the ref before initiating the request and to clear it on failure so the prompt still appears when appropriate.
486-504:⚠️ Potential issue | 🔴 CriticalKeep
RvoImportReviewDataserver-owned.This branch deserializes
RvoImportReviewDataJsonfrom a hidden field and feeds it straight intoprocessRvoImport; the follow-upupdateFieldloop then persistsitem.localField.b_idanditem.rvoField.properties.mestData.IndBufferstrookfrom that same client-controlled payload. A caller can tamper ids, geometry, names, or buffer-strip values before submit and write changes the server never generated.Reload or recompute the review dataset on
save_fieldsand accept only the minimaluserChoicesmap from the browser.Also applies to: 542-567
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm.create`.$b_id_farm.$calendar.upload.tsx around lines 486 - 504, The code currently trusts client-provided RvoImportReviewDataJson and passes it into processRvoImport, then persists fields (e.g., item.localField.b_id and item.rvoField.properties.mestData.IndBufferstrook) which lets a caller tamper server-owned data; instead, stop parsing RvoImportReviewDataJson from the form and reload or recompute the canonical RvoImportReviewData on the server inside the handler that runs save_fields, then apply only the client-supplied userChoices map to that server-side dataset; update the handler that reads RvoImportReviewDataJson, the call site of processRvoImport, and the updateField loop so they use the recomputed RvoImportReviewData and only accept values from userChoices (keep references to RvoImportReviewDataJson, userChoices, processRvoImport, updateField, and save_fields to locate the changes).fdm-rvo/src/shapefile.ts (1)
107-109:⚠️ Potential issue | 🟡 MinorWiden the GeoJSON types to include
MultiPolygon.This module already accepts
MultiPolygonat runtime, but these signatures still narrow the public surface toPolygononly. That makes the API lie about valid shapefile data and forces downstream casts around values the implementation already supports.♻️ Proposed fix
export function convertShapefileFeatureIntoRvoField( - feature: Feature<Polygon, Partial<RvoProperties>>, + feature: Feature<Polygon | MultiPolygon, Partial<RvoProperties>>, ): RvoField { @@ const shapefile: FeatureCollection< - Polygon, + Polygon | MultiPolygon, Partial<RvoProperties> > = combine([geometries, attributes])Also applies to: 191-194
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-rvo/src/shapefile.ts` around lines 107 - 109, Update the exported GeoJSON type annotations that currently restrict features to Polygon so they also accept MultiPolygon; specifically change the signature of convertShapefileFeatureIntoRvoField (and any other exported functions in this module that use Feature<Polygon, ...>) to use Feature<Polygon | MultiPolygon, Partial<RvoProperties>> (or the equivalent union) so runtime-supported MultiPolygon inputs are reflected in the public types; ensure any related internal uses/type guards remain compatible after widening.
🤖 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/rvo/import-review-table.tsx`:
- Around line 725-728: The columnVisibility logic treats undefined flags as "no
buffer info" which shows the editable control by default; update the
columnVisibility initialization (symbol: columnVisibility) so that
flags?.b_bufferstrip_info_available defaults to true (e.g. use
nullish-coalescing) and keep bufferstrook_editable as its negation, ensuring the
read-only bufferstrook column is shown when flags is omitted; also verify any
editable controls (onItemChange) are only rendered/used where provided (symbols:
bufferstrook, bufferstrook_editable, onItemChange).
In `@fdm-app/app/routes/farm.create`.$b_id_farm.$calendar.upload.tsx:
- Around line 405-413: The guard currently rejects uploads if any of shp_file,
shx_file, dbf_file, or prj_file are missing; change it to only require the
mandatory .shp and .dbf files (leave shx_file and prj_file optional) so
getRvoFieldsFromShapefile can handle missing sidecars and fallback to EPSG:4326;
update the conditional that references shp_file, shx_file, dbf_file, prj_file to
only check shp_file and dbf_file and keep the same error message and return
structure.
---
Duplicate comments:
In `@fdm-app/app/routes/farm.create`.$b_id_farm.$calendar.upload.tsx:
- Around line 123-136: The updater passed to setRvoImportReviewData can return
undefined on the early-return paths which will clear state; update the callback
used in setRvoImportReviewData so that both no-data and "item not found"
branches return the previous state (the incoming data) instead of bare return;
locate the anonymous updater where getItemId(originalItem) === id is checked and
replace the bare returns with "return data" to make it a no-op when nothing
should change.
- Around line 214-225: The beforeunload guard (handleBeforeUnload) currently
checks location.pathname to detect a redirect which is unreliable because
beforeunload fires before React Router updates the path; replace that logic by
tracking an explicit submit flag/ref (e.g., isSubmittingRef) set to true at the
start of your save/submit function (save_fields) and set back to false on
error/cancel, then in handleBeforeUnload check isSubmittingRef.current (and/or a
more specific isRedirectingRef) to suppress the prompt instead of using
location.pathname or expectedRedirectPath; update the save_fields submit flow to
set the ref before initiating the request and to clear it on failure so the
prompt still appears when appropriate.
- Around line 486-504: The code currently trusts client-provided
RvoImportReviewDataJson and passes it into processRvoImport, then persists
fields (e.g., item.localField.b_id and
item.rvoField.properties.mestData.IndBufferstrook) which lets a caller tamper
server-owned data; instead, stop parsing RvoImportReviewDataJson from the form
and reload or recompute the canonical RvoImportReviewData on the server inside
the handler that runs save_fields, then apply only the client-supplied
userChoices map to that server-side dataset; update the handler that reads
RvoImportReviewDataJson, the call site of processRvoImport, and the updateField
loop so they use the recomputed RvoImportReviewData and only accept values from
userChoices (keep references to RvoImportReviewDataJson, userChoices,
processRvoImport, updateField, and save_fields to locate the changes).
In `@fdm-rvo/src/shapefile.ts`:
- Around line 107-109: Update the exported GeoJSON type annotations that
currently restrict features to Polygon so they also accept MultiPolygon;
specifically change the signature of convertShapefileFeatureIntoRvoField (and
any other exported functions in this module that use Feature<Polygon, ...>) to
use Feature<Polygon | MultiPolygon, Partial<RvoProperties>> (or the equivalent
union) so runtime-supported MultiPolygon inputs are reflected in the public
types; ensure any related internal uses/type guards remain compatible after
widening.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 55fa6fab-ec30-42d8-a5bd-23ad29a76b7d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
fdm-app/app/components/blocks/rvo/import-review-table.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.rvo.tsxfdm-app/app/routes/farm.create.$b_id_farm.$calendar.rvo.tsxfdm-app/app/routes/farm.create.$b_id_farm.$calendar.upload.tsxfdm-app/package.jsonfdm-core/src/index.tsfdm-rvo/package.jsonfdm-rvo/src/shapefile.tsfdm-rvo/tsdown.config.ts
✅ Files skipped from review due to trivial changes (2)
- fdm-rvo/tsdown.config.ts
- fdm-app/app/routes/farm.$b_id_farm.$calendar.rvo.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- fdm-app/package.json
- fdm-rvo/package.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
fdm-app/app/components/blocks/mijnpercelen/loader-and-action.server.ts (1)
276-282:⚠️ Potential issue | 🟡 Minor
userChoices instanceof Objectalso accepts arrays.
[] instanceof Objectistrue, so a client postinguserChoices=[]passes validation and thenuserChoices[getItemId(item)]silently returnsundefinedfor every item (no-op import). Tighten the guard:- if (!(userChoices instanceof Object)) { + if ( + typeof userChoices !== "object" || + userChoices === null || + Array.isArray(userChoices) + ) { throw new Error("Invalid user choices format") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/mijnpercelen/loader-and-action.server.ts` around lines 276 - 282, The current guard uses "userChoices instanceof Object" which accepts arrays; replace it with a strict object check such as "typeof userChoices === 'object' && userChoices !== null && !Array.isArray(userChoices)" (or an equivalent assertion that rejects arrays) so that userChoices is a plain object before using userChoices[getItemId(item)]; keep the existing rvoImportReviewData Array.isArray check as-is and update the validation around the userChoices variable in the loader-and-action.server.ts logic that follows the rvoImportReviewData check.
🧹 Nitpick comments (4)
fdm-app/app/components/blocks/mijnpercelen/upload-page.tsx (2)
40-42: Initial state isundefinedbut the type says| null.
useState<RvoImportReviewItem<any>[] | null>()initializes toundefined(TS infersT | null | undefined). Either passnullto match the declared union or broaden the type to| null | undefined. Current code works because all checks are truthiness-based, but it's a mild type inconsistency.- const [rvoImportReviewData, setRvoImportReviewData] = useState< - RvoImportReviewItem<any>[] | null - >() + const [rvoImportReviewData, setRvoImportReviewData] = useState< + RvoImportReviewItem<any>[] | null + >(null)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/mijnpercelen/upload-page.tsx` around lines 40 - 42, The declared state rvoImportReviewData uses useState<RvoImportReviewItem<any>[] | null>() which leaves the initial value undefined; update it to be consistent by either initializing it to null (useState<RvoImportReviewItem<any>[] | null>(null)) or broaden the type to include undefined (useState<RvoImportReviewItem<any>[] | null | undefined>()), adjusting any null/undefined checks accordingly; locate the useState call for rvoImportReviewData and change the type or initializer to resolve the mismatch between the declared union and the actual initial value.
78-86: Side effect inside setState updater runs twice under StrictMode.Mutating
window.location.hashfrom within thesetRvoImportReviewDataupdater is fine functionally, but React 18/19 StrictMode invokes updaters twice in development. It's also generally discouraged because updaters should be pure. Moving the hash assignment outside the setter keeps the effect predictable.♻️ Proposed refactor
- if (actionRvoImportReviewData) { - setRvoImportReviewData((oldData) => { - if (!oldData) { - window.location.hash = "#review" - } - return actionRvoImportReviewData - }) + if (actionRvoImportReviewData) { + setRvoImportReviewData((oldData) => { + if (!oldData && typeof window !== "undefined") { + // schedule outside the pure updater + queueMicrotask(() => { + window.location.hash = "#review" + }) + } + return actionRvoImportReviewData + })Or track the "first time review data arrives" with a ref and set the hash from the effect body.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/mijnpercelen/upload-page.tsx` around lines 78 - 86, The setState updater passed to setRvoImportReviewData currently mutates window.location.hash inside the updater (in the useEffect that reads actionRvoImportReviewData), which causes the side effect to run twice under React StrictMode; move the window.location.hash assignment out of the setRvoImportReviewData updater and into the effect body (or use a ref to detect first-arrival) so the updater remains pure — update the effect that references actionRvoImportReviewData and setRvoImportReviewData to set the state only inside the updater and perform window.location.hash = "#review" immediately after (or guarded by a ref) in the same effect.fdm-app/app/components/blocks/mijnpercelen/loader-and-action.server.ts (1)
284-355: Nested transaction: be awareprocessRvoImportopens its own innerfdm.transactionon thetxyou pass.
genericActionwraps everything infdm.transaction(async (tx) => ...)and then callsprocessRvoImport(tx, ...)— which itself callstx.transaction(async (tx) => ...)(seefdm-rvo/src/process.tsline 55). In Drizzle/Postgres this becomes a savepoint, which works but is unnecessary overhead here and couples the two layers. Consider either (a) removing the outerfdm.transactionin this action and lettingprocessRvoImportown the transaction, or (b) havingprocessRvoImportaccept an existingtxand skip the innerfdm.transactionwhen one is already active. The bufferstrip-override loop at lines 329-354 would need to move inside whichever layer owns the transaction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/mijnpercelen/loader-and-action.server.ts` around lines 284 - 355, The outer fdm.transaction wrapper around the call to processRvoImport creates a nested/savepoint transaction because processRvoImport also opens its own tx; choose one approach and move the bufferstrip override loop into the same transaction context: either remove the outer fdm.transaction in this action and let processRvoImport own the transaction (keep the onFieldAdded callback and the post-import bufferstrip loop inside processRvoImport's transaction), or modify processRvoImport to accept an active tx parameter and skip creating its own inner tx when one is provided (then keep the existing outer fdm.transaction and ensure the bufferstrip override loop that calls updateField runs inside that same tx). Ensure references to processRvoImport, fdm.transaction, genericAction, updateField, and the onFieldAdded callback are updated accordingly so the bufferstrip loop executes inside the single owning transaction.fdm-rvo/src/process.ts (1)
55-237: Consider running items sequentially for clarity.
Promise.all(rvoImportReviewData.map((item) => handleItem(item)))runs multiple database operations concurrently on the same transaction connection. While postgres-js (the underlying driver) supports pipelining concurrent queries on a single connection, processing items sequentially inside the transaction is simpler to reason about and easier to debug. The atomicity guarantee remains unchanged.Run the items sequentially;
onFieldAddedcan continue to batch viaPromise.allafter all items complete:- await Promise.all(rvoImportReviewData.map((item) => handleItem(item))) - - if (onFieldAdded) { - await Promise.all( - addedFields.map(({ b_id, geometry }) => - onFieldAdded(tx, b_id, geometry), - ), - ) - } + for (const item of rvoImportReviewData) { + await handleItem(item) + } + + if (onFieldAdded) { + await Promise.all( + addedFields.map(({ b_id, geometry }) => + onFieldAdded(tx, b_id, geometry), + ), + ) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-rvo/src/process.ts` around lines 55 - 237, The current code launches handleItem concurrently with Promise.all over rvoImportReviewData inside fdm.transaction which runs many DB operations on the same transaction connection; change to process items sequentially by iterating rvoImportReviewData and awaiting handleItem(item) one-by-one (replace the Promise.all(rvoImportReviewData.map(...)) call with a for/for-of loop that awaits handleItem), keeping onFieldAdded batched as-is (await Promise.all(addedFields.map(...)) after the sequential loop) so addedFields and fdm.transaction behavior remain unchanged.
🤖 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/mijnpercelen/loader-and-action.server.ts`:
- Around line 104-108: The per-request directory created by createFsFileStorage
using uploadRequestId is never removed; update the cleanup in the finally block
(where fileStorage.remove is called) to also remove the now-empty per-request
directory `./uploads/shapefiles/${uploadRequestId}` (or call a
fileStorage.rmdir/removeDirectory API if available) after individual files are
deleted, or switch to creating uploads under a shared ephemeral parent and
implement a periodic sweep; reference the symbols uploadRequestId, fileStorage
and createFsFileStorage and ensure the directory removal runs regardless of
success/failure.
---
Duplicate comments:
In `@fdm-app/app/components/blocks/mijnpercelen/loader-and-action.server.ts`:
- Around line 276-282: The current guard uses "userChoices instanceof Object"
which accepts arrays; replace it with a strict object check such as "typeof
userChoices === 'object' && userChoices !== null && !Array.isArray(userChoices)"
(or an equivalent assertion that rejects arrays) so that userChoices is a plain
object before using userChoices[getItemId(item)]; keep the existing
rvoImportReviewData Array.isArray check as-is and update the validation around
the userChoices variable in the loader-and-action.server.ts logic that follows
the rvoImportReviewData check.
---
Nitpick comments:
In `@fdm-app/app/components/blocks/mijnpercelen/loader-and-action.server.ts`:
- Around line 284-355: The outer fdm.transaction wrapper around the call to
processRvoImport creates a nested/savepoint transaction because processRvoImport
also opens its own tx; choose one approach and move the bufferstrip override
loop into the same transaction context: either remove the outer fdm.transaction
in this action and let processRvoImport own the transaction (keep the
onFieldAdded callback and the post-import bufferstrip loop inside
processRvoImport's transaction), or modify processRvoImport to accept an active
tx parameter and skip creating its own inner tx when one is provided (then keep
the existing outer fdm.transaction and ensure the bufferstrip override loop that
calls updateField runs inside that same tx). Ensure references to
processRvoImport, fdm.transaction, genericAction, updateField, and the
onFieldAdded callback are updated accordingly so the bufferstrip loop executes
inside the single owning transaction.
In `@fdm-app/app/components/blocks/mijnpercelen/upload-page.tsx`:
- Around line 40-42: The declared state rvoImportReviewData uses
useState<RvoImportReviewItem<any>[] | null>() which leaves the initial value
undefined; update it to be consistent by either initializing it to null
(useState<RvoImportReviewItem<any>[] | null>(null)) or broaden the type to
include undefined (useState<RvoImportReviewItem<any>[] | null | undefined>()),
adjusting any null/undefined checks accordingly; locate the useState call for
rvoImportReviewData and change the type or initializer to resolve the mismatch
between the declared union and the actual initial value.
- Around line 78-86: The setState updater passed to setRvoImportReviewData
currently mutates window.location.hash inside the updater (in the useEffect that
reads actionRvoImportReviewData), which causes the side effect to run twice
under React StrictMode; move the window.location.hash assignment out of the
setRvoImportReviewData updater and into the effect body (or use a ref to detect
first-arrival) so the updater remains pure — update the effect that references
actionRvoImportReviewData and setRvoImportReviewData to set the state only
inside the updater and perform window.location.hash = "#review" immediately
after (or guarded by a ref) in the same effect.
In `@fdm-rvo/src/process.ts`:
- Around line 55-237: The current code launches handleItem concurrently with
Promise.all over rvoImportReviewData inside fdm.transaction which runs many DB
operations on the same transaction connection; change to process items
sequentially by iterating rvoImportReviewData and awaiting handleItem(item)
one-by-one (replace the Promise.all(rvoImportReviewData.map(...)) call with a
for/for-of loop that awaits handleItem), keeping onFieldAdded batched as-is
(await Promise.all(addedFields.map(...)) after the sequential loop) so
addedFields and fdm.transaction behavior remain unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c794ab02-e807-4c1e-b2db-d1262404c896
📒 Files selected for processing (5)
fdm-app/app/components/blocks/mijnpercelen/loader-and-action.server.tsfdm-app/app/components/blocks/mijnpercelen/upload-animation.tsxfdm-app/app/components/blocks/mijnpercelen/upload-page.tsxfdm-app/app/components/blocks/rvo/import-review-table.tsxfdm-rvo/src/process.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
fdm-app/app/components/blocks/mijnpercelen/loader-and-action.server.ts (1)
274-327:⚠️ Potential issue | 🔴 CriticalDo not use hidden review JSON as the source of truth.
This still lets the browser modify
rvoImportReviewDatabefore save; theArray.isArray/instanceof Objectchecks do not protect immutable fields like geometry, source IDs, orlocalField.b_id. Rehydrate the canonical review/upload state server-side or validate every immutable field against a server-stored record before callingprocessRvoImport.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/mijnpercelen/loader-and-action.server.ts` around lines 274 - 327, The code trusts client-provided JSON (rvoImportReviewData / userChoices) as the source of truth before calling processRvoImport, allowing tampered immutable fields (geometry, source IDs, localField.b_id); instead rehydrate or validate those immutable values server-side: load the canonical review/upload record from the database (by the same upload/review id used to render the form) and either replace rvoImportReviewData with that server-side version or iterate rvoImportReviewData and verify each immutable attribute (e.g., geometry, sourceId, localField.b_id) against the server record before calling processRvoImport(tx, session.principal_id, b_id_farm, ...); keep the onFieldAdded flow and NMI lookup as-is but ensure any IDs used there come from the validated / rehydrated data, and throw a validation error (or abort transaction) if mismatches are found.
🤖 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/mijnpercelen/loader-and-action.server.ts`:
- Around line 169-174: The code is reading .name on items from
formData.getAll("shapefile") without ensuring they are File objects, causing
runtime errors for non-file entries; update the handler in
loader-and-action.server.ts to first filter the result of
formData.getAll("shapefile") to only File instances (use a type guard like
instanceof File) before using files.find and checking .name, then proceed with
the existing four-file validation for shp_file, shx_file, dbf_file, and prj_file
to reject malformed uploads.
- Around line 285-327: Currently onFieldAdded (passed into processRvoImport
inside fdm.transaction) performs network I/O via getSoilParameterEstimates and
calls addSoilAnalysis while the DB transaction is open; change this so the
transaction only collects added field IDs/geometries (e.g., push {b_id,
geometry} into an array from onFieldAdded) and returns without external calls,
commit the import, then in a separate short-lived transaction or background job
iterate the collected items to call getSoilParameterEstimates and
addSoilAnalysis; update references in the diff: onFieldAdded, processRvoImport,
addSoilAnalysis, getSoilParameterEstimates, and fdm.transaction to implement
this two-phase flow.
- Around line 193-221: The perimeter/area values passed to
determineIfFieldIsBuffer are wrong: perimeter currently sums all rings
(including interior holes) and returns Turf's default unit (km), and area is in
m²; change the perimeter function to sum only exterior rings (use the first
ring: for Polygon use geometry.coordinates[0], for MultiPolygon use polygon[0]
for each polygon) and call Turf length with units set to meters so the result is
meters, and convert the Turf area result to hectares (area(...) / 10000) before
calling determineIfFieldIsBuffer; update references to perimeter(geometry) and
area(geometry) where determineIfFieldIsBuffer is invoked.
---
Duplicate comments:
In `@fdm-app/app/components/blocks/mijnpercelen/loader-and-action.server.ts`:
- Around line 274-327: The code trusts client-provided JSON (rvoImportReviewData
/ userChoices) as the source of truth before calling processRvoImport, allowing
tampered immutable fields (geometry, source IDs, localField.b_id); instead
rehydrate or validate those immutable values server-side: load the canonical
review/upload record from the database (by the same upload/review id used to
render the form) and either replace rvoImportReviewData with that server-side
version or iterate rvoImportReviewData and verify each immutable attribute
(e.g., geometry, sourceId, localField.b_id) against the server record before
calling processRvoImport(tx, session.principal_id, b_id_farm, ...); keep the
onFieldAdded flow and NMI lookup as-is but ensure any IDs used there come from
the validated / rehydrated data, and throw a validation error (or abort
transaction) if mismatches are found.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 292807f6-2d50-45fb-86c8-a9efbe96b7a9
📒 Files selected for processing (1)
fdm-app/app/components/blocks/mijnpercelen/loader-and-action.server.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
fdm-app/app/components/blocks/mijnpercelen/loader-and-action.server.ts (1)
284-290:⚠️ Potential issue | 🟡 Minor
userChoices instanceof Objectstill lets arrays and functions through.Arrays, functions, and most non-plain values pass
instanceof Object, so a malformed payload like[]would slip past this guard and only fail later insideprocessRvoImport/userChoices[getItemId(item)]as a generic server error. Tighten to a plain-object check.🛡️ Suggested fix
- if (!(userChoices instanceof Object)) { + if ( + typeof userChoices !== "object" || + userChoices === null || + Array.isArray(userChoices) + ) { throw new Error("Invalid user choices format") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/mijnpercelen/loader-and-action.server.ts` around lines 284 - 290, The current guard using "userChoices instanceof Object" is too permissive (allows arrays/functions); tighten the validation in the loader/action where rvoImportReviewData and userChoices are checked by replacing that check with a plain-object test (e.g., ensure typeof userChoices === "object", userChoices !== null, !Array.isArray(userChoices), and userChoices.constructor === Object) so malformed payloads are rejected before calling processRvoImport or indexing userChoices[getItemId(item)]; keep the rvoImportReviewData array check as-is and throw the same Error message on invalid userChoices.
🧹 Nitpick comments (2)
fdm-app/app/components/blocks/mijnpercelen/loader-and-action.server.ts (2)
216-254: Ordering:compareFieldsruns beforemestDatais populated onrvoFields.
compareFieldsat Line 216 is invoked before themestData.IndBufferstrookenrichment loop at Lines 224-234, which is why the subsequent cleanup at Lines 237-254 must stripb_bufferstripfrom every diff and re-evaluateCONFLICT → MATCH. SinceRvoImportReviewDataitems reference the samervoFieldobjects, moving the enrichment loop abovecompareFieldswould letcompareFieldsproduce correct diffs directly and remove the fragile post-hoc filter. Worth considering for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/mijnpercelen/loader-and-action.server.ts` around lines 216 - 254, The compareFields call currently runs before rvoFields are enriched with mestData. Move the enrichment loop that sets rvoField.properties.mestData via determineIfFieldIsBuffer so it executes before calling compareFields(...); this lets compareFields produce correct diffs (including b_bufferstrip) and allows you to remove the subsequent post-hoc removal/filtering of "b_bufferstrip" and status adjustments on RvoImportReviewData and checks against RvoImportReviewStatus, since the rvoField objects will already contain correct IndBufferstrook values.
292-324: HoistgetNmiApiKey()out of the per-field callback.
onFieldAddedruns once per added field insidefdm.transaction, andgetNmiApiKey()is re-evaluated each time (and read inside the open DB transaction). Since it's request-scoped, read it once beforefdm.transactionand close over the result — this also makes it trivial to short-circuit the entire NMI integration when the key is missing.+ const nmiApiKey = getNmiApiKey() await fdm.transaction(async (tx) => { const onFieldAdded = async ( tx: FdmType, b_id: string, geometry: any, ) => { - const nmiApiKey = getNmiApiKey() - if (nmiApiKey) { + if (!nmiApiKey) return + try { ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/mijnpercelen/loader-and-action.server.ts` around lines 292 - 324, Hoist the getNmiApiKey() call out of the per-field callback by calling it once before invoking fdm.transaction and closing over that value in the onFieldAdded function; if the key is falsy, skip wiring the NMI logic entirely to avoid re-reading the request-scoped key inside the open DB transaction. Update code referencing getSoilParameterEstimates, addSoilAnalysis, and session.principal_id inside onFieldAdded to use the outer-scoped nmiApiKey and short-circuit the block when it's undefined, leaving the rest of the transaction logic unchanged.
🤖 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._index.tsx:
- Around line 514-516: Replace the English CardTitle text "Upload Shapefile"
with the Dutch label used elsewhere (e.g., "Shapefile uploaden" or the app's
established Dutch phrasing) so the CardTitle in the component CardTitle matches
the surrounding dashboard and upload route copy.
---
Duplicate comments:
In `@fdm-app/app/components/blocks/mijnpercelen/loader-and-action.server.ts`:
- Around line 284-290: The current guard using "userChoices instanceof Object"
is too permissive (allows arrays/functions); tighten the validation in the
loader/action where rvoImportReviewData and userChoices are checked by replacing
that check with a plain-object test (e.g., ensure typeof userChoices ===
"object", userChoices !== null, !Array.isArray(userChoices), and
userChoices.constructor === Object) so malformed payloads are rejected before
calling processRvoImport or indexing userChoices[getItemId(item)]; keep the
rvoImportReviewData array check as-is and throw the same Error message on
invalid userChoices.
---
Nitpick comments:
In `@fdm-app/app/components/blocks/mijnpercelen/loader-and-action.server.ts`:
- Around line 216-254: The compareFields call currently runs before rvoFields
are enriched with mestData. Move the enrichment loop that sets
rvoField.properties.mestData via determineIfFieldIsBuffer so it executes before
calling compareFields(...); this lets compareFields produce correct diffs
(including b_bufferstrip) and allows you to remove the subsequent post-hoc
removal/filtering of "b_bufferstrip" and status adjustments on
RvoImportReviewData and checks against RvoImportReviewStatus, since the rvoField
objects will already contain correct IndBufferstrook values.
- Around line 292-324: Hoist the getNmiApiKey() call out of the per-field
callback by calling it once before invoking fdm.transaction and closing over
that value in the onFieldAdded function; if the key is falsy, skip wiring the
NMI logic entirely to avoid re-reading the request-scoped key inside the open DB
transaction. Update code referencing getSoilParameterEstimates, addSoilAnalysis,
and session.principal_id inside onFieldAdded to use the outer-scoped nmiApiKey
and short-circuit the block when it's undefined, leaving the rest of the
transaction logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f181d9cd-9269-43a4-b014-75ab390d134c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
fdm-app/app/components/blocks/mijnpercelen/loader-and-action.server.tsfdm-app/app/routes/farm.$b_id_farm._index.tsxfdm-app/app/routes/farm.create.$b_id_farm.$calendar._index.tsxfdm-app/package.jsonfdm-core/src/index.ts
✅ Files skipped from review due to trivial changes (2)
- fdm-app/app/routes/farm.create.$b_id_farm.$calendar._index.tsx
- fdm-app/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- fdm-core/src/index.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/mijnpercelen/upload-page.tsx`:
- Around line 48-65: In handleItemChange, don't rely on the comment — enforce ID
stability by validating that getItemId(item) === id before mutating state:
inside the setRvoImportReviewData updater (and before creating newData), compute
newId = getItemId(item) and if newId !== id either log a warning and return the
original data (or throw/error) to avoid silently corrupting mapping; reference
the handleItemChange function, getItemId, setRvoImportReviewData, and
RvoImportReviewItem when adding this guard so the replacement only happens when
IDs match.
- Around line 78-115: The code currently overwrites userChoices every time
actionRvoImportReviewData changes, losing prior user edits; instead, merge
defaults only for new IDs and preserve existing selections: when handling
actionRvoImportReviewData inside the effect, call setUserChoices with a function
that receives prevChoices and for each item from actionRvoImportReviewData
computes id via getItemId and only assigns the default (based on item.status
like "CONFLICT" -> "UPDATE_FROM_REMOTE", etc.) if prevChoices[id] is undefined;
also keep the existing setRvoImportReviewData usage but only set
window.location.hash and setCanUnloadSafely(false) on the first transition into
review mode (when prev rvo state was falsy) to avoid reinitializing UI state.
- Around line 199-210: The hidden inputs "RvoImportReviewDataJson" and
"userChoices" are being JSON.parsed and only loosely type-checked before use;
instead add strict schema validation (e.g., Zod) in the upload-page component to
parse and validate both payloads (rvoImportReviewData and userChoices)
immediately after JSON.parse and before any DB operations in the handler that
processes them (the code around where JSON.parse is currently called and where
DB operations occur); define schemas that enforce required properties, exact
types, and allowed enum values for userChoices and the expected object/array
shapes for rvoImportReviewData, use schema.parse or safeParse to reject or throw
on invalid payloads, and handle failures by returning a safe error/response
rather than proceeding to the database.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 926b511e-c922-4fe5-9b98-a841e31fedeb
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
fdm-app/app/components/blocks/mijnpercelen/loader-and-action.server.tsfdm-app/app/components/blocks/mijnpercelen/upload-page.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.upload.tsxfdm-app/app/routes/farm.$b_id_farm._index.tsxfdm-app/app/routes/farm.create.$b_id_farm.$calendar._index.tsxfdm-app/package.jsonfdm-core/src/index.ts
✅ Files skipped from review due to trivial changes (2)
- fdm-app/app/routes/farm.create.$b_id_farm.$calendar._index.tsx
- fdm-app/package.json
🚧 Files skipped from review as they are similar to previous changes (4)
- fdm-core/src/index.ts
- fdm-app/app/routes/farm.$b_id_farm._index.tsx
- fdm-app/app/routes/farm.$b_id_farm.$calendar.upload.tsx
- fdm-app/app/components/blocks/mijnpercelen/loader-and-action.server.ts
Comment on lines
+78
to
+115
| useEffect(() => { | ||
| if (actionRvoImportReviewData) { | ||
| setRvoImportReviewData((oldData) => { | ||
| if (!oldData) { | ||
| window.location.hash = "#review" | ||
| } | ||
| return actionRvoImportReviewData | ||
| }) | ||
|
|
||
| // Initialize user choices with defaults | ||
| const initialChoices: UserChoiceMap = {} | ||
| actionRvoImportReviewData.forEach((item) => { | ||
| const id = getItemId(item) | ||
| let defaultAction: ImportReviewAction | ||
|
|
||
| switch (item.status) { | ||
| case "CONFLICT": | ||
| defaultAction = "UPDATE_FROM_REMOTE" | ||
| break | ||
| case "NEW_REMOTE": | ||
| defaultAction = "ADD_REMOTE" | ||
| break | ||
| case "NEW_LOCAL": | ||
| defaultAction = "KEEP_LOCAL" | ||
| break | ||
| case "EXPIRED_LOCAL": | ||
| defaultAction = "CLOSE_LOCAL" | ||
| break | ||
| // In creation wizard, other statuses are unlikely but good to handle defaults | ||
| default: | ||
| defaultAction = "NO_ACTION" | ||
| break | ||
| } | ||
| initialChoices[id] = defaultAction | ||
| }) | ||
| setCanUnloadSafely(false) | ||
| setUserChoices(initialChoices) | ||
| } |
Contributor
There was a problem hiding this comment.
userChoices gets re-initialized on every new actionRvoImportReviewData, which can wipe user edits.
If the action returns review data again (e.g., failed save/retry), defaults are rebuilt and previous selections are lost. Initialize defaults only once (first transition into review mode), or preserve existing keys.
💡 Suggested fix
+ const [isReviewInitialized, setIsReviewInitialized] = useState(false)
useEffect(() => {
if (actionRvoImportReviewData) {
setRvoImportReviewData((oldData) => {
if (!oldData) {
window.location.hash = "#review"
}
return actionRvoImportReviewData
})
- // Initialize user choices with defaults
- const initialChoices: UserChoiceMap = {}
- actionRvoImportReviewData.forEach((item) => {
- const id = getItemId(item)
- let defaultAction: ImportReviewAction
+ if (!isReviewInitialized) {
+ const initialChoices: UserChoiceMap = {}
+ actionRvoImportReviewData.forEach((item) => {
+ const id = getItemId(item)
+ let defaultAction: ImportReviewAction
- switch (item.status) {
- case "CONFLICT":
- defaultAction = "UPDATE_FROM_REMOTE"
- break
- case "NEW_REMOTE":
- defaultAction = "ADD_REMOTE"
- break
- case "NEW_LOCAL":
- defaultAction = "KEEP_LOCAL"
- break
- case "EXPIRED_LOCAL":
- defaultAction = "CLOSE_LOCAL"
- break
- // In creation wizard, other statuses are unlikely but good to handle defaults
- default:
- defaultAction = "NO_ACTION"
- break
- }
- initialChoices[id] = defaultAction
- })
- setCanUnloadSafely(false)
- setUserChoices(initialChoices)
+ switch (item.status) {
+ case "CONFLICT":
+ defaultAction = "UPDATE_FROM_REMOTE"
+ break
+ case "NEW_REMOTE":
+ defaultAction = "ADD_REMOTE"
+ break
+ case "NEW_LOCAL":
+ defaultAction = "KEEP_LOCAL"
+ break
+ case "EXPIRED_LOCAL":
+ defaultAction = "CLOSE_LOCAL"
+ break
+ default:
+ defaultAction = "NO_ACTION"
+ break
+ }
+ initialChoices[id] = defaultAction
+ })
+ setCanUnloadSafely(false)
+ setUserChoices(initialChoices)
+ setIsReviewInitialized(true)
+ }
}
- }, [actionRvoImportReviewData])
+ }, [actionRvoImportReviewData, isReviewInitialized])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useEffect(() => { | |
| if (actionRvoImportReviewData) { | |
| setRvoImportReviewData((oldData) => { | |
| if (!oldData) { | |
| window.location.hash = "#review" | |
| } | |
| return actionRvoImportReviewData | |
| }) | |
| // Initialize user choices with defaults | |
| const initialChoices: UserChoiceMap = {} | |
| actionRvoImportReviewData.forEach((item) => { | |
| const id = getItemId(item) | |
| let defaultAction: ImportReviewAction | |
| switch (item.status) { | |
| case "CONFLICT": | |
| defaultAction = "UPDATE_FROM_REMOTE" | |
| break | |
| case "NEW_REMOTE": | |
| defaultAction = "ADD_REMOTE" | |
| break | |
| case "NEW_LOCAL": | |
| defaultAction = "KEEP_LOCAL" | |
| break | |
| case "EXPIRED_LOCAL": | |
| defaultAction = "CLOSE_LOCAL" | |
| break | |
| // In creation wizard, other statuses are unlikely but good to handle defaults | |
| default: | |
| defaultAction = "NO_ACTION" | |
| break | |
| } | |
| initialChoices[id] = defaultAction | |
| }) | |
| setCanUnloadSafely(false) | |
| setUserChoices(initialChoices) | |
| } | |
| const [isReviewInitialized, setIsReviewInitialized] = useState(false) | |
| useEffect(() => { | |
| if (actionRvoImportReviewData) { | |
| setRvoImportReviewData((oldData) => { | |
| if (!oldData) { | |
| window.location.hash = "#review" | |
| } | |
| return actionRvoImportReviewData | |
| }) | |
| if (!isReviewInitialized) { | |
| const initialChoices: UserChoiceMap = {} | |
| actionRvoImportReviewData.forEach((item) => { | |
| const id = getItemId(item) | |
| let defaultAction: ImportReviewAction | |
| switch (item.status) { | |
| case "CONFLICT": | |
| defaultAction = "UPDATE_FROM_REMOTE" | |
| break | |
| case "NEW_REMOTE": | |
| defaultAction = "ADD_REMOTE" | |
| break | |
| case "NEW_LOCAL": | |
| defaultAction = "KEEP_LOCAL" | |
| break | |
| case "EXPIRED_LOCAL": | |
| defaultAction = "CLOSE_LOCAL" | |
| break | |
| // In creation wizard, other statuses are unlikely but good to handle defaults | |
| default: | |
| defaultAction = "NO_ACTION" | |
| break | |
| } | |
| initialChoices[id] = defaultAction | |
| }) | |
| setCanUnloadSafely(false) | |
| setUserChoices(initialChoices) | |
| setIsReviewInitialized(true) | |
| } | |
| } | |
| }, [actionRvoImportReviewData, isReviewInitialized]) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fdm-app/app/components/blocks/mijnpercelen/upload-page.tsx` around lines 78 -
115, The code currently overwrites userChoices every time
actionRvoImportReviewData changes, losing prior user edits; instead, merge
defaults only for new IDs and preserve existing selections: when handling
actionRvoImportReviewData inside the effect, call setUserChoices with a function
that receives prevChoices and for each item from actionRvoImportReviewData
computes id via getItemId and only assigns the default (based on item.status
like "CONFLICT" -> "UPDATE_FROM_REMOTE", etc.) if prevChoices[id] is undefined;
also keep the existing setRvoImportReviewData usage but only set
window.location.hash and setCanUnloadSafely(false) on the first transition into
review mode (when prev rvo state was falsy) to avoid reinitializing UI state.
Comment on lines
+199
to
+210
| <input | ||
| type="hidden" | ||
| name="userChoices" | ||
| value={JSON.stringify(userChoices)} | ||
| /> | ||
| <input | ||
| type="hidden" | ||
| name="RvoImportReviewDataJson" | ||
| value={JSON.stringify( | ||
| rvoImportReviewData, | ||
| )} | ||
| /> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify server-side parsing/validation of hidden inputs for this flow.
fd -i "loader-and-action.server.ts" | while read -r f; do
echo "=== $f ==="
rg -n -C3 "RvoImportReviewDataJson|userChoices|JSON\\.parse|safeParse|z\\.object|schema|validate" "$f"
doneRepository: nmi-agro/fdm
Length of output: 1945
Add strict schema validation before trusting hidden input payloads.
At lines 280–281, JSON.parse() is called directly on RvoImportReviewDataJson and userChoices with only basic type checks (lines 283–289). This allows tampering: malformed or malicious JSON can bypass detection if it passes Array.isArray() or instanceof Object checks.
Validate both payloads against a strict schema (e.g., Zod) before using them in database operations at lines 298–310. The schema should enforce field structure, required properties, and allowed enum values for user choices.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fdm-app/app/components/blocks/mijnpercelen/upload-page.tsx` around lines 199
- 210, The hidden inputs "RvoImportReviewDataJson" and "userChoices" are being
JSON.parsed and only loosely type-checked before use; instead add strict schema
validation (e.g., Zod) in the upload-page component to parse and validate both
payloads (rvoImportReviewData and userChoices) immediately after JSON.parse and
before any DB operations in the handler that processes them (the code around
where JSON.parse is currently called and where DB operations occur); define
schemas that enforce required properties, exact types, and allowed enum values
for userChoices and the expected object/array shapes for rvoImportReviewData,
use schema.parse or safeParse to reject or throw on invalid payloads, and handle
failures by returning a safe error/response rather than proceeding to the
database.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/mijnpercelen/upload-page.tsx`:
- Around line 137-146: The unload guard is left enabled after a failed save;
update the logic that flips canUnloadSafely so it is turned back off when a save
error occurs. In the effect or callback that handles save response (the code
reacting to actionData and the save_fields flow), add a branch that sets
canUnloadSafely = false (or calls the hook setter that controls canUnloadSafely)
whenever actionData exists and actionData.success is false, and only set
canUnloadSafely = true on a confirmed success; locate references to
canUnloadSafely and the save_fields response handling to implement this
reversal.
In `@fdm-app/app/components/blocks/rvo/import-review-table.tsx`:
- Around line 68-75: Callers of the RvoImportReviewTable component are still
omitting the now-required calendar prop, causing TypeScript errors and "Invalid
Date" output in the einddatum renderer for EXPIRED_LOCAL rows; update every
place that renders RvoImportReviewTable (e.g., the route render functions that
currently call RvoImportReviewTable without calendar) to pass the proper
calendar string (the route/loader's calendar param or derived calendar variable)
into the calendar prop, ensuring the component receives a valid calendar value
and the einddatum renderer can format dates correctly.
In `@fdm-rvo/package.json`:
- Line 62: The project currently lists "geojson" as a runtime dependency while
only importing its types (see shapefile.ts imports and shapefile.test.ts), and
`@types/geojson` must be available to consumers; remove the runtime "geojson"
entry from package.json dependencies, move "@types/geojson" from devDependencies
into dependencies so downstream TypeScript consumers can resolve the exported
GeoJSON types from shapefile.ts, and verify no actual runtime usage remains in
functions or modules referenced by shapefile.ts before publishing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 361107cd-15f6-4d05-9889-71b61b175867
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
fdm-app/app/components/blocks/mijnpercelen/upload-page.tsxfdm-app/app/components/blocks/rvo/import-review-table.tsxfdm-app/package.jsonfdm-core/src/index.tsfdm-rvo/package.json
✅ Files skipped from review due to trivial changes (1)
- fdm-app/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- fdm-core/src/index.ts
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-rvo/src/shapefile.ts`:
- Around line 149-157: The BeginDate and EndDate assignments call new
Date(...).toISOString() directly (using BEGINDAT and EINDDAT) which will throw
on invalid timestamps; validate each timestamp before calling toISOString and
throw the domain-specific error instead of letting Date throw. Update the
BeginDate expression that uses BEGINDAT and the EndDate branch that uses EINDDAT
to check the input (e.g., ensure numeric/parsable and finite) and only call
toISOString when valid; if invalid, throw the same domain error used elsewhere
in shapefile.ts so error handling remains consistent.
- Around line 124-136: The validation currently uses falsy checks that reject
valid zero values; update the conditional that checks SECTORVER, VOLGNR,
BEGINDAT, and EINDDAT so they test for null/undefined and numeric finiteness
rather than truthiness. For each of SECTORVER, VOLGNR, BEGINDAT, EINDDAT replace
checks like "!SECTORVER" with a check such as "SECTORVER == null ||
!Number.isFinite(SECTORVER)" (or coerce to Number first if values may be
strings) so 0 is accepted but null/undefined or non-finite values are rejected;
keep the other checks (e.g., NAAM === undefined) as-is and update the
conditional in shapefile.ts where these symbols are used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d3b98194-383d-450e-adf7-798f042c4747
📒 Files selected for processing (1)
fdm-rvo/src/shapefile.ts
Comment on lines
+149
to
+157
| BeginDate: new Date(BEGINDAT).toISOString(), // b_start | ||
| Country: "nl", // b_lu_catalogue[0] | ||
| CropTypeCode: GEWASCODE, // b_lu_catalogue[1] | ||
| UseTitleCode: TITEL, // b_acquiring_method | ||
| ThirdPartyCropFieldID: undefined, // not needed | ||
| EndDate: | ||
| EINDDAT !== 253402297199 | ||
| ? new Date(EINDDAT).toISOString() | ||
| : undefined, // b_end |
Contributor
There was a problem hiding this comment.
Validate timestamps before toISOString() for consistent domain errors.
new Date(...).toISOString() throws on invalid input. Add a timestamp validity check and throw your domain error to keep error handling predictable.
Suggested fix
+ const beginDate = new Date(BEGINDAT)
+ if (Number.isNaN(beginDate.getTime())) {
+ throw new Error("Field does not have the required attributes")
+ }
+
+ let endDate: string | undefined
+ if (EINDDAT !== 253402297199) {
+ const parsedEndDate = new Date(EINDDAT)
+ if (Number.isNaN(parsedEndDate.getTime())) {
+ throw new Error("Field does not have the required attributes")
+ }
+ endDate = parsedEndDate.toISOString()
+ }
+
return {
type: "Feature",
geometry: geometry,
properties: {
CropFieldID: SECTORID, // b_id_source
CropFieldVersion: "1.0.0", // not needed
CropFieldDesignator: trimmedNaam, // b_name
- BeginDate: new Date(BEGINDAT).toISOString(), // b_start
+ BeginDate: beginDate.toISOString(), // b_start
Country: "nl", // b_lu_catalogue[0]
CropTypeCode: GEWASCODE, // b_lu_catalogue[1]
UseTitleCode: TITEL, // b_acquiring_method
ThirdPartyCropFieldID: undefined, // not needed
- EndDate:
- EINDDAT !== 253402297199
- ? new Date(EINDDAT).toISOString()
- : undefined, // b_end
+ EndDate: endDate, // b_end
VarietyCode: undefined, // not needed🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fdm-rvo/src/shapefile.ts` around lines 149 - 157, The BeginDate and EndDate
assignments call new Date(...).toISOString() directly (using BEGINDAT and
EINDDAT) which will throw on invalid timestamps; validate each timestamp before
calling toISOString and throw the domain-specific error instead of letting Date
throw. Update the BeginDate expression that uses BEGINDAT and the EndDate branch
that uses EINDDAT to check the input (e.g., ensure numeric/parsable and finite)
and only call toISOString when valid; if invalid, throw the same domain error
used elsewhere in shapefile.ts so error handling remains consistent.
SvenVw
approved these changes
Apr 30, 2026
| <CardHeader> | ||
| <div className="flex items-center gap-4"> | ||
| <div className="rounded-lg bg-muted p-3"> | ||
| <CloudDownload className="h-6 w-6 text-primary" /> |
Collaborator
There was a problem hiding this comment.
Would better be an cloud upload icon. This looks duplicate with RVO download button
| </div> | ||
| <div> | ||
| <CardTitle> | ||
| Importeer percelen |
Collaborator
There was a problem hiding this comment.
Should be something like"Upload RVO shapefile"
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
fdm-rvo/src/shapefile.ts (1)
43-47: 💤 Low valueRemove redundant
awaitinsidePromise.all.The
awaitoperators inside the ternary expressions are unnecessary sincePromise.allalready awaits all promises. ThearrayBuffer()andtext()methods return promises thatPromise.allwill resolve.♻️ Suggested fix
const [shpData, shxData, projection] = await Promise.all([ - shp_file instanceof Blob ? await shp_file.arrayBuffer() : shp_file, - shx_file instanceof Blob ? await shx_file.arrayBuffer() : shx_file, - prj_file instanceof Blob ? await prj_file.text() : prj_file, + shp_file instanceof Blob ? shp_file.arrayBuffer() : shp_file, + shx_file instanceof Blob ? shx_file.arrayBuffer() : shx_file, + prj_file instanceof Blob ? prj_file.text() : prj_file, ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-rvo/src/shapefile.ts` around lines 43 - 47, The Promise.all call is using redundant await inside the ternary expressions; remove the inner awaits so shp_file instanceof Blob ? shp_file.arrayBuffer() : shp_file, shx_file instanceof Blob ? shx_file.arrayBuffer() : shx_file, and prj_file instanceof Blob ? prj_file.text() : prj_file are passed directly to Promise.all, leaving Promise.all([...]) to await resolution and assign to shpData, shxData, projection.fdm-rvo/src/shapefile.test.ts (1)
180-183: 💤 Low valueRemove debugging try-catch wrapper.
The try-catch block with
console.errorand re-throw appears to be a debugging artifact. Vitest will report assertion failures clearly without this wrapper.♻️ Suggested fix
it("should handle multi-polygon geometry", async () => { - try { const MOCK_MULTIPOLYGON = geometry("MultiPolygon", [ // ... coordinates ]) // ... test logic expect((parsed[0].geometry as Polygon).coordinates).toStrictEqual( MOCK_MULTIPOLYGON.coordinates, ) - } catch (e) { - console.error(e) - throw e - } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-rvo/src/shapefile.test.ts` around lines 180 - 183, Remove the debugging try-catch wrapper that catches errors as "e" and does console.error(e) then re-throws; specifically delete the try { ... } catch (e) { console.error(e); throw e } block in shapefile.test.ts (the test function surrounding that catch) so the test runner (Vitest) can surface assertion failures normally.
🤖 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-rvo/src/shapefile.test.ts`:
- Around line 180-183: Remove the debugging try-catch wrapper that catches
errors as "e" and does console.error(e) then re-throws; specifically delete the
try { ... } catch (e) { console.error(e); throw e } block in shapefile.test.ts
(the test function surrounding that catch) so the test runner (Vitest) can
surface assertion failures normally.
In `@fdm-rvo/src/shapefile.ts`:
- Around line 43-47: The Promise.all call is using redundant await inside the
ternary expressions; remove the inner awaits so shp_file instanceof Blob ?
shp_file.arrayBuffer() : shp_file, shx_file instanceof Blob ?
shx_file.arrayBuffer() : shx_file, and prj_file instanceof Blob ?
prj_file.text() : prj_file are passed directly to Promise.all, leaving
Promise.all([...]) to await resolution and assign to shpData, shxData,
projection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9516ec45-1173-4829-adbe-192ee684aeda
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
fdm-app/app/components/blocks/mijnpercelen/upload-page.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.rvo.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.upload.tsxfdm-app/app/routes/farm.$b_id_farm._index.tsxfdm-app/app/routes/farm.create.$b_id_farm.$calendar.rvo.tsxfdm-rvo/package.jsonfdm-rvo/src/shapefile.test.tsfdm-rvo/src/shapefile.ts
✅ Files skipped from review due to trivial changes (1)
- fdm-app/app/routes/farm.$b_id_farm.$calendar.upload.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
fdm-app/app/components/blocks/mijnpercelen/loader-and-action.server.ts (1)
258-295:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't trust
RvoImportReviewDataJsononsave_fields.This branch still rebuilds the full review model from hidden form inputs and passes it straight to
processRvoImport. A user can tamper with geometry, source ids, orlocalField.b_idbefore submit and import changes the server never produced. Reload the canonical review state from a server-owned token/session record, or validate every immutable field against server-stored review data before applyinguserChoices.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/mijnpercelen/loader-and-action.server.ts` around lines 258 - 295, Do not trust the client-provided RvoImportReviewDataJson when handling intent === "save_fields"; instead load the canonical review state from the server (e.g., fetch the stored review by session token or review ID in session) and replace/paranoid-validate the parsed rvoImportReviewData before calling processRvoImport; keep only the client-provided userChoices, but validate that any immutable fields (geometry, source ids, localField.b_id, etc.) in userChoices refer to entries in the server-stored review and reject or overwrite any mismatches, then pass the server-authoritative rvoImportReviewData plus the validated userChoices into processRvoImport.
🧹 Nitpick comments (1)
fdm-rvo/src/shapefile.test.ts (1)
5-19: 🏗️ Heavy liftPrefer manual/natural mocks over
viutilities in this test suiteThis file leans on
vi.mock/vi.fn/vi.mockedacross core behavior checks. In this repo, that pattern has been a known source of flaky or misleading tests; please refactor these cases to manual JS mocks / real error-triggering inputs.Based on learnings: When writing tests for the FDM codebase, avoid using Vitest's
vimocking utilities and prefer manual/natural mocks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-rvo/src/shapefile.test.ts` around lines 5 - 19, The test currently uses Vitest's vi.mock utilities to stub shpjs (see the vi.mock block mocking combine, parseShp, parseDbf) which we want to replace with a manual/natural mock; change the test to remove vi.mock and instead supply explicit, deterministic mock implementations for combine/parseShp/parseDbf (or refactor the code under test to accept parser functions so tests can pass in test doubles) and use those real functions to produce controlled input for convertShapefileFeatureIntoRvoField and getRvoFieldsFromShapefile; ensure the manual mocks return realistic shapes and DBF records (and also add tests that feed real error-triggering inputs) so behavior is asserted without using vi.fn/vi.mock.
🤖 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-rvo/src/shapefile.test.ts`:
- Around line 135-138: Replace the incorrect `.shx` filename used in the DBF
argument slot of the test: in the array of File constructors (the sequence
starting with new File([], "invalid.shp"), undefined, new File(...), new
File(...)) change the third entry from "invalid.shx" to "invalid.dbf" so the DBF
fixture is correctly named; locate the occurrence of new File([], "invalid.shx")
in shapefile.test.ts and update it to new File([], "invalid.dbf").
---
Duplicate comments:
In `@fdm-app/app/components/blocks/mijnpercelen/loader-and-action.server.ts`:
- Around line 258-295: Do not trust the client-provided RvoImportReviewDataJson
when handling intent === "save_fields"; instead load the canonical review state
from the server (e.g., fetch the stored review by session token or review ID in
session) and replace/paranoid-validate the parsed rvoImportReviewData before
calling processRvoImport; keep only the client-provided userChoices, but
validate that any immutable fields (geometry, source ids, localField.b_id, etc.)
in userChoices refer to entries in the server-stored review and reject or
overwrite any mismatches, then pass the server-authoritative rvoImportReviewData
plus the validated userChoices into processRvoImport.
---
Nitpick comments:
In `@fdm-rvo/src/shapefile.test.ts`:
- Around line 5-19: The test currently uses Vitest's vi.mock utilities to stub
shpjs (see the vi.mock block mocking combine, parseShp, parseDbf) which we want
to replace with a manual/natural mock; change the test to remove vi.mock and
instead supply explicit, deterministic mock implementations for
combine/parseShp/parseDbf (or refactor the code under test to accept parser
functions so tests can pass in test doubles) and use those real functions to
produce controlled input for convertShapefileFeatureIntoRvoField and
getRvoFieldsFromShapefile; ensure the manual mocks return realistic shapes and
DBF records (and also add tests that feed real error-triggering inputs) so
behavior is asserted without using vi.fn/vi.mock.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 68df923a-c83a-4f50-9612-bdc49372b742
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
fdm-app/app/components/blocks/mijnpercelen/loader-and-action.server.tsfdm-rvo/src/shapefile.test.tsfdm-rvo/src/shapefile.ts
✅ Files skipped from review due to trivial changes (1)
- fdm-rvo/src/shapefile.ts
Comment on lines
+135
to
+138
| new File([], "invalid.shp"), | ||
| undefined, | ||
| new File([], "invalid.shx"), | ||
| new File([], "invalid.prj"), |
Contributor
There was a problem hiding this comment.
Use a .dbf fixture name in the DBF argument slot
In this test, the 3rd argument (DBF position) is named "invalid.shx". Mocks currently hide the mismatch, but it obscures intent and can mask argument-order mistakes in future edits.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fdm-rvo/src/shapefile.test.ts` around lines 135 - 138, Replace the incorrect
`.shx` filename used in the DBF argument slot of the test: in the array of File
constructors (the sequence starting with new File([], "invalid.shp"), undefined,
new File(...), new File(...)) change the third entry from "invalid.shx" to
"invalid.dbf" so the DBF fixture is correctly named; locate the occurrence of
new File([], "invalid.shx") in shapefile.test.ts and update it to new File([],
"invalid.dbf").
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
Summary by CodeRabbit
New Features
Improvements
Tests