Add fdm-calculator module for nutrient dosing and UI updates #75
Conversation
🦋 Changeset detectedLatest commit: 2b32694 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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 |
WalkthroughThis PR introduces new nutrient management functionality by adding two functions— Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GDF as getDoseForField
participant GPA as getFertilizerApplications
participant GF as getField
participant GFs as getFertilizers
participant CD as calculateDose
Client->>GDF: call({ fdm, b_id })
GDF->>GPA: Fetch fertilizer applications
GDF->>GF: Retrieve field info (farm ID)
GDF->>GFs: Get fertilizers for the farm
GDF->>CD: Compute dose (applications, fertilizers)
CD-->>GDF: Return Dose object
GDF-->>Client: Return calculated Dose
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
coderabbitai
bot
changed the title
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Sven Verweij <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Sven Verweij <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Sven Verweij <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Sven Verweij <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
.github/workflows/tests.yml (5)
1-3: Add pull_request trigger to workflow.Consider adding pull_request events to ensure tests run when PRs are opened or updated.
name: Tests -on: push +on: + push: + pull_request:
75-75: Fix typo in comment.There's a typo in the comment.
- # the default usernam + # the default username
159-159: Fix typo in comment.There's a typo in the comment.
- # the default usernam + # the default username
89-171: Consider using reusable workflows to reduce duplication.The calculator job configuration is identical to the core job except for the name and coverage file path. Consider extracting the common configuration into a reusable workflow to improve maintainability.
Example structure:
# .github/workflows/reusable-test.yml name: Reusable Test Job on: workflow_call: inputs: component: required: true type: string jobs: test: name: ${{ inputs.component }} # ... rest of the common configuration ... steps: # ... common steps ... - name: Upload results to Codecov uses: codecov/codecov-action@v5 with: files: ./fdm-${{ inputs.component }}/coverage/coverage-final.json flags: fdm-${{ inputs.component }} token: ${{ secrets.CODECOV_TOKEN }}Then use it like this:
# .github/workflows/tests.yml jobs: test-core: uses: ./.github/workflows/reusable-test.yml with: component: core test-calculator: uses: ./.github/workflows/reusable-test.yml with: component: calculator🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 131-131: trailing spaces
(trailing-spaces)
47-47: Fix YAML formatting issues.
- Remove trailing spaces after
with:on lines 47, 88, and 131- Remove extra blank lines at the end of the file
Also applies to: 88-88, 131-131, 172-173
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 47-47: trailing spaces
(trailing-spaces)
fdm-calculator/src/doses/calculate-dose.ts (2)
7-13: Fix inconsistency in return value documentation.The JSDoc return value description mentions
p_dose_pbut the actual implementation returnsp_dose_p2o5.- * @returns An object representing the total dose of N, P, and K applied, with properties `p_dose_n`, `p_dose_p`, and `p_dose_k`. + * @returns An object representing the total dose of N, P, and K applied, with properties `p_dose_n`, `p_dose_p2o5`, and `p_dose_k2o`.
14-34: Consider enhancing input validation for edge cases.While the negative value checks are good, consider adding validation for NaN and undefined values to make the function more robust.
export function calculateDose({ applications, fertilizers, }: { applications: getFertilizerApplicationType[] fertilizers: getFertilizerType[] }): Dose { // Validate non-negative values + if (!applications?.length || !fertilizers?.length) { + throw new Error("Applications and fertilizers arrays must not be empty") + } if (applications.some((app) => app.p_app_amount < 0)) { throw new Error("Application amounts must be non-negative") } if ( fertilizers.some( (fert) => (fert.p_n_rt && fert.p_n_rt < 0) || (fert.p_p_rt && fert.p_p_rt < 0) || (fert.p_k_rt && fert.p_k_rt < 0), ) ) { throw new Error("Nutrient rates must be non-negative") } + if (applications.some((app) => isNaN(app.p_app_amount))) { + throw new Error("Application amounts must be valid numbers") + }fdm-app/app/components/custom/fertilizer-applications/form.tsx (1)
96-107: Add unit specification to amount input.The placeholder suggests the unit is "ton/ha", but it should be explicitly specified in the UI:
<Input {...field} type="number" value={ field.value === 0 ? "" : field.value } - placeholder="12 ton/ha" + placeholder="12" + endAdornment={<span className="text-muted-foreground">ton/ha</span>} aria-required="true" required />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/tests.yml(1 hunks)fdm-app/app/components/custom/fertilizer-applications/form.tsx(1 hunks)fdm-app/app/components/custom/fertilizer-applications/list.tsx(1 hunks)fdm-calculator/src/doses/calculate-dose.test.ts(1 hunks)fdm-calculator/src/doses/calculate-dose.ts(1 hunks)fdm-calculator/src/doses/get-dose-field.test.ts(1 hunks)fdm-calculator/src/doses/get-dose-field.ts(1 hunks)fdm-calculator/src/setup-test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- fdm-calculator/src/doses/get-dose-field.ts
- fdm-app/app/components/custom/fertilizer-applications/list.tsx
- fdm-calculator/src/doses/calculate-dose.test.ts
- fdm-calculator/src/doses/get-dose-field.test.ts
- fdm-calculator/src/setup-test.ts
🧰 Additional context used
🧠 Learnings (1)
fdm-calculator/src/doses/calculate-dose.ts (1)
Learnt from: SvenVw
PR: SvenVw/fdm#75
File: fdm-app/app/routes/farm.$b_id_farm.field.$b_id.fertilizer.tsx:68-71
Timestamp: 2025-02-14T09:56:37.606Z
Learning: The `calculateDose` function in `@svenvw/fdm-calculator` is a synchronous function that includes built-in validation for negative application amounts and nutrient rates.
🪛 YAMLlint (1.35.1)
.github/workflows/tests.yml
[error] 47-47: trailing spaces
(trailing-spaces)
[error] 88-88: trailing spaces
(trailing-spaces)
[error] 131-131: trailing spaces
(trailing-spaces)
[warning] 172-172: too many blank lines
(1 > 0) (empty-lines)
🔇 Additional comments (7)
.github/workflows/tests.yml (1)
6-88: Well-structured test job configuration!The core test job is well configured with:
- Fixed versions for stability
- Proper caching strategy
- PostgreSQL service with health checks
- Coverage reporting
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 47-47: trailing spaces
(trailing-spaces)
[error] 88-88: trailing spaces
(trailing-spaces)
fdm-calculator/src/doses/calculate-dose.ts (3)
1-5: LGTM! Well-organized imports with proper type definitions.The imports are cleanly structured with appropriate type definitions from both local and external modules.
36-60: LGTM! Robust dose calculation with proper error handling.The calculation logic is well-implemented with:
- Proper handling of missing fertilizers
- Safe access to nutrient rates using nullish coalescing
- Clear comments explaining each step
62-74: LGTM! Clean and efficient dose aggregation.The reduction logic effectively combines individual doses with proper initialization of the accumulator.
fdm-app/app/components/custom/fertilizer-applications/form.tsx (3)
1-29: LGTM! Well-organized imports.The imports are properly organized, with UI components, form handling libraries, and type definitions clearly separated.
30-38: LGTM! Props are well-typed.The component props are properly typed, and the previous feedback about replacing the
anytype has been addressed.
39-53: Verify date validation for the default value.While the form initialization is good, consider adding validation for the default date value to ensure it's within an acceptable range.
defaultValues: { p_app_amount: undefined, - p_app_date: new Date(), + p_app_date: new Date(Math.max(new Date().setHours(0, 0, 0, 0), new Date("1900-01-01").getTime())), },
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
fdm-calculator/src/setup-test.ts (2)
7-9: 🛠️ Refactor suggestionReplace global variables with test context.
Using global variables for test state management can lead to test interference. Consider using test context as per the established pattern.
-let fdm: FdmServerType - -export let migrationsRun = false +interface TestContext { + fdm: FdmServerType; + migrationsRun: boolean; +}
34-48: 🛠️ Refactor suggestionImprove error handling and add cleanup.
The error handling could be more specific and include cleanup logic.
try { fdm = createFdmServer(host, port, user, password, database) if (!migrationsRun) { + try { await migrateFdmServer( fdm, "node_modules/@svenvw/fdm-core/db/migrations", ) migrationsRun = true + } catch (error) { + throw new Error(`Failed to run migrations: ${error.message}`) + } } } catch (error) { - throw new Error( - `Failed to connect/migrate to database: ${error.message}`, - ) + throw new Error(`Failed to connect to database: ${error.message}`) +} finally { + if (fdm) { + try { + await fdm.close() + } catch (error) { + console.error(`Failed to close database connection: ${error.message}`) + } + } }
🧹 Nitpick comments (1)
fdm-calculator/src/setup-test.ts (1)
38-41: Make migration path configurable.The migration path is hard-coded, which could be problematic if the package structure changes or if custom migrations are needed.
+const MIGRATION_PATH = process.env.MIGRATION_PATH || "node_modules/@svenvw/fdm-core/db/migrations" + await migrateFdmServer( fdm, - "node_modules/@svenvw/fdm-core/db/migrations", + MIGRATION_PATH, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fdm-calculator/src/setup-test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
fdm-calculator/src/setup-test.ts (1)
Learnt from: SvenVw
PR: SvenVw/fdm#65
File: fdm-core/src/global-setup.ts:7-9
Timestamp: 2025-01-29T14:25:45.841Z
Learning: In Vitest tests, prefer using test context over global variables for managing test state to avoid test interference and ensure proper cleanup. The test context can be configured by extending the TestContext interface and using beforeAll/afterAll hooks.
🔇 Additional comments (1)
fdm-calculator/src/setup-test.ts (1)
57-65: LGTM! Well-structured type declarations.The ProvidedContext interface extension is properly scoped and follows TypeScript best practices.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
fdm-calculator/src/setup-test.ts (2)
25-32: Remove redundant String conversions.The
String()conversions are unnecessary asprocess.envvalues are already strings.- const host = String(process.env.POSTGRES_HOST) + const host = process.env.POSTGRES_HOST - const user = String(process.env.POSTGRES_USER) + const user = process.env.POSTGRES_USER - const password = String(process.env.POSTGRES_PASSWORD) + const password = process.env.POSTGRES_PASSWORD - const database = String(process.env.POSTGRES_DB) + const database = process.env.POSTGRES_DB
34-48: Improve error handling granularity and avoid hardcoded paths.The error handling could be more specific, and the migration path should be configurable.
try { fdm = createFdmServer(host, port, user, password, database) + } catch (error) { + throw new Error(`Failed to connect to database: ${error.message}`) + } + try { if (!migrationsRun) { + const migrationPath = process.env.MIGRATION_PATH || "node_modules/@svenvw/fdm-core/dist/db/migrations" await migrateFdmServer( fdm, - "node_modules/@svenvw/fdm-core/dist/db/migrations", + migrationPath, ) migrationsRun = true } } catch (error) { throw new Error( - `Failed to connect/migrate to database: ${error.message}`, + `Failed to run migrations: ${error.message}`, ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fdm-calculator/src/setup-test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
fdm-calculator/src/setup-test.ts (1)
Learnt from: SvenVw
PR: SvenVw/fdm#65
File: fdm-core/src/global-setup.ts:7-9
Timestamp: 2025-01-29T14:25:45.841Z
Learning: In Vitest tests, prefer using test context over global variables for managing test state to avoid test interference and ensure proper cleanup. The test context can be configured by extending the TestContext interface and using beforeAll/afterAll hooks.
🔇 Additional comments (3)
fdm-calculator/src/setup-test.ts (3)
7-9: Replace global variables with test context.Using global variables for test state management can lead to test interference. Consider using test context as per the established pattern.
50-54: Consider security implications of exposing credentials in test context.Storing sensitive information like database passwords in the test context could pose a security risk if tests or error messages expose this information.
Consider:
- Using environment variables directly in tests that need credentials
- Creating a secure credential manager for tests
- Using mock credentials for tests
57-65: LGTM!The type declarations for the ProvidedContext interface are well-structured and properly typed.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #75 +/- ##
==========================================
+ Coverage 94.19% 94.39% +0.20%
==========================================
Files 27 33 +6
Lines 3563 3853 +290
Branches 259 288 +29
==========================================
+ Hits 3356 3637 +281
- Misses 207 216 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
fdm-calculator/src/setup-test.ts (1)
7-9: 🛠️ Refactor suggestionReplace global variables with test context.
Using global variables for test state management can lead to test interference. Consider using test context as per the established pattern.
-let fdm: FdmServerType - -export let migrationsRun = true +interface TestContext { + fdm: FdmServerType; + migrationsRun: boolean; +}
🧹 Nitpick comments (7)
fdm-app/app/components/custom/fertilizer-applications/list.tsx (3)
26-26: Remove commented-out code.Remove the commented-out code to maintain cleanliness.
- {/* <div className="text-sm font-medium">Meststoffen</div> */}
37-37: Remove commented-out code.Remove the commented-out code to maintain cleanliness.
- {/* <p className="text-sm text-muted-foreground">[email protected]</p> */}
50-69: Add accessibility improvements to the delete button.The delete button should have an aria-label for better accessibility.
<Button variant="destructive" disabled={fetcher.state === "submitting"} + aria-label={`Delete ${application.p_name_nl}`} onClick={() => { if (application.p_app_ids) { handleDelete(application.p_app_ids) } else { handleDelete([application.p_app_id]) } }} >fdm-calculator/src/doses/get-dose-field.test.ts (1)
30-120: Consider adding more test cases for edge scenarios.The test case for a single application is well-structured and thorough. However, consider adding test cases for:
- Multiple fertilizer applications
- Invalid fertilizer IDs
- Negative application amounts
fdm-app/app/components/custom/fertilizer-applications/form.tsx (1)
162-165: Consider updating the date validation range.The current date validation only prevents dates before 1970. Consider:
- Adding an upper limit to prevent future dates
- Using a more recent lower limit based on business requirements
- disabled={(date) => - date < - new Date("1970-01-01") - } + disabled={(date) => + date > new Date() || + date < new Date("2000-01-01") + }fdm-app/app/components/custom/atlas/atlas-panels.tsx (1)
154-170: LGTM! Error handling is properly implemented.The error handling in
submitSelectedFieldsfollows best practices by:
- Logging the error for debugging.
- Throwing a data object with appropriate status and message.
- Including a TODO for adding toast notifications.
Would you like me to help implement the toast notification functionality mentioned in the TODO comment?
fdm-app/app/components/custom/fertilizer-applications.tsx (1)
257-257: Use self-closing tag for empty div.The empty div should be self-closing for better code style.
- <div className="space-y-4"></div> + <div className="space-y-4" />🧰 Tools
🪛 Biome (1.9.4)
[error] 257-257: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
fdm-app/app/components/custom/atlas/atlas-functions.tsx(1 hunks)fdm-app/app/components/custom/atlas/atlas-panels.tsx(2 hunks)fdm-app/app/components/custom/atlas/atlas.d.tsx(2 hunks)fdm-app/app/components/custom/farm/farm-content.tsx(1 hunks)fdm-app/app/components/custom/fertilizer-applications.tsx(2 hunks)fdm-app/app/components/custom/fertilizer-applications/cards.tsx(1 hunks)fdm-app/app/components/custom/fertilizer-applications/form.tsx(1 hunks)fdm-app/app/components/custom/fertilizer-applications/formschema.tsx(1 hunks)fdm-app/app/components/custom/fertilizer-applications/list.tsx(1 hunks)fdm-app/app/routes/farm.$b_id_farm.field.$b_id.fertilizer.tsx(3 hunks)fdm-app/app/routes/farm.$b_id_farm.field.$b_id.norm.tsx(1 hunks)fdm-app/app/routes/farm.create.$b_id_farm.atlas.tsx(1 hunks)fdm-app/app/routes/farm.create.$b_id_farm.cultivations.$b_lu_catalogue.fertilizers.tsx(3 hunks)fdm-calculator/src/doses/calculate-dose.test.ts(1 hunks)fdm-calculator/src/doses/calculate-dose.ts(1 hunks)fdm-calculator/src/doses/get-dose-field.test.ts(1 hunks)fdm-calculator/src/doses/get-dose-field.ts(1 hunks)fdm-calculator/src/setup-test.ts(1 hunks)fdm-core/src/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- fdm-app/app/routes/farm.create.$b_id_farm.atlas.tsx
- fdm-app/app/components/custom/atlas/atlas-functions.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- fdm-app/app/routes/farm.$b_id_farm.field.$b_id.norm.tsx
- fdm-core/src/index.ts
- fdm-app/app/components/custom/farm/farm-content.tsx
- fdm-app/app/components/custom/fertilizer-applications/formschema.tsx
- fdm-calculator/src/doses/get-dose-field.ts
- fdm-calculator/src/doses/calculate-dose.test.ts
🧰 Additional context used
🧠 Learnings (4)
fdm-calculator/src/doses/calculate-dose.ts (1)
Learnt from: SvenVw
PR: SvenVw/fdm#75
File: fdm-app/app/routes/farm.$b_id_farm.field.$b_id.fertilizer.tsx:68-71
Timestamp: 2025-02-14T09:56:37.606Z
Learning: The `calculateDose` function in `@svenvw/fdm-calculator` is a synchronous function that includes built-in validation for negative application amounts and nutrient rates.
fdm-app/app/components/custom/atlas/atlas-panels.tsx (1)
Learnt from: SvenVw
PR: SvenVw/fdm#67
File: fdm-app/app/components/custom/atlas/atlas-panels.tsx:28-28
Timestamp: 2025-01-31T15:41:43.741Z
Learning: When handling different map event types in react-map-gl v7.1.8, use MapLayerMouseEvent for mouse events (which have the point property) and ViewStateChangeEvent for view state changes. Use a type guard like 'point' in evt to safely access event-specific properties.
fdm-calculator/src/setup-test.ts (2)
Learnt from: SvenVw
PR: SvenVw/fdm#65
File: fdm-core/src/global-setup.ts:7-9
Timestamp: 2025-01-29T14:25:45.841Z
Learning: In Vitest tests, prefer using test context over global variables for managing test state to avoid test interference and ensure proper cleanup. The test context can be configured by extending the TestContext interface and using beforeAll/afterAll hooks.
Learnt from: SvenVw
PR: SvenVw/fdm#65
File: fdm-core/src/global-setup.ts:7-9
Timestamp: 2025-01-29T14:33:41.822Z
Learning: In the fdm project, tests should be organized per file to maintain separation of concerns, rather than consolidating them into a centralized location.
fdm-app/app/routes/farm.create.$b_id_farm.cultivations.$b_lu_catalogue.fertilizers.tsx (2)
Learnt from: SvenVw
PR: SvenVw/fdm#75
File: fdm-app/app/routes/farm.$b_id_farm.field.$b_id.fertilizer.tsx:68-71
Timestamp: 2025-02-14T09:56:37.606Z
Learning: The `calculateDose` function in `@svenvw/fdm-calculator` is a synchronous function that includes built-in validation for negative application amounts and nutrient rates.
Learnt from: SvenVw
PR: SvenVw/fdm#16
File: fdm-app/app/routes/app.addfarm.$b_id_farm.cultivations.$b_lu_catalogue.fertilizers.tsx:1-1
Timestamp: 2024-12-16T10:56:07.561Z
Learning: The project uses `react-router` v7, and the `data` function is exported and used for error handling in loaders and actions.
🪛 Biome (1.9.4)
fdm-app/app/components/custom/fertilizer-applications.tsx
[error] 257-257: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
🔇 Additional comments (18)
fdm-app/app/components/custom/atlas/atlas.d.tsx (1)
16-18: LGTM! String literal types are properly formatted.The changes to use double quotes for string literal types align with TypeScript best practices.
Also applies to: 25-28
fdm-calculator/src/doses/calculate-dose.ts (1)
7-75: LGTM! Well-implemented nutrient dose calculation.The implementation includes:
- Comprehensive input validation
- Clear error messages
- Proper null coalescing for missing rates
- Good error handling for missing fertilizers
fdm-calculator/src/doses/get-dose-field.test.ts (3)
1-12: LGTM!The imports and type declarations are well-organized and include all necessary dependencies.
14-29: LGTM!The test setup with
beforeEachhook ensures a clean environment for each test by creating a fresh FDM server instance.
122-146: LGTM!The test case for no applications properly verifies the default behavior with zero doses.
fdm-app/app/components/custom/fertilizer-applications/cards.tsx (4)
1-12: LGTM!The imports are well-organized and include all necessary UI components and types.
13-74: LGTM!The
FertilizerApplicationsCardcomponent is well-implemented with:
- Clear header and content sections
- Proper tooltips for limit and advice values
- Conditional rendering based on value presence
76-94: LGTM!The
FertilizerApplicationsCardscomponent efficiently maps and renders individual cards in a responsive grid layout.
96-126: Make limit and advice values configurable.The
constructCardsfunction contains hardcoded limit and advice values. Consider making these configurable through environment variables or a configuration file.fdm-app/app/routes/farm.$b_id_farm.field.$b_id.fertilizer.tsx (4)
1-26: LGTM!The imports are well-organized and include all necessary components and functions.
27-80: LGTM!The loader function:
- Properly validates required parameters
- Handles error cases appropriately
- Efficiently calculates doses using the synchronous
calculateDosefunction
82-117: LGTM!The component implementation:
- Uses a responsive grid layout
- Properly integrates form and list components
- Efficiently displays calculated doses
119-175: LGTM!The action function:
- Handles both POST and DELETE methods
- Includes proper error handling
- Returns appropriate success/error responses
fdm-app/app/components/custom/fertilizer-applications/form.tsx (2)
1-30: LGTM!The imports are well-organized and include all necessary form components and utilities.
31-58: LGTM!The form setup:
- Uses proper type definitions for props
- Implements form reset on successful submission
- Includes appropriate default values
fdm-app/app/routes/farm.create.$b_id_farm.cultivations.$b_lu_catalogue.fertilizers.tsx (2)
108-111: LGTM! Dose calculation is correctly integrated.Based on the retrieved learnings,
calculateDoseis a synchronous function with built-in validation for negative application amounts and nutrient rates, making it safe to use here.
132-150: LGTM! UI layout is well-structured.The new grid layout with separate sections for form, list, and cards improves the organization and readability of the interface.
fdm-app/app/components/custom/fertilizer-applications.tsx (1)
58-194: LGTM! Form structure is well-organized.The form implementation follows best practices:
- Uses
RemixFormProviderfor form state management.- Implements proper field validation.
- Includes loading states and error handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
fdm-app/app/routes/farm.$b_id_farm.field._index.tsx (1)
140-142: Remove commented-out code.Remove the commented-out code as it's not providing any value and could become outdated.
- {/* <p className="px-8 text-center text-sm text-muted-foreground"> - De meeste gebruikers lukt het binnen 6 minuten. - </p> */}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fdm-app/app/routes/farm.$b_id_farm.field.$b_id.tsx(1 hunks)fdm-app/app/routes/farm.$b_id_farm.field._index.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
fdm-app/app/routes/farm.$b_id_farm.field._index.tsx (7)
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.
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.
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).
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: When using `updateField` from fdm-core, all 8 parameters must be provided in order: fdm, b_id, b_name, b_geometry, b_area, b_id_source, b_id_farm, and b_id_farm_source.
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.
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 exactly 8 arguments and does not support an object-based parameter approach.
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.
fdm-app/app/routes/farm.$b_id_farm.field.$b_id.tsx (3)
Learnt from: SvenVw
PR: SvenVw/fdm#45
File: fdm-app/app/routes/farm.$b_id_farm._index.tsx:1-1
Timestamp: 2025-01-14T16:06:24.294Z
Learning: In the fdm-app codebase, the `redirect` function should be imported from `react-router`, not `react-router-dom`.
Learnt from: SvenVw
PR: SvenVw/fdm#45
File: fdm-app/app/routes/farm.$b_id_farm.settings._index.tsx:1-1
Timestamp: 2025-01-14T16:06:21.832Z
Learning: In the fdm project, `redirect` and other routing utilities should be imported from `react-router` instead of `react-router-dom`.
Learnt from: SvenVw
PR: SvenVw/fdm#16
File: fdm-app/app/routes/app.addfarm.$b_id_farm.cultivations.$b_lu_catalogue.fertilizers.tsx:1-1
Timestamp: 2024-12-16T10:56:07.561Z
Learning: The project uses `react-router` v7, and the `data` function is exported and used for error handling in loaders and actions.
🔇 Additional comments (4)
fdm-app/app/routes/farm.$b_id_farm.field._index.tsx (3)
1-28: Add missing imports for nutrient dose calculation.The AI summary indicates that this file should include nutrient dose calculation functionality using
calculateDosefrom@svenvw/fdm-calculator, but the import is missing.Let's verify if the
calculateDosefunction exists in the package:#!/bin/bash # Description: Check if the calculateDose function exists in the fdm-calculator package # Search for the calculateDose function definition ast-grep --pattern 'export function calculateDose'Likely an incorrect or invalid review comment.
42-44: LGTM! Good addition of parameter validation.The new validation for
b_id_farmparameter is well-implemented with a clear error message and appropriate status code.
100-222: LGTM! Well-structured component with good UX.The component implementation is well-structured with:
- Clear user feedback for empty states
- Accessible UI elements
- Consistent error handling
- Good use of UI components
fdm-app/app/routes/farm.$b_id_farm.field.$b_id.tsx (1)
37-37:❓ Verification inconclusive
Verify the redirect path behavior.
The change from a relative path (
"./farm") to a direct path ("farm") might affect navigation behavior. Please ensure that this path correctly redirects users to the intended farm overview page.
🏁 Script executed:
#!/bin/bash # Description: Search for similar redirect patterns in the codebase to verify consistency. # Search for redirect patterns in route files rg -A 2 'return redirect\(' 'fdm-app/app/routes/' # Search for farm route definitions to verify the target path ast-grep --pattern $'export default function Farm' 'fdm-app/app/routes/'Length of output: 1925
Action Required: Verify Redirect Behavior for Consistency
In the file
fdm-app/app/routes/farm.$b_id_farm.field.$b_id.tsxat line 37, the redirect is now set as:return redirect("farm")While similar route files typically use a relative path (e.g.,
"./farm"), this change to a bare string may affect how the redirection resolves to the intended farm overview page. Please double-check that this direct path behaves as expected in your routing context. If it was intended to match the relative path behavior seen in other routes, consider updating it toreturn redirect("./farm")for consistency.
| function constructCards(dose: Dose) { | ||
| // Construct the fertilizer application cards | ||
| const cards: FertilizerApplicationsCardProps[] = [ | ||
| { |
There was a problem hiding this comment.
for MINAS we might expand this also with the calculated N surplus as well the critical threshold that should not be exceeded (or N surplus remaining). Probably in aother branch to be added
There was a problem hiding this comment.
Yes, could be done in similiar way or by expanding the cards
Yes, the cards are not related to fields as they just request title, unit, value, advice and limit. It it therefore also available for farm level. However, it would require a function to retrieve all fertilizers for a farm (and thus not for only a field) calculate doses and correct for different field areas |
Summary by CodeRabbit
New Features
Refactor
Tests