Add page to add a single field#116
Conversation
🦋 Changeset detectedLatest commit: a131122 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThis update introduces a new page for adding a field to a farm along with several related improvements. It includes minor version updates for the fdm-app and fdm-core packages, modifications to type definitions, bounding box calculations, and style logic in atlas components. The combobox component is streamlined by replacing the form prop with an onChange callback. A new FieldDetailsDialog component and validation schema have been added for better field detail management. Changes also extend to farm routes, soil analysis query logic, NMI API integration, and various dependency upgrades. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant UI as UI Components (Map, NavLink, Dialog)
participant Loader as Loader Function
participant Form as FieldDetailsDialog
participant Action as Action Function
participant API as External API (Soil Analysis)
U->>UI: Load farm field creation page
UI->>Loader: Request farm, field, and cultivation data
Loader-->>UI: Return necessary data
U->>UI: Interact with map/select field area
UI->>Form: Open FieldDetailsDialog for input
U->>Form: Fill in field details and submit
Form->>Action: Send submission data
Action->>API: Call NMI API for soil parameter estimates (if required)
API-->>Action: Return API response (validated via Zod)
Action-->>UI: Confirm field creation and redirect user
Possibly related PRs
Suggested Reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## development #116 +/- ##
===============================================
+ Coverage 92.64% 92.66% +0.02%
===============================================
Files 44 44
Lines 4702 4717 +15
Branches 502 503 +1
===============================================
+ Hits 4356 4371 +15
Misses 345 345
Partials 1 1
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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
fdm-app/app/routes/farm.$b_id_farm.$calendar.field.new.tsx (2)
138-159: Refactor the duplicated cultivation retrieval logic.This block for fetching and mapping cultivations is repeated in both loader and action. Centralizing it in a utility (e.g.,
getCultivationsForDropdown) can reduce duplication and ensure consistency in validation and error handling.
347-361: Centralize cultivation checks in a shared module.This block duplicates the loader’s logic for retrieving and filtering cultivations. Reusing a shared utility would strengthen maintainability and ensure consistent behavior across loader and action.
🧹 Nitpick comments (5)
fdm-app/app/components/custom/combobox.tsx (2)
84-94: Simplify the aria-label implementation.The current aria-label implementation uses complex string concatenation inside a template literal, which could be simplified for better readability.
- aria-label={`Selecteer ${ - options.find( - (option) => option.value === currentValue, - )?.label || - defaultLabel || - "Klik om te begin met typen..." - }`} + aria-label={`Selecteer ${options.find((option) => option.value === currentValue)?.label || defaultLabel || "Klik om te begin met typen..."}`}
150-151: Consider removing empty form components.The
FormDescriptionandFormMessagecomponents are empty. If they're not being used, consider removing them to clean up the code.- <FormDescription /> - <FormMessage />fdm-app/app/components/custom/field/form.tsx (1)
44-45: Consider extracting the prefix logic into a separate functionThe prefix
nl_being added to the field's cultivation code suggests a naming convention for cultivation values. For maintainability, consider extracting this into a separate utility function.- const b_lu_catalogue = `nl_${field.properties?.b_lu_catalogue ?? ""}` + const getCultivationId = (code: string | undefined) => `nl_${code ?? ""}` + const b_lu_catalogue = getCultivationId(field.properties?.b_lu_catalogue)fdm-app/app/routes/farm.$b_id_farm.$calendar.field.new.tsx (2)
90-100: Consider defensive checks for farm data properties.While you already handle the case where
farm?.b_id_farmorfarm?.b_name_farmis missing, it might help to log or gracefully degrade if future fields are unexpectedly undefined. This can simplify troubleshooting when farm data is malformed.
200-205: Mind the null checks for selected fields.When toggling the field selection, ensure you handle the case where the feature lacks necessary properties. A null or undefined geometry could cause unexpected behavior in the dialog or map logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.changeset/slick-buses-bow.md(1 hunks)fdm-app/app/components/custom/combobox.tsx(2 hunks)fdm-app/app/components/custom/field/form.tsx(1 hunks)fdm-app/app/components/custom/field/schema.tsx(1 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.field.new.tsx(1 hunks)fdm-app/app/routes/farm.create.$b_id_farm.$calendar.atlas.tsx(0 hunks)fdm-core/src/soil.test.ts(1 hunks)fdm-core/src/soil.ts(3 hunks)
💤 Files with no reviewable changes (1)
- fdm-app/app/routes/farm.create.$b_id_farm.$calendar.atlas.tsx
🧰 Additional context used
🧠 Learnings (1)
fdm-app/app/routes/farm.$b_id_farm.$calendar.field.new.tsx (1)
Learnt from: SvenVw
PR: SvenVw/fdm#116
File: fdm-app/app/routes/farm.$b_id_farm.$calendar.field.new.tsx:111-154
Timestamp: 2025-04-04T14:27:39.518Z
Learning: In the FDM application, cultivation retrieval logic should be centralized in utility functions rather than duplicated across loader and action functions to improve maintainability and ensure consistent behavior.
🧬 Code Definitions (3)
fdm-core/src/soil.test.ts (1)
fdm-core/src/soil.ts (3)
addSoilAnalysis(34-89)getSoilAnalyses(245-324)getCurrentSoilData(343-424)
fdm-app/app/components/custom/field/form.tsx (2)
fdm-app/app/components/custom/field/schema.tsx (1)
FormSchema(22-22)fdm-app/app/components/custom/combobox.tsx (1)
Combobox(41-154)
fdm-app/app/routes/farm.$b_id_farm.$calendar.field.new.tsx (6)
fdm-app/app/store/calendar.tsx (1)
useCalendarStore(8-11)fdm-app/app/components/custom/atlas/atlas-styles.tsx (1)
getFieldsStyle(7-47)fdm-app/app/components/custom/atlas/atlas-sources.tsx (1)
FieldsSourceAvailable(103-180)fdm-app/app/components/custom/atlas/atlas-panels.tsx (1)
FieldsPanelZoom(100-143)fdm-app/app/components/custom/field/form.tsx (1)
FieldDetailsDialog(37-142)fdm-app/app/components/custom/field/schema.tsx (1)
FormSchema(22-22)
🪛 Biome (1.9.4)
fdm-app/app/components/custom/combobox.tsx
[error] 79-80: The elements with the following roles can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
🔇 Additional comments (22)
.changeset/slick-buses-bow.md (1)
1-6: Versioning update for soil data filtering looks good.
The changeset correctly documents the patch version update for @svenvw/fdm-core, explaining that it fixes an issue with soil data without sampling dates not being properly excluded. This aligns with the implementation changes in fdm-core/src/soil.ts.
fdm-app/app/components/custom/field/schema.tsx (1)
1-22: Well-structured Zod validation schema for field details.
The schema provides appropriate validation rules and clear error messages for each field. The required validation on b_name and b_lu_catalogue with specific error messages will ensure users receive helpful feedback when inputting data incorrectly.
fdm-app/app/components/custom/combobox.tsx (3)
32-39: Interface updates improve component reusability.
The removal of the form prop and addition of the onChange callback makes this component more flexible and reusable across different contexts, not just within form libraries.
49-52: Good state management implementation.
Adding currentValue state and initializing it with defaultValue ensures the component can track its selection independently of any parent form.
145-149: Hidden input implementation is effective.
The hidden input ensures that the selected value is properly submitted with the form, which is a good pattern for custom form controls.
fdm-core/src/soil.ts (5)
1-1: Import addition reflects updated query logic.
Adding isNull and or imports from 'drizzle-orm' supports the new query conditions that include null handling in the sampling date filtering.
265-272: Proper SQL condition handling for null sampling dates.
The updated query logic in getSoilAnalyses using or with isNull checks correctly addresses the issue mentioned in the changeset. This ensures that soil data records without sampling dates are properly included in the results.
277-280: Consistent null handling for timeframe start.
This change maintains consistency with the other query conditions, ensuring records with null sampling dates are properly handled when filtering by a start date.
285-288: Consistent null handling for timeframe end.
Similar to the previous segment, this ensures proper handling of null sampling dates when filtering by an end date.
364-367: Similar fix applied to getCurrentSoilData function.
The same null-handling pattern is correctly applied in the getCurrentSoilData function, maintaining consistency across the codebase and fully addressing the issue described in the changeset.
fdm-core/src/soil.test.ts (2)
650-698: Well-structured test for handling null sampling dates
This test case properly verifies that soil analyses with null sampling dates can be retrieved and are correctly ordered. The test follows a clear structure: add analyses with valid dates, add an analysis with a null date, retrieve all analyses, and verify the order.
The assertions properly confirm that analyses with valid dates appear before those with null dates, which matches the NULLS LAST clause in the ordering implementation from the getSoilAnalyses function.
700-770: Comprehensive test for null sampling dates with timeframe filtering
This test effectively extends the previous test by adding timeframe filtering and property validation. The code properly tests:
That null sampling dates are correctly handled when filtering by timeframe
That analyses are ordered with valid dates first, null dates last
That the correct property values are retrieved through getCurrentSoilData
The assertions for property values in lines 766-769 are particularly valuable as they verify that property values from both analyses with valid dates and null dates are correctly retrieved based on recency and availability.
fdm-app/app/components/custom/field/form.tsx (7)
29-35: Clear interface definition with appropriate props
The FieldDetailsDialogProps interface is well-defined with all necessary props for the dialog component. The props provide the expected functionality for controlling dialog visibility, providing field data, and customizing the dialog behavior.
47-58: Good form setup with appropriate validation and defaults
The form setup is well-implemented using remix-hook-form with proper validation through Zod. The form includes necessary default values and additional submit data for field properties. The usage of mode: "onTouched" provides a good user experience by validating fields as they're interacted with.
60-68: Well-structured effects for form synchronization
Both effects handle important aspects of form management:
Synchronizing the form value with the selected cultivation state
Resetting the form after successful submission
The dependency arrays are correctly specified for each effect.
83-104: Clean implementation of name field with proper validation
The name field implementation is well-structured with appropriate label, control, and validation message components. The field is correctly marked as required and properly disables during form submission.
122-135: Good user feedback during form submission
The submit button implementation provides clear feedback during submission with a loading spinner and "Opslaan..." text. The button is properly disabled during submission to prevent multiple form submissions.
70-140: Well-structured dialog with appropriate accessibility features
The overall dialog structure follows best practices with:
Clear header with title and description
Proper form structure with fieldset to disable the entire form during submission
Required fields marked with an asterisk
Appropriate button states
Clean layout with proper spacing
The component correctly handles the open/close state of the dialog.
105-119:
✅ Verification successful
Verify that Combobox renders validation messages appropriately
The Combobox component is being used here with the form's validation, but there's no direct integration with the form's control props like the name field above it. Based on the provided context, ensure that validation errors for b_lu_catalogue are displayed properly.
🏁 Script executed:
#!/bin/bash
# Find how Combobox handles form validation messages
rg -A 5 -B 5 "FormMessage" fdm-app/app/components/custom/combobox.tsx
Length of output: 455
Combobox Validation Integration Confirmed
After verifying the Combobox component in fdm-app/app/components/custom/combobox.tsx, we’ve confirmed that it imports and renders the <FormMessage /> component, which displays validation errors as expected. In the usage within the field form (fdm-app/app/components/custom/field/form.tsx), passing the name="b_lu_catalogue" prop correctly integrates with the form’s validation—ensuring that any errors for this field will be rendered by <FormMessage />.
No changes are necessary in this part of the code.
fdm-app/app/routes/farm.$b_id_farm.$calendar.field.new.tsx (3)
73-82: Validate b_id_farm parameter before performing additional logic.
Great job returning early when b_id_farm is missing. This pattern ensures errors are caught quickly and improves clarity. No concerns here.
309-316: Good user interface approach for adding new fields.
Displaying the dialog on field selection is a neat UX pattern. No issues found here—this is straightforward and user-friendly.
321-330: Early return for missing b_id_farm.
Similar to the loader, this ensures immediate feedback to the user if the route parameter is absent. This is an appropriate error-handling approach.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
fdm-app/app/components/custom/combobox.tsx (1)
78-92: Address accessibility issue with role attribute.The static analysis tool flagged an accessibility concern with using
role="combobox"on a Button element.- <Button - variant="outline" - role="combobox" + <select + className="w-full justify-between truncate focus-visible:ring-2 px-3 py-2 border border-input bg-background h-10 rounded-md"Or alternatively, ensure this custom implementation follows all ARIA authoring practices for combobox: https://www.w3.org/WAI/ARIA/apg/patterns/combobox/
🧰 Tools
🪛 Biome (1.9.4)
[error] 78-79: The elements with the following roles can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles (lint/a11y/useSemanticElements) fdm-app/app/routes/farm.$b_id_farm.$calendar.field.new.tsx (2) 139-160: Duplicated cultivation retrieval logic (already noted in past reviews). This code block to fetch and map cultivations is repeated in the action function (lines 349-369). Centralizing it in a utility function (e.g., getCultivationsForDropdown) will improve maintainability and ensure consistency. 349-369: Repeated cultivation check (already noted in past reviews). This duplication matches the loader’s logic at lines 139-160. Extracting and reusing a shared function for cultivation retrieval and validation avoids errors and simplifies future changes. 🧹 Nitpick comments (1) fdm-app/app/routes/farm.$b_id_farm.$calendar.field.new.tsx (1) 324-324: Consider removing the debug log. The console statement is likely for debugging; removing it or replacing it with a structured logger would clean up the production code. - console.log("Action!") + // console.debug("Action triggered") 📜 Review details Configuration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📥 Commits Reviewing files that changed from the base of the PR and between d693cdb and 2ab7ffd. ⛔ Files ignored due to path filters (1) pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml 📒 Files selected for processing (7) fdm-app/app/components/custom/atlas/atlas-panels.tsx (1 hunks) fdm-app/app/components/custom/combobox.tsx (2 hunks) fdm-app/app/components/custom/field/form.tsx (1 hunks) fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.cultivation._index.tsx (1 hunks) fdm-app/app/routes/farm.$b_id_farm.$calendar.field._index.tsx (2 hunks) fdm-app/app/routes/farm.$b_id_farm.$calendar.field.new.tsx (1 hunks) fdm-app/app/routes/farm.create.$b_id_farm.$calendar.atlas.tsx (1 hunks) 🚧 Files skipped from review as they are similar to previous changes (4) fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.cultivation._index.tsx fdm-app/app/routes/farm.$b_id_farm.$calendar.field._index.tsx fdm-app/app/components/custom/field/form.tsx fdm-app/app/routes/farm.create.$b_id_farm.$calendar.atlas.tsx 🧰 Additional context used 🧠 Learnings (1) fdm-app/app/routes/farm.$b_id_farm.$calendar.field.new.tsx (1) Learnt from: SvenVw PR: #116 File: fdm-app/app/routes/farm.$b_id_farm.$calendar.field.new.tsx:111-154 Timestamp: 2025-04-04T14:27:39.518Z Learning: In the FDM application, cultivation retrieval logic should be centralized in utility functions rather than duplicated across loader and action functions to improve maintainability and ensure consistent behavior. 🧬 Code Definitions (1) fdm-app/app/routes/farm.$b_id_farm.$calendar.field.new.tsx (5) fdm-app/app/integrations/mapbox.ts (2) getMapboxToken (3-10) getMapboxStyle (12-14) fdm-app/app/store/calendar.tsx (1) useCalendarStore (8-11) fdm-app/app/components/custom/atlas/atlas-styles.tsx (1) getFieldsStyle (7-47) fdm-app/app/components/custom/field/form.tsx (1) FieldDetailsDialog (37-147) fdm-app/app/integrations/nmi.ts (2) getNmiApiKey (5-12) getSoilParameterEstimates (14-58) 🪛 Biome (1.9.4) fdm-app/app/components/custom/combobox.tsx [error] 78-79: The elements with the following roles can be changed to the following elements:For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
fdm-app/app/components/custom/atlas/atlas-panels.tsx
[error] 20-20: Shouldn't redeclare 'FeatureCollection'. Consider to delete it or rename it.
'FeatureCollection' is defined here:
(lint/suspicious/noRedeclare)
[error] 21-21: Shouldn't redeclare 'throttle'. Consider to delete it or rename it.
'throttle' is defined here:
(lint/suspicious/noRedeclare)
[error] 22-22: Shouldn't redeclare 'Check'. Consider to delete it or rename it.
'Check' is defined here:
(lint/suspicious/noRedeclare)
[error] 22-22: Shouldn't redeclare 'Info'. Consider to delete it or rename it.
'Info' is defined here:
(lint/suspicious/noRedeclare)
[error] 23-23: Shouldn't redeclare 'useEffect'. Consider to delete it or rename it.
'useEffect' is defined here:
(lint/suspicious/noRedeclare)
[error] 23-23: Shouldn't redeclare 'useState'. Consider to delete it or rename it.
'useState' is defined here:
(lint/suspicious/noRedeclare)
[error] 24-24: Shouldn't redeclare 'useMap'. Consider to delete it or rename it.
'useMap' is defined here:
(lint/suspicious/noRedeclare)
[error] 25-25: Shouldn't redeclare 'MapBoxZoomEvent'. Consider to delete it or rename it.
'MapBoxZoomEvent' is defined here:
(lint/suspicious/noRedeclare)
[error] 25-25: Shouldn't redeclare 'MapMouseEvent'. Consider to delete it or rename it.
'MapMouseEvent' is defined here:
(lint/suspicious/noRedeclare)
[error] 26-26: Shouldn't redeclare 'useFetcher'. Consider to delete it or rename it.
'useFetcher' is defined here:
(lint/suspicious/noRedeclare)
🔇 Additional comments (6)
fdm-app/app/components/custom/combobox.tsx (5)
37-37: Good addition of onChange prop.Adding the onChange callback improves the component's API by allowing parent components to react to value changes.
49-51: Good state management implementation.The addition of
currentValuestate with proper initialization fromdefaultValueensures the component correctly tracks the selected option.
116-124: Good implementation of selection handling.The onSelect handler now correctly:
- Updates the internal state
- Calls the onChange callback to notify parent components
- Closes the popover after selection
This provides a complete update cycle for the component.
132-135: Good visual feedback for selection state.Using the Check icon with conditional opacity provides clear visual feedback to users about which option is currently selected.
144-148: Well-implemented hidden input for form submission.The addition of a hidden input that captures the currentValue ensures the selected value is properly submitted with the form, which is a clean solution after removing the form prop.
fdm-app/app/routes/farm.$b_id_farm.$calendar.field.new.tsx (1)
65-73: Well-documented loader function.
These JSDoc comments clearly explain the loader’s purpose, inputs, and outputs, which helps maintainability. Nice job!
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
fdm-app/app/routes/farm.$b_id_farm.$calendar.field.new.tsx (1)
139-155: 🛠️ Refactor suggestionCentralize cultivation retrieval logic.
This block duplicates logic also seen in the action function (lines 350–369). Consider creating a shared utility to fetch, filter, and transform cultivations. This is more maintainable and ensures consistent results for both loader and action.- // Get the available cultivations here - let cultivationOptions = [] - const cultivationsCatalogue = await getCultivationsFromCatalogue(...) - ... - .map((cultivation) => ({ ... })) + // For example, create and use a utility getCultivationsForDropdown(...) + const cultivationOptions = await getCultivationsForDropdown(...)
🧹 Nitpick comments (6)
fdm-app/app/routes/farm.$b_id_farm.$calendar.field.new.tsx (5)
1-22: Consider trimming unused imports or grouping them by domain.
You've included a variety of imports for atlas, mapbox, and other libraries. Some might not be strictly required in this file, or could be grouped to make it easier to see which parts of the code reference external libraries vs. local modules.
74-83: Validate the presence ofparams.b_id_farmearlier in the request pipeline.
While this block properly checks if theb_id_farmparameter is empty, consider validating route parameters and user access constraints at a higher level or earlier in the flow (e.g., with a dedicated param guard or route middleware) to avoid deep nesting of error handling.
186-220: Encapsulate heading and back-navigation in a dedicated component.
Lines 213–227 build a header and provide a back button. Reusable UI patterns like this often benefit from being encapsulated in a dedicated layout, making your Index component simpler.
248-306: Consider lazy-loading the map to improve performance.
You are rendering a large map component with turf geometry, complex layers, and interactions. Consider code-splitting or lazy-loading the map logic so that loading is deferred, especially on slower networks.
322-331: Double-check error messages for user-friendliness.
Whenb_id_farmis missing, you throw a low-level server error referencing “Farm ID is required”. Consider providing more context or a user-readable message, possibly localized.fdm-app/app/integrations/nmi.ts (1)
61-69: Improve error handling with detailed validation errors.The validation with Zod is a good addition, but consider including more specific error details to help with debugging.
- console.error( - "NMI API response validation failed:", - parsedResponse.error, - ) - throw new Error("Invalid response from NMI API") + console.error( + "NMI API response validation failed:", + JSON.stringify(parsedResponse.error.format(), null, 2), + ) + throw new Error(`Invalid response from NMI API: ${parsedResponse.error.message}`)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
fdm-app/app/integrations/mapbox.ts(1 hunks)fdm-app/app/integrations/nmi.ts(4 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.field.new.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- fdm-app/app/integrations/mapbox.ts
🧰 Additional context used
🧠 Learnings (1)
fdm-app/app/routes/farm.$b_id_farm.$calendar.field.new.tsx (1)
Learnt from: SvenVw
PR: SvenVw/fdm#116
File: fdm-app/app/routes/farm.$b_id_farm.$calendar.field.new.tsx:111-154
Timestamp: 2025-04-04T14:27:39.518Z
Learning: In the FDM application, cultivation retrieval logic should be centralized in utility functions rather than duplicated across loader and action functions to improve maintainability and ensure consistent behavior.
🧬 Code Definitions (1)
fdm-app/app/routes/farm.$b_id_farm.$calendar.field.new.tsx (5)
fdm-app/app/integrations/mapbox.ts (2)
getMapboxToken(3-10)getMapboxStyle(12-14)fdm-app/app/components/custom/atlas/atlas-styles.tsx (1)
getFieldsStyle(7-47)fdm-app/app/components/custom/field/form.tsx (1)
FieldDetailsDialog(37-147)fdm-app/app/integrations/nmi.ts (2)
getNmiApiKey(6-13)getSoilParameterEstimates(15-75)fdm-app/app/components/custom/field/schema.tsx (1)
FormSchema(22-22)
🔇 Additional comments (5)
fdm-app/app/routes/farm.$b_id_farm.$calendar.field.new.tsx (2)
408-430: Handle partial or invalid soil parameter estimates gracefully.
WhennmiApiKeyis present, you immediately callgetSoilParameterEstimatesand add a soil analysis entry if the call succeeds. Consider logging or skipping partial data if certain fields are missing, especially ifnmichanges their API responses.
432-441: Confirm the redirect is correct for the new field workflow.
This redirect uses/fertilizerafter adding a new field. Ensure this final destination is where your users expect to go immediately after creating a field, and that the newly created field’s data is loaded properly in the next screen.fdm-app/app/integrations/nmi.ts (3)
2-3: Appropriate imports added.Adding the Geometry and Polygon types from geojson and importing Zod for validation are good additions that support the implementation changes below.
16-16: Good flexibility improvement.Broadening the parameter type from
FeaturetoFeature | Polygonallows for greater flexibility in how this function can be used.
31-37: Well-implemented type handling logic.The logic correctly extracts the geometry regardless of whether the input is a Feature or Polygon, making the function more versatile.
| b_id_source: z.string({ | ||
| required_error: "ID van bron is verplicht", | ||
| }).optional(), | ||
| b_geometry: z.string({ |
There was a problem hiding this comment.
what does "optional" implies here since the input is mandatory?
There was a problem hiding this comment.
Because the user form does not include the geometry, so it can't be validated client side. However, the geometry is added to the formData at submission, thus it is server side validated
| export async function getSoilParameterEstimates( | ||
| field: Feature, | ||
| field: Feature | Polygon, | ||
| nmiApiKey: string | undefined, |
There was a problem hiding this comment.
so only polygon or is this any type of geometry?
There was a problem hiding this comment.
It allows Feature, which is a geometry + properties. In theory every geometry for which the centroid can calculated is possible
| geometry = field.geometry | ||
| } else { | ||
| geometry = field | ||
| } |
There was a problem hiding this comment.
you are certain that field is a spatial object, when the case occurs that geometry is not in field?
There was a problem hiding this comment.
Yes, because in that case it is a Polygon. See the type definition of Field
gerardhros
left a comment
gerardhros
left a comment
There was a problem hiding this comment.
Well done. a few minor remarks for explanation.
2 participants
Summary by CodeRabbit
New Features
Improvements
Chores