feat(bulk-mileage): update errors #16720
Conversation
WalkthroughThe pull request introduces several modifications across multiple files in the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used📓 Path-based instructions (1)libs/api/domains/vehicles/src/lib/services/vehicles.service.ts (1)Pattern
🔇 Additional comments (3)libs/api/domains/vehicles/src/lib/services/vehicles.service.ts (3)
The import changes align with the TypeScript usage guidelines and reflect the modifications in the service methods.
The changes address the previous review concerns by adding proper null checking when accessing the array element.
The changes maintain consistency with putMileageReading while providing enhanced error handling within the try-catch block. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16720 +/- ##
==========================================
+ Coverage 36.53% 36.55% +0.01%
==========================================
Files 6892 6889 -3
Lines 143920 143796 -124
Branches 40997 40971 -26
==========================================
- Hits 52586 52568 -18
+ Misses 91334 91228 -106
Flags with carried forward coverage won't be shown. Click here to find out more. see 11 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 3 Total Test Services: 0 Failed, 3 Passed Test Services
|
libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (18)
libs/api/domains/vehicles/src/lib/models/v3/bulkMileage/bulkMileageRegistrationRequestError.model.ts (2)
11-12: Add JSDoc comment to document the warning serial code field.The field is correctly typed using GraphQL's
Inttype, but adding documentation would improve maintainability.+ /** Serial code indicating the specific type of warning encountered during bulk mileage registration. */ @Field(() => Int, { nullable: true }) warningSerialCode?: number
14-15: Add JSDoc comment to document the warning text field.For consistency with the warning serial code field, add documentation to explain the purpose of this field.
+ /** Human-readable warning message that provides details about the warning encountered. */ @Field({ nullable: true }) warningText?: stringlibs/api/domains/vehicles/project.json (1)
18-22: Consider making the string extraction more reusable.To improve reusability across different domains, consider extracting the command configuration into a shared nx executor or a workspace generator.
Example approach:
- "extract-strings": { - "executor": "nx:run-commands", - "options": { - "command": "yarn ts-node -P libs/localization/tsconfig.lib.json libs/localization/scripts/extract 'libs/api/domains/vehicles/src/lib/messages.ts'" - } - } + "extract-strings": { + "executor": "@island-is/nx-extract-strings", + "options": { + "messagesPath": "src/lib/messages.ts" + } + }libs/api/domains/vehicles/src/lib/messages.ts (2)
1-3: Consider enhancing type safety for message definitions.The implementation looks good but could benefit from explicit typing for better maintainability and type safety.
Consider adding type definitions:
import { defineMessages } from 'react-intl' + type MessageKeys = keyof typeof m + export type VehicleMessages = Record<MessageKeys, string> export const m = defineMessages({
4-82: Message definitions are comprehensive but could benefit from organization and documentation.The message definitions cover all necessary error scenarios and follow a good structure. However, there are a few suggestions for improvement:
- Consider grouping related messages with JSDoc comments for better documentation:
/** * Validation Messages * Messages related to input validation errors */ tooManyPermno: { id: 'api.bulk-vehicle-mileage:too-many-permno', defaultMessage: 'Sama fastanúmer birtist oft í skjali', },
- Consider making message IDs more consistent by following a stricter pattern:
// Current: api.bulk-vehicle-mileage:too-many-permno // Suggested: api.vehicles.bulk-mileage.validation.too-many-permno
- Consider adding a description field to aid translators:
mileageTooLow: { id: 'api.bulk-vehicle-mileage:mileage-too-low', defaultMessage: 'Km staða getur ekki verið minna en 0', + description: 'Error shown when user enters negative mileage value', },libs/service-portal/assets/src/utils/parseFileToMileage.ts (3)
10-13: Consider using named constants for error codesWhile the interface is well-defined, the numeric literals (1 | 2) would be more maintainable as named constants.
+export const enum MileageErrorCode { + INVALID_VEHICLE_HEADER = 1, + INVALID_MILEAGE_HEADER = 2 +} export interface MileageError { - code: 1 | 2 + code: MileageErrorCode message: string }
24-31: Strengthen type safety for errorMapThe current type
Record<number, string>is too permissive. Consider using the error code union type for stronger type safety.-export const errorMap: Record<number, string> = { +export const errorMap: Record<MileageErrorCode, string> = { - 1: `Invalid vehicle column header. Must be one of the following: ${vehicleIndexTitle.join( + [MileageErrorCode.INVALID_VEHICLE_HEADER]: `Invalid vehicle column header. Must be one of the following: ${vehicleIndexTitle.join( ', ', )}`, - 2: `Invalid mileage column header. Must be one of the following: ${mileageIndexTitle.join( + [MileageErrorCode.INVALID_MILEAGE_HEADER]: `Invalid mileage column header. Must be one of the following: ${mileageIndexTitle.join( ', ', )}`, }
Line range hint
33-61: Standardize error handling approachWhile the function has moved to returning errors instead of throwing them for header validation, the XLSX parsing still throws errors. Consider standardizing the error handling approach across all cases.
const parseXlsx = async (file: File) => { - try { - //FIRST SHEET ONLY - const buffer = await file.arrayBuffer() - const parsedFile = XLSX.read(buffer, { type: 'buffer' }) - - const jsonData = XLSX.utils.sheet_to_csv( - parsedFile.Sheets[parsedFile.SheetNames[0]], - { - blankrows: false, - }, - ) - - return parseCsvString(jsonData) - } catch (e) { - throw new Error('Failed to parse XLSX file: ' + e.message) - } + const buffer = await file.arrayBuffer() + const parsedFile = XLSX.read(buffer, { type: 'buffer' }) + + const jsonData = XLSX.utils.sheet_to_csv( + parsedFile.Sheets[parsedFile.SheetNames[0]], + { + blankrows: false, + }, + ) + + return parseCsvString(jsonData) }libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileage.tsx (1)
88-124: Consider extracting nested content into a separate componentThe nested content within IntroWrapper (buttons, table, and pagination) could be extracted into a separate component to improve readability and maintainability.
Example refactor:
interface VehicleBulkMileageContentProps { loading: boolean; error?: Error; vehicles: Array<VehicleType>; page: number; totalPages: number; onPageChange: (page: number) => void; } const VehicleBulkMileageContent: React.FC<VehicleBulkMileageContentProps> = ({ loading, error, vehicles, page, totalPages, onPageChange, }) => ( <Stack space={4}> <Inline space={2}> <LinkButton to={AssetsPaths.AssetsVehiclesBulkMileageUpload} text={formatMessage(vehicleMessage.bulkPostMileage)} icon="upload" variant="utility" /> <LinkButton to={AssetsPaths.AssetsVehiclesBulkMileageJobOverview} text={formatMessage(vehicleMessage.jobOverview)} icon="receipt" variant="utility" /> </Inline> {error && !loading && <Problem error={error} noBorder={false} />} {!error && <VehicleBulkMileageTable loading={loading} vehicles={vehicles} />} {totalPages > 1 && ( <Pagination page={page} totalPages={totalPages} renderLink={(page, className, children) => ( <button aria-label={formatMessage(m.goToPage)} onClick={() => onPageChange(page)} > <span className={className}>{children}</span> </button> )} /> )} </Stack> );libs/service-portal/assets/src/screens/VehicleBulkMileageJobOverview/VehicleBulkMileageJobOverview.tsx (1)
67-70: Optimize array initialization and sorting.While the current implementation works correctly, we can optimize it further by avoiding unnecessary spread operations.
Consider this more efficient approach:
- const sortedJobs = [...jobs] - if (sortedJobs.length > 1) { - sortedJobs.sort((a, b) => sortJobs(a, b)) - } + const sortedJobs = jobs.length <= 1 + ? jobs + : [...jobs].sort((a, b) => sortJobs(a, b))This change:
- Avoids unnecessary array spread when sorting isn't needed
- Maintains the same functionality
- Improves code readability
libs/api/domains/vehicles/src/lib/services/bulkMileage.service.ts (1)
156-158: Consider extracting the magic numberThe fallback value of 999 for warningSerial should be defined as a named constant to improve maintainability and clarity.
+const DEFAULT_WARNING_SERIAL = 999; const warningSerial = - e.warningSerial === -1 ? 999 : e.warningSerial + e.warningSerial === -1 ? DEFAULT_WARNING_SERIAL : e.warningSeriallibs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx (1)
59-78: Consider simplifying error handling logic.The current implementation uses multiple if-else statements for error handling. This could be simplified using a switch statement or an error mapping object for better maintainability.
Consider refactoring to:
- if (records.code === 1) { - setUploadErrorMessage(formatMessage(vehicleMessage.invalidPermNoColumn)) - } else if (records.code === 2) { - setUploadErrorMessage( - formatMessage(vehicleMessage.invalidMileageColumn), - ) - } else { - setUploadErrorMessage(formatMessage(vehicleMessage.uploadFailed)) - } + const errorMessages = { + 1: vehicleMessage.invalidPermNoColumn, + 2: vehicleMessage.invalidMileageColumn, + default: vehicleMessage.uploadFailed + } + setUploadErrorMessage( + formatMessage(errorMessages[records.code] || errorMessages.default) + )libs/service-portal/assets/src/screens/VehicleBulkMileageJobDetail/VehicleBulkMileageJobDetail.tsx (1)
Line range hint
107-134: LGTM: Well-structured IntroWrapper implementation with a type safety suggestion.The IntroWrapper implementation is clean and properly internationalized. However, we could improve type safety for the button group's onClick handler.
Consider extracting the refresh handler to a typed callback:
+const handleRefreshClick = React.useCallback(() => { + handleRefresh() +}, [handleRefresh]) buttonGroup={ <Box marginTop={'containerGutter'}> <Button variant="utility" icon={loading || networkStatus === NetworkStatus.refetch ? undefined : 'reload'} loading={loading || networkStatus === NetworkStatus.refetch} - onClick={handleRefresh} + onClick={handleRefreshClick} >libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageRow.tsx (2)
25-25: Remove unused importThe
isDefinedutility is imported but not used in the component.-import { isDefined } from '@island.is/shared/utils'
214-248: Good use of memoization, but can be improvedThe memoization implementation is good for performance, but there are a few improvements that could be made:
- Remove commented out code
- Make the implementation more concise using array methods
Here's a more concise implementation:
const nestedTable = useMemo(() => { if (!data?.vehiclesMileageRegistrationHistory) { return [[]] } const tableData: Array<Array<string>> = [[]] - if (data?.vehiclesMileageRegistrationHistory?.lastMileageRegistration) { - tableData.push([ - formatDate( - data.vehiclesMileageRegistrationHistory.lastMileageRegistration.date, - ), - data.vehiclesMileageRegistrationHistory.lastMileageRegistration - .originCode, - //'-', - displayWithUnit( - data.vehiclesMileageRegistrationHistory.lastMileageRegistration - .mileage, - 'km', - true, - ), - ]) - } - for (const mileageRegistration of data?.vehiclesMileageRegistrationHistory - ?.mileageRegistrationHistory ?? []) { - if (mileageRegistration) { - tableData.push([ - formatDate(mileageRegistration.date), - mileageRegistration.originCode, - //'-', - displayWithUnit(mileageRegistration.mileage, 'km', true), - ]) - } - } + const { lastMileageRegistration, mileageRegistrationHistory = [] } = + data.vehiclesMileageRegistrationHistory + + const formatRegistration = (registration: typeof lastMileageRegistration) => [ + formatDate(registration.date), + registration.originCode, + displayWithUnit(registration.mileage, 'km', true), + ] + + if (lastMileageRegistration) { + tableData.push(formatRegistration(lastMileageRegistration)) + } + + mileageRegistrationHistory + .filter(Boolean) + .forEach(registration => + tableData.push(formatRegistration(registration)) + ) return tableData }, [data?.vehiclesMileageRegistrationHistory])libs/api/domains/vehicles/src/lib/services/vehicles.service.ts (1)
Line range hint
1-644: Consider extracting common validation logic.Several methods share similar validation patterns:
- Feature flag checking
- Authorization validation
- Mileage registration permission checking
Consider extracting these into reusable decorators or guard methods to improve maintainability and reduce code duplication.
Example implementation:
// Create a decorator for feature flag checking const RequiresMileageFeatureFlag = () => { return function ( target: any, propertyKey: string, descriptor: PropertyDescriptor, ) { const originalMethod = descriptor.value; descriptor.value = async function (...args: any[]) { const auth = args[0]; // Assuming first argument is always auth const featureFlagOn = await this.featureFlagService.getValue( Features.servicePortalVehicleMileagePageEnabled, false, auth, ); if (!featureFlagOn) { return null; } return originalMethod.apply(this, args); }; return descriptor; }; }; // Create a method for mileage registration validation private async validateMileageRegistration(auth: User, permno: string): Promise<void> { await this.hasVehicleServiceAuth(auth, permno); const isAllowed = await this.isAllowedMileageRegistration(auth, permno); if (!isAllowed) { this.logger.error(UNAUTHORIZED_OWNERSHIP_LOG, { category: LOG_CATEGORY, error: 'mileage registration validation failed', }); throw new ForbiddenException(UNAUTHORIZED_OWNERSHIP_LOG); } }libs/clients/vehicles-mileage/src/clientConfig.json (1)
Line range hint
1-1199: General API specification review.The OpenAPI specification is well-structured but has several breaking changes that need careful handling:
- Version updates
- Base URL changes
- Response schema modifications
Recommendations:
- Implement semantic versioning
- Consider using API versioning in the URL (e.g., /v2/vehicle/mileagereading)
- Document migration guides for breaking changes
- Consider implementing a deprecation policy
libs/service-portal/assets/src/lib/messages.ts (1)
1001-1010: Consider making column name matching case-insensitive.While the error messages are clear and helpful by listing all acceptable column names, the case-sensitive matching might cause confusion for users. Consider making the column name matching case-insensitive to improve user experience.
Example implementation:
- 'Fastanúmersdálk vantar eða er skrifaður rangt. Dálkanafn þarf að vera eitt af eftirfarandi; "permno", "vehicleid", "bilnumer","okutaeki","fastanumer"', + 'Fastanúmersdálk vantar eða er skrifaður rangt. Dálkanafn þarf að vera eitt af eftirfarandi (ekki er gerður greinarmunur á há- og lágstöfum); "permno", "vehicleid", "bilnumer","okutaeki","fastanumer"', - 'Kílómetrastöðudálk vantar eða er skrifaður rangt. Dálkanafn þarf að vera eitt af eftirfarandi; "kilometrastada", "mileage", "odometer"', + 'Kílómetrastöðudálk vantar eða er skrifaður rangt. Dálkanafn þarf að vera eitt af eftirfarandi (ekki er gerður greinarmunur á há- og lágstöfum); "kilometrastada", "mileage", "odometer"',
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (18)
libs/api/domains/vehicles/project.json(1 hunks)libs/api/domains/vehicles/src/lib/dto/getBulkVehicleMileageRequestOverview.input.ts(1 hunks)libs/api/domains/vehicles/src/lib/messages.ts(1 hunks)libs/api/domains/vehicles/src/lib/models/v3/bulkMileage/bulkMileageRegistrationRequestError.model.ts(2 hunks)libs/api/domains/vehicles/src/lib/resolvers/bulkMileage.resolver.ts(1 hunks)libs/api/domains/vehicles/src/lib/services/bulkMileage.service.ts(3 hunks)libs/api/domains/vehicles/src/lib/services/errorCodes.ts(1 hunks)libs/api/domains/vehicles/src/lib/services/vehicles.service.ts(3 hunks)libs/api/domains/vehicles/src/lib/vehicles.module.ts(2 hunks)libs/clients/vehicles-mileage/src/clientConfig.json(3 hunks)libs/service-portal/assets/src/lib/messages.ts(3 hunks)libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileage.tsx(3 hunks)libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageRow.tsx(4 hunks)libs/service-portal/assets/src/screens/VehicleBulkMileageJobDetail/VehicleBulkMileageJobDetail.graphql(1 hunks)libs/service-portal/assets/src/screens/VehicleBulkMileageJobDetail/VehicleBulkMileageJobDetail.tsx(8 hunks)libs/service-portal/assets/src/screens/VehicleBulkMileageJobOverview/VehicleBulkMileageJobOverview.tsx(3 hunks)libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx(4 hunks)libs/service-portal/assets/src/utils/parseFileToMileage.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (18)
libs/api/domains/vehicles/project.json (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/api/domains/vehicles/src/lib/dto/getBulkVehicleMileageRequestOverview.input.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/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/api/domains/vehicles/src/lib/models/v3/bulkMileage/bulkMileageRegistrationRequestError.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/bulkMileage.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/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/api/domains/vehicles/src/lib/services/errorCodes.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/api/domains/vehicles/src/lib/vehicles.module.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/clients/vehicles-mileage/src/clientConfig.json (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/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.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/VehicleBulkMileageJobDetail/VehicleBulkMileageJobDetail.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/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/screens/VehicleBulkMileageJobOverview/VehicleBulkMileageJobOverview.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."
🔇 Additional comments (32)
libs/api/domains/vehicles/src/lib/dto/getBulkVehicleMileageRequestOverview.input.ts (2)
2-2: LGTM! Good use of type-only import.
The type-only import of Locale ensures effective tree-shaking during bundling.
6-7: Verify locale handling in the service layer.
The locale field is properly typed and decorated for GraphQL. However, we should verify that the service layer correctly handles all possible locale values.
✅ Verification successful
Let me analyze the verification needed for locale handling in the service layer.
Let me try another search pattern to find the service implementation and locale handling.
Locale handling is properly implemented in the service layer
The service implementation in bulkMileage.service.ts correctly handles the locale parameter by:
- Accepting locale as a typed parameter (Locale type)
- Using the locale parameter with the intl service for message formatting
- Passing it through to the underlying API calls
The implementation follows the proper patterns for internationalization and locale handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify locale handling implementation in the service layer
# Expected: Find the service implementation that handles the locale parameter
# Search for the bulk mileage service implementation
ast-grep --pattern 'class $_ implements BulkMileageService {
$$$
getBulkMileageRegistrationRequestOverview($_) {
$$$
}
$$$
}'
# Search for locale validation or transformation logic
rg -A 5 "locale.*Locale" --type ts
Length of output: 288696
Script:
#!/bin/bash
# Search for the bulk mileage service implementation and locale handling
ast-grep --pattern 'class $_ implements BulkMileageService {
$$$
getBulkMileageRegistrationRequestOverview($input) {
$$$
}
$$$
}'
# Search for locale validation or transformation logic in the service
rg -A 5 "getBulkMileageRegistrationRequestOverview.*locale" --type ts
Length of output: 215
Script:
#!/bin/bash
# Search for bulk mileage service implementation with different pattern
ast-grep --pattern 'class $_ {
$$$
async getBulkMileageRegistrationRequestOverview($_) {
$$$
}
$$$
}'
# Search for locale usage in the bulk mileage related files
rg -l "BulkMileage.*Service" --type ts | xargs rg -A 5 "locale.*Locale"
# Search for the input DTO usage
rg -A 5 "GetbulkmileagereadingrequeststatusGuidGetRequest" --type ts
Length of output: 3064
libs/api/domains/vehicles/src/lib/models/v3/bulkMileage/bulkMileageRegistrationRequestError.model.ts (1)
Line range hint 1-17: LGTM! The model follows best practices.
The implementation:
- ✓ Uses proper TypeScript types and GraphQL decorators
- ✓ Maintains consistency with existing error handling patterns
- ✓ Is properly structured for reuse across the application
Let's verify the usage of this model across the codebase:
libs/service-portal/assets/src/screens/VehicleBulkMileageJobDetail/VehicleBulkMileageJobDetail.graphql (2)
21-22: LGTM! Enhanced error handling structure.
The addition of warningSerialCode and warningText fields improves error reporting capabilities by providing more structured and detailed warning information.
21-22: Verify consistent error handling implementation.
Let's ensure these new warning fields are consistently implemented across the codebase.
✅ Verification successful
The verification results show that the warning fields (warningSerialCode and warningText) are consistently implemented across the codebase:
- The fields are properly defined in the GraphQL model (
BulkMileageRegistrationRequestError) - The service implementation (
bulkMileage.service.ts) correctly maps and formats these fields - The UI components properly consume and display these fields in both table view and CSV export
- The error handling pattern is consistent with other parts of the codebase that use warning messages
Warning fields are properly implemented and consistent across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of warning fields across related files
# Check for warning field usage in TypeScript models
echo "Checking TypeScript models for warning fields..."
rg -t ts "warningSerialCode|warningText" -A 5 -B 5
# Check for warning field usage in GraphQL schemas
echo "Checking other GraphQL schemas for warning fields..."
rg -g "*.graphql" "warningSerialCode|warningText" -A 5 -B 5
# Check for error handling patterns in services
echo "Checking service implementations..."
ast-grep --pattern 'class $_ implements $_ {
$$$
warning$_($$$) {
$$$
}
$$$
}'
Length of output: 12710
libs/api/domains/vehicles/src/lib/services/errorCodes.ts (1)
5-5: Clarify the error code transformation logic
The comment about transforming -1 to 999 suggests there's some error handling logic elsewhere in the codebase. This transformation should be documented more clearly and possibly centralized.
Let's search for related error handling code:
✅ Verification successful
Let me gather more context about the error handling implementation:
Error code transformation is properly implemented and documented
The transformation of -1 to 999 is clearly implemented in bulkMileage.service.ts where it's used as a fallback for invalid array indexing. The error codes are properly mapped in errorCodes.ts and used consistently across the codebase for warning messages. The current implementation and documentation is sufficient.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for places where error code -1 is transformed to 999
rg -A 3 "(-1.*999|999.*-1)" libs/api/domains/vehicles
Length of output: 848
Script:
#!/bin/bash
# Look for the error handling interface/type definitions
ast-grep --pattern 'interface $_Warning {
$$$
warningSerial: $_
$$$
}'
# Also check for any other usages of these error codes
rg -A 3 "warningSerial|errorCode" libs/api/domains/vehicles
Length of output: 3908
libs/api/domains/vehicles/project.json (1)
18-22: LGTM! Verify the localization script path.
The addition of the extract-strings target for i18n support looks good and aligns with the PR's error handling improvements.
Let's verify the existence of the localization script and related files:
✅ Verification successful
The localization setup is correctly configured
The verification confirms that:
- The extraction script exists and is properly implemented to handle message extraction
- The messages.ts file is present in the correct location
- The tsconfig.lib.json exists with proper Node.js types configuration
- The command structure in project.json matches the script's expected usage (it takes a file path as an argument)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of localization files and their consistency
# Check if the localization script exists
fd -t f "extract" "libs/localization/scripts"
# Verify the messages file exists
fd -t f "messages.ts" "libs/api/domains/vehicles/src/lib"
# Check tsconfig exists
fd -t f "tsconfig.lib.json" "libs/localization"
Length of output: 306
Script:
#!/bin/bash
# Let's verify the script's content to ensure it's executable and matches the command usage
cat libs/localization/scripts/extract.ts
# Also check the tsconfig to verify the configuration is correct
cat libs/localization/tsconfig.lib.json
Length of output: 4501
libs/api/domains/vehicles/src/lib/vehicles.module.ts (2)
14-14: LGTM! Clean import statement.
The import follows TypeScript best practices and uses module aliasing effectively.
30-30: LGTM! Good integration of internationalization support.
The addition of CmsTranslationsModule to the imports array properly enables internationalization capabilities for vehicle-related features, which aligns with the module's evolution to support locale-specific error messages and content.
libs/api/domains/vehicles/src/lib/resolvers/bulkMileage.resolver.ts (1)
69-71: LGTM! Verify locale handling in the service layer.
The addition of the locale parameter follows GraphQL resolver best practices and maintains proper separation of concerns by delegating the locale handling to the service layer.
Let's verify the locale handling implementation in the service layer:
✅ Verification successful
Locale handling is properly implemented in the service layer ✅
The service correctly:
- Accepts the
localeparameter of typeLocale - Uses the
IntlServicewith proper dependency injection - Utilizes the locale parameter in
useIntl()for message formatting - Applies translations to warning messages using the provided locale
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the locale handling implementation in BulkMileageService
# Check if the service properly handles the locale parameter
ast-grep --pattern 'getBulkMileageRegistrationRequestOverview(user: User, locale: $_) {
$$$
}'
# Check for proper IntlService usage with the locale parameter
rg -A 5 'IntlService.*locale'
Length of output: 130
Script:
#!/bin/bash
# Let's try different approaches to verify the locale handling
# 1. Search for the service method with a more flexible pattern
ast-grep --pattern 'getVehicleMileageRegistrationRequestOverview($$$locale$$$) {
$$$
}'
# 2. Find the service file first
fd "bulk.*service\.ts$"
# 3. Look for any locale-related code in vehicle or mileage services
rg -l "locale.*getVehicleMileageRegistrationRequestOverview"
# 4. Check for IntlService imports and usage
rg -l "IntlService.*import"
Length of output: 276
Script:
#!/bin/bash
# Now that we found the service file, let's examine its content
cat libs/api/domains/vehicles/src/lib/services/bulkMileage.service.ts
# Also search for any locale-related code in this specific file
rg "locale" libs/api/domains/vehicles/src/lib/services/bulkMileage.service.ts
Length of output: 5843
libs/service-portal/assets/src/utils/parseFileToMileage.ts (1)
121-122: LGTM! Robust CSV parsing options
The addition of skipLinesWithEmptyValues and trim options improves the parsing robustness by handling empty lines and whitespace.
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileage.tsx (1)
Line range hint 65-87: LGTM! IntroWrapper implementation looks good
The component properly implements the IntroWrapper with all required props:
- title and introComponent for content
- serviceProviderSlug and tooltip for provider information
- Proper TypeScript usage and internationalization
libs/service-portal/assets/src/screens/VehicleBulkMileageJobOverview/VehicleBulkMileageJobOverview.tsx (3)
10-11: LGTM! Import follows tree-shaking practices.
The addition of IntroWrapper import is well-organized and follows the module's import structure.
73-78: LGTM! Improved component structure with IntroWrapper.
The replacement of Box with IntroWrapper:
- Enhances component reusability across NextJS apps
- Provides better structure with standardized intro section
- Properly implements TypeScript props
Also applies to: 147-147
73-78: Verify consistent IntroWrapper usage across components.
Let's ensure this component replacement is consistently applied across related components.
✅ Verification successful
IntroWrapper usage is consistent across the codebase
The verification shows that IntroWrapper is being used consistently across multiple components in the service portal, including:
- Vehicle-related screens (Bulk Mileage, Job Overview, Job Detail)
- Education screens (Graduation, Graduation Detail)
- Occupational Licenses screens (Overview, Detail)
The component is properly exported from service-portal/core and other components like IntroHeader and FootNote are marked as deprecated in favor of IntroWrapper. No inconsistent Box component usage was found for intro sections.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining Box components that might need to be replaced with IntroWrapper
ast-grep --pattern 'import { $$$, Box, $$$ } from "@island.is/island-ui/core"'
# Search for IntroWrapper usage patterns
ast-grep --pattern 'import { $$$, IntroWrapper, $$$ } from "@island.is/service-portal/core"'
Length of output: 175
Script:
#!/bin/bash
# Search for IntroWrapper usage with more context
rg "IntroWrapper" -A 3 -B 1
# Search for Box components that might be related to intro sections
rg "Box.*title.*intro" -A 3 -B 1
# Search for imports containing IntroWrapper
rg "import.*IntroWrapper"
Length of output: 17659
libs/api/domains/vehicles/src/lib/services/bulkMileage.service.ts (3)
10-10: LGTM! Clean import additions for i18n support
The new imports are well-structured and follow TypeScript best practices, supporting proper tree-shaking.
Also applies to: 18-21
27-27: LGTM! Clean dependency injection
The IntlService is properly injected following Angular DI patterns with appropriate access modifiers.
135-138: Verify the locale parameter usage
The method now requires a locale parameter. Ensure that all callers of this method have been updated to provide the locale.
✅ Verification successful
Let me try a different approach to find the method calls. The previous ast-grep pattern might have failed due to syntax specifics, so I'm using ripgrep to first locate the files containing the method name and then show the actual usage context.
Method call has been properly updated with locale parameter
The verification shows that the only caller of getBulkMileageRegistrationRequestOverview is in bulkMileage.resolver.ts, and it correctly provides the locale parameter from the input object (input.locale). No issues were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to getBulkMileageRegistrationRequestOverview
ast-grep --pattern 'getBulkMileageRegistrationRequestOverview($$$)'
Length of output: 69
Script:
#!/bin/bash
# Search for calls to getBulkMileageRegistrationRequestOverview using ripgrep
rg -l "getBulkMileageRegistrationRequestOverview" --type ts
# Show the actual calls with context
rg "getBulkMileageRegistrationRequestOverview" --type ts -B 2 -A 2
Length of output: 1200
libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx (2)
13-18: LGTM! Import statements follow best practices.
The imports are well-organized and support effective tree-shaking. The component renaming from IntroHeader to IntroWrapper is properly reflected in the imports.
130-140: LGTM! Component usage follows best practices.
The migration from IntroHeader to IntroWrapper is implemented correctly, maintaining proper prop types and component reusability across NextJS apps.
libs/service-portal/assets/src/screens/VehicleBulkMileageJobDetail/VehicleBulkMileageJobDetail.tsx (4)
16-16: LGTM: Import changes are well-organized.
The addition of IntroWrapper import is properly grouped with other imports from the same package.
37-37: LGTM: Proper implementation of locale support.
The locale extraction and its usage in the GraphQL query are implemented correctly, supporting internationalization requirements.
Also applies to: 60-60
253-255: LGTM: Type-safe error message display.
The error message display implementation properly handles undefined cases and maintains consistency with the updated error field naming.
93-93: Verify consistency of error message field usage.
The change from using message to warningText for error messages needs to be verified across other components using the same error structure.
✅ Verification successful
Change from message to warningText is specific to bulk mileage feature
The change is correct and consistent within its domain. The search results show that:
warningTextis specifically used in the bulk mileage feature, defined in the vehicle domain models and used consistently in related components- Other parts of the codebase use different error message patterns appropriate for their specific features (e.g.,
messagein occupational licenses, mapped error codes in parental leave)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of error message fields to ensure consistency
rg -A 2 "errors\?\.map.*message" "libs/"
rg -A 2 "errors\?\.map.*warningText" "libs/"
Length of output: 85
Script:
#!/bin/bash
# Search for error-related patterns more broadly
rg -A 2 "errors.*map" "libs/"
# Search for warningText usage
rg -A 2 "warningText" "libs/"
# Search for specific error mapping patterns in TypeScript/JavaScript files
fd "\.(ts|tsx|js|jsx)$" libs/ -x rg -A 2 "errors.*map"
Length of output: 7702
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageRow.tsx (2)
400-400: LGTM!
Good use of the memoized value for the NestedFullTable data prop.
Line range hint 1-403: Component follows library coding guidelines
The component properly implements:
- Reusability across NextJS apps with no app-specific dependencies
- TypeScript for props and type definitions
- Tree-shakeable exports and imports
libs/api/domains/vehicles/src/lib/services/vehicles.service.ts (1)
36-36: LGTM! Import cleanup improves tree-shaking.
The removal of unused imports while retaining only the necessary ones enhances bundle optimization.
libs/clients/vehicles-mileage/src/clientConfig.json (3)
12-12:
New base URL path affects API routing.
The addition of a new servers section with the URL "/vehicle/mileagereading" changes the base path for all endpoints. This is a breaking change that requires updates in API clients.
#!/bin/bash
# Check for existing URL references that might need updates
rg -l "/vehicle/mileagereading"5-5: Major version update requires communication with API consumers.
The API version has been updated from Release-6 to Release-21, which is a significant jump. This change might impact API consumers and should be properly communicated.
Consider implementing API versioning in the URL path or using content negotiation to maintain backward compatibility during such major version updates.
795-799: Enhanced error tracking with warning serial.
Good addition of the warningSerial property to improve error tracking capabilities. This enhancement allows for better error identification and debugging.
✅ Verification successful
Let me gather more context about the error handling patterns and message formatting.
Let me check the error codes mapping to understand the complete warning serial implementation.
Warning serial implementation is properly integrated with error handling system
The warningSerial property is correctly integrated into the error handling system:
- It's mapped to predefined error messages in
errorCodeMessageMap - Special handling for -1 code by transforming it to 999
- Properly used in
bulkMileage.service.tsfor formatting error messages - Consistent with the existing error tracking patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent error handling patterns
ast-grep --pattern 'warningSerial'
Length of output: 398
Script:
#!/bin/bash
# Check the implementation around warningSerial usage
ast-grep --pattern 'errorCodeMessageMap'
# Check for related error handling patterns
rg "formatMessage.*warning" -A 2
Length of output: 2281
Script:
#!/bin/bash
# Check the error codes implementation
cat libs/api/domains/vehicles/src/lib/services/errorCodes.ts
# Check the bulk mileage service implementation around warning serial
rg -B 5 -A 5 "warningSerial" libs/api/domains/vehicles/src/lib/services/bulkMileage.service.ts
Length of output: 1535
libs/service-portal/assets/src/lib/messages.ts (2)
221-225: LGTM! Clear error message for invalid file types.
The message clearly indicates which file types are accepted (.xlsx and .csv).
989-992: LGTM! Clear error message for empty files.
The message clearly indicates that the uploaded file contains no data.
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileage.tsx
Show resolved
Hide resolved
libs/api/domains/vehicles/src/lib/services/bulkMileage.service.ts
Outdated
Show resolved
Hide resolved
libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx
Outdated
Show resolved
Hide resolved
…d/VehicleBulkMileageUpload.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (20)
libs/api/domains/vehicles/src/lib/models/v3/bulkMileage/bulkMileageRegistrationRequestError.model.ts (3)
11-12: Consider adding field documentationThe field implementation is correct, using proper GraphQL and TypeScript types. Consider adding a description to the
@Fielddecorator to document the purpose of this warning serial code.- @Field(() => Int, { nullable: true }) + @Field(() => Int, { + nullable: true, + description: 'Serial code identifying the specific warning type' + }) warningSerialCode?: number
14-15: Consider adding field documentationThe field implementation is correct. Consider adding a description to the
@Fielddecorator to document the purpose of this warning text field.- @Field({ nullable: true }) + @Field({ + nullable: true, + description: 'Human-readable warning message' + }) warningText?: string
Line range hint
1-17: Well-structured error model implementationThe class follows best practices for GraphQL object types and TypeScript usage. It's properly structured for reusability across different NextJS apps and follows the single responsibility principle by focusing solely on error representation.
Consider creating a shared documentation file (e.g.,
ERROR_CODES.md) to maintain a catalog of all possible warning serial codes and their meanings, which would help maintain consistency across the application.libs/api/domains/vehicles/project.json (1)
18-22: Consider adding a description comment for the target.To improve maintainability, consider adding a comment explaining the purpose of this string extraction target and when it should be run.
"test": { "executor": "@nx/jest:jest", "outputs": ["{workspaceRoot}/coverage/libs/api/domains/vehicles"], "options": { "jestConfig": "libs/api/domains/vehicles/jest.config.ts" } }, + // Extracts translatable strings from messages.ts for internationalization "extract-strings": { "executor": "nx:run-commands", "options": { "command": "yarn ts-node -P libs/localization/tsconfig.lib.json libs/localization/scripts/extract 'libs/api/domains/vehicles/src/lib/messages.ts'" } }libs/api/domains/vehicles/src/lib/resolvers/bulkMileage.resolver.ts (1)
Line range hint
1-89: Consider centralizing i18n configurationSince this resolver is part of a shared library (
libs/**/*), consider implementing a centralized i18n configuration that can be reused across different NextJS applications. This could involve:
- Creating a shared i18n configuration module
- Implementing locale detection middleware
- Using a higher-order wrapper for consistent locale handling
This would improve maintainability and ensure consistent internationalization behavior across the platform.
libs/api/domains/vehicles/src/lib/messages.ts (1)
3-83: Consider enhancing type safety and maintainability.While the implementation is functional, consider these improvements:
- Add type safety with explicit message type definitions
- Group related messages (e.g., validation, authorization, etc.)
- Define message IDs as constants to prevent typos
Here's a suggested refactor:
import { defineMessages } from 'react-intl' + +// Message ID constants +const MESSAGE_PREFIX = 'api.bulk-vehicle-mileage' as const + +// Message type definitions +type MessageId = `${typeof MESSAGE_PREFIX}:${string}` + +interface VehicleMileageMessage { + id: MessageId + defaultMessage: string +} + +// Group related messages +const validationMessages = { tooManyPermno: { - id: 'api.bulk-vehicle-mileage:too-many-permno', + id: `${MESSAGE_PREFIX}:too-many-permno`, defaultMessage: 'Sama fastanúmer birtist oft í skjali', }, // ... other validation messages } as const + +const authorizationMessages = { + unauthorizedUpdater: { + id: `${MESSAGE_PREFIX}:unauthorized-updater`, + defaultMessage: + 'Tilkynnandi eða innsendur tilkynnandi er hvorki umráðamaður né eigandi ökutækis', + }, + // ... other authorization messages +} as const + +// Export all message groups +export const m = defineMessages({ + ...validationMessages, + ...authorizationMessages, +}) satisfies Record<string, VehicleMileageMessage>libs/service-portal/assets/src/utils/parseFileToMileage.ts (4)
10-13: Consider using an enum for error codesThe current union type
1 | 2for error codes is limiting. Using an enum would provide better extensibility and type safety.+export enum MileageErrorCode { + InvalidVehicleHeader = 1, + InvalidMileageHeader = 2 +} export interface MileageError { - code: 1 | 2 + code: MileageErrorCode message: string }
121-122: Consider logging skipped linesWhile skipping empty lines and trimming values improves robustness, it might silently ignore data issues. Consider logging skipped lines for debugging purposes.
const parser = parse({ delimiter: [';', ','], skipLinesWithEmptyValues: true, trim: true, + onSkipLine: (line) => console.debug('Skipped line:', line), })
145-145: Optimize regex patternThe regex pattern could be compiled once instead of being created on each function call.
+const NUMBER_CLEANUP_PATTERN = new RegExp(/[.,]/g) -const sanitizeNumber = (n: string) => n.replace(new RegExp(/[.,]/g), '') +const sanitizeNumber = (n: string) => n.replace(NUMBER_CLEANUP_PATTERN, '')
Line range hint
33-73: Consider making the parser more reusableAs this is in the
libsdirectory, consider making the parser more generic to support different types of file parsing scenarios across the application.Consider extracting the parsing logic into a more generic utility:
interface ColumnMapping<T> { field: keyof T; headers: string[]; transform?: (value: string) => any; } function parseFile<T>( file: File, mappings: ColumnMapping<T>[], options?: ParseOptions ): Promise<T[] | ParseError>libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileage.tsx (1)
109-122: Consider memoizing the pagination click handler.The click handler for pagination is recreated on every render. Consider memoizing it with useCallback for better performance.
+ const handlePageChange = useCallback((newPage: number) => { + setPage(newPage) + }, []) <Pagination page={page} totalPages={totalPages} renderLink={(page, className, children) => ( <button aria-label={formatMessage(m.goToPage)} - onClick={() => { - setPage(page) - }} + onClick={() => handlePageChange(page)} > <span className={className}>{children}</span> </button> )} />libs/service-portal/assets/src/screens/VehicleBulkMileageJobOverview/VehicleBulkMileageJobOverview.tsx (2)
67-70: Consider using structured clone for deep copyingWhile using the spread operator for creating a copy is good for preventing mutations, it only creates a shallow copy. If the jobs contain nested objects that influence sorting, consider using
structuredClonefor a deep copy:- const sortedJobs = [...jobs] + const sortedJobs = structuredClone(jobs)The current implementation is functional but could be more robust with this change.
Line range hint
54-54: Add explicit return type for better type safetyConsider adding an explicit return type to the component for better type safety and documentation:
- const VehicleBulkMileageUploadJobOverview = () => { + const VehicleBulkMileageUploadJobOverview = (): JSX.Element => {libs/api/domains/vehicles/src/lib/services/bulkMileage.service.ts (2)
156-158: Document the warningSerial replacement logicThe replacement of
-1with999needs documentation explaining the business logic behind this transformation.Add a comment explaining this special case:
+ // Special case: -1 is mapped to 999 for legacy compatibility const warningSerial = e.warningSerial === -1 ? 999 : e.warningSerial
155-167: Consider simplifying the error mapping logicThe error mapping could be simplified using object destructuring and optional chaining.
Consider this more concise version:
- errors: d.errors?.map((e) => { - const warningSerial = - e.warningSerial === -1 ? 999 : e.warningSerial - - return { - code: e.errorCode ?? undefined, - message: e.errorText ?? undefined, - warningSerialCode: e.warningSerial ?? 0, - warningText: warningSerial - ? formatMessage(errorCodeMessageMap[warningSerial ?? 0]) - : undefined, - } - }), + errors: d.errors?.map(({ errorCode, errorText, warningSerial }) => { + const normalizedSerial = warningSerial === -1 ? 999 : warningSerial; + return { + code: errorCode ?? undefined, + message: errorText ?? undefined, + warningSerialCode: warningSerial ?? 0, + warningText: normalizedSerial && errorCodeMessageMap[normalizedSerial] + ? formatMessage(errorCodeMessageMap[normalizedSerial]) + : undefined, + }; + }),libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx (1)
57-90: Consider refactoring error handling for better maintainabilityThe error handling logic could be simplified using a more TypeScript-idiomatic approach.
Consider this refactor to improve readability and type safety:
const postMileage = async (file: File, type: 'xlsx' | 'csv') => { const records = await parseFileToMileageRecord(file, type) - if (!Array.isArray(records)) { - if (records.code === 1) { - setUploadErrorMessage(formatMessage(vehicleMessage.invalidPermNoColumn)) - } else if (records.code === 2) { - setUploadErrorMessage( - formatMessage(vehicleMessage.invalidMileageColumn), - ) - } else { - setUploadErrorMessage(formatMessage(vehicleMessage.uploadFailed)) - } - return + const errorMessages = { + 1: vehicleMessage.invalidPermNoColumn, + 2: vehicleMessage.invalidMileageColumn, + default: vehicleMessage.uploadFailed + } + + if (!Array.isArray(records)) { + const message = records.code ? errorMessages[records.code] : errorMessages.default + setUploadErrorMessage(formatMessage(message)) + return } if (!records.length) { setUploadErrorMessage(formatMessage(vehicleMessage.noDataInUploadedFile)) return } - if (typeof records === 'string') { - setUploadErrorMessage(records) - return - }libs/service-portal/assets/src/screens/VehicleBulkMileageJobDetail/VehicleBulkMileageJobDetail.tsx (1)
Line range hint
107-134: LGTM with a suggestion for loading state textThe IntroWrapper implementation is well-structured with proper internationalization and good UX through the refresh button functionality.
Consider adding loading state text to improve user feedback:
<Text>{formatMessage(vehicleMessage.refreshDataAboutJob)}</Text> +{(loading || networkStatus === NetworkStatus.refetch) && ( + <Text variant="small" color="blue400"> + {formatMessage(vehicleMessage.refreshingData)} + </Text> +)}libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageRow.tsx (1)
214-248: Improve table data transformation logicWhile the memoization is correctly implemented, there are a few improvements that could be made:
Consider this more concise implementation:
const nestedTable = useMemo(() => { if (!data?.vehiclesMileageRegistrationHistory) { - return [[]] + return [] } - const tableData: Array<Array<string>> = [[]] + const tableData: Array<Array<string>> = [] if (data?.vehiclesMileageRegistrationHistory?.lastMileageRegistration) { tableData.push([ formatDate( data.vehiclesMileageRegistrationHistory.lastMileageRegistration.date, ), data.vehiclesMileageRegistrationHistory.lastMileageRegistration .originCode, - //'-', displayWithUnit( data.vehiclesMileageRegistrationHistory.lastMileageRegistration .mileage, 'km', true, ), ]) } - for (const mileageRegistration of data?.vehiclesMileageRegistrationHistory - ?.mileageRegistrationHistory ?? []) { - if (mileageRegistration) { - tableData.push([ - formatDate(mileageRegistration.date), - mileageRegistration.originCode, - //'-', - displayWithUnit(mileageRegistration.mileage, 'km', true), - ]) - } - } + const historyRows = (data?.vehiclesMileageRegistrationHistory?.mileageRegistrationHistory ?? []) + .filter(isDefined) + .map(registration => [ + formatDate(registration.date), + registration.originCode, + displayWithUnit(registration.mileage, 'km', true), + ]) + tableData.push(...historyRows) return tableData }, [data?.vehiclesMileageRegistrationHistory])Changes:
- Removed unnecessary empty array initialization
- Removed commented out code
- Used functional approach with filter and map for better readability
- Leveraged the imported
isDefinedutility for type safetylibs/api/domains/vehicles/src/lib/services/vehicles.service.ts (2)
468-472: Add null check before array accessWhile the TypeScript type suggests the array should always be present, adding a null check would make the code more robust against unexpected API responses.
const dtos = await this.getMileageWithAuth(auth).rootPut({ putMileageReadingModel: input, }) - return dtos[0] + return dtos?.length ? dtos[0] : null
537-540: Add null check and approve error handlingThe error handling for status codes 400 and 429 is well implemented. Consider adding a null check for the response array.
const dtos = await this.getMileageWithAuth(auth).rootPut({ putMileageReadingModel: input, }) - return dtos[0] + return dtos?.length ? dtos[0] : null
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (18)
libs/api/domains/vehicles/project.json(1 hunks)libs/api/domains/vehicles/src/lib/dto/getBulkVehicleMileageRequestOverview.input.ts(1 hunks)libs/api/domains/vehicles/src/lib/messages.ts(1 hunks)libs/api/domains/vehicles/src/lib/models/v3/bulkMileage/bulkMileageRegistrationRequestError.model.ts(2 hunks)libs/api/domains/vehicles/src/lib/resolvers/bulkMileage.resolver.ts(1 hunks)libs/api/domains/vehicles/src/lib/services/bulkMileage.service.ts(3 hunks)libs/api/domains/vehicles/src/lib/services/errorCodes.ts(1 hunks)libs/api/domains/vehicles/src/lib/services/vehicles.service.ts(3 hunks)libs/api/domains/vehicles/src/lib/vehicles.module.ts(2 hunks)libs/clients/vehicles-mileage/src/clientConfig.json(3 hunks)libs/service-portal/assets/src/lib/messages.ts(3 hunks)libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileage.tsx(3 hunks)libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageRow.tsx(4 hunks)libs/service-portal/assets/src/screens/VehicleBulkMileageJobDetail/VehicleBulkMileageJobDetail.graphql(1 hunks)libs/service-portal/assets/src/screens/VehicleBulkMileageJobDetail/VehicleBulkMileageJobDetail.tsx(8 hunks)libs/service-portal/assets/src/screens/VehicleBulkMileageJobOverview/VehicleBulkMileageJobOverview.tsx(3 hunks)libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx(4 hunks)libs/service-portal/assets/src/utils/parseFileToMileage.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (18)
libs/api/domains/vehicles/project.json (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/api/domains/vehicles/src/lib/dto/getBulkVehicleMileageRequestOverview.input.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/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/api/domains/vehicles/src/lib/models/v3/bulkMileage/bulkMileageRegistrationRequestError.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/bulkMileage.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/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/api/domains/vehicles/src/lib/services/errorCodes.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/api/domains/vehicles/src/lib/vehicles.module.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/clients/vehicles-mileage/src/clientConfig.json (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/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.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/VehicleBulkMileageJobDetail/VehicleBulkMileageJobDetail.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/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/screens/VehicleBulkMileageJobOverview/VehicleBulkMileageJobOverview.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."
🔇 Additional comments (36)
libs/api/domains/vehicles/src/lib/dto/getBulkVehicleMileageRequestOverview.input.ts (2)
2-2: LGTM! Good use of type-only import.
The type-only import for Locale ensures optimal tree-shaking in the final bundle.
6-7: LGTM! Verify the locale field usage.
The locale field is properly typed and decorated for GraphQL. Let's verify its usage across the codebase to ensure consistent implementation.
✅ Verification successful
Let me try different search patterns to verify the locale field usage.
Locale field implementation is correctly used across the codebase
The verification shows that the locale field is properly implemented and used:
- It's correctly passed from GraphQL query to resolver in
bulkMileage.resolver.ts - The resolver forwards it to the bulk service method
- The field is used in the frontend GraphQL query for job registrations
- Type safety is maintained throughout the implementation chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of locale field in bulk mileage related files
# Check for GraphQL queries using this input type
echo "Checking GraphQL queries..."
rg -t graphql "BulkVehicleMileageRequestOverviewInput.*locale"
# Check for resolver implementations
echo "Checking resolver implementations..."
ast-grep --pattern 'getVehicleMileageRegistrationRequestOverview($input: BulkVehicleMileageRequestOverviewInput!)'
# Check for service method implementations
echo "Checking service implementations..."
ast-grep --pattern 'getBulkMileageRegistrationRequestOverview.*locale'
Length of output: 515
Script:
#!/bin/bash
# Search for the input type definition and its usages
echo "Checking input type definition and usages..."
rg "BulkVehicleMileageRequestOverviewInput" -A 5 -B 5
# Search for resolver methods that might use this input
echo "Checking resolver methods..."
rg "getVehicleMileageRegistrationRequestOverview" -A 5 -B 5
# Search specifically for locale field usage in relevant files
echo "Checking locale field usage..."
fd "vehicle.*mileage" --type f --exec rg "locale" {} \;
Length of output: 6827
libs/api/domains/vehicles/src/lib/models/v3/bulkMileage/bulkMileageRegistrationRequestError.model.ts (1)
1-1: LGTM! Import statement is correctly updated
The addition of the Int type from @nestjs/graphql is appropriate for the new warningSerialCode field.
libs/service-portal/assets/src/screens/VehicleBulkMileageJobDetail/VehicleBulkMileageJobDetail.graphql (1)
21-22: LGTM! Verify error handling implementation.
The addition of warningSerialCode and warningText fields enhances error reporting capabilities. The structure is well-organized and maintains backward compatibility.
Let's verify the implementation of these new error fields:
libs/api/domains/vehicles/src/lib/services/errorCodes.ts (2)
1-2: LGTM! Imports are well-structured
The imports are correctly defined and support effective tree-shaking.
4-24: Consider improving error code organization and documentation
While the implementation is functional, consider these improvements for better maintainability:
- Use TypeScript enums for error codes to improve type safety and documentation
- Group error codes by category (e.g., validation, authorization, data integrity)
- Add JSDoc comments to document the error code mapping
Here's a suggested refactor:
+/** Error codes for vehicle mileage operations */
+enum VehicleMileageErrorCode {
+ /** Special case: -1 is transformed to 999 due to indexing limitations */
+ TOO_MANY_PERMNO = 999,
+ MISSING_PERMNO = 3,
+ // ... other codes
+}
+/**
+ * Maps error codes to their corresponding message descriptors.
+ * Note: Error code -1 is transformed to 999 for indexing purposes.
+ */
export const errorCodeMessageMap: Record<number, MessageDescriptor> = {
- 999: m.tooManyPermno, //returned error code is -1, which cant be used to index. Transform to 999
+ [VehicleMileageErrorCode.TOO_MANY_PERMNO]: m.tooManyPermno,
+ [VehicleMileageErrorCode.MISSING_PERMNO]: m.missingPermno,
// ... rest of the mapping
}Let's verify that all message keys exist in the messages file:
libs/api/domains/vehicles/project.json (1)
18-22: LGTM! Let's verify the referenced paths exist.
The new extract-strings target is well-structured and follows nx conventions.
Let's verify the referenced paths and files exist:
✅ Verification successful
All referenced paths exist and are valid ✅
All paths referenced in the extract-strings target have been verified:
libs/api/domains/vehicles/src/lib/messages.tsexistslibs/localization/tsconfig.lib.jsonexistslibs/localization/scripts/extract.tsexists
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all referenced paths in the extract-strings target exist
# Check if the messages.ts file exists
echo "Checking messages.ts..."
fd "messages.ts" libs/api/domains/vehicles/src/lib
# Check if the localization tsconfig exists
echo "Checking localization tsconfig..."
fd "tsconfig.lib.json" libs/localization
# Check if the extraction script exists
echo "Checking extraction script..."
fd "extract" libs/localization/scripts
Length of output: 494
libs/api/domains/vehicles/src/lib/vehicles.module.ts (1)
14-14: LGTM! Clean integration of CmsTranslationsModule
The addition of CmsTranslationsModule follows NestJS best practices and aligns with the PR's objective to improve error handling with proper internationalization support.
Also applies to: 30-30
libs/api/domains/vehicles/src/lib/messages.ts (2)
1-3: LGTM! Clean and minimal import/export structure.
The file follows best practices with a single import and a named export.
1-83: Let me try one more search with a corrected pattern to find message usage.
#!/bin/bash
# Search for any usage of the message IDs
echo "Searching for message ID usage:"
rg "api\.bulk-vehicle-mileage:" "libs/api/domains/vehicles/src" -B 1 -A 1
# Check imports of the messages file
echo -e "\nChecking imports of messages:"
rg "from.*messages" "libs/api/domains/vehicles/src"
# List all TypeScript/JavaScript files in the domain
echo -e "\nAll potential files that could use messages:"
fd "\.(ts|js)$" "libs/api/domains/vehicles/src"libs/service-portal/assets/src/utils/parseFileToMileage.ts (1)
66-66:
Review number sanitization logic
The current sanitization removes all dots and commas, which might lead to incorrect number parsing in some locales. Consider using a more robust number parsing approach.
Consider using a more robust approach:
-const mileage = Number(sanitizeNumber(row[mileageIndex]))
+const mileage = parseFloat(row[mileageIndex].replace(',', '.'))libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileage.tsx (2)
8-8: LGTM! Import follows best practices.
The import of IntroWrapper from the shared core library promotes reusability and follows effective tree-shaking practices.
Line range hint 65-87: LGTM! Component structure follows best practices.
The IntroWrapper implementation:
- Maintains proper TypeScript typing
- Follows component reusability guidelines
- Properly handles internationalization
libs/service-portal/assets/src/screens/VehicleBulkMileageJobOverview/VehicleBulkMileageJobOverview.tsx (2)
10-10: LGTM! Good practices for imports
The import of IntroWrapper follows good practices for tree-shaking and component reusability across the service portal.
73-78: LGTM! Good component structure
The replacement of Box with IntroWrapper:
- Improves consistency across the service portal
- Provides better type safety through proper prop typing
- Enhances component reusability
Also applies to: 147-147
libs/api/domains/vehicles/src/lib/services/bulkMileage.service.ts (3)
10-10: LGTM: Import changes are appropriate
The new imports support the internationalization requirements and follow TypeScript best practices.
Also applies to: 18-20
27-27: LGTM: Constructor injection follows best practices
The IntlService is properly injected as a private readonly dependency, following Angular/NestJS patterns.
135-137: Verify all callers handle the new required locale parameter
The method signature change is a breaking change that requires all callers to provide a locale parameter.
Let's verify all callers are updated:
✅ Verification successful
Breaking change is properly handled in all callers
The method signature change is properly handled in the only caller found in bulkMileage.resolver.ts. The resolver already passes the locale parameter from the input object (input.locale), which indicates the breaking change was correctly implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to getBulkMileageRegistrationRequestOverview
ast-grep --pattern 'getBulkMileageRegistrationRequestOverview($$$)'
Length of output: 69
Script:
#!/bin/bash
# Search for method calls using ripgrep for better coverage
rg -l "getBulkMileageRegistrationRequestOverview"
# Also search for the class/method definition to understand the context
ast-grep --pattern 'getBulkMileageRegistrationRequestOverview'
Length of output: 246
Script:
#!/bin/bash
# Get the actual implementation and usage context
rg "getBulkMileageRegistrationRequestOverview" -A 5 -B 5 libs/api/domains/vehicles/src/lib/resolvers/bulkMileage.resolver.ts libs/api/domains/vehicles/src/lib/services/bulkMileage.service.ts
Length of output: 2373
libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx (2)
130-140: LGTM: IntroWrapper implementation
The replacement of IntroHeader with IntroWrapper is well-implemented with proper props for service provider information.
Also applies to: 203-203
13-24: Verify component dependencies for reusability
Since this component is in the libs directory, ensure all imported dependencies are appropriate for reuse across different NextJS apps.
✅ Verification successful
All imports follow reusability guidelines
The component's dependencies are properly scoped and follow the reusability guidelines:
- Uses standard React imports (
react,react-dropzone) - Uses shared
@island.ispackages - Uses relative imports from within the same library
- No app-specific or external dependencies that would limit reusability
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any app-specific imports that might limit reusability
rg --type typescript "from ['\"]((?!@island\.is/|react).)*['\""]" libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/
Length of output: 93
Script:
#!/bin/bash
# Check for any app-specific imports that might limit reusability
rg --type typescript "from ['\"](?!@island\.is|react|\.)[^'\"]*['\"]" libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/
Length of output: 398
Script:
#!/bin/bash
# List all imports in the file to verify dependencies
rg --type typescript "^import.*from" libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx
Length of output: 166
Script:
#!/bin/bash
# List all imports in the file to verify dependencies
rg "^import.*from" libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx
Length of output: 761
libs/service-portal/assets/src/screens/VehicleBulkMileageJobDetail/VehicleBulkMileageJobDetail.tsx (4)
16-16: LGTM: Import of shared component
The import of IntroWrapper from the core package follows the coding guidelines for component reusability across NextJS apps.
253-255: LGTM: Consistent error handling
The error display implementation includes proper null checking and consistently uses the new warningText field, aligning with the error handling improvements.
93-93: Verify the availability of warningText field
The switch from message to warningText aligns with the error handling improvements. Let's verify that this field is properly defined in the GraphQL schema and that there are no cases where it might be undefined.
#!/bin/bash
# Search for the GraphQL type definition
rg -A 5 "type VehiclesBulkMileageRegistrationRequestError"37-37: Verify the impact of the new required locale parameter
The addition of the locale parameter to the query is a good practice for internationalization. However, we should verify that all other components using this query have been updated accordingly.
Also applies to: 60-60
libs/service-portal/assets/src/screens/VehicleBulkMileage/VehicleBulkMileageRow.tsx (2)
11-11: LGTM: Clean import addition
The useMemo import is correctly grouped with other React hooks.
400-400: LGTM: Clean prop update
The memoized table data is correctly passed to the NestedFullTable component.
libs/api/domains/vehicles/src/lib/services/vehicles.service.ts (2)
36-36: LGTM: Import cleanup aligns with data model changes
The removal of unused VehicleMileageDetail import while retaining VehicleMileageOverview reflects the data model simplification mentioned in the summary.
Line range hint 1-650: Compliant with coding guidelines
The service implementation follows the guidelines for libs/**/*:
- Uses TypeScript with proper type definitions
- Follows modular design for reusability
- Implements consistent error handling patterns
libs/clients/vehicles-mileage/src/clientConfig.json (4)
12-12: LGTM: Base URL path clearly defined.
The new servers section properly specifies the base URL path for vehicle mileage reading endpoints.
795-799: Enhanced error reporting with warning serial tracking.
The addition of the warningSerial property to BulkMileageReadingRequestDetailErrors improves error tracking capabilities. This is a good enhancement for better error handling and debugging.
✅ Verification successful
Warning serial implementation aligns with existing error handling patterns
The addition of warningSerial in the vehicles-mileage client config is consistent with the existing error handling patterns across the codebase. The implementation is used similarly in:
- Transport authority services (vehicle operators, plate ordering, plate renewal)
- Vehicle owner change service
- Bulk mileage processing
The field is properly integrated into the error handling flow, particularly in bulkMileage.service.ts where it's used for error code mapping and message formatting.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing warning serial implementations
ast-grep --pattern 'warningSerial'
Length of output: 492
Script:
#!/bin/bash
# Check for warning serial implementations with better context
rg "warningSerial" -A 3 -B 3
Length of output: 12922
5-5: Verify API version compatibility with existing clients.
The API version has been updated from Release-6 to Release-21, which is a significant version jump. This might indicate breaking changes that need to be communicated to API consumers.
247-254:
Breaking Change: PUT endpoint response structure modified.
The PUT endpoint response has undergone two significant changes:
- Status code changed from 201 to 200
- Response schema changed from single object to array of MileageReadingDto
This is a breaking change that requires client updates.
libs/service-portal/assets/src/lib/messages.ts (4)
221-225: LGTM! Clear and helpful error message.
The error message clearly specifies the accepted file types (.xlsx and .csv) which helps users understand and fix the issue.
989-992: LGTM! Concise error message.
The error message clearly communicates that the uploaded file is empty.
1001-1005: LGTM! Comprehensive error message.
The error message is helpful as it:
- Clearly states the issue (missing/incorrect column)
- Lists all accepted column names
- Helps users fix the problem
1006-1010: LGTM! Comprehensive error message.
The error message is helpful as it:
- Clearly states the issue (missing/incorrect column)
- Lists all accepted column names
- Helps users fix the problem
libs/api/domains/vehicles/src/lib/services/bulkMileage.service.ts
Outdated
Show resolved
Hide resolved
libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx
Outdated
Show resolved
Hide resolved
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
128-201: Consider extracting AlertMessage logic for better readability.The success message UI contains nested conditions and complex rendering logic. Consider extracting this into a separate component or function for better maintainability.
Example refactor:
const SuccessAlert = ({ requestGuid, data, loading, error }: { requestGuid: string | null data: any loading: boolean error: any }) => { const { formatMessage } = useLocale() if (!requestGuid || data?.vehicleBulkMileagePost?.errorMessage || loading || error) { return null } return ( <AlertMessage type="success" title={formatMessage(vehicleMessage.uploadSuccess)} message={ <Box> <Text variant="small"> {formatMessage(vehicleMessage.bulkMileageUploadStatus)} </Text> <Box marginTop="smallGutter"> <LinkButton to={AssetsPaths.AssetsVehiclesBulkMileageJobDetail.replace( ':id', data?.vehicleBulkMileagePost?.requestId ?? '', )} text={formatMessage(vehicleMessage.openJob)} variant="text" icon="arrowForward" /> </Box> </Box> } /> ) }Then use it in the render:
<Stack space={2}> {error && <Problem error={error} noBorder={false} />} {/* ... other alerts ... */} <SuccessAlert requestGuid={requestGuid} data={data} loading={loading} error={error} /> <InputFileUpload {/* ... props ... */} /> </Stack>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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 (2)
libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx (2)
13-24: LGTM! Imports follow TypeScript best practices.
The imports are well-organized and properly typed, following the coding guidelines for library components.
93-99: LGTM! Error handling improvements.
The function has been improved by:
- Removing debug console.log statements (addressing previous review comments)
- Using proper error handling with internationalized messages
libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx
Outdated
Show resolved
Hide resolved
* fix: display latest reg * feat: add error code map * feat: add errors * fix: csv parse * fix: jobs sorting * fix: remove strip * fix: translations * fix: add sanitization on number * fix: sanitization * fix: add secret from aws paramter store * fix: sanitize bit less powerfully * fix: more handling * Update libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: coderabbit suggestions --------- Co-authored-by: Ásdís Erna Guðmundsdóttir <disa@hugsmidjan.is> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
* feat(bulk-mileage): update errors (#16720) * fix: display latest reg * feat: add error code map * feat: add errors * fix: csv parse * fix: jobs sorting * fix: remove strip * fix: translations * fix: add sanitization on number * fix: sanitization * fix: add secret from aws paramter store * fix: sanitize bit less powerfully * fix: more handling * Update libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: coderabbit suggestions --------- Co-authored-by: Ásdís Erna Guðmundsdóttir <disa@hugsmidjan.is> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix: remove intro wrapper * fix: remove import --------- Co-authored-by: Ásdís Erna Guðmundsdóttir <disa@hugsmidjan.is> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
* fix: display latest reg * feat: add error code map * feat: add errors * fix: csv parse * fix: jobs sorting * fix: remove strip * fix: translations * fix: add sanitization on number * fix: sanitization * fix: add secret from aws paramter store * fix: sanitize bit less powerfully * fix: more handling * Update libs/service-portal/assets/src/screens/VehicleBulkMileageUpload/VehicleBulkMileageUpload.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: coderabbit suggestions --------- Co-authored-by: Ásdís Erna Guðmundsdóttir <disa@hugsmidjan.is> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
What
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
localeproperty to support internationalization in vehicle mileage requests.Bug Fixes
Chores
Refactor