Conversation
… selected to be uploaded
…e shown when end date is within timeframe or undefined
|
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (4)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #268 +/- ##
=======================================
Coverage 92.78% 92.78%
=======================================
Files 87 87
Lines 13044 13045 +1
Branches 1294 1294
=======================================
+ Hits 12103 12104 +1
Misses 939 939
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
coderabbitai
Bot
changed the title
coderabbitai
Bot
added
branch:main
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
fdm-app/app/routes/farm.create.$b_id_farm.$calendar.upload.tsx (2)
198-215: Don’t use truthiness for required numeric fields.Numeric fields like
SECTORID,SECTORVER, orVOLGNRmay be0(falsy) yet valid. Use null/undefined checks instead.- if ( - !SECTORID || - !SECTORVER || - !NEN3610ID || - !VOLGNR || - !NAAM || - !BEGINDAT || - !EINDDAT || - !GEWASCODE || - !GEWASOMSCH || - !TITEL || - !TITELOMSCH - ) { + if ( + SECTORID == null || + SECTORVER == null || + !NEN3610ID || + VOLGNR == null || + !NAAM || + BEGINDAT == null || + EINDDAT == null || + !GEWASCODE || + !GEWASOMSCH || + !TITEL || + !TITELOMSCH + ) {
219-221: Critical: BEGINDAT/EINDDAT appear to be epoch seconds — currently parsed as ms.
new Date(20240101)yields a date in 1970; this will corrupt timeframe filtering and likely cause the “missing fields” symptom. Convert seconds→milliseconds, and treat “infinite” end dates robustly.- const b_start = new Date(BEGINDAT) - const b_end = EINDDAT === 253402297199 ? null : new Date(EINDDAT) + const epochToDate = (n: number) => new Date(n > 1e12 ? n : n * 1000) + const INFINITE_EPOCH_SEC = 253402300799 // 9999-12-31T23:59:59Z + const b_start = epochToDate(BEGINDAT) + const b_end = + Number(EINDDAT) >= INFINITE_EPOCH_SEC ? null : epochToDate(EINDDAT)fdm-core/src/field.ts (1)
421-427: Re‑throw the handled error from updateField.
catchcallshandleError(...)but does not throw, so the function may resolveundefinedon failure.- } catch (err) { - handleError(err, "Exception for updateField", { + } catch (err) { + throw handleError(err, "Exception for updateField", { b_id, b_name, b_id_source, // b_geometry, b_start, b_acquiring_method, b_end, })fdm-app/app/components/blocks/mijnpercelen/form-upload.tsx (1)
691-693: Harden getFileExtension for names without a dot.Current code returns the last char if no dot exists.
Apply this diff:
-const getFileExtension = (filename: string): string => { - return filename.slice(filename.lastIndexOf(".")).toLowerCase() -} +const getFileExtension = (filename: string): string => { + const idx = filename.lastIndexOf(".") + return idx >= 0 ? filename.slice(idx).toLowerCase() : "" +}
🧹 Nitpick comments (13)
.changeset/grumpy-rules-cut.md (1)
5-5: Clarify the patch note wording.Current sentence is ambiguous. Suggest:
-Fixes at `getFields` that fields acquired before start of timeframe are shown when end date is within timeframe or undefined +Fix `getFields` filtering: include fields acquired before the timeframe start if they overlap the timeframe (end is within the timeframe or undefined)..changeset/tiny-hornets-teach.md (1)
5-5: Polish the sentence for readability.-Fixes that files are removed for upload box when another (new) shapefile is selected to be uploaded +Fix: keep files in the upload box when selecting another (new) shapefile to uploadfdm-app/app/routes/farm.create.$b_id_farm.$calendar.upload.tsx (2)
224-234: Use a stable source identifier to reduce dedup/merge ambiguity.Set
b_id_sourcetoNEN3610IDto uniquely trace percelen back to the source and avoid name-based collisions downstream.- b_name, - null, + b_name, + NEN3610ID,
246-261: Optional: throttle external NMI calls for large uploads.Sequential calls will be slow for many fields; unbounded parallelism can overload the API. Consider a small concurrency limit (e.g., 3–5).
fdm-core/src/field.ts (1)
291-291: Add deterministic tie‑breaker to ordering.Equal areas will produce non‑deterministic order. Tie‑break by name ascending.
- .orderBy(desc(sql<number>`ST_Area(b_geometry::geography)`)) + .orderBy( + desc(sql<number>`ST_Area(b_geometry::geography)`), + asc(schema.fields.b_name), + )fdm-app/app/components/blocks/mijnpercelen/form-upload.tsx (2)
149-156: .dbf detection is case-sensitive; use the extension helper.
.endsWith(".dbf")misses uppercase.DBF. UsegetFileExtension.Apply this diff:
- const dbfFile = validFiles.find((file) => file.name.endsWith(".dbf")) + const dbfFile = validFiles.find( + (file) => getFileExtension(file.name) === ".dbf", + )
284-287: Await the async handler in DnD path for consistency.
handleFilesSetis async; not awaiting can cause racey UI state vs.clearData().Apply this diff:
- handleFilesSet(uniqueFiles) + await handleFilesSet(uniqueFiles).changeset/vast-ideas-lie.md (1)
6-6: Clarify patch notes and punctuate.Mention the timeframe overlap change and UI alignment; add a period.
Apply this diff:
-Fields are sorted based on descending area instead of alphabetical name +Fields are sorted by descending area instead of alphabetical name. Timeframe filtering uses inclusive overlap semantics; app lists now respect core order.fdm-app/app/routes/farm.$b_id_farm.$calendar.field._index.tsx (2)
211-217: Consistent area formatting.Ensure one decimal and locale formatting (nl-NL) for readability.
Apply this diff:
- <p className="text-sm text-muted-foreground"> - {option.b_area && - option.b_area > - 0.1 - ? `${option.b_area} ha` - : "< 0.1 ha"} - </p> + <p className="text-sm text-muted-foreground"> + {option.b_area > 0.1 + ? `${option.b_area.toLocaleString("nl-NL", { + minimumFractionDigits: 1, + maximumFractionDigits: 1, + })} ha` + : "< 0.1 ha"} + </p>
42-58: Docstring no longer matches behavior.It still mentions “sorting the fields alphabetically,” but sorting was removed and ordering now comes from core (area-desc).
Update the comment to reflect current behavior.
fdm-core/src/field.test.ts (3)
341-356: Comment contradicts the expectation.Text says “field 2 and field 4,” but assertion expects 2 and 3.
Apply this diff:
- // Test with a timeframe that includes field 2 and field 4 + // Test with a timeframe that includes field 2 and field 3
363-363: Remove console.log from tests.Leftover debug output adds noise to CI logs.
Apply this diff:
- console.log(fields4) + // console.log removed
311-340: Optional: add boundary assertions.Consider adding cases where
b_end === timeframe.startandb_start === timeframe.endto pin inclusive-overlap semantics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.changeset/grumpy-rules-cut.md(1 hunks).changeset/new-walls-relax.md(1 hunks).changeset/tiny-hornets-teach.md(1 hunks).changeset/vast-ideas-lie.md(1 hunks)fdm-app/app/components/blocks/mijnpercelen/form-upload.tsx(2 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.field._index.tsx(1 hunks)fdm-app/app/routes/farm.create.$b_id_farm.$calendar.fields.tsx(0 hunks)fdm-app/app/routes/farm.create.$b_id_farm.$calendar.upload.tsx(3 hunks)fdm-core/src/field.test.ts(8 hunks)fdm-core/src/field.ts(3 hunks)
💤 Files with no reviewable changes (1)
- fdm-app/app/routes/farm.create.$b_id_farm.$calendar.fields.tsx
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: SvenVw
PR: SvenVw/fdm#0
File: :0-0
Timestamp: 2025-08-13T10:33:05.313Z
Learning: In the fdm project, fdm-calculator integration for new features like b_lu_variety is handled in separate updates from the core data model changes. When fdm-core functions are updated to support new fields, fdm-calculator can consume these enhanced APIs without requiring changes in the same PR that introduces the core functionality.
📚 Learning: 2025-01-31T15:34:20.850Z
Learnt from: SvenVw
PR: SvenVw/fdm#67
File: fdm-app/app/routes/farm.create.$b_id_farm.fields.$b_id.tsx:601-610
Timestamp: 2025-01-31T15:34:20.850Z
Learning: The `updateField` function in fdm-core has optional parameters that don't need to be passed as undefined. Only `fdm` and `b_id` are required.
Applied to files:
.changeset/grumpy-rules-cut.mdfdm-app/app/routes/farm.$b_id_farm.$calendar.field._index.tsxfdm-core/src/field.test.ts
📚 Learning: 2025-01-09T16:03:37.764Z
Learnt from: SvenVw
PR: SvenVw/fdm#42
File: fdm-app/app/routes/farm/_b_id_farm/layout.tsx:46-95
Timestamp: 2025-01-09T16:03:37.764Z
Learning: A shared layout component `FarmLayoutBase` has been created in `components/custom/farm-layout-base.tsx` to maintain consistency across farm-related pages. The component handles farm selection dropdown, breadcrumb navigation, and provides a common layout structure.
Applied to files:
fdm-app/app/routes/farm.$b_id_farm.$calendar.field._index.tsx
📚 Learning: 2025-01-31T15:05:14.310Z
Learnt from: SvenVw
PR: SvenVw/fdm#67
File: fdm-app/app/routes/farm.create.$b_id_farm.fields.$b_id.tsx:601-610
Timestamp: 2025-01-31T15:05:14.310Z
Learning: The `updateField` function in fdm-core has optional parameters after `fdm` and `b_id`. The TypeScript definitions might show 8 required parameters due to a potential version mismatch.
Applied to files:
fdm-app/app/routes/farm.$b_id_farm.$calendar.field._index.tsxfdm-core/src/field.test.ts
📚 Learning: 2024-11-27T12:15:36.425Z
Learnt from: SvenVw
PR: SvenVw/fdm#9
File: fdm-data/src/cultivations/index.test.ts:57-59
Timestamp: 2024-11-27T12:15:36.425Z
Learning: In `fdm-data/src/cultivations/index.test.ts`, the `fdm` object created by `drizzle` does not have an `.end()` method. Cleanup code should not attempt to call `fdm.end();`.
Applied to files:
fdm-core/src/field.test.ts
📚 Learning: 2025-01-23T15:17:23.028Z
Learnt from: SvenVw
PR: SvenVw/fdm#49
File: fdm-app/app/routes/farm.create.$b_id_farm.atlas.tsx:208-208
Timestamp: 2025-01-23T15:17:23.028Z
Learning: The `addField` function in fdm-core should use database transactions and field verification to ensure field availability before resolving its promise, eliminating the need for sleep workarounds.
Applied to files:
fdm-core/src/field.test.ts
📚 Learning: 2025-01-23T15:17:23.027Z
Learnt from: SvenVw
PR: SvenVw/fdm#49
File: fdm-app/app/routes/farm.create.$b_id_farm.atlas.tsx:208-208
Timestamp: 2025-01-23T15:17:23.027Z
Learning: The `addField` function in fdm-core should verify field creation within the same transaction by checking the existence of the field and all its required relations (field data, acquiring info, geometry) before resolving its promise.
Applied to files:
fdm-core/src/field.test.ts
📚 Learning: 2025-01-24T11:28:01.882Z
Learnt from: SvenVw
PR: SvenVw/fdm#49
File: fdm-app/app/routes/farm.create.$b_id_farm.atlas.tsx:208-208
Timestamp: 2025-01-24T11:28:01.882Z
Learning: The `addField` function in fdm-core should use database transactions to ensure atomicity, and since transactions provide ACID guarantees, awaiting the inserts is sufficient to ensure field availability - no additional verification queries are needed.
Applied to files:
fdm-core/src/field.test.ts
📚 Learning: 2025-01-31T15:05:14.310Z
Learnt from: SvenVw
PR: SvenVw/fdm#67
File: fdm-app/app/routes/farm.create.$b_id_farm.fields.$b_id.tsx:601-610
Timestamp: 2025-01-31T15:05:14.310Z
Learning: The `updateField` function in fdm-core requires 8 parameters: fdm, b_id (required), and 6 optional parameters (b_name, b_id_source, b_geometry, b_acquiring_date, b_acquiring_method, b_discarding_date).
Applied to files:
fdm-core/src/field.test.ts
🧬 Code graph analysis (1)
fdm-core/src/field.test.ts (1)
fdm-core/src/field.ts (1)
getFields(212-304)
🔇 Additional comments (10)
.changeset/new-walls-relax.md (1)
5-5: LGTM — clear and accurate.Matches the added cleanup in the upload action’s finally block.
fdm-app/app/routes/farm.create.$b_id_farm.$calendar.upload.tsx (3)
104-107: Good: deterministic cleanup scaffolding added.Creating
LocalFileStorageonce per request and trackingstorageKeysenables reliable cleanup. Verify the storage backend auto-creates./uploads/shapefiles(or ensure it exists on boot).
272-276: LGTM — guaranteed cleanup.
finallyremoves all stored files regardless of success/failure. Good defensive hygiene.
122-127: Confirmed — filename preserved by @mjackson/file-storage/localLocalFileStorage preserves File.name across set(key, fileUpload) and get(key), so files.find(f => f.name.endsWith('.shp')) is safe; no changes required.
fdm-core/src/field.ts (2)
230-245: Interval overlap logic looks correct for start+end timeframes.
b_start <= timeframe.end AND (b_end IS NULL OR b_end >= timeframe.start)is the canonical overlap check.
247-256: Confirm semantics for “start‑only” timeframes.Current branch omits
b_start <= timeframe.start, which will include fields acquired after the given start date. If “active at the start instant” is intended, add the lower bound below.} else if (timeframe?.start) { whereClause = and( eq(schema.fieldAcquiring.b_id_farm, b_id_farm), + lte(schema.fieldAcquiring.b_start, timeframe.start), or( isNull(schema.fieldDiscarding.b_end), and( isNotNull(schema.fieldDiscarding.b_end), gte(schema.fieldDiscarding.b_end, timeframe.start), ), ), )fdm-app/app/components/blocks/mijnpercelen/form-upload.tsx (1)
645-657: Verify 5MB/file limit against real Mijn Percelen exports.Shapefile parts (.shp/.dbf) can exceed 5MB for larger farms; this might block legitimate uploads.
Would you like me to scan recent uploads’ size distribution and propose a safe ceiling (e.g., 25–50MB per part) with clear error copy?
fdm-core/src/field.test.ts (2)
213-226: End-date param in addField looks correct.Dates and call signature changes align with the updated API.
296-309: Field 4 timeframe setup LGTM.Matches the intended outside-2023 window.
fdm-app/app/routes/farm.$b_id_farm.$calendar.field._index.tsx (1)
201-206: Confirmed — no JSX keys use b_name; keep using b_id as the key.Searched the codebase for JSX key props referencing b_name and found no matches; b_name is only used for display text. The PR’s key={option.b_id} resolves the duplicate-name issue (#248).
…is incomplete and combine duplicate into single function
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
fdm-app/app/components/blocks/mijnpercelen/form-upload.tsx (2)
195-203: Fix confirmed: merge vs. replace now handled by helper.Using mergeShapefileParts here resolves the “incomplete new selection replaces full set” bug.
227-235: DnD path reuses the same merge logic — good DRY.Consistent behavior between input and drag-and-drop; also validates with shouldValidate.
🧹 Nitpick comments (7)
fdm-app/app/components/blocks/mijnpercelen/form-upload.tsx (3)
644-679: Enforce one-per-extension and avoid name-based dedupe.Current reduce() dedupes by name, not extension; if currentFiles is already inconsistent, duplicates per extension can persist. Prefer mapping by extension and returning in requiredExtensions order.
Apply this diff:
function mergeShapefileParts( currentFiles: File[], newFiles: File[], requiredExtensions: string[], ) { - const newExtensions = new Set( - newFiles.map((file) => getFileExtension(file.name)), - ) - const hasAllRequired = requiredExtensions.every((ext) => - newExtensions.has(ext), - ) - - if (hasAllRequired) { - return [...newFiles] - } - - const updatedFiles = [...currentFiles] - newFiles.forEach((newFile) => { - const newFileExt = getFileExtension(newFile.name) - const existingFileIndex = updatedFiles.findIndex( - (file) => getFileExtension(file.name) === newFileExt, - ) - if (existingFileIndex !== -1) { - updatedFiles[existingFileIndex] = newFile - } else { - updatedFiles.push(newFile) - } - }) - - return updatedFiles.reduce((acc, current) => { - if (!acc.find((item) => item.name === current.name)) { - acc.push(current) - } - return acc - }, [] as File[]) + const newExts = new Set(newFiles.map((f) => getFileExtension(f.name))) + const newHasAll = requiredExtensions.every((ext) => newExts.has(ext)) + if (newHasAll) return [...newFiles] + + const byExt = new Map<string, File>() + // start with current, then override with new + currentFiles.forEach((f) => byExt.set(getFileExtension(f.name), f)) + newFiles.forEach((f) => byExt.set(getFileExtension(f.name), f)) + // return in requiredExtensions order; filters out accidental dupes + return requiredExtensions + .map((ext) => byExt.get(ext)) + .filter((f): f is File => !!f) }
640-642: Guard getFileExtension against filenames without dots.Prevents slice(-1) returning the last character as an “extension”.
-const getFileExtension = (filename: string): string => { - return filename.slice(filename.lastIndexOf(".")).toLowerCase() -} +const getFileExtension = (filename: string): string => { + const idx = filename.lastIndexOf(".") + return idx === -1 ? "" : filename.slice(idx).toLowerCase() +}
420-429: Add accept to the file input for UX.Limit the picker to shapefile parts on supported browsers.
-<Input +<Input + accept=".shp,.shx,.dbf,.prj"fdm-core/src/field.ts (3)
229-245: Timeframe overlap logic (start+end) is correct and fixes missing fields. Add explicit semantics to JSDoc.
This matches standard interval overlap. Please document the 3 modes to avoid future regressions.Proposed doc tweak:
@@ - * @param timeframe - Optional timeframe to filter fields by start and end dates. + * @param timeframe - Optional timeframe: + * - start & end set: include fields where [b_start, b_end] overlaps [start, end]. + * - only start set: include fields with no end or b_end >= start (active on/after start). + * - only end set: include fields with b_start <= end (started by end).
237-243: Simplify NULL handling in OR.
OR (b_end IS NULL OR b_end >= X)is sufficient;IS NOT NULLguard is redundant.- or( - isNull(schema.fieldDiscarding.b_end), - and( - isNotNull(schema.fieldDiscarding.b_end), - gte(schema.fieldDiscarding.b_end, timeframe.start), - ), - ), + or( + isNull(schema.fieldDiscarding.b_end), + gte(schema.fieldDiscarding.b_end, timeframe.start), + ),
290-291: Add stable tie-breaker to ordering; avoid duplicate area expression.
Current order may be nondeterministic when areas are equal. Also, the area SQL is repeated.-import { - and, - desc, +import { + and, + asc, + desc, @@ - const fields = await fdm + const areaExpr = sql<number>`ST_Area(b_geometry::geography)` + const fields = await fdm @@ - .orderBy(desc(sql<number>`ST_Area(b_geometry::geography)`)) + .orderBy(desc(areaExpr), asc(schema.fields.b_name))fdm-core/src/field.test.ts (1)
341-356: Timeframe3 correctness; consider adding an explicit sort-by-area test.
You switched backend order to area desc; tests don’t assert ordering.Add this test within the same describe to lock ordering:
+ it("should order fields by area descending", async () => { + // Arrange: two different areas + const bigFieldName = "Big Field" + const bigGeom = { + type: "Polygon", + coordinates: [[[0,0],[0,50],[50,50],[50,0],[0,0]]], + } + await addField( + fdm, + principal_id, + b_id_farm, + bigFieldName, + "big-src", + bigGeom, + new Date("2023-01-01"), + "nl_01", + ) + // Act + const res = await getFields(fdm, principal_id, b_id_farm) + // Assert: biggest area first + expect(res[0].b_name).toBe(bigFieldName) + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/grumpy-rules-cut.md(1 hunks).changeset/tiny-hornets-teach.md(1 hunks).changeset/vast-ideas-lie.md(1 hunks)fdm-app/app/components/blocks/mijnpercelen/form-upload.tsx(4 hunks)fdm-core/src/field.test.ts(8 hunks)fdm-core/src/field.ts(4 hunks)
✅ Files skipped from review due to trivial changes (2)
- .changeset/vast-ideas-lie.md
- .changeset/tiny-hornets-teach.md
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/grumpy-rules-cut.md
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-01-31T15:05:14.310Z
Learnt from: SvenVw
PR: SvenVw/fdm#67
File: fdm-app/app/routes/farm.create.$b_id_farm.fields.$b_id.tsx:601-610
Timestamp: 2025-01-31T15:05:14.310Z
Learning: The `updateField` function in fdm-core has optional parameters after `fdm` and `b_id`. The TypeScript definitions might show 8 required parameters due to a potential version mismatch.
Applied to files:
fdm-core/src/field.test.ts
📚 Learning: 2025-01-23T15:17:23.027Z
Learnt from: SvenVw
PR: SvenVw/fdm#49
File: fdm-app/app/routes/farm.create.$b_id_farm.atlas.tsx:208-208
Timestamp: 2025-01-23T15:17:23.027Z
Learning: The `addField` function in fdm-core should verify field creation within the same transaction by checking the existence of the field and all its required relations (field data, acquiring info, geometry) before resolving its promise.
Applied to files:
fdm-core/src/field.test.ts
📚 Learning: 2025-01-23T15:17:23.028Z
Learnt from: SvenVw
PR: SvenVw/fdm#49
File: fdm-app/app/routes/farm.create.$b_id_farm.atlas.tsx:208-208
Timestamp: 2025-01-23T15:17:23.028Z
Learning: The `addField` function in fdm-core should use database transactions and field verification to ensure field availability before resolving its promise, eliminating the need for sleep workarounds.
Applied to files:
fdm-core/src/field.test.ts
📚 Learning: 2025-01-24T11:28:01.882Z
Learnt from: SvenVw
PR: SvenVw/fdm#49
File: fdm-app/app/routes/farm.create.$b_id_farm.atlas.tsx:208-208
Timestamp: 2025-01-24T11:28:01.882Z
Learning: The `addField` function in fdm-core should use database transactions to ensure atomicity, and since transactions provide ACID guarantees, awaiting the inserts is sufficient to ensure field availability - no additional verification queries are needed.
Applied to files:
fdm-core/src/field.test.ts
📚 Learning: 2025-01-31T15:05:14.310Z
Learnt from: SvenVw
PR: SvenVw/fdm#67
File: fdm-app/app/routes/farm.create.$b_id_farm.fields.$b_id.tsx:601-610
Timestamp: 2025-01-31T15:05:14.310Z
Learning: The `updateField` function in fdm-core requires 8 parameters: fdm, b_id (required), and 6 optional parameters (b_name, b_id_source, b_geometry, b_acquiring_date, b_acquiring_method, b_discarding_date).
Applied to files:
fdm-core/src/field.test.tsfdm-core/src/field.ts
📚 Learning: 2025-01-31T15:34:20.850Z
Learnt from: SvenVw
PR: SvenVw/fdm#67
File: fdm-app/app/routes/farm.create.$b_id_farm.fields.$b_id.tsx:601-610
Timestamp: 2025-01-31T15:34:20.850Z
Learning: The `updateField` function in fdm-core has optional parameters that don't need to be passed as undefined. Only `fdm` and `b_id` are required.
Applied to files:
fdm-core/src/field.test.tsfdm-core/src/field.ts
🧬 Code graph analysis (2)
fdm-core/src/field.test.ts (2)
fdm-core/src/field.ts (1)
getFields(211-303)fdm-core/src/index.ts (1)
getFields(97-97)
fdm-core/src/field.ts (1)
fdm-app/app/entry.server.tsx (1)
handleError(180-186)
🔇 Additional comments (12)
fdm-app/app/components/blocks/mijnpercelen/form-upload.tsx (2)
149-151: Extension-based DBF detection — LGTM.Switching to getFileExtension(...) === ".dbf" avoids false negatives on case/extra dots.
594-638: Verify 5MB limit is sufficient for real RVO exports.Some shapefile parts can exceed 5MB; confirm with production data.
If needed, I can adjust the schema and surface a friendlier error per-part and total size.
fdm-core/src/field.ts (3)
3-3: Import change to desc looks good.
246-255: Start-only filter includes future-acquired fields — confirm intent.
This returns records with null end or end >= start regardless of b_start. If the goal is “active on or after start,” this is fine; if it should be “active at start,” addlte(b_start, timeframe.start).Optional adjustment (only if “active at start” is desired):
whereClause = and( eq(schema.fieldAcquiring.b_id_farm, b_id_farm), - or(isNull(schema.fieldDiscarding.b_end), gte(schema.fieldDiscarding.b_end, timeframe.start)), + lte(schema.fieldAcquiring.b_start, timeframe.start), + or(isNull(schema.fieldDiscarding.b_end), gte(schema.fieldDiscarding.b_end, timeframe.start)), )
430-439: Good: rethrowing withthrow handleError(...).
This preserves stack control flow and centralizes context.fdm-core/src/field.test.ts (7)
213-226: Good: field with explicit endDate exercises overlap paths.
242-253: Good: second field without endDate covers open interval.
296-309: Good: later-dated field validates start-only behavior.
311-324: Timeframe1 correctness.
Includes only Field 2 as expected.
327-339: Timeframe2 correctness.
Overlap logic includes Fields 1 & 2.
358-370: Start-only semantics test matches implementation. Document intent in getFields JSDoc to avoid confusion.
376-379: End-only semantics test matches implementation.
gerardhros
left a comment
gerardhros
left a comment
There was a problem hiding this comment.
solves an issue indeed. thankx.
2 participants
Summary by CodeRabbit
Closes #248