feat(register-new-machine): making location required if category is of certain type #17276
Conversation
sigruntg
added
the
deprecated:automerge
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
warning [email protected]: This version is no longer supported. Please see https://eslint.org/version-support for other options. WalkthroughThis pull request introduces a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17276 +/- ##
==========================================
- Coverage 35.70% 35.69% -0.01%
==========================================
Files 6948 6921 -27
Lines 148865 148517 -348
Branches 42503 42419 -84
==========================================
- Hits 53149 53010 -139
+ Misses 95716 95507 -209
Flags with carried forward coverage won't be shown. Click here to find out more. see 31 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 5 Total Test Services: 0 Failed, 5 Passed Test Services
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
libs/application/templates/aosh/register-new-machine/src/fields/LocationInputField/index.tsx (2)
42-44: Improve error message specificityThe current error message uses a default generic message. Consider using a more specific error message that explains the location requirement for elevators.
error={ displayError && location.length === 0 - ? formatMessage(coreErrorMessages.defaultError) + ? formatMessage(machine.errors.locationRequiredForElevator) : undefined }
18-19: Add type safety for form valuesThe
watchfunction results are type cast without validation. Consider using zod or similar for runtime type checking.+import { z } from 'zod'; + +const aboutMachineSchema = z.object({ + category: z.object({ + nameIs: z.string() + }).nullable() +}); + -const watchMachine = watch('machine.aboutMachine') as AboutMachine +const watchMachine = aboutMachineSchema.parse(watch('machine.aboutMachine'))libs/application/templates/aosh/register-new-machine/tsconfig.lib.json (1)
8-9: Consider using path mapping for node_modules referencesUsing relative paths to node_modules can be fragile. Consider using TypeScript's path mapping feature instead.
{ "compilerOptions": { + "paths": { + "@nx/react/*": ["node_modules/@nx/react/*"] + } }, "files": [ - "../../../../../node_modules/@nx/react/typings/cssmodule.d.ts", - "../../../../../node_modules/@nx/react/typings/image.d.ts" + "@nx/react/typings/cssmodule.d.ts", + "@nx/react/typings/image.d.ts" ] }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/application/templates/aosh/register-new-machine/src/fields/LocationInputField/index.tsx(1 hunks)libs/application/templates/aosh/register-new-machine/src/fields/index.ts(1 hunks)libs/application/templates/aosh/register-new-machine/src/forms/RegisterMachineForm/MachineSection/MachineBasicInformation.ts(1 hunks)libs/application/templates/aosh/register-new-machine/tsconfig.lib.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
libs/application/templates/aosh/register-new-machine/src/forms/RegisterMachineForm/MachineSection/MachineBasicInformation.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/aosh/register-new-machine/src/fields/LocationInputField/index.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/aosh/register-new-machine/tsconfig.lib.json (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/aosh/register-new-machine/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."
🔇 Additional comments (4)
libs/application/templates/aosh/register-new-machine/src/fields/index.ts (1)
6-6: LGTM!
The export follows the established pattern in the file.
libs/application/templates/aosh/register-new-machine/tsconfig.lib.json (2)
12-20: LGTM! Test exclusion patterns are well-structured
The exclude patterns are comprehensive and follow a consistent pattern.
22-22: LGTM! Include pattern is appropriately broad
The include pattern correctly captures all relevant source files.
libs/application/templates/aosh/register-new-machine/src/forms/RegisterMachineForm/MachineSection/MachineBasicInformation.ts (1)
129-129: Verify the LocationInputField component implementation
Let's ensure the new custom component exists and follows the library's guidelines for reusable components.
✅ Verification successful
Let me gather more information about the component's implementation and type definitions.
LocationInputField component implementation verified successfully
The component is properly implemented following the library's guidelines:
- Correctly exported and imported in the fields index
- Extends FieldBaseProps from @island.is/application/types
- Follows the same pattern as other form field components in the codebase
- Implements proper form validation and error handling
- Uses shared UI components (InputController) and localization utilities
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if LocationInputField component exists and exports types
rg -A 10 "export.*LocationInputField" "libs/application/templates/aosh/register-new-machine/src/"
# Verify component follows TypeScript guidelines
ast-grep --pattern 'interface $name {
$$$
}' "libs/application/templates/aosh/register-new-machine/src/fields/LocationInputField/"
# Check if the component is properly exported from the fields index
rg "export.*LocationInputField" "libs/application/templates/aosh/register-new-machine/src/fields/index.ts"
Length of output: 2001
Script:
#!/bin/bash
# Check the full implementation of LocationInputField
rg -l "LocationInputField" "libs/application/templates/aosh/register-new-machine/src/" | xargs cat
# Check for any type definitions in the component directory
fd "types?.ts" "libs/application/templates/aosh/register-new-machine/src/fields/LocationInputField" --exec cat
# Check if the component uses FieldBaseProps from a shared location
rg "FieldBaseProps" "libs/application/templates/aosh/register-new-machine/src/"
Length of output: 9181
libs/application/templates/aosh/register-new-machine/src/fields/LocationInputField/index.tsx
Outdated
Show resolved
Hide resolved
libs/application/templates/aosh/register-new-machine/src/fields/LocationInputField/index.tsx
Outdated
Show resolved
Hide resolved
...register-new-machine/src/forms/RegisterMachineForm/MachineSection/MachineBasicInformation.ts
Show resolved
Hide resolved
...register-new-machine/src/forms/RegisterMachineForm/MachineSection/MachineBasicInformation.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
libs/application/templates/aosh/register-new-machine/src/fields/LocationInputField/index.tsx (1)
32-48: Consider extracting error message logicThe error message logic in the render method could be simplified by extracting it to a separate function.
+const getErrorMessage = ( + displayError: boolean, + location: string, + formatMessage: (message: MessageDescriptor) => string +) => + displayError && location.length === 0 + ? formatMessage(coreErrorMessages.defaultError) + : undefined return ( <Box paddingTop={2}> <InputController {...} - error={ - displayError && location.length === 0 - ? formatMessage(coreErrorMessages.defaultError) - : undefined - } + error={getErrorMessage(displayError, location, formatMessage)} /> </Box> )libs/application/templates/aosh/register-new-machine/tsconfig.lib.json (1)
22-22: Consider restricting include patternsThe current include pattern
["**/*.js", "**/*.jsx", "**/*.ts", "**/*.tsx"]might be too broad as it includes files from all directories. Consider restricting it to thesrcdirectory to prevent including unwanted files.-"include": ["**/*.js", "**/*.jsx", "**/*.ts", "**/*.tsx"] +"include": ["src/**/*.js", "src/**/*.jsx", "src/**/*.ts", "src/**/*.tsx"]libs/application/templates/aosh/register-new-machine/src/forms/RegisterMachineForm/MachineSection/MachineBasicInformation.ts (2)
125-130: Consider accessibility: Empty title might affect screen readers.The empty title (
title: '') could impact accessibility. If theLocationInputFieldcomponent handles its own labeling, consider documenting this in a code comment for clarity.buildCustomField({ id: 'machine.basicInformation.location', - title: '', + title: '', // Note: Field label is handled within LocationInputField component width: 'half', component: 'LocationInputField', }),
125-130: Consider type safety improvements.Following the libs coding guidelines for TypeScript usage, consider exporting types for the custom field props to ensure type safety across different NextJS apps.
Consider adding:
export interface LocationInputFieldProps { value?: string; onChange: (value: string) => void; required?: boolean; error?: string; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/application/templates/aosh/register-new-machine/src/fields/LocationInputField/index.tsx(1 hunks)libs/application/templates/aosh/register-new-machine/src/fields/index.ts(1 hunks)libs/application/templates/aosh/register-new-machine/src/forms/RegisterMachineForm/MachineSection/MachineBasicInformation.ts(1 hunks)libs/application/templates/aosh/register-new-machine/tsconfig.lib.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
libs/application/templates/aosh/register-new-machine/src/forms/RegisterMachineForm/MachineSection/MachineBasicInformation.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/aosh/register-new-machine/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/aosh/register-new-machine/src/fields/LocationInputField/index.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/aosh/register-new-machine/tsconfig.lib.json (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/templates/aosh/register-new-machine/src/fields/index.ts (1)
6-6: LGTM!
The export follows the established pattern in the barrel file.
libs/application/templates/aosh/register-new-machine/src/forms/RegisterMachineForm/MachineSection/MachineBasicInformation.ts (1)
125-130: Verify required validation implementation.
According to the PR objectives, location should be required for certain machine categories. Ensure that the LocationInputField component properly implements this conditional validation.
✅ Verification successful
Let me gather more information about the validation implementation.
Location field validation is properly implemented for specific machine categories
The LocationInputField component correctly implements the conditional required validation:
- It tracks the machine category using form context
- Location is required specifically for the category "Fólkslyftur og vörulyftur" (Passenger and cargo lifts)
- Validation is enforced through both the UI (required field indicator) and form submission validation
- Error messages are displayed when validation fails
The implementation aligns with the PR objectives for category-specific location requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify LocationInputField implementation of conditional required validation
# Check the LocationInputField component implementation
ast-grep --pattern 'export const LocationInputField = $_' -A 20
# Look for validation logic related to machine categories
rg -l "machine.*category" --type ts
Length of output: 3303
Script:
#!/bin/bash
# Search for LocationInputField implementation
fd LocationInputField --type ts --exec cat {}
# Search for validation rules or schemas related to machine location
rg "location.*required|required.*location" --type ts -A 3
# Look for machine category validation logic
rg "machine\.basicInformation\.location.*validation|validation.*machine\.basicInformation\.location" --type ts -A 3
Length of output: 397
Script:
#!/bin/bash
# Search for LocationInputField implementation with correct extension
fd -e tsx -e ts LocationInputField --exec cat {}
# Search for validation schemas in the register-new-machine directory
rg -t ts -t tsx "validation|schema|required" libs/application/templates/aosh/register-new-machine/src/ -A 3
# Look for machine category conditions
rg -t ts -t tsx "machine.*category.*location|location.*machine.*category" libs/application/templates/aosh/register-new-machine/src/ -A 3
Length of output: 357
Script:
#!/bin/bash
# Search for LocationInputField implementation
find . -type f \( -name "*.ts" -o -name "*.tsx" \) -exec grep -l "LocationInputField" {} \; -exec cat {} \;
# Search for validation schemas in register-new-machine
find ./libs/application/templates/aosh/register-new-machine/src -type f \( -name "*.ts" -o -name "*.tsx" \) -exec grep -A 3 "validation\|schema\|required" {} \;
# Look for machine category validation
find ./libs/application/templates/aosh/register-new-machine/src -type f \( -name "*.ts" -o -name "*.tsx" \) -exec grep -A 3 "machine.*category.*location\|location.*machine.*category" {} \;
Length of output: 10621
libs/application/templates/aosh/register-new-machine/src/fields/LocationInputField/index.tsx
Show resolved
Hide resolved
libs/application/templates/aosh/register-new-machine/src/fields/LocationInputField/index.tsx
Show resolved
Hide resolved
libs/application/templates/aosh/register-new-machine/src/fields/LocationInputField/index.tsx
Outdated
Show resolved
Hide resolved
libs/application/templates/aosh/register-new-machine/src/fields/LocationInputField/index.tsx
Outdated
Show resolved
Hide resolved
sigruntg
removed
the
deprecated:automerge
sigruntg
added
the
deprecated: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
LocationInputFieldcomponent for location input in the machine registration form.RegisterMachineFormto utilize the new location input field.Bug Fixes
Chores