feat(application-system): Make required a dynamic field #16691
Conversation
WalkthroughThe changes in this pull request introduce a new function, Changes
Suggested labels
Suggested reviewers
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
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16691 +/- ##
=======================================
Coverage 36.63% 36.64%
=======================================
Files 6873 6871 -2
Lines 143132 143051 -81
Branches 40805 40779 -26
=======================================
- Hits 52436 52418 -18
+ Misses 90696 90633 -63 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 20 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 22 Total Test Services: 0 Failed, 21 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (11)
|
![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
🧹 Outside diff range and nitpick comments (3)
libs/application/ui-fields/src/lib/AsyncSelectFormField/AsyncSelectFormField.tsx (1)
68-68: Consider adding type annotation for the application parameterThe implementation correctly uses
buildFieldRequiredto make the required state dynamic. However, to improve type safety and maintainability, consider adding a type annotation for the application parameter.- required={buildFieldRequired(application, required)} + required={buildFieldRequired(application as Application, required)}Don't forget to import the Application type:
import { Application } from '@island.is/application/types'libs/application/core/src/lib/fieldBuilders.ts (1)
540-548: Add JSDoc documentation for consistency.The function implementation looks good, but it's missing documentation unlike other functions in the file. Consider adding JSDoc to explain the parameters and return value.
Add documentation like this:
+/** + * Determines if a field is required based on the application context. + * + * @param {Application} application - The current application state + * @param {MaybeWithApplication<boolean>} [maybeRequired] - Static boolean or function that determines if field is required + * @returns {boolean | undefined} - Whether the field is required + */ export const buildFieldRequired = ( application: Application, maybeRequired?: MaybeWithApplication<boolean>, ) => {libs/application/types/src/lib/Fields.ts (1)
200-201: Address the TODO for non-schema validationThe TODO comment suggests implementing a validation function for non-schema validation. Addressing this would enhance validation capabilities beyond the schema.
Would you like assistance in implementing this validation function or opening a GitHub issue to track this task?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
libs/application/core/src/lib/fieldBuilders.ts(2 hunks)libs/application/types/src/lib/Fields.ts(8 hunks)libs/application/ui-fields/src/lib/AsyncSelectFormField/AsyncSelectFormField.tsx(2 hunks)libs/application/ui-fields/src/lib/CompanySearchFormField/CompanySearchFormField.tsx(2 hunks)libs/application/ui-fields/src/lib/DateFormField/DateFormField.tsx(2 hunks)libs/application/ui-fields/src/lib/NationalIdWithNameFormField/NationalIdWithNameFormField.tsx(2 hunks)libs/application/ui-fields/src/lib/PhoneFormField/PhoneFormField.tsx(2 hunks)libs/application/ui-fields/src/lib/SelectFormField/SelectFormField.tsx(2 hunks)libs/application/ui-fields/src/lib/TextFormField/TextFormField.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
libs/application/core/src/lib/fieldBuilders.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/types/src/lib/Fields.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/ui-fields/src/lib/AsyncSelectFormField/AsyncSelectFormField.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/ui-fields/src/lib/CompanySearchFormField/CompanySearchFormField.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/ui-fields/src/lib/DateFormField/DateFormField.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/ui-fields/src/lib/NationalIdWithNameFormField/NationalIdWithNameFormField.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/ui-fields/src/lib/PhoneFormField/PhoneFormField.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/ui-fields/src/lib/SelectFormField/SelectFormField.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/ui-fields/src/lib/TextFormField/TextFormField.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (26)
libs/application/ui-fields/src/lib/NationalIdWithNameFormField/NationalIdWithNameFormField.tsx (2)
1-1: LGTM! Clean import addition.
The import is well-structured and follows good practices for tree-shaking optimization.
21-21: Verify the buildFieldRequired implementation.
The change aligns with making the required field dynamic and maintains type safety. However, let's verify the implementation of buildFieldRequired to ensure it handles all edge cases.
✅ Verification successful
Let me gather more information about the types and usage patterns to ensure complete verification.
buildFieldRequired implementation is correct and consistently used
The implementation in libs/application/core/src/lib/fieldBuilders.ts properly handles both static boolean values and dynamic functions through the MaybeWithApplication<boolean> type. The function is consistently used across multiple form field components (Phone, Text, Select, NationalId, Date, Company, AsyncSelect) with the same pattern of passing the application and required field property.
The type definition in libs/application/types/src/lib/Fields.ts confirms proper type safety:
type MaybeWithApplication<T> = T | ((application: Application) => T)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify buildFieldRequired implementation and its usage across the codebase
# Test 1: Check buildFieldRequired implementation
echo "Checking buildFieldRequired implementation..."
ast-grep --pattern 'export const buildFieldRequired = $_'
# Test 2: Verify consistent usage across other form fields
echo "Verifying consistent usage across form fields..."
rg "buildFieldRequired\(application, .*\.required\)" --type ts
Length of output: 1183
Script:
#!/bin/bash
# Check the type definition of MaybeWithApplication
echo "Checking MaybeWithApplication type definition..."
rg "type MaybeWithApplication" -A 3 --type ts
# Check other usages of buildFieldRequired to ensure consistent patterns
echo "Checking all buildFieldRequired usages..."
rg "buildFieldRequired\(" -B 1 -A 1 --type ts
# Look for any tests related to buildFieldRequired
echo "Checking for related tests..."
rg "buildFieldRequired" --type test
Length of output: 3422
libs/application/ui-fields/src/lib/CompanySearchFormField/CompanySearchFormField.tsx (1)
6-10: LGTM! Clean import organization.
The imports are well-organized and follow good practices:
- Grouped with related core utilities
- Named imports support tree-shaking
- Shared from core package enabling reuse across apps
libs/application/ui-fields/src/lib/PhoneFormField/PhoneFormField.tsx (2)
3-3: LGTM! Clean import addition.
The import is well-organized and tree-shakeable, bundling related utilities from the core module.
78-78: LGTM! Dynamic required validation implemented correctly.
The change enhances field validation by making it context-aware while maintaining type safety and component reusability.
Let's verify that this pattern is consistently applied across other form fields:
✅ Verification successful
Dynamic required validation is consistently implemented across form fields
The verification confirms that buildFieldRequired(application, required) is consistently used across all form field components in the codebase:
- AsyncSelectFormField
- CompanySearchFormField
- DateFormField
- PhoneFormField
- SelectFormField
- TextFormField
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of buildFieldRequired across form fields
# Expected: All form field components should use buildFieldRequired similarly
# Search for buildFieldRequired usage in form field components
rg -A 2 "buildFieldRequired\(application, required\)" "libs/application/ui-fields/src/lib/"
Length of output: 2273
libs/application/ui-fields/src/lib/SelectFormField/SelectFormField.tsx (2)
7-7: LGTM: Import follows best practices
The import is properly organized with related utility functions and maintains tree-shakeability through named imports.
57-57: 🛠️ Refactor suggestion
Verify default value logic with dynamic required state
While the required prop now uses buildFieldRequired for dynamic validation, the default value logic still uses the static required boolean. This might lead to inconsistent behavior.
Let's check if other form fields have updated their default value logic:
Consider updating the default value logic to use buildFieldRequired:
required={buildFieldRequired(application, required)}
defaultValue={
(getValueViaPath(application.answers, id) ??
getDefaultValue(field, application)) ||
- (required ? '' : undefined)
+ (buildFieldRequired(application, required) ? '' : undefined)
}Also applies to: 60-64
libs/application/ui-fields/src/lib/TextFormField/TextFormField.tsx (2)
4-4: LGTM! Import structure maintains tree-shaking efficiency
The import is properly structured to allow effective tree-shaking, and the placement in the application/core package is appropriate for shared functionality.
93-93: Verify form validation behavior with dynamic required state
The change to use buildFieldRequired enhances flexibility but warrants verification of form validation behavior.
Let's verify other form field components for consistency:
Consider adding a comment explaining the dynamic required behavior for future maintainers:
backgroundColor={backgroundColor}
rows={rows}
+ // Required state is determined dynamically based on application context
required={buildFieldRequired(application, required)}
rightAlign={rightAlign}✅ Verification successful
Form validation behavior with dynamic required state is correctly implemented
The verification shows that:
buildFieldRequiredis consistently used across all form field components in the ui-fields library- The function properly handles both static boolean and dynamic function-based required states
- The implementation is type-safe and follows the same pattern in all form components
The suggested documentation comment is not necessary as this is a standard pattern used consistently throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other form field components to verify consistent usage
rg -l "buildFieldRequired" "libs/application/ui-fields/src"
Length of output: 592
Script:
#!/bin/bash
# Check the implementation of buildFieldRequired function and its usage across components
rg "buildFieldRequired" "libs/application/ui-fields/src" -A 2 -B 2
Length of output: 7944
Script:
#!/bin/bash
# Check the implementation of buildFieldRequired function in the core package
rg "buildFieldRequired" "libs/application/core/src" -A 5 -B 2
Length of output: 641
libs/application/ui-fields/src/lib/AsyncSelectFormField/AsyncSelectFormField.tsx (2)
3-3: LGTM: Import is correctly structured for tree-shaking
The import is properly organized, combining related functions from the same module.
Line range hint 1-99: Component meets reusability requirements
The component is well-structured for reuse across NextJS apps:
- Uses shared UI components and form fields
- Properly typed with TypeScript interfaces
- Clear separation of concerns
- No direct NextJS dependencies
libs/application/ui-fields/src/lib/DateFormField/DateFormField.tsx (2)
3-7: LGTM! Import changes follow best practices
The addition of buildFieldRequired follows proper tree-shaking practices by using named imports and is appropriately grouped with related core utilities.
131-131: Verify consistent implementation across form fields
The change to use buildFieldRequired aligns with making the required state dynamic. However, let's verify this pattern is consistently implemented across other form fields.
✅ Verification successful
Implementation of buildFieldRequired is consistent across form fields
The verification shows that buildFieldRequired is consistently implemented across relevant form field components that require validation:
- AsyncSelectFormField
- CompanySearchFormField
- DateFormField
- NationalIdWithNameFormField
- PhoneFormField
- SelectFormField
- TextFormField
All these components follow the same pattern of using buildFieldRequired(application, required) for their required prop, maintaining consistency with the dynamic required state implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent implementation of buildFieldRequired across form fields
# Expected: All form field components should use buildFieldRequired for required prop
# Search for DatePickerController required prop usage
echo "Checking DatePickerController required prop implementations:"
ast-grep --pattern 'DatePickerController {
$$$
required={$_}
$$$
}'
# Search for other potential form field components that might need this change
echo -e "\nChecking other form components for required prop implementations:"
fd -e tsx -e ts . libs/application/ui-fields/src/lib/ --exec rg -l 'required.*=.*{(?!buildFieldRequired).*}'
Length of output: 5632
Script:
#!/bin/bash
# Let's try a different approach to find form field components and their required prop usage
# First, find all form field components
echo "Form field components in the library:"
fd -e tsx -e ts . libs/application/ui-fields/src/lib/
# Then search for required prop patterns in these files
echo -e "\nChecking required prop implementations:"
rg "required\s*=\s*{" libs/application/ui-fields/src/lib/ -A 1
# Also search for buildFieldRequired usage
echo -e "\nChecking buildFieldRequired usage:"
rg "buildFieldRequired" libs/application/ui-fields/src/lib/
Length of output: 5428
libs/application/core/src/lib/fieldBuilders.ts (1)
47-47: LGTM! Import follows established patterns.
The addition of MaybeWithApplication type import is well-placed with related type imports.
libs/application/types/src/lib/Fields.ts (12)
26-27: Introduction of MaybeWithApplication types enhances field flexibility
Adding MaybeWithApplication<T> and MaybeWithApplicationAndField<T> types allows field properties to be defined as static values or functions dependent on the application context, increasing the dynamic capability of field definitions.
205-207: Refactoring fields to extend InputField promotes consistency
Introducing InputField with the required property and having other fields extend it standardizes the handling of required fields across the application, improving code maintainability.
280-289: CheckboxField now extends InputField
By extending InputField, CheckboxField now uniformly handles the required property, enhancing consistency among input fields.
Line range hint 291-304: DateField now extends InputField
The change allows DateField to utilize the inherited required property, promoting consistent behavior.
315-325: RadioField now extends InputField
Extending InputField ensures RadioField consistently incorporates the required property.
327-334: SelectField now extends InputField
This update standardizes the handling of the required property within SelectField.
337-344: CompanySearchField now extends InputField
By inheriting from InputField, CompanySearchField includes the required property, enhancing consistency.
Line range hint 346-356: AsyncSelectField now extends InputField
This change ensures that AsyncSelectField consistently handles the required property.
Line range hint 358-376: TextField now extends InputField
Extending InputField allows TextField to uniformly utilize the required property.
Line range hint 378-387: PhoneField now extends InputField
The change standardizes the required property handling in PhoneField.
Line range hint 560-571: NationalIdWithNameField now extends InputField
By extending InputField, NationalIdWithNameField inherits the required property, promoting consistency.
Line range hint 636-648: FindVehicleField now extends InputField
This update incorporates the required property into FindVehicleField by extending InputField.
libs/application/ui-fields/src/lib/CompanySearchFormField/CompanySearchFormField.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
libs/application/core/src/lib/fieldBuilders.spec.ts (2)
1-6: Consider consolidating imports from the same module.The three separate imports from '@island.is/application/types' can be combined into a single import statement for better maintainability.
-import { Field } from '@island.is/application/types' -import { FieldComponents, FieldTypes } from '@island.is/application/types' -import { Application } from '@island.is/application/types' +import { Application, Field, FieldComponents, FieldTypes } from '@island.is/application/types'
8-46: Consider adding edge case tests for buildFieldOptions.While the current tests cover the main scenarios well, consider adding tests for:
- Undefined/null options
- Empty array options
- Invalid option shapes
Example additional test:
it('should handle undefined options gracefully', () => { const result = buildFieldOptions(undefined, mockApplication, mockField) expect(result).toEqual([]) // or whatever the expected behavior is })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
libs/application/core/src/lib/fieldBuilders.spec.ts(1 hunks)libs/application/core/src/lib/fieldBuilders.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/application/core/src/lib/fieldBuilders.ts
🧰 Additional context used
📓 Path-based instructions (1)
libs/application/core/src/lib/fieldBuilders.spec.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (2)
libs/application/core/src/lib/fieldBuilders.spec.ts (2)
48-72: Well-structured and comprehensive tests for buildFieldRequired!
The test suite effectively covers:
- Static boolean values
- Undefined input
- Dynamic function-based required field validation
- Proper validation of function calls with correct arguments
The implementation follows TypeScript best practices and provides good test coverage.
1-72: Overall excellent test implementation!
The test file demonstrates:
- Proper TypeScript usage with clear type definitions
- Comprehensive test coverage for utility functions
- Good separation of concerns and test organization
- Reusable test utilities and mock objects
This implementation aligns well with the coding guidelines for library code.
norda-gunni
added
automerge
...
Attach a link to issue if relevant
What
Specify what you're trying to achieve
Why
Specify why you need to achieve this
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes