fix(bulk-mileage): update bulk mileage #16539
Conversation
|
Affected services are: api,service-portal,system-e2e, Deployed services: service-portal,api,consultation-portal,web. |
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces several significant changes related to vehicle mileage management, primarily focusing on enhancing error handling and response structures. New interfaces and union types for error responses are created, alongside updates to existing GraphQL mutations and resolvers. Additionally, improvements in file handling for mileage uploads are implemented, including support for multiple file formats. The modifications aim to provide more detailed error information and streamline the user experience when interacting with vehicle mileage data. 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
|
thorkellmani
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Outside diff range and nitpick comments (9)
libs/api/domains/vehicles/src/lib/dto/updateResponseError.dto.ts (2)
1-12: Add JSDoc documentation and improve type definitions.The interface would benefit from proper documentation and some structural improvements:
- Add JSDoc to describe the purpose and usage of this interface
- Consider extracting the error item type for reusability
Here's the suggested improvement:
+/** + * Represents an error response for vehicle mileage update operations. + * @interface UpdateResponseError + */ export interface UpdateResponseError { type: string title: string status: number - Errors: Array<{ - lookupNo: number - warnSever: string - errorMess: string - permno: string - warningSerialNumber: string - }> + errors: Array<VehicleMileageError> } + +/** + * Represents an individual error entry in the mileage update response. + * @interface VehicleMileageError + */ +interface VehicleMileageError { + lookupNo: number + warnSeverity: string + errorMessage: string + permitNumber: string + warningSerialNumber: string +}
1-12: Consider generalizing the error response structure.The current interface seems specific to vehicle mileage updates but follows a common error response pattern. Consider creating a more generic base error response type that can be extended for specific use cases.
Example approach:
/** * Base error response structure for API responses */ export interface BaseErrorResponse { type: string title: string status: number errors: Array<BaseError> } /** * Base error entry structure */ export interface BaseError { message: string code?: string details?: Record<string, unknown> } /** * Vehicle mileage specific error response */ export interface UpdateResponseError extends BaseErrorResponse { errors: Array<VehicleMileageError> }libs/api/domains/vehicles/src/lib/models/v3/vehicleMileageResponseError.model.ts (1)
4-14: Add JSDoc documentation for the error model.Since this is a public API in a library, please add JSDoc documentation to describe the purpose and usage of this error model.
+/** + * Represents an error response for vehicle mileage updates. + * Used in GraphQL mutations to provide structured error information. + */ @ObjectType() export class VehiclesMileageUpdateError { + /** Human-readable error message */ @Field() message!: string + /** Optional error code for client-side handling */ @Field(() => Int, { nullable: true }) code?: number + /** Additional error details in JSON format */ @Field(() => JSON, { nullable: true }) error?: Record<string, unknown> }libs/api/domains/vehicles/src/lib/models/v3/putVehicleMileageResponse.model.ts (1)
5-15: Add type safety and documentation to the union type.While the implementation is correct, consider these improvements for better maintainability and type safety:
Apply this diff to add JSDoc documentation and type safety:
+/** + * Union type for vehicle mileage update responses. + * Represents either a successful update with vehicle data or an error response. + */ export const VehicleMileagePutResponse = createUnionType({ name: 'VehicleMileagePutResponse', types: () => [VehicleMileagePutModel, VehiclesMileageUpdateError] as const, - resolveType(value) { + resolveType(value: { permno?: string } | Record<string, unknown>) { if (value.permno) { return VehicleMileagePutModel } return VehiclesMileageUpdateError }, })libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileage.graphql (1)
22-47: Consider documenting the error codes for API consumers.The enhanced error handling with union types and structured error responses improves the API's robustness. To maximize the benefit of these improvements, consider:
- Documenting the possible error codes and their meanings
- Adding examples of error handling in the component documentation
- Creating shared error handling utilities for components using these mutations
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageFileDownloader.tsx (1)
Line range hint
15-32: Consider improving loading state management and error handlingThe current implementation has several areas for improvement:
- The shared loading state affects both buttons simultaneously
- The error handling could be more specific to different failure scenarios
- The example data is hardcoded
Consider implementing these improvements:
- const [isLoading, setIsLoading] = useState(false) + const [loadingStates, setLoadingStates] = useState<Record<'csv' | 'xlsx', boolean>>({ + csv: false, + xlsx: false + }) - const downloadExampleFile = async (type: 'csv' | 'xlsx') => { - setIsLoading(true) + const downloadMileageTemplate = async (type: 'csv' | 'xlsx') => { + setLoadingStates(prev => ({ ...prev, [type]: true })) try { downloadFile( `magnskraning_kilometrastodu_example`, ['bilnumer', 'kilometrastada'], - [ - ['ABC001', 10000], - ['DEF002', 99999], - ], + MILEAGE_EXAMPLES, type, ) } catch (error) { - onError(error) + onError( + error instanceof Error + ? `Failed to download ${type.toUpperCase()} template: ${error.message}` + : 'Unknown error occurred while downloading template' + ) } finally { - setIsLoading(false) + setLoadingStates(prev => ({ ...prev, [type]: false })) } }Add at the top of the file:
const MILEAGE_EXAMPLES = [ ['ABC001', 10000], ['DEF002', 99999], ] as const;libs/api/domains/vehicles/src/lib/resolvers/mileage.resolver.ts (1)
38-40: Consider aligning version naming across models and methods.There's a version mismatch between V2 method names and V3 model imports. This inconsistency could lead to confusion in the future. Consider either:
- Moving models to v2/ directory to match method versions, or
- Updating method names to V3 to match model versions
libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx (1)
51-53: Consider improving error message formatting.While the function signature and type handling look good, the error message concatenation could be improved for better maintainability.
Consider this improvement:
- `${formatMessage(vehicleMessage.errorWhileProcessing) + error.message} - `, + formatMessage(vehicleMessage.errorWhileProcessing, { error: error.message })This assumes adding an interpolation parameter to the message definition.
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageRow.tsx (1)
Line range hint
1-168: Enhance TypeScript type definitions.To better align with the coding guidelines for library components:
- Consider extracting and exporting the PostStatus type
- Add explicit return type for the component
+type PostStatus = 'initial' | 'posting' | 'post-success' | 'put-success' | 'error'; interface Props { vehicle: VehicleType } -export const VehicleBulkMileageRow = ({ vehicle }: Props) => { +export const VehicleBulkMileageRow = ({ vehicle }: Props): JSX.Element => { const [postStatus, setPostStatus] = useState<PostStatus>('initial') // ... rest of the component
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
- libs/api/domains/vehicles/src/lib/dto/updateResponseError.dto.ts (1 hunks)
- libs/api/domains/vehicles/src/lib/models/v3/postVehicleMileageResponse.model.ts (1 hunks)
- libs/api/domains/vehicles/src/lib/models/v3/putVehicleMileageResponse.model.ts (1 hunks)
- libs/api/domains/vehicles/src/lib/models/v3/vehicleMileageResponseError.model.ts (1 hunks)
- libs/api/domains/vehicles/src/lib/resolvers/mileage.resolver.ts (2 hunks)
- libs/api/domains/vehicles/src/lib/services/vehicles.service.ts (3 hunks)
- libs/service-portal/assets/src/lib/messages.ts (2 hunks)
- libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileage.graphql (1 hunks)
- libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageFileDownloader.tsx (3 hunks)
- libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageRow.tsx (2 hunks)
- libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx (5 hunks)
- libs/service-portal/assets/src/utils/parseFileToMileage.ts (3 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
libs/api/domains/vehicles/src/lib/dto/updateResponseError.dto.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/vehicles/src/lib/models/v3/postVehicleMileageResponse.model.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/vehicles/src/lib/models/v3/putVehicleMileageResponse.model.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/vehicles/src/lib/models/v3/vehicleMileageResponseError.model.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/vehicles/src/lib/resolvers/mileage.resolver.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/vehicles/src/lib/services/vehicles.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/service-portal/assets/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/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileage.graphql (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/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageFileDownloader.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/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageRow.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/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.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/service-portal/assets/src/utils/parseFileToMileage.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/api/domains/vehicles/src/lib/models/v3/vehicleMileageResponseError.model.ts
[error] 2-2: Do not shadow the global "JSON" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (16)
libs/api/domains/vehicles/src/lib/dto/updateResponseError.dto.ts (1)
5-11:⚠️ Potential issueImprove property naming for clarity and consistency.
Several property names could be more descriptive and consistent:
- 'Errors' should be camelCase as per TypeScript conventions
- 'warnSever' appears to be a typo of 'warnSeverity'
- 'errorMess' should be 'errorMessage'
- 'permno' should be 'permitNumber'
Let's verify if these property names are used elsewhere in the codebase:
libs/api/domains/vehicles/src/lib/models/v3/vehicleMileageResponseError.model.ts (1)
1-14: Verify error model integration with vehicle mileage mutations.The implementation looks good and follows NestJS/GraphQL conventions. However, let's verify its integration with the mileage update mutations.
✅ Verification successful
Let me gather more information about how this error model is integrated with the mutations by examining the resolver and response models.
Error model is properly integrated with vehicle mileage mutations
The verification shows that the
VehiclesMileageUpdateErrormodel is correctly integrated:
- It's properly used in both POST and PUT response union types alongside success models
- The resolver correctly handles error instances in mutation responses
- The union types have proper type resolution logic based on the
permnofield🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of VehiclesMileageUpdateError in mutations # Expected: Should find references in mutation return types and error handling # Search for references to this error type in mutation definitions echo "Searching for mutation definitions using this error type..." ast-grep --pattern 'createUnionType({ $$$ types: () => [$$$VehiclesMileageUpdateError$$$] $$$ })' # Search for error handling implementations echo "Searching for error handling implementations..." rg -l "VehiclesMileageUpdateError" --type tsLength of output: 835
Script:
#!/bin/bash # Check the mileage resolver implementation echo "Checking mileage resolver implementation..." rg -A 10 "VehiclesMileageUpdateError" libs/api/domains/vehicles/src/lib/resolvers/mileage.resolver.ts # Check response models echo -e "\nChecking POST response model..." cat libs/api/domains/vehicles/src/lib/models/v3/postVehicleMileageResponse.model.ts echo -e "\nChecking PUT response model..." cat libs/api/domains/vehicles/src/lib/models/v3/putVehicleMileageResponse.model.tsLength of output: 2532
🧰 Tools
🪛 Biome
[error] 2-2: Do not shadow the global "JSON" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
libs/api/domains/vehicles/src/lib/models/v3/postVehicleMileageResponse.model.ts (2)
1-3: LGTM! Clean and necessary imports.The imports are well-organized and properly scoped for the functionality needed.
5-15: Verify the discriminator field usage across related types.The resolveType function uses 'permno' as a discriminator. Let's verify this is consistently used across related types.
✅ Verification successful
Let me try different commands to verify the discriminator field usage.
Let me search for the VehicleMileageDetail type definition to verify the discriminator field.
The discriminator field usage is correctly implemented
The verification confirms that:
VehicleMileageDetailhaspermnoas an optional string fieldVehiclesMileageUpdateErrordoes not havepermnofield, but hasmessage,code, anderrorfields- The union type's
resolveTypefunction correctly usespermnopresence to discriminate between these typesThis implementation ensures type-safe discrimination between successful mileage updates and error responses.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of 'permno' field in related types # Expected: Find all related type definitions to ensure 'permno' is properly defined # Search for type definitions containing 'permno' rg -t typescript "interface.*VehicleMileageDetail|type.*VehicleMileageDetail" -A 5 # Search for other potential discriminator fields in error types rg -t typescript "interface.*Error|type.*Error.*vehicles.*mileage" -A 5Length of output: 228
Script:
#!/bin/bash # Search for VehicleMileageDetail type definition and its properties rg "VehicleMileageDetail" -A 5 # Search for VehiclesMileageUpdateError type definition rg "VehiclesMileageUpdateError" -A 5 # Search for any other files using permno as discriminator rg "permno.*\?" -A 2Length of output: 51973
Script:
#!/bin/bash # Search for VehicleMileageDetail class/type definition ast-grep --pattern 'class VehicleMileageDetail { $$$ }' # Search for VehiclesMileageUpdateError class/type definition ast-grep --pattern 'class VehiclesMileageUpdateError { $$$ }'Length of output: 3202
libs/api/domains/vehicles/src/lib/models/v3/putVehicleMileageResponse.model.ts (2)
1-3: LGTM! Clean and focused imports.The imports are well-organized and properly scoped for the union type implementation.
8-14: Verify the discriminator logic for type resolution.The current implementation assumes that the presence of
permnois sufficient to determine a successful response. Ensure this is a reliable discriminator across all possible response scenarios.Let's check how these types are used in the codebase:
✅ Verification successful
Let me analyze the code further to verify the discriminator logic.
Let me check one more thing to understand the complete type resolution flow.
The discriminator logic using
permnois reliable and consistentThe verification shows that:
- Both success models (
VehicleMileagePutModelandVehicleMileageDetail) havepermnoas an optional field- The error model (
VehiclesMileageUpdateError) has nopermnofield- This pattern is consistently used across both PUT and POST responses
- The discriminator logic aligns with the GraphQL union type resolution pattern where presence of a field determines the concrete type
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the structure of VehicleMileagePutModel and usage of permno # Check the structure of VehicleMileagePutModel ast-grep --pattern 'class VehicleMileagePutModel { $$$ }' # Check for other uses of permno in vehicle-related responses rg -g '*.ts' -g '*.graphql' 'permno'Length of output: 57047
Script:
#!/bin/bash # Check the structure of VehiclesMileageUpdateError and its usage ast-grep --pattern 'class VehiclesMileageUpdateError { $$$ }' # Check for other response models in the same directory fd ".*Response.model.ts" libs/api/domains/vehicles/src/lib/models/v3/Length of output: 1493
Script:
#!/bin/bash # Check the implementation of postVehicleMileageResponse for comparison cat libs/api/domains/vehicles/src/lib/models/v3/postVehicleMileageResponse.model.ts # Check the VehicleMileagePutModel implementation cat libs/api/domains/vehicles/src/lib/models/getVehicleMileage.model.tsLength of output: 2753
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileage.graphql (2)
22-33: Well-structured mutation with improved error handling!The updated mutation properly handles both success and error cases through union types, making it more robust and maintainable.
38-47: Consistent error handling pattern across mutations!The put mutation maintains symmetry with the post mutation's error handling approach, which is a good practice for API consistency.
libs/api/domains/vehicles/src/lib/resolvers/mileage.resolver.ts (2)
105-151: Well-structured implementation with proper error handling.The V2 mutations are well-implemented with:
- Type-safe error handling
- Consistent patterns between post and put operations
- Proper separation of concerns
- Reusable structure following the coding guidelines
The implementation provides a solid foundation for the XLSX upload support.
105-126: 🛠️ Refactor suggestionAdd input validation for XLSX file type.
Since this PR adds XLSX support, consider adding validation for the file type in the input. This could prevent invalid file uploads early in the request lifecycle.
Consider adding file type validation in the input DTO:
@IsIn(['xlsx', 'xls', 'csv']) fileType: string;Add JSDoc comments for the new V2 mutation.
Consider adding documentation to explain:
- The difference between V1 and V2 endpoints
- The new XLSX support feature
- The error handling capabilities
libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx (2)
9-9: LGTM: Import changes align with XLSX support addition.The imports are properly structured and follow the libs architecture guidelines for reusability.
Also applies to: 24-24
182-182: Verify file extension whitelist consistency.The accept prop includes
.xlsx, but we should verify it matches the fileExtensionWhitelist definitions.✅ Verification successful
Let me try a different search pattern to find the file extension whitelist configuration.
Let me try searching without specifying the file type since TypeScript isn't recognized.
Based on the search results, I can now provide a final response since I found both the
fileExtensionWhitelistdefinition and its usage in the codebase.File extension whitelist is consistent with the accept prop
The accept prop
['.csv', '.xlsx']in VehicleBulkMileageUpload.tsx is consistent with the fileExtensionWhitelist defined inlibs/island-ui/core/types.ts:export const fileExtensionWhitelist = { '.csv': 'text/csv', '.xlsx': 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet', // ... }The code also properly validates the uploaded file's type against these whitelisted extensions:
file.type === fileExtensionWhitelist['.csv'] ? 'csv' : file.type === fileExtensionWhitelist['.xlsx'] ? 'xlsx' : undefined🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for fileExtensionWhitelist definition to ensure consistency rg -A 5 "fileExtensionWhitelist.*=.*{" --type typescriptLength of output: 178
Script:
#!/bin/bash # Search for fileExtensionWhitelist with a simpler pattern rg "fileExtensionWhitelist" --type typescript # Also search for any file extension validation or constants rg "allowedFileExtensions|ALLOWED_EXTENSIONS|FILE_EXTENSIONS" --type typescript # Search for any validation related to xlsx or csv files rg "\.(xlsx|csv)" --type typescript -A 2Length of output: 275
Script:
#!/bin/bash # Search for fileExtensionWhitelist rg "fileExtensionWhitelist" # Search for any file extension validation or constants rg "allowedFileExtensions|ALLOWED_EXTENSIONS|FILE_EXTENSIONS" # Search for any validation related to xlsx or csv files rg "\.(xlsx|csv)" -A 2 # Also search specifically in the VehicleBulkMileageUpload directory fd . libs/service-portal/assets/src/screens/VehicleBulkMileageUpload --exec cat {}Length of output: 29098
libs/service-portal/assets/src/lib/messages.ts (2)
975-978: LGTM: Clear and informative error message.The new error message clearly communicates the accepted file types (.csv and .xlsx) to users.
1033-1033: LGTM: Consistent update to accepted file types.The message has been correctly updated to include .xlsx as an accepted file type, maintaining consistency with the error message and PR objectives.
libs/service-portal/assets/src/utils/parseFileToMileage.ts (2)
2-2: Consider optimizing 'xlsx' import for bundle sizeImporting the entire
xlsxlibrary can increase the bundle size and impact application performance.[performance]
Consider importing only the necessary functions from
xlsxor implementing dynamic imports if applicable. This aligns with effective tree-shaking and bundling practices.
14-21: Titles configuration enhances header recognitionThe
vehicleIndexTitleandmileageIndexTitlearrays include Icelandic titles, enhancing flexibility in recognizing headers.
libs/api/domains/vehicles/src/lib/models/v3/vehicleMileageResponseError.model.ts
Outdated
Show resolved
Hide resolved
libs/api/domains/vehicles/src/lib/models/v3/vehicleMileageResponseError.model.ts
Outdated
Show resolved
Hide resolved
libs/api/domains/vehicles/src/lib/models/v3/postVehicleMileageResponse.model.ts
Outdated
Show resolved
Hide resolved
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageFileDownloader.tsx
Outdated
Show resolved
Hide resolved
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageFileDownloader.tsx
Outdated
Show resolved
Hide resolved
Datadog ReportBranch report: ✅ 0 Failed, 4 Passed, 0 Skipped, 2.68s Total Time |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (6)
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageFileDownloader.tsx (2)
Line range hint
15-32: Enhance example data to be more representativeWhile the function correctly implements file type support, consider making the example data more realistic and helpful:
downloadFile( `magnskraning_kilometrastodu_example`, ['bilnumer', 'kilometrastada'], [ - ['ABC001', 10000], - ['DEF002', 99999], + ['AB A12', 10000], // Common format for Icelandic plates + ['CD B34', 25000], + ['EF C56', 99999], // Include more examples ], type, )
Line range hint
27-29: Enhance error handling with specific error messagesConsider providing more specific error messages based on the error type to align with the PR's objective of improving error messaging.
} catch (error) { - onError(error) + const errorMessage = error instanceof Error + ? error.message + : formatMessage(vehicleMessage.genericDownloadError) + onError(errorMessage) } finally {libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx (2)
51-53: Consider enhancing error message specificity.While the error handling is good, consider providing more specific error messages for different scenarios (empty file, parsing error, invalid format) to help users better understand and fix issues.
Example enhancement:
if (!records.length) { - setUploadErrorMessage(formatMessage(vehicleMessage.uploadFailed)) + setUploadErrorMessage(formatMessage(vehicleMessage.emptyFileError)) return }
Line range hint
143-154: Remove duplicate AlertMessage for upload failure.There are two identical AlertMessage components for the upload failure case. Consider removing the duplicate.
- {data?.vehicleBulkMileagePost?.errorMessage && !loading && !error && ( - <AlertMessage - type="warning" - title={formatMessage(vehicleMessage.uploadFailed)} - message={data.vehicleBulkMileagePost.errorMessage} - /> - )}libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageRow.tsx (1)
41-41: Consider enhancing component reusability.While the component follows TypeScript practices, consider:
- Moving state management logic into a custom hook (e.g.,
useVehicleMileage)- Extracting mutation-related logic into a separate service
This would improve reusability across different NextJS apps and make the component more maintainable.Also applies to: 168-171
libs/service-portal/assets/src/utils/parseFileToMileage.ts (1)
73-74: Remove unnecessary initialization ofparsedLinesThe variable
parsedLinesis initialized but immediately reassigned with the parsed result. The initial assignment is unnecessary and can be removed to clean up the code.Apply this diff to remove the unnecessary initialization:
- let parsedLines: Array<Array<string>> = [[]] let accumulatedChunk = '' let done = false while (!done) { const res = await reader.read() done = res.done if (!done) { accumulatedChunk += decoder.decode(res.value) } } - parsedLines = parseCsvString(accumulatedChunk) + const parsedLines = parseCsvString(accumulatedChunk) return parsedLinesAlso applies to: 86-86
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
- libs/api/domains/vehicles/src/lib/models/v3/postVehicleMileageResponse.model.ts (1 hunks)
- libs/api/domains/vehicles/src/lib/models/v3/putVehicleMileageResponse.model.ts (1 hunks)
- libs/api/domains/vehicles/src/lib/models/v3/vehicleMileageResponseError.model.ts (1 hunks)
- libs/api/domains/vehicles/src/lib/resolvers/mileage.resolver.ts (2 hunks)
- libs/api/domains/vehicles/src/lib/services/vehicles.service.ts (3 hunks)
- libs/service-portal/assets/src/lib/messages.ts (3 hunks)
- libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageFileDownloader.tsx (3 hunks)
- libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageRow.tsx (3 hunks)
- libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx (5 hunks)
- libs/service-portal/assets/src/utils/parseFileToMileage.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- libs/api/domains/vehicles/src/lib/models/v3/postVehicleMileageResponse.model.ts
- libs/api/domains/vehicles/src/lib/models/v3/putVehicleMileageResponse.model.ts
- libs/api/domains/vehicles/src/lib/models/v3/vehicleMileageResponseError.model.ts
- libs/api/domains/vehicles/src/lib/resolvers/mileage.resolver.ts
🧰 Additional context used
📓 Path-based instructions (6)
libs/api/domains/vehicles/src/lib/services/vehicles.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/service-portal/assets/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/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageFileDownloader.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/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageRow.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/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.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/service-portal/assets/src/utils/parseFileToMileage.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/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageFileDownloader.tsx
[error] 37-45: 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 (12)
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageFileDownloader.tsx (1)
1-1: Tree-shaking optimization comment already existslibs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx (2)
9-9: LGTM: Import changes align with XLSX support.The new imports support the enhanced file type handling capabilities.
Also applies to: 24-24
184-184: LGTM: File type support properly updated.The accept prop correctly reflects the supported file types.
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageRow.tsx (2)
28-34: LGTM: Well-defined type for mutation status.The MutationStatus type definition improves type safety and maintainability by clearly defining all possible mutation states.
168-171: Previous review comment about useEffect dependencies is still applicable.libs/api/domains/vehicles/src/lib/services/vehicles.service.ts (3)
14-14: LGTM: New imports are well-organized and properly typedThe imports appropriately support the new error handling functionality and follow TypeScript best practices.
Also applies to: 22-22, 38-41, 44-44, 52-53
473-514: Verify the correctness of status code handlingThe condition
res.raw.status === 200is used to determine if a mileage reading already exists, suggesting that aPUTrequest should be used instead. Typically, a200 OKstatus indicates a successful operation. Please confirm whether a200status correctly signifies that the mileage reading already exists, or if a different status code (such as409 Conflict) should be checked.#!/bin/bash # Search for similar status code handling patterns in the codebase rg -A 2 "raw\.status === 200" --type ts
516-547: Refactor duplicated code in mileage reading methodsThe
postMileageReadingV2andputMileageReadingV2methods contain similar logic for authorization checks and error handling. To improve maintainability and reduce duplication, consider refactoring shared logic into a helper method.Example refactoring:
private async handleMileageReadingRequest<T>( auth: User, input: RootPostRequest['postMileageReadingModel'] | RootPutRequest['putMileageReadingModel'], operation: (auth: User, input: any) => Promise<T>, ): Promise<T | VehiclesMileageUpdateError | null> { if (!input) return null; const isAllowed = await this.isAllowedMileageRegistration(auth, input.permno); if (!isAllowed) { this.logger.error(UNAUTHORIZED_OWNERSHIP_LOG, { category: LOG_CATEGORY, error: 'Mileage reading operation failed', }); throw new ForbiddenException(UNAUTHORIZED_OWNERSHIP_LOG); } try { const result = await operation(auth, input); return result; } catch (e) { if (e instanceof FetchError && (e.status === 400 || e.status === 429)) { const errorBody = e.body as UpdateResponseError; return { code: e.status, message: errorBody.Errors?.[0]?.errorMess || 'Unknown error', }; } else throw e; } }Then update the methods:
async postMileageReadingV2( auth: User, input: RootPostRequest['postMileageReadingModel'], ): Promise<PostMileageReadingModel | VehiclesMileageUpdateError | null> { return this.handleMileageReadingRequest(auth, input, async (auth, input) => { const res = await this.getMileageWithAuth(auth).rootPostRaw({ postMileageReadingModel: input }); if (res.raw.status === 200) { this.logger.info('Tried to post already existing mileage reading. Should use PUT'); return null; } return res.value(); }); } async putMileageReadingV2( auth: User, input: RootPutRequest['putMileageReadingModel'], ): Promise<MileageReadingDto | VehiclesMileageUpdateError | null> { return this.handleMileageReadingRequest(auth, input, (auth, input) => this.getMileageWithAuth(auth).rootPut({ putMileageReadingModel: input }), ); }libs/service-portal/assets/src/lib/messages.ts (3)
975-978: LGTM: Error message for wrong file type.The message definition follows the established pattern and clearly communicates the supported file types.
1033-1033: LGTM: Updated accepted file types message.The message is correctly updated to include both .csv and .xlsx formats, aligning with the new feature support.
1071-1071: LGTM: Template download message with dynamic argument.The message correctly uses the {arg} placeholder for dynamic template type insertion, following the established pattern for parameterized messages.
libs/service-portal/assets/src/utils/parseFileToMileage.ts (1)
1-120: Overall code quality is goodThe implementation effectively handles parsing of both CSV and XLSX files, utilizing TypeScript features and maintaining consistency with coding guidelines. The code is well-structured and aligns with best practices for reusability and type safety.
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageFileDownloader.tsx
Outdated
Show resolved
Hide resolved
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageFileDownloader.tsx
Outdated
Show resolved
Hide resolved
libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx
Outdated
Show resolved
Hide resolved
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageRow.tsx
Outdated
Show resolved
Hide resolved
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageRow.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageFileDownloader.tsx (1)
30-38: Remove duplicate title propThe title is specified twice through
menuLabelandtitleprops. Consider removing one of them as they serve the same purpose.<DropdownMenu icon="ellipsisHorizontal" menuLabel={formatMessage(vehicleMessage.downloadTemplate)} items={(['csv', 'xlsx'] as const).map((type) => ({ title: `.${type}`, onClick: () => downloadExampleFile(type), }))} - title={formatMessage(vehicleMessage.downloadTemplate)} />libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx (1)
Line range hint
56-82: Consider enhancing error handling structure.While the error handling is comprehensive, it could be more maintainable with a clearer structure.
Consider this refactoring:
const postMileage = async (file: File, type: 'xlsx' | 'csv') => { try { const records = await parseFileToMileageRecord(file, type) - if (!records.length) { - setUploadErrorMessage(formatMessage(vehicleMessage.uploadFailed)) - return - } - if (typeof records === 'string') { - setUploadErrorMessage(records) - return - } + if (!Array.isArray(records) || !records.length) { + throw new Error( + typeof records === 'string' + ? records + : formatMessage(vehicleMessage.uploadFailed) + ) + } vehicleBulkMileagePostMutation({ variables: { input: { mileageData: records.map((r) => ({ mileageNumber: r.mileage, vehicleId: r.vehicleId, })), originCode: 'ISLAND.IS', }, }, }) } catch (error) { setUploadErrorMessage( - `${formatMessage(vehicleMessage.errorWhileProcessing) + error.message} - `, + formatMessage(vehicleMessage.errorWhileProcessing) + error.message.trim() ) } }This refactoring:
- Simplifies the validation logic
- Unifies error handling in the catch block
- Removes string template literal whitespace
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- libs/service-portal/assets/src/lib/messages.ts (2 hunks)
- libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageFileDownloader.tsx (1 hunks)
- libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx (5 hunks)
- libs/service-portal/assets/src/utils/parseFileToMileage.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/service-portal/assets/src/lib/messages.ts
🧰 Additional context used
📓 Path-based instructions (3)
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageFileDownloader.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/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.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/service-portal/assets/src/utils/parseFileToMileage.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/service-portal/assets/src/utils/parseFileToMileage.ts (1)
Learnt from: thorkellmani PR: island-is/island.is#16539 File: libs/service-portal/assets/src/utils/parseFileToMileage.ts:103-117 Timestamp: 2024-10-25T16:01:02.482Z Learning: In the `parseFileToMileage.ts` file, prefer using `csv-parse` over `papaparse` for CSV parsing in the Service Portal Assets.
🪛 Biome
libs/service-portal/assets/src/utils/parseFileToMileage.ts
[error] 109-109: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (5)
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageFileDownloader.tsx (1)
30-38: Well-implemented dropdown for file type selection!The DropdownMenu implementation is clean, type-safe, and provides a good user experience for selecting file types. The use of const assertion with
as constensures type safety for the file type options.libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx (3)
9-9: LGTM! Well-structured type definitions and imports.The implementation follows best practices by:
- Using TypeScript's const assertions for type safety
- Defining the extension mapping outside the component for better performance
- Following the modular import structure
Also applies to: 24-29
Line range hint
133-182: Verify error message handling from the service.The PR objective mentions parsing error messages from the service. Let's verify the implementation of error message display.
Let's check the error message structure:
#!/bin/bash # Description: Verify error message structure in GraphQL schema # Expected: Find the error message field definition in the response type ast-grep --pattern 'type VehicleBulkMileagePost { $$$ errorMessage: String $$$ }'
185-185: Verify file extension whitelist alignment.The accept prop includes
.xlsx, which should align with the fileExtensionWhitelist imported from core.Let's verify the whitelist configuration:
✅ Verification successful
File extension whitelist is properly configured
The code shows that the
.xlsxextension is correctly defined in both the accept prop and the fileExtensionWhitelist mapping:[fileExtensionWhitelist['.xlsx']]: 'xlsx'The accept prop and whitelist are properly aligned, ensuring consistent file type validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify file extension whitelist configuration # Expected: Find the whitelist definition that includes .xlsx rg -A 5 "fileExtensionWhitelist.*=.*{" --type typescriptLength of output: 89
Script:
#!/bin/bash # Let's try different search patterns to find the whitelist configuration rg -i "whitelist.*xlsx" --type typescript # Also search for file extension related constants or configurations rg -i "allowedExtensions|fileExtensions|acceptedExtensions|extensions.*xlsx" --type typescript # Search specifically in core or config related files fd -e ts -e tsx config fd -e ts -e tsx core | xargs rg -l -i "extension"Length of output: 40348
Script:
#!/bin/bash # Let's search for file extension related code in the service-portal core and assets rg -A 5 "fileExtension|accept.*xlsx" libs/service-portal/core libs/service-portal/assets # Also search for any file upload related components or utilities ast-grep --pattern 'interface $_ { $$$ accept?: $_ $$$ }'Length of output: 8770
libs/service-portal/assets/src/utils/parseFileToMileage.ts (1)
1-108: Code adheres to coding guidelinesThe code is well-structured, utilizes TypeScript effectively for defining types, and follows best practices for reusability and bundling in a library context.
Also applies to: 110-125
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageFileDownloader.tsx
Show resolved
Hide resolved
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageFileDownloader.tsx
Show resolved
Hide resolved
libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
libs/service-portal/assets/src/utils/parseFileToMileage.ts (2)
19-63: LGTM! Consider adding input validation.The function handles both formats well and includes thorough header validation. Consider adding validation for the input file size and empty files.
Add size validation before processing:
if (file.size === 0) { throw new Error('File is empty'); } if (file.size > MAX_FILE_SIZE) { throw new Error('File size exceeds limit'); }
83-101: LGTM! Consider adding sheet selection option.The XLSX parsing implementation is robust with proper error handling. Consider adding an optional parameter for sheet selection instead of always using the first sheet.
interface XlsxOptions { sheetIndex?: number; } const parseXlsx = async (file: File, options: XlsxOptions = {}) => { const sheetIndex = options.sheetIndex ?? 0; // ... use sheetIndex instead of 0 }libs/service-portal/assets/src/screens/VehicleBulkMileageJobDetail/VehicleBulkMileageJobDetail.tsx (1)
232-256: LGTM! The conditional rendering change improves code clarity.The ternary operator implementation for conditional rendering is more explicit and aligns with React best practices. The code effectively displays error messages which aligns with the PR objective of showing more detailed error information.
Consider extracting the table row rendering logic into a separate component for better reusability:
type VehicleRowProps = { vehicleId: string returnCode: string mileage: number errors?: Array<{ message: string }> } const VehicleRow: React.FC<VehicleRowProps> = ({ vehicleId, returnCode, mileage, errors = [] }) => ( <T.Row> <T.Data> <Box display="flex" justifyContent="spaceBetween"> <Icon icon={returnCode !== 'E' ? 'checkmark' : 'warning'} color={returnCode !== 'E' ? 'mint400' : 'red400'} /> {vehicleId} </Box> </T.Data> <T.Data>{displayWithUnit(mileage, 'km', true)}</T.Data> <T.Data>{errors.map((e) => e.message).join(', ')}</T.Data> </T.Row> )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- libs/service-portal/assets/src/screens/VehicleBulkMileageJobDetail/VehicleBulkMileageJobDetail.tsx (1 hunks)
- libs/service-portal/assets/src/utils/parseFileToMileage.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
libs/service-portal/assets/src/screens/VehicleBulkMileageJobDetail/VehicleBulkMileageJobDetail.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/service-portal/assets/src/utils/parseFileToMileage.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/service-portal/assets/src/utils/parseFileToMileage.ts (1)
Learnt from: thorkellmani PR: island-is/island.is#16539 File: libs/service-portal/assets/src/utils/parseFileToMileage.ts:103-117 Timestamp: 2024-10-25T16:01:02.482Z Learning: In the `parseFileToMileage.ts` file, prefer using `csv-parse` over `papaparse` for CSV parsing in the Service Portal Assets.
🪛 Biome
libs/service-portal/assets/src/utils/parseFileToMileage.ts
[error] 115-115: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (3)
libs/service-portal/assets/src/utils/parseFileToMileage.ts (3)
5-8: LGTM! Well-defined TypeScript interface.The
MileageRecordinterface is well-structured and follows TypeScript best practices, making it reusable across different NextJS apps.
65-81: LGTM! Browser-compatible implementation.The CSV parsing implementation correctly uses TextDecoder and handles streaming appropriately.
115-117:⚠️ Potential issueImprove readability by separating assignment.
The assignment within the while condition can be made more readable.
- while ((record = parser.read()) !== null) { - records.push(record) - } + record = parser.read() + while (record !== null) { + records.push(record) + record = parser.read() + }Likely invalid or redundant comment.
🧰 Tools
🪛 Biome
[error] 115-115: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx (1)
Line range hint 56-77: Consider improving error handling and separation of concerns.
The postMileage function currently handles both file parsing and data submission. Consider:
- Using TypeScript discriminated unions for better error handling
- Separating the parsing and submission logic into distinct functions
type ParseResult =
| { success: true; records: MileageRecord[] }
| { success: false; error: string };
const parseFile = async (file: File, type: 'xlsx' | 'csv'): Promise<ParseResult> => {
try {
const records = await parseFileToMileageRecord(file, type);
if (!records.length) {
return { success: false, error: formatMessage(vehicleMessage.uploadFailed) };
}
if (typeof records === 'string') {
return { success: false, error: records };
}
return { success: true, records };
} catch (error) {
return {
success: false,
error: `${formatMessage(vehicleMessage.errorWhileProcessing)}${error.message}`
};
}
};
const postMileage = async (file: File, type: 'xlsx' | 'csv') => {
const result = await parseFile(file, type);
if (!result.success) {
setUploadErrorMessage(result.error);
return;
}
vehicleBulkMileagePostMutation({
variables: {
input: {
mileageData: result.records.map((r) => ({
mileageNumber: r.mileage,
vehicleId: r.vehicleId,
})),
originCode: 'ISLAND.IS',
},
},
});
};📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- libs/api/domains/vehicles/src/lib/services/bulkMileage.service.ts (3 hunks)
- libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx (5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
libs/api/domains/vehicles/src/lib/services/bulkMileage.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/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (4)
libs/api/domains/vehicles/src/lib/services/bulkMileage.service.ts (2)
16-16: LGTM: Clean import addition for error handling.
The named import of FetchError follows TypeScript best practices and maintains tree-shaking capabilities.
Line range hint 37-72: LGTM: Robust error handling implementation.
The try-catch block with specific handling for rate limiting (429) and guid validation provides comprehensive error management while maintaining type safety.
libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx (2)
24-24: LGTM! Good separation of concerns.
The file parsing logic is properly extracted into a separate utility function, making it reusable across different components and improving maintainability.
178-178: LGTM! File type support properly updated.
The InputFileUpload component's accept prop now correctly includes both CSV and XLSX formats, aligning with the PR's objective to support XLSX file uploads.
libs/api/domains/vehicles/src/lib/services/bulkMileage.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
libs/api/domains/vehicles/src/lib/models/v3/bulkMileage/bulkMileageReadingResponse.model.ts (1)
11-13: Consider documenting error scenarios.
Since both requestId and errorMessage are optional, it would be helpful to document the scenarios when each field would be null/undefined. This helps consumers of the API understand the expected behavior.
@Field({ nullable: true })
+ // Present when the request fails with a specific error message
errorMessage?: string📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- libs/api/domains/vehicles/src/lib/models/v3/bulkMileage/bulkMileageReadingResponse.model.ts (1 hunks)
- libs/api/domains/vehicles/src/lib/services/bulkMileage.service.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/api/domains/vehicles/src/lib/services/bulkMileage.service.ts
🧰 Additional context used
📓 Path-based instructions (1)
libs/api/domains/vehicles/src/lib/models/v3/bulkMileage/bulkMileageReadingResponse.model.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 (1)
libs/api/domains/vehicles/src/lib/models/v3/bulkMileage/bulkMileageReadingResponse.model.ts (1)
6-10: LGTM! Type safety improvements look good.
The changes to make requestId optional both in TypeScript and GraphQL schema are consistent and align well with the error handling improvements.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (2)
libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.graphql (1)
5-5: Consider documenting error codes.
The addition of errorCode improves error handling capabilities. Consider documenting the possible error codes and their meanings in the schema or documentation to help consumers handle errors appropriately.
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageTable.tsx (1)
Line range hint 1-57: Enhance component documentation
While the component follows TypeScript practices, it would benefit from additional documentation explaining its purpose and usage, especially given its location in the libs directory where reusability is important.
Add JSDoc comments to describe the component's purpose and props:
+/**
+ * A table component for displaying and managing vehicle mileage data in bulk.
+ * Used in the service portal for updating mileage information for multiple vehicles.
+ */
interface Props {
+ /** Array of vehicles to display in the table */
vehicles: Array<VehicleType>
+ /** Loading state to show/hide the empty table message */
loading: boolean
}📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- libs/api/domains/vehicles/src/lib/models/v3/bulkMileage/bulkMileageReadingResponse.model.ts (1 hunks)
- libs/api/domains/vehicles/src/lib/services/bulkMileage.service.ts (3 hunks)
- libs/service-portal/assets/src/lib/messages.ts (3 hunks)
- libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageRow.tsx (8 hunks)
- libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageTable.tsx (1 hunks)
- libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.graphql (1 hunks)
- libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/api/domains/vehicles/src/lib/models/v3/bulkMileage/bulkMileageReadingResponse.model.ts
- libs/api/domains/vehicles/src/lib/services/bulkMileage.service.ts
- libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx
🧰 Additional context used
📓 Path-based instructions (4)
libs/service-portal/assets/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/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageRow.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/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageTable.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/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.graphql (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/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.graphql (1)
Line range hint 1-7: Verify error handling implementation.
Let's verify that the error code handling is consistently implemented across the codebase.
libs/service-portal/assets/src/lib/messages.ts (4)
877-880: LGTM: New validation message follows TypeScript and i18n best practices.
The message is properly typed, follows the namespace convention, and maintains consistency with other validation messages.
883-883: LGTM: Message text improvement.
The updated message is more concise while maintaining clarity.
889-893: LGTM: Rate limiting message follows best practices.
The message provides clear feedback and actionable guidance to users when they exceed the upload rate limit.
1042-1042: LGTM: File type message is consistent.
The message correctly lists both accepted file types (.csv and .xlsx).
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageTable.tsx
Outdated
Show resolved
Hide resolved
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageRow.tsx
Show resolved
Hide resolved
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageRow.tsx
Show resolved
Hide resolved
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageRow.tsx
Show resolved
Hide resolved
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageRow.tsx
Show resolved
Hide resolved
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageRow.tsx
Show resolved
Hide resolved
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageRow.tsx
Show resolved
Hide resolved
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageRow.tsx
Outdated
Show resolved
Hide resolved
thorkellmani
added
the
deprecated:automerge
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageRow.tsx
Show resolved
Hide resolved
libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx
Show resolved
Hide resolved
…date' into fix/bulk-milegae-update
* fix: add xlsx * fix: support xlsx * feat: parse and display errors * fix: coderabbit grilling session * chore: prettier button * chore: remove imports * chore: more rabbit grilling * chore: switch in csv-pasre * fix: parse * fix: handle 429 * chore: update message * fix: use service error * feat: fix reveiw errors * fix: standard error handling * fix: bunny comments * feat: add search --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
What
Why
Checklist:
Summary by CodeRabbit
New Features
.csvand.xlsxformats.Bug Fixes
Documentation
Refactor