fix(transport-authority): Get correct requiresMileageRegistration value #16639
Conversation
Stop using mileageRequired from answers in getSelectedVehicle and always look at the value from externalData (it should not change)
…leswithmileageandinsp (not basicVehicleInformation) Cleanup make+vehcom (basicVehicleInformation) vs make (currentvehicles)
WalkthroughThe pull request includes significant updates across several service classes related to vehicle management. Key changes involve the introduction of a new API for vehicle data retrieval, modifications to error handling, and updates to the data structure used for vehicle properties. Several methods have been restructured to enhance validation and streamline the process of handling vehicle ownership changes. Additionally, there are modifications to the GraphQL queries and the schema definitions to align with the new data requirements. Changes
Possibly related PRs
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
🧹 Outside diff range and nitpick comments (16)
libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/getSelectedVehicle.ts (1)
Line range hint 3-46: Consider adding validation and error handling.
The function would benefit from:
- Validating the mileage reading format
- Adding bounds checking for the vehicle index
- Splitting the function into smaller, focused functions (one for selection, one for updating mileage)
Would you like me to provide a refactored version that addresses these concerns?
🧰 Tools
🪛 Biome
[error] 36-36: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/getSelectedVehicle.ts (2)
Line range hint 16-26: Enhance type safety and simplify null checks.
The current implementation could benefit from improved type safety and simplified null checks.
Consider applying these improvements:
const currentVehicleList =
- (externalData?.currentVehicleList?.data as CurrentVehiclesAndRecords) ??
- undefined
+ (externalData?.currentVehicleList?.data as CurrentVehiclesAndRecords | undefined)
const vehicleIndex = getValueViaPath(
answers,
'pickVehicle.vehicle',
- '',
+ undefined,
) as string
+ if (!vehicleIndex) {
+ throw new Error('Vehicle index is required')
+ }Line range hint 26-35: Add validation for mileage reading and index parsing.
The current implementation lacks validation for the mileage reading format and index parsing.
Consider adding these safety checks:
const mileageReading = getValueViaPath(
answers,
'vehicleMileage.mileageReading',
- '',
+ undefined,
) as string
+ if (mileageReading && !/^\d+$/.test(mileageReading)) {
+ throw new Error('Invalid mileage reading format')
+ }
const index = parseInt(vehicleIndex, 10)
+ if (isNaN(index)) {
+ throw new Error('Invalid vehicle index format')
+ }libs/application/templates/transport-authority/change-operator-of-vehicle/src/forms/ChangeOperatorOfVehicleForm/InformationSection/vehicleSubSection.ts (1)
Line range hint 1-91: Consider adding JSDoc comments for better documentation.
The code follows good practices for reusability and type safety. However, adding JSDoc comments would improve maintainability by documenting the purpose of the subsection and its configuration options.
+/**
+ * Builds the vehicle information subsection of the change operator form.
+ * This section displays vehicle details including plate number, type,
+ * and mileage information when required.
+ */
export const vehicleSubSection = buildSubSection({
id: 'vehicle',
title: information.labels.vehicle.sectionTitle,libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/lib/dataSchema.ts (1)
Update validation logic across vehicle-related schemas
The same fragile validation patterns are present in multiple vehicle-related schemas:
transfer-of-vehicle-ownership/src/lib/dataSchema.tschange-operator-of-vehicle/src/lib/dataSchema.ts
All these files use the same unsafe string parsing with split(' ')[0] and mixed string/number comparisons that could lead to runtime errors. The suggested safer implementation should be applied consistently across all these files to maintain uniform validation behavior and robustness.
Key locations requiring updates:
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/lib/dataSchema.tslibs/application/templates/transport-authority/change-operator-of-vehicle/src/lib/dataSchema.tslibs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/lib/dataSchema.ts
🔗 Analysis chain
Line range hint 106-119: Improve validation logic robustness
The current validation logic has several potential issues:
- Unsafe string parsing with
split(' ')[0]could throw if value is undefined - Mixed string/number comparisons using
Number()could result in NaN - Zero mileage is allowed when
requireMileageis false, which might not be intended
Consider this safer implementation:
(x: VehicleMileage) => {
if (x.requireMileage) {
+ const value = Number(x.value?.trim() || '0')
+ const reading = Number(x.mileageReading?.trim() || '0')
+ if (isNaN(value) || isNaN(reading)) return false
return (
- (x.value !== undefined &&
- x.value !== '' &&
- parseInt(x.value?.split(' ')[0]) > 0 &&
- x.mileageReading === undefined) ||
- Number(x.value) >= Number(x.mileageReading)
+ (value > 0 && !x.mileageReading) ||
+ value >= reading
)
} else {
+ if (!x.value?.trim()) return true
+ const value = Number(x.value.trim())
+ return !isNaN(value) && value >= 0
- return (
- x.value === undefined ||
- x.value === '' ||
- parseInt(x.value?.split(' ')[0]) >= 0
- )
}
}This implementation:
- Safely handles undefined values and whitespace
- Properly validates number parsing
- Maintains the same validation rules but with better error handling
Let's verify if there are similar validation patterns elsewhere that might need the same improvements:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar validation patterns in other vehicle-related schemas
rg -l "mileageReading|parseInt.*split.*0.*" libs/application/templates/transport-authority
Length of output: 2479
Script:
#!/bin/bash
# Let's check the validation patterns in the related dataSchema files
rg -A 5 -B 5 "parseInt.*split.*0.*|mileageReading.*value" libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/lib/dataSchema.ts libs/application/templates/transport-authority/change-operator-of-vehicle/src/lib/dataSchema.ts
Length of output: 6379
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/forms/TransferOfVehicleOwnershipForm/InformationSection/vehicleSubSection.ts (1)
Line range hint 89-102: Ensure consistent mileage requirement check.
The condition for showing the mileage input field uses requireMileage directly from the vehicle object, which correctly aligns with the PR's goal of using the source value. However, consider using the hidden input's value instead for consistency.
condition: (answers, externalData) => {
const vehicle = getSelectedVehicle(
externalData,
answers,
) as VehiclesCurrentVehicle
- return vehicle?.requireMileage || false
+ return answers.vehicleMileage?.requireMileage || false
},libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/lib/dataSchema.ts (1)
Line range hint 110-123: Consider refactoring the validation logic for better maintainability.
The current implementation could benefit from:
- Extracting the mileage parsing logic into a helper function
- Using explicit number validation
- Handling edge cases more robustly
Consider this refactoring:
+ const parseMileage = (value?: string): number | null => {
+ if (!value?.trim()) return null;
+ const parsed = parseInt(value.split(' ')[0]);
+ return isNaN(parsed) ? null : parsed;
+ };
.refine(
(x: VehicleMileage) => {
- if (x.requireMileage) {
- return (
- (x.value !== undefined &&
- x.value !== '' &&
- parseInt(x.value?.split(' ')[0]) > 0 &&
- x.mileageReading === undefined) ||
- Number(x.value) >= Number(x.mileageReading)
- )
- } else {
- return (
- x.value === undefined ||
- x.value === '' ||
- parseInt(x.value?.split(' ')[0]) >= 0
- )
- }
+ const value = parseMileage(x.value);
+ const reading = parseMileage(x.mileageReading);
+
+ if (!x.requireMileage) {
+ return value === null || value >= 0;
+ }
+
+ if (!value || value <= 0) return false;
+ return !reading || value >= reading;
},libs/api/domains/energy-funds/src/lib/energyFunds.service.ts (1)
59-66: Enhance readability by simplifying method chaining.
The current method chaining can be simplified for better readability. Consider refactoring the code to make it more concise.
Apply this change:
- const result = await this.vehiclesApiWithAuth(
- auth,
- ).currentvehicleswithmileageandinspGet({
- permno: permno,
- showOwned: true,
- showCoowned: false,
- showOperated: false,
- })
+ const result = await this.vehiclesApiWithAuth(auth)
+ .currentvehicleswithmileageandinspGet({
+ permno,
+ showOwned: true,
+ showCoowned: false,
+ showOperated: false,
+ })libs/api/domains/transport-authority/src/lib/transportAuthority.service.ts (2)
126-130: Consistent naming: Use requiresMileageRegistration in returned object
For consistency, consider using the same property name requiresMileageRegistration in the returned basicVehicleInformation as used in the vehicle object.
Apply this diff for consistency:
return {
basicVehicleInformation: {
permno: vehicle.permno,
make: vehicle.make,
color: vehicle.colorName,
- requireMileage: vehicle.requiresMileageRegistration,
+ requiresMileageRegistration: vehicle.requiresMileageRegistration,
mileageReading: (mileageReadings?.[0]?.mileage ?? '').toString(),
},313-317: Consistent property naming in returned basicVehicleInformation
Again, for consistency, consider using requiresMileageRegistration instead of requireMileage.
Apply this diff:
basicVehicleInformation: {
color: vehicle.colorName,
make: vehicle.make,
permno: vehicle.permno,
- requireMileage: vehicle.requiresMileageRegistration,
+ requiresMileageRegistration: vehicle.requiresMileageRegistration,
mileageReading: (mileageReadings?.[0]?.mileage ?? '').toString(),
},libs/application/template-api-modules/src/lib/modules/templates/transport-authority/change-operator-of-vehicle/change-operator-of-vehicle.service.ts (2)
125-132: Inconsistency Between Comments and Conditional Logic
The comments indicate that when vehicleCount >= 5, radio buttons should be displayed, but the condition checks for countResult > 5. This seems inconsistent. Please review the comments and conditions to ensure they align correctly.
177-177: Ensure Safe Conversion to String
When accessing the mileage reading, consider handling the case where mileageReadings might be empty or mileage is undefined to avoid potential issues with .toString().
You might adjust the line as follows:
- mileageReading: (mileageReadings?.[0]?.mileage ?? '').toString(),
+ mileageReading:
+ mileageReadings && mileageReadings[0] && mileageReadings[0].mileage
+ ? mileageReadings[0].mileage.toString()
+ : undefined,libs/application/template-api-modules/src/lib/modules/templates/transport-authority/transfer-of-vehicle-ownership/transfer-of-vehicle-ownership.service.ts (4)
36-39: Typo in 'Milage' in 'CurrentVehiclesWithMilageAndNextInspDto'
The import CurrentVehiclesWithMilageAndNextInspDto contains a typo in 'Milage'. The correct spelling is 'Mileage'.
Line range hint 81-93: Add Error Handling for External API Call
The call to currentvehicleswithmileageandinspGet lacks error handling. If this external API call fails, it could cause unhandled exceptions.
Consider wrapping the API call in a try-catch block to handle potential errors gracefully.
let countResult = 0
try {
countResult =
(
await this.vehiclesApiWithAuth(auth).currentvehicleswithmileageandinspGet({
showOwned: true,
showCoowned: false,
showOperated: false,
page: 1,
pageSize: 1,
})
).totalRecords || 0
} catch (error) {
this.logger.error('Error fetching vehicle count', error)
throw new TemplateApiError(
{
title: coreErrorMessages.fetchVehicleCountFailed,
summary: coreErrorMessages.fetchVehicleCountFailed,
},
500,
)
}181-181: Simplify Mileage Reading Conversion
The mileage reading conversion can be simplified to handle undefined values more gracefully.
Replace:
mileageReading: (mileageReadings?.[0]?.mileage ?? '').toString(),With:
mileageReading: mileageReadings?.[0]?.mileage?.toString() || '',This avoids calling .toString() on an undefined value.
644-644: Fix Indentation in Template Literal
There appears to be inconsistent indentation in the template literal used for logging.
Ensure consistent indentation for improved readability:
this.logger.error(
`Error sending sms about submitApplication to
a phonenumber in application: ID: ${application.id},
role: ${recipientList[i].role}`,
e,
)📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (19)
- libs/api/domains/energy-funds/src/lib/energyFunds.service.ts (2 hunks)
- libs/api/domains/transport-authority/src/lib/transportAuthority.service.ts (6 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/transport-authority/change-co-owner-of-vehicle/change-co-owner-of-vehicle.service.ts (4 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/transport-authority/change-operator-of-vehicle/change-operator-of-vehicle.service.ts (4 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/transport-authority/transfer-of-vehicle-ownership/transfer-of-vehicle-ownership.service.ts (5 hunks)
- libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/forms/ChangeCoOwnerOfVehicleForm/InformationSection/vehicleSubSection.ts (1 hunks)
- libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/lib/dataSchema.ts (1 hunks)
- libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/shared/types.ts (1 hunks)
- libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/getSelectedVehicle.ts (3 hunks)
- libs/application/templates/transport-authority/change-operator-of-vehicle/src/forms/ChangeOperatorOfVehicleForm/InformationSection/vehicleSubSection.ts (1 hunks)
- libs/application/templates/transport-authority/change-operator-of-vehicle/src/lib/dataSchema.ts (1 hunks)
- libs/application/templates/transport-authority/change-operator-of-vehicle/src/shared/types.ts (1 hunks)
- libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/getSelectedVehicle.ts (3 hunks)
- libs/application/templates/transport-authority/order-vehicle-license-plate/src/graphql/queries.ts (0 hunks)
- libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/forms/TransferOfVehicleOwnershipForm/InformationSection/vehicleSubSection.ts (1 hunks)
- libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/lib/dataSchema.ts (1 hunks)
- libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/shared/types.ts (1 hunks)
- libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/getSelectedVehicle.ts (3 hunks)
- libs/clients/transport-authority/vehicle-codetables/src/lib/vehicleCodetablesClient.service.ts (0 hunks)
💤 Files with no reviewable changes (2)
- libs/application/templates/transport-authority/order-vehicle-license-plate/src/graphql/queries.ts
- libs/clients/transport-authority/vehicle-codetables/src/lib/vehicleCodetablesClient.service.ts
🧰 Additional context used
📓 Path-based instructions (17)
libs/api/domains/energy-funds/src/lib/energyFunds.service.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/api/domains/transport-authority/src/lib/transportAuthority.service.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/template-api-modules/src/lib/modules/templates/transport-authority/change-co-owner-of-vehicle/change-co-owner-of-vehicle.service.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/template-api-modules/src/lib/modules/templates/transport-authority/change-operator-of-vehicle/change-operator-of-vehicle.service.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/template-api-modules/src/lib/modules/templates/transport-authority/transfer-of-vehicle-ownership/transfer-of-vehicle-ownership.service.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/transport-authority/change-co-owner-of-vehicle/src/forms/ChangeCoOwnerOfVehicleForm/InformationSection/vehicleSubSection.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/transport-authority/change-co-owner-of-vehicle/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/transport-authority/change-co-owner-of-vehicle/src/shared/types.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/transport-authority/change-co-owner-of-vehicle/src/utils/getSelectedVehicle.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/transport-authority/change-operator-of-vehicle/src/forms/ChangeOperatorOfVehicleForm/InformationSection/vehicleSubSection.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/transport-authority/change-operator-of-vehicle/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/transport-authority/change-operator-of-vehicle/src/shared/types.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/transport-authority/change-operator-of-vehicle/src/utils/getSelectedVehicle.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/transport-authority/transfer-of-vehicle-ownership/src/forms/TransferOfVehicleOwnershipForm/InformationSection/vehicleSubSection.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/transport-authority/transfer-of-vehicle-ownership/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/transport-authority/transfer-of-vehicle-ownership/src/shared/types.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/transport-authority/transfer-of-vehicle-ownership/src/utils/getSelectedVehicle.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/transport-authority/change-co-owner-of-vehicle/src/utils/getSelectedVehicle.ts
[error] 36-36: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/getSelectedVehicle.ts
[error] 36-36: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/getSelectedVehicle.ts
[error] 36-36: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (25)
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/getSelectedVehicle.ts (2)
16-16: LGTM: Simplified control flow by removing redundant mileage check
The removal of the mileage requirement check here aligns with the PR objective to centralize mileage registration requirements on the SGS side.
Line range hint 1-46: Verify integration with other transport-authority changes
Let's verify that this utility function aligns with other changes in the transport-authority module, particularly regarding the mileage registration requirements.
✅ Verification successful
Let me gather more information about the mileage registration functionality.
Let me try one more search with corrected file type parameters.
Implementation aligns with transport-authority module standards
The getSelectedVehicle utility function in the change-co-owner-of-vehicle template follows the same pattern used across other transport-authority templates (transfer-of-vehicle-ownership, change-operator-of-vehicle) for handling vehicle selection and mileage updates. The implementation:
- Correctly handles mileage registration through the
vehicleMileageform field - Uses consistent data schema validation across templates
- Maintains the same structure for vehicle selection and mileage updates as other transport-authority modules
- Properly integrates with the shared types and form validation patterns
The code is consistent with the transport-authority module's standards and requirements for vehicle management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related changes in transport-authority module
echo "Searching for related vehicle mileage registration changes..."
rg -t typescript "requiresMileageRegistration|mileageReading" "libs/application/templates/transport-authority"
echo "Checking for other instances of currentVehicleList usage..."
ast-grep --pattern 'currentVehicleList?.vehicles' "libs/application/templates/transport-authority"
Length of output: 2762
Script:
#!/bin/bash
# Search for mileage-related form fields and validations
echo "Searching for mileage form fields..."
rg -t ts -t tsx "vehicleMileage|mileageReading" -A 3 "libs/application/templates/transport-authority"
echo "Searching for vehicle selection patterns..."
ast-grep --pattern 'pickVehicle.vehicle'
Length of output: 350
Script:
#!/bin/bash
# Search for mileage-related form fields and validations with correct file extensions
echo "Searching for mileage form fields..."
rg "vehicleMileage|mileageReading" -A 3 --type-add 'ts:*.{ts,tsx}' --type ts "libs/application/templates/transport-authority"
# Check for vehicle selection implementations
echo "Checking vehicle selection implementations..."
ast-grep --pattern 'function getSelectedVehicle($$$) {
$$$
}'
Length of output: 36873
🧰 Tools
🪛 Biome
[error] 36-36: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/getSelectedVehicle.ts (2)
Line range hint 1-3: LGTM! TypeScript implementation is solid.
The code properly utilizes TypeScript types and follows type safety best practices.
38-44: Verify the implications of mutating external data.
While the code correctly checks for frozen objects before mutation, directly modifying the currentVehicleList might have unintended side effects elsewhere in the application. Consider creating a new array instead of mutating the existing one.
Let's check if this data is used elsewhere:
✅ Verification successful
Let me check how currentVehicleList is used in these files to understand the mutation implications.
Based on the search results, I can now provide a final response about the mutation concern:
The mutation of currentVehicleList is safe and intentional in this context.
The mutation is localized to the getSelectedVehicle utility and only updates the mileage reading of a specific vehicle in the list. This is a controlled mutation that:
- Only occurs after checking if the object isn't frozen
- Uses the spread operator to maintain immutability of other properties
- Is scoped to the vehicle selection workflow where the data is managed as form state
The currentVehicleList is used as form state across different vehicle-related templates (license plate, ownership transfer, etc.) where such mutations are expected and handled appropriately through React's form state management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of currentVehicleList to verify mutation safety
rg -l "currentVehicleList" --type ts
Length of output: 3567
Script:
#!/bin/bash
# Check the usage pattern of currentVehicleList in the relevant files
rg "currentVehicleList" --type ts -A 3 -B 3
Length of output: 99490
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/getSelectedVehicle.ts (1)
45-45: 🛠️ Refactor suggestion
Add type safety to return value and align with PR objectives.
The return statement should handle undefined cases and align with the PR's objective of streamlining vehicle data handling.
Apply this improvement:
- return currentVehicleList?.vehicles[index]
+ return currentVehicleList?.vehicles?.[index] ?? nullLet's verify the usage of this function across the codebase:
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/shared/types.ts (2)
42-45: LGTM! Property rename aligns with PR objectives.
The rename from isRequired to requireMileage in the VehicleMileage type is consistent with the PR's goal of streamlining mileage registration handling. This change maintains consistency with similar changes in related schemas.
Line range hint 1-85: Verify type exports for tree-shaking optimization.
The file follows TypeScript best practices with proper type exports. However, let's verify that these types are actually used across the codebase to ensure effective tree-shaking.
✅ Verification successful
Based on the search results, I can generate the final response as we have comprehensive evidence of type usage across the codebase:
All exported types are actively used across multiple templates and components.
The search results confirm that all exported types from this file are properly utilized:
VehicleMileagetype is used in multiple transport authority templates and vehicle-related componentsVehiclesCurrentVehicleand related types (CurrentVehiclesAndRecords,VehiclesCurrentVehicleWithOwnerchangeChecks) are extensively used across vehicle-related templatesReviewSectionProps,ReviewScreenProps, andReviewStatetypes are used consistently in review-related components across multiple application templates
The types follow proper tree-shaking practices as they are:
- Explicitly exported
- Used in multiple dependent components
- Properly referenced in type positions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of exported types across the codebase
# Expected: Find references to these types in other files
echo "Checking usage of VehicleMileage type..."
rg "VehicleMileage" --type ts
echo "Checking usage of other exported types..."
rg "VehiclesCurrentVehicle|CurrentVehiclesAndRecords|VehiclesCurrentVehicleWithOwnerchangeChecks|ReviewSectionProps|ReviewScreenProps|ReviewState" --type ts
Length of output: 64333
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/shared/types.ts (2)
87-87: Clarify property name discrepancy with PR objectives.
The PR objectives mention requiresMileageRegistration, but the type uses requireMileage. While both convey similar meaning, we should ensure this difference is intentional and won't cause confusion.
Let's check if both property names are used in the codebase:
✅ Verification successful
Property name discrepancy is intentional and correctly mapped
The codebase shows a clear pattern where requiresMileageRegistration is used in the core vehicle-related services and APIs, while requireMileage is used specifically in transport authority templates and forms. The mapping between these properties is handled correctly in the services:
// In transport authority services:
requireMileage: vehicle.requiresMileageRegistrationThis is a deliberate design choice where the internal vehicle system uses the more verbose requiresMileageRegistration, while the transport authority forms use the shorter requireMileage property. The mapping is consistent across all transport authority templates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for both property names to ensure consistency
echo "Checking for 'requiresMileageRegistration' usage..."
rg "requiresMileageRegistration"
echo "Checking for 'requireMileage' usage..."
rg "requireMileage"
Length of output: 13724
87-90: LGTM! Type definition follows TypeScript best practices.
The type definition is well-structured and consistent with other vehicle-related types in the file.
Let's verify the impact of the property rename across the codebase:
✅ Verification successful
Let me verify the usage of the new property name and check for any potential issues.
Property rename is correctly implemented and consistently used across the codebase
The verification shows that:
- The
requireMileageproperty is consistently defined in all relevant type definitions - The property is correctly mapped from
requiresMileageRegistrationin the transport authority service - The type structure is identical across all vehicle-related modules
- The property rename maintains backward compatibility through the service layer mapping
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining occurrences of the old property name
# and verify the usage of the new property name
echo "Checking for old property name 'isRequired' in vehicle-related contexts..."
rg -l "isRequired.*vehicle|vehicle.*isRequired"
echo "Verifying usage of new property name..."
rg -l "requireMileage"
Length of output: 3404
Script:
#!/bin/bash
# Let's check the actual usage context of requireMileage in a few key files
echo "Checking usage in types.ts files:"
rg -A 2 -B 2 "requireMileage" "libs/application/templates/transport-authority/*/src/shared/types.ts"
echo -e "\nChecking usage in service files:"
rg -A 2 -B 2 "requireMileage" "libs/api/domains/transport-authority/src/lib/transportAuthority.service.ts"
echo -e "\nChecking GraphQL models:"
rg -A 2 -B 2 "requireMileage" "libs/api/domains/transport-authority/src/lib/graphql/models/getCurrentVehicles.model.ts"
Length of output: 1191
Script:
#!/bin/bash
# Let's check the specific files with correct paths and verify the property mapping
echo "Checking transfer-of-vehicle-ownership types:"
rg -A 2 -B 2 "requireMileage" "libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/shared/types.ts"
echo -e "\nChecking change-operator-of-vehicle types:"
rg -A 2 -B 2 "requireMileage" "libs/application/templates/transport-authority/change-operator-of-vehicle/src/shared/types.ts"
echo -e "\nChecking change-co-owner-of-vehicle types:"
rg -A 2 -B 2 "requireMileage" "libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/shared/types.ts"
echo -e "\nChecking for requiresMileageRegistration in service:"
rg -A 2 -B 2 "requiresMileageRegistration" "libs/api/domains/transport-authority/src/lib/transportAuthority.service.ts"
Length of output: 2349
libs/application/templates/transport-authority/change-operator-of-vehicle/src/shared/types.ts (1)
Line range hint 1-94: LGTM! File structure follows best practices.
The file demonstrates good TypeScript practices:
- Clear and well-defined type definitions
- Proper type exports for reusability
- Modular organization of related types
libs/application/templates/transport-authority/change-operator-of-vehicle/src/forms/ChangeOperatorOfVehicleForm/InformationSection/vehicleSubSection.ts (1)
49-49: Verify property name consistency across the codebase.
The hidden input ID has been updated to use requireMileage, but the PR mentions requiresMileageRegistration. Let's ensure consistent naming is used throughout the codebase.
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/forms/ChangeCoOwnerOfVehicleForm/InformationSection/vehicleSubSection.ts (1)
51-51: Field renaming aligns with standardization effort.
The change from isRequired to requireMileage aligns with the PR's objective to standardize mileage registration handling across the system.
Let's verify the consistency of this field name across the codebase:
✅ Verification successful
Field renaming is consistently applied across the codebase
The verification confirms that:
- No instances of the old field name
vehicleMileage.isRequiredremain in the codebase - The new field name
vehicleMileage.requireMileageis consistently used across multiple transport authority modules:- change-co-owner-of-vehicle
- change-operator-of-vehicle
- transfer-of-vehicle-ownership
- ui-fields/FindVehicleFormField
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of old field name and confirm consistent usage of new field name
# Expected: No instances of old name, consistent usage of new name in related files
# Check for any remaining instances of the old field name
echo "Checking for old field name..."
rg "vehicleMileage\.isRequired"
# Check usage of new field name in related files
echo "Checking usage of new field name..."
rg "vehicleMileage\.requireMileage"
# Check for any potential schema definitions
echo "Checking schema definitions..."
ast-grep --pattern 'interface $_ {
$$$
requireMileage: $_
$$$
}'
Length of output: 2232
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/lib/dataSchema.ts (1)
100-102: LGTM! Property rename improves clarity
The rename from isRequired to requireMileage better describes the property's purpose and aligns with the PR's goal of improving mileage registration handling.
libs/application/templates/transport-authority/change-operator-of-vehicle/src/lib/dataSchema.ts (2)
100-102: LGTM: Schema definition aligns with PR objectives.
The renaming from isRequired to requireMileage aligns with the PR's goal of improving mileage registration handling. The optional fields provide flexibility for different validation scenarios.
Line range hint 106-120: Verify edge cases in mileage validation logic.
While the validation logic looks correct, please ensure these edge cases are covered in tests:
- When
requireMileageis true:- Empty string for value
- Negative values
- Space-separated values (e.g., "100 km")
- When
requireMileageis false:- Undefined value
- Empty string
- Zero value
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/forms/TransferOfVehicleOwnershipForm/InformationSection/vehicleSubSection.ts (1)
Line range hint 70-77: Verify the impact of renaming the hidden input field ID.
The rename from isRequired to requireMileage aligns with the PR objectives. However, we should verify that all references to this field ID have been updated accordingly.
✅ Verification successful
No references to old field ID found, new field ID is used consistently
The verification shows that:
- No instances of the old field ID
vehicleMileage.isRequiredwere found in the codebase - The new field ID
vehicleMileage.requireMileageis used consistently across all relevant files:- Form field definitions in vehicle subsections
- Vehicle selection components (radio and select fields)
- FindVehicleFormField component
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining references to the old field ID
# and verify the new field ID is used consistently
echo "Checking for old field ID references..."
rg "vehicleMileage\.isRequired"
echo "Verifying consistent usage of new field ID..."
rg "vehicleMileage\.requireMileage"
Length of output: 2126
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/lib/dataSchema.ts (1)
104-106: LGTM! Property renaming improves clarity.
The renaming from isRequired to requireMileage better describes the field's purpose and aligns with the PR's objective to improve mileage registration handling.
libs/api/domains/energy-funds/src/lib/energyFunds.service.ts (2)
106-106: Verify the property 'vehicle.requiresMileageRegistration' exists.
Ensure that the vehicle object returned from currentvehicleswithmileageandinspGet has the requiresMileageRegistration property. If it doesn't, update the code to access the correct property or adjust the API call accordingly.
104-104: Confirm the correct property for vehicle color.
Verify whether vehicle.colorName is the appropriate property for the vehicle's color. If the correct property is vehicle.color, consider updating the code.
Apply this diff if necessary:
- color: vehicle.colorName,
+ color: vehicle.color,libs/api/domains/transport-authority/src/lib/transportAuthority.service.ts (5)
28-28: Importing MileageReadingApi is appropriate
The addition of MileageReadingApi will enable mileage reading functionalities as intended.
40-40: Injecting mileageReadingApi into the constructor
Adding mileageReadingApi to the constructor ensures that the service can access mileage reading methods.
47-50: Implementing mileageReadingApiWithAuth method correctly
The mileageReadingApiWithAuth method is properly implemented to apply authentication middleware.
119-122: Handling exceptions when retrieving mileage readings
Ensure that any exceptions thrown by getMileageReading are properly handled to avoid unhandled promise rejections.
Consider adding a try-catch block to handle potential errors:
+ try {
const mileageReadings = await this.mileageReadingApiWithAuth(
auth,
).getMileageReading({ permno: permno })
+ } catch (error) {
+ // Handle or log the error as appropriate
+ throw new Error('Failed to retrieve mileage readings.')
+ }303-306: Handle potential errors when retrieving mileage readings
Similar to the previous method, ensure exceptions from getMileageReading are handled to prevent unhandled errors.
Consider wrapping the call in a try-catch block:
+ try {
const mileageReadings = await this.mileageReadingApiWithAuth(
auth,
).getMileageReading({ permno: permno })
+ } catch (error) {
+ // Handle or log the error as appropriate
+ throw new Error('Failed to retrieve mileage readings.')
+ }libs/application/template-api-modules/src/lib/modules/templates/transport-authority/transfer-of-vehicle-ownership/transfer-of-vehicle-ownership.service.ts (1)
Line range hint 81-144: Adherence to Coding Guidelines for Libraries
This file is under the libs directory. Ensure that:
- The code promotes reusability across different applications.
- All props and types are properly defined and exported using TypeScript.
- Tree-shaking and bundling practices are considered to optimize the build.
...ion/templates/transport-authority/change-co-owner-of-vehicle/src/utils/getSelectedVehicle.ts
Outdated
Show resolved
Hide resolved
...ion/templates/transport-authority/change-co-owner-of-vehicle/src/utils/getSelectedVehicle.ts
Outdated
Show resolved
Hide resolved
...ion/templates/transport-authority/change-co-owner-of-vehicle/src/utils/getSelectedVehicle.ts
Show resolved
Hide resolved
...ion/templates/transport-authority/change-operator-of-vehicle/src/utils/getSelectedVehicle.ts
Outdated
Show resolved
Hide resolved
...ion/templates/transport-authority/change-operator-of-vehicle/src/utils/getSelectedVehicle.ts
Outdated
Show resolved
Hide resolved
...mplates/transport-authority/change-co-owner-of-vehicle/change-co-owner-of-vehicle.service.ts
Show resolved
Hide resolved
...mplates/transport-authority/change-co-owner-of-vehicle/change-co-owner-of-vehicle.service.ts
Show resolved
Hide resolved
...s/transport-authority/transfer-of-vehicle-ownership/transfer-of-vehicle-ownership.service.ts
Show resolved
Hide resolved
...s/transport-authority/transfer-of-vehicle-ownership/transfer-of-vehicle-ownership.service.ts
Outdated
Show resolved
Hide resolved
...s/transport-authority/transfer-of-vehicle-ownership/transfer-of-vehicle-ownership.service.ts
Show resolved
Hide resolved
Datadog ReportAll test runs ✅ 101 Total Test Services: 0 Failed, 98 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (3) |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
libs/api/domains/transport-authority/src/lib/transportAuthority.service.ts (1)
64-86: Enhance error handling in fetchVehicleDataForOwnerCoOwner
The error handling could be more specific by separating the "not found" case from the "no access" case.
Consider this improvement:
- if (!result || !result.data || result.data.length === 0) {
+ if (!result || !result.data) {
+ throw Error('Failed to fetch vehicle data')
+ }
+ if (result.data.length === 0) {
throw Error(
- 'Did not find the vehicle with that permno, or you are neither owner nor co-owner of the vehicle',
+ 'You do not have access to this vehicle. You must be either the owner or co-owner.'
)
}libs/application/template-api-modules/src/lib/modules/templates/transport-authority/change-co-owner-of-vehicle/change-co-owner-of-vehicle.service.ts (2)
93-102: Enhance error message clarity
The error message could be more specific about the requirement for vehicle ownership. Consider adding context about why the user needs to own at least one vehicle for this operation.
Apply this diff to improve the error message:
- title: coreErrorMessages.vehiclesEmptyListOwner,
- summary: coreErrorMessages.vehiclesEmptyListOwner,
+ title: 'Vehicle ownership required',
+ summary: 'You must own at least one vehicle to change co-ownership. No vehicles found under your ownership.',104-141: Document the rationale for vehicle count thresholds
While the code logic is clear, it would be helpful to document why specific thresholds (5 and 20) were chosen and how they affect the UI/UX.
Add explanatory comments:
+ // Vehicle count thresholds determine the UI component used:
+ // > 20: Search box for better performance with large lists
+ // 6-20: Dropdown for manageable list sizes
+ // ≤ 5: Radio buttons for optimal UX with small lists
if (countResult > 20) {
return {
totalRecords: countResult,
vehicles: [],
}
}libs/application/template-api-modules/src/lib/modules/templates/transport-authority/transfer-of-vehicle-ownership/transfer-of-vehicle-ownership.service.ts (3)
Line range hint 81-104: Consider using constants for pagination values
The magic number 1 for page size could be extracted into a named constant for better maintainability.
+ private readonly VEHICLE_COUNT_CHECK_PAGE_SIZE = 1;
+
async getCurrentVehiclesWithOwnerchangeChecks({
auth,
}: TemplateApiModuleActionProps) {
// Get total count of vehicles
const countResult =
(
await this.vehiclesApiWithAuth(
auth,
).currentvehicleswithmileageandinspGet({
showOwned: true,
showCoowned: false,
showOperated: false,
page: 1,
- pageSize: 1,
+ pageSize: this.VEHICLE_COUNT_CHECK_PAGE_SIZE,
})
).totalRecords || 0106-144: Extract threshold values into named constants
The magic numbers 20 and 5 used for vehicle count thresholds should be extracted into named constants to improve code maintainability and make the business logic more explicit.
+ private readonly VEHICLE_COUNT_SEARCH_THRESHOLD = 20;
+ private readonly VEHICLE_COUNT_DROPDOWN_THRESHOLD = 5;
+
async getCurrentVehiclesWithOwnerchangeChecks({
auth,
}: TemplateApiModuleActionProps) {
// ...
- if (countResult > 20) {
+ if (countResult > this.VEHICLE_COUNT_SEARCH_THRESHOLD) {
return {
totalRecords: countResult,
vehicles: [],
}
}
// ...
- if (countResult > 5) {
+ if (countResult > this.VEHICLE_COUNT_DROPDOWN_THRESHOLD) {
return this.mapVehicle(auth, vehicle, false)
}146-185: Enhance type safety with interface for mapped vehicle
Consider creating an interface for the return type of mapVehicle to improve type safety and documentation.
+interface MappedVehicle {
+ permno?: string;
+ make?: string;
+ color?: string;
+ role?: string;
+ requireMileage: boolean;
+ mileageReading: string;
+ isDebtLess?: boolean;
+ validationErrorMessages: string[] | null;
+}
+
- private async mapVehicle(
+ private async mapVehicle(
auth: User,
vehicle: CurrentVehiclesWithMilageAndNextInspDto,
fetchExtraData: boolean,
- ) {
+ ): Promise<MappedVehicle> {📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- libs/api/domains/energy-funds/src/lib/energyFunds.service.ts (2 hunks)
- libs/api/domains/transport-authority/src/lib/transportAuthority.service.ts (7 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/transport-authority/change-co-owner-of-vehicle/change-co-owner-of-vehicle.service.ts (4 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/transport-authority/change-operator-of-vehicle/change-operator-of-vehicle.service.ts (4 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/transport-authority/transfer-of-vehicle-ownership/transfer-of-vehicle-ownership.service.ts (5 hunks)
- libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/getSelectedVehicle.ts (3 hunks)
- libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/getSelectedVehicle.ts (3 hunks)
- libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/getSelectedVehicle.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- libs/api/domains/energy-funds/src/lib/energyFunds.service.ts
- libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/getSelectedVehicle.ts
- libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/getSelectedVehicle.ts
- libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/getSelectedVehicle.ts
🧰 Additional context used
📓 Path-based instructions (4)
libs/api/domains/transport-authority/src/lib/transportAuthority.service.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/template-api-modules/src/lib/modules/templates/transport-authority/change-co-owner-of-vehicle/change-co-owner-of-vehicle.service.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/template-api-modules/src/lib/modules/templates/transport-authority/change-operator-of-vehicle/change-operator-of-vehicle.service.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/template-api-modules/src/lib/modules/templates/transport-authority/transfer-of-vehicle-ownership/transfer-of-vehicle-ownership.service.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 (5)
libs/api/domains/transport-authority/src/lib/transportAuthority.service.ts (2)
28-28: LGTM: Clean dependency injection implementation
The MileageReadingApi is properly imported and injected following the dependency injection pattern.
Also applies to: 40-40
47-49: LGTM: Consistent auth middleware implementation
The method follows the established pattern for applying auth middleware.
libs/application/template-api-modules/src/lib/modules/templates/transport-authority/change-operator-of-vehicle/change-operator-of-vehicle.service.ts (3)
Line range hint 77-140: LGTM! Well-structured pagination and validation logic.
The implementation efficiently handles different vehicle count scenarios with appropriate optimizations:
- Validates empty results upfront
- Uses pagination for large datasets (>20 vehicles)
- Conditionally fetches extra data based on count thresholds
172-181: LGTM! Well-handled null checks and property mapping.
The implementation properly handles undefined values using optional chaining and nullish coalescing operators.
151-168:
Add error handling for external API calls.
The external service calls within the fetchExtraData block should be wrapped in try-catch to handle potential failures gracefully.
Apply this diff:
if (fetchExtraData) {
+ try {
// Get debt status
debtStatus = await this.vehicleServiceFjsV1Client.getVehicleDebtStatus(
auth,
vehicle.permno || '',
)
// Get operator change validation
validation =
await this.vehicleOperatorsClient.validateVehicleForOperatorChange(
auth,
vehicle.permno || '',
)
// Get mileage reading
mileageReadings = await this.mileageReadingApiWithAuth(
auth,
).getMileageReading({ permno: vehicle.permno || '' })
+ } catch (error) {
+ this.logger.error('Failed to fetch vehicle data', { error, permno: vehicle.permno })
+ throw new TemplateApiError(
+ {
+ title: 'Unable to retrieve vehicle information',
+ summary: 'Failed to fetch required vehicle data. Please try again.',
+ },
+ 500
+ )
+ }
}Likely invalid or redundant comment.
libs/api/domains/transport-authority/src/lib/transportAuthority.service.ts
Outdated
Show resolved
Hide resolved
...mplates/transport-authority/change-co-owner-of-vehicle/change-co-owner-of-vehicle.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
libs/api/domains/transport-authority/src/lib/transportAuthority.service.ts (2)
64-86: Enhance error handling specificity
While the consolidation of vehicle data fetching is good, the error handling could be more specific:
- if (!result || !result.data || result.data.length === 0) {
+ if (!result || !result.data) {
+ throw new Error('Failed to fetch vehicle data')
+ }
+ if (!result.totalRecords || result.data.length === 0) {
throw Error(
'Did not find the vehicle with that permno, or you are neither owner nor co-owner of the vehicle',
)
}281-286: Consider consistent property ordering
For better maintainability, consider ordering properties consistently across similar return objects. Match the order used in getVehicleOwnerchangeChecksByPermno:
basicVehicleInformation: {
- color: vehicle.colorName,
- make: vehicle.make,
permno: vehicle.permno,
+ make: vehicle.make,
+ color: vehicle.colorName,
requireMileage: vehicle.requiresMileageRegistration,
mileageReading: mileageReadings?.[0]?.mileage?.toString() ?? '',
},libs/application/template-api-modules/src/lib/modules/templates/transport-authority/change-operator-of-vehicle/change-operator-of-vehicle.service.ts (1)
Line range hint 77-140: LGTM! Consider adding logging for debugging.
The refactored vehicle retrieval logic with count-based validation is well-structured. The different handling paths for vehicle counts (>20, >5, ≤5) are clearly defined and properly implemented.
Consider adding debug logging before returning the results to help with troubleshooting:
+ this.logger.debug(
+ `Returning ${vehicles.length} vehicles out of ${countResult} total`,
+ )
return {
totalRecords: countResult,
vehicles: vehicles,
}libs/application/template-api-modules/src/lib/modules/templates/transport-authority/change-co-owner-of-vehicle/change-co-owner-of-vehicle.service.ts (1)
93-102: Enhance error message for empty vehicle list
The error message could be more specific about why the user needs to own vehicles. Consider adding context about the co-ownership change requirement.
- title: coreErrorMessages.vehiclesEmptyListOwner,
- summary: coreErrorMessages.vehiclesEmptyListOwner,
+ title: coreErrorMessages.vehiclesEmptyListOwner,
+ summary: 'You must own at least one vehicle to change its co-ownership. No vehicles found under your ownership.',libs/application/template-api-modules/src/lib/modules/templates/transport-authority/transfer-of-vehicle-ownership/transfer-of-vehicle-ownership.service.ts (1)
Line range hint 81-94: Consider extracting magic number to a constant
The page size value of 1 used for counting vehicles could be more maintainable as a named constant.
+private readonly VEHICLE_COUNT_PAGE_SIZE = 1;
+
async getCurrentVehiclesWithOwnerchangeChecks({
auth,
}: TemplateApiModuleActionProps) {
// Get total count of vehicles
const countResult =
(
await this.vehiclesApiWithAuth(
auth,
).currentvehicleswithmileageandinspGet({
showOwned: true,
showCoowned: false,
showOperated: false,
page: 1,
- pageSize: 1,
+ pageSize: this.VEHICLE_COUNT_PAGE_SIZE,
})
).totalRecords || 0📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- libs/api/domains/transport-authority/src/lib/transportAuthority.service.ts (7 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/transport-authority/change-co-owner-of-vehicle/change-co-owner-of-vehicle.service.ts (4 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/transport-authority/change-operator-of-vehicle/change-operator-of-vehicle.service.ts (4 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/transport-authority/transfer-of-vehicle-ownership/transfer-of-vehicle-ownership.service.ts (5 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
libs/api/domains/transport-authority/src/lib/transportAuthority.service.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/template-api-modules/src/lib/modules/templates/transport-authority/change-co-owner-of-vehicle/change-co-owner-of-vehicle.service.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/template-api-modules/src/lib/modules/templates/transport-authority/change-operator-of-vehicle/change-operator-of-vehicle.service.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/template-api-modules/src/lib/modules/templates/transport-authority/transfer-of-vehicle-ownership/transfer-of-vehicle-ownership.service.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/api/domains/transport-authority/src/lib/transportAuthority.service.ts (2)
28-28: LGTM: Clean implementation of MileageReadingApi integration
The new dependency and authentication method follow the established patterns in the codebase.
Also applies to: 40-40, 47-49
Line range hint 95-120: LGTM: Successful implementation of PR objectives
The changes correctly implement:
- Integration with the new vehicle data fetching method
- Usage of make without vehcom as specified
- Proper handling of mileage registration requirements
libs/application/template-api-modules/src/lib/modules/templates/transport-authority/change-operator-of-vehicle/change-operator-of-vehicle.service.ts (1)
172-181: LGTM! Clean and consistent property mapping.
The return object structure effectively:
- Uses optional chaining and nullish coalescing for safe property access
- Correctly maps
colorNameinstead ofcolor - Properly includes
requiresMileageRegistrationas specified in PR objectives
libs/application/template-api-modules/src/lib/modules/templates/transport-authority/change-co-owner-of-vehicle/change-co-owner-of-vehicle.service.ts (1)
104-111: Verify pagination implementation for large vehicle lists
When handling more than 20 vehicles, the implementation returns an empty array, relying on search box UI. This could impact user experience if the search functionality isn't properly implemented on the frontend.
✅ Verification successful
Let me gather more information about the vehicle search implementation to verify the pagination and search functionality.
Let me check one more aspect of the search implementation to verify the frontend-backend integration.
Search functionality is properly implemented for large vehicle lists
The implementation is correct and well-designed:
- Frontend has a proper search component with input field, loading states, and error handling
- Search functionality is implemented using GraphQL queries with proper pagination support
- The UI provides clear feedback when search limit is exceeded and includes proper user guidance
- The approach of returning an empty array for large lists (>20) and requiring search is a valid UX pattern to handle large datasets efficiently
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the frontend implements search functionality for vehicles
rg -A 5 "searchVehicle|searchBox|vehicleSearch" --type ts
Length of output: 1971
Script:
#!/bin/bash
# Check for vehicle search related GraphQL queries and mutations
rg -A 10 "vehiclesSearch|vehicleSearchGet" --type graphql --type ts
# Check for any pagination related code in vehicle search
rg -A 5 "pageSize|pageNumber|limit|offset" --type ts --glob "*vehicle*"
Length of output: 11439
Script:
#!/bin/bash
# Check for vehicle search component implementation and UI elements
rg -A 10 "searchBox|SearchBox|<Box|<Input" libs/service-portal/assets/src/screens/Lookup/Lookup.tsx
# Check the GraphQL query implementation for vehicle search
rg -A 10 "useGetVehiclesSearchLazyQuery" --type ts
Length of output: 4562
...mplates/transport-authority/change-operator-of-vehicle/change-operator-of-vehicle.service.ts
Show resolved
Hide resolved
...mplates/transport-authority/change-co-owner-of-vehicle/change-co-owner-of-vehicle.service.ts
Show resolved
Hide resolved
...mplates/transport-authority/change-co-owner-of-vehicle/change-co-owner-of-vehicle.service.ts
Show resolved
Hide resolved
...s/transport-authority/transfer-of-vehicle-ownership/transfer-of-vehicle-ownership.service.ts
Show resolved
Hide resolved
What
The value requiresMileageRegistration is only correct in the /currentvehicleswithmileageandinsp (not in /BasicVehicleInformation), we should start using that value everywhere, instead of hardcoding on our side what fueltypes are electric. Then when changes are made that all cars (not only electric) require mileage registration, the change only needs to be applied on SGS side.
Also cleanup so make value is the same on our side. When using /BasicVehicleInformation we need to use make+vehcom, but when using /currentvehicles or /currentvehicleswithmileageandinsp we need to use only make (SGS has already added the vehcom to that field)
Some cleanup/refactor when fetching currentvehicles + info in template-api-modules, when the totalCount of vehicles affects how much information is fetched
Rename field isRequired -> requireMileage in dataSchema, since there was mismatch between the field name, which maybe(?) caused the bug where user was able to go through ownerchange without registering mileage (was not able to reproduce, but hopefully this will minimize the chance of that happening again)
Checklist:
Summary by CodeRabbit