Add calendar/timeframe, update URLs & state; remove React context#106
Conversation
🦋 Changeset detectedLatest commit: 78ae975 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 |
WalkthroughThis set of changes introduces a new “Kalender” option into the application’s sidebar and integrates dynamic calendar management throughout the front-end and back-end. URL paths, loader functions, and data fetching routines are updated to handle additional parameters—in particular a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Sidebar
participant CalendarStore
participant RouteLoader
participant fdmCore
User->>Sidebar: Select a calendar year ("Kalender" option)
Sidebar->>CalendarStore: Update calendar state
Sidebar->>RouteLoader: Build dynamic URLs with calendar and timeframe
RouteLoader->>fdmCore: Request filtered data (cultivations, fields, etc.)
fdmCore-->>RouteLoader: Return results based on timeframe filtering
RouteLoader-->>User: Render updated view with calendar context
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code Definitions (1)fdm-core/src/cultivation.test.ts (4)
🔇 Additional comments (14)
✨ Finishing Touches
🪧 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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development #106 +/- ##
===============================================
+ Coverage 92.06% 92.63% +0.56%
===============================================
Files 43 44 +1
Lines 4475 4696 +221
Branches 440 502 +62
===============================================
+ Hits 4120 4350 +230
+ Misses 354 345 -9
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: 0
♻️ Duplicate comments (1)
fdm-core/src/cultivation.ts (1)
774-789: 🛠️ Refactor suggestionImplementation of the isCultivationWithinTimeframe function needs refinement.
The function immediately returns false if
!b_lu_start || !timeframe.start || !timeframe.end(line 779), which means it will always return false if eithertimeframe.startortimeframe.endis missing, even if filtering could still be done with the available date. This doesn't match how timeframe filtering is handled elsewhere in the code.Consider refining the implementation to handle cases where only one of the timeframe boundaries is defined:
export function isCultivationWithinTimeframe( b_lu_start: Date | null, b_lu_end: Date | null, timeframe: Timeframe, ): boolean { - if (!b_lu_start || !timeframe.start || !timeframe.end) return false + if (!b_lu_start) return false + // Handle cases where only start or end date is provided + if (!timeframe.start && !timeframe.end) return true if (b_lu_end) { + if (timeframe.start && timeframe.end) { return ( (b_lu_start >= timeframe.start && b_lu_start <= timeframe.end) || (b_lu_end >= timeframe.start && b_lu_end <= timeframe.end) || (b_lu_start <= timeframe.start && b_lu_end >= timeframe.end) ) + } else if (timeframe.start) { + return b_lu_end >= timeframe.start + } else if (timeframe.end) { + return b_lu_start <= timeframe.end + } } - return b_lu_start >= timeframe.start && b_lu_start <= timeframe.end + if (timeframe.start && timeframe.end) { + return b_lu_start >= timeframe.start && b_lu_start <= timeframe.end + } else if (timeframe.start) { + return b_lu_start >= timeframe.start + } else if (timeframe.end) { + return b_lu_start <= timeframe.end + } + return true }
🧹 Nitpick comments (2)
.changeset/eager-nails-draw.md (1)
5-5: Grammar Suggestion: Add a Determiner for Clarity.
The changelog message reads "AddKalenderto sidebar." Consider adding the article "the" before "sidebar" for better clarity, e.g., "AddKalenderto the sidebar."-Add `Kalender` to sidebar. This enables users to filter assets and actions based on the selected year or to show all of them. +Add `Kalender` to the sidebar. This enables users to filter assets and actions based on the selected year or to show all of them.🧰 Tools
🪛 LanguageTool
[uncategorized] ~5-~5: You might be missing the article “the” here.
Context: .../fdm-app": minor --- AddKalenderto sidebar. This enables users to filter assets an...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
fdm-core/src/cultivation.test.ts (1)
29-31: Added imports for database operations and timeframe type.The imports for database operations from
drizzle-orm, the schema types, and theTimeframetype have been correctly added. However, the schema import could use theimport typesyntax since it's only used for types.Consider using
import typesyntax for the schema import since it's only used for types:- import * as schema from "./db/schema" + import type * as schema from "./db/schema"🧰 Tools
🪛 Biome (1.9.4)
[error] 31-31: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.(lint/style/useImportType)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.changeset/eager-nails-draw.md(1 hunks).changeset/salty-nights-buy.md(1 hunks)fdm-app/app/routes/farm.create.$b_id_farm.$calendar.cultivations.$b_lu_catalogue.crop.harvest.$b_id_harvesting.tsx(3 hunks)fdm-core/src/cultivation.test.ts(6 hunks)fdm-core/src/cultivation.ts(13 hunks)fdm-core/src/fertilizer.ts(4 hunks)fdm-core/src/field.test.ts(2 hunks)fdm-core/src/harvest.test.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .changeset/salty-nights-buy.md
- fdm-app/app/routes/farm.create.$b_id_farm.$calendar.cultivations.$b_lu_catalogue.crop.harvest.$b_id_harvesting.tsx
- fdm-core/src/harvest.test.ts
🧰 Additional context used
🧠 Learnings (1)
fdm-core/src/cultivation.test.ts (1)
Learnt from: SvenVw
PR: SvenVw/fdm#87
File: fdm-core/src/farm.ts:22-25
Timestamp: 2025-03-12T14:05:51.527Z
Learning: In the FDM codebase, functions that insert/create data (like `addFarm`) intentionally use `principal_id: string` instead of `PrincipalId` to enforce that only a single principal ID can be used for creation operations, while read operations use the more flexible `PrincipalId` type which supports both single IDs and arrays of IDs.
🧬 Code Definitions (2)
fdm-core/src/cultivation.ts (1)
fdm-core/src/timeframe.d.ts (1) (1)
Timeframe(1:4)
fdm-core/src/fertilizer.ts (1)
fdm-core/src/db/schema.ts (1) (1)
fertilizerApplicationTypeSelect(173:174)
🪛 Biome (1.9.4)
fdm-core/src/cultivation.test.ts
[error] 31-31: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.
(lint/style/useImportType)
🪛 LanguageTool
.changeset/eager-nails-draw.md
[uncategorized] ~5-~5: You might be missing the article “the” here.
Context: .../fdm-app": minor --- Add Kalender to sidebar. This enables users to filter assets an...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🪛 GitHub Check: codecov/patch
fdm-core/src/cultivation.ts
[warning] 696-697: fdm-core/src/cultivation.ts#L696-L697
Added lines #L696 - L697 were not covered by tests
fdm-core/src/fertilizer.ts
[warning] 721-721: fdm-core/src/fertilizer.ts#L721
Added line #L721 was not covered by tests
[warning] 727-727: fdm-core/src/fertilizer.ts#L727
Added line #L727 was not covered by tests
🔇 Additional comments (23)
.changeset/eager-nails-draw.md (1)
1-3: Changeset Header and Version Bump Looks Good.
The header separators and version bump for"@svenvw/fdm-app": minorare correctly formatted.fdm-core/src/fertilizer.ts (4)
1-1: Import of necessary types and functions for timeframe filtering.The imports have been updated to include
gteandltefromdrizzle-ormfor date comparisons and theTimeframetype from "./timeframe". These are appropriate additions for implementing date range filtering.Also applies to: 13-13
667-668: Updated documentation for the new timeframe parameter.The JSDoc comment has been appropriately updated to document the new optional timeframe parameter.
675-675: Added optional timeframe parameter to function signature.The function signature has been updated to include an optional
timeframeparameter of typeTimeframe, which allows filtering fertilizer applications by date range.
712-730: Implemented conditional timeframe filtering for fertilizer applications.The query has been enhanced with date range filtering logic that applies when a timeframe is provided. The implementation correctly handles:
- Filtering by
b_idwhen no timeframe is provided- Adding date range conditions using
gteandltewhen timeframe start/end dates are available- Properly handling cases where only one of start or end date is provided
The static analysis indicates that lines 721 and 727 (the undefined checks) aren't covered by tests.
Consider adding test cases that specifically verify behavior when only start or only end date is provided in the timeframe to improve test coverage.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 721-721: fdm-core/src/fertilizer.ts#L721
Added line #L721 was not covered by tests
[warning] 727-727: fdm-core/src/fertilizer.ts#L727
Added line #L727 was not covered by testsfdm-core/src/field.test.ts (5)
8-8: Added import for FdmType.The import for
FdmTypeis necessary for the type annotation in the new test cases.
82-109: Added comprehensive tests for the getFields function with proper setup.The test setup includes creating a test environment with a dedicated FDM instance and farm, which is a good practice for isolated test cases.
110-171: Thoroughly validates field retrieval by farm ID.This test case properly validates that multiple fields can be retrieved for a farm, ensuring the basic functionality works correctly before testing more complex scenarios.
173-180: Verifies proper permission handling.This test correctly verifies that an error is thrown when an invalid principal ID is used, which is essential for security validation.
182-363: Comprehensive testing of timeframe-based field retrieval.This test case is particularly thorough, covering multiple scenarios:
- Creating fields with different start and end dates
- Testing timeframes that include specific fields
- Testing with only start date specified
- Testing with only end date specified
The expected results are clearly defined and the assertions validate both the number of fields returned and their identity, which is excellent.
fdm-core/src/cultivation.test.ts (4)
5-7: Added imports for new helper functions.The additional imports for the new helper functions (
buildDateRangeCondition,buildDateRangeConditionEnding, andisCultivationWithinTimeframe) are correctly added.Also applies to: 11-11
264-316: Added tests for retrieving cultivations within a timeframe.This test case thoroughly validates that cultivations can be retrieved within a specified timeframe. It creates multiple cultivations with different start and end dates and tests various timeframe ranges to ensure proper filtering.
744-1046: Added comprehensive tests for cultivation plan with timeframe filtering.These tests thoroughly cover:
- Error handling for non-existent farms
- Multiple cultivations and fields retrieval
- Cases where no fertilizer applications are present
- Various timeframe filtering scenarios including when timeframes include or exclude specific cultivations
The tests provide excellent coverage for the new timeframe filtering functionality in the cultivation plan feature.
1085-1318: Added unit tests for helper functions.The tests for the helper functions (
buildDateRangeCondition,buildDateRangeConditionEnding, andisCultivationWithinTimeframe) are comprehensive, covering:
- Various combinations of null/undefined dates
- Different date range scenarios
- Edge cases with dates at the boundaries of timeframes
- Cases with cultivations that span, fall within, or are outside timeframes
These tests ensure the helper functions work correctly under all expected scenarios.
fdm-core/src/cultivation.ts (9)
1-12: Added necessary imports for timeframe functionality.The imports have been updated to include additional SQL operators for date comparison (
gte,lte) and theTimeframetype. These are necessary for implementing the timeframe filtering functionality.Also applies to: 25-25
382-383: Updated getCultivations function to support timeframe filtering.The function signature and documentation have been appropriately updated to include an optional
timeframeparameter, allowing for date-based filtering of cultivations.Also applies to: 394-394
406-415: Implemented date range condition building for cultivation filtering.The code creates appropriate date range conditions based on the provided timeframe, handling both starting dates and ending dates through dedicated helper functions.
445-447: Applied timeframe conditions to the database query.The where clause has been correctly modified to combine the field ID condition with timeframe conditions, ensuring cultivations are filtered by both field and date range when a timeframe is provided.
470-470: Updated getCultivationPlan function to support timeframe filtering.Similar to the getCultivations function, the getCultivationPlan function has been updated with an optional timeframe parameter and appropriate documentation.
Also applies to: 539-539
554-562: Added date range condition building for cultivation plan filtering.The implementation mirrors the approach used in getCultivations, reusing the same helper functions for consistency.
672-672: Applied timeframe conditions to the cultivation plan query.The timeframe clause has been correctly added to the database query's where condition.
688-698: Added additional timeframe check for individual cultivations.The code includes a secondary check using
isCultivationWithinTimeframeto verify each cultivation is within the specified timeframe. However, there's a code coverage issue reported for lines 696-697, which handle the case when a cultivation is outside the timeframe.Consider adding a test case that specifically checks that cultivations outside the timeframe are excluded from the results to improve code coverage for this conditional branch.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 696-697: fdm-core/src/cultivation.ts#L696-L697
Added lines #L696 - L697 were not covered by tests
1046-1104: Helper functions for building date range conditions.The implementation of
buildDateRangeConditionandbuildDateRangeConditionEndinglooks solid. They correctly handle different combinations of start and end dates, returning appropriate SQL conditions or undefined when no dates are provided.
gerardhros
left a comment
gerardhros
left a comment
There was a problem hiding this comment.
nice. Good to add some definitions related to state-year-caldender-timeframe in supporting documentation later.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
fdm-core/src/soil.test.ts (2)
327-335: Consider using forEach instead of map for test validations.The current code uses
map()for validation operations but doesn't use the returned array. Since you're only iterating to perform assertions,forEach()would be more semantically appropriate.-analyses1.map((analysis) => { +analyses1.forEach((analysis) => {Apply the same change to all similar instances in this test.
Also applies to: 348-356, 369-377, 390-395, 408-413
415-416: Add validation for the expected number of analyses in all test cases.You're validating the count for the no-timeframe case, but not for the other test cases. Consider adding count validations for each timeframe scenario to verify not just that the returned records are within the date range, but also that the correct number of records is being returned.
// Example for the first timeframe const analyses1 = await getSoilAnalyses(fdm, principal_id, b_id, timeframe1) +expect(analyses1).toHaveLength(1) // April should only include one analysis analyses1.forEach((analysis) => { // existing validations })fdm-app/app/components/custom/sidebar-app.tsx (1)
212-303: Well-implemented calendar selection UI.The calendar selection UI is well-implemented with:
- Proper conditional rendering
- Clear visual indication of the selected calendar
- Smooth collapsible interaction
- URL path maintenance during navigation
One suggestion for improvement:
Consider extracting the regex pattern
/\/(\d{4}|all)/into a constant or utility function for better maintainability, especially if it's used elsewhere in the application:+ // At the top of the file or in a separate utils file + const CALENDAR_PATH_REGEX = /\/(\d{4}|all)\// // Then in the component const newUrl = location.pathname.replace( - /\/(\d{4}|all)/, + CALENDAR_PATH_REGEX, `/${item}` )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
fdm-app/app/components/custom/sidebar-app.tsx(6 hunks)fdm-app/app/routes/farm.create.$b_id_farm.$calendar.atlas.tsx(4 hunks)fdm-app/app/routes/farm.tsx(4 hunks)fdm-app/package.json(2 hunks)fdm-core/src/index.ts(1 hunks)fdm-core/src/soil.test.ts(1 hunks)fdm-core/src/soil.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- fdm-app/package.json
- fdm-core/src/index.ts
- fdm-app/app/routes/farm.create.$b_id_farm.$calendar.atlas.tsx
🧰 Additional context used
🪛 GitHub Actions: Tests
fdm-core/src/soil.test.ts
[error] 231-231: BaseError: Exception for getSoilAnalyses. Caused by: ReferenceError: sql is not defined
[error] 273-273: BaseError: Exception for getSoilAnalyses. Caused by: ReferenceError: sql is not defined
[error] 321-321: BaseError: Exception for getCurrentSoilData. Caused by: ReferenceError: sql is not defined
[error] 460-460: BaseError: Exception for getCurrentSoilData. Caused by: ReferenceError: sql is not defined
🪛 Biome (1.9.4)
fdm-core/src/soil.ts
[error] 15-15: Shouldn't redeclare 'getSoilAnalysisType'. Consider to delete it or rename it.
'getSoilAnalysisType' is defined here:
(lint/suspicious/noRedeclare)
🔇 Additional comments (13)
fdm-core/src/soil.ts (3)
241-242: Implementation of timeframe parameter looks good.The function signature has been appropriately updated to include the optional timeframe parameter with proper JSDoc documentation.
Also applies to: 246-251
262-281: Well-structured filtering logic for timeframe parameter.The implementation of the timeframe filtering logic is clean and handles all possible combinations of start and end dates correctly:
- Both start and end dates provided
- Only start date provided
- Only end date provided
- No timeframe provided
This approach allows for flexible date-range filtering of soil analyses.
303-303: Query correctly uses dynamically constructed whereClause.The SQL query has been properly modified to use the dynamically constructed whereClause instead of the hardcoded field equality condition.
fdm-core/src/soil.test.ts (1)
277-417: Comprehensive test coverage for timeframe functionality.The new test case thoroughly covers all key scenarios for the timeframe parameter:
- Filtering for a specific month (April 2023)
- Date range spanning multiple months (April-May 2023)
- Complete date range covering all test data (March-June 2023)
- Open-ended ranges (from May 2023 onwards)
- Ranges with only end date (up to April 2023)
- Default behavior with no timeframe specified
For each scenario, the test correctly verifies that returned analyses have sampling dates within the specified timeframe.
🧰 Tools
🪛 GitHub Actions: Tests
[error] 321-321: BaseError: Exception for getCurrentSoilData. Caused by: ReferenceError: sql is not defined
fdm-app/app/routes/farm.tsx (5)
17-18: Good implementation of state management with Zustand stores.The addition of these imports aligns with the PR objective to remove React context in favor of Zustand stores for state management. This is a good architectural choice for better maintainability and scalability.
72-72: Good refactoring from local state to centralized store.Moving from local state to Zustand store for farm ID management is a positive architectural change that improves state consistency across components.
76-76: Correctly added dependency to useEffect.The addition of
setFarmIdto the dependency array follows React's best practices for hooks.
93-101: Solid implementation of calendar state extraction and management.This implementation effectively extracts calendar data from route parameters and synchronizes it with the Zustand store. The code pattern is consistent with the farm ID implementation, making the codebase more predictable.
114-124: Clean component structure after removing context provider.The component structure is clean and properly nested. The removal of the FarmContext provider successfully completes the migration to Zustand stores.
fdm-app/app/components/custom/sidebar-app.tsx (4)
89-89: Effective use of store selectors.Using the selector pattern from Zustand to extract only needed state is an efficient approach that prevents unnecessary re-renders.
91-94: Well-structured calendar state management setup.The calendar state implementation is clear and follows best practices by:
- Using store selectors for global state
- Using local state only for UI concerns (collapsible state)
- Separating the calendar options retrieval
This creates a clean separation of concerns.
103-130: Robust conditional URL generation.The conditional URL generation based on application state (create wizard, farm ID, calendar) ensures consistent navigation. The pattern of declaring variables first and then setting them based on conditions improves readability.
244-248:Details
❓ Verification inconclusive
Verify URL replacement logic for edge cases.
The URL replacement logic assumes that the calendar parameter is always present in the URL in a specific format.
🏁 Script executed:
#!/bin/bash # Check for potential edge cases in URL patterns # Look for all routes that might include calendar parameter rg -A 2 "path.*:calendar" --glob "*.{ts,tsx}" # Check how calendar parameters are extracted from URLs rg -A 2 "params\.calendar" --glob "*.{ts,tsx}" # Look for URL construction with calendar parameter rg "\/\$\{.*calendar.*\}\/" --glob "*.{ts,tsx}"Length of output: 613
Attention: Verify Robustness of URL Replacement Logic
Please ensure that the replacement in
fdm-app/app/components/custom/sidebar-app.tsx(lines 244-248) correctly handles all expected and unexpected URL formats. In particular:
- Confirm that the regex
/\/(\d{4}|all)/reliably matches the intended calendar segment. If the pathname lacks a matching segment (e.g., when the calendar parameter is missing or formatted differently), verify that the new URL remains valid.- Verify that only the desired occurrence is replaced, even if similar patterns appear elsewhere in the URL.
- Consider adding conditional logic or tests to gracefully handle cases where the expected calendar segment is absent.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
fdm-core/src/cultivation.test.ts (1)
29-31: Ensure types are imported properly for schema.The
schemaimport should be marked as a type import to optimize the code.-import * as schema from "./db/schema" -import { Timeframe } from "./timeframe" +import type * as schema from "./db/schema" +import type { Timeframe } from "./timeframe"🧰 Tools
🪛 Biome (1.9.4)
[error] 31-31: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.(lint/style/useImportType)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
fdm-core/src/db/migrations/meta/_journal.jsonis excluded by!fdm-core/src/db/migrations/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
fdm-app/app/components/custom/farm/farm-header.tsx(6 hunks)fdm-app/app/components/custom/sidebar-app.tsx(6 hunks)fdm-app/app/routes/farm.create._index.tsx(1 hunks)fdm-app/package.json(2 hunks)fdm-core/src/cultivation.test.ts(6 hunks)fdm-core/src/fertilizer.test.ts(2 hunks)fdm-core/src/fertilizer.ts(4 hunks)fdm-core/src/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- fdm-core/src/index.ts
- fdm-app/package.json
- fdm-app/app/routes/farm.create._index.tsx
- fdm-app/app/components/custom/sidebar-app.tsx
- fdm-app/app/components/custom/farm/farm-header.tsx
🧰 Additional context used
🧬 Code Definitions (1)
fdm-core/src/fertilizer.test.ts (1)
fdm-core/src/fertilizer.ts (2)
addFertilizerApplication(625-684)getFertilizerApplications(846-912)
🪛 Biome (1.9.4)
fdm-core/src/cultivation.test.ts
[error] 31-31: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.
(lint/style/useImportType)
fdm-core/src/fertilizer.test.ts
[error] 34-34: Shouldn't redeclare 'disableFertilizerCatalogue'. Consider to delete it or rename it.
'disableFertilizerCatalogue' is defined here:
(lint/suspicious/noRedeclare)
[error] 35-35: Shouldn't redeclare 'enableFertilizerCatalogue'. Consider to delete it or rename it.
'enableFertilizerCatalogue' is defined here:
(lint/suspicious/noRedeclare)
🪛 GitHub Check: codecov/patch
fdm-core/src/fertilizer.ts
[warning] 896-896: fdm-core/src/fertilizer.ts#L896
Added line #L896 was not covered by tests
[warning] 902-902: fdm-core/src/fertilizer.ts#L902
Added line #L902 was not covered by tests
🔇 Additional comments (15)
fdm-core/src/fertilizer.test.ts (4)
943-990: Well-structured test for timeframe filtering.This test effectively validates the new timeframe filtering functionality for fertilizer applications. The implementation correctly tests the filtering logic for date ranges and verifies that applications are within the specified timeframe.
992-1010: Good edge case handling.This test properly verifies that an empty array is returned when no fertilizer applications exist within the provided timeframe, covering an important edge case.
1012-1027: Appropriate error handling test for non-existing fields.The test correctly verifies permission error handling when attempting to add a fertilizer application to a non-existent field.
1029-1042: Good validation for non-existing fertilizer error handling.This test effectively verifies the error handling when attempting to use a non-existent fertilizer, ensuring the expected error message is thrown.
fdm-core/src/fertilizer.ts (4)
1-1: Clean import addition for date filtering operators.The addition of
gteandlteoperators fromdrizzle-ormis appropriate for implementing the timeframe filtering functionality.
13-13: Appropriate type import for the new timeframe parameter.The import of the
Timeframetype aligns with the function signature update.
850-850: Clear function signature update for timeframe filtering.The addition of the optional
timeframeparameter to the function signature properly enables date-based filtering of fertilizer applications.
887-905:Details
❓ Verification inconclusive
Well-implemented conditional timeframe filtering.
The modified
whereclause elegantly handles the presence or absence of the timeframe parameter and its start/end dates. The code correctly applies date filtering when a timeframe is provided and falls back to the original filtering when it's not.Note: Static analysis indicates lines 896 and 902 may not be covered by tests. Consider adding test cases for edge cases where either start or end date is undefined.
🏁 Script executed:
#!/bin/bash # Check test coverage for the timeframe filtering implementation # Check for test coverage of partial timeframes (only start or only end) echo "Looking for test cases that cover partial timeframes..." rg -A 3 -B 3 "timeframe.*start.*undefined|timeframe.*end.*undefined" ./fdm-core/src/fertilizer.test.tsLength of output: 225
Subject: Verify Test Coverage for Partial Timeframe Cases
The conditional timeframe filtering is well implemented and effectively handles both scenarios of a provided timeframe and its absence. However, our initial search for tests in
fdm-core/src/fertilizer.test.tsdid not reveal explicit cases covering scenarios where only thestartor only theenddate is defined (notably around lines 896 and 902). Please verify manually—or consider adding new tests—to ensure that these edge cases are adequately covered.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 896-896: fdm-core/src/fertilizer.ts#L896
Added line #L896 was not covered by tests
[warning] 902-902: fdm-core/src/fertilizer.ts#L902
Added line #L902 was not covered by testsfdm-core/src/cultivation.test.ts (7)
5-11: Well-organized imports for the new helper functions.The imports for the new date range condition builders and timeframe validation function are appropriately added.
264-316: Comprehensive test for timeframe-filtered cultivations.This test thoroughly verifies the functionality of retrieving cultivations within specific timeframes. The different test cases cover various scenarios with overlapping timeframes and check sorting order.
752-887: Thorough testing of cultivation plan with multiple entities.These tests thoroughly validate the cultivation plan functionality with multiple fields and cultivation types. The tests ensure that fertilizer applications are properly associated with their respective fields and cultivations.
889-1046: Complete testing of timeframe filtering for cultivation plans.The tests comprehensively cover different timeframe scenarios for cultivation plans, including:
- Timeframes that include only some cultivations
- Timeframes that include all cultivations
- Timeframes that exclude all cultivations
This ensures the timeframe filtering is robust across various scenarios.
1085-1120: Well-structured unit tests for buildDateRangeCondition.These tests thoroughly validate the behavior of the
buildDateRangeConditionfunction with different combinations of inputs:
- Both parameters null/undefined
- Only start date provided
- Only end date provided
- Both dates provided
This ensures the function behaves as expected in all scenarios.
1122-1183: Comprehensive tests for buildDateRangeConditionEnding.The tests for
buildDateRangeConditionEndingare thorough and cover all edge cases and combinations of input parameters, validating the complex condition building logic that handles cultivation ending dates.
1185-1318: Extensive testing of isCultivationWithinTimeframe.This test suite is comprehensive, covering a wide range of scenarios for the timeframe validation function, including:
- Cultivations completely within timeframe
- Cultivations spanning timeframe boundaries
- Cultivations outside timeframe
- Cultivations with missing dates
- Cultivations with single-day durations
The thoroughness ensures the function works as expected in all scenarios.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
fdm-core/src/soil.ts (1)
347-356: Added timeframe filtering for getCurrentSoilData.Good implementation of timeframe filtering specifically for the "end" date to filter out future analyses. The comment on line 347 has a small typo ("analyes" instead of "analyses").
- // Filter out "future" analyes + // Filter out "future" analysesfdm-core/src/fertilizer.test.ts (1)
938-986: Great test coverage for timeframe-based retrieval.
This ensures that multiple fertilizer applications in different date ranges are accurately filtered.Consider adding coverage or clarifications around potential timezone differences, ensuring date comparisons remain consistent in various environments.
fdm-core/src/cultivation.test.ts (1)
264-316: Timeframe-based cultivation test.
This test asserts ordering of returned cultivations. If the underlying query changes its default sort order, the test may fail.Consider specifying the query’s order explicitly or allowing flexible order checks to avoid fragile tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.soil._index.tsx(3 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.soil.analysis.$a_id.tsx(1 hunks)fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.soil.analysis.new.tsx(0 hunks)fdm-core/src/cultivation.test.ts(6 hunks)fdm-core/src/fertilizer.test.ts(1 hunks)fdm-core/src/soil.ts(9 hunks)
💤 Files with no reviewable changes (1)
- fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.soil.analysis.new.tsx
✅ Files skipped from review due to trivial changes (1)
- fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.soil.analysis.$a_id.tsx
🧰 Additional context used
🧬 Code Definitions (4)
fdm-core/src/fertilizer.test.ts (1)
fdm-core/src/fertilizer.ts (2)
addFertilizerApplication(625-684)getFertilizerApplications(846-912)
fdm-core/src/cultivation.test.ts (2)
fdm-core/src/cultivation.ts (6)
addCultivation(152-301)getCultivations(390-457)getCultivationPlan(535-772)buildDateRangeCondition(1047-1068)buildDateRangeConditionEnding(1071-1104)isCultivationWithinTimeframe(774-789)fdm-core/src/db/schema.ts (1)
cultivations(259-270)
fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.soil._index.tsx (1)
fdm-core/src/soil.ts (1)
getCurrentSoilData(331-409)
fdm-core/src/soil.ts (1)
fdm-core/src/index.ts (1)
Timeframe(81-81)
🪛 GitHub Check: core (20)
fdm-core/src/cultivation.test.ts
[failure] 1167-1167: src/cultivation.test.ts > buildDateRangeConditionEnding > should return and condition with or conditions when both dateStart and dateEnd are provided
ReferenceError: schema is not defined
❯ src/cultivation.test.ts:1167:25
[failure] 1151-1151: src/cultivation.test.ts > buildDateRangeConditionEnding > should return or condition with lte when only dateEnd is provided
ReferenceError: schema is not defined
❯ src/cultivation.test.ts:1151:21
[failure] 1137-1137: src/cultivation.test.ts > buildDateRangeConditionEnding > should return or condition with gte when only dateStart is provided
ReferenceError: schema is not defined
❯ src/cultivation.test.ts:1137:21
[failure] 1115-1115: src/cultivation.test.ts > buildDateRangeCondition > should return and condition when both dateStart and dateEnd are provided
ReferenceError: schema is not defined
❯ src/cultivation.test.ts:1115:21
[failure] 1105-1105: src/cultivation.test.ts > buildDateRangeCondition > should return lte condition when only dateEnd is provided
ReferenceError: schema is not defined
❯ src/cultivation.test.ts:1105:17
[failure] 1097-1097: src/cultivation.test.ts > buildDateRangeCondition > should return gte condition when only dateStart is provided
ReferenceError: schema is not defined
❯ src/cultivation.test.ts:1097:17
🪛 GitHub Actions: Tests
fdm-core/src/cultivation.test.ts
[error] 97-97: ReferenceError: schema is not defined
[error] 105-105: ReferenceError: schema is not defined
[error] 115-115: ReferenceError: schema is not defined
[error] 137-137: ReferenceError: schema is not defined
[error] 151-151: ReferenceError: schema is not defined
[error] 167-167: ReferenceError: schema is not defined
🔇 Additional comments (26)
fdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.soil._index.tsx (5)
7-7: New import for timeframe functionality.The added import for
getTimeframefrom the calendar library supports the new timeframe filtering feature being implemented across the application.
15-15: Added Timeframe type import.Good addition of the Timeframe type import, which is needed for proper typing with the updated function signatures.
68-70: Added timeframe extraction from URL parameters.The code now extracts timeframe data from URL parameters, enabling filtering of soil data by date ranges. This aligns with the PR objective of adding calendar functionality.
81-86: Passing timeframe to getSoilAnalyses function.The timeframe parameter is now correctly passed to the getSoilAnalyses function, allowing filtering of soil analyses by date range.
89-94: Passing timeframe to getCurrentSoilData function.The timeframe parameter is now correctly passed to the getCurrentSoilData function, ensuring consistency with the timeframe filtering implementation across the application.
fdm-core/src/soil.ts (7)
1-1: Added missing SQL template tag import.The
sqltemplate tag was previously missing but is used on line 377 for custom SQL ordering. This fixes the issue previously reported in a code review.
8-8: Added Timeframe type import.Good addition of the Timeframe type import from the timeframe module, which is necessary for the updated function signatures.
240-240: Updated getSoilAnalyses function signature with timeframe parameter.The function signature and JSDoc have been updated to include an optional timeframe parameter, aligning with the PR objective to add calendar filtering functionality.
Also applies to: 249-249
261-280: Added comprehensive timeframe filtering logic.Good implementation of conditional timeframe filtering. The code handles all possible combinations of start and end dates:
- Both start and end dates (date range)
- Only start date (from a specific date)
- Only end date (until a specific date)
- No timeframe (all dates)
This provides flexible filtering options for the application's calendar feature.
302-302: Updated where clause to use dynamic timeframe filtering.The query now uses the dynamically constructed whereClause based on the timeframe parameters instead of the static field ID filter.
324-324: Updated getCurrentSoilData function signature with timeframe parameter.The function signature and JSDoc have been updated to include an optional timeframe parameter, consistent with the overall approach to implement calendar filtering.
Also applies to: 335-335
374-374: Updated where clause to use dynamic timeframe filtering.The query now uses the dynamically constructed whereClause based on the timeframe parameters, consistent with the implementation in getSoilAnalyses.
fdm-core/src/fertilizer.test.ts (3)
988-1006: Looks good for empty results validation.
This correctly verifies that no applications are returned outside the given timeframe.
1008-1023: Confirm the expected error message for a non-existing field.
The test expects a permission error, butaddFertilizerApplicationmight throw a different error (Field with b_id ... does not exist). Ensure this mismatch is intentional or update accordingly.
1025-1038: Accurate check for non-existing fertilizer.
This test aligns with the function’s behavior of throwing an "Exception for addFertilizerApplication" when the fertilizer does not exist.fdm-core/src/cultivation.test.ts (11)
5-6: No issues with these new imports.
These function imports appear consistent with the tests below.
11-11: Importing 'isCultivationWithinTimeframe' is fine.
The corresponding tests thoroughly validate this function’s logic.
744-750: Accurate permission check for non-existing farm.
This test properly expects a denial when attempting to access a non-existent farm.
752-863: Comprehensive multi-field plan coverage.
This test thoroughly ensures that multiple cultivations and fertilizer applications across multiple fields are retrieved correctly.
865-887: Correctly handles zero fertilizer applications.
Verifies that the plan includes cultivations even when no fertilizers are present.
889-946: Valid timeframe filtration in cultivation plan.
Checks that 'Corn' (outside timeframe) is excluded, while 'Wheat' is included.
945-999: Timeframe includes all cultivations.
Ensures that multiple cultivations are retrieved when the timeframe covers their entire range.
1001-1046: Empty plan test for out-of-range timeframe.
Properly confirms that cultivations are excluded if they fall outside the specified timeframe.
1084-1120: Reference to 'schema' at runtime triggers a pipeline failure.
Same root cause as in the import statements above.🧰 Tools
🪛 GitHub Check: core (20)
[failure] 1115-1115: src/cultivation.test.ts > buildDateRangeCondition > should return and condition when both dateStart and dateEnd are provided
ReferenceError: schema is not defined
❯ src/cultivation.test.ts:1115:21
[failure] 1105-1105: src/cultivation.test.ts > buildDateRangeCondition > should return lte condition when only dateEnd is provided
ReferenceError: schema is not defined
❯ src/cultivation.test.ts:1105:17
[failure] 1097-1097: src/cultivation.test.ts > buildDateRangeCondition > should return gte condition when only dateStart is provided
ReferenceError: schema is not defined
❯ src/cultivation.test.ts:1097:17
1122-1183: 'schema' usage again encountered at runtime.
Same issue with type-only import.🧰 Tools
🪛 GitHub Check: core (20)
[failure] 1167-1167: src/cultivation.test.ts > buildDateRangeConditionEnding > should return and condition with or conditions when both dateStart and dateEnd are provided
ReferenceError: schema is not defined
❯ src/cultivation.test.ts:1167:25
[failure] 1151-1151: src/cultivation.test.ts > buildDateRangeConditionEnding > should return or condition with lte when only dateEnd is provided
ReferenceError: schema is not defined
❯ src/cultivation.test.ts:1151:21
[failure] 1137-1137: src/cultivation.test.ts > buildDateRangeConditionEnding > should return or condition with gte when only dateStart is provided
ReferenceError: schema is not defined
❯ src/cultivation.test.ts:1137:21
1185-1318: Extensive edge-case coverage for 'isCultivationWithinTimeframe'.
These tests thoroughly validate start/end boundary conditions.
2 participants
Summary by CodeRabbit
New Features
Enhancements