feat(driving-license): start on advanced driving-license #16647
Conversation
WalkthroughThe pull request introduces several changes to the driving license application system. Key modifications include the addition of a new Changes
Possibly related PRs
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
|
stjanilofts
force-pushed
the
feat/dl-advanced
branch
from
5c71287
to
518094f
Compare
stjanilofts
force-pushed
the
feat/dl-advanced
branch
from
518094f
to
35fc794
Compare
Datadog ReportAll test runs ✅ 13 Total Test Services: 0 Failed, 12 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (1)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range and nitpick comments (13)
libs/application/templates/driving-license/src/forms/prerequisites/sectionAdvancedLicenseSelection.ts (2)
1-9: Add TypeScript types for imported builder functions
Consider adding explicit TypeScript types for the imported builder functions to improve type safety and documentation.
import {
buildCustomField,
buildMultiField,
buildSubSection,
getValueViaPath,
+ BuildCustomFieldType,
+ BuildMultiFieldType,
+ BuildSubSectionType,
} from '@island.is/application/core'10-35: Consider implementing feature flag check in section condition
The PR mentions a feature flag "isDrivingLicenseAdvancedEnabled", but it's not implemented in the section condition. Consider combining the feature flag with the license type check.
condition: (answers) => {
+ const isFeatureEnabled = getValueViaPath<boolean>(
+ answers,
+ 'isDrivingLicenseAdvancedEnabled',
+ )
const applicationFor = getValueViaPath<LicenseTypes>(
answers,
'applicationFor',
)
- return applicationFor === LicenseTypes.B_ADVANCED
+ return isFeatureEnabled && applicationFor === LicenseTypes.B_ADVANCED
},libs/application/templates/driving-license/src/lib/getApplicationFeatureFlags.ts (1)
8-8: LGTM! Consider adding JSDoc documentation.
The new feature flag follows consistent naming patterns and TypeScript practices. Consider adding JSDoc documentation to describe the flag's purpose and when it should be enabled.
+ /** Controls the visibility of advanced driving license options in the application */
ALLOW_ADVANCED = 'isDrivingLicenseAdvancedEnabled',libs/application/templates/driving-license/src/forms/prerequisites/getForm.ts (1)
Line range hint 1-64: Architecture recommendation: Consider form section organization.
The form structure is growing with multiple conditional sections. Consider extracting the section configuration logic into a separate function or configuration object to improve maintainability and reusability across different NextJS applications.
Consider creating a separate configuration:
// formSections.ts
export const getFormSections = (config: FormConfig) => ({
externalData: [
...(config.allowFakeData ? [sectionFakeData] : []),
sectionExternalData,
// ... other sections
],
// ... other section groups
});libs/application/templates/driving-license/src/lib/utils/formUtils.ts (1)
90-96: Add JSDoc documentation for better maintainability.
The joinWithAnd function implementation is correct and follows TypeScript best practices. Consider adding JSDoc documentation to improve maintainability.
+/**
+ * Joins an array of strings with commas and a conjunction word.
+ * @param arr - Array of strings to join
+ * @param andString - Conjunction word to use (defaults to 'og')
+ * @returns Formatted string where elements are joined with commas and conjunction
+ * @example
+ * joinWithAnd(['a', 'b', 'c']) // returns "a, b, og c"
+ * joinWithAnd(['x', 'y'], 'and') // returns "x and y"
+ */
export const joinWithAnd = (arr: string[], andString = 'og'): string => {libs/application/templates/driving-license/src/forms/prerequisites/sectionApplicationFor.ts (2)
21-21: Add type annotation for allowAdvanced parameter
While the parameter follows the existing pattern, adding a type annotation would improve type safety and documentation.
- allowAdvanced = false,
+ allowAdvanced: boolean = false,Line range hint 1-135: Implementation aligns well with existing patterns
The advanced license feature implementation follows the established patterns in the codebase and maintains reusability across different NextJS apps. The feature is properly controlled via the allowAdvanced parameter, which aligns with the PR objective of using feature flags.
Consider documenting the relationship between this parameter and the feature flag isDrivingLicenseAdvancedEnabled mentioned in the PR description, possibly in a comment or in the function's JSDoc.
libs/application/templates/driving-license/src/fields/AdvancedLicense/AdvancedLicense.tsx (3)
1-16: LGTM! Consider destructuring children prop.
The component declaration and imports are well-structured. The TypeScript types are properly utilized, ensuring type safety.
Since you're using PropsWithChildren, you can destructure the children prop even if unused:
-const AdvancedLicense: FC<React.PropsWithChildren<FieldBaseProps>> = ({
- application,
-}) => {
+const AdvancedLicense: FC<React.PropsWithChildren<FieldBaseProps>> = ({
+ application,
+ children,
+}) => {17-29: LGTM! Consider memoizing the setValue callback.
The state management is well-implemented with proper synchronization between local state and form context.
Consider using useCallback to memoize the setValue effect to prevent unnecessary re-renders:
+const updateFormValue = useCallback(
+ (selectedLicenses: Array<keyof typeof AdvancedLicenseEnum>) => {
+ if (selectedLicenses.length > 0) {
+ setValue('advancedLicense', selectedLicenses)
+ }
+ },
+ [setValue]
+)
useEffect(() => {
- if (selectedLicenses.length > 0) {
- setValue('advancedLicense', selectedLicenses)
- }
+ updateFormValue(selectedLicenses)
}, [selectedLicenses, setValue])1-1: Add JSDoc documentation for component reusability.
To improve maintainability and reusability across different NextJS apps, consider adding comprehensive JSDoc documentation.
Add documentation above the component:
/**
* A reusable component for managing advanced driving license selection.
*
* @example
* ```tsx
* <AdvancedLicense application={application} />
* ```
*
* @remarks
* This component is designed to be reusable across different NextJS applications
* that implement the driving license application flow.
*/libs/application/templates/driving-license/src/lib/constants.ts (3)
15-21: Remove unnecessary quotes around enum keys in LicenseTypes
The keys in the LicenseTypes enum do not require quotes unless they contain invalid identifier characters. Removing the quotes improves code readability.
Apply this diff to remove the unnecessary quotes:
export enum LicenseTypes {
- 'B_FULL' = 'B-full',
- 'B_TEMP' = 'B-temp',
- 'B_FULL_RENEWAL_65' = 'B-full-renewal-65',
- 'BE' = 'BE',
- 'B_ADVANCED' = 'B-advanced',
+ B_FULL = 'B-full',
+ B_TEMP = 'B-temp',
+ B_FULL_RENEWAL_65 = 'B-full-renewal-65',
+ BE = 'BE',
+ B_ADVANCED = 'B-advanced',
}28-37: Simplify MainAdvancedLicense enum by removing unnecessary quotes
The keys in MainAdvancedLicense do not need quotes. Removing them enhances clarity.
Apply this diff:
export enum MainAdvancedLicense {
- 'C1' = 'C1',
- 'D1' = 'D1',
- 'C' = 'C',
- 'D' = 'D',
- 'C1E' = 'C1E',
- 'D1E' = 'D1E',
- 'CE' = 'CE',
- 'DE' = 'DE',
+ C1 = 'C1',
+ D1 = 'D1',
+ C = 'C',
+ D = 'D',
+ C1E = 'C1E',
+ D1E = 'D1E',
+ CE = 'CE',
+ DE = 'DE',
}39-44: Remove unnecessary quotes in ProfessionalAdvancedLicense enum
For consistency and readability, remove quotes around the enum keys.
Apply this diff:
export enum ProfessionalAdvancedLicense {
- 'C1A' = 'C1A',
- 'D1A' = 'D1A',
- 'CA' = 'CA',
- 'DA' = 'DA',
+ C1A = 'C1A',
+ D1A = 'D1A',
+ CA = 'CA',
+ DA = 'DA',
}📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (13)
- libs/application/templates/driving-license-duplicate/src/fields/Alert.tsx (1 hunks)
- libs/application/templates/driving-license/src/fields/AdvancedLicense/AdvancedLicense.tsx (1 hunks)
- libs/application/templates/driving-license/src/fields/AdvancedLicense/index.tsx (1 hunks)
- libs/application/templates/driving-license/src/fields/index.ts (1 hunks)
- libs/application/templates/driving-license/src/forms/prerequisites/getForm.ts (2 hunks)
- libs/application/templates/driving-license/src/forms/prerequisites/sectionAdvancedLicenseSelection.ts (1 hunks)
- libs/application/templates/driving-license/src/forms/prerequisites/sectionApplicationFor.ts (3 hunks)
- libs/application/templates/driving-license/src/lib/constants.ts (1 hunks)
- libs/application/templates/driving-license/src/lib/dataSchema.ts (2 hunks)
- libs/application/templates/driving-license/src/lib/drivingLicenseTemplate.ts (2 hunks)
- libs/application/templates/driving-license/src/lib/getApplicationFeatureFlags.ts (2 hunks)
- libs/application/templates/driving-license/src/lib/messages.ts (1 hunks)
- libs/application/templates/driving-license/src/lib/utils/formUtils.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/application/templates/driving-license/src/fields/AdvancedLicense/index.tsx
🧰 Additional context used
📓 Path-based instructions (12)
libs/application/templates/driving-license-duplicate/src/fields/Alert.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/templates/driving-license/src/fields/AdvancedLicense/AdvancedLicense.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/templates/driving-license/src/fields/index.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/templates/driving-license/src/forms/prerequisites/getForm.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/templates/driving-license/src/forms/prerequisites/sectionAdvancedLicenseSelection.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/templates/driving-license/src/forms/prerequisites/sectionApplicationFor.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/templates/driving-license/src/lib/constants.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/templates/driving-license/src/lib/dataSchema.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/templates/driving-license/src/lib/drivingLicenseTemplate.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/templates/driving-license/src/lib/getApplicationFeatureFlags.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/templates/driving-license/src/lib/messages.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/templates/driving-license/src/lib/utils/formUtils.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."
🪛 Biome
libs/application/templates/driving-license/src/fields/AdvancedLicense/AdvancedLicense.tsx
[error] 50-50: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (16)
libs/application/templates/driving-license/src/fields/index.ts (2)
7-7: LGTM! Clean export implementation.
The named export follows good practices for tree-shaking and modular code organization.
7-7: Verify TypeScript implementation of AdvancedLicense component.
Since this is a library component that will be reused across different NextJS apps, let's verify the TypeScript implementation.
✅ Verification successful
TypeScript implementation of AdvancedLicense component is properly typed and implemented
The component is well-implemented with proper TypeScript types:
- Uses
FC<React.PropsWithChildren<FieldBaseProps>>type for the component - Properly types the state with
Array<keyof typeof AdvancedLicenseEnum> - Has proper type definitions in dataSchema using zod
- Correctly imports and uses TypeScript enums and constants
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check TypeScript implementation of AdvancedLicense component
# Expected: Component should have proper TypeScript type definitions
# Look for the component implementation and its type definitions
ast-grep --pattern 'interface $name {
$$$
}' ./libs/application/templates/driving-license/src/fields/AdvancedLicense
# Check for React component definition with TypeScript
ast-grep --pattern 'const AdvancedLicense: $type = ($props) => {
$$$
}'
Length of output: 196
Script:
#!/bin/bash
# Let's try a different approach to verify the TypeScript implementation
# First, let's check if the file exists and see its content
cat ./libs/application/templates/driving-license/src/fields/AdvancedLicense.ts* 2>/dev/null
# If the file is not found directly, let's search for it
fd "AdvancedLicense" --type f --extension ts --extension tsx
# Let's also try to find any TypeScript type definitions or interfaces related to AdvancedLicense
rg -l "AdvancedLicense" --type ts
Length of output: 1067
Script:
#!/bin/bash
# Let's examine the actual implementation of the AdvancedLicense component
cat libs/application/templates/driving-license/src/fields/AdvancedLicense/AdvancedLicense.tsx
# Also check the index file
cat libs/application/templates/driving-license/src/fields/AdvancedLicense/index.tsx
# Let's also check for any type definitions in the data schema
cat libs/application/templates/driving-license/src/lib/dataSchema.ts
Length of output: 7866
libs/application/templates/driving-license-duplicate/src/fields/Alert.tsx (2)
30-30: LGTM! Removal of debug logging.
Good cleanup by removing the console.log statement, keeping the production code clean.
Line range hint 1-53: Verify the Alert component's usage in the advanced license feature.
The component looks well-structured and follows best practices. Let's verify its integration with the new advanced license feature.
✅ Verification successful
The Alert component is properly integrated and no issues were found
Based on the code analysis:
- The Alert component is correctly imported and used in the driving license duplicate template
- It properly handles type, title and message props with proper formatting
- No advanced license feature specific issues were found with the Alert component
- The component follows the established pattern of using AlertMessage from island-ui/core
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how the Alert component is used in the advanced license feature
# Test 1: Find direct imports of Alert component
echo "=== Direct imports of Alert component ==="
rg -l "import.*Alert.*from.*driving-license-duplicate.*Alert'"
# Test 2: Check usage patterns in advanced license related files
echo "=== Usage in advanced license files ==="
rg -A 3 "<Alert" --glob "**/*AdvancedLicense*"
# Test 3: Look for alert message types used with advanced license
echo "=== Alert message types in advanced license context ==="
rg "type.*AlertMessageType" --glob "**/*AdvancedLicense*"
Length of output: 479
Script:
#!/bin/bash
# Let's try a broader search to find Alert component usage and related patterns
# Search for Alert component imports with a more general pattern
echo "=== Alert component imports ==="
rg "import.*Alert.*from.*driving-license-duplicate"
# Find any Alert component usage in TSX/JSX files
echo "=== Alert component usage in TSX/JSX files ==="
rg -t tsx -t jsx "<Alert" -A 3
# Look for AlertMessage usage in the codebase
echo "=== AlertMessage usage patterns ==="
rg "<AlertMessage" -A 3
# Search for files related to driving license
echo "=== Files related to driving license ==="
fd -e tsx -e ts -e jsx -e js "driving.*license" --type f
Length of output: 165074
Script:
#!/bin/bash
# Let's search for Alert component usage in driving license related files
# Search for Alert component usage in driving license related files
echo "=== Alert component usage in driving license files ==="
rg -A 3 "<Alert" libs/application/templates/driving-license-duplicate/src/fields/Alert.tsx
# Search for imports of Alert component in driving license files
echo "=== Alert component imports in driving license files ==="
rg "import.*Alert.*from" libs/application/templates/driving-license-duplicate/src/fields/Alert.tsx
# Check if Alert component is used in advanced license feature
echo "=== Alert usage in advanced license feature ==="
rg -l "AdvancedLicense.*Alert"
Length of output: 721
libs/application/templates/driving-license/src/lib/getApplicationFeatureFlags.ts (1)
19-19: LGTM! Feature flag correctly integrated.
The new feature flag is properly added to the processing array, maintaining the existing pattern and allowing for proper feature toggling.
libs/application/templates/driving-license/src/forms/prerequisites/getForm.ts (3)
11-11: LGTM: Import follows tree-shaking best practices.
The new import for sectionAdvancedLicenseSelection is properly defined and follows the same pattern as other section imports, enabling effective tree-shaking.
18-18: Verify TypeScript type definitions for the new parameter.
The allowAdvanced parameter is added with a default value, but its type should be explicitly defined in the function parameters for better type safety.
#!/bin/bash
# Check if the function parameters are properly typed
ast-grep --pattern 'export const getForm = ({ $$$: $type $$$})'Consider adding explicit type annotation:
export const getForm = ({
allowFakeData = false,
allowPickLicense = false,
allowBELicense = false,
allow65Renewal = false,
- allowAdvanced = false,
+ allowAdvanced = false as boolean,
})36-42: Verify the impact of allowAdvanced parameter.
The sectionApplicationFor function now receives the allowAdvanced parameter, but we should ensure this change is properly handled in the section implementation.
✅ Verification successful
The allowAdvanced parameter is properly implemented in the section
The implementation correctly handles the allowAdvanced parameter by:
- Adding an advanced license option to the radio field when
allowAdvancedis true - Properly disabling the advanced option if the user doesn't have a valid full B license
- Including appropriate labels and descriptions for the advanced license option
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the implementation of sectionApplicationFor
rg -A 10 "export const sectionApplicationFor.*allowAdvanced"
Length of output: 203
Script:
#!/bin/bash
# Search for sectionApplicationFor function definition
ast-grep --pattern 'export const sectionApplicationFor = $_'
# Also search for any file containing sectionApplicationFor
rg "sectionApplicationFor"
Length of output: 15671
libs/application/templates/driving-license/src/lib/dataSchema.ts (1)
2-11: LGTM! Import changes follow best practices.
The new imports are properly structured using named exports, which is optimal for tree-shaking. The local module organization maintains good modularity.
libs/application/templates/driving-license/src/forms/prerequisites/sectionApplicationFor.ts (2)
10-10: LGTM: Import addition follows existing patterns
The B_ADVANCED constant import is properly grouped with related driving license constants.
117-126: Verify age requirements for advanced license
The disabled condition checks for a valid 'B' license but doesn't include age restrictions like the BE license option does. Should there be age requirements for advanced licenses?
libs/application/templates/driving-license/src/lib/drivingLicenseTemplate.ts (1)
145-146: LGTM! Feature flag integration is clean.
The allowAdvanced feature flag is properly integrated with the existing feature flags system and follows the established pattern.
libs/application/templates/driving-license/src/lib/constants.ts (4)
23-26: Enum Pickup is correctly defined
The Pickup enum accurately represents the available pickup methods.
46-49: Ensure constant AdvancedLicense correctly merges enums
The AdvancedLicense constant correctly combines MainAdvancedLicense and ProfessionalAdvancedLicense.
61-118: Validate minAge values for advanced licenses
Please verify that the minAge values for each license type are accurate according to legal requirements.
If adjustments are needed, apply this diff with the correct ages.
120-130: organizedAdvancedLicenseMap is effectively grouping licenses
The reduction logic correctly organizes advanced licenses by their groups.
...ication/templates/driving-license/src/forms/prerequisites/sectionAdvancedLicenseSelection.ts
Outdated
Show resolved
Hide resolved
...ication/templates/driving-license/src/forms/prerequisites/sectionAdvancedLicenseSelection.ts
Show resolved
Hide resolved
libs/application/templates/driving-license/src/forms/prerequisites/getForm.ts
Outdated
Show resolved
Hide resolved
libs/application/templates/driving-license/src/lib/dataSchema.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
libs/application/templates/driving-license/src/forms/prerequisites/getForm.ts (1)
Line range hint
21-65: Consider splitting form sections into separate configurationThe form structure is growing with multiple sections and conditional logic. To improve maintainability and reusability across different NextJS apps (as per coding guidelines), consider extracting the sections configuration.
Consider creating a separate configuration file:
// formSections.ts export const getFormSections = (config: DrivingLicenseFormConfig) => ({ externalData: { id: 'externalData', title: m.externalDataSection, children: [ ...(config.allowFakeData ? [sectionFakeData] : []), // ... other sections ], }, // ... other sections });This would make the form structure more maintainable and easier to test.
libs/application/templates/driving-license/src/lib/dataSchema.ts (1)
48-51: Add JSDoc documentation for field relationships.While the schema changes look good, consider adding JSDoc documentation to explain the relationship between
applicationForandadvancedLicensefields, as this would improve the reusability of this schema across different applications./** * @field applicationFor - The type of license application * @field advancedLicense - Required when applicationFor is B_ADVANCED, should not be provided otherwise */libs/application/templates/driving-license/src/lib/messages.ts (1)
886-890: Enhance the advanced license description message.The current description is too generic. Consider providing more details about what advanced licenses are, their purpose, and typical use cases.
- defaultMessage: 'Umsókn um aukin ökuréttindi.', + defaultMessage: 'Umsókn um aukin ökuréttindi fyrir atvinnuakstur og stærri ökutæki.',
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
libs/application/templates/driving-license/src/fields/AdvancedLicense/AdvancedLicense.tsx(1 hunks)libs/application/templates/driving-license/src/forms/prerequisites/getForm.ts(2 hunks)libs/application/templates/driving-license/src/forms/prerequisites/sectionAdvancedLicenseSelection.ts(1 hunks)libs/application/templates/driving-license/src/lib/dataSchema.ts(2 hunks)libs/application/templates/driving-license/src/lib/messages.ts(1 hunks)libs/application/templates/driving-license/src/lib/utils/formUtils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/application/templates/driving-license/src/forms/prerequisites/sectionAdvancedLicenseSelection.ts
- libs/application/templates/driving-license/src/lib/utils/formUtils.ts
🧰 Additional context used
📓 Path-based instructions (4)
libs/application/templates/driving-license/src/fields/AdvancedLicense/AdvancedLicense.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/templates/driving-license/src/forms/prerequisites/getForm.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/templates/driving-license/src/lib/dataSchema.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/templates/driving-license/src/lib/messages.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."
📓 Learnings (1)
libs/application/templates/driving-license/src/lib/dataSchema.ts (1)
Learnt from: stjanilofts
PR: island-is/island.is#16647
File: libs/application/templates/driving-license/src/lib/dataSchema.ts:48-48
Timestamp: 2024-10-30T16:19:29.053Z
Learning: The current version of the validation library in `libs/application/templates/driving-license/src/lib/dataSchema.ts` does not support conditional refinements on the `applicationFor` enum to validate the `advancedLicense` field.
🔇 Additional comments (7)
libs/application/templates/driving-license/src/forms/prerequisites/getForm.ts (2)
11-11: LGTM: Clean import statement
The import for the advanced license section follows the established pattern and maintains good code organization.
36-44: LGTM: Proper feature flag implementation
The code correctly implements conditional rendering based on feature flags:
- Advanced license section in
sectionApplicationForis controlled byallowAdvanced sectionAdvancedLicenseSelectionis conditionally included based on the same flag
This maintains consistency with the PR's feature flag approach.
libs/application/templates/driving-license/src/lib/dataSchema.ts (2)
2-11: LGTM: Import changes follow best practices.
The new imports are properly organized and support effective tree-shaking.
49-51: Verify the AdvancedLicense enum values.
Let's ensure that all necessary advanced license types are included in the enum.
✅ Verification successful
Advanced license enum values are properly defined and complete
The verification shows that the AdvancedLicense object correctly combines two comprehensive enums:
MainAdvancedLicense: C1, D1, C, D, C1EProfessionalAdvancedLicense: C1A, D1A, CA, DA
These values represent standard driving license categories, and the schema's usage of Object.values(AdvancedLicense) will correctly include all these values in the validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the AdvancedLicense enum definition and its usage
# Find the AdvancedLicense enum definition
echo "=== AdvancedLicense Definition ==="
rg -A 5 "export (type|enum|const) AdvancedLicense"
# Find all usages of AdvancedLicense values
echo -e "\n=== AdvancedLicense Usage ==="
rg "AdvancedLicense\.[A-Z_]+"
Length of output: 759
Script:
#!/bin/bash
# Let's check the MainAdvancedLicense and ProfessionalAdvancedLicense definitions since AdvancedLicense spreads from them
echo "=== Main and Professional License Definitions ==="
rg -A 5 "export (type|enum|const) (Main|Professional)AdvancedLicense"
# Let's also check where these values are being used in the schema
echo -e "\n=== Schema Usage ==="
rg -B 2 -A 2 "advancedLicense.*array.*enum"
Length of output: 1269
libs/application/templates/driving-license/src/fields/AdvancedLicense/AdvancedLicense.tsx (2)
1-16: LGTM! Well-structured component declaration with proper TypeScript usage.
The component follows best practices with:
- Proper TypeScript types and interfaces
- Clean import structure supporting tree-shaking
- Reusable base props extension
151-151: LGTM! Clean export statement.
The named export supports proper tree-shaking and module composition.
libs/application/templates/driving-license/src/lib/messages.ts (1)
881-995: LGTM! Well-structured message definitions.
The implementation follows good practices for reusability:
- Consistent naming patterns for message IDs
- Well-organized message grouping
- Proper TypeScript exports
libs/application/templates/driving-license/src/forms/prerequisites/getForm.ts
Outdated
Show resolved
Hide resolved
libs/application/templates/driving-license/src/fields/AdvancedLicense/AdvancedLicense.tsx
Outdated
Show resolved
Hide resolved
libs/application/templates/driving-license/src/fields/AdvancedLicense/AdvancedLicense.tsx
Outdated
Show resolved
Hide resolved
libs/application/templates/driving-license/src/fields/AdvancedLicense/AdvancedLicense.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
libs/application/templates/driving-license/src/fields/AdvancedLicense/AdvancedLicense.tsx (1)
34-154: Consider splitting into smaller, focused components.The current component has multiple responsibilities and complex nested structure. To improve maintainability and reusability, consider:
- Extract the license option rendering logic into a separate
LicenseOptioncomponent- Create a
ProfessionalLicenseOptioncomponent for professional license rendering- Move state management logic into a custom hook (e.g.,
useLicenseSelection)This would make the code more modular and easier to test.
libs/application/templates/driving-license/src/lib/messages.ts (1)
886-890: Enhance the description message with more meaningful content.The description message is too generic and could be more informative about what advanced licenses entail.
- defaultMessage: 'Umsókn um aukin ökuréttindi.', + defaultMessage: 'Umsókn um aukin ökuréttindi fyrir atvinnuakstur og stærri ökutæki.',
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
libs/application/templates/driving-license/src/fields/AdvancedLicense/AdvancedLicense.tsx(1 hunks)libs/application/templates/driving-license/src/forms/prerequisites/getForm.ts(2 hunks)libs/application/templates/driving-license/src/lib/dataSchema.ts(2 hunks)libs/application/templates/driving-license/src/lib/messages.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
libs/application/templates/driving-license/src/fields/AdvancedLicense/AdvancedLicense.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/templates/driving-license/src/forms/prerequisites/getForm.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/templates/driving-license/src/lib/dataSchema.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/templates/driving-license/src/lib/messages.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."
📓 Learnings (1)
libs/application/templates/driving-license/src/lib/dataSchema.ts (1)
Learnt from: stjanilofts
PR: island-is/island.is#16647
File: libs/application/templates/driving-license/src/lib/dataSchema.ts:48-48
Timestamp: 2024-10-30T16:19:29.053Z
Learning: The current version of the validation library in `libs/application/templates/driving-license/src/lib/dataSchema.ts` does not support conditional refinements on the `applicationFor` enum to validate the `advancedLicense` field.
🔇 Additional comments (13)
libs/application/templates/driving-license/src/forms/prerequisites/getForm.ts (5)
11-11: LGTM! Import statement follows best practices.
The import statement is correctly structured for effective tree-shaking and follows the established pattern.
13-19: Well-structured TypeScript interface!
The interface improves type safety and documentation as suggested in the past review. All properties are properly typed as optional booleans.
26-27: LGTM! Function signature properly updated.
The new parameter follows the established pattern and is correctly typed through the interface.
44-52: LGTM! Form structure changes are well-implemented.
The changes properly handle the advanced license feature:
- Correct parameter passing to
sectionApplicationFor - Appropriate conditional rendering based on the feature flag
- Logical placement of the new section in the form structure
Line range hint 1-72: Verify compliance with libs directory guidelines.
The implementation follows the required guidelines for libs directory:
- ✓ Reusability: Form is highly configurable through the
DrivingLicenseFormConfiginterface - ✓ TypeScript: Proper type definitions and interfaces are used
- ✓ Tree-shaking: Named imports/exports enable effective bundling
✅ Verification successful
Driving license template follows libs directory guidelines
The implementation is verified to follow the required guidelines for libs directory:
- ✓ Reusability: The template is extensively used across multiple apps (web, api, application-system) and properly exported as a reusable module
- ✓ TypeScript: Strong typing is maintained through proper interfaces and type definitions across the codebase
- ✓ Tree-shaking: Named exports/imports are consistently used, with clear module boundaries and public API exports
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the component's reusability across different apps
echo "Checking for usage of the driving-license template across apps..."
rg -l "DrivingLicenseApplicationPrerequisitesForm" apps/
Length of output: 197
Script:
#!/bin/bash
# Let's try a broader search to verify reusability
echo "Checking for driving license related imports and usage..."
rg -l "driving-license|DrivingLicense" apps/
# Check if the template is exported from libs
echo -e "\nChecking exports from libs..."
rg -l "export.*DrivingLicense" libs/
# Check for any form usage patterns
echo -e "\nChecking form usage patterns..."
ast-grep --pattern 'buildForm({ id: "DrivingLicenseApplicationPrerequisitesForm" })'
# Check if the template is part of public API
echo -e "\nChecking public exports..."
fd -g "index.ts" libs/application/templates/driving-license/
Length of output: 7693
libs/application/templates/driving-license/src/lib/dataSchema.ts (3)
2-11: LGTM: Import changes are well-structured.
The new imports for B_ADVANCED and AdvancedLicense are properly organized and align with the schema updates.
48-48: LGTM: Application type enum updated.
The applicationFor enum has been correctly extended to include the new B_ADVANCED type.
49-54:
Remove redundant validation in advancedLicense schema.
The current implementation has redundant validation:
.nonempty()already ensures the array has at least one element- The
.refine()check is performing the same validation
Apply this diff to remove the redundancy:
advancedLicense: z
.array(z.enum(Object.values(AdvancedLicense) as [string, ...string[]]))
- .nonempty()
- .refine((value) => {
- return value.length > 0
- }),
+ .nonempty(),Note: Ideally, we would want to add conditional validation to ensure advancedLicense is only provided when applicationFor is B_ADVANCED. However, as learned from previous discussions, the current version of the validation library doesn't support such conditional refinements.
Let's verify the AdvancedLicense enum definition:
✅ Verification successful
Let me check the MainAdvancedLicense and ProfessionalAdvancedLicense definitions to ensure we have the complete context.
Remove redundant array validation in advancedLicense schema
The review comment is correct. The schema has two redundant validations:
.nonempty()ensures the array has at least one element from the combined MainAdvancedLicense and ProfessionalAdvancedLicense enums- The
.refine()check performs the exact same validation by checkingvalue.length > 0
The suggested change to keep only .nonempty() is the right approach as it's more idiomatic with Zod and achieves the same validation requirement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the AdvancedLicense enum definition
rg -A 5 "export (type|enum|const) AdvancedLicense"
Length of output: 573
Script:
#!/bin/bash
# Check the MainAdvancedLicense and ProfessionalAdvancedLicense definitions
rg -A 3 "export (type|enum|const) (Main|Professional)AdvancedLicense"
Length of output: 754
libs/application/templates/driving-license/src/fields/AdvancedLicense/AdvancedLicense.tsx (2)
1-16: LGTM! Well-structured component definition with proper TypeScript usage.
The component follows best practices for reusability with proper type definitions and clear prop interface.
1-1: Verify feature flag implementation.
The PR mentions a feature flag "isDrivingLicenseAdvancedEnabled", but there's no explicit feature flag check in this component. Let's verify the implementation:
✅ Verification successful
Let me check the feature flag implementation in the getApplicationFeatureFlags.ts file since it was found in the search results.
Let me check how the feature flag is used to control the rendering of the AdvancedLicense component.
Let me check how the feature flag controls the section rendering in the form.
Feature flag implementation is properly handled
The feature flag isDrivingLicenseAdvancedEnabled is correctly implemented:
- It's defined in
getApplicationFeatureFlags.tsasALLOW_ADVANCED - The flag controls the inclusion of the advanced license section in the form via
allowAdvanced ? [sectionAdvancedLicenseSelection] : [] - The section itself has an additional condition to check if
applicationFor === LicenseTypes.B_ADVANCED - The AdvancedLicense component is only rendered when both the feature flag is enabled and the user has selected the advanced license option
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for feature flag usage
echo "Searching for feature flag implementation..."
rg -l "isDrivingLicenseAdvancedEnabled"
# Search for component usage to verify conditional rendering
echo "Checking component usage..."
rg -A 5 "AdvancedLicense"
Length of output: 27195
Script:
#!/bin/bash
# Check feature flag implementation
echo "Checking feature flag implementation..."
cat libs/application/templates/driving-license/src/lib/getApplicationFeatureFlags.ts
# Check how the feature flag is used in parent components
echo "Checking feature flag usage in parent components..."
rg -A 5 "isDrivingLicenseAdvancedEnabled" --type ts
Length of output: 2310
Script:
#!/bin/bash
# Check how the feature flag is used in the form configuration
echo "Checking form configuration..."
rg -A 10 "ALLOW_ADVANCED" --type ts
# Check the parent component that renders AdvancedLicense
echo "Checking parent component..."
rg -B 5 -A 10 "component: 'AdvancedLicense'" --type ts
Length of output: 5341
Script:
#!/bin/bash
# Check how the advanced license section is included in the form
echo "Checking form section inclusion..."
rg -B 10 "sectionAdvancedLicenseSelection" --type ts
# Check the condition for advanced license section
echo "Checking section condition..."
cat libs/application/templates/driving-license/src/forms/prerequisites/sectionAdvancedLicenseSelection.ts
Length of output: 5475
libs/application/templates/driving-license/src/lib/messages.ts (3)
896-995: Previous review comments about placeholder content are still valid.
The placeholder content for license categories needs to be replaced with proper descriptions.
996-1000: LGTM: Clear validation error message.
The error message clearly communicates that at least one option must be selected.
891-895: 🛠️ Refactor suggestion
Use named placeholders for better maintainability.
Replace positional arguments with named placeholders to improve clarity and reduce potential translation errors.
- defaultMessage: 'Réttindaaldur fyrir %1 er %2 ára',
+ defaultMessage: 'Réttindaaldur fyrir {licenseType} er {age} ára',Likely invalid or redundant comment.
libs/application/templates/driving-license/src/fields/AdvancedLicense/AdvancedLicense.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
libs/application/templates/driving-license/src/fields/AdvancedLicense/AdvancedLicense.tsx (3)
14-16: Consider enhancing component props for better reusability.As this component is in the libs directory, it should be designed for maximum reusability. Consider extracting a more flexible interface:
interface AdvancedLicenseProps extends FieldBaseProps { onLicenseChange?: (licenses: Array<keyof typeof AdvancedLicenseEnum>) => void; initialLicenses?: Array<keyof typeof AdvancedLicenseEnum>; disabled?: boolean; }
34-154: Add error boundary protection for robust error handling.As this is a critical form component, it should be wrapped with an error boundary to gracefully handle runtime errors.
class AdvancedLicenseErrorBoundary extends React.Component { static getDerivedStateFromError(error: Error) { return { hasError: true, error }; } render() { if (this.state.hasError) { return <ErrorMessage>Unable to load license options</ErrorMessage>; } return this.props.children; } }Wrap the component with this error boundary in the export:
export const AdvancedLicense = withErrorBoundary(AdvancedLicenseComponent);
157-157: Add JSDoc documentation for the exported component.As a library component, comprehensive documentation would improve usability.
/** * AdvancedLicense component allows users to select advanced driving license options. * @component * @example * ```tsx * <AdvancedLicense errors={formErrors} /> * ``` */ export { AdvancedLicense }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
libs/application/templates/driving-license/src/fields/AdvancedLicense/AdvancedLicense.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/application/templates/driving-license/src/fields/AdvancedLicense/AdvancedLicense.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."
| onChange={() => { | ||
| setSelectedLicenses((prev) => { | ||
| return prev.includes(option.code) | ||
| ? prev | ||
| .filter((item) => item !== option.code) | ||
| .filter( | ||
| (item) => item !== option.professional?.code, | ||
| ) | ||
| : [...prev, option.code] | ||
| }) | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify checkbox handlers using callback utilities.
The onChange handlers contain complex logic that could be simplified for better maintainability.
const handleLicenseToggle = useCallback((code: string, professional?: string) => (checked: boolean) => {
setSelectedLicenses(prev => {
if (!checked) {
return prev.filter(item => item !== code && item !== professional);
}
return professional ? [...prev, professional] : [...prev, code];
});
}, []);Then use it in the JSX:
-onChange={() => {
- setSelectedLicenses((prev) => {
- return prev.includes(option.code)
- ? prev
- .filter((item) => item !== option.code)
- .filter(
- (item) => item !== option.professional?.code,
- )
- : [...prev, option.code]
- })
-}}
+onChange={(e) => handleLicenseToggle(option.code, option.professional?.code)(e.target.checked)}Also applies to: 128-138
| const advancedLicenseValue = watch('advancedLicense') ?? [] | ||
|
|
||
| const [selectedLicenses, setSelectedLicenses] = | ||
| useState<Array<keyof typeof AdvancedLicenseEnum>>(advancedLicenseValue) | ||
|
|
||
| useEffect(() => { | ||
| setValue('advancedLicense', selectedLicenses) | ||
| }, [selectedLicenses, setValue]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize hook usage to prevent unnecessary rerenders.
The current implementation could cause unnecessary rerenders due to the watch hook and effect combination.
-const advancedLicenseValue = watch('advancedLicense') ?? []
+const advancedLicenseValue = watch('advancedLicense', [])
-useEffect(() => {
- setValue('advancedLicense', selectedLicenses)
-}, [selectedLicenses, setValue])
+useEffect(() => {
+ if (JSON.stringify(advancedLicenseValue) !== JSON.stringify(selectedLicenses)) {
+ setValue('advancedLicense', selectedLicenses, { shouldDirty: true })
+ }
+}, [selectedLicenses, setValue, advancedLicenseValue])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const advancedLicenseValue = watch('advancedLicense') ?? [] | |
| const [selectedLicenses, setSelectedLicenses] = | |
| useState<Array<keyof typeof AdvancedLicenseEnum>>(advancedLicenseValue) | |
| useEffect(() => { | |
| setValue('advancedLicense', selectedLicenses) | |
| }, [selectedLicenses, setValue]) | |
| const advancedLicenseValue = watch('advancedLicense', []) | |
| const [selectedLicenses, setSelectedLicenses] = | |
| useState<Array<keyof typeof AdvancedLicenseEnum>>(advancedLicenseValue) | |
| useEffect(() => { | |
| if (JSON.stringify(advancedLicenseValue) !== JSON.stringify(selectedLicenses)) { | |
| setValue('advancedLicense', selectedLicenses, { shouldDirty: true }) | |
| } | |
| }, [selectedLicenses, setValue, advancedLicenseValue]) |
Only adding the first step for now until we have the correct endpoints to use.
Behind a feature flag "isDrivingLicenseAdvancedEnabled"
Summary by CodeRabbit
Release Notes
New Features
AdvancedLicensecomponent for selecting advanced driving licenses.Improvements
allowAdvancedparameter for advanced license options.Bug Fixes
Alertcomponent to streamline functionality.